RE: [PATCH V4 1/1] NTB: Add support for AMD PCI-Express Non-Transparent Bridge

[Yu, Xiangliang]

> > > There is some concept of "split bar" in this function, and I want to
> > be sure to
> > > understand it correctly.
> > >
> > > BAR0 - configuration?
> > > BAR1 - 32bit memory window? i.e. "split" bar?
> > > BAR2+3 - 64bit memory window?
> > > BAR4+5 - 64bit memory window?
> >
> > Yes
> 
> Ok.
> 
> > > Note that "split" in the intel driver refers to BAR4+5, which is
> > normally a 64bit
> > > memory window, split into independent 32bit windows BAR4 and BAR5
> by
> > > bios configuration.  Calling it "split" there makes sense.  Here,
> > calling it "split"
> > > is confusing, but as long as the code is correct, I think it's ok.
> >
> > AMD NTB has similar design.
> 
> If by similar design, you mean that BAR2+3 or BAR4+5 can be split, those
> configurations are not currently supported by this driver.  If needed, support
> for those configurations can be added later, as a patch.
> 
> Ok with the current implementation.
> 
> > > I see a lot of readl/writel and ioread/iowrite in the same file,
> > > even
> > in the
> > > same function as there is here.  Pick one variant of the functions,
> > preferably
> > > the ioread/iowrite variant, and be consistent in its usage
> > > throughout
> > the file.
> >
> > Ioread/iowrite is only for 64bit read/write, I don't think it has any
> > confusion.
> 
> It's not just for 64bit: there are ioread8,16,32.  In fact, ioread64 is the one
> that's not provided by io.h: not all hardware can do a true 64bit read or write.
> We don't need a true 64bit operation, so ioread64 is provided locally, for
> convenience within this driver.
> 
> > Actually, Intel NTB driver has same behavior.
> 
> Actually, Intel NTB driver does not use readl/writel.
> 
> Would you please fix this?

I'll change ioread64/iowrite64 to read64/write64, keep consistent witch readl/readw

> > > It's also interesting that if it hits the last branch,
> > > ndev->peer_sta
> > is assigned
> > > zero, but the function returns zero, not LNK_STA.  If the function
> > were
> > > immediately called again, it would return LNK_STA.  Can you please
> > explain
> > > the logic here?
> >
> > Force to read link status register again because the opposite side
> > maybe still In resuming progress, the status variable can't show the
> > right state, have to Read register directly.
> 
> There's no indication in this function that it interacts with the timer.  With
> your suggestion, I'll take a closer look at the interactions between this
> function and the timer.

I'll add a comment.