[PATCH v2 0/3] clk: meson: add a sub EMMC clock controller support

[PATCH v2 0/3] clk: meson: add a sub EMMC clock controller support

[Yixun Lan]

This driver will add a MMC clock controller driver support.
The original idea about adding a clock controller is during the
discussion in the NAND driver mainline effort[1].

I've tested this in the S400 board (AXG platform) by using NAND driver.

Changes since v1 [2]:
 - implement phase clock
 - update compatible name
 - adjust file name
 - divider probe() into small functions, and re-use them

[1] https://lkml.kernel.org/r/20180628090034.0637a062@xps13
[2] https://lkml.kernel.org/r/20180703145716.31860-1-yixun.lan@amlogic.com

Yixun Lan (3):
  clk: meson: add DT documentation for emmc clock controller
  clk: meson: add sub MMC clock dt-bindings IDs
  clk: meson: add sub MMC clock controller driver

 .../bindings/clock/amlogic,mmc-clkc.txt       |  31 ++
 drivers/clk/meson/Kconfig                     |   9 +
 drivers/clk/meson/Makefile                    |   1 +
 drivers/clk/meson/mmc-clkc.c                  | 419 ++++++++++++++++++
 .../clock/amlogic,meson-mmc-clkc.h            |  16 +
 5 files changed, 476 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
 create mode 100644 drivers/clk/meson/mmc-clkc.c
 create mode 100644 include/dt-bindings/clock/amlogic,meson-mmc-clkc.h

-- 
2.18.0

[PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller

[Yixun Lan]

Document the MMC sub clock controller driver, the potential consumer
of this driver is MMC or NAND.

Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 .../bindings/clock/amlogic,mmc-clkc.txt       | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt

diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
new file mode 100644
index 000000000000..ff6b4bf3ecf9
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
@@ -0,0 +1,31 @@
+* Amlogic MMC Sub Clock Controller Driver
+
+The Amlogic MMC clock controller generates and supplies clock to support
+MMC and NAND controller
+
+Required Properties:
+
+- compatible: should be:
+		"amlogic,meson-gx-mmc-clkc"
+		"amlogic,meson-axg-mmc-clkc"
+
+- #clock-cells: should be 1.
+- clocks: phandles to clocks corresponding to the clock-names property
+- clock-names: list of parent clock names
+	- "clkin0", "clkin1"
+
+Parent node should have the following properties :
+- compatible: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
+- reg: base address and size of the MMC control register space.
+
+Example: Clock controller node:
+
+sd_mmc_c_clkc: clock-controller@7000 {
+	compatible = "amlogic,mmc-clkc", "syscon", "simple-mfd";
+	reg = <0x0 0x7000 0x0 0x4>;
+	#clock-cells = <1>;
+
+	clock-names = "clkin0", "clkin1";
+	clocks = <&clkc CLKID_SD_MMC_C_CLK0>,
+			<&clkc CLKID_FCLK_DIV2>;
+};
-- 
2.18.0

Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller

[Rob Herring]

On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:
> Document the MMC sub clock controller driver, the potential consumer
> of this driver is MMC or NAND.

So you all have decided to properly model this now?

> 
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  .../bindings/clock/amlogic,mmc-clkc.txt       | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
> new file mode 100644
> index 000000000000..ff6b4bf3ecf9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
> @@ -0,0 +1,31 @@
> +* Amlogic MMC Sub Clock Controller Driver
> +
> +The Amlogic MMC clock controller generates and supplies clock to support
> +MMC and NAND controller
> +
> +Required Properties:
> +
> +- compatible: should be:
> +		"amlogic,meson-gx-mmc-clkc"
> +		"amlogic,meson-axg-mmc-clkc"
> +
> +- #clock-cells: should be 1.
> +- clocks: phandles to clocks corresponding to the clock-names property
> +- clock-names: list of parent clock names
> +	- "clkin0", "clkin1"
> +
> +Parent node should have the following properties :
> +- compatible: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"

You don't need "simple-mfd" and probably not syscon either. The order is 
wrong too. Most specific first.

> +- reg: base address and size of the MMC control register space.
> +
> +Example: Clock controller node:
> +
> +sd_mmc_c_clkc: clock-controller@7000 {
> +	compatible = "amlogic,mmc-clkc", "syscon", "simple-mfd";

Doesn't match the binding...

> +	reg = <0x0 0x7000 0x0 0x4>;
> +	#clock-cells = <1>;
> +
> +	clock-names = "clkin0", "clkin1";
> +	clocks = <&clkc CLKID_SD_MMC_C_CLK0>,
> +			<&clkc CLKID_FCLK_DIV2>;
> +};
> -- 
> 2.18.0
> 

Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller

[Yixun Lan]

Hi Rob

see my comments

On 07/12/18 03:43, Rob Herring wrote:
> On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:
>> Document the MMC sub clock controller driver, the potential consumer
>> of this driver is MMC or NAND.
> 
> So you all have decided to properly model this now?
> 
Yes, ;-)

>>
>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> ---
>>  .../bindings/clock/amlogic,mmc-clkc.txt       | 31 +++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>> new file mode 100644
>> index 000000000000..ff6b4bf3ecf9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>> @@ -0,0 +1,31 @@
>> +* Amlogic MMC Sub Clock Controller Driver
>> +
>> +The Amlogic MMC clock controller generates and supplies clock to support
>> +MMC and NAND controller
>> +
>> +Required Properties:
>> +
>> +- compatible: should be:
>> +		"amlogic,meson-gx-mmc-clkc"
>> +		"amlogic,meson-axg-mmc-clkc"
>> +
>> +- #clock-cells: should be 1.
>> +- clocks: phandles to clocks corresponding to the clock-names property
>> +- clock-names: list of parent clock names
>> +	- "clkin0", "clkin1"
>> +
>> +Parent node should have the following properties :
>> +- compatible: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
> 
> You don't need "simple-mfd" and probably not syscon either. The order is 
> wrong too. Most specific first.
> 
Ok, I will drop "simple-mfd"..

but the syscon is a must, since this mmc clock model access registers
via the regmap interface

I will fix the order, thanks for pointing this out

>> +- reg: base address and size of the MMC control register space.
>> +
>> +Example: Clock controller node:
>> +
>> +sd_mmc_c_clkc: clock-controller@7000 {
>> +	compatible = "amlogic,mmc-clkc", "syscon", "simple-mfd";
> 
> Doesn't match the binding...
> 
oops, I will update this

>> +	reg = <0x0 0x7000 0x0 0x4>;
>> +	#clock-cells = <1>;
>> +
>> +	clock-names = "clkin0", "clkin1";
>> +	clocks = <&clkc CLKID_SD_MMC_C_CLK0>,
>> +			<&clkc CLKID_FCLK_DIV2>;
>> +};
>> -- 
>> 2.18.0
>>
> 
> .
> 

Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller

[Rob Herring]

On Wed, Jul 11, 2018 at 8:47 PM Yixun Lan <yixun.lan@amlogic.com> wrote:
>
> Hi Rob
>
> see my comments
>
> On 07/12/18 03:43, Rob Herring wrote:
> > On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:
> >> Document the MMC sub clock controller driver, the potential consumer
> >> of this driver is MMC or NAND.
> >
> > So you all have decided to properly model this now?
> >
> Yes, ;-)
>
> >>
> >> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> >> ---
> >>  .../bindings/clock/amlogic,mmc-clkc.txt       | 31 +++++++++++++++++++
> >>  1 file changed, 31 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
> >> new file mode 100644
> >> index 000000000000..ff6b4bf3ecf9
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
> >> @@ -0,0 +1,31 @@
> >> +* Amlogic MMC Sub Clock Controller Driver
> >> +
> >> +The Amlogic MMC clock controller generates and supplies clock to support
> >> +MMC and NAND controller
> >> +
> >> +Required Properties:
> >> +
> >> +- compatible: should be:
> >> +            "amlogic,meson-gx-mmc-clkc"
> >> +            "amlogic,meson-axg-mmc-clkc"
> >> +
> >> +- #clock-cells: should be 1.
> >> +- clocks: phandles to clocks corresponding to the clock-names property
> >> +- clock-names: list of parent clock names
> >> +    - "clkin0", "clkin1"
> >> +
> >> +Parent node should have the following properties :
> >> +- compatible: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
> >
> > You don't need "simple-mfd" and probably not syscon either. The order is
> > wrong too. Most specific first.
> >
> Ok, I will drop "simple-mfd"..
>
> but the syscon is a must, since this mmc clock model access registers
> via the regmap interface

A syscon compatible should not be the only way to get a regmap.
Removing lines 56/57 of drivers/mfd/syscon.c should be sufficient.

Why do you need a regmap in the first place? What else needs to access
this register directly? Don't you need a patch removing the clock code
from within the emmc driver? It's not even using regmap, so using
regmap here doesn't help.

Rob

Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller

[Yixun Lan]

HI Rob

see my comments

