[PATCH V2 0/8] Tegra XUSB charger detect support

[PATCH V2 0/8] Tegra XUSB charger detect support

[Nagarjuna Kristam]

This patch series adds charger detect support on XUSB hardware used in
Tegra210 and Tegra186 SoCs.

This patchset is composed with :
 - dt bindings of XUSB Pad Controller
 - Tegra XUSB device mode driver to add vbus_draw support 
 - Tegra PHY driver for charger detect support

Tests done:
 - Connect USB cable from ubuntu host to micro-B port of DUT to detect
   SDP_TYPE charger
 - Connect USB cable from external powered USB hub(which inturn connects
   to ubuntu host) to micro-B port of DUT to detect CDP_TYPE charger.
 - Connect USB cable from USB charger to micro-B port of DUT to detect
   DCP_TYPE charger.
DUT: Jetson-tx1, Jetson tx2.

V2:
 - Added ACKed-by details for DT patches.
 - All patches rebased.

Nagarjuna Kristam (8):
  dt-bindings: phy: tegra-xusb: Add charger-detect property
  usb: gadget: tegra-xudc: Add vbus_draw support
  phy: tegra: xusb: Add support for UTMI pad power control
  phy: tegra: xusb: Add USB2 pad power control support for Tegra210
  phy: tegra: xusb: Add soc ops API to enable UTMI PAD protection
  phy: tegra: xusb: Add support for charger detect
  phy: tegra: xusb: Enable charger detect for Tegra186
  phy: tegra: xusb: Enable charger detect for Tegra210

 .../bindings/phy/nvidia,tegra124-xusb-padctl.txt   |   4 +
 drivers/phy/tegra/Makefile                         |   2 +-
 drivers/phy/tegra/xusb-tegra-cd.c                  | 300 +++++++++++++++++++++
 drivers/phy/tegra/xusb-tegra186.c                  |  92 +++++--
 drivers/phy/tegra/xusb-tegra210.c                  | 222 +++++++++++----
 drivers/phy/tegra/xusb.c                           |  80 ++++++
 drivers/phy/tegra/xusb.h                           |  22 ++
 drivers/usb/gadget/udc/tegra-xudc.c                |  16 ++
 8 files changed, 653 insertions(+), 85 deletions(-)
 create mode 100644 drivers/phy/tegra/xusb-tegra-cd.c

-- 
2.7.4

[PATCH V2 1/8] dt-bindings: phy: tegra-xusb: Add charger-detect property

[Nagarjuna Kristam]

Add nvidia,charger-detect boolean property for Tegra210 and Tegra186
platforms. This property is used to inform driver to perform charger
detection on corresponding USB 2 port.

Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
Acked-by: Rob Herring <robh@kernel.org>
---
V2:
 - Added Acked-by updates to commit message.
---
 Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
index 38c5fa2..9b2d2dd 100644
--- a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
+++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
@@ -190,6 +190,10 @@ Required properties for OTG/Peripheral capable USB2 ports:
   and peripheral roles. Connector should be added as subnode.
   See usb/usb-conn-gpio.txt.
 
+Optional properties for OTG/Peripheral capable USB2 ports:
+- nvidia,charger-detect: A boolean property whose presence inform driver to
+  perform charger-detect activity.
+
 Optional properties:
 - nvidia,internal: A boolean property whose presence determines that a port
   is internal. In the absence of this property the port is considered to be
-- 
2.7.4

Re: [PATCH V2 1/8] dt-bindings: phy: tegra-xusb: Add charger-detect property

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

On Wed, Apr 15, 2020 at 01:55:01PM +0530, Nagarjuna Kristam wrote:
> Add nvidia,charger-detect boolean property for Tegra210 and Tegra186
> platforms. This property is used to inform driver to perform charger
> detection on corresponding USB 2 port.
> 
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> V2:
>  - Added Acked-by updates to commit message.
> ---
>  Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 4 ++++
>  1 file changed, 4 insertions(+)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V2 2/8] usb: gadget: tegra-xudc: Add vbus_draw support

[Nagarjuna Kristam]

Register vbus_draw to gadget ops and update corresponding vbus
draw current to usb_phy.

Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
---
V2:
 - Patch re-based.
---
 drivers/usb/gadget/udc/tegra-xudc.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
index 52a6add..9d3c109 100644
--- a/drivers/usb/gadget/udc/tegra-xudc.c
+++ b/drivers/usb/gadget/udc/tegra-xudc.c
@@ -492,6 +492,7 @@ struct tegra_xudc {
 	bool powergated;
 
 	struct usb_phy **usbphy;
+	struct usb_phy *curr_usbphy;
 	struct notifier_block vbus_nb;
 
 	struct completion disconnect_complete;
@@ -719,6 +720,7 @@ static int tegra_xudc_vbus_notify(struct notifier_block *nb,
 	if (!xudc->suspended && phy_index != -1) {
 		xudc->curr_utmi_phy = xudc->utmi_phy[phy_index];
 		xudc->curr_usb3_phy = xudc->usb3_phy[phy_index];
+		xudc->curr_usbphy = usbphy;
 		schedule_work(&xudc->usb_role_sw_work);
 	}
 
@@ -2042,6 +2044,19 @@ static int tegra_xudc_gadget_stop(struct usb_gadget *gadget)
 	return 0;
 }
 
+static int tegra_xudc_gadget_vbus_draw(struct usb_gadget *gadget,
+						unsigned int m_a)
+{
+	struct tegra_xudc *xudc = to_xudc(gadget);
+
+	dev_dbg(xudc->dev, "%s: %u mA\n", __func__, m_a);
+
+	if (xudc->curr_usbphy->chg_type == SDP_TYPE)
+		usb_phy_set_power(xudc->curr_usbphy, m_a);
+
+	return 0;
+}
+
 static int tegra_xudc_set_selfpowered(struct usb_gadget *gadget, int is_on)
 {
 	struct tegra_xudc *xudc = to_xudc(gadget);
@@ -2058,6 +2073,7 @@ static struct usb_gadget_ops tegra_xudc_gadget_ops = {
 	.pullup = tegra_xudc_gadget_pullup,
 	.udc_start = tegra_xudc_gadget_start,
 	.udc_stop = tegra_xudc_gadget_stop,
+	.vbus_draw = tegra_xudc_gadget_vbus_draw,
 	.set_selfpowered = tegra_xudc_set_selfpowered,
 };
 
-- 
2.7.4

Re: [PATCH V2 2/8] usb: gadget: tegra-xudc: Add vbus_draw support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1914 bytes --]

On Wed, Apr 15, 2020 at 01:55:02PM +0530, Nagarjuna Kristam wrote:
> Register vbus_draw to gadget ops and update corresponding vbus
> draw current to usb_phy.
> 
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V2:
>  - Patch re-based.
> ---
>  drivers/usb/gadget/udc/tegra-xudc.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
> index 52a6add..9d3c109 100644
> --- a/drivers/usb/gadget/udc/tegra-xudc.c
> +++ b/drivers/usb/gadget/udc/tegra-xudc.c
> @@ -492,6 +492,7 @@ struct tegra_xudc {
>  	bool powergated;
>  
>  	struct usb_phy **usbphy;
> +	struct usb_phy *curr_usbphy;
>  	struct notifier_block vbus_nb;
>  
>  	struct completion disconnect_complete;
> @@ -719,6 +720,7 @@ static int tegra_xudc_vbus_notify(struct notifier_block *nb,
>  	if (!xudc->suspended && phy_index != -1) {
>  		xudc->curr_utmi_phy = xudc->utmi_phy[phy_index];
>  		xudc->curr_usb3_phy = xudc->usb3_phy[phy_index];
> +		xudc->curr_usbphy = usbphy;
>  		schedule_work(&xudc->usb_role_sw_work);
>  	}
>  
> @@ -2042,6 +2044,19 @@ static int tegra_xudc_gadget_stop(struct usb_gadget *gadget)
>  	return 0;
>  }
>  
> +static int tegra_xudc_gadget_vbus_draw(struct usb_gadget *gadget,
> +						unsigned int m_a)
> +{
> +	struct tegra_xudc *xudc = to_xudc(gadget);
> +
> +	dev_dbg(xudc->dev, "%s: %u mA\n", __func__, m_a);
> +
> +	if (xudc->curr_usbphy->chg_type == SDP_TYPE)
> +		usb_phy_set_power(xudc->curr_usbphy, m_a);

Do we need to propagate the error code here in case the USB PHY for some
reason doesn't support the given current? Or is it guaranteed that we
always do support whatever is passed in here?

Regardless of whether we support it or not, it might still be useful to
add proper handling, if for nothing else but to set a good example.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V2 2/8] usb: gadget: tegra-xudc: Add vbus_draw support

[Nagarjuna Kristam]

On 28-04-2020 15:29, Thierry Reding wrote:
>> @@ -2042,6 +2044,19 @@ static int tegra_xudc_gadget_stop(struct usb_gadget *gadget)
>>   	return 0;
>>   }
>>   
>> +static int tegra_xudc_gadget_vbus_draw(struct usb_gadget *gadget,
>> +						unsigned int m_a)
>> +{
>> +	struct tegra_xudc *xudc = to_xudc(gadget);
>> +
>> +	dev_dbg(xudc->dev, "%s: %u mA\n", __func__, m_a);
>> +
>> +	if (xudc->curr_usbphy->chg_type == SDP_TYPE)
>> +		usb_phy_set_power(xudc->curr_usbphy, m_a);
> Do we need to propagate the error code here in case the USB PHY for some
> reason doesn't support the given current? Or is it guaranteed that we
> always do support whatever is passed in here?
> 
> Regardless of whether we support it or not, it might still be useful to
> add proper handling, if for nothing else but to set a good example.
> 
> Thierry
Will update accordingly, propagate the return the code to caller.

[PATCH V2 3/8] phy: tegra: xusb: Add support for UTMI pad power control

[Nagarjuna Kristam]

Add support for UTMI pad power on and off API's via soc ops. These API
can be used by operations like charger detect to power on and off UTMI
pad if needed. Update powered_on flag in the pad power control API's.

Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
---
V2:
 - Patch re-based.
---
 drivers/phy/tegra/xusb-tegra186.c | 51 ++++++++++++++++++---------------------
 drivers/phy/tegra/xusb.h          |  2 ++
 2 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
index 5d64f69..f862254 100644
--- a/drivers/phy/tegra/xusb-tegra186.c
+++ b/drivers/phy/tegra/xusb-tegra186.c
@@ -192,12 +192,8 @@ static void tegra186_utmi_bias_pad_power_on(struct tegra_xusb_padctl *padctl)
 	u32 value;
 	int err;
 
-	mutex_lock(&padctl->lock);
-
-	if (priv->bias_pad_enable++ > 0) {
-		mutex_unlock(&padctl->lock);
+	if (priv->bias_pad_enable++ > 0)
 		return;
-	}
 
 	err = clk_prepare_enable(priv->usb2_trk_clk);
 	if (err < 0)
@@ -221,8 +217,6 @@ static void tegra186_utmi_bias_pad_power_on(struct tegra_xusb_padctl *padctl)
 	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
 	value &= ~USB2_PD_TRK;
 	padctl_writel(padctl, value, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
-
-	mutex_unlock(&padctl->lock);
 }
 
 static void tegra186_utmi_bias_pad_power_off(struct tegra_xusb_padctl *padctl)
@@ -230,44 +224,29 @@ static void tegra186_utmi_bias_pad_power_off(struct tegra_xusb_padctl *padctl)
 	struct tegra186_xusb_padctl *priv = to_tegra186_xusb_padctl(padctl);
 	u32 value;
 
-	mutex_lock(&padctl->lock);
-
-	if (WARN_ON(priv->bias_pad_enable == 0)) {
-		mutex_unlock(&padctl->lock);
+	if (WARN_ON(priv->bias_pad_enable == 0))
 		return;
-	}
 
-	if (--priv->bias_pad_enable > 0) {
-		mutex_unlock(&padctl->lock);
+	if (--priv->bias_pad_enable > 0)
 		return;
-	}
 
 	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
 	value |= USB2_PD_TRK;
 	padctl_writel(padctl, value, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
 
 	clk_disable_unprepare(priv->usb2_trk_clk);
-
-	mutex_unlock(&padctl->lock);
 }
 
 static void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
 	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
-	struct tegra_xusb_usb2_port *port;
-	struct device *dev = padctl->dev;
+	struct tegra_xusb_usb2_lane *usb2 = to_usb2_lane(lane);
 	unsigned int index = lane->index;
 	u32 value;
 
-	if (!phy)
-		return;
-
-	port = tegra_xusb_find_usb2_port(padctl, index);
-	if (!port) {
-		dev_err(dev, "no port found for USB2 lane %u\n", index);
+	if (!phy || usb2->powered_on)
 		return;
-	}
 
 	tegra186_utmi_bias_pad_power_on(padctl);
 
@@ -280,16 +259,19 @@ static void tegra_phy_xusb_utmi_pad_power_on(struct phy *phy)
 	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
 	value &= ~USB2_OTG_PD_DR;
 	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
+
+	usb2->powered_on = true;
 }
 
 static void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
 	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
+	struct tegra_xusb_usb2_lane *usb2 = to_usb2_lane(lane);
 	unsigned int index = lane->index;
 	u32 value;
 
-	if (!phy)
+	if (!phy || !usb2->powered_on)
 		return;
 
 	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
@@ -303,6 +285,8 @@ static void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy)
 	udelay(2);
 
 	tegra186_utmi_bias_pad_power_off(padctl);
+
+	usb2->powered_on = false;
 }
 
 static int tegra186_xusb_padctl_vbus_override(struct tegra_xusb_padctl *padctl,
@@ -413,6 +397,8 @@ static int tegra186_utmi_phy_power_on(struct phy *phy)
 		return -ENODEV;
 	}
 
+	mutex_lock(&padctl->lock);
+
 	value = padctl_readl(padctl, XUSB_PADCTL_USB2_PAD_MUX);
 	value &= ~(USB2_PORT_MASK << USB2_PORT_SHIFT(index));
 	value |= (PORT_XUSB << USB2_PORT_SHIFT(index));
@@ -464,14 +450,23 @@ static int tegra186_utmi_phy_power_on(struct phy *phy)
 
 	/* TODO: pad power saving */
 	tegra_phy_xusb_utmi_pad_power_on(phy);
+
+	mutex_unlock(&padctl->lock);
 	return 0;
 }
 
 static int tegra186_utmi_phy_power_off(struct phy *phy)
 {
+	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
+	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
+
+	mutex_lock(&padctl->lock);
+
 	/* TODO: pad power saving */
 	tegra_phy_xusb_utmi_pad_power_down(phy);
 
+	mutex_unlock(&padctl->lock);
+
 	return 0;
 }
 
@@ -938,6 +933,8 @@ static const struct tegra_xusb_padctl_ops tegra186_xusb_padctl_ops = {
 	.probe = tegra186_xusb_padctl_probe,
 	.remove = tegra186_xusb_padctl_remove,
 	.vbus_override = tegra186_xusb_padctl_vbus_override,
+	.utmi_pad_power_on = tegra_phy_xusb_utmi_pad_power_on,
+	.utmi_pad_power_down = tegra_phy_xusb_utmi_pad_power_down,
 };
 
 #if IS_ENABLED(CONFIG_ARCH_TEGRA_186_SOC)
diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
index ea35af7..6995fc4 100644
--- a/drivers/phy/tegra/xusb.h
+++ b/drivers/phy/tegra/xusb.h
@@ -396,6 +396,8 @@ struct tegra_xusb_padctl_ops {
 				    unsigned int index, bool enable);
 	int (*vbus_override)(struct tegra_xusb_padctl *padctl, bool set);
 	int (*utmi_port_reset)(struct phy *phy);
