[v5,3/3] clk: meson: add sub MMC clock controller driver

Commit Message

Jianxin Pan Oct. 18, 2018, 5:07 a.m. UTC

From: Yixun Lan <[e-mail]>

The patch will add a MMC clock controller driver which used by MMC or NAND,
It provide a mux and divider clock, and three phase clocks - core, tx, tx.

Two clocks are provided as the parent of MMC clock controller from
upper layer clock controller - eg "amlogic,axg-clkc" in AXG platform.

To specify which clock the MMC or NAND driver may consume,
the preprocessor macros in the dt-bindings/clock/amlogic,mmc-clkc.h header
can be used in the device tree sources.

Signed-off-by: Yixun Lan <[e-mail]>
Signed-off-by: Jianxin Pan <[e-mail]>
---
 drivers/clk/meson/Kconfig      |  10 ++
 drivers/clk/meson/Makefile     |   1 +
 drivers/clk/meson/clk-regmap.c |  27 +++-
 drivers/clk/meson/clk-regmap.h |   1 +
 drivers/clk/meson/mmc-clkc.c   | 296 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 334 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/meson/mmc-clkc.c

Comments

Stephen Boyd Oct. 18, 2018, 5:13 p.m. UTC | #1

Quoting Jianxin Pan (2018-10-17 22:07:25)
> diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
> index 305ee30..f96314d 100644
> --- a/drivers/clk/meson/clk-regmap.c
> +++ b/drivers/clk/meson/clk-regmap.c
> @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>                                   clk_div_mask(div->width) << div->shift, val);
>  };
>  
> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> +static void clk_regmap_div_init(struct clk_hw *hw)
> +{
> +       struct clk_regmap *clk = to_clk_regmap(hw);
> +       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> +       unsigned int val;
> +       int ret;
> +
> +       ret = regmap_read(clk->map, div->offset, &val);
> +       if (ret)
> +               return;
>  
> +       val &= (clk_div_mask(div->width) << div->shift);
> +       if (!val)
> +               regmap_update_bits(clk->map, div->offset,
> +                                  clk_div_mask(div->width) << div->shift,
> +                                  clk_div_mask(div->width));
> +}
> +
> +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */

We should add a patch to rename the symbol for qcom, i.e.
qcom_clk_regmap_div_ro_ops, and then any symbols in this directory
should be meson_clk_regmap_div_ro_ops.

Or we should just give up and squash the regmap implementations together
into a new clk_regmap set of ops.

>  const struct clk_ops clk_regmap_divider_ops = {
>         .recalc_rate = clk_regmap_div_recalc_rate,
>         .round_rate = clk_regmap_div_round_rate,
> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> new file mode 100644
> index 0000000..5555e3f
> --- /dev/null
> +++ b/drivers/clk/meson/mmc-clkc.c
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson MMC Sub Clock Controller Driver
> + *
> + * Copyright (c) 2017 Baylibre SAS.
> + * Author: Jerome Brunet <[e-mail]>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <[e-mail]>
> + */
> +
> +#include <linux/clk.h>

clk-provider.h instead of clk.h?

> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/clock/amlogic,mmc-clkc.h>
> +
> +
[...]
> +
> +static struct clk_regmap *
> +mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
> +                                 char *suffix, const char *parent,
> +                                 unsigned long flags,
> +                                 const struct clk_ops *ops, void *data)
> +{
> +       struct clk_init_data init;
> +       struct clk_regmap *clk;
> +
> +       init.ops = ops;
> +       init.flags = flags;
> +       init.parent_names = (const char* const []){ parent, };

Can't we just assign &parent here?

> +       init.num_parents = 1;
> +
> +       clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
> +       if (IS_ERR(clk))
> +               dev_err(dev, "Core %s clock registration failed\n", suffix);
> +
> +       return clk;
> +}
> +
> +static int mmc_clkc_probe(struct platform_device *pdev)
> +{
> +       struct clk_hw_onecell_data *onecell_data;
> +       struct device *dev = &pdev->dev;
> +       struct mmc_clkc_data *data;
> +       struct regmap *map;
> +       struct clk_regmap *mux, *div, *core, *rx, *tx;
> +
> +       data = (struct mmc_clkc_data *)of_device_get_match_data(dev);

Nitpick: Drop the cast.

> +       if (!data)
> +               return -ENODEV;
> +
> +       map = syscon_node_to_regmap(dev->of_node);

Jianxin Pan Oct. 19, 2018, 4:12 p.m. UTC | #2

On 2018/10/19 1:13, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-10-17 22:07:25)
>> diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
>> index 305ee30..f96314d 100644
>> --- a/drivers/clk/meson/clk-regmap.c
>> +++ b/drivers/clk/meson/clk-regmap.c
>> @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>                                   clk_div_mask(div->width) << div->shift, val);
>>  };
>>  
>> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>> +static void clk_regmap_div_init(struct clk_hw *hw)
>> +{
>> +       struct clk_regmap *clk = to_clk_regmap(hw);
>> +       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>> +       unsigned int val;
>> +       int ret;
>> +
>> +       ret = regmap_read(clk->map, div->offset, &val);
>> +       if (ret)
>> +               return;
>>  
>> +       val &= (clk_div_mask(div->width) << div->shift);
>> +       if (!val)
>> +               regmap_update_bits(clk->map, div->offset,
>> +                                  clk_div_mask(div->width) << div->shift,
>> +                                  clk_div_mask(div->width));
>> +}
>> +
>> +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> 
> We should add a patch to rename the symbol for qcom, i.e.
> qcom_clk_regmap_div_ro_ops, and then any symbols in this directory
> should be meson_clk_regmap_div_ro_ops.
"/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */"
This comment is not introduced in this patch.
I followed the naming style in this file and add clk_regmap_divider_with_init_ops.

@Jerome， What's your suggestion about this？
> 
> Or we should just give up and squash the regmap implementations together
> into a new clk_regmap set of ops.
> 
>>  const struct clk_ops clk_regmap_divider_ops = {
>>         .recalc_rate = clk_regmap_div_recalc_rate,
>>         .round_rate = clk_regmap_div_round_rate,
>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>> new file mode 100644
>> index 0000000..5555e3f
>> --- /dev/null
>> +++ b/drivers/clk/meson/mmc-clkc.c
>> @@ -0,0 +1,296 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson MMC Sub Clock Controller Driver
>> + *
>> + * Copyright (c) 2017 Baylibre SAS.
>> + * Author: Jerome Brunet <[e-mail]>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <[e-mail]>
>> + */
>> +
>> +#include <linux/clk.h>
> 
> clk-provider.h instead of clk.h?
OK.
> 
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/platform_device.h>
>> +#include <dt-bindings/clock/amlogic,mmc-clkc.h>
>> +
>> +
> [...]
>> +
>> +static struct clk_regmap *
>> +mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
>> +                                 char *suffix, const char *parent,
>> +                                 unsigned long flags,
>> +                                 const struct clk_ops *ops, void *data)
>> +{
>> +       struct clk_init_data init;
>> +       struct clk_regmap *clk;
>> +
>> +       init.ops = ops;
>> +       init.flags = flags;
>> +       init.parent_names = (const char* const []){ parent, };
> 
> Can't we just assign &parent here?
OK. I will fix it in the next version.
> 
>> +       init.num_parents = 1;
>> +
>> +       clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
>> +       if (IS_ERR(clk))
>> +               dev_err(dev, "Core %s clock registration failed\n", suffix);
>> +
>> +       return clk;
>> +}
>> +
>> +static int mmc_clkc_probe(struct platform_device *pdev)
>> +{
>> +       struct clk_hw_onecell_data *onecell_data;
>> +       struct device *dev = &pdev->dev;
>> +       struct mmc_clkc_data *data;
>> +       struct regmap *map;
>> +       struct clk_regmap *mux, *div, *core, *rx, *tx;
>> +
>> +       data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
> 
> Nitpick: Drop the cast.
OK. I will drop it.
Thanks for the review.

> 
>> +       if (!data)
>> +               return -ENODEV;
>> +
>> +       map = syscon_node_to_regmap(dev->of_node);
> 
> .
>

Stephen Boyd Oct. 19, 2018, 6:03 p.m. UTC | #3

Quoting Jianxin Pan (2018-10-19 09:12:53)
> On 2018/10/19 1:13, Stephen Boyd wrote:
> > Quoting Jianxin Pan (2018-10-17 22:07:25)
> >> diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
> >> index 305ee30..f96314d 100644
> >> --- a/drivers/clk/meson/clk-regmap.c
> >> +++ b/drivers/clk/meson/clk-regmap.c
> >> @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
> >>                                   clk_div_mask(div->width) << div->shift, val);
> >>  };
> >>  
> >> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> >> +static void clk_regmap_div_init(struct clk_hw *hw)
> >> +{
> >> +       struct clk_regmap *clk = to_clk_regmap(hw);
> >> +       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> >> +       unsigned int val;
> >> +       int ret;
> >> +
> >> +       ret = regmap_read(clk->map, div->offset, &val);
> >> +       if (ret)
> >> +               return;
> >>  
> >> +       val &= (clk_div_mask(div->width) << div->shift);
> >> +       if (!val)
> >> +               regmap_update_bits(clk->map, div->offset,
> >> +                                  clk_div_mask(div->width) << div->shift,
> >> +                                  clk_div_mask(div->width));
> >> +}
> >> +
> >> +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> > 
> > We should add a patch to rename the symbol for qcom, i.e.
> > qcom_clk_regmap_div_ro_ops, and then any symbols in this directory
> > should be meson_clk_regmap_div_ro_ops.
> "/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */"
> This comment is not introduced in this patch.
> I followed the naming style in this file and add clk_regmap_divider_with_init_ops.
> 
> @Jerome， What's your suggestion about this？

Yes you don't need to fix anything in this series. Just saying that in
the future we should work on cleaning this up.

Jianxin Pan Oct. 22, 2018, 5:59 a.m. UTC | #4

On 2018/10/20 2:03, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-10-19 09:12:53)
>> On 2018/10/19 1:13, Stephen Boyd wrote:
>>> Quoting Jianxin Pan (2018-10-17 22:07:25)
>>>> diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
>>>> index 305ee30..f96314d 100644
>>>> --- a/drivers/clk/meson/clk-regmap.c
>>>> +++ b/drivers/clk/meson/clk-regmap.c
>>>> @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>>>                                   clk_div_mask(div->width) << div->shift, val);
>>>>  };
>>>>  
>>>> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>>>> +static void clk_regmap_div_init(struct clk_hw *hw)
>>>> +{
>>>> +       struct clk_regmap *clk = to_clk_regmap(hw);
>>>> +       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>>>> +       unsigned int val;
>>>> +       int ret;
>>>> +
>>>> +       ret = regmap_read(clk->map, div->offset, &val);
>>>> +       if (ret)
>>>> +               return;
>>>>  
>>>> +       val &= (clk_div_mask(div->width) << div->shift);
>>>> +       if (!val)
>>>> +               regmap_update_bits(clk->map, div->offset,
>>>> +                                  clk_div_mask(div->width) << div->shift,
>>>> +                                  clk_div_mask(div->width));
>>>> +}
>>>> +
>>>> +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>>>
>>> We should add a patch to rename the symbol for qcom, i.e.
>>> qcom_clk_regmap_div_ro_ops, and then any symbols in this directory
>>> should be meson_clk_regmap_div_ro_ops.
>> "/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */"
>> This comment is not introduced in this patch.
>> I followed the naming style in this file and add clk_regmap_divider_with_init_ops.
>>
>> @Jerome， What's your suggestion about this？
> 
> Yes you don't need to fix anything in this series. Just saying that in
> the future we should work on cleaning this up.
> 
OK. Thank you!
> .
>

Jianxin Pan Oct. 24, 2018, 6:29 a.m. UTC | #5

On 2018/10/19 1:13, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-10-17 22:07:25)
[...]
>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>> new file mode 100644
>> index 0000000..5555e3f
>> --- /dev/null
>> +++ b/drivers/clk/meson/mmc-clkc.c
>> @@ -0,0 +1,296 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson MMC Sub Clock Controller Driver
>> + *
>> + * Copyright (c) 2017 Baylibre SAS.
>> + * Author: Jerome Brunet <[e-mail]>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <[e-mail]>
>> + */
>> +
>> +#include <linux/clk.h>
> 
> clk-provider.h instead of clk.h?>
Maybe we need to keep clk.h
devm_clk_get() is called in mmc_clkc_register_mux() to get parent in from DTS.
I'm sorry to miss this in previous reply.
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/platform_device.h>
[...]> 
> .
>

Stephen Boyd Oct. 24, 2018, 8:47 a.m. UTC | #6

Quoting Jianxin Pan (2018-10-23 23:29:24)
> On 2018/10/19 1:13, Stephen Boyd wrote:
> > Quoting Jianxin Pan (2018-10-17 22:07:25)
> [...]
> >> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> >> new file mode 100644
> >> index 0000000..5555e3f
> >> --- /dev/null
> >> +++ b/drivers/clk/meson/mmc-clkc.c
> >> @@ -0,0 +1,296 @@
> >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >> +/*
> >> + * Amlogic Meson MMC Sub Clock Controller Driver
> >> + *
> >> + * Copyright (c) 2017 Baylibre SAS.
> >> + * Author: Jerome Brunet <[e-mail]>
> >> + *
> >> + * Copyright (c) 2018 Amlogic, inc.
> >> + * Author: Yixun Lan <[e-mail]>
> >> + */
> >> +
> >> +#include <linux/clk.h>
> > 
> > clk-provider.h instead of clk.h?>
> Maybe we need to keep clk.h
> devm_clk_get() is called in mmc_clkc_register_mux() to get parent in from DTS.
> I'm sorry to miss this in previous reply.

OK. But also include clk-provider.h if provider APIs are being used
here.

Jianxin Pan Oct. 24, 2018, 8:51 a.m. UTC | #7

On 2018/10/24 16:47, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-10-23 23:29:24)
>> On 2018/10/19 1:13, Stephen Boyd wrote:
>>> Quoting Jianxin Pan (2018-10-17 22:07:25)
>> [...]
>>>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>>>> new file mode 100644
>>>> index 0000000..5555e3f
>>>> --- /dev/null
>>>> +++ b/drivers/clk/meson/mmc-clkc.c
>>>> @@ -0,0 +1,296 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>>> +/*
>>>> + * Amlogic Meson MMC Sub Clock Controller Driver
>>>> + *
>>>> + * Copyright (c) 2017 Baylibre SAS.
>>>> + * Author: Jerome Brunet <[e-mail]>
>>>> + *
>>>> + * Copyright (c) 2018 Amlogic, inc.
>>>> + * Author: Yixun Lan <[e-mail]>
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>
>>> clk-provider.h instead of clk.h?>
>> Maybe we need to keep clk.h
>> devm_clk_get() is called in mmc_clkc_register_mux() to get parent in from DTS.
>> I'm sorry to miss this in previous reply.
> 
> OK. But also include clk-provider.h if provider APIs are being used
> here.
OK. I will add it. Thank you.
> 
> .
>

Jerome Brunet Oct. 24, 2018, 9 a.m. UTC | #8

On Fri, 2018-10-19 at 11:03 -0700, Stephen Boyd wrote:
> Quoting Jianxin Pan (2018-10-19 09:12:53)
> > On 2018/10/19 1:13, Stephen Boyd wrote:
> > > Quoting Jianxin Pan (2018-10-17 22:07:25)
> > > > diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
> > > > index 305ee30..f96314d 100644
> > > > --- a/drivers/clk/meson/clk-regmap.c
> > > > +++ b/drivers/clk/meson/clk-regmap.c
> > > > @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
> > > >                                   clk_div_mask(div->width) << div->shift, val);
> > > >  };
> > > >  
> > > > -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> > > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > > +{
> > > > +       struct clk_regmap *clk = to_clk_regmap(hw);
> > > > +       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> > > > +       unsigned int val;
> > > > +       int ret;
> > > > +
> > > > +       ret = regmap_read(clk->map, div->offset, &val);
> > > > +       if (ret)
> > > > +               return;
> > > >  
> > > > +       val &= (clk_div_mask(div->width) << div->shift);
> > > > +       if (!val)
> > > > +               regmap_update_bits(clk->map, div->offset,
> > > > +                                  clk_div_mask(div->width) << div->shift,
> > > > +                                  clk_div_mask(div->width));
> > > > +}
> > > > +
> > > > +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> > > 
> > > We should add a patch to rename the symbol for qcom, i.e.
> > > qcom_clk_regmap_div_ro_ops, and then any symbols in this directory
> > > should be meson_clk_regmap_div_ro_ops.
> > 
> > "/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */"
> > This comment is not introduced in this patch.
> > I followed the naming style in this file and add clk_regmap_divider_with_init_ops.
> > 
> > @Jerome， What's your suggestion about this？
> 
> Yes you don't need to fix anything in this series. Just saying that in
> the future we should work on cleaning this up.


Well, first, I wonder why such a change ends up in a patch that is supposed to
add a controller.

If such a change was really required to implement a generic div (which I doubt)
it would need to be in separate with clear explanation.

Stephen,
I agree at some point we should squash the different regmap implementations and
provide generic (enough) implementation. There is not only qcom and meson, some
other controllers are redefining regmap ops and I bet driver outside of
drivers/clk/* could use a generic implementation as well.

Jerome Brunet Oct. 24, 2018, 9:01 a.m. UTC | #9

On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan wrote:
> From: Yixun Lan <[e-mail]>
> 
> The patch will add a MMC clock controller driver which used by MMC or NAND,
> It provide a mux and divider clock, and three phase clocks - core, tx, tx.
> 
> Two clocks are provided as the parent of MMC clock controller from
> upper layer clock controller - eg "amlogic,axg-clkc" in AXG platform.
> 
> To specify which clock the MMC or NAND driver may consume,
> the preprocessor macros in the dt-bindings/clock/amlogic,mmc-clkc.h header
> can be used in the device tree sources.
> 
> Signed-off-by: Yixun Lan <[e-mail]>
> Signed-off-by: Jianxin Pan <[e-mail]>
> ---
>  drivers/clk/meson/Kconfig      |  10 ++
>  drivers/clk/meson/Makefile     |   1 +
>  drivers/clk/meson/clk-regmap.c |  27 +++-
>  drivers/clk/meson/clk-regmap.h |   1 +
>  drivers/clk/meson/mmc-clkc.c   | 296 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 334 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/meson/mmc-clkc.c
> 
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index efaa70f..8b8ccbc 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -15,6 +15,16 @@ config COMMON_CLK_MESON_AO
>  	select COMMON_CLK_REGMAP_MESON
>  	select RESET_CONTROLLER
>  
> +config COMMON_CLK_MMC_MESON
> +	tristate "Meson MMC Sub Clock Controller Driver"
> +	depends on COMMON_CLK_AMLOGIC

COMMON_CLK_AMLOGIC is not something that is manually enabled
You should select it, not depends on it. Have a look at how it is done for the
other controller (like in v4.19)

> +	select MFD_SYSCON
> +	select REGMAP

this is already selected by COMMON_CLK_AMLOGIC, please drop this.

> +	help
> +	  Support for the MMC sub clock controller on Amlogic Meson Platform,
> +	  which include S905 (GXBB, GXL), A113D/X (AXG) devices.
> +	  Say Y if you want this clock enabled.
> +
>  config COMMON_CLK_REGMAP_MESON
>  	bool
>  	select REGMAP
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 39ce566..31c16d5 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -9,4 +9,5 @@ obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>  obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
>  obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)	+= axg-audio.o
> +obj-$(CONFIG_COMMON_CLK_MMC_MESON) 	+= mmc-clkc.o
>  obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
> diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
> index 305ee30..f96314d 100644
> --- a/drivers/clk/meson/clk-regmap.c
> +++ b/drivers/clk/meson/clk-regmap.c
> @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>  				  clk_div_mask(div->width) << div->shift, val);
>  };
>  
> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> +static void clk_regmap_div_init(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(clk->map, div->offset, &val);
> +	if (ret)
> +		return;
>  
> +	val &= (clk_div_mask(div->width) << div->shift);
> +	if (!val)
> +		regmap_update_bits(clk->map, div->offset,
> +				   clk_div_mask(div->width) << div->shift,
> +				   clk_div_mask(div->width));

This is wrong for several reasons:
* You should hard code the initial value in the driver.
* If shift is not 0, I doubt this will give the expected result.

> +}

Please drop this. 

> +
> +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>  const struct clk_ops clk_regmap_divider_ops = {
>  	.recalc_rate = clk_regmap_div_recalc_rate,
>  	.round_rate = clk_regmap_div_round_rate,
> @@ -122,6 +139,14 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>  };
>  EXPORT_SYMBOL_GPL(clk_regmap_divider_ops);
>  
> +const struct clk_ops clk_regmap_divider_with_init_ops = {
> +	.recalc_rate = clk_regmap_div_recalc_rate,
> +	.round_rate = clk_regmap_div_round_rate,
> +	.set_rate = clk_regmap_div_set_rate,
> +	.init = clk_regmap_div_init,
> +};
> +EXPORT_SYMBOL_GPL(clk_regmap_divider_with_init_ops);

... and this.

> +
>  const struct clk_ops clk_regmap_divider_ro_ops = {
>  	.recalc_rate = clk_regmap_div_recalc_rate,
>  	.round_rate = clk_regmap_div_round_rate,
> diff --git a/drivers/clk/meson/clk-regmap.h b/drivers/clk/meson/clk-regmap.h
> index ed2d434..0ab7d37 100644
> --- a/drivers/clk/meson/clk-regmap.h
> +++ b/drivers/clk/meson/clk-regmap.h
> @@ -109,5 +109,6 @@ struct clk_regmap_mux_data {
>  
>  extern const struct clk_ops clk_regmap_mux_ops;
>  extern const struct clk_ops clk_regmap_mux_ro_ops;
> +extern const struct clk_ops clk_regmap_divider_with_init_ops;

... and this as well.

There is no reason to to init the divider to something other than what it is
already when we register the clock controller.

If your consumer needs the clocks to be initialised, it should call
clk_set_rate()

>  
>  #endif /* __CLK_REGMAP_H */
> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> new file mode 100644
> index 0000000..5555e3f
> --- /dev/null
> +++ b/drivers/clk/meson/mmc-clkc.c
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson MMC Sub Clock Controller Driver
> + *
> + * Copyright (c) 2017 Baylibre SAS.
> + * Author: Jerome Brunet <[e-mail]>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <[e-mail]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/clock/amlogic,mmc-clkc.h>
> +
> +#include "clkc.h"
> +
> +/* clock ID used by internal driver */
> +#define CLKID_MMC_MUX			0
> +
> +#define   SD_EMMC_CLOCK		0
> +#define   CLK_DELAY_STEP_PS		200
> +#define   CLK_PHASE_STEP		30
> +#define   CLK_PHASE_POINT_NUM		(360 / CLK_PHASE_STEP)
> +
> +#define MUX_CLK_NUM_PARENTS		2
> +#define MMC_MAX_CLKS			5
> +
> +struct mmc_clkc_data {
> +	struct meson_clk_phase_delay_data	tx;
> +	struct meson_clk_phase_delay_data	rx;
> +};
> +
> +static struct clk_regmap_mux_data mmc_clkc_mux_data = {
> +	.offset		= SD_EMMC_CLOCK,
> +	.mask		= 0x3,
> +	.shift		= 6,
> +	.flags		= CLK_DIVIDER_ROUND_CLOSEST,
> +};
> +
> +static struct clk_regmap_div_data mmc_clkc_div_data = {
> +	.offset		= SD_EMMC_CLOCK,
> +	.shift		= 0,
> +	.width		= 6,
> +	.flags		= CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
> +};
> +
> +static struct meson_clk_phase_data mmc_clkc_core_phase = {
> +	.ph = {
> +		.reg_off	= SD_EMMC_CLOCK,
> +		.shift	= 8,
> +		.width	= 2,
> +	}
> +};
> +
> +static const struct mmc_clkc_data mmc_clkc_gx_data = {
> +	.tx = {
> +		.phase = {
> +			.reg_off	= SD_EMMC_CLOCK,
> +			.shift	= 10,
> +			.width	= 2,
> +		},
> +		.delay = {
> +			.reg_off	= SD_EMMC_CLOCK,
> +			.shift	= 16,
> +			.width	= 4,
> +		},
> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
> +	},
> +	.rx = {
> +		.phase = {
> +			.reg_off	= SD_EMMC_CLOCK,
> +			.shift	= 12,
> +			.width	= 2,
> +		},
> +		.delay = {
> +			.reg_off	= SD_EMMC_CLOCK,
> +			.shift	= 20,
> +			.width	= 4,
> +		},
> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
> +	},
> +};
> +
> +static const struct mmc_clkc_data mmc_clkc_axg_data = {
> +	.tx = {
> +		.phase = {
> +			.reg_off	= SD_EMMC_CLOCK,
> +			.shift	= 10,
> +			.width	= 2,
> +		},
> +		.delay = {
> +			.reg_off	= SD_EMMC_CLOCK,
> +			.shift	= 16,
> +			.width	= 6,
> +		},
> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
> +	},
> +	.rx = {
> +		.phase = {
> +			.reg_off	= SD_EMMC_CLOCK,
> +			.shift	= 12,
> +			.width	= 2,
> +		},
> +		.delay = {
> +			.reg_off	= SD_EMMC_CLOCK,
> +			.shift	= 22,
> +			.width	= 6,
> +		},
> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
> +	},
> +};
> +
> +static const struct of_device_id mmc_clkc_match_table[] = {
> +	{
> +		.compatible	= "amlogic,gx-mmc-clkc",
> +		.data		= &mmc_clkc_gx_data
> +	},
> +	{
> +		.compatible	= "amlogic,axg-mmc-clkc",
> +		.data		= &mmc_clkc_axg_data
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mmc_clkc_match_table);
> +
> +static struct clk_regmap *
> +mmc_clkc_register_clk(struct device *dev, struct regmap *map,
> +		      struct clk_init_data *init,
> +		      const char *suffix, void *data)
> +{
> +	struct clk_regmap *clk;
> +	char *name;
> +	int ret;
> +
> +	clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
> +	if (!clk)
> +		return ERR_PTR(-ENOMEM);
> +
> +	name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), suffix);
> +	if (!name)
> +		return ERR_PTR(-ENOMEM);
> +
> +	init->name = name;
> +
> +	clk->map = map;
> +	clk->data = data;
> +	clk->hw.init = init;
> +
> +	ret = devm_clk_hw_register(dev, &clk->hw);
> +	if (ret)
> +		clk = ERR_PTR(ret);
> +
> +	kfree(name);
> +	return clk;
> +}
> +
> +static struct clk_regmap *mmc_clkc_register_mux(struct device *dev,
> +						struct regmap *map)
> +{
> +	const char *parent_names[MUX_CLK_NUM_PARENTS];
> +	struct clk_init_data init;
> +	struct clk_regmap *mux;
> +	struct clk *clk;
> +	int i;
> +
> +	for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
> +		char name[8];
> +
> +		snprintf(name, sizeof(name), "clkin%d", i);
> +		clk = devm_clk_get(dev, name);
> +		if (IS_ERR(clk)) {
> +			if (clk != ERR_PTR(-EPROBE_DEFER))
> +				dev_err(dev, "Missing clock %s\n", name);
> +			return ERR_PTR((long)clk);
> +		}
> +
> +		parent_names[i] = __clk_get_name(clk);
> +	}
> +
> +	init.ops = &clk_regmap_mux_ops;
> +	init.flags = CLK_SET_RATE_PARENT;
> +	init.parent_names = parent_names;
> +	init.num_parents = MUX_CLK_NUM_PARENTS;
> +
> +	mux = mmc_clkc_register_clk(dev, map, &init, "mux", &mmc_clkc_mux_data);
> +	if (IS_ERR(mux))
> +		dev_err(dev, "Mux clock registration failed\n");
> +
> +	return mux;
> +}
> +
> +static struct clk_regmap *
> +mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
> +				  char *suffix, const char *parent,

