Re: [PATCH] hw/misc/imx6ul_ccm.c: Implement non writable bits in CCM registers

[Peter Maydell]

On Fri, 29 May 2020 at 19:00, Jean-Christophe Dubois
<jcd@tribudubois.net> 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 <jcd@tribudubois.net>
>  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