Background firstly,
Both 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.
Previously a common MMC sub clock framework is implemented and shared by EMMC and
NAND, but that is coupling the EMMC and NAND, although EMMC and NAND is mutually
exclusive. see the link:
[https://lore.kernel.org/all/[email protected]/]
Now we plan to abandon common mmc sub clock framework and recovery the series.
Changes since v3 [4]
- use devm_platform_ioremap_resource_byname
- dt_binding_check for mtd/amlogic,meson-nand.yaml
Changes since v2 [3]
- use fw_name from dts, instead the wrong way using __clk_get_name
- reg resource size change to 0x800
- use reg-names
Changes since v1 [2]
- use clk_parent_data instead of parent_names
- define a reg resource instead of sd_emmc_c_clkc
[1] https://lore.kernel.org/r/[email protected]
https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/all/[email protected]
[3] https://lore.kernel.org/all/[email protected]
Liang Yang (2):
mtd: rawnand: meson: discard the common MMC sub clock framework
dt-bindings: nand: meson: refine Amlogic NAND controller driver
.../bindings/mtd/amlogic,meson-nand.txt | 60 -------------
.../bindings/mtd/amlogic,meson-nand.yaml | 80 +++++++++++++++++
drivers/mtd/nand/raw/meson_nand.c | 89 +++++++++----------
3 files changed, 122 insertions(+), 107 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
create mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
--
2.34.1
EMMC and NAND have the same clock control register named 'SD_EMMC_CLOCK' which is
defined in EMMC port internally. bit0~5 of 'SD_EMMC_CLOCK' is the divider and
bit6~7 is the mux for fix pll and xtal.A common MMC and NAND sub-clock has been
implemented and can be used by the eMMC and NAND controller (which are mutually
exclusive anyway). Let's use this new clock.
Signed-off-by: Liang Yang <[email protected]>
---
drivers/mtd/nand/raw/meson_nand.c | 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);
-
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)
--
2.34.1
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/[email protected]/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 <[email protected]>
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!
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Miquel,
On 2022/4/11 15:00, Miquel Raynal wrote:
> [ EXTERNAL EMAIL ]
>
> Hi Liang,
>
> [email protected] 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,
>>>
>>> [email protected] 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 <[email protected]>
>>>> ---
>>>> 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
>
> .
Hello,
[email protected] wrote on Tue, 19 Apr 2022 17:17:48 +0800:
> Hello Miquel,
>
> On 2022/4/19 16:26, Miquel Raynal wrote:
> > [ EXTERNAL EMAIL ]
> >
> > Hello,
> >
> > [email protected] 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/20 15:29, Miquel Raynal wrote:
> [ EXTERNAL EMAIL ]
>
> Hi Liang,
>
> [email protected] wrote on Wed, 20 Apr 2022 13:44:32 +0800:
>
>> Hi Miquel,
>>
>> On 2022/4/19 23:25, Miquel Raynal wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Hello,
>>>
>>> [email protected] wrote on Tue, 19 Apr 2022 17:17:48 +0800:
>>>
>>>> Hello Miquel,
>>>>
>>>> On 2022/4/19 16:26, Miquel Raynal wrote:
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> Hello,
>>>>>
>>>>> [email protected] 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/[email protected]], 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
>
> .
Hi Miquel,
On 2022/4/19 23:25, Miquel Raynal wrote:
> [ EXTERNAL EMAIL ]
>
> Hello,
>
> [email protected] wrote on Tue, 19 Apr 2022 17:17:48 +0800:
>
>> Hello Miquel,
>>
>> On 2022/4/19 16:26, Miquel Raynal wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Hello,
>>>
>>> [email protected] 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/[email protected]],
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,
[email protected] wrote on Wed, 20 Apr 2022 13:44:32 +0800:
> Hi Miquel,
>
> On 2022/4/19 23:25, Miquel Raynal wrote:
> > [ EXTERNAL EMAIL ]
> >
> > Hello,
> >
> > [email protected] wrote on Tue, 19 Apr 2022 17:17:48 +0800:
> >
> >> Hello Miquel,
> >>
> >> On 2022/4/19 16:26, Miquel Raynal wrote:
> >>> [ EXTERNAL EMAIL ]
> >>>
> >>> Hello,
> >>>
> >>> [email protected] 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/[email protected]], 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