Re: [PATCH] ARM: dts: aspeed-g5: Add mailbox and LPC Control nodes

[Cyril Bur <cyrilbur@gmail.com>]

On Mon, 2017-01-16 at 14:56 +1100, Joel Stanley wrote:
> On Mon, Jan 16, 2017 at 12:06 PM, Cyril Bur <cyrilbur@gmail.com> wrote:
> > This reserves BMC ram for host to BMC communication required by the
> > LPC control driver.
> > 
> > As both these devices exist on the LPC bus these nodes are children
> > of a new LPC node.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> >  arch/arm/boot/dts/aspeed-g5.dtsi | 50 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
> 
> 
> > index d6ff41ee6c58..51c339b46740 100644
> > --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> > +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> > @@ -18,6 +18,18 @@
> >                 };
> >         };
> > 
> > +       reserved-memory {
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +               ranges;
> > +
> > +               flash_memory: region@94000000 {
> > +                       compatible = "aspeed,ast2500-lpc-ctrl";
> 
> That doesn't make sense, the RAM isn't a LPC Host Controller.
> 

Agreed, I have a feeling that the lpc-ctrl driver didn't probe wihtout
that. I don't have either machine that this would work on handy to
test. I'm hopeful I'm wrong here because I agree that it doesn't make
sense that this should have that compatible property.

> > +                       no-map;
> > +                       reg = <0x94000000 0x04000000>; /* 64M */
> 
> We don't want this in the dtsi, as there are platforms that don't use
> LPC buses that lose 64 MB of ram. Put this node in the dts. We might
> decide to have an aspeed-bmc-opp.dtsi with common snippets, but for
> now cut and paste.
> 

I agree we don't want it here but I have a very strong aversion to
large chunks of ctrl+c and ctrl+v.

> Have you investigated reserving this memory when the driver probes
> instead of hardcoding it?

Yes, this is by far the cleanest way of getting such a large chunk. If
we do want to drop our usage to 4k or 64k then there are methods at
runtime but for this amount the DT is really the way to go.

This is somthing we can probably add to driver one day. If it doens't
find a reserved-memory node then it should allocate at runtime but that
 allocation will likely need to be smaller orders of megs

> 
> > +               };
> > +       };
> > +
> >         ahb {
> >                 compatible = "simple-bus";
> >                 #address-cells = <1>;
> > @@ -89,6 +101,44 @@
> >                         };
> >                 };
> > 
> > +               lpc: lpc@1e789000 {
> > +                       compatible = "aspeed,ast2500-lpc", "simple-mfd";
> > +                       reg = <0x1e789000 0x1000>;
> > +
> > +                       #address-cells = <1>;
> > +                       #size-cells = <1>;
> > +                       ranges = <0x0 0x1e789000 0x1000>;
> > +
> > +                       lpc_bmc: lpc-bmc@0 {
> > +                               compatible = "aspeed,ast2500-lpc-bmc";
> > +                               reg = <0x0 0x80>;
> > +                       };
> > +
> > +                       lpc_host: lpc-host@80 {
> > +                               compatible = "aspeed,ast2500-lpc-host", "simple-mfd", "syscon";
> > +                               reg = <0x80 0x1e0>;
> > +                               reg-io-width = <4>;
> > +
> > +                               #address-cells = <1>;
> > +                               #size-cells = <1>;
> > +                               ranges = <0x0 0x80 0x1e0>;
> > +
> > +                               lpc-ctrl@0 {
> 
> This needs to have a label.
> 
> > +                                       compatible = "aspeed,ast2500-lpc-ctrl";
> > +                                       memory-region = <&flash_memory>;
> > +                                       flash = <&spi1>;
> 
> Make this node status = "disabled" in the dtsi, and omit the flash phandle.
> 
> I also suggest we have your phandle links for memory and flash in the dts too.
> 
> > +                                       reg = <0x0 0x80>;
> > +                               };
> > +
> > +                               mbox: mbox@180 {
> > +                                       compatible = "aspeed,ast2500-mbox";
> > +                                       reg = <0x180 0x5c>;
> > +                                       interrupts = <46>;
> > +                                       #mbox-cells = <1>;
> 
> Make this node status = "disabled" in the dtsi.
> 

Re all those comments above, I think I see what you're getting at.

Thanks,

Cyril

> Cheers,
> 
> Joel
> 
> > +                               };
> > +                       };
> > +               };
> > +
> >                 vic: interrupt-controller@1e6c0080 {
> >                         compatible = "aspeed,ast2400-vic";
> >                         interrupt-controller;
> > --
> > 2.11.0
> >