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

[Zulkifli, Muhammad Husaini]

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. 

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;

Or was there any agreement to put these stuff directly to the driver?
[Husaini] There is an agreement between IO Team and U-boot team to use this kind of implementation to support this UHS-1 Mode features for keem bay 
for voltage switching sequence.

Thanks,
Michal