From: 'Manivannan Sadhasivam' <manivannan.sadhasivam@linaro.org>
To: David Laight <David.Laight@ACULAB.COM>
Cc: "mhi@lists.linux.dev" <mhi@lists.linux.dev>,
"quic_hemantk@quicinc.com" <quic_hemantk@quicinc.com>,
"quic_bbhatt@quicinc.com" <quic_bbhatt@quicinc.com>,
"quic_jhugo@quicinc.com" <quic_jhugo@quicinc.com>,
"vinod.koul@linaro.org" <vinod.koul@linaro.org>,
"bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
"dmitry.baryshkov@linaro.org" <dmitry.baryshkov@linaro.org>,
"quic_vbadigan@quicinc.com" <quic_vbadigan@quicinc.com>,
"quic_cang@quicinc.com" <quic_cang@quicinc.com>,
"quic_skananth@quicinc.com" <quic_skananth@quicinc.com>,
"linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"elder@linaro.org" <elder@linaro.org>
Subject: Re: [PATCH v4 05/27] bus: mhi: Use bitfield operations for handling DWORDs of ring elements
Date: Mon, 28 Feb 2022 20:13:36 +0530 [thread overview]
Message-ID: <20220228144336.GB12451@workstation> (raw)
In-Reply-To: <90b1d3954b8c4157a4045db82b562271@AcuMS.aculab.com>
On Mon, Feb 28, 2022 at 02:00:07PM +0000, David Laight wrote:
> From: Manivannan Sadhasivam
> > Sent: 28 February 2022 12:43
> >
> > Instead of using the hardcoded bits in DWORD definitions, let's use the
> > bitfield operations to make it more clear how the DWORDs are structured.
>
> That all makes it as clear as mud.
It depends on how you see it ;)
For instance,
#define MHI_TRE_GET_CMD_TYPE(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 16) & 0xFF)
vs
#define MHI_TRE_GET_CMD_TYPE(tre) (FIELD_GET(GENMASK(23, 16), (MHI_TRE_GET_DWORD(tre, 1))))
The later one makes it more obvious that the "type" field resides between bit 23
and 16. Plus it avoids the extra masking.
> Try reading it!
>
Well I did before sending the patch.
Thanks,
Mani
> David
>
> >
> > Suggested-by: Alex Elder <elder@linaro.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> > drivers/bus/mhi/host/internal.h | 58 +++++++++++++++++++--------------
> > 1 file changed, 33 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
> > index 156bf65b6810..1d1790e83a93 100644
> > --- a/drivers/bus/mhi/host/internal.h
> > +++ b/drivers/bus/mhi/host/internal.h
> > @@ -7,6 +7,7 @@
> > #ifndef _MHI_INT_H
> > #define _MHI_INT_H
> >
> > +#include <linux/bitfield.h>
> > #include <linux/mhi.h>
> >
> > extern struct bus_type mhi_bus_type;
> > @@ -205,58 +206,65 @@ enum mhi_cmd_type {
> > /* No operation command */
> > #define MHI_TRE_CMD_NOOP_PTR (0)
> > #define MHI_TRE_CMD_NOOP_DWORD0 (0)
> > -#define MHI_TRE_CMD_NOOP_DWORD1 (cpu_to_le32(MHI_CMD_NOP << 16))
> > +#define MHI_TRE_CMD_NOOP_DWORD1 (cpu_to_le32(FIELD_PREP(GENMASK(23, 16), MHI_CMD_NOP)))
> >
> > /* Channel reset command */
> > #define MHI_TRE_CMD_RESET_PTR (0)
> > #define MHI_TRE_CMD_RESET_DWORD0 (0)
> > -#define MHI_TRE_CMD_RESET_DWORD1(chid) (cpu_to_le32((chid << 24) | \
> > - (MHI_CMD_RESET_CHAN << 16)))
> > +#define MHI_TRE_CMD_RESET_DWORD1(chid) (cpu_to_le32(FIELD_PREP(GENMASK(31, 24), chid)) | \
> > + FIELD_PREP(GENMASK(23, 16), MHI_CMD_RESET_CHAN))
> >
> > /* Channel stop command */
> > #define MHI_TRE_CMD_STOP_PTR (0)
> > #define MHI_TRE_CMD_STOP_DWORD0 (0)
> > -#define MHI_TRE_CMD_STOP_DWORD1(chid) (cpu_to_le32((chid << 24) | \
> > - (MHI_CMD_STOP_CHAN << 16)))
> > +#define MHI_TRE_CMD_STOP_DWORD1(chid) (cpu_to_le32(FIELD_PREP(GENMASK(31, 24), chid)) | \
> > + FIELD_PREP(GENMASK(23, 16), MHI_CMD_STOP_CHAN))
> >
> > /* Channel start command */
> > #define MHI_TRE_CMD_START_PTR (0)
> > #define MHI_TRE_CMD_START_DWORD0 (0)
> > -#define MHI_TRE_CMD_START_DWORD1(chid) (cpu_to_le32((chid << 24) | \
> > - (MHI_CMD_START_CHAN << 16)))
> > +#define MHI_TRE_CMD_START_DWORD1(chid) (cpu_to_le32(FIELD_PREP(GENMASK(31, 24), chid)) | \
> > + FIELD_PREP(GENMASK(23, 16), MHI_CMD_START_CHAN))
> >
> > #define MHI_TRE_GET_DWORD(tre, word) (le32_to_cpu((tre)->dword[(word)]))
> > -#define MHI_TRE_GET_CMD_CHID(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 24) & 0xFF)
> > -#define MHI_TRE_GET_CMD_TYPE(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 16) & 0xFF)
> > +#define MHI_TRE_GET_CMD_CHID(tre) (FIELD_GET(GENMASK(31, 24), (MHI_TRE_GET_DWORD(tre, 1))))
> > +#define MHI_TRE_GET_CMD_TYPE(tre) (FIELD_GET(GENMASK(23, 16), (MHI_TRE_GET_DWORD(tre, 1))))
> >
> > /* Event descriptor macros */
> > #define MHI_TRE_EV_PTR(ptr) (cpu_to_le64(ptr))
> > -#define MHI_TRE_EV_DWORD0(code, len) (cpu_to_le32((code << 24) | len))
> > -#define MHI_TRE_EV_DWORD1(chid, type) (cpu_to_le32((chid << 24) | (type << 16)))
> > +#define MHI_TRE_EV_DWORD0(code, len) (cpu_to_le32(FIELD_PREP(GENMASK(31, 24), code) | \
> > + FIELD_PREP(GENMASK(15, 0), len)))
> > +#define MHI_TRE_EV_DWORD1(chid, type) (cpu_to_le32(FIELD_PREP(GENMASK(31, 24), chid) | \
> > + FIELD_PREP(GENMASK(23, 16), type)))
> > #define MHI_TRE_GET_EV_PTR(tre) (le64_to_cpu((tre)->ptr))
> > -#define MHI_TRE_GET_EV_CODE(tre) ((MHI_TRE_GET_DWORD(tre, 0) >> 24) & 0xFF)
> > -#define MHI_TRE_GET_EV_LEN(tre) (MHI_TRE_GET_DWORD(tre, 0) & 0xFFFF)
> > -#define MHI_TRE_GET_EV_CHID(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 24) & 0xFF)
> > -#define MHI_TRE_GET_EV_TYPE(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 16) & 0xFF)
> > -#define MHI_TRE_GET_EV_STATE(tre) ((MHI_TRE_GET_DWORD(tre, 0) >> 24) & 0xFF)
> > -#define MHI_TRE_GET_EV_EXECENV(tre) ((MHI_TRE_GET_DWORD(tre, 0) >> 24) & 0xFF)
> > +#define MHI_TRE_GET_EV_CODE(tre) (FIELD_GET(GENMASK(31, 24), (MHI_TRE_GET_DWORD(tre, 0))))
> > +#define MHI_TRE_GET_EV_LEN(tre) (FIELD_GET(GENMASK(15, 0), (MHI_TRE_GET_DWORD(tre, 0))))
> > +#define MHI_TRE_GET_EV_CHID(tre) (FIELD_GET(GENMASK(31, 24), (MHI_TRE_GET_DWORD(tre, 1))))
> > +#define MHI_TRE_GET_EV_TYPE(tre) (FIELD_GET(GENMASK(23, 16), (MHI_TRE_GET_DWORD(tre, 1))))
> > +#define MHI_TRE_GET_EV_STATE(tre) (FIELD_GET(GENMASK(31, 24), (MHI_TRE_GET_DWORD(tre, 0))))
> > +#define MHI_TRE_GET_EV_EXECENV(tre) (FIELD_GET(GENMASK(31, 24), (MHI_TRE_GET_DWORD(tre, 0))))
> > #define MHI_TRE_GET_EV_SEQ(tre) MHI_TRE_GET_DWORD(tre, 0)
> > #define MHI_TRE_GET_EV_TIME(tre) (MHI_TRE_GET_EV_PTR(tre))
> > #define MHI_TRE_GET_EV_COOKIE(tre) lower_32_bits(MHI_TRE_GET_EV_PTR(tre))
> > -#define MHI_TRE_GET_EV_VEID(tre) ((MHI_TRE_GET_DWORD(tre, 0) >> 16) & 0xFF)
> > -#define MHI_TRE_GET_EV_LINKSPEED(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 24) & 0xFF)
> > -#define MHI_TRE_GET_EV_LINKWIDTH(tre) (MHI_TRE_GET_DWORD(tre, 0) & 0xFF)
> > +#define MHI_TRE_GET_EV_VEID(tre) (FIELD_GET(GENMASK(23, 16), (MHI_TRE_GET_DWORD(tre, 0))))
> > +#define MHI_TRE_GET_EV_LINKSPEED(tre) (FIELD_GET(GENMASK(31, 24), (MHI_TRE_GET_DWORD(tre, 1))))
> > +#define MHI_TRE_GET_EV_LINKWIDTH(tre) (FIELD_GET(GENMASK(7, 0), (MHI_TRE_GET_DWORD(tre, 0))))
> >
> > /* Transfer descriptor macros */
> > #define MHI_TRE_DATA_PTR(ptr) (cpu_to_le64(ptr))
> > -#define MHI_TRE_DATA_DWORD0(len) (cpu_to_le32(len & MHI_MAX_MTU))
> > -#define MHI_TRE_DATA_DWORD1(bei, ieot, ieob, chain) (cpu_to_le32((2 << 16) | (bei << 10) \
> > - | (ieot << 9) | (ieob << 8) | chain))
> > +#define MHI_TRE_DATA_DWORD0(len) (cpu_to_le32(FIELD_PREP(GENMASK(15, 0), len)))
> > +#define MHI_TRE_TYPE_TRANSFER 2
> > +#define MHI_TRE_DATA_DWORD1(bei, ieot, ieob, chain) (cpu_to_le32(FIELD_PREP(GENMASK(23, 16), \
> > + MHI_TRE_TYPE_TRANSFER) | \
> > + FIELD_PREP(BIT(10), bei) | \
> > + FIELD_PREP(BIT(9), ieot) | \
> > + FIELD_PREP(BIT(8), ieob) | \
> > + FIELD_PREP(BIT(0), chain)))
> >
> > /* RSC transfer descriptor macros */
> > -#define MHI_RSCTRE_DATA_PTR(ptr, len) (cpu_to_le64(((u64)len << 48) | ptr))
> > +#define MHI_RSCTRE_DATA_PTR(ptr, len) (cpu_to_le64(FIELD_PREP(GENMASK(64, 48), len) | ptr))
> > #define MHI_RSCTRE_DATA_DWORD0(cookie) (cpu_to_le32(cookie))
> > -#define MHI_RSCTRE_DATA_DWORD1 (cpu_to_le32(MHI_PKT_TYPE_COALESCING << 16))
> > +#define MHI_RSCTRE_DATA_DWORD1 (cpu_to_le32(FIELD_PREP(GENMASK(23, 16), MHI_PKT_TYPE_COALESCING)
> >
> > enum mhi_pkt_type {
> > MHI_PKT_TYPE_INVALID = 0x0,
> > --
> > 2.25.1
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
next prev parent reply other threads:[~2022-02-28 14:43 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-28 12:43 [PATCH v4 00/27] Add initial support for MHI endpoint stack Manivannan Sadhasivam
2022-02-28 12:43 ` [PATCH v4 01/27] bus: mhi: Fix pm_state conversion to string Manivannan Sadhasivam
2022-02-28 15:30 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 02/27] bus: mhi: Fix MHI DMA structure endianness Manivannan Sadhasivam
2022-02-28 15:40 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 03/27] bus: mhi: Move host MHI code to "host" directory Manivannan Sadhasivam
2022-02-28 12:43 ` [PATCH v4 04/27] bus: mhi: Use bitfield operations for register read and write Manivannan Sadhasivam
2022-02-28 12:43 ` [PATCH v4 05/27] bus: mhi: Use bitfield operations for handling DWORDs of ring elements Manivannan Sadhasivam
2022-02-28 14:00 ` David Laight
2022-02-28 14:43 ` 'Manivannan Sadhasivam' [this message]
2022-02-28 15:11 ` Alex Elder
2022-02-28 15:40 ` David Laight
2022-02-28 15:51 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 06/27] bus: mhi: Cleanup the register definitions used in headers Manivannan Sadhasivam
2022-02-28 12:43 ` [PATCH v4 07/27] bus: mhi: host: Rename "struct mhi_tre" to "struct mhi_ring_element" Manivannan Sadhasivam
2022-02-28 15:52 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 08/27] bus: mhi: Move common MHI definitions out of host directory Manivannan Sadhasivam
2022-02-28 12:43 ` [PATCH v4 09/27] bus: mhi: Make mhi_state_str[] array static inline and move to common.h Manivannan Sadhasivam
2022-02-28 15:56 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 10/27] bus: mhi: ep: Add support for registering MHI endpoint controllers Manivannan Sadhasivam
2022-02-28 16:06 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 11/27] bus: mhi: ep: Add support for registering MHI endpoint client drivers Manivannan Sadhasivam
2022-02-28 16:09 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 12/27] bus: mhi: ep: Add support for creating and destroying MHI EP devices Manivannan Sadhasivam
2022-02-28 16:10 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 13/27] bus: mhi: ep: Add support for managing MMIO registers Manivannan Sadhasivam
2022-02-28 16:23 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 14/27] bus: mhi: ep: Add support for ring management Manivannan Sadhasivam
2022-02-28 16:27 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 15/27] bus: mhi: ep: Add support for sending events to the host Manivannan Sadhasivam
2022-02-28 16:37 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 16/27] bus: mhi: ep: Add support for managing MHI state machine Manivannan Sadhasivam
2022-02-28 16:41 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 17/27] bus: mhi: ep: Add support for processing MHI endpoint interrupts Manivannan Sadhasivam
2022-02-28 16:45 ` Alex Elder
2022-03-01 6:41 ` Manivannan Sadhasivam
2022-02-28 12:43 ` [PATCH v4 18/27] bus: mhi: ep: Add support for powering up the MHI endpoint stack Manivannan Sadhasivam
2022-02-28 16:47 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 19/27] bus: mhi: ep: Add support for powering down " Manivannan Sadhasivam
2022-02-28 16:49 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 20/27] bus: mhi: ep: Add support for handling MHI_RESET Manivannan Sadhasivam
2022-02-28 12:43 ` [PATCH v4 21/27] bus: mhi: ep: Add support for handling SYS_ERR condition Manivannan Sadhasivam
2022-02-28 12:43 ` [PATCH v4 22/27] bus: mhi: ep: Add support for processing command rings Manivannan Sadhasivam
2022-02-28 12:43 ` [PATCH v4 23/27] bus: mhi: ep: Add support for reading from the host Manivannan Sadhasivam
2022-02-28 12:43 ` [PATCH v4 24/27] bus: mhi: ep: Add support for processing channel rings Manivannan Sadhasivam
2022-02-28 12:43 ` [PATCH v4 25/27] bus: mhi: ep: Add support for queueing SKBs to the host Manivannan Sadhasivam
2022-02-28 16:51 ` Alex Elder
2022-02-28 12:43 ` [PATCH v4 26/27] bus: mhi: ep: Add support for suspending and resuming channels Manivannan Sadhasivam
2022-02-28 12:43 ` [PATCH v4 27/27] bus: mhi: ep: Add uevent support for module autoloading Manivannan Sadhasivam
2022-02-28 16:57 ` [PATCH v4 00/27] Add initial support for MHI endpoint stack Alex Elder
2022-03-01 6:15 ` Manivannan Sadhasivam
2022-03-01 8:50 ` Manivannan Sadhasivam
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=20220228144336.GB12451@workstation \
--to=manivannan.sadhasivam@linaro.org \
--cc=David.Laight@ACULAB.COM \
--cc=bjorn.andersson@linaro.org \
--cc=dmitry.baryshkov@linaro.org \
--cc=elder@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mhi@lists.linux.dev \
--cc=quic_bbhatt@quicinc.com \
--cc=quic_cang@quicinc.com \
--cc=quic_hemantk@quicinc.com \
--cc=quic_jhugo@quicinc.com \
--cc=quic_skananth@quicinc.com \
--cc=quic_vbadigan@quicinc.com \
--cc=vinod.koul@linaro.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox