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(1, 12);
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
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(1, 12);
> > 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(>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
signature.asc
Description: PGP signature
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
---
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 000..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
+#include
+#include
+#include
+
+#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_INVREG_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,
+
Re: [PATCH V2 6/8] phy: tegra: xusb: Add support for charger detect
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
> ---
> 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 000..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
> +#include
> +#include
> +#include
> +
> +#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_INVREG_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);
> +
