[PATCH v4 00/10] Implement setup_interface() in the DaVinci NAND controller

[PATCH v4 00/10] 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 6 depends on PATCH 1-2-3-4-5
PATCH 10 depends on PATCH 6-8-9

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 three 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:
 * v4
   PATCH 1,2,3 [new]:
     - Create the struct aemif_cs_timings inside the already existing
       struct aemif_cs_data
 * v3
   PATCH 1 [new]:
     - Wrap the verification of CS timing configuration into a function
   PATCH 2:
     - Fix comments
   PATCH 3:
     - Replace the spin-lock with a mutex
   PATCH 7:
     - Add handling of the NAND_DATA_IFACE_CHECK_ONLY case
     - setup_interface() returns errno on failures

 * 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

[v1] : https://lore.kernel.org/all/20241030104717.88688-1-bastien.curutchet@bootlin.com/
[v2] : https://lore.kernel.org/all/20241106085507.76425-1-bastien.curutchet@bootlin.com/
[v3] : https://lore.kernel.org/all/20241113094938.44817-1-bastien.curutchet@bootlin.com/

Bastien Curutchet (10):
  memory: ti-aemif: Store timings parameter in number of cycles - 1
  memory: ti-aemif: Remove unnecessary local variables
  memory: ti-aemif: Wrap CS timings into a struct
  memory: ti-aemif: Create aemif_check_cs_timings()
  memory: ti-aemif: Create aemif_set_cs_timings()
  memory: ti-aemif: Export aemif_*_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           | 187 +++++++++++++++++-----------
 drivers/mtd/nand/raw/Kconfig        |   4 +-
 drivers/mtd/nand/raw/davinci_nand.c |  94 +++++++++++++-
 include/linux/memory/ti-aemif.h     |  32 +++++
 4 files changed, 237 insertions(+), 80 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 v4 01/10] memory: ti-aemif: Store timings parameter in number of cycles - 1

[Bastien Curutchet]

The CS configuration register expects timings to be expressed in
'number of cycles - 1' but they are stored in ns in the struct
aemif_cs_data. So at init, the timings currently set are converted to ns
by aemif_get_hw_params(), updated with values from the device-tree
properties, and then converted back to 'number of cycles - 1' before
being applied.

Store the timings directly in 'number of cycles - 1' instead of
nanoseconds.
Perform the conversion from nanosecond during the device-tree parsing.
Remove aemif_cycles_to_nsec() as it isn't used anymore.

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

diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
index d54dc3cfff73..bd0c49ba1939 100644
--- a/drivers/memory/ti-aemif.c
+++ b/drivers/memory/ti-aemif.c
@@ -80,28 +80,28 @@
 				ASIZE_MAX)
 
 /**
- * struct aemif_cs_data: structure to hold cs parameters
+ * struct aemif_cs_data: structure to hold CS parameters
  * @cs: chip-select number
- * @wstrobe: write strobe width, ns
- * @rstrobe: read strobe width, ns
- * @wsetup: write setup width, ns
- * @whold: write hold width, ns
- * @rsetup: read setup width, ns
- * @rhold: read hold width, ns
- * @ta: minimum turn around time, ns
+ * @wstrobe: write strobe width, number of cycles - 1
+ * @rstrobe: read strobe width, number of cycles - 1
+ * @wsetup: write setup width, number of cycles - 1
+ * @whold: write hold width, number of cycles - 1
+ * @rsetup: read setup width, number of cycles - 1
+ * @rhold: read hold width, number of cycles - 1
+ * @ta: minimum turn around time, number of cycles - 1
  * @enable_ss: enable/disable select strobe mode
  * @enable_ew: enable/disable extended wait mode
  * @asize: width of the asynchronous device's data bus
  */
 struct aemif_cs_data {
 	u8	cs;
-	u16	wstrobe;
-	u16	rstrobe;
-	u8	wsetup;
-	u8	whold;
-	u8	rsetup;
-	u8	rhold;
-	u8	ta;
+	u32	wstrobe;
+	u32	rstrobe;
+	u32	wsetup;
+	u32	whold;
+	u32	rsetup;
+	u32	rhold;
+	u32	ta;
 	u8	enable_ss;
 	u8	enable_ew;
 	u8	asize;
@@ -175,26 +175,18 @@ 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;
 	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;
-	}
+	ta	=  data->ta;
+	rhold	=  data->rhold;
+	rstrobe	=  data->rstrobe;
+	rsetup	=  data->rsetup;
+	whold	=  data->whold;
+	wstrobe	=  data->wstrobe;
+	wsetup	=  data->wsetup;
 
 	set = TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) | RSETUP(rsetup) |
 		WHOLD(whold) | WSTROBE(wstrobe) | WSETUP(wsetup);
@@ -213,11 +205,6 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
 	return 0;
 }
 
-static inline int aemif_cycles_to_nsec(int val, unsigned long clk_rate)
-{
-	return ((val + 1) * NSEC_PER_MSEC) / clk_rate;
-}
-
 /**
  * aemif_get_hw_params - function to read hw register values
  * @pdev: platform device to read for
@@ -231,19 +218,18 @@ static void aemif_get_hw_params(struct platform_device *pdev, int csnum)
 {
 	struct aemif_device *aemif = platform_get_drvdata(pdev);
 	struct aemif_cs_data *data = &aemif->cs_data[csnum];
-	unsigned long clk_rate = aemif->clk_rate;
 	u32 val, offset;
 
 	offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
 	val = readl(aemif->base + offset);
 
-	data->ta = aemif_cycles_to_nsec(TA_VAL(val), clk_rate);
-	data->rhold = aemif_cycles_to_nsec(RHOLD_VAL(val), clk_rate);
-	data->rstrobe = aemif_cycles_to_nsec(RSTROBE_VAL(val), clk_rate);
-	data->rsetup = aemif_cycles_to_nsec(RSETUP_VAL(val), clk_rate);
-	data->whold = aemif_cycles_to_nsec(WHOLD_VAL(val), clk_rate);
-	data->wstrobe = aemif_cycles_to_nsec(WSTROBE_VAL(val), clk_rate);
-	data->wsetup = aemif_cycles_to_nsec(WSETUP_VAL(val), clk_rate);
+	data->ta = TA_VAL(val);
+	data->rhold = RHOLD_VAL(val);
+	data->rstrobe = RSTROBE_VAL(val);
+	data->rsetup = RSETUP_VAL(val);
+	data->whold = WHOLD_VAL(val);
+	data->wstrobe = WSTROBE_VAL(val);
+	data->wsetup = WSETUP_VAL(val);
 	data->enable_ew = EW_VAL(val);
 	data->enable_ss = SSTROBE_VAL(val);
 	data->asize = val & ASIZE_MAX;
@@ -261,7 +247,9 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
 				      struct device_node *np)
 {
 	struct aemif_device *aemif = platform_get_drvdata(pdev);
+	unsigned long clk_rate = aemif->clk_rate;
 	struct aemif_cs_data *data;
+	int ret;
 	u32 cs;
 	u32 val;
 
@@ -287,26 +275,61 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
 	aemif_get_hw_params(pdev, aemif->num_cs++);
 
 	/* override the values from device node */
-	if (!of_property_read_u32(np, "ti,cs-min-turnaround-ns", &val))
-		data->ta = val;
+	if (!of_property_read_u32(np, "ti,cs-min-turnaround-ns", &val)) {
+		ret = aemif_calc_rate(pdev, val, clk_rate, TA_MAX);
+		if (ret < 0)
+			return ret;
 
-	if (!of_property_read_u32(np, "ti,cs-read-hold-ns", &val))
-		data->rhold = val;
+		data->ta = ret;
+	}
 
-	if (!of_property_read_u32(np, "ti,cs-read-strobe-ns", &val))
-		data->rstrobe = val;
+	if (!of_property_read_u32(np, "ti,cs-read-hold-ns", &val)) {
+		ret = aemif_calc_rate(pdev, val, clk_rate, RHOLD_MAX);
+		if (ret < 0)
+			return ret;
 
-	if (!of_property_read_u32(np, "ti,cs-read-setup-ns", &val))
-		data->rsetup = val;
+		data->rhold = ret;
+	}
 
-	if (!of_property_read_u32(np, "ti,cs-write-hold-ns", &val))
-		data->whold = val;
+	if (!of_property_read_u32(np, "ti,cs-read-strobe-ns", &val)) {
+		ret = aemif_calc_rate(pdev, val, clk_rate, RSTROBE_MAX);
+		if (ret < 0)
+			return ret;
 
-	if (!of_property_read_u32(np, "ti,cs-write-strobe-ns", &val))
-		data->wstrobe = val;
+		data->rstrobe = ret;
+	}
 
-	if (!of_property_read_u32(np, "ti,cs-write-setup-ns", &val))
-		data->wsetup = val;
+	if (!of_property_read_u32(np, "ti,cs-read-setup-ns", &val)) {
+		ret = aemif_calc_rate(pdev, val, clk_rate, RSETUP_MAX);
+		if (ret < 0)
+			return ret;
+
+		data->rsetup = ret;
+	}
+
+	if (!of_property_read_u32(np, "ti,cs-write-hold-ns", &val)) {
+		ret = aemif_calc_rate(pdev, val, clk_rate, WHOLD_MAX);
+		if (ret < 0)
+			return ret;
+
+		data->whold = ret;
+	}
+
+	if (!of_property_read_u32(np, "ti,cs-write-strobe-ns", &val)) {
+		ret = aemif_calc_rate(pdev, val, clk_rate, WSTROBE_MAX);
+		if (ret < 0)
+			return ret;
+
+		data->wstrobe = ret;
+	}
+
+	if (!of_property_read_u32(np, "ti,cs-write-setup-ns", &val)) {
+		ret = aemif_calc_rate(pdev, val, clk_rate, WSETUP_MAX);
+		if (ret < 0)
+			return ret;
+
+		data->wsetup = ret;
+	}
 
 	if (!of_property_read_u32(np, "ti,cs-bus-width", &val))
 		if (val == 16)
-- 
2.47.0


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

[PATCH v4 02/10] memory: ti-aemif: Remove unnecessary local variables

[Bastien Curutchet]

CS timings are copied to local variables that are then used as is,
without any modifications.

Remove these unneeded local variables and deal directly with the timings
stored in the struct aemif_cs_data.

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

diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
index bd0c49ba1939..6a751a23d41a 100644
--- a/drivers/memory/ti-aemif.c
+++ b/drivers/memory/ti-aemif.c
@@ -174,22 +174,14 @@ 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 offset;
 	u32 set, val;
 
 	offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
 
-	ta	=  data->ta;
-	rhold	=  data->rhold;
-	rstrobe	=  data->rstrobe;
-	rsetup	=  data->rsetup;
-	whold	=  data->whold;
-	wstrobe	=  data->wstrobe;
-	wsetup	=  data->wsetup;
-
-	set = TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) | RSETUP(rsetup) |
-		WHOLD(whold) | WSTROBE(wstrobe) | WSETUP(wsetup);
+	set = TA(data->ta) |
+		RHOLD(data->rhold) | RSTROBE(data->rstrobe) | RSETUP(data->rsetup) |
+		WHOLD(data->whold) | WSTROBE(data->wstrobe) | WSETUP(data->wsetup);
 
 	set |= (data->asize & ACR_ASIZE_MASK);
 	if (data->enable_ew)
-- 
2.47.0


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

[PATCH v4 03/10] memory: ti-aemif: Wrap CS timings into a struct

[Bastien Curutchet]

CS timings are store in the struct aemif_cs_data along with other CS
parameters. It isn't convenient for exposing CS timings to other drivers
without also exposing the other parameters.

Wrap the CS timings in a new the struct aemif_cs_timings to simplify
their export in upcoming patches.

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

diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
index 6a751a23d41a..aec6d6464efa 100644
--- a/drivers/memory/ti-aemif.c
+++ b/drivers/memory/ti-aemif.c
@@ -78,10 +78,8 @@
 				WSETUP(WSETUP_MAX) | \
 				EW(EW_MAX) | SSTROBE(SSTROBE_MAX) | \
 				ASIZE_MAX)
-
 /**
- * struct aemif_cs_data: structure to hold CS parameters
- * @cs: chip-select number
+ * struct aemif_cs_timings: structure to hold CS timings
  * @wstrobe: write strobe width, number of cycles - 1
  * @rstrobe: read strobe width, number of cycles - 1
  * @wsetup: write setup width, number of cycles - 1
@@ -89,12 +87,8 @@
  * @rsetup: read setup width, number of cycles - 1
  * @rhold: read hold width, number of cycles - 1
  * @ta: minimum turn around time, number of cycles - 1
- * @enable_ss: enable/disable select strobe mode
- * @enable_ew: enable/disable extended wait mode
- * @asize: width of the asynchronous device's data bus
  */
-struct aemif_cs_data {
-	u8	cs;
+struct aemif_cs_timings {
 	u32	wstrobe;
 	u32	rstrobe;
 	u32	wsetup;
@@ -102,6 +96,19 @@ struct aemif_cs_data {
 	u32	rsetup;
 	u32	rhold;
 	u32	ta;
+};
+
+/**
+ * struct aemif_cs_data: structure to hold CS parameters
+ * @timings: timings configuration
+ * @cs: chip-select number
+ * @enable_ss: enable/disable select strobe mode
+ * @enable_ew: enable/disable extended wait mode
+ * @asize: width of the asynchronous device's data bus
+ */
+struct aemif_cs_data {
+	struct aemif_cs_timings timings;
+	u8	cs;
 	u8	enable_ss;
 	u8	enable_ew;
 	u8	asize;
@@ -179,9 +186,10 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
 
 	offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
 
-	set = TA(data->ta) |
-		RHOLD(data->rhold) | RSTROBE(data->rstrobe) | RSETUP(data->rsetup) |
-		WHOLD(data->whold) | WSTROBE(data->wstrobe) | WSETUP(data->wsetup);
+	set = TA(data->timings.ta) |
+		RHOLD(data->timings.rhold) | RSTROBE(data->timings.rstrobe) |
+		RSETUP(data->timings.rsetup) | WHOLD(data->timings.whold) |
+		WSTROBE(data->timings.wstrobe) | WSETUP(data->timings.wsetup);
 
 	set |= (data->asize & ACR_ASIZE_MASK);
 	if (data->enable_ew)
@@ -215,13 +223,13 @@ static void aemif_get_hw_params(struct platform_device *pdev, int csnum)
 	offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
 	val = readl(aemif->base + offset);
 
-	data->ta = TA_VAL(val);
-	data->rhold = RHOLD_VAL(val);
-	data->rstrobe = RSTROBE_VAL(val);
-	data->rsetup = RSETUP_VAL(val);
-	data->whold = WHOLD_VAL(val);
-	data->wstrobe = WSTROBE_VAL(val);
-	data->wsetup = WSETUP_VAL(val);
+	data->timings.ta = TA_VAL(val);
+	data->timings.rhold = RHOLD_VAL(val);
+	data->timings.rstrobe = RSTROBE_VAL(val);
+	data->timings.rsetup = RSETUP_VAL(val);
+	data->timings.whold = WHOLD_VAL(val);
+	data->timings.wstrobe = WSTROBE_VAL(val);
+	data->timings.wsetup = WSETUP_VAL(val);
 	data->enable_ew = EW_VAL(val);
 	data->enable_ss = SSTROBE_VAL(val);
 	data->asize = val & ASIZE_MAX;
@@ -272,7 +280,7 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
 		if (ret < 0)
 			return ret;
 
-		data->ta = ret;
+		data->timings.ta = ret;
 	}
 
 	if (!of_property_read_u32(np, "ti,cs-read-hold-ns", &val)) {
@@ -280,7 +288,7 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
 		if (ret < 0)
 			return ret;
 
-		data->rhold = ret;
+		data->timings.rhold = ret;
 	}
 
 	if (!of_property_read_u32(np, "ti,cs-read-strobe-ns", &val)) {
@@ -288,7 +296,7 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
 		if (ret < 0)
 			return ret;
 
-		data->rstrobe = ret;
+		data->timings.rstrobe = ret;
 	}
 
 	if (!of_property_read_u32(np, "ti,cs-read-setup-ns", &val)) {
@@ -296,7 +304,7 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
 		if (ret < 0)
 			return ret;
 
-		data->rsetup = ret;
+		data->timings.rsetup = ret;
 	}
 
 	if (!of_property_read_u32(np, "ti,cs-write-hold-ns", &val)) {
@@ -304,7 +312,7 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
 		if (ret < 0)
 			return ret;
 
-		data->whold = ret;
+		data->timings.whold = ret;
 	}
 
 	if (!of_property_read_u32(np, "ti,cs-write-strobe-ns", &val)) {
@@ -312,7 +320,7 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
 		if (ret < 0)
 			return ret;
 
-		data->wstrobe = ret;
+		data->timings.wstrobe = ret;
 	}
 
 	if (!of_property_read_u32(np, "ti,cs-write-setup-ns", &val)) {
@@ -320,7 +328,7 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
 		if (ret < 0)
 			return ret;
 
-		data->wsetup = ret;
+		data->timings.wsetup = ret;
 	}
 
 	if (!of_property_read_u32(np, "ti,cs-bus-width", &val))
-- 
2.47.0


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

[PATCH v4 04/10] memory: ti-aemif: Create aemif_check_cs_timings()

[Bastien Curutchet]

aemif_calc_rate() check the validity of a new computed timing against a
'max' value given as input. This isn't convenient if we want to check
the CS timing configuration somewhere else in the code.

Wrap the verification of all the chip select's timing configuration into a
single function to ease its exportation in upcoming patches.
Remove the 'max' input from aemif_calc_rate() as it's no longer used.

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

diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
index aec6d6464efa..5c1c6f95185f 100644
--- a/drivers/memory/ti-aemif.c
+++ b/drivers/memory/ti-aemif.c
@@ -132,18 +132,48 @@ struct aemif_device {
 	struct aemif_cs_data cs_data[NUM_CS];
 };
 
+/**
+ * aemif_check_cs_timings - Check the validity of a CS timing configuration.
+ * @timings: timings configuration
+ *
+ * @return: 0 if the timing configuration is valid, negative error number otherwise.
+ */
+static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
+{
+	if (timings->ta > TA_MAX)
+		return -EINVAL;
+
+	if (timings->rhold > RHOLD_MAX)
+		return -EINVAL;
+
+	if (timings->rstrobe > RSTROBE_MAX)
+		return -EINVAL;
+
+	if (timings->rsetup > RSETUP_MAX)
+		return -EINVAL;
+
+	if (timings->whold > WHOLD_MAX)
+		return -EINVAL;
+
+	if (timings->wstrobe > WSTROBE_MAX)
+		return -EINVAL;
+
+	if (timings->wsetup > WSETUP_MAX)
+		return -EINVAL;
+
+	return 0;
+}
+
 /**
  * aemif_calc_rate - calculate timing data.
  * @pdev: platform device to calculate for
  * @wanted: The cycle time needed in nanoseconds.
  * @clk: The input clock rate in kHz.
- * @max: The maximum divider value that can be programmed.
  *
  * On success, returns the calculated timing value minus 1 for easy
  * programming into AEMIF timing registers, else negative errno.
  */
-static int aemif_calc_rate(struct platform_device *pdev, int wanted,
-			   unsigned long clk, int max)
+static int aemif_calc_rate(struct platform_device *pdev, int wanted, unsigned long clk)
 {
 	int result;
 
@@ -156,10 +186,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;
 }
 
@@ -249,7 +275,6 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
 	struct aemif_device *aemif = platform_get_drvdata(pdev);
 	unsigned long clk_rate = aemif->clk_rate;
 	struct aemif_cs_data *data;
-	int ret;
 	u32 cs;
 	u32 val;
 
@@ -275,68 +300,34 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
 	aemif_get_hw_params(pdev, aemif->num_cs++);
 
 	/* override the values from device node */
-	if (!of_property_read_u32(np, "ti,cs-min-turnaround-ns", &val)) {
-		ret = aemif_calc_rate(pdev, val, clk_rate, TA_MAX);
-		if (ret < 0)
-			return ret;
-
-		data->timings.ta = ret;
-	}
+	if (!of_property_read_u32(np, "ti,cs-min-turnaround-ns", &val))
+		data->timings.ta = aemif_calc_rate(pdev, val, clk_rate);
 
-	if (!of_property_read_u32(np, "ti,cs-read-hold-ns", &val)) {
-		ret = aemif_calc_rate(pdev, val, clk_rate, RHOLD_MAX);
-		if (ret < 0)
-			return ret;
-
-		data->timings.rhold = ret;
-	}
+	if (!of_property_read_u32(np, "ti,cs-read-hold-ns", &val))
+		data->timings.rhold = aemif_calc_rate(pdev, val, clk_rate);
 
-	if (!of_property_read_u32(np, "ti,cs-read-strobe-ns", &val)) {
-		ret = aemif_calc_rate(pdev, val, clk_rate, RSTROBE_MAX);
-		if (ret < 0)
-			return ret;
+	if (!of_property_read_u32(np, "ti,cs-read-strobe-ns", &val))
+		data->timings.rstrobe = aemif_calc_rate(pdev, val, clk_rate);
 
-		data->timings.rstrobe = ret;
-	}
+	if (!of_property_read_u32(np, "ti,cs-read-setup-ns", &val))
+		data->timings.rsetup = aemif_calc_rate(pdev, val, clk_rate);
 
-	if (!of_property_read_u32(np, "ti,cs-read-setup-ns", &val)) {
-		ret = aemif_calc_rate(pdev, val, clk_rate, RSETUP_MAX);
-		if (ret < 0)
-			return ret;
+	if (!of_property_read_u32(np, "ti,cs-write-hold-ns", &val))
+		data->timings.whold = aemif_calc_rate(pdev, val, clk_rate);
 
-		data->timings.rsetup = ret;
-	}
+	if (!of_property_read_u32(np, "ti,cs-write-strobe-ns", &val))
+		data->timings.wstrobe = aemif_calc_rate(pdev, val, clk_rate);
 
-	if (!of_property_read_u32(np, "ti,cs-write-hold-ns", &val)) {
-		ret = aemif_calc_rate(pdev, val, clk_rate, WHOLD_MAX);
-		if (ret < 0)
-			return ret;
-
-		data->timings.whold = ret;
-	}
-
-	if (!of_property_read_u32(np, "ti,cs-write-strobe-ns", &val)) {
-		ret = aemif_calc_rate(pdev, val, clk_rate, WSTROBE_MAX);
-		if (ret < 0)
-			return ret;
-
-		data->timings.wstrobe = ret;
-	}
-
-	if (!of_property_read_u32(np, "ti,cs-write-setup-ns", &val)) {
-		ret = aemif_calc_rate(pdev, val, clk_rate, WSETUP_MAX);
-		if (ret < 0)
-			return ret;
-
-		data->timings.wsetup = ret;
-	}
+	if (!of_property_read_u32(np, "ti,cs-write-setup-ns", &val))
+		data->timings.wsetup = aemif_calc_rate(pdev, val, clk_rate);
 
 	if (!of_property_read_u32(np, "ti,cs-bus-width", &val))
 		if (val == 16)
 			data->asize = 1;
 	data->enable_ew = of_property_read_bool(np, "ti,cs-extended-wait-mode");
 	data->enable_ss = of_property_read_bool(np, "ti,cs-select-strobe-mode");
-	return 0;
+
+	return aemif_check_cs_timings(&data->timings);
 }
 
 static const struct of_device_id aemif_of_match[] = {
-- 
2.47.0


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

[PATCH v4 05/10] 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.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/memory/ti-aemif.c | 66 +++++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 16 deletions(-)

diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
index 5c1c6f95185f..dae3441e0cd9 100644
--- a/drivers/memory/ti-aemif.c
+++ b/drivers/memory/ti-aemif.c
@@ -69,15 +69,16 @@
 #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_timings: structure to hold CS timings
  * @wstrobe: write strobe width, number of cycles - 1
@@ -164,6 +165,44 @@ static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
 	return 0;
 }
 
+/**
+ * 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
+ *
+ * @return: 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;
+	int ret;
+
+	if (!timings || !aemif)
+		return -EINVAL;
+
+	if (cs > aemif->num_cs)
+		return -EINVAL;
+
+	ret = aemif_check_cs_timings(timings);
+	if (ret)
+		return ret;
+
+	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
@@ -212,12 +251,7 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
 
 	offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
 
-	set = TA(data->timings.ta) |
-		RHOLD(data->timings.rhold) | RSTROBE(data->timings.rstrobe) |
-		RSETUP(data->timings.rsetup) | WHOLD(data->timings.whold) |
-		WSTROBE(data->timings.wstrobe) | WSETUP(data->timings.wsetup);
-
-	set |= (data->asize & ACR_ASIZE_MASK);
+	set = (data->asize & ACR_ASIZE_MASK);
 	if (data->enable_ew)
 		set |= ACR_EW_MASK;
 	if (data->enable_ss)
@@ -228,7 +262,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, &data->timings);
 }
 
 /**
-- 
2.47.0


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

[PATCH v4 06/10] memory: ti-aemif: Export aemif_*_cs_timings()

[Bastien Curutchet]

Export the aemif_set_cs_timing() and aemif_check_cs_timing() symbols so
they can be used by other drivers

Add a mutex to protect the CS configuration register from concurrent
accesses between the AEMIF and its 'children'.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/memory/ti-aemif.c       | 35 ++++++++++++---------------------
 include/linux/memory/ti-aemif.h | 32 ++++++++++++++++++++++++++++++
 2 files changed, 45 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 dae3441e0cd9..519283028eee 100644
--- a/drivers/memory/ti-aemif.c
+++ b/drivers/memory/ti-aemif.c
@@ -13,7 +13,9 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/memory/ti-aemif.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
@@ -79,26 +81,6 @@
 
 #define CONFIG_MASK	(EW(EW_MAX) | SSTROBE(SSTROBE_MAX) | ASIZE_MAX)
 
-/**
- * struct aemif_cs_timings: structure to hold CS timings
- * @wstrobe: write strobe width, number of cycles - 1
- * @rstrobe: read strobe width, number of cycles - 1
- * @wsetup: write setup width, number of cycles - 1
- * @whold: write hold width, number of cycles - 1
- * @rsetup: read setup width, number of cycles - 1
- * @rhold: read hold width, number of cycles - 1
- * @ta: minimum turn around time, number of cycles - 1
- */
-struct aemif_cs_timings {
-	u32	wstrobe;
-	u32	rstrobe;
-	u32	wsetup;
-	u32	whold;
-	u32	rsetup;
-	u32	rhold;
-	u32	ta;
-};
-
 /**
  * struct aemif_cs_data: structure to hold CS parameters
  * @timings: timings configuration
@@ -123,6 +105,7 @@ struct aemif_cs_data {
  * @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;
@@ -131,6 +114,7 @@ struct aemif_device {
 	u8 num_cs;
 	int cs_offset;
 	struct aemif_cs_data cs_data[NUM_CS];
+	struct mutex config_cs_lock;
 };
 
 /**
@@ -139,7 +123,7 @@ struct aemif_device {
  *
  * @return: 0 if the timing configuration is valid, negative error number otherwise.
  */
-static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
+int aemif_check_cs_timings(struct aemif_cs_timings *timings)
 {
 	if (timings->ta > TA_MAX)
 		return -EINVAL;
@@ -164,6 +148,7 @@ static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
 
 	return 0;
 }
+EXPORT_SYMBOL(aemif_check_cs_timings);
 
 /**
  * aemif_set_cs_timings - Set the timing configuration of a given chip select.
@@ -173,7 +158,7 @@ static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
  *
  * @return: 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 int offset;
 	u32 val, set;
@@ -195,13 +180,16 @@ static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_
 
 	offset = A1CR_OFFSET + cs * 4;
 
+	mutex_lock(&aemif->config_cs_lock);
 	val = readl(aemif->base + offset);
 	val &= ~TIMINGS_MASK;
 	val |= set;
 	writel(val, aemif->base + offset);
+	mutex_unlock(&aemif->config_cs_lock);
 
 	return 0;
 }
+EXPORT_SYMBOL(aemif_set_cs_timings);
 
 /**
  * aemif_calc_rate - calculate timing data.
@@ -257,10 +245,12 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
 	if (data->enable_ss)
 		set |= ACR_SSTROBE_MASK;
 
+	mutex_lock(&aemif->config_cs_lock);
 	val = readl(aemif->base + offset);
 	val &= ~CONFIG_MASK;
 	val |= set;
 	writel(val, aemif->base + offset);
+	mutex_unlock(&aemif->config_cs_lock);
 
 	return aemif_set_cs_timings(aemif, data->cs - aemif->cs_offset, &data->timings);
 }
@@ -399,6 +389,7 @@ static int aemif_probe(struct platform_device *pdev)
 	if (IS_ERR(aemif->base))
 		return PTR_ERR(aemif->base);
 
+	mutex_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..0640d30f6321
--- /dev/null
+++ b/include/linux/memory/ti-aemif.h
@@ -0,0 +1,32 @@
+/* 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);
+int aemif_check_cs_timings(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/

[PATCH v4 07/10] 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 v4 08/10] 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 v4 09/10] 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 v4 10/10] 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]

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

diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
index 563045c7ce08..00627c2783f8 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,82 @@ 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;
+	int ret;
+
+	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);
+
+	ret = aemif_check_cs_timings(&timings);
+	if (ret || chipnr == NAND_DATA_IFACE_CHECK_ONLY)
+		return ret;
+
+	return aemif_set_cs_timings(info->aemif, info->core_chipsel, &timings);
+}
+
 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 +910,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 v4 01/10] memory: ti-aemif: Store timings parameter in number of cycles - 1

[Miquel Raynal]

On 15/11/2024 at 14:26:22 +01, Bastien Curutchet <bastien.curutchet@bootlin.com> wrote:

> The CS configuration register expects timings to be expressed in
> 'number of cycles - 1' but they are stored in ns in the struct
> aemif_cs_data. So at init, the timings currently set are converted to ns
> by aemif_get_hw_params(), updated with values from the device-tree
> properties, and then converted back to 'number of cycles - 1' before
> being applied.
>
> Store the timings directly in 'number of cycles - 1' instead of
> nanoseconds.
> Perform the conversion from nanosecond during the device-tree parsing.
> Remove aemif_cycles_to_nsec() as it isn't used anymore.
>
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>

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

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

Re: [PATCH v4 03/10] memory: ti-aemif: Wrap CS timings into a struct

[Miquel Raynal]

On 15/11/2024 at 14:26:24 +01, Bastien Curutchet <bastien.curutchet@bootlin.com> wrote:

> CS timings are store in the struct aemif_cs_data along with other CS
> parameters. It isn't convenient for exposing CS timings to other drivers
> without also exposing the other parameters.
>
> Wrap the CS timings in a new the struct aemif_cs_timings to simplify

s/the//                        ^^^

> their export in upcoming patches.
>
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>

But otherwise this feels very sensible.

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

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

Re: [PATCH v4 02/10] memory: ti-aemif: Remove unnecessary local variables

[Miquel Raynal]

On 15/11/2024 at 14:26:23 +01, Bastien Curutchet <bastien.curutchet@bootlin.com> wrote:

> CS timings are copied to local variables that are then used as is,
> without any modifications.
>
> Remove these unneeded local variables and deal directly with the timings
> stored in the struct aemif_cs_data.
>
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---

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

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

Re: [PATCH v4 04/10] memory: ti-aemif: Create aemif_check_cs_timings()

[Miquel Raynal]

On 15/11/2024 at 14:26:25 +01, Bastien Curutchet <bastien.curutchet@bootlin.com> wrote:

> aemif_calc_rate() check the validity of a new computed timing against a

                    checks

> 'max' value given as input. This isn't convenient if we want to check
> the CS timing configuration somewhere else in the code.
>
> Wrap the verification of all the chip select's timing configuration into a
> single function to ease its exportation in upcoming patches.
> Remove the 'max' input from aemif_calc_rate() as it's no longer used.
>
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>

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

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

Re: [PATCH v4 00/10] Implement setup_interface() in the DaVinci NAND controller

[Krzysztof Kozlowski]

On 15/11/2024 14:26, 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.
> 
> PATCH 6 depends on PATCH 1-2-3-4-5
> PATCH 10 depends on PATCH 6-8-9

That's a bit too many to go via separate tree, so I'll provide them via
stable tag to Miquel/MTD.

Best regards,
Krzysztof

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

Re: [PATCH v4 03/10] memory: ti-aemif: Wrap CS timings into a struct

[Krzysztof Kozlowski]

On 15/11/2024 14:26, Bastien Curutchet wrote:
> CS timings are store in the struct aemif_cs_data along with other CS
> parameters. It isn't convenient for exposing CS timings to other drivers
> without also exposing the other parameters.
> 
> Wrap the CS timings in a new the struct aemif_cs_timings to simplify
> their export in upcoming patches.
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  drivers/memory/ti-aemif.c | 58 ++++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
> index 6a751a23d41a..aec6d6464efa 100644
> --- a/drivers/memory/ti-aemif.c
> +++ b/drivers/memory/ti-aemif.c
> @@ -78,10 +78,8 @@
>  				WSETUP(WSETUP_MAX) | \
>  				EW(EW_MAX) | SSTROBE(SSTROBE_MAX) | \
>  				ASIZE_MAX)
> -

This does not look related change.

>  /**
> - * struct aemif_cs_data: structure to hold CS parameters
> - * @cs: chip-select number
> + * struct aemif_cs_timings: structure to hold CS timings
>   * @wstrobe: write strobe width, number of cycles - 1
>   * @rstrobe: read strobe width, number of cycles - 1
>   * @wsetup: write setup width, number of cycles - 1
> @@ -89,12 +87,8 @@
>   * @rsetup: read setup width, number of cycles - 1
>   * @rhold: read hold width, number of cycles - 1
>   * @ta: minimum turn around time, number of cycles - 1
> - * @enable_ss: enable/disable select strobe mode
> - * @enable_ew: enable/disable extended wait mode
> - * @asize: width of the asynchronous device's data


Best regards,
Krzysztof

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

Re: [PATCH v4 01/10] memory: ti-aemif: Store timings parameter in number of cycles - 1

[Krzysztof Kozlowski]

On 15/11/2024 14:26, Bastien Curutchet wrote:
> The CS configuration register expects timings to be expressed in
> 'number of cycles - 1' but they are stored in ns in the struct
> aemif_cs_data. So at init, the timings currently set are converted to ns
> by aemif_get_hw_params(), updated with values from the device-tree
> properties, and then converted back to 'number of cycles - 1' before
> being applied.
> 
> Store the timings directly in 'number of cycles - 1' instead of
> nanoseconds.
> Perform the conversion from nanosecond during the device-tree parsing.
> Remove aemif_cycles_to_nsec() as it isn't used anymore.
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  drivers/memory/ti-aemif.c | 137 ++++++++++++++++++++++----------------
>  1 file changed, 80 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
> index d54dc3cfff73..bd0c49ba1939 100644
> --- a/drivers/memory/ti-aemif.c
> +++ b/drivers/memory/ti-aemif.c
> @@ -80,28 +80,28 @@
>  				ASIZE_MAX)
>  
>  /**
> - * struct aemif_cs_data: structure to hold cs parameters
> + * struct aemif_cs_data: structure to hold CS parameters

You are changing this line in next patch, so just do it there.

Best regards,
Krzysztof

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

Re: [PATCH v4 04/10] memory: ti-aemif: Create aemif_check_cs_timings()

[Krzysztof Kozlowski]

On 15/11/2024 14:26, Bastien Curutchet wrote:
> aemif_calc_rate() check the validity of a new computed timing against a
> 'max' value given as input. This isn't convenient if we want to check
> the CS timing configuration somewhere else in the code.
> 
> Wrap the verification of all the chip select's timing configuration into a
> single function to ease its exportation in upcoming patches.
> Remove the 'max' input from aemif_calc_rate() as it's no longer used.
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  drivers/memory/ti-aemif.c | 107 +++++++++++++++++---------------------
>  1 file changed, 49 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
> index aec6d6464efa..5c1c6f95185f 100644
> --- a/drivers/memory/ti-aemif.c
> +++ b/drivers/memory/ti-aemif.c
> @@ -132,18 +132,48 @@ struct aemif_device {
>  	struct aemif_cs_data cs_data[NUM_CS];
>  };
>  
> +/**
> + * aemif_check_cs_timings - Check the validity of a CS timing configuration.
> + * @timings: timings configuration
> + *
> + * @return: 0 if the timing configuration is valid, negative error number otherwise.
> + */
> +static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
> +{
> +	if (timings->ta > TA_MAX)
> +		return -EINVAL;
> +
> +	if (timings->rhold > RHOLD_MAX)
> +		return -EINVAL;
> +
> +	if (timings->rstrobe > RSTROBE_MAX)
> +		return -EINVAL;
> +
> +	if (timings->rsetup > RSETUP_MAX)
> +		return -EINVAL;
> +
> +	if (timings->whold > WHOLD_MAX)
> +		return -EINVAL;
> +
> +	if (timings->wstrobe > WSTROBE_MAX)
> +		return -EINVAL;
> +
> +	if (timings->wsetup > WSETUP_MAX)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  /**
>   * aemif_calc_rate - calculate timing data.
>   * @pdev: platform device to calculate for
>   * @wanted: The cycle time needed in nanoseconds.
>   * @clk: The input clock rate in kHz.
> - * @max: The maximum divider value that can be programmed.
>   *
>   * On success, returns the calculated timing value minus 1 for easy
>   * programming into AEMIF timing registers, else negative errno.
>   */
> -static int aemif_calc_rate(struct platform_device *pdev, int wanted,
> -			   unsigned long clk, int max)
> +static int aemif_calc_rate(struct platform_device *pdev, int wanted, unsigned long clk)
>  {
>  	int result;
>  
> @@ -156,10 +186,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;
>  }
>  
> @@ -249,7 +275,6 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
>  	struct aemif_device *aemif = platform_get_drvdata(pdev);
>  	unsigned long clk_rate = aemif->clk_rate;
>  	struct aemif_cs_data *data;
> -	int ret;
>  	u32 cs;
>  	u32 val;
>  
> @@ -275,68 +300,34 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
>  	aemif_get_hw_params(pdev, aemif->num_cs++);
>  
>  	/* override the values from device node */
> -	if (!of_property_read_u32(np, "ti,cs-min-turnaround-ns", &val)) {
> -		ret = aemif_calc_rate(pdev, val, clk_rate, TA_MAX);
> -		if (ret < 0)
> -			return ret;
> -
> -		data->timings.ta = ret;
> -	}
> +	if (!of_property_read_u32(np, "ti,cs-min-turnaround-ns", &val))
> +		data->timings.ta = aemif_calc_rate(pdev, val, clk_rate);
>  

You just changed these lines in patch #1. Basically this is partial
revert of #1.

Best regards,
Krzysztof

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

Re: [PATCH v4 06/10] memory: ti-aemif: Export aemif_*_cs_timings()

[Krzysztof Kozlowski]

On 15/11/2024 14:26, Bastien Curutchet wrote:
>  	return 0;
>  }
> +EXPORT_SYMBOL(aemif_check_cs_timings);
>  
>  /**
>   * aemif_set_cs_timings - Set the timing configuration of a given chip select.
> @@ -173,7 +158,7 @@ static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
>   *
>   * @return: 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 int offset;
>  	u32 val, set;
> @@ -195,13 +180,16 @@ static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_
>  
>  	offset = A1CR_OFFSET + cs * 4;
>  
> +	mutex_lock(&aemif->config_cs_lock);
>  	val = readl(aemif->base + offset);
>  	val &= ~TIMINGS_MASK;
>  	val |= set;
>  	writel(val, aemif->base + offset);
> +	mutex_unlock(&aemif->config_cs_lock);
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(aemif_set_cs_timings);


EXPORT_SYMBOL_GPL everywhere, these are quite specific to driver's
internals, so internal implementation.

Also, all of exported functions need to have correct kerneldoc but I
think they don't. At least missing(), so maybe rest also did not conform
to kernel doc style.

>  
>  /**
>   * aemif_calc_rate - calculate timing data.
> @@ -257,10 +245,12 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
>  	if (data->enable_ss)
>  		set |= ACR_SSTROBE_MASK;
>  
> +	mutex_lock(&aemif->config_cs_lock);
>  	val = readl(aemif->base + offset);
>  	val &= ~CONFIG_MASK;
>  	val |= set;
>  	writel(val, aemif->base + offset);
> +	mutex_unlock(&aemif->config_cs_lock);
>  
>  	return aemif_set_cs_timings(aemif, data->cs - aemif->cs_offset, &data->timings);
>  }
> @@ -399,6 +389,7 @@ static int aemif_probe(struct platform_device *pdev)
>  	if (IS_ERR(aemif->base))
>  		return PTR_ERR(aemif->base);
>  
> +	mutex_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..0640d30f6321
> --- /dev/null
> +++ b/include/linux/memory/ti-aemif.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __TI_AEMIF_H
> +#define __TI_AEMIF_H


Use some longer header guard, e.g. __MEMORY_TI_AEMIF_H

> +
> +/**



Best regards,
Krzysztof

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

Re: [PATCH v4 00/10] Implement setup_interface() in the DaVinci NAND controller

[Miquel Raynal]

Hello Krzysztof,

On 21/11/2024 at 11:23:51 +01, Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 15/11/2024 14:26, 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.
>> 
>> PATCH 6 depends on PATCH 1-2-3-4-5
>> PATCH 10 depends on PATCH 6-8-9
>
> That's a bit too many to go via separate tree, so I'll provide them via
> stable tag to Miquel/MTD.

Yep, works perfectly for me.

Cheers,
Miquèl

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

Re: [PATCH v4 04/10] memory: ti-aemif: Create aemif_check_cs_timings()

[Bastien Curutchet]

Hi Krzysztof,

On 11/21/24 11:28 AM, Krzysztof Kozlowski wrote:
> On 15/11/2024 14:26, Bastien Curutchet wrote:
>> aemif_calc_rate() check the validity of a new computed timing against a
>> 'max' value given as input. This isn't convenient if we want to check
>> the CS timing configuration somewhere else in the code.
>>
>> Wrap the verification of all the chip select's timing configuration into a
>> single function to ease its exportation in upcoming patches.
>> Remove the 'max' input from aemif_calc_rate() as it's no longer used.
>>
>> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
>> ---
>>   drivers/memory/ti-aemif.c | 107 +++++++++++++++++---------------------
>>   1 file changed, 49 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
>> index aec6d6464efa..5c1c6f95185f 100644
>> --- a/drivers/memory/ti-aemif.c
>> +++ b/drivers/memory/ti-aemif.c
>> @@ -132,18 +132,48 @@ struct aemif_device {
>>   	struct aemif_cs_data cs_data[NUM_CS];
>>   };
>>   
>> +/**
>> + * aemif_check_cs_timings - Check the validity of a CS timing configuration.
>> + * @timings: timings configuration
>> + *
>> + * @return: 0 if the timing configuration is valid, negative error number otherwise.
>> + */
>> +static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
>> +{
>> +	if (timings->ta > TA_MAX)
>> +		return -EINVAL;
>> +
>> +	if (timings->rhold > RHOLD_MAX)
>> +		return -EINVAL;
>> +
>> +	if (timings->rstrobe > RSTROBE_MAX)
>> +		return -EINVAL;
>> +
>> +	if (timings->rsetup > RSETUP_MAX)
>> +		return -EINVAL;
>> +
>> +	if (timings->whold > WHOLD_MAX)
>> +		return -EINVAL;
>> +
>> +	if (timings->wstrobe > WSTROBE_MAX)
>> +		return -EINVAL;
>> +
>> +	if (timings->wsetup > WSETUP_MAX)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>>   /**
>>    * aemif_calc_rate - calculate timing data.
>>    * @pdev: platform device to calculate for
>>    * @wanted: The cycle time needed in nanoseconds.
>>    * @clk: The input clock rate in kHz.
>> - * @max: The maximum divider value that can be programmed.
>>    *
>>    * On success, returns the calculated timing value minus 1 for easy
>>    * programming into AEMIF timing registers, else negative errno.
>>    */
>> -static int aemif_calc_rate(struct platform_device *pdev, int wanted,
>> -			   unsigned long clk, int max)
>> +static int aemif_calc_rate(struct platform_device *pdev, int wanted, unsigned long clk)
>>   {
>>   	int result;
>>   
>> @@ -156,10 +186,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;
>>   }
>>   
>> @@ -249,7 +275,6 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
>>   	struct aemif_device *aemif = platform_get_drvdata(pdev);
>>   	unsigned long clk_rate = aemif->clk_rate;
>>   	struct aemif_cs_data *data;
>> -	int ret;
>>   	u32 cs;
>>   	u32 val;
>>   
>> @@ -275,68 +300,34 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
>>   	aemif_get_hw_params(pdev, aemif->num_cs++);
>>   
>>   	/* override the values from device node */
>> -	if (!of_property_read_u32(np, "ti,cs-min-turnaround-ns", &val)) {
>> -		ret = aemif_calc_rate(pdev, val, clk_rate, TA_MAX);
>> -		if (ret < 0)
>> -			return ret;
>> -
>> -		data->timings.ta = ret;
>> -	}
>> +	if (!of_property_read_u32(np, "ti,cs-min-turnaround-ns", &val))
>> +		data->timings.ta = aemif_calc_rate(pdev, val, clk_rate);
>>   
> 
> You just changed these lines in patch #1. Basically this is partial
> revert of #1.
> 

IMHO this isn't a partial revert of patch #1. Patch #1 moved the call of 
aemif_calc_rate() from aemif_config_abus() to here. Then, this patch 
removes the check of the aemif_calc_rate() as it no longer returns 
negative values. Maybe I should change the aemif_calc_rate() return type 
to u32 by the way.


Best regards,
Bastien


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

Re: [PATCH v4 06/10] memory: ti-aemif: Export aemif_*_cs_timings()

[Bastien Curutchet]

Hi Krzysztof,

On 11/21/24 11:33 AM, Krzysztof Kozlowski wrote:
> On 15/11/2024 14:26, Bastien Curutchet wrote:
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL(aemif_check_cs_timings);
>>   
>>   /**
>>    * aemif_set_cs_timings - Set the timing configuration of a given chip select.
>> @@ -173,7 +158,7 @@ static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
>>    *
>>    * @return: 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 int offset;
>>   	u32 val, set;
>> @@ -195,13 +180,16 @@ static int aemif_set_cs_timings(struct aemif_device *aemif, u8 cs, struct aemif_
>>   
>>   	offset = A1CR_OFFSET + cs * 4;
>>   
>> +	mutex_lock(&aemif->config_cs_lock);
>>   	val = readl(aemif->base + offset);
>>   	val &= ~TIMINGS_MASK;
>>   	val |= set;
>>   	writel(val, aemif->base + offset);
>> +	mutex_unlock(&aemif->config_cs_lock);
>>   
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL(aemif_set_cs_timings);
> 
> 
> EXPORT_SYMBOL_GPL everywhere, these are quite specific to driver's
> internals, so internal implementation.

Ok, I'll use EXPORT_SYMBOL_GPL in next iteration.

> 
> Also, all of exported functions need to have correct kerneldoc but I
> think they don't. At least missing(), so maybe rest also did not conform
> to kernel doc style.
> 

Do you know a tool that could help me checking the kernel doc style ? I 
have no warning when running ```make W=1n drivers/memory/ti-aemif.o```. 
The warnings I get when running ```make W=2 drivers/memory/ti-aemif.o``` 
aren't linked to these comments. And the output I get with 
```./scripts/kernel-doc -export  drivers/memory/ti-aemif.c``` seems fine 
to me.

>>   
>>   /**
>>    * aemif_calc_rate - calculate timing data.
>> @@ -257,10 +245,12 @@ static int aemif_config_abus(struct platform_device *pdev, int csnum)
>>   	if (data->enable_ss)
>>   		set |= ACR_SSTROBE_MASK;
>>   
>> +	mutex_lock(&aemif->config_cs_lock);
>>   	val = readl(aemif->base + offset);
>>   	val &= ~CONFIG_MASK;
>>   	val |= set;
>>   	writel(val, aemif->base + offset);
>> +	mutex_unlock(&aemif->config_cs_lock);
>>   
>>   	return aemif_set_cs_timings(aemif, data->cs - aemif->cs_offset, &data->timings);
>>   }
>> @@ -399,6 +389,7 @@ static int aemif_probe(struct platform_device *pdev)
>>   	if (IS_ERR(aemif->base))
>>   		return PTR_ERR(aemif->base);
>>   
>> +	mutex_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..0640d30f6321
>> --- /dev/null
>> +++ b/include/linux/memory/ti-aemif.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef __TI_AEMIF_H
>> +#define __TI_AEMIF_H
> 
> 
> Use some longer header guard, e.g. __MEMORY_TI_AEMIF_H
> 

Ok, I'll do it in next iteration.


Best regards,
Bastien

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