| Message ID | 20220513123404.48513-3-liang.yang@amlogic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix the meson NFC clock | expand |
On 13/05/2022 14:34, 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. At the beginning, > a common MMC and NAND sub-clock was discussed and planed to be implemented > as NFC clock provider, but now this series of patches of a common MMC and > NAND sub-clock are never being accepted. the reasons for giving up are: > 1. EMMC and NAND, which are mutually exclusive anyway > 2. coupling the EMMC and NAND. > 3. it seems that a common MMC and NAND sub-clock is over engineered. > and let us see the link fot more information: > https://lore.kernel.org/all/20220121074508.42168-5-liang.yang@amlogic.com > so The meson nfc can't work now, let us rework the clock. > > Signed-off-by: Liang Yang <liang.yang@amlogic.com> > --- > drivers/mtd/nand/raw/meson_nand.c | 82 +++++++++++++++---------------- > 1 file changed, 41 insertions(+), 41 deletions(-) > > diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > index ac3be92872d0..cc93667a1e7f 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> > @@ -56,6 +57,9 @@ > > #define NFC_RB_IRQ_EN BIT(21) > > +#define CLK_DIV_SHIFT 0 > +#define CLK_DIV_WIDTH 6 > + > #define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages) \ > ( \ > (cmd_dir) | \ > @@ -151,15 +155,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 *reg_clk; > struct completion completion; > struct list_head chips; > const struct meson_nfc_data *data; > @@ -235,7 +239,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; > @@ -987,6 +991,8 @@ static const struct mtd_ooblayout_ops meson_ooblayout_ops = { > > static int meson_nfc_clk_init(struct meson_nfc *nfc) > { > + struct clk_parent_data nfc_divider_parent_data[1]; > + struct clk_init_data init = {0}; > int ret; > > /* request core clock */ > @@ -1002,21 +1008,28 @@ 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_kasprintf(nfc->dev, > + GFP_KERNEL, "%s#div", > + dev_name(nfc->dev)); > + 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->reg_clk; > + 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->reg_clk), > + nfc->reg_clk); > > ret = clk_prepare_enable(nfc->core_clk); > if (ret) { > @@ -1030,29 +1043,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 +1066,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); > } > @@ -1396,13 +1400,9 @@ static int meson_nfc_probe(struct platform_device *pdev) > 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"); > + nfc->reg_clk = devm_platform_ioremap_resource_byname(pdev, "emmc"); > + if (IS_ERR(nfc->reg_clk)) > return PTR_ERR(nfc->reg_clk); > - } > > irq = platform_get_irq(pdev, 0); > if (irq < 0) Looks fine for me, thank for the changes: Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Hi Liang,
I love your patch! Yet something to improve:
[auto build test ERROR on mtd/mtd/next]
[also build test ERROR on mtd/mtd/fixes mtd/nand/next v5.18-rc6 next-20220513]
[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/fix-the-meson-NFC-clock/20220513-203700
base: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next
config: m68k-buildonly-randconfig-r001-20220512 (https://download.01.org/0day-ci/archive/20220514/202205140935.9YRSlRhB-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.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/2a8371d672486fa71241af04615efeff5efed94b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Liang-Yang/fix-the-meson-NFC-clock/20220513-203700
git checkout 2a8371d672486fa71241af04615efeff5efed94b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k 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 >>):
m68k-linux-ld: drivers/mtd/nand/raw/meson_nand.o: in function `meson_nfc_probe':
>> meson_nand.c:(.text+0xcf8): undefined reference to `clk_divider_ops'
>> m68k-linux-ld: meson_nand.c:(.text+0xd38): undefined reference to `devm_clk_register'
Hi Liang, Liang Yang <liang.yang@amlogic.com> writes: > 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. At the beginning, > a common MMC and NAND sub-clock was discussed and planed to be implemented > as NFC clock provider, but now this series of patches of a common MMC and > NAND sub-clock are never being accepted. the reasons for giving up are: > 1. EMMC and NAND, which are mutually exclusive anyway > 2. coupling the EMMC and NAND. > 3. it seems that a common MMC and NAND sub-clock is over engineered. > and let us see the link fot more information: > https://lore.kernel.org/all/20220121074508.42168-5-liang.yang@amlogic.com > so The meson nfc can't work now, let us rework the clock. > > Signed-off-by: Liang Yang <liang.yang@amlogic.com> Reviewed-by: Kevin Hilman <khilman@baylibre.com> Thank you for your persistence in working on multiple iterations of this until we came to a final agreement. Kevin
Hi Kevin, Thanks for your review. On 2022/5/27 0:28, Kevin Hilman wrote: > [ EXTERNAL EMAIL ] > > Hi Liang, > > Liang Yang <liang.yang@amlogic.com> writes: > >> 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. At the beginning, >> a common MMC and NAND sub-clock was discussed and planed to be implemented >> as NFC clock provider, but now this series of patches of a common MMC and >> NAND sub-clock are never being accepted. the reasons for giving up are: >> 1. EMMC and NAND, which are mutually exclusive anyway >> 2. coupling the EMMC and NAND. >> 3. it seems that a common MMC and NAND sub-clock is over engineered. >> and let us see the link fot more information: >> https://lore.kernel.org/all/20220121074508.42168-5-liang.yang@amlogic.com >> so The meson nfc can't work now, let us rework the clock. >> >> Signed-off-by: Liang Yang <liang.yang@amlogic.com> > > Reviewed-by: Kevin Hilman <khilman@baylibre.com> > > Thank you for your persistence in working on multiple iterations of this > until we came to a final agreement. > > Kevin > > .
diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c index ac3be92872d0..cc93667a1e7f 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> @@ -56,6 +57,9 @@ #define NFC_RB_IRQ_EN BIT(21) +#define CLK_DIV_SHIFT 0 +#define CLK_DIV_WIDTH 6 + #define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages) \ ( \ (cmd_dir) | \ @@ -151,15 +155,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 *reg_clk; struct completion completion; struct list_head chips; const struct meson_nfc_data *data; @@ -235,7 +239,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; @@ -987,6 +991,8 @@ static const struct mtd_ooblayout_ops meson_ooblayout_ops = { static int meson_nfc_clk_init(struct meson_nfc *nfc) { + struct clk_parent_data nfc_divider_parent_data[1]; + struct clk_init_data init = {0}; int ret; /* request core clock */ @@ -1002,21 +1008,28 @@ 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_kasprintf(nfc->dev, + GFP_KERNEL, "%s#div", + dev_name(nfc->dev)); + 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->reg_clk; + 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->reg_clk), + nfc->reg_clk); ret = clk_prepare_enable(nfc->core_clk); if (ret) { @@ -1030,29 +1043,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 +1066,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); } @@ -1396,13 +1400,9 @@ static int meson_nfc_probe(struct platform_device *pdev) 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"); + nfc->reg_clk = devm_platform_ioremap_resource_byname(pdev, "emmc"); + if (IS_ERR(nfc->reg_clk)) return PTR_ERR(nfc->reg_clk); - } 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. At the beginning, a common MMC and NAND sub-clock was discussed and planed to be implemented as NFC clock provider, but now this series of patches of a common MMC and NAND sub-clock are never being accepted. the reasons for giving up are: 1. EMMC and NAND, which are mutually exclusive anyway 2. coupling the EMMC and NAND. 3. it seems that a common MMC and NAND sub-clock is over engineered. and let us see the link fot more information: https://lore.kernel.org/all/20220121074508.42168-5-liang.yang@amlogic.com so The meson nfc can't work now, let us rework the clock. Signed-off-by: Liang Yang <liang.yang@amlogic.com> --- drivers/mtd/nand/raw/meson_nand.c | 82 +++++++++++++++---------------- 1 file changed, 41 insertions(+), 41 deletions(-)