Re: [PATCH] clk: nuvoton: ma35d1: Use clk_hw pointers as mux parents

[Jacky Huang <ychuang570808@gmail.com>]

>> The MA35D1 clock provider registers its muxes with parent data
>> structures filling .fw_name. This is not the ideal approach since that
>> would require a massive amount of internal clock names declaration in
>> the DT. Since the DT does not play the game of exposing all these names,
>> none of the parent lookups performed when instantiating the muxes
>> succeed. As a result, these muxes get registered as root clocks, leading
>> to a sadly flat clock tree and no frequency assigned to most of the
>> peripheral clocks:
> Jacky, this is an actual fix which is a month old now, the SPI
> controller (and maybe other blocks as well) does not work without this
> patch. Would you mind giving this a bit of feedback?
>

Hi Miquel,

Thanks for working on this clock parent conversion and for testing the
MA35D1 hardware.

While reviewing the conversion against the existing clk_parent_data
tables and section 6.5 of the MA35D1 TRM, I noticed an existing issue in
the WDT/WWDT clock definitions.

The original driver contains valid parent names:

     "pclk3_div4096"
     "pclk4_div4096"

For example:

     static const struct clk_parent_data wdt1_sel_clks[] = {
             { .index = -1, },
             { .fw_name = "lxt", },
             { .fw_name = "pclk3_div4096", },
             { .fw_name = "lirc", },
     };

The TRM lists PCLK3/4096 and PCLK4/4096 as valid selectable parent
clocks for the WDT/WWDT blocks.

However, the new conversion changes these entries to -1:

     static const int wdt1_parent_idx[] = {
             -1, LXT, -1, LIRC
     };

This loses a valid hardware parent. I understand that the current
driver does not register corresponding clocks, so this is an existing
driver issue that became visible during this conversion.

Would it be possible to cover this in the series? One possible
implementation would be to add a preparatory patch that registers these
fixed-factor clocks before converting the WDT/WWDT parent tables:

     hws[PCLK3_DIV4096] =
             ma35d1_clk_fixed_factor(dev, "pclk3_div4096",
                                     "pclk3", 1, 4096);

     hws[PCLK4_DIV4096] =
             ma35d1_clk_fixed_factor(dev, "pclk4_div4096",
                                     "pclk4", 1, 4096);

The corresponding clock IDs should be appended to
include/dt-bindings/clock/nuvoton,ma35d1-clk.h without renumbering any
existing IDs, for example:

     #define PCLK3_DIV4096  236
     #define PCLK4_DIV4096  237
     #define CLK_MAX_IDX   238

The WDT/WWDT parent tables could then preserve the original mappings,
for example:

     static const int wdt1_parent_idx[] = {
             -1, LXT, PCLK3_DIV4096, LIRC
     };

This could be submitted as patch 1/2, with the parent conversion as
patch 2/2, although please use your judgement on the final patch
organisation.

The other parent_idx definitions and their ordering match the existing
clock parent tables.
I have also tested this version on an MA35D1 board, and the clock
parent conversion appears to work correctly for the peripherals I
tested.

Thanks again for working on this.

Best regards,
Jacky Huang