[PATCH 0/3] phy: renesas: rcar-gen3-usb2: enable/disable independent irqs

[PATCH 0/3] phy: renesas: rcar-gen3-usb2: enable/disable independent irqs

[Yoshihiro Shimoda]

Since the previous code enabled/disabled the irqs both OHCI and EHCI,
it is possible to cause unexpected interruptions. To avoid this,
this patch creates multiple phy instances from phandle and
enables/disables independent irqs by the instances.

Yoshihiro Shimoda (3):
  dt-bindings: phy: rcar-gen3-phy-usb2: Revise #phy-cells property
  phy: renesas: rcar-gen3-usb2: Use pdev's device pointer on dev_vdbg()
  phy: renesas: rcar-gen3-usb2: enable/disable independent irqs

 .../devicetree/bindings/phy/rcar-gen3-phy-usb2.txt |   7 +-
 drivers/phy/renesas/phy-rcar-gen3-usb2.c           | 191 +++++++++++++++++----
 2 files changed, 168 insertions(+), 30 deletions(-)

-- 
2.7.4

[PATCH 1/3] dt-bindings: phy: rcar-gen3-phy-usb2: Revise #phy-cells property

[Yoshihiro Shimoda]

To have the detailed property on each PHY specifier, this patch revices
the #phy-cells property.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
index ad9c290..7f0d4bf 100644
--- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
+++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
@@ -27,7 +27,12 @@ Required properties:
 
 - reg: offset and length of the partial USB 2.0 Host register block.
 - clocks: clock phandle and specifier pair(s).
-- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
+- #phy-cells: see phy-bindings.txt in the same directory, must be <1>.
+
+The phandle's argument in the PHY specifier is the INT_STATUS bit of controller:
+- 1 = USBH_INTA (OHCI)
+- 2 = USBH_INTB (EHCI)
+- 3 = UCOM_INT (OTG and BC)
 
 Optional properties:
 To use a USB channel where USB 2.0 Host and HSUSB (USB 2.0 Peripheral) are
-- 
2.7.4

[PATCH 2/3] phy: renesas: rcar-gen3-usb2: Use pdev's device pointer on dev_vdbg()

[Yoshihiro Shimoda]

To implement multiple phy instances in the future, this patch uses
pdev's device pointer on dev_vdbg() instead of the phy's device
pointer.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/phy/renesas/phy-rcar-gen3-usb2.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 0a34782..4bdb2ed 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -80,6 +80,7 @@
 
 struct rcar_gen3_chan {
 	void __iomem *base;
+	struct device *dev;	/* platform_device's device */
 	struct extcon_dev *extcon;
 	struct phy *phy;
 	struct regulator *vbus;
@@ -120,7 +121,7 @@ static void rcar_gen3_set_host_mode(struct rcar_gen3_chan *ch, int host)
 	void __iomem *usb2_base = ch->base;
 	u32 val = readl(usb2_base + USB2_COMMCTRL);
 
-	dev_vdbg(&ch->phy->dev, "%s: %08x, %d\n", __func__, val, host);
+	dev_vdbg(ch->dev, "%s: %08x, %d\n", __func__, val, host);
 	if (host)
 		val &= ~USB2_COMMCTRL_OTG_PERI;
 	else
@@ -133,7 +134,7 @@ static void rcar_gen3_set_linectrl(struct rcar_gen3_chan *ch, int dp, int dm)
 	void __iomem *usb2_base = ch->base;
 	u32 val = readl(usb2_base + USB2_LINECTRL1);
 
-	dev_vdbg(&ch->phy->dev, "%s: %08x, %d, %d\n", __func__, val, dp, dm);
+	dev_vdbg(ch->dev, "%s: %08x, %d, %d\n", __func__, val, dp, dm);
 	val &= ~(USB2_LINECTRL1_DP_RPD | USB2_LINECTRL1_DM_RPD);
 	if (dp)
 		val |= USB2_LINECTRL1_DP_RPD;
@@ -147,7 +148,7 @@ static void rcar_gen3_enable_vbus_ctrl(struct rcar_gen3_chan *ch, int vbus)
 	void __iomem *usb2_base = ch->base;
 	u32 val = readl(usb2_base + USB2_ADPCTRL);
 
-	dev_vdbg(&ch->phy->dev, "%s: %08x, %d\n", __func__, val, vbus);
+	dev_vdbg(ch->dev, "%s: %08x, %d\n", __func__, val, vbus);
 	if (vbus)
 		val |= USB2_ADPCTRL_DRVVBUS;
 	else
@@ -401,7 +402,7 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
 	irqreturn_t ret = IRQ_NONE;
 
 	if (status & USB2_OBINT_BITS) {
-		dev_vdbg(&ch->phy->dev, "%s: %08x\n", __func__, status);
+		dev_vdbg(ch->dev, "%s: %08x\n", __func__, status);
 		writel(USB2_OBINT_BITS, usb2_base + USB2_OBINTSTA);
 		rcar_gen3_device_recognition(ch);
 		ret = IRQ_HANDLED;
@@ -499,6 +500,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, channel);
 	phy_set_drvdata(channel->phy, channel);
+	channel->dev = dev;
 
 	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
 	if (IS_ERR(provider)) {
-- 
2.7.4

[PATCH 3/3] phy: renesas: rcar-gen3-usb2: enable/disable independent irqs

[Yoshihiro Shimoda]

Since the previous code enabled/disabled the irqs both OHCI and EHCI,
it is possible to cause unexpected interruptions. To avoid this,
this patch creates multiple phy instances from phandle and
enables/disables independent irqs by the instances.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/phy/renesas/phy-rcar-gen3-usb2.c | 181 ++++++++++++++++++++++++++-----
 1 file changed, 156 insertions(+), 25 deletions(-)

diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 4bdb2ed..bbe0fe5 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -37,11 +37,8 @@
 
 /* INT_ENABLE */
 #define USB2_INT_ENABLE_UCOM_INTEN	BIT(3)
-#define USB2_INT_ENABLE_USBH_INTB_EN	BIT(2)
-#define USB2_INT_ENABLE_USBH_INTA_EN	BIT(1)
-#define USB2_INT_ENABLE_INIT		(USB2_INT_ENABLE_UCOM_INTEN | \
-					 USB2_INT_ENABLE_USBH_INTB_EN | \
-					 USB2_INT_ENABLE_USBH_INTA_EN)
+#define USB2_INT_ENABLE_USBH_INTB_EN	BIT(2)	/* For EHCI */
+#define USB2_INT_ENABLE_USBH_INTA_EN	BIT(1)	/* For OHCI */
 
 /* USBCTR */
 #define USB2_USBCTR_DIRPD	BIT(2)
@@ -78,11 +75,33 @@
 #define USB2_ADPCTRL_IDPULLUP		BIT(5)	/* 1 = ID sampling is enabled */
 #define USB2_ADPCTRL_DRVVBUS		BIT(4)
 
+#define NUM_OF_PHYS			4
+#define PHY_INDEX_BOTH_HC		0
+#define PHY_INDEX_OHCI			1
+#define PHY_INDEX_EHCI			2
+#define PHY_INDEX_HSUSB			3
+
+static const u32 rcar_gen3_int_enable[NUM_OF_PHYS] = {
+	USB2_INT_ENABLE_USBH_INTB_EN | USB2_INT_ENABLE_USBH_INTA_EN,
+	USB2_INT_ENABLE_USBH_INTA_EN,
+	USB2_INT_ENABLE_USBH_INTB_EN,
+	0
+};
+
+struct rcar_gen3_phy {
+	struct phy *phy;
+	struct rcar_gen3_chan *ch;
+	u32 int_enable_bits;
+	bool initialized;
+	bool otg_initialized;
+	bool powered;
+};
+
 struct rcar_gen3_chan {
 	void __iomem *base;
 	struct device *dev;	/* platform_device's device */
 	struct extcon_dev *extcon;
-	struct phy *phy;
+	struct rcar_gen3_phy rphys[NUM_OF_PHYS];
 	struct regulator *vbus;
 	struct work_struct work;
 	enum usb_dr_mode dr_mode;
@@ -250,6 +269,42 @@ static enum phy_mode rcar_gen3_get_phy_mode(struct rcar_gen3_chan *ch)
 	return PHY_MODE_USB_DEVICE;
 }
 
+static bool rcar_gen3_is_any_rphy_initialized(struct rcar_gen3_chan *ch)
+{
+	int i;
+
+	for (i = 0; i < NUM_OF_PHYS; i++) {
+		if (ch->rphys[i].initialized)
+			return true;
+	}
+
+	return false;
+}
+
+static bool rcar_gen3_needs_init_otg(struct rcar_gen3_chan *ch)
+{
+	int i;
+
+	for (i = 0; i < NUM_OF_PHYS; i++) {
+		if (ch->rphys[i].otg_initialized)
+			return false;
+	}
+
+	return true;
+}
+
+static bool rcar_gen3_are_all_rphys_power_off(struct rcar_gen3_chan *ch)
+{
+	int i;
+
+	for (i = 0; i < NUM_OF_PHYS; i++) {
+		if (ch->rphys[i].powered)
+			return false;
+	}
+
+	return true;
+}
+
 static ssize_t role_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t count)
 {
@@ -257,7 +312,7 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr,
 	bool is_b_device;
 	enum phy_mode cur_mode, new_mode;
 
-	if (!ch->is_otg_channel || !ch->phy->init_count)
+	if (!ch->is_otg_channel || !rcar_gen3_is_any_rphy_initialized(ch))
 		return -EIO;
 
 	if (!strncmp(buf, "host", strlen("host")))
@@ -295,7 +350,7 @@ static ssize_t role_show(struct device *dev, struct device_attribute *attr,
 {
 	struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
 
-	if (!ch->is_otg_channel || !ch->phy->init_count)
+	if (!ch->is_otg_channel || !rcar_gen3_is_any_rphy_initialized(ch))
 		return -EIO;
 
 	return sprintf(buf, "%s\n", rcar_gen3_is_host(ch) ? "host" :
@@ -329,37 +384,62 @@ static void rcar_gen3_init_otg(struct rcar_gen3_chan *ch)
 
 static int rcar_gen3_phy_usb2_init(struct phy *p)
 {
-	struct rcar_gen3_chan *channel = phy_get_drvdata(p);
+	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
+	struct rcar_gen3_chan *channel = rphy->ch;
 	void __iomem *usb2_base = channel->base;
+	u32 val;
 
 	/* Initialize USB2 part */
-	writel(USB2_INT_ENABLE_INIT, usb2_base + USB2_INT_ENABLE);
+	val = readl(usb2_base + USB2_INT_ENABLE);
+	val |= USB2_INT_ENABLE_UCOM_INTEN | rphy->int_enable_bits;
+	writel(val, usb2_base + USB2_INT_ENABLE);
 	writel(USB2_SPD_RSM_TIMSET_INIT, usb2_base + USB2_SPD_RSM_TIMSET);
 	writel(USB2_OC_TIMSET_INIT, usb2_base + USB2_OC_TIMSET);
 
 	/* Initialize otg part */
-	if (channel->is_otg_channel)
-		rcar_gen3_init_otg(channel);
+	if (channel->is_otg_channel) {
+		if (rcar_gen3_needs_init_otg(channel))
+			rcar_gen3_init_otg(channel);
+		rphy->otg_initialized = true;
+	}
+
+	rphy->initialized = true;
 
 	return 0;
 }
 
 static int rcar_gen3_phy_usb2_exit(struct phy *p)
 {
-	struct rcar_gen3_chan *channel = phy_get_drvdata(p);
+	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
+	struct rcar_gen3_chan *channel = rphy->ch;
+	void __iomem *usb2_base = channel->base;
+	u32 val;
 
-	writel(0, channel->base + USB2_INT_ENABLE);
+	rphy->initialized = false;
+
+	if (channel->is_otg_channel)
+		rphy->otg_initialized = false;
+
+	val = readl(usb2_base + USB2_INT_ENABLE);
+	val &= ~rphy->int_enable_bits;
+	if (!rcar_gen3_is_any_rphy_initialized(channel))
+		val &= ~USB2_INT_ENABLE_UCOM_INTEN;
+	writel(val, usb2_base + USB2_INT_ENABLE);
 
 	return 0;
 }
 
 static int rcar_gen3_phy_usb2_power_on(struct phy *p)
 {
-	struct rcar_gen3_chan *channel = phy_get_drvdata(p);
+	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
+	struct rcar_gen3_chan *channel = rphy->ch;
 	void __iomem *usb2_base = channel->base;
 	u32 val;
 	int ret;
 
+	if (!rcar_gen3_are_all_rphys_power_off(channel))
+		return 0;
+
 	if (channel->vbus) {
 		ret = regulator_enable(channel->vbus);
 		if (ret)
@@ -372,14 +452,22 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
 	val &= ~USB2_USBCTR_PLL_RST;
 	writel(val, usb2_base + USB2_USBCTR);
 
+	rphy->powered = true;
+
 	return 0;
 }
 
 static int rcar_gen3_phy_usb2_power_off(struct phy *p)
 {
-	struct rcar_gen3_chan *channel = phy_get_drvdata(p);
+	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
+	struct rcar_gen3_chan *channel = rphy->ch;
 	int ret = 0;
 
+	rphy->powered = false;
+
+	if (!rcar_gen3_are_all_rphys_power_off(channel))
+		return 0;
+
 	if (channel->vbus)
 		ret = regulator_disable(channel->vbus);
 
@@ -426,13 +514,51 @@ static const unsigned int rcar_gen3_phy_cable[] = {
 	EXTCON_NONE,
 };
 
+static struct phy *rcar_gen3_phy_usb2_xlate(struct device *dev,
+					    struct of_phandle_args *args)
+{
+	struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
+
+	if (args->args_count == 0)	/* For old version dts */
+		return ch->rphys[PHY_INDEX_BOTH_HC].phy;
+	else if (args->args_count > 1)	/* Prevent invalid args count */
+		return ERR_PTR(-ENODEV);
+
+	if (args->args[0] >= NUM_OF_PHYS)
+		return ERR_PTR(-ENODEV);
+
+	return ch->rphys[args->args[0]].phy;
+}
+
+static enum usb_dr_mode rcar_gen3_get_dr_mode(struct device_node *np)
+{
+	enum usb_dr_mode candidate = USB_DR_MODE_UNKNOWN, tmp;
+	int i;
+
+	/*
+	 * If one of device nodes has other dr_mode except UNKNOWN,
+	 * this function returns UNKNOWN.
+	 */
+	for (i = 0; i < NUM_OF_PHYS; i++) {
+		tmp = of_usb_get_dr_mode_by_phy(np, i);
+		if (tmp != USB_DR_MODE_UNKNOWN) {
+			if (candidate == USB_DR_MODE_UNKNOWN)
+				candidate = tmp;
+			else if (candidate != tmp)
+				return USB_DR_MODE_UNKNOWN;
+		}
+	}
+
+	return candidate;
+}
+
 static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct rcar_gen3_chan *channel;
 	struct phy_provider *provider;
 	struct resource *res;
-	int irq, ret = 0;
+	int irq, ret = 0, i;
 
 	if (!dev->of_node) {
 		dev_err(dev, "This driver needs device tree\n");
@@ -458,7 +584,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 			dev_err(dev, "No irq handler (%d)\n", irq);
 	}
 
-	channel->dr_mode = of_usb_get_dr_mode_by_phy(dev->of_node, 0);
+	channel->dr_mode = rcar_gen3_get_dr_mode(dev->of_node);
 	if (channel->dr_mode != USB_DR_MODE_UNKNOWN) {
 		int ret;
 
@@ -482,11 +608,17 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 	 * And then, phy-core will manage runtime pm for this device.
 	 */
 	pm_runtime_enable(dev);
-	channel->phy = devm_phy_create(dev, NULL, &rcar_gen3_phy_usb2_ops);
-	if (IS_ERR(channel->phy)) {
-		dev_err(dev, "Failed to create USB2 PHY\n");
-		ret = PTR_ERR(channel->phy);
-		goto error;
+	for (i = 0; i < NUM_OF_PHYS; i++) {
+		channel->rphys[i].phy = devm_phy_create(dev, NULL,
+						  &rcar_gen3_phy_usb2_ops);
+		if (IS_ERR(channel->rphys[i].phy)) {
+			dev_err(dev, "Failed to create USB2 PHY\n");
+			ret = PTR_ERR(channel->rphys[i].phy);
+			goto error;
+		}
+		channel->rphys[i].ch = channel;
+		channel->rphys[i].int_enable_bits = rcar_gen3_int_enable[i];
+		phy_set_drvdata(channel->rphys[i].phy, &channel->rphys[i]);
 	}
 
 	channel->vbus = devm_regulator_get_optional(dev, "vbus");
@@ -499,10 +631,9 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, channel);
-	phy_set_drvdata(channel->phy, channel);
 	channel->dev = dev;
 
-	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	provider = devm_of_phy_provider_register(dev, rcar_gen3_phy_usb2_xlate);
 	if (IS_ERR(provider)) {
 		dev_err(dev, "Failed to register PHY provider\n");
 		ret = PTR_ERR(provider);
-- 
2.7.4

Re: [PATCH 3/3] phy: renesas: rcar-gen3-usb2: enable/disable independent irqs

[Simon Horman]

Hi Shimoda-san,

On Mon, Apr 01, 2019 at 09:01:23PM +0900, Yoshihiro Shimoda wrote:
> Since the previous code enabled/disabled the irqs both OHCI and EHCI,
> it is possible to cause unexpected interruptions. To avoid this,
> this patch creates multiple phy instances from phandle and
> enables/disables independent irqs by the instances.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

I noted a few nits below but overall this looks good to me.

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> ---
>  drivers/phy/renesas/phy-rcar-gen3-usb2.c | 181 ++++++++++++++++++++++++++-----
>  1 file changed, 156 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index 4bdb2ed..bbe0fe5 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -37,11 +37,8 @@
>  
>  /* INT_ENABLE */
>  #define USB2_INT_ENABLE_UCOM_INTEN	BIT(3)
> -#define USB2_INT_ENABLE_USBH_INTB_EN	BIT(2)
> -#define USB2_INT_ENABLE_USBH_INTA_EN	BIT(1)
> -#define USB2_INT_ENABLE_INIT		(USB2_INT_ENABLE_UCOM_INTEN | \
> -					 USB2_INT_ENABLE_USBH_INTB_EN | \
> -					 USB2_INT_ENABLE_USBH_INTA_EN)
> +#define USB2_INT_ENABLE_USBH_INTB_EN	BIT(2)	/* For EHCI */
> +#define USB2_INT_ENABLE_USBH_INTA_EN	BIT(1)	/* For OHCI */
>  
>  /* USBCTR */
>  #define USB2_USBCTR_DIRPD	BIT(2)
> @@ -78,11 +75,33 @@
>  #define USB2_ADPCTRL_IDPULLUP		BIT(5)	/* 1 = ID sampling is enabled */
>  #define USB2_ADPCTRL_DRVVBUS		BIT(4)
>  
> +#define NUM_OF_PHYS			4
> +#define PHY_INDEX_BOTH_HC		0
> +#define PHY_INDEX_OHCI			1
> +#define PHY_INDEX_EHCI			2
> +#define PHY_INDEX_HSUSB			3

nit: I think the above #defines would be better expressed as an enum.

...

> +static struct phy *rcar_gen3_phy_usb2_xlate(struct device *dev,
> +					    struct of_phandle_args *args)
> +{
> +	struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
> +
> +	if (args->args_count == 0)	/* For old version dts */
> +		return ch->rphys[PHY_INDEX_BOTH_HC].phy;
> +	else if (args->args_count > 1)	/* Prevent invalid args count */
> +		return ERR_PTR(-ENODEV);
> +
> +	if (args->args[0] >= NUM_OF_PHYS)
> +		return ERR_PTR(-ENODEV);
> +
> +	return ch->rphys[args->args[0]].phy;
> +}
> +
> +static enum usb_dr_mode rcar_gen3_get_dr_mode(struct device_node *np)
> +{
> +	enum usb_dr_mode candidate = USB_DR_MODE_UNKNOWN, tmp;

nit: I think that there could be a better name for tmp, f.e. mode
nit: The scope of tmp could be limited to inside the for loop.

> +	int i;
> +
> +	/*
> +	 * If one of device nodes has other dr_mode except UNKNOWN,
> +	 * this function returns UNKNOWN.
> +	 */
> +	for (i = 0; i < NUM_OF_PHYS; i++) {
> +		tmp = of_usb_get_dr_mode_by_phy(np, i);
> +		if (tmp != USB_DR_MODE_UNKNOWN) {
> +			if (candidate == USB_DR_MODE_UNKNOWN)
> +				candidate = tmp;
> +			else if (candidate != tmp)
> +				return USB_DR_MODE_UNKNOWN;
> +		}
> +	}
> +
> +	return candidate;
> +}

...

Re: [PATCH 1/3] dt-bindings: phy: rcar-gen3-phy-usb2: Revise #phy-cells property

[Simon Horman]

Hi Shimoda-san,

On Mon, Apr 01, 2019 at 09:01:21PM +0900, Yoshihiro Shimoda wrote:
> To have the detailed property on each PHY specifier, this patch revices
> the #phy-cells property.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> index ad9c290..7f0d4bf 100644
> --- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> @@ -27,7 +27,12 @@ Required properties:
>  
>  - reg: offset and length of the partial USB 2.0 Host register block.
>  - clocks: clock phandle and specifier pair(s).
> -- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.

The driver currently supports <0> and with this changeset continues to do
so. I also assume there are existing DTs that use <0>. Would it be
better to describe this as deprecated?

> +- #phy-cells: see phy-bindings.txt in the same directory, must be <1>.
> +
> +The phandle's argument in the PHY specifier is the INT_STATUS bit of controller:
> +- 1 = USBH_INTA (OHCI)
> +- 2 = USBH_INTB (EHCI)
> +- 3 = UCOM_INT (OTG and BC)
>  
>  Optional properties:
>  To use a USB channel where USB 2.0 Host and HSUSB (USB 2.0 Peripheral) are
> -- 
> 2.7.4
> 

Re: [PATCH 2/3] phy: renesas: rcar-gen3-usb2: Use pdev's device pointer on dev_vdbg()

[Simon Horman]

On Mon, Apr 01, 2019 at 09:01:22PM +0900, Yoshihiro Shimoda wrote:
> To implement multiple phy instances in the future, this patch uses
> pdev's device pointer on dev_vdbg() instead of the phy's device
> pointer.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

LGTM

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

RE: [PATCH 1/3] dt-bindings: phy: rcar-gen3-phy-usb2: Revise #phy-cells property

[Fabrizio Castro]

Hello Yoshihiro-san,

Thank you for your patch!

> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Yoshihiro Shimoda
> Sent: 01 April 2019 13:01
> Subject: [PATCH 1/3] dt-bindings: phy: rcar-gen3-phy-usb2: Revise #phy-cells property
> 
> To have the detailed property on each PHY specifier, this patch revices

s/revices/revises/g

> the #phy-cells property.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-
> phy-usb2.txt
> index ad9c290..7f0d4bf 100644
> --- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> @@ -27,7 +27,12 @@ Required properties:
> 
>  - reg: offset and length of the partial USB 2.0 Host register block.
>  - clocks: clock phandle and specifier pair(s).
> -- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
> +- #phy-cells: see phy-bindings.txt in the same directory, must be <1>.
> +
> +The phandle's argument in the PHY specifier is the INT_STATUS bit of controller:

From the driver we are also looking for value 0, as in rcar_gen3_get_dr_mode we
have a for loop that starts from 0, and we call of_usb_get_dr_mode_by_phy with
arg0 == 0, why are we leaving this out from this list?

Thanks,
Fab

> +- 1 = USBH_INTA (OHCI)
> +- 2 = USBH_INTB (EHCI)
> +- 3 = UCOM_INT (OTG and BC)
> 
>  Optional properties:
>  To use a USB channel where USB 2.0 Host and HSUSB (USB 2.0 Peripheral) are
> --
> 2.7.4

RE: [PATCH 2/3] phy: renesas: rcar-gen3-usb2: Use pdev's device pointer on dev_vdbg()

[Fabrizio Castro]

Hello Yoshihiro-san,

Thank you for your patch!

> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Yoshihiro Shimoda
> Sent: 01 April 2019 13:01
> Subject: [PATCH 2/3] phy: renesas: rcar-gen3-usb2: Use pdev's device pointer on dev_vdbg()
> 
> To implement multiple phy instances in the future, this patch uses
> pdev's device pointer on dev_vdbg() instead of the phy's device
> pointer.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

> ---
>  drivers/phy/renesas/phy-rcar-gen3-usb2.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index 0a34782..4bdb2ed 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -80,6 +80,7 @@
> 
>  struct rcar_gen3_chan {
>  	void __iomem *base;
> +	struct device *dev;	/* platform_device's device */
>  	struct extcon_dev *extcon;
>  	struct phy *phy;
>  	struct regulator *vbus;
> @@ -120,7 +121,7 @@ static void rcar_gen3_set_host_mode(struct rcar_gen3_chan *ch, int host)
>  	void __iomem *usb2_base = ch->base;
>  	u32 val = readl(usb2_base + USB2_COMMCTRL);
> 
> -	dev_vdbg(&ch->phy->dev, "%s: %08x, %d\n", __func__, val, host);
> +	dev_vdbg(ch->dev, "%s: %08x, %d\n", __func__, val, host);
>  	if (host)
>  		val &= ~USB2_COMMCTRL_OTG_PERI;
>  	else
> @@ -133,7 +134,7 @@ static void rcar_gen3_set_linectrl(struct rcar_gen3_chan *ch, int dp, int dm)
>  	void __iomem *usb2_base = ch->base;
>  	u32 val = readl(usb2_base + USB2_LINECTRL1);
> 
> -	dev_vdbg(&ch->phy->dev, "%s: %08x, %d, %d\n", __func__, val, dp, dm);
> +	dev_vdbg(ch->dev, "%s: %08x, %d, %d\n", __func__, val, dp, dm);
>  	val &= ~(USB2_LINECTRL1_DP_RPD | USB2_LINECTRL1_DM_RPD);
>  	if (dp)
>  		val |= USB2_LINECTRL1_DP_RPD;
> @@ -147,7 +148,7 @@ static void rcar_gen3_enable_vbus_ctrl(struct rcar_gen3_chan *ch, int vbus)
>  	void __iomem *usb2_base = ch->base;
>  	u32 val = readl(usb2_base + USB2_ADPCTRL);
> 
> -	dev_vdbg(&ch->phy->dev, "%s: %08x, %d\n", __func__, val, vbus);
> +	dev_vdbg(ch->dev, "%s: %08x, %d\n", __func__, val, vbus);
>  	if (vbus)
>  		val |= USB2_ADPCTRL_DRVVBUS;
>  	else
> @@ -401,7 +402,7 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
>  	irqreturn_t ret = IRQ_NONE;
> 
>  	if (status & USB2_OBINT_BITS) {
> -		dev_vdbg(&ch->phy->dev, "%s: %08x\n", __func__, status);
> +		dev_vdbg(ch->dev, "%s: %08x\n", __func__, status);
>  		writel(USB2_OBINT_BITS, usb2_base + USB2_OBINTSTA);
>  		rcar_gen3_device_recognition(ch);
>  		ret = IRQ_HANDLED;
> @@ -499,6 +500,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
> 
>  	platform_set_drvdata(pdev, channel);
>  	phy_set_drvdata(channel->phy, channel);
> +	channel->dev = dev;
> 
>  	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>  	if (IS_ERR(provider)) {
> --
> 2.7.4

RE: [PATCH 3/3] phy: renesas: rcar-gen3-usb2: enable/disable independent irqs

[Fabrizio Castro]

Hello Yoshihiro-san,

Thank you for your partch!

> From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Yoshihiro Shimoda
> Sent: 01 April 2019 13:01
> Subject: [PATCH 3/3] phy: renesas: rcar-gen3-usb2: enable/disable independent irqs
> 
> Since the previous code enabled/disabled the irqs both OHCI and EHCI,
> it is possible to cause unexpected interruptions. To avoid this,
> this patch creates multiple phy instances from phandle and
> enables/disables independent irqs by the instances.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Tested-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

> ---
>  drivers/phy/renesas/phy-rcar-gen3-usb2.c | 181 ++++++++++++++++++++++++++-----
>  1 file changed, 156 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index 4bdb2ed..bbe0fe5 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -37,11 +37,8 @@
> 
>  /* INT_ENABLE */
>  #define USB2_INT_ENABLE_UCOM_INTEN	BIT(3)
> -#define USB2_INT_ENABLE_USBH_INTB_EN	BIT(2)
> -#define USB2_INT_ENABLE_USBH_INTA_EN	BIT(1)
> -#define USB2_INT_ENABLE_INIT		(USB2_INT_ENABLE_UCOM_INTEN | \
> -					 USB2_INT_ENABLE_USBH_INTB_EN | \
> -					 USB2_INT_ENABLE_USBH_INTA_EN)
> +#define USB2_INT_ENABLE_USBH_INTB_EN	BIT(2)	/* For EHCI */
> +#define USB2_INT_ENABLE_USBH_INTA_EN	BIT(1)	/* For OHCI */
> 
>  /* USBCTR */
>  #define USB2_USBCTR_DIRPD	BIT(2)
> @@ -78,11 +75,33 @@
>  #define USB2_ADPCTRL_IDPULLUP		BIT(5)	/* 1 = ID sampling is enabled */
>  #define USB2_ADPCTRL_DRVVBUS		BIT(4)
> 
> +#define NUM_OF_PHYS			4
> +#define PHY_INDEX_BOTH_HC		0
> +#define PHY_INDEX_OHCI			1
> +#define PHY_INDEX_EHCI			2
> +#define PHY_INDEX_HSUSB			3
> +
> +static const u32 rcar_gen3_int_enable[NUM_OF_PHYS] = {
> +	USB2_INT_ENABLE_USBH_INTB_EN | USB2_INT_ENABLE_USBH_INTA_EN,
> +	USB2_INT_ENABLE_USBH_INTA_EN,
> +	USB2_INT_ENABLE_USBH_INTB_EN,
> +	0
> +};
> +
> +struct rcar_gen3_phy {
> +	struct phy *phy;
> +	struct rcar_gen3_chan *ch;
> +	u32 int_enable_bits;
> +	bool initialized;
> +	bool otg_initialized;
> +	bool powered;
> +};
> +
>  struct rcar_gen3_chan {
>  	void __iomem *base;
>  	struct device *dev;	/* platform_device's device */
>  	struct extcon_dev *extcon;
> -	struct phy *phy;
> +	struct rcar_gen3_phy rphys[NUM_OF_PHYS];
>  	struct regulator *vbus;
>  	struct work_struct work;
>  	enum usb_dr_mode dr_mode;
> @@ -250,6 +269,42 @@ static enum phy_mode rcar_gen3_get_phy_mode(struct rcar_gen3_chan *ch)
>  	return PHY_MODE_USB_DEVICE;
>  }
> 
> +static bool rcar_gen3_is_any_rphy_initialized(struct rcar_gen3_chan *ch)
> +{
> +	int i;
> +
> +	for (i = 0; i < NUM_OF_PHYS; i++) {
> +		if (ch->rphys[i].initialized)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool rcar_gen3_needs_init_otg(struct rcar_gen3_chan *ch)
> +{
> +	int i;
> +
> +	for (i = 0; i < NUM_OF_PHYS; i++) {
> +		if (ch->rphys[i].otg_initialized)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool rcar_gen3_are_all_rphys_power_off(struct rcar_gen3_chan *ch)
> +{
> +	int i;
> +
> +	for (i = 0; i < NUM_OF_PHYS; i++) {
> +		if (ch->rphys[i].powered)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  static ssize_t role_store(struct device *dev, struct device_attribute *attr,
>  			  const char *buf, size_t count)
>  {
> @@ -257,7 +312,7 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr,
>  	bool is_b_device;
>  	enum phy_mode cur_mode, new_mode;
> 
> -	if (!ch->is_otg_channel || !ch->phy->init_count)
> +	if (!ch->is_otg_channel || !rcar_gen3_is_any_rphy_initialized(ch))
>  		return -EIO;
> 
>  	if (!strncmp(buf, "host", strlen("host")))
> @@ -295,7 +350,7 @@ static ssize_t role_show(struct device *dev, struct device_attribute *attr,
>  {
>  	struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
> 
> -	if (!ch->is_otg_channel || !ch->phy->init_count)
> +	if (!ch->is_otg_channel || !rcar_gen3_is_any_rphy_initialized(ch))
>  		return -EIO;
> 
>  	return sprintf(buf, "%s\n", rcar_gen3_is_host(ch) ? "host" :
> @@ -329,37 +384,62 @@ static void rcar_gen3_init_otg(struct rcar_gen3_chan *ch)
> 
>  static int rcar_gen3_phy_usb2_init(struct phy *p)
>  {
> -	struct rcar_gen3_chan *channel = phy_get_drvdata(p);
> +	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
> +	struct rcar_gen3_chan *channel = rphy->ch;
>  	void __iomem *usb2_base = channel->base;
> +	u32 val;
> 
>  	/* Initialize USB2 part */
> -	writel(USB2_INT_ENABLE_INIT, usb2_base + USB2_INT_ENABLE);
> +	val = readl(usb2_base + USB2_INT_ENABLE);
> +	val |= USB2_INT_ENABLE_UCOM_INTEN | rphy->int_enable_bits;
> +	writel(val, usb2_base + USB2_INT_ENABLE);
>  	writel(USB2_SPD_RSM_TIMSET_INIT, usb2_base + USB2_SPD_RSM_TIMSET);
>  	writel(USB2_OC_TIMSET_INIT, usb2_base + USB2_OC_TIMSET);
> 
>  	/* Initialize otg part */
> -	if (channel->is_otg_channel)
> -		rcar_gen3_init_otg(channel);
> +	if (channel->is_otg_channel) {
> +		if (rcar_gen3_needs_init_otg(channel))
> +			rcar_gen3_init_otg(channel);
> +		rphy->otg_initialized = true;
> +	}
> +
> +	rphy->initialized = true;
> 
>  	return 0;
>  }
> 
>  static int rcar_gen3_phy_usb2_exit(struct phy *p)
>  {
> -	struct rcar_gen3_chan *channel = phy_get_drvdata(p);
> +	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
> +	struct rcar_gen3_chan *channel = rphy->ch;
> +	void __iomem *usb2_base = channel->base;
> +	u32 val;
> 
> -	writel(0, channel->base + USB2_INT_ENABLE);
> +	rphy->initialized = false;
> +
> +	if (channel->is_otg_channel)
> +		rphy->otg_initialized = false;
> +
> +	val = readl(usb2_base + USB2_INT_ENABLE);
> +	val &= ~rphy->int_enable_bits;
> +	if (!rcar_gen3_is_any_rphy_initialized(channel))
> +		val &= ~USB2_INT_ENABLE_UCOM_INTEN;
> +	writel(val, usb2_base + USB2_INT_ENABLE);
> 
>  	return 0;
>  }
> 
>  static int rcar_gen3_phy_usb2_power_on(struct phy *p)
>  {
> -	struct rcar_gen3_chan *channel = phy_get_drvdata(p);
> +	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
> +	struct rcar_gen3_chan *channel = rphy->ch;
>  	void __iomem *usb2_base = channel->base;
>  	u32 val;
>  	int ret;
> 
> +	if (!rcar_gen3_are_all_rphys_power_off(channel))
> +		return 0;
> +
>  	if (channel->vbus) {
>  		ret = regulator_enable(channel->vbus);
>  		if (ret)
> @@ -372,14 +452,22 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
>  	val &= ~USB2_USBCTR_PLL_RST;
>  	writel(val, usb2_base + USB2_USBCTR);
> 
> +	rphy->powered = true;
> +
>  	return 0;
>  }
> 
>  static int rcar_gen3_phy_usb2_power_off(struct phy *p)
>  {
> -	struct rcar_gen3_chan *channel = phy_get_drvdata(p);
> +	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
> +	struct rcar_gen3_chan *channel = rphy->ch;
>  	int ret = 0;
> 
> +	rphy->powered = false;
> +
> +	if (!rcar_gen3_are_all_rphys_power_off(channel))
> +		return 0;
> +
>  	if (channel->vbus)
>  		ret = regulator_disable(channel->vbus);
> 
> @@ -426,13 +514,51 @@ static const unsigned int rcar_gen3_phy_cable[] = {
>  	EXTCON_NONE,
>  };
> 
> +static struct phy *rcar_gen3_phy_usb2_xlate(struct device *dev,
> +					    struct of_phandle_args *args)
> +{
> +	struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
> +
> +	if (args->args_count == 0)	/* For old version dts */
> +		return ch->rphys[PHY_INDEX_BOTH_HC].phy;
> +	else if (args->args_count > 1)	/* Prevent invalid args count */
> +		return ERR_PTR(-ENODEV);
> +
> +	if (args->args[0] >= NUM_OF_PHYS)
> +		return ERR_PTR(-ENODEV);
> +
> +	return ch->rphys[args->args[0]].phy;
> +}
> +
> +static enum usb_dr_mode rcar_gen3_get_dr_mode(struct device_node *np)
> +{
> +	enum usb_dr_mode candidate = USB_DR_MODE_UNKNOWN, tmp;
> +	int i;
> +
> +	/*
> +	 * If one of device nodes has other dr_mode except UNKNOWN,
> +	 * this function returns UNKNOWN.
> +	 */
> +	for (i = 0; i < NUM_OF_PHYS; i++) {
> +		tmp = of_usb_get_dr_mode_by_phy(np, i);

We are calling of_usb_get_dr_mode_by_phy with i == 0, but we don't document it
in the dt-bindings document?

> +		if (tmp != USB_DR_MODE_UNKNOWN) {
> +			if (candidate == USB_DR_MODE_UNKNOWN)
> +				candidate = tmp;
> +			else if (candidate != tmp)
> +				return USB_DR_MODE_UNKNOWN;
> +		}
> +	}
> +
> +	return candidate;
> +}
> +
>  static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct rcar_gen3_chan *channel;
>  	struct phy_provider *provider;
>  	struct resource *res;
> -	int irq, ret = 0;
> +	int irq, ret = 0, i;
> 
>  	if (!dev->of_node) {
>  		dev_err(dev, "This driver needs device tree\n");
> @@ -458,7 +584,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>  			dev_err(dev, "No irq handler (%d)\n", irq);
>  	}
> 
> -	channel->dr_mode = of_usb_get_dr_mode_by_phy(dev->of_node, 0);
> +	channel->dr_mode = rcar_gen3_get_dr_mode(dev->of_node);
>  	if (channel->dr_mode != USB_DR_MODE_UNKNOWN) {
>  		int ret;
> 
> @@ -482,11 +608,17 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>  	 * And then, phy-core will manage runtime pm for this device.
>  	 */
>  	pm_runtime_enable(dev);
> -	channel->phy = devm_phy_create(dev, NULL, &rcar_gen3_phy_usb2_ops);
> -	if (IS_ERR(channel->phy)) {
> -		dev_err(dev, "Failed to create USB2 PHY\n");
> -		ret = PTR_ERR(channel->phy);
> -		goto error;
> +	for (i = 0; i < NUM_OF_PHYS; i++) {
> +		channel->rphys[i].phy = devm_phy_create(dev, NULL,
> +						  &rcar_gen3_phy_usb2_ops);
> +		if (IS_ERR(channel->rphys[i].phy)) {
> +			dev_err(dev, "Failed to create USB2 PHY\n");
> +			ret = PTR_ERR(channel->rphys[i].phy);
> +			goto error;
> +		}
> +		channel->rphys[i].ch = channel;
> +		channel->rphys[i].int_enable_bits = rcar_gen3_int_enable[i];
> +		phy_set_drvdata(channel->rphys[i].phy, &channel->rphys[i]);
>  	}
> 
>  	channel->vbus = devm_regulator_get_optional(dev, "vbus");
> @@ -499,10 +631,9 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>  	}
> 
>  	platform_set_drvdata(pdev, channel);
> -	phy_set_drvdata(channel->phy, channel);
>  	channel->dev = dev;
> 
> -	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	provider = devm_of_phy_provider_register(dev, rcar_gen3_phy_usb2_xlate);
>  	if (IS_ERR(provider)) {
>  		dev_err(dev, "Failed to register PHY provider\n");
>  		ret = PTR_ERR(provider);
> --
> 2.7.4

RE: [PATCH 1/3] dt-bindings: phy: rcar-gen3-phy-usb2: Revise #phy-cells property

[Yoshihiro Shimoda]

Hi Simon-san,

Thank you for your review!

> From: Simon Horman, Sent: Wednesday, April 3, 2019 6:21 PM
> 
> Hi Shimoda-san,
> 
> On Mon, Apr 01, 2019 at 09:01:21PM +0900, Yoshihiro Shimoda wrote:
> > To have the detailed property on each PHY specifier, this patch revices
> > the #phy-cells property.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> > index ad9c290..7f0d4bf 100644
> > --- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> > +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> > @@ -27,7 +27,12 @@ Required properties:
> >
> >  - reg: offset and length of the partial USB 2.0 Host register block.
> >  - clocks: clock phandle and specifier pair(s).
> > -- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
> 
> The driver currently supports <0> and with this changeset continues to do
> so. I also assume there are existing DTs that use <0>. Would it be
> better to describe this as deprecated?

I got it. I'll fix this in v2.

Best regards,
Yoshihiro Shimoda


> > +- #phy-cells: see phy-bindings.txt in the same directory, must be <1>.
> > +
> > +The phandle's argument in the PHY specifier is the INT_STATUS bit of controller:
> > +- 1 = USBH_INTA (OHCI)
> > +- 2 = USBH_INTB (EHCI)
> > +- 3 = UCOM_INT (OTG and BC)
> >
> >  Optional properties:
> >  To use a USB channel where USB 2.0 Host and HSUSB (USB 2.0 Peripheral) are
> > --
> > 2.7.4
> >

RE: [PATCH 1/3] dt-bindings: phy: rcar-gen3-phy-usb2: Revise #phy-cells property

[Yoshihiro Shimoda]

Hi Fabrizio-san,

Thank you for your review!

> From: Fabrizio Castro, Sent: Wednesday, April 3, 2019 7:35 PM
> 
> Hello Yoshihiro-san,
> 
> Thank you for your patch!
> 
> > From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf Of Yoshihiro Shimoda
> > Sent: 01 April 2019 13:01
> > Subject: [PATCH 1/3] dt-bindings: phy: rcar-gen3-phy-usb2: Revise #phy-cells property
> >
> > To have the detailed property on each PHY specifier, this patch revices
> 
> s/revices/revises/g

Oops! I'll fix it in v2.

> > the #phy-cells property.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> b/Documentation/devicetree/bindings/phy/rcar-gen3-
> > phy-usb2.txt
> > index ad9c290..7f0d4bf 100644
> > --- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> > +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> > @@ -27,7 +27,12 @@ Required properties:
> >
> >  - reg: offset and length of the partial USB 2.0 Host register block.
> >  - clocks: clock phandle and specifier pair(s).
> > -- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
> > +- #phy-cells: see phy-bindings.txt in the same directory, must be <1>.
> > +
> > +The phandle's argument in the PHY specifier is the INT_STATUS bit of controller:
> 
> From the driver we are also looking for value 0, as in rcar_gen3_get_dr_mode we
> have a for loop that starts from 0, and we call of_usb_get_dr_mode_by_phy with
> arg0 == 0

This is backward compatibility for old DTS file (phy-cells = 0).

> why are we leaving this out from this list?

This is because I don't assume any device nodes doesn't have such a property
(the bit 0 of INT_STATUS).

Best regards,
Yoshihiro Shimoda

> Thanks,
> Fab
> 
> > +- 1 = USBH_INTA (OHCI)
> > +- 2 = USBH_INTB (EHCI)
> > +- 3 = UCOM_INT (OTG and BC)
> >
> >  Optional properties:
> >  To use a USB channel where USB 2.0 Host and HSUSB (USB 2.0 Peripheral) are
> > --
> > 2.7.4

RE: [PATCH 3/3] phy: renesas: rcar-gen3-usb2: enable/disable independent irqs

[Yoshihiro Shimoda]

Hi Simon-san,

> From: Simon Horman, Sent: Wednesday, April 3, 2019 6:20 PM
> 
> Hi Shimoda-san,
> 
> On Mon, Apr 01, 2019 at 09:01:23PM +0900, Yoshihiro Shimoda wrote:
> > Since the previous code enabled/disabled the irqs both OHCI and EHCI,
> > it is possible to cause unexpected interruptions. To avoid this,
> > this patch creates multiple phy instances from phandle and
> > enables/disables independent irqs by the instances.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> I noted a few nits below but overall this looks good to me.
> 
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Thank you for your review!

<snip>
> > +#define NUM_OF_PHYS			4
> > +#define PHY_INDEX_BOTH_HC		0
> > +#define PHY_INDEX_OHCI			1
> > +#define PHY_INDEX_EHCI			2
> > +#define PHY_INDEX_HSUSB			3
> 
> nit: I think the above #defines would be better expressed as an enum.

I'll modify it.

<snip>
> > +static enum usb_dr_mode rcar_gen3_get_dr_mode(struct device_node *np)
> > +{
> > +	enum usb_dr_mode candidate = USB_DR_MODE_UNKNOWN, tmp;
> 
> nit: I think that there could be a better name for tmp, f.e. mode
> nit: The scope of tmp could be limited to inside the for loop.

I'll modify it.

Best regards,
Yoshihiro Shimoda

RE: [PATCH 3/3] phy: renesas: rcar-gen3-usb2: enable/disable independent irqs

[Yoshihiro Shimoda]

Hello Fabrizio-san,

> From: Fabrizio Castro, Sent: Wednesday, April 3, 2019 7:49 PM
> 
> Hello Yoshihiro-san,
> 
> Thank you for your partch!
> 
> > From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Yoshihiro Shimoda
> > Sent: 01 April 2019 13:01
> > Subject: [PATCH 3/3] phy: renesas: rcar-gen3-usb2: enable/disable independent irqs
> >
> > Since the previous code enabled/disabled the irqs both OHCI and EHCI,
> > it is possible to cause unexpected interruptions. To avoid this,
> > this patch creates multiple phy instances from phandle and
> > enables/disables independent irqs by the instances.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Reviewed-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Tested-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

Thank you for your review and test!

<snip>
> > +static enum usb_dr_mode rcar_gen3_get_dr_mode(struct device_node *np)
> > +{
> > +	enum usb_dr_mode candidate = USB_DR_MODE_UNKNOWN, tmp;
> > +	int i;
> > +
> > +	/*
> > +	 * If one of device nodes has other dr_mode except UNKNOWN,
> > +	 * this function returns UNKNOWN.
> > +	 */
> > +	for (i = 0; i < NUM_OF_PHYS; i++) {
> > +		tmp = of_usb_get_dr_mode_by_phy(np, i);
> 
> We are calling of_usb_get_dr_mode_by_phy with i == 0, but we don't document it
> in the dt-bindings document?

This (i == 0) is for backward compatibility. So, I'll add such a comment on the driver.

Best regards,
Yoshihiro Shimoda