Re: [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
Hi Marek,
On Wed, Feb 14, 2024 at 9:55 AM Shantur Rathore wrote:
>
> Additional testing of the changes introduced in commit 33e06dcbe57a "common:
> usb-hub: Reset hub port before scanning") revealed that some USB 2.0 and 3.0
> flash drives didn't work in U-Boot on some Allwinner SoCs that support USB
> 2.0 interfaces only. More precisely, some of the tested USB 2.0 and 3.0
> flash drives failed to be detected and work on an OrangePi Zero 3, based on
> the Allwinner H616 SoC that supports USB 2.0 only, while the same USB flash
> drives worked just fine on a Pine64 H64, based on the Allwinner H6 SoC that
> supports both USB 2.0 and USB 3.0 interfaces.
>
> The USB ID of the above-mentioned USB 3.0 flash drive that failed to work is
> 1f75:0917 (Innostor Technology Corporation IS917 Mass storage), it is 32 GB
> in size and sold under the PNY brand. The mentioned USB 2.0 drive is some
> inexpensive no-name drive with an invalid USB ID.
>
> Resetting USB 3.0 hubs only, which this patch introduces to the USB hub
> resets, has been tested to work as expected, resolving the identified issues
> on the Allwinner H616, while not introducing any new issues on other tested
> Allwinner SoCs. Thus, let's fix it that way.
>
> According to the USB 3.0 specification, resetting a USB 3.0 port is required
> when an attached USB device transitions between different states, such as
> when it resumes from suspend. Though, the Linux kernel performs additional
> USB 3.0 port resets upon initial USB device attachment, as visible in commit
> 07194ab7be63 ("USB: Reset USB 3.0 devices on (re)discovery") in the kernel
> source, to ensure proper state of the USB 3.0 hub port and proper USB mode
> negotiation during the initial USB device attachment and enumeration.
>
> These additional types of USB port resets don't exist for USB 2.0 hubs,
> according the USB 2.0 specification. The resets seem to be added to the USB
> 3.0 specification as part of the port and device mode negotiation.
>
> The Linux kernel resets USB 3.0 (i.e. SuperSpeed) hubs only, as visible in
> commit 10d674a82e55 ("USB: When hot reset for USB3 fails, try warm reset.")
> in the kernel source. The check for SuperSpeed hubs is performed in a way
> that also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
> which hopefully makes it future proof.
>
> Fixes: commit 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
>
> Link:
> https://lore.kernel.org/u-boot/20240207102327.35125-...@shantur.com/T/#u
> Link:
> https://lore.kernel.org/u-boot/20240201164604.13315...@donnerap.manchester.arm.com/T/#u
>
> Signed-off-by: Shantur Rathore
> Helped-by: Dragan Simic
> Tested-by: Andre Przywara
> Reviewed-by: Dragan Simic
>
> ---
>
> (no changes since v1)
>
> common/usb_hub.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 3fb7e14d10..2e054eb935 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
>
> debug("enabling power on all ports\n");
> for (i = 0; i < dev->maxchild; i++) {
> - usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
> - debug("Reset : port %d returns %lX\n", i + 1, dev->status);
> + if (usb_hub_is_superspeed(dev)) {
> + usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
> + debug("Reset : port %d returns %lX\n", i + 1,
> dev->status);
> + }
> usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
> debug("PowerOn : port %d returns %lX\n", i + 1, dev->status);
> }
> --
> 2.40.1
>
Is it possible to get this in before the next RC?
Regards,
Shantur
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On Wed, Feb 14, 2024 at 9:56 AM Shantur Rathore wrote:
>
> On Wed, Feb 14, 2024 at 3:48 AM Dragan Simic wrote:
> >
> > Hello Shantur and Marek,
> >
> > On 2024-02-14 04:18, Dragan Simic wrote:
> > > On 2024-02-14 03:04, Andre Przywara wrote:
> > >> On Mon, 12 Feb 2024 21:19:13 +0100 Marek Vasut wrote:
> > >>> On 2/12/24 14:41, Shantur Rathore wrote:
> > >>> > On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore
> > >>> > wrote:
> > >>> >> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic
> > >>> >> wrote:
> > >>> >>> On 2024-02-08 15:17, Dragan Simic wrote:
> > >>> >>>> On 2024-02-08 15:10, Shantur Rathore wrote:
> > >>> >>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic
> > >>> >>>>> wrote:
> > >>> >>>>>> On 2024-02-08 14:33, Marek Vasut wrote:
> > >>> >>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:
> > >>> >>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut
> > >>> >>>>>>>> wrote:
> > >>> >>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:
> > >>> >>>>>>>>>> USB 3.0 spec requires hub to reset device while
> > >>> >>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
> > >>> >>>>>>>>>> handle this well and after implementation of
> > >>> >>>>>>>>>> reset some USB 2.0 disks weren't detected on
> > >>> >>>>>>>>>> Allwinner based boards.
> > >>> >>>>>>>>>>
> > >>> >>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.
> > >>> >>>>>>>>>
> > >>> >>>>>>>>> It would be good to include as many details about the faulty
> > >>> >>>>>>>>> hardware
> > >>> >>>>>>>>> in
> > >>> >>>>>>>>> the commit message as possible, so that when someone else
> > >>> >>>>>>>>> runs into
> > >>> >>>>>>>>> this, they would have all that information available.
> > >>> >>>>>>>>>
> > >>> >>>>>>>>>> Tested-by: Andre Przywara
> > >>> >>>>>>>>>>
> > >>> >>>>>>>>>> Signed-off-by: Shantur Rathore
> > >>> >>>>>>>>>> ---
> > >>> >>>>>>>>>>
> > >>> >>>>>>>>>> common/usb_hub.c | 6 --
> > >>> >>>>>>>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
> > >>> >>>>>>>>>>
> > >>> >>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> > >>> >>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
> > >>> >>>>>>>>>> --- a/common/usb_hub.c
> > >>> >>>>>>>>>> +++ b/common/usb_hub.c
> > >>> >>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
> > >>> >>>>>>>>>> usb_hub_device *hub)
> > >>> >>>>>>>>>>
> > >>> >>>>>>>>>> debug("enabling power on all ports\n");
> > >>> >>>>>>>>>> for (i = 0; i < dev->maxchild; i++) {
> > >>> >>>>>>>>>> - usb_set_port_feature(dev, i + 1,
> > >>> >>>>>>>>>> USB_PORT_FEAT_RESET);
> > >>> >>>>>>>>>> - debug("Reset : port %d returns %lX\n", i + 1,
> > >>> >>>>>>>>>> dev->status);
> > >>> >>>>>>>>>> + if (usb_hub_is_superspeed(dev)) {
> > >>> >>>>>>>>>
> > >>> >>>>>>>>> Should this condi
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On Wed, Feb 14, 2024 at 3:48 AM Dragan Simic wrote:
>
> Hello Shantur and Marek,
>
> On 2024-02-14 04:18, Dragan Simic wrote:
> > On 2024-02-14 03:04, Andre Przywara wrote:
> >> On Mon, 12 Feb 2024 21:19:13 +0100 Marek Vasut wrote:
> >>> On 2/12/24 14:41, Shantur Rathore wrote:
> >>> > On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore
> >>> > wrote:
> >>> >> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic
> >>> >> wrote:
> >>> >>> On 2024-02-08 15:17, Dragan Simic wrote:
> >>> >>>> On 2024-02-08 15:10, Shantur Rathore wrote:
> >>> >>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic
> >>> >>>>> wrote:
> >>> >>>>>> On 2024-02-08 14:33, Marek Vasut wrote:
> >>> >>>>>>> On 2/8/24 12:30, Shantur Rathore wrote:
> >>> >>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut wrote:
> >>> >>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote:
> >>> >>>>>>>>>> USB 3.0 spec requires hub to reset device while
> >>> >>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't
> >>> >>>>>>>>>> handle this well and after implementation of
> >>> >>>>>>>>>> reset some USB 2.0 disks weren't detected on
> >>> >>>>>>>>>> Allwinner based boards.
> >>> >>>>>>>>>>
> >>> >>>>>>>>>> Resetting only when hub is USB 3.0 fixes it.
> >>> >>>>>>>>>
> >>> >>>>>>>>> It would be good to include as many details about the faulty
> >>> >>>>>>>>> hardware
> >>> >>>>>>>>> in
> >>> >>>>>>>>> the commit message as possible, so that when someone else runs
> >>> >>>>>>>>> into
> >>> >>>>>>>>> this, they would have all that information available.
> >>> >>>>>>>>>
> >>> >>>>>>>>>> Tested-by: Andre Przywara
> >>> >>>>>>>>>>
> >>> >>>>>>>>>> Signed-off-by: Shantur Rathore
> >>> >>>>>>>>>> ---
> >>> >>>>>>>>>>
> >>> >>>>>>>>>> common/usb_hub.c | 6 --
> >>> >>>>>>>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>> >>>>>>>>>>
> >>> >>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> >>> >>>>>>>>>> index 3fb7e14d10..2e054eb935 100644
> >>> >>>>>>>>>> --- a/common/usb_hub.c
> >>> >>>>>>>>>> +++ b/common/usb_hub.c
> >>> >>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
> >>> >>>>>>>>>> usb_hub_device *hub)
> >>> >>>>>>>>>>
> >>> >>>>>>>>>> debug("enabling power on all ports\n");
> >>> >>>>>>>>>> for (i = 0; i < dev->maxchild; i++) {
> >>> >>>>>>>>>> - usb_set_port_feature(dev, i + 1,
> >>> >>>>>>>>>> USB_PORT_FEAT_RESET);
> >>> >>>>>>>>>> - debug("Reset : port %d returns %lX\n", i + 1,
> >>> >>>>>>>>>> dev->status);
> >>> >>>>>>>>>> + if (usb_hub_is_superspeed(dev)) {
> >>> >>>>>>>>>
> >>> >>>>>>>>> Should this condition be "all which are lower than superspeed"
> >>> >>>>>>>>> instead ,
> >>> >>>>>>>>> so when the next generation of USB comes, this problem won't
> >>> >>>>>>>>> trigger
> >>> >>>>>>>>> ?
> >>> >>>>>
[PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
Additional testing of the changes introduced in commit 33e06dcbe57a "common:
usb-hub: Reset hub port before scanning") revealed that some USB 2.0 and 3.0
flash drives didn't work in U-Boot on some Allwinner SoCs that support USB
2.0 interfaces only. More precisely, some of the tested USB 2.0 and 3.0
flash drives failed to be detected and work on an OrangePi Zero 3, based on
the Allwinner H616 SoC that supports USB 2.0 only, while the same USB flash
drives worked just fine on a Pine64 H64, based on the Allwinner H6 SoC that
supports both USB 2.0 and USB 3.0 interfaces.
The USB ID of the above-mentioned USB 3.0 flash drive that failed to work is
1f75:0917 (Innostor Technology Corporation IS917 Mass storage), it is 32 GB
in size and sold under the PNY brand. The mentioned USB 2.0 drive is some
inexpensive no-name drive with an invalid USB ID.
Resetting USB 3.0 hubs only, which this patch introduces to the USB hub
resets, has been tested to work as expected, resolving the identified issues
on the Allwinner H616, while not introducing any new issues on other tested
Allwinner SoCs. Thus, let's fix it that way.
According to the USB 3.0 specification, resetting a USB 3.0 port is required
when an attached USB device transitions between different states, such as
when it resumes from suspend. Though, the Linux kernel performs additional
USB 3.0 port resets upon initial USB device attachment, as visible in commit
07194ab7be63 ("USB: Reset USB 3.0 devices on (re)discovery") in the kernel
source, to ensure proper state of the USB 3.0 hub port and proper USB mode
negotiation during the initial USB device attachment and enumeration.
These additional types of USB port resets don't exist for USB 2.0 hubs,
according the USB 2.0 specification. The resets seem to be added to the USB
3.0 specification as part of the port and device mode negotiation.
The Linux kernel resets USB 3.0 (i.e. SuperSpeed) hubs only, as visible in
commit 10d674a82e55 ("USB: When hot reset for USB3 fails, try warm reset.")
in the kernel source. The check for SuperSpeed hubs is performed in a way
that also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
which hopefully makes it future proof.
Fixes: commit 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
Link:
https://lore.kernel.org/u-boot/20240207102327.35125-...@shantur.com/T/#u
Link:
https://lore.kernel.org/u-boot/20240201164604.13315...@donnerap.manchester.arm.com/T/#u
Signed-off-by: Shantur Rathore
Helped-by: Dragan Simic
Tested-by: Andre Przywara
Reviewed-by: Dragan Simic
---
(no changes since v1)
common/usb_hub.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/common/usb_hub.c b/common/usb_hub.c
index 3fb7e14d10..2e054eb935 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
debug("enabling power on all ports\n");
for (i = 0; i < dev->maxchild; i++) {
- usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
- debug("Reset : port %d returns %lX\n", i + 1, dev->status);
+ if (usb_hub_is_superspeed(dev)) {
+ usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
+ debug("Reset : port %d returns %lX\n", i + 1,
dev->status);
+ }
usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
debug("PowerOn : port %d returns %lX\n", i + 1, dev->status);
}
--
2.40.1
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore wrote:
>
> Thanks Dragan,
>
> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic wrote:
> >
> > Hello Shantur,
> >
> > On 2024-02-08 15:17, Dragan Simic wrote:
> > > On 2024-02-08 15:10, Shantur Rathore wrote:
> > >> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic
> > >> wrote:
> > >>> On 2024-02-08 14:33, Marek Vasut wrote:
> > >>> > On 2/8/24 12:30, Shantur Rathore wrote:
> > >>> >> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut wrote:
> > >>> >>> On 2/7/24 11:23, Shantur Rathore wrote:
> > >>> >>>> USB 3.0 spec requires hub to reset device while
> > >>> >>>> enumeration. Some USB 2.0 hubs / devices don't
> > >>> >>>> handle this well and after implementation of
> > >>> >>>> reset some USB 2.0 disks weren't detected on
> > >>> >>>> Allwinner based boards.
> > >>> >>>>
> > >>> >>>> Resetting only when hub is USB 3.0 fixes it.
> > >>> >>>
> > >>> >>> It would be good to include as many details about the faulty
> > >>> >>> hardware
> > >>> >>> in
> > >>> >>> the commit message as possible, so that when someone else runs into
> > >>> >>> this, they would have all that information available.
> > >>> >>>
> > >>> >>>> Tested-by: Andre Przywara
> > >>> >>>>
> > >>> >>>> Signed-off-by: Shantur Rathore
> > >>> >>>> ---
> > >>> >>>>
> > >>> >>>>common/usb_hub.c | 6 --
> > >>> >>>>1 file changed, 4 insertions(+), 2 deletions(-)
> > >>> >>>>
> > >>> >>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> > >>> >>>> index 3fb7e14d10..2e054eb935 100644
> > >>> >>>> --- a/common/usb_hub.c
> > >>> >>>> +++ b/common/usb_hub.c
> > >>> >>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
> > >>> >>>> usb_hub_device *hub)
> > >>> >>>>
> > >>> >>>>debug("enabling power on all ports\n");
> > >>> >>>>for (i = 0; i < dev->maxchild; i++) {
> > >>> >>>> - usb_set_port_feature(dev, i + 1,
> > >>> >>>> USB_PORT_FEAT_RESET);
> > >>> >>>> - debug("Reset : port %d returns %lX\n", i + 1,
> > >>> >>>> dev->status);
> > >>> >>>> + if (usb_hub_is_superspeed(dev)) {
> > >>> >>>
> > >>> >>> Should this condition be "all which are lower than superspeed"
> > >>> >>> instead ,
> > >>> >>> so when the next generation of USB comes, this problem won't trigger
> > >>> >>> ?
> > >>> >>>
> > >>> >>> What does Linux do btw ?
> > >>> >>
> > >>> >> As of now Linux checks if the hub is superspeed
> > >>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
> > >>> >>
> > >>> >> which is
> > >>> >> return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
> > >>> >> USB_HUB_PR_SS = 3
> > >>> >>
> > >>> >> This holds true for newer SuperSpeedPlus hubs as well.
> > >>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
> > >>> >>
> > >>> >> We can change the check to be bDeviceProtocol > 2 but who knows if
> > >>> >> things change in the newer version of spec.
> > >>> >> I am open to suggestions.
> > >>> >
> > >>> > Please just include the ^ in the commit description. Use link to
> > >>> > git.kernel.org , not some mirror . This is extremely useful
> > >>> > information and, well, you already wrote the V2 commit message
> > >>> > addition in this answer.
> > >>>
> > >>> Shantur, if that would
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
Thanks Dragan,
On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic wrote:
>
> Hello Shantur,
>
> On 2024-02-08 15:17, Dragan Simic wrote:
> > On 2024-02-08 15:10, Shantur Rathore wrote:
> >> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic
> >> wrote:
> >>> On 2024-02-08 14:33, Marek Vasut wrote:
> >>> > On 2/8/24 12:30, Shantur Rathore wrote:
> >>> >> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut wrote:
> >>> >>> On 2/7/24 11:23, Shantur Rathore wrote:
> >>> >>>> USB 3.0 spec requires hub to reset device while
> >>> >>>> enumeration. Some USB 2.0 hubs / devices don't
> >>> >>>> handle this well and after implementation of
> >>> >>>> reset some USB 2.0 disks weren't detected on
> >>> >>>> Allwinner based boards.
> >>> >>>>
> >>> >>>> Resetting only when hub is USB 3.0 fixes it.
> >>> >>>
> >>> >>> It would be good to include as many details about the faulty hardware
> >>> >>> in
> >>> >>> the commit message as possible, so that when someone else runs into
> >>> >>> this, they would have all that information available.
> >>> >>>
> >>> >>>> Tested-by: Andre Przywara
> >>> >>>>
> >>> >>>> Signed-off-by: Shantur Rathore
> >>> >>>> ---
> >>> >>>>
> >>> >>>>common/usb_hub.c | 6 --
> >>> >>>>1 file changed, 4 insertions(+), 2 deletions(-)
> >>> >>>>
> >>> >>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> >>> >>>> index 3fb7e14d10..2e054eb935 100644
> >>> >>>> --- a/common/usb_hub.c
> >>> >>>> +++ b/common/usb_hub.c
> >>> >>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
> >>> >>>> usb_hub_device *hub)
> >>> >>>>
> >>> >>>>debug("enabling power on all ports\n");
> >>> >>>>for (i = 0; i < dev->maxchild; i++) {
> >>> >>>> - usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
> >>> >>>> - debug("Reset : port %d returns %lX\n", i + 1,
> >>> >>>> dev->status);
> >>> >>>> + if (usb_hub_is_superspeed(dev)) {
> >>> >>>
> >>> >>> Should this condition be "all which are lower than superspeed"
> >>> >>> instead ,
> >>> >>> so when the next generation of USB comes, this problem won't trigger
> >>> >>> ?
> >>> >>>
> >>> >>> What does Linux do btw ?
> >>> >>
> >>> >> As of now Linux checks if the hub is superspeed
> >>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
> >>> >>
> >>> >> which is
> >>> >> return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
> >>> >> USB_HUB_PR_SS = 3
> >>> >>
> >>> >> This holds true for newer SuperSpeedPlus hubs as well.
> >>> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
> >>> >>
> >>> >> We can change the check to be bDeviceProtocol > 2 but who knows if
> >>> >> things change in the newer version of spec.
> >>> >> I am open to suggestions.
> >>> >
> >>> > Please just include the ^ in the commit description. Use link to
> >>> > git.kernel.org , not some mirror . This is extremely useful
> >>> > information and, well, you already wrote the V2 commit message
> >>> > addition in this answer.
> >>>
> >>> Shantur, if that would be easier or quicker for you, I can write
> >>> a quite detailed patch description for you, in exchange for a
> >>> "Helped-by" tag in the v2 patch submission. :)
> >>
> >> That would be really kind of you Dragan.
> >
> > Sure, I'll write the summary and send it over.
> >
> >> I am down with the flu so that would really help me as my brain is
> >> working at 15% capacity.
> >
> > Oh, I'm really sorry to h
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic wrote:
>
> On 2024-02-08 14:33, Marek Vasut wrote:
> > On 2/8/24 12:30, Shantur Rathore wrote:
> >> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut wrote:
> >>> On 2/7/24 11:23, Shantur Rathore wrote:
> >>>> USB 3.0 spec requires hub to reset device while
> >>>> enumeration. Some USB 2.0 hubs / devices don't
> >>>> handle this well and after implementation of
> >>>> reset some USB 2.0 disks weren't detected on
> >>>> Allwinner based boards.
> >>>>
> >>>> Resetting only when hub is USB 3.0 fixes it.
> >>>
> >>> It would be good to include as many details about the faulty hardware
> >>> in
> >>> the commit message as possible, so that when someone else runs into
> >>> this, they would have all that information available.
> >>>
> >>>> Tested-by: Andre Przywara
> >>>>
> >>>> Signed-off-by: Shantur Rathore
> >>>> ---
> >>>>
> >>>>common/usb_hub.c | 6 --
> >>>>1 file changed, 4 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
> >>>> index 3fb7e14d10..2e054eb935 100644
> >>>> --- a/common/usb_hub.c
> >>>> +++ b/common/usb_hub.c
> >>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct
> >>>> usb_hub_device *hub)
> >>>>
> >>>>debug("enabling power on all ports\n");
> >>>>for (i = 0; i < dev->maxchild; i++) {
> >>>> - usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
> >>>> - debug("Reset : port %d returns %lX\n", i + 1,
> >>>> dev->status);
> >>>> + if (usb_hub_is_superspeed(dev)) {
> >>>
> >>> Should this condition be "all which are lower than superspeed"
> >>> instead ,
> >>> so when the next generation of USB comes, this problem won't trigger
> >>> ?
> >>>
> >>> What does Linux do btw ?
> >>
> >> As of now Linux checks if the hub is superspeed
> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
> >>
> >> which is
> >> return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; //
> >> USB_HUB_PR_SS = 3
> >>
> >> This holds true for newer SuperSpeedPlus hubs as well.
> >> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
> >>
> >> We can change the check to be bDeviceProtocol > 2 but who knows if
> >> things change in the newer version of spec.
> >> I am open to suggestions.
> >
> > Please just include the ^ in the commit description. Use link to
> > git.kernel.org , not some mirror . This is extremely useful
> > information and, well, you already wrote the V2 commit message
> > addition in this answer.
>
> Shantur, if that would be easier or quicker for you, I can write
> a quite detailed patch description for you, in exchange for a
> "Helped-by" tag in the v2 patch submission. :)
That would be really kind of you Dragan.
I am down with the flu so that would really help me as my brain is
working at 15% capacity.
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
Hi Marek,
On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut wrote:
>
> On 2/7/24 11:23, Shantur Rathore wrote:
> > USB 3.0 spec requires hub to reset device while
> > enumeration. Some USB 2.0 hubs / devices don't
> > handle this well and after implementation of
> > reset some USB 2.0 disks weren't detected on
> > Allwinner based boards.
> >
> > Resetting only when hub is USB 3.0 fixes it.
>
> It would be good to include as many details about the faulty hardware in
> the commit message as possible, so that when someone else runs into
> this, they would have all that information available.
>
> > Tested-by: Andre Przywara
> >
> > Signed-off-by: Shantur Rathore
> > ---
> >
> > common/usb_hub.c | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/common/usb_hub.c b/common/usb_hub.c
> > index 3fb7e14d10..2e054eb935 100644
> > --- a/common/usb_hub.c
> > +++ b/common/usb_hub.c
> > @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device
> > *hub)
> >
> > debug("enabling power on all ports\n");
> > for (i = 0; i < dev->maxchild; i++) {
> > - usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
> > - debug("Reset : port %d returns %lX\n", i + 1, dev->status);
> > + if (usb_hub_is_superspeed(dev)) {
>
> Should this condition be "all which are lower than superspeed" instead ,
> so when the next generation of USB comes, this problem won't trigger ?
>
> What does Linux do btw ?
As of now Linux checks if the hub is superspeed
https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
which is
return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; // USB_HUB_PR_SS = 3
This holds true for newer SuperSpeedPlus hubs as well.
https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
We can change the check to be bDeviceProtocol > 2 but who knows if
things change in the newer version of spec.
I am open to suggestions.
Kind regards,
Shantur
[FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
USB 3.0 spec requires hub to reset device while
enumeration. Some USB 2.0 hubs / devices don't
handle this well and after implementation of
reset some USB 2.0 disks weren't detected on
Allwinner based boards.
Resetting only when hub is USB 3.0 fixes it.
Tested-by: Andre Przywara
Signed-off-by: Shantur Rathore
---
common/usb_hub.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/common/usb_hub.c b/common/usb_hub.c
index 3fb7e14d10..2e054eb935 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
debug("enabling power on all ports\n");
for (i = 0; i < dev->maxchild; i++) {
- usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
- debug("Reset : port %d returns %lX\n", i + 1, dev->status);
+ if (usb_hub_is_superspeed(dev)) {
+ usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
+ debug("Reset : port %d returns %lX\n", i + 1,
dev->status);
+ }
usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
debug("PowerOn : port %d returns %lX\n", i + 1, dev->status);
}
--
2.40.1
Re: [PATCH v3] common: usb-hub: Reset hub port before scanning
On Wed, Feb 7, 2024 at 9:22 AM Shantur Rathore wrote: > > Hi Dragan and Andre, > > On Sat, Feb 3, 2024 at 10:39 AM Dragan Simic wrote: > > > > Hello Andre and Shantur, > > > > On 2024-02-02 12:28, Andre Przywara wrote: > > > On Fri, 02 Feb 2024 03:35:24 +0100 Dragan Simic > > > wrote: > > >> On 2024-02-02 01:12, Andre Przywara wrote: > > >> > On Thu, 1 Feb 2024 18:35:28 + Shantur Rathore > > >> > wrote: > > >> >> On Thu, Feb 1, 2024 at 4:46 PM Andre Przywara > > >> >> wrote: > > >> >> > On Thu, 1 Feb 2024 09:39:54 + Shantur Rathore > > >> >> > wrote: > > >> >> > > Which USB disk are you using? Is it a USB 2.0 disk ? > > >> >> > > Can you try with any other USB disk to confirm? > > >> >> > > > >> >> > Yes, this was some USB 2.0 memory stick, on an USB 2.0 port. > > >> >> > Most sunxi boards only have USB 2.0, but there is one SoC with USB > > >> >> > 3.0, I > > >> >> > will try some combinations later tonight. > > >> >> > > >> >> That would be awesome. > > >> >> We need to test > > >> >> > > >> >> USB 3.0 and 2.0 ports with USB 3.0 and 2.0 drives. > > >> > > > >> > So I had some mixed and apparently inconsistent results: > > >> > - On the Pine-H64 (H6 SoC with USB 3.0 + USB 2.0) it worked with > > >> > mainline, but only after I applied some pending sunxi USB PHY > > >> > regulator patches. This is an unrelated issue, those patches > > >> > are needed to enable USB 3.0 support on that board (shared regulator > > >> > conflict). > > >> > I tested a USB 2.0 and a USB 3.0 drive in both kind of slots, with > > >> > and without the patch. > > >> > - On the OrangePi Zero 3 (H616 SoC with USB 2.0 only), the USB 3.0 > > >> > drive would not work with mainline. A USB 2.0 drive was fine. > > >> > Applying your patch below fixed that problem, now both drives worked. > > >> > > >> Let me interject, please... > > >> > > >> Huh, this (mih)behavior with the tested OrangePi Zero 3 is really > > >> strange. As we know, a USB 3.0 drive plugged into a USB 2.0 port > > >> should behave just like a USB 2.0 drive, using the separate set > > >> of pins inside the connector. > > > > > > Yes, I found this odd as well, hence saying inconsistent above. > > > > > >> If possible, performing some additional testing with another USB > > >> 3.0 drive would be quite interesting. Could it be that something > > >> is wrong with the USB 2.0 receptacle on your OrangePi Zero 3, > > >> mechanically or electrically? > > > > > > The receptacle on the OPi Zero3 is a standard USB 2.0 Type-A socket, > > > with clearly only 4 pins. The USB 3.0 stick in question has all 9 pins. > > > So indeed there should be little difference to a USB 2.0 stick. > > > > Ah, sorry, I wasn't clear enough... I actually had some possible > > damage to the Zero 3's USB 2.0 receptacle in mind, or some results > > of the regular wear and tear, which could've possibly caused some > > intermittent issues. > > > > > And I just tested the same board with some other (cheap) USB 2.0 stick: > > > it's the same situation as with the USB 3.0 drive: it does not work > > > with > > > mainline, but works with the patch below. So the USB 3.0 device here > > > might > > > be a red herring, and it's actually more up to an individual device? > > > Or maybe it's like: *Some* USB devices (in Hi-Speed mode?) do not like > > > it > > > when their port is reset before it's powered on? And the root hub > > > implementation also plays a role, which is why we only got reports > > > about > > > Allwinner boards? > > > > Hmm, we've got quite a few unknowns there. Thank you for > > performing the additional testing! > > > > > Also I think Marek said that Linux does the reset only when using > > > SuperSpeed (hence the patch below), so maybe it's something in the USB > > > 3.0 > > > spec that changed the requirements? > > > > I just spent about an hour and a half going through the USB 2.0 > > and USB 3.0 specifications. From what I understood, resetting > >
Re: [PATCH v3] common: usb-hub: Reset hub port before scanning
Hi Dragan and Andre, On Sat, Feb 3, 2024 at 10:39 AM Dragan Simic wrote: > > Hello Andre and Shantur, > > On 2024-02-02 12:28, Andre Przywara wrote: > > On Fri, 02 Feb 2024 03:35:24 +0100 Dragan Simic > > wrote: > >> On 2024-02-02 01:12, Andre Przywara wrote: > >> > On Thu, 1 Feb 2024 18:35:28 + Shantur Rathore > >> > wrote: > >> >> On Thu, Feb 1, 2024 at 4:46 PM Andre Przywara > >> >> wrote: > >> >> > On Thu, 1 Feb 2024 09:39:54 + Shantur Rathore > >> >> > wrote: > >> >> > > Which USB disk are you using? Is it a USB 2.0 disk ? > >> >> > > Can you try with any other USB disk to confirm? > >> >> > > >> >> > Yes, this was some USB 2.0 memory stick, on an USB 2.0 port. > >> >> > Most sunxi boards only have USB 2.0, but there is one SoC with USB > >> >> > 3.0, I > >> >> > will try some combinations later tonight. > >> >> > >> >> That would be awesome. > >> >> We need to test > >> >> > >> >> USB 3.0 and 2.0 ports with USB 3.0 and 2.0 drives. > >> > > >> > So I had some mixed and apparently inconsistent results: > >> > - On the Pine-H64 (H6 SoC with USB 3.0 + USB 2.0) it worked with > >> > mainline, but only after I applied some pending sunxi USB PHY > >> > regulator patches. This is an unrelated issue, those patches > >> > are needed to enable USB 3.0 support on that board (shared regulator > >> > conflict). > >> > I tested a USB 2.0 and a USB 3.0 drive in both kind of slots, with > >> > and without the patch. > >> > - On the OrangePi Zero 3 (H616 SoC with USB 2.0 only), the USB 3.0 > >> > drive would not work with mainline. A USB 2.0 drive was fine. > >> > Applying your patch below fixed that problem, now both drives worked. > >> > >> Let me interject, please... > >> > >> Huh, this (mih)behavior with the tested OrangePi Zero 3 is really > >> strange. As we know, a USB 3.0 drive plugged into a USB 2.0 port > >> should behave just like a USB 2.0 drive, using the separate set > >> of pins inside the connector. > > > > Yes, I found this odd as well, hence saying inconsistent above. > > > >> If possible, performing some additional testing with another USB > >> 3.0 drive would be quite interesting. Could it be that something > >> is wrong with the USB 2.0 receptacle on your OrangePi Zero 3, > >> mechanically or electrically? > > > > The receptacle on the OPi Zero3 is a standard USB 2.0 Type-A socket, > > with clearly only 4 pins. The USB 3.0 stick in question has all 9 pins. > > So indeed there should be little difference to a USB 2.0 stick. > > Ah, sorry, I wasn't clear enough... I actually had some possible > damage to the Zero 3's USB 2.0 receptacle in mind, or some results > of the regular wear and tear, which could've possibly caused some > intermittent issues. > > > And I just tested the same board with some other (cheap) USB 2.0 stick: > > it's the same situation as with the USB 3.0 drive: it does not work > > with > > mainline, but works with the patch below. So the USB 3.0 device here > > might > > be a red herring, and it's actually more up to an individual device? > > Or maybe it's like: *Some* USB devices (in Hi-Speed mode?) do not like > > it > > when their port is reset before it's powered on? And the root hub > > implementation also plays a role, which is why we only got reports > > about > > Allwinner boards? > > Hmm, we've got quite a few unknowns there. Thank you for > performing the additional testing! > > > Also I think Marek said that Linux does the reset only when using > > SuperSpeed (hence the patch below), so maybe it's something in the USB > > 3.0 > > spec that changed the requirements? > > I just spent about an hour and a half going through the USB 2.0 > and USB 3.0 specifications. From what I understood, resetting > a USB 3.0 port should be required when a USB device transitions > between different states, such as when it resumes from suspend, > but maybe not necessarily upon the initial device attachment. > > Though, the Linux kernel USB driver seems to be doing additional > USB 3.0 port resets upon the initial USB device attachment, > presumably to ensure proper state of the USB 3.0 hub port and > proper USB mode negotiation. > > Here are the links to a couple o
Re: [PATCH v3] common: usb-hub: Reset hub port before scanning
Hi Andre,
On Thu, Feb 1, 2024 at 4:46 PM Andre Przywara wrote:
>
> On Thu, 1 Feb 2024 09:39:54 +0000
> Shantur Rathore wrote:
>
> Hi Shantur,
>
> > On Tue, Jan 30, 2024 at 2:01 PM Andre Przywara
> > wrote:
> > >
> > > On Sat, 9 Dec 2023 18:10:56 +
> > > Shantur Rathore wrote:
> > >
> > > Hi,
> > >
> > > > Currently when a hub is turned on, all the ports are powered on.
> > > > This works well for hubs which have individual power control.
> > > >
> > > > For the hubs without individual power control this has no effect.
> > > > Mostly in these scenarios the hub port is powered before the USB
> > > > controller is enabled, this can lead to some devices in unexpected
> > > > state.
> > > >
> > > > With this patch, we explicitly reset the port while powering up hub
> > > > This resets the port for hubs without port power control and has
> > > > no effect on hubs with port power control as the port is still off.
> > > >
> > > > Before this patch AMicro AM8180 based NVME to USB adapter won't be
> > > > detected as a USB3.0 Mass Storage device but with this it works as
> > > > expected.
> > > >
> > > > Tested working after this patch:
> > > > 1. AMicro AM8180 based NVME to USB Adapter
> > > > 2. Kingston DataTraveler 3.0
> > > > 3. GenesysLogic USB3.0 Hub
> > > >
> > > > The drives were tested while connected directly and via the hub.
> > >
> > > so this broke USB operation on some Allwinner boards. The ports still
> > > enumerate correctly, but no devices are detected. With mainline
> > > (containing this patch), and a USB stick connected:
> > > starting USB...
> > > Bus usb@520: USB EHCI 1.00
> > > Bus usb@5200400: USB OHCI 1.0
> > > scanning bus usb@520 for devices... 1 USB Device(s) found
> > > scanning bus usb@5200400 for devices... 1 USB Device(s) found
> > >scanning usb for storage devices... 0 Storage Device(s) found
> > >
> > > With this patch here reverted:
> > > starting USB...
> > > Bus usb@520: USB EHCI 1.00
> > > Bus usb@5200400: USB OHCI 1.0
> > > scanning bus usb@520 for devices... 2 USB Device(s) found
> > > scanning bus usb@5200400 for devices... 1 USB Device(s) found
> > >scanning usb for storage devices... 1 Storage Device(s) found
> > >
> > > I have no clue why this happens, there is no discrete hub anywhere in this
> > > setup, so I guess the code runs for the root hub here?
> > > I am attaching a log with DEBUG enabled for common/usb_hub.c, for both
> > > cases.
> > >
> > > Any clues, debug suggestions, or even a fix are warmly welcome.
> > >
> >
> > Which USB disk are you using? Is it a USB 2.0 disk ?
> > Can you try with any other USB disk to confirm?
>
> Yes, this was some USB 2.0 memory stick, on an USB 2.0 port.
> Most sunxi boards only have USB 2.0, but there is one SoC with USB 3.0, I
> will try some combinations later tonight.
>
> Thanks,
> Andre
That would be awesome.
We need to test
USB 3.0 and 2.0 ports with USB 3.0 and 2.0 drives.
Once you have tested it, can you please try the patch below
debug("enabling power on all ports\n");
for (i = 0; i < dev->maxchild; i++) {
- usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
- debug("Reset : port %d returns %lX\n", i + 1, dev->status);
+ if (usb_hub_is_superspeed(hub)) {
+ usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
+ debug("Reset : port %d returns %lX\n", i + 1,
dev->status);
+ }
usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
debug("PowerOn : port %d returns %lX\n", i + 1, dev->status);
}
Thanks
Re: [PATCH v3] common: usb-hub: Reset hub port before scanning
On Thu, Feb 1, 2024 at 2:53 PM Marek Vasut wrote: > > On 2/1/24 12:18, Shantur Rathore wrote: > > On Thu, Feb 1, 2024 at 11:13 AM Marek Vasut wrote: > >> > >> On 2/1/24 10:39, Shantur Rathore wrote: > >>> Hi Andre, > >>> > >>> On Tue, Jan 30, 2024 at 2:01 PM Andre Przywara > >>> wrote: > >>>> > >>>> On Sat, 9 Dec 2023 18:10:56 + > >>>> Shantur Rathore wrote: > >>>> > >>>> Hi, > >>>> > >>>>> Currently when a hub is turned on, all the ports are powered on. > >>>>> This works well for hubs which have individual power control. > >>>>> > >>>>> For the hubs without individual power control this has no effect. > >>>>> Mostly in these scenarios the hub port is powered before the USB > >>>>> controller is enabled, this can lead to some devices in unexpected > >>>>> state. > >>>>> > >>>>> With this patch, we explicitly reset the port while powering up hub > >>>>> This resets the port for hubs without port power control and has > >>>>> no effect on hubs with port power control as the port is still off. > >>>>> > >>>>> Before this patch AMicro AM8180 based NVME to USB adapter won't be > >>>>> detected as a USB3.0 Mass Storage device but with this it works as > >>>>> expected. > >>>>> > >>>>> Tested working after this patch: > >>>>> 1. AMicro AM8180 based NVME to USB Adapter > >>>>> 2. Kingston DataTraveler 3.0 > >>>>> 3. GenesysLogic USB3.0 Hub > >>>>> > >>>>> The drives were tested while connected directly and via the hub. > >>>> > >>>> so this broke USB operation on some Allwinner boards. The ports still > >>>> enumerate correctly, but no devices are detected. With mainline > >>>> (containing this patch), and a USB stick connected: > >>>> starting USB... > >>>> Bus usb@520: USB EHCI 1.00 > >>>> Bus usb@5200400: USB OHCI 1.0 > >>>> scanning bus usb@520 for devices... 1 USB Device(s) found > >>>> scanning bus usb@5200400 for devices... 1 USB Device(s) found > >>>> scanning usb for storage devices... 0 Storage Device(s) found > >>>> > >>>> With this patch here reverted: > >>>> starting USB... > >>>> Bus usb@520: USB EHCI 1.00 > >>>> Bus usb@5200400: USB OHCI 1.0 > >>>> scanning bus usb@520 for devices... 2 USB Device(s) found > >>>> scanning bus usb@5200400 for devices... 1 USB Device(s) found > >>>> scanning usb for storage devices... 1 Storage Device(s) found > >>>> > >>>> I have no clue why this happens, there is no discrete hub anywhere in > >>>> this > >>>> setup, so I guess the code runs for the root hub here? > >>>> I am attaching a log with DEBUG enabled for common/usb_hub.c, for both > >>>> cases. > >>>> > >>>> Any clues, debug suggestions, or even a fix are warmly welcome. > >>>> > >>> > >>> Which USB disk are you using? Is it a USB 2.0 disk ? > >>> Can you try with any other USB disk to confirm? > >> > >> I don't think this is device specific, see the logs. Hardware from Andre > >> worked before this patch, does not work after this patch. There seem to > >> be no connection event with this patch. > > > > I am trying to evaluate if this is happening due to a USB 2.0 device. > > I see that the host is USB 2.0. I have tested this on my RK3399 board on > > both > > USB 2.0 and USB 3.0 ports. > > If this is not behaving on USB 2.0 ports then I can put a condition > > for this to happen > > only on USB 3.0 ports. > > Doesn't Linux perform this reset for all ports unconditionally ? Looks like it doesn't https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2841
Re: [PATCH v3] common: usb-hub: Reset hub port before scanning
On Thu, Feb 1, 2024 at 11:13 AM Marek Vasut wrote: > > On 2/1/24 10:39, Shantur Rathore wrote: > > Hi Andre, > > > > On Tue, Jan 30, 2024 at 2:01 PM Andre Przywara > > wrote: > >> > >> On Sat, 9 Dec 2023 18:10:56 + > >> Shantur Rathore wrote: > >> > >> Hi, > >> > >>> Currently when a hub is turned on, all the ports are powered on. > >>> This works well for hubs which have individual power control. > >>> > >>> For the hubs without individual power control this has no effect. > >>> Mostly in these scenarios the hub port is powered before the USB > >>> controller is enabled, this can lead to some devices in unexpected > >>> state. > >>> > >>> With this patch, we explicitly reset the port while powering up hub > >>> This resets the port for hubs without port power control and has > >>> no effect on hubs with port power control as the port is still off. > >>> > >>> Before this patch AMicro AM8180 based NVME to USB adapter won't be > >>> detected as a USB3.0 Mass Storage device but with this it works as > >>> expected. > >>> > >>> Tested working after this patch: > >>> 1. AMicro AM8180 based NVME to USB Adapter > >>> 2. Kingston DataTraveler 3.0 > >>> 3. GenesysLogic USB3.0 Hub > >>> > >>> The drives were tested while connected directly and via the hub. > >> > >> so this broke USB operation on some Allwinner boards. The ports still > >> enumerate correctly, but no devices are detected. With mainline > >> (containing this patch), and a USB stick connected: > >> starting USB... > >> Bus usb@520: USB EHCI 1.00 > >> Bus usb@5200400: USB OHCI 1.0 > >> scanning bus usb@520 for devices... 1 USB Device(s) found > >> scanning bus usb@5200400 for devices... 1 USB Device(s) found > >> scanning usb for storage devices... 0 Storage Device(s) found > >> > >> With this patch here reverted: > >> starting USB... > >> Bus usb@520: USB EHCI 1.00 > >> Bus usb@5200400: USB OHCI 1.0 > >> scanning bus usb@520 for devices... 2 USB Device(s) found > >> scanning bus usb@5200400 for devices... 1 USB Device(s) found > >> scanning usb for storage devices... 1 Storage Device(s) found > >> > >> I have no clue why this happens, there is no discrete hub anywhere in this > >> setup, so I guess the code runs for the root hub here? > >> I am attaching a log with DEBUG enabled for common/usb_hub.c, for both > >> cases. > >> > >> Any clues, debug suggestions, or even a fix are warmly welcome. > >> > > > > Which USB disk are you using? Is it a USB 2.0 disk ? > > Can you try with any other USB disk to confirm? > > I don't think this is device specific, see the logs. Hardware from Andre > worked before this patch, does not work after this patch. There seem to > be no connection event with this patch. I am trying to evaluate if this is happening due to a USB 2.0 device. I see that the host is USB 2.0. I have tested this on my RK3399 board on both USB 2.0 and USB 3.0 ports. If this is not behaving on USB 2.0 ports then I can put a condition for this to happen only on USB 3.0 ports.
Re: [PATCH v3] common: usb-hub: Reset hub port before scanning
Hi Andre, On Tue, Jan 30, 2024 at 2:01 PM Andre Przywara wrote: > > On Sat, 9 Dec 2023 18:10:56 +0000 > Shantur Rathore wrote: > > Hi, > > > Currently when a hub is turned on, all the ports are powered on. > > This works well for hubs which have individual power control. > > > > For the hubs without individual power control this has no effect. > > Mostly in these scenarios the hub port is powered before the USB > > controller is enabled, this can lead to some devices in unexpected > > state. > > > > With this patch, we explicitly reset the port while powering up hub > > This resets the port for hubs without port power control and has > > no effect on hubs with port power control as the port is still off. > > > > Before this patch AMicro AM8180 based NVME to USB adapter won't be > > detected as a USB3.0 Mass Storage device but with this it works as > > expected. > > > > Tested working after this patch: > > 1. AMicro AM8180 based NVME to USB Adapter > > 2. Kingston DataTraveler 3.0 > > 3. GenesysLogic USB3.0 Hub > > > > The drives were tested while connected directly and via the hub. > > so this broke USB operation on some Allwinner boards. The ports still > enumerate correctly, but no devices are detected. With mainline > (containing this patch), and a USB stick connected: > starting USB... > Bus usb@520: USB EHCI 1.00 > Bus usb@5200400: USB OHCI 1.0 > scanning bus usb@520 for devices... 1 USB Device(s) found > scanning bus usb@5200400 for devices... 1 USB Device(s) found >scanning usb for storage devices... 0 Storage Device(s) found > > With this patch here reverted: > starting USB... > Bus usb@520: USB EHCI 1.00 > Bus usb@5200400: USB OHCI 1.0 > scanning bus usb@520 for devices... 2 USB Device(s) found > scanning bus usb@5200400 for devices... 1 USB Device(s) found >scanning usb for storage devices... 1 Storage Device(s) found > > I have no clue why this happens, there is no discrete hub anywhere in this > setup, so I guess the code runs for the root hub here? > I am attaching a log with DEBUG enabled for common/usb_hub.c, for both > cases. > > Any clues, debug suggestions, or even a fix are warmly welcome. > Which USB disk are you using? Is it a USB 2.0 disk ? Can you try with any other USB disk to confirm? Regards, Shantur
Re: [PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
Hi Kever, On Thu, Jan 18, 2024 at 3:10 AM Kever Yang wrote: > > Hi Shantur, > > On 2024/1/17 17:38, Shantur Rathore wrote: > > Hi Kever, > > > > On Wed, Jan 17, 2024 at 9:18 AM Kever Yang > > wrote: > >> Hi Shantur, > >> > >> On 2024/1/17 14:26, Shantur Rathore wrote: > >> > >> Hi Kever, > >> > >> On Wed, Jan 10, 2024 at 11:58 AM Shantur Rathore wrote: > >> > >> Hi Kever, > >> > >> On Tue, Jan 9, 2024 at 10:55 AM Kever Yang > >> wrote: > >> > >> Hi Shantur, Tom, > >> > >> On 2023/12/10 04:45, Tom Rini wrote: > >> > >> On Sat, Dec 09, 2023 at 07:49:04PM +, Shantur Rathore wrote: > >> > >> On Sat, Dec 9, 2023 at 7:18 PM Tom Rini wrote: > >> > >> On Fri, Dec 08, 2023 at 10:52:02AM +, Shantur Rathore wrote: > >> > >> Hi Tom / Kever > >> > >> On Sun, Nov 19, 2023 at 5:24 PM Shantur Rathore wrote: > >> > >> Rockchip SoCs can support wide range of bootflows. > >> Without full bootflow commands, it can be difficult to > >> figure out issues if any, hence enable by default. > >> > >> Reviewed-by: Simon Glass > >> > >> Signed-off-by: Shantur Rathore > >> --- > >> > >> (no changes since v1) > >> > >>arch/arm/Kconfig | 1 + > >>1 file changed, 1 insertion(+) > >> > >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > >> index d812685c98..fca6ef6d7e 100644 > >> --- a/arch/arm/Kconfig > >> +++ b/arch/arm/Kconfig > >> @@ -1986,6 +1986,7 @@ config ARCH_ROCKCHIP > >> imply CMD_DM > >> imply DEBUG_UART_BOARD_INIT > >> imply BOOTSTD_DEFAULTS > >> + imply BOOTSTD_FULL if BOOTSTD_DEFAULTS > >> imply FAT_WRITE > >> imply SARADC_ROCKCHIP > >> imply SPL_SYSRESET > >> > >> Can this please be merged in ? > >> > >> I wonder if we shouldn't really globally default to BOOTSTD_FULL if > >> BOOTSTD_DEFAULTS for everyone. > >> > >> Its matter of ~21KB in size, unless any platform is really to its > >> limits it should be alright. > >> > >> Maybe I need to re-check things too, since I wonder how much of that > >> growth is re-enabling things that were removed when dropping the DISTRO > >> stuff, and so for platforms just migrating over now it would be smaller > >> in size if much. > >> > >> A board maintainer is free to enable this option, but I don't agree to > >> enable this for everyone. > >> > >> Not like rk3399 and rk3588, some of other SoCs always want a clean and > >> simple but usable U-Boot, > >> > >> eg. rk3036 and rk3308 are still in the list. > >> > >> The discussion is what we consider "usable U-Boot" > >> By default bootstd doesn't have any options and not even to show what > >> it's going to boot. > >> > >> Would it be acceptable if BOOTSTD_FULL is enabled for SoCs rather than > >> boards? > >> > >> Can you please suggest the way forward? > >> Initially the patch was for RockPro64 and then after discussion it was > >> suggested that as BOOTSTD_DEFAULT is very very limited > >> we should enable BOOTSTD_FULL for SoCs that can support multiple boot > >> modes. > >> > >> Let's enable it for RK3588 first and then maybe other SoCs which not code > >> size sensitive. > >> > > My requirement is for RK3399, so I will enable BOOTSTD_FULL for it. > > While at it do you want me to enable it for RK3588 as well ? > > You can add for RK3399 or both RK3399 and RK3588. > Patch v5 [0] has been submitted with RK3399 and RK3588 [0] - https://patchwork.ozlabs.org/project/uboot/patch/20240121220447.663407-...@shantur.com/ Kind regards, Shantur
[PATCH v5] arch: arm: mach-rockchip: Kconfig: Enable BOOTSTD_FULL for RK3399 and RK3588
Rockchip RK3399 and RK3588 SoCs can support wide range of bootflows. Without full bootflow commands, it can be difficult to figure out issues if any, hence enable by default. Reviewed-by: Simon Glass Signed-off-by: Shantur Rathore --- Changes in v5: - Only enable it for RK3399 and RK3588 as suggested by Kever Changes in v4: - Replace BOOTSTD_DEFAULT with BOOTSTD_FULL as previous version didn't apply on next arch/arm/mach-rockchip/Kconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig index 6ff0aa6911..5a46c986ef 100644 --- a/arch/arm/mach-rockchip/Kconfig +++ b/arch/arm/mach-rockchip/Kconfig @@ -265,6 +265,7 @@ config ROCKCHIP_RK3399 imply TPL_TINY_MEMSET imply TPL_ROCKCHIP_COMMON_BOARD imply SYS_BOOTCOUNT_SINGLEWORD if BOOTCOUNT_LIMIT + imply BOOTSTD_FULL imply CMD_BOOTCOUNT if BOOTCOUNT_LIMIT help The Rockchip RK3399 is a ARM-based SoC with a dual-core Cortex-A72 @@ -319,6 +320,7 @@ config ROCKCHIP_RK3588 imply MISC_INIT_R imply CLK_SCMI imply SCMI_FIRMWARE + imply BOOTSTD_FULL help The Rockchip RK3588 is a ARM-based SoC with quad-core Cortex-A76 and quad-core Cortex-A55 including NEON and GPU, 6TOPS NPU, Mali-G610 MP4, -- 2.40.1
Re: [PATCH v3] common: usb-hub: Reset hub port before scanning
On Tue, Jan 16, 2024 at 12:45 AM Marek Vasut wrote:
>
> On 1/8/24 23:11, Shantur Rathore wrote:
> > Hi Marex,
> >
> > On Sat, Dec 9, 2023 at 6:12 PM Shantur Rathore wrote:
> >>
> >> Currently when a hub is turned on, all the ports are powered on.
> >> This works well for hubs which have individual power control.
> >>
> >> For the hubs without individual power control this has no effect.
> >> Mostly in these scenarios the hub port is powered before the USB
> >> controller is enabled, this can lead to some devices in unexpected
> >> state.
> >>
> >> With this patch, we explicitly reset the port while powering up hub
> >> This resets the port for hubs without port power control and has
> >> no effect on hubs with port power control as the port is still off.
> >>
> >> Before this patch AMicro AM8180 based NVME to USB adapter won't be
> >> detected as a USB3.0 Mass Storage device but with this it works as
> >> expected.
> >>
> >> Tested working after this patch:
> >> 1. AMicro AM8180 based NVME to USB Adapter
> >> 2. Kingston DataTraveler 3.0
> >> 3. GenesysLogic USB3.0 Hub
> >>
> >> The drives were tested while connected directly and via the hub.
> >>
> >> Signed-off-by: Shantur Rathore
> >>
> >> ---
> >>
> >> Changes in v3:
> >> - Split up patches as seperate series
> >>
> >> Changes in v2:
> >> - As requested, added fix for regulator-always-on in RockPro64
> >>
> >> common/usb_hub.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/common/usb_hub.c b/common/usb_hub.c
> >> index 70279f301d..3fb7e14d10 100644
> >> --- a/common/usb_hub.c
> >> +++ b/common/usb_hub.c
> >> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device
> >> *hub)
> >>
> >> debug("enabling power on all ports\n");
> >> for (i = 0; i < dev->maxchild; i++) {
> >> + usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
> >> + debug("Reset : port %d returns %lX\n", i + 1, dev->status);
> >> usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
> >> - debug("port %d returns %lX\n", i + 1, dev->status);
> >> + debug("PowerOn : port %d returns %lX\n", i + 1,
> >> dev->status);
> >> }
> >>
> >> #ifdef CONFIG_SANDBOX
> >> --
> >> 2.40.1
> >>
> >
> > Do you think it is the right time to get this in early so it can be
> > tested for a longer time before the next release ?
>
> Yes, I think so, sorry for the delay.
>
> I asked Tom to pick it directly and provided RB .
Thanks Marek,
Just realised Tom isn't included in this chain.
@Tom Rini - Can I request you to please merge this earlier in this release.
Kind regards,
Shantur
Re: [PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
Hi Kever, On Wed, Jan 17, 2024 at 9:18 AM Kever Yang wrote: > > Hi Shantur, > > On 2024/1/17 14:26, Shantur Rathore wrote: > > Hi Kever, > > On Wed, Jan 10, 2024 at 11:58 AM Shantur Rathore wrote: > > Hi Kever, > > On Tue, Jan 9, 2024 at 10:55 AM Kever Yang wrote: > > Hi Shantur, Tom, > > On 2023/12/10 04:45, Tom Rini wrote: > > On Sat, Dec 09, 2023 at 07:49:04PM +, Shantur Rathore wrote: > > On Sat, Dec 9, 2023 at 7:18 PM Tom Rini wrote: > > On Fri, Dec 08, 2023 at 10:52:02AM +, Shantur Rathore wrote: > > Hi Tom / Kever > > On Sun, Nov 19, 2023 at 5:24 PM Shantur Rathore wrote: > > Rockchip SoCs can support wide range of bootflows. > Without full bootflow commands, it can be difficult to > figure out issues if any, hence enable by default. > > Reviewed-by: Simon Glass > > Signed-off-by: Shantur Rathore > --- > > (no changes since v1) > > arch/arm/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index d812685c98..fca6ef6d7e 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -1986,6 +1986,7 @@ config ARCH_ROCKCHIP > imply CMD_DM > imply DEBUG_UART_BOARD_INIT > imply BOOTSTD_DEFAULTS > + imply BOOTSTD_FULL if BOOTSTD_DEFAULTS > imply FAT_WRITE > imply SARADC_ROCKCHIP > imply SPL_SYSRESET > > Can this please be merged in ? > > I wonder if we shouldn't really globally default to BOOTSTD_FULL if > BOOTSTD_DEFAULTS for everyone. > > Its matter of ~21KB in size, unless any platform is really to its > limits it should be alright. > > Maybe I need to re-check things too, since I wonder how much of that > growth is re-enabling things that were removed when dropping the DISTRO > stuff, and so for platforms just migrating over now it would be smaller > in size if much. > > A board maintainer is free to enable this option, but I don't agree to > enable this for everyone. > > Not like rk3399 and rk3588, some of other SoCs always want a clean and > simple but usable U-Boot, > > eg. rk3036 and rk3308 are still in the list. > > The discussion is what we consider "usable U-Boot" > By default bootstd doesn't have any options and not even to show what > it's going to boot. > > Would it be acceptable if BOOTSTD_FULL is enabled for SoCs rather than boards? > > Can you please suggest the way forward? > Initially the patch was for RockPro64 and then after discussion it was > suggested that as BOOTSTD_DEFAULT is very very limited > we should enable BOOTSTD_FULL for SoCs that can support multiple boot modes. > > Let's enable it for RK3588 first and then maybe other SoCs which not code > size sensitive. > My requirement is for RK3399, so I will enable BOOTSTD_FULL for it. While at it do you want me to enable it for RK3588 as well ? Kind regards, Shantur
Re: [PATCH v1] Kconfig: boot: Imply BOOTSTD_DEFAULT when BOOTSTD_FULL=y
On Sat, Dec 23, 2023 at 6:53 AM Shantur Rathore wrote: > > We need BOOTSTD_DEFAULT when BOOTSTD_FULL is selected. > > Signed-off-by: Shantur Rathore > --- > > boot/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/boot/Kconfig b/boot/Kconfig > index 9f5b8a0cb2..fc96aadb27 100644 > --- a/boot/Kconfig > +++ b/boot/Kconfig > @@ -426,6 +426,7 @@ config VPL_BOOTSTD > config BOOTSTD_FULL > bool "Enhanced features for standard boot" > default y if SANDBOX > + imply BOOTSTD_DEFAULTS > help > This enables various useful features for standard boot, which are > not > essential for operation: > -- > 2.40.1 > Ping
Re: [PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
Hi Kever, On Wed, Jan 10, 2024 at 11:58 AM Shantur Rathore wrote: > > Hi Kever, > > On Tue, Jan 9, 2024 at 10:55 AM Kever Yang wrote: > > > > Hi Shantur, Tom, > > > > On 2023/12/10 04:45, Tom Rini wrote: > > > On Sat, Dec 09, 2023 at 07:49:04PM +, Shantur Rathore wrote: > > >> On Sat, Dec 9, 2023 at 7:18 PM Tom Rini wrote: > > >>> On Fri, Dec 08, 2023 at 10:52:02AM +, Shantur Rathore wrote: > > >>> > > >>>> Hi Tom / Kever > > >>>> > > >>>> On Sun, Nov 19, 2023 at 5:24 PM Shantur Rathore > > >>>> wrote: > > >>>>> Rockchip SoCs can support wide range of bootflows. > > >>>>> Without full bootflow commands, it can be difficult to > > >>>>> figure out issues if any, hence enable by default. > > >>>>> > > >>>>> Reviewed-by: Simon Glass > > >>>>> > > >>>>> Signed-off-by: Shantur Rathore > > >>>>> --- > > >>>>> > > >>>>> (no changes since v1) > > >>>>> > > >>>>> arch/arm/Kconfig | 1 + > > >>>>> 1 file changed, 1 insertion(+) > > >>>>> > > >>>>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > >>>>> index d812685c98..fca6ef6d7e 100644 > > >>>>> --- a/arch/arm/Kconfig > > >>>>> +++ b/arch/arm/Kconfig > > >>>>> @@ -1986,6 +1986,7 @@ config ARCH_ROCKCHIP > > >>>>> imply CMD_DM > > >>>>> imply DEBUG_UART_BOARD_INIT > > >>>>> imply BOOTSTD_DEFAULTS > > >>>>> + imply BOOTSTD_FULL if BOOTSTD_DEFAULTS > > >>>>> imply FAT_WRITE > > >>>>> imply SARADC_ROCKCHIP > > >>>>> imply SPL_SYSRESET > > >>>> Can this please be merged in ? > > >>> I wonder if we shouldn't really globally default to BOOTSTD_FULL if > > >>> BOOTSTD_DEFAULTS for everyone. > > >>> > > >> Its matter of ~21KB in size, unless any platform is really to its > > >> limits it should be alright. > > > Maybe I need to re-check things too, since I wonder how much of that > > > growth is re-enabling things that were removed when dropping the DISTRO > > > stuff, and so for platforms just migrating over now it would be smaller > > > in size if much. > > > > A board maintainer is free to enable this option, but I don't agree to > > enable this for everyone. > > > > Not like rk3399 and rk3588, some of other SoCs always want a clean and > > simple but usable U-Boot, > > > > eg. rk3036 and rk3308 are still in the list. > > > > The discussion is what we consider "usable U-Boot" > By default bootstd doesn't have any options and not even to show what > it's going to boot. > > Would it be acceptable if BOOTSTD_FULL is enabled for SoCs rather than boards? > Can you please suggest the way forward? Initially the patch was for RockPro64 and then after discussion it was suggested that as BOOTSTD_DEFAULT is very very limited we should enable BOOTSTD_FULL for SoCs that can support multiple boot modes. Kind regards, Shantur
Re: [PATCH v3] common: usb-hub: Reset hub port before scanning
On Mon, Jan 8, 2024 at 10:11 PM Shantur Rathore wrote:
>
> Hi Marex,
>
> On Sat, Dec 9, 2023 at 6:12 PM Shantur Rathore wrote:
> >
> > Currently when a hub is turned on, all the ports are powered on.
> > This works well for hubs which have individual power control.
> >
> > For the hubs without individual power control this has no effect.
> > Mostly in these scenarios the hub port is powered before the USB
> > controller is enabled, this can lead to some devices in unexpected
> > state.
> >
> > With this patch, we explicitly reset the port while powering up hub
> > This resets the port for hubs without port power control and has
> > no effect on hubs with port power control as the port is still off.
> >
> > Before this patch AMicro AM8180 based NVME to USB adapter won't be
> > detected as a USB3.0 Mass Storage device but with this it works as
> > expected.
> >
> > Tested working after this patch:
> > 1. AMicro AM8180 based NVME to USB Adapter
> > 2. Kingston DataTraveler 3.0
> > 3. GenesysLogic USB3.0 Hub
> >
> > The drives were tested while connected directly and via the hub.
> >
> > Signed-off-by: Shantur Rathore
> >
> > ---
> >
> > Changes in v3:
> > - Split up patches as seperate series
> >
> > Changes in v2:
> > - As requested, added fix for regulator-always-on in RockPro64
> >
> > common/usb_hub.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/usb_hub.c b/common/usb_hub.c
> > index 70279f301d..3fb7e14d10 100644
> > --- a/common/usb_hub.c
> > +++ b/common/usb_hub.c
> > @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device
> > *hub)
> >
> > debug("enabling power on all ports\n");
> > for (i = 0; i < dev->maxchild; i++) {
> > + usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
> > + debug("Reset : port %d returns %lX\n", i + 1, dev->status);
> > usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
> > - debug("port %d returns %lX\n", i + 1, dev->status);
> > + debug("PowerOn : port %d returns %lX\n", i + 1,
> > dev->status);
> > }
> >
> > #ifdef CONFIG_SANDBOX
> > --
> > 2.40.1
> >
>
> Do you think it is the right time to get this in early so it can be
> tested for a longer time before the next release ?
>
> Kind regards,
> Shantur
Ping.
Re: [PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
Hi Kever, On Tue, Jan 9, 2024 at 10:55 AM Kever Yang wrote: > > Hi Shantur, Tom, > > On 2023/12/10 04:45, Tom Rini wrote: > > On Sat, Dec 09, 2023 at 07:49:04PM +0000, Shantur Rathore wrote: > >> On Sat, Dec 9, 2023 at 7:18 PM Tom Rini wrote: > >>> On Fri, Dec 08, 2023 at 10:52:02AM +, Shantur Rathore wrote: > >>> > >>>> Hi Tom / Kever > >>>> > >>>> On Sun, Nov 19, 2023 at 5:24 PM Shantur Rathore wrote: > >>>>> Rockchip SoCs can support wide range of bootflows. > >>>>> Without full bootflow commands, it can be difficult to > >>>>> figure out issues if any, hence enable by default. > >>>>> > >>>>> Reviewed-by: Simon Glass > >>>>> > >>>>> Signed-off-by: Shantur Rathore > >>>>> --- > >>>>> > >>>>> (no changes since v1) > >>>>> > >>>>> arch/arm/Kconfig | 1 + > >>>>> 1 file changed, 1 insertion(+) > >>>>> > >>>>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > >>>>> index d812685c98..fca6ef6d7e 100644 > >>>>> --- a/arch/arm/Kconfig > >>>>> +++ b/arch/arm/Kconfig > >>>>> @@ -1986,6 +1986,7 @@ config ARCH_ROCKCHIP > >>>>> imply CMD_DM > >>>>> imply DEBUG_UART_BOARD_INIT > >>>>> imply BOOTSTD_DEFAULTS > >>>>> + imply BOOTSTD_FULL if BOOTSTD_DEFAULTS > >>>>> imply FAT_WRITE > >>>>> imply SARADC_ROCKCHIP > >>>>> imply SPL_SYSRESET > >>>> Can this please be merged in ? > >>> I wonder if we shouldn't really globally default to BOOTSTD_FULL if > >>> BOOTSTD_DEFAULTS for everyone. > >>> > >> Its matter of ~21KB in size, unless any platform is really to its > >> limits it should be alright. > > Maybe I need to re-check things too, since I wonder how much of that > > growth is re-enabling things that were removed when dropping the DISTRO > > stuff, and so for platforms just migrating over now it would be smaller > > in size if much. > > A board maintainer is free to enable this option, but I don't agree to > enable this for everyone. > > Not like rk3399 and rk3588, some of other SoCs always want a clean and > simple but usable U-Boot, > > eg. rk3036 and rk3308 are still in the list. > The discussion is what we consider "usable U-Boot" By default bootstd doesn't have any options and not even to show what it's going to boot. Would it be acceptable if BOOTSTD_FULL is enabled for SoCs rather than boards? Kind regards, Shantur
Re: [PATCH v3] common: usb-hub: Reset hub port before scanning
Hi Marex,
On Sat, Dec 9, 2023 at 6:12 PM Shantur Rathore wrote:
>
> Currently when a hub is turned on, all the ports are powered on.
> This works well for hubs which have individual power control.
>
> For the hubs without individual power control this has no effect.
> Mostly in these scenarios the hub port is powered before the USB
> controller is enabled, this can lead to some devices in unexpected
> state.
>
> With this patch, we explicitly reset the port while powering up hub
> This resets the port for hubs without port power control and has
> no effect on hubs with port power control as the port is still off.
>
> Before this patch AMicro AM8180 based NVME to USB adapter won't be
> detected as a USB3.0 Mass Storage device but with this it works as
> expected.
>
> Tested working after this patch:
> 1. AMicro AM8180 based NVME to USB Adapter
> 2. Kingston DataTraveler 3.0
> 3. GenesysLogic USB3.0 Hub
>
> The drives were tested while connected directly and via the hub.
>
> Signed-off-by: Shantur Rathore
>
> ---
>
> Changes in v3:
> - Split up patches as seperate series
>
> Changes in v2:
> - As requested, added fix for regulator-always-on in RockPro64
>
> common/usb_hub.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 70279f301d..3fb7e14d10 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
>
> debug("enabling power on all ports\n");
> for (i = 0; i < dev->maxchild; i++) {
> + usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
> + debug("Reset : port %d returns %lX\n", i + 1, dev->status);
> usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
> - debug("port %d returns %lX\n", i + 1, dev->status);
> + debug("PowerOn : port %d returns %lX\n", i + 1, dev->status);
> }
>
> #ifdef CONFIG_SANDBOX
> --
> 2.40.1
>
Do you think it is the right time to get this in early so it can be
tested for a longer time before the next release ?
Kind regards,
Shantur
[PATCH v1] Kconfig: boot: Imply BOOTSTD_DEFAULT when BOOTSTD_FULL=y
We need BOOTSTD_DEFAULT when BOOTSTD_FULL is selected. Signed-off-by: Shantur Rathore --- boot/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/boot/Kconfig b/boot/Kconfig index 9f5b8a0cb2..fc96aadb27 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -426,6 +426,7 @@ config VPL_BOOTSTD config BOOTSTD_FULL bool "Enhanced features for standard boot" default y if SANDBOX + imply BOOTSTD_DEFAULTS help This enables various useful features for standard boot, which are not essential for operation: -- 2.40.1
Re: [PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
Hi, On Fri, Dec 22, 2023 at 12:09 PM Shantur Rathore wrote: > > On Fri, Dec 22, 2023 at 12:07 PM Tom Rini wrote: > > > > On Fri, Dec 22, 2023 at 12:05:39PM +0000, Shantur Rathore wrote: > > > Hi all, > > > > > > On Mon, Dec 11, 2023 at 6:27 PM Tom Rini wrote: > > > > > > > > On Mon, Dec 11, 2023 at 11:22:45AM -0700, Simon Glass wrote: > > > > > Hi Tom, > > > > [snip] > > > > > > I think in hind-sight too much stuff is omitted without > > > > > > BOOTSTD_FULL. > > > > > > The option itself then enables other stuff too by default, but some > > > > > > parts of the bootflow command itself should be visible even without > > > > > > FULL > > > > > > to make things easier on the user. > > > > > > > > > > At the time the goal was to avoid growth compared to the distro > > > > > scripts. We could perhaps add some more things in with BOOTSTD_FULL > > > > > but still have it as an option? > > > > > > > > Right. But now that we've tried this, some of the feedback has been that > > > > it's just too minimal right now. Like looking at the help message for > > > > bootflow, list and info should probably always be available. And maybe > > > > the flags for "scan" should be re-thought? Too late to change things now > > > > but "bootflow scan -b" should maybe how it's always been for "scan and > > > > boot". > > > > > > What would be the preferred approach for this patch? > > > Is it to update the default capabilities of bootflow or we can have this > > > patch? > > > > Well, I don't see a problem with just adding this to the platforms which > > enable BOOTSTD_FULL until we can rework what's included/excluded by that > > flag, and there's an issue filed for it now. > > > > -- > > Tom > > Great, can we have this merged in please then [0] > > [0] - > https://patchwork.ozlabs.org/project/uboot/patch/20231119172310.1307942-...@shantur.com/ > I tried this patch over the next branch and it failed to build. Sent v4 here : https://patchwork.ozlabs.org/project/uboot/patch/2023134358.563121-...@shantur.com/ Regards, Shantur
[PATCH v4] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
Rockchip SoCs can support wide range of bootflows. Without full bootflow commands, it can be difficult to figure out issues if any, hence enable by default. Reviewed-by: Simon Glass Signed-off-by: Shantur Rathore --- Changes in v4: - Replace BOOTSTD_DEFAULT with BOOTSTD_FULL as previous version didn't work on next arch/arm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index d812685c98..a0265b36ad 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1985,7 +1985,7 @@ config ARCH_ROCKCHIP imply ADC imply CMD_DM imply DEBUG_UART_BOARD_INIT - imply BOOTSTD_DEFAULTS + imply BOOTSTD_FULL imply FAT_WRITE imply SARADC_ROCKCHIP imply SPL_SYSRESET -- 2.40.1
[PATCH v4] dts: rockpro64: Disable usb regulators-always-on
USB port regulators should be controlled by PHYs
so we remove always-on property and let phy manage the
regulator.
phy-supply isn't configured for TypeC port in upstream and
now that we are removing always-on, we need to add the
phy-supply until its fixed upstream.
Signed-off-by: Shantur Rathore
---
Changes in v4:
- otg port must be configured with phy-supply instead of host port node
Changes in v3:
- Split up patches as seperate series
Changes in v2:
- As requested, added fix for regulator-always-on in RockPro64
arch/arm/dts/rk3399-rockpro64-u-boot.dtsi | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
index 732727d9b0..0ac9fa9a03 100644
--- a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
@@ -22,6 +22,18 @@
};
};
+_otg {
+ phy-supply = <_typec>;
+};
+
+_host {
+ /delete-property/ regulator-always-on;
+};
+
+_typec {
+ /delete-property/ regulator-always-on;
+};
+
_center {
regulator-min-microvolt = <95>;
regulator-max-microvolt = <95>;
--
2.40.1
Re: [PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
On Fri, Dec 22, 2023 at 12:07 PM Tom Rini wrote: > > On Fri, Dec 22, 2023 at 12:05:39PM +, Shantur Rathore wrote: > > Hi all, > > > > On Mon, Dec 11, 2023 at 6:27 PM Tom Rini wrote: > > > > > > On Mon, Dec 11, 2023 at 11:22:45AM -0700, Simon Glass wrote: > > > > Hi Tom, > > > [snip] > > > > > I think in hind-sight too much stuff is omitted without BOOTSTD_FULL. > > > > > The option itself then enables other stuff too by default, but some > > > > > parts of the bootflow command itself should be visible even without > > > > > FULL > > > > > to make things easier on the user. > > > > > > > > At the time the goal was to avoid growth compared to the distro > > > > scripts. We could perhaps add some more things in with BOOTSTD_FULL > > > > but still have it as an option? > > > > > > Right. But now that we've tried this, some of the feedback has been that > > > it's just too minimal right now. Like looking at the help message for > > > bootflow, list and info should probably always be available. And maybe > > > the flags for "scan" should be re-thought? Too late to change things now > > > but "bootflow scan -b" should maybe how it's always been for "scan and > > > boot". > > > > What would be the preferred approach for this patch? > > Is it to update the default capabilities of bootflow or we can have this > > patch? > > Well, I don't see a problem with just adding this to the platforms which > enable BOOTSTD_FULL until we can rework what's included/excluded by that > flag, and there's an issue filed for it now. > > -- > Tom Great, can we have this merged in please then [0] [0] - https://patchwork.ozlabs.org/project/uboot/patch/20231119172310.1307942-...@shantur.com/ Regards, Shantur
Re: [PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
Hi all, On Mon, Dec 11, 2023 at 6:27 PM Tom Rini wrote: > > On Mon, Dec 11, 2023 at 11:22:45AM -0700, Simon Glass wrote: > > Hi Tom, > [snip] > > > I think in hind-sight too much stuff is omitted without BOOTSTD_FULL. > > > The option itself then enables other stuff too by default, but some > > > parts of the bootflow command itself should be visible even without FULL > > > to make things easier on the user. > > > > At the time the goal was to avoid growth compared to the distro > > scripts. We could perhaps add some more things in with BOOTSTD_FULL > > but still have it as an option? > > Right. But now that we've tried this, some of the feedback has been that > it's just too minimal right now. Like looking at the help message for > bootflow, list and info should probably always be available. And maybe > the flags for "scan" should be re-thought? Too late to change things now > but "bootflow scan -b" should maybe how it's always been for "scan and > boot". What would be the preferred approach for this patch? Is it to update the default capabilities of bootflow or we can have this patch? Thanks Shantur > > -- > Tom
Re: [tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Hi Tom,
On Mon, Dec 18, 2023 at 4:26 PM Tom Rini wrote:
>
> Here's the latest report.
>
> -- Forwarded message -
> From:
> Date: Mon, Dec 18, 2023 at 8:42 AM
> Subject: New Defects reported by Coverity Scan for Das U-Boot
> To:
>
>
> Hi,
>
> Please find the latest report on new defect(s) introduced to Das
> U-Boot found with Coverity Scan.
>
> 1 new defect(s) introduced to Das U-Boot found with Coverity Scan.
>
>
> New defect(s) Reported-by: Coverity Scan
> Showing 1 of 1 defect(s)
>
>
> ** CID 470930: Uninitialized variables (UNINIT)
> /boot/bootmeth_efi.c: 465 in distro_efi_boot()
>
>
>
> *** CID 470930: Uninitialized variables (UNINIT)
> /boot/bootmeth_efi.c: 465 in distro_efi_boot()
> 459 */
> 460 if (bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT) {
> 461 log_debug("Booting with built-in fdt\n");
> 462 snprintf(cmd, sizeof(cmd), "bootefi %lx", kernel);
> 463 } else {
> 464 log_debug("Booting with external fdt\n");
> >>> CID 470930: Uninitialized variables (UNINIT)
> >>> Using uninitialized value "fdt" when calling "snprintf". [Note: The
> >>> source code implementation of the function has been overridden by a
> >>> builtin model.]
> 465 snprintf(cmd, sizeof(cmd), "bootefi %lx %lx",
> kernel, fdt);
> 466 }
> 467
> 468 if (run_command(cmd, 0))
> 469 return log_msg_ret("run", -EINVAL);
> 470
>
The code in question is
if (!bootmeth_uses_network(bflow)) {
// snip
if (bflow->flags & ~BOOTFLOWF_USE_BUILTIN_FDT)
fdt = bflow->fdt_addr;
} else {
// snip
fdt = env_get_hex("fdt_addr_r", 0);
}
//snip
if (bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT) {
log_debug("Booting with built-in fdt\n");
snprintf(cmd, sizeof(cmd), "bootefi %lx", kernel);
} else {
log_debug("Booting with external fdt\n");
snprintf(cmd, sizeof(cmd), "bootefi %lx %lx", kernel, fdt);
}
I am unsure in which case is fdt uninitialised.
Unless the tool is being thrown off by different version of
bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT
check, I don't see the problem.
Please let me know if you see it.
Kind regards,
Shantur
>
> --
> Tom
Re: Adding EFI runtime support to the Arm's FF-A bus
Hi Ilias, On Thu, Dec 21, 2023 at 6:30 AM Ilias Apalodimas wrote: > > Hi Shantur > > On Thu, 21 Dec 2023 at 00:57, Shantur Rathore wrote: > > > > Hi Illias, > > > > On Wed, Dec 20, 2023 at 3:43 PM Peter Robinson wrote: > > > > > > On Wed, Dec 20, 2023 at 6:37 AM Ilias Apalodimas > > > wrote: > > > > > > > > Hi Michael > > > > > > > > On Tue, 19 Dec 2023 at 14:47, Michael Walle wrote: > > > > > > > > > > Hi Mark, > > > > > > > > > > >> > Any runtime device drivers for variable storage should not be in > > > > > >> > the > > > > > >> > U-Boot runtime but live in the secure world (e.g. OP-TEE) FF-A > > > > > >> > is the > > > > > >> > new ARM protocol for talking to the secure world and hence fits > > > > > >> > into > > > > > >> > the picture. > > > > > >> > > > > > >> What if I just want a simple embedded boot stack where I don't > > > > > >> want any secure world and just want to be able to boot a COTS linux > > > > > >> distribution via EFI? > > > > > > > > > > > > That already works for many Linux distros. As long as the distro > > > > > > installs the appropriate BOOTxxx.EFI file you don't actually need to > > > > > > set any EFI variables for the OS to boot. It can't get any simpler > > > > > > than that. Of the main Linux distros it seems that only Debian > > > > > > doesn't do this. Someone should probably lobby Debian to do this as > > > > > > well as it would mean that Debian would just work on an EBBR > > > > > > compliant > > > > > > system. > > > > > > > > > > I know. Last time I checked CentOS (or was it Ubuntu?) tried to > > > > > set EFI variables and the installer just failed. Might be fixed now, > > > > > though. > > > > > > > > It's not. The error message is less scary in distros nowadays, but > > > > setting the variable for Boot is still not working. That being > > > > said I have a plan to fix it for variables stored in a file, that's > > > > why we standardized the efi variable format in EBBR. > > > > Does this mean supporting SetVariableRT for files ? > > If yes, would it work without the driver model in runtime? > > Yes. > > The reasoning here is pretty simple. You can't keep alive drivers etc > for devices that the *kernel* eventually owns. The proposal is to > ignore the EFI spec and teach the kernel to write those directly. We > already do that when the variables are stored in an RPMB, we need to > figure out a decent scalable architecture for the rest. > Thanks for explaining this. It would be a good idea to provide EFI var storage info like location, offset, maxsize to linux and linux can modify vars as required. Eg. location=espfile://u-boot-efi.vars offset=0 maxsize=-1 or location=spi://0 offset=0x3D maxsize=0x Then the linux kernel is able to modify vars correctly. I think SPI might be simpler to implement vs file as there can be. many different ESP partitions on multiple drives or no ESP partition at all. Kind regards, Shantur > /Ilias > > > > Kind regards, > > Shantur
Re: Adding EFI runtime support to the Arm's FF-A bus
Hi Illias, On Wed, Dec 20, 2023 at 3:43 PM Peter Robinson wrote: > > On Wed, Dec 20, 2023 at 6:37 AM Ilias Apalodimas > wrote: > > > > Hi Michael > > > > On Tue, 19 Dec 2023 at 14:47, Michael Walle wrote: > > > > > > Hi Mark, > > > > > > >> > Any runtime device drivers for variable storage should not be in the > > > >> > U-Boot runtime but live in the secure world (e.g. OP-TEE) FF-A is the > > > >> > new ARM protocol for talking to the secure world and hence fits into > > > >> > the picture. > > > >> > > > >> What if I just want a simple embedded boot stack where I don't > > > >> want any secure world and just want to be able to boot a COTS linux > > > >> distribution via EFI? > > > > > > > > That already works for many Linux distros. As long as the distro > > > > installs the appropriate BOOTxxx.EFI file you don't actually need to > > > > set any EFI variables for the OS to boot. It can't get any simpler > > > > than that. Of the main Linux distros it seems that only Debian > > > > doesn't do this. Someone should probably lobby Debian to do this as > > > > well as it would mean that Debian would just work on an EBBR compliant > > > > system. > > > > > > I know. Last time I checked CentOS (or was it Ubuntu?) tried to > > > set EFI variables and the installer just failed. Might be fixed now, > > > though. > > > > It's not. The error message is less scary in distros nowadays, but > > setting the variable for Boot is still not working. That being > > said I have a plan to fix it for variables stored in a file, that's > > why we standardized the efi variable format in EBBR. Does this mean supporting SetVariableRT for files ? If yes, would it work without the driver model in runtime? Kind regards, Shantur
Re: [PATCH v4] arm: dts: rockpro64: Add RockPro64 smbios
Hi,
On Wed, Dec 13, 2023 at 8:41 PM Simon Glass wrote:
>
> Hi Tom,
>
> On Wed, 13 Dec 2023 at 13:29, Tom Rini wrote:
> >
> > On Wed, Dec 13, 2023 at 12:50:06PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 11 Dec 2023 at 13:53, Tom Rini wrote:
> > > >
> > > > On Mon, Dec 11, 2023 at 08:42:19PM +, Shantur Rathore wrote:
> > > > > Hi,
> > > > >
> > > > > On Mon, Nov 13, 2023 at 11:24 AM Shantur Rathore
> > > > > wrote:
> > > > > >
> > > > > > Add smbios information for Pine64 RockPro64 board and enable in
> > > > > > config
> > > > > >
> > > > > > Signed-off-by: Shantur Rathore
> > > > > > ---
> > > > > > Changes
> > > > > > v4: Change PINE64 to Pine64
> > > > > > v3: Enable SYSINFO and SYSINFO_SMBIOS in defconfig
> > > > > >
> > > > > > arch/arm/dts/rk3399-rockpro64-u-boot.dtsi | 22
> > > > > > ++
> > > > > > configs/rockpro64-rk3399_defconfig| 2 ++
> > > > > > 2 files changed, 24 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> > > > > > b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> > > > > > index 732727d9b0..089732524a 100644
> > > > > > --- a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> > > > > > +++ b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> > > > > > @@ -9,6 +9,28 @@
> > > > > > chosen {
> > > > > > u-boot,spl-boot-order = "same-as-spl", _flash,
> > > > > > ,
> > > > > > };
> > > > > > +
> > > > > > +smbios {
> > > > > > +compatible = "u-boot,sysinfo-smbios";
> > > > > > +smbios {
> > > > > > +system {
> > > > > > +manufacturer = "Pine64";
> > > > > > +product = "RockPro64";
> > > > > > +};
> > [snip]
> > > > Yes, we should just defer this and pickup the SMBIOS series that Ilias
> > > > has posted.
> > >
> > > I don't think it is a suitable substitute, it is just a fallback.
> > >
> > > So I believe this patch should be applied.
> >
> > Please note that this patch is adding _less_ details than the top-level
> > model field contains today for the platform. The only difference is
> > "Pine64" vs "pine64".
>
> The top-level model is "Pine64 RockPro64 v2.1", I believe. But:
>
> - the first part of that is the manufacturer, not the product
> - the second part is what we have in this patch
> - the third part is the version
>
> SMBIOS tries to split things up into separate fields.
>
> So, perhaps a new version of this patch could add:
>
>system {
> version = "2.1";
>};
>
I can add the above or any more details needed to a new patch
Will that be enough to make it merge-able
Kind regards,
Shantur
Re: [PATCH] maintainers: rk3399: remove maintainer
On Wed, Dec 13, 2023 at 11:06 PM Tom Rini wrote: > > On Fri, Nov 24, 2023 at 12:04:31PM +, Shantur Rathore wrote: > > > Remove Akash Gajjar from > > MAINTAINERS as email is bouncing. > > > > Signed-off-by: Shantur Rathore > > Reviewed-by: Kever Yang > > Applied to u-boot/master, thanks! Great, thanks > > -- > Tom
Re: [PATCH v4] arm: dts: rockpro64: Add RockPro64 smbios
Hi,
On Mon, Nov 13, 2023 at 11:24 AM Shantur Rathore wrote:
>
> Add smbios information for Pine64 RockPro64 board and enable in
> config
>
> Signed-off-by: Shantur Rathore
> ---
> Changes
> v4: Change PINE64 to Pine64
> v3: Enable SYSINFO and SYSINFO_SMBIOS in defconfig
>
> arch/arm/dts/rk3399-rockpro64-u-boot.dtsi | 22 ++
> configs/rockpro64-rk3399_defconfig| 2 ++
> 2 files changed, 24 insertions(+)
>
> diff --git a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> index 732727d9b0..089732524a 100644
> --- a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> @@ -9,6 +9,28 @@
> chosen {
> u-boot,spl-boot-order = "same-as-spl", _flash, ,
>
> };
> +
> +smbios {
> +compatible = "u-boot,sysinfo-smbios";
> +smbios {
> +system {
> +manufacturer = "Pine64";
> +product = "RockPro64";
> +};
> +
> +baseboard {
> +manufacturer = "Pine64";
> +product = "RockPro64";
> +};
> +
> +chassis {
> +manufacturer = "Pine64";
> +product = "RockPro64";
> +};
> +};
> +};
> +
> +
> };
>
> {
> diff --git a/configs/rockpro64-rk3399_defconfig
> b/configs/rockpro64-rk3399_defconfig
> index 4cd6b76665..affb6137e0 100644
> --- a/configs/rockpro64-rk3399_defconfig
> +++ b/configs/rockpro64-rk3399_defconfig
> @@ -90,6 +90,8 @@ CONFIG_BAUDRATE=150
> CONFIG_DEBUG_UART_SHIFT=2
> CONFIG_SYS_NS16550_MEM32=y
> CONFIG_ROCKCHIP_SPI=y
> +CONFIG_SYSINFO=y
> +CONFIG_SYSINFO_SMBIOS=y
> CONFIG_SYSRESET=y
> CONFIG_USB=y
> CONFIG_USB_XHCI_HCD=y
> --
> 2.40.1
>
Can this please be merged if no changes are required?
[0] -
https://patchwork.ozlabs.org/project/uboot/patch/20231113112309.730323-...@shantur.com/
Kind regards,
Shantur
Re: [PATCH v2 2/2] dts: rockpro64: Disable usb regulators-always-on
On Sat, Dec 9, 2023 at 10:20 PM Shantur Rathore wrote: > > On Sat, Dec 9, 2023 at 10:05 PM Marek Vasut wrote: > > > > On 12/9/23 18:57, Shantur Rathore wrote: > > > Hi Marek, > > > > > > On Sat, Dec 9, 2023 at 5:31 PM Marek Vasut wrote: > > >> > > >> On 12/8/23 01:09, Shantur Rathore wrote: > > >>> USB port regulators should be controlled by phys > > >> > > >> PHYs > > >> > > >>> so we remove always-on property and let phy manage the > > >>> regulator. > > >>> > > >>> Typec port has misconfugred > > >> > > >> misconfigured (typo) > > > > > > Thanks will fix both typos in v3 > > > > > >> > > >>> phy-supply in upstream and > > >>> now that we are removing always-on, we need to fix the > > >>> phy-supply until its fixed upstream. > > >> > > >> Where is the upstream Linux kernel patch ? > > >> Was it submitted already ? > > >> > > > > > > No, there is no patch yet for this in Linux upstream. > > > > Why not ? I would expect the fix should be consistent between both > > U-Boot and Linux . > > Yes you are right. I will be raising a patch for linux too. This has been published to devicetree - https://lore.kernel.org/linux-devicetree/20231209233536.350876-...@shantur.com/
Re: bootstd: Support for distro specific EFI folders
On Sat, Dec 9, 2023 at 4:26 PM Shantur Rathore wrote: > > Hi Peter, > > On Wed, Dec 6, 2023 at 12:16 PM Peter Robinson wrote: > > > > On Sun, Nov 19, 2023 at 6:55 PM Mark Kettenis > > wrote: > > > > > > > Date: Sat, 18 Nov 2023 23:52:11 +0100 > > > > From: Heinrich Schuchardt > > > > > > Hi Heinrich, > > > > > > > On 11/18/23 22:28, Shantur Rathore wrote: > > > > > Hi Heinrich, > > > > > > > > > > On Fri, Nov 17, 2023 at 3:12 PM Heinrich Schuchardt > > > > > wrote: > > > > >> > > > > >> On 11/16/23 19:45, Shantur Rathore wrote: > > > > >>> On Thu, Nov 16, 2023 at 6:15 PM Heinrich Schuchardt > > > > >>> wrote: > > > > >>>> > > > > >>>> On 11/16/23 17:52, Shantur Rathore wrote: > > > > >>>>> Hi Simon, > > > > >>>>> > > > > >>>>> Currently bootstd - bootmethod_efi only looks for the default > > > > >>>>> bootxx64.efi in /EFI/boot folder only. > > > > >>>>> Generally many distros end up putting their bootloaders in > > > > >>>>> EFI/ folders like EFI/ubuntu and EFI/debian etc. > > > > >>>>> > > > > >>>>> In x86 worlds, the NVRAM is modified and new boot entries are > > > > >>>>> added to > > > > >>>>> support these but in the U-boot world the NVRAM variables are > > > > >>>>> read-only. > > > > >>>> > > > > >>>> I guess you are referring to UEFI boot options. These typically > > > > >>>> are not > > > > >>>> stored in non-volatile RAM but on a SPI flash device. > > > > >>>> > > > > >>> > > > > >>> Thanks for correcting me. > > > > >>> > > > > >>>>> > > > > >>>>> What would be the best way to implement this? > > > > >>>>> > > > > >>>>> I was thinking of having a "efi_prefixes" environment variable > > > > >>>>> which > > > > >>>>> can be set to "ubuntu debian centos" etc and bootmethod_efi can > > > > >>>>> try > > > > >>>>> all of them. Will bootmethod_efi be able to support multiple > > > > >>>>> entries ( > > > > >>>>> thinking of multiboot ) ? > > > > >>>> > > > > >>>> On my laptop I have: > > > > >>>> > > > > >>>> EFI/Microsoft/Boot/bootmgr.efi > > > > >>>> EFI/Microsoft/Boot/memtest.efi > > > > >>>> EFI/Boot/bootx64.efi > > > > >>>> EFI/Boot/fbx64.efi > > > > >>>> EFI/Boot/mmx64.efi > > > > >>>> EFI/debian/shimx64.efi > > > > >>>> EFI/debian/grubx64.efi > > > > >>>> EFI/debian/mmx64.efi > > > > >>>> EFI/debian/fbx64.efi > > > > >>>> EFI/ubuntu/grubx64.efi > > > > >>>> EFI/ubuntu/shimx64.efi > > > > >>>> EFI/ubuntu/mmx64.efi > > > > >>>> > > > > >>>> Obviously each installed operating system provides multiple EFI > > > > >>>> binaries > > > > >>>> and non uses the fallback file name BOOT.EFI. A value "ubuntu > > > > >>>> debian centos" would not be able to describe which file you are > > > > >>>> looking > > > > >>>> for. > > > > >>>> > > > > >>>> We already have the U-Boot command line eficonfig and efidebug > > > > >>>> commands > > > > >>>> to set up UEFI boot options which can describe which EFI binary to > > > > >>>> load > > > > >>>> and which command line to pass to it. These are considered by the > > > > >>>> existing boot flows. > > > > >>> > > > > >>> So, I am building a new RockPro64 based cluster and using Canonical > > > > >>> MAAS to set them up automatically, booting them up usin
Re: [PATCH v2 2/2] dts: rockpro64: Disable usb regulators-always-on
On Sat, Dec 9, 2023 at 10:05 PM Marek Vasut wrote: > > On 12/9/23 18:57, Shantur Rathore wrote: > > Hi Marek, > > > > On Sat, Dec 9, 2023 at 5:31 PM Marek Vasut wrote: > >> > >> On 12/8/23 01:09, Shantur Rathore wrote: > >>> USB port regulators should be controlled by phys > >> > >> PHYs > >> > >>> so we remove always-on property and let phy manage the > >>> regulator. > >>> > >>> Typec port has misconfugred > >> > >> misconfigured (typo) > > > > Thanks will fix both typos in v3 > > > >> > >>> phy-supply in upstream and > >>> now that we are removing always-on, we need to fix the > >>> phy-supply until its fixed upstream. > >> > >> Where is the upstream Linux kernel patch ? > >> Was it submitted already ? > >> > > > > No, there is no patch yet for this in Linux upstream. > > Why not ? I would expect the fix should be consistent between both > U-Boot and Linux . Yes you are right. I will be raising a patch for linux too.
Re: [PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
On Sat, Dec 9, 2023 at 8:56 PM Tom Rini wrote: > > On Fri, Dec 08, 2023 at 01:59:26PM +, Shantur Rathore wrote: > > Hi Peter, > > > > On Fri, Dec 8, 2023 at 12:59 PM Peter Robinson wrote: > > > > > > On Fri, Dec 8, 2023 at 12:52 PM Shantur Rathore wrote: > > > > > > > > Hi Jagan, > > > > > > > > On Fri, Dec 8, 2023 at 11:13 AM Jagan Teki > > > > wrote: > > > > > > > > > > On Sun, Nov 19, 2023 at 10:54 PM Shantur Rathore > > > > > wrote: > > > > > > > > > > > > Rockchip SoCs can support wide range of bootflows. > > > > > > Without full bootflow commands, it can be difficult to > > > > > > figure out issues if any, hence enable by default. > > > > > > > > > > > > Reviewed-by: Simon Glass > > > > > > > > > > > > Signed-off-by: Shantur Rathore > > > > > > --- > > > > > > > > > > > > (no changes since v1) > > > > > > > > > > > > arch/arm/Kconfig | 1 + > > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > > > > > index d812685c98..fca6ef6d7e 100644 > > > > > > --- a/arch/arm/Kconfig > > > > > > +++ b/arch/arm/Kconfig > > > > > > @@ -1986,6 +1986,7 @@ config ARCH_ROCKCHIP > > > > > > imply CMD_DM > > > > > > imply DEBUG_UART_BOARD_INIT > > > > > > imply BOOTSTD_DEFAULTS > > > > > > + imply BOOTSTD_FULL if BOOTSTD_DEFAULTS > > > > > > > > > > Yes, but better to give this option to specific board vendors as > > > > > defaults are enough to boot 1st bootflow and what ever media's it has. > > > > > > > > > > > > > Yes, that's correct it is enough to boot but by default there is no > > > > option > > > > to choose what to boot from. > > > > This was discussed in an earlier version of this patch [0] where I was > > > > explicitly enabling only for RP64. > > > > > > What actual extra functionality does this provide, what is the impact > > > on the size of images? You've not provided reasonable justification > > > outside of very vague statements, it would be useful to know what the > > > added options actually solves. > > > > BOOTSTD_FULL enables all the options in bootflow commands. This is needed if > > - you want to choose between multiple available bootflows rather than > > just boot one default. > > - if you need to list the available bootflows that bootstd has found > > - if you need to select and boot any bootflow other than default. > > By default all other commands in U-boot come with options to show details > > For example, nvme info, nvme detail, usb info, usb tree but with > > bootstd no way to know anything. > > > > Image size - u-boot.itd without BOOTSTD_FULL - 1193984 bytes > > Image size - u-boot.itb with BOOTSTD_FULL - 1214976 bytes > > Difference - 20992 bytes > > > > According to binman for RK3399 u-boot can take upto 4M [1] so we have > > ample space. > > > > This was discussed in the previous patch in the link below [0] > > > > [0] - > > https://patchwork.ozlabs.org/project/uboot/patch/2023001329.537704-...@shantur.com/ > > [1] - > > https://github.com/shantur/u-boot/blob/master/arch/arm/dts/rk3399-u-boot.dtsi#L68 > > If I'm recalling everything right, this also brings "bootflow" as a > command more in line with what could be done with distro_bootcmd in > terms of "cover every possible case and let the user override things" > Yes, That's correct. U_BOOT_LONGHELP(bootflow, #ifdef CONFIG_CMD_BOOTFLOW_FULL "scan [-abeGl] [bdev] - scan for valid bootflows (-l list, -a all, -e errors, -b boot, -G no global)\n" "bootflow list [-e] - list scanned bootflows (-e errors)\n" "bootflow select [|] - select a bootflow\n" "bootflow info [-ds]- show info on current bootflow (-d dump bootflow)\n" "bootflow read - read all current-bootflow files\n" "bootflow boot - boot current bootflow\n" "bootflow menu [-t] - show a menu of available bootflows\n" "bootflow cmdline [set|get|clear|delete|auto] [] - update cmdline" #else "scan - boot first available bootflow\n" #endif );
Re: [PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
On Sat, Dec 9, 2023 at 7:18 PM Tom Rini wrote: > > On Fri, Dec 08, 2023 at 10:52:02AM +, Shantur Rathore wrote: > > > Hi Tom / Kever > > > > On Sun, Nov 19, 2023 at 5:24 PM Shantur Rathore wrote: > > > > > > Rockchip SoCs can support wide range of bootflows. > > > Without full bootflow commands, it can be difficult to > > > figure out issues if any, hence enable by default. > > > > > > Reviewed-by: Simon Glass > > > > > > Signed-off-by: Shantur Rathore > > > --- > > > > > > (no changes since v1) > > > > > > arch/arm/Kconfig | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > > index d812685c98..fca6ef6d7e 100644 > > > --- a/arch/arm/Kconfig > > > +++ b/arch/arm/Kconfig > > > @@ -1986,6 +1986,7 @@ config ARCH_ROCKCHIP > > > imply CMD_DM > > > imply DEBUG_UART_BOARD_INIT > > > imply BOOTSTD_DEFAULTS > > > + imply BOOTSTD_FULL if BOOTSTD_DEFAULTS > > > imply FAT_WRITE > > > imply SARADC_ROCKCHIP > > > imply SPL_SYSRESET > > > > Can this please be merged in ? > > I wonder if we shouldn't really globally default to BOOTSTD_FULL if > BOOTSTD_DEFAULTS for everyone. > Its matter of ~21KB in size, unless any platform is really to its limits it should be alright. Regards, Shantur
Re: [PATCH v2 2/2] dts: rockpro64: Disable usb regulators-always-on
Hi Marek, On Sat, Dec 9, 2023 at 5:57 PM Shantur Rathore wrote: > > Hi Marek, > > On Sat, Dec 9, 2023 at 5:31 PM Marek Vasut wrote: > > > > On 12/8/23 01:09, Shantur Rathore wrote: > > > USB port regulators should be controlled by phys > > > > PHYs > > > > > so we remove always-on property and let phy manage the > > > regulator. > > > > > > Typec port has misconfugred > > > > misconfigured (typo) > > Thanks will fix both typos in v3 > > > > > > phy-supply in upstream and > > > now that we are removing always-on, we need to fix the > > > phy-supply until its fixed upstream. > > > > Where is the upstream Linux kernel patch ? > > Was it submitted already ? > > > > No, there is no patch yet for this in Linux upstream. > > > You should send this patch separately, it doesn't depend on the USB Hub > > 1/2 patch and will go in through different (rockchip) tree. > > Sure, I will split and send v3 for these now. These have been split to [0] - https://patchwork.ozlabs.org/project/uboot/patch/20231209181057.331815-...@shantur.com/ [1] - https://patchwork.ozlabs.org/project/uboot/patch/20231209180437.330969-...@shantur.com/ Thanks, Shantur
[PATCH v3] common: usb-hub: Reset hub port before scanning
Currently when a hub is turned on, all the ports are powered on.
This works well for hubs which have individual power control.
For the hubs without individual power control this has no effect.
Mostly in these scenarios the hub port is powered before the USB
controller is enabled, this can lead to some devices in unexpected
state.
With this patch, we explicitly reset the port while powering up hub
This resets the port for hubs without port power control and has
no effect on hubs with port power control as the port is still off.
Before this patch AMicro AM8180 based NVME to USB adapter won't be
detected as a USB3.0 Mass Storage device but with this it works as
expected.
Tested working after this patch:
1. AMicro AM8180 based NVME to USB Adapter
2. Kingston DataTraveler 3.0
3. GenesysLogic USB3.0 Hub
The drives were tested while connected directly and via the hub.
Signed-off-by: Shantur Rathore
---
Changes in v3:
- Split up patches as seperate series
Changes in v2:
- As requested, added fix for regulator-always-on in RockPro64
common/usb_hub.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/usb_hub.c b/common/usb_hub.c
index 70279f301d..3fb7e14d10 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
debug("enabling power on all ports\n");
for (i = 0; i < dev->maxchild; i++) {
+ usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
+ debug("Reset : port %d returns %lX\n", i + 1, dev->status);
usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
- debug("port %d returns %lX\n", i + 1, dev->status);
+ debug("PowerOn : port %d returns %lX\n", i + 1, dev->status);
}
#ifdef CONFIG_SANDBOX
--
2.40.1
[PATCH v3] dts: rockpro64: Disable usb regulators-always-on
USB port regulators should be controlled by PHYs
so we remove always-on property and let PHY manage the
regulator.
Typec port has misconfigured phy-supply in upstream and
now that we are removing always-on, we need to fix the
phy-supply until its fixed upstream.
Signed-off-by: Shantur Rathore
---
Changes in v3:
- Split up patches as seperate series
Changes in v2:
- As requested, added fix for regulator-always-on in RockPro64
arch/arm/dts/rk3399-rockpro64-u-boot.dtsi | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
index 732727d9b0..638d567357 100644
--- a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
@@ -22,6 +22,18 @@
};
};
+_host {
+ phy-supply = <_typec>;
+};
+
+_host {
+ /delete-property/ regulator-always-on;
+};
+
+_typec {
+ /delete-property/ regulator-always-on;
+};
+
_center {
regulator-min-microvolt = <95>;
regulator-max-microvolt = <95>;
--
2.40.1
Re: [PATCH v2 2/2] dts: rockpro64: Disable usb regulators-always-on
Hi Marek, On Sat, Dec 9, 2023 at 5:31 PM Marek Vasut wrote: > > On 12/8/23 01:09, Shantur Rathore wrote: > > USB port regulators should be controlled by phys > > PHYs > > > so we remove always-on property and let phy manage the > > regulator. > > > > Typec port has misconfugred > > misconfigured (typo) Thanks will fix both typos in v3 > > > phy-supply in upstream and > > now that we are removing always-on, we need to fix the > > phy-supply until its fixed upstream. > > Where is the upstream Linux kernel patch ? > Was it submitted already ? > No, there is no patch yet for this in Linux upstream. > You should send this patch separately, it doesn't depend on the USB Hub > 1/2 patch and will go in through different (rockchip) tree. Sure, I will split and send v3 for these now.
Re: bootstd: Support for distro specific EFI folders
Hi Peter, On Wed, Dec 6, 2023 at 12:16 PM Peter Robinson wrote: > > On Sun, Nov 19, 2023 at 6:55 PM Mark Kettenis wrote: > > > > > Date: Sat, 18 Nov 2023 23:52:11 +0100 > > > From: Heinrich Schuchardt > > > > Hi Heinrich, > > > > > On 11/18/23 22:28, Shantur Rathore wrote: > > > > Hi Heinrich, > > > > > > > > On Fri, Nov 17, 2023 at 3:12 PM Heinrich Schuchardt > > > > wrote: > > > >> > > > >> On 11/16/23 19:45, Shantur Rathore wrote: > > > >>> On Thu, Nov 16, 2023 at 6:15 PM Heinrich Schuchardt > > > >>> wrote: > > > >>>> > > > >>>> On 11/16/23 17:52, Shantur Rathore wrote: > > > >>>>> Hi Simon, > > > >>>>> > > > >>>>> Currently bootstd - bootmethod_efi only looks for the default > > > >>>>> bootxx64.efi in /EFI/boot folder only. > > > >>>>> Generally many distros end up putting their bootloaders in > > > >>>>> EFI/ folders like EFI/ubuntu and EFI/debian etc. > > > >>>>> > > > >>>>> In x86 worlds, the NVRAM is modified and new boot entries are added > > > >>>>> to > > > >>>>> support these but in the U-boot world the NVRAM variables are > > > >>>>> read-only. > > > >>>> > > > >>>> I guess you are referring to UEFI boot options. These typically are > > > >>>> not > > > >>>> stored in non-volatile RAM but on a SPI flash device. > > > >>>> > > > >>> > > > >>> Thanks for correcting me. > > > >>> > > > >>>>> > > > >>>>> What would be the best way to implement this? > > > >>>>> > > > >>>>> I was thinking of having a "efi_prefixes" environment variable which > > > >>>>> can be set to "ubuntu debian centos" etc and bootmethod_efi can try > > > >>>>> all of them. Will bootmethod_efi be able to support multiple > > > >>>>> entries ( > > > >>>>> thinking of multiboot ) ? > > > >>>> > > > >>>> On my laptop I have: > > > >>>> > > > >>>> EFI/Microsoft/Boot/bootmgr.efi > > > >>>> EFI/Microsoft/Boot/memtest.efi > > > >>>> EFI/Boot/bootx64.efi > > > >>>> EFI/Boot/fbx64.efi > > > >>>> EFI/Boot/mmx64.efi > > > >>>> EFI/debian/shimx64.efi > > > >>>> EFI/debian/grubx64.efi > > > >>>> EFI/debian/mmx64.efi > > > >>>> EFI/debian/fbx64.efi > > > >>>> EFI/ubuntu/grubx64.efi > > > >>>> EFI/ubuntu/shimx64.efi > > > >>>> EFI/ubuntu/mmx64.efi > > > >>>> > > > >>>> Obviously each installed operating system provides multiple EFI > > > >>>> binaries > > > >>>> and non uses the fallback file name BOOT.EFI. A value "ubuntu > > > >>>> debian centos" would not be able to describe which file you are > > > >>>> looking > > > >>>> for. > > > >>>> > > > >>>> We already have the U-Boot command line eficonfig and efidebug > > > >>>> commands > > > >>>> to set up UEFI boot options which can describe which EFI binary to > > > >>>> load > > > >>>> and which command line to pass to it. These are considered by the > > > >>>> existing boot flows. > > > >>> > > > >>> So, I am building a new RockPro64 based cluster and using Canonical > > > >>> MAAS to set them up automatically, booting them up using DHCP and > > > >>> installing them over the network. > > > >>> I configured an Armbian image using Packer to be compatible with MAAS > > > >>> and it happily installs it. As part of installation process, a > > > >>> grub-install is run which installs the grub efi, > > > >>> this EFI ends up in EFI/debian instead of expected EFI/boot. > > > >>> To be able to make it boot, I have to add commands to move it to > > > >>> EFI/
Re: [PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
Hi Peter, On Fri, Dec 8, 2023 at 12:59 PM Peter Robinson wrote: > > On Fri, Dec 8, 2023 at 12:52 PM Shantur Rathore wrote: > > > > Hi Jagan, > > > > On Fri, Dec 8, 2023 at 11:13 AM Jagan Teki > > wrote: > > > > > > On Sun, Nov 19, 2023 at 10:54 PM Shantur Rathore wrote: > > > > > > > > Rockchip SoCs can support wide range of bootflows. > > > > Without full bootflow commands, it can be difficult to > > > > figure out issues if any, hence enable by default. > > > > > > > > Reviewed-by: Simon Glass > > > > > > > > Signed-off-by: Shantur Rathore > > > > --- > > > > > > > > (no changes since v1) > > > > > > > > arch/arm/Kconfig | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > > > index d812685c98..fca6ef6d7e 100644 > > > > --- a/arch/arm/Kconfig > > > > +++ b/arch/arm/Kconfig > > > > @@ -1986,6 +1986,7 @@ config ARCH_ROCKCHIP > > > > imply CMD_DM > > > > imply DEBUG_UART_BOARD_INIT > > > > imply BOOTSTD_DEFAULTS > > > > + imply BOOTSTD_FULL if BOOTSTD_DEFAULTS > > > > > > Yes, but better to give this option to specific board vendors as > > > defaults are enough to boot 1st bootflow and what ever media's it has. > > > > > > > Yes, that's correct it is enough to boot but by default there is no option > > to choose what to boot from. > > This was discussed in an earlier version of this patch [0] where I was > > explicitly enabling only for RP64. > > What actual extra functionality does this provide, what is the impact > on the size of images? You've not provided reasonable justification > outside of very vague statements, it would be useful to know what the > added options actually solves. BOOTSTD_FULL enables all the options in bootflow commands. This is needed if - you want to choose between multiple available bootflows rather than just boot one default. - if you need to list the available bootflows that bootstd has found - if you need to select and boot any bootflow other than default. By default all other commands in U-boot come with options to show details For example, nvme info, nvme detail, usb info, usb tree but with bootstd no way to know anything. Image size - u-boot.itd without BOOTSTD_FULL - 1193984 bytes Image size - u-boot.itb with BOOTSTD_FULL - 1214976 bytes Difference - 20992 bytes According to binman for RK3399 u-boot can take upto 4M [1] so we have ample space. This was discussed in the previous patch in the link below [0] [0] - https://patchwork.ozlabs.org/project/uboot/patch/2023001329.537704-...@shantur.com/ [1] - https://github.com/shantur/u-boot/blob/master/arch/arm/dts/rk3399-u-boot.dtsi#L68 Kind regards, Shantur
Re: [PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
Hi Jagan, On Fri, Dec 8, 2023 at 11:13 AM Jagan Teki wrote: > > On Sun, Nov 19, 2023 at 10:54 PM Shantur Rathore wrote: > > > > Rockchip SoCs can support wide range of bootflows. > > Without full bootflow commands, it can be difficult to > > figure out issues if any, hence enable by default. > > > > Reviewed-by: Simon Glass > > > > Signed-off-by: Shantur Rathore > > --- > > > > (no changes since v1) > > > > arch/arm/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > index d812685c98..fca6ef6d7e 100644 > > --- a/arch/arm/Kconfig > > +++ b/arch/arm/Kconfig > > @@ -1986,6 +1986,7 @@ config ARCH_ROCKCHIP > > imply CMD_DM > > imply DEBUG_UART_BOARD_INIT > > imply BOOTSTD_DEFAULTS > > + imply BOOTSTD_FULL if BOOTSTD_DEFAULTS > > Yes, but better to give this option to specific board vendors as > defaults are enough to boot 1st bootflow and what ever media's it has. > Yes, that's correct it is enough to boot but by default there is no option to choose what to boot from. This was discussed in an earlier version of this patch [0] where I was explicitly enabling only for RP64. [0] - https://patchwork.ozlabs.org/project/uboot/patch/2023001329.537704-...@shantur.com/ Kind regards, Shantur
Re: [PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
Hi Tom / Kever On Sun, Nov 19, 2023 at 5:24 PM Shantur Rathore wrote: > > Rockchip SoCs can support wide range of bootflows. > Without full bootflow commands, it can be difficult to > figure out issues if any, hence enable by default. > > Reviewed-by: Simon Glass > > Signed-off-by: Shantur Rathore > --- > > (no changes since v1) > > arch/arm/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index d812685c98..fca6ef6d7e 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -1986,6 +1986,7 @@ config ARCH_ROCKCHIP > imply CMD_DM > imply DEBUG_UART_BOARD_INIT > imply BOOTSTD_DEFAULTS > + imply BOOTSTD_FULL if BOOTSTD_DEFAULTS > imply FAT_WRITE > imply SARADC_ROCKCHIP > imply SPL_SYSRESET > -- > 2.40.1 > Can this please be merged in ? Kind regards, Shantur
Re: [PATCH] common: usb-hub: Reset hub port before scanning
Hi Marek, On Tue, Dec 5, 2023 at 5:40 AM Marek Vasut wrote: > > On 12/4/23 13:10, Shantur Rathore wrote: > > On Mon, Dec 4, 2023 at 11:05 AM Marek Vasut wrote: > >> > >> On 12/4/23 11:59, Shantur Rathore wrote: > >>> Hi Marek, > >>> > >>> On Sun, Dec 3, 2023 at 10:37 PM Marek Vasut wrote: > >>>> > >>>> On 12/3/23 22:42, Shantur Rathore wrote: > >>>>> Hi Marek, > >>>>> > >>>>> On Sun, Dec 3, 2023 at 8:42 PM Marek Vasut wrote: > >>>>>> > >>>>>> On 11/24/23 01:37, Shantur Rathore wrote: > >>>>>>> Hi Marek, > >>>>>> > >>>>>> Hi, > >>>>>> > >>>>>> sorry for the late reply. > >>>>>> > >>>>>>>>>>> In my case RockPro64, the power to usb ports onboard is > >>>>>>>>>>> controlled by > >>>>>>>>>>> a regulator. > >>>>>>>>>>> This regulator is enabled as part of init as here > >>>>>>>>>>> https://github.com/u-boot/u-boot/blob/master/arch/arm/dts/rk3399-rockpro64.dtsi#L177 > >>>>>>>>>>> > >>>>>>>>>>> On init, the usb devices connected to the port are powered up, in > >>>>>>>>>>> my > >>>>>>>>>>> case AM8180 (a RTL9210 based ) NVMe to USB enclosure with UAS. > >>>>>>>>>>> But the usb controller is only enabled at 'usb start' and by this > >>>>>>>>>>> time > >>>>>>>>>>> the device is in some state that it doesn't get detected. > >>>>>>>>>>> > >>>>>>>>>>> One way to make sure the devices connected to the ports are in an > >>>>>>>>>>> initialising state is by issuing a port reset before scanning the > >>>>>>>>>>> port. > >>>>>>>>>> > >>>>>>>>>> Why not remove this regulator-always-on and let the PHY driver > >>>>>>>>>> turn the > >>>>>>>>>> VBUS ON only when it is needed ? > >>>>>>>>>> > >>>>>>>>>> Wouldn't that solve the problem too AND remove unnecessarily > >>>>>>>>>> enabled > >>>>>>>>>> regulator ? > >>>>>>>>>> > >>>>>>>>>> [...] > >>>>>>>>> > >>>>>>>>> Removing the regulator-always-on does make it work but there are > >>>>>>>>> few issues > >>>>>>>>> > >>>>>>>>> 1. regulator is used to control power to multiple ports ( USB3.0 and > >>>>>>>>> USB2.0 in RockPro64 ), > >>>>>>>>> so this could lead to all ports turning off if a phy resets / turns > >>>>>>>>> off power > >>>>>>>> > >>>>>>>> I vaguely recall seeing some refcounting patch for regulator > >>>>>>>> subsystem, > >>>>>>>> would that help ? > >>>>>>> > >>>>>>> I don't think this would be a problem for u-boot, but linux maybe. > >>>>>> > >>>>>> How so ? > >>>>> > >>>>> Actually, I am wrong. I wasn't sure if there is refcounting for the > >>>>> regulator subsystem > >>>>> in linux. just found it has so this is null and void. > >>>>> > >>>>>> > >>>>>>>>> 2. Even with regulator-always-on and without this reset port patch, > >>>>>>>>> linux was always > >>>>>>>>> able to detect the drive on the port, so there is something missing > >>>>>>>>> in u-boot. > >>>>>>>>> 3. Looking at usb hub code in Linux, I found that for USB3.0 it > >>>>>>>>> tries > >>>>>>>>> to reset the port before > >>>>>>>>> scanning. The comment says > >>>>>>>>> > >>&
[PATCH v2 0/2] usb: Fix USB3 detection
This patch series implements USB 3.0 port reset requirement before scanning port. Changes in v2: - As requested, added fix for regulator-always-on in RockPro64 Shantur Rathore (2): common: usb-hub: Reset hub port before scanning dts: rockpro64: Disable usb regulators-always-on arch/arm/dts/rk3399-rockpro64-u-boot.dtsi | 12 common/usb_hub.c | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-) -- 2.40.1
[PATCH v2 2/2] dts: rockpro64: Disable usb regulators-always-on
USB port regulators should be controlled by phys
so we remove always-on property and let phy manage the
regulator.
Typec port has misconfugred phy-supply in upstream and
now that we are removing always-on, we need to fix the
phy-supply until its fixed upstream.
Signed-off-by: Shantur Rathore
---
Changes in v2:
- As requested, added fix for regulator-always-on in RockPro64
arch/arm/dts/rk3399-rockpro64-u-boot.dtsi | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
index 732727d9b0..638d567357 100644
--- a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
@@ -22,6 +22,18 @@
};
};
+_host {
+ phy-supply = <_typec>;
+};
+
+_host {
+ /delete-property/ regulator-always-on;
+};
+
+_typec {
+ /delete-property/ regulator-always-on;
+};
+
_center {
regulator-min-microvolt = <95>;
regulator-max-microvolt = <95>;
--
2.40.1
[PATCH v2 1/2] common: usb-hub: Reset hub port before scanning
Currently when a hub is turned on, all the ports are powered on.
This works well for hubs which have individual power control.
For the hubs without individual power control this has no effect.
Mostly in these scenarios the hub port is powered before the USB
controller is enabled, this can lead to some devices in unexpected
state.
With this patch, we explicitly reset the port while powering up hub
This resets the port for hubs without port power control and has
no effect on hubs with port power control as the port is still off.
Before this patch AMicro AM8180 based NVME to USB adapter won't be
detected as a USB3.0 Mass Storage device but with this it works as
expected.
Tested working after this patch:
1. AMicro AM8180 based NVME to USB Adapter
2. Kingston DataTraveler 3.0
3. GenesysLogic USB3.0 Hub
The drives were tested while connected directly and via the hub.
Signed-off-by: Shantur Rathore
---
(no changes since v1)
common/usb_hub.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/usb_hub.c b/common/usb_hub.c
index 85c0822d8b..06fe436add 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
debug("enabling power on all ports\n");
for (i = 0; i < dev->maxchild; i++) {
+ usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
+ debug("Reset : port %d returns %lX\n", i + 1, dev->status);
usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
- debug("port %d returns %lX\n", i + 1, dev->status);
+ debug("PowerOn : port %d returns %lX\n", i + 1, dev->status);
}
#ifdef CONFIG_SANDBOX
--
2.40.1
Re: bootstd: Support for distro specific EFI folders
Hi Simon and Heinrich, On Sun, Dec 3, 2023 at 8:38 PM Simon Glass wrote: > > Hi Heinrich, > > On Sun, 3 Dec 2023 at 13:00, Heinrich Schuchardt > wrote: > > > > On 12/3/23 20:50, Simon Glass wrote: > > > Hi Heinrich, > > > > > > On Sun, 3 Dec 2023 at 11:33, Heinrich Schuchardt > > > wrote: > > >> > > >> On 12/3/23 19:22, Simon Glass wrote: > > >>> Hi Heinrich, > > >>> > > >>> On Sun, 3 Dec 2023 at 10:53, Heinrich Schuchardt > > >>> wrote: > > >>>> > > >>>> On 12/3/23 18:44, Simon Glass wrote: > > >>>>> Hi Heinrich, > > >>>>> > > >>>>> On Sun, 3 Dec 2023 at 03:55, Heinrich Schuchardt > > >>>>> wrote: > > >>>>>> > > >>>>>> On 12/3/23 00:38, Shantur Rathore wrote: > > >>>>>>> Hi Simon, > > >>>>>>> > > >>>>>>> On Sat, Dec 2, 2023 at 6:33 PM Simon Glass > > >>>>>>> wrote: > > >>>>>>>> > > >>>>>>>> Hi, > > >>>>>>>> > > >>>>>>>> On Mon, 20 Nov 2023 at 00:02, Ilias Apalodimas > > >>>>>>>> wrote: > > >>>>>>>>> > > >>>>>>>>> Hi Mark, > > >>>>>>>>> > > >>>>>>>>> On Sun, 19 Nov 2023 at 19:38, Mark Kettenis > > >>>>>>>>> wrote: > > >>>>>>>>>> > > >>>>>>>>>>> Date: Sat, 18 Nov 2023 23:52:11 +0100 > > >>>>>>>>>>> From: Heinrich Schuchardt > > >>>>>>>>>> > > >>>>>>>>>> Hi Heinrich, > > >>>>>>>>>> > > >>>>>>>>>>> On 11/18/23 22:28, Shantur Rathore wrote: > > >>>>>>>>>>>> Hi Heinrich, > > >>>>>>>>>>>> > > >>>>>>>>>>>> On Fri, Nov 17, 2023 at 3:12 PM Heinrich Schuchardt > > >>>>>>>>>>>> wrote: > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> On 11/16/23 19:45, Shantur Rathore wrote: > > >>>>>>>>>>>>>> On Thu, Nov 16, 2023 at 6:15 PM Heinrich Schuchardt > > >>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> On 11/16/23 17:52, Shantur Rathore wrote: > > >>>>>>>>>>>>>>>> Hi Simon, > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> Currently bootstd - bootmethod_efi only looks for the > > >>>>>>>>>>>>>>>> default > > >>>>>>>>>>>>>>>> bootxx64.efi in /EFI/boot folder only. > > >>>>>>>>>>>>>>>> Generally many distros end up putting their bootloaders in > > >>>>>>>>>>>>>>>> EFI/ folders like EFI/ubuntu and EFI/debian etc. > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> In x86 worlds, the NVRAM is modified and new boot entries > > >>>>>>>>>>>>>>>> are added to > > >>>>>>>>>>>>>>>> support these but in the U-boot world the NVRAM variables > > >>>>>>>>>>>>>>>> are > > >>>>>>>>>>>>>>>> read-only. > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> I guess you are referring to UEFI boot options. These > > >>>>>>>>>>>>>>> typically are not > > >>>>>>>>>>>>>>> stored in non-volatile RAM but on a SPI flash device. > > >>>>>>>>>>>>>>> > > >>>>>>>>>&g
Re: [PATCH v4 0/4] bootflow: bootmeth_efi: Fix network efi boot.
Hi Simon, On Tue, Dec 5, 2023 at 12:52 AM Simon Glass wrote: > > Hi Shantur, > > On Mon, 4 Dec 2023 at 05:38, Shantur Rathore wrote: > > > > Hi Simon, > > > > On Sun, Nov 19, 2023 at 4:56 PM Shantur Rathore wrote: > > > > > > > > > Currently bootmeth_efi crashes while doing a network (dhcp) boot. > > > This patch series fixes issues and both network and disk boot works. > > > > > How can I help to get this patch series merged in? > > You can check patchwork [1] to see who it is assigned to. I don't see > it in my queue though. > It's with Tom Rini I believe but with status Needs Review / ACK. https://patchwork.ozlabs.org/project/uboot/list/?series=382855 Kind Regards, Shantur
Re: [PATCH v4 0/4] bootflow: bootmeth_efi: Fix network efi boot.
Hi Simon, On Sun, Nov 19, 2023 at 4:56 PM Shantur Rathore wrote: > > > Currently bootmeth_efi crashes while doing a network (dhcp) boot. > This patch series fixes issues and both network and disk boot works. > How can I help to get this patch series merged in? Kind Regards Shantur
Re: [PATCH] common: usb-hub: Reset hub port before scanning
On Mon, Dec 4, 2023 at 11:05 AM Marek Vasut wrote: > > On 12/4/23 11:59, Shantur Rathore wrote: > > Hi Marek, > > > > On Sun, Dec 3, 2023 at 10:37 PM Marek Vasut wrote: > >> > >> On 12/3/23 22:42, Shantur Rathore wrote: > >>> Hi Marek, > >>> > >>> On Sun, Dec 3, 2023 at 8:42 PM Marek Vasut wrote: > >>>> > >>>> On 11/24/23 01:37, Shantur Rathore wrote: > >>>>> Hi Marek, > >>>> > >>>> Hi, > >>>> > >>>> sorry for the late reply. > >>>> > >>>>>>>>> In my case RockPro64, the power to usb ports onboard is controlled > >>>>>>>>> by > >>>>>>>>> a regulator. > >>>>>>>>> This regulator is enabled as part of init as here > >>>>>>>>> https://github.com/u-boot/u-boot/blob/master/arch/arm/dts/rk3399-rockpro64.dtsi#L177 > >>>>>>>>> > >>>>>>>>> On init, the usb devices connected to the port are powered up, in my > >>>>>>>>> case AM8180 (a RTL9210 based ) NVMe to USB enclosure with UAS. > >>>>>>>>> But the usb controller is only enabled at 'usb start' and by this > >>>>>>>>> time > >>>>>>>>> the device is in some state that it doesn't get detected. > >>>>>>>>> > >>>>>>>>> One way to make sure the devices connected to the ports are in an > >>>>>>>>> initialising state is by issuing a port reset before scanning the > >>>>>>>>> port. > >>>>>>>> > >>>>>>>> Why not remove this regulator-always-on and let the PHY driver turn > >>>>>>>> the > >>>>>>>> VBUS ON only when it is needed ? > >>>>>>>> > >>>>>>>> Wouldn't that solve the problem too AND remove unnecessarily enabled > >>>>>>>> regulator ? > >>>>>>>> > >>>>>>>> [...] > >>>>>>> > >>>>>>> Removing the regulator-always-on does make it work but there are few > >>>>>>> issues > >>>>>>> > >>>>>>> 1. regulator is used to control power to multiple ports ( USB3.0 and > >>>>>>> USB2.0 in RockPro64 ), > >>>>>>> so this could lead to all ports turning off if a phy resets / turns > >>>>>>> off power > >>>>>> > >>>>>> I vaguely recall seeing some refcounting patch for regulator subsystem, > >>>>>> would that help ? > >>>>> > >>>>> I don't think this would be a problem for u-boot, but linux maybe. > >>>> > >>>> How so ? > >>> > >>> Actually, I am wrong. I wasn't sure if there is refcounting for the > >>> regulator subsystem > >>> in linux. just found it has so this is null and void. > >>> > >>>> > >>>>>>> 2. Even with regulator-always-on and without this reset port patch, > >>>>>>> linux was always > >>>>>>> able to detect the drive on the port, so there is something missing > >>>>>>> in u-boot. > >>>>>>> 3. Looking at usb hub code in Linux, I found that for USB3.0 it tries > >>>>>>> to reset the port before > >>>>>>> scanning. The comment says > >>>>>>> > >>>>>>> /* Is a USB 3.0 port in the Inactive or Compliance Mode state? > >>>>>>> * Port warm reset is required to recover > >>>>>>> */ > >>>>>>> > >>>>>>> i. > >>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L5736 > >>>>>>> ii. > >>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2836 > >>>>>>> > >>>>>>> I believe this isn't implemented in u-boot and hence explicit reset of > >>>>>>> a port fixes the issue. > >>>>>> > >>>>>> I feel this is separate issue and what needs to be fixed first is the > >&g
Re: [PATCH] common: usb-hub: Reset hub port before scanning
Hi Marek, On Sun, Dec 3, 2023 at 10:37 PM Marek Vasut wrote: > > On 12/3/23 22:42, Shantur Rathore wrote: > > Hi Marek, > > > > On Sun, Dec 3, 2023 at 8:42 PM Marek Vasut wrote: > >> > >> On 11/24/23 01:37, Shantur Rathore wrote: > >>> Hi Marek, > >> > >> Hi, > >> > >> sorry for the late reply. > >> > >>>>>>> In my case RockPro64, the power to usb ports onboard is controlled by > >>>>>>> a regulator. > >>>>>>> This regulator is enabled as part of init as here > >>>>>>> https://github.com/u-boot/u-boot/blob/master/arch/arm/dts/rk3399-rockpro64.dtsi#L177 > >>>>>>> > >>>>>>> On init, the usb devices connected to the port are powered up, in my > >>>>>>> case AM8180 (a RTL9210 based ) NVMe to USB enclosure with UAS. > >>>>>>> But the usb controller is only enabled at 'usb start' and by this time > >>>>>>> the device is in some state that it doesn't get detected. > >>>>>>> > >>>>>>> One way to make sure the devices connected to the ports are in an > >>>>>>> initialising state is by issuing a port reset before scanning the > >>>>>>> port. > >>>>>> > >>>>>> Why not remove this regulator-always-on and let the PHY driver turn the > >>>>>> VBUS ON only when it is needed ? > >>>>>> > >>>>>> Wouldn't that solve the problem too AND remove unnecessarily enabled > >>>>>> regulator ? > >>>>>> > >>>>>> [...] > >>>>> > >>>>> Removing the regulator-always-on does make it work but there are few > >>>>> issues > >>>>> > >>>>> 1. regulator is used to control power to multiple ports ( USB3.0 and > >>>>> USB2.0 in RockPro64 ), > >>>>> so this could lead to all ports turning off if a phy resets / turns off > >>>>> power > >>>> > >>>> I vaguely recall seeing some refcounting patch for regulator subsystem, > >>>> would that help ? > >>> > >>> I don't think this would be a problem for u-boot, but linux maybe. > >> > >> How so ? > > > > Actually, I am wrong. I wasn't sure if there is refcounting for the > > regulator subsystem > > in linux. just found it has so this is null and void. > > > >> > >>>>> 2. Even with regulator-always-on and without this reset port patch, > >>>>> linux was always > >>>>> able to detect the drive on the port, so there is something missing in > >>>>> u-boot. > >>>>> 3. Looking at usb hub code in Linux, I found that for USB3.0 it tries > >>>>> to reset the port before > >>>>> scanning. The comment says > >>>>> > >>>>> /* Is a USB 3.0 port in the Inactive or Compliance Mode state? > >>>>> * Port warm reset is required to recover > >>>>> */ > >>>>> > >>>>> i. > >>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L5736 > >>>>> ii. > >>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2836 > >>>>> > >>>>> I believe this isn't implemented in u-boot and hence explicit reset of > >>>>> a port fixes the issue. > >>>> > >>>> I feel this is separate issue and what needs to be fixed first is the > >>>> hard-wired VBus control. > >>> > >>> I beg to differ on this, couple of reasons for this > >>> > >>> 1. The same drive works fine without being reset on the USB2.0 ports - > >>> this > >>> confirms that the issue is related to USB3.0 only. > >> > >> This is a separate issue from the hard-wired VBus control. I agree the > >> issue does exist, I would simply like to avoid conflating the hard-wired > >> VBus control with device reset. > >> > >>> 2. Linux is able to detect the same drive on USB3.0 when u-boot fails - > >>> this > >>> confirms issue doesn't lie with regulator-always-on > >> > >> See above > >> > >>> 3. The links I pasted earlier ment
Re: efi: Set Variable Runtime implementation
Hi Simon, On Sun, Dec 3, 2023 at 5:44 PM Simon Glass wrote: > > Hi Shantur, > > On Sat, 2 Dec 2023 at 16:03, Shantur Rathore wrote: > > > > Hi Simon, > > > > On Fri, Dec 1, 2023 at 6:44 PM Simon Glass wrote: > > > > > > Hi Shantur, > > > > > > On Mon, 27 Nov 2023 at 10:27, Shantur Rathore wrote: > > > > > > > > + Simon as he seems to have done a lot of work in the driver model. > > > > > > > > On Mon, Nov 27, 2023 at 10:12 AM Shantur Rathore > > > > wrote: > > > > > > > > > > Hi Ilias, > > > > > > > > > > On Mon, Nov 27, 2023 at 7:16 AM Ilias Apalodimas > > > > > wrote: > > > > > > > > > > > > Hi Shantur > > > > > > > > > > > > On Sun, 26 Nov 2023 at 12:33, Shantur Rathore > > > > > > wrote: > > > > > > > > > > > > > > Hi Peter, > > > > > > > > > > > > > > On Sat, Nov 25, 2023 at 6:19 AM Peter Robinson > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Shantur, > > > > > > > > > > > > > > > > On Fri, Nov 24, 2023 at 11:55 PM Shantur Rathore > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > > > > > > > On Fri, Nov 24, 2023 at 10:50 PM Ilias Apalodimas > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Hi Shantur > > > > > > > > > > > > > > > > > > > > On Fri, 24 Nov 2023 at 18:51, Shantur Rathore > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > Hi Heinrich, > > > > > > > > > > > > > > > > > > > > > > I am trying to work out how to enable the SetVariableRT > > > > > > > > > > > service in > > > > > > > > > > > U-Boot and came across your patch [1] which initially had > > > > > > > > > > > the > > > > > > > > > > > SetVariable RT service enabled in EFI but in the final > > > > > > > > > > > patch this was > > > > > > > > > > > removed. > > > > > > > > > > > I am hoping to implement it on top of the SPI Flash EFI > > > > > > > > > > > store [2] to > > > > > > > > > > > be able to set Boot order and boot items from Linux the > > > > > > > > > > > UEFI way. > > > > > > > > > > > > > > > > > > > > > > Can I pick your brain on why it was dropped in the patch? > > > > > > > > > > > Is there any limitation in SetVariableRT service ? > > > > > > > > > > > > > > > > > > > > I recently had a talk about it in Plumbers [0]. Generally > > > > > > > > > > speaking, RT > > > > > > > > > > + hardware owned by the kernel is a very weird combination > > > > > > > > > > since you > > > > > > > > > > can't guarantee exclusive access to the flash or the bus > > > > > > > > > > and you have > > > > > > > > > > to preserve a *lot* of code alive in u-boot. > > > > > > > > > > > > > > > > > > > > I'll respond to your v1 patchset and we can discuss details > > > > > > > > > > there as well. > > > > > > > > > > > > > > > > > > > > [0] https://lpc.events/event/17/contributions/1653/ > > > > > > > > > > > > > > > > > > Thanks for the background and helping me understand the > > > > > > > > > problem. > > > > > > > > > Makes me wonder how things work in the PC world. > > > > > > > > > U-boot being only ~1MB, can we not leave it all in memory and > > > > >
Re: [PATCH v3 3/3] defconfig: rockpro64: Enable SF EFI var store
Hi Simon,
On Sun, Dec 3, 2023 at 5:44 PM Simon Glass wrote:
>
> Hi Shantur,
>
> On Sat, 2 Dec 2023 at 16:12, Shantur Rathore wrote:
> >
> > Hi Simon,
> >
> > On Fri, Dec 1, 2023 at 6:44 PM Simon Glass wrote:
> > >
> > > Hi Shantur,
> > >
> > > On Sun, 26 Nov 2023 at 15:09, Shantur Rathore wrote:
> > > >
> > > > RockPro64 uses SPI Flash for storing env, also use it store
> > > > EFI variables.
> > > >
> > > > Signed-off-by: Shantur Rathore
> > > > ---
> > > >
> > > > configs/rockpro64-rk3399_defconfig | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/configs/rockpro64-rk3399_defconfig
> > > > b/configs/rockpro64-rk3399_defconfig
> > > > index 4cd6b76665..f550f2ae43 100644
> > > > --- a/configs/rockpro64-rk3399_defconfig
> > > > +++ b/configs/rockpro64-rk3399_defconfig
> > > > @@ -8,6 +8,8 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> > > > CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x30
> > > > CONFIG_ENV_SIZE=0x8000
> > > > CONFIG_ENV_OFFSET=0x3F8000
> > > > +CONFIG_EFI_VARIABLE_SF_STORE=y
> > > > +CONFIG_EFI_VARIABLE_SF_OFFSET=0x7D
> > >
> > > Can we use this offset in binman when creating the SPI-flash image?
> >
> > Unless I missed something, binman is used when something is bundled
> > with a u-boot image.
> > This is similar to ENV_OFFSET letting u-boot know where it can write
> > the EFI variables.
> > I didn't see ENV_OFFSET referred in rk3399-u-boot.dtsi or
> > rk3399-rockpro64-u-boot.dtsi
> >
> > Please correct me if I am wrong.
>
> Well, this is what I see in arch/arm/dts/rk3399-u-boot.dtsi :
>
> #if defined(CONFIG_ROCKCHIP_SPI_IMAGE) && defined(CONFIG_HAS_ROM)
> {
>multiple-images;
>rom {
> filename = "u-boot.rom";
> size = <0x40>;
> pad-byte = <0xff>;
>
> mkimage {
> args = "-n rk3399 -T rkspi";
> u-boot-spl {
> };
> };
> u-boot-img {
> offset = <0x4>;
> };
> u-boot {
> offset = <0x30>;
> };
> fdtmap {
> };
>};
> };
> #endif /* CONFIG_ROCKCHIP_SPI_IMAGE && CONFIG_HAS_ROM */
>
> So it looks like 0x3f8000 fits in there somewhere.
>
Thanks for clarifying. Now I see how ENV space is declared as used by u-boot.
> In fact, I wonder if we should add something like:
>
> u-boot-env {
>offset = ;
> };
>
> so the environment gets filled into the SPI flash when we write the image?
>
Do we want to reset the environment to default on every u-boot update?
I believe this would overwrite the environment space isn't it.
> I suppose we could also add the EFI variable store, although that
> would require adding a new entry type to binman.
>
I am not sure about this but surely EFI vars can be moved to a location
just after the u-boot environment and increase the u-boot image size.
Would you still need an entry in dts?
Re: [PATCH] common: usb-hub: Reset hub port before scanning
Hi Marek, On Sun, Dec 3, 2023 at 8:42 PM Marek Vasut wrote: > > On 11/24/23 01:37, Shantur Rathore wrote: > > Hi Marek, > > Hi, > > sorry for the late reply. > > >>>>> In my case RockPro64, the power to usb ports onboard is controlled by > >>>>> a regulator. > >>>>> This regulator is enabled as part of init as here > >>>>> https://github.com/u-boot/u-boot/blob/master/arch/arm/dts/rk3399-rockpro64.dtsi#L177 > >>>>> > >>>>> On init, the usb devices connected to the port are powered up, in my > >>>>> case AM8180 (a RTL9210 based ) NVMe to USB enclosure with UAS. > >>>>> But the usb controller is only enabled at 'usb start' and by this time > >>>>> the device is in some state that it doesn't get detected. > >>>>> > >>>>> One way to make sure the devices connected to the ports are in an > >>>>> initialising state is by issuing a port reset before scanning the > >>>>> port. > >>>> > >>>> Why not remove this regulator-always-on and let the PHY driver turn the > >>>> VBUS ON only when it is needed ? > >>>> > >>>> Wouldn't that solve the problem too AND remove unnecessarily enabled > >>>> regulator ? > >>>> > >>>> [...] > >>> > >>> Removing the regulator-always-on does make it work but there are few > >>> issues > >>> > >>> 1. regulator is used to control power to multiple ports ( USB3.0 and > >>> USB2.0 in RockPro64 ), > >>> so this could lead to all ports turning off if a phy resets / turns off > >>> power > >> > >> I vaguely recall seeing some refcounting patch for regulator subsystem, > >> would that help ? > > > > I don't think this would be a problem for u-boot, but linux maybe. > > How so ? Actually, I am wrong. I wasn't sure if there is refcounting for the regulator subsystem in linux. just found it has so this is null and void. > > >>> 2. Even with regulator-always-on and without this reset port patch, > >>> linux was always > >>> able to detect the drive on the port, so there is something missing in > >>> u-boot. > >>> 3. Looking at usb hub code in Linux, I found that for USB3.0 it tries > >>> to reset the port before > >>> scanning. The comment says > >>> > >>> /* Is a USB 3.0 port in the Inactive or Compliance Mode state? > >>>* Port warm reset is required to recover > >>>*/ > >>> > >>> i. > >>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L5736 > >>> ii. > >>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2836 > >>> > >>> I believe this isn't implemented in u-boot and hence explicit reset of > >>> a port fixes the issue. > >> > >> I feel this is separate issue and what needs to be fixed first is the > >> hard-wired VBus control. > > > > I beg to differ on this, couple of reasons for this > > > > 1. The same drive works fine without being reset on the USB2.0 ports - this > > confirms that the issue is related to USB3.0 only. > > This is a separate issue from the hard-wired VBus control. I agree the > issue does exist, I would simply like to avoid conflating the hard-wired > VBus control with device reset. > > > 2. Linux is able to detect the same drive on USB3.0 when u-boot fails - this > > confirms issue doesn't lie with regulator-always-on > > See above > > > 3. The links I pasted earlier mentions that in USB3.0 the ports need reset > > and this is done before the port is scanned. Adding the similar reset in > > u-boot > > fixes all with the same regulator-always-on. AFAIK u-boot doesn't handle > > this special USB3.0 case and maybe this is why I was finding a few posts > > around the drive not being detected in the USB3.0 port in u-boot but works > > in > > a USB2.0 port. > > I am not disputing the need for USB 3.0 device reset, I believe there > are two issues here -- one is the hard-wired VBus regulator -- the other > is this USB 3.0 reset. They should both be fixed. Sure, agreed 100%. Do we need to fix both at the same time? Both patches seem to be independent. Kind regards, Shantur
Re: bootstd: Support for distro specific EFI folders
Hi Simon, On Sat, Dec 2, 2023 at 6:33 PM Simon Glass wrote: > > Hi, > > On Mon, 20 Nov 2023 at 00:02, Ilias Apalodimas > wrote: > > > > Hi Mark, > > > > On Sun, 19 Nov 2023 at 19:38, Mark Kettenis wrote: > > > > > > > Date: Sat, 18 Nov 2023 23:52:11 +0100 > > > > From: Heinrich Schuchardt > > > > > > Hi Heinrich, > > > > > > > On 11/18/23 22:28, Shantur Rathore wrote: > > > > > Hi Heinrich, > > > > > > > > > > On Fri, Nov 17, 2023 at 3:12 PM Heinrich Schuchardt > > > > > wrote: > > > > >> > > > > >> On 11/16/23 19:45, Shantur Rathore wrote: > > > > >>> On Thu, Nov 16, 2023 at 6:15 PM Heinrich Schuchardt > > > > >>> wrote: > > > > >>>> > > > > >>>> On 11/16/23 17:52, Shantur Rathore wrote: > > > > >>>>> Hi Simon, > > > > >>>>> > > > > >>>>> Currently bootstd - bootmethod_efi only looks for the default > > > > >>>>> bootxx64.efi in /EFI/boot folder only. > > > > >>>>> Generally many distros end up putting their bootloaders in > > > > >>>>> EFI/ folders like EFI/ubuntu and EFI/debian etc. > > > > >>>>> > > > > >>>>> In x86 worlds, the NVRAM is modified and new boot entries are > > > > >>>>> added to > > > > >>>>> support these but in the U-boot world the NVRAM variables are > > > > >>>>> read-only. > > > > >>>> > > > > >>>> I guess you are referring to UEFI boot options. These typically > > > > >>>> are not > > > > >>>> stored in non-volatile RAM but on a SPI flash device. > > > > >>>> > > > > >>> > > > > >>> Thanks for correcting me. > > > > >>> > > > > >>>>> > > > > >>>>> What would be the best way to implement this? > > > > >>>>> > > > > >>>>> I was thinking of having a "efi_prefixes" environment variable > > > > >>>>> which > > > > >>>>> can be set to "ubuntu debian centos" etc and bootmethod_efi can > > > > >>>>> try > > > > >>>>> all of them. Will bootmethod_efi be able to support multiple > > > > >>>>> entries ( > > > > >>>>> thinking of multiboot ) ? > > > > >>>> > > > > >>>> On my laptop I have: > > > > >>>> > > > > >>>> EFI/Microsoft/Boot/bootmgr.efi > > > > >>>> EFI/Microsoft/Boot/memtest.efi > > > > >>>> EFI/Boot/bootx64.efi > > > > >>>> EFI/Boot/fbx64.efi > > > > >>>> EFI/Boot/mmx64.efi > > > > >>>> EFI/debian/shimx64.efi > > > > >>>> EFI/debian/grubx64.efi > > > > >>>> EFI/debian/mmx64.efi > > > > >>>> EFI/debian/fbx64.efi > > > > >>>> EFI/ubuntu/grubx64.efi > > > > >>>> EFI/ubuntu/shimx64.efi > > > > >>>> EFI/ubuntu/mmx64.efi > > > > >>>> > > > > >>>> Obviously each installed operating system provides multiple EFI > > > > >>>> binaries > > > > >>>> and non uses the fallback file name BOOT.EFI. A value "ubuntu > > > > >>>> debian centos" would not be able to describe which file you are > > > > >>>> looking > > > > >>>> for. > > > > >>>> > > > > >>>> We already have the U-Boot command line eficonfig and efidebug > > > > >>>> commands > > > > >>>> to set up UEFI boot options which can describe which EFI binary to > > > > >>>> load > > > > >>>> and which command line to pass to it. These are considered by the > > > > >>>> existing boot flows. > > > > >>> > > > > >>> So, I am building a new RockPro64 based cluster and using Canonical > > > > >>> MAAS to set th
Re: [PATCH v3 3/3] defconfig: rockpro64: Enable SF EFI var store
Hi Simon, On Fri, Dec 1, 2023 at 6:44 PM Simon Glass wrote: > > Hi Shantur, > > On Sun, 26 Nov 2023 at 15:09, Shantur Rathore wrote: > > > > RockPro64 uses SPI Flash for storing env, also use it store > > EFI variables. > > > > Signed-off-by: Shantur Rathore > > --- > > > > configs/rockpro64-rk3399_defconfig | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/configs/rockpro64-rk3399_defconfig > > b/configs/rockpro64-rk3399_defconfig > > index 4cd6b76665..f550f2ae43 100644 > > --- a/configs/rockpro64-rk3399_defconfig > > +++ b/configs/rockpro64-rk3399_defconfig > > @@ -8,6 +8,8 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y > > CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x30 > > CONFIG_ENV_SIZE=0x8000 > > CONFIG_ENV_OFFSET=0x3F8000 > > +CONFIG_EFI_VARIABLE_SF_STORE=y > > +CONFIG_EFI_VARIABLE_SF_OFFSET=0x7D > > Can we use this offset in binman when creating the SPI-flash image? Unless I missed something, binman is used when something is bundled with a u-boot image. This is similar to ENV_OFFSET letting u-boot know where it can write the EFI variables. I didn't see ENV_OFFSET referred in rk3399-u-boot.dtsi or rk3399-rockpro64-u-boot.dtsi Please correct me if I am wrong. Kind regards, Shantur
Re: efi: Set Variable Runtime implementation
Hi Simon, On Fri, Dec 1, 2023 at 6:44 PM Simon Glass wrote: > > Hi Shantur, > > On Mon, 27 Nov 2023 at 10:27, Shantur Rathore wrote: > > > > + Simon as he seems to have done a lot of work in the driver model. > > > > On Mon, Nov 27, 2023 at 10:12 AM Shantur Rathore wrote: > > > > > > Hi Ilias, > > > > > > On Mon, Nov 27, 2023 at 7:16 AM Ilias Apalodimas > > > wrote: > > > > > > > > Hi Shantur > > > > > > > > On Sun, 26 Nov 2023 at 12:33, Shantur Rathore wrote: > > > > > > > > > > Hi Peter, > > > > > > > > > > On Sat, Nov 25, 2023 at 6:19 AM Peter Robinson > > > > > wrote: > > > > > > > > > > > > Hi Shantur, > > > > > > > > > > > > On Fri, Nov 24, 2023 at 11:55 PM Shantur Rathore > > > > > > wrote: > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > > > On Fri, Nov 24, 2023 at 10:50 PM Ilias Apalodimas > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Shantur > > > > > > > > > > > > > > > > On Fri, 24 Nov 2023 at 18:51, Shantur Rathore > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Hi Heinrich, > > > > > > > > > > > > > > > > > > I am trying to work out how to enable the SetVariableRT > > > > > > > > > service in > > > > > > > > > U-Boot and came across your patch [1] which initially had the > > > > > > > > > SetVariable RT service enabled in EFI but in the final patch > > > > > > > > > this was > > > > > > > > > removed. > > > > > > > > > I am hoping to implement it on top of the SPI Flash EFI store > > > > > > > > > [2] to > > > > > > > > > be able to set Boot order and boot items from Linux the UEFI > > > > > > > > > way. > > > > > > > > > > > > > > > > > > Can I pick your brain on why it was dropped in the patch? > > > > > > > > > Is there any limitation in SetVariableRT service ? > > > > > > > > > > > > > > > > I recently had a talk about it in Plumbers [0]. Generally > > > > > > > > speaking, RT > > > > > > > > + hardware owned by the kernel is a very weird combination > > > > > > > > since you > > > > > > > > can't guarantee exclusive access to the flash or the bus and > > > > > > > > you have > > > > > > > > to preserve a *lot* of code alive in u-boot. > > > > > > > > > > > > > > > > I'll respond to your v1 patchset and we can discuss details > > > > > > > > there as well. > > > > > > > > > > > > > > > > [0] https://lpc.events/event/17/contributions/1653/ > > > > > > > > > > > > > > Thanks for the background and helping me understand the problem. > > > > > > > Makes me wonder how things work in the PC world. > > > > > > > U-boot being only ~1MB, can we not leave it all in memory and > > > > > > > maybe > > > > > > > just disable SPI access to Linux. > > > > > > > > That would work, but you cant guarantee Linux wont enable the SPI flash. > > > > > > > > > > > > > > > > That's basically it, on x86 there's specific HW that's owned by > > > > > > firmware, I don't know the exact low level details of how that > > > > > > works. > > > > > > > > > > > > I think x86 devices generally use eSPI for this HW [1] but I don't > > > > > > know the low level details. The Arm SBSA (Server HW spec) and SBBR > > > > > > (Server Base Boot Requirements) specs that are key to ServerReady > > > > > > may > > > > > > go into some details too if you're curious. > > > > > > > > On X86 the SPI flash is handled entirely by the firmware and SMM. You > > > >
Re: [PATCH v3 2/3] efi_vars: Implement SPI Flash store
Hi Ilias,
On Thu, Nov 30, 2023 at 10:10 PM Ilias Apalodimas
wrote:
>
> Hi Shantur
>
> I have a few remarks on the architecture.
> Up to now, we are supporting
> 1. Variables on a file
> 2. Variables on an RPMB
>
> The reason those two are in different files is that we generally
> expect to use different bootime services and few differences in
> efi_variables_boot_exit_notify() and efi_init_variables().
> Whatever is common, for example the runtime functions are common
> across those two since they both implement variable runtime service
> via the memory backend, goes into efi_var_common.c, the memory backend
> operations should go in efi_var_mem.c.
>
> Since the SPI and file storage are similar -- and probably any storage
> medium controlled by the non-secure world, I am fine treating treat
> efi_variable.c as the variable management for the non-secure world and
> see if that needs changing in the future.
>
> As Heinrich pointed out the functions you are moving are better off
> moved into efi_var_mem.c.
>
> [...]
Happy to move them efi_var_mem.c
I will do that as part of v4
>
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index adc5ac6a80..f73fb40061 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -354,14 +354,16 @@ efi_status_t efi_set_variable_int(const u16
> > *variable_name,
> > ret = EFI_SUCCESS;
> >
> > /*
> > -* Write non-volatile EFI variables to file
> > +* Write non-volatile EFI variables to file or SPI Flash
> > * TODO: check if a value change has occured to avoid superfluous
> > writes
> > */
> > -#if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
> > if (attributes & EFI_VARIABLE_NON_VOLATILE) {
> > +#if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
> > efi_var_to_file();
> > - }
> > +#elif CONFIG_IS_ENABLED(EFI_VARIABLE_SF_STORE)
> > + efi_var_to_sf();
> > #endif
> > + }
> >
> > return EFI_SUCCESS;
> > }
> > @@ -471,9 +473,14 @@ efi_status_t efi_init_variables(void)
> >
> > #if CONFIG_IS_ENABLED(EFI_VARIABLE_FILE_STORE)
> > ret = efi_var_from_file();
> > +#elif CONFIG_IS_ENABLED(EFI_VARIABLE_SF_STORE)
> > + ret = efi_var_from_sf();
> > +#else
> > + ret = EFI_SUCCESS;
> > +#endif
>
> Instead of those ifdefs can we do something different?
> Define a structure with two function callbacks for read/write(). Add
> the same function in both efi_var_file.c and efi_var_sp.c which fills
> in those callbacks and have an efi_init_variable() call to that
> function (obviously the SPI and file will be mutually exclusive)
>
Sure, will add to v4 too.
Thanks,
Shantur
Re: [PATCH v2 1/3] efi_var_file: refactor to move buffer functions
Hi Heinrich,
On Wed, Nov 29, 2023 at 5:41 PM Heinrich Schuchardt wrote:
>
> On 24.11.23 12:35, Shantur Rathore wrote:
> > Currently efi_var_file.c has functions to store/read
> > EFI variables to/from memory buffer. These functions
> > can be used with other EFI variable stores so move
> > them out to efi_var_common.c
>
> The moved functions relate to filling the memory buffer of variables.
> Wouldn't lib/efi_loader/efi_var_mem.c be more appropriate as target file?
>
As I understand, efi_var_mem.c implements an in-memory store of all
EFI variables.
These functions build a / read from a buffer for writing to an
in-secure storage device ( file / SPI flash ).
The functions are only used by efi_var_sf and efi_var_file hence in
efi_var_common.
Happy to change if you want.
Kind regards,
Shantur
> Best regards
>
> Heinrich
>
> >
> > Signed-off-by: Shantur Rathore
> > ---
> >
> > (no changes since v1)
> >
> > include/efi_variable.h | 5 ++
> > lib/efi_loader/Makefile | 2 +-
> > lib/efi_loader/efi_var_common.c | 109
> > lib/efi_loader/efi_var_file.c | 121
> > lib/efi_loader/efi_variable.c | 8 ++-
> > 5 files changed, 122 insertions(+), 123 deletions(-)
> >
> > diff --git a/include/efi_variable.h b/include/efi_variable.h
> > index 805e6c5f1e..bd0a31fc3e 100644
> > --- a/include/efi_variable.h
> > +++ b/include/efi_variable.h
> > @@ -161,6 +161,11 @@ efi_status_t efi_var_to_file(void);
> > efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp,
> > loff_t *lenp,
> > u32 check_attr_mask);
> >
> > +/* GUID used by Shim to store the MOK database */
> > +#define SHIM_LOCK_GUID \
> > + EFI_GUID(0x605dab50, 0xe046, 0x4300, \
> > + 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
> > +
> > /**
> >* efi_var_restore() - restore EFI variables from buffer
> >*
> > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > index 8d31fc61c6..33b1910249 100644
> > --- a/lib/efi_loader/Makefile
> > +++ b/lib/efi_loader/Makefile
> > @@ -66,7 +66,7 @@ obj-y += efi_string.o
> > obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
> > obj-y += efi_var_common.o
> > obj-y += efi_var_mem.o
> > -obj-y += efi_var_file.o
> > +obj-$(CONFIG_EFI_VARIABLE_FILE_STORE) += efi_var_file.o
> > ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
> > obj-y += efi_variable_tee.o
> > else
> > diff --git a/lib/efi_loader/efi_var_common.c
> > b/lib/efi_loader/efi_var_common.c
> > index ad50bffd2b..7509c30b5a 100644
> > --- a/lib/efi_loader/efi_var_common.c
> > +++ b/lib/efi_loader/efi_var_common.c
> > @@ -10,6 +10,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > enum efi_secure_mode {
> > EFI_MODE_SETUP,
> > @@ -40,6 +41,7 @@ static const struct efi_auth_var_name_type name_type[] = {
> >
> > static bool efi_secure_boot;
> > static enum efi_secure_mode efi_secure_mode;
> > +static const efi_guid_t shim_lock_guid = SHIM_LOCK_GUID;
> >
> > /**
> >* efi_efi_get_variable() - retrieve value of a UEFI variable
> > @@ -417,3 +419,110 @@ void *efi_get_var(const u16 *name, const efi_guid_t
> > *vendor, efi_uintn_t *size)
> >
> > return buf;
> > }
> > +
> > +efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp,
> > loff_t *lenp,
> > + u32 check_attr_mask)
> > +{
> > + size_t len = EFI_VAR_BUF_SIZE;
> > + struct efi_var_file *buf;
> > + struct efi_var_entry *var, *old_var;
> > + size_t old_var_name_length = 2;
> > +
> > + *bufp = NULL; /* Avoid double free() */
> > + buf = calloc(1, len);
> > + if (!buf)
> > + return EFI_OUT_OF_RESOURCES;
> > + var = buf->var;
> > + old_var = var;
> > + for (;;) {
> > + efi_uintn_t data_length, var_name_length;
> > + u8 *data;
> > + efi_status_t ret;
> > +
> > + if ((uintptr_t)buf + len <=
> > + (uintptr_t)var->name + old_var_name_length)
> > + return EFI_BUFFER_TOO_SMALL;
> > +
> > + var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name;
> > + memcpy(var->name, old_var->name, old_var_name
Re: efi: Set Variable Runtime implementation
+ Simon as he seems to have done a lot of work in the driver model. On Mon, Nov 27, 2023 at 10:12 AM Shantur Rathore wrote: > > Hi Ilias, > > On Mon, Nov 27, 2023 at 7:16 AM Ilias Apalodimas > wrote: > > > > Hi Shantur > > > > On Sun, 26 Nov 2023 at 12:33, Shantur Rathore wrote: > > > > > > Hi Peter, > > > > > > On Sat, Nov 25, 2023 at 6:19 AM Peter Robinson > > > wrote: > > > > > > > > Hi Shantur, > > > > > > > > On Fri, Nov 24, 2023 at 11:55 PM Shantur Rathore > > > > wrote: > > > > > > > > > > Hi Ilias, > > > > > > > > > > On Fri, Nov 24, 2023 at 10:50 PM Ilias Apalodimas > > > > > wrote: > > > > > > > > > > > > Hi Shantur > > > > > > > > > > > > On Fri, 24 Nov 2023 at 18:51, Shantur Rathore > > > > > > wrote: > > > > > > > > > > > > > > Hi Heinrich, > > > > > > > > > > > > > > I am trying to work out how to enable the SetVariableRT service in > > > > > > > U-Boot and came across your patch [1] which initially had the > > > > > > > SetVariable RT service enabled in EFI but in the final patch this > > > > > > > was > > > > > > > removed. > > > > > > > I am hoping to implement it on top of the SPI Flash EFI store [2] > > > > > > > to > > > > > > > be able to set Boot order and boot items from Linux the UEFI way. > > > > > > > > > > > > > > Can I pick your brain on why it was dropped in the patch? > > > > > > > Is there any limitation in SetVariableRT service ? > > > > > > > > > > > > I recently had a talk about it in Plumbers [0]. Generally speaking, > > > > > > RT > > > > > > + hardware owned by the kernel is a very weird combination since you > > > > > > can't guarantee exclusive access to the flash or the bus and you > > > > > > have > > > > > > to preserve a *lot* of code alive in u-boot. > > > > > > > > > > > > I'll respond to your v1 patchset and we can discuss details there > > > > > > as well. > > > > > > > > > > > > [0] https://lpc.events/event/17/contributions/1653/ > > > > > > > > > > Thanks for the background and helping me understand the problem. > > > > > Makes me wonder how things work in the PC world. > > > > > U-boot being only ~1MB, can we not leave it all in memory and maybe > > > > > just disable SPI access to Linux. > > > > That would work, but you cant guarantee Linux wont enable the SPI flash. > > > > > > > > > > That's basically it, on x86 there's specific HW that's owned by > > > > firmware, I don't know the exact low level details of how that works. > > > > > > > > I think x86 devices generally use eSPI for this HW [1] but I don't > > > > know the low level details. The Arm SBSA (Server HW spec) and SBBR > > > > (Server Base Boot Requirements) specs that are key to ServerReady may > > > > go into some details too if you're curious. > > > > On X86 the SPI flash is handled entirely by the firmware and SMM. You > > can find more details here [0] > > Thanks for more info. > > > > > > > > > Thanks, > > > I think the firmware is still accessible to PCs as one could update the > > > firmware > > > in Windows so Windows has access to that device. > > > > > > I had some try myself and found that setting a variable to memory backed > > > storage > > > is doable with SetVariable call but we want to store it in any > > > non-volatile storage > > > things really don't look good. > > > > > > To be able to write SetVariable to any device, the whole u-boot driver > > > model would need > > > to be kept in memory, might as well just keep the whole u-boot in > > > memory at this point, it's anyway small. > > > I don't have much knowledge on how to or pros and cons of doing this. > > > > The major problem here is who owns the hardware. With the SPI flash > > implementation as well as the RPMB implementation Linux owns that > > flash. > > For the RPMB we've introduced a mechanism
Re: efi: Set Variable Runtime implementation
Hi Ilias, On Mon, Nov 27, 2023 at 7:16 AM Ilias Apalodimas wrote: > > Hi Shantur > > On Sun, 26 Nov 2023 at 12:33, Shantur Rathore wrote: > > > > Hi Peter, > > > > On Sat, Nov 25, 2023 at 6:19 AM Peter Robinson wrote: > > > > > > Hi Shantur, > > > > > > On Fri, Nov 24, 2023 at 11:55 PM Shantur Rathore wrote: > > > > > > > > Hi Ilias, > > > > > > > > On Fri, Nov 24, 2023 at 10:50 PM Ilias Apalodimas > > > > wrote: > > > > > > > > > > Hi Shantur > > > > > > > > > > On Fri, 24 Nov 2023 at 18:51, Shantur Rathore > > > > > wrote: > > > > > > > > > > > > Hi Heinrich, > > > > > > > > > > > > I am trying to work out how to enable the SetVariableRT service in > > > > > > U-Boot and came across your patch [1] which initially had the > > > > > > SetVariable RT service enabled in EFI but in the final patch this > > > > > > was > > > > > > removed. > > > > > > I am hoping to implement it on top of the SPI Flash EFI store [2] to > > > > > > be able to set Boot order and boot items from Linux the UEFI way. > > > > > > > > > > > > Can I pick your brain on why it was dropped in the patch? > > > > > > Is there any limitation in SetVariableRT service ? > > > > > > > > > > I recently had a talk about it in Plumbers [0]. Generally speaking, RT > > > > > + hardware owned by the kernel is a very weird combination since you > > > > > can't guarantee exclusive access to the flash or the bus and you have > > > > > to preserve a *lot* of code alive in u-boot. > > > > > > > > > > I'll respond to your v1 patchset and we can discuss details there as > > > > > well. > > > > > > > > > > [0] https://lpc.events/event/17/contributions/1653/ > > > > > > > > Thanks for the background and helping me understand the problem. > > > > Makes me wonder how things work in the PC world. > > > > U-boot being only ~1MB, can we not leave it all in memory and maybe > > > > just disable SPI access to Linux. > > That would work, but you cant guarantee Linux wont enable the SPI flash. > > > > > > > That's basically it, on x86 there's specific HW that's owned by > > > firmware, I don't know the exact low level details of how that works. > > > > > > I think x86 devices generally use eSPI for this HW [1] but I don't > > > know the low level details. The Arm SBSA (Server HW spec) and SBBR > > > (Server Base Boot Requirements) specs that are key to ServerReady may > > > go into some details too if you're curious. > > On X86 the SPI flash is handled entirely by the firmware and SMM. You > can find more details here [0] Thanks for more info. > > > > > Thanks, > > I think the firmware is still accessible to PCs as one could update the > > firmware > > in Windows so Windows has access to that device. > > > > I had some try myself and found that setting a variable to memory backed > > storage > > is doable with SetVariable call but we want to store it in any > > non-volatile storage > > things really don't look good. > > > > To be able to write SetVariable to any device, the whole u-boot driver > > model would need > > to be kept in memory, might as well just keep the whole u-boot in > > memory at this point, it's anyway small. > > I don't have much knowledge on how to or pros and cons of doing this. > > The major problem here is who owns the hardware. With the SPI flash > implementation as well as the RPMB implementation Linux owns that > flash. > For the RPMB we've introduced a mechanism so the kernel replaces the > runtime calls with internal functions [1]. I think we should come up > with a similar architecture for SPI. In any case we should keep in > mind that setting authenticated EFI variables should be forbidden on > the file/SPI backends since they are not really secure. > Thanks, I understand now that we can't use SPI flash for saving secure variables and stop Linux from accessing it. My requirement is to be able to save non-Secure boot related variables ( BootOrder, BootNext and BootOptions ). For this purpose as we don't need secure channel and to be compatible with current Linux versions I started implementing SetVariable in runtime in [1] I was able to get it working until it's ready to write stuff into SPI Flash. To be able to use SPI Flash, runtime call needs to access the drivers and this needs the whole driver-model to be in efi runtime memory makes me think would it make sense to keep the whole u-boot in efi runtime memory or just driver model and all SPI drivers for now and keep adding other drivers when needed like RPMB. I need some pointers to what would be the best approach accessing hardware from runtime. [1] - https://github.com/shantur/u-boot/tree/spi-flash-runtime-setvariable Kind regards, Shantur
Re: [PATCH v1 2/3] efi_vars: Implement SPI Flash store
Hi Ilias,
On Mon, Nov 27, 2023 at 7:09 AM Ilias Apalodimas
wrote:
>
> Hi Shantur
>
> Please don't send a v2 unless the v1 discussion has settled. It just
> makes life harder. I'll ignore v2 for now and respond here.
>
Sure, I'm still learning the ways around here.
> [...]
>
> >
> > >
> > > > + if (ret)
> > > > + goto error;
> > > > +
> > > > + ret = spi_flash_erase_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET,
> > > > EFI_VAR_BUF_SIZE);
> > > > + debug("%s - Erased SPI Flash offset %lx\n", __func__,
> > > > CONFIG_EFI_VARIABLE_SF_OFFSET);
> > > > + if (ret)
> > > > + goto error;
> > > > +
> > > > + ret = spi_flash_write_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET,
> > > > len, buf);
> > > > + debug("%s - Wrote buffer to SPI Flash : %ld\n", __func__, ret);
> > > > +
> > > > + if (ret)
> > > > + goto error;
> > > > +
> > > > + ret = EFI_SUCCESS;
> > > > +error:
> > > > + if (ret)
> > > > + log_err("Failed to persist EFI variables in SF\n");
> > > > + free(buf);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +efi_status_t efi_var_from_sf(void)
> > > > +{
> > > > + struct efi_var_file *buf;
> > > > + efi_status_t ret;
> > > > + struct udevice *sfdev;
> > > > +
> > > > + buf = calloc(1, EFI_VAR_BUF_SIZE);
> > > > + if (!buf) {
> > > > + log_err("Out of memory\n");
> > > > + return EFI_OUT_OF_RESOURCES;
> > > > + }
> > >
> > > You don't want normal memory for the variables. Instead, you want EFI
> > > memory which is marked for EFI runtime services data. U-Boot already
> > > has code to support Get/GetNextvariableRT, you just need to use
> > > efi_var_mem_init().
> > > If you need hints have a look at lib/efi_loader/efi_variable_tee.c
> > > which implements variables stored in an RPMB, but still uses the
> > > memory backend for runtime services.
> > >
> >
> > I believe EFI Runtime stuff is handled by efi_variable.c functions.
>
> Yes, they are, but there is a subtle difference here.
> efi_variable.c and efi_variable_tee.c have their own version of
> runtime service implementation. The functions that can be shared
> across the two are in efi_var_common.c.
> What your patch does is shoehorn an SPI backend storage to the
> existing implementation for storing variables to a file. Depending on
> what we decide for runtime calls that can end up being messy.
>
> efi_variable.c -> deals with variables on a file
> efi_variable_tee.c -> deals with variables hidden behind the secure world
>
> I think it would be cleaner to add efi_variable_sf.c and move the
> common functions you need from efi_variable.c -> efi_var_common.c
> Heinrich, do you have a preference for that?
I agree, the v1 patch was not implementing it cleanly.
Based on Akashi's comments I made these changes for v2 ( v3 with some
compiler warning fixes )
- Moved common stuff out of efi_var_file.c to efi_var_common.c
- Added efi_var_sf.c to handle writing vars to SPI Flash as
efi_var_file.c does for File
Happy to change things if needed.
Kind regards,
Shantur
[PATCH v3 3/3] defconfig: rockpro64: Enable SF EFI var store
RockPro64 uses SPI Flash for storing env, also use it store EFI variables. Signed-off-by: Shantur Rathore --- configs/rockpro64-rk3399_defconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig index 4cd6b76665..f550f2ae43 100644 --- a/configs/rockpro64-rk3399_defconfig +++ b/configs/rockpro64-rk3399_defconfig @@ -8,6 +8,8 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x30 CONFIG_ENV_SIZE=0x8000 CONFIG_ENV_OFFSET=0x3F8000 +CONFIG_EFI_VARIABLE_SF_STORE=y +CONFIG_EFI_VARIABLE_SF_OFFSET=0x7D CONFIG_DEFAULT_DEVICE_TREE="rk3399-rockpro64" CONFIG_DM_RESET=y CONFIG_ROCKCHIP_RK3399=y -- 2.40.1
[PATCH v3 1/3] efi_var_file: refactor to move buffer functions
Currently efi_var_file.c has functions to store/read
EFI variables to/from memory buffer. These functions
can be used with other EFI variable stores so move
them out to efi_var_common.c
Signed-off-by: Shantur Rathore
---
(no changes since v1)
include/efi_variable.h | 5 ++
lib/efi_loader/Makefile | 2 +-
lib/efi_loader/efi_var_common.c | 109
lib/efi_loader/efi_var_file.c | 121
lib/efi_loader/efi_variable.c | 8 ++-
5 files changed, 122 insertions(+), 123 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h
index 805e6c5f1e..bd0a31fc3e 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -161,6 +161,11 @@ efi_status_t efi_var_to_file(void);
efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t
*lenp,
u32 check_attr_mask);
+/* GUID used by Shim to store the MOK database */
+#define SHIM_LOCK_GUID \
+ EFI_GUID(0x605dab50, 0xe046, 0x4300, \
+0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
+
/**
* efi_var_restore() - restore EFI variables from buffer
*
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 8d31fc61c6..33b1910249 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -66,7 +66,7 @@ obj-y += efi_string.o
obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
obj-y += efi_var_common.o
obj-y += efi_var_mem.o
-obj-y += efi_var_file.o
+obj-$(CONFIG_EFI_VARIABLE_FILE_STORE) += efi_var_file.o
ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
obj-y += efi_variable_tee.o
else
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
index ad50bffd2b..7509c30b5a 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
enum efi_secure_mode {
EFI_MODE_SETUP,
@@ -40,6 +41,7 @@ static const struct efi_auth_var_name_type name_type[] = {
static bool efi_secure_boot;
static enum efi_secure_mode efi_secure_mode;
+static const efi_guid_t shim_lock_guid = SHIM_LOCK_GUID;
/**
* efi_efi_get_variable() - retrieve value of a UEFI variable
@@ -417,3 +419,110 @@ void *efi_get_var(const u16 *name, const efi_guid_t
*vendor, efi_uintn_t *size)
return buf;
}
+
+efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t
*lenp,
+ u32 check_attr_mask)
+{
+ size_t len = EFI_VAR_BUF_SIZE;
+ struct efi_var_file *buf;
+ struct efi_var_entry *var, *old_var;
+ size_t old_var_name_length = 2;
+
+ *bufp = NULL; /* Avoid double free() */
+ buf = calloc(1, len);
+ if (!buf)
+ return EFI_OUT_OF_RESOURCES;
+ var = buf->var;
+ old_var = var;
+ for (;;) {
+ efi_uintn_t data_length, var_name_length;
+ u8 *data;
+ efi_status_t ret;
+
+ if ((uintptr_t)buf + len <=
+ (uintptr_t)var->name + old_var_name_length)
+ return EFI_BUFFER_TOO_SMALL;
+
+ var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name;
+ memcpy(var->name, old_var->name, old_var_name_length);
+ guidcpy(>guid, _var->guid);
+ ret = efi_get_next_variable_name_int(
+ _name_length, var->name, >guid);
+ if (ret == EFI_NOT_FOUND)
+ break;
+ if (ret != EFI_SUCCESS) {
+ free(buf);
+ return ret;
+ }
+ old_var_name_length = var_name_length;
+ old_var = var;
+
+ data = (u8 *)var->name + old_var_name_length;
+ data_length = (uintptr_t)buf + len - (uintptr_t)data;
+ ret = efi_get_variable_int(var->name, >guid,
+ >attr, _length, data,
+ >time);
+ if (ret != EFI_SUCCESS) {
+ free(buf);
+ return ret;
+ }
+ if ((var->attr & check_attr_mask) == check_attr_mask) {
+ var->length = data_length;
+ var = (struct efi_var_entry *)ALIGN((uintptr_t)data +
data_length, 8);
+ }
+ }
+
+ buf->reserved = 0;
+ buf->magic = EFI_VAR_FILE_MAGIC;
+ len = (uintptr_t)var - (uintptr_t)buf;
+ buf->crc32 = crc32(0, (u8 *)buf->var,
+ len - sizeof(struct efi_var_file));
+ buf->length = len;
+ *bufp = buf;
+ *lenp = len;
+
+ return EFI_SUCCESS;
+}
+
+efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
+{
+ struct efi_var_entry *var, *la
[PATCH v3 2/3] efi_vars: Implement SPI Flash store
Currently U-boot uses ESP as storage for EFI variables.
Devices with SPI Flash are used for storing environment with this
commit we allow EFI variables to be stored on SPI Flash.
Signed-off-by: Shantur Rathore
---
(no changes since v1)
include/efi_variable.h| 21
lib/efi_loader/Kconfig| 25 ++
lib/efi_loader/Makefile | 1 +
lib/efi_loader/efi_var_sf.c | 91 +++
lib/efi_loader/efi_variable.c | 15 --
5 files changed, 149 insertions(+), 4 deletions(-)
create mode 100644 lib/efi_loader/efi_var_sf.c
diff --git a/include/efi_variable.h b/include/efi_variable.h
index bd0a31fc3e..f90bbb0548 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -190,6 +190,27 @@ efi_status_t efi_var_restore(struct efi_var_file *buf,
bool safe);
*/
efi_status_t efi_var_from_file(void);
+/**
+ * efi_var_from_sf() - read variables from SPI Flash
+ *
+ * EFI variable buffer is read from SPI Flash at offset defined with
+ * CONFIG_EFI_VARIABLE_SF_OFFSET of length CONFIG_EFI_VAR_BUF_SIZE
+ *
+ *
+ * Return: status code
+ */
+efi_status_t efi_var_from_sf(void);
+
+/**
+ * efi_var_to_sf() - save non-volatile variables to SPI Flash
+ *
+ * EFI variable buffer is saved to SPI Flash at offset defined with
+ * CONFIG_EFI_VARIABLE_SF_OFFSET of length CONFIG_EFI_VAR_BUF_SIZE.
+ *
+ * Return: status code
+ */
+efi_status_t efi_var_to_sf(void);
+
/**
* efi_var_mem_init() - set-up variable list
*
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 2e3935467c..d3200cb3e3 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -54,6 +54,17 @@ config EFI_VARIABLE_FILE_STORE
Select this option if you want non-volatile UEFI variables to be
stored as file /ubootefi.var on the EFI system partition.
+config EFI_VARIABLE_SF_STORE
+ bool "Store non-volatile UEFI variables in SPI Flash"
+ depends on SPI_FLASH
+ help
+ Select this option if you want non-volatile UEFI variables to be
+ stored in SPI Flash.
+ Define CONFIG_EFI_VARIABLE_SF_OFFSET as offset in SPI Flash to use as
+ the storage for variables. CONFIG_EFI_VAR_BUF_SIZE defines the space
+ needed.
+
+
config EFI_MM_COMM_TEE
bool "UEFI variables storage service via the trusted world"
depends on OPTEE
@@ -108,6 +119,20 @@ config EFI_VARIABLE_NO_STORE
endchoice
+config EFI_VARIABLE_SF_OFFSET
+ hex "EFI variables in SPI flash offset"
+ depends on EFI_VARIABLE_SF_STORE
+ help
+ Offset from the start of the SPI Flash where EFI variables will be
stored.
+ This should be aligned to the sector size of SPI Flash.
+
+config EFI_VARIABLE_SF_DEVICE_INDEX
+ int "Device Index for target SPI Flash"
+ default 0
+ help
+ The index of SPI Flash device used for storing EFI variables. This
would be
+ needed if there are more than 1 SPI Flash devices available to use.
+
config EFI_VARIABLES_PRESEED
bool "Initial values for UEFI variables"
depends on !EFI_MM_COMM_TEE
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 33b1910249..823e0a759f 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) +=
efi_unicode_collation.o
obj-y += efi_var_common.o
obj-y += efi_var_mem.o
obj-$(CONFIG_EFI_VARIABLE_FILE_STORE) += efi_var_file.o
+obj-$(CONFIG_EFI_VARIABLE_SF_STORE) += efi_var_sf.o
ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
obj-y += efi_variable_tee.o
else
diff --git a/lib/efi_loader/efi_var_sf.c b/lib/efi_loader/efi_var_sf.c
new file mode 100644
index 00..79ab99640c
--- /dev/null
+++ b/lib/efi_loader/efi_var_sf.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * SPI Flash interface for UEFI variables
+ *
+ * Copyright (c) 2023, Shantur Rathore
+ */
+
+#define LOG_CATEGORY LOGC_EFI
+
+#include
+#include
+#include
+#include
+
+efi_status_t efi_var_to_sf(void)
+{
+ efi_status_t ret;
+ struct efi_var_file *buf;
+ loff_t len;
+ struct udevice *sfdev;
+
+ ret = efi_var_collect(, , EFI_VARIABLE_NON_VOLATILE);
+ if (len > EFI_VAR_BUF_SIZE) {
+ log_err("EFI var buffer length more than target SPI Flash
size");
+ ret = EFI_OUT_OF_RESOURCES;
+ goto error;
+ }
+
+ log_debug("%s - Got buffer to write buf->len : %d\n", __func__,
buf->length);
+
+ if (ret != EFI_SUCCESS)
+ goto error;
+
+ ret = uclass_get_device(UCLASS_SPI_FLASH,
CONFIG_EFI_VARIABLE_SF_DEVICE_INDEX, );
+ if (ret)
+ goto error;
+
+ ret = spi_flash_erase_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET,
EFI_VAR_BUF_SIZE);
+ log_debug("%s - Erased SPI Flash offset %x\n", __func__,
CONFIG_EFI_VARIABLE_SF_OFFSE
[PATCH v3 0/3] With this patch series we implement SPI Flash storage for EFI
variables. Changes in v3: - Fixed compiler warnings. Changes in v2: - Refactored efi_var_file to move common parts out as requested - Changed ifdefs to use CONFIG_IS_DEFINED - Fixed typos Shantur Rathore (3): efi_var_file: refactor to move buffer functions efi_vars: Implement SPI Flash store defconfig: rockpro64: Enable SF EFI var store configs/rockpro64-rk3399_defconfig | 2 + include/efi_variable.h | 26 +++ lib/efi_loader/Kconfig | 25 ++ lib/efi_loader/Makefile| 3 +- lib/efi_loader/efi_var_common.c| 109 ++ lib/efi_loader/efi_var_file.c | 121 - lib/efi_loader/efi_var_sf.c| 91 ++ lib/efi_loader/efi_variable.c | 17 +++- 8 files changed, 270 insertions(+), 124 deletions(-) create mode 100644 lib/efi_loader/efi_var_sf.c -- 2.40.1
Re: efi: Set Variable Runtime implementation
Hi Peter, On Sat, Nov 25, 2023 at 6:19 AM Peter Robinson wrote: > > Hi Shantur, > > On Fri, Nov 24, 2023 at 11:55 PM Shantur Rathore wrote: > > > > Hi Ilias, > > > > On Fri, Nov 24, 2023 at 10:50 PM Ilias Apalodimas > > wrote: > > > > > > Hi Shantur > > > > > > On Fri, 24 Nov 2023 at 18:51, Shantur Rathore wrote: > > > > > > > > Hi Heinrich, > > > > > > > > I am trying to work out how to enable the SetVariableRT service in > > > > U-Boot and came across your patch [1] which initially had the > > > > SetVariable RT service enabled in EFI but in the final patch this was > > > > removed. > > > > I am hoping to implement it on top of the SPI Flash EFI store [2] to > > > > be able to set Boot order and boot items from Linux the UEFI way. > > > > > > > > Can I pick your brain on why it was dropped in the patch? > > > > Is there any limitation in SetVariableRT service ? > > > > > > I recently had a talk about it in Plumbers [0]. Generally speaking, RT > > > + hardware owned by the kernel is a very weird combination since you > > > can't guarantee exclusive access to the flash or the bus and you have > > > to preserve a *lot* of code alive in u-boot. > > > > > > I'll respond to your v1 patchset and we can discuss details there as well. > > > > > > [0] https://lpc.events/event/17/contributions/1653/ > > > > Thanks for the background and helping me understand the problem. > > Makes me wonder how things work in the PC world. > > U-boot being only ~1MB, can we not leave it all in memory and maybe > > just disable SPI access to Linux. > > That's basically it, on x86 there's specific HW that's owned by > firmware, I don't know the exact low level details of how that works. > > I think x86 devices generally use eSPI for this HW [1] but I don't > know the low level details. The Arm SBSA (Server HW spec) and SBBR > (Server Base Boot Requirements) specs that are key to ServerReady may > go into some details too if you're curious. Thanks, I think the firmware is still accessible to PCs as one could update the firmware in Windows so Windows has access to that device. I had some try myself and found that setting a variable to memory backed storage is doable with SetVariable call but we want to store it in any non-volatile storage things really don't look good. To be able to write SetVariable to any device, the whole u-boot driver model would need to be kept in memory, might as well just keep the whole u-boot in memory at this point, it's anyway small. I don't have much knowledge on how to or pros and cons of doing this. > > BTW I plan to test your other patches on the Pinebook Pro. > > [1] > https://www.totalphase.com/blog/2021/09/what-is-the-espi-protocol-and-how-does-it-improve-upon-lpc/ Thanks, that would be helpful. I am particularly keen on the usb patch here https://patchwork.ozlabs.org/project/uboot/patch/20231110141311.512334-...@shantur.com/ Kind regards, Shantur
Re: efi: Set Variable Runtime implementation
Hi Ilias, On Fri, Nov 24, 2023 at 10:50 PM Ilias Apalodimas wrote: > > Hi Shantur > > On Fri, 24 Nov 2023 at 18:51, Shantur Rathore wrote: > > > > Hi Heinrich, > > > > I am trying to work out how to enable the SetVariableRT service in > > U-Boot and came across your patch [1] which initially had the > > SetVariable RT service enabled in EFI but in the final patch this was > > removed. > > I am hoping to implement it on top of the SPI Flash EFI store [2] to > > be able to set Boot order and boot items from Linux the UEFI way. > > > > Can I pick your brain on why it was dropped in the patch? > > Is there any limitation in SetVariableRT service ? > > I recently had a talk about it in Plumbers [0]. Generally speaking, RT > + hardware owned by the kernel is a very weird combination since you > can't guarantee exclusive access to the flash or the bus and you have > to preserve a *lot* of code alive in u-boot. > > I'll respond to your v1 patchset and we can discuss details there as well. > > [0] https://lpc.events/event/17/contributions/1653/ Thanks for the background and helping me understand the problem. Makes me wonder how things work in the PC world. U-boot being only ~1MB, can we not leave it all in memory and maybe just disable SPI access to Linux. I might be talking rubbish, just thinking what weird ways it can be worked. Regards, Shantur
efi: Set Variable Runtime implementation
Hi Heinrich, I am trying to work out how to enable the SetVariableRT service in U-Boot and came across your patch [1] which initially had the SetVariable RT service enabled in EFI but in the final patch this was removed. I am hoping to implement it on top of the SPI Flash EFI store [2] to be able to set Boot order and boot items from Linux the UEFI way. Can I pick your brain on why it was dropped in the patch? Is there any limitation in SetVariableRT service ? Kind regards, Shantur [1] : https://patches.linaro.org/project/u-boot/patch/20200331060541.4212-1-xypron.g...@gmx.de/ [2] : https://patchwork.ozlabs.org/project/uboot/list/?series=383699
[PATCH] maintainers: rk3399: remove maintainer
Remove Akash Gajjar from MAINTAINERS as email is bouncing. Signed-off-by: Shantur Rathore --- board/pine64/rockpro64_rk3399/MAINTAINERS | 1 - board/rockchip/evb_rk3399/MAINTAINERS | 1 - 2 files changed, 2 deletions(-) diff --git a/board/pine64/rockpro64_rk3399/MAINTAINERS b/board/pine64/rockpro64_rk3399/MAINTAINERS index 303db144aa..220ee21f23 100644 --- a/board/pine64/rockpro64_rk3399/MAINTAINERS +++ b/board/pine64/rockpro64_rk3399/MAINTAINERS @@ -1,5 +1,4 @@ ROCKPRO64 -M: Akash Gajjar M: Jagan Teki S: Maintained F: board/pine64/rockpro64_rk3399 diff --git a/board/rockchip/evb_rk3399/MAINTAINERS b/board/rockchip/evb_rk3399/MAINTAINERS index c7e412b54e..acdb840f20 100644 --- a/board/rockchip/evb_rk3399/MAINTAINERS +++ b/board/rockchip/evb_rk3399/MAINTAINERS @@ -93,7 +93,6 @@ F:configs/rock-4se-rk3399_defconfig F: arch/arm/dts/rk3399-rock-4se-u-boot.dtsi ROCK-PI-4 -M: Akash Gajjar M: Jagan Teki S: Maintained F: configs/rock-pi-4-rk3399_defconfig -- 2.40.1
Re: [PATCH v1 2/3] efi_vars: Implement SPI Flash store
Hi Ilias and Akashi, On Wed, Nov 22, 2023 at 6:58 AM Ilias Apalodimas wrote: > > Hi Shahtur > > On Wed, 22 Nov 2023 at 01:58, Shantur Rathore wrote: > > > > Currently U-boot uses ESP as storage for EFI variables. > > Devices with SPI Flash are used for storing environment with this > > commit we allow EFI variables to be stored on SPI Flash. > > > > Signed-off-by: Shantur Rathore > > --- > > > > include/efi_variable.h| 25 ++ > > lib/efi_loader/Kconfig| 18 +++ > > lib/efi_loader/Makefile | 1 + > > lib/efi_loader/efi_var_sf.c | 91 +++ > > lib/efi_loader/efi_variable.c | 4 ++ > > 5 files changed, 139 insertions(+) > > create mode 100644 lib/efi_loader/efi_var_sf.c > > > > diff --git a/include/efi_variable.h b/include/efi_variable.h > > index ca7e19d514..766dd109f5 100644 > > --- a/include/efi_variable.h > > +++ b/include/efi_variable.h > > @@ -188,6 +188,31 @@ efi_status_t efi_var_from_file(void); > > > > #endif // CONFIG_EFI_VARIABLE_FILE_STORE > > > > +#ifdef CONFIG_EFI_VARIABLE_SF_STORE > > + > > +/** > > + * efi_var_from_sf() - read variables from SPI Flash > > + * > > + * EFI variable buffer is read from SPI Flash at offset defined with > > + * CONFIG_EFI_VARIABLE_SF_OFFSET of length CONFIG_EFI_VAR_BUF_SIZE > > + * > > + * > > + * Return: status code > > + */ > > +efi_status_t efi_var_from_sf(void); > > + > > +/** > > + * efi_var_to_sf() - save non-volatile variables to SPI Flash > > + * > > + * EFI variable buffer is saved to SPI Flash at offset defined with > > + * CONFIG_EFI_VARIABLE_SF_OFFSET of length CONFIG_EFI_VAR_BUF_SIZE. > > + * > > + * Return: status code > > + */ > > +efi_status_t efi_var_to_sf(void); > > + > > +#endif // CONFIG_EFI_VARIABLE_SF_STORE > > + > > /** > > * efi_var_mem_init() - set-up variable list > > * > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > index 4ccd26f94a..1fcc6fabb4 100644 > > --- a/lib/efi_loader/Kconfig > > +++ b/lib/efi_loader/Kconfig > > @@ -54,6 +54,17 @@ config EFI_VARIABLE_FILE_STORE > > Select this option if you want non-volatile UEFI variables to be > > stored as file /ubootefi.var on the EFI system partition. > > > > +config EFI_VARIABLE_SF_STORE > > + bool "Store non-volatile UEFI variables in SPI Flash" > > + depends on SPI_FLASH > > + help > > + Select this option if you want non-volatile UEFI variables to be > > + stored in SPI Flash. > > + Define CONFIG_EFI_VARIABLE_SF_OFFSET as offset in SPI Flash to > > use as > > + the storage for variables. CONFIG_EFI_VAR_BUF_SIZE defines the > > space > > + needed. > > + > > + > > config EFI_MM_COMM_TEE > > bool "UEFI variables storage service via the trusted world" > > depends on OPTEE > > @@ -108,6 +119,13 @@ config EFI_VARIABLE_NO_STORE > > > > endchoice > > > > +config EFI_VARIABLE_SF_OFFSET > > + hex "EFI variables in SPI flash offset" > > + depends on EFI_VARIABLE_SF_STORE > > + default 0x7D > > + help > > + Offset from the start of the SPI Flash where EFI variables will > > be stored. > > + > > config EFI_VARIABLES_PRESEED > > bool "Initial values for UEFI variables" > > depends on !EFI_MM_COMM_TEE > > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile > > index 8d31fc61c6..b9b715b1ff 100644 > > --- a/lib/efi_loader/Makefile > > +++ b/lib/efi_loader/Makefile > > @@ -67,6 +67,7 @@ obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += > > efi_unicode_collation.o > > obj-y += efi_var_common.o > > obj-y += efi_var_mem.o > > obj-y += efi_var_file.o > > +obj-$(CONFIG_EFI_VARIABLE_SF_STORE) += efi_var_sf.o > > ifeq ($(CONFIG_EFI_MM_COMM_TEE),y) > > obj-y += efi_variable_tee.o > > else > > diff --git a/lib/efi_loader/efi_var_sf.c b/lib/efi_loader/efi_var_sf.c > > new file mode 100644 > > index 00..e604a2225c > > --- /dev/null > > +++ b/lib/efi_loader/efi_var_sf.c > > @@ -0,0 +1,91 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * File interface for UEFI variables > > + * > > + * Copyright (c) 2023, Shantur Rtahore > > Name is misspelled > Fixed, thanks. I shouldn't be sending
[PATCH v2 3/3] defconfig: rockpro64: Enable SF EFI var store
RockPro64 uses SPI Flash for storing env, also use it store EFI variables. Signed-off-by: Shantur Rathore --- Changes in v2: - Refactored efi_var_file to move common parts out as requested - Changed ifdefs to use CONFIG_IS_DEFINED - Fixed typos configs/rockpro64-rk3399_defconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig index 4cd6b76665..f550f2ae43 100644 --- a/configs/rockpro64-rk3399_defconfig +++ b/configs/rockpro64-rk3399_defconfig @@ -8,6 +8,8 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x30 CONFIG_ENV_SIZE=0x8000 CONFIG_ENV_OFFSET=0x3F8000 +CONFIG_EFI_VARIABLE_SF_STORE=y +CONFIG_EFI_VARIABLE_SF_OFFSET=0x7D CONFIG_DEFAULT_DEVICE_TREE="rk3399-rockpro64" CONFIG_DM_RESET=y CONFIG_ROCKCHIP_RK3399=y -- 2.40.1
[PATCH v2 2/3] efi_vars: Implement SPI Flash store
Currently U-boot uses ESP as storage for EFI variables.
Devices with SPI Flash are used for storing environment with this
commit we allow EFI variables to be stored on SPI Flash.
Signed-off-by: Shantur Rathore
---
(no changes since v1)
include/efi_variable.h| 21
lib/efi_loader/Kconfig| 25 ++
lib/efi_loader/Makefile | 1 +
lib/efi_loader/efi_var_sf.c | 91 +++
lib/efi_loader/efi_variable.c | 15 --
5 files changed, 149 insertions(+), 4 deletions(-)
create mode 100644 lib/efi_loader/efi_var_sf.c
diff --git a/include/efi_variable.h b/include/efi_variable.h
index bd0a31fc3e..f90bbb0548 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -190,6 +190,27 @@ efi_status_t efi_var_restore(struct efi_var_file *buf,
bool safe);
*/
efi_status_t efi_var_from_file(void);
+/**
+ * efi_var_from_sf() - read variables from SPI Flash
+ *
+ * EFI variable buffer is read from SPI Flash at offset defined with
+ * CONFIG_EFI_VARIABLE_SF_OFFSET of length CONFIG_EFI_VAR_BUF_SIZE
+ *
+ *
+ * Return: status code
+ */
+efi_status_t efi_var_from_sf(void);
+
+/**
+ * efi_var_to_sf() - save non-volatile variables to SPI Flash
+ *
+ * EFI variable buffer is saved to SPI Flash at offset defined with
+ * CONFIG_EFI_VARIABLE_SF_OFFSET of length CONFIG_EFI_VAR_BUF_SIZE.
+ *
+ * Return: status code
+ */
+efi_status_t efi_var_to_sf(void);
+
/**
* efi_var_mem_init() - set-up variable list
*
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 2e3935467c..d3200cb3e3 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -54,6 +54,17 @@ config EFI_VARIABLE_FILE_STORE
Select this option if you want non-volatile UEFI variables to be
stored as file /ubootefi.var on the EFI system partition.
+config EFI_VARIABLE_SF_STORE
+ bool "Store non-volatile UEFI variables in SPI Flash"
+ depends on SPI_FLASH
+ help
+ Select this option if you want non-volatile UEFI variables to be
+ stored in SPI Flash.
+ Define CONFIG_EFI_VARIABLE_SF_OFFSET as offset in SPI Flash to use as
+ the storage for variables. CONFIG_EFI_VAR_BUF_SIZE defines the space
+ needed.
+
+
config EFI_MM_COMM_TEE
bool "UEFI variables storage service via the trusted world"
depends on OPTEE
@@ -108,6 +119,20 @@ config EFI_VARIABLE_NO_STORE
endchoice
+config EFI_VARIABLE_SF_OFFSET
+ hex "EFI variables in SPI flash offset"
+ depends on EFI_VARIABLE_SF_STORE
+ help
+ Offset from the start of the SPI Flash where EFI variables will be
stored.
+ This should be aligned to the sector size of SPI Flash.
+
+config EFI_VARIABLE_SF_DEVICE_INDEX
+ int "Device Index for target SPI Flash"
+ default 0
+ help
+ The index of SPI Flash device used for storing EFI variables. This
would be
+ needed if there are more than 1 SPI Flash devices available to use.
+
config EFI_VARIABLES_PRESEED
bool "Initial values for UEFI variables"
depends on !EFI_MM_COMM_TEE
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 33b1910249..823e0a759f 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) +=
efi_unicode_collation.o
obj-y += efi_var_common.o
obj-y += efi_var_mem.o
obj-$(CONFIG_EFI_VARIABLE_FILE_STORE) += efi_var_file.o
+obj-$(CONFIG_EFI_VARIABLE_SF_STORE) += efi_var_sf.o
ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
obj-y += efi_variable_tee.o
else
diff --git a/lib/efi_loader/efi_var_sf.c b/lib/efi_loader/efi_var_sf.c
new file mode 100644
index 00..ecf25929f5
--- /dev/null
+++ b/lib/efi_loader/efi_var_sf.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * SPI Flash interface for UEFI variables
+ *
+ * Copyright (c) 2023, Shantur Rathore
+ */
+
+#define LOG_CATEGORY LOGC_EFI
+
+#include
+#include
+#include
+#include
+
+efi_status_t efi_var_to_sf(void)
+{
+ efi_status_t ret;
+ struct efi_var_file *buf;
+ loff_t len;
+ struct udevice *sfdev;
+
+ ret = efi_var_collect(, , EFI_VARIABLE_NON_VOLATILE);
+ if (len > EFI_VAR_BUF_SIZE) {
+ log_err("EFI var buffer length more than target SF size");
+ ret = EFI_OUT_OF_RESOURCES;
+ goto error;
+ }
+
+ log_debug("%s - Got buffer to write buf->len : %d\n", __func__,
buf->length);
+
+ if (ret != EFI_SUCCESS)
+ goto error;
+
+ ret = uclass_get_device(UCLASS_SPI_FLASH,
CONFIG_EFI_VARIABLE_SF_DEVICE_INDEX, );
+ if (ret)
+ goto error;
+
+ ret = spi_flash_erase_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET,
EFI_VAR_BUF_SIZE);
+ log_debug("%s - Erased SPI Flash offset %lx\n", __func__,
CONFIG_EFI_VARIABLE_SF_OFFSE
[PATCH v2 1/3] efi_var_file: refactor to move buffer functions
Currently efi_var_file.c has functions to store/read
EFI variables to/from memory buffer. These functions
can be used with other EFI variable stores so move
them out to efi_var_common.c
Signed-off-by: Shantur Rathore
---
(no changes since v1)
include/efi_variable.h | 5 ++
lib/efi_loader/Makefile | 2 +-
lib/efi_loader/efi_var_common.c | 109
lib/efi_loader/efi_var_file.c | 121
lib/efi_loader/efi_variable.c | 8 ++-
5 files changed, 122 insertions(+), 123 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h
index 805e6c5f1e..bd0a31fc3e 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -161,6 +161,11 @@ efi_status_t efi_var_to_file(void);
efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t
*lenp,
u32 check_attr_mask);
+/* GUID used by Shim to store the MOK database */
+#define SHIM_LOCK_GUID \
+ EFI_GUID(0x605dab50, 0xe046, 0x4300, \
+0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
+
/**
* efi_var_restore() - restore EFI variables from buffer
*
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 8d31fc61c6..33b1910249 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -66,7 +66,7 @@ obj-y += efi_string.o
obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
obj-y += efi_var_common.o
obj-y += efi_var_mem.o
-obj-y += efi_var_file.o
+obj-$(CONFIG_EFI_VARIABLE_FILE_STORE) += efi_var_file.o
ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
obj-y += efi_variable_tee.o
else
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
index ad50bffd2b..7509c30b5a 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
enum efi_secure_mode {
EFI_MODE_SETUP,
@@ -40,6 +41,7 @@ static const struct efi_auth_var_name_type name_type[] = {
static bool efi_secure_boot;
static enum efi_secure_mode efi_secure_mode;
+static const efi_guid_t shim_lock_guid = SHIM_LOCK_GUID;
/**
* efi_efi_get_variable() - retrieve value of a UEFI variable
@@ -417,3 +419,110 @@ void *efi_get_var(const u16 *name, const efi_guid_t
*vendor, efi_uintn_t *size)
return buf;
}
+
+efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t
*lenp,
+ u32 check_attr_mask)
+{
+ size_t len = EFI_VAR_BUF_SIZE;
+ struct efi_var_file *buf;
+ struct efi_var_entry *var, *old_var;
+ size_t old_var_name_length = 2;
+
+ *bufp = NULL; /* Avoid double free() */
+ buf = calloc(1, len);
+ if (!buf)
+ return EFI_OUT_OF_RESOURCES;
+ var = buf->var;
+ old_var = var;
+ for (;;) {
+ efi_uintn_t data_length, var_name_length;
+ u8 *data;
+ efi_status_t ret;
+
+ if ((uintptr_t)buf + len <=
+ (uintptr_t)var->name + old_var_name_length)
+ return EFI_BUFFER_TOO_SMALL;
+
+ var_name_length = (uintptr_t)buf + len - (uintptr_t)var->name;
+ memcpy(var->name, old_var->name, old_var_name_length);
+ guidcpy(>guid, _var->guid);
+ ret = efi_get_next_variable_name_int(
+ _name_length, var->name, >guid);
+ if (ret == EFI_NOT_FOUND)
+ break;
+ if (ret != EFI_SUCCESS) {
+ free(buf);
+ return ret;
+ }
+ old_var_name_length = var_name_length;
+ old_var = var;
+
+ data = (u8 *)var->name + old_var_name_length;
+ data_length = (uintptr_t)buf + len - (uintptr_t)data;
+ ret = efi_get_variable_int(var->name, >guid,
+ >attr, _length, data,
+ >time);
+ if (ret != EFI_SUCCESS) {
+ free(buf);
+ return ret;
+ }
+ if ((var->attr & check_attr_mask) == check_attr_mask) {
+ var->length = data_length;
+ var = (struct efi_var_entry *)ALIGN((uintptr_t)data +
data_length, 8);
+ }
+ }
+
+ buf->reserved = 0;
+ buf->magic = EFI_VAR_FILE_MAGIC;
+ len = (uintptr_t)var - (uintptr_t)buf;
+ buf->crc32 = crc32(0, (u8 *)buf->var,
+ len - sizeof(struct efi_var_file));
+ buf->length = len;
+ *bufp = buf;
+ *lenp = len;
+
+ return EFI_SUCCESS;
+}
+
+efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
+{
+ struct efi_var_entry *var, *la
[PATCH v2 0/3] efi: vars: Implement SPI Flash storage
With this patch series we implement SPI Flash storage for EFI variables. Changes in v2: - Refactored efi_var_file to move common parts out as requested - Changed ifdefs to use CONFIG_IS_DEFINED - Fixed typos Shantur Rathore (3): efi_var_file: refactor to move buffer functions efi_vars: Implement SPI Flash store defconfig: rockpro64: Enable SF EFI var store configs/rockpro64-rk3399_defconfig | 2 + include/efi_variable.h | 26 +++ lib/efi_loader/Kconfig | 25 ++ lib/efi_loader/Makefile| 3 +- lib/efi_loader/efi_var_common.c| 109 ++ lib/efi_loader/efi_var_file.c | 121 - lib/efi_loader/efi_var_sf.c| 91 ++ lib/efi_loader/efi_variable.c | 17 +++- 8 files changed, 270 insertions(+), 124 deletions(-) create mode 100644 lib/efi_loader/efi_var_sf.c -- 2.40.1
Re: [PATCH] common: usb-hub: Reset hub port before scanning
Hi Marek, On Thu, Nov 23, 2023 at 9:07 PM Marek Vasut wrote: > > On 11/23/23 12:24, Shantur Rathore wrote: > > Hi Marek, > > > > On Thu, Nov 23, 2023 at 12:06 AM Marek Vasut wrote: > >> > >> On 11/20/23 00:03, Shantur Rathore wrote: > >>> Hi Marek, > >> > >> Hi, > >> > >>> On Sun, Nov 19, 2023 at 8:53 PM Marek Vasut wrote: > >>>> > >>>> On 11/10/23 15:13, Shantur Rathore wrote: > >>>>> Currently when a hub is turned on, all the ports are powered on. > >>>>> This works well for hubs which have individual power control. > >>>>> > >>>>> For the hubs without individual power control this has no effect. > >>>> > >>>> OK > >>>> > >>>>> Mostly in these scenarios the hub port is powered before the USB > >>>>> controller is enabled, this can lead to some devices in unexpected > >>>>> state. > >>>> > >>>> This ^ part needs clarification. > >>>> > >>>> Which devices are in incorrect state, the ones connected to the hub > >>>> downstream facing ports ? > >>>> > >>> > >>> In my case RockPro64, the power to usb ports onboard is controlled by > >>> a regulator. > >>> This regulator is enabled as part of init as here > >>> https://github.com/u-boot/u-boot/blob/master/arch/arm/dts/rk3399-rockpro64.dtsi#L177 > >>> > >>> On init, the usb devices connected to the port are powered up, in my > >>> case AM8180 (a RTL9210 based ) NVMe to USB enclosure with UAS. > >>> But the usb controller is only enabled at 'usb start' and by this time > >>> the device is in some state that it doesn't get detected. > >>> > >>> One way to make sure the devices connected to the ports are in an > >>> initialising state is by issuing a port reset before scanning the > >>> port. > >> > >> Why not remove this regulator-always-on and let the PHY driver turn the > >> VBUS ON only when it is needed ? > >> > >> Wouldn't that solve the problem too AND remove unnecessarily enabled > >> regulator ? > >> > >> [...] > > > > Removing the regulator-always-on does make it work but there are few issues > > > > 1. regulator is used to control power to multiple ports ( USB3.0 and > > USB2.0 in RockPro64 ), > > so this could lead to all ports turning off if a phy resets / turns off > > power > > I vaguely recall seeing some refcounting patch for regulator subsystem, > would that help ? I don't think this would be a problem for u-boot, but linux maybe. > > > 2. Even with regulator-always-on and without this reset port patch, > > linux was always > > able to detect the drive on the port, so there is something missing in > > u-boot. > > 3. Looking at usb hub code in Linux, I found that for USB3.0 it tries > > to reset the port before > > scanning. The comment says > > > > /* Is a USB 3.0 port in the Inactive or Compliance Mode state? > > * Port warm reset is required to recover > > */ > > > > i. > > https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L5736 > > ii. > > https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2836 > > > > I believe this isn't implemented in u-boot and hence explicit reset of > > a port fixes the issue. > > I feel this is separate issue and what needs to be fixed first is the > hard-wired VBus control. I beg to differ on this, couple of reasons for this 1. The same drive works fine without being reset on the USB2.0 ports - this confirms that the issue is related to USB3.0 only. 2. Linux is able to detect the same drive on USB3.0 when u-boot fails - this confirms issue doesn't lie with regulator-always-on 3. The links I pasted earlier mentions that in USB3.0 the ports need reset and this is done before the port is scanned. Adding the similar reset in u-boot fixes all with the same regulator-always-on. AFAIK u-boot doesn't handle this special USB3.0 case and maybe this is why I was finding a few posts around the drive not being detected in the USB3.0 port in u-boot but works in a USB2.0 port. Kind regards, Shantur
Re: [PATCH v5 05/12] usb: Avoid unbinding devices in use by bootflows
Hi Simon, On Tue, Nov 21, 2023 at 2:17 AM Simon Glass wrote: > > Hi Shantur, > > On Sun, 19 Nov 2023 at 15:47, Shantur Rathore wrote: > > > > Hi Simon, > > > > On Sun, Nov 19, 2023 at 7:12 PM Simon Glass wrote: > > > > > > When a USB device is unbound, it causes any bootflows attached to it to > > > be removed, via a call to bootdev_clear_bootflows() from > > > bootdev_pre_unbind(). This obviously makes it impossible to boot the > > > bootflow. > > > > > > However, when booting a bootflow that relies on USB, usb_stop() is > > > called, which unbinds the device. At that point any information > > > attached to the bootflow is dropped. > > > > > > This is quite risky since the contents of freed memory are not > > > guaranteed to remain unchanged. Depending on what other options are > > > done before boot, a hard-to-find bug may crop up. > > > > > > There is no need to unbind all the USB devices just to quiesce them. > > > Add a new usb_pause() call which removes them but leaves them bound. > > > > > > > I am no UEFI / bootloader expert and while trying to gather more info > > on EFI ExitBootServies I came across this > > https://github.com/tianocore-docs/edk2-UefiDriverWritersGuide/blob/master/7_driver_entry_point/77_adding_the_exit_boot_services_feature.md > > > > If I understand this correctly, EFI ( U-boot in this case) should be > > halting all USB controllers like done in usb_stop() > > Is my understanding correct? > > Yes, that is correct. The dm_remove_devices_flags() call should do > that. The code in bootm_disable_interrupts() is a hangover from the > pre-driver model days, I suspect. > > Perhaps we should also remove the eth_halt() and usb_pause() from > bootm_disable_interrupts() ? > Apologies for delayed response. In this case, I think we should remove eth_halt() and usb_stop() (there is no usb_pause() ) from bootm_disable_interrupts(). No point stopping things twice. Kind regards, Shantur
Re: [PATCH] common: usb-hub: Reset hub port before scanning
Hi Marek, On Thu, Nov 23, 2023 at 12:06 AM Marek Vasut wrote: > > On 11/20/23 00:03, Shantur Rathore wrote: > > Hi Marek, > > Hi, > > > On Sun, Nov 19, 2023 at 8:53 PM Marek Vasut wrote: > >> > >> On 11/10/23 15:13, Shantur Rathore wrote: > >>> Currently when a hub is turned on, all the ports are powered on. > >>> This works well for hubs which have individual power control. > >>> > >>> For the hubs without individual power control this has no effect. > >> > >> OK > >> > >>> Mostly in these scenarios the hub port is powered before the USB > >>> controller is enabled, this can lead to some devices in unexpected > >>> state. > >> > >> This ^ part needs clarification. > >> > >> Which devices are in incorrect state, the ones connected to the hub > >> downstream facing ports ? > >> > > > > In my case RockPro64, the power to usb ports onboard is controlled by > > a regulator. > > This regulator is enabled as part of init as here > > https://github.com/u-boot/u-boot/blob/master/arch/arm/dts/rk3399-rockpro64.dtsi#L177 > > > > On init, the usb devices connected to the port are powered up, in my > > case AM8180 (a RTL9210 based ) NVMe to USB enclosure with UAS. > > But the usb controller is only enabled at 'usb start' and by this time > > the device is in some state that it doesn't get detected. > > > > One way to make sure the devices connected to the ports are in an > > initialising state is by issuing a port reset before scanning the > > port. > > Why not remove this regulator-always-on and let the PHY driver turn the > VBUS ON only when it is needed ? > > Wouldn't that solve the problem too AND remove unnecessarily enabled > regulator ? > > [...] Removing the regulator-always-on does make it work but there are few issues 1. regulator is used to control power to multiple ports ( USB3.0 and USB2.0 in RockPro64 ), so this could lead to all ports turning off if a phy resets / turns off power 2. Even with regulator-always-on and without this reset port patch, linux was always able to detect the drive on the port, so there is something missing in u-boot. 3. Looking at usb hub code in Linux, I found that for USB3.0 it tries to reset the port before scanning. The comment says /* Is a USB 3.0 port in the Inactive or Compliance Mode state? * Port warm reset is required to recover */ i. https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L5736 ii. https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2836 I believe this isn't implemented in u-boot and hence explicit reset of a port fixes the issue. Kind regards, Shantur
[PATCH v1 3/3] defconfig: rockpro64: Enable SF EFI var store
RockPro64 uses SPI Flash for storing env, also use it store EFI variables. Signed-off-by: Shantur Rathore --- configs/rockpro64-rk3399_defconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig index affb6137e0..b9dededc9a 100644 --- a/configs/rockpro64-rk3399_defconfig +++ b/configs/rockpro64-rk3399_defconfig @@ -8,6 +8,8 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x30 CONFIG_ENV_SIZE=0x8000 CONFIG_ENV_OFFSET=0x3F8000 +CONFIG_EFI_VARIABLE_SF_STORE=y +CONFIG_EFI_VARIABLE_SF_OFFSET=0x7D CONFIG_DEFAULT_DEVICE_TREE="rk3399-rockpro64" CONFIG_DM_RESET=y CONFIG_ROCKCHIP_RK3399=y -- 2.40.1
[PATCH v1 2/3] efi_vars: Implement SPI Flash store
Currently U-boot uses ESP as storage for EFI variables.
Devices with SPI Flash are used for storing environment with this
commit we allow EFI variables to be stored on SPI Flash.
Signed-off-by: Shantur Rathore
---
include/efi_variable.h| 25 ++
lib/efi_loader/Kconfig| 18 +++
lib/efi_loader/Makefile | 1 +
lib/efi_loader/efi_var_sf.c | 91 +++
lib/efi_loader/efi_variable.c | 4 ++
5 files changed, 139 insertions(+)
create mode 100644 lib/efi_loader/efi_var_sf.c
diff --git a/include/efi_variable.h b/include/efi_variable.h
index ca7e19d514..766dd109f5 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -188,6 +188,31 @@ efi_status_t efi_var_from_file(void);
#endif // CONFIG_EFI_VARIABLE_FILE_STORE
+#ifdef CONFIG_EFI_VARIABLE_SF_STORE
+
+/**
+ * efi_var_from_sf() - read variables from SPI Flash
+ *
+ * EFI variable buffer is read from SPI Flash at offset defined with
+ * CONFIG_EFI_VARIABLE_SF_OFFSET of length CONFIG_EFI_VAR_BUF_SIZE
+ *
+ *
+ * Return: status code
+ */
+efi_status_t efi_var_from_sf(void);
+
+/**
+ * efi_var_to_sf() - save non-volatile variables to SPI Flash
+ *
+ * EFI variable buffer is saved to SPI Flash at offset defined with
+ * CONFIG_EFI_VARIABLE_SF_OFFSET of length CONFIG_EFI_VAR_BUF_SIZE.
+ *
+ * Return: status code
+ */
+efi_status_t efi_var_to_sf(void);
+
+#endif // CONFIG_EFI_VARIABLE_SF_STORE
+
/**
* efi_var_mem_init() - set-up variable list
*
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 4ccd26f94a..1fcc6fabb4 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -54,6 +54,17 @@ config EFI_VARIABLE_FILE_STORE
Select this option if you want non-volatile UEFI variables to be
stored as file /ubootefi.var on the EFI system partition.
+config EFI_VARIABLE_SF_STORE
+ bool "Store non-volatile UEFI variables in SPI Flash"
+ depends on SPI_FLASH
+ help
+ Select this option if you want non-volatile UEFI variables to be
+ stored in SPI Flash.
+ Define CONFIG_EFI_VARIABLE_SF_OFFSET as offset in SPI Flash to use as
+ the storage for variables. CONFIG_EFI_VAR_BUF_SIZE defines the space
+ needed.
+
+
config EFI_MM_COMM_TEE
bool "UEFI variables storage service via the trusted world"
depends on OPTEE
@@ -108,6 +119,13 @@ config EFI_VARIABLE_NO_STORE
endchoice
+config EFI_VARIABLE_SF_OFFSET
+ hex "EFI variables in SPI flash offset"
+ depends on EFI_VARIABLE_SF_STORE
+ default 0x7D
+ help
+ Offset from the start of the SPI Flash where EFI variables will be
stored.
+
config EFI_VARIABLES_PRESEED
bool "Initial values for UEFI variables"
depends on !EFI_MM_COMM_TEE
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 8d31fc61c6..b9b715b1ff 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) +=
efi_unicode_collation.o
obj-y += efi_var_common.o
obj-y += efi_var_mem.o
obj-y += efi_var_file.o
+obj-$(CONFIG_EFI_VARIABLE_SF_STORE) += efi_var_sf.o
ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
obj-y += efi_variable_tee.o
else
diff --git a/lib/efi_loader/efi_var_sf.c b/lib/efi_loader/efi_var_sf.c
new file mode 100644
index 00..e604a2225c
--- /dev/null
+++ b/lib/efi_loader/efi_var_sf.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * File interface for UEFI variables
+ *
+ * Copyright (c) 2023, Shantur Rtahore
+ */
+
+#define LOG_CATEGORY LOGC_EFI
+
+#include
+#include
+#include
+#include
+
+efi_status_t efi_var_to_sf(void)
+{
+ efi_status_t ret;
+ struct efi_var_file *buf;
+ loff_t len;
+ struct udevice *sfdev;
+
+ ret = efi_var_collect(, , EFI_VARIABLE_NON_VOLATILE);
+ if (len > EFI_VAR_BUF_SIZE) {
+ log_err("EFI var buffer length more than target SF size");
+ ret = EFI_BAD_BUFFER_SIZE;
+ goto error;
+ }
+
+ debug("%s - Got buffer to write buf->len : %d\n", __func__,
buf->length);
+
+ if (ret != EFI_SUCCESS)
+ goto error;
+
+ ret = uclass_get_device(UCLASS_SPI_FLASH, 0, );
+ if (ret)
+ goto error;
+
+ ret = spi_flash_erase_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET,
EFI_VAR_BUF_SIZE);
+ debug("%s - Erased SPI Flash offset %lx\n", __func__,
CONFIG_EFI_VARIABLE_SF_OFFSET);
+ if (ret)
+ goto error;
+
+ ret = spi_flash_write_dm(sfdev, CONFIG_EFI_VARIABLE_SF_OFFSET, len,
buf);
+ debug("%s - Wrote buffer to SPI Flash : %ld\n", __func__, ret);
+
+ if (ret)
+ goto error;
+
+ ret = EFI_SUCCESS;
+error:
+ if (ret)
+ log_err("Failed to persist EFI variables in SF\n");
+ free(buf
[PATCH v1 1/3] efi: filestore: don't compile when config disabled
Compile out filestore functions when config isn't enabled.
Signed-off-by: Shantur Rathore
---
include/efi_variable.h| 21 -
lib/efi_loader/efi_var_file.c | 13 +++--
lib/efi_loader/efi_variable.c | 10 +-
3 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h
index 805e6c5f1e..ca7e19d514 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -136,15 +136,6 @@ struct efi_var_file {
struct efi_var_entry var[];
};
-/**
- * efi_var_to_file() - save non-volatile variables as file
- *
- * File ubootefi.var is created on the EFI system partion.
- *
- * Return: status code
- */
-efi_status_t efi_var_to_file(void);
-
/**
* efi_var_collect() - collect variables in buffer
*
@@ -172,6 +163,16 @@ efi_status_t __maybe_unused efi_var_collect(struct
efi_var_file **bufp, loff_t *
*/
efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
+#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
+/**
+ * efi_var_to_file() - save non-volatile variables as file
+ *
+ * File ubootefi.var is created on the EFI system parition.
+ *
+ * Return: status code
+ */
+efi_status_t efi_var_to_file(void);
+
/**
* efi_var_from_file() - read variables from file
*
@@ -185,6 +186,8 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, bool
safe);
*/
efi_status_t efi_var_from_file(void);
+#endif // CONFIG_EFI_VARIABLE_FILE_STORE
+
/**
* efi_var_mem_init() - set-up variable list
*
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
index 62e071bd83..7ceb7e3cf7 100644
--- a/lib/efi_loader/efi_var_file.c
+++ b/lib/efi_loader/efi_var_file.c
@@ -117,6 +117,8 @@ efi_status_t __maybe_unused efi_var_collect(struct
efi_var_file **bufp, loff_t *
return EFI_SUCCESS;
}
+#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
+
/**
* efi_var_to_file() - save non-volatile variables as file
*
@@ -126,7 +128,6 @@ efi_status_t __maybe_unused efi_var_collect(struct
efi_var_file **bufp, loff_t *
*/
efi_status_t efi_var_to_file(void)
{
-#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
efi_status_t ret;
struct efi_var_file *buf;
loff_t len;
@@ -150,11 +151,10 @@ error:
log_err("Failed to persist EFI variables\n");
free(buf);
return ret;
-#else
- return EFI_SUCCESS;
-#endif
}
+#endif // CONFIG_EFI_VARIABLE_FILE_STORE
+
efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
{
struct efi_var_entry *var, *last_var;
@@ -198,6 +198,7 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, bool
safe)
return EFI_SUCCESS;
}
+#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
/**
* efi_var_from_file() - read variables from file
*
@@ -211,7 +212,6 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, bool
safe)
*/
efi_status_t efi_var_from_file(void)
{
-#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
struct efi_var_file *buf;
loff_t len;
efi_status_t ret;
@@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void)
log_err("Invalid EFI variables file\n");
error:
free(buf);
-#endif
return EFI_SUCCESS;
}
+
+#endif // CONFIG_EFI_VARIABLE_FILE_STORE
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index be95ed44e6..7fa51d 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -357,8 +357,11 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
* Write non-volatile EFI variables to file
* TODO: check if a value change has occured to avoid superfluous writes
*/
- if (attributes & EFI_VARIABLE_NON_VOLATILE)
+ if (attributes & EFI_VARIABLE_NON_VOLATILE) {
+#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
efi_var_to_file();
+#endif
+ }
return EFI_SUCCESS;
}
@@ -466,7 +469,12 @@ efi_status_t efi_init_variables(void)
if (ret != EFI_SUCCESS)
return ret;
+#ifdef CONFIG_EFI_VARIABLE_FILE_STORE
ret = efi_var_from_file();
+#else
+ ret = EFI_SUCCESS;
+#endif
+
if (ret != EFI_SUCCESS)
return ret;
if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) {
--
2.40.1
[PATCH v1 0/3] efi_vars: SPI Flash store
With this patch series we implement SPI Flash storage for EFI variables. Shantur Rathore (3): efi: filestore: don't compile when config disabled efi_vars: Implement SPI Flash store defconfig: rockpro64: Enable SF EFI var store configs/rockpro64-rk3399_defconfig | 2 + include/efi_variable.h | 46 --- lib/efi_loader/Kconfig | 18 ++ lib/efi_loader/Makefile| 1 + lib/efi_loader/efi_var_file.c | 13 +++-- lib/efi_loader/efi_var_sf.c| 91 ++ lib/efi_loader/efi_variable.c | 14 - 7 files changed, 169 insertions(+), 16 deletions(-) create mode 100644 lib/efi_loader/efi_var_sf.c -- 2.40.1
Re: [PATCH] common: usb-hub: Reset hub port before scanning
On Sun, Nov 19, 2023 at 11:03 PM Shantur Rathore wrote: > > Hi Marek, > > On Sun, Nov 19, 2023 at 8:53 PM Marek Vasut wrote: > > > > On 11/10/23 15:13, Shantur Rathore wrote: > > > Currently when a hub is turned on, all the ports are powered on. > > > This works well for hubs which have individual power control. > > > > > > For the hubs without individual power control this has no effect. > > > > OK > > > > > Mostly in these scenarios the hub port is powered before the USB > > > controller is enabled, this can lead to some devices in unexpected > > > state. > > > > This ^ part needs clarification. > > > > Which devices are in incorrect state, the ones connected to the hub > > downstream facing ports ? > > > > In my case RockPro64, the power to usb ports onboard is controlled by > a regulator. > This regulator is enabled as part of init as here > https://github.com/u-boot/u-boot/blob/master/arch/arm/dts/rk3399-rockpro64.dtsi#L177 > > On init, the usb devices connected to the port are powered up, in my > case AM8180 (a RTL9210 based ) NVMe to USB enclosure with UAS. > But the usb controller is only enabled at 'usb start' and by this time > the device is in some state that it doesn't get detected. > > One way to make sure the devices connected to the ports are in an > initialising state is by issuing a port reset before scanning the > port. > > > > With this patch, we explicitly reset the port while powering up hub > > > This resets the port for hubs without port power control and has > > > no effect on hubs with port power control as the port is still off. > > > > Should common/usb_hub.c usb_hub_port_connect_change() trigger the reset? > > > > Could it be you do not get a connect change event ? > > Yes, that's correct there is no connect change event while scanning the port. > > > > > > Before this patch AMicro AM8180 based NVME to USB adapter won't be > > > detected as a USB3.0 Mass Storage device but with this it works as > > > expected. > > > > Do you have HUB power control ? > > > > I recall there was some companion hub discussion recently. > > The onboard ports don't have separate power control, only the > regulator as explained above. > > Kind regards, > Shantur PS: If the usb device is kept disconnected on init but connected after 'usb start' while U-boot is scanning usb buses but hasn't scanned the bus the device is connected to, the usb storage device is detected. Also, I just tried - https://patchwork.ozlabs.org/project/uboot/list/?series=379807 and it doesn't fix the issue.
Re: [PATCH 0/8] USB fixes: xHCI error handling
On Sun, Nov 19, 2023 at 8:08 PM Marek Vasut wrote: > > On 10/27/23 01:16, Hector Martin wrote: > > This series is the first of a few bundles of USB fixes we have been > > carrying downstream on the Asahi U-Boot branch for a few months. > > > > Most importantly, this related set of patches makes xHCI error/stall > > recovery more robust (or work at all in some cases). There are also a > > couple patches fixing other xHCI bugs and adding better debug logs. > > > > I believe this should fix this Fedora bug too: > > > > https://bugzilla.redhat.com/show_bug.cgi?id=2244305 > > Was there ever a V2 of these patches I might've missed ? Is it this one? https://patchwork.ozlabs.org/project/uboot/list/?series=379807
Re: [PATCH] common: usb-hub: Reset hub port before scanning
Hi Marek, On Sun, Nov 19, 2023 at 8:53 PM Marek Vasut wrote: > > On 11/10/23 15:13, Shantur Rathore wrote: > > Currently when a hub is turned on, all the ports are powered on. > > This works well for hubs which have individual power control. > > > > For the hubs without individual power control this has no effect. > > OK > > > Mostly in these scenarios the hub port is powered before the USB > > controller is enabled, this can lead to some devices in unexpected > > state. > > This ^ part needs clarification. > > Which devices are in incorrect state, the ones connected to the hub > downstream facing ports ? > In my case RockPro64, the power to usb ports onboard is controlled by a regulator. This regulator is enabled as part of init as here https://github.com/u-boot/u-boot/blob/master/arch/arm/dts/rk3399-rockpro64.dtsi#L177 On init, the usb devices connected to the port are powered up, in my case AM8180 (a RTL9210 based ) NVMe to USB enclosure with UAS. But the usb controller is only enabled at 'usb start' and by this time the device is in some state that it doesn't get detected. One way to make sure the devices connected to the ports are in an initialising state is by issuing a port reset before scanning the port. > > With this patch, we explicitly reset the port while powering up hub > > This resets the port for hubs without port power control and has > > no effect on hubs with port power control as the port is still off. > > Should common/usb_hub.c usb_hub_port_connect_change() trigger the reset? > > Could it be you do not get a connect change event ? Yes, that's correct there is no connect change event while scanning the port. > > > Before this patch AMicro AM8180 based NVME to USB adapter won't be > > detected as a USB3.0 Mass Storage device but with this it works as > > expected. > > Do you have HUB power control ? > > I recall there was some companion hub discussion recently. The onboard ports don't have separate power control, only the regulator as explained above. Kind regards, Shantur
Re: [PATCH v5 05/12] usb: Avoid unbinding devices in use by bootflows
Hi Simon, On Sun, Nov 19, 2023 at 7:12 PM Simon Glass wrote: > > When a USB device is unbound, it causes any bootflows attached to it to > be removed, via a call to bootdev_clear_bootflows() from > bootdev_pre_unbind(). This obviously makes it impossible to boot the > bootflow. > > However, when booting a bootflow that relies on USB, usb_stop() is > called, which unbinds the device. At that point any information > attached to the bootflow is dropped. > > This is quite risky since the contents of freed memory are not > guaranteed to remain unchanged. Depending on what other options are > done before boot, a hard-to-find bug may crop up. > > There is no need to unbind all the USB devices just to quiesce them. > Add a new usb_pause() call which removes them but leaves them bound. > I am no UEFI / bootloader expert and while trying to gather more info on EFI ExitBootServies I came across this https://github.com/tianocore-docs/edk2-UefiDriverWritersGuide/blob/master/7_driver_entry_point/77_adding_the_exit_boot_services_feature.md If I understand this correctly, EFI ( U-boot in this case) should be halting all USB controllers like done in usb_stop() Is my understanding correct? Kind regards Shantur
[PATCH v3] arch: arm: Kconfig: Enable BOOTSTD_FULL for Rockchip SoCs
Rockchip SoCs can support wide range of bootflows. Without full bootflow commands, it can be difficult to figure out issues if any, hence enable by default. Reviewed-by: Simon Glass Signed-off-by: Shantur Rathore --- (no changes since v1) arch/arm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index d812685c98..fca6ef6d7e 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1986,6 +1986,7 @@ config ARCH_ROCKCHIP imply CMD_DM imply DEBUG_UART_BOARD_INIT imply BOOTSTD_DEFAULTS + imply BOOTSTD_FULL if BOOTSTD_DEFAULTS imply FAT_WRITE imply SARADC_ROCKCHIP imply SPL_SYSRESET -- 2.40.1
Re: [PATCH v2 2/4] bootflow: bootmeth_efi: Don't set bootdev again
Hi Simon,
On Sun, Nov 19, 2023 at 12:44 AM Simon Glass wrote:
>
> Hi Shantur,
>
> On Sat, 18 Nov 2023 at 14:17, Shantur Rathore wrote:
> >
> > Hi Simon,
> >
> > On Sat, Nov 18, 2023 at 5:11 PM Simon Glass wrote:
> > >
> > > +Ilias too as this involves a design decision
> > >
> > > Hi Shantur,
> > >
> > > On Fri, 17 Nov 2023 at 14:22, Shantur Rathore wrote:
> > > >
> > > > efi_set_bootdev is already called as part of tftp while doing dhcp_run()
> > >
> > > Is that in tftp_complete() ?
> > >
> > > > Doing this again crashes U-boot and we don't need to call again.
> > >
> > > U-Boot
> > >
> > > The problem is that we might scan for 4 bootflows, then later decide
> > > which one to boot. So we cannot know which one it will be until that
> > > point.
> > >
> > > The hack is needed so that it actually tells EFI about the right one.
> > >
> > > The addition of EFI hacks when reading files (i.e. the code in
> > > tftp_complete()) is a real problem, unfortunately. I understand that
> > > it was a convenient hack, but with standard boot we really don't want
> > > it to do that. We end up calling it multiple times for bootflows that
> > > won't be used.
> > >
> > > So I believe the fix is to be able to disable those calls, leaving it
> > > to bootstd.
> > >
> > > I'm not sure of the best way to do that. Perhaps we should have a
> > > function like bootstd_scanning() which returns a bool indicating that
> > > bootstd is scanning? If that is true then no efi_set_bootdev() calls
> > > are made? We could have a flag in 'struct bootstd_priv' which is set
> > > before bootflow_check() is called and cleared afterwards.
> > >
> > > As to how this should be done with standard boot, we should create a
> > > function like efi_start_app() and pass it the information we need.
> > > That function should do the equivalent of the 'bootefi' command,
> > > except programmatically, with no prior state hanging around.
> > >
> > > As to when we might be able to do that, we need more refactoring of
> > > the bootm code. There is a start on this [1] but it likely needs 2-3
> > > more series before it might be possible.
> > >
> > > So for now, I think bootstd_scanning() is the best approach. If you
> > > are not sure about that, I can send a patch.
> > >
> >
> > My apologies, the fix I submitted is incorrect.
> > After reading your explanation, I started thinking on why dhcp can
> > call this multiple times but bootmethod can't.
> >
> > There it was clear that the issue bflow->fname is null when we call
> > this function.
> > The correct fix should be below
> >
> > + /* bootfile should be setup by dhcp by now*/
> > + bootfile_name = env_get("bootfile");
> > + if (!bootfile_name)
> > + return log_msg_ret("bootfile_name", ret);
> > + bflow->fname = strdup(bootfile_name);
> > +
> > /* do the hideous EFI hack */
> > efi_set_bootdev("Net", "", bflow->fname, map_sysmem(addr, 0),
> > bflow->size);
> >
> > I have a patch ready for this which I will send but I have some
> > queries regarding that.
> > As I sent this a series of patches out of which 2 were reviewed,
> > should I send a new version for the whole series even though 2 are not
> > going to be changed?
>
> Yes you normally send the whole series but include any review/tested
> tags that you received for each patch ('patman status' can help).
>
patman... that's the missing piece in the puzzle. I was trying to find
out how everyone is managing their commits.
Thanks for pointing it out to me.
Kind regards,
Shantur
Re: [PATCH v2 2/4] bootflow: bootmeth_efi: Don't set bootdev again
Hi Ilias, On Sun, Nov 19, 2023 at 7:50 AM Ilias Apalodimas wrote: > > Hi Simon, > > Thanks for looping me in > > On Sat, 18 Nov 2023 at 19:11, Simon Glass wrote: > > > > +Ilias too as this involves a design decision > > > > Hi Shantur, > > > > On Fri, 17 Nov 2023 at 14:22, Shantur Rathore wrote: > > > > > > efi_set_bootdev is already called as part of tftp while doing dhcp_run() > > > > Is that in tftp_complete() ? > > > > > Doing this again crashes U-boot and we don't need to call again. > > Yes, we should fine the root cause here instead of papering over the problem The root cause has been identified and fixed here - https://patchwork.ozlabs.org/project/uboot/patch/20231119165501.1305136-...@shantur.com/ Kind regards, Shantur
[PATCH v4 3/4] bootflow: bootmeth_efi: Handle fdt not available.
While booting with efi, if fdt isn't available externally,
just use the built-in one.
Reviewed-by: Simon Glass
Signed-off-by: Shantur Rathore
---
Changes in v4:
- Update code to use BUILT_IN_FDT flag as suggested
boot/bootmeth_efi.c | 19 +--
include/bootflow.h | 2 ++
2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
index fd224f7c91..e884dc6293 100644
--- a/boot/bootmeth_efi.c
+++ b/boot/bootmeth_efi.c
@@ -313,6 +313,7 @@ static int distro_efi_try_bootflow_files(struct udevice
*dev,
*/
} else {
log_debug("No device tree available\n");
+ bflow->flags |= BOOTFLOWF_USE_BUILTIN_FDT;
}
return 0;
@@ -391,6 +392,7 @@ static int distro_efi_read_bootflow_net(struct bootflow
*bflow)
bflow->fdt_addr = fdt_addr;
} else {
log_debug("No device tree available\n");
+ bflow->flags |= BOOTFLOWF_USE_BUILTIN_FDT;
}
bflow->state = BOOTFLOWST_READY;
@@ -429,13 +431,11 @@ static int distro_efi_boot(struct udevice *dev, struct
bootflow *bflow)
return log_msg_ret("read", ret);
/*
-* use the provided device tree if available, else fall back to
-* the control FDT
+* use the provided device tree if not using the built-in fdt
*/
- if (bflow->fdt_fname)
+ if (bflow->flags & ~BOOTFLOWF_USE_BUILTIN_FDT)
fdt = bflow->fdt_addr;
- else
- fdt = (ulong)map_to_sysmem(gd->fdt_blob);
+
} else {
/*
* This doesn't actually work for network devices:
@@ -452,7 +452,14 @@ static int distro_efi_boot(struct udevice *dev, struct
bootflow *bflow)
* At some point we can add a real interface to bootefi so we can call
* this directly. For now, go through the CLI, like distro boot.
*/
- snprintf(cmd, sizeof(cmd), "bootefi %lx %lx", kernel, fdt);
+ if (bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT) {
+ log_debug("Booting with built-in fdt\n");
+ snprintf(cmd, sizeof(cmd), "bootefi %lx", kernel);
+ } else {
+ log_debug("Booting with external fdt\n");
+ snprintf(cmd, sizeof(cmd), "bootefi %lx %lx", kernel, fdt);
+ }
+
if (run_command(cmd, 0))
return log_msg_ret("run", -EINVAL);
diff --git a/include/bootflow.h b/include/bootflow.h
index fede8f22a2..42112874f6 100644
--- a/include/bootflow.h
+++ b/include/bootflow.h
@@ -45,10 +45,12 @@ enum bootflow_state_t {
* CONFIG_OF_HAS_PRIOR_STAGE is enabled
* @BOOTFLOWF_STATIC_BUF: Indicates that @bflow->buf is statically set, rather
* than being allocated by malloc().
+ * @BOOTFLOWF_USE_BUILTIN_FDT : Indicates that current bootflow uses built-in
FDT
*/
enum bootflow_flags_t {
BOOTFLOWF_USE_PRIOR_FDT = 1 << 0,
BOOTFLOWF_STATIC_BUF= 1 << 1,
+ BOOTFLOWF_USE_BUILTIN_FDT = 1 << 2,
};
/**
--
2.40.1
[PATCH v4 4/4] bootflow: bootmeth_efi: don't free buffer
bootmeth_efi doesn't allocate any buffer to load efi in any case.
enable static buffer flag for all cases.
Reviewed-by: Simon Glass
Signed-off-by: Shantur Rathore
---
(no changes since v1)
boot/bootmeth_efi.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
index e884dc6293..446cb73140 100644
--- a/boot/bootmeth_efi.c
+++ b/boot/bootmeth_efi.c
@@ -160,7 +160,6 @@ static int efiload_read_file(struct bootflow *bflow, ulong
addr)
if (ret)
return log_msg_ret("read", ret);
bflow->buf = map_sysmem(addr, bflow->size);
- bflow->flags |= BOOTFLOWF_STATIC_BUF;
set_efi_bootdev(desc, bflow);
@@ -404,6 +403,12 @@ static int distro_efi_read_bootflow(struct udevice *dev,
struct bootflow *bflow)
{
int ret;
+ /*
+* bootmeth_efi doesn't allocate any buffer neither for blk nor net
device
+* set flag to avoid freeing static buffer.
+*/
+ bflow->flags |= BOOTFLOWF_STATIC_BUF;
+
if (bootmeth_uses_network(bflow)) {
/* we only support reading from one device, so ignore 'dev' */
ret = distro_efi_read_bootflow_net(bflow);
--
2.40.1
[PATCH v4 2/4] bootflow: bootmeth_efi: set bflow->fname from bootfile name
We need to set boot->fname before calling efi_set_bootdev
otherwise this crashes as bflow->fname is null.
Reviewed-by: Simon Glass
Signed-off-by: Shantur Rathore
---
(no changes since v1)
boot/bootmeth_efi.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
index 682cf5b23b..fd224f7c91 100644
--- a/boot/bootmeth_efi.c
+++ b/boot/bootmeth_efi.c
@@ -323,7 +323,7 @@ static int distro_efi_read_bootflow_net(struct bootflow
*bflow)
char file_addr[17], fname[256];
char *tftp_argv[] = {"tftp", file_addr, fname, NULL};
struct cmd_tbl cmdtp = {}; /* dummy */
- const char *addr_str, *fdt_addr_str;
+ const char *addr_str, *fdt_addr_str, *bootfile_name;
int ret, arch, size;
ulong addr, fdt_addr;
char str[36];
@@ -360,6 +360,12 @@ static int distro_efi_read_bootflow_net(struct bootflow
*bflow)
return log_msg_ret("sz", -EINVAL);
bflow->size = size;
+/* bootfile should be setup by dhcp*/
+ bootfile_name = env_get("bootfile");
+ if (!bootfile_name)
+ return log_msg_ret("bootfile_name", ret);
+ bflow->fname = strdup(bootfile_name);
+
/* do the hideous EFI hack */
efi_set_bootdev("Net", "", bflow->fname, map_sysmem(addr, 0),
bflow->size);
--
2.40.1
[PATCH v4 0/4] bootflow: bootmeth_efi: Fix network efi boot.
Currently bootmeth_efi crashes while doing a network (dhcp) boot. This patch series fixes issues and both network and disk boot works. Shantur Rathore (4): bootflow: bootmeth_efi: Set bootp_arch as hex bootflow: bootmeth_efi: set bflow->fname from bootfile name bootflow: bootmeth_efi: Handle fdt not available. bootflow: bootmeth_efi: don't free buffer boot/bootmeth_efi.c | 36 +++- include/bootflow.h | 2 ++ 2 files changed, 29 insertions(+), 9 deletions(-) -- 2.40.1
