| Message ID | 20220402074921.13316-2-liang.yang@amlogic.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | refine the NFC clock framework | expand |
Hi Liang, liang.yang@amlogic.com wrote on Sat, 2 Apr 2022 15:49:19 +0800: > 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 <liang.yang@amlogic.com> > --- > drivers/mtd/nand/raw/meson_nand.c | 89 +++++++++++++++---------------- > 1 file changed, 42 insertions(+), 47 deletions(-) > > diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > index ac3be92872d0..1b1a9407fb2f 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; > @@ -235,7 +240,7 @@ static void meson_nfc_select_chip(struct nand_chip *nand, int chip) > nfc->timing.tbers_max = meson_chip->tbers_max; > > if (nfc->clk_rate != meson_chip->clk_rate) { > - ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate); > + ret = clk_set_rate(nfc->nand_clk, meson_chip->clk_rate); > if (ret) { > dev_err(nfc->dev, "failed to set clock rate\n"); > return; > @@ -406,7 +411,6 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) > cmd = NFC_CMD_RB | NFC_CMD_RB_INT > | nfc->param.chip_select | nfc->timing.tbers_max; > writel(cmd, nfc->reg_base + NFC_REG_CMD); > - Please avoid these spacing changes in the middle of a commit. > ret = wait_for_completion_timeout(&nfc->completion, > msecs_to_jiffies(timeout_ms)); > if (ret == 0) > @@ -985,9 +989,11 @@ static const struct mtd_ooblayout_ops meson_ooblayout_ops = { > .free = meson_ooblayout_free, > }; > > +struct clk_parent_data nfc_divider_parent_data[1]; > static int meson_nfc_clk_init(struct meson_nfc *nfc) > { > int ret; > + struct clk_init_data init = {0}; > > /* request core clock */ > nfc->core_clk = devm_clk_get(nfc->dev, "core"); > @@ -1002,21 +1008,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 = "device"; > + 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 +1041,21 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc) > goto err_device_clk; > } > > - ret = clk_prepare_enable(nfc->phase_tx); > + ret = clk_prepare_enable(nfc->nand_clk); > if (ret) { > - dev_err(nfc->dev, "failed to enable TX clock\n"); > - goto err_phase_tx; > + dev_err(nfc->dev, "pre enable NFC divider fail\n"); > + goto err_nand_clk; > } > > - ret = clk_prepare_enable(nfc->phase_rx); > - if (ret) { > - dev_err(nfc->dev, "failed to enable RX clock\n"); > - goto err_phase_rx; > - } > - > - ret = clk_set_rate(nfc->device_clk, 24000000); > + ret = clk_set_rate(nfc->nand_clk, 24000000); Is this rename really useful? > 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 +1064,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); > } > @@ -1374,7 +1376,6 @@ 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,21 +1389,15 @@ static int meson_nfc_probe(struct platform_device *pdev) > nand_controller_init(&nfc->controller); > INIT_LIST_HEAD(&nfc->chips); > init_completion(&nfc->completion); > - Please don't modify spacing in this commit. > nfc->dev = dev; > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - nfc->reg_base = devm_ioremap_resource(dev, res); > + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); This change seems unrelated. > 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); > - } > + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, "emmc"); > + if (IS_ERR(nfc->sd_emmc_clock)) > + return PTR_ERR(nfc->sd_emmc_clock); While I agree this is much better than the previous solution, we cannot break DT compatibility, so you need to try getting the emmc clock, but if it fails you should fallback to the regmap lookup. > > irq = platform_get_irq(pdev, 0); > if (irq < 0) Thanks, Miquèl
Hi, On 02/04/2022 09:49, Liang Yang 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 <liang.yang@amlogic.com> > --- > drivers/mtd/nand/raw/meson_nand.c | 89 +++++++++++++++---------------- > 1 file changed, 42 insertions(+), 47 deletions(-) > > diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > index ac3be92872d0..1b1a9407fb2f 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; The name could still be reg_clk, even if not a regmap anymore. > struct completion completion; > struct list_head chips; > const struct meson_nfc_data *data; > @@ -235,7 +240,7 @@ static void meson_nfc_select_chip(struct nand_chip *nand, int chip) > nfc->timing.tbers_max = meson_chip->tbers_max; > > if (nfc->clk_rate != meson_chip->clk_rate) { > - ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate); > + ret = clk_set_rate(nfc->nand_clk, meson_chip->clk_rate); > if (ret) { > dev_err(nfc->dev, "failed to set clock rate\n"); > return; > @@ -406,7 +411,6 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) > cmd = NFC_CMD_RB | NFC_CMD_RB_INT > | nfc->param.chip_select | nfc->timing.tbers_max; > writel(cmd, nfc->reg_base + NFC_REG_CMD); > - > ret = wait_for_completion_timeout(&nfc->completion, > msecs_to_jiffies(timeout_ms)); > if (ret == 0) > @@ -985,9 +989,11 @@ static const struct mtd_ooblayout_ops meson_ooblayout_ops = { > .free = meson_ooblayout_free, > }; > > +struct clk_parent_data nfc_divider_parent_data[1]; This should be in the meson_nfc_clk_init() function, not global > static int meson_nfc_clk_init(struct meson_nfc *nfc) > { > int ret; > + struct clk_init_data init = {0}; > > /* request core clock */ > nfc->core_clk = devm_clk_get(nfc->dev, "core"); > @@ -1002,21 +1008,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); name = devm_kasprintf(nfc->dev, GFP_KERNEL, "%s#div", dev_name(dev)); would be better. > + init.ops = &clk_divider_ops; > + nfc_divider_parent_data[0].fw_name = "device"; > + 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 +1041,21 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc) > goto err_device_clk; > } > > - ret = clk_prepare_enable(nfc->phase_tx); > + ret = clk_prepare_enable(nfc->nand_clk); > if (ret) { > - dev_err(nfc->dev, "failed to enable TX clock\n"); > - goto err_phase_tx; > + dev_err(nfc->dev, "pre enable NFC divider fail\n"); > + goto err_nand_clk; > } > > - ret = clk_prepare_enable(nfc->phase_rx); > - if (ret) { > - dev_err(nfc->dev, "failed to enable RX clock\n"); > - goto err_phase_rx; > - } > - > - ret = clk_set_rate(nfc->device_clk, 24000000); > + ret = clk_set_rate(nfc->nand_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 +1064,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); > } > @@ -1374,7 +1376,6 @@ 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,21 +1389,15 @@ 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); > + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); > 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); > - } > + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, "emmc"); > + if (IS_ERR(nfc->sd_emmc_clock)) > + return PTR_ERR(nfc->sd_emmc_clock); > > irq = platform_get_irq(pdev, 0); > if (irq < 0) Thanks, Neil
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.18-rc1 next-20220404]
[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/intel-lab-lkp/linux/commits/Liang-Yang/refine-the-NFC-clock-framework/20220402-155036
base: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next
config: s390-randconfig-r002-20220403 (https://download.01.org/0day-ci/archive/20220404/202204042238.eZq8bjau-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 11.2.0
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/intel-lab-lkp/linux/commit/c34b64ab8005a978739f157a07ed342d247fecac
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Liang-Yang/refine-the-NFC-clock-framework/20220402-155036
git checkout c34b64ab8005a978739f157a07ed342d247fecac
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=s390 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
s390-linux-ld: drivers/mtd/nand/raw/meson_nand.o: in function `meson_nfc_probe':
meson_nand.c:(.text+0xd9e): undefined reference to `clk_divider_ops'
>> s390-linux-ld: meson_nand.c:(.text+0xe34): undefined reference to `devm_clk_register'
s390-linux-ld: net/core/sock.o: in function `sk_destruct':
sock.c:(.text+0x373e): undefined reference to `__sk_defer_free_flush'
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.18-rc1 next-20220405]
[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/intel-lab-lkp/linux/commits/Liang-Yang/refine-the-NFC-clock-framework/20220402-155036
base: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next
config: parisc-randconfig-r001-20220405 (https://download.01.org/0day-ci/archive/20220405/202204051917.AuOOMCQd-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 11.2.0
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/intel-lab-lkp/linux/commit/c34b64ab8005a978739f157a07ed342d247fecac
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Liang-Yang/refine-the-NFC-clock-framework/20220402-155036
git checkout c34b64ab8005a978739f157a07ed342d247fecac
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=parisc SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "devm_clk_register" [drivers/mtd/nand/raw/meson_nand.ko] undefined!
>> ERROR: modpost: "clk_divider_ops" [drivers/mtd/nand/raw/meson_nand.ko] undefined!
Hi Miquel, On 2022/4/4 16:30, Miquel Raynal wrote: > [ EXTERNAL EMAIL ] > > Hi Liang, > > liang.yang@amlogic.com wrote on Sat, 2 Apr 2022 15:49:19 +0800: > >> 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 <liang.yang@amlogic.com> >> --- >> drivers/mtd/nand/raw/meson_nand.c | 89 +++++++++++++++---------------- >> 1 file changed, 42 insertions(+), 47 deletions(-) >> >> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c >> index ac3be92872d0..1b1a9407fb2f 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; >> @@ -235,7 +240,7 @@ static void meson_nfc_select_chip(struct nand_chip *nand, int chip) >> nfc->timing.tbers_max = meson_chip->tbers_max; >> >> if (nfc->clk_rate != meson_chip->clk_rate) { >> - ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate); >> + ret = clk_set_rate(nfc->nand_clk, meson_chip->clk_rate); >> if (ret) { >> dev_err(nfc->dev, "failed to set clock rate\n"); >> return; >> @@ -406,7 +411,6 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) >> cmd = NFC_CMD_RB | NFC_CMD_RB_INT >> | nfc->param.chip_select | nfc->timing.tbers_max; >> writel(cmd, nfc->reg_base + NFC_REG_CMD); >> - > > Please avoid these spacing changes in the middle of a commit. ok, i will fix it. > >> ret = wait_for_completion_timeout(&nfc->completion, >> msecs_to_jiffies(timeout_ms)); >> if (ret == 0) >> @@ -985,9 +989,11 @@ static const struct mtd_ooblayout_ops meson_ooblayout_ops = { >> .free = meson_ooblayout_free, >> }; >> >> +struct clk_parent_data nfc_divider_parent_data[1]; >> static int meson_nfc_clk_init(struct meson_nfc *nfc) >> { >> int ret; >> + struct clk_init_data init = {0}; >> >> /* request core clock */ >> nfc->core_clk = devm_clk_get(nfc->dev, "core"); >> @@ -1002,21 +1008,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 = "device"; >> + 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 +1041,21 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc) >> goto err_device_clk; >> } >> >> - ret = clk_prepare_enable(nfc->phase_tx); >> + ret = clk_prepare_enable(nfc->nand_clk); >> if (ret) { >> - dev_err(nfc->dev, "failed to enable TX clock\n"); >> - goto err_phase_tx; >> + dev_err(nfc->dev, "pre enable NFC divider fail\n"); >> + goto err_nand_clk; >> } >> >> - ret = clk_prepare_enable(nfc->phase_rx); >> - if (ret) { >> - dev_err(nfc->dev, "failed to enable RX clock\n"); >> - goto err_phase_rx; >> - } >> - >> - ret = clk_set_rate(nfc->device_clk, 24000000); >> + ret = clk_set_rate(nfc->nand_clk, 24000000); > > Is this rename really useful? yes, it works. > >> 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 +1064,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); >> } >> @@ -1374,7 +1376,6 @@ 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,21 +1389,15 @@ static int meson_nfc_probe(struct platform_device *pdev) >> nand_controller_init(&nfc->controller); >> INIT_LIST_HEAD(&nfc->chips); >> init_completion(&nfc->completion); >> - > > Please don't modify spacing in this commit. >ok >> nfc->dev = dev; >> >> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> - nfc->reg_base = devm_ioremap_resource(dev, res); >> + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); > > This change seems unrelated. To be consistent with the following devm_platform_ioremap_resource_byname(pdev, "emmc"). do you mean that we don't need it?> >> 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); >> - } >> + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, "emmc"); >> + if (IS_ERR(nfc->sd_emmc_clock)) >> + return PTR_ERR(nfc->sd_emmc_clock); > > While I agree this is much better than the previous solution, we cannot > break DT compatibility, so you need to try getting the emmc clock, but > if it fails you should fallback to the regmap lookup. ok, i will fix it next version. thanks. > >> >> irq = platform_get_irq(pdev, 0); >> if (irq < 0) > > > Thanks, > Miquèl > > .
Hi Neil, On 2022/4/4 20:40, Neil Armstrong wrote: > [ EXTERNAL EMAIL ] > > Hi, > > On 02/04/2022 09:49, Liang Yang 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 <liang.yang@amlogic.com> >> --- >> drivers/mtd/nand/raw/meson_nand.c | 89 +++++++++++++++---------------- >> 1 file changed, 42 insertions(+), 47 deletions(-) >> >> diff --git a/drivers/mtd/nand/raw/meson_nand.c >> b/drivers/mtd/nand/raw/meson_nand.c >> index ac3be92872d0..1b1a9407fb2f 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; > > The name could still be reg_clk, even if not a regmap anymore. ok, i will do it. > >> struct completion completion; >> struct list_head chips; >> const struct meson_nfc_data *data; >> @@ -235,7 +240,7 @@ static void meson_nfc_select_chip(struct nand_chip >> *nand, int chip) >> nfc->timing.tbers_max = meson_chip->tbers_max; >> if (nfc->clk_rate != meson_chip->clk_rate) { >> - ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate); >> + ret = clk_set_rate(nfc->nand_clk, meson_chip->clk_rate); >> if (ret) { >> dev_err(nfc->dev, "failed to set clock rate\n"); >> return; >> @@ -406,7 +411,6 @@ static int meson_nfc_queue_rb(struct meson_nfc >> *nfc, int timeout_ms) >> cmd = NFC_CMD_RB | NFC_CMD_RB_INT >> | nfc->param.chip_select | nfc->timing.tbers_max; >> writel(cmd, nfc->reg_base + NFC_REG_CMD); >> - >> ret = wait_for_completion_timeout(&nfc->completion, >> msecs_to_jiffies(timeout_ms)); >> if (ret == 0) >> @@ -985,9 +989,11 @@ static const struct mtd_ooblayout_ops >> meson_ooblayout_ops = { >> .free = meson_ooblayout_free, >> }; >> +struct clk_parent_data nfc_divider_parent_data[1]; > > This should be in the meson_nfc_clk_init() function, not global ok. > >> static int meson_nfc_clk_init(struct meson_nfc *nfc) >> { >> int ret; >> + struct clk_init_data init = {0}; >> /* request core clock */ >> nfc->core_clk = devm_clk_get(nfc->dev, "core"); >> @@ -1002,21 +1008,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); > > name = devm_kasprintf(nfc->dev, GFP_KERNEL, "%s#div", dev_name(dev)); > > would be better. ok, thanks. > >> + init.ops = &clk_divider_ops; >> + nfc_divider_parent_data[0].fw_name = "device"; >> + 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 +1041,21 @@ static int meson_nfc_clk_init(struct meson_nfc >> *nfc) >> goto err_device_clk; >> } >> - ret = clk_prepare_enable(nfc->phase_tx); >> + ret = clk_prepare_enable(nfc->nand_clk); >> if (ret) { >> - dev_err(nfc->dev, "failed to enable TX clock\n"); >> - goto err_phase_tx; >> + dev_err(nfc->dev, "pre enable NFC divider fail\n"); >> + goto err_nand_clk; >> } >> - ret = clk_prepare_enable(nfc->phase_rx); >> - if (ret) { >> - dev_err(nfc->dev, "failed to enable RX clock\n"); >> - goto err_phase_rx; >> - } >> - >> - ret = clk_set_rate(nfc->device_clk, 24000000); >> + ret = clk_set_rate(nfc->nand_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 +1064,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); >> } >> @@ -1374,7 +1376,6 @@ 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,21 +1389,15 @@ 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); >> + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); >> 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); >> - } >> + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, >> "emmc"); >> + if (IS_ERR(nfc->sd_emmc_clock)) >> + return PTR_ERR(nfc->sd_emmc_clock); >> irq = platform_get_irq(pdev, 0); >> if (irq < 0) > > Thanks, > Neil > > .
Hi Liang, liang.yang@amlogic.com wrote on Mon, 11 Apr 2022 10:40:15 +0800: > Hi Miquel, > > On 2022/4/4 16:30, Miquel Raynal wrote: > > [ EXTERNAL EMAIL ] > > > > Hi Liang, > > > > liang.yang@amlogic.com wrote on Sat, 2 Apr 2022 15:49:19 +0800: > > > >> 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 <liang.yang@amlogic.com> > >> --- > >> drivers/mtd/nand/raw/meson_nand.c | 89 +++++++++++++++---------------- > >> 1 file changed, 42 insertions(+), 47 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > >> index ac3be92872d0..1b1a9407fb2f 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; > >> @@ -235,7 +240,7 @@ static void meson_nfc_select_chip(struct nand_chip *nand, int chip) > >> nfc->timing.tbers_max = meson_chip->tbers_max; > >> >> if (nfc->clk_rate != meson_chip->clk_rate) { > >> - ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate); > >> + ret = clk_set_rate(nfc->nand_clk, meson_chip->clk_rate); > >> if (ret) { > >> dev_err(nfc->dev, "failed to set clock rate\n"); > >> return; > >> @@ -406,7 +411,6 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) > >> cmd = NFC_CMD_RB | NFC_CMD_RB_INT > >> | nfc->param.chip_select | nfc->timing.tbers_max; > >> writel(cmd, nfc->reg_base + NFC_REG_CMD); > >> - > > > > Please avoid these spacing changes in the middle of a commit. > > ok, i will fix it. > > > >> ret = wait_for_completion_timeout(&nfc->completion, > >> msecs_to_jiffies(timeout_ms)); > >> if (ret == 0) > >> @@ -985,9 +989,11 @@ static const struct mtd_ooblayout_ops meson_ooblayout_ops = { > >> .free = meson_ooblayout_free, > >> }; > >> >> +struct clk_parent_data nfc_divider_parent_data[1]; > >> static int meson_nfc_clk_init(struct meson_nfc *nfc) > >> { > >> int ret; > >> + struct clk_init_data init = {0}; > >> >> /* request core clock */ > >> nfc->core_clk = devm_clk_get(nfc->dev, "core"); > >> @@ -1002,21 +1008,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 = "device"; > >> + 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 +1041,21 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc) > >> goto err_device_clk; > >> } > >> >> - ret = clk_prepare_enable(nfc->phase_tx); > >> + ret = clk_prepare_enable(nfc->nand_clk); > >> if (ret) { > >> - dev_err(nfc->dev, "failed to enable TX clock\n"); > >> - goto err_phase_tx; > >> + dev_err(nfc->dev, "pre enable NFC divider fail\n"); > >> + goto err_nand_clk; > >> } > >> >> - ret = clk_prepare_enable(nfc->phase_rx); > >> - if (ret) { > >> - dev_err(nfc->dev, "failed to enable RX clock\n"); > >> - goto err_phase_rx; > >> - } > >> - > >> - ret = clk_set_rate(nfc->device_clk, 24000000); > >> + ret = clk_set_rate(nfc->nand_clk, 24000000); > > > > Is this rename really useful? > > yes, it works. I understand it works, but if this is just a name change of a variable in your driver that has implications everywhere in this driver, then it's probably best to do it in a separate commit to ease the review. > > > > >> 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 +1064,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); > >> } > >> @@ -1374,7 +1376,6 @@ 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,21 +1389,15 @@ static int meson_nfc_probe(struct platform_device *pdev) > >> nand_controller_init(&nfc->controller); > >> INIT_LIST_HEAD(&nfc->chips); > >> init_completion(&nfc->completion); > >> - > > > > Please don't modify spacing in this commit. > >ok > > >> nfc->dev = dev; > >> >> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> - nfc->reg_base = devm_ioremap_resource(dev, res); > >> + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); > > > > This change seems unrelated. > > To be consistent with the following devm_platform_ioremap_resource_byname(pdev, "emmc"). do you mean that we don't need it?> So indeed it should not be in this commit. You can do that as a preparation patch if you wish. > >> 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); > >> - } > >> + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, "emmc"); > >> + if (IS_ERR(nfc->sd_emmc_clock)) > >> + return PTR_ERR(nfc->sd_emmc_clock); > > > > While I agree this is much better than the previous solution, we cannot > > break DT compatibility, so you need to try getting the emmc clock, but > > if it fails you should fallback to the regmap lookup. > > ok, i will fix it next version. thanks. > > > > >> >> irq = platform_get_irq(pdev, 0); > >> if (irq < 0) > > > > > > Thanks, > > Miquèl > > > > . Thanks, Miquèl
Hi Miquel, On 2022/4/11 15:00, Miquel Raynal wrote: > [ EXTERNAL EMAIL ] > > Hi Liang, > > liang.yang@amlogic.com wrote on Mon, 11 Apr 2022 10:40:15 +0800: > >> Hi Miquel, >> >> On 2022/4/4 16:30, Miquel Raynal wrote: >>> [ EXTERNAL EMAIL ] >>> >>> Hi Liang, >>> >>> liang.yang@amlogic.com wrote on Sat, 2 Apr 2022 15:49:19 +0800: >>> >>>> 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 <liang.yang@amlogic.com> >>>> --- >>>> drivers/mtd/nand/raw/meson_nand.c | 89 +++++++++++++++---------------- >>>> 1 file changed, 42 insertions(+), 47 deletions(-) >>>> >>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c >>>> index ac3be92872d0..1b1a9407fb2f 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; >>>> @@ -235,7 +240,7 @@ static void meson_nfc_select_chip(struct nand_chip *nand, int chip) >>>> nfc->timing.tbers_max = meson_chip->tbers_max; >>>> >> if (nfc->clk_rate != meson_chip->clk_rate) { >>>> - ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate); >>>> + ret = clk_set_rate(nfc->nand_clk, meson_chip->clk_rate); >>>> if (ret) { >>>> dev_err(nfc->dev, "failed to set clock rate\n"); >>>> return; >>>> @@ -406,7 +411,6 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) >>>> cmd = NFC_CMD_RB | NFC_CMD_RB_INT >>>> | nfc->param.chip_select | nfc->timing.tbers_max; >>>> writel(cmd, nfc->reg_base + NFC_REG_CMD); >>>> - >>> >>> Please avoid these spacing changes in the middle of a commit. >> >> ok, i will fix it. >>> >>>> ret = wait_for_completion_timeout(&nfc->completion, >>>> msecs_to_jiffies(timeout_ms)); >>>> if (ret == 0) >>>> @@ -985,9 +989,11 @@ static const struct mtd_ooblayout_ops meson_ooblayout_ops = { >>>> .free = meson_ooblayout_free, >>>> }; >>>> >> +struct clk_parent_data nfc_divider_parent_data[1]; >>>> static int meson_nfc_clk_init(struct meson_nfc *nfc) >>>> { >>>> int ret; >>>> + struct clk_init_data init = {0}; >>>> >> /* request core clock */ >>>> nfc->core_clk = devm_clk_get(nfc->dev, "core"); >>>> @@ -1002,21 +1008,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 = "device"; >>>> + 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 +1041,21 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc) >>>> goto err_device_clk; >>>> } >>>> >> - ret = clk_prepare_enable(nfc->phase_tx); >>>> + ret = clk_prepare_enable(nfc->nand_clk); >>>> if (ret) { >>>> - dev_err(nfc->dev, "failed to enable TX clock\n"); >>>> - goto err_phase_tx; >>>> + dev_err(nfc->dev, "pre enable NFC divider fail\n"); >>>> + goto err_nand_clk; >>>> } >>>> >> - ret = clk_prepare_enable(nfc->phase_rx); >>>> - if (ret) { >>>> - dev_err(nfc->dev, "failed to enable RX clock\n"); >>>> - goto err_phase_rx; >>>> - } >>>> - >>>> - ret = clk_set_rate(nfc->device_clk, 24000000); >>>> + ret = clk_set_rate(nfc->nand_clk, 24000000); >>> >>> Is this rename really useful? >> >> yes, it works. > > I understand it works, but if this is just a name change of a variable > in your driver that has implications everywhere in this driver, then > it's probably best to do it in a separate commit to ease the review. > After apply these patches, i think we have to change the clk from device_clk to nand_clk. previously the device_clk comes from <&sd_emmc_c_clkc CLKID_MMC_DIV> in dts and we set device_clk to give NFC controller the clock; now device_clk comes from <&clkc CLKID_FCLK_DIV2> which is the parent node of nand_clk, so we set nand_clk to give NFC controller the clock. >> >>> >>>> 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 +1064,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); >>>> } >>>> @@ -1374,7 +1376,6 @@ 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,21 +1389,15 @@ static int meson_nfc_probe(struct platform_device *pdev) >>>> nand_controller_init(&nfc->controller); >>>> INIT_LIST_HEAD(&nfc->chips); >>>> init_completion(&nfc->completion); >>>> - >>> >>> Please don't modify spacing in this commit. >>> ok >> >>>> nfc->dev = dev; >>>> >> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> - nfc->reg_base = devm_ioremap_resource(dev, res); >>>> + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); >>> >>> This change seems unrelated. >> >> To be consistent with the following devm_platform_ioremap_resource_byname(pdev, "emmc"). do you mean that we don't need it?> > > So indeed it should not be in this commit. You can do that as a > preparation patch if you wish. ok. May i split this change as one of patches in this series? > >>>> 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); >>>> - } >>>> + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, "emmc"); >>>> + if (IS_ERR(nfc->sd_emmc_clock)) >>>> + return PTR_ERR(nfc->sd_emmc_clock); >>> >>> While I agree this is much better than the previous solution, we cannot >>> break DT compatibility, so you need to try getting the emmc clock, but >>> if it fails you should fallback to the regmap lookup. >> >> ok, i will fix it next version. thanks. >> >>> >>>> >> irq = platform_get_irq(pdev, 0); >>>> if (irq < 0) >>> >>> >>> Thanks, >>> Miquèl >>> >>> . > > > Thanks, > Miquèl > > .
Hi Miquel, i have some confusion when i prepare the patches. for DT compatibility, it falls back to the old DT when failed to get resource by the new DT, but there is some points: a. old DT depends on MMC sub clock driver, but it never be merged, so it can't work. b. if it falls back to the old DT, beside the regmap lookup below, it seems that we have to preserve the code of the old clock setting in nfc_clk_init(). do we still need to avoid break DT compatibility? Thanks. On 2022/4/11 10:40, Liang Yang wrote: >>> nfc->dev = dev; >>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> - nfc->reg_base = devm_ioremap_resource(dev, res); >>> + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); >> >> This change seems unrelated. > > To be consistent with the following > devm_platform_ioremap_resource_byname(pdev, "emmc"). do you mean that we > don't need it?> >>> 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); >>> - } >>> + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, >>> "emmc"); >>> + if (IS_ERR(nfc->sd_emmc_clock)) >>> + return PTR_ERR(nfc->sd_emmc_clock); >> >> While I agree this is much better than the previous solution, we cannot >> break DT compatibility, so you need to try getting the emmc clock, but >> if it fails you should fallback to the regmap lookup. > > ok, i will fix it next version. thanks. > >> >>> irq = platform_get_irq(pdev, 0);
Hello, liang.yang@amlogic.com wrote on Mon, 18 Apr 2022 11:40:10 +0800: > Hi Miquel, > > i have some confusion when i prepare the patches. for DT compatibility, it falls back to the old DT when failed to get resource by the new DT, but there is some points: > a. old DT depends on MMC sub clock driver, but it never be merged, so it can't work. I don't get what you mean here, sorry. I believe there is a new way to describe this clock but grabbing the one from the MMC still works, does not it? > b. if it falls back to the old DT, beside the regmap lookup below, it seems that we have to preserve the code of the old clock setting in nfc_clk_init(). Yes, probably. > do we still need to avoid break DT compatibility? We should try our best to avoid breaking the DT, yes. > > Thanks. > > On 2022/4/11 10:40, Liang Yang wrote: > >>> nfc->dev = dev; > >>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>> - nfc->reg_base = devm_ioremap_resource(dev, res); > >>> + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); > >> > >> This change seems unrelated. > > > > To be consistent with the following > devm_platform_ioremap_resource_byname(pdev, "emmc"). do you mean that we > don't need it?> > >>> 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); > >>> - } > >>> + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, >>> "emmc"); > >>> + if (IS_ERR(nfc->sd_emmc_clock)) > >>> + return PTR_ERR(nfc->sd_emmc_clock); > >> > >> While I agree this is much better than the previous solution, we cannot > >> break DT compatibility, so you need to try getting the emmc clock, but > >> if it fails you should fallback to the regmap lookup. > > > > ok, i will fix it next version. thanks. > > > >> > >>> irq = platform_get_irq(pdev, 0); Thanks, Miquèl
Hello Miquel, On 2022/4/19 16:26, Miquel Raynal wrote: > [ EXTERNAL EMAIL ] > > Hello, > > liang.yang@amlogic.com wrote on Mon, 18 Apr 2022 11:40:10 +0800: > >> Hi Miquel, >> >> i have some confusion when i prepare the patches. for DT compatibility, it falls back to the old DT when failed to get resource by the new DT, but there is some points: >> a. old DT depends on MMC sub clock driver, but it never be merged, so it can't work. > > I don't get what you mean here, sorry. I believe there is a new way to > describe this clock but grabbing the one from the MMC still works, does > not it? > No, it doesn't. after the NFC driver using the MMC sub clock framework was merged into the mainline of kernel, we didn't continue to submit the series of patches about MMC sub clock after v9. when i found that, we made a discussion to decide whether to recover the series of patches about MMC sub clock framework, finally, see the description from cover letter, we plan to abandon it and adopt the new clock scheme in this series of patches. Thanks. >> b. if it falls back to the old DT, beside the regmap lookup below, it seems that we have to preserve the code of the old clock setting in nfc_clk_init(). > > Yes, probably. > >> do we still need to avoid break DT compatibility? > > We should try our best to avoid breaking the DT, yes. > >> >> Thanks. >> >> On 2022/4/11 10:40, Liang Yang wrote: >>>>> nfc->dev = dev; >>>>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> - nfc->reg_base = devm_ioremap_resource(dev, res); >>>>> + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); >>>> >>>> This change seems unrelated. >>> >>> To be consistent with the following > devm_platform_ioremap_resource_byname(pdev, "emmc"). do you mean that we > don't need it?> >>>>> 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); >>>>> - } >>>>> + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, >>> "emmc"); >>>>> + if (IS_ERR(nfc->sd_emmc_clock)) >>>>> + return PTR_ERR(nfc->sd_emmc_clock); >>>> >>>> While I agree this is much better than the previous solution, we cannot >>>> break DT compatibility, so you need to try getting the emmc clock, but >>>> if it fails you should fallback to the regmap lookup. >>> >>> ok, i will fix it next version. thanks. >>> >>>> >>>>> irq = platform_get_irq(pdev, 0); > > > Thanks, > Miquèl > > .
Hello, liang.yang@amlogic.com wrote on Tue, 19 Apr 2022 17:17:48 +0800: > Hello Miquel, > > On 2022/4/19 16:26, Miquel Raynal wrote: > > [ EXTERNAL EMAIL ] > > > > Hello, > > > > liang.yang@amlogic.com wrote on Mon, 18 Apr 2022 11:40:10 +0800: > > > >> Hi Miquel, > >> > >> i have some confusion when i prepare the patches. for DT compatibility, it falls back to the old DT when failed to get resource by the new DT, but there is some points: > >> a. old DT depends on MMC sub clock driver, but it never be merged, so it can't work. > > > > I don't get what you mean here, sorry. I believe there is a new way to > > describe this clock but grabbing the one from the MMC still works, does > > not it? > > > > No, it doesn't. after the NFC driver using the MMC sub clock framework was merged into the mainline of kernel, we didn't continue to submit the series of patches about MMC sub clock after v9. when i found that, we made a discussion to decide whether to recover the series of patches about MMC sub clock framework, finally, see the description from cover letter, we plan to abandon it and adopt the new clock scheme in this series of patches. I am not sure to follow. Is the current code completely broken? I believe it is not, so I don't understand your issue. Can you please summarize the situation? > > Thanks. > > >> b. if it falls back to the old DT, beside the regmap lookup below, it seems that we have to preserve the code of the old clock setting in nfc_clk_init(). > > > > Yes, probably. > > > >> do we still need to avoid break DT compatibility? > > > > We should try our best to avoid breaking the DT, yes. > > > >> > >> Thanks. > >> > >> On 2022/4/11 10:40, Liang Yang wrote: > >>>>> nfc->dev = dev; > >>>>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>>>> - nfc->reg_base = devm_ioremap_resource(dev, res); > >>>>> + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); > >>>> > >>>> This change seems unrelated. > >>> > >>> To be consistent with the following > devm_platform_ioremap_resource_byname(pdev, "emmc"). do you mean that we > don't need it?> > >>>>> 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); > >>>>> - } > >>>>> + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, >>> "emmc"); > >>>>> + if (IS_ERR(nfc->sd_emmc_clock)) > >>>>> + return PTR_ERR(nfc->sd_emmc_clock); > >>>> > >>>> While I agree this is much better than the previous solution, we cannot > >>>> break DT compatibility, so you need to try getting the emmc clock, but > >>>> if it fails you should fallback to the regmap lookup. > >>> > >>> ok, i will fix it next version. thanks. > >>> >>>> >>>>> irq = platform_get_irq(pdev, 0); > > > > > > Thanks, > > Miquèl > > > > . Thanks, Miquèl
Hi Miquel, On 2022/4/19 23:25, Miquel Raynal wrote: > [ EXTERNAL EMAIL ] > > Hello, > > liang.yang@amlogic.com wrote on Tue, 19 Apr 2022 17:17:48 +0800: > >> Hello Miquel, >> >> On 2022/4/19 16:26, Miquel Raynal wrote: >>> [ EXTERNAL EMAIL ] >>> >>> Hello, >>> >>> liang.yang@amlogic.com wrote on Mon, 18 Apr 2022 11:40:10 +0800: >>> >>>> Hi Miquel, >>>> >>>> i have some confusion when i prepare the patches. for DT compatibility, it falls back to the old DT when failed to get resource by the new DT, but there is some points: >>>> a. old DT depends on MMC sub clock driver, but it never be merged, so it can't work. >>> >>> I don't get what you mean here, sorry. I believe there is a new way to >>> describe this clock but grabbing the one from the MMC still works, does >>> not it? >>> >> >> No, it doesn't. after the NFC driver using the MMC sub clock framework was merged into the mainline of kernel, we didn't continue to submit the series of patches about MMC sub clock after v9. when i found that, we made a discussion to decide whether to recover the series of patches about MMC sub clock framework, finally, see the description from cover letter, we plan to abandon it and adopt the new clock scheme in this series of patches. > > I am not sure to follow. Is the current code completely broken? I > believe it is not, so I don't understand your issue. i think only the code about the clock is completely broken. > > Can you please summarize the situation? Yes. the current NFC clock implementation depends on the following series of patches [https://lore.kernel.org/all/20220121074508.42168-5-liang.yang@amlogic.com], which we call "Meson MMC Sub Clock Controller Driver". when i was preparing the NFC patchset at that time, we discussed how the clock should be implemented base on the special clock framework for NFC and EMMC port. then we decided to implement a driver "Meson MMC Sub Clock Controller Driver". so another people begin to prepare "Meson MMC Sub Clock Controller Driver", but submitted it by different patchset. finally, now the meson NFC patchset is accepted and merged, but "Meson MMC Sub Clock Controller Driver" patchset is not. also we decide to abandon the patset "Meson MMC Sub Clock Controller Driver" and implement the new clock design in this series. > >> >> Thanks. >> >>>> b. if it falls back to the old DT, beside the regmap lookup below, it seems that we have to preserve the code of the old clock setting in nfc_clk_init(). >>> >>> Yes, probably. >>> >>>> do we still need to avoid break DT compatibility? >>> >>> We should try our best to avoid breaking the DT, yes. >>> >>>> >>>> Thanks. >>>> >>>> On 2022/4/11 10:40, Liang Yang wrote: >>>>>>> nfc->dev = dev; >>>>>>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>>> - nfc->reg_base = devm_ioremap_resource(dev, res); >>>>>>> + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); >>>>>> >>>>>> This change seems unrelated. >>>>> >>>>> To be consistent with the following > devm_platform_ioremap_resource_byname(pdev, "emmc"). do you mean that we > don't need it?> >>>>>>> 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); >>>>>>> - } >>>>>>> + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, >>> "emmc"); >>>>>>> + if (IS_ERR(nfc->sd_emmc_clock)) >>>>>>> + return PTR_ERR(nfc->sd_emmc_clock); >>>>>> >>>>>> While I agree this is much better than the previous solution, we cannot >>>>>> break DT compatibility, so you need to try getting the emmc clock, but >>>>>> if it fails you should fallback to the regmap lookup. >>>>> >>>>> ok, i will fix it next version. thanks. >>>>> >>>> >>>>> irq = platform_get_irq(pdev, 0); >>> >>> >>> Thanks, >>> Miquèl >>> >>> . > > > Thanks, > Miquèl > > .
Hi Liang, liang.yang@amlogic.com wrote on Wed, 20 Apr 2022 13:44:32 +0800: > Hi Miquel, > > On 2022/4/19 23:25, Miquel Raynal wrote: > > [ EXTERNAL EMAIL ] > > > > Hello, > > > > liang.yang@amlogic.com wrote on Tue, 19 Apr 2022 17:17:48 +0800: > > > >> Hello Miquel, > >> > >> On 2022/4/19 16:26, Miquel Raynal wrote: > >>> [ EXTERNAL EMAIL ] > >>> > >>> Hello, > >>> > >>> liang.yang@amlogic.com wrote on Mon, 18 Apr 2022 11:40:10 +0800: > >>> >>>> Hi Miquel, > >>>> > >>>> i have some confusion when i prepare the patches. for DT compatibility, it falls back to the old DT when failed to get resource by the new DT, but there is some points: > >>>> a. old DT depends on MMC sub clock driver, but it never be merged, so it can't work. > >>> > >>> I don't get what you mean here, sorry. I believe there is a new way to > >>> describe this clock but grabbing the one from the MMC still works, does > >>> not it? > >>> >> > >> No, it doesn't. after the NFC driver using the MMC sub clock framework was merged into the mainline of kernel, we didn't continue to submit the series of patches about MMC sub clock after v9. when i found that, we made a discussion to decide whether to recover the series of patches about MMC sub clock framework, finally, see the description from cover letter, we plan to abandon it and adopt the new clock scheme in this series of patches. > > > > I am not sure to follow. Is the current code completely broken? I > > believe it is not, so I don't understand your issue. > > i think only the code about the clock is completely broken. > > > > > Can you please summarize the situation? > > Yes. the current NFC clock implementation depends on the following series of patches [https://lore.kernel.org/all/20220121074508.42168-5-liang.yang@amlogic.com], which we call "Meson MMC Sub Clock Controller Driver". > when i was preparing the NFC patchset at that time, we discussed how the clock should be implemented base on the special clock framework for NFC and EMMC port. then we decided to implement a driver "Meson MMC Sub Clock Controller Driver". so another people begin to prepare "Meson MMC Sub Clock Controller Driver", but submitted it by different patchset. > finally, now the meson NFC patchset is accepted and merged, but "Meson MMC Sub Clock Controller Driver" patchset is not. also we decide to abandon the patset "Meson MMC Sub Clock Controller Driver" and implement the new clock design in this series. Ok thanks for the summary and the link with the discussion with Jerome and Neil, it's informative. So in the end, we are not really breaking anything here as this NAND controller driver never worked in the first place? Or is it only one of the two compatibles which is not working? If this never worked then please do the binding changes (in the first patch of your series) and then do the necessary changes in the code. If this worked with at least one of the two compatibles, then you have to create dedicated helpers, one for each, in order to grab the clocks differently and not break anybody. > > > > >> > >> Thanks. > >> > >>>> b. if it falls back to the old DT, beside the regmap lookup below, it seems that we have to preserve the code of the old clock setting in nfc_clk_init(). > >>> > >>> Yes, probably. > >>> >>>> do we still need to avoid break DT compatibility? > >>> > >>> We should try our best to avoid breaking the DT, yes. > >>> >>>> > >>>> Thanks. > >>>> > >>>> On 2022/4/11 10:40, Liang Yang wrote: > >>>>>>> nfc->dev = dev; > >>>>>>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>>>>>> - nfc->reg_base = devm_ioremap_resource(dev, res); > >>>>>>> + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); > >>>>>> > >>>>>> This change seems unrelated. > >>>>> > >>>>> To be consistent with the following > devm_platform_ioremap_resource_byname(pdev, "emmc"). do you mean that we > don't need it?> > >>>>>>> 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); > >>>>>>> - } > >>>>>>> + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, >>> "emmc"); > >>>>>>> + if (IS_ERR(nfc->sd_emmc_clock)) > >>>>>>> + return PTR_ERR(nfc->sd_emmc_clock); > >>>>>> > >>>>>> While I agree this is much better than the previous solution, we cannot > >>>>>> break DT compatibility, so you need to try getting the emmc clock, but > >>>>>> if it fails you should fallback to the regmap lookup. > >>>>> > >>>>> ok, i will fix it next version. thanks. > >>>>> >>>> >>>>> irq = platform_get_irq(pdev, 0); > >>> > >>> > >>> Thanks, > >>> Miquèl > >>> > >>> . > > > > > > Thanks, > > Miquèl > > > > . Thanks, Miquèl
Hi Miquel, On 2022/4/20 15:29, Miquel Raynal wrote: > [ EXTERNAL EMAIL ] > > Hi Liang, > > liang.yang@amlogic.com wrote on Wed, 20 Apr 2022 13:44:32 +0800: > >> Hi Miquel, >> >> On 2022/4/19 23:25, Miquel Raynal wrote: >>> [ EXTERNAL EMAIL ] >>> >>> Hello, >>> >>> liang.yang@amlogic.com wrote on Tue, 19 Apr 2022 17:17:48 +0800: >>> >>>> Hello Miquel, >>>> >>>> On 2022/4/19 16:26, Miquel Raynal wrote: >>>>> [ EXTERNAL EMAIL ] >>>>> >>>>> Hello, >>>>> >>>>> liang.yang@amlogic.com wrote on Mon, 18 Apr 2022 11:40:10 +0800: >>>>> >>>> Hi Miquel, >>>>>> >>>>>> i have some confusion when i prepare the patches. for DT compatibility, it falls back to the old DT when failed to get resource by the new DT, but there is some points: >>>>>> a. old DT depends on MMC sub clock driver, but it never be merged, so it can't work. >>>>> >>>>> I don't get what you mean here, sorry. I believe there is a new way to >>>>> describe this clock but grabbing the one from the MMC still works, does >>>>> not it? >>>>> >> >>>> No, it doesn't. after the NFC driver using the MMC sub clock framework was merged into the mainline of kernel, we didn't continue to submit the series of patches about MMC sub clock after v9. when i found that, we made a discussion to decide whether to recover the series of patches about MMC sub clock framework, finally, see the description from cover letter, we plan to abandon it and adopt the new clock scheme in this series of patches. >>> >>> I am not sure to follow. Is the current code completely broken? I >>> believe it is not, so I don't understand your issue. >> >> i think only the code about the clock is completely broken. >> >>> >>> Can you please summarize the situation? >> >> Yes. the current NFC clock implementation depends on the following series of patches [https://lore.kernel.org/all/20220121074508.42168-5-liang.yang@amlogic.com], which we call "Meson MMC Sub Clock Controller Driver". >> when i was preparing the NFC patchset at that time, we discussed how the clock should be implemented base on the special clock framework for NFC and EMMC port. then we decided to implement a driver "Meson MMC Sub Clock Controller Driver". so another people begin to prepare "Meson MMC Sub Clock Controller Driver", but submitted it by different patchset. >> finally, now the meson NFC patchset is accepted and merged, but "Meson MMC Sub Clock Controller Driver" patchset is not. also we decide to abandon the patset "Meson MMC Sub Clock Controller Driver" and implement the new clock design in this series. > > Ok thanks for the summary and the link with the discussion with Jerome > and Neil, it's informative. > > So in the end, we are not really breaking anything here as this NAND > controller driver never worked in the first place? Or is it only one of > the two compatibles which is not working? i think no one can work now. i am preparing the newer clock framework in this series. > > If this never worked then please do the binding changes (in the first > patch of your series) and then do the necessary changes in the code. If > this worked with at least one of the two compatibles, then you have to > create dedicated helpers, one for each, in order to grab the clocks > differently and not break anybody. ok, i am changing the bindings and code in this series. thanks for your explanation. > >> >>> >>>> >>>> Thanks. >>>> >>>>>> b. if it falls back to the old DT, beside the regmap lookup below, it seems that we have to preserve the code of the old clock setting in nfc_clk_init(). >>>>> >>>>> Yes, probably. >>>>> >>>> do we still need to avoid break DT compatibility? >>>>> >>>>> We should try our best to avoid breaking the DT, yes. >>>>> >>>> >>>>>> Thanks. >>>>>> >>>>>> On 2022/4/11 10:40, Liang Yang wrote: >>>>>>>>> nfc->dev = dev; >>>>>>>>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>>>>> - nfc->reg_base = devm_ioremap_resource(dev, res); >>>>>>>>> + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); >>>>>>>> >>>>>>>> This change seems unrelated. >>>>>>> >>>>>>> To be consistent with the following > devm_platform_ioremap_resource_byname(pdev, "emmc"). do you mean that we > don't need it?> >>>>>>>>> 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); >>>>>>>>> - } >>>>>>>>> + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, >>> "emmc"); >>>>>>>>> + if (IS_ERR(nfc->sd_emmc_clock)) >>>>>>>>> + return PTR_ERR(nfc->sd_emmc_clock); >>>>>>>> >>>>>>>> While I agree this is much better than the previous solution, we cannot >>>>>>>> break DT compatibility, so you need to try getting the emmc clock, but >>>>>>>> if it fails you should fallback to the regmap lookup. >>>>>>> >>>>>>> ok, i will fix it next version. thanks. >>>>>>> >>>> >>>>> irq = platform_get_irq(pdev, 0); >>>>> >>>>> >>>>> Thanks, >>>>> Miquèl >>>>> >>>>> . >>> >>> >>> Thanks, >>> Miquèl >>> >>> . > > > Thanks, > Miquèl > > .
diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c index ac3be92872d0..1b1a9407fb2f 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; @@ -235,7 +240,7 @@ static void meson_nfc_select_chip(struct nand_chip *nand, int chip) nfc->timing.tbers_max = meson_chip->tbers_max; if (nfc->clk_rate != meson_chip->clk_rate) { - ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate); + ret = clk_set_rate(nfc->nand_clk, meson_chip->clk_rate); if (ret) { dev_err(nfc->dev, "failed to set clock rate\n"); return; @@ -406,7 +411,6 @@ static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) cmd = NFC_CMD_RB | NFC_CMD_RB_INT | nfc->param.chip_select | nfc->timing.tbers_max; writel(cmd, nfc->reg_base + NFC_REG_CMD); - ret = wait_for_completion_timeout(&nfc->completion, msecs_to_jiffies(timeout_ms)); if (ret == 0) @@ -985,9 +989,11 @@ static const struct mtd_ooblayout_ops meson_ooblayout_ops = { .free = meson_ooblayout_free, }; +struct clk_parent_data nfc_divider_parent_data[1]; static int meson_nfc_clk_init(struct meson_nfc *nfc) { int ret; + struct clk_init_data init = {0}; /* request core clock */ nfc->core_clk = devm_clk_get(nfc->dev, "core"); @@ -1002,21 +1008,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 = "device"; + 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 +1041,21 @@ static int meson_nfc_clk_init(struct meson_nfc *nfc) goto err_device_clk; } - ret = clk_prepare_enable(nfc->phase_tx); + ret = clk_prepare_enable(nfc->nand_clk); if (ret) { - dev_err(nfc->dev, "failed to enable TX clock\n"); - goto err_phase_tx; + dev_err(nfc->dev, "pre enable NFC divider fail\n"); + goto err_nand_clk; } - ret = clk_prepare_enable(nfc->phase_rx); - if (ret) { - dev_err(nfc->dev, "failed to enable RX clock\n"); - goto err_phase_rx; - } - - ret = clk_set_rate(nfc->device_clk, 24000000); + ret = clk_set_rate(nfc->nand_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 +1064,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); } @@ -1374,7 +1376,6 @@ 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,21 +1389,15 @@ 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); + nfc->reg_base = devm_platform_ioremap_resource_byname(pdev, "nfc"); 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); - } + nfc->sd_emmc_clock = devm_platform_ioremap_resource_byname(pdev, "emmc"); + if (IS_ERR(nfc->sd_emmc_clock)) + return PTR_ERR(nfc->sd_emmc_clock); irq = platform_get_irq(pdev, 0); if (irq < 0)
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 <liang.yang@amlogic.com> --- drivers/mtd/nand/raw/meson_nand.c | 89 +++++++++++++++---------------- 1 file changed, 42 insertions(+), 47 deletions(-)