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

[Chee, Tien Fong]

Hi Alif,

> -----Original Message-----
> From: Yuslaimi, Alif Zakuan <alif.zakuan.yuslaimi at altera.com>
> Sent: Tuesday, August 5, 2025 2:03 PM
> To: Peng Fan <peng.fan at oss.nxp.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
> 
> > -----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.
> 

I think SSBL has no privilege to direct access clk mgr register, so ATF-mediated access is required. I agreed with Peng, these issues can be solved in clk disable / enable API by having the right API access at both SPL and SSBL boot phases.

Thanks.
TF

[...]