[3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver
Commit Message
Add the M31 USB2 phy driver
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
1 file changed, 360 insertions(+)
create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c
Comments
On 7.06.2023 12:56, Varadarajan Narayanan wrote:
> Add the M31 USB2 phy driver
>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 360 insertions(+)
> create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c
> new file mode 100644
> index 0000000..d29a91e
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-m31.c
> @@ -0,0 +1,360 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb/phy.h>
> +#include <linux/reset.h>
> +#include <linux/of_device.h>
Please sort these
> +
> +enum clk_reset_action {
> + CLK_RESET_DEASSERT = 0,
> + CLK_RESET_ASSERT = 1
> +};
> +
> +#define USB2PHY_PORT_POWERDOWN 0xA4
> +#define POWER_UP BIT(0)
> +#define POWER_DOWN 0
> +
> +#define USB2PHY_PORT_UTMI_CTRL1 0x40
> +
> +#define USB2PHY_PORT_UTMI_CTRL2 0x44
> +#define UTMI_ULPI_SEL BIT(7)
> +#define UTMI_TEST_MUX_SEL BIT(6)
> +
> +#define HS_PHY_CTRL_REG 0x10
> +#define UTMI_OTG_VBUS_VALID BIT(20)
> +#define SW_SESSVLD_SEL BIT(28)
> +
> +#define USB_PHY_CFG0 0x94
> +#define USB_PHY_UTMI_CTRL5 0x50
> +#define USB_PHY_FSEL_SEL 0xB8
> +#define USB_PHY_HS_PHY_CTRL_COMMON0 0x54
> +#define USB_PHY_REFCLK_CTRL 0xA0
> +#define USB_PHY_HS_PHY_CTRL2 0x64
> +#define USB_PHY_UTMI_CTRL0 0x3c
> +#define USB2PHY_USB_PHY_M31_XCFGI_1 0xBC
> +#define USB2PHY_USB_PHY_M31_XCFGI_4 0xC8
> +#define USB2PHY_USB_PHY_M31_XCFGI_5 0xCC
> +#define USB2PHY_USB_PHY_M31_XCFGI_11 0xE4
Could you sort them address-wise?
> +
> +#define USB2_0_TX_ENABLE BIT(2)
> +#define HSTX_SLEW_RATE_565PS 3
> +#define PLL_CHARGING_PUMP_CURRENT_35UA (3 << 3)
> +#define ODT_VALUE_38_02_OHM (3 << 6)
> +#define ODT_VALUE_45_02_OHM BIT(2)
> +#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA (1)
Weird mix of values, bits, bitfields.. perhaps BIT(n) and
GENMASK() (+ FIELD_PREP) would be more suitable?
> +
> +#define UTMI_PHY_OVERRIDE_EN BIT(1)
> +#define POR_EN BIT(1)
Please associate these with their registers, like
#define FOO_REG 0xf00
#define POR_EN BIT(1)
> +#define FREQ_SEL BIT(0)
> +#define COMMONONN BIT(7)
> +#define FSEL BIT(4)
> +#define RETENABLEN BIT(3)
> +#define USB2_SUSPEND_N_SEL BIT(3)
> +#define USB2_SUSPEND_N BIT(2)
> +#define USB2_UTMI_CLK_EN BIT(1)
> +#define CLKCORE BIT(1)
> +#define ATERESET ~BIT(0)
> +#define FREQ_24MHZ (5 << 4)
> +#define XCFG_COARSE_TUNE_NUM (2 << 0)
> +#define XCFG_FINE_TUNE_NUM (1 << 3)
same comment
> +
> +static void m31usb_write_readback(void *base, u32 offset,
> + const u32 mask, u32 val);
We don't need this forward-definition, just move the function up.
> +
> +struct m31usb_phy {
> + struct usb_phy phy;
> + void __iomem *base;
> + void __iomem *qscratch_base;
> +
> + struct reset_control *phy_reset;
> +
> + bool cable_connected;
> + bool suspended;
> + bool ulpi_mode;
> +};
> +
> +static void m31usb_reset(struct m31usb_phy *qphy, u32 action)
> +{
> + if (action == CLK_RESET_ASSERT)
> + reset_control_assert(qphy->phy_reset);
> + else
> + reset_control_deassert(qphy->phy_reset);
> + wmb(); /* ensure data is written to hw register */
Please move the comment above the call.
> +}
> +
> +static void m31usb_phy_enable_clock(struct m31usb_phy *qphy)
> +{
> + /* Enable override ctrl */
> + writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
Some of the comments are missing a space before '*/'
Also, please consider adding some newlines to logically split the
actions.
> + /* Enable POR*/
> + writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5);
> + udelay(15);
> + /* Configure frequency select value*/
> + writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL);
> + /* Configure refclk frequency */
> + writel(COMMONONN | FSEL | RETENABLEN,
> + qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0);
> + /* select refclk source */
> + writel(CLKCORE, qphy->base + USB_PHY_REFCLK_CTRL);
> + /* select ATERESET*/
> + writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5);
> + writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> + qphy->base + USB_PHY_HS_PHY_CTRL2);
> + writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5);
> + writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> + qphy->base + USB_PHY_HS_PHY_CTRL2);
> + /* Disable override ctrl */
> + writel(0x0, qphy->base + USB_PHY_CFG0);
> +}
> +
> +static void ipq5332_m31usb_phy_enable_clock(struct m31usb_phy *qphy)
> +{
> + /* Enable override ctrl */
> + writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
> + /* Enable POR*/
ditto
> + writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5);
> + udelay(15);
> + /* Configure frequency select value*/
> + writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL);
> + /* Configure refclk frequency */
> + writel(COMMONONN | FREQ_24MHZ | RETENABLEN,
> + qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0);
> + /* select ATERESET*/
> + writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5);
> + writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> + qphy->base + USB_PHY_HS_PHY_CTRL2);
> + writel(XCFG_COARSE_TUNE_NUM | XCFG_FINE_TUNE_NUM,
> + qphy->base + USB2PHY_USB_PHY_M31_XCFGI_11);
> + /* Adjust HSTX slew rate to 565 ps*/
> + /* Adjust PLL lock Time counter for release clock to 35uA */
> + /* Adjust Manual control ODT value to 38.02 Ohm */
> + writel(HSTX_SLEW_RATE_565PS | PLL_CHARGING_PUMP_CURRENT_35UA |
> + ODT_VALUE_38_02_OHM, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_4);
These functions seem very similar, with the main difference being that
IPQ5332 adds some more writes at the end. Perhaps some commonizing could
be done?
> + /*
> + * Enable to always turn on USB 2.0 TX driver
> + * when system is in USB 2.0 HS mode
> + */
> + writel(USB2_0_TX_ENABLE, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_1);
> + /* Adjust Manual control ODT value to 45.02 Ohm */
> + /* Adjust HSTX Pre-emphasis level to 0.55mA */
> + writel(ODT_VALUE_45_02_OHM | HSTX_PRE_EMPHASIS_LEVEL_0_55MA,
> + qphy->base + USB2PHY_USB_PHY_M31_XCFGI_5);
> + udelay(4);
> + writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5);
> + writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> + qphy->base + USB_PHY_HS_PHY_CTRL2);
> +}
> +
> +static int m31usb_phy_init(struct usb_phy *phy)
> +{
> + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> +
> + /* Perform phy reset */
> + m31usb_reset(qphy, CLK_RESET_ASSERT);
> + usleep_range(1, 5);
https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
this may not work as intended
> + m31usb_reset(qphy, CLK_RESET_DEASSERT);
> +
> + /* configure for ULPI mode if requested */
> + if (qphy->ulpi_mode)
> + writel_relaxed(0x0, qphy->base + USB2PHY_PORT_UTMI_CTRL2);
> +
> + /* Enable the PHY */
> + writel_relaxed(POWER_UP, qphy->base + USB2PHY_PORT_POWERDOWN);
> +
> + /* Make sure above write completed */
> + wmb();
As you're calling wmb in the reset func, dropping _relaxed from the
ULPI and PORT_POWERDOWN writes should work the same
> +
> + /* Turn on phy ref clock*/
> + if (of_device_is_compatible(phy->dev->of_node,
> + "qcom,ipq5332-m31-usb-hsphy"))
> + ipq5332_m31usb_phy_enable_clock(qphy);
> + else
> + m31usb_phy_enable_clock(qphy);
This should be done using OF match data.
> +
> + /* Set OTG VBUS Valid from HSPHY to controller */
> + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> + UTMI_OTG_VBUS_VALID, UTMI_OTG_VBUS_VALID);
> + /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
> + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> + SW_SESSVLD_SEL, SW_SESSVLD_SEL);
> +
> + return 0;
> +}
> +
> +static void m31usb_phy_shutdown(struct usb_phy *phy)
> +{
> + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> +
> + /* Disable the PHY */
> + writel_relaxed(POWER_DOWN, qphy->base + USB2PHY_PORT_POWERDOWN);
> + /* Make sure above write completed */
> + wmb();
> +}
> +
> +static void m31usb_write_readback(void *base, u32 offset,
> + const u32 mask, u32 val)
The indentation here makes `const u32..` misaligned.
> +{
> + u32 write_val, tmp = readl_relaxed(base + offset);
> +
> + tmp &= ~mask; /* retain other bits */
> + write_val = tmp | val;
> +
> + writel_relaxed(write_val, base + offset);
> + /* Make sure above write completed */
> + wmb();
> +
> + /* Read back to see if val was written */
> + tmp = readl_relaxed(base + offset);
> + tmp &= mask; /* clear other bits */
> +
> + if (tmp != val)
> + pr_err("%s: write: %x to QSCRATCH: %x FAILED\n",
> + __func__, val, offset);
> +}
> +
> +static int m31usb_phy_notify_connect(struct usb_phy *phy,
> + enum usb_device_speed speed)
> +{
> + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> +
> + qphy->cable_connected = true;
> +
> + dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected);
spurious space at the beginning of the string
> +
> + /* Set OTG VBUS Valid from HSPHY to controller */
> + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> + UTMI_OTG_VBUS_VALID,
> + UTMI_OTG_VBUS_VALID);
Please align the lines with the previous opening bracket
> +
> + /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
> + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> + SW_SESSVLD_SEL, SW_SESSVLD_SEL);
> +
> + dev_dbg(phy->dev, "M31USB2 phy connect notification\n");
> + return 0;
> +}
> +
> +static int m31usb_phy_notify_disconnect(struct usb_phy *phy,
> + enum usb_device_speed speed)
> +{
> + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> +
> + qphy->cable_connected = false;
> +
> + dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected);
> +
> + /* Set OTG VBUS Valid from HSPHY to controller */
> + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> + UTMI_OTG_VBUS_VALID, 0);
> +
> + /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
> + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> + SW_SESSVLD_SEL, 0);
> +
> + dev_dbg(phy->dev, "M31USB2 phy disconnect notification\n");
> + return 0;
> +}
> +
> +static int m31usb_phy_probe(struct platform_device *pdev)
> +{
> + struct m31usb_phy *qphy;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + int ret;
> + const char *phy_type;
Please sort these in a reverse-Christmas-tree order.
> +
> + qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
> + if (!qphy)
> + return -ENOMEM;
> +
> + qphy->phy.dev = dev;
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> + "m31usb_phy_base");
> + qphy->base = devm_ioremap_resource(dev, res);
devm_platform_get_and_ioremap_resource?
> + if (IS_ERR(qphy->base))
> + return PTR_ERR(qphy->base);
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> + "qscratch_base");
> + if (res) {
Do we expect platforms without this register space?
> + qphy->qscratch_base = devm_ioremap(dev, res->start,
> + resource_size(res));
> + if (IS_ERR(qphy->qscratch_base)) {
> + dev_dbg(dev, "couldn't ioremap qscratch_base\n");
> + qphy->qscratch_base = NULL;
> + }
> + }
> +
> + qphy->phy_reset = devm_reset_control_get(dev, "usb2_phy_reset");
not _exclusive?
> + if (IS_ERR(qphy->phy_reset))
> + return PTR_ERR(qphy->phy_reset);
> +
> + qphy->ulpi_mode = false;
> + ret = of_property_read_string(dev->of_node, "phy_type", &phy_type);
of_usb_get_phy_mode?
> +
> + if (!ret) {
> + if (!strcasecmp(phy_type, "ulpi"))
> + qphy->ulpi_mode = true;
> + } else {
> + dev_err(dev, "error reading phy_type property\n");
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, qphy);
> +
> + qphy->phy.label = "qcom-m31usb-phy";
> + qphy->phy.init = m31usb_phy_init;
> + qphy->phy.shutdown = m31usb_phy_shutdown;
> + qphy->phy.type = USB_PHY_TYPE_USB2;
> +
> + if (qphy->qscratch_base) {
> + qphy->phy.notify_connect = m31usb_phy_notify_connect;
> + qphy->phy.notify_disconnect = m31usb_phy_notify_disconnect;
> + }
> +
> + ret = usb_add_phy_dev(&qphy->phy);
> +
> + return ret;
> +}
> +
> +static int m31usb_phy_remove(struct platform_device *pdev)
Please make this return void and pass it to .remove_new
> +{
> + struct m31usb_phy *qphy = platform_get_drvdata(pdev);
> +
> + usb_remove_phy(&qphy->phy);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id m31usb_phy_id_table[] = {
> + { .compatible = "qcom,m31-usb-hsphy",},
> + { .compatible = "qcom,ipq5332-m31-usb-hsphy",},
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, m31usb_phy_id_table);
> +
> +static struct platform_driver m31usb_phy_driver = {
> + .probe = m31usb_phy_probe,
> + .remove = m31usb_phy_remove,
> + .driver = {
> + .name = "qcom-m31usb-phy",
> + .of_match_table = of_match_ptr(m31usb_phy_id_table),
of_match_ptr is unnecessary, this driver depends on OF.
Konrad
> + },
> +};
> +
> +module_platform_driver(m31usb_phy_driver);
> +
> +MODULE_DESCRIPTION("USB2 Qualcomm M31 HSPHY driver");
> +MODULE_LICENSE("GPL");
Two minor nits on top of the review:
On 07/06/2023 14:54, Konrad Dybcio wrote:
> On 7.06.2023 12:56, Varadarajan Narayanan wrote:
>> Add the M31 USB2 phy driver
>>
>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>> ---
>> drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 360 insertions(+)
>> create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c
>> new file mode 100644
>> index 0000000..d29a91e
>> --- /dev/null
>> +++ b/drivers/phy/qualcomm/phy-qcom-m31.c
>> @@ -0,0 +1,360 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/usb/phy.h>
>> +#include <linux/reset.h>
>> +#include <linux/of_device.h>
> Please sort these
>
>> +
>> +enum clk_reset_action {
>> + CLK_RESET_DEASSERT = 0,
>> + CLK_RESET_ASSERT = 1
>> +};
>> +
>> +#define USB2PHY_PORT_POWERDOWN 0xA4
>> +#define POWER_UP BIT(0)
>> +#define POWER_DOWN 0
>> +
>> +#define USB2PHY_PORT_UTMI_CTRL1 0x40
>> +
>> +#define USB2PHY_PORT_UTMI_CTRL2 0x44
>> +#define UTMI_ULPI_SEL BIT(7)
>> +#define UTMI_TEST_MUX_SEL BIT(6)
>> +
>> +#define HS_PHY_CTRL_REG 0x10
>> +#define UTMI_OTG_VBUS_VALID BIT(20)
>> +#define SW_SESSVLD_SEL BIT(28)
>> +
>> +#define USB_PHY_CFG0 0x94
>> +#define USB_PHY_UTMI_CTRL5 0x50
>> +#define USB_PHY_FSEL_SEL 0xB8
>> +#define USB_PHY_HS_PHY_CTRL_COMMON0 0x54
>> +#define USB_PHY_REFCLK_CTRL 0xA0
>> +#define USB_PHY_HS_PHY_CTRL2 0x64
>> +#define USB_PHY_UTMI_CTRL0 0x3c
>> +#define USB2PHY_USB_PHY_M31_XCFGI_1 0xBC
>> +#define USB2PHY_USB_PHY_M31_XCFGI_4 0xC8
>> +#define USB2PHY_USB_PHY_M31_XCFGI_5 0xCC
>> +#define USB2PHY_USB_PHY_M31_XCFGI_11 0xE4
> Could you sort them address-wise?
... and lowercase the hex values, please.
>
>> +
>> +#define USB2_0_TX_ENABLE BIT(2)
>> +#define HSTX_SLEW_RATE_565PS 3
>> +#define PLL_CHARGING_PUMP_CURRENT_35UA (3 << 3)
>> +#define ODT_VALUE_38_02_OHM (3 << 6)
>> +#define ODT_VALUE_45_02_OHM BIT(2)
>> +#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA (1)
> Weird mix of values, bits, bitfields.. perhaps BIT(n) and
> GENMASK() (+ FIELD_PREP) would be more suitable?
>
>> +
>> +#define UTMI_PHY_OVERRIDE_EN BIT(1)
>> +#define POR_EN BIT(1)
> Please associate these with their registers, like
>
> #define FOO_REG 0xf00
> #define POR_EN BIT(1)
>
>> +#define FREQ_SEL BIT(0)
>> +#define COMMONONN BIT(7)
>> +#define FSEL BIT(4)
>> +#define RETENABLEN BIT(3)
>> +#define USB2_SUSPEND_N_SEL BIT(3)
>> +#define USB2_SUSPEND_N BIT(2)
>> +#define USB2_UTMI_CLK_EN BIT(1)
>> +#define CLKCORE BIT(1)
>> +#define ATERESET ~BIT(0)
>> +#define FREQ_24MHZ (5 << 4)
>> +#define XCFG_COARSE_TUNE_NUM (2 << 0)
>> +#define XCFG_FINE_TUNE_NUM (1 << 3)
> same comment
>
>> +
>> +static void m31usb_write_readback(void *base, u32 offset,
>> + const u32 mask, u32 val);
> We don't need this forward-definition, just move the function up.
>
>> +
>> +struct m31usb_phy {
>> + struct usb_phy phy;
>> + void __iomem *base;
>> + void __iomem *qscratch_base;
>> +
>> + struct reset_control *phy_reset;
>> +
>> + bool cable_connected;
>> + bool suspended;
>> + bool ulpi_mode;
>> +};
>> +
>> +static void m31usb_reset(struct m31usb_phy *qphy, u32 action)
>> +{
>> + if (action == CLK_RESET_ASSERT)
>> + reset_control_assert(qphy->phy_reset);
>> + else
>> + reset_control_deassert(qphy->phy_reset);
>> + wmb(); /* ensure data is written to hw register */
> Please move the comment above the call.
>
>> +}
Or even better just inline the function. I was never a fan of such
multiplexers.
Also does wmb() make sense here? Doesn't regmap (which is used by reset
controller) remove the need for it?
>> +
>> +static void m31usb_phy_enable_clock(struct m31usb_phy *qphy)
>> +{
>> + /* Enable override ctrl */
>> + writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
> Some of the comments are missing a space before '*/'
>
> Also, please consider adding some newlines to logically split the
> actions.
On Wed, Jun 07, 2023 at 01:54:23PM +0200, Konrad Dybcio wrote:
>
>
> On 7.06.2023 12:56, Varadarajan Narayanan wrote:
> > Add the M31 USB2 phy driver
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> > drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 360 insertions(+)
> > create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c
> > new file mode 100644
> > index 0000000..d29a91e
> > --- /dev/null
> > +++ b/drivers/phy/qualcomm/phy-qcom-m31.c
> > @@ -0,0 +1,360 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/usb/phy.h>
> > +#include <linux/reset.h>
> > +#include <linux/of_device.h>
> Please sort these
Ok.
> > +
> > +enum clk_reset_action {
> > + CLK_RESET_DEASSERT = 0,
> > + CLK_RESET_ASSERT = 1
> > +};
> > +
> > +#define USB2PHY_PORT_POWERDOWN 0xA4
> > +#define POWER_UP BIT(0)
> > +#define POWER_DOWN 0
> > +
> > +#define USB2PHY_PORT_UTMI_CTRL1 0x40
> > +
> > +#define USB2PHY_PORT_UTMI_CTRL2 0x44
> > +#define UTMI_ULPI_SEL BIT(7)
> > +#define UTMI_TEST_MUX_SEL BIT(6)
> > +
> > +#define HS_PHY_CTRL_REG 0x10
> > +#define UTMI_OTG_VBUS_VALID BIT(20)
> > +#define SW_SESSVLD_SEL BIT(28)
> > +
> > +#define USB_PHY_CFG0 0x94
> > +#define USB_PHY_UTMI_CTRL5 0x50
> > +#define USB_PHY_FSEL_SEL 0xB8
> > +#define USB_PHY_HS_PHY_CTRL_COMMON0 0x54
> > +#define USB_PHY_REFCLK_CTRL 0xA0
> > +#define USB_PHY_HS_PHY_CTRL2 0x64
> > +#define USB_PHY_UTMI_CTRL0 0x3c
> > +#define USB2PHY_USB_PHY_M31_XCFGI_1 0xBC
> > +#define USB2PHY_USB_PHY_M31_XCFGI_4 0xC8
> > +#define USB2PHY_USB_PHY_M31_XCFGI_5 0xCC
> > +#define USB2PHY_USB_PHY_M31_XCFGI_11 0xE4
> Could you sort them address-wise?
Ok.
> > +
> > +#define USB2_0_TX_ENABLE BIT(2)
> > +#define HSTX_SLEW_RATE_565PS 3
> > +#define PLL_CHARGING_PUMP_CURRENT_35UA (3 << 3)
> > +#define ODT_VALUE_38_02_OHM (3 << 6)
> > +#define ODT_VALUE_45_02_OHM BIT(2)
> > +#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA (1)
> Weird mix of values, bits, bitfields.. perhaps BIT(n) and
> GENMASK() (+ FIELD_PREP) would be more suitable?
Ok.
> > +
> > +#define UTMI_PHY_OVERRIDE_EN BIT(1)
> > +#define POR_EN BIT(1)
> Please associate these with their registers, like
Ok.
> #define FOO_REG 0xf00
> #define POR_EN BIT(1)
>
> > +#define FREQ_SEL BIT(0)
> > +#define COMMONONN BIT(7)
> > +#define FSEL BIT(4)
> > +#define RETENABLEN BIT(3)
> > +#define USB2_SUSPEND_N_SEL BIT(3)
> > +#define USB2_SUSPEND_N BIT(2)
> > +#define USB2_UTMI_CLK_EN BIT(1)
> > +#define CLKCORE BIT(1)
> > +#define ATERESET ~BIT(0)
> > +#define FREQ_24MHZ (5 << 4)
> > +#define XCFG_COARSE_TUNE_NUM (2 << 0)
> > +#define XCFG_FINE_TUNE_NUM (1 << 3)
> same comment
Ok.
> > +
> > +static void m31usb_write_readback(void *base, u32 offset,
> > + const u32 mask, u32 val);
> We don't need this forward-definition, just move the function up.
>
> > +
> > +struct m31usb_phy {
> > + struct usb_phy phy;
> > + void __iomem *base;
> > + void __iomem *qscratch_base;
> > +
> > + struct reset_control *phy_reset;
> > +
> > + bool cable_connected;
> > + bool suspended;
> > + bool ulpi_mode;
> > +};
> > +
> > +static void m31usb_reset(struct m31usb_phy *qphy, u32 action)
> > +{
> > + if (action == CLK_RESET_ASSERT)
> > + reset_control_assert(qphy->phy_reset);
> > + else
> > + reset_control_deassert(qphy->phy_reset);
> > + wmb(); /* ensure data is written to hw register */
> Please move the comment above the call.
This is used only once. Hence, will move it to the calling location itself.
> > +}
> > +
> > +static void m31usb_phy_enable_clock(struct m31usb_phy *qphy)
> > +{
> > + /* Enable override ctrl */
> > + writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
> Some of the comments are missing a space before '*/'
>
> Also, please consider adding some newlines to logically split the
> actions.
>
> > + /* Enable POR*/
> > + writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5);
> > + udelay(15);
> > + /* Configure frequency select value*/
> > + writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL);
> > + /* Configure refclk frequency */
> > + writel(COMMONONN | FSEL | RETENABLEN,
> > + qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0);
> > + /* select refclk source */
> > + writel(CLKCORE, qphy->base + USB_PHY_REFCLK_CTRL);
> > + /* select ATERESET*/
> > + writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5);
> > + writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> > + qphy->base + USB_PHY_HS_PHY_CTRL2);
> > + writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5);
> > + writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> > + qphy->base + USB_PHY_HS_PHY_CTRL2);
> > + /* Disable override ctrl */
> > + writel(0x0, qphy->base + USB_PHY_CFG0);
> > +}
> > +
> > +static void ipq5332_m31usb_phy_enable_clock(struct m31usb_phy *qphy)
> > +{
> > + /* Enable override ctrl */
> > + writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
> > + /* Enable POR*/
> ditto
>
> > + writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5);
> > + udelay(15);
> > + /* Configure frequency select value*/
> > + writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL);
> > + /* Configure refclk frequency */
> > + writel(COMMONONN | FREQ_24MHZ | RETENABLEN,
> > + qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0);
> > + /* select ATERESET*/
> > + writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5);
> > + writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> > + qphy->base + USB_PHY_HS_PHY_CTRL2);
> > + writel(XCFG_COARSE_TUNE_NUM | XCFG_FINE_TUNE_NUM,
> > + qphy->base + USB2PHY_USB_PHY_M31_XCFGI_11);
> > + /* Adjust HSTX slew rate to 565 ps*/
> > + /* Adjust PLL lock Time counter for release clock to 35uA */
> > + /* Adjust Manual control ODT value to 38.02 Ohm */
> > + writel(HSTX_SLEW_RATE_565PS | PLL_CHARGING_PUMP_CURRENT_35UA |
> > + ODT_VALUE_38_02_OHM, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_4);
> These functions seem very similar, with the main difference being that
> IPQ5332 adds some more writes at the end. Perhaps some commonizing could
> be done?
>
> > + /*
> > + * Enable to always turn on USB 2.0 TX driver
> > + * when system is in USB 2.0 HS mode
> > + */
> > + writel(USB2_0_TX_ENABLE, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_1);
> > + /* Adjust Manual control ODT value to 45.02 Ohm */
> > + /* Adjust HSTX Pre-emphasis level to 0.55mA */
> > + writel(ODT_VALUE_45_02_OHM | HSTX_PRE_EMPHASIS_LEVEL_0_55MA,
> > + qphy->base + USB2PHY_USB_PHY_M31_XCFGI_5);
> > + udelay(4);
> > + writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5);
> > + writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> > + qphy->base + USB_PHY_HS_PHY_CTRL2);
> > +}
Will change the above to table based register init, based on
compatible/data.
> > +
> > +static int m31usb_phy_init(struct usb_phy *phy)
> > +{
> > + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> > +
> > + /* Perform phy reset */
> > + m31usb_reset(qphy, CLK_RESET_ASSERT);
> > + usleep_range(1, 5);
> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
>
> this may not work as intended
Will change it to udelay(5).
> > + m31usb_reset(qphy, CLK_RESET_DEASSERT);
> > +
> > + /* configure for ULPI mode if requested */
> > + if (qphy->ulpi_mode)
> > + writel_relaxed(0x0, qphy->base + USB2PHY_PORT_UTMI_CTRL2);
> > +
> > + /* Enable the PHY */
> > + writel_relaxed(POWER_UP, qphy->base + USB2PHY_PORT_POWERDOWN);
> > +
> > + /* Make sure above write completed */
> > + wmb();
> As you're calling wmb in the reset func, dropping _relaxed from the
> ULPI and PORT_POWERDOWN writes should work the same
Ok.
> > +
> > + /* Turn on phy ref clock*/
> > + if (of_device_is_compatible(phy->dev->of_node,
> > + "qcom,ipq5332-m31-usb-hsphy"))
> > + ipq5332_m31usb_phy_enable_clock(qphy);
> > + else
> > + m31usb_phy_enable_clock(qphy);
> This should be done using OF match data.
Ok.
> > +
> > + /* Set OTG VBUS Valid from HSPHY to controller */
> > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> > + UTMI_OTG_VBUS_VALID, UTMI_OTG_VBUS_VALID);
> > + /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
> > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> > + SW_SESSVLD_SEL, SW_SESSVLD_SEL);
> > +
> > + return 0;
> > +}
> > +
> > +static void m31usb_phy_shutdown(struct usb_phy *phy)
> > +{
> > + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> > +
> > + /* Disable the PHY */
> > + writel_relaxed(POWER_DOWN, qphy->base + USB2PHY_PORT_POWERDOWN);
> > + /* Make sure above write completed */
> > + wmb();
> > +}
> > +
> > +static void m31usb_write_readback(void *base, u32 offset,
> > + const u32 mask, u32 val)
> The indentation here makes `const u32..` misaligned.
>
> > +{
> > + u32 write_val, tmp = readl_relaxed(base + offset);
> > +
> > + tmp &= ~mask; /* retain other bits */
> > + write_val = tmp | val;
> > +
> > + writel_relaxed(write_val, base + offset);
> > + /* Make sure above write completed */
> > + wmb();
> > +
> > + /* Read back to see if val was written */
> > + tmp = readl_relaxed(base + offset);
> > + tmp &= mask; /* clear other bits */
> > +
> > + if (tmp != val)
> > + pr_err("%s: write: %x to QSCRATCH: %x FAILED\n",
> > + __func__, val, offset);
> > +}
> > +
> > +static int m31usb_phy_notify_connect(struct usb_phy *phy,
> > + enum usb_device_speed speed)
> > +{
> > + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> > +
> > + qphy->cable_connected = true;
> > +
> > + dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected);
> spurious space at the beginning of the string
>
> > +
> > + /* Set OTG VBUS Valid from HSPHY to controller */
> > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> > + UTMI_OTG_VBUS_VALID,
> > + UTMI_OTG_VBUS_VALID);
> Please align the lines with the previous opening bracket
>
> > +
> > + /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
> > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> > + SW_SESSVLD_SEL, SW_SESSVLD_SEL);
> > +
> > + dev_dbg(phy->dev, "M31USB2 phy connect notification\n");
> > + return 0;
> > +}
> > +
> > +static int m31usb_phy_notify_disconnect(struct usb_phy *phy,
> > + enum usb_device_speed speed)
> > +{
> > + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> > +
> > + qphy->cable_connected = false;
> > +
> > + dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected);
> > +
> > + /* Set OTG VBUS Valid from HSPHY to controller */
> > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> > + UTMI_OTG_VBUS_VALID, 0);
> > +
> > + /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
> > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> > + SW_SESSVLD_SEL, 0);
> > +
> > + dev_dbg(phy->dev, "M31USB2 phy disconnect notification\n");
> > + return 0;
> > +}
Will remove these functions. They are accessing 'qscratch' area that
is part of the controller space. It doesn't belong in this driver.
> > +static int m31usb_phy_probe(struct platform_device *pdev)
> > +{
> > + struct m31usb_phy *qphy;
> > + struct device *dev = &pdev->dev;
> > + struct resource *res;
> > + int ret;
> > + const char *phy_type;
> Please sort these in a reverse-Christmas-tree order.
ok.
> > +
> > + qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
> > + if (!qphy)
> > + return -ENOMEM;
> > +
> > + qphy->phy.dev = dev;
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > + "m31usb_phy_base");
> > + qphy->base = devm_ioremap_resource(dev, res);
> devm_platform_get_and_ioremap_resource?
ok.
> > + if (IS_ERR(qphy->base))
> > + return PTR_ERR(qphy->base);
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > + "qscratch_base");
> > + if (res) {
> Do we expect platforms without this register space?
>
> > + qphy->qscratch_base = devm_ioremap(dev, res->start,
> > + resource_size(res));
> > + if (IS_ERR(qphy->qscratch_base)) {
> > + dev_dbg(dev, "couldn't ioremap qscratch_base\n");
> > + qphy->qscratch_base = NULL;
> > + }
> > + }
Will remove this qscratch code.
> > + qphy->phy_reset = devm_reset_control_get(dev, "usb2_phy_reset");
> not _exclusive?
Ok.
> > + if (IS_ERR(qphy->phy_reset))
> > + return PTR_ERR(qphy->phy_reset);
> > +
> > + qphy->ulpi_mode = false;
> > + ret = of_property_read_string(dev->of_node, "phy_type", &phy_type);
> of_usb_get_phy_mode?
Ok.
> > +
> > + if (!ret) {
> > + if (!strcasecmp(phy_type, "ulpi"))
> > + qphy->ulpi_mode = true;
> > + } else {
> > + dev_err(dev, "error reading phy_type property\n");
> > + return ret;
> > + }
> > +
> > + platform_set_drvdata(pdev, qphy);
> > +
> > + qphy->phy.label = "qcom-m31usb-phy";
> > + qphy->phy.init = m31usb_phy_init;
> > + qphy->phy.shutdown = m31usb_phy_shutdown;
> > + qphy->phy.type = USB_PHY_TYPE_USB2;
> > +
> > + if (qphy->qscratch_base) {
> > + qphy->phy.notify_connect = m31usb_phy_notify_connect;
> > + qphy->phy.notify_disconnect = m31usb_phy_notify_disconnect;
> > + }
> > +
> > + ret = usb_add_phy_dev(&qphy->phy);
> > +
> > + return ret;
> > +}
> > +
> > +static int m31usb_phy_remove(struct platform_device *pdev)
> Please make this return void and pass it to .remove_new
Ok.
> > +{
> > + struct m31usb_phy *qphy = platform_get_drvdata(pdev);
> > +
> > + usb_remove_phy(&qphy->phy);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id m31usb_phy_id_table[] = {
> > + { .compatible = "qcom,m31-usb-hsphy",},
> > + { .compatible = "qcom,ipq5332-m31-usb-hsphy",},
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, m31usb_phy_id_table);
> > +
> > +static struct platform_driver m31usb_phy_driver = {
> > + .probe = m31usb_phy_probe,
> > + .remove = m31usb_phy_remove,
> > + .driver = {
> > + .name = "qcom-m31usb-phy",
> > + .of_match_table = of_match_ptr(m31usb_phy_id_table),
> of_match_ptr is unnecessary, this driver depends on OF.
Ok.
Thanks
Varada
> Konrad
> > + },
> > +};
> > +
> > +module_platform_driver(m31usb_phy_driver);
> > +
> > +MODULE_DESCRIPTION("USB2 Qualcomm M31 HSPHY driver");
> > +MODULE_LICENSE("GPL");
On Wed, Jun 07, 2023 at 03:29:18PM +0300, Dmitry Baryshkov wrote:
> Two minor nits on top of the review:
>
> On 07/06/2023 14:54, Konrad Dybcio wrote:
> >On 7.06.2023 12:56, Varadarajan Narayanan wrote:
> >>Add the M31 USB2 phy driver
> >>
> >>Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> >>---
> >> drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 360 insertions(+)
> >> create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c
> >>
> >>diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c
> >>new file mode 100644
> >>index 0000000..d29a91e
> >>--- /dev/null
> >>+++ b/drivers/phy/qualcomm/phy-qcom-m31.c
> >>@@ -0,0 +1,360 @@
> >>+// SPDX-License-Identifier: GPL-2.0+
> >>+/*
> >>+ * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
> >>+ */
> >>+
> >>+#include <linux/module.h>
> >>+#include <linux/kernel.h>
> >>+#include <linux/err.h>
> >>+#include <linux/slab.h>
> >>+#include <linux/clk.h>
> >>+#include <linux/delay.h>
> >>+#include <linux/io.h>
> >>+#include <linux/of.h>
> >>+#include <linux/platform_device.h>
> >>+#include <linux/usb/phy.h>
> >>+#include <linux/reset.h>
> >>+#include <linux/of_device.h>
> >Please sort these
> >
> >>+
> >>+enum clk_reset_action {
> >>+ CLK_RESET_DEASSERT = 0,
> >>+ CLK_RESET_ASSERT = 1
> >>+};
> >>+
> >>+#define USB2PHY_PORT_POWERDOWN 0xA4
> >>+#define POWER_UP BIT(0)
> >>+#define POWER_DOWN 0
> >>+
> >>+#define USB2PHY_PORT_UTMI_CTRL1 0x40
> >>+
> >>+#define USB2PHY_PORT_UTMI_CTRL2 0x44
> >>+#define UTMI_ULPI_SEL BIT(7)
> >>+#define UTMI_TEST_MUX_SEL BIT(6)
> >>+
> >>+#define HS_PHY_CTRL_REG 0x10
> >>+#define UTMI_OTG_VBUS_VALID BIT(20)
> >>+#define SW_SESSVLD_SEL BIT(28)
> >>+
> >>+#define USB_PHY_CFG0 0x94
> >>+#define USB_PHY_UTMI_CTRL5 0x50
> >>+#define USB_PHY_FSEL_SEL 0xB8
> >>+#define USB_PHY_HS_PHY_CTRL_COMMON0 0x54
> >>+#define USB_PHY_REFCLK_CTRL 0xA0
> >>+#define USB_PHY_HS_PHY_CTRL2 0x64
> >>+#define USB_PHY_UTMI_CTRL0 0x3c
> >>+#define USB2PHY_USB_PHY_M31_XCFGI_1 0xBC
> >>+#define USB2PHY_USB_PHY_M31_XCFGI_4 0xC8
> >>+#define USB2PHY_USB_PHY_M31_XCFGI_5 0xCC
> >>+#define USB2PHY_USB_PHY_M31_XCFGI_11 0xE4
> >Could you sort them address-wise?
>
> ... and lowercase the hex values, please.
Ok.
> >>+
> >>+#define USB2_0_TX_ENABLE BIT(2)
> >>+#define HSTX_SLEW_RATE_565PS 3
> >>+#define PLL_CHARGING_PUMP_CURRENT_35UA (3 << 3)
> >>+#define ODT_VALUE_38_02_OHM (3 << 6)
> >>+#define ODT_VALUE_45_02_OHM BIT(2)
> >>+#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA (1)
> >Weird mix of values, bits, bitfields.. perhaps BIT(n) and
> >GENMASK() (+ FIELD_PREP) would be more suitable?
> >
> >>+
> >>+#define UTMI_PHY_OVERRIDE_EN BIT(1)
> >>+#define POR_EN BIT(1)
> >Please associate these with their registers, like
> >
> >#define FOO_REG 0xf00
> > #define POR_EN BIT(1)
> >
> >>+#define FREQ_SEL BIT(0)
> >>+#define COMMONONN BIT(7)
> >>+#define FSEL BIT(4)
> >>+#define RETENABLEN BIT(3)
> >>+#define USB2_SUSPEND_N_SEL BIT(3)
> >>+#define USB2_SUSPEND_N BIT(2)
> >>+#define USB2_UTMI_CLK_EN BIT(1)
> >>+#define CLKCORE BIT(1)
> >>+#define ATERESET ~BIT(0)
> >>+#define FREQ_24MHZ (5 << 4)
> >>+#define XCFG_COARSE_TUNE_NUM (2 << 0)
> >>+#define XCFG_FINE_TUNE_NUM (1 << 3)
> >same comment
> >
> >>+
> >>+static void m31usb_write_readback(void *base, u32 offset,
> >>+ const u32 mask, u32 val);
> >We don't need this forward-definition, just move the function up.
> >
> >>+
> >>+struct m31usb_phy {
> >>+ struct usb_phy phy;
> >>+ void __iomem *base;
> >>+ void __iomem *qscratch_base;
> >>+
> >>+ struct reset_control *phy_reset;
> >>+
> >>+ bool cable_connected;
> >>+ bool suspended;
> >>+ bool ulpi_mode;
> >>+};
> >>+
> >>+static void m31usb_reset(struct m31usb_phy *qphy, u32 action)
> >>+{
> >>+ if (action == CLK_RESET_ASSERT)
> >>+ reset_control_assert(qphy->phy_reset);
> >>+ else
> >>+ reset_control_deassert(qphy->phy_reset);
> >>+ wmb(); /* ensure data is written to hw register */
> >Please move the comment above the call.
> >
> >>+}
>
> Or even better just inline the function. I was never a fan of such
> multiplexers.
>
> Also does wmb() make sense here? Doesn't regmap (which is used by reset
> controller) remove the need for it?
Will inline and remove the wmb.
Thanks
Varada
> >>+
> >>+static void m31usb_phy_enable_clock(struct m31usb_phy *qphy)
> >>+{
> >>+ /* Enable override ctrl */
> >>+ writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
> >Some of the comments are missing a space before '*/'
> >
> >Also, please consider adding some newlines to logically split the
> >actions.
>
>
> --
> With best wishes
> Dmitry
>
On 07-06-23, 13:54, Konrad Dybcio wrote:
>
>
> On 7.06.2023 12:56, Varadarajan Narayanan wrote:
> > Add the M31 USB2 phy driver
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> > drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 360 insertions(+)
> > create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c
> > new file mode 100644
> > index 0000000..d29a91e
> > --- /dev/null
> > +++ b/drivers/phy/qualcomm/phy-qcom-m31.c
> > @@ -0,0 +1,360 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/usb/phy.h>
> > +#include <linux/reset.h>
> > +#include <linux/of_device.h>
> Please sort these
>
> > +
> > +enum clk_reset_action {
> > + CLK_RESET_DEASSERT = 0,
> > + CLK_RESET_ASSERT = 1
> > +};
> > +
> > +#define USB2PHY_PORT_POWERDOWN 0xA4
> > +#define POWER_UP BIT(0)
> > +#define POWER_DOWN 0
> > +
> > +#define USB2PHY_PORT_UTMI_CTRL1 0x40
> > +
> > +#define USB2PHY_PORT_UTMI_CTRL2 0x44
> > +#define UTMI_ULPI_SEL BIT(7)
> > +#define UTMI_TEST_MUX_SEL BIT(6)
> > +
> > +#define HS_PHY_CTRL_REG 0x10
> > +#define UTMI_OTG_VBUS_VALID BIT(20)
> > +#define SW_SESSVLD_SEL BIT(28)
> > +
> > +#define USB_PHY_CFG0 0x94
> > +#define USB_PHY_UTMI_CTRL5 0x50
> > +#define USB_PHY_FSEL_SEL 0xB8
> > +#define USB_PHY_HS_PHY_CTRL_COMMON0 0x54
> > +#define USB_PHY_REFCLK_CTRL 0xA0
> > +#define USB_PHY_HS_PHY_CTRL2 0x64
> > +#define USB_PHY_UTMI_CTRL0 0x3c
> > +#define USB2PHY_USB_PHY_M31_XCFGI_1 0xBC
> > +#define USB2PHY_USB_PHY_M31_XCFGI_4 0xC8
> > +#define USB2PHY_USB_PHY_M31_XCFGI_5 0xCC
> > +#define USB2PHY_USB_PHY_M31_XCFGI_11 0xE4
> Could you sort them address-wise?
and lower case hex values as well please
new file mode 100644
@@ -0,0 +1,360 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/usb/phy.h>
+#include <linux/reset.h>
+#include <linux/of_device.h>
+
+enum clk_reset_action {
+ CLK_RESET_DEASSERT = 0,
+ CLK_RESET_ASSERT = 1
+};
+
+#define USB2PHY_PORT_POWERDOWN 0xA4
+#define POWER_UP BIT(0)
+#define POWER_DOWN 0
+
+#define USB2PHY_PORT_UTMI_CTRL1 0x40
+
+#define USB2PHY_PORT_UTMI_CTRL2 0x44
+#define UTMI_ULPI_SEL BIT(7)
+#define UTMI_TEST_MUX_SEL BIT(6)
+
+#define HS_PHY_CTRL_REG 0x10
+#define UTMI_OTG_VBUS_VALID BIT(20)
+#define SW_SESSVLD_SEL BIT(28)
+
+#define USB_PHY_CFG0 0x94
+#define USB_PHY_UTMI_CTRL5 0x50
+#define USB_PHY_FSEL_SEL 0xB8
+#define USB_PHY_HS_PHY_CTRL_COMMON0 0x54
+#define USB_PHY_REFCLK_CTRL 0xA0
+#define USB_PHY_HS_PHY_CTRL2 0x64
+#define USB_PHY_UTMI_CTRL0 0x3c
+#define USB2PHY_USB_PHY_M31_XCFGI_1 0xBC
+#define USB2PHY_USB_PHY_M31_XCFGI_4 0xC8
+#define USB2PHY_USB_PHY_M31_XCFGI_5 0xCC
+#define USB2PHY_USB_PHY_M31_XCFGI_11 0xE4
+
+#define USB2_0_TX_ENABLE BIT(2)
+#define HSTX_SLEW_RATE_565PS 3
+#define PLL_CHARGING_PUMP_CURRENT_35UA (3 << 3)
+#define ODT_VALUE_38_02_OHM (3 << 6)
+#define ODT_VALUE_45_02_OHM BIT(2)
+#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA (1)
+
+#define UTMI_PHY_OVERRIDE_EN BIT(1)
+#define POR_EN BIT(1)
+#define FREQ_SEL BIT(0)
+#define COMMONONN BIT(7)
+#define FSEL BIT(4)
+#define RETENABLEN BIT(3)
+#define USB2_SUSPEND_N_SEL BIT(3)
+#define USB2_SUSPEND_N BIT(2)
+#define USB2_UTMI_CLK_EN BIT(1)
+#define CLKCORE BIT(1)
+#define ATERESET ~BIT(0)
+#define FREQ_24MHZ (5 << 4)
+#define XCFG_COARSE_TUNE_NUM (2 << 0)
+#define XCFG_FINE_TUNE_NUM (1 << 3)
+
+static void m31usb_write_readback(void *base, u32 offset,
+ const u32 mask, u32 val);
+
+struct m31usb_phy {
+ struct usb_phy phy;
+ void __iomem *base;
+ void __iomem *qscratch_base;
+
+ struct reset_control *phy_reset;
+
+ bool cable_connected;
+ bool suspended;
+ bool ulpi_mode;
+};
+
+static void m31usb_reset(struct m31usb_phy *qphy, u32 action)
+{
+ if (action == CLK_RESET_ASSERT)
+ reset_control_assert(qphy->phy_reset);
+ else
+ reset_control_deassert(qphy->phy_reset);
+ wmb(); /* ensure data is written to hw register */
+}
+
+static void m31usb_phy_enable_clock(struct m31usb_phy *qphy)
+{
+ /* Enable override ctrl */
+ writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
+ /* Enable POR*/
+ writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5);
+ udelay(15);
+ /* Configure frequency select value*/
+ writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL);
+ /* Configure refclk frequency */
+ writel(COMMONONN | FSEL | RETENABLEN,
+ qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0);
+ /* select refclk source */
+ writel(CLKCORE, qphy->base + USB_PHY_REFCLK_CTRL);
+ /* select ATERESET*/
+ writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5);
+ writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
+ qphy->base + USB_PHY_HS_PHY_CTRL2);
+ writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5);
+ writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
+ qphy->base + USB_PHY_HS_PHY_CTRL2);
+ /* Disable override ctrl */
+ writel(0x0, qphy->base + USB_PHY_CFG0);
+}
+
+static void ipq5332_m31usb_phy_enable_clock(struct m31usb_phy *qphy)
+{
+ /* Enable override ctrl */
+ writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
+ /* Enable POR*/
+ writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5);
+ udelay(15);
+ /* Configure frequency select value*/
+ writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL);
+ /* Configure refclk frequency */
+ writel(COMMONONN | FREQ_24MHZ | RETENABLEN,
+ qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0);
+ /* select ATERESET*/
+ writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5);
+ writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
+ qphy->base + USB_PHY_HS_PHY_CTRL2);
+ writel(XCFG_COARSE_TUNE_NUM | XCFG_FINE_TUNE_NUM,
+ qphy->base + USB2PHY_USB_PHY_M31_XCFGI_11);
+ /* Adjust HSTX slew rate to 565 ps*/
+ /* Adjust PLL lock Time counter for release clock to 35uA */
+ /* Adjust Manual control ODT value to 38.02 Ohm */
+ writel(HSTX_SLEW_RATE_565PS | PLL_CHARGING_PUMP_CURRENT_35UA |
+ ODT_VALUE_38_02_OHM, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_4);
+ /*
+ * Enable to always turn on USB 2.0 TX driver
+ * when system is in USB 2.0 HS mode
+ */
+ writel(USB2_0_TX_ENABLE, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_1);
+ /* Adjust Manual control ODT value to 45.02 Ohm */
+ /* Adjust HSTX Pre-emphasis level to 0.55mA */
+ writel(ODT_VALUE_45_02_OHM | HSTX_PRE_EMPHASIS_LEVEL_0_55MA,
+ qphy->base + USB2PHY_USB_PHY_M31_XCFGI_5);
+ udelay(4);
+ writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5);
+ writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
+ qphy->base + USB_PHY_HS_PHY_CTRL2);
+}
+
+static int m31usb_phy_init(struct usb_phy *phy)
+{
+ struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
+
+ /* Perform phy reset */
+ m31usb_reset(qphy, CLK_RESET_ASSERT);
+ usleep_range(1, 5);
+ m31usb_reset(qphy, CLK_RESET_DEASSERT);
+
+ /* configure for ULPI mode if requested */
+ if (qphy->ulpi_mode)
+ writel_relaxed(0x0, qphy->base + USB2PHY_PORT_UTMI_CTRL2);
+
+ /* Enable the PHY */
+ writel_relaxed(POWER_UP, qphy->base + USB2PHY_PORT_POWERDOWN);
+
+ /* Make sure above write completed */
+ wmb();
+
+ /* Turn on phy ref clock*/
+ if (of_device_is_compatible(phy->dev->of_node,
+ "qcom,ipq5332-m31-usb-hsphy"))
+ ipq5332_m31usb_phy_enable_clock(qphy);
+ else
+ m31usb_phy_enable_clock(qphy);
+
+ /* Set OTG VBUS Valid from HSPHY to controller */
+ m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
+ UTMI_OTG_VBUS_VALID, UTMI_OTG_VBUS_VALID);
+ /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
+ m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
+ SW_SESSVLD_SEL, SW_SESSVLD_SEL);
+
+ return 0;
+}
+
+static void m31usb_phy_shutdown(struct usb_phy *phy)
+{
+ struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
+
+ /* Disable the PHY */
+ writel_relaxed(POWER_DOWN, qphy->base + USB2PHY_PORT_POWERDOWN);
+ /* Make sure above write completed */
+ wmb();
+}
+
+static void m31usb_write_readback(void *base, u32 offset,
+ const u32 mask, u32 val)
+{
+ u32 write_val, tmp = readl_relaxed(base + offset);
+
+ tmp &= ~mask; /* retain other bits */
+ write_val = tmp | val;
+
+ writel_relaxed(write_val, base + offset);
+ /* Make sure above write completed */
+ wmb();
+
+ /* Read back to see if val was written */
+ tmp = readl_relaxed(base + offset);
+ tmp &= mask; /* clear other bits */
+
+ if (tmp != val)
+ pr_err("%s: write: %x to QSCRATCH: %x FAILED\n",
+ __func__, val, offset);
+}
+
+static int m31usb_phy_notify_connect(struct usb_phy *phy,
+ enum usb_device_speed speed)
+{
+ struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
+
+ qphy->cable_connected = true;
+
+ dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected);
+
+ /* Set OTG VBUS Valid from HSPHY to controller */
+ m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
+ UTMI_OTG_VBUS_VALID,
+ UTMI_OTG_VBUS_VALID);
+
+ /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
+ m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
+ SW_SESSVLD_SEL, SW_SESSVLD_SEL);
+
+ dev_dbg(phy->dev, "M31USB2 phy connect notification\n");
+ return 0;
+}
+
+static int m31usb_phy_notify_disconnect(struct usb_phy *phy,
+ enum usb_device_speed speed)
+{
+ struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
+
+ qphy->cable_connected = false;
+
+ dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected);
+
+ /* Set OTG VBUS Valid from HSPHY to controller */
+ m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
+ UTMI_OTG_VBUS_VALID, 0);
+
+ /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
+ m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
+ SW_SESSVLD_SEL, 0);
+
+ dev_dbg(phy->dev, "M31USB2 phy disconnect notification\n");
+ return 0;
+}
+
+static int m31usb_phy_probe(struct platform_device *pdev)
+{
+ struct m31usb_phy *qphy;
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ int ret;
+ const char *phy_type;
+
+ qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
+ if (!qphy)
+ return -ENOMEM;
+
+ qphy->phy.dev = dev;
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+ "m31usb_phy_base");
+ qphy->base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(qphy->base))
+ return PTR_ERR(qphy->base);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+ "qscratch_base");
+ if (res) {
+ qphy->qscratch_base = devm_ioremap(dev, res->start,
+ resource_size(res));
+ if (IS_ERR(qphy->qscratch_base)) {
+ dev_dbg(dev, "couldn't ioremap qscratch_base\n");
+ qphy->qscratch_base = NULL;
+ }
+ }
+
+ qphy->phy_reset = devm_reset_control_get(dev, "usb2_phy_reset");
+ if (IS_ERR(qphy->phy_reset))
+ return PTR_ERR(qphy->phy_reset);
+
+ qphy->ulpi_mode = false;
+ ret = of_property_read_string(dev->of_node, "phy_type", &phy_type);
+
+ if (!ret) {
+ if (!strcasecmp(phy_type, "ulpi"))
+ qphy->ulpi_mode = true;
+ } else {
+ dev_err(dev, "error reading phy_type property\n");
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, qphy);
+
+ qphy->phy.label = "qcom-m31usb-phy";
+ qphy->phy.init = m31usb_phy_init;
+ qphy->phy.shutdown = m31usb_phy_shutdown;
+ qphy->phy.type = USB_PHY_TYPE_USB2;
+
+ if (qphy->qscratch_base) {
+ qphy->phy.notify_connect = m31usb_phy_notify_connect;
+ qphy->phy.notify_disconnect = m31usb_phy_notify_disconnect;
+ }
+
+ ret = usb_add_phy_dev(&qphy->phy);
+
+ return ret;
+}
+
+static int m31usb_phy_remove(struct platform_device *pdev)
+{
+ struct m31usb_phy *qphy = platform_get_drvdata(pdev);
+
+ usb_remove_phy(&qphy->phy);
+
+ return 0;
+}
+
+static const struct of_device_id m31usb_phy_id_table[] = {
+ { .compatible = "qcom,m31-usb-hsphy",},
+ { .compatible = "qcom,ipq5332-m31-usb-hsphy",},
+ { },
+};
+MODULE_DEVICE_TABLE(of, m31usb_phy_id_table);
+
+static struct platform_driver m31usb_phy_driver = {
+ .probe = m31usb_phy_probe,
+ .remove = m31usb_phy_remove,
+ .driver = {
+ .name = "qcom-m31usb-phy",
+ .of_match_table = of_match_ptr(m31usb_phy_id_table),
+ },
+};
+
+module_platform_driver(m31usb_phy_driver);
+
+MODULE_DESCRIPTION("USB2 Qualcomm M31 HSPHY driver");
+MODULE_LICENSE("GPL");