Message-ID: <073b09d4-51fe-f5fc-bbaf-1a168eac0881@codeaurora.org>
Date:   Thu, 6 Feb 2020 10:08:00 -0700
From:   Jeffrey Hugo <jhugo@...eaurora.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        gregkh@...uxfoundation.org, arnd@...db.de
Cc:     smohanad@...eaurora.org, kvalo@...eaurora.org,
        bjorn.andersson@...aro.org, hemantk@...eaurora.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 02/16] bus: mhi: core: Add support for registering MHI
 controllers

On 1/31/2020 6:49 AM, Manivannan Sadhasivam wrote:
> This commit adds support for registering MHI controller drivers with
> the MHI stack. MHI controller drivers manages the interaction with the
> MHI client devices such as the external modems and WiFi chipsets. They
> are also the MHI bus master in charge of managing the physical link
> between the host and client device.
> 
> This is based on the patch submitted by Sujeev Dias:
> https://lkml.org/lkml/2018/7/9/987
> 
> Signed-off-by: Sujeev Dias <sdias@...eaurora.org>
> Signed-off-by: Siddartha Mohanadoss <smohanad@...eaurora.org>
> [jhugo: added static config for controllers and fixed several bugs]
> Signed-off-by: Jeffrey Hugo <jhugo@...eaurora.org>
> [mani: removed DT dependency, splitted and cleaned up for upstream]
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>

> +/**
> + * enum mhi_device_type - Device types
> + * @MHI_DEVICE_XFER: Handles data transfer
> + * @MHI_DEVICE_TIMESYNC: Use for timesync feature
> + * @MHI_DEVICE_CONTROLLER: Control device
> + */
> +enum mhi_device_type {
> +	MHI_DEVICE_XFER,
> +	MHI_DEVICE_TIMESYNC,
> +	MHI_DEVICE_CONTROLLER,
> +};

Time sync support was removed, no?  I don't see MHI_DEVICE_TIMESYNC used 
anywhere in the code.

> +/**
> + * enum mhi_er_data_type - Event ring data types
> + * @MHI_ER_DATA: Only client data over this ring
> + * @MHI_ER_CTRL: MHI control data and client data
> + * @MHI_ER_TSYNC: Time sync events
> + */
> +enum mhi_er_data_type {
> +	MHI_ER_DATA,
> +	MHI_ER_CTRL,
> +	MHI_ER_TSYNC,
> +};

Time sync support was removed, no?  I don't see MHI_ER_TSYNC used 
anywhere in the code.

> +/**
> + * struct mhi_controller - Master MHI controller structure
> + * @dev: Driver model device node for the controller (required)
> + * @mhi_dev: MHI device instance for the controller
> + * @regs: Base address of MHI MMIO register space (required)
> + * @iova_start: IOMMU starting address for data (required)
> + * @iova_stop: IOMMU stop address for data (required)
> + * @fw_image: Firmware image name for normal booting (required)
> + * @edl_image: Firmware image name for emergency download mode (optional)
> + * @sbl_size: SBL image size downloaded through BHIe (optional)
> + * @seg_len: BHIe vector size (optional)
> + * @mhi_chan: Points to the channel configuration table
> + * @lpm_chans: List of channels that require LPM notifications
> + * @irq: base irq # to request (required)
> + * @max_chan: Maximum number of channels the controller supports
> + * @total_ev_rings: Total # of event rings allocated
> + * @hw_ev_rings: Number of hardware event rings
> + * @sw_ev_rings: Number of software event rings
> + * @nr_irqs_req: Number of IRQs required to operate (optional)
> + * @nr_irqs: Number of IRQ allocated by bus master (required)
> + * @mhi_event: MHI event ring configurations table
> + * @mhi_cmd: MHI command ring configurations table
> + * @mhi_ctxt: MHI device context, shared memory between host and device
> + * @pm_mutex: Mutex for suspend/resume operation
> + * @pm_lock: Lock for protecting MHI power management state
> + * @timeout_ms: Timeout in ms for state transitions
> + * @pm_state: MHI power management state
> + * @db_access: DB access states
> + * @ee: MHI device execution environment
> + * @dev_wake: Device wakeup count
> + * @pending_pkts: Pending packets for the controller
> + * @transition_list: List of MHI state transitions
> + * @transition_lock: Lock for protecting MHI state transition list
> + * @wlock: Lock for protecting device wakeup
> + * @st_worker: State transition worker
> + * @fw_worker: Firmware download worker
> + * @syserr_worker: System error worker
> + * @state_event: State change event
> + * @status_cb: CB function to notify power states of the device (required)
> + * @link_status: CB function to query link status of the device (required)
> + * @wake_get: CB function to assert device wake (optional)
> + * @wake_put: CB function to de-assert device wake (optional)
> + * @wake_toggle: CB function to assert and de-assert device wake (optional)
> + * @runtime_get: CB function to controller runtime resume (required)
> + * @runtimet_put: CB function to decrement pm usage (required)
> + * @buffer_len: Bounce buffer length
> + * @bounce_buf: Use of bounce buffer
> + * @fbc_download: MHI host needs to do complete image transfer (optional)
> + * @pre_init: MHI host needs to do pre-initialization before power up
> + * @wake_set: Device wakeup set flag
> + */

I'm happy the optional/required is listed.  However if I look at this 
from the perspective of someone writing a controller for the first time, 
I'm not confident the required/optional annotations are clear enough. 
Perhaps a quick sentance explaining that those annotations indicate 
fields which must be populated prior to mhi_register_controller() ?

> +
> +/**
> + * struct mhi_device - Structure representing a MHI device which binds
> + *                     to channels
> + * @id: Pointer to MHI device ID struct
> + * @chan_name: Name of the channel to which the device binds
> + * @mhi_cntrl: Controller the device belongs to
> + * @ul_chan: UL channel for the device
> + * @dl_chan: DL channel for the device
> + * @dev: Driver model device node for the MHI device
> + * @dev_type: MHI device type
> + * @tiocm: Device current terminal settings
> + * @dev_wake: Device wakeup counter
> + */
> +struct mhi_device {
> +	const struct mhi_device_id *id;
> +	const char *chan_name;
> +	struct mhi_controller *mhi_cntrl;
> +	struct mhi_chan *ul_chan;
> +	struct mhi_chan *dl_chan;
> +	struct device dev;
> +	enum mhi_device_type dev_type;
> +	u32 tiocm;

This was for the old ioctl support, right?  I don't see it used anywhere.

> +	u32 dev_wake;
> +};


-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Powered by blists - more mailing lists