[PATCH v2 0/6] Implement setup_interface() in the DaVinci NAND controller

[PATCH v2 0/6] Implement setup_interface() in the DaVinci NAND controller

[Bastien Curutchet]

Hi all,

This patch series aims to implement the setup_interface() operation in
the DaVinci NAND controller to enable the use of all ONFI modes and
improve the NAND access speed.

PATCH 2 depends on PATCH 1
PATCH 6 depends on PATCH 2-4-5

This NAND controller is present in the DaVinci (OMAP L138) and Keystone2
SoCs and functions as a 'child' of the AEMIF controller. So its timings
are set by the AEMIF controller itself from device-tree properties.
Implementing the setup_interface() callback implies being able to update
dynamically these timings, so the first two patches of the series modify
the AEMIF driver to provide its 'children' a way to modify their chip
select timing configuration.

The remaining patches implement the setup_interface() operation.
The computation of the register's contents is directly based on
§20.3.2.3 of the OMAP-L138 TRM [1]

This has been tested on two platforms based upon the DaVinci SoC. One is
interfaced with a Macronix MX30UF4G18AC NAND, the other with a Toshiba
NAND.

[1] : https://www.ti.com/lit/ug/spruh77c/spruh77c.pdf

Change log:

 * v2
   Cover letter :
     - Add dependency details
     - Remove the question about ti-aemif.h location
   PATCH 1 :
     - Fix aemif_cs_timings's description in the comments
   PATCH 2 :
     - Fix typo in the config_cs_lock's description in the comments
     - Move include/memory/ti-aemif.h to include/linux/memory/ti-aemif.h
   PATCH 3 [NEW] :
     - Fix dependency issue with aemif controller in Kconfig
   PATCH 5 :
     - Add details about the clock bindings in the commit log
     - Replace devm_clk_get() with devm_clk_get_enabled()
     - Use dev_err_probe() to return the devm_clk_get_enabled() error

Bastien Curutchet (6):
  memory: ti-aemif: Create aemif_set_cs_timings()
  memory: ti-aemif: Export aemif_set_cs_timings()
  mtd: rawnand: davinci: Always depends on TI_AEMIF
  mtd: rawnand: davinci: Order headers alphabetically
  mtd: rawnand: davinci: Add clock resource
  mtd: rawnand: davinci: Implement setup_interface() operation

 drivers/memory/ti-aemif.c           | 102 +++++++++++++++++++---------
 drivers/mtd/nand/raw/Kconfig        |   4 +-
 drivers/mtd/nand/raw/davinci_nand.c |  93 +++++++++++++++++++++++--
 include/linux/memory/ti-aemif.h     |  31 +++++++++
 4 files changed, 191 insertions(+), 39 deletions(-)
 create mode 100644 include/linux/memory/ti-aemif.h

-- 
2.47.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 1/6] memory: ti-aemif: Create aemif_set_cs_timings()

[Bastien Curutchet]

Create an aemif_set_cs_timings() function to isolate the setting of a
chip select timing configuration and ease its exportation.
Move the check of the configuration validity from aemif_calc_rate() to
this new function.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/memory/ti-aemif.c | 111 ++++++++++++++++++++++++++------------
 1 file changed, 78 insertions(+), 33 deletions(-)

diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
index d54dc3cfff73..8d27b2513b2c 100644
--- a/drivers/memory/ti-aemif.c
+++ b/drivers/memory/ti-aemif.c
@@ -69,15 +69,15 @@
 #define ACR_SSTROBE_MASK	BIT(31)
 #define ASIZE_16BIT	1
 
-#define CONFIG_MASK	(TA(TA_MAX) | \
-				RHOLD(RHOLD_MAX) | \
-				RSTROBE(RSTROBE_MAX) |	\
-				RSETUP(RSETUP_MAX) | \
-				WHOLD(WHOLD_MAX) | \
-				WSTROBE(WSTROBE_MAX) | \
-				WSETUP(WSETUP_MAX) | \
-				EW(EW_MAX) | SSTROBE(SSTROBE_MAX) | \
-				ASIZE_MAX)
+#define TIMINGS_MASK	(TA(TA_MAX) | \
+			RHOLD(RHOLD_MAX) | \
+			RSTROBE(RSTROBE_MAX) |	\
+			RSETUP(RSETUP_MAX) | \
+			WHOLD(WHOLD_MAX) | \
+			WSTROBE(WSTROBE_MAX) | \
+			WSETUP(WSETUP_MAX))
+
+#define CONFIG_MASK	(EW(EW_MAX) | SSTROBE(SSTROBE_MAX) | ASIZE_MAX)
 
 /**
  * struct aemif_cs_data: structure to hold cs parameters
@@ -107,6 +107,27 @@ struct aemif_cs_data {
 	u8	asize;
 };
 
+/**
+ * struct aemif_cs_timings: structure to hold cs timing configuration
+ * values are expressed in number of clock cycles - 1
+ * @ta: minimum turn around time
+ * @rhold: read hold width
+ * @rstrobe: read strobe width
+ * @rsetup: read setup width
+ * @whold: write hold width
+ * @wstrobe: write strobe width
+ * @wsetup: write setup width
+ */
+struct aemif_cs_timings {
+	u32	ta;
+	u32	rhold;
+	u32	rstrobe;
+	u32	rsetup;
+	u32	whold;
+	u32	wstrobe;
+	u32	wsetup;
+};
+
 /**
  * struct aemif_device: structure to hold device data
  * @base: base address of AEMIF registers
@@ -125,6 +146,44 @@ struct aemif_device {
 	struct aemif_cs_data cs_data[NUM_CS];
 };
 
+/**
+ * aemif_set_cs_timings - Set the timing configuration of a given chip select.
+ * @aemif: aemif device to configure
+ * @cs: index of the chip select to configure.
+ * @timings: timings configuration to set
+ *
+ * Returns 0 on success, else negative errno.
+ */
+static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_cs_timings *timings)
+{
+	unsigned int offset;
+	u32 val, set;
+
+	if (!timings || !aemif)
+		return -EINVAL;
+
+	if (cs > aemif->num_cs)
+		return -EINVAL;
+
+	if (timings->ta > TA_MAX || timings->rhold > RHOLD_MAX || timings->rstrobe > RSTROBE_MAX ||
+	    timings->rsetup > RSETUP_MAX || timings->whold > WHOLD_MAX ||
+	    timings->wstrobe > WSTROBE_MAX || timings->wsetup > WSETUP_MAX)
+		return -EINVAL;
+
+	set = TA(timings->ta) | RHOLD(timings->rhold) | RSTROBE(timings->rstrobe) |
+	      RSETUP(timings->rsetup) | WHOLD(timings->whold) |
+	      WSTROBE(timings->wstrobe) | WSETUP(timings->wsetup);
+
+	offset = A1CR_OFFSET + cs * 4;
+
+	val = readl(aemif->base + offset);
+	val &= ~TIMINGS_MASK;
+	val |= set;
+	writel(val, aemif->base + offset);
+
+	return 0;
+}
+
 /**
  * aemif_calc_rate - calculate timing data.
  * @pdev: platform device to calculate for
@@ -149,10 +208,6 @@ static int aemif_calc_rate(struct platform_device *pdev, int wanted,
 	if (result < 0)
 		result = 0;
 
-	/* ... But configuring tighter timings is not an option. */
-	else if (result > max)
-		result = -EINVAL;
-
 	return result;
 }
 
