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:
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.
