From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BF71BC433DF for ; Tue, 13 Oct 2020 08:44:34 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 200F0208D5 for ; Tue, 13 Oct 2020 08:44:33 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="uhj2D7a1"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="rEcfkO0N" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 200F0208D5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=zgBx8ptozfmogQiFGzUbBJF+1aFN8J80lDJuicrEmAI=; b=uhj2D7a1n3xeXW6+TpDiJ9a6R NXJO+LsruIph2PCPehBV6YTp0+JWVWllc2ImjIYgbK4L4fXh6/tRHQfrfipjo6BOb/WhFk8JvgMHV jOhdJsDYPv0FVYDX7qzMmGahJVFKXN7TRf23ph0bQ+7ZEF6dHXEAY23sHO/137Te5BMNKIt+oHgvg 5cXVv9iA+QtbV2FRsMqFMMhG0EVBMca1flomoOH06gsTCbPUWLrSJhqzB+0GXzM9wEAVRkO65flu0 T6axpnRMGw3k7O7JVWbOGK0xIP9OdNcBhnih1gCtE/qHGW/Ad26Pyi8ArSwdT3Axds99Org39RfX4 xkK0V3vwg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kSFt9-0000TW-Qp; Tue, 13 Oct 2020 08:42:31 +0000 Received: from mail-vs1-xe41.google.com ([2607:f8b0:4864:20::e41]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kSFt7-0000Sa-UQ for linux-arm-kernel@lists.infradead.org; Tue, 13 Oct 2020 08:42:31 +0000 Received: by mail-vs1-xe41.google.com with SMTP id d4so4995636vsk.13 for ; Tue, 13 Oct 2020 01:42:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=z0tT/OxWzXToVda8iVCF76v2EDXGWGffzNusJWvss8M=; b=rEcfkO0NIHhLROvC5ZLcfK3iHqd8SalrVx1IENPSPL9L4RtFBwu78TU7BnEX+xg+cJ TP2HCQ4n6d55TeBhx9JsQsjtafrU9hn+eTCoGMgsb1Jq6YAGG6BvOFnj4gbfGDOm9CUr G4ySHoroeEO/K/MxVjyOD6J/IJBIGlTW8ug5DEqEPoxfo4e8AZzwxpqGovPe477I8wQx CWq4T7FOZb3HN5rNmiAvXGkr/RyeGMICqrHOW7uxrBDBq9nMjPiDIyu2uYHrj2QU7iCR V0dFbJdC++ZEw/nX1XvT2XJkZ4r53BoeC7XU6LoM2BfY3BCZ2w15qyKx1YZCVJ3GmDn0 LBow== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=z0tT/OxWzXToVda8iVCF76v2EDXGWGffzNusJWvss8M=; b=pbBRQHpRL2gXzWAeC+dGCYSg2SJxADXW/dvAfo8rDz8t0B1w3O8Y0EmD2xssg6hSel G/QUQVcLlX3DGpBncSBSintdulySFtzLHV1xLPBIvc8IuADJVDKIFLg8Vlknn/nhLsKC aeh4/LaQnH9ZMqxJ8qG6VFC3wnTokmXkzkwEBSIgGvasofVCXTeaILWF1OI2KHtTaBhW 45UaDF0l+WVkWJlElgMF7uMmXpJUchILrMDHv0ZoQoCB0zOMHGGjEt8akc2BaFwHlSDq c0bWQdcU++CVrh9S41kfkpt4PkpAZZfD6UN/m5sIWoCo3YdT0sORhScyI5XHiRe47z0E 0NFA== X-Gm-Message-State: AOAM532ssr7EKRKCrYBMS2xppMPtJt93B3gK2+AnUvzM/XvIA7Mfh2AM TCzfUqDVU/7iQuoxrYE/T7LXA/CEf32L4ZncJKQP1g== X-Google-Smtp-Source: ABdhPJz5bidkohOEL1Mad90/4Mq0kOr+hJLXG0AGnTln12QW/VucJ7OezoBQnKjDG2AqA/MAFDjYNvTyBNcAmpZBNjI= X-Received: by 2002:a67:f24e:: with SMTP id y14mr15972235vsm.55.1602578546349; Tue, 13 Oct 2020 01:42:26 -0700 (PDT) MIME-Version: 1.0 References: <20201008020936.19894-1-muhammad.husaini.zulkifli@intel.com> <20201008020936.19894-5-muhammad.husaini.zulkifli@intel.com> In-Reply-To: From: Ulf Hansson Date: Tue, 13 Oct 2020 10:41:49 +0200 Message-ID: Subject: Re: [PATCH v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC To: "Zulkifli, Muhammad Husaini" X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201013_044229_992725_636DA879 X-CRM114-Status: GOOD ( 45.57 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "Shevchenko, Andriy" , Arnd Bergmann , "Raja Subramanian, Lakshmi Bai" , "linux-mmc@vger.kernel.org" , Linux Kernel Mailing List , "Hunter, Adrian" , "Wan Mohamad, Wan Ahmad Zainie" , Michal Simek , Linux ARM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, 9 Oct 2020 at 19:50, Zulkifli, Muhammad Husaini wrote: > > Hi, > > >-----Original Message----- > >From: Ulf Hansson > >Sent: Friday, October 9, 2020 2:56 PM > >To: Zulkifli, Muhammad Husaini > >Cc: Hunter, Adrian ; Michal Simek > >; Shevchenko, Andriy > >; linux-mmc@vger.kernel.org; Linux ARM > >; Linux Kernel Mailing List >kernel@vger.kernel.org>; Raja Subramanian, Lakshmi Bai > >; Wan Mohamad, Wan Ahmad > >Zainie ; Arnd Bergmann > > > >Subject: Re: [PATCH v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for > >Keem Bay SOC > > > >On Thu, 8 Oct 2020 at 19:21, Zulkifli, Muhammad Husaini > > wrote: > >> > >> Hi, > >> > >> >-----Original Message----- > >> >From: Ulf Hansson > >> >Sent: Thursday, October 8, 2020 11:19 PM > >> >To: Zulkifli, Muhammad Husaini > >> >Cc: Hunter, Adrian ; Michal Simek > >> >; Shevchenko, Andriy > >> >; linux-mmc@vger.kernel.org; Linux ARM > >> >; Linux Kernel Mailing List > >> >; Raja Subramanian, Lakshmi Bai > >> >; Wan Mohamad, Wan Ahmad > >> >Zainie ; Arnd Bergmann > >> > > >> >Subject: Re: [PATCH v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 > >> >support for Keem Bay SOC > >> > > >> >On Thu, 8 Oct 2020 at 12:54, Zulkifli, Muhammad Husaini > >> > wrote: > >> >> > >> >> Hi, > >> >> > >> >> >-----Original Message----- > >> >> >From: Ulf Hansson > >> >> >Sent: Thursday, October 8, 2020 5:28 PM > >> >> >To: Zulkifli, Muhammad Husaini > >> >> > > >> >> >Cc: Hunter, Adrian ; Michal Simek > >> >> >; Shevchenko, Andriy > >> >> >; linux-mmc@vger.kernel.org; Linux > >> >> >ARM ; Linux Kernel Mailing > >> >> >List > >> >> >; Raja Subramanian, Lakshmi Bai > >> >> >; Wan Mohamad, Wan > >Ahmad > >> >> >Zainie ; Arnd Bergmann > >> >> > > >> >> >Subject: Re: [PATCH v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 > >> >> >support for Keem Bay SOC > >> >> > > >> >> >On Thu, 8 Oct 2020 at 04:12, > >> >wrote: > >> >> >> > >> >> >> From: Muhammad Husaini Zulkifli > >> >> >> > >> >> >> > >> >> >> Voltage switching sequence is needed to support UHS-1 interface. > >> >> >> There are 2 places to control the voltage. > >> >> >> 1) By setting the AON register using firmware driver calling > >> >> >> system-level platform management layer (SMC) to set the register. > >> >> >> 2) By controlling the GPIO expander value to drive either 1.8V > >> >> >> or 3.3V for power mux input. > >> >> >> > >> >> >> Signed-off-by: Muhammad Husaini Zulkifli > >> >> >> > >> >> >> Reviewed-by: Andy Shevchenko > >> >> >> Reviewed-by: Adrian Hunter > >> >> >> --- > >> >> >> drivers/mmc/host/sdhci-of-arasan.c | 126 > >> >> >> +++++++++++++++++++++++++++++ > >> >> >> 1 file changed, 126 insertions(+) > >> >> >> > >> >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c > >> >> >> b/drivers/mmc/host/sdhci-of-arasan.c > >> >> >> index 46aea6516133..ea2467b0073d 100644 > >> >> >> --- a/drivers/mmc/host/sdhci-of-arasan.c > >> >> >> +++ b/drivers/mmc/host/sdhci-of-arasan.c > >> >> >> @@ -16,6 +16,7 @@ > >> >> >> */ > >> >> >> > >> >> >> #include > >> >> >> +#include > >> >> >> #include > >> >> >> #include > >> >> >> #include > >> >> >> @@ -23,6 +24,7 @@ > >> >> >> #include > >> >> >> #include > >> >> >> #include > >> >> >> +#include > >> >> >> > >> >> >> #include "cqhci.h" > >> >> >> #include "sdhci-pltfm.h" > >> >> >> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data { > >> >> >> * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. > >> >> >> * @soc_ctl_map: Map to get offsets into soc_ctl registers. > >> >> >> * @quirks: Arasan deviations from spec. > >> >> >> + * @uhs_gpio: Pointer to the uhs gpio. > >> >> >> */ > >> >> >> struct sdhci_arasan_data { > >> >> >> struct sdhci_host *host; @@ -150,6 +153,7 @@ struct > >> >> >> sdhci_arasan_data { > >> >> >> struct regmap *soc_ctl_base; > >> >> >> const struct sdhci_arasan_soc_ctl_map *soc_ctl_map; > >> >> >> unsigned int quirks; > >> >> >> + struct gpio_desc *uhs_gpio; > >> >> >> > >> >> >> /* Controller does not have CD wired and will not function > >> >> >> normally without > >> >> >*/ > >> >> >> #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST BIT(0) > >> >> >> @@ -361,6 +365,112 @@ static int > >> >> >> sdhci_arasan_voltage_switch(struct > >> >> >mmc_host *mmc, > >> >> >> return -EINVAL; > >> >> >> } > >> >> >> > >> >> >> +static int sdhci_arasan_keembay_voltage_switch(struct mmc_host > >> >*mmc, > >> >> >> + struct mmc_ios *ios) { > >> >> >> + struct sdhci_host *host = mmc_priv(mmc); > >> >> >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > >> >> >> + struct sdhci_arasan_data *sdhci_arasan = > >> >sdhci_pltfm_priv(pltfm_host); > >> >> >> + u16 ctrl_2, clk; > >> >> >> + int ret; > >> >> >> + > >> >> >> + switch (ios->signal_voltage) { > >> >> >> + case MMC_SIGNAL_VOLTAGE_180: > >> >> >> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > >> >> >> + clk &= ~SDHCI_CLOCK_CARD_EN; > >> >> >> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > >> >> >> + > >> >> >> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > >> >> >> + if (clk & SDHCI_CLOCK_CARD_EN) > >> >> >> + return -EAGAIN; > >> >> >> + > >> >> >> + sdhci_writeb(host, SDHCI_POWER_ON | SDHCI_POWER_180, > >> >> >> + SDHCI_POWER_CONTROL); > >> >> >> + > >> >> >> + /* > >> >> >> + * Set VDDIO_B voltage to Low for 1.8V > >> >> >> + * which is controlling by GPIO Expander. > >> >> >> + */ > >> >> >> + gpiod_set_value_cansleep(sdhci_arasan->uhs_gpio, > >> >> >> + 0); > >> >> >> + > >> >> >> + /* > >> >> >> + * This is like a final gatekeeper. Need to > >> >> >> + ensure changed > >> >voltage > >> >> >> + * is settled before and after turn on this bit. > >> >> >> + */ > >> >> >> + usleep_range(1000, 1100); > >> >> >> + > >> >> >> + ret = > >> >keembay_sd_voltage_selection(KEEMBAY_SET_1V8_VOLT); > >> >> >> + if (ret) > >> >> >> + return ret; > >> >> >> + > >> >> >> + usleep_range(1000, 1100); > >> >> > > >> >> >No, sorry, but I don't like this. > >> >> > > >> >> >This looks like a GPIO regulator with an extension of using the > >> >> >keembay_sd_voltage_selection() thingy. I think you can model these > >> >> >things behind a regulator and hook it up as a vqmmc supply in DT > >> >> >instead. BTW, this is the common way we deal with these things for > >> >> >mmc > >> >host drivers. > >> >> > >> >> The SDcard for Keem Bay SOC does not have its own voltage regulator. > >> >> There are 2 places to control the voltage. > >> >> 1) By setting the AON register calling system-level platform > >> >> management > >> >layer (SMC) > >> >> to set the I/O pads voltage for particular GPIOs line for clk,data and cmd. > >> >> The reason why I use this keembay_sd_voltage_selection() via > >> >> smccc > >> >interface it because during voltage switching > >> >> I need to access to AON register. On a secure system, we could > >> >> not > >> >directly access to AON register due to some security concern from > >> >driver side, thus > >> >> cannot exposed any register or address. > >> >> 2) By controlling the GPIO expander value to drive either 1.8V or > >> >> 3.3V for > >> >power mux input. > >> > > >> >I see, thanks for clarifying. > >> > > >> >To me, it sounds like the best fit is to implement a pinctrl (to > >> >manage the I/O > >> >pads) and a GPIO regulator. > >> > > >> Even with pinctrl, i still need to use the keembay_sd_voltage_selection() > >thingy for AON register. > > > >Yes, I am fine by that. > > > >Although, as it's really a pinctrl, it deserves to be modelled like that. Not as a > >soc specific hack in a mmc host driver. > > > >> Plus, the GPIO pin that control the sd-voltage is in GPIO Expander not using > >Keembay SOC GPIO Pin. > >> The best option is using the gpio consumer function to toggle the pin. > > > >As I said, please no. > > > >The common way to model this is as a GPIO regulator. In this way, you can even > >rely on existing mmc DT bindings. All you have to do is to hook up a vqmmc > >supply to the mmc node. > > > >To be clear, as long as there are no arguments for why a pinctrl and GPIO > >regulator can't be used - I am not going to pick up the patches. > As I mentioned The SDcard does not have its own voltage regulator. > It only uses the voltage rails on the mux input. > > There are 2 things need to be configured before getting the output voltage: > > 1) V_VDDIO_B : > Supplied voltage applied to I/O Rail which is controlled from the Always on domain using specific bits in AON_CFG1 register. > This is where we set for V_VDDIO_B using the keembay_sd_voltage_selection() to set either 1.8v or 3.3v depending on the bit value. > IMHO, we do not pinctrl to do this. > > 2) V_VDDIO_B_MAIN: > The output V_VDDIO_B_MAIN (OUT1) will be either V_3P3_MAIN (IN1) or V_1P8_MAIN (IN2), > depending on the state of GPIO expander Pin value. There is a POWER MUX involving here. > IMHO, we do not need any gpio regulator/regulator api hook up for this. > Most important thing, there is no regulator ic at all. > We still need to manually control and toggle the pin value. > > The final IO voltage is set by V_VDDIO_B (= V_VDDIO_B_MAIN after passing through voltage sense resistor). > > Hope this will clarify. I think I get it, thanks. Again, I haven't seen any reasons for why this can't be modelled as a pinctrl and a gpio-regulator. So, please convert it to that. Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel