Re: [PATCH v2 5/5] vf610-soc: Add Vybrid SoC device tree binding documentation

[Stefan Agner]

On 2016-05-09 15:58, Rob Herring wrote:
> On Mon, May 9, 2016 at 1:25 PM, Stefan Agner <stefan@xxxxxxxx> wrote:
>> On 2016-05-09 10:14, Rob Herring wrote:
>>> On Thu, May 5, 2016 at 3:27 AM,  <maitysanchayan@xxxxxxxxx> wrote:
>>>> Hello Rob,
>>>>
>>>> On 16-05-03 21:30:26, Rob Herring wrote:
>>>>> On Mon, May 02, 2016 at 12:35:04PM +0530, Sanchayan Maity wrote:
>>>>> > Add device tree binding documentation for Vybrid SoC.
>>>>> >
>>>>> > Signed-off-by: Sanchayan Maity <maitysanchayan@xxxxxxxxx>
>>>>> > ---
>>>>> >  .../bindings/arm/freescale/fsl,vf610-soc.txt       | 35 ++++++++++++++++++++++
>>>>> >  1 file changed, 35 insertions(+)
>>>>> >  create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,vf610-soc.txt
>>>>> >
>>>>> > diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,vf610-soc.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,vf610-soc.txt
>>>>> > new file mode 100644
>>>>> > index 0000000..bdd95e8
>>>>> > --- /dev/null
>>>>> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,vf610-soc.txt
>>>>> > @@ -0,0 +1,35 @@
>>>>> > +Vybrid System-on-Chip
>>>>> > +---------------------
>>>>> > +
>>>>> > +Required properties:
>>>>> > +
>>>>> > +- #address-cells: must be 1
>>>>> > +- #size-cells: must be 1
>>>>> > +- compatible: "fsl,vf610-soc-bus", "simple-bus"
>>>>>
>>>>> If this is a bus, put the file in bindings/bus/...
>>>>
>>>> The fsl,vf610-soc-bus binding is used to bind the driver in question with
>>>> an appropriate compatible node.
>>>>
>>>> Basically being a standalone platform driver, there was need of a compatible
>>>> property to bind on. Introducing a separate device tree node for it's sake
>>>> didn't seem appropriate so the alteration to SoC node's compatible.
>>>
>>> Ah, so you are designing a node around the needs of a Linux specific
>>> driver. Don't do that. DT describes the h/w and this node is not a h/w
>>> block.
>>>
>>> Create a platform device based on a matching SOC compatible string
>>> instead and make your driver find the information it needs directly
>>> from the relevant nodes like the ROM node.
>>
>> That reads like my words a year ago:
>> https://lkml.org/lkml/2015/5/22/408
>>
>> Initially pretty much everything was hard-coded in the driver.
>>
>> Arnd then pushed to use more descriptive in the device tree.
>>
>> Of course, we should not end up making up relations which are not there
>> in hardware. We need to find the right balance.
>>
>> Here is my suggestion:
>> 1. Add "fsl,vf610-soc-bus" as compatible string to the soc node, use it
>> to bind the "soc bus driver" as a platform driver located in driver/soc/
> 
> I'm not convinced this is a h/w block. This keeps coming up and I
> think this is a kernel problem, not a DT problem. Let's face it that
> there are drivers at the SOC level which don't fit into a DT node.
> They may be the exception, but they are a common exception. My
> proposal for how to deal with these cases is here[1].

Probably "fsl,vf610-soc" would be better than "fsl,vf610-soc-bus".

But the SoC is definitely a h/w block! Despite all the individual
silicon in there, I can even touch the SoC ;-)

But yeah, I understand what you are saying... We model it as a
container, and do not attribute any specific thing directly to it. But
aren't there other containers where we bind to? Is it wrong to bind a
driver to a container, if that driver implements issues concerning that
level..?

The two drivers under drivers/soc/ which implement the SoC bus API and
both use a compatible string on a container level (e.g.
arm,realview-pb11mp-soc in arch/arm/boot/dts/arm-realview-pb11mp.dts).

> 
> I also think drivers/soc is a mess because it is randomly used or not.
> IMO, using it should not be at the whim of whomever does SOC support.

Are you talking about random drivers under drivers/soc/ or the ones
which implement the SoC bus API? I think that is not the same...

If SoC bus, agreed, it is a mess. Not only its adoption is sparse, the
specification is also interpreted differently across different SoC's,
which I brought up in the past:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333765.html

But it exports really useful information to user space, and that has
real value in practice.

--
Stefan