[PATCH v1] mmc: socfpga_dw_mmc: Retrieve clock manager address via clocks phandle

[Yuslaimi, Alif Zakuan]

> -----Original Message-----
> From: Peng Fan <peng.fan at oss.nxp.com>
> Sent: Tuesday, August 5, 2025 12:08 PM
> To: Yuslaimi, Alif Zakuan <alif.zakuan.yuslaimi at altera.com>
> Cc: u-boot at lists.denx.de; Peng Fan <peng.fan at nxp.com>; Jaehoon Chung
> <jh80.chung at samsung.com>; Tom Rini <trini at konsulko.com>; Chee, Tien Fong
> <tien.fong.chee at altera.com>; Marek Vasut <marex at denx.de>; Simon
> Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> Subject: Re: [PATCH v1] mmc: socfpga_dw_mmc: Retrieve clock manager
> address via clocks phandle
> 
> [CAUTION: This email is from outside your organization. Unless you trust the
> sender, do not click on links or open attachments as it may be a fraudulent email
> attempting to steal your information and/or compromise your computer.]
> 
> On Sun, Aug 03, 2025 at 05:47:10PM -0700, alif.zakuan.yuslaimi at altera.com
> wrote:
> >From: Alif Zakuan Yuslaimi <alif.zakuan.yuslaimi at altera.com>
> >
> >Improve the current clock manager base address retrieval by probing the
> >clocks phandle under the mmc node in the device tree instead of probing
> >the clock manager node name in the device tree.
> >
> >This will help to make the driver more scalable for more families as we
> >no longer need to rely on the exact node name in the device tree to
> >probe the clock manager driver for its base address.
> >
> >Signed-off-by: Alif Zakuan Yuslaimi <alif.zakuan.yuslaimi at altera.com>
> >---
> > drivers/mmc/socfpga_dw_mmc.c | 19 +++++++++++++++----
> > 1 file changed, 15 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/mmc/socfpga_dw_mmc.c
> >b/drivers/mmc/socfpga_dw_mmc.c index 3b86bc9b18c..ff1dd0055f8 100644
> >--- a/drivers/mmc/socfpga_dw_mmc.c
> >+++ b/drivers/mmc/socfpga_dw_mmc.c
> >@@ -54,17 +54,28 @@ static int socfpga_dwmci_clksel(struct dwmci_host
> *host)
> >       u32 sdmmc_mask = ((priv->smplsel & 0x7) <<
> SYSMGR_SDMMC_SMPLSEL_SHIFT) |
> >                        ((priv->drvsel & 0x7) <<
> >SYSMGR_SDMMC_DRVSEL_SHIFT);
> >
> >-      /* Get clock manager base address */
> >+      struct udevice *mmc_dev = host->mmc->dev;
> >+      struct ofnode_phandle_args clkmgr_args;
> >       struct udevice *clkmgr_dev;
> >-      int ret = uclass_get_device_by_name(UCLASS_CLK, "clock-
> controller at ffd10000", &clkmgr_dev);
> >+      fdt_addr_t clkmgr_base;
> >+      int ret;
> >
> >+      /*
> >+       * Get the clkmgr device from the first phandle in the "clocks" property.
> >+       */
> >+      ret = dev_read_phandle_with_args(mmc_dev, "clocks",
> >+ "#clock-cells", 0, 0, &clkmgr_args);
> >       if (ret) {
> >-              printf("Failed to get clkmgr device: %d\n", ret);
> >+              printf("Failed to parse clocks property: %d\n", ret);
> >               return ret;
> >       }
> >
> >-      fdt_addr_t clkmgr_base = dev_read_addr(clkmgr_dev);
> >+      ret = uclass_get_device_by_ofnode(UCLASS_CLK, clkmgr_args.node,
> &clkmgr_dev);
> >+      if (ret) {
> >+              printf("Failed to get clkmgr device: %d\n", ret);
> >+              return ret;
> >+      }
> >
> >+      clkmgr_base = dev_read_addr(clkmgr_dev);
> 
> I see this driver use clk api, so why still directly map the clk manager base and
> configure it. Shouldn't clk api be used or assigned-clock-parents be used?
> 
> Thanks,
> Peng

Hi Peng,

The direct access to the clock manager base and offset in this driver was inherited from the legacy implementation before we had full driver model clock support. Currently, both Agilex and Agilex5 have their own clock drivers under the driver model, but the `clk_enable()` and `clk_disable()` operations are still not implemented.
 
Because of that, we retained the base + offset logic for disabling a particular clock in the SDMMC driver to preserve existing functionality. Once proper enable/disable API support is added to the clock drivers, we can clean this up to fully rely on the clock API or use `assigned-clock-parents` in device tree where applicable.

Thanks,
Alif

> 
> >       if (clkmgr_base == FDT_ADDR_T_NONE) {
> >               printf("Failed to read base address from clkmgr DT node\n");
> >               return -EINVAL;
> >--
> >2.35.3
> >