Currently, the driver acquires clocks and prepare/enable/disable/unprepare
the clocks individually thereby making the driver complex to read.
The driver can be simplified by using the clk_bulk*() APIs.
Use:
- devm_clk_bulk_get_all() API to acquire all the clocks
- clk_bulk_prepare_enable() to prepare/enable clocks
- clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
drivers/pci/controller/dwc/pcie-histb.c | 70 ++++---------------------
1 file changed, 11 insertions(+), 59 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
index a52071589377..4022349e85d2 100644
--- a/drivers/pci/controller/dwc/pcie-histb.c
+++ b/drivers/pci/controller/dwc/pcie-histb.c
@@ -51,10 +51,8 @@
struct histb_pcie {
struct dw_pcie *pci;
- struct clk *aux_clk;
- struct clk *pipe_clk;
- struct clk *sys_clk;
- struct clk *bus_clk;
+ struct clk_bulk_data *clks;
+ int num_clks;
struct phy *phy;
struct reset_control *soft_reset;
struct reset_control *sys_reset;
@@ -204,10 +202,7 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
reset_control_assert(hipcie->sys_reset);
reset_control_assert(hipcie->bus_reset);
- clk_disable_unprepare(hipcie->aux_clk);
- clk_disable_unprepare(hipcie->pipe_clk);
- clk_disable_unprepare(hipcie->sys_clk);
- clk_disable_unprepare(hipcie->bus_clk);
+ clk_bulk_disable_unprepare(hipcie->num_clks, hipcie->clks);
if (hipcie->reset_gpio)
gpiod_set_value_cansleep(hipcie->reset_gpio, 1);
@@ -235,28 +230,10 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
if (hipcie->reset_gpio)
gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
- ret = clk_prepare_enable(hipcie->bus_clk);
+ ret = clk_bulk_prepare_enable(hipcie->num_clks, hipcie->clks);
if (ret) {
- dev_err(dev, "cannot prepare/enable bus clk\n");
- goto err_bus_clk;
- }
-
- ret = clk_prepare_enable(hipcie->sys_clk);
- if (ret) {
- dev_err(dev, "cannot prepare/enable sys clk\n");
- goto err_sys_clk;
- }
-
- ret = clk_prepare_enable(hipcie->pipe_clk);
- if (ret) {
- dev_err(dev, "cannot prepare/enable pipe clk\n");
- goto err_pipe_clk;
- }
-
- ret = clk_prepare_enable(hipcie->aux_clk);
- if (ret) {
- dev_err(dev, "cannot prepare/enable aux clk\n");
- goto err_aux_clk;
+ ret = dev_err_probe(dev, ret, "failed to enable clocks\n");
+ goto reg_dis;
}
reset_control_assert(hipcie->soft_reset);
@@ -270,13 +247,7 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
return 0;
-err_aux_clk:
- clk_disable_unprepare(hipcie->pipe_clk);
-err_pipe_clk:
- clk_disable_unprepare(hipcie->sys_clk);
-err_sys_clk:
- clk_disable_unprepare(hipcie->bus_clk);
-err_bus_clk:
+reg_dis:
if (hipcie->vpcie)
regulator_disable(hipcie->vpcie);
@@ -345,29 +316,10 @@ static int histb_pcie_probe(struct platform_device *pdev)
return ret;
}
- hipcie->aux_clk = devm_clk_get(dev, "aux");
- if (IS_ERR(hipcie->aux_clk)) {
- dev_err(dev, "Failed to get PCIe aux clk\n");
- return PTR_ERR(hipcie->aux_clk);
- }
-
- hipcie->pipe_clk = devm_clk_get(dev, "pipe");
- if (IS_ERR(hipcie->pipe_clk)) {
- dev_err(dev, "Failed to get PCIe pipe clk\n");
- return PTR_ERR(hipcie->pipe_clk);
- }
-
- hipcie->sys_clk = devm_clk_get(dev, "sys");
- if (IS_ERR(hipcie->sys_clk)) {
- dev_err(dev, "Failed to get PCIEe sys clk\n");
- return PTR_ERR(hipcie->sys_clk);
- }
-
- hipcie->bus_clk = devm_clk_get(dev, "bus");
- if (IS_ERR(hipcie->bus_clk)) {
- dev_err(dev, "Failed to get PCIe bus clk\n");
- return PTR_ERR(hipcie->bus_clk);
- }
+ hipcie->num_clks = devm_clk_bulk_get_all(dev, &hipcie->clks);
+ if (hipcie->num_clks < 0)
+ return dev_err_probe(dev, hipcie->num_clks,
+ "failed to get clocks\n");
hipcie->soft_reset = devm_reset_control_get(dev, "soft");
if (IS_ERR(hipcie->soft_reset)) {
--
2.50.1In subject, remove "dwc: " to follow historical convention. (See
"git log --oneline")
On Tue, Aug 26, 2025 at 05:12:40PM +0530, Anand Moon wrote:
> Currently, the driver acquires clocks and prepare/enable/disable/unprepare
> the clocks individually thereby making the driver complex to read.
>
> The driver can be simplified by using the clk_bulk*() APIs.
>
> Use:
> - devm_clk_bulk_get_all() API to acquire all the clocks
> - clk_bulk_prepare_enable() to prepare/enable clocks
> - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk
I assume this means the order in which we prepare/enable and
disable/unprepare will now depend on the order the clocks appear in
the device tree instead of the order in the code? If so, please
mention that here and verify that all upstream device trees have the
correct order.
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> drivers/pci/controller/dwc/pcie-histb.c | 70 ++++---------------------
> 1 file changed, 11 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
> index a52071589377..4022349e85d2 100644
> --- a/drivers/pci/controller/dwc/pcie-histb.c
> +++ b/drivers/pci/controller/dwc/pcie-histb.c
> @@ -51,10 +51,8 @@
>
> struct histb_pcie {
> struct dw_pcie *pci;
> - struct clk *aux_clk;
> - struct clk *pipe_clk;
> - struct clk *sys_clk;
> - struct clk *bus_clk;
> + struct clk_bulk_data *clks;
> + int num_clks;
> struct phy *phy;
> struct reset_control *soft_reset;
> struct reset_control *sys_reset;
> @@ -204,10 +202,7 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
> reset_control_assert(hipcie->sys_reset);
> reset_control_assert(hipcie->bus_reset);
>
> - clk_disable_unprepare(hipcie->aux_clk);
> - clk_disable_unprepare(hipcie->pipe_clk);
> - clk_disable_unprepare(hipcie->sys_clk);
> - clk_disable_unprepare(hipcie->bus_clk);
> + clk_bulk_disable_unprepare(hipcie->num_clks, hipcie->clks);
>
> if (hipcie->reset_gpio)
> gpiod_set_value_cansleep(hipcie->reset_gpio, 1);
> @@ -235,28 +230,10 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
> if (hipcie->reset_gpio)
> gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
>
> - ret = clk_prepare_enable(hipcie->bus_clk);
> + ret = clk_bulk_prepare_enable(hipcie->num_clks, hipcie->clks);
> if (ret) {
> - dev_err(dev, "cannot prepare/enable bus clk\n");
> - goto err_bus_clk;
> - }
> -
> - ret = clk_prepare_enable(hipcie->sys_clk);
> - if (ret) {
> - dev_err(dev, "cannot prepare/enable sys clk\n");
> - goto err_sys_clk;
> - }
> -
> - ret = clk_prepare_enable(hipcie->pipe_clk);
> - if (ret) {
> - dev_err(dev, "cannot prepare/enable pipe clk\n");
> - goto err_pipe_clk;
> - }
> -
> - ret = clk_prepare_enable(hipcie->aux_clk);
> - if (ret) {
> - dev_err(dev, "cannot prepare/enable aux clk\n");
> - goto err_aux_clk;
> + ret = dev_err_probe(dev, ret, "failed to enable clocks\n");
> + goto reg_dis;
> }
>
> reset_control_assert(hipcie->soft_reset);
> @@ -270,13 +247,7 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
>
> return 0;
>
> -err_aux_clk:
> - clk_disable_unprepare(hipcie->pipe_clk);
> -err_pipe_clk:
> - clk_disable_unprepare(hipcie->sys_clk);
> -err_sys_clk:
> - clk_disable_unprepare(hipcie->bus_clk);
> -err_bus_clk:
> +reg_dis:
> if (hipcie->vpcie)
> regulator_disable(hipcie->vpcie);
>
> @@ -345,29 +316,10 @@ static int histb_pcie_probe(struct platform_device *pdev)
> return ret;
> }
>
> - hipcie->aux_clk = devm_clk_get(dev, "aux");
> - if (IS_ERR(hipcie->aux_clk)) {
> - dev_err(dev, "Failed to get PCIe aux clk\n");
> - return PTR_ERR(hipcie->aux_clk);
> - }
> -
> - hipcie->pipe_clk = devm_clk_get(dev, "pipe");
> - if (IS_ERR(hipcie->pipe_clk)) {
> - dev_err(dev, "Failed to get PCIe pipe clk\n");
> - return PTR_ERR(hipcie->pipe_clk);
> - }
> -
> - hipcie->sys_clk = devm_clk_get(dev, "sys");
> - if (IS_ERR(hipcie->sys_clk)) {
> - dev_err(dev, "Failed to get PCIEe sys clk\n");
> - return PTR_ERR(hipcie->sys_clk);
> - }
> -
> - hipcie->bus_clk = devm_clk_get(dev, "bus");
> - if (IS_ERR(hipcie->bus_clk)) {
> - dev_err(dev, "Failed to get PCIe bus clk\n");
> - return PTR_ERR(hipcie->bus_clk);
> - }
> + hipcie->num_clks = devm_clk_bulk_get_all(dev, &hipcie->clks);
> + if (hipcie->num_clks < 0)
> + return dev_err_probe(dev, hipcie->num_clks,
> + "failed to get clocks\n");
>
> hipcie->soft_reset = devm_reset_control_get(dev, "soft");
> if (IS_ERR(hipcie->soft_reset)) {
> --
> 2.50.1
>
Hi Bjorn,
Thanks for your review comments.
On Tue, 26 Aug 2025 at 21:55, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> In subject, remove "dwc: " to follow historical convention. (See
> "git log --oneline")
>
Ok I will keep it as per the git history.
> On Tue, Aug 26, 2025 at 05:12:40PM +0530, Anand Moon wrote:
> > Currently, the driver acquires clocks and prepare/enable/disable/unprepare
> > the clocks individually thereby making the driver complex to read.
> >
> > The driver can be simplified by using the clk_bulk*() APIs.
> >
> > Use:
> > - devm_clk_bulk_get_all() API to acquire all the clocks
> > - clk_bulk_prepare_enable() to prepare/enable clocks
> > - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk
>
> I assume this means the order in which we prepare/enable and
> disable/unprepare will now depend on the order the clocks appear in
> the device tree instead of the order in the code? If so, please
> mention that here and verify that all upstream device trees have the
> correct order.
>
Following is the order in the device tree
clocks = <&crg HISTB_PCIE_AUX_CLK>,
<&crg HISTB_PCIE_PIPE_CLK>,
<&crg HISTB_PCIE_SYS_CLK>,
<&crg HISTB_PCIE_BUS_CLK>;
clock-names = "aux", "pipe", "sys", "bus";
Ok I will update this in the commit message.
Thanks
-Anand
On Tue, Aug 26, 2025 at 11:32:34PM +0530, Anand Moon wrote:
> Hi Bjorn,
>
> Thanks for your review comments.
> On Tue, 26 Aug 2025 at 21:55, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > In subject, remove "dwc: " to follow historical convention. (See
> > "git log --oneline")
> >
> Ok I will keep it as per the git history.
>
> > On Tue, Aug 26, 2025 at 05:12:40PM +0530, Anand Moon wrote:
> > > Currently, the driver acquires clocks and prepare/enable/disable/unprepare
> > > the clocks individually thereby making the driver complex to read.
> > >
> > > The driver can be simplified by using the clk_bulk*() APIs.
> > >
> > > Use:
> > > - devm_clk_bulk_get_all() API to acquire all the clocks
> > > - clk_bulk_prepare_enable() to prepare/enable clocks
> > > - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk
> >
> > I assume this means the order in which we prepare/enable and
> > disable/unprepare will now depend on the order the clocks appear in
> > the device tree instead of the order in the code? If so, please
> > mention that here and verify that all upstream device trees have the
> > correct order.
> >
> Following is the order in the device tree
>
> clocks = <&crg HISTB_PCIE_AUX_CLK>,
> <&crg HISTB_PCIE_PIPE_CLK>,
> <&crg HISTB_PCIE_SYS_CLK>,
> <&crg HISTB_PCIE_BUS_CLK>;
> clock-names = "aux", "pipe", "sys", "bus";
The current order in the code is:
histb_pcie_host_enable
clk_prepare_enable(hipcie->bus_clk);
clk_prepare_enable(hipcie->sys_clk);
clk_prepare_enable(hipcie->pipe_clk);
clk_prepare_enable(hipcie->aux_clk);
histb_pcie_host_disable
clk_disable_unprepare(hipcie->aux_clk);
clk_disable_unprepare(hipcie->pipe_clk);
clk_disable_unprepare(hipcie->sys_clk);
clk_disable_unprepare(hipcie->bus_clk);
After this patch, it looks like we'll have:
histb_pcie_host_enable
clk_bulk_prepare_enable
aux
pipe
sys
bus
histb_pcie_host_disable
clk_bulk_disable_unprepare
bus
sys
pipe
aux
So it looks like this patch will reverse the ordering both for
enabling and disabling, right? I'm totally fine with this as long as
it works.
Bjorn
Hi Bjorn, On Wed, 27 Aug 2025 at 00:32, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Aug 26, 2025 at 11:32:34PM +0530, Anand Moon wrote: > > Hi Bjorn, > > > > Thanks for your review comments. > > On Tue, 26 Aug 2025 at 21:55, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > In subject, remove "dwc: " to follow historical convention. (See > > > "git log --oneline") > > > > > Ok I will keep it as per the git history. > > > > > On Tue, Aug 26, 2025 at 05:12:40PM +0530, Anand Moon wrote: > > > > Currently, the driver acquires clocks and prepare/enable/disable/unprepare > > > > the clocks individually thereby making the driver complex to read. > > > > > > > > The driver can be simplified by using the clk_bulk*() APIs. > > > > > > > > Use: > > > > - devm_clk_bulk_get_all() API to acquire all the clocks > > > > - clk_bulk_prepare_enable() to prepare/enable clocks > > > > - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk > > > > > > I assume this means the order in which we prepare/enable and > > > disable/unprepare will now depend on the order the clocks appear in > > > the device tree instead of the order in the code? If so, please > > > mention that here and verify that all upstream device trees have the > > > correct order. > > > > > Following is the order in the device tree > > > > clocks = <&crg HISTB_PCIE_AUX_CLK>, > > <&crg HISTB_PCIE_PIPE_CLK>, > > <&crg HISTB_PCIE_SYS_CLK>, > > <&crg HISTB_PCIE_BUS_CLK>; > > clock-names = "aux", "pipe", "sys", "bus"; > > The current order in the code is: > > histb_pcie_host_enable > clk_prepare_enable(hipcie->bus_clk); > clk_prepare_enable(hipcie->sys_clk); > clk_prepare_enable(hipcie->pipe_clk); > clk_prepare_enable(hipcie->aux_clk); > > histb_pcie_host_disable > clk_disable_unprepare(hipcie->aux_clk); > clk_disable_unprepare(hipcie->pipe_clk); > clk_disable_unprepare(hipcie->sys_clk); > clk_disable_unprepare(hipcie->bus_clk); > > After this patch, it looks like we'll have: > > histb_pcie_host_enable > clk_bulk_prepare_enable > aux > pipe > sys > bus > > histb_pcie_host_disable > clk_bulk_disable_unprepare > bus > sys > pipe > aux > > So it looks like this patch will reverse the ordering both for > enabling and disabling, right? I'm totally fine with this as long as > it works. > Thank you for the input. Let's wait for additional feedback regarding the changes. > Bjorn Thanks -Anand
© 2016 - 2025 Red Hat, Inc.