Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

[Krzysztof Kozlowski]

On Tue, Mar 22, 2016 at 6:06 AM, Ivaylo Dimitrov
<ivo.g.dimitrov.75@gmail.com> wrote:
> Hi,
>
> On 21.03.2016 21:18, Mark Brown wrote:
>>
>> Currently we only attempt to set the voltage during constraints
>> application if an exact voltage is specified.  Extend this so that if
>> the currently set voltage for the regualtor is outside the bounds set in
>
>
> regulator
>
>> constraints we will move the voltage to the nearest constraint, raising
>> to the minimum or lowering to the maximum as needed.  This ensures that
>> drivers can probe without the hardware being driven out of spec.
>>
>> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>> Signed-off-by: Mark Brown <broonie@kernel.org>
>> ---
>>
>> Untested so far, will give it a spin later/tomorrow.
>>
>>   drivers/regulator/core.c | 32 +++++++++++++++++++++++++-------
>>   1 file changed, 25 insertions(+), 7 deletions(-)
>>
>
> Patch 1 does not apply cleanly on 4.5-rc5, so I applied it by hand, however,
> you may add:
>
> Tested-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>


Cc: Ulf, mmc

I bisected today's next MMC/SD card failure to this patch. Plugging a
SD card on Odroid XU3/XU4 (Exynos5422) causes:
[   11.759954] mmc1: card never left busy state
[   11.762795] mmc1: error -110 whilst initialising SD card

Full log (for booting from SD but it happens also when plugging the
card to the board booted from eMMC):
http://www.krzk.eu/builders/boot-odroid-xu3-multi_v7/builds/538/steps/Boot%20odroid/logs/serial

Bisect log:
# bad: [6f30d29c603cd444d76fadb393ea412e843ff095] Add linux-next
specific files for 20160324
# good: [b562e44f507e863c6792946e4e1b1449fbbac85d] Linux 4.5
git bisect start 'HEAD' 'v4.5'
# good: [35fe688abfcfeadf678664ae2e04815bb416de5a] arm-soc: document merges
git bisect good 35fe688abfcfeadf678664ae2e04815bb416de5a
# good: [2c856e14dad8cb1b085ae1f30c5e125c6d46019b] Merge tag
'arm64-perf' of
git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux
git bisect good 2c856e14dad8cb1b085ae1f30c5e125c6d46019b
# good: [b8ba4526832fcccba7f46e55ce9a8b79902bdcec] Merge tag
'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/dledford/rdma
git bisect good b8ba4526832fcccba7f46e55ce9a8b79902bdcec
# good: [c8c840c7f5d4ae6ae6e1a029a1c8797df3e8756b] Merge
remote-tracking branch 'ubifs/linux-next'
git bisect good c8c840c7f5d4ae6ae6e1a029a1c8797df3e8756b
# good: [974966a719fe6cd162990189c77986713b45dc83] Merge
remote-tracking branch 'input/next'
git bisect good 974966a719fe6cd162990189c77986713b45dc83
# bad: [49e1bab1b81d3a90a05d246f79e31938fe04e488] Merge
remote-tracking branch 'usb-chipidea-next/ci-for-usb-next'
git bisect bad 49e1bab1b81d3a90a05d246f79e31938fe04e488
# bad: [fad6ea2fe037a54db8eee8d6e74936e66489e52c] Merge
remote-tracking branch 'tip/auto-latest'
git bisect bad fad6ea2fe037a54db8eee8d6e74936e66489e52c
# bad: [dfb94c1d845d21740c4eb54e5d87f650d4362e9c] Merge
remote-tracking branch 'spi/for-next'
git bisect bad dfb94c1d845d21740c4eb54e5d87f650d4362e9c
# good: [0480adb1395bb6575d2e4174d0c2ed4c4df0275f] Merge branch
'master' into next
git bisect good 0480adb1395bb6575d2e4174d0c2ed4c4df0275f
# good: [c7faa32adbeacc97a2b2cf209c59161eb5efbc30] Merge
remote-tracking branch 'md/for-next'
git bisect good c7faa32adbeacc97a2b2cf209c59161eb5efbc30
# good: [78b5a17c35bd3b301bac25a72a1988b7537e5dc6] Merge
remote-tracking branches 'spi/fix/omap2' and 'spi/fix/rockchip' into
spi-linus
git bisect good 78b5a17c35bd3b301bac25a72a1988b7537e5dc6
# bad: [7e89db5db77fd4b2e49982c6f2ee39f4306c66a4] Merge
remote-tracking branches 'regulator/fix/constrain' and
'regulator/fix/gpio' into regulator-linus
git bisect bad 7e89db5db77fd4b2e49982c6f2ee39f4306c66a4
# bad: [bae4fdc88d7f7dda1bfb3dd282978c617dbf16e3] regulator: core:
Ensure we are at least in bounds for our constraints
git bisect bad bae4fdc88d7f7dda1bfb3dd282978c617dbf16e3
# good: [895fe2321efaf62023fdd8239d1846394df68570] regulator: core:
Always flag voltage constraints as appliable
git bisect good 895fe2321efaf62023fdd8239d1846394df68570
# first bad commit: [bae4fdc88d7f7dda1bfb3dd282978c617dbf16e3]
regulator: core: Ensure we are at least in bounds for our constraints

Best regards,
Krzysztof

Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

[Krzysztof Kozlowski]

On Thu, Mar 24, 2016 at 3:11 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> On Tue, Mar 22, 2016 at 6:06 AM, Ivaylo Dimitrov
> <ivo.g.dimitrov.75@gmail.com> wrote:
>> Hi,
>>
>> On 21.03.2016 21:18, Mark Brown wrote:
>>>
>>> Currently we only attempt to set the voltage during constraints
>>> application if an exact voltage is specified.  Extend this so that if
>>> the currently set voltage for the regualtor is outside the bounds set in
>>
>>
>> regulator
>>
>>> constraints we will move the voltage to the nearest constraint, raising
>>> to the minimum or lowering to the maximum as needed.  This ensures that
>>> drivers can probe without the hardware being driven out of spec.
>>>
>>> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>>> Signed-off-by: Mark Brown <broonie@kernel.org>
>>> ---
>>>
>>> Untested so far, will give it a spin later/tomorrow.
>>>
>>>   drivers/regulator/core.c | 32 +++++++++++++++++++++++++-------
>>>   1 file changed, 25 insertions(+), 7 deletions(-)
>>>
>>
>> Patch 1 does not apply cleanly on 4.5-rc5, so I applied it by hand, however,
>> you may add:
>>
>> Tested-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>
>
> Cc: Ulf, mmc
>
> I bisected today's next MMC/SD card failure to this patch. Plugging a
> SD card on Odroid XU3/XU4 (Exynos5422) causes:
> [   11.759954] mmc1: card never left busy state
> [   11.762795] mmc1: error -110 whilst initialising SD card
>
> Full log (for booting from SD but it happens also when plugging the
> card to the board booted from eMMC):
> http://www.krzk.eu/builders/boot-odroid-xu3-multi_v7/builds/538/steps/Boot%20odroid/logs/serial

+Cc: linux-samsung-soc, Javier and Marek.

Although bisect complained about this patch but I am not convinced
that it is the cause. Before the patch the constraints were not
applied. To none of the regulators. With the patch the constraints are
applied to bucks... which are not directly used by SD card.

Instead some of the bucks are apparently supplying other LDOs. I
suspect buck9 which is mentioned as a supply for VDD 2.8V (2.8V
happens to be also the voltage of regulators going to SD card).
With the patch following constraints are applied:
                 target_min, target max, current
setting BUCK1 to 1100000 1100000 1100000
setting BUCK2 to 1000000 1000000 1000000
setting BUCK3 to 1000000 1000000 1000000
setting BUCK4 to 1000000 1000000 1000000
setting BUCK5 to 1200000 1200000 1200000
setting BUCK6 to 1025000 1025000 1025000
setting BUCK7 to 900000 900000 900000
setting BUCK8 to 1225000 1225000 1225000
setting BUCK9 to 3750000 3750000 5000000

Why the heck current_uV is 5V? I'll try to figure this out...

Best regards,
Krzysztof

Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

[Bjorn Andersson]

