Hi Tommaso,
> -----Original Message-----
> From: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>
> Sent: 03 March 2025 16:08
> Subject: [PATCH v4 11/17] media: rzg2l-cru: Add register mapping support
>
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Prepare for adding support for RZ/G3E and RZ/V2HP SoCs, which have a CRU-IP that is mostly identical
> to RZ/G2L but with different register offsets and additional registers. Introduce a flexible register
> mapping mechanism to handle these variations.
>
> Define the `rzg2l_cru_info` structure to store register mappings and pass it as part of the OF match
> data. Update the read/write functions to check out-of-bound accesses and use indexed register offsets
> from `rzg2l_cru_info`, ensuring compatibility across different SoC variants.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>
> ---
> Changes since v2:
> - Implemented new rzg2l_cru_write/read() that now are checking out-of-bound
> accesses as suggested by LPinchart.
> - Fixed AMnMBxADDRL() and AMnMBxADDRH() as suggested by LPinchart.
> - Update commit body
>
> .../platform/renesas/rzg2l-cru/rzg2l-core.c | 46 ++++++++++++-
> .../renesas/rzg2l-cru/rzg2l-cru-regs.h | 66 ++++++++++---------
> .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 4 ++
> .../platform/renesas/rzg2l-cru/rzg2l-video.c | 58 ++++++++++++++--
> 4 files changed, 139 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> index eed9d2bd0841..abc2a979833a 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> @@ -22,6 +22,7 @@
> #include <media/v4l2-mc.h>
>
> #include "rzg2l-cru.h"
> +#include "rzg2l-cru-regs.h"
>
> static inline struct rzg2l_cru_dev *notifier_to_cru(struct v4l2_async_notifier *n) { @@ -269,6
> +270,9 @@ static int rzg2l_cru_probe(struct platform_device *pdev)
>
> cru->dev = dev;
> cru->info = of_device_get_match_data(dev);
> + if (!cru->info)
> + return dev_err_probe(dev, -EINVAL,
> + "Failed to get OF match data\n");
>
> irq = platform_get_irq(pdev, 0);
> if (irq < 0)
> @@ -317,8 +321,48 @@ static void rzg2l_cru_remove(struct platform_device *pdev)
> rzg2l_cru_dma_unregister(cru);
> }
>
> +static const u16 rzg2l_cru_regs[] = {
> + [CRUnCTRL] = 0x0,
> + [CRUnIE] = 0x4,
> + [CRUnINTS] = 0x8,
> + [CRUnRST] = 0xc,
> + [AMnMB1ADDRL] = 0x100,
> + [AMnMB1ADDRH] = 0x104,
> + [AMnMB2ADDRL] = 0x108,
> + [AMnMB2ADDRH] = 0x10c,
> + [AMnMB3ADDRL] = 0x110,
> + [AMnMB3ADDRH] = 0x114,
> + [AMnMB4ADDRL] = 0x118,
> + [AMnMB4ADDRH] = 0x11c,
> + [AMnMB5ADDRL] = 0x120,
> + [AMnMB5ADDRH] = 0x124,
> + [AMnMB6ADDRL] = 0x128,
> + [AMnMB6ADDRH] = 0x12c,
> + [AMnMB7ADDRL] = 0x130,
> + [AMnMB7ADDRH] = 0x134,
> + [AMnMB8ADDRL] = 0x138,
> + [AMnMB8ADDRH] = 0x13c,
> + [AMnMBVALID] = 0x148,
> + [AMnMBS] = 0x14c,
> + [AMnAXIATTR] = 0x158,
> + [AMnFIFOPNTR] = 0x168,
> + [AMnAXISTP] = 0x174,
> + [AMnAXISTPACK] = 0x178,
> + [ICnEN] = 0x200,
> + [ICnMC] = 0x208,
> + [ICnMS] = 0x254,
> + [ICnDMR] = 0x26c,
> +};
> +
> +static const struct rzg2l_cru_info rzgl2_cru_info = {
> + .regs = rzg2l_cru_regs,
> +};
> +
> static const struct of_device_id rzg2l_cru_of_id_table[] = {
> - { .compatible = "renesas,rzg2l-cru", },
> + {
> + .compatible = "renesas,rzg2l-cru",
> + .data = &rzgl2_cru_info,
> + },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, rzg2l_cru_of_id_table); diff --git a/drivers/media/platform/renesas/rzg2l-
> cru/rzg2l-cru-regs.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
> index 1c9f22118a5d..86c320286246 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
> @@ -10,71 +10,77 @@
>
> /* HW CRU Registers Definition */
>
> -/* CRU Control Register */
> -#define CRUnCTRL 0x0
> #define CRUnCTRL_VINSEL(x) ((x) << 0)
>
> -/* CRU Interrupt Enable Register */
> -#define CRUnIE 0x4
> #define CRUnIE_EFE BIT(17)
>
> -/* CRU Interrupt Status Register */
> -#define CRUnINTS 0x8
> #define CRUnINTS_SFS BIT(16)
>
> -/* CRU Reset Register */
> -#define CRUnRST 0xc
> #define CRUnRST_VRESETN BIT(0)
>
> /* Memory Bank Base Address (Lower) Register for CRU Image Data */
> -#define AMnMBxADDRL(x) (0x100 + ((x) * 8))
> +#define AMnMBxADDRL(x) (AMnMB1ADDRL + (x) * 2)
>
> /* Memory Bank Base Address (Higher) Register for CRU Image Data */
> -#define AMnMBxADDRH(x) (0x104 + ((x) * 8))
> +#define AMnMBxADDRH(x) (AMnMB1ADDRH + (x) * 2)
>
> -/* Memory Bank Enable Register for CRU Image Data */
> -#define AMnMBVALID 0x148
> #define AMnMBVALID_MBVALID(x) GENMASK(x, 0)
>
> -/* Memory Bank Status Register for CRU Image Data */
> -#define AMnMBS 0x14c
> #define AMnMBS_MBSTS 0x7
>
> -/* AXI Master Transfer Setting Register for CRU Image Data */
> -#define AMnAXIATTR 0x158
> #define AMnAXIATTR_AXILEN_MASK GENMASK(3, 0)
> #define AMnAXIATTR_AXILEN (0xf)
>
> -/* AXI Master FIFO Pointer Register for CRU Image Data */
> -#define AMnFIFOPNTR 0x168
> #define AMnFIFOPNTR_FIFOWPNTR GENMASK(7, 0)
> #define AMnFIFOPNTR_FIFORPNTR_Y GENMASK(23, 16)
>
> -/* AXI Master Transfer Stop Register for CRU Image Data */
> -#define AMnAXISTP 0x174
> #define AMnAXISTP_AXI_STOP BIT(0)
>
> -/* AXI Master Transfer Stop Status Register for CRU Image Data */
> -#define AMnAXISTPACK 0x178
> #define AMnAXISTPACK_AXI_STOP_ACK BIT(0)
>
> -/* CRU Image Processing Enable Register */
> -#define ICnEN 0x200
> #define ICnEN_ICEN BIT(0)
>
> -/* CRU Image Processing Main Control Register */
> -#define ICnMC 0x208
> #define ICnMC_CSCTHR BIT(5)
> #define ICnMC_INF(x) ((x) << 16)
> #define ICnMC_VCSEL(x) ((x) << 22)
> #define ICnMC_INF_MASK GENMASK(21, 16)
>
> -/* CRU Module Status Register */
> -#define ICnMS 0x254
> #define ICnMS_IA BIT(2)
>
> -/* CRU Data Output Mode Register */
> -#define ICnDMR 0x26c
> #define ICnDMR_YCMODE_UYVY (1 << 4)
>
> +enum rzg2l_cru_common_regs {
> + CRUnCTRL, /* CRU Control */
> + CRUnIE, /* CRU Interrupt Enable */
> + CRUnINTS, /* CRU Interrupt Status */
> + CRUnRST, /* CRU Reset */
> + AMnMB1ADDRL, /* Bank 1 Address (Lower) for CRU Image Data */
> + AMnMB1ADDRH, /* Bank 1 Address (Higher) for CRU Image Data */
> + AMnMB2ADDRL, /* Bank 2 Address (Lower) for CRU Image Data */
> + AMnMB2ADDRH, /* Bank 2 Address (Higher) for CRU Image Data */
> + AMnMB3ADDRL, /* Bank 3 Address (Lower) for CRU Image Data */
> + AMnMB3ADDRH, /* Bank 3 Address (Higher) for CRU Image Data */
> + AMnMB4ADDRL, /* Bank 4 Address (Lower) for CRU Image Data */
> + AMnMB4ADDRH, /* Bank 4 Address (Higher) for CRU Image Data */
> + AMnMB5ADDRL, /* Bank 5 Address (Lower) for CRU Image Data */
> + AMnMB5ADDRH, /* Bank 5 Address (Higher) for CRU Image Data */
> + AMnMB6ADDRL, /* Bank 6 Address (Lower) for CRU Image Data */
> + AMnMB6ADDRH, /* Bank 6 Address (Higher) for CRU Image Data */
> + AMnMB7ADDRL, /* Bank 7 Address (Lower) for CRU Image Data */
> + AMnMB7ADDRH, /* Bank 7 Address (Higher) for CRU Image Data */
> + AMnMB8ADDRL, /* Bank 8 Address (Lower) for CRU Image Data */
> + AMnMB8ADDRH, /* Bank 8 Address (Higher) for CRU Image Data */
> + AMnMBVALID, /* Memory Bank Enable for CRU Image Data */
> + AMnMBS, /* Memory Bank Status for CRU Image Data */
> + AMnAXIATTR, /* AXI Master Transfer Setting Register for CRU Image Data */
> + AMnFIFOPNTR, /* AXI Master FIFO Pointer for CRU Image Data */
> + AMnAXISTP, /* AXI Master Transfer Stop for CRU Image Data */
> + AMnAXISTPACK, /* AXI Master Transfer Stop Status for CRU Image Data */
> + ICnEN, /* CRU Image Processing Enable */
> + ICnMC, /* CRU Image Processing Main Control */
> + ICnMS, /* CRU Module Status */
> + ICnDMR, /* CRU Data Output Mode */
> + RZG2L_CRU_MAX_REG,
> +};
> +
> #endif /* __RZG2L_CRU_REGS_H__ */
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> index 8b898ce05b84..00c3f7458e20 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> @@ -80,6 +80,10 @@ struct rzg2l_cru_ip_format {
> bool yuv;
> };
>
> +struct rzg2l_cru_info {
> + const u16 *regs;
> +};
> +
> /**
> * struct rzg2l_cru_dev - Renesas CRU device structure
> * @dev: (OF) device
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index cd69c8a686d3..792f0df51a4b 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -42,16 +42,66 @@ struct rzg2l_cru_buffer {
> /* -----------------------------------------------------------------------------
> * DMA operations
> */
> -static void rzg2l_cru_write(struct rzg2l_cru_dev *cru, u32 offset, u32 value)
> +static void __rzg2l_cru_write(struct rzg2l_cru_dev *cru, u32 offset,
> +u32 value)
> {
> - iowrite32(value, cru->base + offset);
> + const u16 *regs = cru->info->regs;
> +
> + /*
> + * CRUnCTRL is a first register on all CRU supported SoCs so validate
> + * rest of the registers have valid offset being set in cru->info->regs.
> + */
> + if (WARN_ON(offset >= RZG2L_CRU_MAX_REG) ||
> + WARN_ON(offset != CRUnCTRL && regs[offset] == 0))
> + return;
> +
> + iowrite32(value, cru->base + regs[offset]); }
> +
> +static u32 __rzg2l_cru_read(struct rzg2l_cru_dev *cru, u32 offset) {
> + const u16 *regs = cru->info->regs;
> +
> + /*
> + * CRUnCTRL is a first register on all CRU supported SoCs so validate
> + * rest of the registers have valid offset being set in cru->info->regs.
> + */
> + if (WARN_ON(offset >= RZG2L_CRU_MAX_REG) ||
> + WARN_ON(offset != CRUnCTRL && regs[offset] == 0))
> + return 0;
> +
> + return ioread32(cru->base + regs[offset]);
> }
>
> -static u32 rzg2l_cru_read(struct rzg2l_cru_dev *cru, u32 offset)
> +static inline void
> +__rzg2l_cru_write_constant(struct rzg2l_cru_dev *cru, u32 offset, u32
> +value)
> {
> - return ioread32(cru->base + offset);
> + const u16 *regs = cru->info->regs;
> +
> + BUILD_BUG_ON(offset >= RZG2L_CRU_MAX_REG);
> +
> + iowrite32(value, cru->base + regs[offset]);
Do you need this code as the purpose is to test compile time constant and
It won't execute at run time?
> }
>
> +static inline u32
> +__rzg2l_cru_read_constant(struct rzg2l_cru_dev *cru, u32 offset) {
> + const u16 *regs = cru->info->regs;
> +
> + BUILD_BUG_ON(offset >= RZG2L_CRU_MAX_REG);
> +
> + return ioread32(cru->base + regs[offset]);
Do you need this code as the purpose is to test compile time constant and
It won't execute at run time?
Not sure, maybe adding an entry with MAX_ID in LUT,
that will avoid buffer overflows and you can take out
All out of bound array checks?
Cheers,
Biju
}
> +
> +#define rzg2l_cru_write(cru, offset, value) \
> + (__builtin_constant_p(offset) ? \
> + __rzg2l_cru_write_constant(cru, offset, value) : \
> + __rzg2l_cru_write(cru, offset, value))
> +
> +#define rzg2l_cru_read(cru, offset) \
> + (__builtin_constant_p(offset) ? \
> + __rzg2l_cru_read_constant(cru, offset) : \
> + __rzg2l_cru_read(cru, offset))
> +
> /* Need to hold qlock before calling */ static void return_unused_buffers(struct rzg2l_cru_dev *cru,
> enum vb2_buffer_state state)
> --
> 2.43.0
