| Message ID | 1453042744-16196-5-git-send-email-stefan.wahren@i2se.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Stefan, The caps likely need to be looked at in greater detail. Some comments inline. Regards, Scott On 16-01-17 06:59 AM, Stefan Wahren wrote: > Scott Branden from Broadcom said that the BCM2835 eMMC IP core is > very similar to IPROC and share most of the quirks. So use this driver > instead of separate one. > > The sdhci-iproc contains a better workaround for the clock domain > crossing problem which doesn't need any delays. This results in a > better write performance. > > Btw we get the rid of the SDHCI_CAPABILITIES hack in the sdhci_readl > function. > > Suggested-by: Scott Branden <sbranden@broadcom.com> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > --- > drivers/mmc/host/Kconfig | 6 +++--- > drivers/mmc/host/sdhci-iproc.c | 20 ++++++++++++++++++-- > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > index 1526b8a..60de1e4 100644 > --- a/drivers/mmc/host/Kconfig > +++ b/drivers/mmc/host/Kconfig > @@ -318,15 +318,15 @@ config MMC_SDHCI_F_SDH30 > If unsure, say N. > > config MMC_SDHCI_IPROC > - tristate "SDHCI platform support for the iProc SD/MMC Controller" > - depends on ARCH_BCM_IPROC || COMPILE_TEST > + tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller" > + depends on ARCH_BCM2835 || ARCH_BCM_IPROC || COMPILE_TEST > depends on MMC_SDHCI_PLTFM > default ARCH_BCM_IPROC > select MMC_SDHCI_IO_ACCESSORS > help > This selects the iProc SD/MMC controller. > > - If you have an IPROC platform with SD or MMC devices, > + If you have a BCM2835 or IPROC platform with SD or MMC devices, > say Y or M here. > > If unsure, say N. > diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c > index 55bc348..88399eb 100644 > --- a/drivers/mmc/host/sdhci-iproc.c > +++ b/drivers/mmc/host/sdhci-iproc.c > @@ -167,7 +167,20 @@ static const struct sdhci_iproc_data iproc_data = { > .caps1 = 0x00000064, > }; > > +static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = { > + .quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK > + | SDHCI_QUIRK_MISSING_CAPS, > + .ops = &sdhci_iproc_ops, > +}; > + > +static const struct sdhci_iproc_data bcm2835_data = { > + .pdata = &sdhci_bcm2835_pltfm_data, > + .caps = SDHCI_CAN_VDD_330, > + .caps1 = 0x00000000, caps here may be closer to iproc settings - need to review this. > +}; > + > static const struct of_device_id sdhci_iproc_of_match[] = { > + { .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data }, > { .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data }, > { } > }; > @@ -180,6 +193,7 @@ static int sdhci_iproc_probe(struct platform_device *pdev) > struct sdhci_host *host; > struct sdhci_iproc_host *iproc_host; > struct sdhci_pltfm_host *pltfm_host; > + struct device_node *np = pdev->dev.of_node; > int ret; > > match = of_match_device(sdhci_iproc_of_match, &pdev->dev); > @@ -199,8 +213,10 @@ static int sdhci_iproc_probe(struct platform_device *pdev) > mmc_of_parse(host->mmc); > sdhci_get_of_property(pdev); > > - /* Enable EMMC 1/8V DDR capable */ > - host->mmc->caps |= MMC_CAP_1_8V_DDR; > + if (of_device_is_compatible(np, "brcm,sdhci-iproc-cygnus")) { > + /* Enable EMMC 1/8V DDR capable */ > + host->mmc->caps |= MMC_CAP_1_8V_DDR; It is fine to limit this to cygnus. But, it may work on RPI as well for higher speed SD cards... > + } > > pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(pltfm_host->clk)) { > -- 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
On 01/17/2016 11:59 PM, Stefan Wahren wrote: > Scott Branden from Broadcom said that the BCM2835 eMMC IP core is > very similar to IPROC and share most of the quirks. So use this driver > instead of separate one. > > The sdhci-iproc contains a better workaround for the clock domain > crossing problem which doesn't need any delays. This results in a > better write performance. > > Btw we get the rid of the SDHCI_CAPABILITIES hack in the sdhci_readl > function. > > Suggested-by: Scott Branden <sbranden@broadcom.com> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > --- > drivers/mmc/host/Kconfig | 6 +++--- > drivers/mmc/host/sdhci-iproc.c | 20 ++++++++++++++++++-- > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > index 1526b8a..60de1e4 100644 > --- a/drivers/mmc/host/Kconfig > +++ b/drivers/mmc/host/Kconfig > @@ -318,15 +318,15 @@ config MMC_SDHCI_F_SDH30 > If unsure, say N. > > config MMC_SDHCI_IPROC > - tristate "SDHCI platform support for the iProc SD/MMC Controller" > - depends on ARCH_BCM_IPROC || COMPILE_TEST > + tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller" > + depends on ARCH_BCM2835 || ARCH_BCM_IPROC || COMPILE_TEST > depends on MMC_SDHCI_PLTFM > default ARCH_BCM_IPROC > select MMC_SDHCI_IO_ACCESSORS > help > This selects the iProc SD/MMC controller. > > - If you have an IPROC platform with SD or MMC devices, > + If you have a BCM2835 or IPROC platform with SD or MMC devices, > say Y or M here. > > If unsure, say N. > diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c > index 55bc348..88399eb 100644 > --- a/drivers/mmc/host/sdhci-iproc.c > +++ b/drivers/mmc/host/sdhci-iproc.c > @@ -167,7 +167,20 @@ static const struct sdhci_iproc_data iproc_data = { > .caps1 = 0x00000064, > }; > > +static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = { > + .quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK > + | SDHCI_QUIRK_MISSING_CAPS, > + .ops = &sdhci_iproc_ops, > +}; > + > +static const struct sdhci_iproc_data bcm2835_data = { > + .pdata = &sdhci_bcm2835_pltfm_data, > + .caps = SDHCI_CAN_VDD_330, > + .caps1 = 0x00000000, > +}; > + > static const struct of_device_id sdhci_iproc_of_match[] = { > + { .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data }, > { .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data }, > { } > }; > @@ -180,6 +193,7 @@ static int sdhci_iproc_probe(struct platform_device *pdev) > struct sdhci_host *host; > struct sdhci_iproc_host *iproc_host; > struct sdhci_pltfm_host *pltfm_host; > + struct device_node *np = pdev->dev.of_node; > int ret; > > match = of_match_device(sdhci_iproc_of_match, &pdev->dev); > @@ -199,8 +213,10 @@ static int sdhci_iproc_probe(struct platform_device *pdev) > mmc_of_parse(host->mmc); > sdhci_get_of_property(pdev); > > - /* Enable EMMC 1/8V DDR capable */ > - host->mmc->caps |= MMC_CAP_1_8V_DDR; > + if (of_device_is_compatible(np, "brcm,sdhci-iproc-cygnus")) { > + /* Enable EMMC 1/8V DDR capable */ > + host->mmc->caps |= MMC_CAP_1_8V_DDR; > + } Why don't you use the property in device tree? It can be parsed MMC_CAP_1_8V_DDR as property of "mmc-ddr-1_8v". Best Regards, Jaehoon Chung > > pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(pltfm_host->clk)) { > -- 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 Scott, > Scott Branden <sbranden@broadcom.com> hat am 18. Januar 2016 um 22:47 > geschrieben: > > > Hi Stefan, > > The caps likely need to be looked at in greater detail. Some comments > inline. yes i'm hopeful that someone from Broadcom knows more about the capabilities. This patch only copies them from the other driver, because i don't want to introduce new issues. Unfortunately the BCM2835 datasheet refers only to a non-public Asaran document ( SD3.0_Host_AHB_eMMC4.4_Usersguide_ver5.9_jan11_10.pdf ). Best regards Stefan [1] - https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf -- 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 Stefan, On 16-01-19 11:25 AM, Stefan Wahren wrote: > Hi Scott, > >> Scott Branden <sbranden@broadcom.com> hat am 18. Januar 2016 um 22:47 >> geschrieben: >> >> >> Hi Stefan, >> >> The caps likely need to be looked at in greater detail. Some comments >> inline. > > yes i'm hopeful that someone from Broadcom knows more about the capabilities. > This patch only copies them from the other driver, because i don't want to > introduce new issues. This patch can go in as is and we can have another patch when more accurate capabilities are determined. If/when I have time I can go and deduce what the capability settings should be on BCM2835. I should be revisiting this IP for other reasons in the next month or so and will try to look into it. > > Unfortunately the BCM2835 datasheet refers only to a non-public Asaran document > ( SD3.0_Host_AHB_eMMC4.4_Usersguide_ver5.9_jan11_10.pdf ). Yes, if Arasan allowed such document to be published that would help... > > Best regards > Stefan > > [1] - > https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf > Regards, Scott -- 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
> Jaehoon Chung <jh80.chung@samsung.com> hat am 19. Januar 2016 um 02:32 > geschrieben: > > > On 01/17/2016 11:59 PM, Stefan Wahren wrote: > > diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c > > index 55bc348..88399eb 100644 > > --- a/drivers/mmc/host/sdhci-iproc.c > > +++ b/drivers/mmc/host/sdhci-iproc.c > > @@ -167,7 +167,20 @@ static const struct sdhci_iproc_data iproc_data = { > > .caps1 = 0x00000064, > > }; > > > > +static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = { > > + .quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK > > + | SDHCI_QUIRK_MISSING_CAPS, > > + .ops = &sdhci_iproc_ops, > > +}; > > + > > +static const struct sdhci_iproc_data bcm2835_data = { > > + .pdata = &sdhci_bcm2835_pltfm_data, > > + .caps = SDHCI_CAN_VDD_330, > > + .caps1 = 0x00000000, > > +}; > > + > > static const struct of_device_id sdhci_iproc_of_match[] = { > > + { .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data }, > > { .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data }, > > { } > > }; > > @@ -180,6 +193,7 @@ static int sdhci_iproc_probe(struct platform_device > > *pdev) > > struct sdhci_host *host; > > struct sdhci_iproc_host *iproc_host; > > struct sdhci_pltfm_host *pltfm_host; > > + struct device_node *np = pdev->dev.of_node; > > int ret; > > > > match = of_match_device(sdhci_iproc_of_match, &pdev->dev); > > @@ -199,8 +213,10 @@ static int sdhci_iproc_probe(struct platform_device > > *pdev) > > mmc_of_parse(host->mmc); > > sdhci_get_of_property(pdev); > > > > - /* Enable EMMC 1/8V DDR capable */ > > - host->mmc->caps |= MMC_CAP_1_8V_DDR; > > + if (of_device_is_compatible(np, "brcm,sdhci-iproc-cygnus")) { > > + /* Enable EMMC 1/8V DDR capable */ > > + host->mmc->caps |= MMC_CAP_1_8V_DDR; > > + } > > Why don't you use the property in device tree? > It can be parsed MMC_CAP_1_8V_DDR as property of "mmc-ddr-1_8v". I agree but i don't have a iProc Cygnus to test it and we need to keep compatibility to older devicetrees. > > Best Regards, > Jaehoon Chung > > > > > pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); > > if (IS_ERR(pltfm_host->clk)) { > > > -- 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
On 01/17/2016 07:59 AM, Stefan Wahren wrote: > Scott Branden from Broadcom said that the BCM2835 eMMC IP core is > very similar to IPROC and share most of the quirks. So use this driver > instead of separate one. > > The sdhci-iproc contains a better workaround for the clock domain > crossing problem which doesn't need any delays. This results in a > better write performance. > > Btw we get the rid of the SDHCI_CAPABILITIES hack in the sdhci_readl > function. > diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c > +static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = { > + .quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK > + | SDHCI_QUIRK_MISSING_CAPS, > + .ops = &sdhci_iproc_ops, > +}; > + > +static const struct sdhci_iproc_data bcm2835_data = { > + .pdata = &sdhci_bcm2835_pltfm_data, > + .caps = SDHCI_CAN_VDD_330, > + .caps1 = 0x00000000, > +}; > @@ -199,8 +213,10 @@ static int sdhci_iproc_probe(struct platform_device *pdev) > - /* Enable EMMC 1/8V DDR capable */ > - host->mmc->caps |= MMC_CAP_1_8V_DDR; > + if (of_device_is_compatible(np, "brcm,sdhci-iproc-cygnus")) { > + /* Enable EMMC 1/8V DDR capable */ > + host->mmc->caps |= MMC_CAP_1_8V_DDR; > + } Rather that placing device-specific of_device_is_compatible() throughout the driver, I think it'd be better to key off a field in bcm2835_data or sdhci_bcm2835_pltfm_data. That way, if the driver starts supporting additional devices that want this MMC_CAP_1_8V_DDR, you can simply set that up in the relevant data structure, rather than adding a mess of of_device_is_compatible("a") || of_device_is_compatible("b") || ... to the code. -- 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
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index 1526b8a..60de1e4 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -318,15 +318,15 @@ config MMC_SDHCI_F_SDH30 If unsure, say N. config MMC_SDHCI_IPROC - tristate "SDHCI platform support for the iProc SD/MMC Controller" - depends on ARCH_BCM_IPROC || COMPILE_TEST + tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller" + depends on ARCH_BCM2835 || ARCH_BCM_IPROC || COMPILE_TEST depends on MMC_SDHCI_PLTFM default ARCH_BCM_IPROC select MMC_SDHCI_IO_ACCESSORS help This selects the iProc SD/MMC controller. - If you have an IPROC platform with SD or MMC devices, + If you have a BCM2835 or IPROC platform with SD or MMC devices, say Y or M here. If unsure, say N. diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c index 55bc348..88399eb 100644 --- a/drivers/mmc/host/sdhci-iproc.c +++ b/drivers/mmc/host/sdhci-iproc.c @@ -167,7 +167,20 @@ static const struct sdhci_iproc_data iproc_data = { .caps1 = 0x00000064, }; +static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = { + .quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK + | SDHCI_QUIRK_MISSING_CAPS, + .ops = &sdhci_iproc_ops, +}; + +static const struct sdhci_iproc_data bcm2835_data = { + .pdata = &sdhci_bcm2835_pltfm_data, + .caps = SDHCI_CAN_VDD_330, + .caps1 = 0x00000000, +}; + static const struct of_device_id sdhci_iproc_of_match[] = { + { .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data }, { .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data }, { } }; @@ -180,6 +193,7 @@ static int sdhci_iproc_probe(struct platform_device *pdev) struct sdhci_host *host; struct sdhci_iproc_host *iproc_host; struct sdhci_pltfm_host *pltfm_host; + struct device_node *np = pdev->dev.of_node; int ret; match = of_match_device(sdhci_iproc_of_match, &pdev->dev); @@ -199,8 +213,10 @@ static int sdhci_iproc_probe(struct platform_device *pdev) mmc_of_parse(host->mmc); sdhci_get_of_property(pdev); - /* Enable EMMC 1/8V DDR capable */ - host->mmc->caps |= MMC_CAP_1_8V_DDR; + if (of_device_is_compatible(np, "brcm,sdhci-iproc-cygnus")) { + /* Enable EMMC 1/8V DDR capable */ + host->mmc->caps |= MMC_CAP_1_8V_DDR; + } pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(pltfm_host->clk)) {
Scott Branden from Broadcom said that the BCM2835 eMMC IP core is very similar to IPROC and share most of the quirks. So use this driver instead of separate one. The sdhci-iproc contains a better workaround for the clock domain crossing problem which doesn't need any delays. This results in a better write performance. Btw we get the rid of the SDHCI_CAPABILITIES hack in the sdhci_readl function. Suggested-by: Scott Branden <sbranden@broadcom.com> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> --- drivers/mmc/host/Kconfig | 6 +++--- drivers/mmc/host/sdhci-iproc.c | 20 ++++++++++++++++++-- 2 files changed, 21 insertions(+), 5 deletions(-)