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

[Simon Glass]

HI Andy,

On Wed, 8 Apr 2020 at 10:58, Andy Shevchenko
<andriy.shevchenko at linux.intel.com> wrote:
>
> On Tue, Apr 07, 2020 at 08:57:20PM -0600, Simon Glass wrote:
> > Hi Andy,
> >
> > On Fri, 3 Apr 2020 at 05:22, Andy Shevchenko
> > <andriy.shevchenko at linux.intel.com> wrote:
> > >
> > > On Mon, Mar 30, 2020 at 05:12:46PM -0600, Simon Glass wrote:
> > > > At present if reading a BAR returns 0xffffffff (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.
> > >
> > > The above ('the device is not present') is actually not correct.
> > > BAR is not mandatory register and detection is described in PCI spec.
> >
> > What change are you suggesting here? I suggest 'not present' as an
> > example of why this might happen.
>
> I suggest to follow PCI spec.
> Thus, the code below is fragile and working by luck.

I don't know what you are suggesting. This allows an error to be
reported in the common case and helps people discover mistakes in the
driver flow. What is your suggestion?

Rgards,
SImon


>
> > > To get device presence one may have check Vendor ID / Device ID pair rather
> > > then BAR.
> > >
> > > > Update the function to avoid masking in this case.
> > > >
> > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
> > > > Reviewed-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com>
> > > > ---
> > > >
> > > > 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 ceb64517047..d2e10d6868a 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 == 0xffffffff)
> > > > +             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.rc2.310.g2932bb562d-goog
> > > >
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> > >
> >
> > Regards,
> > Simon
>
> --
> With Best Regards,
> Andy Shevchenko
>
>