[v1] spi: spi-fsl-lpspi: Generic fixes and support for S32G devices

[PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

Posted by James Clark 5 months, 4 weeks ago

From: Larisa Grigore <larisa.grigore@nxp.com>

The driver currently supports multiple chip-selects, but only sets the
polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
the desired chip-select.

Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-lpspi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index d44a23f7d6c1..c65eb6d31ee7 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -70,7 +70,7 @@
 #define DER_TDDE	BIT(0)
 #define CFGR1_PCSCFG	BIT(27)
 #define CFGR1_PINCFG	(BIT(24)|BIT(25))
-#define CFGR1_PCSPOL	BIT(8)
+#define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
 #define CFGR1_NOSTALL	BIT(3)
 #define CFGR1_HOST	BIT(0)
 #define FSR_TXCOUNT	(0xFF)
@@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
 	else
 		temp = CFGR1_PINCFG;
 	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
-		temp |= CFGR1_PCSPOL;
+		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
+				   BIT(fsl_lpspi->config.chip_select));
+
 	writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
 
 	temp = readl(fsl_lpspi->base + IMX7ULP_CR);

-- 
2.34.1

Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

Posted by kernel test robot 5 months, 4 weeks ago

Hi James,

kernel test robot noticed the following build errors:

[auto build test ERROR on 0cc53520e68bea7fb80fdc6bdf8d226d1b6a98d9]

url:    https://github.com/intel-lab-lkp/linux/commits/James-Clark/spi-spi-fsl-lpspi-Fix-transmissions-when-using-CONT/20250815-001554
base:   0cc53520e68bea7fb80fdc6bdf8d226d1b6a98d9
patch link:    https://lore.kernel.org/r/20250814-james-nxp-lpspi-v1-2-9586d7815d14%40linaro.org
patch subject: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit
config: csky-randconfig-002-20250815 (https://download.01.org/0day-ci/archive/20250815/202508151101.7lDGTaxi-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250815/202508151101.7lDGTaxi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508151101.7lDGTaxi-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/spi/spi-fsl-lpspi.c: In function 'fsl_lpspi_config':
>> drivers/spi/spi-fsl-lpspi.c:428:25: error: implicit declaration of function 'FIELD_PREP' [-Wimplicit-function-declaration]
     428 |                 temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
         |                         ^~~~~~~~~~


vim +/FIELD_PREP +428 drivers/spi/spi-fsl-lpspi.c

   409	
   410	static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
   411	{
   412		u32 temp;
   413		int ret;
   414	
   415		if (!fsl_lpspi->is_target) {
   416			ret = fsl_lpspi_set_bitrate(fsl_lpspi);
   417			if (ret)
   418				return ret;
   419		}
   420	
   421		fsl_lpspi_set_watermark(fsl_lpspi);
   422	
   423		if (!fsl_lpspi->is_target)
   424			temp = CFGR1_HOST;
   425		else
   426			temp = CFGR1_PINCFG;
   427		if (fsl_lpspi->config.mode & SPI_CS_HIGH)
 > 428			temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
   429					   BIT(fsl_lpspi->config.chip_select));
   430	
   431		writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
   432	
   433		temp = readl(fsl_lpspi->base + IMX7ULP_CR);
   434		temp |= CR_RRF | CR_RTF | CR_MEN;
   435		writel(temp, fsl_lpspi->base + IMX7ULP_CR);
   436	
   437		temp = 0;
   438		if (fsl_lpspi->usedma)
   439			temp = DER_TDDE | DER_RDDE;
   440		writel(temp, fsl_lpspi->base + IMX7ULP_DER);
   441	
   442		return 0;
   443	}
   444	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

Posted by Frank Li 5 months, 4 weeks ago

On Thu, Aug 14, 2025 at 05:06:42PM +0100, James Clark wrote:
> From: Larisa Grigore <larisa.grigore@nxp.com>
>
> The driver currently supports multiple chip-selects, but only sets the
> polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
> the desired chip-select.
>
> Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-lpspi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> index d44a23f7d6c1..c65eb6d31ee7 100644
> --- a/drivers/spi/spi-fsl-lpspi.c
> +++ b/drivers/spi/spi-fsl-lpspi.c
> @@ -70,7 +70,7 @@
>  #define DER_TDDE	BIT(0)
>  #define CFGR1_PCSCFG	BIT(27)
>  #define CFGR1_PINCFG	(BIT(24)|BIT(25))
> -#define CFGR1_PCSPOL	BIT(8)
> +#define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
>  #define CFGR1_NOSTALL	BIT(3)
>  #define CFGR1_HOST	BIT(0)
>  #define FSR_TXCOUNT	(0xFF)
> @@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
>  	else
>  		temp = CFGR1_PINCFG;
>  	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
> -		temp |= CFGR1_PCSPOL;
> +		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
> +				   BIT(fsl_lpspi->config.chip_select));
> +

Feel like FILED_PREP(..., BIT()) is stranged.

I suggest #define CFGR1_PCSPOL(x) BIT((x) + 8)

Frank

>  	writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
>
>  	temp = readl(fsl_lpspi->base + IMX7ULP_CR);
>
> --
> 2.34.1
>

Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

Posted by James Clark 5 months, 3 weeks ago

On 14/08/2025 5:49 pm, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:42PM +0100, James Clark wrote:
>> From: Larisa Grigore <larisa.grigore@nxp.com>
>>
>> The driver currently supports multiple chip-selects, but only sets the
>> polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
>> the desired chip-select.
>>
>> Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-lpspi.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
>> index d44a23f7d6c1..c65eb6d31ee7 100644
>> --- a/drivers/spi/spi-fsl-lpspi.c
>> +++ b/drivers/spi/spi-fsl-lpspi.c
>> @@ -70,7 +70,7 @@
>>   #define DER_TDDE	BIT(0)
>>   #define CFGR1_PCSCFG	BIT(27)
>>   #define CFGR1_PINCFG	(BIT(24)|BIT(25))
>> -#define CFGR1_PCSPOL	BIT(8)
>> +#define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
>>   #define CFGR1_NOSTALL	BIT(3)
>>   #define CFGR1_HOST	BIT(0)
>>   #define FSR_TXCOUNT	(0xFF)
>> @@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
>>   	else
>>   		temp = CFGR1_PINCFG;
>>   	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
>> -		temp |= CFGR1_PCSPOL;
>> +		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
>> +				   BIT(fsl_lpspi->config.chip_select));
>> +
> 
> Feel like FILED_PREP(..., BIT()) is stranged.
> 
> I suggest #define CFGR1_PCSPOL(x) BIT((x) + 8)
> 
> Frank

It's using an existing macro that everyone knows though and I found 65 
instances of exactly this. It can be read as "set bit X and put it into 
the PCSPOL field without any further investigation.

If we make a new macro, first the reader will have to jump to it, then 
it still doesn't immediately explain what the "+ 8" part is. Using 
FIELD_PREP() also has the potential to use autogenerated field masks 
from a machine readable version of the reference manual. You can't 
statically check your macro to see if + 8 is correct or not, and it also 
doesn't catch overflow errors like FIELD_PREP() does.

There might be an argument to add a new global macro like 
FIELD_BIT(mask, bit). But it's not very flexible (can't set multiple 
bits) and you can already accomplish the same thing by adding BIT() to 
the existing one.

Thanks
James

> 
>>   	writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
>>
>>   	temp = readl(fsl_lpspi->base + IMX7ULP_CR);
>>
>> --
>> 2.34.1
>>

Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

Posted by Frank Li 5 months, 3 weeks ago

On Mon, Aug 18, 2025 at 02:05:16PM +0100, James Clark wrote:
>
>
> On 14/08/2025 5:49 pm, Frank Li wrote:
> > On Thu, Aug 14, 2025 at 05:06:42PM +0100, James Clark wrote:
> > > From: Larisa Grigore <larisa.grigore@nxp.com>
> > >
> > > The driver currently supports multiple chip-selects, but only sets the
> > > polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
> > > the desired chip-select.
> > >
> > > Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
> > > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> > > Signed-off-by: James Clark <james.clark@linaro.org>
> > > ---
> > >   drivers/spi/spi-fsl-lpspi.c | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> > > index d44a23f7d6c1..c65eb6d31ee7 100644
> > > --- a/drivers/spi/spi-fsl-lpspi.c
> > > +++ b/drivers/spi/spi-fsl-lpspi.c
> > > @@ -70,7 +70,7 @@
> > >   #define DER_TDDE	BIT(0)
> > >   #define CFGR1_PCSCFG	BIT(27)
> > >   #define CFGR1_PINCFG	(BIT(24)|BIT(25))
> > > -#define CFGR1_PCSPOL	BIT(8)
> > > +#define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
> > >   #define CFGR1_NOSTALL	BIT(3)
> > >   #define CFGR1_HOST	BIT(0)
> > >   #define FSR_TXCOUNT	(0xFF)
> > > @@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
> > >   	else
> > >   		temp = CFGR1_PINCFG;
> > >   	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
> > > -		temp |= CFGR1_PCSPOL;
> > > +		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
> > > +				   BIT(fsl_lpspi->config.chip_select));
> > > +
> >
> > Feel like FILED_PREP(..., BIT()) is stranged.
> >
> > I suggest #define CFGR1_PCSPOL(x) BIT((x) + 8)
> >
> > Frank
>
> It's using an existing macro that everyone knows though and I found 65
> instances of exactly this. It can be read as "set bit X and put it into the
> PCSPOL field without any further investigation.

