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@xxxxxxxxxxxxxx> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > --- > 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@xxxxxxxxxxxxxx/ > --- > 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 >
