From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f46.google.com (mail-ed1-f46.google.com [209.85.208.46]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 489BD2CA6 for ; Tue, 7 Dec 2021 13:50:52 +0000 (UTC) Received: by mail-ed1-f46.google.com with SMTP id r11so56923795edd.9 for ; Tue, 07 Dec 2021 05:50:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=SSWGp/Jc+wL4V0P6sTq5Sh4OeOCw4Xnhn7rLeeXlXy8=; b=A3r+nxQ3L5i3Td5bX5TDSygbIKfhgq1VcdnNuiiLxiFPj7BZDp7UQbCYVR5dxd/dA3 8rkLOUuhpPwM6zQl2IYKbeYW//G8ldUAGjPVeo7RTQqImWikBlQZQ9p5ZmAVEE9C7FGq EMVbArqJXYryV4St9AT3ECSIK6D0h1bCjWb1E9OKjWhe2q8fUj+pNWzBeHB6IRcfjhB8 H9sOoRq5JnuyGNJUslMXFDdjJIJUBtaoUv57iBsYqS++YfcY00PHr38KuEPZCRNhCmJ+ IYVQNs5Vh1lWzxmJ1SQ9PJe0IoBiT3b8PlAjaMYnB+mWbER0WPqlu01qfJibjXlTsYyy TsJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=SSWGp/Jc+wL4V0P6sTq5Sh4OeOCw4Xnhn7rLeeXlXy8=; b=fArOwYsx98a2s/Qs4frzS0LWQ6FprfnxwzG+gaOw+eZL7SuUWmjgxVSvgmb4B2z6RX t7lh3xEYRmal+SLbldONUMBkwXp9ExR45QLCs9t54oWialx/00wg83trsb0j9fFYqkN4 sUxkXFlLcY5FD5N0E8R59Luv9aBTsgvgm5ZDowgYsCzjjL5XR+SeZDKQrMqwfXA2WRFq XV/EpSywOJ++lBgFyCm0KMsv2C/hWyQFAGsa0+nsMOr/F5Yn2Dix314tmOQlTe7GdBzj 8EJVaEKeQcc2wpq4wDNtK9VygSIID8ic9+PUf59NywW0dh1DQOG1YI4ijZwkAlBoZ+2a ZIdw== X-Gm-Message-State: AOAM533hZOTHMh2qHM0lJeiqmA8AoYe8vS5f6fehdJoKO+oY5ZL7MQ5y hEiwUok9/zonhGsq2RRsR+KO+15Sss0ffXUWRGM= X-Google-Smtp-Source: ABdhPJwcWopjS5m+iH20JbI2W4nbbqY+XL8as57hcMJtxMzpzE6n5iV5CuomnXmrXzg9MF7luFjtt3uFNZdXBtbtEAA= X-Received: by 2002:a17:907:75f0:: with SMTP id jz16mr54690074ejc.77.1638885050210; Tue, 07 Dec 2021 05:50:50 -0800 (PST) Precedence: bulk X-Mailing-List: linux-staging@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20211207015446.1250854-1-aford173@gmail.com> <20211207015446.1250854-6-aford173@gmail.com> In-Reply-To: From: Adam Ford Date: Tue, 7 Dec 2021 07:50:38 -0600 Message-ID: Subject: Re: [RFC V2 5/6] media: hantro: split i.MX8MQ G1 and G2 code To: Lucas Stach Cc: Ezequiel Garcia , linux-media , Chris Healy , Benjamin Gaignard , Hans Verkuil , Philipp Zabel , Mauro Carvalho Chehab , Rob Herring , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Greg Kroah-Hartman , "open list:HANTRO VPU CODEC DRIVER" , devicetree , arm-soc , Linux Kernel Mailing List , "open list:STAGING SUBSYSTEM" Content-Type: text/plain; charset="UTF-8" On Tue, Dec 7, 2021 at 7:41 AM Lucas Stach wrote: > > Am Dienstag, dem 07.12.2021 um 07:24 -0600 schrieb Adam Ford: > > On Tue, Dec 7, 2021 at 6:39 AM Ezequiel Garcia > > wrote: > > > > > > Hi Adam, > > > > > > Thanks for the good work! This is looking quite promising. > > > > > > On Mon, Dec 06, 2021 at 07:54:44PM -0600, Adam Ford wrote: > > > > The VPU in the i.MX8MQ is really the combination of Hantro G1 and > > > > Hantro G2. With the updated vpu-blk-ctrl, the power domains system > > > > can enable and disable them separately as well as pull them out of > > > > reset. This simplifies the code and lets them run independently. > > > > > > > > Signed-off-by: Adam Ford > > > > > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > > > > index ab2467998d29..d803252a5aba 100644 > > > > --- a/drivers/staging/media/hantro/hantro_drv.c > > > > +++ b/drivers/staging/media/hantro/hantro_drv.c > > > > @@ -608,8 +608,8 @@ static const struct of_device_id of_hantro_match[] = { > > > > { .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, }, > > > > #endif > > > > #ifdef CONFIG_VIDEO_HANTRO_IMX8M > > > > - { .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, }, > > > > - { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant }, > > > > + { .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant, }, > > > > > > I think it's important to clarify that you are breaking support > > > for the previous device-tree binding. Not only because of the compatible > > > string change, but because the binding is now quite different. > > > > > > Note that in the past Benjamin tried to avoid this. > > > IIRC, his proposal was backwards compatible. > > > > I was looking at how to do this, but the stand-alone vpu bindings did > > the reset and the fuses manually, it causes issues. When the block > > control is enabled those memory locations for the resets and fuses are > > assigned to the blk-ctrl driver, so attempting to access them from a > > different driver was a violation. > > > > When I started looking at this, the stand-alone VPU was trying to be > > both G1 and G2, but when I was testing it before I made changes, I > > didn't see the G2 function at all. When I was done the G2 seemed to > > work, so it seems like this is an improvement. If you want me to keep > > the previous compatible flag, I could rename the nxp,imx8mq-vpu-g1 > > back to nxp,imx8mq-vpu and remove the reset/fuse controls, but I'd > > have to add the blk-ctrl reference, so it seemed to me like a better > > alternative to deprecate the old binding/driver and replace it with > > the new one because of the significant changes. Since I'd like to > > rebase the i.MX8M Mini I did on this work, it seemed weird to have > > nxp,imx8mq-vpu, nxp,imx8mq-vpu-g2, nxp,imx8mm-vpu-g1, and > > nxp,imx8mm-vpu-g2 where the only one without a Gx name was the > > original 8MQ binding but limited to G1 functionality and the G2 > > stripped out. > > I would very much appreciate if we could keep the driver code for the > old binding. It does not need to have any new additional functionality, > but just keep the existing G1 h.264 decode when booted on a old DT with > the old VPU description and no blk-ctrl, so we don't regress > functionality when a new kernel is booted with a old DT. > > New functionality with the G2 can depend on the new VPU binding and the > blk-ctrl driver. How does that work when both the original VPU and the blk-ctrl attempt to manipulate the reset and clock lines? The original binding for the vpu was assigned: reg = <0x38300000 0x10000>, <0x38310000 0x10000>, <0x38320000 0x10000>; reg-names = "g1", "g2", "ctrl"; If G2 is going to run from 38310000 and vpu-blk-ctrl run from 38320000, they'll collide. > > Regards, > Lucas > > > > > > > If this is unavoidable, due to how the blk-ctrl is handled, then that's > > > fine. Given it's a staging driver, we can still play these games. > > > > > > Having said that, let's please make this very clear in the commit > > > description, to it's clear for developers forward-porting their kernels. > > > This applies not only to this commit, but to all commits that affect > > > the binding. > > > > When I post the formal patch, I will add a note that this explicitly > > deprecates the old bindings due to the fact that the G1 and G2 are > > separate IP blocks in the reference manual, and blk-ctrl is a > > completely separate driver altogether. > > > > > > > > Thanks! > > > Ezequiel > > > > > > > + { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant, }, > > > > #endif > > > > #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4 > > > > { .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, }, > > > > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h > > > > index cff817ca8d22..122b83a16663 100644 > > > > --- a/drivers/staging/media/hantro/hantro_hw.h > > > > +++ b/drivers/staging/media/hantro/hantro_hw.h > > > > @@ -299,8 +299,8 @@ enum hantro_enc_fmt { > > > > ROCKCHIP_VPU_ENC_FMT_UYVY422 = 3, > > > > }; > > > > > > > > +extern const struct hantro_variant imx8mq_vpu_g1_variant; > > > > extern const struct hantro_variant imx8mq_vpu_g2_variant; > > > > -extern const struct hantro_variant imx8mq_vpu_variant; > > > > extern const struct hantro_variant px30_vpu_variant; > > > > extern const struct hantro_variant rk3036_vpu_variant; > > > > extern const struct hantro_variant rk3066_vpu_variant; > > > > diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c > > > > index 1a43f6fceef9..c9f6e8472258 100644 > > > > --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c > > > > +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c > > > > @@ -13,67 +13,6 @@ > > > > #include "hantro_g1_regs.h" > > > > #include "hantro_g2_regs.h" > > > > > > > > -#define CTRL_SOFT_RESET 0x00 > > > > -#define RESET_G1 BIT(1) > > > > -#define RESET_G2 BIT(0) > > > > - > > > > -#define CTRL_CLOCK_ENABLE 0x04 > > > > -#define CLOCK_G1 BIT(1) > > > > -#define CLOCK_G2 BIT(0) > > > > - > > > > -#define CTRL_G1_DEC_FUSE 0x08 > > > > -#define CTRL_G1_PP_FUSE 0x0c > > > > -#define CTRL_G2_DEC_FUSE 0x10 > > > > - > > > > -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits) > > > > -{ > > > > - u32 val; > > > > - > > > > - /* Assert */ > > > > - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); > > > > - val &= ~reset_bits; > > > > - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); > > > > - > > > > - udelay(2); > > > > - > > > > - /* Release */ > > > > - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); > > > > - val |= reset_bits; > > > > - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); > > > > -} > > > > - > > > > -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits) > > > > -{ > > > > - u32 val; > > > > - > > > > - val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE); > > > > - val |= clock_bits; > > > > - writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE); > > > > -} > > > > - > > > > -static int imx8mq_runtime_resume(struct hantro_dev *vpu) > > > > -{ > > > > - int ret; > > > > - > > > > - ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks); > > > > - if (ret) { > > > > - dev_err(vpu->dev, "Failed to enable clocks\n"); > > > > - return ret; > > > > - } > > > > - > > > > - imx8m_soft_reset(vpu, RESET_G1 | RESET_G2); > > > > - imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2); > > > > - > > > > - /* Set values of the fuse registers */ > > > > - writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE); > > > > - writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE); > > > > - writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE); > > > > - > > > > - clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks); > > > > - > > > > - return 0; > > > > -} > > > > - > > > > /* > > > > * Supported formats. > > > > */ > > > > @@ -209,27 +148,6 @@ static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id) > > > > return IRQ_HANDLED; > > > > } > > > > > > > > -static int imx8mq_vpu_hw_init(struct hantro_dev *vpu) > > > > -{ > > > > - vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1]; > > > > - > > > > - return 0; > > > > -} > > > > - > > > > -static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx) > > > > -{ > > > > - struct hantro_dev *vpu = ctx->dev; > > > > - > > > > - imx8m_soft_reset(vpu, RESET_G1); > > > > -} > > > > - > > > > -static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx) > > > > -{ > > > > - struct hantro_dev *vpu = ctx->dev; > > > > - > > > > - imx8m_soft_reset(vpu, RESET_G2); > > > > -} > > > > - > > > > /* > > > > * Supported codec ops. > > > > */ > > > > @@ -237,19 +155,16 @@ static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx) > > > > static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = { > > > > [HANTRO_MODE_MPEG2_DEC] = { > > > > .run = hantro_g1_mpeg2_dec_run, > > > > - .reset = imx8m_vpu_g1_reset, > > > > .init = hantro_mpeg2_dec_init, > > > > .exit = hantro_mpeg2_dec_exit, > > > > }, > > > > [HANTRO_MODE_VP8_DEC] = { > > > > .run = hantro_g1_vp8_dec_run, > > > > - .reset = imx8m_vpu_g1_reset, > > > > .init = hantro_vp8_dec_init, > > > > .exit = hantro_vp8_dec_exit, > > > > }, > > > > [HANTRO_MODE_H264_DEC] = { > > > > .run = hantro_g1_h264_dec_run, > > > > - .reset = imx8m_vpu_g1_reset, > > > > .init = hantro_h264_dec_init, > > > > .exit = hantro_h264_dec_exit, > > > > }, > > > > @@ -258,14 +173,12 @@ static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = { > > > > static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = { > > > > [HANTRO_MODE_HEVC_DEC] = { > > > > .run = hantro_g2_hevc_dec_run, > > > > - .reset = imx8m_vpu_g2_reset, > > > > .init = hantro_hevc_dec_init, > > > > .exit = hantro_hevc_dec_exit, > > > > }, > > > > [HANTRO_MODE_VP9_DEC] = { > > > > .run = hantro_g2_vp9_dec_run, > > > > .done = hantro_g2_vp9_dec_done, > > > > - .reset = imx8m_vpu_g2_reset, > > > > .init = hantro_vp9_dec_init, > > > > .exit = hantro_vp9_dec_exit, > > > > }, > > > > @@ -275,7 +188,7 @@ static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = { > > > > * VPU variants. > > > > */ > > > > > > > > -static const struct hantro_irq imx8mq_irqs[] = { > > > > +static const struct hantro_irq imx8mq_g1_irqs[] = { > > > > { "g1", imx8m_vpu_g1_irq }, > > > > }; > > > > > > > > @@ -283,10 +196,12 @@ static const struct hantro_irq imx8mq_g2_irqs[] = { > > > > { "g2", imx8m_vpu_g2_irq }, > > > > }; > > > > > > > > -static const char * const imx8mq_clk_names[] = { "g1", "g2", "bus" }; > > > > -static const char * const imx8mq_reg_names[] = { "g1", "g2", "ctrl" }; > > > > +static const char * const imx8mq_g1_clk_names[] = { "g1" }; > > > > +static const char * const imx8mq_g1_reg_names[] = { "g1" }; > > > > +static const char * const imx8mq_g2_clk_names[] = { "g2" }; > > > > +static const char * const imx8mq_g2_reg_names[] = { "g2" }; > > > > > > > > -const struct hantro_variant imx8mq_vpu_variant = { > > > > +const struct hantro_variant imx8mq_vpu_g1_variant = { > > > > .dec_fmts = imx8m_vpu_dec_fmts, > > > > .num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts), > > > > .postproc_fmts = imx8m_vpu_postproc_fmts, > > > > @@ -295,14 +210,12 @@ const struct hantro_variant imx8mq_vpu_variant = { > > > > .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER | > > > > HANTRO_H264_DECODER, > > > > .codec_ops = imx8mq_vpu_codec_ops, > > > > - .init = imx8mq_vpu_hw_init, > > > > - .runtime_resume = imx8mq_runtime_resume, > > > > - .irqs = imx8mq_irqs, > > > > - .num_irqs = ARRAY_SIZE(imx8mq_irqs), > > > > - .clk_names = imx8mq_clk_names, > > > > - .num_clocks = ARRAY_SIZE(imx8mq_clk_names), > > > > - .reg_names = imx8mq_reg_names, > > > > - .num_regs = ARRAY_SIZE(imx8mq_reg_names) > > > > + .irqs = imx8mq_g1_irqs, > > > > + .num_irqs = ARRAY_SIZE(imx8mq_g1_irqs), > > > > + .clk_names = imx8mq_g1_clk_names, > > > > + .num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names), > > > > + .reg_names = imx8mq_g1_reg_names, > > > > + .num_regs = ARRAY_SIZE(imx8mq_g1_reg_names), > > > > }; > > > > > > > > const struct hantro_variant imx8mq_vpu_g2_variant = { > > > > @@ -314,10 +227,10 @@ const struct hantro_variant imx8mq_vpu_g2_variant = { > > > > .postproc_ops = &hantro_g2_postproc_ops, > > > > .codec = HANTRO_HEVC_DECODER | HANTRO_VP9_DECODER, > > > > .codec_ops = imx8mq_vpu_g2_codec_ops, > > > > - .init = imx8mq_vpu_hw_init, > > > > - .runtime_resume = imx8mq_runtime_resume, > > > > .irqs = imx8mq_g2_irqs, > > > > .num_irqs = ARRAY_SIZE(imx8mq_g2_irqs), > > > > - .clk_names = imx8mq_clk_names, > > > > - .num_clocks = ARRAY_SIZE(imx8mq_clk_names), > > > > + .clk_names = imx8mq_g2_clk_names, > > > > + .num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names), > > > > + .reg_names = imx8mq_g2_reg_names, > > > > + .num_regs = ARRAY_SIZE(imx8mq_g2_reg_names), > > > > }; > > > > -- > > > > 2.32.0 > > > > > > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2A70DC433FE for ; Tue, 7 Dec 2021 13:51:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=me0C76KLGDxt3zZxVfts90fvqFkhtU9JTldI6L8VH0w=; b=sxH4ACJfwqlhq1 BHHzbgyXN2riX9CNMqmSCtsy/Rj+n35QTBP5Hm/2xeAnEbtvBRjK+CNsGbVJmdpUdsoEYh0vWtSi/ Zcb7bRrqU636CgYKbaUMeaQEHRgce2cS3hDTGaBQ8RVi34bA3FLHBOARB6H+yhVeR8B15s0bH+h2n vndAOc+9p4i6EXII9GxfDXLT0zk4mpsELosfwRJ0t6YwTXF1fGvBbCpWlgUUcMYYN82BDTlEwJElO IhuWLAEHEk28Rub21xBnmV+NaiFaaZb8i7VhYICvXZFqp/poy3nJzaHxPk3zVNbN9DeiMSH6YB3UO 5ZKF3M9UcHyeV5K6jH9w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1muarw-008lGN-PR; Tue, 07 Dec 2021 13:50:56 +0000 Received: from mail-ed1-x533.google.com ([2a00:1450:4864:20::533]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1muart-008lEv-7B; Tue, 07 Dec 2021 13:50:55 +0000 Received: by mail-ed1-x533.google.com with SMTP id w1so57133552edc.6; Tue, 07 Dec 2021 05:50:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=SSWGp/Jc+wL4V0P6sTq5Sh4OeOCw4Xnhn7rLeeXlXy8=; b=A3r+nxQ3L5i3Td5bX5TDSygbIKfhgq1VcdnNuiiLxiFPj7BZDp7UQbCYVR5dxd/dA3 8rkLOUuhpPwM6zQl2IYKbeYW//G8ldUAGjPVeo7RTQqImWikBlQZQ9p5ZmAVEE9C7FGq EMVbArqJXYryV4St9AT3ECSIK6D0h1bCjWb1E9OKjWhe2q8fUj+pNWzBeHB6IRcfjhB8 H9sOoRq5JnuyGNJUslMXFDdjJIJUBtaoUv57iBsYqS++YfcY00PHr38KuEPZCRNhCmJ+ IYVQNs5Vh1lWzxmJ1SQ9PJe0IoBiT3b8PlAjaMYnB+mWbER0WPqlu01qfJibjXlTsYyy TsJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=SSWGp/Jc+wL4V0P6sTq5Sh4OeOCw4Xnhn7rLeeXlXy8=; b=EkXYHhcFWqosNodcxe+rjccAk9pvbpFKRXh5oSxVn67Ze4rsUKxQ1aJkob2fd33QvF tpGHq2nmIZM8hwTZdcyBoIK9E3+igUqqQfuljY6/8gdMk45q2G3gbpwu4yDSSbS6C07z pXcU6AtypoyP/ezIn7I5K2bWebma2O27V/ifrwsK+Gko02XYlVfrTQXJo6AJub6nYxwQ OY7r4F9CrMKqxO1a+hEortNHZQ7b8a4pztM19KiivkcKnT0vb2kUwHxWP8sLOOgtOIjr DhUv1eIMObQtlU26Cc+Z9De0SZ5+gDpJeRIXgj+oTbEw03xq1vMMNfodZyCllyW7ba+f O79Q== X-Gm-Message-State: AOAM532yyHNwOSDZ6igEJqiM18qEQp0u5DKMBJD68jkS0ajWxr8bgos7 P8ptt2wb/c4aBvCAdE7UDCkwsAhOwbcvUx3w4t0= X-Google-Smtp-Source: ABdhPJwcWopjS5m+iH20JbI2W4nbbqY+XL8as57hcMJtxMzpzE6n5iV5CuomnXmrXzg9MF7luFjtt3uFNZdXBtbtEAA= X-Received: by 2002:a17:907:75f0:: with SMTP id jz16mr54690074ejc.77.1638885050210; Tue, 07 Dec 2021 05:50:50 -0800 (PST) MIME-Version: 1.0 References: <20211207015446.1250854-1-aford173@gmail.com> <20211207015446.1250854-6-aford173@gmail.com> In-Reply-To: From: Adam Ford Date: Tue, 7 Dec 2021 07:50:38 -0600 Message-ID: Subject: Re: [RFC V2 5/6] media: hantro: split i.MX8MQ G1 and G2 code To: Lucas Stach Cc: Ezequiel Garcia , linux-media , Chris Healy , Benjamin Gaignard , Hans Verkuil , Philipp Zabel , Mauro Carvalho Chehab , Rob Herring , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Greg Kroah-Hartman , "open list:HANTRO VPU CODEC DRIVER" , devicetree , arm-soc , Linux Kernel Mailing List , "open list:STAGING SUBSYSTEM" X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211207_055053_313430_70FA5401 X-CRM114-Status: GOOD ( 55.95 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org On Tue, Dec 7, 2021 at 7:41 AM Lucas Stach wrote: > > Am Dienstag, dem 07.12.2021 um 07:24 -0600 schrieb Adam Ford: > > On Tue, Dec 7, 2021 at 6:39 AM Ezequiel Garcia > > wrote: > > > > > > Hi Adam, > > > > > > Thanks for the good work! This is looking quite promising. > > > > > > On Mon, Dec 06, 2021 at 07:54:44PM -0600, Adam Ford wrote: > > > > The VPU in the i.MX8MQ is really the combination of Hantro G1 and > > > > Hantro G2. With the updated vpu-blk-ctrl, the power domains system > > > > can enable and disable them separately as well as pull them out of > > > > reset. This simplifies the code and lets them run independently. > > > > > > > > Signed-off-by: Adam Ford > > > > > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > > > > index ab2467998d29..d803252a5aba 100644 > > > > --- a/drivers/staging/media/hantro/hantro_drv.c > > > > +++ b/drivers/staging/media/hantro/hantro_drv.c > > > > @@ -608,8 +608,8 @@ static const struct of_device_id of_hantro_match[] = { > > > > { .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, }, > > > > #endif > > > > #ifdef CONFIG_VIDEO_HANTRO_IMX8M > > > > - { .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, }, > > > > - { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant }, > > > > + { .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant, }, > > > > > > I think it's important to clarify that you are breaking support > > > for the previous device-tree binding. Not only because of the compatible > > > string change, but because the binding is now quite different. > > > > > > Note that in the past Benjamin tried to avoid this. > > > IIRC, his proposal was backwards compatible. > > > > I was looking at how to do this, but the stand-alone vpu bindings did > > the reset and the fuses manually, it causes issues. When the block > > control is enabled those memory locations for the resets and fuses are > > assigned to the blk-ctrl driver, so attempting to access them from a > > different driver was a violation. > > > > When I started looking at this, the stand-alone VPU was trying to be > > both G1 and G2, but when I was testing it before I made changes, I > > didn't see the G2 function at all. When I was done the G2 seemed to > > work, so it seems like this is an improvement. If you want me to keep > > the previous compatible flag, I could rename the nxp,imx8mq-vpu-g1 > > back to nxp,imx8mq-vpu and remove the reset/fuse controls, but I'd > > have to add the blk-ctrl reference, so it seemed to me like a better > > alternative to deprecate the old binding/driver and replace it with > > the new one because of the significant changes. Since I'd like to > > rebase the i.MX8M Mini I did on this work, it seemed weird to have > > nxp,imx8mq-vpu, nxp,imx8mq-vpu-g2, nxp,imx8mm-vpu-g1, and > > nxp,imx8mm-vpu-g2 where the only one without a Gx name was the > > original 8MQ binding but limited to G1 functionality and the G2 > > stripped out. > > I would very much appreciate if we could keep the driver code for the > old binding. It does not need to have any new additional functionality, > but just keep the existing G1 h.264 decode when booted on a old DT with > the old VPU description and no blk-ctrl, so we don't regress > functionality when a new kernel is booted with a old DT. > > New functionality with the G2 can depend on the new VPU binding and the > blk-ctrl driver. How does that work when both the original VPU and the blk-ctrl attempt to manipulate the reset and clock lines? The original binding for the vpu was assigned: reg = <0x38300000 0x10000>, <0x38310000 0x10000>, <0x38320000 0x10000>; reg-names = "g1", "g2", "ctrl"; If G2 is going to run from 38310000 and vpu-blk-ctrl run from 38320000, they'll collide. > > Regards, > Lucas > > > > > > > If this is unavoidable, due to how the blk-ctrl is handled, then that's > > > fine. Given it's a staging driver, we can still play these games. > > > > > > Having said that, let's please make this very clear in the commit > > > description, to it's clear for developers forward-porting their kernels. > > > This applies not only to this commit, but to all commits that affect > > > the binding. > > > > When I post the formal patch, I will add a note that this explicitly > > deprecates the old bindings due to the fact that the G1 and G2 are > > separate IP blocks in the reference manual, and blk-ctrl is a > > completely separate driver altogether. > > > > > > > > Thanks! > > > Ezequiel > > > > > > > + { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant, }, > > > > #endif > > > > #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4 > > > > { .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, }, > > > > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h > > > > index cff817ca8d22..122b83a16663 100644 > > > > --- a/drivers/staging/media/hantro/hantro_hw.h > > > > +++ b/drivers/staging/media/hantro/hantro_hw.h > > > > @@ -299,8 +299,8 @@ enum hantro_enc_fmt { > > > > ROCKCHIP_VPU_ENC_FMT_UYVY422 = 3, > > > > }; > > > > > > > > +extern const struct hantro_variant imx8mq_vpu_g1_variant; > > > > extern const struct hantro_variant imx8mq_vpu_g2_variant; > > > > -extern const struct hantro_variant imx8mq_vpu_variant; > > > > extern const struct hantro_variant px30_vpu_variant; > > > > extern const struct hantro_variant rk3036_vpu_variant; > > > > extern const struct hantro_variant rk3066_vpu_variant; > > > > diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c > > > > index 1a43f6fceef9..c9f6e8472258 100644 > > > > --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c > > > > +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c > > > > @@ -13,67 +13,6 @@ > > > > #include "hantro_g1_regs.h" > > > > #include "hantro_g2_regs.h" > > > > > > > > -#define CTRL_SOFT_RESET 0x00 > > > > -#define RESET_G1 BIT(1) > > > > -#define RESET_G2 BIT(0) > > > > - > > > > -#define CTRL_CLOCK_ENABLE 0x04 > > > > -#define CLOCK_G1 BIT(1) > > > > -#define CLOCK_G2 BIT(0) > > > > - > > > > -#define CTRL_G1_DEC_FUSE 0x08 > > > > -#define CTRL_G1_PP_FUSE 0x0c > > > > -#define CTRL_G2_DEC_FUSE 0x10 > > > > - > > > > -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits) > > > > -{ > > > > - u32 val; > > > > - > > > > - /* Assert */ > > > > - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); > > > > - val &= ~reset_bits; > > > > - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); > > > > - > > > > - udelay(2); > > > > - > > > > - /* Release */ > > > > - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); > > > > - val |= reset_bits; > > > > - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); > > > > -} > > > > - > > > > -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits) > > > > -{ > > > > - u32 val; > > > > - > > > > - val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE); > > > > - val |= clock_bits; > > > > - writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE); > > > > -} > > > > - > > > > -static int imx8mq_runtime_resume(struct hantro_dev *vpu) > > > > -{ > > > > - int ret; > > > > - > > > > - ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks); > > > > - if (ret) { > > > > - dev_err(vpu->dev, "Failed to enable clocks\n"); > > > > - return ret; > > > > - } > > > > - > > > > - imx8m_soft_reset(vpu, RESET_G1 | RESET_G2); > > > > - imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2); > > > > - > > > > - /* Set values of the fuse registers */ > > > > - writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE); > > > > - writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE); > > > > - writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE); > > > > - > > > > - clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks); > > > > - > > > > - return 0; > > > > -} > > > > - > > > > /* > > > > * Supported formats. > > > > */ > > > > @@ -209,27 +148,6 @@ static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id) > > > > return IRQ_HANDLED; > > > > } > > > > > > > > -static int imx8mq_vpu_hw_init(struct hantro_dev *vpu) > > > > -{ > > > > - vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1]; > > > > - > > > > - return 0; > > > > -} > > > > - > > > > -static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx) > > > > -{ > > > > - struct hantro_dev *vpu = ctx->dev; > > > > - > > > > - imx8m_soft_reset(vpu, RESET_G1); > > > > -} > > > > - > > > > -static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx) > > > > -{ > > > > - struct hantro_dev *vpu = ctx->dev; > > > > - > > > > - imx8m_soft_reset(vpu, RESET_G2); > > > > -} > > > > - > > > > /* > > > > * Supported codec ops. > > > > */ > > > > @@ -237,19 +155,16 @@ static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx) > > > > static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = { > > > > [HANTRO_MODE_MPEG2_DEC] = { > > > > .run = hantro_g1_mpeg2_dec_run, > > > > - .reset = imx8m_vpu_g1_reset, > > > > .init = hantro_mpeg2_dec_init, > > > > .exit = hantro_mpeg2_dec_exit, > > > > }, > > > > [HANTRO_MODE_VP8_DEC] = { > > > > .run = hantro_g1_vp8_dec_run, > > > > - .reset = imx8m_vpu_g1_reset, > > > > .init = hantro_vp8_dec_init, > > > > .exit = hantro_vp8_dec_exit, > > > > }, > > > > [HANTRO_MODE_H264_DEC] = { > > > > .run = hantro_g1_h264_dec_run, > > > > - .reset = imx8m_vpu_g1_reset, > > > > .init = hantro_h264_dec_init, > > > > .exit = hantro_h264_dec_exit, > > > > }, > > > > @@ -258,14 +173,12 @@ static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = { > > > > static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = { > > > > [HANTRO_MODE_HEVC_DEC] = { > > > > .run = hantro_g2_hevc_dec_run, > > > > - .reset = imx8m_vpu_g2_reset, > > > > .init = hantro_hevc_dec_init, > > > > .exit = hantro_hevc_dec_exit, > > > > }, > > > > [HANTRO_MODE_VP9_DEC] = { > > > > .run = hantro_g2_vp9_dec_run, > > > > .done = hantro_g2_vp9_dec_done, > > > > - .reset = imx8m_vpu_g2_reset, > > > > .init = hantro_vp9_dec_init, > > > > .exit = hantro_vp9_dec_exit, > > > > }, > > > > @@ -275,7 +188,7 @@ static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = { > > > > * VPU variants. > > > > */ > > > > > > > > -static const struct hantro_irq imx8mq_irqs[] = { > > > > +static const struct hantro_irq imx8mq_g1_irqs[] = { > > > > { "g1", imx8m_vpu_g1_irq }, > > > > }; > > > > > > > > @@ -283,10 +196,12 @@ static const struct hantro_irq imx8mq_g2_irqs[] = { > > > > { "g2", imx8m_vpu_g2_irq }, > > > > }; > > > > > > > > -static const char * const imx8mq_clk_names[] = { "g1", "g2", "bus" }; > > > > -static const char * const imx8mq_reg_names[] = { "g1", "g2", "ctrl" }; > > > > +static const char * const imx8mq_g1_clk_names[] = { "g1" }; > > > > +static const char * const imx8mq_g1_reg_names[] = { "g1" }; > > > > +static const char * const imx8mq_g2_clk_names[] = { "g2" }; > > > > +static const char * const imx8mq_g2_reg_names[] = { "g2" }; > > > > > > > > -const struct hantro_variant imx8mq_vpu_variant = { > > > > +const struct hantro_variant imx8mq_vpu_g1_variant = { > > > > .dec_fmts = imx8m_vpu_dec_fmts, > > > > .num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts), > > > > .postproc_fmts = imx8m_vpu_postproc_fmts, > > > > @@ -295,14 +210,12 @@ const struct hantro_variant imx8mq_vpu_variant = { > > > > .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER | > > > > HANTRO_H264_DECODER, > > > > .codec_ops = imx8mq_vpu_codec_ops, > > > > - .init = imx8mq_vpu_hw_init, > > > > - .runtime_resume = imx8mq_runtime_resume, > > > > - .irqs = imx8mq_irqs, > > > > - .num_irqs = ARRAY_SIZE(imx8mq_irqs), > > > > - .clk_names = imx8mq_clk_names, > > > > - .num_clocks = ARRAY_SIZE(imx8mq_clk_names), > > > > - .reg_names = imx8mq_reg_names, > > > > - .num_regs = ARRAY_SIZE(imx8mq_reg_names) > > > > + .irqs = imx8mq_g1_irqs, > > > > + .num_irqs = ARRAY_SIZE(imx8mq_g1_irqs), > > > > + .clk_names = imx8mq_g1_clk_names, > > > > + .num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names), > > > > + .reg_names = imx8mq_g1_reg_names, > > > > + .num_regs = ARRAY_SIZE(imx8mq_g1_reg_names), > > > > }; > > > > > > > > const struct hantro_variant imx8mq_vpu_g2_variant = { > > > > @@ -314,10 +227,10 @@ const struct hantro_variant imx8mq_vpu_g2_variant = { > > > > .postproc_ops = &hantro_g2_postproc_ops, > > > > .codec = HANTRO_HEVC_DECODER | HANTRO_VP9_DECODER, > > > > .codec_ops = imx8mq_vpu_g2_codec_ops, > > > > - .init = imx8mq_vpu_hw_init, > > > > - .runtime_resume = imx8mq_runtime_resume, > > > > .irqs = imx8mq_g2_irqs, > > > > .num_irqs = ARRAY_SIZE(imx8mq_g2_irqs), > > > > - .clk_names = imx8mq_clk_names, > > > > - .num_clocks = ARRAY_SIZE(imx8mq_clk_names), > > > > + .clk_names = imx8mq_g2_clk_names, > > > > + .num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names), > > > > + .reg_names = imx8mq_g2_reg_names, > > > > + .num_regs = ARRAY_SIZE(imx8mq_g2_reg_names), > > > > }; > > > > -- > > > > 2.32.0 > > > > > > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id EE6CDC433F5 for ; Tue, 7 Dec 2021 13:52:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=WZFWJ/0j1stx8r7dsUU9nmtLcl3w+7VCifpYJp0jJEQ=; b=hRDq9wnIQVANkL 7HzmB86UenII6U2pgbfBO74/VGW3OtWVyzO9OUMUrw6PYI07ajOLsSWXrziR2UFcBnStMz7cUh5qp Tmr16J2+jYmswEvQIbot0HiT0/qSYNlWVFdMSMsG+VWfZuscQA0ww/kRh7zlPU7yPLWpO2xcFVcbQ YeX1p48a7BvgO9+0pI42lKlQTD7HwQkaLMyL/nVwUKGmOYQNCFBOVgzNNp2mETvuWeTNtjjT2H8Cq t2hvnYf+W7gaCxJIUCNEspt+Pl9RKocBMbC+LemE0cn7Pj7jOpp7VydNX+tpNLnvdT4x7cd+bPk2m dvdcI5W6fdx0KgaP0Yew==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1muary-008lGc-TK; Tue, 07 Dec 2021 13:50:59 +0000 Received: from mail-ed1-x533.google.com ([2a00:1450:4864:20::533]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1muart-008lEv-7B; Tue, 07 Dec 2021 13:50:55 +0000 Received: by mail-ed1-x533.google.com with SMTP id w1so57133552edc.6; Tue, 07 Dec 2021 05:50:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=SSWGp/Jc+wL4V0P6sTq5Sh4OeOCw4Xnhn7rLeeXlXy8=; b=A3r+nxQ3L5i3Td5bX5TDSygbIKfhgq1VcdnNuiiLxiFPj7BZDp7UQbCYVR5dxd/dA3 8rkLOUuhpPwM6zQl2IYKbeYW//G8ldUAGjPVeo7RTQqImWikBlQZQ9p5ZmAVEE9C7FGq EMVbArqJXYryV4St9AT3ECSIK6D0h1bCjWb1E9OKjWhe2q8fUj+pNWzBeHB6IRcfjhB8 H9sOoRq5JnuyGNJUslMXFDdjJIJUBtaoUv57iBsYqS++YfcY00PHr38KuEPZCRNhCmJ+ IYVQNs5Vh1lWzxmJ1SQ9PJe0IoBiT3b8PlAjaMYnB+mWbER0WPqlu01qfJibjXlTsYyy TsJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=SSWGp/Jc+wL4V0P6sTq5Sh4OeOCw4Xnhn7rLeeXlXy8=; b=EkXYHhcFWqosNodcxe+rjccAk9pvbpFKRXh5oSxVn67Ze4rsUKxQ1aJkob2fd33QvF tpGHq2nmIZM8hwTZdcyBoIK9E3+igUqqQfuljY6/8gdMk45q2G3gbpwu4yDSSbS6C07z pXcU6AtypoyP/ezIn7I5K2bWebma2O27V/ifrwsK+Gko02XYlVfrTQXJo6AJub6nYxwQ OY7r4F9CrMKqxO1a+hEortNHZQ7b8a4pztM19KiivkcKnT0vb2kUwHxWP8sLOOgtOIjr DhUv1eIMObQtlU26Cc+Z9De0SZ5+gDpJeRIXgj+oTbEw03xq1vMMNfodZyCllyW7ba+f O79Q== X-Gm-Message-State: AOAM532yyHNwOSDZ6igEJqiM18qEQp0u5DKMBJD68jkS0ajWxr8bgos7 P8ptt2wb/c4aBvCAdE7UDCkwsAhOwbcvUx3w4t0= X-Google-Smtp-Source: ABdhPJwcWopjS5m+iH20JbI2W4nbbqY+XL8as57hcMJtxMzpzE6n5iV5CuomnXmrXzg9MF7luFjtt3uFNZdXBtbtEAA= X-Received: by 2002:a17:907:75f0:: with SMTP id jz16mr54690074ejc.77.1638885050210; Tue, 07 Dec 2021 05:50:50 -0800 (PST) MIME-Version: 1.0 References: <20211207015446.1250854-1-aford173@gmail.com> <20211207015446.1250854-6-aford173@gmail.com> In-Reply-To: From: Adam Ford Date: Tue, 7 Dec 2021 07:50:38 -0600 Message-ID: Subject: Re: [RFC V2 5/6] media: hantro: split i.MX8MQ G1 and G2 code To: Lucas Stach Cc: Ezequiel Garcia , linux-media , Chris Healy , Benjamin Gaignard , Hans Verkuil , Philipp Zabel , Mauro Carvalho Chehab , Rob Herring , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Greg Kroah-Hartman , "open list:HANTRO VPU CODEC DRIVER" , devicetree , arm-soc , Linux Kernel Mailing List , "open list:STAGING SUBSYSTEM" X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211207_055053_313430_70FA5401 X-CRM114-Status: GOOD ( 55.95 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Dec 7, 2021 at 7:41 AM Lucas Stach wrote: > > Am Dienstag, dem 07.12.2021 um 07:24 -0600 schrieb Adam Ford: > > On Tue, Dec 7, 2021 at 6:39 AM Ezequiel Garcia > > wrote: > > > > > > Hi Adam, > > > > > > Thanks for the good work! This is looking quite promising. > > > > > > On Mon, Dec 06, 2021 at 07:54:44PM -0600, Adam Ford wrote: > > > > The VPU in the i.MX8MQ is really the combination of Hantro G1 and > > > > Hantro G2. With the updated vpu-blk-ctrl, the power domains system > > > > can enable and disable them separately as well as pull them out of > > > > reset. This simplifies the code and lets them run independently. > > > > > > > > Signed-off-by: Adam Ford > > > > > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > > > > index ab2467998d29..d803252a5aba 100644 > > > > --- a/drivers/staging/media/hantro/hantro_drv.c > > > > +++ b/drivers/staging/media/hantro/hantro_drv.c > > > > @@ -608,8 +608,8 @@ static const struct of_device_id of_hantro_match[] = { > > > > { .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, }, > > > > #endif > > > > #ifdef CONFIG_VIDEO_HANTRO_IMX8M > > > > - { .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, }, > > > > - { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant }, > > > > + { .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant, }, > > > > > > I think it's important to clarify that you are breaking support > > > for the previous device-tree binding. Not only because of the compatible > > > string change, but because the binding is now quite different. > > > > > > Note that in the past Benjamin tried to avoid this. > > > IIRC, his proposal was backwards compatible. > > > > I was looking at how to do this, but the stand-alone vpu bindings did > > the reset and the fuses manually, it causes issues. When the block > > control is enabled those memory locations for the resets and fuses are > > assigned to the blk-ctrl driver, so attempting to access them from a > > different driver was a violation. > > > > When I started looking at this, the stand-alone VPU was trying to be > > both G1 and G2, but when I was testing it before I made changes, I > > didn't see the G2 function at all. When I was done the G2 seemed to > > work, so it seems like this is an improvement. If you want me to keep > > the previous compatible flag, I could rename the nxp,imx8mq-vpu-g1 > > back to nxp,imx8mq-vpu and remove the reset/fuse controls, but I'd > > have to add the blk-ctrl reference, so it seemed to me like a better > > alternative to deprecate the old binding/driver and replace it with > > the new one because of the significant changes. Since I'd like to > > rebase the i.MX8M Mini I did on this work, it seemed weird to have > > nxp,imx8mq-vpu, nxp,imx8mq-vpu-g2, nxp,imx8mm-vpu-g1, and > > nxp,imx8mm-vpu-g2 where the only one without a Gx name was the > > original 8MQ binding but limited to G1 functionality and the G2 > > stripped out. > > I would very much appreciate if we could keep the driver code for the > old binding. It does not need to have any new additional functionality, > but just keep the existing G1 h.264 decode when booted on a old DT with > the old VPU description and no blk-ctrl, so we don't regress > functionality when a new kernel is booted with a old DT. > > New functionality with the G2 can depend on the new VPU binding and the > blk-ctrl driver. How does that work when both the original VPU and the blk-ctrl attempt to manipulate the reset and clock lines? The original binding for the vpu was assigned: reg = <0x38300000 0x10000>, <0x38310000 0x10000>, <0x38320000 0x10000>; reg-names = "g1", "g2", "ctrl"; If G2 is going to run from 38310000 and vpu-blk-ctrl run from 38320000, they'll collide. > > Regards, > Lucas > > > > > > > If this is unavoidable, due to how the blk-ctrl is handled, then that's > > > fine. Given it's a staging driver, we can still play these games. > > > > > > Having said that, let's please make this very clear in the commit > > > description, to it's clear for developers forward-porting their kernels. > > > This applies not only to this commit, but to all commits that affect > > > the binding. > > > > When I post the formal patch, I will add a note that this explicitly > > deprecates the old bindings due to the fact that the G1 and G2 are > > separate IP blocks in the reference manual, and blk-ctrl is a > > completely separate driver altogether. > > > > > > > > Thanks! > > > Ezequiel > > > > > > > + { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant, }, > > > > #endif > > > > #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4 > > > > { .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, }, > > > > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h > > > > index cff817ca8d22..122b83a16663 100644 > > > > --- a/drivers/staging/media/hantro/hantro_hw.h > > > > +++ b/drivers/staging/media/hantro/hantro_hw.h > > > > @@ -299,8 +299,8 @@ enum hantro_enc_fmt { > > > > ROCKCHIP_VPU_ENC_FMT_UYVY422 = 3, > > > > }; > > > > > > > > +extern const struct hantro_variant imx8mq_vpu_g1_variant; > > > > extern const struct hantro_variant imx8mq_vpu_g2_variant; > > > > -extern const struct hantro_variant imx8mq_vpu_variant; > > > > extern const struct hantro_variant px30_vpu_variant; > > > > extern const struct hantro_variant rk3036_vpu_variant; > > > > extern const struct hantro_variant rk3066_vpu_variant; > > > > diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c > > > > index 1a43f6fceef9..c9f6e8472258 100644 > > > > --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c > > > > +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c > > > > @@ -13,67 +13,6 @@ > > > > #include "hantro_g1_regs.h" > > > > #include "hantro_g2_regs.h" > > > > > > > > -#define CTRL_SOFT_RESET 0x00 > > > > -#define RESET_G1 BIT(1) > > > > -#define RESET_G2 BIT(0) > > > > - > > > > -#define CTRL_CLOCK_ENABLE 0x04 > > > > -#define CLOCK_G1 BIT(1) > > > > -#define CLOCK_G2 BIT(0) > > > > - > > > > -#define CTRL_G1_DEC_FUSE 0x08 > > > > -#define CTRL_G1_PP_FUSE 0x0c > > > > -#define CTRL_G2_DEC_FUSE 0x10 > > > > - > > > > -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits) > > > > -{ > > > > - u32 val; > > > > - > > > > - /* Assert */ > > > > - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); > > > > - val &= ~reset_bits; > > > > - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); > > > > - > > > > - udelay(2); > > > > - > > > > - /* Release */ > > > > - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET); > > > > - val |= reset_bits; > > > > - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET); > > > > -} > > > > - > > > > -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits) > > > > -{ > > > > - u32 val; > > > > - > > > > - val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE); > > > > - val |= clock_bits; > > > > - writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE); > > > > -} > > > > - > > > > -static int imx8mq_runtime_resume(struct hantro_dev *vpu) > > > > -{ > > > > - int ret; > > > > - > > > > - ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks); > > > > - if (ret) { > > > > - dev_err(vpu->dev, "Failed to enable clocks\n"); > > > > - return ret; > > > > - } > > > > - > > > > - imx8m_soft_reset(vpu, RESET_G1 | RESET_G2); > > > > - imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2); > > > > - > > > > - /* Set values of the fuse registers */ > > > > - writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE); > > > > - writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE); > > > > - writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE); > > > > - > > > > - clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks); > > > > - > > > > - return 0; > > > > -} > > > > - > > > > /* > > > > * Supported formats. > > > > */ > > > > @@ -209,27 +148,6 @@ static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id) > > > > return IRQ_HANDLED; > > > > } > > > > > > > > -static int imx8mq_vpu_hw_init(struct hantro_dev *vpu) > > > > -{ > > > > - vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1]; > > > > - > > > > - return 0; > > > > -} > > > > - > > > > -static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx) > > > > -{ > > > > - struct hantro_dev *vpu = ctx->dev; > > > > - > > > > - imx8m_soft_reset(vpu, RESET_G1); > > > > -} > > > > - > > > > -static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx) > > > > -{ > > > > - struct hantro_dev *vpu = ctx->dev; > > > > - > > > > - imx8m_soft_reset(vpu, RESET_G2); > > > > -} > > > > - > > > > /* > > > > * Supported codec ops. > > > > */ > > > > @@ -237,19 +155,16 @@ static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx) > > > > static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = { > > > > [HANTRO_MODE_MPEG2_DEC] = { > > > > .run = hantro_g1_mpeg2_dec_run, > > > > - .reset = imx8m_vpu_g1_reset, > > > > .init = hantro_mpeg2_dec_init, > > > > .exit = hantro_mpeg2_dec_exit, > > > > }, > > > > [HANTRO_MODE_VP8_DEC] = { > > > > .run = hantro_g1_vp8_dec_run, > > > > - .reset = imx8m_vpu_g1_reset, > > > > .init = hantro_vp8_dec_init, > > > > .exit = hantro_vp8_dec_exit, > > > > }, > > > > [HANTRO_MODE_H264_DEC] = { > > > > .run = hantro_g1_h264_dec_run, > > > > - .reset = imx8m_vpu_g1_reset, > > > > .init = hantro_h264_dec_init, > > > > .exit = hantro_h264_dec_exit, > > > > }, > > > > @@ -258,14 +173,12 @@ static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = { > > > > static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = { > > > > [HANTRO_MODE_HEVC_DEC] = { > > > > .run = hantro_g2_hevc_dec_run, > > > > - .reset = imx8m_vpu_g2_reset, > > > > .init = hantro_hevc_dec_init, > > > > .exit = hantro_hevc_dec_exit, > > > > }, > > > > [HANTRO_MODE_VP9_DEC] = { > > > > .run = hantro_g2_vp9_dec_run, > > > > .done = hantro_g2_vp9_dec_done, > > > > - .reset = imx8m_vpu_g2_reset, > > > > .init = hantro_vp9_dec_init, > > > > .exit = hantro_vp9_dec_exit, > > > > }, > > > > @@ -275,7 +188,7 @@ static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = { > > > > * VPU variants. > > > > */ > > > > > > > > -static const struct hantro_irq imx8mq_irqs[] = { > > > > +static const struct hantro_irq imx8mq_g1_irqs[] = { > > > > { "g1", imx8m_vpu_g1_irq }, > > > > }; > > > > > > > > @@ -283,10 +196,12 @@ static const struct hantro_irq imx8mq_g2_irqs[] = { > > > > { "g2", imx8m_vpu_g2_irq }, > > > > }; > > > > > > > > -static const char * const imx8mq_clk_names[] = { "g1", "g2", "bus" }; > > > > -static const char * const imx8mq_reg_names[] = { "g1", "g2", "ctrl" }; > > > > +static const char * const imx8mq_g1_clk_names[] = { "g1" }; > > > > +static const char * const imx8mq_g1_reg_names[] = { "g1" }; > > > > +static const char * const imx8mq_g2_clk_names[] = { "g2" }; > > > > +static const char * const imx8mq_g2_reg_names[] = { "g2" }; > > > > > > > > -const struct hantro_variant imx8mq_vpu_variant = { > > > > +const struct hantro_variant imx8mq_vpu_g1_variant = { > > > > .dec_fmts = imx8m_vpu_dec_fmts, > > > > .num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts), > > > > .postproc_fmts = imx8m_vpu_postproc_fmts, > > > > @@ -295,14 +210,12 @@ const struct hantro_variant imx8mq_vpu_variant = { > > > > .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER | > > > > HANTRO_H264_DECODER, > > > > .codec_ops = imx8mq_vpu_codec_ops, > > > > - .init = imx8mq_vpu_hw_init, > > > > - .runtime_resume = imx8mq_runtime_resume, > > > > - .irqs = imx8mq_irqs, > > > > - .num_irqs = ARRAY_SIZE(imx8mq_irqs), > > > > - .clk_names = imx8mq_clk_names, > > > > - .num_clocks = ARRAY_SIZE(imx8mq_clk_names), > > > > - .reg_names = imx8mq_reg_names, > > > > - .num_regs = ARRAY_SIZE(imx8mq_reg_names) > > > > + .irqs = imx8mq_g1_irqs, > > > > + .num_irqs = ARRAY_SIZE(imx8mq_g1_irqs), > > > > + .clk_names = imx8mq_g1_clk_names, > > > > + .num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names), > > > > + .reg_names = imx8mq_g1_reg_names, > > > > + .num_regs = ARRAY_SIZE(imx8mq_g1_reg_names), > > > > }; > > > > > > > > const struct hantro_variant imx8mq_vpu_g2_variant = { > > > > @@ -314,10 +227,10 @@ const struct hantro_variant imx8mq_vpu_g2_variant = { > > > > .postproc_ops = &hantro_g2_postproc_ops, > > > > .codec = HANTRO_HEVC_DECODER | HANTRO_VP9_DECODER, > > > > .codec_ops = imx8mq_vpu_g2_codec_ops, > > > > - .init = imx8mq_vpu_hw_init, > > > > - .runtime_resume = imx8mq_runtime_resume, > > > > .irqs = imx8mq_g2_irqs, > > > > .num_irqs = ARRAY_SIZE(imx8mq_g2_irqs), > > > > - .clk_names = imx8mq_clk_names, > > > > - .num_clocks = ARRAY_SIZE(imx8mq_clk_names), > > > > + .clk_names = imx8mq_g2_clk_names, > > > > + .num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names), > > > > + .reg_names = imx8mq_g2_reg_names, > > > > + .num_regs = ARRAY_SIZE(imx8mq_g2_reg_names), > > > > }; > > > > -- > > > > 2.32.0 > > > > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel