Re: [PATCH v8 3/3] phy: Add Qualcomm DWC3 HS/SS PHY driver

[Josh Cartwright]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/