[PATCH 0/6] Chipidea USB driver Enhancements and Bug Fixes

[PATCH 0/6] Chipidea USB driver Enhancements and Bug Fixes

[Manish Narani]

This patch series has some Enhancements and Bug Fixes to chipidea USB
driver related to Zynq platform.

Manish Narani (3):
  usb: chipidea: Add support for VBUS control with PHY
  usb: chipidea: Use usb2 phy for Zynq platform
  usb: chipidea: Check usb_phy exists before using it

Piyush Mehta (3):
  usb: chipidea: Fix return value handling
  usb: chipidea: core: Add return value function check
  usb: chipidea: udc: Add xilinx revision support

 drivers/usb/chipidea/ci_hdrc_usb2.c |  5 +++++
 drivers/usb/chipidea/core.c         | 26 +++++++++++++++++++-------
 drivers/usb/chipidea/host.c         |  9 +++++++++
 drivers/usb/chipidea/otg.c          |  6 +++++-
 drivers/usb/chipidea/otg_fsm.c      |  9 +++++++++
 drivers/usb/chipidea/udc.c          |  3 ++-
 include/linux/usb/chipidea.h        |  1 +
 7 files changed, 50 insertions(+), 9 deletions(-)

-- 
2.1.1

[PATCH 1/6] usb: chipidea: Add support for VBUS control with PHY

[Manish Narani]

Some platforms make use of VBUS control over PHY which
means controller driver has to access PHY registers to
turn on/off VBUS line.This patch adds support for
such platforms in chipidea.

Signed-off-by: Subbaraya Sundeep Bhatta <sbhatta@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/usb/chipidea/ci_hdrc_usb2.c | 1 +
 drivers/usb/chipidea/host.c         | 9 +++++++++
 drivers/usb/chipidea/otg_fsm.c      | 7 +++++++
 include/linux/usb/chipidea.h        | 1 +
 4 files changed, 18 insertions(+)

diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c b/drivers/usb/chipidea/ci_hdrc_usb2.c
index 89e1d82..dc86b12 100644
--- a/drivers/usb/chipidea/ci_hdrc_usb2.c
+++ b/drivers/usb/chipidea/ci_hdrc_usb2.c
@@ -30,6 +30,7 @@ static const struct ci_hdrc_platform_data ci_default_pdata = {
 
 static const struct ci_hdrc_platform_data ci_zynq_pdata = {
 	.capoffset	= DEF_CAPOFFSET,
+	.flags          = CI_HDRC_PHY_VBUS_CONTROL,
 };
 
 static const struct ci_hdrc_platform_data ci_zevio_pdata = {
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index e86d13c..578968d 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -63,6 +63,14 @@ static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
 		priv->enabled = enable;
 	}
 
+	if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL &&
+	    ci->usb_phy && ci->usb_phy->set_vbus) {
+		if (enable)
+			ci->usb_phy->set_vbus(ci->usb_phy, 1);
+		else
+			ci->usb_phy->set_vbus(ci->usb_phy, 0);
+	}
+
 	if (enable && (ci->platdata->phy_mode == USBPHY_INTERFACE_MODE_HSIC)) {
 		/*
 		 * Marvell 28nm HSIC PHY requires forcing the port to HS mode.
@@ -71,6 +79,7 @@ static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
 		hw_port_test_set(ci, 5);
 		hw_port_test_set(ci, 0);
 	}
+
 	return 0;
 };
 
diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
index 6ed4b00..2f7f94d 100644
--- a/drivers/usb/chipidea/otg_fsm.c
+++ b/drivers/usb/chipidea/otg_fsm.c
@@ -471,6 +471,10 @@ static void ci_otg_drv_vbus(struct otg_fsm *fsm, int on)
 				return;
 			}
 		}
+
+		if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL)
+			ci->usb_phy->set_vbus(ci->usb_phy, 1);
+
 		/* Disable data pulse irq */
 		hw_write_otgsc(ci, OTGSC_DPIE, 0);
 
@@ -480,6 +484,9 @@ static void ci_otg_drv_vbus(struct otg_fsm *fsm, int on)
 		if (ci->platdata->reg_vbus)
 			regulator_disable(ci->platdata->reg_vbus);
 
+		if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL)
+			ci->usb_phy->set_vbus(ci->usb_phy, 0);
+
 		fsm->a_bus_drop = 1;
 		fsm->a_bus_req = 0;
 	}
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index edf3342..ee38835 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -62,6 +62,7 @@ struct ci_hdrc_platform_data {
 #define CI_HDRC_REQUIRES_ALIGNED_DMA	BIT(13)
 #define CI_HDRC_IMX_IS_HSIC		BIT(14)
 #define CI_HDRC_PMQOS			BIT(15)
+#define CI_HDRC_PHY_VBUS_CONTROL	BIT(16)
 	enum usb_dr_mode	dr_mode;
 #define CI_HDRC_CONTROLLER_RESET_EVENT		0
 #define CI_HDRC_CONTROLLER_STOPPED_EVENT	1
-- 
2.1.1

[PATCH 2/6] usb: chipidea: Use usb2 phy for Zynq platform

[Manish Narani]

Add usb2 phy to Zynq platform data.

Signed-off-by: Subbaraya Sundeep Bhatta <sbhatta@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/usb/chipidea/ci_hdrc_usb2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c b/drivers/usb/chipidea/ci_hdrc_usb2.c
index dc86b12..be58869 100644
--- a/drivers/usb/chipidea/ci_hdrc_usb2.c
+++ b/drivers/usb/chipidea/ci_hdrc_usb2.c
@@ -65,6 +65,10 @@ static int ci_hdrc_usb2_probe(struct platform_device *pdev)
 	if (match && match->data) {
 		/* struct copy */
 		*ci_pdata = *(struct ci_hdrc_platform_data *)match->data;
+		ci_pdata->usb_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy",
+								0);
+		if (IS_ERR(ci_pdata->usb_phy))
+			return PTR_ERR(ci_pdata->usb_phy);
 	}
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-- 
2.1.1

[PATCH 3/6] usb: chipidea: Check usb_phy exists before using it

[Manish Narani]

usb_phy and usb_phy->set_vbus may not be present all the times
based on PHY driver used. So check for it.

Signed-off-by: Subbaraya Sundeep Bhatta <sbhatta@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/usb/chipidea/otg_fsm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
index 2f7f94d..5f8f5d2 100644
--- a/drivers/usb/chipidea/otg_fsm.c
+++ b/drivers/usb/chipidea/otg_fsm.c
@@ -472,7 +472,8 @@ static void ci_otg_drv_vbus(struct otg_fsm *fsm, int on)
 			}
 		}
 
-		if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL)
+		if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL &&
+		    ci->usb_phy && ci->usb_phy->set_vbus)
 			ci->usb_phy->set_vbus(ci->usb_phy, 1);
 
 		/* Disable data pulse irq */
@@ -484,7 +485,8 @@ static void ci_otg_drv_vbus(struct otg_fsm *fsm, int on)
 		if (ci->platdata->reg_vbus)
 			regulator_disable(ci->platdata->reg_vbus);
 
-		if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL)
+		if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL &&
+		    ci->usb_phy && ci->usb_phy->set_vbus)
 			ci->usb_phy->set_vbus(ci->usb_phy, 0);
 
 		fsm->a_bus_drop = 1;
-- 
2.1.1

[PATCH 4/6] usb: chipidea: Fix return value handling

[Manish Narani]

From: Piyush Mehta <piyush.mehta@xilinx.com>

API was neither captured nor checked.Fixed it by capturing the
return value and then checking for any error.

Addresses-Coverity: "Calling without checking return"
Addresses-Coverity: "CHECKED_RETURN"
Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/usb/chipidea/core.c | 11 +++++++----
 drivers/usb/chipidea/otg.c  |  6 +++++-
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 2b18f50..676346f 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -700,13 +700,16 @@ static int ci_get_platdata(struct device *dev,
 	if (usb_get_maximum_speed(dev) == USB_SPEED_FULL)
 		platdata->flags |= CI_HDRC_FORCE_FULLSPEED;
 
-	of_property_read_u32(dev->of_node, "phy-clkgate-delay-us",
-				     &platdata->phy_clkgate_delay_us);
+	if (of_property_read_u32(dev->of_node, "phy-clkgate-delay-us",
+				 &platdata->phy_clkgate_delay_us))
+		dev_dbg(dev, "Missing phy-clkgate-delay-us property\n");
 
 	platdata->itc_setting = 1;
 
-	of_property_read_u32(dev->of_node, "itc-setting",
-					&platdata->itc_setting);
+	if (of_property_read_u32(dev->of_node, "itc-setting",
+				 &platdata->itc_setting))
+		dev_dbg(dev, "Missing itc-setting property\n");
+
 
 	ret = of_property_read_u32(dev->of_node, "ahb-burst-config",
 				&platdata->ahb_burst_config);
diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
index 8dd5928..d527d9d 100644
--- a/drivers/usb/chipidea/otg.c
+++ b/drivers/usb/chipidea/otg.c
@@ -168,6 +168,7 @@ static int hw_wait_vbus_lower_bsv(struct ci_hdrc *ci)
 static void ci_handle_id_switch(struct ci_hdrc *ci)
 {
 	enum ci_role role = ci_otg_role(ci);
+	int ret;
 
 	if (role != ci->role) {
 		dev_dbg(ci->dev, "switching from %s to %s\n",
@@ -193,7 +194,10 @@ static void ci_handle_id_switch(struct ci_hdrc *ci)
 			 */
 			hw_wait_vbus_lower_bsv(ci);
 
-		ci_role_start(ci, role);
+		ret = ci_role_start(ci, role);
+		if (ret < 0)
+			dev_dbg(ci->dev, "switching err %d\n", ret);
+
 		/* vbus change may have already occurred */
 		if (role == CI_ROLE_GADGET)
 			ci_handle_vbus_change(ci);
-- 
2.1.1

[PATCH 5/6] usb: chipidea: core: Add return value function check

[Manish Narani]

From: Piyush Mehta <piyush.mehta@xilinx.com>

Add return value validation for function phy exit and phy power off.

Addresses-Coverity: "USELESS_CALL"
Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/usb/chipidea/core.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 676346f..37f619e 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -324,7 +324,8 @@ static int _ci_usb_phy_init(struct ci_hdrc *ci)
 
 		ret = phy_power_on(ci->phy);
 		if (ret) {
-			phy_exit(ci->phy);
+			if (phy_exit(ci->phy) < 0)
+				dev_dbg(ci->dev, "phy exit failed\r\n");
 			return ret;
 		}
 	} else {
@@ -341,12 +342,20 @@ static int _ci_usb_phy_init(struct ci_hdrc *ci)
  */
 static void ci_usb_phy_exit(struct ci_hdrc *ci)
 {
+	int ret;
+
 	if (ci->platdata->flags & CI_HDRC_OVERRIDE_PHY_CONTROL)
 		return;
 
 	if (ci->phy) {
-		phy_power_off(ci->phy);
-		phy_exit(ci->phy);
+		ret = phy_power_off(ci->phy);
+		if (ret < 0)
+			dev_dbg(ci->dev, "phy poweroff failed\r\n");
+
+		ret = phy_exit(ci->phy);
+		if (ret < 0)
+			dev_dbg(ci->dev, "phy exit failed\r\n");
+
 	} else {
 		usb_phy_shutdown(ci->usb_phy);
 	}
-- 
2.1.1

[PATCH 6/6] usb: chipidea: udc: Add xilinx revision support

[Manish Narani]

From: Piyush Mehta <piyush.mehta@xilinx.com>

Issue: Adding a dTD to a Primed Endpoint May Not Get Recognized with
revision 2.20a.

There is an issue with the add dTD tripwire semaphore (ATDTW bit in
USBCMD register) that can cause the controller to ignore a dTD that is
added to a primed endpoint. When this happens, the software can read
the tripwire bit and the status bit at '1' even though the endpoint is
unprimed.
This issue observed with the Windows host machine.

Workaround:
The software must implement a periodic cycle, and check for each dTD
pending on execution (Active = 1), if the endpoint is primed. It can do
this by reading the corresponding bits in the ENDPTPRIME and ENDPTSTAT
registers. If these bits are read at 0, the software needs to re-prime
the endpoint by writing 1 to the corresponding bit in the ENDPTPRIME
register.

Added conditional revision check of 2.20[CI_REVISION_22] along with 2.40.

Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/usb/chipidea/udc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 8834ca6..b440205 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -680,7 +680,8 @@ static int _hardware_dequeue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq)
 		if ((TD_STATUS_ACTIVE & tmptoken) != 0) {
 			int n = hw_ep_bit(hwep->num, hwep->dir);
 
-			if (ci->rev == CI_REVISION_24)
+			if (ci->rev == CI_REVISION_24 ||
+			    ci->rev == CI_REVISION_22)
 				if (!hw_read(ci, OP_ENDPTSTAT, BIT(n)))
 					reprime_dtd(ci, hwep, node);
 			hwreq->req.status = -EALREADY;
-- 
2.1.1

Re: [PATCH 1/6] usb: chipidea: Add support for VBUS control with PHY

[Peter Chen]

On 21-08-24 22:46:13, Manish Narani wrote:
> Some platforms make use of VBUS control over PHY which
> means controller driver has to access PHY registers to
> turn on/off VBUS line.This patch adds support for
> such platforms in chipidea.
> 
> Signed-off-by: Subbaraya Sundeep Bhatta <sbhatta@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  drivers/usb/chipidea/ci_hdrc_usb2.c | 1 +
>  drivers/usb/chipidea/host.c         | 9 +++++++++
>  drivers/usb/chipidea/otg_fsm.c      | 7 +++++++
>  include/linux/usb/chipidea.h        | 1 +
>  4 files changed, 18 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c b/drivers/usb/chipidea/ci_hdrc_usb2.c
> index 89e1d82..dc86b12 100644
> --- a/drivers/usb/chipidea/ci_hdrc_usb2.c
> +++ b/drivers/usb/chipidea/ci_hdrc_usb2.c
> @@ -30,6 +30,7 @@ static const struct ci_hdrc_platform_data ci_default_pdata = {
>  
>  static const struct ci_hdrc_platform_data ci_zynq_pdata = {
>  	.capoffset	= DEF_CAPOFFSET,
> +	.flags          = CI_HDRC_PHY_VBUS_CONTROL,
>  };
>  
>  static const struct ci_hdrc_platform_data ci_zevio_pdata = {
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index e86d13c..578968d 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -63,6 +63,14 @@ static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
>  		priv->enabled = enable;
>  	}
>  
> +	if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL &&
> +	    ci->usb_phy && ci->usb_phy->set_vbus) {

Indent is needed. Otherwise:

Acked-by: Peter Chen <peter.chen@kernel.org>

Peter

> +		if (enable)
> +			ci->usb_phy->set_vbus(ci->usb_phy, 1);
> +		else
> +			ci->usb_phy->set_vbus(ci->usb_phy, 0);
> +	}
> +
>  	if (enable && (ci->platdata->phy_mode == USBPHY_INTERFACE_MODE_HSIC)) {
>  		/*
>  		 * Marvell 28nm HSIC PHY requires forcing the port to HS mode.
> @@ -71,6 +79,7 @@ static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
>  		hw_port_test_set(ci, 5);
>  		hw_port_test_set(ci, 0);
>  	}
> +
>  	return 0;
>  };
>  
> diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
> index 6ed4b00..2f7f94d 100644
> --- a/drivers/usb/chipidea/otg_fsm.c
> +++ b/drivers/usb/chipidea/otg_fsm.c
> @@ -471,6 +471,10 @@ static void ci_otg_drv_vbus(struct otg_fsm *fsm, int on)
>  				return;
>  			}
>  		}
> +
> +		if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL)
> +			ci->usb_phy->set_vbus(ci->usb_phy, 1);
> +
>  		/* Disable data pulse irq */
>  		hw_write_otgsc(ci, OTGSC_DPIE, 0);
>  
> @@ -480,6 +484,9 @@ static void ci_otg_drv_vbus(struct otg_fsm *fsm, int on)
>  		if (ci->platdata->reg_vbus)
>  			regulator_disable(ci->platdata->reg_vbus);
>  
> +		if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL)
> +			ci->usb_phy->set_vbus(ci->usb_phy, 0);
> +
>  		fsm->a_bus_drop = 1;
>  		fsm->a_bus_req = 0;
>  	}
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index edf3342..ee38835 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -62,6 +62,7 @@ struct ci_hdrc_platform_data {
>  #define CI_HDRC_REQUIRES_ALIGNED_DMA	BIT(13)
>  #define CI_HDRC_IMX_IS_HSIC		BIT(14)
>  #define CI_HDRC_PMQOS			BIT(15)
> +#define CI_HDRC_PHY_VBUS_CONTROL	BIT(16)
>  	enum usb_dr_mode	dr_mode;
>  #define CI_HDRC_CONTROLLER_RESET_EVENT		0
>  #define CI_HDRC_CONTROLLER_STOPPED_EVENT	1
> -- 
> 2.1.1
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 2/6] usb: chipidea: Use usb2 phy for Zynq platform

[Peter Chen]

On 21-08-24 22:46:14, Manish Narani wrote:
> Add usb2 phy to Zynq platform data.

Please change the subject and comments a little. This file is not only
for xilinx platform, it is for generic chipidea driver. If you would
like this change only for your platform, you may use of_device_id.data
for condition.

Peter

> 
> Signed-off-by: Subbaraya Sundeep Bhatta <sbhatta@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  drivers/usb/chipidea/ci_hdrc_usb2.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c b/drivers/usb/chipidea/ci_hdrc_usb2.c
> index dc86b12..be58869 100644
> --- a/drivers/usb/chipidea/ci_hdrc_usb2.c
> +++ b/drivers/usb/chipidea/ci_hdrc_usb2.c
> @@ -65,6 +65,10 @@ static int ci_hdrc_usb2_probe(struct platform_device *pdev)
>  	if (match && match->data) {
>  		/* struct copy */
>  		*ci_pdata = *(struct ci_hdrc_platform_data *)match->data;
> +		ci_pdata->usb_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy",
> +								0);
> +		if (IS_ERR(ci_pdata->usb_phy))
> +			return PTR_ERR(ci_pdata->usb_phy);
>  	}
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> -- 
> 2.1.1
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 3/6] usb: chipidea: Check usb_phy exists before using it

[Peter Chen]

On 21-08-24 22:46:15, Manish Narani wrote:
> usb_phy and usb_phy->set_vbus may not be present all the times
> based on PHY driver used. So check for it.

Please squash this one with the first one.

Peter
> 
> Signed-off-by: Subbaraya Sundeep Bhatta <sbhatta@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  drivers/usb/chipidea/otg_fsm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
> index 2f7f94d..5f8f5d2 100644
> --- a/drivers/usb/chipidea/otg_fsm.c
> +++ b/drivers/usb/chipidea/otg_fsm.c
> @@ -472,7 +472,8 @@ static void ci_otg_drv_vbus(struct otg_fsm *fsm, int on)
>  			}
>  		}
>  
> -		if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL)
> +		if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL &&
> +		    ci->usb_phy && ci->usb_phy->set_vbus)
>  			ci->usb_phy->set_vbus(ci->usb_phy, 1);
>  
>  		/* Disable data pulse irq */
> @@ -484,7 +485,8 @@ static void ci_otg_drv_vbus(struct otg_fsm *fsm, int on)
>  		if (ci->platdata->reg_vbus)
>  			regulator_disable(ci->platdata->reg_vbus);
>  
> -		if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL)
> +		if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL &&
> +		    ci->usb_phy && ci->usb_phy->set_vbus)
>  			ci->usb_phy->set_vbus(ci->usb_phy, 0);
>  
>  		fsm->a_bus_drop = 1;
> -- 
> 2.1.1
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 5/6] usb: chipidea: core: Add return value function check

