On Wed, 26 Jul 2017, Kever Yang wrote:

mmc using 150000000 as max-frequency and do not use fifo-mode.

Could you expand on the message here (for the benefit of someone reading this a couple years down the line): did your enabled the FIFO mode now that the DRAM protection was disabled (which is my guess)?

Signed-off-by: Kever Yang kever.yang@rock-chips.com Acked-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com

---

arch/arm/dts/rk3229-evb.dts | 2 -- arch/arm/dts/rk322x.dtsi | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm/dts/rk3229-evb.dts b/arch/arm/dts/rk3229-evb.dts index 1eac37d..ae0b0a4 100644 --- a/arch/arm/dts/rk3229-evb.dts +++ b/arch/arm/dts/rk3229-evb.dts @@ -68,7 +68,6 @@

&emmc { u-boot,dm-pre-reloc;

• fifo-mode; status = "okay";

};

@@ -79,7 +78,6 @@ cap-sd-highspeed; card-detect-delay = <200>; disable-wp;

• max-frequency = <50000000>; num-slots = <1>; supports-sd;

}; diff --git a/arch/arm/dts/rk322x.dtsi b/arch/arm/dts/rk322x.dtsi index ddbe113..22324f9 100644 --- a/arch/arm/dts/rk322x.dtsi +++ b/arch/arm/dts/rk322x.dtsi @@ -388,6 +388,7 @@ sdmmc: dwmmc@30000000 { compatible = "rockchip,rk3228-dw-mshc", "rockchip,rk3288-dw-mshc"; reg = <0x30000000 0x4000>;

• max-frequency = <150000000>;
  

  interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cru HCLK_SDMMC>, <&cru SCLK_SDMMC>, <&cru SCLK_SDMMC_DRV>, <&cru SCLK_SDMMC_SAMPLE>;

@@ -414,9 +415,8 @@ emmc: dwmmc@30020000 { compatible = "rockchip,rk3288-dw-mshc"; reg = <0x30020000 0x4000>;

• max-frequency = <150000000>;
  

  interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;

• clock-frequency = <37500000>;
  

• max-frequency = <37500000>;
  

  clocks = <&cru HCLK_EMMC>, <&cru SCLK_EMMC>, <&cru SCLK_EMMC_DRV>, <&cru SCLK_EMMC_SAMPLE>; clock-names = "biu", "ciu", "ciu_drv", "ciu_sample";

On 27 Jul 2017, at 03:12, Kever Yang kever.yang@rock-chips.com wrote:

Philipp,

On 07/27/2017 01:42 AM, Philipp Tomsich wrote:

...

On Wed, 26 Jul 2017, Kever Yang wrote:

...

mmc using 150000000 as max-frequency and do not use fifo-mode.

Could you expand on the message here (for the benefit of someone reading this a couple years down the line): did your enabled the FIFO mode now that the DRAM protection was disabled (which is my guess)?

The 150M for max-frequency is sync from rk3288, from what I have test, the speed change from 37.125M to 49.5M after I update this patch, it improve the mmc speed.

The fifo-mode is not need, I will update it, for the upstream version do not have this property, this only in my local source, sorry.

Just copy this info commit message then and I’ll apply. Thanks.

Thanks,

• Kever

...

...

Signed-off-by: Kever Yang kever.yang@rock-chips.com Acked-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com

---

arch/arm/dts/rk3229-evb.dts | 2 -- arch/arm/dts/rk322x.dtsi | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm/dts/rk3229-evb.dts b/arch/arm/dts/rk3229-evb.dts index 1eac37d..ae0b0a4 100644 --- a/arch/arm/dts/rk3229-evb.dts +++ b/arch/arm/dts/rk3229-evb.dts @@ -68,7 +68,6 @@

&emmc { u-boot,dm-pre-reloc;

• fifo-mode; status = "okay";

};

@@ -79,7 +78,6 @@ cap-sd-highspeed; card-detect-delay = <200>; disable-wp;

• max-frequency = <50000000>; num-slots = <1>; supports-sd;

}; diff --git a/arch/arm/dts/rk322x.dtsi b/arch/arm/dts/rk322x.dtsi index ddbe113..22324f9 100644 --- a/arch/arm/dts/rk322x.dtsi +++ b/arch/arm/dts/rk322x.dtsi @@ -388,6 +388,7 @@ sdmmc: dwmmc@30000000 { compatible = "rockchip,rk3228-dw-mshc", "rockchip,rk3288-dw-mshc"; reg = <0x30000000 0x4000>;

•    max-frequency = <150000000>;
   interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
   clocks = <&cru HCLK_SDMMC>, <&cru SCLK_SDMMC>,
        <&cru SCLK_SDMMC_DRV>, <&cru SCLK_SDMMC_SAMPLE>;
  

@@ -414,9 +415,8 @@ emmc: dwmmc@30020000 { compatible = "rockchip,rk3288-dw-mshc"; reg = <0x30020000 0x4000>;

•    max-frequency = <150000000>;
   interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
  

•    clock-frequency = <37500000>;
  

•    max-frequency = <37500000>;
   clocks = <&cru HCLK_EMMC>, <&cru SCLK_EMMC>,
        <&cru SCLK_EMMC_DRV>, <&cru SCLK_EMMC_SAMPLE>;
   clock-names = "biu", "ciu", "ciu_drv", "ciu_sample";
  
  

On Wed, 26 Jul 2017, Kever Yang wrote:

Use DIV_ROUND_UP instead RATE_TO_DIV for all Rockchip SoC clock driver.

I think nobody is happier than me to see this go ;-) However, there's a few comments below.

Signed-off-by: Kever Yang kever.yang@rock-chips.com Acked-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com

---

drivers/clk/rockchip/clk_rk3188.c | 7 ++----- drivers/clk/rockchip/clk_rk322x.c | 3 --- drivers/clk/rockchip/clk_rk3288.c | 5 +---- drivers/clk/rockchip/clk_rk3368.c | 7 ++----- drivers/clk/rockchip/clk_rk3399.c | 2 +- drivers/clk/rockchip/clk_rv1108.c | 3 --- 6 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/rockchip/clk_rk3188.c b/drivers/clk/rockchip/clk_rk3188.c index ecfd840..e1f0b65 100644 --- a/drivers/clk/rockchip/clk_rk3188.c +++ b/drivers/clk/rockchip/clk_rk3188.c @@ -71,9 +71,6 @@ enum { SOCSTS_GPLL_LOCK = 1 << 8, };

-#define RATE_TO_DIV(input_rate, output_rate) \

• ((input_rate) / (output_rate) - 1);

#define DIV_TO_RATE(input_rate, div) ((input_rate) / ((div) + 1))

#define PLL_DIVISORS(hz, _nr, _no) {\ @@ -350,7 +347,7 @@ static ulong rockchip_spi_get_clk(struct rk3188_cru *cru, uint gclk_rate, static ulong rockchip_spi_set_clk(struct rk3188_cru *cru, uint gclk_rate, int periph, uint freq) {

• int src_clk_div = RATE_TO_DIV(gclk_rate, freq);

• int src_clk_div = DIV_ROUND_UP(gclk_rate, freq) - 1;

  switch (periph) { case SCLK_SPI0:

@@ -400,7 +397,7 @@ static void rkclk_init(struct rk3188_cru *cru, struct rk3188_grf *grf, * reparent aclk_cpu_pre from apll to gpll * set up dependent divisors for PCLK/HCLK and ACLK clocks. */

• aclk_div = RATE_TO_DIV(GPLL_HZ, CPU_ACLK_HZ);

• aclk_div = DIV_ROUND_UP(GPLL_HZ, CPU_ACLK_HZ); assert((aclk_div + 1) * CPU_ACLK_HZ == GPLL_HZ && aclk_div < 0x1f);

  rk_clrsetreg(&cru->cru_clksel_con[0],

diff --git a/drivers/clk/rockchip/clk_rk322x.c b/drivers/clk/rockchip/clk_rk322x.c index b50a852..43b3f4b 100644 --- a/drivers/clk/rockchip/clk_rk322x.c +++ b/drivers/clk/rockchip/clk_rk322x.c @@ -26,9 +26,6 @@ enum { OUTPUT_MIN_HZ = 24 * 1000000, };

-#define RATE_TO_DIV(input_rate, output_rate) \

• ((input_rate) / (output_rate) - 1);

#define DIV_TO_RATE(input_rate, div) ((input_rate) / ((div) + 1))

#define PLL_DIVISORS(hz, _refdiv, _postdiv1, _postdiv2) {\ diff --git a/drivers/clk/rockchip/clk_rk3288.c b/drivers/clk/rockchip/clk_rk3288.c index adcc0a6..8f6df55 100644 --- a/drivers/clk/rockchip/clk_rk3288.c +++ b/drivers/clk/rockchip/clk_rk3288.c @@ -118,9 +118,6 @@ enum { SOCSTS_NPLL_LOCK = 1 << 9, };

-#define RATE_TO_DIV(input_rate, output_rate) \

• ((input_rate) / (output_rate) - 1);

#define DIV_TO_RATE(input_rate, div) ((input_rate) / ((div) + 1))

#define PLL_DIVISORS(hz, _nr, _no) {\ @@ -608,7 +605,7 @@ static ulong rockchip_spi_set_clk(struct rk3288_cru *cru, uint gclk_rate, int src_clk_div;

debug("%s: clk_general_rate=%u\n", __func__, gclk_rate);

• src_clk_div = RATE_TO_DIV(gclk_rate, freq);

• src_clk_div = DIV_ROUND_UP(gclk_rate, freq) - 1;

Shouldn't you check that this didn't overflow the div-field?

switch (periph) { case SCLK_SPI0: rk_clrsetreg(&cru->cru_clksel_con[25], diff --git a/drivers/clk/rockchip/clk_rk3368.c b/drivers/clk/rockchip/clk_rk3368.c index 52cad38..c067fa1 100644 --- a/drivers/clk/rockchip/clk_rk3368.c +++ b/drivers/clk/rockchip/clk_rk3368.c @@ -30,9 +30,6 @@ struct pll_div { #define GPLL_HZ (576 * 1000 * 1000) #define CPLL_HZ (400 * 1000 * 1000)

-#define RATE_TO_DIV(input_rate, output_rate) \

• ((input_rate) / (output_rate) - 1);
  

#define DIV_TO_RATE(input_rate, div) ((input_rate) / ((div) + 1))

#define PLL_DIVISORS(hz, _nr, _no) { \ @@ -171,7 +168,7 @@ static ulong rk3368_mmc_set_clk(struct rk3368_cru *cru, u32 con_id; u32 gpll_rate = rkclk_pll_get_rate(cru, GPLL);

• div = RATE_TO_DIV(gpll_rate, rate << 1);

• div = DIV_ROUND_UP(gpll_rate, rate << 1) - 1;

  switch (clk_id) { case SCLK_SDMMC:

@@ -188,7 +185,7 @@ static ulong rk3368_mmc_set_clk(struct rk3368_cru *cru, }

if (div > 0x3f) {

• div = RATE_TO_DIV(OSC_HZ, rate);
  

• div = DIV_ROUND_UP(OSC_HZ, rate) - 1;
  

This does not proect us against overflowing the div-field. Shouldn't you check whether the divider fits into the field (i.e. max is 0x3f) and clamp it (i.e. use saturated arithmetic)?

Let's assume someone requested a rate of 369kHz, then you'd set a divider or 1 (or 2?) instead of 64 (which is the closest representable one and would yield 375kHz).

rk_clrsetreg(&cru->clksel_con[con_id],
	     MMC_PLL_SEL_MASK | MMC_CLK_DIV_MASK,
	     (MMC_PLL_SEL_24M << MMC_PLL_SEL_SHIFT) |

diff --git a/drivers/clk/rockchip/clk_rk3399.c b/drivers/clk/rockchip/clk_rk3399.c index 40cbfea..e3f663a 100644 --- a/drivers/clk/rockchip/clk_rk3399.c +++ b/drivers/clk/rockchip/clk_rk3399.c @@ -676,7 +676,7 @@ static ulong rk3399_spi_set_clk(struct rk3399_cru *cru, ulong clk_id, uint hz) const struct spi_clkreg *spiclk = NULL; int src_clk_div;

• src_clk_div = RATE_TO_DIV(GPLL_HZ, hz);

• src_clk_div = DIV_ROUND_UP(GPLL_HZ, hz) - 1; assert(src_clk_div < 127);

  switch (clk_id) {

diff --git a/drivers/clk/rockchip/clk_rv1108.c b/drivers/clk/rockchip/clk_rv1108.c index 818293d..cf966bb 100644 --- a/drivers/clk/rockchip/clk_rv1108.c +++ b/drivers/clk/rockchip/clk_rv1108.c @@ -25,9 +25,6 @@ enum { OUTPUT_MIN_HZ = 24 * 1000000, };

-#define RATE_TO_DIV(input_rate, output_rate) \

• ((input_rate) / (output_rate) - 1);

#define DIV_TO_RATE(input_rate, div) ((input_rate) / ((div) + 1))

#define PLL_DIVISORS(hz, _refdiv, _postdiv1, _postdiv2) {\

On 27 Jul 2017, at 04:33, Kever Yang kever.yang@rock-chips.com wrote:

Hi Philipp,

On 07/27/2017 01:40 AM, Philipp Tomsich wrote:

...

Kever,

On Wed, 26 Jul 2017, Kever Yang wrote:

...

Lets set the all the DDR region as non secure in SPL, the trust like OPTEE should have the correct setting for it if there is one.

Signed-off-by: Kever Yang kever.yang@rock-chips.com Acked-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com

---

arch/arm/mach-rockchip/rk322x-board-spl.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/arch/arm/mach-rockchip/rk322x-board-spl.c b/arch/arm/mach-rockchip/rk322x-board-spl.c index 15216c7..aa8a742 100644 --- a/arch/arm/mach-rockchip/rk322x-board-spl.c +++ b/arch/arm/mach-rockchip/rk322x-board-spl.c @@ -41,6 +41,8 @@ static struct rk322x_grf * const grf = (void *)GRF_BASE; CON_IOMUX_UART2SEL_MASK, CON_IOMUX_UART2SEL_21 << CON_IOMUX_UART2SEL_SHIFT); }

+#define SGRF_DDR_CON0 0x10150000

Please use a typed const-variable (e.g. 'const u32 *') in the function where this is needed.

I must have been tired yesterday: I meant 'u32 * const' (i.e. a constant-pointer to a mutable u32).

I don't understand why I can't use the macro directly and it's definitely const.

Having a macro (pasting an integer literal) does not convey the same type information and can not be local to a single function. By making this a constant, you’ll get full use of the typesystem (and there’s no implicit conversion from an integer literal when calling rk_clrreg). Additionally, the debug-info will contain the constant’s name.

This is similar to why Simon was moving towards enums wherever he could… only that I prefer the use of constants, as that provides the most accurate typeinfo.

...

...

void board_init_f(ulong dummy) { struct udevice *dev; @@ -71,6 +73,7 @@ void board_init_f(ulong dummy) return; }

• rk_clrreg(SGRF_DDR_CON0, 0x4000);

Could you add a comment here that explains what is set and what the meanings of these bits are? I think I've seen a comment somewhere (ATF?) explaining that the '4' comes from enabling some sort of bypass, but it would be great to be able to just look here and immediately understand what this line does...

Well, I will add comments here and also update the commit message.

Much appreciated.

Thanks,

• Kever

...

Also (without any comment explaining what goes on), it feels a bit odd that 'set all to non-secure' does not simply zero all bits.

...

#if defined(CONFIG_ROCKCHIP_SPL_BACK_TO_BROM) && !defined(CONFIG_SPL_BOARD_INIT) back_to_bootrom(); #endif