Hi Biju,
On Thu, Mar 14, 2024 at 1:49 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > 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>
> > > --- 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 -- 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
