Aw: Re: BUG: [PATCH v2] isoc: mediatek: Check for error clk pointer

[Frank Wunderlich]

> Gesendet: Dienstag, 01. Februar 2022 um 23:19 Uhr
> Von: "Daniel Golle" <daniel at makrotopia.org>
> An: "Jiasheng Jiang" <jiasheng at iscas.ac.cn>
> Cc: justinkb at gmail.com, gregkh at linuxfoundation.org, linux-kernel at vger.kernel.org, lgirdwood at gmail.com, broonie at kernel.org, linux-mediatek at lists.infradead.org, matthias.bgg at gmail.com, linux-arm-kernel at lists.infradead.org
> Betreff: Re: BUG: [PATCH v2] isoc: mediatek: Check for error clk pointer
>
> On Sun, Jan 30, 2022 at 10:43:35AM +0800, Jiasheng Jiang wrote:
> > On Fri, Jan 28, 2022 at 10:26:51PM +0800, Paul Mulders wrote:
> > > I guess this breaks all MT7622 SoCs since it'll prematurely exit
> > > init_clks (and subsequently init_scp) completely once devm_clk_get
> > > fails to get a reference to the mm clock producer (which happens to be
> > > the first one tried). This is because MT7623 has a GPU (so no mm
> > > clock) and MT7622 doesn't, and as a result the other clock producer
> > > pointers never get initialized (and other stuff in init_scp after
> > > returning from the error never happens).
> > >
> > > The patch seems fundamentally flawed, I guess it was either not tested
> > > at all, or only tested on a MT7623. The initialization functions seem
> > > designed with the idea that it's ok if some clocks aren't present, so
> > > stopping the initialization when one of them isn't present seems
> > > wrong. (For example, there is also a MT7622B variant of the MT7622
> > > which probably also lacks some clocks MT7622(A) does have).
> >
> > I don't think the patch for init_clks() is flawed.
> > At most it is incompleted.
> > What it did is like fixing a potential error in the tool platform
> > providing service for the upper application, like what you said,
> > MT7623 and MT7622.
> > We should not keep the error in the platform because of the upper
> > application.
> > And it seems like it is MT7622 that is flawed.
> > The better way is to fix both the bug in init_clks() and its caller,
> > MT7622.
>
> I agree that further cleaning is needed here.
> Yet the commit in it's current form very obviously breaks at least the
> MT7622 platform if no further fixes are applied.
>
> Imho the whole approach of this driver to hard-code the names of all
> clocks it could try to grab in a string-array accompanied by an enum
> is flawed.
>
> The correct approach would likely be to add the clocks actually present
> to the device tree of each SoC and then grab only those.
>
> I can see that mt2701.dtsi and mt7623.dtsi both have some clocks
> defined for scpsys, mt7622.dtsi however lacks them.
> I didn't check other MediaTek SoCs which also use scpsys.

re-add me to discussion :)

have not yet tested mt7623 without the revert i applied for mt7622. will do this later

regards Frank