On 07/12/2018 10:17 PM, Rob Herring wrote:
> On Wed, Jul 11, 2018 at 8:47 PM Yixun Lan <yixun.lan@amlogic.com> wrote:
>>
>> Hi Rob
>>
>> see my comments
>>
>> On 07/12/18 03:43, Rob Herring wrote:
>>> On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:
>>>> Document the MMC sub clock controller driver, the potential consumer
>>>> of this driver is MMC or NAND.
>>>
>>> So you all have decided to properly model this now?
>>>
>> Yes, ;-)
>>
>>>>
>>>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>>>> ---
>>>>  .../bindings/clock/amlogic,mmc-clkc.txt       | 31 +++++++++++++++++++
>>>>  1 file changed, 31 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>>>> new file mode 100644
>>>> index 000000000000..ff6b4bf3ecf9
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>>>> @@ -0,0 +1,31 @@
>>>> +* Amlogic MMC Sub Clock Controller Driver
>>>> +
>>>> +The Amlogic MMC clock controller generates and supplies clock to support
>>>> +MMC and NAND controller
>>>> +
>>>> +Required Properties:
>>>> +
>>>> +- compatible: should be:
>>>> +            "amlogic,meson-gx-mmc-clkc"
>>>> +            "amlogic,meson-axg-mmc-clkc"
>>>> +
>>>> +- #clock-cells: should be 1.
>>>> +- clocks: phandles to clocks corresponding to the clock-names property
>>>> +- clock-names: list of parent clock names
>>>> +    - "clkin0", "clkin1"
>>>> +
>>>> +Parent node should have the following properties :
>>>> +- compatible: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
>>>
>>> You don't need "simple-mfd" and probably not syscon either. The order is
>>> wrong too. Most specific first.
>>>
>> Ok, I will drop "simple-mfd"..
>>
>> but the syscon is a must, since this mmc clock model access registers
>> via the regmap interface
> 
> A syscon compatible should not be the only way to get a regmap.
do you have any suggestion about other function that I can use? is
devm_regmap_init_mmio() feasible

> Removing lines 56/57 of drivers/mfd/syscon.c should be sufficient.
> 
I'm not sure what's the valid point of removing compatible 'syscon' in
driver/mfd/syscon.c, sounds this will break a lot DT/or need to fix?
will you propose a patch for this? then I can certainly adjust here

> Why do you need a regmap in the first place? What else needs to access
> this register directly?
Yes, the SD_EMMC_CLOCK register contain several bits which not fit well
into common clock model, and they need to be access in the NAND or eMMC
driver itself, Martin had explained this in early thread[1]

In this register
Bit[31] select NAND or eMMC function
Bit[25] enable SDIO IRQ
Bit[24] Clock always on
Bit[15:14] SRAM Power down

[1]
https://lkml.kernel.org/r/CAFBinCBeyXf6LNaZzAw6WnsxzDAv8E=Yp2eem0xCPWMEUi6pnQ@mail.gmail.com

> Don't you need a patch removing the clock code
> from within the emmc driver? It's not even using regmap, so using
> regmap here doesn't help.
>
No, and current eMMC driver still use iomap to access the register

I think we probably would like to take two steps approach.
first, from the hardware perspective, the NAND and eMMC(port C) driver
can't exist at same time, since they share the pins, clock, internal
ram, So we have to only enable one of NAND or eMMC in DT, not enable
both of them.
Second, we might like to convert eMMC driver to also use mmc-clkc model.


Yixun

Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller

[Rob Herring]

On Thu, Jul 12, 2018 at 5:29 PM Yixun Lan <yixun.lan@amlogic.com> wrote:
>
> HI Rob
>
> see my comments
>
> On 07/12/2018 10:17 PM, Rob Herring wrote:
> > On Wed, Jul 11, 2018 at 8:47 PM Yixun Lan <yixun.lan@amlogic.com> wrote:
> >>
> >> Hi Rob
> >>
> >> see my comments
> >>
> >> On 07/12/18 03:43, Rob Herring wrote:
> >>> On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:
> >>>> Document the MMC sub clock controller driver, the potential consumer
> >>>> of this driver is MMC or NAND.
> >>>
> >>> So you all have decided to properly model this now?
> >>>
> >> Yes, ;-)
> >>
> >>>>
> >>>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> >>>> ---
> >>>>  .../bindings/clock/amlogic,mmc-clkc.txt       | 31 +++++++++++++++++++
> >>>>  1 file changed, 31 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
> >>>> new file mode 100644
> >>>> index 000000000000..ff6b4bf3ecf9
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
> >>>> @@ -0,0 +1,31 @@
> >>>> +* Amlogic MMC Sub Clock Controller Driver
> >>>> +
> >>>> +The Amlogic MMC clock controller generates and supplies clock to support
> >>>> +MMC and NAND controller
> >>>> +
> >>>> +Required Properties:
> >>>> +
> >>>> +- compatible: should be:
> >>>> +            "amlogic,meson-gx-mmc-clkc"
> >>>> +            "amlogic,meson-axg-mmc-clkc"
> >>>> +
> >>>> +- #clock-cells: should be 1.
> >>>> +- clocks: phandles to clocks corresponding to the clock-names property
> >>>> +- clock-names: list of parent clock names
> >>>> +    - "clkin0", "clkin1"
> >>>> +
> >>>> +Parent node should have the following properties :
> >>>> +- compatible: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
> >>>
> >>> You don't need "simple-mfd" and probably not syscon either. The order is
> >>> wrong too. Most specific first.
> >>>
> >> Ok, I will drop "simple-mfd"..
> >>
> >> but the syscon is a must, since this mmc clock model access registers
> >> via the regmap interface
> >
> > A syscon compatible should not be the only way to get a regmap.
> do you have any suggestion about other function that I can use? is
> devm_regmap_init_mmio() feasible
>
> > Removing lines 56/57 of drivers/mfd/syscon.c should be sufficient.
> >
> I'm not sure what's the valid point of removing compatible 'syscon' in
> driver/mfd/syscon.c, sounds this will break a lot DT/or need to fix?
> will you propose a patch for this? then I can certainly adjust here

Removing the 2 lines will simply allow any node to be a syscon. If
there's a specific driver for a node, then that makes sense to allow
that.

>
> > Why do you need a regmap in the first place? What else needs to access
> > this register directly?
> Yes, the SD_EMMC_CLOCK register contain several bits which not fit well
> into common clock model, and they need to be access in the NAND or eMMC
> driver itself, Martin had explained this in early thread[1]
>
> In this register
> Bit[31] select NAND or eMMC function
> Bit[25] enable SDIO IRQ
> Bit[24] Clock always on
> Bit[15:14] SRAM Power down
>
> [1]
> https://lkml.kernel.org/r/CAFBinCBeyXf6LNaZzAw6WnsxzDAv8E=Yp2eem0xCPWMEUi6pnQ@mail.gmail.com
>
> > Don't you need a patch removing the clock code
> > from within the emmc driver? It's not even using regmap, so using
> > regmap here doesn't help.
> >
> No, and current eMMC driver still use iomap to access the register

Which means a read-modify-write can corrupt the register value if both
users don't access thru regmap. Changes are probably infrequent enough
that you get lucky...

> I think we probably would like to take two steps approach.
> first, from the hardware perspective, the NAND and eMMC(port C) driver
> can't exist at same time, since they share the pins, clock, internal
> ram, So we have to only enable one of NAND or eMMC in DT, not enable
> both of them.

Yes, of course.

> Second, we might like to convert eMMC driver to also use mmc-clkc model.

IMO, this should be done as part of merging this series. Otherwise, we
have duplicated code for the same thing.

Rob

Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller

[Yixun Lan]

Hi Rob, Jerome, Kevin

see my comments

