Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
From: Michal Simek
Date: Tue Sep 22 2020 - 03:00:37 EST
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.
Thanks,
Michal