I would prefer if you passed the parent clk_hw pointer and call
clk_hw_get_name() in here

> +				  unsigned long flags,
> +				  const struct clk_ops *ops, void *data)
> +{
> +	struct clk_init_data init;
> +	struct clk_regmap *clk;
> +
> +	init.ops = ops;
> +	init.flags = flags;
> +	init.parent_names = (const char* const []){ parent, };
> +	init.num_parents = 1;
> +
> +	clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
> +	if (IS_ERR(clk))
> +		dev_err(dev, "Core %s clock registration failed\n", suffix);
> +
> +	return clk;
> +}
> +
> +static int mmc_clkc_probe(struct platform_device *pdev)
> +{
> +	struct clk_hw_onecell_data *onecell_data;
> +	struct device *dev = &pdev->dev;
> +	struct mmc_clkc_data *data;
> +	struct regmap *map;
> +	struct clk_regmap *mux, *div, *core, *rx, *tx;

You really don't need all these local variables ( I think I already pointed this
out in past reviews ...)

You can keep one struct clk_regmap *clk and store the clk->hw in the onecell
data right after registering the clock.


> +
> +	data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
> +	if (!data)
> +		return -ENODEV;
> +
> +	map = syscon_node_to_regmap(dev->of_node);
> +	if (IS_ERR(map)) {
> +		dev_err(dev, "could not find mmc clock controller\n");
> +		return PTR_ERR(map);
> +	}
> +
> +	onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
> +				    sizeof(*onecell_data->hws) * MMC_MAX_CLKS,
> +				    GFP_KERNEL);
> +	if (!onecell_data)
> +		return -ENOMEM;
> +
> +	mux = mmc_clkc_register_mux(dev, map);
> +	if (IS_ERR(mux))
> +		return PTR_ERR(mux);
> +
> +	div = mmc_clkc_register_clk_with_parent(dev, map, "div",
> +						clk_hw_get_name(&mux->hw),
> +						CLK_SET_RATE_PARENT,
> +						&clk_regmap_divider_with_init_ops,
> +						&mmc_clkc_div_data);
> +	if (IS_ERR(div))
> +		return PTR_ERR(div);
> +
> +	core = mmc_clkc_register_clk_with_parent(dev, map, "core",
> +						 clk_hw_get_name(&div->hw),
> +						 CLK_SET_RATE_PARENT,
> +						 &meson_clk_phase_ops,
> +						 &mmc_clkc_core_phase);
> +	if (IS_ERR(core))
> +		return PTR_ERR(core);
> +
> +	rx = mmc_clkc_register_clk_with_parent(dev, map, "rx",
> +					       clk_hw_get_name(&core->hw),  0,
> +					       &meson_clk_phase_delay_ops,
> +					       &data->rx);
> +	if (IS_ERR(rx))
> +		return PTR_ERR(rx);
> +
> +	tx = mmc_clkc_register_clk_with_parent(dev, map, "tx",
> +					       clk_hw_get_name(&core->hw),  0,
> +					       &meson_clk_phase_delay_ops,
> +					       &data->tx);
> +	if (IS_ERR(tx))
> +		return PTR_ERR(tx);
> +
> +	onecell_data->hws[CLKID_MMC_MUX]		= &mux->hw,
> +	onecell_data->hws[CLKID_MMC_DIV]		= &div->hw,
> +	onecell_data->hws[CLKID_MMC_PHASE_CORE]		= &core->hw,
> +	onecell_data->hws[CLKID_MMC_PHASE_RX]		= &rx->hw,
> +	onecell_data->hws[CLKID_MMC_PHASE_TX]		= &tx->hw,
> +	onecell_data->num				= MMC_MAX_CLKS;
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +					   onecell_data);
> +}
> +
> +static struct platform_driver mmc_clkc_driver = {
> +	.probe		= mmc_clkc_probe,
> +	.driver		= {
> +		.name	= "meson-mmc-clkc",
> +		.of_match_table = of_match_ptr(mmc_clkc_match_table),
> +	},
> +};
> +
> +module_platform_driver(mmc_clkc_driver);

This can be compiled as module so please append the proper:
MODULE_DESCRIPTION()
MODULE_AUTHOR()
MODULE_LICENSE()

Jianxin Pan Oct. 25, 2018, 11:48 a.m. UTC | #10

Hi Jerome,

On 2018/10/24 17:01, Jerome Brunet wrote:
> On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan wrote:
>> From: Yixun Lan <[e-mail]>
>>
>> The patch will add a MMC clock controller driver which used by MMC or NAND,
>> It provide a mux and divider clock, and three phase clocks - core, tx, tx.
>>
[...]
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index efaa70f..8b8ccbc 100644
>> --- a/drivers/clk/meson/Kconfig
>> +++ b/drivers/clk/meson/Kconfig
>> @@ -15,6 +15,16 @@ config COMMON_CLK_MESON_AO
>>  	select COMMON_CLK_REGMAP_MESON
>>  	select RESET_CONTROLLER
>>  
>> +config COMMON_CLK_MMC_MESON
>> +	tristate "Meson MMC Sub Clock Controller Driver"
>> +	depends on COMMON_CLK_AMLOGIC
> 
> COMMON_CLK_AMLOGIC is not something that is manually enabled
> You should select it, not depends on it. Have a look at how it is done for the
> other controller (like in v4.19)
OK. I will fix it.
> 
>> +	select MFD_SYSCON
>> +	select REGMAP
> 
> this is already selected by COMMON_CLK_AMLOGIC, please drop this.
OK.
> 
>> +	help
>> +	  Support for the MMC sub clock controller on Amlogic Meson Platform,
>> +	  which include S905 (GXBB, GXL), A113D/X (AXG) devices.
>> +	  Say Y if you want this clock enabled.
>> +
>>  config COMMON_CLK_REGMAP_MESON
>>  	bool
>>  	select REGMAP
>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> index 39ce566..31c16d5 100644
>> --- a/drivers/clk/meson/Makefile
>> +++ b/drivers/clk/meson/Makefile
>> @@ -9,4 +9,5 @@ obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>>  obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
>>  obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
>>  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)	+= axg-audio.o
>> +obj-$(CONFIG_COMMON_CLK_MMC_MESON) 	+= mmc-clkc.o
>>  obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
>> diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
>> index 305ee30..f96314d 100644
>> --- a/drivers/clk/meson/clk-regmap.c
>> +++ b/drivers/clk/meson/clk-regmap.c
>> @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>  				  clk_div_mask(div->width) << div->shift, val);
>>  };
>>  
>> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>> +static void clk_regmap_div_init(struct clk_hw *hw)
>> +{
>> +	struct clk_regmap *clk = to_clk_regmap(hw);
>> +	struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	ret = regmap_read(clk->map, div->offset, &val);
>> +	if (ret)
>> +		return;
>>  
>> +	val &= (clk_div_mask(div->width) << div->shift);
>> +	if (!val)
>> +		regmap_update_bits(clk->map, div->offset,
>> +				   clk_div_mask(div->width) << div->shift,
>> +				   clk_div_mask(div->width));
> 
> This is wrong for several reasons:
> * You should hard code the initial value in the driver.
> * If shift is not 0, I doubt this will give the expected result.
The value 0x00 of divider means nand clock off, then read/write nand register is forbidden.
Should we set the initial value in nand driver, or in sub emmc clk driver?  
> 
>> +}
> 
> Please drop this. 
> 
>> +
>> +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>>  const struct clk_ops clk_regmap_divider_ops = {
>>  	.recalc_rate = clk_regmap_div_recalc_rate,
>>  	.round_rate = clk_regmap_div_round_rate,
>> @@ -122,6 +139,14 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>  };
>>  EXPORT_SYMBOL_GPL(clk_regmap_divider_ops);
>>  
>> +const struct clk_ops clk_regmap_divider_with_init_ops = {
>> +	.recalc_rate = clk_regmap_div_recalc_rate,
>> +	.round_rate = clk_regmap_div_round_rate,
>> +	.set_rate = clk_regmap_div_set_rate,
>> +	.init = clk_regmap_div_init,
>> +};
>> +EXPORT_SYMBOL_GPL(clk_regmap_divider_with_init_ops);
> 
> ... and this.
> 
>> +
>>  const struct clk_ops clk_regmap_divider_ro_ops = {
>>  	.recalc_rate = clk_regmap_div_recalc_rate,
>>  	.round_rate = clk_regmap_div_round_rate,
>> diff --git a/drivers/clk/meson/clk-regmap.h b/drivers/clk/meson/clk-regmap.h
>> index ed2d434..0ab7d37 100644
>> --- a/drivers/clk/meson/clk-regmap.h
>> +++ b/drivers/clk/meson/clk-regmap.h
>> @@ -109,5 +109,6 @@ struct clk_regmap_mux_data {
>>  
>>  extern const struct clk_ops clk_regmap_mux_ops;
>>  extern const struct clk_ops clk_regmap_mux_ro_ops;
>> +extern const struct clk_ops clk_regmap_divider_with_init_ops;
> 
> ... and this as well.
> 
> There is no reason to to init the divider to something other than what it is
> already when we register the clock controller.
> 
> If your consumer needs the clocks to be initialised, it should call
> clk_set_rate()
> 
>>  
>>  #endif /* __CLK_REGMAP_H */
>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>> new file mode 100644
>> index 0000000..5555e3f
>> --- /dev/null
>> +++ b/drivers/clk/meson/mmc-clkc.c
>> @@ -0,0 +1,296 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson MMC Sub Clock Controller Driver
>> + *
>> + * Copyright (c) 2017 Baylibre SAS.
>> + * Author: Jerome Brunet <[e-mail]>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <[e-mail]>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/platform_device.h>
>> +#include <dt-bindings/clock/amlogic,mmc-clkc.h>
>> +
>> +#include "clkc.h"
>> +
>> +/* clock ID used by internal driver */
>> +#define CLKID_MMC_MUX			0
>> +
>> +#define   SD_EMMC_CLOCK		0
>> +#define   CLK_DELAY_STEP_PS		200
>> +#define   CLK_PHASE_STEP		30
>> +#define   CLK_PHASE_POINT_NUM		(360 / CLK_PHASE_STEP)
>> +
>> +#define MUX_CLK_NUM_PARENTS		2
>> +#define MMC_MAX_CLKS			5
>> +
>> +struct mmc_clkc_data {
>> +	struct meson_clk_phase_delay_data	tx;
>> +	struct meson_clk_phase_delay_data	rx;
>> +};
>> +
>> +static struct clk_regmap_mux_data mmc_clkc_mux_data = {
>> +	.offset		= SD_EMMC_CLOCK,
>> +	.mask		= 0x3,
>> +	.shift		= 6,
>> +	.flags		= CLK_DIVIDER_ROUND_CLOSEST,
>> +};
>> +
>> +static struct clk_regmap_div_data mmc_clkc_div_data = {
>> +	.offset		= SD_EMMC_CLOCK,
>> +	.shift		= 0,
>> +	.width		= 6,
>> +	.flags		= CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
>> +};
>> +
>> +static struct meson_clk_phase_data mmc_clkc_core_phase = {
>> +	.ph = {
>> +		.reg_off	= SD_EMMC_CLOCK,
>> +		.shift	= 8,
>> +		.width	= 2,
>> +	}
>> +};
>> +
>> +static const struct mmc_clkc_data mmc_clkc_gx_data = {
>> +	.tx = {
>> +		.phase = {
>> +			.reg_off	= SD_EMMC_CLOCK,
>> +			.shift	= 10,
>> +			.width	= 2,
>> +		},
>> +		.delay = {
>> +			.reg_off	= SD_EMMC_CLOCK,
>> +			.shift	= 16,
>> +			.width	= 4,
>> +		},
>> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
>> +	},
>> +	.rx = {
>> +		.phase = {
>> +			.reg_off	= SD_EMMC_CLOCK,
>> +			.shift	= 12,
>> +			.width	= 2,
>> +		},
>> +		.delay = {
>> +			.reg_off	= SD_EMMC_CLOCK,
>> +			.shift	= 20,
>> +			.width	= 4,
>> +		},
>> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
>> +	},
>> +};
>> +
>> +static const struct mmc_clkc_data mmc_clkc_axg_data = {
>> +	.tx = {
>> +		.phase = {
>> +			.reg_off	= SD_EMMC_CLOCK,
>> +			.shift	= 10,
>> +			.width	= 2,
>> +		},
>> +		.delay = {
>> +			.reg_off	= SD_EMMC_CLOCK,
>> +			.shift	= 16,
>> +			.width	= 6,
>> +		},
>> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
>> +	},
>> +	.rx = {
>> +		.phase = {
>> +			.reg_off	= SD_EMMC_CLOCK,
>> +			.shift	= 12,
>> +			.width	= 2,
>> +		},
>> +		.delay = {
>> +			.reg_off	= SD_EMMC_CLOCK,
>> +			.shift	= 22,
>> +			.width	= 6,
>> +		},
>> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
>> +	},
>> +};
>> +
>> +static const struct of_device_id mmc_clkc_match_table[] = {
>> +	{
>> +		.compatible	= "amlogic,gx-mmc-clkc",
>> +		.data		= &mmc_clkc_gx_data
>> +	},
>> +	{
>> +		.compatible	= "amlogic,axg-mmc-clkc",
>> +		.data		= &mmc_clkc_axg_data
>> +	},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, mmc_clkc_match_table);
>> +
>> +static struct clk_regmap *
>> +mmc_clkc_register_clk(struct device *dev, struct regmap *map,
>> +		      struct clk_init_data *init,
>> +		      const char *suffix, void *data)
>> +{
>> +	struct clk_regmap *clk;
>> +	char *name;
>> +	int ret;
>> +
>> +	clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
>> +	if (!clk)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), suffix);
>> +	if (!name)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	init->name = name;
>> +
>> +	clk->map = map;
>> +	clk->data = data;
>> +	clk->hw.init = init;
>> +
>> +	ret = devm_clk_hw_register(dev, &clk->hw);
>> +	if (ret)
>> +		clk = ERR_PTR(ret);
>> +
>> +	kfree(name);
>> +	return clk;
>> +}
>> +
>> +static struct clk_regmap *mmc_clkc_register_mux(struct device *dev,
>> +						struct regmap *map)
>> +{
>> +	const char *parent_names[MUX_CLK_NUM_PARENTS];
>> +	struct clk_init_data init;
>> +	struct clk_regmap *mux;
>> +	struct clk *clk;
>> +	int i;
>> +
>> +	for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
>> +		char name[8];
>> +
>> +		snprintf(name, sizeof(name), "clkin%d", i);
>> +		clk = devm_clk_get(dev, name);
>> +		if (IS_ERR(clk)) {
>> +			if (clk != ERR_PTR(-EPROBE_DEFER))
>> +				dev_err(dev, "Missing clock %s\n", name);
>> +			return ERR_PTR((long)clk);
>> +		}
>> +
>> +		parent_names[i] = __clk_get_name(clk);
>> +	}
>> +
>> +	init.ops = &clk_regmap_mux_ops;
>> +	init.flags = CLK_SET_RATE_PARENT;
>> +	init.parent_names = parent_names;
>> +	init.num_parents = MUX_CLK_NUM_PARENTS;
>> +
>> +	mux = mmc_clkc_register_clk(dev, map, &init, "mux", &mmc_clkc_mux_data);
>> +	if (IS_ERR(mux))
>> +		dev_err(dev, "Mux clock registration failed\n");
>> +
>> +	return mux;
>> +}
>> +
>> +static struct clk_regmap *
>> +mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
>> +				  char *suffix, const char *parent,
> 
> I would prefer if you passed the parent clk_hw pointer and call
> clk_hw_get_name() in here
OK. I will fix it. Thanks for your review.
> 
>> +				  unsigned long flags,
>> +				  const struct clk_ops *ops, void *data)
>> +{
>> +	struct clk_init_data init;
>> +	struct clk_regmap *clk;
>> +
>> +	init.ops = ops;
>> +	init.flags = flags;
>> +	init.parent_names = (const char* const []){ parent, };
>> +	init.num_parents = 1;
>> +
>> +	clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
>> +	if (IS_ERR(clk))
>> +		dev_err(dev, "Core %s clock registration failed\n", suffix);
>> +
>> +	return clk;
>> +}
>> +
>> +static int mmc_clkc_probe(struct platform_device *pdev)
>> +{
>> +	struct clk_hw_onecell_data *onecell_data;
>> +	struct device *dev = &pdev->dev;
>> +	struct mmc_clkc_data *data;
>> +	struct regmap *map;
>> +	struct clk_regmap *mux, *div, *core, *rx, *tx;
> 
> You really don't need all these local variables ( I think I already pointed this
> out in past reviews ...)
> 
> You can keep one struct clk_regmap *clk and store the clk->hw in the onecell
> data right after registering the clock.
OK, I will remove them.
> 
> 
>> +
>> +	data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
>> +	if (!data)
>> +		return -ENODEV;
>> +
>> +	map = syscon_node_to_regmap(dev->of_node);
>> +	if (IS_ERR(map)) {
>> +		dev_err(dev, "could not find mmc clock controller\n");
>> +		return PTR_ERR(map);
>> +	}
>> +
>> +	onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
>> +				    sizeof(*onecell_data->hws) * MMC_MAX_CLKS,
>> +				    GFP_KERNEL);
>> +	if (!onecell_data)
>> +		return -ENOMEM;
>> +
>> +	mux = mmc_clkc_register_mux(dev, map);
>> +	if (IS_ERR(mux))
>> +		return PTR_ERR(mux);
>> +
>> +	div = mmc_clkc_register_clk_with_parent(dev, map, "div",
>> +						clk_hw_get_name(&mux->hw),
>> +						CLK_SET_RATE_PARENT,
>> +						&clk_regmap_divider_with_init_ops,
>> +						&mmc_clkc_div_data);
>> +	if (IS_ERR(div))
>> +		return PTR_ERR(div);
>> +
>> +	core = mmc_clkc_register_clk_with_parent(dev, map, "core",
>> +						 clk_hw_get_name(&div->hw),
>> +						 CLK_SET_RATE_PARENT,
>> +						 &meson_clk_phase_ops,
>> +						 &mmc_clkc_core_phase);
>> +	if (IS_ERR(core))
>> +		return PTR_ERR(core);
>> +
>> +	rx = mmc_clkc_register_clk_with_parent(dev, map, "rx",
>> +					       clk_hw_get_name(&core->hw),  0,
>> +					       &meson_clk_phase_delay_ops,
>> +					       &data->rx);
>> +	if (IS_ERR(rx))
>> +		return PTR_ERR(rx);
>> +
>> +	tx = mmc_clkc_register_clk_with_parent(dev, map, "tx",
>> +					       clk_hw_get_name(&core->hw),  0,
>> +					       &meson_clk_phase_delay_ops,
>> +					       &data->tx);
>> +	if (IS_ERR(tx))
>> +		return PTR_ERR(tx);
>> +
>> +	onecell_data->hws[CLKID_MMC_MUX]		= &mux->hw,
>> +	onecell_data->hws[CLKID_MMC_DIV]		= &div->hw,
>> +	onecell_data->hws[CLKID_MMC_PHASE_CORE]		= &core->hw,
>> +	onecell_data->hws[CLKID_MMC_PHASE_RX]		= &rx->hw,
>> +	onecell_data->hws[CLKID_MMC_PHASE_TX]		= &tx->hw,
>> +	onecell_data->num				= MMC_MAX_CLKS;
>> +
>> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>> +					   onecell_data);
>> +}
>> +
>> +static struct platform_driver mmc_clkc_driver = {
>> +	.probe		= mmc_clkc_probe,
>> +	.driver		= {
>> +		.name	= "meson-mmc-clkc",
>> +		.of_match_table = of_match_ptr(mmc_clkc_match_table),
>> +	},
>> +};
>> +
>> +module_platform_driver(mmc_clkc_driver);
> 
> This can be compiled as module so please append the proper:
> MODULE_DESCRIPTION()
> MODULE_AUTHOR()
> MODULE_LICENSE()
OK. I will fix it in the next version.
> 
> .
>

