diff mbox series

[v2,2/2] phy: intel: Add Keem Bay USB PHY support

Message ID 20201109031654.22443-3-wan.ahmad.zainie.wan.mohamad@intel.com
State Superseded
Headers show
Series phy: intel: Add Keem Bay USB PHY support | expand

Commit Message

Wan Ahmad Zainie Nov. 9, 2020, 3:16 a.m. UTC
Add support for USB PHY on Intel Keem Bay SoC.

Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
---
 drivers/phy/intel/Kconfig                 |  12 +
 drivers/phy/intel/Makefile                |   1 +
 drivers/phy/intel/phy-intel-keembay-usb.c | 288 ++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/phy/intel/phy-intel-keembay-usb.c

Comments

Andy Shevchenko Nov. 9, 2020, 11:41 a.m. UTC | #1
On Mon, Nov 09, 2020 at 11:16:54AM +0800, Wan Ahmad Zainie wrote:
> Add support for USB PHY on Intel Keem Bay SoC.

...

> +config PHY_INTEL_KEEMBAY_USB
> +	tristate "Intel Keem Bay USB PHY driver"
> +	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)

> +	depends on OF && HAS_IOMEM

Do you really need dependency to OF (yes, I see that it will be not functional,
but still can be compile tested)?

> +	select GENERIC_PHY
> +	select REGMAP_MMIO
> +	help
> +	  Choose this option if you have an Intel Keem Bay SoC.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called phy-keembay-usb.ko.

...

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>

> +#include <linux/of.h>

No evidence of anything being used in this code.
mod_devicetable.h is missed, though.

> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

...

> +	usleep_range(30, 50);

Why 30-50?

...

> +	usleep_range(20, 50);

Why these numbers?

...

> +	usleep_range(2, 10);

Ditto.

...

> +	usleep_range(20, 50);

Ditto.


...

> +	struct device_node *np = dev->of_node;

It's being used only once and it doesn't bring any benefit.
Wan Ahmad Zainie Nov. 11, 2020, 9:28 a.m. UTC | #2
Hi Andy.

Thanks for the review and sorry for the late reply.

> -----Original Message-----

> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Sent: Monday, November 9, 2020 7:41 PM

> To: Wan Mohamad, Wan Ahmad Zainie

> <wan.ahmad.zainie.wan.mohamad@intel.com>

> Cc: kishon@ti.com; vkoul@kernel.org; robh+dt@kernel.org; linux-

> kernel@vger.kernel.org; devicetree@vger.kernel.org;

> mgross@linux.intel.com; Raja Subramanian, Lakshmi Bai

> <lakshmi.bai.raja.subramanian@intel.com>

> Subject: Re: [PATCH v2 2/2] phy: intel: Add Keem Bay USB PHY support

> 

> On Mon, Nov 09, 2020 at 11:16:54AM +0800, Wan Ahmad Zainie wrote:

> > Add support for USB PHY on Intel Keem Bay SoC.

> 

> ...

> 

> > +config PHY_INTEL_KEEMBAY_USB

> > +	tristate "Intel Keem Bay USB PHY driver"

> > +	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)

> 

> > +	depends on OF && HAS_IOMEM

> 

> Do you really need dependency to OF (yes, I see that it will be not functional,

> but still can be compile tested)?


No, I can drop OF.
I will remove in v3.

> 

> > +	select GENERIC_PHY

> > +	select REGMAP_MMIO

> > +	help

> > +	  Choose this option if you have an Intel Keem Bay SoC.

> > +

> > +	  To compile this driver as a module, choose M here: the module

> > +	  will be called phy-keembay-usb.ko.

> 

> ...

> 

> > +#include <linux/bitfield.h>

> > +#include <linux/bits.h>

> > +#include <linux/clk.h>

> > +#include <linux/delay.h>

> > +#include <linux/module.h>

> 

> > +#include <linux/of.h>

> 

> No evidence of anything being used in this code.

> mod_devicetable.h is missed, though.


I will fix in v3. Remove of.h and add mod_devicetable.h.

> 

> > +#include <linux/phy/phy.h>

> > +#include <linux/platform_device.h>

> > +#include <linux/regmap.h>

> 

> ...

> 

> > +	usleep_range(30, 50);

> 

> Why 30-50?


I take this value from boot firmware.
There is a delay of 30us after clearing IDDQ_enable bit.
I believe the purpose is to ensure all analog blocks are powered up.

> 

> ...

> 

> > +	usleep_range(20, 50);

> 

> Why these numbers?


In Keem Bay data book, under USB initialization section,
there is step that there must be a minimum 20us wait
after clock enable, before bringing PHYs out of reset.

50 is the value that I picked randomly. Is usleep_range(20, 20)
Better?

> 

> ...

> 

> > +	usleep_range(2, 10);

> 

> Ditto.


