Re: [PATCH v3] tpm: display warning if using gpio reset with TPM
> > > Signed-off-by: Jorge Ramirez-Ortiz > > > > You cannot send your SoB like this. SoB means you are carrying some > > code which complies with the license, etc. > > > > Either you were part of the original writing and want to be credited > > for that (you can be the author and first SoB, or suggest Tim to use > > Co-developed-by). Or you reviewed the change (Reviewed-by), you tested > > the change (Tested-by), or you are maintainer/responsible for some part > > that is touched and you agree with the change (Acked-by). > > right, however there is some lenience in the process: some projects accept > the signed off, some others add the acked or reviewed and so on... > > but I agree with you and I certainly do not need/want/expect to be > credited for this work. it is all good on my end :) > > > > > Thanks, > > Miquèl > > btw you could have simply ignored my note btw : notice the "if needed" > part in my response. for reference, I noticed my name in CC, checked the > change, thought it was a good idea and tried to move it a long - just > chose the wrong tag for sure... And this is likely a good habit :) But now you need to send your Reviewed-by because these tags are usually collected by the tools. If you don't write it properly, there are very little chances it will ever be picked-up. Thanks, Miquèl
Re: [PATCH v3] tpm: display warning if using gpio reset with TPM
Hi Jorge, ... > > > - board with no reset gpio > > > u-boot=> tpm init && tpm info > > > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open] > > > - board with a reset gpio > > > u-boot=> tpm init && tpm info > > > tpm@1: TPM gpio reset should not be used on secure production devices > > > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open] > > > > > > [1] > > > https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf > > > > > > Signed-off-by: Tim Harvey > > > > Looks way cleaner, thanks. > > > > Reviewed-by: Miquel Raynal > > > > Miquèl > > nice. if needed > > Signed-off-by: Jorge Ramirez-Ortiz You cannot send your SoB like this. SoB means you are carrying some code which complies with the license, etc. Either you were part of the original writing and want to be credited for that (you can be the author and first SoB, or suggest Tim to use Co-developed-by). Or you reviewed the change (Reviewed-by), you tested the change (Tested-by), or you are maintainer/responsible for some part that is touched and you agree with the change (Acked-by). Thanks, Miquèl
Re: [PATCH v3] tpm: display warning if using gpio reset with TPM
Hi Tim, thar...@gateworks.com wrote on Wed, 15 May 2024 16:21:38 -0700: > Instead of displaying what looks like an error message if a > gpio-reset dt prop is missing for a TPM display a warning that > having a gpio reset on a TPM should not be used for a secure production > device. > > TCG TIS spec [1] says: > "The TPM_Init (LRESET#/SPI_RST#) signal MUST be connected to the > platform CPU Reset signal such that it complies with the requirements > specified in section 1.2.7 HOST Platform Reset in the PC Client > Implementation Specification for Conventional BIOS." > > The reasoning is that you should not be able to toggle a GPIO and reset > the TPM without resetting the CPU as well because if an attacker can > break into your OS via an OS level security flaw they can then reset the > TPM via GPIO and replay the measurements required to unseal keys > that you have otherwise protected. > > Additionally restructure the code for improved readability allowing for > removal of the init label. > > Before: > - board with no reset gpio > u-boot=> tpm init && tpm info > tpm_tis_spi_probe: missing reset GPIO > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open] > - board with a reset gpio > u-boot=> tpm init && tpm info > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open] > > After: > - board with no reset gpio > u-boot=> tpm init && tpm info > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open] > - board with a reset gpio > u-boot=> tpm init && tpm info > tpm@1: TPM gpio reset should not be used on secure production devices > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open] > > [1] > https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf > > Signed-off-by: Tim Harvey Looks way cleaner, thanks. Reviewed-by: Miquel Raynal Miquèl
Re: [PATCH v2] tpm: display warning if using gpio reset with TPM
Hi Ilias,
ilias.apalodi...@linaro.org wrote on Wed, 17 Apr 2024 08:40:14 +0300:
> Hi Miquel
>
> On Mon, 8 Apr 2024 at 10:23, Miquel Raynal wrote:
> >
> > Hello,
> >
> > ilias.apalodi...@linaro.org wrote on Thu, 28 Mar 2024 09:08:37 +0200:
> >
> > > Thanks Tim
> > >
> > > On Thu, 28 Mar 2024 at 00:12, Tim Harvey wrote:
> > > >
> > > > Instead of displaying what looks like an error message if a
> > > > gpio-reset dt prop is missing for a TPM display a warning that
> > > > having a gpio reset on a TPM should not be used for a secure production
> > > > device.
> > > >
> > > > TCG TIS spec [1] says:
> > > > "The TPM_Init (LRESET#/SPI_RST#) signal MUST be connected to the
> > > > platform CPU Reset signal such that it complies with the requirements
> > > > specified in section 1.2.7 HOST Platform Reset in the PC Client
> > > > Implementation Specification for Conventional BIOS."
> > > >
> > > > The reasoning is that you should not be able to toggle a GPIO and reset
> > > > the TPM without resetting the CPU as well because if an attacker can
> > > > break into your OS via an OS level security flaw they can then reset the
> > > > TPM via GPIO and replay the measurements required to unseal keys
> > > > that you have otherwise protected.
> > > >
> > > > [1]
> > > > https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf
> > > >
> > > > Signed-off-by: Tim Harvey
> > > > ---
> > > > v2: change the message to a warning and update commit desc/log
> > > > ---
> > > > drivers/tpm/tpm2_tis_spi.c | 8
> > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> > > > index de9cf8f21e07..c9c83f6f0fc8 100644
> > > > --- a/drivers/tpm/tpm2_tis_spi.c
> > > > +++ b/drivers/tpm/tpm2_tis_spi.c
> > > > @@ -237,14 +237,14 @@ static int tpm_tis_spi_probe(struct udevice *dev)
> > > > /* legacy reset */
> > > > ret = gpio_request_by_name(dev, "gpio-reset", 0,
> > > >_gpio,
> > > > GPIOD_IS_OUT);
> > > > - if (ret) {
> > > > - log(LOGC_NONE, LOGL_NOTICE,
> > > > - "%s: missing reset GPIO\n",
> > > > __func__);
> > > > + if (ret)
> > > > goto init;
> > > > - }
> > > > log(LOGC_NONE, LOGL_NOTICE,
> > > > "%s: gpio-reset is deprecated\n", __func__);
> > > > }
> > > > + log(LOGC_NONE, LOGL_WARNING,
> > > > + "%s: TPM gpio reset should not be used on secure
> > > > production devices\n",
> > > > + dev->name);
> > > > dm_gpio_set_value(_gpio, 1);
> > > > mdelay(1);
> > > > dm_gpio_set_value(_gpio, 0);
> >
> > The current logic expects a reset gpio and bails out if it cannot get
> > it with a (questionable) goto statement.
> >
> > You want to invert that logic, and expect no gpio, but only if there is
> > one you want to warn.
> >
> > This is perfectly fine but the logic here must be clarified. I'd go for:
> >
> > ret = gpio_request()
> > if (ret) {
> > ret = gpio_request()
> > if (!ret)
> > warn(deprecated)
> > }
> >
> > if (!ret) {
> > warn(dangerous)
> > toggle_value()
> > }
> >
> > I would ideally replace the 'if (ret)' clauses with 'if (!reset_gpio)'
> > which would make the checks even clearer.
>
> Fair enough. But the code with the proposed change has no functional
> problems right?
No, this is functionally right, but the code is not clear like that.
> If so I'll send a PR to Tom as is and rework it as suggested later
Well, that's not how contribution work usually. Is there an emergency
in getting this merged?
Thanks,
Miquèl
Re: [PATCH v11 1/2] dt-bindings: mtd: fixed-partitions: Add alignment properties
On Fri, 2024-04-12 at 15:32:48 UTC, Simon Glass wrote: > Add three properties for controlling alignment of partitions, aka > 'entries' in fixed-partition. > > For now there is no explicit mention of hierarchy, so a 'section' is > just the 'fixed-partitions' node. > > These new properties are inputs to the Binman packaging process, but are > also needed if the firmware is repacked, to ensure that alignment > constraints are not violated. Therefore they are provided as part of > the schema. > > Signed-off-by: Simon Glass > Reviewed-by: Rob Herring Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH v11 2/2] dt-bindings: mtd: fixed-partition: Add binman compatibles
On Fri, 2024-04-12 at 15:32:49 UTC, Simon Glass wrote: > Add two compatibles for binman entries, as a starting point for the > schema. > > Note that, after discussion on v2, we decided to keep the existing > meaning of label so as not to require changes to existing userspace > software when moving to use binman nodes to specify the firmware > layout. > > Note also that, after discussion on v6, we decided to use the same > 'fixed-partition' schema for the binman features, so this version > adds a new 'binman.yaml' file providing the new compatibles to the > existing partition.yaml binding. > > Signed-off-by: Simon Glass > Reviewed-by: Rob Herring Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH v10 1/2] dt-bindings: mtd: fixed-partitions: Add alignment properties
Hi Simon, s...@chromium.org wrote on Tue, 26 Mar 2024 14:06:44 -0600: > Add three properties for controlling alignment of partitions, aka > 'entries' in fixed-partition. > > For now there is no explicit mention of hierarchy, so a 'section' is > just the 'fixed-partitions' node. > > These new properties are inputs to the Binman packaging process, but are > also needed if the firmware is repacked, to ensure that alignment > constraints are not violated. Therefore they are provided as part of > the schema. > > Signed-off-by: Simon Glass > Reviewed-by: Rob Herring > --- > > Changes in v10: > - Update the minimum to 2 > > Changes in v9: > - Move binding example to next batch to avoid build error > > Changes in v7: > - Drop patch 'Add binman compatible' > - Put the alignment properties into the fixed-partition binding > > Changes in v6: > - Correct schema-validation errors missed due to older dt-schema > (enum fix and reg addition) > > Changes in v5: > - Add value ranges > - Consistently mention alignment must be power-of-2 > - Mention that alignment refers to bytes > > Changes in v2: > - Fix 'a' typo in commit message > > .../bindings/mtd/partitions/partition.yaml| 51 +++ > 1 file changed, 51 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mtd/partitions/partition.yaml > b/Documentation/devicetree/bindings/mtd/partitions/partition.yaml > index 1ebe9e2347ea..656ca3db1762 100644 > --- a/Documentation/devicetree/bindings/mtd/partitions/partition.yaml > +++ b/Documentation/devicetree/bindings/mtd/partitions/partition.yaml > @@ -57,6 +57,57 @@ properties: >user space from > type: boolean > > + align: > +$ref: /schemas/types.yaml#/definitions/uint32 > +minimum: 2 > +maximum: 0x8000 > +multipleOf: 2 > +description: > + This sets the alignment of the entry in bytes. > + > + The entry offset is adjusted so that the entry starts on an aligned > + boundary within the containing section or image. For example ‘align = > + <16>’ means that the entry will start on a 16-byte boundary. This may > + mean that padding is added before the entry. The padding is part of > + the containing section but is not included in the entry, meaning that > + an empty space may be created before the entry starts. Alignment > + must be a power of 2. If ‘align’ is not provided, no alignment is > + performed. > + > + align-size: > +$ref: /schemas/types.yaml#/definitions/uint32 > +minimum: 2 > +maximum: 0x8000 > +multipleOf: 2 > +description: > + This sets the alignment of the entry size in bytes. It must be a power > + of 2. > + > + For example, to ensure that the size of an entry is a multiple of 64 > + bytes, set this to 64. While this does not affect the contents of the > + entry within binman itself (the padding is performed only when its > + parent section is assembled), the end result is that the entry ends > + with the padding bytes, so may grow. If ‘align-size’ is not provided, > + no alignment is performed. I don't think we should mention binman here. Can we have a software agnostic description? This should be understandable from anyone playing with mtd partitions I guess. > + > + align-end: > +$ref: /schemas/types.yaml#/definitions/uint32 > +minimum: 2 > +maximum: 0x8000 > +multipleOf: 2 seems not to perfectly match the constraint, but I don't know if there is a powerOf keyword? (same above) > +description: > + This sets the alignment (in bytes) of the end of an entry with respect > + to the containing section. It must be a power of 2. > + > + Some entries require that they end on an alignment boundary, > + regardless of where they start. This does not move the start of the > + entry, so the contents of the entry will still start at the beginning. > + But there may be padding at the end. While this does not affect the > + contents of the entry within binman itself (the padding is performed content?same comment about binman? > + only when its parent section is assembled), the end result is that the > + entry ends with the padding bytes, so may grow. If ‘align-end’ is not > + provided, no alignment is performed. > + > if: >not: > required: [ reg ] Thanks, Miquèl
Re: [PATCH v2] tpm: display warning if using gpio reset with TPM
Hello,
ilias.apalodi...@linaro.org wrote on Thu, 28 Mar 2024 09:08:37 +0200:
> Thanks Tim
>
> On Thu, 28 Mar 2024 at 00:12, Tim Harvey wrote:
> >
> > Instead of displaying what looks like an error message if a
> > gpio-reset dt prop is missing for a TPM display a warning that
> > having a gpio reset on a TPM should not be used for a secure production
> > device.
> >
> > TCG TIS spec [1] says:
> > "The TPM_Init (LRESET#/SPI_RST#) signal MUST be connected to the
> > platform CPU Reset signal such that it complies with the requirements
> > specified in section 1.2.7 HOST Platform Reset in the PC Client
> > Implementation Specification for Conventional BIOS."
> >
> > The reasoning is that you should not be able to toggle a GPIO and reset
> > the TPM without resetting the CPU as well because if an attacker can
> > break into your OS via an OS level security flaw they can then reset the
> > TPM via GPIO and replay the measurements required to unseal keys
> > that you have otherwise protected.
> >
> > [1]
> > https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf
> >
> > Signed-off-by: Tim Harvey
> > ---
> > v2: change the message to a warning and update commit desc/log
> > ---
> > drivers/tpm/tpm2_tis_spi.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> > index de9cf8f21e07..c9c83f6f0fc8 100644
> > --- a/drivers/tpm/tpm2_tis_spi.c
> > +++ b/drivers/tpm/tpm2_tis_spi.c
> > @@ -237,14 +237,14 @@ static int tpm_tis_spi_probe(struct udevice *dev)
> > /* legacy reset */
> > ret = gpio_request_by_name(dev, "gpio-reset", 0,
> >_gpio,
> > GPIOD_IS_OUT);
> > - if (ret) {
> > - log(LOGC_NONE, LOGL_NOTICE,
> > - "%s: missing reset GPIO\n", __func__);
> > + if (ret)
> > goto init;
> > - }
> > log(LOGC_NONE, LOGL_NOTICE,
> > "%s: gpio-reset is deprecated\n", __func__);
> > }
> > + log(LOGC_NONE, LOGL_WARNING,
> > + "%s: TPM gpio reset should not be used on secure
> > production devices\n",
> > + dev->name);
> > dm_gpio_set_value(_gpio, 1);
> > mdelay(1);
> > dm_gpio_set_value(_gpio, 0);
The current logic expects a reset gpio and bails out if it cannot get
it with a (questionable) goto statement.
You want to invert that logic, and expect no gpio, but only if there is
one you want to warn.
This is perfectly fine but the logic here must be clarified. I'd go for:
ret = gpio_request()
if (ret) {
ret = gpio_request()
if (!ret)
warn(deprecated)
}
if (!ret) {
warn(dangerous)
toggle_value()
}
I would ideally replace the 'if (ret)' clauses with 'if (!reset_gpio)'
which would make the checks even clearer.
Thanks,
Miquèl
Re: [PATCH v6 1/3] dt-bindings: mtd: partitions: Add binman compatible
Hi Simon, s...@chromium.org wrote on Wed, 13 Mar 2024 11:25:42 +1300: > Hi Miquel, > > On Fri, 8 Mar 2024 at 20:42, Miquel Raynal wrote: > > > > Hi Simon, > > > > s...@chromium.org wrote on Fri, 8 Mar 2024 15:44:25 +1300: > > > > > Hi Miquel, > > > > > > On Tue, 6 Feb 2024 at 01:17, Miquel Raynal > > > wrote: > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > > > > > +description: | > > > > > > > > > > > > > > > > + The binman node provides a layout for > > > > > > > > > > > > > > > > firmware, used when packaging firmware > > > > > > > > > > > > > > > > + from multiple projects. It is based on > > > > > > > > > > > > > > > > fixed-partitions, with some > > > > > > > > > > > > > > > > + extensions, but uses 'compatible' to > > > > > > > > > > > > > > > > indicate the contents of the node, to > > > > > > > > > > > > > > > > + avoid perturbing or confusing existing > > > > > > > > > > > > > > > > installations which use 'label' for a > > > > > > > > > > > > > > > > + particular purpose. > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > + Binman supports properties used as inputs to > > > > > > > > > > > > > > > > the firmware-packaging process, > > > > > > > > > > > > > > > > + such as those which control alignment of > > > > > > > > > > > > > > > > partitions. This binding addresses > > > > > > > > > > > > > > > > + these 'input' properties. For example, it is > > > > > > > > > > > > > > > > common for the 'reg' property > > > > > > > > > > > > > > > > + (an 'output' property) to be set by Binman, > > > > > > > > > > > > > > > > based on the alignment requested > > > > > > > > > > > > > > > > + in the input. > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > + Once processing is complete, input > > > > > > > > > > > > > > > > properties have mostly served their > > > > > > > > > > > > > > > > + purpose, at least until the firmware is > > > > > > > > > > > > > > > > repacked later, e.g. due to a > > > > > > > > > > > > > > > > + firmware update. The 'fixed-partitions' > > > > > > > > > > > > > > > > binding should provide enough > > > > > > > > > > > > > > > > + information to read the firmware at runtime, > > > > > > > > > > > > > > > > including decompression if > > > > > > > > > > > > > > > > + needed. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > How is this going to work exactly? binman reads > > > > > > > > > > > > > > > these nodes and then > > > > > > > > > > > > > > > writes out 'fixed-partitions' nodes. But then > > > > > > > > > > > > > > > you've lost the binman > > > > > > > > > > > > > > > specifc parts needed for repacking. > > > > > > > > > > > > > > > > > > > > > > > > > > > > No, they are the same node. I do want the extra > > > > > > > > > > > > > > information to stick > > > > > > > > > > > > > > around. So long as it is compatible with > > > > > > > > > > > > > > fixed-partition as well, th
Re: [PATCH v6 1/3] dt-bindings: mtd: partitions: Add binman compatible
Hi Simon, s...@chromium.org wrote on Fri, 8 Mar 2024 15:44:25 +1300: > Hi Miquel, > > On Tue, 6 Feb 2024 at 01:17, Miquel Raynal wrote: > > > > Hi Simon, > > > > > > > > > > > > > > > > +description: | > > > > > > > > > > > > > > + The binman node provides a layout for firmware, > > > > > > > > > > > > > > used when packaging firmware > > > > > > > > > > > > > > + from multiple projects. It is based on > > > > > > > > > > > > > > fixed-partitions, with some > > > > > > > > > > > > > > + extensions, but uses 'compatible' to indicate > > > > > > > > > > > > > > the contents of the node, to > > > > > > > > > > > > > > + avoid perturbing or confusing existing > > > > > > > > > > > > > > installations which use 'label' for a > > > > > > > > > > > > > > + particular purpose. > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + Binman supports properties used as inputs to the > > > > > > > > > > > > > > firmware-packaging process, > > > > > > > > > > > > > > + such as those which control alignment of > > > > > > > > > > > > > > partitions. This binding addresses > > > > > > > > > > > > > > + these 'input' properties. For example, it is > > > > > > > > > > > > > > common for the 'reg' property > > > > > > > > > > > > > > + (an 'output' property) to be set by Binman, > > > > > > > > > > > > > > based on the alignment requested > > > > > > > > > > > > > > + in the input. > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + Once processing is complete, input properties > > > > > > > > > > > > > > have mostly served their > > > > > > > > > > > > > > + purpose, at least until the firmware is repacked > > > > > > > > > > > > > > later, e.g. due to a > > > > > > > > > > > > > > + firmware update. The 'fixed-partitions' binding > > > > > > > > > > > > > > should provide enough > > > > > > > > > > > > > > + information to read the firmware at runtime, > > > > > > > > > > > > > > including decompression if > > > > > > > > > > > > > > + needed. > > > > > > > > > > > > > > > > > > > > > > > > > > How is this going to work exactly? binman reads these > > > > > > > > > > > > > nodes and then > > > > > > > > > > > > > writes out 'fixed-partitions' nodes. But then you've > > > > > > > > > > > > > lost the binman > > > > > > > > > > > > > specifc parts needed for repacking. > > > > > > > > > > > > > > > > > > > > > > > > No, they are the same node. I do want the extra > > > > > > > > > > > > information to stick > > > > > > > > > > > > around. So long as it is compatible with > > > > > > > > > > > > fixed-partition as well, this > > > > > > > > > > > > should work OK. > > > > > > > > > > > > > > > > > > > > > > How can it be both? The partitions node compatible can be > > > > > > > > > > > either > > > > > > > > > > > 'fixed-partitions' or 'binman'. > > > > > > > > > > > > > > > > > > > > Can we not allow it to be both? I have tried to adjust > > > > > > > > > > things in > > > > > > > > > > response to feedback but per
Re: [PATCH v6 1/3] dt-bindings: mtd: partitions: Add binman compatible
Hi Simon, > > > > > > > > > > > > +description: | > > > > > > > > > > > > + The binman node provides a layout for firmware, used > > > > > > > > > > > > when packaging firmware > > > > > > > > > > > > + from multiple projects. It is based on > > > > > > > > > > > > fixed-partitions, with some > > > > > > > > > > > > + extensions, but uses 'compatible' to indicate the > > > > > > > > > > > > contents of the node, to > > > > > > > > > > > > + avoid perturbing or confusing existing installations > > > > > > > > > > > > which use 'label' for a > > > > > > > > > > > > + particular purpose. > > > > > > > > > > > > + > > > > > > > > > > > > + Binman supports properties used as inputs to the > > > > > > > > > > > > firmware-packaging process, > > > > > > > > > > > > + such as those which control alignment of partitions. > > > > > > > > > > > > This binding addresses > > > > > > > > > > > > + these 'input' properties. For example, it is common > > > > > > > > > > > > for the 'reg' property > > > > > > > > > > > > + (an 'output' property) to be set by Binman, based on > > > > > > > > > > > > the alignment requested > > > > > > > > > > > > + in the input. > > > > > > > > > > > > + > > > > > > > > > > > > + Once processing is complete, input properties have > > > > > > > > > > > > mostly served their > > > > > > > > > > > > + purpose, at least until the firmware is repacked > > > > > > > > > > > > later, e.g. due to a > > > > > > > > > > > > + firmware update. The 'fixed-partitions' binding > > > > > > > > > > > > should provide enough > > > > > > > > > > > > + information to read the firmware at runtime, > > > > > > > > > > > > including decompression if > > > > > > > > > > > > + needed. > > > > > > > > > > > > > > > > > > > > > > How is this going to work exactly? binman reads these > > > > > > > > > > > nodes and then > > > > > > > > > > > writes out 'fixed-partitions' nodes. But then you've lost > > > > > > > > > > > the binman > > > > > > > > > > > specifc parts needed for repacking. > > > > > > > > > > > > > > > > > > > > No, they are the same node. I do want the extra information > > > > > > > > > > to stick > > > > > > > > > > around. So long as it is compatible with fixed-partition as > > > > > > > > > > well, this > > > > > > > > > > should work OK. > > > > > > > > > > > > > > > > > > How can it be both? The partitions node compatible can be > > > > > > > > > either > > > > > > > > > 'fixed-partitions' or 'binman'. > > > > > > > > > > > > > > > > Can we not allow it to be both? I have tried to adjust things in > > > > > > > > response to feedback but perhaps the feedback was leading me > > > > > > > > down the > > > > > > > > wrong path? > > > > > > > > > > > > > > Sure, but then the schema has to and that means extending > > > > > > > fixed-partitions. > > > > > > > > > > > > Can we cross that bridge later? There might be resistance to it. I'm > > > > > > not sure. For now, perhaps just a binman compatible works well > > > > > > enough > > > > > > to make progress. > > > > > > > > > > Is there any way to make progress on this? I would like to have > > > > > software which doesn't understand the binman compatible to at least be > > > > > able to understand the fixed-partition compatible. Is that > > > > > acceptable? > > > > > > > > There's only 2 ways that it can work. Either binman writes out > > > > fixed-partition nodes dropping/replacing anything only defined for > > > > binman or fixed-partition is extended to include what binman needs. > > > > > > OK, then I suppose the best way is to add a new binman compatible, as > > > is done with this v6 series. People then need to choose it instead of > > > fixed-partition. > > > > I'm sorry this is not at all what Rob suggested, or did I totally > > misunderstand his answer? > > > > In both cases the solution is to generate a "fixed-partition" node. Now > > up to you to decide whether binman should adapt the output to the > > current schema, or if the current schema should be extended to > > understand all binman's output. > > > > At least that is my understanding and also what I kind of agree with. > > I do want to binman schema to include all the features of Binman. > > So are you saying that there should not be a 'binman' schema, but I > should just add all the binman properties to the fixed-partition > schema? This is my current understanding, yes. But acknowledgment from Rob is also welcome. Thanks, Miquèl
Re: [PATCH v6 1/3] dt-bindings: mtd: partitions: Add binman compatible
Hi Simon, s...@chromium.org wrote on Sun, 4 Feb 2024 05:07:38 -0700: > Hi Rob, > > On Wed, 17 Jan 2024 at 08:56, Rob Herring wrote: > > > > On Thu, Jan 4, 2024 at 3:54 PM Simon Glass wrote: > > > > > > Hi Rob, > > > > > > On Thu, Dec 14, 2023 at 2:09 PM Simon Glass wrote: > > > > > > > > Hi Rob, > > > > > > > > On Thu, 14 Dec 2023 at 10:27, Rob Herring wrote: > > > > > > > > > > On Fri, Dec 08, 2023 at 03:58:10PM -0700, Simon Glass wrote: > > > > > > Hi Rob, > > > > > > > > > > > > On Fri, 8 Dec 2023 at 14:56, Rob Herring wrote: > > > > > > > > > > > > > > On Fri, Dec 8, 2023 at 11:47 AM Simon Glass > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Rob, > > > > > > > > > > > > > > > > On Fri, 8 Dec 2023 at 08:00, Rob Herring > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Thu, Nov 16, 2023 at 10:28:50AM -0700, Simon Glass wrote: > > > > > > > > > > Add a compatible string for binman, so we can extend > > > > > > > > > > fixed-partitions > > > > > > > > > > in various ways. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Simon Glass > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > (no changes since v5) > > > > > > > > > > > > > > > > > > > > Changes in v5: > > > > > > > > > > - Add #address/size-cells and parternProperties > > > > > > > > > > - Drop $ref to fixed-partitions.yaml > > > > > > > > > > - Drop 'select: false' > > > > > > > > > > > > > > > > > > > > Changes in v4: > > > > > > > > > > - Change subject line > > > > > > > > > > > > > > > > > > > > Changes in v3: > > > > > > > > > > - Drop fixed-partition additional compatible string > > > > > > > > > > - Drop fixed-partitions from the example > > > > > > > > > > - Mention use of compatible instead of label > > > > > > > > > > > > > > > > > > > > Changes in v2: > > > > > > > > > > - Drop mention of 'enhanced features' in > > > > > > > > > > fixed-partitions.yaml > > > > > > > > > > - Mention Binman input and output properties > > > > > > > > > > - Use plain partition@xxx for the node name > > > > > > > > > > > > > > > > > > > > .../bindings/mtd/partitions/binman.yaml | 68 > > > > > > > > > > +++ > > > > > > > > > > .../bindings/mtd/partitions/partitions.yaml | 1 + > > > > > > > > > > MAINTAINERS | 5 ++ > > > > > > > > > > 3 files changed, 74 insertions(+) > > > > > > > > > > create mode 100644 > > > > > > > > > > Documentation/devicetree/bindings/mtd/partitions/binman.yaml > > > > > > > > > > > > > > > > > > > > diff --git > > > > > > > > > > a/Documentation/devicetree/bindings/mtd/partitions/binman.yaml > > > > > > > > > > > > > > > > > > > > b/Documentation/devicetree/bindings/mtd/partitions/binman.yaml > > > > > > > > > > new file mode 100644 > > > > > > > > > > index ..329217550a98 > > > > > > > > > > --- /dev/null > > > > > > > > > > +++ > > > > > > > > > > b/Documentation/devicetree/bindings/mtd/partitions/binman.yaml > > > > > > > > > > @@ -0,0 +1,68 @@ > > > > > > > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > > > > > > > > +# Copyright 2023 Google LLC > > > > > > > > > > + > > > > > > > > > > +%YAML 1.2 > > > > > > > > > > +--- > > > > > > > > > > +$id: > > > > > > > > > > http://devicetree.org/schemas/mtd/partitions/binman.yaml# > > > > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > > > > > + > > > > > > > > > > +title: Binman firmware layout > > > > > > > > > > + > > > > > > > > > > +maintainers: > > > > > > > > > > + - Simon Glass > > > > > > > > > > + > > > > > > > > > > +description: | > > > > > > > > > > + The binman node provides a layout for firmware, used > > > > > > > > > > when packaging firmware > > > > > > > > > > + from multiple projects. It is based on fixed-partitions, > > > > > > > > > > with some > > > > > > > > > > + extensions, but uses 'compatible' to indicate the > > > > > > > > > > contents of the node, to > > > > > > > > > > + avoid perturbing or confusing existing installations > > > > > > > > > > which use 'label' for a > > > > > > > > > > + particular purpose. > > > > > > > > > > + > > > > > > > > > > + Binman supports properties used as inputs to the > > > > > > > > > > firmware-packaging process, > > > > > > > > > > + such as those which control alignment of partitions. > > > > > > > > > > This binding addresses > > > > > > > > > > + these 'input' properties. For example, it is common for > > > > > > > > > > the 'reg' property > > > > > > > > > > + (an 'output' property) to be set by Binman, based on the > > > > > > > > > > alignment requested > > > > > > > > > > + in the input. > > > > > > > > > > + > > > > > > > > > > + Once processing is complete, input properties have > > > > > > > > > > mostly served their > > > > > > > > > > + purpose, at least until the firmware is repacked later, > > > > > > > > > > e.g. due to a > > > > > > > > > > + firmware update. The
Re: [PATCH V3 1/6] dt-bindings: nvmem: layouts: add U-Boot environment variables layout
Hi Rob,
r...@kernel.org wrote on Mon, 15 Jan 2024 11:09:03 -0600:
> On Thu, Jan 04, 2024 at 10:10:13AM +0100, Rafał Miłecki wrote:
> > On 4.01.2024 08:58, Miquel Raynal wrote:
> > > r...@kernel.org wrote on Wed, 3 Jan 2024 17:11:29 -0700:
> > > > On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote:
> > > > > From: Rafał Miłecki
> > > > >
> > > > > U-Boot env data is a way of storing firmware variables. It's a format
> > > > > that can be used of top of various storage devices. Its binding should
> > > > > be an NVMEM layout instead of a standalone device.
> > > > >
> > > > > This patch adds layout binding which allows using it on top of MTD
> > > > > NVMEM
> > > > > device as well as any other. At the same time it deprecates the old
> > > > > combined binding.
> > > >
> > > > I don't understand the issue. From a DT perspective, there isn't. A
> > > > partition is not a device, but is describing the layout of storage
> > > > already.
> > >
> > > Actually I think what Rafał wants to do goes in the right direction but
> > > I also understand from a binding perspective it may be a little
> > > confusing, even more if we consider "NVMEM" a Linux specific concept.
> > >
> > > There is today a "u-boot env" NVMEM *device* description which
> > > almost sits at the same level as eg. an eeprom device. We cannot
> > > compare "an eeprom device" and "a u-boot environment" of course. But
> > > that's truly what is currently described.
> > >
> > > * Current situation
> > >
> > > Flash device -> U-Boot env layout -> NVMEM cells
>
> Isn't it?:
>
> Flash device -> fixed-partitions -> U-Boot env layout -> NVMEM cells
>
> > >
> > > * Improved situation
> > >
> > > Any storage device -> NVMEM -> U-Boot env layout -> NVMEM cells
>
> Why is this better? We don't need a container to say 'this is NVMEM
> stuff' or 'this is MTD stuff'. 'U-Boot env layout' can tell us 'this is
> NVMEM stuff' or whatever the kernel decides in the future.
Yes, I also want the U-boot env layout to tell us "this is nvmem
stuff". But that's not the case today. Today, it says "this is NVMEM
stuff on top of mtd stuff". This was a mistake in the first place, but
this compatible is heavily tight to mtd and cannot work on anything
else. And correcting this is IMO the right direction.
> > > The latter is of course the most relevant description as we expect
> > > storage devices to expose a storage-agnostic interface (NVMEM in
> > > this case) which can then be parsed (by NVMEM layouts) in a storage
> > > agnostic way.
> > >
> > > In the current case, the current U-Boot env binding tells people to
> > > declare the env layout on top of a flash device (only). The current
> > > description also expects a partition node which is typical to flash
> > > devices. Whereas what we should have described in the first place is a
> > > layout that applies on any kind of NVMEM device.
> > >
> > > Bonus point: We've been working the last couple years on clarifying
> > > bindings, especially with mtd partitions (with the partitions{}
> > > container) and NVMEM layouts (with the nvmem-layout{} container).
> > > The switch proposed in this patch makes use of the latter, of course.
> >
> > Thanks Miquèl for filling bits I missed in commit description. Despite
> > years in Linux/DT I still struggle with more complex designs
> > documentation.
> >
> >
> > As per Rob's comment I think I see his point and a possible design
> > confusion. If you look from a pure DT perspective then "partitions" and
> > "nvmem-layout" serve a very similar purpose. They describe device's data
> > content structure. For fixed structures we have very similar
> > "fixed-partitions" and "fixed-cells".
> >
> > If we were to design those bindings today I'm wondering if we couldn't
> > have s/partitions/layout/ and s/nvmem-layout/layout/.
>
> Why!? It is just a name, and we can't get rid of the old names. We don't
> need 2 names.
We need 2 names because we are not capturing the same concepts here?
> > Rob: other than having different bindings for MTD vs. NVMEM layouts I
> > think they overall design makes sense. A single device may have content
> > structurized on more than 1 level:
> > 1.
Re: [PATCH 1/1] cmd: mtd: avoid unintentional integer overflow
Hi Heinrich, heinrich.schucha...@canonical.com wrote on Thu, 11 Jan 2024 08:31:55 +0100: > mtd dump beyond 4 GiB will show incorrect results. > > Multiplying two u32 will yield a u32. Add a missing cast. Good point, thanks for the fix. Reviewed-by: Miquel Raynal Thanks, Miquèl
Re: [PATCH V3 1/6] dt-bindings: nvmem: layouts: add U-Boot environment variables layout
Hello,
r...@kernel.org wrote on Wed, 3 Jan 2024 17:11:29 -0700:
> On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote:
> > From: Rafał Miłecki
> >
> > U-Boot env data is a way of storing firmware variables. It's a format
> > that can be used of top of various storage devices. Its binding should
> > be an NVMEM layout instead of a standalone device.
> >
> > This patch adds layout binding which allows using it on top of MTD NVMEM
> > device as well as any other. At the same time it deprecates the old
> > combined binding.
>
> I don't understand the issue. From a DT perspective, there isn't. A
> partition is not a device, but is describing the layout of storage
> already.
Actually I think what Rafał wants to do goes in the right direction but
I also understand from a binding perspective it may be a little
confusing, even more if we consider "NVMEM" a Linux specific concept.
There is today a "u-boot env" NVMEM *device* description which
almost sits at the same level as eg. an eeprom device. We cannot
compare "an eeprom device" and "a u-boot environment" of course. But
that's truly what is currently described.
* Current situation
Flash device -> U-Boot env layout -> NVMEM cells
* Improved situation
Any storage device -> NVMEM -> U-Boot env layout -> NVMEM cells
The latter is of course the most relevant description as we expect
storage devices to expose a storage-agnostic interface (NVMEM in
this case) which can then be parsed (by NVMEM layouts) in a storage
agnostic way.
In the current case, the current U-Boot env binding tells people to
declare the env layout on top of a flash device (only). The current
description also expects a partition node which is typical to flash
devices. Whereas what we should have described in the first place is a
layout that applies on any kind of NVMEM device.
Bonus point: We've been working the last couple years on clarifying
bindings, especially with mtd partitions (with the partitions{}
container) and NVMEM layouts (with the nvmem-layout{} container).
The switch proposed in this patch makes use of the latter, of course.
Thanks,
Miquèl
Re: [PATCH V2 5/5] nvmem: layouts: add U-Boot env layout
Hi Rafał, zaj...@gmail.com wrote on Tue, 19 Dec 2023 18:40:25 +0100: > From: Rafał Miłecki > > This patch moves all generic (NVMEM devices independent) code from NVMEM Nit: In general we avoid starting with "This patch does..." and instead use the imperative form, like: "Move all generic code..." > device driver to NVMEM layout driver. Then it adds a simple NVMEM layout Then add... > code on top of it. > > Thanks to proper layout it's possible to support U-Boot env data stored > on any kind of NVMEM device. > > For backward compatibility with old DT bindings we need to keep old > NVMEM device driver functional. To avoid code duplication a parsing > function is exported and reused in it. > > Signed-off-by: Rafał Miłecki > --- > V2: Support new compatibles & use device_get_match_data() helper > > IMPORTANT: > This is based on top of the: > [PATCH v6.8 1/2] nvmem: layouts: refactor .add_cells() callback arguments Thanks for the move. Looks good to me: Reviewed-by: Miquel Raynal Thanks, Miquèl
Re: [PATCH V2 4/5] nvmem: u-boot-env: improve coding style
Hi Rafał, zaj...@gmail.com wrote on Tue, 19 Dec 2023 18:40:24 +0100: > From: Rafał Miłecki > > 1. Prefer kzalloc() over kcalloc() >See memory-allocation.rst which says: "to be on the safe side it's >best to use routines that set memory to zero, like kzalloc()" > 2. Drop dev_err() for u_boot_env_add_cells() fail >It can fail only on -ENOMEM. We don't want to print error then. > 3. Add extra "crc32_addr" variable >It makes code reading header's crc32 easier to understand / review. > > Signed-off-by: Rafał Miłecki Looks much nicer :) Reviewed-by: Miquel Raynal Thanks, Miquèl
Re: [PATCH V2 3/5] nvmem: u-boot-env: use more nvmem subsystem helpers
Hi Rafał, zaj...@gmail.com wrote on Tue, 19 Dec 2023 19:16:37 +0100: > On 19.12.2023 19:13, Greg Kroah-Hartman wrote: > > On Tue, Dec 19, 2023 at 06:40:23PM +0100, Rafał Miłecki wrote: > >> From: Rafał Miłecki > >> > >> 1. Use nvmem_dev_size() and nvmem_device_read() to make this driver less > >> mtd dependent > >> 2. Use nvmem_add_one_cell() to simplify adding NVMEM cells > > > > Shouldn't this be 2 different patches? > > I used to maintainers complaining my patches are too small and not the > other way ;) I think it happened two or three times with mtd subsys. A single patch may be too small if it's alone and we don't see the big picture, otherwise I have no issue with small patches, do I? Anyway, in this case I don't mind the patch being split or kept like this, just keep my R-by applied to both if you do indeed split. Thanks, Miquèl
Re: [PATCH 4/4] nvmem: layouts: add U-Boot env layout
Hi Rafał,
zaj...@gmail.com wrote on Mon, 18 Dec 2023 23:10:20 +0100:
> On 18.12.2023 15:21, Miquel Raynal wrote:
> > Hi Rafał,
> >
> > zaj...@gmail.com wrote on Mon, 18 Dec 2023 14:37:22 +0100:
> >
> >> From: Rafał Miłecki
> >>
> >> This patch moves all generic (NVMEM devices independent) code from NVMEM
> >> device driver to NVMEM layout driver. Then it adds a simple NVMEM layout
> >> code on top of it.
> >>
> >> Thanks to proper layout it's possible to support U-Boot env data stored
> >> on any kind of NVMEM device.
> >>
> >> For backward compatibility with old DT bindings we need to keep old
> >> NVMEM device driver functional. To avoid code duplication a parsing
> >> function is exported and reused in it.
> >>
> >> Signed-off-by: Rafał Miłecki
> >> ---
> >
> > I have a couple of comments about the original driver which gets
> > copy-pasted in the new layout driver, maybe you could clean these
> > (the memory leak should be fixed before the migration so it can be
> > backported easily, the others are just style so it can be done after, I
> > don't mind).
> >
> > ...
> >
> >> +int u_boot_env_parse(struct device *dev, struct nvmem_device *nvmem,
> >> + enum u_boot_env_format format)
> >> +{
> >> + size_t crc32_data_offset;
> >> + size_t crc32_data_len;
> >> + size_t crc32_offset;
> >> + size_t data_offset;
> >> + size_t data_len;
> >> + size_t dev_size;
> >> + uint32_t crc32;
> >> + uint32_t calc;
> >> + uint8_t *buf;
> >> + int bytes;
> >> + int err;
> >> +
> >> + dev_size = nvmem_dev_size(nvmem);
> >> +
> >> + buf = kcalloc(1, dev_size, GFP_KERNEL);
> >
> > Out of curiosity, why kcalloc(1,...) rather than kzalloc() ?
>
> I used kcalloc() initially as I didn't need buffer to be zeroed.
I think kcalloc() initializes the memory to zero.
https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L659
If you don't need it you can switch to kmalloc() instead, I don't mind,
but kcalloc() is meant to be used with arrays, I don't see the point of
using kcalloc() in this case.
>
> I see that memory-allocation.rst however says:
> > And, to be on the safe side it's best to use routines that set memory to
> zero, like kzalloc().
>
> It's probably close to zero cost to zero that buffer so it could be kzalloc().
>
>
> >> + if (!buf) {
> >> + err = -ENOMEM;
> >> + goto err_out;
> >
> > We could directly return ENOMEM here I guess.
> >
> >> + }
> >> +
> >> + bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
> >> + if (bytes < 0)
> >> + return bytes;
> >> + else if (bytes != dev_size)
> >> + return -EIO;
> >
> > Don't we need to free buf in the above cases?
> >
> >> + switch (format) {
> >> + case U_BOOT_FORMAT_SINGLE:
> >> + crc32_offset = offsetof(struct u_boot_env_image_single, crc32);
> >> + crc32_data_offset = offsetof(struct u_boot_env_image_single,
> >> data);
> >> + data_offset = offsetof(struct u_boot_env_image_single, data);
> >> + break;
> >> + case U_BOOT_FORMAT_REDUNDANT:
> >> + crc32_offset = offsetof(struct u_boot_env_image_redundant,
> >> crc32);
> >> + crc32_data_offset = offsetof(struct u_boot_env_image_redundant,
> >> data);
> >> + data_offset = offsetof(struct u_boot_env_image_redundant, data);
> >> + break;
> >> + case U_BOOT_FORMAT_BROADCOM:
> >> + crc32_offset = offsetof(struct u_boot_env_image_broadcom,
> >> crc32);
> >> + crc32_data_offset = offsetof(struct u_boot_env_image_broadcom,
> >> data);
> >> + data_offset = offsetof(struct u_boot_env_image_broadcom, data);
> >> + break;
> >> + }
> >> + crc32 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
> >
> > Looks a bit convoluted, any chances we can use intermediate variables
> > to help decipher this?
> >
> >> + crc32_data_len = dev_size - crc32_data_offset;
> >> + data_len = dev_size - data_offset;
> >> +
> >> + calc = crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L;
> >> + if (calc != crc32) {
> >> + dev_err(dev, "Invalid calculated CRC32: 0
Re: [PATCH 4/4] nvmem: layouts: add U-Boot env layout
Hi Rafał,
zaj...@gmail.com wrote on Mon, 18 Dec 2023 14:37:22 +0100:
> From: Rafał Miłecki
>
> This patch moves all generic (NVMEM devices independent) code from NVMEM
> device driver to NVMEM layout driver. Then it adds a simple NVMEM layout
> code on top of it.
>
> Thanks to proper layout it's possible to support U-Boot env data stored
> on any kind of NVMEM device.
>
> For backward compatibility with old DT bindings we need to keep old
> NVMEM device driver functional. To avoid code duplication a parsing
> function is exported and reused in it.
>
> Signed-off-by: Rafał Miłecki
> ---
I have a couple of comments about the original driver which gets
copy-pasted in the new layout driver, maybe you could clean these
(the memory leak should be fixed before the migration so it can be
backported easily, the others are just style so it can be done after, I
don't mind).
...
> +int u_boot_env_parse(struct device *dev, struct nvmem_device *nvmem,
> + enum u_boot_env_format format)
> +{
> + size_t crc32_data_offset;
> + size_t crc32_data_len;
> + size_t crc32_offset;
> + size_t data_offset;
> + size_t data_len;
> + size_t dev_size;
> + uint32_t crc32;
> + uint32_t calc;
> + uint8_t *buf;
> + int bytes;
> + int err;
> +
> + dev_size = nvmem_dev_size(nvmem);
> +
> + buf = kcalloc(1, dev_size, GFP_KERNEL);
Out of curiosity, why kcalloc(1,...) rather than kzalloc() ?
> + if (!buf) {
> + err = -ENOMEM;
> + goto err_out;
We could directly return ENOMEM here I guess.
> + }
> +
> + bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
> + if (bytes < 0)
> + return bytes;
> + else if (bytes != dev_size)
> + return -EIO;
Don't we need to free buf in the above cases?
> + switch (format) {
> + case U_BOOT_FORMAT_SINGLE:
> + crc32_offset = offsetof(struct u_boot_env_image_single, crc32);
> + crc32_data_offset = offsetof(struct u_boot_env_image_single,
> data);
> + data_offset = offsetof(struct u_boot_env_image_single, data);
> + break;
> + case U_BOOT_FORMAT_REDUNDANT:
> + crc32_offset = offsetof(struct u_boot_env_image_redundant,
> crc32);
> + crc32_data_offset = offsetof(struct u_boot_env_image_redundant,
> data);
> + data_offset = offsetof(struct u_boot_env_image_redundant, data);
> + break;
> + case U_BOOT_FORMAT_BROADCOM:
> + crc32_offset = offsetof(struct u_boot_env_image_broadcom,
> crc32);
> + crc32_data_offset = offsetof(struct u_boot_env_image_broadcom,
> data);
> + data_offset = offsetof(struct u_boot_env_image_broadcom, data);
> + break;
> + }
> + crc32 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
Looks a bit convoluted, any chances we can use intermediate variables
to help decipher this?
> + crc32_data_len = dev_size - crc32_data_offset;
> + data_len = dev_size - data_offset;
> +
> + calc = crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L;
> + if (calc != crc32) {
> + dev_err(dev, "Invalid calculated CRC32: 0x%08x (expected:
> 0x%08x)\n", calc, crc32);
> + err = -EINVAL;
> + goto err_kfree;
> + }
> +
> + buf[dev_size - 1] = '\0';
> + err = u_boot_env_parse_cells(dev, nvmem, buf, data_offset, data_len);
> + if (err)
> + dev_err(dev, "Failed to add cells: %d\n", err);
Please drop this error message, the only reason for which the function
call would fail is apparently an ENOMEM case.
> +
> +err_kfree:
> + kfree(buf);
> +err_out:
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(u_boot_env_parse);
> +
> +static int u_boot_env_add_cells(struct device *dev, struct nvmem_device
> *nvmem)
> +{
> + const struct of_device_id *match;
> + struct device_node *layout_np;
> + enum u_boot_env_format format;
> +
> + layout_np = of_nvmem_layout_get_container(nvmem);
> + if (!layout_np)
> + return -ENOENT;
> +
> + match = of_match_node(u_boot_env_of_match_table, layout_np);
> + if (!match)
> + return -ENOENT;
> +
> + format = (uintptr_t)match->data;
In the core there is currently an unused helper called
nvmem_layout_get_match_data() which does that. I think the original
intent of this function was to be used in this driver, so depending on
your preference, can you please either use it or remove it?
> +
> + of_node_put(layout_np);
> +
> + return u_boot_env_parse(dev, nvmem, format);
> +}
Thanks,
Miquèl
Re: [PATCH 2/4] nvmem: core: add nvmem_dev_size() helper
Hi Rafał, zaj...@gmail.com wrote on Mon, 18 Dec 2023 14:37:20 +0100: > From: Rafał Miłecki > > This is required by layouts that need to read whole NVMEM content. It's > especially useful for NVMEM devices without hardcoded layout (like > U-Boot environment data block). Reviewed-by: Miquel Raynal > > Signed-off-by: Rafał Miłecki Thanks, Miquèl
Re: [PATCH 3/4] nvmem: u-boot-env: use more nvmem subsystem helpers
Hi Rafał, zaj...@gmail.com wrote on Mon, 18 Dec 2023 14:37:21 +0100: > From: Rafał Miłecki > > 1. Use nvmem_dev_size() and nvmem_device_read() to make this driver less >mtd dependent > 2. Use nvmem_add_one_cell() to simplify adding NVMEM cells > > Signed-off-by: Rafał Miłecki Reviewed-by: Miquel Raynal Thanks, Miquèl
Re: [PATCH] tqma6: Fix DDR configuration
Hi Fabio, feste...@gmail.com wrote on Tue, 12 Dec 2023 13:11:53 -0300: > On Fri, Nov 17, 2023 at 1:50 PM Miquel Raynal > wrote: > > > > Initially investigating a Linux network issue causing a lot of drop and > > poor network performances on a custom system based on a TQMA6A module > > (based on an iMX6Q), [1st link below]. > > > > I eventually correlated my observations with a contention at the NIC > > level when in concurrency with the graphics pipeline. Troubleshooting > > this in the kernel lead to disabling DMA bursts accesses made by the IPU > > in order to avoid triggering the QoS at the interconnect level, reducing > > from 50 to 10% the drop rate on eth0, [2nd link below]. The solution > > worked on my setup but not on others, which still suffered from > > abnormally high drop rates even with this "fix". > > > > After looking a while into TQ Systems BSP I figured out a number of > > differences in recent U-Boot out-of-tree patches they had in their > > repository [3rd link]. Parsing the differences one after the other lead > > me to this final solution. > > > > The reset pad of the DDR controller was apparently misconfigured, Bit > > 18-19 picturing the "DDR select field". The current value b11 is > > reserved. The only defined value as of version 6 of the iMX6Q manual was > > b00 "DDR3 and LPDDR2 mode". In practice no register difference has been > > spotted after changing this configuration but all issues tracked thus > > far just vanished. All previous fixes have been proven irrelevant. Just > > clearing this field solved all our network issues and the drop rate as > > measured by iperf3 felt back to 0%. > > > > Link: https://lore.kernel.org/netdev/20231012193410.3d1812cf@xps-13/ > > Link: > > https://lists.freedesktop.org/archives/dri-devel/2023-October/428251.html > > Link: > > https://github.com/tq-systems/u-boot-tqmaxx/commit/15eb6abbefbf6916c28467b85485911dad3da6bc > > Signed-off-by: Miquel Raynal > > Applied, thanks. Thanks a lot. Any chances you would have access to additional information to explain how the network vs video pipeline interact without this patch? I am eager to understand. Cheers, Miquèl
[PATCH] nand: Add a watch command
This is a debug command to monitor the retention state of the data on
the array. The command needs a duplication of the mtd_read_oob()
function to actually return the maximum number of bitflips encountered
while reading the page. We could write a specific implementation for the
Sunxi driver but this is probably enough.
nand watch - check an area for bitflips
nand watch.part - check a partition for bitflips
nand watch.chip - check the whole device for bitflips
The output may be a bit verbose and could look like:
=> nand watch.chip
device 0 whole chip
size adjusted to 0xff6 (5 bad blocks)
NAND watch for bitflips in area 0x0-0xff6:
Page 0 (0x) -> error -74
Page 1 (0x0800) -> error -74
Page 2 (0x1000) -> error -74
Page 3 (0x1800) -> error -74
Page 4 (0x2000) -> error -74
Page 5 (0x2800) -> error -74
Page 6 (0x3000) -> error -74
Page 7 (0x3800) -> error -74
Page 8 (0x4000) -> error -74
Page 9 (0x4800) -> error -74
Page 10 (0x5000) -> error -74
Page 11 (0x5800) -> error -74
Page 12 (0x6000) -> error -74
Page 13 (0x6800) -> error -74
Page 14 (0x7000) -> error -74
Page 15 (0x7800) -> error -74
Page 16 (0x8000) -> error -74
Page 17 (0x8800) -> error -74
Page 18 (0x9000) -> error -74
Page 19 (0x9800) -> error -74
Page 20 (0xa000) -> error -74
Page 21 (0xa800) -> error -74
Page 22 (0xb000) -> error -74
Page 23 (0xb800) -> error -74
Page1110 (0x0022b000) -> up to 1 bf/chunk
Page1122 (0x00231000) -> up to 1 bf/chunk
Page1132 (0x00236000) -> up to 1 bf/chunk
Page1362 (0x002a9000) -> up to 1 bf/chunk
Page4990 (0x009bf000) -> up to 1 bf/chunk
Page5728 (0x00b3) -> up to 1 bf/chunk
Page7116 (0x00de6000) -> up to 1 bf/chunk
Page7160 (0x00dfc000) -> up to 1 bf/chunk
Page7494 (0x00ea3000) -> up to 1 bf/chunk
Page 10842 (0x0152d000) -> up to 1 bf/chunk
Page 11614 (0x016af000) -> up to 1 bf/chunk
Page 11970 (0x01761000) -> up to 1 bf/chunk
Page 12536 (0x0187c000) -> up to 1 bf/chunk
Page 12687 (0x018c7800) -> up to 1 bf/chunk
Page 14298 (0x01bed000) -> up to 1 bf/chunk
Page 18268 (0x023ae000) -> up to 1 bf/chunk
Page 18760 (0x024a4000) -> up to 1 bf/chunk
Page 21440 (0x029e) -> up to 1 bf/chunk
Page 22336 (0x02ba) -> up to 1 bf/chunk
Page 22592 (0x02c2) -> up to 1 bf/chunk
Page 23872 (0x02ea) -> up to 1 bf/chunk
Page 27584 (0x035e) -> up to 1 bf/chunk
Page 35008 (0x0446) -> up to 1 bf/chunk
Page 37184 (0x048a) -> up to 1 bf/chunk
Page 41728 (0x0518) -> up to 1 bf/chunk
Page 42176 (0x0526) -> up to 1 bf/chunk
Page 43200 (0x0546) -> up to 1 bf/chunk
Page 43328 (0x054a) -> up to 1 bf/chunk
Page 45376 (0x058a) -> up to 1 bf/chunk
Page 47040 (0x05be) -> up to 1 bf/chunk
Page 47552 (0x05ce) -> up to 1 bf/chunk
Page 49344 (0x0606) -> up to 1 bf/chunk
Page 49856 (0x0616) -> up to 1 bf/chunk
Page 62784 (0x07aa) -> up to 1 bf/chunk
Page 65153 (0x07f40800) -> up to 1 bf/chunk
Page 65228 (0x07f66000) -> up to 1 bf/chunk
Page 65382 (0x07fb3000) -> up to 1 bf/chunk
Page 98624 (0x0c0a) -> up to 1 bf/chunk
Page 101952 (0x0c72) -> up to 1 bf/chunk
Page 107584 (0x0d22) -> up to 1 bf/chunk
Page 118208 (0x0e6e) -> up to 1 bf/chunk
Page 126656 (0x0f76) -> up to 1 bf/chunk
Page 127680 (0x0f96) -> up to 1 bf/chunk
Page 129920 (0x0fdc) -> up to 1 bf/chunk
Maximum number of bitflips: 1
Pages with bitflips: 44/130752
It is also possible to reduce the output with the .quiet suffix in order
to just show the summary.
=> nand watch.chip
device 0 whole chip
size adjusted to 0xff6 (5 bad blocks)
NAND watch for bitflips in area 0x0-0xff6:
Maximum number of bitflips: 1
Pages with bitflips: 44/130752
Signed-off-by: Miquel Raynal
---
Hello, I recently came across a batch of NANDs with a lot of "natural"
bitflips so in order to easily and objectively characterize how
unstable these parts were, I wrote this little tool which was pretty
handy to have in U-Boot. I believe it can be useful for others as well,
so here is the patch.
Cheers, Miquèl
cmd/Kconfig | 5 ++
cmd/nand.c | 103 ++
Re: [PATCH] tqma6: Fix DDR configuration
Hi Alexander, > > > > --- a/board/tq/tqma6/tqma6q.cfg > > > > +++ b/board/tq/tqma6/tqma6q.cfg > > > > @@ -36,7 +36,7 @@ DATA 4, MX6_IOM_DRAM_SDCLK_1, 0x8030 > > > > > > > > DATA 4, MX6_IOM_DRAM_CAS, 0x8030 > > > > DATA 4, MX6_IOM_DRAM_RAS, 0x8030 > > > > DATA 4, MX6_IOM_GRP_ADDDS, 0x0030 > > > > > > > > -DATA 4, MX6_IOM_DRAM_RESET, 0x000C3030 > > > > +DATA 4, MX6_IOM_DRAM_RESET, 0x3030 > > > > > > Thank you for pointing this out. Originally this error came from an > > > older/ancient reference manual. Sorry that we missed to bring this > > > upstream. We will send the changes for DCD data in the next days. > > > > No problem, I'm glad this can now be solved. By any chance, could you > > point to the relevant location of the manual (ddr or imx6 ?) explaining > > what this is actually about? Because I failed to bring any real > > explanation to my observations so far. > > It's a bit hidden but the comment above that list indicates these settings > are > iomuxc configurations. In this case the register > IOMUXC_SW_PAD_CTL_PAD_DRAM_RESET. See i.MX6Q RM Rev.6 05/2020 section > 36.4.347. > Your patch configures the field "DDR Select Field" from reserved3 to > DDR3_LPDDR2. It seems this field has to be set to 0 in every case. Yes, that's also what I get from reading the iMX6Q manual. But I'm sorry I don't understand why a runtime change of a reset pin configuration can be so impacting. It's not like it only affects the power-on sequence because I can reproduce the strange QoS/NIC issues with a devmem once the kernel has started. I believe there is a bit more than just a pad configuration behind this field. Thanks, Miquèl
Re: [PATCH] tqma6: Fix DDR configuration
Hi Markus, markus.nie...@ew.tq-group.com wrote on Thu, 23 Nov 2023 14:04:43 +0100: > Hello Miquel, > > > Initially investigating a Linux network issue causing a lot of drop and > > poor network performances on a custom system based on a TQMA6A module > > (based on an iMX6Q), [1st link below]. > > > > I eventually correlated my observations with a contention at the NIC > > level when in concurrency with the graphics pipeline. Troubleshooting > > this in the kernel lead to disabling DMA bursts accesses made by the IPU > > in order to avoid triggering the QoS at the interconnect level, reducing > > from 50 to 10% the drop rate on eth0, [2nd link below]. The solution > > worked on my setup but not on others, which still suffered from > > abnormally high drop rates even with this "fix". > > > > After looking a while into TQ Systems BSP I figured out a number of > > differences in recent U-Boot out-of-tree patches they had in their > > repository [3rd link]. Parsing the differences one after the other lead > > me to this final solution. > > > > The reset pad of the DDR controller was apparently misconfigured, Bit > > 18-19 picturing the "DDR select field". The current value b11 is > > reserved. The only defined value as of version 6 of the iMX6Q manual was > > b00 "DDR3 and LPDDR2 mode". In practice no register difference has been > > spotted after changing this configuration but all issues tracked thus > > far just vanished. All previous fixes have been proven irrelevant. Just > > clearing this field solved all our network issues and the drop rate as > > measured by iperf3 felt back to 0%. > > > > Link: > > https://lore.kernel.org/netdev/20231012193410.3d1812cf@xps-13/ > > > > Link: > > https://lists.freedesktop.org/archives/dri-devel/2023-October/428251.html > > > > Link: > > https://github.com/tq-systems/u-boot-tqmaxx/commit/15eb6abbefbf6916c28467b85485911dad3da6bc > > > > Signed-off-by: Miquel Raynal < > > miquel.ray...@bootlin.com > > > > > --- > > board/tq/tqma6/tqma6q.cfg | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/board/tq/tqma6/tqma6q.cfg b/board/tq/tqma6/tqma6q.cfg > > index a49489aed3f..a345c4de93d 100644 > > --- a/board/tq/tqma6/tqma6q.cfg > > +++ b/board/tq/tqma6/tqma6q.cfg > > @@ -36,7 +36,7 @@ DATA 4, MX6_IOM_DRAM_SDCLK_1, 0x8030 > > DATA 4, MX6_IOM_DRAM_CAS, 0x8030 > > DATA 4, MX6_IOM_DRAM_RAS, 0x8030 > > DATA 4, MX6_IOM_GRP_ADDDS, 0x0030 > > -DATA 4, MX6_IOM_DRAM_RESET, 0x000C3030 > > +DATA 4, MX6_IOM_DRAM_RESET, 0x3030 > > Thank you for pointing this out. Originally this error came from an > older/ancient reference manual. Sorry that we missed to bring this > upstream. We will send the changes for DCD data in the next days. No problem, I'm glad this can now be solved. By any chance, could you point to the relevant location of the manual (ddr or imx6 ?) explaining what this is actually about? Because I failed to bring any real explanation to my observations so far. Thanks, Miquèl
[PATCH] tqma6: Fix DDR configuration
Initially investigating a Linux network issue causing a lot of drop and poor network performances on a custom system based on a TQMA6A module (based on an iMX6Q), [1st link below]. I eventually correlated my observations with a contention at the NIC level when in concurrency with the graphics pipeline. Troubleshooting this in the kernel lead to disabling DMA bursts accesses made by the IPU in order to avoid triggering the QoS at the interconnect level, reducing from 50 to 10% the drop rate on eth0, [2nd link below]. The solution worked on my setup but not on others, which still suffered from abnormally high drop rates even with this "fix". After looking a while into TQ Systems BSP I figured out a number of differences in recent U-Boot out-of-tree patches they had in their repository [3rd link]. Parsing the differences one after the other lead me to this final solution. The reset pad of the DDR controller was apparently misconfigured, Bit 18-19 picturing the "DDR select field". The current value b11 is reserved. The only defined value as of version 6 of the iMX6Q manual was b00 "DDR3 and LPDDR2 mode". In practice no register difference has been spotted after changing this configuration but all issues tracked thus far just vanished. All previous fixes have been proven irrelevant. Just clearing this field solved all our network issues and the drop rate as measured by iperf3 felt back to 0%. Link: https://lore.kernel.org/netdev/20231012193410.3d1812cf@xps-13/ Link: https://lists.freedesktop.org/archives/dri-devel/2023-October/428251.html Link: https://github.com/tq-systems/u-boot-tqmaxx/commit/15eb6abbefbf6916c28467b85485911dad3da6bc Signed-off-by: Miquel Raynal --- board/tq/tqma6/tqma6q.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/board/tq/tqma6/tqma6q.cfg b/board/tq/tqma6/tqma6q.cfg index a49489aed3f..a345c4de93d 100644 --- a/board/tq/tqma6/tqma6q.cfg +++ b/board/tq/tqma6/tqma6q.cfg @@ -36,7 +36,7 @@ DATA 4, MX6_IOM_DRAM_SDCLK_1, 0x8030 DATA 4, MX6_IOM_DRAM_CAS, 0x8030 DATA 4, MX6_IOM_DRAM_RAS, 0x8030 DATA 4, MX6_IOM_GRP_ADDDS, 0x0030 -DATA 4, MX6_IOM_DRAM_RESET, 0x000C3030 +DATA 4, MX6_IOM_DRAM_RESET, 0x3030 DATA 4, MX6_IOM_DRAM_SDCKE0, 0x3000 DATA 4, MX6_IOM_DRAM_SDCKE1, 0x DATA 4, MX6_IOM_DRAM_SDBA2, 0x -- 2.34.1
Re: [PATCH v4 2/3] dt-bindings: mtd: binman-partition: Add binman compatibles
Hi Simon,
s...@chromium.org wrote on Tue, 24 Oct 2023 14:40:54 -0700:
> Hi Rob,
>
> On Tue, 24 Oct 2023 at 09:16, Rob Herring wrote:
> >
> > On Mon, Oct 09, 2023 at 04:04:14PM -0600, Simon Glass wrote:
> > > Add two compatible for binman entries, as a starting point for the
> > > schema.
> > >
> > > Note that, after discussion on v2, we decided to keep the existing
> > > meaning of label so as not to require changes to existing userspace
> > > software when moving to use binman nodes to specify the firmware
> > > layout.
> > >
> > > Signed-off-by: Simon Glass
> > > ---
> > >
> > > Changes in v4:
> > > - Correct selection of multiple compatible strings
> > >
> > > Changes in v3:
> > > - Drop fixed-partitions from the example
> > > - Use compatible instead of label
> > >
> > > Changes in v2:
> > > - Use plain partition@xxx for the node name
> > >
> > > .../mtd/partitions/binman-partition.yaml | 49 +++
> > > 1 file changed, 49 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml
> > > b/Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml
> > > new file mode 100644
> > > index ..35a320359ec1
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml
> > > @@ -0,0 +1,49 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +# Copyright 2023 Google LLC
> > > +
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mtd/partitions/binman-partition.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Binman partition
> > > +
> > > +maintainers:
> > > + - Simon Glass
> > > +
> > > +select: false
> >
> > So this schema is never used. 'select: false' is only useful if
> > something else if referencing the schema.
>
> OK. Is there a user guide to this somewhere? I really don't understand
> it very well.
The example-schema.yaml at the root of the dt-bindings directory is
well commented.
> > > +description: |
> > > + This corresponds to a binman 'entry'. It is a single partition which
> > > holds
> > > + data of a defined type.
> > > +
> > > +allOf:
> > > + - $ref: /schemas/mtd/partitions/partition.yaml#
> > > +
> > > +properties:
> > > + compatible:
> > > +oneOf:
> > > + - const: binman,entry # generic binman entry
> >
> > 'binman' is not a vendor. You could add it if you think that's useful.
> > Probably not with only 1 case...
>
> I think it is best to use this for generic things implemented by
> binman, rather than some other project. For example, binman supports a
> 'fill' region. It also supports sections which are groups of
> sub-entries. So we will likely start with half a dozen of these and it
> will likely grow: binman,fill, binman,section, binman,files
>
> If we don't use 'binman', what do you suggest?
>
> >
> > > + - items:
> > > + - const: u-boot # u-boot.bin from U-Boot project
> > > + - const: atf-bl31 # bl31.bin or bl31.elf from TF-A project
> > >
> >
> > Probably should use the new 'tfa' rather than old 'atf'. Is this the
> > only binary for TFA? The naming seems inconsistent in that every image
> > goes in (or can go in) a bl?? section. Why does TFA have it but u-boot
> > doesn't? Perhaps BL?? is orthogonal to defining what is in each
> > partition. Perhaps someone more familar with all this than I am can
> > comment.
>
> From what I can tell TF-A can produce all sorts of binaries, of which
> bl31 is one. U-Boot can also produce lots of binaries, but its naming
> is different (u-boot, u-boot-spl, etc.). Bear in mind that U-Boot is
> used on ARM, where this terminology is defined, and on x86 (for
> example), where it is not.
>
> >
> > Once you actually test this, you'll find you are specifying:
> >
> > compatible = "u-boot", "atf-bl31";
>
> I don't understand that, sorry. I'll send a v5 and see if the problem goes
> away.
For me this means the partition contains U-Boot and TF-A, which is
probably not what you want. I believe Rob is saying that how you define
the compatible property above does not match the examples below. Did
you run make dt_binding_check?
Also, do you really need to say which software project provides a
component? Would using "bl31", "bl33", etc be enough? Or maybe you
could have eg. "bl31-tf-a" and "bl31-u-boot-spl" (in this order) for
clarity? This way one knows which stage a partition contains and also
the software project which provided it.
To be honest I still don't fully get where you want to go and I believe
a more complete schema would probably help, with different examples, to
catch what you need and why.
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > +partitions {
> > > +compatible = "binman";
> > > +
Re: [PATCH v4] dt-bindings: mtd: fixed-partitions: Add compression property
On Wed, 2023-09-27 at 18:05:43 UTC, Simon Glass wrote: > Sometimes the contents of a partition are compressed. Add a property to > express this and define the algorithm used. > > Signed-off-by: Simon Glass Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
[PATCH v3 3/3] usb: udc: Try to clarify an error message
At some point when trying to use USB gadgets, two situations may arise
and lead to a failure. Either the UDC (USB Device Controller) is not
available at all (not described or not probed) or the UDC is already in
use. For instance, as the USB Ethernet gadget remains bound to the UDC,
the use of any other USB gadget (fastboot, dfu, etc) *after* will always
fail with the "couldn't find an available UDC" error.
Let's give a more helpful message by making a difference between the two
cases. Let's also hint people who would get this error and grep it into
the sources a better explanation of what's wrong with their workflow.
Signed-off-by: Miquel Raynal
Reviewed-by: Mattijs Korpershoek
---
While doing this I really wanted to add "much more" error comments but I
faced another reality: often the messages are there but use
pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
I consider this unnecessary, as decreasing the loglevel will make these
messages appear. I would have expected errors to be displayed, but I
understand it makes the binaries even bigger.
---
drivers/usb/gadget/udc/udc-core.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index 7f73926cb3e..8405b03462e 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -323,6 +323,7 @@ err1:
int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
{
struct usb_udc *udc = NULL;
+ unsigned intudc_count = 0;
int ret;
if (!driver || !driver->bind || !driver->setup)
@@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver
*driver)
mutex_lock(_lock);
list_for_each_entry(udc, _list, list) {
+ udc_count++;
+
/* For now we take the first one */
if (!udc->driver)
goto found;
}
- printf("couldn't find an available UDC\n");
+ if (!udc_count)
+ printf("No UDC available in the system\n");
+ else
+ /* When this happens, users should 'unbind '
+* using the output of 'dm tree' and looking at the line right
+* after the USB peripheral/device controller.
+*/
+ printf("All UDCs in use (%d available), use the unbind
command\n",
+ udc_count);
mutex_unlock(_lock);
return -ENODEV;
found:
--
2.34.1
[PATCH v3 2/3] cmd: bind: Try to improve the (un)bind help
While it may sound totally obvious for the regular U-Boot developer to get the parameters of the bind/unbind commands from the output of 'dm tree', it did not felt straightforward to me until I was explicitly told to look there. And even when I knew the command, I did not make a direct link between the arguments of this command and the columns returned by 'dm tree'. Several of us lost a lot of time because of that, I would like to kindly help other users by slightly improving this textual line. Unfortunately, because of how this string is used (like within the 'help' command) I cannot detail much more, but at least the pointer is there. While we add this message, we can also imply CMD_DM when we enable CMD_BIND so the debug message does not lead to an unknown command. This way the 'dm' command will likely be there unless explicitly disabled. Signed-off-by: Miquel Raynal Reviewed-by: Simon Glass Reviewed-by: Mattijs Korpershoek --- cmd/Kconfig | 1 + cmd/bind.c | 4 2 files changed, 5 insertions(+) diff --git a/cmd/Kconfig b/cmd/Kconfig index 6cc3bf6c2d0..668411fd87c 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -996,6 +996,7 @@ config CMD_BCB config CMD_BIND bool "bind/unbind - Bind or unbind a device to/from a driver" depends on DM + imply CMD_DM help Bind or unbind a device to/from a driver from the command line. This is useful in situations where a device may be handled by several diff --git a/cmd/bind.c b/cmd/bind.c index 4d1b7885e60..be0d4d2a711 100644 --- a/cmd/bind.c +++ b/cmd/bind.c @@ -246,6 +246,8 @@ U_BOOT_CMD( "Bind a device to a driver", " \n" "bind \n" + "Use 'dm tree' to list all devices registered in the driver model,\n" + "their path, class, index and current driver.\n" ); U_BOOT_CMD( @@ -254,4 +256,6 @@ U_BOOT_CMD( "\n" "unbind \n" "unbind \n" + "Use 'dm tree' to list all devices registered in the driver model,\n" + "their path, class, index and current driver.\n" ); -- 2.34.1
[PATCH v3 1/3] cmd: Change the dependencies between CMD_BIND and USB_GADGET
Today CMD_BIND defaults to 'y' when USB_ETHER is enabled. In practice, CMD_BIND should default to 'y' when any USB gadget is enabled not only USB_ETHER. Let's invert the logic of the dependency and use the weak 'imply' keyword to enforce this. Signed-off-by: Miquel Raynal --- cmd/Kconfig| 1 - drivers/usb/gadget/Kconfig | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/Kconfig b/cmd/Kconfig index 43ca10f69cc..6cc3bf6c2d0 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -996,7 +996,6 @@ config CMD_BCB config CMD_BIND bool "bind/unbind - Bind or unbind a device to/from a driver" depends on DM - default y if USB_ETHER help Bind or unbind a device to/from a driver from the command line. This is useful in situations where a device may be handled by several diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 1cfe6022842..44f47a07207 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -17,6 +17,7 @@ menuconfig USB_GADGET bool "USB Gadget Support" depends on DM select DM_USB + imply CMD_BIND help USB is a master/slave protocol, organized with one master host (such as a PC) controlling up to 127 peripheral devices. -- 2.34.1
[PATCH v3 0/3] Improve the experience with USB gadgets
Try to ease the use of USB gadgets for people not fully aware of all the terms and constraints. With more helpful error messages a little bit of guidance anyone should be able to adapt to the bind/unbind game that is now necessary with USB gadgets. The idea is to eventually get proper USB composite gadget support, but that is a longer term goal. Changes in v3: * First patch is new. * Collected the tags received. * Imply CMD_DM from CMD_BIND. Miquel Raynal (3): cmd: Change the dependencies between CMD_BIND and USB_GADGET cmd: bind: Try to improve the (un)bind help usb: udc: Try to clarify an error message cmd/Kconfig | 2 +- cmd/bind.c| 4 drivers/usb/gadget/Kconfig| 1 + drivers/usb/gadget/udc/udc-core.c | 13 - 4 files changed, 18 insertions(+), 2 deletions(-) -- 2.34.1
Re: [PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message
Hi Marek,
> >>> --- a/drivers/usb/gadget/udc/udc-core.c
> >>> +++ b/drivers/usb/gadget/udc/udc-core.c
> >>> @@ -323,6 +323,7 @@ err1:
> >>>int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
> >>>{
> >>> struct usb_udc *udc = NULL;
> >>> + unsigned intudc_count = 0;
> >>> int ret;
> >>>> if (!driver || !driver->bind || !driver->setup)
> >>> @@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct
> >>> usb_gadget_driver *driver)
> >>>> mutex_lock(_lock);
> >>> list_for_each_entry(udc, _list, list) {
> >>> + udc_count++;
> >>> +
> >>> /* For now we take the first one */
> >>> if (!udc->driver)
> >>> goto found;
> >>> }
> >>>> -printf("couldn't find an available UDC\n");
> >>> + if (!udc_count)
> >>> + printf("No UDC available in the system\n");
> >>> + else
> >>> + /* When this happens, users should 'unbind '
> >>> + * using the output of 'dm tree' and looking at the line right
> >>> + * after the USB peripheral/device controller.
> >>> + */
> >>> + printf("All UDCs in use (%d available), use the unbind
> >>> command\n",
> >>> +udc_count);
> >>
> >> Users should really use 'bind' command in the first place, to avoid this
> >> guesswork to which UDC (on systems with multiple UDCs) a gadget gets bound
> >> to, and instead bind the gadget to specific UDC they want to bind it to.
> >> This code should then be removed entirely.
> >
> > There are two cases when this can happen:
> > * a single UDC (common) but a function already bound, there is no
> >guessing here
> > * two (or more) UDC and there is some guessing
> >
> > Before your cleanup, drivers would get automatically unbound from the
> > UDC to let the one being needed to bind.
>
> How did that ever work ?
I believe both commands (fastboot vs. network commands like dhcp, tftp,
etc) would automatically bind to the first UDC and unbind once the
command done. But you recently changed that.
> > This is no longer the case,
> > so there is a change in the CLI and I want to help people facing
> > this new problem with a slightly more comprehensive message because
> > people don't fully understand what USB is all about. We cannot
> > ask all U-Boot USB users to be U-Boot experts and USB experts. This
> > is just a little help.
>
> In that case, you have to handle the details like CMD_BIND may not be
> enabled, and CMD_DM may not be enabled.
If I understand correctly you would like this to be described:
CMD_BIND imply CMD_DM
USB_GADGET imply CMD_BIND
Currently there is:
CMD_BIND default y if USB_ETHER
Maybe we should instead have:
CMD_BIND default y if USB_GADGET
And perhaps I would even go further and change the dependency
direction, so:
USB_GADGET imply CMD_BIND
CMD_BIND imply CMD_DM
Would this work for you?
> But then, maybe what we want to do is fix the automatic switching of
> gadgets to make it work again ?
This is a longer term goal I guess.
> > In an ideal world, we will have the possibility to create
> > composite gadgets and all this can go away once it is well
> > integrated, I guess?
>
> I believe you would still have to do a disconnect/connect cycle even with a
> composite driver, you cannot just add functions to existing composite at
> runtime.
I have in mind an environment variable which could define the "default"
composite gadget that we want at boot time, but then if people want to
change the gadgets exposed they would indeed need some kind of
disconnect/connect machinery, agreed.
Thanks, Miquèl
Re: [PATCH RESEND v2 1/2] cmd: bind: Try to improve the (un)bind help
Hi Marek, ma...@denx.de wrote on Thu, 5 Oct 2023 15:01:51 +0200: > On 10/2/23 15:46, Miquel Raynal wrote: > > While it may sound totally obvious for the regular U-Boot developer to > > get the parameters of the bind/unbind commands from the output of 'dm > > tree', it did not felt straightforward to me until I was explicitly > > told to look there. And even when I knew the command, I did not make a > > direct link between the arguments of this command and the columns > > returned by 'dm tree'. > > > > Several of us lost a lot of time because of that, I would like to kindly > > help other users by slightly improving this textual line. Unfortunately, > > because of how this string is used (like within the 'help' command) I > > cannot detail much more, but at least the pointer is there. > > > > Signed-off-by: Miquel Raynal > > --- > > Resend: no change. > > > > Changes in v2: > > * Moved the details in the long text section as suggested by Heinrich. > > * Rephrased the help text as well, not fully following Heinrich > >suggestion, but taking inspiration from it. > > --- > > cmd/bind.c | 4 > > 1 file changed, 4 insertions(+) > > > > diff --git a/cmd/bind.c b/cmd/bind.c > > index 4d1b7885e60..be0d4d2a711 100644 > > --- a/cmd/bind.c > > +++ b/cmd/bind.c > > @@ -246,6 +246,8 @@ U_BOOT_CMD( > > "Bind a device to a driver", > > " \n" > > "bind \n" > > + "Use 'dm tree' to list all devices registered in the driver model,\n" > > + "their path, class, index and current driver.\n" > > ); > > > U_BOOT_CMD( > > @@ -254,4 +256,6 @@ U_BOOT_CMD( > > "\n" > > "unbind \n" > > "unbind \n" > > + "Use 'dm tree' to list all devices registered in the driver model,\n" > > + "their path, class, index and current driver.\n" > > 'dm tree' depends on CMD_DM , you might need some if (IS_ENABLED()) here. Well, no, and that's my point actually. People need to know that this command exist and should be used for this purpose. If you only tell them "use 'dm tree'" conditionally, you loose part of the audience: people not aware of this command or its usefulness. Thanks, Miquèl
Re: [PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message
Hi Marek,
ma...@denx.de wrote on Thu, 5 Oct 2023 15:04:25 +0200:
> On 10/2/23 15:46, Miquel Raynal wrote:
> > At some point when trying to use USB gadgets, two situations may arise
> > and lead to a failure. Either the UDC (USB Device Controller) is not
> > available at all (not described or not probed) or the UDC is already in
> > use. For instance, as the USB Ethernet gadget remains bound to the UDC,
> > the use of any other USB gadget (fastboot, dfu, etc) *after* will always
> > fail with the "couldn't find an available UDC" error.
> >
> > Let's give a more helpful message by making a difference between the two
> > cases. Let's also hint people who would get this error and grep it into
> > the sources a better explanation of what's wrong with their workflow.
> >
> > Signed-off-by: Miquel Raynal
> > ---
> > While doing this I really wanted to add "much more" error comments but I
> > faced another reality: often the messages are there but use
> > pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
> > I consider this unnecessary, as decreasing the loglevel will make these
> > messages appear. I would have expected errors to be displayed, but I
> > understand it makes the binaries even bigger.
> >
> > Resend: no change.
> >
> > Changes in v2:
> > * s/UDC/UDCs/. I kept the sentence as it was as the suggested form did
> >not sound well at all when there is only one UDC.
> > ---
> > drivers/usb/gadget/udc/udc-core.c | 13 -
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/udc/udc-core.c
> > b/drivers/usb/gadget/udc/udc-core.c
> > index 7f73926cb3e..8405b03462e 100644
> > --- a/drivers/usb/gadget/udc/udc-core.c
> > +++ b/drivers/usb/gadget/udc/udc-core.c
> > @@ -323,6 +323,7 @@ err1:
> > int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
> > {
> > struct usb_udc *udc = NULL;
> > + unsigned intudc_count = 0;
> > int ret;
> > > if (!driver || !driver->bind || !driver->setup)
> > @@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver
> > *driver)
> > > mutex_lock(_lock);
> > list_for_each_entry(udc, _list, list) {
> > + udc_count++;
> > +
> > /* For now we take the first one */
> > if (!udc->driver)
> > goto found;
> > }
> > > - printf("couldn't find an available UDC\n");
> > + if (!udc_count)
> > + printf("No UDC available in the system\n");
> > + else
> > + /* When this happens, users should 'unbind '
> > +* using the output of 'dm tree' and looking at the line right
> > +* after the USB peripheral/device controller.
> > +*/
> > + printf("All UDCs in use (%d available), use the unbind
> > command\n",
> > + udc_count);
>
> Users should really use 'bind' command in the first place, to avoid this
> guesswork to which UDC (on systems with multiple UDCs) a gadget gets bound
> to, and instead bind the gadget to specific UDC they want to bind it to. This
> code should then be removed entirely.
There are two cases when this can happen:
* a single UDC (common) but a function already bound, there is no
guessing here
* two (or more) UDC and there is some guessing
Before your cleanup, drivers would get automatically unbound from the
UDC to let the one being needed to bind. This is no longer the case,
so there is a change in the CLI and I want to help people facing
this new problem with a slightly more comprehensive message because
people don't fully understand what USB is all about. We cannot
ask all U-Boot USB users to be U-Boot experts and USB experts. This
is just a little help.
In an ideal world, we will have the possibility to create
composite gadgets and all this can go away once it is well
integrated, I guess?
Thanks,
Miquèl
Re: [PATCH v2 1/3] dt-bindings: mtd: fixed-partitions: Add binman compatible
Hi Simon,
s...@chromium.org wrote on Mon, 2 Oct 2023 11:49:40 -0600:
> Add a compatible string for binman, so we can extend fixed-partitions
> in various ways.
I've been thinking at the proper way to describe the binman partitions.
I am wondering if we should really extend the fixed-partitions
schema. This description is really basic and kind of supposed to remain
like that. Instead, I wonder if we should not just keep the binman
compatible alone, like many others already. This way it would be very clear
what is expected and allowed in both cases. I am thinking about
something like that:
Documentation/devicetree/bindings/mtd/partitions/brcm,bcm4908-partitions.yaml
this file is also referenced there (but this patch does the same, which
is what I'd expect):
Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
I'll let the binding maintainers judge whether they think it's
relevant, it's not a strong opposition.
> Signed-off-by: Simon Glass
> ---
[...]
> +properties:
> + compatible:
> +const: binman
Right now this does not fit (I believe) the example. But if we no
longer extend fixed-partitions but just create binman.yaml, this will
probably be enough.
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +partitions {
> +compatible = "binman", "fixed-partitions";
> +#address-cells = <1>;
> +#size-cells = <1>;
> +
> +partition@10 {
> +label = "u-boot";
> +reg = <0x10 0xf0>;
> +};
> +};
Thanks,
Miquèl
Re: [PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message
Hi Massimo, massimo.pegorer+...@gmail.com wrote on Mon, 2 Oct 2023 16:37:10 +0200: > Hi Miquel, > > Il giorno lun 2 ott 2023 alle ore 15:46 Miquel Raynal > ha scritto: > > > > At some point when trying to use USB gadgets, two situations may arise > > [...cut...] > > > Signed-off-by: Miquel Raynal > > --- > > While doing this I really wanted to add "much more" error comments but I > > faced another reality: often the messages are there but use > > pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so > > I consider this unnecessary, as decreasing the loglevel will make these > > messages appear. I would have expected errors to be displayed, but I > > understand it makes the binaries even bigger. > > This is how it works for pr_err but not for log_err: if you are not > using CONFIG_LOG, all log_xxx with level less than or equal to INFO > are in the binary. On the other hand, if you have CONFIG_LOG=y, > log_xxx are left out based on LOG_MAX_LEVEL value, while pr_xxx are > left out based on both LOGLEVEL and LOG_MAX_LEVEL too (due to the fact > that pr_xxx relies on log_xxx). This is quite confusing IMO. I'm > working on a proposal for a simpler and clearer unified way to log in > U-Boot. Very interesting (and, as you said, way too confusing). I did not even notice the difference, thanks for the explanation. Indeed, it would be nice to simplify this a bit. Thanks, Miquèl
[PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message
At some point when trying to use USB gadgets, two situations may arise
and lead to a failure. Either the UDC (USB Device Controller) is not
available at all (not described or not probed) or the UDC is already in
use. For instance, as the USB Ethernet gadget remains bound to the UDC,
the use of any other USB gadget (fastboot, dfu, etc) *after* will always
fail with the "couldn't find an available UDC" error.
Let's give a more helpful message by making a difference between the two
cases. Let's also hint people who would get this error and grep it into
the sources a better explanation of what's wrong with their workflow.
Signed-off-by: Miquel Raynal
---
While doing this I really wanted to add "much more" error comments but I
faced another reality: often the messages are there but use
pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
I consider this unnecessary, as decreasing the loglevel will make these
messages appear. I would have expected errors to be displayed, but I
understand it makes the binaries even bigger.
Resend: no change.
Changes in v2:
* s/UDC/UDCs/. I kept the sentence as it was as the suggested form did
not sound well at all when there is only one UDC.
---
drivers/usb/gadget/udc/udc-core.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index 7f73926cb3e..8405b03462e 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -323,6 +323,7 @@ err1:
int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
{
struct usb_udc *udc = NULL;
+ unsigned intudc_count = 0;
int ret;
if (!driver || !driver->bind || !driver->setup)
@@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver
*driver)
mutex_lock(_lock);
list_for_each_entry(udc, _list, list) {
+ udc_count++;
+
/* For now we take the first one */
if (!udc->driver)
goto found;
}
- printf("couldn't find an available UDC\n");
+ if (!udc_count)
+ printf("No UDC available in the system\n");
+ else
+ /* When this happens, users should 'unbind '
+* using the output of 'dm tree' and looking at the line right
+* after the USB peripheral/device controller.
+*/
+ printf("All UDCs in use (%d available), use the unbind
command\n",
+ udc_count);
mutex_unlock(_lock);
return -ENODEV;
found:
--
2.34.1
[PATCH RESEND v2 1/2] cmd: bind: Try to improve the (un)bind help
While it may sound totally obvious for the regular U-Boot developer to get the parameters of the bind/unbind commands from the output of 'dm tree', it did not felt straightforward to me until I was explicitly told to look there. And even when I knew the command, I did not make a direct link between the arguments of this command and the columns returned by 'dm tree'. Several of us lost a lot of time because of that, I would like to kindly help other users by slightly improving this textual line. Unfortunately, because of how this string is used (like within the 'help' command) I cannot detail much more, but at least the pointer is there. Signed-off-by: Miquel Raynal --- Resend: no change. Changes in v2: * Moved the details in the long text section as suggested by Heinrich. * Rephrased the help text as well, not fully following Heinrich suggestion, but taking inspiration from it. --- cmd/bind.c | 4 1 file changed, 4 insertions(+) diff --git a/cmd/bind.c b/cmd/bind.c index 4d1b7885e60..be0d4d2a711 100644 --- a/cmd/bind.c +++ b/cmd/bind.c @@ -246,6 +246,8 @@ U_BOOT_CMD( "Bind a device to a driver", " \n" "bind \n" + "Use 'dm tree' to list all devices registered in the driver model,\n" + "their path, class, index and current driver.\n" ); U_BOOT_CMD( @@ -254,4 +256,6 @@ U_BOOT_CMD( "\n" "unbind \n" "unbind \n" + "Use 'dm tree' to list all devices registered in the driver model,\n" + "their path, class, index and current driver.\n" ); -- 2.34.1
Re: [PATCH v2 01/17] dm: usb: udc: Factor out plain udevice handler functions
Hi Marek, ma...@denx.de wrote on Sat, 30 Sep 2023 23:11:17 +0200: > On 9/27/23 15:59, Miquel Raynal wrote: > > Hi Marek, > > Hi, > > > miquel.ray...@bootlin.com wrote on Fri, 22 Sep 2023 12:00:12 +0200: > > > >> Hi Marek, > >> > >> I'm answering here as there is no cover letter. Just to let you know > >> I'm still concerned by the series and want to test it but did not had > >> the time to do so recently. Hopefully next week. > > > > The series looks good to me and works as well on a Beagle Bone Black > > with no visible functional changes regarding the use of the UDC. The > > whole series is: > > > > Tested-by: Miquel Raynal > > > > By the way, following your initial series there have been three > > followup patches trying to improve a little bit the doc, one got merged > > and two others were delegated to you: > > https://patchwork.ozlabs.org/project/uboot/list/?series=367635 > > > > They are almost 2 months old now, would you mind acking or merging > > them so both your initial USB gadget rework and the additional > > (related) doc can be in the same release please? > > Can you resend those and CC this mail address ? Of course! > That said, 1/2 should be in some sort of README instead, and the 'dm tree' > command depends on CMD_DM=y . Well, the README is more something which targets the developer IMO, whereas here I want to target the users. Many people only see U-Boot through Yocto now (that's a different topic) and the purpose of this additional text is to help them in finding their way into U-Boot device model *alone*. It's really short, but I bet it can really help, given how I felt when I actually understood why you advised to type a couple of semi-random bind/unbind commands which eventually worked. Anybody not involved in any DM conversion is likely just not aware of all of that. Having people finding their own way through the DM means less complaints/needs for help on the mailing list. > The usb_gadget_probe_driver() is code synchronized from kernel, you likely > want to add any such clarification to usb_gadget_register_driver() . Actually the kernel functions do have some kind of alternate error message as well now :-) Clearly different given that this code has been adapted to U-Boot's DM, but the idea is close. However usb_gadget_register_driver() is kind of blind regarding the number of UDCs in the system and if they are already bound or not, so I don't really see the point in moving the error message there along with all the logic duplication involved. > That said, the usb_gadget_register_driver() should be reworked into > some new functions, which takes UDC controller *udevice pointer to > avoid this binding heuristics. Yes, I don't know many boards with two UDCs but it's a clear limitation. Thanks, Miquèl
Re: [PATCH 1/3] dt-bindings: mtd: fixed-partitions: Add binman compatible
Hi Simon,
s...@chromium.org wrote on Wed, 27 Sep 2023 14:20:51 -0600:
> Add a compatible string for binman, so we can extend fixed-partitions
> in various ways.
>
> Signed-off-by: Simon Glass
> ---
>
> .../bindings/mtd/partitions/binman.yaml | 49 +++
> .../mtd/partitions/fixed-partitions.yaml | 6 +++
> .../bindings/mtd/partitions/partitions.yaml | 1 +
> MAINTAINERS | 5 ++
> 4 files changed, 61 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/mtd/partitions/binman.yaml
>
> diff --git a/Documentation/devicetree/bindings/mtd/partitions/binman.yaml
> b/Documentation/devicetree/bindings/mtd/partitions/binman.yaml
> new file mode 100644
> index ..34fd10c1a318
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/partitions/binman.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Google LLC
> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/partitions/binman.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Binman firmware layout
> +
> +maintainers:
> + - Simon Glass
> +
> +select: false
> +
> +description: |
> + The binman node provides a layout for firmware, used when packaging
> firmware
> + from multiple projects. It is based on fixed-partitions, with some
> + extensions.
Could you mention the input file vs. output file and which one this
binding describes?
> +
> + Documentation for Binman is available at:
> +
> + https://u-boot.readthedocs.io/en/latest/develop/package/binman.html
> +
> + with the current image-description format at:
> +
> +
> https://u-boot.readthedocs.io/en/latest/develop/package/binman.html#image-description-format
> +
> +allOf:
> + - $ref: /schemas/mtd/partitions/fixed-partitions.yaml#
> +
> +properties:
> + compatible:
> +const: binman
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +partitions {
> +compatible = "binman", "fixed-partitions";
> +#address-cells = <1>;
> +#size-cells = <1>;
> +
> +partition-u-boot@10 {
Do you mind if we avoid playing with the node name? I would prefer:
partition@10 {
label = "foo";
> +label = "u-boot";
> +reg = <0x10 0xf0>;
> +};
> +};
> diff --git
> a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> index 331e564f29dc..1c04bc2b95af 100644
> --- a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> @@ -14,6 +14,9 @@ description: |
>The partition table should be a node named "partitions". Partitions are
> then
>defined as subnodes.
>
> + The Binman tool provides some enhanced features, so provides a compatible
> + string to indicate that these are permitted.
I believe this is not necessary and is implied by the $ref in
partitions.yaml.
> maintainers:
>- Rafał Miłecki
>
> @@ -24,6 +27,9 @@ properties:
>- items:
>- const: sercomm,sc-partitions
>- const: fixed-partitions
> + - items:
> + - const: binman
> + - const: fixed-partitions
>
>"#address-cells": true
>
> diff --git a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> index 1dda2c80747b..849fd15d085c 100644
> --- a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> +++ b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> @@ -15,6 +15,7 @@ maintainers:
>
> oneOf:
>- $ref: arm,arm-firmware-suite.yaml
> + - $ref: binman.yaml
>- $ref: brcm,bcm4908-partitions.yaml
>- $ref: brcm,bcm947xx-cfe-partitions.yaml
>- $ref: fixed-partitions.yaml
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5f18c6ba3c3c..367c843ec348 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3517,6 +3517,11 @@ F: Documentation/filesystems/bfs.rst
> F: fs/bfs/
> F: include/uapi/linux/bfs_fs.h
>
> +BINMAN
> +M: Simon Glass
> +S: Supported
> +F: Documentation/devicetree/bindings/mtd/partitions/binman*
> +
> BITMAP API
> M: Yury Norov
> R: Andy Shevchenko
The rest of the series otherwise lgtm.
Thanks,
Miquèl
Re: [PATCH v2 01/17] dm: usb: udc: Factor out plain udevice handler functions
Hi Marek, miquel.ray...@bootlin.com wrote on Fri, 22 Sep 2023 12:00:12 +0200: > Hi Marek, > > I'm answering here as there is no cover letter. Just to let you know > I'm still concerned by the series and want to test it but did not had > the time to do so recently. Hopefully next week. The series looks good to me and works as well on a Beagle Bone Black with no visible functional changes regarding the use of the UDC. The whole series is: Tested-by: Miquel Raynal By the way, following your initial series there have been three followup patches trying to improve a little bit the doc, one got merged and two others were delegated to you: https://patchwork.ozlabs.org/project/uboot/list/?series=367635 They are almost 2 months old now, would you mind acking or merging them so both your initial USB gadget rework and the additional (related) doc can be in the same release please? Thanks, Miquèl
Re: [PATCH] dt-bindings: mtd: Add a schema for binman
Hello, > > > > > These are firmware bindings, as indicated, but I > > > > > took them out of the /firmware node since that is for a different > > > > > purpose. Rob suggested that partitions was a good place. We have fwupd > > > > > using DT to hold the firmware-update information, so I expect it will > > > > > move to use these bindings too. > > > > > > > > I would definitely use fixed partitions as that's what you need then: > > > > registering where everything starts and ends. If you have "in-band" > > > > meta data you might require a compatible, but I don't think you > > > > do, in this case you should probably carry the content through a label > > > > (which will become the partition name) and we can discuss additional > > > > properties if needed. > > > > > > I believe I am going to need a compatible string at the 'partitions' > > > level to indicate that this is the binman scheme. But we can leave > > > that until later. > > > > Perhaps: > > > > compatible = "binman", "fixed-partitions"; > > > > Though I don't understand why binman couldn't just understand what > > "fixed-partitions" means rather than "binman". > > Well so long as we don't add any binman things in here, you are right. > > But the eventual goal is parity with current Binman functionality, > which writes the entire (augmented) description to the DT, allowing > tools to rebuild / repack / replace pieces later, maintaining the same > alignment constraints, etc. I am assuming that properties like 'align > = <16>' would not fit with fixed-partitions. I am personally not bothered by this kind of properties. But if we plan on adding too much properties, I will advise to indeed use another name than fixed-partitions (or add the "binman" secondary compatible) otherwise it's gonna be hard to support in the code while still restraining as much as we can the other partition schema. > But if we don't preserve > these properties then Binman cannot do repacking reliably. Perhaps for > now I could put the augmented DT in its own section somewhere, but I > am just not sure if that will work in a real system. E.g. with VBE the > goal is to use the DT to figure out how to access the firmware, update > it, etc. > > Is it not possible to have my own node with whatever things Binman > needs in it (subject to review of course)? i.e. could we discuss how > to encode it, but argue less about whether things are needed? I > kind-of feel I know what is needed, since I wrote the tool. > > > > > > > > So you are suggesting 'label' for the contents. Rob suggested > > > 'compatible' [1], so what should I do? > > > > "label" is for consumption by humans, not tools/software. Compatible > > values are documented, label values are not. Though the partition > > stuff started out using label long ago and it's evolved to preferring > > compatible. > > OK so we are agreed that we are going with 'compatible'. Still strongly disagree here. My understanding is that a compatible carries how the content is organized, and how this maybe specific (like you have in-band meta data data that needs to be parsed in a specific way or in your case additional specific properties in the DT which give more context about how the data is stored). But the real content of the partition, ie. if it contains a firmware, the kernel or some user data does not belong to the compatible. I.e: - The first byte of my partition gives the compression algorithm: -> compatible = "compressed-partition-foo"; or -> compatible = "fixed-partitions" + compression-algorithm = "foo"; - The partition contains a picture of my dog: -> label = "my dog is beautiful" but certainly not -> compatible = "my-dog"; I don't see why, for the binman schema, we could not constrain the labels? > > > With this schema, would every node be called 'partition@...' or is > > > there flexibility to use other names? > > > > The preference is to use generic names. Do you mean without a > > unit-address or different from "partition"? The need for the input > > side of binman to have dynamic addresses seems like the biggest issue. > > That's allowed in other cases with "partition-N" or "partition-foo" > > IIRC. I don't think we want to allow that for "fixed-partitions" at > > least in the DTB (i.e. the output side of binman). > > OK I suppose this is the problem with starting small. I was hoping to > build up the schema piece by piece but now I am wondering whether > every little detail will get redirected and I'll end up with something > that Binman cannot use. > > So far all I have is that I can add a 'compress' property and a > 'compatible' which describes the contents. I suppose it is a start. I guess defining all you need in one go would be better. At least showing a full and typical example might help. But some items like encoding if you have TF-A or U-Boot in the compatible, I'm far from convinced... Thanks, Miquèl
Re: [PATCH] dt-bindings: mtd: Add a schema for binman
Hi Simon, > > > > > > > > > I was assuming that I should create a top-level compatible = > > > > > > > > > "binman" > > > > > > > > > node, with subnodes like compatible = "binman,bl31-atf", for > > > > > > > > > example. > > > > > > > > > I should use the compatible string to indicate the contents, > > > > > > > > > right? > > > > > > > > > > > > > > > > Yes for subnodes, and we already have some somewhat standard > > > > > > > > ones for > > > > > > > > "u-boot" and "u-boot-env". Though historically, "label" was > > > > > > > > used. > > > > > > > > > > > > > > Binman has common properties for all entries, including "compress" > > > > > > > which sets the compression algorithm. > > > > > > > > > > > > I see no issue with adding that. It seems useful and something > > > > > > missing > > > > > > in the existing partition schemas. > > > > > > > > > > OK I sent a patch with that. > > > > > > > > > > > > > > > > > > So perhaps I should start by defining a new binman,bl31-atf which > > > > > > > has > > > > > > > common properties from an "binman,entry" definition? > > > > > > > > > > > > I don't understand the binman prefix. The contents are ATF (or TF-A > > > > > > now). Who wrote it to the flash image is not relevant. > > > > > > > > > > Are you suggesting just "atf-bl31", or "arm,atf-bl31" ? Or should we > > > > > change it to "tfa-bl31"? > > > > > > > > I don't really understand the relationship with TF-A here. Can't we > > > > just have a kind of fixed-partitions with additional properties like > > > > the compression? > > > > > > Binman needs to know what to put in there, which is the purpose of the > > > compatible string. > > > > But "what" should be put inside the partition is part of the input > > argument, not the output. You said (maybe I got this wrong) that the > > schema would apply to the output of binman. If you want to let user > > know what's inside, maybe it is worth adding a label, but otherwise I > > don't like the idea of a compatible for that, which for me would mean: > > "here is how to handle that specific portion of the flash/here is how > > the flash is organized". > > But I thought that the compatible string was for that purpose? See for > example "brcm,bcm4908-firmware" and "brcm,bcm963xx-imagetag" and > "linksys,ns-firmware". These three examples apparently need specific handling, the partitions contain meta-data that a parser needs to check or something like that. And finally it looks like partition names are set depending on the content that was discovered, so yes, the partition name is likely the good location to tell users/OSes what's inside. > > > > Also, I still don't understand the purpose of this schema. So binman > > > > generates an image, you want to flash this image and you would like the > > > > tool to generate the corresponding (partition) DT snippet automatically. > > > > Do I get this right? I don't get why you would need new compatibles for > > > > that. > > > > > > It is actually the other way around. The schema tells Binman how to > > > build the image (what goes in there and where). Then outputs an > > > updated DT which describes where everything ended up, for use by other > > > tools, e.g. firmware update. It is a closed loop in that sense. See > > > the references for more information. > > > > Maybe I fail to see why you would want these description to be > > introduced here, if they are not useful to the OS. > > Well I was asked to send them to Linux since they apparently don't > belong in dt-schema. These are firmware bindings, as indicated, but I > took them out of the /firmware node since that is for a different > purpose. Rob suggested that partitions was a good place. We have fwupd > using DT to hold the firmware-update information, so I expect it will > move to use these bindings too. I would definitely use fixed partitions as that's what you need then: registering where everything starts and ends. If you have "in-band" meta data you might require a compatible, but I don't think you do, in this case you should probably carry the content through a label (which will become the partition name) and we can discuss additional properties if needed. > > > [1] https://u-boot.readthedocs.io/en/latest/develop/package/index.html > > > [2] > > > https://pretalx.com/media/osfc2019/submissions/Y7EN9V/resources/Binman_-_A_data-controlled_firmware_packer_for_U-B_pFU3n2K.pdf > > > [3] https://www.youtube.com/watch?v=L84ujgUXBOQ > > > > Regards, > Simon Thanks, Miquèl
Re: [PATCH] dt-bindings: mtd: Add a schema for binman
Hi Simon, s...@chromium.org wrote on Mon, 25 Sep 2023 06:33:14 -0600: > Hi Miquel, > > On Mon, 25 Sept 2023 at 01:21, Miquel Raynal > wrote: > > > > Hi Simon, > > > > s...@chromium.org wrote on Fri, 22 Sep 2023 13:51:14 -0600: > > > > > Hi Rob, > > > > > > On Fri, 22 Sept 2023 at 13:43, Rob Herring wrote: > > > > > > > > On Fri, Sep 22, 2023 at 1:12 PM Simon Glass wrote: > > > > > > > > > > Hi Rob, > > > > > > > > > > On Fri, 22 Sept 2023 at 11:46, Rob Herring wrote: > > > > > > > > > > > > On Fri, Sep 22, 2023 at 11:01:18AM -0600, Simon Glass wrote: > > > > > > > Hi Rob, > > > > > > > > > > > > > > On Fri, 22 Sept 2023 at 10:00, Rob Herring > > > > > > > wrote: > > > > > > > > > > > > > > > > On Thu, Sep 21, 2023 at 1:45 PM Simon Glass > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Binman[1] is a tool for creating firmware images. It allows > > > > > > > > > you to > > > > > > > > > combine various binaries and place them in an output file. > > > > > > > > > > > > > > > > > > Binman uses a DT schema to describe an image, in enough > > > > > > > > > detail that > > > > > > > > > it can be automatically built from component parts, > > > > > > > > > disassembled, > > > > > > > > > replaced, listed, etc. > > > > > > > > > > > > > > > > > > Images are typically stored in flash, which is why this > > > > > > > > > binding is > > > > > > > > > targeted at mtd. Previous discussion is at [2] [3]. > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > https://u-boot.readthedocs.io/en/stable/develop/package/binman.html > > > > > > > > > [2] > > > > > > > > > https://lore.kernel.org/u-boot/20230821180220.2724080-3-...@chromium.org/ > > > > > > > > > [3] https://www.spinics.net/lists/devicetree/msg626149.html > > > > > > > > > > > > > > > > You missed: > > > > > > > > > > > > > > > > https://github.com/devicetree-org/dt-schema/pull/110 > > > > > > > > > > > > > > > > where I said: We certainly shouldn't duplicate the existing > > > > > > > > partitions > > > > > > > > bindings. What's missing from them (I assume we're mostly > > > > > > > > talking > > > > > > > > about "fixed-partitions" which has been around forever I think > > > > > > > > (before > > > > > > > > me))? > > > > > > > > > > > > > > > > To repeat, unless there is some reason binman partitions > > > > > > > > conflict with > > > > > > > > fixed-partitions, you need to start there and extend it. From > > > > > > > > what's > > > > > > > > posted here, it neither conflicts nor needs extending. > > > > > > > > > > > > > > I think at this point I am just hopelessly confused. Have you > > > > > > > taken a > > > > > > > look at the binman schema? [1] > > > > > > > > > > > > Why do I need to? That's used for some tool and has nothing to do > > > > > > with a > > > > > > device's DTB. However, I thought somewhere in this discussion you > > > > > > showed > > > > > > it under a flash device node. > > > > > > > > > > Yes, that is the intent (under a flash node). > > > > > > > > > > > Then I care because then it overlaps with > > > > > > what we already have for partitions. If I misunderstood that, then > > > > > > just > > > > > > put your schema with your tool. Only users of the tool should care > > > > > > about > > > > > > the tool's schema. > &
Re: [PATCH] dt-bindings: mtd: Add a schema for binman
Hi Simon, s...@chromium.org wrote on Fri, 22 Sep 2023 13:51:14 -0600: > Hi Rob, > > On Fri, 22 Sept 2023 at 13:43, Rob Herring wrote: > > > > On Fri, Sep 22, 2023 at 1:12 PM Simon Glass wrote: > > > > > > Hi Rob, > > > > > > On Fri, 22 Sept 2023 at 11:46, Rob Herring wrote: > > > > > > > > On Fri, Sep 22, 2023 at 11:01:18AM -0600, Simon Glass wrote: > > > > > Hi Rob, > > > > > > > > > > On Fri, 22 Sept 2023 at 10:00, Rob Herring wrote: > > > > > > > > > > > > On Thu, Sep 21, 2023 at 1:45 PM Simon Glass > > > > > > wrote: > > > > > > > > > > > > > > Binman[1] is a tool for creating firmware images. It allows you to > > > > > > > combine various binaries and place them in an output file. > > > > > > > > > > > > > > Binman uses a DT schema to describe an image, in enough detail > > > > > > > that > > > > > > > it can be automatically built from component parts, disassembled, > > > > > > > replaced, listed, etc. > > > > > > > > > > > > > > Images are typically stored in flash, which is why this binding is > > > > > > > targeted at mtd. Previous discussion is at [2] [3]. > > > > > > > > > > > > > > [1] > > > > > > > https://u-boot.readthedocs.io/en/stable/develop/package/binman.html > > > > > > > [2] > > > > > > > https://lore.kernel.org/u-boot/20230821180220.2724080-3-...@chromium.org/ > > > > > > > [3] https://www.spinics.net/lists/devicetree/msg626149.html > > > > > > > > > > > > You missed: > > > > > > > > > > > > https://github.com/devicetree-org/dt-schema/pull/110 > > > > > > > > > > > > where I said: We certainly shouldn't duplicate the existing > > > > > > partitions > > > > > > bindings. What's missing from them (I assume we're mostly talking > > > > > > about "fixed-partitions" which has been around forever I think > > > > > > (before > > > > > > me))? > > > > > > > > > > > > To repeat, unless there is some reason binman partitions conflict > > > > > > with > > > > > > fixed-partitions, you need to start there and extend it. From what's > > > > > > posted here, it neither conflicts nor needs extending. > > > > > > > > > > I think at this point I am just hopelessly confused. Have you taken a > > > > > look at the binman schema? [1] > > > > > > > > Why do I need to? That's used for some tool and has nothing to do with a > > > > device's DTB. However, I thought somewhere in this discussion you showed > > > > it under a flash device node. > > > > > > Yes, that is the intent (under a flash node). > > > > > > > Then I care because then it overlaps with > > > > what we already have for partitions. If I misunderstood that, then just > > > > put your schema with your tool. Only users of the tool should care about > > > > the tool's schema. > > > > > > OK. I believe that binman will fit into both camps, since its input is > > > not necessarily fully formed. E.g. if you don't specify the offset of > > > an entry, then it will be packed automatically. But the output is > > > fully formed, in that Binman now knows the offset so can write it to > > > the DT. > > > > I suppose it could take its own format as input and then write out > > something different for the "on the device" format (i.e. > > fixed-partitions). At least for the dynamic offsets, we may need > > something allowed for binman input, but not allowed on device. In > > general, there is support for partitions without addresses/offsets, > > but only for partitions that have some other way to figure that out > > (on disk partition info). > > > > There's also the image filename which doesn't really belong in the on > > device partitions. So maybe the input and output schemas should be > > separate. > > OK, I'll focus on the output schema for now. I suspect this will be a > grey area though. > > As an example, if you replace a binary in the firmware, Binman can > repack the firmware to make room, respecting the alignment and size > constraints. So these need to be in the output schema somehow. > > > > > > > > I saw this file, which seems to extend a partition. > > > > > > > > > > Documentation/devicetree/bindings/mtd/partitions/brcm,bcm4908-partitions.yaml > > > > > > > > > > > > > IIRC, that's a different type where partition locations are stored in > > > > the flash, so we don't need location and size in DT. > > > > > > OK. > > > > > > > > > > > > > > > > > I was assuming that I should create a top-level compatible = "binman" > > > > > node, with subnodes like compatible = "binman,bl31-atf", for example. > > > > > I should use the compatible string to indicate the contents, right? > > > > > > > > Yes for subnodes, and we already have some somewhat standard ones for > > > > "u-boot" and "u-boot-env". Though historically, "label" was used. > > > > > > Binman has common properties for all entries, including "compress" > > > which sets the compression algorithm. > > > > I see no issue with adding that. It seems useful and something missing > > in the existing partition schemas. > > OK I sent a
Re: [PATCH v2 01/17] dm: usb: udc: Factor out plain udevice handler functions
Hi Marek, I'm answering here as there is no cover letter. Just to let you know I'm still concerned by the series and want to test it but did not had the time to do so recently. Hopefully next week. Sorry for the delay. Miquèl ma...@denx.de wrote on Fri, 1 Sep 2023 11:49:47 +0200: > Pull the functionality of UDC uclass that operates on plain udevice > and does not use this dev_array array into separate functions and > expose those functions, so that as much code as possible can be > switched over to these functions and the dev_array can be dropped. > > Reviewed-by: Mattijs Korpershoek > Signed-off-by: Marek Vasut > --- > Cc: Angus Ainslie > Cc: Dmitrii Merkurev > Cc: Eddie Cai > Cc: Kever Yang > Cc: Lukasz Majewski > Cc: Miquel Raynal > Cc: Mattijs Korpershoek > Cc: Nishanth Menon > Cc: Patrice Chotard > Cc: Patrick Delaunay > Cc: Philipp Tomsich > Cc: Simon Glass > Cc: Stefan Roese > Cc: ker...@puri.sm > --- > V2: Add RB from Mattijs > --- > drivers/usb/gadget/udc/Makefile | 2 +- > drivers/usb/gadget/udc/udc-uclass.c | 57 + > include/linux/usb/gadget.h | 17 + > 3 files changed, 68 insertions(+), 8 deletions(-)
Re: [PATCH 01/17] dm: usb: udc: Factor out plain udevice handler functions
Hi Marek, ma...@denx.de wrote on Sat, 19 Aug 2023 16:23:51 +0200: > Pull the functionality of UDC uclass that operates on plain udevice > and does not use this dev_array array into separate functions and > expose those functions, so that as much code as possible can be > switched over to these functions and the dev_array can be dropped. > > Signed-off-by: Marek Vasut > --- > Cc: Angus Ainslie > Cc: Dmitrii Merkurev > Cc: Eddie Cai > Cc: Kever Yang > Cc: Lukasz Majewski > Cc: Miquel Raynal > Cc: Mattijs Korpershoek > Cc: Nishanth Menon > Cc: Patrice Chotard > Cc: Patrick Delaunay > Cc: Philipp Tomsich > Cc: Simon Glass > Cc: Stefan Roese > Cc: ker...@puri.sm > --- Would you mind adding a cover letter to your series? It would be easier for us to (a) know what is the final purpose of the series and (b) have a location where to send global feedback. Indeed, I did not yet investigate, in particular because every time fastboot is broken I cannot easily test new images without going through a whole recovery step with another image, but it seems like the network over USB is no longer working (not 100% sure about that one, sometimes the network is capricious) and anyway just calling unbind crashes: => dm tree [...] misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 usb 0 [ + ] ti-musb-peripheral| | |-- usb@47401000 ethernet 1 [ + ] usb_ether | | | `-- usb_ether bootdev 3 [ ] eth_bootdev | | | `-- usb_ether.bootdev usb 0 [ ] ti-musb-host | | `-- usb@47401800 => unbind ethernet 1 data abort pc : [<9ff9a0e6>] lr : [<9ff9a0e7>] reloc pc : [<808350e6>]lr : [<808350e7>] sp : 9df2f998 ip : 001c fp : 0003 r10: 9df4d800 r9 : 9df44ea0 r8 : 9ffe4a0c r7 : 9ff82ad9 r6 : 9df4d800 r5 : 8400 r4 : 9df4cf48 r3 : 9ff9a0e1 r2 : r1 : r0 : Flags: NzCv IRQs off FIQs on Mode SVC_32 (T) Code: 9ffd b508 f7ec fe63 (f8d0) 00b0 Resetting CPU ... Thanks, Miquèl
[PATCH v2 2/2] usb: udc: Try to clarify an error message
At some point when trying to use USB gadgets, two situations may arise
and lead to a failure. Either the UDC (USB Device Controller) is not
available at all (not described or not probed) or the UDC is already in
use. For instance, as the USB Ethernet gadget remains bound to the UDC,
the use of any other USB gadget (fastboot, dfu, etc) *after* will always
fail with the "couldn't find an available UDC" error.
Let's give a more helpful message by making a difference between the two
cases. Let's also hint people who would get this error and grep it into
the sources a better explanation of what's wrong with their workflow.
Signed-off-by: Miquel Raynal
---
While doing this I really wanted to add "much more" error comments but I
faced another reality: often the messages are there but use
pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
I consider this unnecessary, as decreasing the loglevel will make these
messages appear. I would have expected errors to be displayed, but I
understand it makes the binaries even bigger.
Changes in v2:
* s/UDC/UDCs/. I kept the sentence as it was as the suggested form did
not sound well at all when there is only one UDC.
---
drivers/usb/gadget/udc/udc-core.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index 7f73926cb3e..8405b03462e 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -323,6 +323,7 @@ err1:
int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
{
struct usb_udc *udc = NULL;
+ unsigned intudc_count = 0;
int ret;
if (!driver || !driver->bind || !driver->setup)
@@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver
*driver)
mutex_lock(_lock);
list_for_each_entry(udc, _list, list) {
+ udc_count++;
+
/* For now we take the first one */
if (!udc->driver)
goto found;
}
- printf("couldn't find an available UDC\n");
+ if (!udc_count)
+ printf("No UDC available in the system\n");
+ else
+ /* When this happens, users should 'unbind '
+* using the output of 'dm tree' and looking at the line right
+* after the USB peripheral/device controller.
+*/
+ printf("All UDCs in use (%d available), use the unbind
command\n",
+ udc_count);
mutex_unlock(_lock);
return -ENODEV;
found:
--
2.34.1
[PATCH v2 1/2] cmd: bind: Try to improve the (un)bind help
While it may sound totally obvious for the regular U-Boot developer to get the parameters of the bind/unbind commands from the output of 'dm tree', it did not felt straightforward to me until I was explicitly told to look there. And even when I knew the command, I did not make a direct link between the arguments of this command and the columns returned by 'dm tree'. Several of us lost a lot of time because of that, I would like to kindly help other users by slightly improving this textual line. Unfortunately, because of how this string is used (like within the 'help' command) I cannot detail much more, but at least the pointer is there. Signed-off-by: Miquel Raynal --- Changes in v2: * Moved the details in the long text section as suggested by Heinrich. * Rephrased the help text as well, not fully following Heinrich suggestion, but taking inspiration from it. --- cmd/bind.c | 4 1 file changed, 4 insertions(+) diff --git a/cmd/bind.c b/cmd/bind.c index 4d1b7885e60..be0d4d2a711 100644 --- a/cmd/bind.c +++ b/cmd/bind.c @@ -246,6 +246,8 @@ U_BOOT_CMD( "Bind a device to a driver", " \n" "bind \n" + "Use 'dm tree' to list all devices registered in the driver model,\n" + "their path, class, index and current driver.\n" ); U_BOOT_CMD( @@ -254,4 +256,6 @@ U_BOOT_CMD( "\n" "unbind \n" "unbind \n" + "Use 'dm tree' to list all devices registered in the driver model,\n" + "their path, class, index and current driver.\n" ); -- 2.34.1
Re: [PATCH 2/2] usb: udc: Try to clarify an error message
Hi Heinrich,
heinrich.schucha...@canonical.com wrote on Mon, 7 Aug 2023 11:29:08
+0200:
> On 04.08.23 21:14, Miquel Raynal wrote:
> > At some point when trying to use USB gadgets, two situations may arise
> > and lead to a failure. Either the UDC (USB Device Controller) is not
> > available at all (not described or not probed) or the UDC is already in
> > use. For instance, as the USB Ethernet gadget remains bound to the UDC,
> > the use of any other USB gadget (fastboot, dfu, etc) *after* will always
> > fail with the "couldn't find an available UDC" error.
>
> Are the UDCs real devices or only logical ones?
In this context there is a real device behind. But UDC is also the name
for kind of an interface for gadget drivers. In theory, on USB device
controller could expose several different functions.
> If they are only logical ones, can we generate new ones on the fly?
In general, it's not impossible, but it's not supported at all in U-Boot
(and for a good reason: it would be way too complex to handle) and I
think even in Linux, it's possible, but possibly only using
configfs (not 100% sure on that one).
> > Let's give a more helpful message by making a difference between the two
> > cases. Let's also hint people who would get this error and grep it into
> > the sources a better explanation of what's wrong with their workflow.
> >
> > Signed-off-by: Miquel Raynal
> > ---
> >
> > While doing this I really wanted to add "much more" error comments but I
> > faced another reality: often the messages are there but use
> > pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
> > I consider this unnecessary, as decreasing the loglevel will make these
> > messages appear. I would have expected errors to be displayed, but I
> > understand it makes the binaries even bigger.
> > ---
> > drivers/usb/gadget/udc/udc-core.c | 13 -
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/udc/udc-core.c
> > b/drivers/usb/gadget/udc/udc-core.c
> > index 7f73926cb3e..30f6d73f36e 100644
> > --- a/drivers/usb/gadget/udc/udc-core.c
> > +++ b/drivers/usb/gadget/udc/udc-core.c
> > @@ -323,6 +323,7 @@ err1:
> > int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
> > {
> > struct usb_udc *udc = NULL;
> > + unsigned intudc_count = 0;
> > int ret;
> > > if (!driver || !driver->bind || !driver->setup)
> > @@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver
> > *driver)
> > > mutex_lock(_lock);
> > list_for_each_entry(udc, _list, list) {
> > + udc_count++;
> > +
> > /* For now we take the first one */
> > if (!udc->driver)
> > goto found;
> > }
> > > - printf("couldn't find an available UDC\n");
> > + if (!udc_count)
> > + printf("No UDC available in the system\n");
> > + else
> > + /* When this happens, users should 'unbind '
> > +* using the output of 'dm tree' and looking at the line right
> > +* after the USB peripheral/device controller.
> > +*/
> > + printf("All UDC in use (%d available), use the unbind
> > command\n",
>
> 'UDC' should be plural, e.g.
>
> %s/All UDC in use (%d available)/All %d UDCs bound/
Does not work well with one: "All 1 UDCs bound" does not look great.
I would prefer keeping the original sentence, s/UDC/UDCs/:
"All UDCs in use (%d available)".
Thanks,
Miquèl
Re: [PATCH 1/2] cmd: bind: Try to improve the (un)bind help
Hi Heinrich, > > U_BOOT_CMD( > > bind, 4, 0, do_bind_unbind, > > - "Bind a device to a driver", > > + "Bind a device to a driver, see 'dm tree' for the parameters\n", > > How about > > "Use 'dm tree' to list classes, drivers, and devices." > > as last line of the help long text. Where is this supposed to be? Is there another macro to use than U_BOOT_CMD()? Thanks, Miquèl
[PATCH v2] doc: ti: Explain how the various gadget devices can be used
Describe the current situation wrt the handling of USB devices on AM33xx
based boards, taking the example of a common board (the Beagle Bone
Black) and explaining how the different USB gadgets can be used.
Signed-off-by: Miquel Raynal
---
I've tried to be as transparent and honnest as I could with my limited
knowledge of the situation and the USB world, based on our latest
dicussions. I am fully open to suggestions, changes, rephrasing...
Changes in v2:
* Mostly style changes proposed by Heinrich.
---
doc/board/ti/am335x_evm.rst | 62 +
1 file changed, 62 insertions(+)
diff --git a/doc/board/ti/am335x_evm.rst b/doc/board/ti/am335x_evm.rst
index ee4faec37c2..3332d51b365 100644
--- a/doc/board/ti/am335x_evm.rst
+++ b/doc/board/ti/am335x_evm.rst
@@ -201,3 +201,65 @@ booting and mtdparts have been configured correctly for
the board:
U-Boot # spl export fdt ${loadaddr} - ${fdtaddr}
U-Boot # nand erase.part u-boot-spl-os
U-Boot # nand write ${fdtaddr} u-boot-spl-os
+
+USB device
+--
+
+The platform code for am33xx based designs is legacy in the sense that
+it is not fully compliant with the driver model in its management of the
+various resources. This is particularly true for the USB Ethernet gadget
+which will automatically be bound to the first USB Device Controller
+(UDC). This make the USB Ethernet gadget work out of the box on common
+boards like the Beagle Bone Blacks and by default will prevents other
+gadgets to be used.
+
+The output of the 'dm tree' command shows which driver is bound to which
+device, so the user can easily configure their platform differently from
+the command line:
+
+.. code-block:: text
+
+ => dm tree
+Class Index Probed DriverName
+ ---
+ [...]
+ misc 0 [ + ] ti-musb-wrapper | |-- usb@4740
+ usb 0 [ + ] ti-musb-peripheral| | |-- usb@47401000
+ ethernet 1 [ + ] usb_ether | | | `-- usb_ether
+ bootdev 3 [ ] eth_bootdev | | | `--
usb_ether.bootdev
+ usb 0 [ ] ti-musb-host | | `-- usb@47401800
+
+Typically here any network command performed using the usb_ether
+interface would work, while using other gadgets would fail:
+
+.. code-block:: text
+
+ => fastboot usb 0
+ All UDC in use (1 available), use the unbind command
+ g_dnl_register: failed!, error: -19
+ exit not allowed from main input shell.
+
+As hinted by the primary error message, the only controller available
+(usb@47401000) is currently bound to the usb_ether driver, which makes
+it impossible for the fastboot command to bind with this device (at
+least from a bootloader point of view). The solution here would be to
+use the unbind command specifying the class and index parameters (as
+shown above in the 'dm tree' output) to target the driver to unbind:
+
+.. code-block:: text
+
+ => unbind ethernet 1
+
+The output of the 'dm tree' command now shows the availability of the
+first USB device controller, the fastboot gadget will now be able to
+bind with it:
+
+.. code-block:: text
+
+ => dm tree
+Class Index Probed DriverName
+ ---
+ [...]
+ misc 0 [ + ] ti-musb-wrapper | |-- usb@4740
+ usb 0 [ ] ti-musb-peripheral| | |-- usb@47401000
+ usb 0 [ ] ti-musb-host | | `-- usb@47401800
--
2.34.1
Re: [PATCH] doc: ti: Explain how the various gadget devices can be used
Hi Heinrich,
xypron.g...@gmx.de wrote on Sun, 6 Aug 2023 21:29:54 +0200:
> On 8/4/23 21:37, Miquel Raynal wrote:
> > Describe the current situation wrt the handling of USB devices on AM33xx
> > based boards, taking the example of a common board (the Beagle Bone
> > Black) and explaining how the different USB gadgets can be used.
> >
> > Signed-off-by: Miquel Raynal
> >
> > ---
> >
> > I've tried to be as transparent and honnest as I could with my limited
> > knowledge of the situation and the USB world, based on our latest
> > dicussions. I am fully open to suggestions, changes, rephrasing...
> > ---
> > doc/board/ti/am335x_evm.rst | 61 +
> > 1 file changed, 61 insertions(+)
> >
> > diff --git a/doc/board/ti/am335x_evm.rst b/doc/board/ti/am335x_evm.rst
> > index ee4faec37c2..0587b506ee2 100644
> > --- a/doc/board/ti/am335x_evm.rst
> > +++ b/doc/board/ti/am335x_evm.rst
> > @@ -201,3 +201,64 @@ booting and mtdparts have been configured correctly
> for the board:
> > U-Boot # spl export fdt ${loadaddr} - ${fdtaddr}
> > U-Boot # nand erase.part u-boot-spl-os
> > U-Boot # nand write ${fdtaddr} u-boot-spl-os
> > +
> > +USB device
> > +--
> > +
> > +The platform code for am33xx based designs is legacy in the sense that
> > +it is not fully compliant with the device model in its management of th
> e
>
> Thanks for extending the documentation. Please, have a look at the
> suggestions below.
They all sound sensible to me, I will take them and send a v2.
Thanks,
Miquèl
Re: [PATCH v5 2/3] usb: gadget: ether: Move probe function above driver structure
Hi Marek, ma...@denx.de wrote on Fri, 4 Aug 2023 17:41:10 +0200: > Move the driver probe function above the driver structure, so it > can be placed alongside other related functions, like upcoming > remove function. No functional change. > > Signed-off-by: Marek Vasut > --- > Cc: Kevin Hilman > Cc: Lukasz Majewski > Cc: Miquel Raynal > Cc: Simon Glass > --- Tested-by: Miquel Raynal Thanks, Miquèl
Re: [PATCH v5 2/3] usb: gadget: ether: Move probe function above driver structure
Hi Marek, ma...@denx.de wrote on Fri, 4 Aug 2023 17:41:10 +0200: > Move the driver probe function above the driver structure, so it > can be placed alongside other related functions, like upcoming > remove function. No functional change. > > Signed-off-by: Marek Vasut > --- > Cc: Kevin Hilman > Cc: Lukasz Majewski > Cc: Miquel Raynal > Cc: Simon Glass > --- Tested-by: Miquel Raynal Thanks, Miquèl
Re: [PATCH v5 1/3] usb: gadget: ether: Inline functions used once
Hi Marek, ma...@denx.de wrote on Fri, 4 Aug 2023 17:41:09 +0200: > These functions here are only ever called once since drop of non-DM > networking code. Inline them. No functional change. > > Signed-off-by: Marek Vasut > --- > Cc: Kevin Hilman > Cc: Lukasz Majewski > Cc: Miquel Raynal > Cc: Simon Glass > --- Tested-by: Miquel Raynal Thanks, Miquèl
Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter
Hi Tom, tr...@konsulko.com wrote on Fri, 4 Aug 2023 14:51:07 -0400: > On Fri, Aug 04, 2023 at 08:01:56PM +0200, Miquel Raynal wrote: > > Hi Tom, > > > > tr...@konsulko.com wrote on Fri, 4 Aug 2023 13:20:29 -0400: > > > > > On Fri, Aug 04, 2023 at 07:01:46PM +0200, Miquel Raynal wrote: > > > > Hi Tom, > > > > > > > > > > > >>> Well, what's needed / is it possible to get to the point > > > > > > > >>> where we don't > > > > > > > >>> _need_ to call bind/unbind for each of these cases? Is there > > > > > > > >>> something > > > > > > > >>> we're supposed to be setting in the DT that we aren't? > > > > > > > >> > > > > > > > >> You do need to unbind the ethernet before using fastboot, > > > > > > > >> always, with the 'unbind ethernet 0', you dont need the > > > > > > > >> peripheral unbind/rebind part, so it should behave like > > > > > > > >> before. > > > > > > > > > > > > > > > > And for my own understanding, why don't we need to bind a > > > > > > > > fastboot > > > > > > > > gadget? > > > > > > > > > > > > > > The fastboot command does that internally . > > > > > > > > > > > > This is not visible with dm tree and I did not find how to bind the > > > > > > fastboot gadget manually. > > > > > > > > > > > > This makes the CLI clearly uneven and the internal code badly > > > > > > different > > > > > > between gadgets/commands. Why can't we have them both > > > > > > autoloaded/unloaded like before? I think I missed something which > > > > > > explains the rationale behind this series. > > > > > > > > > > They aren't both auto-loaded currently. We have a legacy call, > > > > > usb_ether_init(), in a few cases, so that gadget mode ethernet starts. > > > > > But this leads to the ref counting problems you encountered and > > > > > re-posted the rejected series for. > > > > > > > > Ok, thanks for the additional details. > > > > > > > > I don't understand why fastboot autoloads the correct gadget driver if > > > > there is none bound, while all network commands just fail to do that if > > > > we don't make the usb_ether_init() call manually. > > > > > > Because "fastboot" or "dfu" are both being told (as part of their call) > > > "find usb gadget controller number X". That's doing the bind. Calling > > > usb_ether_init just takes the first controller and that's that (and so > > > could be / is wrong depending on the platform). > > > > This makes total sense, thanks for pointing it out. > > > > > > I also don't understand why I need to unbind the ethernet gadget but I > > > > cannot bind the fastboot gadget. > > > > > > You can't bind fastboot while ethernet is still configured. > > > > I guess "before this series", the ethernet would not be kept > > configured, because I could use both fastboot and ethernet without > > caring about which driver had to be bound. And that's maybe what lead to > > the bug reports also. So you want to get rid of this. Do I get the > > situation right now? > > We're getting rid of this path because it leads to failures, yes. > > > > It's in > > > use. And we aren't a full blown operating system with the logic to have > > > multiple end points and devices configured and exposed. I mean heck, we > > > don't keep the gadget interface up between network calls so on the host > > > side you need to re-configure the interface (or have something that's > > > bringing it up there again each time). Which is why DFU or fastboot or > > > other "treat USB like USB" options tend to be more popular than "treat > > > USB like ethernet" work flows. > > > > Yes of course. > > > > > > My underlying question is: can we have a single approach for all > > > > drivers, or is it too complex today? Could it be possible, when we > > > > perform these autoloads, to look up the registere
[PATCH] doc: ti: Explain how the various gadget devices can be used
Describe the current situation wrt the handling of USB devices on AM33xx
based boards, taking the example of a common board (the Beagle Bone
Black) and explaining how the different USB gadgets can be used.
Signed-off-by: Miquel Raynal
---
I've tried to be as transparent and honnest as I could with my limited
knowledge of the situation and the USB world, based on our latest
dicussions. I am fully open to suggestions, changes, rephrasing...
---
doc/board/ti/am335x_evm.rst | 61 +
1 file changed, 61 insertions(+)
diff --git a/doc/board/ti/am335x_evm.rst b/doc/board/ti/am335x_evm.rst
index ee4faec37c2..0587b506ee2 100644
--- a/doc/board/ti/am335x_evm.rst
+++ b/doc/board/ti/am335x_evm.rst
@@ -201,3 +201,64 @@ booting and mtdparts have been configured correctly for
the board:
U-Boot # spl export fdt ${loadaddr} - ${fdtaddr}
U-Boot # nand erase.part u-boot-spl-os
U-Boot # nand write ${fdtaddr} u-boot-spl-os
+
+USB device
+--
+
+The platform code for am33xx based designs is legacy in the sense that
+it is not fully compliant with the device model in its management of the
+various resources. This is particularly true for the USB Ethernet gadget
+which will automatically be bound to the first USB Device Controller
+(UDC). This make the USB Ethernet gadget work out of the box on common
+boards like the Beagle Bone Blacks and by default will prevents other
+gadgets to be used.
+
+The output of the 'dm tree' command should be rather explicit in that
+sense, showing exactly what driver is bound to what device, so the user
+can easily configure its platform differently from the command line:
+
+.. code-block:: text
+
+ => dm tree
+Class Index Probed DriverName
+ ---
+ [...]
+ misc 0 [ + ] ti-musb-wrapper | |-- usb@4740
+ usb 0 [ + ] ti-musb-peripheral| | |-- usb@47401000
+ ethernet 1 [ + ] usb_ether | | | `-- usb_ether
+ bootdev 3 [ ] eth_bootdev | | | `--
usb_ether.bootdev
+ usb 0 [ ] ti-musb-host | | `-- usb@47401800
+
+Typically here any network command performed using the usb_ether
+interface would work, while using other gadgets would fail:
+
+.. code-block:: text
+
+ => fastboot usb 0
+ All UDC in use (1 available), use the unbind command
+ g_dnl_register: failed!, error: -19
+ exit not allowed from main input shell.
+
+As hinted by the primary error message, the only controller available
+(usb@47401000) is currently bound to the usb_ether driver, which makes
+it impossible to the fastboot command to bind with this device (at least
+from a Bootloader point of view). The solution here would be to use the
+unbind command using the class and index parameters (as shown above in
+the 'dm tree' output) to target the driver to unbind:
+
+.. code-block:: text
+
+ => unbind ethernet 1
+
+Checking the output of the 'dm tree' command now shows full availability
+for the fastboot gadget to bind with UDC 0:
+
+.. code-block:: text
+
+ => dm tree
+Class Index Probed DriverName
+ ---
+ [...]
+ misc 0 [ + ] ti-musb-wrapper | |-- usb@4740
+ usb 0 [ ] ti-musb-peripheral| | |-- usb@47401000
+ usb 0 [ ] ti-musb-host | | `-- usb@47401800
--
2.34.1
[PATCH 2/2] usb: udc: Try to clarify an error message
At some point when trying to use USB gadgets, two situations may arise
and lead to a failure. Either the UDC (USB Device Controller) is not
available at all (not described or not probed) or the UDC is already in
use. For instance, as the USB Ethernet gadget remains bound to the UDC,
the use of any other USB gadget (fastboot, dfu, etc) *after* will always
fail with the "couldn't find an available UDC" error.
Let's give a more helpful message by making a difference between the two
cases. Let's also hint people who would get this error and grep it into
the sources a better explanation of what's wrong with their workflow.
Signed-off-by: Miquel Raynal
---
While doing this I really wanted to add "much more" error comments but I
faced another reality: often the messages are there but use
pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
I consider this unnecessary, as decreasing the loglevel will make these
messages appear. I would have expected errors to be displayed, but I
understand it makes the binaries even bigger.
---
drivers/usb/gadget/udc/udc-core.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index 7f73926cb3e..30f6d73f36e 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -323,6 +323,7 @@ err1:
int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
{
struct usb_udc *udc = NULL;
+ unsigned intudc_count = 0;
int ret;
if (!driver || !driver->bind || !driver->setup)
@@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver
*driver)
mutex_lock(_lock);
list_for_each_entry(udc, _list, list) {
+ udc_count++;
+
/* For now we take the first one */
if (!udc->driver)
goto found;
}
- printf("couldn't find an available UDC\n");
+ if (!udc_count)
+ printf("No UDC available in the system\n");
+ else
+ /* When this happens, users should 'unbind '
+* using the output of 'dm tree' and looking at the line right
+* after the USB peripheral/device controller.
+*/
+ printf("All UDC in use (%d available), use the unbind
command\n",
+ udc_count);
mutex_unlock(_lock);
return -ENODEV;
found:
--
2.34.1
[PATCH 1/2] cmd: bind: Try to improve the (un)bind help
While it may sound totally obvious for the regular U-Boot developer to get the parameters of the bind/unbind commands from the output of 'dm tree', it did not felt straightforward to me until I was explicitly told to look there. And even when I knew the command, I did not make a direct link between the arguments of this command and the columns returned by 'dm tree'. Several of us lost a lot of time because of that, I would like to kindly help other users by slightly improving this textual line. Unfortunately, because of how this string is used (like within the 'help' command) I cannot detail much more, but at least the pointer is there. Signed-off-by: Miquel Raynal --- cmd/bind.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/bind.c b/cmd/bind.c index 4d1b7885e60..8d29a42899e 100644 --- a/cmd/bind.c +++ b/cmd/bind.c @@ -243,14 +243,14 @@ static int do_bind_unbind(struct cmd_tbl *cmdtp, int flag, int argc, U_BOOT_CMD( bind, 4, 0, do_bind_unbind, - "Bind a device to a driver", + "Bind a device to a driver, see 'dm tree' for the parameters\n", " \n" "bind \n" ); U_BOOT_CMD( unbind, 4, 0, do_bind_unbind, - "Unbind a device from a driver", + "Unbind a device from a driver, see 'dm tree' for the parameters\n", "\n" "unbind \n" "unbind \n" -- 2.34.1
Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter
Hi Tom, tr...@konsulko.com wrote on Fri, 4 Aug 2023 13:20:29 -0400: > On Fri, Aug 04, 2023 at 07:01:46PM +0200, Miquel Raynal wrote: > > Hi Tom, > > > > > > > >>> Well, what's needed / is it possible to get to the point where we > > > > > >>> don't > > > > > >>> _need_ to call bind/unbind for each of these cases? Is there > > > > > >>> something > > > > > >>> we're supposed to be setting in the DT that we aren't? > > > > > >> > > > > > >> You do need to unbind the ethernet before using fastboot, always, > > > > > >> with the 'unbind ethernet 0', you dont need the peripheral > > > > > >> unbind/rebind part, so it should behave like before. > > > > > > > > > > > > And for my own understanding, why don't we need to bind a fastboot > > > > > > gadget? > > > > > > > > > > The fastboot command does that internally . > > > > > > > > This is not visible with dm tree and I did not find how to bind the > > > > fastboot gadget manually. > > > > > > > > This makes the CLI clearly uneven and the internal code badly different > > > > between gadgets/commands. Why can't we have them both > > > > autoloaded/unloaded like before? I think I missed something which > > > > explains the rationale behind this series. > > > > > > They aren't both auto-loaded currently. We have a legacy call, > > > usb_ether_init(), in a few cases, so that gadget mode ethernet starts. > > > But this leads to the ref counting problems you encountered and > > > re-posted the rejected series for. > > > > Ok, thanks for the additional details. > > > > I don't understand why fastboot autoloads the correct gadget driver if > > there is none bound, while all network commands just fail to do that if > > we don't make the usb_ether_init() call manually. > > Because "fastboot" or "dfu" are both being told (as part of their call) > "find usb gadget controller number X". That's doing the bind. Calling > usb_ether_init just takes the first controller and that's that (and so > could be / is wrong depending on the platform). This makes total sense, thanks for pointing it out. > > I also don't understand why I need to unbind the ethernet gadget but I > > cannot bind the fastboot gadget. > > You can't bind fastboot while ethernet is still configured. I guess "before this series", the ethernet would not be kept configured, because I could use both fastboot and ethernet without caring about which driver had to be bound. And that's maybe what lead to the bug reports also. So you want to get rid of this. Do I get the situation right now? > It's in > use. And we aren't a full blown operating system with the logic to have > multiple end points and devices configured and exposed. I mean heck, we > don't keep the gadget interface up between network calls so on the host > side you need to re-configure the interface (or have something that's > bringing it up there again each time). Which is why DFU or fastboot or > other "treat USB like USB" options tend to be more popular than "treat > USB like ethernet" work flows. Yes of course. > > My underlying question is: can we have a single approach for all > > drivers, or is it too complex today? Could it be possible, when we > > perform these autoloads, to look up the registered gadget and > > potentially unbind the one already in place before? > > I would invite you to look in to this, yes. Sounds reasonably complex now, with the reasoning you made above. > No one objects to enhancing > the code, but the "functionality" you see on am33xx is as you've also > seen very broken in other ways, which is why it's not used virtually > anywhere else and instead the "bind" command is. For example, if you > wanted to do this workflow on the new beagle devices, that's DWC3 and > where the "bind" command came from :) Again to be very clear, while I felt misunderstood at the beginning and did not accept Marek's series because it was replacing a data abort with a non-functional setup, I now get a better understanding of the problem (I believe) and, as said before, I am always in favor of writing better code, easier to maintain, clearer to review. I am in favor of such change. I just want the user life to be eased when this happens if we break the CLI. So if you think the right approach is to get rid of the usb_ether_init() call, alright, let's drop it off. But we should not let the users in the dark by providing proper doc or error messages which should compensate the extra step now required. Thanks, Miquèl
Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek, ma...@denx.de wrote on Fri, 4 Aug 2023 19:31:50 +0200: > On 8/4/23 19:24, Miquel Raynal wrote: > > Hi, > > >>>>>>> exit not allowed from main input shell. > >>>>>>> => unbind /ocp/usb@4740/usb@47401000 usb_ether > >>>>>> > >>>>>> Does > >>>>>>>>>> => unbind ethernet 0 > >>>>>> > >>>>>> work ? > >>>>>> > >>>>>> If so, 1/4 in this series can be skipped altogether. > >>>>>> > >>>>>> You likely won't even need the rebinding of ti-musb-peripheral > >>>>>> anymore. > >> > >> Did you test this yet ? > > > > Unfortunately it does not work. Indeed it would be much simpler than > > using the node path. Any idea why? > > Since you provided literally zero information, no. > > Console log would be a good starting point. Here it is, the unbind command itself does not complain has it seems to catch the regular Ethernet controller (there is one in the SoC, but it is not wired on the board). So the first time it does nothing, but the second time it works as the USB gadget get dropped! And after the second call, fastboot works without the bind call. => dm tree misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 usb 0 [ + ] ti-musb-peripheral| | |-- usb@47401000 ethernet 1 [ + ] usb_ether | | | `-- usb_ether bootdev 3 [ ] eth_bootdev | | | `-- usb_ether.bootdev usb 0 [ ] ti-musb-host | | `-- usb@47401800 ethernet 0 [ + ] eth_cpsw | |-- ethernet@4a10 bootdev 2 [ ] eth_bootdev | | `-- ethernet@4a10.bootdev => unbind ethernet 0 => dm tree misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 usb 0 [ + ] ti-musb-peripheral| | |-- usb@47401000 ethernet 0 [ + ] usb_ether | | | `-- usb_ether bootdev 2 [ ] eth_bootdev | | | `-- usb_ether.bootdev usb 0 [ ] ti-musb-host | | `-- usb@47401800 => unbind ethernet 0 => dm tree misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 usb 0 [ ] ti-musb-peripheral| | |-- usb@47401000 usb 0 [ ] ti-musb-host | | `-- usb@47401800 So actually the unbind works, but was not targeting the right controller, because it's listed as the second Ethernet controller on this board. Hence this actually works: => unbind ethernet 1 => fastboot usb 0 \o/ Thanks, Miquèl
Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek, ma...@denx.de wrote on Fri, 4 Aug 2023 17:40:07 +0200: > On 8/4/23 17:12, Miquel Raynal wrote: > > Hi, > > > > ma...@denx.de wrote on Fri, 4 Aug 2023 17:05:01 +0200: > > > >> On 8/4/23 17:01, Tom Rini wrote: > >>> On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote: > >>>> On 8/4/23 09:00, Miquel Raynal wrote: > >>>>> Hi Marek, > >>>>> > >>>>> ma...@denx.de wrote on Wed, 2 Aug 2023 14:46:54 +0200: > >>>>> >>>>>> Extend the driver core to perform lookup by both OF node and > >>>>> driver > >>>>>> bound to the node. Use this to look up specific device instances to > >>>>>> unbind from nodes in the unbind command. One example where this is > >>>>>> needed is USB peripheral controller, which may have multiple gadget > >>>>>> drivers bound to it. The unbind command has to select that specific > >>>>>> gadget driver instance to unbind from the controller, not unbind the > >>>>>> controller driver itself from the controller. > >>>>>> > >>>>>> USB ethernet gadget usage looks as follows with this change. Notice > >>>>>> the extra 'usb_ether' addition in the 'unbind' command at the end. > >>>>>> " > >>>>>> bind /soc/usb-otg@4900 usb_ether > >>>>>> setenv ethact usb_ether > >>>>>> setenv loadaddr 0xc200 > >>>>>> setenv ipaddr 10.0.0.2 > >>>>>> setenv serverip 10.0.0.1 > >>>>>> setenv netmask 255.255.255.0 > >>>>>> tftpboot 0xc200 10.0.0.1:test.file > >>>>>> unbind /soc/usb-otg@4900 usb_ether > >>>>> > >>>>> This extra parameter does not seem to work on the BBBW. I cannot select > >>>>> the "usb_ether" driver like you do. > >>>>> > >>>>> Good news though, I am now able to use fastboot, but it is not > >>>>> straightforward: > >>>>> > >>>>> Here is my sequence right after the boot (reducing the dm tree output > >>>>> to the usb nodes for clarity): > >>>>> >>>>> => dm tree > >>>>> misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 > >>>>> usb 0 [ + ] ti-musb-peripheral| | |-- > >>>>> usb@47401000 > >>>>> ethernet 1 [ + ] usb_ether | | | `-- > >>>>> usb_ether > >>>>> bootdev 3 [ ] eth_bootdev | | | `-- > >>>>> usb_ether.bootdev > >>>>> usb 0 [ ] ti-musb-host | | `-- > >>>>> usb@47401800 > >>>>> => fastboot usb 0 > >>>>> couldn't find an available UDC > >>>>> g_dnl_register: failed!, error: -19 > >>>> > >>>> That is expected and not a bug, since the beagle explicitly binds USB > >>>> ethernet to MUSB gadget in board file, which is legacy deprecated way. > >>> > >>> So, we should do away with, probably all of arch_misc_init() in > >>> arch/arm/mach-omap2/am33xx/board.c for the non-SPL case. > >> > >> Yes > >> > >>>>> exit not allowed from main input shell. > >>>>> => unbind /ocp/usb@4740/usb@47401000 usb_ether > >>>> > >>>> Does > >>>> >>>> => unbind ethernet 0 > >>>> > >>>> work ? > >>>> > >>>> If so, 1/4 in this series can be skipped altogether. > >>>> > >>>> You likely won't even need the rebinding of ti-musb-peripheral anymore. > > Did you test this yet ? Unfortunately it does not work. Indeed it would be much simpler than using the node path. Any idea why? Thanks, Miquèl
Re: [PATCH] cmd: Enable BIND by default if we have USB_ETHER
Hi Tom, tr...@konsulko.com wrote on Fri, 4 Aug 2023 12:06:21 -0400: > The nature of the network stack means that if we are going to use the > gadget mode USB network driver there's no easy path to implicitly > bind/unbind the driver. Enable the "bind" command by default here so > that we can bind/unbind this as needed. > > Signed-off-by: Tom Rini > --- > Cc: Marek Vasut > Cc: Miquel Raynal Reviewed-by: Miquel Raynal Thanks, Miquèl
Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter
Hi Tom, > > > > Cannot find a device with path /ocp/usb@4740/usb@47401000 > > > > => unbind /ocp/usb@4740/usb@47401000 > > > > => dm tree > > > > misc 0 [ + ] ti-musb-wrapper | |-- > > > > usb@4740 > > > > usb 0 [ ] ti-musb-host | | `-- > > > > usb@47401800 > > > > => fastboot usb 0 > > > > => bind /ocp/usb@4740/usb@47401000 ti-musb-peripheral > > > > => dm tree > > > > misc 0 [ + ] ti-musb-wrapper | |-- > > > > usb@4740 > > > > usb 0 [ ] ti-musb-host | | |-- > > > > usb@47401800 > > > > usb 0 [ ] ti-musb-peripheral| | `-- > > > > usb@47401000 > > > > => fastboot usb 0 > > > > musb-hdrc: peripheral reset irq lost! > > > > # works! (the irq-related line above as always been there) > > > > > > > > So now, how do we make this process easy/understandable? > > > > > > What would be your proposal ? > > > > > > > > At least I would appreciate: > > > > - to select CMD_BIND "by default" when relevant > > > > - to make the fastboot error more readable for the regular user > > > > > > Since with this 'unbind ethernet 0' this is orthogonal to this series, > > > send separate patches, thanks. > > > > This is not orthogonal, I am sorry. > > > > version X: > > - tftp works "out of the box" > > - fastboot works "out of the box" > > version X+1: > > - tftp works "out of the box" > > - fastboot returns an obscure error > > > > 1/ If we now *need* the bind/unbind commands, the series must take care > >of it. > > 2/ Without proper error message you just break fastboot for most > >regular users (basically everyone but few U-Boot devs). > > You're missing the class of users that will be impacted here. In order > for there to be a change here, you have to already be in the case where > you have CONFIG_USB_ETHER=y and gadget ethernet device isn't just > enabled but also initialized by default by calling usb_ether_init(). > That's a very small list. It's basically am33xx, two mediatek reference > platforms and xilinx_zynqmp_virt. Given that am33xx defconfigs also > setup DFU, I'm not really sure just how many people use gadget ethernet. > The normal flow on modern devices is to be calling bind/unbind here > already. Can we make this behavior explicit to the user? I am sorry, maybe it is the normal flow for you, but I am a regular U-Boot user and I totally missed that requirement. Typical situation: one needs to use but none is bound to the UDC (or another is bound), could we make the error messages more explicit if we decide not to unbind/bind the right one automatically because it is too "costly"? Thanks, Miquèl
Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter
Hi Tom, > > > >>> Well, what's needed / is it possible to get to the point where we > > > >>> don't > > > >>> _need_ to call bind/unbind for each of these cases? Is there something > > > >>> we're supposed to be setting in the DT that we aren't? > > > >> > > > >> You do need to unbind the ethernet before using fastboot, always, with > > > >> the 'unbind ethernet 0', you dont need the peripheral unbind/rebind > > > >> part, so it should behave like before. > > > > > > > > And for my own understanding, why don't we need to bind a fastboot > > > > gadget? > > > > > > The fastboot command does that internally . > > > > This is not visible with dm tree and I did not find how to bind the > > fastboot gadget manually. > > > > This makes the CLI clearly uneven and the internal code badly different > > between gadgets/commands. Why can't we have them both > > autoloaded/unloaded like before? I think I missed something which > > explains the rationale behind this series. > > They aren't both auto-loaded currently. We have a legacy call, > usb_ether_init(), in a few cases, so that gadget mode ethernet starts. > But this leads to the ref counting problems you encountered and > re-posted the rejected series for. Ok, thanks for the additional details. I don't understand why fastboot autoloads the correct gadget driver if there is none bound, while all network commands just fail to do that if we don't make the usb_ether_init() call manually. I also don't understand why I need to unbind the ethernet gadget but I cannot bind the fastboot gadget. My underlying question is: can we have a single approach for all drivers, or is it too complex today? Could it be possible, when we perform these autoloads, to look up the registered gadget and potentially unbind the one already in place before? Thanks, Miquèl
Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek, ma...@denx.de wrote on Fri, 4 Aug 2023 17:40:07 +0200: > On 8/4/23 17:12, Miquel Raynal wrote: > > Hi, > > > > ma...@denx.de wrote on Fri, 4 Aug 2023 17:05:01 +0200: > > > >> On 8/4/23 17:01, Tom Rini wrote: > >>> On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote: > >>>> On 8/4/23 09:00, Miquel Raynal wrote: > >>>>> Hi Marek, > >>>>> > >>>>> ma...@denx.de wrote on Wed, 2 Aug 2023 14:46:54 +0200: > >>>>> >>>>>> Extend the driver core to perform lookup by both OF node and > >>>>> driver > >>>>>> bound to the node. Use this to look up specific device instances to > >>>>>> unbind from nodes in the unbind command. One example where this is > >>>>>> needed is USB peripheral controller, which may have multiple gadget > >>>>>> drivers bound to it. The unbind command has to select that specific > >>>>>> gadget driver instance to unbind from the controller, not unbind the > >>>>>> controller driver itself from the controller. > >>>>>> > >>>>>> USB ethernet gadget usage looks as follows with this change. Notice > >>>>>> the extra 'usb_ether' addition in the 'unbind' command at the end. > >>>>>> " > >>>>>> bind /soc/usb-otg@4900 usb_ether > >>>>>> setenv ethact usb_ether > >>>>>> setenv loadaddr 0xc200 > >>>>>> setenv ipaddr 10.0.0.2 > >>>>>> setenv serverip 10.0.0.1 > >>>>>> setenv netmask 255.255.255.0 > >>>>>> tftpboot 0xc200 10.0.0.1:test.file > >>>>>> unbind /soc/usb-otg@4900 usb_ether > >>>>> > >>>>> This extra parameter does not seem to work on the BBBW. I cannot select > >>>>> the "usb_ether" driver like you do. > >>>>> > >>>>> Good news though, I am now able to use fastboot, but it is not > >>>>> straightforward: > >>>>> > >>>>> Here is my sequence right after the boot (reducing the dm tree output > >>>>> to the usb nodes for clarity): > >>>>> >>>>> => dm tree > >>>>> misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 > >>>>> usb 0 [ + ] ti-musb-peripheral| | |-- > >>>>> usb@47401000 > >>>>> ethernet 1 [ + ] usb_ether | | | `-- > >>>>> usb_ether > >>>>> bootdev 3 [ ] eth_bootdev | | | `-- > >>>>> usb_ether.bootdev > >>>>> usb 0 [ ] ti-musb-host | | `-- > >>>>> usb@47401800 > >>>>> => fastboot usb 0 > >>>>> couldn't find an available UDC > >>>>> g_dnl_register: failed!, error: -19 > >>>> > >>>> That is expected and not a bug, since the beagle explicitly binds USB > >>>> ethernet to MUSB gadget in board file, which is legacy deprecated way. > >>> > >>> So, we should do away with, probably all of arch_misc_init() in > >>> arch/arm/mach-omap2/am33xx/board.c for the non-SPL case. > >> > >> Yes > >> > >>>>> exit not allowed from main input shell. > >>>>> => unbind /ocp/usb@4740/usb@47401000 usb_ether > >>>> > >>>> Does > >>>> >>>> => unbind ethernet 0 > >>>> > >>>> work ? > >>>> > >>>> If so, 1/4 in this series can be skipped altogether. > >>>> > >>>> You likely won't even need the rebinding of ti-musb-peripheral anymore. > > Did you test this yet ? Not yet, I'll send you the feedback once done. > >>>>> Cannot find a device with path /ocp/usb@4740/usb@47401000 > >>>>> => unbind /ocp/usb@4740/usb@47401000 > >>>>> => dm tree > >>>>> misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 > >>>>> usb 0 [ ] ti-musb-host | | `-- > >>>>> usb@47401800 > >>>>> => fastboot usb 0 > >>>>> => bind /ocp/usb
Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter
Hi, ma...@denx.de wrote on Fri, 4 Aug 2023 17:05:01 +0200: > On 8/4/23 17:01, Tom Rini wrote: > > On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote: > >> On 8/4/23 09:00, Miquel Raynal wrote: > >>> Hi Marek, > >>> > >>> ma...@denx.de wrote on Wed, 2 Aug 2023 14:46:54 +0200: > >>> > >>>> Extend the driver core to perform lookup by both OF node and driver > >>>> bound to the node. Use this to look up specific device instances to > >>>> unbind from nodes in the unbind command. One example where this is > >>>> needed is USB peripheral controller, which may have multiple gadget > >>>> drivers bound to it. The unbind command has to select that specific > >>>> gadget driver instance to unbind from the controller, not unbind the > >>>> controller driver itself from the controller. > >>>> > >>>> USB ethernet gadget usage looks as follows with this change. Notice > >>>> the extra 'usb_ether' addition in the 'unbind' command at the end. > >>>> " > >>>> bind /soc/usb-otg@4900 usb_ether > >>>> setenv ethact usb_ether > >>>> setenv loadaddr 0xc200 > >>>> setenv ipaddr 10.0.0.2 > >>>> setenv serverip 10.0.0.1 > >>>> setenv netmask 255.255.255.0 > >>>> tftpboot 0xc200 10.0.0.1:test.file > >>>> unbind /soc/usb-otg@4900 usb_ether > >>> > >>> This extra parameter does not seem to work on the BBBW. I cannot select > >>> the "usb_ether" driver like you do. > >>> > >>> Good news though, I am now able to use fastboot, but it is not > >>> straightforward: > >>> > >>> Here is my sequence right after the boot (reducing the dm tree output > >>> to the usb nodes for clarity): > >>> > >>> => dm tree > >>>misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 > >>>usb 0 [ + ] ti-musb-peripheral| | |-- usb@47401000 > >>>ethernet 1 [ + ] usb_ether | | | `-- > >>> usb_ether > >>>bootdev 3 [ ] eth_bootdev | | | `-- > >>> usb_ether.bootdev > >>>usb 0 [ ] ti-musb-host | | `-- > >>> usb@47401800 > >>> => fastboot usb 0 > >>> couldn't find an available UDC > >>> g_dnl_register: failed!, error: -19 > >> > >> That is expected and not a bug, since the beagle explicitly binds USB > >> ethernet to MUSB gadget in board file, which is legacy deprecated way. > > > > So, we should do away with, probably all of arch_misc_init() in > > arch/arm/mach-omap2/am33xx/board.c for the non-SPL case. > > Yes > > >>> exit not allowed from main input shell. > >>> => unbind /ocp/usb@4740/usb@47401000 usb_ether > >> > >> Does > >> > >> => unbind ethernet 0 > >> > >> work ? > >> > >> If so, 1/4 in this series can be skipped altogether. > >> > >> You likely won't even need the rebinding of ti-musb-peripheral anymore. > >> > >>> Cannot find a device with path /ocp/usb@4740/usb@47401000 > >>> => unbind /ocp/usb@4740/usb@47401000 > >>> => dm tree > >>>misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 > >>>usb 0 [ ] ti-musb-host | | `-- > >>> usb@47401800 > >>> => fastboot usb 0 > >>> => bind /ocp/usb@4740/usb@47401000 ti-musb-peripheral > >>> => dm tree > >>>misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 > >>>usb 0 [ ] ti-musb-host | | |-- usb@47401800 > >>>usb 0 [ ] ti-musb-peripheral| | `-- > >>> usb@47401000 > >>> => fastboot usb 0 > >>> musb-hdrc: peripheral reset irq lost! > >>> # works! (the irq-related line above as always been there) > >>> > >>> So now, how do we make this process easy/understandable? > >> > >> What would be your proposal ? At least I would appreciate: - to select CMD_BIND "by default" when relevant - to make the fastboot error more readable for the regular user If you want to get rid of all the legacy code, I am not opposed, really, but that must not be the user who is responsible for understanding what changed in the "core". It must be very easily accessible to understand that now: - manual binding/unbinding is needed - which driver must be used when > > Well, what's needed / is it possible to get to the point where we don't > > _need_ to call bind/unbind for each of these cases? Is there something > > we're supposed to be setting in the DT that we aren't? > > You do need to unbind the ethernet before using fastboot, always, with the > 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it > should behave like before. And for my own understanding, why don't we need to bind a fastboot gadget? Thanks, Miquèl
Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek, ma...@denx.de wrote on Wed, 2 Aug 2023 14:46:54 +0200: > Extend the driver core to perform lookup by both OF node and driver > bound to the node. Use this to look up specific device instances to > unbind from nodes in the unbind command. One example where this is > needed is USB peripheral controller, which may have multiple gadget > drivers bound to it. The unbind command has to select that specific > gadget driver instance to unbind from the controller, not unbind the > controller driver itself from the controller. > > USB ethernet gadget usage looks as follows with this change. Notice > the extra 'usb_ether' addition in the 'unbind' command at the end. > " > bind /soc/usb-otg@4900 usb_ether > setenv ethact usb_ether > setenv loadaddr 0xc200 > setenv ipaddr 10.0.0.2 > setenv serverip 10.0.0.1 > setenv netmask 255.255.255.0 > tftpboot 0xc200 10.0.0.1:test.file > unbind /soc/usb-otg@4900 usb_ether This extra parameter does not seem to work on the BBBW. I cannot select the "usb_ether" driver like you do. Good news though, I am now able to use fastboot, but it is not straightforward: Here is my sequence right after the boot (reducing the dm tree output to the usb nodes for clarity): => dm tree misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 usb 0 [ + ] ti-musb-peripheral| | |-- usb@47401000 ethernet 1 [ + ] usb_ether | | | `-- usb_ether bootdev 3 [ ] eth_bootdev | | | `-- usb_ether.bootdev usb 0 [ ] ti-musb-host | | `-- usb@47401800 => fastboot usb 0 couldn't find an available UDC g_dnl_register: failed!, error: -19 exit not allowed from main input shell. => unbind /ocp/usb@4740/usb@47401000 usb_ether Cannot find a device with path /ocp/usb@4740/usb@47401000 => unbind /ocp/usb@4740/usb@47401000 => dm tree misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 usb 0 [ ] ti-musb-host | | `-- usb@47401800 => fastboot usb 0 => bind /ocp/usb@4740/usb@47401000 ti-musb-peripheral => dm tree misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 usb 0 [ ] ti-musb-host | | |-- usb@47401800 usb 0 [ ] ti-musb-peripheral| | `-- usb@47401000 => fastboot usb 0 musb-hdrc: peripheral reset irq lost! # works! (the irq-related line above as always been there) So now, how do we make this process easy/understandable? Thanks, Miquèl
Re: [PATCH v3 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek, ma...@denx.de wrote on Wed, 2 Aug 2023 16:38:47 +0200: > On 8/2/23 09:48, Miquel Raynal wrote: > > Hi Marek, > > > > ma...@denx.de wrote on Wed, 2 Aug 2023 01:07:46 +0200: > > > >> On 8/1/23 20:53, Miquel Raynal wrote: > >>> Hi Marek, > >>> > >>> ma...@denx.de wrote on Mon, 31 Jul 2023 16:40:04 +0200: > >>>>>>> On 7/31/23 16:25, Miquel Raynal wrote: > >>>>> Hi Marek, > >>>>> > >>>>> ma...@denx.de wrote on Mon, 31 Jul 2023 16:08:19 +0200: > >>>>> >>>> On 7/31/23 15:58, Miquel Raynal wrote: > >>>>>>> Hi Marek, > >>>>>>> > >>>>>>> ma...@denx.de wrote on Mon, 31 Jul 2023 15:50:58 +0200: > >>>>>>> >>>> On 7/31/23 15:36, Miquel Raynal wrote: > >>>>>>>>> Hi Marek, > >>>>>>>>> > >>>>>>>>> ma...@denx.de wrote on Mon, 31 Jul 2023 13:44:25 +0200: > >>>>>>>>> >>>> On 7/31/23 11:31, Miquel Raynal wrote: > >>>>>>>>>>> Hi Marek, > >>>>>>>>>>> > >>>>>>>>>>> ma...@denx.de wrote on Sat, 29 Jul 2023 16:57:09 +0200: > >>>>>>>>>>>>>>> Extend the driver core to perform lookup by both OF > >>>>>>>>>>> node and driver > >>>>>>>>>>>> bound to the node. Use this to look up specific device instances > >>>>>>>>>>>> to > >>>>>>>>>>>> unbind from nodes in the unbind command. One example where this > >>>>>>>>>>>> is > >>>>>>>>>>>> needed is USB peripheral controller, which may have multiple > >>>>>>>>>>>> gadget > >>>>>>>>>>>> drivers bound to it. The unbind command has to select that > >>>>>>>>>>>> specific > >>>>>>>>>>>> gadget driver instance to unbind from the controller, not unbind > >>>>>>>>>>>> the > >>>>>>>>>>>> controller driver itself from the controller. > >>>>>>>>>>>> > >>>>>>>>>>>> USB ethernet gadget usage looks as follows with this change. > >>>>>>>>>>>> Notice > >>>>>>>>>>>> the extra 'usb_ether' addition in the 'unbind' command at the > >>>>>>>>>>>> end. > >>>>>>>>>>>> " > >>>>>>>>>>>> bind /soc/usb-otg@4900 usb_ether > >>>>>>>>>>>> setenv ethact usb_ether > >>>>>>>>>>>> setenv loadaddr 0xc200 > >>>>>>>>>>>> setenv ipaddr 10.0.0.2 > >>>>>>>>>>>> setenv serverip 10.0.0.1 > >>>>>>>>>>>> setenv netmask 255.255.255.0 > >>>>>>>>>>>> tftpboot 0xc200 10.0.0.1:test.file > >>>>>>>>>>>> unbind /soc/usb-otg@4900 usb_ether > >>>>>>>>>>>> " > >>>>>>>>>>>> > >>>>>>>>>>>> Signed-off-by: Marek Vasut > >>>>>>>>>>>> --- > >>>>>>>>>>> > >>>>>>>>>>> I am no longer getting wrong pointer dereferences, the SPL is > >>>>>>>>>>> working in > >>>>>>>>>>> recovery mode, TFTP "File not found" errors are no longer a > >>>>>>>>>>> problem and > >>>>>>>>>>> I did not experience any reset while tftp'ing regular files. > >>>>>>>>>>> > >>>>>>>>>>> One last remaining request on my side is the need for using > >>>>>>>>>>> fastboot as > >>>>>>>>>>> well which does no longer work as-is: > >>>>>>>>>>>>&g
Re: [PATCH v3 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek, ma...@denx.de wrote on Wed, 2 Aug 2023 01:07:46 +0200: > On 8/1/23 20:53, Miquel Raynal wrote: > > Hi Marek, > > > > ma...@denx.de wrote on Mon, 31 Jul 2023 16:40:04 +0200: > > > >> On 7/31/23 16:25, Miquel Raynal wrote: > >>> Hi Marek, > >>> > >>> ma...@denx.de wrote on Mon, 31 Jul 2023 16:08:19 +0200: > >>>>>>> On 7/31/23 15:58, Miquel Raynal wrote: > >>>>> Hi Marek, > >>>>> > >>>>> ma...@denx.de wrote on Mon, 31 Jul 2023 15:50:58 +0200: > >>>>> >>>> On 7/31/23 15:36, Miquel Raynal wrote: > >>>>>>> Hi Marek, > >>>>>>> > >>>>>>> ma...@denx.de wrote on Mon, 31 Jul 2023 13:44:25 +0200: > >>>>>>> >>>> On 7/31/23 11:31, Miquel Raynal wrote: > >>>>>>>>> Hi Marek, > >>>>>>>>> > >>>>>>>>> ma...@denx.de wrote on Sat, 29 Jul 2023 16:57:09 +0200: > >>>>>>>>> >>>> Extend the driver core to perform lookup by both OF node > >>>>>>>>> and driver > >>>>>>>>>> bound to the node. Use this to look up specific device instances to > >>>>>>>>>> unbind from nodes in the unbind command. One example where this is > >>>>>>>>>> needed is USB peripheral controller, which may have multiple gadget > >>>>>>>>>> drivers bound to it. The unbind command has to select that specific > >>>>>>>>>> gadget driver instance to unbind from the controller, not unbind > >>>>>>>>>> the > >>>>>>>>>> controller driver itself from the controller. > >>>>>>>>>> > >>>>>>>>>> USB ethernet gadget usage looks as follows with this change. Notice > >>>>>>>>>> the extra 'usb_ether' addition in the 'unbind' command at the end. > >>>>>>>>>> " > >>>>>>>>>> bind /soc/usb-otg@4900 usb_ether > >>>>>>>>>> setenv ethact usb_ether > >>>>>>>>>> setenv loadaddr 0xc200 > >>>>>>>>>> setenv ipaddr 10.0.0.2 > >>>>>>>>>> setenv serverip 10.0.0.1 > >>>>>>>>>> setenv netmask 255.255.255.0 > >>>>>>>>>> tftpboot 0xc200 10.0.0.1:test.file > >>>>>>>>>> unbind /soc/usb-otg@4900 usb_ether > >>>>>>>>>> " > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Marek Vasut > >>>>>>>>>> --- > >>>>>>>>> > >>>>>>>>> I am no longer getting wrong pointer dereferences, the SPL is > >>>>>>>>> working in > >>>>>>>>> recovery mode, TFTP "File not found" errors are no longer a problem > >>>>>>>>> and > >>>>>>>>> I did not experience any reset while tftp'ing regular files. > >>>>>>>>> > >>>>>>>>> One last remaining request on my side is the need for using > >>>>>>>>> fastboot as > >>>>>>>>> well which does no longer work as-is: > >>>>>>>>> >>> => fastboot usb 0 > >>>>>>>>> couldn't find an available UDC > >>>>>>>>> g_dnl_register: failed!, error: -19 > >>>>>>>>> exit not allowed from main input shell. > >>>>>>>>> > >>>>>>>>> Can you advise what bind/unbind command would be necessary here? > >>>>>>>> > >>>>>>>> Either 'unbind usb_ether' or run 'dm tree' -> look up the path to > >>>>>>>> usb_ether in the tree (it will be hanging under usb_peripheral or > >>>>>>>> some such), and then use 'unbind '. > >>>>>>> > >>>>>>> Nice `dm tree` command, never used it before. > >>>>>>> > >>>>>>> Even when I unbind usb_ether I still get the sam
Re: [PATCH v3 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek, ma...@denx.de wrote on Mon, 31 Jul 2023 16:40:04 +0200: > On 7/31/23 16:25, Miquel Raynal wrote: > > Hi Marek, > > > > ma...@denx.de wrote on Mon, 31 Jul 2023 16:08:19 +0200: > > > >> On 7/31/23 15:58, Miquel Raynal wrote: > >>> Hi Marek, > >>> > >>> ma...@denx.de wrote on Mon, 31 Jul 2023 15:50:58 +0200: > >>>>>>> On 7/31/23 15:36, Miquel Raynal wrote: > >>>>> Hi Marek, > >>>>> > >>>>> ma...@denx.de wrote on Mon, 31 Jul 2023 13:44:25 +0200: > >>>>> >>>> On 7/31/23 11:31, Miquel Raynal wrote: > >>>>>>> Hi Marek, > >>>>>>> > >>>>>>> ma...@denx.de wrote on Sat, 29 Jul 2023 16:57:09 +0200: > >>>>>>> >>>> Extend the driver core to perform lookup by both OF node > >>>>>>> and driver > >>>>>>>> bound to the node. Use this to look up specific device instances to > >>>>>>>> unbind from nodes in the unbind command. One example where this is > >>>>>>>> needed is USB peripheral controller, which may have multiple gadget > >>>>>>>> drivers bound to it. The unbind command has to select that specific > >>>>>>>> gadget driver instance to unbind from the controller, not unbind the > >>>>>>>> controller driver itself from the controller. > >>>>>>>> > >>>>>>>> USB ethernet gadget usage looks as follows with this change. Notice > >>>>>>>> the extra 'usb_ether' addition in the 'unbind' command at the end. > >>>>>>>> " > >>>>>>>> bind /soc/usb-otg@4900 usb_ether > >>>>>>>> setenv ethact usb_ether > >>>>>>>> setenv loadaddr 0xc200 > >>>>>>>> setenv ipaddr 10.0.0.2 > >>>>>>>> setenv serverip 10.0.0.1 > >>>>>>>> setenv netmask 255.255.255.0 > >>>>>>>> tftpboot 0xc200 10.0.0.1:test.file > >>>>>>>> unbind /soc/usb-otg@4900 usb_ether > >>>>>>>> " > >>>>>>>> > >>>>>>>> Signed-off-by: Marek Vasut > >>>>>>>> --- > >>>>>>> > >>>>>>> I am no longer getting wrong pointer dereferences, the SPL is working > >>>>>>> in > >>>>>>> recovery mode, TFTP "File not found" errors are no longer a problem > >>>>>>> and > >>>>>>> I did not experience any reset while tftp'ing regular files. > >>>>>>> > >>>>>>> One last remaining request on my side is the need for using fastboot > >>>>>>> as > >>>>>>> well which does no longer work as-is: > >>>>>>> >>> => fastboot usb 0 > >>>>>>> couldn't find an available UDC > >>>>>>> g_dnl_register: failed!, error: -19 > >>>>>>> exit not allowed from main input shell. > >>>>>>> > >>>>>>> Can you advise what bind/unbind command would be necessary here? > >>>>>> > >>>>>> Either 'unbind usb_ether' or run 'dm tree' -> look up the path to > >>>>>> usb_ether in the tree (it will be hanging under usb_peripheral or some > >>>>>> such), and then use 'unbind '. > >>>>> > >>>>> Nice `dm tree` command, never used it before. > >>>>> > >>>>> Even when I unbind usb_ether I still get the same error: > >>>>> >>> => unbind /ocp/usb@4740/usb@47401000 > >>>>> => fastboot usb 0 > >>>>> couldn't find an available UDC > >>>>> g_dnl_register: failed!, error: -19 > >>>>> exit not allowed from main input shell. > >>>>> > >>>>> Is there a specific gadget driver which I should bind again manually? > >>>> > >>>> Can you share the output of dm tree before/after unbind ? > >>>> > >>>> fastboot should auto-bind to the right thing. > >>> > >>> Ok. Apparently it does not, b
Re: [PATCH v3 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek, ma...@denx.de wrote on Mon, 31 Jul 2023 16:08:19 +0200: > On 7/31/23 15:58, Miquel Raynal wrote: > > Hi Marek, > > > > ma...@denx.de wrote on Mon, 31 Jul 2023 15:50:58 +0200: > > > >> On 7/31/23 15:36, Miquel Raynal wrote: > >>> Hi Marek, > >>> > >>> ma...@denx.de wrote on Mon, 31 Jul 2023 13:44:25 +0200: > >>>>>>> On 7/31/23 11:31, Miquel Raynal wrote: > >>>>> Hi Marek, > >>>>> > >>>>> ma...@denx.de wrote on Sat, 29 Jul 2023 16:57:09 +0200: > >>>>> >>>> Extend the driver core to perform lookup by both OF node and > >>>>> driver > >>>>>> bound to the node. Use this to look up specific device instances to > >>>>>> unbind from nodes in the unbind command. One example where this is > >>>>>> needed is USB peripheral controller, which may have multiple gadget > >>>>>> drivers bound to it. The unbind command has to select that specific > >>>>>> gadget driver instance to unbind from the controller, not unbind the > >>>>>> controller driver itself from the controller. > >>>>>> > >>>>>> USB ethernet gadget usage looks as follows with this change. Notice > >>>>>> the extra 'usb_ether' addition in the 'unbind' command at the end. > >>>>>> " > >>>>>> bind /soc/usb-otg@4900 usb_ether > >>>>>> setenv ethact usb_ether > >>>>>> setenv loadaddr 0xc200 > >>>>>> setenv ipaddr 10.0.0.2 > >>>>>> setenv serverip 10.0.0.1 > >>>>>> setenv netmask 255.255.255.0 > >>>>>> tftpboot 0xc200 10.0.0.1:test.file > >>>>>> unbind /soc/usb-otg@4900 usb_ether > >>>>>> " > >>>>>> > >>>>>> Signed-off-by: Marek Vasut > >>>>>> --- > >>>>> > >>>>> I am no longer getting wrong pointer dereferences, the SPL is working in > >>>>> recovery mode, TFTP "File not found" errors are no longer a problem and > >>>>> I did not experience any reset while tftp'ing regular files. > >>>>> > >>>>> One last remaining request on my side is the need for using fastboot as > >>>>> well which does no longer work as-is: > >>>>> >>> => fastboot usb 0 > >>>>> couldn't find an available UDC > >>>>> g_dnl_register: failed!, error: -19 > >>>>> exit not allowed from main input shell. > >>>>> > >>>>> Can you advise what bind/unbind command would be necessary here? > >>>> > >>>> Either 'unbind usb_ether' or run 'dm tree' -> look up the path to > >>>> usb_ether in the tree (it will be hanging under usb_peripheral or some > >>>> such), and then use 'unbind '. > >>> > >>> Nice `dm tree` command, never used it before. > >>> > >>> Even when I unbind usb_ether I still get the same error: > >>>>>> => unbind /ocp/usb@4740/usb@47401000 > >>> => fastboot usb 0 > >>> couldn't find an available UDC > >>> g_dnl_register: failed!, error: -19 > >>> exit not allowed from main input shell. > >>> > >>> Is there a specific gadget driver which I should bind again manually? > >> > >> Can you share the output of dm tree before/after unbind ? > >> > >> fastboot should auto-bind to the right thing. > > > > Ok. Apparently it does not, but I don't have any clue why. If you want > > me to check something else I will. Here is the output: > > > > U-Boot 2023.07-00806-g979e7443428 (Jul 31 2023 - 11:17:06 +0200) > > [...] > > > watchdog 0 [ + ] omap3_wdt | |-- wdt@44e35000 > > misc 0 [ + ] ti-musb-wrapper | |-- usb@4740 > > usb 0 [ + ] ti-musb-peripheral| | |-- usb@47401000 > > ethernet 1 [ + ] usb_ether | | | `-- usb_ether > > bootdev 3 [ ] eth_bootdev | | | `-- > > usb_ether.bootdev > > usb 0 [ ] ti-musb-host | | `-- usb@47401800 > > ethernet 0 [ + ] eth_cpsw | |-- ethernet@4a10 > > bootdev 2 [ ] eth_bootdev | | `-- > > ethernet@4a10.bootdev > > simple_bus 67 [ ] ti_sysc | |-- > > target-module@5310 > > simple_bus 68 [ ] ti_sysc | |-- > > target-module@5350 > > simple_bus 69 [ ] ti_sysc | `-- > > target-module@5600 > > clk 62 [ ] fixed_clock |-- clk_mcasp0_fixed > > bootstd 0 [ ] bootstd_drv |-- bootstd > > bootmeth 0 [ ] bootmeth_efi | |-- efi > > bootmeth 1 [ ] bootmeth_extlinux | |-- extlinux > > bootmeth 2 [ ] bootmeth_pxe | |-- pxe > > bootmeth 3 [ ] vbe_simple| `-- vbe_simple > > timer 0 [ + ] omap_timer`-- timer@0 > > => unbind /ocp/usb@4740/usb@47401000 > > The commit message of this patch contains the following example: > unbind /soc/usb-otg@4900 usb_ether > so in your case, try > unbind /ocp/usb@4740/usb@47401000 usb_ether Does not work, unfortunately: => unbind /ocp/usb@4740/usb@47401000 usb_ether Cannot find a device with path /ocp/usb@4740/usb@47401000 => unbind /ocp/usb@4740/usb@47401000 =>
Re: [PATCH v3 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek, ma...@denx.de wrote on Mon, 31 Jul 2023 15:50:58 +0200: > On 7/31/23 15:36, Miquel Raynal wrote: > > Hi Marek, > > > > ma...@denx.de wrote on Mon, 31 Jul 2023 13:44:25 +0200: > > > >> On 7/31/23 11:31, Miquel Raynal wrote: > >>> Hi Marek, > >>> > >>> ma...@denx.de wrote on Sat, 29 Jul 2023 16:57:09 +0200: > >>>>>>> Extend the driver core to perform lookup by both OF node and > >>> driver > >>>> bound to the node. Use this to look up specific device instances to > >>>> unbind from nodes in the unbind command. One example where this is > >>>> needed is USB peripheral controller, which may have multiple gadget > >>>> drivers bound to it. The unbind command has to select that specific > >>>> gadget driver instance to unbind from the controller, not unbind the > >>>> controller driver itself from the controller. > >>>> > >>>> USB ethernet gadget usage looks as follows with this change. Notice > >>>> the extra 'usb_ether' addition in the 'unbind' command at the end. > >>>> " > >>>> bind /soc/usb-otg@4900 usb_ether > >>>> setenv ethact usb_ether > >>>> setenv loadaddr 0xc200 > >>>> setenv ipaddr 10.0.0.2 > >>>> setenv serverip 10.0.0.1 > >>>> setenv netmask 255.255.255.0 > >>>> tftpboot 0xc200 10.0.0.1:test.file > >>>> unbind /soc/usb-otg@4900 usb_ether > >>>> " > >>>> > >>>> Signed-off-by: Marek Vasut > >>>> --- > >>> > >>> I am no longer getting wrong pointer dereferences, the SPL is working in > >>> recovery mode, TFTP "File not found" errors are no longer a problem and > >>> I did not experience any reset while tftp'ing regular files. > >>> > >>> One last remaining request on my side is the need for using fastboot as > >>> well which does no longer work as-is: > >>>>>> => fastboot usb 0 > >>> couldn't find an available UDC > >>> g_dnl_register: failed!, error: -19 > >>> exit not allowed from main input shell. > >>> > >>> Can you advise what bind/unbind command would be necessary here? > >> > >> Either 'unbind usb_ether' or run 'dm tree' -> look up the path to > >> usb_ether in the tree (it will be hanging under usb_peripheral or some > >> such), and then use 'unbind '. > > > > Nice `dm tree` command, never used it before. > > > > Even when I unbind usb_ether I still get the same error: > > > > => unbind /ocp/usb@4740/usb@47401000 > > => fastboot usb 0 > > couldn't find an available UDC > > g_dnl_register: failed!, error: -19 > > exit not allowed from main input shell. > > > > Is there a specific gadget driver which I should bind again manually? > > Can you share the output of dm tree before/after unbind ? > > fastboot should auto-bind to the right thing. Ok. Apparently it does not, but I don't have any clue why. If you want me to check something else I will. Here is the output: U-Boot 2023.07-00806-g979e7443428 (Jul 31 2023 - 11:17:06 +0200) CPU : AM335X-GP rev 2.1 Model: TI AM335x BeagleBone Black DRAM: 512 MiB Core: 160 devices, 18 uclasses, devicetree: separate WDT: Started wdt@44e35000 with servicing every 1000ms (60s timeout) NAND: 0 MiB MMC: OMAP SD/MMC: 0, OMAP SD/MMC: 1 Loading Environment from FAT... Unable to read "uboot.env" from mmc1:1... not set. Validating first E-fuse MAC Net: Could not get PHY for ethernet@4a10: addr 0 eth2: ethernet@4a10using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in MAC de:ad:be:ef:00:01 HOST MAC de:ad:be:ef:00:00 RNDIS ready , eth3: usb_ether => dm tree Class Index Probed DriverName --- root 0 [ + ] root_driver root_driver rsa_mod_ex0 [ ] mod_exp_sw|-- mod_exp_sw simple_bus0 [ + ] simple_bus|-- ocp simple_bus1 [ + ] simple_bus| |-- l4_wkup@44c0 simple_bus2 [ ] simple_bus| | |-- segment@0 simple_bus3 [ ] simple_bus| | |-- segment@10 simple_bus4 [ + ] simple_bus| | `-- segment@20 simple_bus5 [ + ] ti_sysc | | |-- target-module@0 simple_bus6 [ + ] simple_bus| | | `-- prcm@0 simple_bus7 [ ]
Re: [PATCH v3 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek, ma...@denx.de wrote on Mon, 31 Jul 2023 13:44:25 +0200: > On 7/31/23 11:31, Miquel Raynal wrote: > > Hi Marek, > > > > ma...@denx.de wrote on Sat, 29 Jul 2023 16:57:09 +0200: > > > >> Extend the driver core to perform lookup by both OF node and driver > >> bound to the node. Use this to look up specific device instances to > >> unbind from nodes in the unbind command. One example where this is > >> needed is USB peripheral controller, which may have multiple gadget > >> drivers bound to it. The unbind command has to select that specific > >> gadget driver instance to unbind from the controller, not unbind the > >> controller driver itself from the controller. > >> > >> USB ethernet gadget usage looks as follows with this change. Notice > >> the extra 'usb_ether' addition in the 'unbind' command at the end. > >> " > >> bind /soc/usb-otg@4900 usb_ether > >> setenv ethact usb_ether > >> setenv loadaddr 0xc200 > >> setenv ipaddr 10.0.0.2 > >> setenv serverip 10.0.0.1 > >> setenv netmask 255.255.255.0 > >> tftpboot 0xc200 10.0.0.1:test.file > >> unbind /soc/usb-otg@4900 usb_ether > >> " > >> > >> Signed-off-by: Marek Vasut > >> --- > > > > I am no longer getting wrong pointer dereferences, the SPL is working in > > recovery mode, TFTP "File not found" errors are no longer a problem and > > I did not experience any reset while tftp'ing regular files. > > > > One last remaining request on my side is the need for using fastboot as > > well which does no longer work as-is: > > > > => fastboot usb 0 > > couldn't find an available UDC > > g_dnl_register: failed!, error: -19 > > exit not allowed from main input shell. > > > > Can you advise what bind/unbind command would be necessary here? > > Either 'unbind usb_ether' or run 'dm tree' -> look up the path to usb_ether > in the tree (it will be hanging under usb_peripheral or some such), and then > use 'unbind '. Nice `dm tree` command, never used it before. Even when I unbind usb_ether I still get the same error: => unbind /ocp/usb@4740/usb@47401000 => fastboot usb 0 couldn't find an available UDC g_dnl_register: failed!, error: -19 exit not allowed from main input shell. Is there a specific gadget driver which I should bind again manually? Thanks, Miquèl
Re: [PATCH v3 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek,
ma...@denx.de wrote on Sat, 29 Jul 2023 16:57:09 +0200:
> Extend the driver core to perform lookup by both OF node and driver
> bound to the node. Use this to look up specific device instances to
> unbind from nodes in the unbind command. One example where this is
> needed is USB peripheral controller, which may have multiple gadget
> drivers bound to it. The unbind command has to select that specific
> gadget driver instance to unbind from the controller, not unbind the
> controller driver itself from the controller.
>
> USB ethernet gadget usage looks as follows with this change. Notice
> the extra 'usb_ether' addition in the 'unbind' command at the end.
> "
> bind /soc/usb-otg@4900 usb_ether
> setenv ethact usb_ether
> setenv loadaddr 0xc200
> setenv ipaddr 10.0.0.2
> setenv serverip 10.0.0.1
> setenv netmask 255.255.255.0
> tftpboot 0xc200 10.0.0.1:test.file
> unbind /soc/usb-otg@4900 usb_ether
> "
>
> Signed-off-by: Marek Vasut
> ---
I am no longer getting wrong pointer dereferences, the SPL is working in
recovery mode, TFTP "File not found" errors are no longer a problem and
I did not experience any reset while tftp'ing regular files.
One last remaining request on my side is the need for using fastboot as
well which does no longer work as-is:
=> fastboot usb 0
couldn't find an available UDC
g_dnl_register: failed!, error: -19
exit not allowed from main input shell.
Can you advise what bind/unbind command would be necessary here?
Thanks,
Miquèl
> Cc: Kevin Hilman
> Cc: Lukasz Majewski
> Cc: Miquel Raynal
> Cc: Simon Glass
> ---
> V2: No change
> V3: No change
> ---
> cmd/bind.c| 10 +-
> drivers/core/device.c | 20 +++-
> include/dm/device.h | 17 +
> 3 files changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/cmd/bind.c b/cmd/bind.c
> index 4d1b7885e60..3137cdf6cb5 100644
> --- a/cmd/bind.c
> +++ b/cmd/bind.c
> @@ -162,7 +162,7 @@ static int bind_by_node_path(const char *path, const char
> *drv_name)
> return 0;
> }
>
> -static int unbind_by_node_path(const char *path)
> +static int unbind_by_node_path(const char *path, const char *drv_name)
> {
> struct udevice *dev;
> int ret;
> @@ -174,7 +174,7 @@ static int unbind_by_node_path(const char *path)
> return -EINVAL;
> }
>
> - ret = device_find_global_by_ofnode(ofnode, );
> + ret = device_find_global_by_ofnode_driver(ofnode, drv_name, );
>
> if (!dev || ret) {
> printf("Cannot find a device with path %s\n", path);
> @@ -214,9 +214,9 @@ static int do_bind_unbind(struct cmd_tbl *cmdtp, int
> flag, int argc,
> return CMD_RET_USAGE;
> ret = bind_by_node_path(argv[1], argv[2]);
> } else if (by_node && !bind) {
> - if (argc != 2)
> + if (argc != 2 && argc != 3)
> return CMD_RET_USAGE;
> - ret = unbind_by_node_path(argv[1]);
> + ret = unbind_by_node_path(argv[1], argv[2]);
> } else if (!by_node && bind) {
> int index = (argc > 2) ? dectoul(argv[2], NULL) : 0;
>
> @@ -251,7 +251,7 @@ U_BOOT_CMD(
> U_BOOT_CMD(
> unbind, 4, 0, do_bind_unbind,
> "Unbind a device from a driver",
> - "\n"
> + " []\n"
> "unbind \n"
> "unbind \n"
> );
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 6e26b64fb81..52fceb69341 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -877,15 +877,17 @@ int device_get_child_by_of_offset(const struct udevice
> *parent, int node,
> }
>
> static struct udevice *_device_find_global_by_ofnode(struct udevice *parent,
> - ofnode ofnode)
> + ofnode ofnode,
> + const char *drv)
> {
> struct udevice *dev, *found;
>
> - if (ofnode_equal(dev_ofnode(parent), ofnode))
> + if (ofnode_equal(dev_ofnode(parent), ofnode) &&
> + (!drv || (drv && !strcmp(parent->driver->name, drv
> return parent;
>
> device_foreach_child(dev, parent) {
> - found = _device_find_global_by_ofnode(dev, ofnode);
> + found = _device_find_global_by_ofnode(dev, ofnode, drv);
> if (found)
> return found;
> }
> @@ -895,7 +897,15 @@ static struct udevice
> *_devi
Re: [PATCH v2 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek, ma...@denx.de wrote on Sun, 23 Jul 2023 23:45:55 +0200: > On 7/23/23 19:49, Miquel Raynal wrote: > > Hi Marek, > > Hi, > > > ma...@denx.de wrote on Mon, 17 Jul 2023 13:21:34 +0200: > > > >> Extend the driver core to perform lookup by both OF node and driver > >> bound to the node. Use this to look up specific device instances to > >> unbind from nodes in the unbind command. One example where this is > >> needed is USB peripheral controller, which may have multiple gadget > >> drivers bound to it. The unbind command has to select that specific > >> gadget driver instance to unbind from the controller, not unbind the > >> controller driver itself from the controller. > >> > >> USB ethernet gadget usage looks as follows with this change. Notice > >> the extra 'usb_ether' addition in the 'unbind' command at the end. > >> " > >> bind /soc/usb-otg@4900 usb_ether > > > > I don't really get why this is needed? Yes, having proper bind and > > unbind methods and having them called internally is relevant, but when > > you have a single OTG controller, why is this needed? > > Because you do need to bind the usb_ether driver to a controller. Right now it seems that binding is kind of performed in the context of a command execution (and then everything is cleaned up). I also think it is not optimal, but I would like to avoid breaking common commands like you propose. At the *very least* the error messages should be very clear about what has changed and what the user needs to do now in order to workaround it. But the best would be to avoid user interaction as much as possible. So why not load the first driver needed and expect the user the run the correct unbind/bind commands if it wants to change? > I suspect you are trying to point in the direction of usb_ether_init() , > right ? That is bogus and should be removed, because that is hard-coding one > specific (ethernet) function to an OTG controller. Well, hardcoding is bad, but setting a default is often useful and much more user friendly, IMHO. > > > It basically > > breaks the CLI > > Can you elaborate on this ? > > How does calling a 'bind' command breaks CLI ? I've attempted a bisect between U-Boot 2018 and 2023, it was already very painful. But if I try again in 2024 and need to cope with the new rules added in 2023 for binding drivers which were not needed until, then it becomes even more painful. So yes, it breaks the CLI. Before: => tftp xxx => fastboot xxx just worked, after you patchset these simple operations won't work anymore. > > , making bisects more painful and all updates just fail. > > This is just utter nonsense, sorry. Well, I am a U-Boot user, and I expect this change to break a good amount of boot scripts after an update. > >> setenv ethact usb_ether > >> setenv loadaddr 0xc200 > >> setenv ipaddr 10.0.0.2 > >> setenv serverip 10.0.0.1 > >> setenv netmask 255.255.255.0 > >> tftpboot 0xc200 10.0.0.1:test.file > >> unbind /soc/usb-otg@4900 usb_ether > >> " > >> > >> Signed-off-by: Marek Vasut > >> --- > >> Cc: Kevin Hilman > >> Cc: Lukasz Majewski > >> Cc: Marek Vasut > >> Cc: Simon Glass > > > > I've tested the whole series, unfortunately is does not work on > > AM335x/BBBW: > > > > * Any recovery attempted using the network will now fail in > >the SPL, where, AFAIK, there is no way to manually bind: > > > > U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200) > > Trying to boot from USB eth > > Could not get PHY for eth_cpsw: addr 0 > > eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in > > MAC de:ad:be:ef:00:01 > > HOST MAC de:ad:be:ef:00:00 > > RNDIS ready > > , eth1: usb_ether > > It seems the RNDIS adapter is actually ready based on the above ^ ? > > > * The bind command was not available on my default configuration, > >making it even difficult for people unaware that this command is > >now required to fix their common commands. > > It has been required ever since, it's just that many ignored the need to move > over to DM and depended on board-specific hackage to set up their USB > ethernet. Now that hackage broke, time to switch to the proper way. It breaks because there is something wrong elsewhere. It's not at all the fault of the board hack (if any, I have no idea, I'll trust your judgment regarding the BBB support). My series fixes a real problem in the current code. The fact that it won't be visible after your series is al
Re: [PATCH v2 1/4] cmd: bind: Add unbind command with driver filter
Hi Tom, tr...@konsulko.com wrote on Mon, 24 Jul 2023 14:13:45 -0400: > On Sun, Jul 23, 2023 at 07:49:55PM +0200, Miquel Raynal wrote: > > Hi Marek, > > > > ma...@denx.de wrote on Mon, 17 Jul 2023 13:21:34 +0200: > > > > > Extend the driver core to perform lookup by both OF node and driver > > > bound to the node. Use this to look up specific device instances to > > > unbind from nodes in the unbind command. One example where this is > > > needed is USB peripheral controller, which may have multiple gadget > > > drivers bound to it. The unbind command has to select that specific > > > gadget driver instance to unbind from the controller, not unbind the > > > controller driver itself from the controller. > > > > > > USB ethernet gadget usage looks as follows with this change. Notice > > > the extra 'usb_ether' addition in the 'unbind' command at the end. > > > " > > > bind /soc/usb-otg@4900 usb_ether > > > > I don't really get why this is needed? Yes, having proper bind and > > unbind methods and having them called internally is relevant, but when > > you have a single OTG controller, why is this needed? It basically > > breaks the CLI, making bisects more painful and all updates just fail. > > I think part of the issue here is how usb_ether didn't act like the rest > of the gadget do, for example fastboot. It definitely is. "before it was working, now it does not anymore". That's the gut feeling many people get when the community breaks common habits. If we break something like this, we must at least be very clear on what is the new behavior. > > > setenv ethact usb_ether > > > setenv loadaddr 0xc200 > > > setenv ipaddr 10.0.0.2 > > > setenv serverip 10.0.0.1 > > > setenv netmask 255.255.255.0 > > > tftpboot 0xc200 10.0.0.1:test.file > > > unbind /soc/usb-otg@4900 usb_ether > > > " > > > > > > Signed-off-by: Marek Vasut > > > --- > > > Cc: Kevin Hilman > > > Cc: Lukasz Majewski > > > Cc: Marek Vasut > > > Cc: Simon Glass > > > > I've tested the whole series, unfortunately is does not work on > > AM335x/BBBW: > > > > * Any recovery attempted using the network will now fail in > > the SPL, where, AFAIK, there is no way to manually bind: > > > > U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200) > > Trying to boot from USB eth > > Could not get PHY for eth_cpsw: addr 0 > > eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in > > MAC de:ad:be:ef:00:01 > > HOST MAC de:ad:be:ef:00:00 > > RNDIS ready > > , eth1: usb_ether > > For testing, what happens if you disable CPSW? Does it both bind (as it > shows here) and then use the expected device? Disabling CPSW breaks the link, I had to ensure ft_board_setup in the am335x arch folder was still available (and returned 0). But once I managed to start I did not observe any improvements. U-Boot SPL 2023.07-00806-gac80e6de9cf-dirty (Jul 28 2023 - 14:49:48 +0200) Trying to boot from USB eth Could not get PHY for eth_cpsw: addr 0 eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in MAC de:ad:be:ef:00:01 HOST MAC de:ad:be:ef:00:00 RNDIS ready , eth1: usb_ether > > * The bind command was not available on my default configuration, > > making it even difficult for people unaware that this command is > > now required to fix their common commands. > > Yes, we'll need to make that default y if USB_ETHER. Agreed. > > * Any command that expects the usb_ether driver will now fail badly > > even after the bind call: > > > > => bind /ocp/usb@4740/usb@47401000 usb_ether > > => fastboot usb 0 > > couldn't find an available UDC > > g_dnl_register: failed!, error: -19 > > exit not allowed from main input shell. > > => tftp 0x8100 zImage > > dev_get_priv: null device > > Well, does it work if you do: > bind /ocp/usb@4740/usb@47401000 usb_ether > tftp 0x8100 zImage > ? No, I already tried both: U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200) Trying to boot from MMC2 U-Boot 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200) CPU : AM335X-GP rev 2.1 Model: TI AM335x BeagleBone Black DRAM: 512 MiB Core: 160 devices, 18 uclasses, devicetree: separate WDT: Started wdt@44e35000 with servicing every 1000ms (60s timeout) NAND: 0 MiB MMC: OMAP SD/MMC: 0, OMAP SD/MMC: 1 Loading Environment from FAT... Unable to read "uboot.env" from mmc1:1... not set. Validating fi
Re: [PATCH v2 1/4] cmd: bind: Add unbind command with driver filter
Hi Marek, ma...@denx.de wrote on Mon, 17 Jul 2023 13:21:34 +0200: > Extend the driver core to perform lookup by both OF node and driver > bound to the node. Use this to look up specific device instances to > unbind from nodes in the unbind command. One example where this is > needed is USB peripheral controller, which may have multiple gadget > drivers bound to it. The unbind command has to select that specific > gadget driver instance to unbind from the controller, not unbind the > controller driver itself from the controller. > > USB ethernet gadget usage looks as follows with this change. Notice > the extra 'usb_ether' addition in the 'unbind' command at the end. > " > bind /soc/usb-otg@4900 usb_ether I don't really get why this is needed? Yes, having proper bind and unbind methods and having them called internally is relevant, but when you have a single OTG controller, why is this needed? It basically breaks the CLI, making bisects more painful and all updates just fail. > setenv ethact usb_ether > setenv loadaddr 0xc200 > setenv ipaddr 10.0.0.2 > setenv serverip 10.0.0.1 > setenv netmask 255.255.255.0 > tftpboot 0xc200 10.0.0.1:test.file > unbind /soc/usb-otg@4900 usb_ether > " > > Signed-off-by: Marek Vasut > --- > Cc: Kevin Hilman > Cc: Lukasz Majewski > Cc: Marek Vasut > Cc: Simon Glass I've tested the whole series, unfortunately is does not work on AM335x/BBBW: * Any recovery attempted using the network will now fail in the SPL, where, AFAIK, there is no way to manually bind: U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200) Trying to boot from USB eth Could not get PHY for eth_cpsw: addr 0 eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in MAC de:ad:be:ef:00:01 HOST MAC de:ad:be:ef:00:00 RNDIS ready , eth1: usb_ether * The bind command was not available on my default configuration, making it even difficult for people unaware that this command is now required to fix their common commands. * Any command that expects the usb_ether driver will now fail badly even after the bind call: => bind /ocp/usb@4740/usb@47401000 usb_ether => fastboot usb 0 couldn't find an available UDC g_dnl_register: failed!, error: -19 exit not allowed from main input shell. => tftp 0x8100 zImage dev_get_priv: null device data abort pc : [<9ffa04ba>] lr : [<9ff86d5f>] reloc pc : [<8083b4ba>]lr : [<80821d5f>] sp : 9df2f920 ip : fp : 0003 r10: 9df4cf48 r9 : 9df44ea0 r8 : 9ffec33c r7 : 4a53 r6 : r5 : 0bb8 r4 : 9df4d3c0 r3 : 9ff97653 r2 : fff1102c r1 : r0 : Flags: nzcv IRQs off FIQs on Mode SVC_32 (T) Code: 9ffd b508 f7e6 fc4b (6801) 2000 Resetting CPU ... Is there anything I missed? Thanks, Miquèl
Re: [PATCH 0/2] Fix network commands w/ USB Eth gadget
Hi Tom, tr...@konsulko.com wrote on Sat, 22 Jul 2023 10:43:37 -0400: > On Sat, Jul 22, 2023 at 12:25:35AM +0200, Miquel Raynal wrote: > > > Hello, > > > > I recently came across serious issues using U-Boot on Beagle Bone > > Black. The USB Ethernet gadget is behaving in a way that is not > > compliant with the uclass expectations, leading to use-after-free > > accesses often producing data aborts. All network commands are > > affected. > > > > There are two problems: > > * Any network command after completion could produce a data abort > > * A tftp retrieval with a wrong file name would produce a data abort > > > > Here is how the major issue (the former one) looks like: > > > > => tftp 0x8100 zImage > > using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in > > MAC f8:dc:7a:00:00:02 > > HOST MAC f8:dc:7a:00:00:01 > > RNDIS ready > > musb-hdrc: peripheral reset irq lost! > > high speed config #2: 2 mA, Ethernet Gadget, using RNDIS > > USB RNDIS network up! > > Using usb_ether device > > TFTP from server 192.168.0.1; our IP address is 192.168.0.100 > > Filename 'zImage'. > > Load address: 0x8100 > > Loading: ## 13 MiB > > 4.2 MiB/s > > done > > Bytes transferred = 13634360 (d00b38 hex) > > data abort > > pc : [<9ff80fba>] lr : [<9ff7abd9>] > > reloc pc : [<8081bfba>]lr : [<80815bd9>] > > sp : 9df2f9f8 ip : 0020 fp : 0003 > > r10: 0200 r9 : 9df44ea0 r8 : 9df2fa68 > > r7 : 9df2fa68 r6 : 9ffdbabc r5 : 9ffcdbcd r4 : 0018 > > r3 : 0018 r2 : 9ffdba00 r1 : 0001 r0 : 9df4d348 > > Flags: Nzcv IRQs off FIQs on Mode SVC_32 (T) > > Code: 68c2 6881 f023 0303 (60ca) 4403 > > Resetting CPU ... > > > > While debugging this issue, I came across Qianfan's bug report which > > raised this issue one year ago. Qianfan nicely pointed at two of his > > patches sent on the mailing list following his investigations, which > > IMHO got refused for a wrong reason. > > > > Link: > > https://lore.kernel.org/all/7536b9e1-de7a-a492-6951-485d4eb75...@163.com/ > > Link: > > https://patchwork.ozlabs.org/project/uboot/patch/20220402025836.19374-1-qianfangui...@163.com/ > > Link: > > https://patchwork.ozlabs.org/project/uboot/patch/20220402025836.19374-2-qianfangui...@163.com/ > > > > I've taken over Qianfan's two patches, I took the liberty to explain a > > bit more what these issues were about and why they were serious, > > rewording his first patch, and trying to fix the second issue > > differently, because I believe the second issue should be avoided rather > > than workarounded. > > > > Once ready to send this series, I noticed that two other people already > > tried to fix this: > > Link: > > https://lore.kernel.org/all/20221212204411.2247170-1-b...@baylibre.com/ > > Link: https://lists.denx.de/pipermail/u-boot/2022-December/502055.html > > > > I have no idea why this is still an open issue, I hope the "code > > reorganization" reason that was mentioned in one of the above threads > > does not stand anymore given how serious these issues are, so whatever > > solution is preferred, I hope one will soon be picked-up :-) > > So, the original series was also the wrong direction for the problem, I don't know if it was taking the wrong direction, nothing was strongly bothering me. As Qianfan's proposal was not fitting the maintainer's whishes I tried to think again about it in order to propose something slightly different. > overall. Fortunately Marek was able to address the problem quite > recently: > https://patchwork.ozlabs.org/project/uboot/list/?series=364209=* > > Please test that series, thanks. I don't understand why the different approaches have been so quickly discarded. They participate to make U-Boot more stable, now. Already 4 major releases have been tagged since these bugs were reported. To my eyes we should all focus on getting issues fixed. Of course discussing the fixes is part of the game but this has been "under discussion" for a year, and yet this patchset is immediately rejected. Marek's proposal, besides making total sense, has a real impact, it needs to be discussed and tested (sorry, but it breaks user habits, SPL in recovery modes, and all network commands), so why not taking trivial patches with little-to-no side effects like these and revert them when they are no longer relevant? Thanks, Miquèl
Re: data abort when run 'dhcp'
Hi qianfan, qianfangui...@163.com wrote on Fri, 21 Jul 2023 08:31:17 +0800: > 在 2023/7/21 0:39, Miquel Raynal 写道: > > Hello, > > > > qianfangui...@163.com wrote on Fri, 25 Mar 2022 18:04:46 +0800: > > > >> It's very strange. And I can't detect it's a bug of usb or dlmalloc. > >> > >> 1. Starting u-boot and dhcp via am335x's ethernet(cpsw driver), it's ok. > >> > >> 2. Starting u-boot and dhcp via am335x's usb net, data abort. > >> > >> 3. start fastboot, and CTRL C right now, dhcp via am335x's usb net, it's > >> ok. > > I am sorry to re-open a thread that is one year old but this is > > still an open bug. The BBB is affected. In particular the BBBW > > because there is no Ethernet connector, which makes the Eth-over-USB > > emulation even more important. All U-Boots since 2021 are affected: > > spurious data aborts, usually at the end of network interactions (tftp, > > ping). I could not bisect it because the boot was deeply broken as > > well on a significant range of commits :-/. > > > > On my side I narrowed it down to an env update which fails in malloc as > > well. If I comment the env update, it fails a bit later. It really > > looks like a stack corruption which is either related to the Ethernet > > USB gadget or the USB controller driver itself. Network transfers on > > the BBBW using regular Ethernet does not trigger any error. > > > > I also observe the very strange "fix" mentioned above: starting and > > killing fastboot makes all tftp pass... If anyone has more details to > > share, or perhaps a subsequent thread giving more details, I would > > really like to see this fixed upstream, I suppose I am not the only one > > :-) > Hi: > > Could you please try this two patches? > > http://patchwork.ozlabs.org/project/uboot/patch/20220402025836.19374-1-qianfangui...@163.com/ > > http://patchwork.ozlabs.org/project/uboot/patch/20220402025836.19374-2-qianfangui...@163.com/ Indeed these patches work. I ended up rewriting one of them to propose a different approach. I also found two other proposals for the same issue which are still pending around. I hope this submission will make it to avoid more time to be spent on this :-) Thanks a lot for the pointers, I've Cc'ed you on the submissions. Kind regards, Miquèl
[PATCH 2/2] net: tftp: Prevent too early device removal leading to data aborts
All network related commands are executed through a "net loop", which
handles the initialization, setup and removal of all the required
resources to perform the operation.
Aside from the generic network boilerplate, the net loop calls specific
helpers provided by the different command implementations. The tftp
implementation, among others, registers his own handler with
net_set_udp_handler() in order to catch incoming UDP packets. In this
handler, there are several calls to eth_halt(). Some of them might be
legitimate, but one that is certainly not is the one in the switch case
handling the "access denied" and "file not found errors".
When these error happens, there is nothing wrong with the network
interface but the protocol itself returned an error condition. There is
no reason to immediately halt the Ethernet interface, because the
handler will propagate an error code which the net loop will process, up
to the point where it will halt and remove the Ethernet interface
itself. Said otherwise, halting the Ethernet interface (and at the same
time removing all structures related to it) in the tftp handler bypasses
the net loop.
This would not cause any harm if halting the interface would not
sometimes lead to its entire removal. That is indeed what happens with
the Ethernet USB gadget implementation. In this driver, the ->start()
and ->stop() callbacks are responsible for creating and removing a
virtual Ethernet interface, which means after the ->stop() call, any
structure used to define that interface shall no longer be used. The
eth_halt() call stopping (and removing) the interface in the handler,
would make the net loop perform illegal accesses over freed buffers,
overwriting some content in the heap. These overwrites have been
identified to later lead to serious data aborts within malloc()
execution.
Let's just drop this eth_alt() call which is happening too early, and
is redundant anyway.
Suggested-by: Qianfan Zhao
Signed-off-by: Miquel Raynal
---
net/tftp.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/tftp.c b/net/tftp.c
index 88e71e67de3..ac20a60d222 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -680,7 +680,6 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct
in_addr sip,
case TFTP_ERR_FILE_NOT_FOUND:
case TFTP_ERR_ACCESS_DENIED:
puts("Not retrying...\n");
- eth_halt();
net_set_state(NETLOOP_FAIL);
break;
case TFTP_ERR_UNDEFINED:
--
2.34.1
[PATCH 1/2] net: eth-uclass: Prevent data aborts with the Ethernet USB gadget
From: Qianfan Zhao
The Ethernet USB gadget driver creates a virtual Ethernet interface on
the fly in the ->start() callback. This means, there is no other place
than the ->stop() callback to unregister the interface and free all the
associated structures. Deleting the interface frees the 'priv'
pointer. Unfortunately, the current code does not expect the 'priv'
pointer to disappear and tries to update its internal fields to mention
the interface is no longer running, which is obviously
wrong. Dereferencing a pointer and writing where it leads while the
buffer has been freed means we probably overwrite some heap data.
The fact is, this bug is reproducible since at least 2021, but the
occurrence of the data abort that is associated to it is changing over
the releases and the toolchains. This is probably due to the
use-after-free either touching a heap buffer or some heap meta-data. In
the former case, it does not trigger any failure, besides randomly
smashing some data. In the latter case, it totally breaks malloc
implementation which then processes totally broken data and likely
produces these data aborts.
On an am335x (Beagle Bone Black Wireless) with a GCC 10 toolchain, the
bug produces a data abort every 4 to 8 tftp downloads. With a GCC 11
toolchain, the crash happens at every try. In practice, all network
commands are affected, eg. dhcp and ping also lead to these data aborts,
which happen later in the code and are all pointing at "malloc".
Signed-off-by: Qianfan Zhao
[Miquel Raynal: Write a new commit message to argue why this fix is important]
Signed-off-by: Miquel Raynal
---
net/eth-uclass.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index b01a910938e..bfb092b86f9 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -354,8 +354,18 @@ void eth_halt(void)
return;
eth_get_ops(current)->stop(current);
- priv->state = ETH_STATE_PASSIVE;
- priv->running = false;
+
+ /* The Ethernet USB gadget driver creates a virtual Ethernet interface
+* on the fly in the ->start() callback, which means the ->stop()
+* callbacks deletes it. Deleting the interface frees the 'priv'
+* structure which shall no longer be dereferenced in this case. Ensure
+* 'priv' is still valid after ->stop(), before updating its fields.
+*/
+ priv = dev_get_uclass_priv(current);
+ if (priv) {
+ priv->state = ETH_STATE_PASSIVE;
+ priv->running = false;
+ }
}
int eth_is_active(struct udevice *dev)
--
2.34.1
[PATCH 0/2] Fix network commands w/ USB Eth gadget
Hello, I recently came across serious issues using U-Boot on Beagle Bone Black. The USB Ethernet gadget is behaving in a way that is not compliant with the uclass expectations, leading to use-after-free accesses often producing data aborts. All network commands are affected. There are two problems: * Any network command after completion could produce a data abort * A tftp retrieval with a wrong file name would produce a data abort Here is how the major issue (the former one) looks like: => tftp 0x8100 zImage using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in MAC f8:dc:7a:00:00:02 HOST MAC f8:dc:7a:00:00:01 RNDIS ready musb-hdrc: peripheral reset irq lost! high speed config #2: 2 mA, Ethernet Gadget, using RNDIS USB RNDIS network up! Using usb_ether device TFTP from server 192.168.0.1; our IP address is 192.168.0.100 Filename 'zImage'. Load address: 0x8100 Loading: ## 13 MiB 4.2 MiB/s done Bytes transferred = 13634360 (d00b38 hex) data abort pc : [<9ff80fba>] lr : [<9ff7abd9>] reloc pc : [<8081bfba>]lr : [<80815bd9>] sp : 9df2f9f8 ip : 0020 fp : 0003 r10: 0200 r9 : 9df44ea0 r8 : 9df2fa68 r7 : 9df2fa68 r6 : 9ffdbabc r5 : 9ffcdbcd r4 : 0018 r3 : 0018 r2 : 9ffdba00 r1 : 0001 r0 : 9df4d348 Flags: Nzcv IRQs off FIQs on Mode SVC_32 (T) Code: 68c2 6881 f023 0303 (60ca) 4403 Resetting CPU ... While debugging this issue, I came across Qianfan's bug report which raised this issue one year ago. Qianfan nicely pointed at two of his patches sent on the mailing list following his investigations, which IMHO got refused for a wrong reason. Link: https://lore.kernel.org/all/7536b9e1-de7a-a492-6951-485d4eb75...@163.com/ Link: https://patchwork.ozlabs.org/project/uboot/patch/20220402025836.19374-1-qianfangui...@163.com/ Link: https://patchwork.ozlabs.org/project/uboot/patch/20220402025836.19374-2-qianfangui...@163.com/ I've taken over Qianfan's two patches, I took the liberty to explain a bit more what these issues were about and why they were serious, rewording his first patch, and trying to fix the second issue differently, because I believe the second issue should be avoided rather than workarounded. Once ready to send this series, I noticed that two other people already tried to fix this: Link: https://lore.kernel.org/all/20221212204411.2247170-1-b...@baylibre.com/ Link: https://lists.denx.de/pipermail/u-boot/2022-December/502055.html I have no idea why this is still an open issue, I hope the "code reorganization" reason that was mentioned in one of the above threads does not stand anymore given how serious these issues are, so whatever solution is preferred, I hope one will soon be picked-up :-) Thanks, Miquèl Miquel Raynal (1): net: tftp: Prevent too early device removal leading to data aborts Qianfan Zhao (1): net: eth-uclass: Prevent data aborts with the Ethernet USB gadget net/eth-uclass.c | 14 -- net/tftp.c | 1 - 2 files changed, 12 insertions(+), 3 deletions(-) -- 2.34.1
Re: data abort when run 'dhcp'
Hi Tom, tr...@konsulko.com wrote on Thu, 20 Jul 2023 14:34:52 -0400: > On Thu, Jul 20, 2023 at 06:39:17PM +0200, Miquel Raynal wrote: > > Hello, > > > > qianfangui...@163.com wrote on Fri, 25 Mar 2022 18:04:46 +0800: > > > > > It's very strange. And I can't detect it's a bug of usb or dlmalloc. > > > > > > 1. Starting u-boot and dhcp via am335x's ethernet(cpsw driver), it's ok. > > > > > > 2. Starting u-boot and dhcp via am335x's usb net, data abort. > > > > > > 3. start fastboot, and CTRL C right now, dhcp via am335x's usb net, it's > > > ok. > > > > I am sorry to re-open a thread that is one year old but this is > > still an open bug. The BBB is affected. In particular the BBBW > > because there is no Ethernet connector, which makes the Eth-over-USB > > emulation even more important. All U-Boots since 2021 are affected: > > spurious data aborts, usually at the end of network interactions (tftp, > > ping). I could not bisect it because the boot was deeply broken as > > well on a significant range of commits :-/. > > > > On my side I narrowed it down to an env update which fails in malloc as > > well. If I comment the env update, it fails a bit later. It really > > looks like a stack corruption which is either related to the Ethernet > > USB gadget or the USB controller driver itself. Network transfers on > > the BBBW using regular Ethernet does not trigger any error. > > > > I also observe the very strange "fix" mentioned above: starting and > > killing fastboot makes all tftp pass... If anyone has more details to > > share, or perhaps a subsequent thread giving more details, I would > > really like to see this fixed upstream, I suppose I am not the only one > > :-) > > What happens if you increase the malloc pool from say 32MB (current > value, 0x200) to 64MB (so 0x400) ? Same result. I tried to increment the heap size to 64MB as well as the stack size (16 -> 64MB), same behavior. Thanks, Miquèl
Re: data abort when run 'dhcp'
Hi Heinrich, xypron.g...@gmx.de wrote on Thu, 20 Jul 2023 19:55:39 +0200: > Am 20. Juli 2023 18:39:17 MESZ schrieb Miquel Raynal > : > >Hello, > > > >qianfangui...@163.com wrote on Fri, 25 Mar 2022 18:04:46 +0800: > > > >> It's very strange. And I can't detect it's a bug of usb or dlmalloc. > >> > >> 1. Starting u-boot and dhcp via am335x's ethernet(cpsw driver), it's ok. > >> > >> 2. Starting u-boot and dhcp via am335x's usb net, data abort. > >> > >> 3. start fastboot, and CTRL C right now, dhcp via am335x's usb net, it's > >> ok. > > > >I am sorry to re-open a thread that is one year old but this is > >still an open bug. The BBB is affected. In particular the BBBW > >because there is no Ethernet connector, which makes the Eth-over-USB > >emulation even more important. All U-Boots since 2021 are affected: > >spurious data aborts, usually at the end of network interactions (tftp, > >ping). I could not bisect it because the boot was deeply broken as > >well on a significant range of commits :-/. > > > >On my side I narrowed it down to an env update which fails in malloc as > >well. If I comment the env update, it fails a bit later. It really > >looks like a stack corruption which is either related to the Ethernet > >USB gadget or the USB controller driver itself. Network transfers on > >the BBBW using regular Ethernet does not trigger any error. > > > >I also observe the very strange "fix" mentioned above: starting and > >killing fastboot makes all tftp pass... If anyone has more details to > >share, or perhaps a subsequent thread giving more details, I would > >really like to see this fixed upstream, I suppose I am not the only one > >:-) > > > >Thanks, > >Miquèl > > > Can this problem be reproduced on QEMU? I haven't tried on QEMU, what do you have in mind? What should we try to do? Thanks, Miquèl > > Best regards > > Heinrich > > > > >> > >> 在 2022/3/24 17:33, qianfan 写道: > >> > > >> > 在 2022/3/23 18:12, Heinrich Schuchardt 写道: > >> >> On 3/23/22 11:07, qianfan wrote: > >> >>> > >> >>> 在 2022/3/23 17:51, Heinrich Schuchardt 写道: > >> >>>> On 3/23/22 10:13, qianfan wrote: > >> >>>>> > >> >>>>> 在 2022/3/23 16:02, qianfan 写道: > >> >>>>>> > >> >>>>>> > >> >>>>>> 在 2022/3/23 15:45, qianfan 写道: > >> >>>>>>> > >> >>>>>>> > >> >>>>>>> 在 2022/3/23 10:28, qianfan 写道: > >> >>>>>>>> > >> >>>>>>>> Hi: > >> >>>>>>>> > >> >>>>>>>> I had a custom AM335X board connected my computer by usbnet. It > >> >>>>>>>> always report data abort when 'dhcp': > >> >>>>>>>> > >> >>>>>>>> Next it the log: > >> >>>>>>>> > >> >>>>>>>> U-Boot 2022.01-rc1-00183-gfa5b4e2d19-dirty (Feb 25 2022 - 15:45:02 > >> >>>>>>>> +0800) > >> >>>>>>>> > >> >>>>>>>> CPU : AM335X-GP rev 2.1 > >> >>>>>>>> Model: WISDOM AM335X CCT > >> >>>>>>>> DRAM: 512 MiB > >> >>>>>>>> NAND: 256 MiB > >> >>>>>>>> MMC: OMAP SD/MMC: 0 > >> >>>>>>>> Loading Environment from NAND... *** Warning - bad CRC, using > >> >>>>>>>> default environment > >> >>>>>>>> > >> >>>>>>>> Net: Could not get PHY for ethernet@4a10: addr 0 > >> >>>>>>>> eth2: ethernet@4a10, eth3: usb_ether > >> >>>>>>>> Hit any key to stop autoboot: 0 > >> >>>>>>>> => setenv autoload no > >> >>>>>>>> => dhcp > >> >>>>>>>> using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in > >> >>>>>>>> MAC de:ad:be:ef:00:01 > >> >>>>>>>> HOST MAC de:ad:be:ef:00:00 > >> >>>>>>>> RNDIS ready > >> >>>>>>>> m
Re: data abort when run 'dhcp'
Hello, qianfangui...@163.com wrote on Fri, 25 Mar 2022 18:04:46 +0800: > It's very strange. And I can't detect it's a bug of usb or dlmalloc. > > 1. Starting u-boot and dhcp via am335x's ethernet(cpsw driver), it's ok. > > 2. Starting u-boot and dhcp via am335x's usb net, data abort. > > 3. start fastboot, and CTRL C right now, dhcp via am335x's usb net, it's ok. I am sorry to re-open a thread that is one year old but this is still an open bug. The BBB is affected. In particular the BBBW because there is no Ethernet connector, which makes the Eth-over-USB emulation even more important. All U-Boots since 2021 are affected: spurious data aborts, usually at the end of network interactions (tftp, ping). I could not bisect it because the boot was deeply broken as well on a significant range of commits :-/. On my side I narrowed it down to an env update which fails in malloc as well. If I comment the env update, it fails a bit later. It really looks like a stack corruption which is either related to the Ethernet USB gadget or the USB controller driver itself. Network transfers on the BBBW using regular Ethernet does not trigger any error. I also observe the very strange "fix" mentioned above: starting and killing fastboot makes all tftp pass... If anyone has more details to share, or perhaps a subsequent thread giving more details, I would really like to see this fixed upstream, I suppose I am not the only one :-) Thanks, Miquèl > > 在 2022/3/24 17:33, qianfan 写道: > > > > 在 2022/3/23 18:12, Heinrich Schuchardt 写道: > >> On 3/23/22 11:07, qianfan wrote: > >>> > >>> 在 2022/3/23 17:51, Heinrich Schuchardt 写道: > On 3/23/22 10:13, qianfan wrote: > > > > 在 2022/3/23 16:02, qianfan 写道: > >> > >> > >> 在 2022/3/23 15:45, qianfan 写道: > >>> > >>> > >>> 在 2022/3/23 10:28, qianfan 写道: > > Hi: > > I had a custom AM335X board connected my computer by usbnet. It > always report data abort when 'dhcp': > > Next it the log: > > U-Boot 2022.01-rc1-00183-gfa5b4e2d19-dirty (Feb 25 2022 - 15:45:02 > +0800) > > CPU : AM335X-GP rev 2.1 > Model: WISDOM AM335X CCT > DRAM: 512 MiB > NAND: 256 MiB > MMC: OMAP SD/MMC: 0 > Loading Environment from NAND... *** Warning - bad CRC, using > default environment > > Net: Could not get PHY for ethernet@4a10: addr 0 > eth2: ethernet@4a10, eth3: usb_ether > Hit any key to stop autoboot: 0 > => setenv autoload no > => dhcp > using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in > MAC de:ad:be:ef:00:01 > HOST MAC de:ad:be:ef:00:00 > RNDIS ready > musb-hdrc: peripheral reset irq lost! > high speed config #2: 2 mA, Ethernet Gadget, using RNDIS > USB RNDIS network up! > BOOTP broadcast 1 > BOOTP broadcast 2 > BOOTP broadcast 3 > DHCP client bound to address 192.168.200.4 (757 ms) > data abort > pc : [<9fe9b0a2>] lr : [<9febbc3f>] > reloc pc : [<808130a2>] lr : [<80833c3f>] > sp : 9de53410 ip : 9de53578 fp : 0001 > r10: 9de5345c r9 : 9de67e80 r8 : 9febbae5 > r7 : 9de72c30 r6 : 9feec710 r5 : 000d r4 : 0018 > r3 : 3fdd8e04 r2 : 0002 r1 : 9feec728 r0 : 9feec700 > Flags: Nzcv IRQs off FIQs on Mode SVC_32 (T) > Code: f023 0303 60ca 4403 (6091) 685a > Resetting CPU ... > > resetting ... > > > It's there has any doc about how to debug data abort? Or is the bug > is already fixed? > > Thanks > > >>> This bug doesn't fixed on master code. I found v2021.01 is good and > >>> v2021.04-rc2 is bad. > >>> > >>> Also I had tested this on beaglebone black with am335x_evm_defconfig, > >>> has the simliar problem. > >>> > >>> find the first bug commit via 'git bisect': it told me that commit > >>> e97eb638de0dc8f6e989e20eaeb0342f103cb917 broke it. But it is very > >>> strange due to this commit doesn't touch any dhcp or network code. > >>> > >>> ➜ u-boot-main git:(e97eb638de) ✗ git bisect bug > >>> e97eb638de0dc8f6e989e20eaeb0342f103cb917 is the first bug commit > >>> commit e97eb638de0dc8f6e989e20eaeb0342f103cb917 > >>> Author: Heinrich Schuchardt > >>> Date: Wed Jan 20 22:21:53 2021 +0100 > >>> > >>> fs: fat: consistent error handling for flush_dir() > >>> > >>> Provide function description for flush_dir(). > >>> Move all error messages for flush_dir() from the callers to the > >>> function. > >>> Move mapping of errors to -EIO to the function. > >>> Always check return value of flush_dir() (Coverity CID
Re: [PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer
Hello, ra...@milecki.pl wrote on Thu, 09 Mar 2023 12:52:37 +0100: > On 2023-03-09 12:44, Srinivas Kandagatla wrote: > > On 09/03/2023 11:23, Miquel Raynal wrote: > >> Hi Srinivas, > >> >> srinivas.kandaga...@linaro.org wrote on Thu, 9 Mar 2023 10:53:07 >> > >> >> +: > >> >>> On 09/03/2023 10:32, Miquel Raynal wrote: > >>>> Hi Srinivas, > >>>> >>>> srinivas.kandaga...@linaro.org wrote on Thu, 9 Mar 2023 10:12:24 > >>>> >>>> >>>> +: > >>>> >>>>> On 22/02/2023 17:22, Rafał Miłecki wrote: > >>>>>> @@ -1791,11 +1792,15 @@ ssize_t nvmem_device_cell_read(struct >>>>>> > >>>>>> nvmem_device *nvmem, > >>>>>>if (!nvmem) > >>>>>>return -EINVAL; > >>>>>> > +/* Cells with read_post_process hook may realloc buffer > >>>>>> we >>>>>> can't allow here */ > >>>>>> + if (info->read_post_process) > >>>>>> + return -EINVAL; > >>>>> This should probably go in 1/4 patch. Other than that series looks > >>>>> >>>>> good to me. > >>>> >>>> FYI patch 1/4 is also carried by the nvmem-layouts series, so it's > >>>> probably best to keep these 2 patches separated to simplify the >>>> > >>>> merging. > >>> that is intermediate thing, but Ideally this change belongs to 1/4 >>> > >>> patch, so once I apply these patches then we can always rebase layout >>> > >>> series on top of nvmem-next > >> >> Well, I still don't see the need for this patch because we have no use > >> for it *after* the introduction of layouts. Yes in some cases changing > >> the size of a cell might maybe be needed, but right now the use case >> is > >> to provide a MAC address, we know beforehand the size of the cell, so > >> there is no need, currently, for this hack. > >> > Am confused, should I ignore this series ? I think this series makes sense and addresses a need. But this issue can also be solved with the layouts. Rafał does not want (I still don't get the reason) to use that solution. Whatever. But if you apply this series, it requires to modify the layouts series, thus postponing it even more. I would prefer to merge that big series first and then merge an update of this patch (which changes in the two layout drivers the cell size argument type). > I'm confused no less. > > I think we have 3 different opinions and no agreement on how to proceed. > > > Rafał (me): > NVMEM cells should be registered as they are in the raw format. No size > adjustments should happen while registering them. If NVMEM cell requires > some read post-processing then its size should be adjusted *while* > reading. This implementation only works if you reduce the size of the cell. While writing this, I am realizing that we would actually expect a check on the nvmem side if the size was enlarged because this would be a bug. > Michael: > .read_post_process() should be realloc the buffer This would be more robust. But if we start with 1, we can improve it later, I don't mind as long as an error is returned in case of misuse. > Miquel: > While registering NVMEM cell its size should be already adjusted to > match what .read_post_process() is about to return. Sounds like the simplest solution to me and covers all the uses we have to day, but honestly, I won't fight for it. > I'm really sorry if I got anyone's view wrong. LGTM. > > Whatever. If you want it, just merge it. But *please*, I would like > > :-) > > > to see these layouts in, so what's the plan? > > Am on it, you sent v3 just 24hrs ago :-) Yes, sorry for being pushy. I just wanted to highlight that the two series conflict together, but my answer was clumsy. Take the time you need, that's how it's supposed to work anyway. Thanks, Miquèl
Re: [PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer
Hi Srinivas, srinivas.kandaga...@linaro.org wrote on Thu, 9 Mar 2023 10:53:07 +: > On 09/03/2023 10:32, Miquel Raynal wrote: > > Hi Srinivas, > > > > srinivas.kandaga...@linaro.org wrote on Thu, 9 Mar 2023 10:12:24 +: > > > >> On 22/02/2023 17:22, Rafał Miłecki wrote: > >>> @@ -1791,11 +1792,15 @@ ssize_t nvmem_device_cell_read(struct > >>> nvmem_device *nvmem, > >>> if (!nvmem) > >>> return -EINVAL; > >>>> +/* Cells with read_post_process hook may realloc buffer we > >>> can't allow here */ > >>> + if (info->read_post_process) > >>> + return -EINVAL; > >> This should probably go in 1/4 patch. Other than that series looks good to > >> me. > > > > FYI patch 1/4 is also carried by the nvmem-layouts series, so it's > > probably best to keep these 2 patches separated to simplify the merging. > that is intermediate thing, but Ideally this change belongs to 1/4 patch, so > once I apply these patches then we can always rebase layout series on top of > nvmem-next Well, I still don't see the need for this patch because we have no use for it *after* the introduction of layouts. Yes in some cases changing the size of a cell might maybe be needed, but right now the use case is to provide a MAC address, we know beforehand the size of the cell, so there is no need, currently, for this hack. Whatever. If you want it, just merge it. But *please*, I would like to see these layouts in, so what's the plan? Thanks, Miquèl
Re: [PATCH 2/4] nvmem: core: allow nvmem_cell_post_process_t callbacks to adjust buffer
Hi Srinivas, srinivas.kandaga...@linaro.org wrote on Thu, 9 Mar 2023 10:12:24 +: > On 22/02/2023 17:22, Rafał Miłecki wrote: > > @@ -1791,11 +1792,15 @@ ssize_t nvmem_device_cell_read(struct nvmem_device > > *nvmem, > > if (!nvmem) > > return -EINVAL; > > > + /* Cells with read_post_process hook may realloc buffer we > > can't allow here */ > > + if (info->read_post_process) > > + return -EINVAL; > This should probably go in 1/4 patch. Other than that series looks good to me. FYI patch 1/4 is also carried by the nvmem-layouts series, so it's probably best to keep these 2 patches separated to simplify the merging. Thanks, Miquèl
Re: [RFC PATCH v3 3/9] clk: renesas: add R906G032 driver
Hi Marek, marek.va...@mailbox.org wrote on Thu, 23 Feb 2023 14:56:41 +0100: > On 2/23/23 08:17, Miquel Raynal wrote: > > Hello Marek & Ralph, > > > > marek.va...@mailbox.org wrote on Thu, 23 Feb 2023 01:12:49 +0100: > > > >> On 2/22/23 20:32, Ralph Siemsen wrote: > >>> On Wed, Feb 22, 2023 at 07:45:45PM +0100, Marek Vasut wrote: > >>>> On 2/22/23 19:32, Ralph Siemsen wrote: > >>>>> On Wed, Feb 22, 2023 at 06:47:44PM +0100, Marek Vasut wrote: > >>>>>> Are those fixes in mainline Linux ? > >>>>> > >>>>> Yes, they are in mainline: > >>>>> 2dee50ab9e72 clk: renesas: r9a06g032: Fix UART clkgrp bitsel > >>>>> merged into 6.0, and also backported to earlier LTS branches > >>>>> 2a6da4a11f47 clk: renesas: r9a06g032: Fix the RTC hclock description > >>>>> merged into 5.19, seems to be missing from LTS branches > >>>> > >>>> Use Linux 6.2.y as a base then. > >>> > >>> Okay, done. > >>>>>>> And please submit the missing patches for LTS branch inclusion > >>> too if >> possible, I guess they were missing Fixes: tag ? > >>> > >>> Seems it was part of a series which added support for the RTC device: > >>> https://lore.kernel.org/all/20220421090016.79517-3-miquel.ray...@bootlin.com/ > >>> Since it is new feature, I guess one could argue that the clock table > > >>> fix is not needed in older LTS kernels, since no driver uses that clock. > >>> But most likely it was just overlooked. I'll check with Miquel. > >> > >> Much appreciated, thanks ! > >> > >> +CC Miquel > > > > As Ralph rightly pointed out, even though the clock description was > > wrong there was no user at that time so I did not bother with a Fixes > > tag. Anyhow, as the change does only impact the RTC clock, it should be > > harmless to backport if you want. > > Either way is fine by me, I just want to be sure the u-boot clock tables are > in sync with Linux as much as possible, and can be easily resynced in the > future, that's all. Of course. The fix is mainline already, so on that regard we should be fine. Thanks, Miquèl
Re: [RFC PATCH v3 3/9] clk: renesas: add R906G032 driver
Hello Marek & Ralph, marek.va...@mailbox.org wrote on Thu, 23 Feb 2023 01:12:49 +0100: > On 2/22/23 20:32, Ralph Siemsen wrote: > > On Wed, Feb 22, 2023 at 07:45:45PM +0100, Marek Vasut wrote: > >> On 2/22/23 19:32, Ralph Siemsen wrote: > >>> On Wed, Feb 22, 2023 at 06:47:44PM +0100, Marek Vasut wrote: > Are those fixes in mainline Linux ? > >>> > >>> Yes, they are in mainline: > >>> 2dee50ab9e72 clk: renesas: r9a06g032: Fix UART clkgrp bitsel > >>> merged into 6.0, and also backported to earlier LTS branches > >>> 2a6da4a11f47 clk: renesas: r9a06g032: Fix the RTC hclock description > >>> merged into 5.19, seems to be missing from LTS branches > >> > >> Use Linux 6.2.y as a base then. > > > > Okay, done. > > > >> And please submit the missing patches for LTS branch inclusion too if >> > >> possible, I guess they were missing Fixes: tag ? > > > > Seems it was part of a series which added support for the RTC device: > > https://lore.kernel.org/all/20220421090016.79517-3-miquel.ray...@bootlin.com/ > > Since it is new feature, I guess one could argue that the clock table > fix > > is not needed in older LTS kernels, since no driver uses that clock. > > But most likely it was just overlooked. I'll check with Miquel. > > Much appreciated, thanks ! > > +CC Miquel As Ralph rightly pointed out, even though the clock description was wrong there was no user at that time so I did not bother with a Fixes tag. Anyhow, as the change does only impact the RTC clock, it should be harmless to backport if you want. Thanks, Miquèl
Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0
On Tue, 2023-01-24 at 10:44:44 UTC, Francesco Dolcini wrote: > From: Francesco Dolcini > > Add a mechanism to handle the case in which partitions are present as > direct child of the nand controller node and #size-cells is set to <0>. > > This could happen if the nand-controller node in the DTS is supposed to > have #size-cells set to 0, but for some historical reason/bug it was set > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition > as direct children of the nand-controller defaulting to #size-cells > being to 1. > > This prevents a real boot failure on colibri-imx7 that happened during v6.1 > development cycles. > > Link: > https://lore.kernel.org/all/y4dgbtgnwpm6s...@francesco-nb.int.toradex.com/ > Link: > https://lore.kernel.org/all/20221202071900.1143950-1-france...@dolcini.it/ > Signed-off-by: Francesco Dolcini > Reviewed-by: Greg Kroah-Hartman Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0
Hi Francesco, france...@dolcini.it wrote on Fri, 03 Feb 2023 19:03:27 +0100: > Il 3 febbraio 2023 16:12:02 CET, Miquel Raynal ha > scritto: > >Hi Francesco, > > > >france...@dolcini.it wrote on Thu, 2 Feb 2023 12:33:34 +0100: > > > >> On Thu, Jan 26, 2023 at 10:12:04AM +0100, Miquel Raynal wrote: > >> > gre...@linuxfoundation.org wrote on Thu, 26 Jan 2023 10:01:02 +0100: > >> > > >> > > On Wed, Jan 25, 2023 at 10:06:57PM +0100, Francesco Dolcini wrote: > >> > > > Hello Miquel, Greg and all > >> > > > > >> > > > On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote: > >> > > > > >> > > > > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote: > >> > > > > > >> > > > > > From: Francesco Dolcini > >> > > > > > > >> > > > > > Add a mechanism to handle the case in which partitions are > >> > > > > > present as > >> > > > > > direct child of the nand controller node and #size-cells is set > >> > > > > > to <0>. > >> > > > > > > >> > > > > > This could happen if the nand-controller node in the DTS is > >> > > > > > supposed to > >> > > > > > have #size-cells set to 0, but for some historical reason/bug it > >> > > > > > was set > >> > > > > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the > >> > > > > > partition > >> > > > > > as direct children of the nand-controller defaulting to > >> > > > > > #size-cells > >> > > > > > being to 1. > >> > > > > > > >> > > > > > This prevents a real boot failure on colibri-imx7 that happened > >> > > > > > during v6.1 > >> > > > > > development cycles. > >> > > > > > > >> > > > > > Link: > >> > > > > > https://lore.kernel.org/all/y4dgbtgnwpm6s...@francesco-nb.int.toradex.com/ > >> > > > > > Link: > >> > > > > > https://lore.kernel.org/all/20221202071900.1143950-1-france...@dolcini.it/ > >> > > > > > Signed-off-by: Francesco Dolcini > >> > > > > > Reviewed-by: Greg Kroah-Hartman > >> > > > > > --- > >> > > > > > I do not expect this patch to be backported to stable, however I > >> > > > > > would expect > >> > > > > > that we do not backport nand-controller dts cleanups neither. > >> > > > > > > >> > > > > > v4: > >> > > > > > fixed wrong English spelling in the comment > >> > > > > > > >> > > > > > v3: > >> > > > > > minor formatting change, removed not needed new-line and space. > >> > > > > > > >> > > > > > v2: > >> > > > > > fixup size-cells only when partitions are direct children of > >> > > > > > the nand-controller > >> > > > > > completely revised commit message, comments and warning print > >> > > > > > use pr_warn instead of pr_warn_once > >> > > > > > added Reviewed-by Greg > >> > > > > > removed cc:stable@ and fixes tag, since the problematic commit > >> > > > > > was reverted > >> > > > > > --- > >> > > > > > drivers/mtd/parsers/ofpart_core.c | 19 +++ > >> > > > > > 1 file changed, 19 insertions(+) > >> > > > > > > >> > > > > > diff --git a/drivers/mtd/parsers/ofpart_core.c > >> > > > > > b/drivers/mtd/parsers/ofpart_core.c > >> > > > > > index 192190c42fc8..e7b8e9d0a910 100644 > >> > > > > > --- a/drivers/mtd/parsers/ofpart_core.c > >> > > > > > +++ b/drivers/mtd/parsers/ofpart_core.c > >> > > > > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct > >> > > > > > mtd_info *master, > >> > > > > > > >> > > >
Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0
Hi Francesco,
france...@dolcini.it wrote on Thu, 2 Feb 2023 12:33:34 +0100:
> On Thu, Jan 26, 2023 at 10:12:04AM +0100, Miquel Raynal wrote:
> > gre...@linuxfoundation.org wrote on Thu, 26 Jan 2023 10:01:02 +0100:
> >
> > > On Wed, Jan 25, 2023 at 10:06:57PM +0100, Francesco Dolcini wrote:
> > > > Hello Miquel, Greg and all
> > > >
> > > > On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:
> > > > > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:
> > > > > > From: Francesco Dolcini
> > > > > >
> > > > > > Add a mechanism to handle the case in which partitions are present
> > > > > > as
> > > > > > direct child of the nand controller node and #size-cells is set to
> > > > > > <0>.
> > > > > >
> > > > > > This could happen if the nand-controller node in the DTS is
> > > > > > supposed to
> > > > > > have #size-cells set to 0, but for some historical reason/bug it
> > > > > > was set
> > > > > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the
> > > > > > partition
> > > > > > as direct children of the nand-controller defaulting to #size-cells
> > > > > > being to 1.
> > > > > >
> > > > > > This prevents a real boot failure on colibri-imx7 that happened
> > > > > > during v6.1
> > > > > > development cycles.
> > > > > >
> > > > > > Link:
> > > > > > https://lore.kernel.org/all/y4dgbtgnwpm6s...@francesco-nb.int.toradex.com/
> > > > > > Link:
> > > > > > https://lore.kernel.org/all/20221202071900.1143950-1-france...@dolcini.it/
> > > > > > Signed-off-by: Francesco Dolcini
> > > > > > Reviewed-by: Greg Kroah-Hartman
> > > > > > ---
> > > > > > I do not expect this patch to be backported to stable, however I
> > > > > > would expect
> > > > > > that we do not backport nand-controller dts cleanups neither.
> > > > > >
> > > > > > v4:
> > > > > > fixed wrong English spelling in the comment
> > > > > >
> > > > > > v3:
> > > > > > minor formatting change, removed not needed new-line and space.
> > > > > >
> > > > > > v2:
> > > > > > fixup size-cells only when partitions are direct children of the
> > > > > > nand-controller
> > > > > > completely revised commit message, comments and warning print
> > > > > > use pr_warn instead of pr_warn_once
> > > > > > added Reviewed-by Greg
> > > > > > removed cc:stable@ and fixes tag, since the problematic commit was
> > > > > > reverted
> > > > > > ---
> > > > > > drivers/mtd/parsers/ofpart_core.c | 19 +++
> > > > > > 1 file changed, 19 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/mtd/parsers/ofpart_core.c
> > > > > > b/drivers/mtd/parsers/ofpart_core.c
> > > > > > index 192190c42fc8..e7b8e9d0a910 100644
> > > > > > --- a/drivers/mtd/parsers/ofpart_core.c
> > > > > > +++ b/drivers/mtd/parsers/ofpart_core.c
> > > > > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct
> > > > > > mtd_info *master,
> > > > > >
> > > > > > a_cells = of_n_addr_cells(pp);
> > > > > > s_cells = of_n_size_cells(pp);
> > > > > > + if (!dedicated && s_cells == 0) {
> > > > > > + /*
> > > > > > +* This is a ugly workaround to not create
> > > > > > +* regression on devices that are still creating
> > > > > > +* partitions as direct children of the nand
> > > > > > controller.
> > > > > > +* This can happen in case the nand controller
> > > > > > node has
> > > > > > +* #size-cells equal to 0 and the firmware (e.g.
> > > > > > +* U-Boot) just add the partitions there
> > &g
Re: [PATCH V3 1/6] nvmem: core: add nvmem_dev_size() helper
Hi Rafał,
zaj...@gmail.com wrote on Fri, 27 Jan 2023 13:57:04 +0100:
> From: Rafał Miłecki
>
> This is required by layouts that need to read whole NVMEM space. It
> applies to NVMEM devices without hardcoded layout (like U-Boot
> environment data block).
>
> Signed-off-by: Rafał Miłecki
> ---
> V2: Drop "const" from "const size_t"
It would be good if you could always add a cover-letter, just so that
we can reply to the whole series. In my case I wanted to add a global
Reviewed-by: Miquel Raynal
Because I gave this series a quick review and it looks good to me.
Thanks,
Miquèl
> ---
> drivers/nvmem/core.c | 13 +
> include/linux/nvmem-consumer.h | 1 +
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 38a5728bc65c..9e77af0164aa 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -2063,6 +2063,19 @@ void nvmem_del_cell_lookups(struct nvmem_cell_lookup
> *entries, size_t nentries)
> }
> EXPORT_SYMBOL_GPL(nvmem_del_cell_lookups);
>
> +/**
> + * nvmem_dev_size() - Get the size of a given nvmem device.
> + *
> + * @nvmem: nvmem device.
> + *
> + * Return: size of the nvmem device.
> + */
> +size_t nvmem_dev_size(struct nvmem_device *nvmem)
> +{
> + return nvmem->size;
> +}
> +EXPORT_SYMBOL_GPL(nvmem_dev_size);
> +
> /**
> * nvmem_dev_name() - Get the name of a given nvmem device.
> *
> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
> index fa030d93b768..c3005ab6cc4f 100644
> --- a/include/linux/nvmem-consumer.h
> +++ b/include/linux/nvmem-consumer.h
> @@ -78,6 +78,7 @@ ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
> int nvmem_device_cell_write(struct nvmem_device *nvmem,
> struct nvmem_cell_info *info, void *buf);
>
> +size_t nvmem_dev_size(struct nvmem_device *nvmem);
> const char *nvmem_dev_name(struct nvmem_device *nvmem);
>
> void nvmem_add_cell_lookups(struct nvmem_cell_lookup *entries,
Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0
Hi Greg,
gre...@linuxfoundation.org wrote on Thu, 26 Jan 2023 10:01:02 +0100:
> On Wed, Jan 25, 2023 at 10:06:57PM +0100, Francesco Dolcini wrote:
> > Hello Miquel, Greg and all
> >
> > On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:
> > > > From: Francesco Dolcini
> > > >
> > > > Add a mechanism to handle the case in which partitions are present as
> > > > direct child of the nand controller node and #size-cells is set to <0>.
> > > >
> > > > This could happen if the nand-controller node in the DTS is supposed to
> > > > have #size-cells set to 0, but for some historical reason/bug it was set
> > > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
> > > > as direct children of the nand-controller defaulting to #size-cells
> > > > being to 1.
> > > >
> > > > This prevents a real boot failure on colibri-imx7 that happened during
> > > > v6.1
> > > > development cycles.
> > > >
> > > > Link:
> > > > https://lore.kernel.org/all/y4dgbtgnwpm6s...@francesco-nb.int.toradex.com/
> > > > Link:
> > > > https://lore.kernel.org/all/20221202071900.1143950-1-france...@dolcini.it/
> > > > Signed-off-by: Francesco Dolcini
> > > > Reviewed-by: Greg Kroah-Hartman
> > > > ---
> > > > I do not expect this patch to be backported to stable, however I would
> > > > expect
> > > > that we do not backport nand-controller dts cleanups neither.
> > > >
> > > > v4:
> > > > fixed wrong English spelling in the comment
> > > >
> > > > v3:
> > > > minor formatting change, removed not needed new-line and space.
> > > >
> > > > v2:
> > > > fixup size-cells only when partitions are direct children of the
> > > > nand-controller
> > > > completely revised commit message, comments and warning print
> > > > use pr_warn instead of pr_warn_once
> > > > added Reviewed-by Greg
> > > > removed cc:stable@ and fixes tag, since the problematic commit was
> > > > reverted
> > > > ---
> > > > drivers/mtd/parsers/ofpart_core.c | 19 +++
> > > > 1 file changed, 19 insertions(+)
> > > >
> > > > diff --git a/drivers/mtd/parsers/ofpart_core.c
> > > > b/drivers/mtd/parsers/ofpart_core.c
> > > > index 192190c42fc8..e7b8e9d0a910 100644
> > > > --- a/drivers/mtd/parsers/ofpart_core.c
> > > > +++ b/drivers/mtd/parsers/ofpart_core.c
> > > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info
> > > > *master,
> > > >
> > > > a_cells = of_n_addr_cells(pp);
> > > > s_cells = of_n_size_cells(pp);
> > > > + if (!dedicated && s_cells == 0) {
> > > > + /*
> > > > +* This is a ugly workaround to not create
> > > > +* regression on devices that are still creating
> > > > +* partitions as direct children of the nand
> > > > controller.
> > > > +* This can happen in case the nand controller
> > > > node has
> > > > +* #size-cells equal to 0 and the firmware (e.g.
> > > > +* U-Boot) just add the partitions there
> > > > assuming
> > > > +* 32-bit addressing.
> > > > +*
> > > > +* If you get this warning your firmware and/or
> > > > DTS
> > > > +* should be really fixed.
> > > > +*
> > > > +* This is working only for devices smaller
> > > > than 4GiB.
> > > > +*/
> > > > + pr_warn("%s: ofpart partition %pOF (%pOF)
> > > > #size-cells is wrongly set to <0>, assuming <1> for parsing
> > > > partitions.\n",
> > > > + master->name, pp, mtd_node);
> > >
> > > This is a driver, always use dev_*() calls, not pr_*() calls so that we
> > > know what is being referred to exactly.
> >
> > Is this reasonable here? Where can I get the struct device?
>
> Walk back up the call chain, there has to be a device somewhere
> controlling this, right?
>
> > In general this file uses only pr_* debug API and messages are about OF
> > nodes/properties, not about a device.
>
> OF nodes and properties are part of a device's properties :)
Yes but the warning comes from a wrong DT description, hence it felt
better suited to warn against the node name which is easily identifiable
in a text file and must be fixed rather than the device which is a pure
software component.
Anyway, Francesco, please show us the resultant line and if it feels
meaningful enough we'll take the dev_warn approach.
Thanks,
Miquèl
Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0
Hi Greg,
france...@dolcini.it wrote on Wed, 25 Jan 2023 22:06:57 +0100:
> Hello Miquel, Greg and all
>
> On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:
> > > From: Francesco Dolcini
> > >
> > > Add a mechanism to handle the case in which partitions are present as
> > > direct child of the nand controller node and #size-cells is set to <0>.
> > >
> > > This could happen if the nand-controller node in the DTS is supposed to
> > > have #size-cells set to 0, but for some historical reason/bug it was set
> > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
> > > as direct children of the nand-controller defaulting to #size-cells
> > > being to 1.
> > >
> > > This prevents a real boot failure on colibri-imx7 that happened during
> > > v6.1
> > > development cycles.
> > >
> > > Link:
> > > https://lore.kernel.org/all/y4dgbtgnwpm6s...@francesco-nb.int.toradex.com/
> > > Link:
> > > https://lore.kernel.org/all/20221202071900.1143950-1-france...@dolcini.it/
> > > Signed-off-by: Francesco Dolcini
> > > Reviewed-by: Greg Kroah-Hartman
> > > ---
> > > I do not expect this patch to be backported to stable, however I would
> > > expect
> > > that we do not backport nand-controller dts cleanups neither.
> > >
> > > v4:
> > > fixed wrong English spelling in the comment
> > >
> > > v3:
> > > minor formatting change, removed not needed new-line and space.
> > >
> > > v2:
> > > fixup size-cells only when partitions are direct children of the
> > > nand-controller
> > > completely revised commit message, comments and warning print
> > > use pr_warn instead of pr_warn_once
> > > added Reviewed-by Greg
> > > removed cc:stable@ and fixes tag, since the problematic commit was
> > > reverted
> > > ---
> > > drivers/mtd/parsers/ofpart_core.c | 19 +++
> > > 1 file changed, 19 insertions(+)
> > >
> > > diff --git a/drivers/mtd/parsers/ofpart_core.c
> > > b/drivers/mtd/parsers/ofpart_core.c
> > > index 192190c42fc8..e7b8e9d0a910 100644
> > > --- a/drivers/mtd/parsers/ofpart_core.c
> > > +++ b/drivers/mtd/parsers/ofpart_core.c
> > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info
> > > *master,
> > >
> > > a_cells = of_n_addr_cells(pp);
> > > s_cells = of_n_size_cells(pp);
> > > + if (!dedicated && s_cells == 0) {
> > > + /*
> > > + * This is a ugly workaround to not create
> > > + * regression on devices that are still creating
> > > + * partitions as direct children of the nand controller.
> > > + * This can happen in case the nand controller node has
> > > + * #size-cells equal to 0 and the firmware (e.g.
> > > + * U-Boot) just add the partitions there assuming
> > > + * 32-bit addressing.
> > > + *
> > > + * If you get this warning your firmware and/or DTS
> > > + * should be really fixed.
> > > + *
> > > + * This is working only for devices smaller than 4GiB.
> > > + */
> > > + pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells
> > > is wrongly set to <0>, assuming <1> for parsing partitions.\n",
> > > + master->name, pp, mtd_node);
> >
> > This is a driver, always use dev_*() calls, not pr_*() calls so that we
> > know what is being referred to exactly.
>
> Is this reasonable here? Where can I get the struct device?
>
> In general this file uses only pr_* debug API and messages are about OF
> nodes/properties, not about a device.
I'm also skeptical here, this is not a device driver, it's a generic
parser and it seems more appropriate to warn about an of node rather
than a struct device.
MTD devices inherit from struct device (mtd->dev) which I guess
might be used here. The bus infrastructure device
(mtd->device->parents) is less appropriate as it sometimes points at the
controller (raw NAND) and sometimes at the spi device (SPI-NAND, SPI
NOR).
pr_warn is fine here IMHO, but if Greg insist, switch it to dev_warn, I
don't mind. Maybe it is worth testing that dev_warn still displays an
easy-to-understand message in that case.
> > I take back my "reviewed-by" line above, please fix this up to not need
> > pr_warn, but to use dev_warn() instead.
>
> Francesco
Thanks,
Miquèl
