From: Scott Wood <scottwood@freescale.com>
To: Timur Tabi <timur@freescale.com>
Cc: Varun Sethi <Varun.Sethi@freescale.com>,
joerg.roedel@amd.com, iommu@lists.linux-foundation.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4 v4] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
Date: Mon, 5 Nov 2012 17:11:15 -0600 [thread overview]
Message-ID: <1352157075.28279.10@snotra> (raw)
In-Reply-To: <509845ED.2060904@freescale.com> (from timur@freescale.com on Mon Nov 5 17:04:13 2012)
On 11/05/2012 05:04:13 PM, Timur Tabi wrote:
> Varun Sethi wrote:
> > + /* PAACE Offset 0x00 */
> > + u32 wbah; /* only valid for
> Primary PAACE */
> > + u32 addr_bitfields; /* See P/S PAACE_AF_* */
> > +
> > + /* PAACE Offset 0x08 */
> > + /* Interpretation of first 32 bits dependent on DD above */
> > + union {
> > + struct {
> > + /* Destination ID, see PAACE_DID_* defines */
> > + u8 did;
> > + /* Partition ID */
> > + u8 pid;
> > + /* Snoop ID */
> > + u8 snpid;
> > + /* coherency_required : 1 reserved : 7 */
>
> Please use this format, which is easier to read:
>
> /* 1 == coherency required, 7 == reserved */
>
> Every time I look at this comment, I think you are using bitfields.
It is meant as a pseudo-bitfield. "7 == reserved" doesn't make much
sense -- that would leave a lot of other values neither defined nor
explicitly reserved.
That said, the "See PAACE_DA_*" comment should be sufficient and avoids
making people have to care about what bitfield ordering the comment
writer was assuming.
-Scott
WARNING: multiple messages have this Message-ID (diff)
From: Scott Wood <scottwood@freescale.com>
To: Timur Tabi <timur@freescale.com>
Cc: Varun Sethi <Varun.Sethi@freescale.com>,
iommu@lists.linux-foundation.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, joerg.roedel@amd.com
Subject: Re: [PATCH 4/4 v4] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
Date: Mon, 5 Nov 2012 17:11:15 -0600 [thread overview]
Message-ID: <1352157075.28279.10@snotra> (raw)
In-Reply-To: <509845ED.2060904@freescale.com> (from timur@freescale.com on Mon Nov 5 17:04:13 2012)
On 11/05/2012 05:04:13 PM, Timur Tabi wrote:
> Varun Sethi wrote:
> > + /* PAACE Offset 0x00 */
> > + u32 wbah; /* only valid for =20
> Primary PAACE */
> > + u32 addr_bitfields; /* See P/S PAACE_AF_* */
> > +
> > + /* PAACE Offset 0x08 */
> > + /* Interpretation of first 32 bits dependent on DD above */
> > + union {
> > + struct {
> > + /* Destination ID, see PAACE_DID_* defines */
> > + u8 did;
> > + /* Partition ID */
> > + u8 pid;
> > + /* Snoop ID */
> > + u8 snpid;
> > + /* coherency_required : 1 reserved : 7 */
>=20
> Please use this format, which is easier to read:
>=20
> /* 1 =3D=3D coherency required, 7 =3D=3D reserved */
>=20
> Every time I look at this comment, I think you are using bitfields.
It is meant as a pseudo-bitfield. "7 =3D=3D reserved" doesn't make much =20
sense -- that would leave a lot of other values neither defined nor =20
explicitly reserved.
That said, the "See PAACE_DA_*" comment should be sufficient and avoids =20
making people have to care about what bitfield ordering the comment =20
writer was assuming.
-Scott=
WARNING: multiple messages have this Message-ID (diff)
From: Scott Wood <scottwood@freescale.com>
To: Timur Tabi <timur@freescale.com>
Cc: Varun Sethi <Varun.Sethi@freescale.com>, <joerg.roedel@amd.com>,
<iommu@lists.linux-foundation.org>,
<linuxppc-dev@lists.ozlabs.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/4 v4] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
Date: Mon, 5 Nov 2012 17:11:15 -0600 [thread overview]
Message-ID: <1352157075.28279.10@snotra> (raw)
In-Reply-To: <509845ED.2060904@freescale.com> (from timur@freescale.com on Mon Nov 5 17:04:13 2012)
On 11/05/2012 05:04:13 PM, Timur Tabi wrote:
> Varun Sethi wrote:
> > + /* PAACE Offset 0x00 */
> > + u32 wbah; /* only valid for
> Primary PAACE */
> > + u32 addr_bitfields; /* See P/S PAACE_AF_* */
> > +
> > + /* PAACE Offset 0x08 */
> > + /* Interpretation of first 32 bits dependent on DD above */
> > + union {
> > + struct {
> > + /* Destination ID, see PAACE_DID_* defines */
> > + u8 did;
> > + /* Partition ID */
> > + u8 pid;
> > + /* Snoop ID */
> > + u8 snpid;
> > + /* coherency_required : 1 reserved : 7 */
>
> Please use this format, which is easier to read:
>
> /* 1 == coherency required, 7 == reserved */
>
> Every time I look at this comment, I think you are using bitfields.
It is meant as a pseudo-bitfield. "7 == reserved" doesn't make much
sense -- that would leave a lot of other values neither defined nor
explicitly reserved.
That said, the "See PAACE_DA_*" comment should be sufficient and avoids
making people have to care about what bitfield ordering the comment
writer was assuming.
-Scott
next prev parent reply other threads:[~2012-11-05 23:11 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-05 11:19 [PATCH 0/4] iommu/fsl: Freescale PAMU driver and IOMMU API implementation Varun Sethi
2012-11-05 11:19 ` Varun Sethi
2012-11-05 11:19 ` [PATCH 1/4 v2] iommu/fsl: Store iommu domain information pointer in archdata Varun Sethi
2012-11-05 11:19 ` Varun Sethi
2012-11-05 11:19 ` [PATCH 3/4 v4] iommu/fsl: Add iommu domain attributes required by fsl PAMU driver Varun Sethi
2012-11-05 11:19 ` Varun Sethi
2012-11-05 11:19 ` [PATCH 4/4 v4] iommu/fsl: Freescale PAMU driver and IOMMU API implementation Varun Sethi
2012-11-05 11:19 ` Varun Sethi
[not found] ` <1352114361-25192-4-git-send-email-Varun.Sethi-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
2012-11-05 23:04 ` Timur Tabi
2012-11-05 23:04 ` Timur Tabi
2012-11-05 23:04 ` Timur Tabi
2012-11-05 23:11 ` Scott Wood [this message]
2012-11-05 23:11 ` Scott Wood
2012-11-05 23:11 ` Scott Wood
[not found] ` <1352114361-25192-3-git-send-email-Varun.Sethi-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
2012-11-05 22:10 ` [PATCH 3/4 v4] iommu/fsl: Add iommu domain attributes required by fsl PAMU driver Timur Tabi
2012-11-05 22:10 ` Timur Tabi
2012-11-05 22:10 ` Timur Tabi
2012-11-05 22:21 ` Scott Wood
2012-11-05 22:21 ` Scott Wood
2012-11-05 22:21 ` Scott Wood
2012-11-05 23:56 ` Scott Wood
2012-11-05 23:56 ` Scott Wood
2012-11-05 23:56 ` Scott Wood
2012-11-05 22:05 ` [PATCH 0/4] iommu/fsl: Freescale PAMU driver and IOMMU API implementation Timur Tabi
2012-11-05 22:05 ` Timur Tabi
2012-11-05 22:05 ` Timur Tabi
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=1352157075.28279.10@snotra \
--to=scottwood@freescale.com \
--cc=Varun.Sethi@freescale.com \
--cc=iommu@lists.linux-foundation.org \
--cc=joerg.roedel@amd.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=timur@freescale.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.