@@ -174,32 +229,22 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
 {
 	struct aemif_device *aemif = platform_get_drvdata(pdev);
 	struct aemif_cs_data *data = &aemif->cs_data[csnum];
-	int ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup;
 	unsigned long clk_rate = aemif->clk_rate;
+	struct aemif_cs_timings timings;
 	unsigned offset;
 	u32 set, val;
 
 	offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
 
-	ta	= aemif_calc_rate(pdev, data->ta, clk_rate, TA_MAX);
-	rhold	= aemif_calc_rate(pdev, data->rhold, clk_rate, RHOLD_MAX);
-	rstrobe	= aemif_calc_rate(pdev, data->rstrobe, clk_rate, RSTROBE_MAX);
-	rsetup	= aemif_calc_rate(pdev, data->rsetup, clk_rate, RSETUP_MAX);
-	whold	= aemif_calc_rate(pdev, data->whold, clk_rate, WHOLD_MAX);
-	wstrobe	= aemif_calc_rate(pdev, data->wstrobe, clk_rate, WSTROBE_MAX);
-	wsetup	= aemif_calc_rate(pdev, data->wsetup, clk_rate, WSETUP_MAX);
-
-	if (ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 ||
-	    whold < 0 || wstrobe < 0 || wsetup < 0) {
-		dev_err(&pdev->dev, "%s: cannot get suitable timings\n",
-			__func__);
-		return -EINVAL;
-	}
-
-	set = TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) | RSETUP(rsetup) |
-		WHOLD(whold) | WSTROBE(wstrobe) | WSETUP(wsetup);
+	timings.ta = aemif_calc_rate(pdev, data->ta, clk_rate, TA_MAX);
+	timings.rhold = aemif_calc_rate(pdev, data->rhold, clk_rate, RHOLD_MAX);
+	timings.rstrobe = aemif_calc_rate(pdev, data->rstrobe, clk_rate, RSTROBE_MAX);
+	timings.rsetup = aemif_calc_rate(pdev, data->rsetup, clk_rate, RSETUP_MAX);
+	timings.whold = aemif_calc_rate(pdev, data->whold, clk_rate, WHOLD_MAX);
+	timings.wstrobe = aemif_calc_rate(pdev, data->wstrobe, clk_rate, WSTROBE_MAX);
+	timings.wsetup = aemif_calc_rate(pdev, data->wsetup, clk_rate, WSETUP_MAX);
 
-	set |= (data->asize & ACR_ASIZE_MASK);
+	set = (data->asize & ACR_ASIZE_MASK);
 	if (data->enable_ew)
 		set |= ACR_EW_MASK;
 	if (data->enable_ss)
@@ -210,7 +255,7 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
 	val |= set;
 	writel(val, aemif->base + offset);
 
-	return 0;
+	return aemif_set_cs_timings(aemif, data->cs - aemif->cs_offset, &timings);
 }
 
 static inline int aemif_cycles_to_nsec(int val, unsigned long clk_rate)
-- 
2.47.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/6] memory: ti-aemif: Create aemif_set_cs_timings()

[Miquel Raynal]

Hi Bastien,

2 very minor nits

> +/**
> + * struct aemif_cs_timings: structure to hold cs timing configuration

Nitpick: s/cs/CS/ in plain english?


...

> +/**
> + * aemif_set_cs_timings - Set the timing configuration of a given chip select.
> + * @aemif: aemif device to configure
> + * @cs: index of the chip select to configure.

Nit: Spurious dot                               ^


...

> -	/* ... But configuring tighter timings is not an option. */
> -	else if (result > max)
> -		result = -EINVAL;
> -

Maybe this is too early to remove the check? If someone bisects and
falls in-between the patches of your series, NAND support would be
broken I guess?

With this fixed, feel free to add my:

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 2/6] memory: ti-aemif: Export aemif_set_cs_timings()

[Bastien Curutchet]

Export the aemif_set_cs_timing() symbol so it can be used by other
drivers

Add a spinlock to protect the CS configuration register from concurrent
accesses.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/memory/ti-aemif.c       | 35 ++++++++++++---------------------
 include/linux/memory/ti-aemif.h | 31 +++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 22 deletions(-)
 create mode 100644 include/linux/memory/ti-aemif.h

diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
index 8d27b2513b2c..4587095aa703 100644
--- a/drivers/memory/ti-aemif.c
+++ b/drivers/memory/ti-aemif.c
@@ -13,10 +13,12 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/memory/ti-aemif.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
 
 #define TA_SHIFT	2
 #define RHOLD_SHIFT	4
@@ -107,27 +109,6 @@ struct aemif_cs_data {
 	u8	asize;
 };
 
-/**
- * struct aemif_cs_timings: structure to hold cs timing configuration
- * values are expressed in number of clock cycles - 1
- * @ta: minimum turn around time
- * @rhold: read hold width
- * @rstrobe: read strobe width
- * @rsetup: read setup width
- * @whold: write hold width
- * @wstrobe: write strobe width
- * @wsetup: write setup width
- */
-struct aemif_cs_timings {
-	u32	ta;
-	u32	rhold;
-	u32	rstrobe;
-	u32	rsetup;
-	u32	whold;
-	u32	wstrobe;
-	u32	wsetup;
-};
-
 /**
  * struct aemif_device: structure to hold device data
  * @base: base address of AEMIF registers
@@ -136,6 +117,7 @@ struct aemif_cs_timings {
  * @num_cs: number of assigned chip-selects
  * @cs_offset: start number of cs nodes
  * @cs_data: array of chip-select settings
+ * @config_cs_lock: lock used to access CS configuration
  */
 struct aemif_device {
 	void __iomem *base;
@@ -144,6 +126,7 @@ struct aemif_device {
 	u8 num_cs;
 	int cs_offset;
 	struct aemif_cs_data cs_data[NUM_CS];
+	spinlock_t config_cs_lock;
 };
 
 /**
@@ -154,8 +137,9 @@ struct aemif_device {
  *
  * Returns 0 on success, else negative errno.
  */
-static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_cs_timings *timings)
+int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_cs_timings *timings)
 {
+	unsigned long flags;
 	unsigned int offset;
 	u32 val, set;
 
@@ -176,13 +160,16 @@ static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_
 
 	offset = A1CR_OFFSET + cs * 4;
 
+	spin_lock_irqsave(&aemif->config_cs_lock, flags);
 	val = readl(aemif->base + offset);
 	val &= ~TIMINGS_MASK;
 	val |= set;
 	writel(val, aemif->base + offset);
+	spin_unlock_irqrestore(&aemif->config_cs_lock, flags);
 
 	return 0;
 }
+EXPORT_SYMBOL(aemif_set_cs_timings);
 
 /**
  * aemif_calc_rate - calculate timing data.
@@ -231,6 +218,7 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
 	struct aemif_cs_data *data = &aemif->cs_data[csnum];
 	unsigned long clk_rate = aemif->clk_rate;
 	struct aemif_cs_timings timings;
+	unsigned long flags;
 	unsigned offset;
 	u32 set, val;
 
@@ -250,10 +238,12 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
 	if (data->enable_ss)
 		set |= ACR_SSTROBE_MASK;
 
+	spin_lock_irqsave(&aemif->config_cs_lock, flags);
 	val = readl(aemif->base + offset);
 	val &= ~CONFIG_MASK;
 	val |= set;
 	writel(val, aemif->base + offset);
+	spin_unlock_irqrestore(&aemif->config_cs_lock, flags);
 
 	return aemif_set_cs_timings(aemif, data->cs - aemif->cs_offset, &timings);
 }
@@ -396,6 +386,7 @@ static int aemif_probe(struct platform_device *pdev)
 	if (IS_ERR(aemif->base))
 		return PTR_ERR(aemif->base);
 
+	spin_lock_init(&aemif->config_cs_lock);
 	if (np) {
 		/*
 		 * For every controller device node, there is a cs device node
diff --git a/include/linux/memory/ti-aemif.h b/include/linux/memory/ti-aemif.h
new file mode 100644
index 000000000000..809f0a68605a
--- /dev/null
+++ b/include/linux/memory/ti-aemif.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __TI_AEMIF_H
+#define __TI_AEMIF_H
+
+/**
+ * struct aemif_cs_timings: structure to hold cs timing configuration
+ * values are expressed in number of clock cycles - 1
+ * @ta: minimum turn around time
+ * @rhold: read hold width
+ * @rstrobe: read strobe width
+ * @rsetup: read setup width
+ * @whold: write hold width
+ * @wstrobe: write strobe width
+ * @wsetup: write setup width
+ */
+struct aemif_cs_timings {
+	u32	ta;
+	u32	rhold;
+	u32	rstrobe;
+	u32	rsetup;
+	u32	whold;
+	u32	wstrobe;
+	u32	wsetup;
+};
+
+struct aemif_device;
+
+int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_cs_timings *timings);
+
+#endif // __TI_AEMIF_H
-- 
2.47.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/6] memory: ti-aemif: Export aemif_set_cs_timings()

[Miquel Raynal]

Hello Bastien,

On 06/11/2024 at 09:55:03 +01, Bastien Curutchet <bastien.curutchet@bootlin.com> wrote:

> Export the aemif_set_cs_timing() symbol so it can be used by other
> drivers
>
> Add a spinlock to protect the CS configuration register from concurrent
> accesses.

What concurrent accesses are you trying to protect yourself against?
I fail to see the use case, but TBH I haven't tried hard enough maybe.

Also, what justifies the use of a spin-lock in this case?

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/6] memory: ti-aemif: Export aemif_set_cs_timings()

[Bastien Curutchet]

Hi Miquèl,

On 11/11/24 8:21 PM, Miquel Raynal wrote:
> 
> Hello Bastien,
> 
> On 06/11/2024 at 09:55:03 +01, Bastien Curutchet <bastien.curutchet@bootlin.com> wrote:
> 
>> Export the aemif_set_cs_timing() symbol so it can be used by other
>> drivers
>>
>> Add a spinlock to protect the CS configuration register from concurrent
>> accesses.
> 
> What concurrent accesses are you trying to protect yourself against?
> I fail to see the use case, but TBH I haven't tried hard enough maybe.
> 

