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

Groups > linux.kernel > #1303251 > unrolled thread

RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver

Started by"Yu, Xiangliang" <Xiangliang.Yu@amd.com>
First post2016-01-07 04:00 +0100
Last post2016-01-11 08:30 +0100
Articles 4 — 3 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 V2 1/3] NTB: Add AMD PCI-Express NTB driver "Yu, Xiangliang" <Xiangliang.Yu@amd.com> - 2016-01-07 04:00 +0100
    Re: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver Jon Mason <jdmason@kudzu.us> - 2016-01-08 16:10 +0100
      RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver "Allen Hubbe" <Allen.Hubbe@emc.com> - 2016-01-08 16:50 +0100
        RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver "Yu, Xiangliang" <Xiangliang.Yu@amd.com> - 2016-01-11 08:30 +0100

#1303251 — RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver

From"Yu, Xiangliang" <Xiangliang.Yu@amd.com>
Date2016-01-07 04:00 +0100
SubjectRE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver
Message-ID<qO8XT-1Eh-1@gated-at.bofh.it>
PiA+ICsNCj4gPiArI2RlZmluZSAgICAgICAgUENJX0RFVklDRV9JRF9BTURfTlRCICAgMHgxNDVC
DQo+IA0KPiBUaGlzIGxvb2tzIGxpa2UgYSB0YWIgYW5kIG5vdCBhIHNwYWNlDQoNCkknbGwgdXBk
YXRlIGl0Lg0KDQo+IA0KPiA+ICsjZGVmaW5lIEFNRF9MSU5LX0hCX1RJTUVPVVQgICAgbXNlY3Nf
dG9famlmZmllcygxMDAwKQ0KPiA+ICsjZGVmaW5lIEFNRF9MSU5LX1NUQVRVU19PRkZTRVQgMHg2
OA0KPiA+ICsjZGVmaW5lIE5UQl9MSU5fU1RBX0FDVElWRV9CSVQgMHgwMDAwMDAwMg0KPiA+ICsj
ZGVmaW5lIE5UQl9MTktfU1RBX1NQRUVEX01BU0sgMHgwMDBGMDAwMA0KPiA+ICsjZGVmaW5lIE5U
Ql9MTktfU1RBX1dJRFRIX01BU0sgMHgwM0YwMDAwMA0KPiA+ICsjZGVmaW5lIE5UQl9MTktfU1RB
X0FDVElWRSh4KSAgKCEhKCh4KSAmIE5UQl9MSU5fU1RBX0FDVElWRV9CSVQpKQ0KPiA+ICsjZGVm
aW5lIE5UQl9MTktfU1RBX1NQRUVEKHgpICAgKCgoeCkgJiBOVEJfTE5LX1NUQV9TUEVFRF9NQVNL
KSA+Pg0KPiAxNikNCj4gPiArI2RlZmluZSBOVEJfTE5LX1NUQV9XSURUSCh4KSAgICgoKHgpICYN
Cj4gTlRCX0xOS19TVEFfV0lEVEhfTUFTSykgPj4gMjApDQo+ID4gKw0KPiA+ICsjaWZuZGVmIGlv
cmVhZDY0DQo+ID4gKyNpZmRlZiByZWFkcQ0KPiA+ICsjZGVmaW5lIGlvcmVhZDY0IHJlYWRxDQo+
ID4gKyNlbHNlDQo+ID4gKyNkZWZpbmUgaW9yZWFkNjQgX2lvcmVhZDY0DQo+ID4gK3N0YXRpYyBp
bmxpbmUgdTY0IF9pb3JlYWQ2NCh2b2lkIF9faW9tZW0gKm1taW8pDQo+ID4gK3sNCj4gPiArICAg
ICAgIHU2NCBsb3csIGhpZ2g7DQo+ID4gKw0KPiA+ICsgICAgICAgbG93ID0gaW9yZWFkMzIobW1p
byk7DQo+ID4gKyAgICAgICBoaWdoID0gaW9yZWFkMzIobW1pbyArIHNpemVvZih1MzIpKTsNCj4g
PiArICAgICAgIHJldHVybiBsb3cgfCAoaGlnaCA8PCAzMik7DQo+ID4gK30NCj4gPiArI2VuZGlm
DQo+ID4gKyNlbmRpZg0KPiA+ICsNCj4gPiArI2lmbmRlZiBpb3dyaXRlNjQNCj4gPiArI2lmZGVm
IHdyaXRlcQ0KPiA+ICsjZGVmaW5lIGlvd3JpdGU2NCB3cml0ZXENCj4gPiArI2Vsc2UNCj4gPiAr
I2RlZmluZSBpb3dyaXRlNjQgX2lvd3JpdGU2NA0KPiA+ICtzdGF0aWMgaW5saW5lIHZvaWQgX2lv
d3JpdGU2NCh1NjQgdmFsLCB2b2lkIF9faW9tZW0gKm1taW8pDQo+ID4gK3sNCj4gPiArICAgICAg
IGlvd3JpdGUzMih2YWwsIG1taW8pOw0KPiA+ICsgICAgICAgaW93cml0ZTMyKHZhbCA+PiAzMiwg
bW1pbyArIHNpemVvZih1MzIpKTsNCj4gPiArfQ0KPiA+ICsjZW5kaWYNCj4gPiArI2VuZGlmDQo+
ID4NCg0KSG93IGFib3V0IHB1dCBpb3JlYWQ2NC9pb3dyaXRlNjQgbWFjcm8gaW50byBudGIuaCBm
aWxlPyBTbyBsb3cgbGV2ZWwgZHJpdmVyIGNhbiBhdm9pZCB0byBkZWZpbmUgdGhlc2UgbWFjcm8u
DQoNCj4gPiArI2RlZmluZSBOVEJfUkVBRF9SRUcoYmFzZSwgcikgKGlvcmVhZDMyKGJhc2UgKyBB
TURfICMjIHIgIyMNCj4gX09GRlNFVCkpDQo+ID4gKyNkZWZpbmUgTlRCX1dSSVRFX1JFRyhiYXNl
LCB2YWwsIHIpIChpb3dyaXRlMzIodmFsLCBiYXNlICsgICAgIFwNCj4gPiArICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBBTURfICMjIHIgIyMgX09GRlNFVCkp
DQo+ID4gKyNkZWZpbmUgTlRCX1JFQURfT0ZGU0VUKGJhc2UsIHIsIG9mKSAoaW9yZWFkMzIoYmFz
ZSArIG9mICsgICAgICAgICAgICAgXA0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgIEFNRF8gIyMgciAjIyBfT0ZGU0VUKSkNCj4gPiArI2RlZmluZSBO
VEJfV1JJVEVfT0ZGU0VUKGJhc2UsIHZhbCwgciwgb2YpIChpb3dyaXRlMzIodmFsLCBiYXNlICsg
ICAgICBcDQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgb2YgKyBBTURfICMjIHIgIyMgX09GRlNFVCkpDQo+IA0KPiBQbGVhc2UgZG8gbm90IHVzZSBt
YXJjb3MgdG8gaGlkZSBpb3JlYWQvaW93cml0ZS4gIENhbGwgaW9yd2FkL2lvd3JpdGUgZGlyZWN0
bHkuDQoNCkkgZG9uJ3Qgc2VlIGFueSB3cm9uZyB0byBoaWRlIGlvcmVhZC9pb3dyaXRlLCBhbmQg
SSB0aGluayB0aGUgbWFjcm9zIGNhbiBtYWtlIGNvZGUgcmVhZGFibGUgYW5kIGVhc3kgdG8gbWFp
bnRhaW4uIA0KDQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1304637

