| Message ID | 20180315115545.1884-2-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 15, 2018 at 12:55:42PM +0100, Jerome Brunet wrote: > The HHI register region hosts more than just clocks and needs to > accessed drivers other than the clock controller, such as the display > driver. > > This register region should be managed by syscon. It is already the case > on gxbb/gxl and it soon will be on axg. The clock controllers must use > this system controller instead of directly mapping the registers. Sounds like a kernel problem, not a DT one. > > This changes the bindings of gxbb and axg's clock controllers. This is > due to an initial 'incomplete' knowledge of these SoCs, which is why the > meson bindings are unstable ATM. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > .../devicetree/bindings/clock/amlogic,gxbb-clkc.txt | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt > index e2b377ed6f91..e950599566a9 100644 > --- a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt > +++ b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt > @@ -10,9 +10,6 @@ Required Properties: > "amlogic,gxl-clkc" for GXL and GXM SoC, > "amlogic,axg-clkc" for AXG SoC. > > -- reg: physical base address of the clock controller and length of memory > - mapped region. > - > - #clock-cells: should be 1. > > Each clock is assigned an identifier and client nodes can use this identifier > @@ -20,13 +17,22 @@ to specify the clock which they consume. All available clocks are defined as > preprocessor macros in the dt-bindings/clock/gxbb-clkc.h header and can be > used in device tree sources. > > +Parent node should have the following properties : > +- compatible: "syscon", "simple-mfd, and "amlogic,meson-gx-hhi-sysctrl" or > + "amlogic,meson-axg-hhi-sysctrl" > +- reg: base address and size of the HHI system control register space. > + > Example: Clock controller node: > > - clkc: clock-controller@c883c000 { > +sysctrl: system-controller@0 { > + compatible = "amlogic,meson-gx-hhi-sysctrl", "syscon", "simple-mfd"; > + reg = <0 0 0 0x400>; > + > + clkc: clock-controller { With a single child, there is really no point to this change. A single node can provide multiple functions. Look at nodes that are both reset and clock providers. What other functions are there? Rob
On Sun, 2018-03-18 at 07:52 -0500, Rob Herring wrote: > On Thu, Mar 15, 2018 at 12:55:42PM +0100, Jerome Brunet wrote: > > The HHI register region hosts more than just clocks and needs to > > accessed drivers other than the clock controller, such as the display > > driver. > > > > This register region should be managed by syscon. It is already the case > > on gxbb/gxl and it soon will be on axg. The clock controllers must use > > this system controller instead of directly mapping the registers. > > Sounds like a kernel problem, not a DT one. It's a platform problem, so it has much a kernel problem (solution already merged) as a DT problem DT wise, we've got two devices mapping the same region in arch/arm64/boot/dts/amlogic/meson-gx.dtsi: > system-controller@0 which is used by the display device. and in arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi (same for gxl) > clock-controller@0 It has worked so far because the clock controller claims the region w/o reserving it but it remains unsafe since both device may access the same region. The fix is to have the clock controller go through the existing syscon. > > > > > With a single child, there is really no point to this change. A single > node can provide multiple functions. Look at nodes that are both reset > and clock providers. There is more than a single user, as explained above and in the cover letter of this series. > > What other functions are there? > > Rob
On Sun, Mar 18, 2018 at 10:09 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Sun, 2018-03-18 at 07:52 -0500, Rob Herring wrote: >> On Thu, Mar 15, 2018 at 12:55:42PM +0100, Jerome Brunet wrote: >> > The HHI register region hosts more than just clocks and needs to >> > accessed drivers other than the clock controller, such as the display >> > driver. >> > >> > This register region should be managed by syscon. It is already the case >> > on gxbb/gxl and it soon will be on axg. The clock controllers must use >> > this system controller instead of directly mapping the registers. >> >> Sounds like a kernel problem, not a DT one. > > It's a platform problem, so it has much a kernel problem (solution already > merged) as a DT problem > > DT wise, we've got two devices mapping the same region > > in arch/arm64/boot/dts/amlogic/meson-gx.dtsi: >> system-controller@0 > > which is used by the display device. > > and in arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi (same for gxl) >> clock-controller@0 > > It has worked so far because the clock controller claims the region w/o > reserving it but it remains unsafe since both device may access the same region. > The fix is to have the clock controller go through the existing syscon. We certainly want to avoid multiple nodes using the same mmio region. >> With a single child, there is really no point to this change. A single >> node can provide multiple functions. Look at nodes that are both reset >> and clock providers. > > There is more than a single user, as explained above and in the cover letter of > this series. Right, but a single node can support more than a single user (or provider). You don't have to have multiple nodes to have multiple users/providers. Rob
On 23/03/2018 04:04, Rob Herring wrote: > On Sun, Mar 18, 2018 at 10:09 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: >> On Sun, 2018-03-18 at 07:52 -0500, Rob Herring wrote: >>> On Thu, Mar 15, 2018 at 12:55:42PM +0100, Jerome Brunet wrote: >>>> The HHI register region hosts more than just clocks and needs to >>>> accessed drivers other than the clock controller, such as the display >>>> driver. >>>> >>>> This register region should be managed by syscon. It is already the case >>>> on gxbb/gxl and it soon will be on axg. The clock controllers must use >>>> this system controller instead of directly mapping the registers. >>> >>> Sounds like a kernel problem, not a DT one. >> >> It's a platform problem, so it has much a kernel problem (solution already >> merged) as a DT problem >> >> DT wise, we've got two devices mapping the same region >> >> in arch/arm64/boot/dts/amlogic/meson-gx.dtsi: >>> system-controller@0 >> >> which is used by the display device. >> >> and in arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi (same for gxl) >>> clock-controller@0 >> >> It has worked so far because the clock controller claims the region w/o >> reserving it but it remains unsafe since both device may access the same region. >> The fix is to have the clock controller go through the existing syscon. > > We certainly want to avoid multiple nodes using the same mmio region. > > >>> With a single child, there is really no point to this change. A single >>> node can provide multiple functions. Look at nodes that are both reset >>> and clock providers. >> >> There is more than a single user, as explained above and in the cover letter of >> this series. > > Right, but a single node can support more than a single user (or > provider). You don't have to have multiple nodes to have multiple > users/providers. > > Rob > Hi Rob, It's a question about user/provider, the syscon node represents the entire mmio region which has multiple sparse registers for multiple features, is used indirectly by the display driver, ... and has multiple features, non limited to, clock controller, multiple power domains,... and it is simpler to represent these as subnodes instead of a big fat node. Today, it is already represented like this within the Documentation/devicetree/bindings/power/amlogic,meson-gx-pwrc.txt So this binding is only an extension for the clock controller which was mis-architectured at the time due to a mis-knowledge of the HW architecture. Neil
diff --git a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt index e2b377ed6f91..e950599566a9 100644 --- a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt +++ b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt @@ -10,9 +10,6 @@ Required Properties: "amlogic,gxl-clkc" for GXL and GXM SoC, "amlogic,axg-clkc" for AXG SoC. -- reg: physical base address of the clock controller and length of memory - mapped region. - - #clock-cells: should be 1. Each clock is assigned an identifier and client nodes can use this identifier @@ -20,13 +17,22 @@ to specify the clock which they consume. All available clocks are defined as preprocessor macros in the dt-bindings/clock/gxbb-clkc.h header and can be used in device tree sources. +Parent node should have the following properties : +- compatible: "syscon", "simple-mfd, and "amlogic,meson-gx-hhi-sysctrl" or + "amlogic,meson-axg-hhi-sysctrl" +- reg: base address and size of the HHI system control register space. + Example: Clock controller node: - clkc: clock-controller@c883c000 { +sysctrl: system-controller@0 { + compatible = "amlogic,meson-gx-hhi-sysctrl", "syscon", "simple-mfd"; + reg = <0 0 0 0x400>; + + clkc: clock-controller { #clock-cells = <1>; compatible = "amlogic,gxbb-clkc"; - reg = <0x0 0xc883c000 0x0 0x3db>; }; +}; Example: UART controller node that consumes the clock generated by the clock controller:
The HHI register region hosts more than just clocks and needs to accessed drivers other than the clock controller, such as the display driver. This register region should be managed by syscon. It is already the case on gxbb/gxl and it soon will be on axg. The clock controllers must use this system controller instead of directly mapping the registers. This changes the bindings of gxbb and axg's clock controllers. This is due to an initial 'incomplete' knowledge of these SoCs, which is why the meson bindings are unstable ATM. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- .../devicetree/bindings/clock/amlogic,gxbb-clkc.txt | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)