| 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: <20250424232321.xyyjetk3bocew4yn@synopsys.com>
Date: Thu, 24 Apr 2025 23:23:24 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Bjorn Andersson <bjorn.andersson@....qualcomm.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: qcom: Use bulk clock API and devres
On Tue, Apr 22, 2025, Bjorn Andersson wrote:
> The Qualcomm DWC3 glue driver duplicates the logic of the bulk clock
> API to acquire, prepare, and unprepare the controller's clocks. It also
> manages the life cycle of these handled explicitly.
>
> Transition to the bulk clock API and manage the resources using devres,
> to clean up the code.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@....qualcomm.com>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 84 ++++++--------------------------------------
> 1 file changed, 11 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index d512002e1e88da9523ff82454e653bac55c1409d..ec6e197cfc32abf5ab8f9b23ba590bd1a126b500 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -73,7 +73,7 @@ struct dwc3_qcom {
> struct device *dev;
> void __iomem *qscratch_base;
> struct dwc3 dwc;
> - struct clk **clks;
> + struct clk_bulk_data *clks;
> int num_clocks;
> struct reset_control *resets;
> struct dwc3_qcom_port ports[DWC3_QCOM_MAX_PORTS];
> @@ -431,9 +431,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> dev_err(qcom->dev, "port-%d HS-PHY not in L2\n", i + 1);
> }
> -
> - for (i = qcom->num_clocks - 1; i >= 0; i--)
> - clk_disable_unprepare(qcom->clks[i]);
> + clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>
> ret = dwc3_qcom_interconnect_disable(qcom);
> if (ret)
> @@ -465,14 +463,9 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
> if (dwc3_qcom_is_host(qcom) && wakeup)
> dwc3_qcom_disable_interrupts(qcom);
>
> - for (i = 0; i < qcom->num_clocks; i++) {
> - ret = clk_prepare_enable(qcom->clks[i]);
> - if (ret < 0) {
> - while (--i >= 0)
> - clk_disable_unprepare(qcom->clks[i]);
> - return ret;
> - }
> - }
> + ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
> + if (ret < 0)
> + return ret;
>
> ret = dwc3_qcom_interconnect_enable(qcom);
> if (ret)
> @@ -648,62 +641,14 @@ static int dwc3_qcom_setup_irq(struct dwc3_qcom *qcom, struct platform_device *p
> return 0;
> }
>
> -static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
> -{
> - struct device *dev = qcom->dev;
> - struct device_node *np = dev->of_node;
> - int i;
> -
> - if (!np || !count)
> - return 0;
> -
> - if (count < 0)
> - return count;
> -
> - qcom->num_clocks = count;
> -
> - qcom->clks = devm_kcalloc(dev, qcom->num_clocks,
> - sizeof(struct clk *), GFP_KERNEL);
> - if (!qcom->clks)
> - return -ENOMEM;
> -
> - for (i = 0; i < qcom->num_clocks; i++) {
> - struct clk *clk;
> - int ret;
> -
> - clk = of_clk_get(np, i);
> - if (IS_ERR(clk)) {
> - while (--i >= 0)
> - clk_put(qcom->clks[i]);
> - return PTR_ERR(clk);
> - }
> -
> - ret = clk_prepare_enable(clk);
> - if (ret < 0) {
> - while (--i >= 0) {
> - clk_disable_unprepare(qcom->clks[i]);
> - clk_put(qcom->clks[i]);
> - }
> - clk_put(clk);
> -
> - return ret;
> - }
> -
> - qcom->clks[i] = clk;
> - }
> -
> - return 0;
> -}
> -
> static int dwc3_qcom_probe(struct platform_device *pdev)
> {
> struct dwc3_probe_data probe_data = {};
> - struct device_node *np = pdev->dev.of_node;
> struct device *dev = &pdev->dev;
> struct dwc3_qcom *qcom;
> struct resource res;
> struct resource *r;
> - int ret, i;
> + int ret;
> bool ignore_pipe_clk;
> bool wakeup_source;
>
> @@ -733,11 +678,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> goto reset_assert;
> }
>
> - ret = dwc3_qcom_clk_init(qcom, of_clk_get_parent_count(np));
> - if (ret) {
> + ret = devm_clk_bulk_get_all(&pdev->dev, &qcom->clks);
Did you mean to use devm_clk_bulk_get_all_enabled() or did you mean to
prepare and enable them separately? I don't see that done in probe.
BR,
Thinh
> + if (ret < 0) {
> dev_err_probe(dev, ret, "failed to get clocks\n");
> goto reset_assert;
> }
> + qcom->num_clocks = ret;
>
> r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!r)
> @@ -804,10 +750,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> remove_core:
> dwc3_core_remove(&qcom->dwc);
> clk_disable:
> - for (i = qcom->num_clocks - 1; i >= 0; i--) {
> - clk_disable_unprepare(qcom->clks[i]);
> - clk_put(qcom->clks[i]);
> - }
> + clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
> reset_assert:
> reset_control_assert(qcom->resets);
>
> @@ -818,15 +761,10 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
> {
> struct dwc3 *dwc = platform_get_drvdata(pdev);
> struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
> - int i;
>
> dwc3_core_remove(&qcom->dwc);
>
> - for (i = qcom->num_clocks - 1; i >= 0; i--) {
> - clk_disable_unprepare(qcom->clks[i]);
> - clk_put(qcom->clks[i]);
> - }
> - qcom->num_clocks = 0;
> + clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>
> dwc3_qcom_interconnect_exit(qcom);
> reset_control_assert(qcom->resets);
>
> ---
> base-commit: 2c9c612abeb38aab0e87d48496de6fd6daafb00b
> change-id: 20250422-dwc3-clk-bulk-960a91ed4ab8
>
> Best regards,
> --
> Bjorn Andersson <bjorn.andersson@....qualcomm.com>
>
Powered by blists - more mailing lists