+	void (*utmi_pad_power_on)(struct phy *phy);
+	void (*utmi_pad_power_down)(struct phy *phy);
 };
 
 struct tegra_xusb_padctl_soc {
-- 
2.7.4

Re: [PATCH V2 3/8] phy: tegra: xusb: Add support for UTMI pad power control

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

On Wed, Apr 15, 2020 at 01:55:03PM +0530, Nagarjuna Kristam wrote:
> Add support for UTMI pad power on and off API's via soc ops. These API
> can be used by operations like charger detect to power on and off UTMI
> pad if needed. Update powered_on flag in the pad power control API's.
> 
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V2:
>  - Patch re-based.
> ---
>  drivers/phy/tegra/xusb-tegra186.c | 51 ++++++++++++++++++---------------------
>  drivers/phy/tegra/xusb.h          |  2 ++
>  2 files changed, 26 insertions(+), 27 deletions(-)

Can we not simply pass another reference to the UTMI PHY to the charger
detect code so that we can avoid this custom API?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V2 3/8] phy: tegra: xusb: Add support for UTMI pad power control

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On Wed, Apr 15, 2020 at 01:55:03PM +0530, Nagarjuna Kristam wrote:
> Add support for UTMI pad power on and off API's via soc ops. These API
> can be used by operations like charger detect to power on and off UTMI
> pad if needed. Update powered_on flag in the pad power control API's.
> 
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V2:
>  - Patch re-based.
> ---
>  drivers/phy/tegra/xusb-tegra186.c | 51 ++++++++++++++++++---------------------
>  drivers/phy/tegra/xusb.h          |  2 ++
>  2 files changed, 26 insertions(+), 27 deletions(-)

Nevermind my prior comments, for some reason I thought this was adding a
custom API for driver interoperability, but I see now that this is just
a means of splitting out SoC-specific code, so:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V2 4/8] phy: tegra: xusb: Add USB2 pad power control support for Tegra210

[Nagarjuna Kristam]

Add USB2 pad power on and off API's for TEgra210 and provide its control
via soc ops. It can be used by operations like charger detect to power on
and off USB2 pad if needed.

Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
---
V2:
 - Patch re-based.
---
 drivers/phy/tegra/xusb-tegra210.c | 190 ++++++++++++++++++++++++++------------
 1 file changed, 133 insertions(+), 57 deletions(-)

diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
index 66bd461..caf0890 100644
--- a/drivers/phy/tegra/xusb-tegra210.c
+++ b/drivers/phy/tegra/xusb-tegra210.c
@@ -994,6 +994,128 @@ static int tegra210_xusb_padctl_id_override(struct tegra_xusb_padctl *padctl,
 	return 0;
 }
 
+static void tegra210_usb2_bias_pad_power_on(struct tegra_xusb_usb2_pad *pad)
+{
+	struct tegra_xusb_padctl *padctl = pad->base.padctl;
+	u32 value;
+
+	if (pad->enable++ > 0)
+		return;
+
+	dev_dbg(padctl->dev, "power on BIAS PAD & USB2 tracking\n");
+
+	if (clk_prepare_enable(pad->clk))
+		dev_warn(padctl->dev, "failed to enable BIAS PAD & USB2 tracking\n");
+
+	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
+	value &= ~((XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_MASK <<
+		    XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_SHIFT) |
+		   (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_MASK <<
+		    XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_SHIFT));
+	value |= (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_VAL <<
+		  XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_SHIFT) |
+		 (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_VAL <<
+		  XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_SHIFT);
+	padctl_writel(padctl, value, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL0);
+	value &= ~XUSB_PADCTL_USB2_BIAS_PAD_CTL0_PD;
+	padctl_writel(padctl, value, XUSB_PADCTL_USB2_BIAS_PAD_CTL0);
+
+	udelay(1);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
+	value &= ~XUSB_PADCTL_USB2_BIAS_PAD_CTL1_PD_TRK;
+	padctl_writel(padctl, value, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
+
+	udelay(50);
+}
+
+static void tegra210_usb2_bias_pad_power_off(struct tegra_xusb_usb2_pad *pad)
+{
+	struct tegra_xusb_padctl *padctl = pad->base.padctl;
+	u32 value;
+
+	if (WARN_ON(pad->enable == 0))
+		return;
+
+	if (--pad->enable > 0)
+		return;
+
+	dev_dbg(padctl->dev, "power off USB2 tracking\n");
+
+	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL0);
+	value |= XUSB_PADCTL_USB2_BIAS_PAD_CTL0_PD;
+	padctl_writel(padctl, value, XUSB_PADCTL_USB2_BIAS_PAD_CTL0);
+
+	clk_disable_unprepare(pad->clk);
+}
+
+/* must be called under padctl->lock */
+void tegra210_usb2_pad_power_on(struct phy *phy)
+{
+	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
+	struct tegra_xusb_usb2_lane *usb2 = to_usb2_lane(lane);
+	struct tegra_xusb_usb2_pad *pad = to_usb2_pad(lane->pad);
+	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
+	unsigned int index = lane->index;
+	u32 value;
+
+	if (!phy)
+		return;
+
+	if (usb2->powered_on)
+		return;
+
+	dev_info(padctl->dev, "power on UTMI pads %d\n", index);
+
+	tegra210_usb2_bias_pad_power_on(pad);
+
+	udelay(2);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+	value &= ~XUSB_PADCTL_USB2_OTG_PAD_CTL0_PD;
+	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+
+	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
+	value &= ~XUSB_PADCTL_USB2_OTG_PAD_CTL1_PD_DR;
+	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
+
+	usb2->powered_on = true;
+}
+
+/* must be called under padctl->lock */
+void tegra210_usb2_pad_power_down(struct phy *phy)
+{
+	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
+	struct tegra_xusb_usb2_lane *usb2 = to_usb2_lane(lane);
+	struct tegra_xusb_usb2_pad *pad = to_usb2_pad(lane->pad);
+	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
+	unsigned int index = lane->index;
+	u32 value;
+
+	if (!phy)
+		return;
+
+	if (!usb2->powered_on)
+		return;
+
+	dev_info(padctl->dev, "power down UTMI pad %d\n", index);
+
+	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+	value |= XUSB_PADCTL_USB2_OTG_PAD_CTL0_PD;
+	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+
+	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
+	value |= XUSB_PADCTL_USB2_OTG_PAD_CTL1_PD_DR;
+	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
+
+	udelay(2);
+
+	tegra210_usb2_bias_pad_power_off(pad);
+	usb2->powered_on = false;
+}
+
 static int tegra210_usb2_phy_set_mode(struct phy *phy, enum phy_mode mode,
 				      int submode)
 {
@@ -1037,7 +1159,6 @@ static int tegra210_usb2_phy_power_on(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
 	struct tegra_xusb_usb2_lane *usb2 = to_usb2_lane(lane);
-	struct tegra_xusb_usb2_pad *pad = to_usb2_pad(lane->pad);
 	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
 	struct tegra210_xusb_padctl *priv;
 	struct tegra_xusb_usb2_port *port;
@@ -1053,6 +1174,8 @@ static int tegra210_usb2_phy_power_on(struct phy *phy)
 
 	priv = to_tegra210_xusb_padctl(padctl);
 
+	mutex_lock(&padctl->lock);
+
 	if (port->usb3_port_fake != -1) {
 		value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
 		value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(
@@ -1148,62 +1271,21 @@ static int tegra210_usb2_phy_power_on(struct phy *phy)
 
 	if (port->supply && port->mode == USB_DR_MODE_HOST) {
 		err = regulator_enable(port->supply);
-		if (err)
+		if (err) {
+			mutex_unlock(&padctl->lock);
 			return err;
+		}
 	}
 
-	mutex_lock(&padctl->lock);
-
-	if (pad->enable > 0) {
-		pad->enable++;
-		mutex_unlock(&padctl->lock);
-		return 0;
-	}
-
-	err = clk_prepare_enable(pad->clk);
-	if (err)
-		goto disable_regulator;
+	tegra210_usb2_pad_power_on(phy);
 
-	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
-	value &= ~((XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_MASK <<
-		    XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_SHIFT) |
-		   (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_MASK <<
-		    XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_SHIFT));
-	value |= (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_VAL <<
-		  XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_SHIFT) |
-		 (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_VAL <<
-		  XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_SHIFT);
-	padctl_writel(padctl, value, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
-
-	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL0);
-	value &= ~XUSB_PADCTL_USB2_BIAS_PAD_CTL0_PD;
-	padctl_writel(padctl, value, XUSB_PADCTL_USB2_BIAS_PAD_CTL0);
-
-	udelay(1);
-
-	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
-	value &= ~XUSB_PADCTL_USB2_BIAS_PAD_CTL1_PD_TRK;
-	padctl_writel(padctl, value, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
-
-	udelay(50);
-
-	clk_disable_unprepare(pad->clk);
-
-	pad->enable++;
 	mutex_unlock(&padctl->lock);
-
 	return 0;
-
-disable_regulator:
-	regulator_disable(port->supply);
-	mutex_unlock(&padctl->lock);
-	return err;
 }
 
 static int tegra210_usb2_phy_power_off(struct phy *phy)
 {
 	struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
-	struct tegra_xusb_usb2_pad *pad = to_usb2_pad(lane->pad);
 	struct tegra_xusb_padctl *padctl = lane->pad->padctl;
 	struct tegra_xusb_usb2_port *port;
 	u32 value;
@@ -1217,6 +1299,8 @@ static int tegra210_usb2_phy_power_off(struct phy *phy)
 
 	mutex_lock(&padctl->lock);
 
+	tegra210_usb2_pad_power_down(phy);
+
 	if (port->usb3_port_fake != -1) {
 		value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM1);
 		value |= XUSB_PADCTL_ELPG_PROGRAM1_SSPX_ELPG_CLAMP_EN_EARLY(
@@ -1243,18 +1327,8 @@ static int tegra210_usb2_phy_power_off(struct phy *phy)
 		padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
 	}
 
-	if (WARN_ON(pad->enable == 0))
-		goto out;
-
-	if (--pad->enable > 0)
-		goto out;
-
-	value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL0);
-	value |= XUSB_PADCTL_USB2_BIAS_PAD_CTL0_PD;
-	padctl_writel(padctl, value, XUSB_PADCTL_USB2_BIAS_PAD_CTL0);
-
-out:
 	regulator_disable(port->supply);
+
 	mutex_unlock(&padctl->lock);
 	return 0;
 }
@@ -2215,6 +2289,8 @@ static const struct tegra_xusb_padctl_ops tegra210_xusb_padctl_ops = {
 	.hsic_set_idle = tegra210_hsic_set_idle,
 	.vbus_override = tegra210_xusb_padctl_vbus_override,
 	.utmi_port_reset = tegra210_utmi_port_reset,
+	.utmi_pad_power_on = tegra210_usb2_pad_power_on,
+	.utmi_pad_power_down = tegra210_usb2_pad_power_down,
 };
 
 static const char * const tegra210_xusb_padctl_supply_names[] = {
-- 
2.7.4

Re: [PATCH V2 4/8] phy: tegra: xusb: Add USB2 pad power control support for Tegra210

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 666 bytes --]

On Wed, Apr 15, 2020 at 01:55:04PM +0530, Nagarjuna Kristam wrote:
> Add USB2 pad power on and off API's for TEgra210 and provide its control

"Tegra210"

> via soc ops. It can be used by operations like charger detect to power on
> and off USB2 pad if needed.
> 
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V2:
>  - Patch re-based.
> ---
>  drivers/phy/tegra/xusb-tegra210.c | 190 ++++++++++++++++++++++++++------------
>  1 file changed, 133 insertions(+), 57 deletions(-)

Again, is there any particular reason why this has to be done with a
custom API rather than just passing the UTMI PHY to the charging code?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V2 4/8] phy: tegra: xusb: Add USB2 pad power control support for Tegra210

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

On Wed, Apr 15, 2020 at 01:55:04PM +0530, Nagarjuna Kristam wrote:
> Add USB2 pad power on and off API's for TEgra210 and provide its control
> via soc ops. It can be used by operations like charger detect to power on
> and off USB2 pad if needed.
> 
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V2:
>  - Patch re-based.
> ---
>  drivers/phy/tegra/xusb-tegra210.c | 190 ++++++++++++++++++++++++++------------
>  1 file changed, 133 insertions(+), 57 deletions(-)

Ignore that previous comment, I was making wrong assumptions:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V2 5/8] phy: tegra: xusb: Add soc ops API to enable UTMI PAD protection

[Nagarjuna Kristam]

When USB charger is enabled, UTMI PAD needs to be protected according
to the direction and current level. Add support for the same on Tegra210
and Tegra186.

Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
---
V2:
 - Commit message coorected.
 - Patch re-based.
---
 drivers/phy/tegra/xusb-tegra186.c | 40 +++++++++++++++++++++++++++++++++++++++
 drivers/phy/tegra/xusb-tegra210.c | 31 ++++++++++++++++++++++++++++++
 drivers/phy/tegra/xusb.h          | 13 +++++++++++++
 3 files changed, 84 insertions(+)

diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
index f862254..03bdb5b 100644
--- a/drivers/phy/tegra/xusb-tegra186.c
+++ b/drivers/phy/tegra/xusb-tegra186.c
@@ -68,6 +68,13 @@
 #define   PORTX_SPEED_SUPPORT_MASK		(0x3)
 #define     PORT_SPEED_SUPPORT_GEN1		(0x0)
 
+#define USB2_BATTERY_CHRG_OTGPADX_CTL1(x)       (0x84 + (x) * 0x40)
+#define  PD_VREG                                (1 << 6)
+#define  VREG_LEV(x)                            (((x) & 0x3) << 7)
+#define  VREG_DIR(x)                            (((x) & 0x3) << 11)
+#define  VREG_DIR_IN                            VREG_DIR(1)
+#define  VREG_DIR_OUT                           VREG_DIR(2)
+
 #define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x)	(0x88 + (x) * 0x40)
 #define  HS_CURR_LEVEL(x)			((x) & 0x3f)
 #define  TERM_SEL				BIT(25)
@@ -289,6 +296,37 @@ static void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy)
 	usb2->powered_on = false;
 }
 
+static void tegra186_xusb_padctl_utmi_pad_set_protection_level(
+				struct tegra_xusb_port *port, int level,
+				enum tegra_vbus_dir dir)
+{
+	u32 value;
+	struct tegra_xusb_padctl *padctl = port->padctl;
+	unsigned int index = port->index;
+
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
+
+	if (level < 0) {
+		/* disable pad protection */
+		value |= PD_VREG;
+		value &= ~VREG_LEV(~0);
+		value &= ~VREG_DIR(~0);
+	} else {
+		if (dir == TEGRA_VBUS_SOURCE)
+			value |= VREG_DIR_OUT;
+		else if (dir == TEGRA_VBUS_SINK)
+			value |= VREG_DIR_IN;
+
+		value &= ~PD_VREG;
+		value &= ~VREG_DIR(~0);
+		value &= ~VREG_LEV(~0);
+		value |= VREG_LEV(level);
+	}
+
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
+}
+
+
 static int tegra186_xusb_padctl_vbus_override(struct tegra_xusb_padctl *padctl,
 					       bool status)
 {
@@ -935,6 +973,8 @@ static const struct tegra_xusb_padctl_ops tegra186_xusb_padctl_ops = {
 	.vbus_override = tegra186_xusb_padctl_vbus_override,
 	.utmi_pad_power_on = tegra_phy_xusb_utmi_pad_power_on,
 	.utmi_pad_power_down = tegra_phy_xusb_utmi_pad_power_down,
+	.utmi_pad_set_protection_level =
+			tegra186_xusb_padctl_utmi_pad_set_protection_level,
 };
 
 #if IS_ENABLED(CONFIG_ARCH_TEGRA_186_SOC)
diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
index caf0890..7d84f1a 100644
--- a/drivers/phy/tegra/xusb-tegra210.c
+++ b/drivers/phy/tegra/xusb-tegra210.c
@@ -74,6 +74,8 @@
 #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_MASK 0x3
 #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_VAL 0x1
 #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18 (1 << 6)
+#define USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(x) (((x) & 0x3) << 7)
+#define USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_DIR(x) (((x) & 0x3) << 11)
 
 #define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x) (0x088 + (x) * 0x40)
 #define XUSB_PADCTL_USB2_OTG_PAD_CTL0_PD_ZI (1 << 29)
@@ -1116,6 +1118,33 @@ void tegra210_usb2_pad_power_down(struct phy *phy)
 	usb2->powered_on = false;
 }
 
+static void tegra210_xusb_padctl_utmi_pad_set_protection_level(
+				struct tegra_xusb_port *port, int level,
+				enum tegra_vbus_dir dir)
+{
+	u32 value;
+	struct tegra_xusb_padctl *padctl = port->padctl;
+	unsigned int index = port->index;
+
+	value = padctl_readl(padctl,
+			     XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
+
+	if (level < 0) {
+		/* disable pad protection */
+		value |= XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18;
+		value &= USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(~0);
+		value &= ~USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_DIR(~0);
+	} else {
+		value &= ~XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18;
+		value &= ~USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_DIR(~0);
+		value &= USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(~0);
+		value |= USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(level);
+	}
+
+	padctl_writel(padctl, value,
+		      XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
+}
+
 static int tegra210_usb2_phy_set_mode(struct phy *phy, enum phy_mode mode,
 				      int submode)
 {
@@ -2291,6 +2320,8 @@ static const struct tegra_xusb_padctl_ops tegra210_xusb_padctl_ops = {
 	.utmi_port_reset = tegra210_utmi_port_reset,
 	.utmi_pad_power_on = tegra210_usb2_pad_power_on,
 	.utmi_pad_power_down = tegra210_usb2_pad_power_down,
+	.utmi_pad_set_protection_level =
+			tegra210_xusb_padctl_utmi_pad_set_protection_level,
 };
 
 static const char * const tegra210_xusb_padctl_supply_names[] = {
diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
index 6995fc4..79e96b0 100644
--- a/drivers/phy/tegra/xusb.h
+++ b/drivers/phy/tegra/xusb.h
@@ -259,6 +259,17 @@ to_sata_pad(struct tegra_xusb_pad *pad)
  */
 struct tegra_xusb_port_ops;
 
+/*
+ * Tegra OTG port VBUS direction:
+ * default (based on port capability) or
+ * as source or sink
+ */
+enum tegra_vbus_dir {
+	TEGRA_VBUS_DEFAULT,
+	TEGRA_VBUS_SOURCE,
+	TEGRA_VBUS_SINK
+};
+
 struct tegra_xusb_port {
 	struct tegra_xusb_padctl *padctl;
 	struct tegra_xusb_lane *lane;
@@ -398,6 +409,8 @@ struct tegra_xusb_padctl_ops {
 	int (*utmi_port_reset)(struct phy *phy);
 	void (*utmi_pad_power_on)(struct phy *phy);
 	void (*utmi_pad_power_down)(struct phy *phy);
+	void (*utmi_pad_set_protection_level)(struct tegra_xusb_port *port,
+					int max_ua, enum tegra_vbus_dir dir);
 };
 
 struct tegra_xusb_padctl_soc {
-- 
2.7.4

Re: [PATCH V2 5/8] phy: tegra: xusb: Add soc ops API to enable UTMI PAD protection

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7271 bytes --]

On Wed, Apr 15, 2020 at 01:55:05PM +0530, Nagarjuna Kristam wrote:
> When USB charger is enabled, UTMI PAD needs to be protected according
> to the direction and current level. Add support for the same on Tegra210
> and Tegra186.
> 
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V2:
>  - Commit message coorected.
>  - Patch re-based.
> ---
>  drivers/phy/tegra/xusb-tegra186.c | 40 +++++++++++++++++++++++++++++++++++++++
>  drivers/phy/tegra/xusb-tegra210.c | 31 ++++++++++++++++++++++++++++++
>  drivers/phy/tegra/xusb.h          | 13 +++++++++++++
>  3 files changed, 84 insertions(+)

Oh wait... you're not actually adding custom public APIs for this but
are simply wiring this through the SoC-specific code. Okay, that seems
fine to me.

Ignore my comments on the prior two patches.

> diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
> index f862254..03bdb5b 100644
> --- a/drivers/phy/tegra/xusb-tegra186.c
> +++ b/drivers/phy/tegra/xusb-tegra186.c
> @@ -68,6 +68,13 @@
>  #define   PORTX_SPEED_SUPPORT_MASK		(0x3)
>  #define     PORT_SPEED_SUPPORT_GEN1		(0x0)
>  
> +#define USB2_BATTERY_CHRG_OTGPADX_CTL1(x)       (0x84 + (x) * 0x40)
> +#define  PD_VREG                                (1 << 6)
> +#define  VREG_LEV(x)                            (((x) & 0x3) << 7)
> +#define  VREG_DIR(x)                            (((x) & 0x3) << 11)
> +#define  VREG_DIR_IN                            VREG_DIR(1)
> +#define  VREG_DIR_OUT                           VREG_DIR(2)
> +
>  #define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x)	(0x88 + (x) * 0x40)
>  #define  HS_CURR_LEVEL(x)			((x) & 0x3f)
>  #define  TERM_SEL				BIT(25)
> @@ -289,6 +296,37 @@ static void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy)
>  	usb2->powered_on = false;
>  }
>  
> +static void tegra186_xusb_padctl_utmi_pad_set_protection_level(
> +				struct tegra_xusb_port *port, int level,
> +				enum tegra_vbus_dir dir)

It's more idiomatic to wrap after the return type and while at it,
perhaps make this name a little shorter, like so:

    static void
    tegra186_xusb_padctl_utmi_pad_set_protection(struct tegra_xusb_port *port,
						 int level,
						 enum tegra_vbus_dir dir)

> +{
> +	u32 value;
> +	struct tegra_xusb_padctl *padctl = port->padctl;
> +	unsigned int index = port->index;
> +
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +
> +	if (level < 0) {
> +		/* disable pad protection */
> +		value |= PD_VREG;
> +		value &= ~VREG_LEV(~0);
> +		value &= ~VREG_DIR(~0);
> +	} else {
> +		if (dir == TEGRA_VBUS_SOURCE)
> +			value |= VREG_DIR_OUT;
> +		else if (dir == TEGRA_VBUS_SINK)
> +			value |= VREG_DIR_IN;
> +
> +		value &= ~PD_VREG;
> +		value &= ~VREG_DIR(~0);
> +		value &= ~VREG_LEV(~0);
> +		value |= VREG_LEV(level);
> +	}
> +
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +}
> +
> +

There's an extra blank line above that can be dropped.

>  static int tegra186_xusb_padctl_vbus_override(struct tegra_xusb_padctl *padctl,
>  					       bool status)
>  {
> @@ -935,6 +973,8 @@ static const struct tegra_xusb_padctl_ops tegra186_xusb_padctl_ops = {
>  	.vbus_override = tegra186_xusb_padctl_vbus_override,
>  	.utmi_pad_power_on = tegra_phy_xusb_utmi_pad_power_on,
>  	.utmi_pad_power_down = tegra_phy_xusb_utmi_pad_power_down,
> +	.utmi_pad_set_protection_level =
> +			tegra186_xusb_padctl_utmi_pad_set_protection_level,
>  };
>  
>  #if IS_ENABLED(CONFIG_ARCH_TEGRA_186_SOC)
> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
> index caf0890..7d84f1a 100644
> --- a/drivers/phy/tegra/xusb-tegra210.c
> +++ b/drivers/phy/tegra/xusb-tegra210.c
> @@ -74,6 +74,8 @@
>  #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_MASK 0x3
>  #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_VAL 0x1
>  #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18 (1 << 6)
> +#define USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(x) (((x) & 0x3) << 7)
> +#define USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_DIR(x) (((x) & 0x3) << 11)
>  
>  #define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x) (0x088 + (x) * 0x40)
>  #define XUSB_PADCTL_USB2_OTG_PAD_CTL0_PD_ZI (1 << 29)
> @@ -1116,6 +1118,33 @@ void tegra210_usb2_pad_power_down(struct phy *phy)
>  	usb2->powered_on = false;
>  }
>  
> +static void tegra210_xusb_padctl_utmi_pad_set_protection_level(
> +				struct tegra_xusb_port *port, int level,
> +				enum tegra_vbus_dir dir)

Same comment as above.

> +{
> +	u32 value;
> +	struct tegra_xusb_padctl *padctl = port->padctl;
> +	unsigned int index = port->index;
> +
> +	value = padctl_readl(padctl,
> +			     XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +
> +	if (level < 0) {
> +		/* disable pad protection */
> +		value |= XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18;
> +		value &= USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(~0);
> +		value &= ~USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_DIR(~0);
> +	} else {
> +		value &= ~XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18;
> +		value &= ~USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_DIR(~0);
> +		value &= USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(~0);
> +		value |= USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(level);
> +	}
> +
> +	padctl_writel(padctl, value,
> +		      XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +}
> +
>  static int tegra210_usb2_phy_set_mode(struct phy *phy, enum phy_mode mode,
>  				      int submode)
>  {
> @@ -2291,6 +2320,8 @@ static const struct tegra_xusb_padctl_ops tegra210_xusb_padctl_ops = {
>  	.utmi_port_reset = tegra210_utmi_port_reset,
>  	.utmi_pad_power_on = tegra210_usb2_pad_power_on,
>  	.utmi_pad_power_down = tegra210_usb2_pad_power_down,
> +	.utmi_pad_set_protection_level =
> +			tegra210_xusb_padctl_utmi_pad_set_protection_level,
>  };
>  
>  static const char * const tegra210_xusb_padctl_supply_names[] = {
> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
> index 6995fc4..79e96b0 100644
> --- a/drivers/phy/tegra/xusb.h
> +++ b/drivers/phy/tegra/xusb.h
> @@ -259,6 +259,17 @@ to_sata_pad(struct tegra_xusb_pad *pad)
>   */
>  struct tegra_xusb_port_ops;
>  
> +/*
> + * Tegra OTG port VBUS direction:
> + * default (based on port capability) or
> + * as source or sink
> + */
> +enum tegra_vbus_dir {
> +	TEGRA_VBUS_DEFAULT,
> +	TEGRA_VBUS_SOURCE,
> +	TEGRA_VBUS_SINK
> +};

Can't we key off of something like the OTG mode? I thought we already
carried that elsewhere.

> +
>  struct tegra_xusb_port {
>  	struct tegra_xusb_padctl *padctl;
>  	struct tegra_xusb_lane *lane;
> @@ -398,6 +409,8 @@ struct tegra_xusb_padctl_ops {
>  	int (*utmi_port_reset)(struct phy *phy);
>  	void (*utmi_pad_power_on)(struct phy *phy);
>  	void (*utmi_pad_power_down)(struct phy *phy);
> +	void (*utmi_pad_set_protection_level)(struct tegra_xusb_port *port,
> +					int max_ua, enum tegra_vbus_dir dir);

You call the variable "max_ua" here but it's "level" in the
implementations, which is slightly confusing. Please choose one and
stick with it. Also, if it's a value in microamperes, perhaps just make
it unsigned int?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V2 5/8] phy: tegra: xusb: Add soc ops API to enable UTMI PAD protection

[Nagarjuna Kristam]

On 28-04-2020 15:45, Thierry Reding wrote:
> On Wed, Apr 15, 2020 at 01:55:05PM +0530, Nagarjuna Kristam wrote:
>> When USB charger is enabled, UTMI PAD needs to be protected according
>> to the direction and current level. Add support for the same on Tegra210
>> and Tegra186.
>>
>> Signed-off-by: Nagarjuna Kristam<nkristam@nvidia.com>
>> ---
>> V2:
>>   - Commit message coorected.
>>   - Patch re-based.
>> ---
>>   drivers/phy/tegra/xusb-tegra186.c | 40 +++++++++++++++++++++++++++++++++++++++
>>   drivers/phy/tegra/xusb-tegra210.c | 31 ++++++++++++++++++++++++++++++
>>   drivers/phy/tegra/xusb.h          | 13 +++++++++++++
>>   3 files changed, 84 insertions(+)
> Oh wait... you're not actually adding custom public APIs for this but
> are simply wiring this through the SoC-specific code. Okay, that seems
> fine to me.
> 
> Ignore my comments on the prior two patches.
> 
>> diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
>> index f862254..03bdb5b 100644
>> --- a/drivers/phy/tegra/xusb-tegra186.c
>> +++ b/drivers/phy/tegra/xusb-tegra186.c
>> @@ -68,6 +68,13 @@
>>   #define   PORTX_SPEED_SUPPORT_MASK		(0x3)
>>   #define     PORT_SPEED_SUPPORT_GEN1		(0x0)
>>   
>> +#define USB2_BATTERY_CHRG_OTGPADX_CTL1(x)       (0x84 + (x) * 0x40)
>> +#define  PD_VREG                                (1 << 6)
>> +#define  VREG_LEV(x)                            (((x) & 0x3) << 7)
>> +#define  VREG_DIR(x)                            (((x) & 0x3) << 11)
>> +#define  VREG_DIR_IN                            VREG_DIR(1)
>> +#define  VREG_DIR_OUT                           VREG_DIR(2)
>> +
>>   #define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x)	(0x88 + (x) * 0x40)
>>   #define  HS_CURR_LEVEL(x)			((x) & 0x3f)
>>   #define  TERM_SEL				BIT(25)
>> @@ -289,6 +296,37 @@ static void tegra_phy_xusb_utmi_pad_power_down(struct phy *phy)
>>   	usb2->powered_on = false;
>>   }
>>   
>> +static void tegra186_xusb_padctl_utmi_pad_set_protection_level(
>> +				struct tegra_xusb_port *port, int level,
>> +				enum tegra_vbus_dir dir)
> It's more idiomatic to wrap after the return type and while at it,
> perhaps make this name a little shorter, like so:
> 
>      static void
>      tegra186_xusb_padctl_utmi_pad_set_protection(struct tegra_xusb_port *port,
> 						 int level,
> 						 enum tegra_vbus_dir dir)
> 
Will update accordingly.
>> +{
>> +	u32 value;
>> +	struct tegra_xusb_padctl *padctl = port->padctl;
>> +	unsigned int index = port->index;
>> +
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>> +
>> +	if (level < 0) {
>> +		/* disable pad protection */
>> +		value |= PD_VREG;
>> +		value &= ~VREG_LEV(~0);
>> +		value &= ~VREG_DIR(~0);
>> +	} else {
>> +		if (dir == TEGRA_VBUS_SOURCE)
>> +			value |= VREG_DIR_OUT;
>> +		else if (dir == TEGRA_VBUS_SINK)
>> +			value |= VREG_DIR_IN;
>> +
>> +		value &= ~PD_VREG;
>> +		value &= ~VREG_DIR(~0);
>> +		value &= ~VREG_LEV(~0);
>> +		value |= VREG_LEV(level);
>> +	}
>> +
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>> +}
>> +
>> +
> There's an extra blank line above that can be dropped.
> 
Will remove the same.
>>   static int tegra186_xusb_padctl_vbus_override(struct tegra_xusb_padctl *padctl,
>>   					       bool status)
>>   {
>> @@ -935,6 +973,8 @@ static const struct tegra_xusb_padctl_ops tegra186_xusb_padctl_ops = {
>>   	.vbus_override = tegra186_xusb_padctl_vbus_override,
>>   	.utmi_pad_power_on = tegra_phy_xusb_utmi_pad_power_on,
>>   	.utmi_pad_power_down = tegra_phy_xusb_utmi_pad_power_down,
>> +	.utmi_pad_set_protection_level =
>> +			tegra186_xusb_padctl_utmi_pad_set_protection_level,
>>   };
>>   
>>   #if IS_ENABLED(CONFIG_ARCH_TEGRA_186_SOC)
>> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
>> index caf0890..7d84f1a 100644
>> --- a/drivers/phy/tegra/xusb-tegra210.c
>> +++ b/drivers/phy/tegra/xusb-tegra210.c
>> @@ -74,6 +74,8 @@
>>   #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_MASK 0x3
>>   #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_VAL 0x1
>>   #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18 (1 << 6)
>> +#define USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(x) (((x) & 0x3) << 7)
>> +#define USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_DIR(x) (((x) & 0x3) << 11)
>>   
>>   #define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x) (0x088 + (x) * 0x40)
>>   #define XUSB_PADCTL_USB2_OTG_PAD_CTL0_PD_ZI (1 << 29)
>> @@ -1116,6 +1118,33 @@ void tegra210_usb2_pad_power_down(struct phy *phy)
>>   	usb2->powered_on = false;
>>   }
>>   
>> +static void tegra210_xusb_padctl_utmi_pad_set_protection_level(
>> +				struct tegra_xusb_port *port, int level,
>> +				enum tegra_vbus_dir dir)
> Same comment as above.
> 
will update accordingly.
>> +{
>> +	u32 value;
>> +	struct tegra_xusb_padctl *padctl = port->padctl;
>> +	unsigned int index = port->index;
>> +
>> +	value = padctl_readl(padctl,
>> +			     XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>> +
>> +	if (level < 0) {
>> +		/* disable pad protection */
>> +		value |= XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18;
>> +		value &= USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(~0);
>> +		value &= ~USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_DIR(~0);
>> +	} else {
>> +		value &= ~XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18;
>> +		value &= ~USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_DIR(~0);
>> +		value &= USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(~0);
>> +		value |= USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(level);
>> +	}
>> +
>> +	padctl_writel(padctl, value,
>> +		      XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>> +}
>> +
>>   static int tegra210_usb2_phy_set_mode(struct phy *phy, enum phy_mode mode,
>>   				      int submode)
>>   {
>> @@ -2291,6 +2320,8 @@ static const struct tegra_xusb_padctl_ops tegra210_xusb_padctl_ops = {
>>   	.utmi_port_reset = tegra210_utmi_port_reset,
>>   	.utmi_pad_power_on = tegra210_usb2_pad_power_on,
>>   	.utmi_pad_power_down = tegra210_usb2_pad_power_down,
>> +	.utmi_pad_set_protection_level =
>> +			tegra210_xusb_padctl_utmi_pad_set_protection_level,
>>   };
>>   
>>   static const char * const tegra210_xusb_padctl_supply_names[] = {
>> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
>> index 6995fc4..79e96b0 100644
>> --- a/drivers/phy/tegra/xusb.h
>> +++ b/drivers/phy/tegra/xusb.h
>> @@ -259,6 +259,17 @@ to_sata_pad(struct tegra_xusb_pad *pad)
>>    */
>>   struct tegra_xusb_port_ops;
>>   
>> +/*
>> + * Tegra OTG port VBUS direction:
>> + * default (based on port capability) or
>> + * as source or sink
>> + */
>> +enum tegra_vbus_dir {
>> +	TEGRA_VBUS_DEFAULT,
>> +	TEGRA_VBUS_SOURCE,
>> +	TEGRA_VBUS_SINK
>> +};
> Can't we key off of something like the OTG mode? I thought we already
> carried that elsewhere.
> 
Unlike OTG mode, this is VBUS direction, which is different and not same 
as mode. Hence its a seperate structure.
>> +
>>   struct tegra_xusb_port {
>>   	struct tegra_xusb_padctl *padctl;
>>   	struct tegra_xusb_lane *lane;
>> @@ -398,6 +409,8 @@ struct tegra_xusb_padctl_ops {
>>   	int (*utmi_port_reset)(struct phy *phy);
>>   	void (*utmi_pad_power_on)(struct phy *phy);
>>   	void (*utmi_pad_power_down)(struct phy *phy);
>> +	void (*utmi_pad_set_protection_level)(struct tegra_xusb_port *port,
>> +					int max_ua, enum tegra_vbus_dir dir);
> You call the variable "max_ua" here but it's "level" in the
> implementations, which is slightly confusing. Please choose one and
> stick with it. Also, if it's a value in microamperes, perhaps just make
> it unsigned int?
> 
Its level actually, and needed as int, as it can carry negative value. 
Will syncronize the naming across all places.

> Thierry

[PATCH V2 6/8] phy: tegra: xusb: Add support for charger detect

[Nagarjuna Kristam]

Perform charger-detect operation if corresponding dt property is enabled.
Update usb-phy with the detected charger state and max current values.
Register charger-detect API's of usb-phy to provide needed functionalities.

Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
---
V2:
 - Patch re-based.
---
 drivers/phy/tegra/Makefile        |   2 +-
 drivers/phy/tegra/xusb-tegra-cd.c | 300 ++++++++++++++++++++++++++++++++++++++
 drivers/phy/tegra/xusb.c          |  80 ++++++++++
 drivers/phy/tegra/xusb.h          |   7 +
 4 files changed, 388 insertions(+), 1 deletion(-)
 create mode 100644 drivers/phy/tegra/xusb-tegra-cd.c

diff --git a/drivers/phy/tegra/Makefile b/drivers/phy/tegra/Makefile
index 89b8406..25ea9a9 100644
--- a/drivers/phy/tegra/Makefile
+++ b/drivers/phy/tegra/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_PHY_TEGRA_XUSB) += phy-tegra-xusb.o
 
-phy-tegra-xusb-y += xusb.o
+phy-tegra-xusb-y += xusb.o xusb-tegra-cd.o
 phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_124_SOC) += xusb-tegra124.o
 phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_132_SOC) += xusb-tegra124.o
 phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_210_SOC) += xusb-tegra210.o
diff --git a/drivers/phy/tegra/xusb-tegra-cd.c b/drivers/phy/tegra/xusb-tegra-cd.c
new file mode 100644
index 0000000..0fafc68
--- /dev/null
+++ b/drivers/phy/tegra/xusb-tegra-cd.c
@@ -0,0 +1,300 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020, NVIDIA CORPORATION.  All rights reserved.
+ */
+
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+
+#include "xusb.h"
+
+/* Data contact detection timeout */
+#define TDCD_TIMEOUT_MS                         400
+
+#define USB2_BATTERY_CHRG_OTGPADX_CTL0(x)       (0x80 + (x) * 0x40)
+#define  PD_CHG                                 (1 << 0)
+#define  VDCD_DET_FILTER_EN                     (1 << 4)
+#define  VDAT_DET                               (1 << 5)
+#define  VDAT_DET_FILTER_EN                     (1 << 8)
+#define  OP_SINK_EN                             (1 << 9)
+#define  OP_SRC_EN                              (1 << 10)
+#define  ON_SINK_EN                             (1 << 11)
+#define  ON_SRC_EN                              (1 << 12)
+#define  OP_I_SRC_EN                            (1 << 13)
+#define  ZIP_FILTER_EN                          (1 << 21)
+#define  ZIN_FILTER_EN                          (1 << 25)
+#define  DCD_DETECTED                           (1 << 26)
+
+#define USB2_BATTERY_CHRG_OTGPADX_CTL1(x)       (0x84 + (x) * 0x40)
+#define  PD_VREG                                (1 << 6)
+#define  VREG_LEV(x)                            (((x) & 0x3) << 7)
+#define  VREG_DIR(x)                            (((x) & 0x3) << 11)
+#define  VREG_DIR_IN                            VREG_DIR(1)
+#define  VREG_DIR_OUT                           VREG_DIR(2)
+#define  USBOP_RPD_OVRD                         (1 << 16)
+#define  USBOP_RPD_OVRD_VAL                     (1 << 17)
+#define  USBOP_RPU_OVRD                         (1 << 18)
+#define  USBOP_RPU_OVRD_VAL                     (1 << 19)
+#define  USBON_RPD_OVRD                         (1 << 20)
+#define  USBON_RPD_OVRD_VAL                     (1 << 21)
+#define  USBON_RPU_OVRD                         (1 << 22)
+#define  USBON_RPU_OVRD_VAL                     (1 << 23)
+
+#define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x)	(0x88 + (x) * 0x40)
+#define  USB2_OTG_PD2                           (1 << 27)
+#define  USB2_OTG_PD2_OVRD_EN                   (1 << 28)
+#define  USB2_OTG_PD_ZI                         (1 << 29)
+
+#define XUSB_PADCTL_USB2_BATTERY_CHRG_TDCD_DBNC_TIMER_0 (0x280)
+#define   TDCD_DBNC(x)                          (((x) & 0x7ff) << 0)
+
+static void tegra_xusb_padctl_set_debounce_time(
+				struct tegra_xusb_padctl *padctl, u32 val)
+{
+	u32 value;
+
+	value = padctl_readl(padctl,
+		XUSB_PADCTL_USB2_BATTERY_CHRG_TDCD_DBNC_TIMER_0);
+	value &= ~(TDCD_DBNC(0));
+	value |= TDCD_DBNC(val);
+	padctl_writel(padctl, value,
+		XUSB_PADCTL_USB2_BATTERY_CHRG_TDCD_DBNC_TIMER_0);
+}
+
+static void tegra_xusb_padctl_utmi_pad_charger_detect_on(
+				struct tegra_xusb_padctl *padctl, u32 index)
+{
+	u32 value;
+
+	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+	value &= ~USB2_OTG_PD_ZI;
+	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+
+	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+	value |= (USB2_OTG_PD2 | USB2_OTG_PD2_OVRD_EN);
+	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	value &= ~PD_CHG;
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+
+	/* Set DP/DN Pull up/down to zero by default */
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
+	value &= ~(USBOP_RPD_OVRD_VAL | USBOP_RPU_OVRD_VAL |
+		USBON_RPD_OVRD_VAL | USBON_RPU_OVRD_VAL);
+	value |= (USBOP_RPD_OVRD | USBOP_RPU_OVRD |
+		USBON_RPD_OVRD | USBON_RPU_OVRD);
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
+
+	/* Disable DP/DN as src/sink */
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	value &= ~(OP_SRC_EN | ON_SINK_EN |
+	ON_SRC_EN | OP_SINK_EN);
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+}
+
+static void tegra_xusb_padctl_utmi_pad_charger_detect_off(
+				struct tegra_xusb_padctl *padctl, u32 index)
+{
+	u32 value;
+
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
+	value &= ~(USBOP_RPD_OVRD | USBOP_RPU_OVRD |
+		 USBON_RPD_OVRD | USBON_RPU_OVRD);
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
+
+	/* power down necessary stuff */
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	value |= PD_CHG;
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+
+	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+	value &= ~(USB2_OTG_PD2 | USB2_OTG_PD2_OVRD_EN);
+	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
+}
+
+
+static void tegra_xusb_padctl_detect_filters(
+				struct tegra_xusb_padctl *padctl, u32 index,
+				bool on)
+{
+	u32 value;
+
+	if (on) {
+		value = padctl_readl(padctl,
+				     USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+		value |= (VDCD_DET_FILTER_EN | VDAT_DET_FILTER_EN |
+			  ZIP_FILTER_EN | ZIN_FILTER_EN);
+		padctl_writel(padctl, value,
+			      USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	} else {
+		value = padctl_readl(padctl,
+				     USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+		value &= ~(VDCD_DET_FILTER_EN | VDAT_DET_FILTER_EN |
+			   ZIP_FILTER_EN | ZIN_FILTER_EN);
+		padctl_writel(padctl, value,
+			      USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	}
+}
+
+static void tegra_xusb_padctl_utmi_pad_dcd(struct tegra_xusb_padctl *padctl,
+					      u32 index)
+{
+	u32 value;
+	int dcd_timeout_ms = 0;
+	bool ret = false;
+
+	/* Turn on IDP_SRC */
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	value |= OP_I_SRC_EN;
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+
+	/* Turn on D- pull-down resistor */
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
+	value |= USBON_RPD_OVRD_VAL;
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
+
+	/* Wait for TDCD_DBNC */
+	usleep_range(10000, 120000);
+
+	while (dcd_timeout_ms < TDCD_TIMEOUT_MS) {
+		value = padctl_readl(padctl,
+				     USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+		if (value & DCD_DETECTED) {
+			dev_dbg(padctl->dev, "USB2 port %d DCD successful\n",
+				index);
+			ret = true;
+			break;
+		}
+
+		usleep_range(20000, 22000);
+		dcd_timeout_ms += 22;
+	}
+
+	if (!ret)
+		dev_info(padctl->dev, "%s: DCD timeout %d ms\n", __func__,
+			 dcd_timeout_ms);
+
+	/* Turn off IP_SRC, clear DCD DETECTED*/
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	value &= ~OP_I_SRC_EN;
+	value |= DCD_DETECTED;
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+
+	/* Turn off D- pull-down resistor */
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
+	value &= ~USBON_RPD_OVRD_VAL;
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
+
+	dev_dbg(padctl->dev, "DCD: %d\n", ret);
+}
+
+static bool tegra_xusb_padctl_utmi_pad_primary_charger_detect(
+				struct tegra_xusb_padctl *padctl, u32 index)
+{
+	u32 value;
+	int ret = false;
+
+	/* data contact detection */
+	tegra_xusb_padctl_utmi_pad_dcd(padctl, index);
+
+	/* Source D+ to D- */
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	value |= OP_SRC_EN | ON_SINK_EN;
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+
+	/* Wait for TVDPSRC_ON */
+	msleep(40);
+
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	ret = !!(value & VDAT_DET);
+
+	/* Turn off OP_SRC, ON_SINK, clear VDAT, ZIN status change */
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	value &= ~(OP_SRC_EN | ON_SINK_EN);
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+
+	return ret;
+}
+
+static bool tegra_xusb_padctl_utmi_pad_secondary_charger_detect(
+				struct tegra_xusb_padctl *padctl, u32 index)
+{
+	u32 value;
+	bool ret = false;
+
+	/* Source D- to D+ */
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	value |= ON_SRC_EN | OP_SINK_EN;
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+
+	/* Wait for TVDPSRC_ON */
+	msleep(40);
+
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	ret = !(value & VDAT_DET);
+
+	/* Turn off ON_SRC, OP_SINK, clear VDAT, ZIP status change */
+	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+	value &= ~(ON_SRC_EN | OP_SINK_EN);
+	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
+
+	return ret;
+}
+
+enum usb_charger_type tegra_xusb_padctl_charger_detect(
+					  struct tegra_xusb_port *port)
+{
+	struct tegra_xusb_padctl *padctl = port->padctl;
+	struct phy *phy = port->lane->pad->lanes[port->index];
+	struct tegra_xusb_usb2_lane *usb2 = to_usb2_lane(port->lane);
+	u32 index = port->index;
+	enum usb_charger_type chrg_type;
+	bool pad_power_off = false;
+
+	mutex_lock(&padctl->lock);
+
+	if (!usb2->powered_on) {
+		padctl->soc->ops->utmi_pad_power_on(phy);
+		pad_power_off = true;
+	}
+
+	tegra_xusb_padctl_utmi_pad_charger_detect_on(padctl, index);
+	tegra_xusb_padctl_set_debounce_time(padctl, 0xa);
+	tegra_xusb_padctl_detect_filters(padctl, index, true);
+
+	if (tegra_xusb_padctl_utmi_pad_primary_charger_detect(padctl,
+								 index)) {
+		/*
+		 * wait 20ms (max of TVDMSRC_DIS) for D- to be disabled
+		 * from host side, before we perform secondary detection.
+		 * Some hosts may not respond well if we do secondary
+		 * detection right after primary detection.
+		 */
+		msleep(20);
+		if (tegra_xusb_padctl_utmi_pad_secondary_charger_detect(padctl,
+									index))
+			chrg_type = CDP_TYPE;
+		else
+			chrg_type = DCP_TYPE;
+	} else {
+		chrg_type = SDP_TYPE;
+	}
+
+	dev_dbg(&port->dev, "charger detected of type %d", chrg_type);
+
+	tegra_xusb_padctl_detect_filters(padctl, index, false);
+	tegra_xusb_padctl_utmi_pad_charger_detect_off(padctl, index);
+
+	if (pad_power_off)
+		padctl->soc->ops->utmi_pad_power_down(phy);
+
+	mutex_unlock(&padctl->lock);
+	return chrg_type;
+}
+
+MODULE_AUTHOR("Nagarjuna Kristam <nkristam@nvidia.com>");
+MODULE_DESCRIPTION("NVIDIA Tegra186 XUSB charger detect driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
index de4a46f..e505ac4 100644
--- a/drivers/phy/tegra/xusb.c
+++ b/drivers/phy/tegra/xusb.c
@@ -591,6 +591,50 @@ static enum usb_phy_events to_usb_phy_event(enum usb_role role)
 	}
 }
 
+#define VON_DIV2P0_DET BIT(0)
+#define VON_DIV2P7_DET BIT(1)
+#define VOP_DIV2P0_DET BIT(2)
+#define VOP_DIV2P7_DET BIT(3)
+
+#define VREG_CUR_LEVEL_0        500
+#define VREG_CUR_LEVEL_1        900
+#define VREG_CUR_LEVEL_2        1500
+#define VREG_CUR_LEVEL_3        2000
+
+#define IS_CUR_IN_RANGE(ma, low, high)  \
+	((ma >= VREG_CUR_LEVEL_##low) && (ma <= (VREG_CUR_LEVEL_##high - 1)))
+#define VREG_LVL(ma, level)     IS_CUR_IN_RANGE(ma, level, level + 1)
+
+static void tegra_xusb_padctl_vbus_pad_portection(struct tegra_xusb_port *port)
+{
+	struct tegra_xusb_padctl *padctl = port->padctl;
+	int level = 0;
+	enum tegra_vbus_dir dir = TEGRA_VBUS_SINK;
+	int max_ua, min_ua;
+
+	if (!padctl->soc->ops->utmi_pad_set_protection_level)
+		return;
+
+	usb_phy_get_charger_current(&port->usb_phy, &min_ua, &max_ua);
+
+	if (max_ua == 0) {
+		level = -1;
+		dir = TEGRA_VBUS_DEFAULT;
+	} else if (VREG_LVL(max_ua, 0)) {
+		level = 0;
+	} else if (VREG_LVL(max_ua, 1)) {
+		level = 1;
+	} else if (VREG_LVL(max_ua, 2)) {
+		level = 2;
+	} else if (max_ua >= VREG_CUR_LEVEL_3) {
+		level = 3;
+	} else {
+		return;
+	}
+
+	padctl->soc->ops->utmi_pad_set_protection_level(port, max_ua, dir);
+}
+
 static void tegra_xusb_usb_phy_work(struct work_struct *work)
 {
 	struct tegra_xusb_port *port = container_of(work,
@@ -598,6 +642,10 @@ static void tegra_xusb_usb_phy_work(struct work_struct *work)
 						    usb_phy_work);
 	enum usb_role role = usb_role_switch_get_role(port->usb_role_sw);
 
+	/* Set role to none, if charger is DCP type */
+	if (port->chrg_type == DCP_TYPE)
+		role = USB_ROLE_NONE;
+
 	usb_phy_set_event(&port->usb_phy, to_usb_phy_event(role));
 
 	dev_dbg(&port->dev, "%s(): calling notifier for role %s\n", __func__,
@@ -610,9 +658,26 @@ static int tegra_xusb_role_sw_set(struct usb_role_switch *sw,
 				  enum usb_role role)
 {
 	struct tegra_xusb_port *port = usb_role_switch_get_drvdata(sw);
+	enum usb_charger_state charger_state;
 
 	dev_dbg(&port->dev, "%s(): role %s\n", __func__, usb_roles[role]);
 
+	/* Do charger detect if role is Device and charger detect is enabled */
+	if (port->charger_detect) {
+		if (role == USB_ROLE_DEVICE)
+			port->chrg_type =
+					 tegra_xusb_padctl_charger_detect(port);
+		else
+			port->chrg_type = UNKNOWN_TYPE;
+
+		charger_state = (port->chrg_type == UNKNOWN_TYPE) ?
+				 USB_CHARGER_ABSENT : USB_CHARGER_PRESENT;
+
+		usb_phy_set_charger_state(&port->usb_phy, charger_state);
+
+		tegra_xusb_padctl_vbus_pad_portection(port);
+	}
+
 	schedule_work(&port->usb_phy_work);
 
 	return 0;
@@ -643,6 +708,14 @@ static int tegra_xusb_set_host(struct usb_otg *otg, struct usb_bus *host)
 	return 0;
 }
 
+static enum usb_charger_type tegra_xusb_charger_detect(struct usb_phy *usb_phy)
+{
+	struct tegra_xusb_port *port = container_of(usb_phy,
+						    struct tegra_xusb_port,
+						    usb_phy);
+
+	return port->chrg_type;
+}
 
 static int tegra_xusb_setup_usb_role_switch(struct tegra_xusb_port *port)
 {
@@ -693,6 +766,9 @@ static int tegra_xusb_setup_usb_role_switch(struct tegra_xusb_port *port)
 	port->usb_phy.otg->set_peripheral = tegra_xusb_set_peripheral;
 	port->usb_phy.otg->set_host = tegra_xusb_set_host;
 
+	if (port->charger_detect)
+		port->usb_phy.charger_detect = tegra_xusb_charger_detect;
+
 	err = usb_add_phy_dev(&port->usb_phy);
 	if (err < 0) {
 		dev_err(&port->dev, "Failed to add USB PHY: %d\n", err);
@@ -727,6 +803,10 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2)
 		usb2->mode = USB_DR_MODE_HOST;
 	}
 
+	if (port->padctl->soc->charger_detect &&
+	    of_property_read_bool(np, "nvidia,charger-detect"))
+		port->charger_detect = true;
+
 	/* usb-role-switch property is mandatory for OTG/Peripheral modes */
 	if (usb2->mode == USB_DR_MODE_PERIPHERAL ||
 	    usb2->mode == USB_DR_MODE_OTG) {
diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
index 79e96b0..714bca2 100644
--- a/drivers/phy/tegra/xusb.h
+++ b/drivers/phy/tegra/xusb.h
@@ -282,6 +282,9 @@ struct tegra_xusb_port {
 	struct work_struct usb_phy_work;
 	struct usb_phy usb_phy;
 
+	bool charger_detect;
+	enum usb_charger_type chrg_type;
+
 	const struct tegra_xusb_port_ops *ops;
 };
 
@@ -306,6 +309,9 @@ struct tegra_xusb_port *
 tegra_xusb_find_port(struct tegra_xusb_padctl *padctl, const char *type,
 		     unsigned int index);
 
+enum usb_charger_type tegra_xusb_padctl_charger_detect(
+					  struct tegra_xusb_port *port);
+
 struct tegra_xusb_usb2_port {
 	struct tegra_xusb_port base;
 
@@ -430,6 +436,7 @@ struct tegra_xusb_padctl_soc {
 	unsigned int num_supplies;
 	bool supports_gen2;
 	bool need_fake_usb3_port;
+	bool charger_detect;
 };
 
 struct tegra_xusb_padctl {
-- 
2.7.4

Re: [PATCH V2 6/8] phy: tegra: xusb: Add support for charger detect

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 20935 bytes --]

On Wed, Apr 15, 2020 at 01:55:06PM +0530, Nagarjuna Kristam wrote:
> Perform charger-detect operation if corresponding dt property is enabled.
> Update usb-phy with the detected charger state and max current values.
> Register charger-detect API's of usb-phy to provide needed functionalities.
> 
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V2:
>  - Patch re-based.
> ---
>  drivers/phy/tegra/Makefile        |   2 +-
>  drivers/phy/tegra/xusb-tegra-cd.c | 300 ++++++++++++++++++++++++++++++++++++++
>  drivers/phy/tegra/xusb.c          |  80 ++++++++++
>  drivers/phy/tegra/xusb.h          |   7 +
>  4 files changed, 388 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/phy/tegra/xusb-tegra-cd.c

Looks mostly good to me, just a few small nits.

> 
> diff --git a/drivers/phy/tegra/Makefile b/drivers/phy/tegra/Makefile
> index 89b8406..25ea9a9 100644
> --- a/drivers/phy/tegra/Makefile
> +++ b/drivers/phy/tegra/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_PHY_TEGRA_XUSB) += phy-tegra-xusb.o
>  
> -phy-tegra-xusb-y += xusb.o
> +phy-tegra-xusb-y += xusb.o xusb-tegra-cd.o

Splitting this off into a separate file seems a little arbitrary. If
adding this to xusb.c is really making things too unwieldy, I'd suggest
a different name. Perhaps something like xusb-charger.c, or just cd.c.
This is already in a directory called "tegra" and it's obvious also that
this is part of the XUSB PHY driver.

>  phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_124_SOC) += xusb-tegra124.o
>  phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_132_SOC) += xusb-tegra124.o
>  phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_210_SOC) += xusb-tegra210.o
> diff --git a/drivers/phy/tegra/xusb-tegra-cd.c b/drivers/phy/tegra/xusb-tegra-cd.c
> new file mode 100644
> index 0000000..0fafc68
> --- /dev/null
> +++ b/drivers/phy/tegra/xusb-tegra-cd.c
> @@ -0,0 +1,300 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020, NVIDIA CORPORATION.  All rights reserved.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
> +
> +#include "xusb.h"
> +
> +/* Data contact detection timeout */
> +#define TDCD_TIMEOUT_MS                         400
> +
> +#define USB2_BATTERY_CHRG_OTGPADX_CTL0(x)       (0x80 + (x) * 0x40)
> +#define  PD_CHG                                 (1 << 0)
> +#define  VDCD_DET_FILTER_EN                     (1 << 4)
> +#define  VDAT_DET                               (1 << 5)
> +#define  VDAT_DET_FILTER_EN                     (1 << 8)
> +#define  OP_SINK_EN                             (1 << 9)
> +#define  OP_SRC_EN                              (1 << 10)
> +#define  ON_SINK_EN                             (1 << 11)
> +#define  ON_SRC_EN                              (1 << 12)
> +#define  OP_I_SRC_EN                            (1 << 13)
> +#define  ZIP_FILTER_EN                          (1 << 21)
> +#define  ZIN_FILTER_EN                          (1 << 25)
> +#define  DCD_DETECTED                           (1 << 26)
> +
> +#define USB2_BATTERY_CHRG_OTGPADX_CTL1(x)       (0x84 + (x) * 0x40)
> +#define  PD_VREG                                (1 << 6)
> +#define  VREG_LEV(x)                            (((x) & 0x3) << 7)
> +#define  VREG_DIR(x)                            (((x) & 0x3) << 11)
> +#define  VREG_DIR_IN                            VREG_DIR(1)
> +#define  VREG_DIR_OUT                           VREG_DIR(2)
> +#define  USBOP_RPD_OVRD                         (1 << 16)
> +#define  USBOP_RPD_OVRD_VAL                     (1 << 17)
> +#define  USBOP_RPU_OVRD                         (1 << 18)
> +#define  USBOP_RPU_OVRD_VAL                     (1 << 19)
> +#define  USBON_RPD_OVRD                         (1 << 20)
> +#define  USBON_RPD_OVRD_VAL                     (1 << 21)
> +#define  USBON_RPU_OVRD                         (1 << 22)
> +#define  USBON_RPU_OVRD_VAL                     (1 << 23)
> +
> +#define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x)	(0x88 + (x) * 0x40)

There's a bit of a mix of spaces and tabs for indentation here. Just
pick one and stick with it. I think checkpatch will want you to use tabs
first and then spaces if additional indentation is needed.

> +#define  USB2_OTG_PD2                           (1 << 27)
> +#define  USB2_OTG_PD2_OVRD_EN                   (1 << 28)
> +#define  USB2_OTG_PD_ZI                         (1 << 29)
> +
> +#define XUSB_PADCTL_USB2_BATTERY_CHRG_TDCD_DBNC_TIMER_0 (0x280)
> +#define   TDCD_DBNC(x)                          (((x) & 0x7ff) << 0)
> +
> +static void tegra_xusb_padctl_set_debounce_time(
> +				struct tegra_xusb_padctl *padctl, u32 val)

Perhaps rename "val" to something like "debounce", or "delay" or
something to avoid the "val" vs. "value" confusion. Also, wrapping
should be after the return type. Same for most functions below.

> +{
> +	u32 value;
> +
> +	value = padctl_readl(padctl,
> +		XUSB_PADCTL_USB2_BATTERY_CHRG_TDCD_DBNC_TIMER_0);
> +	value &= ~(TDCD_DBNC(0));
> +	value |= TDCD_DBNC(val);
> +	padctl_writel(padctl, value,
> +		XUSB_PADCTL_USB2_BATTERY_CHRG_TDCD_DBNC_TIMER_0);
> +}
> +
> +static void tegra_xusb_padctl_utmi_pad_charger_detect_on(
> +				struct tegra_xusb_padctl *padctl, u32 index)

In general these function names are a little long for my taste. Charger
detection can only happen on UTMI pads, right? So could we just drop the
_utmi_pad infix in these? That doesn't give us much, but at least it
should make splitting this across multiple lines less awkward.

> +{
> +	u32 value;
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> +	value &= ~USB2_OTG_PD_ZI;
> +	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> +	value |= (USB2_OTG_PD2 | USB2_OTG_PD2_OVRD_EN);
> +	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> +
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	value &= ~PD_CHG;
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +
> +	/* Set DP/DN Pull up/down to zero by default */
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +	value &= ~(USBOP_RPD_OVRD_VAL | USBOP_RPU_OVRD_VAL |
> +		USBON_RPD_OVRD_VAL | USBON_RPU_OVRD_VAL);
> +	value |= (USBOP_RPD_OVRD | USBOP_RPU_OVRD |
> +		USBON_RPD_OVRD | USBON_RPU_OVRD);
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +
> +	/* Disable DP/DN as src/sink */
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	value &= ~(OP_SRC_EN | ON_SINK_EN |
> +	ON_SRC_EN | OP_SINK_EN);
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +}
> +
> +static void tegra_xusb_padctl_utmi_pad_charger_detect_off(
> +				struct tegra_xusb_padctl *padctl, u32 index)
> +{
> +	u32 value;
> +
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +	value &= ~(USBOP_RPD_OVRD | USBOP_RPU_OVRD |
> +		 USBON_RPD_OVRD | USBON_RPU_OVRD);
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +
> +	/* power down necessary stuff */
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	value |= PD_CHG;
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +
> +	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> +	value &= ~(USB2_OTG_PD2 | USB2_OTG_PD2_OVRD_EN);
> +	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> +}
> +
> +
> +static void tegra_xusb_padctl_detect_filters(
> +				struct tegra_xusb_padctl *padctl, u32 index,
> +				bool on)
> +{
> +	u32 value;
> +
> +	if (on) {
> +		value = padctl_readl(padctl,
> +				     USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +		value |= (VDCD_DET_FILTER_EN | VDAT_DET_FILTER_EN |
> +			  ZIP_FILTER_EN | ZIN_FILTER_EN);
> +		padctl_writel(padctl, value,
> +			      USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	} else {
> +		value = padctl_readl(padctl,
> +				     USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +		value &= ~(VDCD_DET_FILTER_EN | VDAT_DET_FILTER_EN |
> +			   ZIP_FILTER_EN | ZIN_FILTER_EN);
> +		padctl_writel(padctl, value,
> +			      USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	}
> +}
> +
> +static void tegra_xusb_padctl_utmi_pad_dcd(struct tegra_xusb_padctl *padctl,
> +					      u32 index)
> +{
> +	u32 value;
> +	int dcd_timeout_ms = 0;
> +	bool ret = false;
> +
> +	/* Turn on IDP_SRC */
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	value |= OP_I_SRC_EN;
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +
> +	/* Turn on D- pull-down resistor */
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +	value |= USBON_RPD_OVRD_VAL;
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +
> +	/* Wait for TDCD_DBNC */
> +	usleep_range(10000, 120000);

From the comment this looks like we're waiting for some hardware
condition. Can we somehow obtain this rather than implementing a fixed
sleep? Especially since the range here is so large.

> +
> +	while (dcd_timeout_ms < TDCD_TIMEOUT_MS) {
> +		value = padctl_readl(padctl,
> +				     USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +		if (value & DCD_DETECTED) {
> +			dev_dbg(padctl->dev, "USB2 port %d DCD successful\n",
> +				index);
> +			ret = true;
> +			break;
> +		}
> +
> +		usleep_range(20000, 22000);
> +		dcd_timeout_ms += 22;
> +	}

Can we just use a timed loop instead? You should be able to use
something like:

		unsigned int offset = USB2_BATTERY_CHRG_OTGPADX_CTL0(index);

		err = readl_poll_timeout(padctl->regs + offset, value,
					 value & DCD_DETECTED,
					 22000, TDCD_TIMEOUT_MS * 1000);

That's slightly suboptimal because it doesn't let you use padctl_readl,
but at least it gives you a standard way of doing this kind of loop.

> +
> +	if (!ret)
> +		dev_info(padctl->dev, "%s: DCD timeout %d ms\n", __func__,
> +			 dcd_timeout_ms);

Should this be a dev_err() or dev_warn()? Is this expected to happen?

> +
> +	/* Turn off IP_SRC, clear DCD DETECTED*/
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	value &= ~OP_I_SRC_EN;
> +	value |= DCD_DETECTED;
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +
> +	/* Turn off D- pull-down resistor */
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +	value &= ~USBON_RPD_OVRD_VAL;
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +
> +	dev_dbg(padctl->dev, "DCD: %d\n", ret);
> +}
> +
> +static bool tegra_xusb_padctl_utmi_pad_primary_charger_detect(
> +				struct tegra_xusb_padctl *padctl, u32 index)
> +{
> +	u32 value;
> +	int ret = false;

It doesn't look like there's a need to initialize this.

> +
> +	/* data contact detection */
> +	tegra_xusb_padctl_utmi_pad_dcd(padctl, index);
> +
> +	/* Source D+ to D- */
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	value |= OP_SRC_EN | ON_SINK_EN;
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +
> +	/* Wait for TVDPSRC_ON */
> +	msleep(40);

Again, is this a hardware condition that we can wait on by polling a
register?

> +
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	ret = !!(value & VDAT_DET);
> +
> +	/* Turn off OP_SRC, ON_SINK, clear VDAT, ZIN status change */
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	value &= ~(OP_SRC_EN | ON_SINK_EN);
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +
> +	return ret;
> +}
> +
> +static bool tegra_xusb_padctl_utmi_pad_secondary_charger_detect(
> +				struct tegra_xusb_padctl *padctl, u32 index)
> +{
> +	u32 value;
> +	bool ret = false;
> +
> +	/* Source D- to D+ */
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	value |= ON_SRC_EN | OP_SINK_EN;
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +
> +	/* Wait for TVDPSRC_ON */
> +	msleep(40);
> +
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	ret = !(value & VDAT_DET);
> +
> +	/* Turn off ON_SRC, OP_SINK, clear VDAT, ZIP status change */
> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +	value &= ~(ON_SRC_EN | OP_SINK_EN);
> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +
> +	return ret;
> +}

This seems to be mostly identical to the primary charger detect, so
perhaps this can be parameterized instead? I'm not generally opposed to
splitting functions up like this if they are fairly small, but in this
particular case, splitting up could make the name a lot shorter, and in
this case it's really quite excessive (I count 51 characters...) =)

> +
> +enum usb_charger_type tegra_xusb_padctl_charger_detect(
> +					  struct tegra_xusb_port *port)
> +{
> +	struct tegra_xusb_padctl *padctl = port->padctl;
> +	struct phy *phy = port->lane->pad->lanes[port->index];
> +	struct tegra_xusb_usb2_lane *usb2 = to_usb2_lane(port->lane);
> +	u32 index = port->index;
> +	enum usb_charger_type chrg_type;
> +	bool pad_power_off = false;
> +
> +	mutex_lock(&padctl->lock);
> +
> +	if (!usb2->powered_on) {
> +		padctl->soc->ops->utmi_pad_power_on(phy);
> +		pad_power_off = true;
> +	}
> +
> +	tegra_xusb_padctl_utmi_pad_charger_detect_on(padctl, index);
> +	tegra_xusb_padctl_set_debounce_time(padctl, 0xa);

Perhaps use 10 here because that's how we're usually used to read time
values.

> +	tegra_xusb_padctl_detect_filters(padctl, index, true);
> +
> +	if (tegra_xusb_padctl_utmi_pad_primary_charger_detect(padctl,
> +								 index)) {
> +		/*
> +		 * wait 20ms (max of TVDMSRC_DIS) for D- to be disabled
> +		 * from host side, before we perform secondary detection.
> +		 * Some hosts may not respond well if we do secondary
> +		 * detection right after primary detection.
> +		 */
> +		msleep(20);

Could use a blank line after this for readability.

> +		if (tegra_xusb_padctl_utmi_pad_secondary_charger_detect(padctl,
> +									index))
> +			chrg_type = CDP_TYPE;
> +		else
> +			chrg_type = DCP_TYPE;
> +	} else {
> +		chrg_type = SDP_TYPE;
> +	}
> +
> +	dev_dbg(&port->dev, "charger detected of type %d", chrg_type);

Do we have a string representation of this?

> +
> +	tegra_xusb_padctl_detect_filters(padctl, index, false);
> +	tegra_xusb_padctl_utmi_pad_charger_detect_off(padctl, index);
> +
> +	if (pad_power_off)
> +		padctl->soc->ops->utmi_pad_power_down(phy);
> +
> +	mutex_unlock(&padctl->lock);
> +	return chrg_type;
> +}
> +
> +MODULE_AUTHOR("Nagarjuna Kristam <nkristam@nvidia.com>");
> +MODULE_DESCRIPTION("NVIDIA Tegra186 XUSB charger detect driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
> index de4a46f..e505ac4 100644
> --- a/drivers/phy/tegra/xusb.c
> +++ b/drivers/phy/tegra/xusb.c
> @@ -591,6 +591,50 @@ static enum usb_phy_events to_usb_phy_event(enum usb_role role)
>  	}
>  }
>  
> +#define VON_DIV2P0_DET BIT(0)
> +#define VON_DIV2P7_DET BIT(1)
> +#define VOP_DIV2P0_DET BIT(2)
> +#define VOP_DIV2P7_DET BIT(3)
> +
> +#define VREG_CUR_LEVEL_0        500
> +#define VREG_CUR_LEVEL_1        900
> +#define VREG_CUR_LEVEL_2        1500
> +#define VREG_CUR_LEVEL_3        2000
> +
> +#define IS_CUR_IN_RANGE(ma, low, high)  \
> +	((ma >= VREG_CUR_LEVEL_##low) && (ma <= (VREG_CUR_LEVEL_##high - 1)))
> +#define VREG_LVL(ma, level)     IS_CUR_IN_RANGE(ma, level, level + 1)
> +
> +static void tegra_xusb_padctl_vbus_pad_portection(struct tegra_xusb_port *port)
> +{
> +	struct tegra_xusb_padctl *padctl = port->padctl;
> +	int level = 0;
> +	enum tegra_vbus_dir dir = TEGRA_VBUS_SINK;
> +	int max_ua, min_ua;
> +
> +	if (!padctl->soc->ops->utmi_pad_set_protection_level)
> +		return;
> +
> +	usb_phy_get_charger_current(&port->usb_phy, &min_ua, &max_ua);
> +
> +	if (max_ua == 0) {
> +		level = -1;
> +		dir = TEGRA_VBUS_DEFAULT;
> +	} else if (VREG_LVL(max_ua, 0)) {
> +		level = 0;
> +	} else if (VREG_LVL(max_ua, 1)) {
> +		level = 1;
> +	} else if (VREG_LVL(max_ua, 2)) {
> +		level = 2;
> +	} else if (max_ua >= VREG_CUR_LEVEL_3) {
> +		level = 3;
> +	} else {
> +		return;
> +	}
> +
> +	padctl->soc->ops->utmi_pad_set_protection_level(port, max_ua, dir);
> +}

level seems to never be used in the above. Instead you just pass max_ua
to the set protection level callback.

> +
>  static void tegra_xusb_usb_phy_work(struct work_struct *work)
>  {
>  	struct tegra_xusb_port *port = container_of(work,
> @@ -598,6 +642,10 @@ static void tegra_xusb_usb_phy_work(struct work_struct *work)
>  						    usb_phy_work);
>  	enum usb_role role = usb_role_switch_get_role(port->usb_role_sw);
>  
> +	/* Set role to none, if charger is DCP type */
> +	if (port->chrg_type == DCP_TYPE)
> +		role = USB_ROLE_NONE;
> +
>  	usb_phy_set_event(&port->usb_phy, to_usb_phy_event(role));
>  
>  	dev_dbg(&port->dev, "%s(): calling notifier for role %s\n", __func__,
> @@ -610,9 +658,26 @@ static int tegra_xusb_role_sw_set(struct usb_role_switch *sw,
>  				  enum usb_role role)
>  {
>  	struct tegra_xusb_port *port = usb_role_switch_get_drvdata(sw);
> +	enum usb_charger_state charger_state;
>  
>  	dev_dbg(&port->dev, "%s(): role %s\n", __func__, usb_roles[role]);
>  
> +	/* Do charger detect if role is Device and charger detect is enabled */
> +	if (port->charger_detect) {
> +		if (role == USB_ROLE_DEVICE)
> +			port->chrg_type =
> +					 tegra_xusb_padctl_charger_detect(port);
> +		else
> +			port->chrg_type = UNKNOWN_TYPE;
> +
> +		charger_state = (port->chrg_type == UNKNOWN_TYPE) ?
> +				 USB_CHARGER_ABSENT : USB_CHARGER_PRESENT;
> +
> +		usb_phy_set_charger_state(&port->usb_phy, charger_state);
> +
> +		tegra_xusb_padctl_vbus_pad_portection(port);
> +	}
> +
>  	schedule_work(&port->usb_phy_work);
>  
>  	return 0;
> @@ -643,6 +708,14 @@ static int tegra_xusb_set_host(struct usb_otg *otg, struct usb_bus *host)
>  	return 0;
>  }
>  
> +static enum usb_charger_type tegra_xusb_charger_detect(struct usb_phy *usb_phy)
> +{
> +	struct tegra_xusb_port *port = container_of(usb_phy,
> +						    struct tegra_xusb_port,
> +						    usb_phy);
> +
> +	return port->chrg_type;
> +}
>  
>  static int tegra_xusb_setup_usb_role_switch(struct tegra_xusb_port *port)
>  {
> @@ -693,6 +766,9 @@ static int tegra_xusb_setup_usb_role_switch(struct tegra_xusb_port *port)
>  	port->usb_phy.otg->set_peripheral = tegra_xusb_set_peripheral;
>  	port->usb_phy.otg->set_host = tegra_xusb_set_host;
>  
> +	if (port->charger_detect)
> +		port->usb_phy.charger_detect = tegra_xusb_charger_detect;
> +
>  	err = usb_add_phy_dev(&port->usb_phy);
>  	if (err < 0) {
>  		dev_err(&port->dev, "Failed to add USB PHY: %d\n", err);
> @@ -727,6 +803,10 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2)
>  		usb2->mode = USB_DR_MODE_HOST;
>  	}
>  
> +	if (port->padctl->soc->charger_detect &&
> +	    of_property_read_bool(np, "nvidia,charger-detect"))
> +		port->charger_detect = true;
> +
>  	/* usb-role-switch property is mandatory for OTG/Peripheral modes */
>  	if (usb2->mode == USB_DR_MODE_PERIPHERAL ||
>  	    usb2->mode == USB_DR_MODE_OTG) {
> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
> index 79e96b0..714bca2 100644
> --- a/drivers/phy/tegra/xusb.h
> +++ b/drivers/phy/tegra/xusb.h
> @@ -282,6 +282,9 @@ struct tegra_xusb_port {
>  	struct work_struct usb_phy_work;
>  	struct usb_phy usb_phy;
>  
> +	bool charger_detect;
> +	enum usb_charger_type chrg_type;
> +
>  	const struct tegra_xusb_port_ops *ops;
>  };
>  
> @@ -306,6 +309,9 @@ struct tegra_xusb_port *
>  tegra_xusb_find_port(struct tegra_xusb_padctl *padctl, const char *type,
>  		     unsigned int index);
>  
> +enum usb_charger_type tegra_xusb_padctl_charger_detect(
> +					  struct tegra_xusb_port *port);
> +
>  struct tegra_xusb_usb2_port {
>  	struct tegra_xusb_port base;
>  
> @@ -430,6 +436,7 @@ struct tegra_xusb_padctl_soc {
>  	unsigned int num_supplies;
>  	bool supports_gen2;
>  	bool need_fake_usb3_port;
> +	bool charger_detect;

Perhaps make this "supports_charger_detection" because it being in the
SoC structure means that it's a capability. "charger_detect" makes it
look like an option that we've enabled or not. That's what struct
tegra_xusb_port.charger_detect already is.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V2 6/8] phy: tegra: xusb: Add support for charger detect

[Nagarjuna Kristam]

 >On 28-04-2020 16:25, Thierry Reding wrote:
>> On Wed, Apr 15, 2020 at 01:55:06PM +0530, Nagarjuna Kristam wrote:
>> Perform charger-detect operation if corresponding dt property is enabled.
>> Update usb-phy with the detected charger state and max current values.
>> Register charger-detect API's of usb-phy to provide needed functionalities.
>>
>> Signed-off-by: Nagarjuna Kristam<nkristam@nvidia.com>
>> ---
>> V2:
>>   - Patch re-based.
>> ---
>>   drivers/phy/tegra/Makefile        |   2 +-
>>   drivers/phy/tegra/xusb-tegra-cd.c | 300 ++++++++++++++++++++++++++++++++++++++
>>   drivers/phy/tegra/xusb.c          |  80 ++++++++++
>>   drivers/phy/tegra/xusb.h          |   7 +
>>   4 files changed, 388 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/phy/tegra/xusb-tegra-cd.c
> Looks mostly good to me, just a few small nits.
> 
>> diff --git a/drivers/phy/tegra/Makefile b/drivers/phy/tegra/Makefile
>> index 89b8406..25ea9a9 100644
>> --- a/drivers/phy/tegra/Makefile
>> +++ b/drivers/phy/tegra/Makefile
>> @@ -1,7 +1,7 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   obj-$(CONFIG_PHY_TEGRA_XUSB) += phy-tegra-xusb.o
>>   
>> -phy-tegra-xusb-y += xusb.o
>> +phy-tegra-xusb-y += xusb.o xusb-tegra-cd.o
> Splitting this off into a separate file seems a little arbitrary. If
> adding this to xusb.c is really making things too unwieldy, I'd suggest
> a different name. Perhaps something like xusb-charger.c, or just cd.c.
> This is already in a directory called "tegra" and it's obvious also that
> this is part of the XUSB PHY driver.
> 
Will add as cd.c
>>   phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_124_SOC) += xusb-tegra124.o
>>   phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_132_SOC) += xusb-tegra124.o
>>   phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_210_SOC) += xusb-tegra210.o
>> diff --git a/drivers/phy/tegra/xusb-tegra-cd.c b/drivers/phy/tegra/xusb-tegra-cd.c
>> new file mode 100644
>> index 0000000..0fafc68
>> --- /dev/null
>> +++ b/drivers/phy/tegra/xusb-tegra-cd.c
>> @@ -0,0 +1,300 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2020, NVIDIA CORPORATION.  All rights reserved.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/phy/phy.h>
>> +
>> +#include "xusb.h"
>> +
>> +/* Data contact detection timeout */
>> +#define TDCD_TIMEOUT_MS                         400
>> +
>> +#define USB2_BATTERY_CHRG_OTGPADX_CTL0(x)       (0x80 + (x) * 0x40)
>> +#define  PD_CHG                                 (1 << 0)
>> +#define  VDCD_DET_FILTER_EN                     (1 << 4)
>> +#define  VDAT_DET                               (1 << 5)
>> +#define  VDAT_DET_FILTER_EN                     (1 << 8)
>> +#define  OP_SINK_EN                             (1 << 9)
>> +#define  OP_SRC_EN                              (1 << 10)
>> +#define  ON_SINK_EN                             (1 << 11)
>> +#define  ON_SRC_EN                              (1 << 12)
>> +#define  OP_I_SRC_EN                            (1 << 13)
>> +#define  ZIP_FILTER_EN                          (1 << 21)
>> +#define  ZIN_FILTER_EN                          (1 << 25)
>> +#define  DCD_DETECTED                           (1 << 26)
>> +
>> +#define USB2_BATTERY_CHRG_OTGPADX_CTL1(x)       (0x84 + (x) * 0x40)
>> +#define  PD_VREG                                (1 << 6)
>> +#define  VREG_LEV(x)                            (((x) & 0x3) << 7)
>> +#define  VREG_DIR(x)                            (((x) & 0x3) << 11)
>> +#define  VREG_DIR_IN                            VREG_DIR(1)
>> +#define  VREG_DIR_OUT                           VREG_DIR(2)
>> +#define  USBOP_RPD_OVRD                         (1 << 16)
>> +#define  USBOP_RPD_OVRD_VAL                     (1 << 17)
>> +#define  USBOP_RPU_OVRD                         (1 << 18)
>> +#define  USBOP_RPU_OVRD_VAL                     (1 << 19)
>> +#define  USBON_RPD_OVRD                         (1 << 20)
>> +#define  USBON_RPD_OVRD_VAL                     (1 << 21)
>> +#define  USBON_RPU_OVRD                         (1 << 22)
>> +#define  USBON_RPU_OVRD_VAL                     (1 << 23)
>> +
>> +#define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x)	(0x88 + (x) * 0x40)
> There's a bit of a mix of spaces and tabs for indentation here. Just
> pick one and stick with it. I think checkpatch will want you to use tabs
> first and then spaces if additional indentation is needed.
> 
Will update accordingly
>> +#define  USB2_OTG_PD2                           (1 << 27)
>> +#define  USB2_OTG_PD2_OVRD_EN                   (1 << 28)
>> +#define  USB2_OTG_PD_ZI                         (1 << 29)
>> +
>> +#define XUSB_PADCTL_USB2_BATTERY_CHRG_TDCD_DBNC_TIMER_0 (0x280)
>> +#define   TDCD_DBNC(x)                          (((x) & 0x7ff) << 0)
>> +
>> +static void tegra_xusb_padctl_set_debounce_time(
>> +				struct tegra_xusb_padctl *padctl, u32 val)
> Perhaps rename "val" to something like "debounce", or "delay" or
> something to avoid the "val" vs. "value" confusion. Also, wrapping
> should be after the return type. Same for most functions below.
> 
Will update accordingly
>> +{
>> +	u32 value;
>> +
>> +	value = padctl_readl(padctl,
>> +		XUSB_PADCTL_USB2_BATTERY_CHRG_TDCD_DBNC_TIMER_0);
>> +	value &= ~(TDCD_DBNC(0));
>> +	value |= TDCD_DBNC(val);
>> +	padctl_writel(padctl, value,
>> +		XUSB_PADCTL_USB2_BATTERY_CHRG_TDCD_DBNC_TIMER_0);
>> +}
>> +
>> +static void tegra_xusb_padctl_utmi_pad_charger_detect_on(
>> +				struct tegra_xusb_padctl *padctl, u32 index)
> In general these function names are a little long for my taste. Charger
> detection can only happen on UTMI pads, right? So could we just drop the
> _utmi_pad infix in these? That doesn't give us much, but at least it
> should make splitting this across multiple lines less awkward.
> 
will remove utmi_pad_ and shorten the functions.
>> +{
>> +	u32 value;
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
>> +	value &= ~USB2_OTG_PD_ZI;
>> +	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
>> +	value |= (USB2_OTG_PD2 | USB2_OTG_PD2_OVRD_EN);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
>> +
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	value &= ~PD_CHG;
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +
>> +	/* Set DP/DN Pull up/down to zero by default */
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>> +	value &= ~(USBOP_RPD_OVRD_VAL | USBOP_RPU_OVRD_VAL |
>> +		USBON_RPD_OVRD_VAL | USBON_RPU_OVRD_VAL);
>> +	value |= (USBOP_RPD_OVRD | USBOP_RPU_OVRD |
>> +		USBON_RPD_OVRD | USBON_RPU_OVRD);
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>> +
>> +	/* Disable DP/DN as src/sink */
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	value &= ~(OP_SRC_EN | ON_SINK_EN |
>> +	ON_SRC_EN | OP_SINK_EN);
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +}
>> +
>> +static void tegra_xusb_padctl_utmi_pad_charger_detect_off(
>> +				struct tegra_xusb_padctl *padctl, u32 index)
>> +{
>> +	u32 value;
>> +
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>> +	value &= ~(USBOP_RPD_OVRD | USBOP_RPU_OVRD |
>> +		 USBON_RPD_OVRD | USBON_RPU_OVRD);
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>> +
>> +	/* power down necessary stuff */
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	value |= PD_CHG;
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +
>> +	value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
>> +	value &= ~(USB2_OTG_PD2 | USB2_OTG_PD2_OVRD_EN);
>> +	padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
>> +}
>> +
>> +
>> +static void tegra_xusb_padctl_detect_filters(
>> +				struct tegra_xusb_padctl *padctl, u32 index,
>> +				bool on)
>> +{
>> +	u32 value;
>> +
>> +	if (on) {
>> +		value = padctl_readl(padctl,
>> +				     USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +		value |= (VDCD_DET_FILTER_EN | VDAT_DET_FILTER_EN |
>> +			  ZIP_FILTER_EN | ZIN_FILTER_EN);
>> +		padctl_writel(padctl, value,
>> +			      USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	} else {
>> +		value = padctl_readl(padctl,
>> +				     USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +		value &= ~(VDCD_DET_FILTER_EN | VDAT_DET_FILTER_EN |
>> +			   ZIP_FILTER_EN | ZIN_FILTER_EN);
>> +		padctl_writel(padctl, value,
>> +			      USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	}
>> +}
>> +
>> +static void tegra_xusb_padctl_utmi_pad_dcd(struct tegra_xusb_padctl *padctl,
>> +					      u32 index)
>> +{
>> +	u32 value;
>> +	int dcd_timeout_ms = 0;
>> +	bool ret = false;
>> +
>> +	/* Turn on IDP_SRC */
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	value |= OP_I_SRC_EN;
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +
>> +	/* Turn on D- pull-down resistor */
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>> +	value |= USBON_RPD_OVRD_VAL;
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>> +
>> +	/* Wait for TDCD_DBNC */
>> +	usleep_range(10000, 120000);
>  From the comment this looks like we're waiting for some hardware
> condition. Can we somehow obtain this rather than implementing a fixed
> sleep? Especially since the range here is so large.
> 
As per data sheet we need to wait for 10 micro seconds as settle time.
>> +
>> +	while (dcd_timeout_ms < TDCD_TIMEOUT_MS) {
>> +		value = padctl_readl(padctl,
>> +				     USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +		if (value & DCD_DETECTED) {
>> +			dev_dbg(padctl->dev, "USB2 port %d DCD successful\n",
>> +				index);
>> +			ret = true;
>> +			break;
>> +		}
>> +
>> +		usleep_range(20000, 22000);
>> +		dcd_timeout_ms += 22;
>> +	}
> Can we just use a timed loop instead? You should be able to use
> something like:
Will update using single API accordingly
> 
> 		unsigned int offset = USB2_BATTERY_CHRG_OTGPADX_CTL0(index);
> 
> 		err = readl_poll_timeout(padctl->regs + offset, value,
> 					 value & DCD_DETECTED,
> 					 22000, TDCD_TIMEOUT_MS * 1000);
> 
> That's slightly suboptimal because it doesn't let you use padctl_readl,
> but at least it gives you a standard way of doing this kind of loop.
> 
>> +
>> +	if (!ret)
>> +		dev_info(padctl->dev, "%s: DCD timeout %d ms\n", __func__,
>> +			 dcd_timeout_ms);
> Should this be a dev_err() or dev_warn()? Is this expected to happen?
> 
In general shouldnot happen, will mark as warn.
>> +
>> +	/* Turn off IP_SRC, clear DCD DETECTED*/
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	value &= ~OP_I_SRC_EN;
>> +	value |= DCD_DETECTED;
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +
>> +	/* Turn off D- pull-down resistor */
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>> +	value &= ~USBON_RPD_OVRD_VAL;
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>> +
>> +	dev_dbg(padctl->dev, "DCD: %d\n", ret);
>> +}
>> +
>> +static bool tegra_xusb_padctl_utmi_pad_primary_charger_detect(
>> +				struct tegra_xusb_padctl *padctl, u32 index)
>> +{
>> +	u32 value;
>> +	int ret = false;
> It doesn't look like there's a need to initialize this.
> 
will remove.
>> +
>> +	/* data contact detection */
>> +	tegra_xusb_padctl_utmi_pad_dcd(padctl, index);
>> +
>> +	/* Source D+ to D- */
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	value |= OP_SRC_EN | ON_SINK_EN;
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +
>> +	/* Wait for TVDPSRC_ON */
>> +	msleep(40);
> Again, is this a hardware condition that we can wait on by polling a
> register?
> 
It HW settle time before reading registers.
>> +
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	ret = !!(value & VDAT_DET);
>> +
>> +	/* Turn off OP_SRC, ON_SINK, clear VDAT, ZIN status change */
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	value &= ~(OP_SRC_EN | ON_SINK_EN);
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +
>> +	return ret;
>> +}
>> +
>> +static bool tegra_xusb_padctl_utmi_pad_secondary_charger_detect(
>> +				struct tegra_xusb_padctl *padctl, u32 index)
>> +{
>> +	u32 value;
>> +	bool ret = false;
>> +
>> +	/* Source D- to D+ */
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	value |= ON_SRC_EN | OP_SINK_EN;
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +
>> +	/* Wait for TVDPSRC_ON */
>> +	msleep(40);
>> +
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	ret = !(value & VDAT_DET);
>> +
>> +	/* Turn off ON_SRC, OP_SINK, clear VDAT, ZIP status change */
>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +	value &= ~(ON_SRC_EN | OP_SINK_EN);
>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>> +
>> +	return ret;
>> +}
> This seems to be mostly identical to the primary charger detect, so
> perhaps this can be parameterized instead? I'm not generally opposed to
> splitting functions up like this if they are fairly small, but in this
> particular case, splitting up could make the name a lot shorter, and in
> this case it's really quite excessive (I count 51 characters...) =)
> Will update accordingly
>> +
>> +enum usb_charger_type tegra_xusb_padctl_charger_detect(
>> +					  struct tegra_xusb_port *port)
>> +{
>> +	struct tegra_xusb_padctl *padctl = port->padctl;
>> +	struct phy *phy = port->lane->pad->lanes[port->index];
>> +	struct tegra_xusb_usb2_lane *usb2 = to_usb2_lane(port->lane);
>> +	u32 index = port->index;
>> +	enum usb_charger_type chrg_type;
>> +	bool pad_power_off = false;
>> +
>> +	mutex_lock(&padctl->lock);
>> +
>> +	if (!usb2->powered_on) {
>> +		padctl->soc->ops->utmi_pad_power_on(phy);
>> +		pad_power_off = true;
>> +	}
>> +
>> +	tegra_xusb_padctl_utmi_pad_charger_detect_on(padctl, index);
>> +	tegra_xusb_padctl_set_debounce_time(padctl, 0xa);
> Perhaps use 10 here because that's how we're usually used to read time
> values.
> 
Will update accordingly
>> +	tegra_xusb_padctl_detect_filters(padctl, index, true);
>> +
>> +	if (tegra_xusb_padctl_utmi_pad_primary_charger_detect(padctl,
>> +								 index)) {
>> +		/*
>> +		 * wait 20ms (max of TVDMSRC_DIS) for D- to be disabled
>> +		 * from host side, before we perform secondary detection.
>> +		 * Some hosts may not respond well if we do secondary
>> +		 * detection right after primary detection.
>> +		 */
>> +		msleep(20);
> Could use a blank line after this for readability.
> 
Will update accordingly
>> +		if (tegra_xusb_padctl_utmi_pad_secondary_charger_detect(padctl,
>> +									index))
>> +			chrg_type = CDP_TYPE;
>> +		else
>> +			chrg_type = DCP_TYPE;
>> +	} else {
>> +		chrg_type = SDP_TYPE;
>> +	}
>> +
>> +	dev_dbg(&port->dev, "charger detected of type %d", chrg_type);
> Do we have a string representation of this?
> 
No String representation available. Shall i add one for wasy reading ?
>> +
>> +	tegra_xusb_padctl_detect_filters(padctl, index, false);
>> +	tegra_xusb_padctl_utmi_pad_charger_detect_off(padctl, index);
>> +
>> +	if (pad_power_off)
>> +		padctl->soc->ops->utmi_pad_power_down(phy);
>> +
>> +	mutex_unlock(&padctl->lock);
>> +	return chrg_type;
>> +}
>> +
>> +MODULE_AUTHOR("Nagarjuna Kristam<nkristam@nvidia.com>");
>> +MODULE_DESCRIPTION("NVIDIA Tegra186 XUSB charger detect driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
>> index de4a46f..e505ac4 100644
>> --- a/drivers/phy/tegra/xusb.c
>> +++ b/drivers/phy/tegra/xusb.c
>> @@ -591,6 +591,50 @@ static enum usb_phy_events to_usb_phy_event(enum usb_role role)
>>   	}
>>   }
>>   
>> +#define VON_DIV2P0_DET BIT(0)
>> +#define VON_DIV2P7_DET BIT(1)
>> +#define VOP_DIV2P0_DET BIT(2)
>> +#define VOP_DIV2P7_DET BIT(3)
>> +
>> +#define VREG_CUR_LEVEL_0        500
>> +#define VREG_CUR_LEVEL_1        900
>> +#define VREG_CUR_LEVEL_2        1500
>> +#define VREG_CUR_LEVEL_3        2000
>> +
>> +#define IS_CUR_IN_RANGE(ma, low, high)  \
>> +	((ma >= VREG_CUR_LEVEL_##low) && (ma <= (VREG_CUR_LEVEL_##high - 1)))
>> +#define VREG_LVL(ma, level)     IS_CUR_IN_RANGE(ma, level, level + 1)
>> +
>> +static void tegra_xusb_padctl_vbus_pad_portection(struct tegra_xusb_port *port)
>> +{
>> +	struct tegra_xusb_padctl *padctl = port->padctl;
>> +	int level = 0;
>> +	enum tegra_vbus_dir dir = TEGRA_VBUS_SINK;
>> +	int max_ua, min_ua;
>> +
>> +	if (!padctl->soc->ops->utmi_pad_set_protection_level)
>> +		return;
>> +
>> +	usb_phy_get_charger_current(&port->usb_phy, &min_ua, &max_ua);
>> +
>> +	if (max_ua == 0) {
>> +		level = -1;
>> +		dir = TEGRA_VBUS_DEFAULT;
>> +	} else if (VREG_LVL(max_ua, 0)) {
>> +		level = 0;
>> +	} else if (VREG_LVL(max_ua, 1)) {
>> +		level = 1;
>> +	} else if (VREG_LVL(max_ua, 2)) {
>> +		level = 2;
>> +	} else if (max_ua >= VREG_CUR_LEVEL_3) {
>> +		level = 3;
>> +	} else {
>> +		return;
>> +	}
>> +
>> +	padctl->soc->ops->utmi_pad_set_protection_level(port, max_ua, dir);
>> +}
> level seems to never be used in the above. Instead you just pass max_ua
> to the set protection level callback.
> 
Will remove accordingly
>> +
>>   static void tegra_xusb_usb_phy_work(struct work_struct *work)
>>   {
>>   	struct tegra_xusb_port *port = container_of(work,
>> @@ -598,6 +642,10 @@ static void tegra_xusb_usb_phy_work(struct work_struct *work)
>>   						    usb_phy_work);
>>   	enum usb_role role = usb_role_switch_get_role(port->usb_role_sw);
>>   
>> +	/* Set role to none, if charger is DCP type */
>> +	if (port->chrg_type == DCP_TYPE)
>> +		role = USB_ROLE_NONE;
>> +
>>   	usb_phy_set_event(&port->usb_phy, to_usb_phy_event(role));
>>   
>>   	dev_dbg(&port->dev, "%s(): calling notifier for role %s\n", __func__,
>> @@ -610,9 +658,26 @@ static int tegra_xusb_role_sw_set(struct usb_role_switch *sw,
>>   				  enum usb_role role)
>>   {
>>   	struct tegra_xusb_port *port = usb_role_switch_get_drvdata(sw);
>> +	enum usb_charger_state charger_state;
>>   
>>   	dev_dbg(&port->dev, "%s(): role %s\n", __func__, usb_roles[role]);
>>   
>> +	/* Do charger detect if role is Device and charger detect is enabled */
>> +	if (port->charger_detect) {
>> +		if (role == USB_ROLE_DEVICE)
>> +			port->chrg_type =
>> +					 tegra_xusb_padctl_charger_detect(port);
>> +		else
>> +			port->chrg_type = UNKNOWN_TYPE;
>> +
>> +		charger_state = (port->chrg_type == UNKNOWN_TYPE) ?
>> +				 USB_CHARGER_ABSENT : USB_CHARGER_PRESENT;
>> +
>> +		usb_phy_set_charger_state(&port->usb_phy, charger_state);
>> +
>> +		tegra_xusb_padctl_vbus_pad_portection(port);
>> +	}
>> +
>>   	schedule_work(&port->usb_phy_work);
>>   
>>   	return 0;
>> @@ -643,6 +708,14 @@ static int tegra_xusb_set_host(struct usb_otg *otg, struct usb_bus *host)
>>   	return 0;
>>   }
>>   
>> +static enum usb_charger_type tegra_xusb_charger_detect(struct usb_phy *usb_phy)
>> +{
>> +	struct tegra_xusb_port *port = container_of(usb_phy,
>> +						    struct tegra_xusb_port,
>> +						    usb_phy);
>> +
>> +	return port->chrg_type;
>> +}
>>   
>>   static int tegra_xusb_setup_usb_role_switch(struct tegra_xusb_port *port)
>>   {
>> @@ -693,6 +766,9 @@ static int tegra_xusb_setup_usb_role_switch(struct tegra_xusb_port *port)
>>   	port->usb_phy.otg->set_peripheral = tegra_xusb_set_peripheral;
>>   	port->usb_phy.otg->set_host = tegra_xusb_set_host;
>>   
>> +	if (port->charger_detect)
>> +		port->usb_phy.charger_detect = tegra_xusb_charger_detect;
>> +
>>   	err = usb_add_phy_dev(&port->usb_phy);
>>   	if (err < 0) {
>>   		dev_err(&port->dev, "Failed to add USB PHY: %d\n", err);
>> @@ -727,6 +803,10 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2)
>>   		usb2->mode = USB_DR_MODE_HOST;
>>   	}
>>   
>> +	if (port->padctl->soc->charger_detect &&
>> +	    of_property_read_bool(np, "nvidia,charger-detect"))
>> +		port->charger_detect = true;
>> +
>>   	/* usb-role-switch property is mandatory for OTG/Peripheral modes */
>>   	if (usb2->mode == USB_DR_MODE_PERIPHERAL ||
>>   	    usb2->mode == USB_DR_MODE_OTG) {
>> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
>> index 79e96b0..714bca2 100644
>> --- a/drivers/phy/tegra/xusb.h
>> +++ b/drivers/phy/tegra/xusb.h
>> @@ -282,6 +282,9 @@ struct tegra_xusb_port {
>>   	struct work_struct usb_phy_work;
>>   	struct usb_phy usb_phy;
>>   
>> +	bool charger_detect;
>> +	enum usb_charger_type chrg_type;
>> +
>>   	const struct tegra_xusb_port_ops *ops;
>>   };
>>   
>> @@ -306,6 +309,9 @@ struct tegra_xusb_port *
>>   tegra_xusb_find_port(struct tegra_xusb_padctl *padctl, const char *type,
>>   		     unsigned int index);
>>   
>> +enum usb_charger_type tegra_xusb_padctl_charger_detect(
>> +					  struct tegra_xusb_port *port);
>> +
>>   struct tegra_xusb_usb2_port {
>>   	struct tegra_xusb_port base;
>>   
>> @@ -430,6 +436,7 @@ struct tegra_xusb_padctl_soc {
>>   	unsigned int num_supplies;
>>   	bool supports_gen2;
>>   	bool need_fake_usb3_port;
>> +	bool charger_detect;
> Perhaps make this "supports_charger_detection" because it being in the
> SoC structure means that it's a capability. "charger_detect" makes it
> look like an option that we've enabled or not. That's what struct
> tegra_xusb_port.charger_detect already is.
> 
Will update accordingly
-Nagarjuna
> Thierry
> 

Re: [PATCH V2 6/8] phy: tegra: xusb: Add support for charger detect

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]

On Mon, May 04, 2020 at 02:32:51PM +0530, Nagarjuna Kristam wrote:
> >On 28-04-2020 16:25, Thierry Reding wrote:
> > > On Wed, Apr 15, 2020 at 01:55:06PM +0530, Nagarjuna Kristam wrote:
[...]
> > > diff --git a/drivers/phy/tegra/xusb-tegra-cd.c b/drivers/phy/tegra/xusb-tegra-cd.c
> > > +static void tegra_xusb_padctl_utmi_pad_dcd(struct tegra_xusb_padctl *padctl,
> > > +					      u32 index)
> > > +{
> > > +	u32 value;
> > > +	int dcd_timeout_ms = 0;
> > > +	bool ret = false;
> > > +
> > > +	/* Turn on IDP_SRC */
> > > +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> > > +	value |= OP_I_SRC_EN;
> > > +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> > > +
> > > +	/* Turn on D- pull-down resistor */
> > > +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> > > +	value |= USBON_RPD_OVRD_VAL;
> > > +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> > > +
> > > +	/* Wait for TDCD_DBNC */
> > > +	usleep_range(10000, 120000);
> >  From the comment this looks like we're waiting for some hardware
> > condition. Can we somehow obtain this rather than implementing a fixed
> > sleep? Especially since the range here is so large.
> > 
> As per data sheet we need to wait for 10 micro seconds as settle time.

Okay, so TDCD_DBNC is a value that comes from a timing diagram in a
datasheet? Seems fine to leave it as-is then. Perhaps add parentheses
and mention which exact datasheet that's from, and perhaps which figure
so that people can more easily reference it. Provided there is a
publicly available datasheet, of course.

Actually, one other thing: If the data sheet says to wait 10 us, why do
you use an upper range of 120 us? Shouldn't a range of 10-20 us be good
enough?

> > > +	/* Wait for TVDPSRC_ON */
> > > +	msleep(40);
> > Again, is this a hardware condition that we can wait on by polling a
> > register?
> > 
> It HW settle time before reading registers.

Again, perhaps link to the datasheet, or alternatively describe in the
comment what this is waiting for. That is, something like:

	/* wait for TVDPSRC_ON (wait for hardware to settle) */

or similar.

> > > +		if (tegra_xusb_padctl_utmi_pad_secondary_charger_detect(padctl,
> > > +									index))
> > > +			chrg_type = CDP_TYPE;
> > > +		else
> > > +			chrg_type = DCP_TYPE;
> > > +	} else {
> > > +		chrg_type = SDP_TYPE;
> > > +	}
> > > +
> > > +	dev_dbg(&port->dev, "charger detected of type %d", chrg_type);
> > Do we have a string representation of this?
> > 
> No String representation available. Shall i add one for wasy reading ?

Yeah, I think that'd be nice.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V2 6/8] phy: tegra: xusb: Add support for charger detect

[Nagarjuna Kristam]

On 04-05-2020 21:20, Thierry Reding wrote:
> 
> On Mon, May 04, 2020 at 02:32:51PM +0530, Nagarjuna Kristam wrote:
>>> On 28-04-2020 16:25, Thierry Reding wrote:
>>>> On Wed, Apr 15, 2020 at 01:55:06PM +0530, Nagarjuna Kristam wrote:
> [...]
>>>> diff --git a/drivers/phy/tegra/xusb-tegra-cd.c b/drivers/phy/tegra/xusb-tegra-cd.c
>>>> +static void tegra_xusb_padctl_utmi_pad_dcd(struct tegra_xusb_padctl *padctl,
>>>> +					      u32 index)
>>>> +{
>>>> +	u32 value;
>>>> +	int dcd_timeout_ms = 0;
>>>> +	bool ret = false;
>>>> +
>>>> +	/* Turn on IDP_SRC */
>>>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>>>> +	value |= OP_I_SRC_EN;
>>>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
>>>> +
>>>> +	/* Turn on D- pull-down resistor */
>>>> +	value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>>>> +	value |= USBON_RPD_OVRD_VAL;
>>>> +	padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
>>>> +
>>>> +	/* Wait for TDCD_DBNC */
>>>> +	usleep_range(10000, 120000);
>>>   From the comment this looks like we're waiting for some hardware
>>> condition. Can we somehow obtain this rather than implementing a fixed
>>> sleep? Especially since the range here is so large.
>>>
>> As per data sheet we need to wait for 10 micro seconds as settle time.
> Okay, so TDCD_DBNC is a value that comes from a timing diagram in a
> datasheet? Seems fine to leave it as-is then. Perhaps add parentheses
> and mention which exact datasheet that's from, and perhaps which figure
> so that people can more easily reference it. Provided there is a
> publicly available datasheet, of course.
> 
Will update reference to table in the data sheet where these values are 
recommended. ITs part of BC 1.2 spec from USB.

> Actually, one other thing: If the data sheet says to wait 10 us, why do
> you use an upper range of 120 us? Shouldn't a range of 10-20 us be good
> enough?
> Yes, will reduce it to 20ms.

-Nagarjuna

[PATCH V2 7/8] phy: tegra: xusb: Enable charger detect for Tegra186

[Nagarjuna Kristam]

Tegra186 SoC supports charger detect, set corresponding soc flag.

Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
---
V2:
 - Patch re-based.
---
 drivers/phy/tegra/xusb-tegra186.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
index 03bdb5b..12ff492 100644
--- a/drivers/phy/tegra/xusb-tegra186.c
+++ b/drivers/phy/tegra/xusb-tegra186.c
@@ -1041,6 +1041,7 @@ const struct tegra_xusb_padctl_soc tegra186_xusb_padctl_soc = {
 	.ops = &tegra186_xusb_padctl_ops,
 	.supply_names = tegra186_xusb_padctl_supply_names,
 	.num_supplies = ARRAY_SIZE(tegra186_xusb_padctl_supply_names),
+	.charger_detect = true,
 };
 EXPORT_SYMBOL_GPL(tegra186_xusb_padctl_soc);
 #endif
-- 
2.7.4

Re: [PATCH V2 7/8] phy: tegra: xusb: Enable charger detect for Tegra186

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

On Wed, Apr 15, 2020 at 01:55:07PM +0530, Nagarjuna Kristam wrote:
> Tegra186 SoC supports charger detect, set corresponding soc flag.

"SoC flag"

> 
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V2:
>  - Patch re-based.
> ---
>  drivers/phy/tegra/xusb-tegra186.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
> index 03bdb5b..12ff492 100644
> --- a/drivers/phy/tegra/xusb-tegra186.c
> +++ b/drivers/phy/tegra/xusb-tegra186.c
> @@ -1041,6 +1041,7 @@ const struct tegra_xusb_padctl_soc tegra186_xusb_padctl_soc = {
>  	.ops = &tegra186_xusb_padctl_ops,
>  	.supply_names = tegra186_xusb_padctl_supply_names,
>  	.num_supplies = ARRAY_SIZE(tegra186_xusb_padctl_supply_names),
> +	.charger_detect = true,

And this may require an update for the change I suggested in patch 6/8,
but with that and the typo above addressed, this is:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V2 8/8] phy: tegra: xusb: Enable charger detect for Tegra210

[Nagarjuna Kristam]

Tegra210 SoC supports charger detect, set corresponding soc flag.

Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
---
V2:
 - Patch re-based.
---
 drivers/phy/tegra/xusb-tegra210.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
index 7d84f1a..2be5201 100644
--- a/drivers/phy/tegra/xusb-tegra210.c
+++ b/drivers/phy/tegra/xusb-tegra210.c
@@ -2352,6 +2352,7 @@ const struct tegra_xusb_padctl_soc tegra210_xusb_padctl_soc = {
 	.supply_names = tegra210_xusb_padctl_supply_names,
 	.num_supplies = ARRAY_SIZE(tegra210_xusb_padctl_supply_names),
 	.need_fake_usb3_port = true,
+	.charger_detect = true,
 };
 EXPORT_SYMBOL_GPL(tegra210_xusb_padctl_soc);
 
-- 
2.7.4

Re: [PATCH V2 8/8] phy: tegra: xusb: Enable charger detect for Tegra210

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 973 bytes --]

On Wed, Apr 15, 2020 at 01:55:08PM +0530, Nagarjuna Kristam wrote:
> Tegra210 SoC supports charger detect, set corresponding soc flag.
> 
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V2:
>  - Patch re-based.
> ---
>  drivers/phy/tegra/xusb-tegra210.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/xusb-tegra210.c
> index 7d84f1a..2be5201 100644
> --- a/drivers/phy/tegra/xusb-tegra210.c
> +++ b/drivers/phy/tegra/xusb-tegra210.c
> @@ -2352,6 +2352,7 @@ const struct tegra_xusb_padctl_soc tegra210_xusb_padctl_soc = {
>  	.supply_names = tegra210_xusb_padctl_supply_names,
>  	.num_supplies = ARRAY_SIZE(tegra210_xusb_padctl_supply_names),
>  	.need_fake_usb3_port = true,
> +	.charger_detect = true,
>  };
>  EXPORT_SYMBOL_GPL(tegra210_xusb_padctl_soc);
>  

Same comments as for patch 7/8, with those addressed:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]