Re: [PATCH v1 2/2] mtd: spi-nor: cadence-quadspi: disable the DMA, DAC and auto poll

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Vignesh,

On 22/8/2019 5:03 PM, Vignesh Raghavendra wrote:
Hi,

On 19/08/19 5:24 PM, Ramuthevar, Vadivel MuruganX wrote:
From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx>

On Intel Lightening Mountain(LGM) SoCs QSPI controller do not use
s/Lightening/Lightning

Thank you so much for the review comments and your time.

(Intel -> Intel's , Lightening -> Lightning) will update

Direct Memory Access(DMA), Direct Access Controller(DAC) and
auto-poll features. This patch introduces to properly disable DMA,
DAC for data transfer instead it uses indirect data transfer.
and also auto polling.

Please split into two patch, one disabling DAC mode and DMA and other
disabling auto polling feature.
Agreed!
Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx>
---
  drivers/mtd/spi-nor/Kconfig           |  2 +-
  drivers/mtd/spi-nor/cadence-quadspi.c | 62 ++++++++++++++++++++++++++++++-----
  2 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 6de83277ce8b..ba2e372ae514 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -34,7 +34,7 @@ config SPI_ASPEED_SMC
config SPI_CADENCE_QUADSPI
  	tristate "Cadence Quad SPI controller"
-	depends on OF && (ARM || ARM64 || COMPILE_TEST)
+	depends on OF && (ARM || ARM64 || COMPILE_TEST || X86)
  	help
  	  Enable support for the Cadence Quad SPI Flash controller.
diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index 67f15a1f16fd..83ae28e055f4 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -67,6 +67,7 @@ struct cqspi_st {
void __iomem *iobase;
  	void __iomem		*ahb_base;
+	resource_size_t		ahb_phy_addr;
What does this represent? trigger address to be programmed in
CQSPI_REG_INDIRECTTRIGGER reg? If so why not use cdns,trigger-address
property?

 indirect operations are controlled and triggered by software via specific APB control/configuration registers instead of AHB access  to flash data.

Yes , sure will use the trigger-address property.

  	resource_size_t		ahb_size;
  	struct completion	transfer_complete;
  	struct mutex		bus_mutex;
@@ -134,6 +135,8 @@ struct cqspi_driver_platdata {
  #define CQSPI_REG_RD_INSTR_TYPE_DATA_MASK	0x3
  #define CQSPI_REG_RD_INSTR_DUMMY_MASK		0x1F
+#define CQSPI_REG_WR_COMPLETION_CTRL 0x38
+#define CQSPI_REG_WR_COMPLETION_DISABLE_AUTO_POLL	BIT(14)
  #define CQSPI_REG_WR_INSTR			0x08
  #define CQSPI_REG_WR_INSTR_OPCODE_LSB		0
  #define CQSPI_REG_WR_INSTR_TYPE_ADDR_LSB	12
@@ -214,6 +217,7 @@ struct cqspi_driver_platdata {
  #define CQSPI_REG_INDIRECTWRWATERMARK		0x74
  #define CQSPI_REG_INDIRECTWRSTARTADDR		0x78
  #define CQSPI_REG_INDIRECTWRBYTES		0x7C
+#define CQSPI_REG_INDIRECTTRIGGERADDRRANGE	0x80
Where is this used? Lets not add defines without a code snippet using it.
agreed, remove it.
  #define CQSPI_REG_CMDADDRESS			0x94
  #define CQSPI_REG_CMDREADDATALOWER		0xA0
@@ -470,6 +474,18 @@ static int cqspi_command_write_addr(struct spi_nor *nor,
  	return cqspi_exec_flash_cmd(cqspi, reg);
  }
+static int cqspi_disable_auto_poll(struct cqspi_st *cqspi)
+{
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int reg;
+
+	reg = readl(reg_base + CQSPI_REG_WR_COMPLETION_CTRL);
+	reg |= CQSPI_REG_WR_COMPLETION_DISABLE_AUTO_POLL;
+	writel(reg, reg_base + CQSPI_REG_WR_COMPLETION_CTRL);
+
+	return 0;
+}
+
  static int cqspi_read_setup(struct spi_nor *nor)
  {
  	struct cqspi_flash_pdata *f_pdata = nor->priv;
@@ -507,6 +523,11 @@ static int cqspi_read_setup(struct spi_nor *nor)
  	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
  	reg |= (nor->addr_width - 1);
  	writel(reg, reg_base + CQSPI_REG_SIZE);
+
+	/* Disable auto-polling */
+	if (!f_pdata->use_direct_mode)
+		cqspi_disable_auto_poll(cqspi);
+
This is a one time setup. So move this to cqspi_controller_init(). More
comments at the end of the patch
Agreed
  	return 0;
  }
@@ -517,12 +538,16 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
  	struct cqspi_st *cqspi = f_pdata->cqspi;
  	void __iomem *reg_base = cqspi->iobase;
  	void __iomem *ahb_base = cqspi->ahb_base;
+	resource_size_t ahb_phy_addr = cqspi->ahb_phy_addr;
  	unsigned int remaining = n_rx;
  	unsigned int mod_bytes = n_rx % 4;
  	unsigned int bytes_to_read = 0;
  	u8 *rxbuf_end = rxbuf + n_rx;
  	int ret = 0;
+ if (!f_pdata->use_direct_mode)
+		writel(ahb_phy_addr, reg_base + CQSPI_REG_INDIRECTTRIGGER);
+
Drop this and use cdns,trigger-address DT property which takes care of
programming CQSPI_REG_INDIRECTTRIGGER in cqspi_controller_init()
Agreed
  	writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
  	writel(remaining, reg_base + CQSPI_REG_INDIRECTRDBYTES);
@@ -609,6 +634,14 @@ static int cqspi_write_setup(struct spi_nor *nor)
  	struct cqspi_st *cqspi = f_pdata->cqspi;
  	void __iomem *reg_base = cqspi->iobase;
+ /* Disable the DMA and direct access controller */
+	if (!f_pdata->use_direct_mode) {
+		reg = readl(reg_base + CQSPI_REG_CONFIG);
+		reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
+		reg &= ~CQSPI_REG_CONFIG_DMA_MASK;
DMA is disabled by default right? No need to clear it explicitly.

On Intel LGM SoC there is no DMA, in-case of it if enabled , system may crash that is the reason, we are disabling it.

Let me double confirm the code changes and validate on platform.

+		writel(reg, reg_base + CQSPI_REG_CONFIG);
+	}
+
And Move this hunk to cqspi_controller_init()
Sure
  	/* Set opcode. */
  	reg = nor->program_opcode << CQSPI_REG_WR_INSTR_OPCODE_LSB;
  	writel(reg, reg_base + CQSPI_REG_WR_INSTR);
@@ -619,6 +652,11 @@ static int cqspi_write_setup(struct spi_nor *nor)
  	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
  	reg |= (nor->addr_width - 1);
  	writel(reg, reg_base + CQSPI_REG_SIZE);
+
+	/* Disable auto-polling */
+	if (!f_pdata->use_direct_mode)
+		cqspi_disable_auto_poll(cqspi);
+
  	return 0;
  }
@@ -1165,13 +1203,16 @@ static int cqspi_of_get_pdata(struct platform_device *pdev)
  		return -ENXIO;
  	}
- if (of_property_read_u32(np, "cdns,trigger-address",
-				 &cqspi->trigger_address)) {
-		dev_err(&pdev->dev, "couldn't determine trigger-address\n");
-		return -ENXIO;
-	}
+	if (!of_device_is_compatible(np, "intel,lgm-qspi")) {
+		if (of_property_read_u32(np, "cdns,trigger-address",
+					 &cqspi->trigger_address)) {
+			dev_err(&pdev->dev,
+				"couldn't determine trigger-address\n");
+			return -ENXIO;
+		}
Why? Can you populate cdns,trigger-address with same address as
res_ahb->start? That should eliminate need for cqspi->ahb_phy_addr

functionality not worked during the development time, better check and validate, try to use

the same available as you have suggested.

-	cqspi->rclk_en = of_property_read_bool(np, "cdns,rclk-en");
+		cqspi->rclk_en = of_property_read_bool(np, "cdns,rclk-en");
+	}
return 0;
  }
@@ -1301,7 +1342,8 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
  		f_pdata->registered = true;
if (mtd->size <= cqspi->ahb_size) {
-			f_pdata->use_direct_mode = true;
+			f_pdata->use_direct_mode =
+				!(of_device_is_compatible(np, "intel,lgm-qspi"));
Instead, introduce a new quirk (similar to CQSPI_NEEDS_WR_DELAY) and
then use the flag to determine when to set use_direct_mode flag. Flag
can also be used to disable DAC mode in cqspi_controller_init().

And also use cqspi_driver_platdata to populate quirks for the  compatible
Agreed
  			dev_dbg(nor->dev, "using direct mode for %s\n",
  				mtd->name);
@@ -1347,7 +1389,10 @@ static int cqspi_probe(struct platform_device *pdev)
  	}
/* Obtain QSPI clock. */
-	cqspi->clk = devm_clk_get(dev, NULL);
+	if (of_device_is_compatible(np, "intel,lgm-qspi"))
+		cqspi->clk = devm_clk_get(dev, "qspi");
+	else
+		cqspi->clk = devm_clk_get(dev, NULL);
Does IP have more than 1 clock input? If yes, please document all input
clk names in Documentation/devicetree/bindings/mtd/cadence-quadspi.txt.

Also,  can be simplified to:

	cqspi->clk = devm_clk_get(dev, "qspi");
	if (IS_ERR(cqspi->clk))
		/* Try w/o clk id */
		cqspi->clk = devm_clk_get(dev, NULL);

If there is only one clk input in DT, then just drop above code.

IP need only one clock.

will update and address your comments in next version patch-set.

---
With Best Regards
Vadivel Murugan
  	if (IS_ERR(cqspi->clk)) {
  		dev_err(dev, "Cannot claim QSPI clock.\n");
  		return PTR_ERR(cqspi->clk);
@@ -1369,6 +1414,7 @@ static int cqspi_probe(struct platform_device *pdev)
  		return PTR_ERR(cqspi->ahb_base);
  	}
  	cqspi->mmap_phys_base = (dma_addr_t)res_ahb->start;
+	cqspi->ahb_phy_addr = res_ahb->start;
  	cqspi->ahb_size = resource_size(res_ahb);
init_completion(&cqspi->transfer_complete);




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux