Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1312499 > unrolled thread

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

Started by"Allen Hubbe" <Allen.Hubbe@emc.com>
First post2016-01-19 21:40 +0100
Last post2016-01-21 03:20 +0100
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH V4 1/1] NTB: Add support for AMD PCI-Express Non-Transparent Bridge "Allen Hubbe" <Allen.Hubbe@emc.com> - 2016-01-19 21:40 +0100
    RE: [PATCH V4 1/1] NTB: Add support for AMD PCI-Express  Non-Transparent Bridge "Yu, Xiangliang" <Xiangliang.Yu@amd.com> - 2016-01-20 04:20 +0100
      RE: [PATCH V4 1/1] NTB: Add support for AMD PCI-Express Non-Transparent Bridge "Allen Hubbe" <Allen.Hubbe@emc.com> - 2016-01-20 17:30 +0100
        RE: [PATCH V4 1/1] NTB: Add support for AMD PCI-Express  Non-Transparent Bridge "Yu, Xiangliang" <Xiangliang.Yu@amd.com> - 2016-01-21 03:20 +0100

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

From"Allen Hubbe" <Allen.Hubbe@emc.com>
Date2016-01-19 21:40 +0100
SubjectRE: [PATCH V4 1/1] NTB: Add support for AMD PCI-Express Non-Transparent Bridge
Message-ID<qSLej-4ue-39@gated-at.bofh.it>
From: Xiangliang Yu <Xiangliang.Yu@amd.com>
> 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@amd.com>

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

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.

> +		/* 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.

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

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?

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

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

Thanks,
Allen

[toc] | [next] | [standalone]

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

From"Yu, Xiangliang" <Xiangliang.Yu@amd.com>
Date2016-01-20 04:20 +0100
SubjectRE: [PATCH V4 1/1] NTB: Add support for AMD PCI-Express Non-Transparent Bridge
Message-ID<qSRto-xe-5@gated-at.bofh.it>
In reply to#1312499
> From: Xiangliang Yu <Xiangliang.Yu@amd.com>
> > 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@amd.com>
> 
> > +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.

[toc] | [prev] | [next] | [standalone]

#1313337

From"Allen Hubbe" <Allen.Hubbe@emc.com>
Date2016-01-20 17:30 +0100
Message-ID<qT3NU-CC-7@gated-at.bofh.it>
In reply to#1312853
> > 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.

[toc] | [prev] | [next] | [standalone]

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

From"Yu, Xiangliang" <Xiangliang.Yu@amd.com>
Date2016-01-21 03:20 +0100
SubjectRE: [PATCH V4 1/1] NTB: Add support for AMD PCI-Express Non-Transparent Bridge
Message-ID<qTd0R-6UH-3@gated-at.bofh.it>
In reply to#1313337
> > > 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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web