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.

2020-05-04 09:14:29

by Nagarjuna Kristam

[permalink] [raw]

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

>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<[email protected]>
>> ---
>> 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<[email protected]>");
>> +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
>

2020-05-04 10:49:33

by Nagarjuna Kristam

[permalink] [raw]

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

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<[email protected]>
>> ---
>> 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

2020-05-04 18:45:57

by Thierry Reding

[permalink] [raw]

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

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

Attachments:

(No filename) (2.65 kB)
signature.asc (849.00 B)
Download all attachments

2020-05-11 03:20:07

by Nagarjuna Kristam

[permalink] [raw]

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

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