2022-02-17 08:23:02

by Liang Yang

[permalink] [raw]
Subject: [PATCH RESEND v2 0/2] refine the NFC clock framework

Background firstly,
EMMC and NAND has the same clock control register named 'SD_EMMC_CLOCK' which is
defined in EMMC port internally. bit0~5 of 'SD_EMMC_CLOCK' is the divider and
bit6~7 is the mux for fix pll and xtal.
Previously a common MMC sub clock framework is implemented and shared by EMMC and
NAND, but that is coupling the EMMC and NAND, although EMMC and NAND is mutually
exclusive. see the link:
[https://lore.kernel.org/all/[email protected]/]
Now we plan to abandon common mmc sub clock framework and recovery the series.

Changes since v1 [2]
- use clk_parent_data instead of parent_names
- define a reg resource instead of sd_emmc_c_clkc

[1]
https://lore.kernel.org/r/[email protected]
https://lore.kernel.org/r/[email protected]
Liang Yang (2):
mtd: rawnand: meson: discard the common MMC sub clock framework
dt-bindings: nand: meson: refine Amlogic NAND controller driver

.../bindings/mtd/amlogic,meson-nand.txt | 60 ----------
.../bindings/mtd/amlogic,meson-nand.yaml | 70 ++++++++++++
drivers/mtd/nand/raw/meson_nand.c | 107 ++++++++++--------
3 files changed, 131 insertions(+), 106 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
create mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml

--
2.34.1


2022-02-17 09:53:46

by Liang Yang

[permalink] [raw]
Subject: [PATCH RESEND v2 2/2] dt-bindings: nand: meson: refine Amlogic NAND controller driver

convert txt to yaml and refine the meson NFC clock document.

Signed-off-by: Liang Yang <[email protected]>
---
.../bindings/mtd/amlogic,meson-nand.txt | 60 ----------------
.../bindings/mtd/amlogic,meson-nand.yaml | 70 +++++++++++++++++++
2 files changed, 70 insertions(+), 60 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
create mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml

diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
deleted file mode 100644
index 5794ab1147c1..000000000000
--- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
+++ /dev/null
@@ -1,60 +0,0 @@
-Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
-
-This file documents the properties in addition to those available in
-the MTD NAND bindings.
-
-Required properties:
-- compatible : contains one of:
- - "amlogic,meson-gxl-nfc"
- - "amlogic,meson-axg-nfc"
-- clocks :
- A list of phandle + clock-specifier pairs for the clocks listed
- in clock-names.
-
-- clock-names: Should contain the following:
- "core" - NFC module gate clock
- "device" - device clock from eMMC sub clock controller
- "rx" - rx clock phase
- "tx" - tx clock phase
-
-- amlogic,mmc-syscon : Required for NAND clocks, it's shared with SD/eMMC
- controller port C
-
-Optional children nodes:
-Children nodes represent the available nand chips.
-
-Other properties:
-see Documentation/devicetree/bindings/mtd/nand-controller.yaml for generic bindings.
-
-Example demonstrate on AXG SoC:
-
- sd_emmc_c_clkc: mmc@7000 {
- compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
- reg = <0x0 0x7000 0x0 0x800>;
- };
-
- nand-controller@7800 {
- compatible = "amlogic,meson-axg-nfc";
- reg = <0x0 0x7800 0x0 0x100>;
- #address-cells = <1>;
- #size-cells = <0>;
- interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
-
- clocks = <&clkc CLKID_SD_EMMC_C>,
- <&sd_emmc_c_clkc CLKID_MMC_DIV>,
- <&sd_emmc_c_clkc CLKID_MMC_PHASE_RX>,
- <&sd_emmc_c_clkc CLKID_MMC_PHASE_TX>;
- clock-names = "core", "device", "rx", "tx";
- amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
-
- pinctrl-names = "default";
- pinctrl-0 = <&nand_pins>;
-
- nand@0 {
- reg = <0>;
- #address-cells = <1>;
- #size-cells = <1>;
-
- nand-on-flash-bbt;
- };
- };
diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
new file mode 100644
index 000000000000..671f0a8fdc7c
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/amlogic,mmc-clkc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
+
+maintainers:
+ - [email protected]
+
+properties:
+ compatible:
+ enum:
+ - "amlogic,meson-gxl-nfc"
+ - "amlogic,meson-axg-nfc"
+
+ reg:
+ maxItems: 2
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: "core", "device"
+
+ "#clock-cells":
+ const: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+ - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ nand-controller@7800 {
+ compatible = "amlogic,meson-axg-nfc";
+ reg = <0x0 0x7800 0x0 0x100>,
+ <0x0 0x7000 0x0 0x1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
+
+ clocks = <&clkc CLKID_SD_EMMC_C>,
+ <&clkc CLKID_FCLK_DIV2>;
+ clock-names = "core", "device";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&nand_pins>;
+
+ nand@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ nand-on-flash-bbt;
+ };
+ };
+
+...
\ No newline at end of file
--
2.34.1

2022-02-17 10:15:39

by Liang Yang

[permalink] [raw]
Subject: [PATCH RESEND v2 1/2] mtd: rawnand: meson: discard the common MMC sub clock framework

EMMC and NAND have the same clock control register named 'SD_EMMC_CLOCK' which is
defined in EMMC port internally. bit0~5 of 'SD_EMMC_CLOCK' is the divider and
bit6~7 is the mux for fix pll and xtal.A common MMC and NAND sub-clock has been
implemented and can be used by the eMMC and NAND controller (which are mutually
exclusive anyway). Let's use this new clock.

Signed-off-by: Liang Yang <[email protected]>
---
drivers/mtd/nand/raw/meson_nand.c | 107 +++++++++++++++++-------------
1 file changed, 61 insertions(+), 46 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index ac3be92872d0..c5b892d38ea0 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -10,6 +10,7 @@
#include <linux/dma-mapping.h>
#include <linux/interrupt.h>
#include <linux/clk.h>
+#include <linux/clk-provider.h>
#include <linux/mtd/rawnand.h>
#include <linux/mtd/mtd.h>
#include <linux/mfd/syscon.h>
@@ -19,6 +20,7 @@
#include <linux/iopoll.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/of_address.h>
#include <linux/sched/task_stack.h>

#define NFC_REG_CMD 0x00
@@ -104,6 +106,9 @@

#define PER_INFO_BYTE 8

+#define CLK_DIV_SHIFT 0
+#define CLK_DIV_WIDTH 6
+
struct meson_nfc_nand_chip {
struct list_head node;
struct nand_chip nand;
@@ -151,15 +156,15 @@ struct meson_nfc {
struct nand_controller controller;
struct clk *core_clk;
struct clk *device_clk;
- struct clk *phase_tx;
- struct clk *phase_rx;
+ struct clk *nand_clk;
+ struct clk_divider nand_divider;

unsigned long clk_rate;
u32 bus_timing;

struct device *dev;
void __iomem *reg_base;
- struct regmap *reg_clk;
+ void __iomem *sd_emmc_clock;
struct completion completion;
struct list_head chips;
const struct meson_nfc_data *data;
@@ -988,6 +993,8 @@ static const struct mtd_ooblayout_ops meson_ooblayout_ops = {
static int meson_nfc_clk_init(struct meson_nfc *nfc)
{
int ret;
+ struct clk_init_data init = {0};
+ struct clk_parent_data nfc_divider_parent_data[1];

/* request core clock */
nfc->core_clk = devm_clk_get(nfc->dev, "core");
@@ -1002,21 +1009,26 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc)
return PTR_ERR(nfc->device_clk);
}

- nfc->phase_tx = devm_clk_get(nfc->dev, "tx");
- if (IS_ERR(nfc->phase_tx)) {
- dev_err(nfc->dev, "failed to get TX clk\n");
- return PTR_ERR(nfc->phase_tx);
- }
-
- nfc->phase_rx = devm_clk_get(nfc->dev, "rx");
- if (IS_ERR(nfc->phase_rx)) {
- dev_err(nfc->dev, "failed to get RX clk\n");
- return PTR_ERR(nfc->phase_rx);
- }
+ init.name = devm_kstrdup(nfc->dev, "nfc#div", GFP_KERNEL);
+ init.ops = &clk_divider_ops;
+ nfc_divider_parent_data[0].fw_name = __clk_get_name(nfc->device_clk);
+ init.parent_data = nfc_divider_parent_data;
+ init.num_parents = 1;
+ nfc->nand_divider.reg = nfc->sd_emmc_clock;
+ nfc->nand_divider.shift = CLK_DIV_SHIFT;
+ nfc->nand_divider.width = CLK_DIV_WIDTH;
+ nfc->nand_divider.hw.init = &init;
+ nfc->nand_divider.flags = CLK_DIVIDER_ONE_BASED |
+ CLK_DIVIDER_ROUND_CLOSEST |
+ CLK_DIVIDER_ALLOW_ZERO;
+
+ nfc->nand_clk = devm_clk_register(nfc->dev, &nfc->nand_divider.hw);
+ if (IS_ERR(nfc->nand_clk))
+ return PTR_ERR(nfc->nand_clk);

/* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
- regmap_update_bits(nfc->reg_clk,
- 0, CLK_SELECT_NAND, CLK_SELECT_NAND);
+ writel(CLK_SELECT_NAND | readl(nfc->sd_emmc_clock),
+ nfc->sd_emmc_clock);

ret = clk_prepare_enable(nfc->core_clk);
if (ret) {
@@ -1030,29 +1042,21 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc)
goto err_device_clk;
}

- ret = clk_prepare_enable(nfc->phase_tx);
- if (ret) {
- dev_err(nfc->dev, "failed to enable TX clock\n");
- goto err_phase_tx;
- }
-
- ret = clk_prepare_enable(nfc->phase_rx);
+ ret = clk_prepare_enable(nfc->nand_clk);
if (ret) {
- dev_err(nfc->dev, "failed to enable RX clock\n");
- goto err_phase_rx;
+ dev_err(nfc->dev, "pre enable NFC divider fail\n");
+ goto err_nand_clk;
}

ret = clk_set_rate(nfc->device_clk, 24000000);
if (ret)
- goto err_disable_rx;
+ goto err_disable_clk;

return 0;

-err_disable_rx:
- clk_disable_unprepare(nfc->phase_rx);
-err_phase_rx:
- clk_disable_unprepare(nfc->phase_tx);
-err_phase_tx:
+err_disable_clk:
+ clk_disable_unprepare(nfc->nand_clk);
+err_nand_clk:
clk_disable_unprepare(nfc->device_clk);
err_device_clk:
clk_disable_unprepare(nfc->core_clk);
@@ -1061,8 +1065,7 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc)

static void meson_nfc_disable_clk(struct meson_nfc *nfc)
{
- clk_disable_unprepare(nfc->phase_rx);
- clk_disable_unprepare(nfc->phase_tx);
+ clk_disable_unprepare(nfc->nand_clk);
clk_disable_unprepare(nfc->device_clk);
clk_disable_unprepare(nfc->core_clk);
}
@@ -1370,11 +1373,31 @@ static const struct of_device_id meson_nfc_id_table[] = {
};
MODULE_DEVICE_TABLE(of, meson_nfc_id_table);

+static int meson_nfc_reg_resource(struct device *dev, struct meson_nfc *nfc)
+{
+ struct resource res;
+ void __iomem *base[2];
+ struct device_node *node = dev->of_node;
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ if (of_address_to_resource(node, i, &res))
+ return -ENOENT;
+
+ base[i] = devm_ioremap_resource(dev, &res);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+ }
+ nfc->reg_base = base[0];
+ nfc->sd_emmc_clock = base[1];
+
+ return 0;
+}
+
static int meson_nfc_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct meson_nfc *nfc;
- struct resource *res;
int ret, irq;

nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
@@ -1388,20 +1411,12 @@ static int meson_nfc_probe(struct platform_device *pdev)
nand_controller_init(&nfc->controller);
INIT_LIST_HEAD(&nfc->chips);
init_completion(&nfc->completion);
-
nfc->dev = dev;

- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- nfc->reg_base = devm_ioremap_resource(dev, res);
- if (IS_ERR(nfc->reg_base))
- return PTR_ERR(nfc->reg_base);
-
- nfc->reg_clk =
- syscon_regmap_lookup_by_phandle(dev->of_node,
- "amlogic,mmc-syscon");
- if (IS_ERR(nfc->reg_clk)) {
- dev_err(dev, "Failed to lookup clock base\n");
- return PTR_ERR(nfc->reg_clk);
+ ret = meson_nfc_reg_resource(dev, nfc);
+ if (ret) {
+ dev_err(dev, "Failed to get reg resource\n");
+ return ret;
}

irq = platform_get_irq(pdev, 0);
--
2.34.1

2022-02-17 15:07:34

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH RESEND v2 1/2] mtd: rawnand: meson: discard the common MMC sub clock framework

Hi Liang,

I love your patch! Yet something to improve:

[auto build test ERROR on mtd/nand/next]
[also build test ERROR on mtd/mtd/next mtd/mtd/fixes robh/for-next v5.17-rc4 next-20220217]
[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]

url: https://github.com/0day-ci/linux/commits/Liang-Yang/refine-the-NFC-clock-framework/20220217-145053
base: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next
config: hexagon-randconfig-r013-20220217 (https://download.01.org/0day-ci/archive/20220217/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0bad7cb56526f2572c74449fcf97c1fcda42b41d)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/d3d7ef65145346ff8212125d56afff6290a5ab68
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Liang-Yang/refine-the-NFC-clock-framework/20220217-145053
git checkout d3d7ef65145346ff8212125d56afff6290a5ab68
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: __clk_get_name
>>> referenced by meson_nand.c
>>> mtd/nand/raw/meson_nand.o:(meson_nfc_probe) in archive drivers/built-in.a
>>> referenced by meson_nand.c
>>> mtd/nand/raw/meson_nand.o:(meson_nfc_probe) in archive drivers/built-in.a
--
>> ld.lld: error: undefined symbol: clk_divider_ops
>>> referenced by meson_nand.c
>>> mtd/nand/raw/meson_nand.o:(meson_nfc_probe) in archive drivers/built-in.a
>>> referenced by meson_nand.c
>>> mtd/nand/raw/meson_nand.o:(meson_nfc_probe) in archive drivers/built-in.a
--
>> ld.lld: error: undefined symbol: devm_clk_register
>>> referenced by meson_nand.c
>>> mtd/nand/raw/meson_nand.o:(meson_nfc_probe) in archive drivers/built-in.a
>>> referenced by meson_nand.c
>>> mtd/nand/raw/meson_nand.o:(meson_nfc_probe) in archive drivers/built-in.a

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

2022-02-28 12:22:50

by Jerome Brunet

[permalink] [raw]
Subject: Re: [PATCH RESEND v2 2/2] dt-bindings: nand: meson: refine Amlogic NAND controller driver


On Thu 17 Feb 2022 at 14:33, Liang Yang <[email protected]> wrote:

> convert txt to yaml and refine the meson NFC clock document.
>
> Signed-off-by: Liang Yang <[email protected]>
> ---
> .../bindings/mtd/amlogic,meson-nand.txt | 60 ----------------
> .../bindings/mtd/amlogic,meson-nand.yaml | 70 +++++++++++++++++++
> 2 files changed, 70 insertions(+), 60 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> create mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
>
> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> deleted file mode 100644
> index 5794ab1147c1..000000000000
> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> +++ /dev/null
> @@ -1,60 +0,0 @@
> -Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
> -
> -This file documents the properties in addition to those available in
> -the MTD NAND bindings.
> -
> -Required properties:
> -- compatible : contains one of:
> - - "amlogic,meson-gxl-nfc"
> - - "amlogic,meson-axg-nfc"
> -- clocks :
> - A list of phandle + clock-specifier pairs for the clocks listed
> - in clock-names.
> -
> -- clock-names: Should contain the following:
> - "core" - NFC module gate clock
> - "device" - device clock from eMMC sub clock controller
> - "rx" - rx clock phase
> - "tx" - tx clock phase
> -
> -- amlogic,mmc-syscon : Required for NAND clocks, it's shared with SD/eMMC
> - controller port C
> -
> -Optional children nodes:
> -Children nodes represent the available nand chips.
> -
> -Other properties:
> -see Documentation/devicetree/bindings/mtd/nand-controller.yaml for generic bindings.
> -
> -Example demonstrate on AXG SoC:
> -
> - sd_emmc_c_clkc: mmc@7000 {
> - compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
> - reg = <0x0 0x7000 0x0 0x800>;
> - };
> -
> - nand-controller@7800 {
> - compatible = "amlogic,meson-axg-nfc";
> - reg = <0x0 0x7800 0x0 0x100>;
> - #address-cells = <1>;
> - #size-cells = <0>;
> - interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
> -
> - clocks = <&clkc CLKID_SD_EMMC_C>,
> - <&sd_emmc_c_clkc CLKID_MMC_DIV>,
> - <&sd_emmc_c_clkc CLKID_MMC_PHASE_RX>,
> - <&sd_emmc_c_clkc CLKID_MMC_PHASE_TX>;
> - clock-names = "core", "device", "rx", "tx";
> - amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
> -
> - pinctrl-names = "default";
> - pinctrl-0 = <&nand_pins>;
> -
> - nand@0 {
> - reg = <0>;
> - #address-cells = <1>;
> - #size-cells = <1>;
> -
> - nand-on-flash-bbt;
> - };
> - };
> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
> new file mode 100644
> index 000000000000..671f0a8fdc7c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/amlogic,mmc-clkc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
> +
> +maintainers:
> + - [email protected]
> +
> +properties:
> + compatible:
> + enum:
> + - "amlogic,meson-gxl-nfc"
> + - "amlogic,meson-axg-nfc"
> +
> + reg:
> + maxItems: 2
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 2
> +
> + clock-names:
> + items:
> + - const: "core", "device"
> +
> + "#clock-cells":
> + const: 1
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - clock-names
> + - "#clock-cells"
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + nand-controller@7800 {
> + compatible = "amlogic,meson-axg-nfc";
> + reg = <0x0 0x7800 0x0 0x100>,
> + <0x0 0x7000 0x0 0x1>;

Please name the register ressources instead of relying on the order

Also, use the actual size of region, not the size you are using in the
driver. AFAIK, the size of the 2nd region is not 0x1

> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
> +
> + clocks = <&clkc CLKID_SD_EMMC_C>,
> + <&clkc CLKID_FCLK_DIV2>;
> + clock-names = "core", "device";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&nand_pins>;
> +
> + nand@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + nand-on-flash-bbt;
> + };
> + };
> +
> +...
> \ No newline at end of file

2022-02-28 13:59:29

by Jerome Brunet

[permalink] [raw]
Subject: Re: [PATCH RESEND v2 1/2] mtd: rawnand: meson: discard the common MMC sub clock framework


On Thu 17 Feb 2022 at 14:33, Liang Yang <[email protected]> wrote:

> EMMC and NAND have the same clock control register named 'SD_EMMC_CLOCK' which is
> defined in EMMC port internally. bit0~5 of 'SD_EMMC_CLOCK' is the divider and
> bit6~7 is the mux for fix pll and xtal.A common MMC and NAND sub-clock has been
> implemented and can be used by the eMMC and NAND controller (which are mutually
> exclusive anyway). Let's use this new clock.
>
> Signed-off-by: Liang Yang <[email protected]>
> ---
> drivers/mtd/nand/raw/meson_nand.c | 107 +++++++++++++++++-------------
> 1 file changed, 61 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> index ac3be92872d0..c5b892d38ea0 100644
> --- a/drivers/mtd/nand/raw/meson_nand.c
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -10,6 +10,7 @@
> #include <linux/dma-mapping.h>
> #include <linux/interrupt.h>
> #include <linux/clk.h>
> +#include <linux/clk-provider.h>
> #include <linux/mtd/rawnand.h>
> #include <linux/mtd/mtd.h>
> #include <linux/mfd/syscon.h>
> @@ -19,6 +20,7 @@
> #include <linux/iopoll.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/of_address.h>
> #include <linux/sched/task_stack.h>
>
> #define NFC_REG_CMD 0x00
> @@ -104,6 +106,9 @@
>
> #define PER_INFO_BYTE 8
>
> +#define CLK_DIV_SHIFT 0
> +#define CLK_DIV_WIDTH 6
> +
> struct meson_nfc_nand_chip {
> struct list_head node;
> struct nand_chip nand;
> @@ -151,15 +156,15 @@ struct meson_nfc {
> struct nand_controller controller;
> struct clk *core_clk;
> struct clk *device_clk;
> - struct clk *phase_tx;
> - struct clk *phase_rx;
> + struct clk *nand_clk;
> + struct clk_divider nand_divider;
>
> unsigned long clk_rate;
> u32 bus_timing;
>
> struct device *dev;
> void __iomem *reg_base;
> - struct regmap *reg_clk;
> + void __iomem *sd_emmc_clock;
> struct completion completion;
> struct list_head chips;
> const struct meson_nfc_data *data;
> @@ -988,6 +993,8 @@ static const struct mtd_ooblayout_ops meson_ooblayout_ops = {
> static int meson_nfc_clk_init(struct meson_nfc *nfc)
> {
> int ret;
> + struct clk_init_data init = {0};
> + struct clk_parent_data nfc_divider_parent_data[1];
>
> /* request core clock */
> nfc->core_clk = devm_clk_get(nfc->dev, "core");
> @@ -1002,21 +1009,26 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc)
> return PTR_ERR(nfc->device_clk);
> }
>
> - nfc->phase_tx = devm_clk_get(nfc->dev, "tx");
> - if (IS_ERR(nfc->phase_tx)) {
> - dev_err(nfc->dev, "failed to get TX clk\n");
> - return PTR_ERR(nfc->phase_tx);
> - }
> -
> - nfc->phase_rx = devm_clk_get(nfc->dev, "rx");
> - if (IS_ERR(nfc->phase_rx)) {
> - dev_err(nfc->dev, "failed to get RX clk\n");
> - return PTR_ERR(nfc->phase_rx);
> - }
> + init.name = devm_kstrdup(nfc->dev, "nfc#div", GFP_KERNEL);
> + init.ops = &clk_divider_ops;
> + nfc_divider_parent_data[0].fw_name = __clk_get_name(nfc->device_clk);

This is broken.
__clk_get_name() gives the actual global name of the clock
"fw_name" is the DT name which allows to find the index on the clock

From the DT doc, the fw_name is either 'core' or 'device'

I don't think any clock would have such a name so I don't think this can
actually work. Did you actually test this ?


> + init.parent_data = nfc_divider_parent_data;
> + init.num_parents = 1;
> + nfc->nand_divider.reg = nfc->sd_emmc_clock;
> + nfc->nand_divider.shift = CLK_DIV_SHIFT;
> + nfc->nand_divider.width = CLK_DIV_WIDTH;
> + nfc->nand_divider.hw.init = &init;
> + nfc->nand_divider.flags = CLK_DIVIDER_ONE_BASED |
> + CLK_DIVIDER_ROUND_CLOSEST |
> + CLK_DIVIDER_ALLOW_ZERO;
> +
> + nfc->nand_clk = devm_clk_register(nfc->dev, &nfc->nand_divider.hw);
> + if (IS_ERR(nfc->nand_clk))
> + return PTR_ERR(nfc->nand_clk);
>
> /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
> - regmap_update_bits(nfc->reg_clk,
> - 0, CLK_SELECT_NAND, CLK_SELECT_NAND);
> + writel(CLK_SELECT_NAND | readl(nfc->sd_emmc_clock),
> + nfc->sd_emmc_clock);
>
> ret = clk_prepare_enable(nfc->core_clk);
> if (ret) {
> @@ -1030,29 +1042,21 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc)
> goto err_device_clk;
> }
>
> - ret = clk_prepare_enable(nfc->phase_tx);
> - if (ret) {
> - dev_err(nfc->dev, "failed to enable TX clock\n");
> - goto err_phase_tx;
> - }
> -
> - ret = clk_prepare_enable(nfc->phase_rx);
> + ret = clk_prepare_enable(nfc->nand_clk);
> if (ret) {
> - dev_err(nfc->dev, "failed to enable RX clock\n");
> - goto err_phase_rx;
> + dev_err(nfc->dev, "pre enable NFC divider fail\n");
> + goto err_nand_clk;
> }
>
> ret = clk_set_rate(nfc->device_clk, 24000000);
> if (ret)
> - goto err_disable_rx;
> + goto err_disable_clk;
>
> return 0;
>
> -err_disable_rx:
> - clk_disable_unprepare(nfc->phase_rx);
> -err_phase_rx:
> - clk_disable_unprepare(nfc->phase_tx);
> -err_phase_tx:
> +err_disable_clk:
> + clk_disable_unprepare(nfc->nand_clk);
> +err_nand_clk:
> clk_disable_unprepare(nfc->device_clk);
> err_device_clk:
> clk_disable_unprepare(nfc->core_clk);
> @@ -1061,8 +1065,7 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc)
>
> static void meson_nfc_disable_clk(struct meson_nfc *nfc)
> {
> - clk_disable_unprepare(nfc->phase_rx);
> - clk_disable_unprepare(nfc->phase_tx);
> + clk_disable_unprepare(nfc->nand_clk);
> clk_disable_unprepare(nfc->device_clk);
> clk_disable_unprepare(nfc->core_clk);
> }
> @@ -1370,11 +1373,31 @@ static const struct of_device_id meson_nfc_id_table[] = {
> };
> MODULE_DEVICE_TABLE(of, meson_nfc_id_table);
>
> +static int meson_nfc_reg_resource(struct device *dev, struct meson_nfc *nfc)
> +{
> + struct resource res;
> + void __iomem *base[2];
> + struct device_node *node = dev->of_node;
> + int i;
> +
> + for (i = 0; i < 2; i++) {
> + if (of_address_to_resource(node, i, &res))
> + return -ENOENT;
> +
> + base[i] = devm_ioremap_resource(dev, &res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> + }
> + nfc->reg_base = base[0];
> + nfc->sd_emmc_clock = base[1];
> +

There is no reason to make a separate function for this.

Please name your ressource and claim them properly instead of interating
on them.


> + return 0;
> +}
> +
> static int meson_nfc_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct meson_nfc *nfc;
> - struct resource *res;
> int ret, irq;
>
> nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
> @@ -1388,20 +1411,12 @@ static int meson_nfc_probe(struct platform_device *pdev)
> nand_controller_init(&nfc->controller);
> INIT_LIST_HEAD(&nfc->chips);
> init_completion(&nfc->completion);
> -
> nfc->dev = dev;
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - nfc->reg_base = devm_ioremap_resource(dev, res);
> - if (IS_ERR(nfc->reg_base))
> - return PTR_ERR(nfc->reg_base);
> -
> - nfc->reg_clk =
> - syscon_regmap_lookup_by_phandle(dev->of_node,
> - "amlogic,mmc-syscon");
> - if (IS_ERR(nfc->reg_clk)) {
> - dev_err(dev, "Failed to lookup clock base\n");
> - return PTR_ERR(nfc->reg_clk);
> + ret = meson_nfc_reg_resource(dev, nfc);
> + if (ret) {
> + dev_err(dev, "Failed to get reg resource\n");
> + return ret;
> }
>
> irq = platform_get_irq(pdev, 0);

2022-03-02 20:41:22

by Jerome Brunet

[permalink] [raw]
Subject: Re: [PATCH RESEND v2 2/2] dt-bindings: nand: meson: refine Amlogic NAND controller driver


On Wed 02 Mar 2022 at 13:19, Liang.Yang <[email protected]> wrote:

> Hi Jerome,
>
> On 2022/2/28 19:36, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On Thu 17 Feb 2022 at 14:33, Liang Yang <[email protected]> wrote:
>>
>>> convert txt to yaml and refine the meson NFC clock document.
>>>
>>> Signed-off-by: Liang Yang <[email protected]>
>>> ---
>>> .../bindings/mtd/amlogic,meson-nand.txt | 60 ----------------
>>> .../bindings/mtd/amlogic,meson-nand.yaml | 70 +++++++++++++++++++
>>> 2 files changed, 70 insertions(+), 60 deletions(-)
>>> delete mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>>> create mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>>> deleted file mode 100644
>>> index 5794ab1147c1..000000000000
>>> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>>> +++ /dev/null
>>> @@ -1,60 +0,0 @@
>>> -Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
>>> -
>>> -This file documents the properties in addition to those available in
>>> -the MTD NAND bindings.
>>> -
>>> -Required properties:
>>> -- compatible : contains one of:
>>> - - "amlogic,meson-gxl-nfc"
>>> - - "amlogic,meson-axg-nfc"
>>> -- clocks :
>>> - A list of phandle + clock-specifier pairs for the clocks listed
>>> - in clock-names.
>>> -
>>> -- clock-names: Should contain the following:
>>> - "core" - NFC module gate clock
>>> - "device" - device clock from eMMC sub clock controller
>>> - "rx" - rx clock phase
>>> - "tx" - tx clock phase
>>> -
>>> -- amlogic,mmc-syscon : Required for NAND clocks, it's shared with SD/eMMC
>>> - controller port C
>>> -
>>> -Optional children nodes:
>>> -Children nodes represent the available nand chips.
>>> -
>>> -Other properties:
>>> -see Documentation/devicetree/bindings/mtd/nand-controller.yaml for generic bindings.
>>> -
>>> -Example demonstrate on AXG SoC:
>>> -
>>> - sd_emmc_c_clkc: mmc@7000 {
>>> - compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
>>> - reg = <0x0 0x7000 0x0 0x800>;
>>> - };
>>> -
>>> - nand-controller@7800 {
>>> - compatible = "amlogic,meson-axg-nfc";
>>> - reg = <0x0 0x7800 0x0 0x100>;
>>> - #address-cells = <1>;
>>> - #size-cells = <0>;
>>> - interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
>>> -
>>> - clocks = <&clkc CLKID_SD_EMMC_C>,
>>> - <&sd_emmc_c_clkc CLKID_MMC_DIV>,
>>> - <&sd_emmc_c_clkc CLKID_MMC_PHASE_RX>,
>>> - <&sd_emmc_c_clkc CLKID_MMC_PHASE_TX>;
>>> - clock-names = "core", "device", "rx", "tx";
>>> - amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
>>> -
>>> - pinctrl-names = "default";
>>> - pinctrl-0 = <&nand_pins>;
>>> -
>>> - nand@0 {
>>> - reg = <0>;
>>> - #address-cells = <1>;
>>> - #size-cells = <1>;
>>> -
>>> - nand-on-flash-bbt;
>>> - };
>>> - };
>>> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
>>> new file mode 100644
>>> index 000000000000..671f0a8fdc7c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
>>> @@ -0,0 +1,70 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/clock/amlogic,mmc-clkc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
>>> +
>>> +maintainers:
>>> + - [email protected]
>>> +
>>> +properties:
>>> + compatible:
>>> + enum:
>>> + - "amlogic,meson-gxl-nfc"
>>> + - "amlogic,meson-axg-nfc"
>>> +
>>> + reg:
>>> + maxItems: 2
>>> +
>>> + interrupts:
>>> + maxItems: 1
>>> +
>>> + clocks:
>>> + maxItems: 2
>>> +
>>> + clock-names:
>>> + items:
>>> + - const: "core", "device"
>>> +
>>> + "#clock-cells":
>>> + const: 1
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - interrupts
>>> + - clocks
>>> + - clock-names
>>> + - "#clock-cells"
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + nand-controller@7800 {
>>> + compatible = "amlogic,meson-axg-nfc";
>>> + reg = <0x0 0x7800 0x0 0x100>,
>>> + <0x0 0x7000 0x0 0x1>;
>> Please name the register ressources instead of relying on the order
> ok
>> Also, use the actual size of region, not the size you are using in the
>> driver. AFAIK, the size of the 2nd region is not 0x1
> yes, it should be 0x4 here, i will fix it.
> we only need to use a 32bits SDEMMC_CLOCK register.
> thanks.

No, it is not the point.
What register you need in the driver is not relevant.
What is relevant is the actual size of the memory region - so it not 0x1
or 0x4 ... more likely 0x800 from what I see in the AXG dts.

>>
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
>>> +
>>> + clocks = <&clkc CLKID_SD_EMMC_C>,
>>> + <&clkc CLKID_FCLK_DIV2>;
>>> + clock-names = "core", "device";
>>> +
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&nand_pins>;
>>> +
>>> + nand@0 {
>>> + reg = <0>;
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>> +
>>> + nand-on-flash-bbt;
>>> + };
>>> + };
>>> +
>>> +...
>>> \ No newline at end of file
>> .

2022-03-11 22:11:17

by Liang Yang

[permalink] [raw]
Subject: Re: [PATCH RESEND v2 1/2] mtd: rawnand: meson: discard the common MMC sub clock framework

Hello Jerome,

On 2022/2/28 19:28, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
>
> On Thu 17 Feb 2022 at 14:33, Liang Yang <[email protected]> wrote:
>
>> EMMC and NAND have the same clock control register named 'SD_EMMC_CLOCK' which is
>> defined in EMMC port internally. bit0~5 of 'SD_EMMC_CLOCK' is the divider and
>> bit6~7 is the mux for fix pll and xtal.A common MMC and NAND sub-clock has been
>> implemented and can be used by the eMMC and NAND controller (which are mutually
>> exclusive anyway). Let's use this new clock.
>>
>> Signed-off-by: Liang Yang <[email protected]>
>> ---
>> drivers/mtd/nand/raw/meson_nand.c | 107 +++++++++++++++++-------------
>> 1 file changed, 61 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>> index ac3be92872d0..c5b892d38ea0 100644
>> --- a/drivers/mtd/nand/raw/meson_nand.c
>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>> @@ -10,6 +10,7 @@
>> #include <linux/dma-mapping.h>
>> #include <linux/interrupt.h>
>> #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> #include <linux/mtd/rawnand.h>
>> #include <linux/mtd/mtd.h>
>> #include <linux/mfd/syscon.h>
>> @@ -19,6 +20,7 @@
>> #include <linux/iopoll.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> #include <linux/sched/task_stack.h>
>>
>> #define NFC_REG_CMD 0x00
>> @@ -104,6 +106,9 @@
>>
>> #define PER_INFO_BYTE 8
>>
>> +#define CLK_DIV_SHIFT 0
>> +#define CLK_DIV_WIDTH 6
>> +
>> struct meson_nfc_nand_chip {
>> struct list_head node;
>> struct nand_chip nand;
>> @@ -151,15 +156,15 @@ struct meson_nfc {
>> struct nand_controller controller;
>> struct clk *core_clk;
>> struct clk *device_clk;
>> - struct clk *phase_tx;
>> - struct clk *phase_rx;
>> + struct clk *nand_clk;
>> + struct clk_divider nand_divider;
>>
>> unsigned long clk_rate;
>> u32 bus_timing;
>>
>> struct device *dev;
>> void __iomem *reg_base;
>> - struct regmap *reg_clk;
>> + void __iomem *sd_emmc_clock;
>> struct completion completion;
>> struct list_head chips;
>> const struct meson_nfc_data *data;
>> @@ -988,6 +993,8 @@ static const struct mtd_ooblayout_ops meson_ooblayout_ops = {
>> static int meson_nfc_clk_init(struct meson_nfc *nfc)
>> {
>> int ret;
>> + struct clk_init_data init = {0};
>> + struct clk_parent_data nfc_divider_parent_data[1];
>>
>> /* request core clock */
>> nfc->core_clk = devm_clk_get(nfc->dev, "core");
>> @@ -1002,21 +1009,26 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc)
>> return PTR_ERR(nfc->device_clk);
>> }
>>
>> - nfc->phase_tx = devm_clk_get(nfc->dev, "tx");
>> - if (IS_ERR(nfc->phase_tx)) {
>> - dev_err(nfc->dev, "failed to get TX clk\n");
>> - return PTR_ERR(nfc->phase_tx);
>> - }
>> -
>> - nfc->phase_rx = devm_clk_get(nfc->dev, "rx");
>> - if (IS_ERR(nfc->phase_rx)) {
>> - dev_err(nfc->dev, "failed to get RX clk\n");
>> - return PTR_ERR(nfc->phase_rx);
>> - }
>> + init.name = devm_kstrdup(nfc->dev, "nfc#div", GFP_KERNEL);
>> + init.ops = &clk_divider_ops;
>> + nfc_divider_parent_data[0].fw_name = __clk_get_name(nfc->device_clk);
>
> This is broken.
> __clk_get_name() gives the actual global name of the clock
> "fw_name" is the DT name which allows to find the index on the clock
>
>>From the DT doc, the fw_name is either 'core' or 'device'
>
> I don't think any clock would have such a name so I don't think this can
> actually work. Did you actually test this ?
> Indeed, it can't work. i have tried it.
so let us get solved by nfc_divider_parent_data[0].fw_name = "device" or
previous parent_names? thanks.

>
>> + init.parent_data = nfc_divider_parent_data;
>> + init.num_parents = 1;
>> + nfc->nand_divider.reg = nfc->sd_emmc_clock;
>> + nfc->nand_divider.shift = CLK_DIV_SHIFT;
>> + nfc->nand_divider.width = CLK_DIV_WIDTH;
>> + nfc->nand_divider.hw.init = &init;
>> + nfc->nand_divider.flags = CLK_DIVIDER_ONE_BASED |
>> + CLK_DIVIDER_ROUND_CLOSEST |
>> + CLK_DIVIDER_ALLOW_ZERO;
>> +
>> + nfc->nand_clk = devm_clk_register(nfc->dev, &nfc->nand_divider.hw);
>> + if (IS_ERR(nfc->nand_clk))
>> + return PTR_ERR(nfc->nand_clk);
>>
>> /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
>> - regmap_update_bits(nfc->reg_clk,
>> - 0, CLK_SELECT_NAND, CLK_SELECT_NAND);
>> + writel(CLK_SELECT_NAND | readl(nfc->sd_emmc_clock),
>> + nfc->sd_emmc_clock);
>>
>> ret = clk_prepare_enable(nfc->core_clk);
>> if (ret) {
>> @@ -1030,29 +1042,21 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc)
>> goto err_device_clk;
>> }
>>
>> - ret = clk_prepare_enable(nfc->phase_tx);
>> - if (ret) {
>> - dev_err(nfc->dev, "failed to enable TX clock\n");
>> - goto err_phase_tx;
>> - }
>> -
>> - ret = clk_prepare_enable(nfc->phase_rx);
>> + ret = clk_prepare_enable(nfc->nand_clk);
>> if (ret) {
>> - dev_err(nfc->dev, "failed to enable RX clock\n");
>> - goto err_phase_rx;
>> + dev_err(nfc->dev, "pre enable NFC divider fail\n");
>> + goto err_nand_clk;
>> }
>>
>> ret = clk_set_rate(nfc->device_clk, 24000000);
>> if (ret)
>> - goto err_disable_rx;
>> + goto err_disable_clk;
>>
>> return 0;
>>
>> -err_disable_rx:
>> - clk_disable_unprepare(nfc->phase_rx);
>> -err_phase_rx:
>> - clk_disable_unprepare(nfc->phase_tx);
>> -err_phase_tx:
>> +err_disable_clk:
>> + clk_disable_unprepare(nfc->nand_clk);
>> +err_nand_clk:
>> clk_disable_unprepare(nfc->device_clk);
>> err_device_clk:
>> clk_disable_unprepare(nfc->core_clk);
>> @@ -1061,8 +1065,7 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc)
>>
>> static void meson_nfc_disable_clk(struct meson_nfc *nfc)
>> {
>> - clk_disable_unprepare(nfc->phase_rx);
>> - clk_disable_unprepare(nfc->phase_tx);
>> + clk_disable_unprepare(nfc->nand_clk);
>> clk_disable_unprepare(nfc->device_clk);
>> clk_disable_unprepare(nfc->core_clk);
>> }
>> @@ -1370,11 +1373,31 @@ static const struct of_device_id meson_nfc_id_table[] = {
>> };
>> MODULE_DEVICE_TABLE(of, meson_nfc_id_table);
>>
>> +static int meson_nfc_reg_resource(struct device *dev, struct meson_nfc *nfc)
>> +{
>> + struct resource res;
>> + void __iomem *base[2];
>> + struct device_node *node = dev->of_node;
>> + int i;
>> +
>> + for (i = 0; i < 2; i++) {
>> + if (of_address_to_resource(node, i, &res))
>> + return -ENOENT;
>> +
>> + base[i] = devm_ioremap_resource(dev, &res);
>> + if (IS_ERR(base))
>> + return PTR_ERR(base);
>> + }
>> + nfc->reg_base = base[0];
>> + nfc->sd_emmc_clock = base[1];
>> +
>
> There is no reason to make a separate function for this.
>
> Please name your ressource and claim them properly instead of interating
> on them.
>
>
>> + return 0;
>> +}
>> +
>> static int meson_nfc_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> struct meson_nfc *nfc;
>> - struct resource *res;
>> int ret, irq;
>>
>> nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
>> @@ -1388,20 +1411,12 @@ static int meson_nfc_probe(struct platform_device *pdev)
>> nand_controller_init(&nfc->controller);
>> INIT_LIST_HEAD(&nfc->chips);
>> init_completion(&nfc->completion);
>> -
>> nfc->dev = dev;
>>
>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> - nfc->reg_base = devm_ioremap_resource(dev, res);
>> - if (IS_ERR(nfc->reg_base))
>> - return PTR_ERR(nfc->reg_base);
>> -
>> - nfc->reg_clk =
>> - syscon_regmap_lookup_by_phandle(dev->of_node,
>> - "amlogic,mmc-syscon");
>> - if (IS_ERR(nfc->reg_clk)) {
>> - dev_err(dev, "Failed to lookup clock base\n");
>> - return PTR_ERR(nfc->reg_clk);
>> + ret = meson_nfc_reg_resource(dev, nfc);
>> + if (ret) {
>> + dev_err(dev, "Failed to get reg resource\n");
>> + return ret;
>> }
>>
>> irq = platform_get_irq(pdev, 0);
>
> .