Messages in this thread | | | Date | Fri, 26 May 2023 11:38:53 +0200 | Subject | Re: [PATCH v2 2/2] clk: mediatek: mt8365: Fix index issue | From | Alexandre Mergnat <> |
| |
On 26/05/2023 10:33, AngeloGioacchino Del Regno wrote: > Il 25/05/23 16:50, Alexandre Mergnat ha scritto: >> Before the patch [1], the clock probe was done directly in the >> clk-mt8365 driver. In this probe function, the array which stores the >> data clocks is sized using the higher defined numbers (*_NR_CLOCK) in >> the clock lists [2]. Currently, with the patch [1], the specific >> clk-mt8365 probe function is replaced by the mtk generic one [3], which >> size the clock data array by adding all the clock descriptor array size >> provided by the clk-mt8365 driver. >> >> Actually, all clock indexes come from the header file [2], that mean, if >> there are more clock (then more index) in the header file [2] than the >> number of clock declared in the clock descriptor arrays (which is the >> case currently), the clock data array will be undersized and then the >> generic probe function will overflow when it will try to write in >> "clk_data[CLK_INDEX]". Actually, instead of crashing at boot, the probe >> function returns an error in the log which looks like: >> "of_clk_hw_onecell_get: invalid index 135", then this clock isn't >> enabled. >> >> Solve this issue by adding in the driver the missing clocks declared in >> the header clock file [2]. >> >> [1]: Commit ffe91cb28f6a ("clk: mediatek: mt8365: Convert to >> mtk_clk_simple_{probe,remove}()") >> [2]: include/dt-bindings/clock/mediatek,mt8365-clk.h >> [3]: drivers/clk/mediatek/clk-mtk.c >> >> Fixes: ffe91cb28f6a ("clk: mediatek: mt8365: Convert to >> mtk_clk_simple_{probe,remove}()") > > This is not fixing the conversion, but the clock driver, as it > originally missed > clock entries and hence was not compliant with its binding (header). > It worked before, probably, but this doesn't mean that this driver > didn't contain > a logic mistake from the beginning :-) > > So, add (or replace the current one with) the relevant Fixes tag... >
Briefly and factually, the mt8365 clk probe mechanism was different
compared to the mtk clk driver. Even if it was an issue or not, it was
working (for sure). When [1] improved the mt8365 clk driver by using
the mtk clk generic probe, some clocks (USB here) no longer worked.
So, IMHO, it still a functional regression introduced by [1], because it
come from the switch of the probe function.
I'm not blaming & shaming the author of [1], as you said, it originally
missed clock entries and hence was not compliant with its binding
(whereas other MTK SoC was I guess). This commit is pointed thanks
to the bisect + test.
-- Regards, Alexandre
| |