Under the same section above, there is a step for 2us wait.
I believe it is for register write to go through.

> 

> ...

> 

> > +	usleep_range(20, 50);

> 

> Ditto.


Under the same section above, there is a step to wait 20us
after setting SRAM load bit, before release the controller
reset.

I will add comment for those 4 delay above.

Before I proceed with v3, I would like to know if I should
use udelay(), instead of usleep_range()?
I refer to https://www.kernel.org/doc/Documentation/timers/timers-howto.txt.

> 

> 

> ...

> 

> > +	struct device_node *np = dev->of_node;

> 

> It's being used only once and it doesn't bring any benefit.


I will fix in v3. Use dev->of_node directly.

> 

> --

> With Best Regards,

> Andy Shevchenko

> 


Best regards,
Zainie
Andy Shevchenko Nov. 11, 2020, 9:49 a.m. UTC | #3
On Wed, Nov 11, 2020 at 09:28:34AM +0000, Wan Mohamad, Wan Ahmad Zainie wrote:
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> > Sent: Monday, November 9, 2020 7:41 PM

> > To: Wan Mohamad, Wan Ahmad Zainie

> > On Mon, Nov 09, 2020 at 11:16:54AM +0800, Wan Ahmad Zainie wrote:


...

> > > +	usleep_range(30, 50);

> > 

> > Why 30-50?

> 

> I take this value from boot firmware.

> There is a delay of 30us after clearing IDDQ_enable bit.

> I believe the purpose is to ensure all analog blocks are powered up.


Then put it into comment.

...

> > > +	usleep_range(20, 50);

> > 

> > Why these numbers?

> 

> In Keem Bay data book, under USB initialization section,

> there is step that there must be a minimum 20us wait

> after clock enable, before bringing PHYs out of reset.

> 

> 50 is the value that I picked randomly. Is usleep_range(20, 20)

> Better?


No, the better as I told you already few times is to comment "why?" these
numbers. Above can be like:
"According to datasheet this step requires 20us wait..."

...

> > > +	usleep_range(2, 10);

> > 

> > Ditto.

> 

> Under the same section above, there is a step for 2us wait.

> I believe it is for register write to go through.


Ditto.

> > 

> > ...

> > 

> > > +	usleep_range(20, 50);

> > 

> > Ditto.

> 

> Under the same section above, there is a step to wait 20us

> after setting SRAM load bit, before release the controller

> reset.

> 

> I will add comment for those 4 delay above.


Yes, please.

...

> Before I proceed with v3, I would like to know if I should

> use udelay(), instead of usleep_range()?

> I refer to https://www.kernel.org/doc/Documentation/timers/timers-howto.txt.


You should know your code better than me. That howto is clear about when of
which API calls can be used.

-- 
With Best Regards,
Andy Shevchenko
diff mbox series

Patch

diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
index 58ec695c92ec..f8bc2527e6e4 100644
--- a/drivers/phy/intel/Kconfig
+++ b/drivers/phy/intel/Kconfig
@@ -14,6 +14,18 @@  config PHY_INTEL_KEEMBAY_EMMC
 	  To compile this driver as a module, choose M here: the module
 	  will be called phy-keembay-emmc.ko.
 
+config PHY_INTEL_KEEMBAY_USB
+	tristate "Intel Keem Bay USB PHY driver"
+	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)
+	depends on OF && HAS_IOMEM
+	select GENERIC_PHY
+	select REGMAP_MMIO
+	help
+	  Choose this option if you have an Intel Keem Bay SoC.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called phy-keembay-usb.ko.
+
 config PHY_INTEL_LGM_COMBO
 	bool "Intel Lightning Mountain ComboPHY driver"
 	depends on X86 || COMPILE_TEST
diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
index a5e0af5ccd75..14550981a707 100644
--- a/drivers/phy/intel/Makefile
+++ b/drivers/phy/intel/Makefile
@@ -1,4 +1,5 @@ 
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_PHY_INTEL_KEEMBAY_EMMC)	+= phy-intel-keembay-emmc.o
+obj-$(CONFIG_PHY_INTEL_KEEMBAY_USB)	+= phy-intel-keembay-usb.o
 obj-$(CONFIG_PHY_INTEL_LGM_COMBO)	+= phy-intel-lgm-combo.o
 obj-$(CONFIG_PHY_INTEL_LGM_EMMC)	+= phy-intel-lgm-emmc.o
