[U-Boot] [PATCH] board: ls1028a: set up integrated PCI stream IDs and cache attributes

[Alex Marginean]

On 8/9/2019 4:27 PM, Bin Meng wrote:
> Hi Alex,
> 
> On Fri, Aug 9, 2019 at 9:15 PM Alex Marginean <alexm.osslist at gmail.com> wrote:
>>
>> Hi Bin,
>>
>> On 8/9/2019 12:58 PM, Bin Meng wrote:
>>> Hi Alex,
>>>
>>> On Fri, Aug 9, 2019 at 3:59 PM Alex Marginean
>>> <alexandru.marginean at nxp.com> wrote:
>>>>
>>>> Configure stream IDs for integrated PCI devices.  There are hardware
>>>> defaults but unfortunately they are outside the acceptable range for
>>>> SMMU, so we need to tune them down.  Use values based on Linux device tree
>>>> iommu-map or, if missing, start from HW base value shifted down by 4.
>>>>
>>>> Signed-off-by: Alex Marginean <alexm.osslist at gmail.com>
>>>> ---
>>>>    board/freescale/ls1028a/ls1028a.c | 64 +++++++++++++++++++++++++++++++
>>>>    1 file changed, 64 insertions(+)
>>>>
>>>> diff --git a/board/freescale/ls1028a/ls1028a.c b/board/freescale/ls1028a/ls1028a.c
>>>> index 49a9292c31..05eac6f9c4 100644
>>>> --- a/board/freescale/ls1028a/ls1028a.c
>>>> +++ b/board/freescale/ls1028a/ls1028a.c
>>>> @@ -118,6 +118,67 @@ void detail_board_ddr_info(void)
>>>>    }
>>>>
>>>>    #ifdef CONFIG_OF_BOARD_SETUP
>>>> +
>>>> +/*
>>>> + * Hardware default stream IDs are 0x4000 + PCI function #, but that's outside
>>>> + * the acceptable range for SMMU.  Use Linux DT values instead or at least
>>>> + * smaller defaults.
>>>> + */
>>>> +#define ECAM_NUM_PFS                   7
>>>> +#define ECAM_IERB_BASE                 0x1F0800000
>>>> +#define ECAM_PFAMQ(pf, vf)             ((ECAM_IERB_BASE + 0x800 + (pf) * \
>>>> +                                         0x1000 + (vf) * 4))
>>>> +/* cache related transaction attributes for PCIe functions */
>>>> +#define ECAM_IERB_MSICAR               (ECAM_IERB_BASE + 0xa400)
>>>> +#define ECAM_IERB_MSICAR_VALUE         0x30
>>>> +
>>>> +/* number of VFs per PF, VFs have their own AMQ settings */
>>>> +static const u8 enetc_vfs[ECAM_NUM_PFS] = { 2, 2 };
>>>> +
>>>> +void setup_ecam_amq(void *blob)
>>>
>>> nits: this should be static
>>
>> I'll send a v2.
>>
>>>
>>>> +{
>>>> +       int streamid, sid_base, off;
>>>> +       int pf, vf, vfnn = 1;
>>>> +       u32 iommu_map[4];
>>>> +       int err;
>>>> +
>>>> +       /*
>>>> +        * Look up the stream ID settings in the DT, if found apply the values
>>>> +        * to HW, otherwise use HW values shifted down by 4.
>>>> +        */
>>>> +       off = fdt_node_offset_by_compatible(blob, 0, "pci-host-ecam-generic");
>>>> +       if (off < 0) {
>>>> +               debug("ECAM node not found\n");
>>>> +               return;
>>>> +       }
>>>> +
>>>> +       err = fdtdec_get_int_array(blob, off, "iommu-map", iommu_map, 4);
>>>> +       if (err) {
>>>> +               sid_base = in_le32(ECAM_PFAMQ(0, 0)) >> 4;
>>>> +               debug("\"iommu-map\" not found, using default SID base %04x\n",
>>>> +                     sid_base);
>>>> +       } else {
>>>> +               sid_base = iommu_map[2];
>>>> +       }
>>>> +       /* set up AMQs for all integrated PCI functions */
>>>> +       for (pf = 0; pf < ECAM_NUM_PFS; pf++) {
>>>> +               streamid = sid_base + pf;
>>>> +               out_le32(ECAM_PFAMQ(pf, 0), streamid);
>>>> +
>>>> +               /* set up AMQs for VFs, if any */
>>>> +               for (vf = 0; vf < enetc_vfs[pf]; vf++, vfnn++) {
>>>> +                       streamid = sid_base + ECAM_NUM_PFS + vfnn;
>>>> +                       out_le32(ECAM_PFAMQ(pf, vf + 1), streamid);
>>>> +               }
>>>> +       }
>>>> +}
>>>> +
>>>> +void setup_ecam_cacheattr(void)
>>>
>>> ditto
>>>
>>>> +{
>>>> +       /* set MSI cache attributes */
>>>> +       out_le32(ECAM_IERB_MSICAR, ECAM_IERB_MSICAR_VALUE);
>>>> +}
>>>> +
>>>>    int ft_board_setup(void *blob, bd_t *bd)
>>>>    {
>>>>           u64 base[CONFIG_NR_DRAM_BANKS];
>>>> @@ -143,6 +204,9 @@ int ft_board_setup(void *blob, bd_t *bd)
>>>>
>>>>           fdt_fixup_memory_banks(blob, base, size, 2);
>>>>
>>>> +       setup_ecam_amq(blob);
>>>> +       setup_ecam_cacheattr();
>>>> +
>>>>           return 0;
>>>>    }
>>>>    #endif
>>>
>>> Not only programming the registers, but also I think we will need fix
>>> up the <msi-map> property used by the "pci-host-ecam-generic", just
>>> like what it was done in fdt_pcie_set_msi_map_entry() in
>>> pcie_layerscape_fixup.c
>>
>> The DT loaded with the kernel has the two maps (iommu and msi) in sync,
>> they are both present and they start from same base ID.  For integrated
> 
> OK, I checked arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi, and
> found pcie at 1f0000000 (ecam) node has <msi-map> and <iommu-map>
> properties, both of which have the same ID 0x17.
> 
>> PCI we use that as a reference in U-Boot rather than assign the value
>> from U-Boot.  Layerscape PCI node as far as I remember doesn't have the
>> msi/iommu-map properties, they are added by U-Boot using IDs it
>> allocates, but we don't do that here.  The intent is to try to keep
> 
> In the same kernel fsl-ls1028a.dtsi, I did not find any layescape PCI
> nodes. I suspect they are not upstreamed yet?

