Re: [PATCH 1/3] dt-bindings: soc: renesas: add MFIS binding documentation

[Wolfram Sang]

Hi Krzysztof,

thanks for the review!

> > 
> > Checked with 'dt_bindings_check'. Family-compatible values are not
> > introduced here because MFIS is usually very different per SoC.
> 
> Not sure with what family this would be compatible, but anyway this
> should be explained in commit msg.

The family would be "rcar-gen5". It was a close call where to put this
paragraph. I can put it to the commit messge as well.

> > +description:
> > +  Renesas Multifunctional Interface (MFIS) provides functionality for
> > +  communication between different CPU cores. Those cores can be in various
> 
> so kind of remoteproc? The soc directory is dumping ground, so you
> should find something more suitable if possible.

I did this. And this was the conclusion because the IP core *is* kind of
a "dumping ground". This paragraph currently might be biased by what we
want support in Linux as of now, like mailboxes and hwspinlock and
product register. But it actually can have more like boot status, error
injection, error detection... and who knows what will be added in the
future. Since DT is HW description, I will update this paragraph to
mention the extra MFIS features even if there is no Linux support
planned.

> > +  reg:
> > +    minItems: 2
> 
> Drop

Ack.

> > +    description:
> > +      An interrupt name is constructed with the prefix 'ch'. Then, the
> > +      channel number as specified in the documentation of the SoC. Finally,
> > +      the letter 'i' if the interrupt is raised by the IICR register. Or 'e'
> > +      if it is raised by the EICR register.
> 
> Describe why is this flexible. These are fixed devices, very specific
> SoCs. They do not come with randomly routed interrupts, usually.

It is not flexible. The interrupts are static and so should be the
naming. The above paragraph aims to describe that. Let me cite part of
the example for "renesas,r8a78000-mfis" from below where all interrupts
(triggered by both, the IICR/EICR registers) are routed to the AP core
interrupt controller. Note the alternating 'i/e' pattern:

	interrupt-names = "ch0i", "ch0e", "ch1i", "ch1e", "ch2i", "ch2e", "ch3i", "ch3e",
			  "ch4i", "ch4e", "ch5i", "ch5e", "ch6i", "ch6e", "ch7i", "ch7e",
			  ...

In comparison, "renesas,r8a78000-mfis-scp" where only the interrupts
triggered by the IICR register are routed to the AP (the EICR interrupts
are routed to the firmware controller). 'i' pattern only:

	interrupt-names = "ch0i", "ch1i", "ch2i", "ch3i", "ch4i", "ch5i", "ch6i", "ch7i",
			  "ch8i", "ch9i", "ch10i", "ch11i", "ch12i", "ch13i", "ch14i", "ch15i",
			  ...

I think the above description is technically correct. Maybe it is not
easy to understand, though. One improvement I could think of: add the
SCP case to the example section as well, so the difference is more
obvious. Note: I decided to use 'pattern:' for describing the
interrupt-names to avoid 128 const entries for the IICR/EICR case, and
64 const entries per IICR-only or EICR-only case.

> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    mfis: syscon@189e0000 {
> 
> Drop label and rename node - that's not syscon.

Yes to drop label. Not sure about renaming the node. Given the above of
the "dumping ground IP core", it is way more of a syscon than of a
mailbox controller. If there is a better common name of such a device, I
don't know of it and couldn't find it when grepping around. "remoteproc"
might be somewhat closer but still...

> > diff --git a/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h b/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h
> > new file mode 100644
> > index 000000000000..89489c2a4847
> > --- /dev/null
> > +++ b/include/dt-bindings/mailbox/renesas,r8a78000-mfis.h

I am wondering now if this is the right place for the include file. My
original intention was that it should go to the mailbox-dir because it
handles only the mailbox-part (second #mbox-cell) of the MFIS. But
maybe all-in-one is better? Other flags are not in flight currently,
though.

> > +/*
> > + * MFIS HW design before r8a78001 requires a channel to be marked as either
> > + * TX or RX.
> > + */
> > +#define MFIS_CHANNEL_TX	(0 << 0)
> 
> 0, bindings constants are abstract (so without dedicated meaning)
> numbers, starting from 0 or 1 and incremented by 1. Shifting this
> implies there is some other logic and that would mean - not a binding.

I'll give in and do it. Using fixed numbers instead of bit numbers
sounds more error prone to me, though. But we don't need to discuss
this, I'll adapt.

And I will change the $subject, too.

Happy hacking,

   Wolfram

Attachment: signature.asc
Description: PGP signature