Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC

[Lad, Prabhakar]

Hi Wolfram,

Thank you for the review.

On Mon, Jun 17, 2024 at 9:31 AM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> > - Ive modelled the regulator now to control the PWEN aswell.
>
> Yay, this looks much better. Good work!
>
> > - I have still kept regulator bits in quirks I was wondering if I should
> >   move this to renesas_sdhi_of_data instead?
>
> I think so. An internal regulator is not a quirk.
>
Agreed.

> > - I still need to add checks if the internal regulator used and
> >   only then call regulator_enable/regulator_set_voltage. ATM I am still
> >   unclear on differentiating if internal/external regulator is used.
>
> When it comes to re-enabling the regulator in sdhi_reset, I think this
> can be a sdhi_flag like SDHI_FLAG_ENABLE_REGULATOR_IN_RESET or alike.
>
OK.

> When it comes to the regulator, I wonder if it wouldn't be clearer to
> replace renesas_sdhi_internal_dmac_register_regulator() with a proper
> probe function and a dedicated compatible value for it. We could use
> platform_driver_probe() to instantiate the new driver within the SDHI
> probe function. This will ensure that the regulator driver will only be
> started once the main driver got all needed resources (mapped
> registers).
>
I did give it a try with platform_driver_probe() and failed.

- Firstly I had to move the regulator node outside the SDHI node for
platform_driver_probe() to succeed or else it failed with -ENODEV (at
https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L953)
- In Renesas SoCs we have multiple instances of SDHI, the problem
being for each instance we are calling platform_driver_probe(). Which
causes a problem as the regulator node will use the first device.

Let me know if I have missed something obvious here.

> My gut feeling is that it will pay off if the internal regulator will be
> described in DT as any other regulator. Like, we could name the
> regulator in DT as always etc...
>
> More opinions on this idea are welcome, though...
>
> > --- a/drivers/mmc/host/renesas_sdhi.h
> > +++ b/drivers/mmc/host/renesas_sdhi.h
> > @@ -11,6 +11,9 @@
> >
> >  #include <linux/dmaengine.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/regmap.h>
>
> Regmap can luckily go now.
>
Agreed.

> > +#include <linux/regulator/driver.h>
> > +#include <linux/regulator/machine.h>
> >  #include "tmio_mmc.h"
> >
> >  struct renesas_sdhi_scc {
> > @@ -49,6 +52,9 @@ struct renesas_sdhi_quirks {
> >       bool manual_tap_correction;
> >       bool old_info1_layout;
> >       u32 hs400_bad_taps;
> > +     bool internal_regulator;
> > +     struct regulator_desc *rdesc;
> > +     struct regulator_init_data *reg_init_data;
> >       const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX];
> >  };
> >
> > @@ -93,6 +99,8 @@ struct renesas_sdhi {
> >       unsigned int tap_set;
> >
> >       struct reset_control *rstc;
> > +
> > +     struct regulator_dev *sd_status;
>
> This is a strange name for the regulater. Especially given that you have
> as well the more fitting 'u32 sd_status' in the code later.
>
I will update it.

> ...
>
> > +static struct regulator_init_data r9a09g057_regulator_init_data = {
> > +     .constraints = {
> > +             .valid_ops_mask = REGULATOR_CHANGE_STATUS,
>
> Don't we need REGULATOR_CHANGE_VOLTAGE here as well? Or is this implicit
> because of REGULATOR_VOLTAGE? Can't find this, though.
>
I will investigate it.

Cheers,
Prabhakar