* [PATCH v2] crypto: sun4i-ss: support the Security System PRNG
From: Corentin Labbe @ 2016-12-05 10:48 UTC (permalink / raw)
To: linux-arm-kernel
From: LABBE Corentin <clabbe.montjoie@gmail.com>
The Security System have a PRNG.
This patch add support for it as an hwrng.
Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
Changes since v1:
- Replaced all spin_lock_bh by simple spin_lock
- Removed handling of size not modulo 4 which will never happen
- Added add_random_ready_callback()
drivers/crypto/Kconfig | 8 +++
drivers/crypto/sunxi-ss/Makefile | 1 +
drivers/crypto/sunxi-ss/sun4i-ss-core.c | 14 +++++
drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c | 99 ++++++++++++++++++++++++++++++++
drivers/crypto/sunxi-ss/sun4i-ss.h | 9 +++
5 files changed, 131 insertions(+)
create mode 100644 drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c
diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 4d2b81f..38f7aca 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -538,6 +538,14 @@ config CRYPTO_DEV_SUN4I_SS
To compile this driver as a module, choose M here: the module
will be called sun4i-ss.
+config CRYPTO_DEV_SUN4I_SS_PRNG
+ bool "Support for Allwinner Security System PRNG"
+ depends on CRYPTO_DEV_SUN4I_SS
+ select HW_RANDOM
+ help
+ This driver provides kernel-side support for the Pseudo-Random
+ Number Generator found in the Security System.
+
config CRYPTO_DEV_ROCKCHIP
tristate "Rockchip's Cryptographic Engine driver"
depends on OF && ARCH_ROCKCHIP
diff --git a/drivers/crypto/sunxi-ss/Makefile b/drivers/crypto/sunxi-ss/Makefile
index 8f4c7a2..ca049ee 100644
--- a/drivers/crypto/sunxi-ss/Makefile
+++ b/drivers/crypto/sunxi-ss/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sun4i-ss.o
sun4i-ss-y += sun4i-ss-core.o sun4i-ss-hash.o sun4i-ss-cipher.o
+sun4i-ss-$(CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG) += sun4i-ss-hwrng.o
diff --git a/drivers/crypto/sunxi-ss/sun4i-ss-core.c b/drivers/crypto/sunxi-ss/sun4i-ss-core.c
index 3ac6c6c..fa739de 100644
--- a/drivers/crypto/sunxi-ss/sun4i-ss-core.c
+++ b/drivers/crypto/sunxi-ss/sun4i-ss-core.c
@@ -359,6 +359,16 @@ static int sun4i_ss_probe(struct platform_device *pdev)
}
}
platform_set_drvdata(pdev, ss);
+
+#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
+ /* Voluntarily made the PRNG optional */
+ err = sun4i_ss_hwrng_register(&ss->hwrng);
+ if (!err)
+ dev_info(ss->dev, "sun4i-ss PRNG loaded");
+ else
+ dev_err(ss->dev, "sun4i-ss PRNG failed");
+#endif
+
return 0;
error_alg:
i--;
@@ -386,6 +396,10 @@ static int sun4i_ss_remove(struct platform_device *pdev)
int i;
struct sun4i_ss_ctx *ss = platform_get_drvdata(pdev);
+#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
+ sun4i_ss_hwrng_remove(&ss->hwrng);
+#endif
+
for (i = 0; i < ARRAY_SIZE(ss_algs); i++) {
switch (ss_algs[i].type) {
case CRYPTO_ALG_TYPE_ABLKCIPHER:
diff --git a/drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c b/drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c
new file mode 100644
index 0000000..8319cae
--- /dev/null
+++ b/drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c
@@ -0,0 +1,99 @@
+#include "sun4i-ss.h"
+
+static void sun4i_ss_seed(struct random_ready_callback *rdy)
+{
+ struct sun4i_ss_ctx *ss;
+
+ ss = container_of(rdy, struct sun4i_ss_ctx, random_ready);
+ get_random_bytes(ss->seed, SS_SEED_LEN);
+ ss->random_ready.func = NULL;
+}
+
+static int sun4i_ss_hwrng_init(struct hwrng *hwrng)
+{
+ struct sun4i_ss_ctx *ss;
+ int ret;
+
+ ss = container_of(hwrng, struct sun4i_ss_ctx, hwrng);
+
+ ss->random_ready.owner = THIS_MODULE;
+ ss->random_ready.func = sun4i_ss_seed;
+
+ ret = add_random_ready_callback(&ss->random_ready);
+ switch (ret) {
+ case 0:
+ break;
+ case -EALREADY:
+ get_random_bytes(ss->seed, SS_SEED_LEN);
+ ss->random_ready.func = NULL;
+ ret = 0;
+ break;
+ default:
+ ss->random_ready.func = NULL;
+ }
+
+ return ret;
+}
+
+static int sun4i_ss_hwrng_read(struct hwrng *hwrng, void *buf,
+ size_t max, bool wait)
+{
+ int i;
+ u32 v;
+ u32 *data = buf;
+ const u32 mode = SS_OP_PRNG | SS_PRNG_CONTINUE | SS_ENABLED;
+ size_t len;
+ struct sun4i_ss_ctx *ss;
+
+ ss = container_of(hwrng, struct sun4i_ss_ctx, hwrng);
+ len = min_t(size_t, SS_DATA_LEN, max);
+
+ /* If the PRNG is not seeded */
+ if (ss->random_ready.func)
+ return -EAGAIN;
+
+ spin_lock(&ss->slock);
+
+ writel(mode, ss->base + SS_CTL);
+
+ /* write the seed */
+ for (i = 0; i < SS_SEED_LEN / 4; i++)
+ writel(ss->seed[i], ss->base + SS_KEY0 + i * 4);
+ writel(mode | SS_PRNG_START, ss->base + SS_CTL);
+
+ /* Read the random data */
+ readsl(ss->base + SS_TXFIFO, data, len / 4);
+
+ /* Update the seed */
+ for (i = 0; i < SS_SEED_LEN / 4; i++) {
+ v = readl(ss->base + SS_KEY0 + i * 4);
+ ss->seed[i] = v;
+ }
+
+ writel(0, ss->base + SS_CTL);
+ spin_unlock(&ss->slock);
+ return len;
+}
+
+int sun4i_ss_hwrng_register(struct hwrng *hwrng)
+{
+ hwrng->name = "sun4i Security System PRNG";
+ hwrng->init = sun4i_ss_hwrng_init;
+ hwrng->read = sun4i_ss_hwrng_read;
+ hwrng->quality = 1000;
+
+ /* Cannot use devm_hwrng_register() since we need to hwrng_unregister
+ * before stopping clocks/regulator
+ */
+ return hwrng_register(hwrng);
+}
+
+void sun4i_ss_hwrng_remove(struct hwrng *hwrng)
+{
+ struct sun4i_ss_ctx *ss;
+
+ ss = container_of(hwrng, struct sun4i_ss_ctx, hwrng);
+ if (ss->random_ready.func)
+ del_random_ready_callback(&ss->random_ready);
+ hwrng_unregister(hwrng);
+}
diff --git a/drivers/crypto/sunxi-ss/sun4i-ss.h b/drivers/crypto/sunxi-ss/sun4i-ss.h
index f04c0f8..85772d7 100644
--- a/drivers/crypto/sunxi-ss/sun4i-ss.h
+++ b/drivers/crypto/sunxi-ss/sun4i-ss.h
@@ -23,6 +23,7 @@
#include <linux/scatterlist.h>
#include <linux/interrupt.h>
#include <linux/delay.h>
+#include <linux/hw_random.h>
#include <crypto/md5.h>
#include <crypto/sha.h>
#include <crypto/hash.h>
@@ -125,6 +126,9 @@
#define SS_RXFIFO_EMP_INT_ENABLE (1 << 2)
#define SS_TXFIFO_AVA_INT_ENABLE (1 << 0)
+#define SS_SEED_LEN (192 / 8)
+#define SS_DATA_LEN (160 / 8)
+
struct sun4i_ss_ctx {
void __iomem *base;
int irq;
@@ -134,6 +138,9 @@ struct sun4i_ss_ctx {
struct device *dev;
struct resource *res;
spinlock_t slock; /* control the use of the device */
+ struct random_ready_callback random_ready;
+ struct hwrng hwrng;
+ u32 seed[SS_SEED_LEN / 4];
};
struct sun4i_ss_alg_template {
@@ -199,3 +206,5 @@ int sun4i_ss_des_setkey(struct crypto_ablkcipher *tfm, const u8 *key,
unsigned int keylen);
int sun4i_ss_des3_setkey(struct crypto_ablkcipher *tfm, const u8 *key,
unsigned int keylen);
+int sun4i_ss_hwrng_register(struct hwrng *hwrng);
+void sun4i_ss_hwrng_remove(struct hwrng *hwrng);
--
2.7.3
^ permalink raw reply related
* [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings
From: Thierry Reding @ 2016-12-05 10:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161205083535.GB14004@dell>
On Mon, Dec 05, 2016 at 08:35:35AM +0000, Lee Jones wrote:
> On Mon, 05 Dec 2016, Thierry Reding wrote:
>
> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> > > Define bindings for pwm-stm32
> > >
> > > version 2:
> > > - use parameters instead of compatible of handle the hardware configuration
> > >
> > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> > > ---
> > > .../devicetree/bindings/pwm/pwm-stm32.txt | 38 ++++++++++++++++++++++
> > > 1 file changed, 38 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > > new file mode 100644
> > > index 0000000..575b9fb
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> > > @@ -0,0 +1,38 @@
> > > +STMicroelectronics PWM driver bindings for STM32
> >
> > Technically this bindings describe devices, so "driver binding" is a
> > somewhat odd wording. Perhaps:
> >
> > STMicroelectronics STM32 General Purpose Timer PWM bindings
> >
> > ?
> >
> > > +
> > > +Must be a sub-node of STM32 general purpose timer driver
> > > +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
> >
> > Again, "driver parent node" is odd. Perhaps:
> >
> > Must be a sub-node of an STM32 General Purpose Timer device tree
> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
> > the parent node.
> >
> > ?
> >
> > > +Required parameters:
> > > +- compatible: Must be "st,stm32-pwm"
> > > +- pinctrl-names: Set to "default".
> > > +- pinctrl-0: List of phandles pointing to pin configuration nodes
> > > + for PWM module.
> > > + For Pinctrl properties, please refer to [1].
> >
> > Your indentation and capitalization are inconsistent. Also, please refer
> > to the pinctrl bindings by relative path and inline, rather than as a
> > footnote reference.
> >
> > > +
> > > +Optional parameters:
> > > +- st,breakinput: Set if the hardware have break input capabilities
> > > +- st,breakinput-polarity: Set break input polarity. Default is 0
> > > + The value define the active polarity:
> > > + - 0 (active LOW)
> > > + - 1 (active HIGH)
> >
> > Could we fold these into a single property? If st,breakinput-polarity is
> > not present it could simply mean that there is no break input, and if it
> > is present you don't have to rely on a default.
> >
> > > +- st,pwm-num-chan: Number of available PWM channels. Default is 0.
> >
> > The pwm- prefix is rather redundant since the node is already named pwm.
> > Why not simply st,channels? Or simply channels, since it's not really
> > anything specific to this hardware.
> >
> > Come to think of it, might be worth having a discussion with our DT
> > gurus about what their stance is on using the # as prefix for numbers
> > (such as in #address-cells or #size-cells). This could be #channels to
> > mark it more explicitly as representing a count.
>
> Unfortunately that ship has sailed.
>
> st,pwm-num-chan already exists (with your blessing). It's usually
I think I did at the time object, though very mildly. The property here
is somewhat different, though. For one this is a PWM specific node, so
the pwm- prefix is completely redundant. Also for pwm-sti where you had
introduced st,pwm-num-chan, the property denoted how many PWM channels
vs. capture channels (st,capture-num-chan) the device was supposed to
use. Here there are only one type of channels.
> suggested to reuse exiting properties when writing new bindings.
Given the above I think this case is different. Further my understanding
is that the desire to reuse existing properties is primarily for generic
properties. Vendor specific properties are always going to have to be
defined in the specific bindings, so it doesn't matter very much whether
they are reused or not.
Lastly, I think st,pwm-num-chan is not optimal, and while it isn't very
bad either, I do believe that when we see ways of improving things then
we should do so, regardless of whether existing ways to describe things
already exist. Especially if it comes at no additional cost.
All of that said, this is my opinion and if everybody thinks that the
st,pwm-num-chan is the better choice I'll merge it. Anyway, we'll need
the Acked-by from one of the device tree bindings maintainers and I'd
like to see at least an attempt at a discussion.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161205/36f76394/attachment.sig>
^ permalink raw reply
* next-20161205 build: 3 failures 4 warnings (next-20161205)
From: Marc Zyngier @ 2016-12-05 10:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161205104455.2m5ky3hgd5n2kb2w@sirena.org.uk>
On 05/12/16 10:44, Mark Brown wrote:
> On Mon, Dec 05, 2016 at 07:56:06AM +0000, Build bot for Mark Brown wrote:
>
> Today's -next fails to build an arm64 allmodconfig:
>
>> arm64-allmodconfig
>> ../arch/arm64/include/asm/probes.h:18:25: fatal error: asm/opcodes.h: No such file or directory
>
> due to bca8f17f57bd76d (arm64: Get rid of asm/opcodes.h) having missed
> one reference to the header.
Fix on the list: https://www.spinics.net/lists/arm-kernel/msg546960.html
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH] i2c: rk3x: keep i2c irq ON in suspend
From: Heiko Stuebner @ 2016-12-05 10:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1480924979-13450-1-git-send-email-david.wu@rock-chips.com>
Hi David,
Am Montag, 5. Dezember 2016, 16:02:59 CET schrieb David Wu:
> During suspend there may still be some i2c access happening.
> And if we don't keep i2c irq ON, there may be i2c access timeout if
> i2c is in irq mode of operation.
can you describe the issue you're trying to fix a bit more please?
I.e. I'd think the i2c-core does suspend i2c-client devices first, so that
these should be able to finish up their ongoing transfers and not start any
new ones instead?
Your irq can still happen slightly after the system started going to actually
sleep, so to me it looks like you just widened the window where irqs can be
handled. Especially as your irq could also just simply stem from the start
state, so you cannot even be sure if your transaction actually is finished.
So to me it looks like the i2c-connected device driver should be fixed instead?
In the past I solved this for example in the zforce_ts driver [0] to
prevent i2c transfers from happening during suspend.
Heiko
[0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/zforce_ts.c
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
> drivers/i2c/busses/i2c-rk3x.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index df22066..67af32a 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -1261,7 +1261,7 @@ static int rk3x_i2c_probe(struct platform_device
> *pdev) }
>
> ret = devm_request_irq(&pdev->dev, irq, rk3x_i2c_irq,
> - 0, dev_name(&pdev->dev), i2c);
> + IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c);
> if (ret < 0) {
> dev_err(&pdev->dev, "cannot request IRQ\n");
> return ret;
^ permalink raw reply
* [PATCH v9 07/16] drivers: acpi: implement acpi_dma_configure
From: Lorenzo Pieralisi @ 2016-12-05 10:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <003001d24edd$40d7d030$c2877090$@codeaurora.org>
On Mon, Dec 05, 2016 at 03:22:02PM +0530, Sricharan wrote:
> Hi Lorenzo,
>
> >
> >On Sat, Dec 3, 2016 at 11:39 AM, Lorenzo Pieralisi
> ><lorenzo.pieralisi@arm.com> wrote:
> >> On Sat, Dec 03, 2016 at 03:11:09AM +0100, Rafael J. Wysocki wrote:
> >>> On Fri, Dec 2, 2016 at 4:38 PM, Lorenzo Pieralisi
> >>> <lorenzo.pieralisi@arm.com> wrote:
> >>> > Rafael, Mark, Suravee,
> >>> >
> >>> > On Mon, Nov 21, 2016 at 10:01:39AM +0000, Lorenzo Pieralisi wrote:
> >>> >> On DT based systems, the of_dma_configure() API implements DMA
> >>> >> configuration for a given device. On ACPI systems an API equivalent to
> >>> >> of_dma_configure() is missing which implies that it is currently not
> >>> >> possible to set-up DMA operations for devices through the ACPI generic
> >>> >> kernel layer.
> >>> >>
> >>> >> This patch fills the gap by introducing acpi_dma_configure/deconfigure()
> >>> >> calls that for now are just wrappers around arch_setup_dma_ops() and
> >>> >> arch_teardown_dma_ops() and also updates ACPI and PCI core code to use
> >>> >> the newly introduced acpi_dma_configure/acpi_dma_deconfigure functions.
> >>> >>
> >>> >> Since acpi_dma_configure() is used to configure DMA operations, the
> >>> >> function initializes the dma/coherent_dma masks to sane default values
> >>> >> if the current masks are uninitialized (also to keep the default values
> >>> >> consistent with DT systems) to make sure the device has a complete
> >>> >> default DMA set-up.
> >>> >
> >>> > I spotted a niggle that unfortunately was hard to spot (and should not
> >>> > be a problem per se but better safe than sorry) and I am not comfortable
> >>> > with it.
> >>> >
> >>> > Following commit d0562674838c ("ACPI / scan: Parse _CCA and setup
> >>> > device coherency") in acpi_bind_one() we check if the acpi_device
> >>> > associated with a device just added supports DMA, first it was
> >>> > done with acpi_check_dma() and then commit 1831eff876bd ("device
> >>> > property: ACPI: Make use of the new DMA Attribute APIs") changed
> >>> > it to acpi_get_dma_attr().
> >>> >
> >>> > The subsequent check (attr != DEV_DMA_NOT_SUPPORTED) is always true
> >>> > on _any_ acpi device we pass to acpi_bind_one() on x86, which was
> >>> > fine because we used it to call arch_setup_dma_ops(), which is a nop
> >>> > on x86. On ARM64 a _CCA method is required to define if a device
> >>> > supports DMA so (attr != DEV_DMA_NOT_SUPPORTED) may well be false.
> >>> >
> >>> > Now, acpi_bind_one() is used to bind an acpi_device to its physical
> >>> > node also for pseudo-devices like cpus and memory nodes. For those
> >>> > objects, on x86, attr will always be != DEV_DMA_NOT_SUPPORTED.
> >>> >
> >>> > So far so good, because on x86 arch_setup_dma_ops() is empty code.
> >>> >
> >>> > With this patch, I use the (attr != DEV_DMA_NOT_SUPPORTED) check
> >>> > to call acpi_dma_configure() which is basically a nop on x86 except
> >>> > that it sets up the dma_mask/coherent_dma_mask to a sane default value
> >>> > (after all we are setting up DMA for the device so it makes sense to
> >>> > initialize the masks there if they were unset since we are configuring
> >>> > DMA for the device in question) for the given device.
> >>> >
> >>> > Problem is, as per the explanation above, we are also setting the
> >>> > default dma masks for pseudo-devices (eg CPUs) that were previously
> >>> > untouched, it should not be a problem per-se but I am not comfortable
> >>> > with that, honestly it does not make much sense.
> >>> >
> >>> > An easy "fix" would be to move the default dma masks initialization out
> >>> > of acpi_dma_configure() (as it was in previous patch versions of this
> >>> > series - I moved it to acpi_dma_configure() just a consolidation point
> >>> > for initializing the masks instead of scattering them in every
> >>> > acpi_dma_configure caller) I can send this as a fix-up patch to Joerg if
> >>> > we think that's the right thing to do (or I can send it to Rafael later
> >>> > when the code is in the merged depending on the timing) just let me
> >>> > know please.
> >>>
> >>> Why can't arch_setup_dma_ops() set those masks too?
> >>
> >> Because the dma masks set-up is done by the caller (see
> >> of_dma_configure()) according to firmware configuration or
> >> platform data knowledge. I wanted to replicate the of_dma_configure()
> >> interface on ACPI for obvious reasons (on ARM systems), I stopped
> >> short of adding ACPI code to mirror of_dma_get_range() equivalent
> >> (through the _DMA object) but I am really really nervous about changing
> >> the code path on x86 because in theory all is fine, in practice even
> >> just setting the masks to sane values can have unexpected consequences,
> >> I just can't know (that's why I wasn't doing it in the first iterations
> >> of this series).
> >>
> >> Side note: DT with of_dma_configure() and ACPI with
> >> acpi_create_platform_device() set the default dma mask for all
> >> platform devices already _regardless_ of what they really are, though
> >> arguably acpi_bind_one() touches ways more devices.
> >>
> >> I really think that removing the default dma masks settings from
> >> acpi_dma_configure() is the safer thing to do for the time being (or
> >> moving acpi_dma_configure() to acpi_create_platform_device(), where the
> >> DMA masks are set-up by default by core ACPI. Mark, Suravee, what was
> >> the rationale behind calling arch_setup_dma_ops() in acpi_bind_one() ?)
> >
> >Alternatively, you can add one more arch wrapper that will be a no-op
> >on x86 and that will set up the default masks and call
> >arch_setup_dma_ops() on ARM. Then, you can invoke that from
> >acpi_dma_configure().
> >
> >Or make the definition of acpi_dma_configure() itself depend on the
> >architecture.
> >
>
> So is it better that either removing the masks from acpi_dma_configure
> (or) creating the wrapper as Rafael mentioned, than moving
> acpi_dma_configure itself , because with something like iommu probe
> deferral that is tried, acpi_dma_configure is getting invoked from a
> device's really_probe, a different path again ?
Yes, I thought about that too. Given what I said above (ie I would like
to extend the mask set-up with _DMA object - that is generic ACPI but
can affect legacy x86 - and if that does not work through IORT specific
bindings), as per Rafael suggestion I added an iort_set_dma_mask wrapper
that is a NOP on x86, leaving acpi_dma_configure() unchanged for ARM64.
Patch coming, thanks everyone.
Lorenzo
^ permalink raw reply
* [PATCH 2/2] arm64: alternatives: Work around NOP generation with broken assembler
From: Marc Zyngier @ 2016-12-05 10:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161205100545.GA14058@arm.com>
On 05/12/16 10:05, Will Deacon wrote:
> On Sat, Dec 03, 2016 at 02:05:38PM +0000, Marc Zyngier wrote:
>> When compiling a .inst directive in an alternative clause with
>> a rather old version of gas (circa 2.24), and when used with
>> the alternative_else_nop_endif feature, the compilation fails
>> with a rather cryptic message such as:
>>
>> arch/arm64/lib/clear_user.S:33: Error: bad or irreducible absolute expression
>>
>> which is caused by the bug described in eb7c11ee3c5c ("arm64:
>> alternative: Work around .inst assembler bugs").
>>
>> This effectively prevents the use of the "nops" macro, which
>> requires the number of instruction as a parameter (the assembler
>> is confused and unable to compute the difference between two labels).
>>
>> As an alternative(!), use the .fill directive to output the number
>> of required NOPs (.fill has the good idea to output the fill pattern
>> in the endianness of the instruction stream).
>
> Are you sure about that? The gas docs say:
>
> `The contents of each repeat bytes is taken from an 8-byte number. The
> highest order 4 bytes are zero. The lowest order 4 bytes are value rendered
> in the byte-order of an integer on the computer as is assembling for.'
>
> and I'd expect "integer" to refer to data values, rather than instructions.
My tests on 2.24 and 2.25 seem to show that the output is always LE,
which could be another GAS issue. I've asked the binutils people for
information.
>
>>
>> Fixes: 792d47379f4d ("arm64: alternative: add auto-nop infrastructure")
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> arch/arm64/include/asm/alternative.h | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
>> index 39feb85..39f8c98 100644
>> --- a/arch/arm64/include/asm/alternative.h
>> +++ b/arch/arm64/include/asm/alternative.h
>> @@ -159,9 +159,19 @@ void apply_alternatives(void *start, size_t length);
>> * of NOPs. The number of NOPs is chosen automatically to match the
>> * previous case.
>> */
>> +
>> +#define NOP_INST 0xd503201f
>
> If we define this, let's put it in insn.h or something. It could even be
> used in the vdso linker script, which open codes this constant.
Sure.
>
>> +
>> .macro alternative_else_nop_endif
>> alternative_else
>> - nops (662b-661b) / AARCH64_INSN_SIZE
>> + /*
>> + * The same assembler bug strikes again here if the first half
>> + * of the alternative sequence contains a .inst, leading to a
>> + * bizarre error message. Fortulately, .fill replaces the
>
> Fortunately
>
>> + * "nops" macro by inserting padding with the target machine
>> + * endianness.
>> + */
>> + .fill (662b-661b) / AARCH64_INSN_SIZE, AARCH64_INSN_SIZE, NOP_INST
>
> Assuming we can get .fill to do the right thing, we should probably use
> it in __nops rather than open-coding it here. Or is the issue that we're
> unable to pass (662b-661b) / AARCH64_INSN_SIZE to any macro?
Having an intermediate macro seems to work (probably because this is not
actually evaluated until it reaches .fill). I'll update the patch if I
get confirmation of the validity of the workaround from the binutils people.
Otherwise, we'll have to find another alternative, and maybe go back to
not using the auto-nop for SET_PSTATE_PAN/UAO.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board
From: Icenowy Zheng @ 2016-12-05 11:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161205094023.mtymfvpmca4x3ohh@lukather>
05.12.2016, 17:40, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> On Mon, Dec 05, 2016 at 04:59:44PM +0800, Icenowy Zheng wrote:
>> ?2016?12?5? 16:52? Maxime Ripard <maxime.ripard@free-electrons.com>???
>> ?>
>> ?> On Fri, Dec 02, 2016 at 10:22:30PM +0800, Icenowy Zheng wrote:
>> ?> >
>> ?> >
>> ?> > 01.12.2016, 17:36, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
>> ?> > > On Mon, Nov 28, 2016 at 12:29:07AM +0000, Andr? Przywara wrote:
>> ?> > >> ?> Something more interesting happened.
>> ?> > >> ?>
>> ?> > >> ?> Xunlong made a add-on board for Orange Pi Zero, which exposes the
>> ?> > >> ?> two USB Controllers exported at expansion bus as USB Type-A
>> ?> > >> ?> connectors.
>> ?> > >> ?>
>> ?> > >> ?> Also it exposes a analog A/V jack and a microphone.
>> ?> > >> ?>
>> ?> > >> ?> Should I enable {e,o}hci{2.3} in the device tree?
>> ?> > >>
>> ?> > >> ?Actually we should do this regardless of this extension board. The USB
>> ?> > >> ?pins are not multiplexed and are exposed on user accessible pins (just
>> ?> > >> ?not soldered, but that's a detail), so I think they qualify for DT
>> ?> > >> ?enablement. And even if a user can't use them, it doesn't hurt to have
>> ?> > >> ?them (since they are not multiplexed).
>> ?> > >
>> ?> > > My main concern about this is that we'll leave regulators enabled by
>> ?> > > default, for a minority of users. And that minority will prevent to do
>> ?> > > a proper power management when the times come since we'll have to keep
>> ?> > > that behaviour forever.
>> ?> >
>> ?> > I think these users can add a 'fdt set /xxx/xxx status "disabled" ' .
>> ?>
>> ?> You can't ask that from the majority of users. These users will take
>> ?> debian or fedora, install it, and expect everything to work
>> ?> properly. I would make the opposite argument actually. If someone is
>> ?> knowledgeable enough to solder the USB pins a connector, then (s)he'll
>> ?> be able to make that u-boot call.
>>
>> ?Now (s)he do not need soldering.
>>
>> ?(S)he needs only paying $1.99 more to Xunlong to get the expansion
>> ?board, and insert it on the OPi Zero.
>
> Which is going to require an overlay anyway, so we could have the USB
> bits in there too.
If so, I think the [PATCH -next v3 2/2] is ready to be merged ;-)
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
^ permalink raw reply
* [PATCH v3 4/7] PWM: add pwm driver for stm32 plaftorm
From: Benjamin Gaignard @ 2016-12-05 11:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161205072357.GB18069@ulmo.ba.sec>
2016-12-05 8:23 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
> On Fri, Dec 02, 2016 at 11:17:19AM +0100, Benjamin Gaignard wrote:
>> This driver add support for pwm driver on stm32 platform.
>
> "adds". Also please use PWM in prose because it's an abbreviation.
>
>> The SoC have multiple instances of the hardware IP and each
>> of them could have small differences: number of channels,
>> complementary output, counter register size...
>> Use DT parameters to handle those differentes configuration
>
> "different configurations"
ok
>
>>
>> version 2:
>> - only keep one comptatible
>> - use DT paramaters to discover hardware block configuration
>
> "parameters"
ok
>
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>> drivers/pwm/Kconfig | 8 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-stm32.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 294 insertions(+)
>> create mode 100644 drivers/pwm/pwm-stm32.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index bf01288..a89fdba 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -388,6 +388,14 @@ config PWM_STI
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-sti.
>>
>> +config PWM_STM32
>> + bool "STMicroelectronics STM32 PWM"
>> + depends on ARCH_STM32
>> + depends on OF
>> + select MFD_STM32_GP_TIMER
>
> Should that be a "depends on"?
I think select is fine because the wanted feature is PWM not the mfd part
>
>> + help
>> + Generic PWM framework driver for STM32 SoCs.
>> +
>> config PWM_STMPE
>> bool "STMPE expander PWM export"
>> depends on MFD_STMPE
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 1194c54..5aa9308 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
>> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
>> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
>> obj-$(CONFIG_PWM_STI) += pwm-sti.o
>> +obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
>> obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
>> obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
>> obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
>> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
>> new file mode 100644
>> index 0000000..a362f63
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-stm32.c
>> @@ -0,0 +1,285 @@
>> +/*
>> + * Copyright (C) STMicroelectronics 2016
>> + * Author: Gerald Baeza <gerald.baeza@st.com>
>
> Could use a blank line between the above. Also, please use a single
> space after : for consistency.
ok
>
>> + * License terms: GNU General Public License (GPL), version 2
>
> Here too.
OK
>
>> + *
>> + * Inspired by timer-stm32.c from Maxime Coquelin
>> + * pwm-atmel.c from Bo Shen
>> + */
>> +
>> +#include <linux/of.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>
> Please sort these alphabetically.
ok
>> +
>> +#include <linux/mfd/stm32-gptimer.h>
>> +
>> +#define DRIVER_NAME "stm32-pwm"
>> +
>> +#define CAP_COMPLEMENTARY BIT(0)
>> +#define CAP_32BITS_COUNTER BIT(1)
>> +#define CAP_BREAKINPUT BIT(2)
>> +#define CAP_BREAKINPUT_POLARITY BIT(3)
>
> Just make these boolean. Makes the conditionals a lot simpler to read.
I will do that for v4
>> +
>> +struct stm32_pwm_dev {
>
> No need for the _dev suffix.
>
>> + struct device *dev;
>> + struct clk *clk;
>> + struct regmap *regmap;
>> + struct pwm_chip chip;
>
> It's slightly more efficient to put this as first field because then
> to_stm32_pwm() becomes a no-op.
Ok I will remove it
>
>> + int caps;
>> + int npwm;
>
> unsigned int, please.
>
>> + u32 polarity;
>
> Maybe use a prefix here to stress that it is the polarity of the
> complementary output. Otherwise one might take it for the PWM signal's
> polarity that's already part of the PWM state.
I will rename it
>
>> +};
>> +
>> +#define to_stm32_pwm_dev(x) container_of(chip, struct stm32_pwm_dev, chip)
>
> Please turn this into a static inline.
with putting struct pwm_chip as first filed I will just cast the structure
>> +
>> +static u32 __active_channels(struct stm32_pwm_dev *pwm_dev)
>
> No need for a __ prefix.
I wlll remove it
>
>> +{
>> + u32 ccer;
>> +
>> + regmap_read(pwm_dev->regmap, TIM_CCER, &ccer);
>> +
>> + return ccer & TIM_CCER_CCXE;
>> +}
>> +
>> +static int write_ccrx(struct stm32_pwm_dev *dev, struct pwm_device *pwm,
>> + u32 ccr)
>
> u32 value, perhaps? I first mistook this to be a register offset.
OK
>
>> +{
>> + switch (pwm->hwpwm) {
>> + case 0:
>> + return regmap_write(dev->regmap, TIM_CCR1, ccr);
>> + case 1:
>> + return regmap_write(dev->regmap, TIM_CCR2, ccr);
>> + case 2:
>> + return regmap_write(dev->regmap, TIM_CCR3, ccr);
>> + case 3:
>> + return regmap_write(dev->regmap, TIM_CCR4, ccr);
>> + }
>> + return -EINVAL;
>> +}
>> +
>> +static int stm32_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> + int duty_ns, int period_ns)
>
> Please implement this as an atomic PWM driver, I don't want new drivers
> to use the legacy callbacks.
Ok I will just use .apply ops.
>
>> +{
>> + struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
>
> I think something like "stm", or "priv" would be more appropriate here.
> If you ever need access to a struct device, you'll be hard-pressed to
> find a good name for it.
I will use priv
>
>> + unsigned long long prd, div, dty;
>> + int prescaler = 0;
>
> If this can never be negative, please make it unsigned int.
>
>> + u32 max_arr = 0xFFFF, ccmr, mask, shift, bdtr;
>> +
>> + if (dev->caps & CAP_32BITS_COUNTER)
>> + max_arr = 0xFFFFFFFF;
>
> I prefer lower-case hexadecimal digits.
I have found a way to remove this code
>
>> +
>> + /* Period and prescaler values depends of clock rate */
>
> "depend on"
fixed
>
>> + div = (unsigned long long)clk_get_rate(dev->clk) * period_ns;
>> +
>> + do_div(div, NSEC_PER_SEC);
>> + prd = div;
>> +
>> + while (div > max_arr) {
>> + prescaler++;
>> + div = prd;
>> + do_div(div, (prescaler + 1));
>> + }
>> + prd = div;
>
> Blank line after blocks, please.
fixed
>
>> +
>> + if (prescaler > MAX_TIM_PSC) {
>> + dev_err(chip->dev, "prescaler exceeds the maximum value\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* All channels share the same prescaler and counter so
>> + * when two channels are active at the same we can't change them
>> + */
>
> This isn't proper block comment style. Also, please use all of the
> columns at your disposal.
done
>
>> + if (__active_channels(dev) & ~(1 << pwm->hwpwm * 4)) {
>> + u32 psc, arr;
>> +
>> + regmap_read(dev->regmap, TIM_PSC, &psc);
>> + regmap_read(dev->regmap, TIM_ARR, &arr);
>> +
>> + if ((psc != prescaler) || (arr != prd - 1))
>> + return -EINVAL;
>
> Maybe -EBUSY to differentiate from other error cases?
done
>
>> + }
>> +
>> + regmap_write(dev->regmap, TIM_PSC, prescaler);
>> + regmap_write(dev->regmap, TIM_ARR, prd - 1);
>> + regmap_update_bits(dev->regmap, TIM_CR1, TIM_CR1_ARPE, TIM_CR1_ARPE);
>> +
>> + /* Calculate the duty cycles */
>> + dty = prd * duty_ns;
>> + do_div(dty, period_ns);
>> +
>> + write_ccrx(dev, pwm, dty);
>> +
>> + /* Configure output mode */
>> + shift = (pwm->hwpwm & 0x1) * 8;
>> + ccmr = (TIM_CCMR_PE | TIM_CCMR_M1) << shift;
>> + mask = 0xFF << shift;
>> +
>> + if (pwm->hwpwm & 0x2)
>
> This looks as though TIM_CCMR1 is used for channels 0 and 1, while
> TIM_CCMR2 is used for channels 2 and 3. Wouldn't it be more natural to
> make the conditional above:
>
> if (pwm->hwpwm >= 2)
>
> ? Or perhaps better yet:
>
> if (pwm->hwpwm < 2)
> /* update TIM_CCMR1 */
> else
> /* update TIM_CCMR2 */
I will code it that, thanks.
>
> The other alternative, which might make the code slightly more readable,
> would be to get rid of all these conditionals by parameterizing the
> offsets per PWM channel. The PWM subsystem has a means of storing per-
> channel chip-specific data (see pwm_{set,get}_chip_data()). It might be
> a little overkill for this particular driver, given that the number of
> conditionals is fairly small.
>
>> + regmap_update_bits(dev->regmap, TIM_CCMR2, mask, ccmr);
>> + else
>> + regmap_update_bits(dev->regmap, TIM_CCMR1, mask, ccmr);
>> +
>> + if (!(dev->caps & CAP_BREAKINPUT))
>> + return 0;
>
> If you make these capabilities boolean, it becomes much more readable,
> especially for negations:
Yes I will fix that
>
> if (!dev->has_breakinput)
>
>> +
>> + bdtr = TIM_BDTR_MOE | TIM_BDTR_AOE;
>> +
>> + if (dev->caps & CAP_BREAKINPUT_POLARITY)
>> + bdtr |= TIM_BDTR_BKE;
>> +
>> + if (dev->polarity)
>> + bdtr |= TIM_BDTR_BKP;
>> +
>> + regmap_update_bits(dev->regmap, TIM_BDTR,
>> + TIM_BDTR_MOE | TIM_BDTR_AOE |
>> + TIM_BDTR_BKP | TIM_BDTR_BKE,
>> + bdtr);
>> +
>> + return 0;
>> +}
>> +
>> +static int stm32_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>> + enum pwm_polarity polarity)
>> +{
>> + u32 mask;
>> + struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
>> +
>> + mask = TIM_CCER_CC1P << (pwm->hwpwm * 4);
>> + if (dev->caps & CAP_COMPLEMENTARY)
>> + mask |= TIM_CCER_CC1NP << (pwm->hwpwm * 4);
>> +
>> + regmap_update_bits(dev->regmap, TIM_CCER, mask,
>> + polarity == PWM_POLARITY_NORMAL ? 0 : mask);
>> +
>> + return 0;
>> +}
>> +
>> +static int stm32_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + u32 mask;
>> + struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
>> +
>> + clk_enable(dev->clk);
>> +
>> + /* Enable channel */
>> + mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
>> + if (dev->caps & CAP_COMPLEMENTARY)
>> + mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
>> +
>> + regmap_update_bits(dev->regmap, TIM_CCER, mask, mask);
>> +
>> + /* Make sure that registers are updated */
>> + regmap_update_bits(dev->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
>> +
>> + /* Enable controller */
>> + regmap_update_bits(dev->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
>> +
>> + return 0;
>> +}
>> +
>> +static void stm32_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + u32 mask;
>> + struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
>> +
>> + /* Disable channel */
>> + mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
>> + if (dev->caps & CAP_COMPLEMENTARY)
>> + mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
>> +
>> + regmap_update_bits(dev->regmap, TIM_CCER, mask, 0);
>> +
>> + /* When all channels are disabled, we can disable the controller */
>> + if (!__active_channels(dev))
>> + regmap_update_bits(dev->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>> +
>> + clk_disable(dev->clk);
>> +}
>
> All of the above can be folded into the ->apply() hook for atomic PWM
> support.
>
> Also, in the above you use clk_enable() in the ->enable() callback and
> clk_disable() in ->disable(). If you need the clock to access registers
> you'll have to enabled it in the others as well because there are no
> guarantees that configuration will only happen between ->enable() and
> ->disable(). Atomic PWM simplifies this a bit, but you still need to be
> careful about when to enable the clock in your ->apply() hook.
I have used regmap functions enable/disable clk for me when accessing to
the registers.
I only have to take care of clk enable/disable when PWM state change.
>
>> +
>> +static const struct pwm_ops stm32pwm_ops = {
>> + .config = stm32_pwm_config,
>> + .set_polarity = stm32_pwm_set_polarity,
>> + .enable = stm32_pwm_enable,
>> + .disable = stm32_pwm_disable,
>> +};
>
> You need to set the .owner field as well.
OK and I will remove legacy ops to use only apply.
>
>> +
>> +static int stm32_pwm_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + struct stm32_gptimer_dev *mfd = dev_get_drvdata(pdev->dev.parent);
>> + struct stm32_pwm_dev *pwm;
>> + int ret;
>> +
>> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
>> + if (!pwm)
>> + return -ENOMEM;
>> +
>> + pwm->regmap = mfd->regmap;
>> + pwm->clk = mfd->clk;
>> +
>> + if (!pwm->regmap || !pwm->clk)
>> + return -EINVAL;
>> +
>> + if (of_property_read_bool(np, "st,complementary"))
>> + pwm->caps |= CAP_COMPLEMENTARY;
>> +
>> + if (of_property_read_bool(np, "st,32bits-counter"))
>> + pwm->caps |= CAP_32BITS_COUNTER;
>> +
>> + if (of_property_read_bool(np, "st,breakinput"))
>> + pwm->caps |= CAP_BREAKINPUT;
>> +
>> + if (!of_property_read_u32(np, "st,breakinput-polarity", &pwm->polarity))
>> + pwm->caps |= CAP_BREAKINPUT_POLARITY;
>> +
>> + of_property_read_u32(np, "st,pwm-num-chan", &pwm->npwm);
>> +
>> + pwm->chip.base = -1;
>> + pwm->chip.dev = dev;
>> + pwm->chip.ops = &stm32pwm_ops;
>> + pwm->chip.npwm = pwm->npwm;
>> +
>> + ret = pwmchip_add(&pwm->chip);
>> + if (ret < 0)
>> + return ret;
>> +
>> + platform_set_drvdata(pdev, pwm);
>> +
>> + return 0;
>> +}
>> +
>> +static int stm32_pwm_remove(struct platform_device *pdev)
>> +{
>> + struct stm32_pwm_dev *pwm = platform_get_drvdata(pdev);
>> + int i;
>
> unsigned int, please.
OK
>
>> +
>> + for (i = 0; i < pwm->npwm; i++)
>> + pwm_disable(&pwm->chip.pwms[i]);
>> +
>> + pwmchip_remove(&pwm->chip);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id stm32_pwm_of_match[] = {
>> + {
>> + .compatible = "st,stm32-pwm",
>> + },
>
> The above can be collapsed into a single line.
OK
>
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32_pwm_of_match);
>> +
>> +static struct platform_driver stm32_pwm_driver = {
>> + .probe = stm32_pwm_probe,
>> + .remove = stm32_pwm_remove,
>> + .driver = {
>> + .name = DRIVER_NAME,
>> + .of_match_table = stm32_pwm_of_match,
>> + },
>> +};
>
> Please don't use tabs for padding within the structure definition since
> it doesn't align properly anyway.
OK
>
>> +module_platform_driver(stm32_pwm_driver);
>> +
>> +MODULE_ALIAS("platform:" DRIVER_NAME);
>> +MODULE_DESCRIPTION("STMicroelectronics STM32 PWM driver");
>> +MODULE_LICENSE("GPL");
>
> According to the header comment this should be "GPL v2".
done
>
> Thanks,
> Thierry
^ permalink raw reply
* [GIT PULL 1/2] DaVinci DT updates for v4.10 (part 4)
From: Sekhar Nori @ 2016-12-05 11:03 UTC (permalink / raw)
To: linux-arm-kernel
The following changes since commit 878e908ad95637dc6567a9b5f6876a580ae90dfa:
ARM: dts: da850: enable memctrl and mstpri nodes per board (2016-11-28 15:51:29 +0530)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/nsekhar/linux-davinci.git tags/davinci-for-v4.10/dt-4
for you to fetch changes up to e4ce904d6289f2a72affd2a0f0a44da6e5d0cce4:
ARM: dts: da850: enable high speed for mmc (2016-12-05 14:20:44 +0530)
----------------------------------------------------------------
- Add device tree nodes for pin pull-up/pull-down
bias control on DA850.
- Enable high speed support on DA850 MMC/SD
----------------------------------------------------------------
Axel Haslam (1):
ARM: dts: da850: enable high speed for mmc
David Lechner (1):
ARM: dts: da850: Add node for pullup/pulldown pinconf
arch/arm/boot/dts/da850.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
^ permalink raw reply
* [GIT PULL 2/2] DaVinci defconfig updates for v4.10 (part 4)
From: Sekhar Nori @ 2016-12-05 11:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161205110316.18514-1-nsekhar@ti.com>
The following changes since commit 213971e7571e27f47b4e926904a9adf794925c51:
ARM: davinci_all_defconfig: Enable OHCI as module (2016-11-22 20:50:46 +0530)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/nsekhar/linux-davinci.git tags/davinci-for-v4.10/defconfig-4
for you to fetch changes up to 47d03e48efa572109229aec9436948d92f44b689:
ARM: davinci_all_defconfig: Enable da8xx usb otg (2016-12-01 19:52:12 +0530)
----------------------------------------------------------------
Enable support for MUSB based USB OTG on DA850.
----------------------------------------------------------------
Alexandre Bailon (1):
ARM: davinci_all_defconfig: Enable da8xx usb otg
arch/arm/configs/davinci_all_defconfig | 2 ++
1 file changed, 2 insertions(+)
^ permalink raw reply
* [RFC PATCH 00/29] arm64: Scalable Vector Extension core support
From: Szabolcs Nagy @ 2016-12-05 11:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <0293f7d3-b3d3-1a68-5b99-75db195eb796@redhat.com>
On 05/12/16 10:44, Florian Weimer wrote:
>>> By the way, how is this implemented? Some of them overlap existing
>>> callee-saved registers.
>>
>> Basically, all the *new* state is caller-save.
>>
>> The Neon/FPSIMD regs V8-V15 are callee-save, so in the SVE view
>> Zn[bits 127:0] is callee-save for all n = 8..15.
>
> Are the extension parts of registers v8 to v15 used for argument passing?
>
> If not, we should be able to use the existing dynamic linker trampoline.
>
if sve arguments are passed to a function then it
has special call abi (which is probably not yet
documented), this call abi requires that such a
call does not go through plt to avoid requiring
sve aware libc.
same for tls access: the top part of sve regs have
to be saved by the caller before accessing tls so
the tlsdesc entry does not have to save them.
so current trampolines should be fine.
^ permalink raw reply
* [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings
From: Benjamin Gaignard @ 2016-12-05 11:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161205065350.GA18069@ulmo.ba.sec>
2016-12-05 7:53 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
> On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
>> Define bindings for pwm-stm32
>>
>> version 2:
>> - use parameters instead of compatible of handle the hardware configuration
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>> .../devicetree/bindings/pwm/pwm-stm32.txt | 38 ++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> new file mode 100644
>> index 0000000..575b9fb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> @@ -0,0 +1,38 @@
>> +STMicroelectronics PWM driver bindings for STM32
>
> Technically this bindings describe devices, so "driver binding" is a
> somewhat odd wording. Perhaps:
>
> STMicroelectronics STM32 General Purpose Timer PWM bindings
>
> ?
done
>
>> +
>> +Must be a sub-node of STM32 general purpose timer driver
>> +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
>
> Again, "driver parent node" is odd. Perhaps:
>
> Must be a sub-node of an STM32 General Purpose Timer device tree
> node. See ../mfd/stm32-general-purpose-timer.txt for details about
> the parent node.
>
> ?
done
>
>> +Required parameters:
>> +- compatible: Must be "st,stm32-pwm"
>> +- pinctrl-names: Set to "default".
>> +- pinctrl-0: List of phandles pointing to pin configuration nodes
>> + for PWM module.
>> + For Pinctrl properties, please refer to [1].
>
> Your indentation and capitalization are inconsistent. Also, please refer
> to the pinctrl bindings by relative path and inline, rather than as a
> footnote reference.
OK
>
>> +
>> +Optional parameters:
>> +- st,breakinput: Set if the hardware have break input capabilities
>> +- st,breakinput-polarity: Set break input polarity. Default is 0
>> + The value define the active polarity:
>> + - 0 (active LOW)
>> + - 1 (active HIGH)
>
> Could we fold these into a single property? If st,breakinput-polarity is
> not present it could simply mean that there is no break input, and if it
> is present you don't have to rely on a default.
I need to know if I have to activate breakinput feature and on which level
I will rewrite it like that:
Optional parameters:
- st,breakinput-polarity-high: Set if break input polarity is active
on high level.
- st,breakinput-polarity-high: Set if break input polarity is active
on low level.
>> +- st,pwm-num-chan: Number of available PWM channels. Default is 0.
>
> The pwm- prefix is rather redundant since the node is already named pwm.
> Why not simply st,channels? Or simply channels, since it's not really
> anything specific to this hardware.
>
> Come to think of it, might be worth having a discussion with our DT
> gurus about what their stance is on using the # as prefix for numbers
> (such as in #address-cells or #size-cells). This could be #channels to
> mark it more explicitly as representing a count.
>
>> +- st,32bits-counter: Set if the hardware have a 32 bits counter
>> +- st,complementary: Set if the hardware have complementary output channels
>
> "hardware has" and also maybe mention explicitly that this is a boolean
> property. Otherwise people might be left wondering what it should be set
> to. Or maybe word this differently to imply that it's boolean:
>
> - st,32bits-counter: if present, the hardware has a 32 bit counter
> - st,complementary: if present, the hardware has a complementary
> output channel
I found a way to detect, at probe time, the number of channels, counter size,
break input capability and complementary channels so I will remove
"st,breakinput", "st,32bits-counter", "st,complementary" and "st,pwm-num-chan"
parameters
>
> Thierry
^ permalink raw reply
* next-20161205 build: 3 failures 4 warnings (next-20161205)
From: Mark Brown @ 2016-12-05 11:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <E1cDo86-0008IX-9W@optimist>
On Mon, Dec 05, 2016 at 07:56:06AM +0000, Build bot for Mark Brown wrote:
Today's -next fails to build an arm64 allnodconfig and allmodconfig
with:
> arm64-allnoconfig
> ../arch/arm64/lib/clear_user.S:33: Error: bad or irreducible absolute expression
> ../arch/arm64/lib/clear_user.S:53: Error: bad or irreducible absolute expression
> ../arch/arm64/lib/clear_user.S:33: Error: attempt to move .org backwards
> ../arch/arm64/lib/clear_user.S:53: Error: attempt to move .org backwards
> ../arch/arm64/lib/copy_from_user.S:67: Error: bad or irreducible absolute expression
> ../arch/arm64/lib/copy_from_user.S:70: Error: bad or irreducible absolute expression
> ../arch/arm64/lib/copy_from_user.S:67: Error: attempt to move .org backwards
> ../arch/arm64/lib/copy_from_user.S:70: Error: attempt to move .org backwards
> ../arch/arm64/lib/copy_in_user.S:68: Error: bad or irreducible absolute expression
> ../arch/arm64/lib/copy_in_user.S:71: Error: bad or irreducible absolute expression
> ../arch/arm64/lib/copy_in_user.S:68: Error: attempt to move .org backwards
> ../arch/arm64/lib/copy_in_user.S:71: Error: attempt to move .org backwards
> ../arch/arm64/lib/copy_to_user.S:66: Error: bad or irreducible absolute expression
> ../arch/arm64/lib/copy_to_user.S:69: Error: bad or irreducible absolute expression
> ../arch/arm64/lib/copy_to_user.S:66: Error: attempt to move .org backwards
> ../arch/arm64/lib/copy_to_user.S:69: Error: attempt to move .org backwards
This was triggered somehow by bca8f17f57bd7 (arm64: Get rid of
asm/opcodes.h) though I didn't figure out how.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161205/cd10bb7a/attachment-0001.sig>
^ permalink raw reply
* [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings
From: Thierry Reding @ 2016-12-05 11:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CA+M3ks4-JQNGYtdiXNj1J701x0njKD0_bf4yN1mLSjEcT2Z=6A@mail.gmail.com>
On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
> 2016-12-05 7:53 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> >> Define bindings for pwm-stm32
> >>
> >> version 2:
> >> - use parameters instead of compatible of handle the hardware configuration
> >>
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> >> ---
> >> .../devicetree/bindings/pwm/pwm-stm32.txt | 38 ++++++++++++++++++++++
> >> 1 file changed, 38 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> new file mode 100644
> >> index 0000000..575b9fb
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> @@ -0,0 +1,38 @@
> >> +STMicroelectronics PWM driver bindings for STM32
> >
> > Technically this bindings describe devices, so "driver binding" is a
> > somewhat odd wording. Perhaps:
> >
> > STMicroelectronics STM32 General Purpose Timer PWM bindings
> >
> > ?
> done
>
> >
> >> +
> >> +Must be a sub-node of STM32 general purpose timer driver
> >> +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
> >
> > Again, "driver parent node" is odd. Perhaps:
> >
> > Must be a sub-node of an STM32 General Purpose Timer device tree
> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
> > the parent node.
> >
> > ?
>
> done
>
> >
> >> +Required parameters:
> >> +- compatible: Must be "st,stm32-pwm"
> >> +- pinctrl-names: Set to "default".
> >> +- pinctrl-0: List of phandles pointing to pin configuration nodes
> >> + for PWM module.
> >> + For Pinctrl properties, please refer to [1].
> >
> > Your indentation and capitalization are inconsistent. Also, please refer
> > to the pinctrl bindings by relative path and inline, rather than as a
> > footnote reference.
>
> OK
>
> >
> >> +
> >> +Optional parameters:
> >> +- st,breakinput: Set if the hardware have break input capabilities
> >> +- st,breakinput-polarity: Set break input polarity. Default is 0
> >> + The value define the active polarity:
> >> + - 0 (active LOW)
> >> + - 1 (active HIGH)
> >
> > Could we fold these into a single property? If st,breakinput-polarity is
> > not present it could simply mean that there is no break input, and if it
> > is present you don't have to rely on a default.
>
> I need to know if I have to activate breakinput feature and on which level
> I will rewrite it like that:
> Optional parameters:
> - st,breakinput-polarity-high: Set if break input polarity is active
> on high level.
> - st,breakinput-polarity-high: Set if break input polarity is active
> on low level.
How is that different from a single property:
Optional properties:
- st,breakinput-polarity: If present, a break input is available
for the channel. In that case the property value denotes the
polarity of the break input:
- 0: active low
- 1: active high
?
> > The pwm- prefix is rather redundant since the node is already named pwm.
> > Why not simply st,channels? Or simply channels, since it's not really
> > anything specific to this hardware.
> >
> > Come to think of it, might be worth having a discussion with our DT
> > gurus about what their stance is on using the # as prefix for numbers
> > (such as in #address-cells or #size-cells). This could be #channels to
> > mark it more explicitly as representing a count.
> >
> >> +- st,32bits-counter: Set if the hardware have a 32 bits counter
> >> +- st,complementary: Set if the hardware have complementary output channels
> >
> > "hardware has" and also maybe mention explicitly that this is a boolean
> > property. Otherwise people might be left wondering what it should be set
> > to. Or maybe word this differently to imply that it's boolean:
> >
> > - st,32bits-counter: if present, the hardware has a 32 bit counter
> > - st,complementary: if present, the hardware has a complementary
> > output channel
>
> I found a way to detect, at probe time, the number of channels, counter size,
> break input capability and complementary channels so I will remove
> "st,breakinput", "st,32bits-counter", "st,complementary" and "st,pwm-num-chan"
> parameters
Oh hey, that's very neat. I suppose in that case my comment above about
the break input polarity is somewhat obsoleted. Still I think you won't
need two properties. Instead you can follow what other similar
properties have done: choose a default (often low-active) and have a
single optional property to override the default (often high-active).
In your case:
- st,breakinput-active-high: Some channels have a break input,
whose polarity will be active low by default. If this
property is present, the channel will be configured with an
active high polarity for the break input.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161205/85093ef8/attachment.sig>
^ permalink raw reply
* [PATCH 1/1] arm64: Correcting format specifier for printing 64 bit addresses
From: Will Deacon @ 2016-12-05 11:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1480925393-8386-1-git-send-email-maninder1.s@samsung.com>
On Mon, Dec 05, 2016 at 01:39:53PM +0530, Maninder Singh wrote:
> This patch corrects format specifier for printing 64 bit addresses.
>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> ---
> arch/arm64/kernel/signal.c | 2 +-
> arch/arm64/kvm/sys_regs.c | 8 ++++++--
> arch/arm64/mm/fault.c | 15 ++++++++++-----
> arch/arm64/mm/mmu.c | 4 ++--
> 4 files changed, 19 insertions(+), 10 deletions(-)
Any reason not to fix kvm/trace.h too?
Anyway, rest of this looks fine:
Acked-by: Will Deacon <will.deacon@arm.com>
Will
^ permalink raw reply
* [PATCH v3 4/7] PWM: add pwm driver for stm32 plaftorm
From: Thierry Reding @ 2016-12-05 11:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CA+M3ks7_utdw1c0A0m+RzuPTfvFAv5d8HoQvQDzXwHR8M6RKJA@mail.gmail.com>
On Mon, Dec 05, 2016 at 12:02:45PM +0100, Benjamin Gaignard wrote:
> 2016-12-05 8:23 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
> > On Fri, Dec 02, 2016 at 11:17:19AM +0100, Benjamin Gaignard wrote:
> >> This driver add support for pwm driver on stm32 platform.
> >
> > "adds". Also please use PWM in prose because it's an abbreviation.
> >
> >> The SoC have multiple instances of the hardware IP and each
> >> of them could have small differences: number of channels,
> >> complementary output, counter register size...
> >> Use DT parameters to handle those differentes configuration
> >
> > "different configurations"
>
> ok
>
> >
> >>
> >> version 2:
> >> - only keep one comptatible
> >> - use DT paramaters to discover hardware block configuration
> >
> > "parameters"
>
> ok
>
> >
> >>
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> >> ---
> >> drivers/pwm/Kconfig | 8 ++
> >> drivers/pwm/Makefile | 1 +
> >> drivers/pwm/pwm-stm32.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 294 insertions(+)
> >> create mode 100644 drivers/pwm/pwm-stm32.c
> >>
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index bf01288..a89fdba 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -388,6 +388,14 @@ config PWM_STI
> >> To compile this driver as a module, choose M here: the module
> >> will be called pwm-sti.
> >>
> >> +config PWM_STM32
> >> + bool "STMicroelectronics STM32 PWM"
> >> + depends on ARCH_STM32
> >> + depends on OF
> >> + select MFD_STM32_GP_TIMER
> >
> > Should that be a "depends on"?
>
> I think select is fine because the wanted feature is PWM not the mfd part
I think in that case you may want to hide the MFD Kconfig option. See
Documentation/kbuild/kconfig-language.txt (notes about select) for the
details.
[...]
> >> +};
> >> +
> >> +#define to_stm32_pwm_dev(x) container_of(chip, struct stm32_pwm_dev, chip)
> >
> > Please turn this into a static inline.
>
> with putting struct pwm_chip as first filed I will just cast the structure
Don't do that, please. container_of() is still preferred because it is
correct and won't break even if you (or somebody else) changes the order
in the future. The fact that it might be optimized away is a detail, or
a micro-optimization. Force-casting is a bad idea because it won't catch
errors if for some reason the field doesn't remain in the first position
forever.
> >> +static void stm32_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >> +{
> >> + u32 mask;
> >> + struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
> >> +
> >> + /* Disable channel */
> >> + mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
> >> + if (dev->caps & CAP_COMPLEMENTARY)
> >> + mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
> >> +
> >> + regmap_update_bits(dev->regmap, TIM_CCER, mask, 0);
> >> +
> >> + /* When all channels are disabled, we can disable the controller */
> >> + if (!__active_channels(dev))
> >> + regmap_update_bits(dev->regmap, TIM_CR1, TIM_CR1_CEN, 0);
> >> +
> >> + clk_disable(dev->clk);
> >> +}
> >
> > All of the above can be folded into the ->apply() hook for atomic PWM
> > support.
> >
> > Also, in the above you use clk_enable() in the ->enable() callback and
> > clk_disable() in ->disable(). If you need the clock to access registers
> > you'll have to enabled it in the others as well because there are no
> > guarantees that configuration will only happen between ->enable() and
> > ->disable(). Atomic PWM simplifies this a bit, but you still need to be
> > careful about when to enable the clock in your ->apply() hook.
>
> I have used regmap functions enable/disable clk for me when accessing to
> the registers.
> I only have to take care of clk enable/disable when PWM state change.
Okay, that's fine then.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161205/79897d78/attachment.sig>
^ permalink raw reply
* [PATCH 1/3] ARM: dts: imx6: Add Savageboard common file
From: Fabio Estevam @ 2016-12-05 11:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161205010729.7047-2-woogyom.kim@gmail.com>
On Sun, Dec 4, 2016 at 11:07 PM, Milo Kim <woogyom.kim@gmail.com> wrote:
> + regulators {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reg_3p3v: regulator at 0 {
> + compatible = "regulator-fixed";
> + reg = <0>;
> + regulator-name = "3P3V";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-always-on;
> + };
Please remove the regulators container and put the regulator node
directly as follows:
reg_3p3v: regulator-3p3v {
compatible = "regulator-fixed";
regulator-name = "3P3V";
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
regulator-always-on;
}
> + };
> +};
> +
> +&clks {
> + assigned-clocks = <&clks IMX6QDL_CLK_LDB_DI0_SEL>,
> + <&clks IMX6QDL_CLK_LDB_DI1_SEL>;
> + assigned-clock-parents = <&clks IMX6QDL_CLK_PLL3_USB_OTG>,
> + <&clks IMX6QDL_CLK_PLL3_USB_OTG>;
> +};
> +
> +&fec {
> + phy-mode = "rgmii";
> + phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_HIGH>;
I think you meant
phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
> +&iomuxc {
> + savageboard {
> + pinctrl_emmc: emmcgrp {
> + fsl,pins = <
> + MX6QDL_PAD_SD4_CMD__SD4_CMD 0x17059
> + MX6QDL_PAD_SD4_CLK__SD4_CLK 0x10059
> + MX6QDL_PAD_SD4_DAT0__SD4_DATA0 0x17059
> + MX6QDL_PAD_SD4_DAT1__SD4_DATA1 0x17059
> + MX6QDL_PAD_SD4_DAT2__SD4_DATA2 0x17059
> + MX6QDL_PAD_SD4_DAT3__SD4_DATA3 0x17059
> + MX6QDL_PAD_SD4_DAT4__SD4_DATA4 0x17059
> + MX6QDL_PAD_SD4_DAT5__SD4_DATA5 0x17059
> + MX6QDL_PAD_SD4_DAT6__SD4_DATA6 0x17059
> + MX6QDL_PAD_SD4_DAT7__SD4_DATA7 0x17059
> + >;
> + };
You can remove the savegeboard level. Please check
arch/arm/boot/dts/imx6q-tbs2910.dts.
iomux usually go as the last node of the dts file.
^ permalink raw reply
* [PATCH 2/2] arm64: alternatives: Work around NOP generation with broken assembler
From: Marc Zyngier @ 2016-12-05 11:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <c7de1968-7d50-a435-56ea-a7f3e9cb52da@arm.com>
On 05/12/16 10:58, Marc Zyngier wrote:
> On 05/12/16 10:05, Will Deacon wrote:
>> On Sat, Dec 03, 2016 at 02:05:38PM +0000, Marc Zyngier wrote:
>>> When compiling a .inst directive in an alternative clause with
>>> a rather old version of gas (circa 2.24), and when used with
>>> the alternative_else_nop_endif feature, the compilation fails
>>> with a rather cryptic message such as:
>>>
>>> arch/arm64/lib/clear_user.S:33: Error: bad or irreducible absolute expression
>>>
>>> which is caused by the bug described in eb7c11ee3c5c ("arm64:
>>> alternative: Work around .inst assembler bugs").
>>>
>>> This effectively prevents the use of the "nops" macro, which
>>> requires the number of instruction as a parameter (the assembler
>>> is confused and unable to compute the difference between two labels).
>>>
>>> As an alternative(!), use the .fill directive to output the number
>>> of required NOPs (.fill has the good idea to output the fill pattern
>>> in the endianness of the instruction stream).
>>
>> Are you sure about that? The gas docs say:
>>
>> `The contents of each repeat bytes is taken from an 8-byte number. The
>> highest order 4 bytes are zero. The lowest order 4 bytes are value rendered
>> in the byte-order of an integer on the computer as is assembling for.'
>>
>> and I'd expect "integer" to refer to data values, rather than instructions.
>
> My tests on 2.24 and 2.25 seem to show that the output is always LE,
> which could be another GAS issue. I've asked the binutils people for
> information.
Well, my testing was wrong, as objdump -d was lying to me by displaying
something in little-endian form. Time for some more head scratching.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH v3 1/5] spi: Add basic support for Armada 3700 SPI Controller
From: Mark Brown @ 2016-12-05 12:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161201102719.4291-2-romain.perier@free-electrons.com>
On Thu, Dec 01, 2016 at 11:27:15AM +0100, Romain Perier wrote:
> +config SPI_ARMADA_3700
> + tristate "Marvell Armada 3700 SPI Controller"
> + depends on ARCH_MVEBU && OF
Why does this not have a COMPILE_TEST dependency?
> + /* Reset SPI unit */
> + val = spireg_read(a3700_spi, A3700_SPI_IF_CFG_REG);
> + val |= A3700_SPI_SRST;
> + spireg_write(a3700_spi, A3700_SPI_IF_CFG_REG, val);
> +
> + for (i = 0; i < A3700_SPI_TIMEOUT; i++)
> + udelay(1);
Why not just do a single udelay()?
> +static irqreturn_t a3700_spi_interrupt(int irq, void *dev_id)
> +{
> + struct spi_master *master = dev_id;
> + struct a3700_spi *a3700_spi;
> + u32 cause;
> +
> + a3700_spi = spi_master_get_devdata(master);
> +
> + /* Get interrupt causes */
> + cause = spireg_read(a3700_spi, A3700_SPI_INT_STAT_REG);
> +
> + /* mask and acknowledge the SPI interrupts */
> + spireg_write(a3700_spi, A3700_SPI_INT_MASK_REG, 0);
> + spireg_write(a3700_spi, A3700_SPI_INT_STAT_REG, cause);
> +
> + /* Wake up the transfer */
> + if (a3700_spi->wait_mask & cause)
> + complete(&a3700_spi->done);
> +
> + return IRQ_HANDLED;
> +}
This reports that we handled an interrupt even if we did not in fact
handle an interrupt. It's also a bit dodgy that it doesn't check what
the interrupt was but that's less serious.
> + master->bus_num = (pdev->id != -1) ? pdev->id : 0;
At most this should be just setting the bus number to pdev->id like
other drivers do.
> + ret = clk_prepare_enable(spi->clk);
> + if (ret) {
> + dev_err(dev, "could not prepare clk: %d\n", ret);
> + goto error;
> + }
I'd expect the hardware prepare/unprepare to be managing the enable and
disable of the clock in order to save a little power.
> + dev_info(dev, "Marvell Armada 3700 SPI Controller at 0x%08lx, irq %d\n",
> + (unsigned long)res->start, spi->irq);
This is just adding noise to the boot, remove it. It's not telling us
anything we read from the hardware or anything.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161205/bce7957d/attachment.sig>
^ permalink raw reply
* [PATCH] arm64: Add CMDLINE_EXTEND
From: Catalin Marinas @ 2016-12-05 12:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <61060b44-64a9-d628-4faf-3a7ae06048aa@infradead.org>
Hi Geoff,
On Fri, Dec 02, 2016 at 02:17:02PM -0800, Geoff Levand wrote:
> The device tree code already supports CMDLINE_EXTEND,
> so add the config option to make it available on arm64.
What's your use-case for this patch? Note that both CMDLINE_FORCE and
CMDLINE_EXTEND (if we introduce it) are ignored by the EFI stub.
However, we don't seem to have stated this anywhere.
--
Catalin
^ permalink raw reply
* [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings
From: Benjamin Gaignard @ 2016-12-05 12:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161205112347.GF19891@ulmo.ba.sec>
2016-12-05 12:23 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
> On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
>> 2016-12-05 7:53 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
>> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
>> >> Define bindings for pwm-stm32
>> >>
>> >> version 2:
>> >> - use parameters instead of compatible of handle the hardware configuration
>> >>
>> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> >> ---
>> >> .../devicetree/bindings/pwm/pwm-stm32.txt | 38 ++++++++++++++++++++++
>> >> 1 file changed, 38 insertions(+)
>> >> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> >> new file mode 100644
>> >> index 0000000..575b9fb
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>> >> @@ -0,0 +1,38 @@
>> >> +STMicroelectronics PWM driver bindings for STM32
>> >
>> > Technically this bindings describe devices, so "driver binding" is a
>> > somewhat odd wording. Perhaps:
>> >
>> > STMicroelectronics STM32 General Purpose Timer PWM bindings
>> >
>> > ?
>> done
>>
>> >
>> >> +
>> >> +Must be a sub-node of STM32 general purpose timer driver
>> >> +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
>> >
>> > Again, "driver parent node" is odd. Perhaps:
>> >
>> > Must be a sub-node of an STM32 General Purpose Timer device tree
>> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
>> > the parent node.
>> >
>> > ?
>>
>> done
>>
>> >
>> >> +Required parameters:
>> >> +- compatible: Must be "st,stm32-pwm"
>> >> +- pinctrl-names: Set to "default".
>> >> +- pinctrl-0: List of phandles pointing to pin configuration nodes
>> >> + for PWM module.
>> >> + For Pinctrl properties, please refer to [1].
>> >
>> > Your indentation and capitalization are inconsistent. Also, please refer
>> > to the pinctrl bindings by relative path and inline, rather than as a
>> > footnote reference.
>>
>> OK
>>
>> >
>> >> +
>> >> +Optional parameters:
>> >> +- st,breakinput: Set if the hardware have break input capabilities
>> >> +- st,breakinput-polarity: Set break input polarity. Default is 0
>> >> + The value define the active polarity:
>> >> + - 0 (active LOW)
>> >> + - 1 (active HIGH)
>> >
>> > Could we fold these into a single property? If st,breakinput-polarity is
>> > not present it could simply mean that there is no break input, and if it
>> > is present you don't have to rely on a default.
>>
>> I need to know if I have to activate breakinput feature and on which level
>> I will rewrite it like that:
>> Optional parameters:
>> - st,breakinput-polarity-high: Set if break input polarity is active
>> on high level.
>> - st,breakinput-polarity-high: Set if break input polarity is active
>> on low level.
>
> How is that different from a single property:
>
> Optional properties:
> - st,breakinput-polarity: If present, a break input is available
> for the channel. In that case the property value denotes the
> polarity of the break input:
> - 0: active low
> - 1: active high
>
> ?
For break input feature I need two information: do I have to active it
and if activated
on which level.
I have two solutions:
- one parameter with a value (0 or 1) -> st,breakinput-polarity
- two parameters without value -> st,breakinput-active-high and
st,breakinput-active-low
By default break input feature is disabled
>
>> > The pwm- prefix is rather redundant since the node is already named pwm.
>> > Why not simply st,channels? Or simply channels, since it's not really
>> > anything specific to this hardware.
>> >
>> > Come to think of it, might be worth having a discussion with our DT
>> > gurus about what their stance is on using the # as prefix for numbers
>> > (such as in #address-cells or #size-cells). This could be #channels to
>> > mark it more explicitly as representing a count.
>> >
>> >> +- st,32bits-counter: Set if the hardware have a 32 bits counter
>> >> +- st,complementary: Set if the hardware have complementary output channels
>> >
>> > "hardware has" and also maybe mention explicitly that this is a boolean
>> > property. Otherwise people might be left wondering what it should be set
>> > to. Or maybe word this differently to imply that it's boolean:
>> >
>> > - st,32bits-counter: if present, the hardware has a 32 bit counter
>> > - st,complementary: if present, the hardware has a complementary
>> > output channel
>>
>> I found a way to detect, at probe time, the number of channels, counter size,
>> break input capability and complementary channels so I will remove
>> "st,breakinput", "st,32bits-counter", "st,complementary" and "st,pwm-num-chan"
>> parameters
>
> Oh hey, that's very neat. I suppose in that case my comment above about
> the break input polarity is somewhat obsoleted. Still I think you won't
> need two properties. Instead you can follow what other similar
> properties have done: choose a default (often low-active) and have a
> single optional property to override the default (often high-active).
>
> In your case:
>
> - st,breakinput-active-high: Some channels have a break input,
> whose polarity will be active low by default. If this
> property is present, the channel will be configured with an
> active high polarity for the break input.
>
> Thierry
--
Benjamin Gaignard
Graphic Study Group
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings
From: Thierry Reding @ 2016-12-05 12:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CA+M3ks5j2e-EQEf74YSduboNnCiWC7+e_PD5aAz5XdFA1tGztQ@mail.gmail.com>
On Mon, Dec 05, 2016 at 01:12:25PM +0100, Benjamin Gaignard wrote:
> 2016-12-05 12:23 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
> > On Mon, Dec 05, 2016 at 12:08:32PM +0100, Benjamin Gaignard wrote:
> >> 2016-12-05 7:53 GMT+01:00 Thierry Reding <thierry.reding@gmail.com>:
> >> > On Fri, Dec 02, 2016 at 11:17:18AM +0100, Benjamin Gaignard wrote:
> >> >> Define bindings for pwm-stm32
> >> >>
> >> >> version 2:
> >> >> - use parameters instead of compatible of handle the hardware configuration
> >> >>
> >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> >> >> ---
> >> >> .../devicetree/bindings/pwm/pwm-stm32.txt | 38 ++++++++++++++++++++++
> >> >> 1 file changed, 38 insertions(+)
> >> >> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> >> new file mode 100644
> >> >> index 0000000..575b9fb
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> >> >> @@ -0,0 +1,38 @@
> >> >> +STMicroelectronics PWM driver bindings for STM32
> >> >
> >> > Technically this bindings describe devices, so "driver binding" is a
> >> > somewhat odd wording. Perhaps:
> >> >
> >> > STMicroelectronics STM32 General Purpose Timer PWM bindings
> >> >
> >> > ?
> >> done
> >>
> >> >
> >> >> +
> >> >> +Must be a sub-node of STM32 general purpose timer driver
> >> >> +Parent node properties are describe in ../mfd/stm32-general-purpose-timer.txt
> >> >
> >> > Again, "driver parent node" is odd. Perhaps:
> >> >
> >> > Must be a sub-node of an STM32 General Purpose Timer device tree
> >> > node. See ../mfd/stm32-general-purpose-timer.txt for details about
> >> > the parent node.
> >> >
> >> > ?
> >>
> >> done
> >>
> >> >
> >> >> +Required parameters:
> >> >> +- compatible: Must be "st,stm32-pwm"
> >> >> +- pinctrl-names: Set to "default".
> >> >> +- pinctrl-0: List of phandles pointing to pin configuration nodes
> >> >> + for PWM module.
> >> >> + For Pinctrl properties, please refer to [1].
> >> >
> >> > Your indentation and capitalization are inconsistent. Also, please refer
> >> > to the pinctrl bindings by relative path and inline, rather than as a
> >> > footnote reference.
> >>
> >> OK
> >>
> >> >
> >> >> +
> >> >> +Optional parameters:
> >> >> +- st,breakinput: Set if the hardware have break input capabilities
> >> >> +- st,breakinput-polarity: Set break input polarity. Default is 0
> >> >> + The value define the active polarity:
> >> >> + - 0 (active LOW)
> >> >> + - 1 (active HIGH)
> >> >
> >> > Could we fold these into a single property? If st,breakinput-polarity is
> >> > not present it could simply mean that there is no break input, and if it
> >> > is present you don't have to rely on a default.
> >>
> >> I need to know if I have to activate breakinput feature and on which level
> >> I will rewrite it like that:
> >> Optional parameters:
> >> - st,breakinput-polarity-high: Set if break input polarity is active
> >> on high level.
> >> - st,breakinput-polarity-high: Set if break input polarity is active
> >> on low level.
> >
> > How is that different from a single property:
> >
> > Optional properties:
> > - st,breakinput-polarity: If present, a break input is available
> > for the channel. In that case the property value denotes the
> > polarity of the break input:
> > - 0: active low
> > - 1: active high
> >
> > ?
>
> For break input feature I need two information: do I have to active it
> and if activated
> on which level.
> I have two solutions:
> - one parameter with a value (0 or 1) -> st,breakinput-polarity
> - two parameters without value -> st,breakinput-active-high and
> st,breakinput-active-low
>
> By default break input feature is disabled
Right, what I was suggesting is that you use the first solution because
it's the typical way to use for tristate options. It's also much easier
to test for in practice because for the second solution you have to
parse two properties before you know whether it is active or not.
The second is typically the solution for required properties where only
one of the properties is used to override the default.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161205/e3aed0cd/attachment-0001.sig>
^ permalink raw reply
* [PATCH] ACPI/IORT: Make dma masks set-up IORT specific
From: Lorenzo Pieralisi @ 2016-12-05 12:26 UTC (permalink / raw)
To: linux-arm-kernel
The introduction of acpi_dma_configure() allows to configure DMA
and related IOMMU for any device that is DMA capable. To achieve
that goal it ensures DMA masks are set-up to sane default values
before proceeding with IOMMU and DMA ops configuration.
On x86/ia64 systems, through acpi_bind_one(), acpi_dma_configure() is
called for every device that has an ACPI companion, in that every device
is considered DMA capable on x86/ia64 systems (ie acpi_get_dma_attr() API),
which has the side effect of initializing dma masks also for
pseudo-devices (eg CPUs and memory nodes) and potentially for devices
whose dma masks were not set-up before the acpi_dma_configure() API was
introduced, which may have noxious side effects.
Therefore, in preparation for IORT firmware specific DMA masks set-up,
wrap the default DMA masks set-up in acpi_dma_configure() inside an IORT
specific wrapper that reverts to a NOP on x86/ia64 systems, restoring the
default expected behaviour on x86/ia64 systems and keeping DMA default
masks set-up on IORT based (ie ARM) arch configurations.
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Tomasz Nowicki <tn@semihalf.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Sricharan R <sricharan@codeaurora.org>
---
Joerg,
pending Rafael's ACK on it, given the 4.10 release timing and that the
series is queued via the IOMMU tree please consider applying this patch to
your arm/smmu branch for 4.10, it is not fixing a bug but it is modifying
the x86/ia64 code path; I prefer preventing any issue related to default
dma masks on x86/ia64 so I hope it can get merged along with the rest of
the ACPI IORT SMMU series.
Thanks a lot and apologies,
Lorenzo
drivers/acpi/arm64/iort.c | 22 ++++++++++++++++++++++
drivers/acpi/scan.c | 14 +-------------
include/linux/acpi_iort.h | 2 ++
3 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 47bace8..e0d2e6e 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -547,6 +547,28 @@ static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
}
/**
+ * iort_set_dma_mask - Set-up dma mask for a device.
+ *
+ * @dev: device to configure
+ */
+void iort_set_dma_mask(struct device *dev)
+{
+ /*
+ * Set default coherent_dma_mask to 32 bit. Drivers are expected to
+ * setup the correct supported mask.
+ */
+ if (!dev->coherent_dma_mask)
+ dev->coherent_dma_mask = DMA_BIT_MASK(32);
+
+ /*
+ * Set it to coherent_dma_mask by default if the architecture
+ * code has not set it.
+ */
+ if (!dev->dma_mask)
+ dev->dma_mask = &dev->coherent_dma_mask;
+}
+
+/**
* iort_iommu_configure - Set-up IOMMU configuration for a device.
*
* @dev: device to configure
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 80698d3..93b00cf 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1380,19 +1380,7 @@ void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
{
const struct iommu_ops *iommu;
- /*
- * Set default coherent_dma_mask to 32 bit. Drivers are expected to
- * setup the correct supported mask.
- */
- if (!dev->coherent_dma_mask)
- dev->coherent_dma_mask = DMA_BIT_MASK(32);
-
- /*
- * Set it to coherent_dma_mask by default if the architecture
- * code has not set it.
- */
- if (!dev->dma_mask)
- dev->dma_mask = &dev->coherent_dma_mask;
+ iort_set_dma_mask(dev);
iommu = iort_iommu_configure(dev);
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index dcb2b60..77e0809 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -35,6 +35,7 @@ bool iort_node_match(u8 type);
u32 iort_msi_map_rid(struct device *dev, u32 req_id);
struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
/* IOMMU interface */
+void iort_set_dma_mask(struct device *dev);
const struct iommu_ops *iort_iommu_configure(struct device *dev);
#else
static inline void acpi_iort_init(void) { }
@@ -45,6 +46,7 @@ static inline struct irq_domain *iort_get_device_domain(struct device *dev,
u32 req_id)
{ return NULL; }
/* IOMMU interface */
+static inline void iort_set_dma_mask(struct device *dev) { }
static inline
const struct iommu_ops *iort_iommu_configure(struct device *dev)
{ return NULL; }
--
2.10.0
^ permalink raw reply related
* Adding a .platform_init callback to sdhci_arasan_ops
From: Sebastian Frias @ 2016-12-05 12:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAD=FV=XrJHFbpbhsJZAEE-HLMe47Ov=BgvHw8ZtfCazzT75pKQ@mail.gmail.com>
Hi Doug,
On 28/11/16 18:46, Doug Anderson wrote:
> Hi,
>
> On Mon, Nov 28, 2016 at 6:39 AM, Sebastian Frias <sf84@laposte.net> wrote:
>>> I will try to send another patch with what a different approach
>>>
>>
>> Here's a different approach (I just tested that it built, because I don't have the
>> rk3399 platform):
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
>> index 410a55b..5be6e67 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -382,22 +382,6 @@ static int sdhci_arasan_resume(struct device *dev)
>> static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend,
>> sdhci_arasan_resume);
>>
>> -static const struct of_device_id sdhci_arasan_of_match[] = {
>> - /* SoC-specific compatible strings w/ soc_ctl_map */
>> - {
>> - .compatible = "rockchip,rk3399-sdhci-5.1",
>> - .data = &rk3399_soc_ctl_map,
>> - },
>> -
>> - /* Generic compatible below here */
>> - { .compatible = "arasan,sdhci-8.9a" },
>> - { .compatible = "arasan,sdhci-5.1" },
>> - { .compatible = "arasan,sdhci-4.9a" },
>> -
>> - { /* sentinel */ }
>> -};
>> -MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
>> -
>> /**
>> * sdhci_arasan_sdcardclk_recalc_rate - Return the card clock rate
>> *
>> @@ -578,28 +562,18 @@ static void sdhci_arasan_unregister_sdclk(struct device *dev)
>> of_clk_del_provider(dev->of_node);
>> }
>>
>> -static int sdhci_arasan_probe(struct platform_device *pdev)
>> +static int sdhci_rockchip_platform_init(struct sdhci_host *host,
>> + struct platform_device *pdev)
>> {
>> int ret;
>> - const struct of_device_id *match;
>> struct device_node *node;
>> - struct clk *clk_xin;
>> - struct sdhci_host *host;
>> struct sdhci_pltfm_host *pltfm_host;
>> struct sdhci_arasan_data *sdhci_arasan;
>> - struct device_node *np = pdev->dev.of_node;
>> -
>> - host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
>> - sizeof(*sdhci_arasan));
>> - if (IS_ERR(host))
>> - return PTR_ERR(host);
>>
>> pltfm_host = sdhci_priv(host);
>> sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>> - sdhci_arasan->host = host;
>>
>> - match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
>> - sdhci_arasan->soc_ctl_map = match->data;
>> + sdhci_arasan->soc_ctl_map = &rk3399_soc_ctl_map;
>>
>> node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0);
>> if (node) {
>> @@ -611,10 +585,107 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>> if (ret != -EPROBE_DEFER)
>> dev_err(&pdev->dev, "Can't get syscon: %d\n",
>> ret);
>> - goto err_pltfm_free;
>> + return -1;
>> }
>> }
>>
>> + if (of_property_read_bool(pdev->dev.of_node, "xlnx,fails-without-test-cd"))
>> + sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
>> +
>> + return 0;
>> +}
>> +
>> +static int sdhci_rockchip_clock_init(struct sdhci_host *host,
>> + struct platform_device *pdev)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host;
>> + struct sdhci_arasan_data *sdhci_arasan;
>> +
>> + pltfm_host = sdhci_priv(host);
>> + sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>> +
>> + if (of_device_is_compatible(pdev->dev.of_node,
>> + "rockchip,rk3399-sdhci-5.1"))
>> + sdhci_arasan_update_clockmultiplier(host, 0x0);
>
> This _does_ belong in a Rockchip-specific init function, for now.
I'm not sure I understood. Are you saying that you agree to put this
into a specific function? Essentially agreeing with the concept the
patch is putting forward?
Note, I'm more interested in the concept (i.e.: init functions) and less
in knowing if my patch (which was a quick and dirty thing) properly moved
the functions into the said init functions. For example, I did not move
the code dealing with "arasan,sdhci-5.1", but it could go into another
callback.
Right now there are no "chip-specific" functions.
Just a code in sdhci_arasan_probe() that by checking various compatible
strings and the presence of other specific properties, acts as a way of
"chip-specific" initialisation, because it calls or not some functions.
(or the functions do nothing if some DT properties are not there).
The proposed patch is an attempt to clean up sdhci_arasan_probe() from
all those checks and move them into separate functions, for clarity and
maintainability reasons.
What are your thoughts in that regard?
>From what I've seen in other drivers, for example drivers/net/ethernet/aurora/nb8800.c
One matches the compatible string to get a (likely) chip-specific set of
callbacks to use in the 'probe' function.
Then, adding support for other chips is just a matter of replacing some
of those callbacks with others adapted to a given platform.
> Other platforms might want different values for
> corecfg_clockmultiplier, I think.
>
> If it becomes common to need to set this, it wouldn't be hard to make
> it generic by putting it in the data matched by the device tree, then
> generically call sdhci_arasan_update_clockmultiplier() in cases where
> it is needed. sdhci_arasan_update_clockmultiplier() itself should be
> generic enough to handle it.
>
>
>> +
>> + sdhci_arasan_update_baseclkfreq(host);
>
> If you make soc_ctl_map always part of "struct sdhci_arasan_cs_ops"
> then other platforms will be able to use it.
I thought that soc_ctl_map was specific and for a given platform.
For what is worth, I don't know which of these calls are or can be made
generic or not.
Indeed, I'm not an expert in this code; However, I think that given the
amount of checks for specific properties, probably part of this is chip-
specific, and as such, it would benefit from some re-factoring so that
the chip-specific parts are clearly separated from the rest, instead of
being mixed together inside the probe function.
>
> As argued in my original patch the field "corecfg_baseclkfreq" is
> documented in the generic Arasan document
> <https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf>
> and thus is unlikely to be Rockchip specific. It is entirely possible
> that not everyone who integrates this IP block will need this register
> set, but in that case they can set an offset as "-1" and they'll be
> fine.
>
> Said another way: the concept of whether or not to set "baseclkfreq"
> doesn't need to be tired to whether or not we're on Rockchip.
>
I see.
For what is worth, the documentation for 'sdhci_arasan_update_baseclkfreq()'
says something like:
* Many existing devices don't seem to do this and work fine. To keep
* compatibility for old hardware where the device tree doesn't provide a
* register map, this function is a noop if a soc_ctl_map hasn't been provided
* for this platform.
>
>> +
>> + return sdhci_arasan_register_sdclk(sdhci_arasan, pltfm_host->clk, &pdev->dev);
>
> This is documented in "bindings/mmc/arasan,sdhci.txt" to be available
> to all people using this driver, not just Rockchip. You should do it
> always. If you don't specify "#clock-cells" in the device tree it
> will be a no-op anyway.
I see, thanks for the explanation.
>
>
>> +}
>> +
>> +static int sdhci_tango_platform_init(struct sdhci_host *host,
>> + struct platform_device *pdev)
>> +{
>> + return 0;
>
> I'll comment in your other patch where you actually had an
> implementation for this.
>
Sounds good. I will reply to you there because now I have a more
complete set of register writes.
Best regards,
Sebastian
>
>> +}
>> +
>> +/* Chip-specific ops */
>> +struct sdhci_arasan_cs_ops {
>> + int (*platform_init)(struct sdhci_host *host,
>> + struct platform_device *pdev);
>> + int (*clock_init)(struct sdhci_host *host,
>> + struct platform_device *pdev);
>> +};
>
> I really think it's a good idea to add the "soc_ctl_map" into this structure.
>
> If nothing else when the next Rockchip SoC comes out that uses this
> then we won't have to do a second level of matching for Rockchip SoCs.
> I'm also a firm believer that others will need "soc_ctl_map" at some
> point in time, but obviously I can't predict the future.
>
>
>> +
>> +static const struct sdhci_arasan_cs_ops sdhci_rockchip_cs_ops = {
>> + .platform_init = sdhci_rockchip_platform_init,
>> + .clock_init = sdhci_rockchip_clock_init,
>> +};
>> +
>> +static const struct sdhci_arasan_cs_ops sdhci_tango_cs_ops = {
>> + .platform_init = sdhci_tango_platform_init,
>> +};
>> +
>> +static const struct of_device_id sdhci_arasan_of_match[] = {
>> + /* SoC-specific compatible strings */
>> + {
>> + .compatible = "rockchip,rk3399-sdhci-5.1",
>> + .data = &sdhci_rockchip_cs_ops,
>> + },
>> + {
>> + .compatible = "sigma,sdio-v1",
>> + .data = &sdhci_tango_cs_ops,
>> + },
>> +
>> + /* Generic compatible below here */
>> + { .compatible = "arasan,sdhci-8.9a" },
>> + { .compatible = "arasan,sdhci-5.1" },
>> + { .compatible = "arasan,sdhci-4.9a" },
>> +
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
>> +
>> +static int sdhci_arasan_probe(struct platform_device *pdev)
>> +{
>> + int ret;
>> + const struct of_device_id *match;
>> + struct clk *clk_xin;
>> + struct sdhci_host *host;
>> + struct sdhci_pltfm_host *pltfm_host;
>> + struct sdhci_arasan_data *sdhci_arasan;
>> + const struct sdhci_arasan_cs_ops *cs_ops;
>> +
>> + host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
>> + sizeof(*sdhci_arasan));
>> + if (IS_ERR(host))
>> + return PTR_ERR(host);
>> +
>> + pltfm_host = sdhci_priv(host);
>> + sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>> + sdhci_arasan->host = host;
>> +
>> + match = of_match_device(sdhci_arasan_of_match, &pdev->dev);
>> + if (match)
>> + cs_ops = match->data;
>> +
>> + /* SoC-specific platform init */
>> + if (cs_ops && cs_ops->platform_init) {
>> + ret = cs_ops->platform_init(host, pdev);
>> + if (ret)
>> + goto err_pltfm_free;
>> + }
>> +
>> sdhci_arasan->clk_ahb = devm_clk_get(&pdev->dev, "clk_ahb");
>> if (IS_ERR(sdhci_arasan->clk_ahb)) {
>> dev_err(&pdev->dev, "clk_ahb clock not found.\n");
>> @@ -642,21 +713,14 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>> }
>>
>> sdhci_get_of_property(pdev);
>> -
>> - if (of_property_read_bool(np, "xlnx,fails-without-test-cd"))
>> - sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
>> -
>> pltfm_host->clk = clk_xin;
>>
>> - if (of_device_is_compatible(pdev->dev.of_node,
>> - "rockchip,rk3399-sdhci-5.1"))
>> - sdhci_arasan_update_clockmultiplier(host, 0x0);
>> -
>> - sdhci_arasan_update_baseclkfreq(host);
>> -
>> - ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, &pdev->dev);
>> - if (ret)
>> - goto clk_disable_all;
>> + /* SoC-specific clock init */
>> + if (cs_ops && cs_ops->clock_init) {
>> + ret = cs_ops->clock_init(host, pdev);
>> + if (ret)
>> + goto clk_disable_all;
>> + }
>>
>> ret = mmc_of_parse(host->mmc);
>> if (ret) {
>>
>>
>> If you are ok with it I can clean it up to submit it properly.
^ permalink raw reply
* Adding a .platform_init callback to sdhci_arasan_ops
From: Sebastian Frias @ 2016-12-05 12:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAD=FV=UhVtQQ1PPtLrvra20ja1k9GKs9hyFcrjBUYmyUmQK45Q@mail.gmail.com>
Hi Doug,
On 28/11/16 19:02, Doug Anderson wrote:
> Hi,
>
> On Mon, Nov 28, 2016 at 5:28 AM, Sebastian Frias <sf84@laposte.net> wrote:
>> +static void sdhci_tango4_platform_init(struct sdhci_host *host)
>> +{
>> + printk("%s\n", __func__);
>> +
>> + /*
>> + pad_mode[2:0]=0 must be 0
>> + sel_sdio[3]=1 must be 1 for SDIO
>> + inv_sdwp_pol[4]=0 if set inverts the SD write protect polarity
>> + inv_sdcd_pol[5]=0 if set inverts the SD card present polarity
>> + */
>> + sdhci_writel(host, 0x00000008, 0x100 + 0x0);
>
> If I were doing this, I'd probably actually add these fields to the
> "sdhci_arasan_soc_ctl_map", then add a 3rd option to
> sdhci_arasan_syscon_write(). Right now it has 2 modes: "hiword
> update" and "non-hiword update". You could add a 3rd indicating that
> you set config registers by just writing at some offset of the main
> SDHCI registers space.
>
> So you'd add 4 fields:
>
> .tango_pad_mode = { .reg = 0x0000, .width = 3, .shift = 0 },
> .sel_sdio = { .reg = 0x0000, .width = 1, .shift = 3},
> .inv_sdwp_pol = { .reg = 0x0000, .width = 1, .shift = 4},
> .inv_sdcd_pol = { .reg = 0x0000, .width = 1, .shift = 5},
Right now I'm using something like:
+ val = 0;
+ val |= PAD_MODE(0); /* must be 0 */
+ val |= SEL_SDIO; /* enable SDIO */
+ sdhci_writel(host, val, 0x100 + 0x0);
+
+ val = 0;
+ val |= TIMEOUT_CLK_UNIT_MHZ; /* unit: MHz */
+ val |= TIMEOUT_CLK_FREQ(52); /* timeout clock: 52MHz */
+ val |= BUS_CLK_FREQ_FOR_SD_CLK(200); /* SDIO clock: 200MHz (TODO: get from DT) */
+ val |= MAX_BLOCK_LENGTH(3); /* max block size: 4096 bytes */
+ val |= EXTENDED_MEDIA_BUS_SUPPORTED; /* DT? */
+ val |= ADMA2_SUPPORTED; /* DT? */
+ val |= HIGH_SPEED_SUPPORTED; /* DT? */
+ val |= SDMA_SUPPORTED; /* DT? */
+ val |= SUSPEND_RESUME_SUPPORTED; /* DT? */
+ val |= VOLTAGE_3_3_V_SUPPORTED; /* DT? */
+#if 0
+ val |= VOLTAGE_1_8_V_SUPPORTED; /* DT? */
+#endif
+ val |= ASYNCHRONOUS_IRQ_SUPPORTED; /* DT? */
+ val |= SLOT_TYPE_REMOVABLE; /* DT? */
+ val |= SDR50_SUPPORTED; /* DT? */
+ sdhci_writel(host, val, 0x100 + 0x40);
+
+ val = 0;
+ val |= TIMER_COUNT_FOR_RETUNING(1); /* DT? */
+ val |= CLOCK_MULTIPLIER(0); /* DT? */
+ val |= SPI_MODE_SUPPORTED; /* DT? */
+ val |= SPI_BLOCK_MODE_SUPPORTED; /* DT? */
+ sdhci_writel(host, val, 0x100 + 0x44);
+
+ sdhci_writel(host, 0x0004022c, 0x100 + 0x28);
+ sdhci_writel(host, 0x00000002, 0x100 + 0x2c);
+
+ sdhci_writel(host, 0x00600000, 0x100 + 0x50);
which seems much easier to handle (and portable).
At any rate, one thing to note from this is that many of these
bits should probably be initialised based on DT, right?
For example, the DT has a "non-removable" property, which I think
should be used to setup SLOT_TYPE_EMBEDDED (if the property is
present) or SLOT_TYPE_REMOVABLE (if the property is not present)
Looking at Documentation/devicetree/bindings/mmc/mmc.txt we can see
more related properties:
Optional properties:
- bus-width: Number of data lines, can be <1>, <4>, or <8>. The default
will be <1> if the property is absent.
- wp-gpios: Specify GPIOs for write protection, see gpio binding
- cd-inverted: when present, polarity on the CD line is inverted. See the note
below for the case, when a GPIO is used for the CD line
- wp-inverted: when present, polarity on the WP line is inverted. See the note
below for the case, when a GPIO is used for the WP line
- disable-wp: When set no physical WP line is present. This property should
only be specified when the controller has a dedicated write-protect
detection logic. If a GPIO is always used for the write-protect detection
logic it is sufficient to not specify wp-gpios property in the absence of a WP
line.
- max-frequency: maximum operating clock frequency
- no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
this system, even if the controller claims it is.
- cap-sd-highspeed: SD high-speed timing is supported
- cap-mmc-highspeed: MMC high-speed timing is supported
- sd-uhs-sdr12: SD UHS SDR12 speed is supported
- sd-uhs-sdr25: SD UHS SDR25 speed is supported
- sd-uhs-sdr50: SD UHS SDR50 speed is supported
- sd-uhs-sdr104: SD UHS SDR104 speed is supported
- sd-uhs-ddr50: SD UHS DDR50 speed is supported
...
which makes me wonder, what is the recommended way of dealing with this?
- Should I use properties on the DT? If so, then I need to add code to set
up the register properly.
- Should I hardcode these values use a minimal DT? If so, then I need an
init function to put all this.
- Should I hardcode stuff at u-Boot level? If so, nothing is required and
should work without any modifications to the Arasan Linux driver.
It appears that the Linux driver is expecting most of these fields to be
hardcoded and "pre-set" before (maybe by the bootloader) it starts, hence
the lack of any "init" function so far.
>
> In your platform-specific init you're proposing you could set
> tango_pad_mode to 0. That seems tango-specific.
>
> You'd want to hook into "set_ios" for setting sel_sdio or not. That's
> important if anyone ever wants to plug in an external SDIO card to
> your slot. This one good argument for putting this in
> sdhci_arasan_soc_ctl_map, since you wouldn't want to do a
> compatibility matching every time set_ios is called.
Thanks for the advice, I will look into that.
>
> I'd have to look more into the whole SD/WP polarity issue. There are
> already so many levels of inversion for these signals in Linux that
> it's confusing. It seems like you might just want to hardcode them to
> "0" and let users use all the existing ways to invert things... You
> could either put that hardcoding into your platform init code or (if
> you're using sdhci_arasan_soc_ctl_map) put it in the main init code so
> that if anyone else needs to init similar signals then they can take
> advantage of it.
Yes, I think I will leave them to 0.
>
> --
>
> One random side note is that as currently documented you need to
> specify a "shift" of -1 for any sdhci_arasan_soc_ctl_map fields you
> aren't using. That seems stupid--not sure why I did that. It seems
> better to clue off "width = 0" so that you could just freely not init
> any fields you don't need.
I see.
So far I'm not really convinced about using "soc_ctl_map" because what I
have so far is more portable, and can easily be put as is somewhere else
(i.e.: in different flavors of bootloaders)
Best regards,
Sebastian
^ 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