* Re: [GIT PULL] DWMMC controller for next
From: Jaehoon Chung @ 2016-11-24 11:50 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc@vger.kernel.org
In-Reply-To: <CAPDyKFqUjLqYT5nS7hsAULK6iDtvcv8a1Cqao_VCtTXm5X82cA@mail.gmail.com>
On 11/24/2016 08:43 PM, Ulf Hansson wrote:
> On 24 November 2016 at 12:17, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> On 11/23/2016 09:31 PM, Jaehoon Chung wrote:
>>> On 11/23/2016 08:50 PM, Jaehoon Chung wrote:
>>>> On 11/23/2016 08:47 PM, Ulf Hansson wrote:
>>>>> On 22 November 2016 at 17:13, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>> On 22 November 2016 at 10:59, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>>>>> Dear Ulf,
>>>>>>>
>>>>>>> Could you pull these patches on your next branch?
>>>>>>> If there is a problem, let me know, plz.
>>>>>>>
>>>>>>> The following changes since commit e2081b37d910c5e6c6925d9b4d0a7a6283f84ec5:
>>>>>>>
>>>>>>> Merge branch 'fixes' into next (2016-11-21 11:09:18 +0100)
>>>>>>>
>>>>>>> are available in the git repository at:
>>>>>>>
>>>>>>> https://github.com/jh80chung/dw-mmc.git for-ulf
>>>>>>>
>>>>>>> for you to fetch changes up to e25fd245b557482a8e0f7ab87841085f30706f3a:
>>>>>>>
>>>>>>> Documentation: synopsys-dw-mshc: remove the unused properties (2016-11-22 10:34:05 +0900)
>>>>>>>
>>>>>>> ----------------------------------------------------------------
>>>>>>> Colin Ian King (1):
>>>>>>> mmc: dw_mmc: fix spelling mistake in dev_dbg message
>>>>>>>
>>>>>>> Jaehoon Chung (9):
>>>>>>> mmc: dw_mmc: display the real register value on debugfs
>>>>>>> mmc: dw_mmc: fix the debug message for checking card's present
>>>>>>> mmc: dw_mmc: change the DW_MCI_FREQ_MIN from 400K to 100K
>>>>>>> mmc: dw_mmc: use the hold register when send stop command
>>>>>>> mmc: dw_mmc: call the dw_mci_prep_stop_abort() by default
>>>>>>> mmc: dw_mmc: use the cookie's enum values for post/pre_req()
>>>>>>> mmc: dw_mmc: remove the unnecessary mmc_data structure
>>>>>>> mmc: dw_mmc: The "clock-freq-min-max" property was deprecated
>>>>>>> Documentation: synopsys-dw-mshc: remove the unused properties
>>>>>>>
>>>>>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 8 ++------
>>>>>>> drivers/mmc/host/dw_mmc.c | 92 ++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------
>>>>>>> include/linux/mmc/dw_mmc.h | 6 ++++++
>>>>>>> 3 files changed, 52 insertions(+), 54 deletions(-)
>>>>>>>
>>>>>>> Best Regards,
>>>>>>> Jaehoon Chung
>>>>>>
>>>>>> Dear Jaehoon, thanks for the pull request. I have add queued it all up
>>>>>> for my next branch!
>>>>>>
>>>>>>
>>>>>> Kind regards
>>>>>> Uffe
>>>>>
>>>>> Jaehoon,
>>>>>
>>>>> It seems like some of these patches breaks Exynos 5250-arndale. Can
>>>>> you please have a look and see what you think?
>>>>> https://kernelci.org/boot/all/job/ulfh/kernel/v4.9-rc5-78-g52eb5d4531e4/
>>>>
>>>> I will check and reply. Thanks for noticing.
>>
>> Ulf,
>>
>> It's strange..My patches didn't have any dependency...
>> There is no any log for dwmmc..and even it doesn't initialize dwmmc controller.
>>
>> Did you think that this failure is my patches problem? What is your thinking?
>
> Well, I don't know but indeed it seems a bit odd. Maybe there are some
> interdependency that changes the behaviour for something outside
> dw_mmc, which triggers the problems!?
>
> It could of course also be completely unrelated.
>
> I do have the Arndale board available. I will try it out within a
> couple of days, to see if I can find something.
If there is effect..it seems that only below patch for clock..
commit 38332b0fd6c06fcecc64bc238bbb7cf80ed6bc7b
Author: Jaehoon Chung <jh80.chung@samsung.com>
Date: Thu Nov 17 16:40:35 2016 +0900
mmc: dw_mmc: change the DW_MCI_FREQ_MIN from 400K to 100K
If there is no property "clock-freq-min-max", mmc->f_min should be set
to 400K by default. But Some SoC can be used 100K.
When 100K is used, MMC core will try to check from 400K to 100K.
otherwise timing?
I wonder when the below patches is applied, what happen..
https://patchwork.kernel.org/patch/9445233/
https://patchwork.kernel.org/patch/9445209/
https://patchwork.kernel.org/patch/9445211/
I will also check this continuously..to find before PR for next.
Best Regards,
Jaehoon Chung
>
> [...]
>
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
^ permalink raw reply
* Re: [GIT PULL] DWMMC controller for next
From: Ulf Hansson @ 2016-11-24 11:43 UTC (permalink / raw)
To: Jaehoon Chung; +Cc: linux-mmc@vger.kernel.org
In-Reply-To: <ca995842-9e10-a5c5-27b9-dd24750e36a1@samsung.com>
On 24 November 2016 at 12:17, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> On 11/23/2016 09:31 PM, Jaehoon Chung wrote:
>> On 11/23/2016 08:50 PM, Jaehoon Chung wrote:
>>> On 11/23/2016 08:47 PM, Ulf Hansson wrote:
>>>> On 22 November 2016 at 17:13, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> On 22 November 2016 at 10:59, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>>>> Dear Ulf,
>>>>>>
>>>>>> Could you pull these patches on your next branch?
>>>>>> If there is a problem, let me know, plz.
>>>>>>
>>>>>> The following changes since commit e2081b37d910c5e6c6925d9b4d0a7a6283f84ec5:
>>>>>>
>>>>>> Merge branch 'fixes' into next (2016-11-21 11:09:18 +0100)
>>>>>>
>>>>>> are available in the git repository at:
>>>>>>
>>>>>> https://github.com/jh80chung/dw-mmc.git for-ulf
>>>>>>
>>>>>> for you to fetch changes up to e25fd245b557482a8e0f7ab87841085f30706f3a:
>>>>>>
>>>>>> Documentation: synopsys-dw-mshc: remove the unused properties (2016-11-22 10:34:05 +0900)
>>>>>>
>>>>>> ----------------------------------------------------------------
>>>>>> Colin Ian King (1):
>>>>>> mmc: dw_mmc: fix spelling mistake in dev_dbg message
>>>>>>
>>>>>> Jaehoon Chung (9):
>>>>>> mmc: dw_mmc: display the real register value on debugfs
>>>>>> mmc: dw_mmc: fix the debug message for checking card's present
>>>>>> mmc: dw_mmc: change the DW_MCI_FREQ_MIN from 400K to 100K
>>>>>> mmc: dw_mmc: use the hold register when send stop command
>>>>>> mmc: dw_mmc: call the dw_mci_prep_stop_abort() by default
>>>>>> mmc: dw_mmc: use the cookie's enum values for post/pre_req()
>>>>>> mmc: dw_mmc: remove the unnecessary mmc_data structure
>>>>>> mmc: dw_mmc: The "clock-freq-min-max" property was deprecated
>>>>>> Documentation: synopsys-dw-mshc: remove the unused properties
>>>>>>
>>>>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 8 ++------
>>>>>> drivers/mmc/host/dw_mmc.c | 92 ++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------
>>>>>> include/linux/mmc/dw_mmc.h | 6 ++++++
>>>>>> 3 files changed, 52 insertions(+), 54 deletions(-)
>>>>>>
>>>>>> Best Regards,
>>>>>> Jaehoon Chung
>>>>>
>>>>> Dear Jaehoon, thanks for the pull request. I have add queued it all up
>>>>> for my next branch!
>>>>>
>>>>>
>>>>> Kind regards
>>>>> Uffe
>>>>
>>>> Jaehoon,
>>>>
>>>> It seems like some of these patches breaks Exynos 5250-arndale. Can
>>>> you please have a look and see what you think?
>>>> https://kernelci.org/boot/all/job/ulfh/kernel/v4.9-rc5-78-g52eb5d4531e4/
>>>
>>> I will check and reply. Thanks for noticing.
>
> Ulf,
>
> It's strange..My patches didn't have any dependency...
> There is no any log for dwmmc..and even it doesn't initialize dwmmc controller.
>
> Did you think that this failure is my patches problem? What is your thinking?
Well, I don't know but indeed it seems a bit odd. Maybe there are some
interdependency that changes the behaviour for something outside
dw_mmc, which triggers the problems!?
It could of course also be completely unrelated.
I do have the Arndale board available. I will try it out within a
couple of days, to see if I can find something.
[...]
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ulf Hansson @ 2016-11-24 11:37 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Adrian Hunter, linux-mmc@vger.kernel.org, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree@vger.kernel.org, Thomas Petazzoni,
linux-arm-kernel@lists.infradead.org, Ziji Hu, Jack(SH) Zhu,
Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao, Doug Jones,
Shiwu Zhang, Victor Gu, Wei(SOCP)
In-Reply-To: <a05ffd140f4edc02fc3128db8445b2264cf38723.1477911954.git-series.gregory.clement@free-electrons.com>
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
^ permalink raw reply
* Re: [GIT PULL] DWMMC controller for next
From: Jaehoon Chung @ 2016-11-24 11:17 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc@vger.kernel.org
In-Reply-To: <a6358997-0e19-05b6-e488-3a17bdac5952@samsung.com>
On 11/23/2016 09:31 PM, Jaehoon Chung wrote:
> On 11/23/2016 08:50 PM, Jaehoon Chung wrote:
>> On 11/23/2016 08:47 PM, Ulf Hansson wrote:
>>> On 22 November 2016 at 17:13, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 22 November 2016 at 10:59, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>>> Dear Ulf,
>>>>>
>>>>> Could you pull these patches on your next branch?
>>>>> If there is a problem, let me know, plz.
>>>>>
>>>>> The following changes since commit e2081b37d910c5e6c6925d9b4d0a7a6283f84ec5:
>>>>>
>>>>> Merge branch 'fixes' into next (2016-11-21 11:09:18 +0100)
>>>>>
>>>>> are available in the git repository at:
>>>>>
>>>>> https://github.com/jh80chung/dw-mmc.git for-ulf
>>>>>
>>>>> for you to fetch changes up to e25fd245b557482a8e0f7ab87841085f30706f3a:
>>>>>
>>>>> Documentation: synopsys-dw-mshc: remove the unused properties (2016-11-22 10:34:05 +0900)
>>>>>
>>>>> ----------------------------------------------------------------
>>>>> Colin Ian King (1):
>>>>> mmc: dw_mmc: fix spelling mistake in dev_dbg message
>>>>>
>>>>> Jaehoon Chung (9):
>>>>> mmc: dw_mmc: display the real register value on debugfs
>>>>> mmc: dw_mmc: fix the debug message for checking card's present
>>>>> mmc: dw_mmc: change the DW_MCI_FREQ_MIN from 400K to 100K
>>>>> mmc: dw_mmc: use the hold register when send stop command
>>>>> mmc: dw_mmc: call the dw_mci_prep_stop_abort() by default
>>>>> mmc: dw_mmc: use the cookie's enum values for post/pre_req()
>>>>> mmc: dw_mmc: remove the unnecessary mmc_data structure
>>>>> mmc: dw_mmc: The "clock-freq-min-max" property was deprecated
>>>>> Documentation: synopsys-dw-mshc: remove the unused properties
>>>>>
>>>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 8 ++------
>>>>> drivers/mmc/host/dw_mmc.c | 92 ++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------
>>>>> include/linux/mmc/dw_mmc.h | 6 ++++++
>>>>> 3 files changed, 52 insertions(+), 54 deletions(-)
>>>>>
>>>>> Best Regards,
>>>>> Jaehoon Chung
>>>>
>>>> Dear Jaehoon, thanks for the pull request. I have add queued it all up
>>>> for my next branch!
>>>>
>>>>
>>>> Kind regards
>>>> Uffe
>>>
>>> Jaehoon,
>>>
>>> It seems like some of these patches breaks Exynos 5250-arndale. Can
>>> you please have a look and see what you think?
>>> https://kernelci.org/boot/all/job/ulfh/kernel/v4.9-rc5-78-g52eb5d4531e4/
>>
>> I will check and reply. Thanks for noticing.
Ulf,
It's strange..My patches didn't have any dependency...
There is no any log for dwmmc..and even it doesn't initialize dwmmc controller.
Did you think that this failure is my patches problem? What is your thinking?
Best Regards,
Jaehoon Chung
>
> Ulf,
>
> I'm not sure but if more stable than now..i think that needs to check NULL pointer.
> What do you want how i do? Send patch or revert commit?
>
> I didn't have the 5250-arndale..so i can't test..How about the below?
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 881ca3e..7a0cb97 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -167,12 +167,14 @@ static int dw_mci_regs_show(struct seq_file *s, void *v)
> {
> struct dw_mci *host = s->private;
>
> - seq_printf(s, "STATUS:\t0x%08x\n", mci_readl(host, STATUS));
> - seq_printf(s, "RINTSTS:\t0x%08x\n", mci_readl(host, RINTSTS));
> - seq_printf(s, "CMD:\t0x%08x\n", mci_readl(host, CMD));
> - seq_printf(s, "CTRL:\t0x%08x\n", mci_readl(host, CTRL));
> - seq_printf(s, "INTMASK:\t0x%08x\n", mci_readl(host, INTMASK));
> - seq_printf(s, "CLKENA:\t0x%08x\n", mci_readl(host, CLKENA));
> + if (host->regs) {
> + seq_printf(s, "STATUS:\t0x%08x\n", mci_readl(host, STATUS));
> + seq_printf(s, "RINTSTS:\t0x%08x\n", mci_readl(host, RINTSTS));
> + seq_printf(s, "CMD:\t0x%08x\n", mci_readl(host, CMD));
> + seq_printf(s, "CTRL:\t0x%08x\n", mci_readl(host, CTRL));
> + seq_printf(s, "INTMASK:\t0x%08x\n", mci_readl(host, INTMASK));
> + seq_printf(s, "CLKENA:\t0x%08x\n", mci_readl(host, CLKENA));
> + }
>
> return 0;
> }
>
> Best Regards,
> Jaehoon Chung
>
>>
>> Beset Regards,
>> Jaehoon Chung
>>
>>>
>>> Kind regards
>>> Uffe
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
^ permalink raw reply
* [PATCH 1/3] mmc: dw_mmc: check the "present" variable before checking flags
From: Jaehoon Chung @ 2016-11-24 11:04 UTC (permalink / raw)
To: linux-mmc; +Cc: ulf.hansson, shawn.lin, Jaehoon Chung
Before checking flags, it has to check "present" variable.
Otherwise, flags should be cleared everytime.
Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
---
drivers/mmc/host/dw_mmc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index d400afc..c6cc618 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1536,7 +1536,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
spin_lock_bh(&host->lock);
if (present && !test_and_set_bit(DW_MMC_CARD_PRESENT, &slot->flags))
dev_dbg(&mmc->class_dev, "card is present\n");
- else if (!test_and_clear_bit(DW_MMC_CARD_PRESENT, &slot->flags))
+ else if (!present &&
+ !test_and_clear_bit(DW_MMC_CARD_PRESENT, &slot->flags))
dev_dbg(&mmc->class_dev, "card is not present\n");
spin_unlock_bh(&host->lock);
--
2.10.1
^ permalink raw reply related
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Arnd Bergmann @ 2016-11-24 11:09 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Hilbert Zhang, Andrew Lunn, Ulf Hansson, Romain Perier,
Liuliu Zhao, Peng Zhu, Adrian Hunter, Nadav Haklai, Jack(SH) Zhu,
Victor Gu, Doug Jones, Jisheng Zhang, Yehuda Yitschak,
Wei(SOCP) Liu, Xueping Liu, Shiwu Zhang, Yu Cao,
Sebastian Hesselbarth, devicetree, Jason Cooper, Keji Zhang,
Kostya Porotchkin, Rob Herring, Ryan Gao
In-Reply-To: <f7334e41-39dd-f868-ff10-199afaebe926@marvell.com>
On Thursday, November 24, 2016 6:57:18 PM CET Ziji Hu wrote:
> >
> > 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.
>
Ok, that makes sense, just put the same text in the changelog comment.
Arnd
^ permalink raw reply
* [PATCH 3/3] mmc: dw_mmc: display the clock message only one time when card is polling
From: Jaehoon Chung @ 2016-11-24 11:04 UTC (permalink / raw)
To: linux-mmc; +Cc: ulf.hansson, shawn.lin, Jaehoon Chung
In-Reply-To: <20161124110442.22058-1-jh80.chung@samsung.com>
When card is polling (broken-cd), there is a spamming messge related to
clock.
After applied this patch, display the message only one time at boot
time. It's enough to check which clock values is used.
Also prevent to display the spamming message.
Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
---
drivers/mmc/host/dw_mmc.c | 13 ++++++++++++-
drivers/mmc/host/dw_mmc.h | 1 +
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index d508225..dd7e95d 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1176,13 +1176,24 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
- if (clock != slot->__clk_old || force_clkinit)
+ if ((clock != slot->__clk_old &&
+ !test_bit(DW_MMC_CARD_NEEDS_POLL, &slot->flags)) ||
+ force_clkinit) {
dev_info(&slot->mmc->class_dev,
"Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
slot->id, host->bus_hz, clock,
div ? ((host->bus_hz / div) >> 1) :
host->bus_hz, div);
+ /*
+ * If card is polling, display the message only
+ * one time at boot time.
+ */
+ if (slot->mmc->caps & MMC_CAP_NEEDS_POLL &&
+ slot->mmc->f_min == clock)
+ set_bit(DW_MMC_CARD_NEEDS_POLL, &slot->flags);
+ }
+
/* disable clock */
mci_writel(host, CLKENA, 0);
mci_writel(host, CLKSRC, 0);
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 4a6ae750..c594658 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -272,6 +272,7 @@ struct dw_mci_slot {
#define DW_MMC_CARD_NEED_INIT 1
#define DW_MMC_CARD_NO_LOW_PWR 2
#define DW_MMC_CARD_NO_USE_HOLD 3
+#define DW_MMC_CARD_NEEDS_POLL 4
int id;
int sdio_id;
};
--
2.10.1
^ permalink raw reply related
* [PATCH 2/3] mmc: dw_mmc: add the debug message for polling and non-removable
From: Jaehoon Chung @ 2016-11-24 11:04 UTC (permalink / raw)
To: linux-mmc; +Cc: ulf.hansson, shawn.lin, Jaehoon Chung
In-Reply-To: <20161124110442.22058-1-jh80.chung@samsung.com>
If card is polling or non-removable, display the more exact message.
It's helpful to debug which detecting scheme is using.
Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
---
drivers/mmc/host/dw_mmc.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index c6cc618..d508225 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1525,9 +1525,23 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
int gpio_cd = mmc_gpio_get_cd(mmc);
/* Use platform get_cd function, else try onboard card detect */
- if ((mmc->caps & MMC_CAP_NEEDS_POLL) || !mmc_card_is_removable(mmc))
+ if (((mmc->caps & MMC_CAP_NEEDS_POLL)
+ || !mmc_card_is_removable(mmc))) {
present = 1;
- else if (gpio_cd >= 0)
+
+ if (!test_bit(DW_MMC_CARD_PRESENT, &slot->flags)) {
+ if (mmc->caps & MMC_CAP_NEEDS_POLL) {
+ dev_info(&mmc->class_dev,
+ "card is polling.\n");
+ } else {
+ dev_info(&mmc->class_dev,
+ "card is non-removable.\n");
+ }
+ set_bit(DW_MMC_CARD_PRESENT, &slot->flags);
+ }
+
+ return present;
+ } else if (gpio_cd >= 0)
present = gpio_cd;
else
present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
--
2.10.1
^ permalink raw reply related
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ziji Hu @ 2016-11-24 10:57 UTC (permalink / raw)
To: Arnd Bergmann, Gregory CLEMENT
Cc: Ulf Hansson, Adrian Hunter, linux-mmc, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Rob Herring, devicetree, Thomas Petazzoni,
linux-arm-kernel, Jack(SH) Zhu, Jimmy Xu, Jisheng Zhang,
Nadav Haklai, Ryan Gao, Doug Jones, Shiwu Zhang, Victor Gu,
Wei(SOCP) Liu, Wilson Ding, Xueping Liu
In-Reply-To: <2204525.IWIYQVjIXl@wuerfel>
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 <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>
>>
>
> 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
>
^ permalink raw reply
* Re: [PATCH mmc/next] mmc: sh_mobile_sdhi: remove support for sh7372
From: Geert Uytterhoeven @ 2016-11-24 10:53 UTC (permalink / raw)
To: Simon Horman
Cc: Wolfram Sang, Ulf Hansson, Magnus Damm, Linux MMC List,
Linux-Renesas
In-Reply-To: <1479984534-25468-1-git-send-email-horms+renesas@verge.net.au>
On Thu, Nov 24, 2016 at 11:48 AM, Simon Horman
<horms+renesas@verge.net.au> wrote:
> Remove documentation of support for the SH7372 (SH-Mobile AP4) from the MMC
> driver. The driver itself appears to have no SH7372 specific code.
>
> Commit edf4100906044225 ("ARM: shmobile: sh7372 dtsi: Remove Legacy file")
> removes this SoC from the kernel in v4.1.
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH mmc/next] mmc: sh_mobile_sdhi: remove support for sh7372
From: Simon Horman @ 2016-11-24 10:48 UTC (permalink / raw)
To: Wolfram Sang, Ulf Hansson
Cc: Magnus Damm, linux-mmc, linux-renesas-soc, Simon Horman
Remove documentation of support for the SH7372 (SH-Mobile AP4) from the MMC
driver. The driver itself appears to have no SH7372 specific code.
Commit edf4100906044225 ("ARM: shmobile: sh7372 dtsi: Remove Legacy file")
removes this SoC from the kernel in v4.1.
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 1 -
1 file changed, 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
index 13df9c2399c3..1db9e74bb9c1 100644
--- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
@@ -11,7 +11,6 @@ optional bindings can be used.
Required properties:
- compatible: "renesas,sdhi-shmobile" - a generic sh-mobile SDHI unit
- "renesas,sdhi-sh7372" - SDHI IP on SH7372 SoC
"renesas,sdhi-sh73a0" - SDHI IP on SH73A0 SoC
"renesas,sdhi-r8a73a4" - SDHI IP on R8A73A4 SoC
"renesas,sdhi-r8a7740" - SDHI IP on R8A7740 SoC
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related
* Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
From: Ulf Hansson @ 2016-11-24 10:43 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Adrian Hunter, linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thomas Petazzoni,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Ziji Hu, Jack(SH) Zhu, Jimmy Xu, Jisheng Zhang, Nadav Haklai,
Ryan Gao, Doug Jones, Shiwu Zhang, Victor Gu, Wei(SOCP)
In-Reply-To: <0390e7a05b6163deabb545f93729ea615eeaaee2.1477911954.git-series.gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
On 31 October 2016 at 12:09, Gregory CLEMENT
<gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> From: Ziji Hu <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>
> 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-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> MAINTAINERS | 1 +-
> drivers/mmc/host/Kconfig | 9 +-
> drivers/mmc/host/Makefile | 3 +-
> drivers/mmc/host/sdhci-xenon.c | 594 ++++++++++++++++++++++++++++++++++-
> drivers/mmc/host/sdhci-xenon.h | 142 ++++++++-
> 5 files changed, 749 insertions(+), 0 deletions(-)
> create mode 100644 drivers/mmc/host/sdhci-xenon.c
> create mode 100644 drivers/mmc/host/sdhci-xenon.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 850a0afb0c8d..d92f4175574b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7608,6 +7608,7 @@ MARVELL XENON MMC/SD/SDIO HOST CONTROLLER DRIVER
> M: Ziji Hu <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> L: linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> S: Supported
> +F: drivers/mmc/host/sdhci-xenon.*
> F: Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
>
> MATROX FRAMEBUFFER DRIVER
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 5274f503a39a..85a53623526a 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -798,3 +798,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
> + help
> + This selects Marvell Xenon eMMC/SD/SDIO SDHCI.
> + If you have a machine with integrated Marvell Xenon SDHC IP,
> + say Y or M here.
> + If unsure, say N.
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index e2bdaaf43184..75eaf743486c 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -80,3 +80,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
> diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
> new file mode 100644
> index 000000000000..3ea059f2aaab
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-xenon.c
> @@ -0,0 +1,594 @@
> +/*
> + * Driver for Marvell SOC Platform Group Xenon SDHC as a platform device
> + *
> + * Copyright (C) 2016 Marvell, All Rights Reserved.
> + *
> + * Author: Hu Ziji <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> + * 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.
> + *
> + * Inspired by Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> + * Special thanks to Video BG4 project team.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/sdio.h>
> +#include <linux/mmc/card.h>
> +#include <linux/mmc/host.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#include "sdhci-pltfm.h"
> +#include "sdhci.h"
> +#include "sdhci-xenon.h"
> +
> +/* Set SDCLK-off-while-idle */
> +static void xenon_set_sdclk_off_idle(struct sdhci_host *host,
> + unsigned char slot_idx, bool enable)
> +{
> + u32 reg;
> + u32 mask;
> +
> + reg = sdhci_readl(host, SDHC_SYS_OP_CTRL);
> + /* Get the bit shift basing on the slot index */
> + mask = (0x1 << (SDCLK_IDLEOFF_ENABLE_SHIFT + slot_idx));
> + if (enable)
> + reg |= mask;
> + else
> + reg &= ~mask;
> +
> + sdhci_writel(host, reg, SDHC_SYS_OP_CTRL);
> +}
> +
> +/* Enable/Disable the Auto Clock Gating function */
> +static void xenon_set_acg(struct sdhci_host *host, bool enable)
> +{
> + u32 reg;
> +
> + reg = sdhci_readl(host, SDHC_SYS_OP_CTRL);
> + if (enable)
> + reg &= ~AUTO_CLKGATE_DISABLE_MASK;
> + else
> + reg |= AUTO_CLKGATE_DISABLE_MASK;
> + sdhci_writel(host, reg, SDHC_SYS_OP_CTRL);
> +}
> +
> +/* Enable this slot */
> +static void xenon_enable_slot(struct sdhci_host *host,
> + unsigned char slot_idx)
> +{
> + u32 reg;
> +
> + reg = sdhci_readl(host, SDHC_SYS_OP_CTRL);
> + reg |= (BIT(slot_idx) << SLOT_ENABLE_SHIFT);
> + sdhci_writel(host, reg, SDHC_SYS_OP_CTRL);
> +
> + /*
> + * Manually set the flag which all the slots require,
> + * including SD, eMMC, SDIO
> + */
> + host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
> +}
> +
> +/* Disable this slot */
> +static void xenon_disable_slot(struct sdhci_host *host,
> + unsigned char slot_idx)
> +{
> + u32 reg;
> +
> + reg = sdhci_readl(host, SDHC_SYS_OP_CTRL);
> + reg &= ~(BIT(slot_idx) << SLOT_ENABLE_SHIFT);
> + sdhci_writel(host, reg, SDHC_SYS_OP_CTRL);
> +}
> +
> +/* Enable Parallel Transfer Mode */
> +static void xenon_enable_slot_parallel_tran(struct sdhci_host *host,
> + unsigned char slot_idx)
> +{
> + u32 reg;
> +
> + reg = sdhci_readl(host, SDHC_SYS_EXT_OP_CTRL);
> + reg |= BIT(slot_idx);
> + sdhci_writel(host, reg, SDHC_SYS_EXT_OP_CTRL);
> +}
> +
> +static void xenon_slot_tuning_setup(struct sdhci_host *host)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> + u32 reg;
> +
> + /* Disable the Re-Tuning Request functionality */
> + reg = sdhci_readl(host, SDHC_SLOT_RETUNING_REQ_CTRL);
> + reg &= ~RETUNING_COMPATIBLE;
> + sdhci_writel(host, reg, SDHC_SLOT_RETUNING_REQ_CTRL);
> +
> + /* Disable the Re-tuning Event Signal Enable */
> + reg = sdhci_readl(host, SDHCI_SIGNAL_ENABLE);
> + reg &= ~SDHCI_INT_RETUNE;
> + sdhci_writel(host, reg, SDHCI_SIGNAL_ENABLE);
> +
> + /* Force to use Tuning Mode 1 */
> + host->tuning_mode = SDHCI_TUNING_MODE_1;
> + /* Set re-tuning period */
> + host->tuning_count = 1 << (priv->tuning_count - 1);
> +}
> +
> +/*
> + * Operations inside struct sdhci_ops
> + */
> +/* Recover the Register Setting cleared during SOFTWARE_RESET_ALL */
> +static void sdhci_xenon_reset_exit(struct sdhci_host *host,
> + unsigned char slot_idx, u8 mask)
> +{
> + /* Only SOFTWARE RESET ALL will clear the register setting */
> + if (!(mask & SDHCI_RESET_ALL))
> + return;
> +
> + /* Disable tuning request and auto-retuning again */
> + xenon_slot_tuning_setup(host);
> +
> + xenon_set_acg(host, true);
> +
> + xenon_set_sdclk_off_idle(host, slot_idx, false);
> +}
> +
> +static void sdhci_xenon_reset(struct sdhci_host *host, u8 mask)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +
> + sdhci_reset(host, mask);
> + sdhci_xenon_reset_exit(host, priv->slot_idx, mask);
> +}
> +
> +/*
> + * Xenon defines different values for HS200 and SDR104
> + * in Host_Control_2
> + */
> +static void xenon_set_uhs_signaling(struct sdhci_host *host,
> + unsigned int timing)
> +{
> + u16 ctrl_2;
> +
> + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + /* Select Bus Speed Mode for host */
> + ctrl_2 &= ~SDHCI_CTRL_UHS_MASK;
> + if (timing == MMC_TIMING_MMC_HS200)
> + ctrl_2 |= XENON_SDHCI_CTRL_HS200;
> + else if (timing == MMC_TIMING_UHS_SDR104)
> + ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
> + else if (timing == MMC_TIMING_UHS_SDR12)
> + ctrl_2 |= SDHCI_CTRL_UHS_SDR12;
> + else if (timing == MMC_TIMING_UHS_SDR25)
> + ctrl_2 |= SDHCI_CTRL_UHS_SDR25;
> + else if (timing == MMC_TIMING_UHS_SDR50)
> + ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
> + else if ((timing == MMC_TIMING_UHS_DDR50) ||
> + (timing == MMC_TIMING_MMC_DDR52))
> + ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
> + else if (timing == MMC_TIMING_MMC_HS400)
> + ctrl_2 |= XENON_SDHCI_CTRL_HS400;
> + sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> +}
> +
> +static const struct sdhci_ops sdhci_xenon_ops = {
> + .set_clock = sdhci_set_clock,
> + .set_bus_width = sdhci_set_bus_width,
> + .reset = sdhci_xenon_reset,
> + .set_uhs_signaling = xenon_set_uhs_signaling,
> + .get_max_clock = sdhci_pltfm_clk_get_max_clock,
> +};
> +
> +static const struct sdhci_pltfm_data sdhci_xenon_pdata = {
> + .ops = &sdhci_xenon_ops,
> + .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC |
> + SDHCI_QUIRK_NO_SIMULT_VDD_AND_POWER |
> + SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> +};
> +
> +/*
> + * Xenon Specific Operations in mmc_host_ops
> + */
> +static void xenon_set_ios(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);
> + unsigned long flags;
> + u32 reg;
> +
> + /*
> + * HS400/HS200/eMMC HS doesn't have Preset Value register.
> + * However, sdhci_set_ios will read HS400/HS200 Preset register.
> + * Disable Preset Value register for HS400/HS200.
> + * eMMC HS with preset_enabled set will trigger a bug in
> + * get_preset_value().
> + */
> + spin_lock_irqsave(&host->lock, flags);
> + if ((ios->timing == MMC_TIMING_MMC_HS400) ||
> + (ios->timing == MMC_TIMING_MMC_HS200) ||
> + (ios->timing == MMC_TIMING_MMC_HS)) {
> + host->preset_enabled = false;
> + host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
> +
> + reg = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + reg &= ~SDHCI_CTRL_PRESET_VAL_ENABLE;
> + sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
> + } else {
> + host->quirks2 &= ~SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
> + }
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + sdhci_set_ios(mmc, ios);
> +
> + if (host->clock > DEFAULT_SDCLK_FREQ) {
> + spin_lock_irqsave(&host->lock, flags);
> + xenon_set_sdclk_off_idle(host, priv->slot_idx, true);
> + spin_unlock_irqrestore(&host->lock, flags);
> + }
> +}
> +
> +static int __emmc_signal_voltage_switch(struct mmc_host *mmc,
> + const unsigned char signal_voltage)
> +{
> + u32 ctrl;
> + unsigned char voltage_code;
> + struct sdhci_host *host = mmc_priv(mmc);
> +
> + if (signal_voltage == MMC_SIGNAL_VOLTAGE_330)
> + voltage_code = EMMC_VCCQ_3_3V;
> + else if (signal_voltage == MMC_SIGNAL_VOLTAGE_180)
> + voltage_code = EMMC_VCCQ_1_8V;
> + else
> + return -EINVAL;
> +
> + /*
> + * This host is for eMMC, XENON self-defined
> + * eMMC slot control register should be accessed
> + * instead of Host Control 2
> + */
> + ctrl = sdhci_readl(host, SDHC_SLOT_EMMC_CTRL);
> + ctrl &= ~EMMC_VCCQ_MASK;
> + ctrl |= voltage_code;
> + sdhci_writel(host, ctrl, SDHC_SLOT_EMMC_CTRL);
> +
> + /* There is no standard to determine this waiting period */
> + usleep_range(1000, 2000);
> +
> + /* Check whether io voltage switch is done */
> + ctrl = sdhci_readl(host, SDHC_SLOT_EMMC_CTRL);
> + ctrl &= EMMC_VCCQ_MASK;
> + /*
> + * This bit is set only when regulator feeds back the voltage switch
> + * results to Xenon SDHC.
> + * However, in actaul implementation, regulator might not provide
> + * this feedback.
> + * Thus we shall not rely on this bit to determine if switch failed.
> + * If the bit is not set, just throw a message.
> + * Besides, error code should not be returned.
> + */
> + if (ctrl != voltage_code)
> + dev_info(mmc_dev(mmc), "fail to detect eMMC signal voltage stable\n");
> + return 0;
> +}
> +
> +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().
> +}
> +
> +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?
> + * 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!?
> +
> + 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.
> + 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
> + */
> + mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
> + mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
> + MMC_CAP2_PACKED_CMD;
> + if (mmc->caps & MMC_CAP_8_BIT_DATA)
> + mmc->caps2 |= MMC_CAP2_HS400_1_8V;
Most of this can be specified as DT configurations. Please use that instead.
More importantly, please don't use the ->init_card() ops to assign
host caps. If not DT, please do it from ->probe().
> + }
> + } else {
> + /*
> + * Set SDIO Card Inserted indication
> + * to inform that the current slot is for SDIO
> + */
> + reg = sdhci_readl(host, SDHC_SYS_CFG_INFO);
> + reg |= (1 << (slot_idx + SLOT_TYPE_SDIO_SHIFT));
> + sdhci_writel(host, reg, SDHC_SYS_CFG_INFO);
So this makes sence to have in the ->init_card() ops. The rest above, not.
> + }
> +}
> +
> +static int xenon_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> +
> + if (host->timing == MMC_TIMING_UHS_DDR50)
> + return 0;
> +
> + return sdhci_execute_tuning(mmc, opcode);
> +}
> +
> +static void xenon_replace_mmc_host_ops(struct sdhci_host *host)
> +{
> + host->mmc_host_ops.set_ios = xenon_set_ios;
> + host->mmc_host_ops.start_signal_voltage_switch =
> + xenon_start_signal_voltage_switch;
> + host->mmc_host_ops.init_card = xenon_init_card;
> + host->mmc_host_ops.execute_tuning = xenon_execute_tuning;
> +}
> +
> +static int xenon_probe_dt(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);
> + int err;
> + u32 slot_idx, nr_slot;
> + u32 tuning_count;
> + u32 reg;
> +
> + /* Standard MMC property */
> + err = mmc_of_parse(mmc);
> + if (err)
> + return err;
> +
> + /* Standard SDHCI property */
> + sdhci_get_of_property(pdev);
> +
> + /*
> + * 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!?
> + else
> + priv->emmc_slot = false;
> +
> + if (!of_property_read_u32(np, "marvell,xenon-slotno", &slot_idx)) {
> + nr_slot = sdhci_readl(host, SDHC_SYS_CFG_INFO);
> + nr_slot &= NR_SUPPORTED_SLOT_MASK;
> + if (unlikely(slot_idx > nr_slot)) {
> + dev_err(mmc_dev(mmc), "Slot Index %d exceeds Number of slots %d\n",
> + slot_idx, nr_slot);
> + return -EINVAL;
> + }
> + } else {
> + priv->slot_idx = 0x0;
> + }
> +
> + if (!of_property_read_u32(np, "marvell,xenon-tun-count",
> + &tuning_count)) {
> + if (unlikely(tuning_count >= TMR_RETUN_NO_PRESENT)) {
> + dev_err(mmc_dev(mmc), "Wrong Re-tuning Count. Set default value %d\n",
> + DEF_TUNING_COUNT);
> + tuning_count = DEF_TUNING_COUNT;
> + }
> + } else {
> + priv->tuning_count = DEF_TUNING_COUNT;
> + }
To make the code a bit easier...
Maybe set "priv->tuning_count = DEF_TUNING_COUNT" before the "if", and
instead have the of_property_read_u32() to update the value when set.
> +
> + if (of_property_read_bool(np, "marvell,xenon-mask-conflict-err")) {
> + reg = sdhci_readl(host, SDHC_SYS_EXT_OP_CTRL);
> + reg |= MASK_CMD_CONFLICT_ERROR;
> + sdhci_writel(host, reg, SDHC_SYS_EXT_OP_CTRL);
> + }
> +
> + return err;
> +}
> +
> +static int xenon_slot_probe(struct sdhci_host *host)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> + u8 slot_idx = priv->slot_idx;
> +
> + /* Enable slot */
> + xenon_enable_slot(host, slot_idx);
> +
> + /* Enable ACG */
> + xenon_set_acg(host, true);
> +
> + /* Enable Parallel Transfer Mode */
> + xenon_enable_slot_parallel_tran(host, slot_idx);
> +
> + priv->timing = MMC_TIMING_FAKE;
> + priv->clock = 0;
What are these used for?
> +
> + return 0;
> +}
> +
> +static void xenon_slot_remove(struct sdhci_host *host)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> + u8 slot_idx = priv->slot_idx;
> +
> + /* disable slot */
> + xenon_disable_slot(host, slot_idx);
> +}
> +
> +static int sdhci_xenon_probe(struct platform_device *pdev)
> +{
> + struct sdhci_pltfm_host *pltfm_host;
> + struct sdhci_host *host;
> + struct clk *clk, *axi_clk;
> + struct sdhci_xenon_priv *priv;
> + int err;
> +
> + host = sdhci_pltfm_init(pdev, &sdhci_xenon_pdata,
> + sizeof(struct sdhci_xenon_priv));
> + if (IS_ERR(host))
> + return PTR_ERR(host);
> +
> + pltfm_host = sdhci_priv(host);
> + priv = sdhci_pltfm_priv(pltfm_host);
> +
> + xenon_set_acg(host, false);
> +
> + /*
> + * Link Xenon specific mmc_host_ops function,
> + * to replace standard ones in sdhci_ops.
> + */
> + xenon_replace_mmc_host_ops(host);
> +
> + clk = devm_clk_get(&pdev->dev, "core");
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "Failed to setup input clk.\n");
> + err = PTR_ERR(clk);
> + goto free_pltfm;
> + }
> + clk_prepare_enable(clk);
Check error code.
> + pltfm_host->clk = clk;
Why not assign pltfm_host->clk immedately when doing devm_clk_get(),
that would make this a bit cleaner, right?
> +
> + /*
> + * Some SOCs require additional clock to
> + * manage AXI bus clock.
> + * It is optional.
> + */
> + axi_clk = devm_clk_get(&pdev->dev, "axi");
> + if (!IS_ERR(axi_clk)) {
> + clk_prepare_enable(axi_clk);
> + priv->axi_clk = axi_clk;
> + }
Same comments as for the above core clock.
> +
> + err = xenon_probe_dt(pdev);
> + if (err)
> + goto err_clk;
> +
> + err = xenon_slot_probe(host);
> + if (err)
> + goto err_clk;
> +
> + err = sdhci_add_host(host);
> + if (err)
> + goto remove_slot;
> +
> + return 0;
> +
> +remove_slot:
> + xenon_slot_remove(host);
> +err_clk:
> + clk_disable_unprepare(pltfm_host->clk);
> + if (!IS_ERR(axi_clk))
> + clk_disable_unprepare(axi_clk);
> +free_pltfm:
> + sdhci_pltfm_free(pdev);
> + return err;
> +}
> +
> +static int sdhci_xenon_remove(struct platform_device *pdev)
> +{
> + struct sdhci_host *host = platform_get_drvdata(pdev);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> + int dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xFFFFFFFF);
> +
> + xenon_slot_remove(host);
> +
> + sdhci_remove_host(host, dead);
> +
> + clk_disable_unprepare(pltfm_host->clk);
> + clk_disable_unprepare(priv->axi_clk);
> +
> + sdhci_pltfm_free(pdev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id sdhci_xenon_dt_ids[] = {
> + { .compatible = "marvell,xenon-sdhci",},
> + { .compatible = "marvell,armada-3700-sdhci",},
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, sdhci_xenon_dt_ids);
> +
> +static struct platform_driver sdhci_xenon_driver = {
> + .driver = {
> + .name = "xenon-sdhci",
> + .of_match_table = sdhci_xenon_dt_ids,
> + .pm = &sdhci_pltfm_pmops,
> + },
> + .probe = sdhci_xenon_probe,
> + .remove = sdhci_xenon_remove,
> +};
> +
> +module_platform_driver(sdhci_xenon_driver);
> +
> +MODULE_DESCRIPTION("SDHCI platform driver for Marvell Xenon SDHC");
> +MODULE_AUTHOR("Hu Ziji <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
> new file mode 100644
> index 000000000000..4601d0a4b22f
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-xenon.h
I don't think you need a specific header for this, let's instead just
put everthing in the c-file.
> @@ -0,0 +1,142 @@
> +/*
> + * Copyright (C) 2016 Marvell, All Rights Reserved.
> + *
> + * Author: Hu Ziji <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> + * 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.
> + */
> +#ifndef SDHCI_XENON_H_
> +#define SDHCI_XENON_H_
> +
> +#include <linux/clk.h>
> +#include <linux/mmc/card.h>
> +#include <linux/of.h>
> +#include "sdhci.h"
> +
> +/* Register Offset of SD Host Controller SOCP self-defined register */
> +#define SDHC_SYS_CFG_INFO 0x0104
> +#define SLOT_TYPE_SDIO_SHIFT 24
> +#define SLOT_TYPE_EMMC_MASK 0xFF
> +#define SLOT_TYPE_EMMC_SHIFT 16
> +#define SLOT_TYPE_SD_SDIO_MMC_MASK 0xFF
> +#define SLOT_TYPE_SD_SDIO_MMC_SHIFT 8
> +#define NR_SUPPORTED_SLOT_MASK 0x7
> +
> +#define SDHC_SYS_OP_CTRL 0x0108
> +#define AUTO_CLKGATE_DISABLE_MASK BIT(20)
> +#define SDCLK_IDLEOFF_ENABLE_SHIFT 8
> +#define SLOT_ENABLE_SHIFT 0
> +
> +#define SDHC_SYS_EXT_OP_CTRL 0x010C
> +#define MASK_CMD_CONFLICT_ERROR BIT(8)
> +
> +#define SDHC_SLOT_OP_STATUS_CTRL 0x0128
> +#define DELAY_90_DEGREE_MASK_EMMC5 BIT(7)
> +#define DELAY_90_DEGREE_SHIFT_EMMC5 7
> +#define EMMC_5_0_PHY_FIXED_DELAY_MASK 0x7F
> +#define EMMC_PHY_FIXED_DELAY_MASK 0xFF
> +#define EMMC_PHY_FIXED_DELAY_WINDOW_MIN (EMMC_PHY_FIXED_DELAY_MASK >> 3)
> +#define SDH_PHY_FIXED_DELAY_MASK 0x1FF
> +#define SDH_PHY_FIXED_DELAY_WINDOW_MIN (SDH_PHY_FIXED_DELAY_MASK >> 4)
> +
> +#define TUN_CONSECUTIVE_TIMES_SHIFT 16
> +#define TUN_CONSECUTIVE_TIMES_MASK 0x7
> +#define TUN_CONSECUTIVE_TIMES 0x4
> +#define TUNING_STEP_SHIFT 12
> +#define TUNING_STEP_MASK 0xF
> +#define TUNING_STEP_DIVIDER BIT(6)
> +
> +#define FORCE_SEL_INVERSE_CLK_SHIFT 11
> +
> +#define SDHC_SLOT_EMMC_CTRL 0x0130
> +#define ENABLE_DATA_STROBE BIT(24)
> +#define SET_EMMC_RSTN BIT(16)
> +#define DISABLE_RD_DATA_CRC BIT(14)
> +#define DISABLE_CRC_STAT_TOKEN BIT(13)
> +#define EMMC_VCCQ_MASK 0x3
> +#define EMMC_VCCQ_1_8V 0x1
> +#define EMMC_VCCQ_3_3V 0x3
> +
> +#define SDHC_SLOT_RETUNING_REQ_CTRL 0x0144
> +/* retuning compatible */
> +#define RETUNING_COMPATIBLE 0x1
> +
> +#define SDHC_SLOT_EXT_PRESENT_STATE 0x014C
> +#define LOCK_STATE 0x1
> +
> +#define SDHC_SLOT_DLL_CUR_DLY_VAL 0x0150
> +
> +/* Tuning Parameter */
> +#define TMR_RETUN_NO_PRESENT 0xF
> +#define DEF_TUNING_COUNT 0x9
> +
> +#define MMC_TIMING_FAKE 0xFF
> +
> +#define DEFAULT_SDCLK_FREQ (400000)
> +
> +/* Xenon specific Mode Select value */
> +#define XENON_SDHCI_CTRL_HS200 0x5
> +#define XENON_SDHCI_CTRL_HS400 0x6
For all defines above:
All these defines needs some *SDHCI* prefix. Can you please update that.
> +
> +struct sdhci_xenon_priv {
> + /*
> + * The bus_width, timing, and clock fields in below
> + * record the current setting of Xenon SDHC.
> + * Driver will call a Sampling Fixed Delay Adjustment
> + * if any setting is changed.
> + */
> + unsigned char bus_width;
> + unsigned char timing;
These two are not used. Please remove.
> + unsigned char tuning_count;
> + unsigned int clock;
"clock" isn't used, please remove.
> + struct clk *axi_clk;
> +
> + /* Slot idx */
> + u8 slot_idx;
> + /* Whether this slot is for eMMC */
> + bool emmc_slot;
> +
> + /*
> + * When initializing card, Xenon has to determine card type and
> + * adjust Sampling Fixed delay for the speed mode in which
> + * DLL tuning is not support.
> + * However, at that time, card structure is not linked to mmc_host.
> + * Thus a card pointer is added here to provide
> + * the delay adjustment function with the card structure
> + * of the card during initialization.
> + *
> + * It is only valid during initialization after it is updated in
> + * xenon_init_card().
> + * Do not access this variable in normal transfers after
> + * initialization completes.
> + */
> + struct mmc_card *card_candidate;
Not activley used in this change, please remove and let's discuss it
in the next step.
> +};
> +
> +static inline int enable_xenon_internal_clk(struct sdhci_host *host)
> +{
> + u32 reg;
> + u8 timeout;
> +
> + reg = sdhci_readl(host, SDHCI_CLOCK_CONTROL);
> + reg |= SDHCI_CLOCK_INT_EN;
> + sdhci_writel(host, reg, SDHCI_CLOCK_CONTROL);
> + /* Wait max 20 ms */
> + timeout = 20;
> + while (!((reg = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
> + & SDHCI_CLOCK_INT_STABLE)) {
> + if (timeout == 0) {
> + pr_err("%s: Internal clock never stabilised.\n",
> + mmc_hostname(host->mmc));
> + return -ETIMEDOUT;
> + }
> + timeout--;
> + mdelay(1);
> + }
> +
> + return 0;
> +}
> +#endif
> --
> git-series 0.8.10
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Ziji Hu @ 2016-11-24 10:38 UTC (permalink / raw)
To: Thomas Petazzoni, Marcin Wojtas
Cc: Gregory CLEMENT, Arnd Bergmann,
linux-arm-kernel@lists.infradead.org, Jimmy Xu, Andrew Lunn,
Ulf Hansson, Romain Perier, Liuliu Zhao, Peng Zhu,
linux-kernel@vger.kernel.org, Nadav Haklai, Victor Gu, Doug Jones,
Jisheng Zhang, Yehuda Yitschak, Xueping Liu, Hilbert Zhang,
Shiwu Zhang, Yu Cao, Sebastian
In-Reply-To: <20161124111029.035553ce@free-electrons.com>
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
>
^ permalink raw reply
* Re: [PATCH v2] mmc: block: delete packed command support
From: Jaehoon Chung @ 2016-11-24 10:23 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-mmc@vger.kernel.org, Ulf Hansson, Chunyan Zhang,
Baolin Wang, Namjae Jeon, Maya Erez
In-Reply-To: <CACRpkdZqL0b9i0r+TwRVYD-uw5PuoNbOsxKoghtHam+Wawp1Rg@mail.gmail.com>
On 11/24/2016 06:50 PM, Linus Walleij wrote:
> On Thu, Nov 24, 2016 at 6:24 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>
>>> Cc: Namjae Jeon <namjae.jeon@samsung.com>
>>> Cc: Maya Erez <qca_merez@qca.qualcomm.com>
>>> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> Did i send the acked-by tag? :)
>> I just agreed some this patch's points.
>> When i become assured, i will send the acked-by tag. is this way right?
>
> As per Documentation/SubmittingPatches, section 12:
>
> --------------------------
> Acked-by: is not as formal as Signed-off-by:. It is a record that the acker
> has at least reviewed the patch and has indicated acceptance. Hence patch
> mergers will sometimes manually convert an acker's "yep, looks good to me"
> into an Acked-by: (but note that it is usually better to ask for an
> explicit ack).
> ---------------------------
Thanks for sharing this information..i didn't read this documentation.
Anyway, until today, i have checked the I/O performance with Exynos3/4/5 boards which i have.
It seems that there is no I/O performance benefit about packed command at this time.
I don't know how other guys are thinking.
Explicit..
Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
Best Regards,
Jaehoon Chung
>
> Yours,
> Linus Walleij
>
>
>
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Thomas Petazzoni @ 2016-11-24 10:10 UTC (permalink / raw)
To: Marcin Wojtas
Cc: Gregory CLEMENT, Arnd Bergmann,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Jimmy Xu, Andrew Lunn, Ulf Hansson, Romain Perier, Liuliu Zhao,
Peng Zhu, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Nadav Haklai, Ziji Hu, Victor Gu, Doug Jones, Jisheng Zhang,
Yehuda Yitschak, Xueping Liu, Hilbert Zhang, Shiwu Zhang
In-Reply-To: <CAPv3WKddPHgpRU2_tVoDF=5Z-nqfFPxjgJ-+z9o-1tR2=fFvAA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
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.
Agreed.
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Arnd Bergmann @ 2016-11-24 10:04 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Thomas Petazzoni, Gregory CLEMENT, Jimmy Xu, Andrew Lunn,
Ulf Hansson, Romain Perier, Liuliu Zhao, Peng Zhu, Adrian Hunter,
Nadav Haklai, Ziji Hu, Victor Gu, Doug Jones, Jisheng Zhang,
Yehuda Yitschak, Wei(SOCP) Liu, Xueping Liu, Hilbert Zhang,
Shiwu Zhang, Yu Cao, Sebastian
In-Reply-To: <20161124104858.3604c11d-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
On Thursday, November 24, 2016 10:48:58 AM CET Thomas Petazzoni wrote:
> Hello,
>
> On Thu, 24 Nov 2016 10:44:48 +0100, Gregory CLEMENT wrote:
>
> > "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."
>
> I think this wording is still very confusing, and continues to cause
> confusion.
>
> We should just state that each Xenon controller supports a single slot,
> and that's it.
>
> The text still says "a single Xenon IP can support multiple slots",
> which continues to cause confusion.
Agreed. Ideally we'd find out why exactly the slot number must
be used for accessing some of the registers to have a better
explanation to put in there, aside from stating that only one
slot is supported but the number must be set.
Could it be that this is some form of pinmuxing, i.e. that each
controller could in theory be used for any of the slots but you
have to pick one of them?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Arnd Bergmann @ 2016-11-24 9:56 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Ulf Hansson, Adrian Hunter, linux-mmc-u79uwXL29TY76Z2rM5mHXA,
Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA, Thomas Petazzoni,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Ziji Hu,
Jack(SH) Zhu, Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao,
Doug Jones, Shiwu Zhang, Victor Gu, Wei(SOCP) Liu, Wilson Ding,
Xueping Liu <xpli>
In-Reply-To: <a05ffd140f4edc02fc3128db8445b2264cf38723.1477911954.git-series.gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
On Monday, October 31, 2016 12:09:56 PM CET Gregory CLEMENT wrote:
> From: Ziji Hu <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>
> 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-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>
Please explain in the changelog why this is not a generic
phy driver (or three of them).
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] mmc: block: delete packed command support
From: Linus Walleij @ 2016-11-24 9:53 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all@01.org, linux-mmc@vger.kernel.org, Ulf Hansson,
Chunyan Zhang, Baolin Wang, Namjae Jeon, Maya Erez
In-Reply-To: <201611241455.f15P3Xee%fengguang.wu@intel.com>
On Thu, Nov 24, 2016 at 7:35 AM, kbuild test robot <lkp@intel.com> wrote:
> Hi Linus,
>
> [auto build test WARNING on ulf.hansson-mmc/next]
> [cannot apply to linus/master linux/master v4.9-rc6 next-20161123]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
This is indeed a false build error.
Patches sent to linux-mmc@vger.kernel.org is probably better tested agains
the "next" branch on Ulf's MMC tree at:
https://git.kernel.org/cgit/linux/kernel/git/ulfh/mmc.git/
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2] mmc: block: delete packed command support
From: Linus Walleij @ 2016-11-24 9:50 UTC (permalink / raw)
To: Jaehoon Chung
Cc: linux-mmc@vger.kernel.org, Ulf Hansson, Chunyan Zhang,
Baolin Wang, Namjae Jeon, Maya Erez
In-Reply-To: <0e12314b-e1ce-d2ce-1136-02465fdbead4@samsung.com>
On Thu, Nov 24, 2016 at 6:24 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> Cc: Namjae Jeon <namjae.jeon@samsung.com>
>> Cc: Maya Erez <qca_merez@qca.qualcomm.com>
>> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> Did i send the acked-by tag? :)
> I just agreed some this patch's points.
> When i become assured, i will send the acked-by tag. is this way right?
As per Documentation/SubmittingPatches, section 12:
--------------------------
Acked-by: is not as formal as Signed-off-by:. It is a record that the acker
has at least reviewed the patch and has indicated acceptance. Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by: (but note that it is usually better to ask for an
explicit ack).
---------------------------
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Marcin Wojtas @ 2016-11-24 9:49 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Arnd Bergmann, linux-arm-kernel@lists.infradead.org, Jimmy Xu,
Andrew Lunn, Ulf Hansson, Romain Perier, Liuliu Zhao, Peng Zhu,
linux-kernel@vger.kernel.org, Nadav Haklai, Ziji Hu, Victor Gu,
Doug Jones, Jisheng Zhang, Yehuda Yitschak, Xueping Liu,
Hilbert Zhang, Shiwu Zhang, Yu Cao,
Sebastian Hesselbarth <sebastian.he>
In-Reply-To: <8737ihmctr.fsf@free-electrons.com>
Hi Gregory,
2016-11-24 10:44 GMT+01:00 Gregory CLEMENT <gregory.clement@free-electrons.com>:
> Hi Arnd,
>
> On jeu., nov. 24 2016, Arnd Bergmann <arnd@arndb.de> wrote:
>
>> On Thursday, November 24, 2016 10:22:31 AM CET Gregory CLEMENT wrote:
>>>
>>> I don't have an option for mmc in general, but using child node do not
>>> fit at all the xenon controller.
>>>
>>> For this controller each slot has its own set of register, so there is
>>> no common ressource to share so no advantage to use it. Using child node
>>> in our case will just make the code more complex for no benefit.
>>
>> If every slot has its own registers, what is it that makes up the
>> 'controller'? It sounds to me that you just have to adjust the terminology
>> and talk about multiple controllers then, with one slot per controller.
>>
>
> I agree and actually there were some words about in at the begining of
> the binding:
>
> "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."
>
> All the confusion came from the fact that we still need to identify a
> slot ID. For an obscure reason the hardware can't guess the slot ID from
> the address register."
>
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.
Best regards,
Marcin
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Thomas Petazzoni @ 2016-11-24 9:48 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Arnd Bergmann, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Jimmy Xu, Andrew Lunn, Ulf Hansson, Romain Perier, Liuliu Zhao,
Peng Zhu, linux-kernel@vger.kernel.org, Nadav Haklai, Ziji Hu,
Victor Gu, Doug Jones, Jisheng Zhang, Yehuda Yitschak,
Marcin Wojtas, Xueping Liu, Hilbert Zhang, Shiwu Zhang, Yu Cao,
Sebastian Hesselbarth <sebastian.hesselbart>
In-Reply-To: <8737ihmctr.fsf-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Hello,
On Thu, 24 Nov 2016 10:44:48 +0100, Gregory CLEMENT wrote:
> "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."
I think this wording is still very confusing, and continues to cause
confusion.
We should just state that each Xenon controller supports a single slot,
and that's it.
The text still says "a single Xenon IP can support multiple slots",
which continues to cause confusion.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Arnd Bergmann @ 2016-11-24 9:34 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Gregory CLEMENT, Jimmy Xu, Andrew Lunn, Ulf Hansson,
Romain Perier, Liuliu Zhao, Peng Zhu,
linux-kernel@vger.kernel.org, Nadav Haklai, Ziji Hu, Victor Gu,
Doug Jones, Jisheng Zhang, Yehuda Yitschak, Marcin Wojtas,
Xueping Liu, Hilbert Zhang, Shiwu Zhang, Yu Cao,
Sebastian Hesselbarth
In-Reply-To: <877f7tmduw.fsf@free-electrons.com>
On Thursday, November 24, 2016 10:22:31 AM CET Gregory CLEMENT wrote:
>
> I don't have an option for mmc in general, but using child node do not
> fit at all the xenon controller.
>
> For this controller each slot has its own set of register, so there is
> no common ressource to share so no advantage to use it. Using child node
> in our case will just make the code more complex for no benefit.
If every slot has its own registers, what is it that makes up the
'controller'? It sounds to me that you just have to adjust the terminology
and talk about multiple controllers then, with one slot per controller.
Arnd
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Gregory CLEMENT @ 2016-11-24 9:22 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ulf Hansson, Rob Herring, Ziji Hu, Adrian Hunter,
linux-mmc@vger.kernel.org, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, devicetree@vger.kernel.org,
Thomas Petazzoni, linux-arm-kernel@lists.infradead.org,
Jack(SH) Zhu, Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao,
Doug Jones, Shiwu Zhang, Victor Gu <xi>
In-Reply-To: <4031579.CBE32NHUoW@wuerfel>
Hi Arnd,
On jeu., nov. 24 2016, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thursday, November 24, 2016 10:05:45 AM CET Ulf Hansson wrote:
>> > You also mentioned other bindings using child nodes, but for this one
>> > we have one controller with only one set of register with multiple slots
>> > (Atmel is an example). Here each slot have it own set of register.
>> >
>> > Actually giving the fact that each slot is controlled by a different set
>> > of register I wonder why the hardware can't also deduce the slot number
>> > from the address register. For me it looks like an hardware bug but we
>> > have to deal with it.
>> >
>> > Do you still think we needchild node here?
>>
>> Using child-nodes for slots like what's done in the atmel case, is
>> currently broken. I would recommend to avoid using child-nodes for
>> slots, if possible.
>>
>> To give you some more background, currently the mmc core treats child
>> nodes as embedded non-removable cards or SDIO funcs. However, we can
>> change to make child-nodes also allowed to describe slots, but it
>> requires a specific compatible for "slots" and of course then we also
>> need to update the DT parsing of the child-nodes in the mmc core.
>>
>> Documentation/devicetree/bindings/mmc/mmc.txt
>> Documentation/devicetree/bindings/mmc/mmc-card.txt
>
> I don't see anything wrong with having child nodes for the slots
> even with the current binding, under one condition:
>
> The mmc.txt binding above must refer only to the child node, while
> the parent node conceptually becomes a plain bus or MFD that
> happens to encapsulate multiple MMC host controllers, and possibly
> provides some shared registers to them.
I don't have an option for mmc in general, but using child node do not
fit at all the xenon controller.
For this controller each slot has its own set of register, so there is
no common ressource to share so no advantage to use it. Using child node
in our case will just make the code more complex for no benefit.
Gregory
>
> Arnd
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Arnd Bergmann @ 2016-11-24 9:11 UTC (permalink / raw)
To: Ulf Hansson
Cc: Gregory CLEMENT, Rob Herring, Ziji Hu, Adrian Hunter,
linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thomas Petazzoni,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Jack(SH) Zhu, Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao,
Doug Jones, Shiwu Zhang, Vi
In-Reply-To: <CAPDyKFpoifsKkse7Fc-bbZAoa=QGT=9QOQ-4D=f60ptx0hzZsA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thursday, November 24, 2016 10:05:45 AM CET Ulf Hansson wrote:
> > You also mentioned other bindings using child nodes, but for this one
> > we have one controller with only one set of register with multiple slots
> > (Atmel is an example). Here each slot have it own set of register.
> >
> > Actually giving the fact that each slot is controlled by a different set
> > of register I wonder why the hardware can't also deduce the slot number
> > from the address register. For me it looks like an hardware bug but we
> > have to deal with it.
> >
> > Do you still think we needchild node here?
>
> Using child-nodes for slots like what's done in the atmel case, is
> currently broken. I would recommend to avoid using child-nodes for
> slots, if possible.
>
> To give you some more background, currently the mmc core treats child
> nodes as embedded non-removable cards or SDIO funcs. However, we can
> change to make child-nodes also allowed to describe slots, but it
> requires a specific compatible for "slots" and of course then we also
> need to update the DT parsing of the child-nodes in the mmc core.
>
> Documentation/devicetree/bindings/mmc/mmc.txt
> Documentation/devicetree/bindings/mmc/mmc-card.txt
I don't see anything wrong with having child nodes for the slots
even with the current binding, under one condition:
The mmc.txt binding above must refer only to the child node, while
the parent node conceptually becomes a plain bus or MFD that
happens to encapsulate multiple MMC host controllers, and possibly
provides some shared registers to them.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Ulf Hansson @ 2016-11-24 9:05 UTC (permalink / raw)
To: Gregory CLEMENT, Rob Herring
Cc: Ziji Hu, Adrian Hunter, linux-mmc@vger.kernel.org, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, devicetree@vger.kernel.org,
Thomas Petazzoni, linux-arm-kernel@lists.infradead.org,
Jack(SH) Zhu, Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao,
Doug Jones, Shiwu Zhang, Victor Gu, Wei(SOCP) Liu
In-Reply-To: <87d1hno2d7.fsf@free-electrons.com>
On 22 November 2016 at 18:23, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hi Rob,
>
> On jeu., nov. 10 2016, Ziji Hu <huziji@marvell.com> wrote:
>
> [...]
>
>>>> +
>>>> +- 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 was indeed what I tried to show in my answer for the 1st version:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/461860.html
>
> Maybe you missed it.
>
> You also mentioned other bindings using child nodes, but for this one
> we have one controller with only one set of register with multiple slots
> (Atmel is an example). Here each slot have it own set of register.
>
> Actually giving the fact that each slot is controlled by a different set
> of register I wonder why the hardware can't also deduce the slot number
> from the address register. For me it looks like an hardware bug but we
> have to deal with it.
>
> Do you still think we needchild node here?
Using child-nodes for slots like what's done in the atmel case, is
currently broken. I would recommend to avoid using child-nodes for
slots, if possible.
To give you some more background, currently the mmc core treats child
nodes as embedded non-removable cards or SDIO funcs. However, we can
change to make child-nodes also allowed to describe slots, but it
requires a specific compatible for "slots" and of course then we also
need to update the DT parsing of the child-nodes in the mmc core.
Documentation/devicetree/bindings/mmc/mmc.txt
Documentation/devicetree/bindings/mmc/mmc-card.txt
>
>>
>> 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.
>
Kind regards
Uffe
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox