[U-Boot] [PATCH 1/4] rockchip: rk322x: set the DDR region as non-secure in SPL

[U-Boot] [PATCH 1/4] rockchip: rk322x: set the DDR region as non-secure in SPL

[Kever Yang]

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>
---

 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
 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);
 #if defined(CONFIG_ROCKCHIP_SPL_BACK_TO_BROM) && !defined(CONFIG_SPL_BOARD_INIT)
 	back_to_bootrom();
 #endif
-- 
1.9.1

[U-Boot] [PATCH 2/4] rockchip: rk322x: update dts node for mmc

[Kever Yang]

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

Signed-off-by: Kever Yang <kever.yang@rock-chips.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 at 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 at 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";
-- 
1.9.1

[U-Boot] [U-Boot, 2/4] rockchip: rk322x: update dts node for mmc

[Philipp Tomsich]

> mmc using 150000000 as max-frequency and do not use fifo-mode.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
> 
>  arch/arm/dts/rk3229-evb.dts | 2 --
>  arch/arm/dts/rk322x.dtsi    | 4 ++--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 

Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

[U-Boot] [U-Boot, 2/4] rockchip: rk322x: update dts node for mmc

[Philipp Tomsich]

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 at 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 at 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";
>

[U-Boot] [U-Boot, 2/4] rockchip: rk322x: update dts node for mmc

[Kever Yang]

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.

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 at 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 at 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";
>>
>

[U-Boot] [U-Boot, 2/4] rockchip: rk322x: update dts node for mmc

[Dr. Philipp Tomsich]

> 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 at 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 at 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";
>>> 
>> 
> 
> 

[U-Boot] [PATCH 3/4] rockchip: clk: update dwmmc clock div

[Kever Yang]

dwmmc controller has default internal divider by 2,
sync code for all Rockchip SoC with:
4055b46 rockchip: clk: rk3288: fix mmc clock setting

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 drivers/clk/rockchip/clk_rk3036.c |  6 +++---
 drivers/clk/rockchip/clk_rk3188.c |  4 ++--
 drivers/clk/rockchip/clk_rk322x.c |  6 +++---
 drivers/clk/rockchip/clk_rk3328.c |  8 ++++----
 drivers/clk/rockchip/clk_rk3399.c | 11 +++++++----
 5 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/rockchip/clk_rk3036.c b/drivers/clk/rockchip/clk_rk3036.c
index 5ecf512..514ea88 100644
--- a/drivers/clk/rockchip/clk_rk3036.c
+++ b/drivers/clk/rockchip/clk_rk3036.c
@@ -235,7 +235,7 @@ static ulong rockchip_mmc_get_clk(struct rk3036_cru *cru, uint clk_general_rate,
 	}
 
 	src_rate = mux == EMMC_SEL_24M ? OSC_HZ : clk_general_rate;
-	return DIV_TO_RATE(src_rate, div);
+	return DIV_TO_RATE(src_rate, div) / 2;
 }
 
 static ulong rockchip_mmc_set_clk(struct rk3036_cru *cru, uint clk_general_rate,
@@ -247,10 +247,10 @@ static ulong rockchip_mmc_set_clk(struct rk3036_cru *cru, uint clk_general_rate,
 	debug("%s: clk_general_rate=%u\n", __func__, clk_general_rate);
 
 	/* mmc clock auto divide 2 in internal */
-	src_clk_div = (clk_general_rate / 2 + freq - 1) / freq;
+	src_clk_div = DIV_ROUND_UP(clk_general_rate / 2, freq);
 
 	if (src_clk_div > 0x7f) {
-		src_clk_div = (OSC_HZ / 2 + freq - 1) / freq;
+		src_clk_div = DIV_ROUND_UP(OSC_HZ / 2, freq);
 		mux = EMMC_SEL_24M;
 	} else {
 		mux = EMMC_SEL_GPLL;
diff --git a/drivers/clk/rockchip/clk_rk3188.c b/drivers/clk/rockchip/clk_rk3188.c
index 6f30332..ecfd840 100644
--- a/drivers/clk/rockchip/clk_rk3188.c
+++ b/drivers/clk/rockchip/clk_rk3188.c
@@ -287,7 +287,7 @@ static ulong rockchip_mmc_get_clk(struct rk3188_cru *cru, uint gclk_rate,
 		return -EINVAL;
 	}
 
-	return DIV_TO_RATE(gclk_rate, div);
+	return DIV_TO_RATE(gclk_rate, div) / 2;
 }
 
 static ulong rockchip_mmc_set_clk(struct rk3188_cru *cru, uint gclk_rate,
@@ -296,7 +296,7 @@ static ulong rockchip_mmc_set_clk(struct rk3188_cru *cru, uint gclk_rate,
 	int src_clk_div;
 
 	debug("%s: gclk_rate=%u\n", __func__, gclk_rate);
-	src_clk_div = RATE_TO_DIV(gclk_rate, freq);
+	src_clk_div = DIV_ROUND_UP(gclk_rate / 2, freq);
 	assert(src_clk_div <= 0x3f);
 
 	switch (periph) {
diff --git a/drivers/clk/rockchip/clk_rk322x.c b/drivers/clk/rockchip/clk_rk322x.c
index fdeb816..b50a852 100644
--- a/drivers/clk/rockchip/clk_rk322x.c
+++ b/drivers/clk/rockchip/clk_rk322x.c
@@ -239,7 +239,7 @@ static ulong rockchip_mmc_get_clk(struct rk322x_cru *cru, uint clk_general_rate,
 	}
 
 	src_rate = mux == EMMC_SEL_24M ? OSC_HZ : clk_general_rate;
-	return DIV_TO_RATE(src_rate, div);
+	return DIV_TO_RATE(src_rate, div) / 2;
 }
 
 static ulong rockchip_mmc_set_clk(struct rk322x_cru *cru, uint clk_general_rate,
@@ -251,10 +251,10 @@ static ulong rockchip_mmc_set_clk(struct rk322x_cru *cru, uint clk_general_rate,
 	debug("%s: clk_general_rate=%u\n", __func__, clk_general_rate);
 
 	/* mmc clock auto divide 2 in internal */
-	src_clk_div = (clk_general_rate / 2 + freq - 1) / freq;
+	src_clk_div = DIV_ROUND_UP(clk_general_rate / 2, freq);
 
 	if (src_clk_div > 0x7f) {
-		src_clk_div = (OSC_HZ / 2 + freq - 1) / freq;
+		src_clk_div = DIV_ROUND_UP(OSC_HZ / 2, freq);
 		mux = EMMC_SEL_24M;
 	} else {
 		mux = EMMC_SEL_GPLL;
diff --git a/drivers/clk/rockchip/clk_rk3328.c b/drivers/clk/rockchip/clk_rk3328.c
index 2065a8a..c1a23ba 100644
--- a/drivers/clk/rockchip/clk_rk3328.c
+++ b/drivers/clk/rockchip/clk_rk3328.c
@@ -412,9 +412,9 @@ static ulong rk3328_mmc_get_clk(struct rk3328_cru *cru, uint clk_id)
 
 	if ((con & CLK_EMMC_PLL_MASK) >> CLK_EMMC_PLL_SHIFT
 	    == CLK_EMMC_PLL_SEL_24M)
-		return DIV_TO_RATE(OSC_HZ, div);
+		return DIV_TO_RATE(OSC_HZ, div) / 2;
 	else
-		return DIV_TO_RATE(GPLL_HZ, div);
+		return DIV_TO_RATE(GPLL_HZ, div) / 2;
 }
 
 static ulong rk3328_mmc_set_clk(struct rk3328_cru *cru,
@@ -436,11 +436,11 @@ static ulong rk3328_mmc_set_clk(struct rk3328_cru *cru,
 		return -EINVAL;
 	}
 	/* Select clk_sdmmc/emmc source from GPLL by default */
-	src_clk_div = GPLL_HZ / set_rate;
+	src_clk_div = DIV_ROUND_UP(GPLL_HZ / 2, set_rate);
 
 	if (src_clk_div > 127) {
 		/* use 24MHz source for 400KHz clock */
-		src_clk_div = OSC_HZ / set_rate;
+		src_clk_div = DIV_ROUND_UP(OSC_HZ / 2, set_rate);
 		rk_clrsetreg(&cru->clksel_con[con_id],
 			     CLK_EMMC_PLL_MASK | CLK_EMMC_DIV_CON_MASK,
 			     CLK_EMMC_PLL_SEL_24M << CLK_EMMC_PLL_SHIFT |
diff --git a/drivers/clk/rockchip/clk_rk3399.c b/drivers/clk/rockchip/clk_rk3399.c
index 54079cd..40cbfea 100644
--- a/drivers/clk/rockchip/clk_rk3399.c
+++ b/drivers/clk/rockchip/clk_rk3399.c
@@ -750,18 +750,21 @@ static ulong rk3399_mmc_get_clk(struct rk3399_cru *cru, uint clk_id)
 	case HCLK_SDMMC:
 	case SCLK_SDMMC:
 		con = readl(&cru->clksel_con[16]);
+		/* dwmmc controller have internal div 2 */
+		div = 2;
 		break;
 	case SCLK_EMMC:
 		con = readl(&cru->clksel_con[21]);
+		div = 1;
 		break;
 	default:
 		return -EINVAL;
 	}
-	div = (con & CLK_EMMC_DIV_CON_MASK) >> CLK_EMMC_DIV_CON_SHIFT;
 
+	div *= (con & CLK_EMMC_DIV_CON_MASK) >> CLK_EMMC_DIV_CON_SHIFT;
 	if ((con & CLK_EMMC_PLL_MASK) >> CLK_EMMC_PLL_SHIFT
 			== CLK_EMMC_PLL_SEL_24M)
-		return DIV_TO_RATE(24*1000*1000, div);
+		return DIV_TO_RATE(OSC_HZ, div);
 	else
 		return DIV_TO_RATE(GPLL_HZ, div);
 }
@@ -776,11 +779,11 @@ static ulong rk3399_mmc_set_clk(struct rk3399_cru *cru,
 	case HCLK_SDMMC:
 	case SCLK_SDMMC:
 		/* Select clk_sdmmc source from GPLL by default */
-		src_clk_div = GPLL_HZ / set_rate;
+		src_clk_div = DIV_ROUND_UP(GPLL_HZ / 2, set_rate);
 
 		if (src_clk_div > 127) {
 			/* use 24MHz source for 400KHz clock */
-			src_clk_div = 24*1000*1000 / set_rate;
+			src_clk_div = DIV_ROUND_UP(OSC_HZ / 2, set_rate);
 			rk_clrsetreg(&cru->clksel_con[16],
 				     CLK_EMMC_PLL_MASK | CLK_EMMC_DIV_CON_MASK,
 				     CLK_EMMC_PLL_SEL_24M << CLK_EMMC_PLL_SHIFT |
-- 
1.9.1

[U-Boot] [U-Boot,3/4] rockchip: clk: update dwmmc clock div

[Philipp Tomsich]

> dwmmc controller has default internal divider by 2,
> sync code for all Rockchip SoC with:
> 4055b46 rockchip: clk: rk3288: fix mmc clock setting
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
> 
>  drivers/clk/rockchip/clk_rk3036.c |  6 +++---
>  drivers/clk/rockchip/clk_rk3188.c |  4 ++--
>  drivers/clk/rockchip/clk_rk322x.c |  6 +++---
>  drivers/clk/rockchip/clk_rk3328.c |  8 ++++----
>  drivers/clk/rockchip/clk_rk3399.c | 11 +++++++----
>  5 files changed, 19 insertions(+), 16 deletions(-)
> 

Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

[U-Boot] [U-Boot,3/4] rockchip: clk: update dwmmc clock div

[Philipp Tomsich]

On Wed, 26 Jul 2017, Kever Yang wrote:

> dwmmc controller has default internal divider by 2,
> sync code for all Rockchip SoC with:
> 4055b46 rockchip: clk: rk3288: fix mmc clock setting

While I know that this is the case (i.e. we measured the output 
frequencies a while back), we should add some  documentation/comment to 
explain where this divider comes from: I didn't see this divider 
documented in the CRU and the DW-MMC states that setting the SDMMC_CLKDIV 
to 0 is 'no division, bypass'.

The reason why I am picky about clearly documenting this is that we need 
to either treat this a a div-2 between the CRU and the DWMMC or capture 
the fact that the SDMMC_CLKDIV of 0 does not mean 'bypass' (in which case 
we should double-check that the generic designware MMC driver does not 
make assumptions that do not apply to us).

> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
> drivers/clk/rockchip/clk_rk3036.c |  6 +++---
> drivers/clk/rockchip/clk_rk3188.c |  4 ++--
> drivers/clk/rockchip/clk_rk322x.c |  6 +++---
> drivers/clk/rockchip/clk_rk3328.c |  8 ++++----
> drivers/clk/rockchip/clk_rk3399.c | 11 +++++++----
> 5 files changed, 19 insertions(+), 16 deletions(-)

[U-Boot] [U-Boot,3/4] rockchip: clk: update dwmmc clock div

[Kever Yang]

Philipp,


On 07/27/2017 02:01 AM, Philipp Tomsich wrote:
>
>
> On Wed, 26 Jul 2017, Kever Yang wrote:
>
>> dwmmc controller has default internal divider by 2,
>> sync code for all Rockchip SoC with:
>> 4055b46 rockchip: clk: rk3288: fix mmc clock setting
>
> While I know that this is the case (i.e. we measured the output 
> frequencies a while back), we should add some documentation/comment to 
> explain where this divider comes from: I didn't see this divider 
> documented in the CRU and the DW-MMC states that setting the 
> SDMMC_CLKDIV to 0 is 'no division, bypass'.
>
The div 2 happens inside the DWMMC, but we treat it happen between CRU 
and DW-MMC,
so we provide double of the clock rate of the mmc driver required.
rk3036 and rk3288 already have this setting in clock driver and rk3036 
do have a comment for it.
I can add comments for all other SoCs.

Thanks,
- Kever
> The reason why I am picky about clearly documenting this is that we 
> need to either treat this a a div-2 between the CRU and the DWMMC or 
> capture the fact that the SDMMC_CLKDIV of 0 does not mean 'bypass' (in 
> which case we should double-check that the generic designware MMC 
> driver does not make assumptions that do not apply to us).
>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>>
>> drivers/clk/rockchip/clk_rk3036.c |  6 +++---
>> drivers/clk/rockchip/clk_rk3188.c |  4 ++--
>> drivers/clk/rockchip/clk_rk322x.c |  6 +++---
>> drivers/clk/rockchip/clk_rk3328.c |  8 ++++----
>> drivers/clk/rockchip/clk_rk3399.c | 11 +++++++----
>> 5 files changed, 19 insertions(+), 16 deletions(-)
>

[U-Boot] [U-Boot,3/4] rockchip: clk: update dwmmc clock div

[Dr. Philipp Tomsich]

> On 27 Jul 2017, at 05:29, Kever Yang <kever.yang@rock-chips.com> wrote:
> 
> Philipp,
> 
> 
> On 07/27/2017 02:01 AM, Philipp Tomsich wrote:
>> 
>> 
>> On Wed, 26 Jul 2017, Kever Yang wrote:
>> 
>>> dwmmc controller has default internal divider by 2,
>>> sync code for all Rockchip SoC with:
>>> 4055b46 rockchip: clk: rk3288: fix mmc clock setting
>> 
>> While I know that this is the case (i.e. we measured the output frequencies a while back), we should add some documentation/comment to explain where this divider comes from: I didn't see this divider documented in the CRU and the DW-MMC states that setting the SDMMC_CLKDIV to 0 is 'no division, bypass'.
>> 
> The div 2 happens inside the DWMMC, but we treat it happen between CRU and DW-MMC,
> so we provide double of the clock rate of the mmc driver required.
> rk3036 and rk3288 already have this setting in clock driver and rk3036 do have a comment for it.
> I can add comments for all other SoCs.

A comment would be appreciated: we spent some time on this both on the RK3399 and RK3368… and I am pretty certain that everyone touching this in the future will also wonder about it.

> Thanks,
> - Kever
>> The reason why I am picky about clearly documenting this is that we need to either treat this a a div-2 between the CRU and the DWMMC or capture the fact that the SDMMC_CLKDIV of 0 does not mean 'bypass' (in which case we should double-check that the generic designware MMC driver does not make assumptions that do not apply to us).
>> 
>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>> ---
>>> 
>>> drivers/clk/rockchip/clk_rk3036.c |  6 +++---
>>> drivers/clk/rockchip/clk_rk3188.c |  4 ++--
>>> drivers/clk/rockchip/clk_rk322x.c |  6 +++---
>>> drivers/clk/rockchip/clk_rk3328.c |  8 ++++----
>>> drivers/clk/rockchip/clk_rk3399.c | 11 +++++++----
>>> 5 files changed, 19 insertions(+), 16 deletions(-)
>> 
> 
> 

[U-Boot] [PATCH 4/4] rockchip: clk: remove RATE_TO_DIV

[Kever Yang]

Use DIV_ROUND_UP instead RATE_TO_DIV for all Rockchip SoC
clock driver.

Signed-off-by: Kever Yang <kever.yang@rock-chips.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;
 	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;
 		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) {\
-- 
1.9.1

[U-Boot] [U-Boot,4/4] rockchip: clk: remove RATE_TO_DIV

[Philipp Tomsich]

> Use DIV_ROUND_UP instead RATE_TO_DIV for all Rockchip SoC
> clock driver.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.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(-)
> 

Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

[U-Boot] [U-Boot,4/4] rockchip: clk: remove RATE_TO_DIV

[Philipp Tomsich]

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) {\
>

[U-Boot] [U-Boot, 1/4] rockchip: rk322x: set the DDR region as non-secure in SPL

[Philipp Tomsich]

> 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>
> ---
> 
>  arch/arm/mach-rockchip/rk322x-board-spl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

[U-Boot] [U-Boot, 1/4] rockchip: rk322x: set the DDR region as non-secure in SPL

[Philipp Tomsich]

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.

> 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...

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
>

[U-Boot] [U-Boot, 1/4] rockchip: rk322x: set the DDR region as non-secure in SPL

[Kever Yang]

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 don't understand why I can't use the macro directly and it's 
definitely const.
>
>> 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.

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
>>
>

[U-Boot] [U-Boot, 1/4] rockchip: rk322x: set the DDR region as non-secure in SPL

[Dr. Philipp Tomsich]

> 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