FromJon Mason <jdmason@kudzu.us>
Date2016-01-08 16:10 +0100
Message-ID<qOGPU-8rM-39@gated-at.bofh.it>
In reply to#1303251
On Wed, Jan 6, 2016 at 9:50 PM, Yu, Xiangliang <Xiangliang.Yu@amd.com> wrote:
>> > +
>> > +#define        PCI_DEVICE_ID_AMD_NTB   0x145B
>>
>> This looks like a tab and not a space
>
> I'll update it.
>
>>
>> > +#define AMD_LINK_HB_TIMEOUT    msecs_to_jiffies(1000)
>> > +#define AMD_LINK_STATUS_OFFSET 0x68
>> > +#define NTB_LIN_STA_ACTIVE_BIT 0x00000002
>> > +#define NTB_LNK_STA_SPEED_MASK 0x000F0000
>> > +#define NTB_LNK_STA_WIDTH_MASK 0x03F00000
>> > +#define NTB_LNK_STA_ACTIVE(x)  (!!((x) & NTB_LIN_STA_ACTIVE_BIT))
>> > +#define NTB_LNK_STA_SPEED(x)   (((x) & NTB_LNK_STA_SPEED_MASK) >>
>> 16)
>> > +#define NTB_LNK_STA_WIDTH(x)   (((x) &
>> NTB_LNK_STA_WIDTH_MASK) >> 20)
>> > +
>> > +#ifndef ioread64
>> > +#ifdef readq
>> > +#define ioread64 readq
>> > +#else
>> > +#define ioread64 _ioread64
>> > +static inline u64 _ioread64(void __iomem *mmio)
>> > +{
>> > +       u64 low, high;
>> > +
>> > +       low = ioread32(mmio);
>> > +       high = ioread32(mmio + sizeof(u32));
>> > +       return low | (high << 32);
>> > +}
>> > +#endif
>> > +#endif
>> > +
>> > +#ifndef iowrite64
>> > +#ifdef writeq
>> > +#define iowrite64 writeq
>> > +#else
>> > +#define iowrite64 _iowrite64
>> > +static inline void _iowrite64(u64 val, void __iomem *mmio)
>> > +{
>> > +       iowrite32(val, mmio);
>> > +       iowrite32(val >> 32, mmio + sizeof(u32));
>> > +}
>> > +#endif
>> > +#endif
>> >
>
> How about put ioread64/iowrite64 macro into ntb.h file? So low level driver can avoid to define these macro.
>
>> > +#define NTB_READ_REG(base, r) (ioread32(base + AMD_ ## r ##
>> _OFFSET))
>> > +#define NTB_WRITE_REG(base, val, r) (iowrite32(val, base +     \
>> > +                                               AMD_ ## r ## _OFFSET))
>> > +#define NTB_READ_OFFSET(base, r, of) (ioread32(base + of +             \
>> > +                                               AMD_ ## r ## _OFFSET))
>> > +#define NTB_WRITE_OFFSET(base, val, r, of) (iowrite32(val, base +      \
>> > +                                               of + AMD_ ## r ## _OFFSET))
>>
>> Please do not use marcos to hide ioread/iowrite.  Call iorwad/iowrite directly.
>
> I don't see any wrong to hide ioread/iowrite, and I think the macros can make code readable and easy to maintain.

I disagree.  It is an unnecessary layer and can add to confusion.
Please make the change.

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

#1304669

From"Allen Hubbe" <Allen.Hubbe@emc.com>
Date2016-01-08 16:50 +0100
Message-ID<qOHsC-gc-27@gated-at.bofh.it>
In reply to#1304637
From: Jon Mason <jdmason@kudzu.us>
> On Wed, Jan 6, 2016 at 9:50 PM, Yu, Xiangliang <Xiangliang.Yu@amd.com>
> wrote:
> >> > +#define NTB_READ_REG(base, r) (ioread32(base + AMD_ ## r ##
> >> _OFFSET))
> >> > +#define NTB_WRITE_REG(base, val, r) (iowrite32(val, base +     \
> >> > +                                               AMD_ ## r ##
> _OFFSET))
> >> > +#define NTB_READ_OFFSET(base, r, of) (ioread32(base + of +
> \
> >> > +                                               AMD_ ## r ##
> _OFFSET))
> >> > +#define NTB_WRITE_OFFSET(base, val, r, of) (iowrite32(val, base +
> \
> >> > +                                               of + AMD_ ## r ##
> _OFFSET))
> >>
> >> Please do not use marcos to hide ioread/iowrite.  Call iorwad/iowrite
> directly.
> >
> > I don't see any wrong to hide ioread/iowrite, and I think the macros
> can make code readable and easy to maintain.
> 
> I disagree.  It is an unnecessary layer and can add to confusion.
> Please make the change.

I don't like AMD_##r##_OFFSET in these macros.  It hides the use of a globally named constant like AMD_FOO_OFFSET, since one would read only FOO in the code.  It makes cross referencing difficult, since the reader needs to know FOO is really AMD_FOO_OFFSET.  This would defeat automatic cross referencing like cscope and lxr.

#define AMD_FOO_OFFSET 0xc0ff33
vs
NTB_READ_OFFSET(dev->foo_base, FOO, offset_in_foo)
// Where is FOO defined?

This macro would have at least been better written without ##; so cross referencing would still work.

NTB_READ_OFFSET(dev->foo_base, FOO, offset_in_foo)
vs
NTB_READ_OFFSET(dev->foo_base, AMD_FOO_OFFSET, offset_in_foo)
// AMD_FOO_OFFSET is 0xcoff33 (obviously)

But without ##, the macro is just the addition of its parameters.  Change the commas to addition, and the macro to ioread, and you'll see there is no benefit for having this macro any more.

NTB_READ_OFFSET(dev->foo_base, AMD_FOO_OFFSET, offset_in_foo)
vs
ioread32(dev->foo_base + AMD_FOO_OFFSET + offset_in_foo)

I second Jon's opinion.  Please make the change.  This would be better as simply ioread/write in the code.

Allen

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

#1305911

From"Yu, Xiangliang" <Xiangliang.Yu@amd.com>
Date2016-01-11 08:30 +0100
Message-ID<qPF5o-7yx-7@gated-at.bofh.it>
In reply to#1304669
> From: Jon Mason <jdmason@kudzu.us>
> > On Wed, Jan 6, 2016 at 9:50 PM, Yu, Xiangliang <Xiangliang.Yu@amd.com>
> > wrote:
> > >> > +#define NTB_READ_REG(base, r) (ioread32(base + AMD_ ## r ##
> > >> _OFFSET))
> > >> > +#define NTB_WRITE_REG(base, val, r) (iowrite32(val, base +     \
> > >> > +                                               AMD_ ## r ##
> > _OFFSET))
> > >> > +#define NTB_READ_OFFSET(base, r, of) (ioread32(base + of +
> > \
> > >> > +                                               AMD_ ## r ##
> > _OFFSET))
> > >> > +#define NTB_WRITE_OFFSET(base, val, r, of) (iowrite32(val, base
> > >> > ++
> > \
> > >> > +                                               of + AMD_ ## r ##
> > _OFFSET))
> > >>
> > >> Please do not use marcos to hide ioread/iowrite.  Call
> > >> iorwad/iowrite
> > directly.
> > >
> > > I don't see any wrong to hide ioread/iowrite, and I think the macros
> > can make code readable and easy to maintain.
> >
> > I disagree.  It is an unnecessary layer and can add to confusion.
> > Please make the change.
> 
> I don't like AMD_##r##_OFFSET in these macros.  It hides the use of a
> globally named constant like AMD_FOO_OFFSET, since one would read only
> FOO in the code.  It makes cross referencing difficult, since the reader needs
> to know FOO is really AMD_FOO_OFFSET.  This would defeat automatic cross
> referencing like cscope and lxr.
> 
> #define AMD_FOO_OFFSET 0xc0ff33
> vs
> NTB_READ_OFFSET(dev->foo_base, FOO, offset_in_foo) // Where is FOO
> defined?
> 
> This macro would have at least been better written without ##; so cross
> referencing would still work.
> 
> NTB_READ_OFFSET(dev->foo_base, FOO, offset_in_foo) vs
> NTB_READ_OFFSET(dev->foo_base, AMD_FOO_OFFSET, offset_in_foo) //
> AMD_FOO_OFFSET is 0xcoff33 (obviously)
> 
> But without ##, the macro is just the addition of its parameters.  Change the
> commas to addition, and the macro to ioread, and you'll see there is no
> benefit for having this macro any more.
> 
> NTB_READ_OFFSET(dev->foo_base, AMD_FOO_OFFSET, offset_in_foo) vs
> ioread32(dev->foo_base + AMD_FOO_OFFSET + offset_in_foo)
> 
> I second Jon's opinion.  Please make the change.  This would be better as
> simply ioread/write in the code.

The main aim of these macros is to focuses on the name of register (FOO), 
not the whole macros of register. 
and I see there are lots of these style in kernel.
If your guys still can't accept the style, I can change it. 
And I'll change ioread/iowrite to readl/writel because it only access mmio space.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web