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

From: Stephen Boyd <sboyd@kernel.org>
To: "AngeloGioacchino Del Regno"
	<angelogioacchino.delregno@collabora.com>,
	"Chen-Yu Tsai" <wenst@chromium.org>,
	"Nícolas F. R. A. Prado" <nfraprado@collabora.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	kernel@collabora.com,
	Andrew-CT Chen <andrew-ct.chen@mediatek.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Tiffany Lin <tiffany.lin@mediatek.com>,
	Yunfei Dong <yunfei.dong@mediatek.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH v2 3/5] media: mediatek: vcodec: Read HW active status from clock
Date: Mon, 12 Jun 2023 12:19:54 -0700	[thread overview]
Message-ID: <d579dc00ed9877f9daf170134fe781e6.sboyd@kernel.org> (raw)
In-Reply-To: <90781ea3-d43a-6267-278c-184050fe456e@collabora.com>

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@collabora.com> wrote:
> >>>> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
> >>>> index 9c652beb3f19..8038472fb67b 100644
> >>>> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
> >>>> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
> >>>
> >>> AFAIK this is still around for clk drivers that haven't moved to clk_hw.
> >>> It shouldn't be used by clock consumers. Would it be better to just pass
> >>> a syscon?
> >>>
> >>
> >> This is a legit usage of __clk_is_enabled().... because that's what we're really
> >> doing here, we're checking if a clock got enabled by the underlying MCU (as that
> >> clock goes up after the VDEC boots).
> >>
> >> If this is *not* acceptable as it is, we will have to add a clock API call to
> >> check if a clock is enabled... but it didn't seem worth doing since we don't
> >> expect anyone else to have any legit usage of that, or at least, we don't know
> >> about anyone else needing that...
> > 
> > The design of the clk.h API has been that no clk consumer should need to
> > find out if a clk is enabled. Instead, the clk consumer should enable
> > the clk if they want it enabled. Is there no other way to know that the
> > vcodec hardware is active?
> > 
> 
> 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.

Another approach could be to wait for some amount of time after telling
firmware to power up and assume the hardware is active.

----

I see that the __clk_is_enabled() API is being used in some other
consumer drivers. I think at one point we were down to one or two users.
I'll try to remove this function entirely, but it will still be possible
to get at the clk_hw for a clk with __clk_get_hw() and then call
clk_hw_is_enabled().

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