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

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

[muhammad.husaini.zulkifli]

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

Hi.

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

The second patch is DT binding for arasan controller for uhs support.

The third patch is to introduce the structure device pointer in arasan controller probe.

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

All of these patches was tested with Keem Bay evaluation module board.

Kindly help to review this patch set.

Thank you.

Changes since v3:
- Add Dt bindings for uhs gpio.
- Fixed comment by Michal and Sudeep on header file for the macro and error code.
- Fixed comment by Andy and created 1 new patch to separate the struc dev pointer in probe func.
- Fixed comment by Michal in arasan controller code.

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 (4):
  firmware: keembay: Add support for Arm Trusted Firmware Service call
  dt-bindings: mmc: Add uhs-gpio for Keem Bay UHS-1 Support
  mmc: sdhci-of-arasan: Add structure device pointer in probe
  mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

 .../devicetree/bindings/mmc/arasan,sdhci.yaml |   8 +-
 drivers/mmc/host/sdhci-of-arasan.c            | 127 ++++++++++++++++++
 .../linux/firmware/intel/keembay_firmware.h   |  47 +++++++
 3 files changed, 181 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/firmware/intel/keembay_firmware.h

--
2.17.1

[PATCH v4 1/4] firmware: keembay: Add support for Arm Trusted Firmware Service call

[muhammad.husaini.zulkifli]

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

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

Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
---
 .../linux/firmware/intel/keembay_firmware.h   | 47 +++++++++++++++++++
 1 file changed, 47 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..8a62abcdfead
--- /dev/null
+++ b/include/linux/firmware/intel/keembay_firmware.h
@@ -0,0 +1,47 @@
+/* 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 ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE		\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,		\
+			   ARM_SMCCC_SMC_32,		\
+			   ARM_SMCCC_OWNER_SIP,		\
+			   0xFF26)
+
+#define KEEMBAY_SET_1V8_VOLT	1
+#define KEEMBAY_SET_3V3_VOLT	0
+
+#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
+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_VOLTAGE, volt, &res);
+	if ((int)res.a0 < 0)
+		return -EINVAL;
+
+	return 0;
+}
+#else
+static inline int keembay_sd_voltage_selection(int volt)
+{
+	return -ENODEV;
+}
+#endif
+#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
-- 
2.17.1

Re: [PATCH v4 1/4] firmware: keembay: Add support for Arm Trusted Firmware Service call

[Michal Simek]

On 08. 10. 20 4:09, 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   | 47 +++++++++++++++++++
>  1 file changed, 47 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..8a62abcdfead
> --- /dev/null
> +++ b/include/linux/firmware/intel/keembay_firmware.h
> @@ -0,0 +1,47 @@
> +/* 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 ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE		\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,		\
> +			   ARM_SMCCC_SMC_32,		\
> +			   ARM_SMCCC_OWNER_SIP,		\
> +			   0xFF26)
> +
> +#define KEEMBAY_SET_1V8_VOLT	1
> +#define KEEMBAY_SET_3V3_VOLT	0
> +
> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
> +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_VOLTAGE, volt, &res);
> +	if ((int)res.a0 < 0)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +#else
> +static inline int keembay_sd_voltage_selection(int volt)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
> 

This looks good to me.

Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

Re: [PATCH v4 1/4] firmware: keembay: Add support for Arm Trusted Firmware Service call

[Andy Shevchenko]

On Thu, Oct 8, 2020 at 5:48 AM <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.

Arm -> ARM ?

...

>  .../linux/firmware/intel/keembay_firmware.h   | 47 +++++++++++++++++++

Please, drop dup of 'firmware' in the filename.

...

> +/*
> + *  Intel Keembay SOC Firmware API Layer
> + *

> + *  Copyright (C) 2020-2021, Intel Corporation

Hello from the future?

> + *
> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>

Author: ...

> + */

...

> +/*
> + * This file defines API function that can be called by device driver in order to

an API
by a device

> + * communicate with Arm Trusted Firmware.

Arm -> ARM ?

> + */

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 1/4] firmware: keembay: Add support for Arm Trusted Firmware Service call

[Zulkifli, Muhammad Husaini]

Hi Andy,

Thanks for the feedback.

>-----Original Message-----
>From: Andy Shevchenko <andy.shevchenko@gmail.com>
>Sent: Thursday, October 8, 2020 4:46 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
><michal.simek@xilinx.com>; Shevchenko, Andriy
><andriy.shevchenko@intel.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 v4 1/4] firmware: keembay: Add support for Arm Trusted
>Firmware Service call
>
>On Thu, Oct 8, 2020 at 5:48 AM <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.
>
>Arm -> ARM ?
I will rename based on Sudeep's mentioned in latest thread
>
>...
>
>>  .../linux/firmware/intel/keembay_firmware.h   | 47 +++++++++++++++++++
>
>Please, drop dup of 'firmware' in the filename.
Noted. Will change to keembay.h
>
>...
>
>> +/*
>> + *  Intel Keembay SOC Firmware API Layer
>> + *
>
>> + *  Copyright (C) 2020-2021, Intel Corporation
>
>Hello from the future?
>
Will edit to Copyright (C) 2020, Intel Corporation
>> + *
>> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
>
>Author: ...
>
>> + */
>
>...
>
>> +/*
>> + * This file defines API function that can be called by device driver
>> +in order to
>
>an API
>by a device
Noted 😊
>
>> + * communicate with Arm Trusted Firmware.
>
>Arm -> ARM ?
I will rename based on Sudeep's mentioned in latest thread
>
>> + */
>
>--
>With Best Regards,
>Andy Shevchenko

Re: [PATCH v4 1/4] firmware: keembay: Add support for Arm Trusted Firmware Service call

[Sudeep Holla]

On Thu, Oct 08, 2020 at 10:09:33AM +0800, 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.

[nit] Since it moved to trusted-firmware.org, it is no longer "Arm"
Trusted Firmware. It is now called Trusted Firmware - A profile(TF-A)
or Trusted Firmware - M profile (TF-M). Please update the subject and
the text above. I know it is silly but I am being asked to get this
fixed as it may create "confusion"(I don't know details, please don't
ask 😁)

Apart from various minor things Andy already pointed out, this looks
good. You can add by Ack once the above naming and all things pointed
by Andy are fixed.

--
Regards,
Sudeep

RE: [PATCH v4 1/4] firmware: keembay: Add support for Arm Trusted Firmware Service call

[Zulkifli, Muhammad Husaini]

Hi Sudeep,

>-----Original Message-----
>From: Sudeep Holla <sudeep.holla@arm.com>
>Sent: Thursday, October 8, 2020 5:45 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Cc: Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>Shevchenko, Andriy <andriy.shevchenko@intel.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>; arnd@arndb.de; Wan Mohamad,
>Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
>Subject: Re: [PATCH v4 1/4] firmware: keembay: Add support for Arm Trusted
>Firmware Service call
>
>On Thu, Oct 08, 2020 at 10:09:33AM +0800,
>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.
>
>[nit] Since it moved to trusted-firmware.org, it is no longer "Arm"
>Trusted Firmware. It is now called Trusted Firmware - A profile(TF-A) or
>Trusted Firmware - M profile (TF-M). Please update the subject and the text
>above. I know it is silly but I am being asked to get this fixed as it may create
>"confusion"(I don't know details, please don't ask 😁)
>
>Apart from various minor things Andy already pointed out, this looks good.
>You can add by Ack once the above naming and all things pointed by Andy are
>fixed.

Noted. Thanks Sudeep 😊
>
>--
>Regards,
>Sudeep

[PATCH v4 2/4] dt-bindings: mmc: Add uhs-gpio for Keem Bay UHS-1 Support

[muhammad.husaini.zulkifli]

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

Add DT bindings of uhs-gpio for Keem Bay SOC UHS Mode Support

Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
---
 Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml b/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml
index 58fe9d02a781..320566a673f0 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml
@@ -83,7 +83,7 @@ properties:
       - const: intel,keembay-sdhci-5.1-sd       # Intel Keem Bay SD controller
         description:
           For this device it is strongly suggested to include
-          arasan,soc-ctl-syscon.
+          arasan,soc-ctl-syscon and uhs-gpio.
       - const: intel,keembay-sdhci-5.1-sdio     # Intel Keem Bay SDIO controller
         description:
           For this device it is strongly suggested to include
@@ -152,6 +152,11 @@ properties:
     description:
       The MIO bank number in which the command and data lines are configured.
 
+  uhs-gpio:
+    description:
+      The power mux input will be configure using the GPIO provided
+      to generate either 1.8v or 3.3v output.
+
 dependencies:
   clock-output-names: [ '#clock-cells' ]
   '#clock-cells': [ clock-output-names ]
@@ -300,4 +305,5 @@ examples:
           clocks = <&scmi_clk KEEM_BAY_PSS_AUX_SD0>,
                    <&scmi_clk KEEM_BAY_PSS_SD0>;
           arasan,soc-ctl-syscon = <&sd0_phy_syscon>;
+          uhs-gpio = <&pca0 17 0>;
     };
-- 
2.17.1

Re: [PATCH v4 2/4] dt-bindings: mmc: Add uhs-gpio for Keem Bay UHS-1 Support

[Michal Simek]

On 08. 10. 20 4:09, muhammad.husaini.zulkifli@intel.com wrote:
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> 
> Add DT bindings of uhs-gpio for Keem Bay SOC UHS Mode Support
> 
> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml b/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml
> index 58fe9d02a781..320566a673f0 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml
> @@ -83,7 +83,7 @@ properties:
>        - const: intel,keembay-sdhci-5.1-sd       # Intel Keem Bay SD controller
>          description:
>            For this device it is strongly suggested to include
> -          arasan,soc-ctl-syscon.
> +          arasan,soc-ctl-syscon and uhs-gpio.
>        - const: intel,keembay-sdhci-5.1-sdio     # Intel Keem Bay SDIO controller
>          description:
>            For this device it is strongly suggested to include
> @@ -152,6 +152,11 @@ properties:
>      description:
>        The MIO bank number in which the command and data lines are configured.
>  
> +  uhs-gpio:
> +    description:
> +      The power mux input will be configure using the GPIO provided
> +      to generate either 1.8v or 3.3v output.
> +
>  dependencies:
>    clock-output-names: [ '#clock-cells' ]
>    '#clock-cells': [ clock-output-names ]
> @@ -300,4 +305,5 @@ examples:
>            clocks = <&scmi_clk KEEM_BAY_PSS_AUX_SD0>,
>                     <&scmi_clk KEEM_BAY_PSS_SD0>;
>            arasan,soc-ctl-syscon = <&sd0_phy_syscon>;
> +          uhs-gpio = <&pca0 17 0>;
>      };
> 

Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

[PATCH v4 3/4] mmc: sdhci-of-arasan: Add structure device pointer in probe

[muhammad.husaini.zulkifli]

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

Add struct device *dev in probe func() so that it can widely use in
probe to make code more readable.

Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
---
 drivers/mmc/host/sdhci-of-arasan.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index f186fbd016b1..46aea6516133 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -1521,6 +1521,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);
-- 
2.17.1

Re: [PATCH v4 3/4] mmc: sdhci-of-arasan: Add structure device pointer in probe

[Michal Simek]

On 08. 10. 20 4:09, muhammad.husaini.zulkifli@intel.com wrote:
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> 
> Add struct device *dev in probe func() so that it can widely use in
> probe to make code more readable.
> 
> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> ---
>  drivers/mmc/host/sdhci-of-arasan.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index f186fbd016b1..46aea6516133 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -1521,6 +1521,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);
> 

This is not what we discussed. You create new variable and you should
just use it in that function.

s/pdev->dev\./dev->/g

Thanks,
Michal

RE: [PATCH v4 3/4] mmc: sdhci-of-arasan: Add structure device pointer in probe

[Zulkifli, Muhammad Husaini]

Hi Michal,

Thanks for the comment. I replied inline

>-----Original Message-----
>From: Michal Simek <michal.simek@xilinx.com>
>Sent: Thursday, October 8, 2020 3:35 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>Shevchenko, Andriy <andriy.shevchenko@intel.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 v4 3/4] mmc: sdhci-of-arasan: Add structure device pointer
>in probe
>
>
>
>On 08. 10. 20 4:09, muhammad.husaini.zulkifli@intel.com wrote:
>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>>
>> Add struct device *dev in probe func() so that it can widely use in
>> probe to make code more readable.
>>
>> Signed-off-by: Muhammad Husaini Zulkifli
>> <muhammad.husaini.zulkifli@intel.com>
>> ---
>>  drivers/mmc/host/sdhci-of-arasan.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>> b/drivers/mmc/host/sdhci-of-arasan.c
>> index f186fbd016b1..46aea6516133 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -1521,6 +1521,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);
>>
>
>This is not what we discussed. You create new variable and you should just use it
>in that function.
>
>s/pdev->dev\./dev->/g

For widely used in future, we plan to put it here and not specific to Keembay function only.
Any comment on this @Andy Shevchenko?
Thanks
>
>Thanks,
>Michal

Re: [PATCH v4 3/4] mmc: sdhci-of-arasan: Add structure device pointer in probe

[Andy Shevchenko]

On Thu, Oct 8, 2020 at 1:36 PM Zulkifli, Muhammad Husaini
<muhammad.husaini.zulkifli@intel.com> wrote:
> >From: Michal Simek <michal.simek@xilinx.com>
> >Sent: Thursday, October 8, 2020 3:35 PM
> >On 08. 10. 20 4:09, muhammad.husaini.zulkifli@intel.com wrote:
> >> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

...

> >> @@ -1521,6 +1521,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);
> >>
> >
> >This is not what we discussed. You create new variable and you should just use it
> >in that function.
> >
> >s/pdev->dev\./dev->/g
>
> For widely used in future, we plan to put it here and not specific to Keembay function only.
> Any comment on this @Andy Shevchenko?

I'm not sure what comment from me is needed. I'm on the same page with
Michal, i.e. replace current users of &pdev->dev with a new temporary
variable.

-- 
With Best Regards,
Andy Shevchenko

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

[muhammad.husaini.zulkifli]

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

Voltage switching sequence is needed to support UHS-1 interface.
There are 2 places to control the voltage.
1) By setting the AON register using firmware driver calling
system-level platform management layer (SMC) to set the register.
2) By controlling the GPIO expander value to drive either 1.8V or 3.3V
for power mux input.

Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-of-arasan.c | 126 +++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 46aea6516133..ea2467b0073d 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"
@@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
  * @soc_ctl_base:	Pointer to regmap for syscon for soc_ctl registers.
  * @soc_ctl_map:	Map to get offsets into soc_ctl registers.
  * @quirks:		Arasan deviations from spec.
+ * @uhs_gpio:		Pointer to the uhs gpio.
  */
 struct sdhci_arasan_data {
 	struct sdhci_host *host;
@@ -150,6 +153,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 +365,112 @@ 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, 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 a 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 a 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,
@@ -1601,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

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

[Michal Simek]

On 08. 10. 20 4:09, 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 | 126 +++++++++++++++++++++++++++++
>  1 file changed, 126 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 46aea6516133..ea2467b0073d 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"
> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
>   * @soc_ctl_base:	Pointer to regmap for syscon for soc_ctl registers.
>   * @soc_ctl_map:	Map to get offsets into soc_ctl registers.
>   * @quirks:		Arasan deviations from spec.
> + * @uhs_gpio:		Pointer to the uhs gpio.
>   */
>  struct sdhci_arasan_data {
>  	struct sdhci_host *host;
> @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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 a 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,
> @@ -1601,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);
> 

Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

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

[Ulf Hansson]

On Thu, 8 Oct 2020 at 04:12, <muhammad.husaini.zulkifli@intel.com> wrote:
>
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>
> Voltage switching sequence is needed to support UHS-1 interface.
> 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 | 126 +++++++++++++++++++++++++++++
>  1 file changed, 126 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 46aea6516133..ea2467b0073d 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"
> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
>   * @quirks:            Arasan deviations from spec.
> + * @uhs_gpio:          Pointer to the uhs gpio.
>   */
>  struct sdhci_arasan_data {
>         struct sdhci_host *host;
> @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);

No, sorry, but I don't like this.

This looks like a GPIO regulator with an extension of using the
keembay_sd_voltage_selection() thingy. I think you can model these
things behind a regulator and hook it up as a vqmmc supply in DT
instead. BTW, this is the common way we deal with these things for mmc
host drivers.

> +
> +               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 a 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,
> @@ -1601,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
>

Kind regards
Uffe

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

[Zulkifli, Muhammad Husaini]

Hi,

>-----Original Message-----
>From: Ulf Hansson <ulf.hansson@linaro.org>
>Sent: Thursday, October 8, 2020 5:28 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
><michal.simek@xilinx.com>; Shevchenko, Andriy
><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>On Thu, 8 Oct 2020 at 04:12, <muhammad.husaini.zulkifli@intel.com> wrote:
>>
>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>>
>> Voltage switching sequence is needed to support UHS-1 interface.
>> 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 | 126
>> +++++++++++++++++++++++++++++
>>  1 file changed, 126 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>> b/drivers/mmc/host/sdhci-of-arasan.c
>> index 46aea6516133..ea2467b0073d 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"
>> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
>>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
>>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
>>   * @quirks:            Arasan deviations from spec.
>> + * @uhs_gpio:          Pointer to the uhs gpio.
>>   */
>>  struct sdhci_arasan_data {
>>         struct sdhci_host *host;
>> @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
>
>No, sorry, but I don't like this.
>
>This looks like a GPIO regulator with an extension of using the
>keembay_sd_voltage_selection() thingy. I think you can model these things
>behind a regulator and hook it up as a vqmmc supply in DT instead. BTW, this is
>the common way we deal with these things for mmc host drivers.

The SDcard for Keem Bay SOC does not have its own voltage regulator. 
There are 2 places to control the voltage.
1) By setting the AON register calling system-level platform management layer (SMC)
   to set the I/O pads voltage for particular GPIOs line for clk,data and cmd.
   The reason why I use this keembay_sd_voltage_selection() via smccc interface it because during voltage switching 
   I need to access to AON register. On a secure system, we could not directly access to AON register due to some security concern from driver side, thus
   cannot exposed any register or address.
2) By controlling the GPIO expander value to drive either 1.8V or 3.3V for power mux input.

Thanks
>
>> +
>> +               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 a 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, @@ -1601,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
>>
>
>Kind regards
>Uffe

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

[Ulf Hansson]

On Thu, 8 Oct 2020 at 12:54, Zulkifli, Muhammad Husaini
<muhammad.husaini.zulkifli@intel.com> wrote:
>
> Hi,
>
> >-----Original Message-----
> >From: Ulf Hansson <ulf.hansson@linaro.org>
> >Sent: Thursday, October 8, 2020 5:28 PM
> >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
> ><michal.simek@xilinx.com>; Shevchenko, Andriy
> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
> ><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
> >Keem Bay SOC
> >
> >On Thu, 8 Oct 2020 at 04:12, <muhammad.husaini.zulkifli@intel.com> wrote:
> >>
> >> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> >>
> >> Voltage switching sequence is needed to support UHS-1 interface.
> >> 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 | 126
> >> +++++++++++++++++++++++++++++
> >>  1 file changed, 126 insertions(+)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> >> b/drivers/mmc/host/sdhci-of-arasan.c
> >> index 46aea6516133..ea2467b0073d 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"
> >> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
> >>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
> >>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
> >>   * @quirks:            Arasan deviations from spec.
> >> + * @uhs_gpio:          Pointer to the uhs gpio.
> >>   */
> >>  struct sdhci_arasan_data {
> >>         struct sdhci_host *host;
> >> @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
> >
> >No, sorry, but I don't like this.
> >
> >This looks like a GPIO regulator with an extension of using the
> >keembay_sd_voltage_selection() thingy. I think you can model these things
> >behind a regulator and hook it up as a vqmmc supply in DT instead. BTW, this is
> >the common way we deal with these things for mmc host drivers.
>
> The SDcard for Keem Bay SOC does not have its own voltage regulator.
> There are 2 places to control the voltage.
> 1) By setting the AON register calling system-level platform management layer (SMC)
>    to set the I/O pads voltage for particular GPIOs line for clk,data and cmd.
>    The reason why I use this keembay_sd_voltage_selection() via smccc interface it because during voltage switching
>    I need to access to AON register. On a secure system, we could not directly access to AON register due to some security concern from driver side, thus
>    cannot exposed any register or address.
> 2) By controlling the GPIO expander value to drive either 1.8V or 3.3V for power mux input.

I see, thanks for clarifying.

To me, it sounds like the best fit is to implement a pinctrl (to
manage the I/O pads) and a GPIO regulator.

Kind regards
Uffe

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

[Zulkifli, Muhammad Husaini]

Hi,

