* [PATCH] arm64: topology: Avoid checking numa mask for scheduler MC selection
From: Morten Rasmussen @ 2018-06-06 15:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <be5f7066-0b44-ff11-37e9-87512df2294f@arm.com>
On Wed, Jun 06, 2018 at 09:53:39AM -0500, Jeremy Linton wrote:
> On 06/06/2018 09:44 AM, Morten Rasmussen wrote:
> >On Tue, Jun 05, 2018 at 02:08:37PM -0500, Jeremy Linton wrote:
> >>The numa mask subset check has problems if !CONFIG_NUMA, over hotplug
> >>operations or during early boot. Lets disable the NUMA siblings checks
> >>for the time being, as NUMA in socket machines have LLC's that will
> >>assure that the scheduler topology isn't "borken".
> >
> >Could we add an explanation why the numa node mask check is needed in
> >the first place. >
> >IIUC, we have the check in case the LLC is shared across numa nodes as
> >this would cause core_siblings > cpumask_of_node() which breaks the
> >scheduler topology.
>
> Yes, that sounds like a good idea, my comments probably assume that the
> reader has been part of these conversations.
>
> >
> >While sharing LLC across numa nodes seems quite unusual, I think it is
> >allowed by ACPI. Those systems might already be broken before, so might
> >not change anything. It is just worth noting why the check should be
> >added back later.
>
> Right, there isn't anything in ACPI that dictates a system topology
> restriction like this. Given that other architectures have built machines
> with large directory caches that span numa nodes the check was a safety
> measure.
Agreed, it seems that another architecture has recently merged support
for that: 1340ccfa9a9a
^ permalink raw reply
* [PATCH v4 5/7] interconnect: qcom: Add msm8916 interconnect provider driver
From: Georgi Djakov @ 2018-06-06 15:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAE=gft4LT_JSGytuiSx5uwzfeGc0DK140+D4c_J8FVUHioHdcg@mail.gmail.com>
Hi Evan,
On 12.05.18 ?. 0:29, Evan Green wrote:
> Hi Georgi,
>
> On Fri, Mar 9, 2018 at 1:11 PM Georgi Djakov <georgi.djakov@linaro.org>
> wrote:
>
>> Add driver for the Qualcomm interconnect buses found in msm8916 based
>> platforms.
>
>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> ---
>> drivers/interconnect/Kconfig | 5 +
>> drivers/interconnect/Makefile | 1 +
>> drivers/interconnect/qcom/Kconfig | 11 +
>> drivers/interconnect/qcom/Makefile | 2 +
>> drivers/interconnect/qcom/msm8916.c | 482
> ++++++++++++++++++++++++++++++++++++
>> include/linux/interconnect/qcom.h | 350 ++++++++++++++++++++++++++
>> 6 files changed, 851 insertions(+)
>> create mode 100644 drivers/interconnect/qcom/Kconfig
>> create mode 100644 drivers/interconnect/qcom/msm8916.c
>> create mode 100644 include/linux/interconnect/qcom.h
> ...
>> diff --git a/drivers/interconnect/qcom/msm8916.c
> b/drivers/interconnect/qcom/msm8916.c
>> new file mode 100644
>> index 000000000000..d5b54f8261c8
>> --- /dev/null
>> +++ b/drivers/interconnect/qcom/msm8916.c
> ...
>> +static int qnoc_probe(struct platform_device *pdev)
>> +{
>> + const struct qcom_icc_desc *desc;
>> + struct qcom_icc_node **qnodes;
>> + struct qcom_icc_provider *qp;
>> + struct resource *res;
>> + struct icc_provider *provider;
>> + size_t num_nodes, i;
>> + int ret;
>> +
>> + /* wait for RPM */
>> + if (!qcom_icc_rpm_smd_available())
>> + return -EPROBE_DEFER;
>> +
>> + desc = of_device_get_match_data(&pdev->dev);
>> + if (!desc)
>> + return -EINVAL;
>> +
>> + qnodes = desc->nodes;
>> + num_nodes = desc->num_nodes;
>> +
>> + qp = devm_kzalloc(&pdev->dev, sizeof(*qp), GFP_KERNEL);
>> + if (!qp)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + qp->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(qp->base))
>> + return PTR_ERR(qp->base);
>> +
>> + qp->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
>> + if (IS_ERR(qp->bus_clk))
>> + return PTR_ERR(qp->bus_clk);
>> +
>> + qp->bus_a_clk = devm_clk_get(&pdev->dev, "bus_a_clk");
>> + if (IS_ERR(qp->bus_a_clk))
>> + return PTR_ERR(qp->bus_a_clk);
>> +
>> + provider = &qp->provider;
>> + provider->dev = &pdev->dev;
>> + provider->set = &qcom_icc_set;
>> + INIT_LIST_HEAD(&provider->nodes);
>> + provider->data = qp;
>> +
>> + ret = icc_add_provider(provider);
>> + if (ret) {
>> + dev_err(&pdev->dev, "error adding interconnect
> provider\n");
>> + return ret;
>> + }
>> +
>> + for (i = 0; i < num_nodes; i++) {
>> + struct icc_node *node;
>> + int ret;
>> + size_t j;
>> +
>> + node = icc_node_create(qnodes[i]->id);
>> + if (IS_ERR(node)) {
>> + ret = PTR_ERR(node);
>> + goto err;
>> + }
>> +
>> + node->name = qnodes[i]->name;
>> + node->data = qnodes[i];
>> + icc_node_add(node, provider);
>> +
>> + dev_dbg(&pdev->dev, "registered node %p %s %d\n", node,
>> + qnodes[i]->name, node->id);
>> +
>> + /* populate links */
>> + for (j = 0; j < qnodes[i]->num_links; j++)
>> + if (qnodes[i]->links[j])
>> + icc_link_create(node,
> qnodes[i]->links[j]);
>> +
>> + ret = qcom_icc_init(node);
>> + if (ret)
>> + dev_err(&pdev->dev, "%s init error (%d)\n",
> node->name,
>> + ret);
>
> Don't you want to call qcom_icc_init before icc_link_create? As soon as
> icc_link_create is called, the node is connected to the graph, and
> qcom_icc_set can be called.
>
I agree.
>> + }
>> +
>> + platform_set_drvdata(pdev, provider);
>> +
>> + return ret;
>> +err:
>> + icc_del_provider(provider);
>> + return ret;
>> +}
>> +
>> +static int qnoc_remove(struct platform_device *pdev)
>> +{
>> + struct icc_provider *provider = platform_get_drvdata(pdev);
>> +
>> + icc_del_provider(provider);
>
> Presumably in the framework nodes and links ought to get cleaned up
> somewhere too, right? As it is now, the devm code frees provider when this
> device is removed, even though provider is still very connected in the
> graph via the nodes and links.
Sorry, this part is incomplete. Will implement the rest of the cleanup code.
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id qnoc_of_match[] = {
>> + { .compatible = "qcom,msm8916-pnoc", .data = &msm8916_pnoc },
>> + { .compatible = "qcom,msm8916-snoc", .data = &msm8916_snoc },
>> + { .compatible = "qcom,msm8916-bimc", .data = &msm8916_bimc },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, qnoc_of_match);
>> +
>> +static struct platform_driver qnoc_driver = {
>> + .probe = qnoc_probe,
>> + .remove = qnoc_remove,
>> + .driver = {
>> + .name = "qnoc-msm8916",
>> + .of_match_table = qnoc_of_match,
>> + },
>> +};
>> +module_platform_driver(qnoc_driver);
>> +MODULE_AUTHOR("Georgi Djakov <georgi.djakov@linaro.org>");
>> +MODULE_DESCRIPTION("Qualcomm msm8916 NoC driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/interconnect/qcom.h
> b/include/linux/interconnect/qcom.h
>> new file mode 100644
>> index 000000000000..2cd378d2f575
>> --- /dev/null
>> +++ b/include/linux/interconnect/qcom.h
>> @@ -0,0 +1,350 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Qualcomm interconnect IDs
>> + *
>> + * Copyright (c) 2018, Linaro Ltd.
>> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
>> + */
>> +
>> +#ifndef __QCOM_INTERCONNECT_IDS_H
>> +#define __QCOM_INTERCONNECT_IDS_H
>> +
>> +#define FAB_BIMC 0
>> +#define FAB_SYS_NOC 1024
>> +#define FAB_MMSS_NOC 2048
>> +#define FAB_OCMEM_NOC 3072
>> +#define FAB_PERIPH_NOC 4096
>> +#define FAB_CONFIG_NOC 5120
>> +#define FAB_OCMEM_VNOC 6144
>
> Looks like you're still following that downstream convention of NoCs being
> divisible by 1024, masters in 1-512, and slaves in 512-1023, then I don't
> know about the 10000s. This was documented somewhere downstream, can you
> add that documentation somewhere in here?
Yes, i am currently following the existing IDs. The 10000s are the nodes
used for the interconnects between the NoCs. I don't think it's worth
documenting this, as the plan is to switch to arbitrary numbers.
Thanks,
Georgi
^ permalink raw reply
* [PATCH v4 4/7] interconnect: qcom: Add RPM communication
From: Georgi Djakov @ 2018-06-06 15:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAE=gft6HTU76M8AqzvN-VeDWE6c8Bgk+TimHHLMfLztWYhCSYQ@mail.gmail.com>
Hi Evan,
On 12.05.18 ?. 0:30, Evan Green wrote:
> On Fri, Mar 9, 2018 at 1:11 PM Georgi Djakov <georgi.djakov@linaro.org>
> wrote:
>
>> On some Qualcomm SoCs, there is a remote processor, which controls some of
>> the Network-On-Chip interconnect resources. Other CPUs express their needs
>> by communicating with this processor. Add a driver to handle comminication
>> with this remote processor.
>
>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> ---
>> .../devicetree/bindings/interconnect/qcom-smd.txt | 31 ++++++++
>> drivers/interconnect/qcom/Makefile | 1 +
>> drivers/interconnect/qcom/smd-rpm.c | 90
> ++++++++++++++++++++++
>> drivers/interconnect/qcom/smd-rpm.h | 15 ++++
>> 4 files changed, 137 insertions(+)
>> create mode 100644
> Documentation/devicetree/bindings/interconnect/qcom-smd.txt
>> create mode 100644 drivers/interconnect/qcom/Makefile
>> create mode 100644 drivers/interconnect/qcom/smd-rpm.c
>> create mode 100644 drivers/interconnect/qcom/smd-rpm.h
>
>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom-smd.txt
> b/Documentation/devicetree/bindings/interconnect/qcom-smd.txt
>> new file mode 100644
>> index 000000000000..14e83ed7019b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interconnect/qcom-smd.txt
>> @@ -0,0 +1,31 @@
>> +Qualcomm SMD-RPM interconnect driver binding
>> +------------------------------------------------
>> +The RPM is a dedicated hardware engine for managing the shared
>> +SoC resources in order to keep the lowest power profile. It
>> +communicates with other hardware subsystems via shared memory
>> +and accepts requests for various resources.
>
> You never say what RPM or SMD stands for. RPM is Resource Power Manager,
> right? But I'm not in the know about SMD. Can you define these somewhere?
>
Sure, it's the Shared Memory Driver back-end used for communicating with
RPM. Now added this explanation into the docs.
Thanks,
Georgi
^ permalink raw reply
* [PATCH v4 1/7] interconnect: Add generic on-chip interconnect API
From: Georgi Djakov @ 2018-06-06 14:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAE=gft76VpU1mXgNPK9-XTj6WoLHj-_SxAGuV-bUSNYVpfSn6w@mail.gmail.com>
Hi Evan,
Thanks for the detailed review!
On 12.05.18 ?. 0:30, Evan Green wrote:
> Hi Georgi,
>
> On Fri, Mar 9, 2018 at 1:12 PM Georgi Djakov <georgi.djakov@linaro.org>
> wrote:
>
>> This patch introduce a new API to get requirements and configure the
>> interconnect buses across the entire chipset to fit with the current
>> demand.
>
>> The API is using a consumer/provider-based model, where the providers are
>> the interconnect buses and the consumers could be various drivers.
>> The consumers request interconnect resources (path) between endpoints and
>> set the desired constraints on this data flow path. The providers receive
>> requests from consumers and aggregate these requests for all master-slave
>> pairs on that path. Then the providers configure each participating in the
>> topology node according to the requested data flow path, physical links
> and
>> constraints. The topology could be complicated and multi-tiered and is SoC
>> specific.
>
>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> ---
>> Documentation/interconnect/interconnect.rst | 96 ++++++
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 1 +
>> drivers/interconnect/Kconfig | 10 +
>> drivers/interconnect/Makefile | 1 +
>> drivers/interconnect/core.c | 489
> ++++++++++++++++++++++++++++
>> include/linux/interconnect-provider.h | 109 +++++++
>> include/linux/interconnect.h | 40 +++
>> 8 files changed, 748 insertions(+)
>> create mode 100644 Documentation/interconnect/interconnect.rst
>> create mode 100644 drivers/interconnect/Kconfig
>> create mode 100644 drivers/interconnect/Makefile
>> create mode 100644 drivers/interconnect/core.c
>> create mode 100644 include/linux/interconnect-provider.h
>> create mode 100644 include/linux/interconnect.h
>
> ...
>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
[..]
>> +static struct icc_path *path_allocate(struct icc_node *node, ssize_t
> num_nodes)
>> +{
>
> So node is really the destination, correct? Then we use ->reverse to walk
> backwards num_nodes steps towards the source. It might increase readability
> to call the parameter dest, then assign that to a local called node for
> traversal.
Yes, exactly. Will change the name of the parameter to dst.
>> + struct icc_path *path;
>> + size_t i;
>> +
>> + path = kzalloc(sizeof(*path) + num_nodes * sizeof(*path->reqs),
>> + GFP_KERNEL);
>> + if (!path)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + path->num_nodes = num_nodes;
>> +
>> + for (i = 0; i < num_nodes; i++) {
>> + hlist_add_head(&path->reqs[i].req_node, &node->req_list);
>> +
>> + path->reqs[i].node = node;
>> + /* reference to previous node was saved during path
> traversal */
>> + node = node->reverse;
>> + }
>> +
>> + return path;
>> +}
>> +
>> +static struct icc_path *path_find(struct icc_node *src, struct icc_node
> *dst)
>> +{
>> + struct icc_node *node = NULL;
>> + struct list_head traverse_list;
>> + struct list_head edge_list;
>> + struct list_head tmp_list;
>> + size_t i, number = 0;
>> + bool found = false;
>> +
>> + INIT_LIST_HEAD(&traverse_list);
>> + INIT_LIST_HEAD(&edge_list);
>> + INIT_LIST_HEAD(&tmp_list);
>
> tmp_list is really the list of nodes you've already visited and need to
> remember to reset is_traversed for. Maybe calling this done_list or
> visited_list would be more descriptive.
Yes, visited_list sounds better. Will change it.
>> +
>> + list_add_tail(&src->search_list, &traverse_list);
>
> For added paranoia, you could set src->reverse to NULL so that somebody
> elsewhere who had a bug in their back-traversal would fall off the end,
> rather than into some previous scrapped path.
Ok.
>> +
>> + do {
>> + list_for_each_entry(node, &traverse_list, search_list) {
>> + if (node == dst) {
>> + found = true;
>> + list_add(&node->search_list, &tmp_list);
>> + break;
>> + }
>> + for (i = 0; i < node->num_links; i++) {
>> + struct icc_node *tmp = node->links[i];
>> +
>> + if (!tmp)
>> + return ERR_PTR(-ENOENT);
>
> You just bail out here, but never clean up the nodes is_traversed, which
> will ruin later searches. Maybe a goto towards the common cleanup path?
Agree. Good catch.
>> +
>> + if (tmp->is_traversed)
>> + continue;
>> +
>> + tmp->is_traversed = true;
>> + tmp->reverse = node;
>> + list_add_tail(&tmp->search_list,
> &edge_list);
>> + }
>> + }
>> + if (found)
>> + break;
>> +
>> + list_splice_init(&traverse_list, &tmp_list);
>> + list_splice_init(&edge_list, &traverse_list);
>> +
>> + /* count the number of nodes */
>> + number++;
>
> Depth might be a better name for this, since this really counts the hops
> away from the source, rather than the number of nodes you've processed.
Ok, will change it to depth.
>> +
>> + } while (!list_empty(&traverse_list));
>> +
>> + /* reset the traversed state */
>> + list_for_each_entry(node, &tmp_list, search_list)
>> + node->is_traversed = false;
>> +
>> + if (found)
>> + return path_allocate(dst, number);
>> +
>> + return ERR_PTR(-EPROBE_DEFER);
>> +}
>> +
>> +static int path_init(struct icc_path *path)
>> +{
>> + struct icc_node *node;
>> + size_t i;
>> +
>> + for (i = 0; i < path->num_nodes; i++) {
>> + node = path->reqs[i].node;
>> +
>> + mutex_lock(&node->provider->lock);
>> + node->provider->users++;
>> + mutex_unlock(&node->provider->lock);
>> + }
>> +
>> + return 0;
>> +}
>
> This function cannot fail, nor do you check its return value, so you should
> change the return type to void.
Ok.
>
> I'm wondering if the locking here is a little sketchy. I was in the process
> of typing a suggestion that you call path_init from within path_find, since
> it seemed weird to have this gray zone of a path without its reference
> counts, when I noticed the locks.
>
> I can't evaluate fully, since the implementation seems to be missing
> icc_node_remove, a critical function in terms of evaluating the locks. You
> have an icc_del_provider, but its warning of if (provider->users) is pretty
> weak, since without node removal provider->users could easily be
> incremented after the provider lock is released. It also leaks all of its
> nodes, since there's no way to remove them.
>
> Here's my suggestion as far as the locking goes:
> * To add or remove links/nodes from the graph, you're going to need to hold
> a global lock to avoid colliding with traversals. You've already got an
> icc_path_mutex, so that would work.
> * icc_link_create needs to hold the global icc_path_mutex, since it's
> messing with arrays and connections used in path traversal, and doesn't
> need to hold the provider lock, since it's not changing anything there.
> * The presumably upcoming icc_link_destroy, or its parent icc_node_destroy,
> also needs to hold the global lock. node_destroy may also need the provider
> lock in symmetry with icc_node_add.
> * Provider->users will be protected under the global icc_path_mutex, rather
> than the provider lock. Then move path_init into path_find, or inline it
> into path_allocate.
> * Once you do that, provider->lock is now only protecting its node list.
> For now, it's probably more efficient to roll the protection of
> provider->nodes under the global lock as well, and remove the lock from the
> provider altogether. If you anticipate other functions in the future that
> will require a lock in the provider, then it might make sense to keep the
> lock, or maybe just add it later with that new functionality.
Ok, i will try to simplify this. Using one global lock would be ok for
now. Will implement the complete set for functions for remove links and
nodes from the topology.
>> +
>> +static void node_aggregate(struct icc_node *node)
>> +{
>> + struct icc_req *r;
>> + u32 agg_avg = 0;
>> + u32 agg_peak = 0;
>> +
>> + hlist_for_each_entry(r, &node->req_list, req_node) {
>> + /* sum(averages) and max(peaks) */
>> + agg_avg += r->avg_bw;
>> + agg_peak = max(agg_peak, r->peak_bw);
>> + }
>> +
>> + node->avg_bw = agg_avg;
>> + node->peak_bw = agg_peak;
>> +}
>> +
>> +static void provider_aggregate(struct icc_provider *provider, u32
> *avg_bw,
>> + u32 *peak_bw)
>> +{
>> + struct icc_node *n;
>> + u32 agg_avg = 0;
>> + u32 agg_peak = 0;
>> +
>> + /* aggregate for the interconnect provider */
>> + list_for_each_entry(n, &provider->nodes, node_list) {
>> + /* sum the average and max the peak */
>> + agg_avg += n->avg_bw;
>> + agg_peak = max(agg_peak, n->peak_bw);
>> + }
>> +
>> + *avg_bw = agg_avg;
>> + *peak_bw = agg_peak;
>> +}
>> +
>> +static int constraints_apply(struct icc_path *path)
>> +{
>
> Nit: maybe name it apply_constraints, since constraints_apply sounds like a
> query (do the constraints apply?).
Ok.
>> + struct icc_node *next, *prev = NULL;
>> + int i;
>> +
>> + for (i = 0; i < path->num_nodes; i++, prev = next) {
>> + struct icc_provider *provider;
>> + u32 avg_bw = 0;
>> + u32 peak_bw = 0;
>> + int ret;
>> +
>> + next = path->reqs[i].node;
>> + /*
>> + * Both endpoints should be valid master-slave pairs of
> the
>> + * same interconnect provider that will be configured.
>> + */
>> + if (!next || !prev)
>> + continue;
>> +
>> + if (next->provider != prev->provider)
>> + continue;
>
> next should never be null, right? So you could shorten this to if (!prev ||
> (next->provider != prev->provider))
Yes, right. Will do so.
>> +
>> + provider = next->provider;
>> + mutex_lock(&provider->lock);
>> +
>> + /* aggregate requests for the provider */
>> + provider_aggregate(provider, &avg_bw, &peak_bw);
>> +
>> + if (provider->set) {
>> + /* set the constraints */
>> + ret = provider->set(prev, next, avg_bw, peak_bw);
>> + }
>> +
>> + mutex_unlock(&provider->lock);
>> +
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * icc_set() - set constraints on an interconnect path between two
> endpoints
>> + * @path: reference to the path returned by icc_get()
>> + * @avg_bw: average bandwidth in kbps
>> + * @peak_bw: peak bandwidth in kbps
>> + *
>> + * This function is used by an interconnect consumer to express its own
> needs
>> + * in term of bandwidth and QoS for a previously requested path between
> two
>
> "in terms of" rather than "in term of", and not really QoS yet, right?
>
>> + * endpoints. The requests are aggregated and each node is updated
> accordingly.
>> + *
>> + * Returns 0 on success, or an approproate error code otherwise.
>
> appropriate
>
Ok, thanks!
>> + */
>> +int icc_set(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>> +{
>> + struct icc_node *node;
>> + size_t i;
>> + int ret;
>> +
>> + if (!path)
>> + return 0;
>
> Can we ditch this null check? My understanding is it's generally preferred
> to skip this if it's only there to avoid developer errors.
Ok.
>> +
>> + for (i = 0; i < path->num_nodes; i++) {
>> + node = path->reqs[i].node;
>> +
>> + mutex_lock(&icc_path_mutex);
>> +
>> + /* update the consumer request for this path */
>> + path->reqs[i].avg_bw = avg_bw;
>> + path->reqs[i].peak_bw = peak_bw;
>> +
>> + /* aggregate requests for this node */
>> + node_aggregate(node);
>> +
>> + mutex_unlock(&icc_path_mutex);
>> + }
>> +
>> + ret = constraints_apply(path);
>> + if (ret)
>> + pr_err("interconnect: error applying constraints (%d)",
> ret);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(icc_set);
>> +
>> +/**
>> + * icc_get() - return a handle for path between two endpoints
>> + * @src_id: source device port id
>> + * @dst_id: destination device port id
>> + *
>> + * This function will search for a path between two endpoints and return
> an
>> + * icc_path handle on success. Use icc_put() to release
>> + * constraints when the they are not needed anymore.
>> + *
>> + * Return: icc_path pointer on success, or ERR_PTR() on error
>> + */
>> +struct icc_path *icc_get(const int src_id, const int dst_id)
>> +{
>> + struct icc_node *src, *dst;
>> + struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
>> +
>> + src = node_find(src_id);
>> + if (!src)
>> + goto out;
>> +
>> + dst = node_find(dst_id);
>> + if (!dst)
>> + goto out;
>> +
>> + mutex_lock(&icc_path_mutex);
>> + path = path_find(src, dst);
>> + mutex_unlock(&icc_path_mutex);
>> + if (IS_ERR(path))
>> + goto out;
>> +
>> + path_init(path);
>> +
>> +out:
>> + return path;
>> +}
>> +EXPORT_SYMBOL_GPL(icc_get);
>> +
>> +/**
>> + * icc_put() - release the reference to the icc_path
>> + * @path: interconnect path
>> + *
>> + * Use this function to release the constraints on a path when the path
> is
>> + * no longer needed. The constraints will be re-aggregated.
>> + */
>> +void icc_put(struct icc_path *path)
>> +{
>> + struct icc_node *node;
>> + size_t i;
>> + int ret;
>> +
>> + if (!path || WARN_ON_ONCE(IS_ERR(path)))
>> + return;
>> +
>> + ret = icc_set(path, 0, 0);
>> + if (ret)
>> + pr_err("%s: error (%d)\n", __func__, ret);
>> +
>> + for (i = 0; i < path->num_nodes; i++) {
>> + node = path->reqs[i].node;
>> + hlist_del(&path->reqs[i].req_node);
>> +
>> + mutex_lock(&node->provider->lock);
>> + node->provider->users--;
>> + mutex_unlock(&node->provider->lock);
>> + }
>> +
>> + kfree(path);
>> +}
>> +EXPORT_SYMBOL_GPL(icc_put);
>> +
>> +/**
>> + * icc_node_create() - create a node
>> + * @id: node id
>> + *
>> + * Return: icc_node pointer on success, or ERR_PTR() on error
>> + */
>> +struct icc_node *icc_node_create(int id)
>> +{
>> + struct icc_node *node;
>> +
>> + /* check if node already exists */
>> + node = node_find(id);
>> + if (node)
>> + return node;
>
> This is probably going to do more harm than good once icc_node_delete comes
> in, since it almost certainly indicates a programmer error or ID collision,
> and will likely result in a double free. We should probably fail with
> EEXIST instead.
In the current approach we create the nodes one by one, and the linked
nodes are created when they are referenced. The other way around would
be to create first all the nodes and then populate the links to avoid
the "chicken and egg" problem.
>> +
>> + node = kzalloc(sizeof(*node), GFP_KERNEL);
>> + if (!node)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + id = idr_alloc(&icc_idr, node, id, id + 1, GFP_KERNEL);
>> + if (WARN(id < 0, "couldn't get idr"))
>> + return ERR_PTR(id);
>> +
>> + node->id = id;
>> +
>> + return node;
>> +}
>> +EXPORT_SYMBOL_GPL(icc_node_create);
>> +
>> +/**
>> + * icc_link_create() - create a link between two nodes
>> + * @src_id: source node id
>> + * @dst_id: destination node id
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +int icc_link_create(struct icc_node *node, const int dst_id)
>> +{
>> + struct icc_node *dst;
>> + struct icc_node **new;
>> + int ret = 0;
>> +
>> + if (IS_ERR_OR_NULL(node))
>> + return PTR_ERR(node);
>
> Remove this.
Ok.
>> +
>> + mutex_lock(&node->provider->lock);
>> +
>> + dst = node_find(dst_id);
>> + if (!dst)
>> + dst = icc_node_create(dst_id);
>
> icc_node_create can fail, you should fail here if it does.
Ok.
>> +
>> + new = krealloc(node->links,
>> + (node->num_links + 1) * sizeof(*node->links),
>> + GFP_KERNEL);
>> + if (!new) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + node->links = new;
>> + node->links[node->num_links++] = dst;
>> +
>> +out:
>> + mutex_unlock(&node->provider->lock);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(icc_link_create);
>> +
>> +/**
>> + * icc_add_node() - add an interconnect node to interconnect provider
>> + * @node: pointer to the interconnect node
>> + * @provider: pointer to the interconnect provider
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +int icc_node_add(struct icc_node *node, struct icc_provider *provider)
>> +{
>> + if (WARN_ON(!node))
>> + return -EINVAL;
>> +
>> + if (WARN_ON(!provider))
>> + return -EINVAL;
>
> Remove these.
Ok.
>> +
>> + node->provider = provider;
>> +
>> + mutex_lock(&provider->lock);
>> + list_add_tail(&node->node_list, &provider->nodes);
>> + mutex_unlock(&provider->lock);
>> +
>> + return 0;
>> +}
>
> icc_node_add should be exported, right? I see it being used in msm8916.c.
> You should make sure that "make allmodconfig" still builds with your
> changes.
Yes. Agree.
>
> I think you should add a safety check in icc_link_create to ensure that the
> node has a provider before adding any links. If some consumer made a
> mistake and added links before adding the node to the provider, path
> traversal would use the uninitialized or NULL provider pointer. I was
> thinking about this while noticing that you assign node->provider before
> acquiring the lock.
Ok.
>> +
>> +/**
>> + * icc_add_provider() - add a new interconnect provider
>> + * @icc_provider: the interconnect provider that will be added into
> topology
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +int icc_add_provider(struct icc_provider *provider)
>> +{
>> + if (WARN_ON(!provider))
>> + return -EINVAL;
>> +
>
> Remove this one. The one below is okay.
Ok.
>> + if (WARN_ON(!provider->set))
>> + return -EINVAL;
>> +
>> + mutex_init(&provider->lock);
>> + INIT_LIST_HEAD(&provider->nodes);
>> +
>> + mutex_lock(&icc_provider_list_mutex);
>> + list_add(&provider->provider_list, &icc_provider_list);
>> + mutex_unlock(&icc_provider_list_mutex);
>> +
>> + dev_dbg(provider->dev, "interconnect provider added to
> topology\n");
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(icc_add_provider);
>> +
>> +/**
>> + * icc_del_provider() - delete previously added interconnect provider
>> + * @icc_provider: the interconnect provider that will be removed from
> topology
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +int icc_del_provider(struct icc_provider *provider)
>> +{
>> + mutex_lock(&provider->lock);
>> + if (provider->users) {
>> + pr_warn("interconnect provider still has %d users\n",
>> + provider->users);
>> + }
>> + mutex_unlock(&provider->lock);
>> +
>> + mutex_lock(&icc_provider_list_mutex);
>> + list_del(&provider->provider_list);
>> + mutex_unlock(&icc_provider_list_mutex);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(icc_del_provider);
>> +
>> +MODULE_AUTHOR("Georgi Djakov <georgi.djakov@linaro.org");
>> +MODULE_DESCRIPTION("Interconnect Driver Core");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/interconnect-provider.h
> b/include/linux/interconnect-provider.h
>> new file mode 100644
>> index 000000000000..779b5b5b1306
>> --- /dev/null
>> +++ b/include/linux/interconnect-provider.h
>> @@ -0,0 +1,109 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (c) 2018, Linaro Ltd.
>> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
>> + */
>> +
>> +#ifndef _LINUX_INTERCONNECT_PROVIDER_H
>> +#define _LINUX_INTERCONNECT_PROVIDER_H
>> +
>> +#include <linux/interconnect.h>
>> +
>> +struct icc_node;
>> +
>> +/**
>> + * struct icc_provider - interconnect provider (controller) entity that
> might
>> + * provide multiple interconnect controls
>> + *
>> + * @provider_list: list of the registered interconnect providers
>> + * @nodes: internal list of the interconnect provider nodes
>> + * @set: pointer to device specific set operation function
>> + * @dev: the device this interconnect provider belongs to
>> + * @lock: lock to provide consistency during aggregation/update of
> constraints
>> + * @users: count of active users
>> + * @data: pointer to private data
>> + */
>> +struct icc_provider {
>> + struct list_head provider_list;
>> + struct list_head nodes;
>> + int (*set)(struct icc_node *src, struct icc_node *dst,
>> + u32 avg_bw, u32 peak_bw);
>> + struct device *dev;
>> + struct mutex lock;
>> + int users;
>> + void *data;
>> +};
>> +
>> +/**
>> + * struct icc_node - entity that is part of the interconnect topology
>> + *
>> + * @id: platform specific node id
>> + * @name: node name used in debugfs
>> + * @links: a list of targets where we can go next when traversing
>> + * @num_links: number of links to other interconnect nodes
>> + * @provider: points to the interconnect provider of this node
>> + * @node_list: list of interconnect nodes associated with @provider
>> + * @search_list: list used when walking the nodes graph
>> + * @reverse: pointer to previous node when walking the nodes graph
>> + * @is_traversed: flag that is used when walking the nodes graph
>> + * @req_list: a list of QoS constraint requests associated with this node
>> + * @avg_bw: aggregated value of average bandwidth
>> + * @peak_bw: aggregated value of peak bandwidth
>> + * @data: pointer to private data
>> + */
>> +struct icc_node {
>> + int id;
>
> Why int here? Are you expecting negative numbers? Maybe u32 instead? Or
> even better, maybe a typedef u32 icc_id? Ooh yeah, that way we know when
> parameters and such are passed around that they refer to this.
It's an int, just to be aligned with the idr API. u32 would also work.
>> + const char *name;
>> + struct icc_node **links;
>> + size_t num_links;
>> +
>> + struct icc_provider *provider;
>> + struct list_head node_list;
>
> So the difference between node_list and links is that node_list nodes live
> inside this node, whereas links point at other peers?
>
> Oh no, I get it now after reading the .c file: node_list is the list entry
> in the parent provider's "nodes" list. The comment description could be
> clearer about that.
Ok. Will improve the wording.
Thanks,
Georgi
^ permalink raw reply
* [PATCH] arm64: topology: Avoid checking numa mask for scheduler MC selection
From: Jeremy Linton @ 2018-06-06 14:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180606144444.GB8461@e105550-lin.cambridge.arm.com>
On 06/06/2018 09:44 AM, Morten Rasmussen wrote:
> On Tue, Jun 05, 2018 at 02:08:37PM -0500, Jeremy Linton wrote:
>> The numa mask subset check has problems if !CONFIG_NUMA, over hotplug
>> operations or during early boot. Lets disable the NUMA siblings checks
>> for the time being, as NUMA in socket machines have LLC's that will
>> assure that the scheduler topology isn't "borken".
>
> Could we add an explanation why the numa node mask check is needed in
> the first place. >
> IIUC, we have the check in case the LLC is shared across numa nodes as
> this would cause core_siblings > cpumask_of_node() which breaks the
> scheduler topology.
Yes, that sounds like a good idea, my comments probably assume that the
reader has been part of these conversations.
>
> While sharing LLC across numa nodes seems quite unusual, I think it is
> allowed by ACPI. Those systems might already be broken before, so might
> not change anything. It is just worth noting why the check should be
> added back later.
Right, there isn't anything in ACPI that dictates a system topology
restriction like this. Given that other architectures have built
machines with large directory caches that span numa nodes the check was
a safety measure.
^ permalink raw reply
* [PATCH] arm64: topology: Avoid checking numa mask for scheduler MC selection
From: Morten Rasmussen @ 2018-06-06 14:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180605190837.493505-1-jeremy.linton@arm.com>
On Tue, Jun 05, 2018 at 02:08:37PM -0500, Jeremy Linton wrote:
> The numa mask subset check has problems if !CONFIG_NUMA, over hotplug
> operations or during early boot. Lets disable the NUMA siblings checks
> for the time being, as NUMA in socket machines have LLC's that will
> assure that the scheduler topology isn't "borken".
Could we add an explanation why the numa node mask check is needed in
the first place?
IIUC, we have the check in case the LLC is shared across numa nodes as
this would cause core_siblings > cpumask_of_node() which breaks the
scheduler topology.
While sharing LLC across numa nodes seems quite unusual, I think it is
allowed by ACPI. Those systems might already be broken before, so might
not change anything. It is just worth noting why the check should be
added back later.
Morten
^ permalink raw reply
* [PATCH] arm64: topology: Avoid checking numa mask for scheduler MC selection
From: Jeremy Linton @ 2018-06-06 14:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3c8a541d-78a1-389a-112c-494f7c1a543d@arm.com>
On 06/06/2018 05:05 AM, Sudeep Holla wrote:
>
>
> On 05/06/18 20:08, Jeremy Linton wrote:
>> The numa mask subset check has problems if !CONFIG_NUMA, over hotplug
>> operations or during early boot. Lets disable the NUMA siblings checks
>> for the time being, as NUMA in socket machines have LLC's that will
>> assure that the scheduler topology isn't "borken".
>>
>
> ^ broken ? (not sure if usage of borken is intentional :))
Well that is what the scheduler says when it doesn't like the topology.
>
>> Futher, as a defensive mechanism during hotplug, lets assure that the
>
> ^ Further
Sure.
>
>> LLC siblings are also masked.
>>
>
> Also add the symptoms of the issue we say as Geert suggested me.
> Something like:
> " This often leads to system hang or crash during CPU hotplug and system
> suspend operation. This is mostly observed on HMP systems where the
> CPU compute capacities are different and ends up in different scheduler
> domains. Since cpumask_of_node is returned instead core_sibling, the
> scheduler is confused with incorrect cpumasks(e.g. one CPU in two
> different sched domains at the same time) on CPU hotplug."
>
> You can add Reported-by: Geert... ?
ok.
>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>> arch/arm64/kernel/topology.c | 11 ++++-------
>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index 7415c166281f..f845a8617812 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -215,13 +215,8 @@ EXPORT_SYMBOL_GPL(cpu_topology);
>>
>> const struct cpumask *cpu_coregroup_mask(int cpu)
>> {
>> - const cpumask_t *core_mask = cpumask_of_node(cpu_to_node(cpu));
>> + const cpumask_t *core_mask = &cpu_topology[cpu].core_sibling;
>>
>> - /* Find the smaller of NUMA, core or LLC siblings */
>> - if (cpumask_subset(&cpu_topology[cpu].core_sibling, core_mask)) {
>> - /* not numa in package, lets use the package siblings */
>> - core_mask = &cpu_topology[cpu].core_sibling;
>> - }
>> if (cpu_topology[cpu].llc_id != -1) {
>> if (cpumask_subset(&cpu_topology[cpu].llc_siblings, core_mask))
>> core_mask = &cpu_topology[cpu].llc_siblings;
>> @@ -239,8 +234,10 @@ static void update_siblings_masks(unsigned int cpuid)
>> for_each_possible_cpu(cpu) {
>> cpu_topo = &cpu_topology[cpu];
>>
>> - if (cpuid_topo->llc_id == cpu_topo->llc_id)
>> + if (cpuid_topo->llc_id == cpu_topo->llc_id) {
>> cpumask_set_cpu(cpu, &cpuid_topo->llc_siblings);
>> + cpumask_set_cpu(cpuid, &cpu_topo->llc_siblings);
>> + }
>>
>> if (cpuid_topo->package_id != cpu_topo->package_id)
>> continue;
>>
>
> Looks good to me for now. I might need to tweek it a bit when I add the
> support to update topology on hotplug. But that's for latter. For now,
>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>
^ permalink raw reply
* [PATCH v5 2/4] kernel hacking: new config NO_AUTO_INLINE to disable compiler auto-inline optimizations
From: Johan Hovold @ 2018-06-06 14:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180606095714.1d3c2def@vmware.local.home>
On Wed, Jun 06, 2018 at 09:57:14AM -0400, Steven Rostedt wrote:
> On Wed, 6 Jun 2018 05:21:55 +0800
> kbuild test robot <lkp@intel.com> wrote:
>
> > Hi Changbin,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on linus/master]
> > [also build test WARNING on v4.17 next-20180605]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> >
> > url: https://github.com/0day-ci/linux/commits/changbin-du-intel-com/kernel-hacking-GCC-optimization-for-better-debug-experience-Og/20180606-001415
> > config: ia64-allmodconfig (attached as .config)
> > compiler: ia64-linux-gcc (GCC) 8.1.0
> > reproduce:
> > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # save the attached .config to linux build tree
> > make.cross ARCH=ia64
> >
> > All warnings (new ones prefixed by >>):
> >
> > drivers//staging/greybus/fw-management.c: In function 'fw_mgmt_load_and_validate_operation':
> > >> drivers//staging/greybus/fw-management.c:153:2: warning: 'strncpy' specified bound 10 equals destination size [-Wstringop-truncation]
> > strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers//staging/greybus/fw-management.c: In function 'fw_mgmt_backend_fw_update_operation':
> > drivers//staging/greybus/fw-management.c:304:2: warning: 'strncpy' specified bound 10 equals destination size [-Wstringop-truncation]
> > strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > --
> > drivers/auxdisplay/panel.c: In function 'panel_bind_key':
> > >> drivers/auxdisplay/panel.c:1509:2: warning: 'strncpy' specified bound 12 equals destination size [-Wstringop-truncation]
> > strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str));
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/auxdisplay/panel.c:1510:2: warning: 'strncpy' specified bound 12 equals destination size [-Wstringop-truncation]
> > strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str));
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Nice! This patch actually caused bugs in other areas of the code to be
> caught by the build system.
>
> The patch is not wrong. The code that has these warnings are.
Looks like the greybus code above is working as intended by checking for
unterminated string after the strncpy, even if this does now triggers
the truncation warning.
drivers/auxdisplay/panel.c looks broken, though.
> > vim +/strncpy +153 drivers//staging/greybus/fw-management.c
> >
> > 013e6653 Viresh Kumar 2016-05-14 138
> > 013e6653 Viresh Kumar 2016-05-14 139 static int fw_mgmt_load_and_validate_operation(struct fw_mgmt *fw_mgmt,
> > 013e6653 Viresh Kumar 2016-05-14 140 u8 load_method, const char *tag)
> > 013e6653 Viresh Kumar 2016-05-14 141 {
> > 013e6653 Viresh Kumar 2016-05-14 142 struct gb_fw_mgmt_load_and_validate_fw_request request;
> > 013e6653 Viresh Kumar 2016-05-14 143 int ret;
> > 013e6653 Viresh Kumar 2016-05-14 144
> > 013e6653 Viresh Kumar 2016-05-14 145 if (load_method != GB_FW_LOAD_METHOD_UNIPRO &&
> > 013e6653 Viresh Kumar 2016-05-14 146 load_method != GB_FW_LOAD_METHOD_INTERNAL) {
> > 013e6653 Viresh Kumar 2016-05-14 147 dev_err(fw_mgmt->parent,
> > 013e6653 Viresh Kumar 2016-05-14 148 "invalid load-method (%d)\n", load_method);
> > 013e6653 Viresh Kumar 2016-05-14 149 return -EINVAL;
> > 013e6653 Viresh Kumar 2016-05-14 150 }
> > 013e6653 Viresh Kumar 2016-05-14 151
> > 013e6653 Viresh Kumar 2016-05-14 152 request.load_method = load_method;
> > b2abeaa1 Viresh Kumar 2016-08-11 @153 strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> > 013e6653 Viresh Kumar 2016-05-14 154
> > 013e6653 Viresh Kumar 2016-05-14 155 /*
> > 013e6653 Viresh Kumar 2016-05-14 156 * The firmware-tag should be NULL terminated, otherwise throw error and
> > 013e6653 Viresh Kumar 2016-05-14 157 * fail.
> > 013e6653 Viresh Kumar 2016-05-14 158 */
> > b2abeaa1 Viresh Kumar 2016-08-11 159 if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> > 013e6653 Viresh Kumar 2016-05-14 160 dev_err(fw_mgmt->parent, "load-and-validate: firmware-tag is not NULL terminated\n");
> > 013e6653 Viresh Kumar 2016-05-14 161 return -EINVAL;
> > 013e6653 Viresh Kumar 2016-05-14 162 }
Viresh, do you want to work around the warning somehow?
Thanks,
Johan
^ permalink raw reply
* [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
From: Sascha Hauer @ 2018-06-06 14:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <AM0PR04MB421169E44830A17D8A52D3F4806A0@AM0PR04MB4211.eurprd04.prod.outlook.com>
On Thu, May 24, 2018 at 08:56:15AM +0000, A.s. Dong wrote:
> Hi Sascha,
>
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Thursday, May 3, 2018 8:41 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; dl-linux-imx
> > <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
> > <fabio.estevam@nxp.com>; shawnguo at kernel.org
> > Subject: Re: [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
> >
> > On Thu, May 03, 2018 at 12:29:40PM +0000, A.s. Dong wrote:
> > > > -----Original Message-----
> > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > Sent: Thursday, May 3, 2018 7:06 PM
> > > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com;
> > > > dl-linux-imx <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio
> > > > Estevam <fabio.estevam@nxp.com>; shawnguo at kernel.org
> > > > Subject: Re: [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
> > > >
> > > > On Wed, May 02, 2018 at 06:40:03PM +0000, A.s. Dong wrote:
> > > > > > -----Original Message-----
> > > > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > > > Sent: Wednesday, May 2, 2018 6:04 PM
> > > > > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > > > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com;
> > > > > > dl-linux-imx <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio
> > > > > > Estevam <fabio.estevam@nxp.com>; shawnguo at kernel.org
> > > > > > Subject: Re: [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
> > > > > >
> > > > > > On Sat, Apr 28, 2018 at 02:46:16AM +0800, Dong Aisheng wrote:
> > > > > > > +/* Initialization of the MU code. */ int __init
> > > > > > > +imx8_scu_init(void) {
> > > > > > > + struct device_node *np, *mu_np;
> > > > > > > + struct resource mu_res;
> > > > > > > + sc_err_t sci_err;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + if (!of_machine_is_compatible("fsl,imx8qxp"))
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + np = of_find_compatible_node(NULL, NULL, "nxp,imx8qxp-
> > scu");
> > > > > > > + if (!np)
> > > > > > > + return -ENODEV;
> > > > > > > +
> > > > > > > + mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > > > > > > + if (WARN_ON(!mu_np))
> > > > > > > + return -EINVAL;
> > > > > > > +
> > > > > > > + ret = of_address_to_resource(mu_np, 0, &mu_res);
> > > > > > > + if (WARN_ON(ret))
> > > > > > > + return -EINVAL;
> > > > > > > +
> > > > > > > + /* we use mu physical address as IPC communication channel
> > ID */
> > > > > > > + sci_err = sc_ipc_open(&scu_ipc_handle,
> > (sc_ipc_id_t)(mu_res.start));
> > > > > > > + if (sci_err != SC_ERR_NONE) {
> > > > > > > + pr_err("Cannot open MU channel to SCU\n");
> > > > > > > + return sci_err;
> > > > > > > + };
> > > > > >
> > > > > > Introducing private error codes always has the risk of just
> > > > > > forwarding them as errno codes as done here.
> > > > > >
> > > > >
> > > > > Yes, you're right.
> > > > > We probably could do the same as scpi_to_linux_errno in
> > > > > drivers/firmware/arm_scpi.c.
> > > > > However, may can't fix the issue permanently.
> > > > >
> > > > > > > +
> > > > > > > + of_node_put(mu_np);
> > > > > > > +
> > > > > > > + pr_info("NXP i.MX SCU Initialized (scu_ipc %u)\n",
> > > > > > > +scu_ipc_handle);
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +early_initcall(imx8_scu_init);
> > > > > >
> > > > > > This code shows that the separate 'scu' device node shouldn't exist.
> > > > > > It is only used as a stepping stone to find the 'mu' node.
> > > > > > Instead of providing a proper driver for the 'mu' node the scu
> > > > > > code registers it
> > > > with its physical address.
> > > > > > I don't think it makes sense to separate mu and scu code in this way.
> > > > > >
> > > > >
> > > > > I agree that may not look quite well.
> > > > > It mainly because we want to use the MU physical address as a MU ID.
> > > > > (can't use virtual address as sc_ipc_id_t is u32 defined by SCU firmware.
> > > > >
> > > > > If you have any better suggestion please let me know.
> > > >
> > > > What I'm suggesting is a single node:
> > > >
> > > > scu at 5d1b0000 {
> > > > compatible = "fsl,imx8qxp-scu";
> > > > reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > > > };
> > > >
> > > > Attach your code to this one, without any further separation between
> > > > mu and scu code.
> > > >
> > >
> > > A bit confused. You're suggesting a single node here without mu or
> > > mailbox node phandle in it? Then how SCU use MU?
> >
> > ioremap the address and mu_receive_msg()/mu_send_msg() on it, just like
> > you do already.
> >
> > >
> > > > We discussed this internally and came to the conclusion that the SCU
> > > > is not a generic user of a MU. The MU is designed to work together
> > > > with a piece of SRAM to form a mailbox system. Instead of working as
> > > > designed the SCU morses the messages through the doorbell (what the
> > > > MU really is). For anything that uses the MU in the way it is
> > > > designed I would suggest using the mailbox interface from
> > > > drivers/mailbox/ along with the binding from
> > Documentation/devicetree/bindings/mailbox/mailbox.txt.
> > > >
> > > > In the way I suggest there is no need for any MU ID.
> > > >
> > >
> > > So you're suggesting switch to use mailbox instead of private MU
> > > library function calls?
> > > Something like:
> > > scu {
> > > compatible = "nxp,imx8qxp-scu", "simple-bus";
> > > mboxes = <&mailbox 0>;
> > > }
> > > Then IPC is implemented based on mailbox?
> > >
> > > As I replied Oleksij Rempel in another mail in this thread, we've
> > > tried mailbox but got performance regression issue and need more time
> > > to investigate whether it's really quite suitable for i.MX SCU
> > > firmware as SCU handling message quite fast in micro seconds. (Mailbox
> > > polling method has much more latency than current MU sample polling we
> > > used) and we want to avoid the SCU firmware API changes.
> >
> > I am not suggesting to do mailboxing (using shared memory) for the SCU.
> > I am also not suggesting any API update for the SCU communication.
> > I am just mentioning that doing mailboxing is the way the MU was originally
> > designed for. Looking at the reference manual I see many MUs on the i.MX8.
> > I guess most of them are for communication between the different cores on
> > the system. For these it's probably worth writing some generic MU driver.
> > The way the MU is used for communication with the SCU though is so special
> > that it's not worth writing a generic driver, so just integrate the MU access
> > functions in the SCU code.
> >
>
> I understand it.
>
> One problem of the way you suggested may be that:
> If we doing like below, we may lose flexibility to change the MU used
> for SCU firmware communication.
> scu at 5d1b0000 {
> compatible = "fsl,imx8qxp-scu";
> reg = <0x0 0x5d1b0000 0x0 0x10000>;
> };
>
> And current design is that the system supports multiple MU channels used
> by various users at the same time, e.g. SCU, Power Domain, Clock and others.
> User can flexibly change it under their nodes: And each MU channel is protected
> by their private lock and not affect each others.
>
> e.g.
> scu {
> compatible = "nxp,imx8qxp-scu", "simple-bus";
> fsl,mu = <&lsio_mu0>;
>
> clk: clk {
> compatible = "fsl,imx8qxp-clk";
> #clock-cells = <1>;
> };
>
> iomuxc: iomuxc {
> compatible = "fsl,imx8qxp-iomuxc";
> fsl,mu = <&lsio_mu3>;
> };
>
> imx8qx-pm {
> #address-cells = <1>;
> #size-cells = <0>;
> fsl,mu = <&lsio_mu4>;
> .............
> }
>
> The default code only uses MU0 which is used by SCU.
>
> The change may affect this design. Any ideas?
Sorry for the delay.
You can add the child nodes to the mu nodes they should use:
scu1 {
compatible = "nxp,imx8qxp-scu";
reg = <0x0 0x5d1b0000 0x0 0x10000>;
clk: clk {
compatible = "fsl,imx8qxp-clk";
#clock-cells = <1>;
};
...
};
scu2 {
compatible = "nxp,imx8qxp-scu";
reg = <0x0 someothermu 0x0 0x10000>;
iomuxc: iomuxc {
compatible = "fsl,imx8qxp-iomuxc";
};
...
};
So instead of adding all possible children to a single mu and phandle to
other mu's, just add the right children to each mu.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [PATCH v5 2/4] kernel hacking: new config NO_AUTO_INLINE to disable compiler auto-inline optimizations
From: Steven Rostedt @ 2018-06-06 14:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201806060444.hdHcKOBy%fengguang.wu@intel.com>
On Wed, 6 Jun 2018 05:34:29 +0800
kbuild test robot <lkp@intel.com> wrote:
> Hi Changbin,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on linus/master]
> [also build test WARNING on v4.17 next-20180605]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/changbin-du-intel-com/kernel-hacking-GCC-optimization-for-better-debug-experience-Og/20180606-001415
> config: sparc64-allyesconfig (attached as .config)
> compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=sparc64
>
> All warnings (new ones prefixed by >>):
>
> >> WARNING: vmlinux.o(.text.unlikely+0x1fc): Section mismatch in reference from the function init_tick_ops() to the function .init.text:get_tick_patch()
> The function init_tick_ops() references
> the function __init get_tick_patch().
> This is often because init_tick_ops lacks a __init
> annotation or the annotation of get_tick_patch is wrong.
And again this patch uncovered a bug someplace else.
-- Steve
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* [PATCH v5 2/4] kernel hacking: new config NO_AUTO_INLINE to disable compiler auto-inline optimizations
From: Steven Rostedt @ 2018-06-06 13:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201806060501.btF3aJMZ%fengguang.wu@intel.com>
On Wed, 6 Jun 2018 05:21:55 +0800
kbuild test robot <lkp@intel.com> wrote:
> Hi Changbin,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on linus/master]
> [also build test WARNING on v4.17 next-20180605]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/changbin-du-intel-com/kernel-hacking-GCC-optimization-for-better-debug-experience-Og/20180606-001415
> config: ia64-allmodconfig (attached as .config)
> compiler: ia64-linux-gcc (GCC) 8.1.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=ia64
>
> All warnings (new ones prefixed by >>):
>
> drivers//staging/greybus/fw-management.c: In function 'fw_mgmt_load_and_validate_operation':
> >> drivers//staging/greybus/fw-management.c:153:2: warning: 'strncpy' specified bound 10 equals destination size [-Wstringop-truncation]
> strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers//staging/greybus/fw-management.c: In function 'fw_mgmt_backend_fw_update_operation':
> drivers//staging/greybus/fw-management.c:304:2: warning: 'strncpy' specified bound 10 equals destination size [-Wstringop-truncation]
> strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> --
> drivers/auxdisplay/panel.c: In function 'panel_bind_key':
> >> drivers/auxdisplay/panel.c:1509:2: warning: 'strncpy' specified bound 12 equals destination size [-Wstringop-truncation]
> strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str));
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/auxdisplay/panel.c:1510:2: warning: 'strncpy' specified bound 12 equals destination size [-Wstringop-truncation]
> strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str));
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Nice! This patch actually caused bugs in other areas of the code to be
caught by the build system.
The patch is not wrong. The code that has these warnings are.
-- Steve
>
> vim +/strncpy +153 drivers//staging/greybus/fw-management.c
>
> 013e6653 Viresh Kumar 2016-05-14 138
> 013e6653 Viresh Kumar 2016-05-14 139 static int fw_mgmt_load_and_validate_operation(struct fw_mgmt *fw_mgmt,
> 013e6653 Viresh Kumar 2016-05-14 140 u8 load_method, const char *tag)
> 013e6653 Viresh Kumar 2016-05-14 141 {
> 013e6653 Viresh Kumar 2016-05-14 142 struct gb_fw_mgmt_load_and_validate_fw_request request;
> 013e6653 Viresh Kumar 2016-05-14 143 int ret;
> 013e6653 Viresh Kumar 2016-05-14 144
> 013e6653 Viresh Kumar 2016-05-14 145 if (load_method != GB_FW_LOAD_METHOD_UNIPRO &&
> 013e6653 Viresh Kumar 2016-05-14 146 load_method != GB_FW_LOAD_METHOD_INTERNAL) {
> 013e6653 Viresh Kumar 2016-05-14 147 dev_err(fw_mgmt->parent,
> 013e6653 Viresh Kumar 2016-05-14 148 "invalid load-method (%d)\n", load_method);
> 013e6653 Viresh Kumar 2016-05-14 149 return -EINVAL;
> 013e6653 Viresh Kumar 2016-05-14 150 }
> 013e6653 Viresh Kumar 2016-05-14 151
> 013e6653 Viresh Kumar 2016-05-14 152 request.load_method = load_method;
> b2abeaa1 Viresh Kumar 2016-08-11 @153 strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> 013e6653 Viresh Kumar 2016-05-14 154
> 013e6653 Viresh Kumar 2016-05-14 155 /*
> 013e6653 Viresh Kumar 2016-05-14 156 * The firmware-tag should be NULL terminated, otherwise throw error and
> 013e6653 Viresh Kumar 2016-05-14 157 * fail.
> 013e6653 Viresh Kumar 2016-05-14 158 */
> b2abeaa1 Viresh Kumar 2016-08-11 159 if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> 013e6653 Viresh Kumar 2016-05-14 160 dev_err(fw_mgmt->parent, "load-and-validate: firmware-tag is not NULL terminated\n");
> 013e6653 Viresh Kumar 2016-05-14 161 return -EINVAL;
> 013e6653 Viresh Kumar 2016-05-14 162 }
> 013e6653 Viresh Kumar 2016-05-14 163
> 013e6653 Viresh Kumar 2016-05-14 164 /* Allocate ids from 1 to 255 (u8-max), 0 is an invalid id */
> 013e6653 Viresh Kumar 2016-05-14 165 ret = ida_simple_get(&fw_mgmt->id_map, 1, 256, GFP_KERNEL);
> 013e6653 Viresh Kumar 2016-05-14 166 if (ret < 0) {
> 013e6653 Viresh Kumar 2016-05-14 167 dev_err(fw_mgmt->parent, "failed to allocate request id (%d)\n",
> 013e6653 Viresh Kumar 2016-05-14 168 ret);
> 013e6653 Viresh Kumar 2016-05-14 169 return ret;
> 013e6653 Viresh Kumar 2016-05-14 170 }
> 013e6653 Viresh Kumar 2016-05-14 171
> 013e6653 Viresh Kumar 2016-05-14 172 fw_mgmt->intf_fw_request_id = ret;
> 04f0e6eb Viresh Kumar 2016-05-14 173 fw_mgmt->intf_fw_loaded = false;
> 013e6653 Viresh Kumar 2016-05-14 174 request.request_id = ret;
> 013e6653 Viresh Kumar 2016-05-14 175
> 013e6653 Viresh Kumar 2016-05-14 176 ret = gb_operation_sync(fw_mgmt->connection,
> 013e6653 Viresh Kumar 2016-05-14 177 GB_FW_MGMT_TYPE_LOAD_AND_VALIDATE_FW, &request,
> 013e6653 Viresh Kumar 2016-05-14 178 sizeof(request), NULL, 0);
> 013e6653 Viresh Kumar 2016-05-14 179 if (ret) {
> 013e6653 Viresh Kumar 2016-05-14 180 ida_simple_remove(&fw_mgmt->id_map,
> 013e6653 Viresh Kumar 2016-05-14 181 fw_mgmt->intf_fw_request_id);
> 013e6653 Viresh Kumar 2016-05-14 182 fw_mgmt->intf_fw_request_id = 0;
> 013e6653 Viresh Kumar 2016-05-14 183 dev_err(fw_mgmt->parent,
> 013e6653 Viresh Kumar 2016-05-14 184 "load and validate firmware request failed (%d)\n",
> 013e6653 Viresh Kumar 2016-05-14 185 ret);
> 013e6653 Viresh Kumar 2016-05-14 186 return ret;
> 013e6653 Viresh Kumar 2016-05-14 187 }
> 013e6653 Viresh Kumar 2016-05-14 188
> 013e6653 Viresh Kumar 2016-05-14 189 return 0;
> 013e6653 Viresh Kumar 2016-05-14 190 }
> 013e6653 Viresh Kumar 2016-05-14 191
>
> :::::: The code at line 153 was first introduced by commit
> :::::: b2abeaa10d5711e7730bb07120dd60ae27d7b930 greybus: firmware: s/_LEN/_SIZE
>
> :::::: TO: Viresh Kumar <viresh.kumar@linaro.org>
> :::::: CC: Greg Kroah-Hartman <gregkh@google.com>
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* [PATCH][next] pinctrl: pinctrl-single: add allocation failure checking of saved_vals
From: Colin King @ 2018-06-06 13:43 UTC (permalink / raw)
To: linux-arm-kernel
From: Colin Ian King <colin.king@canonical.com>
Currently saved_vals is being allocated and there is no check for
failed allocation (which is more likely than normal when using
GFP_ATOMIC). Fix this by checking for a failed allocation and
propagating this error return down the the caller chain.
Detected by CoverityScan, CID#1469841 ("Dereference null return value")
Fixes: 88a1dbdec682 ("pinctrl: pinctrl-single: Add functions to save and restore pinctrl context")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/pinctrl/pinctrl-single.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 9c3c00515aa0..0905ee002041 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1588,8 +1588,11 @@ static int pcs_save_context(struct pcs_device *pcs)
mux_bytes = pcs->width / BITS_PER_BYTE;
- if (!pcs->saved_vals)
+ if (!pcs->saved_vals) {
pcs->saved_vals = devm_kzalloc(pcs->dev, pcs->size, GFP_ATOMIC);
+ if (!pcs->saved_vals)
+ return -ENOMEM;
+ }
switch (pcs->width) {
case 64:
@@ -1649,8 +1652,13 @@ static int pinctrl_single_suspend(struct platform_device *pdev,
if (!pcs)
return -EINVAL;
- if (pcs->flags & PCS_CONTEXT_LOSS_OFF)
- pcs_save_context(pcs);
+ if (pcs->flags & PCS_CONTEXT_LOSS_OFF) {
+ int ret;
+
+ ret = pcs_save_context(pcs);
+ if (ret < 0)
+ return ret;
+ }
return pinctrl_force_sleep(pcs->pctl);
}
--
2.17.0
^ permalink raw reply related
* [PATCH] clk: imx6: fix video_27m parent for IMX6SX_CLK_CKO1_SEL
From: Fabio Estevam @ 2018-06-06 13:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180606092928.11724-1-pp@emlix.com>
On Wed, Jun 6, 2018 at 6:29 AM, Philipp Puschmann <pp@emlix.com> wrote:
> q/dl datasheets list the 5th selection value for ck01_sel as
> video_27M_clk_root.
>
> By replacing the dummy value we then can set IMX6QDL_CLK_VIDEO_27M
> as parent for IMX6QDL_CLK_CKO1_SEL.
>
> Signed-off-by: Philipp Puschmann <pp@emlix.com>
Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
^ permalink raw reply
* [RFC PATCH v2 6/6] serial: uartps: Remove CDNS_UART_NR_PORTS macro
From: Michal Simek @ 2018-06-06 12:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1528288895.git.michal.simek@xilinx.com>
This patch is removing CDNS_UART_NR_PORTS macro which limits number of
ports which can be used. Every instance is registering own struct
uart_driver with minor number which corresponds to alias ID (or 0 now).
and with 1 uart port. The same alias ID is saved to
tty_driver->name_base which is key field for creating ttyPSX name.
Because name_base and minor number are setup already there is no need to
setup any port->line number because 0 is the right value.
~# find /proc/tty/ -name "*uartps*"
/proc/tty/driver/xuartps0
/proc/tty/driver/xuartps100
Unfortunately this driver is setting up major number to 0 for using
dynamic assignment and kernel is allocating different major numbers for
every instance instead of using the same major and different minor
number.
~# ls -la /dev/ttyPS*
crw------- 1 root root 252, 0 Jan 1 03:36 /dev/ttyPS0
crw--w---- 1 root root 253, 1 Jan 1 00:00 /dev/ttyPS1
When major number is not 0. For example 252 then major/minor
combinations are in expected form
~# ls -la /dev/ttyPS*
crw------- 1 root root 252, 0 Jan 1 04:04 /dev/ttyPS0
crw--w---- 1 root root 252, 1 Jan 1 00:00 /dev/ttyPS1
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- Register one uart_driver with unique minor at probe time
This patch not done because id is not unique. This needs to be solved
before this patch can be applied. In v1 I have created
of_alias_check_id() for that. https://lkml.org/lkml/2018/4/26/551
Also we need to run more testing on this to make sure that everything is
working properly.
---
drivers/tty/serial/xilinx_uartps.c | 78 +++++++++++++++++++++++---------------
1 file changed, 48 insertions(+), 30 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index d76efe8cb3df..82cc17ec7b5d 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -30,8 +30,6 @@
#define CDNS_UART_TTY_NAME "ttyPS"
#define CDNS_UART_NAME "xuartps"
#define CDNS_UART_MAJOR 0 /* use dynamic node allocation */
-#define CDNS_UART_MINOR 0 /* works best with devtmpfs */
-#define CDNS_UART_NR_PORTS 2
#define CDNS_UART_FIFO_SIZE 64 /* FIFO size */
#define CDNS_UART_REGISTER_SPACE 0x1000
@@ -1247,8 +1245,6 @@ static int __init cdns_uart_console_setup(struct console *co, char *options)
return uart_set_options(port, co, baud, parity, bits, flow);
}
-static struct uart_driver cdns_uart_uart_driver;
-
static struct console cdns_uart_console = {
.name = CDNS_UART_TTY_NAME,
.write = cdns_uart_console_write,
@@ -1256,7 +1252,6 @@ static int __init cdns_uart_console_setup(struct console *co, char *options)
.setup = cdns_uart_console_setup,
.flags = CON_PRINTBUFFER,
.index = -1, /* Specified on the cmdline (e.g. console=ttyPS ) */
- .data = &cdns_uart_uart_driver,
};
#endif /* CONFIG_SERIAL_XILINX_PS_UART_CONSOLE */
@@ -1419,6 +1414,8 @@ static int cdns_uart_probe(struct platform_device *pdev)
struct resource *res;
struct cdns_uart *cdns_uart_data;
const struct of_device_id *match;
+ struct uart_driver *cdns_uart_uart_driver;
+ char *driver_name;
cdns_uart_data = devm_kzalloc(&pdev->dev, sizeof(*cdns_uart_data),
GFP_KERNEL);
@@ -1431,32 +1428,49 @@ static int cdns_uart_probe(struct platform_device *pdev)
/* Look for a serialN alias */
id = of_alias_get_id(pdev->dev.of_node, "serial");
if (id < 0)
+ /*
+ * FIXME this is not enough because if there the next instance
+ * without alias it will get also id = 0 which is wrong
+ */
id = 0;
- if (id >= CDNS_UART_NR_PORTS) {
- dev_err(&pdev->dev, "Cannot get uart_port structure\n");
- return -ENODEV;
- }
+ cdns_uart_uart_driver = devm_kzalloc(&pdev->dev,
+ sizeof(*cdns_uart_uart_driver),
+ GFP_KERNEL);
+ if (!cdns_uart_uart_driver)
+ return -ENOMEM;
+
+ /* There is a need to use unique driver name */
+ driver_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s%d",
+ CDNS_UART_NAME, id);
+ if (!driver_name)
+ return -ENOMEM;
- if (!cdns_uart_uart_driver.state) {
- cdns_uart_uart_driver.owner = THIS_MODULE,
- cdns_uart_uart_driver.driver_name = CDNS_UART_NAME,
- cdns_uart_uart_driver.dev_name = CDNS_UART_TTY_NAME,
- cdns_uart_uart_driver.major = CDNS_UART_MAJOR,
- cdns_uart_uart_driver.minor = CDNS_UART_MINOR,
- cdns_uart_uart_driver.nr = CDNS_UART_NR_PORTS,
+ cdns_uart_uart_driver->owner = THIS_MODULE;
+ cdns_uart_uart_driver->driver_name = driver_name;
+ cdns_uart_uart_driver->dev_name = CDNS_UART_TTY_NAME;
+ cdns_uart_uart_driver->major = CDNS_UART_MAJOR;
+ cdns_uart_uart_driver->minor = id;
+ cdns_uart_uart_driver->nr = 1;
#ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
- cdns_uart_uart_driver.cons = &cdns_uart_console,
+ cdns_uart_uart_driver->cons = &cdns_uart_console;
+ cdns_uart_console.data = cdns_uart_uart_driver;
#endif
- rc = uart_register_driver(&cdns_uart_uart_driver);
- if (rc < 0) {
- dev_err(&pdev->dev, "Failed to register driver\n");
- return rc;
- }
+ rc = uart_register_driver(cdns_uart_uart_driver);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Failed to register driver\n");
+ return rc;
}
- cdns_uart_data->cdns_uart_driver = &cdns_uart_uart_driver;
+ /*
+ * Setting up proper name_base needs to be done after uart
+ * registration because tty_driver structure is not filled.
+ * name_base is 0 by default.
+ */
+ cdns_uart_uart_driver->tty_driver->name_base = id;
+
+ cdns_uart_data->cdns_uart_driver = cdns_uart_uart_driver;
match = of_match_node(cdns_uart_of_match, pdev->dev.of_node);
if (match && match->data) {
@@ -1473,7 +1487,8 @@ static int cdns_uart_probe(struct platform_device *pdev)
}
if (IS_ERR(cdns_uart_data->pclk)) {
dev_err(&pdev->dev, "pclk clock not found.\n");
- return PTR_ERR(cdns_uart_data->pclk);
+ rc = PTR_ERR(cdns_uart_data->pclk);
+ goto err_out_unregister_driver;
}
cdns_uart_data->uartclk = devm_clk_get(&pdev->dev, "uart_clk");
@@ -1484,13 +1499,14 @@ static int cdns_uart_probe(struct platform_device *pdev)
}
if (IS_ERR(cdns_uart_data->uartclk)) {
dev_err(&pdev->dev, "uart_clk clock not found.\n");
- return PTR_ERR(cdns_uart_data->uartclk);
+ rc = PTR_ERR(cdns_uart_data->uartclk);
+ goto err_out_unregister_driver;
}
rc = clk_prepare_enable(cdns_uart_data->pclk);
if (rc) {
dev_err(&pdev->dev, "Unable to enable pclk clock.\n");
- return rc;
+ goto err_out_unregister_driver;
}
rc = clk_prepare_enable(cdns_uart_data->uartclk);
if (rc) {
@@ -1525,7 +1541,6 @@ static int cdns_uart_probe(struct platform_device *pdev)
port->flags = UPF_BOOT_AUTOCONF;
port->ops = &cdns_uart_ops;
port->fifosize = CDNS_UART_FIFO_SIZE;
- port->line = id;
/*
* Register the port.
@@ -1552,11 +1567,11 @@ static int cdns_uart_probe(struct platform_device *pdev)
* If register_console() don't assign value, then console_port pointer
* is cleanup.
*/
- if (cdns_uart_uart_driver.cons->index == -1)
+ if (cdns_uart_uart_driver->cons->index == -1)
console_port = port;
#endif
- rc = uart_add_one_port(&cdns_uart_uart_driver, port);
+ rc = uart_add_one_port(cdns_uart_uart_driver, port);
if (rc) {
dev_err(&pdev->dev,
"uart_add_one_port() failed; err=%i\n", rc);
@@ -1565,7 +1580,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
#ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
/* This is not port which is used for console that's why clean it up */
- if (cdns_uart_uart_driver.cons->index == -1)
+ if (cdns_uart_uart_driver->cons->index == -1)
console_port = NULL;
#endif
@@ -1583,6 +1598,8 @@ static int cdns_uart_probe(struct platform_device *pdev)
clk_disable_unprepare(cdns_uart_data->uartclk);
err_out_clk_dis_pclk:
clk_disable_unprepare(cdns_uart_data->pclk);
+err_out_unregister_driver:
+ uart_unregister_driver(cdns_uart_data->cdns_uart_driver);
return rc;
}
@@ -1611,6 +1628,7 @@ static int cdns_uart_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
pm_runtime_set_suspended(&pdev->dev);
pm_runtime_dont_use_autosuspend(&pdev->dev);
+ uart_unregister_driver(cdns_uart_data->cdns_uart_driver);
return rc;
}
--
1.9.1
^ permalink raw reply related
* [RFC PATCH v2 5/6] serial: uartps: Fill struct uart_driver in probe()
From: Michal Simek @ 2018-06-06 12:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1528288895.git.michal.simek@xilinx.com>
This is preparation step for dynamic port allocation without
CDNS_UART_NR_PORTS macro. Fill the structure only once at probe.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- new patch - it can be sent separately too
drivers/tty/serial/xilinx_uartps.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index b47d7ccbc38d..d76efe8cb3df 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1260,18 +1260,6 @@ static int __init cdns_uart_console_setup(struct console *co, char *options)
};
#endif /* CONFIG_SERIAL_XILINX_PS_UART_CONSOLE */
-static struct uart_driver cdns_uart_uart_driver = {
- .owner = THIS_MODULE,
- .driver_name = CDNS_UART_NAME,
- .dev_name = CDNS_UART_TTY_NAME,
- .major = CDNS_UART_MAJOR,
- .minor = CDNS_UART_MINOR,
- .nr = CDNS_UART_NR_PORTS,
-#ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
- .cons = &cdns_uart_console,
-#endif
-};
-
#ifdef CONFIG_PM_SLEEP
/**
* cdns_uart_suspend - suspend event
@@ -1451,6 +1439,16 @@ static int cdns_uart_probe(struct platform_device *pdev)
}
if (!cdns_uart_uart_driver.state) {
+ cdns_uart_uart_driver.owner = THIS_MODULE,
+ cdns_uart_uart_driver.driver_name = CDNS_UART_NAME,
+ cdns_uart_uart_driver.dev_name = CDNS_UART_TTY_NAME,
+ cdns_uart_uart_driver.major = CDNS_UART_MAJOR,
+ cdns_uart_uart_driver.minor = CDNS_UART_MINOR,
+ cdns_uart_uart_driver.nr = CDNS_UART_NR_PORTS,
+#ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
+ cdns_uart_uart_driver.cons = &cdns_uart_console,
+#endif
+
rc = uart_register_driver(&cdns_uart_uart_driver);
if (rc < 0) {
dev_err(&pdev->dev, "Failed to register driver\n");
--
1.9.1
^ permalink raw reply related
* [RFC PATCH v2 4/6] serial: uartps: Move alias reading higher in probe()
From: Michal Simek @ 2018-06-06 12:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1528288895.git.michal.simek@xilinx.com>
This cosmetic change is done only for having next patch much easier to
read. Moving id setup higher in probe is not affecting any usage of this
driver and it also simplify error path.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- new patch - it can be sent separately too
drivers/tty/serial/xilinx_uartps.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index fe96fd950d3a..b47d7ccbc38d 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1440,6 +1440,16 @@ static int cdns_uart_probe(struct platform_device *pdev)
if (!port)
return -ENOMEM;
+ /* Look for a serialN alias */
+ id = of_alias_get_id(pdev->dev.of_node, "serial");
+ if (id < 0)
+ id = 0;
+
+ if (id >= CDNS_UART_NR_PORTS) {
+ dev_err(&pdev->dev, "Cannot get uart_port structure\n");
+ return -ENODEV;
+ }
+
if (!cdns_uart_uart_driver.state) {
rc = uart_register_driver(&cdns_uart_uart_driver);
if (rc < 0) {
@@ -1509,16 +1519,6 @@ static int cdns_uart_probe(struct platform_device *pdev)
&cdns_uart_data->clk_rate_change_nb))
dev_warn(&pdev->dev, "Unable to register clock notifier.\n");
#endif
- /* Look for a serialN alias */
- id = of_alias_get_id(pdev->dev.of_node, "serial");
- if (id < 0)
- id = 0;
-
- if (id >= CDNS_UART_NR_PORTS) {
- dev_err(&pdev->dev, "Cannot get uart_port structure\n");
- rc = -ENODEV;
- goto err_out_notif_unreg;
- }
/* At this point, we've got an empty uart_port struct, initialize it */
spin_lock_init(&port->lock);
@@ -1577,7 +1577,6 @@ static int cdns_uart_probe(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
pm_runtime_set_suspended(&pdev->dev);
pm_runtime_dont_use_autosuspend(&pdev->dev);
-err_out_notif_unreg:
#ifdef CONFIG_COMMON_CLK
clk_notifier_unregister(cdns_uart_data->uartclk,
&cdns_uart_data->clk_rate_change_nb);
--
1.9.1
^ permalink raw reply related
* [RFC PATCH v2 3/6] serial: uartps: Do not use static struct uart_driver out of probe()
From: Michal Simek @ 2018-06-06 12:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1528288895.git.michal.simek@xilinx.com>
cdns_uart_suspend()/resume() and remove() are using static reference
to struct uart_driver. Assign this referece to private data structure
as preparation step for dynamic struct uart_driver allocation.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- new patch - it can be sent separately too
drivers/tty/serial/xilinx_uartps.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index e24382f58dea..fe96fd950d3a 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -179,6 +179,7 @@
* @port: Pointer to the UART port
* @uartclk: Reference clock
* @pclk: APB clock
+ * @cdns_uart_driver: Pointer to UART driver
* @baud: Current baud rate
* @clk_rate_change_nb: Notifier block for clock changes
* @quirks: Flags for RXBS support.
@@ -187,6 +188,7 @@ struct cdns_uart {
struct uart_port *port;
struct clk *uartclk;
struct clk *pclk;
+ struct uart_driver *cdns_uart_driver;
unsigned int baud;
struct notifier_block clk_rate_change_nb;
u32 quirks;
@@ -1280,6 +1282,7 @@ static int __init cdns_uart_console_setup(struct console *co, char *options)
static int cdns_uart_suspend(struct device *device)
{
struct uart_port *port = dev_get_drvdata(device);
+ struct cdns_uart *cdns_uart = port->private_data;
struct tty_struct *tty;
struct device *tty_dev;
int may_wake = 0;
@@ -1296,7 +1299,7 @@ static int cdns_uart_suspend(struct device *device)
* Call the API provided in serial_core.c file which handles
* the suspend.
*/
- uart_suspend_port(&cdns_uart_uart_driver, port);
+ uart_suspend_port(cdns_uart->cdns_uart_driver, port);
if (!(console_suspend_enabled && !may_wake)) {
unsigned long flags = 0;
@@ -1324,6 +1327,7 @@ static int cdns_uart_suspend(struct device *device)
static int cdns_uart_resume(struct device *device)
{
struct uart_port *port = dev_get_drvdata(device);
+ struct cdns_uart *cdns_uart = port->private_data;
unsigned long flags = 0;
u32 ctrl_reg;
struct tty_struct *tty;
@@ -1339,8 +1343,6 @@ static int cdns_uart_resume(struct device *device)
}
if (console_suspend_enabled && !may_wake) {
- struct cdns_uart *cdns_uart = port->private_data;
-
clk_enable(cdns_uart->pclk);
clk_enable(cdns_uart->uartclk);
@@ -1374,7 +1376,7 @@ static int cdns_uart_resume(struct device *device)
spin_unlock_irqrestore(&port->lock, flags);
}
- return uart_resume_port(&cdns_uart_uart_driver, port);
+ return uart_resume_port(cdns_uart->cdns_uart_driver, port);
}
#endif /* ! CONFIG_PM_SLEEP */
static int __maybe_unused cdns_runtime_suspend(struct device *dev)
@@ -1446,6 +1448,8 @@ static int cdns_uart_probe(struct platform_device *pdev)
}
}
+ cdns_uart_data->cdns_uart_driver = &cdns_uart_uart_driver;
+
match = of_match_node(cdns_uart_of_match, pdev->dev.of_node);
if (match && match->data) {
const struct cdns_platform_data *data = match->data;
@@ -1603,7 +1607,7 @@ static int cdns_uart_remove(struct platform_device *pdev)
clk_notifier_unregister(cdns_uart_data->uartclk,
&cdns_uart_data->clk_rate_change_nb);
#endif
- rc = uart_remove_one_port(&cdns_uart_uart_driver, port);
+ rc = uart_remove_one_port(cdns_uart_data->cdns_uart_driver, port);
port->mapbase = 0;
clk_disable_unprepare(cdns_uart_data->uartclk);
clk_disable_unprepare(cdns_uart_data->pclk);
--
1.9.1
^ permalink raw reply related
* [RFC PATCH v2 2/6] serial: uartps: Move register to probe based on run time detection
From: Michal Simek @ 2018-06-06 12:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1528288895.git.michal.simek@xilinx.com>
Find out the highest serial alias and allocate that amount of
structures/minor numbers to be able to handle all of them.
Origin setting that there are two prealocated CDNS_UART_NR_PORTS is kept
there.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- Remove nr field logic
Discussed here: https://patchwork.kernel.org/patch/9738445/
The same solution is done in pl011 driver.
---
drivers/tty/serial/xilinx_uartps.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 5f116f3ecd4a..e24382f58dea 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1438,6 +1438,14 @@ static int cdns_uart_probe(struct platform_device *pdev)
if (!port)
return -ENOMEM;
+ if (!cdns_uart_uart_driver.state) {
+ rc = uart_register_driver(&cdns_uart_uart_driver);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Failed to register driver\n");
+ return rc;
+ }
+ }
+
match = of_match_node(cdns_uart_of_match, pdev->dev.of_node);
if (match && match->data) {
const struct cdns_platform_data *data = match->data;
@@ -1617,28 +1625,14 @@ static int cdns_uart_remove(struct platform_device *pdev)
static int __init cdns_uart_init(void)
{
- int retval = 0;
-
- /* Register the cdns_uart driver with the serial core */
- retval = uart_register_driver(&cdns_uart_uart_driver);
- if (retval)
- return retval;
-
/* Register the platform driver */
- retval = platform_driver_register(&cdns_uart_platform_driver);
- if (retval)
- uart_unregister_driver(&cdns_uart_uart_driver);
-
- return retval;
+ return platform_driver_register(&cdns_uart_platform_driver);
}
static void __exit cdns_uart_exit(void)
{
/* Unregister the platform driver */
platform_driver_unregister(&cdns_uart_platform_driver);
-
- /* Unregister the cdns_uart driver */
- uart_unregister_driver(&cdns_uart_uart_driver);
}
arch_initcall(cdns_uart_init);
--
1.9.1
^ permalink raw reply related
* [RFC PATCH v2 1/6] serial: uartps: Do not initialize field to zero again
From: Michal Simek @ 2018-06-06 12:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1528288895.git.michal.simek@xilinx.com>
Writing zero and NULLs to already initialized fields is not needed.
Remove this additional writes.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- new patch - it can be sent separately too
drivers/tty/serial/xilinx_uartps.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 8a3e34234e98..5f116f3ecd4a 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1510,15 +1510,12 @@ static int cdns_uart_probe(struct platform_device *pdev)
/* At this point, we've got an empty uart_port struct, initialize it */
spin_lock_init(&port->lock);
- port->membase = NULL;
- port->irq = 0;
port->type = PORT_UNKNOWN;
port->iotype = UPIO_MEM32;
port->flags = UPF_BOOT_AUTOCONF;
port->ops = &cdns_uart_ops;
port->fifosize = CDNS_UART_FIFO_SIZE;
port->line = id;
- port->dev = NULL;
/*
* Register the port.
--
1.9.1
^ permalink raw reply related
* [RFC PATCH v2 0/6] serial: uartps: Add run time support for more IPs than hardcoded 2
From: Michal Simek @ 2018-06-06 12:41 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
this series is trying to address discussion I had with Alan in past
https://patchwork.kernel.org/patch/9738445/ and also with Rob in v1
https://lkml.org/lkml/2018/4/26/551.
The first 5 patches are preparation patches to have the last patch as
small as possible to focus on changes there.
Cases without DT aliases are not solved in this series but one function
was shared in RFC v1.
The purpose of this series to get feedback on solution where every
driver instance allocate at run time own uart_driver.
For example this is how it works.
uart0 on higher alias
serial0 = &uart1;
serial100 = &uart0;
~# ls -la /dev/ttyPS*
crw------- 1 root root 252, 0 Jun 6 12:19 /dev/ttyPS0
crw--w---- 1 root root 253, 100 Jan 1 1970 /dev/ttyPS100
Thanks,
Michal
Changes in v2:
- new patch - it can be sent separately too
- Remove nr field logic
- new patch - it can be sent separately too
- new patch - it can be sent separately too
- new patch - it can be sent separately too
- Register one uart_driver with unique minor at probe time
Michal Simek (6):
serial: uartps: Do not initialize field to zero again
serial: uartps: Move register to probe based on run time detection
serial: uartps: Do not use static struct uart_driver out of probe()
serial: uartps: Move alias reading higher in probe()
serial: uartps: Fill struct uart_driver in probe()
serial: uartps: Remove CDNS_UART_NR_PORTS macro
drivers/tty/serial/xilinx_uartps.c | 126 ++++++++++++++++++++-----------------
1 file changed, 68 insertions(+), 58 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH 09/10] dpaa_eth: add support for hardware timestamping
From: Y.b. Lu @ 2018-06-06 11:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180605135748.mlarwiyzf2oe27ax@localhost>
Hi Richard,
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran at gmail.com]
> Sent: Tuesday, June 5, 2018 9:58 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: netdev at vger.kernel.org; Madalin-cristian Bucur
> <madalin.bucur@nxp.com>; Rob Herring <robh+dt@kernel.org>; Shawn Guo
> <shawnguo@kernel.org>; David S . Miller <davem@davemloft.net>;
> devicetree at vger.kernel.org; linuxppc-dev at lists.ozlabs.org;
> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org
> Subject: Re: [PATCH 09/10] dpaa_eth: add support for hardware timestamping
>
> On Tue, Jun 05, 2018 at 03:35:28AM +0000, Y.b. Lu wrote:
> > [Y.b. Lu] Actually these timestamping codes affected DPAA networking
> performance in our previous performance test.
> > That's why we used ifdef for it.
>
> How much does time stamping hurt performance?
>
> If the time stamping is compiled in but not enabled at run time, does it still
> affect performace?
[Y.b. Lu] I can't remember and find the old data since it had been a long time.
I just did the iperf test today between two 10G ports. I didn?t see any performance changes with timestamping code ?
So, let's me remove the ifdef in next version.
Thanks a lot.
>
> Thanks,
> Richard
^ permalink raw reply
* panic kexec broken on ARM64?
From: Petr Tesarik @ 2018-06-06 11:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180606100024.50be6b75@ezekiel.suse.cz>
On Wed, 6 Jun 2018 10:00:24 +0200
Petr Tesarik <ptesarik@suse.cz> wrote:
> On Wed, 6 Jun 2018 09:02:04 +0200
> Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
> > Hi Petr,
> >
> > Am 05.06.2018 um 19:46 schrieb James Morse:
> > > Hi Petr,
> > >
> > > (CC: +Akashi, Marc)
> > >
> > > On 05/06/18 09:01, Petr Tesarik wrote:
> > >> I have observed hangs after crash on a Raspberry Pi 3 Model B+ board
> > >> when a panic kernel is loaded.
> > > kdump is a best-effort thing, it looks like this is a case where the
> > > crashed-kernel can't tear itself down.
> > >
> > > Do you have the rest of the stack trace? Was it handling an irq when it decided
> > > to panic?:
> > > https://lkml.org/lkml/2018/3/13/1134
> >
> > the Raspberry Pi 3 B+ support is very fresh (linux-next). Since i didn't
> > see a version, i need to doublecheck.
> >
> > You are actually using linux-next and not the downstream kernel?
>
> Very good point. I'll try again with linux-next.
It took me some time to set up everything correctly again...
Unfortunately, it makes no difference. I set a hardware breakpoint on
machine_crash_shutdown, followed by a breakpoint at __switch_to, and it
did trigger:
(gdb) lx-version
Linux version 4.17.0-next-20180605-18-default (root at thunderx10) (gcc version 4.8.5 (SUSE Linux)) #1 SMP Wed Jun 6 10:26:46 CEST 2018
(gdb) bt
#0 __switch_to (prev=0xffff80002b428240, next=0xffff000008c32700 <init_task>) at arch/arm64/kernel/process.c:419
#1 0xffff0000088003d4 in context_switch (rf=<optimized out>, next=<optimized out>, prev=<optimized out>, rq=<optimized out>) at kernel/sched/core.c:2860
#2 __schedule (preempt=false) at kernel/sched/core.c:3502
#3 0xffff00000880092c in schedule () at kernel/sched/core.c:3546
#4 0xffff000008803e24 in schedule_timeout (timeout=<optimized out>) at kernel/time/timer.c:1801
#5 0xffff00000880144c in do_wait_for_common (state=<optimized out>, timeout=<optimized out>, action=<optimized out>, x=<optimized out>)
at kernel/sched/completion.c:83
#6 __wait_for_common (state=<optimized out>, timeout=<optimized out>, action=<optimized out>, x=<optimized out>) at kernel/sched/completion.c:104
#7 wait_for_common (x=0xffff80002d0ef548, timeout=500, state=<optimized out>) at kernel/sched/completion.c:115
#8 0xffff000008801554 in wait_for_completion_timeout (x=0xffff80002d0ef548, timeout=<optimized out>) at kernel/sched/completion.c:155
#9 0xffff0000008f5ef8 in usb_start_wait_urb (urb=0xffff80002c593400, timeout=5000, actual_length=0xffff80002d0ef5dc) at drivers/usb/core/message.c:62
#10 0xffff0000008f602c in usb_internal_control_msg (timeout=<optimized out>, len=<optimized out>, data=<optimized out>, cmd=<optimized out>, pipe=<optimized out>,
usb_dev=<optimized out>) at drivers/usb/core/message.c:101
#11 usb_control_msg (dev=0xffff80002c684000, pipe=2147484800, request=161 '\241', requesttype=192 '\300', value=0, index=152, data=0xffff80002d421c80, size=4,
timeout=5000) at drivers/usb/core/message.c:152
#12 0xffff000000f29e10 in lan78xx_read_reg (index=152, data=0xffff80002d0ef66c, dev=<optimized out>, dev=<optimized out>) at drivers/net/usb/lan78xx.c:449
#13 0xffff000000f2c018 in lan78xx_irq_bus_sync_unlock (irqd=<optimized out>) at drivers/net/usb/lan78xx.c:1954
#14 0xffff0000081168e4 in chip_bus_sync_unlock (desc=<optimized out>) at kernel/irq/internals.h:147
#15 __irq_put_desc_unlock (desc=0xffff80002e7a9400, flags=<optimized out>, bus=true) at kernel/irq/irqdesc.c:837
#16 0xffff0000081176c0 in irq_put_desc_busunlock (flags=<optimized out>, desc=<optimized out>) at kernel/irq/internals.h:173
#17 irq_set_irqchip_state (irq=<optimized out>, which=IRQCHIP_STATE_ACTIVE, val=false) at kernel/irq/manage.c:2205
#18 0xffff00000809e0b0 in machine_kexec_mask_interrupts () at arch/arm64/kernel/machine_kexec.c:233
#19 machine_crash_shutdown (regs=<optimized out>) at arch/arm64/kernel/machine_kexec.c:259
#20 0xffff00000815b358 in __crash_kexec (regs=0xffff80002d0efb50) at kernel/kexec_core.c:943
#21 0xffff00000815b45c in crash_kexec (regs=0xffff80002d0efb50) at kernel/kexec_core.c:965
#22 0xffff00000808dc84 in die (str=<optimized out>, regs=0xffff80002d0efb50, err=<optimized out>) at arch/arm64/kernel/traps.c:210
#23 0xffff0000080a2114 in die_kernel_fault (msg=0xffff000008a09c88 "NULL pointer dereference", addr=0, esr=2516582468, regs=<optimized out>)
at arch/arm64/mm/fault.c:269
#24 0xffff0000080a1d68 in __do_kernel_fault (addr=0, esr=2516582468, regs=0xffff80002d0efb50) at arch/arm64/mm/fault.c:297
#25 0xffff000008806e38 in do_page_fault (addr=0, esr=2516582468, regs=0xffff80002d0efb50) at arch/arm64/mm/fault.c:599
#26 0xffff0000088070dc in do_translation_fault (addr=0, esr=<optimized out>, regs=<optimized out>) at arch/arm64/mm/fault.c:608
#27 0xffff0000080812cc in do_mem_abort (addr=0, esr=2516582468, regs=0xffff80002d0efb50) at arch/arm64/mm/fault.c:744
#28 0xffff000008082ed0 in el1_sync () at arch/arm64/kernel/entry.S:583
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb)
The system hanged in the idle thread after continuing here.
Petr T
^ permalink raw reply
* panic kexec broken on ARM64?
From: James Morse @ 2018-06-06 11:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5e539510-01b7-ebc5-778e-b49a5261f9bc@i2se.com>
Hi Stefan,
On 06/06/18 08:02, Stefan Wahren wrote:
> Am 05.06.2018 um 19:46 schrieb James Morse:
>> On 05/06/18 09:01, Petr Tesarik wrote:
>>> I attached a hardware debugger and found
>>> out that all CPU cores were stopped except one which was stuck in the
>>> idle thread. It seems that irq_set_irqchip_state() may sleep, which is
>>> definitely not safe after a kernel panic.
>> I don't know much about irqchip stuff, but __irq_get_desc_lock() takes a
>> raw_spin_lock(), and calls gic_irq_get_irqchip_state() which is just poking
>> around in mmio registers, this should all be safe unless you re-entered the same
>> code.
>>> If I'm right, then this is broken in general, but I have only ever seen
>>> it on RPi 3 Model B+ (even RPi3 Model B works fine), so the issue may
>>> be more subtle.
>> Is there a hardware difference around the interrupt controller on these?
> No, but the RPi 3 B has a different USB network chip on board (smsc95xx, Fast
> ethernet) instead of lan78xx (Gigabit ethernet).
Bingo: its the lan78xx driver that is sleeping from the irqchip callbacks; The
smsc95xx driver doesn't have a struct irq_chip, which is why the RPi-3-B doesn't
do this.
It may be valid for kdump to only teardown the 'root irqdomain' (if that even
means anything). I assume these secondary irqchip's would have a
summary-interrupt that goes to another irqchip. But I can't see a way to tell
them apart..,
I think we need to wait until after the merge window for Marc's wisdom on this!
Thanks,
James
^ permalink raw reply
* [PATCH v5 6/6] tty/serial: atmel: changed the driver to work under at91-usart mfd
From: Richard Genoud @ 2018-06-06 11:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180604165943.31381-7-radu.pirea@microchip.com>
Typo in the subject:
changed->change
On 04/06/2018 18:59, Radu Pirea wrote:
> This patch modifies the place where resources and device tree properties
> are searched.
>
> Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
> ---
> drivers/tty/serial/Kconfig | 1 +
> drivers/tty/serial/atmel_serial.c | 41 ++++++++++++++++++-------------
> 2 files changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 3682fd3e960c..25e55332f8b1 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -119,6 +119,7 @@ config SERIAL_ATMEL
> depends on ARCH_AT91 || COMPILE_TEST
> select SERIAL_CORE
> select SERIAL_MCTRL_GPIO if GPIOLIB
> + select MFD_AT91_USART
> help
> This enables the driver for the on-chip UARTs of the Atmel
> AT91 processors.
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index df46a9e88c34..5c74e03396ef 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -193,8 +193,8 @@ static struct console atmel_console;
>
> #if defined(CONFIG_OF)
> static const struct of_device_id atmel_serial_dt_ids[] = {
> - { .compatible = "atmel,at91rm9200-usart" },
> - { .compatible = "atmel,at91sam9260-usart" },
> + { .compatible = "atmel,at91rm9200-usart-serial" },
> + { .compatible = "atmel,at91sam9260-usart-serial" },
Sorry, I didn't catch that before, but we can drop "atmel,at91sam9260-usart-serial" don't we ?
Only "atmel,at91rm9200-usart-serial" is used in the MFD driver.
> { /* sentinel */ }
> };
> #endif
> @@ -915,6 +915,7 @@ static void atmel_tx_dma(struct uart_port *port)
> static int atmel_prepare_tx_dma(struct uart_port *port)
> {
> struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> + struct device *mfd_dev = port->dev->parent;
> dma_cap_mask_t mask;
> struct dma_slave_config config;
> int ret, nent;
> @@ -922,7 +923,7 @@ static int atmel_prepare_tx_dma(struct uart_port *port)
> dma_cap_zero(mask);
> dma_cap_set(DMA_SLAVE, mask);
>
> - atmel_port->chan_tx = dma_request_slave_channel(port->dev, "tx");
> + atmel_port->chan_tx = dma_request_slave_channel(mfd_dev, "tx");
> if (atmel_port->chan_tx == NULL)
> goto chan_err;
> dev_info(port->dev, "using %s for tx DMA transfers\n",
> @@ -1093,6 +1094,7 @@ static void atmel_rx_from_dma(struct uart_port *port)
> static int atmel_prepare_rx_dma(struct uart_port *port)
> {
> struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> + struct device *mfd_dev = port->dev->parent;
> struct dma_async_tx_descriptor *desc;
> dma_cap_mask_t mask;
> struct dma_slave_config config;
> @@ -1104,7 +1106,7 @@ static int atmel_prepare_rx_dma(struct uart_port *port)
> dma_cap_zero(mask);
> dma_cap_set(DMA_CYCLIC, mask);
>
> - atmel_port->chan_rx = dma_request_slave_channel(port->dev, "rx");
> + atmel_port->chan_rx = dma_request_slave_channel(mfd_dev, "rx");
> if (atmel_port->chan_rx == NULL)
> goto chan_err;
> dev_info(port->dev, "using %s for rx DMA transfers\n",
> @@ -1631,7 +1633,7 @@ static void atmel_tasklet_tx_func(unsigned long data)
> static void atmel_init_property(struct atmel_uart_port *atmel_port,
> struct platform_device *pdev)
> {
> - struct device_node *np = pdev->dev.of_node;
> + struct device_node *np = pdev->dev.parent->of_node;
I think this is not needed anymore (cf atmel_probe())
>
> /* DMA/PDC usage specification */
> if (of_property_read_bool(np, "atmel,use-dma-rx")) {
> @@ -2222,8 +2224,8 @@ static const char *atmel_type(struct uart_port *port)
> */
> static void atmel_release_port(struct uart_port *port)
> {
> - struct platform_device *pdev = to_platform_device(port->dev);
> - int size = pdev->resource[0].end - pdev->resource[0].start + 1;
> + struct platform_device *mpdev = to_platform_device(port->dev->parent);
> + int size = resource_size(mpdev->resource);
>
> release_mem_region(port->mapbase, size);
>
> @@ -2238,8 +2240,8 @@ static void atmel_release_port(struct uart_port *port)
> */
> static int atmel_request_port(struct uart_port *port)
> {
> - struct platform_device *pdev = to_platform_device(port->dev);
> - int size = pdev->resource[0].end - pdev->resource[0].start + 1;
> + struct platform_device *mpdev = to_platform_device(port->dev->parent);
> + int size = resource_size(mpdev->resource);
>
> if (!request_mem_region(port->mapbase, size, "atmel_serial"))
> return -EBUSY;
> @@ -2341,27 +2343,28 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
> {
> int ret;
> struct uart_port *port = &atmel_port->uart;
> + struct platform_device *mpdev = to_platform_device(pdev->dev.parent);
>
> atmel_init_property(atmel_port, pdev);
> atmel_set_ops(port);
>
> - uart_get_rs485_mode(&pdev->dev, &port->rs485);
> + uart_get_rs485_mode(&mpdev->dev, &port->rs485);
>
> port->iotype = UPIO_MEM;
> port->flags = UPF_BOOT_AUTOCONF | UPF_IOREMAP;
> port->ops = &atmel_pops;
> port->fifosize = 1;
> port->dev = &pdev->dev;
> - port->mapbase = pdev->resource[0].start;
> - port->irq = pdev->resource[1].start;
> + port->mapbase = mpdev->resource[0].start;
> + port->irq = mpdev->resource[1].start;
> port->rs485_config = atmel_config_rs485;
> - port->membase = NULL;
> + port->membase = NULL;
>
> memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
>
> /* for console, the clock could already be configured */
> if (!atmel_port->clk) {
> - atmel_port->clk = clk_get(&pdev->dev, "usart");
> + atmel_port->clk = clk_get(&mpdev->dev, "usart");
> if (IS_ERR(atmel_port->clk)) {
> ret = PTR_ERR(atmel_port->clk);
> atmel_port->clk = NULL;
> @@ -2652,11 +2655,13 @@ static int atmel_serial_resume(struct platform_device *pdev)
> static void atmel_serial_probe_fifos(struct atmel_uart_port *atmel_port,
> struct platform_device *pdev)
> {
> + struct device *dev = pdev->dev.parent;
> +
Ditto
> atmel_port->fifo_size = 0;
> atmel_port->rts_low = 0;
> atmel_port->rts_high = 0;
>
> - if (of_property_read_u32(pdev->dev.of_node,
> + if (of_property_read_u32(dev->of_node,
Ditto
> "atmel,fifo-size",
> &atmel_port->fifo_size))
> return;
> @@ -2694,13 +2699,15 @@ static void atmel_serial_probe_fifos(struct atmel_uart_port *atmel_port,
> static int atmel_serial_probe(struct platform_device *pdev)
> {
> struct atmel_uart_port *atmel_port;
> - struct device_node *np = pdev->dev.of_node;
> + struct device_node *np = pdev->dev.parent->of_node;
> void *data;
> int ret = -ENODEV;
> bool rs485_enabled;
>
> BUILD_BUG_ON(ATMEL_SERIAL_RINGSIZE & (ATMEL_SERIAL_RINGSIZE - 1));
>
> + pdev->dev.of_node = np;
> +
As atmel_serial device's of_node is now the parent node, the changes
in atmel_init_property() and atmel_serial_probe_fifos() are not needed anymore.
And maybe add a comment to explain that this is part of a MFD and all
the attributes are in the parent node, so we're using the MFD device node
instead of this device node.
> ret = of_alias_get_id(np, "serial");
> if (ret < 0)
> /* port id not found in platform data nor device-tree aliases:
> @@ -2845,7 +2852,7 @@ static struct platform_driver atmel_serial_driver = {
> .suspend = atmel_serial_suspend,
> .resume = atmel_serial_resume,
> .driver = {
> - .name = "atmel_usart",
> + .name = "atmel_usart_serial",
> .of_match_table = of_match_ptr(atmel_serial_dt_ids),
> },
> };
>
Thanks !
Richard.
^ permalink raw reply
* [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
From: A.s. Dong @ 2018-06-06 10:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <AM0PR04MB4211FCC9F3902F14EBD28313806E0@AM0PR04MB4211.eurprd04.prod.outlook.com>
Hi Sascha,
> -----Original Message-----
> From: A.s. Dong
> Sent: Monday, May 28, 2018 5:25 PM
> To: 'Sascha Hauer' <s.hauer@pengutronix.de>
> Cc: 'linux-arm-kernel at lists.infradead.org' <linux-arm-
> kernel at lists.infradead.org>; 'dongas86 at gmail.com' <dongas86@gmail.com>;
> dl-linux-imx <linux-imx@nxp.com>; 'kernel at pengutronix.de'
> <kernel@pengutronix.de>; Fabio Estevam <fabio.estevam@nxp.com>;
> 'shawnguo at kernel.org' <shawnguo@kernel.org>
> Subject: RE: [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
>
...
> > > > > We discussed this internally and came to the conclusion that the
> > > > > SCU is not a generic user of a MU. The MU is designed to work
> > > > > together with a piece of SRAM to form a mailbox system. Instead
> > > > > of working as designed the SCU morses the messages through the
> > > > > doorbell (what the MU really is). For anything that uses the MU
> > > > > in the way it is designed I would suggest using the mailbox
> > > > > interface from drivers/mailbox/ along with the binding from
> > > Documentation/devicetree/bindings/mailbox/mailbox.txt.
> > > > >
> > > > > In the way I suggest there is no need for any MU ID.
> > > > >
> > > >
> > > > So you're suggesting switch to use mailbox instead of private MU
> > > > library function calls?
> > > > Something like:
> > > > scu {
> > > > compatible = "nxp,imx8qxp-scu", "simple-bus";
> > > > mboxes = <&mailbox 0>;
> > > > }
> > > > Then IPC is implemented based on mailbox?
> > > >
> > > > As I replied Oleksij Rempel in another mail in this thread, we've
> > > > tried mailbox but got performance regression issue and need more
> > > > time to investigate whether it's really quite suitable for i.MX
> > > > SCU firmware as SCU handling message quite fast in micro seconds.
> > > > (Mailbox polling method has much more latency than current MU
> > > > sample polling we
> > > > used) and we want to avoid the SCU firmware API changes.
> > >
> > > I am not suggesting to do mailboxing (using shared memory) for the SCU.
> > > I am also not suggesting any API update for the SCU communication.
> > > I am just mentioning that doing mailboxing is the way the MU was
> > > originally designed for. Looking at the reference manual I see many
> > > MUs on
> > the i.MX8.
> > > I guess most of them are for communication between the different
> > > cores on the system. For these it's probably worth writing some
> > > generic MU
> > driver.
> > > The way the MU is used for communication with the SCU though is so
> > > special that it's not worth writing a generic driver, so just
> > > integrate the MU access functions in the SCU code.
> > >
> >
> > I understand it.
> >
> > One problem of the way you suggested may be that:
> > If we doing like below, we may lose flexibility to change the MU used
> > for SCU firmware communication.
> > scu at 5d1b0000 {
> > compatible = "fsl,imx8qxp-scu";
> > reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > };
> >
> > And current design is that the system supports multiple MU channels
> > used by various users at the same time, e.g. SCU, Power Domain, Clock and
> others.
> > User can flexibly change it under their nodes: And each MU channel is
> > protected by their private lock and not affect each others.
> >
> > e.g.
> > scu {
> > compatible = "nxp,imx8qxp-scu", "simple-bus";
> > fsl,mu = <&lsio_mu0>;
> >
> > clk: clk {
> > compatible = "fsl,imx8qxp-clk";
> > #clock-cells = <1>;
> > };
> >
> > iomuxc: iomuxc {
> > compatible = "fsl,imx8qxp-iomuxc";
> > fsl,mu = <&lsio_mu3>;
> > };
> >
> > imx8qx-pm {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > fsl,mu = <&lsio_mu4>;
> > .............
> > }
> >
> > The default code only uses MU0 which is used by SCU.
> >
> > The change may affect this design. Any ideas?
> > Do you think we can keep the current way?
> >
>
> Any comments about this?
>
Have you got a chance to help look at it?
We need identify the direction to go next.
Regards
Dong Aisheng
> Regards
> Dong Aisheng
>
> > Regards
> > Dong Aisheng
> >
> > > Sascha
> > >
> > > --
> > > Pengutronix e.K. | |
> > > Industrial Linux Solutions |
> > >
> >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> > >
> >
> w.pengutronix.de%2F&data=02%7C01%7Caisheng.dong%40nxp.com%7C266
> > >
> 24a5c38e5488a80b708d5b0f3107b%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> > >
> >
> 7C0%7C0%7C636609480354404338&sdata=XP%2BYdt9I7zURrQun2jRbempLhC
> > > XrYtMVMb3dEWrZuro%3D&reserved=0 |
> > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox