RE: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning support for ZynqMP Platform

[Manish Narani]

> -----Original Message-----
> From: Adrian Hunter [mailto:adrian.hunter@xxxxxxxxx]
> Sent: Friday, February 23, 2018 1:04 PM
> To: Manish Narani <MNARANI@xxxxxxxxxx>; michal.simek@xxxxxxxxxx;
> ulf.hansson@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> mmc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; mark.rutland@xxxxxxx; robh+dt@xxxxxxxxxx
> Cc: Anirudha Sarangi <anirudh@xxxxxxxxxx>; Srinivas Goud
> <sgoud@xxxxxxxxxx>
> Subject: Re: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning support for
> ZynqMP Platform
> 
> On 23/02/18 09:29, Manish Narani wrote:
> >
> >
> >> -----Original Message-----
> >> From: Adrian Hunter [mailto:adrian.hunter@xxxxxxxxx]
> >> Sent: Thursday, February 22, 2018 1:50 PM
> >> To: Manish Narani <MNARANI@xxxxxxxxxx>; michal.simek@xxxxxxxxxx;
> >> ulf.hansson@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> >> mmc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >> devicetree@xxxxxxxxxxxxxxx; mark.rutland@xxxxxxx;
> robh+dt@xxxxxxxxxx
> >> Cc: Anirudha Sarangi <anirudh@xxxxxxxxxx>; Srinivas Goud
> >> <sgoud@xxxxxxxxxx>
> >> Subject: Re: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning
> >> support for ZynqMP Platform
> >>
> >> On 21/02/18 17:00, Manish Narani wrote:
> >>> Hi Adrian,
> >>>
> >>>> -----Original Message-----
> >>>> From: Manish Narani
> >>>> Sent: Wednesday, February 21, 2018 11:39 AM
> >>>> To: Adrian Hunter <adrian.hunter@xxxxxxxxx>;
> >>>> michal.simek@xxxxxxxxxx; ulf.hansson@xxxxxxxxxx;
> >>>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- mmc@xxxxxxxxxxxxxxx;
> >>>> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> >>>> mark.rutland@xxxxxxx;
> >> robh+dt@xxxxxxxxxx
> >>>> Cc: Anirudha Sarangi <anirudh@xxxxxxxxxx>; Srinivas Goud
> >>>> <sgoud@xxxxxxxxxx>
> >>>> Subject: RE: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning
> >>>> support for ZynqMP Platform
> >>>>
> >>>> Hi Adrian,
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Adrian Hunter [mailto:adrian.hunter@xxxxxxxxx]
> >>>>> Sent: Friday, February 16, 2018 7:37 PM
> >>>>> To: Manish Narani <MNARANI@xxxxxxxxxx>;
> michal.simek@xxxxxxxxxx;
> >>>>> ulf.hansson@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> >>>>> linux- mmc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >>>>> devicetree@xxxxxxxxxxxxxxx; mark.rutland@xxxxxxx;
> >> robh+dt@xxxxxxxxxx
> >>>>> Cc: Anirudha Sarangi <anirudh@xxxxxxxxxx>; Srinivas Goud
> >>>>> <sgoud@xxxxxxxxxx>; Manish Narani <MNARANI@xxxxxxxxxx>
> >>>>> Subject: Re: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning
> >>>>> support for ZynqMP Platform
> >>>>>
> >>>>> On 30/01/18 20:14, Manish Narani wrote:
> >>>>>> This patch adds support of SD auto tuning for ZynqMP platform.
> >>>>>> Auto tuning sequence sends tuning block to card when operating in
> >>>>>> UHS-1 modes. This resets the DLL and sends CMD19/CMD21 as a part
> >>>>>> of the auto tuning process. Once the auto tuning process gets
> >>>>>> completed, reset the DLL to load the newly obtained SDHC tuned tap
> value.
> >>>>>
> >>>>> How is this different from:
> >>>>> 	1. reset the dll
> >>>>> 	2. call sdhci_execute_tuning
> >>>>> 	3. reset the dll
> >>>>>
> >>> Below is my take on your above comments:
> >>> - 'Reset the dll' is a platform specific call inside
> >>> 'arasan_zynqmp_execute_tuning' which is implemented in
> >>> sdhci-of-arasan.c
> >>> - 'arasan_zynqmp_execute_tuning' is called from
> 'sdhci_execute_tuning'
> >>> as a platform_execute_tuning routine
> >>> - So to keep 'DLL reset' routine called from sdhci-of-arasan.c, I
> >>> have implemented the execute_tuning in sdhci-of-arasan.c
> >>
> >> I meant something like:
> >>
> >> 	if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp-
> >> 8.9a"))
> >> 		host->mmc_host_ops.execute_tuning =
> arasan_zynqmp_execute_tuning;
> >>
> > This will need the removal of 'const' from 'static const struct
> mmc_host_ops sdhci_ops = {}' in sdhci.c file. Please confirm.
> 
> No, it is not const.  You are not looking at the right place. i.e.
> 
> $ grep mmc_host_ops drivers/mmc/host/sdhci.h
>         struct mmc_host_ops mmc_host_ops;       /* MMC host ops */
> 
[Manish] 
This looks fair. I got your point. I will send another patch for the same.
> >
> > Thanks,
> > Manish
> >>
> >> static int arasan_zynqmp_execute_tuning(struct mmc_host *mmc, u32
> >> opcode) {
> >> 	struct sdhci_host *host = mmc_priv(mmc);
> >> 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> 	struct sdhci_arasan_data *sdhci_arasan =
> >> sdhci_pltfm_priv(pltfm_host);
> >> 	int err;
> >>
> >> 	arasan_zynqmp_dll_reset(host, sdhci_arasan->device_id);
> >>
> >> 	err = sdhci_execute_tuning(mmc, opcode);
> >> 	if (err)
> >> 		return err;
> >>
> >> 	arasan_zynqmp_dll_reset(host, sdhci_arasan->device_id);
> >>
> >> 	return 0;
> >> }
> >>
> >>>
> >>> Alternative way (Please review):
> >>> - Define a host->quirk2 bit (SDHCI_QUIRK2_DLL_RESET_NEEDED) in
> >>> sdhci-of-arasan.c indicating DLL reset needed while tuning operation
> >>> - Call 'dll reset' routine before and after __sdhci_execute_tuning()
> >>> in sdhci.c when a host->quirk2 bit (SDHCI_QUIRK2_DLL_RESET_NEEDED)
> >>> is set
> >>
> >> We should try to avoid quirks.