[Peter Chen]

On 21-08-24 22:46:17, Manish Narani wrote:
> From: Piyush Mehta <piyush.mehta@xilinx.com>
> 
> Add return value validation for function phy exit and phy power off.
> 
> Addresses-Coverity: "USELESS_CALL"
> Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  drivers/usb/chipidea/core.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 676346f..37f619e 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -324,7 +324,8 @@ static int _ci_usb_phy_init(struct ci_hdrc *ci)
>  
>  		ret = phy_power_on(ci->phy);
>  		if (ret) {
> -			phy_exit(ci->phy);
> +			if (phy_exit(ci->phy) < 0)
> +				dev_dbg(ci->dev, "phy exit failed\r\n");

You may not need to debug message, the phy_exit shows error message.
You may return 'ret' for phy_exit for error.

>  			return ret;
>  		}
>  	} else {
> @@ -341,12 +342,20 @@ static int _ci_usb_phy_init(struct ci_hdrc *ci)
>   */
>  static void ci_usb_phy_exit(struct ci_hdrc *ci)
>  {
> +	int ret;
> +
>  	if (ci->platdata->flags & CI_HDRC_OVERRIDE_PHY_CONTROL)
>  		return;
>  
>  	if (ci->phy) {
> -		phy_power_off(ci->phy);
> -		phy_exit(ci->phy);
> +		ret = phy_power_off(ci->phy);
> +		if (ret < 0)
> +			dev_dbg(ci->dev, "phy poweroff failed\r\n");
> +
> +		ret = phy_exit(ci->phy);
> +		if (ret < 0)
> +			dev_dbg(ci->dev, "phy exit failed\r\n");
> +

Ditto

>  	} else {
>  		usb_phy_shutdown(ci->usb_phy);
>  	}
> -- 
> 2.1.1
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 6/6] usb: chipidea: udc: Add xilinx revision support

[Peter Chen]

On 21-08-24 22:46:18, Manish Narani wrote:
> From: Piyush Mehta <piyush.mehta@xilinx.com>
> 
> Issue: Adding a dTD to a Primed Endpoint May Not Get Recognized with
> revision 2.20a.
> 
> There is an issue with the add dTD tripwire semaphore (ATDTW bit in
> USBCMD register) that can cause the controller to ignore a dTD that is
> added to a primed endpoint. When this happens, the software can read
> the tripwire bit and the status bit at '1' even though the endpoint is
> unprimed.
> This issue observed with the Windows host machine.
> 
> Workaround:
> The software must implement a periodic cycle, and check for each dTD
> pending on execution (Active = 1), if the endpoint is primed. It can do
> this by reading the corresponding bits in the ENDPTPRIME and ENDPTSTAT
> registers. If these bits are read at 0, the software needs to re-prime
> the endpoint by writing 1 to the corresponding bit in the ENDPTPRIME
> register.
> 
> Added conditional revision check of 2.20[CI_REVISION_22] along with 2.40.
> 
> Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  drivers/usb/chipidea/udc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 8834ca6..b440205 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -680,7 +680,8 @@ static int _hardware_dequeue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq)
>  		if ((TD_STATUS_ACTIVE & tmptoken) != 0) {
>  			int n = hw_ep_bit(hwep->num, hwep->dir);
>  
> -			if (ci->rev == CI_REVISION_24)
> +			if (ci->rev == CI_REVISION_24 ||
> +			    ci->rev == CI_REVISION_22)

Add indent for it, otherwise:

Acked-by: Peter Chen <peter.chen@kernel.org>

>  				if (!hw_read(ci, OP_ENDPTSTAT, BIT(n)))
>  					reprime_dtd(ci, hwep, node);
>  			hwreq->req.status = -EALREADY;
> -- 
> 2.1.1
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 6/6] usb: chipidea: udc: Add xilinx revision support

[Peter Chen]

On 21-08-24 22:46:18, Manish Narani wrote:
> From: Piyush Mehta <piyush.mehta@xilinx.com>
> 
> Issue: Adding a dTD to a Primed Endpoint May Not Get Recognized with
> revision 2.20a.
> 
> There is an issue with the add dTD tripwire semaphore (ATDTW bit in
> USBCMD register) that can cause the controller to ignore a dTD that is
> added to a primed endpoint. When this happens, the software can read
> the tripwire bit and the status bit at '1' even though the endpoint is
> unprimed.
> This issue observed with the Windows host machine.
> 
> Workaround:
> The software must implement a periodic cycle, and check for each dTD
> pending on execution (Active = 1), if the endpoint is primed. It can do
> this by reading the corresponding bits in the ENDPTPRIME and ENDPTSTAT
> registers. If these bits are read at 0, the software needs to re-prime
> the endpoint by writing 1 to the corresponding bit in the ENDPTPRIME
> register.
> 
> Added conditional revision check of 2.20[CI_REVISION_22] along with 2.40.
> 
> Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  drivers/usb/chipidea/udc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 8834ca6..b440205 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -680,7 +680,8 @@ static int _hardware_dequeue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq)
>  		if ((TD_STATUS_ACTIVE & tmptoken) != 0) {
>  			int n = hw_ep_bit(hwep->num, hwep->dir);
>  
> -			if (ci->rev == CI_REVISION_24)
> +			if (ci->rev == CI_REVISION_24 ||
> +			    ci->rev == CI_REVISION_22)

Besides, please change subject a little, it is for specific IP version,
but not related to SoC platforms.

>  				if (!hw_read(ci, OP_ENDPTSTAT, BIT(n)))
>  					reprime_dtd(ci, hwep, node);
>  			hwreq->req.status = -EALREADY;
> -- 
> 2.1.1
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 0/6] Chipidea USB driver Enhancements and Bug Fixes

