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

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

[muhammad.husaini.zulkifli]

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;
+#else
+	return -EINVAL;
+#endif
+}
+
+static int sdhci_arasan_keembay_voltage_switch(struct mmc_host *mmc,
+				       struct mmc_ios *ios)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
+	u16 ctrl_2;
+	u16 clk;
+	int ret;
+
+	switch (ios->signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_180:
+		clk  = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+		clk &= ~SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+		clk  = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+		if (clk & SDHCI_CLOCK_CARD_EN)
+			return -EAGAIN;
+
+		sdhci_writeb(host, SDHCI_POWER_ON | SDHCI_POWER_180,
+				   SDHCI_POWER_CONTROL);
+
+		/* Set VDDIO_B voltage to Low for 1.8V */
+		gpiod_set_value_cansleep(sdhci_arasan->uhs_gpio, 0);
+
+		/*
+		 * This is like final gatekeeper. Need to ensure changed voltage
+		 * is settled before and after turn on this bit.
+		 */
+		usleep_range(1000, 1100);
+
+		ret = sdhci_arasan_keembay_set_voltage(KEEMBAY_AON_SET_1V8_VOLT);
+		if (ret)
+			return ret;
+
+		usleep_range(1000, 1100);
+
+		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		ctrl_2 |= SDHCI_CTRL_VDD_180;
+		sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
+
+		/* Sleep for 5ms to stabilize 1.8V regulator */
+		usleep_range(5000, 5500);
+
+		/* 1.8V regulator output should be stable within 5 ms */
+		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		if (!(ctrl_2 & SDHCI_CTRL_VDD_180))
+			return -EAGAIN;
+
+		clk  = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+		clk |= SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+		break;
+	case MMC_SIGNAL_VOLTAGE_330:
+		/* Set VDDIO_B voltage to High for 3.3V */
+		gpiod_set_value_cansleep(sdhci_arasan->uhs_gpio, 1);
+
+		/*
+		 * This is like final gatekeeper. Need to ensure changed voltage
+		 * is settled before and after turn on this bit.
+		 */
+		usleep_range(1000, 1100);
+
+		ret = sdhci_arasan_keembay_set_voltage(KEEMBAY_AON_SET_3V3_VOLT);
+		if (ret)
+			return ret;
+
+		usleep_range(1000, 1100);
+
+		/* Set 1.8V Signal Enable in the Host Control2 register to 0 */
+		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		ctrl_2 &= ~SDHCI_CTRL_VDD_180;
+		sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
+
+		/* Sleep for 5ms to stabilize 3.3V regulator */
+		usleep_range(5000, 5500);
+
+		/* 3.3V regulator output should be stable within 5 ms */
+		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		if (ctrl_2 & SDHCI_CTRL_VDD_180)
+			return -EAGAIN;
+
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int sdhci_arasan_keembay_select_drive_strength(struct mmc_card *card,
+					unsigned int max_dtr, int host_drv,
+					int card_drv, int *drv_type)
+{
+	if (card->host->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_180)
+		*drv_type = MMC_SET_DRIVER_TYPE_C;
+
+	return 0;
+}
+
 static const struct sdhci_ops sdhci_arasan_ops = {
 	.set_clock = sdhci_arasan_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
@@ -1521,6 +1644,7 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 	struct sdhci_pltfm_host *pltfm_host;
 	struct sdhci_arasan_data *sdhci_arasan;
 	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
 	const struct sdhci_arasan_of_data *data;
 
 	match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
@@ -1600,6 +1724,22 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 		host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
 	}
 
+	if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) {
+		struct gpio_desc *uhs;
+
+		uhs = devm_gpiod_get_optional(dev, "uhs", GPIOD_OUT_HIGH);
+		if (IS_ERR(uhs))
+			return dev_err_probe(dev, PTR_ERR(uhs), "can't get uhs gpio\n");
+
+		sdhci_arasan->uhs_gpio = uhs;
+
+		host->mmc_host_ops.start_signal_voltage_switch =
+			sdhci_arasan_keembay_voltage_switch;
+
+		host->mmc_host_ops.select_drive_strength =
+			sdhci_arasan_keembay_select_drive_strength;
+	}
+
 	sdhci_arasan_update_baseclkfreq(host);
 
 	ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, &pdev->dev);
-- 
2.17.1

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

[Michal Simek]

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

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@xilinx.com> 
Sent: Monday, September 14, 2020 2:46 PM
To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; Hunter, Adrian <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: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@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@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?
[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

Re: [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@xilinx.com> 
> Sent: Monday, September 14, 2020 2:46 PM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; Hunter, Adrian <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: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@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@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?
> [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

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

[Michal Simek]

Hi,

On 22. 09. 20 2:47, Zulkifli, Muhammad Husaini wrote:
> 
> -----Original Message-----
> From: Michal Simek <michal.simek@xilinx.com> 
> Sent: Monday, September 14, 2020 9:40 PM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian <adrian.hunter@intel.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: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@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.simek@xilinx.com>
>> Sent: Monday, September 14, 2020 2:46 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; 
>> Hunter, Adrian <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: Raja Subramanian, Lakshmi Bai 
>> <lakshmi.bai.raja.subramanian@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@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?
>> [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.

Thanks,
Michal

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

[Zulkifli, Muhammad Husaini]

Hi,

-----Original Message-----
From: Michal Simek <michal.simek@xilinx.com> 
Sent: Tuesday, September 22, 2020 3:00 PM
To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian <adrian.hunter@intel.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: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@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.simek@xilinx.com>
> Sent: Monday, September 14, 2020 9:40 PM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; 
> Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian 
> <adrian.hunter@intel.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: Raja Subramanian, Lakshmi Bai 
> <lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad 
> Zainie <wan.ahmad.zainie.wan.mohamad@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.simek@xilinx.com>
>> Sent: Monday, September 14, 2020 2:46 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>> Hunter, Adrian <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: Raja Subramanian, Lakshmi Bai
>> <lakshmi.bai.raja.subramanian@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@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?
>> [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

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

[Michal Simek]

Hi,

On 22. 09. 20 20:38, Zulkifli, Muhammad Husaini wrote:
> Hi,
> 
> -----Original Message-----
> From: Michal Simek <michal.simek@xilinx.com> 
> Sent: Tuesday, September 22, 2020 3:00 PM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian <adrian.hunter@intel.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: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@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.simek@xilinx.com>
>> Sent: Monday, September 14, 2020 9:40 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; 
>> Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian 
>> <adrian.hunter@intel.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: Raja Subramanian, Lakshmi Bai 
>> <lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad 
>> Zainie <wan.ahmad.zainie.wan.mohamad@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.simek@xilinx.com>
>>> Sent: Monday, September 14, 2020 2:46 PM
>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>>> Hunter, Adrian <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: Raja Subramanian, Lakshmi Bai
>>> <lakshmi.bai.raja.subramanian@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@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?
>>> [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

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

[Zulkifli, Muhammad Husaini]

Hi Michal,

>-----Original Message-----
>From: Michal Simek <michal.simek@xilinx.com>
>Sent: Wednesday, September 23, 2020 2:20 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian
><adrian.hunter@intel.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: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>Wan Mohamad, Wan Ahmad Zainie
><wan.ahmad.zainie.wan.mohamad@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 20:38, Zulkifli, Muhammad Husaini wrote:
>> Hi,
>>
>> -----Original Message-----
>> From: Michal Simek <michal.simek@xilinx.com>
>> Sent: Tuesday, September 22, 2020 3:00 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>> Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian
>> <adrian.hunter@intel.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: Raja Subramanian, Lakshmi Bai
>> <lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad
>> Zainie <wan.ahmad.zainie.wan.mohamad@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.simek@xilinx.com>
>>> Sent: Monday, September 14, 2020 9:40 PM
>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>>> Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian
>>> <adrian.hunter@intel.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: Raja Subramanian, Lakshmi Bai
>>> <lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad
>>> Zainie <wan.ahmad.zainie.wan.mohamad@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.simek@xilinx.com>
>>>> Sent: Monday, September 14, 2020 2:46 PM
>>>> To: Zulkifli, Muhammad Husaini
>>>> <muhammad.husaini.zulkifli@intel.com>;
>>>> Hunter, Adrian <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: Raja Subramanian, Lakshmi Bai
>>>> <lakshmi.bai.raja.subramanian@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@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?
>>>> [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.

To clarify, the GPIO pin that control the sd-voltage is in GPIO Expander not using Keembay SOC GPIO Pin.
The best option is using the gpio consumer function to toggle the pin.
As of now ,I have all the changes for arasan and new firmware driver implementation.
May i have your approval to proceed with V2?

Thanks
>
>Thanks,
>Michal

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

[Michal Simek]

Hi,

On 01. 10. 20 11:09, Zulkifli, Muhammad Husaini wrote:
> Hi Michal,
> 
>> -----Original Message-----
>> From: Michal Simek <michal.simek@xilinx.com>
>> Sent: Wednesday, September 23, 2020 2:20 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>> Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian
>> <adrian.hunter@intel.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: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>> Wan Mohamad, Wan Ahmad Zainie
>> <wan.ahmad.zainie.wan.mohamad@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 20:38, Zulkifli, Muhammad Husaini wrote:
>>> Hi,
>>>
>>> -----Original Message-----
>>> From: Michal Simek <michal.simek@xilinx.com>
>>> Sent: Tuesday, September 22, 2020 3:00 PM
>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>>> Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian
>>> <adrian.hunter@intel.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: Raja Subramanian, Lakshmi Bai
>>> <lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad
>>> Zainie <wan.ahmad.zainie.wan.mohamad@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.simek@xilinx.com>
>>>> Sent: Monday, September 14, 2020 9:40 PM
>>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>>>> Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian
>>>> <adrian.hunter@intel.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: Raja Subramanian, Lakshmi Bai
>>>> <lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad
>>>> Zainie <wan.ahmad.zainie.wan.mohamad@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.simek@xilinx.com>
>>>>> Sent: Monday, September 14, 2020 2:46 PM
>>>>> To: Zulkifli, Muhammad Husaini
>>>>> <muhammad.husaini.zulkifli@intel.com>;
>>>>> Hunter, Adrian <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: Raja Subramanian, Lakshmi Bai
>>>>> <lakshmi.bai.raja.subramanian@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@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?
>>>>> [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.
> 
> To clarify, the GPIO pin that control the sd-voltage is in GPIO Expander not using Keembay SOC GPIO Pin.
> The best option is using the gpio consumer function to toggle the pin.
> As of now ,I have all the changes for arasan and new firmware driver implementation.
> May i have your approval to proceed with V2?

Send it out and let's see.

Thanks,
Michal