On 07/13/18 08:15, Rob Herring wrote:
> On Thu, Jul 12, 2018 at 5:29 PM Yixun Lan <yixun.lan@amlogic.com> wrote:
>>
>> HI Rob
>>
>> see my comments
>>
>> On 07/12/2018 10:17 PM, Rob Herring wrote:
>>> On Wed, Jul 11, 2018 at 8:47 PM Yixun Lan <yixun.lan@amlogic.com> wrote:
>>>>
>>>> Hi Rob
>>>>
>>>> see my comments
>>>>
>>>> On 07/12/18 03:43, Rob Herring wrote:
>>>>> On Tue, Jul 10, 2018 at 04:36:56PM +0000, Yixun Lan wrote:
>>>>>> Document the MMC sub clock controller driver, the potential consumer
>>>>>> of this driver is MMC or NAND.
>>>>>
>>>>> So you all have decided to properly model this now?
>>>>>
>>>> Yes, ;-)
>>>>
>>>>>>
>>>>>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>>>>>> ---
>>>>>>  .../bindings/clock/amlogic,mmc-clkc.txt       | 31 +++++++++++++++++++
>>>>>>  1 file changed, 31 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..ff6b4bf3ecf9
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.txt
>>>>>> @@ -0,0 +1,31 @@
>>>>>> +* Amlogic MMC Sub Clock Controller Driver
>>>>>> +
>>>>>> +The Amlogic MMC clock controller generates and supplies clock to support
>>>>>> +MMC and NAND controller
>>>>>> +
>>>>>> +Required Properties:
>>>>>> +
>>>>>> +- compatible: should be:
>>>>>> +            "amlogic,meson-gx-mmc-clkc"
>>>>>> +            "amlogic,meson-axg-mmc-clkc"
>>>>>> +
>>>>>> +- #clock-cells: should be 1.
>>>>>> +- clocks: phandles to clocks corresponding to the clock-names property
>>>>>> +- clock-names: list of parent clock names
>>>>>> +    - "clkin0", "clkin1"
>>>>>> +
>>>>>> +Parent node should have the following properties :
>>>>>> +- compatible: "syscon", "simple-mfd, and "amlogic,meson-axg-mmc-clkc"
>>>>>
>>>>> You don't need "simple-mfd" and probably not syscon either. The order is
>>>>> wrong too. Most specific first.
>>>>>
>>>> Ok, I will drop "simple-mfd"..
>>>>
>>>> but the syscon is a must, since this mmc clock model access registers
>>>> via the regmap interface
>>>
>>> A syscon compatible should not be the only way to get a regmap.
>> do you have any suggestion about other function that I can use? is
>> devm_regmap_init_mmio() feasible
>>
>>> Removing lines 56/57 of drivers/mfd/syscon.c should be sufficient.
>>>
>> I'm not sure what's the valid point of removing compatible 'syscon' in
>> driver/mfd/syscon.c, sounds this will break a lot DT/or need to fix?
>> will you propose a patch for this? then I can certainly adjust here
> 
> Removing the 2 lines will simply allow any node to be a syscon. If
> there's a specific driver for a node, then that makes sense to allow
> that.
> 
>>
>>> Why do you need a regmap in the first place? What else needs to access
>>> this register directly?
>> Yes, the SD_EMMC_CLOCK register contain several bits which not fit well
>> into common clock model, and they need to be access in the NAND or eMMC
>> driver itself, Martin had explained this in early thread[1]
>>
>> In this register
>> Bit[31] select NAND or eMMC function
>> Bit[25] enable SDIO IRQ
>> Bit[24] Clock always on
>> Bit[15:14] SRAM Power down
>>
>> [1]
>> https://lkml.kernel.org/r/CAFBinCBeyXf6LNaZzAw6WnsxzDAv8E=Yp2eem0xCPWMEUi6pnQ@mail.gmail.com
>>
>>> Don't you need a patch removing the clock code
>>> from within the emmc driver? It's not even using regmap, so using
>>> regmap here doesn't help.
>>>
>> No, and current eMMC driver still use iomap to access the register
> 
> Which means a read-modify-write can corrupt the register value if both
> users don't access thru regmap. Changes are probably infrequent enough
> that you get lucky...
> 
What's you says here is true.
and we try to guarantee that only one of NAND or eMMC is enabled, so no
race condition, as a example of the use cases:

1) for enabling NAND driver, we do
   a) enable both mmc-clkc, and NAND driver in DT, they can access
register by using regmap interface
   b) disable eMMC DT node

2) for enabling eMMC driver, we do
   a) enable eMMC node, access register by using iomap (for now)
   b) disable both mmc-clkc and NAND in DT


>> I think we probably would like to take two steps approach.
>> first, from the hardware perspective, the NAND and eMMC(port C) driver
>> can't exist at same time, since they share the pins, clock, internal
>> ram, So we have to only enable one of NAND or eMMC in DT, not enable
>> both of them.
> 
> Yes, of course.
> 
>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
> 
> IMO, this should be done as part of merging this series. Otherwise, we
> have duplicated code for the same thing.

IMO, I'd leave this out of this series, since this patch series is quite
complete as itself. Although, the downside is code duplication.

Still, I need to hear Jerome, or Kevin's option, to see if or how we
should proceed the eMMC's clock conversion.

I could think of three option myself
1) don't do the conversion, downside is code duplication, upside is NO
DT change, no compatibility issue
2) add a syscon node into eMMC DT node, then only convert clock part
into this mmc-clkc model, while still leave other eMMC register access
as the usual iomap way (still no race condition)
3) convert all eMMC register access by using regmap interface.

both 2) and 3) need to update the DT.

and probably 2) is a compromise way, and 1) is also OK, 3) is probably
the worst way due to dramatically change (I think this was already
rejected in the previous discussion)


Yixun

Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller

[Kevin Hilman]

Yixun Lan <yixun.lan@amlogic.com> writes:

[...]

>> 
>>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
>> 
>> IMO, this should be done as part of merging this series. Otherwise, we
>> have duplicated code for the same thing.
>
> IMO, I'd leave this out of this series, since this patch series is quite
> complete as itself. Although, the downside is code duplication.
>
> Still, I need to hear Jerome, or Kevin's option, to see if or how we
> should proceed the eMMC's clock conversion.
>
> I could think of three option myself
> 1) don't do the conversion, downside is code duplication, upside is NO
> DT change, no compatibility issue
> 2) add a syscon node into eMMC DT node, then only convert clock part
> into this mmc-clkc model, while still leave other eMMC register access
> as the usual iomap way (still no race condition)
> 3) convert all eMMC register access by using regmap interface.
>
> both 2) and 3) need to update the DT.
>
> and probably 2) is a compromise way, and 1) is also OK, 3) is probably
> the worst way due to dramatically change (I think this was already
> rejected in the previous discussion)

Because the devices (NAND and eMMC_C) are mutually exclusive, taking the
step-by-step approach is fine (and preferred) by me.

Phase 1:
- add new mmc-clk provider
- add NAND driver using new mmc-clk provider
- boards using NAND should ensure emmc_c is disabled in DT

This allows us to not touch the MMC driver or existing upstream
bindings.  Yes, this means there is duplicate code in the MMC driver and
the new mmc-clk provider, but that can be removed in the next phase.

Phase 2:
- convert MMC driver to use new mmc-clk provider
- update MMC users in DT and bindings

Kevin

Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller

[Yixun Lan]

HI Kevin

On 07/23/2018 10:12 PM, Kevin Hilman wrote:
> Yixun Lan <yixun.lan@amlogic.com> writes:
> 
> [...]
> 
>>>
>>>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
>>>
>>> IMO, this should be done as part of merging this series. Otherwise, we
>>> have duplicated code for the same thing.
>>
>> IMO, I'd leave this out of this series, since this patch series is quite
>> complete as itself. Although, the downside is code duplication.
>>
>> Still, I need to hear Jerome, or Kevin's option, to see if or how we
>> should proceed the eMMC's clock conversion.
>>
>> I could think of three option myself
>> 1) don't do the conversion, downside is code duplication, upside is NO
>> DT change, no compatibility issue
>> 2) add a syscon node into eMMC DT node, then only convert clock part
>> into this mmc-clkc model, while still leave other eMMC register access
>> as the usual iomap way (still no race condition)
>> 3) convert all eMMC register access by using regmap interface.
>>
>> both 2) and 3) need to update the DT.
>>
>> and probably 2) is a compromise way, and 1) is also OK, 3) is probably
>> the worst way due to dramatically change (I think this was already
>> rejected in the previous discussion)
> 
> Because the devices (NAND and eMMC_C) are mutually exclusive, taking the
> step-by-step approach is fine (and preferred) by me.
> 
> Phase 1:
> - add new mmc-clk provider
> - add NAND driver using new mmc-clk provider
> - boards using NAND should ensure emmc_c is disabled in DT
> 
> This allows us to not touch the MMC driver or existing upstream
> bindings.  Yes, this means there is duplicate code in the MMC driver and
> the new mmc-clk provider, but that can be removed in the next phase.
> 
Great, the approach to address this issue is reasonable.
We'd like to focus on phase 1 first, thanks

> Phase 2:
> - convert MMC driver to use new mmc-clk provider
> - update MMC users in DT and bindings
> 
Ok.


Yixun

[PATCH v2 2/3] clk: meson: add sub MMC clock dt-bindings IDs

[Yixun Lan]

Add two clock bindings IDs which provided by the MMC clock controller,
These two clocks will be used by MMC or NAND driver.

Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 .../dt-bindings/clock/amlogic,meson-mmc-clkc.h   | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 include/dt-bindings/clock/amlogic,meson-mmc-clkc.h

diff --git a/include/dt-bindings/clock/amlogic,meson-mmc-clkc.h b/include/dt-bindings/clock/amlogic,meson-mmc-clkc.h
new file mode 100644
index 000000000000..2ae988ebc3ae
--- /dev/null
+++ b/include/dt-bindings/clock/amlogic,meson-mmc-clkc.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Meson MMC sub clock tree IDs
+ *
+ * Copyright (c) 2018 Amlogic, Inc. All rights reserved.
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ */
+
+#ifndef __MMC_CLKC_H
+#define __MMC_CLKC_H
+
+#define CLKID_MMC_DIV				1
+#define CLKID_MMC_PHASE_TX			3
+#define CLKID_MMC_PHASE_RX			4
+
+#endif
-- 
2.18.0