diff --git a/drivers/phy/intel/phy-intel-keembay-usb.c b/drivers/phy/intel/phy-intel-keembay-usb.c
new file mode 100644
index 000000000000..4f5628d77fe1
--- /dev/null
+++ b/drivers/phy/intel/phy-intel-keembay-usb.c
@@ -0,0 +1,288 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel Keem Bay USB PHY driver
+ * Copyright (C) 2020 Intel Corporation
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+/* USS (USB Subsystem) clock control registers */
+#define USS_CPR_CLK_EN		0x00
+#define USS_CPR_CLK_SET		0x04
+#define USS_CPR_CLK_CLR		0x08
+#define USS_CPR_RST_EN		0x10
+#define USS_CPR_RST_SET		0x14
+#define USS_CPR_RST_CLR		0x18
+
+/* USS clock/reset bit fields */
+#define USS_CPR_PHY_TST		BIT(6)
+#define USS_CPR_LOW_JIT		BIT(5)
+#define USS_CPR_CORE		BIT(4)
+#define USS_CPR_SUSPEND		BIT(3)
+#define USS_CPR_ALT_REF		BIT(2)
+#define USS_CPR_REF		BIT(1)
+#define USS_CPR_SYS		BIT(0)
+#define USS_CPR_MASK		0x7f
+
+/* USS APB slave registers */
+#define USS_USB_CTRL_CFG0		0x10
+#define  VCC_RESET_N_MASK		BIT(31)
+
+#define USS_USB_PHY_CFG0		0x30
+#define  POR_MASK			BIT(15)
+#define  PHY_RESET_MASK			BIT(14)
+#define  PHY_REF_USE_PAD_MASK		BIT(5)
+
+#define USS_USB_PHY_CFG6		0x64
+#define  PHY0_SRAM_EXT_LD_DONE_MASK	BIT(23)
+
+#define USS_USB_PARALLEL_IF_CTRL	0xa0
+#define  USB_PHY_CR_PARA_SEL_MASK	BIT(2)
+
+#define USS_USB_TSET_SIGNALS_AND_GLOB	0xac
+#define  USB_PHY_CR_PARA_CLK_EN_MASK	BIT(7)
+
+#define USS_USB_STATUS_REG		0xb8
+#define  PHY0_SRAM_INIT_DONE_MASK	BIT(3)
+
+#define USS_USB_TIEOFFS_CONSTANTS_REG1	0xc0
+#define  IDDQ_ENABLE_MASK		BIT(10)
+
+struct keembay_usb_phy {
+	struct device *dev;
+	struct regmap *regmap_cpr;
+	struct regmap *regmap_slv;
+};
+
+static const struct regmap_config keembay_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+static int keembay_usb_clocks_on(struct keembay_usb_phy *priv)
+{
+	int ret;
+
+	ret = regmap_update_bits(priv->regmap_cpr, USS_CPR_CLK_SET,
+				 USS_CPR_MASK, USS_CPR_MASK);
+	if (ret) {
+		dev_err(priv->dev, "error clock set: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_update_bits(priv->regmap_cpr, USS_CPR_RST_SET,
+				 USS_CPR_MASK, USS_CPR_MASK);
+	if (ret) {
+		dev_err(priv->dev, "error reset set: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_update_bits(priv->regmap_slv,
+				 USS_USB_TIEOFFS_CONSTANTS_REG1,
+				 IDDQ_ENABLE_MASK,
+				 FIELD_PREP(IDDQ_ENABLE_MASK, 0));
+	if (ret) {
+		dev_err(priv->dev, "error iddq enable: %d\n", ret);
+		return ret;
+	}
+
+	usleep_range(30, 50);
+
+	ret = regmap_update_bits(priv->regmap_slv, USS_USB_PHY_CFG0,
+				 PHY_REF_USE_PAD_MASK,
+				 FIELD_PREP(PHY_REF_USE_PAD_MASK, 1));
+	if (ret)
+		dev_err(priv->dev, "error ref clock select: %d\n", ret);
+
+	return ret;
+}
+
+static int keembay_usb_core_off(struct keembay_usb_phy *priv)
+{
+	int ret;
+
+	ret = regmap_update_bits(priv->regmap_slv, USS_USB_CTRL_CFG0,
+				 VCC_RESET_N_MASK,
+				 FIELD_PREP(VCC_RESET_N_MASK, 0));
+	if (ret)
+		dev_err(priv->dev, "error core reset: %d\n", ret);
+
+	return ret;
+}
+
+static int keembay_usb_core_on(struct keembay_usb_phy *priv)
+{
+	int ret;
+
+	ret = regmap_update_bits(priv->regmap_slv, USS_USB_CTRL_CFG0,
+				 VCC_RESET_N_MASK,
+				 FIELD_PREP(VCC_RESET_N_MASK, 1));
+	if (ret)
+		dev_err(priv->dev, "error core on: %d\n", ret);
+
+	return ret;
+}
+
+static int keembay_usb_phys_on(struct keembay_usb_phy *priv)
+{
+	int ret;
+
+	ret = regmap_update_bits(priv->regmap_slv, USS_USB_PHY_CFG0,
+				 POR_MASK | PHY_RESET_MASK,
+				 FIELD_PREP(POR_MASK | PHY_RESET_MASK, 0));
+	if (ret)
+		dev_err(priv->dev, "error phys on: %d\n", ret);
+
+	return ret;
+}
+
+static int keembay_usb_phy_init(struct phy *phy)
+{
+	struct keembay_usb_phy *priv = phy_get_drvdata(phy);
+	u32 val;
+	int ret;
+
+	ret = keembay_usb_core_off(priv);
+	if (ret)
+		return ret;
+
+	usleep_range(20, 50);
+
+	ret = keembay_usb_phys_on(priv);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(priv->regmap_slv,
+				 USS_USB_TSET_SIGNALS_AND_GLOB,
+				 USB_PHY_CR_PARA_CLK_EN_MASK,
+				 FIELD_PREP(USB_PHY_CR_PARA_CLK_EN_MASK, 0));
+	if (ret) {
+		dev_err(priv->dev, "error cr clock disable: %d\n", ret);
+		return ret;
+	}
+
+	usleep_range(2, 10);
+
+	ret = regmap_update_bits(priv->regmap_slv,
+				 USS_USB_PARALLEL_IF_CTRL,
+				 USB_PHY_CR_PARA_SEL_MASK,
+				 FIELD_PREP(USB_PHY_CR_PARA_SEL_MASK, 1));
+	if (ret) {
+		dev_err(priv->dev, "error cr select: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_update_bits(priv->regmap_slv,
+				 USS_USB_TSET_SIGNALS_AND_GLOB,
+				 USB_PHY_CR_PARA_CLK_EN_MASK,
+				 FIELD_PREP(USB_PHY_CR_PARA_CLK_EN_MASK, 1));
+	if (ret) {
+		dev_err(priv->dev, "error cr clock enable: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_read_poll_timeout(priv->regmap_slv, USS_USB_STATUS_REG,
+				       val, val & PHY0_SRAM_INIT_DONE_MASK,
+				       USEC_PER_MSEC, 10 * USEC_PER_MSEC);
+	if (ret) {
+		dev_err(priv->dev, "SRAM init not done: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_update_bits(priv->regmap_slv, USS_USB_PHY_CFG6,
+				 PHY0_SRAM_EXT_LD_DONE_MASK,
+				 FIELD_PREP(PHY0_SRAM_EXT_LD_DONE_MASK, 1));
+	if (ret) {
+		dev_err(priv->dev, "error SRAM init done set: %d\n", ret);
+		return ret;
+	}
+
+	usleep_range(20, 50);
+
+	return keembay_usb_core_on(priv);
+}
+
+static const struct phy_ops ops = {
+	.init		= keembay_usb_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static int keembay_usb_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct keembay_usb_phy *priv;
+	struct phy *generic_phy;
+	struct phy_provider *phy_provider;
+	void __iomem *base;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	base = devm_platform_ioremap_resource_byname(pdev, "cpr-apb-base");
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv->regmap_cpr = devm_regmap_init_mmio(dev, base,
+						 &keembay_regmap_config);
+	if (IS_ERR(priv->regmap_cpr))
+		return PTR_ERR(priv->regmap_cpr);
+
+	base = devm_platform_ioremap_resource_byname(pdev, "slv-apb-base");
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv->regmap_slv = devm_regmap_init_mmio(dev, base,
+						 &keembay_regmap_config);
+	if (IS_ERR(priv->regmap_slv))
+		return PTR_ERR(priv->regmap_slv);
+
+	generic_phy = devm_phy_create(dev, np, &ops);
+	if (IS_ERR(generic_phy))
+		return dev_err_probe(dev, PTR_ERR(generic_phy),
+				     "failed to create PHY\n");
+
+	phy_set_drvdata(generic_phy, priv);
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(phy_provider))
+		return dev_err_probe(dev, PTR_ERR(phy_provider),
+				     "failed to register phy provider\n");
+
+	/* Setup USB subsystem clocks */
+	ret = keembay_usb_clocks_on(priv);
+	if (ret)
+		return ret;
+
+	/* and turn on the DWC3 core, prior to DWC3 driver init. */
+	return keembay_usb_core_on(priv);
+}
+
+static const struct of_device_id keembay_usb_phy_dt_ids[] = {
+	{ .compatible = "intel,keembay-usb-phy" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, keembay_usb_phy_dt_ids);
+
+static struct platform_driver keembay_usb_phy_driver = {
+	.probe		= keembay_usb_phy_probe,
+	.driver		= {
+		.name	= "keembay-usb-phy",
+		.of_match_table = keembay_usb_phy_dt_ids,
+	},
+};
+module_platform_driver(keembay_usb_phy_driver);
+
+MODULE_AUTHOR("Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>");
+MODULE_DESCRIPTION("Intel Keem Bay USB PHY driver");
+MODULE_LICENSE("GPL v2");