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

From: Michal Simek <michal.simek@xilinx.com>
To: muhammad.husaini.zulkifli@intel.com, adrian.hunter@intel.com,
	michal.simek@xilinx.com, ulf.hansson@linaro.org,
	linux-mmc@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>
Cc: lakshmi.bai.raja.subramanian@intel.com
Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
Date: Mon, 14 Sep 2020 08:46:00 +0200	[thread overview]
Message-ID: <21d34b75-5947-e115-7c9a-6d068375bbdd@xilinx.com> (raw)
In-Reply-To: <20200914051214.13918-2-muhammad.husaini.zulkifli@intel.com>

Hi, +Arnd,

On 14. 09. 20 7:12, muhammad.husaini.zulkifli@intel.com wrote:
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@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@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Reviewed-by: Adrian Hunter <adrian.hunter@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?
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.

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?

Or was there any agreement to put these stuff directly to the driver?

Thanks,
Michal