Re: [RFC PATCH 2/2] clk: mediatek: Add frequency hopping support

[Boris Lysov <arz65xx@gmail.com>]

On Thu, 14 Jul 2022 13:04:49 +0200
AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Il 06/07/22 15:07, Edward-JW Yang ha scritto:
> > On Wed, 2022-06-29 at 16:54 +0800, Chen-Yu Tsai wrote:
> >> On Tue, Jun 28, 2022 at 6:09 PM AngeloGioacchino Del Regno
> >> <angelogioacchino.delregno@collabora.com> wrote:
> >>>
> >>> Il 24/06/22 09:12, Edward-JW Yang ha scritto:
> >>>> Hi AngeloGioacchino,
> >>>>
> >>>> Thanks for all the advices.
> >>>>
> >>>> On Mon, 2022-06-13 at 17:43 +0800, AngeloGioacchino Del Regno wrote:
> >>>>> Il 12/06/22 15:54, Johnson Wang ha scritto:
> >>>>>> Add frequency hopping support and spread spectrum clocking
> >>>>>> control for MT8186.
> >>>>>>
> >>>>>> Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> >>>>>> Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> >>>>>
> >>>>> 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..........
> >>>
> >>>> 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