Re: [U-Boot] [upstream-release] [PATCH v2 5/7] armv8: ls1046a: initial icid setup support
Hi Bharat,
Thanks for the review! Comments inline.
On 03.07.2018 17:09, Bharat Bhushan wrote:
>
>
>> -Original Message-
>> From: upstream-release-boun...@linux.freescale.net [mailto:upstream-
>> release-boun...@linux.freescale.net] On Behalf Of Laurentiu Tudor
>> Sent: Tuesday, July 3, 2018 5:42 PM
>> To: York Sun ; Prabhakar Kushwaha
>> ; u-boot@lists.denx.de
>> Cc: Laurentiu Tudor
>> Subject: [upstream-release] [PATCH v2 5/7] armv8: ls1046a: initial icid setup
>> support
>>
>> Add infrastructure for ICID setup and device tree
>> fixup on ARM platforms. This include basic ICID setup
>> for several devices.
>>
>> Signed-off-by: Laurentiu Tudor
>> ---
>> arch/arm/cpu/armv8/fsl-layerscape/Makefile| 1 +
>> arch/arm/cpu/armv8/fsl-layerscape/icid.c | 111 ++
>> .../arm/cpu/armv8/fsl-layerscape/ls1046_ids.c | 29 +
>> arch/arm/cpu/armv8/fsl-layerscape/soc.c | 3 +
>> .../asm/arch-fsl-layerscape/fsl_icid.h| 80 +
>> board/freescale/ls1046aqds/ls1046aqds.c | 2 +
>> board/freescale/ls1046ardb/ls1046ardb.c | 3 +
>> 7 files changed, 229 insertions(+)
>> create mode 100644 arch/arm/cpu/armv8/fsl-layerscape/icid.c
>> create mode 100644 arch/arm/cpu/armv8/fsl-layerscape/ls1046_ids.c
>> create mode 100644 arch/arm/include/asm/arch-fsl-layerscape/fsl_icid.h
>>
>> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/Makefile
>> b/arch/arm/cpu/armv8/fsl-layerscape/Makefile
>> index 1e9e4680fe..5d6f68aad6 100644
>> --- a/arch/arm/cpu/armv8/fsl-layerscape/Makefile
>> +++ b/arch/arm/cpu/armv8/fsl-layerscape/Makefile
>> @@ -37,6 +37,7 @@ endif
>>
>> ifneq ($(CONFIG_ARCH_LS1046A),)
>> obj-$(CONFIG_SYS_HAS_SERDES) += ls1046a_serdes.o
>> +obj-y += icid.o ls1046_ids.o
>> endif
>>
>> ifneq ($(CONFIG_ARCH_LS1088A),)
>> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/icid.c
>> b/arch/arm/cpu/armv8/fsl-layerscape/icid.c
>> new file mode 100644
>> index 00..8694bd6fa1
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv8/fsl-layerscape/icid.c
>> @@ -0,0 +1,111 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright 2018 NXP
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +#include
>> +#include
>> +
>> +static void set_icid(struct icid_id_table *tbl, int size)
>> +{
>> +int i;
>> +
>> +for (i = 0; i < size; i++)
>> +out_be32((u32 *)(tbl[i].reg_addr), tbl[i].reg);
>> +}
>> +
>> +void set_icids(void)
>> +{
>> +/* setup general icid offsets */
>> +set_icid(icid_tbl, icid_tbl_sz);
>
> Icid_tbl[] is currently defined for ls1046 but this code is generic and later
> can be used for ls1043 later.
> We can let caller provide table pointer and size.
Right, but "icid_tbl" is defined in the SoC specific ls1046_ids.c file.
I think that if we add the corresponding ls1043_ids.c this code can be
reused as is.
>> +}
>> +
>> +int fdt_set_iommu_prop(void *blob, int off, int smmu_ph, u32 *ids, int
>> num_ids)
>> +{
>> +int i, ret;
>> +u32 prop[8];
>> +
>> +for (i = 0; i < num_ids; i++) {
>> +prop[i * 2] = cpu_to_fdt32(smmu_ph);
>> +prop[i * 2 + 1] = cpu_to_fdt32(ids[i]);
>> +}
>> +ret = fdt_setprop(blob, off, "iommus",
>> + prop, sizeof(u32) * num_ids * 2);
>> +if (ret > 0) {
>> +printf("WARNING unable to set iommus: %s\n",
>> fdt_strerror(off));
>> +return off;
>> +}
>> +ret = fdt_setprop_empty(blob, off, "dma-coherent");
>> +if (ret > 0) {
>> +printf("WARNING unable to set dma-coherent: %s\n",
>> + fdt_strerror(off));
>> +return off;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +int fdt_fixup_icid_tbl(void *blob, int smmu_ph,
>> + struct icid_id_table *tbl, int size)
>
> What we set in device tree is stream-id, while stream-id is = [PL: BMT :
> ICID];
Also TBU_ID. As far as i know, PL, BMT bits are not implemented on this
platform.
> I think we should use stream-id to the point when we are patching
> device-tree and value set in h/w.
Not sure i understand this statement? Are you commenting that i should
have used a different naming scheme, that is stream_id instead of icid?
> We can say PL = BMT = 0 for now and leave space for setting this in future
> for any specific device for any platform?
I think so. As i mentioned, PL, BMT bits are not implemented on these chips.
>> +{
>> +int i, err, off;
>> +
>> +for (i = 0; i < size; i++) {
>> +if (!tbl[i].compat)
>> +continue;
>> +
>> +off = fdt_node_offset_by_compat_reg(blob,
>> +tbl[i].compat,
>> +tbl[i].compat_addr);
>> +if (off > 0) {
>> +err = fdt_set_iommu_prop(blob, off, smmu_ph,
>> + [i].id, 1);
>> +if
Re: [U-Boot] [upstream-release] [PATCH v2 5/7] armv8: ls1046a: initial icid setup support
> -Original Message-
> From: upstream-release-boun...@linux.freescale.net [mailto:upstream-
> release-boun...@linux.freescale.net] On Behalf Of Laurentiu Tudor
> Sent: Tuesday, July 3, 2018 5:42 PM
> To: York Sun ; Prabhakar Kushwaha
> ; u-boot@lists.denx.de
> Cc: Laurentiu Tudor
> Subject: [upstream-release] [PATCH v2 5/7] armv8: ls1046a: initial icid setup
> support
>
> Add infrastructure for ICID setup and device tree
> fixup on ARM platforms. This include basic ICID setup
> for several devices.
>
> Signed-off-by: Laurentiu Tudor
> ---
> arch/arm/cpu/armv8/fsl-layerscape/Makefile| 1 +
> arch/arm/cpu/armv8/fsl-layerscape/icid.c | 111 ++
> .../arm/cpu/armv8/fsl-layerscape/ls1046_ids.c | 29 +
> arch/arm/cpu/armv8/fsl-layerscape/soc.c | 3 +
> .../asm/arch-fsl-layerscape/fsl_icid.h| 80 +
> board/freescale/ls1046aqds/ls1046aqds.c | 2 +
> board/freescale/ls1046ardb/ls1046ardb.c | 3 +
> 7 files changed, 229 insertions(+)
> create mode 100644 arch/arm/cpu/armv8/fsl-layerscape/icid.c
> create mode 100644 arch/arm/cpu/armv8/fsl-layerscape/ls1046_ids.c
> create mode 100644 arch/arm/include/asm/arch-fsl-layerscape/fsl_icid.h
>
> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/Makefile
> b/arch/arm/cpu/armv8/fsl-layerscape/Makefile
> index 1e9e4680fe..5d6f68aad6 100644
> --- a/arch/arm/cpu/armv8/fsl-layerscape/Makefile
> +++ b/arch/arm/cpu/armv8/fsl-layerscape/Makefile
> @@ -37,6 +37,7 @@ endif
>
> ifneq ($(CONFIG_ARCH_LS1046A),)
> obj-$(CONFIG_SYS_HAS_SERDES) += ls1046a_serdes.o
> +obj-y += icid.o ls1046_ids.o
> endif
>
> ifneq ($(CONFIG_ARCH_LS1088A),)
> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/icid.c
> b/arch/arm/cpu/armv8/fsl-layerscape/icid.c
> new file mode 100644
> index 00..8694bd6fa1
> --- /dev/null
> +++ b/arch/arm/cpu/armv8/fsl-layerscape/icid.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018 NXP
> + */
> +
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +static void set_icid(struct icid_id_table *tbl, int size)
> +{
> + int i;
> +
> + for (i = 0; i < size; i++)
> + out_be32((u32 *)(tbl[i].reg_addr), tbl[i].reg);
> +}
> +
> +void set_icids(void)
> +{
> + /* setup general icid offsets */
> + set_icid(icid_tbl, icid_tbl_sz);
Icid_tbl[] is currently defined for ls1046 but this code is generic and later
can be used for ls1043 later.
We can let caller provide table pointer and size.
> +}
> +
> +int fdt_set_iommu_prop(void *blob, int off, int smmu_ph, u32 *ids, int
> num_ids)
> +{
> + int i, ret;
> + u32 prop[8];
> +
> + for (i = 0; i < num_ids; i++) {
> + prop[i * 2] = cpu_to_fdt32(smmu_ph);
> + prop[i * 2 + 1] = cpu_to_fdt32(ids[i]);
> + }
> + ret = fdt_setprop(blob, off, "iommus",
> + prop, sizeof(u32) * num_ids * 2);
> + if (ret > 0) {
> + printf("WARNING unable to set iommus: %s\n",
> fdt_strerror(off));
> + return off;
> + }
> + ret = fdt_setprop_empty(blob, off, "dma-coherent");
> + if (ret > 0) {
> + printf("WARNING unable to set dma-coherent: %s\n",
> +fdt_strerror(off));
> + return off;
> + }
> +
> + return 0;
> +}
> +
> +int fdt_fixup_icid_tbl(void *blob, int smmu_ph,
> +struct icid_id_table *tbl, int size)
What we set in device tree is stream-id, while stream-id is = [PL: BMT : ICID];
I think we should use stream-id to the point when we are patching device-tree
and value set in h/w.
We can say PL = BMT = 0 for now and leave space for setting this in future for
any specific device for any platform?
> +{
> + int i, err, off;
> +
> + for (i = 0; i < size; i++) {
> + if (!tbl[i].compat)
> + continue;
> +
> + off = fdt_node_offset_by_compat_reg(blob,
> + tbl[i].compat,
> + tbl[i].compat_addr);
> + if (off > 0) {
> + err = fdt_set_iommu_prop(blob, off, smmu_ph,
> + [i].id, 1);
> + if (err)
> + return err;
> + } else {
> + printf("WARNING could not find node %s: %s.\n",
> +tbl[i].compat, fdt_strerror(off));
> + }
> + }
> +
> + return 0;
> +}
> +
> +int fdt_get_smmu_phandle(void *blob)
> +{
> + int noff, smmu_ph;
> +
> + noff = fdt_node_offset_by_compatible(blob, -1, "arm,mmu-500");
> + if (noff < 0) {
> + printf("WARNING failed to get smmu node: %s\n",
> +fdt_strerror(noff));
> + return noff;
> + }
> +
> + smmu_ph = fdt_get_phandle(blob, noff);
> + if (!smmu_ph) {
> +
