Re: [PATCH 1/4] bus: uniphier-system-bus: add UniPhier System Bus Controller driver

[Masahiro Yamada]

Hi Mark,

2015-11-25 2:38 GMT+09:00 Mark Rutland <mark.rutland@xxxxxxx>:
> Hi,
>
>> >> +UniPhier System Bus Controller
>> >> +------------------------------
>> >> +
>> >> +The UniPhier System Bus Controller is a hardware block with registers that
>> >> +controls the System Bus accessing; how each bank is mapped onto the parent bus,
>> >> +various timing parameters of the bus access, etc.
>> >> +
>> >> +Required properties for System Bus Controller:
>> >> +- compatible: should be "socionext,uniphier-system-bus-controller".
>> >> +- reg: offsets and lengths of the register sets for the device.  It should
>> >> +  contain 2 regions: base & control register, misc register, in this order.
>> >
>> > The example also has a system-bus phandle.
>>
>> Actually, I was wondering which is better to describe the relation between
>> the bus and the controller,  phandle or compatible string..
>
> To describe relationships between nodes, use phandles.
>
> Compatible strings alone cannot define relationships -- you cannot
> encode how multiple instances are related.
>
>> > Is the "misc register" part of the bus controller, or is it a shared
>> > system controller?
>>
>> It is a part of the bus controller, but used for another purpose.
>> (i.e. partly this is syscon.  I know this is strange, but it is
>> what the hardware developers designed.)
>
> Ok. What else is going to need to use this in future?
>
>> > Assuming that the controller and bus are 1-1 related, make this a single
>> > node. e.g.
>> >
>> > system-bus {
>> >         compatible = "socionext,uniphier-system-bus";
>> >         reg = <0x58c00000 0x400>, <0x59800000 0x2000>;
>> >         #address-cells = <2>;
>> >         #size-cells = <1>;
>> >         ranges = <1 0x00000000 0x42000000 0x02000000>,
>> >                  <5 0x00000000 0x48000000 0x01000000>;
>> >
>> >         ...
>> >         child nodes here
>> >         ...
>> >
>> > };
>>
>> Hmm, make sense.  But, I prefer to reflect the hardware structure.
>>
>> The range of System Bus is <0x40000000 0x10000000>.
>>
>> The register of the System Bus Controller is
>> <0x58c00000 0x400>  (and <0x59800000 0x2000>)
>>
>>
>> The bus and its controller is different.
>
> So? We always describe the programming interface (i.e. the slave
> interface of the device that responds to the CPU).
>
> There's no need for separate nodes. It only makes the driver more
> complicated.
>
>> >> +static int uniphier_sbc_probe(struct platform_device *pdev)
>> >> +{
>> >> +     struct device *dev = &pdev->dev;
>> >> +     struct uniphier_sbc_priv *priv;
>> >> +     struct resource *regs;
>> >> +     struct device_node *bus_np;
>> >> +     int child_addrc, addrc, sizec, bank;
>> >> +     u64 child_addr, addr, size;
>> >> +     const __be32 *ranges;
>> >> +     int rlen, rone, ret;
>> >> +
>> >> +     bus_np = of_find_compatible_node(NULL, NULL,
>> >> +                                      "socionext,uniphier-system-bus");
>> >
>> > This is broken if you ever have multiple instances.
>> >
>> > Either use a single node, or if there is a more complex relationship
>> > between busses and their controllers, describe that explicitly with
>> > phandles.
>>
>>
>> Probably, I will stick to phandle in v2.
>
> I would prefer a single node unless there's some other complication
> regarding the relationship of the controller and the bus itself.


OK, i will try the single node way.


-- 
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/