Re: [RFC v1 1/2] PCI: tegra: Simplify clock handling by using clk_bulk*() functions

[Jon Hunter]

On 18/09/2025 16:06, Anand Moon wrote:
> Hi Jon,
>
> On Thu, 18 Sept 2025 at 14:47, Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
> > On 17/09/2025 19:26, Anand Moon wrote:
> > > Hi Jon,
> > >
> > > On Wed, 17 Sept 2025 at 19:14, Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
> > > > On 31/08/2025 20:00, 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
> > > > >
> > > > > As part of this cleanup, the legacy has_cml_clk flag and explicit handling
> > > > > of individual clocks (pex, afi, pll_e, cml) are removed. Clock sequencing
> > > > > is now implicitly determined by the order defined in the device tree,
> > > > > eliminating hardcoded logic and improving maintainability.
> > > >
> > > > What platforms have you tested this change on?
> > > I'm using a Jetson Nano 4GB model as my test platform.
> >
> > Thanks. One concern I have about this is that right now the DT binding
> > doc for this device is still in the legacy text format and not converted
> > to yaml. Therefore, there is no way to validate the device-tree bindings
> > for this driver. So by making this change we are susceptible to people
> > getting the device-tree incorrect and there is no way to check. Right
> > now the driver will fail is a given clock is missing but after this
> > change we are completely reliant that the device-tree is correct but no
> > way to validate.
> >
> > It would be great to convert the text binding doc to yaml as part of
> > this series.
> I will convert the legacy text binding to a YAML file as part of this series.
>
> [0] Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt

Thanks!

> > Also if you look at the dwmac-tegra.c driver this one still populates
> > the clock names when using the bulk APIs so that we know that the clocks
> > that we require are present.
> Only the Tegra20 SoC has three clocks.
>      compatible = "nvidia,tegra20-pcie";
>      clocks = <&tegra_car TEGRA20_CLK_PEX>,
>                           <&tegra_car TEGRA20_CLK_AFI>,
>                           <&tegra_car TEGRA20_CLK_PLL_E>;
>                  clock-names = "pex", "afi", "pll_e";
>
> Whereas all the rest of the SoCs have 4 clocks.
>
>    compatible = "nvidia,tegra30-pcie";
>    compatible = "nvidia,tegra124-pcie";
>    compatible = "nvidia,tegra210-pcie";
>    compatible = "nvidia,tegra186-pcie";
>
>    clocks = <&tegra_car TEGRA30_CLK_PCIE>,
>                           <&tegra_car TEGRA30_CLK_AFI>,
>                           <&tegra_car TEGRA30_CLK_PLL_E>,
>                           <&tegra_car TEGRA30_CLK_CML0>;
>                  clock-names = "pex", "afi", "pll_e", "cml";
>
> As suggested, I need to create two clock arrays for the clocks of the SoC.
>
> But the code will introduce more overhead:
>
> bulk clks -> devm_kcalloc (for clocks) -> assign id to clocks ->
> devm_clk_bulk_get -> clk_bulk_prepare_enable.
>
> I believe the use of devm_clk_bulk_get_all() is a cleaner and more modern
> approach for the following reasons:
> It simplifies the code by removing the need for manual memory allocation
> (devm_kcalloc) and populating an array of clock specifications.
> It is more efficient, as all clocks are fetched in a single API call,
> reducing overhead.

Yes that's correct and I don't think it is that much overhead. The clk 
names array can just be part of the 'tegra_pcie_soc' structure.

Jon

--
nvpublic