[v3,08/25] bus: mhi: ep: Add support for registering MHI endpoint controllers

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

Hi Mani,

On 2/12/2022 10:21 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>
> ---
>   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
> +	  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..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.
> + *
> + */
> +
> +#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
> +
> +/* 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
can we get rid of all instances of "_A7" as this corresponds to 
Cortex-A7, in future this can change? At MHI core layer, we can avoid 
this naming convetion, even though register names are inculding them now 
and may change to something different later. This MHI EP driver would 
still be used for those new cortex vers.
> +#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))
> +
[..]

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

On Mon, Feb 14, 2022 at 05:04:23PM -0800, Hemant Kumar wrote:
> Hi Mani,
> 
> On 2/12/2022 10:21 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>
> > ---
> >   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
> > 

[...]

> > +#define MHI_CTRL_INT_STATUS_A7			0x4
> can we get rid of all instances of "_A7" as this corresponds to Cortex-A7,
> in future this can change? At MHI core layer, we can avoid this naming
> convetion, even though register names are inculding them now and may change
> to something different later. This MHI EP driver would still be used for
> those new cortex vers.

Since these registers are not documented by the spec, I just went with
the register definition available for SDX55. But you are right, and Alex
too, that it may change in future.

I'll remove the A7 suffix.

Thanks,
Mani

> > +#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))
> > +
> [..]
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a
> Linux Foundation Collaborative Project

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

On 2/17/22 3:53 AM, Manivannan Sadhasivam wrote:
> On Tue, Feb 15, 2022 at 02:02:41PM -0600, Alex Elder wrote:
> 
> [...]
> 
>>> +#define MHI_REG_OFFSET				0x100
>>> +#define BHI_REG_OFFSET				0x200

. . .

> [...]
> 
>>> +/* 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...

Despite suggesting this more than once, I'm not 100% sure it's
even a correct suggestion.  I trust Arnd's judgement, and I
can see the value of being explicit about *which* fields have
the alignment requirement.  So I'll leave it up to you to
decide...  If you make my suggested change, be sure to test
it.  But I'm fine if you leave these as-is.

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

OK, good.

> 
>>> +
> 
> [...]
> 
>>> +	/*
>>> +	 * 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.

Yes you're right, I missed that.

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

OK.

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

OK.

> [...]
> 
>>> 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.

Sounds good.  Thanks.

					-Alex

> Thanks,
> Mani

On 2/17/2022 2:53 AM, Manivannan Sadhasivam wrote:
> 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.
> 

While you are making changes, maybe don't have a set BHI_REG_OFFSET? 
Sure, I think it is always 0x200, but that is a convention and nothing 
I've seen in the spec mandates it.  You can derive it from the bhi 
offset register.

This way, if it ever moves in some future chip, this code should just work.