Re: [PATCH v2 02/20] rpmsg: glink: Associate indirections for pipe fifo accessor's

[Arun Kumar Neelakantam <aneela@codeaurora.org>]

On 8/24/2017 12:51 PM, Sricharan R wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> With the intention of reusing the glink core protocol commands
> and code across both rpm and smem based transports, the only thing
> different is way of accessing the shared-memory of the transport
> (FIFO). So put the fifo accessor's of the transport's pipe (rx/tx)
> behind indirections, so that the rest of the code can be shared.
>
> For this, have a qcom_glink_pipe that can be used in the common code
> containing the indirections and wrap it with glink_rpm_pipe that contains
> the transport specific members.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>

Acked-by: Arun Kumar Neelakantam <aneela@codeaurora.org>

Regards,
Arun N
> ---
>   drivers/rpmsg/qcom_glink_rpm.c | 144 ++++++++++++++++++++++++++++++-----------
>   1 file changed, 106 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/rpmsg/qcom_glink_rpm.c b/drivers/rpmsg/qcom_glink_rpm.c
> index 56a0a66..870ce32 100644
> --- a/drivers/rpmsg/qcom_glink_rpm.c
> +++ b/drivers/rpmsg/qcom_glink_rpm.c
> @@ -41,12 +41,28 @@
>   #define RPM_GLINK_CID_MIN	1
>   #define RPM_GLINK_CID_MAX	65536
>   
> +#define to_rpm_pipe(p)	container_of(p, struct glink_rpm_pipe, native)
> +
>   struct rpm_toc_entry {
>   	__le32 id;
>   	__le32 offset;
>   	__le32 size;
>   } __packed;
>   
> +struct qcom_glink;
> +
> +struct qcom_glink_pipe {
> +	size_t length;
> +
> +	size_t (*avail)(struct qcom_glink_pipe *glink_pipe);
> +	void (*peak)(struct qcom_glink_pipe *glink_pipe, void *data,
> +		     size_t count);
> +	void (*advance)(struct qcom_glink_pipe *glink_pipe, size_t count);
> +	void (*write)(struct qcom_glink_pipe *glink_pipe,
> +		      const void *hdr, size_t hlen,
> +		      const void *data, size_t dlen);
> +};
> +
>   struct rpm_toc {
>   	__le32 magic;
>   	__le32 count;
> @@ -62,12 +78,12 @@ struct glink_msg {
>   } __packed;
>   
>   struct glink_rpm_pipe {
> +	struct qcom_glink_pipe native;
> +
>   	void __iomem *tail;
>   	void __iomem *head;
>   
>   	void __iomem *fifo;
> -
> -	size_t length;
>   };
>   
>   /**
> @@ -107,8 +123,8 @@ struct qcom_glink {
>   	struct mbox_client mbox_client;
>   	struct mbox_chan *mbox_chan;
>   
> -	struct glink_rpm_pipe rx_pipe;
> -	struct glink_rpm_pipe tx_pipe;
> +	struct qcom_glink_pipe *rx_pipe;
> +	struct qcom_glink_pipe *tx_pipe;
>   
>   	int irq;
>   
> @@ -215,9 +231,9 @@ static void qcom_glink_channel_release(struct kref *ref)
>   	kfree(channel);
>   }
>   
> -static size_t qcom_glink_rx_avail(struct qcom_glink *glink)
> +static size_t glink_rpm_rx_avail(struct qcom_glink_pipe *glink_pipe)
>   {
> -	struct glink_rpm_pipe *pipe = &glink->rx_pipe;
> +	struct glink_rpm_pipe *pipe = to_rpm_pipe(glink_pipe);
>   	unsigned int head;
>   	unsigned int tail;
>   
> @@ -225,21 +241,26 @@ static size_t qcom_glink_rx_avail(struct qcom_glink *glink)
>   	tail = readl(pipe->tail);
>   
>   	if (head < tail)
> -		return pipe->length - tail + head;
> +		return pipe->native.length - tail + head;
>   	else
>   		return head - tail;
>   }
>   
> -static void qcom_glink_rx_peak(struct qcom_glink *glink,
> -			       void *data, size_t count)
> +static size_t qcom_glink_rx_avail(struct qcom_glink *glink)
> +{
> +	return glink->rx_pipe->avail(glink->rx_pipe);
> +}
> +
> +static void glink_rpm_rx_peak(struct qcom_glink_pipe *glink_pipe,
> +			      void *data, size_t count)
>   {
> -	struct glink_rpm_pipe *pipe = &glink->rx_pipe;
> +	struct glink_rpm_pipe *pipe = to_rpm_pipe(glink_pipe);
>   	unsigned int tail;
>   	size_t len;
>   
>   	tail = readl(pipe->tail);
>   
> -	len = min_t(size_t, count, pipe->length - tail);
> +	len = min_t(size_t, count, pipe->native.length - tail);
>   	if (len) {
>   		__ioread32_copy(data, pipe->fifo + tail,
>   				len / sizeof(u32));
> @@ -251,24 +272,35 @@ static void qcom_glink_rx_peak(struct qcom_glink *glink,
>   	}
>   }
>   
> -static void qcom_glink_rx_advance(struct qcom_glink *glink,
> -				  size_t count)
> +static void qcom_glink_rx_peak(struct qcom_glink *glink,
> +			       void *data, size_t count)
>   {
> -	struct glink_rpm_pipe *pipe = &glink->rx_pipe;
> +	glink->rx_pipe->peak(glink->rx_pipe, data, count);
> +}
> +
> +static void glink_rpm_rx_advance(struct qcom_glink_pipe *glink_pipe,
> +				 size_t count)
> +{
> +	struct glink_rpm_pipe *pipe = to_rpm_pipe(glink_pipe);
>   	unsigned int tail;
>   
>   	tail = readl(pipe->tail);
>   
>   	tail += count;
> -	if (tail >= pipe->length)
> -		tail -= pipe->length;
> +	if (tail >= pipe->native.length)
> +		tail -= pipe->native.length;
>   
>   	writel(tail, pipe->tail);
>   }
>   
> -static size_t qcom_glink_tx_avail(struct qcom_glink *glink)
> +static void qcom_glink_rx_advance(struct qcom_glink *glink, size_t count)
> +{
> +	glink->rx_pipe->advance(glink->rx_pipe, count);
> +}
> +
> +static size_t glink_rpm_tx_avail(struct qcom_glink_pipe *glink_pipe)
>   {
> -	struct glink_rpm_pipe *pipe = &glink->tx_pipe;
> +	struct glink_rpm_pipe *pipe = to_rpm_pipe(glink_pipe);
>   	unsigned int head;
>   	unsigned int tail;
>   
> @@ -276,19 +308,23 @@ static size_t qcom_glink_tx_avail(struct qcom_glink *glink)
>   	tail = readl(pipe->tail);
>   
>   	if (tail <= head)
> -		return pipe->length - head + tail;
> +		return pipe->native.length - head + tail;
>   	else
>   		return tail - head;
>   }
>   
> -static unsigned int qcom_glink_tx_write(struct qcom_glink *glink,
> -					unsigned int head,
> -					const void *data, size_t count)
> +static size_t qcom_glink_tx_avail(struct qcom_glink *glink)
> +{
> +	return glink->tx_pipe->avail(glink->tx_pipe);
> +}
> +
> +static unsigned int glink_rpm_tx_write_one(struct glink_rpm_pipe *pipe,
> +					   unsigned int head,
> +					   const void *data, size_t count)
>   {
> -	struct glink_rpm_pipe *pipe = &glink->tx_pipe;
>   	size_t len;
>   
> -	len = min_t(size_t, count, pipe->length - head);
> +	len = min_t(size_t, count, pipe->native.length - head);
>   	if (len) {
>   		__iowrite32_copy(pipe->fifo + head, data,
>   				 len / sizeof(u32));
> @@ -300,23 +336,41 @@ static unsigned int qcom_glink_tx_write(struct qcom_glink *glink,
>   	}
>   
>   	head += count;
> -	if (head >= pipe->length)
> -		head -= pipe->length;
> +	if (head >= pipe->native.length)
> +		head -= pipe->native.length;
>   
>   	return head;
>   }
>   
> +static void glink_rpm_tx_write(struct qcom_glink_pipe *glink_pipe,
> +			       const void *hdr, size_t hlen,
> +			       const void *data, size_t dlen)
> +{
> +	struct glink_rpm_pipe *pipe = to_rpm_pipe(glink_pipe);
> +	unsigned int head;
> +
> +	head = readl(pipe->head);
> +	head = glink_rpm_tx_write_one(pipe, head, hdr, hlen);
> +	head = glink_rpm_tx_write_one(pipe, head, data, dlen);
> +	writel(head, pipe->head);
> +}
> +
> +static void qcom_glink_tx_write(struct qcom_glink *glink,
> +				const void *hdr, size_t hlen,
> +				const void *data, size_t dlen)
> +{
> +	glink->tx_pipe->write(glink->tx_pipe, hdr, hlen, data, dlen);
> +}
> +
>   static int qcom_glink_tx(struct qcom_glink *glink,
>   			 const void *hdr, size_t hlen,
>   			const void *data, size_t dlen, bool wait)
>   {
> -	struct glink_rpm_pipe *pipe = &glink->tx_pipe;
> -	unsigned int head;
>   	unsigned int tlen = hlen + dlen;
>   	int ret;
>   
>   	/* Reject packets that are too big */
> -	if (tlen >= glink->tx_pipe.length)
> +	if (tlen >= glink->tx_pipe->length)
>   		return -EINVAL;
>   
>   	if (WARN(tlen % 8, "Unaligned TX request"))
> @@ -335,10 +389,7 @@ static int qcom_glink_tx(struct qcom_glink *glink,
>   		msleep(10);
>   	}
>   
> -	head = readl(pipe->head);
> -	head = qcom_glink_tx_write(glink, head, hdr, hlen);
> -	head = qcom_glink_tx_write(glink, head, data, dlen);
> -	writel(head, pipe->head);
> +	qcom_glink_tx_write(glink, hdr, hlen, data, dlen);
>   
>   	mbox_send_message(glink->mbox_chan, NULL);
>   	mbox_client_txdone(glink->mbox_chan, 0);
> @@ -1075,14 +1126,14 @@ static int glink_rpm_parse_toc(struct device *dev,
>   
>   		switch (id) {
>   		case RPM_RX_FIFO_ID:
> -			rx->length = size;
> +			rx->native.length = size;
>   
>   			rx->tail = msg_ram + offset;
>   			rx->head = msg_ram + offset + sizeof(u32);
>   			rx->fifo = msg_ram + offset + 2 * sizeof(u32);
>   			break;
>   		case RPM_TX_FIFO_ID:
> -			tx->length = size;
> +			tx->native.length = size;
>   
>   			tx->tail = msg_ram + offset;
>   			tx->head = msg_ram + offset + sizeof(u32);
> @@ -1107,6 +1158,8 @@ static int glink_rpm_parse_toc(struct device *dev,
>   static int glink_rpm_probe(struct platform_device *pdev)
>   {
>   	struct qcom_glink *glink;
> +	struct glink_rpm_pipe *rx_pipe;
> +	struct glink_rpm_pipe *tx_pipe;
>   	struct device_node *np;
>   	void __iomem *msg_ram;
>   	size_t msg_ram_size;
> @@ -1121,6 +1174,11 @@ static int glink_rpm_probe(struct platform_device *pdev)
>   
>   	glink->dev = dev;
>   
> +	rx_pipe = devm_kzalloc(&pdev->dev, sizeof(*rx_pipe), GFP_KERNEL);
> +	tx_pipe = devm_kzalloc(&pdev->dev, sizeof(*tx_pipe), GFP_KERNEL);
> +	if (!rx_pipe || !tx_pipe)
> +		return -ENOMEM;
> +
>   	mutex_init(&glink->tx_lock);
>   	spin_lock_init(&glink->rx_lock);
>   	INIT_LIST_HEAD(&glink->rx_queue);
> @@ -1150,12 +1208,22 @@ static int glink_rpm_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	ret = glink_rpm_parse_toc(dev, msg_ram, msg_ram_size,
> -				  &glink->rx_pipe, &glink->tx_pipe);
> +				  rx_pipe, tx_pipe);
>   	if (ret)
>   		return ret;
>   
> -	writel(0, glink->tx_pipe.head);
> -	writel(0, glink->rx_pipe.tail);
> +	/* Pipe specific accessors */
> +	rx_pipe->native.avail = glink_rpm_rx_avail;
> +	rx_pipe->native.peak = glink_rpm_rx_peak;
> +	rx_pipe->native.advance = glink_rpm_rx_advance;
> +	tx_pipe->native.avail = glink_rpm_tx_avail;
> +	tx_pipe->native.write = glink_rpm_tx_write;
> +
> +	glink->tx_pipe = &tx_pipe->native;
> +	glink->rx_pipe = &rx_pipe->native;
> +
> +	writel(0, tx_pipe->head);
> +	writel(0, rx_pipe->tail);
>   
>   	irq = platform_get_irq(pdev, 0);
>   	ret = devm_request_irq(dev, irq,