* [PATCH 0/3] clk: meson: add a sub EMMC clock controller support @ 2018-07-03 14:57 Yixun Lan 2018-07-03 7:17 ` Boris Brezillon ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Yixun Lan @ 2018-07-03 14:57 UTC (permalink / raw) To: Jerome Brunet, Neil Armstrong Cc: Yixun Lan, Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Boris Brezillon, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel, devicetree This driver will add a EMMC 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. [1] https://lkml.kernel.org/r/20180628090034.0637a062@xps13 Yixun Lan (3): clk: meson: add DT documentation for emmc clock controller clk: meson: add sub EMMC clock dt-bindings IDs clk: meson: add sub EMMC clock controller driver .../bindings/clock/amlogic,emmc-clkc.txt | 45 ++++++ drivers/clk/meson/Kconfig | 9 ++ drivers/clk/meson/Makefile | 1 + drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++ include/dt-bindings/clock/emmc-clkc.h | 14 ++ 5 files changed, 205 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt create mode 100644 drivers/clk/meson/emmc-clkc.c create mode 100644 include/dt-bindings/clock/emmc-clkc.h -- 2.18.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] clk: meson: add a sub EMMC clock controller support 2018-07-03 14:57 [PATCH 0/3] clk: meson: add a sub EMMC clock controller support Yixun Lan @ 2018-07-03 7:17 ` Boris Brezillon 2018-07-03 14:57 ` [PATCH 1/3] clk: meson: add DT documentation for emmc clock controller Yixun Lan ` (2 subsequent siblings) 3 siblings, 0 replies; 17+ messages in thread From: Boris Brezillon @ 2018-07-03 7:17 UTC (permalink / raw) To: Yixun Lan Cc: Jerome Brunet, Neil Armstrong, Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel, devicetree On Tue, 3 Jul 2018 14:57:13 +0000 Yixun Lan <yixun.lan@amlogic.com> wrote: > This driver will add a EMMC 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. > > [1] https://lkml.kernel.org/r/20180628090034.0637a062@xps13 This is much better :-). > > Yixun Lan (3): > clk: meson: add DT documentation for emmc clock controller > clk: meson: add sub EMMC clock dt-bindings IDs > clk: meson: add sub EMMC clock controller driver > > .../bindings/clock/amlogic,emmc-clkc.txt | 45 ++++++ > drivers/clk/meson/Kconfig | 9 ++ > drivers/clk/meson/Makefile | 1 + > drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++ > include/dt-bindings/clock/emmc-clkc.h | 14 ++ > 5 files changed, 205 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt > create mode 100644 drivers/clk/meson/emmc-clkc.c > create mode 100644 include/dt-bindings/clock/emmc-clkc.h > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] clk: meson: add DT documentation for emmc clock controller 2018-07-03 14:57 [PATCH 0/3] clk: meson: add a sub EMMC clock controller support Yixun Lan 2018-07-03 7:17 ` Boris Brezillon @ 2018-07-03 14:57 ` Yixun Lan 2018-07-03 8:16 ` Jerome Brunet 2018-07-03 14:57 ` [PATCH 2/3] clk: meson: add sub EMMC clock dt-bindings IDs Yixun Lan 2018-07-03 14:57 ` [PATCH 3/3] clk: meson: add sub EMMC clock controller driver Yixun Lan 3 siblings, 1 reply; 17+ messages in thread From: Yixun Lan @ 2018-07-03 14:57 UTC (permalink / raw) To: Jerome Brunet, Neil Armstrong Cc: Yixun Lan, Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Boris Brezillon, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel, devicetree Document the EMMC sub clock controller driver, the potential consumer of this driver is EMMC or NAND. Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> --- .../bindings/clock/amlogic,emmc-clkc.txt | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt diff --git a/Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt new file mode 100644 index 000000000000..5534bd446363 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt @@ -0,0 +1,45 @@ +* Amlogic EMMC Sub Clock Controller Driver + +The Amlogic EMMC clock controller generates and supplies clock to support +EMMC and NAND controller + +Required Properties: + +- compatible: should be: + "amlogic,emmc-clkc" + +- #clock-cells: should be 1. + +Two clocks are provided as the parent of this EMMC clock controller driver from +upper layer clock controller - eg "amlogic,axg-clkc" in AXG platfrom. +The main consumer of this driver is EMMC or NAND, to specify which the clock +they may consume, the preprocessor macros in the dt-bindings/clock/emmc-clkc.h +header and can be used in device tree sources. + +Parent node should have the following properties : +- compatible: "syscon", "simple-mfd, and "amlogic,emmc-clkc" +- reg: base address and size of the EMMC control register space. + +Example: Clock controller node: + +sd_emmc_c_clkc: clock-controller@7000 { + compatible = "amlogic,emmc-clkc", "syscon", "simple-mfd"; + reg = <0x0 0x7000 0x0 0x4>; + #clock-cells = <1>; + + clock-names = "clkin0", "clkin1"; + clocks = <&clkc CLKID_SD_EMMC_C_CLK0>, + <&clkc CLKID_FCLK_DIV2>; +}; + +Example: NAND controller node that consumes the clock generated by the clock + controller: + + nand: nfc@7800 { + compatible = "amlogic,meson-axg-nfc"; + reg = <0x0 0x7800 0x0 0x100>; + interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>; + clocks = <&clkc CLKID_SD_EMMC_C>, + <&sd_emmc_c_clkc CLKID_EMMC_C_DIV>; + clock-names = "core", "device"; + }; -- 2.18.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] clk: meson: add DT documentation for emmc clock controller 2018-07-03 14:57 ` [PATCH 1/3] clk: meson: add DT documentation for emmc clock controller Yixun Lan @ 2018-07-03 8:16 ` Jerome Brunet 2018-07-03 9:59 ` Yixun Lan 0 siblings, 1 reply; 17+ messages in thread From: Jerome Brunet @ 2018-07-03 8:16 UTC (permalink / raw) To: Yixun Lan, Neil Armstrong Cc: Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Boris Brezillon, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel, devicetree On Tue, 2018-07-03 at 14:57 +0000, Yixun Lan wrote: > Document the EMMC sub clock controller driver, the potential consumer > of this driver is EMMC or NAND. > > Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> > --- > .../bindings/clock/amlogic,emmc-clkc.txt | 45 +++++++++++++++++++ > 1 file changed, 45 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt > > diff --git a/Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt > new file mode 100644 > index 000000000000..5534bd446363 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt > @@ -0,0 +1,45 @@ > +* Amlogic EMMC Sub Clock Controller Driver > + > +The Amlogic EMMC clock controller generates and supplies clock to support > +EMMC and NAND controller > + > +Required Properties: > + > +- compatible: should be: > + "amlogic,emmc-clkc" missing clocks and clock-names here > + > +- #clock-cells: should be 1. > + > +Two clocks are provided as the parent of this EMMC clock controller driver from > +upper layer clock controller - eg "amlogic,axg-clkc" in AXG platfrom. > +The main consumer of this driver is EMMC or NAND, to specify which the clock > +they may consume, the preprocessor macros in the dt-bindings/clock/emmc-clkc.h > +header and can be used in device tree sources. Not sure this explanation belongs in dt-binding documentation. Maybe this should be in the driver itself. > + > +Parent node should have the following properties : > +- compatible: "syscon", "simple-mfd, and "amlogic,emmc-clkc" > +- reg: base address and size of the EMMC control register space. > + > +Example: Clock controller node: > + > +sd_emmc_c_clkc: clock-controller@7000 { > + compatible = "amlogic,emmc-clkc", "syscon", "simple-mfd"; > + reg = <0x0 0x7000 0x0 0x4>; > + #clock-cells = <1>; > + > + clock-names = "clkin0", "clkin1"; > + clocks = <&clkc CLKID_SD_EMMC_C_CLK0>, > + <&clkc CLKID_FCLK_DIV2>; > +}; > + Not sure the following is necessary, you don't really to show how to use a clock controller. > +Example: NAND controller node that consumes the clock generated by the clock > + controller: > + > + nand: nfc@7800 { > + compatible = "amlogic,meson-axg-nfc"; > + reg = <0x0 0x7800 0x0 0x100>; > + interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>; > + clocks = <&clkc CLKID_SD_EMMC_C>, > + <&sd_emmc_c_clkc CLKID_EMMC_C_DIV>; > + clock-names = "core", "device"; > + }; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] clk: meson: add DT documentation for emmc clock controller 2018-07-03 8:16 ` Jerome Brunet @ 2018-07-03 9:59 ` Yixun Lan 0 siblings, 0 replies; 17+ messages in thread From: Yixun Lan @ 2018-07-03 9:59 UTC (permalink / raw) To: Jerome Brunet, Neil Armstrong Cc: yixun.lan, Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Boris Brezillon, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel, devicetree Hi jerome On 07/03/18 16:16, Jerome Brunet wrote: > On Tue, 2018-07-03 at 14:57 +0000, Yixun Lan wrote: >> Document the EMMC sub clock controller driver, the potential consumer >> of this driver is EMMC or NAND. >> >> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> >> --- >> .../bindings/clock/amlogic,emmc-clkc.txt | 45 +++++++++++++++++++ >> 1 file changed, 45 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt >> >> diff --git a/Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt >> new file mode 100644 >> index 000000000000..5534bd446363 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/amlogic,emmc-clkc.txt >> @@ -0,0 +1,45 @@ >> +* Amlogic EMMC Sub Clock Controller Driver >> + >> +The Amlogic EMMC clock controller generates and supplies clock to support >> +EMMC and NAND controller >> + >> +Required Properties: >> + >> +- compatible: should be: >> + "amlogic,emmc-clkc" > > missing clocks and clock-names here > ok, will update >> + >> +- #clock-cells: should be 1. >> + >> +Two clocks are provided as the parent of this EMMC clock controller driver from >> +upper layer clock controller - eg "amlogic,axg-clkc" in AXG platfrom. >> +The main consumer of this driver is EMMC or NAND, to specify which the clock >> +they may consume, the preprocessor macros in the dt-bindings/clock/emmc-clkc.h >> +header and can be used in device tree sources. > > Not sure this explanation belongs in dt-binding documentation. > Maybe this should be in the driver itself. > ok, I probably will waiting for more comments coming in ;-) >> + >> +Parent node should have the following properties : >> +- compatible: "syscon", "simple-mfd, and "amlogic,emmc-clkc" >> +- reg: base address and size of the EMMC control register space. >> + >> +Example: Clock controller node: >> + >> +sd_emmc_c_clkc: clock-controller@7000 { >> + compatible = "amlogic,emmc-clkc", "syscon", "simple-mfd"; >> + reg = <0x0 0x7000 0x0 0x4>; >> + #clock-cells = <1>; >> + >> + clock-names = "clkin0", "clkin1"; >> + clocks = <&clkc CLKID_SD_EMMC_C_CLK0>, >> + <&clkc CLKID_FCLK_DIV2>; >> +}; >> + > > Not sure the following is necessary, you don't really to show how to use a clock > controller. > ditto, and I'm fine with either way, this was a copy & paste from gxbb-clkc.h >> +Example: NAND controller node that consumes the clock generated by the clock >> + controller: >> + >> + nand: nfc@7800 { >> + compatible = "amlogic,meson-axg-nfc"; >> + reg = <0x0 0x7800 0x0 0x100>; >> + interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>; >> + clocks = <&clkc CLKID_SD_EMMC_C>, >> + <&sd_emmc_c_clkc CLKID_EMMC_C_DIV>; >> + clock-names = "core", "device"; >> + }; > > . > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] clk: meson: add sub EMMC clock dt-bindings IDs 2018-07-03 14:57 [PATCH 0/3] clk: meson: add a sub EMMC clock controller support Yixun Lan 2018-07-03 7:17 ` Boris Brezillon 2018-07-03 14:57 ` [PATCH 1/3] clk: meson: add DT documentation for emmc clock controller Yixun Lan @ 2018-07-03 14:57 ` Yixun Lan 2018-07-03 7:21 ` Boris Brezillon 2018-07-03 14:57 ` [PATCH 3/3] clk: meson: add sub EMMC clock controller driver Yixun Lan 3 siblings, 1 reply; 17+ messages in thread From: Yixun Lan @ 2018-07-03 14:57 UTC (permalink / raw) To: Jerome Brunet, Neil Armstrong Cc: Yixun Lan, Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Boris Brezillon, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel, devicetree Add two clock bindings IDs which provided by the EMMC clock controller, These two clocks will be used by EMMC or NAND driver. Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> --- include/dt-bindings/clock/emmc-clkc.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 include/dt-bindings/clock/emmc-clkc.h diff --git a/include/dt-bindings/clock/emmc-clkc.h b/include/dt-bindings/clock/emmc-clkc.h new file mode 100644 index 000000000000..d9972c400e58 --- /dev/null +++ b/include/dt-bindings/clock/emmc-clkc.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ +/* + * Meson EMMC sub clock tree IDs + * + * Copyright (c) 2018 Amlogic, Inc. All rights reserved. + */ + +#ifndef __EMMC_CLKC_H +#define __EMMC_CLKC_H + +#define CLKID_EMMC_C_MUX 0 +#define CLKID_EMMC_C_DIV 1 + +#endif -- 2.18.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] clk: meson: add sub EMMC clock dt-bindings IDs 2018-07-03 14:57 ` [PATCH 2/3] clk: meson: add sub EMMC clock dt-bindings IDs Yixun Lan @ 2018-07-03 7:21 ` Boris Brezillon 2018-07-03 7:36 ` Yixun Lan 0 siblings, 1 reply; 17+ messages in thread From: Boris Brezillon @ 2018-07-03 7:21 UTC (permalink / raw) To: Yixun Lan Cc: Jerome Brunet, Neil Armstrong, Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel, devicetree On Tue, 3 Jul 2018 14:57:15 +0000 Yixun Lan <yixun.lan@amlogic.com> wrote: > Add two clock bindings IDs which provided by the EMMC clock controller, > These two clocks will be used by EMMC or NAND driver. > > Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> > --- > include/dt-bindings/clock/emmc-clkc.h | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > create mode 100644 include/dt-bindings/clock/emmc-clkc.h > > diff --git a/include/dt-bindings/clock/emmc-clkc.h b/include/dt-bindings/clock/emmc-clkc.h > new file mode 100644 > index 000000000000..d9972c400e58 > --- /dev/null > +++ b/include/dt-bindings/clock/emmc-clkc.h > @@ -0,0 +1,14 @@ > +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ > +/* > + * Meson EMMC sub clock tree IDs > + * > + * Copyright (c) 2018 Amlogic, Inc. All rights reserved. > + */ > + > +#ifndef __EMMC_CLKC_H > +#define __EMMC_CLKC_H > + > +#define CLKID_EMMC_C_MUX 0 Looks like the MUX clk is the parent of the DIV one, and I guess the clk driver is able to select the best parent+div pair for a requested rate. Do you really need to expose the MUX to users? > +#define CLKID_EMMC_C_DIV 1 > + > +#endif ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] clk: meson: add sub EMMC clock dt-bindings IDs 2018-07-03 7:21 ` Boris Brezillon @ 2018-07-03 7:36 ` Yixun Lan 2018-07-03 8:09 ` Jerome Brunet 0 siblings, 1 reply; 17+ messages in thread From: Yixun Lan @ 2018-07-03 7:36 UTC (permalink / raw) To: Boris Brezillon Cc: yixun.lan, Jerome Brunet, Neil Armstrong, Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel, devicetree Hi Broris thanks for your quick response, and see my comments below On 07/03/18 15:21, Boris Brezillon wrote: > On Tue, 3 Jul 2018 14:57:15 +0000 > Yixun Lan <yixun.lan@amlogic.com> wrote: > >> Add two clock bindings IDs which provided by the EMMC clock controller, >> These two clocks will be used by EMMC or NAND driver. >> >> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> >> --- >> include/dt-bindings/clock/emmc-clkc.h | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> create mode 100644 include/dt-bindings/clock/emmc-clkc.h >> >> diff --git a/include/dt-bindings/clock/emmc-clkc.h b/include/dt-bindings/clock/emmc-clkc.h >> new file mode 100644 >> index 000000000000..d9972c400e58 >> --- /dev/null >> +++ b/include/dt-bindings/clock/emmc-clkc.h >> @@ -0,0 +1,14 @@ >> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >> +/* >> + * Meson EMMC sub clock tree IDs >> + * >> + * Copyright (c) 2018 Amlogic, Inc. All rights reserved. >> + */ >> + >> +#ifndef __EMMC_CLKC_H >> +#define __EMMC_CLKC_H >> + >> +#define CLKID_EMMC_C_MUX 0 > > Looks like the MUX clk is the parent of the DIV one, and I guess the clk > driver is able to select the best parent+div pair for a requested rate. > Do you really need to expose the MUX to users? > Yes, It's true, the mux is parent of the div clock. while testing for the NAND driver, I find it's kind of loose about the parent of the clock, so selecting the div (and let CCF decide freely) is actually works fine but for the EMMC driver, especially when running at high clock, it's kind of picky about the parent of the clock, so the driver may want to manually choose the parent of the mux clock (example fclk_div2 here). That's why I'm exporting this clock ID. >> +#define CLKID_EMMC_C_DIV 1 >> + >> +#endif > > . > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] clk: meson: add sub EMMC clock dt-bindings IDs 2018-07-03 7:36 ` Yixun Lan @ 2018-07-03 8:09 ` Jerome Brunet 2018-07-03 9:56 ` Yixun Lan 0 siblings, 1 reply; 17+ messages in thread From: Jerome Brunet @ 2018-07-03 8:09 UTC (permalink / raw) To: Yixun Lan, Boris Brezillon Cc: Neil Armstrong, Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel, devicetree On Tue, 2018-07-03 at 15:36 +0800, Yixun Lan wrote: > Hi Broris > > thanks for your quick response, and see my comments below > > On 07/03/18 15:21, Boris Brezillon wrote: > > On Tue, 3 Jul 2018 14:57:15 +0000 > > Yixun Lan <yixun.lan@amlogic.com> wrote: > > > > > Add two clock bindings IDs which provided by the EMMC clock controller, > > > These two clocks will be used by EMMC or NAND driver. > > > > > > Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> > > > --- > > > include/dt-bindings/clock/emmc-clkc.h | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > create mode 100644 include/dt-bindings/clock/emmc-clkc.h > > > > > > diff --git a/include/dt-bindings/clock/emmc-clkc.h b/include/dt-bindings/clock/emmc-clkc.h > > > new file mode 100644 > > > index 000000000000..d9972c400e58 > > > --- /dev/null > > > +++ b/include/dt-bindings/clock/emmc-clkc.h > > > @@ -0,0 +1,14 @@ > > > +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ > > > +/* > > > + * Meson EMMC sub clock tree IDs > > > + * > > > + * Copyright (c) 2018 Amlogic, Inc. All rights reserved. > > > + */ > > > + > > > +#ifndef __EMMC_CLKC_H > > > +#define __EMMC_CLKC_H > > > + > > > +#define CLKID_EMMC_C_MUX 0 > > > > Looks like the MUX clk is the parent of the DIV one, and I guess the clk > > driver is able to select the best parent+div pair for a requested rate. > > Do you really need to expose the MUX to users? > > > > Yes, It's true, the mux is parent of the div clock. > > while testing for the NAND driver, I find it's kind of loose about the > parent of the clock, so selecting the div (and let CCF decide freely) is > actually works fine > > but for the EMMC driver, especially when running at high clock, it's > kind of picky about the parent of the clock, It would be nice to get an explanation about this behavior. it seems that even of the rate provided by CLKID_SD_EMMC_X_CLK0 (main clock controller) is correct, the eMMC cannot reliably tune with it. Could you elaborate on this ? > so the driver may want to > manually choose the parent of the mux clock (example fclk_div2 here). > That's why I'm exporting this clock ID. ATM the EMMC driver will not use this provider. If this is the only reason, it could be done later. Is the NAND driver "picky" as well ? > > > > > +#define CLKID_EMMC_C_DIV 1 > > > + > > > +#endif > > > > . > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-clk" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] clk: meson: add sub EMMC clock dt-bindings IDs 2018-07-03 8:09 ` Jerome Brunet @ 2018-07-03 9:56 ` Yixun Lan 2018-07-03 10:01 ` Jerome Brunet 0 siblings, 1 reply; 17+ messages in thread From: Yixun Lan @ 2018-07-03 9:56 UTC (permalink / raw) To: Jerome Brunet, Boris Brezillon Cc: yixun.lan, Neil Armstrong, Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel, devicetree Hi Jerome: On 07/03/18 16:09, Jerome Brunet wrote: > On Tue, 2018-07-03 at 15:36 +0800, Yixun Lan wrote: >> Hi Broris >> >> thanks for your quick response, and see my comments below >> >> On 07/03/18 15:21, Boris Brezillon wrote: >>> On Tue, 3 Jul 2018 14:57:15 +0000 >>> Yixun Lan <yixun.lan@amlogic.com> wrote: >>> >>>> Add two clock bindings IDs which provided by the EMMC clock controller, >>>> These two clocks will be used by EMMC or NAND driver. >>>> >>>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> >>>> --- >>>> include/dt-bindings/clock/emmc-clkc.h | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> create mode 100644 include/dt-bindings/clock/emmc-clkc.h >>>> >>>> diff --git a/include/dt-bindings/clock/emmc-clkc.h b/include/dt-bindings/clock/emmc-clkc.h >>>> new file mode 100644 >>>> index 000000000000..d9972c400e58 >>>> --- /dev/null >>>> +++ b/include/dt-bindings/clock/emmc-clkc.h >>>> @@ -0,0 +1,14 @@ >>>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >>>> +/* >>>> + * Meson EMMC sub clock tree IDs >>>> + * >>>> + * Copyright (c) 2018 Amlogic, Inc. All rights reserved. >>>> + */ >>>> + >>>> +#ifndef __EMMC_CLKC_H >>>> +#define __EMMC_CLKC_H >>>> + >>>> +#define CLKID_EMMC_C_MUX 0 >>> >>> Looks like the MUX clk is the parent of the DIV one, and I guess the clk >>> driver is able to select the best parent+div pair for a requested rate. >>> Do you really need to expose the MUX to users? >>> >> >> Yes, It's true, the mux is parent of the div clock. >> >> while testing for the NAND driver, I find it's kind of loose about the >> parent of the clock, so selecting the div (and let CCF decide freely) is >> actually works fine >> >> but for the EMMC driver, especially when running at high clock, it's >> kind of picky about the parent of the clock, > > It would be nice to get an explanation about this behavior. > it seems that even of the rate provided by CLKID_SD_EMMC_X_CLK0 (main clock > controller) is correct, the eMMC cannot reliably tune with it. > > Could you elaborate on this ? > It's during my own test in AXG platform, I found clock path a) fclk_div2 -> sd_emmc_c_clk0_sel -> sd_emmc_c_clk0_div -> sd_emmc_c_clk0 -> sd_emmc_c_mux -> sd_emmc_c_div b) fclk_div2 -> sd_emmc_c_mux -> sd_emmc_c_div path a) doesn't work in EMMC driver, even both clock parent of them from the same fclk_div2 source. sd_emmc_c_mux -> sd_emmc_c_div is the clock from the EMMC register base. I believe it's ASIC design issue >> so the driver may want to >> manually choose the parent of the mux clock (example fclk_div2 here). >> That's why I'm exporting this clock ID. > > ATM the EMMC driver will not use this provider. If this is the only reason, it > could be done later. > sure, I'm fine with this.. we could certainly adjust it later. I will fix this in next patch version > Is the NAND driver "picky" as well ? > No, since the NAND is running at much low clock speed, and during my tests, it works fine with various clock parent >> >> >>>> +#define CLKID_EMMC_C_DIV 1 >>>> + >>>> +#endif >>> >>> . >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-clk" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > . > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] clk: meson: add sub EMMC clock dt-bindings IDs 2018-07-03 9:56 ` Yixun Lan @ 2018-07-03 10:01 ` Jerome Brunet 0 siblings, 0 replies; 17+ messages in thread From: Jerome Brunet @ 2018-07-03 10:01 UTC (permalink / raw) To: Yixun Lan, Boris Brezillon Cc: Neil Armstrong, Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel, devicetree On Tue, 2018-07-03 at 17:56 +0800, Yixun Lan wrote: > > > Yes, It's true, the mux is parent of the div clock. > > > > > > while testing for the NAND driver, I find it's kind of loose about the > > > parent of the clock, so selecting the div (and let CCF decide freely) is > > > actually works fine > > > > > > but for the EMMC driver, especially when running at high clock, it's > > > kind of picky about the parent of the clock, > > > > It would be nice to get an explanation about this behavior. > > it seems that even of the rate provided by CLKID_SD_EMMC_X_CLK0 (main clock > > controller) is correct, the eMMC cannot reliably tune with it. > > > > Could you elaborate on this ? > > > > It's during my own test in AXG platform, I found clock path > a) fclk_div2 -> sd_emmc_c_clk0_sel -> sd_emmc_c_clk0_div -> > sd_emmc_c_clk0 -> sd_emmc_c_mux -> sd_emmc_c_div > > b) fclk_div2 -> sd_emmc_c_mux -> sd_emmc_c_div > > path a) doesn't work in EMMC driver, even both clock parent of them from > the same fclk_div2 source. > > sd_emmc_c_mux -> sd_emmc_c_div is the clock from the EMMC register base. > I believe it's ASIC design issue yes Yixun, I did the same test. What I meant with this question is: could you confirm there a problem with this clock, and what it is exactly so we can adjust the clock as necessary. If FDIV2 entry to this clock is broken, maybe it should be removed. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] clk: meson: add sub EMMC clock controller driver 2018-07-03 14:57 [PATCH 0/3] clk: meson: add a sub EMMC clock controller support Yixun Lan ` (2 preceding siblings ...) 2018-07-03 14:57 ` [PATCH 2/3] clk: meson: add sub EMMC clock dt-bindings IDs Yixun Lan @ 2018-07-03 14:57 ` Yixun Lan 2018-07-03 8:51 ` Jerome Brunet 2018-07-03 18:58 ` Martin Blumenstingl 3 siblings, 2 replies; 17+ messages in thread From: Yixun Lan @ 2018-07-03 14:57 UTC (permalink / raw) To: Jerome Brunet, Neil Armstrong Cc: Yixun Lan, Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Boris Brezillon, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel This patch will add a EMMC clock controller driver support, It provide a mux and divider clock. This clock driver can be protentially used by either EMMC and NAND driver. Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> --- drivers/clk/meson/Kconfig | 9 +++ drivers/clk/meson/Makefile | 1 + drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++++++++++++++++++ 3 files changed, 146 insertions(+) create mode 100644 drivers/clk/meson/emmc-clkc.c diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig index efaa70f682b4..2f27ff08e4eb 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_EMMC_MESON + tristate "Meson EMMC Sub Clock Controller Driver" + depends on COMMON_CLK_AMLOGIC + select MFD_SYSCON + select REGMAP + help + Support for the EMMC 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..2f04f77ba4de 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_EMMC_MESON) += emmc-clkc.o obj-$(CONFIG_COMMON_CLK_REGMAP_MESON) += clk-regmap.o diff --git a/drivers/clk/meson/emmc-clkc.c b/drivers/clk/meson/emmc-clkc.c new file mode 100644 index 000000000000..cf5bb9f34327 --- /dev/null +++ b/drivers/clk/meson/emmc-clkc.c @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Amlogic Meson EMMC 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/init.h> +#include <linux/of_address.h> +#include <linux/of_device.h> +#include <linux/mfd/syscon.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#include <dt-bindings/clock/emmc-clkc.h> + +#include "clkc.h" + +#define SD_EMMC_CLOCK 0 +#define MUX_CLK_NUM_PARENTS 2 +#define EMMC_MAX_CLKS 2 +#define CLK_NAME_LEN 48 + +static struct clk_regmap_mux_data emmc_clkc_mux_data = { + .offset = SD_EMMC_CLOCK, + .mask = 0x3, + .shift = 6, +}; + +static struct clk_regmap_div_data emmc_clkc_div_data = { + .offset = SD_EMMC_CLOCK, + .shift = 0, + .width = 6, + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, +}; + +static const struct of_device_id clkc_match_table[] = { + { .compatible = "amlogic,emmc-clkc" }, + {} +}; + +static int emmc_clkc_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct clk_regmap *mux, *div; + struct regmap *map; + struct clk *clk; + int i, ret; + const char *parent_names[MUX_CLK_NUM_PARENTS]; + struct clk_init_data init; + char mux_name[CLK_NAME_LEN], div_name[CLK_NAME_LEN]; + struct clk_hw_onecell_data *onecell_data; + + map = syscon_node_to_regmap(dev->of_node); + + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL); + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL); + + onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) + + sizeof(*onecell_data->hws) * EMMC_MAX_CLKS, + GFP_KERNEL); + + if (!mux || !div || !onecell_data) + return -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 PTR_ERR(clk); + } + + parent_names[i] = __clk_get_name(clk); + } + + mux->map = map; + mux->data = &emmc_clkc_mux_data; + + snprintf(mux_name, CLK_NAME_LEN, "%s#emmc_mux", dev_name(dev)); + + 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"); + return ret; + } + + parent_names[0] = mux_name; + div->map = map; + div->data = &emmc_clkc_div_data; + + snprintf(div_name, CLK_NAME_LEN, "%s#emmc_div", dev_name(dev)); + + 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, "Div clock registration failed\n"); + return ret; + } + + onecell_data->hws[CLKID_EMMC_C_MUX] = &mux->hw, + onecell_data->hws[CLKID_EMMC_C_DIV] = &div->hw, + onecell_data->num = EMMC_MAX_CLKS; + + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, + onecell_data); +} + +static struct platform_driver emmc_clkc_driver = { + .probe = emmc_clkc_probe, + .driver = { + .name = "emmc-clkc", + .of_match_table = clkc_match_table, + }, +}; + +builtin_platform_driver(emmc_clkc_driver); -- 2.18.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] clk: meson: add sub EMMC clock controller driver 2018-07-03 14:57 ` [PATCH 3/3] clk: meson: add sub EMMC clock controller driver Yixun Lan @ 2018-07-03 8:51 ` Jerome Brunet 2018-07-03 9:56 ` Yixun Lan 2018-07-03 18:58 ` Martin Blumenstingl 1 sibling, 1 reply; 17+ messages in thread From: Jerome Brunet @ 2018-07-03 8:51 UTC (permalink / raw) To: Yixun Lan, Neil Armstrong Cc: Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Boris Brezillon, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel On Tue, 2018-07-03 at 14:57 +0000, Yixun Lan wrote: > This patch will add a EMMC clock controller driver support, > It provide a mux and divider clock. > > This clock driver can be protentially used by either EMMC and > NAND driver. > > Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> > --- > drivers/clk/meson/Kconfig | 9 +++ > drivers/clk/meson/Makefile | 1 + > drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++++++++++++++++++ > 3 files changed, 146 insertions(+) > create mode 100644 drivers/clk/meson/emmc-clkc.c > > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig > index efaa70f682b4..2f27ff08e4eb 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_EMMC_MESON > + tristate "Meson EMMC Sub Clock Controller Driver" > + depends on COMMON_CLK_AMLOGIC > + select MFD_SYSCON > + select REGMAP > + help > + Support for the EMMC sub clock controller on AmLogic Meson Platform, I thought you were not writing amlogic this way anymore -^ ?? ^ ... or be consistent about it. | | Drop the camel case please -------------| > + 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..2f04f77ba4de 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_EMMC_MESON) += emmc-clkc.o > obj-$(CONFIG_COMMON_CLK_REGMAP_MESON) += clk-regmap.o > diff --git a/drivers/clk/meson/emmc-clkc.c b/drivers/clk/meson/emmc-clkc.c > new file mode 100644 > index 000000000000..cf5bb9f34327 > --- /dev/null > +++ b/drivers/clk/meson/emmc-clkc.c > @@ -0,0 +1,136 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Amlogic Meson EMMC 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/init.h> replace with linux/module.h > +#include <linux/of_address.h> Why do you need this ? > +#include <linux/of_device.h> > +#include <linux/mfd/syscon.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include <dt-bindings/clock/emmc-clkc.h> > + > +#include "clkc.h" > + > +#define SD_EMMC_CLOCK 0 > +#define MUX_CLK_NUM_PARENTS 2 > +#define EMMC_MAX_CLKS 2 > +#define CLK_NAME_LEN 48 > + > +static struct clk_regmap_mux_data emmc_clkc_mux_data = { > + .offset = SD_EMMC_CLOCK, > + .mask = 0x3, > + .shift = 6, If you round the divider "closest", shouldn't you do the same for the mux ? > +}; > + > +static struct clk_regmap_div_data emmc_clkc_div_data = { > + .offset = SD_EMMC_CLOCK, > + .shift = 0, > + .width = 6, > + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, > +}; > + > +static const struct of_device_id clkc_match_table[] = { > + { .compatible = "amlogic,emmc-clkc" }, > + {} > +}; > + > +static int emmc_clkc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct clk_regmap *mux, *div; > + struct regmap *map; > + struct clk *clk; > + int i, ret; > + const char *parent_names[MUX_CLK_NUM_PARENTS]; > + struct clk_init_data init; > + char mux_name[CLK_NAME_LEN], div_name[CLK_NAME_LEN]; I'm not big fan, especially if you append the dev_name() to the clock name. > + struct clk_hw_onecell_data *onecell_data; > + > + map = syscon_node_to_regmap(dev->of_node); > + > + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL); > + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL); > + > + onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) + > + sizeof(*onecell_data->hws) * EMMC_MAX_CLKS, > + GFP_KERNEL); > + > + if (!mux || !div || !onecell_data) > + return -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 PTR_ERR(clk); > + } > + > + parent_names[i] = __clk_get_name(clk); > + } > + > + mux->map = map; > + mux->data = &emmc_clkc_mux_data; > + > + snprintf(mux_name, CLK_NAME_LEN, "%s#emmc_mux", dev_name(dev)); I'd prefer if you used kasprintf() and free the name after the name after registration. > + > + init.name = mux_name; > + init.ops = &clk_regmap_mux_ops; > + init.flags = CLK_SET_RATE_PARENT; If you really intend to have manual control over the mux, as commented in patch 2, you need CLK_SET_RATE_NOREPARENT here, otherwise whatever you set may be overwritten by the next clk_set_rate() call. Please choose. > + 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"); > + return ret; > + } > + > + parent_names[0] = mux_name; > + div->map = map; > + div->data = &emmc_clkc_div_data; > + > + snprintf(div_name, CLK_NAME_LEN, "%s#emmc_div", dev_name(dev)); > + > + 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, "Div clock registration failed\n"); s/Div/Divider > + return ret; > + } > + > + onecell_data->hws[CLKID_EMMC_C_MUX] = &mux->hw, > + onecell_data->hws[CLKID_EMMC_C_DIV] = &div->hw, > + onecell_data->num = EMMC_MAX_CLKS; > + > + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > + onecell_data); This is a long function with a lot of locals. please break that up. (for example, one function to register each clock, helper to allocate the clk_hw and names ...) > +} > + > +static struct platform_driver emmc_clkc_driver = { > + .probe = emmc_clkc_probe, > + .driver = { > + .name = "emmc-clkc", > + .of_match_table = clkc_match_table, > + }, > +}; > + It should be a module, not a builtin - especially when the configuration is a tristate > +builtin_platform_driver(emmc_clkc_driver); ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] clk: meson: add sub EMMC clock controller driver 2018-07-03 8:51 ` Jerome Brunet @ 2018-07-03 9:56 ` Yixun Lan 0 siblings, 0 replies; 17+ messages in thread From: Yixun Lan @ 2018-07-03 9:56 UTC (permalink / raw) To: Jerome Brunet, Neil Armstrong Cc: yixun.lan, Kevin Hilman, Carlo Caione, Michael Turquette, Stephen Boyd, Rob Herring, Miquel Raynal, Boris Brezillon, Martin Blumenstingl, Liang Yang, Qiufang Dai, Jian Hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel Hi Jerome: see my comments On 07/03/18 16:51, Jerome Brunet wrote: > On Tue, 2018-07-03 at 14:57 +0000, Yixun Lan wrote: >> This patch will add a EMMC clock controller driver support, >> It provide a mux and divider clock. >> >> This clock driver can be protentially used by either EMMC and >> NAND driver. >> >> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> >> --- >> drivers/clk/meson/Kconfig | 9 +++ >> drivers/clk/meson/Makefile | 1 + >> drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 146 insertions(+) >> create mode 100644 drivers/clk/meson/emmc-clkc.c >> >> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig >> index efaa70f682b4..2f27ff08e4eb 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_EMMC_MESON >> + tristate "Meson EMMC Sub Clock Controller Driver" >> + depends on COMMON_CLK_AMLOGIC >> + select MFD_SYSCON >> + select REGMAP >> + help >> + Support for the EMMC sub clock controller on AmLogic Meson Platform, > I thought you were not writing amlogic this way anymore -^ ?? ^ > ... or be consistent about it. | > | > Drop the camel case please -------------| oops, I'm not trying to find an excuse myself.. but this is copy & paste from COMMON_CLK_AXG_AUDIO section, so it's also need an update. I will fix in next version >> + 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..2f04f77ba4de 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_EMMC_MESON) += emmc-clkc.o >> obj-$(CONFIG_COMMON_CLK_REGMAP_MESON) += clk-regmap.o >> diff --git a/drivers/clk/meson/emmc-clkc.c b/drivers/clk/meson/emmc-clkc.c >> new file mode 100644 >> index 000000000000..cf5bb9f34327 >> --- /dev/null >> +++ b/drivers/clk/meson/emmc-clkc.c >> @@ -0,0 +1,136 @@ >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> +/* >> + * Amlogic Meson EMMC 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/init.h> > > replace with linux/module.h > ok >> +#include <linux/of_address.h> > > Why do you need this ? > this can be removed, I will update >> +#include <linux/of_device.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> + >> +#include <dt-bindings/clock/emmc-clkc.h> >> + >> +#include "clkc.h" >> + >> +#define SD_EMMC_CLOCK 0 >> +#define MUX_CLK_NUM_PARENTS 2 >> +#define EMMC_MAX_CLKS 2 >> +#define CLK_NAME_LEN 48 >> + >> +static struct clk_regmap_mux_data emmc_clkc_mux_data = { >> + .offset = SD_EMMC_CLOCK, >> + .mask = 0x3, >> + .shift = 6, > > If you round the divider "closest", shouldn't you do the same for the mux ? > >> +}; >> + >> +static struct clk_regmap_div_data emmc_clkc_div_data = { >> + .offset = SD_EMMC_CLOCK, >> + .shift = 0, >> + .width = 6, >> + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, >> +}; >> + >> +static const struct of_device_id clkc_match_table[] = { >> + { .compatible = "amlogic,emmc-clkc" }, >> + {} >> +}; >> + >> +static int emmc_clkc_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct clk_regmap *mux, *div; >> + struct regmap *map; >> + struct clk *clk; >> + int i, ret; >> + const char *parent_names[MUX_CLK_NUM_PARENTS]; >> + struct clk_init_data init; >> + char mux_name[CLK_NAME_LEN], div_name[CLK_NAME_LEN]; > > I'm not big fan, especially if you append the dev_name() to the clock name. > the main idea of appending dev_name() is : I want to make this driver work with SD_EMMC_A, SD_EMMC_B clock controller, not just SD_EMMC_C so basically we can have one driver, then populate three EMMC clock instance, and can distinguish from the dev_name() or if you have better suggestion, do speak up for this, I'm not sure if we really have plan to convert the EMMC driver to use this sub emmc clock driver. if the answer is NO, then I could even use static clock data to make this driver much simple (avoid using various devm_kzalloc..) >> + struct clk_hw_onecell_data *onecell_data; >> + >> + map = syscon_node_to_regmap(dev->of_node); >> + >> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL); >> + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL); >> + >> + onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) + >> + sizeof(*onecell_data->hws) * EMMC_MAX_CLKS, >> + GFP_KERNEL); >> + >> + if (!mux || !div || !onecell_data) >> + return -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 PTR_ERR(clk); >> + } >> + >> + parent_names[i] = __clk_get_name(clk); >> + } >> + >> + mux->map = map; >> + mux->data = &emmc_clkc_mux_data; >> + >> + snprintf(mux_name, CLK_NAME_LEN, "%s#emmc_mux", dev_name(dev)); > > I'd prefer if you used kasprintf() and free the name after the name after > registration. > Ok, I can do this >> + >> + init.name = mux_name; >> + init.ops = &clk_regmap_mux_ops; >> + init.flags = CLK_SET_RATE_PARENT; > > If you really intend to have manual control over the mux, as commented in patch > 2, you need CLK_SET_RATE_NOREPARENT here, otherwise whatever you set may be > overwritten by the next clk_set_rate() call. Please choose. > ok, thanks for pointing this >> + 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"); >> + return ret; >> + } >> + >> + parent_names[0] = mux_name; >> + div->map = map; >> + div->data = &emmc_clkc_div_data; >> + >> + snprintf(div_name, CLK_NAME_LEN, "%s#emmc_div", dev_name(dev)); >> + >> + 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, "Div clock registration failed\n"); > > s/Div/Divider > ok >> + return ret; >> + } >> + >> + onecell_data->hws[CLKID_EMMC_C_MUX] = &mux->hw, >> + onecell_data->hws[CLKID_EMMC_C_DIV] = &div->hw, >> + onecell_data->num = EMMC_MAX_CLKS; >> + >> + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, >> + onecell_data); > > This is a long function with a lot of locals. please break that up. > (for example, one function to register each clock, helper to allocate the clk_hw > and names ...) > ok >> +} >> + >> +static struct platform_driver emmc_clkc_driver = { >> + .probe = emmc_clkc_probe, >> + .driver = { >> + .name = "emmc-clkc", >> + .of_match_table = clkc_match_table, >> + }, >> +}; >> + > > It should be a module, not a builtin - especially when the configuration is a > tristate > >> +builtin_platform_driver(emmc_clkc_driver); Ok, I will fix this > > . > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] clk: meson: add sub EMMC clock controller driver 2018-07-03 14:57 ` [PATCH 3/3] clk: meson: add sub EMMC clock controller driver Yixun Lan 2018-07-03 8:51 ` Jerome Brunet @ 2018-07-03 18:58 ` Martin Blumenstingl 2018-07-04 7:17 ` Yixun Lan 1 sibling, 1 reply; 17+ messages in thread From: Martin Blumenstingl @ 2018-07-03 18:58 UTC (permalink / raw) To: yixun.lan Cc: jbrunet, Neil Armstrong, khilman, carlo, mturquette, sboyd, robh, miquel.raynal, boris.brezillon, liang.yang, qiufang.dai, jian.hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel Hi Yixun, apart from what Jerome found this looks good to me. one small "issue" and a question are inline below On Tue, Jul 3, 2018 at 9:00 AM Yixun Lan <yixun.lan@amlogic.com> wrote: > > This patch will add a EMMC clock controller driver support, > It provide a mux and divider clock. > > This clock driver can be protentially used by either EMMC and > NAND driver. > > Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> > --- > drivers/clk/meson/Kconfig | 9 +++ > drivers/clk/meson/Makefile | 1 + > drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++++++++++++++++++ > 3 files changed, 146 insertions(+) > create mode 100644 drivers/clk/meson/emmc-clkc.c > > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig > index efaa70f682b4..2f27ff08e4eb 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_EMMC_MESON > + tristate "Meson EMMC Sub Clock Controller Driver" > + depends on COMMON_CLK_AMLOGIC > + select MFD_SYSCON > + select REGMAP > + help > + Support for the EMMC 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..2f04f77ba4de 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_EMMC_MESON) += emmc-clkc.o > obj-$(CONFIG_COMMON_CLK_REGMAP_MESON) += clk-regmap.o > diff --git a/drivers/clk/meson/emmc-clkc.c b/drivers/clk/meson/emmc-clkc.c > new file mode 100644 > index 000000000000..cf5bb9f34327 > --- /dev/null > +++ b/drivers/clk/meson/emmc-clkc.c > @@ -0,0 +1,136 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Amlogic Meson EMMC 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/init.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/mfd/syscon.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include <dt-bindings/clock/emmc-clkc.h> > + > +#include "clkc.h" > + > +#define SD_EMMC_CLOCK 0 > +#define MUX_CLK_NUM_PARENTS 2 > +#define EMMC_MAX_CLKS 2 > +#define CLK_NAME_LEN 48 > + > +static struct clk_regmap_mux_data emmc_clkc_mux_data = { > + .offset = SD_EMMC_CLOCK, > + .mask = 0x3, > + .shift = 6, > +}; > + > +static struct clk_regmap_div_data emmc_clkc_div_data = { > + .offset = SD_EMMC_CLOCK, > + .shift = 0, > + .width = 6, > + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, > +}; > + > +static const struct of_device_id clkc_match_table[] = { > + { .compatible = "amlogic,emmc-clkc" }, shouldn't this be "amlogic,meson-axg-emmc-clkc" (and optionally also "amlogic,meson-gx-emmc-clkc")? > + {} > +}; > + > +static int emmc_clkc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct clk_regmap *mux, *div; > + struct regmap *map; > + struct clk *clk; > + int i, ret; > + const char *parent_names[MUX_CLK_NUM_PARENTS]; > + struct clk_init_data init; > + char mux_name[CLK_NAME_LEN], div_name[CLK_NAME_LEN]; > + struct clk_hw_onecell_data *onecell_data; > + > + map = syscon_node_to_regmap(dev->of_node); > + > + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL); > + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL); > + > + onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) + > + sizeof(*onecell_data->hws) * EMMC_MAX_CLKS, > + GFP_KERNEL); > + > + if (!mux || !div || !onecell_data) > + return -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 PTR_ERR(clk); > + } > + > + parent_names[i] = __clk_get_name(clk); > + } > + > + mux->map = map; > + mux->data = &emmc_clkc_mux_data; > + > + snprintf(mux_name, CLK_NAME_LEN, "%s#emmc_mux", dev_name(dev)); > + > + 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"); > + return ret; > + } > + > + parent_names[0] = mux_name; > + div->map = map; > + div->data = &emmc_clkc_div_data; > + > + snprintf(div_name, CLK_NAME_LEN, "%s#emmc_div", dev_name(dev)); > + > + 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, "Div clock registration failed\n"); > + return ret; > + } > + > + onecell_data->hws[CLKID_EMMC_C_MUX] = &mux->hw, > + onecell_data->hws[CLKID_EMMC_C_DIV] = &div->hw, > + onecell_data->num = EMMC_MAX_CLKS; you are describing the mux and the divider here however, meson-gx-mmc.c has a few more clock related bits: - CLK_CORE_PHASE_MASK - CLK_TX_PHASE_MASK - CLK_RX_PHASE_MASK - CLK_V2_TX_DELAY_MASK / CLK_V3_TX_DELAY_MASK - CLK_V2_RX_DELAY_MASK / CLK_V3_RX_DELAY_MASK - CLK_V2_ALWAYS_ON / CLK_V3_ALWAYS_ON are these used for the MMC clock or are some of these routed to the NAND pins as well? Regards Martin ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] clk: meson: add sub EMMC clock controller driver 2018-07-03 18:58 ` Martin Blumenstingl @ 2018-07-04 7:17 ` Yixun Lan 2018-07-04 8:07 ` Jerome Brunet 0 siblings, 1 reply; 17+ messages in thread From: Yixun Lan @ 2018-07-04 7:17 UTC (permalink / raw) To: Martin Blumenstingl Cc: yixun.lan, jbrunet, Neil Armstrong, khilman, carlo, mturquette, sboyd, robh, miquel.raynal, boris.brezillon, liang.yang, qiufang.dai, jian.hu, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel Hi Martin On 07/04/18 02:58, Martin Blumenstingl wrote: > Hi Yixun, > > apart from what Jerome found this looks good to me. > one small "issue" and a question are inline below > > On Tue, Jul 3, 2018 at 9:00 AM Yixun Lan <yixun.lan@amlogic.com> wrote: >> >> This patch will add a EMMC clock controller driver support, >> It provide a mux and divider clock. >> >> This clock driver can be protentially used by either EMMC and >> NAND driver. >> >> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com> >> --- >> drivers/clk/meson/Kconfig | 9 +++ >> drivers/clk/meson/Makefile | 1 + >> drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 146 insertions(+) >> create mode 100644 drivers/clk/meson/emmc-clkc.c >> >> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig >> index efaa70f682b4..2f27ff08e4eb 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_EMMC_MESON >> + tristate "Meson EMMC Sub Clock Controller Driver" >> + depends on COMMON_CLK_AMLOGIC >> + select MFD_SYSCON >> + select REGMAP >> + help >> + Support for the EMMC 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..2f04f77ba4de 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_EMMC_MESON) += emmc-clkc.o >> obj-$(CONFIG_COMMON_CLK_REGMAP_MESON) += clk-regmap.o >> diff --git a/drivers/clk/meson/emmc-clkc.c b/drivers/clk/meson/emmc-clkc.c >> new file mode 100644 >> index 000000000000..cf5bb9f34327 >> --- /dev/null >> +++ b/drivers/clk/meson/emmc-clkc.c >> @@ -0,0 +1,136 @@ >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> +/* >> + * Amlogic Meson EMMC 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/init.h> >> +#include <linux/of_address.h> >> +#include <linux/of_device.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> + >> +#include <dt-bindings/clock/emmc-clkc.h> >> + >> +#include "clkc.h" >> + >> +#define SD_EMMC_CLOCK 0 >> +#define MUX_CLK_NUM_PARENTS 2 >> +#define EMMC_MAX_CLKS 2 >> +#define CLK_NAME_LEN 48 >> + >> +static struct clk_regmap_mux_data emmc_clkc_mux_data = { >> + .offset = SD_EMMC_CLOCK, >> + .mask = 0x3, >> + .shift = 6, >> +}; >> + >> +static struct clk_regmap_div_data emmc_clkc_div_data = { >> + .offset = SD_EMMC_CLOCK, >> + .shift = 0, >> + .width = 6, >> + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, >> +}; >> + >> +static const struct of_device_id clkc_match_table[] = { >> + { .compatible = "amlogic,emmc-clkc" }, > shouldn't this be "amlogic,meson-axg-emmc-clkc" (and optionally also > "amlogic,meson-gx-emmc-clkc")? > sounds good to me.. is it a convention to always make the compatible specific ?.. >> + {} >> +}; >> + >> +static int emmc_clkc_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct clk_regmap *mux, *div; >> + struct regmap *map; >> + struct clk *clk; >> + int i, ret; >> + const char *parent_names[MUX_CLK_NUM_PARENTS]; >> + struct clk_init_data init; >> + char mux_name[CLK_NAME_LEN], div_name[CLK_NAME_LEN]; >> + struct clk_hw_onecell_data *onecell_data; >> + >> + map = syscon_node_to_regmap(dev->of_node); >> + >> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL); >> + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL); >> + >> + onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) + >> + sizeof(*onecell_data->hws) * EMMC_MAX_CLKS, >> + GFP_KERNEL); >> + >> + if (!mux || !div || !onecell_data) >> + return -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 PTR_ERR(clk); >> + } >> + >> + parent_names[i] = __clk_get_name(clk); >> + } >> + >> + mux->map = map; >> + mux->data = &emmc_clkc_mux_data; >> + >> + snprintf(mux_name, CLK_NAME_LEN, "%s#emmc_mux", dev_name(dev)); >> + >> + 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"); >> + return ret; >> + } >> + >> + parent_names[0] = mux_name; >> + div->map = map; >> + div->data = &emmc_clkc_div_data; >> + >> + snprintf(div_name, CLK_NAME_LEN, "%s#emmc_div", dev_name(dev)); >> + >> + 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, "Div clock registration failed\n"); >> + return ret; >> + } >> + >> + onecell_data->hws[CLKID_EMMC_C_MUX] = &mux->hw, >> + onecell_data->hws[CLKID_EMMC_C_DIV] = &div->hw, >> + onecell_data->num = EMMC_MAX_CLKS; > you are describing the mux and the divider here > however, meson-gx-mmc.c has a few more clock related bits: > - CLK_CORE_PHASE_MASK > - CLK_TX_PHASE_MASK > - CLK_RX_PHASE_MASK > - CLK_V2_TX_DELAY_MASK / CLK_V3_TX_DELAY_MASK > - CLK_V2_RX_DELAY_MASK / CLK_V3_RX_DELAY_MASK > - CLK_V2_ALWAYS_ON / CLK_V3_ALWAYS_ON > > are these used for the MMC clock or are some of these routed to the > NAND pins as well? There clocks are not used in NAND driver.. I understand your concern here, if there clocks are also routed to NAND pins, then we also need to implement them here actually, to answer your question, I need to query the ASIC team.. > > > Regards > Martin > > . > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] clk: meson: add sub EMMC clock controller driver 2018-07-04 7:17 ` Yixun Lan @ 2018-07-04 8:07 ` Jerome Brunet 0 siblings, 0 replies; 17+ messages in thread From: Jerome Brunet @ 2018-07-04 8:07 UTC (permalink / raw) To: Yixun Lan, Martin Blumenstingl Cc: robh, Neil Armstrong, sboyd, khilman, mturquette, linux-kernel, boris.brezillon, jian.hu, liang.yang, qiufang.dai, miquel.raynal, carlo, linux-amlogic, linux-clk, linux-arm-kernel On Wed, 2018-07-04 at 15:17 +0800, Yixun Lan wrote: > > you are describing the mux and the divider here > > however, meson-gx-mmc.c has a few more clock related bits: > > - CLK_CORE_PHASE_MASK > > - CLK_TX_PHASE_MASK > > - CLK_RX_PHASE_MASK > > - CLK_V2_TX_DELAY_MASK / CLK_V3_TX_DELAY_MASK > > - CLK_V2_RX_DELAY_MASK / CLK_V3_RX_DELAY_MASK > > - CLK_V2_ALWAYS_ON / CLK_V3_ALWAYS_ON > > > > are these used for the MMC clock or are some of these routed to the > > NAND pins as well? > > There clocks are not used in NAND driver.. > > I understand your concern here, if there clocks are also routed to NAND > pins, then we also need to implement them here > actually, to answer your question, I need to query the ASIC team.. Even if the NAND driver does not need to change the phases, it might need to make sure these phases are reset on init. It would not hurt to handle these phases in your clock controller. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-07-04 8:09 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-03 14:57 [PATCH 0/3] clk: meson: add a sub EMMC clock controller support Yixun Lan 2018-07-03 7:17 ` Boris Brezillon 2018-07-03 14:57 ` [PATCH 1/3] clk: meson: add DT documentation for emmc clock controller Yixun Lan 2018-07-03 8:16 ` Jerome Brunet 2018-07-03 9:59 ` Yixun Lan 2018-07-03 14:57 ` [PATCH 2/3] clk: meson: add sub EMMC clock dt-bindings IDs Yixun Lan 2018-07-03 7:21 ` Boris Brezillon 2018-07-03 7:36 ` Yixun Lan 2018-07-03 8:09 ` Jerome Brunet 2018-07-03 9:56 ` Yixun Lan 2018-07-03 10:01 ` Jerome Brunet 2018-07-03 14:57 ` [PATCH 3/3] clk: meson: add sub EMMC clock controller driver Yixun Lan 2018-07-03 8:51 ` Jerome Brunet 2018-07-03 9:56 ` Yixun Lan 2018-07-03 18:58 ` Martin Blumenstingl 2018-07-04 7:17 ` Yixun Lan 2018-07-04 8:07 ` Jerome Brunet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).