Message ID | d673ff3bdc5c236a7f0403c920e719684abd6059.1688991117.git.michal.simek@amd.com |
---|---|
State | Accepted |
Commit | 752e4b6c8efb5bd624f316f7a9686d9956d55dd7 |
Delegated to: | Michal Simek |
Headers | show |
Series | mmc: zynq_sdhci: Dll reset only for ZynqMP platform | expand |
Hi, > -----Original Message----- > From: Michal Simek <michal.simek@amd.com> > Sent: Monday, July 10, 2023 9:12 PM > To: u-boot@lists.denx.de; git@xilinx.com > Cc: Ashok Reddy Soma <ashok.reddy.soma@amd.com>; Jaehoon Chung <jh80.chung@samsung.com>; Peng Fan > <peng.fan@nxp.com> > Subject: [PATCH] mmc: zynq_sdhci: Dll reset only for ZynqMP platform > > From: Ashok Reddy Soma <ashok.reddy.soma@amd.com> > > Dll reset is needed only for ZynqMP platforms, add condition in tuning > to call arasan_zynqmp_dll_reset() just for ZynqMP platforms. > > On other platforms like Versal NET, If this condition is not added, we > see PLM error messages when dll reset smc is called. > > Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@amd.com> > Signed-off-by: Michal Simek <michal.simek@amd.com> > --- > > drivers/mmc/zynq_sdhci.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c > index e779251ce34f..935540d17194 100644 > --- a/drivers/mmc/zynq_sdhci.c > +++ b/drivers/mmc/zynq_sdhci.c > @@ -422,7 +422,8 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode) > > mdelay(1); > > - arasan_zynqmp_dll_reset(host, priv->node_id); > + if (device_is_compatible(mmc->dev, "xlnx,zynqmp-8.9a")) > + arasan_zynqmp_dll_reset(host, priv->node_id); How about using local variable to check whether it needs to reset or not? It's not efficient to call device_is_compatible() everytime. (I'm not sure that it will be added more in future.) e.g) bool reset = device_is_compatible(mmc->dev, "xlx,zynmp-8.8a"); if (reset) arasan_zynqmp_dll_reset(host, priv->node_id); .. If (reset) arasan_zynqmp_dll_reset(host, priv->node_id); Best Regards, Jaehoon Chung > > sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_INT_ENABLE); > sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_SIGNAL_ENABLE); > @@ -468,7 +469,9 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode) > } > > udelay(1); > - arasan_zynqmp_dll_reset(host, priv->node_id); > + > + if (device_is_compatible(mmc->dev, "xlnx,zynqmp-8.9a")) > + arasan_zynqmp_dll_reset(host, priv->node_id); > > /* Enable only interrupts served by the SD controller */ > sdhci_writel(host, SDHCI_INT_DATA_MASK | SDHCI_INT_CMD_MASK, > -- > 2.36.1
Hi, On 7/11/23 07:00, Jaehoon Chung wrote: > Hi, > >> -----Original Message----- >> From: Michal Simek <michal.simek@amd.com> >> Sent: Monday, July 10, 2023 9:12 PM >> To: u-boot@lists.denx.de; git@xilinx.com >> Cc: Ashok Reddy Soma <ashok.reddy.soma@amd.com>; Jaehoon Chung <jh80.chung@samsung.com>; Peng Fan >> <peng.fan@nxp.com> >> Subject: [PATCH] mmc: zynq_sdhci: Dll reset only for ZynqMP platform >> >> From: Ashok Reddy Soma <ashok.reddy.soma@amd.com> >> >> Dll reset is needed only for ZynqMP platforms, add condition in tuning >> to call arasan_zynqmp_dll_reset() just for ZynqMP platforms. >> >> On other platforms like Versal NET, If this condition is not added, we >> see PLM error messages when dll reset smc is called. >> >> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@amd.com> >> Signed-off-by: Michal Simek <michal.simek@amd.com> >> --- >> >> drivers/mmc/zynq_sdhci.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c >> index e779251ce34f..935540d17194 100644 >> --- a/drivers/mmc/zynq_sdhci.c >> +++ b/drivers/mmc/zynq_sdhci.c >> @@ -422,7 +422,8 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode) >> >> mdelay(1); >> >> - arasan_zynqmp_dll_reset(host, priv->node_id); >> + if (device_is_compatible(mmc->dev, "xlnx,zynqmp-8.9a")) >> + arasan_zynqmp_dll_reset(host, priv->node_id); > > How about using local variable to check whether it needs to reset or not? > It's not efficient to call device_is_compatible() everytime. > (I'm not sure that it will be added more in future.) > > e.g) > bool reset = device_is_compatible(mmc->dev, "xlx,zynmp-8.8a"); > > if (reset) > arasan_zynqmp_dll_reset(host, priv->node_id); > > .. > > If (reset) > arasan_zynqmp_dll_reset(host, priv->node_id); This is very valid request and TBH I have already added this to our TODO list to convert all device_is_compatible() to flags because over time the driver was extended and this construct is used more than it should be. This is going to be the last device_is_compatible() patch. Is it fine for you? Thanks, Michal
Hi, > -----Original Message----- > From: Michal Simek <michal.simek@amd.com> > Sent: Tuesday, July 11, 2023 3:28 PM > To: Jaehoon Chung <jh80.chung@samsung.com>; u-boot@lists.denx.de; git@xilinx.com > Cc: 'Ashok Reddy Soma' <ashok.reddy.soma@amd.com>; 'Peng Fan' <peng.fan@nxp.com> > Subject: Re: [PATCH] mmc: zynq_sdhci: Dll reset only for ZynqMP platform > > Hi, > > On 7/11/23 07:00, Jaehoon Chung wrote: > > Hi, > > > >> -----Original Message----- > >> From: Michal Simek <michal.simek@amd.com> > >> Sent: Monday, July 10, 2023 9:12 PM > >> To: u-boot@lists.denx.de; git@xilinx.com > >> Cc: Ashok Reddy Soma <ashok.reddy.soma@amd.com>; Jaehoon Chung <jh80.chung@samsung.com>; Peng Fan > >> <peng.fan@nxp.com> > >> Subject: [PATCH] mmc: zynq_sdhci: Dll reset only for ZynqMP platform > >> > >> From: Ashok Reddy Soma <ashok.reddy.soma@amd.com> > >> > >> Dll reset is needed only for ZynqMP platforms, add condition in tuning > >> to call arasan_zynqmp_dll_reset() just for ZynqMP platforms. > >> > >> On other platforms like Versal NET, If this condition is not added, we > >> see PLM error messages when dll reset smc is called. > >> > >> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@amd.com> > >> Signed-off-by: Michal Simek <michal.simek@amd.com> > >> --- > >> > >> drivers/mmc/zynq_sdhci.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c > >> index e779251ce34f..935540d17194 100644 > >> --- a/drivers/mmc/zynq_sdhci.c > >> +++ b/drivers/mmc/zynq_sdhci.c > >> @@ -422,7 +422,8 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode) > >> > >> mdelay(1); > >> > >> - arasan_zynqmp_dll_reset(host, priv->node_id); > >> + if (device_is_compatible(mmc->dev, "xlnx,zynqmp-8.9a")) > >> + arasan_zynqmp_dll_reset(host, priv->node_id); > > > > How about using local variable to check whether it needs to reset or not? > > It's not efficient to call device_is_compatible() everytime. > > (I'm not sure that it will be added more in future.) > > > > e.g) > > bool reset = device_is_compatible(mmc->dev, "xlx,zynmp-8.8a"); > > > > if (reset) > > arasan_zynqmp_dll_reset(host, priv->node_id); > > > > .. > > > > If (reset) > > arasan_zynqmp_dll_reset(host, priv->node_id); > > This is very valid request and TBH I have already added this to our TODO list to > convert all device_is_compatible() to flags because over time the driver was > extended and this construct is used more than it should be. > > This is going to be the last device_is_compatible() patch. > Is it fine for you? It's fine. Thanks! Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com> Best Regards, Jaehoon Chung > > Thanks, > Michal > >
On 7/10/23 14:11, Michal Simek wrote: > From: Ashok Reddy Soma <ashok.reddy.soma@amd.com> > > Dll reset is needed only for ZynqMP platforms, add condition in tuning > to call arasan_zynqmp_dll_reset() just for ZynqMP platforms. > > On other platforms like Versal NET, If this condition is not added, we > see PLM error messages when dll reset smc is called. > > Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@amd.com> > Signed-off-by: Michal Simek <michal.simek@amd.com> > --- > > drivers/mmc/zynq_sdhci.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c > index e779251ce34f..935540d17194 100644 > --- a/drivers/mmc/zynq_sdhci.c > +++ b/drivers/mmc/zynq_sdhci.c > @@ -422,7 +422,8 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode) > > mdelay(1); > > - arasan_zynqmp_dll_reset(host, priv->node_id); > + if (device_is_compatible(mmc->dev, "xlnx,zynqmp-8.9a")) > + arasan_zynqmp_dll_reset(host, priv->node_id); > > sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_INT_ENABLE); > sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_SIGNAL_ENABLE); > @@ -468,7 +469,9 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode) > } > > udelay(1); > - arasan_zynqmp_dll_reset(host, priv->node_id); > + > + if (device_is_compatible(mmc->dev, "xlnx,zynqmp-8.9a")) > + arasan_zynqmp_dll_reset(host, priv->node_id); > > /* Enable only interrupts served by the SD controller */ > sdhci_writel(host, SDHCI_INT_DATA_MASK | SDHCI_INT_CMD_MASK, Applied. M
diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c index e779251ce34f..935540d17194 100644 --- a/drivers/mmc/zynq_sdhci.c +++ b/drivers/mmc/zynq_sdhci.c @@ -422,7 +422,8 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode) mdelay(1); - arasan_zynqmp_dll_reset(host, priv->node_id); + if (device_is_compatible(mmc->dev, "xlnx,zynqmp-8.9a")) + arasan_zynqmp_dll_reset(host, priv->node_id); sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_INT_ENABLE); sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_SIGNAL_ENABLE); @@ -468,7 +469,9 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode) } udelay(1); - arasan_zynqmp_dll_reset(host, priv->node_id); + + if (device_is_compatible(mmc->dev, "xlnx,zynqmp-8.9a")) + arasan_zynqmp_dll_reset(host, priv->node_id); /* Enable only interrupts served by the SD controller */ sdhci_writel(host, SDHCI_INT_DATA_MASK | SDHCI_INT_CMD_MASK,