Hi, On 01. 10. 20 11:09, Zulkifli, Muhammad Husaini wrote: > Hi Michal, > >> -----Original Message----- >> From: Michal Simek <[email protected]> >> Sent: Wednesday, September 23, 2020 2:20 PM >> To: Zulkifli, Muhammad Husaini <[email protected]>; >> Michal Simek <[email protected]>; Hunter, Adrian >> <[email protected]>; [email protected]; linux- >> [email protected]; [email protected]; linux- >> [email protected]; Arnd Bergmann <[email protected]> >> Cc: Raja Subramanian, Lakshmi Bai <[email protected]>; >> Wan Mohamad, Wan Ahmad Zainie >> <[email protected]> >> 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 <[email protected]> >>> Sent: Tuesday, September 22, 2020 3:00 PM >>> To: Zulkifli, Muhammad Husaini <[email protected]>; >>> Michal Simek <[email protected]>; Hunter, Adrian >>> <[email protected]>; [email protected]; >>> [email protected]; [email protected]; >>> [email protected]; Arnd Bergmann <[email protected]> >>> Cc: Raja Subramanian, Lakshmi Bai >>> <[email protected]>; Wan Mohamad, Wan Ahmad >>> Zainie <[email protected]> >>> 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 <[email protected]> >>>> Sent: Monday, September 14, 2020 9:40 PM >>>> To: Zulkifli, Muhammad Husaini <[email protected]>; >>>> Michal Simek <[email protected]>; Hunter, Adrian >>>> <[email protected]>; [email protected]; >>>> [email protected]; [email protected]; >>>> [email protected]; Arnd Bergmann <[email protected]> >>>> Cc: Raja Subramanian, Lakshmi Bai >>>> <[email protected]>; Wan Mohamad, Wan Ahmad >>>> Zainie <[email protected]> >>>> 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 <[email protected]> >>>>> Sent: Monday, September 14, 2020 2:46 PM >>>>> To: Zulkifli, Muhammad Husaini >>>>> <[email protected]>; >>>>> Hunter, Adrian <[email protected]>; [email protected]; >>>>> [email protected]; [email protected]; >>>>> [email protected]; [email protected]; >>>>> Arnd Bergmann <[email protected]> >>>>> Cc: Raja Subramanian, Lakshmi Bai >>>>> <[email protected]> >>>>> 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, [email protected] wrote: >>>>>> From: Muhammad Husaini Zulkifli >>>>>> <[email protected]> >>>>>> >>>>>> 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 >>>>>> <[email protected]> >>>>>> Reviewed-by: Andy Shevchenko <[email protected]> >>>>>> Reviewed-by: Adrian Hunter <[email protected]> >>>>>> --- >>>>>> 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
