Re: [PATCH v2 1/2] dt-bindings: serial: add documentation for the SiFive UART driver

[Paul Walmsley]

On Wed, 24 Oct 2018, Rob Herring wrote:

> On Tue, Oct 23, 2018 at 10:05:40AM -0700, Paul Walmsley wrote:
> > On 10/20/18 7:21 AM, Rob Herring wrote:
> > > On Fri, Oct 19, 2018 at 5:06 PM Paul Walmsley <paul.walmsley@xxxxxxxxxx> wrote:
> > > > On 10/19/18 1:45 PM, Rob Herring wrote:
> > > > > On Fri, Oct 19, 2018 at 1:48 PM Paul Walmsley <paul.walmsley@xxxxxxxxxx> wrote:
> > > > > > Add DT binding documentation for the Linux driver for the SiFive
> > > > > > asynchronous serial IP block.  Nothing too exotic.
> > > > > >
> > > > > > Cc: linux-serial@xxxxxxxxxxxxxxx
> > > > > > Cc: devicetree@xxxxxxxxxxxxxxx
> > > > > > Cc: linux-riscv@xxxxxxxxxxxxxxxxxxx
> > > > > > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > > > > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > > > > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > > > > > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > > > > > Cc: Palmer Dabbelt <palmer@xxxxxxxxxx>
> > > > > > Reviewed-by: Palmer Dabbelt <palmer@xxxxxxxxxx>
> > > > > > Signed-off-by: Paul Walmsley <paul.walmsley@xxxxxxxxxx>
> > > > > > Signed-off-by: Paul Walmsley <paul@xxxxxxxxx>
> > > > > > ---
> > > > > >    .../bindings/serial/sifive-serial.txt         | 21 +++++++++++++++++++
> > > > > >    1 file changed, 21 insertions(+)
> > > > > >    create mode 100644 Documentation/devicetree/bindings/serial/sifive-serial.txt
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/serial/sifive-serial.txt b/Documentation/devicetree/bindings/serial/sifive-serial.txt
> > > > > > new file mode 100644
> > > > > > index 000000000000..8982338512f5
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/serial/sifive-serial.txt
> > > > > > @@ -0,0 +1,21 @@
> > > > > > +SiFive asynchronous serial interface (UART)
> > > > > > +
> > > > > > +Required properties:
> > > > > > +
> > > > > > +- compatible: should be "sifive,fu540-c000-uart0" or "sifive,uart0"
> > > > > As I mentioned for the
> > > > > intc and now the pwm block bindings, if you are going to do version
> > > > > numbers please document the versioning scheme.
> > > > >
> > > > >
> > > > > Will add that to the binding document.
> > > I don't seem to be making my point clear. I don't want any of this
> > > added to a binding doc for particular IP blocks. Write a common doc
> > > that explains the scheme and addresses the questions I asked. Then
> > > just reference that doc here.
> > >
> > > Maybe this is documented somewhere already? Otherwise, if one is
> > > creating a new IP block, how do they know what the versioning scheme
> > > is or what goes in the DT ROM?
> >
> >
> > Seems like there might be some confusion between IP blocks as integrated on
> > an SoC vs. IP blocks in isolation.  It's not necessarily the SoC integrator
> > that sets an IP block version number; this can come from the IP block vendor
> > itself.  So each IP block may have its own version numbering practices for
> > the IP block alone.
> >
> >
> > For SiFive IP blocks, we at SiFive could probably align on a common version
> > numbering structure for what's in the sifive-blocks repository.
>
> I thought you had that from what Palmer said and what I've seen so far.
> You have at least 3 bindings so far it seems.

Yep.

> > But other IP blocks from other vendors may not align to that, or may not
> > have version numbers exposed at all.  In those cases there's no way for
> > software folks to find out what they are,  as you pointed out earlier.  This
> > is the case with most DT compatible strings in the kernel tree.
> >
> > For example, we've integrated the NVDLA IP block, from NVIDIA, on some
> > designs.  Any NVIDIA version numbers in that IP block will probably not
> > follow the SiFive version numbering scheme.  I'd propose the right thing to
> > do for an IP block compatible string is to follow the vendor's practice, and
> > then use the SoC integrator's version numbering practice for the
> > SoC-integrated compatible string.
>
> Experience has shown that using compatible strings only specific to
> vendor IP blocks (with or without version numbers) is pretty useless.
>
> For licensed IP, I'd suggest you follow standard practices.

OK

> A genericish fallback is generally only used when there's lots of SoCs 
> sharing a block.
>
> In these cases though it needs to be clear what bindings follow some
> common versioning scheme and which don't. That's accomplished
> by referencing what the version scheme is. Otherwise, I'd expect I'll
> see the versioning scheme copied when in fact the source IP in no way
> follows it.
>
> > In effect, an SoC integration DT compatible string like
> > "sifive,fu540-c000-uart" implicitly states an IP block version number:
> > "whatever came out of the fab on the chip"[**].   I'd propose that even in
> > these cases, there's an advantage to keeping the "0" on the end, since it
> > uniquely identifies an SoC-independent IP block, rather than just the type
> > of the IP block.   But if the "0" on the end of the SoC integration DT
> > compatible string is problematic for you, we can certainly drop that last 0
> > from the SoC integration DT compatible string, and only suffer a slight lack
> > of clarity as to what version was integrated on that chip.
>
> Personally I'd leave it off, but I'm fine with either way. It just needs
> to be the way you document for SiFive IP blocks.

OK, we'll leave it off.

> Really, I'd just like to see folks get better at putting version and
> configuration information into registers. We only need DT for what we
> can't discover.

Yep, agreed.


- Paul