[PATCH 0/5] Implement setup_inteface() in the DaVinci NAND controller

[PATCH 0/5] Implement setup_inteface() 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.

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. To do so, I add a ti-aemif.h header, I'm not
sure whether this header should be located in include/memory or in
include/linux/memory. I put it in include/memory because the folder
already exists while include/linux/memory doesn't.

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

Bastien Curutchet (5):
  memory: ti-aemif: Create aemif_set_cs_timings()
  memory: ti-aemif: export aemif_set_cs_timing()
  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/davinci_nand.c |  95 ++++++++++++++++++++++++--
 include/memory/ti-aemif.h           |  31 +++++++++
 3 files changed, 191 insertions(+), 37 deletions(-)
 create mode 100644 include/memory/ti-aemif.h

-- 
2.47.0

[PATCH 1/5] 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..5be6df246075 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_timing: 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

[PATCH 2/5] memory: ti-aemif: export aemif_set_cs_timing()

[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/memory/ti-aemif.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 22 deletions(-)
 create mode 100644 include/memory/ti-aemif.h

diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
index 5be6df246075..ee857c16a02c 100644
--- a/drivers/memory/ti-aemif.c
+++ b/drivers/memory/ti-aemif.c
@@ -17,6 +17,8 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <memory/ti-aemif.h>
 
 #define TA_SHIFT	2
 #define RHOLD_SHIFT	4
@@ -107,27 +109,6 @@ struct aemif_cs_data {
 	u8	asize;
 };
 
-/**
- * struct aemif_cs_timing: 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
+ * @cs_config_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/memory/ti-aemif.h b/include/memory/ti-aemif.h
new file mode 100644
index 000000000000..a1478387a8d0
--- /dev/null
+++ b/include/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_timing: 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

Re: [PATCH 2/5] memory: ti-aemif: export aemif_set_cs_timing()

[kernel test robot]

Hi Bastien,

kernel test robot noticed the following build warnings:

[auto build test WARNING on krzk-mem-ctrl/for-next]
[also build test WARNING on mtd/nand/next linus/master v6.12-rc5 next-20241030]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bastien-Curutchet/memory-ti-aemif-Create-aemif_set_cs_timings/20241030-184949
base:   https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-mem-ctrl.git for-next
patch link:    https://lore.kernel.org/r/20241030104717.88688-3-bastien.curutchet%40bootlin.com
patch subject: [PATCH 2/5] memory: ti-aemif: export aemif_set_cs_timing()
config: arc-randconfig-001-20241031 (https://download.01.org/0day-ci/archive/20241031/202410311304.0fuqFK2M-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241031/202410311304.0fuqFK2M-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410311304.0fuqFK2M-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/memory/ti-aemif.c:130: warning: Function parameter or struct member 'config_cs_lock' not described in 'aemif_device'
>> drivers/memory/ti-aemif.c:130: warning: Excess struct member 'cs_config_lock' description in 'aemif_device'

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]


vim +130 drivers/memory/ti-aemif.c

5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  111  
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  112  /**
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  113   * struct aemif_device: structure to hold device data
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  114   * @base: base address of AEMIF registers
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  115   * @clk: source clock
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  116   * @clk_rate: clock's rate in kHz
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  117   * @num_cs: number of assigned chip-selects
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  118   * @cs_offset: start number of cs nodes
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  119   * @cs_data: array of chip-select settings
0f034c3604121bf Bastien Curutchet 2024-10-30  120   * @cs_config_lock: lock used to access CS configuration
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  121   */
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  122  struct aemif_device {
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  123  	void __iomem *base;
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  124  	struct clk *clk;
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  125  	unsigned long clk_rate;
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  126  	u8 num_cs;
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  127  	int cs_offset;
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  128  	struct aemif_cs_data cs_data[NUM_CS];
0f034c3604121bf Bastien Curutchet 2024-10-30  129  	spinlock_t config_cs_lock;
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24 @130  };
5a7c81547c1db75 Ivan Khoronzhuk   2014-02-24  131  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 3/5] 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

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

[Bastien Curutchet]

NAND controller has a reference clock but the driver doesn't use it.

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
index 3c0efbdd789e..11dc30c29957 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,12 @@ static int nand_davinci_probe(struct platform_device *pdev)
 		return -EADDRNOTAVAIL;
 	}
 
+	info->clk = devm_clk_get(&pdev->dev, "aemif");
+	if (IS_ERR(info->clk)) {
+		dev_err(&pdev->dev, "failed to get clock %ld", PTR_ERR(info->clk));
+		return PTR_ERR(info->clk);
+	}
+
 	info->pdev		= pdev;
 	info->base		= base;
 	info->vaddr		= vaddr;
-- 
2.47.0

Re: [PATCH 4/5] mtd: rawnand: davinci: Add clock resource

[Krzysztof Kozlowski]

On 30/10/2024 11:47, Bastien Curutchet wrote:
> NAND controller has a reference clock but the driver doesn't use it.
> 
> 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 | 9 +++++++++

Where are the bindings?

>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
> index 3c0efbdd789e..11dc30c29957 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,12 @@ static int nand_davinci_probe(struct platform_device *pdev)
>  		return -EADDRNOTAVAIL;
>  	}
>  
> +	info->clk = devm_clk_get(&pdev->dev, "aemif");
> +	if (IS_ERR(info->clk)) {
> +		dev_err(&pdev->dev, "failed to get clock %ld", PTR_ERR(info->clk));

Syntax is return dev_err_probe.


Best regards,
Krzysztof

Re: [PATCH 4/5] mtd: rawnand: davinci: Add clock resource

[Bastien Curutchet]

Hi Krzysztof,

On 10/30/24 12:17 PM, Krzysztof Kozlowski wrote:
> On 30/10/2024 11:47, Bastien Curutchet wrote:
>> NAND controller has a reference clock but the driver doesn't use it.
>>
>> 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 | 9 +++++++++
> 
> Where are the bindings?
> 

The NAND controller bindings are in 
Documentation/devicetree/bindings/mtd/davinci-nand.txt but this clock is 
defined in the AEMIF bindings in 
Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt

>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
>> index 3c0efbdd789e..11dc30c29957 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,12 @@ static int nand_davinci_probe(struct platform_device *pdev)
>>   		return -EADDRNOTAVAIL;
>>   	}
>>   
>> +	info->clk = devm_clk_get(&pdev->dev, "aemif");
>> +	if (IS_ERR(info->clk)) {
>> +		dev_err(&pdev->dev, "failed to get clock %ld", PTR_ERR(info->clk));
> 
> Syntax is return dev_err_probe.
> 

Ok, I'll correct this in V2


Best regards,
Bastien

Re: [PATCH 4/5] mtd: rawnand: davinci: Add clock resource

[Krzysztof Kozlowski]

On 30/10/2024 13:20, Bastien Curutchet wrote:
> Hi Krzysztof,
> 
> On 10/30/24 12:17 PM, Krzysztof Kozlowski wrote:
>> On 30/10/2024 11:47, Bastien Curutchet wrote:
>>> NAND controller has a reference clock but the driver doesn't use it.
>>>
>>> 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 | 9 +++++++++
>>
>> Where are the bindings?
>>
> 
> The NAND controller bindings are in 
> Documentation/devicetree/bindings/mtd/davinci-nand.txt but this clock is 
> defined in the AEMIF bindings in 
> Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt

Mention this in commit msg so you get one reviewer question less...

Best regards,
Krzysztof

[PATCH 5/5] 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 11dc30c29957..76f0306dfe77 100644
--- a/drivers/mtd/nand/raw/davinci_nand.c
+++ b/drivers/mtd/nand/raw/davinci_nand.c
@@ -20,6 +20,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <memory/ti-aemif.h>
 
 #define NRCSR_OFFSET		0x00
 #define NANDFCR_OFFSET		0x60
@@ -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)
@@ -834,6 +911,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

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

[kernel test robot]

Hi Bastien,

kernel test robot noticed the following build errors:

