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

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

[Martin Blumenstingl]

The code seemingly tries to clear RTL_SPI_SFCSR_LEN_MASK (before then
setting either RTL_SPI_SFCSR_LEN1 or RTL_SPI_SFCSR_LEN4) and
RTL_SPI_SFCSR_CS. What it actually does is only keeping these bits and
clearing all other bits, even the ones which were just set before. Fix
the operation to clear the bits in the selected mask and keep all other
ones.

Fixes: a8af5cc2ff1e80 ("spi: realtek-rtl: Add support for Realtek RTL838x/RTL839x SPI controllers")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
I stumbled across this while reading the driver. This patch is untested
because I don't have any hardware with this controller.


 drivers/spi/spi-realtek-rtl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-realtek-rtl.c b/drivers/spi/spi-realtek-rtl.c
index 866b0477dbd7..e5ad0f11996f 100644
--- a/drivers/spi/spi-realtek-rtl.c
+++ b/drivers/spi/spi-realtek-rtl.c
@@ -49,7 +49,7 @@ static void set_size(struct rtspi *rtspi, int size)
 	u32 value;
 
 	value = readl(REG(RTL_SPI_SFCSR));
-	value &= RTL_SPI_SFCSR_LEN_MASK;
+	value &= ~RTL_SPI_SFCSR_LEN_MASK;
 	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;
 	writel(value, REG(RTL_SPI_SFCSR));
 }
 
-- 
2.37.1

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

[Birger Koblitz]

Hi Martin,

On 25/07/2022 21:35, Martin Blumenstingl wrote:
> The code seemingly tries to clear RTL_SPI_SFCSR_LEN_MASK (before then
> setting either RTL_SPI_SFCSR_LEN1 or RTL_SPI_SFCSR_LEN4) and
> RTL_SPI_SFCSR_CS. What it actually does is only keeping these bits and
> clearing all other bits, even the ones which were just set before. Fix
> the operation to clear the bits in the selected mask and keep all other
> ones.
> 
> Fixes: a8af5cc2ff1e80 ("spi: realtek-rtl: Add support for Realtek RTL838x/RTL839x SPI controllers")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> I stumbled across this while reading the driver. This patch is untested
> because I don't have any hardware with this controller.
I believe your fix is correct. In the meantime, more information has been
learned about this hardware, in particular, newer SoC versions and the possibility
to have parallel IO and hardware which uses different chip selects.
I came up with the following patch for supporting this, and it achieves
what you also propose:
https://github.com/bkobl/openwrt/blob/rtl8214qf_merge/target/linux/realtek/patches-5.10/317-spi-cs-support-for-spi-realtek-rtl.patch
It is still is a bit rough, reading it I immediately saw 2 things that
would need to be fixed, but it also improves e.g. that RTL_SPI_SFCSR
is now configured independently of what u-boot did to it.

Cheers,
   Birger

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

[Martin Blumenstingl]

Hi Birger,

(sorry for replying late, I missed your mail)

On Mon, Jul 25, 2022 at 10:19 PM Birger Koblitz <mail@birger-koblitz.de> wrote:
[...]
> > I stumbled across this while reading the driver. This patch is untested
> > because I don't have any hardware with this controller.
> I believe your fix is correct. In the meantime, more information has been
> learned about this hardware, in particular, newer SoC versions and the possibility
> to have parallel IO and hardware which uses different chip selects.
As Sander pointed out (thanks again!) there's actually an issue with my patch.
I just sent v2 with a fix for RTL_SPI_SFCSR_LEN_MASK.

> I came up with the following patch for supporting this, and it achieves
> what you also propose:
> https://github.com/bkobl/openwrt/blob/rtl8214qf_merge/target/linux/realtek/patches-5.10/317-spi-cs-support-for-spi-realtek-rtl.patch
> It is still is a bit rough, reading it I immediately saw 2 things that
> would need to be fixed, but it also improves e.g. that RTL_SPI_SFCSR
> is now configured independently of what u-boot did to it.
Your patch actually addresses an issue which I have seen with RTL_SPI_SFCSR_CS.
Since you seem to have boards with these Realtek SoCs: could you
please clean up your patch and upstream it (splitting into smaller
patches if/where needed)? That would be a win-win: upstream gains
improved SPI support and I won't be confused the next time I look at
the spi-realtek-rtl driver.


Thank you!
Best regards,
Martin

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

[Birger Koblitz]

Hi Martin,

