diff mbox series

[v2,3/6] spi: cadence-quadspi: Add multi-chipselect support for Intel LGM SoC

Message ID 20201021025507.51001-4-vadivel.muruganx.ramuthevar@linux.intel.com
State Superseded
Headers show
Series [v2,1/6] spi: cadence-quadspi: Add QSPI support for Intel LGM SoC | expand

Commit Message

Ramuthevar,Vadivel MuruganX Oct. 21, 2020, 2:55 a.m. UTC
From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

Add multiple chipselect support for Intel LGM SoCs,
currently QSPI-NOR and QSPI-NAND supported.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
 drivers/spi/spi-cadence-quadspi.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Mark Brown Oct. 21, 2020, 2:46 p.m. UTC | #1
On Wed, Oct 21, 2020 at 10:55:04AM +0800, Ramuthevar,Vadivel MuruganX wrote:

> Add multiple chipselect support for Intel LGM SoCs,

> currently QSPI-NOR and QSPI-NAND supported.


> +	if (ddata->hwcaps_mask & CQSPI_SUPPORTS_MULTI_CHIPSELECT)

> +		master->num_chipselect = cqspi->num_chipselect;


I'm not seeing anywhere else where we reference num_chipselect in this
patch - we parse the value, set it in the SPI controller and then never
otherwise use it?  This makes me wonder if the property is really
mandatory.  If it is then there should be something in the binding
document saying that it's required when the compatible is your new
compatible string, that way the validation can verify that the property
is present in DTs including this controller.
Pratyush Yadav Oct. 21, 2020, 3:13 p.m. UTC | #2
Hi,

