| Message ID | 20220309194521.7028-4-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for RZ/G2L VSPD | expand |
Hi Biju, On Wed, Mar 9, 2022 at 8:45 PM Biju Das <biju.das.jz@bp.renesas.com> 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. > > 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 > the info structure. Also the reset line is shared with the DU module > so devm_reset_control_get_shared() call is used in case of RZ/G2L. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > RFC->v1: > * Used data pointer containing info structure to retrieve version information > RFC: > * https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-21-biju.das.jz@bp.renesas.com/ Thanks for the update! > --- a/drivers/media/platform/vsp1/vsp1_drv.c > +++ b/drivers/media/platform/vsp1/vsp1_drv.c > @@ -841,7 +849,14 @@ static int vsp1_probe(struct platform_device *pdev) > if (irq < 0) > return irq; > > - vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); > + vsp1->info = of_device_get_match_data(&pdev->dev); > + if (vsp1->info) { > + vsp1->version = vsp1->info->version; > + vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, NULL); > + } else { > + vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); Making the reset control shared or exclusive dependent on the presence of match data looks fragile to me. I think you want to check the IP version instead (ideally, the SoC, as this is an integration feature). Or just make it shared unconditionally (in the previous patch)? > + } > + > 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; > + } > } > } > > @@ -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] }, > { }, Is VI6_IP_VERSION_MODEL_VSPD_RZG2L = 0x1b an official number? If yes, it might make sense to change the compatible value to "renesas,vsp2-0x1b". Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH 3/3] media: vsp1: Add support for RZ/G2L VSPD > > Hi Biju, > > On Wed, Mar 9, 2022 at 8:45 PM Biju Das <biju.das.jz@bp.renesas.com> > 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. > > > > 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 the info structure. Also the reset line is shared with the > > DU module so devm_reset_control_get_shared() call is used in case of > RZ/G2L. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > RFC->v1: > > * Used data pointer containing info structure to retrieve version > > information > > RFC: > > * > > Thanks for the update! > > > --- a/drivers/media/platform/vsp1/vsp1_drv.c > > +++ b/drivers/media/platform/vsp1/vsp1_drv.c > > > @@ -841,7 +849,14 @@ static int vsp1_probe(struct platform_device *pdev) > > if (irq < 0) > > return irq; > > > > - vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); > > + vsp1->info = of_device_get_match_data(&pdev->dev); > > + if (vsp1->info) { > > + vsp1->version = vsp1->info->version; > > + vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, > NULL); > > + } else { > > + vsp1->rstc = > > + devm_reset_control_get_exclusive(&pdev->dev, NULL); > > Making the reset control shared or exclusive dependent on the presence of > match data looks fragile to me. I think you want to check the IP version > instead (ideally, the SoC, as this is an integration feature). > Or just make it shared unconditionally (in the previous patch)? Agreed. > > > + } > > + > > 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; > > + } > > } > > } > > > > @@ -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] }, > > { }, > > Is VI6_IP_VERSION_MODEL_VSPD_RZG2L = 0x1b an official number? > If yes, it might make sense to change the compatible value to > "renesas,vsp2-0x1b". No, it is not official one. I just use 0x1b as no one claimed it. Cheers, Biju
Hi Biju, 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 Does this mean it is 'not' a VSP2? Is it a VSP2-lite or something different? (As opposed to 'the vsp2 found in an rzg2l part'). > the info structure. Also the reset line is shared with the DU module > so devm_reset_control_get_shared() call is used in case of RZ/G2L. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > RFC->v1: > * Used data pointer containing info structure to retrieve version information > RFC: > * https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-21-biju.das.jz@bp.renesas.com/ > --- > 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 > @@ -811,6 +811,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, > }, > }; > > @@ -841,7 +849,14 @@ static int vsp1_probe(struct platform_device *pdev) > if (irq < 0) > return irq; > > - vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); > + vsp1->info = of_device_get_match_data(&pdev->dev); > + if (vsp1->info) { > + vsp1->version = vsp1->info->version; > + vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, NULL); > + } else { > + vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); > + } > + I'll leave this as Geert has already commented. > 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. > } > } > > @@ -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 ? > { }, > }; > 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)) The comment here directly references V3M, and you haven't updated it. But if this is going to grow I wonder if it will end up needing a quirks flag that can be set per device in the vsp1_device_info rather than coding a massive conditional if (platform x or platform y or platform z.3); > 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) 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. -- Kieran > #define VI6_IP_VERSION_MODEL_VSPD_V3U (0x1c << 8) > > #define VI6_IP_VERSION_SOC_MASK (0xff << 0) > -- > 2.17.1 >
Hi Kieran, Thanks for the feedback. > Subject: Re: [PATCH 3/3] media: vsp1: Add support for RZ/G2L VSPD > > Hi Biju, > > 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 > > Does this mean it is 'not' a VSP2? Is it a VSP2-lite or something > different? (As opposed to 'the vsp2 found in an rzg2l part'). It is just VSPD, see the hardware manual[1]. It does not mention about VSP2-lite. [1] https://www.renesas.com/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0?r=1467981 > > > > the info structure. Also the reset line is shared with the DU module > > so devm_reset_control_get_shared() call is used in case of RZ/G2L. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > RFC->v1: > > * Used data pointer containing info structure to retrieve version > > information > > RFC: > > * > > --- > > 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 > > @@ -811,6 +811,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, > > }, > > }; > > > > @@ -841,7 +849,14 @@ static int vsp1_probe(struct platform_device *pdev) > > if (irq < 0) > > return irq; > > > > - vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); > > + vsp1->info = of_device_get_match_data(&pdev->dev); > > + if (vsp1->info) { > > + vsp1->version = vsp1->info->version; > > + vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, > NULL); > > + } else { > > + vsp1->rstc = devm_reset_control_get_exclusive(&pdev- > >dev, NULL); > > + } > > + > > I'll leave this as Geert has already commented. > > > 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. > > > > } > > } > > > > @@ -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. > > > > { }, > > }; > > 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)) > > The comment here directly references V3M, and you haven't updated it. > But if this is going to grow I wonder if it will end up needing a quirks > flag that can be set per device in the vsp1_device_info rather than coding > a massive conditional if (platform x or platform y or platform z.3); OK I can update the comment above in next version. Currently only V2M and RZ/G2L need this change. In future if it all grows, we can revisit and add the flags in info structure. Are you ok with it? Please let me know. > > > 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) > > 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. Cheers, biju > > > > #define VI6_IP_VERSION_MODEL_VSPD_V3U (0x1c << 8) > > > > #define VI6_IP_VERSION_SOC_MASK (0xff << 0) > > -- > > 2.17.1 > >
Hi Biju, On Thu, Mar 10, 2022 at 12:11 PM Biju Das <biju.das.jz@bp.renesas.com> 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@linux-m68k.org 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
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@bp.renesas.com> > 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
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 @@ -811,6 +811,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, }, }; @@ -841,7 +849,14 @@ static int vsp1_probe(struct platform_device *pdev) if (irq < 0) return irq; - vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); + vsp1->info = of_device_get_match_data(&pdev->dev); + if (vsp1->info) { + vsp1->version = vsp1->info->version; + vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, NULL); + } else { + vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); + } + 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; + } } } @@ -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] }, { }, }; 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)