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

Groups > linux.kernel > #1567323 > unrolled thread

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

Started byUlf Hansson <ulf.hansson@linaro.org>
First post2017-01-26 12:00 +0100
Last post2017-01-30 16:40 +0100
Articles 10 — 3 participants

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 v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality Ulf Hansson <ulf.hansson@linaro.org> - 2017-01-26 12:00 +0100
    Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core  functionality Adrian Hunter <adrian.hunter@intel.com> - 2017-01-26 13:50 +0100
      Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality Ulf Hansson <ulf.hansson@linaro.org> - 2017-01-27 16:20 +0100
        Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core  functionality Adrian Hunter <adrian.hunter@intel.com> - 2017-01-28 09:20 +0100
          Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality Ulf Hansson <ulf.hansson@linaro.org> - 2017-01-30 10:20 +0100
            Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core  functionality Adrian Hunter <adrian.hunter@intel.com> - 2017-01-30 10:50 +0100
              Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality Ulf Hansson <ulf.hansson@linaro.org> - 2017-01-30 11:20 +0100
    Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core  functionality Ziji Hu <huziji@marvell.com> - 2017-01-27 17:50 +0100
      Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality Ulf Hansson <ulf.hansson@linaro.org> - 2017-01-30 09:50 +0100
        Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core  functionality Ziji Hu <huziji@marvell.com> - 2017-01-30 16:40 +0100

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

FromUlf Hansson <ulf.hansson@linaro.org>
Date2017-01-26 12:00 +0100
SubjectRe: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
Message-ID<t3PWx-6it-13@gated-at.bofh.it>
On 11 January 2017 at 18:19, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> From: Hu Ziji <huziji@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 <huziji@marvell.com>
> Tested-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  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.

[...]

> +
> +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.

> +               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.

> +                       priv->init_card_type = MMC_TYPE_MMC;
> +                       mmc->caps |= MMC_CAP_NONREMOVABLE;
> +
> +                       /*
> +                        * Force to clear BUS_TEST to
> +                        * skip bus_test_pre and bus_test_post
> +                        */
> +                       mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
> +                       mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |

This cap is a bit strange. It was added several years ago by Adrian
Hunter, but I am wondering about the reason to why it's needed.

Should it be removed and instead enabled per default? If not, should
we invent a new specific DT binding for it?

I need Adrian's help here to understand the options.

> +                                     MMC_CAP2_PACKED_CMD |

The MMC_CAP2_PACKED_CMD cap has be removed.

> +                                     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.

[...]

