RE: [PATCH v3 8/8] pinctrl: renesas: rzg2l: Add support for clone channel control
From: Biju Das
Date: Wed Apr 29 2026 - 13:28:30 EST
Hi Geert,
Thanks for the feedback,
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 28 April 2026 20:44
> Subject: Re: [PATCH v3 8/8] pinctrl: renesas: rzg2l: Add support for clone channel control
>
> 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...
This is not needed based on below comments.
>
> > +
> > +#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?
Agreed.
>
> > +
> > #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
OK.
>
> > * @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;
Will use 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
OK.
>
> > + 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?
Will use switch statement.
>
> > +
> > + 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)) || ...)
Much simpler now.
>
> > + 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;
I agree. No loop and this is much better and simpler.
>
> > +
> > + 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"?
Agreed.
>
> > + }
> > + }
> > +
> > + 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).
>
Thanks for the logic
> > + /* 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))
Agreed.
>
> > + 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...
OK.
>
> > +
> > + pctrl->syscon = syscon_regmap_lookup_by_phandle_args(np, "renesas,clonech",
> > +
> > + 1, &offset);
>
> ... just pass &pctrl->clone_offset.
OK.
>
> > + 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...
I also don't know actual functionality, but the IP won't work as expected without these settings.
Eg:- I2C won't detect the devices.
Cheers,
Biju