The register that handles the CS configuration belongs to the AEMIF 
component but it will also be accessed by the AEMIF 'children' with the 
aemif_set_cs_timing() function. So far, concurrent accesses shouldn't 
occur because the AEMIF configures the CS timings only once and does so 
before probing its 'children'; but I don't know how things can evolve in 
the future.

> Also, what justifies the use of a spin-lock in this case?
> 

TBH, I always struggle to choose between mutexes and spin-locks. I chose 
spin-lock because it only protects one memory-mapped register so I think 
it doesn't worth going to sleep when waiting for the lock.


Best regards,
Bastien

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/6] memory: ti-aemif: Export aemif_set_cs_timings()

[Miquel Raynal]

Hi Bastien,

On 12/11/2024 at 10:13:38 +01, Bastien Curutchet <bastien.curutchet@bootlin.com> wrote:

> Hi Miquèl,
>
> On 11/11/24 8:21 PM, Miquel Raynal wrote:
>> Hello Bastien,
>> On 06/11/2024 at 09:55:03 +01, Bastien Curutchet
>> <bastien.curutchet@bootlin.com> wrote:
>> 
>>> Export the aemif_set_cs_timing() symbol so it can be used by other
>>> drivers
>>>
>>> Add a spinlock to protect the CS configuration register from concurrent
>>> accesses.
>> What concurrent accesses are you trying to protect yourself against?
>> I fail to see the use case, but TBH I haven't tried hard enough maybe.
>> 
>
> The register that handles the CS configuration belongs to the AEMIF
> component but it will also be accessed by the AEMIF 'children' with the
> aemif_set_cs_timing() function. So far, concurrent accesses shouldn't
> occur because the AEMIF configures the CS timings only once and does so
> before probing its 'children'; but I don't know how things can evolve in
> the future.

Okay, and I guess we can have more than one children, in this case
indeed we need serialization.

>> Also, what justifies the use of a spin-lock in this case?
>> 
>
> TBH, I always struggle to choose between mutexes and spin-locks. I chose
> spin-lock because it only protects one memory-mapped register so I think
> it doesn't worth going to sleep when waiting for the lock.

In general, I believe in a path that is not a hot-path and if you can
sleep, you should go for a mutex.

Cheers,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 3/6] mtd: rawnand: davinci: Always depends on TI_AEMIF

[Bastien Curutchet]

DAVINCI_NAND depends on TI_AEMIF only when ARCH_KEYSTONE is selected
while the NAND controller is also always a part of the AEMIF controller
on DaVinci SoCs.

Set a dependency on TI_AEMIF regardless of the selected architecture.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202411020140.3wsKJOSB-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202411020957.X1T8T9ZR-lkp@intel.com/

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/raw/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index d0aaccf72d78..bb61434347bd 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -279,8 +279,8 @@ config MTD_NAND_SH_FLCTL
 
 config MTD_NAND_DAVINCI
 	tristate "DaVinci/Keystone NAND controller"
-	depends on ARCH_DAVINCI || (ARCH_KEYSTONE && TI_AEMIF) || COMPILE_TEST
-	depends on HAS_IOMEM
+	depends on COMPILE_TEST || ARCH_DAVINCI || ARCH_KEYSTONE
+	depends on HAS_IOMEM && TI_AEMIF
 	help
 	  Enable the driver for NAND flash chips on Texas Instruments
 	  DaVinci/Keystone processors.
-- 
2.47.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 4/6] mtd: rawnand: davinci: Order headers alphabetically

[Bastien Curutchet]

Order headers alphabetically for better readability.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/raw/davinci_nand.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
index 392678143a36..3c0efbdd789e 100644
--- a/drivers/mtd/nand/raw/davinci_nand.c
+++ b/drivers/mtd/nand/raw/davinci_nand.c
@@ -10,15 +10,15 @@
  *   Dirk Behme <Dirk.Behme@gmail.com>
  */
 
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
 #include <linux/err.h>
 #include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/mtd/rawnand.h>
 #include <linux/mtd/partitions.h>
-#include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
 
 #define NRCSR_OFFSET		0x00
 #define NANDFCR_OFFSET		0x60
-- 
2.47.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 5/6] mtd: rawnand: davinci: Add clock resource

[Bastien Curutchet]

NAND controller has a reference clock inherited from the AEMIF
(cf. Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt)
This clock isn't used yet by the driver.

Add a struct clock in the struct davinci_nand_info so it can be used
to compute timings.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/raw/davinci_nand.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
index 3c0efbdd789e..563045c7ce08 100644
--- a/drivers/mtd/nand/raw/davinci_nand.c
+++ b/drivers/mtd/nand/raw/davinci_nand.c
@@ -10,6 +10,7 @@
  *   Dirk Behme <Dirk.Behme@gmail.com>
  */
 
+#include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
@@ -117,6 +118,8 @@ struct davinci_nand_info {
 	uint32_t		mask_cle;
 
 	uint32_t		core_chipsel;
+
+	struct clk		*clk;
 };
 
 static DEFINE_SPINLOCK(davinci_nand_lock);
@@ -822,6 +825,10 @@ static int nand_davinci_probe(struct platform_device *pdev)
 		return -EADDRNOTAVAIL;
 	}
 
+	info->clk = devm_clk_get_enabled(&pdev->dev, "aemif");
+	if (IS_ERR(info->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(info->clk), "failed to get clock");
+
 	info->pdev		= pdev;
 	info->base		= base;
 	info->vaddr		= vaddr;
-- 
2.47.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 6/6] mtd: rawnand: davinci: Implement setup_interface() operation

[Bastien Curutchet]

