Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

From: Michal Simek
Date: Thu Oct 01 2020 - 05:25:27 EST

Next message: Jiri Olsa: "Re: [PATCH 1/9] perf tools: Add build id shell test"
Previous message: AISHA GADDAFI: "Lieber Freund (Assalamu Alaikum),?"
In reply to: Zulkifli, Muhammad Husaini: "RE: [PATCH v1 1/1] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 01. 10. 20 11:09, Zulkifli, Muhammad Husaini wrote:
> Hi Michal,
>
>> -----Original Message-----
>> From: Michal Simek <michal.simek@xxxxxxxxxx>
>> Sent: Wednesday, September 23, 2020 2:20 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@xxxxxxxxx>;
>> Michal Simek <michal.simek@xxxxxxxxxx>; Hunter, Adrian
>> <adrian.hunter@xxxxxxxxx>; ulf.hansson@xxxxxxxxxx; linux-
>> mmc@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx; Arnd Bergmann <arnd@xxxxxxxx>
>> Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@xxxxxxxxx>;
>> Wan Mohamad, Wan Ahmad Zainie
>> <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>
>> 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@xxxxxxxxxx>
>>> Sent: Tuesday, September 22, 2020 3:00 PM
>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@xxxxxxxxx>;
>>> Michal Simek <michal.simek@xxxxxxxxxx>; Hunter, Adrian
>>> <adrian.hunter@xxxxxxxxx>; ulf.hansson@xxxxxxxxxx;
>>> linux-mmc@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
>>> linux-kernel@xxxxxxxxxxxxxxx; Arnd Bergmann <arnd@xxxxxxxx>
>>> Cc: Raja Subramanian, Lakshmi Bai
>>> <lakshmi.bai.raja.subramanian@xxxxxxxxx>; Wan Mohamad, Wan Ahmad
>>> Zainie <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>
>>> 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@xxxxxxxxxx>
>>>> Sent: Monday, September 14, 2020 9:40 PM
>>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@xxxxxxxxx>;
>>>> Michal Simek <michal.simek@xxxxxxxxxx>; Hunter, Adrian
>>>> <adrian.hunter@xxxxxxxxx>; ulf.hansson@xxxxxxxxxx;
>>>> linux-mmc@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
>>>> linux-kernel@xxxxxxxxxxxxxxx; Arnd Bergmann <arnd@xxxxxxxx>
>>>> Cc: Raja Subramanian, Lakshmi Bai
>>>> <lakshmi.bai.raja.subramanian@xxxxxxxxx>; Wan Mohamad, Wan Ahmad
>>>> Zainie <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>
>>>> 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@xxxxxxxxxx>
>>>>> Sent: Monday, September 14, 2020 2:46 PM
>>>>> To: Zulkifli, Muhammad Husaini
>>>>> <muhammad.husaini.zulkifli@xxxxxxxxx>;
>>>>> Hunter, Adrian <adrian.hunter@xxxxxxxxx>; michal.simek@xxxxxxxxxx;
>>>>> ulf.hansson@xxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx;
>>>>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>>>>> Arnd Bergmann <arnd@xxxxxxxx>
>>>>> Cc: Raja Subramanian, Lakshmi Bai
>>>>> <lakshmi.bai.raja.subramanian@xxxxxxxxx>
>>>>> 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@xxxxxxxxx wrote:
>>>>>> From: Muhammad Husaini Zulkifli
>>>>>> <muhammad.husaini.zulkifli@xxxxxxxxx>
>>>>>>
>>>>>> 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@xxxxxxxxx>
>>>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
>>>>>> Reviewed-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>>>>>> ---
>>>>>> 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

Next message: Jiri Olsa: "Re: [PATCH 1/9] perf tools: Add build id shell test"
Previous message: AISHA GADDAFI: "Lieber Freund (Assalamu Alaikum),?"
In reply to: Zulkifli, Muhammad Husaini: "RE: [PATCH v1 1/1] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]