Re: [EXT] Re: [PATCH v6 07/14] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

[Ziji Hu]

Hi Ulf,

On 2017/3/15 20:48, Ulf Hansson wrote:
> [...]
> 
>> +
>> +Example:
>> +- For eMMC:
>> +
>> +   sdhci@aa {
>> +   compatible = "marvell,armada-ap806-sdhci";
>> +   reg = <0xaa 0x1000>;
>> +   interrupts = 
>> +   clocks = <_clk>;
>> +   clock-names = "core";
>> +   bus-width = <4>;
>> +   marvell,xenon-phy-slow-mode;
>> +   marvell,xenon-tun-count = <11>;
> 
> There's no vmmc-supply here.
> 
> How do you control power to the eMMC card?
> 

Sorry for the delayed reply.
Just confirmed with the engineers.

It seems that there is a regulator.
I will ask for a complete node example.

>> +
>> +   #address-cells = <1>;
>> +   #size-cells = <0>;
>> +   apm_mmccard: mmccard@0 {
>> +   compatible = "mmc-card";
>> +   reg = <0>;
>> +   };
>> +   };
>> +
>> +- For SD/SDIO:
>> +
>> +   sdhci@ab {
>> +   compatible = "marvell,armada-cp110-sdhci";
>> +   reg = <0xab 0x1000>;
>> +   interrupts = 
>> +   vqmmc-supply = <_regulator>;
> 
> I guess you know vqmmc is for the I/O voltage.
> 
> Again, how do you power the SD/SDIO card? No vmmc?

The vmmc-supply regulator does exist according to the engineer.
I will ask them to provide a complete one.

> 
>> +   clocks = <>;
>> +   clock-names = "core";
>> +   bus-width = <4>;
>> +   marvell,xenon-tun-count = <9>;
>> +   };
>> +
>> +- For eMMC with compatible "marvell,armada-3700-sdhci":
>> +
>> +   sdhci@aa {
>> +   compatible = "marvell,armada-3700-sdhci";
>> +   reg = <0xaa 0x1000>,
>> + ;
>> +   interrupts = 
>> +   clocks = <>;
>> +   clock-names = "core";
>> +   bus-width = <8>;
>> +   mmc-ddr-1_8v;
>> +   mmc-hs400-1_8v;
> 
> Again, no vmmc?
> 

The engineer told me the power to eMMC card is fixed on this platform.
They don't implement a specific regulator for eMMC core power.

>> +
>> +   marvell,pad-type = "fixed-1-8v";
>> +
>> +   #address-cells = <1>;
>> +   #size-cells = <0>;
>> +   mmccard: mmccard@0 {
>> +   compatible = "mmc-card";
>> +   reg = <0>;
>> +   };
>> +   };
>> +
>> +- For SD/SDIO with compatible "marvell,armada-3700-sdhci":
>> +
>> +   sdhci@ab {
>> +   compatible = "marvell,armada-3700-sdhci";
>> +   reg = <0xab 0x1000>,
>> + ;
>> +   interrupts = 
>> +   vqmmc-supply = <_regulator>;
> 
> Again, no vmmc?
> 

It seems that the core power to SD is also fixed.

>> +   clocks = <>;
>> +   clock-names = "core";
>> +   bus-width = <4>;
>> +
>> +   marvell,pad-type = "sd";
>> +   };
>> --
>> git-series 0.9.1
> 
> 
> Kind regards
> Uffe
> 

Re: [EXT] Re: [PATCH v6 07/14] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

[Ziji Hu]

Hi Ulf,

On 2017/3/15 20:48, Ulf Hansson wrote:
> [...]
> 
>> +
>> +Example:
>> +- For eMMC:
>> +
>> +   sdhci@aa {
>> +   compatible = "marvell,armada-ap806-sdhci";
>> +   reg = <0xaa 0x1000>;
>> +   interrupts = 
>> +   clocks = <_clk>;
>> +   clock-names = "core";
>> +   bus-width = <4>;
>> +   marvell,xenon-phy-slow-mode;
>> +   marvell,xenon-tun-count = <11>;
> 
> There's no vmmc-supply here.
> 
> How do you control power to the eMMC card?
> 

Sorry for the delayed reply.
Just confirmed with the engineers.

It seems that there is a regulator.
I will ask for a complete node example.

>> +
>> +   #address-cells = <1>;
>> +   #size-cells = <0>;
>> +   apm_mmccard: mmccard@0 {
>> +   compatible = "mmc-card";
>> +   reg = <0>;
>> +   };
>> +   };
>> +
>> +- For SD/SDIO:
>> +
>> +   sdhci@ab {
>> +   compatible = "marvell,armada-cp110-sdhci";
>> +   reg = <0xab 0x1000>;
>> +   interrupts = 
>> +   vqmmc-supply = <_regulator>;
> 
> I guess you know vqmmc is for the I/O voltage.
> 
> Again, how do you power the SD/SDIO card? No vmmc?

The vmmc-supply regulator does exist according to the engineer.
I will ask them to provide a complete one.

> 
>> +   clocks = <>;
>> +   clock-names = "core";
>> +   bus-width = <4>;
>> +   marvell,xenon-tun-count = <9>;
>> +   };
>> +
>> +- For eMMC with compatible "marvell,armada-3700-sdhci":
>> +
>> +   sdhci@aa {
>> +   compatible = "marvell,armada-3700-sdhci";
>> +   reg = <0xaa 0x1000>,
>> + ;
>> +   interrupts = 
>> +   clocks = <>;
>> +   clock-names = "core";
>> +   bus-width = <8>;
>> +   mmc-ddr-1_8v;
>> +   mmc-hs400-1_8v;
> 
> Again, no vmmc?
> 

The engineer told me the power to eMMC card is fixed on this platform.
They don't implement a specific regulator for eMMC core power.

>> +
>> +   marvell,pad-type = "fixed-1-8v";
>> +
>> +   #address-cells = <1>;
>> +   #size-cells = <0>;
>> +   mmccard: mmccard@0 {
>> +   compatible = "mmc-card";
>> +   reg = <0>;
>> +   };
>> +   };
>> +
>> +- For SD/SDIO with compatible "marvell,armada-3700-sdhci":
>> +
>> +   sdhci@ab {
>> +   compatible = "marvell,armada-3700-sdhci";
>> +   reg = <0xab 0x1000>,
>> + ;
>> +   interrupts = 
>> +   vqmmc-supply = <_regulator>;
> 
> Again, no vmmc?
> 

It seems that the core power to SD is also fixed.

>> +   clocks = <>;
>> +   clock-names = "core";
>> +   bus-width = <4>;
>> +
>> +   marvell,pad-type = "sd";
>> +   };
>> --
>> git-series 0.9.1
> 
> 
> Kind regards
> Uffe
> 

Re: [PATCH v6 09/14] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2017/3/15 21:39, Ulf Hansson wrote:
> On 14 February 2017 at 18:01, Gregory CLEMENT
>  wrote:
>> From: Hu Ziji 
>>

>> +
>> +   /*
>> +* FIXME: should depends on the specific board timing.
>> +*/
>> +   if ((timing == MMC_TIMING_MMC_HS400) ||
>> +   (timing == MMC_TIMING_MMC_HS200) ||
>> +   (timing == MMC_TIMING_UHS_SDR50) ||
>> +   (timing == MMC_TIMING_UHS_SDR104) ||
>> +   (timing == MMC_TIMING_UHS_DDR50) ||
>> +   (timing == MMC_TIMING_UHS_SDR25) ||
>> +   (timing == MMC_TIMING_MMC_DDR52)) {
>> +   reg = sdhci_readl(host, phy_regs->timing_adj);
>> +   reg &= ~XENON_OUTPUT_QSN_PHASE_SELECT;
>> +   sdhci_writel(host, reg, phy_regs->timing_adj);
>> +   }
>> +

>> +
>> +   reg = sdhci_readl(host, phy_regs->func_ctrl);
>> +   if ((timing == MMC_TIMING_UHS_DDR50) ||
>> +   (timing == MMC_TIMING_MMC_HS400) ||
>> +   (timing == MMC_TIMING_MMC_DDR52))
>> +   reg |= (XENON_DQ_DDR_MODE_MASK << XENON_DQ_DDR_MODE_SHIFT) |
>> +  XENON_CMD_DDR_MODE;
>> +   else
>> +   reg &= ~((XENON_DQ_DDR_MODE_MASK << XENON_DQ_DDR_MODE_SHIFT) 
>> |
>> +XENON_CMD_DDR_MODE);

> 
> Again you are checking the MMC_TIMING_* in several if statements.
> Perhaps you can use switch statement instead and build a couple of
> different register variables, that you use when reading/writing
> afterwards.
> 

Sorry. I don't quite understand your comment.
Do you mean combine all the related  register settings
under a specific MMC_TIMING_*?

swhich (timing) {
case MMC_TIMING_HS400:
config reg_1;
config reg_2;
...
case MMC_TIMING_HS200:
config reg_2;
config reg_3
..
case ...
};

I have a little concern that there might be a lot
of duplicate register settings. Some PHY operations are
irrelevant to the speed mode. They have to be executed in
a particular sequence in all speed modes.

>> +}
>> +
>> +static int emmc_phy_parse_param_dt(struct sdhci_host *host,
>> +  struct device_node *np,
>> +  struct emmc_phy_params *params)
>> +{
>> +   u32 value;
>> +
>> +   if (of_property_read_bool(np, "marvell,xenon-phy-slow-mode"))
>> +   params->slow_mode = true;
>> +   else
>> +   params->slow_mode = false;
>> +
>> +   if (!of_property_read_u32(np, "marvell,xenon-phy-znr", ))
>> +   params->znr = value & XENON_ZNR_MASK;
>> +   else
>> +   params->znr = XENON_ZNR_DEF_VALUE;
>> +
>> +   if (!of_property_read_u32(np, "marvell,xenon-phy-zpr", ))
>> +   params->zpr = value & XENON_ZPR_MASK;
>> +   else
>> +   params->zpr = XENON_ZPR_DEF_VALUE;
>> +
>> +   if (!of_property_read_u32(np, "marvell,xenon-phy-nr-success-tun",
>> + ))
>> +   params->nr_tun_times = value & 
>> XENON_TUN_CONSECUTIVE_TIMES_MASK;
>> +   else
>> +   params->nr_tun_times = XENON_TUN_CONSECUTIVE_TIMES;
>> +
>> +   if (!of_property_read_u32(np, "marvell,xenon-phy-tun-step-divider",
>> + ))
>> +   params->tun_step_divider = value & 0xFF;
>> +   else
>> +   params->tun_step_divider = XENON_TUNING_STEP_DIVIDER;
>> +
>> +   return 0;
> 
> Instead of all these if-else, let's start by assigning default values
> instead. This makes the code more readable.
> 
Sure. I'll change it.

>> +}
>> +
>> +/*
>> + * Setting PHY when card is working in High Speed Mode.
>> + * HS400 set data strobe line.
>> + * HS200/SDR104 set tuning config to prepare for tuning.
>> + */
>> +static int xenon_hs_delay_adj(struct sdhci_host *host)
>> +{
>> +   int ret = 0;
>> +
>> +   if (WARN_ON(host->clock <= XENON_DEFAULT_SDCLK_FREQ))
>> +   return -EINVAL;
>> +
>> +   if (host->timing == MMC_TIMING_MMC_HS400) {
>> +   emmc_phy_strobe_delay_adj(host);
>> +   return 0;
>> +   }
>> +
>> +   if ((host->timing == MMC_TIMING_MMC_HS200) ||
>> +   (host->timing == MMC_TIMING_UHS_SDR104))
>> +   return emmc_phy_config_tuning(host);
>> +
>> +   /*
>> +* DDR Mode requires driver to scan Sampling Fixed Delay Line,
>> +* to find out a perfect operation sampling point.
>> +* It is hard to implement such a scan in host driver since 
>> initiating
>> +* commands by host driver is not safe.
>> +* Thus so far just keep PHY Sampling Fixed Delay in default value
>> +* in DDR mode.
>> +*
>> +* If any timing issue occurs in DDR mode on Marvell 

Re: [PATCH v6 09/14] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2017/3/15 21:39, Ulf Hansson wrote:
> On 14 February 2017 at 18:01, Gregory CLEMENT
>  wrote:
>> From: Hu Ziji 
>>

>> +
>> +   /*
>> +* FIXME: should depends on the specific board timing.
>> +*/
>> +   if ((timing == MMC_TIMING_MMC_HS400) ||
>> +   (timing == MMC_TIMING_MMC_HS200) ||
>> +   (timing == MMC_TIMING_UHS_SDR50) ||
>> +   (timing == MMC_TIMING_UHS_SDR104) ||
>> +   (timing == MMC_TIMING_UHS_DDR50) ||
>> +   (timing == MMC_TIMING_UHS_SDR25) ||
>> +   (timing == MMC_TIMING_MMC_DDR52)) {
>> +   reg = sdhci_readl(host, phy_regs->timing_adj);
>> +   reg &= ~XENON_OUTPUT_QSN_PHASE_SELECT;
>> +   sdhci_writel(host, reg, phy_regs->timing_adj);
>> +   }
>> +

>> +
>> +   reg = sdhci_readl(host, phy_regs->func_ctrl);
>> +   if ((timing == MMC_TIMING_UHS_DDR50) ||
>> +   (timing == MMC_TIMING_MMC_HS400) ||
>> +   (timing == MMC_TIMING_MMC_DDR52))
>> +   reg |= (XENON_DQ_DDR_MODE_MASK << XENON_DQ_DDR_MODE_SHIFT) |
>> +  XENON_CMD_DDR_MODE;
>> +   else
>> +   reg &= ~((XENON_DQ_DDR_MODE_MASK << XENON_DQ_DDR_MODE_SHIFT) 
>> |
>> +XENON_CMD_DDR_MODE);

> 
> Again you are checking the MMC_TIMING_* in several if statements.
> Perhaps you can use switch statement instead and build a couple of
> different register variables, that you use when reading/writing
> afterwards.
> 

Sorry. I don't quite understand your comment.
Do you mean combine all the related  register settings
under a specific MMC_TIMING_*?

swhich (timing) {
case MMC_TIMING_HS400:
config reg_1;
config reg_2;
...
case MMC_TIMING_HS200:
config reg_2;
config reg_3
..
case ...
};

I have a little concern that there might be a lot
of duplicate register settings. Some PHY operations are
irrelevant to the speed mode. They have to be executed in
a particular sequence in all speed modes.

>> +}
>> +
>> +static int emmc_phy_parse_param_dt(struct sdhci_host *host,
>> +  struct device_node *np,
>> +  struct emmc_phy_params *params)
>> +{
>> +   u32 value;
>> +
>> +   if (of_property_read_bool(np, "marvell,xenon-phy-slow-mode"))
>> +   params->slow_mode = true;
>> +   else
>> +   params->slow_mode = false;
>> +
>> +   if (!of_property_read_u32(np, "marvell,xenon-phy-znr", ))
>> +   params->znr = value & XENON_ZNR_MASK;
>> +   else
>> +   params->znr = XENON_ZNR_DEF_VALUE;
>> +
>> +   if (!of_property_read_u32(np, "marvell,xenon-phy-zpr", ))
>> +   params->zpr = value & XENON_ZPR_MASK;
>> +   else
>> +   params->zpr = XENON_ZPR_DEF_VALUE;
>> +
>> +   if (!of_property_read_u32(np, "marvell,xenon-phy-nr-success-tun",
>> + ))
>> +   params->nr_tun_times = value & 
>> XENON_TUN_CONSECUTIVE_TIMES_MASK;
>> +   else
>> +   params->nr_tun_times = XENON_TUN_CONSECUTIVE_TIMES;
>> +
>> +   if (!of_property_read_u32(np, "marvell,xenon-phy-tun-step-divider",
>> + ))
>> +   params->tun_step_divider = value & 0xFF;
>> +   else
>> +   params->tun_step_divider = XENON_TUNING_STEP_DIVIDER;
>> +
>> +   return 0;
> 
> Instead of all these if-else, let's start by assigning default values
> instead. This makes the code more readable.
> 
Sure. I'll change it.

>> +}
>> +
>> +/*
>> + * Setting PHY when card is working in High Speed Mode.
>> + * HS400 set data strobe line.
>> + * HS200/SDR104 set tuning config to prepare for tuning.
>> + */
>> +static int xenon_hs_delay_adj(struct sdhci_host *host)
>> +{
>> +   int ret = 0;
>> +
>> +   if (WARN_ON(host->clock <= XENON_DEFAULT_SDCLK_FREQ))
>> +   return -EINVAL;
>> +
>> +   if (host->timing == MMC_TIMING_MMC_HS400) {
>> +   emmc_phy_strobe_delay_adj(host);
>> +   return 0;
>> +   }
>> +
>> +   if ((host->timing == MMC_TIMING_MMC_HS200) ||
>> +   (host->timing == MMC_TIMING_UHS_SDR104))
>> +   return emmc_phy_config_tuning(host);
>> +
>> +   /*
>> +* DDR Mode requires driver to scan Sampling Fixed Delay Line,
>> +* to find out a perfect operation sampling point.
>> +* It is hard to implement such a scan in host driver since 
>> initiating
>> +* commands by host driver is not safe.
>> +* Thus so far just keep PHY Sampling Fixed Delay in default value
>> +* in DDR mode.
>> +*
>> +* If any timing issue occurs in DDR mode on Marvell products,
>> +* please contact maintainer to ask 

Re: [PATCH v6 08/14] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2017/3/15 21:11, Ulf Hansson wrote:
> On 14 February 2017 at 18:01, Gregory CLEMENT
>  wrote:
>> From: Hu Ziji 

>> +config MMC_SDHCI_XENON
>> +   tristate "Marvell Xenon eMMC/SD/SDIO SDHCI driver"
>> +   depends on MMC_SDHCI_PLTFM
>> +   help
>> + This selects Marvell Xenon eMMC/SD/SDIO SDHCI.
>> + If you have a machine with integrated Marvell Xenon SDHC IP,
> 
> /s/SDHC/SDHCI
> 

Sorry. I don't get your point.
Could you please give more detailed comments?

>> + say Y or M here.
>> + If unsure, say N.
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index ccc9c4cba154..b0a2ab4b256e 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -82,3 +82,6 @@ obj-$(CONFIG_MMC_SDHCI_BRCMSTB)   += 
>> sdhci-brcmstb.o
>>  ifeq ($(CONFIG_CB710_DEBUG),y)
>> CFLAGS-cb710-mmc+= -DDEBUG
>>  endif
>> +
>> +obj-$(CONFIG_MMC_SDHCI_XENON)  += sdhci-xenon-driver.o
>> +sdhci-xenon-driver-y   += sdhci-xenon.o
> 
> Why not only this:
> obj-$(CONFIG_MMC_SDHCI_XENON) += sdhci-xenon.o
> 

Yes. Will try.

>> diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
>> new file mode 100644

> 
> ff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
>> new file mode 100644
>> index ..69de711db9eb
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon.h
> 
> You should probably put all this in the c-file instead. That is how
> most other sdhci variants does it.
> 

Unfortunately, we have some registers like XENON_SLOT_EMMC_CTRL
which are accessed by both sdhci-xenon.c and sdhci-xenon-phy.c

> [...]
> 
> Overall this looks good to me, however I think Adrian needs to have a
> quick look as well.
> 
> One additional very minor nitpick. Perhaps you can align on the
> function names prefix, as those currently varies between "whatever",
> "xenon_" and "sdhci_xenon_".
> 
Sure. Will fix them as you request.

Thank you.

Best regards,
Hu Ziji

> Kind regards
> Uffe
> 

Re: [PATCH v6 08/14] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2017/3/15 21:11, Ulf Hansson wrote:
> On 14 February 2017 at 18:01, Gregory CLEMENT
>  wrote:
>> From: Hu Ziji 

>> +config MMC_SDHCI_XENON
>> +   tristate "Marvell Xenon eMMC/SD/SDIO SDHCI driver"
>> +   depends on MMC_SDHCI_PLTFM
>> +   help
>> + This selects Marvell Xenon eMMC/SD/SDIO SDHCI.
>> + If you have a machine with integrated Marvell Xenon SDHC IP,
> 
> /s/SDHC/SDHCI
> 

Sorry. I don't get your point.
Could you please give more detailed comments?

>> + say Y or M here.
>> + If unsure, say N.
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index ccc9c4cba154..b0a2ab4b256e 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -82,3 +82,6 @@ obj-$(CONFIG_MMC_SDHCI_BRCMSTB)   += 
>> sdhci-brcmstb.o
>>  ifeq ($(CONFIG_CB710_DEBUG),y)
>> CFLAGS-cb710-mmc+= -DDEBUG
>>  endif
>> +
>> +obj-$(CONFIG_MMC_SDHCI_XENON)  += sdhci-xenon-driver.o
>> +sdhci-xenon-driver-y   += sdhci-xenon.o
> 
> Why not only this:
> obj-$(CONFIG_MMC_SDHCI_XENON) += sdhci-xenon.o
> 

Yes. Will try.

>> diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
>> new file mode 100644

> 
> ff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
>> new file mode 100644
>> index ..69de711db9eb
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon.h
> 
> You should probably put all this in the c-file instead. That is how
> most other sdhci variants does it.
> 

Unfortunately, we have some registers like XENON_SLOT_EMMC_CTRL
which are accessed by both sdhci-xenon.c and sdhci-xenon-phy.c

> [...]
> 
> Overall this looks good to me, however I think Adrian needs to have a
> quick look as well.
> 
> One additional very minor nitpick. Perhaps you can align on the
> function names prefix, as those currently varies between "whatever",
> "xenon_" and "sdhci_xenon_".
> 
Sure. Will fix them as you request.

Thank you.

Best regards,
Hu Ziji

> Kind regards
> Uffe
> 

Re: [PATCH v6 03/14] mmc: core: Add mmc-card dt sub-node parse in core layer

[Ziji Hu]

Hi Ulf,

On 2017/3/15 20:43, Ulf Hansson wrote:
> On 14 February 2017 at 18:01, Gregory CLEMENT
>  wrote:
>> From: Hu Ziji 
>>
>> Some vendor host, like Xenon, can support multiple types.
>> In dts, use mmc-card dt sub-node to indicate eMMC is in use.
>>
>> Add a generic mmc-card parse function in mmc core layer.
>> If mmc-card sub-node is detected, set eMMC common caps, such as
>> MMC_CAP_NONREMOVABLE, MMC_CAP2_NO_SD and MMC_CAP2_NO_SDIO.
>>
>> Since it is likely that struct mmc_card is not allocated yet when
>> this mmc-card parse function is called, it will return true if
>> mmc-card sub-node is detected. Otherwise, return false.
>> It can help vendor host determine if the card type is eMMC.
>>

>> +bool mmc_of_parse_mmc_card(struct mmc_host *host)
>> +{
>> +   struct device_node *np;
>> +   bool ret = false;
>> +
>> +   np = mmc_of_find_child_device(host, 0);
> 
> There are already some places in the mmc core where child nodes are
> being parsed. You may have a look for mmc_of_find_child_device() and
> find the callers of it.
> 
> Additionally, we need a generic method of how to describe in DT, when
> an mmc host controller has more than one mmc slot. This will also be
> done using child nodes.
> 

I'd like to confirm if you require a universal child node
parsing function to deal with all the child node use cases.
It might be a little difficult to us.

> So, before starting hacking on this, it seems like we need some
> consolidation of the code. In principle, I would like to move the APIs
> for parsing of the child nodes into host.c, along with existing
> mmc_of_parse() function.
> 

I guess that your requirement is like:
int mmc_of_pars(struct mmc_host *host)
{

...

np = mmc_of_find_child_device(host, 0);
if (np && of_device_is_compatible(np, "mmc-card")) {
...
}

...

}
Am I correct?

Actually, I previously tried this implementation.
But I found that it is difficult to return eMMC card type information
to our vendor driver. Our Xenon driver expects to know if the device
is eMMC card through this parse.

>> +   if (np && of_device_is_compatible(np, "mmc-card")) {
>> +   /* mmc-card sub-node indicates eMMC card is in use. */
>> +   host->caps |= MMC_CAP_NONREMOVABLE;
>> +   host->caps2 |= MMC_CAP2_NO_SDIO | MMC_CAP2_NO_SD;
>> +   ret = true;
>> +   }
>> +
> 
> So instead of providing a new mmc OF parse API, let's make
> mmc_of_parse() call another internal function which deals with child
> node parsing.
> 
>> +   of_node_put(np);
>> +   return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(mmc_of_parse_mmc_card);
>> +
>> +/*
>>   * Starting point for MMC card init.
>>   */
>>  int mmc_attach_mmc(struct mmc_host *host)
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>> index e33cc748dcfe..1b27323f3b6e 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -234,4 +234,6 @@ struct device_node;
>>  extern u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
>>  extern int mmc_of_parse_voltage(struct device_node *np, u32 *mask);
>>
>> +extern bool mmc_of_parse_mmc_card(struct mmc_host *host);
>> +
>>  #endif /* LINUX_MMC_CORE_H */
>> --
>> git-series 0.9.1
> 
> If you didn't quite understand my comments, then please tell me. Then
> I can help out cooking some patches for you.

Thanks a lot. Actually I'm not sure if our implementation to detect
eMMC card type can meet your requirement of child node parsing.
Please help provide me some examples or patches.

Thank you.

Best regards,
Hu Ziji

> 
> Kind regards
> Uffe
> 

Re: [PATCH v6 03/14] mmc: core: Add mmc-card dt sub-node parse in core layer

[Ziji Hu]

Hi Ulf,

On 2017/3/15 20:43, Ulf Hansson wrote:
> On 14 February 2017 at 18:01, Gregory CLEMENT
>  wrote:
>> From: Hu Ziji 
>>
>> Some vendor host, like Xenon, can support multiple types.
>> In dts, use mmc-card dt sub-node to indicate eMMC is in use.
>>
>> Add a generic mmc-card parse function in mmc core layer.
>> If mmc-card sub-node is detected, set eMMC common caps, such as
>> MMC_CAP_NONREMOVABLE, MMC_CAP2_NO_SD and MMC_CAP2_NO_SDIO.
>>
>> Since it is likely that struct mmc_card is not allocated yet when
>> this mmc-card parse function is called, it will return true if
>> mmc-card sub-node is detected. Otherwise, return false.
>> It can help vendor host determine if the card type is eMMC.
>>

