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

[Michal Simek]

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 at xilinx.com> 
> Sent: Monday, September 14, 2020 2:46 PM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli at intel.com>; Hunter, Adrian <adrian.hunter at intel.com>; michal.simek at xilinx.com; ulf.hansson at linaro.org; linux-mmc at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; Arnd Bergmann <arnd at arndb.de>
> Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian at 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.zulkifli at intel.com wrote:
>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli at 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.zulkifli at intel.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko at intel.com>
>> Reviewed-by: Adrian Hunter <adrian.hunter at 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.


> 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.

> 
> 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.

What you do internally is completely different story compare to how this
should be done properly and I think this is just wrong way to go. You
should at least create firmware driver and even check if this feature
you are asking about is available and provided by firmware.

Thanks,
Michal