Hi,
-----Original Message-----
From: Michal Simek <michal.si...@xilinx.com>
Sent: Tuesday, September 22, 2020 3:00 PM
To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulki...@intel.com>; Michal
Simek <michal.si...@xilinx.com>; Hunter, Adrian <adrian.hun...@intel.com>;
ulf.hans...@linaro.org; linux-...@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Arnd
Bergmann <a...@arndb.de>
Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subraman...@intel.com>; Wan
Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.moha...@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.si...@xilinx.com>
> Sent: Monday, September 14, 2020 9:40 PM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulki...@intel.com>;
> Michal Simek <michal.si...@xilinx.com>; Hunter, Adrian
> <adrian.hun...@intel.com>; ulf.hans...@linaro.org;
> linux-...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> linux-kernel@vger.kernel.org; Arnd Bergmann <a...@arndb.de>
> Cc: Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subraman...@intel.com>; Wan Mohamad, Wan Ahmad
> Zainie <wan.ahmad.zainie.wan.moha...@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.si...@xilinx.com>
>> Sent: Monday, September 14, 2020 2:46 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulki...@intel.com>;
>> Hunter, Adrian <adrian.hun...@intel.com>; michal.si...@xilinx.com;
>> ulf.hans...@linaro.org; linux-...@vger.kernel.org;
>> linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
>> Arnd Bergmann <a...@arndb.de>
>> Cc: Raja Subramanian, Lakshmi Bai
>> <lakshmi.bai.raja.subraman...@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.zulki...@intel.com wrote:
>>> From: Muhammad Husaini Zulkifli
>>> <muhammad.husaini.zulki...@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.zulki...@intel.com>
>>> Reviewed-by: Andy Shevchenko <andriy.shevche...@intel.com>
>>> Reviewed-by: Adrian Hunter <adrian.hun...@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.
If ok, I will send out the next patch version.
Thanks
Thanks,
Michal