Jerome Brunet Oct. 25, 2018, 12:54 p.m. UTC | #11

On Thu, 2018-10-25 at 19:48 +0800, Jianxin Pan wrote:
> Hi Jerome,
> 
> On 2018/10/24 17:01, Jerome Brunet wrote:
> > On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan wrote:
> > > From: Yixun Lan <[e-mail]>
> > > 
> > > The patch will add a MMC clock controller driver which used by MMC or NAND,
> > > It provide a mux and divider clock, and three phase clocks - core, tx, tx.
> > > 
> 
> [...]
> > > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> > > index efaa70f..8b8ccbc 100644
> > > --- a/drivers/clk/meson/Kconfig
> > > +++ b/drivers/clk/meson/Kconfig
> > > @@ -15,6 +15,16 @@ config COMMON_CLK_MESON_AO
> > >  	select COMMON_CLK_REGMAP_MESON
> > >  	select RESET_CONTROLLER
> > >  
> > > +config COMMON_CLK_MMC_MESON
> > > +	tristate "Meson MMC Sub Clock Controller Driver"
> > > +	depends on COMMON_CLK_AMLOGIC
> > 
> > COMMON_CLK_AMLOGIC is not something that is manually enabled
> > You should select it, not depends on it. Have a look at how it is done for the
> > other controller (like in v4.19)
> 
> OK. I will fix it.
> > 
> > > +	select MFD_SYSCON
> > > +	select REGMAP
> > 
> > this is already selected by COMMON_CLK_AMLOGIC, please drop this.
> 
> OK.
> > 
> > > +	help
> > > +	  Support for the MMC sub clock controller on Amlogic Meson Platform,
> > > +	  which include S905 (GXBB, GXL), A113D/X (AXG) devices.
> > > +	  Say Y if you want this clock enabled.
> > > +
> > >  config COMMON_CLK_REGMAP_MESON
> > >  	bool
> > >  	select REGMAP
> > > diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> > > index 39ce566..31c16d5 100644
> > > --- a/drivers/clk/meson/Makefile
> > > +++ b/drivers/clk/meson/Makefile
> > > @@ -9,4 +9,5 @@ obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
> > >  obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
> > >  obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
> > >  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)	+= axg-audio.o
> > > +obj-$(CONFIG_COMMON_CLK_MMC_MESON) 	+= mmc-clkc.o
> > >  obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
> > > diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
> > > index 305ee30..f96314d 100644
> > > --- a/drivers/clk/meson/clk-regmap.c
> > > +++ b/drivers/clk/meson/clk-regmap.c
> > > @@ -113,8 +113,25 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
> > >  				  clk_div_mask(div->width) << div->shift, val);
> > >  };
> > >  
> > > -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > +{
> > > +	struct clk_regmap *clk = to_clk_regmap(hw);
> > > +	struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> > > +	unsigned int val;
> > > +	int ret;
> > > +
> > > +	ret = regmap_read(clk->map, div->offset, &val);
> > > +	if (ret)
> > > +		return;
> > >  
> > > +	val &= (clk_div_mask(div->width) << div->shift);
> > > +	if (!val)
> > > +		regmap_update_bits(clk->map, div->offset,
> > > +				   clk_div_mask(div->width) << div->shift,
> > > +				   clk_div_mask(div->width));
> > 
> > This is wrong for several reasons:
> > * You should hard code the initial value in the driver.
> > * If shift is not 0, I doubt this will give the expected result.
> 
> The value 0x00 of divider means nand clock off then read/write nand register is forbidden.

That is not entirely true, you can access the clock register or you'd be in a
chicken and egg situation.

> Should we set the initial value in nand driver, or in sub emmc clk driver?

In the nand driver, which is the consumer of the clock. see my previous comments
about it.

If your device (nand in your case) needs a (sane) clock before doing anything
else, just call clk_set_rate() and enable it with clk_prepare_enable().

Look at our MMC driver, this is the first thing done after registering the
clocks.

The controller does no care at all about the clock rate or state. It is up to
the consumer to express their needs.

On a more general note:
I agree that having a 0 value for CLK_DIVIDER_ONE_BASED divider makes no sense.
If you think this point needs to be addressed, please:

* make separated generic patch
* against driver/clk/clk-divider.c 
* addressing specifically CLK_DIVIDER_ONE_BASED dividers (not all, as your
change does)
* with some comments explaining the intent.

>  
> > 
> > > +}
> > 
> > Please drop this. 
> > 
> > > +
> > > +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
> > >  const struct clk_ops clk_regmap_divider_ops = {
> > >  	.recalc_rate = clk_regmap_div_recalc_rate,
> > >  	.round_rate = clk_regmap_div_round_rate,
> > > @@ -122,6 +139,14 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
> > >  };
> > >  EXPORT_SYMBOL_GPL(clk_regmap_divider_ops);
> > >  
> > > +const struct clk_ops clk_regmap_divider_with_init_ops = {
> > > +	.recalc_rate = clk_regmap_div_recalc_rate,
> > > +	.round_rate = clk_regmap_div_round_rate,
> > > +	.set_rate = clk_regmap_div_set_rate,
> > > +	.init = clk_regmap_div_init,
> > > +};
> > > +EXPORT_SYMBOL_GPL(clk_regmap_divider_with_init_ops);
> > 
> > ... and this.
> > 
> > > +
> > >  const struct clk_ops clk_regmap_divider_ro_ops = {
> > >  	.recalc_rate = clk_regmap_div_recalc_rate,
> > >  	.round_rate = clk_regmap_div_round_rate,
> > > diff --git a/drivers/clk/meson/clk-regmap.h b/drivers/clk/meson/clk-regmap.h
> > > index ed2d434..0ab7d37 100644
> > > --- a/drivers/clk/meson/clk-regmap.h
> > > +++ b/drivers/clk/meson/clk-regmap.h
> > > @@ -109,5 +109,6 @@ struct clk_regmap_mux_data {
> > >  
> > >  extern const struct clk_ops clk_regmap_mux_ops;
> > >  extern const struct clk_ops clk_regmap_mux_ro_ops;
> > > +extern const struct clk_ops clk_regmap_divider_with_init_ops;
> > 
> > ... and this as well.
> > 
> > There is no reason to to init the divider to something other than what it is
> > already when we register the clock controller.
> > 
> > If your consumer needs the clocks to be initialised, it should call
> > clk_set_rate()
> > 
> > >  
> > >  #endif /* __CLK_REGMAP_H */
> > > diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> > > new file mode 100644
> > > index 0000000..5555e3f
> > > --- /dev/null
> > > +++ b/drivers/clk/meson/mmc-clkc.c
> > > @@ -0,0 +1,296 @@
> > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > +/*
> > > + * Amlogic Meson MMC Sub Clock Controller Driver
> > > + *
> > > + * Copyright (c) 2017 Baylibre SAS.
> > > + * Author: Jerome Brunet <[e-mail]>
> > > + *
> > > + * Copyright (c) 2018 Amlogic, inc.
> > > + * Author: Yixun Lan <[e-mail]>
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/platform_device.h>
> > > +#include <dt-bindings/clock/amlogic,mmc-clkc.h>
> > > +
> > > +#include "clkc.h"
> > > +
> > > +/* clock ID used by internal driver */
> > > +#define CLKID_MMC_MUX			0
> > > +
> > > +#define   SD_EMMC_CLOCK		0
> > > +#define   CLK_DELAY_STEP_PS		200
> > > +#define   CLK_PHASE_STEP		30
> > > +#define   CLK_PHASE_POINT_NUM		(360 / CLK_PHASE_STEP)
> > > +
> > > +#define MUX_CLK_NUM_PARENTS		2
> > > +#define MMC_MAX_CLKS			5
> > > +
> > > +struct mmc_clkc_data {
> > > +	struct meson_clk_phase_delay_data	tx;
> > > +	struct meson_clk_phase_delay_data	rx;
> > > +};
> > > +
> > > +static struct clk_regmap_mux_data mmc_clkc_mux_data = {
> > > +	.offset		= SD_EMMC_CLOCK,
> > > +	.mask		= 0x3,
> > > +	.shift		= 6,
> > > +	.flags		= CLK_DIVIDER_ROUND_CLOSEST,

Missed that earlier

This flag makes no sense for a mux.
Anyway this particular mux should never round up has doing so would be unsafe.
The clock provided to the nand or the eMMC should be the requested or lower.

> > > +};
> > > +
> > > +static struct clk_regmap_div_data mmc_clkc_div_data = {
> > > +	.offset		= SD_EMMC_CLOCK,
> > > +	.shift		= 0,
> > > +	.width		= 6,
> > > +	.flags		= CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,

Same here, drop CLK_DIVIDER_ROUND_CLOSEST

> > > +};
> > > +
> > > +static struct meson_clk_phase_data mmc_clkc_core_phase = {
> > > +	.ph = {
> > > +		.reg_off	= SD_EMMC_CLOCK,
> > > +		.shift	= 8,
> > > +		.width	= 2,
> > > +	}
> > > +};
> > > +
> > > +static const struct mmc_clkc_data mmc_clkc_gx_data = {
> > > +	.tx = {
> > > +		.phase = {
> > > +			.reg_off	= SD_EMMC_CLOCK,
> > > +			.shift	= 10,
> > > +			.width	= 2,
> > > +		},
> > > +		.delay = {
> > > +			.reg_off	= SD_EMMC_CLOCK,
> > > +			.shift	= 16,
> > > +			.width	= 4,
> > > +		},
> > > +		.delay_step_ps	= CLK_DELAY_STEP_PS,
> > > +	},
> > > +	.rx = {
> > > +		.phase = {
> > > +			.reg_off	= SD_EMMC_CLOCK,
> > > +			.shift	= 12,
> > > +			.width	= 2,
> > > +		},
> > > +		.delay = {
> > > +			.reg_off	= SD_EMMC_CLOCK,
> > > +			.shift	= 20,
> > > +			.width	= 4,
> > > +		},
> > > +		.delay_step_ps	= CLK_DELAY_STEP_PS,
> > > +	},
> > > +};
> > > +
> > > +static const struct mmc_clkc_data mmc_clkc_axg_data = {
> > > +	.tx = {
> > > +		.phase = {
> > > +			.reg_off	= SD_EMMC_CLOCK,
> > > +			.shift	= 10,
> > > +			.width	= 2,
> > > +		},
> > > +		.delay = {
> > > +			.reg_off	= SD_EMMC_CLOCK,
> > > +			.shift	= 16,
> > > +			.width	= 6,
> > > +		},
> > > +		.delay_step_ps	= CLK_DELAY_STEP_PS,
> > > +	},
> > > +	.rx = {
> > > +		.phase = {
> > > +			.reg_off	= SD_EMMC_CLOCK,
> > > +			.shift	= 12,
> > > +			.width	= 2,
> > > +		},
> > > +		.delay = {
> > > +			.reg_off	= SD_EMMC_CLOCK,
> > > +			.shift	= 22,
> > > +			.width	= 6,
> > > +		},
> > > +		.delay_step_ps	= CLK_DELAY_STEP_PS,
> > > +	},
> > > +};
> > > +
> > > +static const struct of_device_id mmc_clkc_match_table[] = {
> > > +	{
> > > +		.compatible	= "amlogic,gx-mmc-clkc",
> > > +		.data		= &mmc_clkc_gx_data
> > > +	},
> > > +	{
> > > +		.compatible	= "amlogic,axg-mmc-clkc",
> > > +		.data		= &mmc_clkc_axg_data
> > > +	},
> > > +	{}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mmc_clkc_match_table);
> > > +
> > > +static struct clk_regmap *
> > > +mmc_clkc_register_clk(struct device *dev, struct regmap *map,
> > > +		      struct clk_init_data *init,
> > > +		      const char *suffix, void *data)
> > > +{
> > > +	struct clk_regmap *clk;
> > > +	char *name;
> > > +	int ret;
> > > +
> > > +	clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
> > > +	if (!clk)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), suffix);
> > > +	if (!name)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	init->name = name;
> > > +
> > > +	clk->map = map;
> > > +	clk->data = data;
> > > +	clk->hw.init = init;
> > > +
> > > +	ret = devm_clk_hw_register(dev, &clk->hw);
> > > +	if (ret)
> > > +		clk = ERR_PTR(ret);
> > > +
> > > +	kfree(name);
> > > +	return clk;
> > > +}
> > > +
> > > +static struct clk_regmap *mmc_clkc_register_mux(struct device *dev,
> > > +						struct regmap *map)
> > > +{
> > > +	const char *parent_names[MUX_CLK_NUM_PARENTS];
> > > +	struct clk_init_data init;
> > > +	struct clk_regmap *mux;
> > > +	struct clk *clk;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
> > > +		char name[8];
> > > +
> > > +		snprintf(name, sizeof(name), "clkin%d", i);
> > > +		clk = devm_clk_get(dev, name);
> > > +		if (IS_ERR(clk)) {
> > > +			if (clk != ERR_PTR(-EPROBE_DEFER))
> > > +				dev_err(dev, "Missing clock %s\n", name);
> > > +			return ERR_PTR((long)clk);
> > > +		}
> > > +
> > > +		parent_names[i] = __clk_get_name(clk);
> > > +	}
> > > +
> > > +	init.ops = &clk_regmap_mux_ops;
> > > +	init.flags = CLK_SET_RATE_PARENT;
> > > +	init.parent_names = parent_names;
> > > +	init.num_parents = MUX_CLK_NUM_PARENTS;
> > > +
> > > +	mux = mmc_clkc_register_clk(dev, map, &init, "mux", &mmc_clkc_mux_data);
> > > +	if (IS_ERR(mux))
> > > +		dev_err(dev, "Mux clock registration failed\n");
> > > +
> > > +	return mux;
> > > +}
> > > +
> > > +static struct clk_regmap *
> > > +mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
> > > +				  char *suffix, const char *parent,
> > 
> > I would prefer if you passed the parent clk_hw pointer and call
> > clk_hw_get_name() in here
> 
> OK. I will fix it. Thanks for your review.
> > 
> > > +				  unsigned long flags,
> > > +				  const struct clk_ops *ops, void *data)
> > > +{
> > > +	struct clk_init_data init;
> > > +	struct clk_regmap *clk;
> > > +
> > > +	init.ops = ops;
> > > +	init.flags = flags;
> > > +	init.parent_names = (const char* const []){ parent, };
> > > +	init.num_parents = 1;
> > > +
> > > +	clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
> > > +	if (IS_ERR(clk))
> > > +		dev_err(dev, "Core %s clock registration failed\n", suffix);
> > > +
> > > +	return clk;
> > > +}
> > > +
> > > +static int mmc_clkc_probe(struct platform_device *pdev)
> > > +{
> > > +	struct clk_hw_onecell_data *onecell_data;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct mmc_clkc_data *data;
> > > +	struct regmap *map;
> > > +	struct clk_regmap *mux, *div, *core, *rx, *tx;
> > 
> > You really don't need all these local variables ( I think I already pointed this
> > out in past reviews ...)
> > 
> > You can keep one struct clk_regmap *clk and store the clk->hw in the onecell
> > data right after registering the clock.
> 
> OK, I will remove them.
> > 
> > 
> > > +
> > > +	data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
> > > +	if (!data)
> > > +		return -ENODEV;
> > > +
> > > +	map = syscon_node_to_regmap(dev->of_node);
> > > +	if (IS_ERR(map)) {
> > > +		dev_err(dev, "could not find mmc clock controller\n");
> > > +		return PTR_ERR(map);
> > > +	}
> > > +
> > > +	onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
> > > +				    sizeof(*onecell_data->hws) * MMC_MAX_CLKS,
> > > +				    GFP_KERNEL);
> > > +	if (!onecell_data)
> > > +		return -ENOMEM;
> > > +
> > > +	mux = mmc_clkc_register_mux(dev, map);
> > > +	if (IS_ERR(mux))
> > > +		return PTR_ERR(mux);
> > > +
> > > +	div = mmc_clkc_register_clk_with_parent(dev, map, "div",
> > > +						clk_hw_get_name(&mux->hw),
> > > +						CLK_SET_RATE_PARENT,
> > > +						&clk_regmap_divider_with_init_ops,
> > > +						&mmc_clkc_div_data);
> > > +	if (IS_ERR(div))
> > > +		return PTR_ERR(div);
> > > +
> > > +	core = mmc_clkc_register_clk_with_parent(dev, map, "core",
> > > +						 clk_hw_get_name(&div->hw),
> > > +						 CLK_SET_RATE_PARENT,
> > > +						 &meson_clk_phase_ops,
> > > +						 &mmc_clkc_core_phase);
> > > +	if (IS_ERR(core))
> > > +		return PTR_ERR(core);
> > > +
> > > +	rx = mmc_clkc_register_clk_with_parent(dev, map, "rx",
> > > +					       clk_hw_get_name(&core->hw),  0,
> > > +					       &meson_clk_phase_delay_ops,
> > > +					       &data->rx);
> > > +	if (IS_ERR(rx))
> > > +		return PTR_ERR(rx);
> > > +
> > > +	tx = mmc_clkc_register_clk_with_parent(dev, map, "tx",
> > > +					       clk_hw_get_name(&core->hw),  0,
> > > +					       &meson_clk_phase_delay_ops,
> > > +					       &data->tx);
> > > +	if (IS_ERR(tx))
> > > +		return PTR_ERR(tx);
> > > +
> > > +	onecell_data->hws[CLKID_MMC_MUX]		= &mux->hw,
> > > +	onecell_data->hws[CLKID_MMC_DIV]		= &div->hw,
> > > +	onecell_data->hws[CLKID_MMC_PHASE_CORE]		= &core->hw,
> > > +	onecell_data->hws[CLKID_MMC_PHASE_RX]		= &rx->hw,
> > > +	onecell_data->hws[CLKID_MMC_PHASE_TX]		= &tx->hw,
> > > +	onecell_data->num				= MMC_MAX_CLKS;
> > > +
> > > +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> > > +					   onecell_data);
> > > +}
> > > +
> > > +static struct platform_driver mmc_clkc_driver = {
> > > +	.probe		= mmc_clkc_probe,
> > > +	.driver		= {
> > > +		.name	= "meson-mmc-clkc",
> > > +		.of_match_table = of_match_ptr(mmc_clkc_match_table),
> > > +	},
> > > +};
> > > +
> > > +module_platform_driver(mmc_clkc_driver);
> > 
> > This can be compiled as module so please append the proper:
> > MODULE_DESCRIPTION()
> > MODULE_AUTHOR()
> > MODULE_LICENSE()
> 
> OK. I will fix it in the next version.
> > 
> > .
> > 
> 
>