Re: [PATCH v2 2/3] clk: meson: add sub MMC clock dt-bindings IDs

[Rob Herring]

On Tue, Jul 10, 2018 at 04:36:57PM +0000, Yixun Lan wrote:
> Add two clock bindings IDs which provided by the MMC clock controller,
> These two clocks will be used by MMC or NAND driver.

I count 3 ids.

> 
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  .../dt-bindings/clock/amlogic,meson-mmc-clkc.h   | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 include/dt-bindings/clock/amlogic,meson-mmc-clkc.h

This can go with the binding patch.

> 
> diff --git a/include/dt-bindings/clock/amlogic,meson-mmc-clkc.h b/include/dt-bindings/clock/amlogic,meson-mmc-clkc.h
> new file mode 100644
> index 000000000000..2ae988ebc3ae
> --- /dev/null
> +++ b/include/dt-bindings/clock/amlogic,meson-mmc-clkc.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Meson MMC sub clock tree IDs
> + *
> + * Copyright (c) 2018 Amlogic, Inc. All rights reserved.
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + */
> +
> +#ifndef __MMC_CLKC_H
> +#define __MMC_CLKC_H
> +
> +#define CLKID_MMC_DIV				1
> +#define CLKID_MMC_PHASE_TX			3
> +#define CLKID_MMC_PHASE_RX			4
> +
> +#endif
> -- 
> 2.18.0
> 

Re: [PATCH v2 2/3] clk: meson: add sub MMC clock dt-bindings IDs

[Yixun Lan]

Hi Rob

On 07/12/18 03:45, Rob Herring wrote:
> On Tue, Jul 10, 2018 at 04:36:57PM +0000, Yixun Lan wrote:
>> Add two clock bindings IDs which provided by the MMC clock controller,
>> These two clocks will be used by MMC or NAND driver.
> 
> I count 3 ids.
I will update this

> 
>>
>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> ---
>>  .../dt-bindings/clock/amlogic,meson-mmc-clkc.h   | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>  create mode 100644 include/dt-bindings/clock/amlogic,meson-mmc-clkc.h
> 
> This can go with the binding patch.
> 
sure

.

[PATCH v2 3/3] clk: meson: add sub MMC clock controller driver

