Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753384AbcDBOE2 (ORCPT ); Sat, 2 Apr 2016 10:04:28 -0400 Received: from gloria.sntech.de ([95.129.55.99]:33942 "EHLO gloria.sntech.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752392AbcDBOEP convert rfc822-to-8bit (ORCPT ); Sat, 2 Apr 2016 10:04:15 -0400 From: Heiko Stuebner To: Guodong Xu Cc: Shawn Lin , Jaehoon Chung , robh+dt@kernel.org, =?utf-8?B?UGF3ZcWC?= Moll , Mark Rutland , ijc+devicetree@hellion.org.uk, Kumar Gala , Ulf Hansson , devicetree@vger.kernel.org, "linux-kernel@vger.kernel.org" , Xinwei Kong , Zhangfei Gao Subject: Re: [PATCH v3 2/2] mmc: dw_mmc: add resets support to dw_mmc Date: Sat, 02 Apr 2016 16:03:53 +0200 Message-ID: <5864557.bXRX6hBm9o@phil> User-Agent: KMail/4.14.10 (Linux/4.3.0-1-amd64; KDE/4.14.14; x86_64; ; ) In-Reply-To: References: <1459322696-29919-1-git-send-email-guodong.xu@linaro.org> <5552254.5pm4SI4ary@phil> MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT Content-Type: text/plain; charset="iso-8859-1" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3663 Lines: 117 Am Samstag, 2. April 2016, 21:39:11 schrieb Guodong Xu: > On 2 April 2016 at 02:42, Heiko Stuebner 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 > > > -#include > > > > > > #include > > > > > > +#include > > > +#include > > > +#include > > > > 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