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.
Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
drivers/pci/controller/pci-tegra.c | 71 +++++-------------------------
1 file changed, 12 insertions(+), 59 deletions(-)
diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index 467ddc701adce..3841489198b64 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -297,7 +297,6 @@ struct tegra_pcie_soc {
bool has_pex_clkreq_en;
bool has_pex_bias_ctrl;
bool has_intr_prsnt_sense;
- bool has_cml_clk;
bool has_gen2;
bool force_pca_enable;
bool program_uphy;
@@ -330,10 +329,8 @@ struct tegra_pcie {
struct resource cs;
- struct clk *pex_clk;
- struct clk *afi_clk;
- struct clk *pll_e;
- struct clk *cml_clk;
+ struct clk_bulk_data *clks;
+ int num_clks;
struct reset_control *pex_rst;
struct reset_control *afi_rst;
@@ -1153,15 +1150,11 @@ static void tegra_pcie_enable_controller(struct tegra_pcie *pcie)
static void tegra_pcie_power_off(struct tegra_pcie *pcie)
{
struct device *dev = pcie->dev;
- const struct tegra_pcie_soc *soc = pcie->soc;
int err;
reset_control_assert(pcie->afi_rst);
- clk_disable_unprepare(pcie->pll_e);
- if (soc->has_cml_clk)
- clk_disable_unprepare(pcie->cml_clk);
- clk_disable_unprepare(pcie->afi_clk);
+ clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
if (!dev->pm_domain)
tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
@@ -1174,7 +1167,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
static int tegra_pcie_power_on(struct tegra_pcie *pcie)
{
struct device *dev = pcie->dev;
- const struct tegra_pcie_soc *soc = pcie->soc;
int err;
reset_control_assert(pcie->pcie_xrst);
@@ -1202,35 +1194,16 @@ static int tegra_pcie_power_on(struct tegra_pcie *pcie)
}
}
- err = clk_prepare_enable(pcie->afi_clk);
+ err = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
if (err < 0) {
- dev_err(dev, "failed to enable AFI clock: %d\n", err);
+ dev_err(dev, "filed to enable clocks: %d\n", err);
goto powergate;
}
- if (soc->has_cml_clk) {
- err = clk_prepare_enable(pcie->cml_clk);
- if (err < 0) {
- dev_err(dev, "failed to enable CML clock: %d\n", err);
- goto disable_afi_clk;
- }
- }
-
- err = clk_prepare_enable(pcie->pll_e);
- if (err < 0) {
- dev_err(dev, "failed to enable PLLE clock: %d\n", err);
- goto disable_cml_clk;
- }
-
reset_control_deassert(pcie->afi_rst);
return 0;
-disable_cml_clk:
- if (soc->has_cml_clk)
- clk_disable_unprepare(pcie->cml_clk);
-disable_afi_clk:
- clk_disable_unprepare(pcie->afi_clk);
powergate:
if (!dev->pm_domain)
tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
@@ -1254,25 +1227,11 @@ static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
static int tegra_pcie_clocks_get(struct tegra_pcie *pcie)
{
struct device *dev = pcie->dev;
- const struct tegra_pcie_soc *soc = pcie->soc;
-
- pcie->pex_clk = devm_clk_get(dev, "pex");
- if (IS_ERR(pcie->pex_clk))
- return PTR_ERR(pcie->pex_clk);
- pcie->afi_clk = devm_clk_get(dev, "afi");
- if (IS_ERR(pcie->afi_clk))
- return PTR_ERR(pcie->afi_clk);
-
- pcie->pll_e = devm_clk_get(dev, "pll_e");
- if (IS_ERR(pcie->pll_e))
- return PTR_ERR(pcie->pll_e);
-
- if (soc->has_cml_clk) {
- pcie->cml_clk = devm_clk_get(dev, "cml");
- if (IS_ERR(pcie->cml_clk))
- return PTR_ERR(pcie->cml_clk);
- }
+ pcie->num_clks = devm_clk_bulk_get_all(dev, &pcie->clks);
+ if (pcie->num_clks < 0)
+ return dev_err_probe(dev, pcie->num_clks,
+ "failed to get clocks\n");
return 0;
}
@@ -2345,7 +2304,6 @@ static const struct tegra_pcie_soc tegra20_pcie = {
.has_pex_clkreq_en = false,
.has_pex_bias_ctrl = false,
.has_intr_prsnt_sense = false,
- .has_cml_clk = false,
.has_gen2 = false,
.force_pca_enable = false,
.program_uphy = true,
@@ -2374,7 +2332,6 @@ static const struct tegra_pcie_soc tegra30_pcie = {
.has_pex_clkreq_en = true,
.has_pex_bias_ctrl = true,
.has_intr_prsnt_sense = true,
- .has_cml_clk = true,
.has_gen2 = false,
.force_pca_enable = false,
.program_uphy = true,
@@ -2395,7 +2352,6 @@ static const struct tegra_pcie_soc tegra124_pcie = {
.has_pex_clkreq_en = true,
.has_pex_bias_ctrl = true,
.has_intr_prsnt_sense = true,
- .has_cml_clk = true,
.has_gen2 = true,
.force_pca_enable = false,
.program_uphy = true,
@@ -2418,7 +2374,6 @@ static const struct tegra_pcie_soc tegra210_pcie = {
.has_pex_clkreq_en = true,
.has_pex_bias_ctrl = true,
.has_intr_prsnt_sense = true,
- .has_cml_clk = true,
.has_gen2 = true,
.force_pca_enable = true,
.program_uphy = true,
@@ -2459,7 +2414,6 @@ static const struct tegra_pcie_soc tegra186_pcie = {
.has_pex_clkreq_en = true,
.has_pex_bias_ctrl = true,
.has_intr_prsnt_sense = true,
- .has_cml_clk = false,
.has_gen2 = true,
.force_pca_enable = false,
.program_uphy = false,
@@ -2672,7 +2626,7 @@ static int tegra_pcie_pm_suspend(struct device *dev)
}
reset_control_assert(pcie->pex_rst);
- clk_disable_unprepare(pcie->pex_clk);
+ clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
if (IS_ENABLED(CONFIG_PCI_MSI))
tegra_pcie_disable_msi(pcie);
@@ -2706,9 +2660,9 @@ static int tegra_pcie_pm_resume(struct device *dev)
if (IS_ENABLED(CONFIG_PCI_MSI))
tegra_pcie_enable_msi(pcie);
- err = clk_prepare_enable(pcie->pex_clk);
+ err = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
if (err) {
- dev_err(dev, "failed to enable PEX clock: %d\n", err);
+ dev_err(dev, "failed to enable clock: %d\n", err);
goto pex_dpd_enable;
}
@@ -2729,7 +2683,6 @@ static int tegra_pcie_pm_resume(struct device *dev)
disable_pex_clk:
reset_control_assert(pcie->pex_rst);
- clk_disable_unprepare(pcie->pex_clk);
pex_dpd_enable:
pinctrl_pm_select_idle_state(dev);
poweroff:
--
2.50.1On 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? Thanks Jon -- nvpublic
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 > Jon Thanks -Anand
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. 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. Jon [0] drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c -- nvpublic
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
> 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.
Please let me know if this plan addresses your concerns.
> Jon
>
> [0] drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
>
> --
> nvpublic
>
Thanks
-Anand
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
© 2016 - 2025 Red Hat, Inc.