| Message ID | 20230607205714.510012-4-nfraprado@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable decoder for mt8183 | expand |
Il 07/06/23 22:53, Nícolas F. R. A. Prado ha scritto: > Remove the requirement of a VDEC_SYS reg iospace. To achieve that, rely > on the "active" clock being passed through the DT, and read its status > during IRQ handling to check whether the HW is active. > > The old behavior is still present when reg-names aren't supplied, as to > keep backward compatibility. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado <nfraprado@collabora.com> wrote: > > Remove the requirement of a VDEC_SYS reg iospace. To achieve that, rely > on the "active" clock being passed through the DT, and read its status > during IRQ handling to check whether the HW is active. > > The old behavior is still present when reg-names aren't supplied, as to > keep backward compatibility. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > > (no changes since v1) > > .../mediatek/vcodec/mtk_vcodec_dec_drv.c | 59 +++++++++++++++---- > .../mediatek/vcodec/mtk_vcodec_dec_hw.c | 20 +++++-- > .../mediatek/vcodec/mtk_vcodec_dec_pm.c | 12 +++- > .../platform/mediatek/vcodec/mtk_vcodec_drv.h | 1 + > 4 files changed, 74 insertions(+), 18 deletions(-) > > 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. > #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? ChenYu > + > + cg_status = readl(dev->reg_base[VDEC_SYS]); > + return (cg_status & VDEC_HW_ACTIVE) == 0; > +} > + > static irqreturn_t mtk_vcodec_dec_irq_handler(int irq, void *priv) > { > struct mtk_vcodec_dev *dev = priv; > struct mtk_vcodec_ctx *ctx; > - u32 cg_status = 0; > unsigned int dec_done_status = 0; > void __iomem *vdec_misc_addr = dev->reg_base[VDEC_MISC] + > VDEC_IRQ_CFG_REG; > > ctx = mtk_vcodec_get_curr_ctx(dev, MTK_VDEC_CORE); > > - /* check if HW active or not */ > - cg_status = readl(dev->reg_base[0]); > - if ((cg_status & VDEC_HW_ACTIVE) != 0) { > - mtk_v4l2_err("DEC ISR, VDEC active is not 0x0 (0x%08x)", > - cg_status); > + if (!mtk_vcodec_is_hw_active(dev)) { > + mtk_v4l2_err("DEC ISR, VDEC active is not 0x0"); > return IRQ_HANDLED; > } > > @@ -82,6 +90,25 @@ static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev) > { > struct platform_device *pdev = dev->plat_dev; > int reg_num, i; > + struct resource *res; > + bool no_vdecsys_reg = false; > + static const char * const mtk_dec_reg_names[] = { > + "misc", > + "ld", > + "top", > + "cm", > + "ad", > + "av", > + "pp", > + "hwd", > + "hwq", > + "hwb", > + "hwg" > + }; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "misc"); > + if (res) > + no_vdecsys_reg = true; > > /* Sizeof(u32) * 4 bytes for each register base. */ > reg_num = of_property_count_elems_of_size(pdev->dev.of_node, "reg", > @@ -91,12 +118,22 @@ static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev) > return -EINVAL; > } > > - for (i = 0; i < reg_num; i++) { > - dev->reg_base[i] = devm_platform_ioremap_resource(pdev, i); > - if (IS_ERR(dev->reg_base[i])) > - return PTR_ERR(dev->reg_base[i]); > + if (!no_vdecsys_reg) { > + for (i = 0; i < reg_num; i++) { > + dev->reg_base[i] = devm_platform_ioremap_resource(pdev, i); > + if (IS_ERR(dev->reg_base[i])) > + return PTR_ERR(dev->reg_base[i]); > > - mtk_v4l2_debug(2, "reg[%d] base=%p", i, dev->reg_base[i]); > + mtk_v4l2_debug(2, "reg[%d] base=%p", i, dev->reg_base[i]); > + } > + } else { > + for (i = 0; i < reg_num; i++) { > + dev->reg_base[i+1] = devm_platform_ioremap_resource_byname(pdev, mtk_dec_reg_names[i]); > + if (IS_ERR(dev->reg_base[i+1])) > + return PTR_ERR(dev->reg_base[i+1]); > + > + mtk_v4l2_debug(2, "reg[%d] base=%p", i+1, dev->reg_base[i+1]); > + } > } > > return 0; > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c > index b753bf54ebd9..4e786821015d 100644 > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c > @@ -11,6 +11,7 @@ > #include <linux/of_device.h> > #include <linux/pm_runtime.h> > #include <linux/slab.h> > +#include <linux/clk-provider.h> > > #include "mtk_vcodec_drv.h" > #include "mtk_vcodec_dec.h" > @@ -63,22 +64,29 @@ static int mtk_vdec_hw_prob_done(struct mtk_vcodec_dev *vdec_dev) > return 0; > } > > +static bool mtk_vcodec_is_hw_active(struct mtk_vdec_hw_dev *dev) > +{ > + u32 cg_status; > + > + if (!dev->reg_base[VDEC_HW_SYS]) > + return __clk_is_enabled(dev->pm.vdec_active_clk); > + > + cg_status = readl(dev->reg_base[VDEC_HW_SYS]); > + return (cg_status & VDEC_HW_ACTIVE) == 0; > +} > + > static irqreturn_t mtk_vdec_hw_irq_handler(int irq, void *priv) > { > struct mtk_vdec_hw_dev *dev = priv; > struct mtk_vcodec_ctx *ctx; > - u32 cg_status; > unsigned int dec_done_status; > void __iomem *vdec_misc_addr = dev->reg_base[VDEC_HW_MISC] + > VDEC_IRQ_CFG_REG; > > ctx = mtk_vcodec_get_curr_ctx(dev->main_dev, dev->hw_idx); > > - /* check if HW active or not */ > - cg_status = readl(dev->reg_base[VDEC_HW_SYS]); > - if (cg_status & VDEC_HW_ACTIVE) { > - mtk_v4l2_err("vdec active is not 0x0 (0x%08x)", > - cg_status); > + if (!mtk_vcodec_is_hw_active(dev)) { > + mtk_v4l2_err("vdec active is not 0x0"); > return IRQ_HANDLED; > } > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_pm.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_pm.c > index 777d445999e9..53e621965950 100644 > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_pm.c > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_pm.c > @@ -51,6 +51,9 @@ int mtk_vcodec_init_dec_clk(struct platform_device *pdev, struct mtk_vcodec_pm * > clk_info->clk_name); > return PTR_ERR(clk_info->vcodec_clk); > } > + > + if (strcmp(clk_info->clk_name, "active") == 0) > + pm->vdec_active_clk = clk_info->vcodec_clk; > } > > return 0; > @@ -84,6 +87,9 @@ static void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm) > > dec_clk = &pm->vdec_clk; > for (i = 0; i < dec_clk->clk_num; i++) { > + if (strcmp(dec_clk->clk_info[i].clk_name, "active") == 0) > + continue; > + > ret = clk_prepare_enable(dec_clk->clk_info[i].vcodec_clk); > if (ret) { > mtk_v4l2_err("clk_prepare_enable %d %s fail %d", i, > @@ -104,8 +110,12 @@ static void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm) > int i; > > dec_clk = &pm->vdec_clk; > - for (i = dec_clk->clk_num - 1; i >= 0; i--) > + for (i = dec_clk->clk_num - 1; i >= 0; i--) { > + if (strcmp(dec_clk->clk_info[i].clk_name, "active") == 0) > + continue; > + > clk_disable_unprepare(dec_clk->clk_info[i].vcodec_clk); > + } > } > > static void mtk_vcodec_dec_enable_irq(struct mtk_vcodec_dev *vdec_dev, int hw_idx) > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h > index 9acab54fd650..180e74c69042 100644 > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h > @@ -208,6 +208,7 @@ struct mtk_vcodec_pm { > struct mtk_vcodec_clk vdec_clk; > struct mtk_vcodec_clk venc_clk; > struct device *dev; > + struct clk *vdec_active_clk; > }; > > /** > -- > 2.41.0 > >
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: >> >> Remove the requirement of a VDEC_SYS reg iospace. To achieve that, rely >> on the "active" clock being passed through the DT, and read its status >> during IRQ handling to check whether the HW is active. >> >> The old behavior is still present when reg-names aren't supplied, as to >> keep backward compatibility. >> >> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> >> --- >> >> (no changes since v1) >> >> .../mediatek/vcodec/mtk_vcodec_dec_drv.c | 59 +++++++++++++++---- >> .../mediatek/vcodec/mtk_vcodec_dec_hw.c | 20 +++++-- >> .../mediatek/vcodec/mtk_vcodec_dec_pm.c | 12 +++- >> .../platform/mediatek/vcodec/mtk_vcodec_drv.h | 1 + >> 4 files changed, 74 insertions(+), 18 deletions(-) >> >> 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. > >> #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... 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. Cheers, Angelo
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. >
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. >>
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().
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@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. > Not for the current platform, but that may change in future SoCs... we're not sure. > 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? I'm in doubt now on how to proceed. > ---- > > 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(). > Makes sense. Regards, Angelo
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@collabora.com> 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()?
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@collabora.com> 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
Quoting AngeloGioacchino Del Regno (2023-06-15 00:30:56) > 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@collabora.com> 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 You don't have to add the power domain in DT, do you? You can populate a power domain in software directly? > really looks like being the only viable option. > > Stephen, many thanks for the valuable suggestions and the nice conversation. >
Il 15/06/23 19:40, Stephen Boyd ha scritto: > Quoting AngeloGioacchino Del Regno (2023-06-15 00:30:56) >> 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@collabora.com> 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 > > You don't have to add the power domain in DT, do you? You can populate a > power domain in software directly? > Right. I didn't evaluate that possibility at all. Looks good! >> really looks like being the only viable option. >> >> Stephen, many thanks for the valuable suggestions and the nice conversation. >> >
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> #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); + + cg_status = readl(dev->reg_base[VDEC_SYS]); + return (cg_status & VDEC_HW_ACTIVE) == 0; +} + static irqreturn_t mtk_vcodec_dec_irq_handler(int irq, void *priv) { struct mtk_vcodec_dev *dev = priv; struct mtk_vcodec_ctx *ctx; - u32 cg_status = 0; unsigned int dec_done_status = 0; void __iomem *vdec_misc_addr = dev->reg_base[VDEC_MISC] + VDEC_IRQ_CFG_REG; ctx = mtk_vcodec_get_curr_ctx(dev, MTK_VDEC_CORE); - /* check if HW active or not */ - cg_status = readl(dev->reg_base[0]); - if ((cg_status & VDEC_HW_ACTIVE) != 0) { - mtk_v4l2_err("DEC ISR, VDEC active is not 0x0 (0x%08x)", - cg_status); + if (!mtk_vcodec_is_hw_active(dev)) { + mtk_v4l2_err("DEC ISR, VDEC active is not 0x0"); return IRQ_HANDLED; } @@ -82,6 +90,25 @@ static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev) { struct platform_device *pdev = dev->plat_dev; int reg_num, i; + struct resource *res; + bool no_vdecsys_reg = false; + static const char * const mtk_dec_reg_names[] = { + "misc", + "ld", + "top", + "cm", + "ad", + "av", + "pp", + "hwd", + "hwq", + "hwb", + "hwg" + }; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "misc"); + if (res) + no_vdecsys_reg = true; /* Sizeof(u32) * 4 bytes for each register base. */ reg_num = of_property_count_elems_of_size(pdev->dev.of_node, "reg", @@ -91,12 +118,22 @@ static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev) return -EINVAL; } - for (i = 0; i < reg_num; i++) { - dev->reg_base[i] = devm_platform_ioremap_resource(pdev, i); - if (IS_ERR(dev->reg_base[i])) - return PTR_ERR(dev->reg_base[i]); + if (!no_vdecsys_reg) { + for (i = 0; i < reg_num; i++) { + dev->reg_base[i] = devm_platform_ioremap_resource(pdev, i); + if (IS_ERR(dev->reg_base[i])) + return PTR_ERR(dev->reg_base[i]); - mtk_v4l2_debug(2, "reg[%d] base=%p", i, dev->reg_base[i]); + mtk_v4l2_debug(2, "reg[%d] base=%p", i, dev->reg_base[i]); + } + } else { + for (i = 0; i < reg_num; i++) { + dev->reg_base[i+1] = devm_platform_ioremap_resource_byname(pdev, mtk_dec_reg_names[i]); + if (IS_ERR(dev->reg_base[i+1])) + return PTR_ERR(dev->reg_base[i+1]); + + mtk_v4l2_debug(2, "reg[%d] base=%p", i+1, dev->reg_base[i+1]); + } } return 0; diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c index b753bf54ebd9..4e786821015d 100644 --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c @@ -11,6 +11,7 @@ #include <linux/of_device.h> #include <linux/pm_runtime.h> #include <linux/slab.h> +#include <linux/clk-provider.h> #include "mtk_vcodec_drv.h" #include "mtk_vcodec_dec.h" @@ -63,22 +64,29 @@ static int mtk_vdec_hw_prob_done(struct mtk_vcodec_dev *vdec_dev) return 0; } +static bool mtk_vcodec_is_hw_active(struct mtk_vdec_hw_dev *dev) +{ + u32 cg_status; + + if (!dev->reg_base[VDEC_HW_SYS]) + return __clk_is_enabled(dev->pm.vdec_active_clk); + + cg_status = readl(dev->reg_base[VDEC_HW_SYS]); + return (cg_status & VDEC_HW_ACTIVE) == 0; +} + static irqreturn_t mtk_vdec_hw_irq_handler(int irq, void *priv) { struct mtk_vdec_hw_dev *dev = priv; struct mtk_vcodec_ctx *ctx; - u32 cg_status; unsigned int dec_done_status; void __iomem *vdec_misc_addr = dev->reg_base[VDEC_HW_MISC] + VDEC_IRQ_CFG_REG; ctx = mtk_vcodec_get_curr_ctx(dev->main_dev, dev->hw_idx); - /* check if HW active or not */ - cg_status = readl(dev->reg_base[VDEC_HW_SYS]); - if (cg_status & VDEC_HW_ACTIVE) { - mtk_v4l2_err("vdec active is not 0x0 (0x%08x)", - cg_status); + if (!mtk_vcodec_is_hw_active(dev)) { + mtk_v4l2_err("vdec active is not 0x0"); return IRQ_HANDLED; } diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_pm.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_pm.c index 777d445999e9..53e621965950 100644 --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_pm.c +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_pm.c @@ -51,6 +51,9 @@ int mtk_vcodec_init_dec_clk(struct platform_device *pdev, struct mtk_vcodec_pm * clk_info->clk_name); return PTR_ERR(clk_info->vcodec_clk); } + + if (strcmp(clk_info->clk_name, "active") == 0) + pm->vdec_active_clk = clk_info->vcodec_clk; } return 0; @@ -84,6 +87,9 @@ static void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm) dec_clk = &pm->vdec_clk; for (i = 0; i < dec_clk->clk_num; i++) { + if (strcmp(dec_clk->clk_info[i].clk_name, "active") == 0) + continue; + ret = clk_prepare_enable(dec_clk->clk_info[i].vcodec_clk); if (ret) { mtk_v4l2_err("clk_prepare_enable %d %s fail %d", i, @@ -104,8 +110,12 @@ static void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm) int i; dec_clk = &pm->vdec_clk; - for (i = dec_clk->clk_num - 1; i >= 0; i--) + for (i = dec_clk->clk_num - 1; i >= 0; i--) { + if (strcmp(dec_clk->clk_info[i].clk_name, "active") == 0) + continue; + clk_disable_unprepare(dec_clk->clk_info[i].vcodec_clk); + } } static void mtk_vcodec_dec_enable_irq(struct mtk_vcodec_dev *vdec_dev, int hw_idx) diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h index 9acab54fd650..180e74c69042 100644 --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h @@ -208,6 +208,7 @@ struct mtk_vcodec_pm { struct mtk_vcodec_clk vdec_clk; struct mtk_vcodec_clk venc_clk; struct device *dev; + struct clk *vdec_active_clk; }; /**
Remove the requirement of a VDEC_SYS reg iospace. To achieve that, rely on the "active" clock being passed through the DT, and read its status during IRQ handling to check whether the HW is active. The old behavior is still present when reg-names aren't supplied, as to keep backward compatibility. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- (no changes since v1) .../mediatek/vcodec/mtk_vcodec_dec_drv.c | 59 +++++++++++++++---- .../mediatek/vcodec/mtk_vcodec_dec_hw.c | 20 +++++-- .../mediatek/vcodec/mtk_vcodec_dec_pm.c | 12 +++- .../platform/mediatek/vcodec/mtk_vcodec_drv.h | 1 + 4 files changed, 74 insertions(+), 18 deletions(-)