On Thu, Mar 24, 2016 at 12:18 AM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> On Thu, Mar 24, 2016 at 3:11 PM, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
>> On Tue, Mar 22, 2016 at 6:06 AM, Ivaylo Dimitrov
>> <ivo.g.dimitrov.75@gmail.com> wrote:
>>> Hi,
>>>
>>> On 21.03.2016 21:18, Mark Brown wrote:
>>>>
>>>> Currently we only attempt to set the voltage during constraints
>>>> application if an exact voltage is specified.  Extend this so that if
>>>> the currently set voltage for the regualtor is outside the bounds set in
>>>
>>>
>>> regulator
>>>
>>>> constraints we will move the voltage to the nearest constraint, raising
>>>> to the minimum or lowering to the maximum as needed.  This ensures that
>>>> drivers can probe without the hardware being driven out of spec.
>>>>
>>>> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>>>> Signed-off-by: Mark Brown <broonie@kernel.org>
>>>> ---
>>>>
>>>> Untested so far, will give it a spin later/tomorrow.
>>>>
>>>>   drivers/regulator/core.c | 32 +++++++++++++++++++++++++-------
>>>>   1 file changed, 25 insertions(+), 7 deletions(-)
>>>>
>>>
>>> Patch 1 does not apply cleanly on 4.5-rc5, so I applied it by hand, however,
>>> you may add:
>>>
>>> Tested-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>>
>>
>> Cc: Ulf, mmc
>>
>> I bisected today's next MMC/SD card failure to this patch. Plugging a
>> SD card on Odroid XU3/XU4 (Exynos5422) causes:
>> [   11.759954] mmc1: card never left busy state
>> [   11.762795] mmc1: error -110 whilst initialising SD card
>>

Mark,

I'm facing the same issue on my Qualcomm based boards; my eMMCs fails
to talk to me because I'm not powering them.

The problem in my case comes from the vmmc regulator having min_uV ==
max_uV and patch 1 in this series changes the logic of
of_get_regulation_constraints() to no longer set apply_uV. I.e. we end
up never calling set_voltage(), ever.

When the mmc framework calls regulator_set_load() to change voltage
REGULATOR_CHANGE_VOLTAGE is not set (because min == max) so we will
never call set_voltage().

Before patch 1 apply_uV was set and we did set up the voltage earlier,
so regulator_get_voltage() will return a valid voltage and
regulator_set_voltage() will succeed for the valid voltages (if the
range spans the current setting).


Reinstating the following snippet in of_get_regulation_constraints()
sort this out:
if (constraints->min_uV && constraints->max_uV)
                                       constraints->apply_uV = true;


I did look at an alternative of having regulator_set_voltage() pass
and call set_voltage() if the requested voltage matches the
constraints, but this does indeed seem to mess things up. So checking
in with you before continuing on that hack.

Regards,
Bjorn

Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

On Sat, Mar 26, 2016 at 04:50:41PM -0700, Bjorn Andersson wrote:

> Reinstating the following snippet in of_get_regulation_constraints()
> sort this out:

> if (constraints->min_uV && constraints->max_uV)
>                                        constraints->apply_uV = true;

The existing check in the patch should be an || not an ==, or possibly
we should just not bother looking for min_uV at all.  I just pushed out
a version of that, let's see how that goes.

> I did look at an alternative of having regulator_set_voltage() pass
> and call set_voltage() if the requested voltage matches the
> constraints, but this does indeed seem to mess things up. So checking
> in with you before continuing on that hack.

Yes, not everything is writeable.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

[Bjorn Andersson]

On Sun, Mar 27, 2016 at 2:08 AM, Mark Brown <broonie@kernel.org> wrote:
> On Sat, Mar 26, 2016 at 04:50:41PM -0700, Bjorn Andersson wrote:
>
>> Reinstating the following snippet in of_get_regulation_constraints()
>> sort this out:
>
>> if (constraints->min_uV && constraints->max_uV)
>>                                        constraints->apply_uV = true;
>
> The existing check in the patch should be an || not an ==, or possibly
> we should just not bother looking for min_uV at all.  I just pushed out
> a version of that, let's see how that goes.
>

Either way is fine with me, as long as we either go ahead and apply a
voltage setting now or allow a consumer to do so later (your posted
patch does both).

>> I did look at an alternative of having regulator_set_voltage() pass
>> and call set_voltage() if the requested voltage matches the
>> constraints, but this does indeed seem to mess things up. So checking
>> in with you before continuing on that hack.
>
> Yes, not everything is writeable.

Right, looking at your posted patch [1] we're changing this logic so
that normal regulators defined with min == max will be allowed to
set_voltage(). Thinking about it that makes sense and cleans the logic
up, so I'm in favor of this.