The setup_interface() operation isn't implemented. It forces the driver
to use the ONFI mode 0, though it could use more optimal modes.

Implement the setup_interface() operation. It uses the
aemif_set_cs_timings() function from the AEMIF driver to update the
chip select timings. The calculation of the register's contents is
directly extracted from §20.3.2.3 of the DaVinci TRM [1]

These timings are previously set by the AEMIF driver itself from
device-tree properties. Therefore, IMHO, failing to update them in the
setup_interface() isn't critical, which is why 0 is returned even when
timings aren't updated.

MAX_TH_PS and MAX_TSU_PS are the worst case timings based on the
Keystone2 and DaVinci datasheets.

[1] : https://www.ti.com/lit/ug/spruh77c/spruh77c.pdf

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/raw/davinci_nand.c | 78 +++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
index 563045c7ce08..2d0c564c8d17 100644
--- a/drivers/mtd/nand/raw/davinci_nand.c
+++ b/drivers/mtd/nand/raw/davinci_nand.c
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
+#include <linux/memory/ti-aemif.h>
 #include <linux/module.h>
 #include <linux/mtd/rawnand.h>
 #include <linux/mtd/partitions.h>
@@ -44,6 +45,9 @@
 #define	MASK_ALE		0x08
 #define	MASK_CLE		0x10
 
+#define MAX_TSU_PS		3000	/* Input setup time in ps */
+#define MAX_TH_PS		1600	/* Input hold time in ps */
+
 struct davinci_nand_pdata {
 	uint32_t		mask_ale;
 	uint32_t		mask_cle;
@@ -120,6 +124,7 @@ struct davinci_nand_info {
 	uint32_t		core_chipsel;
 
 	struct clk		*clk;
+	struct aemif_device	*aemif;
 };
 
 static DEFINE_SPINLOCK(davinci_nand_lock);
@@ -767,9 +772,81 @@ static int davinci_nand_exec_op(struct nand_chip *chip,
 	return 0;
 }
 
+#define TO_CYCLES(ps, period_ns) (DIV_ROUND_UP((ps) / 1000, (period_ns)))
+
+static int davinci_nand_setup_interface(struct nand_chip *chip, int chipnr,
+					const struct nand_interface_config *conf)
+{
+	struct davinci_nand_info *info = to_davinci_nand(nand_to_mtd(chip));
+	const struct nand_sdr_timings *sdr;
+	struct aemif_cs_timings timings;
+	s32 cfg, min, cyc_ns;
+
+	cyc_ns = 1000000000 / clk_get_rate(info->clk);
+
+	sdr = nand_get_sdr_timings(conf);
+	if (IS_ERR(sdr))
+		return PTR_ERR(sdr);
+
+	cfg = TO_CYCLES(sdr->tCLR_min, cyc_ns) - 1;
+	timings.rsetup = cfg > 0 ? cfg : 0;
+
+	cfg = max_t(s32, TO_CYCLES(sdr->tREA_max + MAX_TSU_PS, cyc_ns),
+		    TO_CYCLES(sdr->tRP_min, cyc_ns)) - 1;
+	timings.rstrobe = cfg > 0 ? cfg : 0;
+
+	min = TO_CYCLES(sdr->tCEA_max + MAX_TSU_PS, cyc_ns) - 2;
+	while ((s32)(timings.rsetup + timings.rstrobe) < min)
+		timings.rstrobe++;
+
+	cfg = TO_CYCLES((s32)(MAX_TH_PS - sdr->tCHZ_max), cyc_ns) - 1;
+	timings.rhold = cfg > 0 ? cfg : 0;
+
+	min = TO_CYCLES(sdr->tRC_min, cyc_ns) - 3;
+	while ((s32)(timings.rsetup + timings.rstrobe + timings.rhold) < min)
+		timings.rhold++;
+
+	cfg = TO_CYCLES((s32)(sdr->tRHZ_max - (timings.rhold + 1) * cyc_ns * 1000), cyc_ns);
+	cfg = max_t(s32, cfg, TO_CYCLES(sdr->tCHZ_max, cyc_ns)) - 1;
+	timings.ta = cfg > 0 ? cfg : 0;
+
+	cfg = TO_CYCLES(sdr->tWP_min, cyc_ns) - 1;
+	timings.wstrobe = cfg > 0 ? cfg : 0;
+
+	cfg = max_t(s32, TO_CYCLES(sdr->tCLS_min, cyc_ns), TO_CYCLES(sdr->tALS_min, cyc_ns));
+	cfg = max_t(s32, cfg, TO_CYCLES(sdr->tCS_min, cyc_ns)) - 1;
+	timings.wsetup = cfg > 0 ? cfg : 0;
+
+	min = TO_CYCLES(sdr->tDS_min, cyc_ns) - 2;
+	while ((s32)(timings.wsetup + timings.wstrobe) < min)
+		timings.wstrobe++;
+
+	cfg = max_t(s32, TO_CYCLES(sdr->tCLH_min, cyc_ns), TO_CYCLES(sdr->tALH_min, cyc_ns));
+	cfg = max_t(s32, cfg, TO_CYCLES(sdr->tCH_min, cyc_ns));
+	cfg = max_t(s32, cfg, TO_CYCLES(sdr->tDH_min, cyc_ns)) - 1;
+	timings.whold = cfg > 0 ? cfg : 0;
+
+	min = TO_CYCLES(sdr->tWC_min, cyc_ns) - 2;
+	while ((s32)(timings.wsetup + timings.wstrobe + timings.whold) < min)
+		timings.whold++;
+
+	dev_dbg(&info->pdev->dev, "RSETUP %x RSTROBE %x RHOLD %x\n",
+		timings.rsetup, timings.rstrobe, timings.rhold);
+	dev_dbg(&info->pdev->dev, "TA %x\n", timings.ta);
+	dev_dbg(&info->pdev->dev, "WSETUP %x WSTROBE %x WHOLD %x\n",
+		timings.wsetup, timings.wstrobe, timings.whold);
+
+	if (aemif_set_cs_timings(info->aemif, info->core_chipsel, &timings) < 0)
+		dev_info(&info->pdev->dev,
+			 "Failed to dynamically update the CS timings, keep them unchanged");
+
+	return 0;
+}
+
 static const struct nand_controller_ops davinci_nand_controller_ops = {
 	.attach_chip = davinci_nand_attach_chip,
 	.exec_op = davinci_nand_exec_op,
+	.setup_interface = davinci_nand_setup_interface,
 };
 
 static int nand_davinci_probe(struct platform_device *pdev)
@@ -832,6 +909,7 @@ static int nand_davinci_probe(struct platform_device *pdev)
 	info->pdev		= pdev;
 	info->base		= base;
 	info->vaddr		= vaddr;
+	info->aemif		= dev_get_drvdata(pdev->dev.parent);
 
 	mtd			= nand_to_mtd(&info->chip);
 	mtd->dev.parent		= &pdev->dev;
-- 
2.47.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 6/6] mtd: rawnand: davinci: Implement setup_interface() operation

[Miquel Raynal]

Hi Bastien,

On 06/11/2024 at 09:55:07 +01, Bastien Curutchet <bastien.curutchet@bootlin.com> wrote:

> The setup_interface() operation isn't implemented. It forces the driver
> to use the ONFI mode 0, though it could use more optimal modes.
>
> Implement the setup_interface() operation. It uses the
> aemif_set_cs_timings() function from the AEMIF driver to update the
> chip select timings. The calculation of the register's contents is
> directly extracted from §20.3.2.3 of the DaVinci TRM [1]
>
> These timings are previously set by the AEMIF driver itself from
> device-tree properties. Therefore, IMHO, failing to update them in the
> setup_interface() isn't critical, which is why 0 is returned even when
> timings aren't updated.

Did you experience failures? Because I'd be more conservative and error
out loudly when something is wrong. In general it is a safest approach
on the long term. Here maybe you have good reasons to do differently, in
this case I am all ears.

> MAX_TH_PS and MAX_TSU_PS are the worst case timings based on the
> Keystone2 and DaVinci datasheets.
>
> [1] : https://www.ti.com/lit/ug/spruh77c/spruh77c.pdf
>
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  drivers/mtd/nand/raw/davinci_nand.c | 78 +++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
> index 563045c7ce08..2d0c564c8d17 100644
> --- a/drivers/mtd/nand/raw/davinci_nand.c
> +++ b/drivers/mtd/nand/raw/davinci_nand.c
> @@ -14,6 +14,7 @@
>  #include <linux/err.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
> +#include <linux/memory/ti-aemif.h>
>  #include <linux/module.h>
>  #include <linux/mtd/rawnand.h>
>  #include <linux/mtd/partitions.h>
> @@ -44,6 +45,9 @@
>  #define	MASK_ALE		0x08
>  #define	MASK_CLE		0x10
>  
> +#define MAX_TSU_PS		3000	/* Input setup time in ps */
> +#define MAX_TH_PS		1600	/* Input hold time in ps */
> +
>  struct davinci_nand_pdata {
>  	uint32_t		mask_ale;
>  	uint32_t		mask_cle;
> @@ -120,6 +124,7 @@ struct davinci_nand_info {
>  	uint32_t		core_chipsel;
>  
>  	struct clk		*clk;
> +	struct aemif_device	*aemif;
>  };
>  
>  static DEFINE_SPINLOCK(davinci_nand_lock);
> @@ -767,9 +772,81 @@ static int davinci_nand_exec_op(struct nand_chip *chip,
>  	return 0;
>  }
>  
> +#define TO_CYCLES(ps, period_ns) (DIV_ROUND_UP((ps) / 1000, (period_ns)))
> +
> +static int davinci_nand_setup_interface(struct nand_chip *chip, int chipnr,
> +					const struct nand_interface_config *conf)
> +{
> +	struct davinci_nand_info *info = to_davinci_nand(nand_to_mtd(chip));
> +	const struct nand_sdr_timings *sdr;
> +	struct aemif_cs_timings timings;
> +	s32 cfg, min, cyc_ns;
> +
> +	cyc_ns = 1000000000 / clk_get_rate(info->clk);
> +
> +	sdr = nand_get_sdr_timings(conf);
> +	if (IS_ERR(sdr))
> +		return PTR_ERR(sdr);
> +
> +	cfg = TO_CYCLES(sdr->tCLR_min, cyc_ns) - 1;
> +	timings.rsetup = cfg > 0 ? cfg : 0;
> +
> +	cfg = max_t(s32, TO_CYCLES(sdr->tREA_max + MAX_TSU_PS, cyc_ns),
> +		    TO_CYCLES(sdr->tRP_min, cyc_ns)) - 1;
> +	timings.rstrobe = cfg > 0 ? cfg : 0;
> +
> +	min = TO_CYCLES(sdr->tCEA_max + MAX_TSU_PS, cyc_ns) - 2;
> +	while ((s32)(timings.rsetup + timings.rstrobe) < min)
> +		timings.rstrobe++;

I see a lot of while loops which just stop if you reach a min/max, I
believe a slightly more robust approach should be attempted, and
returning errors when these limits are crossed would be probably
beneficial on the long term?

> +
> +	cfg = TO_CYCLES((s32)(MAX_TH_PS - sdr->tCHZ_max), cyc_ns) - 1;
> +	timings.rhold = cfg > 0 ? cfg : 0;
> +
> +	min = TO_CYCLES(sdr->tRC_min, cyc_ns) - 3;
> +	while ((s32)(timings.rsetup + timings.rstrobe + timings.rhold) < min)
> +		timings.rhold++;
> +
> +	cfg = TO_CYCLES((s32)(sdr->tRHZ_max - (timings.rhold + 1) * cyc_ns * 1000), cyc_ns);
> +	cfg = max_t(s32, cfg, TO_CYCLES(sdr->tCHZ_max, cyc_ns)) - 1;
> +	timings.ta = cfg > 0 ? cfg : 0;
> +
> +	cfg = TO_CYCLES(sdr->tWP_min, cyc_ns) - 1;
> +	timings.wstrobe = cfg > 0 ? cfg : 0;
> +
> +	cfg = max_t(s32, TO_CYCLES(sdr->tCLS_min, cyc_ns), TO_CYCLES(sdr->tALS_min, cyc_ns));
> +	cfg = max_t(s32, cfg, TO_CYCLES(sdr->tCS_min, cyc_ns)) - 1;
> +	timings.wsetup = cfg > 0 ? cfg : 0;
> +
> +	min = TO_CYCLES(sdr->tDS_min, cyc_ns) - 2;
> +	while ((s32)(timings.wsetup + timings.wstrobe) < min)
> +		timings.wstrobe++;
> +
> +	cfg = max_t(s32, TO_CYCLES(sdr->tCLH_min, cyc_ns), TO_CYCLES(sdr->tALH_min, cyc_ns));
> +	cfg = max_t(s32, cfg, TO_CYCLES(sdr->tCH_min, cyc_ns));
> +	cfg = max_t(s32, cfg, TO_CYCLES(sdr->tDH_min, cyc_ns)) - 1;
> +	timings.whold = cfg > 0 ? cfg : 0;
> +
> +	min = TO_CYCLES(sdr->tWC_min, cyc_ns) - 2;
> +	while ((s32)(timings.wsetup + timings.wstrobe + timings.whold) < min)
> +		timings.whold++;
> +
> +	dev_dbg(&info->pdev->dev, "RSETUP %x RSTROBE %x RHOLD %x\n",
> +		timings.rsetup, timings.rstrobe, timings.rhold);
> +	dev_dbg(&info->pdev->dev, "TA %x\n", timings.ta);
> +	dev_dbg(&info->pdev->dev, "WSETUP %x WSTROBE %x WHOLD %x\n",
> +		timings.wsetup, timings.wstrobe, timings.whold);
> +

Here you probably want to exit in the NAND_DATA_IFACE_CHECK_ONLY case.

> +	if (aemif_set_cs_timings(info->aemif, info->core_chipsel, &timings) < 0)
> +		dev_info(&info->pdev->dev,
> +			 "Failed to dynamically update the CS timings, keep them unchanged");
> +
> +	return 0;
> +}
> +

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 6/6] mtd: rawnand: davinci: Implement setup_interface() operation

