RE: [PATCH] ARC: [plat-hsdk]: unify memory apertures configuration

From: Alexey Brodkin
Date: Tue May 28 2019 - 08:05:23 EST


Hi Eugeniy,

> -----Original Message-----
> From: Eugeniy Paltsev <Eugeniy.Paltsev@xxxxxxxxxxxx>
> Sent: Tuesday, May 28, 2019 11:55 AM
> To: linux-snps-arc@xxxxxxxxxxxxxxxxxxx; Vineet Gupta <vgupta@xxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; Alexey Brodkin <abrodkin@xxxxxxxxxxxx>; Eugeniy Paltsev
> <Eugeniy.Paltsev@xxxxxxxxxxxx>
> Subject: [PATCH] ARC: [plat-hsdk]: unify memory apertures configuration
>
> HSDK SOC has memory bridge which allows to configure memory map

SoC (which stands for "System on Chip").

> for different AXI masters in runtime.
> As of today we adjust memory apertures configuration in U-boot
> so we have different configuration in case of loading kernel
> via U-boot and JTAG.
>
> It isn't really critical in case of existing platform configuration
> as configuration differs for <currently> unused address space
> regions or unused AXI masters. However we may face with this
> issue when we'll bringup new peripherals or touch their address
> space.

Maybe add some background what do we change and why?

> Fix that by copy memory apertures configuration from U-boot to
> HSDK platform code.
>
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@xxxxxxxxxxxx>

A couple of nitpicks, still...

Acked-by: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>

> ---
> This should be done a long time ago and this could save me from a lot

...should have been done... ...could have saved...

> of debugging while bringing up GPU on HSDKv2...
>
> arch/arc/plat-hsdk/platform.c | 144 ++++++++++++++++++++++++++++++++--
> 1 file changed, 136 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arc/plat-hsdk/platform.c b/arch/arc/plat-hsdk/platform.c
> index 2588b842407c..e336e34925b7 100644
> --- a/arch/arc/plat-hsdk/platform.c
> +++ b/arch/arc/plat-hsdk/platform.c
> @@ -35,8 +35,6 @@ static void __init hsdk_init_per_cpu(unsigned int cpu)
>
> #define ARC_PERIPHERAL_BASE 0xf0000000
> #define CREG_BASE (ARC_PERIPHERAL_BASE + 0x1000)
> -#define CREG_PAE (CREG_BASE + 0x180)
> -#define CREG_PAE_UPDATE (CREG_BASE + 0x194)
>
> #define SDIO_BASE (ARC_PERIPHERAL_BASE + 0xA000)
> #define SDIO_UHS_REG_EXT (SDIO_BASE + 0x108)
> @@ -102,20 +100,150 @@ static void __init hsdk_enable_gpio_intc_wire(void)
> iowrite32(GPIO_INT_CONNECTED_MASK, (void __iomem *) GPIO_INTEN);
> }
>
> -static void __init hsdk_init_early(void)
> +enum hsdk_axi_masters {
> + M_HS_CORE = 0,
> + M_HS_RTT,
> + M_AXI_TUN,
> + M_HDMI_VIDEO,
> + M_HDMI_AUDIO,
> + M_USB_HOST,
> + M_ETHERNET,
> + M_SDIO,
> + M_GPU,
> + M_DMAC_0,
> + M_DMAC_1,
> + M_DVFS
> +};
> +
> +#define UPDATE_VAL 1

I'd add some explanation of what that is here like:
- Default (or modified) table from the manual xxx.
- AXI_M_m_SLVx & AXI_M_m_OFFSETx are MMIO regs which are used for ...

> +/*
> + * m master AXI_M_m_SLV0 AXI_M_m_SLV1 AXI_M_m_OFFSET0 AXI_M_m_OFFSET1
> + * 0 HS (CBU) 0x11111111 0x63111111 0xFEDCBA98 0x0E543210
> + * 1 HS (RTT) 0x77777777 0x77777777 0xFEDCBA98 0x76543210
> + * 2 AXI Tunnel 0x88888888 0x88888888 0xFEDCBA98 0x76543210
> + * 3 HDMI-VIDEO 0x77777777 0x77777777 0xFEDCBA98 0x76543210
> + * 4 HDMI-ADUIO 0x77777777 0x77777777 0xFEDCBA98 0x76543210
> + * 5 USB-HOST 0x77777777 0x77999999 0xFEDCBA98 0x76DCBA98
> + * 6 ETHERNET 0x77777777 0x77999999 0xFEDCBA98 0x76DCBA98
> + * 7 SDIO 0x77777777 0x77999999 0xFEDCBA98 0x76DCBA98
> + * 8 GPU 0x77777777 0x77777777 0xFEDCBA98 0x76543210
> + * 9 DMAC (port #1) 0x77777777 0x77777777 0xFEDCBA98 0x76543210
> + * 10 DMAC (port #2) 0x77777777 0x77777777 0xFEDCBA98 0x76543210
> + * 11 DVFS 0x00000000 0x60000000 0x00000000 0x00000000
> + *
> + * Please read ARC HS Development IC Specification, section 17.2 for more
> + * information about apertures configuration.
> + * NOTE: we intentionally modify default settings in U-boot. Default settings
> + * are specified in "Table 111 CREG Address Decoder register reset values".
> + */
> +
> +#define CREG_AXI_M_SLV0(m) ((void __iomem *)(CREG_BASE + 0x020 * (m)))
> +#define CREG_AXI_M_SLV1(m) ((void __iomem *)(CREG_BASE + 0x020 * (m) + 0x004))
> +#define CREG_AXI_M_OFT0(m) ((void __iomem *)(CREG_BASE + 0x020 * (m) + 0x008))
> +#define CREG_AXI_M_OFT1(m) ((void __iomem *)(CREG_BASE + 0x020 * (m) + 0x00C))
> +#define CREG_AXI_M_UPDT(m) ((void __iomem *)(CREG_BASE + 0x020 * (m) + 0x014))

Maybe skip 1 zero? I.e. use 0x04/0x08/0x0c/0x14?

> +
> +#define CREG_AXI_M_HS_CORE_BOOT ((void __iomem *)(CREG_BASE + 0x010))
> +
> +#define CREG_PAE ((void __iomem *)(CREG_BASE + 0x180))
> +#define CREG_PAE_UPDT ((void __iomem *)(CREG_BASE + 0x194))
> +
> +static void __init hsdk_init_memory_bridge(void)
> {
> + u32 reg;
> +
> + /*
> + * M_HS_CORE has one unic register - BOOT.

unique

> + * We need to clean boot mirror (BOOT[1:0]) bits in them.
> + */

Why do we need to do that?

-Alexey