>> +bool mmc_of_parse_mmc_card(struct mmc_host *host)
>> +{
>> +   struct device_node *np;
>> +   bool ret = false;
>> +
>> +   np = mmc_of_find_child_device(host, 0);
> 
> There are already some places in the mmc core where child nodes are
> being parsed. You may have a look for mmc_of_find_child_device() and
> find the callers of it.
> 
> Additionally, we need a generic method of how to describe in DT, when
> an mmc host controller has more than one mmc slot. This will also be
> done using child nodes.
> 

I'd like to confirm if you require a universal child node
parsing function to deal with all the child node use cases.
It might be a little difficult to us.

> So, before starting hacking on this, it seems like we need some
> consolidation of the code. In principle, I would like to move the APIs
> for parsing of the child nodes into host.c, along with existing
> mmc_of_parse() function.
> 

I guess that your requirement is like:
int mmc_of_pars(struct mmc_host *host)
{

...

np = mmc_of_find_child_device(host, 0);
if (np && of_device_is_compatible(np, "mmc-card")) {
...
}

...

}
Am I correct?

Actually, I previously tried this implementation.
But I found that it is difficult to return eMMC card type information
to our vendor driver. Our Xenon driver expects to know if the device
is eMMC card through this parse.

>> +   if (np && of_device_is_compatible(np, "mmc-card")) {
>> +   /* mmc-card sub-node indicates eMMC card is in use. */
>> +   host->caps |= MMC_CAP_NONREMOVABLE;
>> +   host->caps2 |= MMC_CAP2_NO_SDIO | MMC_CAP2_NO_SD;
>> +   ret = true;
>> +   }
>> +
> 
> So instead of providing a new mmc OF parse API, let's make
> mmc_of_parse() call another internal function which deals with child
> node parsing.
> 
>> +   of_node_put(np);
>> +   return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(mmc_of_parse_mmc_card);
>> +
>> +/*
>>   * Starting point for MMC card init.
>>   */
>>  int mmc_attach_mmc(struct mmc_host *host)
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>> index e33cc748dcfe..1b27323f3b6e 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -234,4 +234,6 @@ struct device_node;
>>  extern u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
>>  extern int mmc_of_parse_voltage(struct device_node *np, u32 *mask);
>>
>> +extern bool mmc_of_parse_mmc_card(struct mmc_host *host);
>> +
>>  #endif /* LINUX_MMC_CORE_H */
>> --
>> git-series 0.9.1
> 
> If you didn't quite understand my comments, then please tell me. Then
> I can help out cooking some patches for you.

Thanks a lot. Actually I'm not sure if our implementation to detect
eMMC card type can meet your requirement of child node parsing.
Please help provide me some examples or patches.

Thank you.

Best regards,
Hu Ziji

> 
> Kind regards
> Uffe
> 

Re: [PATCH v6 00/14] mmc: Add support to Marvell Xenon SD Host Controller

[Ziji Hu]

Hi Ulf,

On 2017/2/21 3:32, Ulf Hansson wrote:
> On 20 February 2017 at 17:59, Gregory CLEMENT
>  wrote:
>> Hi Ulf,
>>
>>  On mar., févr. 14 2017, Gregory CLEMENT 
>>  wrote:
>>
>>> Hello,
>>>
>>> This the sixth version of the series adding support for the SDHCI
>>> Xenon controller. It can be currently found on the Armada 37xx and the
>>> Armada 7K/8K but will be also used in more Marvell SoC (and not only
>>> the mvebu ones actually).
>>>
>>> Most of the following changes had been discussed on the mailing list,
>>> except the new compatible string.
>>>
>>
>> I think that with this version we addressed all the concerns expressed
>> about the previous version. Do you think that you will be able to apply
>> this series for 4.11 or do you still see any issues?
>>
>> Thanks,
>>
>> Gregory
> 
> We are in the merge window for v4.11, so new material is out of the
> question. I am preparing the PR to Linus as this very moment.
> 
> Anyway, I will look into the series as soon as I can.

Could you please take a look at our patch set?
Very sorry for pushing you, but your comment is very important to us.

Thank you.

Best regards,
Hu Ziji

> 
> Kind regards
> Uffe
> 

Re: [PATCH v6 00/14] mmc: Add support to Marvell Xenon SD Host Controller

[Ziji Hu]

Hi Ulf,

On 2017/2/21 3:32, Ulf Hansson wrote:
> On 20 February 2017 at 17:59, Gregory CLEMENT
>  wrote:
>> Hi Ulf,
>>
>>  On mar., févr. 14 2017, Gregory CLEMENT 
>>  wrote:
>>
>>> Hello,
>>>
>>> This the sixth version of the series adding support for the SDHCI
>>> Xenon controller. It can be currently found on the Armada 37xx and the
>>> Armada 7K/8K but will be also used in more Marvell SoC (and not only
>>> the mvebu ones actually).
>>>
>>> Most of the following changes had been discussed on the mailing list,
>>> except the new compatible string.
>>>
>>
>> I think that with this version we addressed all the concerns expressed
>> about the previous version. Do you think that you will be able to apply
>> this series for 4.11 or do you still see any issues?
>>
>> Thanks,
>>
>> Gregory
> 
> We are in the merge window for v4.11, so new material is out of the
> question. I am preparing the PR to Linus as this very moment.
> 
> Anyway, I will look into the series as soon as I can.

Could you please take a look at our patch set?
Very sorry for pushing you, but your comment is very important to us.

Thank you.

Best regards,
Hu Ziji

> 
> Kind regards
> Uffe
> 

Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2017/1/30 16:41, Ulf Hansson wrote:
> [...]
> 
 + */
 +static int xenon_child_node_of_parse(struct platform_device *pdev)
 +{
 +   struct device_node *np = pdev->dev.of_node;
 +   struct sdhci_host *host = platform_get_drvdata(pdev);
 +   struct mmc_host *mmc = host->mmc;
 +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
 +   struct device_node *child;
 +   int nr_child;
 +
 +   priv->init_card_type = XENON_CARD_TYPE_UNKNOWN;
 +
 +   nr_child = of_get_child_count(np);
 +   if (!nr_child)
 +   return 0;
 +
 +   for_each_child_of_node(np, child) {
 +   if (of_device_is_compatible(child, "mmc-card")) {
>>>
>>> To avoid code from being duplicated, I would rather see the DT parsing
>>> of the child nodes for "mmc-card", to be done by the mmc core.
>>>
>>> As a matter of fact it is already being done, but perhaps we need to
>>> change that a bit as to fit your case.
>>>
>>> I suggest you have a look and see how to update this in the core,
>>> instead of doing it here in the host driver.
>>>
>>
>> I understand your concern.
>>
>> It seems that so far "mmc-card" is only used in our Xenon driver.
> 
> git grep "mmc-card" tells you more about where it's being parsed by
> the mmc core.
> 
>> Besides, we set Xenon specific parameters and attributions when
>> parsing "mmc-card" property.
> 
> I don't see any Xenon specific properties.
> 
> Instead I think it would make sense to update the generic
> interpretation of the binding for mmc-card, as you have done here.
> 

   OK. I will merge it into core layer.
 
   Our Xenon driver requires to determine whether current SDHC is
   for eMMC before card init. Thus I would like to implement a specific
   function for mmc-card parsing in mmc.c and let mmc_of_parse() in host.c
   to call it.
   But there are some detailed issues then.
   1. mmc_decode_ext_csd() also parses mmc-card to check broken-hpi.
  So mmc-card parse will still be duplicated.
  Shall we merge broken-hpi check into mmc-card parse?
  It might require a new flag to indicate broken-hpi attribution in
  mmc_host structure.

   2. Structure mmc_card is not ready while parsing mmc-card.
  Thus we are not able to indicate card type in mmc_card.
  As a result, our Xenon specific parse function still has to
  parse mmc-card again to check if eMMC card is in used.

   Could you please help suggest any better place in core layer to
   parse mmc-card?

   Thank you.

Best regards,
Hu Ziji

>>
>> May I keep current implementation?
> 
> Rather not. Let's try to make the parsing of the child node for
> mmc-card generic.
> 
>> In my very own opinion, moving it into core layer should be another
>> independent patch.
> 
> Absolutely an independent patch. Whether it can be done as a part of
> mmc_of_parse() or whether we need yet another new mmc_of* API, we can
> discuss.
> 
> My point is that, I don't this to be specific for Xenon (unless there
> are specific reason, which I don't see here).
> 
>> And it will also cost some more time. To be honest, it is difficult
>> for me to bring up a generic core layer implementation to parse
>> "mmc-card", since I'm not clear about other vendor's requirement.
> 
> Well, then you need to learn more about how the mmc core works. In
> this case, it shouldn't be too hard to implement.
> 
> [...]
> 
>>
>>>
 + MMC_CAP2_NO_SD |
 + MMC_CAP2_NO_SDIO;
>>>
>>> I think we need to update the DT documentation for mmc-card.
>>> Typically, we should explicitly state what kind of other existing mmc
>>> DT properties that will be automatically selected, when the mmc-card
>>> is specified.
>>>
>>> Can you please look into updating this DT doc as well.
>>>
>>
>>   Similar to above, may I keep it now and bring up another patch later
>>   to update mmc-card DT and parsing?
> 
> Please, no.
> 
> [...]
> 
> Kind regards
> Uffe
> 

Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2017/1/30 16:41, Ulf Hansson wrote:
> [...]
> 
 + */
 +static int xenon_child_node_of_parse(struct platform_device *pdev)
 +{
 +   struct device_node *np = pdev->dev.of_node;
 +   struct sdhci_host *host = platform_get_drvdata(pdev);
 +   struct mmc_host *mmc = host->mmc;
 +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
 +   struct device_node *child;
 +   int nr_child;
 +
 +   priv->init_card_type = XENON_CARD_TYPE_UNKNOWN;
 +
 +   nr_child = of_get_child_count(np);
 +   if (!nr_child)
 +   return 0;
 +
 +   for_each_child_of_node(np, child) {
 +   if (of_device_is_compatible(child, "mmc-card")) {
>>>
>>> To avoid code from being duplicated, I would rather see the DT parsing
>>> of the child nodes for "mmc-card", to be done by the mmc core.
>>>
>>> As a matter of fact it is already being done, but perhaps we need to
>>> change that a bit as to fit your case.
>>>
>>> I suggest you have a look and see how to update this in the core,
>>> instead of doing it here in the host driver.
>>>
>>
>> I understand your concern.
>>
>> It seems that so far "mmc-card" is only used in our Xenon driver.
> 
> git grep "mmc-card" tells you more about where it's being parsed by
> the mmc core.
> 
>> Besides, we set Xenon specific parameters and attributions when
>> parsing "mmc-card" property.
> 
> I don't see any Xenon specific properties.
> 
> Instead I think it would make sense to update the generic
> interpretation of the binding for mmc-card, as you have done here.
> 

   OK. I will merge it into core layer.
 
   Our Xenon driver requires to determine whether current SDHC is
   for eMMC before card init. Thus I would like to implement a specific
   function for mmc-card parsing in mmc.c and let mmc_of_parse() in host.c
   to call it.
   But there are some detailed issues then.
   1. mmc_decode_ext_csd() also parses mmc-card to check broken-hpi.
  So mmc-card parse will still be duplicated.
  Shall we merge broken-hpi check into mmc-card parse?
  It might require a new flag to indicate broken-hpi attribution in
  mmc_host structure.

   2. Structure mmc_card is not ready while parsing mmc-card.
  Thus we are not able to indicate card type in mmc_card.
  As a result, our Xenon specific parse function still has to
  parse mmc-card again to check if eMMC card is in used.

   Could you please help suggest any better place in core layer to
   parse mmc-card?

   Thank you.

Best regards,
Hu Ziji

>>
>> May I keep current implementation?
> 
> Rather not. Let's try to make the parsing of the child node for
> mmc-card generic.
> 
>> In my very own opinion, moving it into core layer should be another
>> independent patch.
> 
> Absolutely an independent patch. Whether it can be done as a part of
> mmc_of_parse() or whether we need yet another new mmc_of* API, we can
> discuss.
> 
> My point is that, I don't this to be specific for Xenon (unless there
> are specific reason, which I don't see here).
> 
>> And it will also cost some more time. To be honest, it is difficult
>> for me to bring up a generic core layer implementation to parse
>> "mmc-card", since I'm not clear about other vendor's requirement.
> 
> Well, then you need to learn more about how the mmc core works. In
> this case, it shouldn't be too hard to implement.
> 
> [...]
> 
>>
>>>
 + MMC_CAP2_NO_SD |
 + MMC_CAP2_NO_SDIO;
>>>
>>> I think we need to update the DT documentation for mmc-card.
>>> Typically, we should explicitly state what kind of other existing mmc
>>> DT properties that will be automatically selected, when the mmc-card
>>> is specified.
>>>
>>> Can you please look into updating this DT doc as well.
>>>
>>
>>   Similar to above, may I keep it now and bring up another patch later
>>   to update mmc-card DT and parsing?
> 
> Please, no.
> 
> [...]
> 
> Kind regards
> Uffe
> 

Re: [PATCH v5 07/12] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC.

[Ziji Hu]

Hi Ulf,

On 2017/1/26 19:29, Ulf Hansson wrote:
> [...]
> 
>> +
>> +static inline bool temp_stage_hs200_to_hs400(struct sdhci_host *host,
>> +struct sdhci_xenon_priv *priv)
>> +{
>> +   /*
>> +* Tmep stages from HS200 to HS400
>> +* from HS200 to HS in 200MHz
>> +* from 200MHz to 52MHz
>> +*/
>> +   if (((priv->timing == MMC_TIMING_MMC_HS200) &&
>> +(host->timing == MMC_TIMING_MMC_HS)) ||
>> +   ((host->timing == MMC_TIMING_MMC_HS) &&
>> +(priv->clock > host->clock)))
>> +   return true;
>> +
>> +   return false;
>> +}
>> +
>> +static inline bool temp_stage_hs400_to_h200(struct sdhci_host *host,
>> +   struct sdhci_xenon_priv *priv)
>> +{
>> +   /*
>> +* Temp stages from HS400 t0 HS200:
>> +* from 200MHz to 52MHz in HS400
>> +* from HS400 to HS DDR in 52MHz
>> +* from HS DDR to HS in 52MHz
>> +* from HS to HS200 in 52MHz
>> +*/
>> +   if (((priv->timing == MMC_TIMING_MMC_HS400) &&
>> +((host->clock == MMC_HIGH_52_MAX_DTR) ||
>> + (host->timing == MMC_TIMING_MMC_DDR52))) ||
>> +   ((priv->timing == MMC_TIMING_MMC_DDR52) &&
>> +(host->timing == MMC_TIMING_MMC_HS)) ||
>> +   ((host->timing == MMC_TIMING_MMC_HS200) &&
>> +(host->clock == MMC_HIGH_52_MAX_DTR)))
>> +   return true;
>> +
>> +   return false;
>> +}
> 
> Both the above functions seems to be really fragile to use. If
> anything changes in the core layer related to these sequences, you may
> end up getting a wrong validated expression.
> 
> Perhaps an option would be to add one or two new mmc host ops, which
> could be called during the related hs200/400 sequences that could make
> this less fragile for you? What do you think?
> 

Yes, they both look fragile to use.
In my own opinion, hs200->hs400 sequence might be safe since it has been
defined and fixed by eMMC spec. I personally think it is unlikely to be
adjusted again. Please correct me if I am wrong.
However, as you said, hs400->hs200 sequence will have issue if it is
changed in core layer, since such a sequence is not from eMMC spec.

But, in my opinion, adding additional mmc host ops might not be helpful.
The above two functions are part of Xenon PHY adjustment. Those two
functions help save some PHY setting steps in hs200->hs400/hs400->hs200
sequence. But other PHY settings are still necessary,
which should be executed each time when mmc core layer set ios.
Neither of them can be extracted out and put into another mmc host ops.

Instead, it will helps us improve the above two if we can add a flag to
indicate that eMMC is in a temporary status in hs200->hs400 or hs400->hs200
sequence.

>> +
>> +/*
>> + * If eMMC PHY Slow Mode is required in lower speed mode in SDR mode
>> + * (SDLCK < 55MHz), enable Slow Mode to bypass eMMC PHY.
>> + * SDIO slower SDR mode also requires Slow Mode.
>> + *
>> + * If Slow Mode is enabled, return true.
>> + * Otherwise, return false.
>> + */
>> +static bool emmc_phy_slow_mode(struct sdhci_host *host,
>> +  unsigned char timing)
>> +{
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +   struct emmc_phy_params *params = priv->phy_params;
>> +   struct xenon_emmc_phy_regs *phy_regs = priv->emmc_phy_regs;
>> +   u32 reg;
>> +
>> +   /* Skip temp stages from HS200 to HS400 */
>> +   if (temp_stage_hs200_to_hs400(host, priv))
>> +   return false;
>> +
>> +   /* Skip temp stages from HS400 t0 HS200 */
>> +   if (temp_stage_hs400_to_h200(host, priv))
>> +   return false;
>> +
>> +   reg = sdhci_readl(host, phy_regs->timing_adj);
>> +   /* Enable Slow Mode for SDIO in slower SDR mode */
>> +   if ((priv->init_card_type == MMC_TYPE_SDIO) &&
> 
> Ah, noticed that there is some specific things going on here for SDIO
> here as well. So perhaps you do need to implement the ->init_card()
> ops anyway. Which I suggested not to, for patch6. :-)
> 
> Although, there is one problem with using ->init_card(). That is
> particularly for removable cards, as mmc_rescan() doesn't invoke
> ->init_card() when it removes a card, which means, you may use old and
> thus wrong information about what kind of card in the next card
> detection attempt. So perhaps ->init_card() needs to be called from
> the core before it even figured out what kind of card it is trying to
> detect, as to allow the callbacks to reset some data.
> 
> Or perhaps you can think of another clever way!?
>

It is a good question.

Our Xenon requires to set SDIO Mode and PHY Slow Mode for SDIO card.
Both of them are configed in function emmc_phy_set(). In 

Re: [PATCH v5 07/12] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC.

[Ziji Hu]

Hi Ulf,

On 2017/1/26 19:29, Ulf Hansson wrote:
> [...]
> 
>> +
>> +static inline bool temp_stage_hs200_to_hs400(struct sdhci_host *host,
>> +struct sdhci_xenon_priv *priv)
>> +{
>> +   /*
>> +* Tmep stages from HS200 to HS400
>> +* from HS200 to HS in 200MHz
>> +* from 200MHz to 52MHz
>> +*/
>> +   if (((priv->timing == MMC_TIMING_MMC_HS200) &&
>> +(host->timing == MMC_TIMING_MMC_HS)) ||
>> +   ((host->timing == MMC_TIMING_MMC_HS) &&
>> +(priv->clock > host->clock)))
>> +   return true;
>> +
>> +   return false;
>> +}
>> +
>> +static inline bool temp_stage_hs400_to_h200(struct sdhci_host *host,
>> +   struct sdhci_xenon_priv *priv)
>> +{
>> +   /*
>> +* Temp stages from HS400 t0 HS200:
>> +* from 200MHz to 52MHz in HS400
>> +* from HS400 to HS DDR in 52MHz
>> +* from HS DDR to HS in 52MHz
>> +* from HS to HS200 in 52MHz
>> +*/
>> +   if (((priv->timing == MMC_TIMING_MMC_HS400) &&
>> +((host->clock == MMC_HIGH_52_MAX_DTR) ||
>> + (host->timing == MMC_TIMING_MMC_DDR52))) ||
>> +   ((priv->timing == MMC_TIMING_MMC_DDR52) &&
>> +(host->timing == MMC_TIMING_MMC_HS)) ||
>> +   ((host->timing == MMC_TIMING_MMC_HS200) &&
>> +(host->clock == MMC_HIGH_52_MAX_DTR)))
>> +   return true;
>> +
>> +   return false;
>> +}
> 
> Both the above functions seems to be really fragile to use. If
> anything changes in the core layer related to these sequences, you may
> end up getting a wrong validated expression.
> 
> Perhaps an option would be to add one or two new mmc host ops, which
> could be called during the related hs200/400 sequences that could make
> this less fragile for you? What do you think?
> 

Yes, they both look fragile to use.
In my own opinion, hs200->hs400 sequence might be safe since it has been
defined and fixed by eMMC spec. I personally think it is unlikely to be
adjusted again. Please correct me if I am wrong.
However, as you said, hs400->hs200 sequence will have issue if it is
changed in core layer, since such a sequence is not from eMMC spec.

But, in my opinion, adding additional mmc host ops might not be helpful.
The above two functions are part of Xenon PHY adjustment. Those two
functions help save some PHY setting steps in hs200->hs400/hs400->hs200
sequence. But other PHY settings are still necessary,
which should be executed each time when mmc core layer set ios.
Neither of them can be extracted out and put into another mmc host ops.

Instead, it will helps us improve the above two if we can add a flag to
indicate that eMMC is in a temporary status in hs200->hs400 or hs400->hs200
sequence.

>> +
>> +/*
>> + * If eMMC PHY Slow Mode is required in lower speed mode in SDR mode
>> + * (SDLCK < 55MHz), enable Slow Mode to bypass eMMC PHY.
>> + * SDIO slower SDR mode also requires Slow Mode.
>> + *
>> + * If Slow Mode is enabled, return true.
>> + * Otherwise, return false.
>> + */
>> +static bool emmc_phy_slow_mode(struct sdhci_host *host,
>> +  unsigned char timing)
>> +{
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +   struct emmc_phy_params *params = priv->phy_params;
>> +   struct xenon_emmc_phy_regs *phy_regs = priv->emmc_phy_regs;
>> +   u32 reg;
>> +
>> +   /* Skip temp stages from HS200 to HS400 */
>> +   if (temp_stage_hs200_to_hs400(host, priv))
>> +   return false;
>> +
>> +   /* Skip temp stages from HS400 t0 HS200 */
>> +   if (temp_stage_hs400_to_h200(host, priv))
>> +   return false;
>> +
>> +   reg = sdhci_readl(host, phy_regs->timing_adj);
>> +   /* Enable Slow Mode for SDIO in slower SDR mode */
>> +   if ((priv->init_card_type == MMC_TYPE_SDIO) &&
> 
> Ah, noticed that there is some specific things going on here for SDIO
> here as well. So perhaps you do need to implement the ->init_card()
> ops anyway. Which I suggested not to, for patch6. :-)
> 
> Although, there is one problem with using ->init_card(). That is
> particularly for removable cards, as mmc_rescan() doesn't invoke
> ->init_card() when it removes a card, which means, you may use old and
> thus wrong information about what kind of card in the next card
> detection attempt. So perhaps ->init_card() needs to be called from
> the core before it even figured out what kind of card it is trying to
> detect, as to allow the callbacks to reset some data.
> 
> Or perhaps you can think of another clever way!?
>

It is a good question.

Our Xenon requires to set SDIO Mode and PHY Slow Mode for SDIO card.
Both of them are configed in function emmc_phy_set(). In 

Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2017/1/26 18:50, Ulf Hansson wrote:
> On 11 January 2017 at 18:19, Gregory CLEMENT
>  wrote:
>> From: Hu Ziji 
>>
>> Add Xenon eMMC/SD/SDIO host controller core functionality.
>> Add Xenon specific intialization process.
>> Add Xenon specific mmc_host_ops APIs.
>> Add Xenon specific register definitions.
>>
>> Add CONFIG_MMC_SDHCI_XENON support in drivers/mmc/host/Kconfig.
>>
>> Marvell Xenon SDHC conforms to SD Physical Layer Specification
>> Version 3.01 and is designed according to the guidelines provided
>> in the SD Host Controller Standard Specification Version 3.00.
>>
>> Signed-off-by: Hu Ziji 
>> Tested-by: Russell King 
>> Signed-off-by: Gregory CLEMENT 
>> ---
>>  drivers/mmc/host/Kconfig   |   9 +-
>>  drivers/mmc/host/Makefile  |   3 +-
>>  drivers/mmc/host/sdhci-xenon.c | 631 ++-
>>  drivers/mmc/host/sdhci-xenon.h |  70 -
>>  4 files changed, 713 insertions(+)
>>  create mode 100644 drivers/mmc/host/sdhci-xenon.c
>>  create mode 100644 drivers/mmc/host/sdhci-xenon.h
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 2eb97014dc3f..8d2d08de14a0 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -819,3 +819,12 @@ config MMC_SDHCI_BRCMSTB
>>   Broadcom STB SoCs.
>>
>>   If unsure, say Y.
>> +
>> +config MMC_SDHCI_XENON
>> +   tristate "Marvell Xenon eMMC/SD/SDIO SDHCI driver"
>> +   depends on MMC_SDHCI && MMC_SDHCI_PLTFM
> 
> Depending on MMC_SDHCI_PLTFM is enough.
> 

Thanks a lot for your review and detailed suggestions.
They are really helpful.

I will simplify the dependence to MMC_SDHCI_PLTFM only.

> [...]
> 
>> +
>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>> +struct mmc_ios *ios)
>> +{
>> +   struct sdhci_host *host = mmc_priv(mmc);
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +   /*
>> +* Before SD/SDIO set signal voltage, SD bus clock should be
>> +* disabled. However, sdhci_set_clock will also disable the Internal
>> +* clock in mmc_set_signal_voltage().
>> +* If Internal clock is disabled, the 3.3V/1.8V bit can not be 
>> updated.
>> +* Thus here manually enable internal clock.
>> +*
>> +* After switch completes, it is unnecessary to disable internal 
>> clock,
>> +* since keeping internal clock active obeys SD spec.
>> +*/
>> +   enable_xenon_internal_clk(host);
>> +
>> +   if (priv->init_card_type == MMC_TYPE_MMC)
> 
> So, you need a specific voltage switch for eMMC.
> 
> For that, I don't think you need to implement ->init_card(), as to set
> priv->init_card_type and then check it here.
> 
> Instead you should be able to find out whether the card is an eMMC,
> only by the parsing of the child node for the "mmc-card" compatible.
> More about this below.
> 

I would like to discuss about ->init_card() implementation in
my reply to our next patch ([PATCH v5 07/12] mmc: sdhci-xenon:
Add support to PHYs of Marvell Xenon SDHC).

>> +   return xenon_emmc_signal_voltage_switch(mmc, ios);
>> +
>> +   return sdhci_start_signal_voltage_switch(mmc, ios);
>> +}
>> +
>> +/*
> 
> [...]
> 
>> + */
>> +static int xenon_child_node_of_parse(struct platform_device *pdev)
>> +{
>> +   struct device_node *np = pdev->dev.of_node;
>> +   struct sdhci_host *host = platform_get_drvdata(pdev);
>> +   struct mmc_host *mmc = host->mmc;
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +   struct device_node *child;
>> +   int nr_child;
>> +
>> +   priv->init_card_type = XENON_CARD_TYPE_UNKNOWN;
>> +
>> +   nr_child = of_get_child_count(np);
>> +   if (!nr_child)
>> +   return 0;
>> +
>> +   for_each_child_of_node(np, child) {
>> +   if (of_device_is_compatible(child, "mmc-card")) {
> 
> To avoid code from being duplicated, I would rather see the DT parsing
> of the child nodes for "mmc-card", to be done by the mmc core.
> 
> As a matter of fact it is already being done, but perhaps we need to
> change that a bit as to fit your case.
> 
> I suggest you have a look and see how to update this in the core,
> instead of doing it here in the host driver.
> 

I understand your concern.

It seems that so far "mmc-card" is only used in our Xenon driver.
Besides, we set Xenon specific parameters and attributions when
parsing "mmc-card" property.

May I keep current implementation?
In my very own opinion, moving it into core layer should be another
independent patch.

Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2017/1/26 18:50, Ulf Hansson wrote:
> On 11 January 2017 at 18:19, Gregory CLEMENT
>  wrote:
>> From: Hu Ziji 
>>
>> Add Xenon eMMC/SD/SDIO host controller core functionality.
>> Add Xenon specific intialization process.
>> Add Xenon specific mmc_host_ops APIs.
>> Add Xenon specific register definitions.
>>
>> Add CONFIG_MMC_SDHCI_XENON support in drivers/mmc/host/Kconfig.
>>
>> Marvell Xenon SDHC conforms to SD Physical Layer Specification
>> Version 3.01 and is designed according to the guidelines provided
>> in the SD Host Controller Standard Specification Version 3.00.
>>
>> Signed-off-by: Hu Ziji 
>> Tested-by: Russell King 
>> Signed-off-by: Gregory CLEMENT 
>> ---
>>  drivers/mmc/host/Kconfig   |   9 +-
>>  drivers/mmc/host/Makefile  |   3 +-
>>  drivers/mmc/host/sdhci-xenon.c | 631 ++-
>>  drivers/mmc/host/sdhci-xenon.h |  70 -
>>  4 files changed, 713 insertions(+)
>>  create mode 100644 drivers/mmc/host/sdhci-xenon.c
>>  create mode 100644 drivers/mmc/host/sdhci-xenon.h
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 2eb97014dc3f..8d2d08de14a0 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -819,3 +819,12 @@ config MMC_SDHCI_BRCMSTB
>>   Broadcom STB SoCs.
>>
>>   If unsure, say Y.
>> +
>> +config MMC_SDHCI_XENON
>> +   tristate "Marvell Xenon eMMC/SD/SDIO SDHCI driver"
>> +   depends on MMC_SDHCI && MMC_SDHCI_PLTFM
> 
> Depending on MMC_SDHCI_PLTFM is enough.
> 

Thanks a lot for your review and detailed suggestions.
They are really helpful.

I will simplify the dependence to MMC_SDHCI_PLTFM only.

> [...]
> 
>> +
>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>> +struct mmc_ios *ios)
>> +{
>> +   struct sdhci_host *host = mmc_priv(mmc);
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +   /*
>> +* Before SD/SDIO set signal voltage, SD bus clock should be
>> +* disabled. However, sdhci_set_clock will also disable the Internal
>> +* clock in mmc_set_signal_voltage().
>> +* If Internal clock is disabled, the 3.3V/1.8V bit can not be 
>> updated.
>> +* Thus here manually enable internal clock.
>> +*
>> +* After switch completes, it is unnecessary to disable internal 
>> clock,
>> +* since keeping internal clock active obeys SD spec.
>> +*/
>> +   enable_xenon_internal_clk(host);
>> +
>> +   if (priv->init_card_type == MMC_TYPE_MMC)
> 
> So, you need a specific voltage switch for eMMC.
> 
> For that, I don't think you need to implement ->init_card(), as to set
> priv->init_card_type and then check it here.
> 
> Instead you should be able to find out whether the card is an eMMC,
> only by the parsing of the child node for the "mmc-card" compatible.
> More about this below.
> 

I would like to discuss about ->init_card() implementation in
my reply to our next patch ([PATCH v5 07/12] mmc: sdhci-xenon:
Add support to PHYs of Marvell Xenon SDHC).

>> +   return xenon_emmc_signal_voltage_switch(mmc, ios);
>> +
>> +   return sdhci_start_signal_voltage_switch(mmc, ios);
>> +}
>> +
>> +/*
> 
> [...]
> 
>> + */
>> +static int xenon_child_node_of_parse(struct platform_device *pdev)
>> +{
>> +   struct device_node *np = pdev->dev.of_node;
>> +   struct sdhci_host *host = platform_get_drvdata(pdev);
>> +   struct mmc_host *mmc = host->mmc;
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +   struct device_node *child;
>> +   int nr_child;
>> +
>> +   priv->init_card_type = XENON_CARD_TYPE_UNKNOWN;
>> +
>> +   nr_child = of_get_child_count(np);
>> +   if (!nr_child)
>> +   return 0;
>> +
>> +   for_each_child_of_node(np, child) {
>> +   if (of_device_is_compatible(child, "mmc-card")) {
> 
> To avoid code from being duplicated, I would rather see the DT parsing
> of the child nodes for "mmc-card", to be done by the mmc core.
> 
> As a matter of fact it is already being done, but perhaps we need to
> change that a bit as to fit your case.
> 
> I suggest you have a look and see how to update this in the core,
> instead of doing it here in the host driver.
> 

I understand your concern.

It seems that so far "mmc-card" is only used in our Xenon driver.
Besides, we set Xenon specific parameters and attributions when
parsing "mmc-card" property.

May I keep current implementation?
In my very own opinion, moving it into core layer should be another
independent patch.
And it will also cost some more time. To be honest, it is difficult
for me to bring up a generic core layer implementation to parse

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/29 19:11, Ulf Hansson wrote:
> [...]
> 
>

Sorry that I didn't make myself clear.

Our host PHY delay line consists of hundreds of sampling points.
Each sampling point represents a different phase shift.

In lower speed mode, our host driver will scan the delay line.
It will select and test multiple sampling points, other than testing
only single sampling point.

If a sampling point fails to transfer cmd/data, our host driver will
move to test next sampling point, until we find out a group of 
 successful
sampling points which can transfer cmd/data. At last we will select
a perfect one from them.
>>>
>>> Ahh, I see. Unfortunate, this is going to be very hard to implement 
>>> properly.
>>>
>>> The main problem is that the host driver has *no* knowledge about the
>>> internal state of the card, as that is the responsibility of the mmc
>>> core to keep track of.
>>>
>>> If the host driver would send a command during every update of the
>>> "ios" setting, from ->set_ios(), for sure it would lead to commands
>>> being sent that are "forbidden" in the current internal state of the
>>> card.
>>> This would lead to that the card initialization sequence fails,
>>> because the card may move to an unknown internal state and the mmc
>>> core would have no knowledge about what happened.
>>>
>>
>>Yes. In theory, host layer should not initiate a command by itself.
>>
>>We assume that bus is idle and card is stable in Tran state, when core 
>> layer
>>asks host to switch "ios".
> 
> Understand, but this is a wrong assumption. The card may very well in
> another state than Tran state.
> 

   Could you please provide an example that card might not be in Tran state?
   It seems that card should be in Tran state after CMD6 succeed.
   If CMD6 fails, mmc driver will not execute ios setting. Thus ->set_ios()
   will not be called.

>>Besides, we only select the commands which is valid in the whole 
>> procedure,
>>such as CMD8 for eMMC.
>>Those test commands are actually like read operations to card registers.
>>The card will return to Tran state even if transfer fails. It is also easy
>>for host to recover.
> 
> For example, I would recommend you to investigate in detail the
> sequence for when a CMD6 command is sent to the card.
> The host must *not* start sending commands from ->set_ios() during a
> CMD6 sequence. For example a CMD8 is not allowed.
> 
> Moreover, due to this, I wonder if it is even possible to get this HW
> to work properly.
> 

   In my very own opinion, ->set_ios() is only executed after CMD6 sequence
   succeeds, based on current mmc.c/sd.c/sdio.c.
   I personally think that it should not interfere CMD6 sequence.

   I'm afraid that HW cannot help and SW driver has to take care of this.

>>
>>> Hmm..
>>>
>>> Can you specify, *exactly*, under which "ios updates" you need to
>>> verify updated PHY setting changes by sending a cmd/data? Also, please
>>> specify if it's enough to only test the CMD line or also DATA lines.
>>>
>>
>>When one of the three parameters in below changes, our host driver needs
>>to adjust PHY in lower speed mode.
>>1. Speed Mode (timing): like legacy mode --> HS DDR
>>2. Bus Clock: like 400KHz --> 50MHz
>>3. Bus Width: like 1-bit --> 4-bit/8-bit
>>
>>For eMMC, we use CMD8 to test sampling point.
>>For SD, we use CMD13.
>>For SDIO, currently CMD52 is used to read a register from CCCR.
>>Those commands in above are all valid during the whole procedure to switch
>>to high speed mode from legacy mode.
>>
>>It is the best case if the test command can transfer both on CMD and DAT 
>> lines.
>>CMD8 for eMMC can test both CMD line and DAT lines. CMD13 and CMD52 only 
>> test
>>CMD line. We might use ACMD51 for SD and CMD53 for SDIO later thus DAT 
>> lines
>>are also under test.
> 
> Thanks for sharing these details!
> 
> So, if possible, I would recommend you to discuss these issues with
> some of the HW designers. Perhaps you can figure out an alternative
> method of confirming/testing PHY setting changes? Sending commands to
> the card just doesn't work well for all cases.
> 

   Thanks a lot for you patience.

   Actually, we, including HW engineers, have been working on this for
   a very long time. We also test a lot on many actual products. It is
   quiet stable in real use scenarios.

   I know it is still not good enough. It seems to be impossible to find
   another practical and reliable solution, based on our tests.
   Could you please provide some suggestions thus we can try our best to 
improve it
   to meet your requirement?

   Thank you.

Best regards,
Hu Ziji

> Kind regards
> Uffe
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/29 19:11, Ulf Hansson wrote:
> [...]
> 
>

Sorry that I didn't make myself clear.

Our host PHY delay line consists of hundreds of sampling points.
Each sampling point represents a different phase shift.

In lower speed mode, our host driver will scan the delay line.
It will select and test multiple sampling points, other than testing
only single sampling point.

If a sampling point fails to transfer cmd/data, our host driver will
move to test next sampling point, until we find out a group of 
 successful
sampling points which can transfer cmd/data. At last we will select
a perfect one from them.
>>>
>>> Ahh, I see. Unfortunate, this is going to be very hard to implement 
>>> properly.
>>>
>>> The main problem is that the host driver has *no* knowledge about the
>>> internal state of the card, as that is the responsibility of the mmc
>>> core to keep track of.
>>>
>>> If the host driver would send a command during every update of the
>>> "ios" setting, from ->set_ios(), for sure it would lead to commands
>>> being sent that are "forbidden" in the current internal state of the
>>> card.
>>> This would lead to that the card initialization sequence fails,
>>> because the card may move to an unknown internal state and the mmc
>>> core would have no knowledge about what happened.
>>>
>>
>>Yes. In theory, host layer should not initiate a command by itself.
>>
>>We assume that bus is idle and card is stable in Tran state, when core 
>> layer
>>asks host to switch "ios".
> 
> Understand, but this is a wrong assumption. The card may very well in
> another state than Tran state.
> 

   Could you please provide an example that card might not be in Tran state?
   It seems that card should be in Tran state after CMD6 succeed.
   If CMD6 fails, mmc driver will not execute ios setting. Thus ->set_ios()
   will not be called.

>>Besides, we only select the commands which is valid in the whole 
>> procedure,
>>such as CMD8 for eMMC.
>>Those test commands are actually like read operations to card registers.
>>The card will return to Tran state even if transfer fails. It is also easy
>>for host to recover.
> 
> For example, I would recommend you to investigate in detail the
> sequence for when a CMD6 command is sent to the card.
> The host must *not* start sending commands from ->set_ios() during a
> CMD6 sequence. For example a CMD8 is not allowed.
> 
> Moreover, due to this, I wonder if it is even possible to get this HW
> to work properly.
> 

   In my very own opinion, ->set_ios() is only executed after CMD6 sequence
   succeeds, based on current mmc.c/sd.c/sdio.c.
   I personally think that it should not interfere CMD6 sequence.

   I'm afraid that HW cannot help and SW driver has to take care of this.

>>
>>> Hmm..
>>>
>>> Can you specify, *exactly*, under which "ios updates" you need to
>>> verify updated PHY setting changes by sending a cmd/data? Also, please
>>> specify if it's enough to only test the CMD line or also DATA lines.
>>>
>>
>>When one of the three parameters in below changes, our host driver needs
>>to adjust PHY in lower speed mode.
>>1. Speed Mode (timing): like legacy mode --> HS DDR
>>2. Bus Clock: like 400KHz --> 50MHz
>>3. Bus Width: like 1-bit --> 4-bit/8-bit
>>
>>For eMMC, we use CMD8 to test sampling point.
>>For SD, we use CMD13.
>>For SDIO, currently CMD52 is used to read a register from CCCR.
>>Those commands in above are all valid during the whole procedure to switch
>>to high speed mode from legacy mode.
>>
>>It is the best case if the test command can transfer both on CMD and DAT 
>> lines.
>>CMD8 for eMMC can test both CMD line and DAT lines. CMD13 and CMD52 only 
>> test
>>CMD line. We might use ACMD51 for SD and CMD53 for SDIO later thus DAT 
>> lines
>>are also under test.
> 
> Thanks for sharing these details!
> 
> So, if possible, I would recommend you to discuss these issues with
> some of the HW designers. Perhaps you can figure out an alternative
> method of confirming/testing PHY setting changes? Sending commands to
> the card just doesn't work well for all cases.
> 

   Thanks a lot for you patience.

   Actually, we, including HW engineers, have been working on this for
   a very long time. We also test a lot on many actual products. It is
   quiet stable in real use scenarios.

   I know it is still not good enough. It seems to be impossible to find
   another practical and reliable solution, based on our tests.
   Could you please provide some suggestions thus we can try our best to 
improve it
   to meet your requirement?

   Thank you.

Best regards,
Hu Ziji

> Kind regards
> Uffe
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/29 15:49, Ulf Hansson wrote:
> On 29 November 2016 at 03:53, Ziji Hu <huz...@marvell.com> wrote:
>> Hi Ulf,
>>
>> On 2016/11/28 23:16, Ulf Hansson wrote:
>>> On 28 November 2016 at 12:38, Ziji Hu <huz...@marvell.com> wrote:
>>>> Hi Ulf,
>>>>
>>>> On 2016/11/28 19:13, Ulf Hansson wrote:
>>>>>>
>>>>>> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), 
>>>>>> to
>>>>>> send commands for testing current sampling point set in our host PHY.
>>>>>>
>>>>>> According to my test result, it shows that mmc_send_tuning() can 
>>>>>> only support
>>>>>> tuning command (CMD21/CMD19).
>>>>>> As a result, we cannot use mmc_send_tuning() when card is in the 
>>>>>> speed modes
>>>>>> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
>>>>>> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in 
>>>>>> those
>>>>>> speed modes.
>>>>>>
>>>>>> Could you please provide suggestions for the speed mode in which 
>>>>>> tuning is
>>>>>> not available?
>>>>>>
>>>>>
>>>>> Normally the mmc host driver shouldn't have to care about what the
>>>>> card supports, as that is the responsibility of the mmc core to
>>>>> manage.
>>>>>
>>>>> The host should only need to implement the ->execute_tuning() ops,
>>>>> which gets called when the card supports tuning (CMD19/21). Does it
>>>>> make sense?
>>>>>
>>>>I think it is irrelevant to tuning procedure.
>>>>
>>>>Our host requires to adjust PHY setting after each time ios setting
>>>>(SDCLK/bus width/speed mode) is changed.
>>>>The simplified sequence is:
>>>>mmc change ios --> mmc_set_ios() --> ->set_ios() --> after 
>>>> sdhci_set_ios(),
>>>>adjust PHY setting.
>>>>During PHY setting adjustment, out host driver has to send commands to
>>>>test current sampling point. Tuning is another independent step.
>>>
>>> For those speed modes (or other ios changes) that *don't* requires
>>> tuning, then what will you do when you send the command to confirm the
>>> change of PHY setting and it fails?
>>>
>>> My assumption is that you will fail anyway, by propagating the error
>>> to the mmc core. At least that what was my understanding from your
>>> earlier replies, right!?
>>>
>>> Then, I think there are no point having the host driver sending a
>>> command to confirm the PHY settings, as the mmc core will anyway
>>> discover if something goes wrong when the next command is sent.
>>>
>>> Please correct me if I am wrong!
>>>
>>
>>Sorry that I didn't make myself clear.
>>
>>Our host PHY delay line consists of hundreds of sampling points.
>>Each sampling point represents a different phase shift.
>>
>>In lower speed mode, our host driver will scan the delay line.
>>It will select and test multiple sampling points, other than testing
>>only single sampling point.
>>
>>If a sampling point fails to transfer cmd/data, our host driver will
>>move to test next sampling point, until we find out a group of successful
>>sampling points which can transfer cmd/data. At last we will select
>>a perfect one from them.
> 
> Ahh, I see. Unfortunate, this is going to be very hard to implement properly.
> 
> The main problem is that the host driver has *no* knowledge about the
> internal state of the card, as that is the responsibility of the mmc
> core to keep track of.
> 
> If the host driver would send a command during every update of the
> "ios" setting, from ->set_ios(), for sure it would lead to commands
> being sent that are "forbidden" in the current internal state of the
> card.
> This would lead to that the card initialization sequence fails,
> because the card may move to an unknown internal state and the mmc
> core would have no knowledge about what happened.
> 

   Yes. In theory, host layer should not initiate a command by itself.

   We assume that bus is idle and card is stable in Tran state, when core layer
   asks host to switch "ios".
   Besides, we only sele

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/29 15:49, Ulf Hansson wrote:
> On 29 November 2016 at 03:53, Ziji Hu  wrote:
>> Hi Ulf,
>>
>> On 2016/11/28 23:16, Ulf Hansson wrote:
>>> On 28 November 2016 at 12:38, Ziji Hu  wrote:
>>>> Hi Ulf,
>>>>
>>>> On 2016/11/28 19:13, Ulf Hansson wrote:
>>>>>>
>>>>>> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), 
>>>>>> to
>>>>>> send commands for testing current sampling point set in our host PHY.
>>>>>>
>>>>>> According to my test result, it shows that mmc_send_tuning() can 
>>>>>> only support
>>>>>> tuning command (CMD21/CMD19).
>>>>>> As a result, we cannot use mmc_send_tuning() when card is in the 
>>>>>> speed modes
>>>>>> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
>>>>>> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in 
>>>>>> those
>>>>>> speed modes.
>>>>>>
>>>>>> Could you please provide suggestions for the speed mode in which 
>>>>>> tuning is
>>>>>> not available?
>>>>>>
>>>>>
>>>>> Normally the mmc host driver shouldn't have to care about what the
>>>>> card supports, as that is the responsibility of the mmc core to
>>>>> manage.
>>>>>
>>>>> The host should only need to implement the ->execute_tuning() ops,
>>>>> which gets called when the card supports tuning (CMD19/21). Does it
>>>>> make sense?
>>>>>
>>>>I think it is irrelevant to tuning procedure.
>>>>
>>>>Our host requires to adjust PHY setting after each time ios setting
>>>>(SDCLK/bus width/speed mode) is changed.
>>>>The simplified sequence is:
>>>>mmc change ios --> mmc_set_ios() --> ->set_ios() --> after 
>>>> sdhci_set_ios(),
>>>>adjust PHY setting.
>>>>During PHY setting adjustment, out host driver has to send commands to
>>>>test current sampling point. Tuning is another independent step.
>>>
>>> For those speed modes (or other ios changes) that *don't* requires
>>> tuning, then what will you do when you send the command to confirm the
>>> change of PHY setting and it fails?
>>>
>>> My assumption is that you will fail anyway, by propagating the error
>>> to the mmc core. At least that what was my understanding from your
>>> earlier replies, right!?
>>>
>>> Then, I think there are no point having the host driver sending a
>>> command to confirm the PHY settings, as the mmc core will anyway
>>> discover if something goes wrong when the next command is sent.
>>>
>>> Please correct me if I am wrong!
>>>
>>
>>Sorry that I didn't make myself clear.
>>
>>Our host PHY delay line consists of hundreds of sampling points.
>>Each sampling point represents a different phase shift.
>>
>>In lower speed mode, our host driver will scan the delay line.
>>It will select and test multiple sampling points, other than testing
>>only single sampling point.
>>
>>If a sampling point fails to transfer cmd/data, our host driver will
>>move to test next sampling point, until we find out a group of successful
>>sampling points which can transfer cmd/data. At last we will select
>>a perfect one from them.
> 
> Ahh, I see. Unfortunate, this is going to be very hard to implement properly.
> 
> The main problem is that the host driver has *no* knowledge about the
> internal state of the card, as that is the responsibility of the mmc
> core to keep track of.
> 
> If the host driver would send a command during every update of the
> "ios" setting, from ->set_ios(), for sure it would lead to commands
> being sent that are "forbidden" in the current internal state of the
> card.
> This would lead to that the card initialization sequence fails,
> because the card may move to an unknown internal state and the mmc
> core would have no knowledge about what happened.
> 

   Yes. In theory, host layer should not initiate a command by itself.

   We assume that bus is idle and card is stable in Tran state, when core layer
   asks host to switch "ios".
   Besides, we only select the commands which is valid in the whole proce

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/28 23:16, Ulf Hansson wrote:
> On 28 November 2016 at 12:38, Ziji Hu <huz...@marvell.com> wrote:
>> Hi Ulf,
>>
>> On 2016/11/28 19:13, Ulf Hansson wrote:
>>>>
>>>> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
>>>> send commands for testing current sampling point set in our host PHY.
>>>>
>>>> According to my test result, it shows that mmc_send_tuning() can only 
>>>> support
>>>> tuning command (CMD21/CMD19).
>>>> As a result, we cannot use mmc_send_tuning() when card is in the speed 
>>>> modes
>>>> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
>>>> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in 
>>>> those
>>>> speed modes.
>>>>
>>>> Could you please provide suggestions for the speed mode in which 
>>>> tuning is
>>>> not available?
>>>>
>>>
>>> Normally the mmc host driver shouldn't have to care about what the
>>> card supports, as that is the responsibility of the mmc core to
>>> manage.
>>>
>>> The host should only need to implement the ->execute_tuning() ops,
>>> which gets called when the card supports tuning (CMD19/21). Does it
>>> make sense?
>>>
>>I think it is irrelevant to tuning procedure.
>>
>>Our host requires to adjust PHY setting after each time ios setting
>>(SDCLK/bus width/speed mode) is changed.
>>The simplified sequence is:
>>mmc change ios --> mmc_set_ios() --> ->set_ios() --> after 
>> sdhci_set_ios(),
>>adjust PHY setting.
>>During PHY setting adjustment, out host driver has to send commands to
>>test current sampling point. Tuning is another independent step.
> 
> For those speed modes (or other ios changes) that *don't* requires
> tuning, then what will you do when you send the command to confirm the
> change of PHY setting and it fails?
> 
> My assumption is that you will fail anyway, by propagating the error
> to the mmc core. At least that what was my understanding from your
> earlier replies, right!?
> 
> Then, I think there are no point having the host driver sending a
> command to confirm the PHY settings, as the mmc core will anyway
> discover if something goes wrong when the next command is sent.
> 
> Please correct me if I am wrong!
>

   Sorry that I didn't make myself clear.

   Our host PHY delay line consists of hundreds of sampling points.
   Each sampling point represents a different phase shift.

   In lower speed mode, our host driver will scan the delay line.
   It will select and test multiple sampling points, other than testing
   only single sampling point.

   If a sampling point fails to transfer cmd/data, our host driver will
   move to test next sampling point, until we find out a group of successful
   sampling points which can transfer cmd/data. At last we will select
   a perfect one from them.

   Thank you.

Best regards,
Hu Ziji
 
>>
>>Thus our host needs a valid command in PHY setting adjustment. Tuning 
>> command
>>can be borrowed to complete this task in SD SDR50. But for other speed 
>> mode,
>>we have to find out a valid command.
> 
> I thought we agreed on this wasn't necessary? Please see my upper response.
> 
> Kind regards
> Uffe
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/28 23:16, Ulf Hansson wrote:
> On 28 November 2016 at 12:38, Ziji Hu  wrote:
>> Hi Ulf,
>>
>> On 2016/11/28 19:13, Ulf Hansson wrote:
>>>>
>>>> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
>>>> send commands for testing current sampling point set in our host PHY.
>>>>
>>>> According to my test result, it shows that mmc_send_tuning() can only 
>>>> support
>>>> tuning command (CMD21/CMD19).
>>>> As a result, we cannot use mmc_send_tuning() when card is in the speed 
>>>> modes
>>>> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
>>>> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in 
>>>> those
>>>> speed modes.
>>>>
>>>> Could you please provide suggestions for the speed mode in which 
>>>> tuning is
>>>> not available?
>>>>
>>>
>>> Normally the mmc host driver shouldn't have to care about what the
>>> card supports, as that is the responsibility of the mmc core to
>>> manage.
>>>
>>> The host should only need to implement the ->execute_tuning() ops,
>>> which gets called when the card supports tuning (CMD19/21). Does it
>>> make sense?
>>>
>>I think it is irrelevant to tuning procedure.
>>
>>Our host requires to adjust PHY setting after each time ios setting
>>(SDCLK/bus width/speed mode) is changed.
>>The simplified sequence is:
>>mmc change ios --> mmc_set_ios() --> ->set_ios() --> after 
>> sdhci_set_ios(),
>>adjust PHY setting.
>>During PHY setting adjustment, out host driver has to send commands to
>>test current sampling point. Tuning is another independent step.
> 
> For those speed modes (or other ios changes) that *don't* requires
> tuning, then what will you do when you send the command to confirm the
> change of PHY setting and it fails?
> 
> My assumption is that you will fail anyway, by propagating the error
> to the mmc core. At least that what was my understanding from your
> earlier replies, right!?
> 
> Then, I think there are no point having the host driver sending a
> command to confirm the PHY settings, as the mmc core will anyway
> discover if something goes wrong when the next command is sent.
> 
> Please correct me if I am wrong!
>

   Sorry that I didn't make myself clear.

   Our host PHY delay line consists of hundreds of sampling points.
   Each sampling point represents a different phase shift.

   In lower speed mode, our host driver will scan the delay line.
   It will select and test multiple sampling points, other than testing
   only single sampling point.

   If a sampling point fails to transfer cmd/data, our host driver will
   move to test next sampling point, until we find out a group of successful
   sampling points which can transfer cmd/data. At last we will select
   a perfect one from them.

   Thank you.

Best regards,
Hu Ziji
 
>>
>>Thus our host needs a valid command in PHY setting adjustment. Tuning 
>> command
>>can be borrowed to complete this task in SD SDR50. But for other speed 
>> mode,
>>we have to find out a valid command.
> 
> I thought we agreed on this wasn't necessary? Please see my upper response.
> 
> Kind regards
> Uffe
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/28 19:13, Ulf Hansson wrote:
>>
>> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
>> send commands for testing current sampling point set in our host PHY.
>>
>> According to my test result, it shows that mmc_send_tuning() can only 
>> support
>> tuning command (CMD21/CMD19).
>> As a result, we cannot use mmc_send_tuning() when card is in the speed 
>> modes
>> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
>> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in those
>> speed modes.
>>
>> Could you please provide suggestions for the speed mode in which tuning 
>> is
>> not available?
>>
> 
> Normally the mmc host driver shouldn't have to care about what the
> card supports, as that is the responsibility of the mmc core to
> manage.
> 
> The host should only need to implement the ->execute_tuning() ops,
> which gets called when the card supports tuning (CMD19/21). Does it
> make sense?
> 
   I think it is irrelevant to tuning procedure.

   Our host requires to adjust PHY setting after each time ios setting
   (SDCLK/bus width/speed mode) is changed.
   The simplified sequence is:
   mmc change ios --> mmc_set_ios() --> ->set_ios() --> after sdhci_set_ios(),
   adjust PHY setting.
   During PHY setting adjustment, out host driver has to send commands to
   test current sampling point. Tuning is another independent step.

   Thus our host needs a valid command in PHY setting adjustment. Tuning command
   can be borrowed to complete this task in SD SDR50. But for other speed mode,
   we have to find out a valid command.

   Any suggestion please?

   Thank you.

Best regards,
Hu Ziji

> Kind regards
> Uffe
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/28 19:13, Ulf Hansson wrote:
>>
>> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
>> send commands for testing current sampling point set in our host PHY.
>>
>> According to my test result, it shows that mmc_send_tuning() can only 
>> support
>> tuning command (CMD21/CMD19).
>> As a result, we cannot use mmc_send_tuning() when card is in the speed 
>> modes
>> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
>> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in those
>> speed modes.
>>
>> Could you please provide suggestions for the speed mode in which tuning 
>> is
>> not available?
>>
> 
> Normally the mmc host driver shouldn't have to care about what the
> card supports, as that is the responsibility of the mmc core to
> manage.
> 
> The host should only need to implement the ->execute_tuning() ops,
> which gets called when the card supports tuning (CMD19/21). Does it
> make sense?
> 
   I think it is irrelevant to tuning procedure.

   Our host requires to adjust PHY setting after each time ios setting
   (SDCLK/bus width/speed mode) is changed.
   The simplified sequence is:
   mmc change ios --> mmc_set_ios() --> ->set_ios() --> after sdhci_set_ios(),
   adjust PHY setting.
   During PHY setting adjustment, out host driver has to send commands to
   test current sampling point. Tuning is another independent step.

   Thus our host needs a valid command in PHY setting adjustment. Tuning command
   can be borrowed to complete this task in SD SDR50. But for other speed mode,
   we have to find out a valid command.

   Any suggestion please?

   Thank you.

Best regards,
Hu Ziji

> Kind regards
> Uffe
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/24 23:37, Ziji Hu wrote:
> Hi Ulf,
> 
> On 2016/11/24 22:33, Ulf Hansson wrote:

>>>
>>>As result, our SDHC driver has to implement the functionality to
>>>send commands and check the results, in host layer.
>>>If directly calling mmc_wait_for_cmd() is improper, could you please
>>>give us some suggestions?
>>>
>>>For eMMC, CMD8 is used to test current sampling point set in PHY.
>>
>> Try to use mmc_send_tuning().
>>
> 
> Could you please tell me the requirement of "op_code" parameter in
> mmc_send_tuning()?
> According to mmc_send_tuning(),it seems that tuning command(CMD19/CMD21)
> is required. Thus device will not response mmc_send_tuning() if current
> speed mode doesn't support tuning command.
> Please correct me if I am wrong.
>

As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
send commands for testing current sampling point set in our host PHY.

According to my test result, it shows that mmc_send_tuning() can only 
support
tuning command (CMD21/CMD19).
As a result, we cannot use mmc_send_tuning() when card is in the speed modes
which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in those
speed modes.

Could you please provide suggestions for the speed mode in which tuning is
not available?

Thank you.

Best regards,
Hu Ziji

>>>
>>>>> +
>>>>> +   return err;
>>>>> +}
>>>>> +
>>>>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>>>>> +{
>>>>> +   struct mmc_command cmd = {0};
>>>>> +   int err;
>>>>> +
>>>>> +   cmd.opcode = SD_IO_RW_DIRECT;
>>>>> +   cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>>>>> +
>>>>> +   err = mmc_wait_for_cmd(card->host, , 0);
>>>>> +   if (err)
>>>>> +   return err;
>>>>> +
>>>>> +   if (cmd.resp[0] & R5_ERROR)
>>>>> +   return -EIO;
>>>>> +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>>>>> +   return -EINVAL;
>>>>> +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
>>>>> +   return -ERANGE;
>>>>> +   return 0;
>>>>
>>>> No thanks! MMC/SD/SDIO protocol code belongs in the core.
>>>>
>>>For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
>>>in PHY.
>>>Please help provide some suggestion to implement the command transfer.
>>
>> Again, I think mmc_send_tuning() should be possible for you to use.
>>
>> [...]
>>
>>>>> +   if (mmc->card)
>>>>> +   card = mmc->card;
>>>>> +   else
>>>>> +   /*
>>>>> +* Only valid during initialization
>>>>> +* before mmc->card is set
>>>>> +*/
>>>>> +   card = priv->card_candidate;
>>>>> +   if (unlikely(!card)) {
>>>>> +   dev_warn(mmc_dev(mmc), "card is not present\n");
>>>>> +   return -EINVAL;
>>>>> +   }
>>>>
>>>> That your host need to hold a copy of the card pointer, tells me that
>>>> something is not really correct.
>>>>
>>>> I might be wrong, if this turns out to be a special case, but I doubt
>>>> it. Although, if it *is* a special such case, we shall most likely try
>>>> to extend the the mmc core layer instead of adding all these hacks in
>>>> your host driver.
>>>>
>>> This card pointer copies the temporary structure mmc_card
>>> used in mmc_init_card(), mmc_sd_init_card() and mmc_sdio_init_card().
>>> Since we call mmc_wait_for_cmd() to send test commands, we need a copy
>>> of that temporary mmc_card here in our host driver.
>>
>> I see, thanks for clarifying.
>>
>>>
>>> During PHY setting in card initialization, mmc_host->card is not updated
>>> yet with that temporary mmc_card. Thus we are not able to directly use
>>> mmc_host->card. Instead, this card pointer is introduced to enable
>>> mmc_wait_for_cmd().
>>>
>>> If we can improve our host driver to send test commands without 
>>> mmc_card,
>>> this card pointer can be removed.
>>> Could you please share your opinion please?
>>
>> The mmc_send_tuning() API takes the mmc_host as parameter. If you
>> convert to that, perhaps you would be able to remove the need to hold
>> the card pointer.
>>
>> BTW, the reason why mmc_send_tuning() doesn't take the card as a
>> parameter, is exactly those you just described above.
>>
>Got it.
>Thanks a lot for the information.
> 
>Thank you for the great help.
> 
> Best regards,
> Hu Ziji
> 
>> [...]
>>
>> Kind regards
>> Uffe
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/24 23:37, Ziji Hu wrote:
> Hi Ulf,
> 
> On 2016/11/24 22:33, Ulf Hansson wrote:

>>>
>>>As result, our SDHC driver has to implement the functionality to
>>>send commands and check the results, in host layer.
>>>If directly calling mmc_wait_for_cmd() is improper, could you please
>>>give us some suggestions?
>>>
>>>For eMMC, CMD8 is used to test current sampling point set in PHY.
>>
>> Try to use mmc_send_tuning().
>>
> 
> Could you please tell me the requirement of "op_code" parameter in
> mmc_send_tuning()?
> According to mmc_send_tuning(),it seems that tuning command(CMD19/CMD21)
> is required. Thus device will not response mmc_send_tuning() if current
> speed mode doesn't support tuning command.
> Please correct me if I am wrong.
>

As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
send commands for testing current sampling point set in our host PHY.

According to my test result, it shows that mmc_send_tuning() can only 
support
tuning command (CMD21/CMD19).
As a result, we cannot use mmc_send_tuning() when card is in the speed modes
which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in those
speed modes.

Could you please provide suggestions for the speed mode in which tuning is
not available?

Thank you.

Best regards,
Hu Ziji

>>>
>>>>> +
>>>>> +   return err;
>>>>> +}
>>>>> +
>>>>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>>>>> +{
>>>>> +   struct mmc_command cmd = {0};
>>>>> +   int err;
>>>>> +
>>>>> +   cmd.opcode = SD_IO_RW_DIRECT;
>>>>> +   cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>>>>> +
>>>>> +   err = mmc_wait_for_cmd(card->host, , 0);
>>>>> +   if (err)
>>>>> +   return err;
>>>>> +
>>>>> +   if (cmd.resp[0] & R5_ERROR)
>>>>> +   return -EIO;
>>>>> +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>>>>> +   return -EINVAL;
>>>>> +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
>>>>> +   return -ERANGE;
>>>>> +   return 0;
>>>>
>>>> No thanks! MMC/SD/SDIO protocol code belongs in the core.
>>>>
>>>For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
>>>in PHY.
>>>Please help provide some suggestion to implement the command transfer.
>>
>> Again, I think mmc_send_tuning() should be possible for you to use.
>>
>> [...]
>>
>>>>> +   if (mmc->card)
>>>>> +   card = mmc->card;
>>>>> +   else
>>>>> +   /*
>>>>> +* Only valid during initialization
>>>>> +* before mmc->card is set
>>>>> +*/
>>>>> +   card = priv->card_candidate;
>>>>> +   if (unlikely(!card)) {
>>>>> +   dev_warn(mmc_dev(mmc), "card is not present\n");
>>>>> +   return -EINVAL;
>>>>> +   }
>>>>
>>>> That your host need to hold a copy of the card pointer, tells me that
>>>> something is not really correct.
>>>>
>>>> I might be wrong, if this turns out to be a special case, but I doubt
>>>> it. Although, if it *is* a special such case, we shall most likely try
>>>> to extend the the mmc core layer instead of adding all these hacks in
>>>> your host driver.
>>>>
>>> This card pointer copies the temporary structure mmc_card
>>> used in mmc_init_card(), mmc_sd_init_card() and mmc_sdio_init_card().
>>> Since we call mmc_wait_for_cmd() to send test commands, we need a copy
>>> of that temporary mmc_card here in our host driver.
>>
>> I see, thanks for clarifying.
>>
>>>
>>> During PHY setting in card initialization, mmc_host->card is not updated
>>> yet with that temporary mmc_card. Thus we are not able to directly use
>>> mmc_host->card. Instead, this card pointer is introduced to enable
>>> mmc_wait_for_cmd().
>>>
>>> If we can improve our host driver to send test commands without 
>>> mmc_card,
>>> this card pointer can be removed.
>>> Could you please share your opinion please?
>>
>> The mmc_send_tuning() API takes the mmc_host as parameter. If you
>> convert to that, perhaps you would be able to remove the need to hold
>> the card pointer.
>>
>> BTW, the reason why mmc_send_tuning() doesn't take the card as a
>> parameter, is exactly those you just described above.
>>
>Got it.
>Thanks a lot for the information.
> 
>Thank you for the great help.
> 
> Best regards,
> Hu Ziji
> 
>> [...]
>>
>> Kind regards
>> Uffe
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2016/11/25 21:06, Ulf Hansson wrote:
> [...]
> 
>>> +
>>> +   /*
>>> +* Xenon Specific property:
>>> +* emmc: explicitly indicate whether this slot is for eMMC
>>> +* slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO 
>>> register
>>> +* tun-count: the interval between re-tuning
>>> +* PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1"
>>> +*/
>>> +   if (of_property_read_bool(np, "marvell,xenon-emmc"))
>>> +   priv->emmc_slot = true;
>>
>> So, you need this because of the eMMC voltage switch behaviour, right?
>>
>> Then I would rather like to describe this a generic DT bindings for
>> the eMMC voltage level support. There have acutally been some earlier
>> discussions for this, but we haven't yet made some changes.
>>
>> I think what is missing is a mmc-ddr-3_3v DT binding, which when set,
>> allows the host driver to accept I/O voltage switches to 3.3V. If not
>> supported the  ->start_signal_voltage_switch() ops may return -EINVAL.
>> This would inform the mmc core to move on to the next supported
>> voltage level. There might be some minor additional changes to the mmc
>> card initialization sequence, but those should be simple.
>>
>> I can help out to look into this, unless you want to do it yourself of 
>> course!?
>>
>Yes. One of the reasons is to provide eMMC specific voltage setting.
>But in my very own opinion, it should be irrelevant to voltage level.
>The eMMC voltage setting on our SDHC is different from SD/SDIO voltage 
> switch.
>It will become more complex with different SOC implementation details.

 Got it. Although I think we can cope with that fine just by using the
 different SD/eMMC speed modes settings defined in DT (or from the
 SDHCI caps register)

>>> In my very opinion, I'm not sure if there is any corner case that 
>>> driver cannot
>>> determine the eMMC card type from DT and SDHC caps.
>>>
>Unfortunately, MMC driver cannot determine the card type yet when eMMC 
> voltage
>setting should be executed.
>Thus an flag is required here to tell driver to execute eMMC voltage 
> setting.
>
>Besides, additional eMMC specific settings might be implemented in 
> future, besides
>voltage setting. Most of them should be completed before MMC driver 
> recognizes the
>card type. Thus I have to keep this flag to indicate current SDHC is 
> for eMMC.

 I doubt you will need a generic "eMMC" flag, but let's see when we go 
 forward.

 Currently it's clear you don't need such a flag, so I will submit a
 change adding a DT binding for "mmc-ddr-3_3v" then we can take it from
 there, to see if it suits your needs.

>>
>> Another reason for a special "xenon-emmc" property is that our host IP 
>> usually can
>> support both eMMC and SD. Whether a host is used as eMMC or SD depends 
>> on the
>> final implementation of the actual product.
>> Thus our host driver needs to know whether current SDHC is fixed as eMMC 
>> or SD.
>> So far, It can only get the information from DT.
> 
> As a matter of fact for mounted non-removable cards, such as eMMC, we
> already have the option to describe some of their characteristics in
> DT. Perhaps that's what you need?
> 
> Please have a look at:
> Documentation/devicetree/bindings/mmc/mmc-card.txt
> 
   Great!
   I will try this mmc-card sub-node.

   Thank you very much.

Best regards,
Hu Ziji

>>
>> After out host driver get the card type information from DT, it can 
>> prepare eMMC
>> specific voltage, set eMMC specific mmc->caps/caps2 flags and do other
>> vendor specific init, before card init procedure.
>> Otherwise, our host driver has to wait until card type is determined in 
>> mmc_rescan().
>>
>> A generic "eMMC" flag is unnecessary. I just require a private property,
>> which is only used in our host driver and DT.
>>
>> Thank you.
>>
>> Best regards,
>> Hu Ziji
>>
>>>
>>> Actually, our eMMC is usually fixed as 1.8V.
>>>
>>> The pair "no-sd" + "no-sdio" can provide the similar information.
>>> But I'm not sure if it is proper to use those two property in such a 
>>> way.
>>>
>>> Thank you.
>>>
>>> Best regards
>>> Hu Ziji
>>>
 [...]

 Kind regards
 Uffe

>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
> 
> Kind regards
> Uffe
> 

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2016/11/25 21:06, Ulf Hansson wrote:
> [...]
> 
>>> +
>>> +   /*
>>> +* Xenon Specific property:
>>> +* emmc: explicitly indicate whether this slot is for eMMC
>>> +* slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO 
>>> register
>>> +* tun-count: the interval between re-tuning
>>> +* PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1"
>>> +*/
>>> +   if (of_property_read_bool(np, "marvell,xenon-emmc"))
>>> +   priv->emmc_slot = true;
>>
>> So, you need this because of the eMMC voltage switch behaviour, right?
>>
>> Then I would rather like to describe this a generic DT bindings for
>> the eMMC voltage level support. There have acutally been some earlier
>> discussions for this, but we haven't yet made some changes.
>>
>> I think what is missing is a mmc-ddr-3_3v DT binding, which when set,
>> allows the host driver to accept I/O voltage switches to 3.3V. If not
>> supported the  ->start_signal_voltage_switch() ops may return -EINVAL.
>> This would inform the mmc core to move on to the next supported
>> voltage level. There might be some minor additional changes to the mmc
>> card initialization sequence, but those should be simple.
>>
>> I can help out to look into this, unless you want to do it yourself of 
>> course!?
>>
>Yes. One of the reasons is to provide eMMC specific voltage setting.
>But in my very own opinion, it should be irrelevant to voltage level.
>The eMMC voltage setting on our SDHC is different from SD/SDIO voltage 
> switch.
>It will become more complex with different SOC implementation details.

 Got it. Although I think we can cope with that fine just by using the
 different SD/eMMC speed modes settings defined in DT (or from the
 SDHCI caps register)

>>> In my very opinion, I'm not sure if there is any corner case that 
>>> driver cannot
>>> determine the eMMC card type from DT and SDHC caps.
>>>
>Unfortunately, MMC driver cannot determine the card type yet when eMMC 
> voltage
>setting should be executed.
>Thus an flag is required here to tell driver to execute eMMC voltage 
> setting.
>
>Besides, additional eMMC specific settings might be implemented in 
> future, besides
>voltage setting. Most of them should be completed before MMC driver 
> recognizes the
>card type. Thus I have to keep this flag to indicate current SDHC is 
> for eMMC.

 I doubt you will need a generic "eMMC" flag, but let's see when we go 
 forward.

 Currently it's clear you don't need such a flag, so I will submit a
 change adding a DT binding for "mmc-ddr-3_3v" then we can take it from
 there, to see if it suits your needs.

>>
>> Another reason for a special "xenon-emmc" property is that our host IP 
>> usually can
>> support both eMMC and SD. Whether a host is used as eMMC or SD depends 
>> on the
>> final implementation of the actual product.
>> Thus our host driver needs to know whether current SDHC is fixed as eMMC 
>> or SD.
>> So far, It can only get the information from DT.
> 
> As a matter of fact for mounted non-removable cards, such as eMMC, we
> already have the option to describe some of their characteristics in
> DT. Perhaps that's what you need?
> 
> Please have a look at:
> Documentation/devicetree/bindings/mmc/mmc-card.txt
> 
   Great!
   I will try this mmc-card sub-node.

   Thank you very much.

Best regards,
Hu Ziji

>>
>> After out host driver get the card type information from DT, it can 
>> prepare eMMC
>> specific voltage, set eMMC specific mmc->caps/caps2 flags and do other
>> vendor specific init, before card init procedure.
>> Otherwise, our host driver has to wait until card type is determined in 
>> mmc_rescan().
>>
>> A generic "eMMC" flag is unnecessary. I just require a private property,
>> which is only used in our host driver and DT.
>>
>> Thank you.
>>
>> Best regards,
>> Hu Ziji
>>
>>>
>>> Actually, our eMMC is usually fixed as 1.8V.
>>>
>>> The pair "no-sd" + "no-sdio" can provide the similar information.
>>> But I'm not sure if it is proper to use those two property in such a 
>>> way.
>>>
>>> Thank you.
>>>
>>> Best regards
>>> Hu Ziji
>>>
 [...]

 Kind regards
 Uffe

>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
> 
> Kind regards
> Uffe
> 

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2016/11/24 23:00, Ziji Hu wrote:
> Hi Ulf,
> 
> On 2016/11/24 21:34, Ulf Hansson wrote:

>>>>> +
>>>>> +   /*
>>>>> +* Xenon Specific property:
>>>>> +* emmc: explicitly indicate whether this slot is for eMMC
>>>>> +* slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO register
>>>>> +* tun-count: the interval between re-tuning
>>>>> +* PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1"
>>>>> +*/
>>>>> +   if (of_property_read_bool(np, "marvell,xenon-emmc"))
>>>>> +   priv->emmc_slot = true;
>>>>
>>>> So, you need this because of the eMMC voltage switch behaviour, right?
>>>>
>>>> Then I would rather like to describe this a generic DT bindings for
>>>> the eMMC voltage level support. There have acutally been some earlier
>>>> discussions for this, but we haven't yet made some changes.
>>>>
>>>> I think what is missing is a mmc-ddr-3_3v DT binding, which when set,
>>>> allows the host driver to accept I/O voltage switches to 3.3V. If not
>>>> supported the  ->start_signal_voltage_switch() ops may return -EINVAL.
>>>> This would inform the mmc core to move on to the next supported
>>>> voltage level. There might be some minor additional changes to the mmc
>>>> card initialization sequence, but those should be simple.
>>>>
>>>> I can help out to look into this, unless you want to do it yourself of 
>>>> course!?
>>>>
>>>Yes. One of the reasons is to provide eMMC specific voltage setting.
>>>But in my very own opinion, it should be irrelevant to voltage level.
>>>The eMMC voltage setting on our SDHC is different from SD/SDIO voltage 
>>> switch.
>>>It will become more complex with different SOC implementation details.
>>
>> Got it. Although I think we can cope with that fine just by using the
>> different SD/eMMC speed modes settings defined in DT (or from the
>> SDHCI caps register)
>>
> In my very opinion, I'm not sure if there is any corner case that driver 
> cannot
> determine the eMMC card type from DT and SDHC caps.
> 
>>>Unfortunately, MMC driver cannot determine the card type yet when eMMC 
>>> voltage
>>>setting should be executed.
>>>Thus an flag is required here to tell driver to execute eMMC voltage 
>>> setting.
>>>
>>>Besides, additional eMMC specific settings might be implemented in 
>>> future, besides
>>>voltage setting. Most of them should be completed before MMC driver 
>>> recognizes the
>>>card type. Thus I have to keep this flag to indicate current SDHC is for 
>>> eMMC.
>>
>> I doubt you will need a generic "eMMC" flag, but let's see when we go 
>> forward.
>>
>> Currently it's clear you don't need such a flag, so I will submit a
>> change adding a DT binding for "mmc-ddr-3_3v" then we can take it from
>> there, to see if it suits your needs.
>>

Another reason for a special "xenon-emmc" property is that our host IP 
usually can
support both eMMC and SD. Whether a host is used as eMMC or SD depends on 
the
final implementation of the actual product.
Thus our host driver needs to know whether current SDHC is fixed as eMMC or 
SD.
So far, It can only get the information from DT.

After out host driver get the card type information from DT, it can prepare 
eMMC
specific voltage, set eMMC specific mmc->caps/caps2 flags and do other
vendor specific init, before card init procedure.
Otherwise, our host driver has to wait until card type is determined in 
mmc_rescan().

A generic "eMMC" flag is unnecessary. I just require a private property,
which is only used in our host driver and DT.

Thank you.

Best regards,
Hu Ziji

> 
> Actually, our eMMC is usually fixed as 1.8V.
> 
> The pair "no-sd" + "no-sdio" can provide the similar information.
> But I'm not sure if it is proper to use those two property in such a way.
> 
> Thank you.
> 
> Best regards
> Hu Ziji
> 
>> [...]
>>
>> Kind regards
>> Uffe
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2016/11/24 23:00, Ziji Hu wrote:
> Hi Ulf,
> 
> On 2016/11/24 21:34, Ulf Hansson wrote:

>>>>> +
>>>>> +   /*
>>>>> +* Xenon Specific property:
>>>>> +* emmc: explicitly indicate whether this slot is for eMMC
>>>>> +* slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO register
>>>>> +* tun-count: the interval between re-tuning
>>>>> +* PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1"
>>>>> +*/
>>>>> +   if (of_property_read_bool(np, "marvell,xenon-emmc"))
>>>>> +   priv->emmc_slot = true;
>>>>
>>>> So, you need this because of the eMMC voltage switch behaviour, right?
>>>>
>>>> Then I would rather like to describe this a generic DT bindings for
>>>> the eMMC voltage level support. There have acutally been some earlier
>>>> discussions for this, but we haven't yet made some changes.
>>>>
>>>> I think what is missing is a mmc-ddr-3_3v DT binding, which when set,
>>>> allows the host driver to accept I/O voltage switches to 3.3V. If not
>>>> supported the  ->start_signal_voltage_switch() ops may return -EINVAL.
>>>> This would inform the mmc core to move on to the next supported
>>>> voltage level. There might be some minor additional changes to the mmc
>>>> card initialization sequence, but those should be simple.
>>>>
>>>> I can help out to look into this, unless you want to do it yourself of 
>>>> course!?
>>>>
>>>Yes. One of the reasons is to provide eMMC specific voltage setting.
>>>But in my very own opinion, it should be irrelevant to voltage level.
>>>The eMMC voltage setting on our SDHC is different from SD/SDIO voltage 
>>> switch.
>>>It will become more complex with different SOC implementation details.
>>
>> Got it. Although I think we can cope with that fine just by using the
>> different SD/eMMC speed modes settings defined in DT (or from the
>> SDHCI caps register)
>>
> In my very opinion, I'm not sure if there is any corner case that driver 
> cannot
> determine the eMMC card type from DT and SDHC caps.
> 
>>>Unfortunately, MMC driver cannot determine the card type yet when eMMC 
>>> voltage
>>>setting should be executed.
>>>Thus an flag is required here to tell driver to execute eMMC voltage 
>>> setting.
>>>
>>>Besides, additional eMMC specific settings might be implemented in 
>>> future, besides
>>>voltage setting. Most of them should be completed before MMC driver 
>>> recognizes the
>>>card type. Thus I have to keep this flag to indicate current SDHC is for 
>>> eMMC.
>>
>> I doubt you will need a generic "eMMC" flag, but let's see when we go 
>> forward.
>>
>> Currently it's clear you don't need such a flag, so I will submit a
>> change adding a DT binding for "mmc-ddr-3_3v" then we can take it from
>> there, to see if it suits your needs.
>>

Another reason for a special "xenon-emmc" property is that our host IP 
usually can
support both eMMC and SD. Whether a host is used as eMMC or SD depends on 
the
final implementation of the actual product.
Thus our host driver needs to know whether current SDHC is fixed as eMMC or 
SD.
So far, It can only get the information from DT.

After out host driver get the card type information from DT, it can prepare 
eMMC
specific voltage, set eMMC specific mmc->caps/caps2 flags and do other
vendor specific init, before card init procedure.
Otherwise, our host driver has to wait until card type is determined in 
mmc_rescan().

A generic "eMMC" flag is unnecessary. I just require a private property,
which is only used in our host driver and DT.

Thank you.

Best regards,
Hu Ziji

> 
> Actually, our eMMC is usually fixed as 1.8V.
> 
> The pair "no-sd" + "no-sdio" can provide the similar information.
> But I'm not sure if it is proper to use those two property in such a way.
> 
> Thank you.
> 
> Best regards
> Hu Ziji
> 
>> [...]
>>
>> Kind regards
>> Uffe
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/24 22:33, Ulf Hansson wrote:
> [...]
> 
>>>

 +
 +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
 +{
 +   int err;
 +   u8 *ext_csd = NULL;
 +
 +   err = mmc_get_ext_csd(card, _csd);
 +   kfree(ext_csd);
>>>
>>> Why do you read the ext csd here?
>>>
>>I would like to simply introduce the PHY setting of our SDHC.
>>The target of the PHY setting is to achieve a perfect sampling
>>point for transfers, during card initialization.
> 
> Okay, so the phy is involved when running the tuning sequence.
> 
Actually, all the transfers pass our host PHY.

>>
>>For HS200/HS400/SDR104 whose SDCLK is more than 50MHz, SDHC HW
>>will search for this sampling point with DLL's help.
> 
> Apologize for my ignorance, but what is a "DLL" in this case?
> 
   DLL is Delay-locked Loop. It is a HW module similar to PLL.

>>
>>For other speed mode whose SDLCK is less than or equals to 50MHz,
>>SW has to scan the PHY delay line to find out this perfect sampling
>>point. Our driver sends a command to verify a sampling point
>>in current environment.
> 
> Ahh, okay! I guess the important part here is to not only send a
> command, but also to make sure data becomes transferred on the DAT
> lines, as to confirm your tuning sequence!?

   Yes.
   It is the best if the test command can transfer on DAT lines.

> 
> In cases of HS200/HS400/SDR104 you should be able to use the
> mmc_send_tuning() API, don't you think?

   For HS200/HS400/SDR104, we finally call sdhci_execute_tuning() to
   execute tuning. Those test commands are not used.
   In HS200/HS400/SDR104, HW will provide our host driver with suitable
   tuning step. Our host driver set the tuning step in SDHCI register and
   then start standard tuning sequence. The tuning step value provided
   by our host HW will enhance tuning. 

> 
> For the other cases (lower speed modes) which cards doesn't support
> the tuning command, perhaps you can just assume the PHY scan succeeded
> and then allow to core to continue with the card initialization
> sequence? Or do you foresee any issues with that? My point is that, if
> it will fail - it will fail anyway.

  Usually, our host driver will always successfully scan and select a
  perfect sampling point.
  If driver cannot find any suitable sampling point, it is likely that
  transfers will also fail after init. But usually it is a issue, caused by
  incorrect setting on boards/SOC/other PHY parameters, especially in 
development.
  We will fix the issue and then scan will succeed in final product.

> 
>>
>>As result, our SDHC driver has to implement the functionality to
>>send commands and check the results, in host layer.
>>If directly calling mmc_wait_for_cmd() is improper, could you please
>>give us some suggestions?
>>
>>For eMMC, CMD8 is used to test current sampling point set in PHY.
> 
> Try to use mmc_send_tuning().
> 

Could you please tell me the requirement of "op_code" parameter in
mmc_send_tuning()?
According to mmc_send_tuning(),it seems that tuning command(CMD19/CMD21)
is required. Thus device will not response mmc_send_tuning() if current
speed mode doesn't support tuning command.
Please correct me if I am wrong.

>>
 +
 +   return err;
 +}
 +
 +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
 +{
 +   struct mmc_command cmd = {0};
 +   int err;
 +
 +   cmd.opcode = SD_IO_RW_DIRECT;
 +   cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
 +
 +   err = mmc_wait_for_cmd(card->host, , 0);
 +   if (err)
 +   return err;
 +
 +   if (cmd.resp[0] & R5_ERROR)
 +   return -EIO;
 +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
 +   return -EINVAL;
 +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
 +   return -ERANGE;
 +   return 0;
>>>
>>> No thanks! MMC/SD/SDIO protocol code belongs in the core.
>>>
>>For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
>>in PHY.
>>Please help provide some suggestion to implement the command transfer.
> 
> Again, I think mmc_send_tuning() should be possible for you to use.
> 
> [...]
> 
 +   if (mmc->card)
 +   card = mmc->card;
 +   else
 +   /*
 +* Only valid during initialization
 +* before mmc->card is set
 +*/
 +   card = priv->card_candidate;
 +   if (unlikely(!card)) {
 +   dev_warn(mmc_dev(mmc), "card is not present\n");
 +   return -EINVAL;
 +   }
>>>
>>> That your host need to hold a copy of the card pointer, tells me that
>>> something is not really correct.
>>>
>>> I might be wrong, if this turns out to be a special case, but I doubt
>>> it. Although, if 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/24 22:33, Ulf Hansson wrote:
> [...]
> 
>>>

 +
 +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
 +{
 +   int err;
 +   u8 *ext_csd = NULL;
 +
 +   err = mmc_get_ext_csd(card, _csd);
 +   kfree(ext_csd);
>>>
>>> Why do you read the ext csd here?
>>>
>>I would like to simply introduce the PHY setting of our SDHC.
>>The target of the PHY setting is to achieve a perfect sampling
>>point for transfers, during card initialization.
> 
> Okay, so the phy is involved when running the tuning sequence.
> 
Actually, all the transfers pass our host PHY.

>>
>>For HS200/HS400/SDR104 whose SDCLK is more than 50MHz, SDHC HW
>>will search for this sampling point with DLL's help.
> 
> Apologize for my ignorance, but what is a "DLL" in this case?
> 
   DLL is Delay-locked Loop. It is a HW module similar to PLL.

>>
>>For other speed mode whose SDLCK is less than or equals to 50MHz,
>>SW has to scan the PHY delay line to find out this perfect sampling
>>point. Our driver sends a command to verify a sampling point
>>in current environment.
> 
> Ahh, okay! I guess the important part here is to not only send a
> command, but also to make sure data becomes transferred on the DAT
> lines, as to confirm your tuning sequence!?

   Yes.
   It is the best if the test command can transfer on DAT lines.

> 
> In cases of HS200/HS400/SDR104 you should be able to use the
> mmc_send_tuning() API, don't you think?

   For HS200/HS400/SDR104, we finally call sdhci_execute_tuning() to
   execute tuning. Those test commands are not used.
   In HS200/HS400/SDR104, HW will provide our host driver with suitable
   tuning step. Our host driver set the tuning step in SDHCI register and
   then start standard tuning sequence. The tuning step value provided
   by our host HW will enhance tuning. 

> 
> For the other cases (lower speed modes) which cards doesn't support
> the tuning command, perhaps you can just assume the PHY scan succeeded
> and then allow to core to continue with the card initialization
> sequence? Or do you foresee any issues with that? My point is that, if
> it will fail - it will fail anyway.

  Usually, our host driver will always successfully scan and select a
  perfect sampling point.
  If driver cannot find any suitable sampling point, it is likely that
  transfers will also fail after init. But usually it is a issue, caused by
  incorrect setting on boards/SOC/other PHY parameters, especially in 
development.
  We will fix the issue and then scan will succeed in final product.

> 
>>
>>As result, our SDHC driver has to implement the functionality to
>>send commands and check the results, in host layer.
>>If directly calling mmc_wait_for_cmd() is improper, could you please
>>give us some suggestions?
>>
>>For eMMC, CMD8 is used to test current sampling point set in PHY.
> 
> Try to use mmc_send_tuning().
> 

Could you please tell me the requirement of "op_code" parameter in
mmc_send_tuning()?
According to mmc_send_tuning(),it seems that tuning command(CMD19/CMD21)
is required. Thus device will not response mmc_send_tuning() if current
speed mode doesn't support tuning command.
Please correct me if I am wrong.

>>
 +
 +   return err;
 +}
 +
 +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
 +{
 +   struct mmc_command cmd = {0};
 +   int err;
 +
 +   cmd.opcode = SD_IO_RW_DIRECT;
 +   cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
 +
 +   err = mmc_wait_for_cmd(card->host, , 0);
 +   if (err)
 +   return err;
 +
 +   if (cmd.resp[0] & R5_ERROR)
 +   return -EIO;
 +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
 +   return -EINVAL;
 +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
 +   return -ERANGE;
 +   return 0;
>>>
>>> No thanks! MMC/SD/SDIO protocol code belongs in the core.
>>>
>>For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
>>in PHY.
>>Please help provide some suggestion to implement the command transfer.
> 
> Again, I think mmc_send_tuning() should be possible for you to use.
> 
> [...]
> 
 +   if (mmc->card)
 +   card = mmc->card;
 +   else
 +   /*
 +* Only valid during initialization
 +* before mmc->card is set
 +*/
 +   card = priv->card_candidate;
 +   if (unlikely(!card)) {
 +   dev_warn(mmc_dev(mmc), "card is not present\n");
 +   return -EINVAL;
 +   }
>>>
>>> That your host need to hold a copy of the card pointer, tells me that
>>> something is not really correct.
>>>
>>> I might be wrong, if this turns out to be a special case, but I doubt
>>> it. Although, if 

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2016/11/24 21:34, Ulf Hansson wrote:
> On 24 November 2016 at 13:41, Ziji Hu <huz...@marvell.com> wrote:
>> Hi Ulf,
>>
>> On 2016/11/24 18:43, Ulf Hansson wrote:
>>> On 31 October 2016 at 12:09, Gregory CLEMENT
>>> <gregory.clem...@free-electrons.com> wrote:
>>>> From: Ziji Hu <huz...@marvell.com>
>>>>
>> 
>>>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>>>> +   struct mmc_ios *ios)
>>>> +{
>>>> +   unsigned char voltage = ios->signal_voltage;
>>>> +
>>>> +   if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>>>> +   (voltage == MMC_SIGNAL_VOLTAGE_180))
>>>> +   return __emmc_signal_voltage_switch(mmc, voltage);
>>>> +
>>>> +   dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>>>> +   voltage);
>>>> +   return -EINVAL;
>>>
>>> This wrapper function seems unnessarry. It only adds a dev_err(), so
>>> then might as well do that in __emmc_signal_voltage_switch().
>>>
>>  Sure. Will merge it back to __emmc_signal_voltage_switch().
>>
>>>> +}
>>>> +
>>>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>>>> +struct mmc_ios *ios)
>>>> +{
>>>> +   struct sdhci_host *host = mmc_priv(mmc);
>>>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>> +
>>>> +   /*
>>>> +* Before SD/SDIO set signal voltage, SD bus clock should be
>>>> +* disabled. However, sdhci_set_clock will also disable the 
>>>> Internal
>>>> +* clock in mmc_set_signal_voltage().
>>>
>>> If that's the case then that is wrong in the generic sdhci code.
>>> What's the reason why it can't be fixed there instead of having this
>>> workaround?
>>>
>> In my very own opinion, SD Spec doesn't specify whether SDCLK should be
>> enabled or not during power setting.
>> Enabling SDCLK might be a special condition only required by our SDHC.
>> I try to avoid breaking other vendors' SDHC functionality
>> if their SDHCs require SDCLK disabled.
>> Thus I prefer to keep it inside our SDHC driver.
> 
> I let Adrian comment on this.
> 
> For sure we should avoid breaking other sdhci variant, but on the
> other hand *if* the generic code is wrong we should fix it!
> 
Of course.

>>
>>>> +* If Internal clock is disabled, the 3.3V/1.8V bit can not be 
>>>> updated.
>>>> +* Thus here manually enable internal clock.
>>>> +*
>>>> +* After switch completes, it is unnecessary to disable internal 
>>>> clock,
>>>> +* since keeping internal clock active obeys SD spec.
>>>> +*/
>>>> +   enable_xenon_internal_clk(host);
>>>> +
>>>> +   if (priv->emmc_slot)
>>>> +   return xenon_emmc_signal_voltage_switch(mmc, ios);
>>>> +
>>>> +   return sdhci_start_signal_voltage_switch(mmc, ios);
>>>> +}
>>>> +
>>>> +/*
>>>> + * After determining which slot is used for SDIO,
>>>> + * some additional task is required.
>>>> + */
>>>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>>> +{
>>>> +   struct sdhci_host *host = mmc_priv(mmc);
>>>> +   u32 reg;
>>>> +   u8 slot_idx;
>>>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>> +
>>>> +   /* Link the card for delay adjustment */
>>>> +   priv->card_candidate = card;
>>>> +   /* Set tuning functionality of this slot */
>>>> +   xenon_slot_tuning_setup(host);
>>>
>>> This looks weird. I assume this can be done as a part of the regular
>>> tuning seqeunce!?
>>>
>> It is our SDHC specific preparation prior to tuning, rather than a
>> standard step in spec.
>> Thus I leave it inside our driver.
> 
> My point is that this isn't the purpose of ->init_card(). thus you are
> abusing it.

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2016/11/24 21:34, Ulf Hansson wrote:
> On 24 November 2016 at 13:41, Ziji Hu  wrote:
>> Hi Ulf,
>>
>> On 2016/11/24 18:43, Ulf Hansson wrote:
>>> On 31 October 2016 at 12:09, Gregory CLEMENT
>>>  wrote:
>>>> From: Ziji Hu 
>>>>
>> 
>>>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>>>> +   struct mmc_ios *ios)
>>>> +{
>>>> +   unsigned char voltage = ios->signal_voltage;
>>>> +
>>>> +   if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>>>> +   (voltage == MMC_SIGNAL_VOLTAGE_180))
>>>> +   return __emmc_signal_voltage_switch(mmc, voltage);
>>>> +
>>>> +   dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>>>> +   voltage);
>>>> +   return -EINVAL;
>>>
>>> This wrapper function seems unnessarry. It only adds a dev_err(), so
>>> then might as well do that in __emmc_signal_voltage_switch().
>>>
>>  Sure. Will merge it back to __emmc_signal_voltage_switch().
>>
>>>> +}
>>>> +
>>>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>>>> +struct mmc_ios *ios)
>>>> +{
>>>> +   struct sdhci_host *host = mmc_priv(mmc);
>>>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>> +
>>>> +   /*
>>>> +* Before SD/SDIO set signal voltage, SD bus clock should be
>>>> +* disabled. However, sdhci_set_clock will also disable the 
>>>> Internal
>>>> +* clock in mmc_set_signal_voltage().
>>>
>>> If that's the case then that is wrong in the generic sdhci code.
>>> What's the reason why it can't be fixed there instead of having this
>>> workaround?
>>>
>> In my very own opinion, SD Spec doesn't specify whether SDCLK should be
>> enabled or not during power setting.
>> Enabling SDCLK might be a special condition only required by our SDHC.
>> I try to avoid breaking other vendors' SDHC functionality
>> if their SDHCs require SDCLK disabled.
>> Thus I prefer to keep it inside our SDHC driver.
> 
> I let Adrian comment on this.
> 
> For sure we should avoid breaking other sdhci variant, but on the
> other hand *if* the generic code is wrong we should fix it!
> 
Of course.

>>
>>>> +* If Internal clock is disabled, the 3.3V/1.8V bit can not be 
>>>> updated.
>>>> +* Thus here manually enable internal clock.
>>>> +*
>>>> +* After switch completes, it is unnecessary to disable internal 
>>>> clock,
>>>> +* since keeping internal clock active obeys SD spec.
>>>> +*/
>>>> +   enable_xenon_internal_clk(host);
>>>> +
>>>> +   if (priv->emmc_slot)
>>>> +   return xenon_emmc_signal_voltage_switch(mmc, ios);
>>>> +
>>>> +   return sdhci_start_signal_voltage_switch(mmc, ios);
>>>> +}
>>>> +
>>>> +/*
>>>> + * After determining which slot is used for SDIO,
>>>> + * some additional task is required.
>>>> + */
>>>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>>> +{
>>>> +   struct sdhci_host *host = mmc_priv(mmc);
>>>> +   u32 reg;
>>>> +   u8 slot_idx;
>>>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>> +
>>>> +   /* Link the card for delay adjustment */
>>>> +   priv->card_candidate = card;
>>>> +   /* Set tuning functionality of this slot */
>>>> +   xenon_slot_tuning_setup(host);
>>>
>>> This looks weird. I assume this can be done as a part of the regular
>>> tuning seqeunce!?
>>>
>> It is our SDHC specific preparation prior to tuning, rather than a
>> standard step in spec.
>> Thus I leave it inside our driver.
> 
> My point is that this isn't the purpose of ->init_card(). thus you are
> abusing it.
> 
> Try to make it work in another way, please. I think you can.
> 
   Got it.
   I

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

   Thanks a lot for the review.

On 2016/11/24 19:37, Ulf Hansson wrote:
> On 31 October 2016 at 12:09, Gregory CLEMENT
> <gregory.clem...@free-electrons.com> wrote:
>> From: Ziji Hu <huz...@marvell.com>
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji <huz...@marvell.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>> ---
>>  MAINTAINERS|2 +-
>>  drivers/mmc/host/Makefile  |2 +-
>>  drivers/mmc/host/sdhci-xenon-phy.c | 1181 +-
>>  drivers/mmc/host/sdhci-xenon-phy.h |  157 -
>>  drivers/mmc/host/sdhci-xenon.c |4 +-
>>  drivers/mmc/host/sdhci-xenon.h |   17 +-
>>  6 files changed, 1361 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
> 
> Can you please consider to split this up somehow!? It would make it
> easier to review...
> 
Sure. I will try to split them into smaller pieces.

> Anyway, allow me to provide some initial feedback, particularly around
> those things that Adrian and you requested for my input.
> 
> [...]
> 
>>
>> +
>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>> +{
>> +   int err;
>> +   u8 *ext_csd = NULL;
>> +
>> +   err = mmc_get_ext_csd(card, _csd);
>> +   kfree(ext_csd);
> 
> Why do you read the ext csd here?
> 
   I would like to simply introduce the PHY setting of our SDHC.
   The target of the PHY setting is to achieve a perfect sampling
   point for transfers, during card initialization.

   For HS200/HS400/SDR104 whose SDCLK is more than 50MHz, SDHC HW
   will search for this sampling point with DLL's help.

   For other speed mode whose SDLCK is less than or equals to 50MHz,
   SW has to scan the PHY delay line to find out this perfect sampling
   point. Our driver sends a command to verify a sampling point
   in current environment.

   As result, our SDHC driver has to implement the functionality to
   send commands and check the results, in host layer.
   If directly calling mmc_wait_for_cmd() is improper, could you please
   give us some suggestions?

   For eMMC, CMD8 is used to test current sampling point set in PHY.

>> +
>> +   return err;
>> +}
>> +
>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>> +{
>> +   struct mmc_command cmd = {0};
>> +   int err;
>> +
>> +   cmd.opcode = SD_IO_RW_DIRECT;
>> +   cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>> +
>> +   err = mmc_wait_for_cmd(card->host, , 0);
>> +   if (err)
>> +   return err;
>> +
>> +   if (cmd.resp[0] & R5_ERROR)
>> +   return -EIO;
>> +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>> +   return -EINVAL;
>> +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
>> +   return -ERANGE;
>> +   return 0;
> 
> No thanks! MMC/SD/SDIO protocol code belongs in the core.
> 
   For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
   in PHY.
   Please help provide some suggestion to implement the command transfer.

>> +}
>> +
>> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
>> +{
>> +   struct mmc_command cmd = {0};
>> +   int err;
>> +
>> +   cmd.opcode = MMC_SEND_STATUS;
>> +   cmd.arg = card->rca << 16;
>> +   cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>> +
>> +   err = mmc_wait_for_cmd(card->host, , 0);
>> +   return err;
> 
> No thanks! MMC/SD/SDIO protocol code belongs in the core.
> 
>> +}
>> +
> 
> [...]
> 
>> +int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios)
>> +{
>> +   struct mmc_host *mmc = host->mmc;
>> +   struct mmc_card *card;
>> +   int ret = 0;
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +   if (!host->clock) {
>> +   priv->clock = 0;
>> +   return 0;
>> +   }
>> +
>> +   /*
>> +* The timing, frequency or bus width is changed,
>> +* better to set eMMC PHY based on current setting
>> +* and adjust Xenon SDHC delay.
&

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

   Thanks a lot for the review.

On 2016/11/24 19:37, Ulf Hansson wrote:
> On 31 October 2016 at 12:09, Gregory CLEMENT
>  wrote:
>> From: Ziji Hu 
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji 
>> Signed-off-by: Gregory CLEMENT 
>> ---
>>  MAINTAINERS|2 +-
>>  drivers/mmc/host/Makefile  |2 +-
>>  drivers/mmc/host/sdhci-xenon-phy.c | 1181 +-
>>  drivers/mmc/host/sdhci-xenon-phy.h |  157 -
>>  drivers/mmc/host/sdhci-xenon.c |4 +-
>>  drivers/mmc/host/sdhci-xenon.h |   17 +-
>>  6 files changed, 1361 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
> 
> Can you please consider to split this up somehow!? It would make it
> easier to review...
> 
Sure. I will try to split them into smaller pieces.

> Anyway, allow me to provide some initial feedback, particularly around
> those things that Adrian and you requested for my input.
> 
> [...]
> 
>>
>> +
>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>> +{
>> +   int err;
>> +   u8 *ext_csd = NULL;
>> +
>> +   err = mmc_get_ext_csd(card, _csd);
>> +   kfree(ext_csd);
> 
> Why do you read the ext csd here?
> 
   I would like to simply introduce the PHY setting of our SDHC.
   The target of the PHY setting is to achieve a perfect sampling
   point for transfers, during card initialization.

   For HS200/HS400/SDR104 whose SDCLK is more than 50MHz, SDHC HW
   will search for this sampling point with DLL's help.

   For other speed mode whose SDLCK is less than or equals to 50MHz,
   SW has to scan the PHY delay line to find out this perfect sampling
   point. Our driver sends a command to verify a sampling point
   in current environment.

   As result, our SDHC driver has to implement the functionality to
   send commands and check the results, in host layer.
   If directly calling mmc_wait_for_cmd() is improper, could you please
   give us some suggestions?

   For eMMC, CMD8 is used to test current sampling point set in PHY.

>> +
>> +   return err;
>> +}
>> +
>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>> +{
>> +   struct mmc_command cmd = {0};
>> +   int err;
>> +
>> +   cmd.opcode = SD_IO_RW_DIRECT;
>> +   cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>> +
>> +   err = mmc_wait_for_cmd(card->host, , 0);
>> +   if (err)
>> +   return err;
>> +
>> +   if (cmd.resp[0] & R5_ERROR)
>> +   return -EIO;
>> +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>> +   return -EINVAL;
>> +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
>> +   return -ERANGE;
>> +   return 0;
> 
> No thanks! MMC/SD/SDIO protocol code belongs in the core.
> 
   For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
   in PHY.
   Please help provide some suggestion to implement the command transfer.

>> +}
>> +
>> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
>> +{
>> +   struct mmc_command cmd = {0};
>> +   int err;
>> +
>> +   cmd.opcode = MMC_SEND_STATUS;
>> +   cmd.arg = card->rca << 16;
>> +   cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>> +
>> +   err = mmc_wait_for_cmd(card->host, , 0);
>> +   return err;
> 
> No thanks! MMC/SD/SDIO protocol code belongs in the core.
> 
>> +}
>> +
> 
> [...]
> 
>> +int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios)
>> +{
>> +   struct mmc_host *mmc = host->mmc;
>> +   struct mmc_card *card;
>> +   int ret = 0;
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +   if (!host->clock) {
>> +   priv->clock = 0;
>> +   return 0;
>> +   }
>> +
>> +   /*
>> +* The timing, frequency or bus width is changed,
>> +* better to set eMMC PHY based on current setting
>> +* and adjust Xenon SDHC delay.
>> +*/
>> +   if ((host->clock == priv->clock) &&
>> +  

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2016/11/24 18:43, Ulf Hansson wrote:
> On 31 October 2016 at 12:09, Gregory CLEMENT
> <gregory.clem...@free-electrons.com> wrote:
>> From: Ziji Hu <huz...@marvell.com>
>>

>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>> +   struct mmc_ios *ios)
>> +{
>> +   unsigned char voltage = ios->signal_voltage;
>> +
>> +   if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>> +   (voltage == MMC_SIGNAL_VOLTAGE_180))
>> +   return __emmc_signal_voltage_switch(mmc, voltage);
>> +
>> +   dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>> +   voltage);
>> +   return -EINVAL;
> 
> This wrapper function seems unnessarry. It only adds a dev_err(), so
> then might as well do that in __emmc_signal_voltage_switch().
> 
 Sure. Will merge it back to __emmc_signal_voltage_switch().

>> +}
>> +
>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>> +struct mmc_ios *ios)
>> +{
>> +   struct sdhci_host *host = mmc_priv(mmc);
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +   /*
>> +* Before SD/SDIO set signal voltage, SD bus clock should be
>> +* disabled. However, sdhci_set_clock will also disable the Internal
>> +* clock in mmc_set_signal_voltage().
> 
> If that's the case then that is wrong in the generic sdhci code.
> What's the reason why it can't be fixed there instead of having this
> workaround?
> 
In my very own opinion, SD Spec doesn't specify whether SDCLK should be
enabled or not during power setting.
Enabling SDCLK might be a special condition only required by our SDHC.
I try to avoid breaking other vendors' SDHC functionality
if their SDHCs require SDCLK disabled.
Thus I prefer to keep it inside our SDHC driver.

>> +* If Internal clock is disabled, the 3.3V/1.8V bit can not be 
>> updated.
>> +* Thus here manually enable internal clock.
>> +*
>> +* After switch completes, it is unnecessary to disable internal 
>> clock,
>> +* since keeping internal clock active obeys SD spec.
>> +*/
>> +   enable_xenon_internal_clk(host);
>> +
>> +   if (priv->emmc_slot)
>> +   return xenon_emmc_signal_voltage_switch(mmc, ios);
>> +
>> +   return sdhci_start_signal_voltage_switch(mmc, ios);
>> +}
>> +
>> +/*
>> + * After determining which slot is used for SDIO,
>> + * some additional task is required.
>> + */
>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>> +{
>> +   struct sdhci_host *host = mmc_priv(mmc);
>> +   u32 reg;
>> +   u8 slot_idx;
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +   /* Link the card for delay adjustment */
>> +   priv->card_candidate = card;
>> +   /* Set tuning functionality of this slot */
>> +   xenon_slot_tuning_setup(host);
> 
> This looks weird. I assume this can be done as a part of the regular
> tuning seqeunce!?
> 
It is our SDHC specific preparation prior to tuning, rather than a
standard step in spec.
Thus I leave it inside our driver.

>> +
>> +   slot_idx = priv->slot_idx;
>> +   if (!mmc_card_sdio(card)) {
>> +   /* Clear SDIO Card Inserted indication */
> 
> Why do you need this?
> 
> If you need to reset this, I think it's better to do it from
> ->set_ios() at MMC_POWER_OFF.
> 
This field indicates SDIO card and controls async interrupt feature
of SDIO in our SDHC.
This async interrupt feature is enabled when SDIO card is inserted.
It should be disabled if SD card is inserted instead.

>> +   reg = sdhci_readl(host, SDHC_SYS_CFG_INFO);
>> +   reg &= ~(1 << (slot_idx + SLOT_TYPE_SDIO_SHIFT));
>> +   sdhci_writel(host, reg, SDHC_SYS_CFG_INFO);
>> +
>> +   if (mmc_card_mmc(card)) {
>> +   mmc->caps |= MMC_CAP_NONREMOVABLE;
>> +   if (!(host->quirks2 & SDHCI_QUIRK2_NO_1_8_V))
>> +   mmc->caps |= MMC_CAP_1_8V_DDR;
>> +   /*
>> +* Force to clear BUS_TEST to
>> +   

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Ulf,

On 2016/11/24 18:43, Ulf Hansson wrote:
> On 31 October 2016 at 12:09, Gregory CLEMENT
>  wrote:
>> From: Ziji Hu 
>>

>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>> +   struct mmc_ios *ios)
>> +{
>> +   unsigned char voltage = ios->signal_voltage;
>> +
>> +   if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>> +   (voltage == MMC_SIGNAL_VOLTAGE_180))
>> +   return __emmc_signal_voltage_switch(mmc, voltage);
>> +
>> +   dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>> +   voltage);
>> +   return -EINVAL;
> 
> This wrapper function seems unnessarry. It only adds a dev_err(), so
> then might as well do that in __emmc_signal_voltage_switch().
> 
 Sure. Will merge it back to __emmc_signal_voltage_switch().

>> +}
>> +
>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>> +struct mmc_ios *ios)
>> +{
>> +   struct sdhci_host *host = mmc_priv(mmc);
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +   /*
>> +* Before SD/SDIO set signal voltage, SD bus clock should be
>> +* disabled. However, sdhci_set_clock will also disable the Internal
>> +* clock in mmc_set_signal_voltage().
> 
> If that's the case then that is wrong in the generic sdhci code.
> What's the reason why it can't be fixed there instead of having this
> workaround?
> 
In my very own opinion, SD Spec doesn't specify whether SDCLK should be
enabled or not during power setting.
Enabling SDCLK might be a special condition only required by our SDHC.
I try to avoid breaking other vendors' SDHC functionality
if their SDHCs require SDCLK disabled.
Thus I prefer to keep it inside our SDHC driver.

>> +* If Internal clock is disabled, the 3.3V/1.8V bit can not be 
>> updated.
>> +* Thus here manually enable internal clock.
>> +*
>> +* After switch completes, it is unnecessary to disable internal 
>> clock,
>> +* since keeping internal clock active obeys SD spec.
>> +*/
>> +   enable_xenon_internal_clk(host);
>> +
>> +   if (priv->emmc_slot)
>> +   return xenon_emmc_signal_voltage_switch(mmc, ios);
>> +
>> +   return sdhci_start_signal_voltage_switch(mmc, ios);
>> +}
>> +
>> +/*
>> + * After determining which slot is used for SDIO,
>> + * some additional task is required.
>> + */
>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>> +{
>> +   struct sdhci_host *host = mmc_priv(mmc);
>> +   u32 reg;
>> +   u8 slot_idx;
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +   /* Link the card for delay adjustment */
>> +   priv->card_candidate = card;
>> +   /* Set tuning functionality of this slot */
>> +   xenon_slot_tuning_setup(host);
> 
> This looks weird. I assume this can be done as a part of the regular
> tuning seqeunce!?
> 
It is our SDHC specific preparation prior to tuning, rather than a
standard step in spec.
Thus I leave it inside our driver.

>> +
>> +   slot_idx = priv->slot_idx;
>> +   if (!mmc_card_sdio(card)) {
>> +   /* Clear SDIO Card Inserted indication */
> 
> Why do you need this?
> 
> If you need to reset this, I think it's better to do it from
> ->set_ios() at MMC_POWER_OFF.
> 
This field indicates SDIO card and controls async interrupt feature
of SDIO in our SDHC.
This async interrupt feature is enabled when SDIO card is inserted.
It should be disabled if SD card is inserted instead.

>> +   reg = sdhci_readl(host, SDHC_SYS_CFG_INFO);
>> +   reg &= ~(1 << (slot_idx + SLOT_TYPE_SDIO_SHIFT));
>> +   sdhci_writel(host, reg, SDHC_SYS_CFG_INFO);
>> +
>> +   if (mmc_card_mmc(card)) {
>> +   mmc->caps |= MMC_CAP_NONREMOVABLE;
>> +   if (!(host->quirks2 & SDHCI_QUIRK2_NO_1_8_V))
>> +   mmc->caps |= MMC_CAP_1_8V_DDR;
>> +   /*
>> +* Force to clear BUS_TEST to
>> +* skip bus_test_pre and bus_test_post
>

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Arnd,

On 2016/11/24 17:56, Arnd Bergmann wrote:
> On Monday, October 31, 2016 12:09:56 PM CET Gregory CLEMENT wrote:
>> From: Ziji Hu <huz...@marvell.com>
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji <huz...@marvell.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>>
> 
> Please explain in the changelog why this is not a generic
> phy driver (or three of them).
> 
Actually we tried to put the PHY code into Linux PHY framework.
But it cannot fit in Linux common PHY framework.

Our Xenon SDHC PHY register is a part of Xenon SDHC register set.
Besides, during MMC initialization, MMC sequence has to call several 
PHY functions to complete timing setting.
In those PHY setting functions, they have to access SDHC register and 
know current MMC setting, such as bus width, clock frequency and speed mode.
As a result, we have to implement PHY under MMC directory.

Thank you.

Best regards,
Hu Ziji

>   Arnd
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Arnd,

On 2016/11/24 17:56, Arnd Bergmann wrote:
> On Monday, October 31, 2016 12:09:56 PM CET Gregory CLEMENT wrote:
>> From: Ziji Hu 
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji 
>> Signed-off-by: Gregory CLEMENT 
>>
> 
> Please explain in the changelog why this is not a generic
> phy driver (or three of them).
> 
Actually we tried to put the PHY code into Linux PHY framework.
But it cannot fit in Linux common PHY framework.

Our Xenon SDHC PHY register is a part of Xenon SDHC register set.
Besides, during MMC initialization, MMC sequence has to call several 
PHY functions to complete timing setting.
In those PHY setting functions, they have to access SDHC register and 
know current MMC setting, such as bus width, clock frequency and speed mode.
As a result, we have to implement PHY under MMC directory.

Thank you.

Best regards,
Hu Ziji

>   Arnd
> 

Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

[Ziji Hu]

Hi all,

On 2016/11/24 18:10, Thomas Petazzoni wrote:
> Hello,
> 
> On Thu, 24 Nov 2016 10:49:23 +0100, Marcin Wojtas wrote:
> 
>> How about to avoid confusion, by simply renaming this number to
>> port-id/xenon-id or anything else but slot? I guess this may allow to
>> avoid some misunderstandings.
> 
We borrow the term "slot" from PCIe interface from SD spec.
According to Appendix C in SD spec 3.0, slot means an independent set 
of register from the view of SW.

I can avoid using "slot" and replace "slot index" with "sdhc-id".
Thanks for the suggestions.

Thank you.

Best regards,
Hu Ziji

> Agreed.
> 
> Thomas
> 

Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

[Ziji Hu]

Hi all,

On 2016/11/24 18:10, Thomas Petazzoni wrote:
> Hello,
> 
> On Thu, 24 Nov 2016 10:49:23 +0100, Marcin Wojtas wrote:
> 
>> How about to avoid confusion, by simply renaming this number to
>> port-id/xenon-id or anything else but slot? I guess this may allow to
>> avoid some misunderstandings.
> 
We borrow the term "slot" from PCIe interface from SD spec.
According to Appendix C in SD spec 3.0, slot means an independent set 
of register from the view of SW.

I can avoid using "slot" and replace "slot index" with "sdhc-id".
Thanks for the suggestions.

Thank you.

Best regards,
Hu Ziji

> Agreed.
> 
> Thomas
> 

Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

[Ziji Hu]

Hi Rob,

On 2016/11/10 2:24, Rob Herring wrote:
> On Mon, Oct 31, 2016 at 12:09:54PM +0100, Gregory CLEMENT wrote:
>> From: Ziji Hu <huz...@marvell.com>
>>
>> Marvell Xenon SDHC can support eMMC/SD/SDIO.
>> Add Xenon-specific properties.
>> Also add properties for Xenon PHY setting.
>>
>> Signed-off-by: Hu Ziji <huz...@marvell.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>> ---
>>  Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt | 161 +++-
>>  MAINTAINERS   |   1 +-
>>  2 files changed, 162 insertions(+), 0 deletions(-)
>>  create mode 100644 
>> Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt 
>> b/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
>> new file mode 100644
>> index ..0d2d139494d3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
>> @@ -0,0 +1,161 @@
>> +Marvell's Xenon SDHCI Controller device tree bindings
>> +This file documents differences between the core mmc properties
>> +described by mmc.txt and the properties used by the Xenon implementation.
>> +
>> +A single Xenon IP can support multiple slots.
>> +Each slot acts as an independent SDHC. It owns independent resources, such
>> +as register sets clock and PHY.
>> +Each slot should have an independent device tree node.
>> +
>> +Required Properties:
>> +- compatible: should be one of the following
>> +  - "marvell,armada-3700-sdhci": For controllers on Armada-3700 SOC.
>> +  Must provide a second register area and marvell,pad-type.
>> +  - "marvell,xenon-sdhci": For controllers on all the SOCs, other than
>> +  Armada-3700.
> 
> Need SoC specific compatible strings.
> 

Xenon SDHC is a common IP for all Marvell SOCs.
It is difficult to use a single SOC specific compatible to represent 
Xenon SDHC.
There will be so many SOC compatible strings in list if each specific 
SOC owns a compatible.
Actually only few SOCs require special properties.
Any suggestion please?

>> +
>> +- clocks:
>> +  Array of clocks required for SDHCI.
>> +  Requires at least one for Xenon IP core.
>> +  Some SOCs require additional clock for AXI bus.
>> +
>> +- clock-names:
>> +  Array of names corresponding to clocks property.
>> +  The input clock for Xenon IP core should be named as "core".
>> +  The optional AXI clock should be named as "axi".
> 
> When is AXI clock optional? This should be required for ?? compatible 
> strings.
> 
It is required on some SOCs.
I will double check if a suitable compatible string can be determined 
for those SOCs.

>> +
>> +- reg:
>> +  * For "marvell,xenon-sdhci", one register area for Xenon IP.
>> +
>> +  * For "marvell,armada-3700-sdhci", two register areas.
>> +The first one for Xenon IP register. The second one for the Armada 3700 
>> SOC
>> +PHY PAD Voltage Control register.
>> +Please follow the examples with compatible "marvell,armada-3700-sdhci"
>> +in below.
>> +Please also check property marvell,pad-type in below.
>> +
>> +Optional Properties:
>> +- marvell,xenon-slotno:
> 
> Multiple slots should be represented as child nodes IMO. I think some 
> other bindings already do this.
> 

All the slots are entirely independent.
I prefer to consider it as multiple independent SDHCs placed in a 
single IP, instead of that a IP contains multiple child slots.

It is unlike the implementation which put multiple slots behind PCIe EP 
interface. sdhci-pci.c will handle each slot init one by one.
If Xenon SDHC slots are represented as child nodes, there should also 
be a main entry in Xenon driver to init each child node one by one.
In my very own opinion, it is inconvenient and unnecessary.

>> +  Indicate the corresponding bit index of current Xenon SDHC slot in
>> +  SDHC System Operation Control Register Bit[7:0].
>> +  Set/clear the corresponding bit to enable/disable current Xenon SDHC
>> +  slot.
>> +  If this property is not provided, Xenon IP should contain only one
>> +  slot.
>> +
>> +- marvell,xenon-phy-type:
>> +  Xenon support mutilple types of PHYs.
>> +  To select eMMC 5.1 PHY, set:
>> +  marvell,xenon-phy-type = "emmc 5.1 phy"
>> +  eMMC 5.1 PHY is the default choice if this property is not

Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

[Ziji Hu]

Hi Rob,

On 2016/11/10 2:24, Rob Herring wrote:
> On Mon, Oct 31, 2016 at 12:09:54PM +0100, Gregory CLEMENT wrote:
>> From: Ziji Hu 
>>
>> Marvell Xenon SDHC can support eMMC/SD/SDIO.
>> Add Xenon-specific properties.
>> Also add properties for Xenon PHY setting.
>>
>> Signed-off-by: Hu Ziji 
>> Signed-off-by: Gregory CLEMENT 
>> ---
>>  Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt | 161 +++-
>>  MAINTAINERS   |   1 +-
>>  2 files changed, 162 insertions(+), 0 deletions(-)
>>  create mode 100644 
>> Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt 
>> b/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
>> new file mode 100644
>> index ..0d2d139494d3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
>> @@ -0,0 +1,161 @@
>> +Marvell's Xenon SDHCI Controller device tree bindings
>> +This file documents differences between the core mmc properties
>> +described by mmc.txt and the properties used by the Xenon implementation.
>> +
>> +A single Xenon IP can support multiple slots.
>> +Each slot acts as an independent SDHC. It owns independent resources, such
>> +as register sets clock and PHY.
>> +Each slot should have an independent device tree node.
>> +
>> +Required Properties:
>> +- compatible: should be one of the following
>> +  - "marvell,armada-3700-sdhci": For controllers on Armada-3700 SOC.
>> +  Must provide a second register area and marvell,pad-type.
>> +  - "marvell,xenon-sdhci": For controllers on all the SOCs, other than
>> +  Armada-3700.
> 
> Need SoC specific compatible strings.
> 

Xenon SDHC is a common IP for all Marvell SOCs.
It is difficult to use a single SOC specific compatible to represent 
Xenon SDHC.
There will be so many SOC compatible strings in list if each specific 
SOC owns a compatible.
Actually only few SOCs require special properties.
Any suggestion please?

>> +
>> +- clocks:
>> +  Array of clocks required for SDHCI.
>> +  Requires at least one for Xenon IP core.
>> +  Some SOCs require additional clock for AXI bus.
>> +
>> +- clock-names:
>> +  Array of names corresponding to clocks property.
>> +  The input clock for Xenon IP core should be named as "core".
>> +  The optional AXI clock should be named as "axi".
> 
> When is AXI clock optional? This should be required for ?? compatible 
> strings.
> 
It is required on some SOCs.
I will double check if a suitable compatible string can be determined 
for those SOCs.

>> +
>> +- reg:
>> +  * For "marvell,xenon-sdhci", one register area for Xenon IP.
>> +
>> +  * For "marvell,armada-3700-sdhci", two register areas.
>> +The first one for Xenon IP register. The second one for the Armada 3700 
>> SOC
>> +PHY PAD Voltage Control register.
>> +Please follow the examples with compatible "marvell,armada-3700-sdhci"
>> +in below.
>> +Please also check property marvell,pad-type in below.
>> +
>> +Optional Properties:
>> +- marvell,xenon-slotno:
> 
> Multiple slots should be represented as child nodes IMO. I think some 
> other bindings already do this.
> 

All the slots are entirely independent.
I prefer to consider it as multiple independent SDHCs placed in a 
single IP, instead of that a IP contains multiple child slots.

It is unlike the implementation which put multiple slots behind PCIe EP 
interface. sdhci-pci.c will handle each slot init one by one.
If Xenon SDHC slots are represented as child nodes, there should also 
be a main entry in Xenon driver to init each child node one by one.
In my very own opinion, it is inconvenient and unnecessary.

>> +  Indicate the corresponding bit index of current Xenon SDHC slot in
>> +  SDHC System Operation Control Register Bit[7:0].
>> +  Set/clear the corresponding bit to enable/disable current Xenon SDHC
>> +  slot.
>> +  If this property is not provided, Xenon IP should contain only one
>> +  slot.
>> +
>> +- marvell,xenon-phy-type:
>> +  Xenon support mutilple types of PHYs.
>> +  To select eMMC 5.1 PHY, set:
>> +  marvell,xenon-phy-type = "emmc 5.1 phy"
>> +  eMMC 5.1 PHY is the default choice if this property is not provided.
>> +  To select eMMC 5.0 PHY, set:
>> +  marvell,xenon-phy-type = "

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Adrian,

On 2016/10/17 16:14, Adrian Hunter wrote:
> On 13/10/16 08:38, Ziji Hu wrote:
>> Hi Adrian,
>>
>> On 2016/10/12 21:07, Adrian Hunter wrote:
>>> On 12/10/16 14:58, Ziji Hu wrote:
>>>> Hi Adrian,
>>>>
>>>>Thank you very much for your review.
>>>>I will firstly fix the typo.
>>>>
>>>> On 2016/10/11 20:37, Adrian Hunter wrote:
>> 
>>>>>> +
>>>>>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>>>>>> + struct mmc_ios *ios)
>>>>>> +{
>>>>>> +struct sdhci_host *host = mmc_priv(mmc);
>>>>>> +struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>> +struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>>>> +
>>>>>> +/*
>>>>>> + * Before SD/SDIO set signal voltage, SD bus clock should be
>>>>>> + * disabled. However, sdhci_set_clock will also disable the 
>>>>>> Internal
>>>>>> + * clock in mmc_set_signal_voltage().
>>>>>> + * If Internal clock is disabled, the 3.3V/1.8V bit can not be 
>>>>>> updated.
>>>>>> + * Thus here manually enable internal clock.
>>>>>> + *
>>>>>> + * After switch completes, it is unnecessary to disable 
>>>>>> internal clock,
>>>>>> + * since keeping internal clock active obeys SD spec.
>>>>>> + */
>>>>>> +enable_xenon_internal_clk(host);
>>>>>> +
>>>>>> +if (priv->card_candidate) {
>>>>>
>>>>> mmc_power_up() calls __mmc_set_signal_voltage() calls
>>>>> host->ops->start_signal_voltage_switch so priv->card_candidate could be an
>>>>> invalid reference to an old card.
>>>>>
>>>>> So that's not going to work if the card changes - not only for removable
>>>>> cards but even for eMMC if init fails and retries.
>>>>>
>>>>As you point out, this piece of code have defects, even though it 
>>>> actually works on Marvell multiple platforms, unless eMMC card is 
>>>> removable.
>>>>
>>>>I can add a property to explicitly indicate eMMC type in DTS.
>>>>Then card_candidate access can be removed here.
>>>>Does it sounds more reasonable to you?
>>>
>>> Sure
>>>
>>>>
>>>>>> +if (mmc_card_mmc(priv->card_candidate))
>>>>>> +return xenon_emmc_signal_voltage_switch(mmc, 
>>>>>> ios);
>>>>>
>>>>> So if all you need to know is whether it is a eMMC, why can't DT tell you?
>>>>>
>>>>I can add an eMMC type property in DTS, to remove the card_candidate 
>>>> access here.
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +return sdhci_start_signal_voltage_switch(mmc, ios);
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * After determining which slot is used for SDIO,
>>>>>> + * some additional task is required.
>>>>>> + */
>>>>>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>>>>> +{
>>>>>> +struct sdhci_host *host = mmc_priv(mmc);
>>>>>> +u32 reg;
>>>>>> +u8 slot_idx;
>>>>>> +struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>> +struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>>>> +
>>>>>> +/* Link the card for delay adjustment */
>>>>>> +priv->card_candidate = card;
>>>>>
>>>>> You really need a better way to get the card.  I suggest you take up the
>>>>> issue with Ulf.  One possibility is to have mmc core set host->card = card
>>>>> much earlier.
>>>>>
>>>>Could you please tell me if any issue related to card_candidate still 
>>>> exists, after the card_candidate is removed from 
>>>> xenon_start_signal_voltage_switch() in above?
>&g

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Adrian,

On 2016/10/17 16:14, Adrian Hunter wrote:
> On 13/10/16 08:38, Ziji Hu wrote:
>> Hi Adrian,
>>
>> On 2016/10/12 21:07, Adrian Hunter wrote:
>>> On 12/10/16 14:58, Ziji Hu wrote:
>>>> Hi Adrian,
>>>>
>>>>Thank you very much for your review.
>>>>I will firstly fix the typo.
>>>>
>>>> On 2016/10/11 20:37, Adrian Hunter wrote:
>> 
>>>>>> +
>>>>>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>>>>>> + struct mmc_ios *ios)
>>>>>> +{
>>>>>> +struct sdhci_host *host = mmc_priv(mmc);
>>>>>> +struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>> +struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>>>> +
>>>>>> +/*
>>>>>> + * Before SD/SDIO set signal voltage, SD bus clock should be
>>>>>> + * disabled. However, sdhci_set_clock will also disable the 
>>>>>> Internal
>>>>>> + * clock in mmc_set_signal_voltage().
>>>>>> + * If Internal clock is disabled, the 3.3V/1.8V bit can not be 
>>>>>> updated.
>>>>>> + * Thus here manually enable internal clock.
>>>>>> + *
>>>>>> + * After switch completes, it is unnecessary to disable 
>>>>>> internal clock,
>>>>>> + * since keeping internal clock active obeys SD spec.
>>>>>> + */
>>>>>> +enable_xenon_internal_clk(host);
>>>>>> +
>>>>>> +if (priv->card_candidate) {
>>>>>
>>>>> mmc_power_up() calls __mmc_set_signal_voltage() calls
>>>>> host->ops->start_signal_voltage_switch so priv->card_candidate could be an
>>>>> invalid reference to an old card.
>>>>>
>>>>> So that's not going to work if the card changes - not only for removable
>>>>> cards but even for eMMC if init fails and retries.
>>>>>
>>>>As you point out, this piece of code have defects, even though it 
>>>> actually works on Marvell multiple platforms, unless eMMC card is 
>>>> removable.
>>>>
>>>>I can add a property to explicitly indicate eMMC type in DTS.
>>>>Then card_candidate access can be removed here.
>>>>Does it sounds more reasonable to you?
>>>
>>> Sure
>>>
>>>>
>>>>>> +if (mmc_card_mmc(priv->card_candidate))
>>>>>> +return xenon_emmc_signal_voltage_switch(mmc, 
>>>>>> ios);
>>>>>
>>>>> So if all you need to know is whether it is a eMMC, why can't DT tell you?
>>>>>
>>>>I can add an eMMC type property in DTS, to remove the card_candidate 
>>>> access here.
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +return sdhci_start_signal_voltage_switch(mmc, ios);
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * After determining which slot is used for SDIO,
>>>>>> + * some additional task is required.
>>>>>> + */
>>>>>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>>>>> +{
>>>>>> +struct sdhci_host *host = mmc_priv(mmc);
>>>>>> +u32 reg;
>>>>>> +u8 slot_idx;
>>>>>> +struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>> +struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>>>> +
>>>>>> +/* Link the card for delay adjustment */
>>>>>> +priv->card_candidate = card;
>>>>>
>>>>> You really need a better way to get the card.  I suggest you take up the
>>>>> issue with Ulf.  One possibility is to have mmc core set host->card = card
>>>>> much earlier.
>>>>>
>>>>Could you please tell me if any issue related to card_candidate still 
>>>> exists, after the card_candidate is removed from 
>>>> xenon_start_signal_voltage_switch() in above?
>&g

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Adrian,

On 2016/10/17 15:55, Adrian Hunter wrote:
> On 12/10/16 15:17, Ziji Hu wrote:
>> Hi Adrian,
>>
>> On 2016/10/11 20:39, Adrian Hunter wrote:

>>>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>>>> +{
>>>> +  int err;
>>>> +  u8 *ext_csd = NULL;
>>>> +
>>>> +  err = mmc_get_ext_csd(card, _csd);
>>>> +  kfree(ext_csd);
>>>> +
>>>> +  return err;
>>>> +}
>>>> +
>>>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>>>> +{
>>>> +  struct mmc_command cmd = {0};
>>>> +  int err;
>>>> +
>>>> +  cmd.opcode = SD_IO_RW_DIRECT;
>>>> +  cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>>>> +
>>>> +  err = mmc_wait_for_cmd(card->host, , 0);
>>>> +  if (err)
>>>> +  return err;
>>>> +
>>>> +  if (cmd.resp[0] & R5_ERROR)
>>>> +  return -EIO;
>>>> +  if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>>>> +  return -EINVAL;
>>>> +  if (cmd.resp[0] & R5_OUT_OF_RANGE)
>>>> +  return -ERANGE;
>>>> +  return 0;
>>>> +}
>>>> +
>>>> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
>>>> +{
>>>> +  struct mmc_command cmd = {0};
>>>> +  int err;
>>>> +
>>>> +  cmd.opcode = MMC_SEND_STATUS;
>>>> +  cmd.arg = card->rca << 16;
>>>> +  cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>>>> +
>>>> +  err = mmc_wait_for_cmd(card->host, , 0);
>>>> +  return err;
>>>> +}
>>>> +
>>>> +static int xenon_delay_adj_test(struct mmc_card *card)
>>>> +{
>>>> +  WARN_ON(!card);
>>>> +  WARN_ON(!card->host);
>>>> +
>>>> +  if (mmc_card_mmc(card))
>>>> +  return __xenon_emmc_delay_adj_test(card);
>>>> +  else if (mmc_card_sd(card))
>>>> +  return __xenon_sd_delay_adj_test(card);
>>>> +  else if (mmc_card_sdio(card))
>>>> +  return __xenon_sdio_delay_adj_test(card);
>>>> +  else
>>>> +  return -EINVAL;
>>>> +}
>>>
>>> So you are issuing commands from the ->set_ios() callback.  I would want to
>>> get Ulf's OK for that before going further.
>>>
>>  Yes, you are correct.
>>  In some speed mode, Xenon SDHC has to send a series of transfers to 
>> search for a perfect sampling point in PHY delay line.
>>  It is like tuning process.
>>
>>> One thing: you will need to ensure you don't trigger get HS400 re-tuning
>>> because it will call back into ->set_ios().
>>>
>>  Could you please make the term "HS400 re-tuning" more detailed?
>>  In current MMC driver, "HS400 re-tuning" will go back to HS200, execute 
>> HS200 tuning and come back to HS400.
>>  I'm sure our Xenon SDHC will not execute it.
> 
> Currently, re-tuning is automatically enabled whenever tuning is executed,
> and then re-tuning will be done periodically or after CRC errors.  The
> function to disable re-tuning mmc_retune_disable() is not exported, however
> if you have you are determining the sampling point your own way, you could
> simply not implement ->execute_tuning() and then there would be no tuning
> and no re-tuning.
>

It is a little complex in our Xenon SDHC.
For the speed mode which requests tuning, such as SDR104 and HS200, our 
driver will execute standard tuning as spec requires.
However, for those speed mode in which tuning is not requested in spec, 
our driver has to issues commands to search for the best sampling point.

It seems that HS400 re-tuning/tuning is disabled by default. Is it 
correct?

>>
>>  However, in coming eMMC 5.2, there is a real HS400 re-tuning, in which 
>> tuning can be directly executed in HS400 mode.
>>  Our Xenon SDHC will neither trigger this HS400 re-tuning.
>>  But since so far there is no such feature in MMC driver, I cannot give 
>> you a 100% guarantee now.
>>
>>> And you have the problem that you need to get a reference to the card before
>>> the card device has been added.  As I wrote in response to the previous
>>> patch, you should get Ulf's help with that too.
>>>
>>  Sure.
>>  I will get card_candidate solved at first.
>>  Thank you again for your review and help.
>>
>>  Thank you.
>>
>> Best regards,
>> Hu Ziji
>>>
>>
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Adrian,

On 2016/10/17 15:55, Adrian Hunter wrote:
> On 12/10/16 15:17, Ziji Hu wrote:
>> Hi Adrian,
>>
>> On 2016/10/11 20:39, Adrian Hunter wrote:

>>>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>>>> +{
>>>> +  int err;
>>>> +  u8 *ext_csd = NULL;
>>>> +
>>>> +  err = mmc_get_ext_csd(card, _csd);
>>>> +  kfree(ext_csd);
>>>> +
>>>> +  return err;
>>>> +}
>>>> +
>>>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>>>> +{
>>>> +  struct mmc_command cmd = {0};
>>>> +  int err;
>>>> +
>>>> +  cmd.opcode = SD_IO_RW_DIRECT;
>>>> +  cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>>>> +
>>>> +  err = mmc_wait_for_cmd(card->host, , 0);
>>>> +  if (err)
>>>> +  return err;
>>>> +
>>>> +  if (cmd.resp[0] & R5_ERROR)
>>>> +  return -EIO;
>>>> +  if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>>>> +  return -EINVAL;
>>>> +  if (cmd.resp[0] & R5_OUT_OF_RANGE)
>>>> +  return -ERANGE;
>>>> +  return 0;
>>>> +}
>>>> +
>>>> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
>>>> +{
>>>> +  struct mmc_command cmd = {0};
>>>> +  int err;
>>>> +
>>>> +  cmd.opcode = MMC_SEND_STATUS;
>>>> +  cmd.arg = card->rca << 16;
>>>> +  cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>>>> +
>>>> +  err = mmc_wait_for_cmd(card->host, , 0);
>>>> +  return err;
>>>> +}
>>>> +
>>>> +static int xenon_delay_adj_test(struct mmc_card *card)
>>>> +{
>>>> +  WARN_ON(!card);
>>>> +  WARN_ON(!card->host);
>>>> +
>>>> +  if (mmc_card_mmc(card))
>>>> +  return __xenon_emmc_delay_adj_test(card);
>>>> +  else if (mmc_card_sd(card))
>>>> +  return __xenon_sd_delay_adj_test(card);
>>>> +  else if (mmc_card_sdio(card))
>>>> +  return __xenon_sdio_delay_adj_test(card);
>>>> +  else
>>>> +  return -EINVAL;
>>>> +}
>>>
>>> So you are issuing commands from the ->set_ios() callback.  I would want to
>>> get Ulf's OK for that before going further.
>>>
>>  Yes, you are correct.
>>  In some speed mode, Xenon SDHC has to send a series of transfers to 
>> search for a perfect sampling point in PHY delay line.
>>  It is like tuning process.
>>
>>> One thing: you will need to ensure you don't trigger get HS400 re-tuning
>>> because it will call back into ->set_ios().
>>>
>>  Could you please make the term "HS400 re-tuning" more detailed?
>>  In current MMC driver, "HS400 re-tuning" will go back to HS200, execute 
>> HS200 tuning and come back to HS400.
>>  I'm sure our Xenon SDHC will not execute it.
> 
> Currently, re-tuning is automatically enabled whenever tuning is executed,
> and then re-tuning will be done periodically or after CRC errors.  The
> function to disable re-tuning mmc_retune_disable() is not exported, however
> if you have you are determining the sampling point your own way, you could
> simply not implement ->execute_tuning() and then there would be no tuning
> and no re-tuning.
>

It is a little complex in our Xenon SDHC.
For the speed mode which requests tuning, such as SDR104 and HS200, our 
driver will execute standard tuning as spec requires.
However, for those speed mode in which tuning is not requested in spec, 
our driver has to issues commands to search for the best sampling point.

It seems that HS400 re-tuning/tuning is disabled by default. Is it 
correct?

>>
>>  However, in coming eMMC 5.2, there is a real HS400 re-tuning, in which 
>> tuning can be directly executed in HS400 mode.
>>  Our Xenon SDHC will neither trigger this HS400 re-tuning.
>>  But since so far there is no such feature in MMC driver, I cannot give 
>> you a 100% guarantee now.
>>
>>> And you have the problem that you need to get a reference to the card before
>>> the card device has been added.  As I wrote in response to the previous
>>> patch, you should get Ulf's help with that too.
>>>
>>  Sure.
>>  I will get card_candidate solved at first.
>>  Thank you again for your review and help.
>>
>>  Thank you.
>>
>> Best regards,
>> Hu Ziji
>>>
>>
> 

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Adrian,

On 2016/10/12 21:07, Adrian Hunter wrote:
> On 12/10/16 14:58, Ziji Hu wrote:
>> Hi Adrian,
>>
>>  Thank you very much for your review.
>>  I will firstly fix the typo.
>>
>> On 2016/10/11 20:37, Adrian Hunter wrote:

>>>> +
>>>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>>>> +   struct mmc_ios *ios)
>>>> +{
>>>> +  struct sdhci_host *host = mmc_priv(mmc);
>>>> +  struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +  struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>> +
>>>> +  /*
>>>> +   * Before SD/SDIO set signal voltage, SD bus clock should be
>>>> +   * disabled. However, sdhci_set_clock will also disable the Internal
>>>> +   * clock in mmc_set_signal_voltage().
>>>> +   * If Internal clock is disabled, the 3.3V/1.8V bit can not be updated.
>>>> +   * Thus here manually enable internal clock.
>>>> +   *
>>>> +   * After switch completes, it is unnecessary to disable internal clock,
>>>> +   * since keeping internal clock active obeys SD spec.
>>>> +   */
>>>> +  enable_xenon_internal_clk(host);
>>>> +
>>>> +  if (priv->card_candidate) {
>>>
>>> mmc_power_up() calls __mmc_set_signal_voltage() calls
>>> host->ops->start_signal_voltage_switch so priv->card_candidate could be an
>>> invalid reference to an old card.
>>>
>>> So that's not going to work if the card changes - not only for removable
>>> cards but even for eMMC if init fails and retries.
>>>
>>  As you point out, this piece of code have defects, even though it 
>> actually works on Marvell multiple platforms, unless eMMC card is removable.
>>
>>  I can add a property to explicitly indicate eMMC type in DTS.
>>  Then card_candidate access can be removed here.
>>  Does it sounds more reasonable to you?
> 
> Sure
> 
>>
>>>> +  if (mmc_card_mmc(priv->card_candidate))
>>>> +  return xenon_emmc_signal_voltage_switch(mmc, ios);
>>>
>>> So if all you need to know is whether it is a eMMC, why can't DT tell you?
>>>
>>  I can add an eMMC type property in DTS, to remove the card_candidate 
>> access here.
>>
>>>> +  }
>>>> +
>>>> +  return sdhci_start_signal_voltage_switch(mmc, ios);
>>>> +}
>>>> +
>>>> +/*
>>>> + * After determining which slot is used for SDIO,
>>>> + * some additional task is required.
>>>> + */
>>>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>>> +{
>>>> +  struct sdhci_host *host = mmc_priv(mmc);
>>>> +  u32 reg;
>>>> +  u8 slot_idx;
>>>> +  struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +  struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>> +
>>>> +  /* Link the card for delay adjustment */
>>>> +  priv->card_candidate = card;
>>>
>>> You really need a better way to get the card.  I suggest you take up the
>>> issue with Ulf.  One possibility is to have mmc core set host->card = card
>>> much earlier.
>>>
>>  Could you please tell me if any issue related to card_candidate still 
>> exists, after the card_candidate is removed from 
>> xenon_start_signal_voltage_switch() in above?
>>  It seems that when init_card is called, the structure card has already 
>> been updated and stable in MMC/SD/SDIO initialization sequence.
>>  May I keep it here?
> 
> It works by accident rather than by design.  We can do better.
> 
Could you please tell me some details which are satisfied about 
card_candidate?

I must admit that card_candidate in xenon_start_signal_voltage_switch() 
is imperfect.
But card_candidate in init_card() and later in set_ios() work by 
design, rather than by accident. We did a lot of tests on several platforms.

The structure mmc_card passed in here is a stable one. Thus in my very 
own opinion, it is safe and stable to use mmc_card here.
card_candidate is used only in card initialization. It is not active in 
later transfers after initialization is done.
It will always updated with mmc_card in next card initialization.

>>
>>>> +  /* Set tuning functionality of this slot */
>>>> +  

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Adrian,

On 2016/10/12 21:07, Adrian Hunter wrote:
> On 12/10/16 14:58, Ziji Hu wrote:
>> Hi Adrian,
>>
>>  Thank you very much for your review.
>>  I will firstly fix the typo.
>>
>> On 2016/10/11 20:37, Adrian Hunter wrote:

>>>> +
>>>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>>>> +   struct mmc_ios *ios)
>>>> +{
>>>> +  struct sdhci_host *host = mmc_priv(mmc);
>>>> +  struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +  struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>> +
>>>> +  /*
>>>> +   * Before SD/SDIO set signal voltage, SD bus clock should be
>>>> +   * disabled. However, sdhci_set_clock will also disable the Internal
>>>> +   * clock in mmc_set_signal_voltage().
>>>> +   * If Internal clock is disabled, the 3.3V/1.8V bit can not be updated.
>>>> +   * Thus here manually enable internal clock.
>>>> +   *
>>>> +   * After switch completes, it is unnecessary to disable internal clock,
>>>> +   * since keeping internal clock active obeys SD spec.
>>>> +   */
>>>> +  enable_xenon_internal_clk(host);
>>>> +
>>>> +  if (priv->card_candidate) {
>>>
>>> mmc_power_up() calls __mmc_set_signal_voltage() calls
>>> host->ops->start_signal_voltage_switch so priv->card_candidate could be an
>>> invalid reference to an old card.
>>>
>>> So that's not going to work if the card changes - not only for removable
>>> cards but even for eMMC if init fails and retries.
>>>
>>  As you point out, this piece of code have defects, even though it 
>> actually works on Marvell multiple platforms, unless eMMC card is removable.
>>
>>  I can add a property to explicitly indicate eMMC type in DTS.
>>  Then card_candidate access can be removed here.
>>  Does it sounds more reasonable to you?
> 
> Sure
> 
>>
>>>> +  if (mmc_card_mmc(priv->card_candidate))
>>>> +  return xenon_emmc_signal_voltage_switch(mmc, ios);
>>>
>>> So if all you need to know is whether it is a eMMC, why can't DT tell you?
>>>
>>  I can add an eMMC type property in DTS, to remove the card_candidate 
>> access here.
>>
>>>> +  }
>>>> +
>>>> +  return sdhci_start_signal_voltage_switch(mmc, ios);
>>>> +}
>>>> +
>>>> +/*
>>>> + * After determining which slot is used for SDIO,
>>>> + * some additional task is required.
>>>> + */
>>>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>>>> +{
>>>> +  struct sdhci_host *host = mmc_priv(mmc);
>>>> +  u32 reg;
>>>> +  u8 slot_idx;
>>>> +  struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +  struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>> +
>>>> +  /* Link the card for delay adjustment */
>>>> +  priv->card_candidate = card;
>>>
>>> You really need a better way to get the card.  I suggest you take up the
>>> issue with Ulf.  One possibility is to have mmc core set host->card = card
>>> much earlier.
>>>
>>  Could you please tell me if any issue related to card_candidate still 
>> exists, after the card_candidate is removed from 
>> xenon_start_signal_voltage_switch() in above?
>>  It seems that when init_card is called, the structure card has already 
>> been updated and stable in MMC/SD/SDIO initialization sequence.
>>  May I keep it here?
> 
> It works by accident rather than by design.  We can do better.
> 
Could you please tell me some details which are satisfied about 
card_candidate?

I must admit that card_candidate in xenon_start_signal_voltage_switch() 
is imperfect.
But card_candidate in init_card() and later in set_ios() work by 
design, rather than by accident. We did a lot of tests on several platforms.

The structure mmc_card passed in here is a stable one. Thus in my very 
own opinion, it is safe and stable to use mmc_card here.
card_candidate is used only in card initialization. It is not active in 
later transfers after initialization is done.
It will always updated with mmc_card in next card initialization.

>>
>>>> +  /* Set tuning functionality of this slot */
>>>> +  

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Adrian,

On 2016/10/11 20:39, Adrian Hunter wrote:
> On 07/10/16 18:22, Gregory CLEMENT wrote:
>> From: Ziji Hu <huz...@marvell.com>
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji <huz...@marvell.com>
>> Reviewed-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>> ---
>>  MAINTAINERS|1 +-
>>  drivers/mmc/host/Makefile  |2 +-
>>  drivers/mmc/host/sdhci-xenon-phy.c | 1141 +-
>>  drivers/mmc/host/sdhci-xenon-phy.h |  157 -
>>  drivers/mmc/host/sdhci-xenon.c |4 +-
>>  drivers/mmc/host/sdhci-xenon.h |   17 +-
>>  6 files changed, 1321 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 859420e5dfd3..b5673c2ee5f2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7583,6 +7583,7 @@ M: Ziji Hu <huz...@marvell.com>
>>  L:  linux-...@vger.kernel.org
>>  S:  Supported
>>  F:  drivers/mmc/host/sdhci-xenon.*
>> +F:  drivers/mmc/host/sdhci-xenon-phy.*
>>  F:  Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>>  
>>  MATROX FRAMEBUFFER DRIVER
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index 75eaf743486c..4f2854556ff7 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
>>  endif
>>  
>>  obj-$(CONFIG_MMC_SDHCI_XENON)   += sdhci-xenon-driver.o
>> -sdhci-xenon-driver-y+= sdhci-xenon.o
>> +sdhci-xenon-driver-y+= sdhci-xenon.o sdhci-xenon-phy.o
>> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c 
>> b/drivers/mmc/host/sdhci-xenon-phy.c
>> new file mode 100644
>> index ..4eb8fea1bec9
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> 
> 
> 
>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>> +{
>> +int err;
>> +u8 *ext_csd = NULL;
>> +
>> +err = mmc_get_ext_csd(card, _csd);
>> +kfree(ext_csd);
>> +
>> +return err;
>> +}
>> +
>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>> +{
>> +struct mmc_command cmd = {0};
>> +int err;
>> +
>> +cmd.opcode = SD_IO_RW_DIRECT;
>> +cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>> +
>> +err = mmc_wait_for_cmd(card->host, , 0);
>> +if (err)
>> +return err;
>> +
>> +if (cmd.resp[0] & R5_ERROR)
>> +return -EIO;
>> +if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>> +return -EINVAL;
>> +if (cmd.resp[0] & R5_OUT_OF_RANGE)
>> +return -ERANGE;
>> +return 0;
>> +}
>> +
>> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
>> +{
>> +struct mmc_command cmd = {0};
>> +int err;
>> +
>> +cmd.opcode = MMC_SEND_STATUS;
>> +cmd.arg = card->rca << 16;
>> +cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>> +
>> +err = mmc_wait_for_cmd(card->host, , 0);
>> +return err;
>> +}
>> +
>> +static int xenon_delay_adj_test(struct mmc_card *card)
>> +{
>> +WARN_ON(!card);
>> +WARN_ON(!card->host);
>> +
>> +if (mmc_card_mmc(card))
>> +return __xenon_emmc_delay_adj_test(card);
>> +else if (mmc_card_sd(card))
>> +return __xenon_sd_delay_adj_test(card);
>> +else if (mmc_card_sdio(card))
>> +return __xenon_sdio_delay_adj_test(card);
>> +else
>> +return -EINVAL;
>> +}
> 
> So you are issuing commands from the ->set_ios() callback.  I would want to
> get Ulf's OK for that before going further.
> 
Yes, you are correct.
In some speed mode, Xenon SDHC has to send a series of transfers to 
search for a perfect sampling point in PHY delay line.
It is like tuning process.

> One thing: you will need to ensure you don't trigger get HS400 re-tuning
> because it will call back into ->set_ios().
> 
Could you please make the term "HS400 re-tuning" more detaile

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Adrian,

On 2016/10/11 20:39, Adrian Hunter wrote:
> On 07/10/16 18:22, Gregory CLEMENT wrote:
>> From: Ziji Hu 
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji 
>> Reviewed-by: Gregory CLEMENT 
>> Signed-off-by: Gregory CLEMENT 
>> ---
>>  MAINTAINERS|1 +-
>>  drivers/mmc/host/Makefile  |2 +-
>>  drivers/mmc/host/sdhci-xenon-phy.c | 1141 +-
>>  drivers/mmc/host/sdhci-xenon-phy.h |  157 -
>>  drivers/mmc/host/sdhci-xenon.c |4 +-
>>  drivers/mmc/host/sdhci-xenon.h |   17 +-
>>  6 files changed, 1321 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 859420e5dfd3..b5673c2ee5f2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7583,6 +7583,7 @@ M: Ziji Hu 
>>  L:  linux-...@vger.kernel.org
>>  S:  Supported
>>  F:  drivers/mmc/host/sdhci-xenon.*
>> +F:  drivers/mmc/host/sdhci-xenon-phy.*
>>  F:  Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>>  
>>  MATROX FRAMEBUFFER DRIVER
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index 75eaf743486c..4f2854556ff7 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
>>  endif
>>  
>>  obj-$(CONFIG_MMC_SDHCI_XENON)   += sdhci-xenon-driver.o
>> -sdhci-xenon-driver-y+= sdhci-xenon.o
>> +sdhci-xenon-driver-y+= sdhci-xenon.o sdhci-xenon-phy.o
>> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c 
>> b/drivers/mmc/host/sdhci-xenon-phy.c
>> new file mode 100644
>> index ..4eb8fea1bec9
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> 
> 
> 
>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>> +{
>> +int err;
>> +u8 *ext_csd = NULL;
>> +
>> +err = mmc_get_ext_csd(card, _csd);
>> +kfree(ext_csd);
>> +
>> +return err;
>> +}
>> +
>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>> +{
>> +struct mmc_command cmd = {0};
>> +int err;
>> +
>> +cmd.opcode = SD_IO_RW_DIRECT;
>> +cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>> +
>> +err = mmc_wait_for_cmd(card->host, , 0);
>> +if (err)
>> +return err;
>> +
>> +if (cmd.resp[0] & R5_ERROR)
>> +return -EIO;
>> +if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>> +return -EINVAL;
>> +if (cmd.resp[0] & R5_OUT_OF_RANGE)
>> +return -ERANGE;
>> +return 0;
>> +}
>> +
>> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
>> +{
>> +struct mmc_command cmd = {0};
>> +int err;
>> +
>> +cmd.opcode = MMC_SEND_STATUS;
>> +cmd.arg = card->rca << 16;
>> +cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>> +
>> +err = mmc_wait_for_cmd(card->host, , 0);
>> +return err;
>> +}
>> +
>> +static int xenon_delay_adj_test(struct mmc_card *card)
>> +{
>> +WARN_ON(!card);
>> +WARN_ON(!card->host);
>> +
>> +if (mmc_card_mmc(card))
>> +return __xenon_emmc_delay_adj_test(card);
>> +else if (mmc_card_sd(card))
>> +return __xenon_sd_delay_adj_test(card);
>> +else if (mmc_card_sdio(card))
>> +return __xenon_sdio_delay_adj_test(card);
>> +else
>> +return -EINVAL;
>> +}
> 
> So you are issuing commands from the ->set_ios() callback.  I would want to
> get Ulf's OK for that before going further.
> 
Yes, you are correct.
In some speed mode, Xenon SDHC has to send a series of transfers to 
search for a perfect sampling point in PHY delay line.
It is like tuning process.

> One thing: you will need to ensure you don't trigger get HS400 re-tuning
> because it will call back into ->set_ios().
> 
Could you please make the term "HS400 re-tuning" more detailed?
In current MMC driver, "HS400 re-tuning" will go back to HS200, execute 
HS200 tuning and come back to HS400.
I'm sure our Xenon SDHC will not execute it.

However, in coming eMMC 5.2, there is a real HS400 re-tuning, in which 
tuning can be directly executed in HS400 mode.
Our Xenon SDHC will neither trigger this HS400 re-tuning.
But since so far there is no such feature in MMC driver, I cannot give 
you a 100% guarantee now.

> And you have the problem that you need to get a reference to the card before
> the card device has been added.  As I wrote in response to the previous
> patch, you should get Ulf's help with that too.
> 
Sure.
I will get card_candidate solved at first.
Thank you again for your review and help.

Thank you.

Best regards,
Hu Ziji
> 

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Adrian,

Thank you very much for your review.
I will firstly fix the typo.

On 2016/10/11 20:37, Adrian Hunter wrote:
> On 07/10/16 18:22, Gregory CLEMENT wrote:
>> From: Ziji Hu <huz...@marvell.com>
>>
>> Add Xenon eMMC/SD/SDIO host controller core functionality.
>> Add Xenon specific intialization process.
>> Add Xenon specific mmc_host_ops APIs.
>> Add Xenon specific register definitions.
>>
>> Add CONFIG_MMC_SDHCI_XENON support in drivers/mmc/host/Kconfig.
>>
>> Marvell Xenon SDHC conforms to SD Physical Layer Specification
>> Version 3.01 and is designed according to the guidelines provided
>> in the SD Host Controller Standard Specification Version 3.00.
>>
>> Signed-off-by: Hu Ziji <huz...@marvell.com>
>> Reviewed-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
> 
> I looked at a couple of things but you need to sort out the issues with
> card_candidate before going further.
> 
Understood.
I will improve the card_candidate. Please help check the details in 
below.

>> ---

>> +
>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>> +struct mmc_ios *ios)
>> +{
>> +unsigned char voltage = ios->signal_voltage;
>> +
>> +if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>> +(voltage == MMC_SIGNAL_VOLTAGE_180))
>> +return __emmc_signal_voltage_switch(mmc, voltage);
>> +
>> +dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>> +voltage);
>> +return -EINVAL;
>> +}
>> +
>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>> + struct mmc_ios *ios)
>> +{
>> +struct sdhci_host *host = mmc_priv(mmc);
>> +struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +/*
>> + * Before SD/SDIO set signal voltage, SD bus clock should be
>> + * disabled. However, sdhci_set_clock will also disable the Internal
>> + * clock in mmc_set_signal_voltage().
>> + * If Internal clock is disabled, the 3.3V/1.8V bit can not be updated.
>> + * Thus here manually enable internal clock.
>> + *
>> + * After switch completes, it is unnecessary to disable internal clock,
>> + * since keeping internal clock active obeys SD spec.
>> + */
>> +enable_xenon_internal_clk(host);
>> +
>> +if (priv->card_candidate) {
> 
> mmc_power_up() calls __mmc_set_signal_voltage() calls
> host->ops->start_signal_voltage_switch so priv->card_candidate could be an
> invalid reference to an old card.
> 
> So that's not going to work if the card changes - not only for removable
> cards but even for eMMC if init fails and retries.
> 
As you point out, this piece of code have defects, even though it 
actually works on Marvell multiple platforms, unless eMMC card is removable.

I can add a property to explicitly indicate eMMC type in DTS.
Then card_candidate access can be removed here.
Does it sounds more reasonable to you?

>> +if (mmc_card_mmc(priv->card_candidate))
>> +return xenon_emmc_signal_voltage_switch(mmc, ios);
> 
> So if all you need to know is whether it is a eMMC, why can't DT tell you?
> 
I can add an eMMC type property in DTS, to remove the card_candidate 
access here.

>> +}
>> +
>> +return sdhci_start_signal_voltage_switch(mmc, ios);
>> +}
>> +
>> +/*
>> + * After determining which slot is used for SDIO,
>> + * some additional task is required.
>> + */
>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>> +{
>> +struct sdhci_host *host = mmc_priv(mmc);
>> +u32 reg;
>> +u8 slot_idx;
>> +struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +/* Link the card for delay adjustment */
>> +priv->card_candidate = card;
> 
> You really need a better way to get the card.  I suggest you take up the
> issue with Ulf.  One possibility is to have mmc core set host->card = card
> much earlier.
> 
Could you please tell me if any issue related to card_candidate still 
exists, after the card_candidate is removed from 
xenon_start_signal_voltage_switch() in above?
It see

Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu]

Hi Adrian,

Thank you very much for your review.
I will firstly fix the typo.

On 2016/10/11 20:37, Adrian Hunter wrote:
> On 07/10/16 18:22, Gregory CLEMENT wrote:
>> From: Ziji Hu 
>>
>> Add Xenon eMMC/SD/SDIO host controller core functionality.
>> Add Xenon specific intialization process.
>> Add Xenon specific mmc_host_ops APIs.
>> Add Xenon specific register definitions.
>>
>> Add CONFIG_MMC_SDHCI_XENON support in drivers/mmc/host/Kconfig.
>>
>> Marvell Xenon SDHC conforms to SD Physical Layer Specification
>> Version 3.01 and is designed according to the guidelines provided
>> in the SD Host Controller Standard Specification Version 3.00.
>>
>> Signed-off-by: Hu Ziji 
>> Reviewed-by: Gregory CLEMENT 
>> Signed-off-by: Gregory CLEMENT 
> 
> I looked at a couple of things but you need to sort out the issues with
> card_candidate before going further.
> 
Understood.
I will improve the card_candidate. Please help check the details in 
below.

>> ---

>> +
>> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
>> +struct mmc_ios *ios)
>> +{
>> +unsigned char voltage = ios->signal_voltage;
>> +
>> +if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
>> +(voltage == MMC_SIGNAL_VOLTAGE_180))
>> +return __emmc_signal_voltage_switch(mmc, voltage);
>> +
>> +dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
>> +voltage);
>> +return -EINVAL;
>> +}
>> +
>> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
>> + struct mmc_ios *ios)
>> +{
>> +struct sdhci_host *host = mmc_priv(mmc);
>> +struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +/*
>> + * Before SD/SDIO set signal voltage, SD bus clock should be
>> + * disabled. However, sdhci_set_clock will also disable the Internal
>> + * clock in mmc_set_signal_voltage().
>> + * If Internal clock is disabled, the 3.3V/1.8V bit can not be updated.
>> + * Thus here manually enable internal clock.
>> + *
>> + * After switch completes, it is unnecessary to disable internal clock,
>> + * since keeping internal clock active obeys SD spec.
>> + */
>> +enable_xenon_internal_clk(host);
>> +
>> +if (priv->card_candidate) {
> 
> mmc_power_up() calls __mmc_set_signal_voltage() calls
> host->ops->start_signal_voltage_switch so priv->card_candidate could be an
> invalid reference to an old card.
> 
> So that's not going to work if the card changes - not only for removable
> cards but even for eMMC if init fails and retries.
> 
As you point out, this piece of code have defects, even though it 
actually works on Marvell multiple platforms, unless eMMC card is removable.

I can add a property to explicitly indicate eMMC type in DTS.
Then card_candidate access can be removed here.
Does it sounds more reasonable to you?

>> +if (mmc_card_mmc(priv->card_candidate))
>> +return xenon_emmc_signal_voltage_switch(mmc, ios);
> 
> So if all you need to know is whether it is a eMMC, why can't DT tell you?
> 
I can add an eMMC type property in DTS, to remove the card_candidate 
access here.

>> +}
>> +
>> +return sdhci_start_signal_voltage_switch(mmc, ios);
>> +}
>> +
>> +/*
>> + * After determining which slot is used for SDIO,
>> + * some additional task is required.
>> + */
>> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
>> +{
>> +struct sdhci_host *host = mmc_priv(mmc);
>> +u32 reg;
>> +u8 slot_idx;
>> +struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +/* Link the card for delay adjustment */
>> +priv->card_candidate = card;
> 
> You really need a better way to get the card.  I suggest you take up the
> issue with Ulf.  One possibility is to have mmc core set host->card = card
> much earlier.
> 
Could you please tell me if any issue related to card_candidate still 
exists, after the card_candidate is removed from 
xenon_start_signal_voltage_switch() in above?
It seems that when init_card is called, the structure card has already 
been updated and stable in MMC/SD/SDIO initialization sequence.
May I keep i

Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

[Ziji Hu]

Hi Rob,

Thanks a for the review.
It is really helpful to me.

On 2016/10/11 5:34, Rob Herring wrote:
> On Fri, Oct 07, 2016 at 05:22:51PM +0200, Gregory CLEMENT wrote:
>> From: Ziji Hu <huz...@marvell.com>
>>
>> Marvell Xenon SDHC can support eMMC/SD/SDIO.
>> Add Xenon-specific properties.
>> Also add properties for Xenon PHY setting.
>>
>> Signed-off-by: Hu Ziji <huz...@marvell.com>
>> Reviewed-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>> ---
>>  Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt | 164 +++-
>>  MAINTAINERS   |   1 +-
>>  2 files changed, 165 insertions(+), 0 deletions(-)
>>  create mode 100644 
>> Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt 
>> b/Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>> new file mode 100644
>> index ..8b25ad28ebbd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>> @@ -0,0 +1,164 @@
>> +Marvell's Xenon SDHCI Controller device tree bindings
>> +This file documents differences between the core mmc properties
>> +described by mmc.txt and the properties used by the Xenon implementation.
>> +
>> +A single Xenon IP can support multiple slots.
>> +Each slot acts as an independent SDHC. It owns independent resources, such
>> +as register sets clock and PHY.
> 
> Is the phy really part of the same block?
> 
Each SDHC slot owns its PHY. It is part of a SDHC slot.
It is independent to another SDHC slot.

>> +Each slot should have an independent device tree node.
>> +
>> +Required Properties:
>> +- compatible: should be "marvell,sdhci-xenon" or 
>> "marvell,armada-3700-sdhci".
> 
> Perhaps some consistent ordering (w/ -sdhci on the end).
Sure.
I will adjust the ordering.

> 
>> +
>> +- Input Clock Name
> 
> Your formatting of properties is a bit strange. Please restructure like 
> most bindings so the property names are before all the description.
> 
OK.
I will fix the format.

>> +  Some SOCs require additional clock for AXI bus.
> 
> Those SoCs should have a specific compatible string and you need to 
> define which compatible strings have 2 clocks vs. 1 clock.
> 
Actually, I copy this implementation from another Marvell SDIO Host 
Controller, sdhci-pxa.
It is in sdhci-pxa.txt.
I would like to know if it is still acceptable.

>> +  The input clock for Xenon IP core should be named as "core".
>> +  The optional AXI clock should be named as "axi".
>> +  - clocks = <_clk>, <_clock>;
>> +  - clock-names = "core", "axi";
>> +
>> +- Register Set Size
> 
> Is this a property name?

Sorry, it isn't.
I will fix the format.

> 
>> +  Different Xenon SDHC release has different register set size.
>> +  The specific size should also refer to the SOC implementation.
>> +
>> +Optional Properties:
>> +- Slot Index
>> +  A single Xenon IP can support multiple slots.
>> +  During initialization, each slot should set corresponding setting bit in
>> +  some Xenon-specific registers. The corresponding bit is determined by
>> +  this property.
>> +  - xenon,slotno = ;
> 
> Slots should probably be represented as child nodes with the reg 
> property being the slot number.

Since each SDHC slot is independent, I find it is more convenient to 
implement each one as independent SD host/MMC host instant.
Otherwise, a main function should loop and initialize each slot, like 
sdhci-pci. I prefer to avoiding such a unnecessary main function.

It is very hard to determine the slot number by reg property.
Xenon slots are likely to be different types. 1st slot might be eMMC 
and 2nd one might be SD. They might have different register size.
The register size might also varies in different Xenon versions.

> 
> Also, xenon is not a vendor prefix.
> 
Yes. The issue is that there are multiple Marvell SD Host Controllers 
existing in kernel.
If marvell is used as a prefix here, I concern that it might be 
confused with other Marvell sdhc.
Can I use a combination of marvell and xenon as a prefix, such as 
mrvl-xenon?

>> +  If this property is not provided, Xenon IP should contain only one slot
>> +  and the slot index will be 0x0 by

Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

[Ziji Hu]

Hi Rob,

Thanks a for the review.
It is really helpful to me.

On 2016/10/11 5:34, Rob Herring wrote:
> On Fri, Oct 07, 2016 at 05:22:51PM +0200, Gregory CLEMENT wrote:
>> From: Ziji Hu 
>>
>> Marvell Xenon SDHC can support eMMC/SD/SDIO.
>> Add Xenon-specific properties.
>> Also add properties for Xenon PHY setting.
>>
>> Signed-off-by: Hu Ziji 
>> Reviewed-by: Gregory CLEMENT 
>> Signed-off-by: Gregory CLEMENT 
>> ---
>>  Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt | 164 +++-
>>  MAINTAINERS   |   1 +-
>>  2 files changed, 165 insertions(+), 0 deletions(-)
>>  create mode 100644 
>> Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt 
>> b/Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>> new file mode 100644
>> index ..8b25ad28ebbd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>> @@ -0,0 +1,164 @@
>> +Marvell's Xenon SDHCI Controller device tree bindings
>> +This file documents differences between the core mmc properties
>> +described by mmc.txt and the properties used by the Xenon implementation.
>> +
>> +A single Xenon IP can support multiple slots.
>> +Each slot acts as an independent SDHC. It owns independent resources, such
>> +as register sets clock and PHY.
> 
> Is the phy really part of the same block?
> 
Each SDHC slot owns its PHY. It is part of a SDHC slot.
It is independent to another SDHC slot.

>> +Each slot should have an independent device tree node.
>> +
>> +Required Properties:
>> +- compatible: should be "marvell,sdhci-xenon" or 
>> "marvell,armada-3700-sdhci".
> 
> Perhaps some consistent ordering (w/ -sdhci on the end).
Sure.
I will adjust the ordering.

> 
>> +
>> +- Input Clock Name
> 
> Your formatting of properties is a bit strange. Please restructure like 
> most bindings so the property names are before all the description.
> 
OK.
I will fix the format.

>> +  Some SOCs require additional clock for AXI bus.
> 
> Those SoCs should have a specific compatible string and you need to 
> define which compatible strings have 2 clocks vs. 1 clock.
> 
Actually, I copy this implementation from another Marvell SDIO Host 
Controller, sdhci-pxa.
It is in sdhci-pxa.txt.
I would like to know if it is still acceptable.

>> +  The input clock for Xenon IP core should be named as "core".
>> +  The optional AXI clock should be named as "axi".
>> +  - clocks = <_clk>, <_clock>;
>> +  - clock-names = "core", "axi";
>> +
>> +- Register Set Size
> 
> Is this a property name?

Sorry, it isn't.
I will fix the format.

> 
>> +  Different Xenon SDHC release has different register set size.
>> +  The specific size should also refer to the SOC implementation.
>> +
>> +Optional Properties:
>> +- Slot Index
>> +  A single Xenon IP can support multiple slots.
>> +  During initialization, each slot should set corresponding setting bit in
>> +  some Xenon-specific registers. The corresponding bit is determined by
>> +  this property.
>> +  - xenon,slotno = ;
> 
> Slots should probably be represented as child nodes with the reg 
> property being the slot number.

Since each SDHC slot is independent, I find it is more convenient to 
implement each one as independent SD host/MMC host instant.
Otherwise, a main function should loop and initialize each slot, like 
sdhci-pci. I prefer to avoiding such a unnecessary main function.

It is very hard to determine the slot number by reg property.
Xenon slots are likely to be different types. 1st slot might be eMMC 
and 2nd one might be SD. They might have different register size.
The register size might also varies in different Xenon versions.

> 
> Also, xenon is not a vendor prefix.
> 
Yes. The issue is that there are multiple Marvell SD Host Controllers 
existing in kernel.
If marvell is used as a prefix here, I concern that it might be 
confused with other Marvell sdhc.
Can I use a combination of marvell and xenon as a prefix, such as 
mrvl-xenon?

>> +  If this property is not provided, Xenon IP should contain only one slot
>> +  and the slot index will be 0x0 by default.
>> +
>> +- PHY Type
> 
> You're going to need to come of with a common binding for this.
> 
Could you plea

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Shawn,

On 2016/10/8 10:44, Shawn Lin wrote:
> 在 2016/10/7 23:22, Gregory CLEMENT 写道:
>> From: Ziji Hu <huz...@marvell.com>
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji <huz...@marvell.com>
>> Reviewed-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>> ---
>>  MAINTAINERS|1 +-
>>  drivers/mmc/host/Makefile  |2 +-
>>  drivers/mmc/host/sdhci-xenon-phy.c | 1141 +-
>>  drivers/mmc/host/sdhci-xenon-phy.h |  157 -
>>  drivers/mmc/host/sdhci-xenon.c |4 +-
>>  drivers/mmc/host/sdhci-xenon.h |   17 +-
>>  6 files changed, 1321 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 859420e5dfd3..b5673c2ee5f2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7583,6 +7583,7 @@ M:Ziji Hu <huz...@marvell.com>
>>  L:linux-...@vger.kernel.org
>>  S:Supported
>>  F:drivers/mmc/host/sdhci-xenon.*
>> +F:drivers/mmc/host/sdhci-xenon-phy.*
> 
> drivers/mmc/host/sdhci-xenon* shoube enough
> 
>>  F:Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>>
>>  MATROX FRAMEBUFFER DRIVER
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index 75eaf743486c..4f2854556ff7 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
>>  endif
>>
>>  obj-$(CONFIG_MMC_SDHCI_XENON)+= sdhci-xenon-driver.o
>> -sdhci-xenon-driver-y+= sdhci-xenon.o
>> +sdhci-xenon-driver-y+= sdhci-xenon.o sdhci-xenon-phy.o
>> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c 
>> b/drivers/mmc/host/sdhci-xenon-phy.c
>> new file mode 100644
>> index ..4eb8fea1bec9
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> 
> Well, it's legit to use phy API and move your phy
> operations to PHY subsystem. :)
> 

Actually we tried to put the PHY code into Linux PHY framework.
But it cannot fit in Linux common PHY framework.

Our Xenon SDHC PHY register is a part of Xenon SDHC register set.
Besides, during MMC initialization, MMC sequence has to call several PHY 
functions to complete timing setting.
In those PHY setting functions, they have to access SDHC register and know 
current MMC setting, such as bus width, clock frequency and speed mode.
As a result, we have to implement PHY under MMC directory.

Thank you.

Best regards,
Hu Ziji

>> @@ -0,0 +1,1141 @@
>> +/*
>> + * PHY support for Xenon SDHC
>> + *
>> + * Copyright (C) 2016 Marvell, All Rights Reserved.
>> + *
>> + * Author:Hu Ziji <huz...@marvell.com>
>> + * Date:2016-8-24
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "sdhci.h"
>> +#include "sdhci-pltfm.h"
>> +#include "sdhci-xenon.h"
>> +
>> +static const char * const phy_types[] = {
>> +"sdh phy",
>> +"emmc 5.0 phy",
>> +"emmc 5.1 phy"
>> +};
>> +
>> +enum phy_type_enum {
>> +SDH_PHY,
>> +EMMC_5_0_PHY,
>> +EMMC_5_1_PHY,
>> +NR_PHY_TYPES
>> +};
>> +
>> +struct soc_pad_ctrl_table {
>> +const char *soc;
>> +void (*set_soc_pad)(struct sdhci_host *host,
>> +unsigned char signal_voltage);
>> +};
>> +
>> +struct soc_pad_ctrl {
>> +/* Register address of SOC PHY PAD ctrl */
>> +void __iomem*reg;
>> +/* SOC PHY PAD ctrl type */
>> +enum soc_pad_ctrl_type pad_type;
>> +/* SOC specific operation to set SOC PHY PAD */
>> +void (*set_soc_pad)(struct sdhci_host *host,
>> +unsigned char signal_voltage);
>> +};
>> +
>> +static struct xenon_emmc_phy_regs  x

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Shawn,

On 2016/10/8 10:44, Shawn Lin wrote:
> 在 2016/10/7 23:22, Gregory CLEMENT 写道:
>> From: Ziji Hu 
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji 
>> Reviewed-by: Gregory CLEMENT 
>> Signed-off-by: Gregory CLEMENT 
>> ---
>>  MAINTAINERS|1 +-
>>  drivers/mmc/host/Makefile  |2 +-
>>  drivers/mmc/host/sdhci-xenon-phy.c | 1141 +-
>>  drivers/mmc/host/sdhci-xenon-phy.h |  157 -
>>  drivers/mmc/host/sdhci-xenon.c |4 +-
>>  drivers/mmc/host/sdhci-xenon.h |   17 +-
>>  6 files changed, 1321 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 859420e5dfd3..b5673c2ee5f2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7583,6 +7583,7 @@ M:Ziji Hu 
>>  L:linux-...@vger.kernel.org
>>  S:Supported
>>  F:drivers/mmc/host/sdhci-xenon.*
>> +F:drivers/mmc/host/sdhci-xenon-phy.*
> 
> drivers/mmc/host/sdhci-xenon* shoube enough
> 
>>  F:Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>>
>>  MATROX FRAMEBUFFER DRIVER
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index 75eaf743486c..4f2854556ff7 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
>>  endif
>>
>>  obj-$(CONFIG_MMC_SDHCI_XENON)+= sdhci-xenon-driver.o
>> -sdhci-xenon-driver-y+= sdhci-xenon.o
>> +sdhci-xenon-driver-y+= sdhci-xenon.o sdhci-xenon-phy.o
>> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c 
>> b/drivers/mmc/host/sdhci-xenon-phy.c
>> new file mode 100644
>> index ..4eb8fea1bec9
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> 
> Well, it's legit to use phy API and move your phy
> operations to PHY subsystem. :)
> 

Actually we tried to put the PHY code into Linux PHY framework.
But it cannot fit in Linux common PHY framework.

Our Xenon SDHC PHY register is a part of Xenon SDHC register set.
Besides, during MMC initialization, MMC sequence has to call several PHY 
functions to complete timing setting.
In those PHY setting functions, they have to access SDHC register and know 
current MMC setting, such as bus width, clock frequency and speed mode.
As a result, we have to implement PHY under MMC directory.

Thank you.

Best regards,
Hu Ziji

>> @@ -0,0 +1,1141 @@
>> +/*
>> + * PHY support for Xenon SDHC
>> + *
>> + * Copyright (C) 2016 Marvell, All Rights Reserved.
>> + *
>> + * Author:Hu Ziji 
>> + * Date:2016-8-24
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "sdhci.h"
>> +#include "sdhci-pltfm.h"
>> +#include "sdhci-xenon.h"
>> +
>> +static const char * const phy_types[] = {
>> +"sdh phy",
>> +"emmc 5.0 phy",
>> +"emmc 5.1 phy"
>> +};
>> +
>> +enum phy_type_enum {
>> +SDH_PHY,
>> +EMMC_5_0_PHY,
>> +EMMC_5_1_PHY,
>> +NR_PHY_TYPES
>> +};
>> +
>> +struct soc_pad_ctrl_table {
>> +const char *soc;
>> +void (*set_soc_pad)(struct sdhci_host *host,
>> +unsigned char signal_voltage);
>> +};
>> +
>> +struct soc_pad_ctrl {
>> +/* Register address of SOC PHY PAD ctrl */
>> +void __iomem*reg;
>> +/* SOC PHY PAD ctrl type */
>> +enum soc_pad_ctrl_type pad_type;
>> +/* SOC specific operation to set SOC PHY PAD */
>> +void (*set_soc_pad)(struct sdhci_host *host,
>> +unsigned char signal_voltage);
>> +};
>> +
>> +static struct xenon_emmc_phy_regs  xenon_emmc_5_0_phy_regs = {
>> +.timing_adj= EMMC_5_0_PHY_TIMING_ADJUST,
>> +.func_ctrl= EMMC_5_0_PHY_FUNC_CONTROL,
>> +.pad_ctrl= EMMC_5_0_PH

Re: [PATCH 2/10] mmc: sdhci: Export sdhci_start_signal_voltage_switch() in sdhci.c

[Ziji Hu]

Hi Shawn,

On 2016/10/8 10:40, Shawn Lin wrote:
> Hi,
> 
> 在 2016/10/7 23:22, Gregory CLEMENT 写道:
>> From: Ziji Hu <huz...@marvell.com>
>>
>> Export sdhci_start_signal_voltage_switch() from sdhci.c.
>> Thus vendor sdhci driver can implement its own signal voltage
>> switch routine.
>>
> 
> You can overwtite this callback within your driver itself.
> That is what other sdhci variant drivers did, so patch 1-3 are
> unnecessary.

Thanks a lot for your reply.

Our SDHC driver just requests some pre- and post- operations besides common 
standard SDHC functions.
Overwriting those common functions costs too much.

Thank you.

Best regards,
Hu Ziji

> 
>> Signed-off-by: Hu Ziji <huz...@marvell.com>
>> Reviewed-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
>> ---
>>  drivers/mmc/host/sdhci.c | 5 +++--
>>  drivers/mmc/host/sdhci.h | 2 ++
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index d4bb818c52d5..2250ea22231f 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1828,8 +1828,8 @@ static void sdhci_enable_sdio_irq(struct mmc_host 
>> *mmc, int enable)
>>  spin_unlock_irqrestore(>lock, flags);
>>  }
>>
>> -static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>> - struct mmc_ios *ios)
>> +int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>> +  struct mmc_ios *ios)
>>  {
>>  struct sdhci_host *host = mmc_priv(mmc);
>>  u16 ctrl;
>> @@ -1921,6 +1921,7 @@ static int sdhci_start_signal_voltage_switch(struct 
>> mmc_host *mmc,
>>  return 0;
>>  }
>>  }
>> +EXPORT_SYMBOL_GPL(sdhci_start_signal_voltage_switch);
>>
>>  static int sdhci_card_busy(struct mmc_host *mmc)
>>  {
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 21dc80b8ae3d..c38ab65b9a97 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -687,6 +687,8 @@ void sdhci_set_bus_width(struct sdhci_host *host, int 
>> width);
>>  void sdhci_reset(struct sdhci_host *host, u8 mask);
>>  void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
>>  void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios);
>> +int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>> +  struct mmc_ios *ios);
>>
>>  #ifdef CONFIG_PM
>>  extern int sdhci_suspend_host(struct sdhci_host *host);
>>
> 
> 

Re: [PATCH 2/10] mmc: sdhci: Export sdhci_start_signal_voltage_switch() in sdhci.c

[Ziji Hu]

Hi Shawn,

On 2016/10/8 10:40, Shawn Lin wrote:
> Hi,
> 
> 在 2016/10/7 23:22, Gregory CLEMENT 写道:
>> From: Ziji Hu 
>>
>> Export sdhci_start_signal_voltage_switch() from sdhci.c.
>> Thus vendor sdhci driver can implement its own signal voltage
>> switch routine.
>>
> 
> You can overwtite this callback within your driver itself.
> That is what other sdhci variant drivers did, so patch 1-3 are
> unnecessary.

Thanks a lot for your reply.

Our SDHC driver just requests some pre- and post- operations besides common 
standard SDHC functions.
Overwriting those common functions costs too much.

Thank you.

Best regards,
Hu Ziji

> 
>> Signed-off-by: Hu Ziji 
>> Reviewed-by: Gregory CLEMENT 
>> Signed-off-by: Gregory CLEMENT 
>> ---
>>  drivers/mmc/host/sdhci.c | 5 +++--
>>  drivers/mmc/host/sdhci.h | 2 ++
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index d4bb818c52d5..2250ea22231f 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1828,8 +1828,8 @@ static void sdhci_enable_sdio_irq(struct mmc_host 
>> *mmc, int enable)
>>  spin_unlock_irqrestore(>lock, flags);
>>  }
>>
>> -static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>> - struct mmc_ios *ios)
>> +int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>> +  struct mmc_ios *ios)
>>  {
>>  struct sdhci_host *host = mmc_priv(mmc);
>>  u16 ctrl;
>> @@ -1921,6 +1921,7 @@ static int sdhci_start_signal_voltage_switch(struct 
>> mmc_host *mmc,
>>  return 0;
>>  }
>>  }
>> +EXPORT_SYMBOL_GPL(sdhci_start_signal_voltage_switch);
>>
>>  static int sdhci_card_busy(struct mmc_host *mmc)
>>  {
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 21dc80b8ae3d..c38ab65b9a97 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -687,6 +687,8 @@ void sdhci_set_bus_width(struct sdhci_host *host, int 
>> width);
>>  void sdhci_reset(struct sdhci_host *host, u8 mask);
>>  void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
>>  void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios);
>> +int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>> +  struct mmc_ios *ios);
>>
>>  #ifdef CONFIG_PM
>>  extern int sdhci_suspend_host(struct sdhci_host *host);
>>
> 
> 

Re: [PATCH 4/10] MAINTAINERS: add entry for Marvell Xenon MMC Host Controller drivers

[Ziji Hu]

Hi Joe,

On 2016/10/8 4:44, Joe Perches wrote:
> On Fri, 2016-10-07 at 17:22 +0200, Gregory CLEMENT wrote:
>> Add maintainer entry for Marvell Xenon eMMC/SD/SDIO Host
>> Controller drivers.
> []
>> diff --git a/MAINTAINERS b/MAINTAINERS
> []
>> @@ -7578,6 +7578,11 @@ M:Nicolas Pitre <n...@fluxnic.net>
>>  S:  Odd Fixes
>>  F:  drivers/mmc/host/mvsdio.*
>>  
>> +MARVELL XENON MMC/SD/SDIO HOST CONTROLLER DRIVER
>> +M:  Ziji Hu <huz...@marvell.com>
>> +L:  linux-...@vger.kernel.org
>> +S:  Supported
> 
> You should really add F: file patterns here
> 
The specific file patterns will be added later when the corresponding 
file is included in patch.
Otherwise, it cannot pass checkpatch.pl.

Thank you.

Best regards,
Hu Ziji

Re: [PATCH 4/10] MAINTAINERS: add entry for Marvell Xenon MMC Host Controller drivers

[Ziji Hu]

Hi Joe,

On 2016/10/8 4:44, Joe Perches wrote:
> On Fri, 2016-10-07 at 17:22 +0200, Gregory CLEMENT wrote:
>> Add maintainer entry for Marvell Xenon eMMC/SD/SDIO Host
>> Controller drivers.
> []
>> diff --git a/MAINTAINERS b/MAINTAINERS
> []
>> @@ -7578,6 +7578,11 @@ M:Nicolas Pitre 
>>  S:  Odd Fixes
>>  F:  drivers/mmc/host/mvsdio.*
>>  
>> +MARVELL XENON MMC/SD/SDIO HOST CONTROLLER DRIVER
>> +M:  Ziji Hu 
>> +L:  linux-...@vger.kernel.org
>> +S:  Supported
> 
> You should really add F: file patterns here
> 
The specific file patterns will be added later when the corresponding 
file is included in patch.
Otherwise, it cannot pass checkpatch.pl.

Thank you.

Best regards,
Hu Ziji