EXYNOS5: Enable DWMMC, add FDT support for DWMMC and

This patch adds DWMMC device node data for exynos5. This patch also adds binding file for DWMMC device node.

Changes from V1:
1)Added binding file for DWMMC device node at the location "doc/device-tree-bindings/exynos/dwmmc.txt".
2)Removed the propname 'index' from device node. 3)Prefixed the vendor name 'samsung' before propname in device node.

Changes from V2:
1)Updation of commit message and resubmition of proper patch set.

Changes from V3:
No change.

Signed-off-by Vivek Gautam <[email protected]>

Signed-off-by Amar <[email protected]>X

---
 arch/arm/dts/exynos5250.dtsi              | 32 +++++++++++++++++++++++++++++++
 board/samsung/dts/exynos5250-smdk5250.dts | 22 +++++++++++++++++++++
 doc/device-tree-bindings/exynos/dwmmc.txt | 29 ++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+)
 create mode 100644 doc/device-tree-bindings/exynos/dwmmc.txt

arch/arm/dts/exynos5250.dtsi +32 -0

index 1008797..b701ae5 100644
@@ -138,4 +138,36 @@
                reg = <0x131b0000 0x30>;
                interrupts = <0 130 0>;
        };
+
+       dwmmc at 12200000 {
+               #address-cells = <1>;
+               #size-cells = <0>;
+               compatible = "samsung,exynos5250-dwmmc";
+               reg = <0x12200000 0x1000>;
+               interrupts = <0 75 0>;
+       };
+
+       dwmmc at 12210000 {
+               #address-cells = <1>;
+               #size-cells = <0>;
+               compatible = "samsung,exynos5250-dwmmc";
+               reg = <0x12210000 0x1000>;
+               interrupts = <0 76 0>;
+       };
+
+       dwmmc at 12220000 {
+               #address-cells = <1>;
+               #size-cells = <0>;
+               compatible = "samsung,exynos5250-dwmmc";
+               reg = <0x12220000 0x1000>;
+               interrupts = <0 77 0>;
+       };
+
+       dwmmc at 12230000 {
+               #address-cells = <1>;
+               #size-cells = <0>;
+               compatible = "samsung,exynos5250-dwmmc";
+               reg = <0x12230000 0x1000>;
+               interrupts = <0 78 0>;
+       };
 };

board/samsung/dts/exynos5250-smdk5250.dts +0 -0

index a8e62da..9baf622 100644

board/samsung/dts/exynos5250-smdk5250.dts +18 -0

        };
 
        sromc at 12250000 {

@@ -59,4 +63,22 @@
                        compatible = "wolfson,wm8994-codec";
                };
        };
+
+       dwmmc at 12200000 {
+               samsung,bus-width = <8>;
+               samsung,timing = <1 3 3>;
+       };
+
+       dwmmc at 12210000 {
+               status = "disabled";
+       };
+
+       dwmmc at 12220000 {
+               samsung,bus-width = <4>;
+               samsung,timing = <1 2 3>;
+       };
+
+       dwmmc at 12230000 {
+               status = "disabled";
+       };
 };

doc/device-tree-bindings/exynos/dwmmc.txt new file +17 -0

new file mode 100644
index 0000000..6232ad6
@@ -0,0 +1,29 @@
+* Exynos 5250 DWC_mobile_storage
+
+The Exynos 5250 provides DWC_mobile_storage interface which supports
+. Embedded Multimedia Cards (EMMC-version 4.5)
+. Secure Digital memory (SD mem-version 2.0)
+. Secure Digital I/O (SDIO-version 3.0)
+. Consumer Electronics Advanced Transport Architecture (CE-ATA-version 1.1)
+
+The Exynos 5250 DWC_mobile_storage provides four channels.
+SOC specific and Board specific properties are channel specific.
+
+Required SoC Specific Properties:
+
+- compatible: should be
+       - samsung,exynos5250-dwmmc: for exynos5250 platforms
+
+- reg: physical base address of the controller and length of memory mapped

--
1.8.0

Hi Amar,

Quoted 8 lines · Amar

This patch adds DWMMC device node data for exynos5. This patch also adds binding file for DWMMC device node.

Changes from V1:
1)Added binding file for DWMMC device node at the location "doc/device-tree-bindings/exynos/dwmmc.txt". 2)Removed the propname 'index' from device node. 3)Prefixed the vendor name 'samsung' before propname in device

node.

Quoted 6 lines

Changes from V2:
1)Updation of commit message and resubmition of proper patch set.

Changes from V3:
No change.

Sorry I may be too late with this comment.

Quoted 5 lines

Signed-off-by Vivek Gautam <[email protected]>

Signed-off-by Amar <[email protected]>X

---
 arch/arm/dts/exynos5250.dtsi              | 32

+++++++++++++++++++++++++++++++

Quoted 2 lines

 board/samsung/dts/exynos5250-smdk5250.dts | 22 +++++++++++++++++++++
 doc/device-tree-bindings/exynos/dwmmc.txt | 29

++++++++++++++++++++++++++++

Review

Quoted 45 lines

 3 files changed, 83 insertions(+)
 create mode 100644 doc/device-tree-bindings/exynos/dwmmc.txt

arch/arm/dts/exynos5250.dtsi +32 -0

index 1008797..b701ae5 100644
@@ -138,4 +138,36 @@
                reg = <0x131b0000 0x30>;
                interrupts = <0 130 0>;
        };
+
+       dwmmc at 12200000 {
+               #address-cells = <1>;
+               #size-cells = <0>;
+               compatible = "samsung,exynos5250-dwmmc";
+               reg = <0x12200000 0x1000>;
+               interrupts = <0 75 0>;
+       };
+
+       dwmmc at 12210000 {
+               #address-cells = <1>;
+               #size-cells = <0>;
+               compatible = "samsung,exynos5250-dwmmc";
+               reg = <0x12210000 0x1000>;
+               interrupts = <0 76 0>;
+       };
+
+       dwmmc at 12220000 {
+               #address-cells = <1>;
+               #size-cells = <0>;
+               compatible = "samsung,exynos5250-dwmmc";
+               reg = <0x12220000 0x1000>;
+               interrupts = <0 77 0>;
+       };
+
+       dwmmc at 12230000 {
+               #address-cells = <1>;
+               #size-cells = <0>;
+               compatible = "samsung,exynos5250-dwmmc";
+               reg = <0x12230000 0x1000>;
+               interrupts = <0 78 0>;
+       };
 };

(quoted diff) +0 -0

b/board/samsung/dts/exynos5250-smdk5250.dts

board/samsung/dts/exynos5250-smdk5250.dts line 36

Quoted 11 lines

index a8e62da..9baf622 100644

board/samsung/dts/exynos5250-smdk5250.dts +4 -0

@@ -30,6 +30,10 @@
                spi2 = "/spi at 12d40000";
                spi3 = "/spi at 131a0000";
                spi4 = "/spi at 131b0000";
+               dwmmc0 = "/dwmmc at 12200000";
+               dwmmc1 = "/dwmmc at 12210000";
+               dwmmc2 = "/dwmmc at 12220000";
+               dwmmc3 = "/dwmmc at 12230000";

I think this should be mmc0, mmc1 instead of dwmmc0, dwmmc1, since ultimate we might want to support different drivers for each. The alias is support to link the generic mmc device number with the driver, and I don't think the numbering should be specific to the driver.

Ok.

Review

Quoted 27 lines

board/samsung/dts/exynos5250-smdk5250.dts +18 -0

        };
 
        sromc at 12250000 {

@@ -59,4 +63,22 @@
                        compatible = "wolfson,wm8994-codec";
                };
        };
+
+       dwmmc at 12200000 {
+               samsung,bus-width = <8>;
+               samsung,timing = <1 3 3>;
+       };
+
+       dwmmc at 12210000 {
+               status = "disabled";
+       };
+
+       dwmmc at 12220000 {
+               samsung,bus-width = <4>;
+               samsung,timing = <1 2 3>;
+       };
+
+       dwmmc at 12230000 {
+               status = "disabled";
+       };
 };

(quoted diff) +0 -0

b/doc/device-tree-bindings/exynos/dwmmc.txt

doc/device-tree-bindings/exynos/dwmmc.txt line 7

Quoted 12 lines

new file mode 100644
index 0000000..6232ad6

doc/device-tree-bindings/exynos/dwmmc.txt new file +7 -0

@@ -0,0 +1,29 @@
+* Exynos 5250 DWC_mobile_storage
+
+The Exynos 5250 provides DWC_mobile_storage interface which supports
+. Embedded Multimedia Cards (EMMC-version 4.5)
+. Secure Digital memory (SD mem-version 2.0)
+. Secure Digital I/O (SDIO-version 3.0)
+. Consumer Electronics Advanced Transport Architecture (CE-ATA-version

1.1)

doc/device-tree-bindings/exynos/dwmmc.txt

Quoted 10 lines

doc/device-tree-bindings/exynos/dwmmc.txt new file +10 -0

+
+The Exynos 5250 DWC_mobile_storage provides four channels.
+SOC specific and Board specific properties are channel specific.
+
+Required SoC Specific Properties:
+
+- compatible: should be
+       - samsung,exynos5250-dwmmc: for exynos5250 platforms
+
+- reg: physical base address of the controller and length of memory

mapped

doc/device-tree-bindings/exynos/dwmmc.txt line 27

Quoted 10 lines

doc/device-tree-bindings/exynos/dwmmc.txt new file +10 -0

+       region.
+
+- interrupts: The interrupt number to the cpu.
+
+Required Board Specific Properties:
+
+- #address-cells: should be 1.
+- #size-cells: should be 0.
+- samsung,bus-width: The width of the bus used to interface the devices
+       supported by DWC_mobile_storage (SD-MMC/EMMC/SDIO).

typically 4 or 8
Ok. I will mention this.

doc/device-tree-bindings/exynos/dwmmc.txt line 29

Quoted 2 lines

doc/device-tree-bindings/exynos/dwmmc.txt new file +2 -0

+- samsung,timing: The timing values to be written into the
+       Drv/sample clock selection register of corresponding channel.

Please add a bit more detail here - there are 3 values - what do they mean?

Ok

You should add an example for your binding (something will illustrates the binding).

Ok

Also does the kernel use the same binding?

The uboot binding is a subset of kernel binding.

Regards,
Simon

Quoted 3 lines

--
1.8.0

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

This patch enumerates dwmci and set auto stop command during dwmci initialisation.
EMMC read/write is not happening in current implementation due to improper fifo size computation. Hence Modified the fifo size computation to resolve EMMC read write issues.

Changes from V1:
1)Created the macros RX_WMARK_SHIFT and RX_WMARK_MASK in header file.

Changes from V2:
1)Updation of commit message and resubmition of proper patch set.

Changes from V3:
1)Updated to use the macro DWMCI_CTRL_SEND_AS_CCSD instead of the hard coded value (1 << 10).

Signed-off-by Amar <[email protected]>

---
 drivers/mmc/dw_mmc.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

drivers/mmc/dw_mmc.c +0 -0

index 4070d4e..776fdb6 100644

drivers/mmc/dw_mmc.c +9 -1

        }
 
        dwmci_writel(host, DWMCI_RINTSTS, DWMCI_INTMSK_ALL);

@@ -314,7 +315,7 @@ static void dwmci_set_ios(struct mmc *mmc)
 static int dwmci_init(struct mmc *mmc)
 {
        struct dwmci_host *host = (struct dwmci_host *)mmc->priv;
-       u32 fifo_size, fifoth_val;
+       u32 fifo_size, fifoth_val, ier;
 
        dwmci_writel(host, DWMCI_PWREN, 1);
 

@@ -323,6 +324,14 @@ static int dwmci_init(struct mmc *mmc)
                return -1;
        }
 
+       /* Enumerate at 400KHz */
+       dwmci_setup_bus(host, mmc->f_min);
+
+       /* Set auto stop command */
+       ier = dwmci_readl(host, DWMCI_CTRL);
+       ier |= DWMCI_CTRL_SEND_AS_CCSD;
+       dwmci_writel(host, DWMCI_CTRL, ier);
+
        dwmci_writel(host, DWMCI_RINTSTS, 0xFFFFFFFF);
        dwmci_writel(host, DWMCI_INTMASK, 0);
 

drivers/mmc/dw_mmc.c +0 -1

        dwmci_writel(host, DWMCI_FIFOTH, fifoth_val);
 
--

1.8.0

Hi Amar,

Quoted 9 lines · Amar

This patch enumerates dwmci and set auto stop command during dwmci initialisation.
EMMC read/write is not happening in current implementation due to improper fifo size computation. Hence Modified the fifo size computation to resolve EMMC read write issues.

Changes from V1:
1)Created the macros RX_WMARK_SHIFT and RX_WMARK_MASK in header file.

Quoted 6 lines

Changes from V2:
1)Updation of commit message and resubmition of proper patch set.

Changes from V3:
1)Updated to use the macro DWMCI_CTRL_SEND_AS_CCSD instead of the hard coded value (1 << 10).

I suggest you take a look at patman which might simplify your patch sending and change logs - see tools/patman/README for details.

Quoted 6 lines

Signed-off-by Amar <[email protected]>

---
 drivers/mmc/dw_mmc.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

drivers/mmc/dw_mmc.c static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, · line 139

Quoted 9 lines

drivers/mmc/dw_mmc.c +1 -0

index 4070d4e..776fdb6 100644
@@ -136,6 +136,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
                        return TIMEOUT;
                }
                timeout--;
+               mdelay(1);

How long will this delay in total?

Quoted 29 lines

drivers/mmc/dw_mmc.c +9 -1

        }
 
        dwmci_writel(host, DWMCI_RINTSTS, DWMCI_INTMSK_ALL);

@@ -314,7 +315,7 @@ static void dwmci_set_ios(struct mmc *mmc)
 static int dwmci_init(struct mmc *mmc)
 {
        struct dwmci_host *host = (struct dwmci_host *)mmc->priv;
-       u32 fifo_size, fifoth_val;
+       u32 fifo_size, fifoth_val, ier;
 
        dwmci_writel(host, DWMCI_PWREN, 1);
 

@@ -323,6 +324,14 @@ static int dwmci_init(struct mmc *mmc)
                return -1;
        }
 
+       /* Enumerate at 400KHz */
+       dwmci_setup_bus(host, mmc->f_min);
+
+       /* Set auto stop command */
+       ier = dwmci_readl(host, DWMCI_CTRL);
+       ier |= DWMCI_CTRL_SEND_AS_CCSD;
+       dwmci_writel(host, DWMCI_CTRL, ier);
+
        dwmci_writel(host, DWMCI_RINTSTS, 0xFFFFFFFF);
        dwmci_writel(host, DWMCI_INTMASK, 0);
 

drivers/mmc/dw_mmc.c static int dwmci_init(struct mmc *mmc) · line 349

Quoted 10 lines

drivers/mmc/dw_mmc.c +2 -1

@@ -332,10 +341,11 @@ static int dwmci_init(struct mmc *mmc)
        dwmci_writel(host, DWMCI_BMOD, 1);
 
        fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
+       fifo_size = ((fifo_size & RX_WMARK_MASK) >> RX_WMARK_SHIFT) + 1;
        if (host->fifoth_val)
                fifoth_val = host->fifoth_val;
        else
-               fifoth_val = MSIZE(0x2) | RX_WMARK(fifo_size/2 -1) |
+               fifoth_val = MSIZE(0x2) | RX_WMARK(fifo_size/2 - 1) |
                        TX_WMARK(fifo_size/2);

{} around this else I think. Also space around /

Quoted 5 lines

drivers/mmc/dw_mmc.c +0 -1

        dwmci_writel(host, DWMCI_FIFOTH, fifoth_val);
 
--

1.8.0

Regards,
Simon

Quoted 28 lines · Simon Glass

Hi Amar,

Quoted 9 lines · Amar

This patch enumerates dwmci and set auto stop command during dwmci initialisation.
EMMC read/write is not happening in current implementation due to improper fifo size computation. Hence Modified the fifo size computation to resolve EMMC read write issues.

Changes from V1:
1)Created the macros RX_WMARK_SHIFT and RX_WMARK_MASK in header file.

Quoted 6 lines

Changes from V2:
1)Updation of commit message and resubmition of proper patch set.

Changes from V3:
1)Updated to use the macro DWMCI_CTRL_SEND_AS_CCSD instead of the hard coded value (1 << 10).

I suggest you take a look at patman which might simplify your patch sending and change logs - see tools/patman/README for details.

Quoted 6 lines

Signed-off-by Amar <[email protected]>

---
 drivers/mmc/dw_mmc.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Quoted 11 lines

drivers/mmc/dw_mmc.c static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, · line 139

Quoted 9 lines

drivers/mmc/dw_mmc.c +1 -0

index 4070d4e..776fdb6 100644
@@ -136,6 +136,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
                        return TIMEOUT;
                }
                timeout--;
+               mdelay(1);

How long will this delay in total?

i didn't sure why add the mdelay(1)..i think mdelay(1) is too long. Isn't there other approach to resolve read/write issue?

Best Regards,
Jaehoon Chung

Quoted 11 lines · Simon Glass

Hi Amar,

Quoted 8 lines · Amar

This patch enumerates dwmci and set auto stop command during dwmci initialisation.
EMMC read/write is not happening in current implementation due to improper fifo size computation. Hence Modified the fifo size computation to resolve EMMC read write issues.

Changes from V1:
1)Created the macros RX_WMARK_SHIFT and RX_WMARK_MASK in header

file.

Quoted 3 lines

Quoted 3 lines

Changes from V2:
1)Updation of commit message and resubmition of proper patch

set.

Quoted 14 lines

Quoted 4 lines

Changes from V3:
1)Updated to use the macro DWMCI_CTRL_SEND_AS_CCSD instead of the hard coded value (1 << 10).

I suggest you take a look at patman which might simplify your patch sending and change logs - see tools/patman/README for details.

Quoted 6 lines

Signed-off-by Amar <[email protected]>

---
 drivers/mmc/dw_mmc.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Quoted 5 lines

drivers/mmc/dw_mmc.c static int dwmci_send_cmd(struct mmc *mmc, struct

Quoted 5 lines

drivers/mmc/dw_mmc.c +0 -0

index 4070d4e..776fdb6 100644
@@ -136,6 +136,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct

mmc_cmd *cmd,

Quoted 6 lines

drivers/mmc/dw_mmc.c static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, · line 139

Quoted 4 lines

drivers/mmc/dw_mmc.c +1 -0

                        return TIMEOUT;
                }
                timeout--;
+               mdelay(1);

How long will this delay in total?

i didn't sure why add the mdelay(1)..i think mdelay(1) is too long. Isn't there other approach to resolve read/write issue?

Best Regards,
Jaehoon Chung

Quoted 42 lines

Quoted 29 lines

drivers/mmc/dw_mmc.c +9 -1

        }
 
        dwmci_writel(host, DWMCI_RINTSTS, DWMCI_INTMSK_ALL);

@@ -314,7 +315,7 @@ static void dwmci_set_ios(struct mmc *mmc)
 static int dwmci_init(struct mmc *mmc)
 {
        struct dwmci_host *host = (struct dwmci_host *)mmc->priv;
-       u32 fifo_size, fifoth_val;
+       u32 fifo_size, fifoth_val, ier;
 
        dwmci_writel(host, DWMCI_PWREN, 1);
 

@@ -323,6 +324,14 @@ static int dwmci_init(struct mmc *mmc)
                return -1;
        }
 
+       /* Enumerate at 400KHz */
+       dwmci_setup_bus(host, mmc->f_min);
+
+       /* Set auto stop command */
+       ier = dwmci_readl(host, DWMCI_CTRL);
+       ier |= DWMCI_CTRL_SEND_AS_CCSD;
+       dwmci_writel(host, DWMCI_CTRL, ier);
+
        dwmci_writel(host, DWMCI_RINTSTS, 0xFFFFFFFF);
        dwmci_writel(host, DWMCI_INTMASK, 0);
 

drivers/mmc/dw_mmc.c static int dwmci_init(struct mmc *mmc) · line 349

Quoted 10 lines

drivers/mmc/dw_mmc.c +2 -1

@@ -332,10 +341,11 @@ static int dwmci_init(struct mmc *mmc)
        dwmci_writel(host, DWMCI_BMOD, 1);
 
        fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
+       fifo_size = ((fifo_size & RX_WMARK_MASK) >> RX_WMARK_SHIFT) + 1;
        if (host->fifoth_val)
                fifoth_val = host->fifoth_val;
        else
-               fifoth_val = MSIZE(0x2) | RX_WMARK(fifo_size/2 -1) |
+               fifoth_val = MSIZE(0x2) | RX_WMARK(fifo_size/2 - 1) |
                        TX_WMARK(fifo_size/2);

{} around this else I think. Also space around /

Quoted 9 lines

Quoted 5 lines

drivers/mmc/dw_mmc.c +0 -1

        dwmci_writel(host, DWMCI_FIFOTH, fifoth_val);
 
--

1.8.0

Regards,
Simon

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

This patch adds FDT support for DWMMC, by reading the DWMMC node data from the device tree and initialising DWMMC channels as per data obtained from the node.

Changes from V1:
1)Updated code to have same signature for the function exynos_dwmci_init() for both FDT and non-FDT versions. 2)Updated code to pass device_id parameter to the function exynos5_mmc_set_clk_div() instead of index. 3)Updated code to decode the value of "samsung,width" from FDT. 4)Channel index is computed instead of getting from FDT.

Changes from V2:
1)Updation of commit message and resubmition of proper patch set.

Changes from V3:
1)Replaced the new function exynos5_mmc_set_clk_div() with the existing function set_mmc_clk(). set_mmc_clk() will do the purpose. 2)Computation of FSYS block clock divisor (pre-ratio) is added.

Signed-off-by Vivek Gautam <[email protected]>

Signed-off-by Amar <[email protected]>

---
 arch/arm/include/asm/arch-exynos/dwmmc.h |   4 +
 drivers/mmc/exynos_dw_mmc.c              | 129 +++++++++++++++++++++++++++++--
 include/dwmmc.h                          |   4 +
 3 files changed, 130 insertions(+), 7 deletions(-)

arch/arm/include/asm/arch-exynos/dwmmc.h +0 -0

index 8acdf9b..40dcc7b 100644

arch/arm/include/asm/arch-exynos/dwmmc.h +1 -0

 {
        unsigned int base = samsung_get_base_mmc() + (0x10000 * index);
        return exynos_dwmci_init(base, bus_width, index);
 }
