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

[Jon Hunter <jonathanh@nvidia.com>]

On 18/09/2025 16:06, Anand Moon wrote:
> Hi Jon,
> 
> On Thu, 18 Sept 2025 at 14:47, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>
>> On 17/09/2025 19:26, Anand Moon wrote:
>>> Hi Jon,
>>>
>>> On Wed, 17 Sept 2025 at 19:14, Jon Hunter <jonathanh@nvidia.com> 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