| Message ID | 1408019940-11153-1-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 14, 2014 at 5:39 AM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > The operation conditions register (OCR) stores the voltage > profile of the card, however the list of possible voltages > is restricted by the voltage range supported by the supply > used as VCC/VDD. So in mmc_vddrange_to_ocrmask() a OCR mask > is obtained to filter the not supported voltages, from the > value read in the host controller OCR register. > > For fixed regulators, regulator_list_voltage() returns the > fixed output for the first selector but this doesn't happen > for switch (FET) regulators that obtain their voltage from > their parent supply. A call to regulator_get_voltage() is > needed in this case so the regulator core can return the > FET's parent supply voltage output. > > This change is consistent with the fact that for other > fixed regulators (that are not FETs) the OCR mask is > returned even when mmc_regulator_set_ocr() checks if the > regulator is fixed before calling regulator_set_voltage(). > > Without this patch, the following warning is reported when > a FET is used as a vmmc-supply: > > dwmmc_exynos 12220000.mmc: Failed getting OCR mask: -22 > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> https://lkml.org/lkml/2014/8/12/377 Perhaps I misunderstood the discussion in that thread but couldn't this failure also be addressed by adding proper constraints for each FET in individual DTS files to reflect the range of voltages that are safe for all consumers of that supply on the board? I thought the main concern with your other change was that the constraints you listed in the DTSI represented the limits of the PMIC and not the consumers. -Tim -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 14, 2014 at 07:13:00AM -0700, Tim Kryger wrote: > On Thu, Aug 14, 2014 at 5:39 AM, Javier Martinez Canillas > > Without this patch, the following warning is reported when > > a FET is used as a vmmc-supply: > > dwmmc_exynos 12220000.mmc: Failed getting OCR mask: -22 > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > https://lkml.org/lkml/2014/8/12/377 For the benefit of those reading here > Perhaps I misunderstood the discussion in that thread but couldn't > this failure also be addressed by adding proper constraints for each > FET in individual DTS files to reflect the range of voltages that are > safe for all consumers of that supply on the board? > I thought the main concern with your other change was that the > constraints you listed in the DTSI represented the limits of the PMIC > and not the consumers. Right, there's two things going on here. One is that as you describe we shouldn't be putting constraints in .dtsi files if we don't know they're OK for a given board. The other thing is that on this particular board it turns out that there's no support for varying the voltages at all so it doesn't make sense to have to specify a range, there's only one value anyway so the software really should be able to figure out that fixed value all by itself.
Hello Tim, Thanks for your feedback. On 08/14/2014 04:13 PM, Tim Kryger wrote: > > https://lkml.org/lkml/2014/8/12/377 > > Perhaps I misunderstood the discussion in that thread but couldn't > this failure also be addressed by adding proper constraints for each > FET in individual DTS files to reflect the range of voltages that are > safe for all consumers of that supply on the board? > > I thought the main concern with your other change was that the > constraints you listed in the DTSI represented the limits of the PMIC > and not the consumers. > > -Tim > Yes, if the child regulator has the constraints defined then regulator_voltage_list() won't filter the obtained parent voltage so it won't be necessary to call regulator_get_voltage() directly. But as Mark said on the thread you referred [0], if the voltage is not allowed to change for a regulator then it makes no sense to have their constraints specify a voltage range. And in this particular case the parent supply of the FET used as vmmc-supply is a fixed regulator. Now I wonder why mmc_regulator_get_ocrmask() even sets as a valid voltage in the OCR mask the voltage for a fixed regulator if mmc_regulator_set_ocr() is a no-op in this case but I guess is because users of this function shouldn't really care about this peculiarity. In any case, this change is consistent since with this patch FETs behaves the same as other fixed regulators whose voltage can't be changed but the voltage is still reported in the OCR mask. Best regards, Javier [0]: https://lkml.org/lkml/2014/8/13/364 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 14, 2014 at 8:19 AM, Mark Brown <broonie@kernel.org> wrote: > Right, there's two things going on here. One is that as you describe we > shouldn't be putting constraints in .dtsi files if we don't know they're > OK for a given board. The other thing is that on this particular board > it turns out that there's no support for varying the voltages at all so > it doesn't make sense to have to specify a range, there's only one value > anyway so the software really should be able to figure out that fixed > value all by itself. If constraints are truly irrelevant when the voltage supplied to consumers is fixed, why doesn't regulator_list_voltage honor this exemption and skip the voltage filtering that uses (potentially unspecified) constraints when output is entirely determined by a parent (or grandparent) supply that can't change its voltage? It seems odd to make callers be the ones to handle this subtlety. Thanks, Tim Kryger -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Tim, On 08/15/2014 07:36 AM, Tim Kryger wrote: > On Thu, Aug 14, 2014 at 8:19 AM, Mark Brown <broonie@kernel.org> wrote: > >> Right, there's two things going on here. One is that as you describe we >> shouldn't be putting constraints in .dtsi files if we don't know they're >> OK for a given board. The other thing is that on this particular board >> it turns out that there's no support for varying the voltages at all so >> it doesn't make sense to have to specify a range, there's only one value >> anyway so the software really should be able to figure out that fixed >> value all by itself. > > If constraints are truly irrelevant when the voltage supplied to > consumers is fixed, why doesn't regulator_list_voltage honor this > exemption and skip the voltage filtering that uses (potentially > unspecified) constraints when output is entirely determined by a > parent (or grandparent) supply that can't change its voltage? > I had a similar thought before and proposed the patch: "[RFC 3/5] regulator: core: Only apply constraints if available on list voltage" [0]. But then Mark explained to me that this is wrong since in that case regulator_list_voltage() will list voltages that can't really be set [1]. But now I wonder why regulator_list_voltage() even list the voltage for fixed regulators (desc->fixed_uV) since they don't have the ability to vary voltage. The regulator_list_voltage() documentation says: "Returns a voltage that can be passed to @regulator_set_voltage(), zero if this selector code can't be used on this system, or a negative errno." But in the case of fixed regulators, it is actually listing a voltage that can't be selected. Although regulator_set_voltage() checks if the desired voltage is equal to the regulator min_uV and max_uV and just exits in that case, it feels wrong to list the voltage for a fixed regulators. regulator_list_voltage() only works because of the way we define the generic fixed voltage regulators and that is assuming that "regulator-min-microvolt" and "regulator-max-microvolt" DT properties being the same means that the regulator is fixed. This is kind of unfortunate, maybe it would had been better to define it explicitly using a "regulator-fixed-microvolt" or something. If we had such a DT property, then constraints wouldn't had been set for fixed regulators and regulator_list_voltage() wouldn't list its voltage neither. > It seems odd to make callers be the ones to handle this subtlety. > If regulator_list_voltage() didn't list the voltage for fixed regulators, then this subtlety should had been handled by callers before but they didn't because they rely on regulator_list_voltage() to always return a voltage even for fixed regulators. > Thanks, > Tim Kryger > Best regards, Javier [0]: https://lkml.org/lkml/2014/7/29/418 [1]: https://lkml.org/lkml/2014/7/29/453 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 14, 2014 at 10:36:18PM -0700, Tim Kryger wrote: > On Thu, Aug 14, 2014 at 8:19 AM, Mark Brown <broonie@kernel.org> wrote: > > > Right, there's two things going on here. One is that as you describe we > > shouldn't be putting constraints in .dtsi files if we don't know they're > > OK for a given board. The other thing is that on this particular board > > it turns out that there's no support for varying the voltages at all so > > it doesn't make sense to have to specify a range, there's only one value > > anyway so the software really should be able to figure out that fixed > > value all by itself. > If constraints are truly irrelevant when the voltage supplied to > consumers is fixed, why doesn't regulator_list_voltage honor this > exemption and skip the voltage filtering that uses (potentially > unspecified) constraints when output is entirely determined by a > parent (or grandparent) supply that can't change its voltage? > It seems odd to make callers be the ones to handle this subtlety. regulator_list_voltage() tells the consumer what voltages could be set, regulator_get_voltage() tells the consumer what the voltage currently is. These aren't quite the same thing.
On Fri, Aug 15, 2014 at 09:48:43AM +0200, Javier Martinez Canillas wrote: > But now I wonder why regulator_list_voltage() even list the voltage for > fixed regulators (desc->fixed_uV) since they don't have the ability to > vary voltage. The regulator_list_voltage() documentation says: That's because it's very cheap to do and there is a comprehensible thing we can return - if we have to read the voltage that means potentially asking the hardware in an I2C transaction which is not cheap. > > It seems odd to make callers be the ones to handle this subtlety. > If regulator_list_voltage() didn't list the voltage for fixed regulators, > then this subtlety should had been handled by callers before but they > didn't because they rely on regulator_list_voltage() to always return a > voltage even for fixed regulators. There's plenty of potentially variable regulators used in these situations, I expect it's more likely that people were just ignoring the warning since it has no practical effect.
Hello Mark, On 08/15/2014 11:55 AM, Mark Brown wrote: > On Fri, Aug 15, 2014 at 09:48:43AM +0200, Javier Martinez Canillas wrote: > >> But now I wonder why regulator_list_voltage() even list the voltage for >> fixed regulators (desc->fixed_uV) since they don't have the ability to > > That's because it's very cheap to do and there is a comprehensible thing > we can return - if we have to read the voltage that means potentially > asking the hardware in an I2C transaction which is not cheap. > Thanks a lot for the explanation, that does make a lot of sense. > > There's plenty of potentially variable regulators used in these > situations, I expect it's more likely that people were just ignoring the > warning since it has no practical effect. > Indeed. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 15, 2014 at 12:48 AM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > Hello Tim, > > On 08/15/2014 07:36 AM, Tim Kryger wrote: >> On Thu, Aug 14, 2014 at 8:19 AM, Mark Brown <broonie@kernel.org> wrote: >> >>> Right, there's two things going on here. One is that as you describe we >>> shouldn't be putting constraints in .dtsi files if we don't know they're >>> OK for a given board. The other thing is that on this particular board >>> it turns out that there's no support for varying the voltages at all so >>> it doesn't make sense to have to specify a range, there's only one value >>> anyway so the software really should be able to figure out that fixed >>> value all by itself. >> >> If constraints are truly irrelevant when the voltage supplied to >> consumers is fixed, why doesn't regulator_list_voltage honor this >> exemption and skip the voltage filtering that uses (potentially >> unspecified) constraints when output is entirely determined by a >> parent (or grandparent) supply that can't change its voltage? >> > > I had a similar thought before and proposed the patch: > > "[RFC 3/5] regulator: core: Only apply constraints if available on list > voltage" [0]. > [0]: https://lkml.org/lkml/2014/7/29/418 You proposed constraints only be applied when they are defined. That is a little different from my suggestion where the constraints check is skipped when the regulator output is fixed. It effectively does this now when the regulator itself provides the fixed voltage. However, the checks aren't skipped when that fixed voltage is coming from an ancestor. Why the difference? -Tim -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15 August 2014 11:55, Mark Brown <broonie@kernel.org> wrote: > On Fri, Aug 15, 2014 at 09:48:43AM +0200, Javier Martinez Canillas wrote: > >> But now I wonder why regulator_list_voltage() even list the voltage for >> fixed regulators (desc->fixed_uV) since they don't have the ability to >> vary voltage. The regulator_list_voltage() documentation says: > > That's because it's very cheap to do and there is a comprehensible thing > we can return - if we have to read the voltage that means potentially > asking the hardware in an I2C transaction which is not cheap. > >> > It seems odd to make callers be the ones to handle this subtlety. > >> If regulator_list_voltage() didn't list the voltage for fixed regulators, >> then this subtlety should had been handled by callers before but they >> didn't because they rely on regulator_list_voltage() to always return a >> voltage even for fixed regulators. > > There's plenty of potentially variable regulators used in these > situations, I expect it's more likely that people were just ignoring the > warning since it has no practical effect. Just wanted to add some input regarding the errors in the mmc case. These are of high importance. In principle if you get, "Failed getting OCR mask: -22", likely you will be using a wrong OCR mask while negotiating the voltage level with the card. So, somehow we need to address this issue. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 15, 2014 at 07:19:41AM -0700, Tim Kryger wrote: > That is a little different from my suggestion where the constraints > check is skipped when the regulator output is fixed. It effectively > does this now when the regulator itself provides the fixed voltage. > However, the checks aren't skipped when that fixed voltage is coming > from an ancestor. Why the difference? Nobody has written suitable code, and please bear in mind that even if the code is written there will probably be cases where it's too expensive for whatever reason so Javier's change is going to be needed.
On Fri, Aug 15, 2014 at 04:51:38PM +0200, Ulf Hansson wrote: > Just wanted to add some input regarding the errors in the mmc case. > These are of high importance. In principle if you get, "Failed getting > OCR mask: -22", likely you will be using a wrong OCR mask while > negotiating the voltage level with the card. > So, somehow we need to address this issue. Perhaps a WARN_ON() would help here - I'd not blame users for just ignoring the current warning?
On Fri, Aug 15, 2014 at 3:29 PM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Aug 15, 2014 at 07:19:41AM -0700, Tim Kryger wrote: > >> That is a little different from my suggestion where the constraints >> check is skipped when the regulator output is fixed. It effectively >> does this now when the regulator itself provides the fixed voltage. >> However, the checks aren't skipped when that fixed voltage is coming >> from an ancestor. Why the difference? > > Nobody has written suitable code, and please bear in mind that even if > the code is written there will probably be cases where it's too > expensive for whatever reason so Javier's change is going to be needed. I fail to see how replicating similar logic at all current regulator_list_voltage call sites would be any more efficient than handling this directly in regulator core. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Aug 17, 2014 at 10:11:30AM -0700, Tim Kryger wrote: > On Fri, Aug 15, 2014 at 3:29 PM, Mark Brown <broonie@kernel.org> wrote: > > Nobody has written suitable code, and please bear in mind that even if > > the code is written there will probably be cases where it's too > > expensive for whatever reason so Javier's change is going to be needed. > I fail to see how replicating similar logic at all current > regulator_list_voltage call sites would be any more efficient than > handling this directly in regulator core. If you feel like writing a helper function that would be fine but list_voltage() can't be doing expensive talking to the hardware operations - it needs to be something people can be comfortable calling a lot.
Hello Ulf, On 08/15/2014 04:51 PM, Ulf Hansson wrote: > > Just wanted to add some input regarding the errors in the mmc case. > These are of high importance. In principle if you get, "Failed getting > OCR mask: -22", likely you will be using a wrong OCR mask while > negotiating the voltage level with the card. > > So, somehow we need to address this issue. > And do you think that $subject is the right approach to solve this issue? If not please let me know so I can address that. > Kind regards > Uffe > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19 August 2014 13:29, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > Hello Ulf, > > On 08/15/2014 04:51 PM, Ulf Hansson wrote: >> >> Just wanted to add some input regarding the errors in the mmc case. >> These are of high importance. In principle if you get, "Failed getting >> OCR mask: -22", likely you will be using a wrong OCR mask while >> negotiating the voltage level with the card. >> >> So, somehow we need to address this issue. >> > > And do you think that $subject is the right approach to solve this issue? > If not please let me know so I can address that. Well, currently this seems like the best approach. If we end up having some new regulator helper function, future wise, we can convert to such later on. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14 August 2014 14:39, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > The operation conditions register (OCR) stores the voltage > profile of the card, however the list of possible voltages > is restricted by the voltage range supported by the supply > used as VCC/VDD. So in mmc_vddrange_to_ocrmask() a OCR mask > is obtained to filter the not supported voltages, from the > value read in the host controller OCR register. > > For fixed regulators, regulator_list_voltage() returns the > fixed output for the first selector but this doesn't happen > for switch (FET) regulators that obtain their voltage from > their parent supply. A call to regulator_get_voltage() is > needed in this case so the regulator core can return the > FET's parent supply voltage output. > > This change is consistent with the fact that for other > fixed regulators (that are not FETs) the OCR mask is > returned even when mmc_regulator_set_ocr() checks if the > regulator is fixed before calling regulator_set_voltage(). > > Without this patch, the following warning is reported when > a FET is used as a vmmc-supply: > > dwmmc_exynos 12220000.mmc: Failed getting OCR mask: -22 > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> Thanks! Applied for next. Kind regards Uffe > --- > > drivers/mmc/core/core.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 7dc0c85..8abae04 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -1221,15 +1221,14 @@ int mmc_regulator_get_ocrmask(struct regulator *supply) > int result = 0; > int count; > int i; > + int vdd_uV; > + int vdd_mV; > > count = regulator_count_voltages(supply); > if (count < 0) > return count; > > for (i = 0; i < count; i++) { > - int vdd_uV; > - int vdd_mV; > - > vdd_uV = regulator_list_voltage(supply, i); > if (vdd_uV <= 0) > continue; > @@ -1238,6 +1237,15 @@ int mmc_regulator_get_ocrmask(struct regulator *supply) > result |= mmc_vddrange_to_ocrmask(vdd_mV, vdd_mV); > } > > + if (!result) { > + vdd_uV = regulator_get_voltage(supply); > + if (vdd_uV <= 0) > + return vdd_uV; > + > + vdd_mV = vdd_uV / 1000; > + result = mmc_vddrange_to_ocrmask(vdd_mV, vdd_mV); > + } > + > return result; > } > EXPORT_SYMBOL_GPL(mmc_regulator_get_ocrmask); > -- > 2.0.0.rc2 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/19/2014 02:43 PM, Ulf Hansson wrote: > > Well, currently this seems like the best approach. If we end up having > some new regulator helper function, future wise, we can convert to > such later on. > Great, thanks a lot for your help! > Kind regards > Uffe > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 7dc0c85..8abae04 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1221,15 +1221,14 @@ int mmc_regulator_get_ocrmask(struct regulator *supply) int result = 0; int count; int i; + int vdd_uV; + int vdd_mV; count = regulator_count_voltages(supply); if (count < 0) return count; for (i = 0; i < count; i++) { - int vdd_uV; - int vdd_mV; - vdd_uV = regulator_list_voltage(supply, i); if (vdd_uV <= 0) continue; @@ -1238,6 +1237,15 @@ int mmc_regulator_get_ocrmask(struct regulator *supply) result |= mmc_vddrange_to_ocrmask(vdd_mV, vdd_mV); } + if (!result) { + vdd_uV = regulator_get_voltage(supply); + if (vdd_uV <= 0) + return vdd_uV; + + vdd_mV = vdd_uV / 1000; + result = mmc_vddrange_to_ocrmask(vdd_mV, vdd_mV); + } + return result; } EXPORT_SYMBOL_GPL(mmc_regulator_get_ocrmask);
The operation conditions register (OCR) stores the voltage profile of the card, however the list of possible voltages is restricted by the voltage range supported by the supply used as VCC/VDD. So in mmc_vddrange_to_ocrmask() a OCR mask is obtained to filter the not supported voltages, from the value read in the host controller OCR register. For fixed regulators, regulator_list_voltage() returns the fixed output for the first selector but this doesn't happen for switch (FET) regulators that obtain their voltage from their parent supply. A call to regulator_get_voltage() is needed in this case so the regulator core can return the FET's parent supply voltage output. This change is consistent with the fact that for other fixed regulators (that are not FETs) the OCR mask is returned even when mmc_regulator_set_ocr() checks if the regulator is fixed before calling regulator_set_voltage(). Without this patch, the following warning is reported when a FET is used as a vmmc-supply: dwmmc_exynos 12220000.mmc: Failed getting OCR mask: -22 Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- drivers/mmc/core/core.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)