Re: [PATCH V2 2/3] rpmsg: glink: Add support to handle signals command

From: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
To: Deepak Kumar Singh <quic_deesin@quicinc.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: <swboyd@chromium.org>, <quic_clew@quicinc.com>,
	<mathieu.poirier@linaro.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-msm@vger.kernel.org>,
	<linux-remoteproc@vger.kernel.org>,
	Andy Gross <agross@kernel.org>, Ohad Ben-Cohen <ohad@wizery.com>
Subject: Re: [PATCH V2 2/3] rpmsg: glink: Add support to handle signals command
Date: Fri, 1 Apr 2022 15:27:36 +0200	[thread overview]
Message-ID: <57c63984-ddf1-6cd7-40a3-aadf9405da5a@foss.st.com> (raw)
In-Reply-To: <e59b5d03-0737-56ac-c0af-058799bcb88d@quicinc.com>

On 3/23/22 08:20, Deepak Kumar Singh wrote:
> 
> On 3/12/2022 2:39 AM, Bjorn Andersson wrote:
>> On Tue 18 Jan 13:43 CST 2022, Deepak Kumar Singh wrote:
>>
>>> Remote peripherals send signal notifications over glink with
>>> commandID 15.
>>>
>>> Add support to send and receive the signal command and convert the
>>> signals
>>> from NATIVE to TIOCM while receiving and vice versa while sending.
>>>
>>> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
>> Co-developed-by: seems appropriate here, or you need to ensure the
>> author remains Chris, as his S-o-b comes first.
>>
>>> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
>>> ---
>>>   drivers/rpmsg/qcom_glink_native.c | 77
>>> +++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 77 insertions(+)
>>>
>>> diff --git a/drivers/rpmsg/qcom_glink_native.c
>>> b/drivers/rpmsg/qcom_glink_native.c
>>> index 3f377a7..d673d65 100644
>>> --- a/drivers/rpmsg/qcom_glink_native.c
>>> +++ b/drivers/rpmsg/qcom_glink_native.c
>>> @@ -17,6 +17,7 @@
>>>   #include <linux/rpmsg.h>
>>>   #include <linux/sizes.h>
>>>   #include <linux/slab.h>
>>> +#include <linux/termios.h>
>>>   #include <linux/workqueue.h>
>>>   #include <linux/mailbox_client.h>
>>>   @@ -205,9 +206,16 @@ static const struct rpmsg_endpoint_ops
>>> glink_endpoint_ops;
>>>   #define RPM_CMD_TX_DATA_CONT        12
>>>   #define RPM_CMD_READ_NOTIF        13
>>>   #define RPM_CMD_RX_DONE_W_REUSE        14
>>> +#define RPM_CMD_SIGNALS            15
>>>     #define GLINK_FEATURE_INTENTLESS    BIT(1)
>>>   +#define NATIVE_DTR_SIG            BIT(31)
>> Seems reasonable to prefix these with GLINK_, perhaps GLINK_SIGNAL_DTR?
>>
>>> +#define NATIVE_CTS_SIG            BIT(30)
>>> +#define NATIVE_CD_SIG            BIT(29)
>>> +#define NATIVE_RI_SIG            BIT(28)
>>> +#define    SIG_MASK            0x0fff;
>>> +
>>>   static void qcom_glink_rx_done_work(struct work_struct *work);
>>>     static struct glink_channel *qcom_glink_alloc_channel(struct
>>> qcom_glink *glink,
>>> @@ -1003,6 +1011,70 @@ static int qcom_glink_rx_open_ack(struct
>>> qcom_glink *glink, unsigned int lcid)
>>>       return 0;
>>>   }
>>>   +/**
>>> + * qcom_glink_set_flow_control() - convert a signal cmd to wire
>>> format and
>>> + *                    transmit
>>> + * @ept:    Rpmsg endpoint for channel.
>>> + * @enable:    True/False - enable or disable flow control
>> "enable flow control" sounds sufficient (i.e. no need for True/False)
>> part.
>>
>> Regards,
>> Bjorn
> 
> There are some user space clients which require both flow control on and
> off (DTR high/low).
> 
> So i guess true and false both are needed.
> 
>>> + *
>>> + * Return: 0 on success or standard Linux error code.
>>> + */
>>> +static int qcom_glink_set_flow_control(struct rpmsg_endpoint *ept,
>>> bool enable)
>>> +{
>>> +    struct glink_channel *channel = to_glink_channel(ept);
>>> +    struct qcom_glink *glink = channel->glink;
>>> +    struct glink_msg msg;
>>> +    u32 sigs;
>>> +
>>> +    /**
>>> +     * convert signals from TIOCM to NATIVE
>>> +     * sigs = TIOCM_DTR|TIOCM_RTS
>>> +     */
>>> +    if (enable)
>>> +        sigs |= NATIVE_DTR_SIG | NATIVE_CTS_SIG;
>>> +    else
>>> +        sigs |= ~(NATIVE_DTR_SIG | NATIVE_CTS_SIG);
>>> +
>>> +    msg.cmd = cpu_to_le16(RPM_CMD_SIGNALS);
>>> +    msg.param1 = cpu_to_le16(channel->lcid);
>>> +    msg.param2 = cpu_to_le32(sigs);
>>> +
>>> +    return qcom_glink_tx(glink, &msg, sizeof(msg), NULL, 0, true);
>>> +}
>>> +
>>> +static int qcom_glink_handle_signals(struct qcom_glink *glink,
>>> +                     unsigned int rcid, unsigned int sigs)
>>> +{
>>> +    struct glink_channel *channel;
>>> +    unsigned long flags;
>>> +
>>> +    spin_lock_irqsave(&glink->idr_lock, flags);
>>> +    channel = idr_find(&glink->rcids, rcid);
>>> +    spin_unlock_irqrestore(&glink->idr_lock, flags);
>>> +    if (!channel) {
>>> +        dev_err(glink->dev, "signal for non-existing channel\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (!channel->ept.sig_cb)
>>> +        return 0;
>>> +
>>> +    /* convert signals from NATIVE to TIOCM */
>>> +    if (sigs & NATIVE_DTR_SIG)

Regarding specs seems that DTR is from the DTE (Data Terminal Equipment)
to the DCE (Data Communication Equipement)
NATIVE_DSR_SIG instead?

Regards
Arnaud

>>> +        sigs |= TIOCM_DSR;
>>> +    if (sigs & NATIVE_CTS_SIG)
>>> +        sigs |= TIOCM_CTS;
>>> +    if (sigs & NATIVE_CD_SIG)
>>> +        sigs |= TIOCM_CD;
>>> +    if (sigs & NATIVE_RI_SIG)
>>> +        sigs |= TIOCM_RI;
>>> +    sigs &= SIG_MASK;
>>> +
>>> +    channel->ept.sig_cb(channel->ept.rpdev, channel->ept.priv, sigs);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>>>   {
>>>       struct qcom_glink *glink = data;
>>> @@ -1067,6 +1139,10 @@ static irqreturn_t qcom_glink_native_intr(int
>>> irq, void *data)
>>>               qcom_glink_handle_intent_req_ack(glink, param1, param2);
>>>               qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
>>>               break;
>>> +        case RPM_CMD_SIGNALS:
>>> +            qcom_glink_handle_signals(glink, param1, param2);
>>> +            qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
>>> +            break;
>>>           default:
>>>               dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
>>>               ret = -EINVAL;
>>> @@ -1442,6 +1518,7 @@ static const struct rpmsg_endpoint_ops
>>> glink_endpoint_ops = {
>>>       .sendto = qcom_glink_sendto,
>>>       .trysend = qcom_glink_trysend,
>>>       .trysendto = qcom_glink_trysendto,
>>> +    .set_flow_control = qcom_glink_set_flow_control,
>>>   };
>>>     static void qcom_glink_rpdev_release(struct device *dev)
>>> -- 
>>> 2.7.4
>>>