This series adds support for Cadence QSPI IP present in TI's 66AK2G SoC.
The patches enhance the existing cadence-quadspi driver to support
loopback clock circuit, pm_runtime support and tweaks for 66AK2G SoC.
Change log:
v3:
* Fix build warnings reported by kbuild test bot.
Resend:
* Rebase to latest linux-next.
* Collect Acked-bys
v2:
* Drop DT patches. Will be sent as separate series as requested by
maintainer.
* Split binding docs into separate patches.
* Address comments by Rob Herring.
Vignesh R (5):
mtd: spi-nor: cadence-quadspi: Add TI 66AK2G SoC specific compatible
mtd: spi-nor: cadence-quadspi: add a delay in write sequence
mtd: spi-nor: cadence-quadspi: Add new binding to enable loop-back
circuit
mtd: spi-nor: cadence-quadspi: Add support to enable loop-back clock
circuit
mtd: spi-nor: cadence-quadspi: Add runtime PM support
.../devicetree/bindings/mtd/cadence-quadspi.txt | 7 +++-
drivers/mtd/spi-nor/cadence-quadspi.c | 46 ++++++++++++++++++++--
2 files changed, 49 insertions(+), 4 deletions(-)
--
2.14.1
Cadence QSPI IP has a adapted loop-back circuit which can be enabled by
setting BYPASS field to 0 in READCAPTURE register. It enables use of
QSPI return clock to latch the data rather than the internal QSPI
reference clock. For high speed operations, adapted loop-back circuit
using QSPI return clock helps to increase data valid window.
Add DT parameter cdns,rclk-en to help enable adapted loop-back circuit
for boards which do have QSPI return clock provided. Update binding
documentation for the same.
Signed-off-by: Vignesh R <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/mtd/cadence-quadspi.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
index 7dbe3bd9ac56..bb2075df9b38 100644
--- a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
+++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
@@ -16,6 +16,9 @@ Required properties:
Optional properties:
- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
+- cdns,rclk-en : Flag to indicate that QSPI return clock is used to latch
+ the read data rather than the QSPI clock. Make sure that QSPI return
+ clock is populated on the board before using this property.
Optional subnodes:
Subnodes of the Cadence Quad SPI controller are spi slave nodes with additional
--
2.14.1
Add pm_runtime* calls to cadence-quadspi driver. This is required to
switch on QSPI power domain on TI 66AK2G SoC during probe.
Signed-off-by: Vignesh R <[email protected]>
---
drivers/mtd/spi-nor/cadence-quadspi.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index d9629e8f4798..2c8e6226d267 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -31,6 +31,7 @@
#include <linux/of_device.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/sched.h>
#include <linux/spi/spi.h>
#include <linux/timer.h>
@@ -1224,6 +1225,13 @@ static int cqspi_probe(struct platform_device *pdev)
return -ENXIO;
}
+ pm_runtime_enable(&pdev->dev);
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(&pdev->dev);
+ return ret;
+ }
+
ret = clk_prepare_enable(cqspi->clk);
if (ret) {
dev_err(dev, "Cannot enable QSPI clock.\n");
@@ -1275,6 +1283,9 @@ static int cqspi_remove(struct platform_device *pdev)
clk_disable_unprepare(cqspi->clk);
+ pm_runtime_put_sync(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+
return 0;
}
--
2.14.1
Update binding documentation to add a new compatible for TI 66AK2G SoC,
to handle TI SoC specific quirks in the driver.
Signed-off-by: Vignesh R <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/mtd/cadence-quadspi.txt | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
index f248056da24c..7dbe3bd9ac56 100644
--- a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
+++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
@@ -1,7 +1,9 @@
* Cadence Quad SPI controller
Required properties:
-- compatible : Should be "cdns,qspi-nor".
+- compatible : should be one of the following:
+ Generic default - "cdns,qspi-nor".
+ For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor".
- reg : Contains two entries, each of which is a tuple consisting of a
physical address and length. The first entry is the address and
length of the controller register set. The second entry is the
--
2.14.1
As per 66AK2G02 TRM[1] SPRUHY8F section 11.15.5.3 Indirect Access
Controller programming sequence, a delay equal to couple of QSPI master
clock(~5ns) is required after setting CQSPI_REG_INDIRECTWR_START bit and
writing data to the flash. Introduce a quirk flag CQSPI_NEEDS_WR_DELAY
to handle this and set this flag for TI 66AK2G SoC.
[1]http://www.ti.com/lit/ug/spruhy8f/spruhy8f.pdf
Signed-off-by: Vignesh R <[email protected]>
---
v3:
Fix build warnings reported by kbuild test bot.
drivers/mtd/spi-nor/cadence-quadspi.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index 53c7d8e0327a..5cd5d6f7303f 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -38,6 +38,9 @@
#define CQSPI_NAME "cadence-qspi"
#define CQSPI_MAX_CHIPSELECT 16
+/* Quirks */
+#define CQSPI_NEEDS_WR_DELAY BIT(0)
+
struct cqspi_st;
struct cqspi_flash_pdata {
@@ -76,6 +79,7 @@ struct cqspi_st {
u32 fifo_depth;
u32 fifo_width;
u32 trigger_address;
+ u32 wr_delay;
struct cqspi_flash_pdata f_pdata[CQSPI_MAX_CHIPSELECT];
};
@@ -608,6 +612,15 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor,
reinit_completion(&cqspi->transfer_complete);
writel(CQSPI_REG_INDIRECTWR_START_MASK,
reg_base + CQSPI_REG_INDIRECTWR);
+ /*
+ * As per 66AK2G02 TRM SPRUHY8F section 11.15.5.3 Indirect Access
+ * Controller programming sequence, couple of cycles of
+ * QSPI_REF_CLK delay is required for the above bit to
+ * be internally synchronized by the QSPI module. Provide 5
+ * cycles of delay.
+ */
+ if (cqspi->wr_delay)
+ ndelay(cqspi->wr_delay);
while (remaining > 0) {
write_bytes = remaining > page_size ? page_size : remaining;
@@ -1156,6 +1169,7 @@ static int cqspi_probe(struct platform_device *pdev)
struct cqspi_st *cqspi;
struct resource *res;
struct resource *res_ahb;
+ unsigned long data;
int ret;
int irq;
@@ -1213,6 +1227,10 @@ static int cqspi_probe(struct platform_device *pdev)
}
cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk);
+ data = (unsigned long)of_device_get_match_data(dev);
+ if (data & CQSPI_NEEDS_WR_DELAY)
+ cqspi->wr_delay = 5 * DIV_ROUND_UP(NSEC_PER_SEC,
+ cqspi->master_ref_clk_hz);
ret = devm_request_irq(dev, irq, cqspi_irq_handler, 0,
pdev->name, cqspi);
@@ -1284,7 +1302,14 @@ static const struct dev_pm_ops cqspi__dev_pm_ops = {
#endif
static const struct of_device_id cqspi_dt_ids[] = {
- {.compatible = "cdns,qspi-nor",},
+ {
+ .compatible = "cdns,qspi-nor",
+ .data = (void *)0,
+ },
+ {
+ .compatible = "ti,k2g-qspi",
+ .data = (void *)CQSPI_NEEDS_WR_DELAY,
+ },
{ /* end of table */ }
};
--
2.14.1
Cadence QSPI IP has a adapted loop-back circuit which can be enabled by
setting BYPASS field to 0 in READCAPTURE register. It enables use of
QSPI return clock to latch the data rather than the internal QSPI
reference clock. For high speed operations, adapted loop-back circuit
using QSPI return clock helps to increase data valid window.
Based on DT parameter cdns,rclk-en enable adapted loop-back circuit
for boards which do have QSPI return clock provided.
This patch also modifies cqspi_readdata_capture() function's bypass
parameter to bool to match how its used in the function.
Signed-off-by: Vignesh R <[email protected]>
---
drivers/mtd/spi-nor/cadence-quadspi.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index 5cd5d6f7303f..d9629e8f4798 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -78,6 +78,7 @@ struct cqspi_st {
bool is_decoded_cs;
u32 fifo_depth;
u32 fifo_width;
+ bool rclk_en;
u32 trigger_address;
u32 wr_delay;
struct cqspi_flash_pdata f_pdata[CQSPI_MAX_CHIPSELECT];
@@ -788,7 +789,7 @@ static void cqspi_config_baudrate_div(struct cqspi_st *cqspi)
}
static void cqspi_readdata_capture(struct cqspi_st *cqspi,
- const unsigned int bypass,
+ const bool bypass,
const unsigned int delay)
{
void __iomem *reg_base = cqspi->iobase;
@@ -852,7 +853,8 @@ static void cqspi_configure(struct spi_nor *nor)
cqspi->sclk = sclk;
cqspi_config_baudrate_div(cqspi);
cqspi_delay(nor);
- cqspi_readdata_capture(cqspi, 1, f_pdata->read_delay);
+ cqspi_readdata_capture(cqspi, !cqspi->rclk_en,
+ f_pdata->read_delay);
}
if (switch_cs || switch_ck)
@@ -1049,6 +1051,8 @@ static int cqspi_of_get_pdata(struct platform_device *pdev)
return -ENXIO;
}
+ cqspi->rclk_en = of_property_read_bool(np, "cdns,rclk-en");
+
return 0;
}
--
2.14.1
On 09/24/2017 12:59 PM, Vignesh R wrote:
> Add pm_runtime* calls to cadence-quadspi driver. This is required to
> switch on QSPI power domain on TI 66AK2G SoC during probe.
>
> Signed-off-by: Vignesh R <[email protected]>
Are you planning to add some more fine-grained PM control later?
> ---
> drivers/mtd/spi-nor/cadence-quadspi.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
> index d9629e8f4798..2c8e6226d267 100644
> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -31,6 +31,7 @@
> #include <linux/of_device.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> #include <linux/sched.h>
> #include <linux/spi/spi.h>
> #include <linux/timer.h>
> @@ -1224,6 +1225,13 @@ static int cqspi_probe(struct platform_device *pdev)
> return -ENXIO;
> }
>
> + pm_runtime_enable(&pdev->dev);
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(&pdev->dev);
> + return ret;
> + }
> +
> ret = clk_prepare_enable(cqspi->clk);
> if (ret) {
> dev_err(dev, "Cannot enable QSPI clock.\n");
> @@ -1275,6 +1283,9 @@ static int cqspi_remove(struct platform_device *pdev)
>
> clk_disable_unprepare(cqspi->clk);
>
> + pm_runtime_put_sync(&pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> +
> return 0;
> }
>
>
--
Best regards,
Marek Vasut
On 09/24/2017 12:59 PM, Vignesh R wrote:
> As per 66AK2G02 TRM[1] SPRUHY8F section 11.15.5.3 Indirect Access
> Controller programming sequence, a delay equal to couple of QSPI master
> clock(~5ns) is required after setting CQSPI_REG_INDIRECTWR_START bit and
> writing data to the flash. Introduce a quirk flag CQSPI_NEEDS_WR_DELAY
> to handle this and set this flag for TI 66AK2G SoC.
>
> [1]http://www.ti.com/lit/ug/spruhy8f/spruhy8f.pdf
>
> Signed-off-by: Vignesh R <[email protected]>
Is this TI specific or is this controller property ? I wouldn't be
surprised of the later ...
> ---
>
> v3:
> Fix build warnings reported by kbuild test bot.
>
> drivers/mtd/spi-nor/cadence-quadspi.c | 27 ++++++++++++++++++++++++++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
> index 53c7d8e0327a..5cd5d6f7303f 100644
> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -38,6 +38,9 @@
> #define CQSPI_NAME "cadence-qspi"
> #define CQSPI_MAX_CHIPSELECT 16
>
> +/* Quirks */
> +#define CQSPI_NEEDS_WR_DELAY BIT(0)
> +
> struct cqspi_st;
>
> struct cqspi_flash_pdata {
> @@ -76,6 +79,7 @@ struct cqspi_st {
> u32 fifo_depth;
> u32 fifo_width;
> u32 trigger_address;
> + u32 wr_delay;
> struct cqspi_flash_pdata f_pdata[CQSPI_MAX_CHIPSELECT];
> };
>
> @@ -608,6 +612,15 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor,
> reinit_completion(&cqspi->transfer_complete);
> writel(CQSPI_REG_INDIRECTWR_START_MASK,
> reg_base + CQSPI_REG_INDIRECTWR);
> + /*
> + * As per 66AK2G02 TRM SPRUHY8F section 11.15.5.3 Indirect Access
> + * Controller programming sequence, couple of cycles of
> + * QSPI_REF_CLK delay is required for the above bit to
> + * be internally synchronized by the QSPI module. Provide 5
> + * cycles of delay.
> + */
> + if (cqspi->wr_delay)
> + ndelay(cqspi->wr_delay);
>
> while (remaining > 0) {
> write_bytes = remaining > page_size ? page_size : remaining;
> @@ -1156,6 +1169,7 @@ static int cqspi_probe(struct platform_device *pdev)
> struct cqspi_st *cqspi;
> struct resource *res;
> struct resource *res_ahb;
> + unsigned long data;
> int ret;
> int irq;
>
> @@ -1213,6 +1227,10 @@ static int cqspi_probe(struct platform_device *pdev)
> }
>
> cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk);
> + data = (unsigned long)of_device_get_match_data(dev);
> + if (data & CQSPI_NEEDS_WR_DELAY)
> + cqspi->wr_delay = 5 * DIV_ROUND_UP(NSEC_PER_SEC,
> + cqspi->master_ref_clk_hz);
>
> ret = devm_request_irq(dev, irq, cqspi_irq_handler, 0,
> pdev->name, cqspi);
> @@ -1284,7 +1302,14 @@ static const struct dev_pm_ops cqspi__dev_pm_ops = {
> #endif
>
> static const struct of_device_id cqspi_dt_ids[] = {
> - {.compatible = "cdns,qspi-nor",},
> + {
> + .compatible = "cdns,qspi-nor",
> + .data = (void *)0,
> + },
> + {
> + .compatible = "ti,k2g-qspi",
> + .data = (void *)CQSPI_NEEDS_WR_DELAY,
> + },
> { /* end of table */ }
> };
>
>
--
Best regards,
Marek Vasut
On 9/24/2017 5:29 PM, Marek Vasut wrote:
> On 09/24/2017 12:59 PM, Vignesh R wrote:
>> As per 66AK2G02 TRM[1] SPRUHY8F section 11.15.5.3 Indirect Access
>> Controller programming sequence, a delay equal to couple of QSPI master
>> clock(~5ns) is required after setting CQSPI_REG_INDIRECTWR_START bit and
>> writing data to the flash. Introduce a quirk flag CQSPI_NEEDS_WR_DELAY
>> to handle this and set this flag for TI 66AK2G SoC.
>>
>> [1]http://www.ti.com/lit/ug/spruhy8f/spruhy8f.pdf
>>
>> Signed-off-by: Vignesh R <[email protected]>
>
> Is this TI specific or is this controller property ? I wouldn't be
> surprised of the later ...
I am not sure, there is no generic public documentation by cadence for
this IP. TI TRM clearly states this delay is required and I have
verified it practically that this delay is indeed needed.
But current user of this IP, socfpga does not seem to mention anything
about it. So, I guess its TI specific quirk.
>
>> ---
>>
>> v3:
>> Fix build warnings reported by kbuild test bot.
>>
>> drivers/mtd/spi-nor/cadence-quadspi.c | 27 ++++++++++++++++++++++++++-
>> 1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
>> index 53c7d8e0327a..5cd5d6f7303f 100644
>> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
>> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
>> @@ -38,6 +38,9 @@
>> #define CQSPI_NAME "cadence-qspi"
>> #define CQSPI_MAX_CHIPSELECT 16
>>
>> +/* Quirks */
>> +#define CQSPI_NEEDS_WR_DELAY BIT(0)
>> +
>> struct cqspi_st;
>>
>> struct cqspi_flash_pdata {
>> @@ -76,6 +79,7 @@ struct cqspi_st {
>> u32 fifo_depth;
>> u32 fifo_width;
>> u32 trigger_address;
>> + u32 wr_delay;
>> struct cqspi_flash_pdata f_pdata[CQSPI_MAX_CHIPSELECT];
>> };
>>
>> @@ -608,6 +612,15 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor,
>> reinit_completion(&cqspi->transfer_complete);
>> writel(CQSPI_REG_INDIRECTWR_START_MASK,
>> reg_base + CQSPI_REG_INDIRECTWR);
>> + /*
>> + * As per 66AK2G02 TRM SPRUHY8F section 11.15.5.3 Indirect Access
>> + * Controller programming sequence, couple of cycles of
>> + * QSPI_REF_CLK delay is required for the above bit to
>> + * be internally synchronized by the QSPI module. Provide 5
>> + * cycles of delay.
>> + */
>> + if (cqspi->wr_delay)
>> + ndelay(cqspi->wr_delay);
>>
>> while (remaining > 0) {
>> write_bytes = remaining > page_size ? page_size : remaining;
>> @@ -1156,6 +1169,7 @@ static int cqspi_probe(struct platform_device *pdev)
>> struct cqspi_st *cqspi;
>> struct resource *res;
>> struct resource *res_ahb;
>> + unsigned long data;
>> int ret;
>> int irq;
>>
>> @@ -1213,6 +1227,10 @@ static int cqspi_probe(struct platform_device *pdev)
>> }
>>
>> cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk);
>> + data = (unsigned long)of_device_get_match_data(dev);
>> + if (data & CQSPI_NEEDS_WR_DELAY)
>> + cqspi->wr_delay = 5 * DIV_ROUND_UP(NSEC_PER_SEC,
>> + cqspi->master_ref_clk_hz);
>>
>> ret = devm_request_irq(dev, irq, cqspi_irq_handler, 0,
>> pdev->name, cqspi);
>> @@ -1284,7 +1302,14 @@ static const struct dev_pm_ops cqspi__dev_pm_ops = {
>> #endif
>>
>> static const struct of_device_id cqspi_dt_ids[] = {
>> - {.compatible = "cdns,qspi-nor",},
>> + {
>> + .compatible = "cdns,qspi-nor",
>> + .data = (void *)0,
>> + },
>> + {
>> + .compatible = "ti,k2g-qspi",
>> + .data = (void *)CQSPI_NEEDS_WR_DELAY,
>> + },
>> { /* end of table */ }
>> };
>>
>>
>
>
On 9/24/2017 5:31 PM, Marek Vasut wrote:
> On 09/24/2017 12:59 PM, Vignesh R wrote:
>> Add pm_runtime* calls to cadence-quadspi driver. This is required to
>> switch on QSPI power domain on TI 66AK2G SoC during probe.
>>
>> Signed-off-by: Vignesh R <[email protected]>
>
> Are you planning to add some more fine-grained PM control later?
Yes, I will need to add fine-grained PM control at some point. But, for
now SoC does not really support low power mode or runtime power saving
option.
The fact that driver still uses clk_prepare_*() calls to enable/disable
clocks instead of pm_*() calls makes it a bit tricky though.
Just figured out I forgot to add cleanup code in error handling path of
probe(). Will fix that and send a v4.
>
>> ---
>> drivers/mtd/spi-nor/cadence-quadspi.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
>> index d9629e8f4798..2c8e6226d267 100644
>> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
>> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
>> @@ -31,6 +31,7 @@
>> #include <linux/of_device.h>
>> #include <linux/of.h>
>> #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> #include <linux/sched.h>
>> #include <linux/spi/spi.h>
>> #include <linux/timer.h>
>> @@ -1224,6 +1225,13 @@ static int cqspi_probe(struct platform_device *pdev)
>> return -ENXIO;
>> }
>>
>> + pm_runtime_enable(&pdev->dev);
>> + ret = pm_runtime_get_sync(&pdev->dev);
>> + if (ret < 0) {
>> + pm_runtime_put_noidle(&pdev->dev);
>> + return ret;
>> + }
>> +
>> ret = clk_prepare_enable(cqspi->clk);
>> if (ret) {
>> dev_err(dev, "Cannot enable QSPI clock.\n");
>> @@ -1275,6 +1283,9 @@ static int cqspi_remove(struct platform_device *pdev)
>>
>> clk_disable_unprepare(cqspi->clk);
>>
>> + pm_runtime_put_sync(&pdev->dev);
>> + pm_runtime_disable(&pdev->dev);
>> +
>> return 0;
>> }
>>
>>
>
>
On 09/24/2017 03:08 PM, Vignesh R wrote:
>
>
> On 9/24/2017 5:31 PM, Marek Vasut wrote:
>> On 09/24/2017 12:59 PM, Vignesh R wrote:
>>> Add pm_runtime* calls to cadence-quadspi driver. This is required to
>>> switch on QSPI power domain on TI 66AK2G SoC during probe.
>>>
>>> Signed-off-by: Vignesh R <[email protected]>
>>
>> Are you planning to add some more fine-grained PM control later?
>
> Yes, I will need to add fine-grained PM control at some point. But, for
> now SoC does not really support low power mode or runtime power saving
> option.
> The fact that driver still uses clk_prepare_*() calls to enable/disable
> clocks instead of pm_*() calls makes it a bit tricky though.
>
> Just figured out I forgot to add cleanup code in error handling path of
> probe(). Will fix that and send a v4.
OK, fine. Cleanups are welcome. The SoCFPGA doesn't do much runtime PM
either, so it's fine for now.
--
Best regards,
Marek Vasut
On 09/24/2017 02:33 PM, Vignesh R wrote:
>
>
> On 9/24/2017 5:29 PM, Marek Vasut wrote:
>> On 09/24/2017 12:59 PM, Vignesh R wrote:
>>> As per 66AK2G02 TRM[1] SPRUHY8F section 11.15.5.3 Indirect Access
>>> Controller programming sequence, a delay equal to couple of QSPI master
>>> clock(~5ns) is required after setting CQSPI_REG_INDIRECTWR_START bit and
>>> writing data to the flash. Introduce a quirk flag CQSPI_NEEDS_WR_DELAY
>>> to handle this and set this flag for TI 66AK2G SoC.
>>>
>>> [1]http://www.ti.com/lit/ug/spruhy8f/spruhy8f.pdf
>>>
>>> Signed-off-by: Vignesh R <[email protected]>
>>
>> Is this TI specific or is this controller property ? I wouldn't be
>> surprised of the later ...
>
> I am not sure, there is no generic public documentation by cadence for
> this IP. TI TRM clearly states this delay is required and I have
> verified it practically that this delay is indeed needed.
> But current user of this IP, socfpga does not seem to mention anything
> about it. So, I guess its TI specific quirk.
OK, let's go with that then. I didn't observe any stability issues with
SoCFPGA, but I didn't run the flash at 100s of MHz either. At what kind
of frequencies does the quirk become relevant ?
>>
>>> ---
>>>
>>> v3:
>>> Fix build warnings reported by kbuild test bot.
>>>
>>> drivers/mtd/spi-nor/cadence-quadspi.c | 27 ++++++++++++++++++++++++++-
>>> 1 file changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
>>> index 53c7d8e0327a..5cd5d6f7303f 100644
>>> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
>>> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
>>> @@ -38,6 +38,9 @@
>>> #define CQSPI_NAME "cadence-qspi"
>>> #define CQSPI_MAX_CHIPSELECT 16
>>>
>>> +/* Quirks */
>>> +#define CQSPI_NEEDS_WR_DELAY BIT(0)
>>> +
>>> struct cqspi_st;
>>>
>>> struct cqspi_flash_pdata {
>>> @@ -76,6 +79,7 @@ struct cqspi_st {
>>> u32 fifo_depth;
>>> u32 fifo_width;
>>> u32 trigger_address;
>>> + u32 wr_delay;
>>> struct cqspi_flash_pdata f_pdata[CQSPI_MAX_CHIPSELECT];
>>> };
>>>
>>> @@ -608,6 +612,15 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor,
>>> reinit_completion(&cqspi->transfer_complete);
>>> writel(CQSPI_REG_INDIRECTWR_START_MASK,
>>> reg_base + CQSPI_REG_INDIRECTWR);
>>> + /*
>>> + * As per 66AK2G02 TRM SPRUHY8F section 11.15.5.3 Indirect Access
>>> + * Controller programming sequence, couple of cycles of
>>> + * QSPI_REF_CLK delay is required for the above bit to
>>> + * be internally synchronized by the QSPI module. Provide 5
>>> + * cycles of delay.
>>> + */
>>> + if (cqspi->wr_delay)
>>> + ndelay(cqspi->wr_delay);
>>>
>>> while (remaining > 0) {
>>> write_bytes = remaining > page_size ? page_size : remaining;
>>> @@ -1156,6 +1169,7 @@ static int cqspi_probe(struct platform_device *pdev)
>>> struct cqspi_st *cqspi;
>>> struct resource *res;
>>> struct resource *res_ahb;
>>> + unsigned long data;
>>> int ret;
>>> int irq;
>>>
>>> @@ -1213,6 +1227,10 @@ static int cqspi_probe(struct platform_device *pdev)
>>> }
>>>
>>> cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk);
>>> + data = (unsigned long)of_device_get_match_data(dev);
>>> + if (data & CQSPI_NEEDS_WR_DELAY)
>>> + cqspi->wr_delay = 5 * DIV_ROUND_UP(NSEC_PER_SEC,
>>> + cqspi->master_ref_clk_hz);
>>>
>>> ret = devm_request_irq(dev, irq, cqspi_irq_handler, 0,
>>> pdev->name, cqspi);
>>> @@ -1284,7 +1302,14 @@ static const struct dev_pm_ops cqspi__dev_pm_ops = {
>>> #endif
>>>
>>> static const struct of_device_id cqspi_dt_ids[] = {
>>> - {.compatible = "cdns,qspi-nor",},
>>> + {
>>> + .compatible = "cdns,qspi-nor",
>>> + .data = (void *)0,
>>> + },
>>> + {
>>> + .compatible = "ti,k2g-qspi",
>>> + .data = (void *)CQSPI_NEEDS_WR_DELAY,
>>> + },
>>> { /* end of table */ }
>>> };
>>>
>>>
>>
>>
--
Best regards,
Marek Vasut
On 9/24/2017 6:42 PM, Marek Vasut wrote:
> On 09/24/2017 03:08 PM, Vignesh R wrote:
>>
>>
>> On 9/24/2017 5:31 PM, Marek Vasut wrote:
>>> On 09/24/2017 12:59 PM, Vignesh R wrote:
>>>> Add pm_runtime* calls to cadence-quadspi driver. This is required to
>>>> switch on QSPI power domain on TI 66AK2G SoC during probe.
>>>>
>>>> Signed-off-by: Vignesh R <[email protected]>
>>>
>>> Are you planning to add some more fine-grained PM control later?
>>
>> Yes, I will need to add fine-grained PM control at some point. But, for
>> now SoC does not really support low power mode or runtime power saving
>> option.
>> The fact that driver still uses clk_prepare_*() calls to enable/disable
>> clocks instead of pm_*() calls makes it a bit tricky though.
>>
>> Just figured out I forgot to add cleanup code in error handling path of
>> probe(). Will fix that and send a v4.
>
> OK, fine. Cleanups are welcome. The SoCFPGA doesn't do much runtime PM
> either, so it's fine for now.
>
Ok thanks! Do you know if pm_runtime_get_sync() can enable clocks for
QSPI on SoCFPGA or if clk_prepare_enable() is needed? Just trying to see
if its possible to get rid of clk_*() calls in favor of pm_*() calls.
On 09/24/2017 03:27 PM, Vignesh R wrote:
>
>
> On 9/24/2017 6:42 PM, Marek Vasut wrote:
>> On 09/24/2017 03:08 PM, Vignesh R wrote:
>>>
>>>
>>> On 9/24/2017 5:31 PM, Marek Vasut wrote:
>>>> On 09/24/2017 12:59 PM, Vignesh R wrote:
>>>>> Add pm_runtime* calls to cadence-quadspi driver. This is required to
>>>>> switch on QSPI power domain on TI 66AK2G SoC during probe.
>>>>>
>>>>> Signed-off-by: Vignesh R <[email protected]>
>>>>
>>>> Are you planning to add some more fine-grained PM control later?
>>>
>>> Yes, I will need to add fine-grained PM control at some point. But, for
>>> now SoC does not really support low power mode or runtime power saving
>>> option.
>>> The fact that driver still uses clk_prepare_*() calls to enable/disable
>>> clocks instead of pm_*() calls makes it a bit tricky though.
>>>
>>> Just figured out I forgot to add cleanup code in error handling path of
>>> probe(). Will fix that and send a v4.
>>
>> OK, fine. Cleanups are welcome. The SoCFPGA doesn't do much runtime PM
>> either, so it's fine for now.
>>
>
> Ok thanks! Do you know if pm_runtime_get_sync() can enable clocks for
> QSPI on SoCFPGA or if clk_prepare_enable() is needed? Just trying to see
> if its possible to get rid of clk_*() calls in favor of pm_*() calls.
Not of the top of my head, sorry. +CC Matthew, he should know.
--
Best regards,
Marek Vasut
On Sun, 24 Sep 2017, Marek Vasut wrote:
> On 09/24/2017 03:27 PM, Vignesh R wrote:
>>
>>
>> On 9/24/2017 6:42 PM, Marek Vasut wrote:
>>> On 09/24/2017 03:08 PM, Vignesh R wrote:
>>>>
>>>>
>>>> On 9/24/2017 5:31 PM, Marek Vasut wrote:
>>>>> On 09/24/2017 12:59 PM, Vignesh R wrote:
>>>>>> Add pm_runtime* calls to cadence-quadspi driver. This is required to
>>>>>> switch on QSPI power domain on TI 66AK2G SoC during probe.
>>>>>>
>>>>>> Signed-off-by: Vignesh R <[email protected]>
>>>>>
>>>>> Are you planning to add some more fine-grained PM control later?
>>>>
>>>> Yes, I will need to add fine-grained PM control at some point. But, for
>>>> now SoC does not really support low power mode or runtime power saving
>>>> option.
>>>> The fact that driver still uses clk_prepare_*() calls to enable/disable
>>>> clocks instead of pm_*() calls makes it a bit tricky though.
>>>>
>>>> Just figured out I forgot to add cleanup code in error handling path of
>>>> probe(). Will fix that and send a v4.
>>>
>>> OK, fine. Cleanups are welcome. The SoCFPGA doesn't do much runtime PM
>>> either, so it's fine for now.
>>>
>>
>> Ok thanks! Do you know if pm_runtime_get_sync() can enable clocks for
>> QSPI on SoCFPGA or if clk_prepare_enable() is needed? Just trying to see
>> if its possible to get rid of clk_*() calls in favor of pm_*() calls.
>
> Not of the top of my head, sorry. +CC Matthew, he should know.
I am not an expert at the clock framework nor the power management, but I
did ask around a bit. No one I asked was planning to change the clk_*()
calls to pm_*() call, but the feedback was that it would be a good idea.
Matthew Gerlach
>
> --
> Best regards,
> Marek Vasut
>
On 09/26/2017 12:41 AM, [email protected] wrote:
>
>
> On Sun, 24 Sep 2017, Marek Vasut wrote:
>
>> On 09/24/2017 03:27 PM, Vignesh R wrote:
>>>
>>>
>>> On 9/24/2017 6:42 PM, Marek Vasut wrote:
>>>> On 09/24/2017 03:08 PM, Vignesh R wrote:
>>>>>
>>>>>
>>>>> On 9/24/2017 5:31 PM, Marek Vasut wrote:
>>>>>> On 09/24/2017 12:59 PM, Vignesh R wrote:
>>>>>>> Add pm_runtime* calls to cadence-quadspi driver. This is required to
>>>>>>> switch on QSPI power domain on TI 66AK2G SoC during probe.
>>>>>>>
>>>>>>> Signed-off-by: Vignesh R <[email protected]>
>>>>>>
>>>>>> Are you planning to add some more fine-grained PM control later?
>>>>>
>>>>> Yes, I will need to add fine-grained PM control at some point. But,
>>>>> for
>>>>> now SoC does not really support low power mode or runtime power saving
>>>>> option.
>>>>> The fact that driver still uses clk_prepare_*() calls to
>>>>> enable/disable
>>>>> clocks instead of pm_*() calls makes it a bit tricky though.
>>>>>
>>>>> Just figured out I forgot to add cleanup code in error handling
>>>>> path of
>>>>> probe(). Will fix that and send a v4.
>>>>
>>>> OK, fine. Cleanups are welcome. The SoCFPGA doesn't do much runtime PM
>>>> either, so it's fine for now.
>>>>
>>>
>>> Ok thanks! Do you know if pm_runtime_get_sync() can enable clocks for
>>> QSPI on SoCFPGA or if clk_prepare_enable() is needed? Just trying to see
>>> if its possible to get rid of clk_*() calls in favor of pm_*() calls.
>>
>> Not of the top of my head, sorry. +CC Matthew, he should know.
>
> I am not an expert at the clock framework nor the power management, but I
> did ask around a bit. No one I asked was planning to change the clk_*()
> calls to pm_*() call, but the feedback was that it would be a good idea.
The question is, if we do the replacement, will it break on socfpga ?
A quick test might be useful.
--
Best regards,
Marek Vasut
Hi Matthew,
On Tuesday 26 September 2017 05:19 AM, Marek Vasut wrote:
[...]
>>>> Ok thanks! Do you know if pm_runtime_get_sync() can enable clocks for
>>>> QSPI on SoCFPGA or if clk_prepare_enable() is needed? Just trying to see
>>>> if its possible to get rid of clk_*() calls in favor of pm_*() calls.
>>>
>>> Not of the top of my head, sorry. +CC Matthew, he should know.
>>
>> I am not an expert at the clock framework nor the power management, but I
>> did ask around a bit. No one I asked was planning to change the clk_*()
>> calls to pm_*() call, but the feedback was that it would be a good idea.
>
> The question is, if we do the replacement, will it break on socfpga ?
> A quick test might be useful.
>
yes, a quick qspi test with clk_prepare_enable() replaced by pm_*() calls
like below patch would be helpful:
diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index 53c7d8e0327a..7ad3e176cc88 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -34,6 +34,7 @@
#include <linux/sched.h>
#include <linux/spi/spi.h>
#include <linux/timer.h>
+#include <linux/pm_runtime.h>
#define CQSPI_NAME "cadence-qspi"
#define CQSPI_MAX_CHIPSELECT 16
@@ -1206,11 +1207,8 @@ static int cqspi_probe(struct platform_device *pdev)
return -ENXIO;
}
- ret = clk_prepare_enable(cqspi->clk);
- if (ret) {
- dev_err(dev, "Cannot enable QSPI clock.\n");
- return ret;
- }
+ pm_runtime_enable(dev);
+ pm_runtime_get_sync(dev);
cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk);
--
Regards
Vignesh
Hi Vignesh,
I tried this patch on an Arria10 SOCFPGA devkit against the 4.1.33-ltsi
kernel, and it did not go well. Commands to the flash chip timedout
resulting in the probe function failing. I ran into other problems, not
related to cadence-quadspi, that prevented me from testing against 4.9 and
4.12 kernels, but I suspect similar behavior.
Matthew Gerlach
On Wed, 27 Sep 2017, Vignesh R wrote:
> Hi Matthew,
>
> On Tuesday 26 September 2017 05:19 AM, Marek Vasut wrote:
> [...]
>>>>> Ok thanks! Do you know if pm_runtime_get_sync() can enable clocks for
>>>>> QSPI on SoCFPGA or if clk_prepare_enable() is needed? Just trying to see
>>>>> if its possible to get rid of clk_*() calls in favor of pm_*() calls.
>>>>
>>>> Not of the top of my head, sorry. +CC Matthew, he should know.
>>>
>>> I am not an expert at the clock framework nor the power management, but I
>>> did ask around a bit. No one I asked was planning to change the clk_*()
>>> calls to pm_*() call, but the feedback was that it would be a good idea.
>>
>> The question is, if we do the replacement, will it break on socfpga ?
>> A quick test might be useful.
>>
>
> yes, a quick qspi test with clk_prepare_enable() replaced by pm_*() calls
> like below patch would be helpful:
>
>
> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
> index 53c7d8e0327a..7ad3e176cc88 100644
> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -34,6 +34,7 @@
> #include <linux/sched.h>
> #include <linux/spi/spi.h>
> #include <linux/timer.h>
> +#include <linux/pm_runtime.h>
>
> #define CQSPI_NAME "cadence-qspi"
> #define CQSPI_MAX_CHIPSELECT 16
> @@ -1206,11 +1207,8 @@ static int cqspi_probe(struct platform_device *pdev)
> return -ENXIO;
> }
>
> - ret = clk_prepare_enable(cqspi->clk);
> - if (ret) {
> - dev_err(dev, "Cannot enable QSPI clock.\n");
> - return ret;
> - }
> + pm_runtime_enable(dev);
> + pm_runtime_get_sync(dev);
>
> cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk);
>
>
>
>
>
> --
> Regards
> Vignesh
>