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

[Boris Brezillon]

On Tue, 10 May 2016 09:52:41 -0500
Rob Herring <robh@xxxxxxxxxx> wrote:

> 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.

I'd expect the sub-device driver to change the configuration by itself
in such complex cases (I'm planning to expose APIs to let other drivers
manually configure the EBI bus for their specific use case: it's kind
of required for the NAND controller driver anyway).

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com