Re: [PATCH 03/14] bus: mvebu-mbus: Introduce device tree binding

[Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

On Fri, Jun 07, 2013 at 01:44:30PM -0600, Jason Gunthorpe wrote:
> On Fri, Jun 07, 2013 at 09:10:35PM +0200, Arnd Bergmann wrote:
> > On Friday 07 June 2013, Ezequiel Garcia wrote:
> > > +       np = of_find_matching_node(NULL, of_mvebu_mbus_ids);
> > > +       if (!np) {
> > > +               pr_err("could not find a matching SoC family\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       of_id = of_match_node(of_mvebu_mbus_ids, np);
> > > +       mbus_state.soc = of_id->data;
> > > +
> > > +       if (of_address_to_resource(np, 0, &mbuswins_res)) {
> > > +               pr_err("cannot get MBUS register address\n");
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       if (of_address_to_resource(np, 1, &sdramwins_res)) {
> > > +               pr_err("cannot get SDRAM register address\n");
> > > +               return -EINVAL;
> > > +       }
> > 
> > Just an idea to make this more regular: Since the internal-regs can no longer
> > be regarded as a fixed location, we might want to use the same "ranges" property
> > mechanism for resolving the internal regs as we use for everything else.
> > 
> > This would imply that the device node this driver binds to would actually
> > end up being a child of the bus itself, which then goes on to modify the
> > ranges property of its parent node. Does that make sense?
> 
> We have a minimum requirement that the bootloader setup internal regs,
> so the minimum required DT bindings is going to be this:
> 
> mbus {
>   compatible = ...
>   ranges = <INTERNAL_REGS_MAP_ID 0xf1000000 0x100000>;
>   reg = <0xf1000xxx ...>; // MBUS regs block
>   #address/size-cells...
> 
>   internal-regs  {
>       ranges = <0 INTERNAL_REGS_MAP_ID 0x100000>;
>   }
> }
> 

... and the above is the way its done in my proposal,
with INTERNAL_REGS_MAP_ID = 0x0.

> ie the ranges should never be empty.
> 
> Discovery of the address of the mbus control registers is via the reg
> property on its own node (which is untranslated), and all other
> internal regs blocks will automatically translate as they are supposed
> to (Thomas's work to make internal regs 0xd/0xf1 relies on this, AFAIK)
> 
> INTERNAL_REGS_MAP_ID is an invalid target ID value, defined to mean
> the internal registers target. 0xFFFFFFFF is a better choice for this
> than 0, because 0xFFFFFFFF is never going to be a valid target id, it
> is too large.
> 

I agree, using 0x0 was not a good choice. I'll change that.

Thanks both for the feedback!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss