Workaround errata i929
Subsystems (4)
v2 cover Workaround errata i929
The following patches workaround errata i929 by implementing a new tuning algorithm.
Changes in v2:
1. Added patch to document the name of the thermal_zone "cpu_thermal"
2. Rebased to latest linux-next
Faiz Abbas (2):
dt-bindings: sdhci-omap: Add note for cpu_thermal
mmc: sdhci-omap: Workaround errata regarding SDR104/HS200 tuning
failures (i929)
.../devicetree/bindings/mmc/sdhci-omap.txt | 2 + drivers/mmc/host/Kconfig | 2 + drivers/mmc/host/sdhci-omap.c | 90 ++++++++++++++++++- 3 files changed, 93 insertions(+), 1 deletion(-)
Signature · 1 line
2.19.2
PATCH v2 1/2 dt-bindings: sdhci-omap: Add note for cpu_thermal
The driver fetches a thermal zone using the string "cpu_thermal" for tuning operation. Add a note for the same.
--- Documentation/devicetree/bindings/mmc/sdhci-omap.txt | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/mmc/sdhci-omap.txt +2 -0
@@ -2,6 +2,8 @@ Refer to mmc.txt for standard MMC bindings. +For UHS devices which require tuning, the device tree should have a "cpu_thermal" node which maps to the appropriate thermal zone. This is used to get the temperature of the zone during tuning. + Required properties: - compatible: Should be "ti,dra7-sdhci" for DRA7 and DRA72 controllers Should be "ti,k2g-sdhci" for K2G
Signature · 1 line
2.19.2
Re: [PATCH v2 1/2] dt-bindings: sdhci-omap: Add note for cpu_thermal
Quoted 5 lines · Faiz Abbas
The driver fetches a thermal zone using the string "
cpu_thermal" for tuning operation. Add a note for the same.Signed-off-by Faiz Abbas <[email protected]>
Applied for next, thanks!
Kind regards
Uffe
Quoted 20 lines
--- Documentation/devicetree/bindings/mmc/sdhci-omap.txt | 2 ++ 1 file changed, 2 insertions(+)Documentation/devicetree/bindings/mmc/sdhci-omap.txt +2
-1@@ -2,6 +2,8 @@ Refer to mmc.txt for standard MMC bindings. +For UHS devices which require tuning, the device tree should have a "cpu_thermal" node which maps to the appropriate thermal zone. This is used to get the temperature of the zone during tuning. + Required properties: - compatible: Should be "ti,dra7-sdhci" for DRA7 and DRA72 controllers Should be "ti,k2g-sdhci" for K2G --2.19.2
PATCH v2 2/2 mmc: sdhci-omap: Workaround errata regarding SDR104/HS200 tuning failures (i929)
In reply to Faiz AbbasErrata i929 in certain OMAP5/DRA7XX/AM57XX silicon revisions (SPRZ426D - November 2014 - Revised February 2018 [1]) mentions unexpected tuning pattern errors. A small failure band may be present in the tuning range which may be missed by the current algorithm. Furthermore, the failure bands vary with temperature leading to different optimum tuning values for different temperatures.
As suggested in the related Application Report (SPRACA9B - October 2017
- Revised July 2018 [2]), tuning should be done in two stages. In stage 1, assign the optimum ratio in the maximum pass window for the current temperature. In stage 2, if the chosen value is close to the small failure band, move away from it in the appropriate direction.
References:
[1] http://www.ti.com/lit/pdf/sprz426
[2] http://www.ti.com/lit/pdf/SPRACA9
--- drivers/mmc/host/Kconfig | 2 + drivers/mmc/host/sdhci-omap.c | 90 ++++++++++++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 1 deletion(-)
drivers/mmc/host/Kconfig +2 -0
@@ -977,6 +977,8 @@ config MMC_SDHCI_XENON config MMC_SDHCI_OMAP tristate "TI SDHCI Controller Support" depends on MMC_SDHCI_PLTFM && OF + select THERMAL + select TI_SOC_THERMAL help This selects the Secure Digital Host Controller Interface (SDHCI) support present in TI's DRA7 SOCs. The controller supports
drivers/mmc/host/sdhci-omap.c +89 -1
145 lines not on this page · raw message
Signature · 1 line
2.19.2
Re: [PATCH v2 2/2] mmc: sdhci-omap: Workaround errata regarding SDR104/HS200 tuning failures (i929)
+ Thermal maintainers
Quoted 71 lines · Faiz Abbas
Errata i929 in certain OMAP5/DRA7XX/AM57XX silicon revisions (SPRZ426D - November 2014 - Revised February 2018 [1]) mentions unexpected tuning pattern errors. A small failure band may be present in the tuning range which may be missed by the current algorithm. Furthermore, the failure bands vary with temperature leading to different optimum tuning values for different temperatures.
As suggested in the related Application Report (SPRACA9B - October 2017
- Revised July 2018 [2]), tuning should be done in two stages. In stage 1, assign the optimum ratio in the maximum pass window for the current temperature. In stage 2, if the chosen value is close to the small failure band, move away from it in the appropriate direction.References:
[1] http://www.ti.com/lit/pdf/sprz426
[2] http://www.ti.com/lit/pdf/SPRACA9Signed-off-by Faiz Abbas <[email protected]>Acked-by Adrian Hunter <[email protected]>--- drivers/mmc/host/Kconfig | 2 + drivers/mmc/host/sdhci-omap.c | 90 ++++++++++++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 1 deletion(-)drivers/mmc/host/Kconfig +2
-0@@ -977,6 +977,8 @@ config MMC_SDHCI_XENON config MMC_SDHCI_OMAP tristate "TI SDHCI Controller Support" depends on MMC_SDHCI_PLTFM && OF + select THERMAL + select TI_SOC_THERMAL help This selects the Secure Digital Host Controller Interface (SDHCI) support present in TI's DRA7 SOCs. The controller supportsdrivers/mmc/host/sdhci-omap.c +5
-0@@ -27,6 +27,7 @@#include <linux/regulator/consumer.h> #include <linux/pinctrl/consumer.h> #include <linux/sys_soc.h> +#include <linux/thermal.h> #include "sdhci-pltfm.h" @@ -286,15 +287,19 @@ static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode)struct sdhci_host *host =mmc_priv (mmc);struct sdhci_pltfm_host *pltfm_host =sdhci_priv (host);struct sdhci_omap_host *omap_host =sdhci_pltfm_priv (pltfm_host); +struct thermal_zone_device *thermal_dev;struct device *dev = omap_host->dev;struct mmc_ios *ios = &mmc->ios;u32 start_window =0 , max_window =0 ; +bool single_point_failure = false;bool dcrc_was_enabled = false;u8 cur_match, prev_match =0 ;u32 length =0 , max_len =0 ;u32 phase_delay =0 ; +int temperature;int ret =0 ;u32 reg; +int i;/* clock tuning is not needed for upto 52MHz */ if (ios->clock <=52000000 )
drivers/mmc/host/sdhci-omap.c static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode) · line 312
Quoted 4 lines
drivers/mmc/host/sdhci-omap.c +1
-0@@ -304,6 +309,16 @@ static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode)if (ios->timing ==MMC_TIMING_UHS_SDR50 && !(reg &CAPA2_TSDR50 ))return 0 ; + thermal_dev =thermal_zone_get_zone_by_name ("cpu_thermal" );
I couldn't find a corresponding call to a put function, like "thermal_zone_put()" or whatever, which made me realize that the thermal zone API is incomplete. Or depending on how you put it, it lacks object reference counting, unless I am missing something.
For example, what happens if the thermal zone becomes unregistered between this point and when you call thermal_zone_get_temp() a couple of line below. I assume it's a known problem, but just wanted to point it out.
drivers/mmc/host/sdhci-omap.c static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode) · line 321
Quoted 9 lines
drivers/mmc/host/sdhci-omap.c +9
-0+if (IS_ERR (thermal_dev)) { +dev_err (dev,"Unable to get thermal zone for tuning\n" ); +return PTR_ERR (thermal_dev); + } + + ret =thermal_zone_get_temp (thermal_dev, &temperature); +if (ret) +return ret; +
[...]
Anyway, I have applied this for next, thanks!
Kind regards
Uffe
Re: [PATCH v2 2/2] mmc: sdhci-omap: Workaround errata regarding SDR104/HS200 tuning failures (i929)
Hi,
Quoted 102 lines · Ulf Hansson
+ Thermal maintainers
Quoted 71 lines · Faiz Abbas
Errata i929 in certain OMAP5/DRA7XX/AM57XX silicon revisions (SPRZ426D - November 2014 - Revised February 2018 [1]) mentions unexpected tuning pattern errors. A small failure band may be present in the tuning range which may be missed by the current algorithm. Furthermore, the failure bands vary with temperature leading to different optimum tuning values for different temperatures.
As suggested in the related Application Report (SPRACA9B - October 2017
- Revised July 2018 [2]), tuning should be done in two stages. In stage 1, assign the optimum ratio in the maximum pass window for the current temperature. In stage 2, if the chosen value is close to the small failure band, move away from it in the appropriate direction.References:
[1] http://www.ti.com/lit/pdf/sprz426
[2] http://www.ti.com/lit/pdf/SPRACA9Signed-off-by Faiz Abbas <[email protected]>Acked-by Adrian Hunter <[email protected]>--- drivers/mmc/host/Kconfig | 2 + drivers/mmc/host/sdhci-omap.c | 90 ++++++++++++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 1 deletion(-)drivers/mmc/host/Kconfig +2
-0@@ -977,6 +977,8 @@ config MMC_SDHCI_XENON config MMC_SDHCI_OMAP tristate "TI SDHCI Controller Support" depends on MMC_SDHCI_PLTFM && OF + select THERMAL + select TI_SOC_THERMAL help This selects the Secure Digital Host Controller Interface (SDHCI) support present in TI's DRA7 SOCs. The controller supportsdrivers/mmc/host/sdhci-omap.c +5
-0@@ -27,6 +27,7 @@#include <linux/regulator/consumer.h> #include <linux/pinctrl/consumer.h> #include <linux/sys_soc.h> +#include <linux/thermal.h> #include "sdhci-pltfm.h" @@ -286,15 +287,19 @@ static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode)struct sdhci_host *host =mmc_priv (mmc);struct sdhci_pltfm_host *pltfm_host =sdhci_priv (host);struct sdhci_omap_host *omap_host =sdhci_pltfm_priv (pltfm_host); +struct thermal_zone_device *thermal_dev;struct device *dev = omap_host->dev;struct mmc_ios *ios = &mmc->ios;u32 start_window =0 , max_window =0 ; +bool single_point_failure = false;bool dcrc_was_enabled = false;u8 cur_match, prev_match =0 ;u32 length =0 , max_len =0 ;u32 phase_delay =0 ; +int temperature;int ret =0 ;u32 reg; +int i;/* clock tuning is not needed for upto 52MHz */ if (ios->clock <=52000000 )drivers/mmc/host/sdhci-omap.c static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode) · line 312
Quoted 4 lines
drivers/mmc/host/sdhci-omap.c +1
-0@@ -304,6 +309,16 @@ static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode)if (ios->timing ==MMC_TIMING_UHS_SDR50 && !(reg &CAPA2_TSDR50 ))return 0 ; + thermal_dev =thermal_zone_get_zone_by_name ("cpu_thermal" );I couldn't find a corresponding call to a put function, like "
thermal_zone_put()" or whatever, which made me realize that the thermal zone API is incomplete. Or depending on how you put it, it lacks object reference counting, unless I am missing something.For example, what happens if the thermal zone becomes unregistered between this point and when you call
thermal_zone_get_temp()a couple of line below. I assume it's a known problem, but just wanted to point it out.Quoted 9 lines
drivers/mmc/host/sdhci-omap.c +9
-0+if (IS_ERR (thermal_dev)) { +dev_err (dev,"Unable to get thermal zone for tuning\n" ); +return PTR_ERR (thermal_dev); + } + + ret =thermal_zone_get_temp (thermal_dev, &temperature); +if (ret) +return ret; +[...]
Quoted 1 line
Anyway, I have applied this for next, thanks!
This is throwing errors on builds of keystone_defconfig in next and mainline:
http://arm-soc.lixom.net/buildlogs/next/next-20190102/buildall.arm.keystone_defconfig.log.passed
WARNING: unmet direct dependencies detected for TI_SOC_THERMAL
Depends on [n]: THERMAL [=y] && (ARCH_HAS_BANDGAP [=n] ||COMPILE_TEST [=n]) && HAS_IOMEM [=y]
Selected by [y]:
- MMC_SDHCI_OMAP [=y] && MMC [=y] && MMC_SDHCI_PLTFM [=y] && OF [=y]
So, thermal depends on ARCH_HAS_BANDGAP, which keystone doesn't provide.
Selecting a major framework such as THERMAL from a driver config is likely not the right solution anyway, especially since THERMAL does provide stubbed out versions of the functions if it's not enabled.
-Olof
Re: [PATCH v2 2/2] mmc: sdhci-omap: Workaround errata regarding SDR104/HS200 tuning failures (i929)
Quoted 94 lines · Olof Johansson
Hi,
Quoted 90 lines · Ulf Hansson
+ Thermal maintainers
Quoted 71 lines · Faiz Abbas
Errata i929 in certain OMAP5/DRA7XX/AM57XX silicon revisions (SPRZ426D - November 2014 - Revised February 2018 [1]) mentions unexpected tuning pattern errors. A small failure band may be present in the tuning range which may be missed by the current algorithm. Furthermore, the failure bands vary with temperature leading to different optimum tuning values for different temperatures.
As suggested in the related Application Report (SPRACA9B - October 2017
- Revised July 2018 [2]), tuning should be done in two stages. In stage 1, assign the optimum ratio in the maximum pass window for the current temperature. In stage 2, if the chosen value is close to the small failure band, move away from it in the appropriate direction.References:
[1] http://www.ti.com/lit/pdf/sprz426
[2] http://www.ti.com/lit/pdf/SPRACA9Signed-off-by Faiz Abbas <[email protected]>Acked-by Adrian Hunter <[email protected]>--- drivers/mmc/host/Kconfig | 2 + drivers/mmc/host/sdhci-omap.c | 90 ++++++++++++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 1 deletion(-)drivers/mmc/host/Kconfig +2
-0@@ -977,6 +977,8 @@ config MMC_SDHCI_XENON config MMC_SDHCI_OMAP tristate "TI SDHCI Controller Support" depends on MMC_SDHCI_PLTFM && OF + select THERMAL + select TI_SOC_THERMAL help This selects the Secure Digital Host Controller Interface (SDHCI) support present in TI's DRA7 SOCs. The controller supportsdrivers/mmc/host/sdhci-omap.c +5
-0@@ -27,6 +27,7 @@#include <linux/regulator/consumer.h> #include <linux/pinctrl/consumer.h> #include <linux/sys_soc.h> +#include <linux/thermal.h> #include "sdhci-pltfm.h" @@ -286,15 +287,19 @@ static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode)struct sdhci_host *host =mmc_priv (mmc);struct sdhci_pltfm_host *pltfm_host =sdhci_priv (host);struct sdhci_omap_host *omap_host =sdhci_pltfm_priv (pltfm_host); +struct thermal_zone_device *thermal_dev;struct device *dev = omap_host->dev;struct mmc_ios *ios = &mmc->ios;u32 start_window =0 , max_window =0 ; +bool single_point_failure = false;bool dcrc_was_enabled = false;u8 cur_match, prev_match =0 ;u32 length =0 , max_len =0 ;u32 phase_delay =0 ; +int temperature;int ret =0 ;u32 reg; +int i;/* clock tuning is not needed for upto 52MHz */ if (ios->clock <=52000000 )drivers/mmc/host/sdhci-omap.c static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode) · line 312
Quoted 4 lines
drivers/mmc/host/sdhci-omap.c +1
-0@@ -304,6 +309,16 @@ static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode)if (ios->timing ==MMC_TIMING_UHS_SDR50 && !(reg &CAPA2_TSDR50 ))return 0 ; + thermal_dev =thermal_zone_get_zone_by_name ("cpu_thermal" );I couldn't find a corresponding call to a put function, like "
thermal_zone_put()" or whatever, which made me realize that the thermal zone API is incomplete. Or depending on how you put it, it lacks object reference counting, unless I am missing something.For example, what happens if the thermal zone becomes unregistered between this point and when you call
thermal_zone_get_temp()a couple of line below. I assume it's a known problem, but just wanted to point it out.
Yes, there is no ref counting. Specially because the get zones usages were too specific, and mostly used in application cases that module would not really be removed. Though not a good excuse, still, not very problematic. Now, if the API is getting other usages, then refcounting may be necessary.
Quoted 24 lines
Quoted 12 lines
Quoted 9 lines
drivers/mmc/host/sdhci-omap.c +9
-0+if (IS_ERR (thermal_dev)) { +dev_err (dev,"Unable to get thermal zone for tuning\n" ); +return PTR_ERR (thermal_dev); + } + + ret =thermal_zone_get_temp (thermal_dev, &temperature); +if (ret) +return ret; +[...]
Quoted 1 line
Anyway, I have applied this for next, thanks!
This is throwing errors on builds of
keystone_defconfigin next and mainline:http://arm-soc.lixom.net/buildlogs/next/next-20190102/buildall.arm.keystone_defconfig.log.passed
WARNING: unmet direct dependencies detected for
TI_SOC_THERMAL
Depends on [n]: THERMAL [=y] && (ARCH_HAS_BANDGAP[=n] ||COMPILE_TEST[=n]) &&HAS_IOMEM[=y]
Selected by [y]:
-MMC_SDHCI_OMAP[=y] && MMC [=y] &&MMC_SDHCI_PLTFM[=y] && OF [=y]
Quoted 5 lines
So, thermal depends on
ARCH_HAS_BANDGAP, which keystone doesn't provide.Selecting a major framework such as THERMAL from a driver config is likely not the right solution anyway, especially since THERMAL does provide stubbed out versions of the functions if it's not enabled.
Yeah, that seams a bit up-side-down. Can you guys give a bit more of context? Why do you need the cpu thermal zone ? From patch description, looks like you want to have your own zone then apply different tuning values based on temperature (range?). Why do you need to mess up with cpu_thermal zone? Don't you have a bandgap in the mem controller for this application?
Quoted 3 lines
-Olof
Re: [PATCH v2 2/2] mmc: sdhci-omap: Workaround errata regarding SDR104/HS200 tuning failures (i929)
Hi Olof, Eduardo,
Quoted 132 lines · Eduardo Valentin
Quoted 94 lines · Olof Johansson
Hi,
Quoted 90 lines · Ulf Hansson
+ Thermal maintainers
Quoted 71 lines · Faiz Abbas
Errata i929 in certain OMAP5/DRA7XX/AM57XX silicon revisions (SPRZ426D - November 2014 - Revised February 2018 [1]) mentions unexpected tuning pattern errors. A small failure band may be present in the tuning range which may be missed by the current algorithm. Furthermore, the failure bands vary with temperature leading to different optimum tuning values for different temperatures.
As suggested in the related Application Report (SPRACA9B - October 2017
- Revised July 2018 [2]), tuning should be done in two stages. In stage 1, assign the optimum ratio in the maximum pass window for the current temperature. In stage 2, if the chosen value is close to the small failure band, move away from it in the appropriate direction.References:
[1] http://www.ti.com/lit/pdf/sprz426
[2] http://www.ti.com/lit/pdf/SPRACA9Signed-off-by Faiz Abbas <[email protected]>Acked-by Adrian Hunter <[email protected]>--- drivers/mmc/host/Kconfig | 2 + drivers/mmc/host/sdhci-omap.c | 90 ++++++++++++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 1 deletion(-)drivers/mmc/host/Kconfig +2
-0@@ -977,6 +977,8 @@ config MMC_SDHCI_XENON config MMC_SDHCI_OMAP tristate "TI SDHCI Controller Support" depends on MMC_SDHCI_PLTFM && OF + select THERMAL + select TI_SOC_THERMAL help This selects the Secure Digital Host Controller Interface (SDHCI) support present in TI's DRA7 SOCs. The controller supportsdrivers/mmc/host/sdhci-omap.c +5
-0@@ -27,6 +27,7 @@#include <linux/regulator/consumer.h> #include <linux/pinctrl/consumer.h> #include <linux/sys_soc.h> +#include <linux/thermal.h> #include "sdhci-pltfm.h" @@ -286,15 +287,19 @@ static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode)struct sdhci_host *host =mmc_priv (mmc);struct sdhci_pltfm_host *pltfm_host =sdhci_priv (host);struct sdhci_omap_host *omap_host =sdhci_pltfm_priv (pltfm_host); +struct thermal_zone_device *thermal_dev;struct device *dev = omap_host->dev;struct mmc_ios *ios = &mmc->ios;u32 start_window =0 , max_window =0 ; +bool single_point_failure = false;bool dcrc_was_enabled = false;u8 cur_match, prev_match =0 ;u32 length =0 , max_len =0 ;u32 phase_delay =0 ; +int temperature;int ret =0 ;u32 reg; +int i;/* clock tuning is not needed for upto 52MHz */ if (ios->clock <=52000000 )drivers/mmc/host/sdhci-omap.c static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode) · line 312
Quoted 4 lines
drivers/mmc/host/sdhci-omap.c +1
-0@@ -304,6 +309,16 @@ static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode)if (ios->timing ==MMC_TIMING_UHS_SDR50 && !(reg &CAPA2_TSDR50 ))return 0 ; + thermal_dev =thermal_zone_get_zone_by_name ("cpu_thermal" );I couldn't find a corresponding call to a put function, like "
thermal_zone_put()" or whatever, which made me realize that the thermal zone API is incomplete. Or depending on how you put it, it lacks object reference counting, unless I am missing something.For example, what happens if the thermal zone becomes unregistered between this point and when you call
thermal_zone_get_temp()a couple of line below. I assume it's a known problem, but just wanted to point it out.Yes, there is no ref counting. Specially because the get zones usages were too specific, and mostly used in application cases that module would not really be removed. Though not a good excuse, still, not very problematic. Now, if the API is getting other usages, then refcounting may be necessary.
Quoted 24 lines
Quoted 12 lines
Quoted 9 lines
drivers/mmc/host/sdhci-omap.c +9
-0+if (IS_ERR (thermal_dev)) { +dev_err (dev,"Unable to get thermal zone for tuning\n" ); +return PTR_ERR (thermal_dev); + } + + ret =thermal_zone_get_temp (thermal_dev, &temperature); +if (ret) +return ret; +[...]
Quoted 1 line
Anyway, I have applied this for next, thanks!
This is throwing errors on builds of
keystone_defconfigin next and mainline:http://arm-soc.lixom.net/buildlogs/next/next-20190102/buildall.arm.keystone_defconfig.log.passed
WARNING: unmet direct dependencies detected for
TI_SOC_THERMAL
Depends on [n]: THERMAL [=y] && (ARCH_HAS_BANDGAP[=n] ||COMPILE_TEST[=n]) &&HAS_IOMEM[=y]
Selected by [y]:
-MMC_SDHCI_OMAP[=y] && MMC [=y] &&MMC_SDHCI_PLTFM[=y] && OF [=y]Quoted 5 lines
So, thermal depends on
ARCH_HAS_BANDGAP, which keystone doesn't provide.Selecting a major framework such as THERMAL from a driver config is likely not the right solution anyway, especially since THERMAL does provide stubbed out versions of the functions if it's not enabled.
Quoted 7 lines
Yeah, that seams a bit up-side-down. Can you guys give a bit more of context? Why do you need the cpu thermal zone ? From patch description, looks like you want to have your own zone then apply different tuning values based on temperature (range?). Why do you need to mess up with
cpu_thermalzone? Don't you have a bandgap in the mem controller for this application?
Thats correct. We don't have a bandgap in the MMC controller and thus we have to use the cpu one to measure temperature.
THERMAL is critical for tuning. The interface is supposed to fail if we can't get temperature. So IMO we should ensure that it is present.
I can fix this by "select TI_SOC_THERMAL if ARCH_HAS_BANDGAP" if you guys agree.
Thanks,
Faiz
Re: [PATCH v2 2/2] mmc: sdhci-omap: Workaround errata regarding SDR104/HS200 tuning failures (i929)
Quoted 144 lines · Faiz Abbas
Hi Olof, Eduardo,
Quoted 132 lines · Eduardo Valentin
Quoted 94 lines · Olof Johansson
Hi,
Quoted 90 lines · Ulf Hansson
+ Thermal maintainers
Quoted 71 lines · Faiz Abbas
Errata i929 in certain OMAP5/DRA7XX/AM57XX silicon revisions (SPRZ426D - November 2014 - Revised February 2018 [1]) mentions unexpected tuning pattern errors. A small failure band may be present in the tuning range which may be missed by the current algorithm. Furthermore, the failure bands vary with temperature leading to different optimum tuning values for different temperatures.
As suggested in the related Application Report (SPRACA9B - October 2017
- Revised July 2018 [2]), tuning should be done in two stages. In stage 1, assign the optimum ratio in the maximum pass window for the current temperature. In stage 2, if the chosen value is close to the small failure band, move away from it in the appropriate direction.References:
[1] http://www.ti.com/lit/pdf/sprz426
[2] http://www.ti.com/lit/pdf/SPRACA9Signed-off-by Faiz Abbas <[email protected]>Acked-by Adrian Hunter <[email protected]>--- drivers/mmc/host/Kconfig | 2 + drivers/mmc/host/sdhci-omap.c | 90 ++++++++++++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 1 deletion(-)drivers/mmc/host/Kconfig +2
-0@@ -977,6 +977,8 @@ config MMC_SDHCI_XENON config MMC_SDHCI_OMAP tristate "TI SDHCI Controller Support" depends on MMC_SDHCI_PLTFM && OF + select THERMAL + select TI_SOC_THERMAL help This selects the Secure Digital Host Controller Interface (SDHCI) support present in TI's DRA7 SOCs. The controller supportsdrivers/mmc/host/sdhci-omap.c +5
-0@@ -27,6 +27,7 @@#include <linux/regulator/consumer.h> #include <linux/pinctrl/consumer.h> #include <linux/sys_soc.h> +#include <linux/thermal.h> #include "sdhci-pltfm.h" @@ -286,15 +287,19 @@ static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode)struct sdhci_host *host =mmc_priv (mmc);struct sdhci_pltfm_host *pltfm_host =sdhci_priv (host);struct sdhci_omap_host *omap_host =sdhci_pltfm_priv (pltfm_host); +struct thermal_zone_device *thermal_dev;struct device *dev = omap_host->dev;struct mmc_ios *ios = &mmc->ios;u32 start_window =0 , max_window =0 ; +bool single_point_failure = false;bool dcrc_was_enabled = false;u8 cur_match, prev_match =0 ;u32 length =0 , max_len =0 ;u32 phase_delay =0 ; +int temperature;int ret =0 ;u32 reg; +int i;/* clock tuning is not needed for upto 52MHz */ if (ios->clock <=52000000 )drivers/mmc/host/sdhci-omap.c static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode) · line 312
Quoted 4 lines
drivers/mmc/host/sdhci-omap.c +1
-0@@ -304,6 +309,16 @@ static int sdhci_omap_execute_tuning(struct mmc_host *mmc, u32 opcode)if (ios->timing ==MMC_TIMING_UHS_SDR50 && !(reg &CAPA2_TSDR50 ))return 0 ; + thermal_dev =thermal_zone_get_zone_by_name ("cpu_thermal" );I couldn't find a corresponding call to a put function, like "
thermal_zone_put()" or whatever, which made me realize that the thermal zone API is incomplete. Or depending on how you put it, it lacks object reference counting, unless I am missing something.For example, what happens if the thermal zone becomes unregistered between this point and when you call
thermal_zone_get_temp()a couple of line below. I assume it's a known problem, but just wanted to point it out.Yes, there is no ref counting. Specially because the get zones usages were too specific, and mostly used in application cases that module would not really be removed. Though not a good excuse, still, not very problematic. Now, if the API is getting other usages, then refcounting may be necessary.
Quoted 24 lines
Quoted 12 lines
Quoted 9 lines
drivers/mmc/host/sdhci-omap.c +9
-0+if (IS_ERR (thermal_dev)) { +dev_err (dev,"Unable to get thermal zone for tuning\n" ); +return PTR_ERR (thermal_dev); + } + + ret =thermal_zone_get_temp (thermal_dev, &temperature); +if (ret) +return ret; +[...]
Quoted 1 line
Anyway, I have applied this for next, thanks!
This is throwing errors on builds of
keystone_defconfigin next and mainline:http://arm-soc.lixom.net/buildlogs/next/next-20190102/buildall.arm.keystone_defconfig.log.passed
WARNING: unmet direct dependencies detected for
TI_SOC_THERMAL
Depends on [n]: THERMAL [=y] && (ARCH_HAS_BANDGAP[=n] ||COMPILE_TEST[=n]) &&HAS_IOMEM[=y]
Selected by [y]:
-MMC_SDHCI_OMAP[=y] && MMC [=y] &&MMC_SDHCI_PLTFM[=y] && OF [=y]Quoted 5 lines
So, thermal depends on
ARCH_HAS_BANDGAP, which keystone doesn't provide.Selecting a major framework such as THERMAL from a driver config is likely not the right solution anyway, especially since THERMAL does provide stubbed out versions of the functions if it's not enabled.
Quoted 7 lines
Yeah, that seams a bit up-side-down. Can you guys give a bit more of context? Why do you need the cpu thermal zone ? From patch description, looks like you want to have your own zone then apply different tuning values based on temperature (range?). Why do you need to mess up with
cpu_thermalzone? Don't you have a bandgap in the mem controller for this application?
Quoted 8 lines
Thats correct. We don't have a bandgap in the MMC controller and thus we have to use the cpu one to measure temperature.
THERMAL is critical for tuning. The interface is supposed to fail if we can't get temperature. So IMO we should ensure that it is present.
I can fix this by "select
TI_SOC_THERMALifARCH_HAS_BANDGAP" if you guys agree.
Building elaborate select statements is usually fragile, once dependencies for TI_SOC_THERMAL changes you need to come back here to fixup the select.
Supposedly this driver works on keystone (or does it?), it doesn't actually need TI_SOC_THERMAL for basic functionality beyond tuning? Or, at least, it needs to fall back to a reasonable behavior if it's unavailable on keystone.
Having the driver print a warning and refuse to tune to higher speeds is a reasonable way to do this, I think. That would carry to all platforms, i.e. even the ones who have TI_SOC_THERMAL and ARCH_HAS_BANDGAP, without adding the select.
-Olof
Re: [PATCH v2 2/2] mmc: sdhci-omap: Workaround errata regarding SDR104/HS200 tuning failures (i929)
Hi Olof,
Quoted 9 lines · Olof Johansson
Quoted 8 lines · Faiz Abbas
Hi Olof, Eduardo,
Quoted 4 lines · Eduardo Valentin
Quoted 3 lines · Olof Johansson
Hi,
...
Quoted 31 lines
Quoted 31 lines
Quoted 17 lines
Quoted 11 lines
This is throwing errors on builds of
keystone_defconfigin next and mainline:http://arm-soc.lixom.net/buildlogs/next/next-20190102/buildall.arm.keystone_defconfig.log.passed
WARNING: unmet direct dependencies detected for
TI_SOC_THERMAL
Depends on [n]: THERMAL [=y] && (ARCH_HAS_BANDGAP[=n] ||COMPILE_TEST[=n]) &&HAS_IOMEM[=y]
Selected by [y]:
-MMC_SDHCI_OMAP[=y] && MMC [=y] &&MMC_SDHCI_PLTFM[=y] && OF [=y]Quoted 5 lines
So, thermal depends on
ARCH_HAS_BANDGAP, which keystone doesn't provide.Selecting a major framework such as THERMAL from a driver config is likely not the right solution anyway, especially since THERMAL does provide stubbed out versions of the functions if it's not enabled.
Quoted 7 lines
Yeah, that seams a bit up-side-down. Can you guys give a bit more of context? Why do you need the cpu thermal zone ? From patch description, looks like you want to have your own zone then apply different tuning values based on temperature (range?). Why do you need to mess up with
cpu_thermalzone? Don't you have a bandgap in the mem controller for this application?Thats correct. We don't have a bandgap in the MMC controller and thus we have to use the cpu one to measure temperature.
THERMAL is critical for tuning. The interface is supposed to fail if we can't get temperature. So IMO we should ensure that it is present.
Quoted 8 lines
Quoted 2 lines
I can fix this by "select
TI_SOC_THERMALifARCH_HAS_BANDGAP" if you guys agree.Building elaborate select statements is usually fragile, once dependencies for
TI_SOC_THERMALchanges you need to come back here to fixup the select.Supposedly this driver works on keystone (or does it?), it doesn't
Yes. This driver works on keystone.
Quoted 3 lines
actually need
TI_SOC_THERMALfor basic functionality beyond tuning? Or, at least, it needs to fall back to a reasonable behavior if it's unavailable on keystone.
The scenario is this. dra7 devices (omap2plus_defconfig) support tuning which needs THERMAL and TI_SOC_THERMAL. Keystone devices don't need thermal at all. The tuning part of the code will never be touched by the keystone device.
In omap2plus_defconfig, THERMAL AND TI_SOC_THERMAL are modules by default. I need them to be built-in because MMC is built-in.
Quoted 6 lines
Having the driver print a warning and refuse to tune to higher speeds is a reasonable way to do this, I think. That would carry to all platforms, i.e. even the ones who have
TI_SOC_THERMALandARCH_HAS_BANDGAP, without adding the select.
The MMC core today doesn't support a fallback to lower speeds when tuning fails. The interface just fails. This means a bunch cards that were working will now fail. Also, people building with olddefconfig will get build errors because of THERMAL=m. Thus the Kconfig architecture should automatically select the dependencies for SDHCI_OMAP in DRA7.
An "imply TI_SOC_THERMAL" is even better here. It won't force TI_SOC_THERMAL for keystone and doesn't cause any warnings.
So I propose the following patch:
drivers/mmc/host/Kconfig +1 -2
@@ -978,7 +978,7 @@ config MMC_SDHCI_OMAP tristate "TI SDHCI Controller Support" depends on MMC_SDHCI_PLTFM && OF select THERMAL - select TI_SOC_THERMAL + imply TI_SOC_THERMAL help --
Thanks,
Faiz