Re: [PATCH 08/20] bus: mhi: ep: Add support for managing MMIO registers

From: Manivannan Sadhasivam
Date: Thu Feb 03 2022 - 10:55:56 EST

On Wed, Jan 05, 2022 at 06:29:00PM -0600, Alex Elder wrote:
> On 12/2/21 5:35 AM, Manivannan Sadhasivam wrote:
> > Add support for managing the Memory Mapped Input Output (MMIO) registers
> > of the MHI bus. All MHI operations are carried out using the MMIO registers
> > by both host and the endpoint device.
> >
> > The MMIO registers reside inside the endpoint device memory (fixed
> > location based on the platform) and the address is passed by the MHI EP
> > controller driver during its registration.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> > drivers/bus/mhi/ep/Makefile | 2 +-
> > drivers/bus/mhi/ep/internal.h | 36 ++++
> > drivers/bus/mhi/ep/main.c | 6 +-
> > drivers/bus/mhi/ep/mmio.c | 303 ++++++++++++++++++++++++++++++++++
> > include/linux/mhi_ep.h | 18 ++
> > 5 files changed, 363 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/bus/mhi/ep/mmio.c
> >
> > diff --git a/drivers/bus/mhi/ep/Makefile b/drivers/bus/mhi/ep/Makefile
> > index 64e29252b608..a1555ae287ad 100644
> > --- a/drivers/bus/mhi/ep/Makefile
> > +++ b/drivers/bus/mhi/ep/Makefile
> > @@ -1,2 +1,2 @@
> > obj-$(CONFIG_MHI_BUS_EP) += mhi_ep.o
> > -mhi_ep-y := main.o
> > +mhi_ep-y := main.o mmio.o
> > diff --git a/drivers/bus/mhi/ep/internal.h b/drivers/bus/mhi/ep/internal.h
> > index 7b164daf4332..39eeb5f384e2 100644
> > --- a/drivers/bus/mhi/ep/internal.h
> > +++ b/drivers/bus/mhi/ep/internal.h
> > @@ -91,6 +91,12 @@ struct mhi_generic_ctx {
> > __u64 wp __packed __aligned(4);
> > };
>
> Maybe add a comment defining SBL as "secondary boot loader" and AMSS
> as "advanced modem subsystem".
>

Sure, will add kernel doc. But from modem terms, AMSS refers to
"Advanced Mode Subscriber Software".

> > +enum mhi_ep_execenv {
> > + MHI_EP_SBL_EE = 1,
> > + MHI_EP_AMSS_EE = 2,
> > + MHI_EP_UNRESERVED
> > +};
> > +
> > enum mhi_ep_ring_state {
> > RING_STATE_UINT = 0,
> > RING_STATE_IDLE,
> > @@ -155,4 +161,34 @@ struct mhi_ep_chan {
> > bool skip_td;
> > };
> > +/* MMIO related functions */
>
> I would *really* rather have the mmio_read functions *return* the read
> value, rather than having the address of the location to store it passed
> as argument. Your MMIO calls never fail, so there's no need to return
> anything else. Returning the value also makes it more obvious that the
> *result* is getting assigned (rather than sort of implying it by passing
> in the address of the result). And there's no possibility of someone
> passing a bad pointer that way either.
>
> > +void mhi_ep_mmio_read(struct mhi_ep_cntrl *mhi_cntrl, u32 offset, u32 *regval);
>
> In other words:
>
> u32 mhi_ep_mmio_read(struct mhi_ep_ctrl *mhi_ctrl, u32 offset);
>
> > +void mhi_ep_mmio_write(struct mhi_ep_cntrl *mhi_cntrl, u32 offset, u32 val);
> > +void mhi_ep_mmio_masked_write(struct mhi_ep_cntrl *mhi_cntrl, u32 offset,
> > + u32 mask, u32 shift, u32 val);
> > +int mhi_ep_mmio_masked_read(struct mhi_ep_cntrl *dev, u32 offset,
> > + u32 mask, u32 shift, u32 *regval);
> > +void mhi_ep_mmio_enable_ctrl_interrupt(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_disable_ctrl_interrupt(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_enable_cmdb_interrupt(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_disable_cmdb_interrupt(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_enable_chdb_a7(struct mhi_ep_cntrl *mhi_cntrl, u32 chdb_id);
> > +void mhi_ep_mmio_disable_chdb_a7(struct mhi_ep_cntrl *mhi_cntrl, u32 chdb_id);
> > +void mhi_ep_mmio_enable_chdb_interrupts(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_read_chdb_status_interrupts(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_mask_interrupts(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_get_chc_base(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_get_erc_base(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_get_crc_base(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_get_ch_db(struct mhi_ep_ring *ring, u64 *wr_offset);
> > +void mhi_ep_mmio_get_er_db(struct mhi_ep_ring *ring, u64 *wr_offset);
> > +void mhi_ep_mmio_get_cmd_db(struct mhi_ep_ring *ring, u64 *wr_offset);
> > +void mhi_ep_mmio_set_env(struct mhi_ep_cntrl *mhi_cntrl, u32 value);
> > +void mhi_ep_mmio_clear_reset(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_reset(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_get_mhi_state(struct mhi_ep_cntrl *mhi_cntrl, enum mhi_state *state,
> > + bool *mhi_reset);
> > +void mhi_ep_mmio_init(struct mhi_ep_cntrl *mhi_cntrl);
> > +void mhi_ep_mmio_update_ner(struct mhi_ep_cntrl *mhi_cntrl);
> > +
> > #endif
> > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> > index f0b5f49db95a..fddf75dfb9c7 100644
> > --- a/drivers/bus/mhi/ep/main.c
> > +++ b/drivers/bus/mhi/ep/main.c
> > @@ -209,7 +209,7 @@ int mhi_ep_register_controller(struct mhi_ep_cntrl *mhi_cntrl,
> > struct mhi_ep_device *mhi_dev;
> > int ret;
> > - if (!mhi_cntrl || !mhi_cntrl->cntrl_dev)
> > + if (!mhi_cntrl || !mhi_cntrl->cntrl_dev || !mhi_cntrl->mmio)
> > return -EINVAL;
> > ret = parse_ch_cfg(mhi_cntrl, config);
> > @@ -222,6 +222,10 @@ int mhi_ep_register_controller(struct mhi_ep_cntrl *mhi_cntrl,
> > goto err_free_ch;
> > }
> > + /* Set MHI version and AMSS EE before enumeration */
> > + mhi_ep_mmio_write(mhi_cntrl, MHIVER, config->mhi_version);
> > + mhi_ep_mmio_set_env(mhi_cntrl, MHI_EP_AMSS_EE);
> > +
> > /* Set controller index */
> > mhi_cntrl->index = ida_alloc(&mhi_ep_cntrl_ida, GFP_KERNEL);
> > if (mhi_cntrl->index < 0) {
> > diff --git a/drivers/bus/mhi/ep/mmio.c b/drivers/bus/mhi/ep/mmio.c
> > new file mode 100644
> > index 000000000000..157ef1240f6f
> > --- /dev/null
> > +++ b/drivers/bus/mhi/ep/mmio.c
> > @@ -0,0 +1,303 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2021 Linaro Ltd.
> > + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/io.h>
> > +#include <linux/mhi_ep.h>
> > +
> > +#include "internal.h"
> > +
> > +void mhi_ep_mmio_read(struct mhi_ep_cntrl *mhi_cntrl, u32 offset, u32 *regval)
> > +{
> > + *regval = readl(mhi_cntrl->mmio + offset);
>
> return readl(...);
>

done

> > +}
> > +
> > +void mhi_ep_mmio_write(struct mhi_ep_cntrl *mhi_cntrl, u32 offset, u32 val)
> > +{
> > + writel(val, mhi_cntrl->mmio + offset);
> > +}
> > +
> > +void mhi_ep_mmio_masked_write(struct mhi_ep_cntrl *mhi_cntrl, u32 offset, u32 mask,
> > + u32 shift, u32 val)
>
> There is no need for a shift argument here. I would like to say
> "use the bitfield functions" but at the moment they require the
> mask to be constant. You could still do that, by having all
> these be defined as static inline functions in a header though.
> Maybe you can use FIELD_GET() though, I don't know.
>

I've used __ffs to determine the shift.

> Anyway, try to get rid of these shifts; they shouldn't be
> necessary.
>
> > +{
> > + u32 regval;
> > +
> > + mhi_ep_mmio_read(mhi_cntrl, offset, &regval);
> > + regval &= ~mask;
> > + regval |= ((val << shift) & mask);
> > + mhi_ep_mmio_write(mhi_cntrl, offset, regval);
> > +}
> > +
> > +int mhi_ep_mmio_masked_read(struct mhi_ep_cntrl *dev, u32 offset,
> > + u32 mask, u32 shift, u32 *regval)
> > +{
> > + mhi_ep_mmio_read(dev, offset, regval);
> > + *regval &= mask;
> > + *regval >>= shift;
> > +
> > + return 0;
>
> There is no point in returning 0 from this function.
>
> > +}
> > +
> > +void mhi_ep_mmio_get_mhi_state(struct mhi_ep_cntrl *mhi_cntrl, enum mhi_state *state,
> > + bool *mhi_reset)
> > +{
> > + u32 regval;
> > +
> > + mhi_ep_mmio_read(mhi_cntrl, MHICTRL, &regval);
> > + *state = FIELD_GET(MHICTRL_MHISTATE_MASK, regval);
> > + *mhi_reset = !!FIELD_GET(MHICTRL_RESET_MASK, regval);
> > +}
> > +
> > +static void mhi_ep_mmio_mask_set_chdb_int_a7(struct mhi_ep_cntrl *mhi_cntrl,
> > + u32 chdb_id, bool enable)
> > +{
> > + u32 chid_mask, chid_idx, chid_shft, val = 0;
> > +
> > + chid_shft = chdb_id % 32;
> > + chid_mask = BIT(chid_shft);
> > + chid_idx = chdb_id / 32;
> > +
> > + if (chid_idx >= MHI_MASK_ROWS_CH_EV_DB)
> > + return;
>
> The above should maybe issue a warning?
>

ack

> > +
> > + if (enable)
> > + val = 1;
> > +
> > + mhi_ep_mmio_masked_write(mhi_cntrl, MHI_CHDB_INT_MASK_A7_n(chid_idx),
> > + chid_mask, chid_shft, val);
> > + mhi_ep_mmio_read(mhi_cntrl, MHI_CHDB_INT_MASK_A7_n(chid_idx),
> > + &mhi_cntrl->chdb[chid_idx].mask);
>
> Why do you read after writing? Is this to be sure the write completes
> over PCIe or something? Even then I don't think that would be needed
> because the memory is on "this side" of PCIe (right?).
>

This is done to update the mask. We could also do the bit managment stuff here
instead of reading from the register (I guess that'll be faster).

Thanks,
Mani

> > +}
> > +
> > +void mhi_ep_mmio_enable_chdb_a7(struct mhi_ep_cntrl *mhi_cntrl, u32 chdb_id)
> > +{
> > + mhi_ep_mmio_mask_set_chdb_int_a7(mhi_cntrl, chdb_id, true);
> > +}
> > +
> > +void mhi_ep_mmio_disable_chdb_a7(struct mhi_ep_cntrl *mhi_cntrl, u32 chdb_id)
> > +{
> > + mhi_ep_mmio_mask_set_chdb_int_a7(mhi_cntrl, chdb_id, false);
> > +}
> > +
> > +static void mhi_ep_mmio_set_chdb_interrupts(struct mhi_ep_cntrl *mhi_cntrl, bool enable)
> > +{
> > + u32 val = 0, i = 0;
>
> No need for assigning 0 to i.
>
> -Alex
>
> > +
> > + if (enable)
> > + val = MHI_CHDB_INT_MASK_A7_n_EN_ALL;
> > +
> > + for (i = 0; i < MHI_MASK_ROWS_CH_EV_DB; i++) {
> > + mhi_ep_mmio_write(mhi_cntrl, MHI_CHDB_INT_MASK_A7_n(i), val);
> > + mhi_cntrl->chdb[i].mask = val;
> > + }
> > +}
> > +
>
> . . .