>-----Original Message-----
>From: Ulf Hansson <ulf.hansson@linaro.org>
>Sent: Thursday, October 8, 2020 11:19 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
><michal.simek@xilinx.com>; Shevchenko, Andriy
><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>On Thu, 8 Oct 2020 at 12:54, Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@intel.com> wrote:
>>
>> Hi,
>>
>> >-----Original Message-----
>> >From: Ulf Hansson <ulf.hansson@linaro.org>
>> >Sent: Thursday, October 8, 2020 5:28 PM
>> >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
>> ><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>> >support for Keem Bay SOC
>> >
>> >On Thu, 8 Oct 2020 at 04:12, <muhammad.husaini.zulkifli@intel.com>
>wrote:
>> >>
>> >> From: Muhammad Husaini Zulkifli
>> >> <muhammad.husaini.zulkifli@intel.com>
>> >>
>> >> Voltage switching sequence is needed to support UHS-1 interface.
>> >> 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 | 126
>> >> +++++++++++++++++++++++++++++
>> >>  1 file changed, 126 insertions(+)
>> >>
>> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>> >> b/drivers/mmc/host/sdhci-of-arasan.c
>> >> index 46aea6516133..ea2467b0073d 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"
>> >> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
>> >>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
>> >>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
>> >>   * @quirks:            Arasan deviations from spec.
>> >> + * @uhs_gpio:          Pointer to the uhs gpio.
>> >>   */
>> >>  struct sdhci_arasan_data {
>> >>         struct sdhci_host *host;
>> >> @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
>> >
>> >No, sorry, but I don't like this.
>> >
>> >This looks like a GPIO regulator with an extension of using the
>> >keembay_sd_voltage_selection() thingy. I think you can model these
>> >things behind a regulator and hook it up as a vqmmc supply in DT
>> >instead. BTW, this is the common way we deal with these things for mmc
>host drivers.
>>
>> The SDcard for Keem Bay SOC does not have its own voltage regulator.
>> There are 2 places to control the voltage.
>> 1) By setting the AON register calling system-level platform management
>layer (SMC)
>>    to set the I/O pads voltage for particular GPIOs line for clk,data and cmd.
>>    The reason why I use this keembay_sd_voltage_selection() via smccc
>interface it because during voltage switching
>>    I need to access to AON register. On a secure system, we could not
>directly access to AON register due to some security concern from driver side,
>thus
>>    cannot exposed any register or address.
>> 2) By controlling the GPIO expander value to drive either 1.8V or 3.3V for
>power mux input.
>
>I see, thanks for clarifying.
>
>To me, it sounds like the best fit is to implement a pinctrl (to manage the I/O
>pads) and a GPIO regulator.
>
Even with pinctrl, i still need to use the keembay_sd_voltage_selection() thingy for AON register.
Plus, the GPIO pin that control the sd-voltage is in GPIO Expander not using Keembay SOC GPIO Pin.
The best option is using the gpio consumer function to toggle the pin.
Thanks

>Kind regards
>Uffe

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

[Ulf Hansson]

On Thu, 8 Oct 2020 at 19:21, Zulkifli, Muhammad Husaini
<muhammad.husaini.zulkifli@intel.com> wrote:
>
> Hi,
>
> >-----Original Message-----
> >From: Ulf Hansson <ulf.hansson@linaro.org>
> >Sent: Thursday, October 8, 2020 11:19 PM
> >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
> ><michal.simek@xilinx.com>; Shevchenko, Andriy
> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
> ><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
> >Keem Bay SOC
> >
> >On Thu, 8 Oct 2020 at 12:54, Zulkifli, Muhammad Husaini
> ><muhammad.husaini.zulkifli@intel.com> wrote:
> >>
> >> Hi,
> >>
> >> >-----Original Message-----
> >> >From: Ulf Hansson <ulf.hansson@linaro.org>
> >> >Sent: Thursday, October 8, 2020 5:28 PM
> >> >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
> >> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
> >> ><michal.simek@xilinx.com>; Shevchenko, Andriy
> >> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
> >> ><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
> >> >support for Keem Bay SOC
> >> >
> >> >On Thu, 8 Oct 2020 at 04:12, <muhammad.husaini.zulkifli@intel.com>
> >wrote:
> >> >>
> >> >> From: Muhammad Husaini Zulkifli
> >> >> <muhammad.husaini.zulkifli@intel.com>
> >> >>
> >> >> Voltage switching sequence is needed to support UHS-1 interface.
> >> >> 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 | 126
> >> >> +++++++++++++++++++++++++++++
> >> >>  1 file changed, 126 insertions(+)
> >> >>
> >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> >> >> b/drivers/mmc/host/sdhci-of-arasan.c
> >> >> index 46aea6516133..ea2467b0073d 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"
> >> >> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
> >> >>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
> >> >>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
> >> >>   * @quirks:            Arasan deviations from spec.
> >> >> + * @uhs_gpio:          Pointer to the uhs gpio.
> >> >>   */
> >> >>  struct sdhci_arasan_data {
> >> >>         struct sdhci_host *host;
> >> >> @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
> >> >
> >> >No, sorry, but I don't like this.
> >> >
> >> >This looks like a GPIO regulator with an extension of using the
> >> >keembay_sd_voltage_selection() thingy. I think you can model these
> >> >things behind a regulator and hook it up as a vqmmc supply in DT
> >> >instead. BTW, this is the common way we deal with these things for mmc
> >host drivers.
> >>
> >> The SDcard for Keem Bay SOC does not have its own voltage regulator.
> >> There are 2 places to control the voltage.
> >> 1) By setting the AON register calling system-level platform management
> >layer (SMC)
> >>    to set the I/O pads voltage for particular GPIOs line for clk,data and cmd.
> >>    The reason why I use this keembay_sd_voltage_selection() via smccc
> >interface it because during voltage switching
> >>    I need to access to AON register. On a secure system, we could not
> >directly access to AON register due to some security concern from driver side,
> >thus
> >>    cannot exposed any register or address.
> >> 2) By controlling the GPIO expander value to drive either 1.8V or 3.3V for
> >power mux input.
> >
> >I see, thanks for clarifying.
> >
> >To me, it sounds like the best fit is to implement a pinctrl (to manage the I/O
> >pads) and a GPIO regulator.
> >
> Even with pinctrl, i still need to use the keembay_sd_voltage_selection() thingy for AON register.

Yes, I am fine by that.

Although, as it's really a pinctrl, it deserves to be modelled like
that. Not as a soc specific hack in a mmc host driver.

> Plus, the GPIO pin that control the sd-voltage is in GPIO Expander not using Keembay SOC GPIO Pin.
> The best option is using the gpio consumer function to toggle the pin.

As I said, please no.

The common way to model this is as a GPIO regulator. In this way, you
can even rely on existing mmc DT bindings. All you have to do is to
hook up a vqmmc supply to the mmc node.

To be clear, as long as there are no arguments for why a pinctrl and
GPIO regulator can't be used - I am not going to pick up the patches.

Kind regards
Uffe

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

[Michal Simek]

On 09. 10. 20 8:56, Ulf Hansson wrote:
> On Thu, 8 Oct 2020 at 19:21, Zulkifli, Muhammad Husaini
> <muhammad.husaini.zulkifli@intel.com> wrote:
>>
>> Hi,
>>
>>> -----Original Message-----
>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>> Sent: Thursday, October 8, 2020 11:19 PM
>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>>> Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>>> <michal.simek@xilinx.com>; Shevchenko, Andriy
>>> <andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
>>> <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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
>>> Keem Bay SOC
>>>
>>> On Thu, 8 Oct 2020 at 12:54, Zulkifli, Muhammad Husaini
>>> <muhammad.husaini.zulkifli@intel.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>>> -----Original Message-----
>>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>> Sent: Thursday, October 8, 2020 5:28 PM
>>>>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>>>>> Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>>>>> <michal.simek@xilinx.com>; Shevchenko, Andriy
>>>>> <andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
>>>>> <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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>>>>> support for Keem Bay SOC
>>>>>
>>>>> On Thu, 8 Oct 2020 at 04:12, <muhammad.husaini.zulkifli@intel.com>
>>> wrote:
>>>>>>
>>>>>> From: Muhammad Husaini Zulkifli
>>>>>> <muhammad.husaini.zulkifli@intel.com>
>>>>>>
>>>>>> Voltage switching sequence is needed to support UHS-1 interface.
>>>>>> 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 | 126
>>>>>> +++++++++++++++++++++++++++++
>>>>>>  1 file changed, 126 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>>>>>> b/drivers/mmc/host/sdhci-of-arasan.c
>>>>>> index 46aea6516133..ea2467b0073d 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"
>>>>>> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
>>>>>>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
>>>>>>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
>>>>>>   * @quirks:            Arasan deviations from spec.
>>>>>> + * @uhs_gpio:          Pointer to the uhs gpio.
>>>>>>   */
>>>>>>  struct sdhci_arasan_data {
>>>>>>         struct sdhci_host *host;
>>>>>> @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
>>>>>
>>>>> No, sorry, but I don't like this.
>>>>>
>>>>> This looks like a GPIO regulator with an extension of using the
>>>>> keembay_sd_voltage_selection() thingy. I think you can model these
>>>>> things behind a regulator and hook it up as a vqmmc supply in DT
>>>>> instead. BTW, this is the common way we deal with these things for mmc
>>> host drivers.
>>>>
>>>> The SDcard for Keem Bay SOC does not have its own voltage regulator.
>>>> There are 2 places to control the voltage.
>>>> 1) By setting the AON register calling system-level platform management
>>> layer (SMC)
>>>>    to set the I/O pads voltage for particular GPIOs line for clk,data and cmd.
>>>>    The reason why I use this keembay_sd_voltage_selection() via smccc
>>> interface it because during voltage switching
>>>>    I need to access to AON register. On a secure system, we could not
>>> directly access to AON register due to some security concern from driver side,
>>> thus
>>>>    cannot exposed any register or address.
>>>> 2) By controlling the GPIO expander value to drive either 1.8V or 3.3V for
>>> power mux input.
>>>
>>> I see, thanks for clarifying.
>>>
>>> To me, it sounds like the best fit is to implement a pinctrl (to manage the I/O
>>> pads) and a GPIO regulator.
>>>
>> Even with pinctrl, i still need to use the keembay_sd_voltage_selection() thingy for AON register.
> 
> Yes, I am fine by that.
> 
> Although, as it's really a pinctrl, it deserves to be modelled like
> that. Not as a soc specific hack in a mmc host driver.

:-)

> 
>> Plus, the GPIO pin that control the sd-voltage is in GPIO Expander not using Keembay SOC GPIO Pin.
>> The best option is using the gpio consumer function to toggle the pin.
> 
> As I said, please no.
> 
> The common way to model this is as a GPIO regulator. In this way, you
> can even rely on existing mmc DT bindings. All you have to do is to
> hook up a vqmmc supply to the mmc node.

regulators were mentioned here in v1
https://lore.kernel.org/linux-arm-kernel/21d34b75-5947-e115-7c9a-6d068375bbdd@xilinx.com/

> 
> To be clear, as long as there are no arguments for why a pinctrl and
> GPIO regulator can't be used - I am not going to pick up the patches.

I mentioned pinctrl here also in v1 thread.
http://lore.kernel.org/r/6b1c9c71-ca10-47b0-895c-adafc6881063@xilinx.com

Thanks,
Michal

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

[Zulkifli, Muhammad Husaini]

Hi,

>-----Original Message-----
>From: Ulf Hansson <ulf.hansson@linaro.org>
>Sent: Friday, October 9, 2020 2:56 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
><michal.simek@xilinx.com>; Shevchenko, Andriy
><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>On Thu, 8 Oct 2020 at 19:21, Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@intel.com> wrote:
>>
>> Hi,
>>
>> >-----Original Message-----
>> >From: Ulf Hansson <ulf.hansson@linaro.org>
>> >Sent: Thursday, October 8, 2020 11:19 PM
>> >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
>> ><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>> >support for Keem Bay SOC
>> >
>> >On Thu, 8 Oct 2020 at 12:54, Zulkifli, Muhammad Husaini
>> ><muhammad.husaini.zulkifli@intel.com> wrote:
>> >>
>> >> Hi,
>> >>
>> >> >-----Original Message-----
>> >> >From: Ulf Hansson <ulf.hansson@linaro.org>
>> >> >Sent: Thursday, October 8, 2020 5:28 PM
>> >> >To: Zulkifli, Muhammad Husaini
>> >> ><muhammad.husaini.zulkifli@intel.com>
>> >> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>> >> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>> >> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux
>> >> >ARM <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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>> >> >support for Keem Bay SOC
>> >> >
>> >> >On Thu, 8 Oct 2020 at 04:12, <muhammad.husaini.zulkifli@intel.com>
>> >wrote:
>> >> >>
>> >> >> From: Muhammad Husaini Zulkifli
>> >> >> <muhammad.husaini.zulkifli@intel.com>
>> >> >>
>> >> >> Voltage switching sequence is needed to support UHS-1 interface.
>> >> >> 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 | 126
>> >> >> +++++++++++++++++++++++++++++
>> >> >>  1 file changed, 126 insertions(+)
>> >> >>
>> >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>> >> >> b/drivers/mmc/host/sdhci-of-arasan.c
>> >> >> index 46aea6516133..ea2467b0073d 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"
>> >> >> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
>> >> >>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
>> >> >>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
>> >> >>   * @quirks:            Arasan deviations from spec.
>> >> >> + * @uhs_gpio:          Pointer to the uhs gpio.
>> >> >>   */
>> >> >>  struct sdhci_arasan_data {
>> >> >>         struct sdhci_host *host; @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
>> >> >
>> >> >No, sorry, but I don't like this.
>> >> >
>> >> >This looks like a GPIO regulator with an extension of using the
>> >> >keembay_sd_voltage_selection() thingy. I think you can model these
>> >> >things behind a regulator and hook it up as a vqmmc supply in DT
>> >> >instead. BTW, this is the common way we deal with these things for
>> >> >mmc
>> >host drivers.
>> >>
>> >> The SDcard for Keem Bay SOC does not have its own voltage regulator.
>> >> There are 2 places to control the voltage.
>> >> 1) By setting the AON register calling system-level platform
>> >> management
>> >layer (SMC)
>> >>    to set the I/O pads voltage for particular GPIOs line for clk,data and cmd.
>> >>    The reason why I use this keembay_sd_voltage_selection() via
>> >> smccc
>> >interface it because during voltage switching
>> >>    I need to access to AON register. On a secure system, we could
>> >> not
>> >directly access to AON register due to some security concern from
>> >driver side, thus
>> >>    cannot exposed any register or address.
>> >> 2) By controlling the GPIO expander value to drive either 1.8V or
>> >> 3.3V for
>> >power mux input.
>> >
>> >I see, thanks for clarifying.
>> >
>> >To me, it sounds like the best fit is to implement a pinctrl (to
>> >manage the I/O
>> >pads) and a GPIO regulator.
>> >
>> Even with pinctrl, i still need to use the keembay_sd_voltage_selection()
>thingy for AON register.
>
>Yes, I am fine by that.
>
>Although, as it's really a pinctrl, it deserves to be modelled like that. Not as a
>soc specific hack in a mmc host driver.
>
>> Plus, the GPIO pin that control the sd-voltage is in GPIO Expander not using
>Keembay SOC GPIO Pin.
>> The best option is using the gpio consumer function to toggle the pin.
>
>As I said, please no.
>
>The common way to model this is as a GPIO regulator. In this way, you can even
>rely on existing mmc DT bindings. All you have to do is to hook up a vqmmc
>supply to the mmc node.
>
>To be clear, as long as there are no arguments for why a pinctrl and GPIO
>regulator can't be used - I am not going to pick up the patches.
As I mentioned The SDcard does not have its own voltage regulator. 
It only uses the voltage rails on the mux input.

There are 2 things need to be configured before getting the output voltage:

1) V_VDDIO_B :
Supplied voltage applied to I/O Rail which is controlled from the Always on domain using specific bits in AON_CFG1 register. 
This is where we set for V_VDDIO_B using the keembay_sd_voltage_selection() to set either 1.8v or 3.3v depending on the bit value.
IMHO, we do not pinctrl to do this.

2) V_VDDIO_B_MAIN:
The output V_VDDIO_B_MAIN (OUT1) will be either V_3P3_MAIN (IN1) or V_1P8_MAIN (IN2), 
depending on the state of GPIO expander Pin value. There is a POWER MUX involving here.
IMHO, we do not need any gpio regulator/regulator api hook up for this.
Most important thing, there is no regulator ic at all.
We still need to manually control and toggle the pin value.

The final IO voltage is set by V_VDDIO_B (= V_VDDIO_B_MAIN after passing through voltage sense resistor).

Hope this will clarify.

>
>Kind regards
>Uffe

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

[Zulkifli, Muhammad Husaini]

Hi Ulf,

I am following up for below comments and would be grateful for your prompt reply.
Thanks

>-----Original Message-----
>From: Zulkifli, Muhammad Husaini
>Sent: Saturday, October 10, 2020 1:50 AM
>To: Ulf Hansson <ulf.hansson@linaro.org>
>Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
><michal.simek@xilinx.com>; Shevchenko, Andriy
><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>Hi,
>
>>-----Original Message-----
>>From: Ulf Hansson <ulf.hansson@linaro.org>
>>Sent: Friday, October 9, 2020 2:56 PM
>>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>>Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>><michal.simek@xilinx.com>; Shevchenko, Andriy
>><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
>><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support
>>for Keem Bay SOC
>>
>>On Thu, 8 Oct 2020 at 19:21, Zulkifli, Muhammad Husaini
>><muhammad.husaini.zulkifli@intel.com> wrote:
>>>
>>> Hi,
>>>
>>> >-----Original Message-----
>>> >From: Ulf Hansson <ulf.hansson@linaro.org>
>>> >Sent: Thursday, October 8, 2020 11:19 PM
>>> >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>>> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>>> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>>> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
>>> ><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>>> >support for Keem Bay SOC
>>> >
>>> >On Thu, 8 Oct 2020 at 12:54, Zulkifli, Muhammad Husaini
>>> ><muhammad.husaini.zulkifli@intel.com> wrote:
>>> >>
>>> >> Hi,
>>> >>
>>> >> >-----Original Message-----
>>> >> >From: Ulf Hansson <ulf.hansson@linaro.org>
>>> >> >Sent: Thursday, October 8, 2020 5:28 PM
>>> >> >To: Zulkifli, Muhammad Husaini
>>> >> ><muhammad.husaini.zulkifli@intel.com>
>>> >> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>>> >> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>>> >> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux
>>> >> >ARM <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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>>> >> >support for Keem Bay SOC
>>> >> >
>>> >> >On Thu, 8 Oct 2020 at 04:12,
>>> >> ><muhammad.husaini.zulkifli@intel.com>
>>> >wrote:
>>> >> >>
>>> >> >> From: Muhammad Husaini Zulkifli
>>> >> >> <muhammad.husaini.zulkifli@intel.com>
>>> >> >>
>>> >> >> Voltage switching sequence is needed to support UHS-1 interface.
>>> >> >> 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 | 126
>>> >> >> +++++++++++++++++++++++++++++
>>> >> >>  1 file changed, 126 insertions(+)
>>> >> >>
>>> >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>>> >> >> b/drivers/mmc/host/sdhci-of-arasan.c
>>> >> >> index 46aea6516133..ea2467b0073d 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"
>>> >> >> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
>>> >> >>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
>>> >> >>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
>>> >> >>   * @quirks:            Arasan deviations from spec.
>>> >> >> + * @uhs_gpio:          Pointer to the uhs gpio.
>>> >> >>   */
>>> >> >>  struct sdhci_arasan_data {
>>> >> >>         struct sdhci_host *host; @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
>>> >> >
>>> >> >No, sorry, but I don't like this.
>>> >> >
>>> >> >This looks like a GPIO regulator with an extension of using the
>>> >> >keembay_sd_voltage_selection() thingy. I think you can model
>>> >> >these things behind a regulator and hook it up as a vqmmc supply
>>> >> >in DT instead. BTW, this is the common way we deal with these
>>> >> >things for mmc
>>> >host drivers.
>>> >>
>>> >> The SDcard for Keem Bay SOC does not have its own voltage regulator.
>>> >> There are 2 places to control the voltage.
>>> >> 1) By setting the AON register calling system-level platform
>>> >> management
>>> >layer (SMC)
>>> >>    to set the I/O pads voltage for particular GPIOs line for clk,data and cmd.
>>> >>    The reason why I use this keembay_sd_voltage_selection() via
>>> >> smccc
>>> >interface it because during voltage switching
>>> >>    I need to access to AON register. On a secure system, we could
>>> >> not
>>> >directly access to AON register due to some security concern from
>>> >driver side, thus
>>> >>    cannot exposed any register or address.
>>> >> 2) By controlling the GPIO expander value to drive either 1.8V or
>>> >> 3.3V for
>>> >power mux input.
>>> >
>>> >I see, thanks for clarifying.
>>> >
>>> >To me, it sounds like the best fit is to implement a pinctrl (to
>>> >manage the I/O
>>> >pads) and a GPIO regulator.
>>> >
>>> Even with pinctrl, i still need to use the
>>> keembay_sd_voltage_selection()
>>thingy for AON register.
>>
>>Yes, I am fine by that.
>>
>>Although, as it's really a pinctrl, it deserves to be modelled like
>>that. Not as a soc specific hack in a mmc host driver.
>>
>>> Plus, the GPIO pin that control the sd-voltage is in GPIO Expander
>>> not using
>>Keembay SOC GPIO Pin.
>>> The best option is using the gpio consumer function to toggle the pin.
>>
>>As I said, please no.
>>
>>The common way to model this is as a GPIO regulator. In this way, you
>>can even rely on existing mmc DT bindings. All you have to do is to
>>hook up a vqmmc supply to the mmc node.
>>
>>To be clear, as long as there are no arguments for why a pinctrl and
>>GPIO regulator can't be used - I am not going to pick up the patches.
>As I mentioned The SDcard does not have its own voltage regulator.
>It only uses the voltage rails on the mux input.
>
>There are 2 things need to be configured before getting the output voltage:
>
>1) V_VDDIO_B :
>Supplied voltage applied to I/O Rail which is controlled from the Always on
>domain using specific bits in AON_CFG1 register.
>This is where we set for V_VDDIO_B using the keembay_sd_voltage_selection()
>to set either 1.8v or 3.3v depending on the bit value.
>IMHO, we do not pinctrl to do this.
>
>2) V_VDDIO_B_MAIN:
>The output V_VDDIO_B_MAIN (OUT1) will be either V_3P3_MAIN (IN1) or
>V_1P8_MAIN (IN2), depending on the state of GPIO expander Pin value. There is
>a POWER MUX involving here.
>IMHO, we do not need any gpio regulator/regulator api hook up for this.
>Most important thing, there is no regulator ic at all.
>We still need to manually control and toggle the pin value.
>
>The final IO voltage is set by V_VDDIO_B (= V_VDDIO_B_MAIN after passing
>through voltage sense resistor).
>
>Hope this will clarify.
>
>>
>>Kind regards
>>Uffe

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