[Peter Chen]

On 21-08-24 22:46:12, Manish Narani wrote:
> This patch series has some Enhancements and Bug Fixes to chipidea USB
> driver related to Zynq platform.
> 
> Manish Narani (3):
>   usb: chipidea: Add support for VBUS control with PHY
>   usb: chipidea: Use usb2 phy for Zynq platform
>   usb: chipidea: Check usb_phy exists before using it
> 
> Piyush Mehta (3):
>   usb: chipidea: Fix return value handling
>   usb: chipidea: core: Add return value function check
>   usb: chipidea: udc: Add xilinx revision support
> 
>  drivers/usb/chipidea/ci_hdrc_usb2.c |  5 +++++
>  drivers/usb/chipidea/core.c         | 26 +++++++++++++++++++-------
>  drivers/usb/chipidea/host.c         |  9 +++++++++
>  drivers/usb/chipidea/otg.c          |  6 +++++-
>  drivers/usb/chipidea/otg_fsm.c      |  9 +++++++++
>  drivers/usb/chipidea/udc.c          |  3 ++-
>  include/linux/usb/chipidea.h        |  1 +
>  7 files changed, 50 insertions(+), 9 deletions(-)
> 

I am afraid I can't find [4/6] at my INBOX, comments below:

From: Piyush Mehta <piyush.mehta@xilinx.com>

