Re: [PATCH 1/2] pwm: meson: make full use of common clock framework

[Heiner Kallweit <hkallweit1@gmail.com>]

On 11.04.2023 01:27, Martin Blumenstingl wrote:
> Hi Heiner,
> 
> On Sat, Apr 8, 2023 at 10:43 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
> [...]
>> +               init.name = name;
>> +               init.ops = &clk_divider_ops;
>> +               init.flags = CLK_SET_RATE_PARENT;
>> +               parent_hws[0] = &channel->mux.hw;
>> +               init.parent_hws = parent_hws;
>> +               init.num_parents = 1;
> There's a very subtle bug in this code:
> You're re-using the same struct clk_init_data for all clocks.
> The mux above sets
> - init.parent_names
> - init.num_parents
> - (amongst others)
> 
I think we should initialize clk_init_data to all NULL before each use.



> but for the divider and gate you're only overwriting init.num_parents,
> which is why I end up with:
>  xtal                                 8        8        1    24000000
>         0     0  50000         Y
>    [...]
>    c1108650.pwm#gate1                1        1        0    24000000
>        0     0  50000         Y
>    c1108650.pwm#div1                 0        0        0    24000000
>        0     0  50000         Y
>    c1108650.pwm#mux1                 0        0        0    24000000
>        0     0  50000         Y
>    c1108650.pwm#gate0                1        1        0    24000000
>        0     0  50000         Y
>    c1108650.pwm#div0                 0        0        0    24000000
>        0     0  50000         Y
>    c1108650.pwm#mux0                 0        0        0    24000000
>        0     0  50000         Y
> 
Thanks a lot for testing and for the comprehensive feedback.

> My suggestion is to switch to struct clk_parent_data entirely (for the
> mux this could be done with a separate commit before this one).
> Then keep it consistent and don't mix parent_names/parent_hws and
> parent_data (just stick to parent_data, which can manage the previous
> two and more).
> I *think* struct clk_parent_data can even be used to simplify the
> second patch (by just having an "empty" entry - with index = -1 - in
> the array).
> 
After having looked at clk_fetch_parent_index() I think so too.

clk core seems to be a little inconsistent wrt handling parent_names
vs. parent_data.name in clk_core_populate_parent_map(). It complains
about NULL entries in parent_names but accepts parent_data entries
with index == -1 and everything else being NULL (incl. name).
It might be worth a clk core patch to allow NULL parent_names.

Benefit for us would be that we can simply replace the hdmi/video
clock entries with NULL, and avoid the effort of copying the clock
names to parent_data entries.

> [...]
>> +               channel->gate.bit_idx = __ffs(meson_pwm_per_channel_data[i].clk_en_mask);
> I's the only place where clk_en_mask is now used. So I think it's
> valid to just rename clk_en_mask to clk_en_idx and pass the value
> without the BIT macro during initialization.
> 
> 
> Best regards,
> Martin


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic