Re: [PATCH v3 8/8] pinctrl: renesas: rzg2l: Add support for clone channel control
From: Geert Uytterhoeven
Date: Tue Apr 28 2026 - 15:44:33 EST
Hi Biju,
On Tue, 17 Mar 2026 at 11:16, Biju <biju.das.au@xxxxxxxxx> wrote:
> From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> The RZ/G3L SoC has some IP such as I2C ch{2,3},SCIF ch{3,4,5},
> RSPI ch{1,2} and RSCI ch{1,2,3} need to control the clone channel for
> proper operation. As per the RZ/G3L hardware manual, the clone channel
> setting is to be done before the mux setting.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
Thanks for your patch!
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -152,6 +154,26 @@
> FIELD_PREP_CONST(VARIABLE_PIN_CFG_PORT_MASK, (port)) | \
> FIELD_PREP_CONST(PIN_CFG_MASK, (cfg)))
>
> +#define RZG3L_CLONE_CHANNEL_CFG_PIN_START_MASK GENMASK(31, 29)
> +#define RZG3L_CLONE_CHANNEL_CFG_PIN_END_MASK GENMASK(28, 26)
> +#define RZG3L_CLONE_CHANNEL_CFG_PORT_MASK GENMASK(25, 21)
> +#define RZG3L_CLONE_CHANNEL_CFG_DATA_MASK GENMASK(9, 0)
> +#define RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(port, start_pin, end_pin, cfg) \
> + (FIELD_PREP_CONST(RZG3L_CLONE_CHANNEL_CFG_PIN_START_MASK, (start_pin)) | \
> + FIELD_PREP_CONST(RZG3L_CLONE_CHANNEL_CFG_PIN_END_MASK, (end_pin)) | \
> + FIELD_PREP_CONST(RZG3L_CLONE_CHANNEL_CFG_PORT_MASK, (port)) | \
> + FIELD_PREP_CONST(RZG3L_CLONE_CHANNEL_CFG_DATA_MASK, (cfg)))
s/cfg/data/, but...
> +
> +#define RZG3L_CLONE_CHANNEL_BIT_MASK GENMASK(9, 6)
> +#define RZG3L_CLONE_CHANNEL_VAL_MASK BIT(5)
> +#define RZG3L_CLONE_CHANNEL_SHARED_PIN_MASK BIT(4)
> +#define RZG3L_CLONE_CHANNEL_PFC_MASK GENMASK(3, 0)
> +#define RZG3L_CLONE_CHANNEL_PACK(bit, val, shared_pin, pfc) \
> + (FIELD_PREP_CONST(RZG3L_CLONE_CHANNEL_BIT_MASK, (bit)) | \
> + FIELD_PREP_CONST(RZG3L_CLONE_CHANNEL_VAL_MASK, (val)) | \
> + FIELD_PREP_CONST(RZG3L_CLONE_CHANNEL_SHARED_PIN_MASK, (shared_pin)) | \
> + FIELD_PREP_CONST(RZG3L_CLONE_CHANNEL_PFC_MASK, (pfc)))
... the macro RZG3L_CLONE_CHANNEL_PACK() does not seem to offer much
(apart from reducing the number of parameters of the
RZG3L_CLONE_CHANNEL_PIN_CFG_PACK() macro), so perhaps you can
just drop it, together with the (now unused) definition of
RZG3L_CLONE_CHANNEL_CFG_DATA_MASK() above?
> +
> #define P(off) (0x0000 + (off))
> #define PM(off) (0x0100 + (off) * 2)
> #define PMC(off) (0x0200 + (off))
> @@ -346,6 +370,7 @@ struct rzg2l_pinctrl_pin_settings {
> * @pupd: PUPD registers cache
> * @ien: IEN registers cache
> * @smt: SMT registers cache
> + * @clone: Clone registers cache
register
> * @sd_ch: SD_CH registers cache
> * @eth_poc: ET_POC registers cache
> * @other_poc: OTHER_POC register cache
> @@ -361,6 +386,7 @@ struct rzg2l_pinctrl_reg_cache {
> u32 *ien[2];
> u32 *pupd[2];
> u32 *smt;
> + u32 *clone;
Ugh, this is a pointer to a single u32, allocated dynamically using
devm_kzalloc()? Better store the actual value here:
u32 clone;
> u8 sd_ch[2];
> u8 eth_poc[2];
> u8 oen;
> @@ -617,6 +646,54 @@ static int rzg2l_validate_pin(struct rzg2l_pinctrl *pctrl,
> return 0;
> }
>
> +static int rzg2l_pinctrl_set_clone_mode(struct rzg2l_pinctrl *pctrl,
> + u8 port, u8 pin, u8 func)
> +{
> + static const u8 pfc_table_lut[] = { 2, 4, 5, 6, 7 };
> + u8 start_pin, end_pin;
unsigned int
> + unsigned int i;
> +
> + if (!pctrl->data->clone_pin_configs)
> + return 0;
> +
> + for (i = 0; i < ARRAY_SIZE(pfc_table_lut); i++)
> + if (pfc_table_lut[i] == func)
> + break;
> +
> + if (i == ARRAY_SIZE(pfc_table_lut))
> + return 0;
Just use a switch() statement, and let the compiler optimize it?
> +
> + for (i = 0; i < pctrl->data->n_clone_pins; i++) {
> + u32 pin_data = pctrl->data->clone_pin_configs[i];
> + bool is_shared_pin = FIELD_GET(RZG3L_CLONE_CHANNEL_SHARED_PIN_MASK, pin_data);
> + u8 pin_func = FIELD_GET(RZG3L_CLONE_CHANNEL_PFC_MASK, pin_data);
> + unsigned int j, num_pins;
> +
> + if ((pin_func != func && !(is_shared_pin && (pin_func + 1) == func)) ||
De Morgan:
if ((pin_func != func && (!is_shared_pin || (pin_func + 1) != func)) || ...
might be easier to read?
However, if you would store an 8-bit function mask instead of
a function index, you could get rid of the shared pin bit, and the
obscure "pin_func + 1" test, and simplify to:
if (!(pin_func_mask & BIT(func)) || ...)
> + FIELD_GET(RZG3L_CLONE_CHANNEL_CFG_PORT_MASK, pin_data) != port)
> + continue;
> +
> + start_pin = FIELD_GET(RZG3L_CLONE_CHANNEL_CFG_PIN_START_MASK, pin_data);
> + end_pin = FIELD_GET(RZG3L_CLONE_CHANNEL_CFG_PIN_END_MASK, pin_data);
> + num_pins = end_pin - start_pin + 1;
> +
> + for (j = 0; j < num_pins; j++) {
I would say:
for (j = start_pin; j <= end_pin; j++)
> + u32 bit, val;
> +
> + if ((start_pin + j) != pin)
> + continue;
... but you don't reallly need a loop for this?
if (pin >= start_pin && pin <= end_pin)
continue;
If you would store an 8-bit pin mask instead of start and end pin
indices:
if (!(pinmask & BIT(pin)))
continue;
> +
> + bit = FIELD_GET(RZG3L_CLONE_CHANNEL_BIT_MASK, pin_data);
> + val = FIELD_GET(RZG3L_CLONE_CHANNEL_VAL_MASK, pin_data);
> +
> + return regmap_update_bits(pctrl->syscon, pctrl->clone_offset,
> + BIT(bit), field_prep(BIT(bit), val));
val is just 0 or 1, and always fits, so perhaps replace field_prep(...)
by "val << bit"?
> + }
> + }
> +
> + return 0;
> +}
> +
> static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
> u8 pin, u8 off, u8 func)
> {
> @@ -2647,6 +2728,110 @@ static const struct rzg2l_dedicated_configs rzg3l_dedicated_pins[] = {
> (PIN_CFG_IOLH_B | PIN_CFG_IEN | PIN_CFG_PUPD)) },
> };
>
> +static const u32 r9a08g046_clone_channel_pin_cfg[] = {
> + /* I2C ch2 Bit:0 Value:0 PFC:4 */
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PG, 6, 7, RZG3L_CLONE_CHANNEL_PACK(0, 0, 0, 4)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PH, 2, 3, RZG3L_CLONE_CHANNEL_PACK(0, 0, 0, 4)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PK, 0, 1, RZG3L_CLONE_CHANNEL_PACK(0, 0, 0, 4)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PA, 0, 1, RZG3L_CLONE_CHANNEL_PACK(0, 0, 0, 4)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PA, 4, 5, RZG3L_CLONE_CHANNEL_PACK(0, 0, 0, 4)),
If you would store an 8-bit pin mask instead of start and end pin
indices, you could combine multiple entries with the same port number
and config using ORed values of BIT() and GENMASK(), and thus reduce
table size. E.g. these two entries would become a single entry:
RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PA, BIT(5) | BIT(4) | BIT
(1) | BIT(0), ...)
(I used high-to-low order, to match GENMASK(), which is useful for some
of the entries below).
> + /* RSCI ch1 Bit:12 Value:0 PFC:{5,6} shared pins based on RSCI mode */
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PG, 0, 3, RZG3L_CLONE_CHANNEL_PACK(12, 0, 1, 5)),
If you would store an 8-bit function mask instead of a function index,
you could get rid of the shared pin bit, and make it more obvious both
function 5 and 6 are possible:
RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PG, 0, 3, ... BIT(5) | BIT(6))
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PA, 0, 3, RZG3L_CLONE_CHANNEL_PACK(12, 0, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PB, 6, 7, RZG3L_CLONE_CHANNEL_PACK(12, 0, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PC, 0, 1, RZG3L_CLONE_CHANNEL_PACK(12, 0, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PD, 4, 7, RZG3L_CLONE_CHANNEL_PACK(12, 0, 1, 5)),
> + /* RSCI ch1 Bit:12 Value:1 PFC:{5,6} shared pins based on RSCI mode */
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_P5, 0, 3, RZG3L_CLONE_CHANNEL_PACK(12, 1, 1, 5)),
> + /* RSCI ch2 Bit:13 Value:0 PFC:{5,6} shared pins based on RSCI mode */
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PH, 0, 3, RZG3L_CLONE_CHANNEL_PACK(13, 0, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PK, 0, 3, RZG3L_CLONE_CHANNEL_PACK(13, 0, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PA, 4, 7, RZG3L_CLONE_CHANNEL_PACK(13, 0, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PD, 0, 3, RZG3L_CLONE_CHANNEL_PACK(13, 0, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PE, 0, 3, RZG3L_CLONE_CHANNEL_PACK(13, 0, 1, 5)),
> + /* RSCI ch2 Bit:13 Value:1 PFC:{5,6} shared pins based on RSCI mode */
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_P5, 4, 6, RZG3L_CLONE_CHANNEL_PACK(13, 1, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_P6, 0, 0, RZG3L_CLONE_CHANNEL_PACK(13, 1, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_P6, 5, 6, RZG3L_CLONE_CHANNEL_PACK(13, 1, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_P7, 0, 1, RZG3L_CLONE_CHANNEL_PACK(13, 1, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_P7, 6, 7, RZG3L_CLONE_CHANNEL_PACK(13, 1, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_P8, 0, 1, RZG3L_CLONE_CHANNEL_PACK(13, 1, 1, 5)),
> + /* RSCI ch3 Bit:14 Value:0 PFC:{5,6} shared pins based on RSCI mode */
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PE, 6, 7, RZG3L_CLONE_CHANNEL_PACK(14, 0, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_PF, 0, 1, RZG3L_CLONE_CHANNEL_PACK(14, 0, 1, 5)),
> + /* RSCI ch3 Bit:14 Value:1 PFC:{5,6} shared pins based on RSCI mode */
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_P6, 1, 4, RZG3L_CLONE_CHANNEL_PACK(14, 1, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_P7, 2, 5, RZG3L_CLONE_CHANNEL_PACK(14, 1, 1, 5)),
> + RZG3L_CLONE_CHANNEL_PIN_CFG_PACK(RZG3L_P8, 2, 5, RZG3L_CLONE_CHANNEL_PACK(14, 1, 1, 5)),
> +};
> +
> static int rzg2l_gpio_get_gpioint(unsigned int virq, struct rzg2l_pinctrl *pctrl)
> {
> const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[virq];
> @@ -3204,6 +3393,19 @@ static int rzg2l_pinctrl_probe(struct platform_device *pdev)
> "failed to enable GPIO clk\n");
> }
>
> + if (pctrl->data->clone_pin_configs) {
> + struct device_node *np = pctrl->dev->of_node;
> + u32 offset;
No need for the temporary...
> +
> + pctrl->syscon = syscon_regmap_lookup_by_phandle_args(np, "renesas,clonech",
> + 1, &offset);
... just pass &pctrl->clone_offset.
> + if (IS_ERR(pctrl->syscon))
> + return dev_err_probe(pctrl->dev, PTR_ERR(pctrl->syscon),
> + "Failed to parse renesas,clonech\n");
> +
> + pctrl->clone_offset = offset;
> + }
> +
> raw_spin_lock_init(&pctrl->lock);
> spin_lock_init(&pctrl->bitmap_lock);
> mutex_init(&pctrl->mutex);
The rest LGTM
... Except that I still don't understand what this clone channel
functionality is really doing ;-) The documentation doesn't help much...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds