* Re: [PATCH] ARM: dts: sun8i-q8-common: enable bluetooth on SDIO Wi-Fi
From: Maxime Ripard @ 2016-12-16 12:47 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Hans de Goede, Chen-Yu Tsai, linux-kernel, linux-arm-kernel,
devicetree
In-Reply-To: <20161213233658.atGuNCNY@smtp1h.mail.yandex.net>
[-- Attachment #1.1: Type: text/plain, Size: 2945 bytes --]
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.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [RFC PATCH] iommu/arm-smmu: Add global SMR masking property
From: Robin Murphy @ 2016-12-16 13:19 UTC (permalink / raw)
To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: will.deacon-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
stuart.yoder-3arQi8VN3Tc
The current SMR masking support using a 2-cell iommu-specifier is
primarily intended to handle individual masters with large and/or
complex Stream ID assignments; it quickly gets a bit clunky in other SMR
use-cases where we just want to consistently mask out the same part of
every Stream ID (e.g. for MMU-500 configurations where the appended TBU
number gets in the way unnecessarily). Let's add a new property to allow
a single global mask value to better fit the latter situation.
CC: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
Compile-tested only...
Documentation/devicetree/bindings/iommu/arm,smmu.txt | 8 ++++++++
drivers/iommu/arm-smmu.c | 4 +++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index e862d1485205..98f5cbe5fdb4 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -60,6 +60,14 @@ conditions.
aliases of secure registers have to be used during
SMMU configuration.
+- stream-match-mask : Specifies a fixed SMR mask value to combine with
+ the Stream ID value from every iommu-specifier. This
+ may be used instead of an "#iommu-cells" value of 2
+ when there is no need for per-master SMR masks, but
+ it is still desired to mask some portion of every
+ Stream ID (e.g. for certain MMU-500 configurations
+ given globally unique external IDs).
+
** Deprecated properties:
- mmu-masters (deprecated in favour of the generic "iommus" binding) :
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 8f7281444551..f1abcb7dde36 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1534,13 +1534,15 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
{
- u32 fwid = 0;
+ u32 mask, fwid = 0;
if (args->args_count > 0)
fwid |= (u16)args->args[0];
if (args->args_count > 1)
fwid |= (u16)args->args[1] << SMR_MASK_SHIFT;
+ else if (!of_property_read_u32(args->np, "stream-match-mask", &mask))
+ fwid |= (u16)mask << SMR_MASK_SHIFT;
return iommu_fwspec_add_ids(dev, &fwid, 1);
}
--
2.10.2.dirty
--
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: [PATCH v7 1/2] mtd: nand: add tango NFC dt bindings doc
From: Marc Gonzalez @ 2016-12-16 13:26 UTC (permalink / raw)
To: Arnd Bergmann, Boris Brezillon
Cc: Mark Rutland, DT, Mans Rullgard, Mason, Richard Weinberger,
Mark Brown, Rob Herring, linux-mtd, Sebastian Frias
In-Reply-To: <d3cbfda6-23bf-4ad1-b1ce-c179b765272f@sigmadesigns.com>
On 16/12/2016 11:56, Marc Gonzalez wrote:
> On 07/11/2016 10:18, Arnd Bergmann wrote:
>> On Tuesday, October 25, 2016 3:15:50 PM CET Marc Gonzalez wrote:
>>> Add the tango NAND Flash Controller dt bindings documentation.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>> Documentation/devicetree/bindings/mtd/tango-nand.txt | 38 ++++++++++++++++++++++++++++++
>>> 1 file changed, 38 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/tango-nand.txt b/Documentation/devicetree/bindings/mtd/tango-nand.txt
>>> new file mode 100644
>>> index 000000000000..3cbf95d6595a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mtd/tango-nand.txt
>>> @@ -0,0 +1,38 @@
>>> +Sigma Designs Tango4 NAND Flash Controller (NFC)
>>> +
>>> +Required properties:
>>> +
>>> +- compatible: "sigma,smp8758-nand"
>>> +- reg: address/size of nfc_reg, nfc_mem, and pbus_reg
>>> +- dmas: reference to the DMA channel used by the controller
>>> +- dma-names: "nfc_sbox"
>>
>> Drop the "nfc_" prefix here, it seems redundant.
>
> I took a closer look at Documentation/devicetree/bindings and
> arch/arm/boot/dts and it seems the overwhelming nomenclature
> is "rx", "tx" (with variants around these two).
>
> Should I just use "rxtx" for my driver?
On IRC, Arnd wrote:
"The string for dma-names is not important, it just needs to be documented
in the binding. If you have hardware specifications, use the name that is
next to the wire for the dma-request line. "dma-names" is only required so
you can connect a single dmarq to multiple dma engines (most chips only
connect each rq to one dmaengine though)."
In my system, peripheral devices are connected to DMA channels via
a multiplexer called the "host switch box". The documentation calls
the ports FOO_SBOX and SBOX_FOO.
For MLC NAND Flash controller 0, the ports are mlc_flash0_sbox
and sbox_mlc_flash0.
Additionally, the DMA driver handles both directions, so the DT only
defines a single duplex channel.
I originally used "nfc_sbox" (nfc = NAND Flash controller) to stick to
the HW naming, but I am willing to change it to "rxtx" if that's what
Boris prefers, (or just "sbox", but Mans pointed out that this was too
specific, and future HW might do away with the switch box).
I'm open to comments.
Regards.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply
* Re: [PATCH v7 1/2] mtd: nand: add tango NFC dt bindings doc
From: Mark Rutland @ 2016-12-16 13:33 UTC (permalink / raw)
To: Marc Gonzalez
Cc: Arnd Bergmann, Boris Brezillon, linux-mtd, Richard Weinberger, DT,
Rob Herring, Mason, Sebastian Frias, Mans Rullgard, Mark Brown
In-Reply-To: <ee9efd0a-59f5-8c21-0cad-40ca30e3a3e7-y1yR0Z3OICC7zZZRDBGcUA@public.gmane.org>
On Fri, Dec 16, 2016 at 02:26:33PM +0100, Marc Gonzalez wrote:
> On 16/12/2016 11:56, Marc Gonzalez wrote:
> > Should I just use "rxtx" for my driver?
>
> On IRC, Arnd wrote:
>
> "The string for dma-names is not important, it just needs to be documented
> in the binding. If you have hardware specifications, use the name that is
> next to the wire for the dma-request line. "dma-names" is only required so
> you can connect a single dmarq to multiple dma engines (most chips only
> connect each rq to one dmaengine though)."
>
> In my system, peripheral devices are connected to DMA channels via
> a multiplexer called the "host switch box". The documentation calls
> the ports FOO_SBOX and SBOX_FOO.
>
> For MLC NAND Flash controller 0, the ports are mlc_flash0_sbox
> and sbox_mlc_flash0.
>
> Additionally, the DMA driver handles both directions, so the DT only
> defines a single duplex channel.
>
> I originally used "nfc_sbox" (nfc = NAND Flash controller) to stick to
> the HW naming, but I am willing to change it to "rxtx" if that's what
> Boris prefers, (or just "sbox", but Mans pointed out that this was too
> specific, and future HW might do away with the switch box).
"rxtx" is the best option, if you need a name at all.
The name is relative to the device instance, so "nfc_" is unnecessary;
we know the node is an NFC controller. The sbox is part of the SoC
wiring rather than being part of the NFC controller, so the "sbox" part
also shouldn't be part of the name.
Likewise for the NAND flash controller. There, the "0" instance number
also shouldn't have been there -- the name is relative to the instance,
and two instances should use the same names. Too late now, I guess. :(
Thanks,
Mark.
--
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] Docs: dt: Be explicit and consistent in reference to IOMMU specifiers
From: Stuart Yoder @ 2016-12-16 14:08 UTC (permalink / raw)
To: Mark Rutland
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: <20161216113317.GB20265@leverpostej>
> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland-5wv7dgnIgG8@public.gmane.org]
> Sent: Friday, December 16, 2016 5:33 AM
> To: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>
> Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org;
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; will.deacon-5wv7dgnIgG8@public.gmane.org
> Subject: Re: [PATCH] Docs: dt: Be explicit and consistent in reference to IOMMU specifiers
>
> Hi Stuart,
>
> On Thu, Dec 15, 2016 at 06:16:13PM -0600, Stuart Yoder wrote:
> > The generic IOMMU binding says that the meaning of an 'IOMMU specifier'
> > is defined by the binding of a specific SMMU. The ARM SMMU binding
> > never explicitly uses the term 'specifier' at all. Update implicit
> > references to use the explicit term.
> >
> > 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". 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.
Thanks,
Stuart
^ permalink raw reply
* [PATCH] Documentation: dt: Explicitly mark Samsung Exynos SoC bindings as unstable
From: Marek Szyprowski @ 2016-12-16 14:14 UTC (permalink / raw)
To: linux-samsung-soc, devicetree, linux-arm-kernel
Cc: Mark Rutland, Chanwoo Choi, Bartlomiej Zolnierkiewicz,
Seung-Woo Kim, Krzysztof Kozlowski, Inki Dae,
Javier Martinez Canillas, Rob Herring, Kukjin Kim,
Sylwester Nawrocki, Marek Szyprowski
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>
---
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.
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v5 2/6] [media] rc-main: split setup and unregister functions
From: Sean Young @ 2016-12-16 14:16 UTC (permalink / raw)
To: Andi Shyti
Cc: Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Richard Purdie,
Jacek Anaszewski, Heiner Kallweit, linux-media, devicetree,
linux-leds, linux-kernel, Andi Shyti
In-Reply-To: <20161216121026.GA31618@gofer.mess.org>
Hi Andi,
On Fri, Dec 16, 2016 at 12:10:26PM +0000, Sean Young wrote:
> Sorry to add to your woes, but there are some checkpatch warnings and
> errors. Please can you correct these. One is below.
Actually, the changes are pretty minor, I can fix them up before sending
them to Mauro. Sorry for bothering you.
Sean
^ permalink raw reply
* Re: [PATCH] Documentation: dt: Explicitly mark Samsung Exynos SoC bindings as unstable
From: Javier Martinez Canillas @ 2016-12-16 14:18 UTC (permalink / raw)
To: Marek Szyprowski, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Rob Herring, Mark Rutland, Krzysztof Kozlowski,
Bartlomiej Zolnierkiewicz, Kukjin Kim, Inki Dae, Seung-Woo Kim,
Chanwoo Choi, Sylwester Nawrocki
In-Reply-To: <1481897676-13578-1-git-send-email-m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Hello Marek,
On 12/16/2016 11:14 AM, 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
I completely agree with you on this.
Reviewed-by: Javier Martinez Canillas <javier-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
--
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
* RE: RFC: extend iommu-map binding to support #iommu-cells > 1
From: Stuart Yoder @ 2016-12-16 14:21 UTC (permalink / raw)
To: 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: <20161216113914.GC20265@leverpostej>
> -----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". Why can't it be
an iommu specific definition that makes sense for a given IOMMU?
Stuart
^ permalink raw reply
* 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
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