[Ulf Hansson]

On Fri, 9 Oct 2020 at 19:50, Zulkifli, Muhammad Husaini
<muhammad.husaini.zulkifli@intel.com> wrote:
>
> Hi,
>
> >-----Original Message-----
> >From: Ulf Hansson <ulf.hansson@linaro.org>
> >Sent: Friday, October 9, 2020 2:56 PM
> >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
> ><michal.simek@xilinx.com>; Shevchenko, Andriy
> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
> ><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
> >Keem Bay SOC
> >
> >On Thu, 8 Oct 2020 at 19:21, Zulkifli, Muhammad Husaini
> ><muhammad.husaini.zulkifli@intel.com> wrote:
> >>
> >> Hi,
> >>
> >> >-----Original Message-----
> >> >From: Ulf Hansson <ulf.hansson@linaro.org>
> >> >Sent: Thursday, October 8, 2020 11:19 PM
> >> >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
> >> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
> >> ><michal.simek@xilinx.com>; Shevchenko, Andriy
> >> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
> >> ><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
> >> >support for Keem Bay SOC
> >> >
> >> >On Thu, 8 Oct 2020 at 12:54, Zulkifli, Muhammad Husaini
> >> ><muhammad.husaini.zulkifli@intel.com> wrote:
> >> >>
> >> >> Hi,
> >> >>
> >> >> >-----Original Message-----
> >> >> >From: Ulf Hansson <ulf.hansson@linaro.org>
> >> >> >Sent: Thursday, October 8, 2020 5:28 PM
> >> >> >To: Zulkifli, Muhammad Husaini
> >> >> ><muhammad.husaini.zulkifli@intel.com>
> >> >> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
> >> >> ><michal.simek@xilinx.com>; Shevchenko, Andriy
> >> >> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux
> >> >> >ARM <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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
> >> >> >support for Keem Bay SOC
> >> >> >
> >> >> >On Thu, 8 Oct 2020 at 04:12, <muhammad.husaini.zulkifli@intel.com>
> >> >wrote:
> >> >> >>
> >> >> >> From: Muhammad Husaini Zulkifli
> >> >> >> <muhammad.husaini.zulkifli@intel.com>
> >> >> >>
> >> >> >> Voltage switching sequence is needed to support UHS-1 interface.
> >> >> >> 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 | 126
> >> >> >> +++++++++++++++++++++++++++++
> >> >> >>  1 file changed, 126 insertions(+)
> >> >> >>
> >> >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> >> >> >> b/drivers/mmc/host/sdhci-of-arasan.c
> >> >> >> index 46aea6516133..ea2467b0073d 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"
> >> >> >> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
> >> >> >>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
> >> >> >>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
> >> >> >>   * @quirks:            Arasan deviations from spec.
> >> >> >> + * @uhs_gpio:          Pointer to the uhs gpio.
> >> >> >>   */
> >> >> >>  struct sdhci_arasan_data {
> >> >> >>         struct sdhci_host *host; @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
> >> >> >
> >> >> >No, sorry, but I don't like this.
> >> >> >
> >> >> >This looks like a GPIO regulator with an extension of using the
> >> >> >keembay_sd_voltage_selection() thingy. I think you can model these
> >> >> >things behind a regulator and hook it up as a vqmmc supply in DT
> >> >> >instead. BTW, this is the common way we deal with these things for
> >> >> >mmc
> >> >host drivers.
> >> >>
> >> >> The SDcard for Keem Bay SOC does not have its own voltage regulator.
> >> >> There are 2 places to control the voltage.
> >> >> 1) By setting the AON register calling system-level platform
> >> >> management
> >> >layer (SMC)
> >> >>    to set the I/O pads voltage for particular GPIOs line for clk,data and cmd.
> >> >>    The reason why I use this keembay_sd_voltage_selection() via
> >> >> smccc
> >> >interface it because during voltage switching
> >> >>    I need to access to AON register. On a secure system, we could
> >> >> not
> >> >directly access to AON register due to some security concern from
> >> >driver side, thus
> >> >>    cannot exposed any register or address.
> >> >> 2) By controlling the GPIO expander value to drive either 1.8V or
> >> >> 3.3V for
> >> >power mux input.
> >> >
> >> >I see, thanks for clarifying.
> >> >
> >> >To me, it sounds like the best fit is to implement a pinctrl (to
> >> >manage the I/O
> >> >pads) and a GPIO regulator.
> >> >
> >> Even with pinctrl, i still need to use the keembay_sd_voltage_selection()
> >thingy for AON register.
> >
> >Yes, I am fine by that.
> >
> >Although, as it's really a pinctrl, it deserves to be modelled like that. Not as a
> >soc specific hack in a mmc host driver.
> >
> >> Plus, the GPIO pin that control the sd-voltage is in GPIO Expander not using
> >Keembay SOC GPIO Pin.
> >> The best option is using the gpio consumer function to toggle the pin.
> >
> >As I said, please no.
> >
> >The common way to model this is as a GPIO regulator. In this way, you can even
> >rely on existing mmc DT bindings. All you have to do is to hook up a vqmmc
> >supply to the mmc node.
> >
> >To be clear, as long as there are no arguments for why a pinctrl and GPIO
> >regulator can't be used - I am not going to pick up the patches.
> As I mentioned The SDcard does not have its own voltage regulator.
> It only uses the voltage rails on the mux input.
>
> There are 2 things need to be configured before getting the output voltage:
>
> 1) V_VDDIO_B :
> Supplied voltage applied to I/O Rail which is controlled from the Always on domain using specific bits in AON_CFG1 register.
> This is where we set for V_VDDIO_B using the keembay_sd_voltage_selection() to set either 1.8v or 3.3v depending on the bit value.
> IMHO, we do not pinctrl to do this.
>
> 2) V_VDDIO_B_MAIN:
> The output V_VDDIO_B_MAIN (OUT1) will be either V_3P3_MAIN (IN1) or V_1P8_MAIN (IN2),
> depending on the state of GPIO expander Pin value. There is a POWER MUX involving here.
> IMHO, we do not need any gpio regulator/regulator api hook up for this.
> Most important thing, there is no regulator ic at all.
> We still need to manually control and toggle the pin value.
>
> The final IO voltage is set by V_VDDIO_B (= V_VDDIO_B_MAIN after passing through voltage sense resistor).
>
> Hope this will clarify.

I think I get it, thanks.

Again, I haven't seen any reasons for why this can't be modelled as a
pinctrl and a gpio-regulator. So, please convert it to that.

Kind regards
Uffe

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

[Zulkifli, Muhammad Husaini]

Hi Ulf,

