Re: [PATCH 3/3] media: vsp1: Add support for RZ/G2L VSPD

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux