Hi Julien,
> On 23 Sep 2021, at 10:02 am, Julien Grall <jul...@xen.org> wrote:
>
> Hi Stefano,
>
> On 23/09/2021 07:23, Stefano Stabellini wrote:
>> Subject should have xen/arm
>> On Wed, 22 Sep 2021, Rahul Singh wrote:
>>> Implement generic pci access functions to read/write the configuration
>>> space.
>>>
>>> Signed-off-by: Rahul Singh <rahul.si...@arm.com>
>>> ---
>>> Change in v2: Fixed comments
>>> ---
>>> xen/arch/arm/pci/pci-access.c | 58 ++++++++++++++++++++++++++++++
>>> xen/arch/arm/pci/pci-host-common.c | 19 ++++++++++
>>> xen/include/asm-arm/pci.h | 2 ++
>>> 3 files changed, 79 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
>>> index 04fe9fbf92..45500cec2a 100644
>>> --- a/xen/arch/arm/pci/pci-access.c
>>> +++ b/xen/arch/arm/pci/pci-access.c
>>> @@ -16,6 +16,7 @@
>>> #include <asm/io.h>
>>> #define INVALID_VALUE (~0U)
>>> +#define PCI_ERR_VALUE(len) GENMASK(0, len * 8)
>>> int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t
>>> sbdf,
>>> uint32_t reg, uint32_t len, uint32_t *value)
>>> @@ -72,6 +73,63 @@ int pci_generic_config_write(struct pci_host_bridge
>>> *bridge, uint32_t sbdf,
>>> return 0;
>>> }
>>> +static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
>>> + unsigned int len)
>>> +{
>>> + uint32_t val = PCI_ERR_VALUE(len);
>>> +
>> No blank line
>>> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg,
>>> sbdf.bus);
>>> +
>>> + if ( unlikely(!bridge) )
>>> + return val;
>>> +
>>> + if ( unlikely(!bridge->ops->read) )
>>> + return val;
>>> +
>>> + bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);
>> The more I look at these casts the less I like them :-D
>
> I really dislike them. This is kind of defeating the purpose of trying to be
> more typesafe.
>
>> One idea is to move the definition of pci_sbdf_t somewhere else
>> entirely, for instance xen/include/xen/types.h, then we can use
>> pci_sbdf_t everywhere
>
> AFAIU, the problem is the prototype helpers are defined in asm/pci.h which is
> included by xen/pci.h before defining sbdf_t. Is it correct?
>
> If so there are two options:
> 1) define sbdf_t and then include asm/pci.h.
> 2) Name the union and then pre-declare it.
>
> Option 1 is probably nicer is we have more types in the future that are used
> by arch specific but defined in the common headers. We have a few places that
> uses this approach.
>
Thanks for the pointer I will fix this in next version.
Regards,
Rahul
> Cheers,
>
> --
> Julien Grall