Where have such pattern in kernel?

Frank

>
> If we make a new macro, first the reader will have to jump to it, then it
> still doesn't immediately explain what the "+ 8" part is. Using FIELD_PREP()
> also has the potential to use autogenerated field masks from a machine
> readable version of the reference manual. You can't statically check your
> macro to see if + 8 is correct or not, and it also doesn't catch overflow
> errors like FIELD_PREP() does.
>
> There might be an argument to add a new global macro like FIELD_BIT(mask,
> bit). But it's not very flexible (can't set multiple bits) and you can
> already accomplish the same thing by adding BIT() to the existing one.
>
> Thanks
> James
>
> >
> > >   	writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
> > >
> > >   	temp = readl(fsl_lpspi->base + IMX7ULP_CR);
> > >
> > > --
> > > 2.34.1
> > >
>

Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

Posted by James Clark 5 months, 3 weeks ago


On 18/08/2025 4:19 pm, Frank Li wrote:
> On Mon, Aug 18, 2025 at 02:05:16PM +0100, James Clark wrote:
>>
>>
>> On 14/08/2025 5:49 pm, Frank Li wrote:
>>> On Thu, Aug 14, 2025 at 05:06:42PM +0100, James Clark wrote:
>>>> From: Larisa Grigore <larisa.grigore@nxp.com>
>>>>
>>>> The driver currently supports multiple chip-selects, but only sets the
>>>> polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
>>>> the desired chip-select.
>>>>
>>>> Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
>>>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>>> ---
>>>>    drivers/spi/spi-fsl-lpspi.c | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
>>>> index d44a23f7d6c1..c65eb6d31ee7 100644
>>>> --- a/drivers/spi/spi-fsl-lpspi.c
>>>> +++ b/drivers/spi/spi-fsl-lpspi.c
>>>> @@ -70,7 +70,7 @@
>>>>    #define DER_TDDE	BIT(0)
>>>>    #define CFGR1_PCSCFG	BIT(27)
>>>>    #define CFGR1_PINCFG	(BIT(24)|BIT(25))
>>>> -#define CFGR1_PCSPOL	BIT(8)
>>>> +#define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
>>>>    #define CFGR1_NOSTALL	BIT(3)
>>>>    #define CFGR1_HOST	BIT(0)
>>>>    #define FSR_TXCOUNT	(0xFF)
>>>> @@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
>>>>    	else
>>>>    		temp = CFGR1_PINCFG;
>>>>    	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
>>>> -		temp |= CFGR1_PCSPOL;
>>>> +		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
>>>> +				   BIT(fsl_lpspi->config.chip_select));
>>>> +
>>>
>>> Feel like FILED_PREP(..., BIT()) is stranged.
>>>
>>> I suggest #define CFGR1_PCSPOL(x) BIT((x) + 8)
>>>
>>> Frank
>>
>> It's using an existing macro that everyone knows though and I found 65
>> instances of exactly this. It can be read as "set bit X and put it into the
>> PCSPOL field without any further investigation.
> 
> Where have such pattern in kernel?
> 
> Frank
> 

Grep "FIELD_PREP\(.*,\n?.*BIT\("

65 results, e.g:

   return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));

James

>
>> If we make a new macro, first the reader will have to jump to it, then it
>> still doesn't immediately explain what the "+ 8" part is. Using FIELD_PREP()
>> also has the potential to use autogenerated field masks from a machine
>> readable version of the reference manual. You can't statically check your
>> macro to see if + 8 is correct or not, and it also doesn't catch overflow
>> errors like FIELD_PREP() does.
>>
>> There might be an argument to add a new global macro like FIELD_BIT(mask,
>> bit). But it's not very flexible (can't set multiple bits) and you can
>> already accomplish the same thing by adding BIT() to the existing one.
>>
>> Thanks
>> James
>>
>>>
>>>>    	writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
>>>>
>>>>    	temp = readl(fsl_lpspi->base + IMX7ULP_CR);
>>>>
>>>> --
>>>> 2.34.1
>>>>
>>

Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

Posted by Frank Li 5 months, 3 weeks ago

On Tue, Aug 19, 2025 at 09:21:08AM +0100, James Clark wrote:
>
>
> On 18/08/2025 4:19 pm, Frank Li wrote:
> > On Mon, Aug 18, 2025 at 02:05:16PM +0100, James Clark wrote:
> > >
> > >
> > > On 14/08/2025 5:49 pm, Frank Li wrote:
> > > > On Thu, Aug 14, 2025 at 05:06:42PM +0100, James Clark wrote:
> > > > > From: Larisa Grigore <larisa.grigore@nxp.com>
> > > > >
> > > > > The driver currently supports multiple chip-selects, but only sets the
> > > > > polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
> > > > > the desired chip-select.
> > > > >
> > > > > Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
> > > > > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> > > > > Signed-off-by: James Clark <james.clark@linaro.org>
> > > > > ---
> > > > >    drivers/spi/spi-fsl-lpspi.c | 6 ++++--
> > > > >    1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> > > > > index d44a23f7d6c1..c65eb6d31ee7 100644
> > > > > --- a/drivers/spi/spi-fsl-lpspi.c
> > > > > +++ b/drivers/spi/spi-fsl-lpspi.c
> > > > > @@ -70,7 +70,7 @@
> > > > >    #define DER_TDDE	BIT(0)
> > > > >    #define CFGR1_PCSCFG	BIT(27)
> > > > >    #define CFGR1_PINCFG	(BIT(24)|BIT(25))
> > > > > -#define CFGR1_PCSPOL	BIT(8)
> > > > > +#define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
> > > > >    #define CFGR1_NOSTALL	BIT(3)
> > > > >    #define CFGR1_HOST	BIT(0)
> > > > >    #define FSR_TXCOUNT	(0xFF)
> > > > > @@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
> > > > >    	else
> > > > >    		temp = CFGR1_PINCFG;
> > > > >    	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
> > > > > -		temp |= CFGR1_PCSPOL;
> > > > > +		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
> > > > > +				   BIT(fsl_lpspi->config.chip_select));
> > > > > +
> > > >
> > > > Feel like FILED_PREP(..., BIT()) is stranged.
> > > >
> > > > I suggest #define CFGR1_PCSPOL(x) BIT((x) + 8)
> > > >
> > > > Frank
> > >
> > > It's using an existing macro that everyone knows though and I found 65
> > > instances of exactly this. It can be read as "set bit X and put it into the
> > > PCSPOL field without any further investigation.
> >
> > Where have such pattern in kernel?
> >
> > Frank
> >
>
> Grep "FIELD_PREP\(.*,\n?.*BIT\("
>
> 65 results, e.g:
>
>   return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));

Thanks.

Frank
>
> James
>
> >
> > > If we make a new macro, first the reader will have to jump to it, then it
> > > still doesn't immediately explain what the "+ 8" part is. Using FIELD_PREP()
> > > also has the potential to use autogenerated field masks from a machine
> > > readable version of the reference manual. You can't statically check your
> > > macro to see if + 8 is correct or not, and it also doesn't catch overflow
> > > errors like FIELD_PREP() does.
> > >
> > > There might be an argument to add a new global macro like FIELD_BIT(mask,
> > > bit). But it's not very flexible (can't set multiple bits) and you can
> > > already accomplish the same thing by adding BIT() to the existing one.
> > >
> > > Thanks
> > > James
> > >
> > > >
> > > > >    	writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
> > > > >
> > > > >    	temp = readl(fsl_lpspi->base + IMX7ULP_CR);
> > > > >
> > > > > --
> > > > > 2.34.1
> > > > >
> > >
>