| Message ID | 1309426647-31587-2-git-send-email-manjugk@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 30, 2011 at 07:27:02AM -0700, Tony Lindgren wrote: > Hi, > > Few comments on the .dts data layout below. > > * G, Manjunath Kondaiah <manjugk@ti.com> [110630 02:44]: > > --- a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts > > +++ b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts > > @@ -2,11 +2,6 @@ > > > > / { > > i2c@48072000 { > > - compatible = "ti,omap3-i2c"; > > - reg = <0x48072000 0x80>; > > - #address-cells = <1>; > > - #size-cells = <0>; > > - > > eeprom@50 { > > compatible = "at,at24c01"; > > reg = < 0x50 >; > > The board .dts file should include the omap3 SoC .dts file. > > The omap3 SoC .dts file should have the devices mapped to L3 and L4 > busses, and the then i2c@1 would just contain the bus offset. > > Then the i2c@1 entry would be repeated in the board specific > .dts and tell that the i2c@1 is enabled. yup. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 30, 2011 at 03:07:23PM +0500, G, Manjunath Kondaiah wrote: > > Add I2C and it's child device nodes for beagle board. > The I2C1 controller child devices are not populated and it > should be handled along with OMAP clock changes. > > Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com> > --- > arch/arm/boot/dts/omap3-beagle-nunchuck.dts | 5 --- > arch/arm/boot/dts/omap3-beagle.dts | 42 +++++++++++++++++++++++++++ > 2 files changed, 42 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts > index 2607be5..479be11 100644 > --- a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts > +++ b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts This hunk is of course only for my tree since I'm the only one who actually has this modified beagleboard. :-) > @@ -2,11 +2,6 @@ > > / { > i2c@48072000 { > - compatible = "ti,omap3-i2c"; > - reg = <0x48072000 0x80>; > - #address-cells = <1>; > - #size-cells = <0>; > - > eeprom@50 { > compatible = "at,at24c01"; > reg = < 0x50 >; > diff --git a/arch/arm/boot/dts/omap3-beagle.dts b/arch/arm/boot/dts/omap3-beagle.dts > index 4439466..491ee2b 100644 > --- a/arch/arm/boot/dts/omap3-beagle.dts > +++ b/arch/arm/boot/dts/omap3-beagle.dts > @@ -4,4 +4,46 @@ > / { > model = "TI OMAP3 BeagleBoard"; > compatible = "ti,omap3-beagle"; > + interrupt-parent = <&gic>; > + > + gic: interrupt-controller@48241000 { > + compatible = "ti,omap-gic", "arm,gic"; > + interrupt-controller; > + #interrupt-cells = <1>; > + reg = <0x48200000 0x1000>; > + }; > + > + i2c@48070000 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "ti,omap_i2c"; ti,omap3-i2c Use '-' not '_'. and the specific silicon implementation should be specified (omap3 vs. omap). > + reg = <0x48070000 0x100>; > + interrupts = < 88 >; > + interrupt-parent = <&gic>; interrupt-parent isn't needed because it is inherited from the root node. > + clock-frequency = <2600>; > + status = "disabled"; Drop 'status' when you move this node definition to arch/arm/boot/dts/omap3.dtsi. Board overlay files that include the omap3.dtsi should explicitly disable any devices that it does not use (which is opposite to what tegra currently does, but I'm going to change that). > + }; > + > + i2c@48072000 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "ti,omap_i2c"; > + reg = <0x48072000 0x100>; > + interrupts = < 89 >; > + interrupt-parent = <&gic>; > + clock-frequency = <400>; > + status = "ok"; Okay is spelled 'okay'. :-) The kernel does accept 'ok', but I discourage its usage... just because I'm a nitpick about stuff like that. Actually, if the device is enabled, the status property can be dropped entirely because the default behaviour is to enable. > + }; > + > + i2c@48060000 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "ti,omap_i2c"; > + reg = <0x48060000 0x100>; > + interrupts = < 93 >; > + interrupt-parent = <&gic>; > + clock-frequency = <100>; > + status = "ok"; > + }; > + > }; > -- > 1.7.4.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Grant Likely wrote at Wednesday, July 06, 2011 12:56 PM: > On Thu, Jun 30, 2011 at 03:07:23PM +0500, G, Manjunath Kondaiah wrote: > > Add I2C and it's child device nodes for beagle board. > > The I2C1 controller child devices are not populated and it > > should be handled along with OMAP clock changes. ... > > diff --git a/arch/arm/boot/dts/omap3-beagle.dts b/arch/arm/boot/dts/omap3-beagle.dts ... > > + i2c@48070000 { ... > > + clock-frequency = <2600>; > > + status = "disabled"; > > Drop 'status' when you move this node definition to > arch/arm/boot/dts/omap3.dtsi. Board overlay files that include the > omap3.dtsi should explicitly disable any devices that it does not use > (which is opposite to what tegra currently does, but I'm going to > change that). Purely out of idle curiosity: What's the benefit of one way over the other?
On Wed, Jul 6, 2011 at 5:26 PM, Stephen Warren <swarren@nvidia.com> wrote: > Grant Likely wrote at Wednesday, July 06, 2011 12:56 PM: >> On Thu, Jun 30, 2011 at 03:07:23PM +0500, G, Manjunath Kondaiah wrote: >> > Add I2C and it's child device nodes for beagle board. >> > The I2C1 controller child devices are not populated and it >> > should be handled along with OMAP clock changes. > ... >> > diff --git a/arch/arm/boot/dts/omap3-beagle.dts b/arch/arm/boot/dts/omap3-beagle.dts > ... >> > + i2c@48070000 { > ... >> > + clock-frequency = <2600>; >> > + status = "disabled"; >> >> Drop 'status' when you move this node definition to >> arch/arm/boot/dts/omap3.dtsi. Board overlay files that include the >> omap3.dtsi should explicitly disable any devices that it does not use >> (which is opposite to what tegra currently does, but I'm going to >> change that). > > Purely out of idle curiosity: What's the benefit of one way over the other? Mostly consistency. Most of the experience we have with the flattened device tree up to this point hasn't bothered with the 'status' property. It is only when AMP and hypervisors cam online that it became important to use a status property, and that only when the kernel needs to be told that the device does indeed exist, but it must not be touched. I'd like to continue that pattern for new DT users with the default assumption that a device is enabled unless the board .dts explicitly disables it. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 06, 2011 at 06:12:49PM -0600, Grant Likely wrote: > On Wed, Jul 6, 2011 at 5:26 PM, Stephen Warren <swarren@nvidia.com> wrote: > > Grant Likely wrote at Wednesday, July 06, 2011 12:56 PM: > >> On Thu, Jun 30, 2011 at 03:07:23PM +0500, G, Manjunath Kondaiah wrote: > >> > Add I2C and it's child device nodes for beagle board. > >> > The I2C1 controller child devices are not populated and it > >> > should be handled along with OMAP clock changes. > > ... > >> > diff --git a/arch/arm/boot/dts/omap3-beagle.dts b/arch/arm/boot/dts/omap3-beagle.dts > > ... > >> > + i2c@48070000 { > > ... > >> > + clock-frequency = <2600>; > >> > + status = "disabled"; > >> > >> Drop 'status' when you move this node definition to > >> arch/arm/boot/dts/omap3.dtsi. Board overlay files that include the > >> omap3.dtsi should explicitly disable any devices that it does not use > >> (which is opposite to what tegra currently does, but I'm going to > >> change that). > > > > Purely out of idle curiosity: What's the benefit of one way over the other? > > Mostly consistency. Most of the experience we have with the flattened > device tree up to this point hasn't bothered with the 'status' > property. It is only when AMP and hypervisors cam online that it > became important to use a status property, and that only when the > kernel needs to be told that the device does indeed exist, but it must > not be touched. I'd like to continue that pattern for new DT users > with the default assumption that a device is enabled unless the board > .dts explicitly disables it. > When going this way with my i.mx53 related dts files, I feel I like the opposite way. i.mx53 has many number of peripherals inside, while individual board might only use small portion there. For example, i.mx53 has 5 uart controller instances inside, while quick start (aka loco) board only uses one. With the way you suggest here, we will have the following in imx53.dtsi. uart0: uart@... { ... }; uart1: uart@... { ... }; uart2: uart@... { ... }; uart3: uart@... { ... }; uart4: uart@... { ... }; And we will have to tell the 4 we do not use on quick start board in imx53-qs.dtsi uart1: uart@... { ... status = "disabled"; }; uart2: uart@... { ... status = "disabled"; }; uart3: uart@... { ... status = "disabled"; }; uart4: uart@... { ... status = "disabled"; }; Besides the bothering that we have to list so many unused controllers in individual board dts file, it's also hard to tell which controllers are actually available on the board. People have to look at imx53.dts to get a full list and then exclude the ones in imx53-<board>.dts as "disabled". And if we go the way opposite, adding "disabled" status for everyone in imx53.dts, we will only need to specify the peripherals that are actually available on board with "okay" status in imx53-<board>.dts. And it's much more clear for people to see what peripherals are available on individual board. So I'm going the way than you suggested. Please let me know if you strongly dislikes it.
On Wed, Jul 20, 2011 at 07:04:20PM +0800, Shawn Guo wrote: > > Mostly consistency. Most of the experience we have with the flattened > > device tree up to this point hasn't bothered with the 'status' > > property. It is only when AMP and hypervisors cam online that it > > became important to use a status property, and that only when the > > kernel needs to be told that the device does indeed exist, but it must > > not be touched. I'd like to continue that pattern for new DT users > > with the default assumption that a device is enabled unless the board > > .dts explicitly disables it. [...] > Besides the bothering that we have to list so many unused controllers > in individual board dts file, it's also hard to tell which controllers > are actually available on the board. People have to look at imx53.dts > to get a full list and then exclude the ones in imx53-<board>.dts as > "disabled". > > And if we go the way opposite, adding "disabled" status for everyone > in imx53.dts, we will only need to specify the peripherals that are > actually available on board with "okay" status in imx53-<board>.dts. > And it's much more clear for people to see what peripherals are > available on individual board. > > So I'm going the way than you suggested. Please let me know if you > strongly dislikes it. Yes, I strongly dislike it. I understand the concern, but at this early stage with converting to device tree I think consistency between platforms is more important. We can talk about the issue at Linaro Connect in 2 weeks, but in the mean time please use the enabled-by-default/explicitly-disabled pattern. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 20, 2011 at 12:55:13PM -0600, Grant Likely wrote: > On Wed, Jul 20, 2011 at 07:04:20PM +0800, Shawn Guo wrote: > > > Mostly consistency. Most of the experience we have with the flattened > > > device tree up to this point hasn't bothered with the 'status' > > > property. It is only when AMP and hypervisors cam online that it > > > became important to use a status property, and that only when the > > > kernel needs to be told that the device does indeed exist, but it must > > > not be touched. I'd like to continue that pattern for new DT users > > > with the default assumption that a device is enabled unless the board > > > .dts explicitly disables it. > [...] > > Besides the bothering that we have to list so many unused controllers > > in individual board dts file, it's also hard to tell which controllers > > are actually available on the board. People have to look at imx53.dts > > to get a full list and then exclude the ones in imx53-<board>.dts as > > "disabled". > > > > And if we go the way opposite, adding "disabled" status for everyone > > in imx53.dts, we will only need to specify the peripherals that are > > actually available on board with "okay" status in imx53-<board>.dts. > > And it's much more clear for people to see what peripherals are > > available on individual board. > > > > So I'm going the way than you suggested. Please let me know if you > > strongly dislikes it. > > Yes, I strongly dislike it. I understand the concern, but at this > early stage with converting to device tree I think consistency between > platforms is more important. We can talk about the issue at Linaro > Connect in 2 weeks, but in the mean time please use the > enabled-by-default/explicitly-disabled pattern. > Okay, hope you will not ask me to use the opposite pattern when you actually see the patch :)
On Wed, Jul 20, 2011 at 4:33 PM, Shawn Guo <shawn.guo@freescale.com> wrote: > On Wed, Jul 20, 2011 at 12:55:13PM -0600, Grant Likely wrote: >> On Wed, Jul 20, 2011 at 07:04:20PM +0800, Shawn Guo wrote: >> > > Mostly consistency. Most of the experience we have with the flattened >> > > device tree up to this point hasn't bothered with the 'status' >> > > property. It is only when AMP and hypervisors cam online that it >> > > became important to use a status property, and that only when the >> > > kernel needs to be told that the device does indeed exist, but it must >> > > not be touched. I'd like to continue that pattern for new DT users >> > > with the default assumption that a device is enabled unless the board >> > > .dts explicitly disables it. >> [...] >> > Besides the bothering that we have to list so many unused controllers >> > in individual board dts file, it's also hard to tell which controllers >> > are actually available on the board. People have to look at imx53.dts >> > to get a full list and then exclude the ones in imx53-<board>.dts as >> > "disabled". >> > >> > And if we go the way opposite, adding "disabled" status for everyone >> > in imx53.dts, we will only need to specify the peripherals that are >> > actually available on board with "okay" status in imx53-<board>.dts. >> > And it's much more clear for people to see what peripherals are >> > available on individual board. >> > >> > So I'm going the way than you suggested. Please let me know if you >> > strongly dislikes it. >> >> Yes, I strongly dislike it. I understand the concern, but at this >> early stage with converting to device tree I think consistency between >> platforms is more important. We can talk about the issue at Linaro >> Connect in 2 weeks, but in the mean time please use the >> enabled-by-default/explicitly-disabled pattern. >> > Okay, hope you will not ask me to use the opposite pattern when you > actually see the patch :) :-) g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts index 2607be5..479be11 100644 --- a/arch/arm/boot/dts/omap3-beagle-nunchuck.dts +++ b/arch/arm/boot/dts/omap3-beagle-nunchuck.dts @@ -2,11 +2,6 @@ / { i2c@48072000 { - compatible = "ti,omap3-i2c"; - reg = <0x48072000 0x80>; - #address-cells = <1>; - #size-cells = <0>; - eeprom@50 { compatible = "at,at24c01"; reg = < 0x50 >; diff --git a/arch/arm/boot/dts/omap3-beagle.dts b/arch/arm/boot/dts/omap3-beagle.dts index 4439466..491ee2b 100644 --- a/arch/arm/boot/dts/omap3-beagle.dts +++ b/arch/arm/boot/dts/omap3-beagle.dts @@ -4,4 +4,46 @@ / { model = "TI OMAP3 BeagleBoard"; compatible = "ti,omap3-beagle"; + interrupt-parent = <&gic>; + + gic: interrupt-controller@48241000 { + compatible = "ti,omap-gic", "arm,gic"; + interrupt-controller; + #interrupt-cells = <1>; + reg = <0x48200000 0x1000>; + }; + + i2c@48070000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "ti,omap_i2c"; + reg = <0x48070000 0x100>; + interrupts = < 88 >; + interrupt-parent = <&gic>; + clock-frequency = <2600>; + status = "disabled"; + }; + + i2c@48072000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "ti,omap_i2c"; + reg = <0x48072000 0x100>; + interrupts = < 89 >; + interrupt-parent = <&gic>; + clock-frequency = <400>; + status = "ok"; + }; + + i2c@48060000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "ti,omap_i2c"; + reg = <0x48060000 0x100>; + interrupts = < 93 >; + interrupt-parent = <&gic>; + clock-frequency = <100>; + status = "ok"; + }; + };
Add I2C and it's child device nodes for beagle board. The I2C1 controller child devices are not populated and it should be handled along with OMAP clock changes. Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com> --- arch/arm/boot/dts/omap3-beagle-nunchuck.dts | 5 --- arch/arm/boot/dts/omap3-beagle.dts | 42 +++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 5 deletions(-)