> +       priv->sdhc_id = 0x0;
> +       if (!of_property_read_u32(np, "marvell,xenon-sdhc-id", &sdhc_id)) {
> +               nr_sdhc = sdhci_readl(host, XENON_SYS_CFG_INFO);
> +               nr_sdhc &= XENON_NR_SUPPORTED_SLOT_MASK;
> +               if (unlikely(sdhc_id > nr_sdhc)) {
> +                       dev_err(mmc_dev(mmc), "SDHC Index %d exceeds Number of SDHCs %d\n",
> +                               sdhc_id, nr_sdhc);
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       tuning_count = XENON_DEF_TUNING_COUNT;
> +       if (!of_property_read_u32(np, "marvell,xenon-tun-count",
> +                                 &tuning_count)) {
> +               if (unlikely(tuning_count >= XENON_TMR_RETUN_NO_PRESENT)) {
> +                       dev_err(mmc_dev(mmc), "Wrong Re-tuning Count. Set default value %d\n",
> +                               XENON_DEF_TUNING_COUNT);
> +                       tuning_count = XENON_DEF_TUNING_COUNT;
> +               }
> +       }

I suggest you move the parsing of the xenon specific bindings into a
separate function.

> +       priv->tuning_count = tuning_count;
> +
> +       return err;
> +}
> +

[...]

Kind regards
Uffe

[toc] | [next] | [standalone]

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

FromAdrian Hunter <adrian.hunter@intel.com>
Date2017-01-26 13:50 +0100
SubjectRe: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
Message-ID<t3REZ-7lZ-3@gated-at.bofh.it>
In reply to#1567323
On 26/01/17 12:50, Ulf Hansson wrote:
> On 11 January 2017 at 18:19, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> +                       priv->init_card_type = MMC_TYPE_MMC;
>> +                       mmc->caps |= MMC_CAP_NONREMOVABLE;
>> +
>> +                       /*
>> +                        * Force to clear BUS_TEST to
>> +                        * skip bus_test_pre and bus_test_post
>> +                        */
>> +                       mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
>> +                       mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
> 
> This cap is a bit strange. It was added several years ago by Adrian
> Hunter, but I am wondering about the reason to why it's needed.
> 

MMC_CAP2_HC_ERASE_SZ relates to EXT-CSD ERASE_GROUP_DEF.

I think it was added to enable people to choose whether they wanted a large
or small erase granularity.  That probably doesn't matter if the card
supports TRIM.

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

#1568433

FromUlf Hansson <ulf.hansson@linaro.org>
Date2017-01-27 16:20 +0100
Message-ID<t4gtI-5Mm-25@gated-at.bofh.it>
In reply to#1567408
On 26 January 2017 at 13:39, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 26/01/17 12:50, Ulf Hansson wrote:
>> On 11 January 2017 at 18:19, Gregory CLEMENT
>> <gregory.clement@free-electrons.com> wrote:
>>> +                       priv->init_card_type = MMC_TYPE_MMC;
>>> +                       mmc->caps |= MMC_CAP_NONREMOVABLE;
>>> +
>>> +                       /*
>>> +                        * Force to clear BUS_TEST to
>>> +                        * skip bus_test_pre and bus_test_post
>>> +                        */
>>> +                       mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
>>> +                       mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
>>
>> This cap is a bit strange. It was added several years ago by Adrian
>> Hunter, but I am wondering about the reason to why it's needed.
>>
>
> MMC_CAP2_HC_ERASE_SZ relates to EXT-CSD ERASE_GROUP_DEF.
>
> I think it was added to enable people to choose whether they wanted a large
> or small erase granularity.  That probably doesn't matter if the card
> supports TRIM.
>

Huh, the erase/trim/discard code in the mmc core is really hairy. :-)

In mmc_calc_max_discard() the following code/comment exists:

/*
 * Without erase_group_def set, MMC erase timeout depends on clock
 * frequence which can change.  In that case, the best choice is
 * just the preferred erase size.
 */
if (mmc_card_mmc(card) && !(card->ext_csd.erase_group_def & 1))
     return card->pref_erase;


This makes me wonder.

So, when we haven't enabled the high capacity erase groups in the
EXT_CSD register (ext_csd.erase_group_def), we will use the pref_erase
size.

In the other case, as when having MMC_CAP2_HC_ERASE_SZ set (which will
set ext_csd.erase_group_def),  we will instead  do some calculations
to find out the max discards.

Are you saying that these calculations doesn't matter much - or are
you saying that we always want to do them?

Kind regards
Uffe

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

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

FromAdrian Hunter <adrian.hunter@intel.com>
Date2017-01-28 09:20 +0100
SubjectRe: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
Message-ID<t4woN-7ix-5@gated-at.bofh.it>
In reply to#1568433
On 27/01/2017 5:12 p.m., Ulf Hansson wrote:
> On 26 January 2017 at 13:39, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 26/01/17 12:50, Ulf Hansson wrote:
>>> On 11 January 2017 at 18:19, Gregory CLEMENT
>>> <gregory.clement@free-electrons.com> wrote:
>>>> +                       priv->init_card_type = MMC_TYPE_MMC;
>>>> +                       mmc->caps |= MMC_CAP_NONREMOVABLE;
>>>> +
>>>> +                       /*
>>>> +                        * Force to clear BUS_TEST to
>>>> +                        * skip bus_test_pre and bus_test_post
>>>> +                        */
>>>> +                       mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
>>>> +                       mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
>>>
>>> This cap is a bit strange. It was added several years ago by Adrian
>>> Hunter, but I am wondering about the reason to why it's needed.
>>>
>>
>> MMC_CAP2_HC_ERASE_SZ relates to EXT-CSD ERASE_GROUP_DEF.
>>
>> I think it was added to enable people to choose whether they wanted a large
>> or small erase granularity.  That probably doesn't matter if the card
>> supports TRIM.
>>
>
> Huh, the erase/trim/discard code in the mmc core is really hairy. :-)
>
> In mmc_calc_max_discard() the following code/comment exists:
>
> /*
>  * Without erase_group_def set, MMC erase timeout depends on clock
>  * frequence which can change.  In that case, the best choice is
>  * just the preferred erase size.
>  */
> if (mmc_card_mmc(card) && !(card->ext_csd.erase_group_def & 1))
>      return card->pref_erase;
>
>
> This makes me wonder.
>
> So, when we haven't enabled the high capacity erase groups in the
> EXT_CSD register (ext_csd.erase_group_def), we will use the pref_erase
> size.
>
> In the other case, as when having MMC_CAP2_HC_ERASE_SZ set (which will
> set ext_csd.erase_group_def),  we will instead  do some calculations
> to find out the max discards.
>
> Are you saying that these calculations doesn't matter much - or are
> you saying that we always want to do them?

No, I was saying that if you have TRIM then TRIM is preferred to ERASE so 
the erase group size does not come into play when discarding, since ERASE 
granularity is erase groups whereas TRIM granularity is sectors.

However ERASE_GROUP_DEF also affects the size of write protect groups.

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

#1569489

FromUlf Hansson <ulf.hansson@linaro.org>
Date2017-01-30 10:20 +0100
Message-ID<t5ghY-2bl-15@gated-at.bofh.it>
In reply to#1568844
On 28 January 2017 at 09:16, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 27/01/2017 5:12 p.m., Ulf Hansson wrote:
>>
>> On 26 January 2017 at 13:39, Adrian Hunter <adrian.hunter@intel.com>
>> wrote:
>>>
>>> On 26/01/17 12:50, Ulf Hansson wrote:
>>>>
>>>> On 11 January 2017 at 18:19, Gregory CLEMENT
>>>> <gregory.clement@free-electrons.com> wrote:
>>>>>
>>>>> +                       priv->init_card_type = MMC_TYPE_MMC;
>>>>> +                       mmc->caps |= MMC_CAP_NONREMOVABLE;
>>>>> +
>>>>> +                       /*
>>>>> +                        * Force to clear BUS_TEST to
>>>>> +                        * skip bus_test_pre and bus_test_post
>>>>> +                        */
>>>>> +                       mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
>>>>> +                       mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
>>>>
>>>>
>>>> This cap is a bit strange. It was added several years ago by Adrian
>>>> Hunter, but I am wondering about the reason to why it's needed.
>>>>
>>>
>>> MMC_CAP2_HC_ERASE_SZ relates to EXT-CSD ERASE_GROUP_DEF.
>>>
>>> I think it was added to enable people to choose whether they wanted a
>>> large
>>> or small erase granularity.  That probably doesn't matter if the card
>>> supports TRIM.
>>>
>>
>> Huh, the erase/trim/discard code in the mmc core is really hairy. :-)
>>
>> In mmc_calc_max_discard() the following code/comment exists:
>>
>> /*
>>  * Without erase_group_def set, MMC erase timeout depends on clock
>>  * frequence which can change.  In that case, the best choice is
>>  * just the preferred erase size.
>>  */
>> if (mmc_card_mmc(card) && !(card->ext_csd.erase_group_def & 1))
>>      return card->pref_erase;
>>
>>
>> This makes me wonder.
>>
>> So, when we haven't enabled the high capacity erase groups in the
>> EXT_CSD register (ext_csd.erase_group_def), we will use the pref_erase
>> size.
>>
>> In the other case, as when having MMC_CAP2_HC_ERASE_SZ set (which will
>> set ext_csd.erase_group_def),  we will instead  do some calculations
>> to find out the max discards.
>>
>> Are you saying that these calculations doesn't matter much - or are
>> you saying that we always want to do them?
>
>
> No, I was saying that if you have TRIM then TRIM is preferred to ERASE so
> the erase group size does not come into play when discarding, since ERASE
> granularity is erase groups whereas TRIM granularity is sectors.