I don't know for a fact, I assume they have been submitted but not
merged yet.

> So if the we are doing layersacpe PCI <msi-map> fix in U-Boot, but for
> ECAM we are doing the other way around, I don't think it is
> consistent. I believe we should either have DT provide <msi-map> and
> <iommu-map> for layersacpe PCI nodes and use that as a reference in
> U-Boot, or we fix up all in U-Boot.

It's not consistent, no argument there.  For what is worth I would like
to stick to the concept of integrated HW using by default a valid set of
IDs and not do any fix-up at all.  I suppose the next best thing along
that line is to right shift the ID range for ECAM in U-Boot and
integrate this as a erratum work-around, rather than a fix-up.  Linux DT
would be written using the right shifted values and U-Boot wouldn't need
to touch it.  How does that sound?  Nipun, does it look OK to you?

>> fixups to a minimum, in this case the code is needed only because the HW
>> defaults are not OK.
> 
> Speaking of keeping fixups to a minimum, should we move
> setup_ecam_amq() and setup_ecam_cacheattr() to somewhere else? They
> are inside ft_board_setup() which is supposed to do board-level DT fix
> up but here we are actually not doing any DT fixup.

If the code needs access to Linux DT, it has to be in one of the
ft_... functions.  It looks like ft_board_setup and ft_system_setup and
others are all intended just for editing the DT.
Anyway, approaching this as a work-around means access to Linux DT is no
longer needed and the code can go in board_init.

Thank you!
Alex

> 
>> For what is worth I tested ping in Linux with this U-Boot change and it
>> works.
>>
>> I suppose one potential issue is to make sure U-Boot won't
>> allocate in the same range as the values used in DT for ECAM.  The
>> values we use now in Linux DT are OK in the sense that they are over the
>> range used by U-Boot (FSL_PEX_STREAM_ID_START-FSL_PEX_STREAM_ID_END).
>> There is no explicit check for overlap between layerscape PCI stream IDs
>> and ECAM stream IDs, I am guessing such a configuration issue would be
>> noticeable in Linux though.
>>
> 
> Regards,
> Bin
>