[PATCH 0/3] Add support for GPIO based CS

[PATCH 0/3] Add support for GPIO based CS

[Prajna Rajendra Kumar]

The Microchip PolarFire SoC SPI controller supports multiple 
chip selects. However, only one chip select is connected in the MSS. 
Therefore, use GPIO descriptors to configure additional chip select 
lines.

Prajna Rajendra Kumar (3):
  spi: spi-microchip-core: Add support for GPIO based CS
  spi: dt-bindings: Add num-cs property for mpfs-spi
  spi: spi-microchip-core: Fix the number of chip selects supported

 .../bindings/spi/microchip,mpfs-spi.yaml      | 19 ++++++++++++++++---
 drivers/spi/spi-microchip-core.c              |  6 +++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/3] spi: spi-microchip-core: Add support for GPIO based CS

[Prajna Rajendra Kumar]

The SPI controller within the PolarFire SoC is capable of handling
multiple CS, but only one CS line is wired in the MSS. Therefore,
use GPIO descriptors to configure additional CS lines.

Signed-off-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>
---
 drivers/spi/spi-microchip-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/spi/spi-microchip-core.c b/drivers/spi/spi-microchip-core.c
index 634364c7cfe6..71886c27bca3 100644
--- a/drivers/spi/spi-microchip-core.c
+++ b/drivers/spi/spi-microchip-core.c
@@ -258,6 +258,9 @@ static int mchp_corespi_setup(struct spi_device *spi)
 	struct mchp_corespi *corespi = spi_controller_get_devdata(spi->controller);
 	u32 reg;
 
