lkml.org 
[lkml]   [2022]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 05/20] bus: mhi: ep: Add support for registering MHI endpoint controllers
On 12/2/21 5:35 AM, Manivannan Sadhasivam wrote:
> This commit adds support for registering MHI endpoint controller drivers
> with the MHI endpoint stack. MHI endpoint controller drivers manages
> the interaction with the host machines such as x86. They are also the
> 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
> parsing the channel configuration and allocating an MHI EP device.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

See below. Lots of little things, some I've said before.

> ---
> 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 | 158 +++++++++++++++++++++++
> drivers/bus/mhi/ep/main.c | 231 ++++++++++++++++++++++++++++++++++
> include/linux/mhi_ep.h | 140 +++++++++++++++++++++
> 7 files changed, 545 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
> + and communicate with modem devices over a high speed peripheral
> + 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.
> 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..7b164daf4332
> --- /dev/null
> +++ b/drivers/bus/mhi/ep/internal.h
> @@ -0,0 +1,158 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2021, Linaro Ltd.
> + *
> + */
> +
> +#ifndef _MHI_EP_INTERNAL_
> +#define _MHI_EP_INTERNAL_
> +
> +#include <linux/bitfield.h>
> +
> +#include "../common.h"
> +
> +extern struct bus_type mhi_ep_bus_type;
> +
> +/* MHI register definitions */
> +#define MHIREGLEN 0x100

I really think it would be nice if these could be common between the
host and endpoint.

> +#define MHIVER 0x108
> +#define MHICFG 0x110
> +#define CHDBOFF 0x118
> +#define ERDBOFF 0x120
> +#define BHIOFF 0x128
> +#define DEBUGOFF 0x130
> +#define MHICTRL 0x138
> +#define MHISTATUS 0x148
> +#define CCABAP_LOWER 0x158
> +#define CCABAP_HIGHER 0x15c
> +#define ECABAP_LOWER 0x160
> +#define ECABAP_HIGHER 0x164
> +#define CRCBAP_LOWER 0x168
> +#define CRCBAP_HIGHER 0x16c
> +#define CRDB_LOWER 0x170
> +#define CRDB_HIGHER 0x174
> +#define MHICTRLBASE_LOWER 0x180
> +#define MHICTRLBASE_HIGHER 0x184
> +#define MHICTRLLIMIT_LOWER 0x188
> +#define MHICTRLLIMIT_HIGHER 0x18c
> +#define MHIDATABASE_LOWER 0x198
> +#define MHIDATABASE_HIGHER 0x19c
> +#define MHIDATALIMIT_LOWER 0x1a0
> +#define MHIDATALIMIT_HIGHER 0x1a4

It wouldn't hurt to have a one or two line comment explaining how
these compute the offset for a given channel or event ring's
doorbell register.

I think you could use decimal for the multiplier (8 rather than 0x8),
but maybe you prefer not mixing that with a hex base offset.

Overall though, take a look at the macros you define like this.
See if you can decide on whether you can settle on a consistent
form. Some places you use decimal, others hex. It's not a
big deal, but consistency always helps.

> +#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 BHI_INTVEC 0x220
> +#define BHI_EXECENV 0x228
> +#define BHI_IMGTXDB 0x218
> +

Will the AP always be an "A7"?

> +#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)
> +

The term "MASK" here might be confusing. Does a bit set in
this mask register indicate an interrupt is enabled, or
disabled (masked)? A comment (here or where used) could
clear it up without renaming the symbol.

> +#define MHI_CTRL_INT_MASK_A7 0x94
> +#define MHI_CTRL_INT_MASK_A7_MASK_MASK GENMASK(1, 0)
> +#define MHI_CTRL_MHICTRL_MASK BIT(0)
> +#define MHI_CTRL_MHICTRL_SHFT 0
> +#define MHI_CTRL_CRDB_MASK BIT(1)
> +#define MHI_CTRL_CRDB_SHFT 1

Use SHIFT or SHFT (not both), consistently. (But get rid of
this shift definition, and others like it...)

> +#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

Is there ever any reason to believe there will be more than one
command ring for a given MHI instance? I kept seeing loops over
NR_OF_CMD_RINGS, and it just seemed silly.

> +#define MHI_MASK_ROWS_CH_EV_DB 4
> +#define MHI_MASK_CH_EV_LEN 32
> +
> +/* Generic context */

Maybe define the entire structure as packed and aligned.

> +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);
> +};

Are these structures defined separately for host and endpoint?
(I've lost track... If they are, it would be better to define
them in common.)

> +
> +enum mhi_ep_ring_state {
> + RING_STATE_UINT = 0,

I think "UINT" is a *terrible* abbreviation to represent
"uninitialized".

> + RING_STATE_IDLE,

Since there are only two states, uninitialized or idle, maybe
you can get rid of this enum definition and just define the
ring state with "bool initialized".

> +};
> +
> +enum mhi_ep_ring_type {

Is the value 0 significant to hardware? If not, there's no need
to define the numeric value on this first symbol.

> + RING_TYPE_CMD = 0,
> + RING_TYPE_ER,
> + RING_TYPE_CH,

I don't think you ever use RING_TYPE_INVALID, so it does
not need to be defined.

> + RING_TYPE_INVALID,
> +};
> +

I prefer a more meaningful structure definition than this (as
mentioned in I think the first patch).

> +struct mhi_ep_ring_element {
> + u64 ptr;
> + u32 dword[2];
> +};
> +
> +/* Transfer ring element type */

Not transfer ring, just ring. Command, transfer, and event
ring descriptors are different things.

> +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 list_head list;
> + 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;
> + enum mhi_ep_ring_state state;
> + size_t rd_offset;
> + size_t wr_offset;
> + size_t ring_size;
> + u32 db_offset_h;
> + u32 db_offset_l;
> + u32 ch_id;
> +};
> +
> +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..db664360c8ab
> --- /dev/null
> +++ b/drivers/bus/mhi/ep/main.c
> @@ -0,0 +1,231 @@
> +// 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);
> +
> + /*
> + * We need to set the mhi_chan->mhi_dev to NULL here since the MHI
> + * devices for the channels will only get created if the mhi_dev
> + * associated with it is NULL.

Maybe say where in the code what the comment above says happens.

> + */
> + 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)
> +{
> + 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;
> +

I think you should pass the MHI device type as argument here, and
set it within this function. Then use it in the test below, rather
than assuming the mhi_dev pointer will be NULL for the controller
only. Maybe you should set the mhi_dev pointer here as well.

> + if (mhi_cntrl->mhi_dev) {
> + /* for MHI client devices, parent is the MHI controller device */
> + dev->parent = &mhi_cntrl->mhi_dev->dev;
> + } else {
> + /* for MHI controller device, parent is the bus device (e.g. PCI EPF) */
> + dev->parent = mhi_cntrl->cntrl_dev;
> + }
> +
> + mhi_dev->mhi_cntrl = mhi_cntrl;
> +
> + return mhi_dev;
> +}
> +
> +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;
> +
> + ch_cfg = &config->ch_cfg[i];
> +
> + chan = ch_cfg->num;
> + if (chan >= mhi_cntrl->max_chan) {
> + dev_err(dev, "Channel %d not available\n", chan);
> + 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);

Move the error check below earlier, before assigning other values.

> + /* Bi-directional and direction less channels are not supported */
> + if (mhi_chan->dir == DMA_BIDIRECTIONAL || mhi_chan->dir == DMA_NONE) {
> + dev_err(dev, "Invalid channel configuration\n");
> + goto error_chan_cfg;
> + }
> + }
> +
> + return 0;
> +
> +error_chan_cfg:
> + kfree(mhi_cntrl->mhi_chan);
> +
> + 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;

Perhaps you could use a convention like "ep_dev" (and later, "ep_drv")
to represent an mhi_ep_device, different from "mhi_dev" representing
an mhi_device.

> + int ret;
> +
> + if (!mhi_cntrl || !mhi_cntrl->cntrl_dev)
> + return -EINVAL;
> +
> + ret = parse_ch_cfg(mhi_cntrl, config);
> + if (ret)
> + return ret;
> +

NR_OF_CMD_RINGS is 1, and I think always will be, right? This and
elsewhere could be simplified if we just accept that.

> + mhi_cntrl->mhi_cmd = kcalloc(NR_OF_CMD_RINGS, sizeof(*mhi_cntrl->mhi_cmd), GFP_KERNEL);
> + 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);
> + 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;
> + }
> +
> + mhi_dev->dev_type = MHI_DEVICE_CONTROLLER;
> + 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_release_dev;

goto err_put_device?

> +
> + mhi_cntrl->mhi_dev = mhi_dev;
> +
> + dev_dbg(&mhi_dev->dev, "MHI EP Controller registered\n");
> +
> + return 0;
> +
> +err_release_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..14fd40af8974
> --- /dev/null
> +++ b/include/linux/mhi_ep.h

. . .

> +/**
> + * 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
> + * @ul_chan_id: Channel id for UL transfer
> + * @dl_chan_id: Channel id for DL transfer
> + */
> +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;

Could the dev_type just be: bool controller?

> + enum mhi_device_type dev_type;
> + int ul_chan_id;

Can't you ust use ul_chan->chan and dl_chan->chan?

In any case, I think the channel ids should be u32.

-Alex

> + int dl_chan_id;
> +};
> +
> +#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
>

\
 
 \ /
  Last update: 2022-01-06 01:27    [W:0.246 / U:0.716 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site