[U-Boot] [PATCH v2] dwmmc: make driver usable for non-exynos platforms

[U-Boot] [PATCH v2] dwmmc: make driver usable for non-exynos platforms

[Alexey Brodkin]

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);
 
-- 
1.8.4.2

[U-Boot] [PATCH v2] dwmmc: make driver usable for non-exynos platforms

[Jaehoon Chung]

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:
> =======
> 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
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);
>  
> 

[U-Boot] [PATCH v2] dwmmc: make driver usable for non-exynos platforms

[Alexey Brodkin]

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.

[U-Boot] [PATCH v2] dwmmc: make driver usable for non-exynos platforms

[Pantelis Antoniou]

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

[U-Boot] [PATCH v2] dwmmc: make driver usable for non-exynos platforms

[Alexey Brodkin]

> 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] [PATCH v2] dwmmc: make driver usable for non-exynos platforms

[Rajeshwari Birje]

Hi All,

Iam a bit confused here.
After Jaehoon Chung has submitted the following patch:
"https://www.mail-archive.com/u-boot at 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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Regards,
Rajeshwari Shinde

[U-Boot] [PATCH v2] dwmmc: make driver usable for non-exynos platforms

[Jaehoon Chung]

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 at 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 at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
> 
> 
> 

[U-Boot] [PATCH v2] dwmmc: make driver usable for non-exynos platforms

[Alexey Brodkin]

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