Re: [PATCH v3 1/4] pinctrl: aspeed: Read and write bits in LPC and GFX controllers

[Andrew Jeffery]

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.

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:

https://marc.info/?l=devicetree&m=147873554311535&w=4

Thanks,

Andrew

> > +
> > > > +			pinctrl_i2c3_default: i2c3_default {
> > > > +				function = "I2C3";
> > > > +				groups = "I2C3";
> > > > +			};
> > > > +		};
> > > > +	};
> > +};
> > +
> > ÂPlease refer to pinctrl-bindings.txt in this directory for details of the
> > Âcommon pinctrl bindings used by client devices.

Attachment: signature.asc
Description: This is a digitally signed message part