| Message ID | 53FAFCBC.2050407@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 25, 2014 at 2:37 PM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > Hello Yuvaraj, > > On 08/25/2014 10:22 AM, Yuvaraj Cd wrote: >>>> Good question. I'm not that familiar with the dw_mmc host controller nor >>>> its driver implementation so I'll let Yuvaraj or Doug to answer that. >> Well,here it goes! >> 1. Power ON the board LDO4CTRL1[7:6] 11b >> 2. dw_mmc driver enable the vqmmc. >> 3. checks for UHS support, complete the voltage switching t0 1.8V >> 4. Does warm reset by reboot command. >> 5. mmc core calls mmc_set_ios() with MMC_POWER_OFF. >> 6. dw_mmc driver cut-off the regulator with LDO4CTRL1[7:6] is 00b >> 7.dw_mmc driver enable the vqmmc. >> But after step 7 also, LD4CTRL[7:6] is 00b. > > Ok, so the dw_mmc driver is enabling vqmmc, that's good. > >>> >>> I haven't seen the issue that Yuvaraj is reporting (but I also haven't >>> picked up all of the relevant patches and tried to reproduce), so I'm >>> going to have to leave it to Yuvaraj to answer. >> static int max77802_enable(struct regulator_dev *rdev) >> { >> struct max77802_regulator_prv *max77802 = rdev_get_drvdata(rdev); >> int id = rdev_get_id(rdev); >> int shift = max77802_get_opmode_shift(id); >> return regmap_update_bits(rdev->regmap, >> rdev->desc->enable_reg,rdev->desc->enable_mask,max77802->opmode[id] << >> shift); >> } >> I think in the above code snippet, the "val" is what we got it during >> the probe.We always write that value for enabling this regulator(which >> is LDO4CTRL1[7:6] 00b after warm reset) which is not correct according >> the MAX77802 manual. >>> > > I see, so probably until we have a way to define the operating mode for > each regulator using DT we should set the opmode to normal when enabling a > regulator independently of the value the hardware register reported on probe. > > Can you please test the following change [0] so I can post as a proper > patch? Doug, Mark do you think that forcing the regulator to opmode normal > when enabling is the right solution here? > > Best regards, > Javier > > [0] > diff --git a/drivers/regulator/max77802.c b/drivers/regulator/max77802.c > index ad1caa9..917b5ab 100644 > --- a/drivers/regulator/max77802.c > +++ b/drivers/regulator/max77802.c > @@ -180,7 +180,7 @@ static int max77802_enable(struct regulator_dev *rdev) > > return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, > rdev->desc->enable_mask, > - max77802->opmode[id] << shift); > + MAX77802_OPMODE_NORMAL << shift); > } It works.
Javier, On Mon, Aug 25, 2014 at 2:07 AM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > Hello Yuvaraj, > > On 08/25/2014 10:22 AM, Yuvaraj Cd wrote: >>>> Good question. I'm not that familiar with the dw_mmc host controller nor >>>> its driver implementation so I'll let Yuvaraj or Doug to answer that. >> Well,here it goes! >> 1. Power ON the board LDO4CTRL1[7:6] 11b >> 2. dw_mmc driver enable the vqmmc. >> 3. checks for UHS support, complete the voltage switching t0 1.8V >> 4. Does warm reset by reboot command. >> 5. mmc core calls mmc_set_ios() with MMC_POWER_OFF. >> 6. dw_mmc driver cut-off the regulator with LDO4CTRL1[7:6] is 00b >> 7.dw_mmc driver enable the vqmmc. >> But after step 7 also, LD4CTRL[7:6] is 00b. > > Ok, so the dw_mmc driver is enabling vqmmc, that's good. > >>> >>> I haven't seen the issue that Yuvaraj is reporting (but I also haven't >>> picked up all of the relevant patches and tried to reproduce), so I'm >>> going to have to leave it to Yuvaraj to answer. >> static int max77802_enable(struct regulator_dev *rdev) >> { >> struct max77802_regulator_prv *max77802 = rdev_get_drvdata(rdev); >> int id = rdev_get_id(rdev); >> int shift = max77802_get_opmode_shift(id); >> return regmap_update_bits(rdev->regmap, >> rdev->desc->enable_reg,rdev->desc->enable_mask,max77802->opmode[id] << >> shift); >> } >> I think in the above code snippet, the "val" is what we got it during >> the probe.We always write that value for enabling this regulator(which >> is LDO4CTRL1[7:6] 00b after warm reset) which is not correct according >> the MAX77802 manual. >>> > > I see, so probably until we have a way to define the operating mode for > each regulator using DT we should set the opmode to normal when enabling a > regulator independently of the value the hardware register reported on probe. > > Can you please test the following change [0] so I can post as a proper > patch? Doug, Mark do you think that forcing the regulator to opmode normal > when enabling is the right solution here? IMHO that makes sense.
Hello Doug, On 08/25/2014 05:40 PM, Doug Anderson wrote: >> >> I see, so probably until we have a way to define the operating mode for >> each regulator using DT we should set the opmode to normal when enabling a >> regulator independently of the value the hardware register reported on probe. >> >> Can you please test the following change [0] so I can post as a proper >> patch? Doug, Mark do you think that forcing the regulator to opmode normal >> when enabling is the right solution here? > > IMHO that makes sense. > Thanks, I'll post it as a proper patch tomorrow then. Best regards, Javier
On Mon, Aug 25, 2014 at 08:40:40AM -0700, Doug Anderson wrote: > On Mon, Aug 25, 2014 at 2:07 AM, Javier Martinez Canillas > > I see, so probably until we have a way to define the operating mode for > > each regulator using DT we should set the opmode to normal when enabling a > > regulator independently of the value the hardware register reported on probe. > > Can you please test the following change [0] so I can post as a proper > > patch? Doug, Mark do you think that forcing the regulator to opmode normal > > when enabling is the right solution here? > IMHO that makes sense. No, this doesn't make any obvious sense to me at all. Picking normal as a default if the hardware reads back off due to overlapping impelementation or something *might* make sense but not overwriting the hardware state without explicit permission from the machine integration is a key goal for the regulator API.
Hello Mark, On 08/26/2014 09:17 AM, Mark Brown wrote: > On Mon, Aug 25, 2014 at 08:40:40AM -0700, Doug Anderson wrote: >> > Can you please test the following change [0] so I can post as a proper >> > patch? Doug, Mark do you think that forcing the regulator to opmode normal >> > when enabling is the right solution here? > >> IMHO that makes sense. > > No, this doesn't make any obvious sense to me at all. Picking normal as > a default if the hardware reads back off due to overlapping > impelementation or something *might* make sense but not overwriting the > hardware state without explicit permission from the machine integration > is a key goal for the regulator API. > Just to be sure I understood you correctly, what might makes sense to you then is to set the opmode to normal as default on probe only if off is read back from the hardware register but leaving the enable function as it is now using the opmode set on probe? That seems like a safer solution indeed since enable won't overwrite other values different from off read from the hardware register, I'll prepare a patch. Thanks a lot for your help and best regards, Javier
On Tue, Aug 26, 2014 at 11:08:07AM +0200, Javier Martinez Canillas wrote: > On 08/26/2014 09:17 AM, Mark Brown wrote: > > No, this doesn't make any obvious sense to me at all. Picking normal as > > a default if the hardware reads back off due to overlapping > > impelementation or something *might* make sense but not overwriting the > > hardware state without explicit permission from the machine integration > > is a key goal for the regulator API. > Just to be sure I understood you correctly, what might makes sense to you > then is to set the opmode to normal as default on probe only if off is > read back from the hardware register but leaving the enable function as it > is now using the opmode set on probe? Yes.
diff --git a/drivers/regulator/max77802.c b/drivers/regulator/max77802.c index ad1caa9..917b5ab 100644 --- a/drivers/regulator/max77802.c +++ b/drivers/regulator/max77802.c @@ -180,7 +180,7 @@ static int max77802_enable(struct regulator_dev *rdev) return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, rdev->desc->enable_mask, - max77802->opmode[id] << shift); + MAX77802_OPMODE_NORMAL << shift); } _______________________________________________