Re: [PATCH V2 23/27] mmc: mmci: add variant property to request a reset

[Ludovic BARRE]

hi Ulf

On 10/01/2018 11:30 AM, Ulf Hansson wrote:
> On 21 September 2018 at 11:46, Ludovic Barre <ludovic.Barre@xxxxxx> wrote:
> > From: Ludovic Barre <ludovic.barre@xxxxxx>
> >
> > Some variants could require a reset.
> > STM32 sdmmc variant needs to reset hardware block
> > during the power cycle procedure (for re-initialization)
> >
> > Signed-off-by: Ludovic Barre <ludovic.barre@xxxxxx>
> > ---
> >   Documentation/devicetree/bindings/mmc/mmci.txt | 2 ++
> >   drivers/mmc/host/mmci.c                        | 9 +++++++++
> >   drivers/mmc/host/mmci.h                        | 4 ++++
> >   3 files changed, 15 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/mmci.txt b/Documentation/devicetree/bindings/mmc/mmci.txt
> > index 03796cf..e952707 100644
> > --- a/Documentation/devicetree/bindings/mmc/mmci.txt
> > +++ b/Documentation/devicetree/bindings/mmc/mmci.txt
> > @@ -11,6 +11,8 @@ Required properties:
> >   - compatible             : contains "arm,pl18x", "arm,primecell".
> >   - vmmc-supply            : phandle to the regulator device tree node, mentioned
> >                              as the VCC/VDD supply in the eMMC/SD specs.
> > +depend of variant:
> > +- resets                 : phandle to internal reset line.
>
> This looks like a thing depending on the SoC, rather than on the
> variant. Therefore, I suggest you move this to be an optional DT
> property.

could be replaced by:

Optional properties:
- resets                 : phandle to internal reset line.
			   Should be defined for sdmmc variant.

> Also, please move the DT doc update into a separate patch.

OK

> >   Optional properties:
> >   - arm,primecell-periphid : contains the PrimeCell Peripheral ID, it overrides
> > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> > index b1d5bc5..6b3c33f 100644
> > --- a/drivers/mmc/host/mmci.c
> > +++ b/drivers/mmc/host/mmci.c
> > @@ -37,6 +37,7 @@
> >   #include <linux/pm_runtime.h>
> >   #include <linux/types.h>
> >   #include <linux/pinctrl/consumer.h>
> > +#include <linux/reset.h>
> >
> >   #include <asm/div64.h>
> >   #include <asm/io.h>
> > @@ -1854,6 +1855,14 @@ static int mmci_probe(struct amba_device *dev,
> >
> >          dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
> >
> > +       if (variant->reset) {
> > +               host->rst = devm_reset_control_get_exclusive(&dev->dev, NULL);
>
> As suggested, let's make this optional and not depending on the variant.

I done like that because is required for my variant (if no reset, no 
power cycle for sdmmc).

If you prefer, I could move to optional with 
"devm_reset_control_get_optional_exclusive"
And I add a comment in mmci dt binding to specify that not optional
for sdmmc. (see above)

> > +               if (IS_ERR(host->rst)) {
> > +                       ret = PTR_ERR(host->rst);
> > +                       goto clk_disable;
> > +               }
> > +       }
> > +
> >          /* Get regulators and the supported OCR mask */
> >          ret = mmc_regulator_get_supply(mmc);
> >          if (ret)
> > diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> > index e130689..f0fa185 100644
> > --- a/drivers/mmc/host/mmci.h
> > +++ b/drivers/mmc/host/mmci.h
> > @@ -247,6 +247,7 @@ struct mmci_host;
> >    * @start_err: bitmask identifying the STARTBITERR bit inside MMCISTATUS
> >    *            register.
> >    * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register
> > + * @reset: true if variant has need reset signal.
> >    */
> >   struct variant_data {
> >          unsigned int            clkreg;
> > @@ -285,6 +286,7 @@ struct variant_data {
> >          bool                    qcom_dml;
> >          bool                    reversed_irq_handling;
> >          bool                    mmcimask1;
> > +       bool                    reset;
> >          unsigned int            irq_pio_mask;
> >          u32                     start_err;
> >          u32                     opendrain;
> > @@ -318,6 +320,8 @@ struct mmci_host {
> >          struct clk              *clk;
> >          bool                    singleirq;
> >
> > +       struct reset_control    *rst;
> > +
> >          spinlock_t              lock;
> >
> >          unsigned int            mclk;
> > --
> > 2.7.4
>
> Kind regards
> Uffe