Re: [RESEND PATCH] fdt: Use phandle to distinguish DT nodes with same name
Hi Aswath,
On Fri, 27 Nov 2020 at 07:05, Aswath Govindraju wrote:
>
> On 22/11/20 4:37 am, Simon Glass wrote:
> > Hi,
> >
> > On Wed, 18 Nov 2020 at 10:55, Vignesh Raghavendra wrote:
> >>
> >>
> >>
> >> On 11/18/20 8:44 PM, Aswath Govindraju wrote:
> >>> Hi Simon,
> >>>
> >>> On 18/11/20 8:07 pm, Simon Glass wrote:
> Hi Aswath,
>
> On Mon, 16 Nov 2020 at 07:29, Aswath Govindraju
> wrote:
> >
> > While assigning the sequence number to subsystem instances by reading
> > the
> > aliases property, only DT nodes names are compared and not the complete
> > path. This causes a problem when there are two DT nodes with same name
> > but
> > have different paths.
> >
> > Fix it by comparing the phandles of DT nodes after the node names match.
> >
> > Signed-off-by: Aswath Govindraju
> > ---
> >
> > Resending this patch as it was held awaiting for moderator approval
> > because
> > patch was sent by non-member.
> >
> > lib/fdtdec.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > index 2015907dee7d..9e1bfe0b519e 100644
> > --- a/lib/fdtdec.c
> > +++ b/lib/fdtdec.c
> > @@ -478,6 +478,11 @@ int fdtdec_get_alias_seq(const void *blob, const
> > char *base, int offset,
> > slash = strrchr(prop, '/');
> > if (strcmp(slash + 1, find_name))
> > continue;
> > +
> > + if (fdt_get_phandle(blob, offset) !=
> > + fdt_get_phandle(blob, fdt_path_offset(blob, prop)))
> > + continue;
>
> The call to fdt_path_offset() is very slow. Perhaps we can do this
> check only with livetree? What situation is causing a problem for you?
> What are the node / alias names?
> >>>
> >>> In the case of live tree for getting the sequence number the node
> >>> pointers are compared. So, I don't think this problem would come up.
> >>>
> >>> As for the use case,
> >>>
> >>> In AM654 Device tree there are two instances of USB controllers and both
> >>> the controller nodes have the same name usb@1
> >>>
> >>> If dfu is performed through the port connected to second controller.
> >>> Then based on the dr_mode of first controller the instance number to be
> >>> used in dfu command will vary. In order to make the instance number for
> >>> dfu command to be independent, aliases can be used(If aliases are
> >>> defined then the sequence number is assigned as the alias number.).
> >>>
> >>> The problem with current method for acquiring sequence number using
> >>> aliases is that only the name of the node is compared with node name
> >>> from the aliases property. So in the above case both the controllers
> >>> will have the same name. This leads to the first alias number being used
> >>> for the both the controllers to assign sequence number.
> >>>
> >>>
> >>> aliases {
> >>> serial2 = _uart0;
> >>> ethernet0 = _port1;
> >>> usb0 = // This property being used to
> >>> //alias both the controllers
> >>> usb1 =
> >>> };
> >>
> >>
> >> To explain a bit more, here is the DT snippet around usb0 and usb1
> >>
> >> dwc3_0: dwc3@400 {
> >> compatible = "ti,am654-dwc3";
> >> reg = <0x0 0x400 0x0 0x4000>;
> >> #address-cells = <1>;
> >> #size-cells = <1>;
> >> ranges = <0x0 0x0 0x400 0x2>;
> >> ...
> >>
> >> usb0: usb@1 {
> >> compatible = "snps,dwc3";
> >> reg = <0x1 0x1>;
> >> ...
> >> };
> >> };
> >>
> >> dwc3_1: dwc3@402 {
> >> compatible = "ti,am654-dwc3";
> >> reg = <0x0 0x402 0x0 0x4000>;
> >> #address-cells = <1>;
> >> #size-cells = <1>;
> >> ranges = <0x0 0x0 0x402 0x2>;
> >> ...
> >>
> >> usb1: usb@1 {
> >> compatible = "snps,dwc3";
> >> reg = <0x1 0x1>;
> >> ...
> >> };
> >> };
> >>
> >> In above case, (with CONFIG_OF_LIVE disabled),
> >> fdtdec_get_alias_seq() fails to pick the correct instance for USB
> >> controller for a given index. This is because fdtdec_get_alias_seq()
> >> only compares the leaf node name (usb@1) with alias path and thus
> >> both usb instances match to usb0.
> >>
> >>>
> >>> So, to distinguish nodes with same name, phandles can be used while
> >>> assigning sequence numbers.
> >
>
> I apologize for the delay in response.
No hurry!
>
>
> > Thank you both for the detai. I understand it and
Re: [RESEND PATCH] fdt: Use phandle to distinguish DT nodes with same name
On 22/11/20 4:37 am, Simon Glass wrote:
> Hi,
>
> On Wed, 18 Nov 2020 at 10:55, Vignesh Raghavendra wrote:
>>
>>
>>
>> On 11/18/20 8:44 PM, Aswath Govindraju wrote:
>>> Hi Simon,
>>>
>>> On 18/11/20 8:07 pm, Simon Glass wrote:
Hi Aswath,
On Mon, 16 Nov 2020 at 07:29, Aswath Govindraju
wrote:
>
> While assigning the sequence number to subsystem instances by reading the
> aliases property, only DT nodes names are compared and not the complete
> path. This causes a problem when there are two DT nodes with same name but
> have different paths.
>
> Fix it by comparing the phandles of DT nodes after the node names match.
>
> Signed-off-by: Aswath Govindraju
> ---
>
> Resending this patch as it was held awaiting for moderator approval
> because
> patch was sent by non-member.
>
> lib/fdtdec.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 2015907dee7d..9e1bfe0b519e 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -478,6 +478,11 @@ int fdtdec_get_alias_seq(const void *blob, const
> char *base, int offset,
> slash = strrchr(prop, '/');
> if (strcmp(slash + 1, find_name))
> continue;
> +
> + if (fdt_get_phandle(blob, offset) !=
> + fdt_get_phandle(blob, fdt_path_offset(blob, prop)))
> + continue;
The call to fdt_path_offset() is very slow. Perhaps we can do this
check only with livetree? What situation is causing a problem for you?
What are the node / alias names?
>>>
>>> In the case of live tree for getting the sequence number the node
>>> pointers are compared. So, I don't think this problem would come up.
>>>
>>> As for the use case,
>>>
>>> In AM654 Device tree there are two instances of USB controllers and both
>>> the controller nodes have the same name usb@1
>>>
>>> If dfu is performed through the port connected to second controller.
>>> Then based on the dr_mode of first controller the instance number to be
>>> used in dfu command will vary. In order to make the instance number for
>>> dfu command to be independent, aliases can be used(If aliases are
>>> defined then the sequence number is assigned as the alias number.).
>>>
>>> The problem with current method for acquiring sequence number using
>>> aliases is that only the name of the node is compared with node name
>>> from the aliases property. So in the above case both the controllers
>>> will have the same name. This leads to the first alias number being used
>>> for the both the controllers to assign sequence number.
>>>
>>>
>>> aliases {
>>> serial2 = _uart0;
>>> ethernet0 = _port1;
>>> usb0 = // This property being used to
>>> //alias both the controllers
>>> usb1 =
>>> };
>>
>>
>> To explain a bit more, here is the DT snippet around usb0 and usb1
>>
>> dwc3_0: dwc3@400 {
>> compatible = "ti,am654-dwc3";
>> reg = <0x0 0x400 0x0 0x4000>;
>> #address-cells = <1>;
>> #size-cells = <1>;
>> ranges = <0x0 0x0 0x400 0x2>;
>> ...
>>
>> usb0: usb@1 {
>> compatible = "snps,dwc3";
>> reg = <0x1 0x1>;
>> ...
>> };
>> };
>>
>> dwc3_1: dwc3@402 {
>> compatible = "ti,am654-dwc3";
>> reg = <0x0 0x402 0x0 0x4000>;
>> #address-cells = <1>;
>> #size-cells = <1>;
>> ranges = <0x0 0x0 0x402 0x2>;
>> ...
>>
>> usb1: usb@1 {
>> compatible = "snps,dwc3";
>> reg = <0x1 0x1>;
>> ...
>> };
>> };
>>
>> In above case, (with CONFIG_OF_LIVE disabled),
>> fdtdec_get_alias_seq() fails to pick the correct instance for USB
>> controller for a given index. This is because fdtdec_get_alias_seq()
>> only compares the leaf node name (usb@1) with alias path and thus
>> both usb instances match to usb0.
>>
>>>
>>> So, to distinguish nodes with same name, phandles can be used while
>>> assigning sequence numbers.
>
I apologize for the delay in response.
> Thank you both for the detai. I understand it and in fact I think this
> has come up before.
>
> Would it be OK to use livetree?
>
Currently live tree has not been enabled in the configurations of the
AM65 board and there are some issues that I am facing after enabling it.
> If not, can we put this code behind a Kconfig so the extra time
> penalty is only incurred on boards that need it?
>
I
Re: [RESEND PATCH] fdt: Use phandle to distinguish DT nodes with same name
Hi,
On Wed, 18 Nov 2020 at 10:55, Vignesh Raghavendra wrote:
>
>
>
> On 11/18/20 8:44 PM, Aswath Govindraju wrote:
> > Hi Simon,
> >
> > On 18/11/20 8:07 pm, Simon Glass wrote:
> >> Hi Aswath,
> >>
> >> On Mon, 16 Nov 2020 at 07:29, Aswath Govindraju
> >> wrote:
> >>>
> >>> While assigning the sequence number to subsystem instances by reading the
> >>> aliases property, only DT nodes names are compared and not the complete
> >>> path. This causes a problem when there are two DT nodes with same name but
> >>> have different paths.
> >>>
> >>> Fix it by comparing the phandles of DT nodes after the node names match.
> >>>
> >>> Signed-off-by: Aswath Govindraju
> >>> ---
> >>>
> >>> Resending this patch as it was held awaiting for moderator approval
> >>> because
> >>> patch was sent by non-member.
> >>>
> >>> lib/fdtdec.c | 5 +
> >>> 1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> >>> index 2015907dee7d..9e1bfe0b519e 100644
> >>> --- a/lib/fdtdec.c
> >>> +++ b/lib/fdtdec.c
> >>> @@ -478,6 +478,11 @@ int fdtdec_get_alias_seq(const void *blob, const
> >>> char *base, int offset,
> >>> slash = strrchr(prop, '/');
> >>> if (strcmp(slash + 1, find_name))
> >>> continue;
> >>> +
> >>> + if (fdt_get_phandle(blob, offset) !=
> >>> + fdt_get_phandle(blob, fdt_path_offset(blob, prop)))
> >>> + continue;
> >>
> >> The call to fdt_path_offset() is very slow. Perhaps we can do this
> >> check only with livetree? What situation is causing a problem for you?
> >> What are the node / alias names?
> >
> > In the case of live tree for getting the sequence number the node
> > pointers are compared. So, I don't think this problem would come up.
> >
> > As for the use case,
> >
> > In AM654 Device tree there are two instances of USB controllers and both
> > the controller nodes have the same name usb@1
> >
> > If dfu is performed through the port connected to second controller.
> > Then based on the dr_mode of first controller the instance number to be
> > used in dfu command will vary. In order to make the instance number for
> > dfu command to be independent, aliases can be used(If aliases are
> > defined then the sequence number is assigned as the alias number.).
> >
> > The problem with current method for acquiring sequence number using
> > aliases is that only the name of the node is compared with node name
> > from the aliases property. So in the above case both the controllers
> > will have the same name. This leads to the first alias number being used
> > for the both the controllers to assign sequence number.
> >
> >
> > aliases {
> > serial2 = _uart0;
> > ethernet0 = _port1;
> > usb0 = // This property being used to
> > //alias both the controllers
> > usb1 =
> > };
>
>
> To explain a bit more, here is the DT snippet around usb0 and usb1
>
> dwc3_0: dwc3@400 {
> compatible = "ti,am654-dwc3";
> reg = <0x0 0x400 0x0 0x4000>;
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0x0 0x0 0x400 0x2>;
> ...
>
> usb0: usb@1 {
> compatible = "snps,dwc3";
> reg = <0x1 0x1>;
> ...
> };
> };
>
> dwc3_1: dwc3@402 {
> compatible = "ti,am654-dwc3";
> reg = <0x0 0x402 0x0 0x4000>;
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0x0 0x0 0x402 0x2>;
> ...
>
> usb1: usb@1 {
> compatible = "snps,dwc3";
> reg = <0x1 0x1>;
> ...
> };
> };
>
> In above case, (with CONFIG_OF_LIVE disabled),
> fdtdec_get_alias_seq() fails to pick the correct instance for USB
> controller for a given index. This is because fdtdec_get_alias_seq()
> only compares the leaf node name (usb@1) with alias path and thus
> both usb instances match to usb0.
>
> >
> > So, to distinguish nodes with same name, phandles can be used while
> > assigning sequence numbers.
Thank you both for the detai. I understand it and in fact I think this
has come up before.
Would it be OK to use livetree?
If not, can we put this code behind a Kconfig so the extra time
penalty is only incurred on boards that need it?
Regards,
Simon
Re:[RESEND PATCH] fdt: Use phandle to distinguish DT nodes with same name
On 11/18/20 8:44 PM, Aswath Govindraju wrote:
> Hi Simon,
>
> On 18/11/20 8:07 pm, Simon Glass wrote:
>> Hi Aswath,
>>
>> On Mon, 16 Nov 2020 at 07:29, Aswath Govindraju wrote:
>>>
>>> While assigning the sequence number to subsystem instances by reading the
>>> aliases property, only DT nodes names are compared and not the complete
>>> path. This causes a problem when there are two DT nodes with same name but
>>> have different paths.
>>>
>>> Fix it by comparing the phandles of DT nodes after the node names match.
>>>
>>> Signed-off-by: Aswath Govindraju
>>> ---
>>>
>>> Resending this patch as it was held awaiting for moderator approval because
>>> patch was sent by non-member.
>>>
>>> lib/fdtdec.c | 5 +
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>>> index 2015907dee7d..9e1bfe0b519e 100644
>>> --- a/lib/fdtdec.c
>>> +++ b/lib/fdtdec.c
>>> @@ -478,6 +478,11 @@ int fdtdec_get_alias_seq(const void *blob, const char
>>> *base, int offset,
>>> slash = strrchr(prop, '/');
>>> if (strcmp(slash + 1, find_name))
>>> continue;
>>> +
>>> + if (fdt_get_phandle(blob, offset) !=
>>> + fdt_get_phandle(blob, fdt_path_offset(blob, prop)))
>>> + continue;
>>
>> The call to fdt_path_offset() is very slow. Perhaps we can do this
>> check only with livetree? What situation is causing a problem for you?
>> What are the node / alias names?
>
> In the case of live tree for getting the sequence number the node
> pointers are compared. So, I don't think this problem would come up.
>
> As for the use case,
>
> In AM654 Device tree there are two instances of USB controllers and both
> the controller nodes have the same name usb@1
>
> If dfu is performed through the port connected to second controller.
> Then based on the dr_mode of first controller the instance number to be
> used in dfu command will vary. In order to make the instance number for
> dfu command to be independent, aliases can be used(If aliases are
> defined then the sequence number is assigned as the alias number.).
>
> The problem with current method for acquiring sequence number using
> aliases is that only the name of the node is compared with node name
> from the aliases property. So in the above case both the controllers
> will have the same name. This leads to the first alias number being used
> for the both the controllers to assign sequence number.
>
>
> aliases {
> serial2 = _uart0;
> ethernet0 = _port1;
> usb0 = // This property being used to
> //alias both the controllers
> usb1 =
> };
To explain a bit more, here is the DT snippet around usb0 and usb1
dwc3_0: dwc3@400 {
compatible = "ti,am654-dwc3";
reg = <0x0 0x400 0x0 0x4000>;
#address-cells = <1>;
#size-cells = <1>;
ranges = <0x0 0x0 0x400 0x2>;
...
usb0: usb@1 {
compatible = "snps,dwc3";
reg = <0x1 0x1>;
...
};
};
dwc3_1: dwc3@402 {
compatible = "ti,am654-dwc3";
reg = <0x0 0x402 0x0 0x4000>;
#address-cells = <1>;
#size-cells = <1>;
ranges = <0x0 0x0 0x402 0x2>;
...
usb1: usb@1 {
compatible = "snps,dwc3";
reg = <0x1 0x1>;
...
};
};
In above case, (with CONFIG_OF_LIVE disabled),
fdtdec_get_alias_seq() fails to pick the correct instance for USB
controller for a given index. This is because fdtdec_get_alias_seq()
only compares the leaf node name (usb@1) with alias path and thus
both usb instances match to usb0.
>
> So, to distinguish nodes with same name, phandles can be used while
> assigning sequence numbers.
>
> Thanks,
> Aswath
>
>>
>>> +
>>> val = trailing_strtol(name);
>>> if (val != -1) {
>>> *seqp = val;
>>> --
>>> 2.17.1
>>>
>>
>> Regards,
>> Simon
>>
>
Re: [EXTERNAL] Re: [RESEND PATCH] fdt: Use phandle to distinguish DT nodes with same name
Hi Simon,
On 18/11/20 8:07 pm, Simon Glass wrote:
> Hi Aswath,
>
> On Mon, 16 Nov 2020 at 07:29, Aswath Govindraju wrote:
>>
>> While assigning the sequence number to subsystem instances by reading the
>> aliases property, only DT nodes names are compared and not the complete
>> path. This causes a problem when there are two DT nodes with same name but
>> have different paths.
>>
>> Fix it by comparing the phandles of DT nodes after the node names match.
>>
>> Signed-off-by: Aswath Govindraju
>> ---
>>
>> Resending this patch as it was held awaiting for moderator approval because
>> patch was sent by non-member.
>>
>> lib/fdtdec.c | 5 +
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>> index 2015907dee7d..9e1bfe0b519e 100644
>> --- a/lib/fdtdec.c
>> +++ b/lib/fdtdec.c
>> @@ -478,6 +478,11 @@ int fdtdec_get_alias_seq(const void *blob, const char
>> *base, int offset,
>> slash = strrchr(prop, '/');
>> if (strcmp(slash + 1, find_name))
>> continue;
>> +
>> + if (fdt_get_phandle(blob, offset) !=
>> + fdt_get_phandle(blob, fdt_path_offset(blob, prop)))
>> + continue;
>
> The call to fdt_path_offset() is very slow. Perhaps we can do this
> check only with livetree? What situation is causing a problem for you?
> What are the node / alias names?
In the case of live tree for getting the sequence number the node
pointers are compared. So, I don't think this problem would come up.
As for the use case,
In AM654 Device tree there are two instances of USB controllers and both
the controller nodes have the same name usb@1
If dfu is performed through the port connected to second controller.
Then based on the dr_mode of first controller the instance number to be
used in dfu command will vary. In order to make the instance number for
dfu command to be independent, aliases can be used(If aliases are
defined then the sequence number is assigned as the alias number.).
The problem with current method for acquiring sequence number using
aliases is that only the name of the node is compared with node name
from the aliases property. So in the above case both the controllers
will have the same name. This leads to the first alias number being used
for the both the controllers to assign sequence number.
aliases {
serial2 = _uart0;
ethernet0 = _port1;
usb0 = // This property being used to
//alias both the controllers
usb1 =
};
So, to distinguish nodes with same name, phandles can be used while
assigning sequence numbers.
Thanks,
Aswath
>
>> +
>> val = trailing_strtol(name);
>> if (val != -1) {
>> *seqp = val;
>> --
>> 2.17.1
>>
>
> Regards,
> Simon
>
Re: [RESEND PATCH] fdt: Use phandle to distinguish DT nodes with same name
Hi Aswath,
On Mon, 16 Nov 2020 at 07:29, Aswath Govindraju wrote:
>
> While assigning the sequence number to subsystem instances by reading the
> aliases property, only DT nodes names are compared and not the complete
> path. This causes a problem when there are two DT nodes with same name but
> have different paths.
>
> Fix it by comparing the phandles of DT nodes after the node names match.
>
> Signed-off-by: Aswath Govindraju
> ---
>
> Resending this patch as it was held awaiting for moderator approval because
> patch was sent by non-member.
>
> lib/fdtdec.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 2015907dee7d..9e1bfe0b519e 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -478,6 +478,11 @@ int fdtdec_get_alias_seq(const void *blob, const char
> *base, int offset,
> slash = strrchr(prop, '/');
> if (strcmp(slash + 1, find_name))
> continue;
> +
> + if (fdt_get_phandle(blob, offset) !=
> + fdt_get_phandle(blob, fdt_path_offset(blob, prop)))
> + continue;
The call to fdt_path_offset() is very slow. Perhaps we can do this
check only with livetree? What situation is causing a problem for you?
What are the node / alias names?
> +
> val = trailing_strtol(name);
> if (val != -1) {
> *seqp = val;
> --
> 2.17.1
>
Regards,
Simon
Re: [Patch] fdt: Use phandle to distinguish DT nodes with same name
Hi Aswath,
On Wed, 11 Nov 2020 at 07:26, Aswath Govindraju wrote:
>
> While assigning the sequence number to subsystem instances by reading the
> aliases property, only DT nodes names are compared and not the complete
> path. This causes a problem when there are two DT nodes with same name but
> have different paths.
>
> Fix it by comparing the phandles of DT nodes after the node names match.
>
> Signed-off-by: Aswath Govindraju
> ---
> lib/fdtdec.c | 5 +
> 1 file changed, 5 insertions(+)
fdt_path_offset() is slow. Could we just add this for the livetree
case perhaps, thus avoiding the speed penalty?
Can you also add a case where this causes a problem?
>
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 2015907dee7d..9e1bfe0b519e 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -478,6 +478,11 @@ int fdtdec_get_alias_seq(const void *blob, const char
> *base, int offset,
> slash = strrchr(prop, '/');
> if (strcmp(slash + 1, find_name))
> continue;
> +
> + if (fdt_get_phandle(blob, offset) !=
> + fdt_get_phandle(blob, fdt_path_offset(blob, prop)))
> + continue;
> +
> val = trailing_strtol(name);
> if (val != -1) {
> *seqp = val;
> --
> 2.17.1
>
Regards,
Simon
[Patch] fdt: Use phandle to distinguish DT nodes with same name
While assigning the sequence number to subsystem instances by reading the
aliases property, only DT nodes names are compared and not the complete
path. This causes a problem when there are two DT nodes with same name but
have different paths.
Fix it by comparing the phandles of DT nodes after the node names match.
Signed-off-by: Aswath Govindraju
---
lib/fdtdec.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 2015907dee7d..9e1bfe0b519e 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -478,6 +478,11 @@ int fdtdec_get_alias_seq(const void *blob, const char
*base, int offset,
slash = strrchr(prop, '/');
if (strcmp(slash + 1, find_name))
continue;
+
+ if (fdt_get_phandle(blob, offset) !=
+ fdt_get_phandle(blob, fdt_path_offset(blob, prop)))
+ continue;
+
val = trailing_strtol(name);
if (val != -1) {
*seqp = val;
--
2.17.1