On 21/10/20 10:55AM, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> 
> Add multiple chipselect support for Intel LGM SoCs,
> currently QSPI-NOR and QSPI-NAND supported.
> 
> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> ---
>  drivers/spi/spi-cadence-quadspi.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 3d017b484114..3bf6d3697631 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -38,6 +38,7 @@
>  
>  /* Capabilities */
>  #define CQSPI_SUPPORTS_OCTAL		BIT(0)
> +#define CQSPI_SUPPORTS_MULTI_CHIPSELECT BIT(1)
>  
>  struct cqspi_st;
>  
> @@ -75,6 +76,7 @@ struct cqspi_st {
>  	bool			is_decoded_cs;
>  	u32			fifo_depth;
>  	u32			fifo_width;
> +	u32			num_chipselect;
>  	bool			rclk_en;
>  	u32			trigger_address;
>  	u32			wr_delay;
> @@ -1070,6 +1072,14 @@ static int cqspi_of_get_pdata(struct cqspi_st *cqspi)
>  		return -ENXIO;
>  	}
>  
> +	if (!cqspi->use_direct_mode) {

Shouldn't this be guarded by CQSPI_SUPPORTS_MULTI_CHIPSELECT instead of 
cqspi->use_direct_mode?

Also, cqspi->use_direct_mode would always be false here because 
cqspi_of_get_pdata() is called before we set it...

> +		if (of_property_read_u32(np, "num-chipselect",
> +					 &cqspi->num_chipselect)) {
> +			dev_err(dev, "couldn't determine number of cs\n");
> +			return -ENXIO;

... so even if someone doesn't want to use multiple chip selects they 
would have to specify this property or the probe will fail, which is the 
case on J721E EVM for example.

> +		}
> +	}
> +
>  	cqspi->rclk_en = of_property_read_bool(np, "cdns,rclk-en");
>  
>  	return 0;
> @@ -1307,6 +1317,9 @@ static int cqspi_probe(struct platform_device *pdev)
>  	cqspi->current_cs = -1;
>  	cqspi->sclk = 0;
>  
> +	if (ddata->hwcaps_mask & CQSPI_SUPPORTS_MULTI_CHIPSELECT)
> +		master->num_chipselect = cqspi->num_chipselect;
> +
>  	ret = cqspi_setup_flash(cqspi);
>  	if (ret) {
>  		dev_err(dev, "failed to setup flash parameters %d\n", ret);
> @@ -1396,6 +1409,7 @@ static const struct cqspi_driver_platdata am654_ospi = {
>  };
>  
>  static const struct cqspi_driver_platdata intel_lgm_qspi = {
> +	.hwcaps_mask = CQSPI_SUPPORTS_MULTI_CHIPSELECT,
>  	.quirks = CQSPI_DISABLE_DAC_MODE,
>  };
>  
> -- 
> 2.11.0
>
Ramuthevar,Vadivel MuruganX Oct. 22, 2020, 2:07 a.m. UTC | #3
Hi Pratyush,

On 21/10/2020 11:13 pm, Pratyush Yadav wrote:
> Hi,

> 

> On 21/10/20 10:55AM, Ramuthevar,Vadivel MuruganX wrote:

>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

>>

>> Add multiple chipselect support for Intel LGM SoCs,

>> currently QSPI-NOR and QSPI-NAND supported.

>>

>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

>> ---

>>   drivers/spi/spi-cadence-quadspi.c | 14 ++++++++++++++

>>   1 file changed, 14 insertions(+)

>>

>> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c

>> index 3d017b484114..3bf6d3697631 100644

>> --- a/drivers/spi/spi-cadence-quadspi.c

>> +++ b/drivers/spi/spi-cadence-quadspi.c

>> @@ -38,6 +38,7 @@

>>   

>>   /* Capabilities */

>>   #define CQSPI_SUPPORTS_OCTAL		BIT(0)

>> +#define CQSPI_SUPPORTS_MULTI_CHIPSELECT BIT(1)

>>   

>>   struct cqspi_st;

>>   

>> @@ -75,6 +76,7 @@ struct cqspi_st {

>>   	bool			is_decoded_cs;

>>   	u32			fifo_depth;

>>   	u32			fifo_width;

>> +	u32			num_chipselect;

>>   	bool			rclk_en;

>>   	u32			trigger_address;

>>   	u32			wr_delay;

>> @@ -1070,6 +1072,14 @@ static int cqspi_of_get_pdata(struct cqspi_st *cqspi)

>>   		return -ENXIO;

>>   	}

>>   

>> +	if (!cqspi->use_direct_mode) {

> 

> Shouldn't this be guarded by CQSPI_SUPPORTS_MULTI_CHIPSELECT instead of

> cqspi->use_direct_mode?

Yes, we can use CQSPI_SUPPORTS_MULTI_CHIPSELECT instead of 
cqspi->use_direct_mode
> 

> Also, cqspi->use_direct_mode would always be false here because

> cqspi_of_get_pdata() is called before we set it...

Good catch, thanks!

Regards
Vadivel
> 

>> +		if (of_property_read_u32(np, "num-chipselect",

>> +					 &cqspi->num_chipselect)) {

>> +			dev_err(dev, "couldn't determine number of cs\n");

>> +			return -ENXIO;

> 

> ... so even if someone doesn't want to use multiple chip selects they

> would have to specify this property or the probe will fail, which is the

> case on J721E EVM for example.

> 

>> +		}

>> +	}

>> +

>>   	cqspi->rclk_en = of_property_read_bool(np, "cdns,rclk-en");

>>   

>>   	return 0;

>> @@ -1307,6 +1317,9 @@ static int cqspi_probe(struct platform_device *pdev)

>>   	cqspi->current_cs = -1;

>>   	cqspi->sclk = 0;

>>   

>> +	if (ddata->hwcaps_mask & CQSPI_SUPPORTS_MULTI_CHIPSELECT)

>> +		master->num_chipselect = cqspi->num_chipselect;

>> +

>>   	ret = cqspi_setup_flash(cqspi);

>>   	if (ret) {

>>   		dev_err(dev, "failed to setup flash parameters %d\n", ret);

>> @@ -1396,6 +1409,7 @@ static const struct cqspi_driver_platdata am654_ospi = {

>>   };

>>   

>>   static const struct cqspi_driver_platdata intel_lgm_qspi = {

>> +	.hwcaps_mask = CQSPI_SUPPORTS_MULTI_CHIPSELECT,

>>   	.quirks = CQSPI_DISABLE_DAC_MODE,

>>   };

>>   

>> -- 

>> 2.11.0

>>

>
diff mbox series

Patch

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 3d017b484114..3bf6d3697631 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -38,6 +38,7 @@ 
 
 /* Capabilities */
 #define CQSPI_SUPPORTS_OCTAL		BIT(0)
+#define CQSPI_SUPPORTS_MULTI_CHIPSELECT BIT(1)
 
 struct cqspi_st;
 
@@ -75,6 +76,7 @@  struct cqspi_st {
 	bool			is_decoded_cs;
 	u32			fifo_depth;
 	u32			fifo_width;
+	u32			num_chipselect;
 	bool			rclk_en;
 	u32			trigger_address;
 	u32			wr_delay;
@@ -1070,6 +1072,14 @@  static int cqspi_of_get_pdata(struct cqspi_st *cqspi)
 		return -ENXIO;
 	}
 
+	if (!cqspi->use_direct_mode) {
+		if (of_property_read_u32(np, "num-chipselect",
+					 &cqspi->num_chipselect)) {
+			dev_err(dev, "couldn't determine number of cs\n");
+			return -ENXIO;
+		}
+	}
+
 	cqspi->rclk_en = of_property_read_bool(np, "cdns,rclk-en");
 
 	return 0;
@@ -1307,6 +1317,9 @@  static int cqspi_probe(struct platform_device *pdev)
 	cqspi->current_cs = -1;
 	cqspi->sclk = 0;
 
+	if (ddata->hwcaps_mask & CQSPI_SUPPORTS_MULTI_CHIPSELECT)
+		master->num_chipselect = cqspi->num_chipselect;
+
 	ret = cqspi_setup_flash(cqspi);
 	if (ret) {
 		dev_err(dev, "failed to setup flash parameters %d\n", ret);
@@ -1396,6 +1409,7 @@  static const struct cqspi_driver_platdata am654_ospi = {
 };
 
 static const struct cqspi_driver_platdata intel_lgm_qspi = {
+	.hwcaps_mask = CQSPI_SUPPORTS_MULTI_CHIPSELECT,
 	.quirks = CQSPI_DISABLE_DAC_MODE,
 };