Fixes and improvements for SDHCI on Armada 38x
Subsystems (3)
v2 cover Fixes and improvements for SDHCI on Armada 38x
Hi,
this series brings fixes and improvements for the SDHCI controller of the Armada 38x SoCs.
The main change for this second version was to put back the authorship on patch 2 and 4 from Marcin as I ported them a 3.10 kernel.
The first two patches are fixes and should be also applied on the stable branch (I added stable in copy for this).
The first one removes the SDR50 and DDR50 mode timing from the capabilities of the controller because the current implementation doesn't support it.
The second one fix controller's caps according to the limitation of the hardware.
The third one extends the Device Tree binding of the Armada 38x. It allows using the SDIO3 configuration register.
The forth patch adds the support of this new register in the driver. Thanks to this one, specific clock adjustments can be done in order to support the SDR50 and DDR50 modes timing.
The fifth patch is device tree clean-up.
The sixth patch update the SDHCI node on Armada 38x in order to use the new register and then to be able to support the SDR50 and DDR50 modes timing.
Finally, the seventh patch add the description of SDHCI for the Armada 388 RD board which was missing.
Patches 1 to 4 should be merged through the mmc tree and patches 5 to 7 should be merged through mvebu and then arm-soc.
Patch 4 depend on patch 2.
Patch 2 depend on patch 1.
And patch 1 depend on commit aa8165f91442 "mmc: sdhci-pxav3: do the mbus window configuration after enabling clocks" which have been merged.
But as all this patch should be merged through the same tree it would not be a problem
The patches 5 to 7 depend on the patches already merged in mvebu.
As they are fixes, patches 1 and 2 should be merged in 3.18-rc or at least in 3.19 and then they will be part of 3.18.1. For the other patches it would be nice if they could be part of 3.19.
Thanks,
Gregory
Changelog:
- Put back Marcin as author of patch 2 and 3
- Removed MMC_CAP_1_8V_DDR in the probe function
Gregory CLEMENT (5):
mmc: sdhci-pxav3: Fix SDR50 and DDR50 capabilities for the Armada 38x
flavor
mmc: sdhci-pxav3: Extend binding with SDIO3 conf reg for the Armada
38x
ARM: mvebu: Use macros for interrupt flags on Armada 38x sdhci node
ARM: mvebu: Update the SDHCI node on Armada 38x
ARM: mvebu: Add Device Tree description of SDHCI for Armada 388 RD
Marcin Wojtas (2):
mmc: sdhci-pxav3: Fix Armada 38x controller's caps according to
erratum ERR-7878951
mmc: sdhci-pxav3: Modify clock settings for the SDR50 and DDR50 modes
.../devicetree/bindings/mmc/sdhci-pxa.txt | 15 ++-- arch/arm/boot/dts/armada-388-rd.dts | 10 +++ arch/arm/boot/dts/armada-38x.dtsi | 7 +- drivers/mmc/host/sdhci-pxav3.c | 82 +++++++++++++++++++++- 4 files changed, 105 insertions(+), 9 deletions(-)
Signature · 1 line
2.1.0
PATCH v2 1/7 mmc: sdhci-pxav3: Fix SDR50 and DDR50 capabilities for the Armada 38x flavor
According to erratum 'FE-2946959' both SDR50 and DDR50 modes require specific clock adjustments in SDIO3 Configuration register. However, this register was not part of the device tree binding. Even if the binding can (and will) be extended we still need handling the case where this register was not available. In this case we use the SDHCI_QUIRK_MISSING_CAPS quirk remove them from the capabilities.
This commit is based on the work done by Marcin Wojtas<[email protected]>
--- drivers/mmc/host/sdhci-pxav3.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
drivers/mmc/host/sdhci-pxav3.c +18 -0
@@ -118,6 +118,20 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,return 0 ; } +static int armada_38x_quirks (struct sdhci_host *host) +{ + host->quirks |=SDHCI_QUIRK_MISSING_CAPS ; +/* +* According to erratum 'FE-2946959' both SDR50 and DDR50 +* modes require specific clock adjustments in SDIO3 +* Configuration register, if the adjustment is not done, +* remove them from the capabilities. +*/ + host->caps1 =sdhci_readl (host,SDHCI_CAPABILITIES_1 ); + host->caps1 &= ~(SDHCI_SUPPORT_SDR50 |SDHCI_SUPPORT_DDR50 ); +return 0 ; +} +static void pxav3_reset (struct sdhci_host *host,u8 mask) {struct platform_device *pdev =to_platform_device (mmc_dev (host->mmc));
@@ -319,6 +333,9 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)clk_prepare_enable (pxa->clk_core);if (of_device_is_compatible (np,"marvell,armada-380-sdhci" )) { + ret =armada_38x_quirks (host); +if (ret <0 ) + goto err_quirks; ret =mv_conf_mbus_windows (pdev,mv_mbus_dram_info ());if (ret <0 ) goto err_mbus_win;
@@ -400,6 +417,7 @@ err_mbus_win:if (!IS_ERR (pxa->clk_core))clk_disable_unprepare (pxa->clk_core); err_clk_get: +err_quirks:sdhci_pltfm_free (pdev);return ret; }
Signature · 1 line
2.1.0
PATCH v2 1/7 mmc: sdhci-pxav3: Fix SDR50 and DDR50 capabilities for the Armada 38x flavor
Quoted 22 lines · Gregory CLEMENT
According to erratum 'FE-2946959' both SDR50 and DDR50 modes require specific clock adjustments in SDIO3 Configuration register. However, this register was not part of the device tree binding. Even if the binding can (and will) be extended we still need handling the case where this register was not available. In this case we use the
SDHCI_QUIRK_MISSING_CAPSquirk remove them from the capabilities.This commit is based on the work done by Marcin Wojtas<[email protected]>
Fixes 5491ce3f79ee ("mmc: sdhci-pxav3: add support for the Armada 38x SDHCI controller")Cc <[email protected]> # v3.15+Signed-off-by Gregory CLEMENT <[email protected]>Signed-off-by Marcin Wojtas <[email protected]>--- drivers/mmc/host/sdhci-pxav3.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/mmc/host/sdhci-pxav3.c +0
-0
drivers/mmc/host/sdhci-pxav3.c static int mv_conf_mbus_windows(struct platform_device *pdev, · line 121
Quoted 4 lines
drivers/mmc/host/sdhci-pxav3.c +1
-0@@ -118,6 +118,20 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,return 0 ; } +static int armada_38x_quirks (struct sdhci_host *host)
Seems like this function can be void instead of always returning 0.
Quoted 35 lines
drivers/mmc/host/sdhci-pxav3.c +17
-1+{ + host->quirks |=SDHCI_QUIRK_MISSING_CAPS ; +/* +* According to erratum 'FE-2946959' both SDR50 and DDR50 +* modes require specific clock adjustments in SDIO3 +* Configuration register, if the adjustment is not done, +* remove them from the capabilities. +*/ + host->caps1 =sdhci_readl (host,SDHCI_CAPABILITIES_1 ); + host->caps1 &= ~(SDHCI_SUPPORT_SDR50 |SDHCI_SUPPORT_DDR50 ); +return 0 ; +} +static void pxav3_reset (struct sdhci_host *host,u8 mask) {struct platform_device *pdev =to_platform_device (mmc_dev (host->mmc));@@ -319,6 +333,9 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)clk_prepare_enable (pxa->clk_core);if (of_device_is_compatible (np,"marvell,armada-380-sdhci" )) { + ret =armada_38x_quirks (host); +if (ret <0 ) + goto err_quirks; ret =mv_conf_mbus_windows (pdev,mv_mbus_dram_info ());if (ret <0 ) goto err_mbus_win;@@ -400,6 +417,7 @@ err_mbus_win:if (!IS_ERR (pxa->clk_core))clk_disable_unprepare (pxa->clk_core); err_clk_get: +err_quirks:sdhci_pltfm_free (pdev);return ret; } --
Quoted 2 lines
2.1.0
Kind regards
Uffe
PATCH v2 1/7 mmc: sdhci-pxav3: Fix SDR50 and DDR50 capabilities for the Armada 38x flavor
Hi Ulf,
Quoted 27 lines · Ulf Hansson
Quoted 25 lines · Gregory CLEMENT
According to erratum 'FE-2946959' both SDR50 and DDR50 modes require specific clock adjustments in SDIO3 Configuration register. However, this register was not part of the device tree binding. Even if the binding can (and will) be extended we still need handling the case where this register was not available. In this case we use the
SDHCI_QUIRK_MISSING_CAPSquirk remove them from the capabilities.This commit is based on the work done by Marcin Wojtas<[email protected]>
Fixes 5491ce3f79ee ("mmc: sdhci-pxav3: add support for the Armada 38x SDHCI controller")Cc <[email protected]> # v3.15+Signed-off-by Gregory CLEMENT <[email protected]>Signed-off-by Marcin Wojtas <[email protected]>--- drivers/mmc/host/sdhci-pxav3.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)drivers/mmc/host/sdhci-pxav3.c +0
-0@@ -118,6 +118,20 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,return 0 ; }
Quoted 3 lines
drivers/mmc/host/sdhci-pxav3.c static int mv_conf_mbus_windows(struct platform_device *pdev, · line 121
Quoted 1 line
drivers/mmc/host/sdhci-pxav3.c +1
-0+static int armada_38x_quirks (struct sdhci_host *host)Seems like this function can be void instead of always returning 0.
In patch 4 "mmc: sdhci-pxav3: Modify clock settings for the SDR50 and DDR50 modes", this function can return other values than 0.
I could change the prototype in patch 4, but it would also imply removing the test of the return value in this patch and adding in back patch 4. By returning a value in this patch, it reduced the amount of change over the patches.
But if you still prefer than I this function return void in this patch, I can do it.
Thanks,
Gregory
Quoted 42 lines
Quoted 36 lines
drivers/mmc/host/sdhci-pxav3.c +17
-1+{ + host->quirks |=SDHCI_QUIRK_MISSING_CAPS ; +/* +* According to erratum 'FE-2946959' both SDR50 and DDR50 +* modes require specific clock adjustments in SDIO3 +* Configuration register, if the adjustment is not done, +* remove them from the capabilities. +*/ + host->caps1 =sdhci_readl (host,SDHCI_CAPABILITIES_1 ); + host->caps1 &= ~(SDHCI_SUPPORT_SDR50 |SDHCI_SUPPORT_DDR50 ); +return 0 ; +} +static void pxav3_reset (struct sdhci_host *host,u8 mask) {struct platform_device *pdev =to_platform_device (mmc_dev (host->mmc));@@ -319,6 +333,9 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)clk_prepare_enable (pxa->clk_core);if (of_device_is_compatible (np,"marvell,armada-380-sdhci" )) { + ret =armada_38x_quirks (host); +if (ret <0 ) + goto err_quirks; ret =mv_conf_mbus_windows (pdev,mv_mbus_dram_info ());if (ret <0 ) goto err_mbus_win;@@ -400,6 +417,7 @@ err_mbus_win:if (!IS_ERR (pxa->clk_core))clk_disable_unprepare (pxa->clk_core); err_clk_get: +err_quirks:sdhci_pltfm_free (pdev);return ret; } --Quoted 2 lines
2.1.0
Kind regards
Uffe
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH v2 1/7 mmc: sdhci-pxav3: Fix SDR50 and DDR50 capabilities for the Armada 38x flavor
[...]
Quoted 5 lines
Quoted 1 line
Seems like this function can be void instead of always returning 0.
In patch 4 "mmc: sdhci-pxav3: Modify clock settings for the SDR50 and DDR50 modes", this function can return other values than 0.
Quoted 7 lines
I could change the prototype in patch 4, but it would also imply removing the test of the return value in this patch and adding in back patch 4. By returning a value in this patch, it reduced the amount of change over the patches.
But if you still prefer than I this function return void in this patch, I can do it.
No worries, let's keep it as an int. But then I have a few other comments, see below.
Quoted 27 lines
Thanks,
Gregory
Quoted 20 lines
Quoted 20 lines
drivers/mmc/host/sdhci-pxav3.c +13
-0+{ + host->quirks |=SDHCI_QUIRK_MISSING_CAPS ; +/* +* According to erratum 'FE-2946959' both SDR50 and DDR50 +* modes require specific clock adjustments in SDIO3 +* Configuration register, if the adjustment is not done, +* remove them from the capabilities. +*/ + host->caps1 =sdhci_readl (host,SDHCI_CAPABILITIES_1 ); + host->caps1 &= ~(SDHCI_SUPPORT_SDR50 |SDHCI_SUPPORT_DDR50 ); +return 0 ; +} +static void pxav3_reset (struct sdhci_host *host,u8 mask) {struct platform_device *pdev =to_platform_device (mmc_dev (host->mmc));@@ -319,6 +333,9 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)clk_prepare_enable (pxa->clk_core);
Quoted 3 lines
Quoted 3 lines
drivers/mmc/host/sdhci-pxav3.c static int sdhci_pxav3_probe(struct platform_device *pdev) · line 337
Quoted 3 lines
drivers/mmc/host/sdhci-pxav3.c +2
-0if (of_device_is_compatible (np,"marvell,armada-380-sdhci" )) { + ret =armada_38x_quirks (host); +if (ret <0 )
Since in patch 4 you return a proper error code, let's also adopt to that here by changing to:
"if (IS_ERR(ret))
Quoted 9 lines
Quoted 9 lines
drivers/mmc/host/sdhci-pxav3.c err_mbus_win: · line 420
Quoted 9 lines
drivers/mmc/host/sdhci-pxav3.c +2
-0+ goto err_quirks; ret =mv_conf_mbus_windows (pdev,mv_mbus_dram_info ());if (ret <0 ) goto err_mbus_win;@@ -400,6 +417,7 @@ err_mbus_win:if (!IS_ERR (pxa->clk_core))clk_disable_unprepare (pxa->clk_core); err_clk_get: +err_quirks:
You don't need the new label, you could use "err_clk_get".
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/mmc/host/sdhci-pxav3.c +0
-1sdhci_pltfm_free (pdev);return ret; } --2.1.0
Kind regards
Uffe
PATCH v2 1/7 mmc: sdhci-pxav3: Fix SDR50 and DDR50 capabilities for the Armada 38x flavor
Hi Ulf,
Quoted 12 lines · Ulf Hansson
[...]
Quoted 10 lines
Quoted 1 line
Seems like this function can be void instead of always returning 0.
In patch 4 "mmc: sdhci-pxav3: Modify clock settings for the SDR50 and DDR50 modes", this function can return other values than 0.
I could change the prototype in patch 4, but it would also imply removing the test of the return value in this patch and adding in back patch 4. By returning a value in this patch, it reduced the amount of change over the patches.
Quoted 5 lines
Quoted 2 lines
But if you still prefer than I this function return void in this patch, I can do it.
No worries, let's keep it as an int. But then I have a few other comments, see below.
OK
Quoted 28 lines
Quoted 28 lines
Thanks,
Gregory
Quoted 20 lines
Quoted 20 lines
drivers/mmc/host/sdhci-pxav3.c +13
-0+{ + host->quirks |=SDHCI_QUIRK_MISSING_CAPS ; +/* +* According to erratum 'FE-2946959' both SDR50 and DDR50 +* modes require specific clock adjustments in SDIO3 +* Configuration register, if the adjustment is not done, +* remove them from the capabilities. +*/ + host->caps1 =sdhci_readl (host,SDHCI_CAPABILITIES_1 ); + host->caps1 &= ~(SDHCI_SUPPORT_SDR50 |SDHCI_SUPPORT_DDR50 ); +return 0 ; +} +static void pxav3_reset (struct sdhci_host *host,u8 mask) {struct platform_device *pdev =to_platform_device (mmc_dev (host->mmc));@@ -319,6 +333,9 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)clk_prepare_enable (pxa->clk_core);
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
drivers/mmc/host/sdhci-pxav3.c static int sdhci_pxav3_probe(struct platform_device *pdev) · line 337
Quoted 3 lines
drivers/mmc/host/sdhci-pxav3.c +2
-0if (of_device_is_compatible (np,"marvell,armada-380-sdhci" )) { + ret =armada_38x_quirks (host); +if (ret <0 )Since in patch 4 you return a proper error code, let's also adopt to that here by changing to:
"if (
IS_ERR(ret))
The function returns an int and IS_ERR expects a pointer. I am not sure this macro would be appropriate here.
Quoted 12 lines
Quoted 10 lines
Quoted 10 lines
drivers/mmc/host/sdhci-pxav3.c err_mbus_win: · line 420
Quoted 10 lines
drivers/mmc/host/sdhci-pxav3.c +2
-0+ goto err_quirks; ret =mv_conf_mbus_windows (pdev,mv_mbus_dram_info ());if (ret <0 ) goto err_mbus_win;@@ -400,6 +417,7 @@ err_mbus_win:if (!IS_ERR (pxa->clk_core))clk_disable_unprepare (pxa->clk_core); err_clk_get: +err_quirks:You don't need the new label, you could use "
err_clk_get".
Right.
Thanks,
Gregory
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH v2 1/7 mmc: sdhci-pxav3: Fix SDR50 and DDR50 capabilities for the Armada 38x flavor
Quoted 58 lines · Gregory CLEMENT
Hi Ulf,
Quoted 12 lines · Ulf Hansson
[...]
Quoted 10 lines
Quoted 1 line
Seems like this function can be void instead of always returning 0.
In patch 4 "mmc: sdhci-pxav3: Modify clock settings for the SDR50 and DDR50 modes", this function can return other values than 0.
I could change the prototype in patch 4, but it would also imply removing the test of the return value in this patch and adding in back patch 4. By returning a value in this patch, it reduced the amount of change over the patches.
Quoted 5 lines
Quoted 2 lines
But if you still prefer than I this function return void in this patch, I can do it.
No worries, let's keep it as an int. But then I have a few other comments, see below.
OK
Quoted 35 lines
Quoted 28 lines
Thanks,
Gregory
Quoted 20 lines
Quoted 20 lines
drivers/mmc/host/sdhci-pxav3.c +13
-0+{ + host->quirks |=SDHCI_QUIRK_MISSING_CAPS ; +/* +* According to erratum 'FE-2946959' both SDR50 and DDR50 +* modes require specific clock adjustments in SDIO3 +* Configuration register, if the adjustment is not done, +* remove them from the capabilities. +*/ + host->caps1 =sdhci_readl (host,SDHCI_CAPABILITIES_1 ); + host->caps1 &= ~(SDHCI_SUPPORT_SDR50 |SDHCI_SUPPORT_DDR50 ); +return 0 ; +} +static void pxav3_reset (struct sdhci_host *host,u8 mask) {struct platform_device *pdev =to_platform_device (mmc_dev (host->mmc));@@ -319,6 +333,9 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)clk_prepare_enable (pxa->clk_core);Quoted 3 lines
Quoted 3 lines
drivers/mmc/host/sdhci-pxav3.c static int sdhci_pxav3_probe(struct platform_device *pdev) · line 337
Quoted 3 lines
drivers/mmc/host/sdhci-pxav3.c +2
-0if (of_device_is_compatible (np,"marvell,armada-380-sdhci" )) { + ret =armada_38x_quirks (host); +if (ret <0 )Since in patch 4 you return a proper error code, let's also adopt to that here by changing to:
Quoted 4 lines
Quoted 1 line
"if (
IS_ERR(ret))The function returns an int and
IS_ERRexpects a pointer. I am not sure this macro would be appropriate here.
You are right. Don't know what I was thinking. :-)
Kind regards
Uffe
PATCH v2 2/7 mmc: sdhci-pxav3: Fix Armada 38x controller's caps according to erratum ERR-7878951
In reply to Gregory CLEMENTFrom: Marcin Wojtas <[email protected]>
According to erratum 'ERR-7878951' Armada 38x SDHCI controller has different capabilities than the ones shown in its registers:
- it doesn't support the voltage switching: it can work either with 3.3V or 1.8V supply
- it doesn't support the SDR104 mode
- SDR50 mode doesn't need tuning
The SDHCI_QUIRK_MISSING_CAPS quirk is used for updating the capabilities accordingly.
[gregory.clement at free-electrons.com: port from 3.10]
--- drivers/mmc/host/sdhci-pxav3.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)
drivers/mmc/host/sdhci-pxav3.c +19 -5
@@ -118,8 +118,11 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,return 0 ; } -static int armada_38x_quirks (struct sdhci_host *host) +static int armada_38x_quirks (struct platform_device *pdev, +struct sdhci_host *host) { +struct device_node *np = pdev->dev.of_node; + host->quirks |=SDHCI_QUIRK_MISSING_CAPS ;/* * According to erratum 'FE-2946959' both SDR50 and DDR50
@@ -129,6 +132,20 @@ static int armada_38x_quirks(struct sdhci_host *host)*/ host->caps1 =sdhci_readl (host,SDHCI_CAPABILITIES_1 ); host->caps1 &= ~(SDHCI_SUPPORT_SDR50 |SDHCI_SUPPORT_DDR50 ); + +/* +* According to erratum 'ERR-7878951' Armada 38x SDHCI +* controller has different capabilities than the ones shown +* in its registers +*/ + host->caps =sdhci_readl (host,SDHCI_CAPABILITIES ); +if (of_get_property (np,"no-1-8-v" ,NULL )) { + host->caps &= ~SDHCI_CAN_VDD_180 ; + host->mmc->caps &= ~MMC_CAP_1_8V_DDR ; + }else + host->caps &= ~SDHCI_CAN_VDD_330 ; + host->caps1 &= ~(SDHCI_SUPPORT_SDR104 |SDHCI_USE_SDR50_TUNING ); +return 0 ; }
@@ -333,7 +350,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)clk_prepare_enable (pxa->clk_core);if (of_device_is_compatible (np,"marvell,armada-380-sdhci" )) { - ret =armada_38x_quirks (host); + ret =armada_38x_quirks (pdev, host);if (ret <0 ) goto err_quirks; ret =mv_conf_mbus_windows (pdev,mv_mbus_dram_info ());
@@ -341,9 +358,6 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) goto err_mbus_win; } -/* enable 1/8V DDR capable */ - host->mmc->caps |=MMC_CAP_1_8V_DDR ; - match =of_match_device (of_match_ptr (sdhci_pxav3_of_match), &pdev->dev);if (match) { ret =mmc_of_parse (host->mmc);
Signature · 1 line
2.1.0
PATCH v2 2/7 mmc: sdhci-pxav3: Fix Armada 38x controller's caps according to erratum ERR-7878951
[...]
drivers/mmc/host/sdhci-pxav3.c static int armada_38x_quirks(struct sdhci_host *host) · line 142
Quoted 7 lines
drivers/mmc/host/sdhci-pxav3.c +7
-0+/* +* According to erratum 'ERR-7878951' Armada 38x SDHCI +* controller has different capabilities than the ones shown +* in its registers +*/ + host->caps =sdhci_readl (host,SDHCI_CAPABILITIES ); +if (of_get_property (np,"no-1-8-v" ,NULL )) {
Please use of_property_read_bool(np, "no-1-8-v")
drivers/mmc/host/sdhci-pxav3.c static int armada_38x_quirks(struct sdhci_host *host) · line 144
Quoted 2 lines
drivers/mmc/host/sdhci-pxav3.c +2
-0+ host->caps &= ~SDHCI_CAN_VDD_180 ; + host->mmc->caps &= ~MMC_CAP_1_8V_DDR ;
Is SDHCI_CAN_VDD_330 always set elsewhere in this case?
drivers/mmc/host/sdhci-pxav3.c static int armada_38x_quirks(struct sdhci_host *host) · line 146
Quoted 2 lines
drivers/mmc/host/sdhci-pxav3.c +2
-0+ }else + host->caps &= ~SDHCI_CAN_VDD_330 ;
If one branch in an if-else pair is braced, both sides should be (as Documentation/CodingStyle says). Please brace the else case.
Thanks,
Mark.
PATCH v2 2/7 mmc: sdhci-pxav3: Fix Armada 38x controller's caps according to erratum ERR-7878951
Hi Mark
2015-01-23 12:03 GMT+01:00 Mark Rutland <[email protected]>:
Quoted 10 lines
[...]
drivers/mmc/host/sdhci-pxav3.c static int armada_38x_quirks(struct sdhci_host *host) · line 142
Quoted 7 lines
drivers/mmc/host/sdhci-pxav3.c +7
-0+/* +* According to erratum 'ERR-7878951' Armada 38x SDHCI +* controller has different capabilities than the ones shown +* in its registers +*/ + host->caps =sdhci_readl (host,SDHCI_CAPABILITIES ); +if (of_get_property (np,"no-1-8-v" ,NULL )) {
Quoted 6 lines
Please use
of_property_read_bool(np, "no-1-8-v")drivers/mmc/host/sdhci-pxav3.c static int armada_38x_quirks(struct sdhci_host *host) · line 144
Quoted 2 lines
drivers/mmc/host/sdhci-pxav3.c +2
-0+ host->caps &= ~SDHCI_CAN_VDD_180 ; + host->mmc->caps &= ~MMC_CAP_1_8V_DDR ;Is
SDHCI_CAN_VDD_330always set elsewhere in this case?
Yes, it's always set in SDHCI_CAPABILITIES register of Armada 38x (assigned to host->caps few lines above), as well as SDHCI_CAN_VDD_180. The point is, that in reality they can't be used simultaneously for this device, i.e. voltage switching is impossible.
Thanks,
Marcin
Quoted 4 lines
Quoted 3 lines
drivers/mmc/host/sdhci-pxav3.c +2
-0+ }else + host->caps &= ~SDHCI_CAN_VDD_330 ;
PATCH v2 2/7 mmc: sdhci-pxav3: Fix Armada 38x controller's caps according to erratum ERR-7878951
In reply to Gregory CLEMENTDear Gregory,
Quoted 19 lines
drivers/mmc/host/sdhci-pxav3.c +6
-1@@ -118,8 +118,11 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,return 0 ; } -static int armada_38x_quirks (struct sdhci_host *host) +static int armada_38x_quirks (struct platform_device *pdev, +struct sdhci_host *host) { +struct device_node *np = pdev->dev.of_node; + host->quirks |=SDHCI_QUIRK_MISSING_CAPS ;/* * According to erratum 'FE-2946959' both SDR50 and DDR50 @@ -129,6 +132,20 @@ static int armada_38x_quirks(struct sdhci_host *host)*/ host->caps1 =sdhci_readl (host,SDHCI_CAPABILITIES_1 ); host->caps1 &= ~(SDHCI_SUPPORT_SDR50 |SDHCI_SUPPORT_DDR50 ); + +/*
drivers/mmc/host/sdhci-pxav3.c static int armada_38x_quirks(struct sdhci_host *host) · line 146
Quoted 10 lines
drivers/mmc/host/sdhci-pxav3.c +10
-0+* According to erratum 'ERR-7878951' Armada 38x SDHCI +* controller has different capabilities than the ones shown +* in its registers +*/ + host->caps =sdhci_readl (host,SDHCI_CAPABILITIES ); +if (of_get_property (np,"no-1-8-v" ,NULL )) { + host->caps &= ~SDHCI_CAN_VDD_180 ; + host->mmc->caps &= ~MMC_CAP_1_8V_DDR ; + }else + host->caps &= ~SDHCI_CAN_VDD_330 ;
In this "else" there is one thing missinghost->mmc->caps |= MMC_CAP_1_8V_DDR;
because it's not set by default, however it's not a must - please see my remark below.
Quoted 15 lines
drivers/mmc/host/sdhci-pxav3.c +3
-1+ host->caps1 &= ~(SDHCI_SUPPORT_SDR104 |SDHCI_USE_SDR50_TUNING ); +return 0 ; }@@ -333,7 +350,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)clk_prepare_enable (pxa->clk_core);if (of_device_is_compatible (np,"marvell,armada-380-sdhci" )) { - ret =armada_38x_quirks (host); + ret =armada_38x_quirks (pdev, host);if (ret <0 ) goto err_quirks; ret =mv_conf_mbus_windows (pdev,mv_mbus_dram_info ());
drivers/mmc/host/sdhci-pxav3.c static int sdhci_pxav3_probe(struct platform_device *pdev) · line 346
Quoted 6 lines
drivers/mmc/host/sdhci-pxav3.c +0
-3@@ -341,9 +358,6 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) goto err_mbus_win; } -/* enable 1/8V DDR capable */ - host->mmc->caps |=MMC_CAP_1_8V_DDR ; -
Now, with this change MMC_CAP_1_8V_DDR is disabled for devices other than Armada 38x, that use sdhci-pxav3. There are two options:
1. Move those lines above the place where armada_38x_quirks is called (IMO the easiest option - 'else' in this function would not need to be supplemented, as I pointed above)
2. Leave it "as is" here, but a condition below is needed (+ supplementation of 'else' in armada_38x_quirks)
if (!of_device_is_compatible(np, "marvell,armada-380-sdhci"))
Best regards,
Marcin
PATCH v2 2/7 mmc: sdhci-pxav3: Fix Armada 38x controller's caps according to erratum ERR-7878951
Hi Marcin,
Quoted 56 lines · Marcin Wojtas
Dear Gregory,
Quoted 19 lines
drivers/mmc/host/sdhci-pxav3.c +6
-1@@ -118,8 +118,11 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,return 0 ; } -static int armada_38x_quirks (struct sdhci_host *host) +static int armada_38x_quirks (struct platform_device *pdev, +struct sdhci_host *host) { +struct device_node *np = pdev->dev.of_node; + host->quirks |=SDHCI_QUIRK_MISSING_CAPS ;/* * According to erratum 'FE-2946959' both SDR50 and DDR50 @@ -129,6 +132,20 @@ static int armada_38x_quirks(struct sdhci_host *host)*/ host->caps1 =sdhci_readl (host,SDHCI_CAPABILITIES_1 ); host->caps1 &= ~(SDHCI_SUPPORT_SDR50 |SDHCI_SUPPORT_DDR50 ); + +/* drivers/mmc/host/sdhci-pxav3.c static int armada_38x_quirks(struct sdhci_host *host) · line 146
Quoted 10 lines
drivers/mmc/host/sdhci-pxav3.c +10
-0+* According to erratum 'ERR-7878951' Armada 38x SDHCI +* controller has different capabilities than the ones shown +* in its registers +*/ + host->caps =sdhci_readl (host,SDHCI_CAPABILITIES ); +if (of_get_property (np,"no-1-8-v" ,NULL )) { + host->caps &= ~SDHCI_CAN_VDD_180 ; + host->mmc->caps &= ~MMC_CAP_1_8V_DDR ; + }else + host->caps &= ~SDHCI_CAN_VDD_330 ;In this "else" there is one thing missing
host->mmc->caps|=MMC_CAP_1_8V_DDR;
because it's not set by default, however it's not a must - please see my remark below.Quoted 18 lines
drivers/mmc/host/sdhci-pxav3.c +3
-1+ host->caps1 &= ~(SDHCI_SUPPORT_SDR104 |SDHCI_USE_SDR50_TUNING ); +return 0 ; }@@ -333,7 +350,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)clk_prepare_enable (pxa->clk_core);if (of_device_is_compatible (np,"marvell,armada-380-sdhci" )) { - ret =armada_38x_quirks (host); + ret =armada_38x_quirks (pdev, host);if (ret <0 ) goto err_quirks; ret =mv_conf_mbus_windows (pdev,mv_mbus_dram_info ());@@ -341,9 +358,6 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) goto err_mbus_win; }
Quoted 13 lines
drivers/mmc/host/sdhci-pxav3.c static int sdhci_pxav3_probe(struct platform_device *pdev) · line 346
Quoted 3 lines
drivers/mmc/host/sdhci-pxav3.c +0
-3-/* enable 1/8V DDR capable */ - host->mmc->caps |=MMC_CAP_1_8V_DDR ; -Now, with this change
MMC_CAP_1_8V_DDRis disabled for devices other than Armada 38x, that use sdhci-pxav3. There are two options:
1. Move those lines above the place wherearmada_38x_quirksis called (IMO the easiest option - 'else' in this function would not need to be supplemented, as I pointed above)
2. Leave it "as is" here, but a condition below is needed (+ supplementation of 'else' inarmada_38x_quirks)
if (!of_device_is_compatible(np, "marvell,armada-380-sdhci"))
I will take the first option.
A third version is coming soon addressing also Mark's comments.
Thanks,
Gregory
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH v2 3/7 mmc: sdhci-pxav3: Extend binding with SDIO3 conf reg for the Armada 38x
In reply to Gregory CLEMENTThe SDHCI unit used on the Armada 38x needs using an extra register to do specific clock adjustments in order to support the SDR50 and DDR50 modes. This patch extends the binding to allow using this register.
--- Documentation/devicetree/bindings/mmc/sdhci-pxa.txt | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/mmc/sdhci-pxa.txt +11 -4
@@ -9,9 +9,13 @@ Required properties: - reg: * for "mrvl,pxav2-mmc" and "mrvl,pxav3-mmc", one register area for the SDHCI registers. - * for "marvell,armada-380-sdhci", two register areas. The first one - for the SDHCI registers themselves, and the second one for the - AXI/Mbus bridge registers of the SDHCI unit. + + * for "marvell,armada-380-sdhci", three register areas. The first + one for the SDHCI registers themselves, the second one for the + AXI/Mbus bridge registers of the SDHCI unit, the third one for the + SDIO3 Configuration register +- reg names: should be "sdhci", "mbus", "conf-sdio3". only mandatory + for "marvell,armada-380-sdhci" - clocks: Array of clocks required for SDHCI; requires at least one for I/O clock. - clock-names: Array of names corresponding to clocks property; shall be
@@ -35,7 +39,10 @@ sdhci at d4280800 { sdhci at d8000 { compatible = "marvell,armada-380-sdhci"; - reg = <0xd8000 0x1000>, <0xdc000 0x100>; + reg-names = "sdhci", "mbus", "conf-sdio3"; + reg = <0xd8000 0x1000>, + <0xdc000 0x100>; + <0x18454 0x4>; interrupts = <0 25 0x4>; clocks = <&gateclk 17>; clock-names = "io";
Signature · 1 line
2.1.0
PATCH v2 4/7 mmc: sdhci-pxav3: Modify clock settings for the SDR50 and DDR50 modes
In reply to Gregory CLEMENTFrom: Marcin Wojtas <[email protected]>
According to erratum 'FE-2946959' both SDR50 and DDR50 modes require specific clock adjustments in SDIO3 Configuration register.
This commit add the support of this register and for SDR50 or DDR50 mode use it as suggested by the erratum:
- Set the SDIO3 Clock Inv field in SDIO3 Configuration register to not inverted.
- Set the Sample FeedBack Clock field to 0x1
[gregory.clement at free-electrons.com: port from 3.10]
--- drivers/mmc/host/sdhci-pxav3.c | 60 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 8 deletions(-)
drivers/mmc/host/sdhci-pxav3.c +52 -8
@@ -62,6 +62,7 @@ struct sdhci_pxa {struct clk *clk_core;struct clk *clk_io;u8 power_mode; +void __iomem *sdio3_conf_reg; };/*
@@ -72,6 +73,14 @@ struct sdhci_pxa {#define SDHCI_WINDOW_BASE (i) (0x84 + ((i) << 3))#define SDHCI_MAX_WIN_NUM 8 +/* +* Fields below belong to SDIO3 Configuration Register (third register +* region for the Armada 38x flavor) +*/ + +#define SDIO3_CONF_CLK_INV BIT(0) +#define SDIO3_CONF_SD_FB_CLK BIT(2) +static int mv_conf_mbus_windows (struct platform_device *pdev,const struct mbus_dram_target_info *dram) {
@@ -122,16 +131,31 @@ static int armada_38x_quirks(struct platform_device *pdev, struct sdhci_host *host) {struct device_node *np = pdev->dev.of_node; +struct sdhci_pltfm_host *pltfm_host =sdhci_priv (host); +struct sdhci_pxa *pxa = pltfm_host->priv; +struct resource *res; host->quirks |=SDHCI_QUIRK_MISSING_CAPS ; -/* -* According to erratum 'FE-2946959' both SDR50 and DDR50 -* modes require specific clock adjustments in SDIO3 -* Configuration register, if the adjustment is not done, -* remove them from the capabilities. -*/ - host->caps1 =sdhci_readl (host,SDHCI_CAPABILITIES_1 ); - host->caps1 &= ~(SDHCI_SUPPORT_SDR50 |SDHCI_SUPPORT_DDR50 ); + res =platform_get_resource_byname (pdev,IORESOURCE_MEM , +"conf-sdio3" ); +if (res) { + pxa->sdio3_conf_reg =devm_ioremap_resource (&pdev->dev, res); +if (IS_ERR (pxa->sdio3_conf_reg)) +return PTR_ERR (pxa->sdio3_conf_reg); + }else { +/* +* According to erratum 'FE-2946959' both SDR50 and DDR50 +* modes require specific clock adjustments in SDIO3 +* Configuration register, if the adjustment is not done, +* remove them from the capabilities. +*/ + host->caps1 =sdhci_readl (host,SDHCI_CAPABILITIES_1 ); + host->caps1 &= ~(SDHCI_SUPPORT_SDR50 |SDHCI_SUPPORT_DDR50 ); + +dev_warn (&pdev->dev,"conf-sdio3 register not found\n" ); +dev_warn (&pdev->dev,"disabling SDR50 and DDR50 modes\n" ); +dev_warn (&pdev->dev,"consider updating your dtb\n" ); + }/* * According to erratum 'ERR-7878951' Armada 38x SDHCI
@@ -225,6 +249,8 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)static void pxav3_set_uhs_signaling (struct sdhci_host *host,unsigned int uhs) { +struct sdhci_pltfm_host *pltfm_host =sdhci_priv (host); +struct sdhci_pxa *pxa = pltfm_host->priv;u16 ctrl_2;/*
@@ -254,6 +280,24 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)break ; } +/* +* Update SDIO3 Configuration register according to erratum +* FE-2946959 +*/ +if (pxa->sdio3_conf_reg) { +u8 reg_val =readb (pxa->sdio3_conf_reg); + +if (uhs ==MMC_TIMING_UHS_SDR50 || + uhs ==MMC_TIMING_UHS_DDR50 ) { + reg_val &= ~SDIO3_CONF_CLK_INV ; + reg_val |=SDIO3_CONF_SD_FB_CLK ; + }else { + reg_val |=SDIO3_CONF_CLK_INV ; + reg_val &= ~SDIO3_CONF_SD_FB_CLK ; + } +writeb (reg_val, pxa->sdio3_conf_reg); + } +sdhci_writew (host, ctrl_2,SDHCI_HOST_CONTROL2 ); dev_dbg(mmc_dev(host->mmc), "%s uhs = %d, ctrl_2 = %04X\n",
Signature · 1 line
2.1.0
PATCH v2 5/7 ARM: mvebu: Use macros for interrupt flags on Armada 38x sdhci node
In reply to Gregory CLEMENTInstead of hardcoding the values of the interrupt flags, use the macros provided by <include/dt-bindings/interrupt-controller/irq.h> and <include/dt-bindings/interrupt-controller/arm-gic.h> for the Armada 38x SDHCI node.
--- arch/arm/boot/dts/armada-38x.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/boot/dts/armada-38x.dtsi +1 -1
@@ -516,7 +516,7 @@ sdhci at d8000 { compatible = "marvell,armada-380-sdhci"; reg = <0xd8000 0x1000>, <0xdc000 0x100>; - interrupts = <0 25 0x4>; + interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>; clocks = <&gateclk 17>; mrvl,clk-delay-cycles = <0x1F>; status = "disabled";
Signature · 1 line
2.1.0
PATCH v2 6/7 ARM: mvebu: Update the SDHCI node on Armada 38x
In reply to Gregory CLEMENTThe binding of the armada-380-sdhci has been extended with a new register in order to be able to use the SDR50 and DDR50 mode. This commit add the resource associated to this new register for the Armada 38x.
--- arch/arm/boot/dts/armada-38x.dtsi | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
arch/arm/boot/dts/armada-38x.dtsi +4 -1
@@ -515,7 +515,10 @@ sdhci at d8000 { compatible = "marvell,armada-380-sdhci"; - reg = <0xd8000 0x1000>, <0xdc000 0x100>; + reg-names = "sdhci", "mbus", "conf-sdio3"; + reg = <0xd8000 0x1000>, + <0xdc000 0x100>, + <0x18454 0x4>; interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>; clocks = <&gateclk 17>; mrvl,clk-delay-cycles = <0x1F>;
Signature · 1 line
2.1.0
PATCH v2 7/7 ARM: mvebu: Add Device Tree description of SDHCI for Armada 388 RD
In reply to Gregory CLEMENTThe Device Tree description of SDHCI on Armada 388 RD board was missing. This commit adds the node for it.
--- arch/arm/boot/dts/armada-388-rd.dts | 10 ++++++++++ 1 file changed, 10 insertions(+)
arch/arm/boot/dts/armada-388-rd.dts +10 -0
@@ -51,6 +51,16 @@ clock-frequency = <100000>; }; + sdhci at d8000 { + pinctrl-names = "default"; + pinctrl-0 = <&sdhci_pins>; + broken-cd; + no-1-8-v; + wp-inverted; + bus-width = <8>; + status = "okay"; + }; + serial at 12000 { status = "okay"; };
Signature · 1 line
2.1.0