[v6,0/7] Introduce framework for SLIMbus device drivers

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

It's been very long time since there was any activity on the slimbus patches,
Am currently working on getting Qualcomm DSP based audio working on top
of mainline. Slimbus is one of the major component for getting any analog
audio on QCOM SoC's. So am taking intiative to address the review
comments on the older patchset and send it.
I have tested this patch on IFC6410 board with wcd9310 codec.


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.

Changes from V5 to V6:
* aligned slim_driver_register more like other buses, suggested by Arnd.
* removed boardinfo and add_device apis for now, suggested by Arnd
* Few namespace cleanups suggested by Masami
* merged of apis in to first patch as suggested by Arnd.
* slimbus clients "compatible" name space made much inline with USB
  and PCIE, suggested by Rob and Arnd.
* Removed memory allocations to controller drivers, as suggested by Arnd.
* Various bindings comments addressed as suggested by Mark and others.
* Added regmap interface so that codecs can write more generic code.
* Added MAINTAINER file.

Sagar Dharia (5):
  slimbus: Device management on SLIMbus
  slimbus: Add messaging APIs to slimbus framework
  slimbus: qcom: Add Qualcomm Slimbus controller driver
  slimbus: Add support for 'clock-pause' feature
  slimbus: qcom: Add runtime-pm support using clock-pause feature

Srinivas Kandagatla (2):
  regmap: add SLIMBUS support
  MAINTAINERS: Add SLIMbus maintainer

 Documentation/devicetree/bindings/slimbus/bus.txt  |  57 ++
 .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
 Documentation/slimbus/summary                      | 109 ++++
 MAINTAINERS                                        |   8 +
 drivers/Kconfig                                    |   2 +
 drivers/Makefile                                   |   1 +
 drivers/base/regmap/Kconfig                        |   4 +
 drivers/base/regmap/Makefile                       |   1 +
 drivers/base/regmap/regmap-slimbus.c               |  89 +++
 drivers/slimbus/Kconfig                            |  20 +
 drivers/slimbus/Makefile                           |   8 +
 drivers/slimbus/slim-core.c                        | 725 +++++++++++++++++++++
 drivers/slimbus/slim-messaging.c                   | 509 +++++++++++++++
 drivers/slimbus/slim-qcom-ctrl.c                   | 714 ++++++++++++++++++++
 drivers/slimbus/slim-qcom.h                        |  64 ++
 drivers/slimbus/slim-sched.c                       | 126 ++++
 include/linux/mod_devicetable.h                    |  13 +
 include/linux/regmap.h                             |  18 +
 include/linux/slimbus.h                            | 512 +++++++++++++++
 19 files changed, 3023 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/slimbus/summary
 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/slim-core.c
 create mode 100644 drivers/slimbus/slim-messaging.c
 create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
 create mode 100644 drivers/slimbus/slim-qcom.h
 create mode 100644 drivers/slimbus/slim-sched.c
 create mode 100644 include/linux/slimbus.h

Hi,

On Fri, Oct 06, 2017 at 05:51:35PM +0200, srinivas.kandagatla@linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> This patch adds support to read/write slimbus value elements.
> Currently it only supports byte read/write. Adding this support in
> regmap would give codec drivers more flexibility when there are more
> than 2 control interfaces like slimbus, i2c.
> 
> Without this patch each codec driver has to directly call slimbus value
> element apis, and this could would get messy once we want to add i2c
> interface to it.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
[...]
> +static int regmap_slimbus_byte_reg_read(void *context, unsigned int reg,
> +					unsigned int *val)
> +{
> +	struct slim_device *slim = context;
> +	struct slim_val_inf msg = {0,};
> +
> +	msg.start_offset = reg;
> +	msg.num_bytes = 1;
> +	msg.rbuf = (void *)val;
> +
> +	return slim_request_val_element(slim, &msg);
> +}

This looks like it won't work on big-endian systems. I know big endian
is pretty uncommon in devices that will likely have SLIMBus, but it's
better to be endian-independent.

> +static int regmap_slimbus_byte_reg_write(void *context, unsigned int reg,
> +					 unsigned int val)
> +{
> +	struct slim_device *slim = context;
> +	struct slim_val_inf msg = {0,};
> +
> +	msg.start_offset = reg;
> +	msg.num_bytes = 1;
> +	msg.wbuf = (void *)&val;
> +
> +	return slim_change_val_element(slim, &msg);
> +}

dito

> +static struct regmap_bus regmap_slimbus_bus = {
> +	.reg_write = regmap_slimbus_byte_reg_write,
> +	.reg_read = regmap_slimbus_byte_reg_read,
> +};


Thanks,
Jonathan Neuschäfer

Hi,

On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@linaro.org wrote:
> From: Sagar Dharia <sdharia@codeaurora.org>
> 
> Slimbus devices use value-element, and information elements to
> control device parameters (e.g. value element is used to represent
> gain for codec, information element is used to represent interrupt
> status for codec when codec interrupt fires).
> Messaging APIs are used to set/get these value and information
> elements. Slimbus specification uses 8-bit "transaction IDs" for
> messages where a read-value is anticipated. Framework uses a table
> of pointers to store those TIDs and responds back to the caller in
> O(1).
> Caller can opt to do synchronous, or asynchronous reads/writes. For
> asynchronous operations, the callback will be called from atomic
> context.
> TX and RX circular rings are used to allow queuing of multiple
> transfers per controller. Controller can choose size of these rings
> based of controller HW implementation. The buffers are coerently

s/based of/based on/
s/coerently/coherently/

> mapped so that controller can utilize DMA operations for the
> transactions without remapping every transaction buffer.
> Statically allocated rings help to improve performance by avoiding
> overhead of dynamically allocating transactions on need basis.
> 
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Tested-by: Naveen Kaje <nkaje@codeaurora.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
[...]
> +static u16 slim_slicecodefromsize(u16 req)
> +{
> +	static const u8 codetosize[8] = {1, 2, 3, 4, 6, 8, 12, 16};
> +
> +	if (req >= ARRAY_SIZE(codetosize))
> +		return 0;
> +	else
> +		return codetosize[req];
> +}
> +
> +static u16 slim_slicesize(int code)
> +{
> +	static const u8 sizetocode[16] = {
> +		0, 1, 2, 3, 3, 4, 4, 5, 5, 5, 5, 6, 6, 6, 6, 7
> +	};
> +
> +	clamp(code, 1, (int)ARRAY_SIZE(sizetocode));
> +	return sizetocode[code - 1];
> +}
> +
> +int slim_xfer_msg(struct slim_controller *ctrl,
> +			struct slim_device *sbdev, struct slim_val_inf *msg,
> +			u8 mc)
> +{
> +	DEFINE_SLIM_LDEST_TXN(txn_stack, mc, 6, sbdev->laddr, msg);
> +	struct slim_msg_txn *txn = &txn_stack;
> +	int ret;
> +	u16 sl, cur;
> +
> +	ret = slim_val_inf_sanity(ctrl, msg, mc);
> +	if (ret)
> +		return ret;
> +
> +	sl = slim_slicesize(msg->num_bytes);
> +
> +	dev_dbg(&ctrl->dev, "SB xfer msg:os:%x, len:%d, MC:%x, sl:%x\n",
> +		msg->start_offset, msg->num_bytes, mc, sl);
> +
> +	cur = slim_slicecodefromsize(sl);
> +	txn->ec = ((sl | (1 << 3)) | ((msg->start_offset & 0xFFF) << 4));

Shouldn't this be (cur | (1 << 3)?
(Also, what does cur mean? Cursor? Current?)

> +
> +	switch (mc) {
> +	case SLIM_MSG_MC_REQUEST_CHANGE_VALUE:
> +	case SLIM_MSG_MC_CHANGE_VALUE:
> +	case SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION:
> +	case SLIM_MSG_MC_CLEAR_INFORMATION:
> +		txn->rl += msg->num_bytes;
> +	default:
> +		break;
> +	}
> +
> +	if (slim_tid_txn(txn->mt, txn->mc))
> +		txn->rl++;
> +
> +	return slim_processtxn(ctrl, txn);
> +}
> +EXPORT_SYMBOL_GPL(slim_xfer_msg);
[...]
> +/*
> + * slim_request_val_element: change and request a given value element
> + * @sb: client handle requesting elemental message reads, writes.
> + * @msg: Input structure for start-offset, number of bytes to write.
> + * context: can sleep
> + * Returns:
> + * -EINVAL: Invalid parameters
> + * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
> + *	not being clocked or driven by controller)
> + * -ENOTCONN: If the transmitted message was not ACKed by destination device.

Does rbuf contain the old value after this function finishes?

> + */
> +int slim_request_change_val_element(struct slim_device *sb,
> +					struct slim_val_inf *msg)
> +{
> +	struct slim_controller *ctrl = sb->ctrl;
> +
> +	if (!ctrl)
> +		return -EINVAL;
> +
> +	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_CHANGE_VALUE);
> +}
> +EXPORT_SYMBOL_GPL(slim_request_change_val_element);
[...]
> +/**
> + * struct slim_pending: context of pending transfers
> + * @cb: callback for this transfer
> + * @ctx: contex for the callback function

s/contex/context/

> + * @need_tid: True if this transfer need Transaction ID
> + */
> +struct slim_pending {
> +	void (*cb)(void *ctx, int err);
> +	void *ctx;
> +	bool need_tid;
> +};


Thanks,
Jonathan Neuschäfer

Hi, some trivial comments below.

