Hi.
Intel Keem Bay USB subsystem incorporates DesignWare USB3.1 controller,
an USB3.1 (Gen1/2) PHY and an USB2.0 PHY. It is a Dual Role Device
(DRD), operating as either a USB host or a USB device.
The patchset is tested on Keem Bay EVM.
Thank you.
Best regards,
Zainie
Changes since v1:
- Remove 2 patches that had been merged. Reduced To, Cc list.
- Rebased to v5.10-rc3.
- Add Rob's Reviewed-by tag in the first patch.
- Use ARCH_KEEMBAY in Kconfig.
- Update #include header; remove <linux/of_address.h>, and add
<linux/bits.h>.
- Remove unnecessary comments.
Note: I resend this v2 as my previous v2 sent on Oct 28 not seen
in mailing lists.
Wan Ahmad Zainie (2):
dt-bindings: phy: Add Intel Keem Bay USB PHY bindings
phy: intel: Add Keem Bay USB PHY support
.../bindings/phy/intel,phy-keembay-usb.yaml | 44 +++
drivers/phy/intel/Kconfig | 12 +
drivers/phy/intel/Makefile | 1 +
drivers/phy/intel/phy-intel-keembay-usb.c | 288 ++++++++++++++++++
4 files changed, 345 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/intel,phy-keembay-usb.yaml
create mode 100644 drivers/phy/intel/phy-intel-keembay-usb.c
--
2.17.1
Binding description for Intel Keem Bay USB PHY.
Signed-off-by: Wan Ahmad Zainie <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../bindings/phy/intel,phy-keembay-usb.yaml | 44 +++++++++++++++++++
1 file changed, 44 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/intel,phy-keembay-usb.yaml
diff --git a/Documentation/devicetree/bindings/phy/intel,phy-keembay-usb.yaml b/Documentation/devicetree/bindings/phy/intel,phy-keembay-usb.yaml
new file mode 100644
index 000000000000..a217bb8ac5bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/intel,phy-keembay-usb.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/intel,phy-keembay-usb.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel Keem Bay USB PHY bindings
+
+maintainers:
+ - Wan Ahmad Zainie <[email protected]>
+
+properties:
+ compatible:
+ const: intel,keembay-usb-phy
+
+ reg:
+ items:
+ - description: USB APB CPR (clock, power, reset) register
+ - description: USB APB slave register
+
+ reg-names:
+ items:
+ - const: cpr-apb-base
+ - const: slv-apb-base
+
+ '#phy-cells':
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - '#phy-cells'
+
+additionalProperties: false
+
+examples:
+ - |
+ usb-phy@20400000 {
+ compatible = "intel,keembay-usb-phy";
+ reg = <0x20400000 0x1c>,
+ <0x20480000 0xd0>;
+ reg-names = "cpr-apb-base", "slv-apb-base";
+ #phy-cells = <0>;
+ };
--
2.17.1
Add support for USB PHY on Intel Keem Bay SoC.
Signed-off-by: Wan Ahmad Zainie <[email protected]>
---
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
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 <[email protected]>");
+MODULE_DESCRIPTION("Intel Keem Bay USB PHY driver");
+MODULE_LICENSE("GPL v2");
--
2.17.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.
--
With Best Regards,
Andy Shevchenko
Hi Andy.
Thanks for the review and sorry for the late reply.
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Monday, November 9, 2020 7:41 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; Raja Subramanian, Lakshmi Bai
> <[email protected]>
> 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
On Wed, Nov 11, 2020 at 09:28:34AM +0000, Wan Mohamad, Wan Ahmad Zainie wrote:
> > From: Andy Shevchenko <[email protected]>
> > 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