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. Thanks, Michal
