lkml.org 
[lkml]   [2017]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [Patch v6 1/7] slimbus: Device management on SLIMbus
From
Date
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@linaro.org wrote:
>> From: Sagar Dharia <sdharia@codeaurora.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@codeaurora.org>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
> [...]
>> +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
>

\
 
 \ /
  Last update: 2017-10-07 12:25    [W:0.190 / U:15.424 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site