Hi,
> -----Original Message-----
> From: Chee, Tien Fong <tien.fong.c...@altera.com>
> Sent: Tuesday, August 5, 2025 3:34 PM
> To: Yuslaimi, Alif Zakuan <alif.zakuan.yusla...@altera.com>; Peng Fan
> <peng....@oss.nxp.com>
> Cc: u-boot@lists.denx.de; Peng Fan <peng....@nxp.com>; Jaehoon Chung
> <jh80.ch...@samsung.com>; Tom Rini <tr...@konsulko.com>; Marek Vasut
> <ma...@denx.de>; Simon Goldschmidt <simon.k.r.goldschm...@gmail.com>
> Subject: RE: [PATCH v1] mmc: socfpga_dw_mmc: Retrieve clock manager
> address via clocks phandle
>
> Hi Alif,
>
> > -----Original Message-----
> > From: Yuslaimi, Alif Zakuan <alif.zakuan.yusla...@altera.com>
> > Sent: Tuesday, August 5, 2025 2:03 PM
> > To: Peng Fan <peng....@oss.nxp.com>
> > Cc: u-boot@lists.denx.de; Peng Fan <peng....@nxp.com>; Jaehoon Chung
> > <jh80.ch...@samsung.com>; Tom Rini <tr...@konsulko.com>; Chee, Tien
> > Fong <tien.fong.c...@altera.com>; Marek Vasut <ma...@denx.de>; Simon
> > Goldschmidt <simon.k.r.goldschm...@gmail.com>
> > Subject: RE: [PATCH v1] mmc: socfpga_dw_mmc: Retrieve clock manager
> > address via clocks phandle
> >
> > > -----Original Message-----
> > > From: Peng Fan <peng....@oss.nxp.com>
> > > Sent: Tuesday, August 5, 2025 12:08 PM
> > > To: Yuslaimi, Alif Zakuan <alif.zakuan.yusla...@altera.com>
> > > Cc: u-boot@lists.denx.de; Peng Fan <peng....@nxp.com>; Jaehoon Chung
> > > <jh80.ch...@samsung.com>; Tom Rini <tr...@konsulko.com>; Chee, Tien
> > > Fong <tien.fong.c...@altera.com>; Marek Vasut <ma...@denx.de>;
> > Simon
> > > Goldschmidt <simon.k.r.goldschm...@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.yusla...@altera.com
> > > wrote:
> > > >From: Alif Zakuan Yuslaimi <alif.zakuan.yusla...@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.yusla...@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@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
>
Thanks for the comments, I will update the clock driver to support
enable/disable APIs
and call those APIs in this MMC driver as recommended in v2.
Alif
> [...]
