Re: [PATCH] acpi: device: Fix check for sequence number
Hi Wolfgang, On Thu, Sep 10, 2020 at 3:01 PM Wolfgang Wallner wrote: > > Hi Simon, > > -"Simon Glass" schrieb: ----- > > Betreff: Re: [PATCH] acpi: device: Fix check for sequence number > > > > Hi Wolfgang, > > > > On Thu, 13 Aug 2020 at 01:23, Wolfgang Wallner > > wrote: > > > > > > Hi Simon, > > > > > > -"Simon Glass" schrieb: - > > > > Betreff: Re: [PATCH] acpi: device: Fix check for sequence number > > > > > > > > Hi Wolfgang, > > > > > > > > On Thu, 30 Jul 2020 at 06:47, Wolfgang Wallner > > > > wrote: > > > > > > > > > > Currently the function acpi_check_seq() checks whether dev->req_seq is > > > > > unequal to "-1", but it should actually check dev->seq. Change it to > > > > > check dev->seq. > > > > > > > > > > For req_seq the value "-1" would be a valid (meaning 'any'), while for > > > > > seq the value "-1" means 'none' and is not valid. > > > > > > > > > > Quoting the description of udevice in device.h: > > > > > * @req_seq: Requested sequence number for this device (-1 = any) > > > > > * @seq: Allocated sequence number for this device (-1 = none). > > > > > * This is set up when the device is probed and will be unique > > > > > * within the device's uclass. > > > > > > > > > > Signed-off-by: Wolfgang Wallner > > > > > > > > > > Fixes: commit fefac0b0643b ("dm: acpi: Enhance acpi_get_name()") > > > > > > > > > > --- > > > > > > > > > > lib/acpi/acpi_device.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > What problem are you seeing without this patch? > > > > > > I see the following warning: "Device 'serial@18,2' has no seq". > > > > > > In my case req_seq for the UART is -1 ("any"), while seq is 0. > > > Why would we check for req_seq and not for seq in this function? > > > > > > > At present the ACPI device may not always be probed, and probing is > > > > when the sequence number is currently set up. > > > > > > In my case the UART is already probed before the ACPI tables are > > > generated. > > > > I would expect req_seq to be set to the UART number, i.e. the value of > > the alias (uart0, uart1) that points to the node. > > > > I wonder why that doesn't work in your case? > > I did not have an alias for my serial. I have added one and now it works as > expected. > > I misunderstood how that code is expected to work. Thanks for the explanation, > now it makes sense. > > This also means that my patch is wrong and should be dropped. > @Bin: please drop this patch. Thanks. Have updated the status of this patch in patchwork. > > > Are you sure that all UARTs are probed before ACPI tables are created? > > Normally U-Boot would only probe the one being used for the console. > > > > > > > > > > > > > I have been thinking about dropping req_seq and doing everything when > > > > the device is bound, but haven't dug into it in detail yet. > Regards, Bin
Re: [PATCH] acpi: device: Fix check for sequence number
Hi Simon, -"Simon Glass" schrieb: - > Betreff: Re: [PATCH] acpi: device: Fix check for sequence number > > Hi Wolfgang, > > On Thu, 13 Aug 2020 at 01:23, Wolfgang Wallner > wrote: > > > > Hi Simon, > > > > -----"Simon Glass" schrieb: - > > > Betreff: Re: [PATCH] acpi: device: Fix check for sequence number > > > > > > Hi Wolfgang, > > > > > > On Thu, 30 Jul 2020 at 06:47, Wolfgang Wallner > > > wrote: > > > > > > > > Currently the function acpi_check_seq() checks whether dev->req_seq is > > > > unequal to "-1", but it should actually check dev->seq. Change it to > > > > check dev->seq. > > > > > > > > For req_seq the value "-1" would be a valid (meaning 'any'), while for > > > > seq the value "-1" means 'none' and is not valid. > > > > > > > > Quoting the description of udevice in device.h: > > > > * @req_seq: Requested sequence number for this device (-1 = any) > > > > * @seq: Allocated sequence number for this device (-1 = none). > > > > * This is set up when the device is probed and will be unique > > > > * within the device's uclass. > > > > > > > > Signed-off-by: Wolfgang Wallner > > > > > > > > Fixes: commit fefac0b0643b ("dm: acpi: Enhance acpi_get_name()") > > > > > > > > --- > > > > > > > > lib/acpi/acpi_device.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > What problem are you seeing without this patch? > > > > I see the following warning: "Device 'serial@18,2' has no seq". > > > > In my case req_seq for the UART is -1 ("any"), while seq is 0. > > Why would we check for req_seq and not for seq in this function? > > > > > At present the ACPI device may not always be probed, and probing is > > > when the sequence number is currently set up. > > > > In my case the UART is already probed before the ACPI tables are generated. > > I would expect req_seq to be set to the UART number, i.e. the value of > the alias (uart0, uart1) that points to the node. > > I wonder why that doesn't work in your case? I did not have an alias for my serial. I have added one and now it works as expected. I misunderstood how that code is expected to work. Thanks for the explanation, now it makes sense. This also means that my patch is wrong and should be dropped. @Bin: please drop this patch. > Are you sure that all UARTs are probed before ACPI tables are created? > Normally U-Boot would only probe the one being used for the console. > > > > > > > > > I have been thinking about dropping req_seq and doing everything when > > > the device is bound, but haven't dug into it in detail yet. regards, Wolfgang
Re: [PATCH] acpi: device: Fix check for sequence number
Hi Wolfgang, On Thu, 13 Aug 2020 at 01:23, Wolfgang Wallner wrote: > > Hi Simon, > > -"Simon Glass" schrieb: ----- > > Betreff: Re: [PATCH] acpi: device: Fix check for sequence number > > > > Hi Wolfgang, > > > > On Thu, 30 Jul 2020 at 06:47, Wolfgang Wallner > > wrote: > > > > > > Currently the function acpi_check_seq() checks whether dev->req_seq is > > > unequal to "-1", but it should actually check dev->seq. Change it to > > > check dev->seq. > > > > > > For req_seq the value "-1" would be a valid (meaning 'any'), while for > > > seq the value "-1" means 'none' and is not valid. > > > > > > Quoting the description of udevice in device.h: > > > * @req_seq: Requested sequence number for this device (-1 = any) > > > * @seq: Allocated sequence number for this device (-1 = none). > > > * This is set up when the device is probed and will be unique > > > * within the device's uclass. > > > > > > Signed-off-by: Wolfgang Wallner > > > > > > Fixes: commit fefac0b0643b ("dm: acpi: Enhance acpi_get_name()") > > > > > > --- > > > > > > lib/acpi/acpi_device.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > What problem are you seeing without this patch? > > I see the following warning: "Device 'serial@18,2' has no seq". > > In my case req_seq for the UART is -1 ("any"), while seq is 0. > Why would we check for req_seq and not for seq in this function? > > > At present the ACPI device may not always be probed, and probing is > > when the sequence number is currently set up. > > In my case the UART is already probed before the ACPI tables are generated. I would expect req_seq to be set to the UART number, i.e. the value of the alias (uart0, uart1) that points to the node. I wonder why that doesn't work in your case? Are you sure that all UARTs are probed before ACPI tables are created? Normally U-Boot would only probe the one being used for the console. > > > > > I have been thinking about dropping req_seq and doing everything when > > the device is bound, but haven't dug into it in detail yet. > > regards, Wolfgang Regards, Simon
Re: [PATCH] acpi: device: Fix check for sequence number
Hi Simon, -"Simon Glass" schrieb: - > Betreff: Re: [PATCH] acpi: device: Fix check for sequence number > > Hi Wolfgang, > > On Thu, 30 Jul 2020 at 06:47, Wolfgang Wallner > wrote: > > > > Currently the function acpi_check_seq() checks whether dev->req_seq is > > unequal to "-1", but it should actually check dev->seq. Change it to > > check dev->seq. > > > > For req_seq the value "-1" would be a valid (meaning 'any'), while for > > seq the value "-1" means 'none' and is not valid. > > > > Quoting the description of udevice in device.h: > > * @req_seq: Requested sequence number for this device (-1 = any) > > * @seq: Allocated sequence number for this device (-1 = none). > > * This is set up when the device is probed and will be unique > > * within the device's uclass. > > > > Signed-off-by: Wolfgang Wallner > > > > Fixes: commit fefac0b0643b ("dm: acpi: Enhance acpi_get_name()") > > > > --- > > > > lib/acpi/acpi_device.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > What problem are you seeing without this patch? I see the following warning: "Device 'serial@18,2' has no seq". In my case req_seq for the UART is -1 ("any"), while seq is 0. Why would we check for req_seq and not for seq in this function? > At present the ACPI device may not always be probed, and probing is > when the sequence number is currently set up. In my case the UART is already probed before the ACPI tables are generated. > > I have been thinking about dropping req_seq and doing everything when > the device is bound, but haven't dug into it in detail yet. regards, Wolfgang
Re: [PATCH] acpi: device: Fix check for sequence number
Hi Wolfgang, On Thu, 30 Jul 2020 at 06:47, Wolfgang Wallner wrote: > > Currently the function acpi_check_seq() checks whether dev->req_seq is > unequal to "-1", but it should actually check dev->seq. Change it to > check dev->seq. > > For req_seq the value "-1" would be a valid (meaning 'any'), while for > seq the value "-1" means 'none' and is not valid. > > Quoting the description of udevice in device.h: > * @req_seq: Requested sequence number for this device (-1 = any) > * @seq: Allocated sequence number for this device (-1 = none). > * This is set up when the device is probed and will be unique > * within the device's uclass. > > Signed-off-by: Wolfgang Wallner > > Fixes: commit fefac0b0643b ("dm: acpi: Enhance acpi_get_name()") > > --- > > lib/acpi/acpi_device.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > What problem are you seeing without this patch? At present the ACPI device may not always be probed, and probing is when the sequence number is currently set up. I have been thinking about dropping req_seq and doing everything when the device is bound, but haven't dug into it in detail yet. Regards, SImon