Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Jonathan NeuschÃfer]

Hi, I have some more or less trivial comments below.

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandagatla@xxxxxxxxxx wrote:
> From: Sagar Dharia <sdharia@xxxxxxxxxxxxxx>
> 
> SLIMbus (Serial Low Power Interchip Media Bus) is a specification
> developed by MIPI (Mobile Industry Processor Interface) alliance.
> SLIMbus is a 2-wire implementation, which is used to communicate with
> peripheral components like audio-codec.
> SLIMbus uses Time-Division-Multiplexing to accommodate multiple data
> channels, and control channel. Control channel has messages to do
> device-enumeration, messages to send/receive control-data to/from
> slimbus devices, messages for port/channel management, and messages to
> do bandwidth allocation.
> The framework supports multiple instances of the bus (1 controller per
> bus), and multiple slave devices per controller.
> 
> This patch does device enumeration, logical address assignment,
> informing device when the device reports present/absent etc.
> Reporting present may need the driver to do the needful (e.g. turning
> on voltage regulators powering the device). Additionally device is
> probed when it reports present if that device doesn't need any such
> steps mentioned above.
> 
> Signed-off-by: Sagar Dharia <sdharia@xxxxxxxxxxxxxx>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> ---
[...]
> +SLIMbus example for Qualcomm's slimbus manager component:
> +
> +	slim@28080000 {
> +		compatible = "qcom,slim-msm";
> +		reg = <0x28080000 0x2000>,
> +		interrupts = <0 33 0>;
> +		clocks = <&lcc SLIMBUS_SRC>, <&lcc AUDIO_SLIMBUS_CLK>;
> +		clock-names = "iface_clk", "core_clk";
> +		#address-cells = <2>;
> +		#size-cells = <0>;
> +
> +		codec: wcd9310@1{
> +			compatible = "slim217,60"";
			                        ^ spurious quote?

> +			reg = <1 0>;
> +		};
> +	};
> diff --git a/Documentation/slimbus/summary b/Documentation/slimbus/summary
> new file mode 100644
> index 0000000..e7f90bb
> --- /dev/null
> +++ b/Documentation/slimbus/summary

Should this file have a .rst extension, like other Restructured Text
files?

> @@ -0,0 +1,109 @@
> +Overview of Linux kernel SLIMbus support
> +========================================
[...]
> +Device notifications to the driver:
> +-----------------------------------
> +Since SLIMbus devices have mechanisms for reporting their presence, the
> +framework allows drivers to bind when corresponding devices report their
> +presence on the bus.
> +However, it is possible that the driver needs to be probed
> +first so that it can enable corresponding SLIMbus devie (e.g. power it up and/or

s/devie/device/ I guess

> +take it out of reset). To support that behavior, the framework allows drivers
> +to probe first as well  (e.g. using standard DeviceTree compatbility field).
> +This creates the necessity for the driver to know when the device is functional
> +(i.e. reported present). device_up callback is used for that reason when the
> +device reports present and is assigned a logical address by the controller.
[...]
> +/**
> + * struct slim_addrt: slimbus address used internally by the slimbus framework.
> + * @valid: If the device is present. Valid is set to false when device reports
> + *	absent.
> + * @eaddr: Enumeration address
> + * @laddr: It is possible that controller will set a predefined logical address
> + *	rather than the one assigned by framework. (i.e. logical address may
> + *	not be same as index into this table). This entry will store the
> + *	logical address value for this enumeration address.
> + */
> +struct slim_addrt {
> +	bool			valid;
> +	struct slim_eaddr	eaddr;
> +	u8			laddr;
> +};

I wonder if valid should be moved after eaddr, to reduce the need for
padding. AFAICS, struct slim_eaddr is 6 bytes long and requires 2-byte
alignment, so if valid is one byte long, there would be one byte of
padding after it, slightly bloating struct slim_addrt, unnecessarily.

> +/**
> + * struct slim_controller: Controls every instance of SLIMbus
> + *				(similar to 'master' on SPI)
> + *	'Manager device' is responsible for  device management, bandwidth
> + *	allocation, channel setup, and port associations per channel.
> + *	Device management means Logical address assignment/removal based on
> + *	enumeration (report-present, report-absent) if a device.

s/if a device/of a device/ ?

> + *	Bandwidth allocation is done dynamically by the manager based on active
> + *	channels on the bus, message-bandwidth requests made by slimbus devices.
> + *	Based on current bandwidth usage, manager chooses a frequency to run
> + *	the bus at (in steps of 'clock-gear', 1 through 10, each clock gear
> + *	representing twice the frequency than the previous gear).
> + *	Manager is also responsible for entering (and exiting) low-power-mode
> + *	(known as 'clock pause').
> + *	Manager can do handover of framer if there are multiple framers on the
> + *	bus and a certain usecase warrants using certain framer to avoid keeping
> + *	previous framer being powered-on.
> + *
> + *	Controller here performs duties of the manager device, and 'interface
> + *	device'. Interface device is responsible for monitoring the bus and
> + *	reporting information such as loss-of-synchronization, data
> + *	slot-collision.
> + * @dev: Device interface to this driver
> + * @nr: Board-specific number identifier for this controller/bus
> + * @list: Link with other slimbus controllers

I don't see list in the struct.

> + * @name: Name for this controller
> + * @min_cg: Minimum clock gear supported by this controller (default value: 1)
> + * @max_cg: Maximum clock gear supported by this controller (default value: 10)
> + * @clkgear: Current clock gear in which this bus is running
> + * @a_framer: Active framer which is clocking the bus managed by this controller
> + * @m_ctrl: Mutex protecting controller data structures
> + * @addrt: Logical address table
> + * @num_dev: Number of active slimbus slaves on this bus
> + * @wq: Workqueue per controller used to notify devices when they report present
> + * @xfer_msg: Transfer a message on this controller (this can be a broadcast
> + *	control/status message like data channel setup, or a unicast message
> + *	like value element read/write.

I don't see xfer_msg in the struct.

> + * @set_laddr: Setup logical address at laddr for the slave with elemental
> + *	address e_addr. Drivers implementing controller will be expected to
> + *	send unicast message to this device with its logical address.
> + * @get_laddr: It is possible that controller needs to set fixed logical
> + *	address table and get_laddr can be used in that case so that controller
> + *	can do this assignment.
> + */
> +struct slim_controller {
> +	struct device		dev;
> +	unsigned int		nr;
> +	char			name[SLIMBUS_NAME_SIZE];
> +	int			min_cg;
> +	int			max_cg;
> +	int			clkgear;
> +	struct slim_framer	*a_framer;
> +	struct mutex		m_ctrl;
> +	struct slim_addrt	*addrt;
> +	u8			num_dev;
> +	struct workqueue_struct *wq;
> +	int			(*set_laddr)(struct slim_controller *ctrl,
> +					     struct slim_eaddr *ea, u8 laddr);
> +	int			(*get_laddr)(struct slim_controller *ctrl,
> +					     struct slim_eaddr *ea, u8 *laddr);
> +};


Thanks,
Jonathan NeuschÃfer

Attachment: signature.asc
Description: PGP signature