Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
HI Kevin On 07/23/2018 10:12 PM, Kevin Hilman wrote: > Yixun Lan 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
Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
HI Kevin On 07/23/2018 10:12 PM, Kevin Hilman wrote: > Yixun Lan 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
Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
Yixun Lan 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 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
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 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 wrote: Hi Rob see my comments On 07/12/18 03:43, Rob Herring wrote: > On Tue, Jul 10, 2018 at 04:36:56PM +, 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 >> --- >> .../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 ..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=yp2eem0xcpwmeui6...@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
Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
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 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 wrote: Hi Rob see my comments On 07/12/18 03:43, Rob Herring wrote: > On Tue, Jul 10, 2018 at 04:36:56PM +, 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 >> --- >> .../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 ..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=yp2eem0xcpwmeui6...@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
Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
On Thu, Jul 12, 2018 at 5:29 PM Yixun Lan 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 wrote: > >> > >> Hi Rob > >> > >> see my comments > >> > >> On 07/12/18 03:43, Rob Herring wrote: > >>> On Tue, Jul 10, 2018 at 04:36:56PM +, 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 > --- > .../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 ..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=yp2eem0xcpwmeui6...@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
On Thu, Jul 12, 2018 at 5:29 PM Yixun Lan 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 wrote: > >> > >> Hi Rob > >> > >> see my comments > >> > >> On 07/12/18 03:43, Rob Herring wrote: > >>> On Tue, Jul 10, 2018 at 04:36:56PM +, 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 > --- > .../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 ..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=yp2eem0xcpwmeui6...@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
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 wrote: >> >> Hi Rob >> >> see my comments >> >> On 07/12/18 03:43, Rob Herring wrote: >>> On Tue, Jul 10, 2018 at 04:36:56PM +, 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 --- .../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 ..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=yp2eem0xcpwmeui6...@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
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 wrote: >> >> Hi Rob >> >> see my comments >> >> On 07/12/18 03:43, Rob Herring wrote: >>> On Tue, Jul 10, 2018 at 04:36:56PM +, 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 --- .../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 ..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=yp2eem0xcpwmeui6...@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
On Wed, Jul 11, 2018 at 8:47 PM Yixun Lan wrote: > > Hi Rob > > see my comments > > On 07/12/18 03:43, Rob Herring wrote: > > On Tue, Jul 10, 2018 at 04:36:56PM +, 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 > >> --- > >> .../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 ..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
On Wed, Jul 11, 2018 at 8:47 PM Yixun Lan wrote: > > Hi Rob > > see my comments > > On 07/12/18 03:43, Rob Herring wrote: > > On Tue, Jul 10, 2018 at 04:36:56PM +, 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 > >> --- > >> .../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 ..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
Hi Rob
see my comments
On 07/12/18 03:43, Rob Herring wrote:
> On Tue, Jul 10, 2018 at 04:36:56PM +, 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
>> ---
>> .../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 ..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 = < CLKID_SD_MMC_C_CLK0>,
>> +< CLKID_FCLK_DIV2>;
>> +};
>> --
>> 2.18.0
>>
>
> .
>
Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
Hi Rob
see my comments
On 07/12/18 03:43, Rob Herring wrote:
> On Tue, Jul 10, 2018 at 04:36:56PM +, 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
>> ---
>> .../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 ..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 = < CLKID_SD_MMC_C_CLK0>,
>> +< CLKID_FCLK_DIV2>;
>> +};
>> --
>> 2.18.0
>>
>
> .
>
Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
On Tue, Jul 10, 2018 at 04:36:56PM +, 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
> ---
> .../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 ..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 = < CLKID_SD_MMC_C_CLK0>,
> + < CLKID_FCLK_DIV2>;
> +};
> --
> 2.18.0
>
Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
On Tue, Jul 10, 2018 at 04:36:56PM +, 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
> ---
> .../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 ..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 = < CLKID_SD_MMC_C_CLK0>,
> + < CLKID_FCLK_DIV2>;
> +};
> --
> 2.18.0
>
