[U-Boot] [PATCH 2/2] armv8: ls1088a: add icid setup for platform devices

From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 2/2] armv8: ls1088a: add icid setup for platform devices
Date: Thu, 21 Mar 2019 12:42:42 +0000	[thread overview]
Message-ID: <ac7b1570-a56b-71e4-c1fe-67917ad90ff0@nxp.com> (raw)
In-Reply-To: <VI1PR0402MB3485776739890FB7EB2CCE2898420@VI1PR0402MB3485.eurprd04.prod.outlook.com>

Hi Horia,

On 21.03.2019 12:36, Horia Geanta wrote:
> On 3/20/2019 4:31 PM, laurentiu.tudor at nxp.com wrote:
>> +struct icid_id_table icid_tbl[] = {
>> +	SET_SDHC_ICID(FSL_SDMMC_STREAM_ID),
>> +	SET_USB_ICID(1, "snps,dwc3", FSL_USB1_STREAM_ID),
>> +	SET_USB_ICID(2, "snps,dwc3", FSL_USB2_STREAM_ID),
>> +	SET_SATA_ICID(1, "fsl,ls1088a-ahci", FSL_SATA1_STREAM_ID),
>> +	SET_SEC_JR_ICID_ENTRY(0, FSL_SEC_STREAM_ID),
>> +	SET_SEC_JR_ICID_ENTRY(1, FSL_SEC_STREAM_ID),
>> +	SET_SEC_JR_ICID_ENTRY(2, FSL_SEC_STREAM_ID),
>> +	SET_SEC_JR_ICID_ENTRY(3, FSL_SEC_STREAM_ID),
>> +	SET_SEC_RTIC_ICID_ENTRY(0, FSL_SEC_STREAM_ID),
>> +	SET_SEC_RTIC_ICID_ENTRY(1, FSL_SEC_STREAM_ID),
>> +	SET_SEC_RTIC_ICID_ENTRY(2, FSL_SEC_STREAM_ID),
>> +	SET_SEC_RTIC_ICID_ENTRY(3, FSL_SEC_STREAM_ID),
>> +	SET_SEC_DECO_ICID_ENTRY(0, FSL_SEC_STREAM_ID),
>> +	SET_SEC_DECO_ICID_ENTRY(1, FSL_SEC_STREAM_ID),
>> +	SET_SEC_DECO_ICID_ENTRY(2, FSL_SEC_STREAM_ID),
>> +	SET_SEC_DECO_ICID_ENTRY(3, FSL_SEC_STREAM_ID),
>> +};
> A single ICID is allocated to all SEC sub-blocks able to initiate transactions.
> I think at least the job rings should have different ICIDs, while the rest could
> share another ICID.

I agree here, problem is that on these chips ICIDs are scarce resource 
because DPAA2 gets a big chunk of them. I'd suggest to leave them like 
that and, if a user needs distinct, per JR ICIDs (s)he can easily tune them.

>>   #define SET_SEC_QI_ICID(streamid) \
>> -	SET_ICID_ENTRY("fsl,sec-v4.0", streamid, \
>> +	SET_ICID_ENTRY("fsl,sec-v4.0", SEC_ICID_REG_VAL(streamid), \
> Is this a fix for LS104x? Then it should be a separate patch.

Not really. I added an intermediate macro, SEC_ICID_REG_VAL(streamid) 
that forms the correct register value starting from ICID, depending on 
the chip version (the register layouts are different between the two).

>> +#else /* CONFIG_FSL_LSCH2 */
> [...]
>> +#define SEC_ICID_REG_VAL(streamid) ((streamid) << 24)
> ICID is in lower 6:0 bits, not in 31:24.

That was also my initial impression but it didn't work (smmu global 
faults with icid 0). Probably there's an ambiguity related to endianness 
in the documentation.

>> +#endif /* !CONFIG_FSL_LSCH2 */
> Nitpick: comment for #endif should match #if condition, which is CONFIG_FSL_LSCH2.
> 

Ok.

Thanks for the review!

---
Best Regards, Laurentiu