From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754422AbcEYNd7 (ORCPT ); Wed, 25 May 2016 09:33:59 -0400 Received: from mail-io0-f172.google.com ([209.85.223.172]:36603 "EHLO mail-io0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754293AbcEYNd4 convert rfc822-to-8bit (ORCPT ); Wed, 25 May 2016 09:33:56 -0400 MIME-Version: 1.0 In-Reply-To: <5864557.bXRX6hBm9o@phil> References: <1459322696-29919-1-git-send-email-guodong.xu@linaro.org> <5552254.5pm4SI4ary@phil> <5864557.bXRX6hBm9o@phil> Date: Wed, 25 May 2016 21:33:55 +0800 Message-ID: Subject: Re: [PATCH v3 2/2] mmc: dw_mmc: add resets support to dw_mmc From: Guodong Xu To: Heiko Stuebner Cc: Shawn Lin , Jaehoon Chung , Rob Herring , =?UTF-8?Q?Pawe=C5=82_Moll?= , Mark Rutland , Ian Campbell , Kumar Gala , Ulf Hansson , devicetree@vger.kernel.org, "linux-kernel@vger.kernel.org" , Xinwei Kong , Zhangfei Gao Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2 April 2016 at 22:03, Heiko Stuebner wrote: > 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. > After confirmation with SoC hardware engineer, yeah, a correct _assert action is expected. I will modify it as the above. Regarding usleep_range(x, y) values, here is suggestion: + usleep_range(10, 50); /* 1/400kHz = 2.5us */ 400kHz is the minimal bus speed for MMC. It stands for 2.5us per cycle. 10us is 4 cycles, and 50us is 20 cycles. Does this setting make sense to you? > >> > > 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 :-) . Got your point. I will remove this. Make it simple. -Guodong > > > Heiko