diff mbox series

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

Message ID 20240313181602.156840-4-biju.das.jz@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Fix USB pipe configuration for RZ/G2L | expand

Commit Message

Biju Das March 13, 2024, 6:16 p.m. UTC 
From: Huy Nguyen <huy.nguyen.wh@renesas.com>

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@renesas.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
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.
---
 drivers/usb/renesas_usbhs/common.c | 25 +++++++++++++++++++++++--
 drivers/usb/renesas_usbhs/rza.h    |  1 +
 drivers/usb/renesas_usbhs/rza2.c   | 30 ++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 2 deletions(-)

Comments

Geert Uytterhoeven March 14, 2024, 9:14 a.m. UTC | #1 
Hi Biju,

On Wed, Mar 13, 2024 at 7:16 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> From: Huy Nguyen <huy.nguyen.wh@renesas.com>
>
> 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@renesas.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> 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.

>         } 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;
>  }
>
> +/* 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.

> +
> +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
Biju Das March 14, 2024, 12:49 p.m. UTC | #2 
Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> 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@bp.renesas.com> wrote:
> > From: Huy Nguyen <huy.nguyen.wh@renesas.com>
> >
> > 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@renesas.com>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > 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@linux-m68k.org
> 
> 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
Geert Uytterhoeven March 14, 2024, 1:04 p.m. UTC | #3 
Hi Biju,

On Thu, Mar 14, 2024 at 1:49 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > On Wed, Mar 13, 2024 at 7:16 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > From: Huy Nguyen <huy.nguyen.wh@renesas.com>
> > > 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@renesas.com>
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

> > > --- 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
Biju Das March 14, 2024, 1:09 p.m. UTC | #4 
Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Thursday, March 14, 2024 1:04 PM
> Subject: 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 <biju.das.jz@bp.renesas.com> wrote:
> > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Wed, Mar 13, 2024
> > > at 7:16 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > From: Huy Nguyen <huy.nguyen.wh@renesas.com> 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@renesas.com>
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> > > > --- 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)
> 
> > 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.
> 

Agreed. Will send v3 with these changes.

Cheers,
Biju
diff mbox series

Patch

diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
index 0c62e4c6c88d..47c37c2043cb 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -565,6 +565,18 @@  static const struct of_device_id usbhs_of_match[] = {
 		.compatible = "renesas,usbhs-r8a77995",
 		.data = &usbhs_rcar_gen3_with_pll_plat_info,
 	},
+	{
+		.compatible = "renesas,usbhs-r9a07g043",
+		.data = &usbhs_rzg2l_plat_info,
+	},
+	{
+		.compatible = "renesas,usbhs-r9a07g044",
+		.data = &usbhs_rzg2l_plat_info,
+	},
+	{
+		.compatible = "renesas,usbhs-r9a07g054",
+		.data = &usbhs_rzg2l_plat_info,
+	},
 	{
 		.compatible = "renesas,rcar-gen2-usbhs",
 		.data = &usbhs_rcar_gen2_plat_info,
@@ -581,6 +593,10 @@  static const struct of_device_id usbhs_of_match[] = {
 		.compatible = "renesas,rza2-usbhs",
 		.data = &usbhs_rza2_plat_info,
 	},
+	{
+		.compatible = "renesas,rzg2l-usbhs",
+		.data = &usbhs_rzg2l_plat_info,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, usbhs_of_match);
@@ -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);
+		}
 	} 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;
 }
 
+/* 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),
+};
+
+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,