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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux