Hi Chrisさん Thank you for the patch!
> From: Chris Brandt, Sent: Tuesday, May 7, 2019 8:46 AM > > The RZ/A2 is similar to the R-Car Gen3 with some small differences. > > Signed-off-by: Chris Brandt <[email protected]> > --- > drivers/usb/renesas_usbhs/Makefile | 2 +- > drivers/usb/renesas_usbhs/common.c | 27 +++++++++---- > drivers/usb/renesas_usbhs/common.h | 13 ++++++ > drivers/usb/renesas_usbhs/fifo.c | 8 +++- > drivers/usb/renesas_usbhs/rza.h | 1 + > drivers/usb/renesas_usbhs/rza2.c | 82 > ++++++++++++++++++++++++++++++++++++++ > include/linux/usb/renesas_usbhs.h | 1 + > 7 files changed, 124 insertions(+), 10 deletions(-) > create mode 100644 drivers/usb/renesas_usbhs/rza2.c > > diff --git a/drivers/usb/renesas_usbhs/Makefile > b/drivers/usb/renesas_usbhs/Makefile > index 5c5b51bb48ef..a1fed56b0957 100644 > --- a/drivers/usb/renesas_usbhs/Makefile > +++ b/drivers/usb/renesas_usbhs/Makefile > @@ -5,7 +5,7 @@ > > obj-$(CONFIG_USB_RENESAS_USBHS) += renesas_usbhs.o > > -renesas_usbhs-y := common.o mod.o pipe.o fifo.o rcar2.o > rcar3.o rza.o > +renesas_usbhs-y := common.o mod.o pipe.o fifo.o rcar2.o > rcar3.o rza.o rza2.o > > ifneq ($(CONFIG_USB_RENESAS_USBHS_HCD),) > renesas_usbhs-y += mod_host.o > diff --git a/drivers/usb/renesas_usbhs/common.c > b/drivers/usb/renesas_usbhs/common.c > index 249fbee97f3f..8293f34b964a 100644 > --- a/drivers/usb/renesas_usbhs/common.c > +++ b/drivers/usb/renesas_usbhs/common.c > @@ -44,13 +44,6 @@ > */ > > > -#define USBHSF_RUNTIME_PWCTRL (1 << 0) > - > -/* status */ > -#define usbhsc_flags_init(p) do {(p)->flags = 0; } while (0) > -#define usbhsc_flags_set(p, b) ((p)->flags |= (b)) > -#define usbhsc_flags_clr(p, b) ((p)->flags &= ~(b)) > -#define usbhsc_flags_has(p, b) ((p)->flags & (b)) I would like to separate this patch to some patches like below to review the patch(es) easily: 1. Just move these definitions to common.h. 2. Add USBHSF_HAS_CNEN and related code. 3. Add USBHSF_CFIFO_BYTE_ADDR and related code. 4. Add RZ/A2 support. <snip> > @@ -620,6 +623,11 @@ static struct renesas_usbhs_platform_info > *usbhs_parse_dt(struct device *dev) > dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe); > } > > + if (dparam->type == USBHS_TYPE_RZA2) { > + dparam->pipe_configs = usbhsc_new_pipe; > + dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe); > + } > + It's the same with RZA1. So, I think we can reuse the code like below. What do you think? + if (dparam->type == USBHS_TYPE_RZA1 || + dparam->type == USBHS_TYPE_RZA2) { dparam->pipe_configs = usbhsc_new_pipe; dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe); } <snip> > diff --git a/drivers/usb/renesas_usbhs/fifo.c > b/drivers/usb/renesas_usbhs/fifo.c > index 39fa2fc1b8b7..9b8220c2c9cc 100644 > --- a/drivers/usb/renesas_usbhs/fifo.c > +++ b/drivers/usb/renesas_usbhs/fifo.c > @@ -543,8 +543,12 @@ static int usbhsf_pio_try_push(struct usbhs_pkt *pkt, > int *is_done) > } > > /* the rest operation */ > - for (i = 0; i < len; i++) > - iowrite8(buf[i], addr + (0x03 - (i & 0x03))); > + if (usbhsc_flags_has(priv, USBHSF_CFIFO_BYTE_ADDR)) > + for (i = 0; i < len; i++) > + iowrite8(buf[i], addr + (i & 0x03)); > + else > + for (i = 0; i < len; i++) > + iowrite8(buf[i], addr + (0x03 - (i & 0x03))); I prefer to add "{ }" on "if" and "else" like below. if (usbhsc_flags_has(priv, USBHSF_CFIFO_BYTE_ADDR)) { for (i = 0; i < len; i++) iowrite8(buf[i], addr + (i & 0x03)); } else { for (i = 0; i < len; i++) iowrite8(buf[i], addr + (0x03 - (i & 0x03))); } > /* > * variable update > diff --git a/drivers/usb/renesas_usbhs/rza.h b/drivers/usb/renesas_usbhs/rza.h > index ca917ca54f6d..073a53d1d442 100644 > --- a/drivers/usb/renesas_usbhs/rza.h > +++ b/drivers/usb/renesas_usbhs/rza.h > @@ -2,3 +2,4 @@ > #include "common.h" > > extern const struct renesas_usbhs_platform_callback usbhs_rza1_ops; > +extern const struct renesas_usbhs_platform_callback usbhs_rza2_ops; > diff --git a/drivers/usb/renesas_usbhs/rza2.c > b/drivers/usb/renesas_usbhs/rza2.c > new file mode 100644 > index 000000000000..c0b5dfa4b85d > --- /dev/null > +++ b/drivers/usb/renesas_usbhs/rza2.c > @@ -0,0 +1,82 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Renesas USB driver RZ/A2 initialization and power control > + * > + * Copyright (C) 2019 Chris Brandt > + * Copyright (C) 2019 Renesas Electronics Corporation > + */ > + > +#include <linux/delay.h> > +#include <linux/io.h> > +#include <linux/of_device.h> > +#include <linux/phy/phy.h> > +#include "common.h" > +#include "rza.h" > + > + > +static int usbhs_rza2_hardware_init(struct platform_device *pdev) > +{ > + struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev); > + > + if (IS_ENABLED(CONFIG_GENERIC_PHY)) { > + struct phy *phy = phy_get(&pdev->dev, "usb"); > + > + if (IS_ERR(phy)) > + return PTR_ERR(phy); > + > + priv->phy = phy; > + return 0; > + } > + return -ENXIO; > +} > + > +static int usbhs_rza2_hardware_exit(struct platform_device *pdev) > +{ > + struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev); > + > + if (priv->phy) { > + phy_put(priv->phy); > + priv->phy = NULL; > + } > + > + return 0; > +} > + > +static int usbhs_rza2_power_ctrl(struct platform_device *pdev, > + void __iomem *base, int enable) > +{ > + struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev); > + int retval = -ENODEV; > + > + if (priv->phy) { > + if (enable) { > + retval = phy_init(priv->phy); > + if (enable) { > + usbhs_bset(priv, SUSPMODE, SUSPM, SUSPM); > + /* Wait 100 usec for PLL to become stable */ > + udelay(100); > + } else { This else code never runs. So, > + usbhs_bset(priv, SUSPMODE, SUSPM, 0); this code should be on the below "here"? > + } > + if (!retval) > + retval = phy_power_on(priv->phy); > + } else { here? Best regardsm Yoshihiro Shimoda
