Re: [PATCH v1 1/2] PCI: dwc: histb: Simplify clock handling by using clk_bulk*() functions

[Bjorn Helgaas <helgaas@kernel.org>]

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