+#endif

drivers/mmc/exynos_dw_mmc.c +3 -1

index 72a31b7..d7ca7d0 100644
@@ -19,39 +19,154 @@
  */
 
 #include <common.h>
-#include <malloc.h>
 #include <dwmmc.h>
+#include <fdtdec.h>
+#include <libfdt.h>
+#include <malloc.h>
 #include <asm/arch/dwmmc.h>

drivers/mmc/exynos_dw_mmc.c +16 -4

+u32 timing[3];
 
+/*
+ * Function used as callback function to initialise the
+ * CLKSEL register for every mmc channel.
+ */
 static void exynos_dwmci_clksel(struct dwmci_host *host)
 {
-       u32 val;
-       val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
-               DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(0);
+       dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);
+}
 
-       dwmci_writel(host, DWMCI_CLKSEL, val);
+unsigned int exynos_dwmci_get_clk(int dev_index)
+{
+       return get_mmc_clk(dev_index);
 }
 
 int exynos_dwmci_init(u32 regbase, int bus_width, int index)
 {
        struct dwmci_host *host = NULL;
+       unsigned int clock, div;
        host = malloc(sizeof(struct dwmci_host));
        if (!host) {
                printf("dwmci_host malloc fail!\n");
                return 1;
        }
 
+       /*
+        * The max operating freq of FSYS block is 400MHz.
+        * Scale down the 400MHz to number 400.
+        * Scale down the MPLL clock by dividing MPLL_CLK with ONE_MEGA_HZ.
+        * Arrive at the divisor value taking 400 as the reference.

drivers/mmc/exynos_dw_mmc.c +16 -0

        return 0;
 }
 
+#ifdef CONFIG_OF_CONTROL
+unsigned int exynos_dwmmc_init(const void *blob)
+{
+       u32 base;
+       int index, bus_width;
+       int node_list[DWMMC_MAX_CH_NUM];
+       int err = 0;
+       int dev_id, flag;
+       int count, i;
+
+       count = fdtdec_find_aliases_for_id(blob, "dwmmc",
+                               COMPAT_SAMSUNG_EXYNOS5_DWMMC, node_list,
+                               DWMMC_MAX_CH_NUM);
+
+       for (i = 0; i < count; i++) {
+               int node = node_list[i];

drivers/mmc/exynos_dw_mmc.c +41 -0

+                       debug("DWMMC: Can't get bus-width\n");
+                       return -1;
+               }
+               if (8 == bus_width)
+                       flag = PINMUX_FLAG_8BIT_MODE;
+               else
+                       flag = PINMUX_FLAG_NONE;
+
+               /* config pinmux for each mmc channel */
+               err = exynos_pinmux_config(dev_id, flag);
+               if (err) {
+                       debug("DWMMC not configured\n");
+                       return err;
+               }
+
+               index = dev_id - PERIPH_ID_SDMMC0;
+
+               /* Get the base address from the device node */
+               base = fdtdec_get_addr(blob, node, "reg");
+               if (!base) {
+                       debug("DWMMC: Can't get base address\n");
+                       return -1;
+               }
+               /* Extract the timing info from the node */
+               err = fdtdec_get_int_array(blob, node, "samsung,timing",
+                                       timing, 3);
+               if (err) {
+                       debug("Can't get sdr-timings for divider\n");
+                       return -1;
+               }
+               /* Initialise each mmc channel */
+               err =  exynos_dwmci_init(base, bus_width, index);
+               if (err) {
+                       debug("Can't do dwmci init\n");
+                       return -1;
+               }
+       }
+
+       return 0;
+}
+#endif

include/dwmmc.h +0 -0

index c8b1d40..4a42849 100644

include/dwmmc.h +1 -1

 #define DWMCI_IDMAC_OWN                (1 << 31)
 #define DWMCI_IDMAC_CH         (1 << 4)

@@ -144,6 +147,7 @@ struct dwmci_host {
        unsigned int bus_hz;
        int dev_index;
        int buswidth;
+       u32 clksel_val;
        u32 fifoth_val;
        struct mmc *mmc;
 
--

1.8.0

Hi Amar,

Quoted 33 lines · Amar

This patch adds FDT support for DWMMC, by reading the DWMMC node data from the device tree and initialising DWMMC channels as per data obtained from the node.

Changes from V1:
1)Updated code to have same signature for the function exynos_dwmci_init() for both FDT and non-FDT versions. 2)Updated code to pass device_id parameter to the function exynos5_mmc_set_clk_div() instead of index. 3)Updated code to decode the value of "samsung,width" from FDT. 4)Channel index is computed instead of getting from FDT.

Changes from V2:
1)Updation of commit message and resubmition of proper patch set.

Changes from V3:
1)Replaced the new function exynos5_mmc_set_clk_div() with the existing function set_mmc_clk(). set_mmc_clk() will do the purpose. 2)Computation of FSYS block clock divisor (pre-ratio) is added.

Signed-off-by Vivek Gautam <[email protected]>

Signed-off-by Amar <[email protected]>

---
 arch/arm/include/asm/arch-exynos/dwmmc.h |   4 +
 drivers/mmc/exynos_dw_mmc.c              | 129 +++++++++++++++++++++++++++++--
 include/dwmmc.h                          |   4 +
 3 files changed, 130 insertions(+), 7 deletions(-)

arch/arm/include/asm/arch-exynos/dwmmc.h +0 -0

index 8acdf9b..40dcc7b 100644

arch/arm/include/asm/arch-exynos/dwmmc.h line 35

Quoted 7 lines

arch/arm/include/asm/arch-exynos/dwmmc.h +3 -0

@@ -29,8 +29,12 @@
 
 int exynos_dwmci_init(u32 regbase, int bus_width, int index);
 
+#ifdef CONFIG_OF_CONTROL
+unsigned int exynos_dwmmc_init(const void *blob);
+#else
 static inline unsigned int exynos_dwmmc_init(int index, int bus_width)

Why unsigned?

I'm really not that keen on functions which change their signature based on an #ifdef. Can we perhaps have

int exynos_dwmmc_init(const void *blob);

which will pass NULL when there is no FDT, and

int exynos_dwmmc_add_port(int index, int bus_width)

for use by non-FDT boards?

Quoted 24 lines

arch/arm/include/asm/arch-exynos/dwmmc.h +1 -0

 {
        unsigned int base = samsung_get_base_mmc() + (0x10000 * index);
        return exynos_dwmci_init(base, bus_width, index);
 }
+#endif

drivers/mmc/exynos_dw_mmc.c +7 -1

index 72a31b7..d7ca7d0 100644
@@ -19,39 +19,154 @@
  */
 
 #include <common.h>
-#include <malloc.h>
 #include <dwmmc.h>
+#include <fdtdec.h>
+#include <libfdt.h>
+#include <malloc.h>
 #include <asm/arch/dwmmc.h>
 #include <asm/arch/clk.h>
+#include <asm/arch/pinmux.h>
+
+#define        DWMMC_MAX_CH_NUM                4
+#define        DWMMC_MAX_FREQ                  52000000

drivers/mmc/exynos_dw_mmc.c line 36

Quoted 5 lines

drivers/mmc/exynos_dw_mmc.c +5 -0

+#define        DWMMC_MIN_FREQ                  400000
+#define        DWMMC_MMC0_CLKSEL_VAL           0x03030001
+#define        DWMMC_MMC2_CLKSEL_VAL           0x03020001
+#define        ONE_MEGA_HZ                     1000000
+#define        SCALED_VAL_FOUR_HUNDRED         400

I don't think you need these last two - you can just write the number in the code

drivers/mmc/exynos_dw_mmc.c line 38

Quoted 3 lines

drivers/mmc/exynos_dw_mmc.c +0 -0

 static char *EXYNOS_NAME = "EXYNOS DWMMC";

Same with this I think

Quoted 40 lines

drivers/mmc/exynos_dw_mmc.c +20 -4

+u32 timing[3];
 
+/*
+ * Function used as callback function to initialise the
+ * CLKSEL register for every mmc channel.
+ */
 static void exynos_dwmci_clksel(struct dwmci_host *host)
 {
-       u32 val;
-       val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
-               DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(0);
+       dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);
+}
 
-       dwmci_writel(host, DWMCI_CLKSEL, val);
+unsigned int exynos_dwmci_get_clk(int dev_index)
+{
+       return get_mmc_clk(dev_index);
 }
 
 int exynos_dwmci_init(u32 regbase, int bus_width, int index)
 {
        struct dwmci_host *host = NULL;
+       unsigned int clock, div;
        host = malloc(sizeof(struct dwmci_host));
        if (!host) {
                printf("dwmci_host malloc fail!\n");
                return 1;
        }
 
+       /*
+        * The max operating freq of FSYS block is 400MHz.
+        * Scale down the 400MHz to number 400.
+        * Scale down the MPLL clock by dividing MPLL_CLK with ONE_MEGA_HZ.
+        * Arrive at the divisor value taking 400 as the reference.
+        */
+
+       /* get mpll clock and divide it by ONE_MEGA_HZ */
+       clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;

drivers/mmc/exynos_dw_mmc.c line 79

Quoted 6 lines

drivers/mmc/exynos_dw_mmc.c +6 -0

+
+       /* Arrive at the divisor value. */
+       for (div = 0; div <= 0xf; div++) {
+               if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
+                       break;
+       }

What if you don't find the right divisor?

drivers/mmc/exynos_dw_mmc.c line 90

Quoted 11 lines

drivers/mmc/exynos_dw_mmc.c +8 -0

+
+       /* set the clock divisor for mmc */
+       set_mmc_clk(index, div);
+
        host->name = EXYNOS_NAME;
        host->ioaddr = (void *)regbase;
        host->buswidth = bus_width;
+#ifdef CONFIG_OF_CONTROL
+       host->clksel_val = (DWMCI_SET_SAMPLE_CLK(timing[0]) |
+                               DWMCI_SET_DRV_CLK(timing[1]) |
+                               DWMCI_SET_DIV_RATIO(timing[2]));

timing should be a parameter, not a global. For the non-FDT case perhaps you can accept NULL, meaning default? Then:

if (timing)
do the code above
else
do the code below

drivers/mmc/exynos_dw_mmc.c line 101

Quoted 13 lines

drivers/mmc/exynos_dw_mmc.c +9 -2

+#else
+       if (0 == index)
+               host->clksel_val = DWMMC_MMC0_CLKSEL_VAL;
+       if (2 == index)
+               host->clksel_val = DWMMC_MMC2_CLKSEL_VAL;
+#endif
        host->clksel = exynos_dwmci_clksel;
        host->dev_index = index;
-
-       add_dwmci(host, 52000000, 400000);
+       host->mmc_clk = exynos_dwmci_get_clk;
+       /* Add the mmc chennel to be registered with mmc core */
+       add_dwmci(host, DWMMC_MAX_FREQ, DWMMC_MIN_FREQ);

Is error checking needed here?

Quoted 20 lines

drivers/mmc/exynos_dw_mmc.c +16 -0

        return 0;
 }
 
