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

Groups > linux.kernel > #1313856 > unrolled thread

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

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 V5 1/1] NTB: Add support for AMD PCI-Express Non-Transparent Bridge "Allen Hubbe" <Allen.Hubbe@emc.com> - 2016-01-21 05:40 +0100
    RE: [PATCH V5 1/1] NTB: Add support for AMD PCI-Express  Non-Transparent Bridge "Yu, Xiangliang" <Xiangliang.Yu@amd.com> - 2016-01-21 07:00 +0100
      RE: [PATCH V5 1/1] NTB: Add support for AMD PCI-Express Non-Transparent Bridge "Allen Hubbe" <Allen.Hubbe@emc.com> - 2016-01-21 07:20 +0100
        RE: [PATCH V5 1/1] NTB: Add support for AMD PCI-Express  Non-Transparent Bridge "Yu, Xiangliang" <Xiangliang.Yu@amd.com> - 2016-01-21 07:40 +0100

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

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>

> Signed-off-by: Jon Mason <jdmason@kudzu.us>
> Signed-off-by: Allen Hubbe <Allen.Hubbe@emc.com>

NO.


> +		/* set and verify setting the translation address */
> +		write64(addr, peer_mmio + xlat_reg);
> +		reg_val = read64(peer_mmio + xlat_reg);
> +		if (reg_val != addr) {
> +			write64(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 what you did there, change iowrite64 to write64.

What I meant was:
 - change readl to ioread32.
 - change writel to iowrite32.
 - change readb, readw, writeb, writew (if there are any)
 - leave ioread64 and iowrite64 as they were.

Why: http://www.makelinux.net/ldd3/chp-9-sect-4

Quote: "If you read through the kernel source, you see many calls to an older set of functions when I/O memory is being used. These functions still work, but their use in new code is discouraged. Among other things, they are less safe because they do not perform the same sort of type checking."

The "older set of functions" are read[bwl], write[bwl].  This is a new driver, with all new code.  Please use the ioread/iowrite variants.

> +static int amd_link_is_up(struct amd_ntb_dev *ndev)
> +{
> +	if (!ndev->peer_sta)
> +		return NTB_LNK_STA_ACTIVE(ndev->cntl_sta);
> +
> +	/* If peer_sta is reset or D0 event, the ISR has
> +	 * started a timer to check link status of hardware.
> +	 * So here just clear status bit. And if peer_sta is
> +	 * D3 or PME_TO, D0/reset event will be happened when
> +	 * system wakeup/poweron, so do nothing here.
> +	 */
> +	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;
> +}

Thanks.  This is much better.

> +static void amd_handle_event(struct amd_ntb_dev *ndev, int vec)
...
> +	case AMD_PEER_D0_EVENT:
...
> +		/* start a timer to poll link status */
> +		schedule_delayed_work(&ndev->hb_timer,
> +					      AMD_LINK_HB_TIMEOUT);

This is different from v4.  It used to be:

if (amd_link_is_up())
	ntb_link_event();
else
	schedule_delayed_work();

Why is v5 correct?
Why was v4 incorrect?

I'm nervous about ndev->peer_sta, the behavior of link_is_up, timers... unexplained changes to a fragile bit of code - not just this code, but any code that deals with parallel or asynchronous behaviors.  With the comment in link_is_up, this code is much better, but any changes to this whole link state mechanism need to be explained.


Allen

[toc] | [next] | [standalone]

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

> 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>
> 
> > Signed-off-by: Jon Mason <jdmason@kudzu.us>
> > Signed-off-by: Allen Hubbe <Allen.Hubbe@emc.com>
> 
> NO.

Ok, I'll change it if you doesn't want to change it.

> 
> > +		/* set and verify setting the translation address */
> > +		write64(addr, peer_mmio + xlat_reg);
> > +		reg_val = read64(peer_mmio + xlat_reg);
> > +		if (reg_val != addr) {
> > +			write64(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 what you did there, change iowrite64 to write64.
> 
> What I meant was:
>  - change readl to ioread32.
>  - change writel to iowrite32.
>  - change readb, readw, writeb, writew (if there are any)
>  - leave ioread64 and iowrite64 as they were.
> 
> Why: http://www.makelinux.net/ldd3/chp-9-sect-4
> 
> Quote: "If you read through the kernel source, you see many calls to an older
> set of functions when I/O memory is being used. These functions still work,
> but their use in new code is discouraged. Among other things, they are less
> safe because they do not perform the same sort of type checking."
> 
> The "older set of functions" are read[bwl], write[bwl].  This is a new driver,
> with all new code.  Please use the ioread/iowrite variants.

I don’t think so. In here, the i/o memory is only happened when pci_iomap return
Success, so the register can't be accessed through IO port way. And ioread* will
Check if the memory type is mmio type or IO port type (please see the definition).
 I don’t think we need to check It, so I use read* because It can make more efficient. 
I think we need to think about actual usage, not only follow book.
And, I have said it in previous version, I don’t like explain it again, and again.
If you have any concern, please tell me after my comment.

> > +static int amd_link_is_up(struct amd_ntb_dev *ndev) {
> > +	if (!ndev->peer_sta)
> > +		return NTB_LNK_STA_ACTIVE(ndev->cntl_sta);
> > +
> > +	/* If peer_sta is reset or D0 event, the ISR has
> > +	 * started a timer to check link status of hardware.
> > +	 * So here just clear status bit. And if peer_sta is
> > +	 * D3 or PME_TO, D0/reset event will be happened when
> > +	 * system wakeup/poweron, so do nothing here.
> > +	 */
> > +	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;
> > +}
> 
> Thanks.  This is much better.
> 
> > +static void amd_handle_event(struct amd_ntb_dev *ndev, int vec)
> ...
> > +	case AMD_PEER_D0_EVENT:
> ...
> > +		/* start a timer to poll link status */
> > +		schedule_delayed_work(&ndev->hb_timer,
> > +					      AMD_LINK_HB_TIMEOUT);
> 
> This is different from v4.  It used to be:
> 
> if (amd_link_is_up())
> 	ntb_link_event();
> else
> 	schedule_delayed_work();
> 
> Why is v5 correct?
> Why was v4 incorrect?

Because peer_sta is change to 0, so amd_link_is_up will return 0 (offline)
And will not check hardware link status. So It maybe make it offline forever

> I'm nervous about ndev->peer_sta, the behavior of link_is_up, timers...
> unexplained changes to a fragile bit of code - not just this code, but any code
> that deals with parallel or asynchronous behaviors.  With the comment in
> link_is_up, this code is much better, but any changes to this whole link state
> mechanism need to be explained.
Actually, the code is designed according to Atom NTB, except for the peer_sta. 
I'll add the explaination when having changes.

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

#1313940

From: Yu, Xiangliang [mailto:Xiangliang.Yu@amd.com]
> > > Signed-off-by: Jon Mason <jdmason@kudzu.us>
> > > Signed-off-by: Allen Hubbe <Allen.Hubbe@emc.com>
> >
> > NO.
> 
> Ok, I'll change it if you doesn't want to change it.

Nah, just remember it for next time...

I'm satisfied with this v5.

Reviewed-by: Allen Hubbe <Allen.Hubbe@emc.com>

> I don’t think so. In here, the i/o memory is only happened when
> pci_iomap return
> Success, so the register can't be accessed through IO port way. And
> ioread* will
> Check if the memory type is mmio type or IO port type (please see the
> definition).
>  I don’t think we need to check It, so I use read* because It can make
> more efficient.
> I think we need to think about actual usage, not only follow book.
> And, I have said it in previous version, I don’t like explain it again,
> and again.
> If you have any concern, please tell me after my comment.

It's not more efficient, on this platform it's the same.

If it were my driver I would change it... but you can keep it this way.

> > This is different from v4.  It used to be:
> 
> Because peer_sta is change to 0, so amd_link_is_up will return 0
> (offline)
> And will not check hardware link status. So It maybe make it offline
> forever

It fixed a bug?  Great!

> > I'm nervous about ndev->peer_sta, the behavior of link_is_up,
> > timers...
>
> Actually, the code is designed according to Atom NTB, except for the
> peer_sta.

Except for peer_sta, and that's a pretty critical design change.  I'm still nervous, but I'll trust that you have been able to test this behavior thourougly.

> I'll add the explaination when having changes.

Thanks.

Allen

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

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

> From: Yu, Xiangliang [mailto:Xiangliang.Yu@amd.com]
> > > > Signed-off-by: Jon Mason <jdmason@kudzu.us>
> > > > Signed-off-by: Allen Hubbe <Allen.Hubbe@emc.com>
> > >
> > > NO.
> >
> > Ok, I'll change it if you doesn't want to change it.
> 
> Nah, just remember it for next time...
> 
> I'm satisfied with this v5.
> 
> Reviewed-by: Allen Hubbe <Allen.Hubbe@emc.com>

Ok, I'll change it and resend V5

> > I don’t think so. In here, the i/o memory is only happened when
> > pci_iomap return Success, so the register can't be accessed through IO
> > port way. And
> > ioread* will
> > Check if the memory type is mmio type or IO port type (please see the
> > definition).
> >  I don’t think we need to check It, so I use read* because It can make
> > more efficient.
> > I think we need to think about actual usage, not only follow book.
> > And, I have said it in previous version, I don’t like explain it
> > again, and again.
> > If you have any concern, please tell me after my comment.
> 
> It's not more efficient, on this platform it's the same.
> 
> If it were my driver I would change it... but you can keep it this way.

Because my previous SATA experience, I'd like to use this way.

> > > This is different from v4.  It used to be:
> >
> > Because peer_sta is change to 0, so amd_link_is_up will return 0
> > (offline)
> > And will not check hardware link status. So It maybe make it offline
> > forever
> 
> It fixed a bug?  Great!
> 
> > > I'm nervous about ndev->peer_sta, the behavior of link_is_up,
> > > timers...
> >
> > Actually, the code is designed according to Atom NTB, except for the
> > peer_sta.
> 
> Except for peer_sta, and that's a pretty critical design change.  I'm still
> nervous, but I'll trust that you have been able to test this behavior
> thourougly.

Yes, the part of code will be changed in future because hardware design is
Being changed  too.

> 
> > I'll add the explaination when having changes.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web