On 22/11/20 4:37 am, Simon Glass wrote:
> Hi,
>
> On Wed, 18 Nov 2020 at 10:55, Vignesh Raghavendra <vigne...@ti.com> 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 <a-govindr...@ti.com>
>>>> 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 <a-govindr...@ti.com>
>>>>> ---
>>>>>
>>>>> 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@10000
>>>
>>> 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 = &main_uart0;
>>> ethernet0 = &cpsw_port1;
>>> usb0 = &usb0; // This property being used to
>>> //alias both the controllers
>>> usb1 = &usb1;
>>> };
>>
>>
>> To explain a bit more, here is the DT snippet around usb0 and usb1
>>
>> dwc3_0: dwc3@4000000 {
>> compatible = "ti,am654-dwc3";
>> reg = <0x0 0x4000000 0x0 0x4000>;
>> #address-cells = <1>;
>> #size-cells = <1>;
>> ranges = <0x0 0x0 0x4000000 0x20000>;
>> ...
>>
>> usb0: usb@10000 {
>> compatible = "snps,dwc3";
>> reg = <0x10000 0x10000>;
>> ...
>> };
>> };
>>
>> dwc3_1: dwc3@4020000 {
>> compatible = "ti,am654-dwc3";
>> reg = <0x0 0x4020000 0x0 0x4000>;
>> #address-cells = <1>;
>> #size-cells = <1>;
>> ranges = <0x0 0x0 0x4020000 0x20000>;
>> ...
>>
>> usb1: usb@10000 {
>> compatible = "snps,dwc3";
>> reg = <0x10000 0x10000>;
>> ...
>> };
>> };
>>
>> 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@10000) 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 think putting it under Kconfig is a good idea and I think I will do that.
Also, I have alternate method to implement the comparison that does not
use fdt_path_offset(), compares by getting the path name. I have
attached it to this mail. I think it is slightly better in terms of time
penalty. Can you please look at it and suggest if it is better to implement.
Thanks,
Aswath
> Regards,
> Simon
>
>From 9effa3d192aca526d3c37a76ded011bdd21a92ca Mon Sep 17 00:00:00 2001
From: Aswath Govindraju <a-govindr...@ti.com>
Date: Tue, 24 Nov 2020 01:00:12 +0530
Subject: [PATCH] Check the complete path after node names match
Signed-off-by: Aswath Govindraju <a-govindr...@ti.com>
---
lib/fdtdec.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index ee1bd41b0818..529e9aff6f92 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -479,6 +479,10 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
int prop_offset;
int aliases;
+ int parent_offset;
+ const char *parent_name;
+ int parent_namelen;
+
find_name = fdt_get_name(blob, offset, &find_namelen);
debug("Looking for '%s' at %d, name %s\n", base, offset, find_name);
@@ -491,6 +495,9 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
const char *slash;
int len, val;
+ bool flag = false;
+ const char *p;
+
prop = fdt_getprop_by_offset(blob, prop_offset, &name, &len);
debug(" - %s, %s\n", name, prop);
if (len < find_namelen || *prop != '/' || prop[len - 1] ||
@@ -500,6 +507,22 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
slash = strrchr(prop, '/');
if (strcmp(slash + 1, find_name))
continue;
+
+ parent_offset = fdt_parent_offset(blob, offset);
+ while ((parent_offset > 0) && (!flag)) {
+ parent_name = fdt_get_name(blob, parent_offset, &parent_namelen);
+ p = parent_name + parent_namelen - 1;
+ do {
+ if (*(--slash) != *p)
+ flag = true;
+ } while ((--p >= parent_name) && (!flag));
+ parent_offset = fdt_parent_offset(blob, parent_offset);
+ --slash;
+ }
+
+ if (flag)
+ continue;
+
val = trailing_strtol(name);
if (val != -1) {
*seqp = val;
--
2.17.1
