| Message ID | 20201021025507.51001-4-vadivel.muruganx.ramuthevar@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: cadence-quadspi: Add QSPI controller support for Intel LGM SoC | expand |
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.
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 >
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 --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, };