[auto build test ERROR on krzk-mem-ctrl/for-next]
[also build test ERROR on linus/master v6.12-rc5]
[cannot apply to mtd/nand/next next-20241101]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bastien-Curutchet/memory-ti-aemif-Create-aemif_set_cs_timings/20241030-184949
base:   https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-mem-ctrl.git for-next
patch link:    https://lore.kernel.org/r/20241030104717.88688-6-bastien.curutchet%40bootlin.com
patch subject: [PATCH 5/5] mtd: rawnand: davinci: Implement setup_interface() operation
config: arc-randconfig-001-20241101 (https://download.01.org/0day-ci/archive/20241102/202411020140.3wsKJOSB-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241102/202411020140.3wsKJOSB-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411020140.3wsKJOSB-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in lib/zlib_inflate/zlib_inflate.o
>> ERROR: modpost: "aemif_set_cs_timings" [drivers/mtd/nand/raw/davinci_nand.ko] undefined!

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Bastien,

kernel test robot noticed the following build errors:

[auto build test ERROR on krzk-mem-ctrl/for-next]
[also build test ERROR on linus/master v6.12-rc5]
[cannot apply to mtd/nand/next next-20241101]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bastien-Curutchet/memory-ti-aemif-Create-aemif_set_cs_timings/20241030-184949
base:   https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-mem-ctrl.git for-next
patch link:    https://lore.kernel.org/r/20241030104717.88688-6-bastien.curutchet%40bootlin.com
patch subject: [PATCH 5/5] mtd: rawnand: davinci: Implement setup_interface() operation
config: arm-randconfig-003-20241101 (https://download.01.org/0day-ci/archive/20241102/202411020957.X1T8T9ZR-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241102/202411020957.X1T8T9ZR-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411020957.X1T8T9ZR-lkp@intel.com/

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: drivers/mtd/nand/raw/davinci_nand.o: in function `davinci_nand_setup_interface':
>> davinci_nand.c:(.text+0x688): undefined reference to `aemif_set_cs_timings'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 0/5] Implement setup_inteface() in the DaVinci NAND controller

[Krzysztof Kozlowski]

On 30/10/2024 11:47, Bastien Curutchet wrote:
> 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.
> 

Your changelog is supposed to explain also merging dependencies. Within
patchset or external.

> 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. To do so, I add a ti-aemif.h header, I'm not
> sure whether this header should be located in include/memory or in
> include/linux/memory. I put it in include/memory because the folder
> already exists while include/linux/memory doesn't.

All Linux headers go to include/linux/, so this one should as well.



Best regards,
Krzysztof

Re: [PATCH 0/5] Implement setup_inteface() in the DaVinci NAND controller

[Bastien Curutchet]

Hi Krzysztof,

On 10/30/24 12:17 PM, Krzysztof Kozlowski wrote:
> On 30/10/2024 11:47, Bastien Curutchet wrote:
>> 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.
>>
> 
> Your changelog is supposed to explain also merging dependencies. Within
> patchset or external.

I'm not sure I understand what you mean here. Do you mean that I need to 
explicitly state that the patches in the 
drivers/mtd/nand/raw/davinci_nand.c depend on the ones in 
drivers/memory/ti-aemif.c ?

There isn't any external dependency on this patch series. The ONFI modes 
are already managed by the NAND core driver (in 
drivers/mtd/nand/raw/nand_base.c). If a NAND controller wants to benefit 
from all the ONFI modes, it needs to implement the setup_interface() 
operation; otherwise it can only use the mode 0 which is the slowest.

> 
>> 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. To do so, I add a ti-aemif.h header, I'm not
>> sure whether this header should be located in include/memory or in
>> include/linux/memory. I put it in include/memory because the folder
>> already exists while include/linux/memory doesn't.
> 
> All Linux headers go to include/linux/, so this one should as well.
> 

Ok thank you, I'll move it there in V2.


Best regards,
Bastien

Re: [PATCH 0/5] Implement setup_inteface() in the DaVinci NAND controller

[Krzysztof Kozlowski]

On 30/10/2024 13:39, Bastien Curutchet wrote:
> Hi Krzysztof,
> 
> On 10/30/24 12:17 PM, Krzysztof Kozlowski wrote:
>> On 30/10/2024 11:47, Bastien Curutchet wrote:
>>> 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.
>>>
>>
>> Your changelog is supposed to explain also merging dependencies. Within
>> patchset or external.
> 
> I'm not sure I understand what you mean here. Do you mean that I need to 
> explicitly state that the patches in the 
> drivers/mtd/nand/raw/davinci_nand.c depend on the ones in 
> drivers/memory/ti-aemif.c ?

Well, you target different subsystems, so maintainers need to know what
they can take and what do you expect of them. Otherwise what do you
expect of me exactly and how can I guess it?


Best regards,
Krzysztof