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

[Yu, Xiangliang]

> From: Xiangliang Yu <Xiangliang.Yu@xxxxxxx>
> > This adds support for AMD's PCI-Express Non-Transparent Bridge
> > (NTB) device on the Zeppelin platform. The driver connnects to the
> > standard NTB sub-system interface, with modification to add hooks for
> > power management in a separate patch. The AMD NTB device has 3
> memory
> > windows, 16 doorbell, 16 scratch-pad registers, and supports up to 16
> > PCIe lanes running a Gen3 speeds.
> >
> > Signed-off-by: Xiangliang Yu <Xiangliang.Yu@xxxxxxx>
> 
> > +static int amd_ntb_mw_set_trans(struct ntb_dev *ntb, int idx,
> > +				dma_addr_t addr, resource_size_t size) {
> 
> 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 

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

> 
> > +		/* set and verify setting the translation address */
> > +		iowrite64(addr, peer_mmio + xlat_reg);
> > +		reg_val = ioread64(peer_mmio + xlat_reg);
> > +		if (reg_val != addr) {
> > +			iowrite64(0, peer_mmio + xlat_reg);
> > +			return -EIO;
> > +		}
> > +
> > +		/* set and verify setting the limit */
> > +		writel(limit, mmio + limit_reg);
> > +		reg_val = readl(mmio + limit_reg);
> > +		if (reg_val != limit) {
> > +			writel(base_addr, mmio + limit_reg);
> > +			writel(0, peer_mmio + xlat_reg);
> > +			return -EIO;
> > +		}
> 
> 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.
Actually, Intel NTB driver has same behavior.

> > +static int amd_link_is_up(struct amd_ntb_dev *ndev) {
> > +	if (!ndev->peer_sta)
> > +		return NTB_LNK_STA_ACTIVE(ndev->cntl_sta);
> > +	else if (ndev->peer_sta & AMD_PEER_RESET_EVENT)
> > +		ndev->peer_sta &= ~AMD_PEER_RESET_EVENT;
> > +	else if (ndev->peer_sta & AMD_PEER_D0_EVENT)
> > +		ndev->peer_sta = 0;
> > +
> > +	return 0;
> > +}
> 
> This function changed since v3.
> 
> The first "else" is not necessary after a branch that returns.  It would be more
> clearly written as follows.

Make sense

> if (!ndev->peer_sta)
> 	return LNK_STA;
> 
> if (ndev->peer_sta & RESET)
> 	...;
> else if (ndev->peer_sta & D0)
> 	...;
> 
> return 0;
> 
> 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.

> The upper layer may poll the link status at any time, so unless the link status
> actually changed (as indicated by the hardware) between polls, the result of
> polling should be the same.
> 
> If the first time polling returns zero as a result of peer_sta,
>  - AND the second time polling returns LNK_STA,
>  - AND the hardware link state is UP,

Start a timer to poll hardware link status and will change status variable if link
Change. This will make it only check link variable, not peer_sta.
Please check timer code.

>  - AND the only difference is not the hardware
>     link state but the value of peer_sta,
>  - THEN this is a bug.
> 
> I should have noticed and made the comment in v1, which had the same
> behavior, though it was more explicit.

V4 make code clear, so I change it.

> From v1:
> > +	} else if (ndev->peer_sta & AMD_PEER_D0_EVENT) {
> > +		ndev->peer_sta = 0;
> > +		return 0;

And please spend more time to go through all code, and I'll waiting for your
Feedback. I have send the code long time ago. It make me more comfortable
If you can comment all concern in one version.