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 <chris.brandt@xxxxxxxxxxx>
> ---
> 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
