[PATCH v1 1/1] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
Michal Simek
michal.simek at xilinx.com
Thu Oct 1 05:25:11 EDT 2020
Hi,
On 01. 10. 20 11:09, Zulkifli, Muhammad Husaini wrote:
> Hi Michal,
>
>> -----Original Message-----
>> From: Michal Simek <michal.simek at xilinx.com>
>> Sent: Wednesday, September 23, 2020 2:20 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli at intel.com>;
>> Michal Simek <michal.simek at xilinx.com>; Hunter, Adrian
>> <adrian.hunter at intel.com>; ulf.hansson at linaro.org; linux-
>> mmc at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
>> kernel at vger.kernel.org; Arnd Bergmann <arnd at arndb.de>
>> Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian at intel.com>;
>> Wan Mohamad, Wan Ahmad Zainie
>> <wan.ahmad.zainie.wan.mohamad at intel.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 at xilinx.com>
>>> Sent: Tuesday, September 22, 2020 3:00 PM
>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli at intel.com>;
>>> Michal Simek <michal.simek at xilinx.com>; Hunter, Adrian
>>> <adrian.hunter at intel.com>; ulf.hansson at linaro.org;
>>> linux-mmc at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>>> linux-kernel at vger.kernel.org; Arnd Bergmann <arnd at arndb.de>
>>> Cc: Raja Subramanian, Lakshmi Bai
>>> <lakshmi.bai.raja.subramanian at intel.com>; Wan Mohamad, Wan Ahmad
>>> Zainie <wan.ahmad.zainie.wan.mohamad at intel.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 at xilinx.com>
>>>> Sent: Monday, September 14, 2020 9:40 PM
>>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli at intel.com>;
>>>> Michal Simek <michal.simek at xilinx.com>; Hunter, Adrian
>>>> <adrian.hunter at intel.com>; ulf.hansson at linaro.org;
>>>> linux-mmc at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>>>> linux-kernel at vger.kernel.org; Arnd Bergmann <arnd at arndb.de>
>>>> Cc: Raja Subramanian, Lakshmi Bai
>>>> <lakshmi.bai.raja.subramanian at intel.com>; Wan Mohamad, Wan Ahmad
>>>> Zainie <wan.ahmad.zainie.wan.mohamad at intel.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 at xilinx.com>
>>>>> Sent: Monday, September 14, 2020 2:46 PM
>>>>> To: Zulkifli, Muhammad Husaini
>>>>> <muhammad.husaini.zulkifli at intel.com>;
>>>>> Hunter, Adrian <adrian.hunter at intel.com>; michal.simek at xilinx.com;
>>>>> ulf.hansson at linaro.org; linux-mmc at vger.kernel.org;
>>>>> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
>>>>> Arnd Bergmann <arnd at arndb.de>
>>>>> Cc: Raja Subramanian, Lakshmi Bai
>>>>> <lakshmi.bai.raja.subramanian at intel.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 at intel.com wrote:
>>>>>> From: Muhammad Husaini Zulkifli
>>>>>> <muhammad.husaini.zulkifli at intel.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 at intel.com>
>>>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko at intel.com>
>>>>>> Reviewed-by: Adrian Hunter <adrian.hunter at intel.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
More information about the linux-arm-kernel
mailing list