Hi Geert,
Thanks for the feedback.
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: Thursday, March 14, 2024 9:15 AM
> Subject: Re: [PATCH v2 3/5] usb: renesas_usbhs: Update usbhs pipe configuration for RZ/G2L family
>
> Hi Biju,
>
> On Wed, Mar 13, 2024 at 7:16 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > From: Huy Nguyen <huy.nguyen.wh@xxxxxxxxxxx>
> >
> > The RZ/G2L family SoCs has 10 PIPE buffers compared to 16 pipe buffers
> > on RZ/A2M. Update the pipe configuration for RZ/G2L family SoCs and
> > use family SoC specific compatible to handle this difference.
> >
> > Added SoC specific compatible to OF table toavoid ABI breakage with
> > old DTB. To optimize memory usage the SoC specific compatible will be
> > removed later.
> >
> > Signed-off-by: Huy Nguyen <huy.nguyen.wh@xxxxxxxxxxx>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v1->v2:
> > * Dropped using of_device_is_compatible() in probe.
> > * Added usbhs_rzg2l_plat_info and replaced the device data for RZ/G2L
> > from usbhs_rza2_plat_info->usbhs_rzg2l_plat_info.
> > * Moved usbhsc_rzg2l_pipe table near to the user.
> > * Updated commit description.
>
> Thanks for the update!
>
> > --- a/drivers/usb/renesas_usbhs/common.c
> > +++ b/drivers/usb/renesas_usbhs/common.c
> > @@ -640,8 +656,13 @@ static int usbhs_probe(struct platform_device
> > *pdev)
> >
> > /* set default param if platform doesn't have */
> > if (usbhs_get_dparam(priv, has_new_pipe_configs)) {
> > - priv->dparam.pipe_configs = usbhsc_new_pipe;
> > - priv->dparam.pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> > + if (info->driver_param.pipe_configs) {
> > + priv->dparam.pipe_configs = info->driver_param.pipe_configs;
> > + priv->dparam.pipe_size = info->driver_param.pipe_size;
> > + } else {
> > + priv->dparam.pipe_configs = usbhsc_new_pipe;
> > + priv->dparam.pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> > + }
>
> I think it would be cleaner to populate
> renesas_usbhs_platform_info.driver_param.pipe_{configs,size} everywhere, and use info-
> >driver_param.pipe_{configs,size} unconditionally.
You mean, drop static and share the usbhsc_rcar_new_pipe[] to {rcar3,rcar2,rza,rza2}
Like [1]??
[1]
diff --git a/drivers/usb/renesas_usbhs/common.h b/drivers/usb/renesas_usbhs/common.h
index 3fb5bc94dc0d..9dde537c4e2f 100644
--- a/drivers/usb/renesas_usbhs/common.h
+++ b/drivers/usb/renesas_usbhs/common.h
+extern struct renesas_usbhs_driver_pipe_config usbhsc_rcar_new_pipe[];
+
diff --git a/drivers/usb/renesas_usbhs/rcar2.c b/drivers/usb/renesas_usbhs/rcar2.c
index 52756fc2ac9c..3117f76ab556 100644
--- a/drivers/usb/renesas_usbhs/rcar2.c
+++ b/drivers/usb/renesas_usbhs/rcar2.c
@@ -69,7 +69,8 @@ const struct renesas_usbhs_platform_info usbhs_rcar_gen2_plat_info = {
.get_id = usbhs_get_id_as_gadget,
},
.driver_param = {
+ .pipe_configs = usbhsc_rcar_new_pipe,
+ .pipe_size = 16,
>
> > } else if (!priv->dparam.pipe_configs) {
> > priv->dparam.pipe_configs = usbhsc_default_pipe;
> > priv->dparam.pipe_size =
> > ARRAY_SIZE(usbhsc_default_pipe); diff --git
> > a/drivers/usb/renesas_usbhs/rza.h b/drivers/usb/renesas_usbhs/rza.h
> > index a29b75fef057..8b879aa34a20 100644
> > --- a/drivers/usb/renesas_usbhs/rza.h
> > +++ b/drivers/usb/renesas_usbhs/rza.h
> > @@ -3,3 +3,4 @@
> >
> > extern const struct renesas_usbhs_platform_info usbhs_rza1_plat_info;
> > extern const struct renesas_usbhs_platform_info usbhs_rza2_plat_info;
> > +extern const struct renesas_usbhs_platform_info
> > +usbhs_rzg2l_plat_info;
> > diff --git a/drivers/usb/renesas_usbhs/rza2.c
> > b/drivers/usb/renesas_usbhs/rza2.c
> > index f079817250bb..0336b419b37c 100644
> > --- a/drivers/usb/renesas_usbhs/rza2.c
> > +++ b/drivers/usb/renesas_usbhs/rza2.c
> > @@ -58,6 +58,36 @@ static int usbhs_rza2_power_ctrl(struct platform_device *pdev,
> > return retval;
> > }
> >
[1]
> > +/* commonly used on RZ/G2L family */
> > +static struct renesas_usbhs_driver_pipe_config usbhsc_rzg2l_pipe[] = {
> > + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_CONTROL, 64, 0x00, false),
> > + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_ISOC, 1024, 0x08, true),
> > + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_ISOC, 1024, 0x28, true),
> > + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_BULK, 512, 0x48, true),
> > + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_BULK, 512, 0x58, true),
> > + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_BULK, 512, 0x68, true),
> > + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_INT, 64, 0x04, false),
> > + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_INT, 64, 0x05, false),
> > + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_INT, 64, 0x06, false),
> > + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_INT, 64, 0x07, false), };
>
> This is similar (but slightly different) from usbhsc_default_pipe[].
> Can RZ/G2L work with usbhsc_default_pipe[] instead? If yes, you could just set .has_new_pipe_configs
> to zero instead of adding new code/data.
All our customers are using [1] compared to default_pipe[2], from HW manual, I feel [1] is better
as it involves fewer interrupts. Can we replace [2] with [1]?
The difference is setting double buffer on Isochronous Transfers.
Setting the buffer operating mode enables high-speed data transfers with fewer interrupts
to be performed by using double-buffering and continuous transfer of data packets.
Since [1] is better compared to [2], if SH can work with [1], we can
replace [2] with [1], do we have any SH platform to test this?
Cheers,
Biju
[2]
RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_CONTROL, 64, 0x00, false),
RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_ISOC, 1024, 0x08, false),
RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_ISOC, 1024, 0x18, false),
RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_BULK, 512, 0x28, true),
RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_BULK, 512, 0x38, true),
RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_BULK, 512, 0x48, true),
RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_INT, 64, 0x04, false),
RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_INT, 64, 0x05, false),
RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_INT, 64, 0x06, false),
RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_INT, 64, 0x07, false),
>
> > +
> > +const struct renesas_usbhs_platform_info usbhs_rzg2l_plat_info = {
> > + .platform_callback = {
> > + .hardware_init = usbhs_rza2_hardware_init,
> > + .hardware_exit = usbhs_rza2_hardware_exit,
> > + .power_ctrl = usbhs_rza2_power_ctrl,
> > + .get_id = usbhs_get_id_as_gadget,
> > + },
> > + .driver_param = {
> > + .pipe_configs = usbhsc_rzg2l_pipe,
> > + .pipe_size = ARRAY_SIZE(usbhsc_rzg2l_pipe),
> > + .has_cnen = 1,
> > + .cfifo_byte_addr = 1,
> > + .has_new_pipe_configs = 1,
> > + },
> > +};
> > +
> > const struct renesas_usbhs_platform_info usbhs_rza2_plat_info = {
> > .platform_callback = {
> > .hardware_init = usbhs_rza2_hardware_init,
>
> 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
