[1/4] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs

Add controller driver for QCOM SoCs that have hardware based shared
resource management. The hardware IP known as RSC (Resource State
Coordinator) houses multiple Direct Resource Voter (DRV) for different
execution levels. A DRV is a unique voter on the state of a shared
resource. A Trigger Control Set (TCS) is a bunch of slots that can house
multiple resource state requests, that when triggered will issue those
requests through an internal bus to the Resource Power Manager Hardened
(RPMH) blocks. These hardware blocks are capable of adjusting clocks,
voltages etc. The resource state request from a DRV are aggregated along
with state requests from other processors in the SoC and the aggregate
value is applied on the resource.

Some important aspects of the RPMH communication -
- Requests are <addr, value> with some header information
- Multiple requests (upto 16) may be sent through a TCS, at a time
- Requests in a TCS are sent in sequence
- Requests may be fire-n-forget or completion (response expected)
- Multiple TCS from the same DRV may be triggered simultaneously
- Cannot send a request if another reques for the same addr is in
  progress from the same DRV
- When all the requests from a TCS are complete, an IRQ is raised
- The IRQ handler needs to clear the TCS before it is available for
  reuse
- TCS configuration is specific to a DRV
- Platform drivers may use DRV from different RSCs to make requests

Resource state requests made when CPUs are active are called 'active'
state requests. Requests made when all the CPUs are powered down (idle
state) are called 'sleep' state requests. They are matched by a
corresponding 'wake' state requests which puts the resources back in to
previously requested active state before resuming any CPU. TCSes are
dedicated for each type of requests. Control TCS are used to provide
specific information to the controller.

Only active requests are currently supported by the driver.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/soc/qcom/Kconfig                |   7 +
 drivers/soc/qcom/Makefile               |   1 +
 drivers/soc/qcom/rpmh-internal.h        |  24 ++
 drivers/soc/qcom/rpmh-rsc.c             | 674 ++++++++++++++++++++++++++++++++
 include/dt-bindings/soc/qcom,rpmh-rsc.h |  16 +
 include/soc/qcom/tcs.h                  |  38 ++
 6 files changed, 760 insertions(+)
 create mode 100644 drivers/soc/qcom/rpmh-internal.h
 create mode 100644 drivers/soc/qcom/rpmh-rsc.c
 create mode 100644 include/dt-bindings/soc/qcom,rpmh-rsc.h
 create mode 100644 include/soc/qcom/tcs.h

On Thu 18 Jan 16:01 PST 2018, Lina Iyer wrote:
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> new file mode 100644
> index 000000000000..3e68cef5513e
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -0,0 +1,674 @@
> +/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */

Please use SPDX headers for new files.

// SPDX-License-Identifier: GPL-2.0
/*
 * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
 */

> +
> +#define pr_fmt(fmt) "%s " fmt, KBUILD_MODNAME
> +
> +#include <linux/atomic.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>

Unused?

> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include <asm-generic/io.h>
> +#include <soc/qcom/tcs.h>
> +#include <dt-bindings/soc/qcom,rpmh-rsc.h>
> +
> +#include "rpmh-internal.h"
> +
> +#define MAX_CMDS_PER_TCS		16
> +#define MAX_TCS_PER_TYPE		3
> +
> +#define RSC_DRV_TCS_OFFSET		672
> +#define RSC_DRV_CMD_OFFSET		20
> +
> +/* DRV Configuration Information Register */
> +#define DRV_PRNT_CHLD_CONFIG		0x0C
> +#define DRV_NUM_TCS_MASK		0x3F
> +#define DRV_NUM_TCS_SHIFT		6
> +#define DRV_NCPT_MASK			0x1F
> +#define DRV_NCPT_SHIFT			27
> +
> +/* Register offsets */
> +#define RSC_DRV_IRQ_ENABLE		0x00
> +#define RSC_DRV_IRQ_STATUS		0x04
> +#define RSC_DRV_IRQ_CLEAR		0x08
> +#define RSC_DRV_CMD_WAIT_FOR_CMPL	0x10
> +#define RSC_DRV_CONTROL			0x14
> +#define RSC_DRV_STATUS			0x18
> +#define RSC_DRV_CMD_ENABLE		0x1C
> +#define RSC_DRV_CMD_MSGID		0x30
> +#define RSC_DRV_CMD_ADDR		0x34
> +#define RSC_DRV_CMD_DATA		0x38
> +#define RSC_DRV_CMD_STATUS		0x3C
> +#define RSC_DRV_CMD_RESP_DATA		0x40
> +
> +#define TCS_AMC_MODE_ENABLE		BIT(16)
> +#define TCS_AMC_MODE_TRIGGER		BIT(24)
> +
> +/* TCS CMD register bit mask */
> +#define CMD_MSGID_LEN			8
> +#define CMD_MSGID_RESP_REQ		BIT(8)
> +#define CMD_MSGID_WRITE			BIT(16)
> +#define CMD_STATUS_ISSUED		BIT(8)
> +#define CMD_STATUS_COMPL		BIT(16)
> +
> +#define TCS_TYPE_NR			4
> +#define MAX_TCS_NR			(MAX_TCS_PER_TYPE * TCS_TYPE_NR)
> +
> +struct rsc_drv;
> +
> +/**
> + * tcs_response: Response object for a request
> + *
> + * @drv: the controller
> + * @msg: the request for this response
> + * @m: the tcs identifier
> + * @err: error reported in the response
> + * @list: link list object.
> + */
> +struct tcs_response {
> +	struct rsc_drv *drv;
> +	struct tcs_mbox_msg *msg;
> +	u32 m;
> +	int err;
> +	struct list_head list;
> +};
> +
> +/**
> + * tcs_mbox: group of TCSes for a request state
> + *
> + * @type: type of the TCS in this group - active, sleep, wake
> + * @tcs_mask: mask of the TCSes relative to all the TCSes in the RSC
> + * @tcs_offset: start of the TCS group relative to the TCSes in the RSC
> + * @num_tcs: number of TCSes in this type
> + * @ncpt: number of commands in each TCS
> + * @tcs_lock: lock for synchronizing this TCS writes
> + * @responses: response objects for requests sent from each TCS
> + */
> +struct tcs_mbox {

struct tcs_group ?

> +	struct rsc_drv *drv;
> +	int type;
> +	u32 tcs_mask;
> +	u32 tcs_offset;
> +	int num_tcs;
> +	int ncpt;
> +	spinlock_t tcs_lock;
> +	struct tcs_response *responses[MAX_TCS_PER_TYPE];
> +};
> +
> +/**
> + * rsc_drv: the RSC controller
> + *
> + * @pdev: platform device of this controller
> + * @name: controller identifier
> + * @base: start address of the controller
> + * @reg_base: start address of the registers in this controller
> + * @drv_id: instance id in the controller (DRV)
> + * @num_tcs: number of TCSes in this DRV
> + * @tasklet: handle responses, off-load work from IRQ handler
> + * @response_pending: list of responses that needs to be sent to caller
> + * @tcs: TCS groups
> + * @tcs_in_use: s/w state of the TCS
> + * @drv_lock: synchronize state of the controller
> + */
> +struct rsc_drv {
> +	struct platform_device *pdev;

This is not used outside rpmh_rsc_probe() and I presume you would like
to be able to use it for dev_prints(), if so store the device * instead.

> +	const char *name;
> +	void __iomem *base;
> +	void __iomem *reg_base;
> +	int drv_id;

@base and @drv_id are local to rpmh_rsc_probe(), so you can make it them
local variables.

> +	int num_tcs;
> +	struct tasklet_struct tasklet;
> +	struct list_head response_pending;
> +	struct tcs_mbox tcs[TCS_TYPE_NR];
> +	atomic_t tcs_in_use[MAX_TCS_NR];

Does this really need to be an atomic_t? Seems like it's protected by
the drv_lock.

> +	spinlock_t drv_lock;
> +};
> +
> +static inline struct tcs_mbox *get_tcs_from_index(struct rsc_drv *drv, int m)