Martin Blumenstingl Oct. 25, 2018, 8:58 p.m. UTC | #12

Hi Jerome,

On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <[e-mail]> wrote:
[snip]
> > > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > > +{
> > > > + struct clk_regmap *clk = to_clk_regmap(hw);
> > > > + struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> > > > + unsigned int val;
> > > > + int ret;
> > > > +
> > > > + ret = regmap_read(clk->map, div->offset, &val);
> > > > + if (ret)
> > > > +         return;
> > > >
> > > > + val &= (clk_div_mask(div->width) << div->shift);
> > > > + if (!val)
> > > > +         regmap_update_bits(clk->map, div->offset,
> > > > +                            clk_div_mask(div->width) << div->shift,
> > > > +                            clk_div_mask(div->width));
> > >
> > > This is wrong for several reasons:
> > > * You should hard code the initial value in the driver.
> > > * If shift is not 0, I doubt this will give the expected result.
> >
> > The value 0x00 of divider means nand clock off then read/write nand register is forbidden.
>
> That is not entirely true, you can access the clock register or you'd be in a
> chicken and egg situation.
>
> > Should we set the initial value in nand driver, or in sub emmc clk driver?
>
> In the nand driver, which is the consumer of the clock. see my previous comments
> about it.
an old version of this series had the code still in the NAND driver
(by writing to the registers directly instead of using the clk API).
this looks pretty much like a "sclk-div" to me (as I commented in v3
of this series: [0]):
- value 0 means disabled
- positive divider values
- (probably no duty control, but that's optional as far as I
understand sclk-div)
- uses max divider value when enabling the clock

if switching to sclk-div works then we can get rid of some duplicate code


Regards
Martin


[0] https://patchwork.kernel.org/patch/10607157/#22238243

Jianxin Pan Oct. 28, 2018, 3:12 p.m. UTC | #13

Hi Jerome,

On 2018/10/25 20:54, Jerome Brunet wrote:
> On Thu, 2018-10-25 at 19:48 +0800, Jianxin Pan wrote:
>> Hi Jerome,
>>
>> On 2018/10/24 17:01, Jerome Brunet wrote:
>>> On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan wrote:
>>>> From: Yixun Lan <[e-mail]>
>>>>
>>>> The patch will add a MMC clock controller driver which used by MMC or NAND,
>>>> It provide a mux and divider clock, and three phase clocks - core, tx, tx.
>>>>
[...]
>>>>  
>>>> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>>>> +static void clk_regmap_div_init(struct clk_hw *hw)
>>>> +{
>>>> +	struct clk_regmap *clk = to_clk_regmap(hw);
>>>> +	struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>>>> +	unsigned int val;
>>>> +	int ret;
>>>> +
>>>> +	ret = regmap_read(clk->map, div->offset, &val);
>>>> +	if (ret)
>>>> +		return;
>>>>  
>>>> +	val &= (clk_div_mask(div->width) << div->shift);
>>>> +	if (!val)
>>>> +		regmap_update_bits(clk->map, div->offset,
>>>> +				   clk_div_mask(div->width) << div->shift,
>>>> +				   clk_div_mask(div->width));
>>>
>>> This is wrong for several reasons:
>>> * You should hard code the initial value in the driver.
>>> * If shift is not 0, I doubt this will give the expected result.
>>
>> The value 0x00 of divider means nand clock off then read/write nand register is forbidden.
> 
> That is not entirely true, you can access the clock register or you'd be in a
> chicken and egg situation.
> 
>> Should we set the initial value in nand driver, or in sub emmc clk driver?
> 
> In the nand driver, which is the consumer of the clock. see my previous comments
> about it.
> 
> If your device (nand in your case) needs a (sane) clock before doing anything
> else, just call clk_set_rate() and enable it with clk_prepare_enable().
> 
> Look at our MMC driver, this is the first thing done after registering the
> clocks.
> 
> The controller does no care at all about the clock rate or state. It is up to
> the consumer to express their needs.
> 
> On a more general note:
> I agree that having a 0 value for CLK_DIVIDER_ONE_BASED divider makes no sense.
> If you think this point needs to be addressed, please:
> 
> * make separated generic patch
> * against driver/clk/clk-divider.c 
> * addressing specifically CLK_DIVIDER_ONE_BASED dividers (not all, as your
> change does)
> * with some comments explaining the intent.
In our emmc driver, the CLK_DIV_MASK is hard coded before clock registering in meson_mmc_clk_init():
 clk_reg |= CLK_DIV_MASK;
 writel(clk_reg, host->regs + SD_EMMC_CLOCK);
It's hard coded In previous version of nand driver.  I will drop the new ops.

In addition , Martin suggested in previous discussions that "sclk-div" could be used [0][1]. 
This divider is just like a "sclk-div" with sclk->hi->width ==0. 


> 
>>  
>>>
>>>> +}
>>>
>>> Please drop this. 
OK. Thank you.
>>>
>>>> +
>>>> +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
>>>>  const struct clk_ops clk_regmap_divider_ops = {
>>>>  	.recalc_rate = clk_regmap_div_recalc_rate,
>>>>  	.round_rate = clk_regmap_div_round_rate,
>>>> @@ -122,6 +139,14 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>>>  };
>>>>  EXPORT_SYMBOL_GPL(clk_regmap_divider_ops);
>>>>  
[...]
>>>> +
>>>> +static struct clk_regmap_mux_data mmc_clkc_mux_data = {
>>>> +	.offset		= SD_EMMC_CLOCK,
>>>> +	.mask		= 0x3,
>>>> +	.shift		= 6,
>>>> +	.flags		= CLK_DIVIDER_ROUND_CLOSEST,
> 
> Missed that earlier
> 
> This flag makes no sense for a mux.
> Anyway this particular mux should never round up has doing so would be unsafe.
> The clock provided to the nand or the eMMC should be the requested or lower.
> 
OK, I will drop it. Thanks for your time.
>>>> +};
>>>> +
>>>> +static struct clk_regmap_div_data mmc_clkc_div_data = {
>>>> +	.offset		= SD_EMMC_CLOCK,
>>>> +	.shift		= 0,
>>>> +	.width		= 6,
>>>> +	.flags		= CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
> 
> Same here, drop CLK_DIVIDER_ROUND_CLOSEST
OK, I will drop it. Thank you!
> 
>>>> +};
>>>> +
>>>> +static struct meson_clk_phase_data mmc_clkc_core_phase = {
>>>> +	.ph = {
>>>> +		.reg_off	= SD_EMMC_CLOCK,
>>>> +		.shift	= 8,
[...]
> 
> .
> 

[0] https://patchwork.kernel.org/patch/10607157/#22238243
[1]https://lore.kernel.org/lkml/[e-mail]/T/#u

Jerome Brunet Oct. 28, 2018, 7:16 p.m. UTC | #14

On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
> Hi Jerome,
> 
> On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <[e-mail]> wrote:
> [snip]
> > > > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > > > +{
> > > > > + struct clk_regmap *clk = to_clk_regmap(hw);
> > > > > + struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> > > > > + unsigned int val;
> > > > > + int ret;
> > > > > +
> > > > > + ret = regmap_read(clk->map, div->offset, &val);
> > > > > + if (ret)
> > > > > +         return;
> > > > > 
> > > > > + val &= (clk_div_mask(div->width) << div->shift);
> > > > > + if (!val)
> > > > > +         regmap_update_bits(clk->map, div->offset,
> > > > > +                            clk_div_mask(div->width) << div->shift,
> > > > > +                            clk_div_mask(div->width));
> > > > 
> > > > This is wrong for several reasons:
> > > > * You should hard code the initial value in the driver.
> > > > * If shift is not 0, I doubt this will give the expected result.
> > > 
> > > The value 0x00 of divider means nand clock off then read/write nand register is forbidden.
> > 
> > That is not entirely true, you can access the clock register or you'd be in a
> > chicken and egg situation.
> > 
> > > Should we set the initial value in nand driver, or in sub emmc clk driver?
> > 
> > In the nand driver, which is the consumer of the clock. see my previous comments
> > about it.
> 
> an old version of this series had the code still in the NAND driver
> (by writing to the registers directly instead of using the clk API).
> this looks pretty much like a "sclk-div" to me (as I commented in v3
> of this series: [0]):
> - value 0 means disabled
> - positive divider values
> - (probably no duty control, but that's optional as far as I
> understand sclk-div)
> - uses max divider value when enabling the clock
> 
> if switching to sclk-div works then we can get rid of some duplicate code

It is possible:
There is a couple of things to note though:

* sclk does not 'uses max divider value when enabling the clock': Since this
divider can gate, it needs to save the divider value when disabling, since the
divider value is no longer stored in the register,
On init, this cached value is  saved as it is. If the divider is initially
disabled, we have to set the cached value to something that makes sense, in case
the clock is enabled without a prior call to clk_set_rate().

So in sclk, the clock setting is not changed nor hard coded in init, and this is
a very important difference.
 
* Even if sclk zero value means gated, it is still a zero based divider, while
eMMC/Nand divider is one based. It this controller was to sclk, then something
needs to be done for this.

* Since sclk caches a value in its data, and there can multiple instance of eMMC
/NAND clock controller, some care must be taken when registering the data.

Both the generic divider and sclk could work here ... it's up to you Jianxin.

> 
> 
> Regards
> Martin
> 
> 
> [0] https://patchwork.kernel.org/patch/10607157/#22238243

Martin Blumenstingl Oct. 29, 2018, 7:45 p.m. UTC | #15

Hi Jerome,

many thanks for the whole explanation!

On Sun, Oct 28, 2018 at 8:16 PM Jerome Brunet <[e-mail]> wrote:
>
> On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
> > Hi Jerome,
> >
> > On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <[e-mail]> wrote:
> > [snip]
> > > > > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > > > > +{
> > > > > > + struct clk_regmap *clk = to_clk_regmap(hw);
> > > > > > + struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> > > > > > + unsigned int val;
> > > > > > + int ret;
> > > > > > +
> > > > > > + ret = regmap_read(clk->map, div->offset, &val);
> > > > > > + if (ret)
> > > > > > +         return;
> > > > > >
> > > > > > + val &= (clk_div_mask(div->width) << div->shift);
> > > > > > + if (!val)
> > > > > > +         regmap_update_bits(clk->map, div->offset,
> > > > > > +                            clk_div_mask(div->width) << div->shift,
> > > > > > +                            clk_div_mask(div->width));
> > > > >
> > > > > This is wrong for several reasons:
> > > > > * You should hard code the initial value in the driver.
> > > > > * If shift is not 0, I doubt this will give the expected result.
> > > >
> > > > The value 0x00 of divider means nand clock off then read/write nand register is forbidden.
> > >
> > > That is not entirely true, you can access the clock register or you'd be in a
> > > chicken and egg situation.
> > >
> > > > Should we set the initial value in nand driver, or in sub emmc clk driver?
> > >
> > > In the nand driver, which is the consumer of the clock. see my previous comments
> > > about it.
> >
> > an old version of this series had the code still in the NAND driver
> > (by writing to the registers directly instead of using the clk API).
> > this looks pretty much like a "sclk-div" to me (as I commented in v3
> > of this series: [0]):
> > - value 0 means disabled
> > - positive divider values
> > - (probably no duty control, but that's optional as far as I
> > understand sclk-div)
> > - uses max divider value when enabling the clock
> >
> > if switching to sclk-div works then we can get rid of some duplicate code
>
> It is possible:
> There is a couple of things to note though:
>
> * sclk does not 'uses max divider value when enabling the clock': Since this
> divider can gate, it needs to save the divider value when disabling, since the
> divider value is no longer stored in the register,
> On init, this cached value is  saved as it is. If the divider is initially
> disabled, we have to set the cached value to something that makes sense, in case
> the clock is enabled without a prior call to clk_set_rate().
>
> So in sclk, the clock setting is not changed nor hard coded in init, and this is
> a very important difference.
>
> * Even if sclk zero value means gated, it is still a zero based divider, while
> eMMC/Nand divider is one based. It this controller was to sclk, then something
> needs to be done for this.
>
> * Since sclk caches a value in its data, and there can multiple instance of eMMC
> /NAND clock controller, some care must be taken when registering the data.
>
> Both the generic divider and sclk could work here ... it's up to you Jianxin.
to give even more options:
the generic divider will (probably) get a CLK_DIVIDER_ZERO_GATE flag
in the next development cycle, see [0] (this may require a small
change to clk-regmap on top)


Regards
Martin


[0] https://patchwork.kernel.org/patch/10650797/

Jianxin Pan Oct. 30, 2018, 1:41 p.m. UTC | #16

Hi Jerome,

On 2018/10/29 3:16, Jerome Brunet wrote:
> On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
>> Hi Jerome,
>>
>> On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <[e-mail]> wrote:
>> [snip]
>>>>>> +static void clk_regmap_div_init(struct clk_hw *hw)
>>>>>> +{
>>>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>>>> + struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>>>>>> + unsigned int val;
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = regmap_read(clk->map, div->offset, &val);
>>>>>> + if (ret)
>>>>>> +         return;
>>>>>>
>>>>>> + val &= (clk_div_mask(div->width) << div->shift);
>>>>>> + if (!val)
>>>>>> +         regmap_update_bits(clk->map, div->offset,
>>>>>> +                            clk_div_mask(div->width) << div->shift,
>>>>>> +                            clk_div_mask(div->width));
>>>>>
>>>>> This is wrong for several reasons:
>>>>> * You should hard code the initial value in the driver.
>>>>> * If shift is not 0, I doubt this will give the expected result.
>>>>
>>>> The value 0x00 of divider means nand clock off then read/write nand register is forbidden.
>>>
>>> That is not entirely true, you can access the clock register or you'd be in a
>>> chicken and egg situation.
>>>
>>>> Should we set the initial value in nand driver, or in sub emmc clk driver?
>>>
>>> In the nand driver, which is the consumer of the clock. see my previous comments
>>> about it.
>>
>> an old version of this series had the code still in the NAND driver
>> (by writing to the registers directly instead of using the clk API).
>> this looks pretty much like a "sclk-div" to me (as I commented in v3
>> of this series: [0]):
>> - value 0 means disabled
>> - positive divider values
>> - (probably no duty control, but that's optional as far as I
>> understand sclk-div)
>> - uses max divider value when enabling the clock
>>
>> if switching to sclk-div works then we can get rid of some duplicate code
> 
> It is possible:
> There is a couple of things to note though:
> 
> * sclk does not 'uses max divider value when enabling the clock': Since this
> divider can gate, it needs to save the divider value when disabling, since the
> divider value is no longer stored in the register,
> On init, this cached value is  saved as it is. If the divider is initially
> disabled, we have to set the cached value to something that makes sense, in case
> the clock is enabled without a prior call to clk_set_rate().
> 
> So in sclk, the clock setting is not changed nor hard coded in init, and this is
> a very important difference.
>  
> * Even if sclk zero value means gated, it is still a zero based divider, while
> eMMC/Nand divider is one based. It this controller was to sclk, then something
> needs to be done for this.
> 
> * Since sclk caches a value in its data, and there can multiple instance of eMMC
> /NAND clock controller, some care must be taken when registering the data.
> 
> Both the generic divider and sclk could work here ... it's up to you Jianxin.
Thank you for the detailed explanation.
I will use sclk here.

With generic divider, there is a WARNING in divider_recalc_rate() durning clk_register():
[    0.918238] ffe05000.clock-controller#div: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set
[    0.925581] WARNING: CPU: 3 PID: 1 at drivers/clk/clk-divider.c:127 divider_recalc_rate+0x88/0x90
Then I still need to hard code the initual value, or add CLK_DIVIDER_ALLOW_ZERO flags.
> 
>>
>>
>> Regards
>> Martin
>>
>>
>> [0] https://patchwork.kernel.org/patch/10607157/#22238243
> 
> 
> .
>

Jianxin Pan Nov. 3, 2018, 6:01 p.m. UTC | #17

Hi Jerome,

Thanks for the review, we really appreciate your time.

I'm very sorry maybe I don't catch all your meaning very well. 

Please see my comments below.

On 2018/10/29 3:16, Jerome Brunet wrote:
> On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
>> Hi Jerome,
>>
>> On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <[e-mail]> wrote:
>> [snip]
>>>>>> +static void clk_regmap_div_init(struct clk_hw *hw)
>>>>>> +{
>>>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>>>> + struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>>>>>> + unsigned int val;
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = regmap_read(clk->map, div->offset, &val);
>>>>>> + if (ret)
>>>>>> +         return;
>>>>>>
>>>>>> + val &= (clk_div_mask(div->width) << div->shift);
>>>>>> + if (!val)
>>>>>> +         regmap_update_bits(clk->map, div->offset,
>>>>>> +                            clk_div_mask(div->width) << div->shift,
>>>>>> +                            clk_div_mask(div->width));
>>>>>
>>>>> This is wrong for several reasons:
>>>>> * You should hard code the initial value in the driver.
>>>>> * If shift is not 0, I doubt this will give the expected result.
>>>>
>>>> The value 0x00 of divider means nand clock off then read/write nand register is forbidden.
>>>
>>> That is not entirely true, you can access the clock register or you'd be in a
>>> chicken and egg situation.
>>>
>>>> Should we set the initial value in nand driver, or in sub emmc clk driver?
>>>
>>> In the nand driver, which is the consumer of the clock. see my previous comments
>>> about it.
>>
>> an old version of this series had the code still in the NAND driver
>> (by writing to the registers directly instead of using the clk API).
>> this looks pretty much like a "sclk-div" to me (as I commented in v3
>> of this series: [0]):
>> - value 0 means disabled
>> - positive divider values
>> - (probably no duty control, but that's optional as far as I
>> understand sclk-div)
>> - uses max divider value when enabling the clock
>>
>> if switching to sclk-div works then we can get rid of some duplicate code
> 
> It is possible:
> There is a couple of things to note though:
> 
> * sclk does not 'uses max divider value when enabling the clock': Since this
> divider can gate, it needs to save the divider value when disabling, since the
> divider value is no longer stored in the register,
> On init, this cached value is  saved as it is. If the divider is initially
> disabled, we have to set the cached value to something that makes sense, in case
> the clock is enabled without a prior call to clk_set_rate().
>> So in sclk, the clock setting is not changed nor hard coded in init, and this is
> a very important difference.
>
I think It's ok for the latest sub mmc clock and nand driver now:
1. in mmc_clkc_register_clk_with_parent("div", ...) from mmc_clkc_probe():
   cached_div is set to div_max durning clk register，but is not set to div hw register.

2. In meson nand driver v6: 	https://lore.kernel.org/lkml/[e-mail]
1) In meson_nfc_clk_init() from probe:   get clock handle, then prepare_enable and set default rate.
   nfc->device_clk = devm_clk_get(nfc->dev, "device");
   ret = clk_prepare_enable(nfc->device_clk);			//Here div hw register changed from 0 -> cached_div.
   default_clk_rate = clk_round_rate(nfc->device_clk, 24000000);
   ret = clk_set_rate(nfc->device_clk, default_clk_rate);	//Then register and cached_div are both updated to the default 24M.
2) In meson_nfc_select_chip(), set the actual frequency
  ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate);	//Here register and cached_div are changed again.
3) if clk_disable() is called, set div hw register to zero, and cached_div  keep unchagned.
   if clk_disable() is called again,  cached_div is restored to div hw register.

When enabling the clock, divider register does not need to be div_max.  
Any value is OK except ZERO, the cached_div from init or set_rate is ok
>  
> * Even if sclk zero value means gated, it is still a zero based divider, while
> eMMC/Nand divider is one based. It this controller was to sclk, then something
> needs to be done for this.
Could I add another patch to this patchset for sclk to support CLK_DIVIDER_ONE_BASED ?
> 
> * Since sclk caches a value in its data, and there can multiple instance of eMMC
> /NAND clock controller, some care must be taken when registering the data.
OK, I will fix it and alloc mmc_clkc_div_data danymicly durning probe.
Thank you. 
> 
> Both the generic divider and sclk could work here ... it's up to you Jianxin.
> 
== Why use meson_sclk_div_ops instead of clk_regmap_divider_ops?
The default divider hw register vaule is 0 when system power on.
Then there is a WARNING in divider_recalc_rate() durning clk_hw_register():
[    0.918238] ffe05000.clock-controller#div: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set
[    0.925581] WARNING: CPU: 3 PID: 1 at drivers/clk/clk-divider.c:127 divider_recalc_rate+0x88/0x90
Then I still need to hard code the initual value to nand driver without CLK_DIVIDER_ALLOW_ZERO flags.
>>
>>
>> Regards
>> Martin
>>
>>
>> [0] https://patchwork.kernel.org/patch/10607157/#22238243
> 
> 
> .
>

Jerome Brunet Nov. 5, 2018, 9:46 a.m. UTC | #18

On Sun, 2018-11-04 at 02:01 +0800, Jianxin Pan wrote:
> Hi Jerome,
> 
> Thanks for the review, we really appreciate your time.
> 
> I'm very sorry maybe I don't catch all your meaning very well. 
> 
> Please see my comments below.
> 
> On 2018/10/29 3:16, Jerome Brunet wrote:
> > On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
> > > Hi Jerome,
> > > 
> > > On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <[e-mail]>
> > > wrote:
> > > [snip]
> > > > > > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > > > > > +{
> > > > > > > + struct clk_regmap *clk = to_clk_regmap(hw);
> > > > > > > + struct clk_regmap_div_data *div =
> > > > > > > clk_get_regmap_div_data(clk);
> > > > > > > + unsigned int val;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + ret = regmap_read(clk->map, div->offset, &val);
> > > > > > > + if (ret)
> > > > > > > +         return;
> > > > > > > 
> > > > > > > + val &= (clk_div_mask(div->width) << div->shift);
> > > > > > > + if (!val)
> > > > > > > +         regmap_update_bits(clk->map, div->offset,
> > > > > > > +                            clk_div_mask(div->width) << div-
> > > > > > > >shift,
> > > > > > > +                            clk_div_mask(div->width));
> > > > > > 
> > > > > > This is wrong for several reasons:
> > > > > > * You should hard code the initial value in the driver.
> > > > > > * If shift is not 0, I doubt this will give the expected result.
> > > > > 
> > > > > The value 0x00 of divider means nand clock off then read/write nand
> > > > > register is forbidden.
> > > > 
> > > > That is not entirely true, you can access the clock register or you'd
> > > > be in a
> > > > chicken and egg situation.
> > > > 
> > > > > Should we set the initial value in nand driver, or in sub emmc clk
> > > > > driver?
> > > > 
> > > > In the nand driver, which is the consumer of the clock. see my
> > > > previous comments
> > > > about it.
> > > 
> > > an old version of this series had the code still in the NAND driver
> > > (by writing to the registers directly instead of using the clk API).
> > > this looks pretty much like a "sclk-div" to me (as I commented in v3
> > > of this series: [0]):
> > > - value 0 means disabled
> > > - positive divider values
> > > - (probably no duty control, but that's optional as far as I
> > > understand sclk-div)
> > > - uses max divider value when enabling the clock
> > > 
> > > if switching to sclk-div works then we can get rid of some duplicate
> > > code
> > 
> > It is possible:
> > There is a couple of things to note though:
> > 
> > * sclk does not 'uses max divider value when enabling the clock': Since
> > this
> > divider can gate, it needs to save the divider value when disabling, since
> > the
> > divider value is no longer stored in the register,
> > On init, this cached value is  saved as it is. If the divider is initially
> > disabled, we have to set the cached value to something that makes sense,
> > in case
> > the clock is enabled without a prior call to clk_set_rate().
> > > So in sclk, the clock setting is not changed nor hard coded in init, and
> > > this is
> > a very important difference.
> > 
> I think It's ok for the latest sub mmc clock and nand driver now:
> 1. in mmc_clkc_register_clk_with_parent("div", ...) from mmc_clkc_probe():
>    cached_div is set to div_max durning clk register，but is not set to div
> hw register.
> 
> 2. In meson nand driver v6: 	
> https://lore.kernel.org/lkml/[e-mail]
> 1) In meson_nfc_clk_init() from probe:   get clock handle, then
> prepare_enable and set default rate.
>    nfc->device_clk = devm_clk_get(nfc->dev, "device");
>    ret = clk_prepare_enable(nfc->device_clk);			//Here div hw
> register changed from 0 -> cached_div.
>    default_clk_rate = clk_round_rate(nfc->device_clk, 24000000);
>    ret = clk_set_rate(nfc->device_clk, default_clk_rate);	//Then
> register and cached_div are both updated to the default 24M.
> 2) In meson_nfc_select_chip(), set the actual frequency
>   ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate);	//Here
> register and cached_div are changed again.
> 3) if clk_disable() is called, set div hw register to zero, and
> cached_div  keep unchagned.
>    if clk_disable() is called again,  cached_div is restored to div hw
> register.

