Re: [PATCH RFC v1] spi: realtek-rtl: Fix clearing some register bits

[Sander Vanheule]

On Wed, 2022-07-27 at 21:13 +0200, Martin Blumenstingl wrote:
> Hi Sander,
> 
> On Tue, Jul 26, 2022 at 11:03 AM Sander Vanheule <sander@xxxxxxxxxxxxx> wrote:
> [...]
> > >         value = readl(REG(RTL_SPI_SFCSR));
> > > -       value &= RTL_SPI_SFCSR_LEN_MASK;
> > > +       value &= ~RTL_SPI_SFCSR_LEN_MASK;
> > 
> > Although typically a field mask has the only the bits of that field set,
> > RTL_SPI_SFCSR_LEN_MASK is already inverted. So LEN_MASK has all bits set,
> > *except* for those where LEN is stored.
> > 
> > This means the code currently is:
> >         value &= ~(0x3 << 28);
> > 
> > which is correct AFAICT, as it clears the LEN bits, but keeps all the others.
> Thank you for this hint! I completely missed that when reading the
> definition of the macro.
> 
> > While this part is currently not wrong, I wouldn't be opposed to a patch to make
> > it less confusing by not inverting the field mask in the definition of
> > RTL_SPI_SFCSR_LEN_MASK.
> I can re-spin this patch and move the ~ operator where most people
> expect it to be.
> 
> > >         if (size == 4)
> > >                 value |= RTL_SPI_SFCSR_LEN4;
> > >         else if (size == 1)
> > > @@ -143,7 +143,7 @@ static void init_hw(struct rtspi *rtspi)
> > >         /* Permanently disable CS1, since it's never used */
> > >         value |= RTL_SPI_SFCSR_CSB1;
> > >         /* Select CS0 for use */
> > > -       value &= RTL_SPI_SFCSR_CS;
> > > +       value &= ~RTL_SPI_SFCSR_CS;
> > 
> > This macro is not inverted, so it does clear any previously set bits, and
> > probably doesn't end up with RTL_SPI_SFCRS_CS set. However, is in an init call
> > and it doesn't appear to cause any issues later on, right? Is this because the
> > SFCSR register is (unintentionally) cleared and that is actually required?
> I'm not sure what's right or wrong here but the code reads strange:
>     value = readl(...);
>     value |= BIT(30); /* RTL_SPI_SFCSR_CSB1 */
>     value &= BIT(24); /* RTL_SPI_SFCSR_CS */
> What's the point in setting RTL_SPI_SFCSR_CSB1 (bit 30) when it's
> immediately cleared in the next operation?
> 
> Also any bits read from the register except RTL_SPI_SFCSR_CS (bit 24)
> are cleared - why even bother reading that register then?

I agree that this is rather suspicious code, to say the least. However, I have not been involved in
the development of this driver until now, so I would also have to look into the original code to
decipher the required behaviour.

> 
> If you have any advice on how to change this code then I'm happy to do so.
> Otherwise I'd leave it as is, especially since I cannot test this in any way.

Since I have multiple devices available for testing, I could try out patches. You can look into this
issue if you want, but you don't have to spend time on this if you don't have any to spare.

If you (or anyone) would like to investigate more and require a reference, you can use e.g. the GPL
code archive for the TP-Link T1600G-52PS v4:
https://static.tp-link.com/resources/gpl/t1600g-52ps-v4_gpl.tar.gz

Some likely useful files under /t1600g-52ps-v4_gpl/ldk_realtek/realtek-V2.1.6.pre2 would be:
 * u-boot-2011.12/board/Realtek/rtl838x/flash_spi.c
 * sdk/system/linux/linux-2.6.32.x/drivers/mtd/maps/rtk-*.c

Best,
Sander