> > 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? > > 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.
