On Wed, Jul 29, 2015 at 2:08 PM, Sudeep Holla <sudeep.ho...@arm.com> wrote: > On 29/07/15 09:05, Jassi Brar wrote: > >> >>> +static int scpi_probe(struct platform_device *pdev) >>> +{ >>> + int count, idx, ret; >>> + struct resource res; >>> + struct scpi_chan *scpi_chan; >>> + struct device *dev = &pdev->dev; >>> + struct device_node *np = dev->of_node; >>> + >>> + scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL); >>> + if (!scpi_info) >>> + return -ENOMEM; >>> + >>> + count = of_count_phandle_with_args(np, "mboxes", "#mbox-cells"); >>> + if (count < 0) { >>> + dev_err(dev, "no mboxes property in '%s'\n", >>> np->full_name); >>> + return -ENODEV; >>> + } >>> + >>> + scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), >>> GFP_KERNEL); >>> + if (!scpi_chan) >>> + return -ENOMEM; >>> + >>> + for (idx = 0; idx < count; idx++) { >>> + resource_size_t size; >>> + struct scpi_chan *pchan = scpi_chan + idx; >>> + struct mbox_client *cl = &pchan->cl; >>> + struct device_node *shmem = of_parse_phandle(np, "shmem", >>> idx); >>> + >>> + if (of_address_to_resource(shmem, 0, &res)) { >>> + dev_err(dev, "failed to get SCPI payload mem >>> resource\n"); >>> + ret = -EINVAL; >>> + goto err; >>> + } >>> + >>> + size = resource_size(&res); >>> + pchan->rx_payload = devm_ioremap(dev, res.start, size); >>> + if (!pchan->rx_payload) { >>> + dev_err(dev, "failed to ioremap SCPI payload\n"); >>> + ret = -EADDRNOTAVAIL; >>> + goto err; >>> + } >>> + pchan->tx_payload = pchan->rx_payload + (size >> 1); >>> + >>> + cl->dev = dev; >>> + cl->rx_callback = scpi_handle_remote_msg; >>> + cl->tx_prepare = scpi_tx_prepare; >>> + cl->tx_block = true; >>> + cl->tx_tout = 50; >>> + cl->knows_txdone = false; /* controller can ack */ >>> >> This is the cause of your problems that you think should be solved by >> using hrtimer. >> > > Ah sorry, it's stupid mistake on my part while writing the comment. It > should have been controller can't ack, fixed locally now thanks for > pointing it out. > No, I am talking about code, not the comment.
>> Controller may or may not (like MHU) set txdone_irq. However every >> scpi command (struct scpi_ops members) is replied to as a response >> packet reporting success or failure. > > > No that's not true, I have already mentioned that couple of times in the > other thread. It's just wrong comment here which went unnoticed from > day#1, sorry for that. > >> So the client should set 'knows_txdone' to be true unless it is told >> the controller on that platform supports txdone_irq (what you call >> 'ack'). >> > I got the concept but SCP can't ack via protocol, protocol has no such > provision and it sets flags in MHU status register. > You either don't get the concept of TXDONE_BY_ACK or deliberately overlook my point. Assuming the former, let me explain. When a client receives a response, it can be sure that the request has already been read by the remote. If the protocol specifies every request has some response, the client should assert 'knows_txdone' and call mbox_client_txdone() upon receiving a reply packet. So I said, cl->knows_txdone = false; is the root of problems you report. If you fix that, the performance should be even better than using hrtimer. -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/