>-----Original Message-----
>From: Ulf Hansson <ulf.hansson@linaro.org>
>Sent: Tuesday, October 13, 2020 4:42 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
><michal.simek@xilinx.com>; Shevchenko, Andriy
><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>On Fri, 9 Oct 2020 at 19:50, Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@intel.com> wrote:
>>
>> Hi,
>>
>> >-----Original Message-----
>> >From: Ulf Hansson <ulf.hansson@linaro.org>
>> >Sent: Friday, October 9, 2020 2:56 PM
>> >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
>> ><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>> >support for Keem Bay SOC
>> >
>> >On Thu, 8 Oct 2020 at 19:21, Zulkifli, Muhammad Husaini
>> ><muhammad.husaini.zulkifli@intel.com> wrote:
>> >>
>> >> Hi,
>> >>
>> >> >-----Original Message-----
>> >> >From: Ulf Hansson <ulf.hansson@linaro.org>
>> >> >Sent: Thursday, October 8, 2020 11:19 PM
>> >> >To: Zulkifli, Muhammad Husaini
>> >> ><muhammad.husaini.zulkifli@intel.com>
>> >> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>> >> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>> >> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux
>> >> >ARM <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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>> >> >support for Keem Bay SOC
>> >> >
>> >> >On Thu, 8 Oct 2020 at 12:54, Zulkifli, Muhammad Husaini
>> >> ><muhammad.husaini.zulkifli@intel.com> wrote:
>> >> >>
>> >> >> Hi,
>> >> >>
>> >> >> >-----Original Message-----
>> >> >> >From: Ulf Hansson <ulf.hansson@linaro.org>
>> >> >> >Sent: Thursday, October 8, 2020 5:28 PM
>> >> >> >To: Zulkifli, Muhammad Husaini
>> >> >> ><muhammad.husaini.zulkifli@intel.com>
>> >> >> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>> >> >> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>> >> >> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux
>> >> >> >ARM <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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>> >> >> >support for Keem Bay SOC
>> >> >> >
>> >> >> >On Thu, 8 Oct 2020 at 04:12,
>> >> >> ><muhammad.husaini.zulkifli@intel.com>
>> >> >wrote:
>> >> >> >>
>> >> >> >> From: Muhammad Husaini Zulkifli
>> >> >> >> <muhammad.husaini.zulkifli@intel.com>
>> >> >> >>
>> >> >> >> Voltage switching sequence is needed to support UHS-1 interface.
>> >> >> >> 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 | 126
>> >> >> >> +++++++++++++++++++++++++++++
>> >> >> >>  1 file changed, 126 insertions(+)
>> >> >> >>
>> >> >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>> >> >> >> b/drivers/mmc/host/sdhci-of-arasan.c
>> >> >> >> index 46aea6516133..ea2467b0073d 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"
>> >> >> >> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
>> >> >> >>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl
>registers.
>> >> >> >>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
>> >> >> >>   * @quirks:            Arasan deviations from spec.
>> >> >> >> + * @uhs_gpio:          Pointer to the uhs gpio.
>> >> >> >>   */
>> >> >> >>  struct sdhci_arasan_data {
>> >> >> >>         struct sdhci_host *host; @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
>> >> >> >
>> >> >> >No, sorry, but I don't like this.
>> >> >> >
>> >> >> >This looks like a GPIO regulator with an extension of using the
>> >> >> >keembay_sd_voltage_selection() thingy. I think you can model
>> >> >> >these things behind a regulator and hook it up as a vqmmc
>> >> >> >supply in DT instead. BTW, this is the common way we deal with
>> >> >> >these things for mmc
>> >> >host drivers.
>> >> >>
>> >> >> The SDcard for Keem Bay SOC does not have its own voltage regulator.
>> >> >> There are 2 places to control the voltage.
>> >> >> 1) By setting the AON register calling system-level platform
>> >> >> management
>> >> >layer (SMC)
>> >> >>    to set the I/O pads voltage for particular GPIOs line for clk,data and
>cmd.
>> >> >>    The reason why I use this keembay_sd_voltage_selection() via
>> >> >> smccc
>> >> >interface it because during voltage switching
>> >> >>    I need to access to AON register. On a secure system, we
>> >> >> could not
>> >> >directly access to AON register due to some security concern from
>> >> >driver side, thus
>> >> >>    cannot exposed any register or address.
>> >> >> 2) By controlling the GPIO expander value to drive either 1.8V
>> >> >> or 3.3V for
>> >> >power mux input.
>> >> >
>> >> >I see, thanks for clarifying.
>> >> >
>> >> >To me, it sounds like the best fit is to implement a pinctrl (to
>> >> >manage the I/O
>> >> >pads) and a GPIO regulator.
>> >> >
>> >> Even with pinctrl, i still need to use the
>> >> keembay_sd_voltage_selection()
>> >thingy for AON register.
>> >
>> >Yes, I am fine by that.
>> >
>> >Although, as it's really a pinctrl, it deserves to be modelled like
>> >that. Not as a soc specific hack in a mmc host driver.
>> >
>> >> Plus, the GPIO pin that control the sd-voltage is in GPIO Expander
>> >> not using
>> >Keembay SOC GPIO Pin.
>> >> The best option is using the gpio consumer function to toggle the pin.
>> >
>> >As I said, please no.
>> >
>> >The common way to model this is as a GPIO regulator. In this way, you
>> >can even rely on existing mmc DT bindings. All you have to do is to
>> >hook up a vqmmc supply to the mmc node.
>> >
>> >To be clear, as long as there are no arguments for why a pinctrl and
>> >GPIO regulator can't be used - I am not going to pick up the patches.
>> As I mentioned The SDcard does not have its own voltage regulator.
>> It only uses the voltage rails on the mux input.
>>
>> There are 2 things need to be configured before getting the output voltage:
>>
>> 1) V_VDDIO_B :
>> Supplied voltage applied to I/O Rail which is controlled from the Always on
>domain using specific bits in AON_CFG1 register.
>> This is where we set for V_VDDIO_B using the
>keembay_sd_voltage_selection() to set either 1.8v or 3.3v depending on the bit
>value.
>> IMHO, we do not pinctrl to do this.
>>
>> 2) V_VDDIO_B_MAIN:
>> The output V_VDDIO_B_MAIN (OUT1) will be either V_3P3_MAIN (IN1) or
>> V_1P8_MAIN (IN2), depending on the state of GPIO expander Pin value. There
>is a POWER MUX involving here.
>> IMHO, we do not need any gpio regulator/regulator api hook up for this.
>> Most important thing, there is no regulator ic at all.
>> We still need to manually control and toggle the pin value.
>>
>> The final IO voltage is set by V_VDDIO_B (= V_VDDIO_B_MAIN after passing
>through voltage sense resistor).
>>
>> Hope this will clarify.
>
>I think I get it, thanks.
>
>Again, I haven't seen any reasons for why this can't be modelled as a pinctrl and
>a gpio-regulator. So, please convert it to that.
For gpio-regulator, I believe I could not use the current gpio-regulator.c framework as there is no consumer API for me to change the state of gpio pin during voltage switching.
Do I need to create a specific gpio-regulator driver under drivers/regulator for keem bay?
>
>Kind regards
>Uffe

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

[Ulf Hansson]

[...]

> >> >> >> The SDcard for Keem Bay SOC does not have its own voltage regulator.
> >> >> >> There are 2 places to control the voltage.
> >> >> >> 1) By setting the AON register calling system-level platform
> >> >> >> management
> >> >> >layer (SMC)
> >> >> >>    to set the I/O pads voltage for particular GPIOs line for clk,data and
> >cmd.
> >> >> >>    The reason why I use this keembay_sd_voltage_selection() via
> >> >> >> smccc
> >> >> >interface it because during voltage switching
> >> >> >>    I need to access to AON register. On a secure system, we
> >> >> >> could not
> >> >> >directly access to AON register due to some security concern from
> >> >> >driver side, thus
> >> >> >>    cannot exposed any register or address.
> >> >> >> 2) By controlling the GPIO expander value to drive either 1.8V
> >> >> >> or 3.3V for
> >> >> >power mux input.
> >> >> >
> >> >> >I see, thanks for clarifying.
> >> >> >
> >> >> >To me, it sounds like the best fit is to implement a pinctrl (to
> >> >> >manage the I/O
> >> >> >pads) and a GPIO regulator.
> >> >> >
> >> >> Even with pinctrl, i still need to use the
> >> >> keembay_sd_voltage_selection()
> >> >thingy for AON register.
> >> >
> >> >Yes, I am fine by that.
> >> >
> >> >Although, as it's really a pinctrl, it deserves to be modelled like
> >> >that. Not as a soc specific hack in a mmc host driver.
> >> >
> >> >> Plus, the GPIO pin that control the sd-voltage is in GPIO Expander
> >> >> not using
> >> >Keembay SOC GPIO Pin.
> >> >> The best option is using the gpio consumer function to toggle the pin.
> >> >
> >> >As I said, please no.
> >> >
> >> >The common way to model this is as a GPIO regulator. In this way, you
> >> >can even rely on existing mmc DT bindings. All you have to do is to
> >> >hook up a vqmmc supply to the mmc node.
> >> >
> >> >To be clear, as long as there are no arguments for why a pinctrl and
> >> >GPIO regulator can't be used - I am not going to pick up the patches.
> >> As I mentioned The SDcard does not have its own voltage regulator.
> >> It only uses the voltage rails on the mux input.
> >>
> >> There are 2 things need to be configured before getting the output voltage:
> >>
> >> 1) V_VDDIO_B :
> >> Supplied voltage applied to I/O Rail which is controlled from the Always on
> >domain using specific bits in AON_CFG1 register.
> >> This is where we set for V_VDDIO_B using the
> >keembay_sd_voltage_selection() to set either 1.8v or 3.3v depending on the bit
> >value.
> >> IMHO, we do not pinctrl to do this.
> >>
> >> 2) V_VDDIO_B_MAIN:
> >> The output V_VDDIO_B_MAIN (OUT1) will be either V_3P3_MAIN (IN1) or
> >> V_1P8_MAIN (IN2), depending on the state of GPIO expander Pin value. There
> >is a POWER MUX involving here.
> >> IMHO, we do not need any gpio regulator/regulator api hook up for this.
> >> Most important thing, there is no regulator ic at all.
> >> We still need to manually control and toggle the pin value.
> >>
> >> The final IO voltage is set by V_VDDIO_B (= V_VDDIO_B_MAIN after passing
> >through voltage sense resistor).
> >>
> >> Hope this will clarify.
> >
> >I think I get it, thanks.
> >
> >Again, I haven't seen any reasons for why this can't be modelled as a pinctrl and
> >a gpio-regulator. So, please convert it to that.
> For gpio-regulator, I believe I could not use the current gpio-regulator.c framework as there is no consumer API for me to change the state of gpio pin during voltage switching.

The consumer API you want to use, is the regulator consumer API,
regulator_enable|disable(), for example.

Although, as I stated earlier, the mmc core already provides helper
functions for this. I suggest you have a look at
mmc_regulator_set_vqmmc() and how it's used by other mmc host drivers.

> Do I need to create a specific gpio-regulator driver under drivers/regulator for keem bay?
> >

I don't think so. Please have a look at
Documentation/devicetree/bindings/regulator/gpio-regulator.yaml. This
allows you to specify your GPIO regulator in DT.

Then from the mmc node you add a "vqmmc-supply" specifier with the
phandle to the regulator - that should be it.

Kind regards
Uffe

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

[Zulkifli, Muhammad Husaini]

Hi,