[Bastien Curutchet]

Hi Miquèl,

On 11/11/24 8:32 PM, Miquel Raynal wrote:
> Hi Bastien,
> 
> On 06/11/2024 at 09:55:07 +01, Bastien Curutchet <bastien.curutchet@bootlin.com> wrote:
> 
>> The setup_interface() operation isn't implemented. It forces the driver
>> to use the ONFI mode 0, though it could use more optimal modes.
>>
>> Implement the setup_interface() operation. It uses the
>> aemif_set_cs_timings() function from the AEMIF driver to update the
>> chip select timings. The calculation of the register's contents is
>> directly extracted from §20.3.2.3 of the DaVinci TRM [1]
>>
>> These timings are previously set by the AEMIF driver itself from
>> device-tree properties. Therefore, IMHO, failing to update them in the
>> setup_interface() isn't critical, which is why 0 is returned even when
>> timings aren't updated.
> 
> Did you experience failures? Because I'd be more conservative and error
> out loudly when something is wrong. In general it is a safest approach
> on the long term. Here maybe you have good reasons to do differently, in
> this case I am all ears.
> 

The DaVinci's configuration isn't very wide so if its reference clock 
rate is too high, mode 0 timings can be too slow to fit while higher 
modes will fit. So returning -EINVAL here will cause the probe to fail 
with 'Failed to configure data interface to SDR timing mode 0' while 
higher modes would have worked.