+	if (spi->cs_gpiod)
+		return 0;
+
 	/*
 	 * Active high targets need to be specifically set to their inactive
 	 * states during probe by adding them to the "control group" & thus
@@ -516,6 +519,7 @@ static int mchp_corespi_probe(struct platform_device *pdev)
 
 	host->num_chipselect = num_cs;
 	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
+	host->use_gpio_descriptors = true;
 	host->setup = mchp_corespi_setup;
 	host->bits_per_word_mask = SPI_BPW_MASK(8);
 	host->transfer_one = mchp_corespi_transfer_one;
-- 
2.25.1

[PATCH 2/3] spi: dt-bindings: Add num-cs property for mpfs-spi

[Prajna Rajendra Kumar]

The PolarFire SoC SPI controller supports multiple chip selects,but in
the MSS, only one CS line is physically wired. To reflect this hardware
limitation in the device tree, the binding enforces that the 'num-cs'
property defaults to 1 and cannot exceed 1 unless additional
chip select lines are explicitly defined using GPIO descriptors.

Fixes: 2da187304e55 ("spi: add bindings for microchip mpfs spi")
Signed-off-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>
---
 .../bindings/spi/microchip,mpfs-spi.yaml      | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
index 74a817cc7d94..19951951fdd6 100644
--- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
+++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
@@ -13,9 +13,6 @@ description:
 maintainers:
   - Conor Dooley <conor.dooley@microchip.com>
 
-allOf:
-  - $ref: spi-controller.yaml#
-
 properties:
   compatible:
     oneOf:
@@ -43,6 +40,22 @@ required:
   - interrupts
   - clocks
 
+allOf:
+  - $ref: spi-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: microchip,mpfs-spi
+      not:
+        required:
+          - cs-gpios
+    then:
+      properties:
+        num-cs:
+          default: 1
+          maximum: 1
+
 unevaluatedProperties: false
 
 examples:
-- 
2.25.1

[PATCH 3/3] spi: spi-microchip-core: Fix the number of chip selects supported

[Prajna Rajendra Kumar]

The SPI controller in PolarFire SoC has multiple chip selects, but only
one is wired up in the MSS. Therefore, fix the driver to chose one
chip select.

Fixes: 9ac8d17694b6 ("spi: add support for microchip fpga spi controllers")
Signed-off-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>
---
 drivers/spi/spi-microchip-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-microchip-core.c b/drivers/spi/spi-microchip-core.c
index 71886c27bca3..4289dfba9af5 100644
--- a/drivers/spi/spi-microchip-core.c
+++ b/drivers/spi/spi-microchip-core.c
@@ -21,7 +21,7 @@
 #include <linux/spi/spi.h>
 
 #define MAX_LEN				(0xffff)
-#define MAX_CS				(8)
+#define MAX_CS				(1)
 #define DEFAULT_FRAMESIZE		(8)
 #define FIFO_DEPTH			(32)
 #define CLK_GEN_MODE1_MAX		(255)
-- 
2.25.1

Re: [PATCH 2/3] spi: dt-bindings: Add num-cs property for mpfs-spi

[Krzysztof Kozlowski]

On 02/05/2024 16:34, Prajna Rajendra Kumar wrote:
> The PolarFire SoC SPI controller supports multiple chip selects,but in
> the MSS, only one CS line is physically wired. To reflect this hardware
> limitation in the device tree, the binding enforces that the 'num-cs'
> property defaults to 1 and cannot exceed 1 unless additional
> chip select lines are explicitly defined using GPIO descriptors.
> 

You marked it as Fix for bug, but I don't understand where the bug is.
Do you describe above the issue or the solution?

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: microchip,mpfs-spi
> +      not:
> +        required:
> +          - cs-gpios

I don't understand what you are expressing here. Did you actually
validate it that it achieves exactly what you want?

> +    then:
> +      properties:
> +        num-cs:
> +          default: 1
> +          maximum: 1
> +
>  unevaluatedProperties: false
>  
>  examples:

Best regards,
Krzysztof

Re: [PATCH 3/3] spi: spi-microchip-core: Fix the number of chip selects supported

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

On Thu, May 02, 2024 at 03:34:10PM +0100, Prajna Rajendra Kumar wrote:
> The SPI controller in PolarFire SoC has multiple chip selects, but only
> one is wired up in the MSS. Therefore, fix the driver to chose one
> chip select.

Given Krzysztof's comments on the binding "only one is wired up in the
MSS" probably means nothing to anyone other than us, and we should
rephrase this so that we explain what this actually means.
The actual change is fine to me though, since I do understand what's
being said here. With a revised commit message:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> 
> Fixes: 9ac8d17694b6 ("spi: add support for microchip fpga spi controllers")
> Signed-off-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>
> ---
>  drivers/spi/spi-microchip-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-microchip-core.c b/drivers/spi/spi-microchip-core.c
> index 71886c27bca3..4289dfba9af5 100644
> --- a/drivers/spi/spi-microchip-core.c
> +++ b/drivers/spi/spi-microchip-core.c
> @@ -21,7 +21,7 @@
>  #include <linux/spi/spi.h>
>  
>  #define MAX_LEN				(0xffff)
> -#define MAX_CS				(8)
> +#define MAX_CS				(1)
>  #define DEFAULT_FRAMESIZE		(8)
>  #define FIFO_DEPTH			(32)
>  #define CLK_GEN_MODE1_MAX		(255)
> -- 
> 2.25.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] spi: spi-microchip-core: Add support for GPIO based CS

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

On Thu, May 02, 2024 at 03:34:08PM +0100, Prajna Rajendra Kumar wrote:
> The SPI controller within the PolarFire SoC is capable of handling
> multiple CS, but only one CS line is wired in the MSS. Therefore,
> use GPIO descriptors to configure additional CS lines.
> 
> Signed-off-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] spi: spi-microchip-core: Add support for GPIO based CS

[kernel test robot]

Hi Prajna,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-spi/for-next]
[also build test WARNING on robh/for-next krzk-dt/for-next linus/master v6.9-rc6 next-20240502]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Prajna-Rajendra-Kumar/spi-spi-microchip-core-Add-support-for-GPIO-based-CS/20240502-223714
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
patch link:    https://lore.kernel.org/r/20240502143410.12629-2-prajna.rajendrakumar%40microchip.com
patch subject: [PATCH 1/3] spi: spi-microchip-core: Add support for GPIO based CS
config: arm-randconfig-002-20240503 (https://download.01.org/0day-ci/archive/20240503/202405031328.ljBB1tMb-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 37ae4ad0eef338776c7e2cffb3896153d43dcd90)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240503/202405031328.ljBB1tMb-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/202405031328.ljBB1tMb-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/spi/spi-microchip-core.c:21:
   In file included from include/linux/spi/spi.h:17:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:2208:
   include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     522 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/spi/spi-microchip-core.c:261:11: warning: address of array 'spi->cs_gpiod' will always evaluate to 'true' [-Wpointer-bool-conversion]
     261 |         if (spi->cs_gpiod)
         |         ~~  ~~~~~^~~~~~~~
   2 warnings generated.


vim +261 drivers/spi/spi-microchip-core.c

   255	
   256	static int mchp_corespi_setup(struct spi_device *spi)
   257	{
   258		struct mchp_corespi *corespi = spi_controller_get_devdata(spi->controller);
   259		u32 reg;
   260	
 > 261		if (spi->cs_gpiod)
   262			return 0;
   263	
   264		/*
   265		 * Active high targets need to be specifically set to their inactive
   266		 * states during probe by adding them to the "control group" & thus
   267		 * driving their select line low.
   268		 */
   269		if (spi->mode & SPI_CS_HIGH) {
   270			reg = mchp_corespi_read(corespi, REG_SLAVE_SELECT);
   271			reg |= BIT(spi_get_chipselect(spi, 0));
   272			mchp_corespi_write(corespi, REG_SLAVE_SELECT, reg);
   273		}
   274		return 0;
   275	}
   276	

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

Re: [PATCH 1/3] spi: spi-microchip-core: Add support for GPIO based CS

[kernel test robot]

Hi Prajna,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-spi/for-next]
[also build test WARNING on robh/for-next krzk-dt/for-next linus/master v6.9-rc6 next-20240502]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Prajna-Rajendra-Kumar/spi-spi-microchip-core-Add-support-for-GPIO-based-CS/20240502-223714
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
patch link:    https://lore.kernel.org/r/20240502143410.12629-2-prajna.rajendrakumar%40microchip.com
patch subject: [PATCH 1/3] spi: spi-microchip-core: Add support for GPIO based CS
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20240503/202405031322.8UuTAWrf-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240503/202405031322.8UuTAWrf-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/202405031322.8UuTAWrf-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/spi/spi-microchip-core.c: In function 'mchp_corespi_setup':
>> drivers/spi/spi-microchip-core.c:261:13: warning: the comparison will always evaluate as 'true' for the address of 'cs_gpiod' will never be NULL [-Waddress]
     261 |         if (spi->cs_gpiod)
         |             ^~~
   In file included from drivers/spi/spi-microchip-core.c:21:
   include/linux/spi/spi.h:219:34: note: 'cs_gpiod' declared here
     219 |         struct gpio_desc        *cs_gpiod[SPI_CS_CNT_MAX];      /* Chip select gpio desc */
         |                                  ^~~~~~~~


vim +261 drivers/spi/spi-microchip-core.c

   255	
   256	static int mchp_corespi_setup(struct spi_device *spi)
   257	{
   258		struct mchp_corespi *corespi = spi_controller_get_devdata(spi->controller);
   259		u32 reg;
   260	
 > 261		if (spi->cs_gpiod)
   262			return 0;
   263	
   264		/*
   265		 * Active high targets need to be specifically set to their inactive
   266		 * states during probe by adding them to the "control group" & thus
   267		 * driving their select line low.
   268		 */
   269		if (spi->mode & SPI_CS_HIGH) {
   270			reg = mchp_corespi_read(corespi, REG_SLAVE_SELECT);
   271			reg |= BIT(spi_get_chipselect(spi, 0));
   272			mchp_corespi_write(corespi, REG_SLAVE_SELECT, reg);
   273		}
   274		return 0;
   275	}
   276	

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

Re: [PATCH 2/3] spi: dt-bindings: Add num-cs property for mpfs-spi

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]

On Thu, May 02, 2024 at 03:34:09PM +0100, Prajna Rajendra Kumar wrote:
> The PolarFire SoC SPI controller supports multiple chip selects,but in
> the MSS, only one CS line is physically wired. To reflect this hardware
> limitation in the device tree, the binding enforces that the 'num-cs'
> property defaults to 1 and cannot exceed 1 unless additional
> chip select lines are explicitly defined using GPIO descriptors.
> 
> Fixes: 2da187304e55 ("spi: add bindings for microchip mpfs spi")
> Signed-off-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>
> ---
>  .../bindings/spi/microchip,mpfs-spi.yaml      | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
> index 74a817cc7d94..19951951fdd6 100644
> --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
> +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
> @@ -13,9 +13,6 @@ description:
>  maintainers:
>    - Conor Dooley <conor.dooley@microchip.com>
>  
> -allOf:
> -  - $ref: spi-controller.yaml#
> -
>  properties:
>    compatible:
>      oneOf:
> @@ -43,6 +40,22 @@ required:
>    - interrupts
>    - clocks
>  
> +allOf:
> +  - $ref: spi-controller.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: microchip,mpfs-spi
> +      not:
> +        required:
> +          - cs-gpios
> +    then:
> +      properties:
> +        num-cs:
> +          default: 1
> +          maximum: 1