Right. Thanks for clarifying.

>
> However ERASE_GROUP_DEF also affects the size of write protect groups.

In either case.

What do you think of removing MMC_CAP2_HC_ERASE_SZ? I don't like these
kind of soft polices, it's better if we can decide on a common
behaviour - whatever that is.

Kind regards
Uffe

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

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

FromAdrian Hunter <adrian.hunter@intel.com>
Date2017-01-30 10:50 +0100
SubjectRe: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
Message-ID<t5gKZ-2ll-7@gated-at.bofh.it>
In reply to#1569489
On 30/01/17 11:10, Ulf Hansson wrote:
> On 28 January 2017 at 09:16, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 27/01/2017 5:12 p.m., Ulf Hansson wrote:
>>>
>>> On 26 January 2017 at 13:39, Adrian Hunter <adrian.hunter@intel.com>
>>> wrote:
>>>>
>>>> On 26/01/17 12:50, Ulf Hansson wrote:
>>>>>
>>>>> On 11 January 2017 at 18:19, Gregory CLEMENT
>>>>> <gregory.clement@free-electrons.com> wrote:
>>>>>>
>>>>>> +                       priv->init_card_type = MMC_TYPE_MMC;
>>>>>> +                       mmc->caps |= MMC_CAP_NONREMOVABLE;
>>>>>> +
>>>>>> +                       /*
>>>>>> +                        * Force to clear BUS_TEST to
>>>>>> +                        * skip bus_test_pre and bus_test_post
>>>>>> +                        */
>>>>>> +                       mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
>>>>>> +                       mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
>>>>>
>>>>>
>>>>> This cap is a bit strange. It was added several years ago by Adrian
>>>>> Hunter, but I am wondering about the reason to why it's needed.
>>>>>
>>>>
>>>> MMC_CAP2_HC_ERASE_SZ relates to EXT-CSD ERASE_GROUP_DEF.
>>>>
>>>> I think it was added to enable people to choose whether they wanted a
>>>> large
>>>> or small erase granularity.  That probably doesn't matter if the card
>>>> supports TRIM.
>>>>
>>>
>>> Huh, the erase/trim/discard code in the mmc core is really hairy. :-)
>>>
>>> In mmc_calc_max_discard() the following code/comment exists:
>>>
>>> /*
>>>  * Without erase_group_def set, MMC erase timeout depends on clock
>>>  * frequence which can change.  In that case, the best choice is
>>>  * just the preferred erase size.
>>>  */
>>> if (mmc_card_mmc(card) && !(card->ext_csd.erase_group_def & 1))
>>>      return card->pref_erase;
>>>
>>>
>>> This makes me wonder.
>>>
>>> So, when we haven't enabled the high capacity erase groups in the
>>> EXT_CSD register (ext_csd.erase_group_def), we will use the pref_erase
>>> size.
>>>
>>> In the other case, as when having MMC_CAP2_HC_ERASE_SZ set (which will
>>> set ext_csd.erase_group_def),  we will instead  do some calculations
>>> to find out the max discards.
>>>
>>> Are you saying that these calculations doesn't matter much - or are
>>> you saying that we always want to do them?
>>
>>
>> No, I was saying that if you have TRIM then TRIM is preferred to ERASE so
>> the erase group size does not come into play when discarding, since ERASE
>> granularity is erase groups whereas TRIM granularity is sectors.
> 
> Right. Thanks for clarifying.
> 
>>
>> However ERASE_GROUP_DEF also affects the size of write protect groups.
> 
> In either case.
> 
> What do you think of removing MMC_CAP2_HC_ERASE_SZ? I don't like these
> kind of soft polices, it's better if we can decide on a common
> behaviour - whatever that is.

Changing the value of ERASE_GROUP_DEF could be a problem, for example the
spec. says:

"Similarly if the host set ERASE_GROUP_DEF bit for a device that the default
write protect was already set in some of the area in the previous power
cycle, then the device may show unknown behavior when host issue write
or erase commands to the device. In application, it is mandatory for host to
use same ERASE GROUP DEF value to the device all the time."

Whether or not there is anyone that would actually be affected is hard to know.

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

#1569548

FromUlf Hansson <ulf.hansson@linaro.org>
Date2017-01-30 11:20 +0100
Message-ID<t5he1-2Kr-21@gated-at.bofh.it>
In reply to#1569495
On 30 January 2017 at 10:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 30/01/17 11:10, Ulf Hansson wrote:
>> On 28 January 2017 at 09:16, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 27/01/2017 5:12 p.m., Ulf Hansson wrote:
>>>>
>>>> On 26 January 2017 at 13:39, Adrian Hunter <adrian.hunter@intel.com>
>>>> wrote:
>>>>>
>>>>> On 26/01/17 12:50, Ulf Hansson wrote:
>>>>>>
>>>>>> On 11 January 2017 at 18:19, Gregory CLEMENT
>>>>>> <gregory.clement@free-electrons.com> wrote:
>>>>>>>
>>>>>>> +                       priv->init_card_type = MMC_TYPE_MMC;
>>>>>>> +                       mmc->caps |= MMC_CAP_NONREMOVABLE;
>>>>>>> +
>>>>>>> +                       /*
>>>>>>> +                        * Force to clear BUS_TEST to
>>>>>>> +                        * skip bus_test_pre and bus_test_post
>>>>>>> +                        */
>>>>>>> +                       mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
>>>>>>> +                       mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
>>>>>>
>>>>>>
>>>>>> This cap is a bit strange. It was added several years ago by Adrian
>>>>>> Hunter, but I am wondering about the reason to why it's needed.
>>>>>>
>>>>>
>>>>> MMC_CAP2_HC_ERASE_SZ relates to EXT-CSD ERASE_GROUP_DEF.
>>>>>
>>>>> I think it was added to enable people to choose whether they wanted a
>>>>> large
>>>>> or small erase granularity.  That probably doesn't matter if the card
>>>>> supports TRIM.
>>>>>
>>>>
>>>> Huh, the erase/trim/discard code in the mmc core is really hairy. :-)
>>>>
>>>> In mmc_calc_max_discard() the following code/comment exists:
>>>>
>>>> /*
>>>>  * Without erase_group_def set, MMC erase timeout depends on clock
>>>>  * frequence which can change.  In that case, the best choice is
>>>>  * just the preferred erase size.
>>>>  */
>>>> if (mmc_card_mmc(card) && !(card->ext_csd.erase_group_def & 1))
>>>>      return card->pref_erase;
>>>>
>>>>
>>>> This makes me wonder.
>>>>
>>>> So, when we haven't enabled the high capacity erase groups in the
>>>> EXT_CSD register (ext_csd.erase_group_def), we will use the pref_erase
>>>> size.
>>>>
>>>> In the other case, as when having MMC_CAP2_HC_ERASE_SZ set (which will
>>>> set ext_csd.erase_group_def),  we will instead  do some calculations
>>>> to find out the max discards.
>>>>
>>>> Are you saying that these calculations doesn't matter much - or are
>>>> you saying that we always want to do them?
>>>
>>>
>>> No, I was saying that if you have TRIM then TRIM is preferred to ERASE so
>>> the erase group size does not come into play when discarding, since ERASE
>>> granularity is erase groups whereas TRIM granularity is sectors.
>>
>> Right. Thanks for clarifying.
>>
>>>
>>> However ERASE_GROUP_DEF also affects the size of write protect groups.
>>
>> In either case.
>>
>> What do you think of removing MMC_CAP2_HC_ERASE_SZ? I don't like these
>> kind of soft polices, it's better if we can decide on a common
>> behaviour - whatever that is.
>
> Changing the value of ERASE_GROUP_DEF could be a problem, for example the
> spec. says:
>
> "Similarly if the host set ERASE_GROUP_DEF bit for a device that the default
> write protect was already set in some of the area in the previous power
> cycle, then the device may show unknown behavior when host issue write
> or erase commands to the device. In application, it is mandatory for host to
> use same ERASE GROUP DEF value to the device all the time."
>
> Whether or not there is anyone that would actually be affected is hard to know.

