Re: [PATCH v2 3/5] media: mediatek: vcodec: Read HW active status from clock

[AngeloGioacchino Del Regno]

Il 15/06/23 02:40, Stephen Boyd ha scritto:
> Quoting AngeloGioacchino Del Regno (2023-06-14 01:13:43)
> > Il 12/06/23 21:19, Stephen Boyd ha scritto:
> > > Quoting AngeloGioacchino Del Regno (2023-06-09 00:42:13)
> > > > Il 09/06/23 01:56, Stephen Boyd ha scritto:
> > > > > Quoting AngeloGioacchino Del Regno (2023-06-08 02:01:58)
> > > > > > Il 08/06/23 10:12, Chen-Yu Tsai ha scritto:
> > > > > > > On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado
> > > > > > > <nfraprado@xxxxxxxxxxxxx> wrote:
> > > >
> > > > The firmware gives an indication of "boot done", but that's for the "core" part
> > > > of the vcodec... then it manages this clock internally to enable/disable the
> > > > "compute" IP of the decoder.
> > > >
> > > > As far as I know (and I've been researching about this) the firmware will not
> > > > give any "decoder powered, clocked - ready to get data" indication, and the
> > > > only way that we have to judge whether it is in this specific state or not is
> > > > to check if the "VDEC_ACTIVE" clock got enabled by the firmware.
> > >
> > > Is Linux ever going to use clk consumer APIs like clk_enable/clk_disable
> > > on this VDEC_ACTIVE clk? If the answer is no, then there isn't any
> > > reason to put it in the clk framework, and probably syscon is the way to
> > > go for now.
> >
> > Not for the current platform, but that may change in future SoCs... we're not sure.
>
> If you're not using the clk consumer APIs then it shouldn't be a clk.
>
> > > Another approach could be to wait for some amount of time after telling
> > > firmware to power up and assume the hardware is active.
> >
> > That would be highly error prone though. Expecting that the HW is alive means that
> > we're 100% sure that both firmware and driver are doing the right thing at every
> > moment, which is something that we'd like to assume but, realistically, for safety
> > reasons we just don't.
> >
> > Should we anyway go for a syscon *now* and then change it to a clock later, if any
> > new platform needs this as a clock?
>
> Yeah. Or implement this as a power domain and have it read the register
> directly waiting to return from the power_on()?

A power domain would force us to incorrectly describe the hardware in the bindings
though, I think... so, Nícolas, please, let's go for a syscon at this point, as it
really looks like being the only viable option.

Stephen, many thanks for the valuable suggestions and the nice conversation.

Cheers!
Angelo