I think this isn't quite right actually. The default for num-cs should
be 1, regardless of whether cs-gpios are used or not, only the maximum
is affected by using GPIOs for chip select.

> +
>  unevaluatedProperties: false
>  
>  examples:
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] spi: dt-bindings: Add num-cs property for mpfs-spi

[Prajna.Rajendrakumar]

On Thu, 2024-05-02 at 16:53 +0200, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On 02/05/2024 16:34, Prajna Rajendra Kumar wrote:
> > The PolarFire SoC SPI controller supports multiple chip selects,but
> > in
> > the MSS, only one CS line is physically wired. To reflect this
> > hardware
> > limitation in the device tree, the binding enforces that the 'num-
> > cs'
> > property defaults to 1 and cannot exceed 1 unless additional
> > chip select lines are explicitly defined using GPIO descriptors.
> > 
> 

Hi,

> You marked it as Fix for bug, but I don't understand where the bug
> is.

The bug was that the PolarFire SoC SPI "hard" controller supports eight
chip selects, but only one chip select is connected and can be used.
This was not reflected in the device tree because default num-cs was
never set. Hence, it's marked as a fix.

> Do you describe above the issue or the solution?

It describes both the issue and the solution, but I will revise the
commit message as Conor Dooley suggested.
> 
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: microchip,mpfs-spi
> > +      not:
> > +        required:
> > +          - cs-gpios
> 
> I don't understand what you are expressing here. Did you actually
> validate it that it achieves exactly what you want?

Since the controller supports only one chip select, the num-cs should
default to 1 and cannot exceed 1 unless GPIOs are used as chip selects.

> > +    then:
> > +      properties:
> > +        num-cs:
> > +          default: 1
> > +          maximum: 1
> > +
> >  unevaluatedProperties: false
> > 
> >  examples:
> 
> Best regards,
> Krzysztof
> 
Best regards,
Prajna

Re: [PATCH 2/3] spi: dt-bindings: Add num-cs property for mpfs-spi

[Krzysztof Kozlowski]

On 03/05/2024 14:54, Prajna.Rajendrakumar@microchip.com wrote:
>>
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: microchip,mpfs-spi
>>> +      not:
>>> +        required:
>>> +          - cs-gpios
>>
>> I don't understand what you are expressing here. Did you actually
>> validate it that it achieves exactly what you want?
> 
> Since the controller supports only one chip select, the num-cs should
> default to 1 and cannot exceed 1 unless GPIOs are used as chip selects.

That's not really the answer... or you want to say you did not test it?

Best regards,
Krzysztof

Re: [PATCH 2/3] spi: dt-bindings: Add num-cs property for mpfs-spi

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

On Fri, May 03, 2024 at 04:46:13PM +0200, Krzysztof Kozlowski wrote:
> On 03/05/2024 14:54, Prajna.Rajendrakumar@microchip.com wrote:
> >>
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: microchip,mpfs-spi
> >>> +      not:
> >>> +        required:
> >>> +          - cs-gpios
> >>
> >> I don't understand what you are expressing here. Did you actually
> >> validate it that it achieves exactly what you want?
> > 
> > Since the controller supports only one chip select, the num-cs should
> > default to 1 and cannot exceed 1 unless GPIOs are used as chip selects.
> 
> That's not really the answer... or you want to say you did not test it?

She told me she did, and even if she hadn't, I did, despite what my
email earlier today might suggest!

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] spi: dt-bindings: Add num-cs property for mpfs-spi

[Prajna.Rajendrakumar]

On Fri, 2024-05-03 at 16:46 +0200, Krzysztof Kozlowski wrote:
> [Some people who received this message don't often get email from
> krzk@kernel.org. Learn why this is important at 
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On 03/05/2024 14:54, Prajna.Rajendrakumar@microchip.com wrote:
> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            const: microchip,mpfs-spi
> > > > +      not:
> > > > +        required:
> > > > +          - cs-gpios
> > > 
> > > I don't understand what you are expressing here. Did you actually
> > > validate it that it achieves exactly what you want?
> > 
> > Since the controller supports only one chip select, the num-cs
> > should
> > default to 1 and cannot exceed 1 unless GPIOs are used as chip
> > selects.
> 
> That's not really the answer... or you want to say you did not test
> it?
> 
I overlooked mentioning this in my previous reply. It's been thoroughly
validated.

> Best regards,
> Krzysztof
> 
Best regards,
Prajna