lkml.org 
[lkml]   [2018]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 1/4] mhi_bus: core: Add support for MHI host interface
From
Date


On 04/27/2018 05:18 AM, Arnd Bergmann wrote:
> 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
Originally I did thought about it but I did not see any value of doing
that. Not only that, I would need
the parent device for all memory allocation and mapping since children
device doesn't inherit parent
iommu settings.
>> + /* 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.
From MHI bus point of view, this is a memory mapped device. Not
technically tied to a
pci device. I would like to keep that abstraction.
>> + /* 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?
This is for clients, they are absolutely not pcie aware at all. To them
this is just
a unique way to identify the physical 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thank you
Sujeev

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

\
 
 \ /
  Last update: 2018-04-28 18:09    [W:0.116 / U:0.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site