| Message ID | 1392543658-5030-3-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Feb 16, 2014 at 10:40:55AM +0100, Wolfram Sang wrote: > From: Wolfram Sang <wsa@sang-engineering.com> > > Signed-off-by: Wolfram Sang <wsa@sang-engineering.com> From your other mail: "[2/5] needs to be reworked to exclude the r8a7790 compatible string." > + compatible = "renesas,i2c-r8a7791", "renesas,i2c-r8a7790"; Why is that? From my knowledge, you start with the exact compatible property and hardware compatible entries may follow. This is backed up by Documentation/devicetree/usage-model.txt: === The 'compatible' property contains a sorted list of strings starting with the exact name of the machine, followed by an optional list of boards it is compatible with sorted from most compatible to least. === And from the devicetree wiki [1]: === compatible is a list of strings. The first string in the list specifies the exact device that the node represents in the form "<manufacturer>,<model>". The following strings represent other devices that the device is compatible with. For example, the Freescale MPC8349 System on Chip (SoC) has a serial device which implements the National Semiconductor ns16550 register interface. The compatible property for the MPC8349 serial device should therefore be: compatible = "fsl,mpc8349-uart", "ns16550". In this case, fsl,mpc8349-uart specifies the exact device, and ns16550 states that it is register-level compatible with a National Semiconductor 16550 UART. Note: ns16550 doesn't have a manufacturer prefix purely for historical reasons. All new compatible values should use the manufacturer prefix. This practice allows existing device drivers to be bound to a newer device, while still uniquely identifying the exact hardware. === Has this changed? [1 ]http://www.devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property
Hi Wolfram, On Mon, Feb 17, 2014 at 4:54 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > On Sun, Feb 16, 2014 at 10:40:55AM +0100, Wolfram Sang wrote: >> From: Wolfram Sang <wsa@sang-engineering.com> >> >> Signed-off-by: Wolfram Sang <wsa@sang-engineering.com> > > From your other mail: > > "[2/5] needs to be reworked to exclude the r8a7790 compatible string." > >> + compatible = "renesas,i2c-r8a7791", "renesas,i2c-r8a7790"; > > Why is that? From my knowledge, you start with the exact compatible > property and hardware compatible entries may follow. I think this boils down to if they really are compatible or not. If for instance a 16550 port would be compatible with 8250 on a hardware level then using them in the order of "16550", "8250" makes sense. In this case the r8a7791 i2c is not really strictly based on r8a7790 i2c, it is just that r8a7790 has support in the driver. So it's a short cut instead of actual hardware compatibility. So far we've dealt with this by updating the driver and only relying on the actual SoC name as suffix. I'm sure there are tons of opinions. =) Cheers, / magnus
On Mon, Feb 17, 2014 at 9:02 AM, Magnus Damm <magnus.damm@gmail.com> wrote: > On Mon, Feb 17, 2014 at 4:54 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >> On Sun, Feb 16, 2014 at 10:40:55AM +0100, Wolfram Sang wrote: >>> From: Wolfram Sang <wsa@sang-engineering.com> >>> >>> Signed-off-by: Wolfram Sang <wsa@sang-engineering.com> >> >> From your other mail: >> >> "[2/5] needs to be reworked to exclude the r8a7790 compatible string." >> >>> + compatible = "renesas,i2c-r8a7791", "renesas,i2c-r8a7790"; >> >> Why is that? From my knowledge, you start with the exact compatible >> property and hardware compatible entries may follow. Thanks for posting the link to the device tree wiki/quoting from it. I had faint memories of that quote, but couldn't remember where I read it. > I think this boils down to if they really are compatible or not. If > for instance a 16550 port would be compatible with 8250 on a hardware > level then using them in the order of "16550", "8250" makes sense. In > this case the r8a7791 i2c is not really strictly based on r8a7790 i2c, > it is just that r8a7790 has support in the driver. So it's a short cut > instead of actual hardware compatibility. > > So far we've dealt with this by updating the driver and only relying > on the actual SoC name as suffix. > > I'm sure there are tons of opinions. =) Hehe ;-) I think the tricky part is when a driver for "renesas,i2c-r8a7790" is updated with a new feature for r8a7790, which doesn't necessarily exist in r8a7791. Then the compatible entry above will cause breakage. In our case, this probably won't happen, as we will have "renesas,i2c-r8a7791" in the driver, but the driver could be forked in between the addition of "renesas,i2c-r8a7790" and "renesas,i2c-r8a7791" in our non-ideal world. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
> > Why is that? From my knowledge, you start with the exact compatible > > property and hardware compatible entries may follow. > > I think this boils down to if they really are compatible or not. If > for instance a 16550 port would be compatible with 8250 on a hardware > level then using them in the order of "16550", "8250" makes sense. In > this case the r8a7791 i2c is not really strictly based on r8a7790 i2c, > it is just that r8a7790 has support in the driver. So it's a short cut > instead of actual hardware compatibility. I don't get this point. The legacy board code for koelsch and lager both create a platform_device with "i2c-rcar_gen2", so the cores surely must be compatible? Maybe the cores are not strictly based on each other, but compatible, yes, I'd say. > So far we've dealt with this by updating the driver and only relying > on the actual SoC name as suffix. OK, for consistency reasons I will resend.
> I think the tricky part is when a driver for "renesas,i2c-r8a7790" is updated > with a new feature for r8a7790, which doesn't necessarily exist in r8a7791. > Then the compatible entry above will cause breakage. If the driver is updated in that way, then it MUST introduce an r8a7791 compatible entry and make sure the new feature is only used on r8a7790. Otherwise it is a bug. If this is adhered, we won't have a breakage because the specific entry (here r8a7791) always comes first. Or?
Hi Wolfram, On Mon, Feb 17, 2014 at 10:19 AM, Wolfram Sang <wsa@the-dreams.de> wrote: >> I think the tricky part is when a driver for "renesas,i2c-r8a7790" is updated >> with a new feature for r8a7790, which doesn't necessarily exist in r8a7791. >> Then the compatible entry above will cause breakage. > > If the driver is updated in that way, then it MUST introduce an r8a7791 > compatible entry and make sure the new feature is only used on r8a7790. > Otherwise it is a bug. If this is adhered, we won't have a breakage > because the specific entry (here r8a7791) always comes first. Or? That will indeed prevent breakage. But in the distributed development world, the person who modifies the driver may not be aware of the r8a7791. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Feb 17, 2014 at 10:23:31AM +0100, Geert Uytterhoeven wrote: > Hi Wolfram, > > On Mon, Feb 17, 2014 at 10:19 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> I think the tricky part is when a driver for "renesas,i2c-r8a7790" is updated > >> with a new feature for r8a7790, which doesn't necessarily exist in r8a7791. > >> Then the compatible entry above will cause breakage. > > > > If the driver is updated in that way, then it MUST introduce an r8a7791 > > compatible entry and make sure the new feature is only used on r8a7790. > > Otherwise it is a bug. If this is adhered, we won't have a breakage > > because the specific entry (here r8a7791) always comes first. Or? > > That will indeed prevent breakage. But in the distributed development world, > the person who modifies the driver may not be aware of the r8a7791. Then it will break even with a seperate r8a7791 compatible entry as well, sadly. Currently, the only thing encoded in the .data field of all compatibles, is which generation of the core is used. This is the same for r8a7790 and r8a7791 (this is why they ARE compatible ;)). So, even in the unlikely case of an r8a7790-only-feature, some code/reactoring to make the distinction is absolutely necessary.
On Mon, Feb 17, 2014 at 6:12 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> > Why is that? From my knowledge, you start with the exact compatible >> > property and hardware compatible entries may follow. >> >> I think this boils down to if they really are compatible or not. If >> for instance a 16550 port would be compatible with 8250 on a hardware >> level then using them in the order of "16550", "8250" makes sense. In >> this case the r8a7791 i2c is not really strictly based on r8a7790 i2c, >> it is just that r8a7790 has support in the driver. So it's a short cut >> instead of actual hardware compatibility. > > I don't get this point. The legacy board code for koelsch and lager both > create a platform_device with "i2c-rcar_gen2", so the cores surely must > be compatible? Maybe the cores are not strictly based on each other, but > compatible, yes, I'd say. The devices may be compatible seen from the limited use case coming from the current version of the device driver. But that does not necessarily mean they are compatible from a hardware point of view. My view is that the hardware should not be seen compatible unless it is explicitly documented to be so. For platform devices we make use of "unstable ids" or feature flags to describe a certain part of hardware as we know it at the time of writing the code. Then we can update the features in the driver and make sure all in-tree boards/socs using the "unstable id" keeps on working by for instance adjusting platform data or adding resources. I think this is a fine model for platform devices. In my mind the above model is quite different from how I think we should describe devices via DT. In the DT case I think it is wrong to rely on software defined "unstable ids" like for instance "i2c-rcar-gen2" because they are built on our perhaps limited interpretation of the hardware. Of course, if the hardware documentation would be correct and we have hardware ids described in the documentation then we can start using those as compatible strings. Until then I feel it is safe to use the SoC name in the compatible string. Cheers, / magnus
On Mon, Feb 17, 2014 at 6:31 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > On Mon, Feb 17, 2014 at 10:23:31AM +0100, Geert Uytterhoeven wrote: >> Hi Wolfram, >> >> On Mon, Feb 17, 2014 at 10:19 AM, Wolfram Sang <wsa@the-dreams.de> wrote: >> >> I think the tricky part is when a driver for "renesas,i2c-r8a7790" is updated >> >> with a new feature for r8a7790, which doesn't necessarily exist in r8a7791. >> >> Then the compatible entry above will cause breakage. >> > >> > If the driver is updated in that way, then it MUST introduce an r8a7791 >> > compatible entry and make sure the new feature is only used on r8a7790. >> > Otherwise it is a bug. If this is adhered, we won't have a breakage >> > because the specific entry (here r8a7791) always comes first. Or? >> >> That will indeed prevent breakage. But in the distributed development world, >> the person who modifies the driver may not be aware of the r8a7791. > > Then it will break even with a seperate r8a7791 compatible entry as > well, sadly. Currently, the only thing encoded in the .data field of all > compatibles, is which generation of the core is used. This is the same > for r8a7790 and r8a7791 (this is why they ARE compatible ;)). So, even > in the unlikely case of an r8a7790-only-feature, some code/reactoring to > make the distinction is absolutely necessary. It won't break because anyone who modifies r8a7790 support need to make it specific to that SoC to not pull in r8a7791 into some assumption. Just because the driver assumes something now doesn't mean the hardware actually is compatible. I think I understand your proposal with making an assumption of the current state of the driver and making sure to add the new SoC compatible value before adjusting the fallback. But this mainly seems like a optimization to improve throughput commit-wise. I actually proposed the same way for SDHI, but now when I think about it I realize that there is no shortcut for adding the compatible string to the driver. It has to be done sooner or later anyway. Or what am I missing? =) / magnus
> It won't break because anyone who modifies r8a7790 support need to > make it specific to that SoC to not pull in r8a7791 into some > assumption. Geert's assumption was that exactly this can be forgotten. While this is true, my point was that this is a bug. And as it turned out, neither this or that solution helps the case :) It needs to be fixed or done properly right from the beginning. > realize that there is no shortcut for adding the compatible string to > the driver. It has to be done sooner or later anyway. OK, can be seen this way. As said before, I'll resend.
On Mon, Feb 17, 2014 at 7:08 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> It won't break because anyone who modifies r8a7790 support need to >> make it specific to that SoC to not pull in r8a7791 into some >> assumption. > > Geert's assumption was that exactly this can be forgotten. While this is > true, my point was that this is a bug. And as it turned out, neither > this or that solution helps the case :) It needs to be fixed or done > properly right from the beginning. > >> realize that there is no shortcut for adding the compatible string to >> the driver. It has to be done sooner or later anyway. > > OK, can be seen this way. As said before, I'll resend. Thanks! I'd be happy to discuss this more over a beverage in the not so distant future! =) / magnus
> >> realize that there is no shortcut for adding the compatible string to > >> the driver. It has to be done sooner or later anyway. > > > > OK, can be seen this way. As said before, I'll resend. > > Thanks! I'd be happy to discuss this more over a beverage in the not > so distant future! =) Yay! :)
diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi index 41194fe..5807b7a 100644 --- a/arch/arm/boot/dts/r8a7791.dtsi +++ b/arch/arm/boot/dts/r8a7791.dtsi @@ -19,6 +19,15 @@ #address-cells = <2>; #size-cells = <2>; + aliases { + i2c0 = &i2c0; + i2c1 = &i2c1; + i2c2 = &i2c2; + i2c3 = &i2c3; + i2c4 = &i2c4; + i2c5 = &i2c5; + }; + cpus { #address-cells = <1>; #size-cells = <0>; @@ -170,6 +179,66 @@ <0 17 IRQ_TYPE_LEVEL_HIGH>; }; + i2c0: i2c@e6508000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "renesas,i2c-r8a7791", "renesas,i2c-r8a7790"; + reg = <0 0xe6508000 0 0x40>; + interrupts = <0 287 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp9_clks R8A7791_CLK_I2C0>; + status = "disabled"; + }; + + i2c1: i2c@e6518000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "renesas,i2c-r8a7791", "renesas,i2c-r8a7790"; + reg = <0 0xe6518000 0 0x40>; + interrupts = <0 288 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp9_clks R8A7791_CLK_I2C1>; + status = "disabled"; + }; + + i2c2: i2c@e6530000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "renesas,i2c-r8a7791", "renesas,i2c-r8a7790"; + reg = <0 0xe6530000 0 0x40>; + interrupts = <0 286 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp9_clks R8A7791_CLK_I2C2>; + status = "disabled"; + }; + + i2c3: i2c@e6540000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "renesas,i2c-r8a7791", "renesas,i2c-r8a7790"; + reg = <0 0xe6540000 0 0x40>; + interrupts = <0 290 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp9_clks R8A7791_CLK_I2C3>; + status = "disabled"; + }; + + i2c4: i2c@e6520000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "renesas,i2c-r8a7791", "renesas,i2c-r8a7790"; + reg = <0 0xe6520000 0 0x40>; + interrupts = <0 19 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp9_clks R8A7791_CLK_I2C4>; + status = "disabled"; + }; + + i2c5: i2c@e6528000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "renesas,i2c-r8a7791", "renesas,i2c-r8a7790"; + reg = <0 0xe6528000 0 0x40>; + interrupts = <0 20 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp9_clks R8A7791_CLK_I2C5>; + status = "disabled"; + }; + pfc: pfc@e6060000 { compatible = "renesas,pfc-r8a7791"; reg = <0 0xe6060000 0 0x250>;