>> MAX_TH_PS and MAX_TSU_PS are the worst case timings based on the
>> Keystone2 and DaVinci datasheets.
>>
>> [1] : https://www.ti.com/lit/ug/spruh77c/spruh77c.pdf
>>
>> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
>> ---
>>   drivers/mtd/nand/raw/davinci_nand.c | 78 +++++++++++++++++++++++++++++
>>   1 file changed, 78 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
>> index 563045c7ce08..2d0c564c8d17 100644
>> --- a/drivers/mtd/nand/raw/davinci_nand.c
>> +++ b/drivers/mtd/nand/raw/davinci_nand.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/err.h>
>>   #include <linux/iopoll.h>
>>   #include <linux/kernel.h>
>> +#include <linux/memory/ti-aemif.h>
>>   #include <linux/module.h>
>>   #include <linux/mtd/rawnand.h>
>>   #include <linux/mtd/partitions.h>
>> @@ -44,6 +45,9 @@
>>   #define	MASK_ALE		0x08
>>   #define	MASK_CLE		0x10
>>   
>> +#define MAX_TSU_PS		3000	/* Input setup time in ps */
>> +#define MAX_TH_PS		1600	/* Input hold time in ps */
>> +
>>   struct davinci_nand_pdata {
>>   	uint32_t		mask_ale;
>>   	uint32_t		mask_cle;
>> @@ -120,6 +124,7 @@ struct davinci_nand_info {
>>   	uint32_t		core_chipsel;
>>   
>>   	struct clk		*clk;
>> +	struct aemif_device	*aemif;
>>   };
>>   
>>   static DEFINE_SPINLOCK(davinci_nand_lock);
>> @@ -767,9 +772,81 @@ static int davinci_nand_exec_op(struct nand_chip *chip,
>>   	return 0;
>>   }
>>   
>> +#define TO_CYCLES(ps, period_ns) (DIV_ROUND_UP((ps) / 1000, (period_ns)))
>> +
>> +static int davinci_nand_setup_interface(struct nand_chip *chip, int chipnr,
>> +					const struct nand_interface_config *conf)
>> +{
>> +	struct davinci_nand_info *info = to_davinci_nand(nand_to_mtd(chip));
>> +	const struct nand_sdr_timings *sdr;
>> +	struct aemif_cs_timings timings;
>> +	s32 cfg, min, cyc_ns;
>> +
>> +	cyc_ns = 1000000000 / clk_get_rate(info->clk);
>> +
>> +	sdr = nand_get_sdr_timings(conf);
>> +	if (IS_ERR(sdr))
>> +		return PTR_ERR(sdr);
>> +
>> +	cfg = TO_CYCLES(sdr->tCLR_min, cyc_ns) - 1;
>> +	timings.rsetup = cfg > 0 ? cfg : 0;
>> +
>> +	cfg = max_t(s32, TO_CYCLES(sdr->tREA_max + MAX_TSU_PS, cyc_ns),
>> +		    TO_CYCLES(sdr->tRP_min, cyc_ns)) - 1;
>> +	timings.rstrobe = cfg > 0 ? cfg : 0;
>> +
>> +	min = TO_CYCLES(sdr->tCEA_max + MAX_TSU_PS, cyc_ns) - 2;
>> +	while ((s32)(timings.rsetup + timings.rstrobe) < min)
>> +		timings.rstrobe++;
> 
> I see a lot of while loops which just stop if you reach a min/max, I
> believe a slightly more robust approach should be attempted, and
> returning errors when these limits are crossed would be probably
> beneficial on the long term?
> 

This comes from the DaVinci NAND controller's documentation (cf p908 of 
https://www.ti.com/lit/ug/spruh77c/spruh77c.pdf). A first formula, gives 
the RSETUP timing, a second one the RSTROBE timing and then a third 
formula gives a constraint that has to be met by the sum of RSETUP and 
RSTROBE.
Then the validity of the timings themselves is checked by the 
aemif_set_cs_timings() function.

It works the same way for WSETUP, WSTROBE and WHOLD with the other 
while() loops.

>> +
>> +	cfg = TO_CYCLES((s32)(MAX_TH_PS - sdr->tCHZ_max), cyc_ns) - 1;
>> +	timings.rhold = cfg > 0 ? cfg : 0;
>> +
>> +	min = TO_CYCLES(sdr->tRC_min, cyc_ns) - 3;
>> +	while ((s32)(timings.rsetup + timings.rstrobe + timings.rhold) < min)
>> +		timings.rhold++;
>> +
>> +	cfg = TO_CYCLES((s32)(sdr->tRHZ_max - (timings.rhold + 1) * cyc_ns * 1000), cyc_ns);
>> +	cfg = max_t(s32, cfg, TO_CYCLES(sdr->tCHZ_max, cyc_ns)) - 1;
>> +	timings.ta = cfg > 0 ? cfg : 0;
>> +
>> +	cfg = TO_CYCLES(sdr->tWP_min, cyc_ns) - 1;
>> +	timings.wstrobe = cfg > 0 ? cfg : 0;
>> +
>> +	cfg = max_t(s32, TO_CYCLES(sdr->tCLS_min, cyc_ns), TO_CYCLES(sdr->tALS_min, cyc_ns));
>> +	cfg = max_t(s32, cfg, TO_CYCLES(sdr->tCS_min, cyc_ns)) - 1;
>> +	timings.wsetup = cfg > 0 ? cfg : 0;
>> +
>> +	min = TO_CYCLES(sdr->tDS_min, cyc_ns) - 2;
>> +	while ((s32)(timings.wsetup + timings.wstrobe) < min)
>> +		timings.wstrobe++;
>> +
>> +	cfg = max_t(s32, TO_CYCLES(sdr->tCLH_min, cyc_ns), TO_CYCLES(sdr->tALH_min, cyc_ns));
>> +	cfg = max_t(s32, cfg, TO_CYCLES(sdr->tCH_min, cyc_ns));
>> +	cfg = max_t(s32, cfg, TO_CYCLES(sdr->tDH_min, cyc_ns)) - 1;
>> +	timings.whold = cfg > 0 ? cfg : 0;
>> +
>> +	min = TO_CYCLES(sdr->tWC_min, cyc_ns) - 2;
>> +	while ((s32)(timings.wsetup + timings.wstrobe + timings.whold) < min)
>> +		timings.whold++;
>> +
>> +	dev_dbg(&info->pdev->dev, "RSETUP %x RSTROBE %x RHOLD %x\n",
>> +		timings.rsetup, timings.rstrobe, timings.rhold);
>> +	dev_dbg(&info->pdev->dev, "TA %x\n", timings.ta);
>> +	dev_dbg(&info->pdev->dev, "WSETUP %x WSTROBE %x WHOLD %x\n",
>> +		timings.wsetup, timings.wstrobe, timings.whold);
>> +
> 
> Here you probably want to exit in the NAND_DATA_IFACE_CHECK_ONLY case.
> 

I had missed this NAND_DATA_IFACE_CHECK_ONLY feature. In fact, 
additionnal checks are done in the aemif_set_cs_timings() below. I'll 
add an 'aemif_check_cs_timings()' function in AEMIF's driver in next 
iteration.

>> +	if (aemif_set_cs_timings(info->aemif, info->core_chipsel, &timings) < 0)
>> +		dev_info(&info->pdev->dev,
>> +			 "Failed to dynamically update the CS timings, keep them unchanged");
>> +
>> +	return 0;
>> +}
>> +
> 
> Thanks,
> Miquèl


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/