From: Alexandre Mergnat <amergnat@baylibre.com>
To: AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Chen-Yu Tsai <wenst@chromium.org>
Cc: Markus Schneider-Pargmann <msp@baylibre.com>,
linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org
Subject: Re: [PATCH v2 2/2] clk: mediatek: mt8365: Fix index issue
Date: Fri, 26 May 2023 11:38:53 +0200 [thread overview]
Message-ID: <d2ed4cce-108f-c861-5f84-0c7ac5954346@baylibre.com> (raw)
In-Reply-To: <2a60740f-782d-08d5-f62f-dcc67aaf4d32@collabora.com>
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
WARNING: multiple messages have this Message-ID (diff)
From: Alexandre Mergnat <amergnat@baylibre.com>
To: AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Chen-Yu Tsai <wenst@chromium.org>
Cc: Markus Schneider-Pargmann <msp@baylibre.com>,
linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org
Subject: Re: [PATCH v2 2/2] clk: mediatek: mt8365: Fix index issue
Date: Fri, 26 May 2023 11:38:53 +0200 [thread overview]
Message-ID: <d2ed4cce-108f-c861-5f84-0c7ac5954346@baylibre.com> (raw)
In-Reply-To: <2a60740f-782d-08d5-f62f-dcc67aaf4d32@collabora.com>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-05-26 9:40 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-25 14:50 [PATCH v2 0/2] Fix and clean MT8365 clock indexes Alexandre Mergnat
2023-05-25 14:50 ` Alexandre Mergnat
2023-05-25 14:50 ` [PATCH v2 1/2] dt-bindings: clock: mediatek: replace unusable clock Alexandre Mergnat
2023-05-25 14:50 ` Alexandre Mergnat
2023-05-25 17:51 ` Conor Dooley
2023-05-25 17:51 ` Conor Dooley
2023-05-26 8:54 ` Alexandre Mergnat
2023-05-26 8:54 ` Alexandre Mergnat
2023-05-26 11:39 ` Conor Dooley
2023-05-26 11:39 ` Conor Dooley
2023-05-26 8:30 ` AngeloGioacchino Del Regno
2023-05-26 8:30 ` AngeloGioacchino Del Regno
2023-05-26 9:46 ` Alexandre Mergnat
2023-05-26 9:46 ` Alexandre Mergnat
2023-05-25 14:50 ` [PATCH v2 2/2] clk: mediatek: mt8365: Fix index issue Alexandre Mergnat
2023-05-25 14:50 ` Alexandre Mergnat
2023-05-26 8:33 ` AngeloGioacchino Del Regno
2023-05-26 8:33 ` AngeloGioacchino Del Regno
2023-05-26 9:38 ` Alexandre Mergnat [this message]
2023-05-26 9:38 ` Alexandre Mergnat
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d2ed4cce-108f-c861-5f84-0c7ac5954346@baylibre.com \
--to=amergnat@baylibre.com \
--cc=angelogioacchino.delregno@collabora.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=msp@baylibre.com \
--cc=mturquette@baylibre.com \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
--cc=wenst@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.