* [PATCH v3 0/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
@ 2020-10-06 15:55 ` muhammad.husaini.zulkifli
0 siblings, 0 replies; 41+ messages in thread
From: muhammad.husaini.zulkifli @ 2020-10-06 15:55 UTC (permalink / raw)
To: adrian.hunter, michal.simek, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: lakshmi.bai.raja.subramanian, wan.ahmad.zainie.wan.mohamad,
muhammad.husaini.zulkifli, arnd
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v3 0/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
@ 2020-10-06 15:55 ` muhammad.husaini.zulkifli
0 siblings, 0 replies; 41+ messages in thread
From: muhammad.husaini.zulkifli @ 2020-10-06 15:55 UTC (permalink / raw)
To: adrian.hunter, michal.simek, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: lakshmi.bai.raja.subramanian, muhammad.husaini.zulkifli, arnd,
wan.ahmad.zainie.wan.mohamad
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
2020-10-06 15:55 ` muhammad.husaini.zulkifli
@ 2020-10-06 15:55 ` muhammad.husaini.zulkifli
-1 siblings, 0 replies; 41+ messages in thread
From: muhammad.husaini.zulkifli @ 2020-10-06 15:55 UTC (permalink / raw)
To: adrian.hunter, michal.simek, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: lakshmi.bai.raja.subramanian, wan.ahmad.zainie.wan.mohamad,
muhammad.husaini.zulkifli, arnd
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
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
@ 2020-10-06 15:55 ` muhammad.husaini.zulkifli
0 siblings, 0 replies; 41+ messages in thread
From: muhammad.husaini.zulkifli @ 2020-10-06 15:55 UTC (permalink / raw)
To: adrian.hunter, michal.simek, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: lakshmi.bai.raja.subramanian, muhammad.husaini.zulkifli, arnd,
wan.ahmad.zainie.wan.mohamad
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
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
2020-10-06 15:55 ` muhammad.husaini.zulkifli
@ 2020-10-06 15:55 ` muhammad.husaini.zulkifli
-1 siblings, 0 replies; 41+ messages in thread
From: muhammad.husaini.zulkifli @ 2020-10-06 15:55 UTC (permalink / raw)
To: adrian.hunter, michal.simek, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: lakshmi.bai.raja.subramanian, wan.ahmad.zainie.wan.mohamad,
muhammad.husaini.zulkifli, arnd
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
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
@ 2020-10-06 15:55 ` muhammad.husaini.zulkifli
0 siblings, 0 replies; 41+ messages in thread
From: muhammad.husaini.zulkifli @ 2020-10-06 15:55 UTC (permalink / raw)
To: adrian.hunter, michal.simek, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: lakshmi.bai.raja.subramanian, muhammad.husaini.zulkifli, arnd,
wan.ahmad.zainie.wan.mohamad
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
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
2020-10-06 15:55 ` muhammad.husaini.zulkifli
(?)
@ 2020-10-06 20:37 ` kernel test robot
-1 siblings, 0 replies; 41+ messages in thread
From: kernel test robot @ 2020-10-06 20:37 UTC (permalink / raw)
To: muhammad.husaini.zulkifli, adrian.hunter, michal.simek,
sudeep.holla, ulf.hansson, linux-mmc, linux-arm-kernel,
linux-kernel
Cc: kbuild-all, lakshmi.bai.raja.subramanian,
wan.ahmad.zainie.wan.mohamad, muhammad.husaini.zulkifli
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
@ 2020-10-06 20:37 ` kernel test robot
0 siblings, 0 replies; 41+ messages in thread
From: kernel test robot @ 2020-10-06 20:37 UTC (permalink / raw)
To: muhammad.husaini.zulkifli, adrian.hunter, michal.simek,
sudeep.holla, ulf.hansson, linux-mmc, linux-arm-kernel,
linux-kernel
Cc: lakshmi.bai.raja.subramanian, muhammad.husaini.zulkifli,
kbuild-all, wan.ahmad.zainie.wan.mohamad
[-- 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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
@ 2020-10-06 20:37 ` kernel test robot
0 siblings, 0 replies; 41+ messages in thread
From: kernel test robot @ 2020-10-06 20:37 UTC (permalink / raw)
To: kbuild-all
[-- 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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
2020-10-06 15:55 ` muhammad.husaini.zulkifli
@ 2020-10-07 3:41 ` Zulkifli, Muhammad Husaini
-1 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 3:41 UTC (permalink / raw)
To: Hunter, Adrian, michal.simek, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: Raja Subramanian, Lakshmi Bai, Wan Mohamad, Wan Ahmad Zainie, arnd
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
@ 2020-10-07 3:41 ` Zulkifli, Muhammad Husaini
0 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 3:41 UTC (permalink / raw)
To: Hunter, Adrian, michal.simek, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: Raja Subramanian, Lakshmi Bai, arnd, Wan Mohamad, Wan Ahmad Zainie
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
2020-10-06 15:55 ` muhammad.husaini.zulkifli
@ 2020-10-07 8:20 ` Michal Simek
-1 siblings, 0 replies; 41+ messages in thread
From: Michal Simek @ 2020-10-07 8:20 UTC (permalink / raw)
To: muhammad.husaini.zulkifli, adrian.hunter, michal.simek,
sudeep.holla, ulf.hansson, linux-mmc, linux-arm-kernel,
linux-kernel
Cc: lakshmi.bai.raja.subramanian, wan.ahmad.zainie.wan.mohamad, arnd
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
@ 2020-10-07 8:20 ` Michal Simek
0 siblings, 0 replies; 41+ messages in thread
From: Michal Simek @ 2020-10-07 8:20 UTC (permalink / raw)
To: muhammad.husaini.zulkifli, adrian.hunter, michal.simek,
sudeep.holla, ulf.hansson, linux-mmc, linux-arm-kernel,
linux-kernel
Cc: lakshmi.bai.raja.subramanian, arnd, wan.ahmad.zainie.wan.mohamad
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
2020-10-06 15:55 ` muhammad.husaini.zulkifli
@ 2020-10-07 8:33 ` Michal Simek
-1 siblings, 0 replies; 41+ messages in thread
From: Michal Simek @ 2020-10-07 8:33 UTC (permalink / raw)
To: muhammad.husaini.zulkifli, adrian.hunter, michal.simek,
sudeep.holla, ulf.hansson, linux-mmc, linux-arm-kernel,
linux-kernel
Cc: lakshmi.bai.raja.subramanian, wan.ahmad.zainie.wan.mohamad, arnd
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
@ 2020-10-07 8:33 ` Michal Simek
0 siblings, 0 replies; 41+ messages in thread
From: Michal Simek @ 2020-10-07 8:33 UTC (permalink / raw)
To: muhammad.husaini.zulkifli, adrian.hunter, michal.simek,
sudeep.holla, ulf.hansson, linux-mmc, linux-arm-kernel,
linux-kernel
Cc: lakshmi.bai.raja.subramanian, arnd, wan.ahmad.zainie.wan.mohamad
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
2020-10-07 8:33 ` Michal Simek
@ 2020-10-07 8:55 ` Andy Shevchenko
-1 siblings, 0 replies; 41+ messages in thread
From: Andy Shevchenko @ 2020-10-07 8:55 UTC (permalink / raw)
To: Michal Simek
Cc: muhammad.husaini.zulkifli, Adrian Hunter, Sudeep Holla,
Ulf Hansson, linux-mmc, linux-arm Mailing List,
Linux Kernel Mailing List, lakshmi.bai.raja.subramanian,
Wan Ahmad Zainie, Arnd Bergmann
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
@ 2020-10-07 8:55 ` Andy Shevchenko
0 siblings, 0 replies; 41+ messages in thread
From: Andy Shevchenko @ 2020-10-07 8:55 UTC (permalink / raw)
To: Michal Simek
Cc: Ulf Hansson, Arnd Bergmann, lakshmi.bai.raja.subramanian,
linux-mmc, Adrian Hunter, Linux Kernel Mailing List,
Wan Ahmad Zainie, Sudeep Holla, muhammad.husaini.zulkifli,
linux-arm Mailing List
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
2020-10-07 8:55 ` Andy Shevchenko
@ 2020-10-07 9:10 ` Michal Simek
-1 siblings, 0 replies; 41+ messages in thread
From: Michal Simek @ 2020-10-07 9:10 UTC (permalink / raw)
To: Andy Shevchenko, Michal Simek
Cc: muhammad.husaini.zulkifli, Adrian Hunter, Sudeep Holla,
Ulf Hansson, linux-mmc, linux-arm Mailing List,
Linux Kernel Mailing List, lakshmi.bai.raja.subramanian,
Wan Ahmad Zainie, Arnd Bergmann
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
@ 2020-10-07 9:10 ` Michal Simek
0 siblings, 0 replies; 41+ messages in thread
From: Michal Simek @ 2020-10-07 9:10 UTC (permalink / raw)
To: Andy Shevchenko, Michal Simek
Cc: Ulf Hansson, Arnd Bergmann, lakshmi.bai.raja.subramanian,
linux-mmc, Adrian Hunter, Linux Kernel Mailing List,
Wan Ahmad Zainie, Sudeep Holla, muhammad.husaini.zulkifli,
linux-arm Mailing List
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
2020-10-07 8:20 ` Michal Simek
@ 2020-10-07 10:10 ` Sudeep Holla
-1 siblings, 0 replies; 41+ messages in thread
From: Sudeep Holla @ 2020-10-07 10:10 UTC (permalink / raw)
To: Michal Simek
Cc: muhammad.husaini.zulkifli, adrian.hunter, Sudeep Holla,
ulf.hansson, linux-mmc, linux-arm-kernel, linux-kernel,
lakshmi.bai.raja.subramanian, wan.ahmad.zainie.wan.mohamad, arnd
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
@ 2020-10-07 10:10 ` Sudeep Holla
0 siblings, 0 replies; 41+ messages in thread
From: Sudeep Holla @ 2020-10-07 10:10 UTC (permalink / raw)
To: Michal Simek
Cc: ulf.hansson, arnd, lakshmi.bai.raja.subramanian, linux-mmc,
adrian.hunter, linux-kernel, wan.ahmad.zainie.wan.mohamad,
Sudeep Holla, muhammad.husaini.zulkifli, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
2020-10-07 9:10 ` Michal Simek
@ 2020-10-07 12:08 ` Andy Shevchenko
-1 siblings, 0 replies; 41+ messages in thread
From: Andy Shevchenko @ 2020-10-07 12:08 UTC (permalink / raw)
To: Michal Simek
Cc: muhammad.husaini.zulkifli, Adrian Hunter, Sudeep Holla,
Ulf Hansson, linux-mmc, linux-arm Mailing List,
Linux Kernel Mailing List, lakshmi.bai.raja.subramanian,
Wan Ahmad Zainie, Arnd Bergmann
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
@ 2020-10-07 12:08 ` Andy Shevchenko
0 siblings, 0 replies; 41+ messages in thread
From: Andy Shevchenko @ 2020-10-07 12:08 UTC (permalink / raw)
To: Michal Simek
Cc: Ulf Hansson, Arnd Bergmann, lakshmi.bai.raja.subramanian,
linux-mmc, Adrian Hunter, Linux Kernel Mailing List,
Wan Ahmad Zainie, Sudeep Holla, muhammad.husaini.zulkifli,
linux-arm Mailing List
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
2020-10-07 8:20 ` Michal Simek
@ 2020-10-07 13:21 ` Zulkifli, Muhammad Husaini
-1 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 13:21 UTC (permalink / raw)
To: Michal Simek, Hunter, Adrian, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: Raja Subramanian, Lakshmi Bai, Wan Mohamad, Wan Ahmad Zainie, arnd
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
@ 2020-10-07 13:21 ` Zulkifli, Muhammad Husaini
0 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 13:21 UTC (permalink / raw)
To: Michal Simek, Hunter, Adrian, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: Raja Subramanian, Lakshmi Bai, arnd, Wan Mohamad, Wan Ahmad Zainie
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
2020-10-07 8:33 ` Michal Simek
@ 2020-10-07 13:25 ` Zulkifli, Muhammad Husaini
-1 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 13:25 UTC (permalink / raw)
To: Michal Simek, Hunter, Adrian, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: Raja Subramanian, Lakshmi Bai, Wan Mohamad, Wan Ahmad Zainie, arnd
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
@ 2020-10-07 13:25 ` Zulkifli, Muhammad Husaini
0 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 13:25 UTC (permalink / raw)
To: Michal Simek, Hunter, Adrian, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: Raja Subramanian, Lakshmi Bai, arnd, Wan Mohamad, Wan Ahmad Zainie
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
2020-10-07 8:55 ` Andy Shevchenko
@ 2020-10-07 13:28 ` Zulkifli, Muhammad Husaini
-1 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 13:28 UTC (permalink / raw)
To: Andy Shevchenko, Michal Simek
Cc: Hunter, Adrian, Sudeep Holla, Ulf Hansson, linux-mmc,
linux-arm Mailing List, Linux Kernel Mailing List,
Raja Subramanian, Lakshmi Bai, Wan Mohamad, Wan Ahmad Zainie,
Arnd Bergmann
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
@ 2020-10-07 13:28 ` Zulkifli, Muhammad Husaini
0 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 13:28 UTC (permalink / raw)
To: Andy Shevchenko, Michal Simek
Cc: Ulf Hansson, Arnd Bergmann, Raja Subramanian, Lakshmi Bai,
linux-mmc, Linux Kernel Mailing List, Hunter, Adrian,
Sudeep Holla, Wan Mohamad, Wan Ahmad Zainie,
linux-arm Mailing List
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
2020-10-07 10:10 ` Sudeep Holla
@ 2020-10-07 13:29 ` Zulkifli, Muhammad Husaini
-1 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 13:29 UTC (permalink / raw)
To: Sudeep Holla, Michal Simek
Cc: Hunter, Adrian, ulf.hansson, linux-mmc, linux-arm-kernel,
linux-kernel, Raja Subramanian, Lakshmi Bai, Wan Mohamad,
Wan Ahmad Zainie, arnd
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
@ 2020-10-07 13:29 ` Zulkifli, Muhammad Husaini
0 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 13:29 UTC (permalink / raw)
To: Sudeep Holla, Michal Simek
Cc: ulf.hansson, arnd, Raja Subramanian, Lakshmi Bai, linux-mmc,
Hunter, Adrian, linux-kernel, Wan Mohamad, Wan Ahmad Zainie,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
2020-10-07 13:21 ` Zulkifli, Muhammad Husaini
@ 2020-10-07 13:37 ` Michal Simek
-1 siblings, 0 replies; 41+ messages in thread
From: Michal Simek @ 2020-10-07 13:37 UTC (permalink / raw)
To: Zulkifli, Muhammad Husaini, Michal Simek, Hunter, Adrian,
sudeep.holla, ulf.hansson, linux-mmc, linux-arm-kernel,
linux-kernel
Cc: Raja Subramanian, Lakshmi Bai, Wan Mohamad, Wan Ahmad Zainie, arnd
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
@ 2020-10-07 13:37 ` Michal Simek
0 siblings, 0 replies; 41+ messages in thread
From: Michal Simek @ 2020-10-07 13:37 UTC (permalink / raw)
To: Zulkifli, Muhammad Husaini, Michal Simek, Hunter, Adrian,
sudeep.holla, ulf.hansson, linux-mmc, linux-arm-kernel,
linux-kernel
Cc: Raja Subramanian, Lakshmi Bai, arnd, Wan Mohamad, Wan Ahmad Zainie
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
2020-10-07 13:37 ` Michal Simek
@ 2020-10-07 13:52 ` Zulkifli, Muhammad Husaini
-1 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 13:52 UTC (permalink / raw)
To: Michal Simek, Hunter, Adrian, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: Raja Subramanian, Lakshmi Bai, Wan Mohamad, Wan Ahmad Zainie, arnd
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
@ 2020-10-07 13:52 ` Zulkifli, Muhammad Husaini
0 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 13:52 UTC (permalink / raw)
To: Michal Simek, Hunter, Adrian, sudeep.holla, ulf.hansson,
linux-mmc, linux-arm-kernel, linux-kernel
Cc: Raja Subramanian, Lakshmi Bai, arnd, Wan Mohamad, Wan Ahmad Zainie
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
2020-10-07 13:52 ` Zulkifli, Muhammad Husaini
@ 2020-10-07 13:58 ` Michal Simek
-1 siblings, 0 replies; 41+ messages in thread
From: Michal Simek @ 2020-10-07 13:58 UTC (permalink / raw)
To: Zulkifli, Muhammad Husaini, Michal Simek, Hunter, Adrian,
sudeep.holla, ulf.hansson, linux-mmc, linux-arm-kernel,
linux-kernel
Cc: Raja Subramanian, Lakshmi Bai, Wan Mohamad, Wan Ahmad Zainie, arnd
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
@ 2020-10-07 13:58 ` Michal Simek
0 siblings, 0 replies; 41+ messages in thread
From: Michal Simek @ 2020-10-07 13:58 UTC (permalink / raw)
To: Zulkifli, Muhammad Husaini, Michal Simek, Hunter, Adrian,
sudeep.holla, ulf.hansson, linux-mmc, linux-arm-kernel,
linux-kernel
Cc: Raja Subramanian, Lakshmi Bai, arnd, Wan Mohamad, Wan Ahmad Zainie
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
2020-10-07 13:28 ` Zulkifli, Muhammad Husaini
@ 2020-10-07 14:54 ` Andy Shevchenko
-1 siblings, 0 replies; 41+ messages in thread
From: Andy Shevchenko @ 2020-10-07 14:54 UTC (permalink / raw)
To: Zulkifli, Muhammad Husaini
Cc: Michal Simek, Hunter, Adrian, Sudeep Holla, Ulf Hansson,
linux-mmc, linux-arm Mailing List, Linux Kernel Mailing List,
Raja Subramanian, Lakshmi Bai, Wan Mohamad, Wan Ahmad Zainie,
Arnd Bergmann
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
@ 2020-10-07 14:54 ` Andy Shevchenko
0 siblings, 0 replies; 41+ messages in thread
From: Andy Shevchenko @ 2020-10-07 14:54 UTC (permalink / raw)
To: Zulkifli, Muhammad Husaini
Cc: Ulf Hansson, Arnd Bergmann, Linux Kernel Mailing List, linux-mmc,
Michal Simek, Raja Subramanian, Lakshmi Bai, Hunter, Adrian,
Sudeep Holla, Wan Mohamad, Wan Ahmad Zainie,
linux-arm Mailing List
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
2020-10-07 14:54 ` Andy Shevchenko
@ 2020-10-07 16:08 ` Zulkifli, Muhammad Husaini
-1 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 16:08 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Michal Simek, Hunter, Adrian, Sudeep Holla, Ulf Hansson,
linux-mmc, linux-arm Mailing List, Linux Kernel Mailing List,
Raja Subramanian, Lakshmi Bai, Wan Mohamad, Wan Ahmad Zainie,
Arnd Bergmann
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
@ 2020-10-07 16:08 ` Zulkifli, Muhammad Husaini
0 siblings, 0 replies; 41+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2020-10-07 16:08 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Ulf Hansson, Arnd Bergmann, Linux Kernel Mailing List, linux-mmc,
Michal Simek, Raja Subramanian, Lakshmi Bai, Hunter, Adrian,
Sudeep Holla, Wan Mohamad, Wan Ahmad Zainie,
linux-arm Mailing List
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
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2020-10-07 16:10 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-06 15:55 [PATCH v3 0/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC muhammad.husaini.zulkifli
2020-10-06 15:55 ` muhammad.husaini.zulkifli
2020-10-06 15:55 ` [PATCH v3 1/2] " muhammad.husaini.zulkifli
2020-10-06 15:55 ` muhammad.husaini.zulkifli
2020-10-07 8:33 ` Michal Simek
2020-10-07 8:33 ` Michal Simek
2020-10-07 8:55 ` Andy Shevchenko
2020-10-07 8:55 ` Andy Shevchenko
2020-10-07 9:10 ` Michal Simek
2020-10-07 9:10 ` Michal Simek
2020-10-07 12:08 ` Andy Shevchenko
2020-10-07 12:08 ` Andy Shevchenko
2020-10-07 13:28 ` Zulkifli, Muhammad Husaini
2020-10-07 13:28 ` Zulkifli, Muhammad Husaini
2020-10-07 14:54 ` Andy Shevchenko
2020-10-07 14:54 ` Andy Shevchenko
2020-10-07 16:08 ` Zulkifli, Muhammad Husaini
2020-10-07 16:08 ` Zulkifli, Muhammad Husaini
2020-10-07 13:25 ` Zulkifli, Muhammad Husaini
2020-10-07 13:25 ` Zulkifli, Muhammad Husaini
2020-10-06 15:55 ` [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call muhammad.husaini.zulkifli
2020-10-06 15:55 ` muhammad.husaini.zulkifli
2020-10-06 20:37 ` kernel test robot
2020-10-06 20:37 ` kernel test robot
2020-10-06 20:37 ` kernel test robot
2020-10-07 3:41 ` Zulkifli, Muhammad Husaini
2020-10-07 3:41 ` Zulkifli, Muhammad Husaini
2020-10-07 8:20 ` Michal Simek
2020-10-07 8:20 ` Michal Simek
2020-10-07 10:10 ` Sudeep Holla
2020-10-07 10:10 ` Sudeep Holla
2020-10-07 13:29 ` Zulkifli, Muhammad Husaini
2020-10-07 13:29 ` Zulkifli, Muhammad Husaini
2020-10-07 13:21 ` Zulkifli, Muhammad Husaini
2020-10-07 13:21 ` Zulkifli, Muhammad Husaini
2020-10-07 13:37 ` Michal Simek
2020-10-07 13:37 ` Michal Simek
2020-10-07 13:52 ` Zulkifli, Muhammad Husaini
2020-10-07 13:52 ` Zulkifli, Muhammad Husaini
2020-10-07 13:58 ` Michal Simek
2020-10-07 13:58 ` Michal Simek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.