Hi Laurent,
Thanks for the feedback.
> Subject: Re: [RFC 20/28] media: vsp1: Add support for the RZ/G2L VSPD
>
> Hi Biju,
>
> Thank you for the patch.
>
> On Wed, Jan 12, 2022 at 05:46:04PM +0000, Biju Das wrote:
> > The RZ/G2L VSPD provides a single VSPD instance. it has the following
> > sub modules MAU, CTU, RPF, DPR, LUT, BRS, WPF and LIF.
> >
> > It does not have version register, so added a new compatible string to
> > match to get the version value. Also the reset is shared with DU
> > module.
>
> Does it really lack the version register, or is it just not documented ?
> It hasn't been documented on all R-Car variants, but has consistently been
> present.
No, it is not present on RZ/G2L. the read value of this register is 0x0.
>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > drivers/media/platform/vsp1/vsp1.h | 1 +
> > drivers/media/platform/vsp1/vsp1_drv.c | 31
> > ++++++++++++++++++++++++- drivers/media/platform/vsp1/vsp1_lif.c |
> > 7 ++++-- drivers/media/platform/vsp1/vsp1_regs.h | 1 +
> > 4 files changed, 37 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1.h
> > b/drivers/media/platform/vsp1/vsp1.h
> > index 37cf33c7e6ca..b137c0233db5 100644
> > --- a/drivers/media/platform/vsp1/vsp1.h
> > +++ b/drivers/media/platform/vsp1/vsp1.h
> > @@ -103,6 +103,7 @@ struct vsp1_device {
> > struct media_entity_operations media_ops;
> >
> > struct vsp1_drm *drm;
> > + struct reset_control *rstc;
>
> Could you move this with the toher resources, just after the bus_master
> field ?
Agreed.
>
> > };
> >
> > int vsp1_device_get(struct vsp1_device *vsp1); diff --git
> > a/drivers/media/platform/vsp1/vsp1_drv.c
> > b/drivers/media/platform/vsp1/vsp1_drv.c
> > index c9044785b903..c00ba65030fd 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> > @@ -16,6 +16,7 @@
> > #include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/pm_runtime.h>
> > +#include <linux/reset.h>
> > #include <linux/videodev2.h>
> >
> > #include <media/rcar-fcp.h>
> > @@ -559,6 +560,15 @@ static int vsp1_device_init(struct vsp1_device
> *vsp1)
> > */
> > int vsp1_device_get(struct vsp1_device *vsp1) {
> > + int ret;
> > +
> > + if (vsp1->rstc) {
> > + ret = reset_control_deassert(vsp1->rstc);
>
> Adding reset support could be split to a separate patch.
Agreed.
>
> > + if (ret < 0) {
> > + reset_control_assert(vsp1->rstc);
>
> Is asserting reset needed here ?
Reset is shared between DU and VSPD.
This is just to get balanced reset count like clocks,
As I am using shared reset.
>
> > + return ret;
> > + }
> > + }
> > return pm_runtime_resume_and_get(vsp1->dev);
> > }
> >
> > @@ -571,6 +581,8 @@ int vsp1_device_get(struct vsp1_device *vsp1)
> > void vsp1_device_put(struct vsp1_device *vsp1) {
> > pm_runtime_put_sync(vsp1->dev);
> > + if (vsp1->rstc)
> > + reset_control_assert(vsp1->rstc);
> > }
> >
> > /*
> > ----------------------------------------------------------------------
> > ------- @@ -787,6 +799,14 @@ static const struct vsp1_device_info
> > vsp1_device_infos[] = {
> > .uif_count = 2,
> > .wpf_count = 1,
> > .num_bru_inputs = 5,
> > + }, {
> > + .version = VI6_IP_VERSION_MODEL_VSPD_RZG2L,
> > + .model = "VSP2-D",
> > + .gen = 3,
> > + .features = VSP1_HAS_BRS | VSP1_HAS_WPF_VFLIP |
> VSP1_HAS_EXT_DL,
> > + .lif_count = 1,
> > + .rpf_count = 2,
> > + .wpf_count = 1,
> > },
> > };
> >
> > @@ -826,6 +846,13 @@ static int vsp1_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > + vsp1->version = (uintptr_t)of_device_get_match_data(&pdev->dev);
> > + if (vsp1->version == VI6_IP_VERSION_MODEL_VSPD_RZG2L) {
> > + vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> > + if (IS_ERR(vsp1->rstc))
> > + return PTR_ERR(vsp1->rstc);
>
> As the resets DT property is mandatory, and is present in all .dtsi in
> mainline, should the devm_reset_control_get_shared() call be made for all
> VSPs ?
Agreed. Will call devm_reset_control_get_exclusive for all other VSPs except RZ/G2L,
As other VSP's have exclusive resets.
>
> > + }
> > +
> > /* FCP (optional). */
> > fcp_node = of_parse_phandle(pdev->dev.of_node, "renesas,fcp", 0);
> > if (fcp_node) {
> > @@ -854,7 +881,8 @@ static int vsp1_probe(struct platform_device *pdev)
> > if (ret < 0)
> > goto done;
> >
> > - vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > + if (vsp1->version != VI6_IP_VERSION_MODEL_VSPD_RZG2L)
> > + vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > vsp1_device_put(vsp1);
>
> You could condition the whole block of vsp1_device_get(), vsp1_read() and
> vsp1_device_put() on the version not being set.
Agreed.
>
> >
> > for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) { @@ -905,6
> > +933,7 @@ static int vsp1_remove(struct platform_device *pdev) static
> > const struct of_device_id vsp1_of_match[] = {
> > { .compatible = "renesas,vsp1" },
> > { .compatible = "renesas,vsp2" },
> > + { .compatible = "renesas,vsp2-r9a07g044", .data = (void
> > +*)VI6_IP_VERSION_MODEL_VSPD_RZG2L },
>
> Let's point to the vsp1_device_infos entry instead.
Agreed.
Regards,
Biju
>
> > { },
> > };
> > MODULE_DEVICE_TABLE(of, vsp1_of_match); diff --git
> > a/drivers/media/platform/vsp1/vsp1_lif.c
> > b/drivers/media/platform/vsp1/vsp1_lif.c
> > index 6a6857ac9327..6e997653cfac 100644
> > --- a/drivers/media/platform/vsp1/vsp1_lif.c
> > +++ b/drivers/media/platform/vsp1/vsp1_lif.c
> > @@ -107,6 +107,7 @@ static void lif_configure_stream(struct
> > vsp1_entity *entity,
> >
> > case VI6_IP_VERSION_MODEL_VSPDL_GEN3:
> > case VI6_IP_VERSION_MODEL_VSPD_V3:
> > + case VI6_IP_VERSION_MODEL_VSPD_RZG2L:
> > hbth = 0;
> > obth = 1500;
> > lbth = 0;
> > @@ -135,8 +136,10 @@ static void lif_configure_stream(struct vsp1_entity
> *entity,
> > * may appear on the output). The value required by the manual is
> not
> > * explained but is likely a buffer size or threshold.
> > */
> > - if ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
> > - (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
> > + if (((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
> > + (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) ||
> > + ((entity->vsp1->version & VI6_IP_VERSION_MASK) ==
> > + VI6_IP_VERSION_MODEL_VSPD_RZG2L))
> > vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
> > VI6_LIF_LBA_LBA0 |
> > (1536 << VI6_LIF_LBA_LBA1_SHIFT)); diff --git
> > a/drivers/media/platform/vsp1/vsp1_regs.h
> > b/drivers/media/platform/vsp1/vsp1_regs.h
> > index fae7286eb01e..12c5b09885dc 100644
> > --- a/drivers/media/platform/vsp1/vsp1_regs.h
> > +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> > @@ -766,6 +766,7 @@
> > #define VI6_IP_VERSION_MODEL_VSPD_V3 (0x18 << 8)
> > #define VI6_IP_VERSION_MODEL_VSPDL_GEN3 (0x19 << 8)
> > #define VI6_IP_VERSION_MODEL_VSPBS_GEN3 (0x1a << 8)
> > +#define VI6_IP_VERSION_MODEL_VSPD_RZG2L (0x1b << 8)
> > #define VI6_IP_VERSION_MODEL_VSPD_V3U (0x1c << 8)
> >
> > #define VI6_IP_VERSION_SOC_MASK (0xff << 0)
>
> --
> Regards,
>
> Laurent Pinchart
