[PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

Currently, fixed-rate clocks in U-Boot are named based on their devicetree
node names. For example, given the following node:

osc_24m: clock-osc-24m {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <24000000>;
	clock-output-names = "osc_24m";
};

U-Boot registers the clock as "clock-osc-24m", derived from the node name,
ignoring the clock-output-names property.

This differs from Linux, which uses the clock-output-names property when
assigning clock names. As a result, clock consumers expecting names
like "osc_24m" (as defined in the property) may fail to resolve clocks
correctly in U-Boot.

Update the fixed-rate clock driver to check for the clock-output-names
property and use it as the clock name if present. If not, the fallback
remains the node name. This makes U-Boot behavior consistent with Linux.

One concrete impact is on i.MX8MP, where USB clock lookup failed following
commit b4734c9c333b ("clk: imx: Convert clock-osc-* back to osc_*").

With this change applied, fixed-clocks are correctly registered with their
expected names:

u-boot=> clk dump
 Rate               Usecnt      Name
------------------------------------------
 32768                0        |-- osc_32k
 24000000             5        |-- osc_24m

This change restores i.MX8MP USB functionality by ensuring the proper clock
names are used.

Fixes: b4734c9c333b ("clk: imx: Convert clock-osc-* back to osc_*")
Reported-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Signed-off-by: Fabio Estevam <festevam@denx.de>
Tested-by: Adam Ford <aford173@gmail.com> #imx8mp-beacon
---
Changes since v1:
- Included <dm/read.h> that defines dev_read_string_index() instead
of relying on indirect inclusion via <dm.h>.
- Removed superflous parenthesis around dev_read_string_index().
- Added Adam's Reviewed-by tag.

 drivers/clk/clk_fixed_rate.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/clk/clk_fixed_rate.c b/drivers/clk/clk_fixed_rate.c
index d1da05cc18a5..648e31b4fe14 100644
--- a/drivers/clk/clk_fixed_rate.c
+++ b/drivers/clk/clk_fixed_rate.c
@@ -9,6 +9,7 @@
 #include <dm.h>
 #include <log.h>
 #include <dm/device-internal.h>
+#include <dm/read.h>
 #include <linux/clk-provider.h>
 
 #define UBOOT_DM_CLK_FIXED_RATE "fixed_rate_clock"
@@ -35,6 +36,8 @@ void clk_fixed_rate_ofdata_to_plat_(struct udevice *dev,
 				    struct clk_fixed_rate *plat)
 {
 	struct clk *clk = &plat->clk;
+	const char *clk_name;
+
 	if (CONFIG_IS_ENABLED(OF_REAL))
 		plat->fixed_rate = dev_read_u32_default(dev, "clock-frequency",
 							0);
@@ -45,6 +48,16 @@ void clk_fixed_rate_ofdata_to_plat_(struct udevice *dev,
 
 	clk->dev = dev;
 	clk->enable_count = 0;
+
+	/*
+	 * If the "clock-output-names" property is present, use it
+	 * as the clock name, like it is done in Linux.
+	 * Otherwise, the original behavior will be preserved: the fixed clock
+	 * name will be its node name.
+	 *
+	 */
+	if (!dev_read_string_index(dev, "clock-output-names", 0, &clk_name))
+		dev->name = clk_name;
 }
 
 static ulong clk_fixed_rate_raw_get_rate(struct clk *clk)
-- 
2.34.1

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Marek Vasut]

On 4/17/25 7:11 PM, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@denx.de>
> 
> Currently, fixed-rate clocks in U-Boot are named based on their devicetree
> node names. For example, given the following node:
> 
> osc_24m: clock-osc-24m {
> 	compatible = "fixed-clock";
> 	#clock-cells = <0>;
> 	clock-frequency = <24000000>;
> 	clock-output-names = "osc_24m";
> };
> 
> U-Boot registers the clock as "clock-osc-24m", derived from the node name,
> ignoring the clock-output-names property.
> 
> This differs from Linux, which uses the clock-output-names property when
> assigning clock names. As a result, clock consumers expecting names
> like "osc_24m" (as defined in the property) may fail to resolve clocks
> correctly in U-Boot.
> 
> Update the fixed-rate clock driver to check for the clock-output-names
> property and use it as the clock name if present. If not, the fallback
> remains the node name. This makes U-Boot behavior consistent with Linux.

This part above ^ is fine.

This part below v is wrong .

> One concrete impact is on i.MX8MP, where USB clock lookup failed following
> commit b4734c9c333b ("clk: imx: Convert clock-osc-* back to osc_*").
> 
> With this change applied, fixed-clocks are correctly registered with their
> expected names:
> 
> u-boot=> clk dump
>   Rate               Usecnt      Name
> ------------------------------------------
>   32768                0        |-- osc_32k
>   24000000             5        |-- osc_24m
> 
> This change restores i.MX8MP USB functionality by ensuring the proper clock
> names are used.

This is wrong, because this is NOT how the clock look up works at all.

The lookup works this way. Take for example:

drivers/clk/imx/clk-imx8mp.c

240         clk_dm(IMX8MP_ARM_PLL_REF_SEL, imx_clk_mux(dev, 
"arm_pll_ref_sel", base + 0x84, 0, 2, pll_ref_sels, 
ARRAY_SIZE(pll_ref_sels)));

pll_ref_sels:

  21 static const char * const pll_ref_sels[] = { "osc_24m", "dummy", 
"dummy", "dummy", };

the "osc_24m" is looked up in the clock-controller@30380000 clock-names 
property:

dts/upstream/src/arm64/freescale/imx8mp.dtsi

  745 clk: clock-controller@30380000 {
...
  751         clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
  752                  <&clk_ext3>, <&clk_ext4>;
  753         clock-names = "osc_32k", "osc_24m", ...
                                        ^^^^^^^

The offset of osc_24m in that clock-names property is 1 , so the phandle 
in clocks property array at offset 1 is used to look up those clock:

  745 clk: clock-controller@30380000 {
...
  751         clocks = <&osc_32k>, <&osc_24m>,
                                    ^^^^^^^^

Which leads to this clock node in DT:

  188         osc_24m: clock-osc-24m {
              ^^^^^^^^
  189                 compatible = "fixed-clock";
  190                 #clock-cells = <0>;

The "clock-output-names" is NEVER used for any clock look up.

...

I am fine with the first half of the description, but the second half is 
wrong and this actually does not fix anything. Simply remove 
clock-output-names from the osc_24m and the code will be broken again.

The clk_resolve_parent_clk() is meant to perform the aforementioned 
resolution(), that is what "[PATCH v2 00/24] clk: Add 
clk_resolve_parent_clk() and fix up iMX clock drivers" actually fixed. 
It seems there is something still missing and clk_resolve_parent_clk() 
does not do the resolution properly for some clock, but that is what 
needs to be understood and fixed, not worked around this way.

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Adam Ford]

On Fri, Apr 18, 2025 at 9:53 AM Marek Vasut <marex@denx.de> wrote:
>
> On 4/17/25 7:11 PM, Fabio Estevam wrote:
> > From: Fabio Estevam <festevam@denx.de>
> >
> > Currently, fixed-rate clocks in U-Boot are named based on their devicetree
> > node names. For example, given the following node:
> >
> > osc_24m: clock-osc-24m {
> >       compatible = "fixed-clock";
> >       #clock-cells = <0>;
> >       clock-frequency = <24000000>;
> >       clock-output-names = "osc_24m";
> > };
> >
> > U-Boot registers the clock as "clock-osc-24m", derived from the node name,
> > ignoring the clock-output-names property.
> >
> > This differs from Linux, which uses the clock-output-names property when
> > assigning clock names. As a result, clock consumers expecting names
> > like "osc_24m" (as defined in the property) may fail to resolve clocks
> > correctly in U-Boot.
> >
> > Update the fixed-rate clock driver to check for the clock-output-names
> > property and use it as the clock name if present. If not, the fallback
> > remains the node name. This makes U-Boot behavior consistent with Linux.
>
> This part above ^ is fine.
>
> This part below v is wrong .
>
> > One concrete impact is on i.MX8MP, where USB clock lookup failed following
> > commit b4734c9c333b ("clk: imx: Convert clock-osc-* back to osc_*").
> >
> > With this change applied, fixed-clocks are correctly registered with their
> > expected names:
> >
> > u-boot=> clk dump
> >   Rate               Usecnt      Name
> > ------------------------------------------
> >   32768                0        |-- osc_32k
> >   24000000             5        |-- osc_24m
> >
> > This change restores i.MX8MP USB functionality by ensuring the proper clock
> > names are used.
>
> This is wrong, because this is NOT how the clock look up works at all.
>
> The lookup works this way. Take for example:
>
> drivers/clk/imx/clk-imx8mp.c
>
> 240         clk_dm(IMX8MP_ARM_PLL_REF_SEL, imx_clk_mux(dev,
> "arm_pll_ref_sel", base + 0x84, 0, 2, pll_ref_sels,
> ARRAY_SIZE(pll_ref_sels)));
>
> pll_ref_sels:
>
>   21 static const char * const pll_ref_sels[] = { "osc_24m", "dummy",
> "dummy", "dummy", };
>
> the "osc_24m" is looked up in the clock-controller@30380000 clock-names
> property:
>
> dts/upstream/src/arm64/freescale/imx8mp.dtsi
>
>   745 clk: clock-controller@30380000 {
> ...
>   751         clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
>   752                  <&clk_ext3>, <&clk_ext4>;
>   753         clock-names = "osc_32k", "osc_24m", ...
>                                         ^^^^^^^
>
> The offset of osc_24m in that clock-names property is 1 , so the phandle
> in clocks property array at offset 1 is used to look up those clock:
>
>   745 clk: clock-controller@30380000 {
> ...
>   751         clocks = <&osc_32k>, <&osc_24m>,
>                                     ^^^^^^^^
>
> Which leads to this clock node in DT:
>
>   188         osc_24m: clock-osc-24m {
>               ^^^^^^^^
>   189                 compatible = "fixed-clock";
>   190                 #clock-cells = <0>;
>
> The "clock-output-names" is NEVER used for any clock look up.

For what it's worth, I remove the "clock-out-names" from the device
tree in Linux, and the Linux driver properly coped with it, but I am
curious to know what the point of this entry is.
>
> ...
>
> I am fine with the first half of the description, but the second half is
> wrong and this actually does not fix anything. Simply remove
> clock-output-names from the osc_24m and the code will be broken again.
>
> The clk_resolve_parent_clk() is meant to perform the aforementioned
> resolution(), that is what "[PATCH v2 00/24] clk: Add
> clk_resolve_parent_clk() and fix up iMX clock drivers" actually fixed.
> It seems there is something still missing and clk_resolve_parent_clk()
> does not do the resolution properly for some clock, but that is what
> needs to be understood and fixed, not worked around this way.

I added some debug code to clk_resolve_parent_clk it does return
osc_24m as the name of the clock, but when searching for index, it
appears to fail. Do you have any suggestion on how searching for the
index can query up the chain to the parent CCM driver on how to return
the reference to proper clock?  The clock-names property is at the CCM
parent and not inside the children, and the number of levels vary
depending on the child clocks, you can't assume the parent's parent
has the 'clock-name's property.

adam

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Fabio Estevam]

Hi Marek,

On Fri, Apr 18, 2025 at 11:53 AM Marek Vasut <marex@denx.de> wrote:

> The "clock-output-names" is NEVER used for any clock look up.

In Linux, "clock-output-names" is used to register the name of the
fixed-rate clock.

From Linux drivers/clk/clk-fixed-rate.c:

```
static struct clk_hw *_of_fixed_clk_setup(struct device_node *node)
{
....

of_property_read_string(node, "clock-output-names", &clk_name);

hw = clk_hw_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
    0, rate, accuracy);
```

In U-Boot, we should register a fixed-rate clock with the same name as in Linux.

This patch aims to make the U-Boot fixed-rate clock name conform to
Linux standards.

> I am fine with the first half of the description, but the second half is
> wrong and this actually does not fix anything. Simply remove
> clock-output-names from the osc_24m and the code will be broken again.

In Linux, if "clock-output-names" is removed from the osc_24m node,
the console becomes corrupted
on an imx8mp-evk.

> The clk_resolve_parent_clk() is meant to perform the aforementioned
> resolution(), that is what "[PATCH v2 00/24] clk: Add
> clk_resolve_parent_clk() and fix up iMX clock drivers" actually fixed.
> It seems there is something still missing and clk_resolve_parent_clk()
> does not do the resolution properly for some clock, but that is what
> needs to be understood and fixed, not worked around this way.

Please try to reproduce the problem on your end.

On an imx8mp-based board, run 'usb start' in U-Boot, and this command will fail.

Thanks

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Adam Ford]

On Sat, Apr 19, 2025 at 2:12 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Marek,
>
> On Fri, Apr 18, 2025 at 11:53 AM Marek Vasut <marex@denx.de> wrote:
>
> > The "clock-output-names" is NEVER used for any clock look up.
>
> In Linux, "clock-output-names" is used to register the name of the
> fixed-rate clock.
>
> From Linux drivers/clk/clk-fixed-rate.c:
>
> ```
> static struct clk_hw *_of_fixed_clk_setup(struct device_node *node)
> {
> ....
>
> of_property_read_string(node, "clock-output-names", &clk_name);
>
> hw = clk_hw_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
>     0, rate, accuracy);
> ```
>
> In U-Boot, we should register a fixed-rate clock with the same name as in Linux.
>
> This patch aims to make the U-Boot fixed-rate clock name conform to
> Linux standards.
>
> > I am fine with the first half of the description, but the second half is
> > wrong and this actually does not fix anything. Simply remove
> > clock-output-names from the osc_24m and the code will be broken again.
>
> In Linux, if "clock-output-names" is removed from the osc_24m node,
> the console becomes corrupted
> on an imx8mp-evk.
>
> > The clk_resolve_parent_clk() is meant to perform the aforementioned
> > resolution(), that is what "[PATCH v2 00/24] clk: Add
> > clk_resolve_parent_clk() and fix up iMX clock drivers" actually fixed.
> > It seems there is something still missing and clk_resolve_parent_clk()
> > does not do the resolution properly for some clock, but that is what
> > needs to be understood and fixed, not worked around this way.
>
> Please try to reproduce the problem on your end.
>
> On an imx8mp-based board, run 'usb start' in U-Boot, and this command will fail.

From what I can tell, on the imx8mp, the usb_phy_ref parent->dev->name
value s "clock-osc-24m" which never matches "osc_24" and I put some
debug code into clk_mux_fetch_parent_index.

I think clk_mux_fetch_parent_index needs some additional logic to go
up the chain to the root node and look for 'clock-names' to match in
addition to what it already does. I am just not sure of the best
method to go up the tree to find the root node that defines it.

adam

>
> Thanks

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Marek Vasut]

On 4/19/25 2:59 PM, Adam Ford wrote:
> On Fri, Apr 18, 2025 at 9:53 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 4/17/25 7:11 PM, Fabio Estevam wrote:
>>> From: Fabio Estevam <festevam@denx.de>
>>>
>>> Currently, fixed-rate clocks in U-Boot are named based on their devicetree
>>> node names. For example, given the following node:
>>>
>>> osc_24m: clock-osc-24m {
>>>        compatible = "fixed-clock";
>>>        #clock-cells = <0>;
>>>        clock-frequency = <24000000>;
>>>        clock-output-names = "osc_24m";
>>> };
>>>
>>> U-Boot registers the clock as "clock-osc-24m", derived from the node name,
>>> ignoring the clock-output-names property.
>>>
>>> This differs from Linux, which uses the clock-output-names property when
>>> assigning clock names. As a result, clock consumers expecting names
>>> like "osc_24m" (as defined in the property) may fail to resolve clocks
>>> correctly in U-Boot.
>>>
>>> Update the fixed-rate clock driver to check for the clock-output-names
>>> property and use it as the clock name if present. If not, the fallback
>>> remains the node name. This makes U-Boot behavior consistent with Linux.
>>
>> This part above ^ is fine.
>>
>> This part below v is wrong .
>>
>>> One concrete impact is on i.MX8MP, where USB clock lookup failed following
>>> commit b4734c9c333b ("clk: imx: Convert clock-osc-* back to osc_*").
>>>
>>> With this change applied, fixed-clocks are correctly registered with their
>>> expected names:
>>>
>>> u-boot=> clk dump
>>>    Rate               Usecnt      Name
>>> ------------------------------------------
>>>    32768                0        |-- osc_32k
>>>    24000000             5        |-- osc_24m
>>>
>>> This change restores i.MX8MP USB functionality by ensuring the proper clock
>>> names are used.
>>
>> This is wrong, because this is NOT how the clock look up works at all.
>>
>> The lookup works this way. Take for example:
>>
>> drivers/clk/imx/clk-imx8mp.c
>>
>> 240         clk_dm(IMX8MP_ARM_PLL_REF_SEL, imx_clk_mux(dev,
>> "arm_pll_ref_sel", base + 0x84, 0, 2, pll_ref_sels,
>> ARRAY_SIZE(pll_ref_sels)));
>>
>> pll_ref_sels:
>>
>>    21 static const char * const pll_ref_sels[] = { "osc_24m", "dummy",
>> "dummy", "dummy", };
>>
>> the "osc_24m" is looked up in the clock-controller@30380000 clock-names
>> property:
>>
>> dts/upstream/src/arm64/freescale/imx8mp.dtsi
>>
>>    745 clk: clock-controller@30380000 {
>> ...
>>    751         clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
>>    752                  <&clk_ext3>, <&clk_ext4>;
>>    753         clock-names = "osc_32k", "osc_24m", ...
>>                                          ^^^^^^^
>>
>> The offset of osc_24m in that clock-names property is 1 , so the phandle
>> in clocks property array at offset 1 is used to look up those clock:
>>
>>    745 clk: clock-controller@30380000 {
>> ...
>>    751         clocks = <&osc_32k>, <&osc_24m>,
>>                                      ^^^^^^^^
>>
>> Which leads to this clock node in DT:
>>
>>    188         osc_24m: clock-osc-24m {
>>                ^^^^^^^^
>>    189                 compatible = "fixed-clock";
>>    190                 #clock-cells = <0>;
>>
>> The "clock-output-names" is NEVER used for any clock look up.
> 
> For what it's worth, I remove the "clock-out-names" from the device
> tree in Linux, and the Linux driver properly coped with it, but I am
> curious to know what the point of this entry is.

DT schema dtschema/schemas/clock/clock.yaml says:

  82   clock-output-names:
  83     description: |
  84       Recommended to be a list of strings of clock output signal
  85       names indexed by the first cell in the clock specifier.
  86       However, the meaning of clock-output-names is domain
  87       specific to the clock provider, and is only provided to
  88       encourage using the same meaning for the majority of clock
  89       providers.  This format may not work for clock providers
  90       using a complex clock specifier format.  In those cases it
  91       is recommended to omit this property and create a binding
  92       specific names property.
  93
  94       Clock consumer nodes must never directly reference
  95       the provider\'s clock-output-names property.

>>
>> ...
>>
>> I am fine with the first half of the description, but the second half is
>> wrong and this actually does not fix anything. Simply remove
>> clock-output-names from the osc_24m and the code will be broken again.
>>
>> The clk_resolve_parent_clk() is meant to perform the aforementioned
>> resolution(), that is what "[PATCH v2 00/24] clk: Add
>> clk_resolve_parent_clk() and fix up iMX clock drivers" actually fixed.
>> It seems there is something still missing and clk_resolve_parent_clk()
>> does not do the resolution properly for some clock, but that is what
>> needs to be understood and fixed, not worked around this way.
> 
> I added some debug code to clk_resolve_parent_clk it does return
> osc_24m as the name of the clock, but when searching for index, it
> appears to fail.

What exactly fails and where does it fail ?

> Do you have any suggestion on how searching for the
> index can query up the chain to the parent CCM driver on how to return
> the reference to proper clock?  The clock-names property is at the CCM
> parent and not inside the children, and the number of levels vary
> depending on the child clocks, you can't assume the parent's parent
> has the 'clock-name's property.

It is always the clock-controller@30380000 node to which the 
drivers/clk/imx/clk-imx8mp.c is bound , so drivers/clk/imx/clk-imx8mp.c 
does look up in the clock-controller@30380000 node clock-names and 
clocks . And from there on, it is always on phandle to &osc_24m , right ?

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Marek Vasut]

On 4/19/25 9:11 PM, Fabio Estevam wrote:
> Hi Marek,
> 
> On Fri, Apr 18, 2025 at 11:53 AM Marek Vasut <marex@denx.de> wrote:
> 
>> The "clock-output-names" is NEVER used for any clock look up.
> 
> In Linux, "clock-output-names" is used to register the name of the
> fixed-rate clock.
> 
>  From Linux drivers/clk/clk-fixed-rate.c:
> 
> ```
> static struct clk_hw *_of_fixed_clk_setup(struct device_node *node)
> {
> ....
> 
> of_property_read_string(node, "clock-output-names", &clk_name);
> 
> hw = clk_hw_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
>      0, rate, accuracy);
> ```
> 
> In U-Boot, we should register a fixed-rate clock with the same name as in Linux.
> 
> This patch aims to make the U-Boot fixed-rate clock name conform to
> Linux standards.
> 
>> I am fine with the first half of the description, but the second half is
>> wrong and this actually does not fix anything. Simply remove
>> clock-output-names from the osc_24m and the code will be broken again.
> 
> In Linux, if "clock-output-names" is removed from the osc_24m node,
> the console becomes corrupted
> on an imx8mp-evk.

This is likely unrelated bug, see reply from Adam.

>> The clk_resolve_parent_clk() is meant to perform the aforementioned
>> resolution(), that is what "[PATCH v2 00/24] clk: Add
>> clk_resolve_parent_clk() and fix up iMX clock drivers" actually fixed.
>> It seems there is something still missing and clk_resolve_parent_clk()
>> does not do the resolution properly for some clock, but that is what
>> needs to be understood and fixed, not worked around this way.
> 
> Please try to reproduce the problem on your end.
> 
> On an imx8mp-based board, run 'usb start' in U-Boot, and this command will fail.
I'll see what I can do tomorrow-ish .

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Marek Vasut]

[-- Attachment #1: Type: text/plain, Size: 4616 bytes --]

On 4/19/25 10:40 PM, Adam Ford wrote:
> On Sat, Apr 19, 2025 at 2:12 PM Fabio Estevam <festevam@gmail.com> wrote:
>>
>> Hi Marek,
>>
>> On Fri, Apr 18, 2025 at 11:53 AM Marek Vasut <marex@denx.de> wrote:
>>
>>> The "clock-output-names" is NEVER used for any clock look up.
>>
>> In Linux, "clock-output-names" is used to register the name of the
>> fixed-rate clock.
>>
>>  From Linux drivers/clk/clk-fixed-rate.c:
>>
>> ```
>> static struct clk_hw *_of_fixed_clk_setup(struct device_node *node)
>> {
>> ....
>>
>> of_property_read_string(node, "clock-output-names", &clk_name);
>>
>> hw = clk_hw_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
>>      0, rate, accuracy);
>> ```
>>
>> In U-Boot, we should register a fixed-rate clock with the same name as in Linux.
>>
>> This patch aims to make the U-Boot fixed-rate clock name conform to
>> Linux standards.
>>
>>> I am fine with the first half of the description, but the second half is
>>> wrong and this actually does not fix anything. Simply remove
>>> clock-output-names from the osc_24m and the code will be broken again.
>>
>> In Linux, if "clock-output-names" is removed from the osc_24m node,
>> the console becomes corrupted
>> on an imx8mp-evk.
>>
>>> The clk_resolve_parent_clk() is meant to perform the aforementioned
>>> resolution(), that is what "[PATCH v2 00/24] clk: Add
>>> clk_resolve_parent_clk() and fix up iMX clock drivers" actually fixed.
>>> It seems there is something still missing and clk_resolve_parent_clk()
>>> does not do the resolution properly for some clock, but that is what
>>> needs to be understood and fixed, not worked around this way.
>>
>> Please try to reproduce the problem on your end.
>>
>> On an imx8mp-based board, run 'usb start' in U-Boot, and this command will fail.
> 
>  From what I can tell, on the imx8mp, the usb_phy_ref parent->dev->name
> value s "clock-osc-24m" which never matches "osc_24" and I put some
> debug code into clk_mux_fetch_parent_index.
> 
> I think clk_mux_fetch_parent_index needs some additional logic to go
> up the chain to the root node and look for 'clock-names' to match in
> addition to what it already does. I am just not sure of the best
> method to go up the tree to find the root node that defines it.
[...]

Try something like this (also attached), maybe this needs to be made 
generic ?

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index 9e3b5191767..207224b1320 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -155,6 +155,8 @@ struct clk *clk_register_composite(struct udevice 
*dev, const char *name,
  		goto err;
  	}

+	composite->dev = dev;
+
  	if (composite->mux)
  		composite->mux->dev = clk->dev;
  	if (composite->rate)
diff --git a/drivers/clk/imx/clk-composite-8m.c 
b/drivers/clk/imx/clk-composite-8m.c
index 14c5b92939c..e1a3c0af308 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -116,14 +116,42 @@ static const struct clk_ops 
imx8m_clk_composite_divider_ops = {
  	.set_rate = imx8m_clk_composite_divider_set_rate,
  };

+static int imx8m_clk_mux_fetch_parent_index(struct udevice *cdev, 
struct clk *clk, struct clk *parent)
+{
+	struct clk_mux *mux = to_clk_mux(clk);
+	struct clk cclk;
+	int ret;
+	int i;
+
+	if (!parent)
+		return -EINVAL;
+
+	for (i = 0; i < mux->num_parents; i++) {
+		ret = clk_get_by_name(cdev, mux->parent_names[i], &cclk);
+		if (!ret && ofnode_equal(dev_ofnode(parent->dev), dev_ofnode(cclk.dev)))
+			return i;
+
+		if (!strcmp(parent->dev->name, mux->parent_names[i]))
+			return i;
+		if (!strcmp(parent->dev->name,
+			    clk_resolve_parent_clk(clk->dev,
+						   mux->parent_names[i])))
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+
  static int imx8m_clk_mux_set_parent(struct clk *clk, struct clk *parent)
  {
  	struct clk_mux *mux = to_clk_mux(clk);
+	struct clk_composite *composite = (struct clk_composite *)clk->data;
  	int index;
  	u32 val;
  	u32 reg;

-	index = clk_mux_fetch_parent_index(clk, parent);
+	index = imx8m_clk_mux_fetch_parent_index(composite->dev, clk, parent);
  	if (index < 0) {
  		log_err("Could not fetch index\n");
  		return index;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5ea2171492e..267757939e0 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -219,6 +219,8 @@ struct clk_composite {
  	const struct clk_ops	*mux_ops;
  	const struct clk_ops	*rate_ops;
  	const struct clk_ops	*gate_ops;
+
+	struct udevice *dev;
  };

  #define to_clk_composite(_clk) container_of(_clk, struct 
clk_composite, clk)

[-- Attachment #2: clk.diff --]
[-- Type: text/x-patch, Size: 2250 bytes --]

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index 9e3b5191767..207224b1320 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -155,6 +155,8 @@ struct clk *clk_register_composite(struct udevice *dev, const char *name,
 		goto err;
 	}
 
+	composite->dev = dev;
+
 	if (composite->mux)
 		composite->mux->dev = clk->dev;
 	if (composite->rate)
diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index 14c5b92939c..e1a3c0af308 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -116,14 +116,42 @@ static const struct clk_ops imx8m_clk_composite_divider_ops = {
 	.set_rate = imx8m_clk_composite_divider_set_rate,
 };
 
+static int imx8m_clk_mux_fetch_parent_index(struct udevice *cdev, struct clk *clk, struct clk *parent)
+{
+	struct clk_mux *mux = to_clk_mux(clk);
+	struct clk cclk;
+	int ret;
+	int i;
+
+	if (!parent)
+		return -EINVAL;
+
+	for (i = 0; i < mux->num_parents; i++) {
+		ret = clk_get_by_name(cdev, mux->parent_names[i], &cclk);
+		if (!ret && ofnode_equal(dev_ofnode(parent->dev), dev_ofnode(cclk.dev)))
+			return i;
+
+		if (!strcmp(parent->dev->name, mux->parent_names[i]))
+			return i;
+		if (!strcmp(parent->dev->name,
+			    clk_resolve_parent_clk(clk->dev,
+						   mux->parent_names[i])))
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+
 static int imx8m_clk_mux_set_parent(struct clk *clk, struct clk *parent)
 {
 	struct clk_mux *mux = to_clk_mux(clk);
+	struct clk_composite *composite = (struct clk_composite *)clk->data;
 	int index;
 	u32 val;
 	u32 reg;
 
-	index = clk_mux_fetch_parent_index(clk, parent);
+	index = imx8m_clk_mux_fetch_parent_index(composite->dev, clk, parent);
 	if (index < 0) {
 		log_err("Could not fetch index\n");
 		return index;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5ea2171492e..267757939e0 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -219,6 +219,8 @@ struct clk_composite {
 	const struct clk_ops	*mux_ops;
 	const struct clk_ops	*rate_ops;
 	const struct clk_ops	*gate_ops;
+
+	struct udevice *dev;
 };
 
 #define to_clk_composite(_clk) container_of(_clk, struct clk_composite, clk)

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Fabio Estevam]

On Sat, Apr 19, 2025 at 9:48 PM Marek Vasut <marex@denx.de> wrote:

> Try something like this (also attached), maybe this needs to be made
> generic ?

It works for me:

Tested-by: Fabio Estevam <festevam@gmail.com>

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Marek Vasut]

On 4/20/25 3:07 AM, Fabio Estevam wrote:
> On Sat, Apr 19, 2025 at 9:48 PM Marek Vasut <marex@denx.de> wrote:
> 
>> Try something like this (also attached), maybe this needs to be made
>> generic ?
> 
> It works for me:
> 
> Tested-by: Fabio Estevam <festevam@gmail.com>

Can you send that as a fix ?

And also, fix this patch up and drop the bogus second half of the commit 
message and do a V3.

Thanks

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Adam Ford]

On Sun, Apr 20, 2025 at 2:20 PM Marek Vasut <marex@denx.de> wrote:
>
> On 4/20/25 3:07 AM, Fabio Estevam wrote:
> > On Sat, Apr 19, 2025 at 9:48 PM Marek Vasut <marex@denx.de> wrote:
> >
> >> Try something like this (also attached), maybe this needs to be made
> >> generic ?

Thanks for doing this.

> >
> > It works for me:
> >
> > Tested-by: Fabio Estevam <festevam@gmail.com>
>
> Can you send that as a fix ?
>
> And also, fix this patch up and drop the bogus second half of the commit
> message and do a V3.

Feel free to add:

Tested-by: Adam Ford <aford173@gmail.com> # imx8mp-beacon

adam
>
> Thanks

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Fabio Estevam]

Hi Marek,

On Sun, Apr 20, 2025 at 4:20 PM Marek Vasut <marex@denx.de> wrote:

> Can you send that as a fix ?
>
> And also, fix this patch up and drop the bogus second half of the commit
> message and do a V3.

Sorry for the delay.

I found another issue on an imx8mm-evk board.

U-Boot SPL 2025.04-01379-g10f48365112b-dirty (Apr 24 2025 - 22:56:59 -0300)
No pmic
WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
SEC0:  RNG instantiated
Trying to boot from MMC1

(Pause here for about 3 seconds)

NOTICE:  Do not release JR0 to NS as it can be used by HAB
NOTICE:  BL31: v2.12.0(release):v2.12.0
NOTICE:  BL31: Built : 15:28:24, Apr 24 2025


U-Boot 2025.04-01379-g10f48365112b-dirty (Apr 24 2025 - 22:56:59 -0300)

CPU:   NXP i.MX8MMQ Rev1.0 A53 at 1200 MHz
CPU:   Consumer temperature grade (0C to 95C) at 37C
Model: FSL i.MX8MM EVK board
DRAM:  2 GiB
Core:  188 devices, 24 uclasses, devicetree: separate
WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
MMC:   FSL_SDHC: 1, FSL_SDHC: 2
Loading Environment from MMC... Reading from MMC(1)... OK
In:    serial@30890000
Out:   serial@30890000
Err:   serial@30890000
SEC0:  RNG instantiated
Net:   eth0: ethernet@30be0000
Hit any key to stop autoboot:  0
u-boot=>

The issue above is the long pause after "Trying to boot from MMC1".

If I manually revert commit b4734c9c333b ("clk: imx: Convert
clock-osc-* back to osc_*"), then it boots normally, without the long
pause.

Applying your patch helped resolve the USB issue, but not the long pause issue.

Thanks

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Marek Vasut]

On 4/25/25 4:02 AM, Fabio Estevam wrote:
> Hi Marek,
> 
> On Sun, Apr 20, 2025 at 4:20 PM Marek Vasut <marex@denx.de> wrote:
> 
>> Can you send that as a fix ?
>>
>> And also, fix this patch up and drop the bogus second half of the commit
>> message and do a V3.
> 
> Sorry for the delay.
> 
> I found another issue on an imx8mm-evk board.
> 
> U-Boot SPL 2025.04-01379-g10f48365112b-dirty (Apr 24 2025 - 22:56:59 -0300)
> No pmic
> WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
> SEC0:  RNG instantiated
> Trying to boot from MMC1
> 
> (Pause here for about 3 seconds)
> 
> NOTICE:  Do not release JR0 to NS as it can be used by HAB
> NOTICE:  BL31: v2.12.0(release):v2.12.0
> NOTICE:  BL31: Built : 15:28:24, Apr 24 2025
> 
> 
> U-Boot 2025.04-01379-g10f48365112b-dirty (Apr 24 2025 - 22:56:59 -0300)
> 
> CPU:   NXP i.MX8MMQ Rev1.0 A53 at 1200 MHz
> CPU:   Consumer temperature grade (0C to 95C) at 37C
> Model: FSL i.MX8MM EVK board
> DRAM:  2 GiB
> Core:  188 devices, 24 uclasses, devicetree: separate
> WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
> MMC:   FSL_SDHC: 1, FSL_SDHC: 2
> Loading Environment from MMC... Reading from MMC(1)... OK
> In:    serial@30890000
> Out:   serial@30890000
> Err:   serial@30890000
> SEC0:  RNG instantiated
> Net:   eth0: ethernet@30be0000
> Hit any key to stop autoboot:  0
> u-boot=>
> 
> The issue above is the long pause after "Trying to boot from MMC1".
> 
> If I manually revert commit b4734c9c333b ("clk: imx: Convert
> clock-osc-* back to osc_*"), then it boots normally, without the long
> pause.
> 
> Applying your patch helped resolve the USB issue, but not the long pause issue.
Probably some timeout in the esdhc driver, maybe some clock are not 
enabled ? I don't see it on the MX8MP.

Did you already apply
Re: [PATCH] clk: imx: Finish converting clock-osc-24 back to osc_24

?

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Fabio Estevam]

On Thu, Apr 24, 2025 at 11:20 PM Marek Vasut <marex@denx.de> wrote:

> Probably some timeout in the esdhc driver, maybe some clock are not
> enabled ? I don't see it on the MX8MP.

Same here. I didn't see this error on an imx8mp-evk.

>
> Did you already apply
> Re: [PATCH] clk: imx: Finish converting clock-osc-24 back to osc_24

Yes, I applied this one locally, but it didn't make a difference.

I haven't had a chance to debug this issue yet.

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Adam Ford]

On Thu, Apr 24, 2025 at 9:35 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Thu, Apr 24, 2025 at 11:20 PM Marek Vasut <marex@denx.de> wrote:
>
> > Probably some timeout in the esdhc driver, maybe some clock are not
> > enabled ? I don't see it on the MX8MP.
>
> Same here. I didn't see this error on an imx8mp-evk.

If memory serves, I thought SPL used the Boot ROM on Plus and Nano to
load U-Boot where the Mini has an older boot ROM and required more
driver initialization.

I could be wrong.  I'll try to test my Mini this weekend.

adam
>
> >
> > Did you already apply
> > Re: [PATCH] clk: imx: Finish converting clock-osc-24 back to osc_24
>
> Yes, I applied this one locally, but it didn't make a difference.
>
> I haven't had a chance to debug this issue yet.

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Adam Ford]

On Thu, Apr 24, 2025 at 9:37 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Thu, Apr 24, 2025 at 9:35 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > On Thu, Apr 24, 2025 at 11:20 PM Marek Vasut <marex@denx.de> wrote:
> >
> > > Probably some timeout in the esdhc driver, maybe some clock are not
> > > enabled ? I don't see it on the MX8MP.
> >
> > Same here. I didn't see this error on an imx8mp-evk.
>
> If memory serves, I thought SPL used the Boot ROM on Plus and Nano to
> load U-Boot where the Mini has an older boot ROM and required more
> driver initialization.
>
> I could be wrong.  I'll try to test my Mini this weekend.

I pulled my mini out.  With Marek's clk.diff file applied on top of
U-Boot next, it boots quickly without delay, and I don't get a PMIC
error.  I don't have an EVK at my home lab to test.


U-Boot SPL 2025.04-rc5-00835-g61128a5b704d-dirty (Apr 24 2025 - 21:53:22 -0500)
WDT:   Not starting watchdog@30280000
Trying to boot from MMC1
NOTICE:  Do not release JR0 to NS as it can be used by HAB
NOTICE:  BL31: v2.10.14(release):lts-v2.10.14-380-gd7da75fef
NOTICE:  BL31: Built : 20:12:47, Apr  3 2025


U-Boot 2025.04-rc5-00835-g61128a5b704d-dirty (Apr 24 2025 - 21:53:22 -0500)

CPU:   NXP i.MX8MMQ Rev1.0 A53 at 1200 MHz
CPU:   Industrial temperature grade (-40C to 105C) at 30C
Model: Beacon EmbeddedWorks i.MX8M Mini Development Kit
DRAM:  2 GiB
Core:  225 devices, 26 uclasses, devicetree: separate
WDT:   Not starting watchdog@30280000
MMC:   FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
Loading Environment from MMC... Reading from MMC(2)... OK
In:    serial@30890000
Out:   serial@30890000
Err:   serial@30890000
SEC0:  RNG instantiated
Net:   eth0: ethernet@30be0000
Hit any key to stop autoboot:  0
u-boot=>



>
> adam
> >
> > >
> > > Did you already apply
> > > Re: [PATCH] clk: imx: Finish converting clock-osc-24 back to osc_24
> >
> > Yes, I applied this one locally, but it didn't make a difference.
> >
> > I haven't had a chance to debug this issue yet.

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Fabio Estevam]

On Thu, Apr 24, 2025 at 11:35 PM Fabio Estevam <festevam@gmail.com> wrote:

> I haven't had a chance to debug this issue yet.

I applied this debug patch:

--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -1539,6 +1539,7 @@ static int fsl_esdhc_probe(struct udevice *dev)
        }

        priv->sdhc_clk = clk_get_rate(&priv->per_clk);
+       printf("****** sdhc clk is %d\n", priv->sdhc_clk);
 #else
        init_clk_usdhc(dev_seq(dev));

With the top-of-tree U-Boot, SPL calculates the wrong sdhc_clk on the
imx8mm-evk:

U-Boot SPL 2025.04-01380-ga9820e12db18-dirty (Apr 25 2025 - 09:40:45 -0300)
No pmic
WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
SEC0:  RNG instantiated
Trying to boot from MMC1
****** sdhc clk is 204522251

If I revert b4734c9c333b ("clk: imx: Convert clock-osc-* back to
osc_*"), the clock is calculated correctly:

U-Boot 2025.04-01380-ga9820e12db18-dirty (Apr 25 2025 - 09:40:45 -0300)

CPU:   NXP i.MX8MMQ Rev1.0 A53 at 1200 MHz
CPU:   Consumer temperature grade (0C to 95C) at 42C
Model: FSL i.MX8MM EVK board
DRAM:  2 GiB
Core:  188 devices, 24 uclasses, devicetree: separate
WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
MMC:   fsl_esdhc mmc@30b50000: no vqmmc-supply
****** sdhc clk is 200000000

If I do:

--- a/dts/upstream/src/arm64/freescale/imx8mm-evk.dtsi
+++ b/dts/upstream/src/arm64/freescale/imx8mm-evk.dtsi
@@ -614,8 +614,6 @@
 };

 &usdhc2 {
-       assigned-clocks = <&clk IMX8MM_CLK_USDHC2>;
-       assigned-clock-rates = <200000000>;
        pinctrl-names = "default", "state_100mhz", "state_200mhz";
        pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>;
        pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>;

Then the pause is gone, but the printed clock is incorrect:

U-Boot SPL 2025.04-01380-ga9820e12db18-dirty (Apr 25 2025 - 09:52:28 -0300)
No pmic
WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
SEC0:  RNG instantiated
Trying to boot from MMC1
****** sdhc clk is 1431655607

Adam, imx8mm-beacon does not have the
assigned-clocks/assigned-clock-rates properties.

Maybe this is why you haven't observed the problem?

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Fabio Estevam]

On Fri, Apr 25, 2025 at 9:55 AM Fabio Estevam <festevam@gmail.com> wrote:

> With the top-of-tree U-Boot, SPL calculates the wrong sdhc_clk on the
> imx8mm-evk:
>
> U-Boot SPL 2025.04-01380-ga9820e12db18-dirty (Apr 25 2025 - 09:40:45 -0300)
> No pmic
> WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
> SEC0:  RNG instantiated
> Trying to boot from MMC1
> ****** sdhc clk is 204522251

I have just sent a series that fixes this problem.

It contains some clock patches from Marek that I missed applying
earlier, as well as an IMX8MM SPL OSC24 M clock fix.

Marek, care to submit your clk.diff patch formally so that the i.MX8MP
USB functionality could be restored?

Thanks

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Marek Vasut]

On 4/25/25 2:55 PM, Fabio Estevam wrote:
> On Thu, Apr 24, 2025 at 11:35 PM Fabio Estevam <festevam@gmail.com> wrote:
> 
>> I haven't had a chance to debug this issue yet.
> 
> I applied this debug patch:
> 
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -1539,6 +1539,7 @@ static int fsl_esdhc_probe(struct udevice *dev)
>          }
> 
>          priv->sdhc_clk = clk_get_rate(&priv->per_clk);
> +       printf("****** sdhc clk is %d\n", priv->sdhc_clk);
>   #else
>          init_clk_usdhc(dev_seq(dev));
> 
> With the top-of-tree U-Boot, SPL calculates the wrong sdhc_clk on the
> imx8mm-evk:
> 
> U-Boot SPL 2025.04-01380-ga9820e12db18-dirty (Apr 25 2025 - 09:40:45 -0300)
> No pmic
> WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
> SEC0:  RNG instantiated
> Trying to boot from MMC1
> ****** sdhc clk is 204522251

Look at cmd/clk.c and import soc_clk_dump() into this driver, and then 
call it here, so you would get the current topology of the clock tree. 
What do you see there in the output ? Please include it here.

Re: [PATCH v2] clk: fixed-rate: Use "clock-output-names" to name fixed clocks

[Marek Vasut]

On 4/25/25 3:55 PM, Fabio Estevam wrote:
> On Fri, Apr 25, 2025 at 9:55 AM Fabio Estevam <festevam@gmail.com> wrote:
> 
>> With the top-of-tree U-Boot, SPL calculates the wrong sdhc_clk on the
>> imx8mm-evk:
>>
>> U-Boot SPL 2025.04-01380-ga9820e12db18-dirty (Apr 25 2025 - 09:40:45 -0300)
>> No pmic
>> WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
>> SEC0:  RNG instantiated
>> Trying to boot from MMC1
>> ****** sdhc clk is 204522251
> 
> I have just sent a series that fixes this problem.
> 
> It contains some clock patches from Marek that I missed applying
> earlier, as well as an IMX8MM SPL OSC24 M clock fix.
> 
> Marek, care to submit your clk.diff patch formally so that the i.MX8MP
> USB functionality could be restored?
If it is OK with you, send it as part of the fixup series, else I will 
send it sometimes next week.