* [PATCH v6 0/3] Add support for RZ/G2L VSPD
@ 2022-03-16 11:55 Biju Das
2022-03-16 11:55 ` [PATCH v6 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings Biju Das
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Biju Das @ 2022-03-16 11:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Philipp Zabel
Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media,
linux-renesas-soc, devicetree, Geert Uytterhoeven,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
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,rzg2l-vsp2" with a data pointer containing
the info structure. Also the reset line is shared with the DU module.
v5->v6:
* Rebased to media_staging and updated commit header
* Removed LCDC reference clock description from bindings
* Changed the clock name from du.0->aclk from bindings
* Added Rb tag from Laurent for reset patch
* Added forward declaration for struct reset_control
* Updated vsp1_device_get() with changes suggested by Laurent
* Updated error message for reset_control_get form ctrl->control.
* Removed the extra tab from rzg2l_vsp2_device_info
* Changed the function vsp1_lookup->vsp1_lookup_info and
all info match related code moved here.
* Add VI6_IP_VERSION_VSP and VI6_IP_VERSION_VSP_SW macros to
distinguish HW & SW IP_VSP_Version.
* Used 0x80 for RZG2L VSPD model and SoC identification
* Updated Switch() for LIF0 buffer attribute handling.
v4->v5:
* Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
* To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
for SoC identification for RZ/G2L SoC's.
v3->v4:
* Restored error check for pm_runtime_resume_and_get and calls
assert() in case of failure.
* Added Rb tag from Geert
* Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M SoC's
v2->v3:
* Added Rb tags from Krzysztof and Philipp
* If reset_control_deassert() failed, return ret directly.
* Fixed version comparison in vsp1_lookup()
v1->v2:
* Used reference counted reset handle to perform deassert/assert
* Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
* Added standalone device info for rzg2l-vsp2.
* Added vsp1_lookup helper function.
* Updated comments for LIF0 buffer attribute register
* Used last ID for rzg2l-vsp2.
RFC->v1:
* Added reset support as separate patch
* Moved rstc just after the bus_master field in struct vsp1_device
* Used data pointer containing info structure to retrieve version information
* Updated commit description
* Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
* Defined the clocks
* Clock max Items is based on SoC Compatible string
RFC:
* https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-21-biju.das.jz@bp.renesas.com/
* https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-20-biju.das.jz@bp.renesas.com/
Biju Das (3):
media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD
bindings
media: renesas: vsp1: Add support to deassert/assert reset line
media: renesas: vsp1: Add support for RZ/G2L VSPD
.../bindings/media/renesas,vsp1.yaml | 52 +++++++++----
drivers/media/platform/renesas/vsp1/vsp1.h | 2 +
.../media/platform/renesas/vsp1/vsp1_drv.c | 75 +++++++++++++++----
.../media/platform/renesas/vsp1/vsp1_lif.c | 18 +++--
.../media/platform/renesas/vsp1/vsp1_regs.h | 8 ++
5 files changed, 121 insertions(+), 34 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v6 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings
2022-03-16 11:55 [PATCH v6 0/3] Add support for RZ/G2L VSPD Biju Das
@ 2022-03-16 11:55 ` Biju Das
2022-03-16 11:55 ` [PATCH v6 2/3] media: renesas: vsp1: Add support to deassert/assert reset line Biju Das
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Biju Das @ 2022-03-16 11:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Rob Herring
Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media,
linux-renesas-soc, devicetree, Geert Uytterhoeven,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
Document VSPD found in RZ/G2L and RZ/V2L family SoC's. VSPD block is
similar to VSP2-D found on R-Car SoC's, but it does not have a version
register and it has 3 clocks compared to 1 clock on vsp1 and vsp2.
This patch introduces a new compatible 'renesas,rzg2l-vsp2' to handle
these differences.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
v5->v6:
* Removed LCDC reference clock description
* Changed the clock name from du.0->aclk
v4->v5:
* No change
v3->v4:
* No change
v2->v3:
* Added Rb tag from Krzysztof.
v1->v2:
* Changed compatible from vsp2-rzg2l->rzg2l-vsp2
RFC->v1:
* Updated commit description
* Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
* Defined the clocks
* Clock max Items is based on SoC Compatible string
RFC:
* https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-20-biju.das.jz@bp.renesas.com/
---
.../bindings/media/renesas,vsp1.yaml | 52 ++++++++++++++-----
1 file changed, 39 insertions(+), 13 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
index 990e9c1dbc43..a236b266fa4b 100644
--- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
@@ -19,6 +19,7 @@ properties:
enum:
- renesas,vsp1 # R-Car Gen2 and RZ/G1
- renesas,vsp2 # R-Car Gen3 and RZ/G2
+ - renesas,rzg2l-vsp2 # RZ/G2L and RZ/V2L
reg:
maxItems: 1
@@ -26,8 +27,8 @@ properties:
interrupts:
maxItems: 1
- clocks:
- maxItems: 1
+ clocks: true
+ clock-names: true
power-domains:
maxItems: 1
@@ -50,17 +51,42 @@ required:
additionalProperties: false
-if:
- properties:
- compatible:
- items:
- - const: renesas,vsp1
-then:
- properties:
- renesas,fcp: false
-else:
- required:
- - renesas,fcp
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: renesas,vsp1
+ then:
+ properties:
+ renesas,fcp: false
+ else:
+ required:
+ - renesas,fcp
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: renesas,rzg2l-vsp2
+ then:
+ properties:
+ clocks:
+ items:
+ - description: Main clock
+ - description: Register access clock
+ - description: Video clock
+ clock-names:
+ items:
+ - const: aclk
+ - const: pclk
+ - const: vclk
+ required:
+ - clock-names
+ else:
+ properties:
+ clocks:
+ maxItems: 1
examples:
# R8A7790 (R-Car H2) VSP1-S
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v6 2/3] media: renesas: vsp1: Add support to deassert/assert reset line
2022-03-16 11:55 [PATCH v6 0/3] Add support for RZ/G2L VSPD Biju Das
2022-03-16 11:55 ` [PATCH v6 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings Biju Das
@ 2022-03-16 11:55 ` Biju Das
2022-03-16 11:55 ` [PATCH v6 3/3] media: renesas: vsp1: Add support for RZ/G2L VSPD Biju Das
2022-04-14 7:15 ` [PATCH v6 0/3] " Biju Das
3 siblings, 0 replies; 8+ messages in thread
From: Biju Das @ 2022-03-16 11:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Philipp Zabel
Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media,
linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
As the resets DT property is mandatory, and is present in all .dtsi
in mainline, add support to perform deassert/assert using reference
counted reset handle.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
v5->v6:
* Rebased to media_staging and updated commit header
* Added Rb tag from Laurent
* Added forward declaration for struct reset_control
* Updated vsp1_device_get() with changes suggested by Laurent
* Updated error message for reset_control_get form ctrl->control.
v4->v5:
* Added Rb tag from Geert
v3->v4:
* Restored error check for pm_runtime_resume_and_get and calls
assert() in case of failure.
v2->v3:
* Added Rb tag from Philipp
* If reset_control_deassert() failed, return ret directly.
v1->v2:
* Used reference counted reset handle to perform deassert/assert
RFC->v1:
* Added reset support as separate patch
* Moved rstc just after the bus_master field in struct vsp1_device
RFC:
* https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-21-biju.das.jz@bp.renesas.com/
---
drivers/media/platform/renesas/vsp1/vsp1.h | 2 ++
.../media/platform/renesas/vsp1/vsp1_drv.c | 19 ++++++++++++++++++-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/renesas/vsp1/vsp1.h b/drivers/media/platform/renesas/vsp1/vsp1.h
index 37cf33c7e6ca..baf898d577ec 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1.h
@@ -22,6 +22,7 @@
struct clk;
struct device;
struct rcar_fcp_device;
+struct reset_control;
struct vsp1_drm;
struct vsp1_entity;
@@ -79,6 +80,7 @@ struct vsp1_device {
void __iomem *mmio;
struct rcar_fcp_device *fcp;
struct device *bus_master;
+ struct reset_control *rstc;
struct vsp1_brx *brs;
struct vsp1_brx *bru;
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
index 502c7d9d6890..159b68fa0829 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -16,6 +16,7 @@
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+#include <linux/reset.h>
#include <linux/videodev2.h>
#include <media/rcar-fcp.h>
@@ -569,7 +570,17 @@ static void vsp1_mask_all_interrupts(struct vsp1_device *vsp1)
*/
int vsp1_device_get(struct vsp1_device *vsp1)
{
- return pm_runtime_resume_and_get(vsp1->dev);
+ int ret;
+
+ ret = reset_control_deassert(vsp1->rstc);
+ if (ret < 0)
+ return ret;
+
+ ret = pm_runtime_resume_and_get(vsp1->dev);
+ if (ret < 0)
+ reset_control_assert(vsp1->rstc);
+
+ return ret;
}
/*
@@ -581,6 +592,7 @@ int vsp1_device_get(struct vsp1_device *vsp1)
void vsp1_device_put(struct vsp1_device *vsp1)
{
pm_runtime_put_sync(vsp1->dev);
+ reset_control_assert(vsp1->rstc);
}
/* -----------------------------------------------------------------------------
@@ -827,6 +839,11 @@ static int vsp1_probe(struct platform_device *pdev)
if (irq < 0)
return irq;
+ vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
+ if (IS_ERR(vsp1->rstc))
+ return dev_err_probe(&pdev->dev, PTR_ERR(vsp1->rstc),
+ "failed to get reset control\n");
+
/* FCP (optional). */
fcp_node = of_parse_phandle(pdev->dev.of_node, "renesas,fcp", 0);
if (fcp_node) {
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v6 3/3] media: renesas: vsp1: Add support for RZ/G2L VSPD
2022-03-16 11:55 [PATCH v6 0/3] Add support for RZ/G2L VSPD Biju Das
2022-03-16 11:55 ` [PATCH v6 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings Biju Das
2022-03-16 11:55 ` [PATCH v6 2/3] media: renesas: vsp1: Add support to deassert/assert reset line Biju Das
@ 2022-03-16 11:55 ` Biju Das
2022-04-14 8:39 ` Kieran Bingham
2022-04-14 7:15 ` [PATCH v6 0/3] " Biju Das
3 siblings, 1 reply; 8+ messages in thread
From: Biju Das @ 2022-03-16 11:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Philipp Zabel
Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media,
linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
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,rzg2l-vsp2" with a data pointer containing
the info structure. Also the reset line is shared with the DU module.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v5->v6:
* Rebased to media_staging and updated commit header
* Removed the extra tab from rzg2l_vsp2_device_info
* Changed the function vsp1_lookup->vsp1_lookup_info and
all info match related code moved here.
* Add VI6_IP_VERSION_VSP and VI6_IP_VERSION_VSP_SW macros to
distinguish HW & SW IP_VSP_Version.
* Used 0x80 for RZG2L VSPD model and SoC identification
* Updated Switch() for LIF0 buffer attribute handling.
v4->v5:
* Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
* To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
for RZ/G2L SoC's.
v3->v4:
* Added Rb tag from Geert
* Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M
v2->v3:
* Fixed version comparison in vsp1_lookup()
v1->v2:
* Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
* Added standalone device info for rzg2l-vsp2.
* Added vsp1_lookup helper function.
* Updated comments for LIF0 buffer attribute register
* Used last ID for rzg2l-vsp2.
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/
---
.../media/platform/renesas/vsp1/vsp1_drv.c | 56 ++++++++++++++-----
.../media/platform/renesas/vsp1/vsp1_lif.c | 18 ++++--
.../media/platform/renesas/vsp1/vsp1_regs.h | 8 +++
3 files changed, 62 insertions(+), 20 deletions(-)
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
index 159b68fa0829..f1f52c0c1c59 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
@@ -812,11 +812,47 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
},
};
+static const struct vsp1_device_info rzg2l_vsp2_device_info = {
+ .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,
+};
+
+static const struct vsp1_device_info *vsp1_lookup_info(struct vsp1_device *vsp1)
+{
+ const struct vsp1_device_info *info;
+ unsigned int i;
+
+ /*
+ * Try the info stored in match data first for devices that don't have
+ * a version register.
+ */
+ info = of_device_get_match_data(vsp1->dev);
+ if (info)
+ return info;
+
+ vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
+
+ for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
+ info = &vsp1_device_infos[i];
+
+ if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info->version)
+ return info;
+ }
+
+ dev_err(vsp1->dev, "unsupported IP version 0x%08x\n", vsp1->version);
+
+ return NULL;
+}
+
static int vsp1_probe(struct platform_device *pdev)
{
struct vsp1_device *vsp1;
struct device_node *fcp_node;
- unsigned int i;
int ret;
int irq;
@@ -872,25 +908,16 @@ static int vsp1_probe(struct platform_device *pdev)
if (ret < 0)
goto done;
- 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;
- }
- }
-
+ vsp1->info = vsp1_lookup_info(vsp1);
if (!vsp1->info) {
- dev_err(&pdev->dev, "unsupported IP version 0x%08x\n",
- vsp1->version);
vsp1_device_put(vsp1);
ret = -ENXIO;
goto done;
}
- dev_dbg(&pdev->dev, "IP version 0x%08x\n", vsp1->version);
+ if ((vsp1->version & VI6_IP_VERSION_VSP_MASK) != VI6_IP_VERSION_VSP)
+ vsp1->version = VI6_IP_VERSION_VSP_SW | vsp1->info->version |
+ VI6_IP_VERSION_SOC_RZG2L;
/*
* Previous use of the hardware (e.g. by the bootloader) could leave
@@ -941,6 +968,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,rzg2l-vsp2", .data = &rzg2l_vsp2_device_info },
{ },
};
MODULE_DEVICE_TABLE(of, vsp1_of_match);
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_lif.c b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
index 6a6857ac9327..e36ed2d2b22b 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
+++ b/drivers/media/platform/renesas/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;
@@ -130,16 +131,21 @@ static void lif_configure_stream(struct vsp1_entity *entity,
VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
/*
- * On R-Car V3M the LIF0 buffer attribute register has to be set to a
- * non-default value to guarantee proper operation (otherwise artifacts
- * may appear on the output). The value required by the manual is not
- * explained but is likely a buffer size or threshold.
+ * On R-Car V3M and RZ/G2L the LIF0 buffer attribute register has to be
+ * set to a non-default value to guarantee proper operation (otherwise
+ * artifacts 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))
+ switch (entity->vsp1->version) {
+ case (VI6_IP_VERSION_VSP | VI6_IP_VERSION_MODEL_VSPD_V3 |
+ VI6_IP_VERSION_SOC_V3M):
+ case (VI6_IP_VERSION_VSP_SW | VI6_IP_VERSION_MODEL_VSPD_RZG2L |
+ VI6_IP_VERSION_SOC_RZG2L):
vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
VI6_LIF_LBA_LBA0 |
(1536 << VI6_LIF_LBA_LBA1_SHIFT));
+ break;
+ }
}
static const struct vsp1_entity_operations lif_entity_ops = {
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
index fae7286eb01e..e66553c42e50 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
@@ -767,6 +767,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_V3U (0x1c << 8)
+/* RZ/G2L SoC's have no version register, So use 0x80 as the model version */
+#define VI6_IP_VERSION_MODEL_VSPD_RZG2L (0x80 << 8)
#define VI6_IP_VERSION_SOC_MASK (0xff << 0)
#define VI6_IP_VERSION_SOC_H2 (0x01 << 0)
@@ -780,6 +782,12 @@
#define VI6_IP_VERSION_SOC_M3N (0x04 << 0)
#define VI6_IP_VERSION_SOC_E3 (0x04 << 0)
#define VI6_IP_VERSION_SOC_V3U (0x05 << 0)
+/* RZ/G2L SoC's have no version register, So use 0x80 for SoC Identification */
+#define VI6_IP_VERSION_SOC_RZG2L (0x80 << 0)
+
+#define VI6_IP_VERSION_VSP_MASK (0xffff << 16)
+#define VI6_IP_VERSION_VSP (0x0101 << 16) /* HW VSP version */
+#define VI6_IP_VERSION_VSP_SW (0xfffe << 16) /* SW VSP version */
/* -----------------------------------------------------------------------------
* RPF CLUT Registers
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH v6 0/3] Add support for RZ/G2L VSPD
2022-03-16 11:55 [PATCH v6 0/3] Add support for RZ/G2L VSPD Biju Das
` (2 preceding siblings ...)
2022-03-16 11:55 ` [PATCH v6 3/3] media: renesas: vsp1: Add support for RZ/G2L VSPD Biju Das
@ 2022-04-14 7:15 ` Biju Das
3 siblings, 0 replies; 8+ messages in thread
From: Biju Das @ 2022-04-14 7:15 UTC (permalink / raw)
To: Biju Das, Mauro Carvalho Chehab, Philipp Zabel
Cc: Laurent Pinchart, Kieran Bingham, linux-media, linux-renesas-soc,
devicetree, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
Hi All,
Gentle ping. Are we happy with this patch set?
Please let me know.
Cheers,
Biju
> Subject: [PATCH v6 0/3] Add support for RZ/G2L VSPD
>
> 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,rzg2l-vsp2" with a data pointer containing the
> info structure. Also the reset line is shared with the DU module.
>
> v5->v6:
> * Rebased to media_staging and updated commit header
> * Removed LCDC reference clock description from bindings
> * Changed the clock name from du.0->aclk from bindings
> * Added Rb tag from Laurent for reset patch
> * Added forward declaration for struct reset_control
> * Updated vsp1_device_get() with changes suggested by Laurent
> * Updated error message for reset_control_get form ctrl->control.
> * Removed the extra tab from rzg2l_vsp2_device_info
> * Changed the function vsp1_lookup->vsp1_lookup_info and
> all info match related code moved here.
> * Add VI6_IP_VERSION_VSP and VI6_IP_VERSION_VSP_SW macros to
> distinguish HW & SW IP_VSP_Version.
> * Used 0x80 for RZG2L VSPD model and SoC identification
> * Updated Switch() for LIF0 buffer attribute handling.
> v4->v5:
> * Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
> * To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
> for SoC identification for RZ/G2L SoC's.
> v3->v4:
> * Restored error check for pm_runtime_resume_and_get and calls
> assert() in case of failure.
> * Added Rb tag from Geert
> * Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M
> SoC's
> v2->v3:
> * Added Rb tags from Krzysztof and Philipp
> * If reset_control_deassert() failed, return ret directly.
> * Fixed version comparison in vsp1_lookup()
> v1->v2:
> * Used reference counted reset handle to perform deassert/assert
> * Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
> * Added standalone device info for rzg2l-vsp2.
> * Added vsp1_lookup helper function.
> * Updated comments for LIF0 buffer attribute register
> * Used last ID for rzg2l-vsp2.
> RFC->v1:
> * Added reset support as separate patch
> * Moved rstc just after the bus_master field in struct vsp1_device
> * Used data pointer containing info structure to retrieve version
> information
> * Updated commit description
> * Changed compatible from vsp2-r9a07g044->vsp2-rzg2l
> * Defined the clocks
> * Clock max Items is based on SoC Compatible string
>
> RFC:
> *
>
> Biju Das (3):
> media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD
> bindings
> media: renesas: vsp1: Add support to deassert/assert reset line
> media: renesas: vsp1: Add support for RZ/G2L VSPD
>
> .../bindings/media/renesas,vsp1.yaml | 52 +++++++++----
> drivers/media/platform/renesas/vsp1/vsp1.h | 2 +
> .../media/platform/renesas/vsp1/vsp1_drv.c | 75 +++++++++++++++----
> .../media/platform/renesas/vsp1/vsp1_lif.c | 18 +++--
> .../media/platform/renesas/vsp1/vsp1_regs.h | 8 ++
> 5 files changed, 121 insertions(+), 34 deletions(-)
>
> --
> 2.17.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6 3/3] media: renesas: vsp1: Add support for RZ/G2L VSPD
2022-03-16 11:55 ` [PATCH v6 3/3] media: renesas: vsp1: Add support for RZ/G2L VSPD Biju Das
@ 2022-04-14 8:39 ` Kieran Bingham
2022-04-14 11:34 ` Biju Das
0 siblings, 1 reply; 8+ messages in thread
From: Kieran Bingham @ 2022-04-14 8:39 UTC (permalink / raw)
To: Biju Das, Mauro Carvalho Chehab, Philipp Zabel
Cc: Biju Das, Laurent Pinchart, linux-media, linux-renesas-soc,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
Quoting Biju Das (2022-03-16 11:55:51)
> 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,rzg2l-vsp2" with a data pointer containing
> the info structure. Also the reset line is shared with the DU module.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v5->v6:
> * Rebased to media_staging and updated commit header
> * Removed the extra tab from rzg2l_vsp2_device_info
> * Changed the function vsp1_lookup->vsp1_lookup_info and
> all info match related code moved here.
> * Add VI6_IP_VERSION_VSP and VI6_IP_VERSION_VSP_SW macros to
> distinguish HW & SW IP_VSP_Version.
> * Used 0x80 for RZG2L VSPD model and SoC identification
> * Updated Switch() for LIF0 buffer attribute handling.
> v4->v5:
> * Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
> * To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
> for RZ/G2L SoC's.
> v3->v4:
> * Added Rb tag from Geert
> * Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M
> v2->v3:
> * Fixed version comparison in vsp1_lookup()
> v1->v2:
> * Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
> * Added standalone device info for rzg2l-vsp2.
> * Added vsp1_lookup helper function.
> * Updated comments for LIF0 buffer attribute register
> * Used last ID for rzg2l-vsp2.
> 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/
> ---
> .../media/platform/renesas/vsp1/vsp1_drv.c | 56 ++++++++++++++-----
> .../media/platform/renesas/vsp1/vsp1_lif.c | 18 ++++--
> .../media/platform/renesas/vsp1/vsp1_regs.h | 8 +++
> 3 files changed, 62 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> index 159b68fa0829..f1f52c0c1c59 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> @@ -812,11 +812,47 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
> },
> };
>
> +static const struct vsp1_device_info rzg2l_vsp2_device_info = {
> + .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,
> +};
> +
> +static const struct vsp1_device_info *vsp1_lookup_info(struct vsp1_device *vsp1)
> +{
> + const struct vsp1_device_info *info;
> + unsigned int i;
> +
> + /*
> + * Try the info stored in match data first for devices that don't have
> + * a version register.
> + */
> + info = of_device_get_match_data(vsp1->dev);
> + if (info)
Presumably - as this will not call vsp1_read(vsp1, VI6_IP_VERSION), we
could/should always set vsp1->version here, or'ing in the _SW flag with
the derived version and SoC identifiers from the info structure.
> + return info;
> +
> + vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> +
> + for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> + info = &vsp1_device_infos[i];
> +
> + if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info->version)
> + return info;
> + }
> +
> + dev_err(vsp1->dev, "unsupported IP version 0x%08x\n", vsp1->version);
> +
> + return NULL;
> +}
> +
> static int vsp1_probe(struct platform_device *pdev)
> {
> struct vsp1_device *vsp1;
> struct device_node *fcp_node;
> - unsigned int i;
> int ret;
> int irq;
>
> @@ -872,25 +908,16 @@ static int vsp1_probe(struct platform_device *pdev)
> if (ret < 0)
> goto done;
>
> - 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;
> - }
> - }
> -
> + vsp1->info = vsp1_lookup_info(vsp1);
> if (!vsp1->info) {
> - dev_err(&pdev->dev, "unsupported IP version 0x%08x\n",
> - vsp1->version);
> vsp1_device_put(vsp1);
> ret = -ENXIO;
> goto done;
> }
>
> - dev_dbg(&pdev->dev, "IP version 0x%08x\n", vsp1->version);
> + if ((vsp1->version & VI6_IP_VERSION_VSP_MASK) != VI6_IP_VERSION_VSP)
> + vsp1->version = VI6_IP_VERSION_VSP_SW | vsp1->info->version |
> + VI6_IP_VERSION_SOC_RZG2L;
It seems odd to have this specific version assignment here. Shouldn't
that be set during vsp1_lookup_info() in the case that there is a match
from of_device_get_match_data()? That way it would be extendable by
adding just a new vsp1_device_info structure for the next platform that
has this issue. This implies that they will 'always' be RZG2L but that
information should live in the vsp1_device_info structure I think.
Could be handled when/if we get a new device added I guess, but I think
that VI6_IP_VERSION_SOC_RZG2L should be something that is retrieved from
the vsp1_device_info structure.
Re-reading the vsp1_lookup_info() function - it does seem like something
suited to there, as the vsp1->version is never read from hardware in the
new case.
>
> /*
> * Previous use of the hardware (e.g. by the bootloader) could leave
> @@ -941,6 +968,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,rzg2l-vsp2", .data = &rzg2l_vsp2_device_info },
> { },
> };
> MODULE_DEVICE_TABLE(of, vsp1_of_match);
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_lif.c b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> index 6a6857ac9327..e36ed2d2b22b 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> +++ b/drivers/media/platform/renesas/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;
> @@ -130,16 +131,21 @@ static void lif_configure_stream(struct vsp1_entity *entity,
> VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
>
> /*
> - * On R-Car V3M the LIF0 buffer attribute register has to be set to a
> - * non-default value to guarantee proper operation (otherwise artifacts
> - * may appear on the output). The value required by the manual is not
> - * explained but is likely a buffer size or threshold.
> + * On R-Car V3M and RZ/G2L the LIF0 buffer attribute register has to be
> + * set to a non-default value to guarantee proper operation (otherwise
> + * artifacts 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))
> + switch (entity->vsp1->version) {
> + case (VI6_IP_VERSION_VSP | VI6_IP_VERSION_MODEL_VSPD_V3 |
> + VI6_IP_VERSION_SOC_V3M):
> + case (VI6_IP_VERSION_VSP_SW | VI6_IP_VERSION_MODEL_VSPD_RZG2L |
> + VI6_IP_VERSION_SOC_RZG2L):
If this is going to grow - I would think it would be better served with
a feature flag - although this isn't so much of a feature, and more of a
quirk, so I wonder if that would push us closer to getting a quirks
flag.
I'm weary that this may not scale otherwise, but ... for now this works,
but I think it means we have multiple ways of handling platform specific
code already.
> vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
> VI6_LIF_LBA_LBA0 |
> (1536 << VI6_LIF_LBA_LBA1_SHIFT));
> + break;
> + }
> }
>
> static const struct vsp1_entity_operations lif_entity_ops = {
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> index fae7286eb01e..e66553c42e50 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> @@ -767,6 +767,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_V3U (0x1c << 8)
> +/* RZ/G2L SoC's have no version register, So use 0x80 as the model version */
> +#define VI6_IP_VERSION_MODEL_VSPD_RZG2L (0x80 << 8)
>
> #define VI6_IP_VERSION_SOC_MASK (0xff << 0)
> #define VI6_IP_VERSION_SOC_H2 (0x01 << 0)
> @@ -780,6 +782,12 @@
> #define VI6_IP_VERSION_SOC_M3N (0x04 << 0)
> #define VI6_IP_VERSION_SOC_E3 (0x04 << 0)
> #define VI6_IP_VERSION_SOC_V3U (0x05 << 0)
> +/* RZ/G2L SoC's have no version register, So use 0x80 for SoC Identification */
> +#define VI6_IP_VERSION_SOC_RZG2L (0x80 << 0)
> +
> +#define VI6_IP_VERSION_VSP_MASK (0xffff << 16)
> +#define VI6_IP_VERSION_VSP (0x0101 << 16) /* HW VSP version */
Is this constant on all supported platforms? both Gen2 and Gen3? (Is
there a gen1?). Does it need to be specified to the generation?
There's nothing specifically complex there or blocking I don't think -
so with comments considered as required:
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> +#define VI6_IP_VERSION_VSP_SW (0xfffe << 16) /* SW VSP version */
>
> /* -----------------------------------------------------------------------------
> * RPF CLUT Registers
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v6 3/3] media: renesas: vsp1: Add support for RZ/G2L VSPD
2022-04-14 8:39 ` Kieran Bingham
@ 2022-04-14 11:34 ` Biju Das
[not found] ` <164996310425.22830.2992726762723537347@Monstersaurus>
0 siblings, 1 reply; 8+ messages in thread
From: Biju Das @ 2022-04-14 11:34 UTC (permalink / raw)
To: Kieran Bingham, Mauro Carvalho Chehab, Philipp Zabel
Cc: Laurent Pinchart, linux-media, linux-renesas-soc,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
Hi Kieran,
Thanks for the feedback.
> Subject: Re: [PATCH v6 3/3] media: renesas: vsp1: Add support for RZ/G2L
> VSPD
>
> Quoting Biju Das (2022-03-16 11:55:51)
> > 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,rzg2l-vsp2" with a data pointer
> > containing the info structure. Also the reset line is shared with the DU
> module.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > v5->v6:
> > * Rebased to media_staging and updated commit header
> > * Removed the extra tab from rzg2l_vsp2_device_info
> > * Changed the function vsp1_lookup->vsp1_lookup_info and
> > all info match related code moved here.
> > * Add VI6_IP_VERSION_VSP and VI6_IP_VERSION_VSP_SW macros to
> > distinguish HW & SW IP_VSP_Version.
> > * Used 0x80 for RZG2L VSPD model and SoC identification
> > * Updated Switch() for LIF0 buffer attribute handling.
> > v4->v5:
> > * Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
> > * To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
> > for RZ/G2L SoC's.
> > v3->v4:
> > * Added Rb tag from Geert
> > * Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M
> > v2->v3:
> > * Fixed version comparison in vsp1_lookup()
> > v1->v2:
> > * Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
> > * Added standalone device info for rzg2l-vsp2.
> > * Added vsp1_lookup helper function.
> > * Updated comments for LIF0 buffer attribute register
> > * Used last ID for rzg2l-vsp2.
> > RFC->v1:
> > * Used data pointer containing info structure to retrieve version
> > information
> > RFC:
> > ---
> > .../media/platform/renesas/vsp1/vsp1_drv.c | 56 ++++++++++++++-----
> > .../media/platform/renesas/vsp1/vsp1_lif.c | 18 ++++--
> > .../media/platform/renesas/vsp1/vsp1_regs.h | 8 +++
> > 3 files changed, 62 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > index 159b68fa0829..f1f52c0c1c59 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > @@ -812,11 +812,47 @@ static const struct vsp1_device_info
> vsp1_device_infos[] = {
> > },
> > };
> >
> > +static const struct vsp1_device_info rzg2l_vsp2_device_info = {
> > + .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,
> > +};
> > +
> > +static const struct vsp1_device_info *vsp1_lookup_info(struct
> > +vsp1_device *vsp1) {
> > + const struct vsp1_device_info *info;
> > + unsigned int i;
> > +
> > + /*
> > + * Try the info stored in match data first for devices that
> don't have
> > + * a version register.
> > + */
> > + info = of_device_get_match_data(vsp1->dev);
> > + if (info)
>
> Presumably - as this will not call vsp1_read(vsp1, VI6_IP_VERSION), we
> could/should always set vsp1->version here, or'ing in the _SW flag with
> the derived version and SoC identifiers from the info structure.
OK, I have prototyped as per your suggestion, and it looks good.
Here it is
if (info) {
vsp1->quirks = LIF_BUF_ATTR_QUIRKS;
vsp1->version = VI6_IP_VERSION_VSP_SW | info->version |
VI6_IP_VERSION_SOC_RZG2L;
>
> > + return info;
> > +
> > + vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > +
> > + for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> > + info = &vsp1_device_infos[i];
> > +
> > + if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info-
> >version)
Here it is
if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info->version) {
if ((vsp1->version & VI6_IP_VERSION_MASK) ==
(VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
vsp1->quirks = LIF_BUF_ATTR_QUIRKS;
> > + return info;
> > + }
> > +
> > + dev_err(vsp1->dev, "unsupported IP version 0x%08x\n",
> > + vsp1->version);
> > +
> > + return NULL;
> > +}
> > +
> > static int vsp1_probe(struct platform_device *pdev) {
> > struct vsp1_device *vsp1;
> > struct device_node *fcp_node;
> > - unsigned int i;
> > int ret;
> > int irq;
> >
> > @@ -872,25 +908,16 @@ static int vsp1_probe(struct platform_device
> *pdev)
> > if (ret < 0)
> > goto done;
> >
> > - 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;
> > - }
> > - }
> > -
> > + vsp1->info = vsp1_lookup_info(vsp1);
> > if (!vsp1->info) {
> > - dev_err(&pdev->dev, "unsupported IP version 0x%08x\n",
> > - vsp1->version);
> > vsp1_device_put(vsp1);
> > ret = -ENXIO;
> > goto done;
> > }
> >
> > - dev_dbg(&pdev->dev, "IP version 0x%08x\n", vsp1->version);
> > + if ((vsp1->version & VI6_IP_VERSION_VSP_MASK) !=
> VI6_IP_VERSION_VSP)
> > + vsp1->version = VI6_IP_VERSION_VSP_SW | vsp1->info-
> >version |
> > + VI6_IP_VERSION_SOC_RZG2L;
>
> It seems odd to have this specific version assignment here. Shouldn't that
> be set during vsp1_lookup_info() in the case that there is a match from
> of_device_get_match_data()? That way it would be extendable by adding just
> a new vsp1_device_info structure for the next platform that has this
> issue. This implies that they will 'always' be RZG2L but that information
> should live in the vsp1_device_info structure I think.
>
We can remove this assignment from here and move vsp1_lookup_info.
> Could be handled when/if we get a new device added I guess, but I think
> that VI6_IP_VERSION_SOC_RZG2L should be something that is retrieved from
> the vsp1_device_info structure.
>
> Re-reading the vsp1_lookup_info() function - it does seem like something
> suited to there, as the vsp1->version is never read from hardware in the
> new case.
Ok, Agreed.
>
> >
> > /*
> > * Previous use of the hardware (e.g. by the bootloader) could
> > leave @@ -941,6 +968,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,rzg2l-vsp2", .data =
> > + &rzg2l_vsp2_device_info },
> > { },
> > };
> > MODULE_DEVICE_TABLE(of, vsp1_of_match); diff --git
> > a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > index 6a6857ac9327..e36ed2d2b22b 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > +++ b/drivers/media/platform/renesas/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;
> > @@ -130,16 +131,21 @@ static void lif_configure_stream(struct
> vsp1_entity *entity,
> > VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
> >
> > /*
> > - * On R-Car V3M the LIF0 buffer attribute register has to be set
> to a
> > - * non-default value to guarantee proper operation (otherwise
> artifacts
> > - * may appear on the output). The value required by the manual
> is not
> > - * explained but is likely a buffer size or threshold.
> > + * On R-Car V3M and RZ/G2L the LIF0 buffer attribute register
> has to be
> > + * set to a non-default value to guarantee proper operation
> (otherwise
> > + * artifacts 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))
> > + switch (entity->vsp1->version) {
> > + case (VI6_IP_VERSION_VSP | VI6_IP_VERSION_MODEL_VSPD_V3 |
> > + VI6_IP_VERSION_SOC_V3M):
> > + case (VI6_IP_VERSION_VSP_SW | VI6_IP_VERSION_MODEL_VSPD_RZG2L |
> > + VI6_IP_VERSION_SOC_RZG2L):
>
> If this is going to grow - I would think it would be better served with a
> feature flag - although this isn't so much of a feature, and more of a
> quirk, so I wonder if that would push us closer to getting a quirks flag.
>
> I'm weary that this may not scale otherwise, but ... for now this works,
> but I think it means we have multiple ways of handling platform specific
> code already.
Here it is
if (lif->quirks)
vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
VI6_LIF_LBA_LBA0 |
(1536 << VI6_LIF_LBA_LBA1_SHIFT));
And the below change in vsp1_lif_create
lif->quirks = vsp1->quirks & LIF_BUF_ATTR_QUIRKS;
>
>
> > vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
> > VI6_LIF_LBA_LBA0 |
> > (1536 << VI6_LIF_LBA_LBA1_SHIFT));
> > + break;
> > + }
> > }
> >
> > static const struct vsp1_entity_operations lif_entity_ops = { diff
> > --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > index fae7286eb01e..e66553c42e50 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > @@ -767,6 +767,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_V3U (0x1c << 8)
> > +/* RZ/G2L SoC's have no version register, So use 0x80 as the model
> version */
> > +#define VI6_IP_VERSION_MODEL_VSPD_RZG2L (0x80 << 8)
> >
> > #define VI6_IP_VERSION_SOC_MASK (0xff << 0)
> > #define VI6_IP_VERSION_SOC_H2 (0x01 << 0)
> > @@ -780,6 +782,12 @@
> > #define VI6_IP_VERSION_SOC_M3N (0x04 << 0)
> > #define VI6_IP_VERSION_SOC_E3 (0x04 << 0)
> > #define VI6_IP_VERSION_SOC_V3U (0x05 << 0)
> > +/* RZ/G2L SoC's have no version register, So use 0x80 for SoC
> Identification */
> > +#define VI6_IP_VERSION_SOC_RZG2L (0x80 << 0)
> > +
> > +#define VI6_IP_VERSION_VSP_MASK (0xffff << 16)
> > +#define VI6_IP_VERSION_VSP (0x0101 << 16) /* HW VSP version
> */
>
> Is this constant on all supported platforms? both Gen2 and Gen3? (Is there
> a gen1?). Does it need to be specified to the generation?
I have checked Gen1 and Gen2 HW manual I don't find this info. So I would like
to remove this macro as it is unused after quirk changes.
I am planning to send V7 with these changes, please let me know if you have any feedback.
Cheers,
Biju
>
> There's nothing specifically complex there or blocking I don't think - so
> with comments considered as required:
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> > +#define VI6_IP_VERSION_VSP_SW (0xfffe << 16) /* SW VSP version
> */
> >
> > /* --------------------------------------------------------------------
> ---------
> > * RPF CLUT Registers
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6 3/3] media: renesas: vsp1: Add support for RZ/G2L VSPD
[not found] ` <165037712258.2548121.489298521958356607@Monstersaurus>
@ 2022-04-19 15:55 ` Laurent Pinchart
0 siblings, 0 replies; 8+ messages in thread
From: Laurent Pinchart @ 2022-04-19 15:55 UTC (permalink / raw)
To: Kieran Bingham
Cc: Biju Das, Mauro Carvalho Chehab, Philipp Zabel, linux-media@,
linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
On Tue, Apr 19, 2022 at 03:05:22PM +0100, Kieran Bingham wrote:
> Quoting Biju Das (2022-04-14 22:37:37)
> > > Subject: RE: [PATCH v6 3/3] media: renesas: vsp1: Add support for RZ/G2L
> > > VSPD
> > >
> > > Quoting Biju Das (2022-04-14 12:34:53)
> > > > > Subject: Re: [PATCH v6 3/3] media: renesas: vsp1: Add support for
> > > > > RZ/G2L VSPD
> > > > >
> > > > > Quoting Biju Das (2022-03-16 11:55:51)
> > > > > > 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,rzg2l-vsp2" with a data
> > > > > > pointer containing the info structure. Also the reset line is
> > > > > > shared with the DU module.
> > > > > >
> > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > > ---
> > > > > > v5->v6:
> > > > > > * Rebased to media_staging and updated commit header
> > > > > > * Removed the extra tab from rzg2l_vsp2_device_info
> > > > > > * Changed the function vsp1_lookup->vsp1_lookup_info and
> > > > > > all info match related code moved here.
> > > > > > * Add VI6_IP_VERSION_VSP and VI6_IP_VERSION_VSP_SW macros to
> > > > > > distinguish HW & SW IP_VSP_Version.
> > > > > > * Used 0x80 for RZG2L VSPD model and SoC identification
> > > > > > * Updated Switch() for LIF0 buffer attribute handling.
> > > > > > v4->v5:
> > > > > > * Fixed typo VI6_IP_VERSION_MODEL_MASK->VI6_IP_VERSION_MASK
> > > > > > * To be consistent with other SoC's, introduced VI6_IP_VERSION_SOC_G2L
> > > > > > for RZ/G2L SoC's.
> > > > > > v3->v4:
> > > > > > * Added Rb tag from Geert
> > > > > > * Add switch() for LIF0 buffer attribute handling for RZ/G2L and V3M
> > > > > > v2->v3:
> > > > > > * Fixed version comparison in vsp1_lookup()
> > > > > > v1->v2:
> > > > > > * Changed the compatible from vsp2-rzg2l->rzg2l-vsp2
> > > > > > * Added standalone device info for rzg2l-vsp2.
> > > > > > * Added vsp1_lookup helper function.
> > > > > > * Updated comments for LIF0 buffer attribute register
> > > > > > * Used last ID for rzg2l-vsp2.
> > > > > > RFC->v1:
> > > > > > * Used data pointer containing info structure to retrieve version information
> > > > > > RFC:
> > > > > > ---
> > > > > > .../media/platform/renesas/vsp1/vsp1_drv.c | 56 ++++++++++++++--
> > > ---
> > > > > > .../media/platform/renesas/vsp1/vsp1_lif.c | 18 ++++--
> > > > > > .../media/platform/renesas/vsp1/vsp1_regs.h | 8 +++
> > > > > > 3 files changed, 62 insertions(+), 20 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > > > > > b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > > > > > index 159b68fa0829..f1f52c0c1c59 100644
> > > > > > --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > > > > > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> > > > > > @@ -812,11 +812,47 @@ static const struct vsp1_device_info
> > > > > vsp1_device_infos[] = {
> > > > > > },
> > > > > > };
> > > > > >
> > > > > > +static const struct vsp1_device_info rzg2l_vsp2_device_info = {
> > > > > > + .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,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct vsp1_device_info *vsp1_lookup_info(struct
> > > > > > +vsp1_device *vsp1) {
> > > > > > + const struct vsp1_device_info *info;
> > > > > > + unsigned int i;
> > > > > > +
> > > > > > + /*
> > > > > > + * Try the info stored in match data first for devices that don't have
> > > > > > + * a version register.
> > > > > > + */
> > > > > > + info = of_device_get_match_data(vsp1->dev);
> > > > > > + if (info)
> > > > >
> > > > > Presumably - as this will not call vsp1_read(vsp1, VI6_IP_VERSION),
> > > > > we could/should always set vsp1->version here, or'ing in the _SW
> > > > > flag with the derived version and SoC identifiers from the info
> > > > > structure.
> > > >
> > > > OK, I have prototyped as per your suggestion, and it looks good.
> > > >
> > > > Here it is
> > > >
> > > > if (info) {
> > > > vsp1->quirks = LIF_BUF_ATTR_QUIRKS;
> > >
> > > No - quirks are device specific - they (/it) shouldn't be here. I think
> > > it should be something that is set in the vsp1_device_info like the
> > > features flags.
> >
> > If we want to add it in vsp1_sevice_info, then the list grows as
> > Currently both V3M and V3H SoC's have single info structure and the info->version
> > for both V3M and V3H is "VI6_IP_VERSION_MODEL_VSPD_V3"
> >
> > OK, I will expand this to separate info structure for V3M and V3H.
>
> I think that's reasonable. I see some more discussion with Laurent in v7
> already - so perhaps best to re-review on a v8 anyway.
>
> > > Then a helper would come with it just like the vsp1_feature()
> > >
> > >
> > > #define vsp1_quirk(vsp1, f) ((vsp1)->info->quirks & (f))
> > >
> > > so that code reliant upon a device specific quirk would read:
> > >
> > > if (vsp1_quirk(vsp1, VSP1_QUIRK_LIF_BUF_ATTR)
> > > .... do thing ...
> > >
> > > But it should still keep the comments explaining why the quirk is there
> > > and what it's doing of course.
> >
> > OK.
> >
> > > > vsp1->version = VI6_IP_VERSION_VSP_SW | info->version |
> > > > VI6_IP_VERSION_SOC_RZG2L;
> > >
> > > I think vsp1_device_info should be extended with a .soc to set
> > > VI6_IP_VERSION_SOC_RZG2L in rzg2l_vsp2_device_info, because (planning
> > > ahead to when you add a second or third of these) the SOC could be
> > > different.
> >
> > The IP is identical for all RZ/G2L alike SoC's (for eg:- RZ/V2L, RZ/G2{L,LC} and RZ/G2UL)
> > The same generic VSPD quirk change works on all these SoC's. For me, it is just unnecessary wastage
> > of memory for adding separate vsp1_device_info for all RZ/G2L alike SoC's.
>
> But what happens for RZG3L?
>
> To me - adding a new platform support should be adding a new
> vsp1_device_info either in vsp1_device_infos, or directly matched with a
> compatible string as you have implemented here.
>
> But this means /all/ devices added by compatible string are
> VI6_IP_VERSION_SOC_RZG2L which likely isn't true in the future.
>
> Put differently, Having a vsp1_device_infos passed by matching the
> compatible string does not mean the device is an RZG2L, which is what
> this code is defining.
>
> > On V3M case we need to separate as V3H doesn't need quirk where as V3M it need,
> > here it is not the case all SoC's need quirks.
> >
> > If it all need to be extended, we can start with 0x80 for VI6_IP_VERSION_SOC_RZG2L, 0x81 for
> > VI6_IP_VERSION_SOC_RZV2L etc.. I don't think it is required unless I am missing something.
> >
> > Please let me know your thoughts on this.
> >
> > > > > > + return info;
> > > > > > +
> > > > > > + vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > > > > > +
> > > > > > + for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> > > > > > + info = &vsp1_device_infos[i];
> > > > > > +
> > > > > > + if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info->version)
> > > >
> > > > Here it is
> > > > if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) == info->version) {
> > > > if ((vsp1->version & VI6_IP_VERSION_MASK) ==
> > > > (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M))
> > > > vsp1->quirks = LIF_BUF_ATTR_QUIRKS;
> > > >
> > >
> > > As above - I think that should be stored per vsp1_device_info.
> >
> > Ok, will add separate info for V3M and V3H.
>
> i think that's best, I hope Laurent agrees, I'm not sure he's seen the
> comments on this thread as he replied to v7.
That's fine with me, but I'm curious to see how it will look like, as
the device info structure is currently matched based on the model only,
not the SoC. I don't want to duplicate all instances of device info
per-SoC when they don't differ.
> > > > > > + return info;
> > > > > > + }
> > > > > > +
> > > > > > + dev_err(vsp1->dev, "unsupported IP version 0x%08x\n",
> > > > > > + vsp1->version);
> > > > > > +
> > > > > > + return NULL;
> > > > > > +}
> > > > > > +
> > > > > > static int vsp1_probe(struct platform_device *pdev) {
> > > > > > struct vsp1_device *vsp1;
> > > > > > struct device_node *fcp_node;
> > > > > > - unsigned int i;
> > > > > > int ret;
> > > > > > int irq;
> > > > > >
> > > > > > @@ -872,25 +908,16 @@ static int vsp1_probe(struct platform_device*pdev)
> > > > > > if (ret < 0)
> > > > > > goto done;
> > > > > >
> > > > > > - 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;
> > > > > > - }
> > > > > > - }
> > > > > > -
> > > > > > + vsp1->info = vsp1_lookup_info(vsp1);
> > > > > > if (!vsp1->info) {
> > > > > > - dev_err(&pdev->dev, "unsupported IP version 0x%08x\n",
> > > > > > - vsp1->version);
> > > > > > vsp1_device_put(vsp1);
> > > > > > ret = -ENXIO;
> > > > > > goto done;
> > > > > > }
> > > > > >
> > > > > > - dev_dbg(&pdev->dev, "IP version 0x%08x\n", vsp1->version);
> > > > > > + if ((vsp1->version & VI6_IP_VERSION_VSP_MASK) != VI6_IP_VERSION_VSP)
> > > > > > + vsp1->version = VI6_IP_VERSION_VSP_SW | vsp1->info->version |
> > > > > > + VI6_IP_VERSION_SOC_RZG2L;
> > > > >
> > > > > It seems odd to have this specific version assignment here.
> > > > > Shouldn't that be set during vsp1_lookup_info() in the case that
> > > > > there is a match from of_device_get_match_data()? That way it would
> > > > > be extendable by adding just a new vsp1_device_info structure for
> > > > > the next platform that has this issue. This implies that they will
> > > > > 'always' be RZG2L but that information should live in the
> > > vsp1_device_info structure I think.
> > > > >
> > > >
> > > > We can remove this assignment from here and move vsp1_lookup_info.
> > > >
> > > > > Could be handled when/if we get a new device added I guess, but I
> > > > > think that VI6_IP_VERSION_SOC_RZG2L should be something that is
> > > > > retrieved from the vsp1_device_info structure.
> > > > >
> > > > > Re-reading the vsp1_lookup_info() function - it does seem like
> > > > > something suited to there, as the vsp1->version is never read from
> > > > > hardware in the new case.
> > > >
> > > > Ok, Agreed.
> > > >
> > > > > >
> > > > > > /*
> > > > > > * Previous use of the hardware (e.g. by the bootloader) could leave
> > > > > > @@ -941,6 +968,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,rzg2l-vsp2", .data =
> > > > > > + &rzg2l_vsp2_device_info },
> > > > > > { },
> > > > > > };
> > > > > > MODULE_DEVICE_TABLE(of, vsp1_of_match); diff= --git
> > > > > > a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > > > > > b/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > > > > > index 6a6857ac9327..e36ed2d2b22b 100644
> > > > > > --- a/drivers/media/platform/renesas/vsp1/vsp1_lif.c
> > > > > > +++ b/drivers/media/platform/renesas/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;
> > > > > > @@ -130,16 +131,21 @@ static void lif_configure_stream(struct vsp1_entity *entity,
> > > > > > VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN);
> > > > > >
> > > > > > /*
> > > > > > - * On R-Car V3M the LIF0 buffer attribute register has to be set to a
> > > > > > - * non-default value to guarantee proper operation (otherwise artifacts
> > > > > > - * may appear on the output). The value required by the manual is not
> > > > > > - * explained but is likely a buffer size or threshold.
> > > > > > + * On R-Car V3M and RZ/G2L the LIF0 buffer attribute register has to be
> > > > > > + * set to a non-default value to guarantee proper operation (otherwise
> > > > > > + * artifacts 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))
> > > > > > + switch (entity->vsp1->version) {
> > > > > > + case (VI6_IP_VERSION_VSP | VI6_IP_VERSION_MODEL_VSPD_V3 |
> > > > > > + VI6_IP_VERSION_SOC_V3M):
> > > > > > + case (VI6_IP_VERSION_VSP_SW | VI6_IP_VERSION_MODEL_VSPD_RZG2L |
> > > > > > + VI6_IP_VERSION_SOC_RZG2L):
> > > > >
> > > > > If this is going to grow - I would think it would be better served
> > > > > with a feature flag - although this isn't so much of a feature, and
> > > > > more of a quirk, so I wonder if that would push us closer to getting a
> > > > > quirks flag.
> > > > >
> > > > > I'm weary that this may not scale otherwise, but ... for now this
> > > > > works, but I think it means we have multiple ways of handling
> > > > > platform specific code already.
> > > >
> > > > Here it is
> > > >
> > > > if (lif->quirks)
> > > > vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
> > > > VI6_LIF_LBA_LBA0 |
> > > > (1536 << VI6_LIF_LBA_LBA1_SHIFT));
> > >
> > > I think
> > >
> > > if (vsp1_quirk(vsp1, VSP1_QUIRK_LIF_BUF_ATTR)
> > > vsp1_lif_write(lif, dlb, VI6_LIF_LBA, |
> > > (1536 << VI6_LIF_LBA_LBA1_SHIFT));
> > >
> > > and keeping the block comment above would be better. There's a risk of the
> > > platfoms defining the quirk, and the comment describing the platforms
> > > affected getting out of sync but I think that's going to be cleaner than
> > > extending this with a growing set of unreadable model and soc masks.
> > >
> > >
> > > > And the below change in vsp1_lif_create
> > > >
> > > > lif->quirks = vsp1->quirks & LIF_BUF_ATTR_QUIRKS;
> > >
> > > I don't think the lif needs a quirk flag storage. It can come from the
> > > vsp1->info (Through a helper)/
> >
> > OK. Agreed.
> >
> > > > > > vsp1_lif_write(lif, dlb, VI6_LIF_LBA,
> > > > > > VI6_LIF_LBA_LBA0 |
> > > > > > (1536 << VI6_LIF_LBA_LBA1_SHIFT));
> > > > > > + break;
> > > > > > + }
> > > > > > }
> > > > > >
> > > > > > static const struct vsp1_entity_operations lif_entity_ops = {
> > > > > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > > > > > b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > > > > > index fae7286eb01e..e66553c42e50 100644
> > > > > > --- a/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > > > > > +++ b/drivers/media/platform/renesas/vsp1/vsp1_regs.h
> > > > > > @@ -767,6 +767,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_V3U (0x1c << 8)
> > > > > > +/* RZ/G2L SoC's have no version register, So use 0x80 as the model version */
> > > > > > +#define VI6_IP_VERSION_MODEL_VSPD_RZG2L (0x80 << 8)
> > > > > >
> > > > > > #define VI6_IP_VERSION_SOC_MASK (0xff << 0)
> > > > > > #define VI6_IP_VERSION_SOC_H2 (0x01 << 0)
> > > > > > @@ -780,6 +782,12 @@
> > > > > > #define VI6_IP_VERSION_SOC_M3N (0x04 << 0)
> > > > > > #define VI6_IP_VERSION_SOC_E3 (0x04 << 0)
> > > > > > #define VI6_IP_VERSION_SOC_V3U (0x05 << 0)
> > > > > > +/* RZ/G2L SoC's have no version register, So use 0x80 for SoC Identification */
> > > > > > +#define VI6_IP_VERSION_SOC_RZG2L (0x80 << 0)
> > > > > > +
> > > > > > +#define VI6_IP_VERSION_VSP_MASK (0xffff << 16)
> > > > > > +#define VI6_IP_VERSION_VSP (0x0101 << 16) /* HW VSP version */
> > > > >
> > > > > Is this constant on all supported platforms? both Gen2 and Gen3? (Is
> > > > > there a gen1?). Does it need to be specified to the generation?
> > > >
> > > > I have checked Gen1 and Gen2 HW manual I don't find this info. So I
> > > > would like to remove this macro as it is unused after quirk changes.
> > > >
> > >
> > > If it's unused, then yes - I think it can be removed.
> > >
> > > > I am planning to send V7 with these changes, please let me know if you
> > > > have any feedback.
> > >
> > > I think I'm too late ... but comments above.
> > >
> > > > > There's nothing specifically complex there or blocking I don't think
> > > > > - so with comments considered as required:
> > > > >
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > > >
> > > > > > +#define VI6_IP_VERSION_VSP_SW (0xfffe << 16) /* SW VSP version
> > > > > */
> > > > > >
> > > > > > /* -----------------------------------------------------------------------------
> > > > > > * RPF CLUT Registers
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-04-19 15:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-16 11:55 [PATCH v6 0/3] Add support for RZ/G2L VSPD Biju Das
2022-03-16 11:55 ` [PATCH v6 1/3] media: dt-bindings: media: renesas,vsp1: Document RZ/{G2L,V2L} VSPD bindings Biju Das
2022-03-16 11:55 ` [PATCH v6 2/3] media: renesas: vsp1: Add support to deassert/assert reset line Biju Das
2022-03-16 11:55 ` [PATCH v6 3/3] media: renesas: vsp1: Add support for RZ/G2L VSPD Biju Das
2022-04-14 8:39 ` Kieran Bingham
2022-04-14 11:34 ` Biju Das
[not found] ` <164996310425.22830.2992726762723537347@Monstersaurus>
[not found] ` <OS0PR01MB59226E537BB85C234DC87CBD86EF9@OS0PR01MB5922.jpnprd01.prod.outlook.com>
[not found] ` <165037712258.2548121.489298521958356607@Monstersaurus>
2022-04-19 15:55 ` Laurent Pinchart
2022-04-14 7:15 ` [PATCH v6 0/3] " Biju Das
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).