| Message ID | 272e23e0123f02c559bfa4ada9de73eb197aced8.1606917949.git.michal.simek@xilinx.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: zynqmp: DT updates to match latest drivers | expand |
Hi Michal, Thank you for the patch. On Wed, Dec 02, 2020 at 03:06:05PM +0100, Michal Simek wrote: > Add label which is used by bootloader for adding bootloader specific flag. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > U-Boot needs to add u-boot,dm-pre-reloc; property I'm not entirely sure what best practice rules are in this area, but shouldn't U-Boot locate the node by name instead of label ? > --- > arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi > index 4fa820f78d76..8e9b54b5e70c 100644 > --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi > +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi > @@ -99,7 +99,7 @@ opp03 { > }; > }; > > - zynqmp_ipi { > + zynqmp_ipi: zynqmp_ipi { > compatible = "xlnx,zynqmp-ipi-mailbox"; > interrupt-parent = <&gic>; > interrupts = <0 35 4>; -- Regards, Laurent Pinchart
On Mon, Dec 07, 2020 at 12:46:56AM +0200, Laurent Pinchart wrote: > Hi Michal, > > Thank you for the patch. > > On Wed, Dec 02, 2020 at 03:06:05PM +0100, Michal Simek wrote: > > Add label which is used by bootloader for adding bootloader specific flag. > > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > --- > > > > U-Boot needs to add u-boot,dm-pre-reloc; property > > I'm not entirely sure what best practice rules are in this area, but > shouldn't U-Boot locate the node by name instead of label ? And regardless of what mechanism is used, it should be documented in the bindings. > > --- > > arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi > > index 4fa820f78d76..8e9b54b5e70c 100644 > > --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi > > +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi > > @@ -99,7 +99,7 @@ opp03 { > > }; > > }; > > > > - zynqmp_ipi { > > + zynqmp_ipi: zynqmp_ipi { > > compatible = "xlnx,zynqmp-ipi-mailbox"; > > interrupt-parent = <&gic>; > > interrupts = <0 35 4>; -- Regards, Laurent Pinchart
Hi, On 06. 12. 20 23:46, Laurent Pinchart wrote: > Hi Michal, > > Thank you for the patch. > > On Wed, Dec 02, 2020 at 03:06:05PM +0100, Michal Simek wrote: >> Add label which is used by bootloader for adding bootloader specific flag. >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> >> U-Boot needs to add u-boot,dm-pre-reloc; property > > I'm not entirely sure what best practice rules are in this area, but > shouldn't U-Boot locate the node by name instead of label ? Labels are not listed in dt binding and there are two approaches how to reference nodes. Via full path with node name or via labels. I do normally use labels which are much simple. And also if you take a look how dtb looks like (convert back to dts) you can see that for example aliases are using full path (just &label) but clocks/gic which is the part of <> is handled via phandles as numbers. And labels names can vary and shouldn't be the part of binding doc as far as I know. But I can be wrong of course. Thanks, Michal
On 06. 12. 20 23:48, Laurent Pinchart wrote: > On Mon, Dec 07, 2020 at 12:46:56AM +0200, Laurent Pinchart wrote: >> Hi Michal, >> >> Thank you for the patch. >> >> On Wed, Dec 02, 2020 at 03:06:05PM +0100, Michal Simek wrote: >>> Add label which is used by bootloader for adding bootloader specific flag. >>> >>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >>> --- >>> >>> U-Boot needs to add u-boot,dm-pre-reloc; property >> >> I'm not entirely sure what best practice rules are in this area, but >> shouldn't U-Boot locate the node by name instead of label ? > > And regardless of what mechanism is used, it should be documented in the > bindings. I don't think we should be documenting labels because names can be whatever. DT binding spec is just talking about name rules. 6.2 chapter. A label shall be between - 1 to 31 characters in length, be composed only of the characters in the set Table 6.1, and must not start with a number. - Table 6.1: Valid characters for DTS labels Character Description 0-9 digit a-z lowercase letter A-Z uppercase letter _ underscore Thanks, Michal
Hi Michal, On Mon, Dec 07, 2020 at 10:39:25AM +0100, Michal Simek wrote: > On 06. 12. 20 23:46, Laurent Pinchart wrote: > > On Wed, Dec 02, 2020 at 03:06:05PM +0100, Michal Simek wrote: > >> Add label which is used by bootloader for adding bootloader specific flag. > >> > >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> > >> --- > >> > >> U-Boot needs to add u-boot,dm-pre-reloc; property > > > > I'm not entirely sure what best practice rules are in this area, but > > shouldn't U-Boot locate the node by name instead of label ? > > Labels are not listed in dt binding and there are two approaches how to > reference nodes. Via full path with node name or via labels. > I do normally use labels which are much simple. Note that labels require the DTB to be compiled with the -@ option, otherwise they're not present in the binary. > And also if you take a look how dtb looks like (convert back to dts) you > can see that for example aliases are using full path (just &label) but > clocks/gic which is the part of <> is handled via phandles as numbers. > > And labels names can vary and shouldn't be the part of binding doc as > far as I know. But I can be wrong of course. The DT bindings should document the interface with the operating system, and if applicable, the boot loader. If the boot loader requires a particular label, then it becomes part of the ABI, and I think it should be documented in the bindings. -- Regards, Laurent Pinchart
Hi Laurent, On 07. 12. 20 23:16, Laurent Pinchart wrote: > Hi Michal, > > On Mon, Dec 07, 2020 at 10:39:25AM +0100, Michal Simek wrote: >> On 06. 12. 20 23:46, Laurent Pinchart wrote: >>> On Wed, Dec 02, 2020 at 03:06:05PM +0100, Michal Simek wrote: >>>> Add label which is used by bootloader for adding bootloader specific flag. >>>> >>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >>>> --- >>>> >>>> U-Boot needs to add u-boot,dm-pre-reloc; property >>> >>> I'm not entirely sure what best practice rules are in this area, but >>> shouldn't U-Boot locate the node by name instead of label ? >> >> Labels are not listed in dt binding and there are two approaches how to >> reference nodes. Via full path with node name or via labels. >> I do normally use labels which are much simple. > > Note that labels require the DTB to be compiled with the -@ option, > otherwise they're not present in the binary. U-Boot is using different concept. You can see that there are a lot of -u-boot.dtsi files in dts folders. These are automatically included to DTS before DTC is called. It means you don't need to build overlay to get merged. > >> And also if you take a look how dtb looks like (convert back to dts) you >> can see that for example aliases are using full path (just &label) but >> clocks/gic which is the part of <> is handled via phandles as numbers. >> >> And labels names can vary and shouldn't be the part of binding doc as >> far as I know. But I can be wrong of course. > > The DT bindings should document the interface with the operating system, > and if applicable, the boot loader. If the boot loader requires a > particular label, then it becomes part of the ABI, and I think it should > be documented in the bindings. We have been discussing with Rob some month ago but didn't have a time to do step further. Just keep it short Rob was ok to keep bootloader binding inside Linux repo. There is no hardcoding for a particular name. There is just a need to have any label. U-Boot needs to have one property(e.g. u-boot,dm-pre-reloc;) just to do early allocation. The name is just reference and none is really looking for it. It is just a way how to include it in much easier way. Thanks, Michal
Hi Michal, I've just realized I forgot to reply to this e-mail, sorry. On Tue, Dec 08, 2020 at 08:26:41AM +0100, Michal Simek wrote: > On 07. 12. 20 23:16, Laurent Pinchart wrote: > > On Mon, Dec 07, 2020 at 10:39:25AM +0100, Michal Simek wrote: > >> On 06. 12. 20 23:46, Laurent Pinchart wrote: > >>> On Wed, Dec 02, 2020 at 03:06:05PM +0100, Michal Simek wrote: > >>>> Add label which is used by bootloader for adding bootloader specific flag. > >>>> > >>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> > >>>> --- > >>>> > >>>> U-Boot needs to add u-boot,dm-pre-reloc; property > >>> > >>> I'm not entirely sure what best practice rules are in this area, but > >>> shouldn't U-Boot locate the node by name instead of label ? > >> > >> Labels are not listed in dt binding and there are two approaches how to > >> reference nodes. Via full path with node name or via labels. > >> I do normally use labels which are much simple. > > > > Note that labels require the DTB to be compiled with the -@ option, > > otherwise they're not present in the binary. > > U-Boot is using different concept. You can see that there are a lot of > -u-boot.dtsi files in dts folders. These are automatically included to > DTS before DTC is called. It means you don't need to build overlay to > get merged. > > >> And also if you take a look how dtb looks like (convert back to dts) you > >> can see that for example aliases are using full path (just &label) but > >> clocks/gic which is the part of <> is handled via phandles as numbers. > >> > >> And labels names can vary and shouldn't be the part of binding doc as > >> far as I know. But I can be wrong of course. > > > > The DT bindings should document the interface with the operating system, > > and if applicable, the boot loader. If the boot loader requires a > > particular label, then it becomes part of the ABI, and I think it should > > be documented in the bindings. > > We have been discussing with Rob some month ago but didn't have a time > to do step further. Just keep it short Rob was ok to keep bootloader > binding inside Linux repo. I think that makes sense, DT bindings are meant to be OS-agnostic, so boot loader requirements should be documented there. > There is no hardcoding for a particular name. There is just a need to > have any label. U-Boot needs to have one property(e.g. > u-boot,dm-pre-reloc;) just to do early allocation. > The name is just reference and none is really looking for it. It is just > a way how to include it in much easier way. Just to make sure I understand this issue correctly, does this mean that you need to reference the node in a *-u-boot.dtsi file, and want a label to do so ? The label name needs to be the same in the base file (taken from the Linux source tree) and the *-u-boot.dtsi file (in the U-Boot source tree) in that case. Isn't it the role of DT bindings to document such requirements ? -- Regards, Laurent Pinchart
Hi, On 1/21/21 11:29 PM, Laurent Pinchart wrote: > Hi Michal, > > I've just realized I forgot to reply to this e-mail, sorry. > > On Tue, Dec 08, 2020 at 08:26:41AM +0100, Michal Simek wrote: >> On 07. 12. 20 23:16, Laurent Pinchart wrote: >>> On Mon, Dec 07, 2020 at 10:39:25AM +0100, Michal Simek wrote: >>>> On 06. 12. 20 23:46, Laurent Pinchart wrote: >>>>> On Wed, Dec 02, 2020 at 03:06:05PM +0100, Michal Simek wrote: >>>>>> Add label which is used by bootloader for adding bootloader specific flag. >>>>>> >>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >>>>>> --- >>>>>> >>>>>> U-Boot needs to add u-boot,dm-pre-reloc; property >>>>> >>>>> I'm not entirely sure what best practice rules are in this area, but >>>>> shouldn't U-Boot locate the node by name instead of label ? >>>> >>>> Labels are not listed in dt binding and there are two approaches how to >>>> reference nodes. Via full path with node name or via labels. >>>> I do normally use labels which are much simple. >>> >>> Note that labels require the DTB to be compiled with the -@ option, >>> otherwise they're not present in the binary. >> >> U-Boot is using different concept. You can see that there are a lot of >> -u-boot.dtsi files in dts folders. These are automatically included to >> DTS before DTC is called. It means you don't need to build overlay to >> get merged. >> >>>> And also if you take a look how dtb looks like (convert back to dts) you >>>> can see that for example aliases are using full path (just &label) but >>>> clocks/gic which is the part of <> is handled via phandles as numbers. >>>> >>>> And labels names can vary and shouldn't be the part of binding doc as >>>> far as I know. But I can be wrong of course. >>> >>> The DT bindings should document the interface with the operating system, >>> and if applicable, the boot loader. If the boot loader requires a >>> particular label, then it becomes part of the ABI, and I think it should >>> be documented in the bindings. >> >> We have been discussing with Rob some month ago but didn't have a time >> to do step further. Just keep it short Rob was ok to keep bootloader >> binding inside Linux repo. > > I think that makes sense, DT bindings are meant to be OS-agnostic, so > boot loader requirements should be documented there. > >> There is no hardcoding for a particular name. There is just a need to >> have any label. U-Boot needs to have one property(e.g. >> u-boot,dm-pre-reloc;) just to do early allocation. >> The name is just reference and none is really looking for it. It is just >> a way how to include it in much easier way. > > Just to make sure I understand this issue correctly, does this mean that > you need to reference the node in a *-u-boot.dtsi file, and want a label > to do so ? The label name needs to be the same in the base file (taken > from the Linux source tree) and the *-u-boot.dtsi file (in the U-Boot > source tree) in that case. Isn't it the role of DT bindings to document > such requirements ? I prefer to have all nodes with labels just in case you need to reference it. Simply based on experience it happens time to time that something needs to be aligned, new property added and sometimes we touch dt overlays. It means I prefer to have labels for all nodes. In connection to u-boot. U-Boot introduced *-u-boot.dtsi files for bootloader specific configurations. I think that's sort of bad design and it should be done differently by simply document/align this binding with the kernel. And in this u-boot,dm-pre-reloc case it should be handle differently and any transition is required. Normally *-u-boot.dtsi should just reference node in "root" dts via labels. And u-boot is trying to align dts with the kernel. And last part on this if make sense to also document labels as requirement. I don't think it is good idea. You can have in general multiple instances of the same IP which you need to add some properties to. You would have to defined all of them for all existing SOCs which will be very painful. It also takes so much time to upstream it that's why checking one more thing there is IMHO just additional step. I think that everybody should be trying to align all things together across all projects. That's for example what I want to do in this gtr/dp between Linux/U-Boot. Thanks, Michal
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi index 4fa820f78d76..8e9b54b5e70c 100644 --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi @@ -99,7 +99,7 @@ opp03 { }; }; - zynqmp_ipi { + zynqmp_ipi: zynqmp_ipi { compatible = "xlnx,zynqmp-ipi-mailbox"; interrupt-parent = <&gic>; interrupts = <0 35 4>;
Add label which is used by bootloader for adding bootloader specific flag. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- U-Boot needs to add u-boot,dm-pre-reloc; property --- arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)