You don't need to do all this in your NAND driver: enable - round - set_rate -
disable is a waste of time. 

Directly calling set_rate(24000000), with the clock still off, will have the
same result. Then if your HW needs this clock to be ON to access registers
(like you told us) you should probably turn it on.

> 
> When enabling the clock, divider register does not need to be div_max.  
> Any value is OK except ZERO, the cached_div from init or set_rate is ok
> >  
> > * Even if sclk zero value means gated, it is still a zero based divider,
> > while
> > eMMC/Nand divider is one based. It this controller was to sclk, then
> > something
> > needs to be done for this.
> Could I add another patch to this patchset for sclk to support
> CLK_DIVIDER_ONE_BASED ?

Yes, you should otherwise the calculation are just wrong for your clock.

> > * Since sclk caches a value in its data, and there can multiple instance
> > of eMMC
> > /NAND clock controller, some care must be taken when registering the data.
> OK, I will fix it and alloc mmc_clkc_div_data danymicly durning probe.
> Thank you. 
> > Both the generic divider and sclk could work here ... it's up to you
> > Jianxin.
> > 
> == Why use meson_sclk_div_ops instead of clk_regmap_divider_ops?
> The default divider hw register vaule is 0 when system power on.
> Then there is a WARNING in divider_recalc_rate() durning clk_hw_register():
> [    0.918238] ffe05000.clock-controller#div: Zero divisor and
> CLK_DIVIDER_ALLOW_ZERO not set
> [    0.925581] WARNING: CPU: 3 PID: 1 at drivers/clk/clk-divider.c:127
> divider_recalc_rate+0x88/0x90
> Then I still need to hard code the initual value to nand driver without
> CLK_DIVIDER_ALLOW_ZERO flags.
> > > 
> > > Regards
> > > Martin
> > > 
> > > 
> > > [0] https://patchwork.kernel.org/patch/10607157/#22238243
> > 
> > .
> > 
> 
>

