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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1481760043.3112.60.camel@aj.id.au>
Date:   Thu, 15 Dec 2016 10:30:43 +1030
From:   Andrew Jeffery <andrew@...id.au>
To:     Rob Herring <robh@...nel.org>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Mark Rutland <mark.rutland@....com>,
        Joel Stanley <joel@....id.au>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/4] pinctrl: aspeed: Read and write bits in LPC and
 GFX controllers

On Wed, 2016-12-14 at 10:46 -0600, Rob Herring wrote:
> >> > +                   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? 

Yes.

> If it is sparse like that,
> then yes you probably just want to have reg in the parent for the
> whole block.

Alright, we will leave it as-is.

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

Thanks.

Andrew
Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)