get_tcs_group_by_index(), or maybe even get_tcs_group_by_tcs()? To
clarify that this resolves a group and not a single tcs.

Also, please drop the "inline".

> +{
> +	struct tcs_mbox *tcs = NULL;
> +	int i;
> +
> +	for (i = 0; i < drv->num_tcs; i++) {
> +		tcs = &drv->tcs[i];
> +		if (tcs->tcs_mask & (u32)BIT(m))

I don't think you need this cast.

> +			break;
> +	}
> +
> +	if (i == drv->num_tcs) {
> +		WARN(1, "Incorrect TCS index %d", m);
> +		tcs = NULL;
> +	}
> +
> +	return tcs;
> +}
> +
> +static struct tcs_response *setup_response(struct rsc_drv *drv,
> +			struct tcs_mbox_msg *msg, int m)
> +{
> +	struct tcs_response *resp;
> +	struct tcs_mbox *tcs;
> +
> +	resp = kcalloc(1, sizeof(*resp), GFP_ATOMIC);
> +	if (!resp)
> +		return ERR_PTR(-ENOMEM);

Rather than allocating a response for each write request and freeing it
in the tasklet I think you should just allocate MAX_TCS_PER_TYPE
responses when you're probing the driver.

> +
> +	resp->drv = drv;
> +	resp->msg = msg;
> +	resp->err = 0;
> +
> +	tcs = get_tcs_from_index(drv, m);
> +	if (!tcs)
> +		return ERR_PTR(-EINVAL);
> +
> +	/*
> +	 * We should have been called from a TCS-type locked context, and
> +	 * we overwrite the previously saved response.
> +	 */

I assume that this overwrite wouldn't happen because we're only writing
to the tcs if it's not in use? Wouldn't overwriting a valid response
lead to us leaking the previously allocated resp?

> +	tcs->responses[m - tcs->tcs_offset] = resp;
> +
> +	return resp;
> +}
> +
> +static void free_response(struct tcs_response *resp)
> +{
> +	kfree(resp);
> +}
> +
> +static inline struct tcs_response *get_response(struct rsc_drv *drv, u32 m)
> +{
> +	struct tcs_mbox *tcs = get_tcs_from_index(drv, m);
> +
> +	return tcs->responses[m - tcs->tcs_offset];
> +}
> +
> +static inline u32 read_drv_config(void __iomem *base)
> +{
> +	return le32_to_cpu(readl_relaxed(base + DRV_PRNT_CHLD_CONFIG));

readl_relaxed() on arm64 is defined as le32_to_cpu(__raw_readl(c)), so I
would suggest that you drop the le32_to_cpu() and just inline your
readl() directly in rpmh_rsc_probe().

> +}
> +
> +static inline u32 read_tcs_reg(void __iomem *base, int reg, int m, int n)
> +{

I would suggest that you pass the rsc_drv to these functions, so that
it's obvious which "base" we're operating on here.

> +	return le32_to_cpu(readl_relaxed(base + reg +
> +			RSC_DRV_TCS_OFFSET * m + RSC_DRV_CMD_OFFSET * n));

No le32_to_cpu()

> +}
> +
> +static inline void write_tcs_reg(void __iomem *base, int reg, int m, int n,
> +				u32 data)
> +{
> +	writel_relaxed(cpu_to_le32(data), base + reg +
> +			RSC_DRV_TCS_OFFSET * m + RSC_DRV_CMD_OFFSET * n);

As with readl, writel does cpu_to_le32()

> +}
> +
> +static inline void write_tcs_reg_sync(void __iomem *base, int reg, int m, int n,
> +				u32 data)
> +{
> +	do {
> +		write_tcs_reg(base, reg, m, n, data);
> +		if (data == read_tcs_reg(base, reg, m, n))
> +			break;
> +		udelay(1);
> +	} while (1);
> +}
> +
> +static inline bool tcs_is_free(struct rsc_drv *drv, int m)
> +{
> +	void __iomem *base = drv->reg_base;
> +
> +	return read_tcs_reg(base, RSC_DRV_STATUS, m, 0) &&
> +			!atomic_read(&drv->tcs_in_use[m]);

If you flip these conditions around you won't do the io access when
tcs_in_use[m].


What are the cases when tcs_in_use will tell you that the tcs is free,
but the hardware will say it's not?

> +}
> +
> +static inline struct tcs_mbox *get_tcs_of_type(struct rsc_drv *drv, int type)
> +{
> +	int i;
> +	struct tcs_mbox *tcs;
> +
> +	for (i = 0; i < TCS_TYPE_NR; i++)
> +		if (type == drv->tcs[i].type)
> +			break;
> +
> +	if (i == TCS_TYPE_NR)
> +		return ERR_PTR(-EINVAL);
> +
> +	tcs = &drv->tcs[i];
> +	if (!tcs->num_tcs)
> +		return ERR_PTR(-EINVAL);
> +
> +	return tcs;
> +}
> +
> +static inline struct tcs_mbox *get_tcs_for_msg(struct rsc_drv *drv,
> +						struct tcs_mbox_msg *msg)
> +{
> +	int type = -1;
> +
> +	switch (msg->state) {
> +	case RPMH_ACTIVE_ONLY_STATE:
> +		type = ACTIVE_TCS;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (type < 0)
> +		return ERR_PTR(-EINVAL);

afaict you can put this return in the default case.

> +
> +	return get_tcs_of_type(drv, type);
> +}
> +
> +static inline void send_tcs_response(struct tcs_response *resp)
> +{
> +	struct rsc_drv *drv = resp->drv;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&drv->drv_lock, flags);
> +	INIT_LIST_HEAD(&resp->list);
> +	list_add_tail(&resp->list, &drv->response_pending);
> +	spin_unlock_irqrestore(&drv->drv_lock, flags);
> +
> +	tasklet_schedule(&drv->tasklet);
> +}

