On Thu, 14 Jul 2022 13:04:49 +0200 AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxx> > >>>>>> Signed-off-by: Johnson Wang <johnson.wang@xxxxxxxxxxxx> > >>>>> > >>>>> 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
