Re: [PATCH v2 3/5] usb: renesas_usbhs: Update usbhs pipe configuration for RZ/G2L family

Hi Biju,

On Thu, Mar 14, 2024 at 1:49 PM Biju Das <[email protected]> wrote:
> > From: Geert Uytterhoeven <[email protected]>
> > On Wed, Mar 13, 2024 at 7:16 PM Biju Das <[email protected]> wrote:
> > > From: Huy Nguyen <[email protected]>
> > > 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 <[email protected]>
> > > Signed-off-by: Biju Das <[email protected]>

> > > --- 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,

Yes, something like that.

> > >         } 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/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.

OK.

> 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?

I don't have an sh7757lcr or ecovec24 to test. But the risk looks low.

So it looks like a good idea to have two patches:
  1. Improve usbhsc_default_pipe[] for isochronous transfers,
  2. Fix support for RZ/G2L using the default 10-entry pipe.

> [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),

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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