Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI

[Mark Rutland]

On Wed, Oct 26, 2016 at 06:55:22AM +0000, M.H. Lian wrote:
> Hi Robin,
> 
> Please see my comments inline.
> 
> Thanks,
> Minghuan
> 
> > -----Original Message-----
> > From: Robin Murphy [mailto:robin.murphy@xxxxxxx]
> > Sent: Tuesday, October 25, 2016 9:01 PM
> > To: M.H. Lian <minghuan.lian@xxxxxxx>; linux-arm-
> > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx
> > Cc: Marc Zyngier <marc.zyngier@xxxxxxx>; Stuart Yoder
> > <stuart.yoder@xxxxxxx>; Leo Li <leoyang.li@xxxxxxx>; Scott Wood
> > <scott.wood@xxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Mingkai Hu
> > <mingkai.hu@xxxxxxx>
> > Subject: Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG
> > MSI
> > 
> > On 25/10/16 13:35, Minghuan Lian wrote:
> > > 1. The different version of a SoC may have different MSI
> > > implementation. But compatible "fsl,<soc-name>-msi" can not describe
> > > the SoC version.
> > 
> > Can't it?
> > 
> > 	compatible = "fsl-ls1043a-rev11-msi";
> > 
> > Oh, I guess it can!
> > 
> > Joking aside, if there are multiple versions of a piece of hardware which
> > require *different* treatment by drivers, then it is obviously wrong to use
> > the same compatible string because *they are not compatible*.
> > 
> [Minghuan Lian]  Yes, but Rev1.0 and Rev1.1 SoC will use the same dts files.
> We cannot create different dts files for each revision of the same kind of SoC.

... why?

The DT should describe the hardware; if hardware differs then it should
have a different DT.

> It means that there are different variants in the different versions
> of the same SoC that will use the same compatible string.

Why can you not add a string for each variant, in addition to the SoC
string? We do that elsewhere.

> So I have to use SoC match interface to get the versions.
> 
> I'm too radical. I do not want to first check SoC family via
> compatible string and then check revision via SoC match or SVR.

You can have *both* in the the compatible string list, e.g. at the top
level:

	compatible = "vendor,soc-rev", "vendor-soc";

For devices which differ, this can be encoded similarly in the device
compatible string list.

> I selected the "SoC match" like the following to get the related information at only one place. 
> 
> static struct soc_device_attribute soc_msi_matches[] = {
> 	{ .family = "QorIQ LS1021A",
> 	  .data = &ls1021_msi_cfg },
> 	{ .family = "QorIQ LS1012A",
> 	  .data = &ls1021_msi_cfg },
> 	{ .family = "QorIQ LS1043A", .revision = "1.0",
> 	  .data = &ls1021_msi_cfg },
> 	{ .family = "QorIQ LS1043A", .revision = "1.1",
> 	  .data = &ls1043_rev11_msi_cfg },
> 	{ .family = "QorIQ LS1046A",
> 	  .data = &ls1046_msi_cfg },
> 	{ },
> };
> 
> I will remain the SoC related compatible and try to describe the
> difference via some kind of the property.

As commented on the driver side, this should be described with DT
properties on the devices.

Thanks,
Mark.