| Message ID | 20171115141043.29202-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | Introduce framework for SLIMbus device driver | expand |
On Wed, Nov 15, 2017 at 02:10:32PM +0000, 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. > > This patch adds device tree bindings for the slimbus. > > Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > Documentation/devicetree/bindings/slimbus/bus.txt | 50 +++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > create mode 100644 Documentation/devicetree/bindings/slimbus/bus.txt > > diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt b/Documentation/devicetree/bindings/slimbus/bus.txt > new file mode 100644 > index 000000000000..413b5076858e > --- /dev/null > +++ b/Documentation/devicetree/bindings/slimbus/bus.txt > @@ -0,0 +1,50 @@ > +SLIM(Serial Low Power Interchip Media Bus) bus > + > +SLIMbus is a 2-wire bus, and is used to communicate with peripheral > +components like audio-codec. > + > +Required property for SLIMbus controller node: > +- compatible - name of SLIMbus controller > + > +Child nodes: > +Every SLIMbus controller node can contain zero or more child nodes > +representing slave devices on the bus. Every SLIMbus slave device is > +uniquely determined by the enumeration address containing 4 fields: > +Manufacturer ID, Product code, Device index, and Instance value for > +the device. > +If child node is not present and it is instantiated after device > +discovery (slave device reporting itself present). So you allow the devices to work even if the respective firmware description is absent? > + > +In some cases it may be necessary to describe non-probeable device > +details such as non-standard ways of powering up a device. In > +such cases, child nodes for those devices will be present as > +slaves of the slimbus-controller, as detailed below. > + > +Required property for SLIMbus child node if it is present: > +- reg - Should be ('Device index', 'Instance ID') from SLIMbus > + Enumeration Address. > + Device Index Uniquely identifies multiple Devices within > + a single Component. > + Instance ID Is for the cases where multiple Devices of the > + same type or Class are attached to the bus. > + > +- compatible -"slimMID,PID". The textual representation of Manufacturer ID, > + Product Code, shall be in lower case hexadecimal with leading > + zeroes suppressed > + > +SLIMbus example for Qualcomm's slimbus manager component: > + > + slim@28080000 { > + compatible = "qcom,apq8064-slim", "qcom,slim"; > + reg = <0x28080000 0x2000>, > + interrupts = <0 33 0>; > + clocks = <&lcc SLIMBUS_SRC>, <&lcc AUDIO_SLIMBUS_CLK>; > + clock-names = "iface", "core"; > + #address-cells = <2>; > + #size-cell = <0>; > + > + codec: wcd9310@1,0{ > + compatible = "slim217,60"; > + reg = <1 0>; > + }; > + }; Pardon my ignorance as I am not very familiar with DT nodes, but where are the Manufacturer ID, Product code, Device index, and Instance values here? -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the review comments. On 16/11/17 13:09, Vinod Koul wrote: > On Wed, Nov 15, 2017 at 02:10:32PM +0000, srinivas.kandagatla@linaro.org wrote: >> From: Sagar Dharia <sdharia@codeaurora.org> > >> + >> +Child nodes: >> +Every SLIMbus controller node can contain zero or more child nodes >> +representing slave devices on the bus. Every SLIMbus slave device is >> +uniquely determined by the enumeration address containing 4 fields: >> +Manufacturer ID, Product code, Device index, and Instance value for >> +the device. >> +If child node is not present and it is instantiated after device >> +discovery (slave device reporting itself present). > > So you allow the devices to work even if the respective firmware description > is absent? Yes, As SLIMbus itself itself is discoverable bus. > >> + >> +In some cases it may be necessary to describe non-probeable device >> +details such as non-standard ways of powering up a device. In >> +such cases, child nodes for those devices will be present as >> +slaves of the slimbus-controller, as detailed below. >> + >> +Required property for SLIMbus child node if it is present: >> +- reg - Should be ('Device index', 'Instance ID') from SLIMbus >> + Enumeration Address. >> + Device Index Uniquely identifies multiple Devices within >> + a single Component. >> + Instance ID Is for the cases where multiple Devices of the >> + same type or Class are attached to the bus. >> + >> +- compatible -"slimMID,PID". The textual representation of Manufacturer ID, >> + Product Code, shall be in lower case hexadecimal with leading >> + zeroes suppressed >> + >> + codec: wcd9310@1,0{ >> + compatible = "slim217,60"; >> + reg = <1 0>; >> + }; >> + }; > > Pardon my ignorance as I am not very familiar with DT nodes, but where are > the Manufacturer ID, Product code, Device index, and Instance values here? Manfacturer ID and Product code is part of compatible string Device index and Instance value are part of reg. > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 16, 2017 at 05:29:35PM +0000, Srinivas Kandagatla wrote: > thanks for the comments. > > > On 16/11/17 16:42, Vinod Koul wrote: > >On Wed, Nov 15, 2017 at 02:10:34PM +0000, srinivas.kandagatla@linaro.org wrote: > > > >>+static void slim_dev_release(struct device *dev) > >>+{ > >>+ struct slim_device *sbdev = to_slim_device(dev); > >>+ > >>+ put_device(sbdev->ctrl->dev); > > > >which device would that be? > This is controller device > > > > >>+static int slim_add_device(struct slim_controller *ctrl, > >>+ struct slim_device *sbdev, > >>+ struct device_node *node) > >>+{ > >>+ sbdev->dev.bus = &slimbus_bus; > >>+ sbdev->dev.parent = ctrl->dev; > >>+ sbdev->dev.release = slim_dev_release; > >>+ sbdev->dev.driver = NULL; > >>+ sbdev->ctrl = ctrl; > >>+ > >>+ dev_set_name(&sbdev->dev, "%x:%x:%x:%x", > >>+ sbdev->e_addr.manf_id, > >>+ sbdev->e_addr.prod_code, > >>+ sbdev->e_addr.dev_index, > >>+ sbdev->e_addr.instance); > >>+ > >>+ get_device(ctrl->dev); > > > >is this controller device and you ensuring it doesnt go away while you have > >slaves on it? > > Yes. I thought since you are marking ctrl->dev as parent, the device core should ensure that parent doesn't go off when you have child device? Greg, is that understanding correct, if so we may not need these calls. -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 15, 2017 at 02:10:37PM +0000, srinivas.kandagatla@linaro.org wrote: > From: Sagar Dharia <sdharia@codeaurora.org> > > Per slimbus specification, a reconfiguration sequence known as > 'clock pause' needs to be broadcast over the bus while entering low- > power mode. Clock-pause is initiated by the controller driver. > To exit clock-pause, controller typically wakes up the framer device. > Since wakeup precedure is controller-specific, framework calls it via > controller's function pointer to invoke it. > > Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > +/** > + * struct slim_sched: Framework uses this structure internally for scheduling. Missing kernel doc for clkgear here. > + * @clk_state: Controller's clock state from enum slim_clk_state > + * @pause_comp: Signals completion of clock pause sequence. This is useful when > + * client tries to call slimbus transaction when controller is entering > + * clock pause. > + * @m_reconf: This mutex is held until current reconfiguration (data channel > + * scheduling, message bandwidth reservation) is done. Message APIs can > + * use the bus concurrently when this mutex is held since elemental access > + * messages can be sent on the bus when reconfiguration is in progress. > + */ > +struct slim_sched { > + int clkgear; > + enum slim_clk_state clk_state; > + struct completion pause_comp; > + struct mutex m_reconf; > +}; Thanks, Charles -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Srinivas and Charles, On Thu, Nov 23, 2017 at 10:07:03AM +0000, Charles Keepax wrote: > On Wed, Nov 15, 2017 at 02:10:41PM +0000, srinivas.kandagatla@linaro.org wrote: > > From: Sagar Dharia <sdharia@codeaurora.org> > > > > This controller driver programs manager, interface, and framer > > devices for Qualcomm's slimbus HW block. > > Manager component currently implements logical address setting, > > and messaging interface. > > Interface device reports bus synchronization information, and framer > > device clocks the bus from the time it's woken up, until clock-pause > > is executed by the manager device. > > > > Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > --- [...] > > +static void qcom_slim_rxwq(struct work_struct *work) > > +{ > > + u8 buf[SLIM_MSGQ_BUF_LEN]; > > + u8 mc, mt, len; > > + int i, ret; > > + struct qcom_slim_ctrl *ctrl = container_of(work, struct qcom_slim_ctrl, > > + wd); > > + > > + while ((slim_get_current_rxbuf(ctrl, buf)) != -ENODATA) { > > + len = SLIM_HEADER_GET_RL(buf[0]); > > + mt = SLIM_HEADER_GET_MT(buf[0]); > > + mc = SLIM_HEADER_GET_MC(buf[1]); > > + if (mt == SLIM_MSG_MT_CORE && > > + mc == SLIM_MSG_MC_REPORT_PRESENT) { > > + u8 laddr; > > + struct slim_eaddr ea; > > + u8 e_addr[6]; > > + > > + for (i = 0; i < 6; i++) > > + e_addr[i] = buf[7-i]; > > + > > + ea.manf_id = (u16)(e_addr[5] << 8) | e_addr[4]; > > + ea.prod_code = (u16)(e_addr[3] << 8) | e_addr[2]; > > + ea.dev_index = e_addr[1]; > > + ea.instance = e_addr[0]; > > If we are just bitshifting this out of the bytes does it really > make it much more clear to reverse the byte order first? Feels > like you might as well shift it out of buf directly. In any case, there is a predefined function to make this code a little nicer in <asm/byteorder.h>: le16_to_cpu(x): Converts the 16-bit little endian value x to CPU-endian le16_to_cpup(p): Converts the 16-bit little endian value pointed to by p to CPU-endian If you use le16_to_cpup, you need to cast your pointer to __le16 *: ea.manf_id = le16_to_cpup((__le16 *)&e_addr[4]); Like Charles, I don't quite see the point of the for loop that fills e_addr. I guess it did effectively a byteswap, so the original code, that assumed little-endian, could simply dereference a u16 *. This does not make a lot of sense anymore, once you use properly (CPU-)endian- independent code. (Of course, you'll need to replace le16 with be16 if you drop that loop.) Thanks, Jonathan Neuschäfer
On Mon, Nov 20, 2017 at 06:47:58AM +0000, Srinivas Kandagatla wrote: > >>>thanks for the comments. > >>> > >>> > >>>On 16/11/17 16:42, Vinod Koul wrote: > >>>>On Wed, Nov 15, 2017 at 02:10:34PM +0000,srinivas.kandagatla@linaro.org wrote: > >>>> > >>>>>+static void slim_dev_release(struct device *dev) > >>>>>+{ > >>>>>+ struct slim_device *sbdev = to_slim_device(dev); > >>>>>+ > >>>>>+ put_device(sbdev->ctrl->dev); > >>>>which device would that be? > >>>This is controller device > >>> > >>>>>+static int slim_add_device(struct slim_controller *ctrl, > >>>>>+ struct slim_device *sbdev, > >>>>>+ struct device_node *node) > >>>>>+{ > >>>>>+ sbdev->dev.bus = &slimbus_bus; > >>>>>+ sbdev->dev.parent = ctrl->dev; > >>>>>+ sbdev->dev.release = slim_dev_release; > >>>>>+ sbdev->dev.driver = NULL; > >>>>>+ sbdev->ctrl = ctrl; > >>>>>+ > >>>>>+ dev_set_name(&sbdev->dev, "%x:%x:%x:%x", > >>>>>+ sbdev->e_addr.manf_id, > >>>>>+ sbdev->e_addr.prod_code, > >>>>>+ sbdev->e_addr.dev_index, > >>>>>+ sbdev->e_addr.instance); > >>>>>+ > >>>>>+ get_device(ctrl->dev); > >>>>is this controller device and you ensuring it doesnt go away while you have > >>>>slaves on it? > >>>Yes. > >>I thought since you are marking ctrl->dev as parent, the device core should > >>ensure that parent doesn't go off when you have child device? > >> > >>Greg, is that understanding correct, if so we may not need these calls. > >That understanding should be correct, as the reference count is > >incremented on the parent when a child is added. > > > >It would be trivial for this to be tested, and yes, I am pretty sure you > >don't need this call. > > Thanks for suggestion, I will remove this in next version. I think it might be helpful to test the assumption as Greg noted :) -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Thanks for everyone who reviewed v6 patchset, here is v7 with review comments addressed. 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. Framework is introduced to support multiple instances of the bus (1 controller per bus), and multiple slave devices per controller. SPI and I2C frameworks, and comments from last time when I submitted the patches were referred-to while working on this framework. These patchsets introduce device-management, OF helpers, and messaging APIs, controller driver for Qualcomm's slimbus controller, and clock-pause feature for entering/exiting low-power mode for SLIMbus. Framework patches to do channel, port and bandwidth management are work-in-progress and will be sent out once these initial patches are accepted. These patchsets were tested on Qualcomm Snapdragon processor board using the controller driver, and a test slave device. v6: https://lwn.net/Articles/735693/ Changes from v6 to v7: * Various improvements in commenting style and kerneldoc suggested by various reviewers. * Removed usage of semaphores. * Split patches into more reviewable sizes. * Move buffer management into the controller driver. * use IDR for TID transactions. * use IDA for controller ids. * Remove lot of redundant code. * remove framer booted call for now, which can be added in future. * Added slim_readb/writeb and slim_read/write apis for simplified value element interface. Inspired by Soundwire patches. * Added slim device state so the the drivers which are probed after the device is announced, can get device state. * Lots of code cleanup. * move summary to .rst format. * move from callbacks to completions for async reads. * split slimbus.h into device and controller specific parts. Sagar Dharia (9): Documentation: Add SLIMbus summary dt-bindings: Add SLIMbus bindings slimbus: Add SLIMbus bus type slimbus: core: Add slim controllers support slimbus: Add messaging APIs to slimbus framework slimbus: Add support for 'clock-pause' feature dt-bindings: Add qcom slimbus controller bindings slimbus: qcom: Add Qualcomm Slimbus controller driver slimbus: qcom: Add runtime-pm support using clock-pause Srinivas Kandagatla (4): slimbus: core: add support to device tree helper regmap: add SLIMbus support slimbus: core: add common defines required for controllers MAINTAINERS: Add SLIMbus maintainer Documentation/devicetree/bindings/slimbus/bus.txt | 50 ++ .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt | 43 ++ Documentation/driver-api/slimbus/index.rst | 15 + Documentation/driver-api/slimbus/summary.rst | 108 +++ MAINTAINERS | 8 + drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/base/regmap/Kconfig | 4 + drivers/base/regmap/Makefile | 1 + drivers/base/regmap/regmap-slimbus.c | 95 +++ drivers/slimbus/Kconfig | 22 + drivers/slimbus/Makefile | 9 + drivers/slimbus/core.c | 500 +++++++++++++ drivers/slimbus/messaging.c | 448 ++++++++++++ drivers/slimbus/qcom-ctrl.c | 791 +++++++++++++++++++++ drivers/slimbus/sched.c | 128 ++++ drivers/slimbus/slimbus.h | 276 +++++++ include/linux/mod_devicetable.h | 13 + include/linux/regmap.h | 18 + include/linux/slimbus.h | 167 +++++ 20 files changed, 2699 insertions(+) create mode 100644 Documentation/devicetree/bindings/slimbus/bus.txt create mode 100644 Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt create mode 100644 Documentation/driver-api/slimbus/index.rst create mode 100644 Documentation/driver-api/slimbus/summary.rst create mode 100644 drivers/base/regmap/regmap-slimbus.c create mode 100644 drivers/slimbus/Kconfig create mode 100644 drivers/slimbus/Makefile create mode 100644 drivers/slimbus/core.c create mode 100644 drivers/slimbus/messaging.c create mode 100644 drivers/slimbus/qcom-ctrl.c create mode 100644 drivers/slimbus/sched.c create mode 100644 drivers/slimbus/slimbus.h create mode 100644 include/linux/slimbus.h -- 2.15.0 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html