[PATCH v5 10/29] pci: Adjust dm_pci_read_bar32() to return errors correctly

2020-04-08 Thread Simon Glass
At present if reading a BAR returns 0x (e.g. the device is not
present) then the value is masked and a different value is returned.
This makes it harder to detect the problem when debugging.

Update the function to avoid masking in this case.

Signed-off-by: Simon Glass 
Reviewed-by: Bin Meng 
Reviewed-by: Wolfgang Wallner 
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/pci-uclass.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index 213381da6bd..7f46e901fb2 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -1213,7 +1213,14 @@ u32 dm_pci_read_bar32(const struct udevice *dev, int 
barnum)
 
bar = PCI_BASE_ADDRESS_0 + barnum * 4;
dm_pci_read_config32(dev, bar, &addr);
-   if (addr & PCI_BASE_ADDRESS_SPACE_IO)
+
+   /*
+* If we get an invalid address, return this so that comparisons with
+* FDT_ADDR_T_NONE work correctly
+*/
+   if (addr == 0x)
+   return addr;
+   else if (addr & PCI_BASE_ADDRESS_SPACE_IO)
return addr & PCI_BASE_ADDRESS_IO_MASK;
else
return addr & PCI_BASE_ADDRESS_MEM_MASK;
-- 
2.26.0.292.g33ef6b2f38-goog



Re: [PATCH v5 10/29] pci: Adjust dm_pci_read_bar32() to return errors correctly

2020-04-09 Thread Andy Shevchenko
On Thu, Apr 9, 2020 at 2:00 AM Simon Glass  wrote:
>
> At present if reading a BAR returns 0x (e.g. the device is not
> present) then the value is masked and a different value is returned.
> This makes it harder to detect the problem when debugging.

If you insisting on the code, you may need to reword the commit
message, at least by removing ambiguous "device not present", because
this is not how PCI spec defines situations when device is not
present.

-- 
With Best Regards,
Andy Shevchenko


Re: [PATCH v5 10/29] pci: Adjust dm_pci_read_bar32() to return errors correctly

2020-04-09 Thread Mark Kettenis
> From: Andy Shevchenko 
> Date: Thu, 9 Apr 2020 12:06:11 +0300
> 
> On Thu, Apr 9, 2020 at 2:00 AM Simon Glass  wrote:
> >
> > At present if reading a BAR returns 0x (e.g. the device is not
> > present) then the value is masked and a different value is returned.
> > This makes it harder to detect the problem when debugging.
> 
> If you insisting on the code, you may need to reword the commit
> message, at least by removing ambiguous "device not present", because
> this is not how PCI spec defines situations when device is not
> present.

In addition to that:

The 0x value is an artifact of how x86 systems handle aborted
PCI transactions.  Other architectures will probably generate a fault
of some sorts.  It looks like at least some of the pci drivers in
u-boot emulate the x86 behaviour but I'm not sure all of them do.


Re: [PATCH v5 10/29] pci: Adjust dm_pci_read_bar32() to return errors correctly

2020-04-09 Thread Simon Glass
Hi,

On Thu, 9 Apr 2020 at 03:39, Mark Kettenis  wrote:
>
> > From: Andy Shevchenko 
> > Date: Thu, 9 Apr 2020 12:06:11 +0300
> >
> > On Thu, Apr 9, 2020 at 2:00 AM Simon Glass  wrote:
> > >
> > > At present if reading a BAR returns 0x (e.g. the device is not
> > > present) then the value is masked and a different value is returned.
> > > This makes it harder to detect the problem when debugging.
> >
> > If you insisting on the code, you may need to reword the commit
> > message, at least by removing ambiguous "device not present", because
> > this is not how PCI spec defines situations when device is not
> > present.
>
> In addition to that:
>
> The 0x value is an artifact of how x86 systems handle aborted
> PCI transactions.  Other architectures will probably generate a fault
> of some sorts.  It looks like at least some of the pci drivers in
> u-boot emulate the x86 behaviour but I'm not sure all of them do.

This is not intended to be used to detect missing devices. It's just
that we should not be masking an obviously invalid value, to make it
look more valid. It is very confusing.

I'll update the comment message.

Regards,
Simon