Re: [PATCH 5/5] media: hantro: Add support for the Rockchip PX30
From: Ezequiel Garcia <hidden>
Date: 2021-01-07 19:09:03
Also in:
linux-arm-kernel, linux-media, linux-rockchip, lkml
Hi Paul, Happy to see this patch. It was on my TODO list, but I hadn't had time to bringup my rk3326 device. A few comments. On Thu, 2021-01-07 at 14:41 +0100, Paul Kocialkowski wrote:
The PX30 SoC includes both the VDPU2 and VEPU2 blocks which are similar to the RK3399 (Hantro G1/H1 with shuffled registers). Besides taking an extra clock, it also shares an interrupt with the IOMMU so it's necessary to request the interrupt shared.
Could you clarify on the commit description which iommu device interrupt line is being shared?
quoted hunk ↗ jump to hunk
Signed-off-by: Paul Kocialkowski <redacted> --- drivers/staging/media/hantro/hantro_drv.c | 5 +++-- drivers/staging/media/hantro/hantro_hw.h | 1 + drivers/staging/media/hantro/rk3399_vpu_hw.c | 21 ++++++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-)diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c index e5f200e64993..076a7782b476 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c@@ -472,6 +472,7 @@ static const struct v4l2_file_operations hantro_fops = {static const struct of_device_id of_hantro_match[] = { #ifdef CONFIG_VIDEO_HANTRO_ROCKCHIP + { .compatible = "rockchip,px30-vpu", .data = &px30_vpu_variant, }, { .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, }, { .compatible = "rockchip,rk3328-vpu", .data = &rk3328_vpu_variant, }, { .compatible = "rockchip,rk3288-vpu", .data = &rk3288_vpu_variant, },@@ -796,8 +797,8 @@ static int hantro_probe(struct platform_device *pdev)return -ENXIO; ret = devm_request_irq(vpu->dev, irq, - vpu->variant->irqs[i].handler, 0, - dev_name(vpu->dev), vpu); + vpu->variant->irqs[i].handler, + IRQF_SHARED, dev_name(vpu->dev), vpu);
Maybe this irq flag should be part of vpu->variant? It sounds like an IP block integration specific thing. Also, you will need a px30-specific interrupt handler now, since the rk3399 one is not shared-friendly.
quoted hunk ↗ jump to hunk
if (ret) { dev_err(vpu->dev, "Could not request %s IRQ.\n", irq_name);diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h index 34c9e4649a25..07f516fd7a2e 100644 --- a/drivers/staging/media/hantro/hantro_hw.h +++ b/drivers/staging/media/hantro/hantro_hw.h@@ -148,6 +148,7 @@ enum hantro_enc_fmt {RK3288_VPU_ENC_FMT_UYVY422 = 3, }; +extern const struct hantro_variant px30_vpu_variant; extern const struct hantro_variant rk3399_vpu_variant; extern const struct hantro_variant rk3328_vpu_variant; extern const struct hantro_variant rk3288_vpu_variant;diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw.c b/drivers/staging/media/hantro/rk3399_vpu_hw.c index 7a7962cf771e..4112f98baa60 100644 --- a/drivers/staging/media/hantro/rk3399_vpu_hw.c +++ b/drivers/staging/media/hantro/rk3399_vpu_hw.c
Perhaps it's time to rename this to rockchip_vpu_hw.c, and merge rk3288 and rk3399? It's a nitpick, though.
quoted hunk ↗ jump to hunk
@@ -220,3 +220,24 @@ const struct hantro_variant rk3328_vpu_variant = {.clk_names = rk3399_clk_names, .num_clocks = ARRAY_SIZE(rk3399_clk_names), }; + +static const char * const px30_clk_names[] = { + "aclk", "hclk", "sclk" +}; + +const struct hantro_variant px30_vpu_variant = { + .enc_offset = 0x0, + .enc_fmts = rk3399_vpu_enc_fmts, + .num_enc_fmts = ARRAY_SIZE(rk3399_vpu_enc_fmts), + .dec_offset = 0x400, + .dec_fmts = rk3399_vpu_dec_fmts, + .num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts), + .codec = HANTRO_JPEG_ENCODER | HANTRO_MPEG2_DECODER | + HANTRO_VP8_DECODER, + .codec_ops = rk3399_vpu_codec_ops, + .irqs = rk3399_irqs, + .num_irqs = ARRAY_SIZE(rk3399_irqs), + .init = rk3399_vpu_hw_init, + .clk_names = px30_clk_names, + .num_clocks = ARRAY_SIZE(px30_clk_names) +};
Thanks, Ezequiel