Messages in this thread | | | Date | Fri, 9 Jun 2023 09:42:13 +0200 | Subject | Re: [PATCH v2 3/5] media: mediatek: vcodec: Read HW active status from clock | From | AngeloGioacchino Del Regno <> |
| |
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 >>>> @@ -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? >
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.
That's *synthetically* the whole story...
>> >> 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. >>
| |