Jianxin Pan Nov. 5, 2018, 11:29 a.m. UTC | #19

On 2018/11/5 17:46, [e-mail] wrote:
> On Sun, 2018-11-04 at 02:01 +0800, Jianxin Pan wrote:
>> Hi Jerome,
>>
>> Thanks for the review, we really appreciate your time.
>>
>> I'm very sorry maybe I don't catch all your meaning very well. 
>>
>> Please see my comments below.
>>
>> On 2018/10/29 3:16, Jerome Brunet wrote:
>>> On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
>>>> Hi Jerome,
>>>>
>>>> On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <[e-mail]>
>>>> wrote:
>>>> [snip]
>>>>>>>> +static void clk_regmap_div_init(struct clk_hw *hw)
>>>>>>>> +{
>>>>>>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>>>>>>> + struct clk_regmap_div_data *div =
>>>>>>>> clk_get_regmap_div_data(clk);
>>>>>>>> + unsigned int val;
>>>>>>>> + int ret;
>>>>>>>> +
>>>>>>>> + ret = regmap_read(clk->map, div->offset, &val);
>>>>>>>> + if (ret)
>>>>>>>> +         return;
>>>>>>>>
>>>>>>>> + val &= (clk_div_mask(div->width) << div->shift);
>>>>>>>> + if (!val)
>>>>>>>> +         regmap_update_bits(clk->map, div->offset,
>>>>>>>> +                            clk_div_mask(div->width) << div-
>>>>>>>>> shift,
>>>>>>>> +                            clk_div_mask(div->width));
>>>>>>>
>>>>>>> This is wrong for several reasons:
>>>>>>> * You should hard code the initial value in the driver.
>>>>>>> * If shift is not 0, I doubt this will give the expected result.
>>>>>>
>>>>>> The value 0x00 of divider means nand clock off then read/write nand
>>>>>> register is forbidden.
>>>>>
>>>>> That is not entirely true, you can access the clock register or you'd
>>>>> be in a
>>>>> chicken and egg situation.
>>>>>
>>>>>> Should we set the initial value in nand driver, or in sub emmc clk
>>>>>> driver?
>>>>>
>>>>> In the nand driver, which is the consumer of the clock. see my
>>>>> previous comments
>>>>> about it.
>>>>
>>>> an old version of this series had the code still in the NAND driver
>>>> (by writing to the registers directly instead of using the clk API).
>>>> this looks pretty much like a "sclk-div" to me (as I commented in v3
>>>> of this series: [0]):
>>>> - value 0 means disabled
>>>> - positive divider values
>>>> - (probably no duty control, but that's optional as far as I
>>>> understand sclk-div)
>>>> - uses max divider value when enabling the clock
>>>>
>>>> if switching to sclk-div works then we can get rid of some duplicate
>>>> code
>>>
>>> It is possible:
>>> There is a couple of things to note though:
>>>
>>> * sclk does not 'uses max divider value when enabling the clock': Since
>>> this
>>> divider can gate, it needs to save the divider value when disabling, since
>>> the
>>> divider value is no longer stored in the register,
>>> On init, this cached value is  saved as it is. If the divider is initially
>>> disabled, we have to set the cached value to something that makes sense,
>>> in case
>>> the clock is enabled without a prior call to clk_set_rate().
>>>> So in sclk, the clock setting is not changed nor hard coded in init, and
>>>> this is
>>> a very important difference.
>>>
>> I think It's ok for the latest sub mmc clock and nand driver now:
>> 1. in mmc_clkc_register_clk_with_parent("div", ...) from mmc_clkc_probe():
>>    cached_div is set to div_max durning clk register，but is not set to div
>> hw register.
>>
>> 2. In meson nand driver v6: 	
>> https://lore.kernel.org/lkml/[e-mail]
>> 1) In meson_nfc_clk_init() from probe:   get clock handle, then
>> prepare_enable and set default rate.
>>    nfc->device_clk = devm_clk_get(nfc->dev, "device");
>>    ret = clk_prepare_enable(nfc->device_clk);			//Here div hw
>> register changed from 0 -> cached_div.
>>    default_clk_rate = clk_round_rate(nfc->device_clk, 24000000);
>>    ret = clk_set_rate(nfc->device_clk, default_clk_rate);	//Then
>> register and cached_div are both updated to the default 24M.
>> 2) In meson_nfc_select_chip(), set the actual frequency
>>   ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate);	//Here
>> register and cached_div are changed again.
>> 3) if clk_disable() is called, set div hw register to zero, and
>> cached_div  keep unchagned.
>>    if clk_disable() is called again,  cached_div is restored to div hw
>> register.
> 
> You don't need to do all this in your NAND driver: enable - round - set_rate -
> disable is a waste of time. 
> 
> Directly calling set_rate(24000000), with the clock still off, will have the
> same result. Then if your HW needs this clock to be ON to access registers
> (like you told us) you should probably turn it on.
I'm sorry I didn't describe it very clearly in last mail. 
The steps in nand v6 probe are: enable -> round(24M) -> set_rate(24M)， then this clock is always on.
And it's disabled only in the nand remove() callback. 
I will remove round(24M) in next version .
Thank you.
> 
>>
>> When enabling the clock, divider register does not need to be div_max.  
>> Any value is OK except ZERO, the cached_div from init or set_rate is ok
>>>  
>>> * Even if sclk zero value means gated, it is still a zero based divider,
>>> while
>>> eMMC/Nand divider is one based. It this controller was to sclk, then
>>> something
>>> needs to be done for this.
>> Could I add another patch to this patchset for sclk to support
>> CLK_DIVIDER_ONE_BASED ?
> 
> Yes, you should otherwise the calculation are just wrong for your clock.
OK. Thank you.
> 
>>> * Since sclk caches a value in its data, and there can multiple instance
>>> of eMMC
>>> /NAND clock controller, some care must be taken when registering the data.
>> OK, I will fix it and alloc mmc_clkc_div_data danymicly durning probe.
>> Thank you. 
>>> Both the generic divider and sclk could work here ... it's up to you
>>> Jianxin.
>>>
>> == Why use meson_sclk_div_ops instead of clk_regmap_divider_ops?
>> The default divider hw register vaule is 0 when system power on.
>> Then there is a WARNING in divider_recalc_rate() durning clk_hw_register():
>> [    0.918238] ffe05000.clock-controller#div: Zero divisor and
>> CLK_DIVIDER_ALLOW_ZERO not set
>> [    0.925581] WARNING: CPU: 3 PID: 1 at drivers/clk/clk-divider.c:127
>> divider_recalc_rate+0x88/0x90
>> Then I still need to hard code the initual value to nand driver without
>> CLK_DIVIDER_ALLOW_ZERO flags.
>>>>
>>>> Regards
>>>> Martin
>>>>
>>>>
>>>> [0] https://patchwork.kernel.org/patch/10607157/#22238243
>>>
>>> .
>>>
>>
>>
> 
> 
> .
>

Patch

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index efaa70f..8b8ccbc 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -15,6 +15,16 @@  config COMMON_CLK_MESON_AO
 	select COMMON_CLK_REGMAP_MESON
 	select RESET_CONTROLLER
 
+config COMMON_CLK_MMC_MESON
+	tristate "Meson MMC Sub Clock Controller Driver"
+	depends on COMMON_CLK_AMLOGIC
+	select MFD_SYSCON
+	select REGMAP
+	help
+	  Support for the MMC sub clock controller on Amlogic Meson Platform,
+	  which include S905 (GXBB, GXL), A113D/X (AXG) devices.
+	  Say Y if you want this clock enabled.
+
 config COMMON_CLK_REGMAP_MESON
 	bool
 	select REGMAP
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 39ce566..31c16d5 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -9,4 +9,5 @@  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
 obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
 obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
 obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)	+= axg-audio.o
+obj-$(CONFIG_COMMON_CLK_MMC_MESON) 	+= mmc-clkc.o
 obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
index 305ee30..f96314d 100644
--- a/drivers/clk/meson/clk-regmap.c
+++ b/drivers/clk/meson/clk-regmap.c
@@ -113,8 +113,25 @@  static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
 				  clk_div_mask(div->width) << div->shift, val);
 };
 
-/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
+static void clk_regmap_div_init(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(clk->map, div->offset, &val);
+	if (ret)
+		return;
 
+	val &= (clk_div_mask(div->width) << div->shift);
+	if (!val)
+		regmap_update_bits(clk->map, div->offset,
+				   clk_div_mask(div->width) << div->shift,
+				   clk_div_mask(div->width));
+}
+
+/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */
 const struct clk_ops clk_regmap_divider_ops = {
 	.recalc_rate = clk_regmap_div_recalc_rate,
 	.round_rate = clk_regmap_div_round_rate,
@@ -122,6 +139,14 @@  static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
 };
 EXPORT_SYMBOL_GPL(clk_regmap_divider_ops);
 
+const struct clk_ops clk_regmap_divider_with_init_ops = {
+	.recalc_rate = clk_regmap_div_recalc_rate,
+	.round_rate = clk_regmap_div_round_rate,
+	.set_rate = clk_regmap_div_set_rate,
+	.init = clk_regmap_div_init,
+};
+EXPORT_SYMBOL_GPL(clk_regmap_divider_with_init_ops);
+
 const struct clk_ops clk_regmap_divider_ro_ops = {
 	.recalc_rate = clk_regmap_div_recalc_rate,
 	.round_rate = clk_regmap_div_round_rate,
diff --git a/drivers/clk/meson/clk-regmap.h b/drivers/clk/meson/clk-regmap.h
index ed2d434..0ab7d37 100644
--- a/drivers/clk/meson/clk-regmap.h
+++ b/drivers/clk/meson/clk-regmap.h
@@ -109,5 +109,6 @@  struct clk_regmap_mux_data {
 
 extern const struct clk_ops clk_regmap_mux_ops;
 extern const struct clk_ops clk_regmap_mux_ro_ops;
+extern const struct clk_ops clk_regmap_divider_with_init_ops;
 
 #endif /* __CLK_REGMAP_H */
diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
new file mode 100644
index 0000000..5555e3f
--- /dev/null
+++ b/drivers/clk/meson/mmc-clkc.c
@@ -0,0 +1,296 @@ 
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson MMC Sub Clock Controller Driver
+ *
+ * Copyright (c) 2017 Baylibre SAS.
+ * Author: Jerome Brunet <[e-mail]>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <[e-mail]>
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <dt-bindings/clock/amlogic,mmc-clkc.h>
+
+#include "clkc.h"
+
+/* clock ID used by internal driver */
+#define CLKID_MMC_MUX			0
+
+#define   SD_EMMC_CLOCK		0
+#define   CLK_DELAY_STEP_PS		200
+#define   CLK_PHASE_STEP		30
+#define   CLK_PHASE_POINT_NUM		(360 / CLK_PHASE_STEP)
+
+#define MUX_CLK_NUM_PARENTS		2
+#define MMC_MAX_CLKS			5
+
+struct mmc_clkc_data {
+	struct meson_clk_phase_delay_data	tx;
+	struct meson_clk_phase_delay_data	rx;
+};
+
+static struct clk_regmap_mux_data mmc_clkc_mux_data = {
+	.offset		= SD_EMMC_CLOCK,
+	.mask		= 0x3,
+	.shift		= 6,
+	.flags		= CLK_DIVIDER_ROUND_CLOSEST,
+};
+
+static struct clk_regmap_div_data mmc_clkc_div_data = {
+	.offset		= SD_EMMC_CLOCK,
+	.shift		= 0,
+	.width		= 6,
+	.flags		= CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
+};
+
+static struct meson_clk_phase_data mmc_clkc_core_phase = {
+	.ph = {
+		.reg_off	= SD_EMMC_CLOCK,
+		.shift	= 8,
+		.width	= 2,
+	}
+};
+
+static const struct mmc_clkc_data mmc_clkc_gx_data = {
+	.tx = {
+		.phase = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 10,
+			.width	= 2,
+		},
+		.delay = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 16,
+			.width	= 4,
+		},
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+	.rx = {
+		.phase = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 12,
+			.width	= 2,
+		},
+		.delay = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 20,
+			.width	= 4,
+		},
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+};
+
+static const struct mmc_clkc_data mmc_clkc_axg_data = {
+	.tx = {
+		.phase = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 10,
+			.width	= 2,
+		},
+		.delay = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 16,
+			.width	= 6,
+		},
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+	.rx = {
+		.phase = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 12,
+			.width	= 2,
+		},
+		.delay = {
+			.reg_off	= SD_EMMC_CLOCK,
+			.shift	= 22,
+			.width	= 6,
+		},
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+};
+
+static const struct of_device_id mmc_clkc_match_table[] = {
+	{
+		.compatible	= "amlogic,gx-mmc-clkc",
+		.data		= &mmc_clkc_gx_data
+	},
+	{
+		.compatible	= "amlogic,axg-mmc-clkc",
+		.data		= &mmc_clkc_axg_data
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, mmc_clkc_match_table);
+
+static struct clk_regmap *
+mmc_clkc_register_clk(struct device *dev, struct regmap *map,
+		      struct clk_init_data *init,
+		      const char *suffix, void *data)
+{
+	struct clk_regmap *clk;
+	char *name;
+	int ret;
+
+	clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
+	if (!clk)
+		return ERR_PTR(-ENOMEM);
+
+	name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), suffix);
+	if (!name)
+		return ERR_PTR(-ENOMEM);
+
+	init->name = name;
+
+	clk->map = map;
+	clk->data = data;
+	clk->hw.init = init;
+
+	ret = devm_clk_hw_register(dev, &clk->hw);
+	if (ret)
+		clk = ERR_PTR(ret);
+
+	kfree(name);
+	return clk;
+}
+
+static struct clk_regmap *mmc_clkc_register_mux(struct device *dev,
+						struct regmap *map)
+{
+	const char *parent_names[MUX_CLK_NUM_PARENTS];
+	struct clk_init_data init;
+	struct clk_regmap *mux;
+	struct clk *clk;
+	int i;
+
+	for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
+		char name[8];
+
+		snprintf(name, sizeof(name), "clkin%d", i);
+		clk = devm_clk_get(dev, name);
+		if (IS_ERR(clk)) {
+			if (clk != ERR_PTR(-EPROBE_DEFER))
+				dev_err(dev, "Missing clock %s\n", name);
+			return ERR_PTR((long)clk);
+		}
+
+		parent_names[i] = __clk_get_name(clk);
+	}
+
+	init.ops = &clk_regmap_mux_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	init.parent_names = parent_names;
+	init.num_parents = MUX_CLK_NUM_PARENTS;
+
+	mux = mmc_clkc_register_clk(dev, map, &init, "mux", &mmc_clkc_mux_data);
+	if (IS_ERR(mux))
+		dev_err(dev, "Mux clock registration failed\n");
+
+	return mux;
+}
+
+static struct clk_regmap *
+mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
+				  char *suffix, const char *parent,
+				  unsigned long flags,
+				  const struct clk_ops *ops, void *data)
+{
+	struct clk_init_data init;
+	struct clk_regmap *clk;
+
+	init.ops = ops;
+	init.flags = flags;
+	init.parent_names = (const char* const []){ parent, };
+	init.num_parents = 1;
+
+	clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
+	if (IS_ERR(clk))
+		dev_err(dev, "Core %s clock registration failed\n", suffix);
+
+	return clk;
+}
+
+static int mmc_clkc_probe(struct platform_device *pdev)
+{
+	struct clk_hw_onecell_data *onecell_data;
+	struct device *dev = &pdev->dev;
+	struct mmc_clkc_data *data;
+	struct regmap *map;
+	struct clk_regmap *mux, *div, *core, *rx, *tx;
+
+	data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
+	if (!data)
+		return -ENODEV;
+
+	map = syscon_node_to_regmap(dev->of_node);
+	if (IS_ERR(map)) {
+		dev_err(dev, "could not find mmc clock controller\n");
+		return PTR_ERR(map);
+	}
+
+	onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
+				    sizeof(*onecell_data->hws) * MMC_MAX_CLKS,
+				    GFP_KERNEL);
+	if (!onecell_data)
+		return -ENOMEM;
+
+	mux = mmc_clkc_register_mux(dev, map);
+	if (IS_ERR(mux))
+		return PTR_ERR(mux);
+
+	div = mmc_clkc_register_clk_with_parent(dev, map, "div",
+						clk_hw_get_name(&mux->hw),
+						CLK_SET_RATE_PARENT,
+						&clk_regmap_divider_with_init_ops,
+						&mmc_clkc_div_data);
+	if (IS_ERR(div))
+		return PTR_ERR(div);
+
+	core = mmc_clkc_register_clk_with_parent(dev, map, "core",
+						 clk_hw_get_name(&div->hw),
+						 CLK_SET_RATE_PARENT,
+						 &meson_clk_phase_ops,
+						 &mmc_clkc_core_phase);
+	if (IS_ERR(core))
+		return PTR_ERR(core);
+
+	rx = mmc_clkc_register_clk_with_parent(dev, map, "rx",
+					       clk_hw_get_name(&core->hw),  0,
+					       &meson_clk_phase_delay_ops,
+					       &data->rx);
+	if (IS_ERR(rx))
+		return PTR_ERR(rx);
+
+	tx = mmc_clkc_register_clk_with_parent(dev, map, "tx",
+					       clk_hw_get_name(&core->hw),  0,
+					       &meson_clk_phase_delay_ops,
+					       &data->tx);
+	if (IS_ERR(tx))
+		return PTR_ERR(tx);
+
+	onecell_data->hws[CLKID_MMC_MUX]		= &mux->hw,
+	onecell_data->hws[CLKID_MMC_DIV]		= &div->hw,
+	onecell_data->hws[CLKID_MMC_PHASE_CORE]		= &core->hw,
+	onecell_data->hws[CLKID_MMC_PHASE_RX]		= &rx->hw,
+	onecell_data->hws[CLKID_MMC_PHASE_TX]		= &tx->hw,
+	onecell_data->num				= MMC_MAX_CLKS;
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					   onecell_data);
+}
+
+static struct platform_driver mmc_clkc_driver = {
+	.probe		= mmc_clkc_probe,
+	.driver		= {
+		.name	= "meson-mmc-clkc",
+		.of_match_table = of_match_ptr(mmc_clkc_match_table),
+	},
+};
+
+module_platform_driver(mmc_clkc_driver);