mbox series

[0/2] slimbus: Add QCOM SLIMBus NGD driver

Message ID 20180516165118.16551-1-srinivas.kandagatla@linaro.org
Headers show
Series slimbus: Add QCOM SLIMBus NGD driver | expand

Message

Srinivas Kandagatla May 16, 2018, 4:51 p.m. UTC 
This patchset adds support to basic version of Qualcomm NGD SLIMBus
controller driver found SoCs from B family.

This controller is light-weight SLIMBus controller driver responsible for
communicating with slave HW directly over the bus using messaging
interface, and communicating with master component residing on ADSP
for bandwidth and data-channel management.

Tested this patchset on DB820c with WCD9335 codec.
I have pushed my working branch to [1] incase someone want to try.
 

Thanks,
srini

[1] https://git.linaro.org/people/srinivas.kandagatla/linux.git/log/?h=slimbus-ngd

Srinivas Kandagatla (2):
  slimbus: ngd: dt-bindings: Add slim ngd dt bindings
  slimbus: ngd: Add qcom SLIMBus NGD driver

 .../bindings/slimbus/slim-ngd-qcom-ctrl.txt        |   70 ++
 drivers/slimbus/Kconfig                            |   10 +
 drivers/slimbus/Makefile                           |    3 +
 drivers/slimbus/qcom-ngd-ctrl.c                    | 1298 ++++++++++++++++++++
 drivers/slimbus/slimbus.h                          |    8 +
 5 files changed, 1389 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/slimbus/slim-ngd-qcom-ctrl.txt
 create mode 100644 drivers/slimbus/qcom-ngd-ctrl.c

Comments

Vinod Koul May 21, 2018, 11:33 a.m. UTC | #1 
On 16-05-18, 17:51, Srinivas Kandagatla wrote:
> This patch adds suppor to Qualcomm SLIMBus Non-Generic Device (NGD)

/s/suppor/support

> +/* NGD (Non-ported Generic Device) registers */
> +#define	NGD_CFG			0x0
> +#define	NGD_CFG_ENABLE		BIT(0)
> +#define	NGD_CFG_RX_MSGQ_EN	BIT(1)
> +#define	NGD_CFG_TX_MSGQ_EN	BIT(2)
> +#define	NGD_STATUS		0x4
> +#define NGD_LADDR		BIT(1)
> +#define	NGD_RX_MSGQ_CFG		0x8
> +#define	NGD_INT_EN		0x10
> +#define	NGD_INT_RECFG_DONE	BIT(24)
> +#define	NGD_INT_TX_NACKED_2	BIT(25)
> +#define	NGD_INT_MSG_BUF_CONTE	BIT(26)
> +#define	NGD_INT_MSG_TX_INVAL	BIT(27)
> +#define	NGD_INT_IE_VE_CHG	BIT(28)
> +#define	NGD_INT_DEV_ERR		BIT(29)
> +#define	NGD_INT_RX_MSG_RCVD	BIT(30)
> +#define	NGD_INT_TX_MSG_SENT	BIT(31)
> +#define	NGD_INT_STAT		0x14
> +#define	NGD_INT_CLR		0x18
> +#define DEF_NGD_INT_MASK (NGD_INT_TX_NACKED_2 | NGD_INT_MSG_BUF_CONTE | \
> +				NGD_INT_MSG_TX_INVAL | NGD_INT_IE_VE_CHG | \
> +				NGD_INT_DEV_ERR | NGD_INT_TX_MSG_SENT | \
> +				NGD_INT_RX_MSG_RCVD)
> +
> +/* Slimbus QMI service */
> +#define SLIMBUS_QMI_SVC_ID 0x0301
> +#define SLIMBUS_QMI_SVC_V1 1
> +#define SLIMBUS_QMI_INS_ID 0
> +#define SLIMBUS_QMI_SELECT_INSTANCE_REQ_V01 0x0020
> +#define SLIMBUS_QMI_SELECT_INSTANCE_RESP_V01 0x0020
> +#define SLIMBUS_QMI_POWER_REQ_V01 0x0021
> +#define SLIMBUS_QMI_POWER_RESP_V01 0x0021
> +#define SLIMBUS_QMI_CHECK_FRAMER_STATUS_REQ 0x0022
> +#define SLIMBUS_QMI_CHECK_FRAMER_STATUS_RESP 0x0022
> +#define SLIMBUS_QMI_POWER_REQ_MAX_MSG_LEN 14
> +#define SLIMBUS_QMI_POWER_RESP_MAX_MSG_LEN 7
> +#define SLIMBUS_QMI_SELECT_INSTANCE_REQ_MAX_MSG_LEN 14
> +#define SLIMBUS_QMI_SELECT_INSTANCE_RESP_MAX_MSG_LEN 7
> +#define SLIMBUS_QMI_CHECK_FRAMER_STAT_RESP_MAX_MSG_LEN 7
> +/* QMI response timeout of 500ms */
> +#define SLIMBUS_QMI_RESP_TOUT 1000

Tabs or spaces, take your pick and use one, not both...

> +static void qcom_slim_qmi_power_resp_cb(struct qmi_handle *handle,
> +					struct sockaddr_qrtr *sq,
> +					struct qmi_txn *txn, const void *data)
> +{
> +	struct slimbus_power_resp_msg_v01 *resp;
> +
> +	resp = (struct slimbus_power_resp_msg_v01 *)data;

you dont need cast away from void

> +	if (resp->resp.result != QMI_RESULT_SUCCESS_V01)
> +		pr_err("%s: QMI power request failed 0x%x\n", __func__,
> +				resp->resp.result);

cant we use dev_err?

> +static int qcom_slim_qmi_send_power_request(struct qcom_slim_ngd_ctrl *ctrl,
> +				struct slimbus_power_req_msg_v01 *req)
> +{
> +	struct slimbus_power_resp_msg_v01 resp = { { 0, 0 } };
> +	struct qmi_txn txn;
> +	int rc;
> +
> +	rc = qmi_txn_init(ctrl->qmi.handle, &txn,
> +				slimbus_power_resp_msg_v01_ei, &resp);
> +
> +	rc = qmi_send_request(ctrl->qmi.handle, NULL, &txn,
> +				SLIMBUS_QMI_POWER_REQ_V01,
> +				SLIMBUS_QMI_POWER_REQ_MAX_MSG_LEN,
> +				slimbus_power_req_msg_v01_ei, req);
> +	if (rc < 0) {
> +		dev_err(ctrl->dev, "%s: QMI send req fail %d\n", __func__, rc);
> +		qmi_txn_cancel(&txn);
> +	}
> +
> +	if (rc < 0)
> +		return rc;

why not add this is prev error check?

> +static int qcom_slim_qmi_init(struct qcom_slim_ngd_ctrl *ctrl,
> +			      bool apps_is_master)
> +{
> +	int rc = 0;

superfluous init

> +static u32 *qcom_slim_ngd_tx_msg_get(struct qcom_slim_ngd_ctrl *ctrl, int len,
> +				     struct completion *comp)
> +{
> +	struct qcom_slim_ngd_dma_desc *desc;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ctrl->tx_buf_lock, flags);
> +
> +	if ((ctrl->tx_tail + 1) % QCOM_SLIM_NGD_DESC_NUM == ctrl->tx_head) {
> +		spin_unlock_irqrestore(&ctrl->tx_buf_lock, flags);
> +		return NULL;
> +	}
> +	desc  = &ctrl->txdesc[ctrl->tx_tail];
> +	desc->base = (u32 *)((u8 *)ctrl->tx_base +
> +				(ctrl->tx_tail * SLIM_MSGQ_BUF_LEN));

too many casts

> +static int qcom_slim_ngd_post_rx_msgq(struct qcom_slim_ngd_ctrl *ctrl)
> +{
> +	struct qcom_slim_ngd_dma_desc *desc;
> +	struct dma_slave_config conf = {
> +		.direction = DMA_DEV_TO_MEM,
> +	};
> +	int ret, i;
> +
> +	ret = dmaengine_slave_config(ctrl->dma_rx_channel, &conf);

so you are only setting direction for conf, not any other parameters? If not why
bother setting direction

> +	if (ret)
> +		dev_err(ctrl->dev, "Error Configuring rx dma\n");
> +
> +	for (i = 0; i < QCOM_SLIM_NGD_DESC_NUM; i++) {
> +		desc = &ctrl->rx_desc[i];
> +		desc->phys = ctrl->rx_phys_base + i * SLIM_MSGQ_BUF_LEN;
> +		desc->ctrl = ctrl;
> +		desc->base = ctrl->rx_base + i * SLIM_MSGQ_BUF_LEN;
> +		desc->desc = dmaengine_prep_slave_single(ctrl->dma_rx_channel,
> +						desc->phys, SLIM_MSGQ_BUF_LEN,
> +						DMA_DEV_TO_MEM,
> +						DMA_PREP_INTERRUPT);

why issue multiple slave_single to dmaengine, you can bundle them and issue
dmaengine_prep_slave_sg().. 

> +static int qcom_slim_ngd_qmi_svc_event_init(struct qcom_slim_ngd_qmi *qmi)
> +{
> +	int ret = 0;

superfluous init here too

> +static int qcom_slim_ngd_runtime_idle(struct device *dev)
> +{
> +	struct qcom_slim_ngd_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	if (ctrl->state == QCOM_SLIM_NGD_CTRL_AWAKE)
> +		ctrl->state = QCOM_SLIM_NGD_CTRL_IDLE;
> +	pm_request_autosuspend(dev);
> +	return -EAGAIN;
> +}
> +
> +

double empty lines, here
Srinivas Kandagatla May 21, 2018, 11:55 a.m. UTC | #2 
Thanks Vinod for the review!

On 21/05/18 12:33, Vinod wrote:
> On 16-05-18, 17:51, Srinivas Kandagatla wrote:
>> This patch adds suppor to Qualcomm SLIMBus Non-Generic Device (NGD)
> 
> /s/suppor/support
> 
Yep, Will fix this in next version.

>> +/* NGD (Non-ported Generic Device) registers */
>> +#define	NGD_CFG			0x0
>> +#define	NGD_CFG_ENABLE		BIT(0)
>> +#define	NGD_CFG_RX_MSGQ_EN	BIT(1)
>> +#define	NGD_CFG_TX_MSGQ_EN	BIT(2)
>> +#define	NGD_STATUS		0x4
>> +#define NGD_LADDR		BIT(1)
>> +#define	NGD_RX_MSGQ_CFG		0x8
>> +#define	NGD_INT_EN		0x10
>> +#define SLIMBUS_QMI_POWER_RESP_MAX_MSG_LEN 7
>> +#define SLIMBUS_QMI_SELECT_INSTANCE_REQ_MAX_MSG_LEN 14
>> +#define SLIMBUS_QMI_SELECT_INSTANCE_RESP_MAX_MSG_LEN 7
>> +#define SLIMBUS_QMI_CHECK_FRAMER_STAT_RESP_MAX_MSG_LEN 7
>> +/* QMI response timeout of 500ms */
>> +#define SLIMBUS_QMI_RESP_TOUT 1000
> 
> Tabs or spaces, take your pick and use one, not both...
I will check for such instances once again before sending v2.

> 
>> +static void qcom_slim_qmi_power_resp_cb(struct qmi_handle *handle,
>> +					struct sockaddr_qrtr *sq,
>> +					struct qmi_txn *txn, const void *data)
>> +{
>> +	struct slimbus_power_resp_msg_v01 *resp;
>> +
>> +	resp = (struct slimbus_power_resp_msg_v01 *)data;
> 
> you dont need cast away from void
It's a const void * so the compiler keeps complaining about this without 
cast.

> 
>> +	if (resp->resp.result != QMI_RESULT_SUCCESS_V01)
>> +		pr_err("%s: QMI power request failed 0x%x\n", __func__,
>> +				resp->resp.result);
> 
> cant we use dev_err?
I will have a look!
> 
>> +static int qcom_slim_qmi_send_power_request(struct qcom_slim_ngd_ctrl *ctrl,
>> +				struct slimbus_power_req_msg_v01 *req)
>> +{
>> +	struct slimbus_power_resp_msg_v01 resp = { { 0, 0 } };
>> +	struct qmi_txn txn;
>> +	int rc;
>> +
>> +	rc = qmi_txn_init(ctrl->qmi.handle, &txn,
>> +				slimbus_power_resp_msg_v01_ei, &resp);
>> +
>> +	rc = qmi_send_request(ctrl->qmi.handle, NULL, &txn,
>> +				SLIMBUS_QMI_POWER_REQ_V01,
>> +				SLIMBUS_QMI_POWER_REQ_MAX_MSG_LEN,
>> +				slimbus_power_req_msg_v01_ei, req);
>> +	if (rc < 0) {
>> +		dev_err(ctrl->dev, "%s: QMI send req fail %d\n", __func__, rc);
>> +		qmi_txn_cancel(&txn);
>> +	}
>> +
>> +	if (rc < 0)
>> +		return rc;
> 
> why not add this is prev error check?
Yes, we should move this to previous check, Will fix this in v2.
> 
>> +static int qcom_slim_qmi_init(struct qcom_slim_ngd_ctrl *ctrl,
>> +			      bool apps_is_master)
>> +{
>> +	int rc = 0;
> 
> superfluous init
> 
>> +static u32 *qcom_slim_ngd_tx_msg_get(struct qcom_slim_ngd_ctrl *ctrl, int len,
>> +				     struct completion *comp)
>> +{
>> +	struct qcom_slim_ngd_dma_desc *desc;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&ctrl->tx_buf_lock, flags);
>> +
>> +	if ((ctrl->tx_tail + 1) % QCOM_SLIM_NGD_DESC_NUM == ctrl->tx_head) {
>> +		spin_unlock_irqrestore(&ctrl->tx_buf_lock, flags);
>> +		return NULL;
>> +	}
>> +	desc  = &ctrl->txdesc[ctrl->tx_tail];
>> +	desc->base = (u32 *)((u8 *)ctrl->tx_base +
>> +				(ctrl->tx_tail * SLIM_MSGQ_BUF_LEN));
> 
> too many casts
> 
>> +static int qcom_slim_ngd_post_rx_msgq(struct qcom_slim_ngd_ctrl *ctrl)
>> +{
>> +	struct qcom_slim_ngd_dma_desc *desc;
>> +	struct dma_slave_config conf = {
>> +		.direction = DMA_DEV_TO_MEM,
>> +	};
>> +	int ret, i;
>> +
>> +	ret = dmaengine_slave_config(ctrl->dma_rx_channel, &conf);
> 
> so you are only setting direction for conf, not any other parameters? If not why
> bother setting direction
Nice hint, I will remove this!
> 
>> +	if (ret)
>> +		dev_err(ctrl->dev, "Error Configuring rx dma\n");
>> +
>> +	for (i = 0; i < QCOM_SLIM_NGD_DESC_NUM; i++) {
>> +		desc = &ctrl->rx_desc[i];
>> +		desc->phys = ctrl->rx_phys_base + i * SLIM_MSGQ_BUF_LEN;
>> +		desc->ctrl = ctrl;
>> +		desc->base = ctrl->rx_base + i * SLIM_MSGQ_BUF_LEN;
>> +		desc->desc = dmaengine_prep_slave_single(ctrl->dma_rx_channel,
>> +						desc->phys, SLIM_MSGQ_BUF_LEN,
>> +						DMA_DEV_TO_MEM,
>> +						DMA_PREP_INTERRUPT);
> 
> why issue multiple slave_single to dmaengine, you can bundle them and issue
> dmaengine_prep_slave_sg()..
Am reusing the descriptors here, My plan is to issue multiple 
descriptors with callback for each, and in each callback queue it back.

> 
>> +static int qcom_slim_ngd_qmi_svc_event_init(struct qcom_slim_ngd_qmi *qmi)
>> +{
>> +	int ret = 0;
> 
> superfluous init here too
> 
Yep.
>> +static int qcom_slim_ngd_runtime_idle(struct device *dev)
>> +{
>> +	struct qcom_slim_ngd_ctrl *ctrl = dev_get_drvdata(dev);
>> +
>> +	if (ctrl->state == QCOM_SLIM_NGD_CTRL_AWAKE)
>> +		ctrl->state = QCOM_SLIM_NGD_CTRL_IDLE;
>> +	pm_request_autosuspend(dev);
>> +	return -EAGAIN;
>> +}
>> +
>> +
> 
> double empty lines, here
Sure, will fix this in v2.
> 

thanks,
srini
--
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