| Message ID | 1440861022-22674-12-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Michal Simek |
| Headers | show |
Simon, 2015-08-30 0:10 GMT+09:00 Simon Glass <sjg@chromium.org>: > We need to mark some device tree nodes so that they are available before > relocation. This enables driver model to find these automatically. In the > case of SPL it ensures that these nodes will be retained in SPL. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > arch/arm/dts/zynq-7000.dtsi | 1 + > arch/arm/dts/zynq-microzed.dts | 5 +++++ > arch/arm/dts/zynq-picozed.dts | 5 +++++ > arch/arm/dts/zynq-zc702.dts | 1 + > arch/arm/dts/zynq-zc706.dts | 1 + > arch/arm/dts/zynq-zc770-xm010.dts | 1 + > arch/arm/dts/zynq-zc770-xm011.dts | 1 + > arch/arm/dts/zynq-zc770-xm012.dts | 1 + > arch/arm/dts/zynq-zc770-xm013.dts | 1 + > arch/arm/dts/zynq-zed.dts | 1 + > arch/arm/dts/zynq-zybo.dts | 1 + > 11 files changed, 19 insertions(+) > > diff --git a/arch/arm/dts/zynq-7000.dtsi b/arch/arm/dts/zynq-7000.dtsi > index 0b62cb0..12614f2 100644 > --- a/arch/arm/dts/zynq-7000.dtsi > +++ b/arch/arm/dts/zynq-7000.dtsi > @@ -54,6 +54,7 @@ > }; > > amba: amba { > + u-boot,dm-pre-reloc; > compatible = "simple-bus"; > #address-cells = <1>; > #size-cells = <1>; You are adding "u-boot,dm-pre-reloc" to a simple-bus node. With this commit, all the children are bound before relocation. simple_bus_post_bind() calls dm_scan_fdt_node() with false for 'pre_reloc_only'. I guess, this implementation is a problem. Currently, 'pre_reloc_only' can be specified for the first-level nodes. For example, assume node structure like this: amba { uart0 { }; pinctrl { }; usb { }; eth { }; }; Please tell me: how to bind "uart0" and "pinctrl", but not "usb", "eth" before relocation. Any idea to propagate 'pre_reloc_only' downwards?
On 08/29/2015 05:10 PM, Simon Glass wrote: > We need to mark some device tree nodes so that they are available before > relocation. This enables driver model to find these automatically. In the > case of SPL it ensures that these nodes will be retained in SPL. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > arch/arm/dts/zynq-7000.dtsi | 1 + > arch/arm/dts/zynq-microzed.dts | 5 +++++ > arch/arm/dts/zynq-picozed.dts | 5 +++++ > arch/arm/dts/zynq-zc702.dts | 1 + > arch/arm/dts/zynq-zc706.dts | 1 + > arch/arm/dts/zynq-zc770-xm010.dts | 1 + > arch/arm/dts/zynq-zc770-xm011.dts | 1 + > arch/arm/dts/zynq-zc770-xm012.dts | 1 + > arch/arm/dts/zynq-zc770-xm013.dts | 1 + > arch/arm/dts/zynq-zed.dts | 1 + > arch/arm/dts/zynq-zybo.dts | 1 + > 11 files changed, 19 insertions(+) > > diff --git a/arch/arm/dts/zynq-7000.dtsi b/arch/arm/dts/zynq-7000.dtsi > index 0b62cb0..12614f2 100644 > --- a/arch/arm/dts/zynq-7000.dtsi > +++ b/arch/arm/dts/zynq-7000.dtsi > @@ -54,6 +54,7 @@ > }; > > amba: amba { > + u-boot,dm-pre-reloc; > compatible = "simple-bus"; > #address-cells = <1>; > #size-cells = <1>; > diff --git a/arch/arm/dts/zynq-microzed.dts b/arch/arm/dts/zynq-microzed.dts > index c373a2c..5dff18e6 100644 > --- a/arch/arm/dts/zynq-microzed.dts > +++ b/arch/arm/dts/zynq-microzed.dts > @@ -21,3 +21,8 @@ > reg = <0 0x40000000>; > }; > }; > + > +&uart1 { > + u-boot,dm-pre-reloc; Was this reviewed on DT mailing list? TBH adding this to every node seems to me a lot of work. Why not just add one more uboot property to chosen with list of IPs which needs to be relocated? Thanks, Michal
Hi Masahiro, On 31 August 2015 at 04:01, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Simon, > > > 2015-08-30 0:10 GMT+09:00 Simon Glass <sjg@chromium.org>: >> We need to mark some device tree nodes so that they are available before >> relocation. This enables driver model to find these automatically. In the >> case of SPL it ensures that these nodes will be retained in SPL. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> arch/arm/dts/zynq-7000.dtsi | 1 + >> arch/arm/dts/zynq-microzed.dts | 5 +++++ >> arch/arm/dts/zynq-picozed.dts | 5 +++++ >> arch/arm/dts/zynq-zc702.dts | 1 + >> arch/arm/dts/zynq-zc706.dts | 1 + >> arch/arm/dts/zynq-zc770-xm010.dts | 1 + >> arch/arm/dts/zynq-zc770-xm011.dts | 1 + >> arch/arm/dts/zynq-zc770-xm012.dts | 1 + >> arch/arm/dts/zynq-zc770-xm013.dts | 1 + >> arch/arm/dts/zynq-zed.dts | 1 + >> arch/arm/dts/zynq-zybo.dts | 1 + >> 11 files changed, 19 insertions(+) >> >> diff --git a/arch/arm/dts/zynq-7000.dtsi b/arch/arm/dts/zynq-7000.dtsi >> index 0b62cb0..12614f2 100644 >> --- a/arch/arm/dts/zynq-7000.dtsi >> +++ b/arch/arm/dts/zynq-7000.dtsi >> @@ -54,6 +54,7 @@ >> }; >> >> amba: amba { >> + u-boot,dm-pre-reloc; >> compatible = "simple-bus"; >> #address-cells = <1>; >> #size-cells = <1>; > > > You are adding "u-boot,dm-pre-reloc" to a simple-bus node. > > With this commit, all the children are bound before relocation. That should not happen unless you put the property in each child. > > simple_bus_post_bind() calls dm_scan_fdt_node() with false for 'pre_reloc_only'. > I guess, this implementation is a problem. Yes, that is wrong. I remember noticing this at the time but not having it as a parameter so I wasn't sure how to best regenerate it. > > > Currently, 'pre_reloc_only' can be specified for the first-level nodes. > > For example, assume node structure like this: > > amba { > uart0 { > > }; > > pinctrl { > > }; > > usb { > > }; > > eth { > > }; > }; > > > > Please tell me: > how to bind "uart0" and "pinctrl", but not "usb", "eth" > before relocation. > > > Any idea to propagate 'pre_reloc_only' downwards? Checking the GD_FLG_RELOC is the only thing I can think of. We don't want to add parameter to the bind() method I think. Regards, Simon
Hi Michal, On 31 August 2015 at 05:30, Michal Simek <monstr@monstr.eu> wrote: > On 08/29/2015 05:10 PM, Simon Glass wrote: >> We need to mark some device tree nodes so that they are available before >> relocation. This enables driver model to find these automatically. In the >> case of SPL it ensures that these nodes will be retained in SPL. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> arch/arm/dts/zynq-7000.dtsi | 1 + >> arch/arm/dts/zynq-microzed.dts | 5 +++++ >> arch/arm/dts/zynq-picozed.dts | 5 +++++ >> arch/arm/dts/zynq-zc702.dts | 1 + >> arch/arm/dts/zynq-zc706.dts | 1 + >> arch/arm/dts/zynq-zc770-xm010.dts | 1 + >> arch/arm/dts/zynq-zc770-xm011.dts | 1 + >> arch/arm/dts/zynq-zc770-xm012.dts | 1 + >> arch/arm/dts/zynq-zc770-xm013.dts | 1 + >> arch/arm/dts/zynq-zed.dts | 1 + >> arch/arm/dts/zynq-zybo.dts | 1 + >> 11 files changed, 19 insertions(+) >> >> diff --git a/arch/arm/dts/zynq-7000.dtsi b/arch/arm/dts/zynq-7000.dtsi >> index 0b62cb0..12614f2 100644 >> --- a/arch/arm/dts/zynq-7000.dtsi >> +++ b/arch/arm/dts/zynq-7000.dtsi >> @@ -54,6 +54,7 @@ >> }; >> >> amba: amba { >> + u-boot,dm-pre-reloc; >> compatible = "simple-bus"; >> #address-cells = <1>; >> #size-cells = <1>; >> diff --git a/arch/arm/dts/zynq-microzed.dts b/arch/arm/dts/zynq-microzed.dts >> index c373a2c..5dff18e6 100644 >> --- a/arch/arm/dts/zynq-microzed.dts >> +++ b/arch/arm/dts/zynq-microzed.dts >> @@ -21,3 +21,8 @@ >> reg = <0 0x40000000>; >> }; >> }; >> + >> +&uart1 { >> + u-boot,dm-pre-reloc; > > Was this reviewed on DT mailing list? There is a thread going at present for Raspberry Pi but it had not yielded much light last time I looked. > > TBH adding this to every node seems to me a lot of work. This i how it works at present. Typically we only have a UART and that is not necessary since U-Boot can force-bind this. But when the UART is not in the root node we have to add something. > Why not just add one more uboot property to chosen with list of IPs > which needs to be relocated? You mean a list of devices needed before relocation? If you like you could look at working up a patch for this. I'm certainly interested in other ideas. It does need to be efficient. Regards, Simon
On 08/31/2015 03:54 PM, Simon Glass wrote: > Hi Michal, > > On 31 August 2015 at 05:30, Michal Simek <monstr@monstr.eu> wrote: >> On 08/29/2015 05:10 PM, Simon Glass wrote: >>> We need to mark some device tree nodes so that they are available before >>> relocation. This enables driver model to find these automatically. In the >>> case of SPL it ensures that these nodes will be retained in SPL. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> arch/arm/dts/zynq-7000.dtsi | 1 + >>> arch/arm/dts/zynq-microzed.dts | 5 +++++ >>> arch/arm/dts/zynq-picozed.dts | 5 +++++ >>> arch/arm/dts/zynq-zc702.dts | 1 + >>> arch/arm/dts/zynq-zc706.dts | 1 + >>> arch/arm/dts/zynq-zc770-xm010.dts | 1 + >>> arch/arm/dts/zynq-zc770-xm011.dts | 1 + >>> arch/arm/dts/zynq-zc770-xm012.dts | 1 + >>> arch/arm/dts/zynq-zc770-xm013.dts | 1 + >>> arch/arm/dts/zynq-zed.dts | 1 + >>> arch/arm/dts/zynq-zybo.dts | 1 + >>> 11 files changed, 19 insertions(+) >>> >>> diff --git a/arch/arm/dts/zynq-7000.dtsi b/arch/arm/dts/zynq-7000.dtsi >>> index 0b62cb0..12614f2 100644 >>> --- a/arch/arm/dts/zynq-7000.dtsi >>> +++ b/arch/arm/dts/zynq-7000.dtsi >>> @@ -54,6 +54,7 @@ >>> }; >>> >>> amba: amba { >>> + u-boot,dm-pre-reloc; >>> compatible = "simple-bus"; >>> #address-cells = <1>; >>> #size-cells = <1>; >>> diff --git a/arch/arm/dts/zynq-microzed.dts b/arch/arm/dts/zynq-microzed.dts >>> index c373a2c..5dff18e6 100644 >>> --- a/arch/arm/dts/zynq-microzed.dts >>> +++ b/arch/arm/dts/zynq-microzed.dts >>> @@ -21,3 +21,8 @@ >>> reg = <0 0x40000000>; >>> }; >>> }; >>> + >>> +&uart1 { >>> + u-boot,dm-pre-reloc; >> >> Was this reviewed on DT mailing list? > > There is a thread going at present for Raspberry Pi but it had not > yielded much light last time I looked. > >> >> TBH adding this to every node seems to me a lot of work. > > This i how it works at present. Typically we only have a UART and that > is not necessary since U-Boot can force-bind this. But when the UART > is not in the root node we have to add something. It is partially problem with DT mess that we have platforms with and without bus. :-) > >> Why not just add one more uboot property to chosen with list of IPs >> which needs to be relocated? > > You mean a list of devices needed before relocation? I mean something like this: chosen { u-boot,dm-pre-reloc = <&uart1 ...>; } And then just go through this list. I expect that you are looking for that property anyway. > If you like you could look at working up a patch for this. I'm > certainly interested in other ideas. It does need to be efficient. I will test this series and will look at it in more details soon. Thanks, Michal
Hi Michal, On 31 August 2015 at 08:16, Michal Simek <monstr@monstr.eu> wrote: > On 08/31/2015 03:54 PM, Simon Glass wrote: >> Hi Michal, >> >> On 31 August 2015 at 05:30, Michal Simek <monstr@monstr.eu> wrote: >>> On 08/29/2015 05:10 PM, Simon Glass wrote: >>>> We need to mark some device tree nodes so that they are available before >>>> relocation. This enables driver model to find these automatically. In the >>>> case of SPL it ensures that these nodes will be retained in SPL. >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>> --- >>>> >>>> arch/arm/dts/zynq-7000.dtsi | 1 + >>>> arch/arm/dts/zynq-microzed.dts | 5 +++++ >>>> arch/arm/dts/zynq-picozed.dts | 5 +++++ >>>> arch/arm/dts/zynq-zc702.dts | 1 + >>>> arch/arm/dts/zynq-zc706.dts | 1 + >>>> arch/arm/dts/zynq-zc770-xm010.dts | 1 + >>>> arch/arm/dts/zynq-zc770-xm011.dts | 1 + >>>> arch/arm/dts/zynq-zc770-xm012.dts | 1 + >>>> arch/arm/dts/zynq-zc770-xm013.dts | 1 + >>>> arch/arm/dts/zynq-zed.dts | 1 + >>>> arch/arm/dts/zynq-zybo.dts | 1 + >>>> 11 files changed, 19 insertions(+) >>>> >>>> diff --git a/arch/arm/dts/zynq-7000.dtsi b/arch/arm/dts/zynq-7000.dtsi >>>> index 0b62cb0..12614f2 100644 >>>> --- a/arch/arm/dts/zynq-7000.dtsi >>>> +++ b/arch/arm/dts/zynq-7000.dtsi >>>> @@ -54,6 +54,7 @@ >>>> }; >>>> >>>> amba: amba { >>>> + u-boot,dm-pre-reloc; >>>> compatible = "simple-bus"; >>>> #address-cells = <1>; >>>> #size-cells = <1>; >>>> diff --git a/arch/arm/dts/zynq-microzed.dts b/arch/arm/dts/zynq-microzed.dts >>>> index c373a2c..5dff18e6 100644 >>>> --- a/arch/arm/dts/zynq-microzed.dts >>>> +++ b/arch/arm/dts/zynq-microzed.dts >>>> @@ -21,3 +21,8 @@ >>>> reg = <0 0x40000000>; >>>> }; >>>> }; >>>> + >>>> +&uart1 { >>>> + u-boot,dm-pre-reloc; >>> >>> Was this reviewed on DT mailing list? >> >> There is a thread going at present for Raspberry Pi but it had not >> yielded much light last time I looked. >> >>> >>> TBH adding this to every node seems to me a lot of work. >> >> This i how it works at present. Typically we only have a UART and that >> is not necessary since U-Boot can force-bind this. But when the UART >> is not in the root node we have to add something. > > It is partially problem with DT mess that we have platforms with and > without bus. :-) > >> >>> Why not just add one more uboot property to chosen with list of IPs >>> which needs to be relocated? >> >> You mean a list of devices needed before relocation? > > I mean something like this: > > chosen { > u-boot,dm-pre-reloc = <&uart1 ...>; > } > > And then just go through this list. I expect that you are looking for > that property anyway. In this case wouldn't it need to list the simple-bus also? We also use this with fdtgrep to remove nodes not needed for SPL. So we would have to come up with a tool to make that work. At present 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we want (it finds nodes with that property). I'm actually not sure that this approach is any easier/better. What are the advantages? > > >> If you like you could look at working up a patch for this. I'm >> certainly interested in other ideas. It does need to be efficient. > > I will test this series and will look at it in more details soon. Thanks. BTW are there zynqmp dev boards available at reasonable cost? I did this Zynq series because I discovered some old patches that were not applied and decided to update then. It's a really interesting platform - FPGA, etc. Regards, Simon
On 08/29/2015 05:10 PM, Simon Glass wrote: > We need to mark some device tree nodes so that they are available before > relocation. This enables driver model to find these automatically. In the > case of SPL it ensures that these nodes will be retained in SPL. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > arch/arm/dts/zynq-7000.dtsi | 1 + > arch/arm/dts/zynq-microzed.dts | 5 +++++ > arch/arm/dts/zynq-picozed.dts | 5 +++++ > arch/arm/dts/zynq-zc702.dts | 1 + > arch/arm/dts/zynq-zc706.dts | 1 + > arch/arm/dts/zynq-zc770-xm010.dts | 1 + > arch/arm/dts/zynq-zc770-xm011.dts | 1 + > arch/arm/dts/zynq-zc770-xm012.dts | 1 + > arch/arm/dts/zynq-zc770-xm013.dts | 1 + > arch/arm/dts/zynq-zed.dts | 1 + > arch/arm/dts/zynq-zybo.dts | 1 + > 11 files changed, 19 insertions(+) > > diff --git a/arch/arm/dts/zynq-7000.dtsi b/arch/arm/dts/zynq-7000.dtsi > index 0b62cb0..12614f2 100644 > --- a/arch/arm/dts/zynq-7000.dtsi > +++ b/arch/arm/dts/zynq-7000.dtsi > @@ -54,6 +54,7 @@ > }; > > amba: amba { > + u-boot,dm-pre-reloc; One more point here IRC the first line in every node should be compatible string not any property that's why this should be move below. Thanks, Michal
On 09/01/2015 01:13 AM, Simon Glass wrote: > Hi Michal, > > On 31 August 2015 at 08:16, Michal Simek <monstr@monstr.eu> wrote: >> On 08/31/2015 03:54 PM, Simon Glass wrote: >>> Hi Michal, >>> >>> On 31 August 2015 at 05:30, Michal Simek <monstr@monstr.eu> wrote: >>>> On 08/29/2015 05:10 PM, Simon Glass wrote: >>>>> We need to mark some device tree nodes so that they are available before >>>>> relocation. This enables driver model to find these automatically. In the >>>>> case of SPL it ensures that these nodes will be retained in SPL. >>>>> >>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>> --- >>>>> >>>>> arch/arm/dts/zynq-7000.dtsi | 1 + >>>>> arch/arm/dts/zynq-microzed.dts | 5 +++++ >>>>> arch/arm/dts/zynq-picozed.dts | 5 +++++ >>>>> arch/arm/dts/zynq-zc702.dts | 1 + >>>>> arch/arm/dts/zynq-zc706.dts | 1 + >>>>> arch/arm/dts/zynq-zc770-xm010.dts | 1 + >>>>> arch/arm/dts/zynq-zc770-xm011.dts | 1 + >>>>> arch/arm/dts/zynq-zc770-xm012.dts | 1 + >>>>> arch/arm/dts/zynq-zc770-xm013.dts | 1 + >>>>> arch/arm/dts/zynq-zed.dts | 1 + >>>>> arch/arm/dts/zynq-zybo.dts | 1 + >>>>> 11 files changed, 19 insertions(+) >>>>> >>>>> diff --git a/arch/arm/dts/zynq-7000.dtsi b/arch/arm/dts/zynq-7000.dtsi >>>>> index 0b62cb0..12614f2 100644 >>>>> --- a/arch/arm/dts/zynq-7000.dtsi >>>>> +++ b/arch/arm/dts/zynq-7000.dtsi >>>>> @@ -54,6 +54,7 @@ >>>>> }; >>>>> >>>>> amba: amba { >>>>> + u-boot,dm-pre-reloc; >>>>> compatible = "simple-bus"; >>>>> #address-cells = <1>; >>>>> #size-cells = <1>; >>>>> diff --git a/arch/arm/dts/zynq-microzed.dts b/arch/arm/dts/zynq-microzed.dts >>>>> index c373a2c..5dff18e6 100644 >>>>> --- a/arch/arm/dts/zynq-microzed.dts >>>>> +++ b/arch/arm/dts/zynq-microzed.dts >>>>> @@ -21,3 +21,8 @@ >>>>> reg = <0 0x40000000>; >>>>> }; >>>>> }; >>>>> + >>>>> +&uart1 { >>>>> + u-boot,dm-pre-reloc; >>>> >>>> Was this reviewed on DT mailing list? >>> >>> There is a thread going at present for Raspberry Pi but it had not >>> yielded much light last time I looked. >>> >>>> >>>> TBH adding this to every node seems to me a lot of work. >>> >>> This i how it works at present. Typically we only have a UART and that >>> is not necessary since U-Boot can force-bind this. But when the UART >>> is not in the root node we have to add something. >> >> It is partially problem with DT mess that we have platforms with and >> without bus. :-) >> >>> >>>> Why not just add one more uboot property to chosen with list of IPs >>>> which needs to be relocated? >>> >>> You mean a list of devices needed before relocation? >> >> I mean something like this: >> >> chosen { >> u-boot,dm-pre-reloc = <&uart1 ...>; >> } >> >> And then just go through this list. I expect that you are looking for >> that property anyway. > > In this case wouldn't it need to list the simple-bus also? yes for zc702 case u-boot,dm-pre-reloc = <&amba &uart1>; > > We also use this with fdtgrep to remove nodes not needed for SPL. So > we would have to come up with a tool to make that work. At present > 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we want (it > finds nodes with that property). > > I'm actually not sure that this approach is any easier/better. What > are the advantages? The question is if current solution which you are using is fully compatible with binding. Adding bootloader property to the HW node doesn't look like a best solution. On the other hand chosen node is the location where OS specific properties are coming that's why I have suggested to use it. >> >> >>> If you like you could look at working up a patch for this. I'm >>> certainly interested in other ideas. It does need to be efficient. >> >> I will test this series and will look at it in more details soon. > > Thanks. > > BTW are there zynqmp dev boards available at reasonable cost? I did > this Zynq series because I discovered some old patches that were not > applied and decided to update then. It's a really interesting platform > - FPGA, etc. Not now. But you can use qemu platform. I will look at it to see what there is. But I have confirmation that you should be able to run u-boot there. Thanks, Michal
Simon, 2015-08-31 22:54 GMT+09:00 Simon Glass <sjg@chromium.org>: >> >> >> Any idea to propagate 'pre_reloc_only' downwards? > > Checking the GD_FLG_RELOC is the only thing I can think of. We don't > want to add parameter to the bind() method I think. Sounds good to me. Are you willing to do this? For now, I am planing to increase CONFIG_SYS_MALLOC_F_LEN to work around the "All or Nothing" logic of the simple-bus binding. http://patchwork.ozlabs.org/patch/512847/
Hi. 2015-09-02 0:41 GMT+09:00 Michal Simek <monstr@monstr.eu>: >>> >>>> >>>>> Why not just add one more uboot property to chosen with list of IPs >>>>> which needs to be relocated? >>>> >>>> You mean a list of devices needed before relocation? >>> >>> I mean something like this: >>> >>> chosen { >>> u-boot,dm-pre-reloc = <&uart1 ...>; >>> } >>> >>> And then just go through this list. I expect that you are looking for >>> that property anyway. >> >> In this case wouldn't it need to list the simple-bus also? > > yes for zc702 case > > u-boot,dm-pre-reloc = <&amba &uart1>; I think this should be u-boot,dm-pre-reloc = &amba, &uart1; <&label> is used for phandle, while &label is replaced with a string standing for the full path for the node. For example, stdout-path takes that: stdout-path = &serial0; > >> >> We also use this with fdtgrep to remove nodes not needed for SPL. So >> we would have to come up with a tool to make that work. At present >> 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we want (it >> finds nodes with that property). >> >> I'm actually not sure that this approach is any easier/better. What >> are the advantages? > > The question is if current solution which you are using is fully > compatible with binding. Adding bootloader property to the HW node > doesn't look like a best solution. > On the other hand chosen node is the location where OS specific > properties are coming that's why I have suggested to use it. I like Michal's idea. We need some work for fdtgrep, but I believe it is worthwhile. From Michal's recent patches (http://patchwork.ozlabs.org/patch/498609/), I guess he is tackling on syncing device trees between Linux and U-boot, and this is right thing to do. Inserting the U-boot specific property here and there makes it difficult.
+Tom and a few others who may have an opinion. Hi, On 1 September 2015 at 10:19, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi. > > > 2015-09-02 0:41 GMT+09:00 Michal Simek <monstr@monstr.eu>: > >>>> >>>>> >>>>>> Why not just add one more uboot property to chosen with list of IPs >>>>>> which needs to be relocated? >>>>> >>>>> You mean a list of devices needed before relocation? >>>> >>>> I mean something like this: >>>> >>>> chosen { >>>> u-boot,dm-pre-reloc = <&uart1 ...>; >>>> } >>>> >>>> And then just go through this list. I expect that you are looking for >>>> that property anyway. >>> >>> In this case wouldn't it need to list the simple-bus also? >> >> yes for zc702 case >> >> u-boot,dm-pre-reloc = <&amba &uart1>; > > I think this should be > > u-boot,dm-pre-reloc = &amba, &uart1; > > > <&label> is used for phandle, while &label is replaced with a string > standing for the full path for the node. > > > For example, stdout-path takes that: > > > stdout-path = &serial0; > > > > > >> >>> >>> We also use this with fdtgrep to remove nodes not needed for SPL. So >>> we would have to come up with a tool to make that work. At present >>> 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we want (it >>> finds nodes with that property). >>> >>> I'm actually not sure that this approach is any easier/better. What >>> are the advantages? >> >> The question is if current solution which you are using is fully >> compatible with binding. Adding bootloader property to the HW node >> doesn't look like a best solution. >> On the other hand chosen node is the location where OS specific >> properties are coming that's why I have suggested to use it. > > > I like Michal's idea. > We need some work for fdtgrep, but I believe it is worthwhile. > > From Michal's recent patches (http://patchwork.ozlabs.org/patch/498609/), > I guess he is tackling on syncing device trees between Linux and U-boot, > and this is right thing to do. > > Inserting the U-boot specific property here and there > makes it difficult. Yes it is attractive. With this scheme we cannot put the properties into .dtsi (i.e. make them common for the soc). Is there a way around that or would we just have to live with it? If we go this way, who is going to write the patch?? Regards, Simon
On 09/02/2015 04:49 AM, Simon Glass wrote: > +Tom and a few others who may have an opinion. > > Hi, > > On 1 September 2015 at 10:19, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> Hi. >> >> >> 2015-09-02 0:41 GMT+09:00 Michal Simek <monstr@monstr.eu>: >> >>>>> >>>>>> >>>>>>> Why not just add one more uboot property to chosen with list of IPs >>>>>>> which needs to be relocated? >>>>>> >>>>>> You mean a list of devices needed before relocation? >>>>> >>>>> I mean something like this: >>>>> >>>>> chosen { >>>>> u-boot,dm-pre-reloc = <&uart1 ...>; >>>>> } >>>>> >>>>> And then just go through this list. I expect that you are looking for >>>>> that property anyway. >>>> >>>> In this case wouldn't it need to list the simple-bus also? >>> >>> yes for zc702 case >>> >>> u-boot,dm-pre-reloc = <&amba &uart1>; >> >> I think this should be >> >> u-boot,dm-pre-reloc = &amba, &uart1; >> >> >> <&label> is used for phandle, while &label is replaced with a string >> standing for the full path for the node. >> >> >> For example, stdout-path takes that: >> >> >> stdout-path = &serial0; >> >> >> >> >> >>> >>>> >>>> We also use this with fdtgrep to remove nodes not needed for SPL. So >>>> we would have to come up with a tool to make that work. At present >>>> 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we want (it >>>> finds nodes with that property). >>>> >>>> I'm actually not sure that this approach is any easier/better. What >>>> are the advantages? >>> >>> The question is if current solution which you are using is fully >>> compatible with binding. Adding bootloader property to the HW node >>> doesn't look like a best solution. >>> On the other hand chosen node is the location where OS specific >>> properties are coming that's why I have suggested to use it. >> >> >> I like Michal's idea. >> We need some work for fdtgrep, but I believe it is worthwhile. >> >> From Michal's recent patches (http://patchwork.ozlabs.org/patch/498609/), >> I guess he is tackling on syncing device trees between Linux and U-boot, >> and this is right thing to do. >> >> Inserting the U-boot specific property here and there >> makes it difficult. > > Yes it is attractive. > > With this scheme we cannot put the properties into .dtsi (i.e. make > them common for the soc). Is there a way around that or would we just > have to live with it? Why not? You can add chosen node to dtsi without any problem which should be shared for all boards. The only one question remains which is what exactly you need to add to dtsi. One example is Zynq. We have 2 serial PS IPs. Which one you want to add? Both? System can have 0, 1 or 2 uarts or uart on any other IP in PL. Thanks, Michal
Hi Michal, On 3 September 2015 at 05:35, Michal Simek <monstr@monstr.eu> wrote: > On 09/02/2015 04:49 AM, Simon Glass wrote: >> +Tom and a few others who may have an opinion. >> >> Hi, >> >> On 1 September 2015 at 10:19, Masahiro Yamada >> <yamada.masahiro@socionext.com> wrote: >>> Hi. >>> >>> >>> 2015-09-02 0:41 GMT+09:00 Michal Simek <monstr@monstr.eu>: >>> >>>>>> >>>>>>> >>>>>>>> Why not just add one more uboot property to chosen with list of IPs >>>>>>>> which needs to be relocated? >>>>>>> >>>>>>> You mean a list of devices needed before relocation? >>>>>> >>>>>> I mean something like this: >>>>>> >>>>>> chosen { >>>>>> u-boot,dm-pre-reloc = <&uart1 ...>; >>>>>> } >>>>>> >>>>>> And then just go through this list. I expect that you are looking for >>>>>> that property anyway. >>>>> >>>>> In this case wouldn't it need to list the simple-bus also? >>>> >>>> yes for zc702 case >>>> >>>> u-boot,dm-pre-reloc = <&amba &uart1>; >>> >>> I think this should be >>> >>> u-boot,dm-pre-reloc = &amba, &uart1; >>> >>> >>> <&label> is used for phandle, while &label is replaced with a string >>> standing for the full path for the node. >>> >>> >>> For example, stdout-path takes that: >>> >>> >>> stdout-path = &serial0; >>> >>> >>> >>> >>> >>>> >>>>> >>>>> We also use this with fdtgrep to remove nodes not needed for SPL. So >>>>> we would have to come up with a tool to make that work. At present >>>>> 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we want (it >>>>> finds nodes with that property). >>>>> >>>>> I'm actually not sure that this approach is any easier/better. What >>>>> are the advantages? >>>> >>>> The question is if current solution which you are using is fully >>>> compatible with binding. Adding bootloader property to the HW node >>>> doesn't look like a best solution. >>>> On the other hand chosen node is the location where OS specific >>>> properties are coming that's why I have suggested to use it. >>> >>> >>> I like Michal's idea. >>> We need some work for fdtgrep, but I believe it is worthwhile. >>> >>> From Michal's recent patches (http://patchwork.ozlabs.org/patch/498609/), >>> I guess he is tackling on syncing device trees between Linux and U-boot, >>> and this is right thing to do. >>> >>> Inserting the U-boot specific property here and there >>> makes it difficult. >> >> Yes it is attractive. >> >> With this scheme we cannot put the properties into .dtsi (i.e. make >> them common for the soc). Is there a way around that or would we just >> have to live with it? > > Why not? You can add chosen node to dtsi without any problem which > should be shared for all boards. The only one question remains which is > what exactly you need to add to dtsi. > One example is Zynq. We have 2 serial PS IPs. Which one you want to add? > Both? If you have something like this: soc { uart0 { }; uart1 { }; } in the common .dtsi then you can currently put the marker in the soc node. I'm not sure how you do that with your scheme. If we put it in the .dtsi then the .dts will override it. Does this matter? > > System can have 0, 1 or 2 uarts or uart on any other IP in PL. > > Thanks, > Michal Regards, Simon
On 09/04/2015 02:22 AM, Simon Glass wrote: > Hi Michal, > > On 3 September 2015 at 05:35, Michal Simek <monstr@monstr.eu> wrote: >> On 09/02/2015 04:49 AM, Simon Glass wrote: >>> +Tom and a few others who may have an opinion. >>> >>> Hi, >>> >>> On 1 September 2015 at 10:19, Masahiro Yamada >>> <yamada.masahiro@socionext.com> wrote: >>>> Hi. >>>> >>>> >>>> 2015-09-02 0:41 GMT+09:00 Michal Simek <monstr@monstr.eu>: >>>> >>>>>>> >>>>>>>> >>>>>>>>> Why not just add one more uboot property to chosen with list of IPs >>>>>>>>> which needs to be relocated? >>>>>>>> >>>>>>>> You mean a list of devices needed before relocation? >>>>>>> >>>>>>> I mean something like this: >>>>>>> >>>>>>> chosen { >>>>>>> u-boot,dm-pre-reloc = <&uart1 ...>; >>>>>>> } >>>>>>> >>>>>>> And then just go through this list. I expect that you are looking for >>>>>>> that property anyway. >>>>>> >>>>>> In this case wouldn't it need to list the simple-bus also? >>>>> >>>>> yes for zc702 case >>>>> >>>>> u-boot,dm-pre-reloc = <&amba &uart1>; >>>> >>>> I think this should be >>>> >>>> u-boot,dm-pre-reloc = &amba, &uart1; >>>> >>>> >>>> <&label> is used for phandle, while &label is replaced with a string >>>> standing for the full path for the node. >>>> >>>> >>>> For example, stdout-path takes that: >>>> >>>> >>>> stdout-path = &serial0; >>>> >>>> >>>> >>>> >>>> >>>>> >>>>>> >>>>>> We also use this with fdtgrep to remove nodes not needed for SPL. So >>>>>> we would have to come up with a tool to make that work. At present >>>>>> 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we want (it >>>>>> finds nodes with that property). >>>>>> >>>>>> I'm actually not sure that this approach is any easier/better. What >>>>>> are the advantages? >>>>> >>>>> The question is if current solution which you are using is fully >>>>> compatible with binding. Adding bootloader property to the HW node >>>>> doesn't look like a best solution. >>>>> On the other hand chosen node is the location where OS specific >>>>> properties are coming that's why I have suggested to use it. >>>> >>>> >>>> I like Michal's idea. >>>> We need some work for fdtgrep, but I believe it is worthwhile. >>>> >>>> From Michal's recent patches (http://patchwork.ozlabs.org/patch/498609/), >>>> I guess he is tackling on syncing device trees between Linux and U-boot, >>>> and this is right thing to do. >>>> >>>> Inserting the U-boot specific property here and there >>>> makes it difficult. >>> >>> Yes it is attractive. >>> >>> With this scheme we cannot put the properties into .dtsi (i.e. make >>> them common for the soc). Is there a way around that or would we just >>> have to live with it? >> >> Why not? You can add chosen node to dtsi without any problem which >> should be shared for all boards. The only one question remains which is >> what exactly you need to add to dtsi. >> One example is Zynq. We have 2 serial PS IPs. Which one you want to add? >> Both? > > If you have something like this: > > soc { > uart0 { > }; > uart1 { > }; > } > > in the common .dtsi then you can currently put the marker in the soc > node. I'm not sure how you do that with your scheme. If we put it in > the .dtsi then the .dts will override it. > > Does this matter? As far as I understand DTSI is shared across all boards. And DTS can overwrite DTSI at any time. Let me extend this to make it more clear soc: soc { uart0: uart@XXX { }; uart1: uart@XXX { }; } In DTSI I would put probably this to show everybody what needs to be there. chosen { u-boot,dm-pre-reloc = &soc &uart0 &uart1; } And in DTS if you have only one uart DTS will overwrite it. chosen { u-boot,dm-pre-reloc = &soc &uart0; } Or the next option is to make code smarter and detect that uart1 is disabled that's why it is not used and only chosen from DTSI should be enough. Thanks, Michal
Hi Michal, On 4 September 2015 at 00:04, Michal Simek <monstr@monstr.eu> wrote: > On 09/04/2015 02:22 AM, Simon Glass wrote: >> Hi Michal, >> >> On 3 September 2015 at 05:35, Michal Simek <monstr@monstr.eu> wrote: >>> On 09/02/2015 04:49 AM, Simon Glass wrote: >>>> +Tom and a few others who may have an opinion. >>>> >>>> Hi, >>>> >>>> On 1 September 2015 at 10:19, Masahiro Yamada >>>> <yamada.masahiro@socionext.com> wrote: >>>>> Hi. >>>>> >>>>> >>>>> 2015-09-02 0:41 GMT+09:00 Michal Simek <monstr@monstr.eu>: >>>>> >>>>>>>> >>>>>>>>> >>>>>>>>>> Why not just add one more uboot property to chosen with list of IPs >>>>>>>>>> which needs to be relocated? >>>>>>>>> >>>>>>>>> You mean a list of devices needed before relocation? >>>>>>>> >>>>>>>> I mean something like this: >>>>>>>> >>>>>>>> chosen { >>>>>>>> u-boot,dm-pre-reloc = <&uart1 ...>; >>>>>>>> } >>>>>>>> >>>>>>>> And then just go through this list. I expect that you are looking for >>>>>>>> that property anyway. >>>>>>> >>>>>>> In this case wouldn't it need to list the simple-bus also? >>>>>> >>>>>> yes for zc702 case >>>>>> >>>>>> u-boot,dm-pre-reloc = <&amba &uart1>; >>>>> >>>>> I think this should be >>>>> >>>>> u-boot,dm-pre-reloc = &amba, &uart1; >>>>> >>>>> >>>>> <&label> is used for phandle, while &label is replaced with a string >>>>> standing for the full path for the node. >>>>> >>>>> >>>>> For example, stdout-path takes that: >>>>> >>>>> >>>>> stdout-path = &serial0; >>>>> >>>>> >>>>> >>>>> >>>>> >>>>>> >>>>>>> >>>>>>> We also use this with fdtgrep to remove nodes not needed for SPL. So >>>>>>> we would have to come up with a tool to make that work. At present >>>>>>> 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we want (it >>>>>>> finds nodes with that property). >>>>>>> >>>>>>> I'm actually not sure that this approach is any easier/better. What >>>>>>> are the advantages? >>>>>> >>>>>> The question is if current solution which you are using is fully >>>>>> compatible with binding. Adding bootloader property to the HW node >>>>>> doesn't look like a best solution. >>>>>> On the other hand chosen node is the location where OS specific >>>>>> properties are coming that's why I have suggested to use it. >>>>> >>>>> >>>>> I like Michal's idea. >>>>> We need some work for fdtgrep, but I believe it is worthwhile. >>>>> >>>>> From Michal's recent patches (http://patchwork.ozlabs.org/patch/498609/), >>>>> I guess he is tackling on syncing device trees between Linux and U-boot, >>>>> and this is right thing to do. >>>>> >>>>> Inserting the U-boot specific property here and there >>>>> makes it difficult. >>>> >>>> Yes it is attractive. >>>> >>>> With this scheme we cannot put the properties into .dtsi (i.e. make >>>> them common for the soc). Is there a way around that or would we just >>>> have to live with it? >>> >>> Why not? You can add chosen node to dtsi without any problem which >>> should be shared for all boards. The only one question remains which is >>> what exactly you need to add to dtsi. >>> One example is Zynq. We have 2 serial PS IPs. Which one you want to add? >>> Both? >> >> If you have something like this: >> >> soc { >> uart0 { >> }; >> uart1 { >> }; >> } >> >> in the common .dtsi then you can currently put the marker in the soc >> node. I'm not sure how you do that with your scheme. If we put it in >> the .dtsi then the .dts will override it. >> >> Does this matter? > > As far as I understand DTSI is shared across all boards. > And DTS can overwrite DTSI at any time. > > Let me extend this to make it more clear > soc: soc { > uart0: uart@XXX { > }; > uart1: uart@XXX { > }; > } > > > In DTSI I would put probably this to show everybody what needs to be there. > chosen { > u-boot,dm-pre-reloc = &soc &uart0 &uart1; > } > > And in DTS if you have only one uart DTS will overwrite it. > chosen { > u-boot,dm-pre-reloc = &soc &uart0; > } > > Or the next option is to make code smarter and detect that uart1 is > disabled that's why it is not used and only chosen from DTSI should be > enough. Yes I see - you just overwrite the property in the main file. If it includes pinctrl nodes, gpio nodes, etc. then you would have to add those also. For mainline Rockchip this means that each board would have to have something like: chosen { u-boot,dm-pre-reloc = &mmc0, &serial0, &dmc, &power, &syscon0 &syscon1 &pinctrl &gpio3 &gpio8 &leds &led_work &led_power; } That seems a bit unwieldy to me. What do you think? Regards, Simon
Hi Simon, <cut some previous parts> >>>>>>>> >>>>>>>> We also use this with fdtgrep to remove nodes not needed for SPL. So >>>>>>>> we would have to come up with a tool to make that work. At present >>>>>>>> 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we want (it >>>>>>>> finds nodes with that property). >>>>>>>> >>>>>>>> I'm actually not sure that this approach is any easier/better. What >>>>>>>> are the advantages? >>>>>>> >>>>>>> The question is if current solution which you are using is fully >>>>>>> compatible with binding. Adding bootloader property to the HW node >>>>>>> doesn't look like a best solution. >>>>>>> On the other hand chosen node is the location where OS specific >>>>>>> properties are coming that's why I have suggested to use it. >>>>>> >>>>>> >>>>>> I like Michal's idea. >>>>>> We need some work for fdtgrep, but I believe it is worthwhile. >>>>>> >>>>>> From Michal's recent patches (http://patchwork.ozlabs.org/patch/498609/), >>>>>> I guess he is tackling on syncing device trees between Linux and U-boot, >>>>>> and this is right thing to do. >>>>>> >>>>>> Inserting the U-boot specific property here and there >>>>>> makes it difficult. >>>>> >>>>> Yes it is attractive. >>>>> >>>>> With this scheme we cannot put the properties into .dtsi (i.e. make >>>>> them common for the soc). Is there a way around that or would we just >>>>> have to live with it? >>>> >>>> Why not? You can add chosen node to dtsi without any problem which >>>> should be shared for all boards. The only one question remains which is >>>> what exactly you need to add to dtsi. >>>> One example is Zynq. We have 2 serial PS IPs. Which one you want to add? >>>> Both? >>> >>> If you have something like this: >>> >>> soc { >>> uart0 { >>> }; >>> uart1 { >>> }; >>> } >>> >>> in the common .dtsi then you can currently put the marker in the soc >>> node. I'm not sure how you do that with your scheme. If we put it in >>> the .dtsi then the .dts will override it. >>> >>> Does this matter? >> >> As far as I understand DTSI is shared across all boards. >> And DTS can overwrite DTSI at any time. >> >> Let me extend this to make it more clear >> soc: soc { >> uart0: uart@XXX { >> }; >> uart1: uart@XXX { >> }; >> } >> >> >> In DTSI I would put probably this to show everybody what needs to be there. >> chosen { >> u-boot,dm-pre-reloc = &soc &uart0 &uart1; >> } >> >> And in DTS if you have only one uart DTS will overwrite it. >> chosen { >> u-boot,dm-pre-reloc = &soc &uart0; >> } >> >> Or the next option is to make code smarter and detect that uart1 is >> disabled that's why it is not used and only chosen from DTSI should be >> enough. > > Yes I see - you just overwrite the property in the main file. If it > includes pinctrl nodes, gpio nodes, etc. then you would have to add > those also. Right - DTSI is most like a pattern to follow and DTS is doing that platform specific stuff. It means in DTSI you can have guidance what to use. Also in algorithm can be checking if that IP has status okay or disabled and do relocation or not. > For mainline Rockchip this means that each board would > have to have something like: > > chosen { > u-boot,dm-pre-reloc = &mmc0, &serial0, &dmc, &power, &syscon0 > &syscon1 &pinctrl &gpio3 &gpio8 &leds &led_work &led_power; > } > > That seems a bit unwieldy to me. What do you think? (note: without commas) I think that this solution is compatible with the binding and it is better than adding bootloader specific property to nodes which I have never seen before. Chosen node was used for that maybe even from beginning. Regarding technical aspects - I have never checked if there is any line length limitation on parameter side which can be problematic. On the other hand if yes phandles should be used. Regarding compactness of this solution. From one line you can see what will be pre relocated which looks pretty compact to me and you can easy check if something is missing. Thanks, Michal
Hi Michal, On Friday, 4 September 2015, Michal Simek <monstr@monstr.eu> wrote: > > Hi Simon, > > <cut some previous parts> > > >>>>>>>> > >>>>>>>> We also use this with fdtgrep to remove nodes not needed for SPL. So > >>>>>>>> we would have to come up with a tool to make that work. At present > >>>>>>>> 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we want (it > >>>>>>>> finds nodes with that property). > >>>>>>>> > >>>>>>>> I'm actually not sure that this approach is any easier/better. What > >>>>>>>> are the advantages? > >>>>>>> > >>>>>>> The question is if current solution which you are using is fully > >>>>>>> compatible with binding. Adding bootloader property to the HW node > >>>>>>> doesn't look like a best solution. > >>>>>>> On the other hand chosen node is the location where OS specific > >>>>>>> properties are coming that's why I have suggested to use it. > >>>>>> > >>>>>> > >>>>>> I like Michal's idea. > >>>>>> We need some work for fdtgrep, but I believe it is worthwhile. > >>>>>> > >>>>>> From Michal's recent patches (http://patchwork.ozlabs.org/patch/498609/), > >>>>>> I guess he is tackling on syncing device trees between Linux and U-boot, > >>>>>> and this is right thing to do. > >>>>>> > >>>>>> Inserting the U-boot specific property here and there > >>>>>> makes it difficult. > >>>>> > >>>>> Yes it is attractive. > >>>>> > >>>>> With this scheme we cannot put the properties into .dtsi (i.e. make > >>>>> them common for the soc). Is there a way around that or would we just > >>>>> have to live with it? > >>>> > >>>> Why not? You can add chosen node to dtsi without any problem which > >>>> should be shared for all boards. The only one question remains which is > >>>> what exactly you need to add to dtsi. > >>>> One example is Zynq. We have 2 serial PS IPs. Which one you want to add? > >>>> Both? > >>> > >>> If you have something like this: > >>> > >>> soc { > >>> uart0 { > >>> }; > >>> uart1 { > >>> }; > >>> } > >>> > >>> in the common .dtsi then you can currently put the marker in the soc > >>> node. I'm not sure how you do that with your scheme. If we put it in > >>> the .dtsi then the .dts will override it. > >>> > >>> Does this matter? > >> > >> As far as I understand DTSI is shared across all boards. > >> And DTS can overwrite DTSI at any time. > >> > >> Let me extend this to make it more clear > >> soc: soc { > >> uart0: uart@XXX { > >> }; > >> uart1: uart@XXX { > >> }; > >> } > >> > >> > >> In DTSI I would put probably this to show everybody what needs to be there. > >> chosen { > >> u-boot,dm-pre-reloc = &soc &uart0 &uart1; > >> } > >> > >> And in DTS if you have only one uart DTS will overwrite it. > >> chosen { > >> u-boot,dm-pre-reloc = &soc &uart0; > >> } > >> > >> Or the next option is to make code smarter and detect that uart1 is > >> disabled that's why it is not used and only chosen from DTSI should be > >> enough. > > > > Yes I see - you just overwrite the property in the main file. If it > > includes pinctrl nodes, gpio nodes, etc. then you would have to add > > those also. > > Right - DTSI is most like a pattern to follow and DTS is doing that > platform specific stuff. It means in DTSI you can have guidance what to > use. Do you mean we should add a comment to the .dtsi to tell you want you need in your .dts? Is this improving anything? > > Also in algorithm can be checking if that IP has status okay or disabled > and do relocation or not. Can you please rephrase that? I don't understand. > > > > For mainline Rockchip this means that each board would > > have to have something like: > > > > chosen { > > u-boot,dm-pre-reloc = &mmc0, &serial0, &dmc, &power, &syscon0 > > &syscon1 &pinctrl &gpio3 &gpio8 &leds &led_work &led_power; > > } > > > > That seems a bit unwieldy to me. What do you think? > > (note: without commas) > I think that this solution is compatible with the binding and it is > better than adding bootloader specific property to nodes which I have > never seen before. Chosen node was used for that maybe even from beginning. There are Linux-specific properties, so I really don't see the difference. Don't forget that we are using device tree to control our boot loader, so it would be unusual to not see at least some U-Boot-specific properties. My understanding of /chosen is that it is for the OS. But in any case it would still be a binding change, wouldn't it? What makes you think that this scheme would be more acceptable as a binding change? > > > Regarding technical aspects - I have never checked if there is any line > length limitation on parameter side which can be problematic. On the > other hand if yes phandles should be used. No there is not limit I know of. It should be fine. > > > Regarding compactness of this solution. From one line you can see what > will be pre relocated which looks pretty compact to me and you can easy > check if something is missing. Yes it is nice to have this in once place. But it's not really in one place as you need to reference nodes from elsewhere, and update all .dts files if something in the .dtsi changes. Granted that shouldn't happen often. One advantage is that this copes with boards that have a common .dtsi but different drivers needed before relocation - e.g. one board might want to use a serial port attached to a /soc node, another might want to use something on /pci. But I have not seen this happen yet. So overall i can see benefits and drawbacks. I'm on the fence. Regards, Simon
Hi Simon, first of all sorry for late reply. I am traveling and I have pretty busy time now. 2015-09-09 20:07 GMT+02:00 Simon Glass <sjg@chromium.org>: > Hi Michal, > > On Friday, 4 September 2015, Michal Simek <monstr@monstr.eu> wrote: > > > > Hi Simon, > > > > <cut some previous parts> > > > > >>>>>>>> > > >>>>>>>> We also use this with fdtgrep to remove nodes not needed for > SPL. So > > >>>>>>>> we would have to come up with a tool to make that work. At > present > > >>>>>>>> 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we > want (it > > >>>>>>>> finds nodes with that property). > > >>>>>>>> > > >>>>>>>> I'm actually not sure that this approach is any easier/better. > What > > >>>>>>>> are the advantages? > > >>>>>>> > > >>>>>>> The question is if current solution which you are using is fully > > >>>>>>> compatible with binding. Adding bootloader property to the HW > node > > >>>>>>> doesn't look like a best solution. > > >>>>>>> On the other hand chosen node is the location where OS specific > > >>>>>>> properties are coming that's why I have suggested to use it. > > >>>>>> > > >>>>>> > > >>>>>> I like Michal's idea. > > >>>>>> We need some work for fdtgrep, but I believe it is worthwhile. > > >>>>>> > > >>>>>> From Michal's recent patches ( > http://patchwork.ozlabs.org/patch/498609/), > > >>>>>> I guess he is tackling on syncing device trees between Linux and > U-boot, > > >>>>>> and this is right thing to do. > > >>>>>> > > >>>>>> Inserting the U-boot specific property here and there > > >>>>>> makes it difficult. > > >>>>> > > >>>>> Yes it is attractive. > > >>>>> > > >>>>> With this scheme we cannot put the properties into .dtsi (i.e. make > > >>>>> them common for the soc). Is there a way around that or would we > just > > >>>>> have to live with it? > > >>>> > > >>>> Why not? You can add chosen node to dtsi without any problem which > > >>>> should be shared for all boards. The only one question remains > which is > > >>>> what exactly you need to add to dtsi. > > >>>> One example is Zynq. We have 2 serial PS IPs. Which one you want to > add? > > >>>> Both? > > >>> > > >>> If you have something like this: > > >>> > > >>> soc { > > >>> uart0 { > > >>> }; > > >>> uart1 { > > >>> }; > > >>> } > > >>> > > >>> in the common .dtsi then you can currently put the marker in the soc > > >>> node. I'm not sure how you do that with your scheme. If we put it in > > >>> the .dtsi then the .dts will override it. > > >>> > > >>> Does this matter? > > >> > > >> As far as I understand DTSI is shared across all boards. > > >> And DTS can overwrite DTSI at any time. > > >> > > >> Let me extend this to make it more clear > > >> soc: soc { > > >> uart0: uart@XXX { > > >> }; > > >> uart1: uart@XXX { > > >> }; > > >> } > > >> > > >> > > >> In DTSI I would put probably this to show everybody what needs to be > there. > > >> chosen { > > >> u-boot,dm-pre-reloc = &soc &uart0 &uart1; > > >> } > > >> > > >> And in DTS if you have only one uart DTS will overwrite it. > > >> chosen { > > >> u-boot,dm-pre-reloc = &soc &uart0; > > >> } > > >> > > >> Or the next option is to make code smarter and detect that uart1 is > > >> disabled that's why it is not used and only chosen from DTSI should be > > >> enough. > > > > > > Yes I see - you just overwrite the property in the main file. If it > > > includes pinctrl nodes, gpio nodes, etc. then you would have to add > > > those also. > > > > Right - DTSI is most like a pattern to follow and DTS is doing that > > platform specific stuff. It means in DTSI you can have guidance what to > > use. > > > Do you mean we should add a comment to the .dtsi to tell you want you > need in your .dts? Is this improving anything? > what I think it will be the best to add that line to DTSI with all IPs which should be pre relocated. Then algorithm should be like this for_each_IP_in_property { if (status=ok) relocate else ignore it } If you have others nodes in DTS then you have to rewrite it. > > > > > Also in algorithm can be checking if that IP has status okay or disabled > > and do relocation or not. > > > Can you please rephrase that? I don't understand. > > > > > > > For mainline Rockchip this means that each board would > > > have to have something like: > > > > > > chosen { > > > u-boot,dm-pre-reloc = &mmc0, &serial0, &dmc, &power, &syscon0 > > > &syscon1 &pinctrl &gpio3 &gpio8 &leds &led_work &led_power; > > > } > > > > > > That seems a bit unwieldy to me. What do you think? > > > > (note: without commas) > > I think that this solution is compatible with the binding and it is > > better than adding bootloader specific property to nodes which I have > > never seen before. Chosen node was used for that maybe even from > beginning. > > > There are Linux-specific properties, so I really don't see the > difference. Don't forget that we are using device tree to control our > boot loader, so it would be unusual to not see at least some > U-Boot-specific properties. > > My understanding of /chosen is that it is for the OS. But in any case > it would still be a binding change, wouldn't it? What makes you think > that this scheme would be more acceptable as a binding change? > I think we should move this discussion to device-tree mailing list. I simple just don't think that OS/bootloader property in the device node is a good idea. Maybe there is better solution but i think adding OS/Bootloader to chosen is just better option then added this to every node. Not sure if sequence matter or not but via one property you can also control it. > > > > > > Regarding technical aspects - I have never checked if there is any line > > length limitation on parameter side which can be problematic. On the > > other hand if yes phandles should be used. > > > No there is not limit I know of. It should be fine. > > > > > > Regarding compactness of this solution. From one line you can see what > > will be pre relocated which looks pretty compact to me and you can easy > > check if something is missing. > > > Yes it is nice to have this in once place. But it's not really in one > place as you need to reference nodes from elsewhere, and update all > .dts files if something in the .dtsi changes. Granted that shouldn't > happen often. > > One advantage is that this copes with boards that have a common .dtsi > but different drivers needed before relocation - e.g. one board might > want to use a serial port attached to a /soc node, another might want > to use something on /pci. But I have not seen this happen yet. > > So overall i can see benefits and drawbacks. I'm on the fence. > TBH there should be any ACK from DT guys before this u-boot property was used. From that discussion should come how this can be used. In past I have some experience with using incorrect binding in private drivers and it ends up in disaster that's why I want to make sure before we start to use this that it was properly review. Thanks, Michal
On Sat, Sep 19, 2015 at 03:07:54AM +0200, Michal Simek wrote: > Hi Simon, > > first of all sorry for late reply. I am traveling and I have pretty busy > time now. > > 2015-09-09 20:07 GMT+02:00 Simon Glass <sjg@chromium.org>: [snip] > > There are Linux-specific properties, so I really don't see the > > difference. Don't forget that we are using device tree to control our > > boot loader, so it would be unusual to not see at least some > > U-Boot-specific properties. > > > > My understanding of /chosen is that it is for the OS. But in any case > > it would still be a binding change, wouldn't it? What makes you think > > that this scheme would be more acceptable as a binding change? > > > > I think we should move this discussion to device-tree mailing list. > I simple just don't think that OS/bootloader property in the device node is > a good idea. > Maybe there is better solution but i think adding OS/Bootloader to chosen > is just better option > then added this to every node. > Not sure if sequence matter or not but via one property you can also > control it. I think that with ELC-E coming up and that once again there will be a DT BoF, we (the U-Boot community) need to show up there and in turn see how many folks we can also get to show up in our mini-summit. I've already bugged Pantelis about this a few times.
Hi Michal, On 18 September 2015 at 19:07, Michal Simek <monstr@monstr.eu> wrote: > Hi Simon, > > first of all sorry for late reply. I am traveling and I have pretty busy > time now. That's OK, I don't think there is any particular hurry. > > 2015-09-09 20:07 GMT+02:00 Simon Glass <sjg@chromium.org>: >> >> Hi Michal, >> >> On Friday, 4 September 2015, Michal Simek <monstr@monstr.eu> wrote: >> > >> > Hi Simon, >> > >> > <cut some previous parts> >> > >> > >>>>>>>> >> > >>>>>>>> We also use this with fdtgrep to remove nodes not needed for >> > >>>>>>>> SPL. So >> > >>>>>>>> we would have to come up with a tool to make that work. At >> > >>>>>>>> present >> > >>>>>>>> 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we >> > >>>>>>>> want (it >> > >>>>>>>> finds nodes with that property). >> > >>>>>>>> >> > >>>>>>>> I'm actually not sure that this approach is any easier/better. >> > >>>>>>>> What >> > >>>>>>>> are the advantages? >> > >>>>>>> >> > >>>>>>> The question is if current solution which you are using is fully >> > >>>>>>> compatible with binding. Adding bootloader property to the HW >> > >>>>>>> node >> > >>>>>>> doesn't look like a best solution. >> > >>>>>>> On the other hand chosen node is the location where OS specific >> > >>>>>>> properties are coming that's why I have suggested to use it. >> > >>>>>> >> > >>>>>> >> > >>>>>> I like Michal's idea. >> > >>>>>> We need some work for fdtgrep, but I believe it is worthwhile. >> > >>>>>> >> > >>>>>> From Michal's recent patches >> > >>>>>> (http://patchwork.ozlabs.org/patch/498609/), >> > >>>>>> I guess he is tackling on syncing device trees between Linux and >> > >>>>>> U-boot, >> > >>>>>> and this is right thing to do. >> > >>>>>> >> > >>>>>> Inserting the U-boot specific property here and there >> > >>>>>> makes it difficult. >> > >>>>> >> > >>>>> Yes it is attractive. >> > >>>>> >> > >>>>> With this scheme we cannot put the properties into .dtsi (i.e. >> > >>>>> make >> > >>>>> them common for the soc). Is there a way around that or would we >> > >>>>> just >> > >>>>> have to live with it? >> > >>>> >> > >>>> Why not? You can add chosen node to dtsi without any problem which >> > >>>> should be shared for all boards. The only one question remains >> > >>>> which is >> > >>>> what exactly you need to add to dtsi. >> > >>>> One example is Zynq. We have 2 serial PS IPs. Which one you want to >> > >>>> add? >> > >>>> Both? >> > >>> >> > >>> If you have something like this: >> > >>> >> > >>> soc { >> > >>> uart0 { >> > >>> }; >> > >>> uart1 { >> > >>> }; >> > >>> } >> > >>> >> > >>> in the common .dtsi then you can currently put the marker in the soc >> > >>> node. I'm not sure how you do that with your scheme. If we put it in >> > >>> the .dtsi then the .dts will override it. >> > >>> >> > >>> Does this matter? >> > >> >> > >> As far as I understand DTSI is shared across all boards. >> > >> And DTS can overwrite DTSI at any time. >> > >> >> > >> Let me extend this to make it more clear >> > >> soc: soc { >> > >> uart0: uart@XXX { >> > >> }; >> > >> uart1: uart@XXX { >> > >> }; >> > >> } >> > >> >> > >> >> > >> In DTSI I would put probably this to show everybody what needs to be >> > >> there. >> > >> chosen { >> > >> u-boot,dm-pre-reloc = &soc &uart0 &uart1; >> > >> } >> > >> >> > >> And in DTS if you have only one uart DTS will overwrite it. >> > >> chosen { >> > >> u-boot,dm-pre-reloc = &soc &uart0; >> > >> } >> > >> >> > >> Or the next option is to make code smarter and detect that uart1 is >> > >> disabled that's why it is not used and only chosen from DTSI should >> > >> be >> > >> enough. >> > > >> > > Yes I see - you just overwrite the property in the main file. If it >> > > includes pinctrl nodes, gpio nodes, etc. then you would have to add >> > > those also. >> > >> > Right - DTSI is most like a pattern to follow and DTS is doing that >> > platform specific stuff. It means in DTSI you can have guidance what to >> > use. >> >> >> Do you mean we should add a comment to the .dtsi to tell you want you >> need in your .dts? Is this improving anything? > > > what I think it will be the best to add that line to DTSI with all IPs which > should be pre relocated. > Then algorithm should be like this > > for_each_IP_in_property { > if (status=ok) > relocate > else > ignore it > } > > If you have others nodes in DTS then you have to rewrite it. Normally the .dts file will specify the UART which means that it will often need rewriting with this scheme. The potential for stuff-ups and confusion is there. With the current scheme at least marking can be kept as a property of the device (where arguably it belongs) rather than having to be recreated for each board. Perhaps we could allow multiple properties in /chosen and scan them all (u-boot,dm-pre-reloc-soc, u-boot,dm-pre-reloc-pinctrl, etc.)? > > >> >> >> > >> > Also in algorithm can be checking if that IP has status okay or disabled >> > and do relocation or not. >> >> >> Can you please rephrase that? I don't understand. >> > >> > >> > > For mainline Rockchip this means that each board would >> > > have to have something like: >> > > >> > > chosen { >> > > u-boot,dm-pre-reloc = &mmc0, &serial0, &dmc, &power, &syscon0 >> > > &syscon1 &pinctrl &gpio3 &gpio8 &leds &led_work &led_power; >> > > } >> > > >> > > That seems a bit unwieldy to me. What do you think? >> > >> > (note: without commas) >> > I think that this solution is compatible with the binding and it is >> > better than adding bootloader specific property to nodes which I have >> > never seen before. Chosen node was used for that maybe even from >> > beginning. >> >> >> There are Linux-specific properties, so I really don't see the >> difference. Don't forget that we are using device tree to control our >> boot loader, so it would be unusual to not see at least some >> U-Boot-specific properties. >> >> My understanding of /chosen is that it is for the OS. But in any case >> it would still be a binding change, wouldn't it? What makes you think >> that this scheme would be more acceptable as a binding change? > > > I think we should move this discussion to device-tree mailing list. > I simple just don't think that OS/bootloader property in the device node is > a good idea. > Maybe there is better solution but i think adding OS/Bootloader to chosen is > just better option > then added this to every node. > Not sure if sequence matter or not but via one property you can also control > it. OK, by all means. But please cc the U-Boot mailing list. I'm not tied to the current solution at all. It does work and something like it is necessary. It allows SOC people to set things up so that it is relatively easy for people to write now .dts files. But I'd like to change it once someone has figured out the implications of the change: - Impact on maintainability of .dts/.dtsi files - Mechanism for implementation of device tree subset feature (currently fdtgrep / SPL) - Code changes required in U-Boot - Maintaining the efficiency (the algorithm above would require many scans of the DT) - A patch :-) I feel in general that there is some conflict between hard-nosed efficiency and perceived beauty in device tree bindings. This doesn't seem to matter much in Linux, but in U-Boot it is more important. I'd like to err on the side of keeping things simple and fast, bringing in new functionality as needed (perhaps behind an option) rather than requiring a lump of 10KB of parsing code just to get get a serial driver running. > > > >> >> > >> > >> > Regarding technical aspects - I have never checked if there is any line >> > length limitation on parameter side which can be problematic. On the >> > other hand if yes phandles should be used. >> >> >> No there is not limit I know of. It should be fine. >> > >> > >> > Regarding compactness of this solution. From one line you can see what >> > will be pre relocated which looks pretty compact to me and you can easy >> > check if something is missing. >> >> >> Yes it is nice to have this in once place. But it's not really in one >> place as you need to reference nodes from elsewhere, and update all >> .dts files if something in the .dtsi changes. Granted that shouldn't >> happen often. >> >> One advantage is that this copes with boards that have a common .dtsi >> but different drivers needed before relocation - e.g. one board might >> want to use a serial port attached to a /soc node, another might want >> to use something on /pci. But I have not seen this happen yet. >> >> So overall i can see benefits and drawbacks. I'm on the fence. > > > TBH there should be any ACK from DT guys before this u-boot property was > used. > From that discussion should come how this can be used. > In past I have some experience with using incorrect binding in private > drivers and it ends up > in disaster that's why I want to make sure before we start to use this that > it was properly review. Well it does follow device tree guidelines - it has a' u-boot,' prefix just like all the properties which have a 'linux,' prefix. So I think it is sane from that point of view. It's just that it's a pain to add it to several nodes, and not obvious that another approach (a list of nodes somewhere) is better. Yes I'd really like the 'DT guys' to get involved in this sort of thing, including contributing patches to U-Boot in this area. It takes a lot of energy to go through these discussions and having something on the inside track who can figure these things out and help build out the infrastructure would be a big help. As Tom suggests we could try to figure this out at ELCE? I can go to the BoF session if there is time to discuss it. So far the topics seem to be quite 'high-end' :-) But we could talk about it at the U-Boot one. Regards, Simon
diff --git a/arch/arm/dts/zynq-7000.dtsi b/arch/arm/dts/zynq-7000.dtsi index 0b62cb0..12614f2 100644 --- a/arch/arm/dts/zynq-7000.dtsi +++ b/arch/arm/dts/zynq-7000.dtsi @@ -54,6 +54,7 @@ }; amba: amba { + u-boot,dm-pre-reloc; compatible = "simple-bus"; #address-cells = <1>; #size-cells = <1>; diff --git a/arch/arm/dts/zynq-microzed.dts b/arch/arm/dts/zynq-microzed.dts index c373a2c..5dff18e6 100644 --- a/arch/arm/dts/zynq-microzed.dts +++ b/arch/arm/dts/zynq-microzed.dts @@ -21,3 +21,8 @@ reg = <0 0x40000000>; }; }; + +&uart1 { + u-boot,dm-pre-reloc; + status = "okay"; +}; diff --git a/arch/arm/dts/zynq-picozed.dts b/arch/arm/dts/zynq-picozed.dts index 686b98f..3408df8 100644 --- a/arch/arm/dts/zynq-picozed.dts +++ b/arch/arm/dts/zynq-picozed.dts @@ -21,3 +21,8 @@ reg = <0 0x40000000>; }; }; + +&uart1 { + u-boot,dm-pre-reloc; + status = "okay"; +}; diff --git a/arch/arm/dts/zynq-zc702.dts b/arch/arm/dts/zynq-zc702.dts index 6691a8d..4fcef4b 100644 --- a/arch/arm/dts/zynq-zc702.dts +++ b/arch/arm/dts/zynq-zc702.dts @@ -375,6 +375,7 @@ }; &uart1 { + u-boot,dm-pre-reloc; status = "okay"; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_uart1_default>; diff --git a/arch/arm/dts/zynq-zc706.dts b/arch/arm/dts/zynq-zc706.dts index cf7bce4..8198917 100644 --- a/arch/arm/dts/zynq-zc706.dts +++ b/arch/arm/dts/zynq-zc706.dts @@ -296,6 +296,7 @@ }; &uart1 { + u-boot,dm-pre-reloc; status = "okay"; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_uart1_default>; diff --git a/arch/arm/dts/zynq-zc770-xm010.dts b/arch/arm/dts/zynq-zc770-xm010.dts index 680f24c..e2473d5 100644 --- a/arch/arm/dts/zynq-zc770-xm010.dts +++ b/arch/arm/dts/zynq-zc770-xm010.dts @@ -83,6 +83,7 @@ }; &uart1 { + u-boot,dm-pre-reloc; status = "okay"; }; diff --git a/arch/arm/dts/zynq-zc770-xm011.dts b/arch/arm/dts/zynq-zc770-xm011.dts index f73c0dd..77e3bb0 100644 --- a/arch/arm/dts/zynq-zc770-xm011.dts +++ b/arch/arm/dts/zynq-zc770-xm011.dts @@ -55,6 +55,7 @@ }; &uart1 { + u-boot,dm-pre-reloc; status = "okay"; }; diff --git a/arch/arm/dts/zynq-zc770-xm012.dts b/arch/arm/dts/zynq-zc770-xm012.dts index 4289e31..3e1769a 100644 --- a/arch/arm/dts/zynq-zc770-xm012.dts +++ b/arch/arm/dts/zynq-zc770-xm012.dts @@ -62,5 +62,6 @@ }; &uart1 { + u-boot,dm-pre-reloc; status = "okay"; }; diff --git a/arch/arm/dts/zynq-zc770-xm013.dts b/arch/arm/dts/zynq-zc770-xm013.dts index 5124cdc..288e248 100644 --- a/arch/arm/dts/zynq-zc770-xm013.dts +++ b/arch/arm/dts/zynq-zc770-xm013.dts @@ -75,5 +75,6 @@ }; &uart0 { + u-boot,dm-pre-reloc; status = "okay"; }; diff --git a/arch/arm/dts/zynq-zed.dts b/arch/arm/dts/zynq-zed.dts index 5762576..81c64a6 100644 --- a/arch/arm/dts/zynq-zed.dts +++ b/arch/arm/dts/zynq-zed.dts @@ -53,6 +53,7 @@ }; &uart1 { + u-boot,dm-pre-reloc; status = "okay"; }; diff --git a/arch/arm/dts/zynq-zybo.dts b/arch/arm/dts/zynq-zybo.dts index 10f7815..dcfc00e 100644 --- a/arch/arm/dts/zynq-zybo.dts +++ b/arch/arm/dts/zynq-zybo.dts @@ -49,5 +49,6 @@ }; &uart1 { + u-boot,dm-pre-reloc; status = "okay"; };
We need to mark some device tree nodes so that they are available before relocation. This enables driver model to find these automatically. In the case of SPL it ensures that these nodes will be retained in SPL. Signed-off-by: Simon Glass <sjg@chromium.org> --- arch/arm/dts/zynq-7000.dtsi | 1 + arch/arm/dts/zynq-microzed.dts | 5 +++++ arch/arm/dts/zynq-picozed.dts | 5 +++++ arch/arm/dts/zynq-zc702.dts | 1 + arch/arm/dts/zynq-zc706.dts | 1 + arch/arm/dts/zynq-zc770-xm010.dts | 1 + arch/arm/dts/zynq-zc770-xm011.dts | 1 + arch/arm/dts/zynq-zc770-xm012.dts | 1 + arch/arm/dts/zynq-zc770-xm013.dts | 1 + arch/arm/dts/zynq-zed.dts | 1 + arch/arm/dts/zynq-zybo.dts | 1 + 11 files changed, 19 insertions(+)