Hi Stefano,
> On 10 Sep 2021, at 12:41 am, Stefano Stabellini <sstabell...@kernel.org>
> wrote:
>
> On Thu, 19 Aug 2021, Rahul Singh wrote:
>> Implement generic pci access functions to read/write the configuration
>> space.
>>
>> Signed-off-by: Rahul Singh <rahul.si...@arm.com>
>> ---
>> xen/arch/arm/pci/pci-access.c | 31 +++++++++++++++++++++++++++++-
>> xen/arch/arm/pci/pci-host-common.c | 19 ++++++++++++++++++
>> xen/include/asm-arm/pci.h | 2 ++
>> 3 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
>> index f39f6a3a38..b94de3c3ac 100644
>> --- a/xen/arch/arm/pci/pci-access.c
>> +++ b/xen/arch/arm/pci/pci-access.c
>> @@ -72,12 +72,41 @@ int pci_generic_config_write(struct pci_host_bridge
>> *bridge, uint32_t sbdf,
>> static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
>> unsigned int len)
>> {
>> - return ~0U;
>> + uint32_t val = GENMASK(0, len * 8);
>
> This seems to be another default error value that it would be better to
> define with its own macro
This default error is used once do you want to me define as macro.
>
>
>> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg,
>> sbdf.bus);
>> +
>> + if ( unlikely(!bridge) )
>> + {
>> + printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
>> + sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
>
> You are not actually printing sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn ?
Yes I am printing with “PRI_pci".
>
>
>> + return val;
>> + }
>> +
>> + if ( unlikely(!bridge->ops->read) )
>> + return val;
>> +
>> + bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);
>
> Would it make sense to make the interface take a pci_sbdf_t directly
> instead of casting to uint32_t and back?
pci_sbdf_t is defined in "xen/pci.h” and "xen/pci.h” includes "asm-arm/pci.h”.
If I modify the function argument in "asm-arm/pci.h” to use pci_sbdf_t I will
get compilation error.
>
>
>> + return val;
>> }
>>
>> static void pci_config_write(pci_sbdf_t sbdf, unsigned int reg,
>> unsigned int len, uint32_t val)
>> {
>> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg,
>> sbdf.bus);
>> +
>> + if ( unlikely(!bridge) )
>> + {
>> + printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
>> + sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
>
> same here
Yes I am printing with “PRI_pci".
>
Regards
Rahul
