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

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

[muhammad.husaini.zulkifli]

From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

Hi.

The first patch is to enable UHS-1 Support for Keem Bay EVM.

The second patch is the header file to handle ATF call.

These 2 patches was tested with Keem Bay evaluation module board.

Kindly help to review this patch set.

Thank you.

Changes since v2:
- Removed Document DT Bindings for Keembay Firmware.
- Removed Firmware Driver to handle ATF Service call.
- Add header file to handle API function for device driver to communicate with Arm Trusted Firmware.

Changes since v1:
- Add Document DT Bindings for Keembay Firmware.
- Created Firmware Driver to handle ATF Service call
- Provide API for arasan driver for sd card voltage changes

Muhammad Husaini Zulkifli (2):
  mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
  firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

 drivers/mmc/host/sdhci-of-arasan.c            | 127 ++++++++++++++++++
 .../linux/firmware/intel/keembay_firmware.h   |  46 +++++++
 2 files changed, 173 insertions(+)
 create mode 100644 include/linux/firmware/intel/keembay_firmware.h

--
2.17.1

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

[muhammad.husaini.zulkifli]

From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

Hi.

The first patch is to enable UHS-1 Support for Keem Bay EVM.

The second patch is the header file to handle ATF call.

These 2 patches was tested with Keem Bay evaluation module board.

Kindly help to review this patch set.

Thank you.

Changes since v2:
- Removed Document DT Bindings for Keembay Firmware.
- Removed Firmware Driver to handle ATF Service call.
- Add header file to handle API function for device driver to communicate with Arm Trusted Firmware.

Changes since v1:
- Add Document DT Bindings for Keembay Firmware.
- Created Firmware Driver to handle ATF Service call
- Provide API for arasan driver for sd card voltage changes

Muhammad Husaini Zulkifli (2):
  mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
  firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

 drivers/mmc/host/sdhci-of-arasan.c            | 127 ++++++++++++++++++
 .../linux/firmware/intel/keembay_firmware.h   |  46 +++++++
 2 files changed, 173 insertions(+)
 create mode 100644 include/linux/firmware/intel/keembay_firmware.h

--
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/2] 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.
There are 2 places to control the voltage.
1) By setting the AON register using firmware driver calling
system-level platform management layer (SMC) to set the register.
2) By controlling the GPIO expander value to drive either 1.8V or 3.3V
for power mux input.

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 | 127 +++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index f186fbd016b1..e681e6f860ba 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/gpio/consumer.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
@@ -23,6 +24,7 @@
 #include <linux/regmap.h>
 #include <linux/of.h>
 #include <linux/firmware/xlnx-zynqmp.h>
+#include <linux/firmware/intel/keembay_firmware.h>
 
 #include "cqhci.h"
 #include "sdhci-pltfm.h"
@@ -150,6 +152,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 +364,113 @@ static int sdhci_arasan_voltage_switch(struct mmc_host *mmc,
 	return -EINVAL;
 }
 
+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
+		 * which is controlling by GPIO Expander.
+		 */
+		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 = keembay_sd_voltage_selection(KEEMBAY_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
+		 * which is controlling by GPIO Expander.
+		 */
+		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 = keembay_sd_voltage_selection(KEEMBAY_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 +1631,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 +1711,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

[PATCH v3 1/2] 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.
There are 2 places to control the voltage.
1) By setting the AON register using firmware driver calling
system-level platform management layer (SMC) to set the register.
2) By controlling the GPIO expander value to drive either 1.8V or 3.3V
for power mux input.

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 | 127 +++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index f186fbd016b1..e681e6f860ba 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/gpio/consumer.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
@@ -23,6 +24,7 @@
 #include <linux/regmap.h>
 #include <linux/of.h>
 #include <linux/firmware/xlnx-zynqmp.h>
+#include <linux/firmware/intel/keembay_firmware.h>
 
 #include "cqhci.h"
 #include "sdhci-pltfm.h"
@@ -150,6 +152,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 +364,113 @@ static int sdhci_arasan_voltage_switch(struct mmc_host *mmc,
 	return -EINVAL;
 }
 
+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
+		 * which is controlling by GPIO Expander.
+		 */
+		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 = keembay_sd_voltage_selection(KEEMBAY_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
+		 * which is controlling by GPIO Expander.
+		 */
+		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 = keembay_sd_voltage_selection(KEEMBAY_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 +1631,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 +1711,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Michal Simek]

On 06. 10. 20 17:55, 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.
> There are 2 places to control the voltage.
> 1) By setting the AON register using firmware driver calling
> system-level platform management layer (SMC) to set the register.
> 2) By controlling the GPIO expander value to drive either 1.8V or 3.3V
> for power mux input.
> 
> 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 | 127 +++++++++++++++++++++++++++++
>  1 file changed, 127 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index f186fbd016b1..e681e6f860ba 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -16,6 +16,7 @@
>   */
>  
>  #include <linux/clk-provider.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> @@ -23,6 +24,7 @@
>  #include <linux/regmap.h>
>  #include <linux/of.h>
>  #include <linux/firmware/xlnx-zynqmp.h>
> +#include <linux/firmware/intel/keembay_firmware.h>
>  
>  #include "cqhci.h"
>  #include "sdhci-pltfm.h"
> @@ -150,6 +152,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 +364,113 @@ static int sdhci_arasan_voltage_switch(struct mmc_host *mmc,
>  	return -EINVAL;
>  }
>  
> +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;

nit: put it to one line.

> +	int ret;
> +
> +	switch (ios->signal_voltage) {
> +	case MMC_SIGNAL_VOLTAGE_180:
> +		clk  = sdhci_readw(host, SDHCI_CLOCK_CONTROL);

nit: double space

> +		clk &= ~SDHCI_CLOCK_CARD_EN;
> +		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +
> +		clk  = sdhci_readw(host, SDHCI_CLOCK_CONTROL);

nit: double space again.

> +		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
> +		 * which is controlling by GPIO Expander.
> +		 */
> +		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 = keembay_sd_voltage_selection(KEEMBAY_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
> +		 * which is controlling by GPIO Expander.
> +		 */
> +		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 = keembay_sd_voltage_selection(KEEMBAY_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 +1631,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;

nit: I got this but as I see 3 lines below maybe would be better to use
it everywhere but it can be done in separate patch.

>  	const struct sdhci_arasan_of_data *data;
>  
>  	match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
> @@ -1600,6 +1711,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);

I can't see change in dt binding to record uhs gpio.


Better
sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs",
GPIOD_OUT_HIGH);

then you can avoid uhs variable.

> +		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);
> 

Thanks,
Michal

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

[Michal Simek]

On 06. 10. 20 17:55, 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.
> There are 2 places to control the voltage.
> 1) By setting the AON register using firmware driver calling
> system-level platform management layer (SMC) to set the register.
> 2) By controlling the GPIO expander value to drive either 1.8V or 3.3V
> for power mux input.
> 
> 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 | 127 +++++++++++++++++++++++++++++
>  1 file changed, 127 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index f186fbd016b1..e681e6f860ba 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -16,6 +16,7 @@
>   */
>  
>  #include <linux/clk-provider.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> @@ -23,6 +24,7 @@
>  #include <linux/regmap.h>
>  #include <linux/of.h>
>  #include <linux/firmware/xlnx-zynqmp.h>
> +#include <linux/firmware/intel/keembay_firmware.h>
>  
>  #include "cqhci.h"
>  #include "sdhci-pltfm.h"
> @@ -150,6 +152,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 +364,113 @@ static int sdhci_arasan_voltage_switch(struct mmc_host *mmc,
>  	return -EINVAL;
>  }
>  
> +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;

nit: put it to one line.

> +	int ret;
> +
> +	switch (ios->signal_voltage) {
> +	case MMC_SIGNAL_VOLTAGE_180:
> +		clk  = sdhci_readw(host, SDHCI_CLOCK_CONTROL);

nit: double space

> +		clk &= ~SDHCI_CLOCK_CARD_EN;
> +		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +
> +		clk  = sdhci_readw(host, SDHCI_CLOCK_CONTROL);

nit: double space again.

> +		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
> +		 * which is controlling by GPIO Expander.
> +		 */
> +		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 = keembay_sd_voltage_selection(KEEMBAY_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
> +		 * which is controlling by GPIO Expander.
> +		 */
> +		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 = keembay_sd_voltage_selection(KEEMBAY_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 +1631,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;

nit: I got this but as I see 3 lines below maybe would be better to use
it everywhere but it can be done in separate patch.

>  	const struct sdhci_arasan_of_data *data;
>  
>  	match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
> @@ -1600,6 +1711,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);

I can't see change in dt binding to record uhs gpio.


Better
sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs",
GPIOD_OUT_HIGH);

then you can avoid uhs variable.

> +		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);
> 

Thanks,
Michal

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com> wrote:
> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:

...

> > +             /*
> > +              * This is like final gatekeeper. Need to ensure changed voltage

like a final

> > +              * is settled before and after turn on this bit.
> > +              */

...

> > +             /*
> > +              * This is like final gatekeeper. Need to ensure changed voltage

Likewise.

> > +              * is settled before and after turn on this bit.
> > +              */

...

> > +     struct device *dev = &pdev->dev;
>
> nit: I got this but as I see 3 lines below maybe would be better to use
> it everywhere but it can be done in separate patch.

In that case I think it would be better to have that patch first. It
make follow up code cleaner.

...

> > +     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);
>
> I can't see change in dt binding to record uhs gpio.
>
>
> Better
> sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs",
> GPIOD_OUT_HIGH);
>
> then you can avoid uhs variable.

Actually it's readability vs. additional variable. It was my
suggestion to have a variable to make readability better.
Are you insisting on this change?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com> wrote:
> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:

...

> > +             /*
> > +              * This is like final gatekeeper. Need to ensure changed voltage

like a final

> > +              * is settled before and after turn on this bit.
> > +              */

...

> > +             /*
> > +              * This is like final gatekeeper. Need to ensure changed voltage

Likewise.

> > +              * is settled before and after turn on this bit.
> > +              */

...

> > +     struct device *dev = &pdev->dev;
>
> nit: I got this but as I see 3 lines below maybe would be better to use
> it everywhere but it can be done in separate patch.

In that case I think it would be better to have that patch first. It
make follow up code cleaner.

...

> > +     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);
>
> I can't see change in dt binding to record uhs gpio.
>
>
> Better
> sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs",
> GPIOD_OUT_HIGH);
>
> then you can avoid uhs variable.

Actually it's readability vs. additional variable. It was my
suggestion to have a variable to make readability better.
Are you insisting on this change?

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Michal Simek]

On 07. 10. 20 10:55, Andy Shevchenko wrote:
> On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com> wrote:
>> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
> 
> ...
> 
>>> +             /*
>>> +              * This is like final gatekeeper. Need to ensure changed voltage
> 
> like a final
> 
>>> +              * is settled before and after turn on this bit.
>>> +              */
> 
> ...
> 
>>> +             /*
>>> +              * This is like final gatekeeper. Need to ensure changed voltage
> 
> Likewise.
> 
>>> +              * is settled before and after turn on this bit.
>>> +              */
> 
> ...
> 
>>> +     struct device *dev = &pdev->dev;
>>
>> nit: I got this but as I see 3 lines below maybe would be better to use
>> it everywhere but it can be done in separate patch.
> 
> In that case I think it would be better to have that patch first. It
> make follow up code cleaner.
> 
> ...
> 
>>> +     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);
>>
>> I can't see change in dt binding to record uhs gpio.
>>
>>
>> Better
>> sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs",
>> GPIOD_OUT_HIGH);
>>
>> then you can avoid uhs variable.
> 
> Actually it's readability vs. additional variable. It was my
> suggestion to have a variable to make readability better.
> Are you insisting on this change?

