| Message ID | 20171128125330.363-4-yixun.lan@amlogic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 11/28, Yixun Lan wrote: > diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi > index b932a784b02a..36a2e98338a8 100644 > --- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi > +++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi > @@ -7,6 +7,7 @@ > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/interrupt-controller/irq.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > +#include <dt-bindings/clock/axg-clkc.h> > > / { > compatible = "amlogic,meson-axg"; > @@ -148,6 +149,20 @@ > #address-cells = <0>; > }; > > + hiubus: hiubus@ff63c000 { Maybe just call the node "bus@ff63c000"? > + compatible = "simple-bus"; > + reg = <0x0 0xff63c000 0x0 0x1c00>; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges = <0x0 0x0 0x0 0xff63c000 0x0 0x1c00>; > + > + clkc: clock-controller@0 { > + compatible = "amlogic,axg-clkc"; > + #clock-cells = <1>; > + reg = <0x0 0x0 0x0 0x320>; > + }; > + }; > + > mailbox: mailbox@ff63dc00 { > compatible = "amlogic,meson-gx-mhu", "amlogic,meson-gxbb-mhu"; > reg = <0 0xff63dc00 0 0x400>; > -- > 2.15.0 >
Hi Stephen On 11/30/17 03:35, Stephen Boyd wrote: > On 11/28, Yixun Lan wrote: >> diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi >> index b932a784b02a..36a2e98338a8 100644 >> --- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi >> +++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi >> @@ -7,6 +7,7 @@ >> #include <dt-bindings/gpio/gpio.h> >> #include <dt-bindings/interrupt-controller/irq.h> >> #include <dt-bindings/interrupt-controller/arm-gic.h> >> +#include <dt-bindings/clock/axg-clkc.h> >> >> / { >> compatible = "amlogic,meson-axg"; >> @@ -148,6 +149,20 @@ >> #address-cells = <0>; >> }; >> >> + hiubus: hiubus@ff63c000 { > > Maybe just call the node "bus@ff63c000"? > isn't this just a name? what's the benefits to change? personally, I tend to keep it this way, because it's better map to the data sheet we also has 'aobus', 'cbus' scattered there.. >> + compatible = "simple-bus"; >> + reg = <0x0 0xff63c000 0x0 0x1c00>; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges = <0x0 0x0 0x0 0xff63c000 0x0 0x1c00>; >> + >> + clkc: clock-controller@0 { >> + compatible = "amlogic,axg-clkc"; >> + #clock-cells = <1>; >> + reg = <0x0 0x0 0x0 0x320>; >> + }; >> + }; >> + >> mailbox: mailbox@ff63dc00 { >> compatible = "amlogic,meson-gx-mhu", "amlogic,meson-gxbb-mhu"; >> reg = <0 0xff63dc00 0 0x400>; >> -- >> 2.15.0 >> >
On 11/30, Yixun Lan wrote: > Hi Stephen > > On 11/30/17 03:35, Stephen Boyd wrote: > > On 11/28, Yixun Lan wrote: > >> diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi > >> index b932a784b02a..36a2e98338a8 100644 > >> --- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi > >> +++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi > >> @@ -7,6 +7,7 @@ > >> #include <dt-bindings/gpio/gpio.h> > >> #include <dt-bindings/interrupt-controller/irq.h> > >> #include <dt-bindings/interrupt-controller/arm-gic.h> > >> +#include <dt-bindings/clock/axg-clkc.h> > >> > >> / { > >> compatible = "amlogic,meson-axg"; > >> @@ -148,6 +149,20 @@ > >> #address-cells = <0>; > >> }; > >> > >> + hiubus: hiubus@ff63c000 { > > > > Maybe just call the node "bus@ff63c000"? > > > isn't this just a name? what's the benefits to change? > personally, I tend to keep it this way, because it's better map to the > data sheet > > we also has 'aobus', 'cbus' scattered there.. Per the ePAPR node names are supposed to be generic, like disk, cpu, display-controller, gpu, etc. I've never heard of a hiubus, so probably it's some vendor specific thing? We have the phandle anyway so it's not like we're losing much information here.
On Fri, 2017-12-01 at 08:34 -0800, Stephen Boyd wrote: > On 11/30, Yixun Lan wrote: > > Hi Stephen > > > > On 11/30/17 03:35, Stephen Boyd wrote: > > > On 11/28, Yixun Lan wrote: > > > > diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi > > > > b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi > > > > index b932a784b02a..36a2e98338a8 100644 > > > > --- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi > > > > +++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi > > > > @@ -7,6 +7,7 @@ > > > > #include <dt-bindings/gpio/gpio.h> > > > > #include <dt-bindings/interrupt-controller/irq.h> > > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > +#include <dt-bindings/clock/axg-clkc.h> > > > > > > > > / { > > > > compatible = "amlogic,meson-axg"; > > > > @@ -148,6 +149,20 @@ > > > > #address-cells = <0>; > > > > }; > > > > > > > > + hiubus: hiubus@ff63c000 { > > > > > > Maybe just call the node "bus@ff63c000"? > > > > > > > isn't this just a name? what's the benefits to change? > > personally, I tend to keep it this way, because it's better map to the > > data sheet > > > > we also has 'aobus', 'cbus' scattered there.. > > Per the ePAPR node names are supposed to be generic, like disk, > cpu, display-controller, gpu, etc. I've never heard of a hiubus, > so probably it's some vendor specific thing? We have the phandle > anyway so it's not like we're losing much information here. Stephen, there is a lot of busses on platform. We can't just call them all 'bus'. I don't get the problem with this name. We are re-using the name from the datasheet here, no fancy invention. It seems to be quite common. >
On 12/01, Jerome Brunet wrote: > On Fri, 2017-12-01 at 08:34 -0800, Stephen Boyd wrote: > > On 11/30, Yixun Lan wrote: > > > Hi Stephen > > > > > > On 11/30/17 03:35, Stephen Boyd wrote: > > > > > > > > Maybe just call the node "bus@ff63c000"? > > > > > > > > > > isn't this just a name? what's the benefits to change? > > > personally, I tend to keep it this way, because it's better map to the > > > data sheet > > > > > > we also has 'aobus', 'cbus' scattered there.. > > > > Per the ePAPR node names are supposed to be generic, like disk, > > cpu, display-controller, gpu, etc. I've never heard of a hiubus, > > so probably it's some vendor specific thing? We have the phandle > > anyway so it's not like we're losing much information here. > > Stephen, there is a lot of busses on platform. We can't just call them all > 'bus'. > I don't get the problem with this name. > We are re-using the name from the datasheet here, no fancy invention. It seems > to be quite common. > Ok. I'm not the maintainer of the DTS so no worries from me. I'm just pointing out that the ePAPR says that node names should be generic, and 'hiubus' doesn't sound generic to me. If it matches some datasheet then I suppose that's good, but probably that sort of distinction should have gone into the compatible string instead of the node name.
Stephen Boyd <sboyd@codeaurora.org> writes: > On 12/01, Jerome Brunet wrote: >> On Fri, 2017-12-01 at 08:34 -0800, Stephen Boyd wrote: >> > On 11/30, Yixun Lan wrote: >> > > Hi Stephen >> > > >> > > On 11/30/17 03:35, Stephen Boyd wrote: >> > > > >> > > > Maybe just call the node "bus@ff63c000"? >> > > > >> > > >> > > isn't this just a name? what's the benefits to change? >> > > personally, I tend to keep it this way, because it's better map to the >> > > data sheet >> > > >> > > we also has 'aobus', 'cbus' scattered there.. >> > >> > Per the ePAPR node names are supposed to be generic, like disk, >> > cpu, display-controller, gpu, etc. I've never heard of a hiubus, >> > so probably it's some vendor specific thing? We have the phandle >> > anyway so it's not like we're losing much information here. >> >> Stephen, there is a lot of busses on platform. We can't just call them all >> 'bus'. >> I don't get the problem with this name. >> We are re-using the name from the datasheet here, no fancy invention. It seems >> to be quite common. >> > > Ok. I'm not the maintainer of the DTS so no worries from me. I'm > just pointing out that the ePAPR says that node names should be > generic, and 'hiubus' doesn't sound generic to me. If it matches > some datasheet then I suppose that's good, but probably that sort > of distinction should have gone into the compatible string > instead of the node name. Stephen is right, the node-name should be generic (e.g. "bus") but the label can (should) be more SoC-specific, so it should look like: hiubus: bus@ff63c000 { Note that we weren't strict about this for all the rest of the amlogic SoCs (mostly because I didn't notice ) but we should start doing it correctly now. I'll also clean up the existing DTs. Kevin
diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi index b932a784b02a..36a2e98338a8 100644 --- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi @@ -7,6 +7,7 @@ #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/interrupt-controller/irq.h> #include <dt-bindings/interrupt-controller/arm-gic.h> +#include <dt-bindings/clock/axg-clkc.h> / { compatible = "amlogic,meson-axg"; @@ -148,6 +149,20 @@ #address-cells = <0>; }; + hiubus: hiubus@ff63c000 { + compatible = "simple-bus"; + reg = <0x0 0xff63c000 0x0 0x1c00>; + #address-cells = <2>; + #size-cells = <2>; + ranges = <0x0 0x0 0x0 0xff63c000 0x0 0x1c00>; + + clkc: clock-controller@0 { + compatible = "amlogic,axg-clkc"; + #clock-cells = <1>; + reg = <0x0 0x0 0x0 0x320>; + }; + }; + mailbox: mailbox@ff63dc00 { compatible = "amlogic,meson-gx-mhu", "amlogic,meson-gxbb-mhu"; reg = <0 0xff63dc00 0 0x400>;