Message-ID: <8af434fd-01fe-11e4-20c0-791b915acc9a@xilinx.com>
Date:   Thu, 1 Oct 2020 11:25:11 +0200
From:   Michal Simek <michal.simek@...inx.com>
To:     "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>,
        Michal Simek <michal.simek@...inx.com>,
        "Hunter, Adrian" <adrian.hunter@...el.com>,
        "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>
Cc:     "Raja Subramanian, Lakshmi Bai" 
        <lakshmi.bai.raja.subramanian@...el.com>,
        "Wan Mohamad, Wan Ahmad Zainie" 
        <wan.ahmad.zainie.wan.mohamad@...el.com>
Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Enable UHS-1 support for
 Keem Bay SOC

Hi,

On 01. 10. 20 11:09, Zulkifli, Muhammad Husaini wrote:
> Hi Michal,
> 
>> -----Original Message-----
>> From: Michal Simek <michal.simek@...inx.com>
>> Sent: Wednesday, September 23, 2020 2:20 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@...el.com>;
>> Michal Simek <michal.simek@...inx.com>; Hunter, Adrian
>> <adrian.hunter@...el.com>; ulf.hansson@...aro.org; linux-
>> mmc@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
>> kernel@...r.kernel.org; Arnd Bergmann <arnd@...db.de>
>> Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@...el.com>;
>> Wan Mohamad, Wan Ahmad Zainie
>> <wan.ahmad.zainie.wan.mohamad@...el.com>
>> Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Enable UHS-1 support for
>> Keem Bay SOC
>>
>> Hi,
>>
>> On 22. 09. 20 20:38, Zulkifli, Muhammad Husaini wrote:
>>> Hi,
>>>
>>> -----Original Message-----
>>> From: Michal Simek <michal.simek@...inx.com>
>>> Sent: Tuesday, September 22, 2020 3:00 PM
>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@...el.com>;
>>> Michal Simek <michal.simek@...inx.com>; Hunter, Adrian
>>> <adrian.hunter@...el.com>; ulf.hansson@...aro.org;
>>> linux-mmc@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
>>> linux-kernel@...r.kernel.org; Arnd Bergmann <arnd@...db.de>
>>> Cc: Raja Subramanian, Lakshmi Bai
>>> <lakshmi.bai.raja.subramanian@...el.com>; Wan Mohamad, Wan Ahmad
>>> Zainie <wan.ahmad.zainie.wan.mohamad@...el.com>
>>> Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Enable UHS-1 support
>>> for Keem Bay SOC
>>>
>>> Hi,
>>>
>>> On 22. 09. 20 2:47, Zulkifli, Muhammad Husaini wrote:
>>>>
>>>> -----Original Message-----
>>>> From: Michal Simek <michal.simek@...inx.com>
>>>> Sent: Monday, September 14, 2020 9:40 PM
>>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@...el.com>;
>>>> Michal Simek <michal.simek@...inx.com>; Hunter, Adrian
>>>> <adrian.hunter@...el.com>; ulf.hansson@...aro.org;
>>>> linux-mmc@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
>>>> linux-kernel@...r.kernel.org; Arnd Bergmann <arnd@...db.de>
>>>> Cc: Raja Subramanian, Lakshmi Bai
>>>> <lakshmi.bai.raja.subramanian@...el.com>; Wan Mohamad, Wan Ahmad
>>>> Zainie <wan.ahmad.zainie.wan.mohamad@...el.com>
>>>> Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Enable UHS-1
>>>> support for Keem Bay SOC
>>>>
>>>> Hi,
>>>>
>>>> On 14. 09. 20 15:26, Zulkifli, Muhammad Husaini wrote:
>>>>> HI Michal,
>>>>>
>>>>> Thanks for the comments.
>>>>> I replied inline
>>>>>
>>>>> -----Original Message-----
>>>>> From: Michal Simek <michal.simek@...inx.com>
>>>>> Sent: Monday, September 14, 2020 2:46 PM
>>>>> To: Zulkifli, Muhammad Husaini
>>>>> <muhammad.husaini.zulkifli@...el.com>;
>>>>> Hunter, Adrian <adrian.hunter@...el.com>; michal.simek@...inx.com;
>>>>> ulf.hansson@...aro.org; linux-mmc@...r.kernel.org;
>>>>> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org;
>>>>> Arnd Bergmann <arnd@...db.de>
>>>>> Cc: Raja Subramanian, Lakshmi Bai
>>>>> <lakshmi.bai.raja.subramanian@...el.com>
>>>>> Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Enable UHS-1
>>>>> support for Keem Bay SOC
>>>>>
>>>>> Hi, +Arnd,
>>>>>
>>>>> On 14. 09. 20 7: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 as
>>>>>> Keem Bay EVM is using external voltage regulator to switch between
>>>>>> 1.8V and 3.3V.
>>>>>>
>>>>>> 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 | 140
>>>>>> +++++++++++++++++++++++++++++
>>>>>>  1 file changed, 140 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>>>>>> b/drivers/mmc/host/sdhci-of-arasan.c
>>>>>> index f186fbd016b1..c133408d0c74 100644
>>>>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>>>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>>>>> @@ -16,7 +16,9 @@
>>>>>>   */
>>>>>>
>>>>>>  #include <linux/clk-provider.h>
>>>>>> +#include <linux/gpio/consumer.h>
>>>>>>  #include <linux/mfd/syscon.h>
>>>>>> +#include <linux/arm-smccc.h>
>>>>>>  #include <linux/module.h>
>>>>>>  #include <linux/of_device.h>
>>>>>>  #include <linux/phy/phy.h>
>>>>>> @@ -41,6 +43,11 @@
>>>>>>  #define SDHCI_ITAPDLY_ENABLE		0x100
>>>>>>  #define SDHCI_OTAPDLY_ENABLE		0x40
>>>>>>
>>>>>> +/* Setting for Keem Bay IO Pad 1.8 Voltage Selection */
>>>>>> +#define KEEMBAY_AON_SIP_FUNC_ID		0x8200ff26
>>>>>> +#define KEEMBAY_AON_SET_1V8_VOLT	0x01
>>>>>> +#define KEEMBAY_AON_SET_3V3_VOLT	0x00
>>>>>> +
>>>>>>  /* Default settings for ZynqMP Clock Phases */
>>>>>>  #define ZYNQMP_ICLK_PHASE {0, 63, 63, 0, 63,  0,   0, 183, 54,  0, 0}
>>>>>>  #define ZYNQMP_OCLK_PHASE {0, 72, 60, 0, 60, 72, 135, 48, 72, 135,
>>>>>> 0} @@ -150,6 +157,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 +369,121 @@ static int sdhci_arasan_voltage_switch(struct
>> mmc_host *mmc,
>>>>>>  	return -EINVAL;
>>>>>>  }
>>>>>>
>>>>>> +static int sdhci_arasan_keembay_set_voltage(int volt) { #if
>>>>>> +IS_ENABLED(CONFIG_HAVE_ARM_SMCCC)
>>>>>> +	struct arm_smccc_res res;
>>>>>> +
>>>>>> +	arm_smccc_smc(KEEMBAY_AON_SIP_FUNC_ID, volt, 0, 0, 0, 0, 0, 0,
>> &res);
>>>>>> +	if (res.a0)
>>>>>> +		return -EINVAL;
>>>>>> +	return 0;
>>>>>
>>>>> I am just curious about calling this smc directly from device driver. I see that
>> several drivers are doing this but isn't it better to hide these in firmware driver?
>>>>> [Husaini] In order to change the voltage selection for IO Pads voltage
>> switching level control, I need to access/write to AON register.
>>>>> Due to security concern, U-Boot Team provided an interface using this SIP
>> Service for me to call during kernel driver voltage switching operation.
>>>>
>>>> I expect U-Boot team is any internal team not U-Boot upstream folks.
>>>> [Husaini] I requote my statement. It is ATF that provided the services. They
>> are in the process of upstreaming the code as well.
>>>> That is a great idea to hide these in firmware driver.
>>>> I created one firmware driver under /drivers/firmware. This firmware driver
>> provide an api for device driver to call for the operations.
>>>>
>>>>
>>>>> Also the part of FUNC_ID is smc32, sip service call (0x82000000) function
>> identifier which is likely something what should be used as macro in shared
>> location that others can use it too.
>>>>> [Husaini] The only thing provided was the FUNC_ID value and argument.
>>>>>
>>>>> Another part is that based on description you are talking to external
>> voltage regulator without using regulator framework at all. Isn't it better just to
>> create firmware based regulator for this purpose?
>>>>> [Husaini] This is for Keembay specific and we did not use regulator
>> framework.
>>>>> During the voltage switching, this SIP function need to be executed to
>> change the Keem Bay IO Pad Switching Level Control to 1.8V for UHS or 3.3v for
>> default mode.
>>>>> To be specific, below line of code must come together during the voltage
>> switching operation.
>>>>>
>>>>> For 1.8V
>>>>> +		/* Set VDDIO_B voltage to Low for 1.8V */
>>>>> +		gpiod_set_value_cansleep(sdhci_arasan->uhs_gpio, 0);
>>>>> +
>>>>> +		ret =
>> sdhci_arasan_keembay_set_voltage(KEEMBAY_AON_SET_1V8_VOLT);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>>
>>>>> For 3.3V
>>>>> 		/* Set VDDIO_B voltage to High for 3.3V */
>>>>> +		gpiod_set_value_cansleep(sdhci_arasan->uhs_gpio, 1);
>>>>> +
>>>>> +		ret =
>> sdhci_arasan_keembay_set_voltage(KEEMBAY_AON_SET_3V3_VOLT);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>
>>>>
>>>> I understand that you need to change voltage here but I don't think the code
>> you have written is how this should be done. I understand that this is the
>> quickest and direct way how to do it but I don't think this is done via proper
>> interface. I pretty much dislike that you are putting Func IDs to drivers instead
>> of adding them to central place that it is visible what your platform needs.
>>>> [Husaini] let me rephase my sentences . I make some confusion here and in
>> commit message. To summarize there are 2 places to final generate the IO
>> Voltage.
>>>>
>>>> 1) Setting the V_VDDIO_B . AON Register for IO PADS Voltage Switching Level
>> Control.
>>>> This register defines the IO Voltage for particular GPIOs pin for
>> clk,cmd,data1-2.
>>>>
>>>> 2) Setting the GPIO expander pin value to drive either 1.8V or 3.3V.
>>>> SD card IO can operate at 3.3V (default) or 1.8V.
>>>> Keem Bay has a bank of IO that can be switched between 3.3V or 1.8V for
>> this reason.
>>>> The output V_VDDIO_B_MAIN be either 3.3v (in) or 1.8v(in), depending on
>> the state of GPIO expander PIN value.
>>>>
>>>> The final IO voltage is set by V_VDDIO_B (= V_VDDIO_B_MAIN after passing
>> through voltage sense resistor).
>>>> I will use the gpio consumer interface to specify a direction and value for the
>> gpio expander pin.
>>>> Is this OK with these 2 implementation?
>>>
>>> Ok. This more sounds like changing IO state which targets pin control
>>> driver. Take a look at sdhci-tegra.c and trace pinctrl_state_3v3 and
>>> pinctrl_state_1v8 and pinctrl_select_state and corresponding DT binding.
>>>
>>> IMHO you should create pin control driver which will call firmware driver to
>> change voltage.
>>> [Husaini] Thank you for suggesting that. Is it Ok to move with current
>> implementation first without the pinctrl driver.
>>> That one consider another next implementation.
>>
>> I don't think we are working in this mode. Hack something first and fix it later
>> which won't happen any time soon. Please do it properly directly.
> 
> To clarify, 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 of now ,I have all the changes for arasan and new firmware driver implementation.
> May i have your approval to proceed with V2?

Send it out and let's see.

Thanks,
Michal

Powered by blists - more mailing lists