I understand that it is just about preference. I would use it directly
not to deal with it. If your preference is via variable I am fine with
it too.

Thanks,
Michal

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

[Michal Simek]

On 07. 10. 20 10:55, Andy Shevchenko wrote:
> On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com> wrote:
>> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
> 
> ...
> 
>>> +             /*
>>> +              * This is like final gatekeeper. Need to ensure changed voltage
> 
> like a final
> 
>>> +              * is settled before and after turn on this bit.
>>> +              */
> 
> ...
> 
>>> +             /*
>>> +              * This is like final gatekeeper. Need to ensure changed voltage
> 
> Likewise.
> 
>>> +              * is settled before and after turn on this bit.
>>> +              */
> 
> ...
> 
>>> +     struct device *dev = &pdev->dev;
>>
>> nit: I got this but as I see 3 lines below maybe would be better to use
>> it everywhere but it can be done in separate patch.
> 
> In that case I think it would be better to have that patch first. It
> make follow up code cleaner.
> 
> ...
> 
>>> +     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);
>>
>> I can't see change in dt binding to record uhs gpio.
>>
>>
>> Better
>> sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs",
>> GPIOD_OUT_HIGH);
>>
>> then you can avoid uhs variable.
> 
> Actually it's readability vs. additional variable. It was my
> suggestion to have a variable to make readability better.
> Are you insisting on this change?

I understand that it is just about preference. I would use it directly
not to deal with it. If your preference is via variable I am fine with
it too.

Thanks,
Michal

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

On Wed, Oct 7, 2020 at 12:10 PM Michal Simek <michal.simek@xilinx.com> wrote:
> On 07. 10. 20 10:55, Andy Shevchenko wrote:
> > On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com> wrote:
> >> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:

...

> >>> +     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);
> >>
> >> I can't see change in dt binding to record uhs gpio.
> >>
> >>
> >> Better
> >> sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs",
> >> GPIOD_OUT_HIGH);
> >>
> >> then you can avoid uhs variable.
> >
> > Actually it's readability vs. additional variable. It was my
> > suggestion to have a variable to make readability better.
> > Are you insisting on this change?
>
> I understand that it is just about preference.

Correct.

> I would use it directly
> not to deal with it. If your preference is via variable I am fine with
> it too.

Yes, I suggested that and I still have the same opinion (It seems the
original code tries to follow 80 character rules that's why I
suggested the change).
Thanks!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Oct 7, 2020 at 12:10 PM Michal Simek <michal.simek@xilinx.com> wrote:
> On 07. 10. 20 10:55, Andy Shevchenko wrote:
> > On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com> wrote:
> >> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:

...

> >>> +     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);
> >>
> >> I can't see change in dt binding to record uhs gpio.
> >>
> >>
> >> Better
> >> sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs",
> >> GPIOD_OUT_HIGH);
> >>
> >> then you can avoid uhs variable.
> >
> > Actually it's readability vs. additional variable. It was my
> > suggestion to have a variable to make readability better.
> > Are you insisting on this change?
>
> I understand that it is just about preference.

Correct.

> I would use it directly
> not to deal with it. If your preference is via variable I am fine with
> it too.

Yes, I suggested that and I still have the same opinion (It seems the
original code tries to follow 80 character rules that's why I
suggested the change).
Thanks!

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Zulkifli, Muhammad Husaini]

Hi Andy,

Thanks for the feedback. I replied inline

>-----Original Message-----
>From: Andy Shevchenko <andy.shevchenko@gmail.com>
>Sent: Wednesday, October 7, 2020 4:56 PM
>To: Michal Simek <michal.simek@xilinx.com>
>Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>Hunter, Adrian <adrian.hunter@intel.com>; Sudeep Holla
><sudeep.holla@arm.com>; Ulf Hansson <ulf.hansson@linaro.org>; linux-mmc
><linux-mmc@vger.kernel.org>; linux-arm Mailing List <linux-arm-
>kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
>kernel@vger.kernel.org>; Raja Subramanian, Lakshmi Bai
><lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad
>Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Arnd Bergmann
><arnd@arndb.de>
>Subject: Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com>
>wrote:
>> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>
>...
>
>> > +             /*
>> > +              * This is like final gatekeeper. Need to ensure
>> > + changed voltage
>
>like a final
Noted. Done the changes
>
>> > +              * is settled before and after turn on this bit.
>> > +              */
>
>...
>
>> > +             /*
>> > +              * This is like final gatekeeper. Need to ensure
>> > + changed voltage
>
>Likewise.
Noted. Done the changes
>
>> > +              * is settled before and after turn on this bit.
>> > +              */
>
>...
>
>> > +     struct device *dev = &pdev->dev;
>>
>> nit: I got this but as I see 3 lines below maybe would be better to
>> use it everywhere but it can be done in separate patch.
>
>In that case I think it would be better to have that patch first. It make follow up
>code cleaner.
I want to get some clarification here.
Do I need a separate patch for this struct device *dev = &pdev->dev;?
Can I embedded together with UHS patch?
>
>...
>
>> > +     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);
>>
>> I can't see change in dt binding to record uhs gpio.
>>
>>
>> Better
>> sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs",
>> GPIOD_OUT_HIGH);
>>
>> then you can avoid uhs variable.
>
>Actually it's readability vs. additional variable. It was my suggestion to have a
>variable to make readability better.
>Are you insisting on this change?
>
>--
>With Best Regards,
>Andy Shevchenko

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

[Zulkifli, Muhammad Husaini]

Hi Andy,

Thanks for the feedback. I replied inline

>-----Original Message-----
>From: Andy Shevchenko <andy.shevchenko@gmail.com>
>Sent: Wednesday, October 7, 2020 4:56 PM
>To: Michal Simek <michal.simek@xilinx.com>
>Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>Hunter, Adrian <adrian.hunter@intel.com>; Sudeep Holla
><sudeep.holla@arm.com>; Ulf Hansson <ulf.hansson@linaro.org>; linux-mmc
><linux-mmc@vger.kernel.org>; linux-arm Mailing List <linux-arm-
>kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
>kernel@vger.kernel.org>; Raja Subramanian, Lakshmi Bai
><lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad
>Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Arnd Bergmann
><arnd@arndb.de>
>Subject: Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com>
>wrote:
>> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>
>...
>
>> > +             /*
>> > +              * This is like final gatekeeper. Need to ensure
>> > + changed voltage
>
>like a final
Noted. Done the changes
>
>> > +              * is settled before and after turn on this bit.
>> > +              */
>
>...
>
>> > +             /*
>> > +              * This is like final gatekeeper. Need to ensure
>> > + changed voltage
>
>Likewise.
Noted. Done the changes
>
>> > +              * is settled before and after turn on this bit.
>> > +              */
>
>...
>
>> > +     struct device *dev = &pdev->dev;
>>
>> nit: I got this but as I see 3 lines below maybe would be better to
>> use it everywhere but it can be done in separate patch.
>
>In that case I think it would be better to have that patch first. It make follow up
>code cleaner.
I want to get some clarification here.
Do I need a separate patch for this struct device *dev = &pdev->dev;?
Can I embedded together with UHS patch?
>
>...
>
>> > +     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);
>>
>> I can't see change in dt binding to record uhs gpio.
>>
>>
>> Better
>> sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs",
>> GPIOD_OUT_HIGH);
>>
>> then you can avoid uhs variable.
>
>Actually it's readability vs. additional variable. It was my suggestion to have a
>variable to make readability better.
>Are you insisting on this change?
>
>--
>With Best Regards,
>Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

On Wed, Oct 7, 2020 at 4:28 PM Zulkifli, Muhammad Husaini
<muhammad.husaini.zulkifli@intel.com> wrote:
> >From: Andy Shevchenko <andy.shevchenko@gmail.com>
> >Sent: Wednesday, October 7, 2020 4:56 PM
> >On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com>
> >wrote:
> >> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:

...

> >> > +     struct device *dev = &pdev->dev;
> >>
> >> nit: I got this but as I see 3 lines below maybe would be better to
> >> use it everywhere but it can be done in separate patch.
> >
> >In that case I think it would be better to have that patch first. It make follow up
> >code cleaner.
> I want to get some clarification here.

> Do I need a separate patch for this struct device *dev = &pdev->dev;?

It should be a separate patch and better your series starts with it,
so it won't interfere with new code.

> Can I embedded together with UHS patch?

Better to avoid merging orthogonal things together in one change.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Oct 7, 2020 at 4:28 PM Zulkifli, Muhammad Husaini
<muhammad.husaini.zulkifli@intel.com> wrote:
> >From: Andy Shevchenko <andy.shevchenko@gmail.com>
> >Sent: Wednesday, October 7, 2020 4:56 PM
> >On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com>
> >wrote:
> >> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:

...

> >> > +     struct device *dev = &pdev->dev;
> >>
> >> nit: I got this but as I see 3 lines below maybe would be better to
> >> use it everywhere but it can be done in separate patch.
> >
> >In that case I think it would be better to have that patch first. It make follow up
> >code cleaner.
> I want to get some clarification here.

> Do I need a separate patch for this struct device *dev = &pdev->dev;?

It should be a separate patch and better your series starts with it,
so it won't interfere with new code.

> Can I embedded together with UHS patch?

Better to avoid merging orthogonal things together in one change.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Zulkifli, Muhammad Husaini]

Hi ,

>-----Original Message-----
>From: Andy Shevchenko <andy.shevchenko@gmail.com>
>Sent: Wednesday, October 7, 2020 10:55 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Cc: Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian
><adrian.hunter@intel.com>; Sudeep Holla <sudeep.holla@arm.com>; Ulf
>Hansson <ulf.hansson@linaro.org>; linux-mmc <linux-mmc@vger.kernel.org>;
>linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>; Linux Kernel
>Mailing List <linux-kernel@vger.kernel.org>; Raja Subramanian, Lakshmi Bai
><lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad Zainie
><wan.ahmad.zainie.wan.mohamad@intel.com>; Arnd Bergmann
><arnd@arndb.de>
>Subject: Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>On Wed, Oct 7, 2020 at 4:28 PM Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@intel.com> wrote:
>> >From: Andy Shevchenko <andy.shevchenko@gmail.com>
>> >Sent: Wednesday, October 7, 2020 4:56 PM On Wed, Oct 7, 2020 at 11:38
>> >AM Michal Simek <michal.simek@xilinx.com>
>> >wrote:
>> >> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>
>...
>
>> >> > +     struct device *dev = &pdev->dev;
>> >>
>> >> nit: I got this but as I see 3 lines below maybe would be better to
>> >> use it everywhere but it can be done in separate patch.
>> >
>> >In that case I think it would be better to have that patch first. It
>> >make follow up code cleaner.
>> I want to get some clarification here.
>
>> Do I need a separate patch for this struct device *dev = &pdev->dev;?
>
>It should be a separate patch and better your series starts with it, so it won't
>interfere with new code.
>
>> Can I embedded together with UHS patch?
>
>Better to avoid merging orthogonal things together in one change.
Noted. Thanks 😉
>
>--
>With Best Regards,
>Andy Shevchenko

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

[Zulkifli, Muhammad Husaini]

Hi ,

