| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220216164543.GD6225@workstation>
Date: Wed, 16 Feb 2022 22:15:43 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Alex Elder <elder@...aro.org>
Cc: mhi@...ts.linux.dev, quic_hemantk@...cinc.com,
quic_bbhatt@...cinc.com, quic_jhugo@...cinc.com,
vinod.koul@...aro.org, bjorn.andersson@...aro.org,
dmitry.baryshkov@...aro.org, quic_vbadigan@...cinc.com,
quic_cang@...cinc.com, quic_skananth@...cinc.com,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 07/25] bus: mhi: Get rid of SHIFT macros and use
bitfield operations
On Tue, Feb 15, 2022 at 02:02:34PM -0600, Alex Elder wrote:
> On 2/12/22 12:20 PM, Manivannan Sadhasivam wrote:
> > Instead of using the hardcoded SHIFT values, use the bitfield macros to
> > derive the shift value from mask during build time.
>
> You accomplished this by changing the way mhi_read_reg_field(),
> mhi_poll_reg_field(), and mhi_write_reg_field() are defined.
> It would be helpful for you to point out that fact up front.
> Then it's fairly clear that the _SHIFT (and _SHFT) definitions
> can just go away. Very nice to remove those though.
>
> > For shift values that cannot be determined during build time, "__ffs()"
> > helper is used find the shift value in runtime.
>
> Yeah this is an annoying feature of the bitfield functions,
> but you *know* when you're working with a variable mask.
>
> I still think the mask values that are 32 bits wide are
> overkill, e.g.:
>
> #define MHIREGLEN_MHIREGLEN_MASK GENMASK(31, 0)
>
>
> Thise are full 32-bit registers, and I don't see any reason
> they would ever *not* be full registers, so there's no point
> in applying a mask to them. Even if some day it did make
> sense to use a mask (less than 32 bits wide, for example),
> that's something that could be added when that becomes an
> issue, rather than complicating the code unnecessarily now.
>
Okay. Got rid of 32bit masks and modified the commit message.
Thanks,
Mani
> If you eliminate the 32-bit wide masks, great, but even if
> you don't:
>
> Reviewed-by: Alex Elder <elder@...aro.org>
>
> > Suggested-by: Alex Elder <elder@...aro.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > ---
> > drivers/bus/mhi/common.h | 45 ----------------------
> > drivers/bus/mhi/host/boot.c | 15 ++------
> > drivers/bus/mhi/host/debugfs.c | 10 ++---
> > drivers/bus/mhi/host/init.c | 67 +++++++++++++++------------------
> > drivers/bus/mhi/host/internal.h | 10 ++---
> > drivers/bus/mhi/host/main.c | 16 ++++----
> > drivers/bus/mhi/host/pm.c | 18 +++------
> > 7 files changed, 55 insertions(+), 126 deletions(-)
> >
> > diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
> > index f226f06d4ff9..728c82928d8d 100644
> > --- a/drivers/bus/mhi/common.h
> > +++ b/drivers/bus/mhi/common.h
> > @@ -63,9 +63,7 @@
> > /* BHI register bits */
> > #define BHI_TXDB_SEQNUM_BMSK GENMASK(29, 0)
> > -#define BHI_TXDB_SEQNUM_SHFT 0
> > #define BHI_STATUS_MASK GENMASK(31, 30)
> > -#define BHI_STATUS_SHIFT 30
> > #define BHI_STATUS_ERROR 0x03
> > #define BHI_STATUS_SUCCESS 0x02
> > #define BHI_STATUS_RESET 0x00
> > @@ -85,89 +83,51 @@
> > /* BHIE register bits */
> > #define BHIE_TXVECDB_SEQNUM_BMSK GENMASK(29, 0)
> > -#define BHIE_TXVECDB_SEQNUM_SHFT 0
> > #define BHIE_TXVECSTATUS_SEQNUM_BMSK GENMASK(29, 0)
> > -#define BHIE_TXVECSTATUS_SEQNUM_SHFT 0
> > #define BHIE_TXVECSTATUS_STATUS_BMSK GENMASK(31, 30)
> > -#define BHIE_TXVECSTATUS_STATUS_SHFT 30
> > #define BHIE_TXVECSTATUS_STATUS_RESET 0x00
> > #define BHIE_TXVECSTATUS_STATUS_XFER_COMPL 0x02
> > #define BHIE_TXVECSTATUS_STATUS_ERROR 0x03
> > #define BHIE_RXVECDB_SEQNUM_BMSK GENMASK(29, 0)
> > -#define BHIE_RXVECDB_SEQNUM_SHFT 0
> > #define BHIE_RXVECSTATUS_SEQNUM_BMSK GENMASK(29, 0)
> > -#define BHIE_RXVECSTATUS_SEQNUM_SHFT 0
> > #define BHIE_RXVECSTATUS_STATUS_BMSK GENMASK(31, 30)
> > -#define BHIE_RXVECSTATUS_STATUS_SHFT 30
> > #define BHIE_RXVECSTATUS_STATUS_RESET 0x00
> > #define BHIE_RXVECSTATUS_STATUS_XFER_COMPL 0x02
> > #define BHIE_RXVECSTATUS_STATUS_ERROR 0x03
> > /* MHI register bits */
> > #define MHIREGLEN_MHIREGLEN_MASK GENMASK(31, 0)
> > -#define MHIREGLEN_MHIREGLEN_SHIFT 0
> > #define MHIVER_MHIVER_MASK GENMASK(31, 0)
> > -#define MHIVER_MHIVER_SHIFT 0
> > #define MHICFG_NHWER_MASK GENMASK(31, 24)
> > -#define MHICFG_NHWER_SHIFT 24
> > #define MHICFG_NER_MASK GENMASK(23, 16)
> > -#define MHICFG_NER_SHIFT 16
> > #define MHICFG_NHWCH_MASK GENMASK(15, 8)
> > -#define MHICFG_NHWCH_SHIFT 8
> > #define MHICFG_NCH_MASK GENMASK(7, 0)
> > -#define MHICFG_NCH_SHIFT 0
> > #define CHDBOFF_CHDBOFF_MASK GENMASK(31, 0)
> > -#define CHDBOFF_CHDBOFF_SHIFT 0
> > #define ERDBOFF_ERDBOFF_MASK GENMASK(31, 0)
> > -#define ERDBOFF_ERDBOFF_SHIFT 0
> > #define BHIOFF_BHIOFF_MASK GENMASK(31, 0)
> > -#define BHIOFF_BHIOFF_SHIFT 0
> > #define BHIEOFF_BHIEOFF_MASK GENMASK(31, 0)
> > -#define BHIEOFF_BHIEOFF_SHIFT 0
> > #define DEBUGOFF_DEBUGOFF_MASK GENMASK(31, 0)
> > -#define DEBUGOFF_DEBUGOFF_SHIFT 0
> > #define MHICTRL_MHISTATE_MASK GENMASK(15, 8)
> > -#define MHICTRL_MHISTATE_SHIFT 8
> > #define MHICTRL_RESET_MASK BIT(1)
> > -#define MHICTRL_RESET_SHIFT 1
> > #define MHISTATUS_MHISTATE_MASK GENMASK(15, 8)
> > -#define MHISTATUS_MHISTATE_SHIFT 8
> > #define MHISTATUS_SYSERR_MASK BIT(2)
> > -#define MHISTATUS_SYSERR_SHIFT 2
> > #define MHISTATUS_READY_MASK BIT(0)
> > -#define MHISTATUS_READY_SHIFT 0
> > #define CCABAP_LOWER_CCABAP_LOWER_MASK GENMASK(31, 0)
> > -#define CCABAP_LOWER_CCABAP_LOWER_SHIFT 0
> > #define CCABAP_HIGHER_CCABAP_HIGHER_MASK GENMASK(31, 0)
> > -#define CCABAP_HIGHER_CCABAP_HIGHER_SHIFT 0
> > #define ECABAP_LOWER_ECABAP_LOWER_MASK GENMASK(31, 0)
> > -#define ECABAP_LOWER_ECABAP_LOWER_SHIFT 0
> > #define ECABAP_HIGHER_ECABAP_HIGHER_MASK GENMASK(31, 0)
> > -#define ECABAP_HIGHER_ECABAP_HIGHER_SHIFT 0
> > #define CRCBAP_LOWER_CRCBAP_LOWER_MASK GENMASK(31, 0)
> > -#define CRCBAP_LOWER_CRCBAP_LOWER_SHIFT 0
> > #define CRCBAP_HIGHER_CRCBAP_HIGHER_MASK GENMASK(31, 0)
> > -#define CRCBAP_HIGHER_CRCBAP_HIGHER_SHIFT 0
> > #define CRDB_LOWER_CRDB_LOWER_MASK GENMASK(31, 0)
> > -#define CRDB_LOWER_CRDB_LOWER_SHIFT 0
> > #define CRDB_HIGHER_CRDB_HIGHER_MASK GENMASK(31, 0)
> > -#define CRDB_HIGHER_CRDB_HIGHER_SHIFT 0
> > #define MHICTRLBASE_LOWER_MHICTRLBASE_LOWER_MASK GENMASK(31, 0)
> > -#define MHICTRLBASE_LOWER_MHICTRLBASE_LOWER_SHIFT 0
> > #define MHICTRLBASE_HIGHER_MHICTRLBASE_HIGHER_MASK GENMASK(31, 0)
> > -#define MHICTRLBASE_HIGHER_MHICTRLBASE_HIGHER_SHIFT 0
> > #define MHICTRLLIMIT_LOWER_MHICTRLLIMIT_LOWER_MASK GENMASK(31, 0)
> > -#define MHICTRLLIMIT_LOWER_MHICTRLLIMIT_LOWER_SHIFT 0
> > #define MHICTRLLIMIT_HIGHER_MHICTRLLIMIT_HIGHER_MASK GENMASK(31, 0)
> > -#define MHICTRLLIMIT_HIGHER_MHICTRLLIMIT_HIGHER_SHIFT 0
> > #define MHIDATABASE_LOWER_MHIDATABASE_LOWER_MASK GENMASK(31, 0)
> > -#define MHIDATABASE_LOWER_MHIDATABASE_LOWER_SHIFT 0
> > #define MHIDATABASE_HIGHER_MHIDATABASE_HIGHER_MASK GENMASK(31, 0)
> > -#define MHIDATABASE_HIGHER_MHIDATABASE_HIGHER_SHIFT 0
> > #define MHIDATALIMIT_LOWER_MHIDATALIMIT_LOWER_MASK GENMASK(31, 0)
> > -#define MHIDATALIMIT_LOWER_MHIDATALIMIT_LOWER_SHIFT 0
> > #define MHIDATALIMIT_HIGHER_MHIDATALIMIT_HIGHER_MASK GENMASK(31, 0)
> > -#define MHIDATALIMIT_HIGHER_MHIDATALIMIT_HIGHER_SHIFT 0
> > /* Command Ring Element macros */
> > /* No operation command */
> > @@ -277,9 +237,7 @@ enum mhi_cmd_type {
> > #define EV_CTX_RESERVED_MASK GENMASK(7, 0)
> > #define EV_CTX_INTMODC_MASK GENMASK(15, 8)
> > -#define EV_CTX_INTMODC_SHIFT 8
> > #define EV_CTX_INTMODT_MASK GENMASK(31, 16)
> > -#define EV_CTX_INTMODT_SHIFT 16
> > struct mhi_event_ctxt {
> > __le32 intmod;
> > __le32 ertype;
> > @@ -292,11 +250,8 @@ struct mhi_event_ctxt {
> > };
> > #define CHAN_CTX_CHSTATE_MASK GENMASK(7, 0)
> > -#define CHAN_CTX_CHSTATE_SHIFT 0
> > #define CHAN_CTX_BRSTMODE_MASK GENMASK(9, 8)
> > -#define CHAN_CTX_BRSTMODE_SHIFT 8
> > #define CHAN_CTX_POLLCFG_MASK GENMASK(15, 10)
> > -#define CHAN_CTX_POLLCFG_SHIFT 10
> > #define CHAN_CTX_RESERVED_MASK GENMASK(31, 16)
> > struct mhi_chan_ctxt {
> > __le32 chcfg;
> > diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
> > index 93cb705614c6..b0da7ca4519c 100644
> > --- a/drivers/bus/mhi/host/boot.c
> > +++ b/drivers/bus/mhi/host/boot.c
> > @@ -46,8 +46,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
> > sequence_id = MHI_RANDOM_U32_NONZERO(BHIE_RXVECSTATUS_SEQNUM_BMSK);
> > mhi_write_reg_field(mhi_cntrl, base, BHIE_RXVECDB_OFFS,
> > - BHIE_RXVECDB_SEQNUM_BMSK, BHIE_RXVECDB_SEQNUM_SHFT,
> > - sequence_id);
> > + BHIE_RXVECDB_SEQNUM_BMSK, sequence_id);
> > dev_dbg(dev, "Address: %p and len: 0x%zx sequence: %u\n",
> > &mhi_buf->dma_addr, mhi_buf->len, sequence_id);
> > @@ -127,9 +126,7 @@ static int __mhi_download_rddm_in_panic(struct mhi_controller *mhi_cntrl)
> > while (retry--) {
> > ret = mhi_read_reg_field(mhi_cntrl, base, BHIE_RXVECSTATUS_OFFS,
> > - BHIE_RXVECSTATUS_STATUS_BMSK,
> > - BHIE_RXVECSTATUS_STATUS_SHFT,
> > - &rx_status);
> > + BHIE_RXVECSTATUS_STATUS_BMSK, &rx_status);
> > if (ret)
> > return -EIO;
> > @@ -168,7 +165,6 @@ int mhi_download_rddm_image(struct mhi_controller *mhi_cntrl, bool in_panic)
> > mhi_read_reg_field(mhi_cntrl, base,
> > BHIE_RXVECSTATUS_OFFS,
> > BHIE_RXVECSTATUS_STATUS_BMSK,
> > - BHIE_RXVECSTATUS_STATUS_SHFT,
> > &rx_status) || rx_status,
> > msecs_to_jiffies(mhi_cntrl->timeout_ms));
> > @@ -203,8 +199,7 @@ static int mhi_fw_load_bhie(struct mhi_controller *mhi_cntrl,
> > mhi_write_reg(mhi_cntrl, base, BHIE_TXVECSIZE_OFFS, mhi_buf->len);
> > mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS,
> > - BHIE_TXVECDB_SEQNUM_BMSK, BHIE_TXVECDB_SEQNUM_SHFT,
> > - sequence_id);
> > + BHIE_TXVECDB_SEQNUM_BMSK, sequence_id);
> > read_unlock_bh(pm_lock);
> > /* Wait for the image download to complete */
> > @@ -213,7 +208,6 @@ static int mhi_fw_load_bhie(struct mhi_controller *mhi_cntrl,
> > mhi_read_reg_field(mhi_cntrl, base,
> > BHIE_TXVECSTATUS_OFFS,
> > BHIE_TXVECSTATUS_STATUS_BMSK,
> > - BHIE_TXVECSTATUS_STATUS_SHFT,
> > &tx_status) || tx_status,
> > msecs_to_jiffies(mhi_cntrl->timeout_ms));
> > if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state) ||
> > @@ -265,8 +259,7 @@ static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
> > ret = wait_event_timeout(mhi_cntrl->state_event,
> > MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state) ||
> > mhi_read_reg_field(mhi_cntrl, base, BHI_STATUS,
> > - BHI_STATUS_MASK, BHI_STATUS_SHIFT,
> > - &tx_status) || tx_status,
> > + BHI_STATUS_MASK, &tx_status) || tx_status,
> > msecs_to_jiffies(mhi_cntrl->timeout_ms));
> > if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state))
> > goto invalid_pm_state;
> > diff --git a/drivers/bus/mhi/host/debugfs.c b/drivers/bus/mhi/host/debugfs.c
> > index 399d0db1f1eb..cfec7811dfbb 100644
> > --- a/drivers/bus/mhi/host/debugfs.c
> > +++ b/drivers/bus/mhi/host/debugfs.c
> > @@ -61,9 +61,9 @@ static int mhi_debugfs_events_show(struct seq_file *m, void *d)
> > seq_printf(m, "Index: %d intmod count: %lu time: %lu",
> > i, (le32_to_cpu(er_ctxt->intmod) & EV_CTX_INTMODC_MASK) >>
> > - EV_CTX_INTMODC_SHIFT,
> > + __ffs(EV_CTX_INTMODC_MASK),
> > (le32_to_cpu(er_ctxt->intmod) & EV_CTX_INTMODT_MASK) >>
> > - EV_CTX_INTMODT_SHIFT);
> > + __ffs(EV_CTX_INTMODT_MASK));
> > seq_printf(m, " base: 0x%0llx len: 0x%llx", le64_to_cpu(er_ctxt->rbase),
> > le64_to_cpu(er_ctxt->rlen));
> > @@ -107,10 +107,10 @@ static int mhi_debugfs_channels_show(struct seq_file *m, void *d)
> > seq_printf(m,
> > "%s(%u) state: 0x%lx brstmode: 0x%lx pollcfg: 0x%lx",
> > mhi_chan->name, mhi_chan->chan, (le32_to_cpu(chan_ctxt->chcfg) &
> > - CHAN_CTX_CHSTATE_MASK) >> CHAN_CTX_CHSTATE_SHIFT,
> > + CHAN_CTX_CHSTATE_MASK) >> __ffs(CHAN_CTX_CHSTATE_MASK),
> > (le32_to_cpu(chan_ctxt->chcfg) & CHAN_CTX_BRSTMODE_MASK) >>
> > - CHAN_CTX_BRSTMODE_SHIFT, (le32_to_cpu(chan_ctxt->chcfg) &
> > - CHAN_CTX_POLLCFG_MASK) >> CHAN_CTX_POLLCFG_SHIFT);
> > + __ffs(CHAN_CTX_BRSTMODE_MASK), (le32_to_cpu(chan_ctxt->chcfg) &
> > + CHAN_CTX_POLLCFG_MASK) >> __ffs(CHAN_CTX_POLLCFG_MASK));
> > seq_printf(m, " type: 0x%x event ring: %u", le32_to_cpu(chan_ctxt->chtype),
> > le32_to_cpu(chan_ctxt->erindex));
> > diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> > index 0e301f3f305e..05e457d12446 100644
> > --- a/drivers/bus/mhi/host/init.c
> > +++ b/drivers/bus/mhi/host/init.c
> > @@ -4,6 +4,7 @@
> > *
> > */
> > +#include <linux/bitfield.h>
> > #include <linux/debugfs.h>
> > #include <linux/device.h>
> > #include <linux/dma-direction.h>
> > @@ -283,11 +284,11 @@ int mhi_init_dev_ctxt(struct mhi_controller *mhi_cntrl)
> > tmp = le32_to_cpu(chan_ctxt->chcfg);
> > tmp &= ~CHAN_CTX_CHSTATE_MASK;
> > - tmp |= (MHI_CH_STATE_DISABLED << CHAN_CTX_CHSTATE_SHIFT);
> > + tmp |= FIELD_PREP(CHAN_CTX_CHSTATE_MASK, MHI_CH_STATE_DISABLED);
> > tmp &= ~CHAN_CTX_BRSTMODE_MASK;
> > - tmp |= (mhi_chan->db_cfg.brstmode << CHAN_CTX_BRSTMODE_SHIFT);
> > + tmp |= FIELD_PREP(CHAN_CTX_BRSTMODE_MASK, mhi_chan->db_cfg.brstmode);
> > tmp &= ~CHAN_CTX_POLLCFG_MASK;
> > - tmp |= (mhi_chan->db_cfg.pollcfg << CHAN_CTX_POLLCFG_SHIFT);
> > + tmp |= FIELD_PREP(CHAN_CTX_POLLCFG_MASK, mhi_chan->db_cfg.pollcfg);
> > chan_ctxt->chcfg = cpu_to_le32(tmp);
> > chan_ctxt->chtype = cpu_to_le32(mhi_chan->type);
> > @@ -319,7 +320,7 @@ int mhi_init_dev_ctxt(struct mhi_controller *mhi_cntrl)
> > tmp = le32_to_cpu(er_ctxt->intmod);
> > tmp &= ~EV_CTX_INTMODC_MASK;
> > tmp &= ~EV_CTX_INTMODT_MASK;
> > - tmp |= (mhi_event->intmod << EV_CTX_INTMODT_SHIFT);
> > + tmp |= FIELD_PREP(EV_CTX_INTMODT_MASK, mhi_event->intmod);
> > er_ctxt->intmod = cpu_to_le32(tmp);
> > er_ctxt->ertype = cpu_to_le32(MHI_ER_TYPE_VALID);
> > @@ -425,71 +426,70 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
> > struct {
> > u32 offset;
> > u32 mask;
> > - u32 shift;
> > u32 val;
> > } reg_info[] = {
> > {
> > - CCABAP_HIGHER, U32_MAX, 0,
> > + CCABAP_HIGHER, U32_MAX,
> > upper_32_bits(mhi_cntrl->mhi_ctxt->chan_ctxt_addr),
> > },
> > {
> > - CCABAP_LOWER, U32_MAX, 0,
> > + CCABAP_LOWER, U32_MAX,
> > lower_32_bits(mhi_cntrl->mhi_ctxt->chan_ctxt_addr),
> > },
> > {
> > - ECABAP_HIGHER, U32_MAX, 0,
> > + ECABAP_HIGHER, U32_MAX,
> > upper_32_bits(mhi_cntrl->mhi_ctxt->er_ctxt_addr),
> > },
> > {
> > - ECABAP_LOWER, U32_MAX, 0,
> > + ECABAP_LOWER, U32_MAX,
> > lower_32_bits(mhi_cntrl->mhi_ctxt->er_ctxt_addr),
> > },
> > {
> > - CRCBAP_HIGHER, U32_MAX, 0,
> > + CRCBAP_HIGHER, U32_MAX,
> > upper_32_bits(mhi_cntrl->mhi_ctxt->cmd_ctxt_addr),
> > },
> > {
> > - CRCBAP_LOWER, U32_MAX, 0,
> > + CRCBAP_LOWER, U32_MAX,
> > lower_32_bits(mhi_cntrl->mhi_ctxt->cmd_ctxt_addr),
> > },
> > {
> > - MHICFG, MHICFG_NER_MASK, MHICFG_NER_SHIFT,
> > + MHICFG, MHICFG_NER_MASK,
> > mhi_cntrl->total_ev_rings,
> > },
> > {
> > - MHICFG, MHICFG_NHWER_MASK, MHICFG_NHWER_SHIFT,
> > + MHICFG, MHICFG_NHWER_MASK,
> > mhi_cntrl->hw_ev_rings,
> > },
> > {
> > - MHICTRLBASE_HIGHER, U32_MAX, 0,
> > + MHICTRLBASE_HIGHER, U32_MAX,
> > upper_32_bits(mhi_cntrl->iova_start),
> > },
> > {
> > - MHICTRLBASE_LOWER, U32_MAX, 0,
> > + MHICTRLBASE_LOWER, U32_MAX,
> > lower_32_bits(mhi_cntrl->iova_start),
> > },
> > {
> > - MHIDATABASE_HIGHER, U32_MAX, 0,
> > + MHIDATABASE_HIGHER, U32_MAX,
> > upper_32_bits(mhi_cntrl->iova_start),
> > },
> > {
> > - MHIDATABASE_LOWER, U32_MAX, 0,
> > + MHIDATABASE_LOWER, U32_MAX,
> > lower_32_bits(mhi_cntrl->iova_start),
> > },
> > {
> > - MHICTRLLIMIT_HIGHER, U32_MAX, 0,
> > + MHICTRLLIMIT_HIGHER, U32_MAX,
> > upper_32_bits(mhi_cntrl->iova_stop),
> > },
> > {
> > - MHICTRLLIMIT_LOWER, U32_MAX, 0,
> > + MHICTRLLIMIT_LOWER, U32_MAX,
> > lower_32_bits(mhi_cntrl->iova_stop),
> > },
> > {
> > - MHIDATALIMIT_HIGHER, U32_MAX, 0,
> > + MHIDATALIMIT_HIGHER, U32_MAX,
> > upper_32_bits(mhi_cntrl->iova_stop),
> > },
> > {
> > - MHIDATALIMIT_LOWER, U32_MAX, 0,
> > + MHIDATALIMIT_LOWER, U32_MAX,
> > lower_32_bits(mhi_cntrl->iova_stop),
> > },
> > { 0, 0, 0 }
> > @@ -498,8 +498,7 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
> > dev_dbg(dev, "Initializing MHI registers\n");
> > /* Read channel db offset */
> > - ret = mhi_read_reg_field(mhi_cntrl, base, CHDBOFF, CHDBOFF_CHDBOFF_MASK,
> > - CHDBOFF_CHDBOFF_SHIFT, &val);
> > + ret = mhi_read_reg_field(mhi_cntrl, base, CHDBOFF, CHDBOFF_CHDBOFF_MASK, &val);
> > if (ret) {
> > dev_err(dev, "Unable to read CHDBOFF register\n");
> > return -EIO;
> > @@ -515,8 +514,7 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
> > mhi_chan->tre_ring.db_addr = base + val;
> > /* Read event ring db offset */
> > - ret = mhi_read_reg_field(mhi_cntrl, base, ERDBOFF, ERDBOFF_ERDBOFF_MASK,
> > - ERDBOFF_ERDBOFF_SHIFT, &val);
> > + ret = mhi_read_reg_field(mhi_cntrl, base, ERDBOFF, ERDBOFF_ERDBOFF_MASK, &val);
> > if (ret) {
> > dev_err(dev, "Unable to read ERDBOFF register\n");
> > return -EIO;
> > @@ -537,8 +535,7 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
> > /* Write to MMIO registers */
> > for (i = 0; reg_info[i].offset; i++)
> > mhi_write_reg_field(mhi_cntrl, base, reg_info[i].offset,
> > - reg_info[i].mask, reg_info[i].shift,
> > - reg_info[i].val);
> > + reg_info[i].mask, reg_info[i].val);
> > return 0;
> > }
> > @@ -571,7 +568,7 @@ void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl,
> > tmp = le32_to_cpu(chan_ctxt->chcfg);
> > tmp &= ~CHAN_CTX_CHSTATE_MASK;
> > - tmp |= (MHI_CH_STATE_DISABLED << CHAN_CTX_CHSTATE_SHIFT);
> > + tmp |= FIELD_PREP(CHAN_CTX_CHSTATE_MASK, MHI_CH_STATE_DISABLED);
> > chan_ctxt->chcfg = cpu_to_le32(tmp);
> > /* Update to all cores */
> > @@ -608,7 +605,7 @@ int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl,
> > tmp = le32_to_cpu(chan_ctxt->chcfg);
> > tmp &= ~CHAN_CTX_CHSTATE_MASK;
> > - tmp |= (MHI_CH_STATE_ENABLED << CHAN_CTX_CHSTATE_SHIFT);
> > + tmp |= FIELD_PREP(CHAN_CTX_CHSTATE_MASK, MHI_CH_STATE_ENABLED);
> > chan_ctxt->chcfg = cpu_to_le32(tmp);
> > chan_ctxt->rbase = cpu_to_le64(tre_ring->iommu_base);
> > @@ -952,14 +949,10 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
> > if (ret)
> > goto err_destroy_wq;
> > - mhi_cntrl->family_number = (soc_info & SOC_HW_VERSION_FAM_NUM_BMSK) >>
> > - SOC_HW_VERSION_FAM_NUM_SHFT;
> > - mhi_cntrl->device_number = (soc_info & SOC_HW_VERSION_DEV_NUM_BMSK) >>
> > - SOC_HW_VERSION_DEV_NUM_SHFT;
> > - mhi_cntrl->major_version = (soc_info & SOC_HW_VERSION_MAJOR_VER_BMSK) >>
> > - SOC_HW_VERSION_MAJOR_VER_SHFT;
> > - mhi_cntrl->minor_version = (soc_info & SOC_HW_VERSION_MINOR_VER_BMSK) >>
> > - SOC_HW_VERSION_MINOR_VER_SHFT;
> > + mhi_cntrl->family_number = FIELD_GET(SOC_HW_VERSION_FAM_NUM_BMSK, soc_info);
> > + mhi_cntrl->device_number = FIELD_GET(SOC_HW_VERSION_DEV_NUM_BMSK, soc_info);
> > + mhi_cntrl->major_version = FIELD_GET(SOC_HW_VERSION_MAJOR_VER_BMSK, soc_info);
> > + mhi_cntrl->minor_version = FIELD_GET(SOC_HW_VERSION_MINOR_VER_BMSK, soc_info);
> > mhi_cntrl->index = ida_alloc(&mhi_controller_ida, GFP_KERNEL);
> > if (mhi_cntrl->index < 0) {
> > diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
> > index 762055a6ec9f..21381781d7c5 100644
> > --- a/drivers/bus/mhi/host/internal.h
> > +++ b/drivers/bus/mhi/host/internal.h
> > @@ -82,13 +82,9 @@ extern struct bus_type mhi_bus_type;
> > #define SOC_HW_VERSION_OFFS 0x224
> > #define SOC_HW_VERSION_FAM_NUM_BMSK GENMASK(31, 28)
> > -#define SOC_HW_VERSION_FAM_NUM_SHFT 28
> > #define SOC_HW_VERSION_DEV_NUM_BMSK GENMASK(27, 16)
> > -#define SOC_HW_VERSION_DEV_NUM_SHFT 16
> > #define SOC_HW_VERSION_MAJOR_VER_BMSK GENMASK(15, 8)
> > -#define SOC_HW_VERSION_MAJOR_VER_SHFT 8
> > #define SOC_HW_VERSION_MINOR_VER_BMSK GENMASK(7, 0)
> > -#define SOC_HW_VERSION_MINOR_VER_SHFT 0
> > struct mhi_ctxt {
> > struct mhi_event_ctxt *er_ctxt;
> > @@ -393,14 +389,14 @@ int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl,
> > void __iomem *base, u32 offset, u32 *out);
> > int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
> > void __iomem *base, u32 offset, u32 mask,
> > - u32 shift, u32 *out);
> > + u32 *out);
> > int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl,
> > void __iomem *base, u32 offset, u32 mask,
> > - u32 shift, u32 val, u32 delayus);
> > + u32 val, u32 delayus);
> > void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base,
> > u32 offset, u32 val);
> > void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base,
> > - u32 offset, u32 mask, u32 shift, u32 val);
> > + u32 offset, u32 mask, u32 val);
> > void mhi_ring_er_db(struct mhi_event *mhi_event);
> > void mhi_write_db(struct mhi_controller *mhi_cntrl, void __iomem *db_addr,
> > dma_addr_t db_val);
> > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> > index e436c2993d97..02ac5faf9178 100644
> > --- a/drivers/bus/mhi/host/main.c
> > +++ b/drivers/bus/mhi/host/main.c
> > @@ -24,7 +24,7 @@ int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl,
> > int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
> > void __iomem *base, u32 offset,
> > - u32 mask, u32 shift, u32 *out)
> > + u32 mask, u32 *out)
> > {
> > u32 tmp;
> > int ret;
> > @@ -33,21 +33,20 @@ int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
> > if (ret)
> > return ret;
> > - *out = (tmp & mask) >> shift;
> > + *out = (tmp & mask) >> __ffs(mask);
> > return 0;
> > }
> > int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl,
> > void __iomem *base, u32 offset,
> > - u32 mask, u32 shift, u32 val, u32 delayus)
> > + u32 mask, u32 val, u32 delayus)
> > {
> > int ret;
> > u32 out, retry = (mhi_cntrl->timeout_ms * 1000) / delayus;
> > while (retry--) {
> > - ret = mhi_read_reg_field(mhi_cntrl, base, offset, mask, shift,
> > - &out);
> > + ret = mhi_read_reg_field(mhi_cntrl, base, offset, mask, &out);
> > if (ret)
> > return ret;
> > @@ -67,7 +66,7 @@ void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base,
> > }
> > void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base,
> > - u32 offset, u32 mask, u32 shift, u32 val)
> > + u32 offset, u32 mask, u32 val)
> > {
> > int ret;
> > u32 tmp;
> > @@ -77,7 +76,7 @@ void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base,
> > return;
> > tmp &= ~mask;
> > - tmp |= (val << shift);
> > + tmp |= (val << __ffs(mask));
> > mhi_write_reg(mhi_cntrl, base, offset, tmp);
> > }
> > @@ -159,8 +158,7 @@ enum mhi_state mhi_get_mhi_state(struct mhi_controller *mhi_cntrl)
> > {
> > u32 state;
> > int ret = mhi_read_reg_field(mhi_cntrl, mhi_cntrl->regs, MHISTATUS,
> > - MHISTATUS_MHISTATE_MASK,
> > - MHISTATUS_MHISTATE_SHIFT, &state);
> > + MHISTATUS_MHISTATE_MASK, &state);
> > return ret ? MHI_STATE_MAX : state;
> > }
> > EXPORT_SYMBOL_GPL(mhi_get_mhi_state);
> > diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
> > index 088ade0f3e0b..3d90b8ecd3d9 100644
> > --- a/drivers/bus/mhi/host/pm.c
> > +++ b/drivers/bus/mhi/host/pm.c
> > @@ -131,11 +131,10 @@ void mhi_set_mhi_state(struct mhi_controller *mhi_cntrl, enum mhi_state state)
> > {
> > if (state == MHI_STATE_RESET) {
> > mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
> > - MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 1);
> > + MHICTRL_RESET_MASK, 1);
> > } else {
> > mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
> > - MHICTRL_MHISTATE_MASK,
> > - MHICTRL_MHISTATE_SHIFT, state);
> > + MHICTRL_MHISTATE_MASK, state);
> > }
> > }
> > @@ -167,16 +166,14 @@ int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl)
> > /* Wait for RESET to be cleared and READY bit to be set by the device */
> > ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
> > - MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0,
> > - interval_us);
> > + MHICTRL_RESET_MASK, 0, interval_us);
> > if (ret) {
> > dev_err(dev, "Device failed to clear MHI Reset\n");
> > return ret;
> > }
> > ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHISTATUS,
> > - MHISTATUS_READY_MASK, MHISTATUS_READY_SHIFT, 1,
> > - interval_us);
> > + MHISTATUS_READY_MASK, 1, interval_us);
> > if (ret) {
> > dev_err(dev, "Device failed to enter MHI Ready\n");
> > return ret;
> > @@ -470,8 +467,7 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl)
> > /* Wait for the reset bit to be cleared by the device */
> > ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
> > - MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0,
> > - 25000);
> > + MHICTRL_RESET_MASK, 0, 25000);
> > if (ret)
> > dev_err(dev, "Device failed to clear MHI Reset\n");
> > @@ -602,7 +598,6 @@ static void mhi_pm_sys_error_transition(struct mhi_controller *mhi_cntrl)
> > mhi_cntrl->regs,
> > MHICTRL,
> > MHICTRL_RESET_MASK,
> > - MHICTRL_RESET_SHIFT,
> > &in_reset) ||
> > !in_reset, timeout);
> > if (!ret || in_reset) {
> > @@ -1093,8 +1088,7 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
> > if (state == MHI_STATE_SYS_ERR) {
> > mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET);
> > ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
> > - MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0,
> > - interval_us);
> > + MHICTRL_RESET_MASK, 0, interval_us);
> > if (ret) {
> > dev_info(dev, "Failed to reset MHI due to syserr state\n");
> > goto error_exit;
>
Powered by blists - more mailing lists