Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH 3/3] media: vsp1: Add support for RZ/G2L VSPD
>
> Hi Biju,
>
> On Thu, Mar 10, 2022 at 12:11 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > > Subject: Re: [PATCH 3/3] media: vsp1: Add support for RZ/G2L VSPD
> > > Quoting Biju Das (2022-03-09 19:45:21)
> > > > The RZ/G2L VSPD provides a single VSPD instance. It has the
> > > > following sub modules MAU, CTU, RPF, DPR, LUT, BRS, WPF and LIF.
> > > >
> > > > The VSPD block on RZ/G2L does not have a version register, so
> > > > added a new compatible string "renesas,vsp2-rzg2l" with a data
> > > > pointer containing
>
> > > > drivers/media/platform/vsp1/vsp1_drv.c | 32
> > > > +++++++++++++++++++------ drivers/media/platform/vsp1/vsp1_lif.c
> > > > +++++++++++++++++++|
> > > > 7 ++++-- drivers/media/platform/vsp1/vsp1_regs.h | 1 +
> > > > 3 files changed, 31 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> > > > b/drivers/media/platform/vsp1/vsp1_drv.c
> > > > index 77da6a6732d8..40c6d9290681 100644
> > > > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > > > +++ b/drivers/media/platform/vsp1/vsp1_drv.c
>
> > > > if (IS_ERR(vsp1->rstc))
> > > > return dev_err_probe(&pdev->dev, PTR_ERR(vsp1-
> >rstc),
> > > > "failed to get reset
> > > > ctrl\n"); @@
> > > > -874,13 +889,15 @@ static int vsp1_probe(struct platform_device
> *pdev)
> > > > if (ret < 0)
> > > > goto done;
> > > >
> > > > - vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > > > + if (!vsp1->info) {
> > > > + vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > > >
> > > > - for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> > > > - if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) ==
> > > > - vsp1_device_infos[i].version) {
> > > > - vsp1->info = &vsp1_device_infos[i];
> > > > - break;
> > > > + for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i)
> {
> > > > + if ((vsp1->version &
> > > > + VI6_IP_VERSION_MODEL_MASK)
> > > ==
> > > > + vsp1_device_infos[i].version) {
> > > > + vsp1->info = &vsp1_device_infos[i];
> > > > + break;
> > > > + }
> > >
> > >
> > > This is looking like it gets a bit awkward. Two methods for
> > > identifying the version and info structure is going to be a pain.
> >
> > On RFC, Laurent suggested to use info for RZ/G2L. Do you have better
> > Suggestion? Please let me know.
>
> I'm afraid we have no other option. But the flow could be made prettier
> by moving the table lookup into its own function, and using something like
> below in the probe function:
Sounds good to me. Kieran/Laurent, are you OK with Geert's suggestion?
>
> vsp1->info = of_device_get_match_data(&pdev->dev);
> if (!vsp1->info)
> vsp1->info = vsp1_lookup(vsp1_read(vsp1, VI6_IP_VERSION));
> if (!vsp1->info)
> return -ENODEV.
>
> > > > @@ -943,6 +960,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-rzg2l", .data =
> > > > + &vsp1_device_infos[14] },
> > >
> > > I don't think you should reference a specific index of the infos
> table.
> > > What happens if someone adds an entry higher in the table which
> > > pushes the indexes down ?
> >
> > I can think of adding macros in info structure and use that macro here
> > to avoid such condition, if it all needed.
> >
> > Do you have any other better alternative to handle this scenario?
> > Please let me know.
>
> I would use a pointer to an independent struct vsp1_device_info, not part
> of vsp1_device_infos[], so it can never be matched by accident (see
> below).
Sounds good to me. Kieran/Laurent, are you OK with Geert's suggestion?
>
> > > > --- 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)
> > >
> > > I don't like the idea of using a value here that could really be
> > > used on a real device somewhere.
> > >
> > > The hole in the sequence is only there because we havent' seen a
> > > datasheet with 0x1b defined.
> > >
> > > If there truely is no version register on this hardware, we're going
> > > to have to make sure this version value can't conflict.
> >
> > Currently, I don't see any device with 0x1b. If in future if we found
> > a device With 0x1b, This can be moved to a higher value for eg:- 0xfe.
> >
> > Please let me know your thoughts.
>
> I agree with Kieran, and strongly recommend against using a number that
> might exist for real on current or future SoCs. Unfortunately there's
> only 8 bits available, precluding the use of e.g. (0xbeef01 << 8). But
> starting from (0xff << 8), and counting down for future entries, if
> needed, sounds like a good compromise.
>
> And of course there should be a comment next to the definition, to make it
> clear this is a made-up number.
Sounds good to me. Kieran/Laurent, are you OK with Geert's suggestion?
>
> P.S. If possible, please communicate to the hardware engineers it
> was IMHO a bad decision to get rid of the version register,
> which should be reconsidered for future SoCs.
I am checking with HW engineer, I will update you once I get feedback from them.
Cheers,
Biju
