Message-ID: <e9f3357a-3094-8c6d-7a8d-3353e8af192d@linaro.org>
Date:   Sat, 7 Oct 2017 11:24:18 +0100
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Jonathan Neuschäfer <j.neuschaefer@....net>
Cc:     gregkh@...uxfoundation.org, broonie@...nel.org,
        alsa-devel@...a-project.org, sdharia@...eaurora.org, bp@...e.de,
        poeschel@...onage.de, treding@...dia.com,
        gong.chen@...ux.intel.com, andreas.noever@...il.com,
        alan@...ux.intel.com, mathieu.poirier@...aro.org, daniel@...ll.ch,
        jkosina@...e.cz, sharon.dvir1@...l.huji.ac.il, joe@...ches.com,
        davem@...emloft.net, james.hogan@...tec.com,
        michael.opdenacker@...e-electrons.com, robh+dt@...nel.org,
        pawel.moll@....com, mark.rutland@....com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        kheitke@...ience.com, linux-arm-msm@...r.kernel.org, arnd@...db.de
Subject: Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

Thanks for the comments.

On 07/10/17 05:14, Jonathan Neuschäfer wrote:
> Hi, I have some more or less trivial comments below.
> 
> On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandagatla@...aro.org wrote:
>> From: Sagar Dharia <sdharia@...eaurora.org>
>>
>> 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@...eaurora.org>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>> ---
> [...]
>> +SLIMbus example for Qualcomm's slimbus manager component:
>> +
>> +	slim@...80000 {
>> +		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?

Will try to sort this out in next version.

> 
>> @@ -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.
Makes sense!!

> 
>> +/**
>> + * 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/ ?

Yep, will fix this in next version.
> 
>> + *	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.
I think its a left over of the cleanup.. will fix this in next version.
> 
>> + * @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.
Will add in next version.

> 
>> + * @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
> 

Powered by blists - more mailing lists