Hey Andy-
Mostly cosmetic things below:
On Fri, Sep 12, 2014 at 12:29:46PM -0500, Andy Gross wrote:
> This patch adds a new driver for the Qualcomm USB 3.0 PHY that exists on some
> Qualcomm platforms. This driver uses the generic PHY framework and will
> interact with the DWC3 controller.
>
> Signed-off-by: Andy Gross <agross@xxxxxxxxxxxxxx>
> ---
> drivers/phy/Kconfig | 11 +
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-qcom-dwc3.c | 500 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 512 insertions(+)
> create mode 100644 drivers/phy/phy-qcom-dwc3.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 0dd7427..5d56161 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -230,4 +230,15 @@ config PHY_XGENE
> help
> This option enables support for APM X-Gene SoC multi-purpose PHY.
>
> +config PHY_QCOM_DWC3
> + tristate "QCOM DWC3 USB PHY support"
> + depends on ARCH_QCOM
> + depends on HAS_IOMEM
> + depends on OF
> + select GENERIC_PHY
> + help
> + This option enables support for the Synopsis PHYs present inside the
> + Qualcomm USB3.0 DWC3 controller. This driver supports both HS and SS
> + PHY controllers.
> +
> endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 95c69ed..aa16f30 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -28,3 +28,4 @@ obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA) += phy-qcom-ipq806x-sata.o
> obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY) += phy-spear1310-miphy.o
> obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY) += phy-spear1340-miphy.o
> obj-$(CONFIG_PHY_XGENE) += phy-xgene.o
> +obj-$(CONFIG_PHY_QCOM_DWC3) += phy-qcom-dwc3.o
> diff --git a/drivers/phy/phy-qcom-dwc3.c b/drivers/phy/phy-qcom-dwc3.c
> new file mode 100644
> index 0000000..05b8e1d
> --- /dev/null
> +++ b/drivers/phy/phy-qcom-dwc3.c
[..]
> +/* TX OVRD DRV LO register bits */
> +#define TX_OVRD_DRV_LO_AMPLITUDE_MASK 0x007F
> +#define TX_OVRD_DRV_LO_PREEMPH_MASK 0x3F80
> +#define TX_OVRD_DRV_LO_PREEMPH_SHIFT 7
> +#define TX_OVRD_DRV_LO_EN BIT(14)
> +
> +struct qcom_dwc3_usb_phy;
Do you need this?
> +
> +enum qcom_dwc3_phy_id {
> + QCOM_DWC3_PHY_UTMI = 0,
> + QCOM_DWC3_PHY_PIPE,
> + QCOM_DWC3_PHYS_NUM,
> +};
> +
> +struct qcom_dwc3_usb_phy {
> + void __iomem *base;
> + struct device *dev;
> + struct phy *phy;
> + enum qcom_dwc3_phy_id index;
> +
> + struct clk *xo_clk;
> + struct clk *ref_clk;
> +};
> +
> +/**
> + * Write register and read back masked value to confirm it is written
> + *
> + * @base - QCOM DWC3 PHY base virtual address.
> + * @offset - register offset.
> + * @mask - register bitmask specifying what should be updated
> + * @val - value to write.
> + */
> +static inline void qcom_dwc3_phy_write_readback(
> + struct qcom_dwc3_usb_phy *phy_dwc3, u32 offset,
> + const u32 mask, u32 val)
> +{
> + u32 write_val, tmp = readl(phy_dwc3->base + offset);
> +
> + tmp &= ~mask; /* retain other bits */
> + write_val = tmp | val;
> +
> + writel(write_val, phy_dwc3->base + offset);
> +
> + /* Read back to see if val was written */
> + tmp = readl(phy_dwc3->base + offset);
> + tmp &= mask; /* clear other bits */
> +
> + if (tmp != val)
> + dev_err(phy_dwc3->dev, "write: %x to QSCRATCH: %x FAILED\n",
> + val, offset);
> +}
> +
> +static int wait_for_latch(void __iomem *addr)
> +{
> + u32 retry = 10;
> +
> + while (true) {
> + if (!readl(addr))
> + break;
> +
> + if (--retry == 0)
> + return -ETIMEDOUT;
> +
> + usleep_range(10, 20);
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * Write SSPHY register
> + *
> + * @base - QCOM DWC3 PHY base virtual address.
> + * @addr - SSPHY address to write.
> + * @val - value to write.
> + */
> +static int qcom_dwc3_ss_write_phycreg(void __iomem *base, u32 addr, u32 val)
> +{
> + writel(addr, base + CR_PROTOCOL_DATA_IN_REG);
> + writel(0x1, base + CR_PROTOCOL_CAP_ADDR_REG);
> +
> + if (wait_for_latch(base + CR_PROTOCOL_CAP_ADDR_REG))
> + return 1;
I'm not too familiar with the GENERIC_PHY infrastructure, does it
understand that '1' is an error case? I'm wondering if you should
instead propogate the -ETIMEDOUT up.
> +
> + writel(val, base + CR_PROTOCOL_DATA_IN_REG);
> + writel(0x1, base + CR_PROTOCOL_CAP_DATA_REG);
> +
> + if (wait_for_latch(base + CR_PROTOCOL_CAP_DATA_REG))
> + return 1;
> +
> + writel(0x1, base + CR_PROTOCOL_WRITE_REG);
> +
> + if (wait_for_latch(base + CR_PROTOCOL_WRITE_REG))
> + return 1;
> +
> + return 0;
> +}
> +
[..]
> +
> +static struct phy_ops qcom_dwc3_phy_ops = {
> + .init = qcom_dwc3_phy_init,
> + .exit = qcom_dwc3_phy_exit,
> + .power_on = qcom_dwc3_phy_power_on,
> + .power_off = qcom_dwc3_phy_power_off,
> + .owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id qcom_dwc3_phy_table[] = {
> + {
> + .compatible = "qcom,dwc3-hs-usb-phy",
> + .data = (void *)QCOM_DWC3_PHY_UTMI,
> + },
> + {
> + .compatible = "qcom,dwc3-ss-usb-phy",
> + .data = (void *)QCOM_DWC3_PHY_PIPE,
> + },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_dwc3_phy_table);
I'd suggest just creating a separate ops table, and have '.data' point
to it. Might cleanup all of the switch() statements above. You could
also probably drop the 'index' member of 'struct qcom_dwc3_usb_phy', and
maybe even 'enum qcom_dwc3_phy_id' altogether.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
