mmc: dw_mmc-rockchip: allow tuning using the clk-phase api
Subsystems (5)
Heiko Stuebner
maintainerv2 cover mmc: dw_mmc-rockchip: allow tuning using the clk-phase api
This series resurrects and adapts some individual patches whose sum enable the dw_mmc hosts on Rockchip socs to tune clock phases using the generic phase api (rk3288 and following have this capability).
The changes to the original mmc-phase clocks are expanded by further findings resulting from devices being used in the field.
Similarly the regulator handling changes do use the brand new regulator_set_voltage_triplet call to allow specifying lower and upper limits. One possible point of discussion are the two voltage ranges that are tried for the 3.3V signal level. Trying to stay near vmmc at first and only then opening the range to the full 2.7-3.6V.
This mainly circumvents a shortcoming of the regulator voltage setting, in that even with regulator_set_voltage_triplet the regulator framework will take the lowest possible voltage when the possible voltages are below the target voltage. While it may be ideal to solve this on the regulator side, I'm not seeing this appearing in the short term, mainly because all regulator parts (including regulator drivers) are keyed to selecting the lowest voltage from the range, while on the mmc side we know which voltages may work and trying this in two steps does not create to much overhead, as unsupported voltages are already filtered out by the regulator_is_voltage_selected calls.
changes since v1:
- address comment from Jaehoon Chung and keep this local to Rockchip for the time being
- address comments from Ulf and Doug in making it more explicit that the two-step 3.3V voltage setting essentially works around a limitation of regulator_set_voltage_triplet
Alexandru M Stan (3):
mmc: dw_mmc-rockchip: dt-binding: Add tuning related things
mmc: dw_mmc: Generic MMC tuning with the clock phase framework
ARM: dts: rockchip: Add drive/sample clocks for rk3288 dw_mmc devices
Douglas Anderson (4):
clk: rockchip: Allow more precision for some mmc clock phases
clk: rockchip: Make calculations use rounding
mmc: core: Add mmc_regulator_set_vqmmc()
mmc: dw_mmc: Use mmc_regulator_set_vqmmc in
start_signal_voltage_switch
Heiko Stuebner (1):
ARM: dts: rockchip: add tuning related settings to veyron devices
.../devicetree/bindings/mmc/rockchip-dw-mshc.txt | 13 ++ arch/arm/boot/dts/rk3288-veyron-sdmmc.dtsi | 7 +- arch/arm/boot/dts/rk3288-veyron.dtsi | 6 + arch/arm/boot/dts/rk3288.dtsi | 20 ++- drivers/clk/rockchip/clk-mmc-phase.c | 54 ++++--- drivers/mmc/core/core.c | 74 ++++++++++ drivers/mmc/host/dw_mmc-rockchip.c | 162 +++++++++++++++++++++ drivers/mmc/host/dw_mmc.c | 17 +-- include/linux/mmc/host.h | 7 + 9 files changed, 321 insertions(+), 39 deletions(-)
Signature · 1 line
2.5.1
Heiko Stuebner
maintainerPATCH v2 1/8 clk: rockchip: Allow more precision for some mmc clock phases
From: Douglas Anderson <[email protected]>
Because of the inexact nature of the extra MMC delay elements (it's not possible to keep the phase monotonic and to also make phases (mod
90) > 70), we previously only allowed phases (mod 90) of 22.5, 45, and 67.5.
But it's not the end of the world if the MMC clock phase goes non-monotonic. At most we'll be 25 degrees off. It's way better to test more phases to look for bad ones than to be 25 degrees off, because in the case of MMC really the point is to find bad phases and get as far asway from the as possible. If we get to test extra phases by going slightly non-monotonic then that might be fine. Worst case we would end up at a phases that's slight differnt than the one we wanted, but at least we'd still be quite far away from the a bad phase.
Fold in more precise variance-values of 44-77 instead of 40-80. Fold in the actual removal of the monotonic requirement and adapt patch message accordingly.
--- drivers/clk/rockchip/clk-mmc-phase.c | 45 ++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 15 deletions(-)
drivers/clk/rockchip/clk-mmc-phase.c +30 -15
@@ -45,8 +45,8 @@ static unsigned long rockchip_mmc_recalc(struct clk_hw *hw,#define PSECS_PER_SEC 1000000000000LL/* -* Each fine delay is between 40ps-80ps. Assume each fine delay is 60ps to -* simplify calculations. So 45degs could be anywhere between 33deg and 66deg. +* Each fine delay is between 44ps-77ps. Assume each fine delay is 60ps to +* simplify calculations. So 45degs could be anywhere between 33deg and 57.8deg. */ #define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC 60
@@ -84,22 +84,37 @@ static int rockchip_mmc_set_phase(struct clk_hw *hw, int degrees)u32 raw_value;u64 delay; -/* allow 22 to be 22.5 */ - degrees++; -/* floor to 22.5 increment */ - degrees -= ((degrees) *10 %225 ) /10 ; - nineties = degrees /90 ; -/* 22.5 multiples */ - remainder = (degrees %90 ) /22 ; - + remainder = (degrees %90 ); + +/* +* Due to the inexact nature of the "fine" delay, we might +* actually go non-monotonic. We don't go _too_ monotonic +* though, so we should be OK. Here are options of how we may +* work: +* +* Ideally we end up with: +* 1.0, 2.0, ..., 69.0, 70.0, ..., 89.0, 90.0 +* +* On one extreme (if delay is actually 44ps): +* .73, 1.5, ..., 50.6, 51.3, ..., 65.3, 90.0 +* The other (if delay is actually 77ps): +* 1.3, 2.6, ..., 88.6. 89.8, ..., 114.0, 90 +* +* It's possible we might make a delay that is up to 25 +* degrees off from what we think we're making. That's OK +* though because we should be REALLY far from any bad range. +*/ + +/* +* Convert to delay; do a little extra work to make sure we +* don't overflow 32-bit / 64-bit numbers. +*/ delay =PSECS_PER_SEC ; -do_div (delay, rate); -/* / 360 / 22.5 */ -do_div (delay,16 ); -do_div (delay,ROCKCHIP_MMC_DELAY_ELEMENT_PSEC ); - delay *= remainder; +do_div (delay,10000 ); +do_div (delay, (rate /1000 ) *36 *ROCKCHIP_MMC_DELAY_ELEMENT_PSEC ); + delay_num = (u8 )min (delay,255ULL ); raw_value = delay_num ?ROCKCHIP_MMC_DELAY_SEL :0 ;
Signature · 1 line
2.5.1
PATCH v2 1/8 clk: rockchip: Allow more precision for some mmc clock phases
Quoting Heiko Stuebner (2015-09-30 07:07:37)
Quoted 16 lines
From: Douglas Anderson <[email protected]>
Because of the inexact nature of the extra MMC delay elements (it's not possible to keep the phase monotonic and to also make phases (mod
90) > 70), we previously only allowed phases (mod 90) of 22.5, 45, and 67.5.But it's not the end of the world if the MMC clock phase goes non-monotonic. At most we'll be 25 degrees off. It's way better to test more phases to look for bad ones than to be 25 degrees off, because in the case of MMC really the point is to find bad phases and get as far asway from the as possible. If we get to test extra phases by going slightly non-monotonic then that might be fine. Worst case we would end up at a phases that's slight differnt than the one we wanted, but at least we'd still be quite far away from the a bad phase.
Quoted 6 lines
Signed-off-by Douglas Anderson <[email protected]>Fold in more precise variance-values of 44-77 instead of 40-80. Fold in the actual removal of the monotonic requirement and adapt patch message accordingly.
Signed-off-by Heiko Stuebner <[email protected]>
Looks good to me. What tree do you want this to go through?
Regards,
Mike
Quoted 73 lines
--- drivers/clk/rockchip/clk-mmc-phase.c | 45 ++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 15 deletions(-)drivers/clk/rockchip/clk-mmc-phase.c +30
-15@@ -45,8 +45,8 @@ static unsigned long rockchip_mmc_recalc(struct clk_hw *hw,#define PSECS_PER_SEC 1000000000000LL/* -* Each fine delay is between 40ps-80ps. Assume each fine delay is 60ps to -* simplify calculations. So 45degs could be anywhere between 33deg and 66deg. +* Each fine delay is between 44ps-77ps. Assume each fine delay is 60ps to +* simplify calculations. So 45degs could be anywhere between 33deg and 57.8deg. */ #define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC 60@@ -84,22 +84,37 @@ static int rockchip_mmc_set_phase(struct clk_hw *hw, int degrees)u32 raw_value;u64 delay; -/* allow 22 to be 22.5 */ - degrees++; -/* floor to 22.5 increment */ - degrees -= ((degrees) *10 %225 ) /10 ; - nineties = degrees /90 ; -/* 22.5 multiples */ - remainder = (degrees %90 ) /22 ; - + remainder = (degrees %90 ); + +/* +* Due to the inexact nature of the "fine" delay, we might +* actually go non-monotonic. We don't go _too_ monotonic +* though, so we should be OK. Here are options of how we may +* work: +* +* Ideally we end up with: +* 1.0, 2.0, ..., 69.0, 70.0, ..., 89.0, 90.0 +* +* On one extreme (if delay is actually 44ps): +* .73, 1.5, ..., 50.6, 51.3, ..., 65.3, 90.0 +* The other (if delay is actually 77ps): +* 1.3, 2.6, ..., 88.6. 89.8, ..., 114.0, 90 +* +* It's possible we might make a delay that is up to 25 +* degrees off from what we think we're making. That's OK +* though because we should be REALLY far from any bad range. +*/ + +/* +* Convert to delay; do a little extra work to make sure we +* don't overflow 32-bit / 64-bit numbers. +*/ delay =PSECS_PER_SEC ; -do_div (delay, rate); -/* / 360 / 22.5 */ -do_div (delay,16 ); -do_div (delay,ROCKCHIP_MMC_DELAY_ELEMENT_PSEC ); - delay *= remainder; +do_div (delay,10000 ); +do_div (delay, (rate /1000 ) *36 *ROCKCHIP_MMC_DELAY_ELEMENT_PSEC ); + delay_num = (u8 )min (delay,255ULL ); raw_value = delay_num ?ROCKCHIP_MMC_DELAY_SEL :0 ;Signature · 1 line
2.5.1
Heiko Stübner
maintainerPATCH v2 1/8 clk: rockchip: Allow more precision for some mmc clock phases
Hi Mike,
Am Donnerstag, 22. Oktober 2015, 05:03:26 schrieb Michael Turquette:
Quoted 18 lines
Quoting Heiko Stuebner (2015-09-30 07:07:37)
Quoted 16 lines
From: Douglas Anderson <[email protected]>
Because of the inexact nature of the extra MMC delay elements (it's not possible to keep the phase monotonic and to also make phases (mod
90) > 70), we previously only allowed phases (mod 90) of 22.5, 45, and 67.5.But it's not the end of the world if the MMC clock phase goes non-monotonic. At most we'll be 25 degrees off. It's way better to test more phases to look for bad ones than to be 25 degrees off, because in the case of MMC really the point is to find bad phases and get as far asway from the as possible. If we get to test extra phases by going slightly non-monotonic then that might be fine. Worst case we would end up at a phases that's slight differnt than the one we wanted, but at least we'd still be quite far away from the a bad phase.
Quoted 8 lines
Quoted 6 lines
Signed-off-by Douglas Anderson <[email protected]>Fold in more precise variance-values of 44-77 instead of 40-80. Fold in the actual removal of the monotonic requirement and adapt patch message accordingly.
Signed-off-by Heiko Stuebner <[email protected]>Looks good to me. What tree do you want this to go through?
we got already an Ack from Stephen [in the thread of patch3] for the clock patches and they are already in the mmc.tree [0].
Heiko
[0]
https://git.linaro.org/people/ulf.hansson/mmc.git/commit/6cf04362a13de8cdb59b85b8dec1d53f90307dd1
Heiko Stuebner
maintainerPATCH v2 2/8 clk: rockchip: Make calculations use rounding
In reply to Heiko StuebnerFrom: Douglas Anderson <[email protected]>
Let's use DIV_ROUND_CLOSEST for rounding, not just truncating division. This lets us get closer to the right rate.
Before this:
set_phase(86) delay_nums=26 reg[0xf000420c]=0x468 actual_degrees=83 set_phase(89) delay_nums=27 reg[0xf000420c]=0x46c actual_degrees=86
After this:
set_phase(86) delay_nums=27 reg[0xf000420c]=0x46c actual_degrees=86 set_phase(89) delay_nums=28 reg[0xf000420c]=0x470 actual_degrees=90
--- drivers/clk/rockchip/clk-mmc-phase.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/clk/rockchip/clk-mmc-phase.c +7 -6
@@ -69,7 +69,7 @@ static int rockchip_mmc_get_phase(struct clk_hw *hw) delay_num = (raw_value &ROCKCHIP_MMC_DELAYNUM_MASK ); delay_num >>=ROCKCHIP_MMC_DELAYNUM_OFFSET ; - degrees += delay_num * factor /10000 ; + degrees +=DIV_ROUND_CLOSEST (delay_num * factor,10000 ); }return degrees %360 ;
@@ -82,7 +82,7 @@ static int rockchip_mmc_set_phase(struct clk_hw *hw, int degrees)u8 nineties, remainder;u8 delay_num;u32 raw_value; -u64 delay; +u32 delay; nineties = degrees /90 ; remainder = (degrees %90 );
@@ -110,12 +110,13 @@ static int rockchip_mmc_set_phase(struct clk_hw *hw, int degrees)* Convert to delay; do a little extra work to make sure we * don't overflow 32-bit / 64-bit numbers. */ - delay =PSECS_PER_SEC ; + delay =10000000 ;/* PSECS_PER_SEC / 10000 / 10 */ delay *= remainder; -do_div (delay,10000 ); -do_div (delay, (rate /1000 ) *36 *ROCKCHIP_MMC_DELAY_ELEMENT_PSEC ); + delay =DIV_ROUND_CLOSEST (delay, + (rate /1000 ) *36 * + (ROCKCHIP_MMC_DELAY_ELEMENT_PSEC /10 )); - delay_num = (u8 )min (delay,255ULL ); + delay_num = (u8 )min_t (u32, delay,255 ); raw_value = delay_num ?ROCKCHIP_MMC_DELAY_SEL :0 ; raw_value |= delay_num <<ROCKCHIP_MMC_DELAYNUM_OFFSET ;
Signature · 1 line
2.5.1
PATCH v2 2/8 clk: rockchip: Make calculations use rounding
Quoting Heiko Stuebner (2015-09-30 07:07:38)
Quoted 9 lines
From: Douglas Anderson <[email protected]>
Let's use
DIV_ROUND_CLOSESTfor rounding, not just truncating division. This lets us get closer to the right rate.Before this:
set_phase(86)delay_nums=26 reg[0xf000420c]=0x468 actual_degrees=83set_phase(89)delay_nums=27 reg[0xf000420c]=0x46c actual_degrees=86
Quoted 6 lines
After this:
set_phase(86)delay_nums=27 reg[0xf000420c]=0x46c actual_degrees=86set_phase(89)delay_nums=28 reg[0xf000420c]=0x470 actual_degrees=90Signed-off-by Douglas Anderson <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>
Looks good to me. What tree do you want this to go through?
Regards,
Mike
Quoted 47 lines
--- drivers/clk/rockchip/clk-mmc-phase.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)drivers/clk/rockchip/clk-mmc-phase.c +7
-6@@ -69,7 +69,7 @@ static int rockchip_mmc_get_phase(struct clk_hw *hw) delay_num = (raw_value &ROCKCHIP_MMC_DELAYNUM_MASK ); delay_num >>=ROCKCHIP_MMC_DELAYNUM_OFFSET ; - degrees += delay_num * factor /10000 ; + degrees +=DIV_ROUND_CLOSEST (delay_num * factor,10000 ); }return degrees %360 ;@@ -82,7 +82,7 @@ static int rockchip_mmc_set_phase(struct clk_hw *hw, int degrees)u8 nineties, remainder;u8 delay_num;u32 raw_value; -u64 delay; +u32 delay; nineties = degrees /90 ; remainder = (degrees %90 );@@ -110,12 +110,13 @@ static int rockchip_mmc_set_phase(struct clk_hw *hw, int degrees)* Convert to delay; do a little extra work to make sure we * don't overflow 32-bit / 64-bit numbers. */ - delay =PSECS_PER_SEC ; + delay =10000000 ;/* PSECS_PER_SEC / 10000 / 10 */ delay *= remainder; -do_div (delay,10000 ); -do_div (delay, (rate /1000 ) *36 *ROCKCHIP_MMC_DELAY_ELEMENT_PSEC ); + delay =DIV_ROUND_CLOSEST (delay, + (rate /1000 ) *36 * + (ROCKCHIP_MMC_DELAY_ELEMENT_PSEC /10 )); - delay_num = (u8 )min (delay,255ULL ); + delay_num = (u8 )min_t (u32, delay,255 ); raw_value = delay_num ?ROCKCHIP_MMC_DELAY_SEL :0 ; raw_value |= delay_num <<ROCKCHIP_MMC_DELAYNUM_OFFSET ;Signature · 1 line
2.5.1
Heiko Stuebner
maintainerPATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
In reply to Heiko StuebnerFrom: Douglas Anderson <[email protected]>
This adds logic to the MMC core to set VQMMC. This is expected to be called by MMC drivers like dw_mmc as part of (or instead of) their start_signal_voltage_switch() callback.
A few notes:
* When setting the signal voltage to 3.3V we do our best to make VQMMC and VMMC match. It's been reported that this makes some old cards happy since they were tested back in the day before UHS when VQMMC and VMMC were provided by the same regulator. A nice side effect of this is that we don't end up on the hairy edge of VQMMC (2.7V), which some EEs claim is a little too close to the minimum for comfort.
This is done in two steps. At first we try to find a VQMMC within a 0.3V tolerance of VMMC and if this is not supported by the supplying regulator we try to find a suitable voltage within the whole 2.7V-3.6V area of the spec.
* The two step approach is currently necessary, as the used regulator_set_voltage_triplet(min, target, max) uses a simple implementation that just tries two basic steps:
regulator_set_voltage(target, max); regulator_set_voltage(min, target);
So with only one step with 2.7-3.6V borders, if a suitable voltage is a bit below VMMC, we would directly get the lowest 2.7V which some boards (like Rockchips) don't like at all.
* When setting the signal voltage to 1.8V or 1.2V we aim for that specific voltage instead of picking the lowest one in the range.
* We very purposely don't print errors in mmc_regulator_set_vqmmc(). There are cases where the MMC core will try several different voltages and we don't want to pollute the logs.
--- drivers/mmc/core/core.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/mmc/host.h | 7 +++++ 2 files changed, 81 insertions(+)
drivers/mmc/core/core.c +74 -0
@@ -1437,6 +1437,80 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc, }EXPORT_SYMBOL_GPL (mmc_regulator_set_ocr); +static int mmc_regulator_set_voltage_if_supported (struct regulator *regulator, +int min_uV,int target_uV, +int max_uV) +{ +/* +* Check if supported first to avoid errors since we may try several +* signal levels during power up and don't want to show errors. +*/ +if (!regulator_is_supported_voltage (regulator, min_uV, max_uV)) +return -EINVAL ; + +return regulator_set_voltage_triplet (regulator, min_uV, target_uV, + max_uV); +} + +/** +* mmc_regulator_set_vqmmc - Set VQMMC as per the ios +* +* For 3.3V signaling, we try to match VQMMC to VMMC as closely as possible. +* That will match the behavior of old boards where VQMMC and VMMC were supplied +* by the same supply. The Bus Operating conditions for 3.3V signaling in the +* SD card spec also define VQMMC in terms of VMMC. +* If this is not possible we'll try the full 2.7-3.6V of the spec. +* +* For 1.2V and 1.8V signaling we'll try to get as close as possible to the +* requested voltage. This is definitely a good idea for UHS where there's a +* separate regulator on the card that's trying to make 1.8V and it's best if +* we match. +* +* This function is expected to be used by a controller's +* start_signal_voltage_switch() function. +*/ +int mmc_regulator_set_vqmmc (struct mmc_host *mmc,struct mmc_ios *ios) +{ +int volt, min_uV, max_uV; + +/* If no vqmmc supply then we can't change the voltage */ +if (IS_ERR (mmc->supply.vqmmc)) +return -EINVAL ; + +switch (ios->signal_voltage) { +case MMC_SIGNAL_VOLTAGE_120 : +return mmc_regulator_set_voltage_if_supported (mmc->supply.vqmmc, +1100000 ,1200000 ,1300000 ); +case MMC_SIGNAL_VOLTAGE_180 : +return mmc_regulator_set_voltage_if_supported (mmc->supply.vqmmc, +1700000 ,1800000 ,1950000 ); +case MMC_SIGNAL_VOLTAGE_330 : + volt =regulator_get_voltage (mmc->supply.vmmc); +if (volt <0 ) +return volt; + + min_uV =max (volt -300000 ,2700000 ); + max_uV =min (volt +300000 ,3600000 ); + +/* +* Due to a limitation in the current implementation of +* regulator_set_voltage_triplet() which is taking the lowest +* voltage possible if below the target, search for a suitable +* voltage in two steps and try to stay close to vmmc +* with a 0.3V tolerance@first. +*/ +if (!mmc_regulator_set_voltage_if_supported (mmc->supply.vqmmc, + min_uV, volt, max_uV)) +return 0 ; + +return mmc_regulator_set_voltage_if_supported (mmc->supply.vqmmc, +2700000 , volt,3600000 ); +default : +return -EINVAL ; + } +} +EXPORT_SYMBOL_GPL (mmc_regulator_set_vqmmc); +#endif /* CONFIG_REGULATOR */ int mmc_regulator_get_supply (struct mmc_host *mmc)
include/linux/mmc/host.h +7 -0
@@ -423,6 +423,7 @@ int mmc_regulator_get_ocrmask(struct regulator *supply);int mmc_regulator_set_ocr (struct mmc_host *mmc,struct regulator *supply,unsigned short vdd_bit); +int mmc_regulator_set_vqmmc (struct mmc_host *mmc,struct mmc_ios *ios);#else static inline int mmc_regulator_get_ocrmask (struct regulator *supply) {
@@ -435,6 +436,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc, {return 0 ; } + +static inline int mmc_regulator_set_vqmmc (struct mmc_host *mmc, +struct mmc_ios *ios) +{ +return -EINVAL ; +}#endif int mmc_regulator_get_supply (struct mmc_host *mmc);
Signature · 1 line
2.5.1
PATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
Quoted 32 lines · Heiko Stuebner
From: Douglas Anderson <[email protected]>
This adds logic to the MMC core to set VQMMC. This is expected to be called by MMC drivers like
dw_mmcas part of (or instead of) theirstart_signal_voltage_switch()callback.A few notes:
* When setting the signal voltage to 3.3V we do our best to make VQMMC and VMMC match. It's been reported that this makes some old cards happy since they were tested back in the day before UHS when VQMMC and VMMC were provided by the same regulator. A nice side effect of this is that we don't end up on the hairy edge of VQMMC (2.7V), which some EEs claim is a little too close to the minimum for comfort.
This is done in two steps. At first we try to find a VQMMC within a 0.3V tolerance of VMMC and if this is not supported by the supplying regulator we try to find a suitable voltage within the whole 2.7V-3.6V area of the spec.* The two step approach is currently necessary, as the used
regulator_set_voltage_triplet(min, target, max)uses a simple implementation that just tries two basic steps:regulator_set_voltage(target, max); regulator_set_voltage(min, target);So with only one step with 2.7-3.6V borders, if a suitable voltage is a bit below VMMC, we would directly get the lowest 2.7V which some boards (like Rockchips) don't like at all.
* When setting the signal voltage to 1.8V or 1.2V we aim for that specific voltage instead of picking the lowest one in the range.
Quoted 6 lines
* We very purposely don't print errors in
mmc_regulator_set_vqmmc(). There are cases where the MMC core will try several different voltages and we don't want to pollute the logs.Signed-off-by Douglas Anderson <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>
This looks good to me!
Once all are happy with the patches, can we take the mmc patches via my mmc tree or does it all have to go together?
Kind regards
Uffe
Quoted 118 lines
--- drivers/mmc/core/core.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/mmc/host.h | 7 +++++ 2 files changed, 81 insertions(+)drivers/mmc/core/core.c +74
-0@@ -1437,6 +1437,80 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc, }EXPORT_SYMBOL_GPL (mmc_regulator_set_ocr); +static int mmc_regulator_set_voltage_if_supported (struct regulator *regulator, +int min_uV,int target_uV, +int max_uV) +{ +/* +* Check if supported first to avoid errors since we may try several +* signal levels during power up and don't want to show errors. +*/ +if (!regulator_is_supported_voltage (regulator, min_uV, max_uV)) +return -EINVAL ; + +return regulator_set_voltage_triplet (regulator, min_uV, target_uV, + max_uV); +} + +/** +* mmc_regulator_set_vqmmc - Set VQMMC as per the ios +* +* For 3.3V signaling, we try to match VQMMC to VMMC as closely as possible. +* That will match the behavior of old boards where VQMMC and VMMC were supplied +* by the same supply. The Bus Operating conditions for 3.3V signaling in the +* SD card spec also define VQMMC in terms of VMMC. +* If this is not possible we'll try the full 2.7-3.6V of the spec. +* +* For 1.2V and 1.8V signaling we'll try to get as close as possible to the +* requested voltage. This is definitely a good idea for UHS where there's a +* separate regulator on the card that's trying to make 1.8V and it's best if +* we match. +* +* This function is expected to be used by a controller's +* start_signal_voltage_switch() function. +*/ +int mmc_regulator_set_vqmmc (struct mmc_host *mmc,struct mmc_ios *ios) +{ +int volt, min_uV, max_uV; + +/* If no vqmmc supply then we can't change the voltage */ +if (IS_ERR (mmc->supply.vqmmc)) +return -EINVAL ; + +switch (ios->signal_voltage) { +case MMC_SIGNAL_VOLTAGE_120 : +return mmc_regulator_set_voltage_if_supported (mmc->supply.vqmmc, +1100000 ,1200000 ,1300000 ); +case MMC_SIGNAL_VOLTAGE_180 : +return mmc_regulator_set_voltage_if_supported (mmc->supply.vqmmc, +1700000 ,1800000 ,1950000 ); +case MMC_SIGNAL_VOLTAGE_330 : + volt =regulator_get_voltage (mmc->supply.vmmc); +if (volt <0 ) +return volt; + + min_uV =max (volt -300000 ,2700000 ); + max_uV =min (volt +300000 ,3600000 ); + +/* +* Due to a limitation in the current implementation of +* regulator_set_voltage_triplet() which is taking the lowest +* voltage possible if below the target, search for a suitable +* voltage in two steps and try to stay close to vmmc +* with a 0.3V tolerance at first. +*/ +if (!mmc_regulator_set_voltage_if_supported (mmc->supply.vqmmc, + min_uV, volt, max_uV)) +return 0 ; + +return mmc_regulator_set_voltage_if_supported (mmc->supply.vqmmc, +2700000 , volt,3600000 ); +default : +return -EINVAL ; + } +} +EXPORT_SYMBOL_GPL (mmc_regulator_set_vqmmc); +#endif /* CONFIG_REGULATOR */ int mmc_regulator_get_supply (struct mmc_host *mmc)include/linux/mmc/host.h +7
-1@@ -423,6 +423,7 @@ int mmc_regulator_get_ocrmask(struct regulator *supply);int mmc_regulator_set_ocr (struct mmc_host *mmc,struct regulator *supply,unsigned short vdd_bit); +int mmc_regulator_set_vqmmc (struct mmc_host *mmc,struct mmc_ios *ios);#else static inline int mmc_regulator_get_ocrmask (struct regulator *supply) {@@ -435,6 +436,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc, {return 0 ; } + +static inline int mmc_regulator_set_vqmmc (struct mmc_host *mmc, +struct mmc_ios *ios) +{ +return -EINVAL ; +}#endif int mmc_regulator_get_supply (struct mmc_host *mmc); --2.5.1
Heiko Stübner
maintainerPATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
Am Mittwoch, 30. September 2015, 16:42:05 schrieb Ulf Hansson:
Quoted 40 lines · Heiko Stuebner
Quoted 39 lines
From: Douglas Anderson <[email protected]>
This adds logic to the MMC core to set VQMMC. This is expected to be called by MMC drivers like
dw_mmcas part of (or instead of) theirstart_signal_voltage_switch()callback.A few notes:
* When setting the signal voltage to 3.3V we do our best to make VQMMC
and VMMC match. It's been reported that this makes some old cards happy since they were tested back in the day before UHS when VQMMC and VMMC were provided by the same regulator. A nice side effect of this is that we don't end up on the hairy edge of VQMMC (2.7V), which some EEs claim is a little too close to the minimum for comfort.
This is done in two steps. At first we try to find a VQMMC within a 0.3V tolerance of VMMC and if this is not supported by the supplying regulator we try to find a suitable voltage within the whole 2.7V-3.6V area of the spec.* The two step approach is currently necessary, as the used
regulator_set_voltage_triplet(min, target, max)uses a simpleimplementation that just tries two basic steps: regulator_set_voltage(target, max); regulator_set_voltage(min, target);So with only one step with 2.7-3.6V borders, if a suitable voltage is a bit below VMMC, we would directly get the lowest 2.7V which some boards (like Rockchips) don't like at all.
* When setting the signal voltage to 1.8V or 1.2V we aim for that
specific voltage instead of picking the lowest one in the range.
* We very purposely don't print errors in
mmc_regulator_set_vqmmc().
Quoted 7 lines
Quoted 5 lines
There are cases where the MMC core will try several different voltages and we don't want to pollute the logs.
Signed-off-by Douglas Anderson <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>This looks good to me!
very cool :-)
Quoted 2 lines
Once all are happy with the patches, can we take the mmc patches via my mmc tree or does it all have to go together?
The clock changes of course only touch internals of the phase-clocks, so should have no problem going through another tree.
For the devicetree part I'm unsure. If the boards enable the tuning-related settings without the new voltage handling, 2.7V gets set on all Rockchip boards which doesn't work on those at all.
So either the dts patches would need to go into your tree, I would need a stable branch or we could simply postpone dts changes for the next cycle.
Heiko
Quoted 138 lines
Quoted 138 lines
---
drivers/mmc/core/core.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/mmc/host.h | 7 +++++ 2 files changed, 81 insertions(+)drivers/mmc/core/core.c +1
-0@@ -1437,6 +1437,80 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc, }EXPORT_SYMBOL_GPL (mmc_regulator_set_ocr); +static int mmc_regulator_set_voltage_if_supported(struct regulator *regulator, + int min_uV,int
target_uV, + intmax_uV)drivers/mmc/core/core.c +3
-0+{ +/* +* Check if supported first to avoid errors since we may try several + * signal levels during power up and don't want to show
errors. + */drivers/mmc/core/core.c +11
-0+if (!regulator_is_supported_voltage (regulator, min_uV, max_uV)) +return -EINVAL ; + +return regulator_set_voltage_triplet (regulator, min_uV, target_uV, + max_uV); +} + +/** +* mmc_regulator_set_vqmmc - Set VQMMC as per the ios +* +* For 3.3V signaling, we try to match VQMMC to VMMC as closely as possible. + * That will match the behavior of old boards where VQMMC and VMMC were supplied + * by the same supply. The Bus Operating conditions for 3.3V signaling in the + * SD card spec also define VQMMC in terms of VMMC.
+ * If this is not possible we'll try the full 2.7-3.6V of the spec.
+ *
+ * For 1.2V and 1.8V signaling we'll try to get as close as possible to the + * requested voltage. This is definitely a good idea for UHS where there's a + * separate regulator on the card that's trying to make 1.8V and it's best if + * we match.
+ *
+ * This function is expected to be used by a controller's
+ *start_signal_voltage_switch()function.
+ */drivers/mmc/core/core.c +11
-0+int mmc_regulator_set_vqmmc (struct mmc_host *mmc,struct mmc_ios *ios) +{ +int volt, min_uV, max_uV; + +/* If no vqmmc supply then we can't change the voltage */ +if (IS_ERR (mmc->supply.vqmmc)) +return -EINVAL ; + +switch (ios->signal_voltage) { +case MMC_SIGNAL_VOLTAGE_120 : +return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc, +
1100000, 1200000, 1300000); + case MMC_SIGNAL_VOLTAGE_180: + return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc, + 1700000, 1800000, 1950000); + case
MMC_SIGNAL_VOLTAGE_330:drivers/mmc/core/core.c +10
-0+ volt =regulator_get_voltage (mmc->supply.vmmc); +if (volt <0 ) +return volt; + + min_uV =max (volt -300000 ,2700000 ); + max_uV =min (volt +300000 ,3600000 ); + +/* +* Due to a limitation in the current implementation of +* regulator_set_voltage_triplet() which is taking the lowest + * voltage possible if below the target, search
for a suitable + * voltage in two steps and try to stay
close to vmmc + * with a 0.3V tolerance at first.drivers/mmc/core/core.c +2
-0+*/ + if(!mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc, +
min_uV, volt, max_uV)) + return 0;drivers/mmc/core/core.c +2
-0+ +return mmc_regulator_set_voltage_if_supported(mmc->supply.vqmmc, +
2700000, volt, 3600000); + default:drivers/mmc/core/core.c +5
-0+ return -EINVAL; + } +} +EXPORT_SYMBOL_GPL (mmc_regulator_set_vqmmc); +#endif /* CONFIG_REGULATOR */ int mmc_regulator_get_supply (struct mmc_host *mmc)include/linux/mmc/host.h +1
-0@@ -423,6 +423,7 @@ int mmc_regulator_get_ocrmask(struct regulator *supply);>int mmc_regulator_set_ocr (struct mmc_host *mmc,struct regulator *supply,unsigned short vdd_bit); +int mmc_regulator_set_vqmmc (struct mmc_host *mmc,struct mmc_ios *ios);#else
static inline intmmc_regulator_get_ocrmask(struct regulator *supply){include/linux/mmc/host.h +0
-0@@ -435,6 +436,12 @@ static inline int mmc_regulator_set_ocr(struct
mmc_host*mmc,>
{return 0;}
include/linux/mmc/host.h +6
-1+ +static inline int mmc_regulator_set_vqmmc (struct mmc_host *mmc, +struct mmc_ios *ios) +{ +return -EINVAL ; +}#endif int mmc_regulator_get_supply (struct mmc_host *mmc); --2.5.1
PATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
Quoted 49 lines · Heiko St?bner
Am Mittwoch, 30. September 2015, 16:42:05 schrieb Ulf Hansson:
Quoted 40 lines · Heiko Stuebner
Quoted 39 lines
From: Douglas Anderson <[email protected]>
This adds logic to the MMC core to set VQMMC. This is expected to be called by MMC drivers like
dw_mmcas part of (or instead of) theirstart_signal_voltage_switch()callback.A few notes:
* When setting the signal voltage to 3.3V we do our best to make VQMMC
and VMMC match. It's been reported that this makes some old cards happy since they were tested back in the day before UHS when VQMMC and VMMC were provided by the same regulator. A nice side effect of this is that we don't end up on the hairy edge of VQMMC (2.7V), which some EEs claim is a little too close to the minimum for comfort.
This is done in two steps. At first we try to find a VQMMC within a 0.3V tolerance of VMMC and if this is not supported by the supplying regulator we try to find a suitable voltage within the whole 2.7V-3.6V area of the spec.* The two step approach is currently necessary, as the used
regulator_set_voltage_triplet(min, target, max)uses a simpleimplementation that just tries two basic steps: regulator_set_voltage(target, max); regulator_set_voltage(min, target);So with only one step with 2.7-3.6V borders, if a suitable voltage is a bit below VMMC, we would directly get the lowest 2.7V which some boards (like Rockchips) don't like at all.
* When setting the signal voltage to 1.8V or 1.2V we aim for that
specific voltage instead of picking the lowest one in the range.
* We very purposely don't print errors in
mmc_regulator_set_vqmmc().Quoted 7 lines
Quoted 5 lines
There are cases where the MMC core will try several different voltages and we don't want to pollute the logs.
Signed-off-by Douglas Anderson <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>This looks good to me!
Quoted 8 lines
very cool :-)
Quoted 2 lines
Once all are happy with the patches, can we take the mmc patches via my mmc tree or does it all have to go together?
The clock changes of course only touch internals of the phase-clocks, so should have no problem going through another tree.
What happens if I take mmc and dt changes, wouldn't I need the clock patches as well?
Quoted 8 lines
For the devicetree part I'm unsure. If the boards enable the tuning-related settings without the new voltage handling, 2.7V gets set on all Rockchip boards which doesn't work on those at all.
So either the dts patches would need to go into your tree, I would need a stable branch or we could simply postpone dts changes for the next cycle.
Kind regards
Uffe
Heiko Stübner
maintainerPATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
Am Donnerstag, 1. Oktober 2015, 11:54:24 schrieb Ulf Hansson:
Quoted 52 lines · Heiko St?bner
Quoted 51 lines
Am Mittwoch, 30. September 2015, 16:42:05 schrieb Ulf Hansson:
Quoted 40 lines · Heiko Stuebner
Quoted 39 lines
From: Douglas Anderson <[email protected]>
This adds logic to the MMC core to set VQMMC. This is expected to be called by MMC drivers like
dw_mmcas part of (or instead of) theirstart_signal_voltage_switch()callback.A few notes:
* When setting the signal voltage to 3.3V we do our best to make VQMMC
and VMMC match. It's been reported that this makes some old cards happy since they were tested back in the day before UHS when VQMMC and VMMC were provided by the same regulator. A nice side effect of this is that we don't end up on the hairy edge of VQMMC (2.7V), which some EEs claim is a little too close to the minimum for comfort.
This is done in two steps. At first we try to find a VQMMC within a 0.3V tolerance of VMMC and if this is not supported by the supplying regulator we try to find a suitable voltage within the whole 2.7V-3.6V area of the spec.* The two step approach is currently necessary, as the used
regulator_set_voltage_triplet(min, target, max)uses a simpleimplementation that just tries two basic steps: regulator_set_voltage(target, max); regulator_set_voltage(min, target);So with only one step with 2.7-3.6V borders, if a suitable voltage is a bit below VMMC, we would directly get the lowest 2.7V which some boards (like Rockchips) don't like at all.
* When setting the signal voltage to 1.8V or 1.2V we aim for that
specific voltage instead of picking the lowest one in the range.
* We very purposely don't print errors in
mmc_regulator_set_vqmmc().Quoted 7 lines
Quoted 5 lines
There are cases where the MMC core will try several different voltages and we don't want to pollute the logs.
Signed-off-by Douglas Anderson <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>This looks good to me!
very cool :-)
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Once all are happy with the patches, can we take the mmc patches via my mmc tree or does it all have to go together?
The clock changes of course only touch internals of the phase-clocks, so should have no problem going through another tree.
What happens if I take mmc and dt changes, wouldn't I need the clock patches as well?
The API stays of course the same, only the degree to settings translation gets optimized, so I guess in the worst case you would get no good phase and thus fall back to non-highspeed modes - but the system would stay running.
But of course, if the clock maintainers could Ack the two clock patches and everything would stay together that would work even better :-)
Heiko
Quoted 11 lines
Quoted 8 lines
For the devicetree part I'm unsure. If the boards enable the tuning-related
settings without the new voltage handling, 2.7V gets set on all Rockchip boards which doesn't work on those at all.So either the dts patches would need to go into your tree, I would need a stable branch or we could simply postpone dts changes for the next cycle.
Kind regards
Uffe
PATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
Quoted 10 lines · Heiko St?bner
Am Donnerstag, 1. Oktober 2015, 11:54:24 schrieb Ulf Hansson:
Quoted 8 lines · Heiko St?bner
Quoted 4 lines
Am Mittwoch, 30. September 2015, 16:42:05 schrieb Ulf Hansson:
Quoted 1 line · Heiko Stuebner
The clock changes of course only touch internals of the phase-clocks, so should have no problem going through another tree.
What happens if I take mmc and dt changes, wouldn't I need the clock patches as well?
Quoted 7 lines
The API stays of course the same, only the degree to settings translation gets optimized, so I guess in the worst case you would get no good phase and thus fall back to non-highspeed modes - but the system would stay running.
But of course, if the clock maintainers could Ack the two clock patches and everything would stay together that would work even better :-)
If Ulf doesn't want to take them we can apply them to clk tree. Otherwise, you can have my acked-by on the clk patches.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
Quoted 15 lines · Stephen Boyd
Quoted 14 lines · Heiko St?bner
Am Donnerstag, 1. Oktober 2015, 11:54:24 schrieb Ulf Hansson:
Quoted 8 lines · Heiko St?bner
Quoted 4 lines
Am Mittwoch, 30. September 2015, 16:42:05 schrieb Ulf Hansson:
Quoted 1 line · Heiko Stuebner
The clock changes of course only touch internals of the phase-clocks, so should have no problem going through another tree.
What happens if I take mmc and dt changes, wouldn't I need the clock patches as well?
The API stays of course the same, only the degree to settings translation gets optimized, so I guess in the worst case you would get no good phase and thus fall back to non-highspeed modes - but the system would stay running.
Quoted 6 lines
Quoted 3 lines
But of course, if the clock maintainers could Ack the two clock patches and everything would stay together that would work even better :-)
If Ulf doesn't want to take them we can apply them to clk tree. Otherwise, you can have my acked-by on the clk patches.
I don't mind picking up the clock patches. So I consider this as an ack for both patch 1 and patch2, thanks.
Now, let's give Jaehoon some time to review the dw_mmc parts.
Kind regards
Uffe
Jaehoon Chung
maintainerPATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
Quoted 20 lines · Ulf Hansson
Quoted 19 lines · Stephen Boyd
Quoted 17 lines · Heiko St?bner
Am Donnerstag, 1. Oktober 2015, 11:54:24 schrieb Ulf Hansson:
Quoted 8 lines · Heiko St?bner
Quoted 4 lines
Am Mittwoch, 30. September 2015, 16:42:05 schrieb Ulf Hansson:
Quoted 1 line · Heiko Stuebner
The clock changes of course only touch internals of the phase-clocks, so should have no problem going through another tree.
What happens if I take mmc and dt changes, wouldn't I need the clock patches as well?
The API stays of course the same, only the degree to settings translation gets optimized, so I guess in the worst case you would get no good phase and thus fall back to non-highspeed modes - but the system would stay running.
But of course, if the clock maintainers could Ack the two clock patches and everything would stay together that would work even better :-)
Quoted 7 lines
Quoted 2 lines
If Ulf doesn't want to take them we can apply them to clk tree. Otherwise, you can have my acked-by on the clk patches.
I don't mind picking up the clock patches. So I consider this as an ack for both patch 1 and patch2, thanks.
Now, let's give Jaehoon some time to review the
dw_mmcparts.
I will check other patches on today, if it's ok, i will apply at my repository. Thanks for giving time! :)
Best Regards,
Jaehoon Chung
Quoted 4 lines
Kind regards
Uffe
Heiko Stübner
maintainerPATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
Am Freitag, 2. Oktober 2015, 11:52:00 schrieb Jaehoon Chung:
Quoted 7 lines · Ulf Hansson
Quoted 6 lines · Stephen Boyd
Quoted 5 lines · Heiko St?bner
Quoted 4 lines
Am Donnerstag, 1. Oktober 2015, 11:54:24 schrieb Ulf Hansson:
Quoted 3 lines · Heiko St?bner
Quoted 2 lines
Am Mittwoch, 30. September 2015, 16:42:05 schrieb Ulf Hansson:
Quoted 1 line
On 30 September 2015 at 16:07, Heiko Stuebner <[email protected]>
Quoted 19 lines · wrote:
Quoted 19 lines
Quoted 18 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
The clock changes of course only touch internals of the phase-clocks, so
should have no problem going through another tree.What happens if I take mmc and dt changes, wouldn't I need the clock patches as well?
Quoted 8 lines
The API stays of course the same, only the degree to settings translation gets optimized, so I guess in the worst case you would get no good phase and thus fall back to non-highspeed modes - but the system would stay running.
But of course, if the clock maintainers could Ack the two clock patches and
everything would stay together that would work even better :-)If Ulf doesn't want to take them we can apply them to clk tree. Otherwise, you can have my acked-by on the clk patches.
Quoted 7 lines
Quoted 4 lines
I don't mind picking up the clock patches. So I consider this as an ack for both patch 1 and patch2, thanks.
Now, let's give Jaehoon some time to review the
dw_mmcparts.I will check other patches on today, if it's ok, i will apply at my repository. Thanks for giving time! :)
I think Ulf wanted to apply the whole series via the mmc tree directly, but I guess that is between you two to decide ;-)
Heiko
Jaehoon Chung
maintainerPATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
Quoted 31 lines · Heiko St?bner
Am Freitag, 2. Oktober 2015, 11:52:00 schrieb Jaehoon Chung:
Quoted 7 lines · Ulf Hansson
Quoted 6 lines · Stephen Boyd
Quoted 5 lines · Heiko St?bner
Quoted 4 lines
Am Donnerstag, 1. Oktober 2015, 11:54:24 schrieb Ulf Hansson:
Quoted 3 lines · Heiko St?bner
Quoted 2 lines
Am Mittwoch, 30. September 2015, 16:42:05 schrieb Ulf Hansson:
Quoted 1 line
On 30 September 2015 at 16:07, Heiko Stuebner <[email protected]>
Quoted 22 lines · wrote:
Quoted 22 lines
Quoted 12 lines
Quoted 12 lines
Quoted 6 lines
Quoted 3 lines
The clock changes of course only touch internals of the phase-clocks, so
should have no problem going through another tree.What happens if I take mmc and dt changes, wouldn't I need the clock patches as well?
The API stays of course the same, only the degree to settings translation gets optimized, so I guess in the worst case you would get no good phase and thus fall back to non-highspeed modes - but the system would stay running.
Quoted 6 lines
Quoted 3 lines
But of course, if the clock maintainers could Ack the two clock patches and
everything would stay together that would work even better :-)If Ulf doesn't want to take them we can apply them to clk tree. Otherwise, you can have my acked-by on the clk patches.
I don't mind picking up the clock patches. So I consider this as an ack for both patch 1 and patch2, thanks.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Now, let's give Jaehoon some time to review the
dw_mmcparts.I will check other patches on today, if it's ok, i will apply at my repository. Thanks for giving time! :)
I think Ulf wanted to apply the whole series via the mmc tree directly, but I guess that is between you two to decide ;-)
If it will get Stephen's acked-by, i will pick up all after checked the dwmmc patches. how about?
Best Regards,
Jaehoon Chung
Quoted 4 lines
Heiko
PATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
Quoted 34 lines · Jaehoon Chung
Quoted 33 lines · Heiko St?bner
Am Freitag, 2. Oktober 2015, 11:52:00 schrieb Jaehoon Chung:
Quoted 7 lines · Ulf Hansson
Quoted 6 lines · Stephen Boyd
Quoted 5 lines · Heiko St?bner
Quoted 4 lines
Am Donnerstag, 1. Oktober 2015, 11:54:24 schrieb Ulf Hansson:
Quoted 3 lines · Heiko St?bner
Quoted 2 lines
Am Mittwoch, 30. September 2015, 16:42:05 schrieb Ulf Hansson:
Quoted 1 line
On 30 September 2015 at 16:07, Heiko Stuebner <[email protected]>
Quoted 24 lines · wrote:
Quoted 23 lines
Quoted 12 lines
Quoted 12 lines
Quoted 6 lines
Quoted 3 lines
The clock changes of course only touch internals of the phase-clocks, so
should have no problem going through another tree.What happens if I take mmc and dt changes, wouldn't I need the clock patches as well?
The API stays of course the same, only the degree to settings translation gets optimized, so I guess in the worst case you would get no good phase and thus fall back to non-highspeed modes - but the system would stay running.
Quoted 6 lines
Quoted 3 lines
But of course, if the clock maintainers could Ack the two clock patches and
everything would stay together that would work even better :-)If Ulf doesn't want to take them we can apply them to clk tree. Otherwise, you can have my acked-by on the clk patches.
I don't mind picking up the clock patches. So I consider this as an ack for both patch 1 and patch2, thanks.
Now, let's give Jaehoon some time to review the
dw_mmcparts.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
I will check other patches on today, if it's ok, i will apply at my repository. Thanks for giving time! :)
I think Ulf wanted to apply the whole series via the mmc tree directly, but I guess that is between you two to decide ;-)
If it will get Stephen's acked-by, i will pick up all after checked the dwmmc patches. how about?
Stephen already acked the clock patches from this thread.
Perhaps this time, it's easier if you ack the dw_mmc patches and I can pick up the hole series? Does that work for you?
Kind regards
Uffe
Jaehoon Chung
maintainerPATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
Quoted 41 lines · Ulf Hansson
Quoted 40 lines · Jaehoon Chung
Quoted 38 lines · Heiko St?bner
Am Freitag, 2. Oktober 2015, 11:52:00 schrieb Jaehoon Chung:
Quoted 7 lines · Ulf Hansson
Quoted 6 lines · Stephen Boyd
Quoted 5 lines · Heiko St?bner
Quoted 4 lines
Am Donnerstag, 1. Oktober 2015, 11:54:24 schrieb Ulf Hansson:
Quoted 3 lines · Heiko St?bner
Quoted 2 lines
Am Mittwoch, 30. September 2015, 16:42:05 schrieb Ulf Hansson:
Quoted 1 line
On 30 September 2015 at 16:07, Heiko Stuebner <[email protected]>
Quoted 19 lines · wrote:
Quoted 19 lines
Quoted 18 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
The clock changes of course only touch internals of the phase-clocks, so
should have no problem going through another tree.What happens if I take mmc and dt changes, wouldn't I need the clock patches as well?
Quoted 8 lines
The API stays of course the same, only the degree to settings translation gets optimized, so I guess in the worst case you would get no good phase and thus fall back to non-highspeed modes - but the system would stay running.
But of course, if the clock maintainers could Ack the two clock patches and
everything would stay together that would work even better :-)If Ulf doesn't want to take them we can apply them to clk tree. Otherwise, you can have my acked-by on the clk patches.
Quoted 7 lines
Quoted 4 lines
I don't mind picking up the clock patches. So I consider this as an ack for both patch 1 and patch2, thanks.
Now, let's give Jaehoon some time to review the
dw_mmcparts.I will check other patches on today, if it's ok, i will apply at my repository. Thanks for giving time! :)
I think Ulf wanted to apply the whole series via the mmc tree directly, but I guess that is between you two to decide ;-)
Quoted 6 lines
Quoted 1 line
If it will get Stephen's acked-by, i will pick up all after checked the dwmmc patches. how about?
Stephen already acked the clock patches from this thread.
Perhaps this time, it's easier if you ack the
dw_mmcpatches and I can pick up the hole series? Does that work for you?
I doesn't review dwmmc patches (Sorry), could you give me more time? If you're ok, i will notice to you...then you can pick up all. :)
Best Regards,
Jaehoon Chung
Quoted 4 lines
Kind regards
Uffe
PATCH v2 3/8 mmc: core: Add mmc_regulator_set_vqmmc()
[...]
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Now, let's give Jaehoon some time to review the
dw_mmcparts.I will check other patches on today, if it's ok, i will apply at my repository. Thanks for giving time! :)
I think Ulf wanted to apply the whole series via the mmc tree directly, but I guess that is between you two to decide ;-)
If it will get Stephen's acked-by, i will pick up all after checked the dwmmc patches. how about?
Quoted 7 lines
Quoted 4 lines
Stephen already acked the clock patches from this thread.
Perhaps this time, it's easier if you ack the
dw_mmcpatches and I can pick up the hole series? Does that work for you?I doesn't review dwmmc patches (Sorry), could you give me more time? If you're ok, i will notice to you...then you can pick up all. :)
Sure, no worries. Take your time!
Kind regards
Uffe
Heiko Stuebner
maintainerPATCH v2 4/8 mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch
In reply to Heiko StuebnerFrom: Douglas Anderson <[email protected]>
We've introduced a new helper in the MMC core:mmc_regulator_set_vqmmc(). Let's use this in dw_mmc. Using this new helper has some advantages:
1. We get the mmc_regulator_set_vqmmc() behavior of trying to match VQMMC and VMMC when the signal voltage is 3.3V. This ensures max compatibility.
2. We get rid of a few more warnings when probing unsupported voltages.
3. We get rid of some non-dw_mmc specific code in dw_mmc.
--- drivers/mmc/host/dw_mmc.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
drivers/mmc/host/dw_mmc.c +6 -11
@@ -1279,7 +1279,6 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)const struct dw_mci_drv_data *drv_data = host->drv_data;u32 uhs;u32 v18 =SDMMC_UHS_18V << slot->id; -int min_uv, max_uv;int ret;if (drv_data && drv_data->switch_voltage)
@@ -1291,22 +1290,18 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)* does no harm but you need to set the regulator directly. Try both. */ uhs =mci_readl (host,UHS_REG ); -if (ios->signal_voltage ==MMC_SIGNAL_VOLTAGE_330 ) { - min_uv =2700000 ; - max_uv =3600000 ; +if (ios->signal_voltage ==MMC_SIGNAL_VOLTAGE_330 ) uhs &= ~v18; - }else { - min_uv =1700000 ; - max_uv =1950000 ; +else uhs |= v18; - } +if (!IS_ERR (mmc->supply.vqmmc)) { - ret =regulator_set_voltage (mmc->supply.vqmmc, min_uv, max_uv); + ret =mmc_regulator_set_vqmmc (mmc, ios);if (ret) {dev_dbg (&mmc->class_dev, -"Regulator set error %d: %d - %d\n" , - ret, min_uv, max_uv); +"Regulator set error %d - %s V\n" , + ret, uhs & v18 ?"1.8" :"3.3" );return ret; } }
Signature · 1 line
2.5.1
Jaehoon Chung
maintainerPATCH v2 4/8 mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch
Hi, Heiko.
Best Regards,
Jaehoon Chung
Quoted 63 lines · Heiko Stuebner
From: Douglas Anderson <[email protected]>
We've introduced a new helper in the MMC core:
mmc_regulator_set_vqmmc(). Let's use this indw_mmc. Using this new helper has some advantages:1. We get the
mmc_regulator_set_vqmmc()behavior of trying to match VQMMC and VMMC when the signal voltage is 3.3V. This ensures max compatibility.2. We get rid of a few more warnings when probing unsupported voltages.
3. We get rid of some non-dw_mmc specific code in
dw_mmc.Signed-off-by Douglas Anderson <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>--- drivers/mmc/host/dw_mmc.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)drivers/mmc/host/dw_mmc.c +6
-11@@ -1279,7 +1279,6 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)const struct dw_mci_drv_data *drv_data = host->drv_data;u32 uhs;u32 v18 =SDMMC_UHS_18V << slot->id; -int min_uv, max_uv;int ret;if (drv_data && drv_data->switch_voltage)@@ -1291,22 +1290,18 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)* does no harm but you need to set the regulator directly. Try both. */ uhs =mci_readl (host,UHS_REG ); -if (ios->signal_voltage ==MMC_SIGNAL_VOLTAGE_330 ) { - min_uv =2700000 ; - max_uv =3600000 ; +if (ios->signal_voltage ==MMC_SIGNAL_VOLTAGE_330 ) uhs &= ~v18; - }else { - min_uv =1700000 ; - max_uv =1950000 ; +else uhs |= v18; - } +if (!IS_ERR (mmc->supply.vqmmc)) { - ret =regulator_set_voltage (mmc->supply.vqmmc, min_uv, max_uv); + ret =mmc_regulator_set_vqmmc (mmc, ios);if (ret) {dev_dbg (&mmc->class_dev, -"Regulator set error %d: %d - %d\n" , - ret, min_uv, max_uv); +"Regulator set error %d - %s V\n" , + ret, uhs & v18 ?"1.8" :"3.3" );return ret; } }
PATCH v2 4/8 mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch
In reply to Heiko StuebnerQuoted 10 lines · Heiko Stuebner
From: Douglas Anderson <[email protected]>
We've introduced a new helper in the MMC core:
mmc_regulator_set_vqmmc(). Let's use this indw_mmc. Using this new helper has some advantages:1. We get the
mmc_regulator_set_vqmmc()behavior of trying to match VQMMC and VMMC when the signal voltage is 3.3V. This ensures max compatibility.
Quoted 7 lines
2. We get rid of a few more warnings when probing unsupported voltages.
3. We get rid of some non-dw_mmc specific code in
dw_mmc.Signed-off-by Douglas Anderson <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>
kernelci.org found a new failure in linux-next on exynos5800-peach-pi[1] and it was bisected down to this patch, which has hit linux-next in the form of commit fba2e23128d0 (mmc: dw_mmc:
Use mmc_regulator_set_vqmmc in start_signal_voltage_switch). Based on a quick glance at the boot log[2], it looks to be a bad pointer dereference in regulator_get_voltage().
I confirmed that reverting this patch on top of next-20151009 gets my peach-pi booting again.
Note that the boot failure is only on exynos_defconfig, not multi_v7_defconfig.
Kevin
Quoted 53 lines
--- drivers/mmc/host/dw_mmc.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)drivers/mmc/host/dw_mmc.c +6
-12@@ -1279,7 +1279,6 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)const struct dw_mci_drv_data *drv_data = host->drv_data;u32 uhs;u32 v18 =SDMMC_UHS_18V << slot->id; -int min_uv, max_uv;int ret;if (drv_data && drv_data->switch_voltage)@@ -1291,22 +1290,18 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)* does no harm but you need to set the regulator directly. Try both. */ uhs =mci_readl (host,UHS_REG ); -if (ios->signal_voltage ==MMC_SIGNAL_VOLTAGE_330 ) { - min_uv =2700000 ; - max_uv =3600000 ; +if (ios->signal_voltage ==MMC_SIGNAL_VOLTAGE_330 ) uhs &= ~v18; - }else { - min_uv =1700000 ; - max_uv =1950000 ; +else uhs |= v18; - } +if (!IS_ERR (mmc->supply.vqmmc)) { - ret =regulator_set_voltage (mmc->supply.vqmmc, min_uv, max_uv); + ret =mmc_regulator_set_vqmmc (mmc, ios);if (ret) {dev_dbg (&mmc->class_dev, -"Regulator set error %d: %d - %d\n" , - ret, min_uv, max_uv); +"Regulator set error %d - %s V\n" , + ret, uhs & v18 ?"1.8" :"3.3" );return ret; } } --2.5.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 4/8 mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch
Kevin,
Quoted 26 lines · Kevin Hilman
Quoted 10 lines · Heiko Stuebner
From: Douglas Anderson <[email protected]>
We've introduced a new helper in the MMC core:
mmc_regulator_set_vqmmc(). Let's use this indw_mmc. Using this new helper has some advantages:1. We get the
mmc_regulator_set_vqmmc()behavior of trying to match VQMMC and VMMC when the signal voltage is 3.3V. This ensures max compatibility.Quoted 7 lines
2. We get rid of a few more warnings when probing unsupported voltages.
3. We get rid of some non-dw_mmc specific code in
dw_mmc.Signed-off-by Douglas Anderson <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>kernelci.org found a new failure in linux-next on exynos5800-peach-pi[1] and it was bisected down to this patch, which has hit linux-next in the form of commit
fba2e23128d0(mmc:dw_mmc:
Usemmc_regulator_set_vqmmcinstart_signal_voltage_switch). Based on a quick glance at the boot log[2], it looks to be a bad pointer dereference inregulator_get_voltage().
Quoted 4 lines
I confirmed that reverting this patch on top of next-20151009 gets my peach-pi booting again.
Note that the boot failure is only on
exynos_defconfig, notmulti_v7_defconfig.
Right. See <http://www.spinics.net/lists/arm-kernel/msg450802.html>
for discussion. Ulf responded to the cover letter rather than the individual patch.
-Doug
PATCH v2 4/8 mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch
Quoted 29 lines · Doug Anderson
Kevin,
Quoted 26 lines · Kevin Hilman
Quoted 10 lines · Heiko Stuebner
From: Douglas Anderson <[email protected]>
We've introduced a new helper in the MMC core:
mmc_regulator_set_vqmmc(). Let's use this indw_mmc. Using this new helper has some advantages:1. We get the
mmc_regulator_set_vqmmc()behavior of trying to match VQMMC and VMMC when the signal voltage is 3.3V. This ensures max compatibility.Quoted 7 lines
2. We get rid of a few more warnings when probing unsupported voltages.
3. We get rid of some non-dw_mmc specific code in
dw_mmc.Signed-off-by Douglas Anderson <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>kernelci.org found a new failure in linux-next on exynos5800-peach-pi[1] and it was bisected down to this patch, which has hit linux-next in the form of commit
fba2e23128d0(mmc:dw_mmc:
Usemmc_regulator_set_vqmmcinstart_signal_voltage_switch). Based on a quick glance at the boot log[2], it looks to be a bad pointer dereference inregulator_get_voltage().
Quoted 8 lines
Quoted 4 lines
I confirmed that reverting this patch on top of next-20151009 gets my peach-pi booting again.
Note that the boot failure is only on
exynos_defconfig, notmulti_v7_defconfig.Right. See <http://www.spinics.net/lists/arm-kernel/msg450802.html>
for discussion. Ulf responded to the cover letter rather than the individual patch.
Ah, I hadn't noticed that. Thanks.
I guess it should be dropped by tomorrow's linux-next then. I'll keep an eye on it.
Kevin
Heiko Stuebner
maintainerPATCH v2 5/8 mmc: dw_mmc-rockchip: dt-binding: Add tuning related things
In reply to Heiko StuebnerFrom: Alexandru M Stan <[email protected]>
Add ciu_drive, ciu_sample clocks and default-sample-phase. This will later be used by tuning code.
We do not touch ciu_drive (and by extension define default-drive-phase). Drive phase is mostly used to define minimum hold times, while one could write some code to determine what phase meets the minimum hold time (ex 10 degrees) this will not work with the current clock phase framework (which floors angles, so we'll get 0 deg, and there's no way to know what resolution the floors happen at). We assume that the default drive angles set by the hardware are good enough.
--- Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt | 13 +++++++++++++ 1 file changed, 13 insertions(+)
Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt +13 -0
@@ -14,6 +14,19 @@ Required Properties: before RK3288 - "rockchip,rk3288-dw-mshc": for Rockchip RK3288 +Optional Properties: +* clocks: from common clock binding: if ciu_drive and ciu_sample are + specified in clock-names, should contain handles to these clocks. + +* clock-names: Apart from the clock-names described in synopsys-dw-mshc.txt + two more clocks "ciu-drive" and "ciu-sample" are supported. They are used + to control the clock phases, "ciu-sample" is required for tuning high- + speed modes. + +* rockchip,default-sample-phase: The default phase to set ciu_sample at + probing, low speeds or in case where all phases work at tuning time. + If not specified 0 deg will be used. + Example: rkdwmmc0 at 12200000 {
Signature · 1 line
2.5.1
Jaehoon Chung
maintainerPATCH v2 5/8 mmc: dw_mmc-rockchip: dt-binding: Add tuning related things
Hi, Heiko.
Best Regards,
Jaehoon Chung
Quoted 44 lines · Heiko Stuebner
From: Alexandru M Stan <[email protected]>
Add
ciu_drive,ciu_sampleclocks and default-sample-phase. This will later be used by tuning code.We do not touch
ciu_drive(and by extension define default-drive-phase). Drive phase is mostly used to define minimum hold times, while one could write some code to determine what phase meets the minimum hold time (ex 10 degrees) this will not work with the current clock phase framework (which floors angles, so we'll get 0 deg, and there's no way to know what resolution the floors happen at). We assume that the default drive angles set by the hardware are good enough.Signed-off-by Alexandru M Stan <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>--- Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt | 13 +++++++++++++ 1 file changed, 13 insertions(+)Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt +13
-0@@ -14,6 +14,19 @@ Required Properties: before RK3288 - "rockchip,rk3288-dw-mshc": for Rockchip RK3288 +Optional Properties: +* clocks: from common clock binding: if ciu_drive and ciu_sample are + specified in clock-names, should contain handles to these clocks. + +* clock-names: Apart from the clock-names described in synopsys-dw-mshc.txt + two more clocks "ciu-drive" and "ciu-sample" are supported. They are used + to control the clock phases, "ciu-sample" is required for tuning high- + speed modes. + +* rockchip,default-sample-phase: The default phase to set ciu_sample at + probing, low speeds or in case where all phases work at tuning time. + If not specified 0 deg will be used. + Example: rkdwmmc0 at 12200000 {
Heiko Stuebner
maintainerPATCH v2 6/8 mmc: dw_mmc-rockchip: MMC tuning with the clock phase framework
In reply to Heiko StuebnerFrom: Alexandru M Stan <[email protected]>
This algorithm will try 1 degree increments, since there's no way to tell what resolution the underlying phase code uses. As an added bonus, doing many tunings yields better results since some tests are run more than once (ex: if the underlying driver uses 45 degree increments, the tuning code will try the same angle more than once).
It will then construct a list of good phase ranges (even ranges that cross 360/0), will pick the biggest range then it will set the sample_clk to the middle of that range.
We do not touch ciu_drive (and by extension define default-drive-phase). Drive phase is mostly used to define minimum hold times, while one could write some code to determine what phase meets the minimum hold time (ex 10 degrees) this will not work with the current clock phase framework (which floors angles, so we'll get 0 deg, and there's no way to know what resolution the floors happen at). We assume that the default drive angles set by the hardware are good enough.
If a device has device specific code (like exynos) then that will still take precedence, otherwise this new code will execute. If the device wants to tune, but has no sample_clk defined we'll return EIO with an error message.
--- drivers/mmc/host/dw_mmc-rockchip.c | 162 +++++++++++++++++++++++++++++++++++++ 1 file changed, 162 insertions(+)
drivers/mmc/host/dw_mmc-rockchip.c +162 -0
@@ -13,12 +13,19 @@#include <linux/mmc/host.h> #include <linux/mmc/dw_mmc.h> #include <linux/of_address.h> +#include <linux/slab.h> #include "dw_mmc.h" #include "dw_mmc-pltfm.h" #define RK3288_CLKGEN_DIV 2 +struct dw_mci_rockchip_priv_data { +struct clk *drv_clk; +struct clk *sample_clk; +int default_sample_phase; +}; +static void dw_mci_rockchip_prepare_command (struct dw_mci *host,u32 *cmdr) { *cmdr |=SDMMC_CMD_USE_HOLD_REG ;
@@ -33,6 +40,7 @@ static int dw_mci_rk3288_setup_clock(struct dw_mci *host)static void dw_mci_rk3288_set_ios (struct dw_mci *host,struct mmc_ios *ios) { +struct dw_mci_rockchip_priv_data *priv = host->priv;int ret;unsigned int cclkin;u32 bus_hz;
@@ -66,6 +74,158 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)/* force dw_mci_setup_bus() */ host->current_speed =0 ; } + +/* Make sure we use phases which we can enumerate with */ +if (!IS_ERR (priv->sample_clk)) +clk_set_phase (priv->sample_clk, priv->default_sample_phase); +} + +#define NUM_PHASES 360 +#define TUNING_ITERATION_TO_PHASE (i) (DIV_ROUND_UP((i) * 360, NUM_PHASES)) + +static int dw_mci_rk3288_execute_tuning (struct dw_mci_slot *slot) +{ +struct dw_mci *host = slot->host; +struct dw_mci_rockchip_priv_data *priv = host->priv; +struct mmc_host *mmc = slot->mmc; +int ret =0 ; +int i; +bool v, prev_v =0 , first_v; +struct range_t { +int start; +int end;/* inclusive */ + }; +struct range_t *ranges; +unsigned int range_count =0 ; +int longest_range_len =-1 ; +int longest_range =-1 ; +int middle_phase; + +if (IS_ERR (priv->sample_clk)) { +dev_err (host->dev,"Tuning clock (sample_clk) not defined.\n" ); +return -EIO ; + } + + ranges =kmalloc_array (NUM_PHASES /2 +1 ,sizeof (*ranges),GFP_KERNEL ); +if (!ranges) +return -ENOMEM ; + +/* Try each phase and extract good ranges */ +for (i =0 ; i <NUM_PHASES ; ) { +clk_set_phase (priv->sample_clk,TUNING_ITERATION_TO_PHASE (i)); + + v = !mmc_send_tuning (mmc); + +if (i ==0 ) + first_v = v; + +if ((!prev_v) && v) { + range_count++; + ranges[range_count-1 ].start = i; + } +if (v) { + ranges[range_count-1 ].end = i; + i++; + }else if (i ==NUM_PHASES -1 ) { +/* No extra skipping rules if we're at the end */ + i++; + }else { +/* +* No need to check too close to an invalid +* one since testing bad phases is slow. Skip +* 20 degrees. +*/ + i +=DIV_ROUND_UP (20 *NUM_PHASES ,360 ); + +/* Always test the last one */ +if (i >=NUM_PHASES ) + i =NUM_PHASES -1 ; + } + + prev_v = v; + } + +if (range_count ==0 ) { +dev_warn (host->dev,"All phases bad!" ); + ret = -EIO ; + goto free; + } + +/* wrap around case, merge the end points */ +if ((range_count >1 ) && first_v && v) { + ranges[0 ].start = ranges[range_count-1 ].start; + range_count--; + } + +if (ranges[0 ].start ==0 && ranges[0 ].end ==NUM_PHASES -1 ) { +clk_set_phase (priv->sample_clk, priv->default_sample_phase); +dev_info (host->dev,"All phases work, using default phase %d." , + priv->default_sample_phase); + goto free; + } + +/* Find the longest range */ +for (i =0 ; i < range_count; i++) { +int len = (ranges[i].end - ranges[i].start +1 ); + +if (len <0 ) + len +=NUM_PHASES ; + +if (longest_range_len < len) { + longest_range_len = len; + longest_range = i; + } + +dev_dbg (host->dev,"Good phase range %d-%d (%d len)\n" , +TUNING_ITERATION_TO_PHASE (ranges[i].start), +TUNING_ITERATION_TO_PHASE (ranges[i].end), + len + ); + } + +dev_dbg (host->dev,"Best phase range %d-%d (%d len)\n" , +TUNING_ITERATION_TO_PHASE (ranges[longest_range].start), +TUNING_ITERATION_TO_PHASE (ranges[longest_range].end), + longest_range_len + ); + + middle_phase = ranges[longest_range].start + longest_range_len /2 ; + middle_phase %=NUM_PHASES ; +dev_info (host->dev,"Successfully tuned phase to %d\n" , +TUNING_ITERATION_TO_PHASE (middle_phase)); + +clk_set_phase (priv->sample_clk, +TUNING_ITERATION_TO_PHASE (middle_phase)); + +free: +kfree (ranges); +return ret; +} + +static int dw_mci_rk3288_parse_dt (struct dw_mci *host) +{ +struct device_node *np = host->dev->of_node; +struct dw_mci_rockchip_priv_data *priv; + + priv =devm_kzalloc (host->dev,sizeof (*priv),GFP_KERNEL ); +if (!priv) +return -ENOMEM ; + +if (of_property_read_u32 (np,"rockchip,default-sample-phase" , + &priv->default_sample_phase)) + priv->default_sample_phase =0 ; + + priv->drv_clk =devm_clk_get (host->dev,"ciu-drive" ); +if (IS_ERR (priv->drv_clk)) +dev_dbg (host->dev,"ciu_drv not available\n" ); + + priv->sample_clk =devm_clk_get (host->dev,"ciu-sample" ); +if (IS_ERR (priv->sample_clk)) +dev_dbg (host->dev,"ciu_sample not available\n" ); + + host->priv = priv; + +return 0 ; }static int dw_mci_rockchip_init (struct dw_mci *host)
@@ -95,6 +255,8 @@ static const struct dw_mci_drv_data rk3288_drv_data = { .caps = dw_mci_rk3288_dwmmc_caps, .prepare_command = dw_mci_rockchip_prepare_command, .set_ios = dw_mci_rk3288_set_ios, + .execute_tuning = dw_mci_rk3288_execute_tuning, + .parse_dt = dw_mci_rk3288_parse_dt, .setup_clock = dw_mci_rk3288_setup_clock, .init = dw_mci_rockchip_init, };
Signature · 1 line
2.5.1
Jaehoon Chung
maintainerPATCH v2 6/8 mmc: dw_mmc-rockchip: MMC tuning with the clock phase framework
Hi, Heiko.
Best Regards,
Jaehoon Chung
Quoted 232 lines · Heiko Stuebner
From: Alexandru M Stan <[email protected]>
This algorithm will try 1 degree increments, since there's no way to tell what resolution the underlying phase code uses. As an added bonus, doing many tunings yields better results since some tests are run more than once (ex: if the underlying driver uses 45 degree increments, the tuning code will try the same angle more than once).
It will then construct a list of good phase ranges (even ranges that cross 360/0), will pick the biggest range then it will set the
sample_clkto the middle of that range.We do not touch
ciu_drive(and by extension define default-drive-phase). Drive phase is mostly used to define minimum hold times, while one could write some code to determine what phase meets the minimum hold time (ex 10 degrees) this will not work with the current clock phase framework (which floors angles, so we'll get 0 deg, and there's no way to know what resolution the floors happen at). We assume that the default drive angles set by the hardware are good enough.If a device has device specific code (like exynos) then that will still take precedence, otherwise this new code will execute. If the device wants to tune, but has no
sample_clkdefined we'll return EIO with an error message.Signed-off-by Alexandru M Stan <[email protected]>Signed-off-by Heiko Stuebner <[email protected]>--- drivers/mmc/host/dw_mmc-rockchip.c | 162 +++++++++++++++++++++++++++++++++++++ 1 file changed, 162 insertions(+)drivers/mmc/host/dw_mmc-rockchip.c +162
-0@@ -13,12 +13,19 @@#include <linux/mmc/host.h> #include <linux/mmc/dw_mmc.h> #include <linux/of_address.h> +#include <linux/slab.h> #include "dw_mmc.h" #include "dw_mmc-pltfm.h" #define RK3288_CLKGEN_DIV 2 +struct dw_mci_rockchip_priv_data { +struct clk *drv_clk; +struct clk *sample_clk; +int default_sample_phase; +}; +static void dw_mci_rockchip_prepare_command (struct dw_mci *host,u32 *cmdr) { *cmdr |=SDMMC_CMD_USE_HOLD_REG ;@@ -33,6 +40,7 @@ static int dw_mci_rk3288_setup_clock(struct dw_mci *host)static void dw_mci_rk3288_set_ios (struct dw_mci *host,struct mmc_ios *ios) { +struct dw_mci_rockchip_priv_data *priv = host->priv;int ret;unsigned int cclkin;u32 bus_hz;@@ -66,6 +74,158 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)/* force dw_mci_setup_bus() */ host->current_speed =0 ; } + +/* Make sure we use phases which we can enumerate with */ +if (!IS_ERR (priv->sample_clk)) +clk_set_phase (priv->sample_clk, priv->default_sample_phase); +} + +#define NUM_PHASES 360 +#define TUNING_ITERATION_TO_PHASE (i) (DIV_ROUND_UP((i) * 360, NUM_PHASES)) + +static int dw_mci_rk3288_execute_tuning (struct dw_mci_slot *slot) +{ +struct dw_mci *host = slot->host; +struct dw_mci_rockchip_priv_data *priv = host->priv; +struct mmc_host *mmc = slot->mmc; +int ret =0 ; +int i; +bool v, prev_v =0 , first_v; +struct range_t { +int start; +int end;/* inclusive */ + }; +struct range_t *ranges; +unsigned int range_count =0 ; +int longest_range_len =-1 ; +int longest_range =-1 ; +int middle_phase; + +if (IS_ERR (priv->sample_clk)) { +dev_err (host->dev,"Tuning clock (sample_clk) not defined.\n" ); +return -EIO ; + } + + ranges =kmalloc_array (NUM_PHASES /2 +1 ,sizeof (*ranges),GFP_KERNEL ); +if (!ranges) +return -ENOMEM ; + +/* Try each phase and extract good ranges */ +for (i =0 ; i <NUM_PHASES ; ) { +clk_set_phase (priv->sample_clk,TUNING_ITERATION_TO_PHASE (i)); + + v = !mmc_send_tuning (mmc); + +if (i ==0 ) + first_v = v; + +if ((!prev_v) && v) { + range_count++; + ranges[range_count-1 ].start = i; + } +if (v) { + ranges[range_count-1 ].end = i; + i++; + }else if (i ==NUM_PHASES -1 ) { +/* No extra skipping rules if we're at the end */ + i++; + }else { +/* +* No need to check too close to an invalid +* one since testing bad phases is slow. Skip +* 20 degrees. +*/ + i +=DIV_ROUND_UP (20 *NUM_PHASES ,360 ); + +/* Always test the last one */ +if (i >=NUM_PHASES ) + i =NUM_PHASES -1 ; + } + + prev_v = v; + } + +if (range_count ==0 ) { +dev_warn (host->dev,"All phases bad!" ); + ret = -EIO ; + goto free; + } + +/* wrap around case, merge the end points */ +if ((range_count >1 ) && first_v && v) { + ranges[0 ].start = ranges[range_count-1 ].start; + range_count--; + } + +if (ranges[0 ].start ==0 && ranges[0 ].end ==NUM_PHASES -1 ) { +clk_set_phase (priv->sample_clk, priv->default_sample_phase); +dev_info (host->dev,"All phases work, using default phase %d." , + priv->default_sample_phase); + goto free; + } + +/* Find the longest range */ +for (i =0 ; i < range_count; i++) { +int len = (ranges[i].end - ranges[i].start +1 ); + +if (len <0 ) + len +=NUM_PHASES ; + +if (longest_range_len < len) { + longest_range_len = len; + longest_range = i; + } + +dev_dbg (host->dev,"Good phase range %d-%d (%d len)\n" , +TUNING_ITERATION_TO_PHASE (ranges[i].start), +TUNING_ITERATION_TO_PHASE (ranges[i].end), + len + ); + } + +dev_dbg (host->dev,"Best phase range %d-%d (%d len)\n" , +TUNING_ITERATION_TO_PHASE (ranges[longest_range].start), +TUNING_ITERATION_TO_PHASE (ranges[longest_range].end), + longest_range_len + ); + + middle_phase = ranges[longest_range].start + longest_range_len /2 ; + middle_phase %=NUM_PHASES ; +dev_info (host->dev,"Successfully tuned phase to %d\n" , +TUNING_ITERATION_TO_PHASE (middle_phase)); + +clk_set_phase (priv->sample_clk, +TUNING_ITERATION_TO_PHASE (middle_phase)); + +free: +kfree (ranges); +return ret; +} + +static int dw_mci_rk3288_parse_dt (struct dw_mci *host) +{ +struct device_node *np = host->dev->of_node; +struct dw_mci_rockchip_priv_data *priv; + + priv =devm_kzalloc (host->dev,sizeof (*priv),GFP_KERNEL ); +if (!priv) +return -ENOMEM ; + +if (of_property_read_u32 (np,"rockchip,default-sample-phase" , + &priv->default_sample_phase)) + priv->default_sample_phase =0 ; + + priv->drv_clk =devm_clk_get (host->dev,"ciu-drive" ); +if (IS_ERR (priv->drv_clk)) +dev_dbg (host->dev,"ciu_drv not available\n" ); + + priv->sample_clk =devm_clk_get (host->dev,"ciu-sample" ); +if (IS_ERR (priv->sample_clk)) +dev_dbg (host->dev,"ciu_sample not available\n" ); + + host->priv = priv; + +return 0 ; }static int dw_mci_rockchip_init (struct dw_mci *host)@@ -95,6 +255,8 @@ static const struct dw_mci_drv_data rk3288_drv_data = { .caps = dw_mci_rk3288_dwmmc_caps, .prepare_command = dw_mci_rockchip_prepare_command, .set_ios = dw_mci_rk3288_set_ios, + .execute_tuning = dw_mci_rk3288_execute_tuning, + .parse_dt = dw_mci_rk3288_parse_dt, .setup_clock = dw_mci_rk3288_setup_clock, .init = dw_mci_rockchip_init, };
Heiko Stuebner
maintainerPATCH v2 7/8 ARM: dts: rockchip: Add drive/sample clocks for rk3288 dw_mmc devices
In reply to Heiko StuebnerFrom: Alexandru M Stan <[email protected]>
The drive/sample clocks can be phase shifted. The drive clock could be used in a future patch to adjust hold times. The sample clock is used for tuning.
--- arch/arm/boot/dts/rk3288.dtsi | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
arch/arm/boot/dts/rk3288.dtsi +12 -8
@@ -225,8 +225,9 @@ sdmmc: dwmmc at ff0c0000 { compatible = "rockchip,rk3288-dw-mshc"; clock-freq-min-max = <400000 150000000>; - clocks = <&cru HCLK_SDMMC>, <&cru SCLK_SDMMC>; - clock-names = "biu", "ciu"; + clocks = <&cru HCLK_SDMMC>, <&cru SCLK_SDMMC>, + <&cru SCLK_SDMMC_DRV>, <&cru SCLK_SDMMC_SAMPLE>; + clock-names = "biu", "ciu", "ciu-drive", "ciu-sample"; fifo-depth = <0x100>; interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; reg = <0xff0c0000 0x4000>;
@@ -236,8 +237,9 @@ sdio0: dwmmc at ff0d0000 { compatible = "rockchip,rk3288-dw-mshc"; clock-freq-min-max = <400000 150000000>; - clocks = <&cru HCLK_SDIO0>, <&cru SCLK_SDIO0>; - clock-names = "biu", "ciu"; + clocks = <&cru HCLK_SDIO0>, <&cru SCLK_SDIO0>, + <&cru SCLK_SDIO0_DRV>, <&cru SCLK_SDIO0_SAMPLE>; + clock-names = "biu", "ciu", "ciu-drive", "ciu-sample"; fifo-depth = <0x100>; interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>; reg = <0xff0d0000 0x4000>;
@@ -247,8 +249,9 @@ sdio1: dwmmc at ff0e0000 { compatible = "rockchip,rk3288-dw-mshc"; clock-freq-min-max = <400000 150000000>; - clocks = <&cru HCLK_SDIO1>, <&cru SCLK_SDIO1>; - clock-names = "biu", "ciu"; + clocks = <&cru HCLK_SDIO1>, <&cru SCLK_SDIO1>, + <&cru SCLK_SDIO1_DRV>, <&cru SCLK_SDIO1_SAMPLE>; + clock-names = "biu", "ciu", "ciu-drive", "ciu-sample"; fifo-depth = <0x100>; interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>; reg = <0xff0e0000 0x4000>;
@@ -258,8 +261,9 @@ emmc: dwmmc at ff0f0000 { compatible = "rockchip,rk3288-dw-mshc"; clock-freq-min-max = <400000 150000000>; - clocks = <&cru HCLK_EMMC>, <&cru SCLK_EMMC>; - clock-names = "biu", "ciu"; + clocks = <&cru HCLK_EMMC>, <&cru SCLK_EMMC>, + <&cru SCLK_EMMC_DRV>, <&cru SCLK_EMMC_SAMPLE>; + clock-names = "biu", "ciu", "ciu-drive", "ciu-sample"; fifo-depth = <0x100>; interrupts = <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>; reg = <0xff0f0000 0x4000>;
Signature · 1 line
2.5.1
Heiko Stuebner
maintainerPATCH v2 8/8 ARM: dts: rockchip: add tuning related settings to veyron devices
In reply to Heiko StuebnerThis allows the tuning code to run and use higher speeds on capable cards.
--- arch/arm/boot/dts/rk3288-veyron-sdmmc.dtsi | 7 ++++++- arch/arm/boot/dts/rk3288-veyron.dtsi | 6 ++++++ 2 files changed, 12 insertions(+), 1 deletion(-)
arch/arm/boot/dts/rk3288-veyron-sdmmc.dtsi +6 -1
@@ -90,7 +90,7 @@ regulators { vccio_sd: LDO_REG4 { regulator-name = "vccio_sd"; - regulator-min-microvolt = <3300000>; + regulator-min-microvolt = <1800000>; regulator-max-microvolt = <3300000>; regulator-state-mem { regulator-off-in-suspend;
@@ -116,7 +116,12 @@ cap-sd-highspeed; card-detect-delay = <200>; cd-gpios = <&gpio7 5 GPIO_ACTIVE_LOW>; + rockchip,default-sample-phase = <90>; num-slots = <1>; + sd-uhs-sdr12; + sd-uhs-sdr25; + sd-uhs-sdr50; + sd-uhs-sdr104; vmmc-supply = <&vcc33_sd>; vqmmc-supply = <&vccio_sd>; };
arch/arm/boot/dts/rk3288-veyron.dtsi +6 -0
@@ -196,7 +196,9 @@ broken-cd; bus-width = <8>; cap-mmc-highspeed; + rockchip,default-sample-phase = <158>; disable-wp; + mmc-hs200-1_8v; mmc-pwrseq = <&emmc_pwrseq>; non-removable; num-slots = <1>;
@@ -413,6 +415,10 @@ num-slots = <1>; pinctrl-names = "default"; pinctrl-0 = <&sdio0_clk &sdio0_cmd &sdio0_bus4>; + sd-uhs-sdr12; + sd-uhs-sdr25; + sd-uhs-sdr50; + sd-uhs-sdr104; vmmc-supply = <&vcc33_sys>; vqmmc-supply = <&vcc18_wl>; };
Signature · 1 line
2.5.1
Ulf Hansson
Appliedv2 cover mmc: dw_mmc-rockchip: allow tuning using the clk-phase api
In reply to Heiko StuebnerQuoted 57 lines · Heiko Stuebner
This series resurrects and adapts some individual patches whose sum enable the
dw_mmchosts on Rockchip socs to tune clock phases using the generic phase api (rk3288 and following have this capability).The changes to the original mmc-phase clocks are expanded by further findings resulting from devices being used in the field.
Similarly the regulator handling changes do use the brand new
regulator_set_voltage_tripletcall to allow specifying lower and upper limits. One possible point of discussion are the two voltage ranges that are tried for the 3.3V signal level. Trying to stay near vmmc at first and only then opening the range to the full 2.7-3.6V.This mainly circumvents a shortcoming of the regulator voltage setting, in that even with
regulator_set_voltage_tripletthe regulator framework will take the lowest possible voltage when the possible voltages are below the target voltage. While it may be ideal to solve this on the regulator side, I'm not seeing this appearing in the short term, mainly because all regulator parts (including regulator drivers) are keyed to selecting the lowest voltage from the range, while on the mmc side we know which voltages may work and trying this in two steps does not create to much overhead, as unsupported voltages are already filtered out by theregulator_is_voltage_selectedcalls.changes since v1:
- address comment from Jaehoon Chung and keep this local to Rockchip for the time being
- address comments from Ulf and Doug in making it more explicit that the two-step 3.3V voltage setting essentially works around a limitation ofregulator_set_voltage_tripletAlexandru M Stan (3):
mmc: dw_mmc-rockchip: dt-binding: Add tuning related things
mmc:dw_mmc: Generic MMC tuning with the clock phase framework
ARM: dts: rockchip: Add drive/sample clocks for rk3288dw_mmcdevicesDouglas Anderson (4):
clk: rockchip: Allow more precision for some mmc clock phases
clk: rockchip: Make calculations use rounding
mmc: core: Addmmc_regulator_set_vqmmc()
mmc:dw_mmc: Usemmc_regulator_set_vqmmcin
start_signal_voltage_switchHeiko Stuebner (1):
ARM: dts: rockchip: add tuning related settings to veyron devices.../devicetree/bindings/mmc/rockchip-dw-mshc.txt | 13 ++ arch/arm/boot/dts/rk3288-veyron-sdmmc.dtsi | 7 +- arch/arm/boot/dts/rk3288-veyron.dtsi | 6 + arch/arm/boot/dts/rk3288.dtsi | 20 ++- drivers/clk/rockchip/clk-mmc-phase.c | 54 ++++--- drivers/mmc/core/core.c | 74 ++++++++++ drivers/mmc/host/dw_mmc-rockchip.c | 162 +++++++++++++++++++++ drivers/mmc/host/dw_mmc.c | 17 +-- include/linux/mmc/host.h | 7 + 9 files changed, 321 insertions(+), 39 deletions(-)
Quoted 3 lines
--
2.5.1
Thanks, applied for next!
Kind regards
Uffe
v2 cover mmc: dw_mmc-rockchip: allow tuning using the clk-phase api
Quoted 58 lines · Ulf Hansson
Quoted 57 lines · Heiko Stuebner
This series resurrects and adapts some individual patches whose sum enable the
dw_mmchosts on Rockchip socs to tune clock phases using the generic phase api (rk3288 and following have this capability).The changes to the original mmc-phase clocks are expanded by further findings resulting from devices being used in the field.
Similarly the regulator handling changes do use the brand new
regulator_set_voltage_tripletcall to allow specifying lower and upper limits. One possible point of discussion are the two voltage ranges that are tried for the 3.3V signal level. Trying to stay near vmmc at first and only then opening the range to the full 2.7-3.6V.This mainly circumvents a shortcoming of the regulator voltage setting, in that even with
regulator_set_voltage_tripletthe regulator framework will take the lowest possible voltage when the possible voltages are below the target voltage. While it may be ideal to solve this on the regulator side, I'm not seeing this appearing in the short term, mainly because all regulator parts (including regulator drivers) are keyed to selecting the lowest voltage from the range, while on the mmc side we know which voltages may work and trying this in two steps does not create to much overhead, as unsupported voltages are already filtered out by theregulator_is_voltage_selectedcalls.changes since v1:
- address comment from Jaehoon Chung and keep this local to Rockchip for the time being
- address comments from Ulf and Doug in making it more explicit that the two-step 3.3V voltage setting essentially works around a limitation ofregulator_set_voltage_tripletAlexandru M Stan (3):
mmc: dw_mmc-rockchip: dt-binding: Add tuning related things
mmc:dw_mmc: Generic MMC tuning with the clock phase framework
ARM: dts: rockchip: Add drive/sample clocks for rk3288dw_mmcdevicesDouglas Anderson (4):
clk: rockchip: Allow more precision for some mmc clock phases
clk: rockchip: Make calculations use rounding
mmc: core: Addmmc_regulator_set_vqmmc()
mmc:dw_mmc: Usemmc_regulator_set_vqmmcin
start_signal_voltage_switchHeiko Stuebner (1):
ARM: dts: rockchip: add tuning related settings to veyron devices.../devicetree/bindings/mmc/rockchip-dw-mshc.txt | 13 ++ arch/arm/boot/dts/rk3288-veyron-sdmmc.dtsi | 7 +- arch/arm/boot/dts/rk3288-veyron.dtsi | 6 + arch/arm/boot/dts/rk3288.dtsi | 20 ++- drivers/clk/rockchip/clk-mmc-phase.c | 54 ++++--- drivers/mmc/core/core.c | 74 ++++++++++ drivers/mmc/host/dw_mmc-rockchip.c | 162 +++++++++++++++++++++ drivers/mmc/host/dw_mmc.c | 17 +-- include/linux/mmc/host.h | 7 + 9 files changed, 321 insertions(+), 39 deletions(-)
Quoted 8 lines
Quoted 3 lines
--
2.5.1Thanks, applied for next!
Kind regards
Uffe
Dough, Heiko,
This patchset seems to be causing a boot regression for exynos5800-peach-pi [1].
Apparently, the vmmc regulator doesn't exist for one of the controller but vqmmc is. This leads to the following NULL pointer exception.
.....
[ 5.672281] dwmmc_exynos 12210000.mmc: No vmmc regulator found [ 5.678234] dwmmc_exynos 12210000.mmc: allocated mmc-pwrseq [ 5.683890] Unable to handle kernel NULL pointer dereference at virtual address 0000001d [ 5.691727] pgd = c0204000 [ 5.694296] [0000001d] *pgd=00000000 [ 5.697853] Internal error: Oops: 17 [#1] SMP ARM [ 5.702538] Modules linked in: snd_soc_max98090 exynosdrm snd_soc_core rtc_max77802 max77802 spi_s3c64xx snd_compress snd_pcm_dmaengine snd_pcm pwm_samsung snd_timer clk_max77802 snd soundcore rtc_s3c exynos_adc phy_exynos_usb2 ohci_exynos [ 5.723702] CPU: 6 PID: 83 Comm: kworker/u16:1 Not tainted 4.3.0-rc4-00051-gaf00b2580295 #1 [ 5.732053] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [ 5.738120] Workqueue: deferwq deferred_probe_work_func [ 5.743305] task: ed7297c0 ti: ed780000 task.ti: ed780000 [ 5.746177] ntc-thermistor 12d10000.adc:ncp15wb473 at 3: Thermistor type: ncp15wb473 successfully probed. [ 5.746514] ntc-thermistor 12d10000.adc:ncp15wb473 at 4: Thermistor type: ncp15wb473 successfully probed. [ 5.746913] ntc-thermistor 12d10000.adc:ncp15wb473 at 5: Thermistor type: ncp15wb473 successfully probed. [ 5.747150] ntc-thermistor 12d10000.adc:ncp15wb473 at 6: Thermistor type: ncp15wb473 successfully probed. [ 5.785853] PC is at regulator_get_voltage+0x8/0x34 [ 5.790681] LR is at mmc_regulator_set_vqmmc+0x40/0xe8 [ 5.795798] pc : [<c0553bcc>] lr : [<c080b820>] psr: 80000013 [ 5.795798] sp : ed781dd0 ip : ec02e5f8 fp : 00000000 [ 5.807269] r10: 00000000 r9 : ec02e700 r8 : 00000000 [ 5.812450] r7 : 00000000 r6 : 00000001 r5 : 00000000 r4 : ffffffed [ 5.818958] r3 : ec02e400 r2 : 00000000 r1 : ec02e5f8 r0 : ffffffed [ 5.825466] Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none [ 5.832579] Control: 10c5387d Table: 6c18006a DAC: 00000051 [ 5.838303] Process kworker/u16:1 (pid: 83, stack limit = 0xed780220) [ 5.844722] Stack: (0xed781dd0 to 0xed782000) [ 5.849061] 1dc0: ec02e400 ec02e400 00000000 c080b820 [ 5.857240] 1de0: ec57da10 00000000 00000001 00000000 00000000 c08330e0 c0833074 ec02e400 [ 5.865395] 1e00: ec02e5f8 c080cb58 00000000 ec02e400 00000000 ec02e400 00000000 c080d83c [ 5.873550] 1e20: ec02e400 c080e8b8 00000001 ec57da10 00000000 c083460c 00000040 c0cbe7e0 [ 5.881704] 1e40: ec57da10 ed223bc0 ed23d410 00000000 c0ac26a8 17d78400 edfc15b4 ec57da10 [ 5.889858] 1e60: ec57da10 fffffffe ed23d410 fffffdfb c0f3a920 0000000b ed723080 00000001 [ 5.898013] 1e80: 00000001 c0623668 c0623620 ed23d410 c0fbde24 00000000 c0f3a920 c0622098 [ 5.906168] 1ea0: 00000000 ed781ed0 c0622344 00000001 00000000 c0620730 ed08b770 ed3836b8 [ 5.914323] 1ec0: ed23d410 ed23d444 c0f1a910 c0621e2c ed23d410 00000001 00000002 ed23d410 [ 5.922477] 1ee0: ed23d410 c0f1a910 ed7a3800 c06215a8 ed23d410 c0f1a8b0 c0f1a898 c0621980 [ 5.930631] 1f00: ed723080 c0f1a8cc ed01b800 c025ed00 ed781f5c ed01b800 00000001 ed01b814 [ 5.938785] 1f20: 00000001 ed01b800 ed723098 00000088 ed723080 ed01b800 00000001 c025ef60 [ 5.946940] 1f40: ed7297c0 ed762000 00000000 ed723080 c025ef38 00000000 00000000 00000000 [ 5.955094] 1f60: 00000000 c02640a0 ed083dc0 00000000 00000000 ed723080 00000000 00000000 [ 5.963249] 1f80: ed781f80 ed781f80 00000000 00000000 ed781f90 ed781f90 ed781fac ed762000 [ 5.971403] 1fa0: c0263fc8 00000000 00000000 c0210b38 00000000 00000000 00000000 00000000 [ 5.979557] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 5.987712] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000 [ 5.995883] [<c0553bcc>] (regulator_get_voltage) from [<c080b820>] (mmc_regulator_set_vqmmc+0x40/0xe8) [ 6.005158] [<c080b820>] (mmc_regulator_set_vqmmc) from [<c08330e0>] (dw_mci_switch_voltage+0x6c/0x84) [ 6.014438] [<c08330e0>] (dw_mci_switch_voltage) from [<c080cb58>] (mmc_power_up+0x68/0x104) [ 6.022852] [<c080cb58>] (mmc_power_up) from [<c080d83c>] (mmc_start_host+0x50/0x80) [ 6.030569] [<c080d83c>] (mmc_start_host) from [<c080e8b8>] (mmc_add_host+0x58/0x7c) [ 6.038289] [<c080e8b8>] (mmc_add_host) from [<c083460c>] (dw_mci_probe+0x5d8/0xc68) [ 6.046013] [<c083460c>] (dw_mci_probe) from [<c0623668>] (platform_drv_probe+0x48/0xa4) [ 6.054082] [<c0623668>] (platform_drv_probe) from [<c0622098>] (driver_probe_device+0x1e0/0x2a0) [ 6.062930] [<c0622098>] (driver_probe_device) from [<c0620730>] (bus_for_each_drv+0x44/0x8c) [ 6.071434] [<c0620730>] (bus_for_each_drv) from [<c0621e2c>] (__device_attach+0xa0/0x104) [ 6.079670] [<c0621e2c>] (__device_attach) from [<c06215a8>] (bus_probe_device+0x84/0x8c) [ 6.087825] [<c06215a8>] (bus_probe_device) from [<c0621980>] (deferred_probe_work_func+0x5c/0x88) [ 6.096763] [<c0621980>] (deferred_probe_work_func) from [<c025ed00>] (process_one_work+0x134/0x338) [ 6.105871] [<c025ed00>] (process_one_work) from [<c025ef60>] (worker_thread+0x28/0x4cc) [ 6.113938] [<c025ef60>] (worker_thread) from [<c02640a0>] (kthread+0xd8/0xf4) [ 6.121139] [<c02640a0>] (kthread) from [<c0210b38>] (ret_from_fork+0x14/0x3c) [ 6.128333] Code: e5843010 eaffffdb e92d4038 e1a04000 (e5900030) [ 6.134460] ---[ end trace ee25ca3d5cf9ee44 ]---
....
Looking into the new mmc_regulator_set_vqmmc() API, I believe it's quite obvious where the error is, as there's a missing validation of "mmc->supply.vmmc" before trying regulator_get_voltage() for it.
I think this is the proper solution:
- Instead of using the "mmc->supply.vmmc" to fetch the current voltage level, let's use "mmc->ios.vdd" (we need to translate the bit into a voltage levels), which thus make the new the API independent of valid regulator for vmmc.
Until we fixed that, I am dropping patch3 and onwards.
Kind regards
Uffe
v2 cover mmc: dw_mmc-rockchip: allow tuning using the clk-phase api
Hi,
Quoted 6 lines · Ulf Hansson
Dough, Heiko,
This patchset seems to be causing a boot regression for exynos5800-peach-pi [1].
Apparently, the vmmc regulator doesn't exist for one of the controller but vqmmc is. This leads to the following NULL pointer exception.
Ugh. Hrm, it didn't used to be possible for vmmc to be NULL. :-/ It changed in v3.16:
4d1f52f mmc: core: Improve support for deferred regulators
...and I didn't notice it (most of my development is done on 3.14). :(
Is it sensible to have vqmmc without vmmc. One option would be to change mmc_regulator_get_supply() to not even bother trying to get vqmmc if vmmc is not there.
Another option would be to just totally fail mmc_regulator_set_vqmmc() always if vmmc is an error. ...or assume that any bogus vmmc (either vmmc is an error or regulator_get_voltage(vmmc) returns an error) is 3.3V?
I guess the safest is to assume that bogus vmmc means 3.3V...
-Doug
v2 cover mmc: dw_mmc-rockchip: allow tuning using the clk-phase api
Hi,
Quoted 22 lines · Doug Anderson
Hi,
Quoted 6 lines · Ulf Hansson
Dough, Heiko,
This patchset seems to be causing a boot regression for exynos5800-peach-pi [1].
Apparently, the vmmc regulator doesn't exist for one of the controller but vqmmc is. This leads to the following NULL pointer exception.
Ugh. Hrm, it didn't used to be possible for vmmc to be NULL. :-/ It changed in v3.16:
4d1f52f mmc: core: Improve support for deferred regulators
...and I didn't notice it (most of my development is done on 3.14). :(
Is it sensible to have vqmmc without vmmc. One option would be to change
mmc_regulator_get_supply()to not even bother trying to get vqmmc if vmmc is not there.
Quoted 7 lines
Another option would be to just totally fail
mmc_regulator_set_vqmmc()always if vmmc is an error. ...or assume that any bogus vmmc (either vmmc is an error orregulator_get_voltage(vmmc)returns an error) is 3.3V?I guess the safest is to assume that bogus vmmc means 3.3V...
Ah, doh. Just noticed Ulf's suggestion all the way at the bottom of the crash info, which I somehow missed before. Yes, let's do that.
Heiko: are you going to re-post?
-Doug
Heiko Stuebner
maintainerv2 cover mmc: dw_mmc-rockchip: allow tuning using the clk-phase api
Am Freitag, 9. Oktober 2015, 13:11:21 schrieb Doug Anderson:
Quoted 3 lines
Hi,
On Fri, Oct 9, 2015 at 10:19 AM, Doug Anderson <[email protected]>
Quoted 3 lines · wrote:
Quoted 3 lines
Hi,
On Fri, Oct 9, 2015 at 1:10 AM, Ulf Hansson <[email protected]>
Quoted 26 lines · wrote:
Quoted 26 lines
Quoted 7 lines
Dough, Heiko,
This patchset seems to be causing a boot regression for exynos5800-peach-pi [1].
Apparently, the vmmc regulator doesn't exist for one of the controller but vqmmc is. This leads to the following NULL pointer exception.
Ugh. Hrm, it didn't used to be possible for vmmc to be NULL. :-/ It
changed in v3.16:
4d1f52f mmc: core: Improve support for deferred regulators...and I didn't notice it (most of my development is done on 3.14). :(
Is it sensible to have vqmmc without vmmc. One option would be to change
mmc_regulator_get_supply()to not even bother trying to get vqmmc if vmmc is not there.Another option would be to just totally fail
mmc_regulator_set_vqmmc()always if vmmc is an error. ...or assume that any bogus vmmc (either vmmc is an error orregulator_get_voltage(vmmc)returns an error) is 3.3V?
Quoted 5 lines
Quoted 1 line
I guess the safest is to assume that bogus vmmc means 3.3V...
Ah, doh. Just noticed Ulf's suggestion all the way at the bottom of the crash info, which I somehow missed before. Yes, let's do that.
Heiko: are you going to re-post?
definitly :-) .
I was just travelling back from ELCE today, otherwise I would've done that directly today. So I'm hoping to get that done tomorrow.
Heiko