Re: [RESEND PATCH v5 2/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan

[Shawn Lin]

Hi Adrian,

On 2016/3/4 20:12, Adrian Hunter wrote:

[...]

> > > I don't know that disabling clocks is necessarily the right thing to do if
> > > the resume fails.  You might want to consider what happens if the system
> > > tries to use the device when it is in that state.  It's been a long time
> > > since I did any ARM work but aren't you risking bus errors.  Might just as
> > > well not bother and save a few lines of code.
> >
> > Firstly, I really just follow the orginal err handle.
> > It's risking bus errors if disabling ahb clk, but ahb clk is most very
> > likely to shared by many controllers. So regarding to the clk reference
> > count, the ahb_clk is not *really* disabled and just decrease the
> > reference count. How about keep these handles here?
>
> I would remove the error handling on the grounds that it does not do
> anything useful.

Take another ronud of thinking it, probably you are right. When failing
to resume from deelp sleep, we return err to dpm. dpm still roll-forward
to wakeup by calling resume callback one-by-one. It means that finally
when system in wakeup state with disabled ahb_clk, but we then going to
access mmc as normal, which is not acceptable. Especially for debuging
the failure, we have to enable ahb_clk manually.

Will remove the error handling.

Thanks.

> > > > +    return ret;
> > > >    }
> > > >    #endif /* ! CONFIG_PM_SLEEP */
> > > >
> > > > @@ -183,6 +207,30 @@ static int sdhci_arasan_probe(struct platform_device
> > > > *pdev)
> > > >            goto clk_disable_all;
> > > >        }
> > > >
> > > > +    sdhci_arasan->phy = ERR_PTR(-ENODEV);
> > > > +    if (of_device_is_compatible(pdev->dev.of_node,
> > > > +                    "arasan,sdhci-5.1")) {
> > > > +        sdhci_arasan->phy = devm_phy_get(&pdev->dev,
> > > > +                         "phy_arasan");
> > > > +        if (IS_ERR(sdhci_arasan->phy)) {
> > > > +            ret = PTR_ERR(sdhci_arasan->phy);
> > > > +            dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
> > > > +            goto clk_disable_all;
> > > > +        }
> > > > +
> > > > +        ret = phy_init(sdhci_arasan->phy);
> > > > +        if (ret < 0) {
> > > > +            dev_err(&pdev->dev, "phy_init err.\n");
> > > > +            goto clk_disable_all;
> > > > +        }
> > > > +
> > > > +        ret = phy_power_on(sdhci_arasan->phy);
> > > > +        if (ret < 0) {
> > > > +            dev_err(&pdev->dev, "phy_power_on err.\n");
> > > > +            goto err_phy_power;
> > > > +        }
> > > > +    }
> > > > +
> > > >        ret = sdhci_add_host(host);
> > > >        if (ret)
> > > >            goto err_pltfm_free;
> > > > @@ -190,7 +238,12 @@ static int sdhci_arasan_probe(struct platform_device
> > > > *pdev)
> > > >        return 0;
> > > >
> > > >    err_pltfm_free:
> > > > +    if (!IS_ERR(sdhci_arasan->phy))
> > > > +        phy_power_off(sdhci_arasan->phy);
> > > >        sdhci_pltfm_free(pdev);
> > > > +err_phy_power:
> > > > +    if (!IS_ERR(sdhci_arasan->phy))
> > >
> > > Looks like you are using sdhci_arasan after it has been free'd by
> > > sdhci_pltfm_free().
> > >
> > > Also there seem to be cases where sdhci_arasan_probe() can exit after
> > > sdhci_pltfm_init() without calling sdhci_pltfm_free().
> >
> > Good catch, will fix it.
> >
> > > > +        phy_exit(sdhci_arasan->phy);
> > > >    clk_disable_all:
> > > >        clk_disable_unprepare(clk_xin);
> > > >    clk_dis_ahb:
> > > > @@ -205,6 +258,11 @@ static int sdhci_arasan_remove(struct
> > > > platform_device *pdev)
> > > >        struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > >        struct sdhci_arasan_data *sdhci_arasan = pltfm_host->priv;
> > > >
> > > > +    if (!IS_ERR(sdhci_arasan->phy)) {
> > > > +        phy_power_off(sdhci_arasan->phy);
> > > > +        phy_exit(sdhci_arasan->phy);
> > > > +    }
> > > > +
> > >
> > > When you re-base, take care to keep this chunk above
> > > sdhci_pltfm_unregister().
> >
> > sure.
> >
> > Thanks for reviewing it.
> >
> > > >        clk_disable_unprepare(sdhci_arasan->clk_ahb);
> > > >
> > > >        return sdhci_pltfm_unregister(pdev);


--
Best Regards
Shawn Lin