On Tue, Dec 13, 2016 at 12:12 AM, Andrew Jeffery [off-list ref] wrote:

On Mon, 2016-12-12 at 10:27 -0600, Rob Herring wrote:

quoted

On Tue, Dec 06, 2016 at 02:11:49PM +1100, Andrew Jeffery wrote:

quoted

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.

quoted

quoted

Signed-off-by: Andrew Jeffery <redacted>

---
 .../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:

quoted

quoted

-               "aspeed,ast2400-pinctrl"
-               "aspeed,g4-pinctrl"
-               "aspeed,ast2500-pinctrl"
-               "aspeed,g5-pinctrl"

+Required properties for g4:

quoted

quoted

+- compatible :                         Should be any one of the following:
+                               "aspeed,ast2400-pinctrl"
+                               "aspeed,g4-pinctrl"

+
+Required properties for g5:

quoted

quoted

+- compatible :                         Should be any one of the following:
+                               "aspeed,ast2500-pinctrl"
+                               "aspeed,g5-pinctrl"

+

quoted

quoted

+- 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:

quoted

quoted

 syscon: scu@1e6e2000 {
        compatible = "syscon", "simple-mfd";

@@ -63,5 +70,34 @@ syscon: scu@1e6e2000 {
        };

 };

+g5 Example:
+
+apb {

quoted

quoted

quoted

quoted

+   gfx: display@1e6e6000 {

+               compatible = "aspeed,ast2500-gfx", "syscon";
+               reg = <0x1e6e6000 0x1000>;
+       };

+

quoted

quoted

quoted

quoted

+   lpchc: lpchc@1e7890a0 {

+               compatible = "aspeed,ast2500-lpchc", "syscon";
+               reg = <0x1e7890a0 0xc4>;
+       };

+

quoted

quoted

quoted

quoted

+   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.

quoted

quoted

quoted

quoted

+               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

quoted

quoted

+                   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.

quoted

quoted

+                   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