| Message ID | 1422010594-1735-3-git-send-email-gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[...] > + /* > + * According to erratum 'ERR-7878951' Armada 38x SDHCI > + * controller has different capabilities than the ones shown > + * in its registers > + */ > + host->caps = sdhci_readl(host, SDHCI_CAPABILITIES); > + if (of_get_property(np, "no-1-8-v", NULL)) { Please use of_property_read_bool(np, "no-1-8-v") > + host->caps &= ~SDHCI_CAN_VDD_180; > + host->mmc->caps &= ~MMC_CAP_1_8V_DDR; Is SDHCI_CAN_VDD_330 always set elsewhere in this case? > + } else > + host->caps &= ~SDHCI_CAN_VDD_330; If one branch in an if-else pair is braced, both sides should be (as Documentation/CodingStyle says). Please brace the else case. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark 2015-01-23 12:03 GMT+01:00 Mark Rutland <mark.rutland@arm.com>: > [...] > >> + /* >> + * According to erratum 'ERR-7878951' Armada 38x SDHCI >> + * controller has different capabilities than the ones shown >> + * in its registers >> + */ >> + host->caps = sdhci_readl(host, SDHCI_CAPABILITIES); >> + if (of_get_property(np, "no-1-8-v", NULL)) { > > Please use of_property_read_bool(np, "no-1-8-v") > >> + host->caps &= ~SDHCI_CAN_VDD_180; >> + host->mmc->caps &= ~MMC_CAP_1_8V_DDR; > > Is SDHCI_CAN_VDD_330 always set elsewhere in this case? Yes, it's always set in SDHCI_CAPABILITIES register of Armada 38x (assigned to host->caps few lines above), as well as SDHCI_CAN_VDD_180. The point is, that in reality they can't be used simultaneously for this device, i.e. voltage switching is impossible. Thanks, Marcin > >> + } else >> + host->caps &= ~SDHCI_CAN_VDD_330; > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Gregory, > @@ -118,8 +118,11 @@ static int mv_conf_mbus_windows(struct platform_device *pdev, > return 0; > } > > -static int armada_38x_quirks(struct sdhci_host *host) > +static int armada_38x_quirks(struct platform_device *pdev, > + struct sdhci_host *host) > { > + struct device_node *np = pdev->dev.of_node; > + > host->quirks |= SDHCI_QUIRK_MISSING_CAPS; > /* > * According to erratum 'FE-2946959' both SDR50 and DDR50 > @@ -129,6 +132,20 @@ static int armada_38x_quirks(struct sdhci_host *host) > */ > host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1); > host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50); > + > + /* > + * According to erratum 'ERR-7878951' Armada 38x SDHCI > + * controller has different capabilities than the ones shown > + * in its registers > + */ > + host->caps = sdhci_readl(host, SDHCI_CAPABILITIES); > + if (of_get_property(np, "no-1-8-v", NULL)) { > + host->caps &= ~SDHCI_CAN_VDD_180; > + host->mmc->caps &= ~MMC_CAP_1_8V_DDR; > + } else > + host->caps &= ~SDHCI_CAN_VDD_330; In this "else" there is one thing missing host->mmc->caps |= MMC_CAP_1_8V_DDR; because it's not set by default, however it's not a must - please see my remark below. > + host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_USE_SDR50_TUNING); > + > return 0; > } > > @@ -333,7 +350,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > clk_prepare_enable(pxa->clk_core); > > if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) { > - ret = armada_38x_quirks(host); > + ret = armada_38x_quirks(pdev, host); > if (ret < 0) > goto err_quirks; > ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info()); > @@ -341,9 +358,6 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > goto err_mbus_win; > } > > - /* enable 1/8V DDR capable */ > - host->mmc->caps |= MMC_CAP_1_8V_DDR; > - Now, with this change MMC_CAP_1_8V_DDR is disabled for devices other than Armada 38x, that use sdhci-pxav3. There are two options: 1. Move those lines above the place where armada_38x_quirks is called (IMO the easiest option - 'else' in this function would not need to be supplemented, as I pointed above) 2. Leave it "as is" here, but a condition below is needed (+ supplementation of 'else' in armada_38x_quirks) if (!of_device_is_compatible(np, "marvell,armada-380-sdhci")) Best regards, Marcin -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marcin, On 23/01/2015 12:33, Marcin Wojtas wrote: > Dear Gregory, > > >> @@ -118,8 +118,11 @@ static int mv_conf_mbus_windows(struct platform_device *pdev, >> return 0; >> } >> >> -static int armada_38x_quirks(struct sdhci_host *host) >> +static int armada_38x_quirks(struct platform_device *pdev, >> + struct sdhci_host *host) >> { >> + struct device_node *np = pdev->dev.of_node; >> + >> host->quirks |= SDHCI_QUIRK_MISSING_CAPS; >> /* >> * According to erratum 'FE-2946959' both SDR50 and DDR50 >> @@ -129,6 +132,20 @@ static int armada_38x_quirks(struct sdhci_host *host) >> */ >> host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1); >> host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50); >> + >> + /* >> + * According to erratum 'ERR-7878951' Armada 38x SDHCI >> + * controller has different capabilities than the ones shown >> + * in its registers >> + */ >> + host->caps = sdhci_readl(host, SDHCI_CAPABILITIES); >> + if (of_get_property(np, "no-1-8-v", NULL)) { >> + host->caps &= ~SDHCI_CAN_VDD_180; >> + host->mmc->caps &= ~MMC_CAP_1_8V_DDR; >> + } else >> + host->caps &= ~SDHCI_CAN_VDD_330; > > In this "else" there is one thing missing > host->mmc->caps |= MMC_CAP_1_8V_DDR; > because it's not set by default, however it's not a must - please see > my remark below. > >> + host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_USE_SDR50_TUNING); >> + >> return 0; >> } >> >> @@ -333,7 +350,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) >> clk_prepare_enable(pxa->clk_core); >> >> if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) { >> - ret = armada_38x_quirks(host); >> + ret = armada_38x_quirks(pdev, host); >> if (ret < 0) >> goto err_quirks; >> ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info()); >> @@ -341,9 +358,6 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) >> goto err_mbus_win; >> } >> >> - /* enable 1/8V DDR capable */ >> - host->mmc->caps |= MMC_CAP_1_8V_DDR; >> - > > Now, with this change MMC_CAP_1_8V_DDR is disabled for devices other > than Armada 38x, that use sdhci-pxav3. There are two options: > 1. Move those lines above the place where armada_38x_quirks is called > (IMO the easiest option - 'else' in this function would not need to be > supplemented, as I pointed above) > 2. Leave it "as is" here, but a condition below is needed (+ > supplementation of 'else' in armada_38x_quirks) > if (!of_device_is_compatible(np, "marvell,armada-380-sdhci")) > I will take the first option. A third version is coming soon addressing also Mark's comments. Thanks, Gregory
diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c index 7b07325b4fba..a53968010991 100644 --- a/drivers/mmc/host/sdhci-pxav3.c +++ b/drivers/mmc/host/sdhci-pxav3.c @@ -118,8 +118,11 @@ static int mv_conf_mbus_windows(struct platform_device *pdev, return 0; } -static int armada_38x_quirks(struct sdhci_host *host) +static int armada_38x_quirks(struct platform_device *pdev, + struct sdhci_host *host) { + struct device_node *np = pdev->dev.of_node; + host->quirks |= SDHCI_QUIRK_MISSING_CAPS; /* * According to erratum 'FE-2946959' both SDR50 and DDR50 @@ -129,6 +132,20 @@ static int armada_38x_quirks(struct sdhci_host *host) */ host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1); host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50); + + /* + * According to erratum 'ERR-7878951' Armada 38x SDHCI + * controller has different capabilities than the ones shown + * in its registers + */ + host->caps = sdhci_readl(host, SDHCI_CAPABILITIES); + if (of_get_property(np, "no-1-8-v", NULL)) { + host->caps &= ~SDHCI_CAN_VDD_180; + host->mmc->caps &= ~MMC_CAP_1_8V_DDR; + } else + host->caps &= ~SDHCI_CAN_VDD_330; + host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_USE_SDR50_TUNING); + return 0; } @@ -333,7 +350,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) clk_prepare_enable(pxa->clk_core); if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) { - ret = armada_38x_quirks(host); + ret = armada_38x_quirks(pdev, host); if (ret < 0) goto err_quirks; ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info()); @@ -341,9 +358,6 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) goto err_mbus_win; } - /* enable 1/8V DDR capable */ - host->mmc->caps |= MMC_CAP_1_8V_DDR; - match = of_match_device(of_match_ptr(sdhci_pxav3_of_match), &pdev->dev); if (match) { ret = mmc_of_parse(host->mmc);