Message-ID: <DM6PR11MB2876BF8F8549486D151755C9B8ED0@DM6PR11MB2876.namprd11.prod.outlook.com>
Date:   Fri, 6 Nov 2020 07:16:08 +0000
From:   "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
CC:     "Hunter, Adrian" <adrian.hunter@...el.com>,
        Michal Simek <michal.simek@...inx.com>,
        "Shevchenko, Andriy" <andriy.shevchenko@...el.com>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Raja Subramanian, Lakshmi Bai" 
        <lakshmi.bai.raja.subramanian@...el.com>,
        "Wan Mohamad, Wan Ahmad Zainie" 
        <wan.ahmad.zainie.wan.mohamad@...el.com>,
        Arnd Bergmann <arnd@...db.de>
Subject: RE: [PATCH v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
 Keem Bay SOC

Hi,

>-----Original Message-----
>From: Ulf Hansson <ulf.hansson@...aro.org>
>Sent: Tuesday, October 13, 2020 4:42 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@...el.com>
>Cc: Hunter, Adrian <adrian.hunter@...el.com>; Michal Simek
><michal.simek@...inx.com>; Shevchenko, Andriy
><andriy.shevchenko@...el.com>; linux-mmc@...r.kernel.org; Linux ARM
><linux-arm-kernel@...ts.infradead.org>; Linux Kernel Mailing List <linux-
>kernel@...r.kernel.org>; Raja Subramanian, Lakshmi Bai
><lakshmi.bai.raja.subramanian@...el.com>; Wan Mohamad, Wan Ahmad
>Zainie <wan.ahmad.zainie.wan.mohamad@...el.com>; Arnd Bergmann
><arnd@...db.de>
>Subject: Re: [PATCH v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>On Fri, 9 Oct 2020 at 19:50, Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@...el.com> wrote:
>>
>> Hi,
>>
>> >-----Original Message-----
>> >From: Ulf Hansson <ulf.hansson@...aro.org>
>> >Sent: Friday, October 9, 2020 2:56 PM
>> >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@...el.com>
>> >Cc: Hunter, Adrian <adrian.hunter@...el.com>; Michal Simek
>> ><michal.simek@...inx.com>; Shevchenko, Andriy
>> ><andriy.shevchenko@...el.com>; linux-mmc@...r.kernel.org; Linux ARM
>> ><linux-arm-kernel@...ts.infradead.org>; Linux Kernel Mailing List
>> ><linux- kernel@...r.kernel.org>; Raja Subramanian, Lakshmi Bai
>> ><lakshmi.bai.raja.subramanian@...el.com>; Wan Mohamad, Wan Ahmad
>> >Zainie <wan.ahmad.zainie.wan.mohamad@...el.com>; Arnd Bergmann
>> ><arnd@...db.de>
>> >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
>> ><muhammad.husaini.zulkifli@...el.com> wrote:
>> >>
>> >> Hi,
>> >>
>> >> >-----Original Message-----
>> >> >From: Ulf Hansson <ulf.hansson@...aro.org>
>> >> >Sent: Thursday, October 8, 2020 11:19 PM
>> >> >To: Zulkifli, Muhammad Husaini
>> >> ><muhammad.husaini.zulkifli@...el.com>
>> >> >Cc: Hunter, Adrian <adrian.hunter@...el.com>; Michal Simek
>> >> ><michal.simek@...inx.com>; Shevchenko, Andriy
>> >> ><andriy.shevchenko@...el.com>; linux-mmc@...r.kernel.org; Linux
>> >> >ARM <linux-arm-kernel@...ts.infradead.org>; Linux Kernel Mailing
>> >> >List
>> >> ><linux- kernel@...r.kernel.org>; Raja Subramanian, Lakshmi Bai
>> >> ><lakshmi.bai.raja.subramanian@...el.com>; Wan Mohamad, Wan
>Ahmad
>> >> >Zainie <wan.ahmad.zainie.wan.mohamad@...el.com>; Arnd Bergmann
>> >> ><arnd@...db.de>
>> >> >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
>> >> ><muhammad.husaini.zulkifli@...el.com> wrote:
>> >> >>
>> >> >> Hi,
>> >> >>
>> >> >> >-----Original Message-----
>> >> >> >From: Ulf Hansson <ulf.hansson@...aro.org>
>> >> >> >Sent: Thursday, October 8, 2020 5:28 PM
>> >> >> >To: Zulkifli, Muhammad Husaini
>> >> >> ><muhammad.husaini.zulkifli@...el.com>
>> >> >> >Cc: Hunter, Adrian <adrian.hunter@...el.com>; Michal Simek
>> >> >> ><michal.simek@...inx.com>; Shevchenko, Andriy
>> >> >> ><andriy.shevchenko@...el.com>; linux-mmc@...r.kernel.org; Linux
>> >> >> >ARM <linux-arm-kernel@...ts.infradead.org>; Linux Kernel
>> >> >> >Mailing List
>> >> >> ><linux- kernel@...r.kernel.org>; Raja Subramanian, Lakshmi Bai
>> >> >> ><lakshmi.bai.raja.subramanian@...el.com>; Wan Mohamad, Wan
>> >Ahmad
>> >> >> >Zainie <wan.ahmad.zainie.wan.mohamad@...el.com>; Arnd
>Bergmann
>> >> >> ><arnd@...db.de>
>> >> >> >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,
>> >> >> ><muhammad.husaini.zulkifli@...el.com>
>> >> >wrote:
>> >> >> >>
>> >> >> >> From: Muhammad Husaini Zulkifli
>> >> >> >> <muhammad.husaini.zulkifli@...el.com>
>> >> >> >>
>> >> >> >> 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
>> >> >> >> <muhammad.husaini.zulkifli@...el.com>
>> >> >> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...el.com>
>> >> >> >> Reviewed-by: Adrian Hunter <adrian.hunter@...el.com>
>> >> >> >> ---
>> >> >> >>  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 <linux/clk-provider.h>
>> >> >> >> +#include <linux/gpio/consumer.h>
>> >> >> >>  #include <linux/mfd/syscon.h>  #include <linux/module.h>
>> >> >> >> #include <linux/of_device.h> @@ -23,6 +24,7 @@  #include
>> >> >> >> <linux/regmap.h>  #include <linux/of.h>  #include
>> >> >> >> <linux/firmware/xlnx-zynqmp.h>
>> >> >> >> +#include <linux/firmware/intel/keembay_firmware.h>
>> >> >> >>
>> >> >> >>  #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.

I've implemented based on gpio regulator modelling.
but for pinctrl modelling, can we invite any pinctrl maintainer in this discussion whether they are agree with what 
we discuss here to implement the SMCC SIP services in the pinctrl itself.
Maybe they can suggest a way on how to do it? Is it feasible to do that?

>
>Kind regards
>Uffe

Powered by blists - more mailing lists