| Message ID | 20220613195658.5607-12-brad@pensando.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support AMD Pensando Elba SoC | expand |
Hi Brad, On 13/06/22 12:56PM, Brad Larson wrote: > From: Brad Larson <blarson@amd.com> > > The AMD Pensando Elba SoC has the Cadence QSPI controller integrated. > > The quirk CQSPI_NEEDS_APB_AHB_HAZARD_WAR is added and if enabled > a dummy readback from the controller is performed to ensure > synchronization. > > Signed-off-by: Brad Larson <blarson@amd.com> > --- > drivers/spi/spi-cadence-quadspi.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > index 72b1a5a2298c..ebb77ea8e6ba 100644 > --- a/drivers/spi/spi-cadence-quadspi.c > +++ b/drivers/spi/spi-cadence-quadspi.c > @@ -39,6 +39,7 @@ > #define CQSPI_DISABLE_DAC_MODE BIT(1) > #define CQSPI_SUPPORT_EXTERNAL_DMA BIT(2) > #define CQSPI_NO_SUPPORT_WR_COMPLETION BIT(3) > +#define CQSPI_NEEDS_APB_AHB_HAZARD_WAR BIT(4) > > /* Capabilities */ > #define CQSPI_SUPPORTS_OCTAL BIT(0) > @@ -87,6 +88,7 @@ struct cqspi_st { > bool use_dma_read; > u32 pd_dev_id; > bool wr_completion; > + bool apb_ahb_hazard; > }; > > struct cqspi_driver_platdata { > @@ -952,6 +954,13 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata, > if (cqspi->wr_delay) > ndelay(cqspi->wr_delay); > > + /* > + * If a hazard exists between the APB and AHB interfaces, perform a > + * dummy readback from the controller to ensure synchronization. > + */ This is needed for TI's SoCs as well. APB and AHB accesses are independent of each other on the interconnect and can be racy. I wrote a couple patches [0][1] to fix this on TI's fork. I never got around to sending them upstream. It would be great if you can pick those up. They fix the race in all paths, not just indirect write. I would also prefer if we do this unconditionally. I don't think it has much downside even on platforms that do not strictly need this. [0] https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/drivers/spi/spi-cadence-quadspi.c?h=ti-linux-5.10.y&id=027f03a8512086e5ef05dc4e4ff53b2628848f95 [1] https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/drivers/spi/spi-cadence-quadspi.c?h=ti-linux-5.10.y&id=4c367e58bab7d3f9c470c3778441f73546f20398 > + if (cqspi->apb_ahb_hazard) > + (void)readl(reg_base + CQSPI_REG_INDIRECTWR); > + > while (remaining > 0) { > size_t write_words, mod_bytes; > > @@ -1667,6 +1676,8 @@ static int cqspi_probe(struct platform_device *pdev) > cqspi->use_dma_read = true; > if (ddata->quirks & CQSPI_NO_SUPPORT_WR_COMPLETION) > cqspi->wr_completion = false; > + if (ddata->quirks & CQSPI_NEEDS_APB_AHB_HAZARD_WAR) > + cqspi->apb_ahb_hazard = true; > > if (of_device_is_compatible(pdev->dev.of_node, > "xlnx,versal-ospi-1.0")) > @@ -1789,6 +1800,10 @@ static const struct cqspi_driver_platdata versal_ospi = { > .get_dma_status = cqspi_get_versal_dma_status, > }; > > +static const struct cqspi_driver_platdata pen_cdns_qspi = { > + .quirks = CQSPI_NEEDS_APB_AHB_HAZARD_WAR | CQSPI_DISABLE_DAC_MODE, > +}; > + > static const struct of_device_id cqspi_dt_ids[] = { > { > .compatible = "cdns,qspi-nor", > @@ -1814,6 +1829,10 @@ static const struct of_device_id cqspi_dt_ids[] = { > .compatible = "intel,socfpga-qspi", > .data = &socfpga_qspi, > }, > + { > + .compatible = "amd,pensando-elba-qspi", > + .data = &pen_cdns_qspi, > + }, > { /* end of table */ } > }; > > -- > 2.17.1 >
On Mon, Jun 13, 2022 at 12:56:54PM -0700, Brad Larson wrote: > + /* > + * If a hazard exists between the APB and AHB interfaces, perform a > + * dummy readback from the controller to ensure synchronization. > + */ > + if (cqspi->apb_ahb_hazard) > + (void)readl(reg_base + CQSPI_REG_INDIRECTWR); You shouldn't need the cast here.
Hi Pratyush, On Tue, Jun 14, 2022 at 1:49 AM Pratyush Yadav <p.yadav@ti.com> wrote: > This is needed for TI's SoCs as well. APB and AHB accesses are > independent of each other on the interconnect and can be racy. I wrote a > couple patches [0][1] to fix this on TI's fork. I never got around to > sending them upstream. It would be great if you can pick those up. They > fix the race in all paths, not just indirect write. > > I would also prefer if we do this unconditionally. I don't think it has > much downside even on platforms that do not strictly need this. > > [0] https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/drivers/spi/spi-cadence-quadspi.c?h=ti-linux-5.10.y&id=027f03a8512086e5ef05dc4e4ff53b2628848f95 > [1] https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/drivers/spi/spi-cadence-quadspi.c?h=ti-linux-5.10.y&id=4c367e58bab7d3f9c470c3778441f73546f20398 Let's get Elba specific support in first and then in a separate patch go for the unconditional. An extra op for devices for which its not currently done will result in questions I can't answer. Regards, Brad
Hi Mark, On Tue, Jun 14, 2022 at 5:01 AM Mark Brown <broonie@kernel.org> wrote: > > On Mon, Jun 13, 2022 at 12:56:54PM -0700, Brad Larson wrote: > > > + /* > > + * If a hazard exists between the APB and AHB interfaces, perform a > > + * dummy readback from the controller to ensure synchronization. > > + */ > > + if (cqspi->apb_ahb_hazard) > > + (void)readl(reg_base + CQSPI_REG_INDIRECTWR); > > You shouldn't need the cast here. Removed (void) cast Regards, Brad
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index 72b1a5a2298c..ebb77ea8e6ba 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -39,6 +39,7 @@ #define CQSPI_DISABLE_DAC_MODE BIT(1) #define CQSPI_SUPPORT_EXTERNAL_DMA BIT(2) #define CQSPI_NO_SUPPORT_WR_COMPLETION BIT(3) +#define CQSPI_NEEDS_APB_AHB_HAZARD_WAR BIT(4) /* Capabilities */ #define CQSPI_SUPPORTS_OCTAL BIT(0) @@ -87,6 +88,7 @@ struct cqspi_st { bool use_dma_read; u32 pd_dev_id; bool wr_completion; + bool apb_ahb_hazard; }; struct cqspi_driver_platdata { @@ -952,6 +954,13 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata, if (cqspi->wr_delay) ndelay(cqspi->wr_delay); + /* + * If a hazard exists between the APB and AHB interfaces, perform a + * dummy readback from the controller to ensure synchronization. + */ + if (cqspi->apb_ahb_hazard) + (void)readl(reg_base + CQSPI_REG_INDIRECTWR); + while (remaining > 0) { size_t write_words, mod_bytes; @@ -1667,6 +1676,8 @@ static int cqspi_probe(struct platform_device *pdev) cqspi->use_dma_read = true; if (ddata->quirks & CQSPI_NO_SUPPORT_WR_COMPLETION) cqspi->wr_completion = false; + if (ddata->quirks & CQSPI_NEEDS_APB_AHB_HAZARD_WAR) + cqspi->apb_ahb_hazard = true; if (of_device_is_compatible(pdev->dev.of_node, "xlnx,versal-ospi-1.0")) @@ -1789,6 +1800,10 @@ static const struct cqspi_driver_platdata versal_ospi = { .get_dma_status = cqspi_get_versal_dma_status, }; +static const struct cqspi_driver_platdata pen_cdns_qspi = { + .quirks = CQSPI_NEEDS_APB_AHB_HAZARD_WAR | CQSPI_DISABLE_DAC_MODE, +}; + static const struct of_device_id cqspi_dt_ids[] = { { .compatible = "cdns,qspi-nor", @@ -1814,6 +1829,10 @@ static const struct of_device_id cqspi_dt_ids[] = { .compatible = "intel,socfpga-qspi", .data = &socfpga_qspi, }, + { + .compatible = "amd,pensando-elba-qspi", + .data = &pen_cdns_qspi, + }, { /* end of table */ } };