| Message ID | 1499336663-23875-4-git-send-email-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 06, 2017 at 12:24:23PM +0200, Neil Armstrong wrote: > On the first revision of the bindings, only the gates + resets were known > in the AO Clock HW, but more registers used to configures AO clock are known > to be spread among the AO register space. > This patch adds these registers to the Ao Clock bindings with direct access > and shared extcon access. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > .../devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) This looks like the binding might be too specific with a reg list of single registers, and you should define a system controller node instead. Depends on what else is in the "A0" block. Acked-by: Rob Herring <robh@kernel.org>
On 07/09, Rob Herring wrote: > On Thu, Jul 06, 2017 at 12:24:23PM +0200, Neil Armstrong wrote: > > On the first revision of the bindings, only the gates + resets were known > > in the AO Clock HW, but more registers used to configures AO clock are known > > to be spread among the AO register space. > > This patch adds these registers to the Ao Clock bindings with direct access > > and shared extcon access. > > > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > > --- > > .../devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > This looks like the binding might be too specific with a reg list of > single registers, and you should define a system controller node > instead. Depends on what else is in the "A0" block. > Agreed. Why can't we expand the size in DT and then access the registers directly in the driver. Hopefully it keeps working to apply the dts patch without the driver patch too (and vice-versa), because the kernel can only make a mapping as small as a page which would cover these newly added reg properties anyway.
On 07/21/2017 10:44 PM, Stephen Boyd wrote: > On 07/09, Rob Herring wrote: >> On Thu, Jul 06, 2017 at 12:24:23PM +0200, Neil Armstrong wrote: >>> On the first revision of the bindings, only the gates + resets were known >>> in the AO Clock HW, but more registers used to configures AO clock are known >>> to be spread among the AO register space. >>> This patch adds these registers to the Ao Clock bindings with direct access >>> and shared extcon access. >>> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>> --- >>> .../devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> This looks like the binding might be too specific with a reg list of >> single registers, and you should define a system controller node >> instead. Depends on what else is in the "A0" block. >> > > Agreed. Why can't we expand the size in DT and then access the > registers directly in the driver. Hopefully it keeps working to > apply the dts patch without the driver patch too (and > vice-versa), because the kernel can only make a mapping as small > as a page which would cover these newly added reg properties > anyway. > Hi Rob, Stephen, Thanks for your feedback, but on these Amlogic platforms, the AO register bank is filled with interleaved registers used for various purposes. For instance, the first 1k of registers are : 0-c RTI_STATUS c-14 RTI_PWR_CNTL 14-1c PIN_MUX 1c RTI_STATUS 24-30 GPIO 30 JTAG_CONFIG 34 WD 38-40 CPU_CTRL 40 RTI_GEN_CTRL 44 CPU_CTRL 4c-58 TIMER 58 OSCIN 60 AHB2DDR ... And so on, and the clock related registers are split among this space. For sure, this could be declared as an system controller node, but this would imply completely re-designing the actual clock driver and drop the actual bindings. (and with re-writting the clock gate code to use regmap registers access) Anyway, I'm open to suggestions... Neil
On Mon, 2017-07-24 at 13:47 +0200, Neil Armstrong wrote: > On 07/21/2017 10:44 PM, Stephen Boyd wrote: > > On 07/09, Rob Herring wrote: > > > On Thu, Jul 06, 2017 at 12:24:23PM +0200, Neil Armstrong wrote: > > > > On the first revision of the bindings, only the gates + resets were > > > > known > > > > in the AO Clock HW, but more registers used to configures AO clock are > > > > known > > > > to be spread among the AO register space. > > > > This patch adds these registers to the Ao Clock bindings with direct > > > > access > > > > and shared extcon access. > > > > > > > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > > > > --- > > > > .../devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt | 11 > > > > +++++++++-- > > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > This looks like the binding might be too specific with a reg list of > > > single registers, and you should define a system controller node > > > instead. Depends on what else is in the "A0" block. > > > > > > > Agreed. Why can't we expand the size in DT and then access the > > registers directly in the driver. Hopefully it keeps working to > > apply the dts patch without the driver patch too (and > > vice-versa), because the kernel can only make a mapping as small > > as a page which would cover these newly added reg properties > > anyway. > > > > Hi Rob, Stephen, > > Thanks for your feedback, but on these Amlogic platforms, the AO register bank > is > filled with interleaved registers used for various purposes. > > For instance, the first 1k of registers are : > > 0-c RTI_STATUS > c-14 RTI_PWR_CNTL > 14-1c PIN_MUX > 1c RTI_STATUS > 24-30 GPIO > 30 JTAG_CONFIG > 34 WD > 38-40 CPU_CTRL > 40 RTI_GEN_CTRL > 44 CPU_CTRL > 4c-58 TIMER > 58 OSCIN > 60 AHB2DDR > ... > > > And so on, and the clock related registers are split among this space. > > For sure, this could be declared as an system controller node, but this would > imply completely > re-designing the actual clock driver and drop the actual bindings. > (and with re-writting the clock gate code to use regmap registers access) Maybe it is time to investigate having the regmap clock from qcom available to every other platform ? > > Anyway, I'm open to suggestions... > > Neil
On 07/24, Jerome Brunet wrote: > On Mon, 2017-07-24 at 13:47 +0200, Neil Armstrong wrote: > > > > Hi Rob, Stephen, > > > > Thanks for your feedback, but on these Amlogic platforms, the AO register bank > > is > > filled with interleaved registers used for various purposes. > > > > For instance, the first 1k of registers are : > > > > 0-c RTI_STATUS > > c-14 RTI_PWR_CNTL > > 14-1c PIN_MUX > > 1c RTI_STATUS > > 24-30 GPIO > > 30 JTAG_CONFIG > > 34 WD > > 38-40 CPU_CTRL > > 40 RTI_GEN_CTRL > > 44 CPU_CTRL > > 4c-58 TIMER > > 58 OSCIN > > 60 AHB2DDR > > ... > > > > > > And so on, and the clock related registers are split among this space. > > > > For sure, this could be declared as an system controller node, but this would > > imply completely > > re-designing the actual clock driver and drop the actual bindings. > > (and with re-writting the clock gate code to use regmap registers access) I'm not suggesting a sycon node or binding usage here. There's an "always on" hw block here that could be implemented as an MFD driver that binds and creates a clk subdevice and whatever else is sitting in here. Those subdevices could be informed of the register base by knowing their parent driver is an MFD with a certain driver data structure inside and then get at an __iomem pointer through the parent's driver data. Or they could use a regmap approach and rewrite a bunch of code. Or there could be just a clk driver that binds to this node for now until a later time that other features are needed. > > Maybe it is time to investigate having the regmap clock from qcom available to > every other platform ? I think we have regmap clk duplicated a couple times in the drivers/clk/ directory now. Not sure how this is related, except for that there looks to be a desire to use a syscon binding here and that forces regmap on drivers?
On Tue, 2017-07-25 at 17:48 -0700, Stephen Boyd wrote: > > > > Maybe it is time to investigate having the regmap clock from qcom available > > to > > every other platform ? > > I think we have regmap clk duplicated a couple times in the > drivers/clk/ directory now. Which is why we may start thinking of common and generic "regmap" compatible solution in the CCF, at least for things like gates, dividers and muxes The approach used in qcom with regmap clocks could be a candidate for this, don't you think ? > Not sure how this is related, except > for that there looks to be a desire to use a syscon binding here > and that forces regmap on drivers? Syscon has been created exactly for this case where you want to create an mfd just for sharing a register region between several, otherwise well separated, devices, isn't it ?
diff --git a/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt b/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt index a55d31b..5c5ccec 100644 --- a/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt +++ b/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt @@ -7,7 +7,10 @@ Required Properties: - compatible: should be "amlogic,gxbb-aoclkc" - reg: physical base address of the clock controller and length of memory - mapped region. + mapped region for each registers listed in reg-names. +- reg-names: should contain the following register names : + "aoclk", "aocrt" and "aortc". +- amlogic,pwr-ctrl: A phandle to the AO Power Control node. - #clock-cells: should be 1. @@ -27,9 +30,13 @@ Example: AO Clock controller node: clkc_AO: clock-controller@040 { compatible = "amlogic,gxbb-aoclkc"; - reg = <0x0 0x040 0x0 0x4>; + reg = <0x0 0x00040 0x0 0x4>, + <0x0 0x00068 0x0 0x4>, + <0x0 0x00094 0x0 0x8>; + reg-names = "aoclk", "aocrt", "aortc"; #clock-cells = <1>; #reset-cells = <1>; + amlogic,pwr-ctrl = <&pwr_AO>; }; Example: UART controller node that consumes the clock and reset generated
On the first revision of the bindings, only the gates + resets were known in the AO Clock HW, but more registers used to configures AO clock are known to be spread among the AO register space. This patch adds these registers to the Ao Clock bindings with direct access and shared extcon access. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- .../devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)