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@xxxxxxxxxxxxxx>
> ---
> 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));
>  }
>