On Thu, 14 Jul 2022 13:04:49 +0200
AngeloGioacchino Del Regno [off-list ref] wrote:

Il 06/07/22 15:07, Edward-JW Yang ha scritto:

quoted

On Wed, 2022-06-29 at 16:54 +0800, Chen-Yu Tsai wrote:

quoted

On Tue, Jun 28, 2022 at 6:09 PM AngeloGioacchino Del Regno
[off-list ref] wrote:

quoted

Il 24/06/22 09:12, Edward-JW Yang ha scritto:

quoted

Hi AngeloGioacchino,

Thanks for all the advices.

On Mon, 2022-06-13 at 17:43 +0800, AngeloGioacchino Del Regno wrote:

quoted

Il 12/06/22 15:54, Johnson Wang ha scritto:

quoted

Add frequency hopping support and spread spectrum clocking
control for MT8186.

Signed-off-by: Edward-JW Yang <redacted>
Signed-off-by: Johnson Wang <redacted>

Before going on with the review, there's one important consideration:
the Frequency Hopping control is related to PLLs only (so, no other
clock types get in the mix).

Checking the code, the *main* thing that we do here is initializing the
FHCTL by setting some registers, and we're performing the actual
frequency hopping operation in clk-pll, which is right but, at this
point, I think that the best way to proceed is to add the "FHCTL
superpowers" to clk-pll itself, instead of adding multiple new files
and devicetree bindings that are specific to the FHCTL itself.

This would mean that the `fh-id` and `perms` params that you're setting
in the devicetree get transferred to clk-mt8186 (and hardcoded there),
as to extend the PLL declarations to include these two: that will also
simplify the driver so that you won't have to match names here and
there.

Just an example:

      PLL(CLK_APMIXED_CCIPLL, "ccipll", 0x0224, 0x0230, 0,

          PLL_AO, 0, 22, 0x0228, 24, 0, 0, 0, 0x0228, 2,
FHCTL_PERM_DBG_DUMP),

Besides, there are another couple of reasons why you should do that
instead, of which:
    - The devicetree should be "generic enough", we shall not see the
direct value to write to the registers in there (yet, perms assigns
exactly that)
    - These values won't change on a per-device basis, I believe?
They're SoC-related, not board-related, right?

In case they're board related (and/or related to TZ permissions), we
can always add a bool property to the apmixedsys to advertise that
board X needs to use an alternative permission (ex.:
`mediatek,secure-fhctl`).

I think we should remain clk-fhctl files because FHCTL is a independent
HW and is not a necessary component of clk-pll.

I know what FHCTL is, but thank you anyway for the explanation, that's
appreciated. In any case, this not being a *mandatory* component doesn't
mean that when it is enabled it's not changing the way we manage the
PLLs..........

quoted

Frequency hopping function from FHCTL is not used to replace original
flow of set_rate in clk-pll. They are two different ways to change PLL's
frequency. The

I disagree: when we want to use FHCTL, we effectively hand-over PLL
control from APMIXEDSYS to the Frequency Hopping controller - and we're
effectively replacing the set_rate() logic of clk-pll.

Do you mean we need to drop the current set_rate() logic (direct register
write) and use Frequency Hopping Controller instead?

On PLLs that are supported by the Frequency Hopping controller, yes: we should
simply use a different .set_rate() callback in clk-pll.c, and we should return
a failure if the FHCTL fails to set the rate - so we should *not* fall back to
direct register writes, as on some platforms and in some conditions, using
direct register writes (which means that we skip FHCTL), may lead to unstable
system.

This means that we need logic such that, in mtk_clk_register_pll(), we end up
having something like that:

if (fhctl_is_enabled(pll))
	init.ops = &mtk_pll_fhctl_ops;
else
	init.ops = &mtk_pll_ops;

Looks like accepting my patch [1] wouldn't be a bad idea, after all.
[1] https://lists.infradead.org/pipermail/linux-mediatek/2022-May/041293.html

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