Message-ID: <20220217095319.GA11964@workstation>
Date:   Thu, 17 Feb 2022 15:23:19 +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 08/25] bus: mhi: ep: Add support for registering MHI
 endpoint controllers

On Tue, Feb 15, 2022 at 02:02:41PM -0600, Alex Elder wrote:

[...]

> > +#define MHI_REG_OFFSET				0x100
> > +#define BHI_REG_OFFSET				0x200
> 
> Rather than defining the REG_OFFSET values here and adding
> them to every definition below, why not have the base
> address used (e.g., in mhi_write_reg_field()) be adjusted
> by the constant amount?
> 
> I'm just looking at mhi_init_mmio() (in the existing code)
> as an example, but for example, the base address used
> comes from mhi_cntrl->regs.  Can you instead just define
> a pointer somewhere that is the base of the MHI register
> range, which is already offset by the appropriate amount?
> 

I've defined two set of APIs for MHI and BHI read/write. They will add the
respective offsets.

> > +

[...]

> > +/* Generic context */
> > +struct mhi_generic_ctx {
> > +	__u32 reserved0;
> > +	__u32 reserved1;
> > +	__u32 reserved2;
> > +
> > +	__u64 rbase __packed __aligned(4);
> > +	__u64 rlen __packed __aligned(4);
> > +	__u64 rp __packed __aligned(4);
> > +	__u64 wp __packed __aligned(4);
> > +};
> 
> I'm pretty sure this constitutes an external interface, so
> every field should have its endianness annotated.
> 
> Mentioned elsewhere, I think you can define the structure
> with those attributes rather than the multiple fields.
> 

As I said before, this was suggested by Arnd during MHI host review. He
suggested adding the alignment and packed to only members that require
them.

But I think I should change it now...

> > +
> > +enum mhi_ep_ring_type {
> > +	RING_TYPE_CMD = 0,
> > +	RING_TYPE_ER,
> > +	RING_TYPE_CH,
> > +};
> > +
> > +struct mhi_ep_ring_element {
> > +	u64 ptr;
> > +	u32 dword[2];
> > +};
> 
> Are these host resident rings?  Even if not, this is an external
> interface, so this should be defined with explicit endianness.
> The cpu_to_le64() call will be a no-op so there is no cost
> to correcting this.
> 

Ah, this should be reusing the "struct mhi_tre" defined in host. Will do.

> > +
> > +/* Ring element */
> > +union mhi_ep_ring_ctx {
> > +	struct mhi_cmd_ctxt cmd;
> > +	struct mhi_event_ctxt ev;
> > +	struct mhi_chan_ctxt ch;
> > +	struct mhi_generic_ctx generic;
> > +};
> > +
> > +struct mhi_ep_ring {
> > +	struct mhi_ep_cntrl *mhi_cntrl;
> > +	int (*ring_cb)(struct mhi_ep_ring *ring, struct mhi_ep_ring_element *el);
> > +	union mhi_ep_ring_ctx *ring_ctx;
> > +	struct mhi_ep_ring_element *ring_cache;
> > +	enum mhi_ep_ring_type type;
> > +	size_t rd_offset;
> > +	size_t wr_offset;
> > +	size_t ring_size;
> > +	u32 db_offset_h;
> > +	u32 db_offset_l;
> > +	u32 ch_id;
> > +};
> 
> Not sure about the db_offset fields, etc. here, but it's possible
> they need endianness annotations.  I'm going to stop making this
> comment; please make sure anything that's exposed to the host
> specifies that it's little endian.  (The host and endpoint should
> have a common definition of these shared structures anyway; maybe
> I'm misreading this or assuming something incorrectly.)
> 

db_offset_* just holds the register offsets so they don't require
endianness annotation. All MMIO read/write are using readl/writel APIs
and they handle the endianness conversion implicitly.

Rest of the host memory accesses are annotated properly.

> > +

[...]

> > +	/*
> > +	 * Allocate max_channels supported by the MHI endpoint and populate
> > +	 * only the defined channels
> > +	 */
> > +	mhi_cntrl->mhi_chan = kcalloc(mhi_cntrl->max_chan, sizeof(*mhi_cntrl->mhi_chan),
> > +				      GFP_KERNEL);
> > +	if (!mhi_cntrl->mhi_chan)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < config->num_channels; i++) {
> > +		struct mhi_ep_chan *mhi_chan;
> 
> This entire block could be encapsulated in mhi_channel_add()
> or something,

Wrapping up in a function is useful if the same code is used in
different places. But I don't think it adds any value here.

> 
> > +		ch_cfg = &config->ch_cfg[i];
> 
> Move the above assignment down a few lines, to just before
> where it's used.
> 

No, ch_cfg is used just below this.

> > +
> > +		chan = ch_cfg->num;
> > +		if (chan >= mhi_cntrl->max_chan) {
> > +			dev_err(dev, "Channel %d not available\n", chan);
> 
> Maybe report the maximum channel so it's obvious why it's
> not available.
> 
> > +			goto error_chan_cfg;
> > +		}
> > +
> > +		/* Bi-directional and direction less channels are not supported */
> > +		if (ch_cfg->dir == DMA_BIDIRECTIONAL || ch_cfg->dir == DMA_NONE) {
> > +			dev_err(dev, "Invalid channel configuration\n");
> 
> Maybe be more specific in your message about what's wrong here.
> 
> > +			goto error_chan_cfg;
> > +		}
> > +
> > +		mhi_chan = &mhi_cntrl->mhi_chan[chan];
> > +		mhi_chan->name = ch_cfg->name;
> > +		mhi_chan->chan = chan;
> > +		mhi_chan->dir = ch_cfg->dir;
> > +		mutex_init(&mhi_chan->lock);
> > +	}
> > +
> > +	return 0;
> > +
> > +error_chan_cfg:
> > +	kfree(mhi_cntrl->mhi_chan);
> 
> I'm not sure what the caller does, but maybe null this
> after it's freed, or don't assign mhi_cntrll->mhi_chan
> until the initialization is successful.
> 

This is not required here as there will be no access to the pointer
after failing.

> 
> > +	return ret;
> > +}
> > +
> > +/*
> > + * Allocate channel and command rings here. Event rings will be allocated
> > + * in mhi_ep_power_up() as the config comes from the host.
> > + */
> > +int mhi_ep_register_controller(struct mhi_ep_cntrl *mhi_cntrl,
> > +				const struct mhi_ep_cntrl_config *config)
> > +{
> > +	struct mhi_ep_device *mhi_dev;
> > +	int ret;
> > +
> > +	if (!mhi_cntrl || !mhi_cntrl->cntrl_dev)
> > +		return -EINVAL;
> > +
> > +	ret = parse_ch_cfg(mhi_cntrl, config);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mhi_cntrl->mhi_cmd = kcalloc(NR_OF_CMD_RINGS, sizeof(*mhi_cntrl->mhi_cmd), GFP_KERNEL);
> 
> I said before I thought it was silly to even define NR_OF_CMD_RINGS.
> Does the MHI specification actually allow more than one command
> ring for a given MHI controller?  Ever?
> 

MHI spec doesn't limit the number of command rings. Eventhough I don't
envision adding more command rings in the future, I'm going to keep this
macro for now as the MHI host does the same.

[...]

> > diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h
> > new file mode 100644
> > index 000000000000..20238e9df1b3
> > --- /dev/null
> > +++ b/include/linux/mhi_ep.h

[...]

> > +struct mhi_ep_device {
> > +	struct device dev;
> > +	struct mhi_ep_cntrl *mhi_cntrl;
> > +	const struct mhi_device_id *id;
> > +	const char *name;
> > +	struct mhi_ep_chan *ul_chan;
> > +	struct mhi_ep_chan *dl_chan;
> > +	enum mhi_device_type dev_type;
> 
> There are two device types, controller and transfer.  Unless
> there is ever going to be anything more than that, I think
> the distinction is better represented as a Boolean, such as:
> 
> 	bool controller;

Again, this is how it is done in MHI host also. Since I'm going to
maintain both stacks, it makes it easier for me if similarities are
maintained. But I'll keep this suggestion and the one above for later.

Thanks,
Mani

Powered by blists - more mailing lists