| Message ID | 20220309194521.7028-4-biju.das.jz@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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 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 > >
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)