+#ifdef CONFIG_OF_CONTROL
+unsigned int exynos_dwmmc_init(const void *blob)
+{
+       u32 base;
+       int index, bus_width;
+       int node_list[DWMMC_MAX_CH_NUM];
+       int err = 0;
+       int dev_id, flag;
+       int count, i;
+
+       count = fdtdec_find_aliases_for_id(blob, "dwmmc",
+                               COMPAT_SAMSUNG_EXYNOS5_DWMMC, node_list,
+                               DWMMC_MAX_CH_NUM);
+
+       for (i = 0; i < count; i++) {
+               int node = node_list[i];

drivers/mmc/exynos_dw_mmc.c line 131

Quoted 10 lines

drivers/mmc/exynos_dw_mmc.c +10 -0

+
+               if (node <= 0)
+                       continue;
+
+               /* Extract device id for each mmc channel */
+               dev_id = pinmux_decode_periph_id(blob, node);
+
+               /* Get the bus width from the device node */
+               bus_width = fdtdec_get_int(blob, node, "samsung,bus-width", 0);
+               if (bus_width < 0) {

<= 0? The function will return 0 if there is no property.

Quoted 46 lines

drivers/mmc/exynos_dw_mmc.c +41 -0

+                       debug("DWMMC: Can't get bus-width\n");
+                       return -1;
+               }
+               if (8 == bus_width)
+                       flag = PINMUX_FLAG_8BIT_MODE;
+               else
+                       flag = PINMUX_FLAG_NONE;
+
+               /* config pinmux for each mmc channel */
+               err = exynos_pinmux_config(dev_id, flag);
+               if (err) {
+                       debug("DWMMC not configured\n");
+                       return err;
+               }
+
+               index = dev_id - PERIPH_ID_SDMMC0;
+
+               /* Get the base address from the device node */
+               base = fdtdec_get_addr(blob, node, "reg");
+               if (!base) {
+                       debug("DWMMC: Can't get base address\n");
+                       return -1;
+               }
+               /* Extract the timing info from the node */
+               err = fdtdec_get_int_array(blob, node, "samsung,timing",
+                                       timing, 3);
+               if (err) {
+                       debug("Can't get sdr-timings for divider\n");
+                       return -1;
+               }
+               /* Initialise each mmc channel */
+               err =  exynos_dwmci_init(base, bus_width, index);
+               if (err) {
+                       debug("Can't do dwmci init\n");
+                       return -1;
+               }
+       }
+
+       return 0;
+}
+#endif

include/dwmmc.h +0 -0

index c8b1d40..4a42849 100644

include/dwmmc.h line 128

Quoted 6 lines

include/dwmmc.h +3 -0

@@ -123,6 +123,9 @@
 #define MSIZE(x)               ((x) << 28)
 #define RX_WMARK(x)            ((x) << 16)
 #define TX_WMARK(x)            (x)
+#define RX_WMARK_SHIFT         16
+#define RX_WMARK_MASK          (0xfff << RX_WMARK_SHIFT)
+

Remove this extra line?

Quoted 12 lines

include/dwmmc.h +1 -1

 #define DWMCI_IDMAC_OWN                (1 << 31)
 #define DWMCI_IDMAC_CH         (1 << 4)

@@ -144,6 +147,7 @@ struct dwmci_host {
        unsigned int bus_hz;
        int dev_index;
        int buswidth;
+       u32 clksel_val;
        u32 fifoth_val;
        struct mmc *mmc;
 
--

Quoted 2 lines

1.8.0

Regards,
Simon

Quoted 81 lines · Simon Glass

Hi Amar,

Quoted 33 lines · Amar

This patch adds FDT support for DWMMC, by reading the DWMMC node data from the device tree and initialising DWMMC channels as per data obtained from the node.

Changes from V1:
1)Updated code to have same signature for the function exynos_dwmci_init() for both FDT and non-FDT versions. 2)Updated code to pass device_id parameter to the function exynos5_mmc_set_clk_div() instead of index. 3)Updated code to decode the value of "samsung,width" from FDT. 4)Channel index is computed instead of getting from FDT.

Changes from V2:
1)Updation of commit message and resubmition of proper patch set.

Changes from V3:
1)Replaced the new function exynos5_mmc_set_clk_div() with the existing function set_mmc_clk(). set_mmc_clk() will do the purpose. 2)Computation of FSYS block clock divisor (pre-ratio) is added.

Signed-off-by Vivek Gautam <[email protected]>

Signed-off-by Amar <[email protected]>

---
 arch/arm/include/asm/arch-exynos/dwmmc.h |   4 +
 drivers/mmc/exynos_dw_mmc.c              | 129 +++++++++++++++++++++++++++++--
 include/dwmmc.h                          |   4 +
 3 files changed, 130 insertions(+), 7 deletions(-)

arch/arm/include/asm/arch-exynos/dwmmc.h +0 -0

index 8acdf9b..40dcc7b 100644

arch/arm/include/asm/arch-exynos/dwmmc.h line 35

Quoted 7 lines

arch/arm/include/asm/arch-exynos/dwmmc.h +3 -0

@@ -29,8 +29,12 @@
 
 int exynos_dwmci_init(u32 regbase, int bus_width, int index);
 
+#ifdef CONFIG_OF_CONTROL
+unsigned int exynos_dwmmc_init(const void *blob);
+#else
 static inline unsigned int exynos_dwmmc_init(int index, int bus_width)

Why unsigned?

I'm really not that keen on functions which change their signature based on an #ifdef. Can we perhaps have

int exynos_dwmmc_init(const void *blob);

which will pass NULL when there is no FDT, and

int exynos_dwmmc_add_port(int index, int bus_width)

for use by non-FDT boards?

Quoted 24 lines

arch/arm/include/asm/arch-exynos/dwmmc.h +1 -0

 {
        unsigned int base = samsung_get_base_mmc() + (0x10000 * index);
        return exynos_dwmci_init(base, bus_width, index);
 }
+#endif

drivers/mmc/exynos_dw_mmc.c +7 -1

index 72a31b7..d7ca7d0 100644
@@ -19,39 +19,154 @@
  */
 
 #include <common.h>
-#include <malloc.h>
 #include <dwmmc.h>
+#include <fdtdec.h>
+#include <libfdt.h>
+#include <malloc.h>
 #include <asm/arch/dwmmc.h>
 #include <asm/arch/clk.h>
+#include <asm/arch/pinmux.h>
+
+#define        DWMMC_MAX_CH_NUM                4
+#define        DWMMC_MAX_FREQ                  52000000

Quoted 8 lines

drivers/mmc/exynos_dw_mmc.c line 36

Quoted 5 lines

drivers/mmc/exynos_dw_mmc.c +5 -0

+#define        DWMMC_MIN_FREQ                  400000
+#define        DWMMC_MMC0_CLKSEL_VAL           0x03030001
+#define        DWMMC_MMC2_CLKSEL_VAL           0x03020001
+#define        ONE_MEGA_HZ                     1000000
+#define        SCALED_VAL_FOUR_HUNDRED         400

I don't think you need these last two - you can just write the number in the code

Why didn't add into the dwmmc.h?

Quoted 5 lines

drivers/mmc/exynos_dw_mmc.c line 38

Quoted 3 lines

drivers/mmc/exynos_dw_mmc.c +0 -0

 static char *EXYNOS_NAME = "EXYNOS DWMMC";

Same with this I think

Sorry..What means? Also need not?

Quoted 40 lines

Quoted 40 lines

drivers/mmc/exynos_dw_mmc.c +20 -4

+u32 timing[3];
 
+/*
+ * Function used as callback function to initialise the
+ * CLKSEL register for every mmc channel.
+ */
 static void exynos_dwmci_clksel(struct dwmci_host *host)
 {
-       u32 val;
-       val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
-               DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(0);
+       dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);
+}
 
-       dwmci_writel(host, DWMCI_CLKSEL, val);
+unsigned int exynos_dwmci_get_clk(int dev_index)
+{
+       return get_mmc_clk(dev_index);
 }
 
 int exynos_dwmci_init(u32 regbase, int bus_width, int index)
 {
        struct dwmci_host *host = NULL;
+       unsigned int clock, div;
        host = malloc(sizeof(struct dwmci_host));
        if (!host) {
                printf("dwmci_host malloc fail!\n");
                return 1;
        }
 
+       /*
+        * The max operating freq of FSYS block is 400MHz.
+        * Scale down the 400MHz to number 400.
+        * Scale down the MPLL clock by dividing MPLL_CLK with ONE_MEGA_HZ.
+        * Arrive at the divisor value taking 400 as the reference.
+        */
+
+       /* get mpll clock and divide it by ONE_MEGA_HZ */
+       clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;

Quoted 8 lines

drivers/mmc/exynos_dw_mmc.c line 79

Quoted 6 lines

drivers/mmc/exynos_dw_mmc.c +6 -0

+
+       /* Arrive at the divisor value. */
+       for (div = 0; div <= 0xf; div++) {
+               if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
+                       break;
+       }

What if you don't find the right divisor?

i want to use like this.

sclk = mmc_get_clk(); -> then we can get the FSYS1 clock value div = DIV_ROUND_UP(sclk, freq); => freq is request clock value. mmc_set_clk(index, div);

Then we can set to div value at clock register.
It didn't need to add this code...

Hi Jaehoon,

Quoted 16 lines · Jaehoon Chung

Quoted 15 lines · Simon Glass

Hi Amar,

Quoted 12 lines · Amar

This patch adds FDT support for DWMMC, by reading the DWMMC node data from the device tree and initialising DWMMC channels as per data obtained from the node.

Changes from V1:
1)Updated code to have same signature for the function exynos_dwmci_init() for both FDT and non-FDT versions. 2)Updated code to pass device_id parameter to the function exynos5_mmc_set_clk_div() instead of index. 3)Updated code to decode the value of "samsung,width" from FDT. 4)Channel index is computed instead of getting from FDT.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Changes from V2:
1)Updation of commit message and resubmition of proper patch

set.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Changes from V3:
1)Replaced the new function exynos5_mmc_set_clk_div() with the existing function set_mmc_clk(). set_mmc_clk() will do the

purpose.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

2)Computation of FSYS block clock divisor (pre-ratio) is added.

Signed-off-by Vivek Gautam <[email protected]>

Signed-off-by Amar <[email protected]>

---
 arch/arm/include/asm/arch-exynos/dwmmc.h |   4 +
 drivers/mmc/exynos_dw_mmc.c              | 129

+++++++++++++++++++++++++++++--

Quoted 4 lines

Quoted 4 lines

Review

Quoted 4 lines

 include/dwmmc.h                          |   4 +
 3 files changed, 130 insertions(+), 7 deletions(-)

(quoted diff) +0 -0

b/arch/arm/include/asm/arch-exynos/dwmmc.h

Quoted 13 lines

Quoted 13 lines

arch/arm/include/asm/arch-exynos/dwmmc.h line 35

Quoted 11 lines

index 8acdf9b..40dcc7b 100644

arch/arm/include/asm/arch-exynos/dwmmc.h +3 -0

@@ -29,8 +29,12 @@
 
 int exynos_dwmci_init(u32 regbase, int bus_width, int index);
 
+#ifdef CONFIG_OF_CONTROL
+unsigned int exynos_dwmmc_init(const void *blob);
+#else
 static inline unsigned int exynos_dwmmc_init(int index, int bus_width)

Why unsigned?

Quoted 4 lines

Quoted 4 lines

I'm really not that keen on functions which change their signature based on an #ifdef. Can we perhaps have

Quoted 7 lines

Quoted 7 lines

int exynos_dwmmc_init(const void *blob);

which will pass NULL when there is no FDT, and

int exynos_dwmmc_add_port(int index, int bus_width)

for use by non-FDT boards?

Quoted 33 lines

Quoted 33 lines

Quoted 20 lines

arch/arm/include/asm/arch-exynos/dwmmc.h +1 -0

 {
        unsigned int base = samsung_get_base_mmc() + (0x10000 * index);
        return exynos_dwmci_init(base, bus_width, index);
 }
+#endif

drivers/mmc/exynos_dw_mmc.c +3 -1

index 72a31b7..d7ca7d0 100644
@@ -19,39 +19,154 @@
  */
 
 #include <common.h>
-#include <malloc.h>
 #include <dwmmc.h>
+#include <fdtdec.h>
+#include <libfdt.h>
+#include <malloc.h>
 #include <asm/arch/dwmmc.h>

drivers/mmc/exynos_dw_mmc.c line 36

Quoted 10 lines

drivers/mmc/exynos_dw_mmc.c +9 -0

 #include <asm/arch/clk.h>
+#include <asm/arch/pinmux.h>
+
+#define        DWMMC_MAX_CH_NUM                4
+#define        DWMMC_MAX_FREQ                  52000000
+#define        DWMMC_MIN_FREQ                  400000
+#define        DWMMC_MMC0_CLKSEL_VAL           0x03030001
+#define        DWMMC_MMC2_CLKSEL_VAL           0x03020001
+#define        ONE_MEGA_HZ                     1000000
+#define        SCALED_VAL_FOUR_HUNDRED         400

I don't think you need these last two - you can just write the number in the code

Quoted 1 line

Why didn't add into the dwmmc.h?

Quoted 6 lines

Quoted 5 lines

drivers/mmc/exynos_dw_mmc.c line 38

Quoted 3 lines

drivers/mmc/exynos_dw_mmc.c +0 -0

 static char *EXYNOS_NAME = "EXYNOS DWMMC";

Same with this I think

Sorry..What means? Also need not?

Yes I mean that you probably don't need this - just put the string in the code.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/mmc/exynos_dw_mmc.c +1 -0

+u32 timing[3];
 

Quoted 9 lines

Quoted 9 lines

drivers/mmc/exynos_dw_mmc.c

Quoted 9 lines

drivers/mmc/exynos_dw_mmc.c +4 -3

+/*
+ * Function used as callback function to initialise the
+ * CLKSEL register for every mmc channel.
+ */
 static void exynos_dwmci_clksel(struct dwmci_host *host)
 {
-       u32 val;
-       val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
-               DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) |

DWMCI_SET_DIV_RATIO(0);

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

drivers/mmc/exynos_dw_mmc.c +6 -1

+       dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);
+}
 
-       dwmci_writel(host, DWMCI_CLKSEL, val);
+unsigned int exynos_dwmci_get_clk(int dev_index)
+{
+       return get_mmc_clk(dev_index);
 }
 
 int exynos_dwmci_init(u32 regbase, int bus_width, int index)
 {
        struct dwmci_host *host = NULL;
+       unsigned int clock, div;
        host = malloc(sizeof(struct dwmci_host));
        if (!host) {
                printf("dwmci_host malloc fail!\n");
                return 1;
        }
 

Quoted 4 lines

Quoted 4 lines

drivers/mmc/exynos_dw_mmc.c

Quoted 4 lines

drivers/mmc/exynos_dw_mmc.c +4 -0

+       /*
+        * The max operating freq of FSYS block is 400MHz.
+        * Scale down the 400MHz to number 400.
+        * Scale down the MPLL clock by dividing MPLL_CLK with

ONE_MEGA_HZ.

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

drivers/mmc/exynos_dw_mmc.c +5 -0

+        * Arrive at the divisor value taking 400 as the reference.
+        */
+
+       /* get mpll clock and divide it by ONE_MEGA_HZ */
+       clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;

Quoted 8 lines

drivers/mmc/exynos_dw_mmc.c line 79

Quoted 6 lines

drivers/mmc/exynos_dw_mmc.c +6 -0

+
+       /* Arrive at the divisor value. */
+       for (div = 0; div <= 0xf; div++) {
+               if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
+                       break;
+       }

What if you don't find the right divisor?

Quoted 8 lines

i want to use like this.

sclk = mmc_get_clk(); -> then we can get the FSYS1 clock value div = DIV_ROUND_UP(sclk, freq); => freq is request clock value. mmc_set_clk(index, div);

Then we can set to div value at clock register.
It didn't need to add this code...

OK, sounds good.

Ok, shall implement as suggested by Jaehoon.

Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Hi Simon / Jaehoon,

Thanks for review comments.
Please find the responses below.

Thanks & Regards
Amarendra Reddy

Quoted 34 lines · Simon Glass

Hi Jaehoon,

Quoted 16 lines · Jaehoon Chung

Quoted 15 lines · Simon Glass

Hi Amar,

Quoted 12 lines · Amar

This patch adds FDT support for DWMMC, by reading the DWMMC node data from the device tree and initialising DWMMC channels as per data obtained from the node.

Changes from V1:
1)Updated code to have same signature for the function exynos_dwmci_init() for both FDT and non-FDT versions. 2)Updated code to pass device_id parameter to the function exynos5_mmc_set_clk_div() instead of index. 3)Updated code to decode the value of "samsung,width" from FDT. 4)Channel index is computed instead of getting from FDT.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Changes from V2:
1)Updation of commit message and resubmition of proper patch

set.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Changes from V3:
1)Replaced the new function exynos5_mmc_set_clk_div() with the existing function set_mmc_clk(). set_mmc_clk() will do the

purpose.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

2)Computation of FSYS block clock divisor (pre-ratio) is added.

Signed-off-by Vivek Gautam <[email protected]>

Signed-off-by Amar <[email protected]>

---
  arch/arm/include/asm/arch-exynos/dwmmc.h |   4 +
  drivers/mmc/exynos_dw_mmc.c              | 129

Quoted 18 lines

+++++++++++++++++++++++++++++--

Quoted 4 lines

Quoted 4 lines

Review

Quoted 4 lines

  include/dwmmc.h                          |   4 +
  3 files changed, 130 insertions(+), 7 deletions(-)

(quoted diff) +0 -0

b/arch/arm/include/asm/arch-exynos/dwmmc.h

Quoted 12 lines

Quoted 12 lines

arch/arm/include/asm/arch-exynos/dwmmc.h line 35

Quoted 11 lines

index 8acdf9b..40dcc7b 100644

arch/arm/include/asm/arch-exynos/dwmmc.h +3 -0

@@ -29,8 +29,12 @@
 
  int exynos_dwmci_init(u32 regbase, int bus_width, int index);
 
+#ifdef CONFIG_OF_CONTROL
+unsigned int exynos_dwmmc_init(const void *blob);
+#else
  static inline unsigned int exynos_dwmmc_init(int index, int bus_width)

Why unsigned?

Ok, shall replace "unsigned int exynos_dwmmc_init(int index, int bus_width)" with
int exynos_dwmci_add_port(int index, u32 regbase, int bus_width, u32 clksel).
Regarding the parameter *'clksel':*
i) "timing" value shall be passed in case of FDT, to be written into CLKSEL register.
ii) NULL will be passed in case of non-FDT.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

I'm really not that keen on functions which change their signature based on an #ifdef. Can we perhaps have

int exynos_dwmmc_init(const void *blob);

which will pass NULL when there is no FDT, and

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

int exynos_dwmmc_add_port(int index, int bus_width)

for use by non-FDT boards?

Ok. I will call the function int exynos_dwmmc_init(NULL) for non-FDT and int exynos_dwmmc_init(const void *blob) for FDT.
And use "int exynos_dwmci_add_port(int index, u32 regbase, int bus_width, u32 clksel)".

Quoted 32 lines

Quoted 32 lines

Quoted 32 lines

Quoted 20 lines

arch/arm/include/asm/arch-exynos/dwmmc.h +1 -0

  {
         unsigned int base = samsung_get_base_mmc() + (0x10000 * index);
         return exynos_dwmci_init(base, bus_width, index);
  }
+#endif

drivers/mmc/exynos_dw_mmc.c +3 -1

index 72a31b7..d7ca7d0 100644
@@ -19,39 +19,154 @@
   */
 
  #include <common.h>
-#include <malloc.h>
  #include <dwmmc.h>
+#include <fdtdec.h>
+#include <libfdt.h>
+#include <malloc.h>
  #include <asm/arch/dwmmc.h>

drivers/mmc/exynos_dw_mmc.c line 36

Quoted 10 lines

drivers/mmc/exynos_dw_mmc.c +9 -0

  #include <asm/arch/clk.h>
+#include <asm/arch/pinmux.h>
+
+#define        DWMMC_MAX_CH_NUM                4
+#define        DWMMC_MAX_FREQ                  52000000
+#define        DWMMC_MIN_FREQ                  400000
+#define        DWMMC_MMC0_CLKSEL_VAL           0x03030001
+#define        DWMMC_MMC2_CLKSEL_VAL           0x03020001
+#define        ONE_MEGA_HZ                     1000000
+#define        SCALED_VAL_FOUR_HUNDRED         400

I don't think you need these last two - you can just write the number in the code

Quoted 1 line

Quoted 1 line

Why didn't add into the dwmmc.h?

Ok, will just write the number in the code.

Quoted 7 lines

Quoted 4 lines

Quoted 3 lines

drivers/mmc/exynos_dw_mmc.c line 38

Quoted 2 lines

drivers/mmc/exynos_dw_mmc.c +0 -0

  static char *EXYNOS_NAME = "EXYNOS DWMMC";

Same with this I think

Sorry..What means? Also need not?

Yes I mean that you probably don't need this - just put the string in the code.

Ok.

Quoted 58 lines

Quoted 11 lines

Quoted 11 lines

drivers/mmc/exynos_dw_mmc.c

Quoted 11 lines

drivers/mmc/exynos_dw_mmc.c +5 -3

+u32 timing[3];
 
+/*
+ * Function used as callback function to initialise the
+ * CLKSEL register for every mmc channel.
+ */
  static void exynos_dwmci_clksel(struct dwmci_host *host)
  {
-       u32 val;
-       val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
-               DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) |

DWMCI_SET_DIV_RATIO(0);

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

drivers/mmc/exynos_dw_mmc.c +6 -1

+       dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);
+}
 
-       dwmci_writel(host, DWMCI_CLKSEL, val);
+unsigned int exynos_dwmci_get_clk(int dev_index)
+{
+       return get_mmc_clk(dev_index);
  }
 
  int exynos_dwmci_init(u32 regbase, int bus_width, int index)
  {
         struct dwmci_host *host = NULL;
+       unsigned int clock, div;
         host = malloc(sizeof(struct dwmci_host));
         if (!host) {
                 printf("dwmci_host malloc fail!\n");
                 return 1;
         }
 

Quoted 4 lines

Quoted 4 lines

drivers/mmc/exynos_dw_mmc.c

Quoted 4 lines

drivers/mmc/exynos_dw_mmc.c +4 -0

+       /*
+        * The max operating freq of FSYS block is 400MHz.
+        * Scale down the 400MHz to number 400.
+        * Scale down the MPLL clock by dividing MPLL_CLK with

ONE_MEGA_HZ.

Quoted 12 lines

Quoted 4 lines

Quoted 4 lines

drivers/mmc/exynos_dw_mmc.c +4 -0

+        * Arrive at the divisor value taking 400 as the reference.
+        */
+
+       /* get mpll clock and divide it by ONE_MEGA_HZ */

Quoted 8 lines

drivers/mmc/exynos_dw_mmc.c line 79

Quoted 7 lines

drivers/mmc/exynos_dw_mmc.c +7 -0

+       clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;
+
+       /* Arrive at the divisor value. */
+       for (div = 0; div <= 0xf; div++) {
+               if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
+                       break;
+       }

What if you don't find the right divisor?

Quoted 8 lines

i want to use like this.

sclk = mmc_get_clk(); -> then we can get the FSYS1 clock value div = DIV_ROUND_UP(sclk, freq); => freq is request clock value. mmc_set_clk(index, div);

Then we can set to div value at clock register.
It didn't need to add this code...

OK, sounds good.

Quoted 9 lines

Ok, shall implement as suggested by Jaehoon.

Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Quoted 100 lines · Amarendra Reddy

Hi Simon / Jaehoon,

Thanks for review comments.
Please find the responses below.

Thanks & Regards
Amarendra Reddy

On 11 January 2013 11:14, Simon Glass <[email protected]> wrote:

Hi Jaehoon,

Quoted 68 lines

Quoted 64 lines · Jaehoon Chung

Quoted 17 lines · Simon Glass

Hi Amar,

Quoted 13 lines · Amar

This patch adds FDT support for DWMMC, by reading the DWMMC node data from the device tree and initialising DWMMC channels as per data obtained from the node.

Changes from V1:
1)Updated code to have same signature for the function exynos_dwmci_init() for both FDT and non-FDT versions. 2)Updated code to pass device_id parameter to the function exynos5_mmc_set_clk_div() instead of index. 3)Updated code to decode the value of "samsung,width" from
FDT.
4)Channel index is computed instead of getting from FDT.

Quoted 4 lines

Quoted 3 lines

Changes from V2:
1)Updation of commit message and resubmition of proper patch

set.

Changes from V3:

Quoted 5 lines

Quoted 4 lines

1)Replaced the new function exynos5_mmc_set_clk_div() with
the
existing function set_mmc_clk(). set_mmc_clk() will do the

purpose.

2)Computation of FSYS block clock divisor (pre-ratio) is

Quoted 9 lines

Quoted 8 lines

added.

Signed-off-by Vivek Gautam <[email protected]>

Signed-off-by Amar <[email protected]>

---
  arch/arm/include/asm/arch-**exynos/dwmmc.h |   4 +
  drivers/mmc/exynos_dw_mmc.c              | 129

+++++++++++++++++++++++++++++-**-

   include/dwmmc.h                          |   4 +

Quoted 5 lines

Review

Quoted 4 lines

  3 files changed, 130 insertions(+), 7 deletions(-)

(quoted diff) +0 -0

b/arch/arm/include/asm/arch-**exynos/dwmmc.h

index 8acdf9b..40dcc7b 100644

Quoted 14 lines

arch/arm/include/asm/arch-**exynos/dwmmc.h line 37

Quoted 12 lines

arch/arm/include/asm/arch-**exynos/dwmmc.h +3 -0

@@ -29,8 +29,12 @@
 
  int exynos_dwmci_init(u32 regbase, int bus_width, int index);
 
+#ifdef CONFIG_OF_CONTROL
+unsigned int exynos_dwmmc_init(const void *blob);
+#else
  static inline unsigned int exynos_dwmmc_init(int index, int
 bus_width)
 

Why unsigned?

Quoted 1 line

Quoted 1 line

Ok, shall replace "unsigned int exynos_dwmmc_init(int index, int

bus_width)" with
int exynos_dwmci_add_port(int index, u32 regbase, int bus_width, u32 clksel).
Regarding the parameter *'clksel':*

i) "timing" value shall be passed in case of FDT, to be written into CLKSEL
register.
ii) NULL will be passed in case of non-FDT.

Quoted 9 lines

Quoted 9 lines

I'm really not that keen on functions which change their signature

Quoted 6 lines

based on an #ifdef. Can we perhaps have

int exynos_dwmmc_init(const void *blob);

which will pass NULL when there is no FDT, and

Quoted 10 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

int exynos_dwmmc_add_port(int index, int bus_width)

for use by non-FDT boards?

Ok. I will call the function int exynos_dwmmc_init(NULL) for non-FDT and

int exynos_dwmmc_init(const void *blob) for FDT.
And use "int exynos_dwmci_add_port(int index, u32 regbase, int bus_width, u32 clksel)".

But patch v5 doesn't use exynos_dwmmc_init(NULL) and it uses exynos_dwmci_add_port directly in board file.

The initial thought was to use

Note that in the absence of an FDT it is supposed to be the board file which knows which MMC ports are active.

{
     #ifdef CONFIG_OF_CONTROL

            /* Read data from FDT */

            exynos_dwmmc_add_port(index, bus_width, ...)

This code should go in the mmc driver. One of the ideas behind FDT is that the drivers can figure out by themselves what ports to set up. Also only the driver knows about its particular fields.

     #else

            exynos_dwmmc_add_port(0,8...)

            exynos_dwmmc_add_port(2,4...)

This code should go in the board file, since without an FDT the driver can't know what ports to init.

Why we need blob argument in exynos_dwmmc_init? Already spi_init() just uses gd->fdt_blob directly of drivers/spi/exynos_spi.c

I think exynos_dwmmc_init function needs a struct argument for int index and int bus_width such follows.

struct exynos_dwmmc_config {
    int index;
    int bus_width;
};

exynos_dwmmc_init(struct exynos_dwmmc_config *config) {
...
}

If config is NULL and gd->fdt_blob isn't NULL, we can get index and bus_width from blob.

Yes, this is a good approach.

Thanks.

Quoted 135 lines

Quoted 44 lines

Quoted 36 lines

   {

Quoted 24 lines

Quoted 24 lines

arch/arm/include/asm/arch-**exynos/dwmmc.h +1 -0

         unsigned int base = samsung_get_base_mmc() + (0x10000 *
 index);
         return exynos_dwmci_init(base, bus_width, index);
  }
+#endif

drivers/mmc/exynos_dw_mmc.c +7 -1

index 72a31b7..d7ca7d0 100644
@@ -19,39 +19,154 @@
   */
 
  #include <common.h>
-#include <malloc.h>
  #include <dwmmc.h>
+#include <fdtdec.h>
+#include <libfdt.h>
+#include <malloc.h>
  #include <asm/arch/dwmmc.h>
  #include <asm/arch/clk.h>
+#include <asm/arch/pinmux.h>
+
+#define        DWMMC_MAX_CH_NUM                4
+#define        DWMMC_MAX_FREQ                  52000000

Quoted 9 lines

drivers/mmc/exynos_dw_mmc.c line 37

Quoted 6 lines

drivers/mmc/exynos_dw_mmc.c +5 -0

+#define        DWMMC_MIN_FREQ                  400000
+#define        DWMMC_MMC0_CLKSEL_VAL           0x03030001
+#define        DWMMC_MMC2_CLKSEL_VAL           0x03020001
+#define        ONE_MEGA_HZ                     1000000
+#define        SCALED_VAL_FOUR_HUNDRED         400
 

I don't think you need these last two - you can just write the number in the code

Quoted 2 lines

Why didn't add into the dwmmc.h?

Ok, will just write the number in the code.

Quoted 4 lines

   static char *EXYNOS_NAME = "EXYNOS DWMMC";

Quoted 1 line

Quoted 1 line

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Same with this I think

Sorry..What means? Also need not?

Yes I mean that you probably don't need this - just put the string in the code.

Ok.

+u32 timing[3];

Quoted 59 lines

Quoted 59 lines

Quoted 11 lines

drivers/mmc/exynos_dw_mmc.c

Quoted 11 lines

drivers/mmc/exynos_dw_mmc.c +4 -3

+/*
+ * Function used as callback function to initialise the
+ * CLKSEL register for every mmc channel.
+ */
  static void exynos_dwmci_clksel(struct dwmci_host *host)
  {
-       u32 val;
-       val = DWMCI_SET_SAMPLE_CLK(DWMCI_**SHIFT_0) |
-               DWMCI_SET_DRV_CLK(DWMCI_SHIFT_**0) |

Quoted 1 line

DWMCI_SET_DIV_RATIO(0);

+ dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);

Quoted 18 lines

Quoted 18 lines

drivers/mmc/exynos_dw_mmc.c +5 -1

+}
 
-       dwmci_writel(host, DWMCI_CLKSEL, val);
+unsigned int exynos_dwmci_get_clk(int dev_index)
+{
+       return get_mmc_clk(dev_index);
  }
 
  int exynos_dwmci_init(u32 regbase, int bus_width, int index)
  {
         struct dwmci_host *host = NULL;
+       unsigned int clock, div;
         host = malloc(sizeof(struct dwmci_host));
         if (!host) {
                 printf("dwmci_host malloc fail!\n");
                 return 1;
         }

Quoted 6 lines

drivers/mmc/exynos_dw_mmc.c

Quoted 5 lines

drivers/mmc/exynos_dw_mmc.c +4 -0

+       /*
+        * The max operating freq of FSYS block is 400MHz.
+        * Scale down the 400MHz to number 400.
+        * Scale down the MPLL clock by dividing MPLL_CLK with

ONE_MEGA_HZ.

+ * Arrive at the divisor value taking 400 as the reference.

Quoted 4 lines

Quoted 4 lines

drivers/mmc/exynos_dw_mmc.c +4 -0

+        */
+
+       /* get mpll clock and divide it by ONE_MEGA_HZ */
+       clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;

Quoted 9 lines

drivers/mmc/exynos_dw_mmc.c line 79

Quoted 7 lines

drivers/mmc/exynos_dw_mmc.c +6 -0

+
+       /* Arrive at the divisor value. */
+       for (div = 0; div <= 0xf; div++) {
+               if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
+                       break;
+       }

What if you don't find the right divisor?

i want to use like this.

sclk = mmc_get_clk(); -> then we can get the FSYS1 clock value div = DIV_ROUND_UP(sclk, freq); => freq is request clock value. mmc_set_clk(index, div);

Quoted 7 lines

Quoted 3 lines

Then we can set to div value at clock register.
It didn't need to add this code...

OK, sounds good.

Ok, shall implement as suggested by Jaehoon.

Regards,

Quoted 7 lines

Simon
______________________________**_________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/**listinfo/u-boot<http://lists.denx.de/mailman/listinfo/u-boot>

______________________________**_________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/**listinfo/u-boot<http://lists.denx.de/mailman/listinfo/u-boot>

This API computes the divisor value based on MPLL clock and writes it into the FSYS1 register.

Changes from V1:
1)Updated the function exynos5_mmc_set_clk_div() to receive 'device_i'd as input parameter instead of 'index'.

Changes from V2:
1)Updation of commit message and resubmition of proper patch set.

Changes from V3:
1)Removed the new API exynos5_mmc_set_clk_div() from clock.c, because existing API set_mmc_clk() can be used to set mmc clock.

Signed-off-by Amar <[email protected]>

---
 arch/arm/cpu/armv7/exynos/clock.c      | 4 ++--
 arch/arm/include/asm/arch-exynos/clk.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

arch/arm/cpu/armv7/exynos/clock.c +0 -0

index 973b84e..89574ba 100644

arch/arm/cpu/armv7/exynos/clock.c +1 -1

        sel = readl(&clk->src_fsys);
        sel = (sel >> (dev_index << 2)) & 0xf;

@@ -539,7 +539,7 @@ static unsigned long exynos5_get_mmc_clk(int dev_index)
                (struct exynos5_clock *)samsung_get_base_clock();
        unsigned long uclk, sclk;
        unsigned int sel, ratio, pre_ratio;
-       int shift;
+       int shift = 0;
 
        sel = readl(&clk->src_fsys);
        sel = (sel >> (dev_index << 2)) & 0xf;

arch/arm/include/asm/arch-exynos/clk.h +0 -0

index 1935b0b..a4d5b4e 100644

arch/arm/include/asm/arch-exynos/clk.h +1 -1

+
 unsigned long get_pll_clk(int pllreg);
 unsigned long get_arm_clk(void);
 unsigned long get_i2c_clk(void);
--

1.8.0

Hi Amar,

Quoted 20 lines · Amar

This API computes the divisor value based on MPLL clock and writes it into the FSYS1 register.

Changes from V1:
1)Updated the function exynos5_mmc_set_clk_div() to receive 'device_i'd as input parameter instead of 'index'.

Changes from V2:
1)Updation of commit message and resubmition of proper patch set.

Changes from V3:
1)Removed the new API exynos5_mmc_set_clk_div() from clock.c, because existing API set_mmc_clk() can be used to set mmc clock.

Signed-off-by Amar <[email protected]>

---
 arch/arm/cpu/armv7/exynos/clock.c      | 4 ++--
 arch/arm/include/asm/arch-exynos/clk.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

arch/arm/cpu/armv7/exynos/clock.c static unsigned long exynos4_get_mmc_clk(int dev_index) · line 493

Quoted 10 lines

arch/arm/cpu/armv7/exynos/clock.c +1 -1

index 973b84e..89574ba 100644
@@ -490,7 +490,7 @@ static unsigned long exynos4_get_mmc_clk(int dev_index)
                (struct exynos4_clock *)samsung_get_base_clock();
        unsigned long uclk, sclk;
        unsigned int sel, ratio, pre_ratio;
-       int shift;
+       int shift = 0;

Is this fixing a warning?

Quoted 21 lines

arch/arm/cpu/armv7/exynos/clock.c +1 -1

        sel = readl(&clk->src_fsys);
        sel = (sel >> (dev_index << 2)) & 0xf;

@@ -539,7 +539,7 @@ static unsigned long exynos5_get_mmc_clk(int dev_index)
                (struct exynos5_clock *)samsung_get_base_clock();
        unsigned long uclk, sclk;
        unsigned int sel, ratio, pre_ratio;
-       int shift;
+       int shift = 0;
 
        sel = readl(&clk->src_fsys);
        sel = (sel >> (dev_index << 2)) & 0xf;

arch/arm/include/asm/arch-exynos/clk.h +0 -0

index 1935b0b..a4d5b4e 100644
@@ -29,6 +29,9 @@
 #define VPLL   4
 #define BPLL   5
 

arch/arm/include/asm/arch-exynos/clk.h line 33

Quoted 2 lines

arch/arm/include/asm/arch-exynos/clk.h +2 -0

+#define FSYS1_MMC0_DIV_MASK    0xff0f
+#define FSYS1_MMC0_DIV_VAL     0x0701

What is this used for? I don't see it in this patch.

Overall it is not clear what this patch is for.

Quoted 8 lines

arch/arm/include/asm/arch-exynos/clk.h +1 -1

+
 unsigned long get_pll_clk(int pllreg);
 unsigned long get_arm_clk(void);
 unsigned long get_i2c_clk(void);
--

1.8.0

Regards,
Simon

Quoted 12 lines · Simon Glass

Hi Amar,

Quoted 9 lines · Amar

This API computes the divisor value based on MPLL clock and writes it into the FSYS1 register.

Changes from V1:
1)Updated the function exynos5_mmc_set_clk_div() to receive 'device_i'd as input parameter instead of 'index'.

Changes from V2:
1)Updation of commit message and resubmition of proper patch

set.

Quoted 5 lines

Quoted 5 lines

Changes from V3:
1)Removed the new API exynos5_mmc_set_clk_div() from clock.c, because existing API set_mmc_clk() can be used to set mmc clock.

Quoted 7 lines

Review

Quoted 7 lines

Signed-off-by Amar <[email protected]>

---
 arch/arm/cpu/armv7/exynos/clock.c      | 4 ++--
 arch/arm/include/asm/arch-exynos/clk.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/arch/arm/cpu/armv7/exynos/clock.c

Quoted 4 lines

arch/arm/cpu/armv7/exynos/clock.c static unsigned long exynos4_get_mmc_clk(int

Quoted 4 lines

index 973b84e..89574ba 100644

arch/arm/cpu/armv7/exynos/clock.c +0 -0

@@ -490,7 +490,7 @@ static unsigned long exynos4_get_mmc_clk(int

dev_index)

Quoted 7 lines

arch/arm/cpu/armv7/exynos/clock.c static unsigned long exynos4_get_mmc_clk(int dev_index) · line 493

Quoted 5 lines

arch/arm/cpu/armv7/exynos/clock.c +1 -1

                (struct exynos4_clock *)samsung_get_base_clock();
        unsigned long uclk, sclk;
        unsigned int sel, ratio, pre_ratio;
-       int shift;
+       int shift = 0;

Is this fixing a warning?

Maybe..fix the compiler warning..

Quoted 5 lines

arch/arm/cpu/armv7/exynos/clock.c static unsigned long exynos5_get_mmc_clk(int

Quoted 5 lines

arch/arm/cpu/armv7/exynos/clock.c +0 -0

        sel = readl(&clk->src_fsys);
        sel = (sel >> (dev_index << 2)) & 0xf;

@@ -539,7 +539,7 @@ static unsigned long exynos5_get_mmc_clk(int

dev_index)

Quoted 9 lines

Review

Quoted 9 lines

arch/arm/cpu/armv7/exynos/clock.c +1 -1

                (struct exynos5_clock *)samsung_get_base_clock();
        unsigned long uclk, sclk;
        unsigned int sel, ratio, pre_ratio;
-       int shift;
+       int shift = 0;
 
        sel = readl(&clk->src_fsys);
        sel = (sel >> (dev_index << 2)) & 0xf;

(quoted diff) +0 -0

b/arch/arm/include/asm/arch-exynos/clk.h

Quoted 7 lines

Quoted 7 lines

index 1935b0b..a4d5b4e 100644

arch/arm/include/asm/arch-exynos/clk.h +0 -0

@@ -29,6 +29,9 @@
 #define VPLL   4
 #define BPLL   5
 

Quoted 6 lines

arch/arm/include/asm/arch-exynos/clk.h line 33

Quoted 2 lines

arch/arm/include/asm/arch-exynos/clk.h +2 -0

+#define FSYS1_MMC0_DIV_MASK    0xff0f
+#define FSYS1_MMC0_DIV_VAL     0x0701

What is this used for? I don't see it in this patch.

Overall it is not clear what this patch is for.

This define didn't need. That value is not static value, isn't?

Best Regards,
Jaehoon Chung

Quoted 9 lines

Quoted 8 lines

arch/arm/include/asm/arch-exynos/clk.h +1 -1

+
 unsigned long get_pll_clk(int pllreg);
 unsigned long get_arm_clk(void);
 unsigned long get_i2c_clk(void);
--

1.8.0

Quoted 3 lines

Regards,
Simon

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Hi Jaehoon / Simon,

Thanks for review comments.
Please find my responses below.

Thanks & Regards
Amarendra Reddy

Quoted 41 lines · Jaehoon Chung

Quoted 39 lines · Simon Glass

Hi Amar,

Quoted 28 lines · Amar

This API computes the divisor value based on MPLL clock and writes it into the FSYS1 register.

Changes from V1:
1)Updated the function exynos5_mmc_set_clk_div() to receive 'device_i'd as input parameter instead of 'index'.

Changes from V2:
1)Updation of commit message and resubmition of proper patch
set.

Changes from V3:
1)Removed the new API exynos5_mmc_set_clk_div() from clock.c, because existing API set_mmc_clk() can be used to set mmc
clock.

Signed-off-by Amar <[email protected]>

---
 arch/arm/cpu/armv7/exynos/clock.c      | 4 ++--
 arch/arm/include/asm/arch-exynos/clk.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

arch/arm/cpu/armv7/exynos/clock.c +0 -0

index 973b84e..89574ba 100644

arch/arm/cpu/armv7/exynos/clock.c static unsigned long exynos4_get_mmc_clk(int · line 494

Quoted 6 lines

arch/arm/cpu/armv7/exynos/clock.c +1 -1

@@ -490,7 +490,7 @@ static unsigned long exynos4_get_mmc_clk(int
 dev_index)
                (struct exynos4_clock *)samsung_get_base_clock();
        unsigned long uclk, sclk;
        unsigned int sel, ratio, pre_ratio;
-       int shift;
+       int shift = 0;

Is this fixing a warning?

Quoted 1 line

Maybe..fix the compiler warning..

As 'shift' was uninitialised, it had garbage value which was causing a problem when "dev_index = 0 or 2".

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

arch/arm/cpu/armv7/exynos/clock.c +1 -1

        sel = readl(&clk->src_fsys);
        sel = (sel >> (dev_index << 2)) & 0xf;

@@ -539,7 +539,7 @@ static unsigned long exynos5_get_mmc_clk(int
 dev_index)
                (struct exynos5_clock *)samsung_get_base_clock();
        unsigned long uclk, sclk;
        unsigned int sel, ratio, pre_ratio;
-       int shift;
+       int shift = 0;
 
        sel = readl(&clk->src_fsys);

        sel = (sel >> (dev_index << 2)) & 0xf;

arch/arm/include/asm/arch-exynos/clk.h +0 -0

index 1935b0b..a4d5b4e 100644
@@ -29,6 +29,9 @@
 #define VPLL   4
 #define BPLL   5
 

Quoted 7 lines

Quoted 6 lines

arch/arm/include/asm/arch-exynos/clk.h line 33

Quoted 2 lines

arch/arm/include/asm/arch-exynos/clk.h +2 -0

+#define FSYS1_MMC0_DIV_MASK    0xff0f
+#define FSYS1_MMC0_DIV_VAL     0x0701

What is this used for? I don't see it in this patch.

Overall it is not clear what this patch is for.

This define didn't need. That value is not static value, isn't?

In fact, V2 of this patch had a new function (which I added). That new function was using the #define values.
But later in V4 the new function has been removed.

As of now the #define values are used in the file
board/samsung/smdk5250/clock_init.c.
The values are used during "booting from EMMC".

Quoted 21 lines

Best Regards,
Jaehoon Chung

Quoted 9 lines

Quoted 8 lines

arch/arm/include/asm/arch-exynos/clk.h +1 -1

+
 unsigned long get_pll_clk(int pllreg);
 unsigned long get_arm_clk(void);
 unsigned long get_i2c_clk(void);
--

1.8.0

Quoted 3 lines

Regards,
Simon

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

This patch enables and initialises DWMMC for SMDK5250.
Supports both FDT and non-FDT. This patch creates a new file 'exynos5-dt.c' meant for FDT support.
exynos5-dt.c: This file shall contain all code which supports FDT.
Any addition of FDT support for any module needs to be added in this file.
smdk5250.c: This file shall contain the code which supports non-FDT.
version. Any addition of non-FDT support for any module needs to be added in this file.
May be, the file smdk5250.c can be removed in near future when non-FDT is not required.

The Makefile is updated to compile only one of the files exynos5-dt.c / smdk5250.c based on FDT configuration.

NOTE:
Please note that all additions corresponding to FDT need to be added into the file exynos5-dt.c.
At same time if non-FDT support is required then add the corresponding updations into smdk5250.c.

Changes from V1:
1)A new file 'exynos5-dt.c' is created meant for FDT support 2)Makefile is updated to compile only one of the files exynos5-dt.c / smdk5250.c based on FDT configuration

Changes from V2:
1)Updation of commit message and resubmition of proper patch set.

Changes from V3:
No change.

Signed-off-by Amar <[email protected]>

---
 board/samsung/smdk5250/Makefile     |   4 +
 board/samsung/smdk5250/exynos5-dt.c | 242 ++++++++++++++++++++++++++++++++++++
 board/samsung/smdk5250/smdk5250.c   |  97 +++++++--------
 include/configs/exynos5250-dt.h     |   2 +
 include/i2c.h                       |   2 +
 5 files changed, 292 insertions(+), 55 deletions(-)
 create mode 100644 board/samsung/smdk5250/exynos5-dt.c

board/samsung/smdk5250/Makefile +4 -0

index 47c6a5a..ecca9f3 100644
@@ -32,8 +32,12 @@ COBJS        += tzpc_init.o
 COBJS  += smdk5250_spl.o
 
 ifndef CONFIG_SPL_BUILD
+ifdef CONFIG_OF_CONTROL
+COBJS  += exynos5-dt.o
+else
 COBJS  += smdk5250.o
 endif
+endif
 
 ifdef CONFIG_SPL_BUILD
 COBJS  += spl_boot.o

board/samsung/smdk5250/exynos5-dt.c new file +47 -0

new file mode 100644
index 0000000..da539ca
@@ -0,0 +1,242 @@
+/*
+ * Copyright (C) 2012 Samsung Electronics
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <fdtdec.h>
+#include <asm/io.h>
+#include <i2c.h>
+#include <netdev.h>
+#include <spi.h>
+#include <asm/arch/cpu.h>
+#include <asm/arch/dwmmc.h>
+#include <asm/arch/gpio.h>
+#include <asm/arch/mmc.h>
+#include <asm/arch/pinmux.h>
+#include <asm/arch/sromc.h>
+#include <power/pmic.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+int board_init(void)
+{
+       gd->bd->bi_boot_params = (PHYS_SDRAM_1 + 0x100UL);
+#ifdef CONFIG_EXYNOS_SPI
+       spi_init();
+#endif
+       return 0;
+}
+

board/samsung/smdk5250/exynos5-dt.c new file +22 -0

+               return -1;
+       else
+               return 0;
+}
+#endif
+
+void dram_init_banksize(void)
+{
+       gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
+       gd->bd->bi_dram[0].size = get_ram_size((long *)PHYS_SDRAM_1,
+                                                       PHYS_SDRAM_1_SIZE);
+       gd->bd->bi_dram[1].start = PHYS_SDRAM_2;
+       gd->bd->bi_dram[1].size = get_ram_size((long *)PHYS_SDRAM_2,
+                                                       PHYS_SDRAM_2_SIZE);
+       gd->bd->bi_dram[2].start = PHYS_SDRAM_3;
+       gd->bd->bi_dram[2].size = get_ram_size((long *)PHYS_SDRAM_3,
+                                                       PHYS_SDRAM_3_SIZE);
+       gd->bd->bi_dram[3].start = PHYS_SDRAM_4;
+       gd->bd->bi_dram[3].size = get_ram_size((long *)PHYS_SDRAM_4,
+                                                       PHYS_SDRAM_4_SIZE);
+       gd->bd->bi_dram[4].start = PHYS_SDRAM_5;
+       gd->bd->bi_dram[4].size = get_ram_size((long *)PHYS_SDRAM_5,

board/samsung/smdk5250/exynos5-dt.c new file +10 -0

+}
+
+static int decode_sromc(const void *blob, struct fdt_sromc *config)
+{
+       int err;
+       int node;
+
+       node = fdtdec_next_compatible(blob, 0, COMPAT_SAMSUNG_EXYNOS5_SROMC);
+       if (node < 0) {
+               debug("Could not find SROMC node\n");

board/samsung/smdk5250/exynos5-dt.c new file +31 -0

+       }
+
+       return 0;
+}
+
+int board_eth_init(bd_t *bis)
+{
+#ifdef CONFIG_SMC911X
+       u32 smc_bw_conf, smc_bc_conf;
+       struct fdt_sromc config;
+       fdt_addr_t base_addr;
+       int node;
+
+       node = decode_sromc(gd->fdt_blob, &config);
+       if (node < 0) {
+               debug("%s: Could not find sromc configuration\n", __func__);
+               return 0;
+       }
+       node = fdtdec_next_compatible(gd->fdt_blob, node, COMPAT_SMSC_LAN9215);
+       if (node < 0) {
+               debug("%s: Could not find lan9215 configuration\n", __func__);
+               return 0;
+       }
+
+       /* We now have a node, so any problems from now on are errors */
+       base_addr = fdtdec_get_addr(gd->fdt_blob, node, "reg");
+       if (base_addr == FDT_ADDR_T_NONE) {
+               debug("%s: Could not find lan9215 address\n", __func__);
+               return -1;
+       }
+

board/samsung/smdk5250/exynos5-dt.c new file +18 -0

+                       SROMC_BC_TCOS(config.timing[FDT_SROM_TCOS]) |\
+                       SROMC_BC_TACC(config.timing[FDT_SROM_TACC]) |\
+                       SROMC_BC_TCOH(config.timing[FDT_SROM_TCOH]) |\
+                       SROMC_BC_TAH(config.timing[FDT_SROM_TAH])   |\
+                       SROMC_BC_TACP(config.timing[FDT_SROM_TACP]) |\
+                       SROMC_BC_PMC(config.timing[FDT_SROM_PMC]);
+
+       /* Select and configure the SROMC bank */
+       exynos_pinmux_config(PERIPH_ID_SROMC, config.bank);
+       s5p_config_sromc(config.bank, smc_bw_conf, smc_bc_conf);
+       return smc911x_initialize(0, base_addr);
+#endif
+       return 0;
+}
+
+#ifdef CONFIG_DISPLAY_BOARDINFO
+int checkboard(void)
+{

board/samsung/smdk5250/exynos5-dt.c new file +27 -0

+
+       /* dwmmc initializattion for available channels */
+       ret = exynos_dwmmc_init(gd->fdt_blob);
+       if (ret)
+               debug("dwmmc init failed\n");
+
+       return ret;
+}
+#endif
+
+static int board_uart_init(void)
+{
+       int err;
+
+       err = exynos_pinmux_config(PERIPH_ID_UART0, PINMUX_FLAG_NONE);
+       if (err) {
+               debug("UART0 not configured\n");
+               return err;
+       }
+
+       err = exynos_pinmux_config(PERIPH_ID_UART1, PINMUX_FLAG_NONE);
+       if (err) {
+               debug("UART1 not configured\n");
+               return err;
+       }
+
+       err = exynos_pinmux_config(PERIPH_ID_UART2, PINMUX_FLAG_NONE);

board/samsung/smdk5250/exynos5-dt.c new file +11 -0

+       err = board_uart_init();
+       if (err) {
+               debug("UART init failed\n");
+               return err;
+       }
+#ifdef CONFIG_SYS_I2C_INIT_BOARD
+       board_i2c_init(gd->fdt_blob);
+#endif
+       return err;
+}
+#endif

board/samsung/smdk5250/smdk5250.c +3 -49

index 73c3ec0..e0fec11 100644
@@ -27,6 +27,7 @@
 #include <netdev.h>
 #include <spi.h>
 #include <asm/arch/cpu.h>
+#include <asm/arch/dwmmc.h>
 #include <asm/arch/gpio.h>
 #include <asm/arch/mmc.h>
 #include <asm/arch/pinmux.h>

@@ -95,59 +96,13 @@ void dram_init_banksize(void)
                                                        PHYS_SDRAM_8_SIZE);
 }
 
-#ifdef CONFIG_OF_CONTROL
-static int decode_sromc(const void *blob, struct fdt_sromc *config)
-{
-       int err;
-       int node;
-
-       node = fdtdec_next_compatible(blob, 0, COMPAT_SAMSUNG_EXYNOS5_SROMC);
-       if (node < 0) {
-               debug("Could not find SROMC node\n");
-               return node;
-       }
-
-       config->bank = fdtdec_get_int(blob, node, "bank", 0);
-       config->width = fdtdec_get_int(blob, node, "width", 2);
-
-       err = fdtdec_get_int_array(blob, node, "srom-timing", config->timing,
-                       FDT_SROM_TIMING_COUNT);
-       if (err < 0) {
-               debug("Could not decode SROMC configuration\n");
-               return -FDT_ERR_NOTFOUND;
-       }
-
-       return 0;
-}
-#endif
-
 int board_eth_init(bd_t *bis)
 {
 #ifdef CONFIG_SMC911X
        u32 smc_bw_conf, smc_bc_conf;
        struct fdt_sromc config;
        fdt_addr_t base_addr;
-       int node;
-
-#ifdef CONFIG_OF_CONTROL
-       node = decode_sromc(gd->fdt_blob, &config);
-       if (node < 0) {
-               debug("%s: Could not find sromc configuration\n", __func__);
-               return 0;
-       }
-       node = fdtdec_next_compatible(gd->fdt_blob, node, COMPAT_SMSC_LAN9215);
-       if (node < 0) {
-               debug("%s: Could not find lan9215 configuration\n", __func__);
-               return 0;
-       }
 
-       /* We now have a node, so any problems from now on are errors */
-       base_addr = fdtdec_get_addr(gd->fdt_blob, node, "reg");
-       if (base_addr == FDT_ADDR_T_NONE) {
-               debug("%s: Could not find lan9215 address\n", __func__);
-               return -1;
-       }
-#else
        /* Non-FDT configuration - bank number and timing parameters*/
        config.bank = CONFIG_ENV_SROM_BANK;
        config.width = 2;

@@ -160,7 +115,6 @@ int board_eth_init(bd_t *bis)
        config.timing[FDT_SROM_TACP] = 0x09;
        config.timing[FDT_SROM_PMC] = 0x01;
        base_addr = CONFIG_SMC911X_BASE;
-#endif
 
        /* Ethernet needs data bus width of 16 bits */
        if (config.width != 2) {

@@ -199,16 +153,31 @@ int checkboard(void)
 #ifdef CONFIG_GENERIC_MMC
 int board_mmc_init(bd_t *bis)
 {
-       int err;
+       int err = 0, ret = 0;
 
        err = exynos_pinmux_config(PERIPH_ID_SDMMC0, PINMUX_FLAG_8BIT_MODE);
-       if (err) {
+       if (err)