On Tue, Dec 13, 2016 at 12:12 AM, Andrew Jeffery <andrew@xxxxxxxx> wrote: > On Mon, 2016-12-12 at 10:27 -0600, Rob Herring wrote: >> On Tue, Dec 06, 2016 at 02:11:49PM +1100, Andrew Jeffery wrote: >> > The System Control Unit IP block in the Aspeed SoCs is typically where >> > the pinmux configuration is found, but not always. A number of pins >> > depend on state in one of LPC Host Control (LHC) or SoC Display >> > Controller (GFX) IP blocks, so the Aspeed pinmux drivers should have the >> > means to adjust these as necessary. >> > >> > We use syscon to cast a regmap over the GFX and LPC blocks, which is >> > used as an arbitration layer between the relevant driver and the pinctrl >> > subsystem. The regmaps are then exposed to the SoC-specific pinctrl >> > drivers by phandles in the devicetree, and are selected during a mux >> > request by querying a new 'ip' member in struct aspeed_sig_desc. >> > >> > > > Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx> >> > --- >> > .../devicetree/bindings/pinctrl/pinctrl-aspeed.txt | 50 ++++++- >> > drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c | 18 +-- >> > drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 48 ++++-- >> > drivers/pinctrl/aspeed/pinctrl-aspeed.c | 161 +++++++++++++-------- >> > drivers/pinctrl/aspeed/pinctrl-aspeed.h | 32 ++-- >> > 5 files changed, 214 insertions(+), 95 deletions(-) >> > >> > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt >> > index 2ad18c4ea55c..115b0cce6c1c 100644 >> > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt >> > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt >> > @@ -4,12 +4,19 @@ Aspeed Pin Controllers >> > The Aspeed SoCs vary in functionality inside a generation but have a common mux >> > device register layout. >> > >> > -Required properties: >> > -- compatible : Should be any one of the following: >> > > > - "aspeed,ast2400-pinctrl" >> > > > - "aspeed,g4-pinctrl" >> > > > - "aspeed,ast2500-pinctrl" >> > > > - "aspeed,g5-pinctrl" >> > +Required properties for g4: >> > > > +- compatible : Should be any one of the following: >> > > > + "aspeed,ast2400-pinctrl" >> > > > + "aspeed,g4-pinctrl" >> > + >> > +Required properties for g5: >> > > > +- compatible : Should be any one of the following: >> > > > + "aspeed,ast2500-pinctrl" >> > > > + "aspeed,g5-pinctrl" >> > + >> > > > +- aspeed,external-nodes: A cell of phandles to external controller nodes: >> > > > + 0: compatible with "aspeed,ast2500-gfx", "syscon" >> > > > + 1: compatible with "aspeed,ast2500-lpchc", "syscon" >> > >> > The pin controller node should be a child of a syscon node with the required >> > property: >> > @@ -47,7 +54,7 @@ RGMII1 RGMII2 RMII1 RMII2 SD1 SPI1 SPI1DEBUG SPI1PASSTHRU TIMER4 TIMER5 TIMER6 >> > TIMER7 TIMER8 VGABIOSROM >> > >> > >> > -Examples: >> > +g4 Example: >> > >> > > > syscon: scu@1e6e2000 { >> > > > compatible = "syscon", "simple-mfd"; >> > > > @@ -63,5 +70,34 @@ syscon: scu@1e6e2000 { >> > > > }; >> > }; >> > >> > +g5 Example: >> > + >> > +apb { >> > > > > > + gfx: display@1e6e6000 { >> > > > + compatible = "aspeed,ast2500-gfx", "syscon"; >> > > > + reg = <0x1e6e6000 0x1000>; >> > > > + }; >> > + >> > > > > > + lpchc: lpchc@1e7890a0 { >> > > > + compatible = "aspeed,ast2500-lpchc", "syscon"; >> > > > + reg = <0x1e7890a0 0xc4>; >> > > > + }; >> > + >> > > > > > + syscon: scu@1e6e2000 { >> > + compatible = "syscon", "simple-mfd"; >> >> I must have missed this the first time, but "syscon" should be used with >> a specific compatible. Though, the scu binding does define one. > > Yes, the example should be fixed. > >> >> > > > + reg = <0x1e6e2000 0x1a8>; >> > + >> > + pinctrl: pinctrl { >> >> Is this the only child? > > No. A incomplete list of other functions in the SCU includes: > > * An RNG > * Power management > * PCI configuration > * System reset > * Clock configuration > >> >> > + compatible = "aspeed,g5-pinctrl"; >> >> There's no register range for pinctrl? > > This may be a mistake on my part; when I wrote this I had no experience > with writing devicetree bindings (and still don't have a lot). > > The SCU does have register regions for pinctrl but on reflection I feel > neither the mfd nor syscon bindings describe how children's resources > should be treated in general. The example in the mfd bindings is for > hardware that is register-bit-led,compatible, whose bindings use the > 'offset' property rather than 'reg', which still describes where, but > not using the reg property. Given my uncertainty with reg in an mfd > child, I wrote the pinctrl/pinmux driver using offsets from the base of > the SCU's syscon rather than describing the exact region(s) of the > syscon that should be used. > > The issue you raise here occurred to me when writing the LPC Host > Controller bindings, but there I wasn't convinced using the ranges > property to give offsets was the right thing to do either. > > Regardless, whilst there are two dedicated regions of pinmux registers, > the mux state also depends on bits in SCU registers outside of these > regions. Assuming we define an appropriate ranges property for the SCU > syscon the pinctrl reg property would look like: > > reg = <0x2c 0x1>, <0x3c 0x1>, <0x48 0x1>, <0x70 0x1>, <0x7c 0x1>, <0x80 0x18>, <0xa0 0x10>, <0xd0 0x1>; > > This is the list of registers affecting the mux taken from the pinctrl- > aspeed.h. With other registers in the holes, right? If it is sparse like that, then yes you probably just want to have reg in the parent for the whole block. > What action do you recommend here? The pinctrl dts patches for the > Aspeed SoCs are yet to be applied, so changing the bindings to require > a reg property can't break any existing in-tree users as there are > none. The pinctrl driver can be patched to respect the reg property > after the fact, though actually using the region descriptions might be > interesting. > >> >> > + aspeed,external-nodes = <&gfx, &lpchc>; > > Did you have feedback on this approach? I queried you about it in the > previous revision, but never received a reply: It seems okay. At least, I don't have a better suggestion. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html