From: David Laight <David.Laight@ACULAB.COM>
To: 'Ben Dooks' <ben.dooks@codethink.co.uk>,
Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>,
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"wg@grandegger.com" <wg@grandegger.com>,
"linux-can@vger.kernel.org" <linux-can@vger.kernel.org>,
Linux-sh list <linux-sh@vger.kernel.org>,
"vksavl@gmail.com" <vksavl@gmail.com>
Subject: RE: [PATCH v5] can: add Renesas R-Car CAN driver
Date: Mon, 20 Jan 2014 12:13:23 +0000 [thread overview]
Message-ID: <063D6719AE5E284EB5DD2968C1650D6D460082@AcuExch.aculab.com> (raw)
In-Reply-To: <52DD1058.4090205@codethink.co.uk>
RnJvbTogDQo+IE9uIDIwLzAxLzE0IDExOjU4LCBNYXJjIEtsZWluZS1CdWRkZSB3cm90ZToNCj4g
PiBPbiAwMS8yMC8yMDE0IDEyOjUyIFBNLCBHZWVydCBVeXR0ZXJob2V2ZW4gd3JvdGU6DQo+ID4+
IE9uIE1vbiwgSmFuIDIwLCAyMDE0IGF0IDEyOjQ3IFBNLCBNYXJjIEtsZWluZS1CdWRkZSA8bWts
QHBlbmd1dHJvbml4LmRlPiB3cm90ZToNCj4gPj4+IE9uIDAxLzIwLzIwMTQgMTI6NDMgUE0sIEdl
ZXJ0IFV5dHRlcmhvZXZlbiB3cm90ZToNCj4gPj4+PiBPbiBUaHUsIERlYyAyNiwgMjAxMyBhdCAx
MDozNyBQTSwgU2VyZ2VpIFNodHlseW92DQo+ID4+Pj4gPHNlcmdlaS5zaHR5bHlvdkBjb2dlbnRl
bWJlZGRlZC5jb20+IHdyb3RlOg0KPiA+Pj4+PiBDaGFuZ2VzIGluIHZlcnNpb24gMzoNCj4gPj4+
Pg0KPiA+Pj4+PiAtIGFkZGVkICdfX3BhY2tlZCcgdG8gJ3N0cnVjdCByY2FyX2Nhbl9tYm94X3Jl
Z3MnIGFuZCAnc3RydWN0IHJjYXJfY2FuX3JlZ3MnOw0KPiA+Pj4+PiAtIHJlbW92ZWQgdW5uZWVk
ZWQgdHlwZSBjYXN0IGluIHRoZSBwcm9iZSgpIG1ldGhvZC4NCj4gPj4+Pg0KPiA+Pj4+PiArLyog
TWFpbGJveCByZWdpc3RlcnMgc3RydWN0dXJlICovDQo+ID4+Pj4+ICtzdHJ1Y3QgcmNhcl9jYW5f
bWJveF9yZWdzIHsNCj4gPj4+Pj4gKyAgICAgICB1MzIgaWQ7ICAgICAgICAgLyogSURFIGFuZCBS
VFIgYml0cywgU0lEIGFuZCBFSUQgKi8NCj4gPj4+Pj4gKyAgICAgICB1OCBzdHViOyAgICAgICAg
LyogTm90IHVzZWQgKi8NCj4gPj4+Pj4gKyAgICAgICB1OCBkbGM7ICAgICAgICAgLyogRGF0YSBM
ZW5ndGggQ29kZSAtIGJpdHMgWzAuLjNdICovDQo+ID4+Pj4+ICsgICAgICAgdTggZGF0YVs4XTsg
ICAgIC8qIERhdGEgQnl0ZXMgKi8NCj4gPj4+Pj4gKyAgICAgICB1OCB0c2g7ICAgICAgICAgLyog
VGltZSBTdGFtcCBIaWdoZXIgQnl0ZSAqLw0KPiA+Pj4+PiArICAgICAgIHU4IHRzbDsgICAgICAg
ICAvKiBUaW1lIFN0YW1wIExvd2VyIEJ5dGUgKi8NCj4gPj4+Pj4gK30gX19wYWNrZWQ7DQo+ID4+
Pj4NCj4gPj4+PiBTb3JyeSwgSSBtaXNzZWQgdGhlIGVhcmxpZXIgZGlzY3Vzc2lvbiwgYnV0IHdo
eSB0aGUgX3BhY2tlZD8NCj4gPj4+PiBPbmUgdTMyIGFuZCAxMiBieXRlcyBtYWtlcyBhIG5pY2Ug
bXVsdGlwbGUgb2YgNC4NCj4gPj4+DQo+ID4+PiBCZXR0ZXIgc2FmZSB0aGFuIHNvcnJ5LCBpdCdz
IHRoZSBsYXlvdXQgb2YgdGhlIHJlZ2lzdGVycyBpbiBoYXJkd2FyZSwNCj4gPj4+IGRvbid0IGxl
dCB0aGUgY29tcGlsZXIgb3B0aW1pemUgaGVyZS4NCg0KV2h5IG5vdCBqdXN0IGFkZCBhIGNvbXBp
bGUgdGltZSBjaGVjayBhZ2FpbnN0IHRoZSBzaXplIG9mIHRoZQ0Kc3RydWN0dXJlIC0gdGhhdCB3
aWxsIGVuc3VyZSB0aGF0IG5vIHBhZGRpbmcgaXMgYWNjaWRlbnRhbGx5IGFkZGVkLg0KDQo+ID4+
IEFjdHVhbGx5IF9fcGFja2VkIG1ha2VzIGl0IGxlc3Mgc2FmZSwgYXMgdGhlIGNvbXBpbGVyIG5v
dyBhc3N1bWVzDQo+ID4+IHRoZSB1MzIgaWQgbWVtYmVyIGlzIHVuYWxpZ25lZCwgYW5kIHRodXMg
bWF5IHR1cm4gMzItYml0IGFjY2Vzc2VzIGludG8gNA0KPiA+PiBieXRlIGFjY2Vzc2VzLg0KPiA+
Pg0KPiA+PiBGb3J0dW5hdGVseSBpdCB3b24ndCBoYXBwZW4gaW4gdGhpcyBjYXNlIGFzIHRoZSBj
b2RlIHVzZXMgd3JpdGVsL3JlYWRsIHRvDQo+ID4+IGFjY2VzIHRoZSBpZCBtZW1iZXIuDQoNCldo
aWNoIG1lYW5zIHRoYXQgaXQgd2lsbCBiZSBhbGlnbmVkIChhbmQgbXVzdCBiZSBhbGlnbmVkKS4N
ClNvIHRoZSBwYWNrZWQgaXMgY29tcGxldGVseSB1c2VsZXNzIGFuZCBwb2ludGxlc3MuDQoNCj4g
PiBZZXMsIGFzIHRoaXMgYXJlIHJlZ2lzdGVycyB0aGV5IG11c3Qgbm90IGJlIGFjY2Vzc2VkIGRp
cmVjdGx5LiBIb3dldmVyDQo+ID4gd2UgY2FuIHVzZSAiX19hdHRyaWJ1dGVfXyAoKHBhY2tlZCwg
YWxpZ25lZCg0KSkpIiB0byB0ZWxsIHRoZSBjb21waWxlcg0KPiA+IHRoYXQgdGhlIGJhc2UgYWRk
cmVzcyBvZiB0aGlzIHN0cnVjdCBpcyBhbHdheXMgYWxpZ25lZCB0byA0IGJ5dGVzLg0KPiANCj4g
SSB0aG91Z2h0IHdlJ2QgZ290dGVuIHBhc3QgdGhlIHN0YWdlIG9mIGV2ZXIgd3JpdGluZyByZWdp
c3Rlcg0KPiBkZWZpbml0aW9ucyBhcyBzdHJ1Y3R1cmVzPw0KDQpBbnkgb3RoZXIgd2F5IGlzIGxp
a2VseSB0byBiZSBtb3JlIGVycm9yIHByb25lIHNpbmNlIG5vdGhpbmcNCnRpZXMgdGhlIG9mZnNl
dHMgd2l0aCB0aGUgY29ycmVjdCBiYXNlIGFkZHJlc3MuDQoNCklNSE8gaXQgaXMgYmVzdCB0byBv
bmx5IHNwZWNpZnkgcGFja2VkIGlmIHRoZSBzdHJ1Y3R1cmUgaXMgZXhwZWN0ZWQNCnRvIGJlIG1p
c2FsaWduZWQuIE1pc2FsaWduZWQgbWVtYmVycyBjYW4gYmUgJ2ZpeGVkJyBieSBtYXJraW5nIHRo
ZW0NCmFsaWduZWQoKS4NCg0KRXZlbiB0aGVuIHlvdSBtaWdodCB3YW50IHRvIGRlZmluZSB0aGUg
c3RydWN0dXJlIGl0c2VsZiB3aXRob3V0DQp0aGUgJ3BhY2tlZCcgYW5kIHRoZSBkZWZpbmUgYSBz
ZWNvbmQgcGFja2VkIHN0cnVjdHVyZSBjb250YWluaW5nDQphIHNpbmdsZSBtZW1iZXIuDQoNCmVn
OiB5b3UgbWlnaHQgaGF2ZToNCg0Kc3RydWN0IGEgew0KCXUzMglhXzE7DQoJdTY0CWFfMiBfX2F0
dHJpYnV0ZV9fKChhbGlnbmVkKDQpKSk7DQoJdTMyCWFfMzsNCn07DQoNCnN0cnVjdCBhX3BhY2tl
ZCB7DQoJc3RydWN0IGEgcDsNCn0gX19hdHRyaWJ1dGVfXygocGFja2VkKSk7DQoNCllvdSBtaWdo
dCBldmVuIG5lZWQgYSB1bmlvbiBvZiB0aGUgdHdvIHR5cGVzIQ0KDQoJRGF2aWQNCg0K
WARNING: multiple messages have this Message-ID (diff)
From: David Laight <David.Laight@ACULAB.COM>
To: 'Ben Dooks' <ben.dooks@codethink.co.uk>,
Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>,
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"wg@grandegger.com" <wg@grandegger.com>,
"linux-can@vger.kernel.org" <linux-can@vger.kernel.org>,
Linux-sh list <linux-sh@vger.kernel.org>,
"vksavl@gmail.com" <vksavl@gmail.com>
Subject: RE: [PATCH v5] can: add Renesas R-Car CAN driver
Date: Mon, 20 Jan 2014 12:13:23 +0000 [thread overview]
Message-ID: <063D6719AE5E284EB5DD2968C1650D6D460082@AcuExch.aculab.com> (raw)
In-Reply-To: <52DD1058.4090205@codethink.co.uk>
From:
> On 20/01/14 11:58, Marc Kleine-Budde wrote:
> > On 01/20/2014 12:52 PM, Geert Uytterhoeven wrote:
> >> On Mon, Jan 20, 2014 at 12:47 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >>> On 01/20/2014 12:43 PM, Geert Uytterhoeven wrote:
> >>>> On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
> >>>> <sergei.shtylyov@cogentembedded.com> wrote:
> >>>>> Changes in version 3:
> >>>>
> >>>>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
> >>>>> - removed unneeded type cast in the probe() method.
> >>>>
> >>>>> +/* Mailbox registers structure */
> >>>>> +struct rcar_can_mbox_regs {
> >>>>> + u32 id; /* IDE and RTR bits, SID and EID */
> >>>>> + u8 stub; /* Not used */
> >>>>> + u8 dlc; /* Data Length Code - bits [0..3] */
> >>>>> + u8 data[8]; /* Data Bytes */
> >>>>> + u8 tsh; /* Time Stamp Higher Byte */
> >>>>> + u8 tsl; /* Time Stamp Lower Byte */
> >>>>> +} __packed;
> >>>>
> >>>> Sorry, I missed the earlier discussion, but why the _packed?
> >>>> One u32 and 12 bytes makes a nice multiple of 4.
> >>>
> >>> Better safe than sorry, it's the layout of the registers in hardware,
> >>> don't let the compiler optimize here.
Why not just add a compile time check against the size of the
structure - that will ensure that no padding is accidentally added.
> >> Actually __packed makes it less safe, as the compiler now assumes
> >> the u32 id member is unaligned, and thus may turn 32-bit accesses into 4
> >> byte accesses.
> >>
> >> Fortunately it won't happen in this case as the code uses writel/readl to
> >> acces the id member.
Which means that it will be aligned (and must be aligned).
So the packed is completely useless and pointless.
> > Yes, as this are registers they must not be accessed directly. However
> > we can use "__attribute__ ((packed, aligned(4)))" to tell the compiler
> > that the base address of this struct is always aligned to 4 bytes.
>
> I thought we'd gotten past the stage of ever writing register
> definitions as structures?
Any other way is likely to be more error prone since nothing
ties the offsets with the correct base address.
IMHO it is best to only specify packed if the structure is expected
to be misaligned. Misaligned members can be 'fixed' by marking them
aligned().
Even then you might want to define the structure itself without
the 'packed' and the define a second packed structure containing
a single member.
eg: you might have:
struct a {
u32 a_1;
u64 a_2 __attribute__((aligned(4)));
u32 a_3;
};
struct a_packed {
struct a p;
} __attribute__((packed));
You might even need a union of the two types!
David
next prev parent reply other threads:[~2014-01-20 12:13 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-26 20:37 [PATCH v5] can: add Renesas R-Car CAN driver Sergei Shtylyov
2013-12-26 21:37 ` Sergei Shtylyov
2014-01-13 13:46 ` Sergei Shtylyov
2014-01-13 13:46 ` Sergei Shtylyov
2014-01-20 9:18 ` Marc Kleine-Budde
2014-01-20 9:18 ` Marc Kleine-Budde
2014-01-25 0:34 ` Sergei Shtylyov
2014-01-25 1:34 ` Sergei Shtylyov
2014-02-13 12:12 ` Marc Kleine-Budde
2014-02-13 12:12 ` Marc Kleine-Budde
2014-02-20 22:48 ` Sergei Shtylyov
2014-02-20 23:48 ` Sergei Shtylyov
2014-02-28 9:08 ` Marc Kleine-Budde
2014-02-28 9:08 ` Marc Kleine-Budde
2014-02-28 11:16 ` Sergei Shtylyov
2014-02-28 11:16 ` Sergei Shtylyov
2014-02-28 11:37 ` Marc Kleine-Budde
2014-02-28 11:37 ` Marc Kleine-Budde
2014-02-28 11:41 ` Geert Uytterhoeven
2014-02-28 11:41 ` Geert Uytterhoeven
2014-02-28 11:47 ` David Laight
2014-02-28 11:47 ` David Laight
2014-02-28 11:50 ` Marc Kleine-Budde
2014-02-28 11:50 ` Marc Kleine-Budde
2014-02-28 12:02 ` David Laight
2014-02-28 12:02 ` David Laight
2014-02-28 11:49 ` Marc Kleine-Budde
2014-02-28 11:49 ` Marc Kleine-Budde
2014-02-28 12:05 ` Sergei Shtylyov
2014-02-28 12:05 ` Sergei Shtylyov
2014-02-28 12:17 ` David Laight
2014-02-28 12:17 ` David Laight
2014-02-28 12:34 ` Sergei Shtylyov
2014-02-28 12:34 ` Sergei Shtylyov
2014-01-20 11:43 ` Geert Uytterhoeven
2014-01-20 11:43 ` Geert Uytterhoeven
2014-01-20 11:47 ` Marc Kleine-Budde
2014-01-20 11:47 ` Marc Kleine-Budde
2014-01-20 11:52 ` Geert Uytterhoeven
2014-01-20 11:52 ` Geert Uytterhoeven
2014-01-20 11:58 ` Marc Kleine-Budde
2014-01-20 11:58 ` Marc Kleine-Budde
2014-01-20 12:02 ` Ben Dooks
2014-01-20 12:05 ` Geert Uytterhoeven
2014-01-20 12:05 ` Geert Uytterhoeven
2014-01-20 12:08 ` Marc Kleine-Budde
2014-01-20 12:08 ` Marc Kleine-Budde
2014-01-20 12:05 ` Marc Kleine-Budde
2014-01-20 12:05 ` Marc Kleine-Budde
2014-01-20 12:13 ` David Laight [this message]
2014-01-20 12:13 ` David Laight
2014-01-20 12:35 ` Marc Kleine-Budde
2014-01-20 12:35 ` Marc Kleine-Budde
2014-01-20 19:16 ` David Miller
2014-01-20 19:16 ` David Miller
2014-01-20 21:12 ` Sergei Shtylyov
2014-01-20 22:12 ` Sergei Shtylyov
2014-01-20 21:17 ` Marc Kleine-Budde
2014-01-20 21:17 ` Marc Kleine-Budde
2014-01-22 11:52 ` Ben Dooks
2014-01-22 11:54 ` Geert Uytterhoeven
2014-01-22 11:54 ` Geert Uytterhoeven
2014-01-22 11:58 ` David Laight
2014-01-22 11:58 ` David Laight
2014-01-20 12:12 ` Sergei Shtylyov
2014-01-20 12:12 ` Sergei Shtylyov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=063D6719AE5E284EB5DD2968C1650D6D460082@AcuExch.aculab.com \
--to=david.laight@aculab.com \
--cc=ben.dooks@codethink.co.uk \
--cc=geert@linux-m68k.org \
--cc=linux-can@vger.kernel.org \
--cc=linux-sh@vger.kernel.org \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=sergei.shtylyov@cogentembedded.com \
--cc=vksavl@gmail.com \
--cc=wg@grandegger.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.