Hello,
I am trying to replace the xway_nand driver (which is still using the
legacy NAND API) with the intel-nand-controller driver. The Intel LGM
IP (for which intel-nand-controller was implemented) uses a newer
version of the EBU NAND and HSNAND IP found in Lantiq XWAY SoCs. The
most notable change is the addition of HSNAND Intel LGM SoCs (it's not
clear to me if/which Lantiq SoCs also have this DMA engine).
While testing my changes on a Lantiq xRX200 SoC I came across some
issues with the intel-nand-controller driver. The problems I found are:
1) Mismatch between dt-bindings and driver implementation (compatible
string, patch #1 and patch #4) and hardware capabilities (number of
CS lines, patch #1).
2) The driver reads the CS (chip select) line from the NAND controller's
reg property. In the dt-bindings example this is 0xe0f00000. Instead
it must be read from the NAND chip (child node).
3) A few smaller code cleanups to make the driver easier to understand
(patches #5 to #8)
4) I tried to understand the timing parameter calculation code but found
that it probably doesn't work on the Intel LGM SoCs either. The
dt-bindings example use clock ID 125 which is LGM_GCLK_EBU. So far
this is fine because EBU is the actual IP block for the NAND
interface. However, drivers/clk/x86/clk-lgm.c defines this clock as
a gate without a parent, so it's rate (as read by Linux) is always 0.
The intel-nand-controller driver then tries to calculate:
rate = clk_get_rate(ctrl->clk) / HZ_PER_MHZ
(rate will be 0 because clk_get_rate() returns 0) and then:
DIV_ROUND_UP(USEC_PER_SEC, rate)
(this then tries to divide by zero)
For me to move forward with the transition from xway_nand to the
intel-nand-controller driver I to understand a few more details:
- Who from Maxlinear (who took over Intel's AnyWAN division, which
previously worked on the drivers for the Intel LGM SoCs) can send a
patch to correct the LGM_GCLK_EBU clock rate in
drivers/clk/x86/clk-lgm.c? Or is LGM dead and the various drivers
should be removed instead?
- Who from Maxlinear can provide insights into which clock is connected
to the EBU NAND controller on Lantiq XWAY (Danube, xRX100, xRX200,
xRX300) SoCs as well as newer GRX350/GRX550 SoCs so that I can make
the intel-nand-controller work without hardcoded timing settings on
the XWAY SoCs?
Due to the severity of issues 2) and 4) above I am targeting linux-next
with this series. In my opinion there's no point in backporting these
fixes to a driver which has been broken since it was upstreamed.
Changes since v1 from [0]:
- Thanks to Miguel for confirming that the reg property of the NAND chip
is the chip select number of the NAND controller. I removed a question
about this from the cover-letter.
- Fixed accidental $id change in patch #1 which fixes a binding error
reported by Rob's bot
- Dropped RFC status
Changes since v2 from [1]:
- Renamed the binding file (in patch #1) to match the new compatible
string as suggested by Rob
- Added Rob's Acked-by to patch #2 (thank you!)
Best regards,
Martin
[0] https://lore.kernel.org/linux-mtd/20220628163850.17c56935@xps-13/T/#m4b2b6e1c970adf074a17ab9568637aff90e6ca36
[1] https://lore.kernel.org/linux-mtd/[email protected]/T/#u
Martin Blumenstingl (8):
dt-bindings: mtd: intel: lgm-nand: Fix compatible string
dt-bindings: mtd: intel: lgm-nand: Fix maximum chip select value
mtd: rawnand: intel: Read the chip-select line from the correct OF
node
mtd: rawnand: intel: Remove undocumented compatible string
mtd: rawnand: intel: Don't re-define NAND_DATA_IFACE_CHECK_ONLY
mtd: rawnand: intel: Remove unused nand_pa member from ebu_nand_cs
mtd: rawnand: intel: Remove unused clk_rate member from struct
ebu_nand
mtd: rawnand: intel: Use devm_platform_ioremap_resource_byname()
...l,lgm-nand.yaml => intel,lgm-ebunand.yaml} | 8 +++---
drivers/mtd/nand/raw/intel-nand-controller.c | 28 +++++++++----------
2 files changed, 17 insertions(+), 19 deletions(-)
rename Documentation/devicetree/bindings/mtd/{intel,lgm-nand.yaml => intel,lgm-ebunand.yaml} (91%)
--
2.37.0
The chip select has to be read from the flash node which is a child node
of the NAND controller.
Fixes: 0b1039f016e8a3 ("mtd: rawnand: Add NAND controller support on Intel LGM SoC")
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/mtd/nand/raw/intel-nand-controller.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/raw/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c
index e91b879b32bd..3df3f32423f9 100644
--- a/drivers/mtd/nand/raw/intel-nand-controller.c
+++ b/drivers/mtd/nand/raw/intel-nand-controller.c
@@ -16,6 +16,7 @@
#include <linux/mtd/rawnand.h>
#include <linux/mtd/nand.h>
+#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/sched.h>
#include <linux/slab.h>
@@ -580,6 +581,7 @@ static int ebu_nand_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct ebu_nand_controller *ebu_host;
+ struct device_node *chip_np;
struct nand_chip *nand;
struct mtd_info *mtd;
struct resource *res;
@@ -604,7 +606,12 @@ static int ebu_nand_probe(struct platform_device *pdev)
if (IS_ERR(ebu_host->hsnand))
return PTR_ERR(ebu_host->hsnand);
- ret = device_property_read_u32(dev, "reg", &cs);
+ chip_np = of_get_next_child(dev->of_node, NULL);
+ if (!chip_np)
+ return dev_err_probe(dev, -EINVAL,
+ "Could not find child node for the NAND chip\n");
+
+ ret = of_property_read_u32(chip_np, "reg", &cs);
if (ret) {
dev_err(dev, "failed to get chip select: %d\n", ret);
return ret;
@@ -660,7 +667,7 @@ static int ebu_nand_probe(struct platform_device *pdev)
writel(ebu_host->cs[cs].addr_sel | EBU_ADDR_MASK(5) | EBU_ADDR_SEL_REGEN,
ebu_host->ebu + EBU_ADDR_SEL(cs));
- nand_set_flash_node(&ebu_host->chip, dev->of_node);
+ nand_set_flash_node(&ebu_host->chip, chip_np);
mtd = nand_to_mtd(&ebu_host->chip);
if (!mtd->name) {
--
2.37.0
The Intel LGM NAND IP only supports two chip selects: There's only two
CS and ADDR_SEL register sets. Fix the maximum allowed chip select value
according to the dt-bindings.
Fixes: 2f9cea8eae44f5 ("dt-bindings: mtd: Add Nand Flash Controller support for Intel LGM SoC")
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Martin Blumenstingl <[email protected]>
---
Documentation/devicetree/bindings/mtd/intel,lgm-ebunand.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/mtd/intel,lgm-ebunand.yaml b/Documentation/devicetree/bindings/mtd/intel,lgm-ebunand.yaml
index 763ee3e1faf3..04f26196c4c1 100644
--- a/Documentation/devicetree/bindings/mtd/intel,lgm-ebunand.yaml
+++ b/Documentation/devicetree/bindings/mtd/intel,lgm-ebunand.yaml
@@ -51,7 +51,7 @@ patternProperties:
properties:
reg:
minimum: 0
- maximum: 7
+ maximum: 1
nand-ecc-mode: true
--
2.37.0
The "intel,nand-controller" compatible string is not part of the
dt-bindings. Remove it from the driver as it's not supposed to be used
without any documentation for it.
Fixes: 0b1039f016e8a3 ("mtd: rawnand: Add NAND controller support on Intel LGM SoC")
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/mtd/nand/raw/intel-nand-controller.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c
index 3df3f32423f9..056835fd4562 100644
--- a/drivers/mtd/nand/raw/intel-nand-controller.c
+++ b/drivers/mtd/nand/raw/intel-nand-controller.c
@@ -723,7 +723,6 @@ static int ebu_nand_remove(struct platform_device *pdev)
}
static const struct of_device_id ebu_nand_match[] = {
- { .compatible = "intel,nand-controller" },
{ .compatible = "intel,lgm-ebunand" },
{}
};
--
2.37.0
NAND_DATA_IFACE_CHECK_ONLY is already defined in
include/linux/mtd/rawnand.h which is also included by the driver. Drop
the re-definition from the intel-nand-controller driver.
Fixes: 0b1039f016e8a3 ("mtd: rawnand: Add NAND controller support on Intel LGM SoC")
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/mtd/nand/raw/intel-nand-controller.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mtd/nand/raw/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c
index 056835fd4562..3df16d5ecae8 100644
--- a/drivers/mtd/nand/raw/intel-nand-controller.c
+++ b/drivers/mtd/nand/raw/intel-nand-controller.c
@@ -100,8 +100,6 @@
#define HSNAND_ECC_OFFSET 0x008
-#define NAND_DATA_IFACE_CHECK_ONLY -1
-
#define MAX_CS 2
#define USEC_PER_SEC 1000000L
--
2.37.0
The nand_pa member from struct ebu_nand_cs is only written but never
read. Remove this unused and unneeded member.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/mtd/nand/raw/intel-nand-controller.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mtd/nand/raw/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c
index 3df16d5ecae8..de4f85368988 100644
--- a/drivers/mtd/nand/raw/intel-nand-controller.c
+++ b/drivers/mtd/nand/raw/intel-nand-controller.c
@@ -106,7 +106,6 @@
struct ebu_nand_cs {
void __iomem *chipaddr;
- dma_addr_t nand_pa;
u32 addr_sel;
};
@@ -626,7 +625,6 @@ static int ebu_nand_probe(struct platform_device *pdev)
ebu_host->cs[cs].chipaddr = devm_ioremap_resource(dev, res);
if (IS_ERR(ebu_host->cs[cs].chipaddr))
return PTR_ERR(ebu_host->cs[cs].chipaddr);
- ebu_host->cs[cs].nand_pa = res->start;
ebu_host->clk = devm_clk_get(dev, NULL);
if (IS_ERR(ebu_host->clk))
--
2.37.0
Switch from open-coded platform_get_resource_byname() and
devm_ioremap_resource() to devm_platform_ioremap_resource_byname() where
possible to simplify the code.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/mtd/nand/raw/intel-nand-controller.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/nand/raw/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c
index e486db11ecc3..d4a0987e93ac 100644
--- a/drivers/mtd/nand/raw/intel-nand-controller.c
+++ b/drivers/mtd/nand/raw/intel-nand-controller.c
@@ -592,13 +592,11 @@ static int ebu_nand_probe(struct platform_device *pdev)
ebu_host->dev = dev;
nand_controller_init(&ebu_host->controller);
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
- ebu_host->ebu = devm_ioremap_resource(&pdev->dev, res);
+ ebu_host->ebu = devm_platform_ioremap_resource_byname(pdev, "ebunand");
if (IS_ERR(ebu_host->ebu))
return PTR_ERR(ebu_host->ebu);
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
- ebu_host->hsnand = devm_ioremap_resource(&pdev->dev, res);
+ ebu_host->hsnand = devm_platform_ioremap_resource_byname(pdev, "hsnand");
if (IS_ERR(ebu_host->hsnand))
return PTR_ERR(ebu_host->hsnand);
@@ -620,8 +618,8 @@ static int ebu_nand_probe(struct platform_device *pdev)
ebu_host->cs_num = cs;
resname = devm_kasprintf(dev, GFP_KERNEL, "nand_cs%d", cs);
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, resname);
- ebu_host->cs[cs].chipaddr = devm_ioremap_resource(dev, res);
+ ebu_host->cs[cs].chipaddr = devm_platform_ioremap_resource_byname(pdev,
+ resname);
if (IS_ERR(ebu_host->cs[cs].chipaddr))
return PTR_ERR(ebu_host->cs[cs].chipaddr);
--
2.37.0
The driver which was added at the same time as the dt-bindings uses the
compatible string "intel,lgm-ebunand". Use the same compatible string
also in the dt-bindings and rename the bindings file accordingly.
Fixes: 2f9cea8eae44f5 ("dt-bindings: mtd: Add Nand Flash Controller support for Intel LGM SoC")
Signed-off-by: Martin Blumenstingl <[email protected]>
---
.../mtd/{intel,lgm-nand.yaml => intel,lgm-ebunand.yaml} | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
rename Documentation/devicetree/bindings/mtd/{intel,lgm-nand.yaml => intel,lgm-ebunand.yaml} (92%)
diff --git a/Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml b/Documentation/devicetree/bindings/mtd/intel,lgm-ebunand.yaml
similarity index 92%
rename from Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml
rename to Documentation/devicetree/bindings/mtd/intel,lgm-ebunand.yaml
index 30e0c66ab0eb..763ee3e1faf3 100644
--- a/Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml
+++ b/Documentation/devicetree/bindings/mtd/intel,lgm-ebunand.yaml
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
%YAML 1.2
---
-$id: http://devicetree.org/schemas/mtd/intel,lgm-nand.yaml#
+$id: http://devicetree.org/schemas/mtd/intel,lgm-ebunand.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
title: Intel LGM SoC NAND Controller Device Tree Bindings
@@ -14,7 +14,7 @@ maintainers:
properties:
compatible:
- const: intel,lgm-nand
+ const: intel,lgm-ebunand
reg:
maxItems: 6
@@ -75,7 +75,7 @@ additionalProperties: false
examples:
- |
nand-controller@e0f00000 {
- compatible = "intel,lgm-nand";
+ compatible = "intel,lgm-ebunand";
reg = <0xe0f00000 0x100>,
<0xe1000000 0x300>,
<0xe1400000 0x8000>,
--
2.37.0
The clk_rate member from struct ebu_nand is only written but never read.
Remove this unused and unneeded member.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/mtd/nand/raw/intel-nand-controller.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mtd/nand/raw/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c
index de4f85368988..e486db11ecc3 100644
--- a/drivers/mtd/nand/raw/intel-nand-controller.c
+++ b/drivers/mtd/nand/raw/intel-nand-controller.c
@@ -118,7 +118,6 @@ struct ebu_nand_controller {
struct dma_chan *dma_tx;
struct dma_chan *dma_rx;
struct completion dma_access_complete;
- unsigned long clk_rate;
struct clk *clk;
u32 nd_para0;
u8 cs_num;
@@ -636,7 +635,6 @@ static int ebu_nand_probe(struct platform_device *pdev)
dev_err(dev, "failed to enable clock: %d\n", ret);
return ret;
}
- ebu_host->clk_rate = clk_get_rate(ebu_host->clk);
ebu_host->dma_tx = dma_request_chan(dev, "tx");
if (IS_ERR(ebu_host->dma_tx)) {
--
2.37.0
On Sun, 03 Jul 2022 01:12:20 +0200, Martin Blumenstingl wrote:
> The driver which was added at the same time as the dt-bindings uses the
> compatible string "intel,lgm-ebunand". Use the same compatible string
> also in the dt-bindings and rename the bindings file accordingly.
>
> Fixes: 2f9cea8eae44f5 ("dt-bindings: mtd: Add Nand Flash Controller support for Intel LGM SoC")
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> .../mtd/{intel,lgm-nand.yaml => intel,lgm-ebunand.yaml} | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> rename Documentation/devicetree/bindings/mtd/{intel,lgm-nand.yaml => intel,lgm-ebunand.yaml} (92%)
>
Reviewed-by: Rob Herring <[email protected]>
On Sat, 2022-07-02 at 23:12:25 UTC, Martin Blumenstingl wrote:
> The nand_pa member from struct ebu_nand_cs is only written but never
> read. Remove this unused and unneeded member.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel
On Sat, 2022-07-02 at 23:12:20 UTC, Martin Blumenstingl wrote:
> The driver which was added at the same time as the dt-bindings uses the
> compatible string "intel,lgm-ebunand". Use the same compatible string
> also in the dt-bindings and rename the bindings file accordingly.
>
> Fixes: 2f9cea8eae44f5 ("dt-bindings: mtd: Add Nand Flash Controller support for Intel LGM SoC")
> Signed-off-by: Martin Blumenstingl <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel
On Sat, 2022-07-02 at 23:12:23 UTC, Martin Blumenstingl wrote:
> The "intel,nand-controller" compatible string is not part of the
> dt-bindings. Remove it from the driver as it's not supposed to be used
> without any documentation for it.
>
> Fixes: 0b1039f016e8a3 ("mtd: rawnand: Add NAND controller support on Intel LGM SoC")
> Signed-off-by: Martin Blumenstingl <[email protected]>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel
On Sat, 2022-07-02 at 23:12:24 UTC, Martin Blumenstingl wrote:
> NAND_DATA_IFACE_CHECK_ONLY is already defined in
> include/linux/mtd/rawnand.h which is also included by the driver. Drop
> the re-definition from the intel-nand-controller driver.
>
> Fixes: 0b1039f016e8a3 ("mtd: rawnand: Add NAND controller support on Intel LGM SoC")
> Signed-off-by: Martin Blumenstingl <[email protected]>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel
On Sat, 2022-07-02 at 23:12:22 UTC, Martin Blumenstingl wrote:
> The chip select has to be read from the flash node which is a child node
> of the NAND controller.
>
> Fixes: 0b1039f016e8a3 ("mtd: rawnand: Add NAND controller support on Intel LGM SoC")
> Signed-off-by: Martin Blumenstingl <[email protected]>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel
On Sat, 2022-07-02 at 23:12:26 UTC, Martin Blumenstingl wrote:
> The clk_rate member from struct ebu_nand is only written but never read.
> Remove this unused and unneeded member.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel
On Sat, 2022-07-02 at 23:12:21 UTC, Martin Blumenstingl wrote:
> The Intel LGM NAND IP only supports two chip selects: There's only two
> CS and ADDR_SEL register sets. Fix the maximum allowed chip select value
> according to the dt-bindings.
>
> Fixes: 2f9cea8eae44f5 ("dt-bindings: mtd: Add Nand Flash Controller support for Intel LGM SoC")
> Acked-by: Rob Herring <[email protected]>
> Signed-off-by: Martin Blumenstingl <[email protected]>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel
On Sat, 2022-07-02 at 23:12:27 UTC, Martin Blumenstingl wrote:
> Switch from open-coded platform_get_resource_byname() and
> devm_ioremap_resource() to devm_platform_ioremap_resource_byname() where
> possible to simplify the code.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel