* Re: [PATCH] Documentation: dt: Explicitly mark Samsung Exynos SoC bindings as unstable
From: Bartlomiej Zolnierkiewicz @ 2016-12-16 14:27 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-samsung-soc, devicetree, linux-arm-kernel, Rob Herring,
Mark Rutland, Krzysztof Kozlowski, Javier Martinez Canillas,
Kukjin Kim, Inki Dae, Seung-Woo Kim, Chanwoo Choi,
Sylwester Nawrocki
In-Reply-To: <1481897676-13578-1-git-send-email-m.szyprowski@samsung.com>
Hi,
On Friday, December 16, 2016 03:14:36 PM Marek Szyprowski wrote:
> Samsung Exynos SoCs and boards related bindings evolved since the initial
> introduction, but initially the bindings were minimal and a bit incomplete
> (they never described all the hardware modules available in the SoCs).
> Since then some significant (not fully compatible) changes have been
> already committed a few times (like gpio replaced by pinctrl, display ddc,
> mfc reserved memory, some core clocks added to various hardware modules,
> added more required nodes).
>
> On the other side there are no boards which have device tree embedded in
> the bootloader. Device tree blob is always compiled from the kernel tree
> and updated together with the kernel image.
>
> Thus to avoid further adding a bunch of workarounds for old/missing
> bindings and allow to make cleanup of the existing code and device tree
> files, lets mark Samsung Exynos SoC platform bindings as unstable. This
> means that bindings can may change at any time and users should use the
> dtb file compiled from the same kernel source tree as the kernel image.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
This change is long overdue..
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
> Documentation/devicetree/bindings/arm/samsung/exynos.txt | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/samsung/exynos.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos.txt b/Documentation/devicetree/bindings/arm/samsung/exynos.txt
> new file mode 100644
> index 000000000000..0c606f4c6e85
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos.txt
> @@ -0,0 +1,12 @@
> +Samsung Exynos SoC Family Device Tree Bindings
> +---------------------------------------------------------------
> +
> +Work in progress statement:
> +
> +Device tree files and bindings applying to Samsung Exynos SoCs and boards are
> +considered "unstable". Any Samsung Exynos device tree binding may change at any
> +time. Be sure to use a device tree binary and a kernel image generated from the
> +same source tree.
> +
> +Please refer to Documentation/devicetree/bindings/ABI.txt for a definition of a
> +stable binding/ABI.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH] ARM: dts: sun8i-q8-common: enable bluetooth on SDIO Wi-Fi
From: Icenowy Zheng @ 2016-12-16 14:40 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans de Goede, Chen-Yu Tsai, linux-kernel,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org
In-Reply-To: <20161216124748.rkvnnlo4x5onzpvk@lukather>
16.12.2016, 20:47, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> On Fri, Dec 09, 2016 at 07:49:00PM +0800, Icenowy Zheng wrote:
>> 2016年12月9日 下午4:07于 Maxime Ripard <maxime.ripard@free-electrons.com>写道：
>> >
>> > On Tue, Dec 06, 2016 at 04:08:38PM +0800, Icenowy Zheng wrote:
>> > > Some SDIO Wi-Fi chips (such as RTL8703AS) have a UART bluetooth, which
>> > > has a dedicated enable pin (PL8 in the reference design).
>> > >
>> > > Enable the pin in the same way as the WLAN enable pins.
>> > >
>> > > Tested on an A33 Q8 tablet with RTL8703AS.
>> > >
>> > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>> > > ---
>> > >
>> > > This patch should be coupled with the uart1 node patch I send before:
>> > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/471997.html
>> > >
>> > > For RTL8703AS, the rtl8723bs bluetooth code is used, which can be retrieve from:
>> > > https://github.com/lwfinger/rtl8723bs_bt
>> > >
>> > > arch/arm/boot/dts/sun8i-q8-common.dtsi | 2 +-
>> > > 1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/arch/arm/boot/dts/sun8i-q8-common.dtsi b/arch/arm/boot/dts/sun8i-q8-common.dtsi
>> > > index c676940..4aeb5bb 100644
>> > > --- a/arch/arm/boot/dts/sun8i-q8-common.dtsi
>> > > +++ b/arch/arm/boot/dts/sun8i-q8-common.dtsi
>> > > @@ -88,7 +88,7 @@
>> > >
>> > > &r_pio {
>> > > wifi_pwrseq_pin_q8: wifi_pwrseq_pin@0 {
>> > > - pins = "PL6", "PL7", "PL11";
>> > > + pins = "PL6", "PL7", "PL8", "PL11";
>> > > function = "gpio_in";
>> > > bias-pull-up;
>> > > };
>> >
>> > There's several things wrong here. The first one is that you rely
>> > solely on the pinctrl state to maintain a reset line. This is very
>> > fragile (especially since the GPIO pinctrl state are likely to go away
>> > at some point), but it also means that if your driver wants to recover
>> > from that situation at some point, it won't work.
>> >
>> > The other one is that the bluetooth and wifi chips are two devices in
>> > linux, and you assign that pin to the wrong device (wifi).
>> >
>> > rfkill-gpio is made just for that, so please use it.
>>
>> The GPIO is not for the radio, but for the full Bluetooth part.
>
> I know.
>
>> If it's set to 0, then the bluetooth part will reset, and the
>> hciattach will fail.
>
> Both rfkill-gpio and rfkill-regulator will shutdown when called
> (either by poking the reset pin or shutting down the regulator), so
> that definitely seems like an expected behavior to put the device in
> reset.
>
>> The BSP uses this as a rfkill, and the result is that the bluetooth
>> on/off switch do not work properly.
>
> Then rfkill needs fixing, but working around it by hoping that the
> core will probe an entirely different device, and enforcing a default
> that the rest of the kernel might or might not change is both fragile
> and wrong.
I think a rfkill-gpio here works just like the BSP rfkill...
The real problem is that the Realtek UART bluetooth driver is a userspace
program (a modified hciattach), which is not capable of the GPIO reset...
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: RFC: extend iommu-map binding to support #iommu-cells > 1
From: Robin Murphy @ 2016-12-16 14:50 UTC (permalink / raw)
To: Stuart Yoder, Mark Rutland
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
will.deacon-5wv7dgnIgG8@public.gmane.org,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
In-Reply-To: <VI1PR0401MB26387C2712FC4BF74B4B96028D9C0-9IDQY6o3qQjcXZ0H4ZLnAo3W/0Ik+aLCnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
On 16/12/16 14:21, Stuart Yoder wrote:
>
>
>> -----Original Message-----
>> From: Mark Rutland [mailto:mark.rutland-5wv7dgnIgG8@public.gmane.org]
>> Sent: Friday, December 16, 2016 5:39 AM
>> To: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>
>> Cc: robin.murphy-5wv7dgnIgG8@public.gmane.org; will.deacon-5wv7dgnIgG8@public.gmane.org; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Bharat Bhushan
>> <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>; Nipun Gupta <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana Madalina Craciun
>> <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>> Subject: Re: RFC: extend iommu-map binding to support #iommu-cells > 1
>>
>> On Fri, Dec 16, 2016 at 02:36:57AM +0000, Stuart Yoder wrote:
>>> For context, please see the thread:
>>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-
>> kernel%2Fmsg539066.html&data=01%7C01%7Cstuart.yoder%40nxp.com%7C0464e12bddfd42e0f0a508d425a847cb%7C686ea
>> 1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=KnzeOlbts271GwD1cpx2FLMsKnxw%2FOdiGVTp6%2BKFrbM%3D&reserved=0
>>>
>>> The existing iommu-map binding did not account for the situation where
>>> #iommu-cells == 2, as permitted in the ARM SMMU binding. The 2nd cell
>>> of the IOMMU specifier being the SMR mask. The existing binding defines
>>> the mapping as:
>>> Any RID r in the interval [rid-base, rid-base + length) is associated with
>>> the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
>>>
>>> ...and that does not work if iommu-base is 2 cells, the second being the
>>> SMR mask.
>>>
>>> While this can be worked around by always having length=1, it seems we
>>> can get this cleaned up by updating the binding definition for iommu-map.
>>
>> To reiterate, I'm very much not keen on the pci-iommu binding having
>> knowledge of the decomposition of iommu-specifiers from other bindings.
>
> With the current definition of iommu-map we already have baked in an
> assumption that an iommu-specifier is a value that can be incremented
> by 1 to get to the next sequential specifier. So the binding already
> has "knowledge" that applies in most, but not all cases.
>
> The generic iommu binding also defines a case where #iommu-cells=4
> for some IOMMUs.
>
> Since the ARM SMMU is a special case, perhaps the intepretation
> of an iommu-specifier in the context of iommu-map should be moved
> into the SMMU binding.
>
>> As mentioned previously, there's an intended interpretation [1] that we
>> need to fix up the pci-iommu binding with. With that, I don't think it's
>> even necessary to bodge iommu-cells = <1> AFAICT.
>
> You had said in the previous thread:
>
> > I had intended that the offset was added to the final cell of the
> > iommu-specifier (i.e. that the iommu-specifier was treated as a large
> > number).
>
> > You can handle this case by adding additional entries in the map table,
> > with a single entry length.
>
> I understand that, and it works, but I don't see why the definition has
> to be that the offset is added to the "final cell".
Because the cells of a specifier form a single opaque big-endian value.
Were DT little-endian it would be the "first cell". To be pedantic, both
of those descriptions are technically wrong because they fail to account
for overflow and carry up into the next cell (in whichever direction).
> Why can't it be
> an iommu specific definition that makes sense for a given IOMMU?
Because the implementation would then become a nightmarish rabbit-warren
of special-cases, largely defeating the point of a *generic* binding. At
the very least it'd have to chase every phandle and determine its
compatible just to work out what to do with any given specifier - merely
thinking about the complexity of the error handling for the number of
additional failure modes that introduces is enough to put me off.
Robin.
>
> Stuart
>
^ permalink raw reply
* Re: [PATCH] Docs: dt: Be explicit and consistent in reference to IOMMU specifiers
From: Mark Rutland @ 2016-12-16 14:56 UTC (permalink / raw)
To: Stuart Yoder, treding-DDmLM1+adcrQT0dZR+AlfA
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
will.deacon-5wv7dgnIgG8@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
In-Reply-To: <VI1PR0401MB263816D0AA09CF94E2B5C7E58D9C0-9IDQY6o3qQjcXZ0H4ZLnAo3W/0Ik+aLCnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
On Fri, Dec 16, 2016 at 02:08:09PM +0000, Stuart Yoder wrote:
>
> > -----Original Message-----
> > From: Mark Rutland [mailto:mark.rutland-5wv7dgnIgG8@public.gmane.org]
> >
> > On Thu, Dec 15, 2016 at 06:16:13PM -0600, Stuart Yoder wrote:
> > > In the iommu-map binding change references to iommu-specifier to
> > > "IOMMU specifier" so we are 100% consistent everywhere with terminology
> > > and capitalization.
> >
> > Elsewhere, we always use lower case "xxx-specifier" or "xxx specifier",
> > e.g. Documentation/devicetree/bindings/gpio/gpio.txt defines
> > "gpio-specifier", ePAPR defines "interrupt specifier".
> >
> > Given we're morstly consistent on "iommu-specifier" today,could we
> > please jsut update the ARM SMMU binding to match that? If we're going to
> > fix the dash mismatch, that's a more general, cross-binding thing.
>
> The notable place where we don't use "iommu-specifier" in in the generic
> IOMMU binding itself where we use "IOMMU specifier".
True; I failed to notice that. You are right in that the pci-iommu
binding is the odd one out. Sorry for the misinformation above. :/
> You're suggesting using "iommu-specifier" everywhere including the
> generic binding? Sounds fine to me. It's a nit but would like to see
> it consistent everywhere.
I certainly agree that we should be consistent.
So FWIW, for this patch (as-is):
Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v4 1/4] mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC
From: Cédric Le Goater @ 2016-12-16 14:56 UTC (permalink / raw)
To: Cyrille Pitchen, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Mark Rutland, Boris Brezillon, devicetree-u79uwXL29TY76Z2rM5mHXA,
Richard Weinberger, Marek Vasut, Rob Herring, Joel Stanley,
Cyrille Pitchen, Brian Norris, David Woodhouse
In-Reply-To: <5566c62d-cc72-f207-e1dd-5a59e6947c24-yU5RGvR974pGWvitb5QawA@public.gmane.org>
Hello Cyrille,
On 12/16/2016 12:15 AM, Cyrille Pitchen wrote:
> Hi Cedric,
>
> Le 12/12/2016 à 16:40, Cédric Le Goater a écrit :
>> This driver adds mtd support for the Aspeed AST2500 SoC static memory
>> controllers :
>>
>> * Firmware SPI Memory Controller (FMC)
>> . BMC firmware
>> . 3 chip select pins (CE0 ~ CE2)
>> . supports SPI type flash memory (CE0-CE1)
>> . CE2 can be of NOR type flash but this is not supported by the
>> driver
>>
>> * SPI Flash Controller (SPI1 and SPI2)
>> . host firmware
>> . 2 chip select pins (CE0 ~ CE1)
>> . supports SPI type flash memory
>>
>> Each controller has a memory range on which it maps its flash module
>> slaves. Each slave is assigned a memory window for its mapping that
>> can be changed at bootime with the Segment Address Register.
>>
>> Each SPI flash slave can then be accessed in two modes: Command and
>> User. When in User mode, accesses to the memory segment of the slaves
>> are translated in SPI transfers. When in Command mode, the HW
>> generates the SPI commands automatically and the memory segment is
>> accessed as if doing a MMIO.
>>
>> Currently, only the User mode is supported. Command mode needs a
>> little more work to check that the memory window on the AHB bus fits
>> the module size.
>>
>> Based on previous work from Milton D. Miller II <miltonm-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>>
>> Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>
>> Reviewed-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
>> ---
>>
>> Changes since v3:
>> - reworked IO routines to use io{read,write}32_rep
>> - changed config option to SPI_ASPEED_SMC
>> - fixed aspeed_smc_chip_setup_init() returned value
>> - merged the use of the "label" property"
>>
>> drivers/mtd/spi-nor/Kconfig | 10 +
>> drivers/mtd/spi-nor/Makefile | 1 +
>> drivers/mtd/spi-nor/aspeed-smc.c | 719 +++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 730 insertions(+)
>> create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c
>>
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 4a682ee0f632..42168e9d6097 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -29,6 +29,16 @@ config MTD_SPI_NOR_USE_4K_SECTORS
>> Please note that some tools/drivers/filesystems may not work with
>> 4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum).
>>
>> +config SPI_ASPEED_SMC
>> + tristate "Aspeed flash controllers in SPI mode"
>> + depends on ARCH_ASPEED || COMPILE_TEST
>> + depends on HAS_IOMEM && OF
>> + help
>> + This enables support for the Firmware Memory controller (FMC)
>> + in the Aspeed AST2500 SoC when attached to SPI NOR chips,
>> + and support for the SPI flash memory controller (SPI) for
>> + the host firmware. The implementation only supports SPI NOR.
>> +
>> config SPI_ATMEL_QUADSPI
>> tristate "Atmel Quad SPI Controller"
>> depends on ARCH_AT91 || (ARM && COMPILE_TEST)
>> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
>> index 121695e83542..6ff64bc7fa0e 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -1,4 +1,5 @@
>> obj-$(CONFIG_MTD_SPI_NOR) += spi-nor.o
>> +obj-$(CONFIG_SPI_ASPEED_SMC) += aspeed-smc.o
>> obj-$(CONFIG_SPI_ATMEL_QUADSPI) += atmel-quadspi.o
>> obj-$(CONFIG_SPI_CADENCE_QUADSPI) += cadence-quadspi.o
>> obj-$(CONFIG_SPI_FSL_QUADSPI) += fsl-quadspi.o
>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>> new file mode 100644
>> index 000000000000..2667ab7aeb9b
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>> @@ -0,0 +1,719 @@
>> +/*
>> + * ASPEED Static Memory Controller driver
>> + *
>> + * Copyright (c) 2015-2016, IBM Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + */
>> +
>> +#include <linux/bug.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/partitions.h>
>> +#include <linux/mtd/spi-nor.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/sysfs.h>
>> +
>> +#define DEVICE_NAME "aspeed-smc"
>> +
>> +/*
>> + * The driver only support SPI flash
>> + */
>> +enum aspeed_smc_flash_type {
>> + smc_type_nor = 0,
>> + smc_type_nand = 1,
>> + smc_type_spi = 2,
>> +};
>> +
>> +struct aspeed_smc_chip;
>> +
>> +struct aspeed_smc_info {
>> + u32 maxsize; /* maximum size of chip window */
>> + u8 nce; /* number of chip enables */
>> + bool hastype; /* flash type field exists in config reg */
>> + u8 we0; /* shift for write enable bit for CE0 */
>> + u8 ctl0; /* offset in regs of ctl for CE0 */
>> +
>> + void (*set_4b)(struct aspeed_smc_chip *chip);
>> +};
>> +
>> +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip);
>> +
>> +static const struct aspeed_smc_info fmc_2500_info = {
>> + .maxsize = 256 * 1024 * 1024,
>> + .nce = 3,
>> + .hastype = true,
>> + .we0 = 16,
>> + .ctl0 = 0x10,
>> + .set_4b = aspeed_smc_chip_set_4b,
>> +};
>> +
>> +static const struct aspeed_smc_info spi_2500_info = {
>> + .maxsize = 128 * 1024 * 1024,
>> + .nce = 2,
>> + .hastype = false,
>> + .we0 = 16,
>> + .ctl0 = 0x10,
>> + .set_4b = aspeed_smc_chip_set_4b,
>> +};
>> +
>> +enum aspeed_smc_ctl_reg_value {
>> + smc_base, /* base value without mode for other commands */
>> + smc_read, /* command reg for (maybe fast) reads */
>> + smc_write, /* command reg for writes */
>> + smc_max,
>> +};
>> +
>> +struct aspeed_smc_controller;
>> +
>> +struct aspeed_smc_chip {
>> + int cs;
>> + struct aspeed_smc_controller *controller;
>> + void __iomem *ctl; /* control register */
>> + void __iomem *ahb_base; /* base of chip window */
>> + u32 ctl_val[smc_max]; /* control settings */
>> + enum aspeed_smc_flash_type type; /* what type of flash */
>> + struct spi_nor nor;
>> +};
>> +
>> +struct aspeed_smc_controller {
>> + struct device *dev;
>> +
>> + struct mutex mutex; /* controller access mutex */
>> + const struct aspeed_smc_info *info; /* type info of controller */
>> + void __iomem *regs; /* controller registers */
>> + void __iomem *ahb_base; /* per-chip windows resource */
>> +
>> + struct aspeed_smc_chip *chips[0]; /* pointers to attached chips */
>> +};
>> +
>> +/*
>> + * SPI Flash Configuration Register (AST2500 SPI)
>> + * or
>> + * Type setting Register (AST2500 FMC).
>> + * CE0 and CE1 can only be of type SPI. CE2 can be of type NOR but the
>> + * driver does not support it.
>> + */
>> +#define CONFIG_REG 0x0
>> +#define CONFIG_DISABLE_LEGACY BIT(31) /* 1 */
>> +
>> +#define CONFIG_CE2_WRITE BIT(18)
>> +#define CONFIG_CE1_WRITE BIT(17)
>> +#define CONFIG_CE0_WRITE BIT(16)
>> +
>> +#define CONFIG_CE2_TYPE BIT(4) /* AST2500 FMC only */
>> +#define CONFIG_CE1_TYPE BIT(2) /* AST2500 FMC only */
>> +#define CONFIG_CE0_TYPE BIT(0) /* AST2500 FMC only */
>> +
>> +/*
>> + * CE Control Register
>> + */
>> +#define CE_CONTROL_REG 0x4
>> +
>> +/*
>> + * CEx Control Register
>> + */
>> +#define CONTROL_AAF_MODE BIT(31)
>> +#define CONTROL_IO_MODE_MASK GENMASK(30, 28)
>> +#define CONTROL_IO_DUAL_DATA BIT(29)
>> +#define CONTROL_IO_DUAL_ADDR_DATA (BIT(29) | BIT(28))
>> +#define CONTROL_IO_QUAD_DATA BIT(30)
>> +#define CONTROL_IO_QUAD_ADDR_DATA (BIT(30) | BIT(28))
>> +#define CONTROL_CE_INACTIVE_SHIFT 24
>> +#define CONTROL_CE_INACTIVE_MASK GENMASK(27, \
>> + CONTROL_CE_INACTIVE_SHIFT)
>> +/* 0 = 16T ... 15 = 1T T=HCLK */
>> +#define CONTROL_COMMAND_SHIFT 16
>> +#define CONTROL_DUMMY_COMMAND_OUT BIT(15)
>> +#define CONTROL_IO_DUMMY_HI BIT(14)
>> +#define CONTROL_IO_DUMMY_HI_SHIFT 14
>> +#define CONTROL_CLK_DIV4 BIT(13) /* others */
>> +#define CONTROL_RW_MERGE BIT(12)
>> +#define CONTROL_IO_DUMMY_LO_SHIFT 6
>> +#define CONTROL_IO_DUMMY_LO GENMASK(7, \
>> + CONTROL_IO_DUMMY_LO_SHIFT)
>> +#define CONTROL_IO_DUMMY_MASK (CONTROL_IO_DUMMY_HI | \
>> + CONTROL_IO_DUMMY_LO)
>> +#define CONTROL_IO_DUMMY_SET(dummy) \
>> + (((((dummy) >> 2) & 0x1) << CONTROL_IO_DUMMY_HI_SHIFT) | \
>> + (((dummy) & 0x3) << CONTROL_IO_DUMMY_LO_SHIFT))
>> +
>> +#define CONTROL_CLOCK_FREQ_SEL_SHIFT 8
>> +#define CONTROL_CLOCK_FREQ_SEL_MASK GENMASK(11, \
>> + CONTROL_CLOCK_FREQ_SEL_SHIFT)
>> +#define CONTROL_LSB_FIRST BIT(5)
>> +#define CONTROL_CLOCK_MODE_3 BIT(4)
>> +#define CONTROL_IN_DUAL_DATA BIT(3)
>> +#define CONTROL_CE_STOP_ACTIVE_CONTROL BIT(2)
>> +#define CONTROL_COMMAND_MODE_MASK GENMASK(1, 0)
>> +#define CONTROL_COMMAND_MODE_NORMAL 0
>> +#define CONTROL_COMMAND_MODE_FREAD 1
>> +#define CONTROL_COMMAND_MODE_WRITE 2
>> +#define CONTROL_COMMAND_MODE_USER 3
>> +
>> +#define CONTROL_KEEP_MASK \
>> + (CONTROL_AAF_MODE | CONTROL_CE_INACTIVE_MASK | CONTROL_CLK_DIV4 | \
>> + CONTROL_IO_DUMMY_MASK | CONTROL_CLOCK_FREQ_SEL_MASK | \
>> + CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
>> +
>> +/*
>> + * The Segment Register uses a 8MB unit to encode the start address
>> + * and the end address of the mapping window of a flash SPI slave :
>> + *
>> + * | byte 1 | byte 2 | byte 3 | byte 4 |
>> + * +--------+--------+--------+--------+
>> + * | end | start | 0 | 0 |
>> + */
>> +#define SEGMENT_ADDR_REG0 0x30
>> +#define SEGMENT_ADDR_START(_r) ((((_r) >> 16) & 0xFF) << 23)
>> +#define SEGMENT_ADDR_END(_r) ((((_r) >> 24) & 0xFF) << 23)
>> +
>> +/*
>> + * In user mode all data bytes read or written to the chip decode address
>> + * range are transferred to or from the SPI bus. The range is treated as a
>> + * fifo of arbitratry 1, 2, or 4 byte width but each write has to be aligned
>> + * to its size. The address within the multiple 8kB range is ignored when
>> + * sending bytes to the SPI bus.
>> + *
>> + * On the arm architecture, as of Linux version 4.3, memcpy_fromio and
>> + * memcpy_toio on little endian targets use the optimized memcpy routines
>> + * that were designed for well behavied memory storage. These routines
>> + * have a stutter if the source and destination are not both word aligned,
>> + * once with a duplicate access to the source after aligning to the
>> + * destination to a word boundary, and again with a duplicate access to
>> + * the source when the final byte count is not word aligned.
>> + *
>> + * When writing or reading the fifo this stutter discards data or sends
>> + * too much data to the fifo and can not be used by this driver.
>> + *
>> + * While the low level io string routines that implement the insl family do
>> + * the desired accesses and memory increments, the cross architecture io
>> + * macros make them essentially impossible to use on a memory mapped address
>> + * instead of a a token from the call to iomap of an io port.
>> + *
>> + * These fifo routines use readl and friends to a constant io port and update
>> + * the memory buffer pointer and count via explicit code. The final updates
>> + * to len are optimistically suppressed.
>> + */
>> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src,
>> + size_t len)
>> +{
>> + if (IS_ALIGNED((uintptr_t)src, sizeof(uintptr_t)) &&
>> + IS_ALIGNED((uintptr_t)buf, sizeof(uintptr_t)) &&
>> + IS_ALIGNED(len, sizeof(u32))) {
>> + ioread32_rep(src, buf, len >> 2);
>> + } else {
>> + ioread8_rep(src, buf, len);
>> + }
>> + return 0;
>> +}
>> +
>
> Maybe It might be something like:
>
> size_t offset = 0;
>
> if (IS_ALIGNED((uintptr_t)src, sizeof(uintptr_t)) &&
> IS_ALIGNED((uintptr_t)buf, sizeof(uintptr_t))) {
> ioread32_rep(src, buf, len >> 2);
> offset = len & ~0x3;
> len -= offset;
> }
> ioread8_rep(src, (const u8 *)buf + offset, len);
yes. We had an earlier version doing something similar, not as well
crafted tough.
> I assume the Aspeed SPI controller allows to read as much 32-bit words
> as possible before reading the remaining bytes.
yes.
> This is just a suggested optimization, no need to use it if you don't
> want to.
well, it depends if there is a v4. If so, I will.
> In v3, with readl()/readb(), you used to increment both src and buf in
> your while() loop but now with ioreadX_rep() only buf is incremented: it
> always reads from src without incrementing this latest address.
>
> I guess it means that the Aspeed SPI controller doesn't care of the
> actual value of src as long as it lays inside the chip address range.
yes :) in 'User' mode, the address has no meaning.
The previous routine was practical to cover both mode: the currently
supported 'User' mode in which we read/write the ops in the memory window
of the flash, and the 'Command' mode in which we read/write directly the
flash contents from the AHB bus. This mode requires a preliminary setup
of the CEx Control Register, which is what the ctl_val field is for.
We have postponed 'Command' mode for the moment because we have flash
modules which exceed the maximum window size on some boards, and 'Command'
mode does not work in that case. Covering this mode and these special
cases needs more work. 'User' mode is simpler to start with but the code
prepares ground for the other mode.
> This is what you explain in the 1st paragraph of the comment, isn't it?
yes. That might be a little outdated.
>> +static int aspeed_smc_write_to_ahb(void __iomem *dst, const void *buf,
>> + size_t len)
>> +{
>> + if (IS_ALIGNED((uintptr_t)dst, sizeof(uintptr_t)) &&
>> + IS_ALIGNED((uintptr_t)buf, sizeof(uintptr_t)) &&
>> + IS_ALIGNED(len, sizeof(u32))) {
>> + iowrite32_rep(dst, buf, len >> 2);
>> + } else {
>> + iowrite8_rep(dst, buf, len);
>> + }
>> + return 0;
>> +}
>> +
>> +static inline u32 aspeed_smc_chip_write_bit(struct aspeed_smc_chip *chip)
>> +{
>> + return BIT(chip->controller->info->we0 + chip->cs);
>> +}
>> +
>> +static void aspeed_smc_chip_check_config(struct aspeed_smc_chip *chip)
>> +{
>> + struct aspeed_smc_controller *controller = chip->controller;
>> + u32 reg;
>> +
>> + reg = readl(controller->regs + CONFIG_REG);
>> +
>> + if (reg & aspeed_smc_chip_write_bit(chip))
>> + return;
>> +
>> + dev_dbg(controller->dev, "config write is not set ! @%p: 0x%08x\n",
>> + controller->regs + CONFIG_REG, reg);
>> + reg |= aspeed_smc_chip_write_bit(chip);
>> + writel(reg, controller->regs + CONFIG_REG);
>> +}
>> +
>> +static void aspeed_smc_start_user(struct spi_nor *nor)
>> +{
>> + struct aspeed_smc_chip *chip = nor->priv;
>> + u32 ctl = chip->ctl_val[smc_base];
>> +
>> + /*
>> + * When the chip is controlled in user mode, we need write
>> + * access to send the opcodes to it. So check the config.
>> + */
>> + aspeed_smc_chip_check_config(chip);
>> +
>> + ctl |= CONTROL_COMMAND_MODE_USER |
>> + CONTROL_CE_STOP_ACTIVE_CONTROL;
>> + writel(ctl, chip->ctl);
>> +
>> + ctl &= ~CONTROL_CE_STOP_ACTIVE_CONTROL;
>> + writel(ctl, chip->ctl);
>> +}
>> +
>> +static void aspeed_smc_stop_user(struct spi_nor *nor)
>> +{
>> + struct aspeed_smc_chip *chip = nor->priv;
>> +
>> + u32 ctl = chip->ctl_val[smc_read];
>> + u32 ctl2 = ctl | CONTROL_COMMAND_MODE_USER |
>> + CONTROL_CE_STOP_ACTIVE_CONTROL;
>> +
>> + writel(ctl2, chip->ctl); /* stop user CE control */
>> + writel(ctl, chip->ctl); /* default to fread or read mode */
>> +}
>> +
>
> This driver seems to use only the "USER" mode so why do you go back the
> the "FREAD" or "READ" modes at the very end of aspeed_smc_stop_user() as
> the comment suggests?
>
> Do you plan to implement other modes later?
yes. I would like to, I have some code for it already but there some
little issues as said above.
> Can't you just stay in "USER" mode?
well, yes. we are also preparing ground for the Command mode and
the DMA support.
> I guess you just need the chip-select control part.
yes.
>> +static int aspeed_smc_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>> +{
>> + struct aspeed_smc_chip *chip = nor->priv;
>> +
>> + mutex_lock(&chip->controller->mutex);
>> + return 0;
>> +}
>> +
>> +static void aspeed_smc_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>> +{
>> + struct aspeed_smc_chip *chip = nor->priv;
>> +
>> + mutex_unlock(&chip->controller->mutex);
>> +}
>> +
>> +static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>> +{
>> + struct aspeed_smc_chip *chip = nor->priv;
>> +
>> + aspeed_smc_start_user(nor);
>> + aspeed_smc_write_to_ahb(chip->ahb_base, &opcode, 1);
>> + aspeed_smc_read_from_ahb(buf, chip->ahb_base, len);
>> + aspeed_smc_stop_user(nor);
>> + return 0;
>> +}
>> +
>> +static int aspeed_smc_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
>> + int len)
>> +{
>> + struct aspeed_smc_chip *chip = nor->priv;
>> +
>> + aspeed_smc_start_user(nor);
>> + aspeed_smc_write_to_ahb(chip->ahb_base, &opcode, 1);
>> + aspeed_smc_write_to_ahb(chip->ahb_base, buf, len);
>> + aspeed_smc_stop_user(nor);
>> + return 0;
>> +}
>> +
>> +static void aspeed_smc_send_cmd_addr(struct spi_nor *nor, u8 cmd, u32 addr)
>> +{
>> + struct aspeed_smc_chip *chip = nor->priv;
>> + __be32 temp;
>> + u32 cmdaddr;
>> +
>> + switch (nor->addr_width) {
>> + default:
>> + WARN_ONCE(1, "Unexpected address width %u, defaulting to 3\n",
>> + nor->addr_width);
>> + /* FALLTHROUGH */
>> + case 3:
>> + cmdaddr = addr & 0xFFFFFF;
>> + cmdaddr |= cmd << 24;
>> +
>> + temp = cpu_to_be32(cmdaddr);
>> + aspeed_smc_write_to_ahb(chip->ahb_base, &temp, 4);
>> + break;
>> + case 4:
>> + temp = cpu_to_be32(addr);
>> + aspeed_smc_write_to_ahb(chip->ahb_base, &cmd, 1);
>> + aspeed_smc_write_to_ahb(chip->ahb_base, &temp, 4);
>> + break;
>> + }
>> +}
>> +
>> +static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>> + size_t len, u_char *read_buf)
>> +{
>> + struct aspeed_smc_chip *chip = nor->priv;
>> +
>> + aspeed_smc_start_user(nor);
>> + aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>
> Here, please check nor->read_dummy to write the relevant number dummy
> bytes between the address and data cycles.
>
> It should not need too much work to add support to the dummy clock
> cycles and it's more reliable/safe.
>
> Indeed, even if you call the current spi_nor_scan() function with the
> enum read_mode SPI_NOR_NORMAL value, this function just doesn't care and
> selects the Fast Read (0Bh) command instead of the Read (03h) command
> for nor->read_opcode if the "m25p,fast-read" DT property is set.
>
> So if any end user sets this property in a custom DT,
> aspeed_smc_read_user() would just fail.
>
> Hence I think it's worth dealing with dummy cycles now rather than later.
>
> Actually all (Fast) Read commands but the legacy Read (03h) command need
> dummy cycles. So the Read SFDP (5Ah) command does.
>
> For all the (Q)SPI memories I've seen till now, the default factory
> settings for the number of dummy cycles are chosen so it always
> corresponds to entire bytes, whatever the SPI protocol is (SPI 1-1-2,
> 1-2-2, 1-1-4, 1-4-4, ...).
>
> Besides, I recommend you use the 0xFF value for dummy cycles: this value
> prevents the memory from entering its continuous mode by mistake.
> The 0xFF value works for all manufacturers. The SFDP specification seems
> to confirm that.
OK. I will take a closer look at that.
>
>> + aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>> + aspeed_smc_stop_user(nor);
>> + return len;
>> +}
>> +
>> +static ssize_t aspeed_smc_write_user(struct spi_nor *nor, loff_t to,
>> + size_t len, const u_char *write_buf)
>> +{
>> + struct aspeed_smc_chip *chip = nor->priv;
>> +
>> + aspeed_smc_start_user(nor);
>> + aspeed_smc_send_cmd_addr(nor, nor->program_opcode, to);
>> + aspeed_smc_write_to_ahb(chip->ahb_base, write_buf, len);
>> + aspeed_smc_stop_user(nor);
>> + return len;
>> +}
>> +
>> +static int aspeed_smc_unregister(struct aspeed_smc_controller *controller)
>> +{
>> + struct aspeed_smc_chip *chip;
>> + int n;
>> +
>> + for (n = 0; n < controller->info->nce; n++) {
>> + chip = controller->chips[n];
>> + if (chip)
>> + mtd_device_unregister(&chip->nor.mtd);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_smc_remove(struct platform_device *dev)
>> +{
>> + return aspeed_smc_unregister(platform_get_drvdata(dev));
>> +}
>> +
>> +static const struct of_device_id aspeed_smc_matches[] = {
>> + { .compatible = "aspeed,ast2500-fmc", .data = &fmc_2500_info },
>> + { .compatible = "aspeed,ast2500-spi", .data = &spi_2500_info },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, aspeed_smc_matches);
>> +
>> +/*
>> + * Each chip has a mapping window defined by a segment address
>> + * register defining a start and an end address on the AHB bus. These
>> + * addresses can be configured to fit the chip size and offer a
>> + * contiguous memory region across chips. For the moment, we only
>> + * check that each chip segment is valid.
>> + */
>> +static void __iomem *aspeed_smc_chip_base(struct aspeed_smc_chip *chip,
>> + struct resource *res)
>> +{
>> + struct aspeed_smc_controller *controller = chip->controller;
>> + u32 offset = 0;
>> + u32 reg;
>> +
>> + if (controller->info->nce > 1) {
>> + reg = readl(controller->regs + SEGMENT_ADDR_REG0 +
>> + chip->cs * 4);
>> +
>> + if (SEGMENT_ADDR_START(reg) >= SEGMENT_ADDR_END(reg))
>> + return NULL;
>> +
>> + offset = SEGMENT_ADDR_START(reg) - res->start;
>> + }
>> +
>> + return controller->ahb_base + offset;
>> +}
>> +
>> +static void aspeed_smc_chip_enable_write(struct aspeed_smc_chip *chip)
>> +{
>> + struct aspeed_smc_controller *controller = chip->controller;
>> + u32 reg;
>> +
>> + reg = readl(controller->regs + CONFIG_REG);
>> +
>> + reg |= aspeed_smc_chip_write_bit(chip);
>> + writel(reg, controller->regs + CONFIG_REG);
>> +}
>> +
>> +static void aspeed_smc_chip_set_type(struct aspeed_smc_chip *chip, int type)
>> +{
>> + struct aspeed_smc_controller *controller = chip->controller;
>> + u32 reg;
>> +
>> + chip->type = type;
>> +
>> + reg = readl(controller->regs + CONFIG_REG);
>> + reg &= ~(3 << (chip->cs * 2));
>> + reg |= chip->type << (chip->cs * 2);
>> + writel(reg, controller->regs + CONFIG_REG);
>> +}
>> +
>> +/*
>> + * The AST2500 FMC flash controller should be strapped by hardware, or
>> + * autodetected, but the AST2500 SPI flash needs to be set.
>> + */
>> +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip)
>> +{
>> + struct aspeed_smc_controller *controller = chip->controller;
>> + u32 reg;
>> +
>> + if (chip->controller->info == &spi_2500_info) {
>> + reg = readl(controller->regs + CE_CONTROL_REG);
>> + reg |= 1 << chip->cs;
>> + writel(reg, controller->regs + CE_CONTROL_REG);
>> + }
>> +}
>> +
>> +static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip,
>> + struct resource *res)
>> +{
>> + struct aspeed_smc_controller *controller = chip->controller;
>> + const struct aspeed_smc_info *info = controller->info;
>> + u32 reg, base_reg;
>> +
>> + /*
>> + * Always turn on the write enable bit to allow opcodes to be
>> + * sent in user mode.
>> + */
>> + aspeed_smc_chip_enable_write(chip);
>> +
>> + /* The driver only supports SPI type flash */
>> + if (info->hastype)
>> + aspeed_smc_chip_set_type(chip, smc_type_spi);
>> +
>> + /*
>> + * Configure chip base address in memory
>> + */
>> + chip->ahb_base = aspeed_smc_chip_base(chip, res);
>> + if (!chip->ahb_base) {
>> + dev_warn(chip->nor.dev, "CE segment window closed.\n");
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * Get value of the inherited control register. U-Boot usually
>> + * does some timing calibration on the FMC chip, so it's good
>> + * to keep them. In the future, we should handle calibration
>> + * from Linux.
>> + */
>> + reg = readl(chip->ctl);
>> + dev_dbg(controller->dev, "control register: %08x\n", reg);
>> +
>> + base_reg = reg & CONTROL_KEEP_MASK;
>> + if (base_reg != reg) {
>> + dev_info(controller->dev,
>> + "control register changed to: %08x\n",
>> + base_reg);
>
> dev_dbg() should be enough: end users don't know what to do with the new
> control register value, do they?
>
> This is just a suggestion, you can keep dev_info() if you want, I don't
> mind :)
No, you are right. This is debug stuff. We had a bunch of user space tools
poking in the SMC controller MMIO region in early days and it was nice to
know what was the initial setup.
>> + }
>> + chip->ctl_val[smc_base] = base_reg;
>> +
>> + /*
>> + * Retain the prior value of the control register as the
>> + * default if it was normal access mode. Otherwise start with
>> + * the sanitized base value set to read mode.
>> + */
>> + if ((reg & CONTROL_COMMAND_MODE_MASK) ==
>> + CONTROL_COMMAND_MODE_NORMAL)
>> + chip->ctl_val[smc_read] = reg;
>> + else
>> + chip->ctl_val[smc_read] = chip->ctl_val[smc_base] |
>> + CONTROL_COMMAND_MODE_NORMAL;
>> +
>> + dev_dbg(controller->dev, "default control register: %08x\n",
>> + chip->ctl_val[smc_read]);
>> + return 0;
>> +}
>> +
>> +static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>> +{
>> + struct aspeed_smc_controller *controller = chip->controller;
>> + const struct aspeed_smc_info *info = controller->info;
>> + u32 cmd;
>> +
>> + if (chip->nor.addr_width == 4 && info->set_4b)
>> + info->set_4b(chip);
>> +
>> + /*
>> + * base mode has not been optimized yet. use it for writes.
>> + */
>> + chip->ctl_val[smc_write] = chip->ctl_val[smc_base] |
>> + chip->nor.program_opcode << CONTROL_COMMAND_SHIFT |
>> + CONTROL_COMMAND_MODE_WRITE;
>> +
>> + dev_dbg(controller->dev, "write control register: %08x\n",
>> + chip->ctl_val[smc_write]);
>> +
>> + /*
>> + * TODO: Adjust clocks if fast read is supported and interpret
>> + * SPI-NOR flags to adjust controller settings.
>> + */
>> + switch (chip->nor.flash_read) {
>> + case SPI_NOR_NORMAL:
>> + cmd = CONTROL_COMMAND_MODE_NORMAL;
>> + break;
>> + case SPI_NOR_FAST:
>> + cmd = CONTROL_COMMAND_MODE_FREAD;
>> + break;
>> + default:
>> + dev_err(chip->nor.dev, "unsupported SPI read mode\n");
>> + return -EINVAL;
>> + }
>> +
>> + chip->ctl_val[smc_read] |= cmd |
>> + CONTROL_IO_DUMMY_SET(chip->nor.read_dummy / 8);
>> +
>> + dev_dbg(controller->dev, "base control register: %08x\n",
>> + chip->ctl_val[smc_read]);
>> + return 0;
>> +}
>> +
>
> Why do you configure both chip->ctrl_val[smc_write] and
> chip->ctrl_val[smc_read] if the driver actually only uses
> chip->ctrl_val[smc_base] ?
we expect Command mode support and DMAs will use it.
> all aspeed_smc_[read|write]_[reg|user]() functions call
> aspeed_smc_[start|stop]_user(), so this driver always selects the "USER"
> mode and configures the control register based on chip->ctrl_val[smc_base].
yes.
Maybe you think it is too early to prepare ground for the other
mode ? Is that really confusing in the code ? Do you think I should
remove it for the initial support in the driver and stick to 'User'
mode.
>> +static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>> + struct device_node *np, struct resource *r)
>> +{
>> + const struct aspeed_smc_info *info = controller->info;
>> + struct device *dev = controller->dev;
>> + struct device_node *child;
>> + unsigned int cs;
>> + int ret = -ENODEV;
>> +
>> + for_each_available_child_of_node(np, child) {
>> + struct aspeed_smc_chip *chip;
>> + struct spi_nor *nor;
>> + struct mtd_info *mtd;
>> +
>> + /* This driver does not support NAND or NOR flash devices. */
>> + if (!of_device_is_compatible(child, "jedec,spi-nor"))
>> + continue;
>> +
>> + ret = of_property_read_u32(child, "reg", &cs);
>> + if (ret) {
>> + dev_err(dev, "Couldn't not read chip select.\n");
>> + break;
>> + }
>> +
>> + if (cs >= info->nce) {
>> + dev_err(dev, "Chip select %d out of range.\n",
>> + cs);
>> + ret = -ERANGE;
>> + break;
>> + }
>> +
>> + if (controller->chips[cs]) {
>> + dev_err(dev, "Chip select %d already in use by %s\n",
>> + cs, dev_name(controller->chips[cs]->nor.dev));
>> + ret = -EBUSY;
>> + break;
>> + }
>> +
>> + chip = devm_kzalloc(controller->dev, sizeof(*chip), GFP_KERNEL);
>> + if (!chip) {
>> + ret = -ENOMEM;
>> + break;
>> + }
>> +
>> + chip->controller = controller;
>> + chip->ctl = controller->regs + info->ctl0 + cs * 4;
>> + chip->cs = cs;
>> +
>> + nor = &chip->nor;
>> + mtd = &nor->mtd;
>> +
>> + nor->dev = dev;
>> + nor->priv = chip;
>> + spi_nor_set_flash_node(nor, child);
>> + nor->read = aspeed_smc_read_user;
>> + nor->write = aspeed_smc_write_user;
>> + nor->read_reg = aspeed_smc_read_reg;
>> + nor->write_reg = aspeed_smc_write_reg;
>> + nor->prepare = aspeed_smc_prep;
>> + nor->unprepare = aspeed_smc_unprep;
>> +
>> + mtd->name = of_get_property(child, "label", NULL);
>
> This new "label" DT property should be removed from this patch and send
> in a dedicated patch to be discussed separately. However I agree with
> Marek: it looks generic so maybe it could be managed directly from
> mtd_device_register() since setting such as name could also be done when
> using a NAND flash. Anyway, I don't see the link between this name and
> spi-nor. Hence I don't think the DT property should be documented in
> jedec,spi-nor.txt.
OK. I will remove it in the next version.
> Be patient because I expect such a topic to be discussed quite a long
> time before we all agree, even if this is "just" a new DT property ;)
yeah. I expected that also :) But it is quite pratical to give user
space a hint on the flash nature. Systems can have up to 4 different
ones. So there is need for it IMO.
How should I proceed then ? Shall I start a discussion with a single
patch changing mtd_device_register() ? but I need to know which binding
would be the more consensual for such a prop.
Thanks,
C.
>
> Best regards,
>
> Cyrille
>
>
>> +
>> + ret = aspeed_smc_chip_setup_init(chip, r);
>> + if (ret)
>> + break;
>> +
>> + /*
>> + * TODO: Add support for SPI_NOR_QUAD and SPI_NOR_DUAL
>> + * attach when board support is present as determined
>> + * by of property.
>> + */
>> + ret = spi_nor_scan(nor, NULL, SPI_NOR_NORMAL);
>> + if (ret)
>> + break;
>> +
>> + ret = aspeed_smc_chip_setup_finish(chip);
>> + if (ret)
>> + break;
>> +
>> + ret = mtd_device_register(mtd, NULL, 0);
>> + if (ret)
>> + break;
>> +
>> + controller->chips[cs] = chip;
>> + }
>> +
>> + if (ret)
>> + aspeed_smc_unregister(controller);
>> +
>> + return ret;
>> +}
>> +
>> +static int aspeed_smc_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct device *dev = &pdev->dev;
>> + struct aspeed_smc_controller *controller;
>> + const struct of_device_id *match;
>> + const struct aspeed_smc_info *info;
>> + struct resource *res;
>> + int ret;
>> +
>> + match = of_match_device(aspeed_smc_matches, &pdev->dev);
>> + if (!match || !match->data)
>> + return -ENODEV;
>> + info = match->data;
>> +
>> + controller = devm_kzalloc(&pdev->dev, sizeof(*controller) +
>> + info->nce * sizeof(controller->chips[0]), GFP_KERNEL);
>> + if (!controller)
>> + return -ENOMEM;
>> + controller->info = info;
>> + controller->dev = dev;
>> +
>> + mutex_init(&controller->mutex);
>> + platform_set_drvdata(pdev, controller);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + controller->regs = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(controller->regs)) {
>> + dev_err(dev, "Cannot remap controller address.\n");
>> + return PTR_ERR(controller->regs);
>> + }
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + controller->ahb_base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(controller->ahb_base)) {
>> + dev_err(dev, "Cannot remap controller address.\n");
>> + return PTR_ERR(controller->ahb_base);
>> + }
>> +
>> + ret = aspeed_smc_setup_flash(controller, np, res);
>> + if (ret)
>> + dev_err(dev, "Aspeed SMC probe failed %d\n", ret);
>> +
>> + return ret;
>> +}
>> +
>> +static struct platform_driver aspeed_smc_driver = {
>> + .probe = aspeed_smc_probe,
>> + .remove = aspeed_smc_remove,
>> + .driver = {
>> + .name = DEVICE_NAME,
>> + .of_match_table = aspeed_smc_matches,
>> + }
>> +};
>> +
>> +module_platform_driver(aspeed_smc_driver);
>> +
>> +MODULE_DESCRIPTION("ASPEED Static Memory Controller Driver");
>> +MODULE_AUTHOR("Cedric Le Goater <clg-Bxea+6Xhats@public.gmane.org>");
>> +MODULE_LICENSE("GPL v2");
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: RFC: extend iommu-map binding to support #iommu-cells > 1
From: Stuart Yoder @ 2016-12-16 15:06 UTC (permalink / raw)
To: Bharat Bhushan, Mark Rutland,
robin.murphy-5wv7dgnIgG8@public.gmane.org,
will.deacon-5wv7dgnIgG8@public.gmane.org
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
In-Reply-To: <AM5PR0401MB25455AE3850ED6E724B025EF9A9C0-oQ3wXcTHOqrg6d/1FbYcvI3W/0Ik+aLCnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
> -----Original Message-----
> From: Bharat Bhushan
> Sent: Thursday, December 15, 2016 9:46 PM
> To: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>; Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>; robin.murphy-5wv7dgnIgG8@public.gmane.org;
> will.deacon-5wv7dgnIgG8@public.gmane.org
> Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Nipun Gupta <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana Madalina Craciun
> <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Subject: RE: RFC: extend iommu-map binding to support #iommu-cells > 1
>
>
>
> > -----Original Message-----
> > From: Stuart Yoder
> > Sent: Friday, December 16, 2016 8:07 AM
> > To: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>; robin.murphy-5wv7dgnIgG8@public.gmane.org;
> > will.deacon-5wv7dgnIgG8@public.gmane.org
> > Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Bharat Bhushan <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>;
> > Nipun Gupta <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana Madalina Craciun
> > <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; iommu-cunTk1MwBs/ROKNJybVBZg@public.gmane.org
> > foundation.org
> > Subject: RFC: extend iommu-map binding to support #iommu-cells > 1
> >
> > For context, please see the thread:
> > https://www.spinics.net/lists/arm-kernel/msg539066.html
> >
> > The existing iommu-map binding did not account for the situation where
> > #iommu-cells == 2, as permitted in the ARM SMMU binding. The 2nd cell of
> > the IOMMU specifier being the SMR mask. The existing binding defines the
> > mapping as:
> > Any RID r in the interval [rid-base, rid-base + length) is associated with
> > the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
> >
> > ...and that does not work if iommu-base is 2 cells, the second being the SMR
> > mask.
> >
> > While this can be worked around by always having length=1, it seems we can
> > get this cleaned up by updating the binding definition for iommu-map.
> >
> > See patch below. Thoughts?
> >
> > Thanks,
> > Stuart
> >
> > -------------------------------------------------------------------------
> >
> > diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt
> > b/Documentation/devicetree/bindings/pci/pci-iommu.txt
> > index 56c8296..e81b461 100644
> > --- a/Documentation/devicetree/bindings/pci/pci-iommu.txt
> > +++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
> > @@ -38,8 +38,20 @@ Optional properties
> > The property is an arbitrary number of tuples of
> > (rid-base,iommu,iommu-base,length).
> >
> > - Any RID r in the interval [rid-base, rid-base + length) is associated with
> > - the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
> > + If the associated IOMMU has an #iommu-cells value of 1, any RID r in
> > + the interval [rid-base, rid-base + length) is associated with the
> > + listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
> > +
> > + ARM SMMU Note:
> > + The ARM SMMU binding permits an #iommu-cells value of 2 and in this
> > + case defines an IOMMU specifier to be: (stream-id,smr-mask)
> > +
> > + In an iommu-map this means the iommu-base consists of 2 cells:
> > + (rid-base,iommu,[stream-id,smr-mask],length).
> > +
> > + In this case the RID to IOMMU specifier mapping is defined to be:
> > + any RID r in the interval [rid-base, rid-base + length) is associated
> > + with the listed IOMMU, with the iommu-specifier (r - rid-base + stream-
> > id).
>
> Should not this be (r - rid-base + (stream-id & smr-mask)) ?
No, the SMR mask is not applied here-- that is programmed in the SMMU
hardware and not applied by software. The SMR mask in the case of NXP
is 0x7C00, and so is the inverse of a typical mask of 1 bits.
If the map was defined as: <0x0 &smmu 0x10 0x7c00 10>;
An RID value of 0x2 would get mapped to the 2-cell specifier: <0x12 0x7c00>
Stuart
^ permalink raw reply
* RE: RFC: extend iommu-map binding to support #iommu-cells > 1
From: Bharat Bhushan @ 2016-12-16 15:07 UTC (permalink / raw)
To: Robin Murphy, Stuart Yoder, Mark Rutland
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
will.deacon-5wv7dgnIgG8@public.gmane.org,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
In-Reply-To: <07c70fde-2bce-a517-0f5f-a2405049c87c-5wv7dgnIgG8@public.gmane.org>
> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy-5wv7dgnIgG8@public.gmane.org]
> Sent: Friday, December 16, 2016 8:21 PM
> To: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>; Mark Rutland
> <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: will.deacon-5wv7dgnIgG8@public.gmane.org; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Bharat Bhushan
> <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>; Nipun Gupta <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana
> Madalina Craciun <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Subject: Re: RFC: extend iommu-map binding to support #iommu-cells > 1
>
> On 16/12/16 14:21, Stuart Yoder wrote:
> >
> >
> >> -----Original Message-----
> >> From: Mark Rutland [mailto:mark.rutland-5wv7dgnIgG8@public.gmane.org]
> >> Sent: Friday, December 16, 2016 5:39 AM
> >> To: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>
> >> Cc: robin.murphy-5wv7dgnIgG8@public.gmane.org; will.deacon-5wv7dgnIgG8@public.gmane.org;
> robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
> >> Bharat Bhushan <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>; Nipun Gupta
> >> <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana Madalina Craciun
> >> <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> >> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> >> Subject: Re: RFC: extend iommu-map binding to support #iommu-cells >
> >> 1
> >>
> >> On Fri, Dec 16, 2016 at 02:36:57AM +0000, Stuart Yoder wrote:
> >>> For context, please see the thread:
> >>>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
> >>> ww.spinics.net%2Flists%2Farm-
> >>
> kernel%2Fmsg539066.html&data=01%7C01%7Cstuart.yoder%40nxp.com%7C
> 0464e
> >> 12bddfd42e0f0a508d425a847cb%7C686ea
> >>
> 1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=KnzeOlbts271GwD1cpx2FLMsKn
> xw%2F
> >> OdiGVTp6%2BKFrbM%3D&reserved=0
> >>>
> >>> The existing iommu-map binding did not account for the situation
> >>> where #iommu-cells == 2, as permitted in the ARM SMMU binding. The
> >>> 2nd cell of the IOMMU specifier being the SMR mask. The existing
> >>> binding defines the mapping as:
> >>> Any RID r in the interval [rid-base, rid-base + length) is associated with
> >>> the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-
> base).
> >>>
> >>> ...and that does not work if iommu-base is 2 cells, the second being
> >>> the SMR mask.
> >>>
> >>> While this can be worked around by always having length=1, it seems
> >>> we can get this cleaned up by updating the binding definition for iommu-
> map.
> >>
> >> To reiterate, I'm very much not keen on the pci-iommu binding having
> >> knowledge of the decomposition of iommu-specifiers from other
> bindings.
> >
> > With the current definition of iommu-map we already have baked in an
> > assumption that an iommu-specifier is a value that can be incremented
> > by 1 to get to the next sequential specifier. So the binding already
> > has "knowledge" that applies in most, but not all cases.
> >
> > The generic iommu binding also defines a case where #iommu-cells=4 for
> > some IOMMUs.
> >
> > Since the ARM SMMU is a special case, perhaps the intepretation of an
> > iommu-specifier in the context of iommu-map should be moved into the
> > SMMU binding.
> >
> >> As mentioned previously, there's an intended interpretation [1] that
> >> we need to fix up the pci-iommu binding with. With that, I don't
> >> think it's even necessary to bodge iommu-cells = <1> AFAICT.
> >
> > You had said in the previous thread:
> >
> > > I had intended that the offset was added to the final cell of the
> > > iommu-specifier (i.e. that the iommu-specifier was treated as a large
> > > number).
> >
> > > You can handle this case by adding additional entries in the map table,
> > > with a single entry length.
> >
> > I understand that, and it works, but I don't see why the definition
> > has to be that the offset is added to the "final cell".
>
> Because the cells of a specifier form a single opaque big-endian value.
> Were DT little-endian it would be the "first cell". To be pedantic, both of
> those descriptions are technically wrong because they fail to account for
> overflow and carry up into the next cell (in whichever direction).
>
> > Why can't it be
> > an iommu specific definition that makes sense for a given IOMMU?
>
> Because the implementation would then become a nightmarish rabbit-
> warren of special-cases, largely defeating the point of a *generic* binding. At
> the very least it'd have to chase every phandle and determine its compatible
> just to work out what to do with any given specifier - merely thinking about
> the complexity of the error handling for the number of additional failure
> modes that introduces is enough to put me off.
Currently if iommu-cells = 2 then SMMU treats the 2 cells as stream-id + stream-id mask. While DT binding is silent on how 2 cells, which means that actually stream-id could be more than 32bit, but SMMU uses it in the way it wants to, which is not correct. If we be more explicit on defining the meaning of two cells than it will avoid confusion and implementation will match the DT binding definition.
Thanks
-Bharat
>
> Robin.
>
> >
> > Stuart
> >
^ permalink raw reply
* [PATCH 0/3] regulator: add support for GPIO power load switches
From: Bartosz Golaszewski @ 2016-12-16 15:52 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Rob Herring, Mark Rutland
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Kevin Hilman, Patrick Titiano,
Neil Armstrong, Bartosz Golaszewski
We would like to add support for GPIO-controlled power load switches
(for example: the tps229* series from Texas Instruments). We use this
chip to power-cycle devices whose power consumption is measured using
baylibre-acme probes thus we need a way to control them from user
space.
I initially submitted a series adding this support via the iio
framework, but it was decided that iio is not the right interface for
that.
This series extends the regulator core to support regulators controlled
from user space and reuses the fixed regulator driver to support gpio
power switches.
Bartosz Golaszewski (3):
regulator: add support for user space controlled regulators
doc: DT: add new compatible to fixed regulator's binding
regulator: fixed: add support for gpio power switches
.../bindings/regulator/fixed-regulator.txt | 4 ++-
drivers/regulator/core.c | 38 +++++++++++++++++++++-
drivers/regulator/fixed.c | 33 ++++++++++++++-----
include/linux/regulator/driver.h | 5 +++
4 files changed, 70 insertions(+), 10 deletions(-)
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 1/3] regulator: add support for user space controlled regulators
From: Bartosz Golaszewski @ 2016-12-16 15:52 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Rob Herring, Mark Rutland
Cc: linux-kernel, devicetree, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Kevin Hilman, Patrick Titiano, Neil Armstrong,
Bartosz Golaszewski
In-Reply-To: <1481903550-3582-1-git-send-email-bgolaszewski@baylibre.com>
Add a new flag to struct regulator_desc indicating whether a regulator
can be controlled from user space and implement a routine in regulator
core allowing to toggle the regulator state via the sysfs 'state'
attribute.
This is useful for gpio power switches.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/regulator/core.c | 38 +++++++++++++++++++++++++++++++++++++-
include/linux/regulator/driver.h | 5 +++++
2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5c1519b..f77de8f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -99,6 +99,7 @@ struct regulator_supply_alias {
};
static int _regulator_is_enabled(struct regulator_dev *rdev);
+static int _regulator_enable(struct regulator_dev *rdev);
static int _regulator_disable(struct regulator_dev *rdev);
static int _regulator_get_voltage(struct regulator_dev *rdev);
static int _regulator_get_current_limit(struct regulator_dev *rdev);
@@ -401,7 +402,42 @@ static ssize_t regulator_state_show(struct device *dev,
return ret;
}
-static DEVICE_ATTR(state, 0444, regulator_state_show, NULL);
+static ssize_t regulator_state_set(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct regulator_dev *rdev = dev_get_drvdata(dev);
+ bool enable;
+ ssize_t ret;
+
+ if (!rdev->desc->userspace_control)
+ return -EPERM;
+
+ if (sysfs_streq(buf, "enabled\n") || sysfs_streq(buf, "1"))
+ enable = true;
+ else if (sysfs_streq(buf, "disabled\n") || sysfs_streq(buf, "0"))
+ enable = false;
+ else
+ return -EINVAL;
+
+ mutex_lock(&rdev->mutex);
+
+ if ((enable && _regulator_is_enabled(rdev)) ||
+ (!enable && !_regulator_is_enabled(rdev))) {
+ mutex_unlock(&rdev->mutex);
+ return -EBUSY;
+ }
+
+ ret = enable ? _regulator_enable(rdev) : _regulator_disable(rdev);
+
+ mutex_unlock(&rdev->mutex);
+
+ if (ret)
+ return ret;
+
+ return len;
+}
+static DEVICE_ATTR(state, 0644, regulator_state_show, regulator_state_set);
static ssize_t regulator_status_show(struct device *dev,
struct device_attribute *attr, char *buf)
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 37b5324..0e7ad95 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -293,6 +293,9 @@ enum regulator_type {
* @off_on_delay: guard time (in uS), before re-enabling a regulator
*
* @of_map_mode: Maps a hardware mode defined in a DeviceTree to a standard mode
+ *
+ * @userspace_control: A flag to indicate whether this regulator can be
+ * controlled from user-space.
*/
struct regulator_desc {
const char *name;
@@ -347,6 +350,8 @@ struct regulator_desc {
unsigned int off_on_delay;
unsigned int (*of_map_mode)(unsigned int mode);
+
+ unsigned int userspace_control;
};
/**
--
2.9.3
^ permalink raw reply related
* [PATCH 2/3] doc: DT: add new compatible to fixed regulator's binding
From: Bartosz Golaszewski @ 2016-12-16 15:52 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Rob Herring, Mark Rutland
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Kevin Hilman, Patrick Titiano,
Neil Armstrong, Bartosz Golaszewski
In-Reply-To: <1481903550-3582-1-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Extend the fixed regulator's device tree bindings with a new
compatible describing GPIO-driven power load switches.
Signed-off-by: Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
Documentation/devicetree/bindings/regulator/fixed-regulator.txt | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
index 4fae41d..306931c 100644
--- a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
@@ -1,7 +1,9 @@
Fixed Voltage regulators
Required properties:
-- compatible: Must be "regulator-fixed";
+- compatible:
+ "regulator-fixed" for regular fixed-regulators
+ "gpio-power-switch" for gpio-driven power load switches
Optional properties:
- gpio: gpio to use for enable control
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 3/3] regulator: fixed: add support for gpio power switches
From: Bartosz Golaszewski @ 2016-12-16 15:52 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Rob Herring, Mark Rutland
Cc: linux-kernel, devicetree, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Kevin Hilman, Patrick Titiano, Neil Armstrong,
Bartosz Golaszewski
In-Reply-To: <1481903550-3582-1-git-send-email-bgolaszewski@baylibre.com>
The difference between a regular fixed regulator and a GPIO power load
switch is the fact that the latter may be controlled from user space.
Reuse the fixed regulator driver to support power switches.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/regulator/fixed.c | 33 +++++++++++++++++++++++++--------
1 file changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 988a747..f125a3e 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -31,6 +31,8 @@
#include <linux/regulator/of_regulator.h>
#include <linux/regulator/machine.h>
+#define REG_FIXED_POWER_SWITCH BIT(0)
+
struct fixed_voltage_data {
struct regulator_desc desc;
struct regulator_dev *dev;
@@ -97,11 +99,27 @@ of_get_fixed_voltage_config(struct device *dev,
static struct regulator_ops fixed_voltage_ops = {
};
+#if defined(CONFIG_OF)
+static const struct of_device_id fixed_of_match[] = {
+ {
+ .compatible = "regulator-fixed",
+ },
+ {
+ .compatible = "gpio-power-switch",
+ .data = (void *)REG_FIXED_POWER_SWITCH,
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, fixed_of_match);
+#endif
+
static int reg_fixed_voltage_probe(struct platform_device *pdev)
{
struct fixed_voltage_config *config;
struct fixed_voltage_data *drvdata;
struct regulator_config cfg = { };
+ const struct of_device_id *of_id;
+ long flags = 0;
int ret;
drvdata = devm_kzalloc(&pdev->dev, sizeof(struct fixed_voltage_data),
@@ -175,6 +193,13 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
cfg.driver_data = drvdata;
cfg.of_node = pdev->dev.of_node;
+ of_id = of_match_node(fixed_of_match, pdev->dev.of_node);
+ if (of_id)
+ flags = (long)of_id->data;
+
+ if (flags & REG_FIXED_POWER_SWITCH)
+ drvdata->desc.userspace_control = 1;
+
drvdata->dev = devm_regulator_register(&pdev->dev, &drvdata->desc,
&cfg);
if (IS_ERR(drvdata->dev)) {
@@ -191,14 +216,6 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
return 0;
}
-#if defined(CONFIG_OF)
-static const struct of_device_id fixed_of_match[] = {
- { .compatible = "regulator-fixed", },
- {},
-};
-MODULE_DEVICE_TABLE(of, fixed_of_match);
-#endif
-
static struct platform_driver regulator_fixed_voltage_driver = {
.probe = reg_fixed_voltage_probe,
.driver = {
--
2.9.3
^ permalink raw reply related
* RE: RFC: extend iommu-map binding to support #iommu-cells > 1
From: Stuart Yoder @ 2016-12-16 15:56 UTC (permalink / raw)
To: Robin Murphy, Mark Rutland
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
will.deacon-5wv7dgnIgG8@public.gmane.org,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
In-Reply-To: <07c70fde-2bce-a517-0f5f-a2405049c87c-5wv7dgnIgG8@public.gmane.org>
> >>> The existing iommu-map binding did not account for the situation where
> >>> #iommu-cells == 2, as permitted in the ARM SMMU binding. The 2nd cell
> >>> of the IOMMU specifier being the SMR mask. The existing binding defines
> >>> the mapping as:
> >>> Any RID r in the interval [rid-base, rid-base + length) is associated with
> >>> the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
> >>>
> >>> ...and that does not work if iommu-base is 2 cells, the second being the
> >>> SMR mask.
> >>>
> >>> While this can be worked around by always having length=1, it seems we
> >>> can get this cleaned up by updating the binding definition for iommu-map.
> >>
> >> To reiterate, I'm very much not keen on the pci-iommu binding having
> >> knowledge of the decomposition of iommu-specifiers from other bindings.
> >
> > With the current definition of iommu-map we already have baked in an
> > assumption that an iommu-specifier is a value that can be incremented
> > by 1 to get to the next sequential specifier. So the binding already
> > has "knowledge" that applies in most, but not all cases.
> >
> > The generic iommu binding also defines a case where #iommu-cells=4
> > for some IOMMUs.
> >
> > Since the ARM SMMU is a special case, perhaps the intepretation
> > of an iommu-specifier in the context of iommu-map should be moved
> > into the SMMU binding.
> >
> >> As mentioned previously, there's an intended interpretation [1] that we
> >> need to fix up the pci-iommu binding with. With that, I don't think it's
> >> even necessary to bodge iommu-cells = <1> AFAICT.
> >
> > You had said in the previous thread:
> >
> > > I had intended that the offset was added to the final cell of the
> > > iommu-specifier (i.e. that the iommu-specifier was treated as a large
> > > number).
> >
> > > You can handle this case by adding additional entries in the map table,
> > > with a single entry length.
> >
> > I understand that, and it works, but I don't see why the definition has
> > to be that the offset is added to the "final cell".
>
> Because the cells of a specifier form a single opaque big-endian value.
> Were DT little-endian it would be the "first cell". To be pedantic, both
> of those descriptions are technically wrong because they fail to account
> for overflow and carry up into the next cell (in whichever direction).
If it is really opaque how can we reliably add 1 to it?
> > Why can't it be
> > an iommu specific definition that makes sense for a given IOMMU?
>
> Because the implementation would then become a nightmarish rabbit-warren
> of special-cases, largely defeating the point of a *generic* binding. At
> the very least it'd have to chase every phandle and determine its
> compatible just to work out what to do with any given specifier - merely
> thinking about the complexity of the error handling for the number of
> additional failure modes that introduces is enough to put me off.
In order to decode an iommu-map at all we have to chase every phandle, no?
Isn't that how we know how many cells an iommu-map entry has?
I have not thought through the implementation, but my thought was that
the code could use a callback to handle the IOMMU-specific case. If
callback==NULL then the default case is what we have today. An IOMMU like
the SMMU can implement a simple callback that can return the mapping.
Not sure how this is that much harder than any other map, such as interrupt-map.
Stuart
^ permalink raw reply
* Re: [PATCH] ARM: dts: Add missing CPU frequencies for Exynos5422/5800
From: Javier Martinez Canillas @ 2016-12-16 16:22 UTC (permalink / raw)
To: Markus Reichl, Krzysztof Kozlowski, Doug Anderson
Cc: Bartlomiej Zolnierkiewicz, Arjun K V, Kukjin Kim, Rob Herring,
Mark Rutland, Russell King, Andreas Faerber, Thomas Abraham,
Ben Gamari, linux-samsung-soc,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Alim
In-Reply-To: <1b6e8d3a-ec7a-db5d-dd0e-ef9d1480f80a-SRyzfwRm/0rPTwkrwQOX7A@public.gmane.org>
Hello Markus,
On 12/16/2016 06:08 AM, Markus Reichl wrote:
> Am 16.12.2016 um 08:37 schrieb Krzysztof Kozlowski:
>> On Thu, Dec 15, 2016 at 04:52:58PM -0800, Doug Anderson wrote:
>>>> [ I added Arjun to Cc:, maybe he can help in explaining this issue
>>>> (unfortunately Inderpal's email is no longer working). ]
>>>>
>>>> Please also note that on Exynos5422/5800 SoCs the same ARM rail
>>>> voltage is used for 1.9 GHz & 2.0 GHz OPPs as for the 1.8 GHz one.
>>>> IOW if the problem exists it is already present in the mainline
>>>> kernel.
>>>
>>> Interesting. In the ChromeOS tree I see significantly higher voltages
>>> needed... Note that one might naively look at
>>> <https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/cpufreq/exynos5420-cpufreq.c#178>.
>>>
>>> 1362500, /* L0 2100 */
>>> 1312500, /* L1 2000 */
>>>
>>> ..but, amazingly enough those voltages aren't used at all. Surprise!
>>>
>>> I believe that the above numbers are actually not used and the ASV
>>> numbers are used instead. See
>>> <https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/arch/arm/mach-exynos/include/mach/asv-exynos542x.h#452>
>>>
>>> { 2100000,
>>> 1350000, 1350000, 1350000, 1350000, 1350000,
>>> 1337500, 1325000, 1312500, 1300000, 1287500,
>>> 1275000, 1262500, 1250000, 1237500 },
>>>
>>> I believe that interpretation there is: some bins of the CPU can run
>>> at 2.1 GHz just fine at 1.25 V but others need up to 1.35V.
>>
>> That is definitely the case. One could just look at vendors ASV table
>> (for 1.9 GHz):
>> { 1900000, 1300000, 1287500, 1262500, 1237500, 1225000, 1212500,
>> 1200000, 1187500, 1175000, 1162500, 1150000,
>> 1137500, 1125000, 1112500, 1112500},
>>
>> The theoretical difference is up to 1.875V! From my experiments I saw
>> BIN1 chips which should be the same... but some working on 1.2V, some on
>> 1.225V (@1.9 GHz). I didn't see any requiring higher voltages but that
>> does not mean that there aren't such...
>>
>>> ...so if you're running at 2.1 GHz at 1.25V then perhaps you're just
>>> running on a CPU from a nice bin?
>
> I've been running the proposed frequency/voltage combinations without any
> stability problems on my XU4, XU3 and even XU3-lite ( I did not delete the
> nodes on XU3-lite dts) with make -j8 kernel and ssvb-cpuburn.
> The chips are poorly cooled, especially the XU4 and quickly step down.
>
>>
>> Would be nice to see a dump of PKG_ID and AUX_INFO chipid registers
>> along with name of tested board. Because the "Tested on XU3" is not
>> sufficient.
>
> If you point me to how to read these values out, I will publish them.
>
You can use the exynos-chipid driver posted by Pankaj. Apply patches 1 and
2 from this series (http://www.spinics.net/lists/arm-kernel/msg548384.html)
and then this diff to get the values of the registers that Krzysztof asked:
diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
index cf0128b18ee2..49fa76ec6d49 100644
--- a/drivers/soc/samsung/exynos-chipid.c
+++ b/drivers/soc/samsung/exynos-chipid.c
@@ -22,6 +22,9 @@
#define EXYNOS_MAINREV_MASK (0xF << 0)
#define EXYNOS_REV_MASK (EXYNOS_SUBREV_MASK | EXYNOS_MAINREV_MASK)
+#define EXYNOS_PKG_ID 0x04
+#define EXYNOS_AUX_INFO 0x1C
+
static const struct exynos_soc_id {
const char *name;
unsigned int id;
@@ -71,6 +74,8 @@ int __init exynos_chipid_early_init(void)
const struct of_device_id *match;
u32 product_id;
u32 revision;
+ u32 pkg_id;
+ u32 aux_info;
np = of_find_matching_node_and_match(NULL,
of_exynos_chipid_ids, &match);
@@ -84,6 +89,8 @@ int __init exynos_chipid_early_init(void)
product_id = readl_relaxed(exynos_chipid_base);
revision = product_id & EXYNOS_REV_MASK;
+ pkg_id = readl_relaxed(exynos_chipid_base + EXYNOS_PKG_ID);
+ aux_info = readl_relaxed(exynos_chipid_base + EXYNOS_AUX_INFO);
iounmap(exynos_chipid_base);
soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
@@ -100,8 +107,8 @@ int __init exynos_chipid_early_init(void)
soc_dev_attr->soc_id = product_id_to_soc_id(product_id);
- pr_info("Exynos: CPU[%s] CPU_REV[0x%x] Detected\n",
- product_id_to_soc_id(product_id), revision);
+ pr_info("Exynos: CPU[%s] CPU_REV[0x%x] PKG_ID[0x%x] AUX_INFO[0x%x] \n",
+ product_id_to_soc_id(product_id), revision, pkg_id, aux_info);
soc_dev = soc_device_register(soc_dev_attr);
if (IS_ERR(soc_dev)) {
Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: RFC: extend iommu-map binding to support #iommu-cells > 1
From: Robin Murphy @ 2016-12-16 16:49 UTC (permalink / raw)
To: Stuart Yoder, Mark Rutland
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
will.deacon-5wv7dgnIgG8@public.gmane.org,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
In-Reply-To: <VI1PR0401MB2638870C58173D5906AE0D9E8D9C0-9IDQY6o3qQjcXZ0H4ZLnAo3W/0Ik+aLCnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
On 16/12/16 15:56, Stuart Yoder wrote:
>>>>> The existing iommu-map binding did not account for the situation where
>>>>> #iommu-cells == 2, as permitted in the ARM SMMU binding. The 2nd cell
>>>>> of the IOMMU specifier being the SMR mask. The existing binding defines
>>>>> the mapping as:
>>>>> Any RID r in the interval [rid-base, rid-base + length) is associated with
>>>>> the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
>>>>>
>>>>> ...and that does not work if iommu-base is 2 cells, the second being the
>>>>> SMR mask.
>>>>>
>>>>> While this can be worked around by always having length=1, it seems we
>>>>> can get this cleaned up by updating the binding definition for iommu-map.
>>>>
>>>> To reiterate, I'm very much not keen on the pci-iommu binding having
>>>> knowledge of the decomposition of iommu-specifiers from other bindings.
>>>
>>> With the current definition of iommu-map we already have baked in an
>>> assumption that an iommu-specifier is a value that can be incremented
>>> by 1 to get to the next sequential specifier. So the binding already
>>> has "knowledge" that applies in most, but not all cases.
>>>
>>> The generic iommu binding also defines a case where #iommu-cells=4
>>> for some IOMMUs.
>>>
>>> Since the ARM SMMU is a special case, perhaps the intepretation
>>> of an iommu-specifier in the context of iommu-map should be moved
>>> into the SMMU binding.
>>>
>>>> As mentioned previously, there's an intended interpretation [1] that we
>>>> need to fix up the pci-iommu binding with. With that, I don't think it's
>>>> even necessary to bodge iommu-cells = <1> AFAICT.
>>>
>>> You had said in the previous thread:
>>>
>>> > I had intended that the offset was added to the final cell of the
>>> > iommu-specifier (i.e. that the iommu-specifier was treated as a large
>>> > number).
>>>
>>> > You can handle this case by adding additional entries in the map table,
>>> > with a single entry length.
>>>
>>> I understand that, and it works, but I don't see why the definition has
>>> to be that the offset is added to the "final cell".
>>
>> Because the cells of a specifier form a single opaque big-endian value.
>> Were DT little-endian it would be the "first cell". To be pedantic, both
>> of those descriptions are technically wrong because they fail to account
>> for overflow and carry up into the next cell (in whichever direction).
>
> If it is really opaque how can we reliably add 1 to it?
The pci-iommu binding isn't adding 1 to an opaque value. It is
*generating* an IOMMU specifier of the form of a single numeric value,
as defined by some linear translation of a PCI RID relative to a numeric
base value appropriate for the IOMMU topology. It is explicit therein
that a single numeric value must be the appropriate interpretation of
that specifier. That happens to be true of a 1-cell arm-smmu specifier,
therefore iommu-map can be used with arm-smmu with #iommu-cells=1. It is
also true of a 2-cell some-other-iommu specifier, therefore iommu-map
can be used with some-other-iommu with #iommu-cells=2 (if we fix the
fact that the current implementation fails to consider more than one
cell). It is not, however, true of a 2-cell arm-smmu specifier,
therefore iommu-map cannot be used with arm-smmu with #iommu-cells=2,
although the fact that of_pci_iommu_configure() unconditionally
generates a 1-cell specifier at the moment does happen to sidestep that.
The point of the 2-cell arm-smmu specifier is really to handle the
devices (which exist) with dozens or hundreds of stream IDs, which are
*only* usable via SMR masking, and particularly with a hand-crafted mask
that is able to assume the non-existence of overlapping IDs (that aspect
being actually quite significant for optimal allocation - one of the
reasons my automatic-mask-generation prototype is now gathering dust).
The MMU-500 TBU use-case is really an entirely different kettle of fish,
hence the RFC I posted earlier.
Robin.
>>> Why can't it be
>>> an iommu specific definition that makes sense for a given IOMMU?
>>
>> Because the implementation would then become a nightmarish rabbit-warren
>> of special-cases, largely defeating the point of a *generic* binding. At
>> the very least it'd have to chase every phandle and determine its
>> compatible just to work out what to do with any given specifier - merely
>> thinking about the complexity of the error handling for the number of
>> additional failure modes that introduces is enough to put me off.
>
> In order to decode an iommu-map at all we have to chase every phandle, no?
> Isn't that how we know how many cells an iommu-map entry has?
>
> I have not thought through the implementation, but my thought was that
> the code could use a callback to handle the IOMMU-specific case. If
> callback==NULL then the default case is what we have today. An IOMMU like
> the SMMU can implement a simple callback that can return the mapping.
>
> Not sure how this is that much harder than any other map, such as interrupt-map.
>
> Stuart
>
^ permalink raw reply
* RE: RFC: extend iommu-map binding to support #iommu-cells > 1
From: Stuart Yoder @ 2016-12-16 17:05 UTC (permalink / raw)
To: Robin Murphy, Mark Rutland
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
will.deacon-5wv7dgnIgG8@public.gmane.org,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
In-Reply-To: <41edab27-c73a-7a1a-4369-fb43d7f57f78-5wv7dgnIgG8@public.gmane.org>
> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy-5wv7dgnIgG8@public.gmane.org]
> Sent: Friday, December 16, 2016 10:50 AM
> To: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>; Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: will.deacon-5wv7dgnIgG8@public.gmane.org; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Bharat Bhushan <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>; Nipun Gupta
> <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana Madalina Craciun <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Subject: Re: RFC: extend iommu-map binding to support #iommu-cells > 1
>
> On 16/12/16 15:56, Stuart Yoder wrote:
> >>>>> The existing iommu-map binding did not account for the situation where
> >>>>> #iommu-cells == 2, as permitted in the ARM SMMU binding. The 2nd cell
> >>>>> of the IOMMU specifier being the SMR mask. The existing binding defines
> >>>>> the mapping as:
> >>>>> Any RID r in the interval [rid-base, rid-base + length) is associated with
> >>>>> the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
> >>>>>
> >>>>> ...and that does not work if iommu-base is 2 cells, the second being the
> >>>>> SMR mask.
> >>>>>
> >>>>> While this can be worked around by always having length=1, it seems we
> >>>>> can get this cleaned up by updating the binding definition for iommu-map.
> >>>>
> >>>> To reiterate, I'm very much not keen on the pci-iommu binding having
> >>>> knowledge of the decomposition of iommu-specifiers from other bindings.
> >>>
> >>> With the current definition of iommu-map we already have baked in an
> >>> assumption that an iommu-specifier is a value that can be incremented
> >>> by 1 to get to the next sequential specifier. So the binding already
> >>> has "knowledge" that applies in most, but not all cases.
> >>>
> >>> The generic iommu binding also defines a case where #iommu-cells=4
> >>> for some IOMMUs.
> >>>
> >>> Since the ARM SMMU is a special case, perhaps the intepretation
> >>> of an iommu-specifier in the context of iommu-map should be moved
> >>> into the SMMU binding.
> >>>
> >>>> As mentioned previously, there's an intended interpretation [1] that we
> >>>> need to fix up the pci-iommu binding with. With that, I don't think it's
> >>>> even necessary to bodge iommu-cells = <1> AFAICT.
> >>>
> >>> You had said in the previous thread:
> >>>
> >>> > I had intended that the offset was added to the final cell of the
> >>> > iommu-specifier (i.e. that the iommu-specifier was treated as a large
> >>> > number).
> >>>
> >>> > You can handle this case by adding additional entries in the map table,
> >>> > with a single entry length.
> >>>
> >>> I understand that, and it works, but I don't see why the definition has
> >>> to be that the offset is added to the "final cell".
> >>
> >> Because the cells of a specifier form a single opaque big-endian value.
> >> Were DT little-endian it would be the "first cell". To be pedantic, both
> >> of those descriptions are technically wrong because they fail to account
> >> for overflow and carry up into the next cell (in whichever direction).
> >
> > If it is really opaque how can we reliably add 1 to it?
>
> The pci-iommu binding isn't adding 1 to an opaque value. It is
> *generating* an IOMMU specifier of the form of a single numeric value,
> as defined by some linear translation of a PCI RID relative to a numeric
> base value appropriate for the IOMMU topology. It is explicit therein
> that a single numeric value must be the appropriate interpretation of
> that specifier. That happens to be true of a 1-cell arm-smmu specifier,
> therefore iommu-map can be used with arm-smmu with #iommu-cells=1. It is
> also true of a 2-cell some-other-iommu specifier, therefore iommu-map
> can be used with some-other-iommu with #iommu-cells=2 (if we fix the
> fact that the current implementation fails to consider more than one
> cell). It is not, however, true of a 2-cell arm-smmu specifier,
> therefore iommu-map cannot be used with arm-smmu with #iommu-cells=2,
> although the fact that of_pci_iommu_configure() unconditionally
> generates a 1-cell specifier at the moment does happen to sidestep that.
>
> The point of the 2-cell arm-smmu specifier is really to handle the
> devices (which exist) with dozens or hundreds of stream IDs, which are
> *only* usable via SMR masking, and particularly with a hand-crafted mask
> that is able to assume the non-existence of overlapping IDs (that aspect
> being actually quite significant for optimal allocation - one of the
> reasons my automatic-mask-generation prototype is now gathering dust).
>
> The MMU-500 TBU use-case is really an entirely different kettle of fish,
> hence the RFC I posted earlier.
I had not seen your RFC when I replied previously...the global SMR mask
is actually much preferred and more straightforward. We will test it.
Thanks,
Stuart
^ permalink raw reply
* Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399
From: Doug Anderson @ 2016-12-16 17:28 UTC (permalink / raw)
To: Xing Zheng
Cc: Heiko Stuebner, Frank Wang, Brian Norris, William wu, Rob Herring,
Mark Rutland, Catalin Marinas, Will Deacon, Caesar Wang,
Jianqun Xu, Elaine Zhang,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Dmitry Torokhov, Tao Huang, open list:ARM/Rockchip SoC...
In-Reply-To: <5853903D.8030605-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Hi,
On Thu, Dec 15, 2016 at 10:57 PM, Xing Zheng <zhengxing-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> Hi Heiko, Doug,
>
> On 2016年12月16日 02:18, Heiko Stuebner wrote:
>
> Am Donnerstag, 15. Dezember 2016, 08:34:09 CET schrieb Doug Anderson:
>
>
> I still need to digest all of the things that were added to this
> thread overnight, but nothing I've seen so far indicates that you need
> the post-gated clock. AKA I still think you need to redo your patch
> to replace:
>
> clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
> <&cru SCLK_USBPHY0_480M_SRC>;
>
> with:
>
> clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
> <&u2phy0>;
>
> Can you please comment on that?
>
> Also, with the change, the ehci will keep the clock (and thus the phy)
> always
> on. Does the phy-autosuspend even save anything now?
>
> In any case, could we make the clock-names entry sound nicer than
> usbphy0_480m
> please? bindings/usb/atmel-usb.txt calls its UTMI clock simply "usb_clk",
> but
> something like "utmi" should also work.
> While at it you could also fix up the other clock names to something like
> "host" and "arbiter" or so?.
>
>
> Heiko
>
>
> The usbphy related clock tress like this:
>
>
> Actually, at drivers/phy/phy-rockchip-inno-usb2.c, we can only
> enable/disable the master gate via GRF is PHY_PLL, not UTMI_CLK.
>
> And the naming style of the "hclk_host0" keep the name "hclk_host0" on the
> clcok tree diagram:
>
>
> Therefore, could we rename the clock name like this:
> ----
> for usb_host0_ehci and usb_host0_ohci:
> clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
> <&cru SCLK_U2PHY0>;
> clock-names = "hclk_host0", "hclk_host0_arb",
> "sclk_u2phy0";
>
> for usb_host1_ehci and usb_host1_ohci:
> clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>,
> <&cru SCLK_U2PHY1>;
> clock-names = "hclk_host1", "hclk_host1_arb",
> "sclk_u2phy1";
> ----
>
> BTW, the "arb" is an abbreviation for arbiter.
You don't specify what this new "SCLK_U2PHY0" ID is, so it's a little
hard for me to know what you're intending.
...however, I still don't see any reason why you can't just use the
solution I proposed. Specifying the clock as "<&u2phy0>" is the
correct thing to do. The input clock to the EHCI driver is exactly
the clock provided by the USB PHY with no gate in between (just as I
said). There is no reason to somehow buffer it by the cru. The cru
doesn't see this clock and has no reason to be involved.
> Thanks.
Note that there were many other comments on this thread besides mine.
Are you planning to address any of them?
-Doug
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/3] regulator: add support for user space controlled regulators
From: Mark Brown @ 2016-12-16 18:10 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Liam Girdwood, Rob Herring, Mark Rutland,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Kevin Hilman, Patrick Titiano,
Neil Armstrong
In-Reply-To: <1481903550-3582-2-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 602 bytes --]
On Fri, Dec 16, 2016 at 04:52:28PM +0100, Bartosz Golaszewski wrote:
> Add a new flag to struct regulator_desc indicating whether a regulator
> can be controlled from user space and implement a routine in regulator
> core allowing to toggle the regulator state via the sysfs 'state'
> attribute.
No, we've been through this repeatedly before - search the archives.
Write a driver for your hardware which exposes a control for bouncing
the power if that's something that makes sense in your application.
Doing this at the regulator level is just far too likely to result in
poorly integrated systems.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] regulator: fixed: add support for gpio power switches
From: Mark Brown @ 2016-12-16 18:12 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Liam Girdwood, Rob Herring, Mark Rutland,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Kevin Hilman, Patrick Titiano,
Neil Armstrong
In-Reply-To: <1481903550-3582-4-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 594 bytes --]
On Fri, Dec 16, 2016 at 04:52:30PM +0100, Bartosz Golaszewski wrote:
> The difference between a regular fixed regulator and a GPIO power load
> switch is the fact that the latter may be controlled from user space.
This is not something I would understand or expect from the naming here.
I would expect the difference between a regulator and a switch to be
that the switch does no regulation, such things are fairly common in
system design for example as part of the logic that switches between the
different input power supplies available to the system to provide the
root system power rail.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCHv2 3/8] rtc: add STM32 RTC driver
From: Mathieu Poirier @ 2016-12-16 19:08 UTC (permalink / raw)
To: Amelie Delaunay
Cc: Alessandro Zummo, Alexandre Belloni, Rob Herring, Mark Rutland,
Maxime Coquelin, Alexandre Torgue, Russell King,
rtc-linux-/JYPxA39Uh5TLH3MbocFFw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Gabriel Fernandez
In-Reply-To: <1481878257-29163-4-git-send-email-amelie.delaunay-qxv4g6HH51o@public.gmane.org>
On Fri, Dec 16, 2016 at 09:50:52AM +0100, Amelie Delaunay wrote:
> This patch adds support for the STM32 RTC.
>
> Signed-off-by: Amelie Delaunay <amelie.delaunay-qxv4g6HH51o@public.gmane.org>
> ---
> drivers/rtc/Kconfig | 11 +
> drivers/rtc/Makefile | 1 +
> drivers/rtc/rtc-stm32.c | 776 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 788 insertions(+)
> create mode 100644 drivers/rtc/rtc-stm32.c
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e859d14..11eb28a 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1706,6 +1706,17 @@ config RTC_DRV_PIC32
> This driver can also be built as a module. If so, the module
> will be called rtc-pic32
>
> +config RTC_DRV_STM32
> + tristate "STM32 RTC"
> + select REGMAP_MMIO
> + depends on ARCH_STM32 || COMPILE_TEST
> + help
> + If you say yes here you get support for the STM32 On-Chip
> + Real Time Clock.
> +
> + This driver can also be built as a module, if so, the module
> + will be called "rtc-stm32".
> +
> comment "HID Sensor RTC drivers"
>
> config RTC_DRV_HID_SENSOR_TIME
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 1ac694a..87bd9cc 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -144,6 +144,7 @@ obj-$(CONFIG_RTC_DRV_SNVS) += rtc-snvs.o
> obj-$(CONFIG_RTC_DRV_SPEAR) += rtc-spear.o
> obj-$(CONFIG_RTC_DRV_STARFIRE) += rtc-starfire.o
> obj-$(CONFIG_RTC_DRV_STK17TA8) += rtc-stk17ta8.o
> +obj-$(CONFIG_RTC_DRV_STM32) += rtc-stm32.o
> obj-$(CONFIG_RTC_DRV_STMP) += rtc-stmp3xxx.o
> obj-$(CONFIG_RTC_DRV_ST_LPC) += rtc-st-lpc.o
> obj-$(CONFIG_RTC_DRV_SUN4V) += rtc-sun4v.o
> diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
> new file mode 100644
> index 0000000..6ce0f5a
> --- /dev/null
> +++ b/drivers/rtc/rtc-stm32.c
> @@ -0,0 +1,776 @@
> +/*
> + * Copyright (C) Amelie Delaunay 2016
> + * Author: Amelie Delaunay <amelie.delaunay-qxv4g6HH51o@public.gmane.org>
> + * License terms: GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/bcd.h>
> +#include <linux/clk.h>
> +#include <linux/iopoll.h>
> +#include <linux/ioport.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/rtc.h>
> +#include <linux/spinlock.h>
> +
> +#define DRIVER_NAME "stm32_rtc"
> +
> +/* STM32 RTC registers */
> +#define STM32_RTC_TR 0x00
> +#define STM32_RTC_DR 0x04
> +#define STM32_RTC_CR 0x08
> +#define STM32_RTC_ISR 0x0C
> +#define STM32_RTC_PRER 0x10
> +#define STM32_RTC_ALRMAR 0x1C
> +#define STM32_RTC_WPR 0x24
> +
> +/* STM32_RTC_TR bit fields */
> +#define STM32_RTC_TR_SEC_SHIFT 0
> +#define STM32_RTC_TR_SEC GENMASK(6, 0)
> +#define STM32_RTC_TR_MIN_SHIFT 8
> +#define STM32_RTC_TR_MIN GENMASK(14, 8)
> +#define STM32_RTC_TR_HOUR_SHIFT 16
> +#define STM32_RTC_TR_HOUR GENMASK(21, 16)
> +
> +/* STM32_RTC_DR bit fields */
> +#define STM32_RTC_DR_DATE_SHIFT 0
> +#define STM32_RTC_DR_DATE GENMASK(5, 0)
> +#define STM32_RTC_DR_MONTH_SHIFT 8
> +#define STM32_RTC_DR_MONTH GENMASK(12, 8)
> +#define STM32_RTC_DR_WDAY_SHIFT 13
> +#define STM32_RTC_DR_WDAY GENMASK(15, 13)
> +#define STM32_RTC_DR_YEAR_SHIFT 16
> +#define STM32_RTC_DR_YEAR GENMASK(23, 16)
> +
> +/* STM32_RTC_CR bit fields */
> +#define STM32_RTC_CR_FMT BIT(6)
> +#define STM32_RTC_CR_ALRAE BIT(8)
> +#define STM32_RTC_CR_ALRAIE BIT(12)
> +
> +/* STM32_RTC_ISR bit fields */
> +#define STM32_RTC_ISR_ALRAWF BIT(0)
> +#define STM32_RTC_ISR_INITS BIT(4)
> +#define STM32_RTC_ISR_RSF BIT(5)
> +#define STM32_RTC_ISR_INITF BIT(6)
> +#define STM32_RTC_ISR_INIT BIT(7)
> +#define STM32_RTC_ISR_ALRAF BIT(8)
> +
> +/* STM32_RTC_PRER bit fields */
> +#define STM32_RTC_PRER_PRED_S_SHIFT 0
> +#define STM32_RTC_PRER_PRED_S GENMASK(14, 0)
> +#define STM32_RTC_PRER_PRED_A_SHIFT 16
> +#define STM32_RTC_PRER_PRED_A GENMASK(22, 16)
> +
> +/* STM32_RTC_ALRMAR and STM32_RTC_ALRMBR bit fields */
> +#define STM32_RTC_ALRMXR_SEC_SHIFT 0
> +#define STM32_RTC_ALRMXR_SEC GENMASK(6, 0)
> +#define STM32_RTC_ALRMXR_SEC_MASK BIT(7)
> +#define STM32_RTC_ALRMXR_MIN_SHIFT 8
> +#define STM32_RTC_ALRMXR_MIN GENMASK(14, 8)
> +#define STM32_RTC_ALRMXR_MIN_MASK BIT(15)
> +#define STM32_RTC_ALRMXR_HOUR_SHIFT 16
> +#define STM32_RTC_ALRMXR_HOUR GENMASK(21, 16)
> +#define STM32_RTC_ALRMXR_PM BIT(22)
> +#define STM32_RTC_ALRMXR_HOUR_MASK BIT(23)
> +#define STM32_RTC_ALRMXR_DATE_SHIFT 24
> +#define STM32_RTC_ALRMXR_DATE GENMASK(29, 24)
> +#define STM32_RTC_ALRMXR_WDSEL BIT(30)
> +#define STM32_RTC_ALRMXR_WDAY_SHIFT 24
> +#define STM32_RTC_ALRMXR_WDAY GENMASK(27, 24)
> +#define STM32_RTC_ALRMXR_DATE_MASK BIT(31)
> +
> +/* STM32_RTC_WPR key constants */
> +#define RTC_WPR_1ST_KEY 0xCA
> +#define RTC_WPR_2ND_KEY 0x53
> +#define RTC_WPR_WRONG_KEY 0xFF
> +
> +/*
> + * RTC registers are protected agains parasitic write access.
> + * PWR_CR_DBP bit must be set to enable write access to RTC registers.
> + */
> +/* STM32_PWR_CR */
> +#define PWR_CR 0x00
> +/* STM32_PWR_CR bit field */
> +#define PWR_CR_DBP BIT(8)
> +
> +static struct regmap *dbp;
> +
> +struct stm32_rtc {
> + struct rtc_device *rtc_dev;
> + void __iomem *base;
> + struct clk *ck_rtc;
> + spinlock_t lock; /* Protects registers accesses */
> + int irq_alarm;
> +};
> +
> +static void stm32_rtc_wpr_unlock(struct stm32_rtc *rtc)
> +{
> + writel_relaxed(RTC_WPR_1ST_KEY, rtc->base + STM32_RTC_WPR);
> + writel_relaxed(RTC_WPR_2ND_KEY, rtc->base + STM32_RTC_WPR);
> +}
> +
> +static void stm32_rtc_wpr_lock(struct stm32_rtc *rtc)
> +{
> + writel_relaxed(RTC_WPR_WRONG_KEY, rtc->base + STM32_RTC_WPR);
> +}
> +
> +static int stm32_rtc_enter_init_mode(struct stm32_rtc *rtc)
> +{
> + unsigned int isr = readl_relaxed(rtc->base + STM32_RTC_ISR);
> +
> + if (!(isr & STM32_RTC_ISR_INITF)) {
> + isr |= STM32_RTC_ISR_INIT;
> + writel_relaxed(isr, rtc->base + STM32_RTC_ISR);
> +
> + /*
> + * It takes around 2 ck_rtc clock cycles to enter in
> + * initialization phase mode (and have INITF flag set). As
> + * slowest ck_rtc frequency may be 32kHz and highest should be
> + * 1MHz, we poll every 10 us with a timeout of 100ms.
> + */
> + return readl_relaxed_poll_timeout_atomic(
> + rtc->base + STM32_RTC_ISR,
> + isr, (isr & STM32_RTC_ISR_INITF),
> + 10, 100000);
> + }
> +
> + return 0;
> +}
> +
> +static void stm32_rtc_exit_init_mode(struct stm32_rtc *rtc)
> +{
> + unsigned int isr = readl_relaxed(rtc->base + STM32_RTC_ISR);
> +
> + isr &= ~STM32_RTC_ISR_INIT;
> + writel_relaxed(isr, rtc->base + STM32_RTC_ISR);
> +}
> +
> +static int stm32_rtc_wait_sync(struct stm32_rtc *rtc)
> +{
> + unsigned int isr = readl_relaxed(rtc->base + STM32_RTC_ISR);
> +
> + isr &= ~STM32_RTC_ISR_RSF;
> + writel_relaxed(isr, rtc->base + STM32_RTC_ISR);
> +
> + /*
> + * Wait for RSF to be set to ensure the calendar registers are
> + * synchronised, it takes around 2 ck_rtc clock cycles
> + */
> + return readl_relaxed_poll_timeout_atomic(rtc->base + STM32_RTC_ISR,
> + isr,
> + (isr & STM32_RTC_ISR_RSF),
> + 10, 100000);
> +}
> +
> +static irqreturn_t stm32_rtc_alarm_irq(int irq, void *dev_id)
> +{
> + struct stm32_rtc *rtc = (struct stm32_rtc *)dev_id;
> + unsigned int isr, cr;
> +
> + mutex_lock(&rtc->rtc_dev->ops_lock);
> +
> + isr = readl_relaxed(rtc->base + STM32_RTC_ISR);
> + cr = readl_relaxed(rtc->base + STM32_RTC_CR);
> +
> + if ((isr & STM32_RTC_ISR_ALRAF) &&
> + (cr & STM32_RTC_CR_ALRAIE)) {
> + /* Alarm A flag - Alarm interrupt */
> + dev_dbg(&rtc->rtc_dev->dev, "Alarm occurred\n");
> +
> + /* Pass event to the kernel */
> + rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF);
> +
> + /* Clear event flag, otherwise new events won't be received */
> + writel_relaxed(isr & ~STM32_RTC_ISR_ALRAF,
> + rtc->base + STM32_RTC_ISR);
> + }
> +
> + mutex_unlock(&rtc->rtc_dev->ops_lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +/* Convert rtc_time structure from bin to bcd format */
> +static void tm2bcd(struct rtc_time *tm)
> +{
> + tm->tm_sec = bin2bcd(tm->tm_sec);
> + tm->tm_min = bin2bcd(tm->tm_min);
> + tm->tm_hour = bin2bcd(tm->tm_hour);
> +
> + tm->tm_mday = bin2bcd(tm->tm_mday);
> + tm->tm_mon = bin2bcd(tm->tm_mon + 1);
> + tm->tm_year = bin2bcd(tm->tm_year - 100);
> + /*
> + * Number of days since Sunday
> + * - on kernel side, 0=Sunday...6=Saturday
> + * - on rtc side, 0=invalid,1=Monday...7=Sunday
> + */
> + tm->tm_wday = (!tm->tm_wday) ? 7 : tm->tm_wday;
> +}
> +
> +/* Convert rtc_time structure from bcd to bin format */
> +static void bcd2tm(struct rtc_time *tm)
> +{
> + tm->tm_sec = bcd2bin(tm->tm_sec);
> + tm->tm_min = bcd2bin(tm->tm_min);
> + tm->tm_hour = bcd2bin(tm->tm_hour);
> +
> + tm->tm_mday = bcd2bin(tm->tm_mday);
> + tm->tm_mon = bcd2bin(tm->tm_mon) - 1;
> + tm->tm_year = bcd2bin(tm->tm_year) + 100;
> + /*
> + * Number of days since Sunday
> + * - on kernel side, 0=Sunday...6=Saturday
> + * - on rtc side, 0=invalid,1=Monday...7=Sunday
> + */
> + tm->tm_wday %= 7;
> +}
> +
> +static int stm32_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> + unsigned int tr, dr;
> + unsigned long irqflags;
> +
> + spin_lock_irqsave(&rtc->lock, irqflags);
> +
> + /* Time and Date in BCD format */
> + tr = readl_relaxed(rtc->base + STM32_RTC_TR);
> + dr = readl_relaxed(rtc->base + STM32_RTC_DR);
> +
> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> +
> + tm->tm_sec = (tr & STM32_RTC_TR_SEC) >> STM32_RTC_TR_SEC_SHIFT;
> + tm->tm_min = (tr & STM32_RTC_TR_MIN) >> STM32_RTC_TR_MIN_SHIFT;
> + tm->tm_hour = (tr & STM32_RTC_TR_HOUR) >> STM32_RTC_TR_HOUR_SHIFT;
> +
> + tm->tm_mday = (dr & STM32_RTC_DR_DATE) >> STM32_RTC_DR_DATE_SHIFT;
> + tm->tm_mon = (dr & STM32_RTC_DR_MONTH) >> STM32_RTC_DR_MONTH_SHIFT;
> + tm->tm_year = (dr & STM32_RTC_DR_YEAR) >> STM32_RTC_DR_YEAR_SHIFT;
> + tm->tm_wday = (dr & STM32_RTC_DR_WDAY) >> STM32_RTC_DR_WDAY_SHIFT;
> +
> + /* We don't report tm_yday and tm_isdst */
> +
> + bcd2tm(tm);
> +
> + if (rtc_valid_tm(tm) < 0) {
> + dev_err(dev, "%s: rtc_time is not valid.\n", __func__);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int stm32_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> + unsigned int tr, dr;
> + unsigned long irqflags;
> + int ret = 0;
> +
> + if (rtc_valid_tm(tm) < 0) {
> + dev_err(dev, "%s: rtc_time is not valid.\n", __func__);
> + return -EINVAL;
> + }
> +
> + tm2bcd(tm);
> +
> + /* Time in BCD format */
> + tr = ((tm->tm_sec << STM32_RTC_TR_SEC_SHIFT) & STM32_RTC_TR_SEC) |
> + ((tm->tm_min << STM32_RTC_TR_MIN_SHIFT) & STM32_RTC_TR_MIN) |
> + ((tm->tm_hour << STM32_RTC_TR_HOUR_SHIFT) & STM32_RTC_TR_HOUR);
> +
> + /* Date in BCD format */
> + dr = ((tm->tm_mday << STM32_RTC_DR_DATE_SHIFT) & STM32_RTC_DR_DATE) |
> + ((tm->tm_mon << STM32_RTC_DR_MONTH_SHIFT) & STM32_RTC_DR_MONTH) |
> + ((tm->tm_year << STM32_RTC_DR_YEAR_SHIFT) & STM32_RTC_DR_YEAR) |
> + ((tm->tm_wday << STM32_RTC_DR_WDAY_SHIFT) & STM32_RTC_DR_WDAY);
> +
> + spin_lock_irqsave(&rtc->lock, irqflags);
> +
> + stm32_rtc_wpr_unlock(rtc);
> +
> + ret = stm32_rtc_enter_init_mode(rtc);
> + if (ret) {
> + dev_err(dev, "Can't enter in init mode. Set time aborted.\n");
> + goto end;
> + }
> +
> + writel_relaxed(tr, rtc->base + STM32_RTC_TR);
> + writel_relaxed(dr, rtc->base + STM32_RTC_DR);
> +
> + stm32_rtc_exit_init_mode(rtc);
> +
> + ret = stm32_rtc_wait_sync(rtc);
> +end:
> + stm32_rtc_wpr_lock(rtc);
> +
> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> +
> + return ret;
> +}
> +
> +static int stm32_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> + struct rtc_time *tm = &alrm->time;
> + unsigned int alrmar, cr, isr;
> + unsigned long irqflags;
> +
> + spin_lock_irqsave(&rtc->lock, irqflags);
> +
> + alrmar = readl_relaxed(rtc->base + STM32_RTC_ALRMAR);
> + cr = readl_relaxed(rtc->base + STM32_RTC_CR);
> + isr = readl_relaxed(rtc->base + STM32_RTC_ISR);
> +
> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> +
> + if (alrmar & STM32_RTC_ALRMXR_DATE_MASK) {
> + /*
> + * Date/day doesn't matter in Alarm comparison so alarm
> + * triggers every day
> + */
> + tm->tm_mday = -1;
> + tm->tm_wday = -1;
> + } else {
> + if (alrmar & STM32_RTC_ALRMXR_WDSEL) {
> + /* Alarm is set to a day of week */
> + tm->tm_mday = -1;
> + tm->tm_wday = (alrmar & STM32_RTC_ALRMXR_WDAY) >>
> + STM32_RTC_ALRMXR_WDAY_SHIFT;
> + tm->tm_wday %= 7;
> + } else {
> + /* Alarm is set to a day of month */
> + tm->tm_wday = -1;
> + tm->tm_mday = (alrmar & STM32_RTC_ALRMXR_DATE) >>
> + STM32_RTC_ALRMXR_DATE_SHIFT;
> + }
> + }
> +
> + if (alrmar & STM32_RTC_ALRMXR_HOUR_MASK) {
> + /* Hours don't matter in Alarm comparison */
> + tm->tm_hour = -1;
> + } else {
> + tm->tm_hour = (alrmar & STM32_RTC_ALRMXR_HOUR) >>
> + STM32_RTC_ALRMXR_HOUR_SHIFT;
> + if (alrmar & STM32_RTC_ALRMXR_PM)
> + tm->tm_hour += 12;
> + }
> +
> + if (alrmar & STM32_RTC_ALRMXR_MIN_MASK) {
> + /* Minutes don't matter in Alarm comparison */
> + tm->tm_min = -1;
> + } else {
> + tm->tm_min = (alrmar & STM32_RTC_ALRMXR_MIN) >>
> + STM32_RTC_ALRMXR_MIN_SHIFT;
> + }
> +
> + if (alrmar & STM32_RTC_ALRMXR_SEC_MASK) {
> + /* Seconds don't matter in Alarm comparison */
> + tm->tm_sec = -1;
> + } else {
> + tm->tm_sec = (alrmar & STM32_RTC_ALRMXR_SEC) >>
> + STM32_RTC_ALRMXR_SEC_SHIFT;
> + }
> +
> + bcd2tm(tm);
> +
> + alrm->enabled = (cr & STM32_RTC_CR_ALRAE) ? 1 : 0;
> + alrm->pending = (isr & STM32_RTC_ISR_ALRAF) ? 1 : 0;
> +
> + return 0;
> +}
> +
> +static int stm32_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> + unsigned long irqflags;
> + unsigned int isr, cr;
> +
> + spin_lock_irqsave(&rtc->lock, irqflags);
> +
> + cr = readl_relaxed(rtc->base + STM32_RTC_CR);
> +
> + stm32_rtc_wpr_unlock(rtc);
> +
> + /* We expose Alarm A to the kernel */
> + if (enabled)
> + cr |= (STM32_RTC_CR_ALRAIE | STM32_RTC_CR_ALRAE);
> + else
> + cr &= ~(STM32_RTC_CR_ALRAIE | STM32_RTC_CR_ALRAE);
> + writel_relaxed(cr, rtc->base + STM32_RTC_CR);
> +
> + /* Clear event irqflags, otherwise new events won't be received */
> + isr = readl_relaxed(rtc->base + STM32_RTC_ISR);
> + isr &= ~STM32_RTC_ISR_ALRAF;
> + writel_relaxed(isr, rtc->base + STM32_RTC_ISR);
> +
> + stm32_rtc_wpr_lock(rtc);
> +
> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> +
> + return 0;
> +}
> +
> +static int stm32_rtc_valid_alrm(struct stm32_rtc *rtc, struct rtc_time *tm)
> +{
> + unsigned int cur_day, cur_mon, cur_year, cur_hour, cur_min, cur_sec;
> + unsigned int dr = readl_relaxed(rtc->base + STM32_RTC_DR);
> + unsigned int tr = readl_relaxed(rtc->base + STM32_RTC_TR);
> +
> + cur_day = (dr & STM32_RTC_DR_DATE) >> STM32_RTC_DR_DATE_SHIFT;
> + cur_mon = (dr & STM32_RTC_DR_MONTH) >> STM32_RTC_DR_MONTH_SHIFT;
> + cur_year = (dr & STM32_RTC_DR_YEAR) >> STM32_RTC_DR_YEAR_SHIFT;
> + cur_sec = (tr & STM32_RTC_TR_SEC) >> STM32_RTC_TR_SEC_SHIFT;
> + cur_min = (tr & STM32_RTC_TR_MIN) >> STM32_RTC_TR_MIN_SHIFT;
> + cur_hour = (tr & STM32_RTC_TR_HOUR) >> STM32_RTC_TR_HOUR_SHIFT;
> +
> + /*
> + * Assuming current date is M-D-Y H:M:S.
> + * RTC alarm can't be set on a specific month and year.
> + * So the valid alarm range is:
> + * M-D-Y H:M:S < alarm <= (M+1)-D-Y H:M:S
> + * with a specific case for December...
> + */
> + if ((((tm->tm_year > cur_year) &&
> + (tm->tm_mon == 0x1) && (cur_mon == 0x12)) ||
> + ((tm->tm_year == cur_year) &&
> + (tm->tm_mon <= cur_mon + 1))) &&
> + ((tm->tm_mday < cur_day) ||
> + ((tm->tm_mday == cur_day) &&
> + ((tm->tm_hour < cur_hour) ||
> + ((tm->tm_hour == cur_hour) && (tm->tm_min < cur_min)) ||
> + ((tm->tm_hour == cur_hour) && (tm->tm_min == cur_min) &&
> + (tm->tm_sec <= cur_sec))))))
> + return 0;
> +
> + return -EINVAL;
> +}
> +
> +static int stm32_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> + struct rtc_time *tm = &alrm->time;
> + unsigned long irqflags;
> + unsigned int cr, isr, alrmar;
> + int ret = 0;
> +
> + if (rtc_valid_tm(tm)) {
> + dev_err(dev, "Alarm time not valid.\n");
> + return -EINVAL;
> + }
> +
> + tm2bcd(tm);
> +
> + /*
> + * RTC alarm can't be set on a specific date, unless this date is
> + * up to the same day of month next month.
> + */
> + if (stm32_rtc_valid_alrm(rtc, tm) < 0) {
> + dev_err(dev, "Alarm can be set only on upcoming month.\n");
> + return -EINVAL;
> + }
> +
> + spin_lock_irqsave(&rtc->lock, irqflags);
> +
> + stm32_rtc_wpr_unlock(rtc);
> +
> + /* Disable Alarm */
> + cr = readl_relaxed(rtc->base + STM32_RTC_CR);
> + cr &= ~STM32_RTC_CR_ALRAE;
> + writel_relaxed(cr, rtc->base + STM32_RTC_CR);
> +
> + /*
> + * Poll Alarm write flag to be sure that Alarm update is allowed: it
> + * takes around 2 ck_rtc clock cycles
> + */
> + ret = readl_relaxed_poll_timeout_atomic(rtc->base + STM32_RTC_ISR,
> + isr,
> + (isr & STM32_RTC_ISR_ALRAWF),
> + 10, 100000);
> +
> + if (ret) {
> + dev_err(dev, "Alarm update not allowed\n");
> + goto end;
> + }
> +
> + alrmar = 0;
> + /* tm_year and tm_mon are not used because not supported by RTC */
> + alrmar |= (tm->tm_mday << STM32_RTC_ALRMXR_DATE_SHIFT) &
> + STM32_RTC_ALRMXR_DATE;
> + /* 24-hour format */
> + alrmar &= ~STM32_RTC_ALRMXR_PM;
> + alrmar |= (tm->tm_hour << STM32_RTC_ALRMXR_HOUR_SHIFT) &
> + STM32_RTC_ALRMXR_HOUR;
> + alrmar |= (tm->tm_min << STM32_RTC_ALRMXR_MIN_SHIFT) &
> + STM32_RTC_ALRMXR_MIN;
> + alrmar |= (tm->tm_sec << STM32_RTC_ALRMXR_SEC_SHIFT) &
> + STM32_RTC_ALRMXR_SEC;
All this work on alrmar is done while the spinlock is held. If I'm not
mistaking nothing prevents you from doing that processing before taking the
spinlock.
> +
> + /* Write to Alarm register */
> + writel_relaxed(alrmar, rtc->base + STM32_RTC_ALRMAR);
> +
> + if (alrm->enabled)
> + stm32_rtc_alarm_irq_enable(dev, 1);
> + else
> + stm32_rtc_alarm_irq_enable(dev, 0);
> +
> +end:
> + stm32_rtc_wpr_lock(rtc);
> +
> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> +
> + return ret;
> +}
> +
> +static const struct rtc_class_ops stm32_rtc_ops = {
> + .read_time = stm32_rtc_read_time,
> + .set_time = stm32_rtc_set_time,
> + .read_alarm = stm32_rtc_read_alarm,
> + .set_alarm = stm32_rtc_set_alarm,
> + .alarm_irq_enable = stm32_rtc_alarm_irq_enable,
> +};
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id stm32_rtc_of_match[] = {
> + { .compatible = "st,stm32-rtc" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, stm32_rtc_of_match);
> +#endif
> +
> +static int stm32_rtc_init(struct platform_device *pdev,
> + struct stm32_rtc *rtc)
> +{
> + unsigned int prer, pred_a, pred_s, pred_a_max, pred_s_max, cr;
> + unsigned int rate;
> + unsigned long irqflags;
> + int ret = 0;
> +
> + rate = clk_get_rate(rtc->ck_rtc);
> +
> + /* Find prediv_a and prediv_s to obtain the 1Hz calendar clock */
> + pred_a_max = STM32_RTC_PRER_PRED_A >> STM32_RTC_PRER_PRED_A_SHIFT;
> + pred_s_max = STM32_RTC_PRER_PRED_S >> STM32_RTC_PRER_PRED_S_SHIFT;
> +
> + for (pred_a = pred_a_max; pred_a >= 0; pred_a--) {
> + pred_s = (rate / (pred_a + 1)) - 1;
> +
> + if (((pred_s + 1) * (pred_a + 1)) == rate)
> + break;
> + }
> +
> + /*
> + * Can't find a 1Hz, so give priority to RTC power consumption
> + * by choosing the higher possible value for prediv_a
> + */
> + if ((pred_s > pred_s_max) || (pred_a > pred_a_max)) {
> + pred_a = pred_a_max;
> + pred_s = (rate / (pred_a + 1)) - 1;
> +
> + dev_warn(&pdev->dev, "ck_rtc is %s\n",
> + (rate - ((pred_a + 1) * (pred_s + 1)) < 0) ?
> + "fast" : "slow");
> + }
> +
> + spin_lock_irqsave(&rtc->lock, irqflags);
> +
> + stm32_rtc_wpr_unlock(rtc);
> +
> + ret = stm32_rtc_enter_init_mode(rtc);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Can't enter in init mode. Prescaler config failed.\n");
> + goto end;
> + }
> +
> + prer = (pred_s << STM32_RTC_PRER_PRED_S_SHIFT) & STM32_RTC_PRER_PRED_S;
> + writel_relaxed(prer, rtc->base + STM32_RTC_PRER);
> + prer |= (pred_a << STM32_RTC_PRER_PRED_A_SHIFT) & STM32_RTC_PRER_PRED_A;
> + writel_relaxed(prer, rtc->base + STM32_RTC_PRER);
> +
> + /* Force 24h time format */
> + cr = readl_relaxed(rtc->base + STM32_RTC_CR);
> + cr &= ~STM32_RTC_CR_FMT;
> + writel_relaxed(cr, rtc->base + STM32_RTC_CR);
> +
> + stm32_rtc_exit_init_mode(rtc);
> +
> + ret = stm32_rtc_wait_sync(rtc);
> +end:
> + stm32_rtc_wpr_lock(rtc);
> +
> + spin_unlock_irqrestore(&rtc->lock, irqflags);
> +
> + return ret;
> +}
> +
> +static int stm32_rtc_probe(struct platform_device *pdev)
> +{
> + struct stm32_rtc *rtc;
> + struct resource *res;
> + int ret;
> +
> + rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> + if (!rtc)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + rtc->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(rtc->base))
> + return PTR_ERR(rtc->base);
> +
> + dbp = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "st,syscfg");
> + if (IS_ERR(dbp)) {
> + dev_err(&pdev->dev, "no st,syscfg\n");
> + return PTR_ERR(dbp);
> + }
> +
> + spin_lock_init(&rtc->lock);
> +
> + rtc->ck_rtc = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(rtc->ck_rtc)) {
> + dev_err(&pdev->dev, "no ck_rtc clock");
> + return PTR_ERR(rtc->ck_rtc);
> + }
> +
> + ret = clk_prepare_enable(rtc->ck_rtc);
> + if (ret)
> + return ret;
> +
> + regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, PWR_CR_DBP);
> +
> + /*
> + * After a system reset, RTC_ISR.INITS flag can be read to check if
> + * the calendar has been initalized or not. INITS flag is reset by a
> + * power-on reset (no vbat, no power-supply). It is not reset if
> + * ck_rtc parent clock has changed (so RTC prescalers need to be
> + * changed). That's why we cannot rely on this flag to know if RTC
> + * init has to be done.
> + */
> + ret = stm32_rtc_init(pdev, rtc);
> + if (ret)
> + goto err;
> +
> + rtc->irq_alarm = platform_get_irq(pdev, 0);
> + if (rtc->irq_alarm <= 0) {
> + dev_err(&pdev->dev, "no alarm irq\n");
> + ret = -ENOENT;
Function platform_get_irq() returns a wealth of error codes that are lost here.
Doing 'ret = rtc->irq_alarm;' would prevent that from happening.
> + goto err;
> + }
> +
> + platform_set_drvdata(pdev, rtc);
> +
> + ret = device_init_wakeup(&pdev->dev, true);
> + if (ret)
> + dev_warn(&pdev->dev,
> + "alarm won't be able to wake up the system");
> +
> + rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, pdev->name,
> + &stm32_rtc_ops, THIS_MODULE);
> + if (IS_ERR(rtc->rtc_dev)) {
> + ret = PTR_ERR(rtc->rtc_dev);
> + dev_err(&pdev->dev, "rtc device registration failed, err=%d\n",
> + ret);
> + goto err;
> + }
> +
> + /* Handle RTC alarm interrupts */
> + ret = devm_request_threaded_irq(&pdev->dev, rtc->irq_alarm, NULL,
> + stm32_rtc_alarm_irq,
> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> + pdev->name, rtc);
> + if (ret) {
> + dev_err(&pdev->dev, "IRQ%d (alarm interrupt) already claimed\n",
> + rtc->irq_alarm);
> + goto err;
> + }
> +
> + /*
> + * If INITS flag is reset (calendar year field set to 0x00), calendar
> + * must be initialized
> + */
> + if (!(readl_relaxed(rtc->base + STM32_RTC_ISR) & STM32_RTC_ISR_INITS))
> + dev_warn(&pdev->dev, "Date/Time must be initialized\n");
> +
> + return 0;
> +err:
> + clk_disable_unprepare(rtc->ck_rtc);
> +
> + regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, ~PWR_CR_DBP);
> +
> + device_init_wakeup(&pdev->dev, false);
> +
> + return ret;
> +}
> +
> +static int __exit stm32_rtc_remove(struct platform_device *pdev)
> +{
> + struct stm32_rtc *rtc = platform_get_drvdata(pdev);
> + unsigned int cr;
> +
> + /* Disable interrupts */
> + stm32_rtc_wpr_unlock(rtc);
> + cr = readl_relaxed(rtc->base + STM32_RTC_CR);
> + cr &= ~STM32_RTC_CR_ALRAIE;
> + writel_relaxed(cr, rtc->base + STM32_RTC_CR);
> + stm32_rtc_wpr_lock(rtc);
> +
> + clk_disable_unprepare(rtc->ck_rtc);
> +
> + /* Enable backup domain write protection */
> + regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, ~PWR_CR_DBP);
> +
> + device_init_wakeup(&pdev->dev, false);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int stm32_rtc_suspend(struct device *dev)
> +{
> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> +
> + if (device_may_wakeup(dev))
> + return enable_irq_wake(rtc->irq_alarm);
> +
> + return 0;
> +}
> +
> +static int stm32_rtc_resume(struct device *dev)
> +{
> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + ret = stm32_rtc_wait_sync(rtc);
> + if (ret < 0)
> + return ret;
> +
> + if (device_may_wakeup(dev))
> + return disable_irq_wake(rtc->irq_alarm);
> +
> + return ret;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(stm32_rtc_pm_ops,
> + stm32_rtc_suspend, stm32_rtc_resume);
> +
> +static struct platform_driver stm32_rtc_driver = {
> + .probe = stm32_rtc_probe,
> + .remove = stm32_rtc_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .pm = &stm32_rtc_pm_ops,
> + .of_match_table = stm32_rtc_of_match,
> + },
> +};
> +
> +module_platform_driver(stm32_rtc_driver);
> +
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay-qxv4g6HH51o@public.gmane.org>");
> +MODULE_DESCRIPTION("STMicroelectronics STM32 Real Time Clock driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
--
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH v2] ARM: dts: Add missing CPU frequencies for Exynos5422/5800
From: Javier Martinez Canillas @ 2016-12-16 19:18 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
Cc: Kukjin Kim, Rob Herring, Mark Rutland, Russell King,
Doug Anderson, Andreas Faerber, Thomas Abraham, Ben Gamari,
Arjun K V, linux-samsung-soc, linux-arm-kernel, linux-pm,
devicetree, linux-kernel
In-Reply-To: <10512254.nyUcL0zgTP@amdc3058>
Hello Bartlomiej,
On 12/15/2016 08:55 AM, Bartlomiej Zolnierkiewicz wrote:
> Add missing 2000MHz & 1900MHz OPPs (for A15 cores) and 1400MHz OPP
> (for A7 cores). Also update common Odroid-XU3 Lite/XU3/XU4 thermal
> cooling maps to account for new OPPs.
>
> Since new OPPs are not available on all Exynos5422/5800 boards modify
> dts files for Odroid-XU3 Lite (limited to 1.8 GHz / 1.3 GHz) & Peach
> Pi (limited to 2.0 GHz / 1.3 GHz) accordingly.
>
> Tested on Odroid-XU3 and XU3 Lite.
>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> Cc: Andreas Faerber <afaerber@suse.de>
> Cc: Thomas Abraham <thomas.ab@samsung.com>
> Cc: Ben Gamari <ben@smart-cactus.org>
> Cc: Arjun K V <arjun.kv@samsung.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
> v2:
> - added comments about limitations of SoC revisions used by Odroid-XU3 Lite and
> Peach Pi boards (suggested by Javier)
> - removed redundant opp_a7_14 label
> - added Arjun to Cc:
>
> Javier, could you test it on Peach Pi board?
>
I've tested this patch on my Peach Pi by setting the CPUFreq governor to
performance and checking that the current scaling frequency was 2.0 GHz:
$ cat /sys/devices/system/cpu/cpu*/cpufreq/scaling_cur_freq
2000000
2000000
2000000
2000000
1400000
1400000
1400000
1400000
I ran the machine for hours and it didn't became unstable. Also installed
the stress [0] tool and setting the governor to ondemand, I executed:
$ stress --cpu 8
For about 30 minutes, the current frequencies were again the maximum and
had no issues.
Finally, I tested some devices that are under the INT rail (MFC and GSC)
by using the following GStreamer pipeline:
$ gst-launch-1.0 filesrc location=test-1080p.mp4 ! qtdemux ! h264parse ! \
v4l2video5dec ! v4l2video7convert capture-io-mode=dmabuf ! kmssink
Again, the system keeps working correctly. So I think it's safe to say:
Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
I can't provide a Reviewed-by since is still not clear to me if these OPP
values are correct or not due the ChromiumOS tree using higher voltages.
FWIW, the chip ID block information on my Exynos5800 Peach Pi is:
Exynos: CPU[EXYNOS5800] CPU_REV[0x1] PKG_ID[0x350c82f8] AUX_INFO[0x4d]
[0]: http://people.seas.harvard.edu/~apw/stress/
Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
^ permalink raw reply
* [PATCH v2 1/3] Bluetooth: btusb: Use an error label for error paths
From: Rajat Jain @ 2016-12-16 19:30 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Marcel Holtmann, Gustavo Padovan,
Johan Hedberg, Amitkumar Karwar, Wei-Ning Huang, Xinming Hu,
netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA, Brian Norris,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Rajat Jain, rajatxjain-Re5JQEeQqe8AvxtiuMwx3w
Use a label to remove the repetetive cleanup, for error cases.
Signed-off-by: Rajat Jain <rajatja-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
v2: same as v1
drivers/bluetooth/btusb.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 2f633df..ce22cef 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2991,18 +2991,15 @@ static int btusb_probe(struct usb_interface *intf,
err = usb_set_interface(data->udev, 0, 0);
if (err < 0) {
BT_ERR("failed to set interface 0, alt 0 %d", err);
- hci_free_dev(hdev);
- return err;
+ goto out_free_dev;
}
}
if (data->isoc) {
err = usb_driver_claim_interface(&btusb_driver,
data->isoc, data);
- if (err < 0) {
- hci_free_dev(hdev);
- return err;
- }
+ if (err < 0)
+ goto out_free_dev;
}
#ifdef CONFIG_BT_HCIBTUSB_BCM
@@ -3016,14 +3013,16 @@ static int btusb_probe(struct usb_interface *intf,
#endif
err = hci_register_dev(hdev);
- if (err < 0) {
- hci_free_dev(hdev);
- return err;
- }
+ if (err < 0)
+ goto out_free_dev;
usb_set_intfdata(intf, data);
return 0;
+
+out_free_dev:
+ hci_free_dev(hdev);
+ return err;
}
static void btusb_disconnect(struct usb_interface *intf)
--
2.8.0.rc3.226.g39d4020
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 2/3] Bluetooth: btusb: Add out-of-band wakeup support
From: Rajat Jain @ 2016-12-16 19:30 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Marcel Holtmann, Gustavo Padovan,
Johan Hedberg, Amitkumar Karwar, Wei-Ning Huang, Xinming Hu,
netdev, devicetree, linux-bluetooth, Brian Norris, linux-kernel
Cc: Rajat Jain, rajatxjain
In-Reply-To: <1481916604-114279-1-git-send-email-rajatja@google.com>
Some onboard BT chips (e.g. Marvell 8997) contain a wakeup pin that
can be connected to a gpio on the CPU side, and can be used to wakeup
the host out-of-band. This can be useful in situations where the
in-band wakeup is not possible or not preferable (e.g. the in-band
wakeup may require the USB host controller to remain active, and
hence consuming more system power during system sleep).
The oob gpio interrupt to be used for wakeup on the CPU side, is
read from the device tree node, (using standard interrupt descriptors).
A devcie tree binding document is also added for the driver. The
compatible string is in compliance with
Documentation/devicetree/bindings/usb/usb-device.txt
Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: * Use interrupt-names ("wakeup") instead of assuming first interrupt.
* Leave it on device tree to specify IRQ flags (level /edge triggered)
* Mark the device as non wakeable on exit.
Documentation/devicetree/bindings/net/btusb.txt | 40 ++++++++++++
drivers/bluetooth/btusb.c | 84 +++++++++++++++++++++++++
2 files changed, 124 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/btusb.txt
diff --git a/Documentation/devicetree/bindings/net/btusb.txt b/Documentation/devicetree/bindings/net/btusb.txt
new file mode 100644
index 0000000..2c0355c
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/btusb.txt
@@ -0,0 +1,40 @@
+Generic Bluetooth controller over USB (btusb driver)
+---------------------------------------------------
+
+Required properties:
+
+ - compatible : should comply with the format "usbVID,PID" specified in
+ Documentation/devicetree/bindings/usb/usb-device.txt
+ At the time of writing, the only OF supported devices
+ (more may be added later) are:
+
+ "usb1286,204e" (Marvell 8997)
+
+Optional properties:
+
+ - interrupt-parent: phandle of the parent interrupt controller
+ - interrupt-names: (see below)
+ - interrupts : The interrupt specified by the name "wakeup" is the interrupt
+ that shall be used for out-of-band wake-on-bt. Driver will
+ request this interrupt for wakeup. During system suspend, the
+ irq will be enabled so that the bluetooth chip can wakeup host
+ platform out of band. During system resume, the irq will be
+ disabled to make sure unnecessary interrupt is not received.
+
+Example:
+
+Following example uses irq pin number 3 of gpio0 for out of band wake-on-bt:
+
+&usb_host1_ehci {
+ status = "okay";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ mvl_bt1: bt@1 {
+ compatible = "usb1286,204e";
+ reg = <1>;
+ interrupt-parent = <&gpio0>;
+ interrupt-name = "wakeup";
+ interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+ };
+};
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index ce22cef..beca4e9 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -24,6 +24,8 @@
#include <linux/module.h>
#include <linux/usb.h>
#include <linux/firmware.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
#include <asm/unaligned.h>
#include <net/bluetooth/bluetooth.h>
@@ -369,6 +371,7 @@ static const struct usb_device_id blacklist_table[] = {
#define BTUSB_BOOTING 9
#define BTUSB_RESET_RESUME 10
#define BTUSB_DIAG_RUNNING 11
+#define BTUSB_OOB_WAKE_DISABLED 12
struct btusb_data {
struct hci_dev *hdev;
@@ -416,6 +419,8 @@ struct btusb_data {
int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
int (*setup_on_usb)(struct hci_dev *hdev);
+
+ int oob_wake_irq; /* irq for out-of-band wake-on-bt */
};
static inline void btusb_free_frags(struct btusb_data *data)
@@ -2728,6 +2733,65 @@ static int btusb_bcm_set_diag(struct hci_dev *hdev, bool enable)
}
#endif
+#ifdef CONFIG_PM
+static irqreturn_t btusb_oob_wake_handler(int irq, void *priv)
+{
+ struct btusb_data *data = priv;
+
+ /* Disable only if not already disabled (keep it balanced) */
+ if (!test_and_set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags)) {
+ disable_irq_nosync(irq);
+ disable_irq_wake(irq);
+ }
+ pm_wakeup_event(&data->udev->dev, 0);
+ return IRQ_HANDLED;
+}
+
+static const struct of_device_id btusb_match_table[] = {
+ { .compatible = "usb1286,204e" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, btusb_match_table);
+
+/* Use an oob wakeup pin? */
+static int btusb_config_oob_wake(struct hci_dev *hdev)
+{
+ struct btusb_data *data = hci_get_drvdata(hdev);
+ struct device *dev = &data->udev->dev;
+ int irq, ret;
+
+ if (!of_match_device(btusb_match_table, dev))
+ return 0;
+
+ /* Move on if no IRQ specified */
+ irq = of_irq_get_byname(dev->of_node, "wakeup");
+ if (irq <= 0) {
+ bt_dev_dbg(hdev, "%s: no OOB Wakeup IRQ in DT", __func__);
+ return 0;
+ }
+
+ set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags);
+
+ ret = devm_request_irq(&hdev->dev, irq, btusb_oob_wake_handler,
+ 0, "OOB Wake-on-BT", data);
+ if (ret) {
+ bt_dev_err(hdev, "%s: IRQ request failed", __func__);
+ return ret;
+ }
+
+ ret = device_init_wakeup(dev, true);
+ if (ret) {
+ bt_dev_err(hdev, "%s: failed to init_wakeup\n", __func__);
+ return ret;
+ }
+
+ data->oob_wake_irq = irq;
+ disable_irq(irq);
+ bt_dev_info(hdev, "OOB Wake-on-BT configured at IRQ %u\n", irq);
+ return 0;
+}
+#endif
+
static int btusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
@@ -2849,6 +2913,11 @@ static int btusb_probe(struct usb_interface *intf,
hdev->send = btusb_send_frame;
hdev->notify = btusb_notify;
+#ifdef CONFIG_PM
+ err = btusb_config_oob_wake(hdev);
+ if (err)
+ goto out_free_dev;
+#endif
if (id->driver_info & BTUSB_CW6622)
set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
@@ -3061,6 +3130,9 @@ static void btusb_disconnect(struct usb_interface *intf)
usb_driver_release_interface(&btusb_driver, data->isoc);
}
+ if (data->oob_wake_irq)
+ device_init_wakeup(&data->udev->dev, false);
+
hci_free_dev(hdev);
}
@@ -3089,6 +3161,12 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
btusb_stop_traffic(data);
usb_kill_anchored_urbs(&data->tx_anchor);
+ if (data->oob_wake_irq && device_may_wakeup(&data->udev->dev)) {
+ clear_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags);
+ enable_irq_wake(data->oob_wake_irq);
+ enable_irq(data->oob_wake_irq);
+ }
+
/* Optionally request a device reset on resume, but only when
* wakeups are disabled. If wakeups are enabled we assume the
* device will stay powered up throughout suspend.
@@ -3126,6 +3204,12 @@ static int btusb_resume(struct usb_interface *intf)
if (--data->suspend_count)
return 0;
+ /* Disable only if not already disabled (keep it balanced) */
+ if (!test_and_set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags)) {
+ disable_irq(data->oob_wake_irq);
+ disable_irq_wake(data->oob_wake_irq);
+ }
+
if (!test_bit(HCI_RUNNING, &hdev->flags))
goto done;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 3/3] Bluetooth: btusb: Configure Marvell to use one of the pins for oob wakeup
From: Rajat Jain @ 2016-12-16 19:30 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Marcel Holtmann, Gustavo Padovan,
Johan Hedberg, Amitkumar Karwar, Wei-Ning Huang, Xinming Hu,
netdev, devicetree, linux-bluetooth, Brian Norris, linux-kernel
Cc: Rajat Jain, rajatxjain
In-Reply-To: <1481916604-114279-1-git-send-email-rajatja@google.com>
The Marvell devices may have many gpio pins, and hence for wakeup
on these out-of-band pins, the chip needs to be told which pin is
to be used for wakeup, using an hci command.
Thus, we read the pin number etc from the device tree node and send
a command to the chip.
Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: Fix the binding document to specify to use "wakeup" interrupt-name
.../{marvell-bt-sd8xxx.txt => marvell-bt-8xxx.txt} | 46 ++++++++++++++---
drivers/bluetooth/btusb.c | 59 ++++++++++++++++++++++
2 files changed, 97 insertions(+), 8 deletions(-)
rename Documentation/devicetree/bindings/net/{marvell-bt-sd8xxx.txt => marvell-bt-8xxx.txt} (50%)
diff --git a/Documentation/devicetree/bindings/net/marvell-bt-sd8xxx.txt b/Documentation/devicetree/bindings/net/marvell-bt-8xxx.txt
similarity index 50%
rename from Documentation/devicetree/bindings/net/marvell-bt-sd8xxx.txt
rename to Documentation/devicetree/bindings/net/marvell-bt-8xxx.txt
index 6a9a63c..9be1059 100644
--- a/Documentation/devicetree/bindings/net/marvell-bt-sd8xxx.txt
+++ b/Documentation/devicetree/bindings/net/marvell-bt-8xxx.txt
@@ -1,16 +1,21 @@
-Marvell 8897/8997 (sd8897/sd8997) bluetooth SDIO devices
+Marvell 8897/8997 (sd8897/sd8997) bluetooth devices (SDIO or USB based)
------
+The 8997 devices supports multiple interfaces. When used on SDIO interfaces,
+the btmrvl driver is used and when used on USB interface, the btusb driver is
+used.
Required properties:
- compatible : should be one of the following:
- * "marvell,sd8897-bt"
- * "marvell,sd8997-bt"
+ * "marvell,sd8897-bt" (for SDIO)
+ * "marvell,sd8997-bt" (for SDIO)
+ * "usb1286,204e" (for USB)
Optional properties:
- marvell,cal-data: Calibration data downloaded to the device during
initialization. This is an array of 28 values(u8).
+ This is only applicable to SDIO devices.
- marvell,wakeup-pin: It represents wakeup pin number of the bluetooth chip.
firmware will use the pin to wakeup host system (u16).
@@ -18,10 +23,15 @@ Optional properties:
platform. The value will be configured to firmware. This
is needed to work chip's sleep feature as expected (u16).
- interrupt-parent: phandle of the parent interrupt controller
- - interrupts : interrupt pin number to the cpu. Driver will request an irq based
- on this interrupt number. During system suspend, the irq will be
- enabled so that the bluetooth chip can wakeup host platform under
- certain condition. During system resume, the irq will be disabled
+ - interrupt-names: Used only for USB based devices (See below)
+ - interrupts : specifies the interrupt pin number to the cpu. For SDIO, the
+ driver will use the first interrupt specified in the interrupt
+ array. For USB based devices, the driver will use the interrupt
+ named "wakeup" from the interrupt-names and interrupt arrays.
+ The driver will request an irq based on this interrupt number.
+ During system suspend, the irq will be enabled so that the
+ bluetooth chip can wakeup host platform under certain
+ conditions. During system resume, the irq will be disabled
to make sure unnecessary interrupt is not received.
Example:
@@ -29,7 +39,9 @@ Example:
IRQ pin 119 is used as system wakeup source interrupt.
wakeup pin 13 and gap 100ms are configured so that firmware can wakeup host
using this device side pin and wakeup latency.
-calibration data is also available in below example.
+
+Example for SDIO device follows (calibration data is also available in
+below example).
&mmc3 {
status = "okay";
@@ -54,3 +66,21 @@ calibration data is also available in below example.
marvell,wakeup-gap-ms = /bits/ 16 <0x64>;
};
};
+
+Example for USB device:
+
+&usb_host1_ohci {
+ status = "okay";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ mvl_bt1: bt@1 {
+ compatible = "usb1286,204e";
+ reg = <1>;
+ interrupt-parent = <&gpio0>;
+ interrupt-names = "wakeup";
+ interrupts = <119 IRQ_TYPE_LEVEL_LOW>;
+ marvell,wakeup-pin = /bits/ 16 <0x0d>;
+ marvell,wakeup-gap-ms = /bits/ 16 <0x64>;
+ };
+};
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index beca4e9..455b3d0 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2343,6 +2343,58 @@ static int btusb_shutdown_intel(struct hci_dev *hdev)
return 0;
}
+#ifdef CONFIG_PM
+static const struct of_device_id mvl_oob_wake_match_table[] = {
+ { .compatible = "usb1286,204e" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, mvl_oob_wake_match_table);
+
+/* Configure an out-of-band gpio as wake-up pin, if specified in device tree */
+static int marvell_config_oob_wake(struct hci_dev *hdev)
+{
+ struct sk_buff *skb;
+ struct btusb_data *data = hci_get_drvdata(hdev);
+ struct device *dev = &data->udev->dev;
+ u16 pin, gap, opcode;
+ int ret;
+ u8 cmd[5];
+
+ if (!of_match_device(mvl_oob_wake_match_table, dev))
+ return 0;
+
+ if (of_property_read_u16(dev->of_node, "marvell,wakeup-pin", &pin) ||
+ of_property_read_u16(dev->of_node, "marvell,wakeup-gap-ms", &gap))
+ return -EINVAL;
+
+ /* Vendor specific command to configure a GPIO as wake-up pin */
+ opcode = hci_opcode_pack(0x3F, 0x59);
+ cmd[0] = opcode & 0xFF;
+ cmd[1] = opcode >> 8;
+ cmd[2] = 2; /* length of parameters that follow */
+ cmd[3] = pin;
+ cmd[4] = gap; /* time in ms, for which wakeup pin should be asserted */
+
+ skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
+ if (!skb) {
+ bt_dev_err(hdev, "%s: No memory\n", __func__);
+ return -ENOMEM;
+ }
+
+ memcpy(skb_put(skb, sizeof(cmd)), cmd, sizeof(cmd));
+ hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
+
+ ret = btusb_send_frame(hdev, skb);
+ if (ret) {
+ bt_dev_err(hdev, "%s: configuration failed\n", __func__);
+ kfree_skb(skb);
+ return ret;
+ }
+
+ return 0;
+}
+#endif
+
static int btusb_set_bdaddr_marvell(struct hci_dev *hdev,
const bdaddr_t *bdaddr)
{
@@ -2917,6 +2969,13 @@ static int btusb_probe(struct usb_interface *intf,
err = btusb_config_oob_wake(hdev);
if (err)
goto out_free_dev;
+
+ /* Marvell devices may need a specific chip configuration */
+ if (id->driver_info & BTUSB_MARVELL && data->oob_wake_irq) {
+ err = marvell_config_oob_wake(hdev);
+ if (err)
+ goto out_free_dev;
+ }
#endif
if (id->driver_info & BTUSB_CW6622)
set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH 2/3] Bluetooth: btusb: Add out-of-band wakeup support
From: Rajat Jain @ 2016-12-16 19:43 UTC (permalink / raw)
To: Brian Norris
Cc: Rob Herring, Mark Rutland, Marcel Holtmann, Gustavo Padovan,
Johan Hedberg, Amitkumar Karwar, Wei-Ning Huang, Xinming Hu,
netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA, Rajat Jain
In-Reply-To: <20161215032105.GA88921-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Hi Brian,
I've just posted a v2 patchset after taking care of your your
comments. Please see inline below.
On Wed, Dec 14, 2016 at 7:21 PM, Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> Hi,
>
> On Wed, Dec 14, 2016 at 11:12:58AM -0800, Rajat Jain wrote:
>> Some BT chips (e.g. Marvell 8997) contain a wakeup pin that can be
>> connected to a gpio on the CPU side, and can be used to wakeup
>> the host out-of-band. This can be useful in situations where the
>> in-band wakeup is not possible or not preferable (e.g. the in-band
>> wakeup may require the USB host controller to remain active, and
>> hence consuming more system power during system sleep).
>>
>> The oob gpio interrupt to be used for wakeup on the CPU side, is
>> read from the device tree node, (using standard interrupt descriptors).
>> A devcie tree binding document is also added for the driver.
>>
>> Signed-off-by: Rajat Jain <rajatja-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>> ---
>> Documentation/devicetree/bindings/net/btusb.txt | 38 ++++++++++++
>> drivers/bluetooth/btusb.c | 82 +++++++++++++++++++++++++
>> 2 files changed, 120 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/btusb.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/btusb.txt b/Documentation/devicetree/bindings/net/btusb.txt
>> new file mode 100644
>> index 0000000..bb27f92
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/btusb.txt
>> @@ -0,0 +1,38 @@
>> +Generic Bluetooth controller over USB (btusb driver)
>> +---------------------------------------------------
>> +
>> +Required properties:
>> +
>> + - compatible : should comply with the format "usbVID,PID" specified in
>> + Documentation/devicetree/bindings/usb/usb-device.txt
>> + At the time of writing, the only OF supported devices
>> + (more may be added later) are:
>> +
>> + "usb1286,204e" (Marvell 8997)
>> +
>> +Optional properties:
>> +
>> + - interrupt-parent: phandle of the parent interrupt controller
>> + - interrupts : The first interrupt specified is the interrupt that shall be
>> + used for out-of-band wake-on-bt. Driver will request an irq
>> + based on this interrupt number. During system suspend, the irq
>> + will be enabled so that the bluetooth chip can wakeup host
>> + platform out of band. During system resume, the irq will be
>> + disabled to make sure unnecessary interrupt is not received.
>
> Might it be worthwhile to define an 'interrupt-names' property (e.g., =
> "wakeup") to help future-proof this?
Good idea, I've used the same.
>
>> +
>> +Example:
>> +
>> +Following example uses irq pin number 3 of gpio0 for out of band wake-on-bt:
>> +
>> +&usb_host1_ehci {
>> + status = "okay";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + mvl_bt1: bt@1 {
>> + compatible = "usb1286,204e";
>> + reg = <1>;
>> + interrupt-parent = <&gpio0>;
>> + interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
>> + };
>> +};
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index ce22cef..32a6f22 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -24,6 +24,8 @@
>> #include <linux/module.h>
>> #include <linux/usb.h>
>> #include <linux/firmware.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>> #include <asm/unaligned.h>
>>
>> #include <net/bluetooth/bluetooth.h>
>> @@ -369,6 +371,7 @@ static const struct usb_device_id blacklist_table[] = {
>> #define BTUSB_BOOTING 9
>> #define BTUSB_RESET_RESUME 10
>> #define BTUSB_DIAG_RUNNING 11
>> +#define BTUSB_OOB_WAKE_DISABLED 12
>>
>> struct btusb_data {
>> struct hci_dev *hdev;
>> @@ -416,6 +419,8 @@ struct btusb_data {
>> int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
>>
>> int (*setup_on_usb)(struct hci_dev *hdev);
>> +
>> + int oob_wake_irq; /* irq for out-of-band wake-on-bt */
>> };
>>
>> static inline void btusb_free_frags(struct btusb_data *data)
>> @@ -2728,6 +2733,65 @@ static int btusb_bcm_set_diag(struct hci_dev *hdev, bool enable)
>> }
>> #endif
>>
>> +#ifdef CONFIG_PM
>> +static irqreturn_t btusb_oob_wake_handler(int irq, void *priv)
>> +{
>> + struct btusb_data *data = priv;
>> +
>> + /* Disable only if not already disabled (keep it balanced) */
>> + if (!test_and_set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags)) {
>> + disable_irq_wake(irq);
>> + disable_irq_nosync(irq);
>> + }
>> + pm_wakeup_event(&data->udev->dev, 0);
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static const struct of_device_id btusb_match_table[] = {
>> + { .compatible = "usb1286,204e" },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, btusb_match_table);
>> +
>> +/* Use an oob wakeup pin? */
>> +static int btusb_config_oob_wake(struct hci_dev *hdev)
>> +{
>> + struct btusb_data *data = hci_get_drvdata(hdev);
>> + struct device *dev = &data->udev->dev;
>> + int irq, ret;
>> +
>> + if (!of_match_device(btusb_match_table, dev))
>> + return 0;
>> +
>> + /* Move on if no IRQ specified */
>> + irq = irq_of_parse_and_map(dev->of_node, 0);
>
> Better to use of_irq_get{,_byname}(), no?
I've used_irq_get_byname().
>
>> + if (!irq) {
>> + bt_dev_dbg(hdev, "%s: no oob wake irq in DT", __func__);
>> + return 0;
>> + }
>> +
>> + set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags);
>> +
>> + ret = devm_request_irq(&hdev->dev, irq, btusb_oob_wake_handler,
>> + IRQF_TRIGGER_LOW, "oob wake-on-bt", data);
>
> You're assuming this is level-triggered, and active-low? Can't this just
> be specified in the device tree and just pass 0 here?
>
> Also, it seems like it would be a lot more convenient if we could treat
> this as edge-triggered, so we don't have to do the set/clear flags,
> disable IRQ, etc., dance. You'd just have to change the device tree
> definition. Is there any downside to doing that?
Now, I don't put any assumptions in the driver and use whatever is
specified in the device tree. So platforms can do whatever they want
(However, I think configuring as edge triggered in the device tree (if
some platform chooses to) may leave some corner cases where the
interrupt might be missed if it was already asserted for some reason).
Thanks,
Rajat
>
> It would also then be a better candidate for using something like
> dev_pm_set_dedicated_wake_irq() (although last time I tried using that,
> it didn't do so great if you don't have autosuspend enabled -- but I
> think there are patches outstanding for that; so maybe not yet).
>
>> + if (ret) {
>> + bt_dev_err(hdev, "%s: irq request failed", __func__);
>> + return ret;
>> + }
>> +
>> + ret = device_init_wakeup(dev, true);
>> + if (ret) {
>> + bt_dev_err(hdev, "%s: failed to init_wakeup\n", __func__);
>> + return ret;
>> + }
>> +
>> + data->oob_wake_irq = irq;
>> + disable_irq(irq);
>> + bt_dev_info(hdev, "oob wake-on-bt configured at irq %u\n", irq);
>
> oob and bt are typically capitalized in strings. And maybe irq too.
> Also, you declared irq as 'int', so %d instead of %u.
>
> Brian
>
>> + return 0;
>> +}
>> +#endif
>> +
>> static int btusb_probe(struct usb_interface *intf,
>> const struct usb_device_id *id)
>> {
>> @@ -2849,6 +2913,11 @@ static int btusb_probe(struct usb_interface *intf,
>> hdev->send = btusb_send_frame;
>> hdev->notify = btusb_notify;
>>
>> +#ifdef CONFIG_PM
>> + err = btusb_config_oob_wake(hdev);
>> + if (err)
>> + goto out_free_dev;
>> +#endif
>> if (id->driver_info & BTUSB_CW6622)
>> set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
>>
>> @@ -3089,6 +3158,12 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
>> btusb_stop_traffic(data);
>> usb_kill_anchored_urbs(&data->tx_anchor);
>>
>> + if (data->oob_wake_irq) {
>> + clear_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags);
>> + enable_irq(data->oob_wake_irq);
>> + enable_irq_wake(data->oob_wake_irq);
>> + }
>> +
>> /* Optionally request a device reset on resume, but only when
>> * wakeups are disabled. If wakeups are enabled we assume the
>> * device will stay powered up throughout suspend.
>> @@ -3126,6 +3201,13 @@ static int btusb_resume(struct usb_interface *intf)
>> if (--data->suspend_count)
>> return 0;
>>
>> + /* Disable only if not already disabled (keep it balanced) */
>> + if (data->oob_wake_irq &&
>> + !test_and_set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags)) {
>> + disable_irq_wake(data->oob_wake_irq);
>> + disable_irq(data->oob_wake_irq);
>> + }
>> +
>> if (!test_bit(HCI_RUNNING, &hdev->flags))
>> goto done;
>>
>> --
>> 2.8.0.rc3.226.g39d4020
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v6 0/5] Altera Cyclone Passive Serial SPI FPGA Manager
From: Joshua Clayton @ 2016-12-16 23:17 UTC (permalink / raw)
To: Alan Tull, Moritz Fischer, Russell King, Catalin Marinas,
Will Deacon, Shawn Guo, Sascha Hauer, Fabio Estevam
Cc: Mark Rutland, Rob Herring, Anatolij Gustschin, Joshua Clayton,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-fpga-u79uwXL29TY76Z2rM5mHXA
This series adds an FPGA manager for Altera cyclone FPGAs
that can program them using an spi port and a couple of gpios, using
Alteras passive serial protocol.
Need ACKs from ARCH maintainers for ARCH specific implementations of
__arch_bitrev8x4(), and I've added more ARCHes, so will need more ACKS,
but the generic code will work without that patch, so if there is a holdup
the rest of the series can go in without patch 2
Changes from v5:
- Rebased on next-20161214xi
- Corrected for FPGA Mgr API change in write_init() and write_complete()
- Better describe the device cyclone-ps-spi runs on in the file header.
- Split the bitrev8x4 patch into generic and arch specific patches...
- Added AARCH64 and MIPS implementations of bitrev8x4()... they all have to
have an implementation for it to compile cleanly across platforms
- Added the changes to imx6q-evi.dts to the patch set.
Changes from v4:
- Added the needed return statement to __arch_bitrev8x4()
- Added Rob Herrings ACK for and fix a typo in the commit log of patch 2
Changes from v3:
- Fixed up the state() function to return the state of the status pin
reqested by Alan Tull
- Switched the pin to ACTIVE_LOW and coresponding logic level, and updated
the corresponding documentation. Thanks Rob Herring for pointing out my
mistake.
- Per Rob Herring, switched from "gpio" to "gpios" in dts
Changes from v2:
- Merged patch 3 and 4 as suggested in review by Moritz Fischer
- Changed FPGA_MIN_DELAY from 250 to 50 ms is the time advertized by
Altera. This now works, as we don't assume it is done
Changes from v1:
- Changed the name from cyclone-spi-fpga-mgr to cyclone-ps-spi-fpga-mgr
This name change was requested by Alan Tull, to be specific about which
programming method is being employed on the fpga.
- Changed the name of the reset-gpio to config-gpio to closer match the
way the pins are described in the Altera manual
- Moved MODULE_LICENCE, _AUTHOR, and _DESCRIPTION to the bottom
- Added a bitrev8x4() function to the bitrev headers and implemented ARM
const, runtime, and ARM specific faster versions (This may end up
needing to be a standalone patch)
- Moved the bitswapping into cyclonespi_write(), as requested.
This falls short of my desired generic lsb first spi support, but is a step
in that direction.
- Fixed whitespace problems introduced during refactoring
- Replaced magic number for initial delay with a descriptive macro
- Poll the fpga to see when it is ready rather than a fixed 1 ms sleep
Joshua Clayton (5):
lib: add bitrev8x4()
lib: implement __arch_bitrev8x4()
doc: dt: add cyclone-ps-spi binding document
fpga manager: Add cyclone-ps-spi driver for Altera FPGAs
ARM: dts: imx6q-evi: support cyclone-ps-spi
.../bindings/fpga/cyclone-ps-spi-fpga-mgr.txt | 25 +++
arch/arm/boot/dts/imx6q-evi.dts | 16 ++
arch/arm/include/asm/bitrev.h | 6 +
arch/arm64/include/asm/bitrev.h | 6 +
arch/mips/include/asm/bitrev.h | 6 +
drivers/fpga/Kconfig | 7 +
drivers/fpga/Makefile | 1 +
drivers/fpga/cyclone-ps-spi.c | 186 +++++++++++++++++++++
include/linux/bitrev.h | 26 +++
9 files changed, 279 insertions(+)
create mode 100644 Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
create mode 100644 drivers/fpga/cyclone-ps-spi.c
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ 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