>-----Original Message-----
>From: Andy Shevchenko <andy.shevchenko@gmail.com>
>Sent: Wednesday, October 7, 2020 10:55 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Cc: Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian
><adrian.hunter@intel.com>; Sudeep Holla <sudeep.holla@arm.com>; Ulf
>Hansson <ulf.hansson@linaro.org>; linux-mmc <linux-mmc@vger.kernel.org>;
>linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>; Linux Kernel
>Mailing List <linux-kernel@vger.kernel.org>; Raja Subramanian, Lakshmi Bai
><lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad Zainie
><wan.ahmad.zainie.wan.mohamad@intel.com>; Arnd Bergmann
><arnd@arndb.de>
>Subject: Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>On Wed, Oct 7, 2020 at 4:28 PM Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@intel.com> wrote:
>> >From: Andy Shevchenko <andy.shevchenko@gmail.com>
>> >Sent: Wednesday, October 7, 2020 4:56 PM On Wed, Oct 7, 2020 at 11:38
>> >AM Michal Simek <michal.simek@xilinx.com>
>> >wrote:
>> >> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>
>...
>
>> >> > +     struct device *dev = &pdev->dev;
>> >>
>> >> nit: I got this but as I see 3 lines below maybe would be better to
>> >> use it everywhere but it can be done in separate patch.
>> >
>> >In that case I think it would be better to have that patch first. It
>> >make follow up code cleaner.
>> I want to get some clarification here.
>
>> Do I need a separate patch for this struct device *dev = &pdev->dev;?
>
>It should be a separate patch and better your series starts with it, so it won't
>interfere with new code.
>
>> Can I embedded together with UHS patch?
>
>Better to avoid merging orthogonal things together in one change.
Noted. Thanks 😉
>
>--
>With Best Regards,
>Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Zulkifli, Muhammad Husaini]

Hi Michal,

Thanks again for the feedback. I replied inline

>-----Original Message-----
>From: Michal Simek <michal.simek@xilinx.com>
>Sent: Wednesday, October 7, 2020 4:34 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>sudeep.holla@arm.com; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org;
>linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>Wan Mohamad, Wan Ahmad Zainie
><wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>Subject: Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>
>
>On 06. 10. 20 17:55, 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.
>> There are 2 places to control the voltage.
>> 1) By setting the AON register using firmware driver calling
>> system-level platform management layer (SMC) to set the register.
>> 2) By controlling the GPIO expander value to drive either 1.8V or 3.3V
>> for power mux input.
>>
>> 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 | 127
>> +++++++++++++++++++++++++++++
>>  1 file changed, 127 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>> b/drivers/mmc/host/sdhci-of-arasan.c
>> index f186fbd016b1..e681e6f860ba 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -16,6 +16,7 @@
>>   */
>>
>>  #include <linux/clk-provider.h>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/module.h>
>>  #include <linux/of_device.h>
>> @@ -23,6 +24,7 @@
>>  #include <linux/regmap.h>
>>  #include <linux/of.h>
>>  #include <linux/firmware/xlnx-zynqmp.h>
>> +#include <linux/firmware/intel/keembay_firmware.h>
>>
>>  #include "cqhci.h"
>>  #include "sdhci-pltfm.h"
>> @@ -150,6 +152,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 +364,113 @@ static int sdhci_arasan_voltage_switch(struct
>mmc_host *mmc,
>>  	return -EINVAL;
>>  }
>>
>> +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;
>
>nit: put it to one line.
Noted. Done the changes.
>
>> +	int ret;
>> +
>> +	switch (ios->signal_voltage) {
>> +	case MMC_SIGNAL_VOLTAGE_180:
>> +		clk  = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>
>nit: double space
Noted. Done the changes.
>
>> +		clk &= ~SDHCI_CLOCK_CARD_EN;
>> +		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>> +
>> +		clk  = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>
>nit: double space again.
Noted. Done the changes.
>
>> +		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
>> +		 * which is controlling by GPIO Expander.
>> +		 */
>> +		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 =
>keembay_sd_voltage_selection(KEEMBAY_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
>> +		 * which is controlling by GPIO Expander.
>> +		 */
>> +		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 =
>keembay_sd_voltage_selection(KEEMBAY_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 +1631,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;
>
>nit: I got this but as I see 3 lines below maybe would be better to use it
>everywhere but it can be done in separate patch.
>
>>  	const struct sdhci_arasan_of_data *data;
>>
>>  	match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
>@@
>> -1600,6 +1711,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);
>
>I can't see change in dt binding to record uhs gpio.
Noted. Done the changes. Will add binding for this
>
>
>Better
>sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs",
>GPIOD_OUT_HIGH);
>
>then you can avoid uhs variable.
Will remain as it is to make it more readable.
>
>> +		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);
>>
>
>Thanks,
>Michal

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

[Zulkifli, Muhammad Husaini]

Hi Michal,

Thanks again for the feedback. I replied inline

>-----Original Message-----
>From: Michal Simek <michal.simek@xilinx.com>
>Sent: Wednesday, October 7, 2020 4:34 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>sudeep.holla@arm.com; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org;
>linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>Wan Mohamad, Wan Ahmad Zainie
><wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>Subject: Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>
>
>On 06. 10. 20 17:55, 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.
>> There are 2 places to control the voltage.
>> 1) By setting the AON register using firmware driver calling
>> system-level platform management layer (SMC) to set the register.
>> 2) By controlling the GPIO expander value to drive either 1.8V or 3.3V
>> for power mux input.
>>
>> 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 | 127
>> +++++++++++++++++++++++++++++
>>  1 file changed, 127 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>> b/drivers/mmc/host/sdhci-of-arasan.c
>> index f186fbd016b1..e681e6f860ba 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -16,6 +16,7 @@
>>   */
>>
>>  #include <linux/clk-provider.h>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/module.h>
>>  #include <linux/of_device.h>
>> @@ -23,6 +24,7 @@
>>  #include <linux/regmap.h>
>>  #include <linux/of.h>
>>  #include <linux/firmware/xlnx-zynqmp.h>
>> +#include <linux/firmware/intel/keembay_firmware.h>
>>
>>  #include "cqhci.h"
>>  #include "sdhci-pltfm.h"
>> @@ -150,6 +152,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 +364,113 @@ static int sdhci_arasan_voltage_switch(struct
>mmc_host *mmc,
>>  	return -EINVAL;
>>  }
>>
>> +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;
>
>nit: put it to one line.
Noted. Done the changes.
>
>> +	int ret;
>> +
>> +	switch (ios->signal_voltage) {
>> +	case MMC_SIGNAL_VOLTAGE_180:
>> +		clk  = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>
>nit: double space
Noted. Done the changes.
>
>> +		clk &= ~SDHCI_CLOCK_CARD_EN;
>> +		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>> +
>> +		clk  = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>
>nit: double space again.
Noted. Done the changes.
>
>> +		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
>> +		 * which is controlling by GPIO Expander.
>> +		 */
>> +		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 =
>keembay_sd_voltage_selection(KEEMBAY_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
>> +		 * which is controlling by GPIO Expander.
>> +		 */
>> +		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 =
>keembay_sd_voltage_selection(KEEMBAY_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 +1631,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;
>
>nit: I got this but as I see 3 lines below maybe would be better to use it
>everywhere but it can be done in separate patch.
>
>>  	const struct sdhci_arasan_of_data *data;
>>
>>  	match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
>@@
>> -1600,6 +1711,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);
>
>I can't see change in dt binding to record uhs gpio.
Noted. Done the changes. Will add binding for this
>
>
>Better
>sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs",
>GPIOD_OUT_HIGH);
>
>then you can avoid uhs variable.
Will remain as it is to make it more readable.
>
>> +		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);
>>
>
>Thanks,
>Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[muhammad.husaini.zulkifli]

From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

Add header file to handle API function for device driver to communicate
with Arm Trusted Firmware.

Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
---
 .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 include/linux/firmware/intel/keembay_firmware.h

diff --git a/include/linux/firmware/intel/keembay_firmware.h b/include/linux/firmware/intel/keembay_firmware.h
new file mode 100644
index 000000000000..9adb8c87b788
--- /dev/null
+++ b/include/linux/firmware/intel/keembay_firmware.h
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *  Intel Keembay SOC Firmware API Layer
+ *
+ *  Copyright (C) 2020-2021, Intel Corporation
+ *
+ *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
+ */
+
+#ifndef __FIRMWARE_KEEMBAY_SMC_H__
+#define __FIRMWARE_KEEMBAY_SMC_H__
+
+#include <linux/arm-smccc.h>
+
+/**
+ * This file defines API function that can be called by device driver in order to
+ * communicate with Arm Trusted Firmware.
+ */
+
+/* Setting for Keem Bay IO Pad Line Voltage Selection */
+#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
+#define KEEMBAY_SET_1V8_VOLT		0x01
+#define KEEMBAY_SET_3V3_VOLT		0x00
+
+#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
+static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
+
+	return res.a0;
+}
+
+int keembay_sd_voltage_selection(int volt)
+{
+	return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0);
+}
+#else
+static inline int keembay_sd_voltage_selection(int volt)
+{
+	return -ENODEV;
+}
+#endif
+
+#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
-- 
2.17.1

[PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[muhammad.husaini.zulkifli]

From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

Add header file to handle API function for device driver to communicate
with Arm Trusted Firmware.

Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
---
 .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 include/linux/firmware/intel/keembay_firmware.h

diff --git a/include/linux/firmware/intel/keembay_firmware.h b/include/linux/firmware/intel/keembay_firmware.h
new file mode 100644
index 000000000000..9adb8c87b788
--- /dev/null
+++ b/include/linux/firmware/intel/keembay_firmware.h
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *  Intel Keembay SOC Firmware API Layer
+ *
+ *  Copyright (C) 2020-2021, Intel Corporation
+ *
+ *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
+ */
+
+#ifndef __FIRMWARE_KEEMBAY_SMC_H__
+#define __FIRMWARE_KEEMBAY_SMC_H__
+
+#include <linux/arm-smccc.h>
+
+/**
+ * This file defines API function that can be called by device driver in order to
+ * communicate with Arm Trusted Firmware.
+ */
+
+/* Setting for Keem Bay IO Pad Line Voltage Selection */
+#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
+#define KEEMBAY_SET_1V8_VOLT		0x01
+#define KEEMBAY_SET_3V3_VOLT		0x00
+
+#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
+static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
+
+	return res.a0;
+}
+
+int keembay_sd_voltage_selection(int volt)
+{
+	return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0);
+}
+#else
+static inline int keembay_sd_voltage_selection(int volt)
+{
+	return -ENODEV;
+}
+#endif
+
+#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.9-rc8 next-20201006]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/muhammad-husaini-zulkifli-intel-com/mmc-sdhci-of-arasan-Enable-UHS-1-support-for-Keem-Bay-SOC/20201007-000542
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 7575fdda569b2a2e8be32c1a64ecb05d6f96a500
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/273db2ef816560e1679afac43fd4e0624d5b7816
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review muhammad-husaini-zulkifli-intel-com/mmc-sdhci-of-arasan-Enable-UHS-1-support-for-Keem-Bay-SOC/20201007-000542
        git checkout 273db2ef816560e1679afac43fd4e0624d5b7816
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/mmc/host/sdhci-of-arasan.c:27:
>> include/linux/firmware/intel/keembay_firmware.h:35:5: warning: no previous prototype for 'keembay_sd_voltage_selection' [-Wmissing-prototypes]
      35 | int keembay_sd_voltage_selection(int volt)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/keembay_sd_voltage_selection +35 include/linux/firmware/intel/keembay_firmware.h

    34	
  > 35	int keembay_sd_voltage_selection(int volt)
    36	{
    37		return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0);
    38	}
    39	#else
    40	static inline int keembay_sd_voltage_selection(int volt)
    41	{
    42		return -ENODEV;
    43	}
    44	#endif
    45	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 53229 bytes --]

Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2423 bytes --]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.9-rc8 next-20201006]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/muhammad-husaini-zulkifli-intel-com/mmc-sdhci-of-arasan-Enable-UHS-1-support-for-Keem-Bay-SOC/20201007-000542
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 7575fdda569b2a2e8be32c1a64ecb05d6f96a500
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/273db2ef816560e1679afac43fd4e0624d5b7816
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review muhammad-husaini-zulkifli-intel-com/mmc-sdhci-of-arasan-Enable-UHS-1-support-for-Keem-Bay-SOC/20201007-000542
        git checkout 273db2ef816560e1679afac43fd4e0624d5b7816
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/mmc/host/sdhci-of-arasan.c:27:
>> include/linux/firmware/intel/keembay_firmware.h:35:5: warning: no previous prototype for 'keembay_sd_voltage_selection' [-Wmissing-prototypes]
      35 | int keembay_sd_voltage_selection(int volt)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/keembay_sd_voltage_selection +35 include/linux/firmware/intel/keembay_firmware.h

    34	
  > 35	int keembay_sd_voltage_selection(int volt)
    36	{
    37		return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0);
    38	}
    39	#else
    40	static inline int keembay_sd_voltage_selection(int volt)
    41	{
    42		return -ENODEV;
    43	}
    44	#endif
    45	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 53229 bytes --]

Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.9-rc8 next-20201006]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/muhammad-husaini-zulkifli-intel-com/mmc-sdhci-of-arasan-Enable-UHS-1-support-for-Keem-Bay-SOC/20201007-000542
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 7575fdda569b2a2e8be32c1a64ecb05d6f96a500
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/273db2ef816560e1679afac43fd4e0624d5b7816
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review muhammad-husaini-zulkifli-intel-com/mmc-sdhci-of-arasan-Enable-UHS-1-support-for-Keem-Bay-SOC/20201007-000542
        git checkout 273db2ef816560e1679afac43fd4e0624d5b7816
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/mmc/host/sdhci-of-arasan.c:27:
>> include/linux/firmware/intel/keembay_firmware.h:35:5: warning: no previous prototype for 'keembay_sd_voltage_selection' [-Wmissing-prototypes]
      35 | int keembay_sd_voltage_selection(int volt)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/keembay_sd_voltage_selection +35 include/linux/firmware/intel/keembay_firmware.h

    34	
  > 35	int keembay_sd_voltage_selection(int volt)
    36	{
    37		return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0);
    38	}
    39	#else
    40	static inline int keembay_sd_voltage_selection(int volt)
    41	{
    42		return -ENODEV;
    43	}
    44	#endif
    45	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 53229 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Zulkifli, Muhammad Husaini]

Hi Sudeep and Michal,

I would like to receive some feedback on this patch before send out another version to fix the autobot compiler warning.

Really appreciated it.
Thanks

>-----Original Message-----
>From: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Sent: Tuesday, October 6, 2020 11:56 PM
>To: Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>sudeep.holla@arm.com; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org;
>linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>Wan Mohamad, Wan Ahmad Zainie
><wan.ahmad.zainie.wan.mohamad@intel.com>; Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@intel.com>; arnd@arndb.de
>Subject: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted
>Firmware Service call
>
>From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>
>Add header file to handle API function for device driver to communicate with
>Arm Trusted Firmware.
>
>Signed-off-by: Muhammad Husaini Zulkifli
><muhammad.husaini.zulkifli@intel.com>
>---
> .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
> 1 file changed, 46 insertions(+)
> create mode 100644 include/linux/firmware/intel/keembay_firmware.h
>
>diff --git a/include/linux/firmware/intel/keembay_firmware.h
>b/include/linux/firmware/intel/keembay_firmware.h
>new file mode 100644
>index 000000000000..9adb8c87b788
>--- /dev/null
>+++ b/include/linux/firmware/intel/keembay_firmware.h
>@@ -0,0 +1,46 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+/*
>+ *  Intel Keembay SOC Firmware API Layer
>+ *
>+ *  Copyright (C) 2020-2021, Intel Corporation
>+ *
>+ *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>+ */
>+
>+#ifndef __FIRMWARE_KEEMBAY_SMC_H__
>+#define __FIRMWARE_KEEMBAY_SMC_H__
>+
>+#include <linux/arm-smccc.h>
>+
>+/**
>+ * This file defines API function that can be called by device driver
>+in order to
>+ * communicate with Arm Trusted Firmware.
>+ */
>+
>+/* Setting for Keem Bay IO Pad Line Voltage Selection */
>+#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
>+#define KEEMBAY_SET_1V8_VOLT		0x01
>+#define KEEMBAY_SET_3V3_VOLT		0x00
>+
>+#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
>+static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1) {
>+	struct arm_smccc_res res;
>+
>+	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
>+
>+	return res.a0;
>+}
>+
>+int keembay_sd_voltage_selection(int volt) {
>+	return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0); }
>#else
>+static inline int keembay_sd_voltage_selection(int volt) {
>+	return -ENODEV;
>+}
>+#endif
>+
>+#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
>--
>2.17.1

RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Zulkifli, Muhammad Husaini]

Hi Sudeep and Michal,

I would like to receive some feedback on this patch before send out another version to fix the autobot compiler warning.

Really appreciated it.
Thanks

>-----Original Message-----
>From: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Sent: Tuesday, October 6, 2020 11:56 PM
>To: Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>sudeep.holla@arm.com; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org;
>linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>Wan Mohamad, Wan Ahmad Zainie
><wan.ahmad.zainie.wan.mohamad@intel.com>; Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@intel.com>; arnd@arndb.de
>Subject: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted
>Firmware Service call
>
>From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>
>Add header file to handle API function for device driver to communicate with
>Arm Trusted Firmware.
>
>Signed-off-by: Muhammad Husaini Zulkifli
><muhammad.husaini.zulkifli@intel.com>
>---
> .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
> 1 file changed, 46 insertions(+)
> create mode 100644 include/linux/firmware/intel/keembay_firmware.h
>
>diff --git a/include/linux/firmware/intel/keembay_firmware.h
>b/include/linux/firmware/intel/keembay_firmware.h
>new file mode 100644
>index 000000000000..9adb8c87b788
>--- /dev/null
>+++ b/include/linux/firmware/intel/keembay_firmware.h
>@@ -0,0 +1,46 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+/*
>+ *  Intel Keembay SOC Firmware API Layer
>+ *
>+ *  Copyright (C) 2020-2021, Intel Corporation
>+ *
>+ *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>+ */
>+
>+#ifndef __FIRMWARE_KEEMBAY_SMC_H__
>+#define __FIRMWARE_KEEMBAY_SMC_H__
>+
>+#include <linux/arm-smccc.h>
>+
>+/**
>+ * This file defines API function that can be called by device driver
>+in order to
>+ * communicate with Arm Trusted Firmware.
>+ */
>+
>+/* Setting for Keem Bay IO Pad Line Voltage Selection */
>+#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
>+#define KEEMBAY_SET_1V8_VOLT		0x01
>+#define KEEMBAY_SET_3V3_VOLT		0x00
>+
>+#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
>+static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1) {
>+	struct arm_smccc_res res;
>+
>+	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
>+
>+	return res.a0;
>+}
>+
>+int keembay_sd_voltage_selection(int volt) {
>+	return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0); }
>#else
>+static inline int keembay_sd_voltage_selection(int volt) {
>+	return -ENODEV;
>+}
>+#endif
>+
>+#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
>--
>2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Michal Simek]

Hi,

1. Keem Bay: in subject is wrong. Tools are working with it and you
should just use keembay: instead.

2. This should come first before actual change to keep the tree bisectable.

On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> 
> Add header file to handle API function for device driver to communicate
> with Arm Trusted Firmware.
> 
> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> ---
>  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
> 
> diff --git a/include/linux/firmware/intel/keembay_firmware.h b/include/linux/firmware/intel/keembay_firmware.h
> new file mode 100644
> index 000000000000..9adb8c87b788
> --- /dev/null
> +++ b/include/linux/firmware/intel/keembay_firmware.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *  Intel Keembay SOC Firmware API Layer
> + *
> + *  Copyright (C) 2020-2021, Intel Corporation
> + *
> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
> + */
> +
> +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
> +#define __FIRMWARE_KEEMBAY_SMC_H__
> +
> +#include <linux/arm-smccc.h>
> +
> +/**

This is not a kernel doc comment. Just use /*

> + * This file defines API function that can be called by device driver in order to
> + * communicate with Arm Trusted Firmware.
> + */
> +
> +/* Setting for Keem Bay IO Pad Line Voltage Selection */
> +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26

Sudeep: Don't we have any macros for composing these IDs?
nit: IMHO composing these IDs from macros would make more sense to me.


> +#define KEEMBAY_SET_1V8_VOLT		0x01

0x01 is just 1

> +#define KEEMBAY_SET_3V3_VOLT		0x00

0x00 is just 0

> +
> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
> +static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
> +
> +	return res.a0;

I am not big fan of this error propagation in case of failure.

If smc fails you get via res.a0 SMCCC_RET_NOT_SUPPORTED which is defined
as -1 which is based on errno-base.h defined as EPERM.

That driver which Sudeep pointed you to is using EINVAL instead.

It means I would add a code to check it.


> +}
> +
> +int keembay_sd_voltage_selection(int volt)

as was reported by robot

> +{
> +	return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0);
> +}
> +#else
> +static inline int keembay_sd_voltage_selection(int volt)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
> +#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
> 

M

Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Michal Simek]

Hi,

1. Keem Bay: in subject is wrong. Tools are working with it and you
should just use keembay: instead.

2. This should come first before actual change to keep the tree bisectable.

On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> 
> Add header file to handle API function for device driver to communicate
> with Arm Trusted Firmware.
> 
> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> ---
>  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
> 
> diff --git a/include/linux/firmware/intel/keembay_firmware.h b/include/linux/firmware/intel/keembay_firmware.h
> new file mode 100644
> index 000000000000..9adb8c87b788
> --- /dev/null
> +++ b/include/linux/firmware/intel/keembay_firmware.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *  Intel Keembay SOC Firmware API Layer
> + *
> + *  Copyright (C) 2020-2021, Intel Corporation
> + *
> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
> + */
> +
> +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
> +#define __FIRMWARE_KEEMBAY_SMC_H__
> +
> +#include <linux/arm-smccc.h>
> +
> +/**

This is not a kernel doc comment. Just use /*

> + * This file defines API function that can be called by device driver in order to
> + * communicate with Arm Trusted Firmware.
> + */
> +
> +/* Setting for Keem Bay IO Pad Line Voltage Selection */
> +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26

Sudeep: Don't we have any macros for composing these IDs?
nit: IMHO composing these IDs from macros would make more sense to me.


> +#define KEEMBAY_SET_1V8_VOLT		0x01

0x01 is just 1

> +#define KEEMBAY_SET_3V3_VOLT		0x00

0x00 is just 0

> +
> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
> +static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
> +
> +	return res.a0;

I am not big fan of this error propagation in case of failure.

If smc fails you get via res.a0 SMCCC_RET_NOT_SUPPORTED which is defined
as -1 which is based on errno-base.h defined as EPERM.

That driver which Sudeep pointed you to is using EINVAL instead.

It means I would add a code to check it.


> +}
> +
> +int keembay_sd_voltage_selection(int volt)

as was reported by robot

> +{
> +	return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0);
> +}
> +#else
> +static inline int keembay_sd_voltage_selection(int volt)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
> +#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
> 

M

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Sudeep Holla]

On Wed, Oct 07, 2020 at 10:20:21AM +0200, Michal Simek wrote:
> Hi,
> 
> 1. Keem Bay: in subject is wrong. Tools are working with it and you
> should just use keembay: instead.
> 
> 2. This should come first before actual change to keep the tree bisectable.
> 
> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
> > From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> > 
> > Add header file to handle API function for device driver to communicate
> > with Arm Trusted Firmware.
> > 
> > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> > ---
> >  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> >  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
> > 
> > diff --git a/include/linux/firmware/intel/keembay_firmware.h b/include/linux/firmware/intel/keembay_firmware.h
> > new file mode 100644
> > index 000000000000..9adb8c87b788
> > --- /dev/null
> > +++ b/include/linux/firmware/intel/keembay_firmware.h
> > @@ -0,0 +1,46 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + *  Intel Keembay SOC Firmware API Layer
> > + *
> > + *  Copyright (C) 2020-2021, Intel Corporation
> > + *
> > + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
> > + */
> > +
> > +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
> > +#define __FIRMWARE_KEEMBAY_SMC_H__
> > +
> > +#include <linux/arm-smccc.h>
> > +
> > +/**
> 
> This is not a kernel doc comment. Just use /*
> 
> > + * This file defines API function that can be called by device driver in order to
> > + * communicate with Arm Trusted Firmware.
> > + */
> > +
> > +/* Setting for Keem Bay IO Pad Line Voltage Selection */
> > +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
> 
> Sudeep: Don't we have any macros for composing these IDs?
> nit: IMHO composing these IDs from macros would make more sense to me.
> 

Yes we do. Refer include/linux/arm-smccc.h
I expect something like below, which also indicated you are using wrong
OWNER space. You can't be 0 which is reserved for CPU ARCH. You need to
be SIP(0x2)

#define ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE          \
        ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,       \
                           ARM_SMCCC_SMC_32,          \
                           ARM_SMCCC_OWNER_SIP,       \
                           0xFF26)


-- 
Regards,
Sudeep

Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Sudeep Holla]

On Wed, Oct 07, 2020 at 10:20:21AM +0200, Michal Simek wrote:
> Hi,
> 
> 1. Keem Bay: in subject is wrong. Tools are working with it and you
> should just use keembay: instead.
> 
> 2. This should come first before actual change to keep the tree bisectable.
> 
> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
> > From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> > 
> > Add header file to handle API function for device driver to communicate
> > with Arm Trusted Firmware.
> > 
> > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> > ---
> >  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> >  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
> > 
> > diff --git a/include/linux/firmware/intel/keembay_firmware.h b/include/linux/firmware/intel/keembay_firmware.h
> > new file mode 100644
> > index 000000000000..9adb8c87b788
> > --- /dev/null
> > +++ b/include/linux/firmware/intel/keembay_firmware.h
> > @@ -0,0 +1,46 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + *  Intel Keembay SOC Firmware API Layer
> > + *
> > + *  Copyright (C) 2020-2021, Intel Corporation
> > + *
> > + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
> > + */
> > +
> > +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
> > +#define __FIRMWARE_KEEMBAY_SMC_H__
> > +
> > +#include <linux/arm-smccc.h>
> > +
> > +/**
> 
> This is not a kernel doc comment. Just use /*
> 
> > + * This file defines API function that can be called by device driver in order to
> > + * communicate with Arm Trusted Firmware.
> > + */
> > +
> > +/* Setting for Keem Bay IO Pad Line Voltage Selection */
> > +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
> 
> Sudeep: Don't we have any macros for composing these IDs?
> nit: IMHO composing these IDs from macros would make more sense to me.
> 

Yes we do. Refer include/linux/arm-smccc.h
I expect something like below, which also indicated you are using wrong
OWNER space. You can't be 0 which is reserved for CPU ARCH. You need to
be SIP(0x2)

#define ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE          \
        ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,       \
                           ARM_SMCCC_SMC_32,          \
                           ARM_SMCCC_OWNER_SIP,       \
                           0xFF26)


-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Zulkifli, Muhammad Husaini]

Hi Sudeep,

Thanks for the feedback. I replied inline

>-----Original Message-----
>From: Sudeep Holla <sudeep.holla@arm.com>
>Sent: Wednesday, October 7, 2020 6:11 PM
>To: Michal Simek <michal.simek@xilinx.com>
>Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>Hunter, Adrian <adrian.hunter@intel.com>; Sudeep Holla
><sudeep.holla@arm.com>; ulf.hansson@linaro.org; linux-
>mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>kernel@vger.kernel.org; Raja Subramanian, Lakshmi Bai
><lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad
>Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted
>Firmware Service call
>
>On Wed, Oct 07, 2020 at 10:20:21AM +0200, Michal Simek wrote:
>> Hi,
>>
>> 1. Keem Bay: in subject is wrong. Tools are working with it and you
>> should just use keembay: instead.
>>
>> 2. This should come first before actual change to keep the tree bisectable.
>>
>> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>> > From: Muhammad Husaini Zulkifli
>> > <muhammad.husaini.zulkifli@intel.com>
>> >
>> > Add header file to handle API function for device driver to
>> > communicate with Arm Trusted Firmware.
>> >
>> > Signed-off-by: Muhammad Husaini Zulkifli
>> > <muhammad.husaini.zulkifli@intel.com>
>> > ---
>> >  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
>> >  1 file changed, 46 insertions(+)
>> >  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
>> >
>> > diff --git a/include/linux/firmware/intel/keembay_firmware.h
>> > b/include/linux/firmware/intel/keembay_firmware.h
>> > new file mode 100644
>> > index 000000000000..9adb8c87b788
>> > --- /dev/null
>> > +++ b/include/linux/firmware/intel/keembay_firmware.h
>> > @@ -0,0 +1,46 @@
>> > +/* SPDX-License-Identifier: GPL-2.0 */
>> > +/*
>> > + *  Intel Keembay SOC Firmware API Layer
>> > + *
>> > + *  Copyright (C) 2020-2021, Intel Corporation
>> > + *
>> > + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>> > + */
>> > +
>> > +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
>> > +#define __FIRMWARE_KEEMBAY_SMC_H__
>> > +
>> > +#include <linux/arm-smccc.h>
>> > +
>> > +/**
>>
>> This is not a kernel doc comment. Just use /*
>>
>> > + * This file defines API function that can be called by device
>> > + driver in order to
>> > + * communicate with Arm Trusted Firmware.
>> > + */
>> > +
>> > +/* Setting for Keem Bay IO Pad Line Voltage Selection */
>> > +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
>>
>> Sudeep: Don't we have any macros for composing these IDs?
>> nit: IMHO composing these IDs from macros would make more sense to me.
>>
>
>Yes we do. Refer include/linux/arm-smccc.h I expect something like below,
>which also indicated you are using wrong OWNER space. You can't be 0 which is
>reserved for CPU ARCH. You need to be SIP(0x2)
>
>#define ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE          \
>        ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,       \
>                           ARM_SMCCC_SMC_32,          \
>                           ARM_SMCCC_OWNER_SIP,       \
>                           0xFF26)
>
>
>--

Testing with below func and definition. It is working .
#define ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE          \
        ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,       \
                           ARM_SMCCC_SMC_32,          \
                           ARM_SMCCC_OWNER_SIP,       \
                           0xFF26)

int keembay_sd_voltage_selection(int volt)
{
	struct arm_smccc_res res;
	
	arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE, volt, &res);
	if ((int)res.a0 < 0)
		return -EINVAL;

	return 0;
}
>Regards,
>Sudeep

RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Zulkifli, Muhammad Husaini]

Hi Sudeep,

Thanks for the feedback. I replied inline

>-----Original Message-----
>From: Sudeep Holla <sudeep.holla@arm.com>
>Sent: Wednesday, October 7, 2020 6:11 PM
>To: Michal Simek <michal.simek@xilinx.com>
>Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>Hunter, Adrian <adrian.hunter@intel.com>; Sudeep Holla
><sudeep.holla@arm.com>; ulf.hansson@linaro.org; linux-
>mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>kernel@vger.kernel.org; Raja Subramanian, Lakshmi Bai
><lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad
>Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted
>Firmware Service call
>
>On Wed, Oct 07, 2020 at 10:20:21AM +0200, Michal Simek wrote:
>> Hi,
>>
>> 1. Keem Bay: in subject is wrong. Tools are working with it and you
>> should just use keembay: instead.
>>
>> 2. This should come first before actual change to keep the tree bisectable.
>>
>> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>> > From: Muhammad Husaini Zulkifli
>> > <muhammad.husaini.zulkifli@intel.com>
>> >
>> > Add header file to handle API function for device driver to
>> > communicate with Arm Trusted Firmware.
>> >
>> > Signed-off-by: Muhammad Husaini Zulkifli
>> > <muhammad.husaini.zulkifli@intel.com>
>> > ---
>> >  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
>> >  1 file changed, 46 insertions(+)
>> >  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
>> >
>> > diff --git a/include/linux/firmware/intel/keembay_firmware.h
>> > b/include/linux/firmware/intel/keembay_firmware.h
>> > new file mode 100644
>> > index 000000000000..9adb8c87b788
>> > --- /dev/null
>> > +++ b/include/linux/firmware/intel/keembay_firmware.h
>> > @@ -0,0 +1,46 @@
>> > +/* SPDX-License-Identifier: GPL-2.0 */
>> > +/*
>> > + *  Intel Keembay SOC Firmware API Layer
>> > + *
>> > + *  Copyright (C) 2020-2021, Intel Corporation
>> > + *
>> > + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>> > + */
>> > +
>> > +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
>> > +#define __FIRMWARE_KEEMBAY_SMC_H__
>> > +
>> > +#include <linux/arm-smccc.h>
>> > +
>> > +/**
>>
>> This is not a kernel doc comment. Just use /*
>>
>> > + * This file defines API function that can be called by device
>> > + driver in order to
>> > + * communicate with Arm Trusted Firmware.
>> > + */
>> > +
>> > +/* Setting for Keem Bay IO Pad Line Voltage Selection */
>> > +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
>>
>> Sudeep: Don't we have any macros for composing these IDs?
>> nit: IMHO composing these IDs from macros would make more sense to me.
>>
>
>Yes we do. Refer include/linux/arm-smccc.h I expect something like below,
>which also indicated you are using wrong OWNER space. You can't be 0 which is
>reserved for CPU ARCH. You need to be SIP(0x2)
>
>#define ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE          \
>        ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,       \
>                           ARM_SMCCC_SMC_32,          \
>                           ARM_SMCCC_OWNER_SIP,       \
>                           0xFF26)
>
>
>--

Testing with below func and definition. It is working .
#define ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE          \
        ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,       \
                           ARM_SMCCC_SMC_32,          \
                           ARM_SMCCC_OWNER_SIP,       \
                           0xFF26)

int keembay_sd_voltage_selection(int volt)
{
	struct arm_smccc_res res;
	
	arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE, volt, &res);
	if ((int)res.a0 < 0)
		return -EINVAL;

	return 0;
}
>Regards,
>Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Zulkifli, Muhammad Husaini]

Hi Michal,

Thanks for the feedback. I replied inline

>-----Original Message-----
>From: Michal Simek <michal.simek@xilinx.com>
>Sent: Wednesday, October 7, 2020 4:20 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>sudeep.holla@arm.com; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org;
>linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>Wan Mohamad, Wan Ahmad Zainie
><wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted
>Firmware Service call
>
>Hi,
>
>1. Keem Bay: in subject is wrong. Tools are working with it and you should just
>use keembay: instead.
Are you saying like this ? 
Keem Bay: Add support for Arm Trusted Firmware Service call

>
>2. This should come first before actual change to keep the tree bisectable.
Noted. Done the changes
>
>On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>>
>> Add header file to handle API function for device driver to
>> communicate with Arm Trusted Firmware.
>>
>> Signed-off-by: Muhammad Husaini Zulkifli
>> <muhammad.husaini.zulkifli@intel.com>
>> ---
>>  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
>>
>> diff --git a/include/linux/firmware/intel/keembay_firmware.h
>> b/include/linux/firmware/intel/keembay_firmware.h
>> new file mode 100644
>> index 000000000000..9adb8c87b788
>> --- /dev/null
>> +++ b/include/linux/firmware/intel/keembay_firmware.h
>> @@ -0,0 +1,46 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + *  Intel Keembay SOC Firmware API Layer
>> + *
>> + *  Copyright (C) 2020-2021, Intel Corporation
>> + *
>> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>> + */
>> +
>> +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
>> +#define __FIRMWARE_KEEMBAY_SMC_H__
>> +
>> +#include <linux/arm-smccc.h>
>> +
>> +/**
>
>This is not a kernel doc comment. Just use /*
>
>> + * This file defines API function that can be called by device driver
>> + in order to
>> + * communicate with Arm Trusted Firmware.
>> + */
>> +
>> +/* Setting for Keem Bay IO Pad Line Voltage Selection */
>> +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
>
>Sudeep: Don't we have any macros for composing these IDs?
>nit: IMHO composing these IDs from macros would make more sense to me.
>
>
>> +#define KEEMBAY_SET_1V8_VOLT		0x01
>
>0x01 is just 1
Noted. Done the changes
>
>> +#define KEEMBAY_SET_3V3_VOLT		0x00
>
>0x00 is just 0
Noted. Done the changes
>
>> +
>> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
>> +static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1) {
>> +	struct arm_smccc_res res;
>> +
>> +	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
>> +
>> +	return res.a0;
>
>I am not big fan of this error propagation in case of failure.
>
>If smc fails you get via res.a0 SMCCC_RET_NOT_SUPPORTED which is defined as
>-1 which is based on errno-base.h defined as EPERM.
>
>That driver which Sudeep pointed you to is using EINVAL instead.
>
>It means I would add a code to check it.

Yeah I changed to below line of codes. Is this Ok? Tested working.
int keembay_sd_voltage_selection(int volt)
{
	struct arm_smccc_res res;

	arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE, volt, &res);
	if ((int)res.a0 < 0)
		return -EINVAL;

	return 0;
}

>
>
>> +}
>> +
>> +int keembay_sd_voltage_selection(int volt)
>
>as was reported by robot
Added the func prototype. 
int keembay_sd_voltage_selection(int volt);
No error observed after that.
>
>> +{
>> +	return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0); }
>> +#else static inline int keembay_sd_voltage_selection(int volt) {
>> +	return -ENODEV;
>> +}
>> +#endif
>> +
>> +#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
>>
>
>M

RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Zulkifli, Muhammad Husaini]

Hi Michal,

Thanks for the feedback. I replied inline

>-----Original Message-----
>From: Michal Simek <michal.simek@xilinx.com>
>Sent: Wednesday, October 7, 2020 4:20 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>sudeep.holla@arm.com; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org;
>linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>Wan Mohamad, Wan Ahmad Zainie
><wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted
>Firmware Service call
>
>Hi,
>
>1. Keem Bay: in subject is wrong. Tools are working with it and you should just
>use keembay: instead.
Are you saying like this ? 
Keem Bay: Add support for Arm Trusted Firmware Service call

>
>2. This should come first before actual change to keep the tree bisectable.
Noted. Done the changes
>
>On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>>
>> Add header file to handle API function for device driver to
>> communicate with Arm Trusted Firmware.
>>
>> Signed-off-by: Muhammad Husaini Zulkifli
>> <muhammad.husaini.zulkifli@intel.com>
>> ---
>>  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
>>
>> diff --git a/include/linux/firmware/intel/keembay_firmware.h
>> b/include/linux/firmware/intel/keembay_firmware.h
>> new file mode 100644
>> index 000000000000..9adb8c87b788
>> --- /dev/null
>> +++ b/include/linux/firmware/intel/keembay_firmware.h
>> @@ -0,0 +1,46 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + *  Intel Keembay SOC Firmware API Layer
>> + *
>> + *  Copyright (C) 2020-2021, Intel Corporation
>> + *
>> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>> + */
>> +
>> +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
>> +#define __FIRMWARE_KEEMBAY_SMC_H__
>> +
>> +#include <linux/arm-smccc.h>
>> +
>> +/**
>
>This is not a kernel doc comment. Just use /*
>
>> + * This file defines API function that can be called by device driver
>> + in order to
>> + * communicate with Arm Trusted Firmware.
>> + */
>> +
>> +/* Setting for Keem Bay IO Pad Line Voltage Selection */
>> +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
>
>Sudeep: Don't we have any macros for composing these IDs?
>nit: IMHO composing these IDs from macros would make more sense to me.
>
>
>> +#define KEEMBAY_SET_1V8_VOLT		0x01
>
>0x01 is just 1
Noted. Done the changes
>
>> +#define KEEMBAY_SET_3V3_VOLT		0x00
>
>0x00 is just 0
Noted. Done the changes
>
>> +
>> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
>> +static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1) {
>> +	struct arm_smccc_res res;
>> +
>> +	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
>> +
>> +	return res.a0;
>
>I am not big fan of this error propagation in case of failure.
>
>If smc fails you get via res.a0 SMCCC_RET_NOT_SUPPORTED which is defined as
>-1 which is based on errno-base.h defined as EPERM.
>
>That driver which Sudeep pointed you to is using EINVAL instead.
>
>It means I would add a code to check it.

Yeah I changed to below line of codes. Is this Ok? Tested working.
int keembay_sd_voltage_selection(int volt)
{
	struct arm_smccc_res res;

	arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE, volt, &res);
	if ((int)res.a0 < 0)
		return -EINVAL;

	return 0;
}

>
>
>> +}
>> +
>> +int keembay_sd_voltage_selection(int volt)
>
>as was reported by robot
Added the func prototype. 
int keembay_sd_voltage_selection(int volt);
No error observed after that.
>
>> +{
>> +	return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0); }
>> +#else static inline int keembay_sd_voltage_selection(int volt) {
>> +	return -ENODEV;
>> +}
>> +#endif
>> +
>> +#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
>>
>
>M
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Michal Simek]

Hi,

On 07. 10. 20 15:21, Zulkifli, Muhammad Husaini wrote:
> Hi Michal,
> 
> Thanks for the feedback. I replied inline
> 
>> -----Original Message-----
>> From: Michal Simek <michal.simek@xilinx.com>
>> Sent: Wednesday, October 7, 2020 4:20 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>> Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>> sudeep.holla@arm.com; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>> Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>> Wan Mohamad, Wan Ahmad Zainie
>> <wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>> Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted
>> Firmware Service call
>>
>> Hi,
>>
>> 1. Keem Bay: in subject is wrong. Tools are working with it and you should just
>> use keembay: instead.
> Are you saying like this ? 
> Keem Bay: Add support for Arm Trusted Firmware Service call

like this:
firmware: keembay: Add support for Arm Trusted Firmware Service call

> 
>>
>> 2. This should come first before actual change to keep the tree bisectable.
> Noted. Done the changes
>>
>> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>>>
>>> Add header file to handle API function for device driver to
>>> communicate with Arm Trusted Firmware.
>>>
>>> Signed-off-by: Muhammad Husaini Zulkifli
>>> <muhammad.husaini.zulkifli@intel.com>
>>> ---
>>>  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
>>>  1 file changed, 46 insertions(+)
>>>  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
>>>
>>> diff --git a/include/linux/firmware/intel/keembay_firmware.h
>>> b/include/linux/firmware/intel/keembay_firmware.h
>>> new file mode 100644
>>> index 000000000000..9adb8c87b788
>>> --- /dev/null
>>> +++ b/include/linux/firmware/intel/keembay_firmware.h
>>> @@ -0,0 +1,46 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + *  Intel Keembay SOC Firmware API Layer
>>> + *
>>> + *  Copyright (C) 2020-2021, Intel Corporation
>>> + *
>>> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>>> + */
>>> +
>>> +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
>>> +#define __FIRMWARE_KEEMBAY_SMC_H__
>>> +
>>> +#include <linux/arm-smccc.h>
>>> +
>>> +/**
>>
>> This is not a kernel doc comment. Just use /*
>>
>>> + * This file defines API function that can be called by device driver
>>> + in order to
>>> + * communicate with Arm Trusted Firmware.
>>> + */
>>> +
>>> +/* Setting for Keem Bay IO Pad Line Voltage Selection */
>>> +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
>>
>> Sudeep: Don't we have any macros for composing these IDs?
>> nit: IMHO composing these IDs from macros would make more sense to me.
>>
>>
>>> +#define KEEMBAY_SET_1V8_VOLT		0x01
>>
>> 0x01 is just 1
> Noted. Done the changes
>>
>>> +#define KEEMBAY_SET_3V3_VOLT		0x00
>>
>> 0x00 is just 0
> Noted. Done the changes
>>
>>> +
>>> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
>>> +static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1) {
>>> +	struct arm_smccc_res res;
>>> +
>>> +	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
>>> +
>>> +	return res.a0;
>>
>> I am not big fan of this error propagation in case of failure.
>>
>> If smc fails you get via res.a0 SMCCC_RET_NOT_SUPPORTED which is defined as
>> -1 which is based on errno-base.h defined as EPERM.
>>
>> That driver which Sudeep pointed you to is using EINVAL instead.
>>
>> It means I would add a code to check it.
> 
> Yeah I changed to below line of codes. Is this Ok? Tested working.
> int keembay_sd_voltage_selection(int volt)

static inline here shouldn't hurt.

> {
> 	struct arm_smccc_res res;
> 
> 	arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE, volt, &res);
> 	if ((int)res.a0 < 0)
> 		return -EINVAL;
> 
> 	return 0;
> }

This is fine.

M

Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Michal Simek]

Hi,

On 07. 10. 20 15:21, Zulkifli, Muhammad Husaini wrote:
> Hi Michal,
> 
> Thanks for the feedback. I replied inline
> 
>> -----Original Message-----
>> From: Michal Simek <michal.simek@xilinx.com>
>> Sent: Wednesday, October 7, 2020 4:20 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>> Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>> sudeep.holla@arm.com; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>> Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>> Wan Mohamad, Wan Ahmad Zainie
>> <wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>> Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted
>> Firmware Service call
>>
>> Hi,
>>
>> 1. Keem Bay: in subject is wrong. Tools are working with it and you should just
>> use keembay: instead.
> Are you saying like this ? 
> Keem Bay: Add support for Arm Trusted Firmware Service call

like this:
firmware: keembay: Add support for Arm Trusted Firmware Service call

> 
>>
>> 2. This should come first before actual change to keep the tree bisectable.
> Noted. Done the changes
>>
>> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>>>
>>> Add header file to handle API function for device driver to
>>> communicate with Arm Trusted Firmware.
>>>
>>> Signed-off-by: Muhammad Husaini Zulkifli
>>> <muhammad.husaini.zulkifli@intel.com>
>>> ---
>>>  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
>>>  1 file changed, 46 insertions(+)
>>>  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
>>>
>>> diff --git a/include/linux/firmware/intel/keembay_firmware.h
>>> b/include/linux/firmware/intel/keembay_firmware.h
>>> new file mode 100644
>>> index 000000000000..9adb8c87b788
>>> --- /dev/null
>>> +++ b/include/linux/firmware/intel/keembay_firmware.h
>>> @@ -0,0 +1,46 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + *  Intel Keembay SOC Firmware API Layer
>>> + *
>>> + *  Copyright (C) 2020-2021, Intel Corporation
>>> + *
>>> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>>> + */
>>> +
>>> +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
>>> +#define __FIRMWARE_KEEMBAY_SMC_H__
>>> +
>>> +#include <linux/arm-smccc.h>
>>> +
>>> +/**
>>
>> This is not a kernel doc comment. Just use /*
>>
>>> + * This file defines API function that can be called by device driver
>>> + in order to
>>> + * communicate with Arm Trusted Firmware.
>>> + */
>>> +
>>> +/* Setting for Keem Bay IO Pad Line Voltage Selection */
>>> +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
>>
>> Sudeep: Don't we have any macros for composing these IDs?
>> nit: IMHO composing these IDs from macros would make more sense to me.
>>
>>
>>> +#define KEEMBAY_SET_1V8_VOLT		0x01
>>
>> 0x01 is just 1
> Noted. Done the changes
>>
>>> +#define KEEMBAY_SET_3V3_VOLT		0x00
>>
>> 0x00 is just 0
> Noted. Done the changes
>>
>>> +
>>> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
>>> +static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1) {
>>> +	struct arm_smccc_res res;
>>> +
>>> +	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
>>> +
>>> +	return res.a0;
>>
>> I am not big fan of this error propagation in case of failure.
>>
>> If smc fails you get via res.a0 SMCCC_RET_NOT_SUPPORTED which is defined as
>> -1 which is based on errno-base.h defined as EPERM.
>>
>> That driver which Sudeep pointed you to is using EINVAL instead.
>>
>> It means I would add a code to check it.
> 
> Yeah I changed to below line of codes. Is this Ok? Tested working.
> int keembay_sd_voltage_selection(int volt)

static inline here shouldn't hurt.

> {
> 	struct arm_smccc_res res;
> 
> 	arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE, volt, &res);
> 	if ((int)res.a0 < 0)
> 		return -EINVAL;
> 
> 	return 0;
> }

This is fine.

M

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Zulkifli, Muhammad Husaini]

Hi,

>-----Original Message-----
>From: Michal Simek <michal.simek@xilinx.com>
>Sent: Wednesday, October 7, 2020 9:37 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian
><adrian.hunter@intel.com>; sudeep.holla@arm.com; ulf.hansson@linaro.org;
>linux-mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>kernel@vger.kernel.org
>Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>Wan Mohamad, Wan Ahmad Zainie
><wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted
>Firmware Service call
>
>Hi,
>
>On 07. 10. 20 15:21, Zulkifli, Muhammad Husaini wrote:
>> Hi Michal,
>>
>> Thanks for the feedback. I replied inline
>>
>>> -----Original Message-----
>>> From: Michal Simek <michal.simek@xilinx.com>
>>> Sent: Wednesday, October 7, 2020 4:20 PM
>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>>> Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>>> sudeep.holla@arm.com; ulf.hansson@linaro.org;
>>> linux-mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>> linux-kernel@vger.kernel.org
>>> Cc: Raja Subramanian, Lakshmi Bai
>>> <lakshmi.bai.raja.subramanian@intel.com>;
>>> Wan Mohamad, Wan Ahmad Zainie
>>> <wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>>> Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm
>>> Trusted Firmware Service call
>>>
>>> Hi,
>>>
>>> 1. Keem Bay: in subject is wrong. Tools are working with it and you
>>> should just use keembay: instead.
>> Are you saying like this ?
>> Keem Bay: Add support for Arm Trusted Firmware Service call
>
>like this:
>firmware: keembay: Add support for Arm Trusted Firmware Service call
>
>>
>>>
>>> 2. This should come first before actual change to keep the tree bisectable.
>> Noted. Done the changes
>>>
>>> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>>>> From: Muhammad Husaini Zulkifli
>>>> <muhammad.husaini.zulkifli@intel.com>
>>>>
>>>> Add header file to handle API function for device driver to
>>>> communicate with Arm Trusted Firmware.
>>>>
>>>> Signed-off-by: Muhammad Husaini Zulkifli
>>>> <muhammad.husaini.zulkifli@intel.com>
>>>> ---
>>>>  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
>>>>  1 file changed, 46 insertions(+)
>>>>  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
>>>>
>>>> diff --git a/include/linux/firmware/intel/keembay_firmware.h
>>>> b/include/linux/firmware/intel/keembay_firmware.h
>>>> new file mode 100644
>>>> index 000000000000..9adb8c87b788
>>>> --- /dev/null
>>>> +++ b/include/linux/firmware/intel/keembay_firmware.h
>>>> @@ -0,0 +1,46 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + *  Intel Keembay SOC Firmware API Layer
>>>> + *
>>>> + *  Copyright (C) 2020-2021, Intel Corporation
>>>> + *
>>>> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>>>> + */
>>>> +
>>>> +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
>>>> +#define __FIRMWARE_KEEMBAY_SMC_H__
>>>> +
>>>> +#include <linux/arm-smccc.h>
>>>> +
>>>> +/**
>>>
>>> This is not a kernel doc comment. Just use /*
>>>
>>>> + * This file defines API function that can be called by device
>>>> + driver in order to
>>>> + * communicate with Arm Trusted Firmware.
>>>> + */
>>>> +
>>>> +/* Setting for Keem Bay IO Pad Line Voltage Selection */
>>>> +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
>>>
>>> Sudeep: Don't we have any macros for composing these IDs?
>>> nit: IMHO composing these IDs from macros would make more sense to me.
>>>
>>>
>>>> +#define KEEMBAY_SET_1V8_VOLT		0x01
>>>
>>> 0x01 is just 1
>> Noted. Done the changes
>>>
>>>> +#define KEEMBAY_SET_3V3_VOLT		0x00
>>>
>>> 0x00 is just 0
>> Noted. Done the changes
>>>
>>>> +
>>>> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
>>>> +static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1) {
>>>> +	struct arm_smccc_res res;
>>>> +
>>>> +	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
>>>> +
>>>> +	return res.a0;
>>>
>>> I am not big fan of this error propagation in case of failure.
>>>
>>> If smc fails you get via res.a0 SMCCC_RET_NOT_SUPPORTED which is
>>> defined as
>>> -1 which is based on errno-base.h defined as EPERM.
>>>
>>> That driver which Sudeep pointed you to is using EINVAL instead.
>>>
>>> It means I would add a code to check it.
>>
>> Yeah I changed to below line of codes. Is this Ok? Tested working.
>> int keembay_sd_voltage_selection(int volt)
>
>static inline here shouldn't hurt.
due to func() prototype " int keembay_sd_voltage_selection(int volt);" to solve warning issues by robot , I cannot set static inline here. 
Will observed below error:

error: static declaration of ‘keembay_sd_voltage_selection’ follows non-static declaration
 static inline int keembay_sd_voltage_selection(int volt).

>
>> {
>> 	struct arm_smccc_res res;
>>
>>
>	arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAG
>E, volt, &res);
>> 	if ((int)res.a0 < 0)
>> 		return -EINVAL;
>>
>> 	return 0;
>> }
>
>This is fine.
>
>M

RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Zulkifli, Muhammad Husaini]

Hi,

>-----Original Message-----
>From: Michal Simek <michal.simek@xilinx.com>
>Sent: Wednesday, October 7, 2020 9:37 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian
><adrian.hunter@intel.com>; sudeep.holla@arm.com; ulf.hansson@linaro.org;
>linux-mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>kernel@vger.kernel.org
>Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>Wan Mohamad, Wan Ahmad Zainie
><wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted
>Firmware Service call
>
>Hi,
>
>On 07. 10. 20 15:21, Zulkifli, Muhammad Husaini wrote:
>> Hi Michal,
>>
>> Thanks for the feedback. I replied inline
>>
>>> -----Original Message-----
>>> From: Michal Simek <michal.simek@xilinx.com>
>>> Sent: Wednesday, October 7, 2020 4:20 PM
>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>>> Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>>> sudeep.holla@arm.com; ulf.hansson@linaro.org;
>>> linux-mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>> linux-kernel@vger.kernel.org
>>> Cc: Raja Subramanian, Lakshmi Bai
>>> <lakshmi.bai.raja.subramanian@intel.com>;
>>> Wan Mohamad, Wan Ahmad Zainie
>>> <wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>>> Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm
>>> Trusted Firmware Service call
>>>
>>> Hi,
>>>
>>> 1. Keem Bay: in subject is wrong. Tools are working with it and you
>>> should just use keembay: instead.
>> Are you saying like this ?
>> Keem Bay: Add support for Arm Trusted Firmware Service call
>
>like this:
>firmware: keembay: Add support for Arm Trusted Firmware Service call
>
>>
>>>
>>> 2. This should come first before actual change to keep the tree bisectable.
>> Noted. Done the changes
>>>
>>> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>>>> From: Muhammad Husaini Zulkifli
>>>> <muhammad.husaini.zulkifli@intel.com>
>>>>
>>>> Add header file to handle API function for device driver to
>>>> communicate with Arm Trusted Firmware.
>>>>
>>>> Signed-off-by: Muhammad Husaini Zulkifli
>>>> <muhammad.husaini.zulkifli@intel.com>
>>>> ---
>>>>  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
>>>>  1 file changed, 46 insertions(+)
>>>>  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
>>>>
>>>> diff --git a/include/linux/firmware/intel/keembay_firmware.h
>>>> b/include/linux/firmware/intel/keembay_firmware.h
>>>> new file mode 100644
>>>> index 000000000000..9adb8c87b788
>>>> --- /dev/null
>>>> +++ b/include/linux/firmware/intel/keembay_firmware.h
>>>> @@ -0,0 +1,46 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + *  Intel Keembay SOC Firmware API Layer
>>>> + *
>>>> + *  Copyright (C) 2020-2021, Intel Corporation
>>>> + *
>>>> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>>>> + */
>>>> +
>>>> +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
>>>> +#define __FIRMWARE_KEEMBAY_SMC_H__
>>>> +
>>>> +#include <linux/arm-smccc.h>
>>>> +
>>>> +/**
>>>
>>> This is not a kernel doc comment. Just use /*
>>>
>>>> + * This file defines API function that can be called by device
>>>> + driver in order to
>>>> + * communicate with Arm Trusted Firmware.
>>>> + */
>>>> +
>>>> +/* Setting for Keem Bay IO Pad Line Voltage Selection */
>>>> +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
>>>
>>> Sudeep: Don't we have any macros for composing these IDs?
>>> nit: IMHO composing these IDs from macros would make more sense to me.
>>>
>>>
>>>> +#define KEEMBAY_SET_1V8_VOLT		0x01
>>>
>>> 0x01 is just 1
>> Noted. Done the changes
>>>
>>>> +#define KEEMBAY_SET_3V3_VOLT		0x00
>>>
>>> 0x00 is just 0
>> Noted. Done the changes
>>>
>>>> +
>>>> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
>>>> +static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1) {
>>>> +	struct arm_smccc_res res;
>>>> +
>>>> +	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
>>>> +
>>>> +	return res.a0;
>>>
>>> I am not big fan of this error propagation in case of failure.
>>>
>>> If smc fails you get via res.a0 SMCCC_RET_NOT_SUPPORTED which is
>>> defined as
>>> -1 which is based on errno-base.h defined as EPERM.
>>>
>>> That driver which Sudeep pointed you to is using EINVAL instead.
>>>
>>> It means I would add a code to check it.
>>
>> Yeah I changed to below line of codes. Is this Ok? Tested working.
>> int keembay_sd_voltage_selection(int volt)
>
>static inline here shouldn't hurt.
due to func() prototype " int keembay_sd_voltage_selection(int volt);" to solve warning issues by robot , I cannot set static inline here. 
Will observed below error:

error: static declaration of ‘keembay_sd_voltage_selection’ follows non-static declaration
 static inline int keembay_sd_voltage_selection(int volt).

>
>> {
>> 	struct arm_smccc_res res;
>>
>>
>	arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAG
>E, volt, &res);
>> 	if ((int)res.a0 < 0)
>> 		return -EINVAL;
>>
>> 	return 0;
>> }
>
>This is fine.
>
>M
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Michal Simek]

Hi,

On 07. 10. 20 15:52, Zulkifli, Muhammad Husaini wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Michal Simek <michal.simek@xilinx.com>
>> Sent: Wednesday, October 7, 2020 9:37 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>> Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian
>> <adrian.hunter@intel.com>; sudeep.holla@arm.com; ulf.hansson@linaro.org;
>> linux-mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org
>> Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>> Wan Mohamad, Wan Ahmad Zainie
>> <wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>> Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted
>> Firmware Service call
>>
>> Hi,
>>
>> On 07. 10. 20 15:21, Zulkifli, Muhammad Husaini wrote:
>>> Hi Michal,
>>>
>>> Thanks for the feedback. I replied inline
>>>
>>>> -----Original Message-----
>>>> From: Michal Simek <michal.simek@xilinx.com>
>>>> Sent: Wednesday, October 7, 2020 4:20 PM
>>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>>>> Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>>>> sudeep.holla@arm.com; ulf.hansson@linaro.org;
>>>> linux-mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>> linux-kernel@vger.kernel.org
>>>> Cc: Raja Subramanian, Lakshmi Bai
>>>> <lakshmi.bai.raja.subramanian@intel.com>;
>>>> Wan Mohamad, Wan Ahmad Zainie
>>>> <wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>>>> Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm
>>>> Trusted Firmware Service call
>>>>
>>>> Hi,
>>>>
>>>> 1. Keem Bay: in subject is wrong. Tools are working with it and you
>>>> should just use keembay: instead.
>>> Are you saying like this ?
>>> Keem Bay: Add support for Arm Trusted Firmware Service call
>>
>> like this:
>> firmware: keembay: Add support for Arm Trusted Firmware Service call
>>
>>>
>>>>
>>>> 2. This should come first before actual change to keep the tree bisectable.
>>> Noted. Done the changes
>>>>
>>>> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>>>>> From: Muhammad Husaini Zulkifli
>>>>> <muhammad.husaini.zulkifli@intel.com>
>>>>>
>>>>> Add header file to handle API function for device driver to
>>>>> communicate with Arm Trusted Firmware.
>>>>>
>>>>> Signed-off-by: Muhammad Husaini Zulkifli
>>>>> <muhammad.husaini.zulkifli@intel.com>
>>>>> ---
>>>>>  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
>>>>>  1 file changed, 46 insertions(+)
>>>>>  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
>>>>>
>>>>> diff --git a/include/linux/firmware/intel/keembay_firmware.h
>>>>> b/include/linux/firmware/intel/keembay_firmware.h
>>>>> new file mode 100644
>>>>> index 000000000000..9adb8c87b788
>>>>> --- /dev/null
>>>>> +++ b/include/linux/firmware/intel/keembay_firmware.h
>>>>> @@ -0,0 +1,46 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>> +/*
>>>>> + *  Intel Keembay SOC Firmware API Layer
>>>>> + *
>>>>> + *  Copyright (C) 2020-2021, Intel Corporation
>>>>> + *
>>>>> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>>>>> + */
>>>>> +
>>>>> +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
>>>>> +#define __FIRMWARE_KEEMBAY_SMC_H__
>>>>> +
>>>>> +#include <linux/arm-smccc.h>
>>>>> +
>>>>> +/**
>>>>
>>>> This is not a kernel doc comment. Just use /*
>>>>
>>>>> + * This file defines API function that can be called by device
>>>>> + driver in order to
>>>>> + * communicate with Arm Trusted Firmware.
>>>>> + */
>>>>> +
>>>>> +/* Setting for Keem Bay IO Pad Line Voltage Selection */
>>>>> +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
>>>>
>>>> Sudeep: Don't we have any macros for composing these IDs?
>>>> nit: IMHO composing these IDs from macros would make more sense to me.
>>>>
>>>>
>>>>> +#define KEEMBAY_SET_1V8_VOLT		0x01
>>>>
>>>> 0x01 is just 1
>>> Noted. Done the changes
>>>>
>>>>> +#define KEEMBAY_SET_3V3_VOLT		0x00
>>>>
>>>> 0x00 is just 0
>>> Noted. Done the changes
>>>>
>>>>> +
>>>>> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
>>>>> +static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1) {
>>>>> +	struct arm_smccc_res res;
>>>>> +
>>>>> +	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
>>>>> +
>>>>> +	return res.a0;
>>>>
>>>> I am not big fan of this error propagation in case of failure.
>>>>
>>>> If smc fails you get via res.a0 SMCCC_RET_NOT_SUPPORTED which is
>>>> defined as
>>>> -1 which is based on errno-base.h defined as EPERM.
>>>>
>>>> That driver which Sudeep pointed you to is using EINVAL instead.
>>>>
>>>> It means I would add a code to check it.
>>>
>>> Yeah I changed to below line of codes. Is this Ok? Tested working.
>>> int keembay_sd_voltage_selection(int volt)
>>
>> static inline here shouldn't hurt.
> due to func() prototype " int keembay_sd_voltage_selection(int volt);" to solve warning issues by robot , I cannot set static inline here. 
> Will observed below error:
> 
> error: static declaration of ‘keembay_sd_voltage_selection’ follows non-static declaration
>  static inline int keembay_sd_voltage_selection(int volt).

Will take a look at when you send new version.

Thanks,
Michal

Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Michal Simek]

Hi,

On 07. 10. 20 15:52, Zulkifli, Muhammad Husaini wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Michal Simek <michal.simek@xilinx.com>
>> Sent: Wednesday, October 7, 2020 9:37 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>> Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian
>> <adrian.hunter@intel.com>; sudeep.holla@arm.com; ulf.hansson@linaro.org;
>> linux-mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org
>> Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>> Wan Mohamad, Wan Ahmad Zainie
>> <wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>> Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted
>> Firmware Service call
>>
>> Hi,
>>
>> On 07. 10. 20 15:21, Zulkifli, Muhammad Husaini wrote:
>>> Hi Michal,
>>>
>>> Thanks for the feedback. I replied inline
>>>
>>>> -----Original Message-----
>>>> From: Michal Simek <michal.simek@xilinx.com>
>>>> Sent: Wednesday, October 7, 2020 4:20 PM
>>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>>>> Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>>>> sudeep.holla@arm.com; ulf.hansson@linaro.org;
>>>> linux-mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>> linux-kernel@vger.kernel.org
>>>> Cc: Raja Subramanian, Lakshmi Bai
>>>> <lakshmi.bai.raja.subramanian@intel.com>;
>>>> Wan Mohamad, Wan Ahmad Zainie
>>>> <wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de
>>>> Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm
>>>> Trusted Firmware Service call
>>>>
>>>> Hi,
>>>>
>>>> 1. Keem Bay: in subject is wrong. Tools are working with it and you
>>>> should just use keembay: instead.
>>> Are you saying like this ?
>>> Keem Bay: Add support for Arm Trusted Firmware Service call
>>
>> like this:
>> firmware: keembay: Add support for Arm Trusted Firmware Service call
>>
>>>
>>>>
>>>> 2. This should come first before actual change to keep the tree bisectable.
>>> Noted. Done the changes
>>>>
>>>> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote:
>>>>> From: Muhammad Husaini Zulkifli
>>>>> <muhammad.husaini.zulkifli@intel.com>
>>>>>
>>>>> Add header file to handle API function for device driver to
>>>>> communicate with Arm Trusted Firmware.
>>>>>
>>>>> Signed-off-by: Muhammad Husaini Zulkifli
>>>>> <muhammad.husaini.zulkifli@intel.com>
>>>>> ---
>>>>>  .../linux/firmware/intel/keembay_firmware.h   | 46 +++++++++++++++++++
>>>>>  1 file changed, 46 insertions(+)
>>>>>  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
>>>>>
>>>>> diff --git a/include/linux/firmware/intel/keembay_firmware.h
>>>>> b/include/linux/firmware/intel/keembay_firmware.h
>>>>> new file mode 100644
>>>>> index 000000000000..9adb8c87b788
>>>>> --- /dev/null
>>>>> +++ b/include/linux/firmware/intel/keembay_firmware.h
>>>>> @@ -0,0 +1,46 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>> +/*
>>>>> + *  Intel Keembay SOC Firmware API Layer
>>>>> + *
>>>>> + *  Copyright (C) 2020-2021, Intel Corporation
>>>>> + *
>>>>> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>>>>> + */
>>>>> +
>>>>> +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
>>>>> +#define __FIRMWARE_KEEMBAY_SMC_H__
>>>>> +
>>>>> +#include <linux/arm-smccc.h>
>>>>> +
>>>>> +/**
>>>>
>>>> This is not a kernel doc comment. Just use /*
>>>>
>>>>> + * This file defines API function that can be called by device
>>>>> + driver in order to
>>>>> + * communicate with Arm Trusted Firmware.
>>>>> + */
>>>>> +
>>>>> +/* Setting for Keem Bay IO Pad Line Voltage Selection */
>>>>> +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID	0x8200ff26
>>>>
>>>> Sudeep: Don't we have any macros for composing these IDs?
>>>> nit: IMHO composing these IDs from macros would make more sense to me.
>>>>
>>>>
>>>>> +#define KEEMBAY_SET_1V8_VOLT		0x01
>>>>
>>>> 0x01 is just 1
>>> Noted. Done the changes
>>>>
>>>>> +#define KEEMBAY_SET_3V3_VOLT		0x00
>>>>
>>>> 0x00 is just 0
>>> Noted. Done the changes
>>>>
>>>>> +
>>>>> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
>>>>> +static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1) {
>>>>> +	struct arm_smccc_res res;
>>>>> +
>>>>> +	arm_smccc_1_1_invoke(func_id, arg0, arg1, &res);
>>>>> +
>>>>> +	return res.a0;
>>>>
>>>> I am not big fan of this error propagation in case of failure.
>>>>
>>>> If smc fails you get via res.a0 SMCCC_RET_NOT_SUPPORTED which is
>>>> defined as
>>>> -1 which is based on errno-base.h defined as EPERM.
>>>>
>>>> That driver which Sudeep pointed you to is using EINVAL instead.
>>>>
>>>> It means I would add a code to check it.
>>>
>>> Yeah I changed to below line of codes. Is this Ok? Tested working.
>>> int keembay_sd_voltage_selection(int volt)
>>
>> static inline here shouldn't hurt.
> due to func() prototype " int keembay_sd_voltage_selection(int volt);" to solve warning issues by robot , I cannot set static inline here. 
> Will observed below error:
> 
> error: static declaration of ‘keembay_sd_voltage_selection’ follows non-static declaration
>  static inline int keembay_sd_voltage_selection(int volt).

Will take a look at when you send new version.

Thanks,
Michal



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel