Re: [Patch v6 1/7] slimbus: Device management on SLIMbus
From: Srinivas Kandagatla
Date: Sat Oct 07 2017 - 06:24:33 EST
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@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?
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