Re: [PATCH 4/8] sunxi: Convert H616 DRAM options to single setting

Andre Przywara
2022-12-12T17:51:01Z

On Sun, 11 Dec 2022 17:32:09 +0100
Jernej Skrabec <jernej.skrabec@gmail.com> wrote:

Hi,

> Vendor DRAM settings use TPR10 parameter to enable various features.
> There are many mores features that just those that are currently
> mentioned. Since new will be added later and most are not known, let's
> reuse value from vendor DRAM driver as-is. This will also help adding
> support for new boards.

Looks good. I checked that the actual code in
mctl_phy_bit_delay_compensation() just gets indented (no real changes), and
is now guarded by the new checks.
The values in the _defconfig still result in the same decisions, though
many are blocked anyway by no one actually calling that bit_delay function
at all. (I wonder what happens when we enable it?)
One small thing below.

> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
> .../include/asm/arch-sunxi/dram_sun50i_h616.h | 10 +
> arch/arm/mach-sunxi/Kconfig | 38 +---
> arch/arm/mach-sunxi/dram_sun50i_h616.c | 197 +++++++++---------
> configs/orangepi_zero2_defconfig | 5 +-
> 4 files changed, 117 insertions(+), 133 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch-sunxi/dram_sun50i_h616.h b/arch/arm/include/asm/arch-sunxi/dram_sun50i_h616.h
> index c9e1f84bfcdd..b5140c79b70e 100644
> --- a/arch/arm/include/asm/arch-sunxi/dram_sun50i_h616.h
> +++ b/arch/arm/include/asm/arch-sunxi/dram_sun50i_h616.h
> @@ -137,6 +137,15 @@ check_member(sunxi_mctl_ctl_reg, unk_0x4240, 0x4240);
> #define MSTR_ACTIVE_RANKS(x) (((x == 2) ? 3 : 1) << 24)
> #define MSTR_BURST_LENGTH(x) (((x) >> 1) << 16)
>
> +/* TODO: figure out what unknown features do */
> +#define TPR10_UNKNOWN_FEAT0 BIT(16)
> +#define TPR10_UNKNOWN_FEAT1 BIT(17)
> +#define TPR10_UNKNOWN_FEAT2 BIT(18)

Could you just put the bit number in the macro? Like TPR10_BIT16? That
both avoids inventing meaningless names, and helps readers to correlate
code more easily. Or skip the symbol altogether for those and use just
"BIT(16)" in the code.

Cheers,
Andre

> +#define TPR10_WRITE_LEVELING BIT(20)
> +#define TPR10_READ_CALIBRATION BIT(21)
> +#define TPR10_READ_TRAINING BIT(22)
> +#define TPR10_WRITE_TRAINING BIT(23)
> +
> struct dram_para {
> u32 clk;
> enum sunxi_dram_type type;
> @@ -147,6 +156,7 @@ struct dram_para {
> u32 dx_odt;
> u32 dx_dri;
> u32 ca_dri;
> + u32 tpr10;
> };
>
>
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index cad53f19912c..abcbd0fb9061 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -52,38 +52,6 @@ config DRAM_SUN50I_H616
> like H616.
>
> if DRAM_SUN50I_H616
> -config DRAM_SUN50I_H616_WRITE_LEVELING
> - bool "H616 DRAM write leveling"
> - ---help---
> - Select this when DRAM on your H616 board needs write leveling.
> -
> -config DRAM_SUN50I_H616_READ_CALIBRATION
> - bool "H616 DRAM read calibration"
> - ---help---
> - Select this when DRAM on your H616 board needs read calibration.
> -
> -config DRAM_SUN50I_H616_READ_TRAINING
> - bool "H616 DRAM read training"
> - ---help---
> - Select this when DRAM on your H616 board needs read training.
> -
> -config DRAM_SUN50I_H616_WRITE_TRAINING
> - bool "H616 DRAM write training"
> - ---help---
> - Select this when DRAM on your H616 board needs write training.
> -
> -config DRAM_SUN50I_H616_BIT_DELAY_COMPENSATION
> - bool "H616 DRAM bit delay compensation"
> - ---help---
> - Select this when DRAM on your H616 board needs bit delay
> - compensation.
> -
> -config DRAM_SUN50I_H616_UNKNOWN_FEATURE
> - bool "H616 DRAM unknown feature"
> - ---help---
> - Select this when DRAM on your H616 board needs this unknown
> - feature.
> -
> config DRAM_SUN50I_H616_DX_ODT
> hex "H616 DRAM DX ODT parameter"
> help
> @@ -98,6 +66,12 @@ config DRAM_SUN50I_H616_CA_DRI
> hex "H616 DRAM CA DRI parameter"
> help
> CA DRI value from vendor DRAM settings.
> +
> +config DRAM_SUN50I_H616_TPR10
> + hex "H616 DRAM TPR10 parameter"
> + help
> + TPR10 value from vendor DRAM settings. It tells which features
> + should be configured, like write leveling, read calibration, etc.
> endif
>
> config SUN6I_PRCM
> diff --git a/arch/arm/mach-sunxi/dram_sun50i_h616.c b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> index 06a07dfbf9cc..14a01a3c4e54 100644
> --- a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> +++ b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> @@ -577,109 +577,112 @@ static bool mctl_phy_bit_delay_compensation(struct dram_para *para)
> u32 *ptr;
> int i;
>
> - clrbits_le32(SUNXI_DRAM_PHY0_BASE + 0x60, 1);
> - setbits_le32(SUNXI_DRAM_PHY0_BASE + 8, 8);
> - clrbits_le32(SUNXI_DRAM_PHY0_BASE + 0x190, 0x10);
> + if (para->tpr10 & TPR10_UNKNOWN_FEAT2) {
> + clrbits_le32(SUNXI_DRAM_PHY0_BASE + 0x60, 1);
> + setbits_le32(SUNXI_DRAM_PHY0_BASE + 8, 8);
> + clrbits_le32(SUNXI_DRAM_PHY0_BASE + 0x190, 0x10);
>
> - ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x484);
> - for (i = 0; i < 9; i++) {
> - writel_relaxed(0x16, ptr);
> - writel_relaxed(0x16, ptr + 0x30);
> - ptr += 2;
> - }
> - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x4d0);
> - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x590);
> - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x4cc);
> - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x58c);
> + ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x484);
> + for (i = 0; i < 9; i++) {
> + writel_relaxed(0x16, ptr);
> + writel_relaxed(0x16, ptr + 0x30);
> + ptr += 2;
> + }
> + writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x4d0);
> + writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x590);
> + writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x4cc);
> + writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x58c);
>
> - ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x4d8);
> - for (i = 0; i < 9; i++) {
> - writel_relaxed(0x1a, ptr);
> - writel_relaxed(0x1a, ptr + 0x30);
> - ptr += 2;
> - }
> - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x524);
> - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x5e4);
> - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x520);
> - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x5e0);
> + ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x4d8);
> + for (i = 0; i < 9; i++) {
> + writel_relaxed(0x1a, ptr);
> + writel_relaxed(0x1a, ptr + 0x30);
> + ptr += 2;
> + }
> + writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x524);
> + writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x5e4);
> + writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x520);
> + writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x5e0);
>
> - ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x604);
> - for (i = 0; i < 9; i++) {
> - writel_relaxed(0x1a, ptr);
> - writel_relaxed(0x1a, ptr + 0x30);
> - ptr += 2;
> - }
> - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x650);
> - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x710);
> - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x64c);
> - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x70c);
> + ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x604);
> + for (i = 0; i < 9; i++) {
> + writel_relaxed(0x1a, ptr);
> + writel_relaxed(0x1a, ptr + 0x30);
> + ptr += 2;
> + }
> + writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x650);
> + writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x710);
> + writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x64c);
> + writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x70c);
>
> - ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x658);
> - for (i = 0; i < 9; i++) {
> - writel_relaxed(0x1a, ptr);
> - writel_relaxed(0x1a, ptr + 0x30);
> - ptr += 2;
> - }
> - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x6a4);
> - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x764);
> - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x6a0);
> - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x760);
> + ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x658);
> + for (i = 0; i < 9; i++) {
> + writel_relaxed(0x1a, ptr);
> + writel_relaxed(0x1a, ptr + 0x30);
> + ptr += 2;
> + }
> + writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x6a4);
> + writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x764);
> + writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x6a0);
> + writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x760);
>
> - dmb();
> + dmb();
>
> - setbits_le32(SUNXI_DRAM_PHY0_BASE + 0x60, 1);
> + setbits_le32(SUNXI_DRAM_PHY0_BASE + 0x60, 1);
> + }
>
> - /* second part */
> - clrbits_le32(SUNXI_DRAM_PHY0_BASE + 0x54, 0x80);
> - clrbits_le32(SUNXI_DRAM_PHY0_BASE + 0x190, 4);
> + if (para->tpr10 & TPR10_UNKNOWN_FEAT1) {
> + clrbits_le32(SUNXI_DRAM_PHY0_BASE + 0x54, 0x80);
> + clrbits_le32(SUNXI_DRAM_PHY0_BASE + 0x190, 4);
>
> - ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x480);
> - for (i = 0; i < 9; i++) {
> - writel_relaxed(0x10, ptr);
> - writel_relaxed(0x10, ptr + 0x30);
> - ptr += 2;
> - }
> - writel_relaxed(0x18, SUNXI_DRAM_PHY0_BASE + 0x528);
> - writel_relaxed(0x18, SUNXI_DRAM_PHY0_BASE + 0x5e8);
> - writel_relaxed(0x18, SUNXI_DRAM_PHY0_BASE + 0x4c8);
> - writel_relaxed(0x18, SUNXI_DRAM_PHY0_BASE + 0x588);
> + ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x480);
> + for (i = 0; i < 9; i++) {
> + writel_relaxed(0x10, ptr);
> + writel_relaxed(0x10, ptr + 0x30);
> + ptr += 2;
> + }
> + writel_relaxed(0x18, SUNXI_DRAM_PHY0_BASE + 0x528);
> + writel_relaxed(0x18, SUNXI_DRAM_PHY0_BASE + 0x5e8);
> + writel_relaxed(0x18, SUNXI_DRAM_PHY0_BASE + 0x4c8);
> + writel_relaxed(0x18, SUNXI_DRAM_PHY0_BASE + 0x588);
>
> - ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x4d4);
> - for (i = 0; i < 9; i++) {
> - writel_relaxed(0x12, ptr);
> - writel_relaxed(0x12, ptr + 0x30);
> - ptr += 2;
> - }
> - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x52c);
> - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x5ec);
> - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x51c);
> - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x5dc);
> + ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x4d4);
> + for (i = 0; i < 9; i++) {
> + writel_relaxed(0x12, ptr);
> + writel_relaxed(0x12, ptr + 0x30);
> + ptr += 2;
> + }
> + writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x52c);
> + writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x5ec);
> + writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x51c);
> + writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x5dc);
>
> - ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x600);
> - for (i = 0; i < 9; i++) {
> - writel_relaxed(0x12, ptr);
> - writel_relaxed(0x12, ptr + 0x30);
> - ptr += 2;
> - }
> - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x6a8);
> - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x768);
> - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x648);
> - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x708);
> + ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x600);
> + for (i = 0; i < 9; i++) {
> + writel_relaxed(0x12, ptr);
> + writel_relaxed(0x12, ptr + 0x30);
> + ptr += 2;
> + }
> + writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x6a8);
> + writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x768);
> + writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x648);
> + writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x708);
>
> - ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x654);
> - for (i = 0; i < 9; i++) {
> - writel_relaxed(0x14, ptr);
> - writel_relaxed(0x14, ptr + 0x30);
> - ptr += 2;
> - }
> - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x6ac);
> - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x76c);
> - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x69c);
> - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x75c);
> + ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x654);
> + for (i = 0; i < 9; i++) {
> + writel_relaxed(0x14, ptr);
> + writel_relaxed(0x14, ptr + 0x30);
> + ptr += 2;
> + }
> + writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x6ac);
> + writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x76c);
> + writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x69c);
> + writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x75c);
>
> - dmb();
> + dmb();
>
> - setbits_le32(SUNXI_DRAM_PHY0_BASE + 0x54, 0x80);
> + setbits_le32(SUNXI_DRAM_PHY0_BASE + 0x54, 0x80);
> + }
>
> return true;
> }
> @@ -718,7 +721,7 @@ static bool mctl_phy_init(struct dram_para *para)
> for (i = 0; i < ARRAY_SIZE(phy_init); i++)
> writel(phy_init[i], &ptr[i]);
>
> - if (IS_ENABLED(CONFIG_DRAM_SUN50I_H616_UNKNOWN_FEATURE)) {
> + if (para->tpr10 & TPR10_UNKNOWN_FEAT0) {
> ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x780);
> for (i = 0; i < 32; i++)
> writel(0x16, &ptr[i]);
> @@ -800,7 +803,7 @@ static bool mctl_phy_init(struct dram_para *para)
> clrbits_le32(&mctl_ctl->rfshctl3, 1);
> writel(1, &mctl_ctl->swctl);
>
> - if (IS_ENABLED(CONFIG_DRAM_SUN50I_H616_WRITE_LEVELING)) {
> + if (para->tpr10 & TPR10_WRITE_LEVELING) {
> for (i = 0; i < 5; i++)
> if (mctl_phy_write_leveling(para))
> break;
> @@ -810,7 +813,7 @@ static bool mctl_phy_init(struct dram_para *para)
> }
> }
>
> - if (IS_ENABLED(CONFIG_DRAM_SUN50I_H616_READ_CALIBRATION)) {
> + if (para->tpr10 & TPR10_READ_CALIBRATION) {
> for (i = 0; i < 5; i++)
B> if (mctl_phy_read_calibration(para))
> break;
> @@ -820,7 +823,7 @@ static bool mctl_phy_init(struct dram_para *para)
> }
> }
>
> - if (IS_ENABLED(CONFIG_DRAM_SUN50I_H616_READ_TRAINING)) {
> + if (para->tpr10 & TPR10_READ_TRAINING) {
> for (i = 0; i < 5; i++)
> if (mctl_phy_read_training(para))
> break;
> @@ -830,7 +833,7 @@ static bool mctl_phy_init(struct dram_para *para)
> }
> }
>
> - if (IS_ENABLED(CONFIG_DRAM_SUN50I_H616_WRITE_TRAINING)) {
> + if (para->tpr10 & TPR10_WRITE_TRAINING) {
> for (i = 0; i < 5; i++)
> if (mctl_phy_write_training(para))
> break;
> @@ -840,8 +843,7 @@ static bool mctl_phy_init(struct dram_para *para)
> }
> }
>
> - if (IS_ENABLED(CONFIG_DRAM_SUN50I_H616_BIT_DELAY_COMPENSATION))
> - mctl_phy_bit_delay_compensation(para);
> + mctl_phy_bit_delay_compensation(para);
>
> clrbits_le32(SUNXI_DRAM_PHY0_BASE + 0x60, 4);
>
> @@ -1022,6 +1024,7 @@ unsigned long sunxi_dram_init(void)
> .dx_odt = CONFIG_DRAM_SUN50I_H616_DX_ODT,
> .dx_dri = CONFIG_DRAM_SUN50I_H616_DX_DRI,
> .ca_dri = CONFIG_DRAM_SUN50I_H616_CA_DRI,
> + .tpr10 = CONFIG_DRAM_SUN50I_H616_TPR10,
> };
> unsigned long size;
>
> diff --git a/configs/orangepi_zero2_defconfig b/configs/orangepi_zero2_defconfig
> index ca398faef1d3..f2024a2502c7 100644
> --- a/configs/orangepi_zero2_defconfig
> +++ b/configs/orangepi_zero2_defconfig
> @@ -2,13 +2,10 @@ CONFIG_ARM=y
> CONFIG_ARCH_SUNXI=y
> CONFIG_DEFAULT_DEVICE_TREE="sun50i-h616-orangepi-zero2"
> CONFIG_SPL=y
> -CONFIG_DRAM_SUN50I_H616_WRITE_LEVELING=y
> -CONFIG_DRAM_SUN50I_H616_READ_CALIBRATION=y
> -CONFIG_DRAM_SUN50I_H616_READ_TRAINING=y
> -CONFIG_DRAM_SUN50I_H616_WRITE_TRAINING=y
> CONFIG_DRAM_SUN50I_H616_DX_ODT=0x8080808
> CONFIG_DRAM_SUN50I_H616_DX_DRI=0xe0e0e0e
> CONFIG_DRAM_SUN50I_H616_CA_DRI=0xe0e
> +CONFIG_DRAM_SUN50I_H616_TPR10=0xf83438
> CONFIG_MACH_SUN50I_H616=y
> CONFIG_MMC0_CD_PIN="PF6"
> CONFIG_R_I2C_ENABLE=y