Okay. I am going to submit a patch that just removes the behaviour for
MMC_CAP2_HC_ERASE_SZ and the cap itself.

The cap isn't particular widely used anyway. Let's see what people think of it.

Thanks a lot for you input!

Kind regards
Uffe

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

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

FromZiji Hu <huziji@marvell.com>
Date2017-01-27 17:50 +0100
SubjectRe: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
Message-ID<t4hSN-6xB-15@gated-at.bofh.it>
In reply to#1567323
Hi Ulf,

On 2017/1/26 18:50, Ulf Hansson wrote:
> On 11 January 2017 at 18:19, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> From: Hu Ziji <huziji@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 <huziji@marvell.com>
>> Tested-by: Russell King <rmk+kernel@armlinux.org.uk>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>  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
    "mmc-card", since I'm not clear about other vendor's requirement.

>> +                       priv->init_card_type = MMC_TYPE_MMC;
>> +                       mmc->caps |= MMC_CAP_NONREMOVABLE;
>> +
>> +                       /*
>> +                        * Force to clear BUS_TEST to
>> +                        * skip bus_test_pre and bus_test_post
>> +                        */
>> +                       mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
>> +                       mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
> 
> This cap is a bit strange. It was added several years ago by Adrian
> Hunter, but I am wondering about the reason to why it's needed.
> 
> Should it be removed and instead enabled per default? If not, should
> we invent a new specific DT binding for it?
> 
> I need Adrian's help here to understand the options.
> 
>> +                                     MMC_CAP2_PACKED_CMD |
> 
> The MMC_CAP2_PACKED_CMD cap has be removed.

    Will remove it in next version.

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

> [...]
> 
>> +       priv->sdhc_id = 0x0;
>> +       if (!of_property_read_u32(np, "marvell,xenon-sdhc-id", &sdhc_id)) {
>> +               nr_sdhc = sdhci_readl(host, XENON_SYS_CFG_INFO);
>> +               nr_sdhc &= XENON_NR_SUPPORTED_SLOT_MASK;
>> +               if (unlikely(sdhc_id > nr_sdhc)) {
>> +                       dev_err(mmc_dev(mmc), "SDHC Index %d exceeds Number of SDHCs %d\n",
>> +                               sdhc_id, nr_sdhc);
>> +                       return -EINVAL;
>> +               }
>> +       }
>> +
>> +       tuning_count = XENON_DEF_TUNING_COUNT;
>> +       if (!of_property_read_u32(np, "marvell,xenon-tun-count",
>> +                                 &tuning_count)) {
>> +               if (unlikely(tuning_count >= XENON_TMR_RETUN_NO_PRESENT)) {
>> +                       dev_err(mmc_dev(mmc), "Wrong Re-tuning Count. Set default value %d\n",
>> +                               XENON_DEF_TUNING_COUNT);
>> +                       tuning_count = XENON_DEF_TUNING_COUNT;
>> +               }
>> +       }
> 
> I suggest you move the parsing of the xenon specific bindings into a
> separate function.
> 

    I totally agree with you.

>> +       priv->tuning_count = tuning_count;
>> +
>> +       return err;
>> +}
>> +
> 
> [...]
> 
> Kind regards
> Uffe
>

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

#1569467

FromUlf Hansson <ulf.hansson@linaro.org>
Date2017-01-30 09:50 +0100
Message-ID<t5fOW-1MH-9@gated-at.bofh.it>
In reply to#1568504
[...]

>>> + */
>>> +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.

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

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

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

FromZiji Hu <huziji@marvell.com>
Date2017-01-30 16:40 +0100
SubjectRe: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
Message-ID<t5mdH-5F2-5@gated-at.bofh.it>
In reply to#1569467
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
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web