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

From: Zulkifli, Muhammad Husaini
Date: Thu Oct 08 2020 - 06:54:57 EST

Next message: Sergey Senozhatsky: "Re: [PATCH] printk: handle blank console arguments passed in."
Previous message: Maxim Levitsky: "Re: [PATCH] KVM: SVM: Use a separate vmcb for the nested L2 guest"
In reply to: Ulf Hansson: "Re: [PATCH v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC"
Next in thread: Ulf Hansson: "Re: [PATCH v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

>-----Original Message-----
>From: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
>Sent: Thursday, October 8, 2020 5:28 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@xxxxxxxxx>
>Cc: Hunter, Adrian <adrian.hunter@xxxxxxxxx>; Michal Simek
><michal.simek@xxxxxxxxxx>; Shevchenko, Andriy
><andriy.shevchenko@xxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx; Linux ARM
><linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux-
>kernel@xxxxxxxxxxxxxxx>; Raja Subramanian, Lakshmi Bai
><lakshmi.bai.raja.subramanian@xxxxxxxxx>; Wan Mohamad, Wan Ahmad
>Zainie <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>; Arnd Bergmann
><arnd@xxxxxxxx>
>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@xxxxxxxxx> wrote:
>>
>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
>> Reviewed-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>> ---
>> 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

Next message: Sergey Senozhatsky: "Re: [PATCH] printk: handle blank console arguments passed in."
Previous message: Maxim Levitsky: "Re: [PATCH] KVM: SVM: Use a separate vmcb for the nested L2 guest"
In reply to: Ulf Hansson: "Re: [PATCH v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC"
Next in thread: Ulf Hansson: "Re: [PATCH v4 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]