Hi Arun, Thanks for the review. On 8/22/2017 11:28 AM, Arun Kumar Neelakantam wrote: > > > On 8/16/2017 10:48 PM, Sricharan R wrote: >> + >> +struct glink_msg { >> + __le16 cmd; >> + __le16 param1; >> + __le32 param2; >> + u8 data[]; >> +} __packed; > > why we are using extra u8 data[] member here ? > Just as a zero-sized placeholder, to read a variable length header if required.
>> + >> +/** >> + * struct glink_defer_cmd - deferred incoming control message >> + * @node: list node >> + * @msg: message header >> + * data: payload of the message >> + * >> + * Copy of a received control message, to be added to @rx_queue and >> processed >> + * by @rx_work of @glink_rpm. >> + */ >> +struct glink_defer_cmd { >> + struct list_head node; >> + >> + struct glink_msg msg; >> + u8 data[]; >> +}; >> + >> +/** >> + * struct glink_rpm - driver context, relates to one remote subsystem > > glink_rpm to qcom_glink > ok, will change. >> +static int qcom_glink_tx(struct qcom_glink *glink, >> + const void *hdr, size_t hlen, >> + const void *data, size_t dlen, bool wait) >> +{ >> + unsigned int tlen = hlen + dlen; >> + int ret; >> + >> + /* Reject packets that are too big */ >> + if (tlen >= glink->tx_pipe->length) >> + return -EINVAL; > > we need to add support for split packets, in some cases packets may be > greater than 16K. ok, the plan was to add that in next set of patches and keep this series to a minimum. >> + >> + if (WARN(tlen % 8, "Unaligned TX request")) >> + return -EINVAL; >> + >> + ret = mutex_lock_interruptible(&glink->tx_lock); >> + if (ret) >> + return ret; >> + >> + while (qcom_glink_tx_avail(glink) < tlen) { >> + if (!wait) { >> + ret = -ENOMEM; > > This condition is either reader is slow or not reading data, should we return > -EAGAIN here instead of -ENOMEM? ok, will change. >> + goto out; >> + } >> + >> + msleep(10); >> + } >> + >> + qcom_glink_tx_write(glink, hdr, hlen, data, dlen); >> + >> + mbox_send_message(glink->mbox_chan, NULL); >> + mbox_client_txdone(glink->mbox_chan, 0); >> + >> +out: >> + mutex_unlock(&glink->tx_lock); >> + >> + return ret; >> +} >> + > > >> +/** >> + * qcom_glink_send_open_req() - send a RPM_CMD_OPEN request to the remote >> + * @glink: >> + * @channel: > > Missed information for @ glink and @channel ok, will add. >> + * >> + * Allocates a local channel id and sends a RPM_CMD_OPEN message to the >> remote. >> + * Will return with refcount held, regardless of outcome. >> + * >> + * Returns 0 on success, negative errno otherwise. >> + */ >> +static int qcom_glink_send_open_req(struct qcom_glink *glink, >> + struct glink_channel *channel) > > >> +static irqreturn_t qcom_glink_native_intr(int irq, void *data) >> +{ >> + struct qcom_glink *glink = data; >> + struct glink_msg msg; >> + unsigned int param1; >> + unsigned int param2; >> + unsigned int avail; >> + unsigned int cmd; >> + int ret; >> + >> + for (;;) { >> + avail = qcom_glink_rx_avail(glink); >> + if (avail < sizeof(msg)) >> + break; >> + >> + qcom_glink_rx_peak(glink, &msg, sizeof(msg)); >> + >> + cmd = le16_to_cpu(msg.cmd); >> + param1 = le16_to_cpu(msg.param1); >> + param2 = le32_to_cpu(msg.param2); >> + >> + switch (cmd) { >> + case RPM_CMD_VERSION: >> + case RPM_CMD_VERSION_ACK: >> + case RPM_CMD_CLOSE: >> + case RPM_CMD_CLOSE_ACK: >> + ret = qcom_glink_rx_defer(glink, 0); >> + break; >> + case RPM_CMD_OPEN_ACK: >> + ret = qcom_glink_rx_open_ack(glink, param1); >> + qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8)); >> + break; >> + case RPM_CMD_OPEN: >> + ret = qcom_glink_rx_defer(glink, param2); >> + break; >> + case RPM_CMD_TX_DATA: >> + case RPM_CMD_TX_DATA_CONT: >> + ret = qcom_glink_rx_data(glink, avail); >> + break; >> + case RPM_CMD_READ_NOTIF: >> + qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8)); >> + >> + mbox_send_message(glink->mbox_chan, NULL); >> + mbox_client_txdone(glink->mbox_chan, 0); >> + >> + ret = 0; >> + break; >> + default: >> + dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd); > > please add more information in error log to find the remote peripheral also > other wise after adding SMEM transport it will be difficult to find for which > peripheral the error came. ok, you refer to logging the "edge" name, that should be taken care by the dev_err print ? That said, i think the dev name in glink smem can be modified to reflect the edge name instead of simply printing the remoteproc name. Will change that. >> + ret = -EINVAL; >> + break; >> + } >> + >> + if (ret) >> + break; >> + } >> + >> + return IRQ_HANDLED; >> +} >> + > >> +struct qcom_glink *qcom_glink_native_probe(struct device *dev, >> + struct qcom_glink_pipe *rx, >> + struct qcom_glink_pipe *tx) >> +{ >> + int irq; >> + int ret; >> + struct qcom_glink *glink; >> + >> + glink = devm_kzalloc(dev, sizeof(*glink), GFP_KERNEL); >> + if (!glink) >> + return ERR_PTR(-ENOMEM); >> + >> + glink->dev = dev; >> + glink->tx_pipe = tx; >> + glink->rx_pipe = rx; >> + >> + mutex_init(&glink->tx_lock); >> + spin_lock_init(&glink->rx_lock); >> + INIT_LIST_HEAD(&glink->rx_queue); >> + INIT_WORK(&glink->rx_work, qcom_glink_work); >> + >> + mutex_init(&glink->idr_lock); >> + idr_init(&glink->lcids); >> + idr_init(&glink->rcids); >> + >> + glink->mbox_client.dev = dev; >> + glink->mbox_chan = mbox_request_channel(&glink->mbox_client, 0); >> + if (IS_ERR(glink->mbox_chan)) { >> + if (PTR_ERR(glink->mbox_chan) != -EPROBE_DEFER) >> + dev_err(dev, "failed to acquire IPC channel\n"); >> + return ERR_CAST(glink->mbox_chan); >> + } >> + >> + irq = of_irq_get(dev->of_node, 0); >> + ret = devm_request_irq(dev, irq, >> + qcom_glink_native_intr, >> + IRQF_NO_SUSPEND | IRQF_SHARED, >> + "glink-native", glink); >> + if (ret) { >> + dev_err(dev, "failed to request IRQ\n"); >> + return ERR_PTR(ret); >> + } > > These IRQs are wake-up-capable, please use enable_irq_wale() also ok. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus