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: Thu, 08 Jun 2023 16:56:28 -0700	[thread overview]
Message-ID: <83770481aa762b69738c27f9d9934dd9.sboyd@kernel.org> (raw)
In-Reply-To: <f0018817-d47b-d772-ed9f-9126bf71a0d1@collabora.com>

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
> >> @@ -16,6 +16,7 @@
> >>   #include <media/v4l2-mem2mem.h>
> >>   #include <media/videobuf2-dma-contig.h>
> >>   #include <media/v4l2-device.h>
> >> +#include <linux/clk-provider.h>
> > 
> >                     ^^^^^^^^^^^^^^
> > 
> > This seems like a violation of the API separation.

Yes.

> > 
> >>   #include "mtk_vcodec_drv.h"
> >>   #include "mtk_vcodec_dec.h"
> >> @@ -38,22 +39,29 @@ static int mtk_vcodec_get_hw_count(struct mtk_vcodec_dev *dev)
> >>          }
> >>   }
> >>
> >> +static bool mtk_vcodec_is_hw_active(struct mtk_vcodec_dev *dev)
> >> +{
> >> +       u32 cg_status = 0;
> >> +
> >> +       if (!dev->reg_base[VDEC_SYS])
> >> +               return __clk_is_enabled(dev->pm.vdec_active_clk);
> > 
> > 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?

> 
> As for the syscon, that's something that we've been discussing as well... the
> thing is: we're really *really* checking if a clock is enabled, so we should
> be using clock related calls... reading from a syscon means that we'd have to
> perform a register read (of.. again.. a clock) outside of the clock framework
> which, in my opinion, wouldn't be clean; I'd expect that to become a bit messy
> in the future too, should more MediaTek SoCs (I think MT8192/95 are already in
> the list, Nicolas please correct me if I'm wrong here) need the same thing, as
> we'd be adding more definitions around.
>

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