Tegra boots failing due to as3722 I/O errors

Tegra boots failing due to as3722 I/O errors

[Mark Brown]

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

Hi,

Currently the Jetson TK1 is failing to get regulators for MMC with at
least my regulator tree and probably -next also due to:

[    1.665750] as3722 4-0040: AS3722 with revision 0x1 found
[    1.682945] +VDDIO_SDMMC3: failed to get the current voltage(-22)
[    1.689135] as3722-regulator as3722-regulator: regulator 13 register failed -22
[    1.697593] as3722-regulator: probe of as3722-regulator failed with error -22

The get voltage operation here is just a simple mapping of a bitfield in
the regmap which suggests that the underlying problem is somewhere
further down the stack like the MFD or I2C drivers.  The error is
-EINVAL which suggests something like the register not being marked as
readable but the _VOLTAGE_REG registers appear to be marked as readable
in the MFD.

Full log for one boot at:

  https://storage.kernelci.org/broonie-regulator/v4.6-rc1-24-ga7e614dd1f91/arm-multi_v7_defconfig+CONFIG_SMP=n/lab-mhart/boot-tegra124-jetson-tk1.html

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

Re: Tegra boots failing due to as3722 I/O errors

[Jon Hunter]

Hi Mark,

On 29/03/16 20:03, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> Hi,
> 
> Currently the Jetson TK1 is failing to get regulators for MMC with at
> least my regulator tree and probably -next also due to:
> 
> [    1.665750] as3722 4-0040: AS3722 with revision 0x1 found
> [    1.682945] +VDDIO_SDMMC3: failed to get the current voltage(-22)
> [    1.689135] as3722-regulator as3722-regulator: regulator 13 register failed -22
> [    1.697593] as3722-regulator: probe of as3722-regulator failed with error -22
> 
> The get voltage operation here is just a simple mapping of a bitfield in
> the regmap which suggests that the underlying problem is somewhere
> further down the stack like the MFD or I2C drivers.  The error is
> -EINVAL which suggests something like the register not being marked as
> readable but the _VOLTAGE_REG registers appear to be marked as readable
> in the MFD.
> 
> Full log for one boot at:
> 
>   https://storage.kernelci.org/broonie-regulator/v4.6-rc1-24-ga7e614dd1f91/arm-multi_v7_defconfig+CONFIG_SMP=n/lab-mhart/boot-tegra124-jetson-tk1.html

Thanks for the report. I took a quick look at this and it turns out
that i2c and regmap are all working fine. The problem is the
VDDIO_SDMMC3 LDO (aka. LDO6) on the as3722 has a bypass mode which is
not represented by the voltage ranges for the LDO. On boot the LDO is
in bypass and returns a value which is not recognised and hence an
EINVAL is returned. The following change fixes the problem. The 0x3F
value represents the bypass mode. I will send out a formal patch.

Cheers
Jon

diff --git a/drivers/regulator/as3722-regulator.c b/drivers/regulator/as3722-regulator.c
index 8b046ee..f16d0e5 100644
--- a/drivers/regulator/as3722-regulator.c
+++ b/drivers/regulator/as3722-regulator.c
@@ -438,6 +438,13 @@ static const struct regulator_linear_range as3722_ldo_ranges[] = {
        REGULATOR_LINEAR_RANGE(1725000, 0x40, 0x7F, 25000),
 };
 
+static const struct regulator_linear_range as3722_ldo6_ranges[] = {
+       REGULATOR_LINEAR_RANGE(0, 0x00, 0x00, 0),
+       REGULATOR_LINEAR_RANGE(825000, 0x01, 0x24, 25000),
+       REGULATOR_LINEAR_RANGE(3300000, 0x3F, 0x3F, 0),
+       REGULATOR_LINEAR_RANGE(1725000, 0x40, 0x7F, 25000),
+};
+
 static struct regulator_ops as3722_ldo_ops = {
        .is_enabled = regulator_is_enabled_regmap,
        .enable = regulator_enable_regmap,
@@ -863,6 +870,16 @@ static int as3722_regulator_probe(struct platform_device *pdev)
                        as3722_regs->desc[id].n_linear_ranges =
                                        ARRAY_SIZE(as3722_sd2345_ranges);
                        break;
+               case AS3722_REGULATOR_ID_LDO6:
+                       if (reg_config->ext_control)
+                               ops = &as3722_ldo_extcntrl_ops;
+                       else
+                               ops = &as3722_ldo_ops;
+                       as3722_regs->desc[id].enable_time = 500;
+                       as3722_regs->desc[id].linear_ranges = as3722_ldo6_ranges;
+                       as3722_regs->desc[id].n_linear_ranges =
+                                               ARRAY_SIZE(as3722_ldo6_ranges);
+                       break;
                default:
                        if (reg_config->ext_control)
                                ops = &as3722_ldo_extcntrl_ops;

Re: Tegra boots failing due to as3722 I/O errors

[Jon Hunter]

Hi Mark,

On 30/03/16 12:31, Jon Hunter wrote:
> On 29/03/16 20:03, Mark Brown wrote:
>> * PGP Signed by an unknown key
>>
>> Hi,
>>
>> Currently the Jetson TK1 is failing to get regulators for MMC with at
>> least my regulator tree and probably -next also due to:
>>
>> [    1.665750] as3722 4-0040: AS3722 with revision 0x1 found
>> [    1.682945] +VDDIO_SDMMC3: failed to get the current voltage(-22)
>> [    1.689135] as3722-regulator as3722-regulator: regulator 13 register failed -22
>> [    1.697593] as3722-regulator: probe of as3722-regulator failed with error -22
>>
>> The get voltage operation here is just a simple mapping of a bitfield in
>> the regmap which suggests that the underlying problem is somewhere
>> further down the stack like the MFD or I2C drivers.  The error is
>> -EINVAL which suggests something like the register not being marked as
>> readable but the _VOLTAGE_REG registers appear to be marked as readable
>> in the MFD.
>>
>> Full log for one boot at:
>>
>>   https://storage.kernelci.org/broonie-regulator/v4.6-rc1-24-ga7e614dd1f91/arm-multi_v7_defconfig+CONFIG_SMP=n/lab-mhart/boot-tegra124-jetson-tk1.html
> 
> Thanks for the report. I took a quick look at this and it turns out
> that i2c and regmap are all working fine. The problem is the
> VDDIO_SDMMC3 LDO (aka. LDO6) on the as3722 has a bypass mode which is
> not represented by the voltage ranges for the LDO. On boot the LDO is
> in bypass and returns a value which is not recognised and hence an
> EINVAL is returned. The following change fixes the problem. The 0x3F
> value represents the bypass mode. I will send out a formal patch.
> 
> Cheers
> Jon
> 
> diff --git a/drivers/regulator/as3722-regulator.c b/drivers/regulator/as3722-regulator.c
> index 8b046ee..f16d0e5 100644
> --- a/drivers/regulator/as3722-regulator.c
> +++ b/drivers/regulator/as3722-regulator.c
> @@ -438,6 +438,13 @@ static const struct regulator_linear_range as3722_ldo_ranges[] = {
>         REGULATOR_LINEAR_RANGE(1725000, 0x40, 0x7F, 25000),
>  };
>  
> +static const struct regulator_linear_range as3722_ldo6_ranges[] = {
> +       REGULATOR_LINEAR_RANGE(0, 0x00, 0x00, 0),
> +       REGULATOR_LINEAR_RANGE(825000, 0x01, 0x24, 25000),
> +       REGULATOR_LINEAR_RANGE(3300000, 0x3F, 0x3F, 0),
> +       REGULATOR_LINEAR_RANGE(1725000, 0x40, 0x7F, 25000),
> +};
> +
>  static struct regulator_ops as3722_ldo_ops = {
>         .is_enabled = regulator_is_enabled_regmap,
>         .enable = regulator_enable_regmap,
> @@ -863,6 +870,16 @@ static int as3722_regulator_probe(struct platform_device *pdev)
>                         as3722_regs->desc[id].n_linear_ranges =
>                                         ARRAY_SIZE(as3722_sd2345_ranges);
>                         break;
> +               case AS3722_REGULATOR_ID_LDO6:
> +                       if (reg_config->ext_control)
> +                               ops = &as3722_ldo_extcntrl_ops;
> +                       else
> +                               ops = &as3722_ldo_ops;
> +                       as3722_regs->desc[id].enable_time = 500;
> +                       as3722_regs->desc[id].linear_ranges = as3722_ldo6_ranges;
> +                       as3722_regs->desc[id].n_linear_ranges =
> +                                               ARRAY_SIZE(as3722_ldo6_ranges);
> +                       break;
>                 default:
>                         if (reg_config->ext_control)
>                                 ops = &as3722_ldo_extcntrl_ops;

On second thoughts, is this the correct way to fix this? I see that
there are some operators defined for get/set_bypass for a regulator and
these are not specified for LDO6 either. However, even if they were I
don't believe this will fix the problem alone.

The failure is caused by _regulator_get_voltage() returning an error.
This function does not check if the regulator is bypassed, but I am
wondering if it should and then return the bypass voltage?

Cheers
Jon

Re: Tegra boots failing due to as3722 I/O errors

[Mark Brown]

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

On Wed, Mar 30, 2016 at 02:15:16PM +0100, Jon Hunter wrote:

> On second thoughts, is this the correct way to fix this? I see that
> there are some operators defined for get/set_bypass for a regulator and
> these are not specified for LDO6 either. However, even if they were I
> don't believe this will fix the problem alone.

If this is trying to implement bypass support it should use bypass
support (and the other things doing this fixed to do so).

> The failure is caused by _regulator_get_voltage() returning an error.
> This function does not check if the regulator is bypassed, but I am
> wondering if it should and then return the bypass voltage?

Yes, like we already do for switches.  I'll send an untested patch
shortly.

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

Re: Tegra boots failing due to as3722 I/O errors

[Jon Hunter]

On 30/03/16 17:46, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Mar 30, 2016 at 02:15:16PM +0100, Jon Hunter wrote:
> 
>> On second thoughts, is this the correct way to fix this? I see that
>> there are some operators defined for get/set_bypass for a regulator and
>> these are not specified for LDO6 either. However, even if they were I
>> don't believe this will fix the problem alone.
> 
> If this is trying to implement bypass support it should use bypass
> support (and the other things doing this fixed to do so).

I have created 2 patches to add bypass support for LDO6 on the as3722 [0].

>> The failure is caused by _regulator_get_voltage() returning an error.
>> This function does not check if the regulator is bypassed, but I am
>> wondering if it should and then return the bypass voltage?
> 
> Yes, like we already do for switches.  I'll send an untested patch
> shortly.

Thanks. I have tested the patch, but as I mentioned in my response, the
issue is still not resolved even my patches for as3722. Hence, I have
not sent out my patches yet (although I verified they are working and
detecting bypass correctly).

Unfortunately, I am leaving for a weeks holiday starting tomorrow.

Laxman, Thierry, if you have some time to look at this over the next
week that would be great. Otherwise I will pick it up when I get back. I
don't think that it is too hard to resolve, we just need to add support
for bypass on LDO6 for as3722 (hopefully I have done this now) and then
ensure the bypass voltage is read correctly when registering the LDO.
Mark's initial patch to do this [1] is not working so far.

By the way, this issue has been recently exposed by a change to the
regulator core (bisect showed the problem is first seen after commit
fa93fd4ecc9c "regulator: core: Ensure we are at least in bounds for our
constraints" was added).

Cheers
Jon

[0] https://github.com/jonhunter/linux/commits/as3722	
[1] http://marc.info/?l=linux-kernel&m=145935914903138&w=2