linux-kernel - Re: [PATCH v3 2/2] mmc: dw_mmc: add resets support to dw

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>] [thread-next>] [day] [month] [year] [list]

Message-ID: <5864557.bXRX6hBm9o@phil>
Date:	Sat, 02 Apr 2016 16:03:53 +0200
From:	Heiko Stuebner <heiko@...ech.de>
To:	Guodong Xu <guodong.xu@...aro.org>
Cc:	Shawn Lin <shawn.lin@...k-chips.com>,
	Jaehoon Chung <jh80.chung@...sung.com>, robh+dt@...nel.org,
	Paweł Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	ijc+devicetree@...lion.org.uk, Kumar Gala <galak@...eaurora.org>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	devicetree@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Xinwei Kong <kong.kongxinwei@...ilicon.com>,
	Zhangfei Gao <zhangfei.gao@...aro.org>
Subject: Re: [PATCH v3 2/2] mmc: dw_mmc: add resets support to dw_mmc

Am Samstag, 2. April 2016, 21:39:11 schrieb Guodong Xu:
> On 2 April 2016 at 02:42, Heiko Stuebner <heiko@...ech.de> wrote:
> > Am Mittwoch, 30. März 2016, 15:24:56 schrieb Guodong Xu:
> > 
> > [...]
> > 
> > > @@ -2949,7 +2956,9 @@ int dw_mci_probe(struct dw_mci *host)
> > > 
> > >       if (!host->pdata) {
> > >       
> > >               host->pdata = dw_mci_parse_dt(host);
> > > 
> > > -             if (IS_ERR(host->pdata)) {
> > > +             if (PTR_ERR(host->pdata) == -EPROBE_DEFER) {
> > > +                     return -EPROBE_DEFER;
> > > +             } else if (IS_ERR(host->pdata)) {
> > 
> > how is this related to adding the reset handling?
> 
> I added this into dw_mci_parse_dt(host), and that's the first time it may
> return -EPROBE_DEFER
> 
>   /* find reset controller when exist */
>   pdata->rstc = devm_reset_control_get_optional(dev, NULL);
> 
> So, I added processing to this error in this patch.

ah, you're right of course


> > Making the driver handle probe deferral better should be a separate
> > patch.> 
> > >                       dev_err(host->dev, "platform data not
> > 
> > available\n");
> > 
> > >                       return -EINVAL;
> > >               
> > >               }
> > > 
> > > @@ -3012,6 +3021,9 @@ int dw_mci_probe(struct dw_mci *host)
> > > 
> > >               }
> > >       
> > >       }
> > > 
> > > +     if (!IS_ERR(host->pdata->rstc))
> > > +             reset_control_deassert(host->pdata->rstc);
> > > +
> > 
> > Wouldn't reset_control_reset be better? The way it is now it would
> > expect
> > the reset to be asserted somewhere else before dw_mmc probes?
> 
> It relates to how the SoC's reset logic is like. One bit set can clear all
> dw_mmc host controller registers. It doesn't need do assert then
> deassert.
> 
> That's what I saw in hi6220 (it integrates three dw_mmc host controller),
> drivers/reset/hisilicon/hi6220_reset.c
> , which I wrote this patch for.

I just realized again that reset_control_reset is a completely separate 
operation (not related to assert / deassert).

What I was originally getting at is that I don't see any assert-counterpart. 
So if the reset-control is already deasserted, nothing will happen on some 
designs.

For example on Rockchip SoCs the reset controller needs the signal to be 
high to assert the reset and the dw_mmc part of the manual explicitly says 
that the "reset_n should be asserted(active-low) for at least two clocks of 
clk or cclk_in".

So I would expect something like

	reset_control_assert(reset);
	usleep_range(x, y);
	reset_control_deassert(reset);

instead of only trying to deassert the reset.


> > >       setup_timer(&host->cmd11_timer,
> > >       
> > >                   dw_mci_cmd11_timer, (unsigned long)host);
> > 
> > [...]
> > 
> > > diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> > > index 7b41c6d..b95cd84 100644
> > > --- a/include/linux/mmc/dw_mmc.h
> > > +++ b/include/linux/mmc/dw_mmc.h
> > > @@ -14,9 +14,10 @@
> > > 
> > >  #ifndef LINUX_MMC_DW_MMC_H
> > >  #define LINUX_MMC_DW_MMC_H
> > > 
> > > -#include <linux/scatterlist.h>
> > > -#include <linux/mmc/core.h>
> > > 
> > >  #include <linux/dmaengine.h>
> > > 
> > > +#include <linux/mmc/core.h>
> > > +#include <linux/reset.h>
> > > +#include <linux/scatterlist.h>
> > 
> > unrelated changed regarding the reordering of includes.
> 
> Making them in the order of alphabetic. If you dislike, I will not add.

It's Jaehoon's call and that change above is pretty small, but generally 
introducing things unrelated to the change you actually want to make is not 
that nice - that's what separate patches are for :-) .


Heiko