| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7975c638-29cf-45ce-9d76-b8a93d750eb7@quicinc.com>
Date: Tue, 3 Oct 2023 19:51:42 +0530
From: Praveenkumar I <quic_ipkumar@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
<agross@...nel.org>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <vkoul@...nel.org>,
<kishon@...nel.org>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
<gregkh@...uxfoundation.org>, <catalin.marinas@....com>,
<will@...nel.org>, <p.zabel@...gutronix.de>,
<geert+renesas@...der.be>, <arnd@...db.de>,
<neil.armstrong@...aro.org>, <nfraprado@...labora.com>,
<u-kumar1@...com>, <peng.fan@....com>, <quic_wcheng@...cinc.com>,
<quic_varada@...cinc.com>, <linux-arm-msm@...r.kernel.org>,
<linux-phy@...ts.infradead.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>
CC: <quic_kathirav@...cinc.com>, <quic_nsekar@...cinc.com>,
<quic_srichara@...cinc.com>
Subject: Re: [PATCH 2/8] phy: qcom: Introduce Super-Speed USB UNIPHY driver
On 9/30/2023 10:48 PM, Dmitry Baryshkov wrote:
> On 29/09/2023 11:42, Praveenkumar I wrote:
>> Adds Qualcomm 22ull Super-Speed USB UNIPHY driver support which
>> is present in Qualcomm IPQ5332 SoC. This PHY is interfaced with
>> SNPS DWC3 USB and SNPS DWC PCIe. Either one of the interface
>> can use the it and selection is done via mux present in TCSR
>> register. This driver selects the PHY for DWC3 USB and handles
>> the reset, clocks and regulator.
>>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@...cinc.com>
>> ---
>> drivers/phy/qualcomm/Kconfig | 11 +
>> drivers/phy/qualcomm/Makefile | 1 +
>> drivers/phy/qualcomm/phy-qcom-uniphy-usb.c | 322 +++++++++++++++++++++
>> 3 files changed, 334 insertions(+)
>> create mode 100644 drivers/phy/qualcomm/phy-qcom-uniphy-usb.c
>>
>> diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
>> index d891058b7c39..7257c8455c53 100644
>> --- a/drivers/phy/qualcomm/Kconfig
>> +++ b/drivers/phy/qualcomm/Kconfig
>> @@ -154,6 +154,17 @@ config PHY_QCOM_M31_USB
>> management. This driver is required even for peripheral only or
>> host only mode configurations.
>> +config PHY_QCOM_UNIPHY_USB
>> + tristate "Qualcomm USB Super-Speed UNIPHY driver"
>
> Can we please have more specific driver name? As I wrote earlier,
> there are two other (different) kinds of Qualcomm UNI PHY devices:
> - DSI / HDMI UNIPHY on apq8064 / msm8974 / msm8960 (?)
> - USB QMP UNI PHY drivers
>
> Adding a driver called UNIPHY, which is not related to those two kinds
> sounds pretty confusing to me.
This UNIPHY is different from above mentioned ones. This a custom
version for 22nm on Qualcomm IPQ5332.
Can we name the driver as phy-qcom-uniphy-usb-ss-22ull.c /
phy-qcom-usb-ss-22ull.c ?
>
>> + depends on USB && (ARCH_QCOM || COMPILE_TEST)
>> + select GENERIC_PHY
>> + help
>> + Enable this to support the Qualcomm USB Super-Speed UNIPHY
>> transceiver
>> + with DWC3 USB core. It handles PHY initialization, clock
>> + management required after resetting the hardware and power
>> + management. This driver is required even for peripheral only or
>> + host only mode configurations.
>> +
>> config PHY_QCOM_USB_HS
>> tristate "Qualcomm USB HS PHY module"
>> depends on USB_ULPI_BUS
>> diff --git a/drivers/phy/qualcomm/Makefile
>> b/drivers/phy/qualcomm/Makefile
>> index ffd609ac6233..c3e0112a7a70 100644
>> --- a/drivers/phy/qualcomm/Makefile
>> +++ b/drivers/phy/qualcomm/Makefile
>> @@ -17,6 +17,7 @@ obj-$(CONFIG_PHY_QCOM_QMP_USB_LEGACY) +=
>> phy-qcom-qmp-usb-legacy.o
>> obj-$(CONFIG_PHY_QCOM_QUSB2) += phy-qcom-qusb2.o
>> obj-$(CONFIG_PHY_QCOM_SNPS_EUSB2) += phy-qcom-snps-eusb2.o
>> obj-$(CONFIG_PHY_QCOM_EUSB2_REPEATER) += phy-qcom-eusb2-repeater.o
>> +obj-$(CONFIG_PHY_QCOM_UNIPHY_USB) += phy-qcom-uniphy-usb.o
>> obj-$(CONFIG_PHY_QCOM_USB_HS) += phy-qcom-usb-hs.o
>> obj-$(CONFIG_PHY_QCOM_USB_HSIC) += phy-qcom-usb-hsic.o
>> obj-$(CONFIG_PHY_QCOM_USB_HS_28NM) += phy-qcom-usb-hs-28nm.o
>> diff --git a/drivers/phy/qualcomm/phy-qcom-uniphy-usb.c
>> b/drivers/phy/qualcomm/phy-qcom-uniphy-usb.c
>> new file mode 100644
>> index 000000000000..fdfc9c225995
>> --- /dev/null
>> +++ b/drivers/phy/qualcomm/phy-qcom-uniphy-usb.c
>
> So, is it a USB PHY or UNI PHY (where I would expect that it handles
> USB and PCIe?)
It is a USB PHY and the PHY name is UNIPHY. Added the usb in the file
name to differentiate it.
>
>> @@ -0,0 +1,322 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights
>> reserved.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/reset.h>
>> +
>> +#define PCIE_USB_COMBO_PHY_CFG_MISC1 0x214
>> +#define PCIE_USB_COMBO_PHY_CFG_RX_AFE_2 0x7C4
>> +#define PCIE_USB_COMBO_PHY_CFG_RX_DLF_DEMUX_2 0x7E8
>> +
>> +/* TCSR_USB_MUX_SEL regiter bits */
>> +#define TCSR_USB_MUX_SEL BIT(0)
>> +
>> +struct phy_init_tbl {
>> + unsigned int offset;
>> + unsigned int val;
>> +};
>> +
>> +#define PHY_INIT_CFG(o, v) \
>> + { \
>> + .offset = o, \
>> + .val = v, \
>> + }
>> +
>> +static const struct phy_init_tbl ipq5332_usb_uniphy_init_tbl[] = {
>> + PHY_INIT_CFG(PCIE_USB_COMBO_PHY_CFG_RX_AFE_2, 0x1076),
>> + PHY_INIT_CFG(PCIE_USB_COMBO_PHY_CFG_RX_DLF_DEMUX_2, 0x3142),
>> + PHY_INIT_CFG(PCIE_USB_COMBO_PHY_CFG_MISC1, 0x3),
>> +};
>
> We already have this issue in QMP drivers. Could you please move data
> definitions to come after all struct definitions?
Sure, will move this definitions.
>
>> +
>> +struct uniphy_cfg {
>> + const struct phy_init_tbl *init_seq;
>> + int num_init_seq;
>> +};
>> +
>> +struct uniphy_usb {
>> + struct device *dev;
>> + const struct uniphy_cfg *cfg;
>> + struct phy *phy;
>> + void __iomem *base;
>> + struct clk_bulk_data *clks;
>> + unsigned int num_clks;
>> + struct reset_control *reset;
>> + struct regulator *vreg;
>> + struct clk_fixed_rate pipe_clk_fixed;
>> + struct regmap *tcsr;
>> + unsigned int usb_mux_offset;
>> +};
>> +
>> +static const struct uniphy_cfg ipq5332_usb_uniphy_cfg = {
>> + .init_seq = ipq5332_usb_uniphy_init_tbl,
>> + .num_init_seq = ARRAY_SIZE(ipq5332_usb_uniphy_init_tbl),
>> +};
>> +
>> +static int uniphy_usb_mux_enable(struct uniphy_usb *uniphy, bool
>> enable)
>> +{
>> + struct device *dev = uniphy->dev;
>> + unsigned int val;
>> + int ret;
>> +
>> + if (!uniphy->tcsr)
>> + return -EINVAL;
>> +
>> + ret = regmap_read(uniphy->tcsr, uniphy->usb_mux_offset, &val);
>> + if (ret) {
>> + dev_err(dev, "Mux read failed: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + val = enable ? (val | TCSR_USB_MUX_SEL) : (val &
>> ~TCSR_USB_MUX_SEL);
>> + ret = regmap_write(uniphy->tcsr, uniphy->usb_mux_offset, val);
>
> regmap_update_bits()
Will update.
>
>> + if (ret) {
>> + dev_err(dev, "Mux write failed: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int uniphy_usb_init(struct phy *phy)
>> +{
>> + struct uniphy_usb *uniphy = phy_get_drvdata(phy);
>> + const struct uniphy_cfg *cfg = uniphy->cfg;
>> + const struct phy_init_tbl *tbl = cfg->init_seq;
>> + void __iomem *base = uniphy->base;
>> + struct device *dev = uniphy->dev;
>> + int i, ret;
>> +
>> + ret = regulator_enable(uniphy->vreg);
>> + if (ret) {
>> + dev_err(dev, "failed to enable regulator, %d\n", ret);
>> + return ret;
>> + }
>> +
>> + /* Perform phy reset */
>> + reset_control_assert(uniphy->reset);
>> + usleep_range(1, 5);
>> + reset_control_deassert(uniphy->reset);
>
> Error checkig, please.
Sure, will add.
>
>> +
>> + ret = uniphy_usb_mux_enable(uniphy, true);
>> + if (ret < 0)
>> + goto err_assert_reset;
>> +
>> + ret = clk_bulk_prepare_enable(uniphy->num_clks, uniphy->clks);
>> + if (ret) {
>> + dev_err(dev, "failed to enable clocks: %d\n", ret);
>> + goto err_assert_reset;
>> + }
>> +
>> + /* phy autoload delay */
>> + usleep_range(35, 40);
>> +
>> + for (i = 0; i < cfg->num_init_seq; i++)
>> + writel(tbl[i].val, base + tbl[i].offset);
>> +
>> + return 0;
>> +
>> +err_assert_reset:
>> + /* Assert phy reset */
>> + reset_control_assert(uniphy->reset);
>> +
>> + return ret;
>> +}
>> +
>> +static int uniphy_usb_shutdown(struct phy *phy)
>> +{
>> + struct uniphy_usb *uniphy = phy_get_drvdata(phy);
>> +
>> + clk_bulk_disable_unprepare(uniphy->num_clks, uniphy->clks);
>> +
>> + uniphy_usb_mux_enable(uniphy, false);
>> +
>> + /* Assert phy reset */
>> + reset_control_assert(uniphy->reset);
>> +
>> + regulator_disable(uniphy->vreg);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct phy_ops uniphy_usb_ops = {
>> + .power_on = uniphy_usb_init,
>> + .power_off = uniphy_usb_shutdown,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int qcom_uniphy_usb_mux_init(struct uniphy_usb *uniphy)
>
> Inline this function please.
Will do.
>
>> +{
>> + struct device *dev = uniphy->dev;
>> + int ret;
>> +
>> + uniphy->tcsr =
>> syscon_regmap_lookup_by_phandle_args(dev->of_node,
>> "qcom,phy-usb-mux-sel",
>> + 1, &uniphy->usb_mux_offset);
>> + if (IS_ERR(uniphy->tcsr)) {
>> + ret = PTR_ERR(uniphy->tcsr);
>> + uniphy->tcsr = NULL;
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_uniphy_usb_clk_init(struct uniphy_usb *uniphy)
>
> Inline
Will do.
>
>> +{
>> + struct device *dev = uniphy->dev;
>> + int ret;
>> +
>> + ret = devm_clk_bulk_get_all(dev, &uniphy->clks);
>> + if (ret < 0)
>> + return ret;
>> +
>> + uniphy->num_clks = ret;
>> +
>> + return 0;
>> +}
>> +
>> +static void phy_clk_release_provider(void *res)
>> +{
>> + of_clk_del_provider(res);
>> +}
>> +
>> +/*
>> + * Register a fixed rate pipe clock.
>> + *
>> + * The <s>_pipe_clksrc generated by PHY goes to the GCC that gate
>> + * controls it. The <s>_pipe_clk coming out of the GCC is requested
>> + * by the PHY driver for its operations.
>> + * We register the <s>_pipe_clksrc here. The gcc driver takes care
>> + * of assigning this <s>_pipe_clksrc as parent to <s>_pipe_clk.
>> + * Below picture shows this relationship.
>> + *
>> + * +---------------+
>> + * | PHY block |<<---------------------------------------+
>> + * | | |
>> + * | +-------+ | +-----+ |
>> + * I/P---^-->| PLL |---^--->pipe_clksrc--->| GCC |--->pipe_clk---+
>> + * clk | +-------+ | +-----+
>> + * +---------------+
>> + */
>> +static int phy_pipe_clk_register(struct uniphy_usb *uniphy, struct
>> device_node *np)
>> +{
>> + struct clk_fixed_rate *fixed = &uniphy->pipe_clk_fixed;
>> + struct device *dev = uniphy->dev;
>> + struct clk_init_data init = { };
>> + int ret;
>> +
>> + ret = of_property_read_string(np, "clock-output-names",
>> &init.name);
>> + if (ret) {
>> + dev_err(dev, "%pOFn: No clock-output-names\n", np);
>> + return ret;
>> + }
>> +
>> + init.ops = &clk_fixed_rate_ops;
>> +
>> + fixed->fixed_rate = 250000000;
>> + fixed->hw.init = &init;
>> +
>> + ret = devm_clk_hw_register(dev, &fixed->hw);
>
> devm_clk_hw_register_fixed_rate()
Will change.
>
>> + if (ret)
>> + return ret;
>> +
>> + ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
>> + if (ret)
>> + return ret;
>> +
>> + return devm_add_action_or_reset(dev, phy_clk_release_provider, np);
>
> devm_of_clk_add_hw_provider().
Will change.
>
>> +}
>> +
>> +static int qcom_uniphy_usb_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct phy_provider *phy_provider;
>> + struct uniphy_usb *uniphy;
>> + struct device_node *np;
>> + int ret;
>> +
>> + uniphy = devm_kzalloc(dev, sizeof(*uniphy), GFP_KERNEL);
>> + if (!uniphy)
>> + return -ENOMEM;
>> +
>> + uniphy->dev = dev;
>> +
>> + uniphy->cfg = of_device_get_match_data(dev);
>> + if (!uniphy->cfg)
>> + return -EINVAL;
>> +
>> + uniphy->base = devm_platform_ioremap_resource(pdev, 0);
>> + if (IS_ERR(uniphy->base))
>> + return PTR_ERR(uniphy->base);
>> +
>> + ret = qcom_uniphy_usb_clk_init(uniphy);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "failed to get clock\n");
>> +
>> + ret = qcom_uniphy_usb_mux_init(uniphy);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "failed to get USB mux\n");
>> +
>> + uniphy->reset = devm_reset_control_get_exclusive_by_index(dev, 0);
>> + if (IS_ERR(uniphy->reset))
>> + return dev_err_probe(dev, PTR_ERR(uniphy->reset), "failed to
>> get reset\n");
>> +
>> + uniphy->vreg = devm_regulator_get_exclusive(dev, "vdd");
>
> Why do you need the exclusive control here?
Will change it to devm_regulator_get().
>
>> + if (IS_ERR(uniphy->vreg))
>> + return dev_err_probe(dev, PTR_ERR(uniphy->phy), "failed to
>> get vreg\n");
>> +
>> + np = of_node_get(dev->of_node);
>
> No need to get/put it, just use dev->of_node directly.
Sure, will change it.
>
>> + ret = phy_pipe_clk_register(uniphy, np);
>> + if (ret) {
>> + dev_err_probe(dev, ret, "failed to register pipe clk\n");
>> + goto err;
>> + }
>> +
>> + uniphy->phy = devm_phy_create(dev, NULL, &uniphy_usb_ops);
>> + if (IS_ERR(uniphy->phy)) {
>> + ret = PTR_ERR(uniphy->phy);
>> + dev_err_probe(dev, ret, "failed to create PHY\n");
>> + goto err;
>> + }
>> +
>> + phy_set_drvdata(uniphy->phy, uniphy);
>> +
>> + phy_provider = devm_of_phy_provider_register(dev,
>> of_phy_simple_xlate);
>> +
>> + ret = PTR_ERR_OR_ZERO(phy_provider);
>> +
>> +err:
>> + of_node_put(np);
>> + return ret;
>> +}
>> +
>> +static const struct of_device_id qcom_uniphy_usb_of_match[] = {
>> + { .compatible = "qcom,ipq5332-usb-uniphy", .data =
>> &ipq5332_usb_uniphy_cfg},
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_uniphy_usb_of_match);
>> +
>> +static struct platform_driver qcom_uniphy_usb_driver = {
>> + .probe = qcom_uniphy_usb_probe,
>> + .driver = {
>> + .of_match_table = qcom_uniphy_usb_of_match,
>> + .name = "qcom,uniphy-usb",
>> + }
>> +};
>> +module_platform_driver(qcom_uniphy_usb_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm Super-Speed USB UNIPHY driver");
>> +MODULE_LICENSE("GPL");
>
--
Thanks,
Praveenkumar
Powered by blists - more mailing lists