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

Message ID	20201202150205.20150-5-muhammad.husaini.zulkifli@intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=P2QD=FG=lists.infradead.org=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@kernel.org> DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3AB22221FB IronPort-SDR: S1+m9das3MwFIG/jubLZhYtKE59zpfmASSgLaRibzk3L7Uq9lng5XY4ysejIQlaDTSMoOglEeP 018c8ZUIdT6w== IronPort-SDR: 4SC9ziNrhq4S2fUKDLefuSmXn4qw9CUKx/v09rUf5B2bGyYu9xh0sPPlAEei2evNax/8ugNX/h RzEgkmFjkERQ== From: muhammad.husaini.zulkifli@intel.com To: ulf.hansson@linaro.org, adrian.hunter@intel.com, michal.simek@xilinx.com, linux-mmc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: [PATCH v6 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC Date: Wed, 2 Dec 2020 23:02:04 +0800 Message-Id: <20201202150205.20150-5-muhammad.husaini.zulkifli@intel.com> In-Reply-To: <20201202150205.20150-1-muhammad.husaini.zulkifli@intel.com> References: <20201202150205.20150-1-muhammad.husaini.zulkifli@intel.com> Precedence: list Cc: lakshmi.bai.raja.subramanian@intel.com, muhammad.husaini.zulkifli@intel.com, mgross@linux.intel.com, andriy.shevchenko@intel.com, wan.ahmad.zainie.wan.mohamad@intel.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" <linux-arm-kernel-bounces@lists.infradead.org> Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org
Series	mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC \| expand [v6,0/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC [v6,1/4] mmc: sdhci-of-arasan: Add structure device pointer in probe func [v6,2/4] firmware: keembay: Add support for Trusted Firmware Service call [v6,3/4] dt-bindings: mmc: Add phys, vmmc and vqmmc supplies for Keem Bay SOC [v6,4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

Hi Linus, Thanks for your input. I replied inline. >-----Original Message----- >From: Linus Walleij <linus.walleij@linaro.org> >Sent: Thursday, December 3, 2020 2:55 AM >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> >Cc: Ulf Hansson <ulf.hansson@linaro.org>; Hunter, Adrian ><adrian.hunter@intel.com>; Michal Simek <michal.simek@xilinx.com>; linux- >mmc <linux-mmc@vger.kernel.org>; Linux ARM <linux-arm- >kernel@lists.infradead.org>; linux-kernel@vger.kernel.org; Shevchenko, >Andriy <andriy.shevchenko@intel.com>; Raja Subramanian, Lakshmi Bai ><lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad >Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Mark Gross ><mgross@linux.intel.com> >Subject: Re: [PATCH v6 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for >Keem Bay SOC > >Hi Muhammad, > >thanks for your patch! > >On Wed, Dec 2, 2020 at 8:04 AM <muhammad.husaini.zulkifli@intel.com> >wrote: > >> Keem Bay SOC can support dual voltage operations for GPIO SD Pins to >> either 1.8V or 3.3V for bus IO line power. In order to operate the >> GPIOs line for Clk,Cmd and Data on Keem Bay Hardware, it is important >> to configure the supplied voltage applied to their I/O Rail and the >> output of the i2c expander pin. Final Voltage applied on the GPIOs >> Line are dependent by both supplied voltage rail and expander pin >> output as it is been set after passing through the voltage sense resistor. > >I think I understand this part. > >> The Keem Bay HW is somewhat unique in the way of how IO bus line >> voltage are been controlled. Output of the Expander pins is been >> configured using regulator. > >That much is clear. > >> Voltage rail output is being configured using >> keembay_io_rail_supplied_voltage() API in the sdhci driver directly. > >And that is an SMC call like that: > >+static inline int keembay_io_rail_supplied_voltage(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; > >That can set the voltage by calling into the Arm secure world I guess? Yes calling the Arm SMCC . The selection of the supply voltage applied to the rail depends on the AON_CFG1[9] . Setting Bit9 will change the voltage rail supply. Set Bit 9 = 1 // Operation voltage is 1.8V Set Bit 9 = 0 // Operation voltage is 3.3V > >> Pin control based implementation becomes problematic to control the >> voltage rail due to the base address of Always On Register is >> different fromThe driver does not have to be in the the base address >> of GPIO(Pinctrl). Thus, there is no way to control the I/O Rail using GPIO >Pad configuration. > >I don't see why this would be pin control related, and that is as you point out >leading to some confused discussions here. This is not related to a pin control. I pointed out this in the commit description to indicate that for Keem Bay HW to modelling as pinctrl to call the SMCC Arm is not a good approach. > >We do have something like this generic pin config: > > * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different >power > * supplies, the argument to this parameter (on a custom format) tells > * the driver which alternative power source to use. > >But it's ... yeah. It usually has a very specific purpose of selecting one of two >available voltage rails inside the SoC. And it needs to apply to one pin or pin >group. Also it kind of implies that those voltages are always on. > For Keembay HW, we could not apply on any pins because we do not have a direct access to control this pad. That is why, currently I configure this voltage rail supply through the SMCCs Arm wrapper. Unless if we fake some pin example GPIO32-37 for SD, which using "*pinconf_set" pointed to config param of PIN_CONFIG_POWER_SOURCE, then call the keembay_io_rail_supplied_voltage() to change the IO rail voltage feeding. By means calling keembay_io_rail_supplied_voltage(), we call ARM SMCC in this case condition ? I was referring to this https://elixir.bootlin.com/linux/v5.10-rc6/source/drivers/pinctrl/renesas/pinctrl.c#L706 >As you say: > >> From the Databook itself with additional confirmation from Keem Bay HW >> SOC Design Architect, there is no direct control of these AON register >> bits from GPIO pads. > >The keembay_io_rail_supplied_voltage() more resembles a selector (choose >one on a menu) voltage regulator to me if anything. Keem Bay HW SoC does not have its own voltage regulator for sd card. Final IO line voltage depends on supplied voltage applied to their I/O Rail and the output of the i2c expander pin which go into power mux. > >> On the other hand, using ARM SMC (Secure Monitor Call) directly from >> pin control driver for the sake of implement it as pin control model >> is not a good approach. > >Yeah it has to be called from somewhere, if you want an abstraction to make >the driver neutral to any machine, then use a selector regulator. It can be >placed anywhere in the kernel as long as you can reference it. Sorry. I am not really aware of selector regulator. Can you point me to any references for this? > >The register is called (according to the code) AON_CGF1 (really? not >AON_CFG1?) and the "ON" part in "AON" makes it sound like "analog ON" >implying this is something that can be turned on/off and configured into two >voltages and it has been wrapped in these custom SMCCs by a secure world >developer >(right?) It is Always On Power Domain Register. Yes it's been wrapped with SMCCC call due to secure register. By default voltage rail feed is 3.3v. > >If it should use any abstraction it should be a selector regulator IMO and >while that may seem overengineered it adds something because regulators >are used in the MMC subsystem for vdd and vqmmc because we are handling >the OCR mask with that and it can support any amount of present and future >voltages for signal levels with that as well. Any future changes to how the >different signal voltages are set or which voltages exist can then be done in >that regulator driver. This is limitation of Keem Bay HW and I would say Keem Bay HW is somewhat unique in the way of handling the IO bus line voltage. SDcard does not have its own voltage regulator. I created one function sdhci_arasan_keembay_io_line_supply_operation() in sdhci-of-arasan.c to handle the vqmmc(io line supply operation) specific for Keem Bay SoC. For Keem Bay, to actually modelling this as regulator ,for vqmmc, , we need to handle 2 things: 1) Output expander pins : using gpio regulator 2) voltage rail : call keembay_io_rail_supplied_voltage() to handle the SMCC Arm. Other hardware might not need this as they might easily configure the vqmmc hooked up to regulator. IMHO, we do not need to overengineered it to add custom selector regulator just to suit this Keem Bay HW design. > >Just my €0.01... > >Yours, >Linus Walleij

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c index 196e3d65277e..00ecdf54128c 100644 --- a/drivers/mmc/host/sdhci-of-arasan.c +++ b/drivers/mmc/host/sdhci-of-arasan.c @@ -23,6 +23,7 @@ #include <linux/regmap.h> #include <linux/of.h> #include <linux/firmware/xlnx-zynqmp.h> +#include <linux/firmware/intel/keembay.h> #include "cqhci.h" #include "sdhci-pltfm.h" @@ -79,6 +80,8 @@ struct sdhci_arasan_soc_ctl_field { * @baseclkfreq: Where to find corecfg_baseclkfreq * @clockmultiplier: Where to find corecfg_clockmultiplier * @support64b: Where to find SUPPORT64B bit + * @otap_delay: Where to find otap_delay + * @sel_clk_buffer: Where to find clock buffer delay * @hiword_update: If true, use HIWORD_UPDATE to access the syscon * * It's up to the licensee of the Arsan IP block to make these available @@ -89,6 +92,8 @@ struct sdhci_arasan_soc_ctl_map { struct sdhci_arasan_soc_ctl_field baseclkfreq; struct sdhci_arasan_soc_ctl_field clockmultiplier; struct sdhci_arasan_soc_ctl_field support64b; + struct sdhci_arasan_soc_ctl_field otap_delay; + struct sdhci_arasan_soc_ctl_field sel_clk_buffer; bool hiword_update; }; @@ -189,6 +194,8 @@ static const struct sdhci_arasan_soc_ctl_map intel_keembay_soc_ctl_map = { .baseclkfreq = { .reg = 0x0, .width = 8, .shift = 14 }, .clockmultiplier = { .reg = 0x4, .width = 8, .shift = 14 }, .support64b = { .reg = 0x4, .width = 1, .shift = 24 }, + .otap_delay = { .reg = 0x24, .width = 5, .shift = 23 }, + .sel_clk_buffer = { .reg = 0x2c, .width = 3, .shift = 25 }, .hiword_update = false, }; @@ -364,6 +371,144 @@ static int sdhci_arasan_voltage_switch(struct mmc_host *mmc, return -EINVAL; } +/** + * sdhci_arasan_keembay_io_line_supply_operation - Supply for the bus IO line power + * + * @mmc: Pointer to sdhci_host + * @ios: Pointer to IO bus setting + * + * For Keem Bay HW, in order to operate the GPIOs line for Clk,Cmd and Data, + * it is important to configure the supplied voltage applied to their I/O Rail + * and the output of the i2c expander Pin. + * + * Note that to configure the voltage rail setting, specific bits in AON_CFG + * register must be set. While to configure the i2c expander pin output, + * gpio regulator modelling is been used to control the pin state. + * + * Always on Domain register having different base address from GPIO base address + * and it is a secure register. There is no way to control the I/O Voltage Rail + * from the GPIO Pad. SMC CALL is been used to set the bits in AON_CFG1 register. + * + * Final Voltage applied on the GPIOs Line are dependent by both supplied voltage + * I/O Rail and expander pin output as it is been set after passing through the + * voltage sense resistor. + * + * Return: 0 on success and error value on error + */ +static int sdhci_arasan_keembay_io_line_supply_operation(struct mmc_host *mmc, + struct mmc_ios *ios) +{ + int ret, min_uV, max_uV, volt_rail; + + if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) { + min_uV = IOV_1V8; + max_uV = IOV_1V8; + volt_rail = KEEMBAY_SET_1V8_IO_RAIL; + } else { + min_uV = IOV_3V3; + max_uV = IOV_3V3; + volt_rail = KEEMBAY_SET_3V3_IO_RAIL; + } + + /* If no vqmmc supply then we can't change the voltage */ + if (IS_ERR(mmc->supply.vqmmc)) + return -EINVAL; + + ret = regulator_set_voltage(mmc->supply.vqmmc, min_uV, max_uV); + if (ret) + return ret; + + /* + * 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_io_rail_supplied_voltage(volt_rail); + if (ret) + return ret; + + usleep_range(1000, 1100); + + return 0; +} + +static int sdhci_arasan_keembay_voltage_switch(struct mmc_host *mmc, + struct mmc_ios *ios) +{ + struct sdhci_host *host = mmc_priv(mmc); + 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); + + ret = sdhci_arasan_keembay_io_line_supply_operation(mmc, ios); + if (ret) + return ret; + + 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: + ret = sdhci_arasan_keembay_io_line_supply_operation(mmc, ios); + if (ret) + return ret; + + /* 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, @@ -964,6 +1109,77 @@ static void sdhci_arasan_update_baseclkfreq(struct sdhci_host *host) sdhci_arasan_syscon_write(host, &soc_ctl_map->baseclkfreq, mhz); } +/** + * sdhci_arasan_update_otap_delay - Set otap delay + * + * This is used to manualy control the txclk Tap Delay, + * for flopping the final stage flops. + * + * NOTES: + * - Many existing devices don't seem to do this and work fine. To keep + * compatibility for old hardware where the device tree doesn't provide a + * register map, this function is a noop if a soc_ctl_map hasn't been provided + * for this platform. + * + * @host: The sdhci_host + */ +static void sdhci_arasan_update_otap_delay(struct sdhci_host *host, + u32 value) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); + const struct sdhci_arasan_soc_ctl_map *soc_ctl_map = + sdhci_arasan->soc_ctl_map; + + /* Having a map is optional */ + if (!soc_ctl_map) + return; + + /* If we have a map, we expect to have a syscon */ + if (!sdhci_arasan->soc_ctl_base) { + pr_warn("%s: Have regmap, but no soc-ctl-syscon\n", + mmc_hostname(host->mmc)); + return; + } + + sdhci_arasan_syscon_write(host, &soc_ctl_map->otap_delay, value); +} + +/** + * sdhci_arasan_update_clk_sel_buff - Clock buffer select + * + * This is used to delay the clock buffer + * + * NOTES: + * - Many existing devices don't seem to do this and work fine. To keep + * compatibility for old hardware where the device tree doesn't provide a + * register map, this function is a noop if a soc_ctl_map hasn't been provided + * for this platform. + * + * @host: The sdhci_host + */ +static void sdhci_arasan_update_sel_clkbuf(struct sdhci_host *host, + u32 value) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); + const struct sdhci_arasan_soc_ctl_map *soc_ctl_map = + sdhci_arasan->soc_ctl_map; + + /* Having a map is optional */ + if (!soc_ctl_map) + return; + + /* If we have a map, we expect to have a syscon */ + if (!sdhci_arasan->soc_ctl_base) { + pr_warn("%s: Have regmap, but no soc-ctl-syscon\n", + mmc_hostname(host->mmc)); + return; + } + + sdhci_arasan_syscon_write(host, &soc_ctl_map->sel_clk_buffer, value); +} + static void sdhci_arasan_set_clk_delays(struct sdhci_host *host) { struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); @@ -1587,6 +1803,34 @@ 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 device_node *phys; + u32 otap_delay, sel_clk_buffer; + + phys = of_parse_phandle(dev->of_node, "phys", 0); + if (!phys) { + dev_err(dev, "Can't get phys for sd0\n"); + ret = -ENODEV; + goto err_pltfm_free; + } + + of_property_read_u32(phys, + "intel,keembay-emmc-phy-otap-dly", &otap_delay); + of_property_read_u32(phys, + "intel,keembay-emmc-phy-sel-clkbuf", &sel_clk_buffer); + + of_node_put(phys); + + sdhci_arasan_update_otap_delay(host, otap_delay); + sdhci_arasan_update_sel_clkbuf(host, sel_clk_buffer); + + 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, dev);

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

Commit Message

Comments

Patch