On 7/28/22 17:27, Martin Blumenstingl wrote:
> Your patch actually addresses an issue which I have seen with RTL_SPI_SFCSR_CS.
> Since you seem to have boards with these Realtek SoCs: could you
> please clean up your patch and upstream it (splitting into smaller
> patches if/where needed)? That would be a win-win: upstream gains
> improved SPI support and I won't be confused the next time I look at
> the spi-realtek-rtl driver.

Thanks for suggesting this! I will send a patch-set later today along 
these lines, I first need to set up a linux dev environment as the 
development so far was within OpenWRT.

Cheers,
   Birger

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

[Martin Blumenstingl]

Hi Birger,

On Fri, Jul 29, 2022 at 10:20 AM Birger Koblitz <mail@birger-koblitz.de> wrote:
>
> Hi Martin,
>
> On 7/28/22 17:27, Martin Blumenstingl wrote:
> > Your patch actually addresses an issue which I have seen with RTL_SPI_SFCSR_CS.
> > Since you seem to have boards with these Realtek SoCs: could you
> > please clean up your patch and upstream it (splitting into smaller
> > patches if/where needed)? That would be a win-win: upstream gains
> > improved SPI support and I won't be confused the next time I look at
> > the spi-realtek-rtl driver.
>
> Thanks for suggesting this! I will send a patch-set later today along
> these lines, I first need to set up a linux dev environment as the
> development so far was within OpenWRT.
There's not a single right or wrong. The fewer patches a target has in
OpenWrt the easier it gets in my experience. The most simple approach
in my opinion is to make OpenWrt use an external kernel tree (using
CONFIG_EXTERNAL_KERNEL_TREE).


Best regards,
Martin

Subject: [PATCH 0/7] new SoC support and further improvements

[Birger Koblitz]

Hi Martin,

Thanks for the tip on the build environment, so here it comes:
This adds support for the RTL93xx newer generation of Switch SoCs
from Realtek. It adds device flags to describe the difference in
capabilities with respect to the RTL83xx Socs, support for more
than CS#0, parallel IO and SPI bus frequency setting.

Birger Koblitz (7):
   spi: realteak-rtl: Add support for RTL93xx SoCs
   spi: realtek-rtl: set device capability flags
   spi: realtek-rtl: allow use of chip-select other than CS0
   spi: realtek-rtl: add parallel IO suppport
   spi: realtek-rtl: set transfer mode in transfer_one_message()
   spi: realtek-rtl: add support to configure bus speed
   spi: realtek-rtl: update devicetree documentation

  .../bindings/spi/realtek,rtl-spi.yaml         |  10 +
  drivers/spi/spi-realtek-rtl.c                 | 196 ++++++++++++++----
  2 files changed, 162 insertions(+), 44 deletions(-)

-- 
2.25.1

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

[Sander Vanheule]

Hi Martin,

On Mon, 2022-07-25 at 21:35 +0200, Martin Blumenstingl wrote:
> The code seemingly tries to clear RTL_SPI_SFCSR_LEN_MASK (before then
> setting either RTL_SPI_SFCSR_LEN1 or RTL_SPI_SFCSR_LEN4) and
> RTL_SPI_SFCSR_CS. What it actually does is only keeping these bits and
> clearing all other bits, even the ones which were just set before. Fix
> the operation to clear the bits in the selected mask and keep all other
> ones.
> 
> Fixes: a8af5cc2ff1e80 ("spi: realtek-rtl: Add support for Realtek
> RTL838x/RTL839x SPI controllers")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> I stumbled across this while reading the driver. This patch is untested
> because I don't have any hardware with this controller.
> 
> 
>  drivers/spi/spi-realtek-rtl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-realtek-rtl.c b/drivers/spi/spi-realtek-rtl.c
> index 866b0477dbd7..e5ad0f11996f 100644
> --- a/drivers/spi/spi-realtek-rtl.c
> +++ b/drivers/spi/spi-realtek-rtl.c
> @@ -49,7 +49,7 @@ static void set_size(struct rtspi *rtspi, int size)
>         u32 value;
>  
>         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.

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.

>         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?

Best,
Sander

>         writel(value, REG(RTL_SPI_SFCSR));
>  }
>  

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

[Martin Blumenstingl]

Hi Sander,

On Tue, Jul 26, 2022 at 11:03 AM Sander Vanheule <sander@svanheule.net> 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?

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.


Best regards,
Martin

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@svanheule.net> 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