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.
> 
> > +                       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.
> 
> Have you investigated reserving this memory when the driver probes
> instead of hardcoding it?
> 
> > +               };
> > +       };
> > +
> >         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.
> 

Oops missed this, do we want to make this disabled - it should exist on
every ast2500 and it doesn't depend on anything else...

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