Re: [PATCH v7 2/2] memory: atmel-ebi: add DT bindings documentation

[Rob Herring]

On Tue, May 10, 2016 at 8:08 AM, Jean-Jacques Hiblot
<jjhiblot@xxxxxxxxxxxxxxx> wrote:
> 2016-05-10 14:41 GMT+02:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>:
>> On Tue, 10 May 2016 12:07:42 +0100
>> Mark Rutland <mark.rutland@xxxxxxx> wrote:
>>
>>> On Tue, May 10, 2016 at 10:04:48AM +0200, Boris Brezillon wrote:
>>> > On Wed, 4 May 2016 15:35:47 +0200
>>> > Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote:
>>> >
>>> > > On Wed, 4 May 2016 08:06:10 -0500
>>> > > Rob Herring <robh@xxxxxxxxxx> wrote:
>>> > >
>>> > > > On Wed, May 4, 2016 at 4:38 AM, Boris Brezillon
>>> > > > <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote:
>>> > > > > Hi Rob,
>>> > > > >
>>> > > > > On Tue, 3 May 2016 14:11:04 -0500
>>> > > > > Rob Herring <robh@xxxxxxxxxx> wrote:
>>> > > > >
>>> > > > >> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
>>> > > > >> <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote:
>>> > > > >> > Hi Rob,
>>> > > > >> >
>>> > > > >> > On Tue, 3 May 2016 11:40:19 -0500
>>> > > > >> > Rob Herring <robh@xxxxxxxxxx> wrote:
>>> > > > >> >
>>> > > > >> >> On Thu, Apr 28, 2016 at 02:03:27PM +0200, Boris Brezillon wrote:
>>> > > > >> >> > The EBI (External Bus Interface) is used to access external peripherals
>>> > > > >> >> > (NOR, SRAM, NAND, and other specific devices like ethernet controllers).
>>> > > > >> >> > Each device is assigned a CS line and an address range and can have its
>>> > > > >> >> > own configuration (timings, access mode, bus width, ...).
>>> > > > >> >> > This driver provides a generic DT binding to configure a device according
>>> > > > >> >> > to its requirements.
>>> > > > >> >> > For specific device controllers (like the NAND one) the SMC timings
>>> > > > >> >> > should be configured by the controller driver through the matrix and smc
>>> > > > >> >> > syscon regmaps.
>>> > > >
>>> > > > [...]
>>> > > >
>>> > > > >> >> > +EBI bus configuration associated with specific chip-select will be defined in
>>> > > > >> >> > +the configs subnode. This configs node will in turn contain several subnodes
>>> > > > >> >> > +named config-<cs-id>, each of them containing the following properties.
>>> > > > >> >>
>>> > > > >> >> This is a bit unusual. Why not just part of the child device nodes?
>>> > > > >> >
>>> > > > >> > Oh, come on! I reworked the binding because Mark complained about the
>>> > > > >> > previous binding which was doing exactly what you're suggesting. Can
>>> > > > >> > you please be consistent in your reviews...
>>> > > > >>
>>> > > > >> No, Mark and I both have our opinions. Which part of this patch
>>> > > > >> explains the history?
>>> > > > >
>>> > > > > Hm, it's in patch 1/2 (just dropped the cover letter, which might not
>>> > > > > be such a good idea).
>>> > > > >
>>> > > > >> If the revision history is not in the patch, I'm
>>> > > > >> not looking at it.
>>> > > > >>
>>> > > > >> My issue with it this way is that it has invented yet another way to
>>> > > > >> describe timings. I would like to be consistent across external bus
>>> > > > >> descriptions, but we're not very consistent to begin with though. The
>>> > > > >> most common seems to be the way you first did it. But I agree that it
>>> > > > >> is kind of screwy to have an intermediate node unless the controller
>>> > > > >> itself has sub-blocks within it and is not the established way to
>>> > > > >> describe a bus with chip selects. I would either put the properties
>>> > > > >> directly in the child nodes (e.g. flash@0,0) or put your config nodes
>>> > > > >> in the device node. I'd call it timings instead of config, but that's
>>> > > > >> just bikeshedding.
>>> > > > >
>>> > > > > Well, it's not only describing timings (see atmel,bus-width,
>>> > > > > atmel,byte-access-type, ...), but I'm fine with either names :).
>>> > > > >
>>> > > > >>
>>> > > > >> memory-controller@1000 {
>>> > > > >>   ...
>>> > > > >>   flash@0,0 {
>>> > > > >>     timings {
>>> > > > >>       ...
>>> > > > >>     };
>>> > > > >>   };
>>> > > > >> };
>>> > > > >
>>> > > > > Okay. Mark, what do you think of this approach?
>>> > > > >
>>> > > > > Note that one of my previous version was defining timings directly in
>>> > > > > the EBI device node, and Arnd noted that doing so may cause problems
>>> > > > > if one of the EBI property (or the config/timing node name) conflict
>>> > > > > with the sub-device binding, which is why I decided to put the EBI
>>> > > > > config definitions in a separate subnode.
>>> > > >
>>> > > > You have vendor prefixes on all the properties so I don't think a
>>> > > > collision is really a problem. It's also an established pattern in
>>> > > > i.MX WEIM and OMAP GPMC (which are hiding in bindings/bus/) and I
>>> > > > prefer consistency.
>>> > >
>>> > > So let's summarize that.
>>> > >
>>> > > memory-controller@1000 {
>>> > >   ...
>>> > >   flash@0,0 {
>>> > >           atmel,<ebi-prop-name> = <value>;
>>> > >           ...
>>> > >           <flash-device-prop> = <value>;
>>> > >   };
>>> > > };
>>> > >
>>> > > Would everyone agree on this representation?
>>> > >
>>> > > With this approach, it's a bit more complicated to detect the case
>>> > > where we want to keep bootloader/firmware config, but it should be
>>> > > doable (it's much more easier to test for the presence of a
>>> > > config/timing node than verifying that either all or none of the
>>> > > mandatory properties are here).
>>> > >
>>> > > Still remains the problem mentioned by Jean-Jacques: what if the
>>> > > sub-device takes 2 CS lines. Should we apply the same setting to those
>>> > > slots?
>>> > >
>>> >
>>> > Rob, Mark, Arnd, can you take a decision regarding this binding? This
>>> > driver is floating around for quite some time, and we were asked to
>>> > rework the binding several times (in time in an opposite direction).
>>> >
>>> > For the record, here is the thread I mentioned earlier [1]. In his
>>> > answer, Arnd suggests to put timing and bus config description
>>> > outside of the sub-device node. Mark recently complained about this
>>> > representation, which led me to the configs/config-X appraoch, and now
>>> > Rob suggests to go back to the first proposal.
>>> >
>>> > I'm fine doing that, but can you please all confirm that you agree on
>>> > this binding?
>>>
>>> Sorry for the delay in getting round to this, and sorry that this
>>> appears to be going in circles.
>>>
>>> Please go with Rob's suggestion.
>>
>> Okay. This changes a bit the constraints defined in the binding doc
>> (no default values for undefined properties: we just keep the
>> bootloader/firmware config), but otherwise should be easy to implement.
>>
>>>
>>> I'm not sure about the case where a device takes 2 CS lines. I would
>>> assume that in practice that a sub-device covered my multiple CS lines
>>> expects the same timings for all its MMIO space, and so having that
>>> uniform makes sense. Do we have a counter-example?
>>
>> Nope, I don't. JJH had one (interfacing with an FPGA), maybe he can
>> detail this use case.
>>
> I don't either. It makes sense that a single device with 2 CS uses the
> same timings.
> My use case was the other way around: 1 CS for several devices.

Ah, I thought it was just wanting to share timings for several CS. In
this case, it would probably make sense to have 3 levels of nodes
(EBI, CS node with timings, device nodes) as you do have some logic in
between to do address decoding. But I think the simple case should
still be 2 levels of nodes and that doesn't really affect the EBI
binding. It just cares that timings are in the immediate child nodes.

Rob