On Fri, 29 May 2020 at 19:00, Jean-Christophe Dubois
<[email protected]> wrote:
>
> Some bits of the CCM registers are non writable.
>
> This was left undone in the initial commit (all bits of registers were
> writable).
>
> This patch add the required code to protect non writable bits.
>
> Signed-off-by: Jean-Christophe Dubois <[email protected]>
> static uint64_t imx6ul_analog_read(void *opaque, hwaddr offset, unsigned
> size)
> @@ -737,7 +790,8 @@ static void imx6ul_analog_write(void *opaque, hwaddr
> offset, uint64_t value,
> * the REG_NAME register. So we change the value of the
> * REG_NAME register, setting bits passed in the value.
> */
> - s->analog[index - 1] |= value;
> + s->analog[index - 1] = s->analog[index - 1] |
> + (value & ~analog_mask[index - 1]);
Not sure why you didn't retain the use of the |= operator here?
> break;
> case CCM_ANALOG_PLL_ARM_CLR:
> case CCM_ANALOG_PLL_USB1_CLR:
> @@ -762,7 +816,8 @@ static void imx6ul_analog_write(void *opaque, hwaddr
> offset, uint64_t value,
> * the REG_NAME register. So we change the value of the
> * REG_NAME register, unsetting bits passed in the value.
> */
> - s->analog[index - 2] &= ~value;
> + s->analog[index - 2] = s->analog[index - 2] &
> + ~(value & ~analog_mask[index - 2]);
Similarly here with &=.
> break;
> case CCM_ANALOG_PLL_ARM_TOG:
> case CCM_ANALOG_PLL_USB1_TOG:
> @@ -787,14 +842,14 @@ static void imx6ul_analog_write(void *opaque, hwaddr
> offset, uint64_t value,
> * the REG_NAME register. So we change the value of the
> * REG_NAME register, toggling bits passed in the value.
> */
> - s->analog[index - 3] ^= value;
> + s->analog[index - 3] = (s->analog[index - 3] &
> + analog_mask[index - 3]) |
> + ((value ^ s->analog[index - 3]) &
> + ~analog_mask[index - 3]);
I think this does the right thing (toggle bits which are set in
value as long as they're not read-only), but isn't this a simpler
way to write it?
s->analog[index - 3] ^= (value & ~analog_mask[index - 3]);
That is, we toggle the bits that are set in 'value' and not set
in the mask of read-only bits.
> break;
> default:
> - /*
> - * We will do a better implementation later. In particular some bits
> - * cannot be written to.
> - */
> - s->analog[index] = value;
> + s->analog[index] = (s->analog[index] & analog_mask[index]) |
> + (value & ~analog_mask[index]);
> break;
> }
> }
thanks
-- PMM
