Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1529220 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell  Xenon SDHC Ulf Hansson <ulf.hansson@linaro.org> - 2016-11-24 12:40 +0100
    Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell  Xenon SDHC Ziji Hu <huziji@marvell.com> - 2016-11-24 14:40 +0100
      Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell  Xenon SDHC Ulf Hansson <ulf.hansson@linaro.org> - 2016-11-24 15:40 +0100
        Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell  Xenon SDHC Ziji Hu <huziji@marvell.com> - 2016-11-24 16:40 +0100

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

On 31 October 2016 at 12:09, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> From: Ziji Hu <huziji@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 <huziji@marvell.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@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...

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, &ext_csd);
> +       kfree(ext_csd);

Why do you read the ext csd here?

> +
> +       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, &cmd, 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.

> +}
> +
> +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, &cmd, 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) &&
> +           (ios->bus_width == priv->bus_width) &&
> +           (ios->timing == priv->timing))
> +               return 0;
> +
> +       xenon_phy_set(host, ios->timing);
> +
> +       /* Update the record */
> +       priv->bus_width = ios->bus_width;
> +       /* Temp stage from HS200 to HS400 */
> +       if (((priv->timing == MMC_TIMING_MMC_HS200) &&
> +            (ios->timing == MMC_TIMING_MMC_HS)) ||
> +           ((ios->timing == MMC_TIMING_MMC_HS) &&
> +            (priv->clock > host->clock))) {
> +               priv->timing = ios->timing;
> +               priv->clock = host->clock;
> +               return 0;
> +       }
> +       /*
> +        * Skip 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) ||
> +             (ios->timing == MMC_TIMING_MMC_DDR52))) ||
> +           ((priv->timing == MMC_TIMING_MMC_DDR52) &&
> +            (ios->timing == MMC_TIMING_MMC_HS)) ||
> +           ((ios->timing == MMC_TIMING_MMC_HS200) &&
> +            (ios->clock == MMC_HIGH_52_MAX_DTR))) {
> +               priv->timing = ios->timing;
> +               priv->clock = host->clock;
> +               return 0;
> +       }
> +       priv->timing = ios->timing;
> +       priv->clock = host->clock;
> +
> +       /* Legacy mode is a special case */
> +       if (ios->timing == MMC_TIMING_LEGACY)
> +               return 0;
> +
> +       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.

[...]

Another suggestion of a general improvement; could you perhaps try to
add some brief information about what goes on in function headers.
Perhaps that could help to more easily understand things.

Kind regards
Uffe

[toc] | [next] | [standalone]

#1529285

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.clement@free-electrons.com> wrote:
>> From: Ziji Hu <huziji@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 <huziji@marvell.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@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, &ext_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, &cmd, 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, &cmd, 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) &&
>> +           (ios->bus_width == priv->bus_width) &&
>> +           (ios->timing == priv->timing))
>> +               return 0;
>> +
>> +       xenon_phy_set(host, ios->timing);
>> +
>> +       /* Update the record */
>> +       priv->bus_width = ios->bus_width;
>> +       /* Temp stage from HS200 to HS400 */
>> +       if (((priv->timing == MMC_TIMING_MMC_HS200) &&
>> +            (ios->timing == MMC_TIMING_MMC_HS)) ||
>> +           ((ios->timing == MMC_TIMING_MMC_HS) &&
>> +            (priv->clock > host->clock))) {
>> +               priv->timing = ios->timing;
>> +               priv->clock = host->clock;
>> +               return 0;
>> +       }
>> +       /*
>> +        * Skip 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) ||
>> +             (ios->timing == MMC_TIMING_MMC_DDR52))) ||
>> +           ((priv->timing == MMC_TIMING_MMC_DDR52) &&
>> +            (ios->timing == MMC_TIMING_MMC_HS)) ||
>> +           ((ios->timing == MMC_TIMING_MMC_HS200) &&
>> +            (ios->clock == MMC_HIGH_52_MAX_DTR))) {
>> +               priv->timing = ios->timing;
>> +               priv->clock = host->clock;
>> +               return 0;
>> +       }
>> +       priv->timing = ios->timing;
>> +       priv->clock = host->clock;
>> +
>> +       /* Legacy mode is a special case */
>> +       if (ios->timing == MMC_TIMING_LEGACY)
>> +               return 0;
>> +
>> +       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.

    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?
 
> [...]
> 
> Another suggestion of a general improvement; could you perhaps try to
> add some brief information about what goes on in function headers.
> Perhaps that could help to more easily understand things.
>
   Sorry about any inconvenience. Most of the functions here are our host specific.
   It is really difficult to understand them without proper comment.
   I will add more information.

   Thank you.

Best regards,
Hu Ziji
 
> Kind regards
> Uffe
> 

[toc] | [prev] | [next] | [standalone]

#1529340

[...]

>>
>>>
>>> +
>>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>>> +{
>>> +       int err;
>>> +       u8 *ext_csd = NULL;
>>> +
>>> +       err = mmc_get_ext_csd(card, &ext_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.

>
>    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?

>
>    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!?

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

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.

>
>    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().

>
>>> +
>>> +       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, &cmd, 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.

[...]

Kind regards
Uffe

[toc] | [prev] | [next] | [standalone]

#1529496

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, &ext_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, &cmd, 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
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web