[PATCH 2/5] mmc: zynq-sdhci: refactor tapdelay settings
Steffen Dirkwinkel
lists at steffen.cc
Fri Mar 22 08:58:25 CET 2024
Hi Venkatesh,
we're not using the firmware when running in el3, so that should still
work. The sdhci driver needs to know which controller it's operating on
and uses the power-domain node for that. I can't add the power domain
node without a firmware node (i think?), but the firmware node without
a compatible shouldn't be used by anything?
Alternatively we could exit without an error from these functions if
our node id doesn't match sd0 or sd1. That wouldn't apply any settings,
but that's the current behaviour already.
Thanks
Steffen
On Fri, 2024-03-15 at 09:52 +0000, Abbarapu, Venkatesh wrote:
> Hi Steffen,
> For mini u-boot cases ZYNQMP_FIRMWARE config is disabled as there
> won't be any ATF(TF-A) present.
>
> Thanks
> Venkatesh
>
> > -----Original Message-----
> > From: Steffen Dirkwinkel <lists at steffen.cc>
> > Sent: Thursday, March 14, 2024 1:23 PM
> > To: Kumar, Love <love.kumar at amd.com>; u-boot at lists.denx.de
> > Cc: Algapally Santosh Sagar <santoshsagar.algapally at amd.com>; Ashok
> > Reddy Soma <ashok.reddy.soma at amd.com>; Jaehoon Chung
> > <jh80.chung at samsung.com>; Johan Jonker <jbx6244 at gmail.com>; Simek,
> > Michal <michal.simek at amd.com>; Peng Fan <peng.fan at nxp.com>; Tom
> > Rini
> > <trini at konsulko.com>; Abbarapu, Venkatesh
> > <venkatesh.abbarapu at amd.com>
> > Subject: Re: [PATCH 2/5] mmc: zynq-sdhci: refactor tapdelay
> > settings
> >
> > Hi Love,
> >
> > On Thu, 2024-03-14 at 12:15 +0530, Love Kumar wrote:
> > > Hi,
> > >
> > > When we run in el3 for zynqmp board, we are seeing below issue
> > > with
> > > this
> > > patch:
> > >
> > >
> > > Model: ZynqMP MINI EMMC0
> > > Board: Xilinx ZynqMP
> > > DRAM: 512 MiB
> > > EL Level: EL3
> > > Secure Boot: not authenticated, not encrypted
> > > Multiboot: 0
> > > Core: 10 devices, 9 uclasses, devicetree: embed
> > > MMC: sdhci at ff160000: 0
> > > Loading Environment from <NULL>... OK
> > > In: dcc
> > > Out: dcc
> > > Err: dcc
> > > ZynqMP> mmc list
> > > sdhci at ff160000: 0
> > > ZynqMP> mmc dev 0 0
> > > arasan_sdhci sdhci at ff160000: Error setting Input Tap Delay
> > > sdhci_set_clock: Error while setting tap delay
> > > sdhci_send_command: Timeout for status update: 00000000 00000001
> > > ZynqMP>
> >
> > Thank you for testing this.
> > It looks like the zynqmp-mini-emmc0 device lacks the power-domain
> > node
> > and doesn't have a sd node id. The code before my change would have
> > ignored the unknown node id, applied settings for sdhci1 instead of
> > sdhci0
> > and returned without an error. Maybe there's a different way to
> > find out
> > which sdhci we want to operate on?
> >
> > Can you try setting the node id in device tree?
> > Something like this:
> >
> > diff --git a/arch/arm/dts/zynqmp-mini-emmc0.dts
> > b/arch/arm/dts/zynqmp-
> > mini-emmc0.dts
> > index 02e80bd85e1..87c4a1b6ad0 100644
> > --- a/arch/arm/dts/zynqmp-mini-emmc0.dts
> > +++ b/arch/arm/dts/zynqmp-mini-emmc0.dts
> > @@ -7,6 +7,8 @@
> > * Siva Durga Prasad Paladugu <siva.durga.prasad.paladugu at amd.com>
> > */
> >
> > +#include <dt-bindings/power/xlnx-zynqmp-power.h>
> > +
> > /dts-v1/;
> >
> > / {
> > @@ -41,6 +43,12 @@
> > clock-frequency = <200000000>;
> > };
> >
> > + firmware {
> > + zynqmp_firmware: zynqmp-firmware {
> > + #power-domain-cells = <1>;
> > + };
> > + };
> > +
> > amba: amba {
> > compatible = "simple-bus";
> > #address-cells = <2>;
> > @@ -56,6 +64,7 @@
> > reg = <0x0 0xff160000 0x0 0x1000>;
> > clock-names = "clk_xin", "clk_ahb";
> > clocks = <&clk_xin &clk_xin>;
> > + power-domains = <&zynqmp_firmware PD_SD_0>;
> > };
> > };
> > };
> >
> > Thanks,
> > Steffen
> >
> > >
> > > Regards,
> > > Love Kumar
> > >
> > > On 23/02/24 7:36 pm, Steffen Dirkwinkel wrote:
> > > > From: Steffen Dirkwinkel <s.dirkwinkel at beckhoff.com>
> > > >
> > > > Previously we were setting in tapdelay for SD1 every time even
> > > > if
> > > > SD0 was requested.
> > > > The SD tapdelay settings are shifted by 16 bits between SD0 and
> > > > SD1.
> > > > We can use that to make our tapdelay setup simpler. This is
> > > > also how
> > > > it currently works in arm-trusted-firmware.
> > > >
> > > > Signed-off-by: Steffen Dirkwinkel <s.dirkwinkel at beckhoff.com>
> > > > ---
> > > >
> > > > drivers/mmc/zynq_sdhci.c | 65
> > > > +++++++++++++++++-----------------------
> > > > 1 file changed, 28 insertions(+), 37 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/zynq_sdhci.c
> > > > b/drivers/mmc/zynq_sdhci.c
> > > > index 935540d1719..d4845245b2a 100644
> > > > --- a/drivers/mmc/zynq_sdhci.c
> > > > +++ b/drivers/mmc/zynq_sdhci.c
> > > > @@ -42,14 +42,11 @@
> > > > #define SD_OTAP_DLY 0xFF180318
> > > > #define SD0_DLL_RST BIT(2)
> > > > #define SD1_DLL_RST BIT(18)
> > > > +#define SD1_TAP_OFFSET 16
> > > > #define SD0_ITAPCHGWIN BIT(9)
> > > > -#define SD1_ITAPCHGWIN BIT(25)
> > > > #define SD0_ITAPDLYENA BIT(8)
> > > > -#define SD1_ITAPDLYENA BIT(24)
> > > > #define SD0_ITAPDLYSEL_MASK GENMASK(7, 0)
> > > > -#define SD1_ITAPDLYSEL_MASK GENMASK(23, 16)
> > > > #define SD0_OTAPDLYSEL_MASK GENMASK(5, 0)
> > > > -#define SD1_OTAPDLYSEL_MASK GENMASK(21, 16)
> > > >
> > > > #define MIN_PHY_CLK_HZ 50000000
> > > >
> > > > @@ -275,44 +272,32 @@ static int arasan_sdhci_config_dll(struct
> > > > sdhci_host *host, unsigned int clock,
> > > > static inline int arasan_zynqmp_set_in_tapdelay(u32 node_id,
> > > > u32
> > > > itap_delay)
> > > > {
> > > > int ret;
> > > > + u32 shift;
> > > >
> > > > - if (IS_ENABLED(CONFIG_SPL_BUILD) || current_el() == 3)
> > > > {
> > > > - if (node_id == NODE_SD_0) {
> > > > - ret = zynqmp_mmio_write(SD_ITAP_DLY,
> > SD0_ITAPCHGWIN,
> > > > -
> > > > SD0_ITAPCHGWIN);
> > > > - if (ret)
> > > > - return ret;
> > > > -
> > > > - ret = zynqmp_mmio_write(SD_ITAP_DLY,
> > SD0_ITAPDLYENA,
> > > > -
> > > > SD0_ITAPDLYENA);
> > > > - if (ret)
> > > > - return ret;
> > > > -
> > > > - ret = zynqmp_mmio_write(SD_ITAP_DLY,
> > SD0_ITAPDLYSEL_MASK,
> > > > - itap_delay);
> > > > - if (ret)
> > > > - return ret;
> > > > + if (node_id == NODE_SD_0)
> > > > + shift = 0;
> > > > + else if (node_id == NODE_SD_1)
> > > > + shift = SD1_TAP_OFFSET;
> > > > + else
> > > > + return -EINVAL;
> > > >
> > > > - ret = zynqmp_mmio_write(SD_ITAP_DLY,
> > SD0_ITAPCHGWIN, 0);
> > > > - if (ret)
> > > > - return ret;
> > > > - }
> > > > - ret = zynqmp_mmio_write(SD_ITAP_DLY,
> > > > SD1_ITAPCHGWIN,
> > > > - SD1_ITAPCHGWIN);
> > > > + if (IS_ENABLED(CONFIG_SPL_BUILD) || current_el() == 3)
> > > > {
> > > > + ret = zynqmp_mmio_write(SD_ITAP_DLY,
> > > > SD0_ITAPCHGWIN
> > << shift,
> > > > + SD0_ITAPCHGWIN <<
> > > > shift);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - ret = zynqmp_mmio_write(SD_ITAP_DLY,
> > > > SD1_ITAPDLYENA,
> > > > - SD1_ITAPDLYENA);
> > > > + ret = zynqmp_mmio_write(SD_ITAP_DLY,
> > > > SD0_ITAPDLYENA <<
> > shift,
> > > > + SD0_ITAPDLYENA <<
> > > > shift);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - ret = zynqmp_mmio_write(SD_ITAP_DLY,
> > SD1_ITAPDLYSEL_MASK,
> > > > - (itap_delay << 16));
> > > > + ret = zynqmp_mmio_write(SD_ITAP_DLY,
> > SD0_ITAPDLYSEL_MASK << shift,
> > > > + itap_delay << shift);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - ret = zynqmp_mmio_write(SD_ITAP_DLY,
> > > > SD1_ITAPCHGWIN,
> > 0);
> > > > + ret = zynqmp_mmio_write(SD_ITAP_DLY,
> > > > SD0_ITAPCHGWIN
> > << shift, 0);
> > > > if (ret)
> > > > return ret;
> > > > } else {
> > > > @@ -326,14 +311,20 @@ static inline int
> > > > arasan_zynqmp_set_in_tapdelay(u32 node_id, u32 itap_delay)
> > > >
> > > > static inline int arasan_zynqmp_set_out_tapdelay(u32 node_id,
> > > > u32
> > > > otap_delay)
> > > > {
> > > > + u32 shift;
> > > > +
> > > > + if (node_id == NODE_SD_0)
> > > > + shift = 0;
> > > > + else if (node_id == NODE_SD_1)
> > > > + shift = SD1_TAP_OFFSET;
> > > > + else
> > > > + return -EINVAL;
> > > > +
> > > > if (IS_ENABLED(CONFIG_SPL_BUILD) || current_el() == 3)
> > > > {
> > > > - if (node_id == NODE_SD_0)
> > > > - return zynqmp_mmio_write(SD_OTAP_DLY,
> > > > -
> > > > SD0_OTAPDLYSEL_MASK,
> > > > - otap_delay);
> > > > + return zynqmp_mmio_write(SD_OTAP_DLY,
> > > > + SD0_OTAPDLYSEL
> > > > _MASK <<
> > shift,
> > > > + otap_delay <<
> > > > shift);
> > > >
> > > > - return zynqmp_mmio_write(SD_OTAP_DLY,
> > SD1_OTAPDLYSEL_MASK,
> > > > - (otap_delay << 16));
> > > > } else {
> > > > return xilinx_pm_request(PM_IOCTL, node_id,
> > > >
> > > > IOCTL_SET_SD_TAPDELAY,
>
More information about the U-Boot
mailing list