From patchwork Fri Jun 20 21:43:05 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ashwin Chaugule X-Patchwork-Id: 32299 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-ve0-f198.google.com (mail-ve0-f198.google.com [209.85.128.198]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 38185203C2 for ; Fri, 20 Jun 2014 21:43:08 +0000 (UTC) Received: by mail-ve0-f198.google.com with SMTP id db11sf14349264veb.1 for ; Fri, 20 Jun 2014 14:43:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:in-reply-to:references :date:message-id:subject:from:to:cc:x-original-sender :x-original-authentication-results:precedence:mailing-list:list-id :list-post:list-help:list-archive:list-unsubscribe:content-type; bh=pTkc70vlkTcB75BWaaSeWAt972Pe8UXPvcxM9UWF9eU=; b=XC6uRgZdK9M0xpyQTJ4OiTWSPUWChZlzkSb53ECzTUYZ08W8g9ZRZtlRwfNm0Ds2rC OQ+UOz8uUQQ5dhBO2I+02texPZKtAlf16QNzTmjWRQeLX05ClEcjLz2ndPorIFBg2fSB fXtu85SLVxAPkCbSUbkjOARKFfdysgc+O61L9JULQiZt25v6qhPHwbCJRL6M6IzAFcnm MZDORD02tlVhEqcT2uSjpgkB+dqLuT24sSVR5XF8rm3nukuKaJeBE1Ir+34KrHxQXQBi zOjWYEZgwwOXODAPRJ8Z7jK0k5+6vSbMpQBNtwGc0zoE4+fAUVsEvAOnL/gC5DHmkYFY wCfQ== X-Gm-Message-State: ALoCoQkgKwsWBNdNByE/2017zWegj23HgulOQ9s9Lab/gqYFdYlkdfK+1N8/cTj76sfAVLIaUH2Z X-Received: by 10.236.128.112 with SMTP id e76mr2223797yhi.38.1403300587893; Fri, 20 Jun 2014 14:43:07 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.98.11 with SMTP id n11ls291558qge.11.gmail; Fri, 20 Jun 2014 14:43:07 -0700 (PDT) X-Received: by 10.52.252.226 with SMTP id zv2mr4265217vdc.19.1403300587750; Fri, 20 Jun 2014 14:43:07 -0700 (PDT) Received: from mail-ve0-f178.google.com (mail-ve0-f178.google.com [209.85.128.178]) by mx.google.com with ESMTPS id 6si4822905vct.68.2014.06.20.14.43.07 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 20 Jun 2014 14:43:07 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.128.178 as permitted sender) client-ip=209.85.128.178; Received: by mail-ve0-f178.google.com with SMTP id oy12so4161602veb.9 for ; Fri, 20 Jun 2014 14:43:07 -0700 (PDT) X-Received: by 10.58.219.166 with SMTP id pp6mr5282888vec.1.1403300587649; Fri, 20 Jun 2014 14:43:07 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patches@linaro.org Received: by 10.221.37.5 with SMTP id tc5csp52125vcb; Fri, 20 Jun 2014 14:43:06 -0700 (PDT) X-Received: by 10.180.149.240 with SMTP id ud16mr7161277wib.3.1403300586095; Fri, 20 Jun 2014 14:43:06 -0700 (PDT) Received: from mail-wi0-f180.google.com (mail-wi0-f180.google.com [209.85.212.180]) by mx.google.com with ESMTPS id m8si8337456wjb.164.2014.06.20.14.43.05 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 20 Jun 2014 14:43:06 -0700 (PDT) Received-SPF: pass (google.com: domain of ashwin.chaugule@linaro.org designates 209.85.212.180 as permitted sender) client-ip=209.85.212.180; Received: by mail-wi0-f180.google.com with SMTP id hi2so1452654wib.1 for ; Fri, 20 Jun 2014 14:43:05 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.194.71.12 with SMTP id q12mr7533270wju.5.1403300585399; Fri, 20 Jun 2014 14:43:05 -0700 (PDT) Received: by 10.217.67.6 with HTTP; Fri, 20 Jun 2014 14:43:05 -0700 (PDT) In-Reply-To: <4600246.SKVsl5kA9F@wuerfel> References: <1402591692-7736-1-git-send-email-ashwin.chaugule@linaro.org> <5207109.Ir8j6FZuiX@wuerfel> <4600246.SKVsl5kA9F@wuerfel> Date: Fri, 20 Jun 2014 17:43:05 -0400 Message-ID: Subject: Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI From: Ashwin Chaugule To: Arnd Bergmann Cc: "linux-arm-kernel@lists.infradead.org" , Patch Tracking , Jassi Brar , rjw@rjwysocki.net, "linaro-acpi@lists.linaro.org" , linux-acpi@vger.kernel.org, Mark Brown X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: ashwin.chaugule@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.128.178 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Precedence: list Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org List-ID: X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , Hello, On 20 June 2014 16:49, Arnd Bergmann wrote: > On Friday 20 June 2014 15:29:18 Ashwin Chaugule wrote: >> >> On 20 June 2014 15:08, Arnd Bergmann wrote: >> > On Friday 20 June 2014 14:55:16 Ashwin Chaugule wrote: >> >> So, in order to get an mbox->dev for ACPI platforms, we'd need an >> >> entry in the DSDT table. That seems rather pointless, since the DSDT >> >> is reserved for devices and is supposed to be OS agnostic. Since the >> >> mailbox controller itself is not really a "device" with a resource >> >> descriptor, I dont see the point in adding a dummy DSDT entry for the >> >> sake of getting this `struct device`. Also, I'm told adding new >> >> entries to this table requires registering a unique 4 character >> >> identifier and approval from some committees. If there are other ways >> >> to get this structure I'd like to hear about it. >> >> >> >> The other alternative would be to piggy back on the ACPI CPU detection >> >> code, which looks for the ACPI0007 device node in the DSDT and use >> >> that as the mbox controller device. This node is already registered >> >> and is an established method to detect CPUs. But I'm not sure what >> >> happens when CPUs are hotplugged off, we surely dont want mailbox >> >> clients such as PCC to break. >> > >> > The main question here is whether you expect having to support multiple >> > mailbox devices in an ACPI system. If you think there is never more than >> > one, you wouldn't need a DSDT entry, but if you can end up in a situation >> > where another device needs to specify which mailbox it is using, then >> > you need that entry anyway. >> >> At this point, I dont see the need for multiple mailbox devices. But >> I'm not seeing why we'd need a DSDT entry only if there are more than >> one mailbox devices? I'd obviously prefer not having a DSDT entry for >> this, and the patch I posted is the only way I could see to keep DT >> and ACPI mbox supported at runtime without DSDT involved. Please let >> me know if there are better ways. > > It's mostly a matter of consistency: We can have multiple interrupt > controllers, pin controllers, clock controllers, dma engines, etc, > and in the DT case we use references to the nodes wherever we have > other devices referring to a mailbox name. > > I believe Intel's embedded chips are moving in the same direction > with their ACPI support. If the ACPI spec gains support for mailbox > devices, locking them into having only a single device may be > a problem later for them. > > Note that "device" here doesn't have to mean a platform device that > is instantiated from DSDT, it can be any mailbox provider that is > registered in an arbitrary way, as long as you have a method to map > back from the (consumer-device, name-string) tuple back to the > (provider, channel) tuple. I have read your patch again now and noticed > that you actually tried to do this, but unfortunately you got it > wrong by requiring the consumer to fill out the name of the provider > in the request. You can't do that, because it's not generic enough > to support devices that can be reused, and it means that drivers > using the API are never portable between DT and ACPI. You have to > get rid of the "ctrl_name" field in the mbox_client structure and > change the lookup to be based only on cd->dev and cl->chan_name, > using whatever tables you have available in ACPI. I think you looked at the previous version of the patch. I'm attaching the latest version here again FWIW. In this version, I removed the "ctrl_name" field and rely on the cl->chan_name to provide the info as described in Jassi' original patch. linux/mailbox_client.h 18 * struct mbox_client - User of a mailbox 19 * @dev: The client device 20 * @chan_name: The "controller:channel" this client wants Instead of dev, I added a name string to the mbox controller structure. So now the client gets its channel by requesting "controller:channel" where controller should match with mbox->name and channel becomes an index into mbox->chans[]. Cheers, Ashwin >From 6fe6e583f0b23b08643a4a85545a9a5338b9b1a0 Mon Sep 17 00:00:00 2001 From: Ashwin Chaugule Date: Wed, 11 Jun 2014 16:09:35 -0400 Subject: [PATCH] Mailbox: Add support for ACPI The current mailbox framework only supports DT based bindings. Add another mechanism for mailbox clients to register with mailbox controllers and request for specific mailbox channels. This enables usage of the mailbox framework on kernels with ACPI support. Signed-off-by: Ashwin Chaugule --- drivers/mailbox/mailbox.c | 177 ++++++++++++++++++++++++++----------- include/linux/mailbox_client.h | 2 +- include/linux/mailbox_controller.h | 1 + 3 files changed, 129 insertions(+), 51 deletions(-) diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c index d83d12c..e2704f5 100644 --- a/drivers/mailbox/mailbox.c +++ b/drivers/mailbox/mailbox.c @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -273,100 +274,166 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) } EXPORT_SYMBOL_GPL(mbox_send_message); -/** - * mbox_request_channel - Request a mailbox channel. - * @cl: Identity of the client requesting the channel. - * - * The Client specifies its requirements and capabilities while asking for - * a mailbox channel by name. It can't be called from atomic context. - * The channel is exclusively allocated and can't be used by another - * client before the owner calls mbox_free_channel. - * After assignment, any packet received on this channel will be - * handed over to the client via the 'rx_callback'. - * - * Return: Pointer to the channel assigned to the client if successful. - * ERR_PTR for request failure. - */ -struct mbox_chan *mbox_request_channel(struct mbox_client *cl) +static int init_channel(struct mbox_chan *chan, + struct mbox_client *cl) +{ + unsigned long flags; + int ret; + + if (!chan) { + pr_err("No mailbox channel specified\n"); + return -EINVAL; + } + + spin_lock_irqsave(&chan->lock, flags); + chan->msg_free = 0; + chan->msg_count = 0; + chan->active_req = NULL; + chan->cl = cl; + + if (!cl->tx_tout) /* wait for ever */ + cl->tx_tout = msecs_to_jiffies(3600000); + else + cl->tx_tout = msecs_to_jiffies(cl->tx_tout); + if (chan->txdone_method == TXDONE_BY_POLL + && cl->knows_txdone) + chan->txdone_method |= TXDONE_BY_ACK; + spin_unlock_irqrestore(&chan->lock, flags); + + ret = chan->mbox->ops->startup(chan); + if (ret) { + pr_err("Unable to startup the chan\n"); + mbox_free_channel(chan); + chan = ERR_PTR(ret); + } + + return ret; +} + +static int get_acpi_mbox_chan(struct mbox_client *cl, + struct mbox_chan **chan) +{ + struct mbox_controller *mbox; + int chan_id, ret, len; + char *chan_ptr; + + if (!cl->chan_name) + return -ENODEV; + + list_for_each_entry(mbox, &mbox_cons, node) { + if (mbox->name) { + /* + * The cl->chan_name has the format => controller:channel + * as described in mailbox_client.h + */ + + len = strlen(mbox->name); + chan_ptr = cl->chan_name + len + 1; + + ret = kstrtou32(cl->chan_name, 0, &chan_id); + + if (ret < 0) { + pr_err("Err while parsing mailbox:%s channel idx\n", + mbox->name); + continue; + } + + if (!strncmp(cl->chan_name, mbox->name, len)) { + *chan = &mbox->chans[chan_id]; + return init_channel(*chan, cl); + } + } + } + + return -ENODEV; +} + +static int get_of_mbox_chan(struct mbox_client *cl, + struct mbox_chan **chan) { struct device *dev = cl->dev; struct mbox_controller *mbox; struct of_phandle_args spec; - struct mbox_chan *chan; - unsigned long flags; int count, i, ret; if (!dev || !dev->of_node) { pr_err("%s: No owner device node\n", __func__); - return ERR_PTR(-ENODEV); + return -ENODEV; } count = of_property_count_strings(dev->of_node, "mbox-names"); if (count < 0) { pr_err("%s: mbox-names property of node '%s' missing\n", __func__, dev->of_node->full_name); - return ERR_PTR(-ENODEV); + return -ENODEV; } - - mutex_lock(&con_mutex); - - ret = -ENODEV; for (i = 0; i < count; i++) { const char *s; if (of_property_read_string_index(dev->of_node, - "mbox-names", i, &s)) + "mbox-names", i, &s)) continue; if (strcmp(cl->chan_name, s)) continue; if (of_parse_phandle_with_args(dev->of_node, - "mbox", "#mbox-cells", i, &spec)) + "mbox", "#mbox-cells", i, &spec)) continue; - chan = NULL; list_for_each_entry(mbox, &mbox_cons, node) if (mbox->dev->of_node == spec.np) { - chan = mbox->of_xlate(mbox, &spec); + *chan = mbox->of_xlate(mbox, &spec); break; } of_node_put(spec.np); - if (!chan) + if (!(*chan)) continue; ret = -EBUSY; - if (!chan->cl && try_module_get(mbox->dev->driver->owner)) + if (!(*chan)->cl && acpi_disabled && + try_module_get(mbox->dev->driver->owner)) break; } if (i == count) { mutex_unlock(&con_mutex); - return ERR_PTR(ret); + return ret; } - spin_lock_irqsave(&chan->lock, flags); - chan->msg_free = 0; - chan->msg_count = 0; - chan->active_req = NULL; - chan->cl = cl; - if (!cl->tx_tout) /* wait for ever */ - cl->tx_tout = msecs_to_jiffies(3600000); + return init_channel(*chan, cl); +} + +/** + * mbox_request_channel - Request a mailbox channel. + * @cl: Identity of the client requesting the channel. + * + * The Client specifies its requirements and capabilities while asking for + * a mailbox channel by name. It can't be called from atomic context. + * The channel is exclusively allocated and can't be used by another + * client before the owner calls mbox_free_channel. + * After assignment, any packet received on this channel will be + * handed over to the client via the 'rx_callback'. + * + * Return: Pointer to the channel assigned to the client if successful. + * ERR_PTR for request failure. + */ +struct mbox_chan *mbox_request_channel(struct mbox_client *cl) +{ + struct mbox_chan *chan = NULL; + int ret; + + mutex_lock(&con_mutex); + + if (acpi_disabled) + ret = get_of_mbox_chan(cl, &chan); else - cl->tx_tout = msecs_to_jiffies(cl->tx_tout); - if (chan->txdone_method == TXDONE_BY_POLL - && cl->knows_txdone) - chan->txdone_method |= TXDONE_BY_ACK; - spin_unlock_irqrestore(&chan->lock, flags); + ret = get_acpi_mbox_chan(cl, &chan); - ret = chan->mbox->ops->startup(chan); - if (ret) { - pr_err("Unable to startup the chan\n"); - mbox_free_channel(chan); - chan = ERR_PTR(ret); - } + if (ret) + pr_err("No mailbox channels found\n"); mutex_unlock(&con_mutex); return chan; @@ -394,7 +461,9 @@ void mbox_free_channel(struct mbox_chan *chan) if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) chan->txdone_method = TXDONE_BY_POLL; - module_put(chan->mbox->dev->driver->owner); + if (chan->mbox->dev) + module_put(chan->mbox->dev->driver->owner); + spin_unlock_irqrestore(&chan->lock, flags); } EXPORT_SYMBOL_GPL(mbox_free_channel); @@ -422,7 +491,15 @@ int mbox_controller_register(struct mbox_controller *mbox) int i, txdone; /* Sanity check */ - if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans) + if (!mbox || !mbox->ops || !mbox->num_chans) + return -EINVAL; + + /* + * For ACPI platforms, to get mbox->dev, we'd need to + * have a fake meaningless entry in the DSDT for the + * mailbox controller. + */ + if (acpi_disabled && !mbox->dev) return -EINVAL; if (mbox->txdone_irq) diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h index bbac2d2..716fbae 100644 --- a/include/linux/mailbox_client.h +++ b/include/linux/mailbox_client.h @@ -29,7 +29,7 @@ struct mbox_chan; */ struct mbox_client { struct device *dev; - const char *chan_name; + char *chan_name; void (*rx_callback)(struct mbox_client *cl, void *mssg); void (*tx_done)(struct mbox_client *cl, void *mssg, enum mbox_result r); bool tx_block; diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h index cf81e80..06476ef 100644 --- a/include/linux/mailbox_controller.h +++ b/include/linux/mailbox_controller.h @@ -78,6 +78,7 @@ struct mbox_controller { unsigned period; /* Hook to add to the global controller list */ struct list_head node; + char *name; } __aligned(32); /* -- 1.8.3.2