I know it's published, but fwiw you have my Acked-by on the posted patch.


[1] https://git.kernel.org/cgit/linux/kernel/git/broonie/regulator.git/commit/?h=for-next&id=fa93fd4ecc9c58475abac6db93a797bff893bc16

Regards,
Bjorn

Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

[Geert Uytterhoeven]

Hi Mark,

On Sun, Mar 27, 2016 at 11:08 AM, Mark Brown <broonie@kernel.org> wrote:
> On Sat, Mar 26, 2016 at 04:50:41PM -0700, Bjorn Andersson wrote:
>
>> Reinstating the following snippet in of_get_regulation_constraints()
>> sort this out:
>
>> if (constraints->min_uV && constraints->max_uV)
>>                                        constraints->apply_uV = true;
>
> The existing check in the patch should be an || not an ==, or possibly
> we should just not bother looking for min_uV at all.  I just pushed out
> a version of that, let's see how that goes.

Has the fix really been pushed out?

With commit fa93fd4ecc9c58475abac6db93a797bff893bc16
Author: Mark Brown <broonie@kernel.org>
Date:   Mon Mar 21 18:12:52 2016 +0000

    regulator: core: Ensure we are at least in bounds for our constraints

I see a few cases of

------------[ cut here ]------------
WARNING: CPU: 1 PID: 31 at drivers/regulator/core.c:2223
_regulator_disable+0x2c/0x128
unbalanced disables for SDHI0 VccQ
Modules linked in:
CPU: 1 PID: 31 Comm: kworker/1:1 Not tainted
4.6.0-rc1-koelsch-00724-g58d619227282dc16 #2422
Hardware name: Generic R8A7791 (Flattened Device Tree)
Workqueue: events_freezable mmc_rescan
sh_mobile_sdhi ee140000.sd: could not set regulator OCR (-22)

[<c020e040>] (unwind_backtrace) from [<c0209d70>] (show_stack+0x10/0x14)
[<c0209d70>] (show_stack) from [<c03c7584>] (dump_stack+0x7c/0x9c)
[<c03c7584>] (dump_stack) from [<c021fa48>] (__warn+0xcc/0xfc)
[<c021fa48>] (__warn) from [<c021faac>] (warn_slowpath_fmt+0x34/0x44)
[<c021faac>] (warn_slowpath_fmt) from [<c041e464>]
(_regulator_disable+0x2c/0x128)
[<c041e464>] (_regulator_disable) from [<c041e590>]
(regulator_disable+0x30/0x60)
[<c041e590>] (regulator_disable) from [<c0572e18>] (tmio_mmc_set_ios+0xb8/0x1d4)
[<c0572e18>] (tmio_mmc_set_ios) from [<c056300c>] (mmc_power_off+0x34/0x54)
[<c056300c>] (mmc_power_off) from [<c0563a5c>] (mmc_rescan+0x214/0x30c)
[<c0563a5c>] (mmc_rescan) from [<c0233368>] (process_one_work+0x1bc/0x2f4)
[<c0233368>] (process_one_work) from [<c0233a2c>] (worker_thread+0x2a8/0x3d0)
[<c0233a2c>] (worker_thread) from [<c0237a9c>] (kthread+0xd8/0xec)
[<c0237a9c>] (kthread) from [<c0206b68>] (ret_from_fork+0x14/0x2c)
---[ end trace 1c61a7f6221c11ea ]---

when booting on r8a7791/koelsch.

I'm a bit confused by the discussion of "&&" vs. "||" vs. "==", but the
warnings do go away when using "!=", cfr. the whitespace-damaged patch below.

--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -43,7 +43,7 @@ static void of_get_regulation_constraints(struct device_node *
                constraints->max_uV = pval;

        /* Voltage change possible? */
-       if (constraints->min_uV && constraints->max_uV) {
+       if (constraints->min_uV != constraints->max_uV) {
                constraints->valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
                constraints->apply_uV = true;
        }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]

On Tue, Mar 29, 2016 at 02:00:52PM +0200, Geert Uytterhoeven wrote:
> On Sun, Mar 27, 2016 at 11:08 AM, Mark Brown <broonie@kernel.org> wrote:

> > The existing check in the patch should be an || not an ==, or possibly
> > we should just not bother looking for min_uV at all.  I just pushed out
> > a version of that, let's see how that goes.

> Has the fix really been pushed out?

Yes.

> WARNING: CPU: 1 PID: 31 at drivers/regulator/core.c:2223
> _regulator_disable+0x2c/0x128
> unbalanced disables for SDHI0 VccQ

> when booting on r8a7791/koelsch.

This seems like a bug somewhere else in your code, we're looking at
changes in the voltage setting code but this is an unbalanced disable.

> I'm a bit confused by the discussion of "&&" vs. "||" vs. "==", but the
> warnings do go away when using "!=", cfr. the whitespace-damaged patch below.

>         /* Voltage change possible? */
> -       if (constraints->min_uV && constraints->max_uV) {
> +       if (constraints->min_uV != constraints->max_uV) {

Do you have constraints that specify a maximum voltage but no minimum
(which I'd say are broken), or constraints that are just plain wrong but
are now being applied like specifying the entire range of the regulator?
The above might be some follow on error handling from something that
happened the change in handling of the constraints.  You need to look at
what the relevant regulator constraints are...

Previously both voltages needed to be non-zero and equal for anything to
happen, now they only need to both be non-zero.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

[Geert Uytterhoeven]

On Tue, Mar 29, 2016 at 4:58 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Mar 29, 2016 at 02:00:52PM +0200, Geert Uytterhoeven wrote:
>> On Sun, Mar 27, 2016 at 11:08 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> > The existing check in the patch should be an || not an ==, or possibly
>> > we should just not bother looking for min_uV at all.  I just pushed out
>> > a version of that, let's see how that goes.
>
>> Has the fix really been pushed out?
>
> Yes.
>
>> WARNING: CPU: 1 PID: 31 at drivers/regulator/core.c:2223
>> _regulator_disable+0x2c/0x128
>> unbalanced disables for SDHI0 VccQ
>
>> when booting on r8a7791/koelsch.
>
> This seems like a bug somewhere else in your code, we're looking at
> changes in the voltage setting code but this is an unbalanced disable.
>
>> I'm a bit confused by the discussion of "&&" vs. "||" vs. "==", but the
>> warnings do go away when using "!=", cfr. the whitespace-damaged patch below.
>
>>         /* Voltage change possible? */
>> -       if (constraints->min_uV && constraints->max_uV) {
>> +       if (constraints->min_uV != constraints->max_uV) {
>
> Do you have constraints that specify a maximum voltage but no minimum
> (which I'd say are broken), or constraints that are just plain wrong but
> are now being applied like specifying the entire range of the regulator?
> The above might be some follow on error handling from something that
> happened the change in handling of the constraints.  You need to look at
> what the relevant regulator constraints are...
>
> Previously both voltages needed to be non-zero and equal for anything to
> happen, now they only need to both be non-zero.

There are 3 regulators with equal constraints:

/regulator@0: constraints->min_uV = 3300000, constraints->max_uV = 3300000
/regulator@2: constraints->min_uV = 3300000, constraints->max_uV = 3300000
/regulator@4: constraints->min_uV = 3300000, constraints->max_uV = 3300000

and 3 with different constraints:

/regulator@1: constraints->min_uV = 1800000, constraints->max_uV = 3300000
/regulator@3: constraints->min_uV = 1800000, constraints->max_uV = 3300000
/regulator@5: constraints->min_uV = 1800000, constraints->max_uV = 3300000

For the first SDHI channel, these come from:

        vcc_sdhi0: regulator@0 {
                compatible = "regulator-fixed";

                regulator-name = "SDHI0 Vcc";
                regulator-min-microvolt = <3300000>;
                regulator-max-microvolt = <3300000>;

                gpio = <&gpio7 17 GPIO_ACTIVE_HIGH>;
                enable-active-high;
        };

        vccq_sdhi0: regulator@1 {
                compatible = "regulator-gpio";

                regulator-name = "SDHI0 VccQ";
                regulator-min-microvolt = <1800000>;
                regulator-max-microvolt = <3300000>;

                gpios = <&gpio2 12 GPIO_ACTIVE_HIGH>;
                gpios-states = <1>;
                states = <3300000 1
                          1800000 0>;
        };

        &sdhi0 {
                pinctrl-0 = <&sdhi0_pins>;
                pinctrl-names = "default";

                vmmc-supply = <&vcc_sdhi0>;
                vqmmc-supply = <&vccq_sdhi0>;
                cd-gpios = <&gpio6 6 GPIO_ACTIVE_LOW>;
                wp-gpios = <&gpio6 7 GPIO_ACTIVE_HIGH>;
                status = "okay";
        };

giving:

        sh_mobile_sdhi ee100000.sd: GPIO lookup for consumer cd
        sh_mobile_sdhi ee100000.sd: using device tree for GPIO lookup
        of_get_named_gpiod_flags: parsed 'cd-gpios' property of node
'/sd@ee100000[0]' - status (0)
        sh_mobile_sdhi ee100000.sd: Got CD GPIO
        sh_mobile_sdhi ee100000.sd: GPIO lookup for consumer wp
        sh_mobile_sdhi ee100000.sd: using device tree for GPIO lookup
        of_get_named_gpiod_flags: parsed 'wp-gpios' property of node
'/sd@ee100000[0]' - status (0)
        sh_mobile_sdhi ee100000.sd: Got WP GPIO
======> sh_mobile_sdhi ee100000.sd: could not set regulator OCR (-22)
        gpio_rcar e6055400.gpio: sense irq = 6, type = 3
        sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 97 MHz

The line marked with the arrow is introduced by the changed check, and looks
to be the origin of the failure.

Later, the "unbalanced disables for SDHI0 VccQ" warning is triggered.
Probably the error path always disables the second regulator, even if it was
never enabled due an the earlier failure.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Tue, Mar 29, 2016 at 08:05:34PM +0200, Geert Uytterhoeven wrote:

>         sh_mobile_sdhi ee100000.sd: Got WP GPIO
> ======> sh_mobile_sdhi ee100000.sd: could not set regulator OCR (-22)
>         gpio_rcar e6055400.gpio: sense irq = 6, type = 3
>         sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 97 MHz

> The line marked with the arrow is introduced by the changed check, and looks
> to be the origin of the failure.

This isn't making any sense.  Why would a change in how we apply voltage
constraints on initial probe of the regulator have an impact here?  The
changed code shouldn't even be executing at the point where the SDHCI
driver is trying to use the regulator.  There's something else going on
here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

[Haibo Chen]

Hi Brown,

I also meet the similar issue on i.MX6 platforms.

With your patch   ---> regulator: core: Ensure we are at least in bounds for our constraints
When I insert an SD3.0 card, shows the following log:

root@imx6qdlsolo:~# [   59.733941] sdhci-esdhc-imx 2190000.usdhc: could not set regulator OCR (-22)
[   60.829911] sdhci-esdhc-imx 2190000.usdhc: could not set regulator OCR (-22)
[   61.917951] sdhci-esdhc-imx 2190000.usdhc: could not set regulator OCR (-22)
[   63.009498] sdhci-esdhc-imx 2190000.usdhc: could not set regulator OCR (-22)

I did a quick debug, and find when I change the operator   &&  to  !=  , this issue gone.
-       if (constraints->min_uV != constraints->max_uV) {
+       if (constraints->min_uV && constraints->max_uV) {


In our sdhci.c, we call the function 
regulator_set_voltage ---> regulator_set_voltage_unlocked(struct regulator *regulator, int min_uV, int max_uV)
here, the parameter min_uV is 3300000, and the max_uV is 3400000

currently with your patch (the upper operator is &&), when insert a SD3.0 card, 
it will do the sanity check, and return -EINVAL

but when I change the upper operator from && to !=, 
before the sanity check, it will first get the current_uV, and then go to out.

I'm not familiar with regulator common code. Hope the upper describe can help you debug this issue.

The following attach our dts piece code.

126 &usdhc1 {
127         pinctrl-names = "default", "state_100mhz", "state_200mhz";
128         pinctrl-0 = <&pinctrl_usdhc1>;
129         pinctrl-1 = <&pinctrl_usdhc1_100mhz>;
130         pinctrl-2 = <&pinctrl_usdhc1_200mhz>;
131         cd-gpios = <&gpio1 19 GPIO_ACTIVE_LOW>;
132         keep-power-in-suspend;
133         wakeup-source;
134         vmmc-supply = <&reg_sd1_vmmc>;
135         status = "okay";
136 };

regulators {
 26                 compatible = "simple-bus";
 27                 #address-cells = <1>;
 28                 #size-cells = <0>;
 29
 30                 reg_sd1_vmmc: sd1_regulator {
 31                         compatible = "regulator-fixed";
 32                         regulator-name = "VSD_3V3";
 33                         regulator-min-microvolt = <3300000>;
 34                         regulator-max-microvolt = <3300000>;
 35                         gpio = <&gpio1 9 GPIO_ACTIVE_HIGH>;
 36                         enable-active-high;
 37                 };
 38         };

Best Regards
Haibo Chen



> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Mark Brown
> Sent: Wednesday, March 30, 2016 2:27 AM
> To: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Bjorn Andersson <bjorn@kryo.se>; Krzysztof Kozlowski
> <k.kozlowski@samsung.com>; Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>;
> Liam Girdwood <lgirdwood@gmail.com>; linux-kernel@vger.kernel.org; Ulf
> Hansson <ulf.hansson@linaro.org>; linux-mmc <linux-mmc@vger.kernel.org>;
> linux-samsung-soc <linux-samsung-soc@vger.kernel.org>; Javier Martinez
> Canillas <javier@osg.samsung.com>; Marek Szyprowski
> <m.szyprowski@samsung.com>; linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for
> our constraints
> 
> On Tue, Mar 29, 2016 at 08:05:34PM +0200, Geert Uytterhoeven wrote:
> 
> >         sh_mobile_sdhi ee100000.sd: Got WP GPIO ======> sh_mobile_sdhi
> > ee100000.sd: could not set regulator OCR (-22)
> >         gpio_rcar e6055400.gpio: sense irq = 6, type = 3
> >         sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate
> > 97 MHz
> 
> > The line marked with the arrow is introduced by the changed check, and
> > looks to be the origin of the failure.
> 
> This isn't making any sense.  Why would a change in how we apply voltage
> constraints on initial probe of the regulator have an impact here?  The changed
> code shouldn't even be executing at the point where the SDHCI driver is trying
> to use the regulator.  There's something else going on here.

Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3283 bytes --]

On Wed, Mar 30, 2016 at 09:07:21AM +0000, Haibo Chen wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> In our sdhci.c, we call the function 
> regulator_set_voltage ---> regulator_set_voltage_unlocked(struct regulator *regulator, int min_uV, int max_uV)
> here, the parameter min_uV is 3300000, and the max_uV is 3400000
> 
> currently with your patch (the upper operator is &&), when insert a SD3.0 card, 
> it will do the sanity check, and return -EINVAL

> but when I change the upper operator from && to !=, 
> before the sanity check, it will first get the current_uV, and then go to out.
> 
> I'm not familiar with regulator common code. Hope the upper describe can help you debug this issue.

No, that's not helping clarify anything.  To repeat what I said to Geert 
this patch changes code that is only called when the regulator is
probed.  This means that the changed code is not running when the SDHCI
code is running.  You need to investigate what exactly is causing the
error.

Just randomly thrashing around with a separate bit of code with no
coharent explanation for what you think is happening is not helping
here, we need some analysis of what is going on.  The change you are
both proposing is guaranteed to break other boards since it means that
the case we supported originally where we set a specific voltage that is
specified by setting equal minimum and maximum constraints is no longer
going have the voltage applied.

Having taken another look I *suspect* that the SDHCI code is broken in
the way it enumerates the set of voltages that can be set and that this
will probably trigger in other situations where the set of voltages that
can be set is limited but I can't put my finger on it, someone with the
ability to run the code will need to investigate.

The following patch *might* help the SDHCI but I'm just guessing and
like I say it looks like this is flagging up a problem in the SDHCI
code.

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index f45106a44635..cd828dbf9d52 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -43,10 +43,12 @@ static void of_get_regulation_constraints(struct device_node *np,
 		constraints->max_uV = pval;
 
 	/* Voltage change possible? */
-	if (constraints->min_uV && constraints->max_uV) {
+	if (constraints->min_uV != constraints->max_uV)
 		constraints->valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
+
+	/* Do we have a voltage range, if so try to apply it? */
+	if (constraints->min_uV && constraints->max_uV)
 		constraints->apply_uV = true;
-	}
 
 	if (!of_property_read_u32(np, "regulator-microvolt-offset", &pval))
 		constraints->uV_offset = pval;

> > -----Original Message-----
> > From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> > owner@vger.kernel.org] On Behalf Of Mark Brown

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]