| Message ID | 20180412035145.21488-3-andrew@aj.id.au |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | Miscellaneous BMC interfaces | expand |
On 12 April 2018 at 13:21, Andrew Jeffery <andrew@aj.id.au> wrote: > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > arch/arm/boot/dts/aspeed-g5.dtsi | 48 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi > index 1468b4ad22dc..8321df50c593 100644 > --- a/arch/arm/boot/dts/aspeed-g5.dtsi > +++ b/arch/arm/boot/dts/aspeed-g5.dtsi > @@ -158,6 +158,11 @@ > > }; > > + vga_scratch: scratch@50 { > + compatible = "aspeed,bmc-misc"; Convention is to use aspeed,<socname>-<hardware>, so this would be aspeed,ast2500-bmc-misc. > + reg = <0x50 0x20>; > + }; > + > hwrng@78 { > compatible = "timeriomem_rng"; > reg = <0x78 0x4>; > @@ -1425,3 +1430,46 @@ > groups = "WDTRST2"; > }; > }; > + > +&vga_scratch { > + vga0 { Do these names come out in the userspace API? > + offset = <0x50>; Would reg <> be more devicetree-y than offset = <>? > + bit-mask = <0xffffffff>; > + bit-shift = <0>; Should we assume this is the default? Or something else? Having a default for the common case would be nice. > + }; > + vga1 { > + offset = <0x54>; > + bit-mask = <0xffffffff>; > + bit-shift = <0>;
On Thu, 19 Apr 2018, at 12:57, Joel Stanley wrote: > On 12 April 2018 at 13:21, Andrew Jeffery <andrew@aj.id.au> wrote: > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > --- > > arch/arm/boot/dts/aspeed-g5.dtsi | 48 ++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 48 insertions(+) > > > > diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi > > index 1468b4ad22dc..8321df50c593 100644 > > --- a/arch/arm/boot/dts/aspeed-g5.dtsi > > +++ b/arch/arm/boot/dts/aspeed-g5.dtsi > > @@ -158,6 +158,11 @@ > > > > }; > > > > + vga_scratch: scratch@50 { > > + compatible = "aspeed,bmc-misc"; > > Convention is to use aspeed,<socname>-<hardware>, so this would be > aspeed,ast2500-bmc-misc. Yeah, this partly comes back to the naming problem you mentioned in the other patch. The functionality is generic, and so probably shouldn't mention aspeed at all, let alone the SoC revision. > > > > + reg = <0x50 0x20>; > > + }; > > + > > hwrng@78 { > > compatible = "timeriomem_rng"; > > reg = <0x78 0x4>; > > @@ -1425,3 +1430,46 @@ > > groups = "WDTRST2"; > > }; > > }; > > + > > +&vga_scratch { > > + vga0 { > > Do these names come out in the userspace API? Yes, though you can override the use of the node name by adding a label property inside the node. That way if someone complains about the node naming we don't have to break userspace. > > > + offset = <0x50>; > > Would reg <> be more devicetree-y than offset = <>? There's a mix. I was looking through some other syscon-type stuff and some of those drivers used offset. This approach might avoid militant opinions around the relationship between the unit address and the reg property, as you "can't" have multiple nodes with the same unit address. There's some discussion on this in the PECI threads. > > > > + bit-mask = <0xffffffff>; > > + bit-shift = <0>; > > Should we assume this is the default? Or something else? Having a > default for the common case would be nice. Whilst the nodes I've added in these examples expose an entire register, I think this would actually be a bit of an unusual case. More likely you'll want to expose a single bit from some register, in which case a default for mask (though maybe 0x1?) or shift isn't going to make any sense. Cheers, Andrew
On 19 April 2018 at 13:30, Andrew Jeffery <andrew@aj.id.au> wrote: > On Thu, 19 Apr 2018, at 12:57, Joel Stanley wrote: >> On 12 April 2018 at 13:21, Andrew Jeffery <andrew@aj.id.au> wrote: >> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >> > --- >> > arch/arm/boot/dts/aspeed-g5.dtsi | 48 ++++++++++++++++++++++++++++++++++++++++ >> > 1 file changed, 48 insertions(+) >> > >> > diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi >> > index 1468b4ad22dc..8321df50c593 100644 >> > --- a/arch/arm/boot/dts/aspeed-g5.dtsi >> > +++ b/arch/arm/boot/dts/aspeed-g5.dtsi >> > @@ -158,6 +158,11 @@ >> > >> > }; >> > >> > + vga_scratch: scratch@50 { >> > + compatible = "aspeed,bmc-misc"; >> >> Convention is to use aspeed,<socname>-<hardware>, so this would be >> aspeed,ast2500-bmc-misc. > > Yeah, this partly comes back to the naming problem you mentioned in the other patch. The functionality is generic, and so probably shouldn't mention aspeed at all, let alone the SoC revision. > I disagree. The bindings need to be hardware specific, but the code can be generic. We have a preceidnce for this in the reset controller. There's a controller called reset-simple that contains generic code (with some quirks for some systems it supports), and a list of compatible strings. On the other side of the equation, the bindings themselves are specific to each SoC. If we get push back for being generic we can cite them as our inspiration :) >> >> >> > + reg = <0x50 0x20>; >> > + }; >> > + >> > hwrng@78 { >> > compatible = "timeriomem_rng"; >> > reg = <0x78 0x4>; >> > @@ -1425,3 +1430,46 @@ >> > groups = "WDTRST2"; >> > }; >> > }; >> > + >> > +&vga_scratch { >> > + vga0 { >> >> Do these names come out in the userspace API? > > Yes, though you can override the use of the node name by adding a label property inside the node. That way if someone complains about the node naming we don't have to break userspace. Similar to the leds bindings, sgtm. > >> >> > + offset = <0x50>; >> >> Would reg <> be more devicetree-y than offset = <>? > > There's a mix. I was looking through some other syscon-type stuff and some of those drivers used offset. This approach might avoid militant opinions around the relationship between the unit address and the reg property, as you "can't" have multiple nodes with the same unit address. There's some discussion on this in the PECI threads. > >> >> >> > + bit-mask = <0xffffffff>; >> > + bit-shift = <0>; >> >> Should we assume this is the default? Or something else? Having a >> default for the common case would be nice. > > Whilst the nodes I've added in these examples expose an entire register, I think this would actually be a bit of an unusual case. More likely you'll want to expose a single bit from some register, in which case a default for mask (though maybe 0x1?) or shift isn't going to make any sense. Ok. If you can think of something that makes the bindings less verbose I'm all for that.
> > > >> > >> > >> > + bit-mask = <0xffffffff>; > >> > + bit-shift = <0>; > >> > >> Should we assume this is the default? Or something else? Having a > >> default for the common case would be nice. > > > > Whilst the nodes I've added in these examples expose an entire register, I think this would actually be a bit of an unusual case. More likely you'll want to expose a single bit from some register, in which case a default for mask (though maybe 0x1?) or shift isn't going to make any sense. > > Ok. If you can think of something that makes the bindings less verbose > I'm all for that. Part of the issue (IMO) is we're stretching the intent of the driver here to expose the scratch registers. Hence the bullet point on sysfs vs chardev mentioned in the cover letter. Using the driver for the scratch registers was driven by some conversations with BenH; we could not do this and go the chardev route instead. I'd like Ben to chime in, as I know JK is more for the chardev approach. Andrew
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi index 1468b4ad22dc..8321df50c593 100644 --- a/arch/arm/boot/dts/aspeed-g5.dtsi +++ b/arch/arm/boot/dts/aspeed-g5.dtsi @@ -158,6 +158,11 @@ }; + vga_scratch: scratch@50 { + compatible = "aspeed,bmc-misc"; + reg = <0x50 0x20>; + }; + hwrng@78 { compatible = "timeriomem_rng"; reg = <0x78 0x4>; @@ -1425,3 +1430,46 @@ groups = "WDTRST2"; }; }; + +&vga_scratch { + vga0 { + offset = <0x50>; + bit-mask = <0xffffffff>; + bit-shift = <0>; + }; + vga1 { + offset = <0x54>; + bit-mask = <0xffffffff>; + bit-shift = <0>; + }; + vga2 { + offset = <0x58>; + bit-mask = <0xffffffff>; + bit-shift = <0>; + }; + vga3 { + offset = <0x5c>; + bit-mask = <0xffffffff>; + bit-shift = <0>; + }; + vga4 { + offset = <0x60>; + bit-mask = <0xffffffff>; + bit-shift = <0>; + }; + vga5 { + offset = <0x64>; + bit-mask = <0xffffffff>; + bit-shift = <0>; + }; + vga6 { + offset = <0x68>; + bit-mask = <0xffffffff>; + bit-shift = <0>; + }; + vga7 { + offset = <0x6c>; + bit-mask = <0xffffffff>; + bit-shift = <0>; + }; +};
Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- arch/arm/boot/dts/aspeed-g5.dtsi | 48 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)