linux-kernel - Re: Re: [PATCH 1/3] mmc:sunxi-mmc:add support on discrete device power supply

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <CAPDyKFpnTk9Ky-zr-dakTZJr1N_65_6py9=3_78vwOR930apEQ@mail.gmail.com>
Date:   Fri, 31 Dec 2021 13:14:30 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     "michael@...winnertech.com" <michael@...winnertech.com>
Cc:     mripard <mripard@...nel.org>, wens <wens@...e.org>,
        samuel <samuel@...lland.org>,
        "andre.przywara" <andre.przywara@....com>,
        "jernej.skrabec" <jernej.skrabec@...il.com>,
        linux-mmc <linux-mmc@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-sunxi <linux-sunxi@...ts.linux.dev>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: Re: [PATCH 1/3] mmc:sunxi-mmc:add support on discrete device
 power supply

On Fri, 31 Dec 2021 at 09:28, michael@...winnertech.com
<michael@...winnertech.com> wrote:
>
> > From: Ulf Hansson
> > Date: 2021-12-29 00:49
> > To: Michael Wu
> > CC: mripard; wens; samuel; andre.przywara; jernej.skrabec; linux-mmc; linux-arm-kernel; linux-sunxi; linux-kernel
> > Subject: Re: [PATCH 1/3] mmc:sunxi-mmc:add support on discrete device power supply
> > On Wed, 22 Dec 2021 at 04:07, Michael Wu <michael@...winnertech.com> wrote:
> > >
> > > Because some platform has no regulator, only use discrete devices
> > > to supply power,For this situation, to use sd/mmc card, we add ocr manually
> > >
> > > Signed-off-by: Michael Wu <michael@...winnertech.com>
> > > ---
> > >  drivers/mmc/host/sunxi-mmc.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> > > index 2702736a1c57..afeefead6501 100644
> > > --- a/drivers/mmc/host/sunxi-mmc.c
> > > +++ b/drivers/mmc/host/sunxi-mmc.c
> > > @@ -1300,6 +1300,14 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
> > >         if (ret)
> > >                 return ret;
> > >
> > > +       /**
> > > +        * Some platforms has no regulator. Discrete devices are used instead.
> > > +        * To support sd/mmc card, we need to add ocr manually.
> > > +        */
> > > +       if (!host->mmc->ocr_avail)
> > > +               host->mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > > +
> >
> > Rather than doing this, I suggest you hook up a fixed vmmc regulator in the DTS.
> >
> > Nevertheless, it seems reasonable to check that the ocr_avail gets set
> > up correctly. And if it doesn't, perhaps we should print a warning and
> > return an error code.
> >
> > > +
> > >         host->reg_base = devm_platform_ioremap_resource(pdev, 0);
> > >         if (IS_ERR(host->reg_base))
> > >                 return PTR_ERR(host->reg_base);
> >
> > Kind regards
> > Uffe
>
> Dear Uffe,
> Thanks for your suggestion. It is a better solution.
> I've modified my patch. Please check if it's reasonable. If it is, I'll re-sumbit it later.
>
> ---
> Subject: [PATCH v2] mmc: sunxi-mmc: check ocr_avail on resource request
>
> Some platforms have no regulator, discrete power devices are used instead.
> However, sunxi_mmc_probe does not catch this exception when regulator is
> absent in DTS. This leads to sd or eMMC init failure.
> To solve this, a fixed vmmc regulator must be hooked up in DTS, like this:
> reg_dummy_vmmc: dummy_vmmc {
>         compatible = "regulator-fixed";
>         regulator-name = "dummy-vmmc";
>         regulator-min-microvolt = <500000>;
>         regulator-max-microvolt = <3500000>;

The min/max should be set to the same value as you can't really change
the voltage levels.

If you know the voltage level that is supplied for your platform, then
state this value - otherwise I would suggest picking 3.3V, which is
rather commonly used for MMC/SD.

> };
> mmc0:mmc@...0000 {
>         compatible = "allwinner,sun50i-a100-emmc";
>         device_type = "mmc0";
>         vmmc-supply = <&reg_dummy_vmmc>;
> }
> In this patch, we print an error message and abort the probe process if
> the regulator is not specified in DTS.
>
> Signed-off-by: Michael Wu <michael@...winnertech.com>
> ---
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 2702736..0da74bd 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -1300,6 +1300,11 @@
>         if (ret)
>                 return ret;
>
> +       if (!host->mmc->ocr_avail) {
> +               dev_err(&pdev->dev, "Could not get mmc regulator\n");
> +               return -EINVAL;
> +       }
> +
>         host->reg_base = devm_platform_ioremap_resource(pdev, 0);
>         if (IS_ERR(host->reg_base))
>                 return PTR_ERR(host->reg_base);
>
> Best Regards,
> Michael Wu

Yep, this looks good to me!

Kind regards
Uffe