On Fri, Oct 06, 2017 at 05:51:33PM +0200, 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>
> ---
[...]
> @@ -429,6 +444,14 @@ void slim_return_tx(struct slim_controller *ctrl, int err)
>  		cur.cb(cur.ctx, err);
>  
>  	up(&ctrl->tx_sem);
> +	if (!cur.clk_pause && (!cur.need_tid || err)) {
> +		/**

This isn't really a kerneldoc comment.

> +		 * remove runtime-pm vote if this was TX only, or
> +		 * if there was error during this transaction
> +		 */
> +		pm_runtime_mark_last_busy(ctrl->dev.parent);
> +		pm_runtime_put_autosuspend(ctrl->dev.parent);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(slim_return_tx);
>  
[...]
> +/**
> + * slim_ctrl_clk_pause: Called by slimbus controller to enter/exit 'clock pause'
> + * Slimbus specification needs this sequence to turn-off clocks for the bus.
> + * The sequence involves sending 3 broadcast messages (reconfiguration
> + * sequence) to inform all devices on the bus.
> + * To exit clock-pause, controller typically wakes up active framer device.
> + * @ctrl: controller requesting bus to be paused or woken up
> + * @wakeup: Wakeup this controller from clock pause.
> + * @restart: Restart time value per spec used for clock pause. This value
> + *	isn't used when controller is to be woken up.
> + * This API executes clock pause reconfiguration sequence if wakeup is false.
> + * If wakeup is true, controller's wakeup is called.
> + * For entering clock-pause, -EBUSY is returned if a message txn in pending.
> + */
> +int slim_ctrl_clk_pause(struct slim_controller *ctrl, bool wakeup, u8 restart)
> +{
> +	int i, ret = 0;
> +	unsigned long flags;
> +	struct slim_sched *sched = &ctrl->sched;
> +	struct slim_val_inf msg = {0, 0, NULL, NULL, NULL, NULL};
> +
> +	DEFINE_SLIM_BCAST_TXN(txn, SLIM_MSG_MC_BEGIN_RECONFIGURATION,
> +				3, SLIM_LA_MANAGER, &msg);
> +
> +	if (wakeup == false && restart > SLIM_CLK_UNSPECIFIED)
> +		return -EINVAL;
> +
> +	mutex_lock(&sched->m_reconf);
> +	if (wakeup) {
> +		if (sched->clk_state == SLIM_CLK_ACTIVE) {
> +			mutex_unlock(&sched->m_reconf);
> +			return 0;
> +		}
> +
> +		/**

ditto

> +		 * Fine-tune calculation based on clock gear,
> +		 * message-bandwidth after bandwidth management
> +		 */
> +		ret = wait_for_completion_timeout(&sched->pause_comp,
> +				msecs_to_jiffies(100));
> +		if (!ret) {
> +			mutex_unlock(&sched->m_reconf);
> +			pr_err("Previous clock pause did not finish");
> +			return -ETIMEDOUT;
> +		}
> +		ret = 0;
> +
> +		/**

ditto

> +		 * Slimbus framework will call controller wakeup
> +		 * Controller should make sure that it sets active framer
> +		 * out of clock pause
> +		 */
> +		if (sched->clk_state == SLIM_CLK_PAUSED && ctrl->wakeup)
> +			ret = ctrl->wakeup(ctrl);
> +		if (!ret)
> +			sched->clk_state = SLIM_CLK_ACTIVE;
> +		mutex_unlock(&sched->m_reconf);
> +
> +		return ret;
> +	}
[...]


Thanks,
Jonathan Neuschäfer

Hi, some more trivial comments below.

On Fri, Oct 06, 2017 at 05:51:34PM +0200, srinivas.kandagatla@linaro.org wrote:
> From: Sagar Dharia <sdharia@codeaurora.org>
> 
> Slimbus HW mandates that clock-pause sequence has to be executed
> before disabling relevant interface and core clocks.
> Runtime-PM's autosuspend feature is used here to enter/exit low
> power mode for Qualcomm's Slimbus controller. Autosuspend feature
> enables driver to avoid changing power-modes too frequently since
> entering clock-pause is an expensive sequence
> 
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
[...]
> +static int msm_clk_pause_wakeup(struct slim_controller *ctrl)
> +{
> +	struct msm_slim_ctrl *dev = slim_get_ctrldata(ctrl);
> +
> +	clk_prepare_enable(dev->hclk);
> +	clk_prepare_enable(dev->rclk);
> +	enable_irq(dev->irq);
> +
> +	writel_relaxed(1, dev->base + FRM_WAKEUP);
> +	/* Make sure framer wakeup write goes through before ISR fires */
> +	mb();
> +	/**

This isn't really a kerneldoc comment.

> +	 * HW Workaround: Currently, slave is reporting lost-sync messages
> +	 * after slimbus comes out of clock pause.
> +	 * Transaction with slave fail before slave reports that message
> +	 * Give some time for that report to come
> +	 * Slimbus wakes up in clock gear 10 at 24.576MHz. With each superframe
> +	 * being 250 usecs, we wait for 5-10 superframes here to ensure
> +	 * we get the message
> +	 */
> +	usleep_range(1250, 2500);
> +	return 0;
> +}
[...]
> +#ifdef CONFIG_PM_SLEEP
> +static int msm_slim_suspend(struct device *dev)
> +{
> +	int ret = 0;
> +
> +	if (!pm_runtime_enabled(dev) ||
> +		(!pm_runtime_suspended(dev))) {
> +		dev_dbg(dev, "system suspend");
> +		ret = msm_slim_runtime_suspend(dev);
> +	}
> +	if (ret == -EISCONN) {
> +	/**

ditto.
Also, it looks misindented.

> +	 * If the clock pause failed due to active channels, there is
> +	 * a possibility that some audio stream is active during suspend.
> +	 * (e.g. modem usecase during suspend)
> +	 * We dont want to return suspend failure in that case so that
> +	 * display and relevant components can still go to suspend.
> +	 * If there is some other error, then it should prevent
> +	 * system level suspend
> +	 */
> +		ret = 0;
> +	}
> +	return ret;
> +}


Thanks,
Jonathan Neuschäfer

Thanks for the comments.

On 07/10/17 07:42, Jonathan Neuschäfer wrote:
> Hi,
> 
> On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@linaro.org wrote:
>> From: Sagar Dharia <sdharia@codeaurora.org>
>>
>> Slimbus devices use value-element, and information elements to
>> control device parameters (e.g. value element is used to represent
>> gain for codec, information element is used to represent interrupt
>> status for codec when codec interrupt fires).
>> Messaging APIs are used to set/get these value and information
>> elements. Slimbus specification uses 8-bit "transaction IDs" for
>> messages where a read-value is anticipated. Framework uses a table
>> of pointers to store those TIDs and responds back to the caller in
>> O(1).
>> Caller can opt to do synchronous, or asynchronous reads/writes. For
>> asynchronous operations, the callback will be called from atomic
>> context.
>> TX and RX circular rings are used to allow queuing of multiple
>> transfers per controller. Controller can choose size of these rings
>> based of controller HW implementation. The buffers are coerently
> 
> s/based of/based on/
> s/coerently/coherently/
Yep.. will fix this in next version.
> 
>> mapped so that controller can utilize DMA operations for the
>> transactions without remapping every transaction buffer.
>> Statically allocated rings help to improve performance by avoiding
>> overhead of dynamically allocating transactions on need basis.
>>
>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>> Tested-by: Naveen Kaje <nkaje@codeaurora.org>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
> [...]
>> +int slim_xfer_msg(struct slim_controller *ctrl,
>> +			struct slim_device *sbdev, struct slim_val_inf *msg,
>> +			u8 mc)
>> +{
>> +	DEFINE_SLIM_LDEST_TXN(txn_stack, mc, 6, sbdev->laddr, msg);
>> +	struct slim_msg_txn *txn = &txn_stack;
>> +	int ret;
>> +	u16 sl, cur;
>> +
>> +	ret = slim_val_inf_sanity(ctrl, msg, mc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sl = slim_slicesize(msg->num_bytes);
>> +
>> +	dev_dbg(&ctrl->dev, "SB xfer msg:os:%x, len:%d, MC:%x, sl:%x\n",
>> +		msg->start_offset, msg->num_bytes, mc, sl);
>> +
>> +	cur = slim_slicecodefromsize(sl);
>> +	txn->ec = ((sl | (1 << 3)) | ((msg->start_offset & 0xFFF) << 4));
> 
> Shouldn't this be (cur | (1 << 3)?
cur seems to be redundant TBH, the only difference between cur and sl is 
that the slim_slicesize() can give slice size to program for any lengths 
between 1-16 bytes. However the slim_slicecodefromsize() can only 
handle 1,2,3,4, 6,8,12,16 byte sizes.

So we can delete slim_slicecodefromsize() call and function together.
looks like it was a leftover from downstream.

> (Also, what does cur mean? Cursor? Current?)
No Idea!! :-) it is supposed to return slice size as per number of bytes.

> 
>> +
>> +	switch (mc) {
>> +	case SLIM_MSG_MC_REQUEST_CHANGE_VALUE:
>> +	case SLIM_MSG_MC_CHANGE_VALUE:
>> +	case SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION:
>> +	case SLIM_MSG_MC_CLEAR_INFORMATION:
>> +		txn->rl += msg->num_bytes;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	if (slim_tid_txn(txn->mt, txn->mc))
>> +		txn->rl++;
>> +
>> +	return slim_processtxn(ctrl, txn);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_xfer_msg);
> [...]
>> +/*
>> + * slim_request_val_element: change and request a given value element

name should be fixed here..
>> + * @sb: client handle requesting elemental message reads, writes.
>> + * @msg: Input structure for start-offset, number of bytes to write.
>> + * context: can sleep
>> + * Returns:
>> + * -EINVAL: Invalid parameters
>> + * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
>> + *	not being clocked or driven by controller)
>> + * -ENOTCONN: If the transmitted message was not ACKed by destination device.
> 
> Does rbuf contain the old value after this function finishes?
> 
Yep, device should send a reply value with the old value with matching tid.

>> + */
>> +int slim_request_change_val_element(struct slim_device *sb,
>> +					struct slim_val_inf *msg)
>> +{
>> +	struct slim_controller *ctrl = sb->ctrl;
>> +
>> +	if (!ctrl)
>> +		return -EINVAL;
>> +
>> +	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_CHANGE_VALUE);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_request_change_val_element);
> [...]
>> +/**
>> + * struct slim_pending: context of pending transfers
>> + * @cb: callback for this transfer
>> + * @ctx: contex for the callback function
> 
> s/contex/context/
> 
Will fix all these instances.
>> + * @need_tid: True if this transfer need Transaction ID
>> + */
>> +struct slim_pending {
>> +	void (*cb)(void *ctx, int err);
>> +	void *ctx;
>> +	bool need_tid;
>> +};
> 
> 
> Thanks,
> Jonathan Neuschäfer
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 07/10/17 09:06, Jonathan Neuschäfer wrote:
> Hi, some trivial comments below.
> 
> On Fri, Oct 06, 2017 at 05:51:33PM +0200, 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>
>> ---
> [...]
>> @@ -429,6 +444,14 @@ void slim_return_tx(struct slim_controller *ctrl, int err)
>>   		cur.cb(cur.ctx, err);
>>   
>>   	up(&ctrl->tx_sem);
>> +	if (!cur.clk_pause && (!cur.need_tid || err)) {
>> +		/**
> 
> This isn't really a kerneldoc comment.
Will fix all such instances in next version.
> 
>> +		 * remove runtime-pm vote if this was TX only, or
>> +		 * if there was error during this transaction
>> +		 */
>> +		pm_runtime_mark_last_busy(ctrl->dev.parent);
>> +		pm_runtime_put_autosuspend(ctrl->dev.parent);
>> +	}
>>   }
>>   EXPORT_SYMBOL_GPL(slim_return_tx);
>>   
> [...]
>> +/**
>> + * slim_ctrl_clk_pause: Called by slimbus controller to enter/exit 'clock pause'
>> + * Slimbus specification needs this sequence to turn-off clocks for the bus.
>> + * The sequence involves sending 3 broadcast messages (reconfiguration
>> + * sequence) to inform all devices on the bus.
>> + * To exit clock-pause, controller typically wakes up active framer device.
>> + * @ctrl: controller requesting bus to be paused or woken up
>> + * @wakeup: Wakeup this controller from clock pause.
>> + * @restart: Restart time value per spec used for clock pause. This value
>> + *	isn't used when controller is to be woken up.
>> + * This API executes clock pause reconfiguration sequence if wakeup is false.
>> + * If wakeup is true, controller's wakeup is called.
>> + * For entering clock-pause, -EBUSY is returned if a message txn in pending.
>> + */
>> +int slim_ctrl_clk_pause(struct slim_controller *ctrl, bool wakeup, u8 restart)
>> +{
>> +	int i, ret = 0;
>> +	unsigned long flags;
>> +	struct slim_sched *sched = &ctrl->sched;
>> +	struct slim_val_inf msg = {0, 0, NULL, NULL, NULL, NULL};
>> +
>> +	DEFINE_SLIM_BCAST_TXN(txn, SLIM_MSG_MC_BEGIN_RECONFIGURATION,
>> +				3, SLIM_LA_MANAGER, &msg);
>> +
>> +	if (wakeup == false && restart > SLIM_CLK_UNSPECIFIED)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&sched->m_reconf);
>> +	if (wakeup) {
>> +		if (sched->clk_state == SLIM_CLK_ACTIVE) {
>> +			mutex_unlock(&sched->m_reconf);
>> +			return 0;
>> +		}
>> +
>> +		/**
> 
> ditto
> 
>> +		 * Fine-tune calculation based on clock gear,
>> +		 * message-bandwidth after bandwidth management
>> +		 */
>> +		ret = wait_for_completion_timeout(&sched->pause_comp,
>> +				msecs_to_jiffies(100));
>> +		if (!ret) {
>> +			mutex_unlock(&sched->m_reconf);
>> +			pr_err("Previous clock pause did not finish");
>> +			return -ETIMEDOUT;
>> +		}
>> +		ret = 0;
>> +
>> +		/**
> 
> ditto
> 
>> +		 * Slimbus framework will call controller wakeup
>> +		 * Controller should make sure that it sets active framer
>> +		 * out of clock pause
>> +		 */
>> +		if (sched->clk_state == SLIM_CLK_PAUSED && ctrl->wakeup)
>> +			ret = ctrl->wakeup(ctrl);
>> +		if (!ret)
>> +			sched->clk_state = SLIM_CLK_ACTIVE;
>> +		mutex_unlock(&sched->m_reconf);
>> +
>> +		return ret;
>> +	}
> [...]
> 
> 
> Thanks,
> Jonathan Neuschäfer
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 07/10/17 09:22, Jonathan Neuschäfer wrote:
> Hi, some more trivial comments below.
> 
> On Fri, Oct 06, 2017 at 05:51:34PM +0200, srinivas.kandagatla@linaro.org wrote:
>> From: Sagar Dharia <sdharia@codeaurora.org>
>>
>> Slimbus HW mandates that clock-pause sequence has to be executed
>> before disabling relevant interface and core clocks.
>> Runtime-PM's autosuspend feature is used here to enter/exit low
>> power mode for Qualcomm's Slimbus controller. Autosuspend feature
>> enables driver to avoid changing power-modes too frequently since
>> entering clock-pause is an expensive sequence
>>
>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
> [...]
>> +static int msm_clk_pause_wakeup(struct slim_controller *ctrl)
>> +{
>> +	struct msm_slim_ctrl *dev = slim_get_ctrldata(ctrl);
>> +
>> +	clk_prepare_enable(dev->hclk);
>> +	clk_prepare_enable(dev->rclk);
>> +	enable_irq(dev->irq);
>> +
>> +	writel_relaxed(1, dev->base + FRM_WAKEUP);
>> +	/* Make sure framer wakeup write goes through before ISR fires */
>> +	mb();
>> +	/**
> 
> This isn't really a kerneldoc comment.
Yep I agree will fix all such instances.

> 
>> +	 * HW Workaround: Currently, slave is reporting lost-sync messages
>> +	 * after slimbus comes out of clock pause.
>> +	 * Transaction with slave fail before slave reports that message
>> +	 * Give some time for that report to come
>> +	 * Slimbus wakes up in clock gear 10 at 24.576MHz. With each superframe
>> +	 * being 250 usecs, we wait for 5-10 superframes here to ensure
>> +	 * we get the message
>> +	 */
>> +	usleep_range(1250, 2500);
>> +	return 0;
>> +}
> [...]
>> +#ifdef CONFIG_PM_SLEEP
>> +static int msm_slim_suspend(struct device *dev)
>> +{
>> +	int ret = 0;
>> +
>> +	if (!pm_runtime_enabled(dev) ||
>> +		(!pm_runtime_suspended(dev))) {
>> +		dev_dbg(dev, "system suspend");
>> +		ret = msm_slim_runtime_suspend(dev);
>> +	}
>> +	if (ret == -EISCONN) {
>> +	/**
> 
> ditto.
> Also, it looks misindented.
Yep. will fix this too.

> 
>> +	 * If the clock pause failed due to active channels, there is
>> +	 * a possibility that some audio stream is active during suspend.
>> +	 * (e.g. modem usecase during suspend)
>> +	 * We dont want to return suspend failure in that case so that
>> +	 * display and relevant components can still go to suspend.
>> +	 * If there is some other error, then it should prevent
>> +	 * system level suspend
>> +	 */
>> +		ret = 0;
>> +	}
>> +	return ret;
>> +}
> 
> 
> Thanks,
> Jonathan Neuschäfer
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 07/10/17 06:02, Jonathan Neuschäfer wrote:
> Hi,
> 
> On Fri, Oct 06, 2017 at 05:51:35PM +0200, srinivas.kandagatla@linaro.org wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds support to read/write slimbus value elements.
>> Currently it only supports byte read/write. Adding this support in
>> regmap would give codec drivers more flexibility when there are more
>> than 2 control interfaces like slimbus, i2c.
>>
>> Without this patch each codec driver has to directly call slimbus value
>> element apis, and this could would get messy once we want to add i2c
>> interface to it.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
> [...]
>> +static int regmap_slimbus_byte_reg_read(void *context, unsigned int reg,
>> +					unsigned int *val)
>> +{
>> +	struct slim_device *slim = context;
>> +	struct slim_val_inf msg = {0,};
>> +
>> +	msg.start_offset = reg;
>> +	msg.num_bytes = 1;
>> +	msg.rbuf = (void *)val;
>> +
>> +	return slim_request_val_element(slim, &msg);
>> +}
> 
> This looks like it won't work on big-endian systems. I know big endian
> is pretty uncommon in devices that will likely have SLIMBus, but it's
> better to be endian-independent.

Yep, I agree! will fix it in next version.

> 
>> +static int regmap_slimbus_byte_reg_write(void *context, unsigned int reg,
>> +					 unsigned int val)
>> +{
>> +	struct slim_device *slim = context;
>> +	struct slim_val_inf msg = {0,};
>> +
>> +	msg.start_offset = reg;
>> +	msg.num_bytes = 1;
>> +	msg.wbuf = (void *)&val;
>> +
>> +	return slim_change_val_element(slim, &msg);
>> +}
> 
> dito
> 
>> +static struct regmap_bus regmap_slimbus_bus = {
>> +	.reg_write = regmap_slimbus_byte_reg_write,
>> +	.reg_read = regmap_slimbus_byte_reg_read,
>> +};
> 
> 
> Thanks,
> Jonathan Neuschäfer
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, Oct 07, 2017 at 11:24:33AM +0100, Srinivas Kandagatla wrote:
> Thanks for the comments.
> 
> On 07/10/17 07:42, Jonathan Neuschäfer wrote:
> > Hi,
> > 
> > On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@linaro.org wrote:
> > > From: Sagar Dharia <sdharia@codeaurora.org>
[...]
> > > +int slim_xfer_msg(struct slim_controller *ctrl,
> > > +			struct slim_device *sbdev, struct slim_val_inf *msg,
> > > +			u8 mc)
> > > +{
> > > +	DEFINE_SLIM_LDEST_TXN(txn_stack, mc, 6, sbdev->laddr, msg);
> > > +	struct slim_msg_txn *txn = &txn_stack;
> > > +	int ret;
> > > +	u16 sl, cur;
> > > +
> > > +	ret = slim_val_inf_sanity(ctrl, msg, mc);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	sl = slim_slicesize(msg->num_bytes);
> > > +
> > > +	dev_dbg(&ctrl->dev, "SB xfer msg:os:%x, len:%d, MC:%x, sl:%x\n",
> > > +		msg->start_offset, msg->num_bytes, mc, sl);
> > > +
> > > +	cur = slim_slicecodefromsize(sl);
> > > +	txn->ec = ((sl | (1 << 3)) | ((msg->start_offset & 0xFFF) << 4));
> > 
> > Shouldn't this be (cur | (1 << 3)?

I misread the code here: I thought that cur was assigned the
"compressed" message length (in the range 0..7) here, but that's not
true, as slim_slicecodefromsize returns an "uncompressed" number. Thus
cur is a "quantized"[1] version of msg->num_bytes.

> cur seems to be redundant TBH, the only difference between cur and sl is
> that the slim_slicesize() can give slice size to program for any lengths
> between 1-16 bytes. However the slim_slicecodefromsize() can only handle
> 1,2,3,4, 6,8,12,16 byte sizes.

In any case, cur is only assigned and not used, as the code currently
is.

> So we can delete slim_slicecodefromsize() call and function together.
> looks like it was a leftover from downstream.

I agree. I don't know how it *might* be used, because I haven't read the
SLIMbus spec, but it is unused here.

> > (Also, what does cur mean? Cursor? Current?)
> No Idea!! :-) it is supposed to return slice size as per number of bytes.

Another problem solved by deleting slim_slicecodefromsize :-)

(As a small side-note, I think slim_slicesize and slim_slicecodefromsize
are named backwards: I would call sl, as used above, a "slice code",
because it encodes the message length)


> > > +/*
> > > + * slim_request_val_element: change and request a given value element
> 
> name should be fixed here..

Good catch.

> > > + * @sb: client handle requesting elemental message reads, writes.
> > > + * @msg: Input structure for start-offset, number of bytes to write.
> > > + * context: can sleep
> > > + * Returns:
> > > + * -EINVAL: Invalid parameters
> > > + * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
> > > + *	not being clocked or driven by controller)
> > > + * -ENOTCONN: If the transmitted message was not ACKed by destination device.
> > 
> > Does rbuf contain the old value after this function finishes?
> > 
> Yep, device should send a reply value with the old value with matching tid.

I think you should document this in the comment to help readers.


Thanks,
Jonathan Neuschäfer

[1]: https://en.wikipedia.org/wiki/Quantization

On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@linaro.org wrote:
> From: Sagar Dharia <sdharia@codeaurora.org>
> 
> Slimbus devices use value-element, and information elements to
> control device parameters (e.g. value element is used to represent
> gain for codec, information element is used to represent interrupt
> status for codec when codec interrupt fires).
> Messaging APIs are used to set/get these value and information
> elements. Slimbus specification uses 8-bit "transaction IDs" for
> messages where a read-value is anticipated. Framework uses a table
> of pointers to store those TIDs and responds back to the caller in
> O(1).
> Caller can opt to do synchronous, or asynchronous reads/writes. For
> asynchronous operations, the callback will be called from atomic
> context.
> TX and RX circular rings are used to allow queuing of multiple
> transfers per controller. Controller can choose size of these rings
> based of controller HW implementation. The buffers are coerently
> mapped so that controller can utilize DMA operations for the
> transactions without remapping every transaction buffer.
> Statically allocated rings help to improve performance by avoiding
> overhead of dynamically allocating transactions on need basis.
> 
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Tested-by: Naveen Kaje <nkaje@codeaurora.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/slimbus/Makefile         |   2 +-
>  drivers/slimbus/slim-core.c      |  15 ++
>  drivers/slimbus/slim-messaging.c | 471 +++++++++++++++++++++++++++++++++++++++
>  include/linux/slimbus.h          | 161 +++++++++++++
>  4 files changed, 648 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/slimbus/slim-messaging.c
> 
> +/**
> + * slim_processtxn: Process a slimbus-messaging transaction
> + * @ctrl: Controller handle
> + * @txn: Transaction to be sent over SLIMbus
> + * Called by controller to transmit messaging transactions not dealing with
> + * Interface/Value elements. (e.g. transmittting a message to assign logical
> + * address to a slave device
> + * Returns:
> + * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
> + *	not being clocked or driven by controller)
> + * -ENOTCONN: If the transmitted message was not ACKed by destination device.
> + */
> +int slim_processtxn(struct slim_controller *ctrl,
> +				struct slim_msg_txn *txn)

Can all go on one line.

> +{
> +	int ret, i = 0;
> +	unsigned long flags;
> +	u8 *buf;
> +	bool async = false;
> +	struct slim_cb_data cbd;
> +	DECLARE_COMPLETION_ONSTACK(done);
> +	bool need_tid = slim_tid_txn(txn->mt, txn->mc);
> +
> +	if (!txn->msg->comp_cb) {
> +		txn->msg->comp_cb = slim_sync_default_cb;
> +		cbd.comp = &done;
> +		txn->msg->ctx = &cbd;
> +	} else {
> +		async = true;
> +	}
> +
> +	buf = slim_get_tx(ctrl, txn, need_tid);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (need_tid) {
> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
> +		for (i = 0; i < ctrl->last_tid; i++) {
> +			if (ctrl->tid_tbl[i] == NULL)
> +				break;
> +		}
> +		if (i >= ctrl->last_tid) {
> +			if (ctrl->last_tid == (SLIM_MAX_TIDS - 1)) {
> +				spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +				slim_return_tx(ctrl, -ENOMEM);
> +				return -ENOMEM;
> +			}
> +			ctrl->last_tid++;
> +		}
> +		ctrl->tid_tbl[i] = txn->msg;
> +		txn->tid = i;
> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +	}
> +
> +	ret = ctrl->xfer_msg(ctrl, txn, buf);
> +
> +	if (!ret && !async) { /* sync transaction */
> +		/* Fine-tune calculation after bandwidth management */
> +		unsigned long ms = txn->rl + 100;
> +
> +		ret = wait_for_completion_timeout(&done,
> +						  msecs_to_jiffies(ms));
> +		if (!ret)
> +			slim_return_tx(ctrl, -ETIMEDOUT);
> +
> +		ret = cbd.ret;
> +	}
> +
> +	if (ret && need_tid) {
> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
> +		/* Invalidate the transaction */
> +		ctrl->tid_tbl[txn->tid] = NULL;
> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +	}
> +	if (ret)
> +		dev_err(&ctrl->dev, "Tx:MT:0x%x, MC:0x%x, LA:0x%x failed:%d\n",
> +			txn->mt, txn->mc, txn->la, ret);
> +	if (!async) {
> +		txn->msg->comp_cb = NULL;
> +		txn->msg->ctx = NULL;
> +	}

What is the intent of this if statement here? We set async
locally so this code only runs if we executed the else on the if
statement at the top. If its just clearing anything the user
might have put in these fields why not do it up there.

> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(slim_processtxn);
> +
> +int slim_xfer_msg(struct slim_controller *ctrl,
> +			struct slim_device *sbdev, struct slim_val_inf *msg,
> +			u8 mc)
> +{
> +	DEFINE_SLIM_LDEST_TXN(txn_stack, mc, 6, sbdev->laddr, msg);
> +	struct slim_msg_txn *txn = &txn_stack;
> +	int ret;
> +	u16 sl, cur;
> +
> +	ret = slim_val_inf_sanity(ctrl, msg, mc);
> +	if (ret)
> +		return ret;
> +
> +	sl = slim_slicesize(msg->num_bytes);
> +
> +	dev_dbg(&ctrl->dev, "SB xfer msg:os:%x, len:%d, MC:%x, sl:%x\n",
> +		msg->start_offset, msg->num_bytes, mc, sl);
> +
> +	cur = slim_slicecodefromsize(sl);

cur should probably be removed until it is needed.

> +	txn->ec = ((sl | (1 << 3)) | ((msg->start_offset & 0xFFF) << 4));
> +
> +	switch (mc) {
> +	case SLIM_MSG_MC_REQUEST_CHANGE_VALUE:
> +	case SLIM_MSG_MC_CHANGE_VALUE:
> +	case SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION:
> +	case SLIM_MSG_MC_CLEAR_INFORMATION:
> +		txn->rl += msg->num_bytes;
> +	default:
> +		break;
> +	}
> +
> +	if (slim_tid_txn(txn->mt, txn->mc))
> +		txn->rl++;
> +
> +	return slim_processtxn(ctrl, txn);
> +}
> +EXPORT_SYMBOL_GPL(slim_xfer_msg);
> +
> +/* Message APIs Unicast message APIs used by slimbus slave drivers */
> +
> +/*
> + * slim_request_val_element: request value element
> + * @sb: client handle requesting elemental message reads, writes.
> + * @msg: Input structure for start-offset, number of bytes to read.
> + * context: can sleep
> + * Returns:
> + * -EINVAL: Invalid parameters
> + * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
> + *	not being clocked or driven by controller)
> + * -ENOTCONN: If the transmitted message was not ACKed by destination device.
> + */
> +int slim_request_val_element(struct slim_device *sb,
> +				struct slim_val_inf *msg)
> +{
> +	struct slim_controller *ctrl = sb->ctrl;
> +
> +	if (!ctrl)
> +		return -EINVAL;

You could put this check into slim_xfer_msg and save duplicating
it. Would also then cover cases that call that function directly,
also would let you make these functions either inlines or macros.

> +
> +	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_VALUE);
> +}
> +EXPORT_SYMBOL_GPL(slim_request_val_element);
> +
> +/* Functions to get/return TX, RX buffers for messaging. */
> +
> +/**
> + * slim_get_rx: To get RX buffers for messaging.
> + * @ctrl: Controller handle
> + * These functions are called by controller to process the RX buffers.
> + * RX buffer is requested by controller when data is received from HW, but is
> + * not processed (e.g. 'report-present message was sent by HW in ISR and SW
> + * needs more time to process the buffer to assign Logical Address)
> + * RX buffer is returned back to the pool when associated RX action
> + * is taken (e.g. Received message is decoded and client's
> + * response buffer is filled in.)
> + */
> +void *slim_get_rx(struct slim_controller *ctrl)
> +{
> +	unsigned long flags;
> +	int idx;
> +
> +	spin_lock_irqsave(&ctrl->rx.lock, flags);
> +	if ((ctrl->rx.tail + 1) % ctrl->rx.n == ctrl->rx.head) {
> +		spin_unlock_irqrestore(&ctrl->rx.lock, flags);
> +		dev_err(&ctrl->dev, "RX QUEUE full!");
> +		return NULL;
> +	}
> +	idx = ctrl->rx.tail;
> +	ctrl->rx.tail = (ctrl->rx.tail + 1) % ctrl->rx.n;
> +	spin_unlock_irqrestore(&ctrl->rx.lock, flags);
> +
> +	return ctrl->rx.base + (idx * ctrl->rx.sl_sz);
> +}
> +EXPORT_SYMBOL_GPL(slim_get_rx);
> +
> +int slim_return_rx(struct slim_controller *ctrl, void *buf)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ctrl->rx.lock, flags);
> +	if (ctrl->rx.tail == ctrl->rx.head) {
> +		spin_unlock_irqrestore(&ctrl->rx.lock, flags);
> +		return -ENODATA;
> +	}
> +	memcpy(buf, ctrl->rx.base + (ctrl->rx.head * ctrl->rx.sl_sz),
> +				ctrl->rx.sl_sz);
> +	ctrl->rx.head = (ctrl->rx.head + 1) % ctrl->rx.n;
> +	spin_unlock_irqrestore(&ctrl->rx.lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(slim_return_rx);

I find the combination of get/return a bit odd, would get/put
maybe more idiomatic? Also the return could use some kernel doc.

> +
> +void slim_return_tx(struct slim_controller *ctrl, int err)
> +{
> +	unsigned long flags;
> +	int idx;
> +	struct slim_pending cur;
> +
> +	spin_lock_irqsave(&ctrl->tx.lock, flags);
> +	idx = ctrl->tx.head;
> +	ctrl->tx.head = (ctrl->tx.head + 1) % ctrl->tx.n;
> +	cur = ctrl->pending_wr[idx];
> +	spin_unlock_irqrestore(&ctrl->tx.lock, flags);
> +
> +	if (!cur.cb)
> +		dev_err(&ctrl->dev, "NULL Transaction or completion");
> +	else
> +		cur.cb(cur.ctx, err);
> +
> +	up(&ctrl->tx_sem);
> +}
> +EXPORT_SYMBOL_GPL(slim_return_tx);
> +
> +/**
> + * slim_get_tx: To get TX buffers for messaging.
> + * @ctrl: Controller handle
> + * These functions are called by controller to process the TX buffers.
> + * TX buffer is requested by controller when it's filled-in and sent to the
> + * HW. When HW has finished processing this buffer, controller should return it
> + * back to the pool.
> + */
> +void *slim_get_tx(struct slim_controller *ctrl, struct slim_msg_txn *txn,
> +		bool need_tid)
> +{
> +	unsigned long flags;
> +	int ret, idx;
> +
> +	ret = down_interruptible(&ctrl->tx_sem);
> +	if (ret < 0) {
> +		dev_err(&ctrl->dev, "TX semaphore down returned:%d", ret);
> +		return NULL;
> +	}
> +	spin_lock_irqsave(&ctrl->tx.lock, flags);
> +
> +	if (((ctrl->tx.head + 1) % ctrl->tx.n) == ctrl->tx.tail) {
> +		spin_unlock_irqrestore(&ctrl->tx.lock, flags);
> +		dev_err(&ctrl->dev, "controller TX buf unavailable");
> +		up(&ctrl->tx_sem);
> +		return NULL;
> +	}
> +	idx = ctrl->tx.tail;
> +	ctrl->tx.tail = (ctrl->tx.tail + 1) % ctrl->tx.n;
> +	ctrl->pending_wr[idx].cb = txn->msg->comp_cb;
> +	ctrl->pending_wr[idx].ctx = txn->msg->ctx;
> +	ctrl->pending_wr[idx].need_tid = need_tid;
> +	spin_unlock_irqrestore(&ctrl->tx.lock, flags);
> +
> +	return ctrl->tx.base + (idx * ctrl->tx.sl_sz);
> +}
> +EXPORT_SYMBOL_GPL(slim_get_tx);

The rx calls seem ok that is basically the controller's job to
call those, but with these two calls it seems sometimes the
framework calls them sometimes the controller driver has to. Is
there anyway we can simplify that a bit? Or at least include some
documentation as to when the controller should call them.

> diff --git a/include/linux/slimbus.h b/include/linux/slimbus.h
> index b5064b6..080d86a 100644
> --- a/include/linux/slimbus.h
> +++ b/include/linux/slimbus.h
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/device.h>
>  #include <linux/mutex.h>
> +#include <linux/semaphore.h>
>  #include <linux/mod_devicetable.h>
>  
>  /**
> @@ -34,6 +35,9 @@ extern struct bus_type slimbus_type;
>  #define SLIM_FRM_SLOTS_PER_SUPERFRAME	16
>  #define SLIM_GDE_SLOTS_PER_SUPERFRAME	2
>  
> +/* MAX in-flight transactions neededing transaction ID (8-bit, per spec) */

s/neededing/needing/

> +
> +/* Frequently used message transaction structures */
> +#define DEFINE_SLIM_LDEST_TXN(name, mc, rl, la, msg) \
> +	struct slim_msg_txn name = { rl, 0, mc, SLIM_MSG_DEST_LOGICALADDR, 0,\
> +					0, la, msg, }
> +
> +#define DEFINE_SLIM_BCAST_TXN(name, mc, rl, la, msg) \
> +	struct slim_msg_txn name = { rl, 0, mc, SLIM_MSG_DEST_BROADCAST, 0,\
> +					0, la, msg, }

If the LA isn't used in broadcast messages wouldn't it be simpler
to set it to a fixed value in this macro?

> +
> +#define DEFINE_SLIM_EDEST_TXN(name, mc, rl, la, msg) \
> +	struct slim_msg_txn name = { rl, 0, mc, SLIM_MSG_DEST_ENUMADDR, 0,\
> +					0, la, msg, }
> +

Also one final overall comment this commit contains a lot of two
and three letter abreviations that are not always clear. I would
probably suggest expanding a few of the less standard ones out to
make the code a little easier to follow.

Thanks,
Charles
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 10/10/17 13:19, Charles Keepax wrote:
> On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@linaro.org wrote:
>> From: Sagar Dharia <sdharia@codeaurora.org>
>>
>> Slimbus devices use value-element, and information elements to
>> control device parameters (e.g. value element is used to represent
>> gain for codec, information element is used to represent interrupt
>> status for codec when codec interrupt fires).
>> Messaging APIs are used to set/get these value and information
>> elements. Slimbus specification uses 8-bit "transaction IDs" for
>> messages where a read-value is anticipated. Framework uses a table
>> of pointers to store those TIDs and responds back to the caller in
>> O(1).
>> Caller can opt to do synchronous, or asynchronous reads/writes. For
>> asynchronous operations, the callback will be called from atomic
>> context.
>> TX and RX circular rings are used to allow queuing of multiple
>> transfers per controller. Controller can choose size of these rings
>> based of controller HW implementation. The buffers are coerently
>> mapped so that controller can utilize DMA operations for the
>> transactions without remapping every transaction buffer.
>> Statically allocated rings help to improve performance by avoiding
>> overhead of dynamically allocating transactions on need basis.
>>
>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>> Tested-by: Naveen Kaje <nkaje@codeaurora.org>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/slimbus/Makefile         |   2 +-
>>   drivers/slimbus/slim-core.c      |  15 ++
>>   drivers/slimbus/slim-messaging.c | 471 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/slimbus.h          | 161 +++++++++++++
>>   4 files changed, 648 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/slimbus/slim-messaging.c
>>
>> +/**
>> + * slim_processtxn: Process a slimbus-messaging transaction
>> + * @ctrl: Controller handle
>> + * @txn: Transaction to be sent over SLIMbus
>> + * Called by controller to transmit messaging transactions not dealing with
>> + * Interface/Value elements. (e.g. transmittting a message to assign logical
>> + * address to a slave device
>> + * Returns:
>> + * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
>> + *	not being clocked or driven by controller)
>> + * -ENOTCONN: If the transmitted message was not ACKed by destination device.
>> + */
>> +int slim_processtxn(struct slim_controller *ctrl,
>> +				struct slim_msg_txn *txn)
> 
> Can all go on one line.
Thats true, Will fix it in next version.
> 
>> +{
>> +	int ret, i = 0;
>> +	unsigned long flags;
>> +	u8 *buf;
>> +	bool async = false;
>> +	struct slim_cb_data cbd;
>> +	DECLARE_COMPLETION_ONSTACK(done);
>> +	bool need_tid = slim_tid_txn(txn->mt, txn->mc);
>> +
>> +	if (!txn->msg->comp_cb) {
>> +		txn->msg->comp_cb = slim_sync_default_cb;
>> +		cbd.comp = &done;
>> +		txn->msg->ctx = &cbd;
>> +	} else {
>> +		async = true;
>> +	}
>> +
>> +	buf = slim_get_tx(ctrl, txn, need_tid);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	if (need_tid) {
>> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
>> +		for (i = 0; i < ctrl->last_tid; i++) {
>> +			if (ctrl->tid_tbl[i] == NULL)
>> +				break;
>> +		}
>> +		if (i >= ctrl->last_tid) {
>> +			if (ctrl->last_tid == (SLIM_MAX_TIDS - 1)) {
>> +				spin_unlock_irqrestore(&ctrl->txn_lock, flags);
>> +				slim_return_tx(ctrl, -ENOMEM);
>> +				return -ENOMEM;
>> +			}
>> +			ctrl->last_tid++;
>> +		}
>> +		ctrl->tid_tbl[i] = txn->msg;
>> +		txn->tid = i;
>> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
>> +	}
>> +
>> +	ret = ctrl->xfer_msg(ctrl, txn, buf);
>> +
>> +	if (!ret && !async) { /* sync transaction */
>> +		/* Fine-tune calculation after bandwidth management */
>> +		unsigned long ms = txn->rl + 100;
>> +
>> +		ret = wait_for_completion_timeout(&done,
>> +						  msecs_to_jiffies(ms));
>> +		if (!ret)
>> +			slim_return_tx(ctrl, -ETIMEDOUT);
>> +
>> +		ret = cbd.ret;
>> +	}
>> +
>> +	if (ret && need_tid) {
>> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
>> +		/* Invalidate the transaction */
>> +		ctrl->tid_tbl[txn->tid] = NULL;
>> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
>> +	}
>> +	if (ret)
>> +		dev_err(&ctrl->dev, "Tx:MT:0x%x, MC:0x%x, LA:0x%x failed:%d\n",
>> +			txn->mt, txn->mc, txn->la, ret);
>> +	if (!async) {
>> +		txn->msg->comp_cb = NULL;
>> +		txn->msg->ctx = NULL;
>> +	}
> 
> What is the intent of this if statement here? We set async
> locally so this code only runs if we executed the else on the if
> statement at the top. If its just clearing anything the user
> might have put in these fields why not do it up there.
Its clearing the temporary callback and context field when user wants to 
read/write on simbus synchronously.

If async is false user should not put anything in comp_cb or ctx.
comp_cb and ctx are only used when drivers are doing asynchronous 
read/writes on slimbus. Completion of those are indicated by comp_cb() 
with context.



> 
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(slim_processtxn);
>> +
>> +int slim_xfer_msg(struct slim_controller *ctrl,
>> +			struct slim_device *sbdev, struct slim_val_inf *msg,
>> +			u8 mc)
>> +{
>> +	DEFINE_SLIM_LDEST_TXN(txn_stack, mc, 6, sbdev->laddr, msg);
>> +	struct slim_msg_txn *txn = &txn_stack;
>> +	int ret;
>> +	u16 sl, cur;
>> +
>> +	ret = slim_val_inf_sanity(ctrl, msg, mc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sl = slim_slicesize(msg->num_bytes);
>> +
>> +	dev_dbg(&ctrl->dev, "SB xfer msg:os:%x, len:%d, MC:%x, sl:%x\n",
>> +		msg->start_offset, msg->num_bytes, mc, sl);
>> +
>> +	cur = slim_slicecodefromsize(sl);
> 
> cur should probably be removed until it is needed.
Yep.

> 
>> +	txn->ec = ((sl | (1 << 3)) | ((msg->start_offset & 0xFFF) << 4));
>> +
>> +	switch (mc) {
>> +	case SLIM_MSG_MC_REQUEST_CHANGE_VALUE:
>> +	case SLIM_MSG_MC_CHANGE_VALUE:
>> +	case SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION:
>> +	case SLIM_MSG_MC_CLEAR_INFORMATION:
>> +		txn->rl += msg->num_bytes;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	if (slim_tid_txn(txn->mt, txn->mc))
>> +		txn->rl++;
>> +
>> +	return slim_processtxn(ctrl, txn);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_xfer_msg);
>> +
>> +/* Message APIs Unicast message APIs used by slimbus slave drivers */
>> +
>> +/*
>> + * slim_request_val_element: request value element
>> + * @sb: client handle requesting elemental message reads, writes.
>> + * @msg: Input structure for start-offset, number of bytes to read.
>> + * context: can sleep
>> + * Returns:
>> + * -EINVAL: Invalid parameters
>> + * -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
>> + *	not being clocked or driven by controller)
>> + * -ENOTCONN: If the transmitted message was not ACKed by destination device.
>> + */
>> +int slim_request_val_element(struct slim_device *sb,
>> +				struct slim_val_inf *msg)
>> +{
>> +	struct slim_controller *ctrl = sb->ctrl;
>> +
>> +	if (!ctrl)
>> +		return -EINVAL;
> 
> You could put this check into slim_xfer_msg and save duplicating
> it. Would also then cover cases that call that function directly,
> also would let you make these functions either inlines or macros.

I will give that a try in next version.

> 
>> +
>> +	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_VALUE);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_request_val_element);
>> +
>> +/* Functions to get/return TX, RX buffers for messaging. */
>> +
>> +/**
>> + * slim_get_rx: To get RX buffers for messaging.
>> + * @ctrl: Controller handle
>> + * These functions are called by controller to process the RX buffers.
>> + * RX buffer is requested by controller when data is received from HW, but is
>> + * not processed (e.g. 'report-present message was sent by HW in ISR and SW
>> + * needs more time to process the buffer to assign Logical Address)
>> + * RX buffer is returned back to the pool when associated RX action
>> + * is taken (e.g. Received message is decoded and client's
>> + * response buffer is filled in.)
>> + */
>> +void *slim_get_rx(struct slim_controller *ctrl)
>> +{
>> +	unsigned long flags;
>> +	int idx;
>> +
>> +	spin_lock_irqsave(&ctrl->rx.lock, flags);
>> +	if ((ctrl->rx.tail + 1) % ctrl->rx.n == ctrl->rx.head) {
>> +		spin_unlock_irqrestore(&ctrl->rx.lock, flags);
>> +		dev_err(&ctrl->dev, "RX QUEUE full!");
>> +		return NULL;
>> +	}
>> +	idx = ctrl->rx.tail;
>> +	ctrl->rx.tail = (ctrl->rx.tail + 1) % ctrl->rx.n;
>> +	spin_unlock_irqrestore(&ctrl->rx.lock, flags);
>> +
>> +	return ctrl->rx.base + (idx * ctrl->rx.sl_sz);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_get_rx);
>> +
>> +int slim_return_rx(struct slim_controller *ctrl, void *buf)
>> +{
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&ctrl->rx.lock, flags);
>> +	if (ctrl->rx.tail == ctrl->rx.head) {
>> +		spin_unlock_irqrestore(&ctrl->rx.lock, flags);
>> +		return -ENODATA;
>> +	}
>> +	memcpy(buf, ctrl->rx.base + (ctrl->rx.head * ctrl->rx.sl_sz),
>> +				ctrl->rx.sl_sz);
>> +	ctrl->rx.head = (ctrl->rx.head + 1) % ctrl->rx.n;
>> +	spin_unlock_irqrestore(&ctrl->rx.lock, flags);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(slim_return_rx);
> 
> I find the combination of get/return a bit odd, would get/put
> maybe more idiomatic? Also the return could use some kernel doc.

If that makes it more readable I can rename the functions as suggested, 
and I will also add kernel doc in next version.

> 
>> +
>> +void slim_return_tx(struct slim_controller *ctrl, int err)
>> +{
>> +	unsigned long flags;
>> +	int idx;
>> +	struct slim_pending cur;
>> +
>> +	spin_lock_irqsave(&ctrl->tx.lock, flags);
>> +	idx = ctrl->tx.head;
>> +	ctrl->tx.head = (ctrl->tx.head + 1) % ctrl->tx.n;
>> +	cur = ctrl->pending_wr[idx];
>> +	spin_unlock_irqrestore(&ctrl->tx.lock, flags);
>> +
>> +	if (!cur.cb)
>> +		dev_err(&ctrl->dev, "NULL Transaction or completion");
>> +	else
>> +		cur.cb(cur.ctx, err);
>> +
>> +	up(&ctrl->tx_sem);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_return_tx);
>> +
>> +/**
>> + * slim_get_tx: To get TX buffers for messaging.
>> + * @ctrl: Controller handle
>> + * These functions are called by controller to process the TX buffers.
>> + * TX buffer is requested by controller when it's filled-in and sent to the
>> + * HW. When HW has finished processing this buffer, controller should return it
>> + * back to the pool.
>> + */
>> +void *slim_get_tx(struct slim_controller *ctrl, struct slim_msg_txn *txn,
>> +		bool need_tid)
>> +{
>> +	unsigned long flags;
>> +	int ret, idx;
>> +
>> +	ret = down_interruptible(&ctrl->tx_sem);
>> +	if (ret < 0) {
>> +		dev_err(&ctrl->dev, "TX semaphore down returned:%d", ret);
>> +		return NULL;
>> +	}
>> +	spin_lock_irqsave(&ctrl->tx.lock, flags);
>> +
>> +	if (((ctrl->tx.head + 1) % ctrl->tx.n) == ctrl->tx.tail) {
>> +		spin_unlock_irqrestore(&ctrl->tx.lock, flags);
>> +		dev_err(&ctrl->dev, "controller TX buf unavailable");
>> +		up(&ctrl->tx_sem);
>> +		return NULL;
>> +	}
>> +	idx = ctrl->tx.tail;
>> +	ctrl->tx.tail = (ctrl->tx.tail + 1) % ctrl->tx.n;
>> +	ctrl->pending_wr[idx].cb = txn->msg->comp_cb;
>> +	ctrl->pending_wr[idx].ctx = txn->msg->ctx;
>> +	ctrl->pending_wr[idx].need_tid = need_tid;
>> +	spin_unlock_irqrestore(&ctrl->tx.lock, flags);
>> +
>> +	return ctrl->tx.base + (idx * ctrl->tx.sl_sz);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_get_tx);
> 
> The rx calls seem ok that is basically the controller's job to
> call those, but with these two calls it seems sometimes the
> framework calls them sometimes the controller driver has to. Is
> there anyway we can simplify that a bit? Or at least include some
> documentation as to when the controller should call them.

I will try to do add some details in the document in next version.

> 
>> diff --git a/include/linux/slimbus.h b/include/linux/slimbus.h
>> index b5064b6..080d86a 100644
>> --- a/include/linux/slimbus.h
>> +++ b/include/linux/slimbus.h
>> @@ -15,6 +15,7 @@
>>   #include <linux/module.h>
>>   #include <linux/device.h>
>>   #include <linux/mutex.h>
>> +#include <linux/semaphore.h>
>>   #include <linux/mod_devicetable.h>
>>   
>>   /**
>> @@ -34,6 +35,9 @@ extern struct bus_type slimbus_type;
>>   #define SLIM_FRM_SLOTS_PER_SUPERFRAME	16
>>   #define SLIM_GDE_SLOTS_PER_SUPERFRAME	2
>>   
>> +/* MAX in-flight transactions neededing transaction ID (8-bit, per spec) */
> 
> s/neededing/needing/
> 
Will fix this in next version.
>> +
>> +/* Frequently used message transaction structures */
>> +#define DEFINE_SLIM_LDEST_TXN(name, mc, rl, la, msg) \
>> +	struct slim_msg_txn name = { rl, 0, mc, SLIM_MSG_DEST_LOGICALADDR, 0,\
>> +					0, la, msg, }
>> +
>> +#define DEFINE_SLIM_BCAST_TXN(name, mc, rl, la, msg) \
>> +	struct slim_msg_txn name = { rl, 0, mc, SLIM_MSG_DEST_BROADCAST, 0,\
>> +					0, la, msg, }
> 
> If the LA isn't used in broadcast messages wouldn't it be simpler
> to set it to a fixed value in this macro?
> 
Yep, if the destination type is broadcast we should not set la or ea in 
the header. may be set 0 here.

>> +
>> +#define DEFINE_SLIM_EDEST_TXN(name, mc, rl, la, msg) \
>> +	struct slim_msg_txn name = { rl, 0, mc, SLIM_MSG_DEST_ENUMADDR, 0,\
>> +					0, la, msg, }
>> +
> 
> Also one final overall comment this commit contains a lot of two
> and three letter abreviations that are not always clear. I would
> probably suggest expanding a few of the less standard ones out to
> make the code a little easier to follow.
Will do that!!

thanks
srini
> 
> Thanks,
> Charles
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@linaro.org wrote:

>  	mutex_init(&ctrl->m_ctrl);
> +	spin_lock_init(&ctrl->tx.lock);
> +	spin_lock_init(&ctrl->rx.lock);

locks galore :) My assumption is that you want to optimize these? But given
that audio user is going to be serialized do we practically need two locks?

> +
> +	ctrl->pending_wr = kcalloc((ctrl->tx.n - 1),
> +				   sizeof(struct slim_pending),
> +				   GFP_KERNEL);
> +	if (!ctrl->pending_wr) {
> +		ret = -ENOMEM;
> +		goto wr_alloc_failed;
> +	}
> +
> +	sema_init(&ctrl->tx_sem, (ctrl->tx.n - 1));

i though v5 comment from Arnd was not to use semaphores..

> +/* Copyright (c) 2011-2016, The Linux Foundation. All rights reserved.

2017?

> +int slim_processtxn(struct slim_controller *ctrl,

slim_process_txn seems more readable to me

> +				struct slim_msg_txn *txn)
> +{
> +	int ret, i = 0;
> +	unsigned long flags;
> +	u8 *buf;
> +	bool async = false;
> +	struct slim_cb_data cbd;
> +	DECLARE_COMPLETION_ONSTACK(done);
> +	bool need_tid = slim_tid_txn(txn->mt, txn->mc);
> +
> +	if (!txn->msg->comp_cb) {
> +		txn->msg->comp_cb = slim_sync_default_cb;
> +		cbd.comp = &done;
> +		txn->msg->ctx = &cbd;
> +	} else {
> +		async = true;
> +	}
> +
> +	buf = slim_get_tx(ctrl, txn, need_tid);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (need_tid) {
> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
> +		for (i = 0; i < ctrl->last_tid; i++) {
> +			if (ctrl->tid_tbl[i] == NULL)
> +				break;
> +		}
> +		if (i >= ctrl->last_tid) {
> +			if (ctrl->last_tid == (SLIM_MAX_TIDS - 1)) {
> +				spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +				slim_return_tx(ctrl, -ENOMEM);
> +				return -ENOMEM;
> +			}
> +			ctrl->last_tid++;
> +		}
> +		ctrl->tid_tbl[i] = txn->msg;
> +		txn->tid = i;
> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +	}
> +
> +	ret = ctrl->xfer_msg(ctrl, txn, buf);
> +
> +	if (!ret && !async) { /* sync transaction */
> +		/* Fine-tune calculation after bandwidth management */
> +		unsigned long ms = txn->rl + 100;
> +
> +		ret = wait_for_completion_timeout(&done,
> +						  msecs_to_jiffies(ms));
> +		if (!ret)
> +			slim_return_tx(ctrl, -ETIMEDOUT);
> +
> +		ret = cbd.ret;
> +	}
> +
> +	if (ret && need_tid) {
> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
> +		/* Invalidate the transaction */
> +		ctrl->tid_tbl[txn->tid] = NULL;
> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +	}
> +	if (ret)
> +		dev_err(&ctrl->dev, "Tx:MT:0x%x, MC:0x%x, LA:0x%x failed:%d\n",
> +			txn->mt, txn->mc, txn->la, ret);
> +	if (!async) {
> +		txn->msg->comp_cb = NULL;
> +		txn->msg->ctx = NULL;
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(slim_processtxn);

that is interesting, I was expecting this to be internal API. So users are
expected to use this which is not very convenient IMO. Can we hide the gory
details and give users simple tx/rx or read/write APIs. FWIW most of the
usage would be thru regmap where people would call regmap_read/write()

> +
> +static int slim_val_inf_sanity(struct slim_controller *ctrl,
> +			       struct slim_val_inf *msg, u8 mc)
> +{
> +	if (!msg || msg->num_bytes > 16 ||
> +	    (msg->start_offset + msg->num_bytes) > 0xC00)
> +		goto reterr;

line break here

> +	switch (mc) {
> +	case SLIM_MSG_MC_REQUEST_VALUE:
> +	case SLIM_MSG_MC_REQUEST_INFORMATION:

what does MC refer to?

> +		if (msg->rbuf != NULL)
> +			return 0;
> +		break;

after each break too

> +static u16 slim_slicecodefromsize(u16 req)

hmmm Linux code doesnt prefernamesnames like this :)

> +EXPORT_SYMBOL_GPL(slim_request_inf_element);
> +
> +

unnecessary double space

> +struct slim_val_inf {
> +	u16			start_offset;
> +	u8			num_bytes;
> +	u8			*rbuf;
> +	const u8		*wbuf;

can we do read and write, if not it can be a buf which maybe rbuf or wbug
based on type

On Wed, Oct 11, 2017 at 6:38 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@linaro.org wrote:
>> +     ctrl->pending_wr = kcalloc((ctrl->tx.n - 1),
>> +                                sizeof(struct slim_pending),
>> +                                GFP_KERNEL);
>> +     if (!ctrl->pending_wr) {
>> +             ret = -ENOMEM;
>> +             goto wr_alloc_failed;
>> +     }
>> +
>> +     sema_init(&ctrl->tx_sem, (ctrl->tx.n - 1));
>
> i though v5 comment from Arnd was not to use semaphores..

Right, these need to go away. Thanks for spotting it!

     Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 11/10/17 05:38, Vinod Koul wrote:
> On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@linaro.org wrote:
> 
>>   	mutex_init(&ctrl->m_ctrl);
>> +	spin_lock_init(&ctrl->tx.lock);
>> +	spin_lock_init(&ctrl->rx.lock);
> 
> locks galore :) My assumption is that you want to optimize these? But given
> that audio user is going to be serialized do we practically need two locks?
> 
If we remove the locking, It will be issue if we have multiple devices 
in a component, which is common atleast with the codec which am looking at.


>> +
>> +	ctrl->pending_wr = kcalloc((ctrl->tx.n - 1),
>> +				   sizeof(struct slim_pending),
>> +				   GFP_KERNEL);
>> +	if (!ctrl->pending_wr) {
>> +		ret = -ENOMEM;
>> +		goto wr_alloc_failed;
>> +	}
>> +
>> +	sema_init(&ctrl->tx_sem, (ctrl->tx.n - 1));
> 
> i though v5 comment from Arnd was not to use semaphores..

I will revisit this area once again and get rid of this semaphore all 
together in next version.

> 
>> +/* Copyright (c) 2011-2016, The Linux Foundation. All rights reserved.
> 
> 2017?
Yep.
> 
>> +int slim_processtxn(struct slim_controller *ctrl,
> 
> slim_process_txn seems more readable to me
> 
I can change it in next version.

>> +				struct slim_msg_txn *txn)
>> +{
>> +	int ret, i = 0;
>> +	unsigned long flags;
>> +	u8 *buf;
>> +	bool async = false;
>> +	struct slim_cb_data cbd;
>> +	DECLARE_COMPLETION_ONSTACK(done);
>> +	bool need_tid = slim_tid_txn(txn->mt, txn->mc);
>> +
>> +	if (!txn->msg->comp_cb) {
>> +		txn->msg->comp_cb = slim_sync_default_cb;
>> +		cbd.comp = &done;
>> +		txn->msg->ctx = &cbd;
>> +	} else {
>> +		async = true;
>> +	}
>> +
>> +	buf = slim_get_tx(ctrl, txn, need_tid);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	if (need_tid) {
>> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
>> +		for (i = 0; i < ctrl->last_tid; i++) {
>> +			if (ctrl->tid_tbl[i] == NULL)
>> +				break;
>> +		}
>> +		if (i >= ctrl->last_tid) {
>> +			if (ctrl->last_tid == (SLIM_MAX_TIDS - 1)) {
>> +				spin_unlock_irqrestore(&ctrl->txn_lock, flags);
>> +				slim_return_tx(ctrl, -ENOMEM);
>> +				return -ENOMEM;
>> +			}
>> +			ctrl->last_tid++;
>> +		}
>> +		ctrl->tid_tbl[i] = txn->msg;
>> +		txn->tid = i;
>> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
>> +	}
>> +
>> +	ret = ctrl->xfer_msg(ctrl, txn, buf);
>> +
>> +	if (!ret && !async) { /* sync transaction */
>> +		/* Fine-tune calculation after bandwidth management */
>> +		unsigned long ms = txn->rl + 100;
>> +
>> +		ret = wait_for_completion_timeout(&done,
>> +						  msecs_to_jiffies(ms));
>> +		if (!ret)
>> +			slim_return_tx(ctrl, -ETIMEDOUT);
>> +
>> +		ret = cbd.ret;
>> +	}
>> +
>> +	if (ret && need_tid) {
>> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
>> +		/* Invalidate the transaction */
>> +		ctrl->tid_tbl[txn->tid] = NULL;
>> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
>> +	}
>> +	if (ret)
>> +		dev_err(&ctrl->dev, "Tx:MT:0x%x, MC:0x%x, LA:0x%x failed:%d\n",
>> +			txn->mt, txn->mc, txn->la, ret);
>> +	if (!async) {
>> +		txn->msg->comp_cb = NULL;
>> +		txn->msg->ctx = NULL;
>> +	}
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(slim_processtxn);
> 
> that is interesting, I was expecting this to be internal API. So users are
> expected to use this which is not very convenient IMO. Can we hide the gory
> details and give users simple tx/rx or read/write APIs. FWIW most of the
> usage would be thru regmap where people would call regmap_read/write()
> 

Currently the only user of the api is qcom slim controller. May be the 
reason for it to use this api is to fit in with all the locking and 
sequencing mechanism with in the core.

I will be revisiting the semaphore thingy before I send next version, 
hopefully I can to align with your above comments.



>> +
>> +static int slim_val_inf_sanity(struct slim_controller *ctrl,
>> +			       struct slim_val_inf *msg, u8 mc)
>> +{
>> +	if (!msg || msg->num_bytes > 16 ||
>> +	    (msg->start_offset + msg->num_bytes) > 0xC00)
>> +		goto reterr;
> 
> line break here

I agree.

> 
>> +	switch (mc) {
>> +	case SLIM_MSG_MC_REQUEST_VALUE:
>> +	case SLIM_MSG_MC_REQUEST_INFORMATION:
> 
> what does MC refer to?

Message Code.

> 
>> +		if (msg->rbuf != NULL)
>> +			return 0;
>> +		break;
> 
> after each break too
> 
Sure, will fix it in next version.

>> +static u16 slim_slicecodefromsize(u16 req)
> 
> hmmm Linux code doesnt prefernamesnames like this :)
Yep, this function is unused, am going to delete this in next version.
> 
>> +EXPORT_SYMBOL_GPL(slim_request_inf_element);
>> +
>> +
> 
> unnecessary double space
> 
>> +struct slim_val_inf {
>> +	u16			start_offset;
>> +	u8			num_bytes;
>> +	u8			*rbuf;
>> +	const u8		*wbuf;
> 
> can we do read and write, if not it can be a buf which maybe rbuf or wbug
> based on type
With REQUEST_CHANGE_VALUE single command we can read old value at the 
same time we can write new value.

> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Oct 11, 2017 at 10:42:28AM +0100, Srinivas Kandagatla wrote:
> On 11/10/17 05:38, Vinod Koul wrote:
> >On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@linaro.org wrote:
> >
> >>  	mutex_init(&ctrl->m_ctrl);
> >>+	spin_lock_init(&ctrl->tx.lock);
> >>+	spin_lock_init(&ctrl->rx.lock);
> >
> >locks galore :) My assumption is that you want to optimize these? But given
> >that audio user is going to be serialized do we practically need two locks?
> >
> If we remove the locking, It will be issue if we have multiple devices in a
> component, which is common atleast with the codec which am looking at.

can you explian what you mean by a "device" here?

> >>+	switch (mc) {
> >>+	case SLIM_MSG_MC_REQUEST_VALUE:
> >>+	case SLIM_MSG_MC_REQUEST_INFORMATION:
> >
> >what does MC refer to?
> 
> Message Code.

isnt SLIM_MSG enough :D I think we cna get rid of MC here..

> >>+struct slim_val_inf {
> >>+	u16			start_offset;
> >>+	u8			num_bytes;
> >>+	u8			*rbuf;
> >>+	const u8		*wbuf;
> >
> >can we do read and write, if not it can be a buf which maybe rbuf or wbug
> >based on type
> With REQUEST_CHANGE_VALUE single command we can read old value at the same
> time we can write new value.

so that is a read modify write, correct? Is that implemented in HW, if so we
need to provide only write value

On 11/10/17 11:24, Vinod Koul wrote:
> On Wed, Oct 11, 2017 at 10:42:28AM +0100, Srinivas Kandagatla wrote:
>> On 11/10/17 05:38, Vinod Koul wrote:
>>> On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@linaro.org wrote:
>>>
>>>>   	mutex_init(&ctrl->m_ctrl);
>>>> +	spin_lock_init(&ctrl->tx.lock);
>>>> +	spin_lock_init(&ctrl->rx.lock);
>>>
>>> locks galore :) My assumption is that you want to optimize these? But given
>>> that audio user is going to be serialized do we practically need two locks?
>>>
>> If we remove the locking, It will be issue if we have multiple devices in a
>> component, which is common atleast with the codec which am looking at.
> 
> can you explian what you mean by a "device" here?

SLIMbus component contain two or more SLIMbus devices,
like function(Generic) device, interface device.
Interface device provides bus management services, where as generic 
device provides more of application specific functionality like ADC/DAC..


> 
>>>> +	switch (mc) {
>>>> +	case SLIM_MSG_MC_REQUEST_VALUE:
>>>> +	case SLIM_MSG_MC_REQUEST_INFORMATION:
>>>
>>> what does MC refer to?
>>
>> Message Code.
> 
> isnt SLIM_MSG enough :D I think we cna get rid of MC here..
> 
>>>> +struct slim_val_inf {
>>>> +	u16			start_offset;
>>>> +	u8			num_bytes;
>>>> +	u8			*rbuf;
>>>> +	const u8		*wbuf;
>>>
>>> can we do read and write, if not it can be a buf which maybe rbuf or wbug
>>> based on type
>> With REQUEST_CHANGE_VALUE single command we can read old value at the same
>> time we can write new value.
> 
> so that is a read modify write, correct? Is that implemented in HW, if so we
> need to provide only write value

Its not really a read-modify-write,
REQUEST_CHANGE_VALUE/REQUEST_CLEAR_INFORMATION commands are part of 
SLIMbus Spec.
We need provide write value + buffer for read value to store.

all REQUEST_CHANGE_VALUE cmd do is this in single operation:

1> save the old value
2> update new value from wbuf
3> return the saved value from step 1, into rbuf

Not sure what is the real usecase for this, I have not seen its usage in 
any Qualcomm downstream code.

May be it can be used to implement some class of atomic ops.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandagatla@linaro.org wrote:

> From: Sagar Dharia <sdharia@codeaurora.org>
> 
> Slimbus devices use value-element, and information elements to
> control device parameters (e.g. value element is used to represent
> gain for codec, information element is used to represent interrupt
> status for codec when codec interrupt fires).
> Messaging APIs are used to set/get these value and information
> elements. Slimbus specification uses 8-bit "transaction IDs" for
> messages where a read-value is anticipated. Framework uses a table
> of pointers to store those TIDs and responds back to the caller in
> O(1).

I think we can implement this "optimization" with less complex code,
regardless I don't think we need to mention this in the commit
message...

[..]
> diff --git a/drivers/slimbus/slim-messaging.c b/drivers/slimbus/slim-messaging.c
[..]
> +/**
> + * slim_msg_response: Deliver Message response received from a device to the
> + *	framework.
> + * @ctrl: Controller handle
> + * @reply: Reply received from the device
> + * @len: Length of the reply
> + * @tid: Transaction ID received with which framework can associate reply.
> + * Called by controller to inform framework about the response received.
> + * This helps in making the API asynchronous, and controller-driver doesn't need
> + * to manage 1 more table other than the one managed by framework mapping TID
> + * with buffers
> + */
> +void slim_msg_response(struct slim_controller *ctrl, u8 *reply, u8 tid, u8 len)

Even if tid and len comes from the spec I recommend you making them int
and size_t.

> +{
> +	struct slim_val_inf *msg;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ctrl->txn_lock, flags);
> +	msg = ctrl->tid_tbl[tid];
> +	if (msg == NULL || msg->rbuf == NULL) {

if (!msg || !msg->rbuf)


When is it valid to add a transaction to tid_tbl with msg->rbuf = NULL?
Should we reject it earlier?

> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +		dev_err(&ctrl->dev, "Got response to invalid TID:%d, len:%d\n",
> +				tid, len);
> +		return;
> +	}
> +	ctrl->tid_tbl[tid] = NULL;
> +	spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +
> +	memcpy(msg->rbuf, reply, len);
> +	if (msg->comp_cb)
> +		msg->comp_cb(msg->ctx, 0);
> +}
> +EXPORT_SYMBOL_GPL(slim_msg_response);
[..]
> +int slim_processtxn(struct slim_controller *ctrl,
> +				struct slim_msg_txn *txn)
> +{
> +	int ret, i = 0;
> +	unsigned long flags;
> +	u8 *buf;
> +	bool async = false;
> +	struct slim_cb_data cbd;
> +	DECLARE_COMPLETION_ONSTACK(done);
> +	bool need_tid = slim_tid_txn(txn->mt, txn->mc);
> +
> +	if (!txn->msg->comp_cb) {
> +		txn->msg->comp_cb = slim_sync_default_cb;
> +		cbd.comp = &done;
> +		txn->msg->ctx = &cbd;
> +	} else {
> +		async = true;
> +	}
> +
> +	buf = slim_get_tx(ctrl, txn, need_tid);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (need_tid) {
> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
> +		for (i = 0; i < ctrl->last_tid; i++) {
> +			if (ctrl->tid_tbl[i] == NULL)
> +				break;
> +		}
> +		if (i >= ctrl->last_tid) {
> +			if (ctrl->last_tid == (SLIM_MAX_TIDS - 1)) {
> +				spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +				slim_return_tx(ctrl, -ENOMEM);
> +				return -ENOMEM;
> +			}
> +			ctrl->last_tid++;
> +		}
> +		ctrl->tid_tbl[i] = txn->msg;
> +		txn->tid = i;
> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +	}
> +
> +	ret = ctrl->xfer_msg(ctrl, txn, buf);
> +
> +	if (!ret && !async) { /* sync transaction */
> +		/* Fine-tune calculation after bandwidth management */
> +		unsigned long ms = txn->rl + 100;
> +
> +		ret = wait_for_completion_timeout(&done,
> +						  msecs_to_jiffies(ms));
> +		if (!ret)
> +			slim_return_tx(ctrl, -ETIMEDOUT);
> +
> +		ret = cbd.ret;
> +	}
> +
> +	if (ret && need_tid) {
> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
> +		/* Invalidate the transaction */
> +		ctrl->tid_tbl[txn->tid] = NULL;
> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
> +	}
> +	if (ret)
> +		dev_err(&ctrl->dev, "Tx:MT:0x%x, MC:0x%x, LA:0x%x failed:%d\n",
> +			txn->mt, txn->mc, txn->la, ret);

if (ret) {
	if (need_tid)
		drop();
	
	dev_err();
}

Would probably make this a little bit cleaner...

> +	if (!async) {
> +		txn->msg->comp_cb = NULL;
> +		txn->msg->ctx = NULL;

I believe txn->msg is always required, so you don't need to do this
contidionally.

> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(slim_processtxn);
[..]
> +int slim_request_val_element(struct slim_device *sb,
> +				struct slim_val_inf *msg)
> +{
> +	struct slim_controller *ctrl = sb->ctrl;
> +
> +	if (!ctrl)
> +		return -EINVAL;

>From patch 1 I believe it's invalid for sb->ctrl to be NULL, so there
shouldn't be a need to check this.

> +
> +	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_VALUE);
> +}
> +EXPORT_SYMBOL_GPL(slim_request_val_element);
[..]
> +int slim_return_rx(struct slim_controller *ctrl, void *buf)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ctrl->rx.lock, flags);
> +	if (ctrl->rx.tail == ctrl->rx.head) {
> +		spin_unlock_irqrestore(&ctrl->rx.lock, flags);
> +		return -ENODATA;
> +	}
> +	memcpy(buf, ctrl->rx.base + (ctrl->rx.head * ctrl->rx.sl_sz),
> +				ctrl->rx.sl_sz);
> +	ctrl->rx.head = (ctrl->rx.head + 1) % ctrl->rx.n;
> +	spin_unlock_irqrestore(&ctrl->rx.lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(slim_return_rx);
> +

Please provide kerneldoc for exported symbols.

> +void slim_return_tx(struct slim_controller *ctrl, int err)
> +{
> +	unsigned long flags;
> +	int idx;
> +	struct slim_pending cur;
> +
> +	spin_lock_irqsave(&ctrl->tx.lock, flags);
> +	idx = ctrl->tx.head;
> +	ctrl->tx.head = (ctrl->tx.head + 1) % ctrl->tx.n;
> +	cur = ctrl->pending_wr[idx];

Why is this doing struct copy?

> +	spin_unlock_irqrestore(&ctrl->tx.lock, flags);
> +
> +	if (!cur.cb)
> +		dev_err(&ctrl->dev, "NULL Transaction or completion");
> +	else
> +		cur.cb(cur.ctx, err);
> +
> +	up(&ctrl->tx_sem);
> +}
> +EXPORT_SYMBOL_GPL(slim_return_tx);
[..]
>  /**
> + * struct slim_val_inf: Slimbus value or information element
> + * @start_offset: Specifies starting offset in information/value element map
> + * @num_bytes: upto 16. This ensures that the message will fit the slicesize
> + *		per slimbus spec
> + * @comp_cb: Callback if this read/write is asynchronous
> + * @ctx: Argument for comp_cb
> + */
> +struct slim_val_inf {
> +	u16			start_offset;
> +	u8			num_bytes;
> +	u8			*rbuf;

This is not mentioned in the kerneldoc. Use void * for data buffers.

> +	const u8		*wbuf;

Can a message ever be read and write? Otherwise it should be sufficient
to only have one data pointer.

> +	void			(*comp_cb)(void *ctx, int err);
> +	void			*ctx;
> +};
> +
[..]
> +/**
> + * struct slim_ctrl_buf: circular buffer used by contoller for TX, RX
> + * @base: virtual base address for this buffer
> + * @phy: physical address for this buffer (this is useful if controller can
> + *	  DMA the buffers for TX and RX to/from controller hardware
> + * @lock: lock protecting head and tail
> + * @head: index where buffer is returned back
> + * @tail: index from where buffer is consumed
> + * @sl_sz: byte-size of each slot in this buffer
> + * @n:	  number of elements in this circular ring, note that this needs to be
> + *	1 more than actual buffers to allow for one open slot
> + */

Is this ringbuffer mechanism defined in the slimbus specification? Looks
like something specific to the Qualcomm controller, rather than
something that should be enforced in the framework.

> +struct slim_ctrl_buf {
> +	void		*base;
> +	phys_addr_t	phy;
> +	spinlock_t	lock;
> +	int		head;
> +	int		tail;
> +	int		sl_sz;
> +	int		n;
> +};
[..]
> +/**
>   * struct slim_controller: Controls every instance of SLIMbus
>   *				(similar to 'master' on SPI)
>   *	'Manager device' is responsible for  device management, bandwidth
> @@ -139,6 +246,16 @@ struct slim_addrt {
>   * @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
> + * @tid_tbl: Table of transactions having transaction ID
> + * @txn_lock: Lock to protect table of transactions
> + * @rx: RX buffers used by controller to receive messages. Ctrl may receive more
> + *	than 1 message (e.g. multiple report-present messages or messages from
> + *	multiple slaves).
> + * @tx: TX buffers used by controller to transmit messages. Ctrl may have
> + *	ability to send/queue multiple messages to HW at once.
> + * @pending_wr: Pending write transactions to be acknowledged by controller

This is out list of pending write requests, yet it's implemented as an
array used in a complex ring buffer fashion. Wouldn't it be easier to
just have this as a linked list of slim_pending struct?

> + * @tx_sem: Semaphore for available TX buffers for this controller
> + * @last_tid: Last used entry for TID transactions
>   * @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.
> @@ -161,6 +278,15 @@ struct slim_controller {
>  	struct slim_addrt	*addrt;
>  	u8			num_dev;
>  	struct workqueue_struct *wq;
> +	struct slim_val_inf	*tid_tbl[SLIM_MAX_TIDS];
> +	u8			last_tid;

I suggest that you replace these two with an idr, rather than having a
fixed size array and then last_tid as an optimization to limit how far
you linear search for an empty space.

> +	spinlock_t		txn_lock;
> +	struct slim_ctrl_buf	tx;
> +	struct slim_ctrl_buf	rx;
> +	struct slim_pending	*pending_wr;
> +	struct semaphore	tx_sem;

Please don't use semaphores. If you keep pending_wr as a list you can
use list_empty() instead...

> +	int			(*xfer_msg)(struct slim_controller *ctrl,
> +					    struct slim_msg_txn *tx, void *buf);

I believe buf has fixed size, so please document this.

>  	int			(*set_laddr)(struct slim_controller *ctrl,
>  					     struct slim_eaddr *ea, u8 laddr);
>  	int			(*get_laddr)(struct slim_controller *ctrl,
> @@ -295,5 +421,40 @@ static inline void slim_set_devicedata(struct slim_device *dev, void *data)
>  {
>  	dev_set_drvdata(&dev->dev, data);
>  }

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks for Review Comments,


On 18/10/17 07:15, Bjorn Andersson wrote:
> On Fri 06 Oct 08:51 PDT 2017, srinivas.kandagatla@linaro.org wrote:
> 
>> From: Sagar Dharia <sdharia@codeaurora.org>
>>
>> Slimbus devices use value-element, and information elements to
>> control device parameters (e.g. value element is used to represent
>> gain for codec, information element is used to represent interrupt
>> status for codec when codec interrupt fires).
>> Messaging APIs are used to set/get these value and information
>> elements. Slimbus specification uses 8-bit "transaction IDs" for
>> messages where a read-value is anticipated. Framework uses a table
>> of pointers to store those TIDs and responds back to the caller in
>> O(1).
> 
> I think we can implement this "optimization" with less complex code,
> regardless I don't think we need to mention this in the commit
> message...
> 
> [..]
>> diff --git a/drivers/slimbus/slim-messaging.c b/drivers/slimbus/slim-messaging.c
> [..]
>> +/**
>> + * slim_msg_response: Deliver Message response received from a device to the
>> + *	framework.
>> + * @ctrl: Controller handle
>> + * @reply: Reply received from the device
>> + * @len: Length of the reply
>> + * @tid: Transaction ID received with which framework can associate reply.
>> + * Called by controller to inform framework about the response received.
>> + * This helps in making the API asynchronous, and controller-driver doesn't need
>> + * to manage 1 more table other than the one managed by framework mapping TID
>> + * with buffers
>> + */
>> +void slim_msg_response(struct slim_controller *ctrl, u8 *reply, u8 tid, u8 len)
> 
> Even if tid and len comes from the spec I recommend you making them int
> and size_t.
okay, will give that a go.
> 
>> +{
>> +	struct slim_val_inf *msg;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&ctrl->txn_lock, flags);
>> +	msg = ctrl->tid_tbl[tid];
>> +	if (msg == NULL || msg->rbuf == NULL) {
> 
> if (!msg || !msg->rbuf)
> 
> 
> When is it valid to add a transaction to tid_tbl with msg->rbuf = NULL?
> Should we reject it earlier?

We do sanity checks before posting the request, however there are cases 
where this checks are not in place like calling slim_processtxn() directly.

May be we should add this check all the case
> 
>> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
>> +		dev_err(&ctrl->dev, "Got response to invalid TID:%d, len:%d\n",
>> +				tid, len);
>> +		return;
>> +	}
>> +	ctrl->tid_tbl[tid] = NULL;
>> +	spin_unlock_irqrestore(&ctrl->txn_lock, flags);
>> +
>> +	memcpy(msg->rbuf, reply, len);
>> +	if (msg->comp_cb)
>> +		msg->comp_cb(msg->ctx, 0);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_msg_response);
> [..]
>> +int slim_processtxn(struct slim_controller *ctrl,
>> +				struct slim_msg_txn *txn)
>> +{
>> +	int ret, i = 0;
>> +	unsigned long flags;
>> +	u8 *buf;
>> +	bool async = false;
>> +	struct slim_cb_data cbd;
>> +	DECLARE_COMPLETION_ONSTACK(done);
>> +	bool need_tid = slim_tid_txn(txn->mt, txn->mc);
>> +
>> +	if (!txn->msg->comp_cb) {
>> +		txn->msg->comp_cb = slim_sync_default_cb;
>> +		cbd.comp = &done;
>> +		txn->msg->ctx = &cbd;
>> +	} else {
>> +		async = true;
>> +	}
>> +
>> +	buf = slim_get_tx(ctrl, txn, need_tid);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	if (need_tid) {
>> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
>> +		for (i = 0; i < ctrl->last_tid; i++) {
>> +			if (ctrl->tid_tbl[i] == NULL)
>> +				break;
>> +		}
>> +		if (i >= ctrl->last_tid) {
>> +			if (ctrl->last_tid == (SLIM_MAX_TIDS - 1)) {
>> +				spin_unlock_irqrestore(&ctrl->txn_lock, flags);
>> +				slim_return_tx(ctrl, -ENOMEM);
>> +				return -ENOMEM;
>> +			}
>> +			ctrl->last_tid++;
>> +		}
>> +		ctrl->tid_tbl[i] = txn->msg;
>> +		txn->tid = i;
>> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
>> +	}
>> +
>> +	ret = ctrl->xfer_msg(ctrl, txn, buf);
>> +
>> +	if (!ret && !async) { /* sync transaction */
>> +		/* Fine-tune calculation after bandwidth management */
>> +		unsigned long ms = txn->rl + 100;
>> +
>> +		ret = wait_for_completion_timeout(&done,
>> +						  msecs_to_jiffies(ms));
>> +		if (!ret)
>> +			slim_return_tx(ctrl, -ETIMEDOUT);
>> +
>> +		ret = cbd.ret;
>> +	}
>> +
>> +	if (ret && need_tid) {
>> +		spin_lock_irqsave(&ctrl->txn_lock, flags);
>> +		/* Invalidate the transaction */
>> +		ctrl->tid_tbl[txn->tid] = NULL;
>> +		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
>> +	}
>> +	if (ret)
>> +		dev_err(&ctrl->dev, "Tx:MT:0x%x, MC:0x%x, LA:0x%x failed:%d\n",
>> +			txn->mt, txn->mc, txn->la, ret);
> 
> if (ret) {
> 	if (need_tid)
> 		drop();
> 	
> 	dev_err();
> }
> 
> Would probably make this a little bit cleaner...

I agree.

> 
>> +	if (!async) {
>> +		txn->msg->comp_cb = NULL;
>> +		txn->msg->ctx = NULL;
> 
> I believe txn->msg is always required, so you don't need to do this
> contidionally.

I don't get this, why do you want to set comp_cb to NULL unconditionally?


> 
>> +	}
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(slim_processtxn);
> [..]
>> +int slim_request_val_element(struct slim_device *sb,
>> +				struct slim_val_inf *msg)
>> +{
>> +	struct slim_controller *ctrl = sb->ctrl;
>> +
>> +	if (!ctrl)
>> +		return -EINVAL;
> 
>  From patch 1 I believe it's invalid for sb->ctrl to be NULL, so there
> shouldn't be a need to check this.
> 
yep.

>> +
>> +	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_VALUE);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_request_val_element);
> [..]
>> +int slim_return_rx(struct slim_controller *ctrl, void *buf)
>> +{
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&ctrl->rx.lock, flags);
>> +	if (ctrl->rx.tail == ctrl->rx.head) {
>> +		spin_unlock_irqrestore(&ctrl->rx.lock, flags);
>> +		return -ENODATA;
>> +	}
>> +	memcpy(buf, ctrl->rx.base + (ctrl->rx.head * ctrl->rx.sl_sz),
>> +				ctrl->rx.sl_sz);
>> +	ctrl->rx.head = (ctrl->rx.head + 1) % ctrl->rx.n;
>> +	spin_unlock_irqrestore(&ctrl->rx.lock, flags);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(slim_return_rx);
>> +
> 
> Please provide kerneldoc for exported symbols.
> 

Yes, I will fix this in next version.

>> +void slim_return_tx(struct slim_controller *ctrl, int err)
>> +{
>> +	unsigned long flags;
>> +	int idx;
>> +	struct slim_pending cur;
>> +
>> +	spin_lock_irqsave(&ctrl->tx.lock, flags);
>> +	idx = ctrl->tx.head;
>> +	ctrl->tx.head = (ctrl->tx.head + 1) % ctrl->tx.n;
>> +	cur = ctrl->pending_wr[idx];
> 
> Why is this doing struct copy?
> 
Not sure, do you see any issue with this?

>> +	spin_unlock_irqrestore(&ctrl->tx.lock, flags);
>> +
>> +	if (!cur.cb)
>> +		dev_err(&ctrl->dev, "NULL Transaction or completion");
>> +	else
>> +		cur.cb(cur.ctx, err);
>> +
>> +	up(&ctrl->tx_sem);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_return_tx);
> [..]
>>   /**
>> + * struct slim_val_inf: Slimbus value or information element
>> + * @start_offset: Specifies starting offset in information/value element map
>> + * @num_bytes: upto 16. This ensures that the message will fit the slicesize
>> + *		per slimbus spec
>> + * @comp_cb: Callback if this read/write is asynchronous
>> + * @ctx: Argument for comp_cb
>> + */
>> +struct slim_val_inf {
>> +	u16			start_offset;
>> +	u8			num_bytes;
>> +	u8			*rbuf;
> 
> This is not mentioned in the kerneldoc. Use void * for data buffers.
> 
>> +	const u8		*wbuf;
> 
> Can a message ever be read and write? Otherwise it should be sufficient
> to only have one data pointer.

some of the SLIMBus commands are request_response type, meaning the old 
value is returned and at the same time the new value is updated.
> 
>> +	void			(*comp_cb)(void *ctx, int err);
>> +	void			*ctx;
>> +};
>> +
> [..]
>> +/**
>> + * struct slim_ctrl_buf: circular buffer used by contoller for TX, RX
>> + * @base: virtual base address for this buffer
>> + * @phy: physical address for this buffer (this is useful if controller can
>> + *	  DMA the buffers for TX and RX to/from controller hardware
>> + * @lock: lock protecting head and tail
>> + * @head: index where buffer is returned back
>> + * @tail: index from where buffer is consumed
>> + * @sl_sz: byte-size of each slot in this buffer
>> + * @n:	  number of elements in this circular ring, note that this needs to be
>> + *	1 more than actual buffers to allow for one open slot
>> + */
> 
> Is this ringbuffer mechanism defined in the slimbus specification? Looks
> like something specific to the Qualcomm controller, rather than
> something that should be enforced in the framework.
> 

Yes, this is not part of the slimbus specs, but Qcom SOCs have concept 
of Message Queues.

Are you suggesting that this buffer handling has to be moved out of core 
into controller driver?


>> +struct slim_ctrl_buf {
>> +	void		*base;
>> +	phys_addr_t	phy;
>> +	spinlock_t	lock;
>> +	int		head;
>> +	int		tail;
>> +	int		sl_sz;
>> +	int		n;
>> +};
> [..]
>> +/**
>>    * struct slim_controller: Controls every instance of SLIMbus
>>    *				(similar to 'master' on SPI)
>>    *	'Manager device' is responsible for  device management, bandwidth
>> @@ -139,6 +246,16 @@ struct slim_addrt {
>>    * @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
>> + * @tid_tbl: Table of transactions having transaction ID
>> + * @txn_lock: Lock to protect table of transactions
>> + * @rx: RX buffers used by controller to receive messages. Ctrl may receive more
>> + *	than 1 message (e.g. multiple report-present messages or messages from
>> + *	multiple slaves).
>> + * @tx: TX buffers used by controller to transmit messages. Ctrl may have
>> + *	ability to send/queue multiple messages to HW at once.
>> + * @pending_wr: Pending write transactions to be acknowledged by controller
> 
> This is out list of pending write requests, yet it's implemented as an
> array used in a complex ring buffer fashion. Wouldn't it be easier to
> just have this as a linked list of slim_pending struct?

Yes, its possible to implement this as list, i will give that a try.

> 
>> + * @tx_sem: Semaphore for available TX buffers for this controller
>> + * @last_tid: Last used entry for TID transactions
>>    * @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.
>> @@ -161,6 +278,15 @@ struct slim_controller {
>>   	struct slim_addrt	*addrt;
>>   	u8			num_dev;
>>   	struct workqueue_struct *wq;
>> +	struct slim_val_inf	*tid_tbl[SLIM_MAX_TIDS];
>> +	u8			last_tid;
> 
> I suggest that you replace these two with an idr, rather than having a
> fixed size array and then last_tid as an optimization to limit how far
> you linear search for an empty space.

Will try that and see how it looks!


> 
>> +	spinlock_t		txn_lock;
>> +	struct slim_ctrl_buf	tx;
>> +	struct slim_ctrl_buf	rx;
>> +	struct slim_pending	*pending_wr;
>> +	struct semaphore	tx_sem;
> 
> Please don't use semaphores. If you keep pending_wr as a list you can
> use list_empty() instead...

will give that a go.

> 
>> +	int			(*xfer_msg)(struct slim_controller *ctrl,
>> +					    struct slim_msg_txn *tx, void *buf);
> 
> I believe buf has fixed size, so please document this.
Yep. Will do that in next version.
> 
>>   	int			(*set_laddr)(struct slim_controller *ctrl,
>>   					     struct slim_eaddr *ea, u8 laddr);
>>   	int			(*get_laddr)(struct slim_controller *ctrl,
>> @@ -295,5 +421,40 @@ static inline void slim_set_devicedata(struct slim_device *dev, void *data)
>>   {
>>   	dev_set_drvdata(&dev->dev, data);
>>   }
> 
> Regards,
> Bjorn
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed 18 Oct 09:39 PDT 2017, Srinivas Kandagatla wrote:

> Thanks for Review Comments,
>
>
> On 18/10/17 07:15, Bjorn Andersson wrote:
> > On Fri 06 Oct 08:51 PDT 2017, srinivas.kandagatla@linaro.org wrote:
[..]
> >
> > > + if (!async) {
> > > + txn->msg->comp_cb = NULL;
> > > + txn->msg->ctx = NULL;
> >
> > I believe txn->msg is always required, so you don't need to do this
> > contidionally.
>
> I don't get this, why do you want to set comp_cb to NULL unconditionally?
>

I'm just not happy about the complexity of this function, but perhaps
it's confusing to always set them, regardless of them being used. Feel
free to keep it.

[..]
> > > +void slim_return_tx(struct slim_controller *ctrl, int err)
> > > +{
> > > + unsigned long flags;
> > > + int idx;
> > > + struct slim_pending cur;
> > > +
> > > + spin_lock_irqsave(&ctrl->tx.lock, flags);
> > > + idx = ctrl->tx.head;
> > > + ctrl->tx.head = (ctrl->tx.head + 1) % ctrl->tx.n;
> > > + cur = ctrl->pending_wr[idx];
> >
> > Why is this doing struct copy?
> >
> Not sure, do you see any issue with this?
>

It's a rarely used feature and I don't see a reason for using it here.

It's probably better to make a copy of cur.cb and cur.ctx to make their
use after the spin-unlock more obvious (but should be fine as the
spinlock is for the pending_wr array.

> > > + spin_unlock_irqrestore(&ctrl->tx.lock, flags);
> > > +
> > > + if (!cur.cb)
> > > + dev_err(&ctrl->dev, "NULL Transaction or completion");
> > > + else
> > > + cur.cb(cur.ctx, err);
> > > +
> > > + up(&ctrl->tx_sem);
> > > +}
> > > +EXPORT_SYMBOL_GPL(slim_return_tx);
[..]
> > > +/**
> > > + * struct slim_ctrl_buf: circular buffer used by contoller for TX, RX
> > > + * @base: virtual base address for this buffer
> > > + * @phy: physical address for this buffer (this is useful if controller can
> > > + *  DMA the buffers for TX and RX to/from controller hardware
> > > + * @lock: lock protecting head and tail
> > > + * @head: index where buffer is returned back
> > > + * @tail: index from where buffer is consumed
> > > + * @sl_sz: byte-size of each slot in this buffer
> > > + * @n:  number of elements in this circular ring, note that this needs to be
> > > + * 1 more than actual buffers to allow for one open slot
> > > + */
> >
> > Is this ringbuffer mechanism defined in the slimbus specification? Looks
> > like something specific to the Qualcomm controller, rather than
> > something that should be enforced in the framework.
> >
>
> Yes, this is not part of the slimbus specs, but Qcom SOCs have concept of
> Message Queues.
>
> Are you suggesting that this buffer handling has to be moved out of core
> into controller driver?
>

The fact that this seems to describe a physical ring buffer, with some
set of properties that are related to how a ring buffer works in the
Qualcomm hardware and it carries a notion of physical mapping, all
indicates to me that this describes some Qualcomm hardware interface.

I believe this is a hardware implementation detail that should reside in
the hardware part of the implementation (i.e. the Qualcomm driver).

>
> > > +struct slim_ctrl_buf {
> > > + void *base;
> > > + phys_addr_t phy;
> > > + spinlock_t lock;
> > > + int head;
> > > + int tail;
> > > + int sl_sz;
> > > + int n;
> > > +};

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandagatla@linaro.org wrote:

> diff --git a/drivers/base/regmap/regmap-slimbus.c b/drivers/base/regmap/regmap-slimbus.c
[..]
> +static int regmap_slimbus_byte_reg_read(void *context, unsigned int reg,
> + unsigned int *val)
> +{
> + struct slim_device *slim = context;
> + struct slim_val_inf msg = {0,};
> +
> + msg.start_offset = reg;
> + msg.num_bytes = 1;
> + msg.rbuf = (void *)val;

Turn rbuf into a void * and you don't need this cast (think I commented
on this on a previous patch as well).

> +
> + return slim_request_val_element(slim, &msg);
> +}
> +
> +static int regmap_slimbus_byte_reg_write(void *context, unsigned int reg,
> + unsigned int val)
> +{
> + struct slim_device *slim = context;
> + struct slim_val_inf msg = {0,};
> +
> + msg.start_offset = reg;
> + msg.num_bytes = 1;
> + msg.wbuf = (void *)&val;

Dito

> +
> + return slim_change_val_element(slim, &msg);
> +}

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandagatla@linaro.org wrote:

> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> Add myself as maintainer for slimbus.
>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  MAINTAINERS | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2281af4..014f74b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12320,6 +12320,14 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
>  F: include/linux/srcu.h
>  F: kernel/rcu/srcu.c
>
> +SERIAL LOW-POWER INTER-CHIP MEDIA BUS (SLIMbus)
> +M: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> +L: alsa-devel@alsa-project.org (moderated for non-subscribers)
> +S: Maintained
> +F: drivers/slimbus/
> +F: Documentation/devicetree/bindings/slimbus/
> +F: include/linux/slimbus.h
> +
>  SMACK SECURITY MODULE
>  M: Casey Schaufler <casey@schaufler-ca.com>
>  L: linux-security-module@vger.kernel.org
> --
> 2.9.3
>
> --
> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html