[PATCH 1/2] pwm: meson: make full use of common clock framework
Heiner Kallweit
hkallweit1 at gmail.com
Mon Apr 10 23:34:02 PDT 2023
On 11.04.2023 01:27, Martin Blumenstingl wrote:
> Hi Heiner,
>
> On Sat, Apr 8, 2023 at 10:43 PM Heiner Kallweit <hkallweit1 at 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
More information about the linux-amlogic
mailing list