Messages in this thread | | | Date | Thu, 31 Aug 2023 17:35:48 +0530 | Subject | Re: [PATCH 3/9] phy: qcom: uniphy: Update UNIPHY driver to be a common driver | From | Praveenkumar I <> |
| |
On 8/29/2023 8:07 PM, Dmitry Baryshkov wrote: > On Tue, 29 Aug 2023 at 17:00, Praveenkumar I <quic_ipkumar@quicinc.com> wrote: >> This patch updates the UNIPHY driver to be a common driver to >> accommodate all UNIPHY / Combo PHY. This driver can be used for >> both USB and PCIe UNIPHY. Using phy-mul-sel from DTS MUX selection >> for USB / PCIe can be acheived. > I'm not sure why you are talking about PCIe here. This patch adds only > SS PHY support. > > Also, I'd like to point out that we had this 'USB and PCIe and > everything else' design in the QMP driver. We had to split the driver > into individual pieces to make it manageable again. This Uniphy 22ull is a combo PHY used between USB GEN3 or PCIe GEN3. Via mux selection the one of the controller interface can use it. Hence thought to have a same PHY driver for both USB3 and PCIe. Will drop the plan of adding the PCIe support. > >> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com> >> --- >> drivers/phy/qualcomm/phy-qcom-uniphy.c | 401 +++++++++++++++++++++---- >> 1 file changed, 335 insertions(+), 66 deletions(-) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-uniphy.c b/drivers/phy/qualcomm/phy-qcom-uniphy.c >> index da6f290af722..eb71588f5417 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-uniphy.c >> +++ b/drivers/phy/qualcomm/phy-qcom-uniphy.c >> @@ -5,141 +5,410 @@ >> * Based on code from >> * Allwinner Technology Co., Ltd. <www.allwinnertech.com> >> * >> + * Modified the driver to be common for Qualcomm UNIPHYs >> + * Copyright (c) 2023, The Linux Foundation. All rights reserved. > I'd not call this 'modified', but rather 'rewritten from scratch. Sure, will correct. > >> */ >> >> +#include <linux/bitfield.h> >> +#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/mutex.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> >> >> -struct ipq4019_usb_phy { >> +struct uniphy_init_tbl { >> + unsigned int offset; >> + unsigned int val; >> +}; > unused > >> + >> +#define UNIPHY_INIT_CFG(o, v) \ >> + { \ >> + .offset = o, \ >> + .val = v, \ >> + } > unused > >> + >> +struct uniphy_cfg { >> + const struct uniphy_init_tbl *init_seq; >> + int num_init_seq; >> + const char * const *clk_list; >> + int num_clks; >> + const char * const *reset_list; >> + int num_resets; >> + const char * const *vreg_list; >> + int num_vregs; >> + unsigned int pipe_clk_rate; >> + unsigned int reset_udelay; >> + unsigned int autoload_udelay; >> +}; >> + >> +struct qcom_uniphy { >> struct device *dev; >> + const struct uniphy_cfg *cfg; >> struct phy *phy; >> void __iomem *base; >> - struct reset_control *por_rst; >> - struct reset_control *srif_rst; >> + struct clk_bulk_data *clks; >> + struct reset_control_bulk_data *resets; >> + struct regulator_bulk_data *vregs; >> + struct clk_fixed_rate pipe_clk_fixed; >> +}; >> + >> +static const char * const ipq4019_ssphy_reset_l[] = { >> + "por_rst", >> +}; >> + >> +static const struct uniphy_cfg ipq4019_usb_ssphy_cfg = { >> + .reset_list = ipq4019_ssphy_reset_l, >> + .num_resets = ARRAY_SIZE(ipq4019_ssphy_reset_l), >> + .reset_udelay = 10000, >> + >> }; >> >> -static int ipq4019_ss_phy_power_off(struct phy *_phy) >> +static const char * const ipq4019_hsphy_reset_l[] = { >> + "por_rst", "srif_rst", >> +}; > + >> +static const struct uniphy_cfg ipq4019_usb_hsphy_cfg = { >> + .reset_list = ipq4019_hsphy_reset_l, >> + .num_resets = ARRAY_SIZE(ipq4019_hsphy_reset_l), >> + .reset_udelay = 10000, >> +}; >> + >> +static int phy_mux_sel(struct phy *phy) >> +{ >> + struct qcom_uniphy *uniphy = phy_get_drvdata(phy); >> + struct device *dev = uniphy->dev; >> + struct regmap *tcsr; >> + unsigned int args[2]; >> + int ret; >> + >> + tcsr = syscon_regmap_lookup_by_phandle_args(dev->of_node, "qcom,phy-mux-sel", >> + ARRAY_SIZE(args), args); > No. mux data should come from match_data rather than polluting DT with it. > >> + if (IS_ERR(tcsr)) { >> + ret = PTR_ERR(tcsr); >> + if (ret == -ENOENT) >> + return 0; >> + >> + dev_err(dev, "failed to lookup syscon for phy mux %d\n", ret); >> + return ret; >> + } >> + >> + /* PHY MUX registers only have this BIT0 */ > huh? > >> + ret = regmap_write(tcsr, args[0], args[1]); >> + if (ret < 0) { >> + dev_err(dev, "PHY Mux selection failed: %d\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int uniphy_enable(struct phy *phy) >> { >> - struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy); >> + struct qcom_uniphy *uniphy = phy_get_drvdata(phy); >> + const struct uniphy_cfg *cfg = uniphy->cfg; >> + const struct uniphy_init_tbl *tbl; >> + void __iomem *base = uniphy->base; >> + int i, ret; >> >> - reset_control_assert(phy->por_rst); >> - msleep(10); >> + ret = regulator_bulk_enable(cfg->num_vregs, uniphy->vregs); >> + if (ret) { >> + dev_err(uniphy->dev, "failed to enable regulators: %d\n", ret); >> + return ret; >> + } >> + >> + /* Assert all available resets */ >> + for (i = 0; i < cfg->num_resets; i++) { >> + ret = reset_control_assert(uniphy->resets[i].rstc); >> + if (ret) { >> + dev_err(uniphy->dev, "reset assert failed: %d\n", ret); >> + goto err_assert_reset; >> + } >> + if (cfg->reset_udelay) >> + usleep_range(cfg->reset_udelay, cfg->reset_udelay + 10); >> + } > There is a whole reset_control_bulk_*() set of API. Please use it > instead of hardcoding reset cycles. > >> + >> + /* Deassert all available resets */ >> + for (i = 0; i < cfg->num_resets; i++) { >> + ret = reset_control_deassert(uniphy->resets[i].rstc); >> + if (ret) { >> + dev_err(uniphy->dev, "reset deassert failed: %d\n", ret); >> + goto err_assert_reset; >> + } >> + if (cfg->reset_udelay) >> + usleep_range(cfg->reset_udelay, cfg->reset_udelay + 10); >> + } >> + >> + ret = phy_mux_sel(phy); >> + if (ret < 0) >> + goto err_assert_reset; >> + >> + ret = clk_bulk_prepare_enable(cfg->num_clks, uniphy->clks); >> + if (ret) { >> + dev_err(uniphy->dev, "failed to enable clocks: %d\n", ret); >> + goto err_assert_reset; >> + } >> + >> + if (cfg->autoload_udelay) >> + usleep_range(cfg->autoload_udelay, cfg->autoload_udelay + 10); >> + >> + if (cfg->num_init_seq) { >> + tbl = cfg->init_seq; >> + for (i = 0; i < cfg->num_init_seq; i++, tbl++) >> + writel(tbl->val, base + tbl->offset); >> + } > unused > >> return 0; >> + >> +err_assert_reset: >> + /* Assert all available resets */ >> + for (i = 0; i < cfg->num_resets; i++) { >> + reset_control_assert(uniphy->resets[i].rstc); >> + if (cfg->reset_udelay) >> + usleep_range(cfg->reset_udelay, cfg->reset_udelay + 10); >> + } >> + >> + return ret; >> } >> >> -static int ipq4019_ss_phy_power_on(struct phy *_phy) >> +static int uniphy_disable(struct phy *phy) >> { >> - struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy); >> + struct qcom_uniphy *uniphy = phy_get_drvdata(phy); >> + const struct uniphy_cfg *cfg = uniphy->cfg; >> + int i; >> >> - ipq4019_ss_phy_power_off(_phy); >> + /* Assert all available resets */ >> + for (i = 0; i < cfg->num_resets; i++) { >> + reset_control_assert(uniphy->resets[i].rstc); >> + if (cfg->reset_udelay) >> + usleep_range(cfg->reset_udelay, cfg->reset_udelay + 10); >> + } >> >> - reset_control_deassert(phy->por_rst); >> + clk_bulk_disable_unprepare(cfg->num_clks, uniphy->clks); >> + >> + regulator_bulk_disable(cfg->num_vregs, uniphy->vregs); >> >> return 0; >> } >> >> -static const struct phy_ops ipq4019_usb_ss_phy_ops = { >> - .power_on = ipq4019_ss_phy_power_on, >> - .power_off = ipq4019_ss_phy_power_off, >> +static const struct phy_ops uniphy_phy_ops = { >> + .power_on = uniphy_enable, >> + .power_off = uniphy_disable, > Using _enable / _disable for power_on() and power_off() isn't logical. > >> + .owner = THIS_MODULE, >> }; >> >> -static int ipq4019_hs_phy_power_off(struct phy *_phy) >> +static int qcom_uniphy_vreg_init(struct qcom_uniphy *uniphy) >> +{ >> + const struct uniphy_cfg *cfg = uniphy->cfg; >> + struct device *dev = uniphy->dev; >> + int i, ret; >> + >> + uniphy->vregs = devm_kcalloc(dev, cfg->num_vregs, >> + sizeof(*uniphy->vregs), GFP_KERNEL); > You know the maximum amount of regulators. Can you use an array > instead of allocating data? > >> + if (!uniphy->vregs) >> + return -ENOMEM; >> + >> + for (i = 0; i < cfg->num_vregs; i++) >> + uniphy->vregs[i].supply = cfg->vreg_list[i]; >> + >> + ret = devm_regulator_bulk_get(dev, cfg->num_vregs, uniphy->vregs); >> + > Drop empty lines between ret assignment and check. > >> + if (ret) >> + return dev_err_probe(dev, ret, "failed to get regulators\n"); >> + >> + return 0; >> +} >> + >> +static int qcom_uniphy_reset_init(struct qcom_uniphy *uniphy) >> { >> - struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy); >> + const struct uniphy_cfg *cfg = uniphy->cfg; >> + struct device *dev = uniphy->dev; >> + int i, ret; >> + >> + uniphy->resets = devm_kcalloc(dev, cfg->num_resets, >> + sizeof(*uniphy->resets), GFP_KERNEL); > Same here, can you use an array? > >> + if (!uniphy->resets) >> + return -ENOMEM; >> >> - reset_control_assert(phy->por_rst); >> - msleep(10); >> + for (i = 0; i < cfg->num_resets; i++) >> + uniphy->resets[i].id = cfg->reset_list[i]; > Declare common resets list and use > devm_reset_control_bulk_get_optional_exclusive(). > >> - reset_control_assert(phy->srif_rst); >> - msleep(10); >> + ret = devm_reset_control_bulk_get_exclusive(dev, cfg->num_resets, uniphy->resets); >> + if (ret) >> + return dev_err_probe(dev, ret, "failed to get resets\n"); >> >> return 0; >> } >> >> -static int ipq4019_hs_phy_power_on(struct phy *_phy) >> +static int qcom_uniphy_clk_init(struct qcom_uniphy *uniphy) > I don't see clocks actually being used. Please do not introduce unused features. > >> { >> - struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy); >> + const struct uniphy_cfg *cfg = uniphy->cfg; >> + struct device *dev = uniphy->dev; >> + int i, ret; >> >> - ipq4019_hs_phy_power_off(_phy); >> >> - reset_control_deassert(phy->srif_rst); >> - msleep(10); >> + uniphy->clks = devm_kcalloc(dev, cfg->num_clks, >> + sizeof(*uniphy->clks), GFP_KERNEL); >> + if (!uniphy->clks) >> + return -ENOMEM; >> + >> + for (i = 0; i < cfg->num_clks; i++) >> + uniphy->clks[i].id = cfg->clk_list[i]; >> >> - reset_control_deassert(phy->por_rst); >> + ret = devm_clk_bulk_get(dev, cfg->num_clks, uniphy->clks); >> + if (ret) >> + return dev_err_probe(dev, ret, "failed to get clocks\n"); >> >> return 0; >> } >> >> -static const struct phy_ops ipq4019_usb_hs_phy_ops = { >> - .power_on = ipq4019_hs_phy_power_on, >> - .power_off = ipq4019_hs_phy_power_off, >> -}; >> +static void phy_clk_release_provider(void *res) >> +{ >> + of_clk_del_provider(res); >> +} >> >> -static const struct of_device_id ipq4019_usb_phy_of_match[] = { >> - { .compatible = "qcom,usb-hs-ipq4019-phy", .data = &ipq4019_usb_hs_phy_ops}, >> - { .compatible = "qcom,usb-ss-ipq4019-phy", .data = &ipq4019_usb_ss_phy_ops}, >> - { }, >> -}; >> -MODULE_DEVICE_TABLE(of, ipq4019_usb_phy_of_match); >> +/* >> + * 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 qcom_uniphy *uniphy, struct device_node *np) >> +{ >> + struct clk_fixed_rate *fixed = &uniphy->pipe_clk_fixed; >> + const struct uniphy_cfg *cfg = uniphy->cfg; >> + 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 = cfg->pipe_clk_rate; >> + fixed->hw.init = &init; >> >> -static int ipq4019_usb_phy_probe(struct platform_device *pdev) >> + ret = devm_clk_hw_register(dev, &fixed->hw); >> + if (ret) >> + return ret; >> + >> + ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw); >> + if (ret) >> + return ret; > When you c&p something, please take care to understand the code you are copying. > Unlike QMP drivers you can (and should) use devm_of_clk_add_hw_provider() here. > > Not to mention that pipe clocks are not in this patch. > >> + >> + /* >> + * Roll a devm action because the clock provider is the child node, but >> + * the child node is not actually a device. >> + */ >> + return devm_add_action_or_reset(dev, phy_clk_release_provider, np); >> +} >> + >> +static int qcom_uniphy_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct phy_provider *phy_provider; >> - struct ipq4019_usb_phy *phy; >> + struct qcom_uniphy *uniphy; >> + struct device_node *np; >> + int ret; >> >> - phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL); >> - if (!phy) >> + uniphy = devm_kzalloc(dev, sizeof(*uniphy), GFP_KERNEL); >> + if (!uniphy) >> return -ENOMEM; >> >> - phy->dev = &pdev->dev; >> - phy->base = devm_platform_ioremap_resource(pdev, 0); >> - if (IS_ERR(phy->base)) { >> - dev_err(dev, "failed to remap register memory\n"); >> - return PTR_ERR(phy->base); >> - } >> + uniphy->dev = dev; >> >> - phy->por_rst = devm_reset_control_get(phy->dev, "por_rst"); >> - if (IS_ERR(phy->por_rst)) { >> - if (PTR_ERR(phy->por_rst) != -EPROBE_DEFER) >> - dev_err(dev, "POR reset is missing\n"); >> - return PTR_ERR(phy->por_rst); >> + 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)) { >> + ret = PTR_ERR(uniphy->base); >> + dev_err_probe(dev, ret, "failed to remap register memory\n"); >> + return ret; >> } >> >> - phy->srif_rst = devm_reset_control_get_optional(phy->dev, "srif_rst"); >> - if (IS_ERR(phy->srif_rst)) >> - return PTR_ERR(phy->srif_rst); >> + ret = qcom_uniphy_clk_init(uniphy); >> + if (ret) >> + return ret; >> + >> + ret = qcom_uniphy_reset_init(uniphy); >> + if (ret) >> + return ret; >> + >> + ret = qcom_uniphy_vreg_init(uniphy); >> + if (ret < 0) >> + return ret; >> + >> + if (uniphy->cfg->pipe_clk_rate) { >> + np = of_node_get(dev->of_node); > What for? Do you think that the driver can outlive struct device? > >> + ret = phy_pipe_clk_register(uniphy, np); >> + if (ret) { >> + dev_err_probe(dev, ret, "failed to register pipe clk\n"); >> + goto err; >> + } >> + } >> >> - phy->phy = devm_phy_create(dev, NULL, of_device_get_match_data(dev)); >> - if (IS_ERR(phy->phy)) { >> - dev_err(dev, "failed to create PHY\n"); >> - return PTR_ERR(phy->phy); >> + uniphy->phy = devm_phy_create(dev, NULL, &uniphy_phy_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(phy->phy, phy); >> + >> + phy_set_drvdata(uniphy->phy, uniphy); >> >> phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); >> >> - return PTR_ERR_OR_ZERO(phy_provider); >> + ret = PTR_ERR_OR_ZERO(phy_provider); >> + >> +err: >> + if (uniphy->cfg->pipe_clk_rate) >> + of_node_put(np); >> + return ret; >> } >> >> -static struct platform_driver ipq4019_usb_phy_driver = { >> - .probe = ipq4019_usb_phy_probe, >> +static const struct of_device_id qcom_uniphy_of_match[] = { >> + { .compatible = "qcom,usb-hs-ipq4019-phy", .data = &ipq4019_usb_hsphy_cfg}, >> + { .compatible = "qcom,usb-ss-ipq4019-phy", .data = &ipq4019_usb_ssphy_cfg}, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, qcom_uniphy_of_match); >> + >> +static struct platform_driver qcom_uniphy_driver = { >> + .probe = qcom_uniphy_probe, >> .driver = { >> - .of_match_table = ipq4019_usb_phy_of_match, >> - .name = "ipq4019-usb-phy", >> + .of_match_table = qcom_uniphy_of_match, >> + .name = "qcom-uniphy", >> } >> }; >> -module_platform_driver(ipq4019_usb_phy_driver); >> +module_platform_driver(qcom_uniphy_driver); >> >> -MODULE_DESCRIPTION("QCOM/IPQ4019 USB phy driver"); >> +MODULE_DESCRIPTION("QCOM uniphy driver"); >> MODULE_AUTHOR("John Crispin <john@phrozen.org>"); >> MODULE_LICENSE("GPL v2"); > General comment: please consider dropping this beast and starting from > scratch, adding only really necessary bits to the existing ipq4019 USB > PHY driver. Rewritten the entire driver considering the IPQ5332 as well and because of it there are unused code which are utilized in patch [8/9]. I should have added only the IPQ4019 support alone in this patch and then incrementally update it for IPQ5332. Will follow that here onwards. Will address all the review comments.
- Praveenkumar
| |