Re: [PATCH v1 05/14] xen/arm: PCI host bridge discovery within XEN on ARM

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 13.09.21 23:23, Stefano Stabellini wrote:
> On Mon, 13 Sep 2021, Oleksandr Andrushchenko wrote:
>> Hi, Rahul!
>>
>> On 19.08.21 15:02, Rahul Singh wrote:
>>> XEN during boot will read the PCI device tree node “reg” property
>>> and will map the PCI config space to the XEN memory.
>> [snip]
>>> +static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
>>> +                                              int ecam_reg_idx)
>>> +{
>>> +    int err;
>>> +    struct pci_config_window *cfg;
>>> +    paddr_t addr, size;
>>> +
>>> +    cfg = xzalloc(struct pci_config_window);
>>> +    if ( !cfg )
>>> +        return NULL;
>>> +
>>> +    err = dt_pci_parse_bus_range(dev, cfg);
>>> +    if ( !err ) {
>>> +        cfg->busn_start = 0;
>>> +        cfg->busn_end = 0xff;
>>> +        printk(XENLOG_ERR "%s:No bus range found for pci controller\n",
>>> +               dt_node_full_name(dev));
>>> +    } else {
>>> +        if ( cfg->busn_end > cfg->busn_start + 0xff )
>>> +            cfg->busn_end = cfg->busn_start + 0xff;
>>> +    }
>>> +
>>> +    /* Parse our PCI ecam register address*/
>>> +    err = dt_device_get_address(dev, ecam_reg_idx, &addr, &size);
>> I am a bit worried here that we don't get the reg index from the device tree,
>>
>> but for generic ECAM we use reg[0] and for Xilinx we use reg[2].
>>
>> For example, for Xilinx we have
>>
>> reg = <0x00 0xfd0e0000 0x00 0x1000 0x00 0xfd480000 0x00 0x1000 0x80 0x00 
>> 0x00 0x1000000>;
>> reg-names = "breg\0pcireg\0cfg";
>>
>> so, we can parse the reg-names and understand that the configuration space 
>> is the last in the reg property.
>>
>> The same I think can be done for other device trees probably.
> Well spotted!
>
>
>> Rahul, do you know if reg-names "cfg" is vendor specific of used widely?
> Unfortunately it seems to be vendor specific :-(
> Which means that "dt_device_get_address" or similar should be moved to a
> vendor specific function.

I just stepped on this while working on the patch for not mapping

MMIOs and config to Dom0 and need to find a way to tell them from

device registers. So, yes, it seems we will need something more generic

than just an index. We can still have the callback for this optional,

so the bridges with the same reg names can use the default callback

and don't need to implement the same