* [PATCH 5/5 v2] clk: versatile: respect parent rate in ICST clock
From: Linus Walleij @ 2014-01-20 21:54 UTC (permalink / raw)
To: linux-arm-kernel
If the ICST clock has a parent, respect the rate of the parent
when calculating the clock frequency. As this involves modifying
the ICST parameter struct, make a cloned copy (the divisor
arrays should be safe) so we can update the .ref field.
Do not define the reference clock on the Integrator as we have
the reference clock from the device tree. Keep it everywhere
else.
Cc: Mike Turquette <mturquette@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- As Russell observed: the params are static objects, so if
we want to alter them, we better make a copy of them.
Hi Mike, looking for an ACK for this one as well.
---
drivers/clk/versatile/clk-icst.c | 20 ++++++++++++++++----
drivers/clk/versatile/clk-integrator.c | 1 -
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/versatile/clk-icst.c b/drivers/clk/versatile/clk-icst.c
index c98adbe62733..a820b0cfcf57 100644
--- a/drivers/clk/versatile/clk-icst.c
+++ b/drivers/clk/versatile/clk-icst.c
@@ -33,7 +33,7 @@ struct clk_icst {
struct clk_hw hw;
void __iomem *vcoreg;
void __iomem *lockreg;
- const struct icst_params *params;
+ struct icst_params *params;
unsigned long rate;
};
@@ -84,6 +84,8 @@ static unsigned long icst_recalc_rate(struct clk_hw *hw,
struct clk_icst *icst = to_icst(hw);
struct icst_vco vco;
+ if (parent_rate)
+ icst->params->ref = parent_rate;
vco = vco_get(icst->vcoreg);
icst->rate = icst_hz(icst->params, vco);
return icst->rate;
@@ -105,6 +107,8 @@ static int icst_set_rate(struct clk_hw *hw, unsigned long rate,
struct clk_icst *icst = to_icst(hw);
struct icst_vco vco;
+ if (parent_rate)
+ icst->params->ref = parent_rate;
vco = icst_hz_to_vco(icst->params, rate);
icst->rate = icst_hz(icst->params, vco);
vco_set(icst->lockreg, icst->vcoreg, vco);
@@ -126,19 +130,27 @@ struct clk *icst_clk_register(struct device *dev,
struct clk *clk;
struct clk_icst *icst;
struct clk_init_data init;
+ struct icst_params *pclone;
icst = kzalloc(sizeof(struct clk_icst), GFP_KERNEL);
if (!icst) {
pr_err("could not allocate ICST clock!\n");
return ERR_PTR(-ENOMEM);
}
+
+ pclone = kmemdup(desc->params, sizeof(*pclone), GFP_KERNEL);
+ if (!pclone) {
+ pr_err("could not clone ICST params\n");
+ return ERR_PTR(-ENOMEM);
+ }
+
init.name = name;
init.ops = &icst_ops;
init.flags = CLK_IS_ROOT;
- init.parent_names = NULL;
- init.num_parents = 0;
+ init.parent_names = (parent_name ? &parent_name : NULL);
+ init.num_parents = (parent_name ? 1 : 0);
icst->hw.init = &init;
- icst->params = desc->params;
+ icst->params = pclone;
icst->vcoreg = base + desc->vco_offset;
icst->lockreg = base + desc->lock_offset;
diff --git a/drivers/clk/versatile/clk-integrator.c b/drivers/clk/versatile/clk-integrator.c
index 5d36a719fefb..734c4b8fe6ab 100644
--- a/drivers/clk/versatile/clk-integrator.c
+++ b/drivers/clk/versatile/clk-integrator.c
@@ -21,7 +21,6 @@
static void __iomem *cm_base;
static const struct icst_params cp_auxosc_params = {
- .ref = 24000000,
.vco_max = ICST525_VCO_MAX_5V,
.vco_min = ICST525_VCO_MIN,
.vd_min = 8,
--
1.8.4.2
^ permalink raw reply related
* [PATCH v2 02/11] iommu/arm-smmu: Introduce iommu_group notifier block
From: Andreas Herrmann @ 2014-01-20 21:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140120215351.GG3471@alberich>
On Mon, Jan 20, 2014 at 10:53:51PM +0100, Andreas Herrmann wrote:
>
> At the moment just handle BUS_NOTIFY_BIND_DRIVER to conditionally
> isolate all master devices for an SMMU.
Oops, sent out too early. Please ignore this crap.
(An iommu_group notifier that is handling BUS_NOTIFY_BIND_DRIVER ...)
Andreas
^ permalink raw reply
* Nomadik clk not MULTI_V5 friendly
From: Andrew Lunn @ 2014-01-20 22:01 UTC (permalink / raw)
To: linux-arm-kernel
Hi Linus
drivers/clk/clk-nomadik.c:
static int __init nomadik_src_clk_init_debugfs(void)
{
src_pcksr0_boot = readl(src_base + SRC_PCKSR0);
src_pcksr1_boot = readl(src_base + SRC_PCKSR1);
debugfs_create_file("nomadik-src-clk", S_IFREG | S_IRUGO,
NULL, NULL, &nomadik_src_clk_debugfs_ops);
return 0;
}
module_init(nomadik_src_clk_init_debugfs);
It does not check if it is actually running on a Nomadik, so when this
module_init() is executed on kirkwood as part of a MULTI_V5 kernel,
bad things happen. Please could you add the necessary check.
Thanks
Andrew
^ permalink raw reply
* [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block
From: Andreas Herrmann @ 2014-01-20 22:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1389876263-25759-3-git-send-email-andreas.herrmann@calxeda.com>
At the moment just handle IOMMU_GROUP_NOTIFY_BIND_DRIVER to
conditionally isolate all master devices for an SMMU.
Depending on DT information each device is put into its own protection
domain (if possible). For configuration with one or just a few
masters per SMMU that is easy to achieve.
In case of many devices per SMMU (e.g. MMU-500 with it's distributed
translation support) isolation of each device might not be possible --
depending on number of available SMR groups and/or context banks.
Default is that device isolation is contolled per SMMU with SMMU node
property "arm,smmu-isolate-devices" in a DT. If this property is set
for an SMMU node, device isolation is performed.
W/o device isolation the driver detects SMMUs but no translation is
configured (transactions just bypass translation process).
Note that for device isolation dma_base and size are fixed as 0 and
SZ_128M at the moment. Additional patches will address this
restriction and allow automatic growth of mapping size.
Cc: Varun Sethi <Varun.Sethi@freescale.com>
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
drivers/iommu/arm-smmu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
Hi Will,
This new patch addresses Varun's comments:
- use iommu_group notifier instead of bus notifier
- remove superfluous call to arm_smmu_add_device in
notifier function
This patch depends on commit "iommu/arm-smmu: add devices attached to
the SMMU to an IOMMU group" as found in your git tree (e.g. in branch
iommu/devel or for-joerg/arm-smmu/updates).
Andreas
PS: This time with a proper adaption of the notifier function.
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 0a5649f..da19bd6 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -46,6 +46,7 @@
#include <linux/amba/bus.h>
#include <asm/pgalloc.h>
+#include <asm/dma-iommu.h>
/* Driver options */
#define ARM_SMMU_OPT_ISOLATE_DEVICES (1 << 0)
@@ -1517,6 +1518,47 @@ static int arm_smmu_domain_has_cap(struct iommu_domain *domain,
return !!(cap & caps);
}
+static int arm_smmu_group_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+ struct dma_iommu_mapping *mapping;
+ struct arm_smmu_device *smmu;
+ int ret;
+
+ switch (action) {
+ case IOMMU_GROUP_NOTIFY_BIND_DRIVER:
+
+ smmu = dev->archdata.iommu;
+ if (!smmu || !(smmu->options & ARM_SMMU_OPT_ISOLATE_DEVICES))
+ break;
+
+ mapping = arm_iommu_create_mapping(&platform_bus_type,
+ 0, SZ_128M, 0);
+ if (IS_ERR(mapping)) {
+ ret = PTR_ERR(mapping);
+ dev_info(dev, "arm_iommu_create_mapping failed\n");
+ break;
+ }
+
+ ret = arm_iommu_attach_device(dev, mapping);
+ if (ret < 0) {
+ dev_info(dev, "arm_iommu_attach_device failed\n");
+ arm_iommu_release_mapping(mapping);
+ }
+
+ break;
+ default:
+ break;
+ }
+
+ return 0;
+}
+
+static struct notifier_block group_nb = {
+ .notifier_call = arm_smmu_group_notifier,
+};
+
static int arm_smmu_add_device(struct device *dev)
{
struct arm_smmu_device *child, *parent, *smmu;
@@ -1566,6 +1608,8 @@ static int arm_smmu_add_device(struct device *dev)
return PTR_ERR(group);
}
+ iommu_group_register_notifier(group, &group_nb);
+
ret = iommu_group_add_device(group, dev);
iommu_group_put(group);
dev->archdata.iommu = smmu;
--
1.7.9.5
^ permalink raw reply related
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
From: Grant Likely @ 2014-01-20 22:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140115161224.GH25824@e106331-lin.cambridge.arm.com>
On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > Another, more invasive option would be extend the dts syntax and teach
> > dtc to handle property appending. Then the soc dts could stay as it is,
> > and the board dts could have something like:
> >
> > /append-property/ interrupts = <&intc1 6 1>;
> > /append-property/ interrupt-names = "board-specific-irq";
> >
> > Both these options solve the issue at the source, are general to other
> > properties, and allow more than one level of hierarchy (the proposed
> > interrupts-extended-2 only allows one level).
>
> I've just had a go at implementing the append-property mechanism above
> in dtc, and it was far easier than I expected (patch below).
>
> Does anyone have any issues with the /append-property/ idea?
I think that is reasonable.
g.
>
> Thanks,
> Mark.
>
> ---->8----
>
> From 88be0036b6a966bd7506f58e3cb9ce9ea4c5ac48 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland@arm.com>
> Date: Wed, 15 Jan 2014 15:43:51 +0000
> Subject: [PATCH] dtc: add ability to append properties
>
> When dealing with hierarchies of dtsi files, handling minute differences
> between individual boards can become very painful. Adding a single
> board-specific interrupt requires duplicating the entire interrupts
> property, which requires duplication of common values. This makes bug
> fixing painful and if not handled very carefully files diverge rapdily.
>
> To ameliorate this, this patch adds the ability to append properties,
> allowing board files to describe only the additional values required in
> a property. This functionality also works with strings, so parallel
> properties like interupts and interrupt-names stay in sync. Properties
> may be appended multiple times, and deleting properties clears all
> previous appended values.
>
> To append a property, secondary definitions must be prefixed with
> /append-property/. This is longer than a possible '+=' syntax, but makes
> it far easier to spot when appending behaviour is requested, and so
> hopefully will lead to fewer buggy dts files.
>
> For example, if the following dts fragements are compiled together:
>
> / {
> interrupts = <0>, <1>;
> interrupt-names = "zero", "one";
> };
>
> / {
> /append-property/ interrupts = <2>, <3>;
> /append-property/ interrupt-names = "two", "three";
> };
>
> They will result in a dtb equivalent to the following dts fragment:
>
> / {
> interrupts = <0>, <1>, <2>, <3>;
> interrupt-names = "zero", "one", "two", "three";
> };
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
> dtc-lexer.l | 7 +++++++
> dtc-parser.y | 5 +++++
> dtc.h | 2 ++
> livetree.c | 15 ++++++++++++++-
> 4 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 0cd7e67..7989abe 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -131,6 +131,13 @@ static bool pop_input_file(void);
> return DT_DEL_PROP;
> }
>
> +<*>"/append-property/" {
> + DPRINT("Keyword: /append-property/\n");
> + DPRINT("<PROPNODENAME>\n");
> + BEGIN(PROPNODENAME);
> + return DT_APPEND_PROP;
> + }
> +
> <*>"/delete-node/" {
> DPRINT("Keyword: /delete-node/\n");
> DPRINT("<PROPNODENAME>\n");
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 4864631..a8409ed 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -63,6 +63,7 @@ static unsigned char eval_char_literal(const char *s);
> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
> %token DT_BITS
> %token DT_DEL_PROP
> +%token DT_APPEND_PROP
> %token DT_DEL_NODE
> %token <propnodename> DT_PROPNODENAME
> %token <literal> DT_LITERAL
> @@ -195,6 +196,10 @@ propdef:
> {
> $$ = build_property($1, empty_data);
> }
> + | DT_APPEND_PROP DT_PROPNODENAME '=' propdata ';'
> + {
> + $$ = build_property_append($2, $4);
> + }
> | DT_DEL_PROP DT_PROPNODENAME ';'
> {
> $$ = build_property_delete($2);
> diff --git a/dtc.h b/dtc.h
> index 20e4d56..8687530 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -133,6 +133,7 @@ struct label {
> };
>
> struct property {
> + bool appended;
> bool deleted;
> char *name;
> struct data val;
> @@ -186,6 +187,7 @@ void delete_labels(struct label **labels);
>
> struct property *build_property(char *name, struct data val);
> struct property *build_property_delete(char *name);
> +struct property *build_property_append(char *name, struct data val);
> struct property *chain_property(struct property *first, struct property *list);
> struct property *reverse_properties(struct property *first);
>
> diff --git a/livetree.c b/livetree.c
> index b61465f..894e42b 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -74,6 +74,15 @@ struct property *build_property_delete(char *name)
> return new;
> }
>
> +struct property *build_property_append(char *name, struct data val)
> +{
> + struct property *new = build_property(name, val);
> +
> + new->appended = 1;
> +
> + return new;
> +}
> +
> struct property *chain_property(struct property *first, struct property *list)
> {
> assert(first->next == NULL);
> @@ -167,7 +176,11 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
> for_each_label_withdel(new_prop->labels, l)
> add_label(&old_prop->labels, l->label);
>
> - old_prop->val = new_prop->val;
> + if (new_prop->appended)
> + old_prop->val = data_merge(old_prop->val, new_prop->val);
> + else
> + old_prop->val = new_prop->val;
> +
> old_prop->deleted = 0;
> free(new_prop);
> new_prop = NULL;
> --
> 1.8.1.1
>
^ permalink raw reply
* [PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI
From: Laurent Pinchart @ 2014-01-20 22:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140120155643.GQ17314@sirena.org.uk>
Hi Mark,
On Monday 20 January 2014 15:56:43 Mark Brown wrote:
> On Mon, Jan 20, 2014 at 04:48:10PM +0100, Laurent Pinchart wrote:
> > The problem isn't as simple as it seems, and more advanced implementations
> > that would allow listing clocks that should be managed automatically (or
> > the other way around) would also add another level of complexity. The
> > required information is platform-dependent, but we currently don't
> > express it as such in DT.
>
> Well, the set of clocks an IP requires will tend to be the same - it's
> normally just that integrators may have done things like tie them together
> or decide to spread confusion by renaming them.
That's the problem :-) How should the runtime PM core be given the list of
clocks it needs to manage ? That information needs to come from somewhere.
--
Regards,
Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140120/a2303b43/attachment.sig>
^ permalink raw reply
* [PATCH v2 1/2] dmaengine: add Qualcomm BAM dma driver
From: Andy Gross @ 2014-01-20 22:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201401172349.28229.arnd@arndb.de>
On Fri, Jan 17, 2014 at 11:49:27PM +0100, Arnd Bergmann wrote:
> On Friday 10 January 2014, Andy Gross wrote:
>
> > +static bool bam_dma_filter(struct dma_chan *chan, void *data)
> > +{
> > + struct bam_filter_args *args = data;
> > + struct bam_chan *bchan = to_bam_chan(chan);
> > +
> > + if (args->dev == chan->device &&
> > + args->id == bchan->id) {
> > +
> > + /* we found the channel, so lets set the EE and dir */
> > + bchan->ee = args->ee;
> > + bchan->slave.direction = args->dir ?
> > + DMA_DEV_TO_MEM : DMA_MEM_TO_DEV;
> > + return true;
> > + }
> > +
> > + return false;
> > +}
>
> A filter function should no longer be needed.
>
Ok, will remove.
> > +static struct dma_chan *bam_dma_xlate(struct of_phandle_args *dma_spec,
> > + struct of_dma *of)
> > +{
> > + struct bam_filter_args args;
> > + dma_cap_mask_t cap;
> > +
> > + if (dma_spec->args_count != 3)
> > + return NULL;
> > +
> > + args.dev = of->of_dma_data;
> > + args.id = dma_spec->args[0];
> > + args.ee = dma_spec->args[1];
> > + args.dir = dma_spec->args[2];
> > +
> > + dma_cap_zero(cap);
> > + dma_cap_set(DMA_SLAVE, cap);
> > +
> > + return dma_request_channel(cap, bam_dma_filter, &args);
> > +}
>
> Instead, call dma_get_slave_channel() with the right channel that you already know
> here.
>
Agreed.
> > + if (pdev->dev.of_node) {
> > + ret = of_dma_controller_register(pdev->dev.of_node,
> > + bam_dma_xlate, &bdev->common);
> > +
> > + if (ret) {
> > + dev_err(bdev->dev, "failed to register of_dma\n");
> > + goto err_unregister_dma;
> > + }
> > + }
>
> No need to check for pdev->dev.of_node when that is the only mode of probing.
>
Good point. I'll remove extraneous check.
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id bam_of_match[] = {
> > + { .compatible = "qcom,bam-v1.4.0", },
> > + { .compatible = "qcom,bam-v1.4.1", },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, bam_of_match);
> > +#endif
>
> Also, you can remove the #ifdef here and the of_match_ptr() below.
>
If this is removed, then I'll have to add the OF dependency in the Kconfig,
correct?
> > +
> > +static struct platform_driver bam_dma_driver = {
> > + .probe = bam_dma_probe,
> > + .remove = bam_dma_remove,
> > + .driver = {
> > + .name = "bam-dma-engine",
> > + .owner = THIS_MODULE,
> > + .of_match_table = of_match_ptr(bam_of_match),
> > + },
> > +};
> > +
> > +static int __init bam_dma_init(void)
> > +{
> > + return platform_driver_register(&bam_dma_driver);
> > +}
> > +
> > +static void __exit bam_dma_exit(void)
> > +{
> > + return platform_driver_unregister(&bam_dma_driver);
> > +}
> > +
>
> module_platform_driver()
>
Will fix.
> > diff --git a/drivers/dma/qcom_bam_dma.h b/drivers/dma/qcom_bam_dma.h
> > new file mode 100644
> > index 0000000..2cb3b5f
> > --- /dev/null
> > +++ b/drivers/dma/qcom_bam_dma.h
> > @@ -0,0 +1,268 @@
> > +#ifndef __QCOM_BAM_DMA_H__
> > +#define __QCOM_BAM_DMA_H__
> > +
> > +#include <linux/dmaengine.h>
> > +#include "virt-dma.h"
> > +
> > +enum bam_channel_dir {
> > + BAM_PIPE_CONSUMER = 0, /* channel reads from data-fifo or memory */
> > + BAM_PIPE_PRODUCER, /* channel writes to data-fifo or memory */
> > +};
>
> Since the header does not serve as an interface, just move all the contents
> into the driver directly.
>
OK. SBoyd made the same comment. I'll go ahead and collapse both down to 1
file.
> > +struct bam_desc_hw {
> > + u32 addr; /* Buffer physical address */
> > + u16 size; /* Buffer size in bytes */
> > + u16 flags;
> > +} __packed;
>
> Remove __packed here, it only makes the access less efficient but does not change
> the layout, which is already packed.
Ok. Will fix.
--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply
* [PATCH v2 1/2] dmaengine: add Qualcomm BAM dma driver
From: Andy Gross @ 2014-01-20 23:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140114194348.GH14405@codeaurora.org>
On Tue, Jan 14, 2014 at 11:43:48AM -0800, Stephen Boyd wrote:
> (Mostly nitpicks)
>
> On 01/10, Andy Gross wrote:
> > Add the DMA engine driver for the QCOM Bus Access Manager (BAM) DMA controller
> > found in the MSM 8x74 platforms.
> >
> > Each BAM DMA device is associated with a specific on-chip peripheral. Each
> > channel provides a uni-directional data transfer engine that is capable of
> > transferring data between the peripheral and system memory (System mode), or
> > between two peripherals (BAM2BAM).
> >
> > The initial release of this driver only supports slave transfers between
> > peripherals and system memory.
> >
> > Signed-off-by: Andy Gross <agross@codeaurora.org>
> > ---
> > drivers/dma/Kconfig | 9 +
> > drivers/dma/Makefile | 1 +
> > drivers/dma/qcom_bam_dma.c | 843 +++++++++++++++++++++++++++++++++++++++++++++
> > drivers/dma/qcom_bam_dma.h | 268 ++++++++++++++
> > 4 files changed, 1121 insertions(+)
> > create mode 100644 drivers/dma/qcom_bam_dma.c
> > create mode 100644 drivers/dma/qcom_bam_dma.h
> >
> > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> > index c823daa..e58e6d2 100644
> > --- a/drivers/dma/Kconfig
> > +++ b/drivers/dma/Kconfig
> > @@ -384,4 +384,13 @@ config DMATEST
> > config DMA_ENGINE_RAID
> > bool
> >
> > +config QCOM_BAM_DMA
> > + tristate "QCOM BAM DMA support"
> > + depends on ARCH_MSM || COMPILE_TEST
>
> I don't think writel_relaxed() is available on every arch, so
> it's possible this will break some random arch that doesn't have
> that function.
>
I'll look into this to see. If that's the case, I can remove the COMPILE_TEST
if there is no alternative.
> > + select DMA_ENGINE
> > + select DMA_VIRTUAL_CHANNELS
> > + ---help---
> > + Enable support for the QCOM BAM DMA controller. This controller
> > + provides DMA capabilities for a variety of on-chip devices.
> > +
> > endif
> > diff --git a/drivers/dma/qcom_bam_dma.c b/drivers/dma/qcom_bam_dma.c
> > new file mode 100644
> > index 0000000..7a84b02
> > --- /dev/null
> > +++ b/drivers/dma/qcom_bam_dma.c
> [...]
> > +static int bam_alloc_chan(struct dma_chan *chan)
> [...]
> > +
> > + /* Go ahead and initialize the pipe/channel hardware here
> > + - Reset the channel to clear internal state of the FIFO
> > + - Program in the FIFO address
> > + - Configure the irq based on the EE passed in from the DT entry
> > + - Set mode, direction and enable channel
> > +
> > + We do this here because the channel can only be enabled once and
> > + can only be disabled via a reset. If done here, we don't have to
> > + manage additional state to figure out when to do this
> > + */
>
> Multi-line comments are of the form:
>
> /*
> * comment
> */
>
Right. I converted some comments and didn't do the correct multi-line
> > +
> > + bam_reset_channel(bdev, bchan->id);
> > +
> > + /* write out 8 byte aligned address. We have enough space for this
> > + because we allocated 1 more descriptor (8 bytes) than we can use */
> > + writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
> > + bdev->regs + BAM_P_DESC_FIFO_ADDR(bchan->id));
> > + writel_relaxed(BAM_DESC_FIFO_SIZE, bdev->regs +
> > + BAM_P_FIFO_SIZES(bchan->id));
> [...]
> > +
> > +/**
> > + * bam_dma_terminate_all - terminate all transactions
> > + * @chan: dma channel
> > + *
> > + * Idles channel and dequeues and frees all transactions
> > + * No callbacks are done
> > + *
> > +*/
>
> Weird '*' starting the line here and on the next function.
>
Will fix.
> > +static void bam_dma_terminate_all(struct dma_chan *chan)
> > +{
> > + struct bam_chan *bchan = to_bam_chan(chan);
> > + struct bam_device *bdev = bchan->bdev;
> > +
> > + bam_reset_channel(bdev, bchan->id);
> > +
> > + vchan_free_chan_resources(&bchan->vc);
> > +}
> > +
> > +/**
> > + * bam_control - DMA device control
> > + * @chan: dma channel
> > + * @cmd: control cmd
> > + * @arg: cmd argument
> > + *
> > + * Perform DMA control command
> > + *
> > +*/
> > +static int bam_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> > + unsigned long arg)
> > +{
> > + struct bam_chan *bchan = to_bam_chan(chan);
> > + struct bam_device *bdev = bchan->bdev;
> > + int ret = 0;
> > + unsigned long flag;
> > +
> [...]
> > +/**
> > + * bam_dma_irq - irq handler for bam controller
> > + * @irq: IRQ of interrupt
> > + * @data: callback data
> > + *
> > + * IRQ handler for the bam controller
> > + */
> > +static irqreturn_t bam_dma_irq(int irq, void *data)
> > +{
> > + struct bam_device *bdev = (struct bam_device *)data;
>
> Unnecessary cast from void.
>
Fixed.
> > +static int bam_dma_probe(struct platform_device *pdev)
> > +{
> [...]
> > +
> > + irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > + if (!irq_res) {
> > + dev_err(bdev->dev, "irq resource is missing\n");
> > + return -EINVAL;
> > + }
>
> Please use platform_get_irq() instead.
>
Fixed.
> > diff --git a/drivers/dma/qcom_bam_dma.h b/drivers/dma/qcom_bam_dma.h
> > new file mode 100644
> > index 0000000..2cb3b5f
> > --- /dev/null
> > +++ b/drivers/dma/qcom_bam_dma.h
> [...]
> > +#define BAM_CNFG_BITS_DEFAULT (BAM_PIPE_CNFG | \
> > + BAM_NO_EXT_P_RST | \
> > + BAM_IBC_DISABLE | \
> > + BAM_SB_CLK_REQ | \
> > + BAM_PSM_CSW_REQ | \
> > + BAM_PSM_P_RES | \
> > + BAM_AU_P_RES | \
> > + BAM_SI_P_RES | \
> > + BAM_WB_P_RES | \
> > + BAM_WB_BLK_CSW | \
> > + BAM_WB_CSW_ACK_IDL | \
> > + BAM_WB_RETR_SVPNT | \
> > + BAM_WB_DSC_AVL_P_RST | \
> > + BAM_REG_P_EN | \
> > + BAM_PSM_P_HD_DATA | \
> > + BAM_AU_ACCUMED | \
> > + BAM_CMD_ENABLE)
> > +
> > +/* PIPE CTRL */
> > +#define P_EN BIT(1)
>
> Nit: Weird formatting here?
>
That is odd. Will fix.
> > +#define P_DIRECTION BIT(3)
> [...]
> > +
> > +
> > +struct bam_device {
> > + void __iomem *regs;
> > + struct device *dev;
> > + struct dma_device common;
> > + struct device_dma_parameters dma_parms;
> > + struct bam_chan *channels;
>
> Maybe this should be a flexible array. It looks like probe might
> need to be rewritten to detect the number of channels from the
> hardware before assigning anything, but it should be possible.
> But it probably doesn't matter.
>
You can't take the number of channels at face value. Only a subset of that
number are actually usable by the CPUs execution environment.
> > + u32 num_channels;
> > + u32 num_ees;
> > + unsigned long enabled_ees;
> > + int irq;
>
> Is irq used?
>
Will remove.
> > + struct clk *bamclk;
> > +
> > + /* dma start transaction tasklet */
> > + struct tasklet_struct task;
> > +};
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply
* [PATCH] mmc: omap_hsmmc: Add support for Erratum 2.1.1.128 in device tree boot
From: Nishanth Menon @ 2014-01-20 23:29 UTC (permalink / raw)
To: linux-arm-kernel
When device is booted using devicetree, platforms impacted by
Erratum 2.1.1.128 is not detected easily in the mmc driver. This erratum
indicates that the module cannot do multi-block transfers.
Handle this by providing a boolean flag to indicate to driver that it is
working on a hardware with mentioned limitation.
Signed-off-by: Nishanth Menon <nm@ti.com>
---
This explains the logs I see:
OMAP3430 LDP (ES2.2):
uImage only boot: http://slexy.org/raw/s2YrbMAi7c
uImage+dtb concatenated boot: http://slexy.org/raw/s20qVg17T0
With the following flag set, device is now able to consistently boot with
device tree supported uImage+dtb concat boot.
.../devicetree/bindings/mmc/ti-omap-hsmmc.txt | 2 ++
drivers/mmc/host/omap_hsmmc.c | 3 +++
2 files changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
index 8c8908a..ab36f8b 100644
--- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
+++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
@@ -26,6 +26,8 @@ specifier is required.
dma-names: List of DMA request names. These strings correspond
1:1 with the DMA specifiers listed in dmas. The string naming is
to be "rx" and "tx" for RX and TX DMA requests, respectively.
+ti,erratum-2.1.1.128: boolean, for OMAP3430/OMAP35xx platforms with broken
+multiblock reads
Examples:
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 014bfe5..f2d5940 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1730,6 +1730,9 @@ static struct omap_mmc_platform_data *of_get_hsmmc_pdata(struct device *dev)
if (of_find_property(np, "ti,dual-volt", NULL))
pdata->controller_flags |= OMAP_HSMMC_SUPPORTS_DUAL_VOLT;
+ if (of_find_property(np, "ti,erratum-2.1.1.128", NULL))
+ pdata->controller_flags |= OMAP_HSMMC_BROKEN_MULTIBLOCK_READ;
+
/* This driver only supports 1 slot */
pdata->nr_slots = 1;
pdata->slots[0].switch_pin = cd_gpio;
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 1/2] dmaengine: add Qualcomm BAM dma driver
From: Andy Gross @ 2014-01-20 23:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1389609060.1871.312.camel@smile>
On Mon, Jan 13, 2014 at 10:31:01AM +0000, Shevchenko, Andriy wrote:
> On Fri, 2014-01-10 at 13:07 -0600, Andy Gross wrote:
> > Add the DMA engine driver for the QCOM Bus Access Manager (BAM) DMA controller
> > found in the MSM 8x74 platforms.
> >
> > Each BAM DMA device is associated with a specific on-chip peripheral. Each
> > channel provides a uni-directional data transfer engine that is capable of
> > transferring data between the peripheral and system memory (System mode), or
> > between two peripherals (BAM2BAM).
> >
> > The initial release of this driver only supports slave transfers between
> > peripherals and system memory.
> >
> > Signed-off-by: Andy Gross <agross@codeaurora.org>
> > ---
> > drivers/dma/Kconfig | 9 +
> > drivers/dma/Makefile | 1 +
> > drivers/dma/qcom_bam_dma.c | 843 +++++++++++++++++++++++++++++++++++++++++++++
> > drivers/dma/qcom_bam_dma.h | 268 ++++++++++++++
> > 4 files changed, 1121 insertions(+)
> > create mode 100644 drivers/dma/qcom_bam_dma.c
> > create mode 100644 drivers/dma/qcom_bam_dma.h
> >
[...]
> > + * bam_tx_status - returns status of transaction
> > + * @chan: dma channel
> > + * @cookie: transaction cookie
> > + * @txstate: DMA transaction state
> > + *
> > + * Return status of dma transaction
> > + */
> > +static enum dma_status bam_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> > + struct dma_tx_state *txstate)
> > +{
> > + struct bam_chan *bchan = to_bam_chan(chan);
> > + struct virt_dma_desc *vd;
> > + int ret;
> > + size_t residue = 0;
> > + unsigned int i;
> > + unsigned long flags;
> > +
> > + ret = dma_cookie_status(chan, cookie, txstate);
> > +
>
> Redundant empty line.
>
Will remove.
> > + if (ret == DMA_COMPLETE)
> > + return ret;
> > +
> > + if (!txstate)
> > + return bchan->paused ? DMA_PAUSED : ret;
> > +
> > + spin_lock_irqsave(&bchan->vc.lock, flags);
> > + vd = vchan_find_desc(&bchan->vc, cookie);
> > + if (vd)
> > + residue = container_of(vd, struct bam_async_desc, vd)->length;
> > + else if (bchan->curr_txd && bchan->curr_txd->vd.tx.cookie == cookie)
> > + for (i = 0; i < bchan->curr_txd->num_desc; i++)
> > + residue += bchan->curr_txd->curr_desc[i].size;
> > +
> > + dma_set_residue(txstate, residue);
>
> I'm pretty sure you could do this outside of spin lock.
>
Yes, I'll move it.
> > +
> > + spin_unlock_irqrestore(&bchan->vc.lock, flags);
> > +
> > + if (ret == DMA_IN_PROGRESS && bchan->paused)
> > + ret = DMA_PAUSED;
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * bam_start_dma - start next transaction
> > + * @bchan - bam dma channel
> > + *
> > + * Note: must hold bam dma channel vc.lock
> > + */
> > +static void bam_start_dma(struct bam_chan *bchan)
> > +{
> > + struct virt_dma_desc *vd = vchan_next_desc(&bchan->vc);
> > + struct bam_device *bdev = bchan->bdev;
> > + struct bam_async_desc *async_desc;
> > + struct bam_desc_hw *desc;
> > + struct bam_desc_hw *fifo = PTR_ALIGN(bchan->fifo_virt,
> > + sizeof(struct bam_desc_hw));
>
> > +
> > + if (!vd)
> > + return;
> > +
> > + list_del(&vd->node);
> > +
> > + async_desc = container_of(vd, struct bam_async_desc, vd);
> > + bchan->curr_txd = async_desc;
> > +
> > + desc = bchan->curr_txd->curr_desc;
> > +
> > + if (async_desc->num_desc > MAX_DESCRIPTORS)
> > + async_desc->xfer_len = MAX_DESCRIPTORS;
> > + else
> > + async_desc->xfer_len = async_desc->num_desc;
> > +
> > + /* set INT on last descriptor */
> > + desc[async_desc->xfer_len - 1].flags |= DESC_FLAG_INT;
> > +
> > + if (bchan->tail + async_desc->xfer_len > MAX_DESCRIPTORS) {
> > + u32 partial = MAX_DESCRIPTORS - bchan->tail;
> > +
> > + memcpy(&fifo[bchan->tail], desc,
> > + partial * sizeof(struct bam_desc_hw));
> > + memcpy(fifo, &desc[partial], (async_desc->xfer_len - partial) *
> > + sizeof(struct bam_desc_hw));
>
> I'm just curious if you could avoid memcpys at all somehow.
>
Unfortunately not. The descriptors have to be copied into the FIFO memory that
is being used by the dma controller for this channel. Due to the way the FIFO
works, I have to copy into the FIFO during either the issue_pending or when I
start the next transaction. Either way, it means copying from the txd to the
FIFO.
> > + } else
>
> Keep style
>
OK.
> } else {
> ...
> }
>
> Have you run checkpatch.pl?
>
Yes. And I fixed any discrepancies before sending this.
> > + memcpy(&fifo[bchan->tail], desc,
> > + async_desc->xfer_len * sizeof(struct bam_desc_hw));
> > +
> > + bchan->tail += async_desc->xfer_len;
> > + bchan->tail %= MAX_DESCRIPTORS;
> > +
> > + /* ensure descriptor writes and dma start not reordered */
> > + wmb();
> > + writel_relaxed(bchan->tail * sizeof(struct bam_desc_hw),
> > + bdev->regs + BAM_P_EVNT_REG(bchan->id));
> > +}
> > +
> > +/**
> > + * dma_tasklet - DMA IRQ tasklet
> > + * @data: tasklet argument (bam controller structure)
> > + *
> > + * Sets up next DMA operation and then processes all completed transactions
> > + */
> > +static void dma_tasklet(unsigned long data)
> > +{
> > + struct bam_device *bdev = (struct bam_device *)data;
> > + struct bam_chan *bchan;
> > + unsigned long flags;
> > + unsigned int i;
> > +
> > + /* go through the channels and kick off transactions */
> > + for (i = 0; i < bdev->num_channels; i++) {
> > + bchan = &bdev->channels[i];
> > + spin_lock_irqsave(&bchan->vc.lock, flags);
> > +
> > + if (!list_empty(&bchan->vc.desc_issued) && !bchan->curr_txd)
> > + bam_start_dma(bchan);
> > + spin_unlock_irqrestore(&bchan->vc.lock, flags);
> > + }
> > +}
> > +
> > +/**
> > + * bam_issue_pending - starts pending transactions
> > + * @chan: dma channel
> > + *
> > + * Calls tasklet directly which in turn starts any pending transactions
> > + */
> > +static void bam_issue_pending(struct dma_chan *chan)
> > +{
> > + struct bam_chan *bchan = to_bam_chan(chan);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&bchan->vc.lock, flags);
> > +
> > + /* if work pending and idle, start a transaction */
> > + if (vchan_issue_pending(&bchan->vc) && !bchan->curr_txd)
> > + bam_start_dma(bchan);
> > +
> > + spin_unlock_irqrestore(&bchan->vc.lock, flags);
> > +}
> > +
> > +/**
> > + * bam_dma_free_desc - free descriptor memory
> > + * @vd: virtual descriptor
> > + *
> > + */
> > +static void bam_dma_free_desc(struct virt_dma_desc *vd)
> > +{
> > + struct bam_async_desc *async_desc = container_of(vd,
> > + struct bam_async_desc, vd);
> > +
> > + kfree(async_desc);
> > +}
> > +
> > +struct bam_filter_args {
> > + struct dma_device *dev;
> > + u32 id;
> > + u32 ee;
> > + u32 dir;
> > +};
> > +
> > +static bool bam_dma_filter(struct dma_chan *chan, void *data)
> > +{
> > + struct bam_filter_args *args = data;
> > + struct bam_chan *bchan = to_bam_chan(chan);
> > +
> > + if (args->dev == chan->device &&
> > + args->id == bchan->id) {
> > +
> > + /* we found the channel, so lets set the EE and dir */
> > + bchan->ee = args->ee;
> > + bchan->slave.direction = args->dir ?
> > + DMA_DEV_TO_MEM : DMA_MEM_TO_DEV;
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static struct dma_chan *bam_dma_xlate(struct of_phandle_args *dma_spec,
> > + struct of_dma *of)
> > +{
> > + struct bam_filter_args args;
> > + dma_cap_mask_t cap;
> > +
> > + if (dma_spec->args_count != 3)
> > + return NULL;
> > +
> > + args.dev = of->of_dma_data;
> > + args.id = dma_spec->args[0];
> > + args.ee = dma_spec->args[1];
> > + args.dir = dma_spec->args[2];
> > +
> > + dma_cap_zero(cap);
> > + dma_cap_set(DMA_SLAVE, cap);
> > +
> > + return dma_request_channel(cap, bam_dma_filter, &args);
> > +}
> > +
> > +/**
> > + * bam_init
> > + * @bdev: bam device
> > + *
> > + * Initialization helper for global bam registers
> > + */
> > +static void bam_init(struct bam_device *bdev)
> > +{
> > + u32 val;
> > +
> > + /* read versioning information */
> > + val = readl_relaxed(bdev->regs + BAM_REVISION);
> > + bdev->num_ees = val & NUM_EES_MASK;
> > +
> > + val = readl_relaxed(bdev->regs + BAM_NUM_PIPES);
> > + bdev->num_channels = val & BAM_NUM_PIPES_MASK;
> > +
> > + /* s/w reset bam */
> > + /* after reset all pipes are disabled and idle */
> > + val = readl_relaxed(bdev->regs + BAM_CTRL);
> > + val |= BAM_SW_RST;
> > + writel_relaxed(val, bdev->regs + BAM_CTRL);
> > + val &= ~BAM_SW_RST;
> > + writel_relaxed(val, bdev->regs + BAM_CTRL);
> > +
> > + /* make sure previous stores are visible before enabling BAM */
> > + wmb();
> > +
> > + /* enable bam */
> > + val |= BAM_EN;
> > + writel_relaxed(val, bdev->regs + BAM_CTRL);
> > +
> > + /* set descriptor threshhold, start with 4 bytes */
> > + writel_relaxed(DEFAULT_CNT_THRSHLD, bdev->regs + BAM_DESC_CNT_TRSHLD);
> > +
> > + /* Enable default set of h/w workarounds, ie all except BAM_FULL_PIPE */
> > + writel_relaxed(BAM_CNFG_BITS_DEFAULT, bdev->regs + BAM_CNFG_BITS);
> > +
> > + /* enable irqs for errors */
> > + writel_relaxed(BAM_ERROR_EN | BAM_HRESP_ERR_EN,
> > + bdev->regs + BAM_IRQ_EN);
> > +}
> > +
> > +static void bam_channel_init(struct bam_device *bdev, struct bam_chan *bchan,
> > + u32 index)
> > +{
> > + bchan->id = index;
> > + bchan->bdev = bdev;
> > +
> > + vchan_init(&bchan->vc, &bdev->common);
> > + bchan->vc.desc_free = bam_dma_free_desc;
> > +
> > + bam_reset_channel(bdev, bchan->id);
> > +}
> > +
> > +static int bam_dma_probe(struct platform_device *pdev)
> > +{
> > + struct bam_device *bdev;
> > + struct resource *iores, *irq_res;
> > + int ret, i;
> > +
> > + bdev = devm_kzalloc(&pdev->dev, sizeof(*bdev), GFP_KERNEL);
> > + if (!bdev)
> > + return -ENOMEM;
> > +
> > + bdev->dev = &pdev->dev;
> > +
> > + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!iores) {
> > + dev_err(bdev->dev, "register resource is missing\n");
> > + return -EINVAL;
> > + }
>
> Useless check and messaging, devm_ioremap_resource will do this for you.
>
Will fix this along with the other resource comment.
> > +
> > + bdev->regs = devm_ioremap_resource(&pdev->dev, iores);
> > + if (IS_ERR(bdev->regs))
> > + return PTR_ERR(bdev->regs);
> > +
> > + irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > + if (!irq_res) {
> > + dev_err(bdev->dev, "irq resource is missing\n");
> > + return -EINVAL;
> > + }
> > +
> > + bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk");
> > + if (IS_ERR(bdev->bamclk))
> > + return PTR_ERR(bdev->bamclk);
> > +
> > + ret = clk_prepare_enable(bdev->bamclk);
> > + if (ret) {
> > + dev_err(bdev->dev, "failed to prepare/enable clock");
> > + return ret;
> > + }
> > +
> > + bam_init(bdev);
> > +
> > + tasklet_init(&bdev->task, dma_tasklet, (unsigned long)bdev);
> > +
> > + bdev->channels = devm_kzalloc(bdev->dev,
>
> devm_kcalloc.
>
Will fix.
> > + sizeof(*bdev->channels) * bdev->num_channels,
> > + GFP_KERNEL);
> > +
> > + if (!bdev->channels) {
> > + ret = -ENOMEM;
> > + goto err_disable_clk;
> > + }
> > +
> > + /* allocate and initialize channels */
> > + INIT_LIST_HEAD(&bdev->common.channels);
> > +
> > + for (i = 0; i < bdev->num_channels; i++)
> > + bam_channel_init(bdev, &bdev->channels[i], i);
> > +
> > + ret = devm_request_irq(bdev->dev, irq_res->start, bam_dma_irq,
> > + IRQF_TRIGGER_HIGH, "bam_dma", bdev);
> > + if (ret) {
> > + dev_err(bdev->dev, "cannot register IRQ\n");
> > + goto err_disable_clk;
> > + }
> > +
> > + /* set max dma segment size */
> > + bdev->common.dev = bdev->dev;
> > + bdev->common.dev->dma_parms = &bdev->dma_parms;
> > + ret = dma_set_max_seg_size(bdev->common.dev, BAM_MAX_DATA_SIZE);
> > + if (ret) {
> > + dev_err(bdev->dev, "cannot set maximum segment size\n");
> > + goto err_disable_clk;
> > + }
> > +
> > + platform_set_drvdata(pdev, bdev);
> > +
> > + /* set capabilities */
> > + dma_cap_zero(bdev->common.cap_mask);
> > + dma_cap_set(DMA_SLAVE, bdev->common.cap_mask);
> > +
> > + /* initialize dmaengine apis */
> > + bdev->common.device_alloc_chan_resources = bam_alloc_chan;
> > + bdev->common.device_free_chan_resources = bam_free_chan;
> > + bdev->common.device_prep_slave_sg = bam_prep_slave_sg;
> > + bdev->common.device_control = bam_control;
> > + bdev->common.device_issue_pending = bam_issue_pending;
> > + bdev->common.device_tx_status = bam_tx_status;
> > + bdev->common.dev = bdev->dev;
> > +
> > + ret = dma_async_device_register(&bdev->common);
> > + if (ret) {
> > + dev_err(bdev->dev, "failed to register dma async device\n");
> > + goto err_disable_clk;
> > + }
> > +
> > + if (pdev->dev.of_node) {
> > + ret = of_dma_controller_register(pdev->dev.of_node,
> > + bam_dma_xlate, &bdev->common);
> > +
> > + if (ret) {
> > + dev_err(bdev->dev, "failed to register of_dma\n");
> > + goto err_unregister_dma;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err_unregister_dma:
> > + dma_async_device_unregister(&bdev->common);
> > +err_disable_clk:
> > + clk_disable_unprepare(bdev->bamclk);
>
>
>
> > + return ret;
> > +}
> > +
> > +static int bam_dma_remove(struct platform_device *pdev)
> > +{
> > + struct bam_device *bdev = platform_get_drvdata(pdev);
> > +
> > + dma_async_device_unregister(&bdev->common);
> > +
> > + if (pdev->dev.of_node)
> > + of_dma_controller_free(pdev->dev.of_node);
> > +
> > + clk_disable_unprepare(bdev->bamclk);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id bam_of_match[] = {
> > + { .compatible = "qcom,bam-v1.4.0", },
> > + { .compatible = "qcom,bam-v1.4.1", },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, bam_of_match);
> > +#endif
> > +
> > +static struct platform_driver bam_dma_driver = {
> > + .probe = bam_dma_probe,
> > + .remove = bam_dma_remove,
> > + .driver = {
> > + .name = "bam-dma-engine",
> > + .owner = THIS_MODULE,
> > + .of_match_table = of_match_ptr(bam_of_match),
> > + },
> > +};
> > +
> > +static int __init bam_dma_init(void)
> > +{
> > + return platform_driver_register(&bam_dma_driver);
> > +}
> > +
> > +static void __exit bam_dma_exit(void)
> > +{
> > + return platform_driver_unregister(&bam_dma_driver);
> > +}
> > +
> > +module_init(bam_dma_init);
> > +module_exit(bam_dma_exit);
>
> module_platform_driver() ?
>
Will fix.
> > +
> > +MODULE_AUTHOR("Andy Gross <agross@codeaurora.org>");
> > +MODULE_DESCRIPTION("QCOM BAM DMA engine driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/dma/qcom_bam_dma.h b/drivers/dma/qcom_bam_dma.h
> > new file mode 100644
> > index 0000000..2cb3b5f
> > --- /dev/null
> > +++ b/drivers/dma/qcom_bam_dma.h
> > @@ -0,0 +1,268 @@
> > +/*
> > + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
>
> 2014 ?
>
Should probably be 2013-2014 since development has spanned the change in year.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 and
> > + * only version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +#ifndef __QCOM_BAM_DMA_H__
> > +#define __QCOM_BAM_DMA_H__
> > +
> > +#include <linux/dmaengine.h>
> > +#include "virt-dma.h"
> > +
> > +enum bam_channel_dir {
> > + BAM_PIPE_CONSUMER = 0, /* channel reads from data-fifo or memory */
> > + BAM_PIPE_PRODUCER, /* channel writes to data-fifo or memory */
> > +};
> > +
> > +struct bam_desc_hw {
> > + u32 addr; /* Buffer physical address */
> > + u16 size; /* Buffer size in bytes */
> > + u16 flags;
> > +} __packed;
> > +
> > +#define DESC_FLAG_INT BIT(15)
> > +#define DESC_FLAG_EOT BIT(14)
> > +#define DESC_FLAG_EOB BIT(13)
> > +
> > +struct bam_async_desc {
> > + struct virt_dma_desc vd;
> > +
> > + u32 num_desc;
> > + u32 xfer_len;
> > + struct bam_desc_hw *curr_desc;
> > +
> > + enum bam_channel_dir dir;
> > + size_t length;
> > + struct bam_desc_hw desc[0];
> > +};
> > +
> > +#define BAM_CTRL 0x0000
> > +#define BAM_REVISION 0x0004
> > +#define BAM_SW_REVISION 0x0080
> > +#define BAM_NUM_PIPES 0x003C
> > +#define BAM_TIMER 0x0040
> > +#define BAM_TIMER_CTRL 0x0044
> > +#define BAM_DESC_CNT_TRSHLD 0x0008
> > +#define BAM_IRQ_SRCS 0x000C
> > +#define BAM_IRQ_SRCS_MSK 0x0010
> > +#define BAM_IRQ_SRCS_UNMASKED 0x0030
> > +#define BAM_IRQ_STTS 0x0014
> > +#define BAM_IRQ_CLR 0x0018
> > +#define BAM_IRQ_EN 0x001C
> > +#define BAM_CNFG_BITS 0x007C
> > +#define BAM_IRQ_SRCS_EE(pipe) (0x0800 + ((pipe) * 0x80))
> > +#define BAM_IRQ_SRCS_MSK_EE(pipe) (0x0804 + ((pipe) * 0x80))
> > +#define BAM_P_CTRL(pipe) (0x1000 + ((pipe) * 0x1000))
> > +#define BAM_P_RST(pipe) (0x1004 + ((pipe) * 0x1000))
> > +#define BAM_P_HALT(pipe) (0x1008 + ((pipe) * 0x1000))
> > +#define BAM_P_IRQ_STTS(pipe) (0x1010 + ((pipe) * 0x1000))
> > +#define BAM_P_IRQ_CLR(pipe) (0x1014 + ((pipe) * 0x1000))
> > +#define BAM_P_IRQ_EN(pipe) (0x1018 + ((pipe) * 0x1000))
> > +#define BAM_P_EVNT_DEST_ADDR(pipe) (0x182C + ((pipe) * 0x1000))
> > +#define BAM_P_EVNT_REG(pipe) (0x1818 + ((pipe) * 0x1000))
> > +#define BAM_P_SW_OFSTS(pipe) (0x1800 + ((pipe) * 0x1000))
> > +#define BAM_P_DATA_FIFO_ADDR(pipe) (0x1824 + ((pipe) * 0x1000))
> > +#define BAM_P_DESC_FIFO_ADDR(pipe) (0x181C + ((pipe) * 0x1000))
> > +#define BAM_P_EVNT_TRSHLD(pipe) (0x1828 + ((pipe) * 0x1000))
> > +#define BAM_P_FIFO_SIZES(pipe) (0x1820 + ((pipe) * 0x1000))
> > +
> > +/* BAM CTRL */
> > +#define BAM_SW_RST BIT(0)
> > +#define BAM_EN BIT(1)
> > +#define BAM_EN_ACCUM BIT(4)
> > +#define BAM_TESTBUS_SEL_SHIFT 5
> > +#define BAM_TESTBUS_SEL_MASK 0x3F
> > +#define BAM_DESC_CACHE_SEL_SHIFT 13
> > +#define BAM_DESC_CACHE_SEL_MASK 0x3
> > +#define BAM_CACHED_DESC_STORE BIT(15)
> > +#define IBC_DISABLE BIT(16)
> > +
> > +/* BAM REVISION */
> > +#define REVISION_SHIFT 0
> > +#define REVISION_MASK 0xFF
> > +#define NUM_EES_SHIFT 8
> > +#define NUM_EES_MASK 0xF
> > +#define CE_BUFFER_SIZE BIT(13)
> > +#define AXI_ACTIVE BIT(14)
> > +#define USE_VMIDMT BIT(15)
> > +#define SECURED BIT(16)
> > +#define BAM_HAS_NO_BYPASS BIT(17)
> > +#define HIGH_FREQUENCY_BAM BIT(18)
> > +#define INACTIV_TMRS_EXST BIT(19)
> > +#define NUM_INACTIV_TMRS BIT(20)
> > +#define DESC_CACHE_DEPTH_SHIFT 21
> > +#define DESC_CACHE_DEPTH_1 (0 << DESC_CACHE_DEPTH_SHIFT)
> > +#define DESC_CACHE_DEPTH_2 (1 << DESC_CACHE_DEPTH_SHIFT)
> > +#define DESC_CACHE_DEPTH_3 (2 << DESC_CACHE_DEPTH_SHIFT)
> > +#define DESC_CACHE_DEPTH_4 (3 << DESC_CACHE_DEPTH_SHIFT)
> > +#define CMD_DESC_EN BIT(23)
> > +#define INACTIV_TMR_BASE_SHIFT 24
> > +#define INACTIV_TMR_BASE_MASK 0xFF
> > +
> > +/* BAM NUM PIPES */
> > +#define BAM_NUM_PIPES_SHIFT 0
> > +#define BAM_NUM_PIPES_MASK 0xFF
> > +#define PERIPH_NON_PIPE_GRP_SHIFT 16
> > +#define PERIPH_NON_PIP_GRP_MASK 0xFF
> > +#define BAM_NON_PIPE_GRP_SHIFT 24
> > +#define BAM_NON_PIPE_GRP_MASK 0xFF
> > +
> > +/* BAM CNFG BITS */
> > +#define BAM_PIPE_CNFG BIT(2)
> > +#define BAM_FULL_PIPE BIT(11)
> > +#define BAM_NO_EXT_P_RST BIT(12)
> > +#define BAM_IBC_DISABLE BIT(13)
> > +#define BAM_SB_CLK_REQ BIT(14)
> > +#define BAM_PSM_CSW_REQ BIT(15)
> > +#define BAM_PSM_P_RES BIT(16)
> > +#define BAM_AU_P_RES BIT(17)
> > +#define BAM_SI_P_RES BIT(18)
> > +#define BAM_WB_P_RES BIT(19)
> > +#define BAM_WB_BLK_CSW BIT(20)
> > +#define BAM_WB_CSW_ACK_IDL BIT(21)
> > +#define BAM_WB_RETR_SVPNT BIT(22)
> > +#define BAM_WB_DSC_AVL_P_RST BIT(23)
> > +#define BAM_REG_P_EN BIT(24)
> > +#define BAM_PSM_P_HD_DATA BIT(25)
> > +#define BAM_AU_ACCUMED BIT(26)
> > +#define BAM_CMD_ENABLE BIT(27)
> > +
> > +#define BAM_CNFG_BITS_DEFAULT (BAM_PIPE_CNFG | \
> > + BAM_NO_EXT_P_RST | \
> > + BAM_IBC_DISABLE | \
> > + BAM_SB_CLK_REQ | \
> > + BAM_PSM_CSW_REQ | \
> > + BAM_PSM_P_RES | \
> > + BAM_AU_P_RES | \
> > + BAM_SI_P_RES | \
> > + BAM_WB_P_RES | \
> > + BAM_WB_BLK_CSW | \
> > + BAM_WB_CSW_ACK_IDL | \
> > + BAM_WB_RETR_SVPNT | \
> > + BAM_WB_DSC_AVL_P_RST | \
> > + BAM_REG_P_EN | \
> > + BAM_PSM_P_HD_DATA | \
> > + BAM_AU_ACCUMED | \
> > + BAM_CMD_ENABLE)
> > +
> > +/* PIPE CTRL */
> > +#define P_EN BIT(1)
> > +#define P_DIRECTION BIT(3)
> > +#define P_SYS_STRM BIT(4)
> > +#define P_SYS_MODE BIT(5)
> > +#define P_AUTO_EOB BIT(6)
> > +#define P_AUTO_EOB_SEL_SHIFT 7
> > +#define P_AUTO_EOB_SEL_512 (0 << P_AUTO_EOB_SEL_SHIFT)
> > +#define P_AUTO_EOB_SEL_256 (1 << P_AUTO_EOB_SEL_SHIFT)
> > +#define P_AUTO_EOB_SEL_128 (2 << P_AUTO_EOB_SEL_SHIFT)
> > +#define P_AUTO_EOB_SEL_64 (3 << P_AUTO_EOB_SEL_SHIFT)
> > +#define P_PREFETCH_LIMIT_SHIFT 9
> > +#define P_PREFETCH_LIMIT_32 (0 << P_PREFETCH_LIMIT_SHIFT)
> > +#define P_PREFETCH_LIMIT_16 (1 << P_PREFETCH_LIMIT_SHIFT)
> > +#define P_PREFETCH_LIMIT_4 (2 << P_PREFETCH_LIMIT_SHIFT)
> > +#define P_WRITE_NWD BIT(11)
> > +#define P_LOCK_GROUP_SHIFT 16
> > +#define P_LOCK_GROUP_MASK 0x1F
> > +
> > +/* BAM_DESC_CNT_TRSHLD */
> > +#define CNT_TRSHLD 0xffff
> > +#define DEFAULT_CNT_THRSHLD 0x4
> > +
> > +/* BAM_IRQ_SRCS */
> > +#define BAM_IRQ BIT(31)
> > +#define P_IRQ 0x7fffffff
> > +
> > +/* BAM_IRQ_SRCS_MSK */
> > +#define BAM_IRQ_MSK BAM_IRQ
> > +#define P_IRQ_MSK P_IRQ
> > +
> > +/* BAM_IRQ_STTS */
> > +#define BAM_TIMER_IRQ BIT(4)
> > +#define BAM_EMPTY_IRQ BIT(3)
> > +#define BAM_ERROR_IRQ BIT(2)
> > +#define BAM_HRESP_ERR_IRQ BIT(1)
> > +
> > +/* BAM_IRQ_CLR */
> > +#define BAM_TIMER_CLR BIT(4)
> > +#define BAM_EMPTY_CLR BIT(3)
> > +#define BAM_ERROR_CLR BIT(2)
> > +#define BAM_HRESP_ERR_CLR BIT(1)
> > +
> > +/* BAM_IRQ_EN */
> > +#define BAM_TIMER_EN BIT(4)
> > +#define BAM_EMPTY_EN BIT(3)
> > +#define BAM_ERROR_EN BIT(2)
> > +#define BAM_HRESP_ERR_EN BIT(1)
> > +
> > +/* BAM_P_IRQ_EN */
> > +#define P_PRCSD_DESC_EN BIT(0)
> > +#define P_TIMER_EN BIT(1)
> > +#define P_WAKE_EN BIT(2)
> > +#define P_OUT_OF_DESC_EN BIT(3)
> > +#define P_ERR_EN BIT(4)
> > +#define P_TRNSFR_END_EN BIT(5)
> > +#define P_DEFAULT_IRQS_EN (P_PRCSD_DESC_EN | P_ERR_EN | P_TRNSFR_END_EN)
> > +
> > +/* BAM_P_SW_OFSTS */
> > +#define P_SW_OFSTS_MASK 0xffff
> > +
> > +#define BAM_DESC_FIFO_SIZE SZ_32K
> > +#define MAX_DESCRIPTORS (BAM_DESC_FIFO_SIZE / sizeof(struct bam_desc_hw) - 1)
> > +#define BAM_MAX_DATA_SIZE (SZ_32K - 8)
> > +
> > +struct bam_chan {
> > + struct virt_dma_chan vc;
> > +
> > + struct bam_device *bdev;
> > +
> > + /* configuration from device tree */
> > + u32 id;
> > + u32 ee;
> > +
> > + struct bam_async_desc *curr_txd; /* current running dma */
> > +
> > + /* runtime configuration */
> > + struct dma_slave_config slave;
> > +
> > + /* fifo storage */
> > + struct bam_desc_hw *fifo_virt;
> > + dma_addr_t fifo_phys;
> > +
> > + /* fifo markers */
> > + unsigned short head; /* start of active descriptor entries */
> > + unsigned short tail; /* end of active descriptor entries */
> > +
> > + unsigned int paused; /* is the channel paused? */
> > +
> > + struct list_head node;
> > +};
> > +
> > +static inline struct bam_chan *to_bam_chan(struct dma_chan *common)
> > +{
> > + return container_of(common, struct bam_chan, vc.chan);
> > +}
> > +
> > +struct bam_device {
> > + void __iomem *regs;
> > + struct device *dev;
> > + struct dma_device common;
> > + struct device_dma_parameters dma_parms;
> > + struct bam_chan *channels;
> > + u32 num_channels;
> > + u32 num_ees;
> > + unsigned long enabled_ees;
> > + int irq;
> > + struct clk *bamclk;
> > +
> > + /* dma start transaction tasklet */
> > + struct tasklet_struct task;
> > +};
> > +
> > +#endif /* __QCOM_BAM_DMA_H__ */
>
> --
> Andy Shevchenko <andriy.shevchenko@intel.com>
> Intel Finland Oy
> ---------------------------------------------------------------------
> Intel Finland Oy
> Registered Address: PL 281, 00181 Helsinki
> Business Identity Code: 0357606 - 4
> Domiciled in Helsinki
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply
* [PATCH 1/3] ACPI / idle: Move idle_boot_override out of the arch directory
From: Rafael J. Wysocki @ 2014-01-20 23:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52DD2DE9.9060202@linaro.org>
On Monday, January 20, 2014 10:08:41 PM Hanjun Guo wrote:
> On 2014?01?18? 21:47, Rafael J. Wysocki wrote:
> > On Saturday, January 18, 2014 11:52:18 AM Hanjun Guo wrote:
> >> On 2014-1-18 11:45, Hanjun Guo wrote:
> >>> On 2014-1-17 20:06, Sudeep Holla wrote:
> >>>> On 17/01/14 02:03, Hanjun Guo wrote:
> >>>>> Move idle_boot_override out of the arch directory to be a single enum
> >>>>> including both platforms values, this will make it rather easier to
> >>>>> avoid ifdefs around which definitions are for which processor in
> >>>>> generally used ACPI code.
> >>>>>
> >>>>> IDLE_FORCE_MWAIT for IA64 is not used anywhere, so romove it.
> >>>>>
> >>>>> No functional change in this patch.
> >>>>>
> >>>>> Suggested-by: Alan <gnomes@lxorguk.ukuu.org.uk>
> >>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >>>>> ---
> >> [...]
> >>>>> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> >>>>> index 03e235ad..e324561 100644
> >>>>> --- a/include/linux/cpu.h
> >>>>> +++ b/include/linux/cpu.h
> >>>>> @@ -220,6 +220,14 @@ void cpu_idle(void);
> >>>>>
> >>>>> void cpu_idle_poll_ctrl(bool enable);
> >>>>>
> >>>>> +enum idle_boot_override {
> >>>>> + IDLE_NO_OVERRIDE = 0,
> >>>>> + IDLE_HALT,
> >>>>> + IDLE_NOMWAIT,
> >>>>> + IDLE_POLL,
> >>>>> + IDLE_POWERSAVE_OFF
> >>>>> +};
> >>>>> +
> >>>> I do understand the idea behind this change, but IMO HALT and MWAIT are x86
> >>>> specific and may not make sense for other architectures.
> >>> yes, this is the strange part, the value is arch-dependent.
> >>>
> >>>> It will also require every architecture using ACPI to export
> >>>> boot_option_idle_override which may not be really required.
> >>> so, how about forget this patch and move boot_option_idle_override
> >>> related code into arch directory such as arch/x86/acpi/boot.c for
> >>> x86?
> >> The general idea is that we can move all the arch-dependent codes
> >> in ACPI driver to arch directory, then make codes in drivers/acpi/
> >> arch independent.
> > Well, MWAIT is arch-dependent, so I'm not sure how IDLE_NOMWAIT fits into
> > include/linux/cpu.h?
>
> So you will not happy with this patch and should find another solution?
No, I'm not happy with it.
If you want to move that to an arch-agnostic header, the symbol names cannot
be arch-dependent any more.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* [PATCH] ARM: dts: omap3-ldp: fix mmc configuration
From: Nishanth Menon @ 2014-01-20 23:34 UTC (permalink / raw)
To: linux-arm-kernel
MMC1 is the only MMC interface available on the platform. Further,
since the platform is based on older revision of SoC which is not
capable of doing multi-block writes, mark it so and add pinmux
to ensure that all relevant pins are configured for non-MMC boot
mode.
Signed-off-by: Nishanth Menon <nm@ti.com>
---
ti,erratum-2.1.1.128 introduced in https://patchwork.kernel.org/patch/3514851/
hence depends on the same.
arch/arm/boot/dts/omap3-ldp.dts | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/arch/arm/boot/dts/omap3-ldp.dts b/arch/arm/boot/dts/omap3-ldp.dts
index ddce0d8..bc0cc66 100644
--- a/arch/arm/boot/dts/omap3-ldp.dts
+++ b/arch/arm/boot/dts/omap3-ldp.dts
@@ -176,6 +176,17 @@
&mmc1 {
vmmc-supply = <&vmmc1>;
bus-width = <4>;
+ ti,erratum-2.1.1.128;
+ pinctrl-names = "default";
+ pinctrl-0 = <&mmc1_pins>;
+};
+
+&mmc2 {
+ status="disabled";
+};
+
+&mmc3 {
+ status="disabled";
};
&omap3_pmx_core {
@@ -209,6 +220,17 @@
0x174 (PIN_OUTPUT | MUX_MODE0) /* hsusb0_stp.hsusb0_stp */
>;
};
+
+ mmc1_pins: pinmux_mmc1_pins {
+ pinctrl-single,pins = <
+ OMAP3_CORE1_IOPAD(0x2144, PIN_INPUT_PULLUP | MUX_MODE0) /* mmc1_clk.mmc1_clk */
+ OMAP3_CORE1_IOPAD(0x2146, PIN_INPUT_PULLUP | MUX_MODE0) /* mmc1_cmd.mmc1_cmd */
+ OMAP3_CORE1_IOPAD(0x2148, PIN_INPUT_PULLUP | MUX_MODE0) /* mmc1_dat0.mmc1_dat0 */
+ OMAP3_CORE1_IOPAD(0x214A, PIN_INPUT_PULLUP | MUX_MODE0) /* mmc1_dat1.mmc1_dat1 */
+ OMAP3_CORE1_IOPAD(0x214C, PIN_INPUT_PULLUP | MUX_MODE0) /* mmc1_dat2.mmc1_dat2 */
+ OMAP3_CORE1_IOPAD(0x214e, PIN_INPUT_PULLUP | MUX_MODE0) /* mmc1_dat3.mmc1_dat3 */
+ >;
+ };
};
&usb_otg_hs {
--
1.7.9.5
^ permalink raw reply related
* [PATCH] mmc: omap_hsmmc: Add support for Erratum 2.1.1.128 in device tree boot
From: Felipe Balbi @ 2014-01-20 23:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390260542-22213-1-git-send-email-nm@ti.com>
On Mon, Jan 20, 2014 at 05:29:02PM -0600, Nishanth Menon wrote:
> When device is booted using devicetree, platforms impacted by
> Erratum 2.1.1.128 is not detected easily in the mmc driver. This erratum
> indicates that the module cannot do multi-block transfers.
>
> Handle this by providing a boolean flag to indicate to driver that it is
> working on a hardware with mentioned limitation.
sure there's no way of reading the revision register to figure this one
out without having to add a new DT attribute ?
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140120/25bddc13/attachment.sig>
^ permalink raw reply
* Deadlock in do_page_fault() on ARM (old kernel)
From: Alan Ott @ 2014-01-20 23:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140118012034.GM27282@n2100.arm.linux.org.uk>
On 01/17/2014 08:20 PM, Russell King - ARM Linux wrote:
> On Fri, Jan 17, 2014 at 07:57:16PM -0500, Alan Ott wrote:
>> On 01/17/2014 08:46 AM, Russell King - ARM Linux wrote:
>>> My suspicion therefore is that some other thread must have died while
>>> holding the mmap_sem, so there's probably a kernel oops earlier...
>>> that's my best guess at the moment without seeing the full backtrace.
>> There's no oops that I'm able to see.
>>
>> Each of the tasks which lockdep reports as "holding" mmap_sem are
>> blocking for it. If some other task had taken it and then crashed, I
>> assume lockdep would list the crashed task as also holding the resource
>> in the printout.
> My point is this:
>
> - the five (or six) threads which are trying to take the mmap_sem in
> read-mode in the fault handler are all blocked on it - they haven't
> taken the lock, which will only happen because there's a pending writer.
> - of these in your original post, there are two which faulted from
> __copy_to_user_std(). __copy_to_user_std() doesn't take the mmap_sem -
> this is the non-uaccess-with-memcpy path.
> - the pending writers are the two threads in sys_mmap_pgoff(), both of
> which are blocked waiting to gain the write lock.
> - there are no *other* threads holding the mmap_sem lock.
Yes, all true. I don't remember why I started looking at the memcpy() case.
> So... there's a question here how we got into this state - and frankly
> I don't know. What I do see from your latest dump is that there's two
> unknown modules there - something called rcu2m and another called
> buttoms, and there are two threads inside ioctls there. Both have
> faulted from the function at 0xc0d2a394 (which won't appear in the
> backtrace, but is most likely __copy_to_user_std.)
Yes, there are a handful of out-of-tree modules.
> So, in the absence of you saying anything about there being any preceding
> oopses, my conclusion now is that one of those modules is taking the
> mmap_sem itself, and is the culpret inducing this deadlock.
Yes, I came to that as well. I had checked for the presence of mmap_sem
in the sources of the out-of-tree modules and didn't see it. However,
upon closer inspection, my grep-fu failed me as there were some backward
symlinks I didn't account for. TI's cmemk module _is_ taking out
mmap_sem. I wish I had seen this days ago. That's my new investigation path.
> Note that your dump ([2]) in your reply was just the hung task detector
> printing out the stacktrace for a few tasks, not the full all-threads
> stack dump which I was expecting.
Yes, in a misguided attempt to keep the SNR high, I didn't include the
full dump, but only what I thought was the interesting part. I did
another capture and the full dump is at [1] .
> So I'm pulling out these conclusions from the very little information
> you're supplying.
I appreciate it. Thank you for taking the time to reply.
Alan.
[1]
http://www.signal11.us/~alan/stack_dump_all_tasks_with_frame_pointers.txt
^ permalink raw reply
* PWM...
From: Simon Horman @ 2014-01-21 0:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140120161147.GS17314@sirena.org.uk>
On Mon, Jan 20, 2014 at 04:11:47PM +0000, Mark Brown wrote:
> On Mon, Jan 20, 2014 at 09:14:46AM +0900, Simon Horman wrote:
> > On Sun, Jan 19, 2014 at 09:26:40PM +0100, Arnd Bergmann wrote:
>
> > > Right: if you change an existing dts file from #pwm-cells=<2> to
> > > #pwm-cells=<3>, that requires changing all references to the pwm
>
> > Would this change imply that old dtbs would no longer work with new kernels?
>
> So long as the code can still handle both values for #pwm-cells existing
> DTBs should work fine.
Thanks, as long as that is the case I have no objections at this time.
^ permalink raw reply
* [PATCHv2] ARM: mvebu: fix compilation warning on Armada 370 (i.e. non-SMP)
From: Jason Cooper @ 2014-01-21 0:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87txcy8goq.fsf@natisbad.org>
On Mon, Jan 20, 2014 at 10:52:05PM +0100, Arnaud Ebalard wrote:
>
> The following appears during compilation for an Armada 370 target
> because 'irq_controller_lock' is used only when CONFIG_SMP is
> enabled:
>
> drivers/irqchip/irq-armada-370-xp.c:62:8: warning: 'irq_controller_lock' defined but not used [-Wunused-variable]
>
> Fix that warning by moving declaration of 'irq_controller_lock'
> inside existing #ifdef.
>
> Signed-off-by: Arnaud Ebalard <arno@natisbad.org>
> ---
>
> Changes since v1:
> - moved declaration inside existing #ifdef as suggested by Arnd,
> instead of adding a new one.
>
> drivers/irqchip/irq-armada-370-xp.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Applied to mvebu/fixes
thx,
Jason.
^ permalink raw reply
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
From: Olof Johansson @ 2014-01-21 1:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140120224742.EC905C40B12@trevor.secretlab.ca>
On Mon, Jan 20, 2014 at 2:47 PM, Grant Likely <grant.likely@linaro.org> wrote:
> On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland@arm.com> wrote:
>> >
>> > Another, more invasive option would be extend the dts syntax and teach
>> > dtc to handle property appending. Then the soc dts could stay as it is,
>> > and the board dts could have something like:
>> >
>> > /append-property/ interrupts = <&intc1 6 1>;
>> > /append-property/ interrupt-names = "board-specific-irq";
>> >
>> > Both these options solve the issue at the source, are general to other
>> > properties, and allow more than one level of hierarchy (the proposed
>> > interrupts-extended-2 only allows one level).
>>
>> I've just had a go at implementing the append-property mechanism above
>> in dtc, and it was far easier than I expected (patch below).
>>
>> Does anyone have any issues with the /append-property/ idea?
>
> I think that is reasonable.
The main problem with this (same for clocks) is if you need to append
something with a name when the original didn't have any.
Reordering entries might not work for interrupts, since the bindings
might have requirements on order.
I'm not aware of a good solution for this. Suggestions welcome.
-Olof
^ permalink raw reply
* pinctrl: at91: drive strength control
From: Marek Roszko @ 2014-01-21 1:07 UTC (permalink / raw)
To: linux-arm-kernel
Hello Nick and Jean-Christophe,
I submit a idea/patch for comments. The current pintctrl driver is missing the ability to set the drive strength in the SAMA5D3s and a few of the SAM9s.
A little feature I myself need for the SAMA5D3s because somebody let an analog engineer do the hardware design.
Issues with this patch I need help with:
1. Atmel for some reason shifted the PIO_DRIVER1 and PIO_DRIVER2 registers by one register address between the SAM9 chips and the SAMA5D3s
i.e. this is the SAM9s
#define PIO_DRIVER1_V1 0x114
#define PIO_DRIVER2_V1 0x118
this is the SAMA5D3s
#define PIO_DRIVER1_V2 0x118
#define PIO_DRIVER2_V2 0x11C
2. Atmel changed the meaning of value of "low", "medium" and "high" drive strengths between the two sets of chips as well.
SAM9s do:
00 = high
01 = medium
10 = low
11 = reserved/undefined
SAMA5D3s do:
00 = low
01 = low
10 = medium
11 = high
3. The SAM9G25, SAM9G35 have the PIO_DRIVER but the SAM9G45 does not have the PIO_DRIVER register as an example of
how oddly the behavior doesn't appear everywhere. This could also be because the datasheets for the G45 and G46 are
not updated to the new style while the others already have been?
Side note: The SAMA5D3 datasheet says the default drive strength is "low" with register values 0x00000000,
in reality the default is "medium" with 0xAAAAAAAA as the default value.
Confirmed with the JLINK debugger and by Atmel support. Not an real issue, just a note when testing.
So the only simple way I see is to #ifdef the SOC type/chip.
There's probably better far better ways I don't know.
---
arch/arm/mach-at91/include/mach/at91_pio.h | 9 +++++
drivers/pinctrl/pinctrl-at91.c | 54 +++++++++++++++++++++++++++-
include/dt-bindings/pinctrl/at91.h | 13 +++++++
3 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-at91/include/mach/at91_pio.h b/arch/arm/mach-at91/include/mach/at91_pio.h
index 732b11c..28e8801 100644
--- a/arch/arm/mach-at91/include/mach/at91_pio.h
+++ b/arch/arm/mach-at91/include/mach/at91_pio.h
@@ -66,6 +66,15 @@
#define PIO_FRLHSR 0xd8 /* Fall/Rise - Low/High Status Register */
#define PIO_SCHMITT 0x100 /* Schmitt Trigger Register */
+/*
+ * SoC Specific PIO Address Offsets
+ */
+#define PIO_DRIVER1_V1 0x114
+#define PIO_DRIVER2_V1 0x118
+
+#define PIO_DRIVER1_V2 0x118
+#define PIO_DRIVER2_V2 0x11C
+
#define ABCDSR_PERIPH_A 0x0
#define ABCDSR_PERIPH_B 0x1
#define ABCDSR_PERIPH_C 0x2
diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index a7549c4..ccf456e 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -62,10 +62,37 @@ static int gpio_banks;
#define DEGLITCH (1 << 2)
#define PULL_DOWN (1 << 3)
#define DIS_SCHMIT (1 << 4)
+#define SET_DRIVE_STRENGTH (1 << 5)
+#define DRIVE_STRENGTH_SHIFT 6
+#define DRIVE_STRENGTH (0x3 << DRIVE_STRENGTH_SHIFT)
#define DEBOUNCE (1 << 16)
#define DEBOUNCE_VAL_SHIFT 17
#define DEBOUNCE_VAL (0x3fff << DEBOUNCE_VAL_SHIFT)
+#define DRIVE_STRENGTH_MASK 0x3
+
+#define NUM_PINS_PER_DRIVE_STRENGTH_REG 16
+
+#define TWO_BIT_PIN_TO_SHIFT(pin)\
+ (2*((pin >= NUM_PINS_PER_DRIVE_STRENGTH_REG) ? \
+ pin - NUM_PINS_PER_DRIVE_STRENGTH_REG : pin))
+
+#define TWO_BIT_PIN_TO_MASK(pin)\
+ (DRIVE_STRENGTH_MASK << TWO_BIT_PIN_TO_SHIFT(pin))
+
+#if defined(CONFIG_SOC_SAMA5D3)
+ #define PIO_DRIVER1 PIO_DRIVER1_V2 /* Drive Strength Register 1 */
+ #define PIO_DRIVER2 PIO_DRIVER2_V2 /* Drive Strength Register 2 */
+#elif defined(CONFIG_SOC_AT91SAM9X5) || defined(SOC_AT91SAM9G35) || defined(SOC_AT91SAM9G25)
+ #define PIO_DRIVER1 PIO_DRIVER1_V1 /* Drive Strength Register 1 */
+ #define PIO_DRIVER2 PIO_DRIVER2_V1 /* Drive Strength Register 2 */
+#endif
+
+#if defined(PIO_DRIVER1) && defined(PIO_DRIVER2)
+ #define PIO_DRIVER(pin)\
+ ((pin > NUM_PINS_PER_DRIVE_STRENGTH_REG-1) ? PIO_DRIVER2 : PIO_DRIVER1)
+#endif
+
/**
* struct at91_pmx_func - describes AT91 pinmux functions
* @name: the name of this specific function
@@ -148,6 +175,8 @@ struct at91_pinctrl_mux_ops {
void (*set_deglitch)(void __iomem *pio, unsigned mask, bool is_on);
bool (*get_debounce)(void __iomem *pio, unsigned pin, u32 *div);
void (*set_debounce)(void __iomem *pio, unsigned mask, bool is_on, u32 div);
+ int (*get_drivestrength)(void __iomem *pio, unsigned pin);
+ void (*set_drivestrength)(void __iomem *pio, unsigned pin, u32 strength);
bool (*get_pulldown)(void __iomem *pio, unsigned pin);
void (*set_pulldown)(void __iomem *pio, unsigned mask, bool is_on);
bool (*get_schmitt_trig)(void __iomem *pio, unsigned pin);
@@ -462,6 +491,20 @@ static void at91_mux_pio3_set_pulldown(void __iomem *pio, unsigned mask, bool is
__raw_writel(mask, pio + (is_on ? PIO_PPDER : PIO_PPDDR));
}
+static int at91_mux_pio3_get_drivestrength(void __iomem *pio, unsigned pin)
+{
+ return (__raw_readl(pio + PIO_DRIVER(pin)) >> TWO_BIT_PIN_TO_SHIFT(pin)) & DRIVE_STRENGTH_MASK;
+}
+
+static void at91_mux_pio3_set_drivestrength(void __iomem *pio, unsigned pin, u32 strength)
+{
+ unsigned tmp = __raw_readl(pio + PIO_DRIVER(pin));
+ tmp &= ~TWO_BIT_PIN_TO_MASK(pin);
+ tmp |= strength << TWO_BIT_PIN_TO_SHIFT(pin);
+
+ __raw_writel(tmp, pio + PIO_DRIVER(pin));
+}
+
static void at91_mux_pio3_disable_schmitt_trig(void __iomem *pio, unsigned mask)
{
__raw_writel(__raw_readl(pio + PIO_SCHMITT) | mask, pio + PIO_SCHMITT);
@@ -491,6 +534,8 @@ static struct at91_pinctrl_mux_ops at91sam9x5_ops = {
.set_deglitch = at91_mux_pio3_set_deglitch,
.get_debounce = at91_mux_pio3_get_debounce,
.set_debounce = at91_mux_pio3_set_debounce,
+ .get_drivestrength = at91_mux_pio3_get_drivestrength,
+ .set_drivestrength = at91_mux_pio3_set_drivestrength,
.get_pulldown = at91_mux_pio3_get_pulldown,
.set_pulldown = at91_mux_pio3_set_pulldown,
.get_schmitt_trig = at91_mux_pio3_get_schmitt_trig,
@@ -736,6 +781,8 @@ static int at91_pinconf_get(struct pinctrl_dev *pctldev,
*config |= DEGLITCH;
if (info->ops->get_debounce && info->ops->get_debounce(pio, pin, &div))
*config |= DEBOUNCE | (div << DEBOUNCE_VAL_SHIFT);
+ if (info->ops->get_drivestrength)
+ *config |= DRIVE_STRENGTH | (info->ops->get_drivestrength(pio, pin) << DRIVE_STRENGTH_SHIFT);
if (info->ops->get_pulldown && info->ops->get_pulldown(pio, pin))
*config |= PULL_DOWN;
if (info->ops->get_schmitt_trig && info->ops->get_schmitt_trig(pio, pin))
@@ -753,6 +800,7 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
void __iomem *pio;
int i;
unsigned long config;
+ unsigned pin;
for (i = 0; i < num_configs; i++) {
config = configs[i];
@@ -761,7 +809,8 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
"%s:%d, pin_id=%d, config=0x%lx",
__func__, __LINE__, pin_id, config);
pio = pin_to_controller(info, pin_to_bank(pin_id));
- mask = pin_to_mask(pin_id % MAX_NB_GPIO_PER_BANK);
+ pin = pin_id % MAX_NB_GPIO_PER_BANK;
+ mask = pin_to_mask(pin);
if (config & PULL_UP && config & PULL_DOWN)
return -EINVAL;
@@ -773,6 +822,9 @@ static int at91_pinconf_set(struct pinctrl_dev *pctldev,
if (info->ops->set_debounce)
info->ops->set_debounce(pio, mask, config & DEBOUNCE,
(config & DEBOUNCE_VAL) >> DEBOUNCE_VAL_SHIFT);
+ if (info->ops->set_drivestrength && config & SET_DRIVE_STRENGTH)
+ info->ops->set_drivestrength(pio, pin,
+ (config & DRIVE_STRENGTH) >> DRIVE_STRENGTH_SHIFT);
if (info->ops->set_pulldown)
info->ops->set_pulldown(pio, mask, config & PULL_DOWN);
if (info->ops->disable_schmitt_trig && config & DIS_SCHMIT)
diff --git a/include/dt-bindings/pinctrl/at91.h b/include/dt-bindings/pinctrl/at91.h
index 0fee6ff..78621a4 100644
--- a/include/dt-bindings/pinctrl/at91.h
+++ b/include/dt-bindings/pinctrl/at91.h
@@ -20,6 +20,19 @@
#define AT91_PINCTRL_PULL_UP_DEGLITCH (AT91_PINCTRL_PULL_UP | AT91_PINCTRL_DEGLITCH)
+#define AT91_PINCTRL_SET_DRIVE_STRENGTH (1 << 5)
+
+#if defined(CONFIG_SOC_SAMA5D3)
+ #define AT91_PINCTRL_DRIVE_STRENGTH_LOW (0x2 << 6)
+ #define AT91_PINCTRL_DRIVE_STRENGTH_MED (0x1 << 6)
+ #define AT91_PINCTRL_DRIVE_STRENGTH_HI (0x0 << 6)
+#elif defined(CONFIG_SOC_AT91SAM9X5) || defined(SOC_AT91SAM9G35) || defined(SOC_AT91SAM9G25)
+ #define AT91_PINCTRL_DRIVE_STRENGTH_LOW (0x1 << 6)
+ #define AT91_PINCTRL_DRIVE_STRENGTH_MED (0x2 << 6)
+ #define AT91_PINCTRL_DRIVE_STRENGTH_HI (0x3 << 6)
+#endif
+
+
#define AT91_PIOA 0
#define AT91_PIOB 1
#define AT91_PIOC 2
--
1.7.10.4
--
Mark
^ permalink raw reply related
* [PATCH] arm64: mm: fix the function name in comment of __flush_dcache_area
From: Jingoo Han @ 2014-01-21 1:17 UTC (permalink / raw)
To: linux-arm-kernel
Fix the function name of comment of __flush_dcache_area,
because __flush_dcache_area is the correct name. Also,
the missing variable 'size' is added to the comment.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
arch/arm64/mm/cache.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S
index 48a3860..1ea9f26 100644
--- a/arch/arm64/mm/cache.S
+++ b/arch/arm64/mm/cache.S
@@ -146,7 +146,7 @@ ENDPROC(flush_icache_range)
ENDPROC(__flush_cache_user_range)
/*
- * __flush_kern_dcache_page(kaddr)
+ * __flush_dcache_area(kaddr, size)
*
* Ensure that the data held in the page kaddr is written back to the
* page in question.
--
1.7.10.4
^ permalink raw reply related
* [PATCH REPOST 1/5] ARM: kvm: replace push and pop with stdmb and ldmia instrs to enable assembler.h inclusion
From: Christoffer Dall @ 2014-01-21 1:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1387558125-3460-2-git-send-email-victor.kamensky@linaro.org>
On Fri, Dec 20, 2013 at 08:48:41AM -0800, Victor Kamensky wrote:
> Before fix kvm interrupt.S and interrupt_head.S used push and pop assembler
> instruction. It causes problem if <asm/assembler.h> file should be include. In
> assembler.h "push" is defined as macro so it causes compilation errors like
> this:
"Before fix kvm..." doesn't read very pleasently, consider using
something like "Prior to this commit...."
"causes a problem" or "causes problems"
change "if <asm/assembler.h> file should be include..." to "if
<asm/assembler.h> is included, because assember.h defines 'push' as a
macro..."
>
> arch/arm/kvm/interrupts.S: Assembler messages:
> arch/arm/kvm/interrupts.S:51: Error: ARM register expected -- `lsr {r2,r3}'
>
> Solution implemented by this patch replaces all 'push {...}' with
> 'stdmb sp!, {...}' instruction; and all 'pop {...}' with 'ldmia sp!, {...}'.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
> arch/arm/kvm/interrupts.S | 38 +++++++++++++++++++-------------------
> arch/arm/kvm/interrupts_head.S | 34 +++++++++++++++++-----------------
> 2 files changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index ddc1553..df19133 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -47,7 +47,7 @@ __kvm_hyp_code_start:
> * instead, ignoring the ipa value.
> */
> ENTRY(__kvm_tlb_flush_vmid_ipa)
> - push {r2, r3}
> + stmdb sp!, {r2, r3}
>
> dsb ishst
> add r0, r0, #KVM_VTTBR
> @@ -62,7 +62,7 @@ ENTRY(__kvm_tlb_flush_vmid_ipa)
> mcrr p15, 6, r2, r3, c2 @ Back to VMID #0
> isb @ Not necessary if followed by eret
>
> - pop {r2, r3}
> + ldmia sp!, {r2, r3}
> bx lr
> ENDPROC(__kvm_tlb_flush_vmid_ipa)
>
> @@ -110,7 +110,7 @@ ENTRY(__kvm_vcpu_run)
> #ifdef CONFIG_VFPv3
> @ Set FPEXC_EN so the guest doesn't trap floating point instructions
> VFPFMRX r2, FPEXC @ VMRS
> - push {r2}
> + stmdb sp!, {r2}
> orr r2, r2, #FPEXC_EN
> VFPFMXR FPEXC, r2 @ VMSR
> #endif
> @@ -175,7 +175,7 @@ __kvm_vcpu_return:
>
> after_vfp_restore:
> @ Restore FPEXC_EN which we clobbered on entry
> - pop {r2}
> + ldmia sp!, {r2}
> VFPFMXR FPEXC, r2
> #endif
>
> @@ -260,7 +260,7 @@ ENTRY(kvm_call_hyp)
>
> /* Handle undef, svc, pabt, or dabt by crashing with a user notice */
> .macro bad_exception exception_code, panic_str
> - push {r0-r2}
> + stmdb sp!, {r0-r2}
> mrrc p15, 6, r0, r1, c2 @ Read VTTBR
> lsr r1, r1, #16
> ands r1, r1, #0xff
> @@ -338,7 +338,7 @@ hyp_hvc:
> * Getting here is either becuase of a trap from a guest or from calling
> * HVC from the host kernel, which means "switch to Hyp mode".
> */
> - push {r0, r1, r2}
> + stmdb sp!, {r0, r1, r2}
>
> @ Check syndrome register
> mrc p15, 4, r1, c5, c2, 0 @ HSR
> @@ -361,11 +361,11 @@ hyp_hvc:
> bne guest_trap @ Guest called HVC
>
> host_switch_to_hyp:
> - pop {r0, r1, r2}
> + ldmia sp!, {r0, r1, r2}
>
> - push {lr}
> + stmdb sp!, {lr}
> mrs lr, SPSR
> - push {lr}
> + stmdb sp!, {lr}
>
> mov lr, r0
> mov r0, r1
> @@ -375,9 +375,9 @@ host_switch_to_hyp:
> THUMB( orr lr, #1)
> blx lr @ Call the HYP function
>
> - pop {lr}
> + ldmia sp!, {lr}
> msr SPSR_csxf, lr
> - pop {lr}
> + ldmia sp!, {lr}
> eret
>
> guest_trap:
> @@ -418,7 +418,7 @@ guest_trap:
>
> /* Preserve PAR */
> mrrc p15, 0, r0, r1, c7 @ PAR
> - push {r0, r1}
> + stmdb sp!, {r0, r1}
>
> /* Resolve IPA using the xFAR */
> mcr p15, 0, r2, c7, c8, 0 @ ATS1CPR
> @@ -431,7 +431,7 @@ guest_trap:
> orr r2, r2, r1, lsl #24
>
> /* Restore PAR */
> - pop {r0, r1}
> + ldmia sp!, {r0, r1}
> mcrr p15, 0, r0, r1, c7 @ PAR
>
> 3: load_vcpu @ Load VCPU pointer to r0
> @@ -440,10 +440,10 @@ guest_trap:
> 1: mov r1, #ARM_EXCEPTION_HVC
> b __kvm_vcpu_return
>
> -4: pop {r0, r1} @ Failed translation, return to guest
> +4: ldmia sp!, {r0, r1} @ Failed translation, return to guest
> mcrr p15, 0, r0, r1, c7 @ PAR
> clrex
> - pop {r0, r1, r2}
> + ldmia sp!, {r0, r1, r2}
> eret
>
> /*
> @@ -455,7 +455,7 @@ guest_trap:
> #ifdef CONFIG_VFPv3
> switch_to_guest_vfp:
> load_vcpu @ Load VCPU pointer to r0
> - push {r3-r7}
> + stmdb sp!, {r3-r7}
>
> @ NEON/VFP used. Turn on VFP access.
> set_hcptr vmexit, (HCPTR_TCP(10) | HCPTR_TCP(11))
> @@ -467,15 +467,15 @@ switch_to_guest_vfp:
> add r7, r0, #VCPU_VFP_GUEST
> restore_vfp_state r7
>
> - pop {r3-r7}
> - pop {r0-r2}
> + ldmia sp!, {r3-r7}
> + ldmia sp!, {r0-r2}
> clrex
> eret
> #endif
>
> .align
> hyp_irq:
> - push {r0, r1, r2}
> + stmdb sp!, {r0, r1, r2}
> mov r1, #ARM_EXCEPTION_IRQ
> load_vcpu @ Load VCPU pointer to r0
> b __kvm_vcpu_return
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 6f18695..c371db7 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -63,7 +63,7 @@ vcpu .req r0 @ vcpu pointer always in r0
> mrs r2, SP_\mode
> mrs r3, LR_\mode
> mrs r4, SPSR_\mode
> - push {r2, r3, r4}
> + stmdb sp!, {r2, r3, r4}
> .endm
>
> /*
> @@ -73,13 +73,13 @@ vcpu .req r0 @ vcpu pointer always in r0
> .macro save_host_regs
> /* Hyp regs. Only ELR_hyp (SPSR_hyp already saved) */
> mrs r2, ELR_hyp
> - push {r2}
> + stmdb sp!, {r2}
>
> /* usr regs */
> - push {r4-r12} @ r0-r3 are always clobbered
> + stmdb sp!, {r4-r12} @ r0-r3 are always clobbered
> mrs r2, SP_usr
> mov r3, lr
> - push {r2, r3}
> + stmdb sp!, {r2, r3}
>
> push_host_regs_mode svc
> push_host_regs_mode abt
> @@ -95,11 +95,11 @@ vcpu .req r0 @ vcpu pointer always in r0
> mrs r7, SP_fiq
> mrs r8, LR_fiq
> mrs r9, SPSR_fiq
> - push {r2-r9}
> + stmdb sp!, {r2-r9}
> .endm
>
> .macro pop_host_regs_mode mode
> - pop {r2, r3, r4}
> + ldmia sp!, {r2, r3, r4}
> msr SP_\mode, r2
> msr LR_\mode, r3
> msr SPSR_\mode, r4
> @@ -110,7 +110,7 @@ vcpu .req r0 @ vcpu pointer always in r0
> * Clobbers all registers, in all modes, except r0 and r1.
> */
> .macro restore_host_regs
> - pop {r2-r9}
> + ldmia sp!, {r2-r9}
> msr r8_fiq, r2
> msr r9_fiq, r3
> msr r10_fiq, r4
> @@ -125,12 +125,12 @@ vcpu .req r0 @ vcpu pointer always in r0
> pop_host_regs_mode abt
> pop_host_regs_mode svc
>
> - pop {r2, r3}
> + ldmia sp!, {r2, r3}
> msr SP_usr, r2
> mov lr, r3
> - pop {r4-r12}
> + ldmia sp!, {r4-r12}
>
> - pop {r2}
> + ldmia sp!, {r2}
> msr ELR_hyp, r2
> .endm
>
> @@ -218,7 +218,7 @@ vcpu .req r0 @ vcpu pointer always in r0
> add r2, vcpu, #VCPU_USR_REG(3)
> stm r2, {r3-r12}
> add r2, vcpu, #VCPU_USR_REG(0)
> - pop {r3, r4, r5} @ r0, r1, r2
> + ldmia sp!, {r3, r4, r5} @ r0, r1, r2
> stm r2, {r3, r4, r5}
> mrs r2, SP_usr
> mov r3, lr
> @@ -258,7 +258,7 @@ vcpu .req r0 @ vcpu pointer always in r0
> mrc p15, 2, r12, c0, c0, 0 @ CSSELR
>
> .if \store_to_vcpu == 0
> - push {r2-r12} @ Push CP15 registers
> + stmdb sp!, {r2-r12} @ Push CP15 registers
> .else
> str r2, [vcpu, #CP15_OFFSET(c1_SCTLR)]
> str r3, [vcpu, #CP15_OFFSET(c1_CPACR)]
> @@ -286,7 +286,7 @@ vcpu .req r0 @ vcpu pointer always in r0
> mrc p15, 0, r12, c12, c0, 0 @ VBAR
>
> .if \store_to_vcpu == 0
> - push {r2-r12} @ Push CP15 registers
> + stmdb sp!, {r2-r12} @ Push CP15 registers
> .else
> str r2, [vcpu, #CP15_OFFSET(c13_CID)]
> str r3, [vcpu, #CP15_OFFSET(c13_TID_URW)]
> @@ -305,7 +305,7 @@ vcpu .req r0 @ vcpu pointer always in r0
> mrrc p15, 0, r4, r5, c7 @ PAR
>
> .if \store_to_vcpu == 0
> - push {r2,r4-r5}
> + stmdb sp!, {r2,r4-r5}
> .else
> str r2, [vcpu, #CP15_OFFSET(c14_CNTKCTL)]
> add r12, vcpu, #CP15_OFFSET(c7_PAR)
> @@ -322,7 +322,7 @@ vcpu .req r0 @ vcpu pointer always in r0
> */
> .macro write_cp15_state read_from_vcpu
> .if \read_from_vcpu == 0
> - pop {r2,r4-r5}
> + ldmia sp!, {r2,r4-r5}
> .else
> ldr r2, [vcpu, #CP15_OFFSET(c14_CNTKCTL)]
> add r12, vcpu, #CP15_OFFSET(c7_PAR)
> @@ -333,7 +333,7 @@ vcpu .req r0 @ vcpu pointer always in r0
> mcrr p15, 0, r4, r5, c7 @ PAR
>
> .if \read_from_vcpu == 0
> - pop {r2-r12}
> + ldmia sp!, {r2-r12}
> .else
> ldr r2, [vcpu, #CP15_OFFSET(c13_CID)]
> ldr r3, [vcpu, #CP15_OFFSET(c13_TID_URW)]
> @@ -361,7 +361,7 @@ vcpu .req r0 @ vcpu pointer always in r0
> mcr p15, 0, r12, c12, c0, 0 @ VBAR
>
> .if \read_from_vcpu == 0
> - pop {r2-r12}
> + ldmia sp!, {r2-r12}
> .else
> ldr r2, [vcpu, #CP15_OFFSET(c1_SCTLR)]
> ldr r3, [vcpu, #CP15_OFFSET(c1_CPACR)]
> --
> 1.8.1.4
>
If you fix to address Dave's comments, then the code change otherwise
looks good.
Thanks,
--
Christoffer
^ permalink raw reply
* [PATCH REPOST 2/5] ARM: fix KVM assembler files to work in BE case
From: Christoffer Dall @ 2014-01-21 1:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1387558125-3460-3-git-send-email-victor.kamensky@linaro.org>
On Fri, Dec 20, 2013 at 08:48:42AM -0800, Victor Kamensky wrote:
> ARM v7 KVM assembler files fixes to work in big endian mode:
I don't think 'files fixes' is proper English, could be something like:
Fix ARM v7 KVM assembler files to work...
>
> vgic h/w registers are little endian; when asm code reads/writes from/to
the vgic h/w registers
> them, it needs to do byteswap after/before. Byte swap code uses ARM_BE8
Byteswap
> wrapper to add swap only if BIG_ENDIAN kernel is configured
what is the config symbol, CONFIG_BIG_ENDIAN?
>
> mcrr and mrrc instructions take couple 32 bit registers as argument, one
The mcrr and mrrc...
a couple of
as their arguments
> is supposed to be high part of 64 bit value and another is low part of
> 64 bit. Typically those values are loaded/stored with ldrd and strd
one is supposed to be?
> instructions and those will load high and low parts in opposite register
> depending on endianity. Introduce and use rr_lo_hi macro that swap
opposite register? This text is more confusing that clarifying, I think
you need to explain what how the rr_lo_hi macro is intended to be used
if anything.
> registers in BE mode when they are passed to mcrr and mrrc instructions.
>
> function that returns 64 bit result __kvm_vcpu_run in couple registers
> has to be adjusted for BE case.
The __kvm_vcpu_run function returns a 64-bit result in two registers,
which has...
>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
> arch/arm/include/asm/assembler.h | 7 +++++++
> arch/arm/include/asm/kvm_asm.h | 4 ++--
> arch/arm/kvm/init.S | 7 +++++--
> arch/arm/kvm/interrupts.S | 12 +++++++++---
> arch/arm/kvm/interrupts_head.S | 27 ++++++++++++++++++++-------
> 5 files changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
> index 5c22851..ad1ad31 100644
> --- a/arch/arm/include/asm/assembler.h
> +++ b/arch/arm/include/asm/assembler.h
> @@ -60,6 +60,13 @@
> #define ARM_BE8(code...)
> #endif
>
> +/* swap pair of registers position depending on current endianity */
> +#ifdef CONFIG_CPU_ENDIAN_BE8
> +#define rr_lo_hi(a1, a2) a2, a1
> +#else
> +#define rr_lo_hi(a1, a2) a1, a2
> +#endif
> +
I'm not convinced that this is needed generally in the kernel and not
locally to KVM, but if it is, then I think it needs to be documented
more. I assume the idea here is that a1 is always the lowered number
register in an ldrd instruction loading the values to write to the
register?
> /*
> * Data preload for architectures that support it
> */
> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
> index 661da11..12981d6 100644
> --- a/arch/arm/include/asm/kvm_asm.h
> +++ b/arch/arm/include/asm/kvm_asm.h
> @@ -26,9 +26,9 @@
> #define c1_ACTLR 4 /* Auxilliary Control Register */
> #define c1_CPACR 5 /* Coprocessor Access Control */
> #define c2_TTBR0 6 /* Translation Table Base Register 0 */
> -#define c2_TTBR0_high 7 /* TTBR0 top 32 bits */
> +#define c2_TTBR0_hilo 7 /* TTBR0 top 32 bits in LE case, low 32 bits in BE case */
> #define c2_TTBR1 8 /* Translation Table Base Register 1 */
> -#define c2_TTBR1_high 9 /* TTBR1 top 32 bits */
> +#define c2_TTBR1_hilo 9 /* TTBR1 top 32 bits in LE case, low 32 bits in BE case */
These lines far exceed 80 chars, but not sure how to improve on that...
> #define c2_TTBCR 10 /* Translation Table Base Control R. */
> #define c3_DACR 11 /* Domain Access Control Register */
> #define c5_DFSR 12 /* Data Fault Status Register */
> diff --git a/arch/arm/kvm/init.S b/arch/arm/kvm/init.S
> index 1b9844d..2d10b2d 100644
> --- a/arch/arm/kvm/init.S
> +++ b/arch/arm/kvm/init.S
> @@ -22,6 +22,7 @@
> #include <asm/kvm_asm.h>
> #include <asm/kvm_arm.h>
> #include <asm/kvm_mmu.h>
> +#include <asm/assembler.h>
>
> /********************************************************************
> * Hypervisor initialization
> @@ -70,8 +71,10 @@ __do_hyp_init:
> cmp r0, #0 @ We have a SP?
> bne phase2 @ Yes, second stage init
>
> +ARM_BE8(setend be) @ Switch to Big Endian mode if needed
> +
> @ Set the HTTBR to point to the hypervisor PGD pointer passed
> - mcrr p15, 4, r2, r3, c2
> + mcrr p15, 4, rr_lo_hi(r2, r3), c2
>
> @ Set the HTCR and VTCR to the same shareability and cacheability
> @ settings as the non-secure TTBCR and with T0SZ == 0.
> @@ -137,7 +140,7 @@ phase2:
> mov pc, r0
>
> target: @ We're now in the trampoline code, switch page tables
> - mcrr p15, 4, r2, r3, c2
> + mcrr p15, 4, rr_lo_hi(r2, r3), c2
> isb
I guess you could switch r2 and r3 (without a third register or using
stack space) on big endian to avoid the need for the macro in a header
file and define the macro locally in the interrupts*.S files... Hmmm,
undecided.
>
> @ Invalidate the old TLBs
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index df19133..0784ec3 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -25,6 +25,7 @@
> #include <asm/kvm_asm.h>
> #include <asm/kvm_arm.h>
> #include <asm/vfpmacros.h>
> +#include <asm/assembler.h>
> #include "interrupts_head.S"
>
> .text
> @@ -52,14 +53,14 @@ ENTRY(__kvm_tlb_flush_vmid_ipa)
> dsb ishst
> add r0, r0, #KVM_VTTBR
> ldrd r2, r3, [r0]
> - mcrr p15, 6, r2, r3, c2 @ Write VTTBR
> + mcrr p15, 6, rr_lo_hi(r2, r3), c2 @ Write VTTBR
> isb
> mcr p15, 0, r0, c8, c3, 0 @ TLBIALLIS (rt ignored)
> dsb ish
> isb
> mov r2, #0
> mov r3, #0
> - mcrr p15, 6, r2, r3, c2 @ Back to VMID #0
> + mcrr p15, 6, rr_lo_hi(r2, r3), c2 @ Back to VMID #0
> isb @ Not necessary if followed by eret
>
> ldmia sp!, {r2, r3}
> @@ -135,7 +136,7 @@ ENTRY(__kvm_vcpu_run)
> ldr r1, [vcpu, #VCPU_KVM]
> add r1, r1, #KVM_VTTBR
> ldrd r2, r3, [r1]
> - mcrr p15, 6, r2, r3, c2 @ Write VTTBR
> + mcrr p15, 6, rr_lo_hi(r2, r3), c2 @ Write VTTBR
>
> @ We're all done, just restore the GPRs and go to the guest
> restore_guest_regs
> @@ -199,8 +200,13 @@ after_vfp_restore:
>
> restore_host_regs
> clrex @ Clear exclusive monitor
> +#ifndef __ARMEB__
> mov r0, r1 @ Return the return code
> mov r1, #0 @ Clear upper bits in return value
> +#else
> + @ r1 already has return code
> + mov r0, #0 @ Clear upper bits in return value
> +#endif /* __ARMEB__ */
> bx lr @ return to IOCTL
>
> /********************************************************************
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index c371db7..67b4002 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -251,8 +251,8 @@ vcpu .req r0 @ vcpu pointer always in r0
> mrc p15, 0, r3, c1, c0, 2 @ CPACR
> mrc p15, 0, r4, c2, c0, 2 @ TTBCR
> mrc p15, 0, r5, c3, c0, 0 @ DACR
> - mrrc p15, 0, r6, r7, c2 @ TTBR 0
> - mrrc p15, 1, r8, r9, c2 @ TTBR 1
> + mrrc p15, 0, rr_lo_hi(r6, r7), c2 @ TTBR 0
> + mrrc p15, 1, rr_lo_hi(r8, r9), c2 @ TTBR 1
> mrc p15, 0, r10, c10, c2, 0 @ PRRR
> mrc p15, 0, r11, c10, c2, 1 @ NMRR
> mrc p15, 2, r12, c0, c0, 0 @ CSSELR
> @@ -380,8 +380,8 @@ vcpu .req r0 @ vcpu pointer always in r0
> mcr p15, 0, r3, c1, c0, 2 @ CPACR
> mcr p15, 0, r4, c2, c0, 2 @ TTBCR
> mcr p15, 0, r5, c3, c0, 0 @ DACR
> - mcrr p15, 0, r6, r7, c2 @ TTBR 0
> - mcrr p15, 1, r8, r9, c2 @ TTBR 1
> + mcrr p15, 0, rr_lo_hi(r6, r7), c2 @ TTBR 0
> + mcrr p15, 1, rr_lo_hi(r8, r9), c2 @ TTBR 1
> mcr p15, 0, r10, c10, c2, 0 @ PRRR
> mcr p15, 0, r11, c10, c2, 1 @ NMRR
> mcr p15, 2, r12, c0, c0, 0 @ CSSELR
> @@ -413,13 +413,21 @@ vcpu .req r0 @ vcpu pointer always in r0
> ldr r9, [r2, #GICH_ELRSR1]
> ldr r10, [r2, #GICH_APR]
>
> +ARM_BE8(rev r3, r3 )
> str r3, [r11, #VGIC_CPU_HCR]
> +ARM_BE8(rev r4, r4 )
> str r4, [r11, #VGIC_CPU_VMCR]
> +ARM_BE8(rev r5, r5 )
> str r5, [r11, #VGIC_CPU_MISR]
> +ARM_BE8(rev r6, r6 )
> str r6, [r11, #VGIC_CPU_EISR]
> +ARM_BE8(rev r7, r7 )
> str r7, [r11, #(VGIC_CPU_EISR + 4)]
> +ARM_BE8(rev r8, r8 )
> str r8, [r11, #VGIC_CPU_ELRSR]
> +ARM_BE8(rev r9, r9 )
> str r9, [r11, #(VGIC_CPU_ELRSR + 4)]
> +ARM_BE8(rev r10, r10 )
> str r10, [r11, #VGIC_CPU_APR]
Wouldn't it be semantically cleaner to to the byteswap after the loads
from the hardware instead?
>
> /* Clear GICH_HCR */
> @@ -431,6 +439,7 @@ vcpu .req r0 @ vcpu pointer always in r0
> add r3, r11, #VGIC_CPU_LR
> ldr r4, [r11, #VGIC_CPU_NR_LR]
> 1: ldr r6, [r2], #4
> +ARM_BE8(rev r6, r6 )
> str r6, [r3], #4
> subs r4, r4, #1
> bne 1b
> @@ -459,8 +468,11 @@ vcpu .req r0 @ vcpu pointer always in r0
> ldr r4, [r11, #VGIC_CPU_VMCR]
> ldr r8, [r11, #VGIC_CPU_APR]
>
> +ARM_BE8(rev r3, r3 )
> str r3, [r2, #GICH_HCR]
> +ARM_BE8(rev r4, r4 )
> str r4, [r2, #GICH_VMCR]
> +ARM_BE8(rev r8, r8 )
> str r8, [r2, #GICH_APR]
>
> /* Restore list registers */
> @@ -468,6 +480,7 @@ vcpu .req r0 @ vcpu pointer always in r0
> add r3, r11, #VGIC_CPU_LR
> ldr r4, [r11, #VGIC_CPU_NR_LR]
> 1: ldr r6, [r3], #4
> +ARM_BE8(rev r6, r6 )
> str r6, [r2], #4
> subs r4, r4, #1
> bne 1b
> @@ -498,7 +511,7 @@ vcpu .req r0 @ vcpu pointer always in r0
> mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL
> isb
>
> - mrrc p15, 3, r2, r3, c14 @ CNTV_CVAL
> + mrrc p15, 3, rr_lo_hi(r2, r3), c14 @ CNTV_CVAL
> ldr r4, =VCPU_TIMER_CNTV_CVAL
> add r5, vcpu, r4
> strd r2, r3, [r5]
> @@ -538,12 +551,12 @@ vcpu .req r0 @ vcpu pointer always in r0
>
> ldr r2, [r4, #KVM_TIMER_CNTVOFF]
> ldr r3, [r4, #(KVM_TIMER_CNTVOFF + 4)]
> - mcrr p15, 4, r2, r3, c14 @ CNTVOFF
> + mcrr p15, 4, rr_lo_hi(r2, r3), c14 @ CNTVOFF
>
> ldr r4, =VCPU_TIMER_CNTV_CVAL
> add r5, vcpu, r4
> ldrd r2, r3, [r5]
> - mcrr p15, 3, r2, r3, c14 @ CNTV_CVAL
> + mcrr p15, 3, rr_lo_hi(r2, r3), c14 @ CNTV_CVAL
> isb
>
> ldr r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
> --
> 1.8.1.4
>
Thanks,
--
Christoffer
^ permalink raw reply
* [PATCH REPOST 3/5] ARM: kvm one_reg coproc set and get BE fixes
From: Christoffer Dall @ 2014-01-21 1:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1387558125-3460-4-git-send-email-victor.kamensky@linaro.org>
Hi Victor,
On Fri, Dec 20, 2013 at 08:48:43AM -0800, Victor Kamensky wrote:
> This patch fixes issue of reading and writing
interesting line break.
an issue with
> ARM V7 registers values from/to user land. Existing code was designed to
> work only in LE case.
The existing code...
'LE case'? 'little-endian'?
>
> struct kvm_one_reg
> ------------------
>
> registers value passed through kvm_one_reg structure. It is used by
registers value passed through? What are you trying to say?
> KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls. Note by looking at structure
by the KVM...
the structure
> itself we cannot tell what is size of register. Note that structure carries
the size of the register
> address of user memory, 'addr' where register should be read or written
I'm a little confused as to the value of this Section of the commit
text. I believe the ONE_REG interface is quite well documented
already...
>
> Setting register (from user-land to kvm)
> ----------------------------------------
>
> kvm_arm_set_reg takes vcpu and pointer to struct kvm_one_reg which already
> read from user space
I think you could ditch this first sentence
>
> kvm_arm_set_reg calls set_core_reg or kvm_arm_coproc_set_reg
nit: adding kvm_arm_set_reg() makes it clear that this is the function
you're refering to, and not the ioctl as a concept.
>
> set_core_reg deals only with 4 bytes registers, it just reads 4 bytes from
> user space and store it properly into vcpu->arch.regs
stores
>
> kvm_arm_coproc_set_reg deals with registers of different size. At certain
different sizes
At a certain point
> point code reaches phase where it retrieves description of register by id
the description of a register
> and it knows register size, which could be either 4 or 8 bytes. Kernel code
s/could be/is/
Kernel code is ready?
> is ready to read values from user space, but destination type may vary. It
> could be pointer to 32 bit integer or it could be pointer to 64 bit
> integer. And all possible permutation of size and destination pointer are
permutations
> possible. Depending on destination pointer type, 4 bytes or 8 bytes, two
the destination pointer type
> new helper functions are introduced - reg_from_user32 and reg_from_user64.
> They are used instead of reg_from_user function which could work only in
> LE case.
which only worked in
>
> Size sizeof(*DstInt) Function used to read from user
> 4 4 reg_from_user32
> 8 4 reg_from_user32 - read two registers
> 4 8 reg_from_user64 - need special handling for BE
> 8 8 reg_from_user64
>
> Getting register (to user-land from kvm)
> ----------------------------------------
>
> Situation with reading registers is similar to writing. Integer pointer
The situation
> type of register to be copied could be 4 or 8 bytes. And size passed in
The integer pointer
pointer to be copied? Please clarify what you are referring to.
> struct kvm_one_reg could be 4 or 8. And any permutation is possible.
Any permutation of source pointer type and size is possible.
> Depending on src pointer type, 4 bytes or 8 bytes, two new helper functions
> are introduced - reg_from_user32 and reg_to_user64. They are used instead
reg_to_user32?
> of reg_to_user function, which could work only in LE case.
the reg_to_user, which worked only for LE.
>
> Size sizeof(*SrcInt) Function used to write to user
> 4 4 reg_to_user32
> 8 4 reg_to_user32 - writes two registers
> 4 8 reg_to_user64 - need special handleing for BE
> 8 8 reg_to_user64
I think it could be slightly more helpful to put a comment on the
functions, like "Write to 32-bit user pointer" on reg_to_user32, but
it's up to you.
>
> Note code does assume that it can only deals with 4 or 8 byte registers.
Note: We only support register sizes of 4 or 8 bytes.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
I don't mean to hammer on your commit message with all my comments. I
really do appreciate you taking the time to document your changes.
However, with the level of detail you are providing in the commit, I
think you have to be slightly more careful with the language, so that it
doesn't end up being misleading instead of helpful. I think you could
sum this up much shorter to simply say that core register handling is
already endian-safe, but coprocessors and vfpregs use reg_to_user which
is not endian-safe, and therefore needs changing.
The motivation about the pointer types and register sizes being
arbitrarily different is important though, so I appreciate you listing
that.
> ---
> arch/arm/kvm/coproc.c | 94 +++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 69 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 78c0885..64b2b94 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -634,17 +634,61 @@ static struct coproc_reg invariant_cp15[] = {
> { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
> };
>
> -static int reg_from_user(void *val, const void __user *uaddr, u64 id)
> +static int reg_from_user64(u64 *val, const void __user *uaddr, u64 id)
> +{
> + unsigned long regsize = KVM_REG_SIZE(id);
> + union {
> + u32 word;
> + u64 dword;
> + } tmp = {0};
> +
> + if (copy_from_user(&tmp, uaddr, regsize) != 0)
> + return -EFAULT;
> +
> + switch (regsize) {
> + case 4:
> + *val = tmp.word;
> + break;
> + case 8:
> + *val = tmp.dword;
> + break;
> + }
> + return 0;
> +}
You stated in the commit message that any permutation of
KVM_REG_SIZE(id) and sizeof(*val) is possible.
So doesn't this totally mess up the the kernel if I pass a 32-bit
pointer to reg_from_user64? Or is that not really the case and that's
an exception to all of the permutations?
Basically you KVM_REG_SIZE(id) and sizeof your destination pointer type
should always match, but we abuse this slightly so far. I don't think
you should cater to that, but just require callers to always provide a
consistent size/type pair (you could also add a union you use as a
parameter instead, or have two typed parameter) and simplify into a
single function.
The only special cases you now have to deal with are in:
set_invariant_cp15(): declare two temp variables of u32 and u64 sizes
get_invariant_cp15(): either have temporary values or change val in
corproc_reg to be a union
The current scheme is pretty hard to understand and to make sure we're
not breaking anything...
> +
> +/* Note it may really copy two u32 registers */
> +static int reg_from_user32(u32 *val, const void __user *uaddr, u64 id)
> {
> - /* This Just Works because we are little endian. */
> if (copy_from_user(val, uaddr, KVM_REG_SIZE(id)) != 0)
> return -EFAULT;
> return 0;
> }
>
> -static int reg_to_user(void __user *uaddr, const void *val, u64 id)
> +static int reg_to_user64(void __user *uaddr, const u64 *val, u64 id)
> +{
> + unsigned long regsize = KVM_REG_SIZE(id);
> + union {
> + u32 word;
> + u64 dword;
> + } tmp;
> +
> + switch (regsize) {
> + case 4:
> + tmp.word = *val;
> + break;
> + case 8:
> + tmp.dword = *val;
> + break;
> + }
> +
> + if (copy_to_user(uaddr, &tmp, regsize) != 0)
> + return -EFAULT;
> + return 0;
> +}
> +
> +/* Note it may really copy two u32 registers */
> +static int reg_to_user32(void __user *uaddr, const u32 *val, u64 id)
> {
> - /* This Just Works because we are little endian. */
> if (copy_to_user(uaddr, val, KVM_REG_SIZE(id)) != 0)
> return -EFAULT;
> return 0;
> @@ -662,7 +706,7 @@ static int get_invariant_cp15(u64 id, void __user *uaddr)
> if (!r)
> return -ENOENT;
>
> - return reg_to_user(uaddr, &r->val, id);
> + return reg_to_user64(uaddr, &r->val, id);
> }
>
> static int set_invariant_cp15(u64 id, void __user *uaddr)
> @@ -678,7 +722,7 @@ static int set_invariant_cp15(u64 id, void __user *uaddr)
> if (!r)
> return -ENOENT;
>
> - err = reg_from_user(&val, uaddr, id);
> + err = reg_from_user64(&val, uaddr, id);
> if (err)
> return err;
>
> @@ -846,7 +890,7 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
> if (vfpid < num_fp_regs()) {
> if (KVM_REG_SIZE(id) != 8)
> return -ENOENT;
> - return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
> + return reg_to_user64(uaddr, &vcpu->arch.vfp_guest.fpregs[vfpid],
> id);
> }
>
> @@ -856,22 +900,22 @@ static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
>
> switch (vfpid) {
> case KVM_REG_ARM_VFP_FPEXC:
> - return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
> + return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpexc, id);
> case KVM_REG_ARM_VFP_FPSCR:
> - return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
> + return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpscr, id);
> case KVM_REG_ARM_VFP_FPINST:
> - return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
> + return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpinst, id);
> case KVM_REG_ARM_VFP_FPINST2:
> - return reg_to_user(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
> + return reg_to_user32(uaddr, &vcpu->arch.vfp_guest.fpinst2, id);
> case KVM_REG_ARM_VFP_MVFR0:
> val = fmrx(MVFR0);
> - return reg_to_user(uaddr, &val, id);
> + return reg_to_user32(uaddr, &val, id);
> case KVM_REG_ARM_VFP_MVFR1:
> val = fmrx(MVFR1);
> - return reg_to_user(uaddr, &val, id);
> + return reg_to_user32(uaddr, &val, id);
> case KVM_REG_ARM_VFP_FPSID:
> val = fmrx(FPSID);
> - return reg_to_user(uaddr, &val, id);
> + return reg_to_user32(uaddr, &val, id);
> default:
> return -ENOENT;
> }
> @@ -890,8 +934,8 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)
> if (vfpid < num_fp_regs()) {
> if (KVM_REG_SIZE(id) != 8)
> return -ENOENT;
> - return reg_from_user(&vcpu->arch.vfp_guest.fpregs[vfpid],
> - uaddr, id);
> + return reg_from_user64(&vcpu->arch.vfp_guest.fpregs[vfpid],
> + uaddr, id);
> }
>
> /* FP control registers are all 32 bit. */
> @@ -900,28 +944,28 @@ static int vfp_set_reg(struct kvm_vcpu *vcpu, u64 id, const void __user *uaddr)
>
> switch (vfpid) {
> case KVM_REG_ARM_VFP_FPEXC:
> - return reg_from_user(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
> + return reg_from_user32(&vcpu->arch.vfp_guest.fpexc, uaddr, id);
> case KVM_REG_ARM_VFP_FPSCR:
> - return reg_from_user(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
> + return reg_from_user32(&vcpu->arch.vfp_guest.fpscr, uaddr, id);
> case KVM_REG_ARM_VFP_FPINST:
> - return reg_from_user(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
> + return reg_from_user32(&vcpu->arch.vfp_guest.fpinst, uaddr, id);
> case KVM_REG_ARM_VFP_FPINST2:
> - return reg_from_user(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
> + return reg_from_user32(&vcpu->arch.vfp_guest.fpinst2, uaddr, id);
> /* These are invariant. */
> case KVM_REG_ARM_VFP_MVFR0:
> - if (reg_from_user(&val, uaddr, id))
> + if (reg_from_user32(&val, uaddr, id))
> return -EFAULT;
> if (val != fmrx(MVFR0))
> return -EINVAL;
> return 0;
> case KVM_REG_ARM_VFP_MVFR1:
> - if (reg_from_user(&val, uaddr, id))
> + if (reg_from_user32(&val, uaddr, id))
> return -EFAULT;
> if (val != fmrx(MVFR1))
> return -EINVAL;
> return 0;
> case KVM_REG_ARM_VFP_FPSID:
> - if (reg_from_user(&val, uaddr, id))
> + if (reg_from_user32(&val, uaddr, id))
> return -EFAULT;
> if (val != fmrx(FPSID))
> return -EINVAL;
> @@ -968,7 +1012,7 @@ int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> return get_invariant_cp15(reg->id, uaddr);
>
> /* Note: copies two regs if size is 64 bit. */
> - return reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
> + return reg_to_user32(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
> }
>
> int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> @@ -987,7 +1031,7 @@ int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> return set_invariant_cp15(reg->id, uaddr);
>
> /* Note: copies two regs if size is 64 bit */
> - return reg_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
> + return reg_from_user32(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
> }
>
> static unsigned int num_demux_regs(void)
> --
> 1.8.1.4
>
Thanks,
--
Christoffer
^ permalink raw reply
* [PATCH REPOST 4/5] ARM: kvm vgic mmio should return data in BE format in BE case
From: Christoffer Dall @ 2014-01-21 1:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1387558125-3460-5-git-send-email-victor.kamensky@linaro.org>
On Fri, Dec 20, 2013 at 08:48:44AM -0800, Victor Kamensky wrote:
> KVM mmio in BE case assumes that data it recieves is in BE format. Vgic
> operates in LE, so need byteswap data in BE case.
>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
> virt/kvm/arm/vgic.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 685fc72..7e11458 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -236,12 +236,12 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu *vcpu, int irq)
>
> static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask)
> {
> - return *((u32 *)mmio->data) & mask;
> + return le32_to_cpu(*((u32 *)mmio->data)) & mask;
> }
>
> static void mmio_data_write(struct kvm_exit_mmio *mmio, u32 mask, u32 value)
> {
> - *((u32 *)mmio->data) = value & mask;
> + *((u32 *)mmio->data) = cpu_to_le32(value) & mask;
> }
>
> /**
> --
> 1.8.1.4
>
The VGIC code is complicated enough without adding endianness logic in
its depths. I would strongly prefer that the VGIC emulation is an
endianness-agnostic software model of a device. In fact, a better fix
for this whole situation would probably be to let the vgic_handle_mmio()
function take a typed union (or a u64) instead of the byte array and
deal with any endianness conversion outside of the vgic itself.
-Christoffer
^ permalink raw reply
* [PATCH REPOST 5/5] ARM: kvm MMIO support BE host running LE code
From: Christoffer Dall @ 2014-01-21 1:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAA3XUr1=yeO=jJrWZJvrdBWbfes8+dHqkGkccjked=Racz3y_w@mail.gmail.com>
On Mon, Jan 06, 2014 at 05:59:03PM -0800, Victor Kamensky wrote:
> On 6 January 2014 14:56, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > On Mon, Jan 06, 2014 at 10:31:42PM +0000, Peter Maydell wrote:
> >> On 6 January 2014 18:20, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >> > No matter how data is stored in memory (BE, LE, or
> >> > even PDP endianness), CPU registers always have a consistent
> >> > representation. They are immune to CPU endianness change, and storing
> >> > to/reading from memory won't change the value, as long as you use the
> >> > same endianness for writing/reading.
> >>
> >> Ah, endianness. This always confuses me, but I hope the following
> >> is correct... (in all the following when I say BE I mean BE8, not BE32,
> >> since BE32 and virtualization never occur in the same CPU).
> >>
> >> Certainly registers don't have endianness, but the entire point
> >> of the CPSR.E bit is exactly that it changes the value as it is
> >> stored to / read from memory, isn't it? -- that's where and when the
> >> byte-lane flipping happens.
> >>
> >> Where this impacts the hypervisor is that instead of actually sending
> >> the data out to the bus via the byte-swapping h/w, we've trapped instead.
> >> The hypervisor reads the original data directly from the guest CPU
> >> registers, and so it's the hypervisor and userspace support code that
> >> between them have to emulate the equivalent of the byte lane
> >> swapping h/w. You could argue that it shouldn't be the kernel's
> >> job, but since the kernel has to do it for the devices it emulates
> >> internally, I'm not sure that makes much sense.
> >
> > As far as I understand, this is exactly what vcpu_data_guest_to_host and
> > vcpu_data_host_to_guest do; emulate the byte lane swapping.
> >
> > The problem is that it only works on a little-endian host with the
> > current code, because be16_to_cpu (for example), actually perform a
> > byteswap, which is what needs to be emulated. On a big-endian host, we
> > do nothing, so we end up giving a byteswapped value to the emulated
> > device.
>
> Yes, that was my point on the thread: vcpu_data_guest_to_host and
> vcpu_data_host_to_guest functions for any given host endianity should
> give opposite endian results depending on CPSR E bit value. And
> currently it is not happening in BE host case. It seems that Peter and
> you agree with that and I gave example in another email with
> dynamically switching E bit illustrating this problem for BE host.
>
> > I think a cleaner fix than this patch is to just change the
> > be16_to_cpu() to a __swab16() instead, which clearly indicates that
> > 'here is the byte lane swapping'.
>
> Yes, that may work, but it is a bit orthogonal issue.
Why? I don't think it is, I think it's addressing exactly the point at
hand.
> And I don't think
> it is better. For this to work one need to change canonical endianity on
> one of the sides around vcpu_data_guest_to_host and
> vcpu_data_host_to_guest functions.
You have to simply clearly define which format you want mmio.data to be
in. This is a user space interface across multiple architectures and
therefore something you have to consider carefully and you're limited in
choices to something that works with existing user space code.
>
> Changing it on side that faces hypervisor (code that handles guest spilled
> CPU register set) does not make sense at all - if we will keep guest CPU
> register set in memory in LE form and hypervisor runs in BE (BE host),
> code that spills registers would need to do constant byteswaps. Also any
> access by host kernel and hypervisor (all running in BE) would need to do
> byteswaps while working with guest saved registers.
>
> Changing canonical form of data on side that faces emulator and mmio
> part of kvm_run does not make sense either. kvm_run mmio.data field is
> bytes array, when it comes to host kernel from emulator, it already contains
> device memory in correct endian order that corresponds to endianity of
> emulated device. For example for LE device word read access, after call is
> emulated, mmio.data will contain mmio.data[0], mmio.data[1], mmio.data[2]
> mmio.data[3] values in LE order (mmio.data[3] is MSB). Now look at
> mmio_read_buf function introduced by Marc's 6d89d2d9 commit, this function
> will byte copy this mmio.data buffer into integer according to ongoing mmio
> access size. Note in BE host case such integer, in 'data' variable of
> kvm_handle_mmio_return function, will have byteswapped value. Now when it will
> be passed into vcpu_data_host_to_guest function, and it emulates read access
> of guest with E bit set, and if we follow your suggestion, it will be
> byteswapped.
> I.e 'data' integer will contain non byteswapped value of LE device. It will be
> further stored into some vcpu_reg register, still in native format (BE
> store), and
> further restored into guest CPU register, still non byteswapped (BE hypervisor).
> And that is not what BE client reading word of LE device expects - BE client
> knowing that it reads LE device with E bit set, it will issue additional rev
> instruction to get device memory as integer. If we really want to follow your
> suggestion, one may introduce compensatory byteswaps in mmio_read_buf
> and mmio_write_buf functions in case of BE host, rather then just do
> memcpy ... but I am not sure what it will buy us - in BE case it will swap data
> twice.
>
> Note in above description by "canonical" I mean some form of data regardless
> of current access CPSR E value. But it may differ depending on host endianess.
>
There's a lot of text to digest here, talking about a canonical form
here doesn't help; just define the layout of the destination byte array.
I also got completely lost in what you're referring to when you talk
about 'sides' here.
The thing we must decide is how the data is stored in
kvm_exit_mmio.data. See Peter's recent thread "KVM and
variable-endianness guest CPUs". Once we agree on this, the rest should
be easy (assuming we use the same structure for the data in the kernel's
internal kvm_exit_mmio declared on the stack in io_mem_abort()).
The format you suggest requires any consumer of this data to consider
the host endianness, which I don't think makes anything more clear (see
my comment on the vgic patch).
The in-kernel interface between the io_mem_abort() code and any
in-kernel emulated device must do exactly the same as the interface
between KVM and QEMU must do for KVM_EXIT_MMIO.
--
Christoffer
^ permalink raw reply
* [PATCH] ARM: asm: __und_usr_thumb need byteswap instructions in BE case
From: Stephen Boyd @ 2014-01-21 1:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140114090518.GA2222@e103592.cambridge.arm.com>
On 01/14/14 01:05, Dave Martin wrote:
> On Mon, Jan 13, 2014 at 02:30:51PM -0800, Victor Kamensky wrote:
>> __und_usr_thumb function deals with thumb2 opcodes. In case of BE
>> image, it needs to byteswap half word thumb2 encoded instructions
>> before further processing them.
>>
>> Without this fix BE image user-land thread executing first VFP
>> instruction encoded in thumb2 fails with SIGILL, because kernel
>> does not recognize instruction and does not enable VFP.
>>
>> Reported-by: Corey Melton <comelton@cisco.com>
>> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> Good spot. This looks like the correct fix to me.
>
> Acked-by: Dave Martin <Dave.Martin@arm.com>
>
Can you please send this to the patch tracker? Feel free to add
Tested-by: Stephen Boyd <sboyd@codeaurora.org>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ 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