| Message ID | 20201006155549.3595-2-muhammad.husaini.zulkifli@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC | expand |
On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote: > From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > > Voltage switching sequence is needed to support UHS-1 interface. > There are 2 places to control the voltage. > 1) By setting the AON register using firmware driver calling > system-level platform management layer (SMC) to set the register. > 2) By controlling the GPIO expander value to drive either 1.8V or 3.3V > for power mux input. > > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> > Reviewed-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-of-arasan.c | 127 +++++++++++++++++++++++++++++ > 1 file changed, 127 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > index f186fbd016b1..e681e6f860ba 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -16,6 +16,7 @@ > */ > > #include <linux/clk-provider.h> > +#include <linux/gpio/consumer.h> > #include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/of_device.h> > @@ -23,6 +24,7 @@ > #include <linux/regmap.h> > #include <linux/of.h> > #include <linux/firmware/xlnx-zynqmp.h> > +#include <linux/firmware/intel/keembay_firmware.h> > > #include "cqhci.h" > #include "sdhci-pltfm.h" > @@ -150,6 +152,7 @@ struct sdhci_arasan_data { > struct regmap *soc_ctl_base; > const struct sdhci_arasan_soc_ctl_map *soc_ctl_map; > unsigned int quirks; > + struct gpio_desc *uhs_gpio; > > /* Controller does not have CD wired and will not function normally without */ > #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST BIT(0) > @@ -361,6 +364,113 @@ static int sdhci_arasan_voltage_switch(struct mmc_host *mmc, > return -EINVAL; > } > > +static int sdhci_arasan_keembay_voltage_switch(struct mmc_host *mmc, > + struct mmc_ios *ios) > +{ > + struct sdhci_host *host = mmc_priv(mmc); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); > + u16 ctrl_2; > + u16 clk; nit: put it to one line. > + int ret; > + > + switch (ios->signal_voltage) { > + case MMC_SIGNAL_VOLTAGE_180: > + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); nit: double space > + clk &= ~SDHCI_CLOCK_CARD_EN; > + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > + > + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); nit: double space again. > + if (clk & SDHCI_CLOCK_CARD_EN) > + return -EAGAIN; > + > + sdhci_writeb(host, SDHCI_POWER_ON | SDHCI_POWER_180, > + SDHCI_POWER_CONTROL); > + > + /* > + * Set VDDIO_B voltage to Low for 1.8V > + * which is controlling by GPIO Expander. > + */ > + gpiod_set_value_cansleep(sdhci_arasan->uhs_gpio, 0); > + > + /* > + * This is like final gatekeeper. Need to ensure changed voltage > + * is settled before and after turn on this bit. > + */ > + usleep_range(1000, 1100); > + > + ret = keembay_sd_voltage_selection(KEEMBAY_SET_1V8_VOLT); > + if (ret) > + return ret; > + > + usleep_range(1000, 1100); > + > + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); > + ctrl_2 |= SDHCI_CTRL_VDD_180; > + sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2); > + > + /* Sleep for 5ms to stabilize 1.8V regulator */ > + usleep_range(5000, 5500); > + > + /* 1.8V regulator output should be stable within 5 ms */ > + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); > + if (!(ctrl_2 & SDHCI_CTRL_VDD_180)) > + return -EAGAIN; > + > + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > + clk |= SDHCI_CLOCK_CARD_EN; > + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > + break; > + case MMC_SIGNAL_VOLTAGE_330: > + /* > + * Set VDDIO_B voltage to High for 3.3V > + * which is controlling by GPIO Expander. > + */ > + gpiod_set_value_cansleep(sdhci_arasan->uhs_gpio, 1); > + > + /* > + * This is like final gatekeeper. Need to ensure changed voltage > + * is settled before and after turn on this bit. > + */ > + usleep_range(1000, 1100); > + > + ret = keembay_sd_voltage_selection(KEEMBAY_SET_3V3_VOLT); > + if (ret) > + return ret; > + > + usleep_range(1000, 1100); > + > + /* Set 1.8V Signal Enable in the Host Control2 register to 0 */ > + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); > + ctrl_2 &= ~SDHCI_CTRL_VDD_180; > + sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2); > + > + /* Sleep for 5ms to stabilize 3.3V regulator */ > + usleep_range(5000, 5500); > + > + /* 3.3V regulator output should be stable within 5 ms */ > + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); > + if (ctrl_2 & SDHCI_CTRL_VDD_180) > + return -EAGAIN; > + > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int sdhci_arasan_keembay_select_drive_strength(struct mmc_card *card, > + unsigned int max_dtr, int host_drv, > + int card_drv, int *drv_type) > +{ > + if (card->host->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_180) > + *drv_type = MMC_SET_DRIVER_TYPE_C; > + > + return 0; > +} > + > static const struct sdhci_ops sdhci_arasan_ops = { > .set_clock = sdhci_arasan_set_clock, > .get_max_clock = sdhci_pltfm_clk_get_max_clock, > @@ -1521,6 +1631,7 @@ static int sdhci_arasan_probe(struct platform_device *pdev) > struct sdhci_pltfm_host *pltfm_host; > struct sdhci_arasan_data *sdhci_arasan; > struct device_node *np = pdev->dev.of_node; > + struct device *dev = &pdev->dev; nit: I got this but as I see 3 lines below maybe would be better to use it everywhere but it can be done in separate patch. > const struct sdhci_arasan_of_data *data; > > match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node); > @@ -1600,6 +1711,22 @@ static int sdhci_arasan_probe(struct platform_device *pdev) > host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; > } > > + if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) { > + struct gpio_desc *uhs; > + > + uhs = devm_gpiod_get_optional(dev, "uhs", GPIOD_OUT_HIGH); I can't see change in dt binding to record uhs gpio. Better sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs", GPIOD_OUT_HIGH); then you can avoid uhs variable. > + if (IS_ERR(uhs)) > + return dev_err_probe(dev, PTR_ERR(uhs), "can't get uhs gpio\n"); > + > + sdhci_arasan->uhs_gpio = uhs; > + > + host->mmc_host_ops.start_signal_voltage_switch = > + sdhci_arasan_keembay_voltage_switch; > + > + host->mmc_host_ops.select_drive_strength = > + sdhci_arasan_keembay_select_drive_strength; > + } > + > sdhci_arasan_update_baseclkfreq(host); > > ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, &pdev->dev); > Thanks, Michal
On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com> wrote: > On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote: ... > > + /* > > + * This is like final gatekeeper. Need to ensure changed voltage like a final > > + * is settled before and after turn on this bit. > > + */ ... > > + /* > > + * This is like final gatekeeper. Need to ensure changed voltage Likewise. > > + * is settled before and after turn on this bit. > > + */ ... > > + struct device *dev = &pdev->dev; > > nit: I got this but as I see 3 lines below maybe would be better to use > it everywhere but it can be done in separate patch. In that case I think it would be better to have that patch first. It make follow up code cleaner. ... > > + if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) { > > + struct gpio_desc *uhs; > > + > > + uhs = devm_gpiod_get_optional(dev, "uhs", GPIOD_OUT_HIGH); > > I can't see change in dt binding to record uhs gpio. > > > Better > sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs", > GPIOD_OUT_HIGH); > > then you can avoid uhs variable. Actually it's readability vs. additional variable. It was my suggestion to have a variable to make readability better. Are you insisting on this change?
On 07. 10. 20 10:55, Andy Shevchenko wrote: > On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com> wrote: >> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote: > > ... > >>> + /* >>> + * This is like final gatekeeper. Need to ensure changed voltage > > like a final > >>> + * is settled before and after turn on this bit. >>> + */ > > ... > >>> + /* >>> + * This is like final gatekeeper. Need to ensure changed voltage > > Likewise. > >>> + * is settled before and after turn on this bit. >>> + */ > > ... > >>> + struct device *dev = &pdev->dev; >> >> nit: I got this but as I see 3 lines below maybe would be better to use >> it everywhere but it can be done in separate patch. > > In that case I think it would be better to have that patch first. It > make follow up code cleaner. > > ... > >>> + if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) { >>> + struct gpio_desc *uhs; >>> + >>> + uhs = devm_gpiod_get_optional(dev, "uhs", GPIOD_OUT_HIGH); >> >> I can't see change in dt binding to record uhs gpio. >> >> >> Better >> sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs", >> GPIOD_OUT_HIGH); >> >> then you can avoid uhs variable. > > Actually it's readability vs. additional variable. It was my > suggestion to have a variable to make readability better. > Are you insisting on this change? I understand that it is just about preference. I would use it directly not to deal with it. If your preference is via variable I am fine with it too. Thanks, Michal
On Wed, Oct 7, 2020 at 12:10 PM Michal Simek <michal.simek@xilinx.com> wrote: > On 07. 10. 20 10:55, Andy Shevchenko wrote: > > On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com> wrote: > >> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote: ... > >>> + if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) { > >>> + struct gpio_desc *uhs; > >>> + > >>> + uhs = devm_gpiod_get_optional(dev, "uhs", GPIOD_OUT_HIGH); > >> > >> I can't see change in dt binding to record uhs gpio. > >> > >> > >> Better > >> sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs", > >> GPIOD_OUT_HIGH); > >> > >> then you can avoid uhs variable. > > > > Actually it's readability vs. additional variable. It was my > > suggestion to have a variable to make readability better. > > Are you insisting on this change? > > I understand that it is just about preference. Correct. > I would use it directly > not to deal with it. If your preference is via variable I am fine with > it too. Yes, I suggested that and I still have the same opinion (It seems the original code tries to follow 80 character rules that's why I suggested the change). Thanks!
Hi Michal, Thanks again for the feedback. I replied inline >-----Original Message----- >From: Michal Simek <michal.simek@xilinx.com> >Sent: Wednesday, October 7, 2020 4:34 PM >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; >Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com; >sudeep.holla@arm.com; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; >linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org >Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>; >Wan Mohamad, Wan Ahmad Zainie ><wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de >Subject: Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for >Keem Bay SOC > > > >On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote: >> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> >> >> Voltage switching sequence is needed to support UHS-1 interface. >> There are 2 places to control the voltage. >> 1) By setting the AON register using firmware driver calling >> system-level platform management layer (SMC) to set the register. >> 2) By controlling the GPIO expander value to drive either 1.8V or 3.3V >> for power mux input. >> >> Signed-off-by: Muhammad Husaini Zulkifli >> <muhammad.husaini.zulkifli@intel.com> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> >> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com> >> --- >> drivers/mmc/host/sdhci-of-arasan.c | 127 >> +++++++++++++++++++++++++++++ >> 1 file changed, 127 insertions(+) >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c >> b/drivers/mmc/host/sdhci-of-arasan.c >> index f186fbd016b1..e681e6f860ba 100644 >> --- a/drivers/mmc/host/sdhci-of-arasan.c >> +++ b/drivers/mmc/host/sdhci-of-arasan.c >> @@ -16,6 +16,7 @@ >> */ >> >> #include <linux/clk-provider.h> >> +#include <linux/gpio/consumer.h> >> #include <linux/mfd/syscon.h> >> #include <linux/module.h> >> #include <linux/of_device.h> >> @@ -23,6 +24,7 @@ >> #include <linux/regmap.h> >> #include <linux/of.h> >> #include <linux/firmware/xlnx-zynqmp.h> >> +#include <linux/firmware/intel/keembay_firmware.h> >> >> #include "cqhci.h" >> #include "sdhci-pltfm.h" >> @@ -150,6 +152,7 @@ struct sdhci_arasan_data { >> struct regmap *soc_ctl_base; >> const struct sdhci_arasan_soc_ctl_map *soc_ctl_map; >> unsigned int quirks; >> + struct gpio_desc *uhs_gpio; >> >> /* Controller does not have CD wired and will not function normally without >*/ >> #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST BIT(0) >> @@ -361,6 +364,113 @@ static int sdhci_arasan_voltage_switch(struct >mmc_host *mmc, >> return -EINVAL; >> } >> >> +static int sdhci_arasan_keembay_voltage_switch(struct mmc_host *mmc, >> + struct mmc_ios *ios) >> +{ >> + struct sdhci_host *host = mmc_priv(mmc); >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct sdhci_arasan_data *sdhci_arasan = >sdhci_pltfm_priv(pltfm_host); >> + u16 ctrl_2; >> + u16 clk; > >nit: put it to one line. Noted. Done the changes. > >> + int ret; >> + >> + switch (ios->signal_voltage) { >> + case MMC_SIGNAL_VOLTAGE_180: >> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > >nit: double space Noted. Done the changes. > >> + clk &= ~SDHCI_CLOCK_CARD_EN; >> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >> + >> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > >nit: double space again. Noted. Done the changes. > >> + if (clk & SDHCI_CLOCK_CARD_EN) >> + return -EAGAIN; >> + >> + sdhci_writeb(host, SDHCI_POWER_ON | SDHCI_POWER_180, >> + SDHCI_POWER_CONTROL); >> + >> + /* >> + * Set VDDIO_B voltage to Low for 1.8V >> + * which is controlling by GPIO Expander. >> + */ >> + gpiod_set_value_cansleep(sdhci_arasan->uhs_gpio, 0); >> + >> + /* >> + * This is like final gatekeeper. Need to ensure changed voltage >> + * is settled before and after turn on this bit. >> + */ >> + usleep_range(1000, 1100); >> + >> + ret = >keembay_sd_voltage_selection(KEEMBAY_SET_1V8_VOLT); >> + if (ret) >> + return ret; >> + >> + usleep_range(1000, 1100); >> + >> + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); >> + ctrl_2 |= SDHCI_CTRL_VDD_180; >> + sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2); >> + >> + /* Sleep for 5ms to stabilize 1.8V regulator */ >> + usleep_range(5000, 5500); >> + >> + /* 1.8V regulator output should be stable within 5 ms */ >> + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); >> + if (!(ctrl_2 & SDHCI_CTRL_VDD_180)) >> + return -EAGAIN; >> + >> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); >> + clk |= SDHCI_CLOCK_CARD_EN; >> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >> + break; >> + case MMC_SIGNAL_VOLTAGE_330: >> + /* >> + * Set VDDIO_B voltage to High for 3.3V >> + * which is controlling by GPIO Expander. >> + */ >> + gpiod_set_value_cansleep(sdhci_arasan->uhs_gpio, 1); >> + >> + /* >> + * This is like final gatekeeper. Need to ensure changed voltage >> + * is settled before and after turn on this bit. >> + */ >> + usleep_range(1000, 1100); >> + >> + ret = >keembay_sd_voltage_selection(KEEMBAY_SET_3V3_VOLT); >> + if (ret) >> + return ret; >> + >> + usleep_range(1000, 1100); >> + >> + /* Set 1.8V Signal Enable in the Host Control2 register to 0 */ >> + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); >> + ctrl_2 &= ~SDHCI_CTRL_VDD_180; >> + sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2); >> + >> + /* Sleep for 5ms to stabilize 3.3V regulator */ >> + usleep_range(5000, 5500); >> + >> + /* 3.3V regulator output should be stable within 5 ms */ >> + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); >> + if (ctrl_2 & SDHCI_CTRL_VDD_180) >> + return -EAGAIN; >> + >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int sdhci_arasan_keembay_select_drive_strength(struct mmc_card >*card, >> + unsigned int max_dtr, int host_drv, >> + int card_drv, int *drv_type) >> +{ >> + if (card->host->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_180) >> + *drv_type = MMC_SET_DRIVER_TYPE_C; >> + >> + return 0; >> +} >> + >> static const struct sdhci_ops sdhci_arasan_ops = { >> .set_clock = sdhci_arasan_set_clock, >> .get_max_clock = sdhci_pltfm_clk_get_max_clock, @@ -1521,6 +1631,7 >> @@ static int sdhci_arasan_probe(struct platform_device *pdev) >> struct sdhci_pltfm_host *pltfm_host; >> struct sdhci_arasan_data *sdhci_arasan; >> struct device_node *np = pdev->dev.of_node; >> + struct device *dev = &pdev->dev; > >nit: I got this but as I see 3 lines below maybe would be better to use it >everywhere but it can be done in separate patch. > >> const struct sdhci_arasan_of_data *data; >> >> match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node); >@@ >> -1600,6 +1711,22 @@ static int sdhci_arasan_probe(struct platform_device >*pdev) >> host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; >> } >> >> + if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) { >> + struct gpio_desc *uhs; >> + >> + uhs = devm_gpiod_get_optional(dev, "uhs", >GPIOD_OUT_HIGH); > >I can't see change in dt binding to record uhs gpio. Noted. Done the changes. Will add binding for this > > >Better >sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs", >GPIOD_OUT_HIGH); > >then you can avoid uhs variable. Will remain as it is to make it more readable. > >> + if (IS_ERR(uhs)) >> + return dev_err_probe(dev, PTR_ERR(uhs), "can't get >uhs gpio\n"); >> + >> + sdhci_arasan->uhs_gpio = uhs; >> + >> + host->mmc_host_ops.start_signal_voltage_switch = >> + sdhci_arasan_keembay_voltage_switch; >> + >> + host->mmc_host_ops.select_drive_strength = >> + sdhci_arasan_keembay_select_drive_strength; >> + } >> + >> sdhci_arasan_update_baseclkfreq(host); >> >> ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, >> &pdev->dev); >> > >Thanks, >Michal
Hi Andy, Thanks for the feedback. I replied inline >-----Original Message----- >From: Andy Shevchenko <andy.shevchenko@gmail.com> >Sent: Wednesday, October 7, 2020 4:56 PM >To: Michal Simek <michal.simek@xilinx.com> >Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; >Hunter, Adrian <adrian.hunter@intel.com>; Sudeep Holla ><sudeep.holla@arm.com>; Ulf Hansson <ulf.hansson@linaro.org>; linux-mmc ><linux-mmc@vger.kernel.org>; linux-arm Mailing List <linux-arm- >kernel@lists.infradead.org>; Linux Kernel Mailing List <linux- >kernel@vger.kernel.org>; Raja Subramanian, Lakshmi Bai ><lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad >Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Arnd Bergmann ><arnd@arndb.de> >Subject: Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for >Keem Bay SOC > >On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com> >wrote: >> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote: > >... > >> > + /* >> > + * This is like final gatekeeper. Need to ensure >> > + changed voltage > >like a final Noted. Done the changes > >> > + * is settled before and after turn on this bit. >> > + */ > >... > >> > + /* >> > + * This is like final gatekeeper. Need to ensure >> > + changed voltage > >Likewise. Noted. Done the changes > >> > + * is settled before and after turn on this bit. >> > + */ > >... > >> > + struct device *dev = &pdev->dev; >> >> nit: I got this but as I see 3 lines below maybe would be better to >> use it everywhere but it can be done in separate patch. > >In that case I think it would be better to have that patch first. It make follow up >code cleaner. I want to get some clarification here. Do I need a separate patch for this struct device *dev = &pdev->dev;? Can I embedded together with UHS patch? > >... > >> > + if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) { >> > + struct gpio_desc *uhs; >> > + >> > + uhs = devm_gpiod_get_optional(dev, "uhs", >> > + GPIOD_OUT_HIGH); >> >> I can't see change in dt binding to record uhs gpio. >> >> >> Better >> sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs", >> GPIOD_OUT_HIGH); >> >> then you can avoid uhs variable. > >Actually it's readability vs. additional variable. It was my suggestion to have a >variable to make readability better. >Are you insisting on this change? > >-- >With Best Regards, >Andy Shevchenko
On Wed, Oct 7, 2020 at 4:28 PM Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> wrote: > >From: Andy Shevchenko <andy.shevchenko@gmail.com> > >Sent: Wednesday, October 7, 2020 4:56 PM > >On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com> > >wrote: > >> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote: ... > >> > + struct device *dev = &pdev->dev; > >> > >> nit: I got this but as I see 3 lines below maybe would be better to > >> use it everywhere but it can be done in separate patch. > > > >In that case I think it would be better to have that patch first. It make follow up > >code cleaner. > I want to get some clarification here. > Do I need a separate patch for this struct device *dev = &pdev->dev;? It should be a separate patch and better your series starts with it, so it won't interfere with new code. > Can I embedded together with UHS patch? Better to avoid merging orthogonal things together in one change.
Hi , >-----Original Message----- >From: Andy Shevchenko <andy.shevchenko@gmail.com> >Sent: Wednesday, October 7, 2020 10:55 PM >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> >Cc: Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian ><adrian.hunter@intel.com>; Sudeep Holla <sudeep.holla@arm.com>; Ulf >Hansson <ulf.hansson@linaro.org>; linux-mmc <linux-mmc@vger.kernel.org>; >linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>; Linux Kernel >Mailing List <linux-kernel@vger.kernel.org>; Raja Subramanian, Lakshmi Bai ><lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad Zainie ><wan.ahmad.zainie.wan.mohamad@intel.com>; Arnd Bergmann ><arnd@arndb.de> >Subject: Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for >Keem Bay SOC > >On Wed, Oct 7, 2020 at 4:28 PM Zulkifli, Muhammad Husaini ><muhammad.husaini.zulkifli@intel.com> wrote: >> >From: Andy Shevchenko <andy.shevchenko@gmail.com> >> >Sent: Wednesday, October 7, 2020 4:56 PM On Wed, Oct 7, 2020 at 11:38 >> >AM Michal Simek <michal.simek@xilinx.com> >> >wrote: >> >> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote: > >... > >> >> > + struct device *dev = &pdev->dev; >> >> >> >> nit: I got this but as I see 3 lines below maybe would be better to >> >> use it everywhere but it can be done in separate patch. >> > >> >In that case I think it would be better to have that patch first. It >> >make follow up code cleaner. >> I want to get some clarification here. > >> Do I need a separate patch for this struct device *dev = &pdev->dev;? > >It should be a separate patch and better your series starts with it, so it won't >interfere with new code. > >> Can I embedded together with UHS patch? > >Better to avoid merging orthogonal things together in one change. Noted. Thanks
diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c index f186fbd016b1..e681e6f860ba 100644 --- a/drivers/mmc/host/sdhci-of-arasan.c +++ b/drivers/mmc/host/sdhci-of-arasan.c @@ -16,6 +16,7 @@ */ #include <linux/clk-provider.h> +#include <linux/gpio/consumer.h> #include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of_device.h> @@ -23,6 +24,7 @@ #include <linux/regmap.h> #include <linux/of.h> #include <linux/firmware/xlnx-zynqmp.h> +#include <linux/firmware/intel/keembay_firmware.h> #include "cqhci.h" #include "sdhci-pltfm.h" @@ -150,6 +152,7 @@ struct sdhci_arasan_data { struct regmap *soc_ctl_base; const struct sdhci_arasan_soc_ctl_map *soc_ctl_map; unsigned int quirks; + struct gpio_desc *uhs_gpio; /* Controller does not have CD wired and will not function normally without */ #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST BIT(0) @@ -361,6 +364,113 @@ static int sdhci_arasan_voltage_switch(struct mmc_host *mmc, return -EINVAL; } +static int sdhci_arasan_keembay_voltage_switch(struct mmc_host *mmc, + struct mmc_ios *ios) +{ + struct sdhci_host *host = mmc_priv(mmc); + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); + u16 ctrl_2; + u16 clk; + int ret; + + switch (ios->signal_voltage) { + case MMC_SIGNAL_VOLTAGE_180: + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); + clk &= ~SDHCI_CLOCK_CARD_EN; + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); + + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); + if (clk & SDHCI_CLOCK_CARD_EN) + return -EAGAIN; + + sdhci_writeb(host, SDHCI_POWER_ON | SDHCI_POWER_180, + SDHCI_POWER_CONTROL); + + /* + * Set VDDIO_B voltage to Low for 1.8V + * which is controlling by GPIO Expander. + */ + gpiod_set_value_cansleep(sdhci_arasan->uhs_gpio, 0); + + /* + * This is like final gatekeeper. Need to ensure changed voltage + * is settled before and after turn on this bit. + */ + usleep_range(1000, 1100); + + ret = keembay_sd_voltage_selection(KEEMBAY_SET_1V8_VOLT); + if (ret) + return ret; + + usleep_range(1000, 1100); + + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); + ctrl_2 |= SDHCI_CTRL_VDD_180; + sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2); + + /* Sleep for 5ms to stabilize 1.8V regulator */ + usleep_range(5000, 5500); + + /* 1.8V regulator output should be stable within 5 ms */ + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); + if (!(ctrl_2 & SDHCI_CTRL_VDD_180)) + return -EAGAIN; + + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); + clk |= SDHCI_CLOCK_CARD_EN; + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); + break; + case MMC_SIGNAL_VOLTAGE_330: + /* + * Set VDDIO_B voltage to High for 3.3V + * which is controlling by GPIO Expander. + */ + gpiod_set_value_cansleep(sdhci_arasan->uhs_gpio, 1); + + /* + * This is like final gatekeeper. Need to ensure changed voltage + * is settled before and after turn on this bit. + */ + usleep_range(1000, 1100); + + ret = keembay_sd_voltage_selection(KEEMBAY_SET_3V3_VOLT); + if (ret) + return ret; + + usleep_range(1000, 1100); + + /* Set 1.8V Signal Enable in the Host Control2 register to 0 */ + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); + ctrl_2 &= ~SDHCI_CTRL_VDD_180; + sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2); + + /* Sleep for 5ms to stabilize 3.3V regulator */ + usleep_range(5000, 5500); + + /* 3.3V regulator output should be stable within 5 ms */ + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2); + if (ctrl_2 & SDHCI_CTRL_VDD_180) + return -EAGAIN; + + break; + default: + return -EINVAL; + } + + return 0; +} + +static int sdhci_arasan_keembay_select_drive_strength(struct mmc_card *card, + unsigned int max_dtr, int host_drv, + int card_drv, int *drv_type) +{ + if (card->host->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_180) + *drv_type = MMC_SET_DRIVER_TYPE_C; + + return 0; +} + static const struct sdhci_ops sdhci_arasan_ops = { .set_clock = sdhci_arasan_set_clock, .get_max_clock = sdhci_pltfm_clk_get_max_clock, @@ -1521,6 +1631,7 @@ static int sdhci_arasan_probe(struct platform_device *pdev) struct sdhci_pltfm_host *pltfm_host; struct sdhci_arasan_data *sdhci_arasan; struct device_node *np = pdev->dev.of_node; + struct device *dev = &pdev->dev; const struct sdhci_arasan_of_data *data; match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node); @@ -1600,6 +1711,22 @@ static int sdhci_arasan_probe(struct platform_device *pdev) host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; } + if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) { + struct gpio_desc *uhs; + + uhs = devm_gpiod_get_optional(dev, "uhs", GPIOD_OUT_HIGH); + if (IS_ERR(uhs)) + return dev_err_probe(dev, PTR_ERR(uhs), "can't get uhs gpio\n"); + + sdhci_arasan->uhs_gpio = uhs; + + host->mmc_host_ops.start_signal_voltage_switch = + sdhci_arasan_keembay_voltage_switch; + + host->mmc_host_ops.select_drive_strength = + sdhci_arasan_keembay_select_drive_strength; + } + sdhci_arasan_update_baseclkfreq(host); ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, &pdev->dev);