LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Arnd Bergmann <arnd@arndb.de>
To: Sujeev Dias <sdias@codeaurora.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-arm-msm@vger.kernel.org,
	Tony Truong <truong@codeaurora.org>
Subject: Re: [PATCH v1 1/4] mhi_bus: core: Add support for MHI host interface
Date: Fri, 27 Apr 2018 14:18:56 +0200
Message-ID: <CAK8P3a0wRwRDw86Ex7Ynn+nnnHJ6oEZFfrtrqGtOhJNpTfrTNQ@mail.gmail.com> (raw)
In-Reply-To: <1524795811-21399-2-git-send-email-sdias@codeaurora.org>

On Fri, Apr 27, 2018 at 4:23 AM, Sujeev Dias <sdias@codeaurora.org> wrote:

> diff --git a/Documentation/devicetree/bindings/bus/mhi.txt b/Documentation/devicetree/bindings/bus/mhi.txt
> new file mode 100644
> index 0000000..ea1b620
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/mhi.txt
> @@ -0,0 +1,141 @@
> +MHI Host Interface
> +
> +MHI used by the host to control and communicate with modem over
> +high speed peripheral bus.
> +
> +==============
> +Node Structure
> +==============
> +
> +Main node properties:
> +
> +- mhi,max-channels
> +  Usage: required
> +  Value type: <u32>
> +  Definition: Maximum number of channels supported by this controller
> +
> +- mhi,chan-cfg
> +  Usage: required
> +  Value type: Array of <u32>
> +  Definition: Array of tuples describe channel configuration.
> +       1st element: Physical channel number
> +       2nd element: Transfer ring length in elements
> +       3rd element: Event ring associated with this channel
> +       4th element: Channel direction as defined by enum dma_data_direction
> +               1 = UL data transfer
> +               2 = DL data transfer
> +       5th element: Channel doorbell mode configuration as defined by
> +       enum MHI_BRSTMODE
> +               2 = burst mode disabled
> +               3 = burst mode enabled
> +       6th element: mhi doorbell configuration, valid only when burst mode
> +       enabled.
> +               0 = Use default (device specific) polling configuration
> +               For UL channels, value specifies the timer to poll MHI context
> +               in milliseconds.
> +               For DL channels, the threshold to poll the MHI context
> +               in multiple of eight ring element.
> +       7th element: Channel execution enviornment as defined by enum MHI_EE
> +               1 = Bootloader stage
> +               2 = AMSS mode
> +       8th element: data transfer type accepted as defined by enum
> +       MHI_XFER_TYPE
> +               0 = accept cpu address for buffer
> +               1 = accept skb
> +               2 = accept scatterlist
> +               3 = offload channel, does not accept any transfer type
> +       9th element: Bitwise configuration settings for the channel
> +               Bit mask:
> +               BIT(0) : LPM notify, this channel master requre lpm enter/exit
> +               notifications.
> +               BIT(1) : Offload channel, MHI host only involved in setting up
> +               the data pipe. Not involved in active data transfer.
> +               BIT(2) : Must switch to doorbell mode whenever MHI M0 state
> +               transition happens.
> +               BIT(3) : MHI bus driver pre-allocate buffer for this channel.
> +               If set, clients not allowed to queue buffers. Valid only for DL
> +               direction.
> +
> +- mhi,chan-names
> +  Usage: required
> +  Value type: Array of <string>
> +  Definition: Channel names configured in mhi,chan-cfg.

I think the top-level structure would be better done by requiring
a child for each channel and moving the mhi,chan-cfg
into the children nodes, either as separate properties, or
some of the fields combined into a 'reg' property.

> +- mhi,fw-name
> +  Usage: optional
> +  Value type: <string>
> +  Definition: Firmware image name to upload

The device tree should have no knowledge of a firmware file name.

What you should do instead is to have the driver generate the file
name from the "compatible" property. You could use a lookup table
in the driver, or have some algorithmic way of doing that, but you
want the driver to have some form of intelligence to let it pick
a different firmware image, e.g. when you have added new
capabilities to the driver and the firmware but want to use an
existing device tree.

> +Data structures
> +---------------
> +Host memory : Directly accessed by the host to manage the MHI data structures
> +and buffers. The device accesses the host memory over the PCIe interface.
> +
> +Channel context array : All channel configurations are organized in channel
> +context data array.
> +
> +struct __packed mhi_chan_ctxt;
> +struct mhi_ctxt.chan_ctxt;
> +
> +Transfer rings : Used by host to schedule work items for a channel and organized
> +as a circular queue of transfer descriptors (TD).
> +
> +struct __packed mhi_tre;
> +struct mhi_chan.tre_ring;
> +

This looks like you reinvented virtio. What are the reasons for not just
using virtio directly as we do for other modem interfaces?

> +static int mhi_fw_load_sbl(struct mhi_controller *mhi_cntrl,
> +                          void *buf,
> +                          size_t size)
> +{
> +       u32 tx_status, val;
> +       int i, ret;
> +       void __iomem *base = mhi_cntrl->bhi;
> +       rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
> +       dma_addr_t phys = dma_map_single(mhi_cntrl->dev, buf, size,
> +                                        DMA_TO_DEVICE);

Please don't name a dma address 'phys', it's very confusing ;-)

> +struct mhi_bus mhi_bus;

One global instance? I suspec this duplicates data that is already
in the bus_type structure, so just use that instead.

> +void mhi_init_debugfs(struct mhi_controller *mhi_cntrl)
> +{
> +       struct dentry *dentry;
> +       char node[32];
> +
> +       if (!mhi_cntrl->parent)
> +               return;
> +
> +       snprintf(node, sizeof(node), "%04x_%02u:%02u.%02u",
> +                mhi_cntrl->dev_id, mhi_cntrl->domain, mhi_cntrl->bus,
> +                mhi_cntrl->slot);
> +
> +       dentry = debugfs_create_dir(node, mhi_cntrl->parent);
> +       if (IS_ERR_OR_NULL(dentry))
> +               return;

Using IS_ERR_OR_NULL() just means that you have misunderstood
the interface, you want IS_ERR() here.

> +#if defined(CONFIG_OF)
> +static int of_parse_ev_cfg(struct mhi_controller *mhi_cntrl,
> +                          struct device_node *of_node)

Why the #ifdef? Do you ever build this on architectures that don't support
CONFIG_OF yet? Which ones?

> +struct __packed mhi_event_ctxt {
> +       u32 reserved : 8;
> +       u32 intmodc : 8;
> +       u32 intmodt : 16;
> +       u32 ertype;
> +       u32 msivec;
> +       u64 rbase;
> +       u64 rlen;
> +       u64 rp;
> +       u64 wp;
> +};

I would generally only mark those members as __packed that
don't already have natural alignment, such as

struct mhi_event_ctxt {
       u32 reserved : 8;
       u32 intmodc : 8;
       u32 intmodt : 16;
       u32 ertype;
       u32 msivec;
       u64 rbase __packed __aligned(4);
       u64 rlen__packed __aligned(4);
       u64 rp __packed __aligned(4);
       u64 wp__packed __aligned(4);
};

This clarifies where the hardware designers screwed up
without forcing bytewise access to the structures.

> +
> +#ifdef CONFIG_MHI_DEBUG
> +
> +#define MHI_ASSERT(cond, msg) do { \
> +       if (cond) \
> +               panic(msg); \
> +} while (0)
> +
> +#else
> +
> +#define MHI_ASSERT(cond, msg) do { \
> +       if (cond) { \
> +               MHI_ERR(msg); \
> +               WARN_ON(cond); \
> +       } \
> +} while (0)

Remove these.

> +struct mhi_controller {
> +       struct list_head node;
> +
> +       /* device node for iommu ops */
> +       struct device *dev;
> +       struct device_node *of_node;

Hmm, shouldn't the mhi_controller itself be a device that
is a child of ->dev? It feels like

> +       /* mmio base */
> +       void __iomem *regs;
> +       void __iomem *bhi;
> +       void __iomem *wake_db;
> +
> +       /* device topology */
> +       u32 dev_id;
> +       u32 domain;
> +       u32 bus;
> +       u32 slot;

These are even stranger. This looks PCI specific, but if you have
a pci_dev then you know all of them while for the case you don't have
a pci_dev, they don't make sense.

> +       /* kernel log level */
> +       enum MHI_DEBUG_LEVEL klog_lvl;
> +
> +       /* private log level controller driver to set */
> +       enum MHI_DEBUG_LEVEL log_lvl;
> +
> +       /* controller specific data */
> +       void *priv_data;
> +       void *log_buf;
> +       struct dentry *dentry;
> +       struct dentry *parent;

All the debugfs stuff is a bit worrying.  It seems so pervasive in
the driver that it's hard to tell if any of it is actually used for more
than just debugging. It might be better to split that out of the
series and add back the debug files one at a time in separate
patches that each explain why the files are still required.
If you have successfully debugged a problem with one
file, it may have seemed useful but maybe it's not needed
for any future bugs.

You might also be able to get more value out of tracepoints
than debugfs files.

> +};
> +
> +/**
> + * struct mhi_device - mhi device structure associated bind to channel
> + * @dev: Device associated with the channels
> + * @mtu: Maximum # of bytes controller support
> + * @ul_chan_id: MHI channel id for UL transfer
> + * @dl_chan_id: MHI channel id for DL transfer
> + * @priv: Driver private data
> + */
> +struct mhi_device {
> +       struct device dev;
> +       u32 dev_id;
> +       u32 domain;
> +       u32 bus;
> +       u32 slot;

Again, shouldn't the pci stuff be part of the parent or grandparent
device?

> +
> +#if defined(CONFIG_MHI_BUS)

In what situation would you include this header without enabling
CONFIG_MHI_BUS?

> +
> +#ifdef CONFIG_MHI_DEBUG
> +
> +#define MHI_VERB(fmt, ...) do { \
> +               if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_VERBOSE) \
> +                       pr_debug("[D][%s] " fmt, __func__, ##__VA_ARGS__);\
> +} while (0)
> +
> +#else
> +
> +#define MHI_VERB(fmt, ...)
> +
> +#endif
> +
> +#define MHI_LOG(fmt, ...) do { \
> +               if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_INFO) \
> +                       pr_info("[I][%s] " fmt, __func__, ##__VA_ARGS__);\
> +} while (0)
> +
> +#define MHI_ERR(fmt, ...) do { \
> +               if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_ERROR) \
> +                       pr_err("[E][%s] " fmt, __func__, ##__VA_ARGS__); \
> +} while (0)
> +
> +#define MHI_CRITICAL(fmt, ...) do { \
> +               if (mhi_cntrl->klog_lvl <= MHI_MSG_LVL_CRITICAL) \
> +                       pr_alert("[C][%s] " fmt, __func__, ##__VA_ARGS__); \
> +} while (0)
> +

Again, these all should be removed. Just use dev_err() etc.

> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 7d361be..1e11e30 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -734,4 +734,15 @@ struct tb_service_id {
>  #define TBSVC_MATCH_PROTOCOL_VERSION   0x0004
>  #define TBSVC_MATCH_PROTOCOL_REVISION  0x0008
>
> +
> +/**
> + * struct mhi_device_id - MHI device identification
> + * @chan: MHI channel name
> + * @driver_data: driver data;
> + */
> +struct mhi_device_id {
> +       const char *chan;
> +       kernel_ulong_t driver_data;
> +};

I think you have to use a fixed-length string here, otherwise
module autoloading cannot work.

      Arnd

  parent reply index

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-27  2:23 MHI initial design review Sujeev Dias
2018-04-27  2:23 ` [PATCH v1 1/4] mhi_bus: core: Add support for MHI host interface Sujeev Dias
2018-04-27  7:22   ` Greg Kroah-Hartman
2018-04-28 14:28     ` Sujeev Dias
2018-04-28 15:50       ` Greg Kroah-Hartman
2018-04-27  7:23   ` Greg Kroah-Hartman
2018-04-27 12:18   ` Arnd Bergmann [this message]
2018-04-28 16:08     ` Sujeev Dias
2018-04-28  0:28   ` kbuild test robot
2018-04-28  2:52   ` kbuild test robot
2018-05-03 19:21   ` Pavel Machek
2018-05-04  3:05     ` Sujeev Dias
2018-06-22 23:03   ` Randy Dunlap
2018-04-27  2:23 ` [PATCH v1 2/4] mhi_bus: controller: MHI support for QCOM modems Sujeev Dias
2018-04-27 11:32   ` Arnd Bergmann
2018-04-28 15:40     ` Sujeev Dias
2018-04-28  3:05   ` kbuild test robot
2018-04-28  3:12   ` kbuild test robot
2018-04-27  2:23 ` [PATCH v1 3/4] mhi_bus: dev: netdev: add network interface driver Sujeev Dias
2018-04-27 11:19   ` Arnd Bergmann
2018-04-28 15:25     ` Sujeev Dias
2018-04-27  2:23 ` [PATCH v1 4/4] mhi_bus: dev: uci: add user space " Sujeev Dias
2018-04-27 11:36   ` Arnd Bergmann
2018-04-28  1:03   ` kbuild test robot
2018-04-28  5:16   ` [PATCH] mhi_bus: dev: uci: fix semicolon.cocci warnings kbuild test robot
2018-04-28  5:16   ` [PATCH v1 4/4] mhi_bus: dev: uci: add user space interface driver kbuild test robot
2018-07-09 20:08 ` MHI code review Sujeev Dias
2018-07-09 20:08   ` [PATCH v2 1/7] mhi_bus: core: initial checkin for modem host interface bus driver Sujeev Dias
2018-07-09 20:50     ` Greg Kroah-Hartman
2018-07-09 20:52     ` Greg Kroah-Hartman
2018-07-10  6:36     ` Greg Kroah-Hartman
2018-07-11 19:30     ` Rob Herring
2018-08-09 18:39     ` Randy Dunlap
2018-07-09 20:08   ` [PATCH v2 2/7] mhi_bus: core: add power management support Sujeev Dias
2018-07-09 20:08   ` [PATCH v2 3/7] mhi_bus: core: add support for data transfer Sujeev Dias
2018-07-10  6:29     ` Greg Kroah-Hartman
2018-07-09 20:08   ` [PATCH v2 4/7] mhi_bus: core: add support for handling ioctl cmds Sujeev Dias
2018-07-09 20:08   ` [PATCH v2 5/7] mhi_bus: core: add support to get external modem time Sujeev Dias
2018-07-11 19:32     ` Rob Herring
2018-08-09 20:17     ` Randy Dunlap
2018-07-09 20:08   ` [PATCH v2 6/7] mhi_bus: controller: MHI support for QCOM modems Sujeev Dias
2018-07-11 19:36     ` Rob Herring
2018-07-09 20:08   ` [PATCH v2 7/7] mhi_bus: dev: uci: add user space interface driver Sujeev Dias
2019-04-30 15:10   ` MHI code review Daniele Palmas
2019-06-12 17:54     ` Sujeev Dias
2019-06-12 20:58       ` Daniele Palmas
2019-06-12 18:00     ` Sujeev Dias

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAK8P3a0wRwRDw86Ex7Ynn+nnnHJ6oEZFfrtrqGtOhJNpTfrTNQ@mail.gmail.com \
    --to=arnd@arndb.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sdias@codeaurora.org \
    --cc=truong@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox