[U-Boot] [PATCH v2] dwmmc: make driver usable for non-exynos platforms
There's no point in having of "dwmmc.h" per architecure - we're talking about device driver (DesignWare MMC controller) which is expected to work with any CPU it is attached to.
Actually this change should have been an essential part of commit: ======= 6f0b7caa671f92c2d4676c84381d17fb90f7d2cd DWMMC: SMDK5420: Disable SMU for eMMC =======
Or even better I'd prefer all non pure DW MMC functionality (those item in ifdefs) to be moved to corresponding files like "exynos_dw_mmc.c" via call-backs etc. This will keep DW MMC driver clean and simple.
As it is said in http://www.denx.de/wiki/U-Boot/DesignPrinciples: ======= 8. Keep it Maintainable Avoid #ifdefs where possible =======
Signed-off-by: Alexey Brodkin abrodkin@synopsys.com
Cc: Mischa Jonker mjonker@synopsys.com Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Andy Fleming afleming@gmail.com Cc: Alim Akhtar alim.akhtar@samsung.com Cc: Rajeshwari Shinde rajeshwari.s@samsung.com Cc: Simon Glass sjg@chromium.org Cc: Pantelis Antoniou panto@antoniou-consulting.com
Changes compared to initial version: Instead of modification of headers only disable some parts of code with ifdefs. --- drivers/mmc/dw_mmc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 1e0f72b..fdae40b 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -11,7 +11,9 @@ #include <mmc.h> #include <dwmmc.h> #include <asm-generic/errno.h> +#ifdef CONFIG_EXYNOS5420 #include <asm/arch/dwmmc.h> +#endif
#define PAGE_SIZE 4096
@@ -302,6 +304,7 @@ static int dwmci_init(struct mmc *mmc) struct dwmci_host *host = (struct dwmci_host *)mmc->priv; u32 fifo_size;
+#ifdef CONFIG_EXYNOS5420 if (host->quirks & DWMCI_QUIRK_DISABLE_SMU) { dwmci_writel(host, EMMCP_MPSBEGIN0, 0); dwmci_writel(host, EMMCP_SEND0, 0); @@ -311,6 +314,7 @@ static int dwmci_init(struct mmc *mmc) MPSCTRL_NON_SECURE_READ_BIT | MPSCTRL_NON_SECURE_WRITE_BIT | MPSCTRL_VALID); } +#endif
dwmci_writel(host, DWMCI_PWREN, 1);
Hi Alexey,
I have replied the previous patch.
On 11/29/2013 06:39 PM, Alexey Brodkin wrote:
There's no point in having of "dwmmc.h" per architecure - we're talking about device driver (DesignWare MMC controller) which is expected to work with any CPU it is attached to.
Actually this change should have been an essential part of commit:
6f0b7caa671f92c2d4676c84381d17fb90f7d2cd DWMMC: SMDK5420: Disable SMU for eMMC =======
Or even better I'd prefer all non pure DW MMC functionality (those item in ifdefs) to be moved to corresponding files like "exynos_dw_mmc.c" via call-backs etc. This will keep DW MMC driver clean and simple.
As it is said in http://www.denx.de/wiki/U-Boot/DesignPrinciples:
- Keep it Maintainable
Avoid #ifdefs where possible
Signed-off-by: Alexey Brodkin abrodkin@synopsys.com
Cc: Mischa Jonker mjonker@synopsys.com Cc: Jaehoon Chung jh80.chung@samsung.com Cc: Andy Fleming afleming@gmail.com Cc: Alim Akhtar alim.akhtar@samsung.com Cc: Rajeshwari Shinde rajeshwari.s@samsung.com Cc: Simon Glass sjg@chromium.org Cc: Pantelis Antoniou panto@antoniou-consulting.com
Changes compared to initial version: Instead of modification of headers only disable some parts of code with ifdefs.
drivers/mmc/dw_mmc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 1e0f72b..fdae40b 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -11,7 +11,9 @@ #include <mmc.h> #include <dwmmc.h> #include <asm-generic/errno.h> +#ifdef CONFIG_EXYNOS5420 #include <asm/arch/dwmmc.h> +#endif
Just remove this.
#define PAGE_SIZE 4096
@@ -302,6 +304,7 @@ static int dwmci_init(struct mmc *mmc) struct dwmci_host *host = (struct dwmci_host *)mmc->priv; u32 fifo_size;
+#ifdef CONFIG_EXYNOS5420 if (host->quirks & DWMCI_QUIRK_DISABLE_SMU) { dwmci_writel(host, EMMCP_MPSBEGIN0, 0); dwmci_writel(host, EMMCP_SEND0, 0); @@ -311,6 +314,7 @@ static int dwmci_init(struct mmc *mmc) MPSCTRL_NON_SECURE_READ_BIT | MPSCTRL_NON_SECURE_WRITE_BIT | MPSCTRL_VALID); } +#endif
remove this and use callback function like the host->board_init()? if (host->board_init()) host->board_init(host);
Then we can reuse the this function for board specific code. Anyway, I think that exynos code didn't exist at here(dw-mmc.c).
Best Regards, Jaehoon Chung
dwmci_writel(host, DWMCI_PWREN, 1);
On Fri, 2013-11-29 at 19:00 +0900, Jaehoon Chung wrote:
remove this and use callback function like the host->board_init()? if (host->board_init()) host->board_init(host);
Then we can reuse the this function for board specific code. Anyway, I think that exynos code didn't exist at here(dw-mmc.c).
Best Regards, Jaehoon Chung
dwmci_writel(host, DWMCI_PWREN, 1);
I may do this change but I don't own corresponding HW (Exynos-5420 based) so I won't be able to test my changes.
And that's why I propose to apply this patch so driver is now again gets built for any platform and if anybody who have access to Exynos-5420 HW have some time to clean-up then it's good, anyways people will be happy again :)
-Alexey
P.S. I saw you reply on v1 patch you just sent - so feel free to clean-up the driver if it's ok for you or let's keep my "work-around" for now.
Hi Alexey,
On Nov 29, 2013, at 12:08 PM, Alexey Brodkin wrote:
On Fri, 2013-11-29 at 19:00 +0900, Jaehoon Chung wrote:
remove this and use callback function like the host->board_init()? if (host->board_init()) host->board_init(host);
Then we can reuse the this function for board specific code. Anyway, I think that exynos code didn't exist at here(dw-mmc.c).
Best Regards, Jaehoon Chung
dwmci_writel(host, DWMCI_PWREN, 1);
I may do this change but I don't own corresponding HW (Exynos-5420 based) so I won't be able to test my changes.
And that's why I propose to apply this patch so driver is now again gets built for any platform and if anybody who have access to Exynos-5420 HW have some time to clean-up then it's good, anyways people will be happy again :)
-Alexey
P.S. I saw you reply on v1 patch you just sent - so feel free to clean-up the driver if it's ok for you or let's keep my "work-around" for now.
All looks good from my side;
Please do a boot test on the real hardware and let me know before I can apply.
Regards
-- Pantelis
Hi All,
Iam a bit confused here. After Jaehoon Chung has submitted the following patch: "https://www.mail-archive.com/u-boot@lists.denx.de/msg126921.html"
Do we need to include this patch also?
Regards, Rajeshwari Shinde.
On Fri, Nov 29, 2013 at 5:46 PM, Alexey Brodkin Alexey.Brodkin@synopsys.com wrote:
All looks good from my side;
Please do a boot test on the real hardware and let me know before I can apply.
Hi Pantelis,
confirming - builds/works good for me.
-Alexey _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi, Rajeshwari
It didn't need to include this patch.
Best Regards, Jaehoon Chung
On 12/02/2013 02:39 PM, Rajeshwari Birje wrote:
Hi All,
Iam a bit confused here. After Jaehoon Chung has submitted the following patch: "https://www.mail-archive.com/u-boot@lists.denx.de/msg126921.html"
Do we need to include this patch also?
Regards, Rajeshwari Shinde.
On Fri, Nov 29, 2013 at 5:46 PM, Alexey Brodkin Alexey.Brodkin@synopsys.com wrote:
All looks good from my side;
Please do a boot test on the real hardware and let me know before I can apply.
Hi Pantelis,
confirming - builds/works good for me.
-Alexey _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On Mon, 2013-12-02 at 14:45 +0900, Jaehoon Chung wrote:
Hi, Rajeshwari
It didn't need to include this patch.
Best Regards, Jaehoon Chung
Agree, there's no need in this particular patch any more. Jaehoon Chung did appropriate changes in dwmmc driver.
Thanks for collaboration.
-Alexey
participants (4)
-
Alexey Brodkin -
Jaehoon Chung -
Pantelis Antoniou -
Rajeshwari Birje