>-----Original Message-----
>From: Ulf Hansson <ulf.hansson@linaro.org>
>Sent: Tuesday, October 13, 2020 4:42 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
><michal.simek@xilinx.com>; Shevchenko, Andriy
><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>On Fri, 9 Oct 2020 at 19:50, Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@intel.com> wrote:
>>
>> Hi,
>>
>> >-----Original Message-----
>> >From: Ulf Hansson <ulf.hansson@linaro.org>
>> >Sent: Friday, October 9, 2020 2:56 PM
>> >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
>> ><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>> >support for Keem Bay SOC
>> >
>> >On Thu, 8 Oct 2020 at 19:21, Zulkifli, Muhammad Husaini
>> ><muhammad.husaini.zulkifli@intel.com> wrote:
>> >>
>> >> Hi,
>> >>
>> >> >-----Original Message-----
>> >> >From: Ulf Hansson <ulf.hansson@linaro.org>
>> >> >Sent: Thursday, October 8, 2020 11:19 PM
>> >> >To: Zulkifli, Muhammad Husaini
>> >> ><muhammad.husaini.zulkifli@intel.com>
>> >> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>> >> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>> >> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux
>> >> >ARM <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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>> >> >support for Keem Bay SOC
>> >> >
>> >> >On Thu, 8 Oct 2020 at 12:54, Zulkifli, Muhammad Husaini
>> >> ><muhammad.husaini.zulkifli@intel.com> wrote:
>> >> >>
>> >> >> Hi,
>> >> >>
>> >> >> >-----Original Message-----
>> >> >> >From: Ulf Hansson <ulf.hansson@linaro.org>
>> >> >> >Sent: Thursday, October 8, 2020 5:28 PM
>> >> >> >To: Zulkifli, Muhammad Husaini
>> >> >> ><muhammad.husaini.zulkifli@intel.com>
>> >> >> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>> >> >> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>> >> >> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux
>> >> >> >ARM <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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>> >> >> >support for Keem Bay SOC
>> >> >> >
>> >> >> >On Thu, 8 Oct 2020 at 04:12,
>> >> >> ><muhammad.husaini.zulkifli@intel.com>
>> >> >wrote:
>> >> >> >>
>> >> >> >> From: Muhammad Husaini Zulkifli
>> >> >> >> <muhammad.husaini.zulkifli@intel.com>
>> >> >> >>
>> >> >> >> Voltage switching sequence is needed to support UHS-1 interface.
>> >> >> >> 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 | 126
>> >> >> >> +++++++++++++++++++++++++++++
>> >> >> >>  1 file changed, 126 insertions(+)
>> >> >> >>
>> >> >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>> >> >> >> b/drivers/mmc/host/sdhci-of-arasan.c
>> >> >> >> index 46aea6516133..ea2467b0073d 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"
>> >> >> >> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
>> >> >> >>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl
>registers.
>> >> >> >>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
>> >> >> >>   * @quirks:            Arasan deviations from spec.
>> >> >> >> + * @uhs_gpio:          Pointer to the uhs gpio.
>> >> >> >>   */
>> >> >> >>  struct sdhci_arasan_data {
>> >> >> >>         struct sdhci_host *host; @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
>> >> >> >
>> >> >> >No, sorry, but I don't like this.
>> >> >> >
>> >> >> >This looks like a GPIO regulator with an extension of using the
>> >> >> >keembay_sd_voltage_selection() thingy. I think you can model
>> >> >> >these things behind a regulator and hook it up as a vqmmc
>> >> >> >supply in DT instead. BTW, this is the common way we deal with
>> >> >> >these things for mmc
>> >> >host drivers.
>> >> >>
>> >> >> The SDcard for Keem Bay SOC does not have its own voltage
>regulator.
>> >> >> There are 2 places to control the voltage.
>> >> >> 1) By setting the AON register calling system-level platform
>> >> >> management
>> >> >layer (SMC)
>> >> >>    to set the I/O pads voltage for particular GPIOs line for clk,data and
>cmd.
>> >> >>    The reason why I use this keembay_sd_voltage_selection() via
>> >> >> smccc
>> >> >interface it because during voltage switching
>> >> >>    I need to access to AON register. On a secure system, we
>> >> >> could not
>> >> >directly access to AON register due to some security concern from
>> >> >driver side, thus
>> >> >>    cannot exposed any register or address.
>> >> >> 2) By controlling the GPIO expander value to drive either 1.8V
>> >> >> or 3.3V for
>> >> >power mux input.
>> >> >
>> >> >I see, thanks for clarifying.
>> >> >
>> >> >To me, it sounds like the best fit is to implement a pinctrl (to
>> >> >manage the I/O
>> >> >pads) and a GPIO regulator.
>> >> >
>> >> Even with pinctrl, i still need to use the
>> >> keembay_sd_voltage_selection()
>> >thingy for AON register.
>> >
>> >Yes, I am fine by that.
>> >
>> >Although, as it's really a pinctrl, it deserves to be modelled like
>> >that. Not as a soc specific hack in a mmc host driver.
>> >
>> >> Plus, the GPIO pin that control the sd-voltage is in GPIO Expander
>> >> not using
>> >Keembay SOC GPIO Pin.
>> >> The best option is using the gpio consumer function to toggle the pin.
>> >
>> >As I said, please no.
>> >
>> >The common way to model this is as a GPIO regulator. In this way, you
>> >can even rely on existing mmc DT bindings. All you have to do is to
>> >hook up a vqmmc supply to the mmc node.
>> >
>> >To be clear, as long as there are no arguments for why a pinctrl and
>> >GPIO regulator can't be used - I am not going to pick up the patches.
>> As I mentioned The SDcard does not have its own voltage regulator.
>> It only uses the voltage rails on the mux input.
>>
>> There are 2 things need to be configured before getting the output voltage:
>>
>> 1) V_VDDIO_B :
>> Supplied voltage applied to I/O Rail which is controlled from the Always on
>domain using specific bits in AON_CFG1 register.
>> This is where we set for V_VDDIO_B using the
>keembay_sd_voltage_selection() to set either 1.8v or 3.3v depending on the
>bit value.
>> IMHO, we do not pinctrl to do this.
>>
>> 2) V_VDDIO_B_MAIN:
>> The output V_VDDIO_B_MAIN (OUT1) will be either V_3P3_MAIN (IN1) or
>> V_1P8_MAIN (IN2), depending on the state of GPIO expander Pin value.
>There is a POWER MUX involving here.
>> IMHO, we do not need any gpio regulator/regulator api hook up for this.
>> Most important thing, there is no regulator ic at all.
>> We still need to manually control and toggle the pin value.
>>
>> The final IO voltage is set by V_VDDIO_B (= V_VDDIO_B_MAIN after passing
>through voltage sense resistor).
>>
>> Hope this will clarify.
>
>I think I get it, thanks.
>
>Again, I haven't seen any reasons for why this can't be modelled as a pinctrl
>and a gpio-regulator. So, please convert it to that.

I've implemented based on gpio regulator modelling.
but for pinctrl modelling, can we invite any pinctrl maintainer in this discussion whether they are agree with what 
we discuss here to implement the SMCC SIP services in the pinctrl itself.
Maybe they can suggest a way on how to do it? Is it feasible to do that?

>
>Kind regards
>Uffe

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

[Zulkifli, Muhammad Husaini]

Hi All,