API was neither captured nor checked.Fixed it by capturing the
return value and then checking for any error.

Addresses-Coverity: "Calling without checking return"
Addresses-Coverity: "CHECKED_RETURN"
Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/usb/chipidea/core.c | 11 +++++++----
 drivers/usb/chipidea/otg.c  |  6 +++++-
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 2b18f50..676346f 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -700,13 +700,16 @@ static int ci_get_platdata(struct device *dev,
 	if (usb_get_maximum_speed(dev) == USB_SPEED_FULL)
 		platdata->flags |= CI_HDRC_FORCE_FULLSPEED;

-	of_property_read_u32(dev->of_node, "phy-clkgate-delay-us",
-				     &platdata->phy_clkgate_delay_us);
+	if (of_property_read_u32(dev->of_node, "phy-clkgate-delay-us",
+				 &platdata->phy_clkgate_delay_us))
+		dev_dbg(dev, "Missing phy-clkgate-delay-us property\n");

 	platdata->itc_setting = 1;

-	of_property_read_u32(dev->of_node, "itc-setting",
-					&platdata->itc_setting);
+	if (of_property_read_u32(dev->of_node, "itc-setting",
+				 &platdata->itc_setting))
+		dev_dbg(dev, "Missing itc-setting property\n");
+

You may check how to handle this for other properties in this function.


 	ret = of_property_read_u32(dev->of_node, "ahb-burst-config",
 				&platdata->ahb_burst_config);
diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
index 8dd5928..d527d9d 100644
--- a/drivers/usb/chipidea/otg.c
+++ b/drivers/usb/chipidea/otg.c
@@ -168,6 +168,7 @@ static int hw_wait_vbus_lower_bsv(struct ci_hdrc *ci)
 static void ci_handle_id_switch(struct ci_hdrc *ci)
 {
 	enum ci_role role = ci_otg_role(ci);
+	int ret;

 	if (role != ci->role) {
 		dev_dbg(ci->dev, "switching from %s to %s\n",
@@ -193,7 +194,10 @@ static void ci_handle_id_switch(struct ci_hdrc *ci)
 			 */
 			hw_wait_vbus_lower_bsv(ci);

-		ci_role_start(ci, role);
+		ret = ci_role_start(ci, role);
+		if (ret < 0)
+			dev_dbg(ci->dev, "switching err %d\n", ret);
+

You may use dev_err for it.

 		/* vbus change may have already occurred */
 		if (role == CI_ROLE_GADGET)
 			ci_handle_vbus_change(ci);

-- 

Thanks,
Peter Chen

Re: [PATCH 1/6] usb: chipidea: Add support for VBUS control with PHY

[Jun Li]

Manish Narani <manish.narani@xilinx.com> 于2021年8月25日周三 上午1:23写道：
>
> Some platforms make use of VBUS control over PHY which
> means controller driver has to access PHY registers to
> turn on/off VBUS line.This patch adds support for
> such platforms in chipidea.
>
> Signed-off-by: Subbaraya Sundeep Bhatta <sbhatta@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  drivers/usb/chipidea/ci_hdrc_usb2.c | 1 +
>  drivers/usb/chipidea/host.c         | 9 +++++++++
>  drivers/usb/chipidea/otg_fsm.c      | 7 +++++++
>  include/linux/usb/chipidea.h        | 1 +
>  4 files changed, 18 insertions(+)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c b/drivers/usb/chipidea/ci_hdrc_usb2.c
> index 89e1d82..dc86b12 100644
> --- a/drivers/usb/chipidea/ci_hdrc_usb2.c
> +++ b/drivers/usb/chipidea/ci_hdrc_usb2.c
> @@ -30,6 +30,7 @@ static const struct ci_hdrc_platform_data ci_default_pdata = {
>
>  static const struct ci_hdrc_platform_data ci_zynq_pdata = {
>         .capoffset      = DEF_CAPOFFSET,
> +       .flags          = CI_HDRC_PHY_VBUS_CONTROL,
>  };
>
>  static const struct ci_hdrc_platform_data ci_zevio_pdata = {
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index e86d13c..578968d 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -63,6 +63,14 @@ static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
>                 priv->enabled = enable;
>         }
>
> +       if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL &&
> +           ci->usb_phy && ci->usb_phy->set_vbus) {
> +               if (enable)
> +                       ci->usb_phy->set_vbus(ci->usb_phy, 1);
> +               else
> +                       ci->usb_phy->set_vbus(ci->usb_phy, 0);
> +       }
> +

Can existing API usb_phy_vbus_on/off() work for you? I think that
helper will make it simple so you don't need a new flag and those
checks.

Li Jun
>         if (enable && (ci->platdata->phy_mode == USBPHY_INTERFACE_MODE_HSIC)) {
>                 /*
>                  * Marvell 28nm HSIC PHY requires forcing the port to HS mode.
> @@ -71,6 +79,7 @@ static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
>                 hw_port_test_set(ci, 5);
>                 hw_port_test_set(ci, 0);
>         }
> +
>         return 0;
>  };
>
> diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
> index 6ed4b00..2f7f94d 100644
> --- a/drivers/usb/chipidea/otg_fsm.c
> +++ b/drivers/usb/chipidea/otg_fsm.c
> @@ -471,6 +471,10 @@ static void ci_otg_drv_vbus(struct otg_fsm *fsm, int on)
>                                 return;
>                         }
>                 }
> +
> +               if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL)
> +                       ci->usb_phy->set_vbus(ci->usb_phy, 1);
> +
>                 /* Disable data pulse irq */
>                 hw_write_otgsc(ci, OTGSC_DPIE, 0);
>
> @@ -480,6 +484,9 @@ static void ci_otg_drv_vbus(struct otg_fsm *fsm, int on)
>                 if (ci->platdata->reg_vbus)
>                         regulator_disable(ci->platdata->reg_vbus);
>
> +               if (ci->platdata->flags & CI_HDRC_PHY_VBUS_CONTROL)
> +                       ci->usb_phy->set_vbus(ci->usb_phy, 0);
> +
>                 fsm->a_bus_drop = 1;
>                 fsm->a_bus_req = 0;
>         }
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index edf3342..ee38835 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -62,6 +62,7 @@ struct ci_hdrc_platform_data {
>  #define CI_HDRC_REQUIRES_ALIGNED_DMA   BIT(13)
>  #define CI_HDRC_IMX_IS_HSIC            BIT(14)
>  #define CI_HDRC_PMQOS                  BIT(15)
> +#define CI_HDRC_PHY_VBUS_CONTROL       BIT(16)
>         enum usb_dr_mode        dr_mode;
>  #define CI_HDRC_CONTROLLER_RESET_EVENT         0
>  #define CI_HDRC_CONTROLLER_STOPPED_EVENT       1
> --
> 2.1.1
>