| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 4 May 2020 14:32:51 +0530
From: Nagarjuna Kristam <nkristam@...dia.com>
To: Thierry Reding <thierry.reding@...il.com>
CC: <balbi@...nel.org>, <gregkh@...uxfoundation.org>,
<jonathanh@...dia.com>, <mark.rutland@....com>,
<robh+dt@...nel.org>, <kishon@...com>,
<devicetree@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
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<nkristam@...dia.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@...dia.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
>
Powered by blists - more mailing lists