[Yixun Lan]

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/emmc-clkc.h header
can be used in the device tree sources.

Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/clk/meson/Kconfig    |   9 +
 drivers/clk/meson/Makefile   |   1 +
 drivers/clk/meson/mmc-clkc.c | 419 +++++++++++++++++++++++++++++++++++
 3 files changed, 429 insertions(+)
 create mode 100644 drivers/clk/meson/mmc-clkc.c

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index efaa70f682b4..edc18e65c89b 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -15,6 +15,15 @@ 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,
+	  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 72ec8c40d848..4b3817f80ba1 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/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
new file mode 100644
index 000000000000..43b7a376746d
--- /dev/null
+++ b/drivers/clk/meson/mmc-clkc.c
@@ -0,0 +1,419 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson MMC Sub Clock Controller Driver
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.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,meson-mmc-clkc.h>
+
+#include "clkc.h"
+
+/* clock ID used by internal driver */
+#define CLKID_MMC_MUX				0
+#define CLKID_MMC_PHASE_CORE			2
+
+#define SD_EMMC_CLOCK		0
+#define   CLK_DIV_MASK GENMASK(5, 0)
+#define   CLK_SRC_MASK GENMASK(7, 6)
+#define   CLK_CORE_PHASE_MASK GENMASK(9, 8)
+#define   CLK_TX_PHASE_MASK GENMASK(11, 10)
+#define   CLK_RX_PHASE_MASK GENMASK(13, 12)
+#define   CLK_V2_TX_DELAY_MASK GENMASK(19, 16)
+#define   CLK_V2_RX_DELAY_MASK GENMASK(23, 20)
+#define   CLK_V2_ALWAYS_ON BIT(24)
+
+#define   CLK_V3_TX_DELAY_MASK GENMASK(21, 16)
+#define   CLK_V3_RX_DELAY_MASK GENMASK(27, 22)
+#define   CLK_V3_ALWAYS_ON BIT(28)
+
+#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 clk_regmap_phase_data {
+	unsigned long			phase_mask;
+	unsigned long			delay_mask;
+	unsigned int			delay_step_ps;
+};
+
+struct mmc_clkc_data {
+	struct clk_regmap_phase_data	tx;
+	struct clk_regmap_phase_data	rx;
+};
+
+struct mmc_clkc_info {
+	struct device			*dev;
+	struct regmap			*map;
+	struct mmc_clkc_data		*data;
+};
+
+static inline struct clk_regmap_phase_data *
+clk_get_regmap_phase_data(struct clk_regmap *clk)
+{
+	return (struct clk_regmap_phase_data *)clk->data;
+}
+
+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 clk_regmap_phase_data mmc_clkc_core_phase = {
+	.phase_mask	= CLK_CORE_PHASE_MASK,
+};
+
+static const struct mmc_clkc_data mmc_clkc_gx_data = {
+	{
+		.phase_mask	= CLK_TX_PHASE_MASK,
+		.delay_mask	= CLK_V2_TX_DELAY_MASK,
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+	{
+		.phase_mask	= CLK_RX_PHASE_MASK,
+		.delay_mask	= CLK_V2_RX_DELAY_MASK,
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+};
+
+static const struct mmc_clkc_data mmc_clkc_axg_data = {
+	{
+		.phase_mask	= CLK_TX_PHASE_MASK,
+		.delay_mask	= CLK_V3_TX_DELAY_MASK,
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+	{
+		.phase_mask	= CLK_RX_PHASE_MASK,
+		.delay_mask	= CLK_V3_RX_DELAY_MASK,
+		.delay_step_ps	= CLK_DELAY_STEP_PS,
+	},
+};
+
+static const struct of_device_id mmc_clkc_match_table[] = {
+	{
+		.compatible = "amlogic,meson-gx-mmc-clkc",
+		.data = &mmc_clkc_gx_data
+	},
+	{
+		.compatible = "amlogic,meson-axg-mmc-clkc",
+		.data = &mmc_clkc_axg_data
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, mmc_clkc_match_table);
+
+static struct clk_regmap *mmc_clkc_register_mux(struct mmc_clkc_info *clkc)
+{
+	const char *parent_names[MUX_CLK_NUM_PARENTS];
+	struct device *dev = clkc->dev;
+	struct clk_init_data init;
+	struct clk_regmap *mux;
+	struct clk *clk;
+	char *mux_name;
+	int i, ret;
+
+	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return ERR_PTR(-ENOMEM);
+
+	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);
+	}
+
+	mux_name = kasprintf(GFP_KERNEL, "%s#mux", dev_name(dev));
+	if (!mux_name)
+		return ERR_PTR(-ENOMEM);
+
+	mux->map = clkc->map;
+	mux->data = &mmc_clkc_mux_data;
+
+	init.name = mux_name;
+	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->hw.init = &init;
+	ret = devm_clk_hw_register(dev, &mux->hw);
+	if (ret) {
+		dev_err(dev, "Mux clock registration failed\n");
+		mux = ERR_PTR(ret);
+	}
+
+	kfree(mux_name);
+	return mux;
+}
+
+static struct clk_regmap *mmc_clkc_register_div(struct mmc_clkc_info *clkc)
+{
+	const char *parent_names[MUX_CLK_NUM_PARENTS];
+	struct device *dev = clkc->dev;
+	struct clk_init_data init;
+	struct clk_regmap *div;
+	char *mux_name, *div_name;
+	int ret;
+
+	div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
+	if (!div)
+		return ERR_PTR(-ENOMEM);
+
+	mux_name = kasprintf(GFP_KERNEL, "%s#mux", dev_name(dev));
+	div_name = kasprintf(GFP_KERNEL, "%s#div", dev_name(dev));
+	if (!mux_name || !div_name) {
+		div = ERR_PTR(-ENOMEM);
+		goto err;
+	}
+
+	parent_names[0] = mux_name;
+	div->map = clkc->map;
+	div->data = &mmc_clkc_div_data;
+
+	init.name = div_name;
+	init.ops = &clk_regmap_divider_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	init.parent_names = parent_names;
+	init.num_parents = 1;
+
+	div->hw.init = &init;
+	ret = devm_clk_hw_register(dev, &div->hw);
+	if (ret) {
+		dev_err(dev, "Divider clock registration failed\n");
+		div = ERR_PTR(ret);
+	}
+
+err:
+	kfree(mux_name);
+	kfree(div_name);
+	return div;
+}
+
+static int clk_regmap_get_phase(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct clk_regmap_phase_data *ph = clk_get_regmap_phase_data(clk);
+	unsigned int phase_num = 1 <<  hweight_long(ph->phase_mask);
+	unsigned long period_ps, p, d;
+	int degrees;
+	u32 val;
+
+	regmap_read(clk->map, SD_EMMC_CLOCK, &val);
+	p = (val & ph->phase_mask) >> __ffs(ph->phase_mask);
+	degrees = p * 360 / phase_num;
+
+	if (ph->delay_mask) {
+		period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
+					 clk_get_rate(hw->clk));
+		d = (val & ph->delay_mask) >> __ffs(ph->delay_mask);
+		degrees += d * ph->delay_step_ps * 360 / period_ps;
+		degrees %= 360;
+	}
+
+	return degrees;
+}
+
+static void clk_regmap_apply_phase_delay(struct clk_regmap *clk,
+					 unsigned int phase,
+					 unsigned int delay)
+{
+	struct clk_regmap_phase_data *ph = clk->data;
+	u32 val;
+
+	regmap_read(clk->map, SD_EMMC_CLOCK, &val);
+
+	val &= ~ph->phase_mask;
+	val |= phase << __ffs(ph->phase_mask);
+
+	if (ph->delay_mask) {
+		val &= ~ph->delay_mask;
+		val |= delay << __ffs(ph->delay_mask);
+	}
+
+	regmap_write(clk->map, SD_EMMC_CLOCK, val);
+}
+
+static int clk_regmap_set_phase(struct clk_hw *hw, int degrees)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct clk_regmap_phase_data *ph = clk_get_regmap_phase_data(clk);
+	unsigned int phase_num = 1 <<  hweight_long(ph->phase_mask);
+	unsigned long period_ps, d = 0, r;
+	u64 p;
+
+	p = degrees % 360;
+
+	if (!ph->delay_mask) {
+		p = DIV_ROUND_CLOSEST_ULL(p, 360 / phase_num);
+	} else {
+		period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
+					 clk_get_rate(hw->clk));
+
+		/* First compute the phase index (p), the remainder (r) is the
+		 * part we'll try to acheive using the delays (d).
+		 */
+		r = do_div(p, 360 / phase_num);
+		d = DIV_ROUND_CLOSEST(r * period_ps,
+				      360 * ph->delay_step_ps);
+		d = min(d, ph->delay_mask >> __ffs(ph->delay_mask));
+	}
+
+	clk_regmap_apply_phase_delay(clk, p, d);
+	return 0;
+}
+
+static const struct clk_ops clk_regmap_phase_ops = {
+	.get_phase = clk_regmap_get_phase,
+	.set_phase = clk_regmap_set_phase,
+};
+
+static struct clk_regmap *
+mmc_clkc_register_phase_clk(struct mmc_clkc_info *clkc,
+			    char *name, char *parent_name, unsigned long flags,
+			    struct clk_regmap_phase_data *phase_data)
+
+{
+	const char *parent_names[MUX_CLK_NUM_PARENTS];
+	struct device *dev = clkc->dev;
+	struct clk_init_data init;
+	struct clk_regmap *x;
+	char *clk_name, *core_name;
+	int ret;
+
+	/* create the mmc rx clock */
+	x = devm_kzalloc(dev, sizeof(*x), GFP_KERNEL);
+	if (!x)
+		return ERR_PTR(-ENOMEM);
+
+	clk_name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), name);
+	core_name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), parent_name);
+
+	if (!clk_name || !core_name) {
+		x = ERR_PTR(-ENOMEM);
+		goto err;
+	}
+	parent_names[0] = core_name;
+
+	init.name = clk_name;
+	init.ops = &clk_regmap_phase_ops;
+	init.flags = flags;
+	init.parent_names = parent_names;
+	init.num_parents = 1;
+
+	x->map = clkc->map;
+	x->data = phase_data;
+	x->hw.init = &init;
+
+	ret = devm_clk_hw_register(dev, &x->hw);
+	if (ret) {
+		dev_err(dev, "Core %s clock registration failed\n", name);
+		x = ERR_PTR(ret);
+	}
+err:
+	kfree(clk_name);
+	kfree(core_name);
+	return x;
+}
+
+static int mmc_clkc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mmc_clkc_info *clkc;
+	struct clk_regmap *mux, *div, *core, *rx, *tx;
+	struct clk_hw_onecell_data *onecell_data;
+
+	clkc = devm_kzalloc(dev, sizeof(*clkc), GFP_KERNEL);
+	if (!clkc)
+		return -ENOMEM;
+
+	clkc->data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
+	if (!clkc->data)
+		return -EINVAL;
+
+	clkc->dev = dev;
+	clkc->map = syscon_node_to_regmap(dev->of_node);
+
+	if (IS_ERR(clkc->map)) {
+		dev_err(dev, "could not find mmc clock controller\n");
+		return PTR_ERR(clkc->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(clkc);
+	if (IS_ERR(mux))
+		return PTR_ERR(mux);
+
+	div = mmc_clkc_register_div(clkc);
+	if (IS_ERR(div))
+		return PTR_ERR(div);
+
+	core = mmc_clkc_register_phase_clk(clkc, "core", "div",
+					   CLK_SET_RATE_PARENT,
+					   &mmc_clkc_core_phase);
+	if (IS_ERR(core))
+		return PTR_ERR(core);
+
+	rx = mmc_clkc_register_phase_clk(clkc, "rx", "core", 0,
+					 &clkc->data->rx);
+	if (IS_ERR(rx))
+		return PTR_ERR(rx);
+
+	tx = mmc_clkc_register_phase_clk(clkc, "tx", "core", 0,
+					 &clkc->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);
-- 
2.18.0

Re: [PATCH v2 3/3] clk: meson: add sub MMC clock controller driver

[Jerome Brunet]

On Tue, 2018-07-10 at 16:36 +0000, Yixun Lan wrote:
> 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/emmc-clkc.h header
> can be used in the device tree sources.
> 
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  drivers/clk/meson/Kconfig    |   9 +
>  drivers/clk/meson/Makefile   |   1 +
>  drivers/clk/meson/mmc-clkc.c | 419 +++++++++++++++++++++++++++++++++++
>  3 files changed, 429 insertions(+)
>  create mode 100644 drivers/clk/meson/mmc-clkc.c
> 
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index efaa70f682b4..edc18e65c89b 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -15,6 +15,15 @@ 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,
> +	  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 72ec8c40d848..4b3817f80ba1 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/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> new file mode 100644
> index 000000000000..43b7a376746d
> --- /dev/null
> +++ b/drivers/clk/meson/mmc-clkc.c
> @@ -0,0 +1,419 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson MMC Sub Clock Controller Driver

 * Copyright (c) 2017 Baylibre SAS.
 * Author: Jerome Brunet <jbrunet@baylibre.com>

Considering that a fair share of the code below has been copied from the clock
portion of the eMMC driver, which I wrote last year.

> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.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,meson-mmc-clkc.h>
> +
> +#include "clkc.h"
> +
> +/* clock ID used by internal driver */
> +#define CLKID_MMC_MUX				0
> +#define CLKID_MMC_PHASE_CORE			2
> +
> +#define SD_EMMC_CLOCK		0
> +#define   CLK_DIV_MASK GENMASK(5, 0)
> +#define   CLK_SRC_MASK GENMASK(7, 6)
> +#define   CLK_CORE_PHASE_MASK GENMASK(9, 8)
> +#define   CLK_TX_PHASE_MASK GENMASK(11, 10)
> +#define   CLK_RX_PHASE_MASK GENMASK(13, 12)
> +#define   CLK_V2_TX_DELAY_MASK GENMASK(19, 16)
> +#define   CLK_V2_RX_DELAY_MASK GENMASK(23, 20)
> +#define   CLK_V2_ALWAYS_ON BIT(24)
> +
> +#define   CLK_V3_TX_DELAY_MASK GENMASK(21, 16)
> +#define   CLK_V3_RX_DELAY_MASK GENMASK(27, 22)
> +#define   CLK_V3_ALWAYS_ON BIT(28)
> +
> +#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

Some defines are aligned, some aren't. please be consistent about it.
I personally prefer when things are aligned but it is just a preference.  

> +
> +struct clk_regmap_phase_data {

Considering the recent addition of clk_phase in clk/meson, it is not the best
name to choose.

clk_phase_delay_data ?

> +	unsigned long			phase_mask;
> +	unsigned long			delay_mask;
> +	unsigned int			delay_step_ps;
> +};
> +
> +struct mmc_clkc_data {
> +	struct clk_regmap_phase_data	tx;
> +	struct clk_regmap_phase_data	rx;
> +};



> +
> +struct mmc_clkc_info {
> +	struct device			*dev;

I don't think you need keep dev here, considering this is the device data.

> +	struct regmap			*map;
> +	struct mmc_clkc_data		*data;
> +};

Overall, I don't think you need this structure

> +
> +static inline struct clk_regmap_phase_data *
> +clk_get_regmap_phase_data(struct clk_regmap *clk)

Update name as well

> +{
> +	return (struct clk_regmap_phase_data *)clk->data;
> +}
> +
> +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,
> +};

Same comment on alignement. Some structure you've aligned, and some you haven't
Consistency ...

> +
> +static struct clk_regmap_phase_data mmc_clkc_core_phase = {
> +	.phase_mask	= CLK_CORE_PHASE_MASK,
> +};
> +
> +static const struct mmc_clkc_data mmc_clkc_gx_data = {
> +	{
> +		.phase_mask	= CLK_TX_PHASE_MASK,
> +		.delay_mask	= CLK_V2_TX_DELAY_MASK,
> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
> +	},
> +	{
> +		.phase_mask	= CLK_RX_PHASE_MASK,
> +		.delay_mask	= CLK_V2_RX_DELAY_MASK,
> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
> +	},
> +};
> +
> +static const struct mmc_clkc_data mmc_clkc_axg_data = {
> +	{
> +		.phase_mask	= CLK_TX_PHASE_MASK,
> +		.delay_mask	= CLK_V3_TX_DELAY_MASK,
> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
> +	},
> +	{
> +		.phase_mask	= CLK_RX_PHASE_MASK,
> +		.delay_mask	= CLK_V3_RX_DELAY_MASK,
> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
> +	},
> +};
> +
> +static const struct of_device_id mmc_clkc_match_table[] = {
> +	{
> +		.compatible = "amlogic,meson-gx-mmc-clkc",
> +		.data = &mmc_clkc_gx_data
> +	},
> +	{
> +		.compatible = "amlogic,meson-axg-mmc-clkc",
> +		.data = &mmc_clkc_axg_data
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mmc_clkc_match_table);
> +
> +static struct clk_regmap *mmc_clkc_register_mux(struct mmc_clkc_info *clkc)
> +{
> +	const char *parent_names[MUX_CLK_NUM_PARENTS];
> +	struct device *dev = clkc->dev;
> +	struct clk_init_data init;
> +	struct clk_regmap *mux;
> +	struct clk *clk;
> +	char *mux_name;
> +	int i, ret;
> +
> +	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> +	if (!mux)
> +		return ERR_PTR(-ENOMEM);
> +
> +	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);
> +	}
> +
> +	mux_name = kasprintf(GFP_KERNEL, "%s#mux", dev_name(dev));
> +	if (!mux_name)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mux->map = clkc->map;
> +	mux->data = &mmc_clkc_mux_data;
> +
> +	init.name = mux_name;
> +	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->hw.init = &init;
> +	ret = devm_clk_hw_register(dev, &mux->hw);
> +	if (ret) {
> +		dev_err(dev, "Mux clock registration failed\n");
> +		mux = ERR_PTR(ret);
> +	}
> +
> +	kfree(mux_name);
> +	return mux;
> +}
> +
> +static struct clk_regmap *mmc_clkc_register_div(struct mmc_clkc_info *clkc)
> +{
> +	const char *parent_names[MUX_CLK_NUM_PARENTS];
> +	struct device *dev = clkc->dev;
> +	struct clk_init_data init;
> +	struct clk_regmap *div;
> +	char *mux_name, *div_name;
> +	int ret;
> +
> +	div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
> +	if (!div)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mux_name = kasprintf(GFP_KERNEL, "%s#mux", dev_name(dev));
> +	div_name = kasprintf(GFP_KERNEL, "%s#div", dev_name(dev));
> +	if (!mux_name || !div_name) {
> +		div = ERR_PTR(-ENOMEM);
> +		goto err;
> +	}
> +
> +	parent_names[0] = mux_name;
> +	div->map = clkc->map;
> +	div->data = &mmc_clkc_div_data;
> +
> +	init.name = div_name;
> +	init.ops = &clk_regmap_divider_ops;
> +	init.flags = CLK_SET_RATE_PARENT;
> +	init.parent_names = parent_names;
> +	init.num_parents = 1;
> +
> +	div->hw.init = &init;
> +	ret = devm_clk_hw_register(dev, &div->hw);
> +	if (ret) {
> +		dev_err(dev, "Divider clock registration failed\n");
> +		div = ERR_PTR(ret);
> +	}
> +
> +err:
> +	kfree(mux_name);
> +	kfree(div_name);
> +	return div;
> +}
> +
> +static int clk_regmap_get_phase(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct clk_regmap_phase_data *ph = clk_get_regmap_phase_data(clk);
> +	unsigned int phase_num = 1 <<  hweight_long(ph->phase_mask);
> +	unsigned long period_ps, p, d;
> +	int degrees;
> +	u32 val;
> +
> +	regmap_read(clk->map, SD_EMMC_CLOCK, &val);
> +	p = (val & ph->phase_mask) >> __ffs(ph->phase_mask);
> +	degrees = p * 360 / phase_num;
> +
> +	if (ph->delay_mask) {
> +		period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
> +					 clk_get_rate(hw->clk));
> +		d = (val & ph->delay_mask) >> __ffs(ph->delay_mask);
> +		degrees += d * ph->delay_step_ps * 360 / period_ps;
> +		degrees %= 360;
> +	}
> +
> +	return degrees;
> +}
> +
> +static void clk_regmap_apply_phase_delay(struct clk_regmap *clk,
> +					 unsigned int phase,
> +					 unsigned int delay)
> +{
> +	struct clk_regmap_phase_data *ph = clk->data;
> +	u32 val;
> +
> +	regmap_read(clk->map, SD_EMMC_CLOCK, &val);
> +
> +	val &= ~ph->phase_mask;
> +	val |= phase << __ffs(ph->phase_mask);
> +
> +	if (ph->delay_mask) {
> +		val &= ~ph->delay_mask;
> +		val |= delay << __ffs(ph->delay_mask);
> +	}
> +
> +	regmap_write(clk->map, SD_EMMC_CLOCK, val);
> +}
> +
> +static int clk_regmap_set_phase(struct clk_hw *hw, int degrees)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct clk_regmap_phase_data *ph = clk_get_regmap_phase_data(clk);
> +	unsigned int phase_num = 1 <<  hweight_long(ph->phase_mask);
> +	unsigned long period_ps, d = 0, r;
> +	u64 p;
> +
> +	p = degrees % 360;
> +
> +	if (!ph->delay_mask) {
> +		p = DIV_ROUND_CLOSEST_ULL(p, 360 / phase_num);
> +	} else {
> +		period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
> +					 clk_get_rate(hw->clk));
> +
> +		/* First compute the phase index (p), the remainder (r) is the
> +		 * part we'll try to acheive using the delays (d).
> +		 */
> +		r = do_div(p, 360 / phase_num);
> +		d = DIV_ROUND_CLOSEST(r * period_ps,
> +				      360 * ph->delay_step_ps);
> +		d = min(d, ph->delay_mask >> __ffs(ph->delay_mask));
> +	}
> +
> +	clk_regmap_apply_phase_delay(clk, p, d);
> +	return 0;
> +}
> +
> +static const struct clk_ops clk_regmap_phase_ops = {
> +	.get_phase = clk_regmap_get_phase,
> +	.set_phase = clk_regmap_set_phase,
> +};
> +
> +static struct clk_regmap *
> +mmc_clkc_register_phase_clk(struct mmc_clkc_info *clkc,
> +			    char *name, char *parent_name, unsigned long flags,
> +			    struct clk_regmap_phase_data *phase_data)
> +
> +{
> +	const char *parent_names[MUX_CLK_NUM_PARENTS];
> +	struct device *dev = clkc->dev;
> +	struct clk_init_data init;
> +	struct clk_regmap *x;

x is not a very good name, 'clk' is just 2 more letters ...

> +	char *clk_name, *core_name;
> +	int ret;
> +
> +	/* create the mmc rx clock */
> +	x = devm_kzalloc(dev, sizeof(*x), GFP_KERNEL);
> +	if (!x)
> +		return ERR_PTR(-ENOMEM);
> +
> +	clk_name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), name);
> +	core_name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), parent_name);
> +
> +	if (!clk_name || !core_name) {
> +		x = ERR_PTR(-ENOMEM);
> +		goto err;
> +	}
> +	parent_names[0] = core_name;
> +
> +	init.name = clk_name;
> +	init.ops = &clk_regmap_phase_ops;
> +	init.flags = flags;
> +	init.parent_names = parent_names;
> +	init.num_parents = 1;
> +
> +	x->map = clkc->map;
> +	x->data = phase_data;
> +	x->hw.init = &init;
> +
> +	ret = devm_clk_hw_register(dev, &x->hw);
> +	if (ret) {
> +		dev_err(dev, "Core %s clock registration failed\n", name);
> +		x = ERR_PTR(ret);
> +	}
> +err:
> +	kfree(clk_name);
> +	kfree(core_name);
> +	return x;
> +}

