Messages in this thread | | | Date | Tue, 15 Feb 2022 14:02:41 -0600 | Subject | Re: [PATCH v3 08/25] bus: mhi: ep: Add support for registering MHI endpoint controllers | From | Alex Elder <> |
| |
On 2/12/22 12:21 PM, Manivannan Sadhasivam wrote: > This commit adds support for registering MHI endpoint controller drivers > with the MHI endpoint stack. MHI endpoint controller drivers manages
s/manages/manage/
> the interaction with the host machines such as x86. They are also the
(such as x86)
> MHI endpoint bus master in charge of managing the physical link between the > host and endpoint device. > > The endpoint controller driver encloses all information about the > underlying physical bus like PCIe. The registration process involves
s/like PCIe/(i.e., PCIe)/
> parsing the channel configuration and allocating an MHI EP device. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
OK!!! On to the MHI endpoint code!
Quite a few comments below, but nothing very major.
-Alex
> --- > drivers/bus/mhi/Kconfig | 1 + > drivers/bus/mhi/Makefile | 3 + > drivers/bus/mhi/ep/Kconfig | 10 ++ > drivers/bus/mhi/ep/Makefile | 2 + > drivers/bus/mhi/ep/internal.h | 160 +++++++++++++++++++++++ > drivers/bus/mhi/ep/main.c | 234 ++++++++++++++++++++++++++++++++++ > include/linux/mhi_ep.h | 143 +++++++++++++++++++++ > 7 files changed, 553 insertions(+) > create mode 100644 drivers/bus/mhi/ep/Kconfig > create mode 100644 drivers/bus/mhi/ep/Makefile > create mode 100644 drivers/bus/mhi/ep/internal.h > create mode 100644 drivers/bus/mhi/ep/main.c > create mode 100644 include/linux/mhi_ep.h > > diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig > index 4748df7f9cd5..b39a11e6c624 100644 > --- a/drivers/bus/mhi/Kconfig > +++ b/drivers/bus/mhi/Kconfig > @@ -6,3 +6,4 @@ > # > > source "drivers/bus/mhi/host/Kconfig" > +source "drivers/bus/mhi/ep/Kconfig" > diff --git a/drivers/bus/mhi/Makefile b/drivers/bus/mhi/Makefile > index 5f5708a249f5..46981331b38f 100644 > --- a/drivers/bus/mhi/Makefile > +++ b/drivers/bus/mhi/Makefile > @@ -1,2 +1,5 @@ > # Host MHI stack > obj-y += host/ > + > +# Endpoint MHI stack > +obj-y += ep/ > diff --git a/drivers/bus/mhi/ep/Kconfig b/drivers/bus/mhi/ep/Kconfig > new file mode 100644 > index 000000000000..229c71397b30 > --- /dev/null > +++ b/drivers/bus/mhi/ep/Kconfig > @@ -0,0 +1,10 @@ > +config MHI_BUS_EP > + tristate "Modem Host Interface (MHI) bus Endpoint implementation" > + help > + Bus driver for MHI protocol. Modem Host Interface (MHI) is a > + communication protocol used by the host processors to control
s/the host processors/a host processor/
> + and communicate with modem devices over a high speed peripheral
s/modem devices/a modem device/
> + bus or shared memory. > + > + MHI_BUS_EP implements the MHI protocol for the endpoint devices > + like SDX55 modem connected to the host machine over PCIe.
s/devices like/devices, such as/
> diff --git a/drivers/bus/mhi/ep/Makefile b/drivers/bus/mhi/ep/Makefile > new file mode 100644 > index 000000000000..64e29252b608 > --- /dev/null > +++ b/drivers/bus/mhi/ep/Makefile > @@ -0,0 +1,2 @@ > +obj-$(CONFIG_MHI_BUS_EP) += mhi_ep.o > +mhi_ep-y := main.o > diff --git a/drivers/bus/mhi/ep/internal.h b/drivers/bus/mhi/ep/internal.h > new file mode 100644 > index 000000000000..e313a2546664 > --- /dev/null > +++ b/drivers/bus/mhi/ep/internal.h > @@ -0,0 +1,160 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2021, Linaro Ltd.
Update your copyright statement (here and everywhere before you send your next version).
> + * > + */ > + > +#ifndef _MHI_EP_INTERNAL_ > +#define _MHI_EP_INTERNAL_ > + > +#include <linux/bitfield.h> > + > +#include "../common.h" > + > +extern struct bus_type mhi_ep_bus_type; > + > +#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?
> + > +/* MHI registers */ > +#define MHIREGLEN (MHI_REG_OFFSET + REG_MHIREGLEN) > +#define MHIVER (MHI_REG_OFFSET + REG_MHIVER) > +#define MHICFG (MHI_REG_OFFSET + REG_MHICFG) > +#define CHDBOFF (MHI_REG_OFFSET + REG_CHDBOFF) > +#define ERDBOFF (MHI_REG_OFFSET + REG_ERDBOFF) > +#define BHIOFF (MHI_REG_OFFSET + REG_BHIOFF) > +#define BHIEOFF (MHI_REG_OFFSET + REG_BHIEOFF) > +#define DEBUGOFF (MHI_REG_OFFSET + REG_DEBUGOFF) > +#define MHICTRL (MHI_REG_OFFSET + REG_MHICTRL) > +#define MHISTATUS (MHI_REG_OFFSET + REG_MHISTATUS) > +#define CCABAP_LOWER (MHI_REG_OFFSET + REG_CCABAP_LOWER) > +#define CCABAP_HIGHER (MHI_REG_OFFSET + REG_CCABAP_HIGHER) > +#define ECABAP_LOWER (MHI_REG_OFFSET + REG_ECABAP_LOWER) > +#define ECABAP_HIGHER (MHI_REG_OFFSET + REG_ECABAP_HIGHER) > +#define CRCBAP_LOWER (MHI_REG_OFFSET + REG_CRCBAP_LOWER) > +#define CRCBAP_HIGHER (MHI_REG_OFFSET + REG_CRCBAP_HIGHER) > +#define CRDB_LOWER (MHI_REG_OFFSET + REG_CRDB_LOWER) > +#define CRDB_HIGHER (MHI_REG_OFFSET + REG_CRDB_HIGHER) > +#define MHICTRLBASE_LOWER (MHI_REG_OFFSET + REG_MHICTRLBASE_LOWER) > +#define MHICTRLBASE_HIGHER (MHI_REG_OFFSET + REG_MHICTRLBASE_HIGHER) > +#define MHICTRLLIMIT_LOWER (MHI_REG_OFFSET + REG_MHICTRLLIMIT_LOWER) > +#define MHICTRLLIMIT_HIGHER (MHI_REG_OFFSET + REG_MHICTRLLIMIT_HIGHER) > +#define MHIDATABASE_LOWER (MHI_REG_OFFSET + REG_MHIDATABASE_LOWER) > +#define MHIDATABASE_HIGHER (MHI_REG_OFFSET + REG_MHIDATABASE_HIGHER) > +#define MHIDATALIMIT_LOWER (MHI_REG_OFFSET + REG_MHIDATALIMIT_LOWER) > +#define MHIDATALIMIT_HIGHER (MHI_REG_OFFSET + REG_MHIDATALIMIT_HIGHER) > + > +/* MHI BHI registers */ > +#define BHI_IMGTXDB (BHI_REG_OFFSET + REG_BHI_IMGTXDB) > +#define BHI_EXECENV (BHI_REG_OFFSET + REG_BHI_EXECENV) > +#define BHI_INTVEC (BHI_REG_OFFSET + REG_BHI_INTVEC) > + > +/* MHI Doorbell registers */ > +#define CHDB_LOWER_n(n) (0x400 + 0x8 * (n)) > +#define CHDB_HIGHER_n(n) (0x404 + 0x8 * (n)) > +#define ERDB_LOWER_n(n) (0x800 + 0x8 * (n)) > +#define ERDB_HIGHER_n(n) (0x804 + 0x8 * (n)) > + > +#define MHI_CTRL_INT_STATUS_A7 0x4 > +#define MHI_CTRL_INT_STATUS_A7_MSK BIT(0) > +#define MHI_CTRL_INT_STATUS_CRDB_MSK BIT(1) > +#define MHI_CHDB_INT_STATUS_A7_n(n) (0x28 + 0x4 * (n)) > +#define MHI_ERDB_INT_STATUS_A7_n(n) (0x38 + 0x4 * (n)) > + > +#define MHI_CTRL_INT_CLEAR_A7 0x4c > +#define MHI_CTRL_INT_MMIO_WR_CLEAR BIT(2) > +#define MHI_CTRL_INT_CRDB_CLEAR BIT(1) > +#define MHI_CTRL_INT_CRDB_MHICTRL_CLEAR BIT(0) > + > +#define MHI_CHDB_INT_CLEAR_A7_n(n) (0x70 + 0x4 * (n)) > +#define MHI_CHDB_INT_CLEAR_A7_n_CLEAR_ALL GENMASK(31, 0) > +#define MHI_ERDB_INT_CLEAR_A7_n(n) (0x80 + 0x4 * (n)) > +#define MHI_ERDB_INT_CLEAR_A7_n_CLEAR_ALL GENMASK(31, 0) > + > +/* > + * Unlike the usual "masking" convention, writing "1" to a bit in this register > + * enables the interrupt and writing "0" will disable it.. > + */ > +#define MHI_CTRL_INT_MASK_A7 0x94 > +#define MHI_CTRL_INT_MASK_A7_MASK GENMASK(1, 0) > +#define MHI_CTRL_MHICTRL_MASK BIT(0) > +#define MHI_CTRL_CRDB_MASK BIT(1) > + > +#define MHI_CHDB_INT_MASK_A7_n(n) (0xb8 + 0x4 * (n)) > +#define MHI_CHDB_INT_MASK_A7_n_EN_ALL GENMASK(31, 0) > +#define MHI_ERDB_INT_MASK_A7_n(n) (0xc8 + 0x4 * (n)) > +#define MHI_ERDB_INT_MASK_A7_n_EN_ALL GENMASK(31, 0) > + > +#define NR_OF_CMD_RINGS 1 > +#define MHI_MASK_ROWS_CH_EV_DB 4 > +#define MHI_MASK_CH_EV_LEN 32 > + > +/* 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.
> + > +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.
> + > +/* 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.)
> + > +struct mhi_ep_cmd { > + struct mhi_ep_ring ring; > +}; > + > +struct mhi_ep_event { > + struct mhi_ep_ring ring; > +}; > + > +struct mhi_ep_chan { > + char *name; > + struct mhi_ep_device *mhi_dev; > + struct mhi_ep_ring ring; > + struct mutex lock; > + void (*xfer_cb)(struct mhi_ep_device *mhi_dev, struct mhi_result *result); > + enum mhi_ch_state state; > + enum dma_data_direction dir; > + u64 tre_loc; > + u32 tre_size; > + u32 tre_bytes_left; > + u32 chan; > + bool skip_td; > +}; > + > +#endif > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c > new file mode 100644 > index 000000000000..b006011d025d > --- /dev/null > +++ b/drivers/bus/mhi/ep/main.c > @@ -0,0 +1,234 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * MHI Bus Endpoint stack > + * > + * Copyright (C) 2021 Linaro Ltd. > + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > + */ > + > +#include <linux/bitfield.h> > +#include <linux/delay.h> > +#include <linux/dma-direction.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/mhi_ep.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include "internal.h" > + > +static DEFINE_IDA(mhi_ep_cntrl_ida); > + > +static void mhi_ep_release_device(struct device *dev) > +{ > + struct mhi_ep_device *mhi_dev = to_mhi_ep_device(dev); > + > + if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER) > + mhi_dev->mhi_cntrl->mhi_dev = NULL; > + > + /* > + * We need to set the mhi_chan->mhi_dev to NULL here since the MHI > + * devices for the channels will only get created during start > + * channel if the mhi_dev associated with it is NULL. > + */
Can you mention where in the code the above occurs? Just for reference. Like, "will only get created in mhi_ep_create_device() if the..." or whatever.
> + if (mhi_dev->ul_chan) > + mhi_dev->ul_chan->mhi_dev = NULL; > + > + if (mhi_dev->dl_chan) > + mhi_dev->dl_chan->mhi_dev = NULL; > + > + kfree(mhi_dev); > +} > + > +static struct mhi_ep_device *mhi_ep_alloc_device(struct mhi_ep_cntrl *mhi_cntrl, > + enum mhi_device_type dev_type) > +{ > + struct mhi_ep_device *mhi_dev; > + struct device *dev; > + > + mhi_dev = kzalloc(sizeof(*mhi_dev), GFP_KERNEL); > + if (!mhi_dev) > + return ERR_PTR(-ENOMEM); > + > + dev = &mhi_dev->dev; > + device_initialize(dev); > + dev->bus = &mhi_ep_bus_type; > + dev->release = mhi_ep_release_device; > +
Maybe mention that the controller device is always allocated first.
> + if (dev_type == MHI_DEVICE_CONTROLLER) > + /* for MHI controller device, parent is the bus device (e.g. PCI EPF) */ > + dev->parent = mhi_cntrl->cntrl_dev; > + else > + /* for MHI client devices, parent is the MHI controller device */ > + dev->parent = &mhi_cntrl->mhi_dev->dev; > + > + mhi_dev->mhi_cntrl = mhi_cntrl; > + mhi_dev->dev_type = dev_type; > + > + return mhi_dev; > +} > +
I think the name of the next function could be better. Yes, it parses the channel configuration, but what it *really* does is alloocate and initialize the channel array. So maybe something more like mhi_chan_init()?
> +static int parse_ch_cfg(struct mhi_ep_cntrl *mhi_cntrl, > + const struct mhi_ep_cntrl_config *config) > +{ > + const struct mhi_ep_channel_config *ch_cfg; > + struct device *dev = mhi_cntrl->cntrl_dev; > + u32 chan, i; > + int ret = -EINVAL; > + > + mhi_cntrl->max_chan = config->max_channels; > + > + /* > + * 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,
> + ch_cfg = &config->ch_cfg[i];
Move the above assignment down a few lines, to just before where it's used.
> + > + 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.
> + 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?
> + if (!mhi_cntrl->mhi_cmd) { > + ret = -ENOMEM; > + goto err_free_ch; > + } > + > + /* Set controller index */ > + mhi_cntrl->index = ida_alloc(&mhi_ep_cntrl_ida, GFP_KERNEL); > + if (mhi_cntrl->index < 0) { > + ret = mhi_cntrl->index; > + goto err_free_cmd; > + } > + > + /* Allocate the controller device */ > + mhi_dev = mhi_ep_alloc_device(mhi_cntrl, MHI_DEVICE_CONTROLLER); > + if (IS_ERR(mhi_dev)) { > + dev_err(mhi_cntrl->cntrl_dev, "Failed to allocate controller device\n"); > + ret = PTR_ERR(mhi_dev); > + goto err_ida_free; > + } > + > + dev_set_name(&mhi_dev->dev, "mhi_ep%d", mhi_cntrl->index); > + mhi_dev->name = dev_name(&mhi_dev->dev); > + > + ret = device_add(&mhi_dev->dev); > + if (ret) > + goto err_put_dev; > +
Should the mhi_dev pointer be set before device_add() gets called?
> + mhi_cntrl->mhi_dev = mhi_dev; > + > + dev_dbg(&mhi_dev->dev, "MHI EP Controller registered\n"); > + > + return 0; > + > +err_put_dev: > + put_device(&mhi_dev->dev); > +err_ida_free: > + ida_free(&mhi_ep_cntrl_ida, mhi_cntrl->index); > +err_free_cmd: > + kfree(mhi_cntrl->mhi_cmd); > +err_free_ch: > + kfree(mhi_cntrl->mhi_chan); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(mhi_ep_register_controller); > + > +void mhi_ep_unregister_controller(struct mhi_ep_cntrl *mhi_cntrl) > +{ > + struct mhi_ep_device *mhi_dev = mhi_cntrl->mhi_dev; > + > + kfree(mhi_cntrl->mhi_cmd); > + kfree(mhi_cntrl->mhi_chan); > + > + device_del(&mhi_dev->dev); > + put_device(&mhi_dev->dev); > + > + ida_free(&mhi_ep_cntrl_ida, mhi_cntrl->index); > +} > +EXPORT_SYMBOL_GPL(mhi_ep_unregister_controller); > + > +static int mhi_ep_match(struct device *dev, struct device_driver *drv) > +{ > + struct mhi_ep_device *mhi_dev = to_mhi_ep_device(dev); > + > + /* > + * If the device is a controller type then there is no client driver > + * associated with it > + */ > + if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER) > + return 0; > + > + return 0; > +}; > + > +struct bus_type mhi_ep_bus_type = { > + .name = "mhi_ep", > + .dev_name = "mhi_ep", > + .match = mhi_ep_match, > +}; > + > +static int __init mhi_ep_init(void) > +{ > + return bus_register(&mhi_ep_bus_type); > +} > + > +static void __exit mhi_ep_exit(void) > +{ > + bus_unregister(&mhi_ep_bus_type); > +} > + > +postcore_initcall(mhi_ep_init); > +module_exit(mhi_ep_exit); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("MHI Bus Endpoint stack"); > +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>"); > 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 > @@ -0,0 +1,143 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2021, Linaro Ltd. > + * > + */ > +#ifndef _MHI_EP_H_ > +#define _MHI_EP_H_ > + > +#include <linux/dma-direction.h> > +#include <linux/mhi.h> > + > +#define MHI_EP_DEFAULT_MTU 0x8000 > + > +/** > + * struct mhi_ep_channel_config - Channel configuration structure for controller > + * @name: The name of this channel > + * @num: The number assigned to this channel > + * @num_elements: The number of elements that can be queued to this channel > + * @dir: Direction that data may flow on this channel > + */ > +struct mhi_ep_channel_config { > + char *name; > + u32 num; > + u32 num_elements; > + enum dma_data_direction dir; > +}; > + > +/** > + * struct mhi_ep_cntrl_config - MHI Endpoint controller configuration > + * @max_channels: Maximum number of channels supported > + * @num_channels: Number of channels defined in @ch_cfg > + * @ch_cfg: Array of defined channels > + * @mhi_version: MHI spec version supported by the controller > + */ > +struct mhi_ep_cntrl_config { > + u32 max_channels; > + u32 num_channels; > + const struct mhi_ep_channel_config *ch_cfg; > + u32 mhi_version;
Put mhi_version first?
> +}; > + > +/** > + * struct mhi_ep_db_info - MHI Endpoint doorbell info > + * @mask: Mask of the doorbell interrupt > + * @status: Status of the doorbell interrupt > + */ > +struct mhi_ep_db_info { > + u32 mask; > + u32 status; > +}; > + > +/** > + * struct mhi_ep_cntrl - MHI Endpoint controller structure > + * @cntrl_dev: Pointer to the struct device of physical bus acting as the MHI > + * Endpoint controller > + * @mhi_dev: MHI Endpoint device instance for the controller > + * @mmio: MMIO region containing the MHI registers > + * @mhi_chan: Points to the channel configuration table > + * @mhi_event: Points to the event ring configurations table > + * @mhi_cmd: Points to the command ring configurations table > + * @sm: MHI Endpoint state machine > + * @raise_irq: CB function for raising IRQ to the host > + * @alloc_addr: CB function for allocating memory in endpoint for storing host context > + * @map_addr: CB function for mapping host context to endpoint > + * @free_addr: CB function to free the allocated memory in endpoint for storing host context > + * @unmap_addr: CB function to unmap the host context in endpoint > + * @read_from_host: CB function for reading from host memory from endpoint > + * @write_to_host: CB function for writing to host memory from endpoint > + * @mhi_state: MHI Endpoint state > + * @max_chan: Maximum channels supported by the endpoint controller > + * @mru: MRU (Maximum Receive Unit) value of the endpoint controller > + * @index: MHI Endpoint controller index > + */ > +struct mhi_ep_cntrl { > + struct device *cntrl_dev; > + struct mhi_ep_device *mhi_dev; > + void __iomem *mmio; > + > + struct mhi_ep_chan *mhi_chan; > + struct mhi_ep_event *mhi_event; > + struct mhi_ep_cmd *mhi_cmd; > + struct mhi_ep_sm *sm; > + > + void (*raise_irq)(struct mhi_ep_cntrl *mhi_cntrl, u32 vector); > + void __iomem *(*alloc_addr)(struct mhi_ep_cntrl *mhi_cntrl, phys_addr_t *phys_addr, > + size_t size); > + int (*map_addr)(struct mhi_ep_cntrl *mhi_cntrl, phys_addr_t phys_addr, u64 pci_addr, > + size_t size); > + void (*free_addr)(struct mhi_ep_cntrl *mhi_cntrl, phys_addr_t phys_addr, > + void __iomem *virt_addr, size_t size); > + void (*unmap_addr)(struct mhi_ep_cntrl *mhi_cntrl, phys_addr_t phys_addr); > + int (*read_from_host)(struct mhi_ep_cntrl *mhi_cntrl, u64 from, void __iomem *to, > + size_t size); > + int (*write_to_host)(struct mhi_ep_cntrl *mhi_cntrl, void __iomem *from, u64 to, > + size_t size); > + > + enum mhi_state mhi_state; > + > + u32 max_chan; > + u32 mru; > + int index;
Will index ever be negative?
> +}; > + > +/** > + * struct mhi_ep_device - Structure representing an MHI Endpoint device that binds > + * to channels or is associated with controllers > + * @dev: Driver model device node for the MHI Endpoint device > + * @mhi_cntrl: Controller the device belongs to > + * @id: Pointer to MHI Endpoint device ID struct > + * @name: Name of the associated MHI Endpoint device > + * @ul_chan: UL channel for the device > + * @dl_chan: DL channel for the device > + * @dev_type: MHI device type > + */ > +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;
> +}; > + > +#define to_mhi_ep_device(dev) container_of(dev, struct mhi_ep_device, dev) > + > +/** > + * mhi_ep_register_controller - Register MHI Endpoint controller > + * @mhi_cntrl: MHI Endpoint controller to register > + * @config: Configuration to use for the controller > + * > + * Return: 0 if controller registrations succeeds, a negative error code otherwise. > + */ > +int mhi_ep_register_controller(struct mhi_ep_cntrl *mhi_cntrl, > + const struct mhi_ep_cntrl_config *config); > + > +/** > + * mhi_ep_unregister_controller - Unregister MHI Endpoint controller > + * @mhi_cntrl: MHI Endpoint controller to unregister > + */ > +void mhi_ep_unregister_controller(struct mhi_ep_cntrl *mhi_cntrl); > + > +#endif
| |