fitimage: Allow match against config node name

fitimage: Allow match against config node name

[Hans Christian Lønstad]

Support fitimage configuration nodes without populated compatible fields

Yocto fit image recipe does not populate the compatible field
in the generated ITS file configuration nodes.
Barebox is thus only able to load the default configuration
preventing the use of variant based bootloader update bundles.

If the compatible match fails, fall through using a global
variable boot.fitnode allowing a match against the configuration
node name.

This allows variant boards to pick the correct configuration.

---
common/image-fit.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/common/image-fit.c b/common/image-fit.c
index a410632d70..f92e813a8b 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -7,6 +7,7 @@

#define pr_fmt(fmt) "FIT: " fmt
#include <common.h>
+#include <environment.h>
#include <init.h>
#include <bootm.h>
#include <libfile.h>
@@ -663,6 +664,7 @@ static int fit_find_compatible_unit(struct device_node *conf_node,
struct device_node *barebox_root;
const char *machine;
int ret;
+ const char *config_node;

barebox_root = of_get_root_node();
if (!barebox_root)
@@ -680,6 +682,22 @@ static int fit_find_compatible_unit(struct device_node *conf_node,
}
}

+ /*
+ * If the match against compatible in config node does not match
+ * (or is missing as in Yocto fitimage recipe)
+ * check for matching node name using global.boot.fitnode
+ */
+ config_node = getenv("global.boot.fitnode");
+ if (config_node) {
+ for_each_child_of_node (conf_node, child) {
+ if (strcmp(child->name, config_node) == 0) {
+ *unit = child->name;
+ pr_info("matching node name unit '%s' found\n", *unit);
+ return 0;
+ }
+ }
+ }
+
default_unit:
pr_info("No match found. Trying default.\n");
if (of_property_read_string(conf_node, "default", unit) == 0)
-- 
2.34.1

Re: fitimage: Allow match against config node name

[Ahmad Fatoum]

Hello Hans,

On 16.09.22 15:36, Hans Christian Lønstad wrote:
> Support fitimage configuration nodes without populated compatible fields
> 
> Yocto fit image recipe does not populate the compatible field
> in the generated ITS file configuration nodes.
> Barebox is thus only able to load the default configuration
> preventing the use of variant based bootloader update bundles.
> 
> If the compatible match fails, fall through using a global
> variable boot.fitnode allowing a match against the configuration
> node name.
> 
> This allows variant boards to pick the correct configuration.

Thanks for your patch.

> 
> ---
> common/image-fit.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
> 
> diff --git a/common/image-fit.c b/common/image-fit.c
> index a410632d70..f92e813a8b 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -7,6 +7,7 @@
> 
> #define pr_fmt(fmt) "FIT: " fmt
> #include <common.h>
> +#include <environment.h>
> #include <init.h>
> #include <bootm.h>
> #include <libfile.h>
> @@ -663,6 +664,7 @@ static int fit_find_compatible_unit(struct device_node *conf_node,
> struct device_node *barebox_root;
> const char *machine;
> int ret;
> + const char *config_node;
> 
> barebox_root = of_get_root_node();
> if (!barebox_root)
> @@ -680,6 +682,22 @@ static int fit_find_compatible_unit(struct device_node *conf_node,
> }
> }
> 
> + /*
> + * If the match against compatible in config node does not match
> + * (or is missing as in Yocto fitimage recipe)
> + * check for matching node name using global.boot.fitnode
> + */
> + config_node = getenv("global.boot.fitnode");
> + if (config_node) {
> + for_each_child_of_node (conf_node, child) {
> + if (strcmp(child->name, config_node) == 0) {
> + *unit = child->name;
> + pr_info("matching node name unit '%s' found\n", *unit);
> + return 0;
> + }
> + }
> + }

Whitespace is broken (git send-email normally does the correct thing).

But are you aware that you can have your boot-target like:

bootm /dev/mmc0.fit@configuration1

and that configuration1 will be chosen? Does this already cover
your use case? If it does, a documentation patch adding this
information at the place where you didn't find it is surely
a welcome alternate contribution. ;)

(also generally speaking magicvars need to be documented with
 BAREBOX_MAGICVAR).

Cheers,
Ahmad


> +
> default_unit:
> pr_info("No match found. Trying default.\n");
> if (of_property_read_string(conf_node, "default", unit) == 0)


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: fitimage: Allow match against config node name

[Hans Christian Lønstad]

Thank you for pointing out the existing option, it resolves the issue.
Regarding patch submissions:
Are GitHub pull requests an option? Actually have one pending that is needed on USB for IMX8MP.

Hans Christian

> 16. sep. 2022 kl. 19:26 skrev Ahmad Fatoum <a.fatoum@pengutronix.de>:
> 
> Hello Hans,
> 
> On 16.09.22 15:36, Hans Christian Lønstad wrote:
>> Support fitimage configuration nodes without populated compatible fields
>> 
>> Yocto fit image recipe does not populate the compatible field
>> in the generated ITS file configuration nodes.
>> Barebox is thus only able to load the default configuration
>> preventing the use of variant based bootloader update bundles.
>> 
>> If the compatible match fails, fall through using a global
>> variable boot.fitnode allowing a match against the configuration
>> node name.
>> 
>> This allows variant boards to pick the correct configuration.
> 
> Thanks for your patch.
> 
>> 
>> ---
>> common/image-fit.c | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>> 
>> diff --git a/common/image-fit.c b/common/image-fit.c
>> index a410632d70..f92e813a8b 100644
>> --- a/common/image-fit.c
>> +++ b/common/image-fit.c
>> @@ -7,6 +7,7 @@
>> 
>> #define pr_fmt(fmt) "FIT: " fmt
>> #include <common.h>
>> +#include <environment.h>
>> #include <init.h>
>> #include <bootm.h>
>> #include <libfile.h>
>> @@ -663,6 +664,7 @@ static int fit_find_compatible_unit(struct device_node *conf_node,
>> struct device_node *barebox_root;
>> const char *machine;
>> int ret;
>> + const char *config_node;
>> 
>> barebox_root = of_get_root_node();
>> if (!barebox_root)
>> @@ -680,6 +682,22 @@ static int fit_find_compatible_unit(struct device_node *conf_node,
>> }
>> }
>> 
>> + /*
>> + * If the match against compatible in config node does not match
>> + * (or is missing as in Yocto fitimage recipe)
>> + * check for matching node name using global.boot.fitnode
>> + */
>> + config_node = getenv("global.boot.fitnode");
>> + if (config_node) {
>> + for_each_child_of_node (conf_node, child) {
>> + if (strcmp(child->name, config_node) == 0) {
>> + *unit = child->name;
>> + pr_info("matching node name unit '%s' found\n", *unit);
>> + return 0;
>> + }
>> + }
>> + }
> 
> Whitespace is broken (git send-email normally does the correct thing).
> 
> But are you aware that you can have your boot-target like:
> 
> bootm /dev/mmc0.fit@configuration1
> 
> and that configuration1 will be chosen? Does this already cover
> your use case? If it does, a documentation patch adding this
> information at the place where you didn't find it is surely
> a welcome alternate contribution. ;)
> 
> (also generally speaking magicvars need to be documented with
> BAREBOX_MAGICVAR).
> 
> Cheers,
> Ahmad
> 
> 
>> +
>> default_unit:
>> pr_info("No match found. Trying default.\n");
>> if (of_property_read_string(conf_node, "default", unit) == 0)
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: fitimage: Allow match against config node name

[Ahmad Fatoum]

Hi Hans,

On 17.09.22 07:51, Hans Christian Lønstad wrote:
> Thank you for pointing out the existing option, it resolves the issue.

:)

> Regarding patch submissions:
> Are GitHub pull requests an option? Actually have one pending that is needed on USB for IMX8MP.

Mailing list is the usual contribution method, but Sascha has accepted
Github PRs in the past.

Regarding your existing DWC3 PR, you will need to reword the commit message
to make this acceptable for barebox inclusion:

1) All contributions must have a Signed-off-by certifying origin, see
   https://developercertificate.org/

2) Commit message needs to be useful on its own. The Github issue can
   be referenced, but should not be the only documentation

3) Commit title needs to be adapted to convention for the driver.


Here's my suggestion:

  usb: dwc3: support snps,dis-u2-freeclk-exists-quirk for i.MX8MP

  The driver currently configures the Synopsis USB 2.0 PHY to
  unconditionally provide a free-running PHY clock. This must
  be disabled on the i.MX8MP as well as RK3328 and RK3399 SoC,
  because the clock control input is inactive.

  The upstream Linux binding specifies a boolean DT property
  snps,dis-u2-freeclk-exists-quirk to disable the free-running
  PHY clock and the property is already used in the i.MX8MP
  DT, that barebox imports from upstream. Thus implement the
  binding for barebox. This fixes barebox USB host support on the
  i.MX8MP, which previously triggered a BUG() when enumerating[1].

  Link: https://github.com/saschahauer/barebox/issues/13 [1]
  Fixes: e213627bbe1d ("usb: dwc3: of-simple: add i.MX8MP compatible")
  Signed-off-by: Author Name <author.email@example.com> 


If you squash this commit message into your PR, feel free to
append my

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Also Cc'ing Lucas, as I am wondering why you ran into the issue, but Lucas
apparently didn't.

Cheers,
Ahmad

> Hans Christian
> 
>> 16. sep. 2022 kl. 19:26 skrev Ahmad Fatoum <a.fatoum@pengutronix.de>:
>>
>> Hello Hans,
>>
>> On 16.09.22 15:36, Hans Christian Lønstad wrote:
>>> Support fitimage configuration nodes without populated compatible fields
>>>
>>> Yocto fit image recipe does not populate the compatible field
>>> in the generated ITS file configuration nodes.
>>> Barebox is thus only able to load the default configuration
>>> preventing the use of variant based bootloader update bundles.
>>>
>>> If the compatible match fails, fall through using a global
>>> variable boot.fitnode allowing a match against the configuration
>>> node name.
>>>
>>> This allows variant boards to pick the correct configuration.
>>
>> Thanks for your patch.
>>
>>>
>>> ---
>>> common/image-fit.c | 18 ++++++++++++++++++
>>> 1 file changed, 18 insertions(+)
>>>
>>> diff --git a/common/image-fit.c b/common/image-fit.c
>>> index a410632d70..f92e813a8b 100644
>>> --- a/common/image-fit.c
>>> +++ b/common/image-fit.c
>>> @@ -7,6 +7,7 @@
>>>
>>> #define pr_fmt(fmt) "FIT: " fmt
>>> #include <common.h>
>>> +#include <environment.h>
>>> #include <init.h>
>>> #include <bootm.h>
>>> #include <libfile.h>
>>> @@ -663,6 +664,7 @@ static int fit_find_compatible_unit(struct device_node *conf_node,
>>> struct device_node *barebox_root;
>>> const char *machine;
>>> int ret;
>>> + const char *config_node;
>>>
>>> barebox_root = of_get_root_node();
>>> if (!barebox_root)
>>> @@ -680,6 +682,22 @@ static int fit_find_compatible_unit(struct device_node *conf_node,
>>> }
>>> }
>>>
>>> + /*
>>> + * If the match against compatible in config node does not match
>>> + * (or is missing as in Yocto fitimage recipe)
>>> + * check for matching node name using global.boot.fitnode
>>> + */
>>> + config_node = getenv("global.boot.fitnode");
>>> + if (config_node) {
>>> + for_each_child_of_node (conf_node, child) {
>>> + if (strcmp(child->name, config_node) == 0) {
>>> + *unit = child->name;
>>> + pr_info("matching node name unit '%s' found\n", *unit);
>>> + return 0;
>>> + }
>>> + }
>>> + }
>>
>> Whitespace is broken (git send-email normally does the correct thing).
>>
>> But are you aware that you can have your boot-target like:
>>
>> bootm /dev/mmc0.fit@configuration1
>>
>> and that configuration1 will be chosen? Does this already cover
>> your use case? If it does, a documentation patch adding this
>> information at the place where you didn't find it is surely
>> a welcome alternate contribution. ;)
>>
>> (also generally speaking magicvars need to be documented with
>> BAREBOX_MAGICVAR).
>>
>> Cheers,
>> Ahmad
>>
>>
>>> +
>>> default_unit:
>>> pr_info("No match found. Trying default.\n");
>>> if (of_property_read_string(conf_node, "default", unit) == 0)
>>
>>
>> -- 
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: fitimage: Allow match against config node name

[Hans Christian Lønstad]

Great, re-issued PR with the proper wording …

Hans Christian

> 17. sep. 2022 kl. 12:13 skrev Ahmad Fatoum <a.fatoum@pengutronix.de>:
> 
> Hi Hans,
> 
> On 17.09.22 07:51, Hans Christian Lønstad wrote:
>> Thank you for pointing out the existing option, it resolves the issue.
> 
> :)
> 
>> Regarding patch submissions:
>> Are GitHub pull requests an option? Actually have one pending that is needed on USB for IMX8MP.
> 
> Mailing list is the usual contribution method, but Sascha has accepted
> Github PRs in the past.
> 
> Regarding your existing DWC3 PR, you will need to reword the commit message
> to make this acceptable for barebox inclusion:
> 
> 1) All contributions must have a Signed-off-by certifying origin, see
>   https://developercertificate.org/
> 
> 2) Commit message needs to be useful on its own. The Github issue can
>   be referenced, but should not be the only documentation
> 
> 3) Commit title needs to be adapted to convention for the driver.
> 
> 
> Here's my suggestion:
> 
>  usb: dwc3: support snps,dis-u2-freeclk-exists-quirk for i.MX8MP
> 
>  The driver currently configures the Synopsis USB 2.0 PHY to
>  unconditionally provide a free-running PHY clock. This must
>  be disabled on the i.MX8MP as well as RK3328 and RK3399 SoC,
>  because the clock control input is inactive.
> 
>  The upstream Linux binding specifies a boolean DT property
>  snps,dis-u2-freeclk-exists-quirk to disable the free-running
>  PHY clock and the property is already used in the i.MX8MP
>  DT, that barebox imports from upstream. Thus implement the
>  binding for barebox. This fixes barebox USB host support on the
>  i.MX8MP, which previously triggered a BUG() when enumerating[1].
> 
>  Link: https://github.com/saschahauer/barebox/issues/13 [1]
>  Fixes: e213627bbe1d ("usb: dwc3: of-simple: add i.MX8MP compatible")
>  Signed-off-by: Author Name <author.email@example.com> 
> 
> 
> If you squash this commit message into your PR, feel free to
> append my
> 
> Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> Also Cc'ing Lucas, as I am wondering why you ran into the issue, but Lucas
> apparently didn't.
> 
> Cheers,
> Ahmad
> 
>> Hans Christian
>> 
>>> 16. sep. 2022 kl. 19:26 skrev Ahmad Fatoum <a.fatoum@pengutronix.de>:
>>> 
>>> Hello Hans,
>>> 
>>> On 16.09.22 15:36, Hans Christian Lønstad wrote:
>>>> Support fitimage configuration nodes without populated compatible fields
>>>> 
>>>> Yocto fit image recipe does not populate the compatible field
>>>> in the generated ITS file configuration nodes.
>>>> Barebox is thus only able to load the default configuration
>>>> preventing the use of variant based bootloader update bundles.
>>>> 
>>>> If the compatible match fails, fall through using a global
>>>> variable boot.fitnode allowing a match against the configuration
>>>> node name.
>>>> 
>>>> This allows variant boards to pick the correct configuration.
>>> 
>>> Thanks for your patch.
>>> 
>>>> 
>>>> ---
>>>> common/image-fit.c | 18 ++++++++++++++++++
>>>> 1 file changed, 18 insertions(+)
>>>> 
>>>> diff --git a/common/image-fit.c b/common/image-fit.c
>>>> index a410632d70..f92e813a8b 100644
>>>> --- a/common/image-fit.c
>>>> +++ b/common/image-fit.c
>>>> @@ -7,6 +7,7 @@
>>>> 
>>>> #define pr_fmt(fmt) "FIT: " fmt
>>>> #include <common.h>
>>>> +#include <environment.h>
>>>> #include <init.h>
>>>> #include <bootm.h>
>>>> #include <libfile.h>
>>>> @@ -663,6 +664,7 @@ static int fit_find_compatible_unit(struct device_node *conf_node,
>>>> struct device_node *barebox_root;
>>>> const char *machine;
>>>> int ret;
>>>> + const char *config_node;
>>>> 
>>>> barebox_root = of_get_root_node();
>>>> if (!barebox_root)
>>>> @@ -680,6 +682,22 @@ static int fit_find_compatible_unit(struct device_node *conf_node,
>>>> }
>>>> }
>>>> 
>>>> + /*
>>>> + * If the match against compatible in config node does not match
>>>> + * (or is missing as in Yocto fitimage recipe)
>>>> + * check for matching node name using global.boot.fitnode
>>>> + */
>>>> + config_node = getenv("global.boot.fitnode");
>>>> + if (config_node) {
>>>> + for_each_child_of_node (conf_node, child) {
>>>> + if (strcmp(child->name, config_node) == 0) {
>>>> + *unit = child->name;
>>>> + pr_info("matching node name unit '%s' found\n", *unit);
>>>> + return 0;
>>>> + }
>>>> + }
>>>> + }
>>> 
>>> Whitespace is broken (git send-email normally does the correct thing).
>>> 
>>> But are you aware that you can have your boot-target like:
>>> 
>>> bootm /dev/mmc0.fit@configuration1
>>> 
>>> and that configuration1 will be chosen? Does this already cover
>>> your use case? If it does, a documentation patch adding this
>>> information at the place where you didn't find it is surely
>>> a welcome alternate contribution. ;)
>>> 
>>> (also generally speaking magicvars need to be documented with
>>> BAREBOX_MAGICVAR).
>>> 
>>> Cheers,
>>> Ahmad
>>> 
>>> 
>>>> +
>>>> default_unit:
>>>> pr_info("No match found. Trying default.\n");
>>>> if (of_property_read_string(conf_node, "default", unit) == 0)
>>> 
>>> 
>>> -- 
>>> Pengutronix e.K.                           |                             |
>>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>> 
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |