[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/misc/imx6ul_ccm.c: Implement non writable bits in CCM reg
From: |
Peter Maydell |
Subject: |
Re: [PATCH] hw/misc/imx6ul_ccm.c: Implement non writable bits in CCM registers |
Date: |
Fri, 5 Jun 2020 15:49:53 +0100 |
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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH] hw/misc/imx6ul_ccm.c: Implement non writable bits in CCM registers,
Peter Maydell <=