I would like to get MMC Maintainer and Pin Control Maintainer attention regarding this patch.
From last discussion, based on Ulf's comment, he would like me to remodel the current way of handling below
line of code to gpio regulator and pinctrl modelling

		/*
		 * Set VDDIO_B voltage to Low for 1.8V
		 * which is controlling by GPIO Expander.
		 */
		gpiod_set_value_cansleep(sdhci_arasan->uhs_gpio, 0);   --->[(1) modelling to gpio regulator]

		/*
		 * This is like a 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); ---> [(2) modelling to pinctrl]


Both (1) and (2) are used to control the supply for the bus IO line power.
Due to hardware limitation of Keem Bay SOC, in order to control the bus IO line either 1.8V or 3.3V, 
there are 2 things need to be done as in (1) and (2) above. 

Below is the summary on how the voltage operation happen for SDcard in Keem Bay.

There are few GPIOS involves for SDHost operation for clk, data, cmd which are GPIO32-GPIO37.
GPIO 32-37 operate at dual voltage operation either 1.8V or 3.3V I/O levels depending on:

i)  The output V_VDDIO_B_MAIN depending on the state of GPIO expander Pin value.[(1)]
ii) VDDIO_B voltage rails output and need to be configured through the Always On register.[(2)]

The final IO voltage is set by item (i) and (ii) after passing through voltage sense resistor.

As for item (i), I have made changes accordingly to model it as regulator.
For item (ii), I would like to get attention of pin control maintainer on how should I modelling to pinctrl model?

I could not use any pinctrl_lookup_state() or pinctrl_select_state() to control the IO pin state as
AON_CFG register is using another register base address which is 0x2026_0000 while GPIO(PINCTRL DRIVER) base address is 0x2032_0000.
There is no way to control AON_CFG register from GPIO_PAD_CFG_CTRL.

I need to configure bit 9 of the AON_CFG register, to select either 1.8v or 3.3v for this voltage rails output.
We cannot exposed any Always On register (AON) address through DT for secure reason.
Current solution is to call SMCCC SIP service to change the bit value.

Does it possible if I expose an API in /include/linux/pinctrl/consumer.h, where mmc driver can call this API to use the SMCCC SIP Services
For example creating pinctrl_request_smccc_call(int func_id, int arg0, int arg1, int arg2);

	in mmc driver use above api to do the operation:

	ret = pinctrl_request_smccc_call(0xFF26, KEEMBAY_SET_1V8_VOLT, 0, 0);

I need Pin Control Maintainer's advices on this pinctrl modelling.
Is there any other way to do this? Does it ok if we implementing this SIP Services in pinctrl subsystem?

Please advices.

Thanks

>-----Original Message-----
>From: Zulkifli, Muhammad Husaini
>Sent: Friday, November 6, 2020 3:16 PM
>To: Ulf Hansson <ulf.hansson@linaro.org>
>Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
><michal.simek@xilinx.com>; Shevchenko, Andriy
><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>Hi,
>
>>-----Original Message-----
>>From: Ulf Hansson <ulf.hansson@linaro.org>
>>Sent: Tuesday, October 13, 2020 4:42 PM
>>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>>Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>><michal.simek@xilinx.com>; Shevchenko, Andriy
>><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
>><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support
>>for Keem Bay SOC
>>
>>On Fri, 9 Oct 2020 at 19:50, Zulkifli, Muhammad Husaini
>><muhammad.husaini.zulkifli@intel.com> wrote:
>>>
>>> Hi,
>>>
>>> >-----Original Message-----
>>> >From: Ulf Hansson <ulf.hansson@linaro.org>
>>> >Sent: Friday, October 9, 2020 2:56 PM
>>> >To: Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@intel.com>
>>> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>>> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>>> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux ARM
>>> ><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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>>> >support for Keem Bay SOC
>>> >
>>> >On Thu, 8 Oct 2020 at 19:21, Zulkifli, Muhammad Husaini
>>> ><muhammad.husaini.zulkifli@intel.com> wrote:
>>> >>
>>> >> Hi,
>>> >>
>>> >> >-----Original Message-----
>>> >> >From: Ulf Hansson <ulf.hansson@linaro.org>
>>> >> >Sent: Thursday, October 8, 2020 11:19 PM
>>> >> >To: Zulkifli, Muhammad Husaini
>>> >> ><muhammad.husaini.zulkifli@intel.com>
>>> >> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>>> >> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>>> >> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org; Linux
>>> >> >ARM <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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>>> >> >support for Keem Bay SOC
>>> >> >
>>> >> >On Thu, 8 Oct 2020 at 12:54, Zulkifli, Muhammad Husaini
>>> >> ><muhammad.husaini.zulkifli@intel.com> wrote:
>>> >> >>
>>> >> >> Hi,
>>> >> >>
>>> >> >> >-----Original Message-----
>>> >> >> >From: Ulf Hansson <ulf.hansson@linaro.org>
>>> >> >> >Sent: Thursday, October 8, 2020 5:28 PM
>>> >> >> >To: Zulkifli, Muhammad Husaini
>>> >> >> ><muhammad.husaini.zulkifli@intel.com>
>>> >> >> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; Michal Simek
>>> >> >> ><michal.simek@xilinx.com>; Shevchenko, Andriy
>>> >> >> ><andriy.shevchenko@intel.com>; linux-mmc@vger.kernel.org;
>>> >> >> >Linux ARM <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 v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1
>>> >> >> >support for Keem Bay SOC
>>> >> >> >
>>> >> >> >On Thu, 8 Oct 2020 at 04:12,
>>> >> >> ><muhammad.husaini.zulkifli@intel.com>
>>> >> >wrote:
>>> >> >> >>
>>> >> >> >> From: Muhammad Husaini Zulkifli
>>> >> >> >> <muhammad.husaini.zulkifli@intel.com>
>>> >> >> >>
>>> >> >> >> Voltage switching sequence is needed to support UHS-1
>interface.
>>> >> >> >> 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 | 126
>>> >> >> >> +++++++++++++++++++++++++++++
>>> >> >> >>  1 file changed, 126 insertions(+)
>>> >> >> >>
>>> >> >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>>> >> >> >> b/drivers/mmc/host/sdhci-of-arasan.c
>>> >> >> >> index 46aea6516133..ea2467b0073d 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"
>>> >> >> >> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
>>> >> >> >>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl
>>registers.
>>> >> >> >>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
>>> >> >> >>   * @quirks:            Arasan deviations from spec.
>>> >> >> >> + * @uhs_gpio:          Pointer to the uhs gpio.
>>> >> >> >>   */
>>> >> >> >>  struct sdhci_arasan_data {
>>> >> >> >>         struct sdhci_host *host; @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
>>> >> >> >
>>> >> >> >No, sorry, but I don't like this.
>>> >> >> >
>>> >> >> >This looks like a GPIO regulator with an extension of using
>>> >> >> >the
>>> >> >> >keembay_sd_voltage_selection() thingy. I think you can model
>>> >> >> >these things behind a regulator and hook it up as a vqmmc
>>> >> >> >supply in DT instead. BTW, this is the common way we deal with
>>> >> >> >these things for mmc
>>> >> >host drivers.
>>> >> >>
>>> >> >> The SDcard for Keem Bay SOC does not have its own voltage
>>regulator.
>>> >> >> There are 2 places to control the voltage.
>>> >> >> 1) By setting the AON register calling system-level platform
>>> >> >> management
>>> >> >layer (SMC)
>>> >> >>    to set the I/O pads voltage for particular GPIOs line for
>>> >> >> clk,data and
>>cmd.
>>> >> >>    The reason why I use this keembay_sd_voltage_selection() via
>>> >> >> smccc
>>> >> >interface it because during voltage switching
>>> >> >>    I need to access to AON register. On a secure system, we
>>> >> >> could not
>>> >> >directly access to AON register due to some security concern from
>>> >> >driver side, thus
>>> >> >>    cannot exposed any register or address.
>>> >> >> 2) By controlling the GPIO expander value to drive either 1.8V
>>> >> >> or 3.3V for
>>> >> >power mux input.
>>> >> >
>>> >> >I see, thanks for clarifying.
>>> >> >
>>> >> >To me, it sounds like the best fit is to implement a pinctrl (to
>>> >> >manage the I/O
>>> >> >pads) and a GPIO regulator.
>>> >> >
>>> >> Even with pinctrl, i still need to use the
>>> >> keembay_sd_voltage_selection()
>>> >thingy for AON register.
>>> >
>>> >Yes, I am fine by that.
>>> >
>>> >Although, as it's really a pinctrl, it deserves to be modelled like
>>> >that. Not as a soc specific hack in a mmc host driver.
>>> >
>>> >> Plus, the GPIO pin that control the sd-voltage is in GPIO Expander
>>> >> not using
>>> >Keembay SOC GPIO Pin.
>>> >> The best option is using the gpio consumer function to toggle the pin.
>>> >
>>> >As I said, please no.
>>> >
>>> >The common way to model this is as a GPIO regulator. In this way,
>>> >you can even rely on existing mmc DT bindings. All you have to do is
>>> >to hook up a vqmmc supply to the mmc node.
>>> >
>>> >To be clear, as long as there are no arguments for why a pinctrl and
>>> >GPIO regulator can't be used - I am not going to pick up the patches.
>>> As I mentioned The SDcard does not have its own voltage regulator.
>>> It only uses the voltage rails on the mux input.
>>>
>>> There are 2 things need to be configured before getting the output
>voltage:
>>>
>>> 1) V_VDDIO_B :
>>> Supplied voltage applied to I/O Rail which is controlled from the
>>> Always on
>>domain using specific bits in AON_CFG1 register.
>>> This is where we set for V_VDDIO_B using the
>>keembay_sd_voltage_selection() to set either 1.8v or 3.3v depending on
>>the bit value.
>>> IMHO, we do not pinctrl to do this.
>>>
>>> 2) V_VDDIO_B_MAIN:
>>> The output V_VDDIO_B_MAIN (OUT1) will be either V_3P3_MAIN (IN1) or
>>> V_1P8_MAIN (IN2), depending on the state of GPIO expander Pin value.
>>There is a POWER MUX involving here.
>>> IMHO, we do not need any gpio regulator/regulator api hook up for this.
>>> Most important thing, there is no regulator ic at all.
>>> We still need to manually control and toggle the pin value.
>>>
>>> The final IO voltage is set by V_VDDIO_B (= V_VDDIO_B_MAIN after
>>> passing
>>through voltage sense resistor).
>>>
>>> Hope this will clarify.
>>
>>I think I get it, thanks.
>>
>>Again, I haven't seen any reasons for why this can't be modelled as a
>>pinctrl and a gpio-regulator. So, please convert it to that.
>
>I've implemented based on gpio regulator modelling.
>but for pinctrl modelling, can we invite any pinctrl maintainer in this
>discussion whether they are agree with what we discuss here to implement
>the SMCC SIP services in the pinctrl itself.
>Maybe they can suggest a way on how to do it? Is it feasible to do that?
>
>>
>>Kind regards
>>Uffe

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

[Adrian Hunter]

On 8/10/20 12:27 pm, Ulf Hansson wrote:
> On Thu, 8 Oct 2020 at 04:12, <muhammad.husaini.zulkifli@intel.com> wrote:
>>
>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>>
>> Voltage switching sequence is needed to support UHS-1 interface.
>> 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 | 126 +++++++++++++++++++++++++++++
>>  1 file changed, 126 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
>> index 46aea6516133..ea2467b0073d 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"
>> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
>>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
>>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
>>   * @quirks:            Arasan deviations from spec.
>> + * @uhs_gpio:          Pointer to the uhs gpio.
>>   */
>>  struct sdhci_arasan_data {
>>         struct sdhci_host *host;
>> @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
> 
> No, sorry, but I don't like this.
> 
> This looks like a GPIO regulator with an extension of using the
> keembay_sd_voltage_selection() thingy. I think you can model these
> things behind a regulator and hook it up as a vqmmc supply in DT
> instead. BTW, this is the common way we deal with these things for mmc
> host drivers.

It seemed to me that would just result in calling regulator API instead of
GPIO API but the flow above would otherwise be unchanged i.e. no benefit

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

[Ulf Hansson]

On Thu, 8 Oct 2020 at 12:58, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 8/10/20 12:27 pm, Ulf Hansson wrote:
> > On Thu, 8 Oct 2020 at 04:12, <muhammad.husaini.zulkifli@intel.com> wrote:
> >>
> >> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> >>
> >> Voltage switching sequence is needed to support UHS-1 interface.
> >> 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 | 126 +++++++++++++++++++++++++++++
> >>  1 file changed, 126 insertions(+)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> >> index 46aea6516133..ea2467b0073d 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"
> >> @@ -136,6 +138,7 @@ struct sdhci_arasan_clk_data {
> >>   * @soc_ctl_base:      Pointer to regmap for syscon for soc_ctl registers.
> >>   * @soc_ctl_map:       Map to get offsets into soc_ctl registers.
> >>   * @quirks:            Arasan deviations from spec.
> >> + * @uhs_gpio:          Pointer to the uhs gpio.
> >>   */
> >>  struct sdhci_arasan_data {
> >>         struct sdhci_host *host;
> >> @@ -150,6 +153,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 +365,112 @@ 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, 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 a 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);
> >
> > No, sorry, but I don't like this.
> >
> > This looks like a GPIO regulator with an extension of using the
> > keembay_sd_voltage_selection() thingy. I think you can model these
> > things behind a regulator and hook it up as a vqmmc supply in DT
> > instead. BTW, this is the common way we deal with these things for mmc
> > host drivers.
>
> It seemed to me that would just result in calling regulator API instead of
> GPIO API but the flow above would otherwise be unchanged i.e. no benefit
>

To me, the benefit is about avoiding platform specific code in drivers
- but also about consistency. For I/O signal voltage, the common
method here, is to model this as a GPIO regulator. This means we can
use these available helpers from the core:

mmc_regulator_set_vqmmc()
mmc_regulator_get_supply()

Kind regards
Uffe