In all your mmc_clkc_register_xxx_() you should a pattern repeating itself. It
is something I pointed out in the previous version of your patchset

1. allocate a clk_regmap
2. allocate a clock name using dev name and a suffix.
3. register clk_hw
4. free clock name
5. return clk_regmap

You should be able to make an helper function out of this
Prototype will probably look like this:

static struct clk_regmap *
mmc_clkc_register_clk(struct device *dev, struct regmap *map,
		      struct clk_init_data *init, const char* suffix)

> +
> +static int mmc_clkc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mmc_clkc_info *clkc;
> +	struct clk_regmap *mux, *div, *core, *rx, *tx;
> +	struct clk_hw_onecell_data *onecell_data;
> +
> +	clkc = devm_kzalloc(dev, sizeof(*clkc), GFP_KERNEL);
> +	if (!clkc)
> +		return -ENOMEM;
> +
> +	clkc->data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
> +	if (!clkc->data)
> +		return -EINVAL;

Prefer ENODEV for this

> +
> +	clkc->dev = dev;
> +	clkc->map = syscon_node_to_regmap(dev->of_node);
> +
> +	if (IS_ERR(clkc->map)) {
> +		dev_err(dev, "could not find mmc clock controller\n");
> +		return PTR_ERR(clkc->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(clkc);
> +	if (IS_ERR(mux))
> +		return PTR_ERR(mux);
> +
> +	div = mmc_clkc_register_div(clkc);
> +	if (IS_ERR(div))
> +		return PTR_ERR(div);
> +
> +	core = mmc_clkc_register_phase_clk(clkc, "core", "div",
> +					   CLK_SET_RATE_PARENT,
> +					   &mmc_clkc_core_phase);
> +	if (IS_ERR(core))
> +		return PTR_ERR(core);
> +
> +	rx = mmc_clkc_register_phase_clk(clkc, "rx", "core", 0,
> +					 &clkc->data->rx);
> +	if (IS_ERR(rx))
> +		return PTR_ERR(rx);
> +
> +	tx = mmc_clkc_register_phase_clk(clkc, "tx", "core", 0,
> +					 &clkc->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);

Re: [PATCH v2 3/3] clk: meson: add sub MMC clock controller driver

[Yixun Lan]

Hi Jerome

thanks for the review

On 07/12/18 17:09, Jerome Brunet wrote:
> On Tue, 2018-07-10 at 16:36 +0000, Yixun Lan wrote:
>> 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/emmc-clkc.h header
>> can be used in the device tree sources.
>>
>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> ---
>>  drivers/clk/meson/Kconfig    |   9 +
>>  drivers/clk/meson/Makefile   |   1 +
>>  drivers/clk/meson/mmc-clkc.c | 419 +++++++++++++++++++++++++++++++++++
>>  3 files changed, 429 insertions(+)
>>  create mode 100644 drivers/clk/meson/mmc-clkc.c
>>
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index efaa70f682b4..edc18e65c89b 100644
>> --- a/drivers/clk/meson/Kconfig
>> +++ b/drivers/clk/meson/Kconfig
>> @@ -15,6 +15,15 @@ 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,
>> +	  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 72ec8c40d848..4b3817f80ba1 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/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>> new file mode 100644
>> index 000000000000..43b7a376746d
>> --- /dev/null
>> +++ b/drivers/clk/meson/mmc-clkc.c
>> @@ -0,0 +1,419 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson MMC Sub Clock Controller Driver
> 
>  * Copyright (c) 2017 Baylibre SAS.
>  * Author: Jerome Brunet <jbrunet@baylibre.com>
> 
> Considering that a fair share of the code below has been copied from the clock
> portion of the eMMC driver, which I wrote last year.
> 
Ok, fair enough, will fix in next version

>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.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,meson-mmc-clkc.h>
>> +
>> +#include "clkc.h"
>> +
>> +/* clock ID used by internal driver */
>> +#define CLKID_MMC_MUX				0
>> +#define CLKID_MMC_PHASE_CORE			2
>> +
>> +#define SD_EMMC_CLOCK		0
>> +#define   CLK_DIV_MASK GENMASK(5, 0)
>> +#define   CLK_SRC_MASK GENMASK(7, 6)
>> +#define   CLK_CORE_PHASE_MASK GENMASK(9, 8)
>> +#define   CLK_TX_PHASE_MASK GENMASK(11, 10)
>> +#define   CLK_RX_PHASE_MASK GENMASK(13, 12)
>> +#define   CLK_V2_TX_DELAY_MASK GENMASK(19, 16)
>> +#define   CLK_V2_RX_DELAY_MASK GENMASK(23, 20)
>> +#define   CLK_V2_ALWAYS_ON BIT(24)
>> +
>> +#define   CLK_V3_TX_DELAY_MASK GENMASK(21, 16)
>> +#define   CLK_V3_RX_DELAY_MASK GENMASK(27, 22)
>> +#define   CLK_V3_ALWAYS_ON BIT(28)
>> +
>> +#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
> 
> Some defines are aligned, some aren't. please be consistent about it.
> I personally prefer when things are aligned but it is just a preference.  
> 
sounds good to me, I can fix this in next version

>> +
>> +struct clk_regmap_phase_data {
> 
> Considering the recent addition of clk_phase in clk/meson, it is not the best
> name to choose.
> 
> clk_phase_delay_data ?
> 
ok

>> +	unsigned long			phase_mask;
>> +	unsigned long			delay_mask;
>> +	unsigned int			delay_step_ps;
>> +};
>> +
>> +struct mmc_clkc_data {
>> +	struct clk_regmap_phase_data	tx;
>> +	struct clk_regmap_phase_data	rx;
>> +};
> 
> 
> 
>> +
>> +struct mmc_clkc_info {
>> +	struct device			*dev;
> 
> I don't think you need keep dev here, considering this is the device data.
> 
>> +	struct regmap			*map;
>> +	struct mmc_clkc_data		*data;
>> +};
> 
> Overall, I don't think you need this structure
> 
ok, I could simplify this and drop this structure.

>> +
>> +static inline struct clk_regmap_phase_data *
>> +clk_get_regmap_phase_data(struct clk_regmap *clk)
> 
> Update name as well
> 
sure

>> +{
>> +	return (struct clk_regmap_phase_data *)clk->data;
>> +}
>> +
>> +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,
>> +};
> 
> Same comment on alignement. Some structure you've aligned, and some you haven't
> Consistency ...
> 
will fix

>> +
>> +static struct clk_regmap_phase_data mmc_clkc_core_phase = {
>> +	.phase_mask	= CLK_CORE_PHASE_MASK,
>> +};
>> +
>> +static const struct mmc_clkc_data mmc_clkc_gx_data = {
>> +	{
>> +		.phase_mask	= CLK_TX_PHASE_MASK,
>> +		.delay_mask	= CLK_V2_TX_DELAY_MASK,
>> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
>> +	},
>> +	{
>> +		.phase_mask	= CLK_RX_PHASE_MASK,
>> +		.delay_mask	= CLK_V2_RX_DELAY_MASK,
>> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
>> +	},
>> +};
>> +
>> +static const struct mmc_clkc_data mmc_clkc_axg_data = {
>> +	{
>> +		.phase_mask	= CLK_TX_PHASE_MASK,
>> +		.delay_mask	= CLK_V3_TX_DELAY_MASK,
>> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
>> +	},
>> +	{
>> +		.phase_mask	= CLK_RX_PHASE_MASK,
>> +		.delay_mask	= CLK_V3_RX_DELAY_MASK,
>> +		.delay_step_ps	= CLK_DELAY_STEP_PS,
>> +	},
>> +};
>> +
>> +static const struct of_device_id mmc_clkc_match_table[] = {
>> +	{
>> +		.compatible = "amlogic,meson-gx-mmc-clkc",
>> +		.data = &mmc_clkc_gx_data
>> +	},
>> +	{
>> +		.compatible = "amlogic,meson-axg-mmc-clkc",
>> +		.data = &mmc_clkc_axg_data
>> +	},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, mmc_clkc_match_table);
>> +
>> +static struct clk_regmap *mmc_clkc_register_mux(struct mmc_clkc_info *clkc)
>> +{
>> +	const char *parent_names[MUX_CLK_NUM_PARENTS];
>> +	struct device *dev = clkc->dev;
>> +	struct clk_init_data init;
>> +	struct clk_regmap *mux;
>> +	struct clk *clk;
>> +	char *mux_name;
>> +	int i, ret;
>> +
>> +	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
>> +	if (!mux)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	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);
>> +	}
>> +
>> +	mux_name = kasprintf(GFP_KERNEL, "%s#mux", dev_name(dev));
>> +	if (!mux_name)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	mux->map = clkc->map;
>> +	mux->data = &mmc_clkc_mux_data;
>> +
>> +	init.name = mux_name;
>> +	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->hw.init = &init;
>> +	ret = devm_clk_hw_register(dev, &mux->hw);
>> +	if (ret) {
>> +		dev_err(dev, "Mux clock registration failed\n");
>> +		mux = ERR_PTR(ret);
>> +	}
>> +
>> +	kfree(mux_name);
>> +	return mux;
>> +}
>> +
>> +static struct clk_regmap *mmc_clkc_register_div(struct mmc_clkc_info *clkc)
>> +{
>> +	const char *parent_names[MUX_CLK_NUM_PARENTS];
>> +	struct device *dev = clkc->dev;
>> +	struct clk_init_data init;
>> +	struct clk_regmap *div;
>> +	char *mux_name, *div_name;
>> +	int ret;
>> +
>> +	div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
>> +	if (!div)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	mux_name = kasprintf(GFP_KERNEL, "%s#mux", dev_name(dev));
>> +	div_name = kasprintf(GFP_KERNEL, "%s#div", dev_name(dev));
>> +	if (!mux_name || !div_name) {
>> +		div = ERR_PTR(-ENOMEM);
>> +		goto err;
>> +	}
>> +
>> +	parent_names[0] = mux_name;
>> +	div->map = clkc->map;
>> +	div->data = &mmc_clkc_div_data;
>> +
>> +	init.name = div_name;
>> +	init.ops = &clk_regmap_divider_ops;
>> +	init.flags = CLK_SET_RATE_PARENT;
>> +	init.parent_names = parent_names;
>> +	init.num_parents = 1;
>> +
>> +	div->hw.init = &init;
>> +	ret = devm_clk_hw_register(dev, &div->hw);
>> +	if (ret) {
>> +		dev_err(dev, "Divider clock registration failed\n");
>> +		div = ERR_PTR(ret);
>> +	}
>> +
>> +err:
>> +	kfree(mux_name);
>> +	kfree(div_name);
>> +	return div;
>> +}
>> +
>> +static int clk_regmap_get_phase(struct clk_hw *hw)
>> +{
>> +	struct clk_regmap *clk = to_clk_regmap(hw);
>> +	struct clk_regmap_phase_data *ph = clk_get_regmap_phase_data(clk);
>> +	unsigned int phase_num = 1 <<  hweight_long(ph->phase_mask);
>> +	unsigned long period_ps, p, d;
>> +	int degrees;
>> +	u32 val;
>> +
>> +	regmap_read(clk->map, SD_EMMC_CLOCK, &val);
>> +	p = (val & ph->phase_mask) >> __ffs(ph->phase_mask);
>> +	degrees = p * 360 / phase_num;
>> +
>> +	if (ph->delay_mask) {
>> +		period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
>> +					 clk_get_rate(hw->clk));
>> +		d = (val & ph->delay_mask) >> __ffs(ph->delay_mask);
>> +		degrees += d * ph->delay_step_ps * 360 / period_ps;
>> +		degrees %= 360;
>> +	}
>> +
>> +	return degrees;
>> +}
>> +
>> +static void clk_regmap_apply_phase_delay(struct clk_regmap *clk,
>> +					 unsigned int phase,
>> +					 unsigned int delay)
>> +{
>> +	struct clk_regmap_phase_data *ph = clk->data;
>> +	u32 val;
>> +
>> +	regmap_read(clk->map, SD_EMMC_CLOCK, &val);
>> +
>> +	val &= ~ph->phase_mask;
>> +	val |= phase << __ffs(ph->phase_mask);
>> +
>> +	if (ph->delay_mask) {
>> +		val &= ~ph->delay_mask;
>> +		val |= delay << __ffs(ph->delay_mask);
>> +	}
>> +
>> +	regmap_write(clk->map, SD_EMMC_CLOCK, val);
>> +}
>> +
>> +static int clk_regmap_set_phase(struct clk_hw *hw, int degrees)
>> +{
>> +	struct clk_regmap *clk = to_clk_regmap(hw);
>> +	struct clk_regmap_phase_data *ph = clk_get_regmap_phase_data(clk);
>> +	unsigned int phase_num = 1 <<  hweight_long(ph->phase_mask);
>> +	unsigned long period_ps, d = 0, r;
>> +	u64 p;
>> +
>> +	p = degrees % 360;
>> +
>> +	if (!ph->delay_mask) {
>> +		p = DIV_ROUND_CLOSEST_ULL(p, 360 / phase_num);
>> +	} else {
>> +		period_ps = DIV_ROUND_UP((unsigned long)NSEC_PER_SEC * 1000,
>> +					 clk_get_rate(hw->clk));
>> +
>> +		/* First compute the phase index (p), the remainder (r) is the
>> +		 * part we'll try to acheive using the delays (d).
>> +		 */
>> +		r = do_div(p, 360 / phase_num);
>> +		d = DIV_ROUND_CLOSEST(r * period_ps,
>> +				      360 * ph->delay_step_ps);
>> +		d = min(d, ph->delay_mask >> __ffs(ph->delay_mask));
>> +	}
>> +
>> +	clk_regmap_apply_phase_delay(clk, p, d);
>> +	return 0;
>> +}
>> +
>> +static const struct clk_ops clk_regmap_phase_ops = {
>> +	.get_phase = clk_regmap_get_phase,
>> +	.set_phase = clk_regmap_set_phase,
>> +};
>> +
>> +static struct clk_regmap *
>> +mmc_clkc_register_phase_clk(struct mmc_clkc_info *clkc,
>> +			    char *name, char *parent_name, unsigned long flags,
>> +			    struct clk_regmap_phase_data *phase_data)
>> +
>> +{
>> +	const char *parent_names[MUX_CLK_NUM_PARENTS];
>> +	struct device *dev = clkc->dev;
>> +	struct clk_init_data init;
>> +	struct clk_regmap *x;
> 
> x is not a very good name, 'clk' is just 2 more letters ...
> 
ok

>> +	char *clk_name, *core_name;
>> +	int ret;
>> +
>> +	/* create the mmc rx clock */
>> +	x = devm_kzalloc(dev, sizeof(*x), GFP_KERNEL);
>> +	if (!x)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	clk_name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), name);
>> +	core_name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), parent_name);
>> +
>> +	if (!clk_name || !core_name) {
>> +		x = ERR_PTR(-ENOMEM);
>> +		goto err;
>> +	}
>> +	parent_names[0] = core_name;
>> +
>> +	init.name = clk_name;
>> +	init.ops = &clk_regmap_phase_ops;
>> +	init.flags = flags;
>> +	init.parent_names = parent_names;
>> +	init.num_parents = 1;
>> +
>> +	x->map = clkc->map;
>> +	x->data = phase_data;
>> +	x->hw.init = &init;
>> +
>> +	ret = devm_clk_hw_register(dev, &x->hw);
>> +	if (ret) {
>> +		dev_err(dev, "Core %s clock registration failed\n", name);
>> +		x = ERR_PTR(ret);
>> +	}
>> +err:
>> +	kfree(clk_name);
>> +	kfree(core_name);
>> +	return x;
>> +}
> 
> In all your mmc_clkc_register_xxx_() you should a pattern repeating itself. It
> is something I pointed out in the previous version of your patchset
> 
I probably mis-understand your suggestion.. which I gave a try to
construct a helper..

I need to rethink about this.

> 1. allocate a clk_regmap
> 2. allocate a clock name using dev name and a suffix.
> 3. register clk_hw
> 4. free clock name
> 5. return clk_regmap
> 
> You should be able to make an helper function out of this
> Prototype will probably look like this:
> 
> static struct clk_regmap *
> mmc_clkc_register_clk(struct device *dev, struct regmap *map,
> 		      struct clk_init_data *init, const char* suffix)
> 
ok, thanks for the suggestion

>> +
>> +static int mmc_clkc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct mmc_clkc_info *clkc;
>> +	struct clk_regmap *mux, *div, *core, *rx, *tx;
>> +	struct clk_hw_onecell_data *onecell_data;
>> +
>> +	clkc = devm_kzalloc(dev, sizeof(*clkc), GFP_KERNEL);
>> +	if (!clkc)
>> +		return -ENOMEM;
>> +
>> +	clkc->data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
>> +	if (!clkc->data)
>> +		return -EINVAL;
> 
> Prefer ENODEV for this
> 
ok

>> +
>> +	clkc->dev = dev;
>> +	clkc->map = syscon_node_to_regmap(dev->of_node);
>> +
>> +	if (IS_ERR(clkc->map)) {
>> +		dev_err(dev, "could not find mmc clock controller\n");
>> +		return PTR_ERR(clkc->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(clkc);
>> +	if (IS_ERR(mux))
>> +		return PTR_ERR(mux);
>> +
>> +	div = mmc_clkc_register_div(clkc);
>> +	if (IS_ERR(div))
>> +		return PTR_ERR(div);
>> +
>> +	core = mmc_clkc_register_phase_clk(clkc, "core", "div",
>> +					   CLK_SET_RATE_PARENT,
>> +					   &mmc_clkc_core_phase);
>> +	if (IS_ERR(core))
>> +		return PTR_ERR(core);
>> +
>> +	rx = mmc_clkc_register_phase_clk(clkc, "rx", "core", 0,
>> +					 &clkc->data->rx);
>> +	if (IS_ERR(rx))
>> +		return PTR_ERR(rx);
>> +
>> +	tx = mmc_clkc_register_phase_clk(clkc, "tx", "core", 0,
>> +					 &clkc->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);
> 
> .
>