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
[...]
