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:
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).
> > > --- 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.
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.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