Is there any particular reason why this uses a tasklet, instead of just
handling the tx_done event from the interrupt handler directly?

> +
> +/**
> + * tcs_irq_handler: TX Done interrupt handler
> + */
> +static irqreturn_t tcs_irq_handler(int irq, void *p)
> +{
> +	struct rsc_drv *drv = p;
> +	void __iomem *base = drv->reg_base;
> +	int m, i;
> +	u32 irq_status, sts;
> +	struct tcs_response *resp;
> +	struct tcs_cmd *cmd;
> +	int err;
> +
> +	irq_status = read_tcs_reg(base, RSC_DRV_IRQ_STATUS, 0, 0);
> +
> +	for (m = 0; m < drv->num_tcs; m++) {
> +		if (!(irq_status & (u32)BIT(m)))
> +			continue;
> +
> +		err = 0;
> +		resp = get_response(drv, m);
> +		if (!resp) {

This conditional section dereferences resp, so I suspect that the
conditional is backwards.

> +			for (i = 0; i < resp->msg->num_payload; i++) {
> +				cmd = &resp->msg->payload[i];
> +				sts = read_tcs_reg(base, RSC_DRV_CMD_STATUS,
> +							m, i);
> +				if ((!(sts & CMD_STATUS_ISSUED)) ||
> +					((resp->msg->is_complete ||
> +					  cmd->complete) &&
> +					(!(sts & CMD_STATUS_COMPL)))) {
> +					resp->err = -EIO;
> +					break;

As this completes the response, aren't we leaking the tcs_in_use?

> +				}
> +			}
> +		}
> +
> +		write_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0, 0);
> +		write_tcs_reg(base, RSC_DRV_IRQ_CLEAR, 0, 0, BIT(m));
> +		atomic_set(&drv->tcs_in_use[m], 0);
> +
> +		if (resp) {
> +			resp->err = err;
> +			send_tcs_response(resp);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * tcs_notify_tx_done: TX Done for requests that got a response
> + *
> + * @data: the tasklet argument
> + *
> + * Tasklet function to notify MBOX that we are done with the request.
> + * Handles all pending reponses whenever run.
> + */
> +static void tcs_notify_tx_done(unsigned long data)

I think you should drop the tasklet and just invoke the handling of
responses directly from the irq context.

> +{
> +	struct rsc_drv *drv = (struct rsc_drv *)data;
> +	struct tcs_response *resp;
> +	unsigned long flags;
> +	int err;
> +
> +	do {
> +		spin_lock_irqsave(&drv->drv_lock, flags);
> +		if (list_empty(&drv->response_pending)) {
> +			spin_unlock_irqrestore(&drv->drv_lock, flags);
> +			break;
> +		}
> +		resp = list_first_entry(&drv->response_pending,
> +					struct tcs_response, list);
> +		list_del(&resp->list);
> +		spin_unlock_irqrestore(&drv->drv_lock, flags);
> +		err = resp->err;
> +		free_response(resp);
> +	} while (1);

Use for (;;) {} or while (1) {}, rather than do {} while(1)

> +}
> +
> +static void __tcs_buffer_write(struct rsc_drv *drv, int m, int n,
> +				struct tcs_mbox_msg *msg)
> +{
> +	void __iomem *base = drv->reg_base;
> +	u32 msgid, cmd_msgid = 0;
> +	u32 cmd_enable = 0;
> +	u32 cmd_complete;
> +	struct tcs_cmd *cmd;
> +	int i;
> +
> +	cmd_msgid = CMD_MSGID_LEN;
> +	cmd_msgid |= (msg->is_complete) ? CMD_MSGID_RESP_REQ : 0;
> +	cmd_msgid |= CMD_MSGID_WRITE;
> +
> +	cmd_complete = read_tcs_reg(base, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
> +
> +	for (i = 0; i < msg->num_payload; i++) {
> +		cmd = &msg->payload[i];
> +		cmd_enable |= BIT(n + i);
> +		cmd_complete |= cmd->complete << (n + i);
> +		msgid = cmd_msgid;
> +		msgid |= (cmd->complete) ? CMD_MSGID_RESP_REQ : 0;
> +		write_tcs_reg(base, RSC_DRV_CMD_MSGID, m, n + i, msgid);
> +		write_tcs_reg(base, RSC_DRV_CMD_ADDR, m, n + i, cmd->addr);
> +		write_tcs_reg(base, RSC_DRV_CMD_DATA, m, n + i, cmd->data);
> +	}
> +
> +	write_tcs_reg(base, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete);
> +	cmd_enable |= read_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0);
> +	write_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0, cmd_enable);
> +}
> +
> +static void __tcs_trigger(struct rsc_drv *drv, int m)
> +{
> +	void __iomem *base = drv->reg_base;
> +	u32 enable;
> +
> +	/*
> +	 * HW req: Clear the DRV_CONTROL and enable TCS again
> +	 * While clearing ensure that the AMC mode trigger is cleared
> +	 * and then the mode enable is cleared.
> +	 */
> +	enable = read_tcs_reg(base, RSC_DRV_CONTROL, m, 0);
> +	enable &= ~TCS_AMC_MODE_TRIGGER;
> +	write_tcs_reg_sync(base, RSC_DRV_CONTROL, m, 0, enable);
> +	enable &= ~TCS_AMC_MODE_ENABLE;
> +	write_tcs_reg_sync(base, RSC_DRV_CONTROL, m, 0, enable);
> +
> +	/* Enable the AMC mode on the TCS and then trigger the TCS */
> +	enable = TCS_AMC_MODE_ENABLE;
> +	write_tcs_reg_sync(base, RSC_DRV_CONTROL, m, 0, enable);
> +	enable |= TCS_AMC_MODE_TRIGGER;
> +	write_tcs_reg_sync(base, RSC_DRV_CONTROL, m, 0, enable);
> +}
> +
> +static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_mbox *tcs,
> +						struct tcs_mbox_msg *msg)
> +{
> +	u32 curr_enabled, addr;
> +	int i, j, k;
> +	void __iomem *base = drv->reg_base;
> +	int m = tcs->tcs_offset;
> +
> +	for (i = 0; i < tcs->num_tcs; i++, m++) {
> +		if (tcs_is_free(drv, m))
> +			continue;
> +
> +		curr_enabled = read_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0);
> +
> +		for (j = 0; j < MAX_CMDS_PER_TCS; j++) {
> +			if (!(curr_enabled & (u32)BIT(j)))
> +				continue;

Consider using for_each_set_bit() here.

> +
> +			addr = read_tcs_reg(base, RSC_DRV_CMD_ADDR, m, j);
> +			for (k = 0; k < msg->num_payload; k++) {
> +				if (addr == msg->payload[k].addr)
> +					return -EBUSY;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int find_free_tcs(struct tcs_mbox *tcs)
> +{
> +	int m;
> +
> +	for (m = 0; m < tcs->num_tcs; m++)
> +		if (tcs_is_free(tcs->drv, tcs->tcs_offset + m))
> +			break;
> +
> +	return (m != tcs->num_tcs) ? m : -EBUSY;
> +}
> +
> +static int tcs_mbox_write(struct rsc_drv *drv, struct tcs_mbox_msg *msg)
> +{
> +	struct tcs_mbox *tcs;
> +	int m;
> +	struct tcs_response *resp = NULL;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	tcs = get_tcs_for_msg(drv, msg);
> +	if (IS_ERR(tcs))
> +		return PTR_ERR(tcs);
> +
> +	spin_lock_irqsave(&tcs->tcs_lock, flags);
> +	m = find_free_tcs(tcs);
> +	if (m < 0) {
> +		ret = m;
> +		goto done_write;
> +	}
> +
> +	/*
> +	 * The h/w does not like if we send a request to the same address,
> +	 * when one is already in-flight or bring processed.
> +	 */
> +	ret = check_for_req_inflight(drv, tcs, msg);
> +	if (ret)
> +		goto done_write;
> +
> +	resp = setup_response(drv, msg, m);
> +	if (IS_ERR_OR_NULL(resp)) {
> +		ret = PTR_ERR(resp);
> +		goto done_write;
> +	}
> +	resp->m = m;
> +
> +	atomic_set(&drv->tcs_in_use[m], 1);
> +	__tcs_buffer_write(drv, m, 0, msg);
> +	__tcs_trigger(drv, m);
> +
> +done_write:
> +	spin_unlock_irqrestore(&tcs->tcs_lock, flags);
> +	return ret;
> +}
> +
> +/**
> + * rpmh_rsc_send_data: Validate the incoming message and write to the
> + * appropriate TCS block.
> + *
> + * @drv: the controller
> + * @msg: the data to be sent
> + *
> + * Returns a negative error for invalid message structure and invalid
> + * message combination, -EBUSY if there is an other active request for
> + * the channel in process, otherwise bubbles up internal error.

Kernel-doc style is "Return: a negative..."

The function never returns -EBUSY, as you're looping while this is being
returned.

Also, it would be nice if the doc mentions that 0 is returned on
success, so perhaps just "Return: 0 on success, negative errno on
error"?

> + */
> +int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_mbox_msg *msg)
> +{
> +	int ret = 0;
> +
> +	if (!msg || !msg->payload || !msg->num_payload ||
> +			msg->num_payload > MAX_RPMH_PAYLOAD)
> +		return -EINVAL;
> +
> +	do {
> +		ret = tcs_mbox_write(drv, msg);
> +		if (ret == -EBUSY) {
> +			pr_info_ratelimited(
> +			"TCS Busy, retrying RPMH message send: addr=0x%x\n",
> +			msg->payload[0].addr);

Indent wrapped lines to the start parenthesis.

> +			udelay(10);
> +		}
> +	} while (ret == -EBUSY);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(rpmh_rsc_send_data);
> +
> +static int rpmh_rsc_probe(struct platform_device *pdev)
> +{
> +	struct device_node *dn = pdev->dev.of_node;
> +	struct rsc_drv *drv;
> +	struct tcs_mbox *tcs;
> +	int irq;
> +	u32 val[8] = { 0 };

8 here should be TCS_TYPE_NR * 2, but I like Tomas(?) suggestion in the
PDC driver of representing arrays of tuples like this as a structured
type.

> +	int st = 0;
> +	int i, ret, nelem;
> +	u32 config, max_tcs, ncpt;
> +	int tcs_type_count[TCS_TYPE_NR] = { 0 };
> +	struct resource *res;
> +
> +	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32(dn, "qcom,drv-id", &drv->drv_id);
> +	if (ret)
> +		return ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EINVAL;
> +	drv->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(drv->base))
> +		return PTR_ERR(drv->base);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res)
> +		return -EINVAL;
> +	drv->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(drv->reg_base))
> +		return PTR_ERR(drv->reg_base);
> +
> +	config = read_drv_config(drv->base);
> +	max_tcs = config & (DRV_NUM_TCS_MASK <<
> +				(DRV_NUM_TCS_SHIFT * drv->drv_id));
> +	max_tcs = max_tcs >> (DRV_NUM_TCS_SHIFT * drv->drv_id);
> +	ncpt = config & (DRV_NCPT_MASK << DRV_NCPT_SHIFT);
> +	ncpt = ncpt >> DRV_NCPT_SHIFT;
> +
> +	nelem = of_property_count_elems_of_size(dn, "qcom,tcs-config",
> +						sizeof(u32));
> +	if (!nelem || (nelem % 2) || (nelem > 2 * TCS_TYPE_NR))
> +		return -EINVAL;
> +
> +	ret = of_property_read_u32_array(dn, "qcom,tcs-config", val, nelem);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < (nelem / 2); i++) {
> +		if (val[2 * i] >= TCS_TYPE_NR)
> +			return -EINVAL;
> +		tcs_type_count[val[2 * i]]++;
> +		if (tcs_type_count[val[2 * i]] > 1)
> +			return -EINVAL;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(tcs_type_count); i++)
> +		if (!tcs_type_count[i])
> +			return -EINVAL;

I think it's better if replace these two loops by adding a check in
below loop to ensure that tcs->drv is NULL and that tcs->type is valid
and then follow up with a loop checking that each drv->tcs was
initialized.

That way you don't need to do the counting of the various types.

> +
> +	for (i = 0; i < (nelem / 2); i++) {
> +		tcs = &drv->tcs[val[2 * i]];
> +		tcs->drv = drv;
> +		tcs->type = val[2 * i];
> +		tcs->num_tcs = val[2 * i + 1];
> +		tcs->ncpt = ncpt;
> +		spin_lock_init(&tcs->tcs_lock);
> +
> +		if (tcs->num_tcs <= 0 || tcs->type == CONTROL_TCS)
> +			continue;
> +
> +		if (tcs->num_tcs > MAX_TCS_PER_TYPE ||
> +			st + tcs->num_tcs > max_tcs ||
> +			st + tcs->num_tcs >= 8 * sizeof(tcs->tcs_mask))
> +			return -EINVAL;
> +
> +		tcs->tcs_mask = ((1 << tcs->num_tcs) - 1) << st;
> +		tcs->tcs_offset = st;
> +		st += tcs->num_tcs;
> +	}
> +
> +	drv->num_tcs = st;
> +	drv->pdev = pdev;
> +	INIT_LIST_HEAD(&drv->response_pending);
> +	spin_lock_init(&drv->drv_lock);
> +	tasklet_init(&drv->tasklet, tcs_notify_tx_done, (unsigned long)drv);
> +
> +	drv->name = of_get_property(pdev->dev.of_node, "label", NULL);
> +	if (!drv->name)
> +		drv->name = dev_name(&pdev->dev);
> +
> +	irq = of_irq_get(dn, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_irq(&pdev->dev, irq, tcs_irq_handler,
> +			IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND, drv->name, drv);
> +	if (ret)
> +		return ret;
> +
> +	write_tcs_reg(drv->reg_base, RSC_DRV_IRQ_ENABLE, 0, 0,
> +					drv->tcs[ACTIVE_TCS].tcs_mask);
> +
> +	for (i = 0; i < ARRAY_SIZE(drv->tcs_in_use); i++)
> +		atomic_set(&drv->tcs_in_use[i], 0);

Perhaps do this before requesting the irq? If nothing else that saves
future readers from wondering if it makes any difference...

> +
> +	dev_set_drvdata(&pdev->dev, drv);

This isn't used until the next patch, so move it there to clarify its
purpose.

> +
> +	return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> +}
> +
> +static const struct of_device_id rpmh_drv_match[] = {
> +	{ .compatible = "qcom,rpmh-rsc", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rpm_drv_match);
> +
> +static struct platform_driver rpmh_driver = {
> +	.probe = rpmh_rsc_probe,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = rpmh_drv_match,

As we don't want to handle unbind, add:

                .suppress_bind_attrs = true,

> +	},
> +};
> +
> +static int __init rpmh_driver_init(void)
> +{
> +	return platform_driver_register(&rpmh_driver);
> +}
> +arch_initcall(rpmh_driver_init);
> diff --git a/include/dt-bindings/soc/qcom,rpmh-rsc.h b/include/dt-bindings/soc/qcom,rpmh-rsc.h
> new file mode 100644
> index 000000000000..24a56a5cf096
> --- /dev/null
> +++ b/include/dt-bindings/soc/qcom,rpmh-rsc.h
> @@ -0,0 +1,16 @@
> +/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#define SLEEP_TCS	0
> +#define WAKE_TCS	1
> +#define ACTIVE_TCS	2
> +#define CONTROL_TCS	3
> diff --git a/include/soc/qcom/tcs.h b/include/soc/qcom/tcs.h
> new file mode 100644
> index 000000000000..07cf6d8d43e3
> --- /dev/null
> +++ b/include/soc/qcom/tcs.h
> @@ -0,0 +1,38 @@
> +/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __SOC_QCOM_TCS_H__
> +#define __SOC_QCOM_TCS_H__
> +
> +#define MAX_RPMH_PAYLOAD	16
> +
> +struct tcs_cmd {
> +	u32 addr;		/* slv_id:18:16 | offset:0:15 */
> +	u32 data;		/* data for resource (or read response) */
> +	bool complete;		/* wait for completion before sending next */
> +};

I think you should use kernel-doc to describe the structs.

> +
> +enum rpmh_state {
> +	RPMH_ACTIVE_ONLY_STATE,	/* Active only (= AMC) */
> +	RPMH_WAKE_ONLY_STATE,	/* Wake only */
> +	RPMH_SLEEP_STATE,	/* Sleep */
> +};
> +
> +struct tcs_mbox_msg {

struct tcs_request ?

> +	enum rpmh_state state;	/* request state */
> +	bool is_complete;	/* wait for resp from accelerator */
> +	u32 num_payload;	/* Limited to MAX_RPMH_PAYLOAD in one msg */
> +	struct tcs_cmd *payload;/* array of tcs_cmds */
> +};
> +
> +#endif /* __SOC_QCOM_TCS_H__ */

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 Mon, Feb 05 2018 at 19:19 +0000, Bjorn Andersson wrote:
>On Thu 18 Jan 16:01 PST 2018, Lina Iyer wrote:
>> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
>> new file mode 100644
>> index 000000000000..3e68cef5513e
>> --- /dev/null
>> +++ b/drivers/soc/qcom/rpmh-rsc.c
>> @@ -0,0 +1,674 @@
>> +/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>
>Please use SPDX headers for new files.
>
>// SPDX-License-Identifier: GPL-2.0
>/*
> * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> */
>
OK.

>> +
>> +#define pr_fmt(fmt) "%s " fmt, KBUILD_MODNAME
>> +
>> +#include <linux/atomic.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>
>Unused?
>
Will remove.

>> +/**
>> + * tcs_mbox: group of TCSes for a request state
>> + *
>> + * @type: type of the TCS in this group - active, sleep, wake
>> + * @tcs_mask: mask of the TCSes relative to all the TCSes in the RSC
>> + * @tcs_offset: start of the TCS group relative to the TCSes in the RSC
>> + * @num_tcs: number of TCSes in this type
>> + * @ncpt: number of commands in each TCS
>> + * @tcs_lock: lock for synchronizing this TCS writes
>> + * @responses: response objects for requests sent from each TCS
>> + */
>> +struct tcs_mbox {
>
>struct tcs_group ?
>
Hmm.. I am okay with that.

>> +	struct rsc_drv *drv;
>> +	int type;
>> +	u32 tcs_mask;
>> +	u32 tcs_offset;
>> +	int num_tcs;
>> +	int ncpt;
>> +	spinlock_t tcs_lock;
>> +	struct tcs_response *responses[MAX_TCS_PER_TYPE];
>> +};
>> +
>> +/**
>> + * rsc_drv: the RSC controller
>> + *
>> + * @pdev: platform device of this controller
>> + * @name: controller identifier
>> + * @base: start address of the controller
>> + * @reg_base: start address of the registers in this controller
>> + * @drv_id: instance id in the controller (DRV)
>> + * @num_tcs: number of TCSes in this DRV
>> + * @tasklet: handle responses, off-load work from IRQ handler
>> + * @response_pending: list of responses that needs to be sent to caller
>> + * @tcs: TCS groups
>> + * @tcs_in_use: s/w state of the TCS
>> + * @drv_lock: synchronize state of the controller
>> + */
>> +struct rsc_drv {
>> +	struct platform_device *pdev;
>
>This is not used outside rpmh_rsc_probe() and I presume you would like
>to be able to use it for dev_prints(), if so store the device * instead.
>
I don't need it. Will remove.

>> +	const char *name;
>> +	void __iomem *base;
>> +	void __iomem *reg_base;
>> +	int drv_id;
>
>@base and @drv_id are local to rpmh_rsc_probe(), so you can make it them
>local variables.
>
They are used in the other functions and are needed in this structure.
See IRQ handler and tcs_is_free().


>> +	int num_tcs;
>> +	struct tasklet_struct tasklet;
>> +	struct list_head response_pending;
>> +	struct tcs_mbox tcs[TCS_TYPE_NR];
>> +	atomic_t tcs_in_use[MAX_TCS_NR];
>
>Does this really need to be an atomic_t? Seems like it's protected by
>the drv_lock.
>
Not in the irq handler.

>> +	spinlock_t drv_lock;
>> +};
>> +
>> +static inline struct tcs_mbox *get_tcs_from_index(struct rsc_drv *drv, int m)
>
>get_tcs_group_by_index(), or maybe even get_tcs_group_by_tcs()? To
>clarify that this resolves a group and not a single tcs.
>
>Also, please drop the "inline".
>
Ok.

>> +{
>> +	struct tcs_mbox *tcs = NULL;
>> +	int i;
>> +
>> +	for (i = 0; i < drv->num_tcs; i++) {
>> +		tcs = &drv->tcs[i];
>> +		if (tcs->tcs_mask & (u32)BIT(m))
>
>I don't think you need this cast.
>
OK.

>> +			break;
>> +	}
>> +
>> +	if (i == drv->num_tcs) {
>> +		WARN(1, "Incorrect TCS index %d", m);
>> +		tcs = NULL;
>> +	}
>> +
>> +	return tcs;
>> +}
>> +
>> +static struct tcs_response *setup_response(struct rsc_drv *drv,
>> +			struct tcs_mbox_msg *msg, int m)
>> +{
>> +	struct tcs_response *resp;
>> +	struct tcs_mbox *tcs;
>> +
>> +	resp = kcalloc(1, sizeof(*resp), GFP_ATOMIC);
>> +	if (!resp)
>> +		return ERR_PTR(-ENOMEM);
>
>Rather than allocating a response for each write request and freeing it
>in the tasklet I think you should just allocate MAX_TCS_PER_TYPE
>responses when you're probing the driver.
>
I am keeping this simple for now. Ideally, a pool of responses would be
more helpful and instead of this. I will probably bring that in later.

>> +
>> +	resp->drv = drv;
>> +	resp->msg = msg;
>> +	resp->err = 0;
>> +
>> +	tcs = get_tcs_from_index(drv, m);
>> +	if (!tcs)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	/*
>> +	 * We should have been called from a TCS-type locked context, and
>> +	 * we overwrite the previously saved response.
>> +	 */
>
>I assume that this overwrite wouldn't happen because we're only writing
>to the tcs if it's not in use? Wouldn't overwriting a valid response
>lead to us leaking the previously allocated resp?
>
No. The freeing of the response is handled in the tasklet. Here we just
start using a new response object while the old one is still being
processed.

>> +	tcs->responses[m - tcs->tcs_offset] = resp;
>> +
>> +	return resp;
>> +}
>> +
>> +static void free_response(struct tcs_response *resp)
>> +{
>> +	kfree(resp);
>> +}
>> +
>> +static inline struct tcs_response *get_response(struct rsc_drv *drv, u32 m)
>> +{
>> +	struct tcs_mbox *tcs = get_tcs_from_index(drv, m);
>> +
>> +	return tcs->responses[m - tcs->tcs_offset];
>> +}
>> +
>> +static inline u32 read_drv_config(void __iomem *base)
>> +{
>> +	return le32_to_cpu(readl_relaxed(base + DRV_PRNT_CHLD_CONFIG));
>
>readl_relaxed() on arm64 is defined as le32_to_cpu(__raw_readl(c)), so I
>would suggest that you drop the le32_to_cpu() and just inline your
>readl() directly in rpmh_rsc_probe().
>
Ok.

>> +}
>> +
>> +static inline u32 read_tcs_reg(void __iomem *base, int reg, int m, int n)
>> +{
>
>I would suggest that you pass the rsc_drv to these functions, so that
>it's obvious which "base" we're operating on here.
>
Hmm. Ok. I used the void * because they are simple register functions
and have no bearing on the DRV they operate on.
But sure.

>> +	return le32_to_cpu(readl_relaxed(base + reg +
>> +			RSC_DRV_TCS_OFFSET * m + RSC_DRV_CMD_OFFSET * n));
>
>No le32_to_cpu()
>
Ok.

>> +}
>> +
>> +static inline void write_tcs_reg(void __iomem *base, int reg, int m, int n,
>> +				u32 data)
>> +{
>> +	writel_relaxed(cpu_to_le32(data), base + reg +
>> +			RSC_DRV_TCS_OFFSET * m + RSC_DRV_CMD_OFFSET * n);
>
>As with readl, writel does cpu_to_le32()
>
Ok

>> +}
>> +
>> +static inline void write_tcs_reg_sync(void __iomem *base, int reg, int m, int n,
>> +				u32 data)
>> +{
>> +	do {
>> +		write_tcs_reg(base, reg, m, n, data);
>> +		if (data == read_tcs_reg(base, reg, m, n))
>> +			break;
>> +		udelay(1);
>> +	} while (1);
>> +}
>> +
>> +static inline bool tcs_is_free(struct rsc_drv *drv, int m)
>> +{
>> +	void __iomem *base = drv->reg_base;
>> +
>> +	return read_tcs_reg(base, RSC_DRV_STATUS, m, 0) &&
>> +			!atomic_read(&drv->tcs_in_use[m]);
>
>If you flip these conditions around you won't do the io access when
>tcs_in_use[m].
>
Sure.

>What are the cases when tcs_in_use will tell you that the tcs is free,
>but the hardware will say it's not?
>
In the case of display RSC, which could be directly used by the display
driver, my state in tcs_in_use() would not reflect the state of the
hardware. This should not happen, but I would like to ensure that it
doesn't.

>> +}
>> +
>> +static inline struct tcs_mbox *get_tcs_of_type(struct rsc_drv *drv, int type)
>> +{
>> +	int i;
>> +	struct tcs_mbox *tcs;
>> +
>> +	for (i = 0; i < TCS_TYPE_NR; i++)
>> +		if (type == drv->tcs[i].type)
>> +			break;
>> +
>> +	if (i == TCS_TYPE_NR)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	tcs = &drv->tcs[i];
>> +	if (!tcs->num_tcs)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	return tcs;
>> +}
>> +
>> +static inline struct tcs_mbox *get_tcs_for_msg(struct rsc_drv *drv,
>> +						struct tcs_mbox_msg *msg)
>> +{
>> +	int type = -1;
>> +
>> +	switch (msg->state) {
>> +	case RPMH_ACTIVE_ONLY_STATE:
>> +		type = ACTIVE_TCS;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	if (type < 0)
>> +		return ERR_PTR(-EINVAL);
>
>afaict you can put this return in the default case.
>
Ok.

>> +
>> +	return get_tcs_of_type(drv, type);
>> +}
>> +
>> +static inline void send_tcs_response(struct tcs_response *resp)
>> +{
>> +	struct rsc_drv *drv = resp->drv;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&drv->drv_lock, flags);
>> +	INIT_LIST_HEAD(&resp->list);
>> +	list_add_tail(&resp->list, &drv->response_pending);
>> +	spin_unlock_irqrestore(&drv->drv_lock, flags);
>> +
>> +	tasklet_schedule(&drv->tasklet);
>> +}
>
>Is there any particular reason why this uses a tasklet, instead of just
>handling the tx_done event from the interrupt handler directly?
>
Its a bit too heavy for the interrupt handler to do all this locking and
respond back and signal completion (done in the next patches). Hence the
work is offloaded to the tasklet, we can be faster in responding to
other requests that have received the responses.

>> +
>> +/**
>> + * tcs_irq_handler: TX Done interrupt handler
>> + */
>> +static irqreturn_t tcs_irq_handler(int irq, void *p)
>> +{
>> +	struct rsc_drv *drv = p;
>> +	void __iomem *base = drv->reg_base;
>> +	int m, i;
>> +	u32 irq_status, sts;
>> +	struct tcs_response *resp;
>> +	struct tcs_cmd *cmd;
>> +	int err;
>> +
>> +	irq_status = read_tcs_reg(base, RSC_DRV_IRQ_STATUS, 0, 0);
>> +
>> +	for (m = 0; m < drv->num_tcs; m++) {
>> +		if (!(irq_status & (u32)BIT(m)))
>> +			continue;
>> +
>> +		err = 0;
>> +		resp = get_response(drv, m);
>> +		if (!resp) {
>
>This conditional section dereferences resp, so I suspect that the
>conditional is backwards.
>
Jeez, how did it not fail. Thanks for catching this..

>> +			for (i = 0; i < resp->msg->num_payload; i++) {
>> +				cmd = &resp->msg->payload[i];
>> +				sts = read_tcs_reg(base, RSC_DRV_CMD_STATUS,
>> +							m, i);
>> +				if ((!(sts & CMD_STATUS_ISSUED)) ||
>> +					((resp->msg->is_complete ||
>> +					  cmd->complete) &&
>> +					(!(sts & CMD_STATUS_COMPL)))) {
>> +					resp->err = -EIO;
>> +					break;
>
>As this completes the response, aren't we leaking the tcs_in_use?
>

No, we would break out of this loop and set the tcs_in_use. We have no
use of this TCS anymore. The h/w status is also reset.

>> +				}
>> +			}
>> +		}
>> +
>> +		write_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0, 0);
>> +		write_tcs_reg(base, RSC_DRV_IRQ_CLEAR, 0, 0, BIT(m));
>> +		atomic_set(&drv->tcs_in_use[m], 0);
>> +
>> +		if (resp) {
>> +			resp->err = err;
>> +			send_tcs_response(resp);
>> +		}
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +/**
>> + * tcs_notify_tx_done: TX Done for requests that got a response
>> + *
>> + * @data: the tasklet argument
>> + *
>> + * Tasklet function to notify MBOX that we are done with the request.
>> + * Handles all pending reponses whenever run.
>> + */
>> +static void tcs_notify_tx_done(unsigned long data)
>
>I think you should drop the tasklet and just invoke the handling of
>responses directly from the irq context.
>
I think, it will affect performance in multi-threaded usecases if we do
that. It is performance critical that we free up the TCS as soon as
possible. We only have 2 of them and there could be multiple clients
competing for that at the same time. Any work offloaded from the IRQ
handler will help move the pipe faster.

>> +{
>> +	struct rsc_drv *drv = (struct rsc_drv *)data;
>> +	struct tcs_response *resp;
>> +	unsigned long flags;
>> +	int err;
>> +
>> +	do {
>> +		spin_lock_irqsave(&drv->drv_lock, flags);
>> +		if (list_empty(&drv->response_pending)) {
>> +			spin_unlock_irqrestore(&drv->drv_lock, flags);
>> +			break;
>> +		}
>> +		resp = list_first_entry(&drv->response_pending,
>> +					struct tcs_response, list);
>> +		list_del(&resp->list);
>> +		spin_unlock_irqrestore(&drv->drv_lock, flags);
>> +		err = resp->err;
>> +		free_response(resp);
>> +	} while (1);
>
>Use for (;;) {} or while (1) {}, rather than do {} while(1)
>
OK.

>> +		curr_enabled = read_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0);
>> +
>> +		for (j = 0; j < MAX_CMDS_PER_TCS; j++) {
>> +			if (!(curr_enabled & (u32)BIT(j)))
>> +				continue;
>
>Consider using for_each_set_bit() here.
>
Ok

>> +/**
>> + * rpmh_rsc_send_data: Validate the incoming message and write to the
>> + * appropriate TCS block.
>> + *
>> + * @drv: the controller
>> + * @msg: the data to be sent
>> + *
>> + * Returns a negative error for invalid message structure and invalid
>> + * message combination, -EBUSY if there is an other active request for
>> + * the channel in process, otherwise bubbles up internal error.
>
>Kernel-doc style is "Return: a negative..."
>
>The function never returns -EBUSY, as you're looping while this is being
>returned.
>
>Also, it would be nice if the doc mentions that 0 is returned on
>success, so perhaps just "Return: 0 on success, negative errno on
>error"?
>
Sure.

>> + */
>> +int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_mbox_msg *msg)
>> +{
>> +	int ret = 0;
>> +
>> +	if (!msg || !msg->payload || !msg->num_payload ||
>> +			msg->num_payload > MAX_RPMH_PAYLOAD)
>> +		return -EINVAL;
>> +
>> +	do {
>> +		ret = tcs_mbox_write(drv, msg);
>> +		if (ret == -EBUSY) {
>> +			pr_info_ratelimited(
>> +			"TCS Busy, retrying RPMH message send: addr=0x%x\n",
>> +			msg->payload[0].addr);
>
>Indent wrapped lines to the start parenthesis.
>
It was hard with the message overflowing the 80 char limit. Will try to
clean it up.

>> +			udelay(10);
>> +		}
>> +	} while (ret == -EBUSY);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(rpmh_rsc_send_data);
>> +
>> +static int rpmh_rsc_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *dn = pdev->dev.of_node;
>> +	struct rsc_drv *drv;
>> +	struct tcs_mbox *tcs;
>> +	int irq;
>> +	u32 val[8] = { 0 };
>
>8 here should be TCS_TYPE_NR * 2, but I like Tomas(?) suggestion in the
>PDC driver of representing arrays of tuples like this as a structured
>type.
>
OK.

>> +	int st = 0;
>> +	int i, ret, nelem;
>> +	u32 config, max_tcs, ncpt;
>> +	int tcs_type_count[TCS_TYPE_NR] = { 0 };
>> +	struct resource *res;
>> +
>> +	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
>> +	if (!drv)
>> +		return -ENOMEM;
>> +
>> +	ret = of_property_read_u32(dn, "qcom,drv-id", &drv->drv_id);
>> +	if (ret)
>> +		return ret;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res)
>> +		return -EINVAL;
>> +	drv->base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(drv->base))
>> +		return PTR_ERR(drv->base);
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +	if (!res)
>> +		return -EINVAL;
>> +	drv->reg_base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(drv->reg_base))
>> +		return PTR_ERR(drv->reg_base);
>> +
>> +	config = read_drv_config(drv->base);
>> +	max_tcs = config & (DRV_NUM_TCS_MASK <<
>> +				(DRV_NUM_TCS_SHIFT * drv->drv_id));
>> +	max_tcs = max_tcs >> (DRV_NUM_TCS_SHIFT * drv->drv_id);
>> +	ncpt = config & (DRV_NCPT_MASK << DRV_NCPT_SHIFT);
>> +	ncpt = ncpt >> DRV_NCPT_SHIFT;
>> +
>> +	nelem = of_property_count_elems_of_size(dn, "qcom,tcs-config",
>> +						sizeof(u32));
>> +	if (!nelem || (nelem % 2) || (nelem > 2 * TCS_TYPE_NR))
>> +		return -EINVAL;
>> +
>> +	ret = of_property_read_u32_array(dn, "qcom,tcs-config", val, nelem);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < (nelem / 2); i++) {
>> +		if (val[2 * i] >= TCS_TYPE_NR)
>> +			return -EINVAL;
>> +		tcs_type_count[val[2 * i]]++;
>> +		if (tcs_type_count[val[2 * i]] > 1)
>> +			return -EINVAL;
>> +	}
>> +
>> +	for (i = 0; i < ARRAY_SIZE(tcs_type_count); i++)
>> +		if (!tcs_type_count[i])
>> +			return -EINVAL;
>
>I think it's better if replace these two loops by adding a check in
>below loop to ensure that tcs->drv is NULL and that tcs->type is valid
>and then follow up with a loop checking that each drv->tcs was
>initialized.
>
>That way you don't need to do the counting of the various types.
>
I need to count to ensure that all types are mentioned in the DT.

>> +
>> +	for (i = 0; i < (nelem / 2); i++) {
>> +		tcs = &drv->tcs[val[2 * i]];
>> +		tcs->drv = drv;
>> +		tcs->type = val[2 * i];
>> +		tcs->num_tcs = val[2 * i + 1];
>> +		tcs->ncpt = ncpt;
>> +		spin_lock_init(&tcs->tcs_lock);
>> +
>> +		if (tcs->num_tcs <= 0 || tcs->type == CONTROL_TCS)
>> +			continue;
>> +
>> +		if (tcs->num_tcs > MAX_TCS_PER_TYPE ||
>> +			st + tcs->num_tcs > max_tcs ||
>> +			st + tcs->num_tcs >= 8 * sizeof(tcs->tcs_mask))
>> +			return -EINVAL;
>> +
>> +		tcs->tcs_mask = ((1 << tcs->num_tcs) - 1) << st;
>> +		tcs->tcs_offset = st;
>> +		st += tcs->num_tcs;
>> +	}
>> +
>> +	drv->num_tcs = st;
>> +	drv->pdev = pdev;
>> +	INIT_LIST_HEAD(&drv->response_pending);
>> +	spin_lock_init(&drv->drv_lock);
>> +	tasklet_init(&drv->tasklet, tcs_notify_tx_done, (unsigned long)drv);
>> +
>> +	drv->name = of_get_property(pdev->dev.of_node, "label", NULL);
>> +	if (!drv->name)
>> +		drv->name = dev_name(&pdev->dev);
>> +
>> +	irq = of_irq_get(dn, 0);
>> +	if (irq < 0)
>> +		return irq;
>> +
>> +	ret = devm_request_irq(&pdev->dev, irq, tcs_irq_handler,
>> +			IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND, drv->name, drv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	write_tcs_reg(drv->reg_base, RSC_DRV_IRQ_ENABLE, 0, 0,
>> +					drv->tcs[ACTIVE_TCS].tcs_mask);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(drv->tcs_in_use); i++)
>> +		atomic_set(&drv->tcs_in_use[i], 0);
>
>Perhaps do this before requesting the irq? If nothing else that saves
>future readers from wondering if it makes any difference...
>
Ok. No difference to me.

>> +
>> +	dev_set_drvdata(&pdev->dev, drv);
>
>This isn't used until the next patch, so move it there to clarify its
>purpose.
>
OK.

>> +
>> +	return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>> +}
>> +
>> +static const struct of_device_id rpmh_drv_match[] = {
>> +	{ .compatible = "qcom,rpmh-rsc", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, rpm_drv_match);
>> +
>> +static struct platform_driver rpmh_driver = {
>> +	.probe = rpmh_rsc_probe,
>> +	.driver = {
>> +		.name = KBUILD_MODNAME,
>> +		.of_match_table = rpmh_drv_match,
>
>As we don't want to handle unbind, add:
>
>                .suppress_bind_attrs = true,
>
OK.

>> +	},
>> +};
>> +
>> diff --git a/include/dt-bindings/soc/qcom,rpmh-rsc.h b/include/dt-bindings/soc/qcom,rpmh-rsc.h
>> new file mode 100644
>> index 000000000000..24a56a5cf096
>> --- /dev/null
>> +++ b/include/dt-bindings/soc/qcom,rpmh-rsc.h

>> +struct tcs_cmd {
>> +	u32 addr;		/* slv_id:18:16 | offset:0:15 */
>> +	u32 data;		/* data for resource (or read response) */
>> +	bool complete;		/* wait for completion before sending next */
>> +};
>
>I think you should use kernel-doc to describe the structs.
>
Ok.

>> +
>> +enum rpmh_state {
>> +	RPMH_ACTIVE_ONLY_STATE,	/* Active only (= AMC) */
>> +	RPMH_WAKE_ONLY_STATE,	/* Wake only */
>> +	RPMH_SLEEP_STATE,	/* Sleep */
>> +};
>> +
>> +struct tcs_mbox_msg {
>
>struct tcs_request ?
>
Hmm.. ok

>> +	enum rpmh_state state;	/* request state */
>> +	bool is_complete;	/* wait for resp from accelerator */
>> +	u32 num_payload;	/* Limited to MAX_RPMH_PAYLOAD in one msg */
>> +	struct tcs_cmd *payload;/* array of tcs_cmds */
>> +};
>> +
>> +#endif /* __SOC_QCOM_TCS_H__ */
>

Thanks for the review Bjorn.

-- Lina

--
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