Re: [PATCH RFC 2/4] ARM: bcm2835: rename sdhci pin group
On 12/05/2015 02:43 AM, Stefan Wahren wrote:
>
>> Stephen Warren hat am 2. Dezember 2015 um 04:42
>> geschrieben:
>>
>>
>> On 11/19/2015 09:06 AM, Stefan Wahren wrote:
>>> The node name of the sdhci pin group doesn't explain it's
>>> real function. So rename it.
>>
>> The real function of this node is not to configure SDHCI pins, but to
>> set pins to alt3, as the current name states. Admittedly it's possible
>> that currently the only pins that need to be set to ALT3 are SDHCI
>> related, but that's incidental.
>
> Yes, i understand the original intension to assign every pin to the available
> mux functions ( gpio_in, gpio_out, alt* ).
>
> But 3f37169fb3 ("ARM: bcm2835: dt: Add Raspberry Pi Model B rev2") introduce a
FYI I don't have that yet and git fetch is being very slow right now:-(
> better self-describing pin group naming for I2S. So my idea was to adapt it
> according to sdhci first and go on.
OK. I'd suggest explaining that directly in the commit description then.
The commit description above has a quite different semantic meaning.
> So here is a possible vision for bcm2835-rpi.dtsi
>
> &gpio {
> pinctrl-names = "default";
>
> act_gpio: gpio {
> brcm,pins = <6>;
> brcm,function = ;
> };
>
> i2c0_alt0: i2c {
> brcm,pins = <0 1>;
> brcm,function = ;
> };
>
> i2c1_alt0: i2c {
> brcm,pins = <2 3>;
> brcm,function = ;
> };
...
OK, I guess that can work; I imagine it would make DT overlays easier by
making the pinctrl setup more fine-grained.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/4] ARM: bcm2835: remove sdhci pins from GPIO pinctrl
On 12/05/2015 02:12 AM, Stefan Wahren wrote:
>
>> Stephen Warren hat am 2. Dezember 2015 um 04:40
>> geschrieben:
>>
>>
>> On 11/19/2015 09:06 AM, Stefan Wahren wrote:
>>> Currently the pins alt3 (sdhci) are assigned to GPIO pinctrl.
>>> This is bad because a user could export it to sysfs and break
>>> sdhci. In order to avoid that remove those pins from GPIO pintrl.
>>
>>> diff --git a/arch/arm/boot/dts/bcm2835-rpi-a-plus.dts
>>> b/arch/arm/boot/dts/bcm2835-rpi-a-plus.dts
>>
>>> &gpio {
>>> - pinctrl-0 = <&gpioout &alt0 &i2s_alt0 &alt3>;
>>> + pinctrl-0 = <&gpioout &alt0 &i2s_alt0>;
>>
>> This doesn't make sense. The current DT content is configuring those
>> pins as SDHCI, not as GPIO. Admitedly this is redundant since the
>> firmware and/or bootloader already did this in order to boot the system,
>> but irrespective, the current DT causes no issues. Removing the pinctrl
>> setting should not influence whether the pins can be exported via GPIO
>> sysfs either.
>
> You are right.
>
> Is it generally possible to avoid the GPIO sysfs export for SDHCI pins?
> Is it an issue of pinctrl-bcm2835?
I believe this same issue exists on all platforms where GPIO pins can be
mux'd onto the same pins as other functions.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/4] ARM: dts: vf610: relicense the device trees under GPLv2/X11
On 12/07/2015 02:51 PM, Stefan Agner wrote: I collected the Acks I received so far and removed them from the list below. Several Freescale addresses are no longer valid (the once starting with --)... I would interprete the Ack from Yuan Yao as an Ack from Freescale. Matt, you introduced the vf610-cosmic.dts board, is it possible to get an ack for this too? I think you did not receive v1 of this patch due to an old email address, sorry about that. I thought I'd already sent an ack for this, but it looks like I forgot somehow. Acked-by: Stephen Warren -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 3/4] ARM: bcm2835: specify card detect pin for RPi B
On 11/19/2015 09:06 AM, Stefan Wahren wrote: > Only the Raspberry Pi B has a card detect pin. Specify it > on board level because it's not free to use. This seems fine, but it should have no effect in practice; when the SD controller driver gpio_get()s the GPIO, the same setting will be programmed into HW. There's a requirement to use the pinctrl bindings to configure pins to non-GPIO mux settings, but no actual requirement to do so for GPIOs. However, the new node sdhci_cd should be added to some pinctrl-0 property or it won't be used. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 2/4] ARM: bcm2835: rename sdhci pin group
On 11/19/2015 09:06 AM, Stefan Wahren wrote: > The node name of the sdhci pin group doesn't explain it's > real function. So rename it. The real function of this node is not to configure SDHCI pins, but to set pins to alt3, as the current name states. Admittedly it's possible that currently the only pins that need to be set to ALT3 are SDHCI related, but that's incidental. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/4] ARM: bcm2835: remove sdhci pins from GPIO pinctrl
On 11/19/2015 09:06 AM, Stefan Wahren wrote:
> Currently the pins alt3 (sdhci) are assigned to GPIO pinctrl.
> This is bad because a user could export it to sysfs and break
> sdhci. In order to avoid that remove those pins from GPIO pintrl.
> diff --git a/arch/arm/boot/dts/bcm2835-rpi-a-plus.dts
> b/arch/arm/boot/dts/bcm2835-rpi-a-plus.dts
> &gpio {
> - pinctrl-0 = <&gpioout &alt0 &i2s_alt0 &alt3>;
> + pinctrl-0 = <&gpioout &alt0 &i2s_alt0>;
This doesn't make sense. The current DT content is configuring those
pins as SDHCI, not as GPIO. Admitedly this is redundant since the
firmware and/or bootloader already did this in order to boot the system,
but irrespective, the current DT causes no issues. Removing the pinctrl
setting should not influence whether the pins can be exported via GPIO
sysfs either.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] ARM: dts: vf610: relicense vf???.dtsi under GPLv2/X11
On 11/23/2015 04:57 PM, Stefan Agner wrote: GPLv2-only devicetrees make reuse difficult for software components licensed under a different license. The consensus is that a GPL/X11 dual-license should allow all necessary uses, so relicense the vfxxx.dtsi, vf500.dtsi and vf610.dtsi files to this combination. CCs were acquired using (updated some email addresses): git shortlog -sne --no-merges arch/arm/boot/dts/vf???.dtsi Acked-by: Stephen Warren -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support
On 11/16/2015 01:30 PM, Stephen Warren wrote: On 11/13/2015 10:58 AM, Andrew Bresticker wrote: On Fri, Nov 13, 2015 at 8:32 AM, Thierry Reding wrote: On Wed, Nov 04, 2015 at 01:59:51PM -0700, Stephen Warren wrote: On 11/04/2015 10:11 AM, Thierry Reding wrote: From: Thierry Reding Extend the binding to cover the set of feature found in Tegra210. diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt +For Tegra210, the list of valid PHY nodes is given below: +- utmi: utmi-0, utmi-1, utmi-2, utmi-3 + - functions: "snps", "xusb", "uart" +- hsic: hsic-0, hsic-1 + - functions: "snps", "xusb" +- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, pcie-5, pcie-6 + - functions: "pcie-x1", "usb3-ss", "pcie-x4" +- sata: sata-0 + - functions: "usb3-ss", "sata" usb2-bias also needs to be present. I'm not sure about this. All of the driver code that I've looked deals with the usb2-bias pad internally. As far as I can tell, this pad needs to be configured to whatever any of the other pads is configured for. I think that means if any of the UTMI pads is configured for XUSB then the usb2-bias pad must also be configured for XUSB. Which would also imply that if one of the UTMI pads is configured for XUSB, all of them must be configured for XUSB. I was told by hardware engineers at NVIDIA that (at least on Tegra124/Tegra132) the usb2-bias pad must be configured in the XUSB_PADCTL register space if UTMI pad 0 is muxed to XUSB. If UTMI pad 0 is muxed to SNPS, then the usb2-bias pad is configured in the USB register space (base 0x7d00). You may want to follow up internally to confirm this. If it's true, that could make things here a bit nastier, especially if we want to support configurations where some pads are muxed to XUSB while others are muxed to SNPS. Hmm. I've certainly successfully tested a configuration where UTMI pad 0 was handled by the SNPS controller and other pads by the XUSB controller *and* where I set the usb2-bias "pad"'s muxing and configuration via the XUSB PADCTL module. In that case, I /had/ to configure usb2-bias via XUSB PADCTL or the other XUSB pads didn't work. However, perhaps that was because the XUSB controller driver probed before the SNPS driver; perhaps if they'd probed the other way around and the SNPS driver configured the bias pad, then everything would have worked without configuring the bias pad via XUSB PADCTL. I suppose I'll have to start another internal thread to get the full details, and differentiate between "recommended" and "supported" and "must" vs. "can"/"should". I've discussed this with a HW engineer, and apparently this rule is simply a recommendation, with the acknowledgement that everything will work perfectly fine if we ignore it. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support
On 11/13/2015 10:58 AM, Andrew Bresticker wrote: On Fri, Nov 13, 2015 at 8:32 AM, Thierry Reding wrote: On Wed, Nov 04, 2015 at 01:59:51PM -0700, Stephen Warren wrote: On 11/04/2015 10:11 AM, Thierry Reding wrote: From: Thierry Reding Extend the binding to cover the set of feature found in Tegra210. diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt +For Tegra210, the list of valid PHY nodes is given below: +- utmi: utmi-0, utmi-1, utmi-2, utmi-3 + - functions: "snps", "xusb", "uart" +- hsic: hsic-0, hsic-1 + - functions: "snps", "xusb" +- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, pcie-5, pcie-6 + - functions: "pcie-x1", "usb3-ss", "pcie-x4" +- sata: sata-0 + - functions: "usb3-ss", "sata" usb2-bias also needs to be present. I'm not sure about this. All of the driver code that I've looked deals with the usb2-bias pad internally. As far as I can tell, this pad needs to be configured to whatever any of the other pads is configured for. I think that means if any of the UTMI pads is configured for XUSB then the usb2-bias pad must also be configured for XUSB. Which would also imply that if one of the UTMI pads is configured for XUSB, all of them must be configured for XUSB. I was told by hardware engineers at NVIDIA that (at least on Tegra124/Tegra132) the usb2-bias pad must be configured in the XUSB_PADCTL register space if UTMI pad 0 is muxed to XUSB. If UTMI pad 0 is muxed to SNPS, then the usb2-bias pad is configured in the USB register space (base 0x7d00). You may want to follow up internally to confirm this. If it's true, that could make things here a bit nastier, especially if we want to support configurations where some pads are muxed to XUSB while others are muxed to SNPS. Hmm. I've certainly successfully tested a configuration where UTMI pad 0 was handled by the SNPS controller and other pads by the XUSB controller *and* where I set the usb2-bias "pad"'s muxing and configuration via the XUSB PADCTL module. In that case, I /had/ to configure usb2-bias via XUSB PADCTL or the other XUSB pads didn't work. However, perhaps that was because the XUSB controller driver probed before the SNPS driver; perhaps if they'd probed the other way around and the SNPS driver configured the bias pad, then everything would have worked without configuring the bias pad via XUSB PADCTL. I suppose I'll have to start another internal thread to get the full details, and differentiate between "recommended" and "supported" and "must" vs. "can"/"should". -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support
On 11/13/2015 09:32 AM, Thierry Reding wrote: On Wed, Nov 04, 2015 at 01:59:51PM -0700, Stephen Warren wrote: On 11/04/2015 10:11 AM, Thierry Reding wrote: From: Thierry Reding Extend the binding to cover the set of feature found in Tegra210. diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt +PCIe pad: +- + +Required properties: +- clocks: Must contain an entry for each entry in clock-names. +- clock-names: Must contain the following entries: + - "pll": phandle and specifier referring to the PLLE +- resets: Must contain an entry for each entry in reset-names. +- reset-names: Must contain the following entries: + - "phy": reset for the PCIe UPHY block I don't recall any clocks or resets properties in the pads for Tegra124. Do we really not need any? Tegra124 had two instances of what used to be called IOPHY, one for PCIe and one for SATA. On Tegra210 these have been replaced by two instances of what's called UPHY. The resets listed in the PCIe and SATA pad nodes are wired to those UPHY instances, hence why they are new on Tegra210. For Tegra124 no resets exist for the IOPHY instances. OK. I wonder if renaming the section title from "PCIe pad" to "Tegra210 PCIe pad" would be helpful; it'd certainly allow the reader to more quickly work out what part of the document they were looking at if jumping around in it. +SATA pad: +- + +Required properties: +- resets: Must contain an entry for each entry in reset-names. +- reset-names: Must contain the following entries: + - "phy": reset for the SATA UPHY block + PHY nodes: Nit: 2 blank lines there. Those were intentional for additional spacing between sections. That seems inconsistent, and not something I recall seeing before, so I'm not sure anyone would realize that. Better to do it with more explicit section names I think. +For Tegra210, the list of valid PHY nodes is given below: +- utmi: utmi-0, utmi-1, utmi-2, utmi-3 + - functions: "snps", "xusb", "uart" +- hsic: hsic-0, hsic-1 + - functions: "snps", "xusb" +- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, pcie-5, pcie-6 + - functions: "pcie-x1", "usb3-ss", "pcie-x4" +- sata: sata-0 + - functions: "usb3-ss", "sata" usb2-bias also needs to be present. I'm not sure about this. All of the driver code that I've looked deals with the usb2-bias pad internally. As far as I can tell, this pad needs to be configured to whatever any of the other pads is configured for. I think that means if any of the UTMI pads is configured for XUSB then the usb2-bias pad must also be configured for XUSB. Which would also imply that if one of the UTMI pads is configured for XUSB, all of them must be configured for XUSB. I don't believe that's true; on Tegra210 I have successfully configured the (legacy) "USB2 controller" to drive the recovery/micro-USB board-level port, and the "XUSB controller" (USB2 and USB3 ports thereof) to drive a couple of other board-level ports. It's also not a pad in the sense that the others are pads. It doesn't directly connect anywhere. It's also shared by all the UTMI pads. That said, there are two registers that allow some of the parameters of the pad to be set, so perhaps adding the node exclusively for configurability might make sense. It wouldn't really be a PHY node, though, so wouldn't fit into the above group. Perhaps something like the following could be added: There is an additional pad that is used to support the bias voltages to the USB2/UTMI pads. This is not a PHY that can be consumed by any I/O controller, but the device tree node can be used to specify parameters needed for the programming of the pad. I think the function shouldn't necessarily be exposed as a parameter, because all that would do is add the possibility for a conflicting set of mux options with the USB2/UTMI pads. OK, if we can come up with a well-described algorithm re: how/when to program/enable this pad, then we can probably represent this differently than the other pads. I might expect DT to contain values for HS_DISCON_LEVEL HS_SQUELCH_LEVEL, although I can't recall if those values are SoC- or board-specific off the top of my head. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding
On 11/13/2015 09:11 AM, Thierry Reding wrote: On Wed, Nov 04, 2015 at 01:54:15PM -0700, Stephen Warren wrote: On 11/04/2015 10:11 AM, Thierry Reding wrote: From: Thierry Reding The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a set of lanes that are used for PCIe, SATA and USB. .../bindings/phy/nvidia,tegra-xusb-padctl.txt | 359 + For Tegra bindings, we usually use the full compatible value as the filename, so I'd expect the chip number in the filename too. I specifically didn't do that to avoid confusion. The XUSB pad controller was introduced on Tegra114, but patches weren't posted until Tegra124. So nvidia,tegra114-xusb-padctl.txt would be the proper name for this binding if we were following the conventions, but then we have never specified that binding (though I think it would look mostly the same as for Tegra124). I can of course still go for nvidia,tegra114-xusb-padctl.txt, the content would explicitly list valid compatible strings. It's just that none of them would match the filename. I was asking that the file be named to match the compatible flag, not the SoC version that contains the HW module. The first/oldest compatible value currently documented is Tegra124, so I'd expect that to appear in the filename. Yes, it's theoretically possible for the binding to be enhanced in the future to cover Tegra114, and at that time we'd probably want to rename the file. However, I believe the likelihood of that happening is zero (or even negative, which is admittedly mathematically impossible, but I'm being hyperbolic). I'd expect to see a patch in this series that edits the existing Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt and mentions that the binding is deprecated. How about this: --- >8 --- diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt index 30676ded85bb..77dfba05ccfd 100644 --- a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt @@ -1,6 +1,11 @@ Device tree binding for NVIDIA Tegra XUSB pad controller +NOTE: It turns out that this binding isn't an accurate description of the XUSB +pad controller. While the description is good enough for the functional subset +required for PCIe and SATA, it lacks the flexibility to represent the features +needed for USB. For the new binding, see ../phy/nvidia,tegra-xusb-padctl.txt. + The Tegra XUSB pad controller manages a set of lanes, each of which can be assigned to one out of a set of different pads. Some of these pads have an associated PHY that must be powered up before the pad can be used. --- >8 --- That sounds good, although I'd suggest explicitly mentioning that the binding is deprecated. Perhaps add ", and so this binding is deprecated" at the end of the first sentence? diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt +- reg: Physical base address and length of the controller's registers. +- resets: Must contain an entry for each entry in reset-names. +- reset-names: Must include the following entries: + - "padctl" Are there no clocks or power domains that affect XUSB padctl? I suppose we can start off without any, and add them later if we need. I don't think there are any. The TRM specifies that it's in the ungated Vaux_soc power partition, and doesn't mention any clocks. OK. Obviously the module must used some clock, since it contains clocked logic. However, equally that clock is obviously on even without the driver explicitly managing it since the HW works right now. So, I suppose we can defer adding any clock entries to the binding/DT until later, if the need arises. It'd still be nice if we could put the complete details into the binding from day one, but I understand that the documentation isn't exactly forthcoming on these topics. +- mboxes: Must contain an entry for each entry in mbox-names. +- mbox-names: Must include the following entries: + - "xusb" I thought we'd decided not to use any mbox binding or drivers in XUSB now? See for example my proposed XUSB controller binding at: http://www.spinics.net/lists/linux-tegra/msg23922.html [PATCH V9] dt: add NVIDIA Tegra XUSB controller binding The idea is that the mailbox should be entirely internal to the XUSB controller driver, and if the receipt of a mailbox message requires any change in the XUSB pad controller programming, the XUSB controller driver should simply call the XUSB pad controller driver to perform that operation. Okay, I think that should
Re: [PATCH v4] Tegra: DT: add device tree binding doc for QSPI
On 11/09/2015 05:45 AM, Thierry Reding wrote: On Mon, Oct 26, 2015 at 05:02:53PM -0600, Stephen Warren wrote: On 10/26/2015 04:22 PM, Tom Warren wrote: This patch adds the device tree binding doc for the Tegra QSPI controller on Tegra210. Acked-by: Stephen Warren (I assume if others approve the binding, Thierry will take it through the Tegra tree) Do we have a kernel driver and hardware where this can be tested? I'm slightly reluctant to merge a binding through the Tegra tree without a driver that implements it. AFAIK, the only HW this can be tested on is a one-off modified version of p2571. The standard boards don't have this. I don't believe there's an (upstream at least) kernel driver for this. I'm sure downstream has a driver. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support
On 11/04/2015 10:11 AM, Thierry Reding wrote: From: Thierry Reding Extend the binding to cover the set of feature found in Tegra210. diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt +PCIe pad: +- + +Required properties: +- clocks: Must contain an entry for each entry in clock-names. +- clock-names: Must contain the following entries: + - "pll": phandle and specifier referring to the PLLE +- resets: Must contain an entry for each entry in reset-names. +- reset-names: Must contain the following entries: + - "phy": reset for the PCIe UPHY block I don't recall any clocks or resets properties in the pads for Tegra124. Do we really not need any? +SATA pad: +- + +Required properties: +- resets: Must contain an entry for each entry in reset-names. +- reset-names: Must contain the following entries: + - "phy": reset for the SATA UPHY block + PHY nodes: Nit: 2 blank lines there. +For Tegra210, the list of valid PHY nodes is given below: +- utmi: utmi-0, utmi-1, utmi-2, utmi-3 + - functions: "snps", "xusb", "uart" +- hsic: hsic-0, hsic-1 + - functions: "snps", "xusb" +- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, pcie-5, pcie-6 + - functions: "pcie-x1", "usb3-ss", "pcie-x4" +- sata: sata-0 + - functions: "usb3-ss", "sata" usb2-bias also needs to be present. + + Port nodes: === Nit: 2 blank lines there. +For Tegra210, the XUSB pad controller exposes the following ports: +- 4x UTMI: utmi-0, utmi-1, utmi-2, utmi-3 +- 2x HSIC: hsic-0, hsic-1 +- 4x super-speed USB: usb3-0, usb3-1, usb3-2, usb3-3 + Examples: = Nit: 2 blank lines there. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding
On 11/04/2015 10:11 AM, Thierry Reding wrote: From: Thierry Reding The NVIDIA Tegra XUSB pad controller provides a set of pads, each with a set of lanes that are used for PCIe, SATA and USB. .../bindings/phy/nvidia,tegra-xusb-padctl.txt | 359 + For Tegra bindings, we usually use the full compatible value as the filename, so I'd expect the chip number in the filename too. I'd expect to see a patch in this series that edits the existing Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt and mentions that the binding is deprecated. diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt +Device tree binding for NVIDIA Tegra XUSB pad controller + + +The Tegra XUSB pad controller manages a set of pads, each of which controls +one or more lanes. The term "pad" usually refers to a single pad/pin/ball/signal on the chip. As such, saying "pad" here for something that's more than one pin is a bit confusing, even if that is what our HW documentation calls it. Each lane can in turn be assigned to one out of a set of +different outputs. That doesn't sound correct. That phrasing implies that the mux is between the HW-blocks-known-as-pads and the "outputs", whereas the mux is actually between the IO controllers and the HW-blocks-known-as-pads > A pad contains logic common for all its lanes. Each lane +can additionally be separately configured and powered up. I'd suggest rephrasing that all as: The Tegra XUSB pad controller manages a set of IO lanes (differential signals) which connect directly to pins/pads on the SoC package. Each lane is controlled by a HW block referred to as a "pad" in the Tegra HW documentation. Each such "pad" may control either one or multiple lanes, and contains any logic common to all its lanes. Each lane can be separately configured and powered up. +Some of the lanes are high-speed lanes, which can be used for PCIe, SATA or +super-speed USB. Other lanes are for various types of low-speed, full-speed +or high-speed USB (such as UTMI, ULPI and HSIC). Perhaps add the following after that? The XUSB pad controller contains a software-configurable mux that sits between the IO controller ports (e.g. PCIe) and the lanes. +Required properties: + +- compatible: Must be: + - "nvidia,tegra124-xusb-padctl": for Tegra124 and Tegra132 For Tegra132, we need both a "tegra124" and a "tegra132 value". I would suggest listing the valid complete property values for each SoC for simplicity and preciseness, as I did in the XUSB controller binding proposal I link to later: - compatible: Valid options are: - Tegra124: "nvidia,tegra124-xusb-padctl". - Tegra132: "nvidia,tegra132-xusb-padctl", \ "nvidia,tegra124-xusb-padctl". This also makes it very easy to add entries for future SoCs without editing the diff touching any existing lines of text. +- reg: Physical base address and length of the controller's registers. +- resets: Must contain an entry for each entry in reset-names. +- reset-names: Must include the following entries: + - "padctl" Are there no clocks or power domains that affect XUSB padctl? I suppose we can start off without any, and add them later if we need. +- mboxes: Must contain an entry for each entry in mbox-names. +- mbox-names: Must include the following entries: + - "xusb" I thought we'd decided not to use any mbox binding or drivers in XUSB now? See for example my proposed XUSB controller binding at: http://www.spinics.net/lists/linux-tegra/msg23922.html [PATCH V9] dt: add NVIDIA Tegra XUSB controller binding The idea is that the mailbox should be entirely internal to the XUSB controller driver, and if the receipt of a mailbox message requires any change in the XUSB pad controller programming, the XUSB controller driver should simply call the XUSB pad controller driver to perform that operation. +Pad nodes: +== +For Tegra124 and Tegra132, the following pads exist: utmi, ulpi, hsic, pcie +and sata. No extra resources are required for operation of these pads. Judging by the diagram in the TRM (e.g. figure 41 in the Tegra124 TRM, figure 36 in the Tegra210 TRM), there is not a single "utmi" pad, but rather a separate pad per USB2 lane. However, the other types of pads are indeed multi-lane. The TRM also refers to "USB2" pads rather than "UTMI" pads. I don't see a ULPI pad in the diagram either. Assuming the diagram in the TRM is consistent with the register layout, I think we should have the following set of pad nodes: utmi-0 utmi-1 utmi-2 hsic pcie sata +Required properties: + +For Tegra124 and Tegra132, the list of valid PHY nodes is given below: +- utmi: utmi-0, utmi-1, utmi-2 + - functions: "snps", "xusb", "uart"
Re: [RFC] rpi: add support to enable usb power domain
On 10/28/2015 02:40 PM, Alexander Aring wrote:
> This patch adds support for RPi several Power Domains and enable support
> to enable the USB Power Domain when it's not enabled before.
>
> This patch based on Eric Anholt's patch to support Power Domains. He had
> an issue about -EPROBE_DEFER inside the power domain subsystem, this
> issue was solved by commit <311fa6a> ("PM / Domains: Return -EPROBE_DEFER
> if we fail to init or turn-on domain").
> diff --git
> a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.txt
> b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.txt
> firmware {
> compatible = "raspberrypi,bcm2835-firmware";
> mboxes = <&mailbox>;
> + #power-domain-cells = <1>;
> +};
I would have expected a separate DT node for the power domains driver
that referenced the firmware node by phandle. I believe that's why the
firmware node exports mailboxes to other drivers. If the firmware driver
was going to implement all the features directly, it wouldn't need to
act as a mailbox provider, since all the mailbox programming would be
internal.
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> +#define RPI_POWER_DOMAIN(_domain, _name) \
> + [_domain] = \
> + { \
I'd expect { wrapped onto the previous line.
> +static int raspberrypi_firmware_set_power(struct rpi_firmware *fw,
> + u32 domain, bool on)
> + packet.on = on;
> + ret = rpi_firmware_property(fw, RPI_FIRMWARE_SET_POWER_STATE, &packet,
> + sizeof(packet));
> + if (!ret && !packet.on)
> + ret = -EINVAL;
The error is only reported for power off requests?
> +/* Asks the firmware to if power is on for a specific power domain. */
> +static int raspberrypi_firmware_power_is_on(struct rpi_firmware *fw,
> + u32 domain)
> + packet.domain = domain;
> + ret = rpi_firmware_property(fw, RPI_FIRMWARE_GET_POWER_STATE, &packet,
> + sizeof(packet));
> + if (ret < 0)
> + return ret;
Hmm. If rpi_firmware_property() returns <0 on error, I'm confused what
the test I commented on above is intended to do.
> +/*
> + * IMPORTANT: be sure this array has no entries which are not specified
> + * between others by RPI_POWER_DOMAIN, otherwise mapping between
> + * generic_pm_domain array doesn't work anymore.
> + */
"has no entries which are not specified between others by
RPI_POWER_DOMAIN" might be better phrased as "is contiguous" or
"contains only contiguous entries".
> @@ -208,15 +312,44 @@ static int rpi_firmware_probe(struct platform_device
> *pdev)
> + for (i = 0; i < num_domains; i++) {
> + bool is_off;
> +
> + rpi_power_domains[i].fw = fw;
> + power_domains[i] = &rpi_power_domains[i].base;
> +
> + /* get the initial state */
> + ret = raspberrypi_firmware_power_is_on(fw, i);
> + if (ret < 0)
> + goto mbox;
The label name "mbox" doesn't give a clue that it's an error handler.
"free_mbox" might be better.
> +mbox:
> + mbox_free_channel(fw->chan);
> + return ret;
> }
Does the pm_genpd_init() call for all the power domains need to be
undone at all?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] Tegra: DT: add device tree binding doc for QSPI
On 10/26/2015 04:22 PM, Tom Warren wrote: This patch adds the device tree binding doc for the Tegra QSPI controller on Tegra210. Acked-by: Stephen Warren (I assume if others approve the binding, Thierry will take it through the Tegra tree) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFT] ARM: bcm2835: enable all bcm2835-relevant in defconfig
On 10/21/2015 10:16 AM, Stefan Wahren wrote: Am 21.10.2015 um 04:32 schrieb Stephen Warren: On 10/15/2015 02:47 PM, Stefan Wahren wrote: Rebuild bcm2835_defconfig using "make bcm2835_defconfig; make savedefconfig", and add the following features: * Enable all bcm2835-relevant drivers (MBOX, WDT, DMA, PWM, SND) * Re-enable some features to keep the current settings (stackprotector, LED GPIO, LED triggers) Can you explain that second bullet a bit more? Yes, the last update to bcm2835_defconfig was on 25th September 2014. So a little bit changed in the configs like renaming of options. After saving the defconfig i noticed that we "lost" 2 features with the new defconfig: * CONFIG_CC_STACKPROTECTOR because of renaming to CONFIG_CC_STACKPROTECTOR_REGULAR * CONFIG_LEDS_TRIGGER_HEARTBEAT because of the following new dependencies * CONFIG_NEW_LEDS * CONFIG_LEDS_CLASS * CONFIG_LEDS_TRIGGERS Ah OK. It'd be nice to have that in the patch description. Do you want 2 separate patches (first update with re-enable and second with new bcm2835 stuff)? It's probably fine as a single patch so long as all the changes (and reason why they're made) are described there. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bcm2835: Add Raspberry Pi thermal sensor to the device tree
On 10/11/2015 01:48 PM, Lubomir Rintel wrote: > Driven via the Raspberry Pi VideoCore 4 firmware interface. > .../bindings/thermal/raspberrypi,bcm2835-thermal.txt| 13 > + > arch/arm/boot/dts/bcm2835-rpi.dtsi | 5 + There should be a separate patch for the DT binding and DT additions, but they can be in a series. As before, CC at least the DT binding to the DT binding maintainers. > diff --git > a/Documentation/devicetree/bindings/thermal/raspberrypi,bcm2835-thermal.txt > b/Documentation/devicetree/bindings/thermal/raspberrypi,bcm2835-thermal.txt > +Raspberry Pi Broadcom BCM2835 thermal control > + > +Required properties: This needs some more explanation of what this is intended to represent and do. For example, what services is this node (the driver instantiated by this node) expected to implement, and what firmware services is it expected to rely upon? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFT] ARM: bcm2835: dt: fix memory of Raspberry Pi B+
On 10/20/2015 02:53 AM, Eric Anholt wrote:
> Stefan Wahren writes:
>
>> Since the Raspberry Pi models differ in memory amount we better
>> define it at board level. After that we are able to fix the
>> memory node of the Raspberry Pi B+ .
>> diff --git a/arch/arm/boot/dts/bcm2835-rpi-a-plus.dts
>> b/arch/arm/boot/dts/bcm2835-rpi-a-plus.dts
>> +memory { + reg = <0 0x1000>; /* 256 MB */ +}; +
>
> Seems like a good idea. The closed firmware passes us an edited
> devicetree with good memory fields, but I don't think U-Boot is up
> to it if you chainload it.
U-Boot fills in the memory size in DT correctly too.
I have no objection to the patch since it looks correct at a very
brief glance, but I think it will have zero practical effect.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFT] ARM: bcm2835: enable all bcm2835-relevant in defconfig
On 10/15/2015 02:47 PM, Stefan Wahren wrote: > Rebuild bcm2835_defconfig using "make bcm2835_defconfig; > make savedefconfig", and add the following features: > > * Enable all bcm2835-relevant drivers (MBOX, WDT, DMA, > PWM, SND) > * Re-enable some features to keep the current settings > (stackprotector, LED GPIO, LED triggers) Can you explain that second bullet a bit more? When regenerating defconfig files, it is quite common for entries not related to your changes to appear or disappear due to other changes in the kernel or some of the remove options being selected by some of the new options you enabled. To check what's going on, I usually do the following when editing defconfig: 1) Rebuild bcm2835_defconfig without editing the .config file at all (make bcm2835_defconfig; make savedefconfig; mv defconfig arch/arm/configs/bcm2835_defconfig; git commit). This allows me to see all the unrelated changes that will happen simply due to rebuilding the defconfig. You should double-check these, but likely ignore them. 2) Now edit the .config (e.g. make menuconfig) and re-generate the defconfig and commit. This change should now only include changes that are a direct result of your .config edits. To submit the patch, I often squash the two together after the separate validation. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] ARM: bcm2835: dt: Add Raspberry Pi Model A+
On 10/11/2015 01:37 PM, Lubomir Rintel wrote:
> Essentially the same as B+.
(It'd be good practice to CC RPi patches to the ARM kernel mailing list too)
> diff --git a/arch/arm/boot/dts/bcm2835-rpi-a-plus.dts
> b/arch/arm/boot/dts/bcm2835-rpi-a-plus.dts
> + leds {
> + act {
> + gpios = <&gpio 47 0>;
> + };
> +
> + pwr {
> + label = "PWR";
> + gpios = <&gpio 35 0>;
> + default-state = "keep";
> + linux,default-trigger = "default-on";
> + };
> + };
Since the A+ and B+ are so similar, I wonder if it makes sense to
sometime create a shared "plus" .dtsi file for the shared parts?
Same for the I2S below.
Perhaps the DTs are so simple that it's a waste of complexity to do that
though.
Out of curiosity, are you planning on creating DT files for all the
possible options:
"bcm2835-rpi-a.dtb",
"bcm2835-rpi-a-plus.dtb",
"bcm2835-rpi-b.dtb",
"bcm2835-rpi-b-i2c0.dtb",
"bcm2835-rpi-b-plus.dtb",
"bcm2835-rpi-b-rev2.dtb",
"bcm2835-rpi-cm.dtb",
"bcm2836-rpi-2-b.dtb",
(list taken from U-Boot's board/raspberrypi/rpi/rpi.c)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bcm2835 (Raspberry Pi) KMS driver
On 10/11/2015 06:39 AM, Stefan Wahren wrote: Am 09.10.2015 um 23:27 schrieb Eric Anholt: This is a respin of the Raspberry Pi KMS series. Now that we've got a real clock driver, I can actually set new video modes. Also in this version, most of the custom DT stuff from before is gone, thanks to finding exynos's platform_driver component matching code (I have sent separate patches to drivers/base to make helpers for doing it). https://github.com/anholt/linux/tree/vc4-kms-squash-2 I want to point out that git format-patch could prepare a nice cover letter and usually the changelog should go there. Well, I guess you could put it there, but that wouldn't remove the need to put the changelog in the individual patches too, so that reviewers don't have to switch back and forth between different messages just to find out what changed in each patch. +1 on sending the cover letter using git format-patch/send-email thoughl; the threading here is a little odd. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: bcm2835: add label for uart0
On 10/06/2015 03:53 PM, Eric Anholt wrote:
> Stefan Wahren writes:
>
>> This patch adds a label for uart0 to allow changing of uart0
>> pins.
>>
>> Signed-off-by: Stefan Wahren
>
> This patch seems innocuous, but could you clarify for me how
> exactly you change the uart0 pins, and why one would do that?
I /assume/ this is so that some other DT file (that includes the
edited file) can add some pinctrl-related properties to this DT node,
using syntax such as:
&uart0 {
new content;
};
If so, the patch,
Acked-by: Stephen Warren
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V9] dt: add NVIDIA Tegra XUSB controller binding
From: Stephen Warren
Add device-tree binding documentation for the XUSB (xHCI) controller
present on Tegra124 and later SoCs.
Signed-off-by: Andrew Bresticker
[swarren, combined separate MFD, mailbox, XHCI bindings into one node]
Signed-off-by: Stephen Warren
Cc: Rob Herring
Cc: Pawel Moll
Cc: Mark Rutland
Cc: Ian Campbell
Cc: Kumar Gala
Cc: Mathias Nyman
Cc: Greg Kroah-Hartman
---
Changes from v8:
- Combined the separate MFD, mailbox, and XHCI bindings into one; there
is a single HW module, and so there should be a single DT node to
represent it. Any Linux-internal driver structure should not influence
the binding structure. This included squashing the various reg and
interrupt resources that were previously in the separate modules into
one list.
- Used lists to document the compatible, reg, and interrupts properties.
- Renamed the primary binding from xhci to xusb to reflect the name of
the overall module.
Changes from v7:
- Added back non-shared reg/interrupts properties.
Changes from v6:
- Removed XUSB_DEV related clocks/resets. They will be consumed by
a separate driver and binding.
- Removed reg/interrupts properties.
No changes from v5.
Changes from v4:
- Updated regulator names, as suggested by Thierry.
No changes from v3.
Changes from v2:
- Added mbox-names property.
Changes from v1:
- Updated to use common mailbox bindings.
- Added remaining XUSB-related clocks and resets.
- Updated list of power supplies to be more accurate wrt to the hardware.
---
.../bindings/usb/nvidia,tegra124-xusb.txt | 96 ++
1 file changed, 96 insertions(+)
create mode 100644
Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt
diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt
b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt
new file mode 100644
index ..f8de8d49602e
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xusb.txt
@@ -0,0 +1,96 @@
+NVIDIA Tegra XUSB (XHCI) controller
+===
+
+The Tegra XUSB controller supports both USB2 and USB3 interfaces exposed
+by the Tegra XUSB pad controller.
+
+Required properties:
+
+ - compatible: Valid options are:
+- Tegra124: "nvidia,tegra124-xusb".
+- Tegra132: "nvidia,tegra132-xusb", "nvidia,tegra124-xusb".
+ - reg: Must contain the following entries, in the following order:
+- The xHCI host registers.
+- The IPFS registers.
+- The FPCI registers.
+ - interrupts: Must contain the following entries, in the following order:
+- The xHCI host interrupt
+- The XUSB mailbox interrupt.
+ - clocks: Must contain an entry for each entry in clock-names.
+ See ../clock/clock-bindings.txt for details.
+ - clock-names: Must include the following entries:
+- xusb_host
+- xusb_host_src
+- xusb_falcon_src
+- xusb_ss
+- xusb_ss_src
+- xusb_ss_div2
+- xusb_hs_src
+- xusb_fs_src
+- pll_u_480m
+- clk_m
+- pll_e
+ - resets: Must contain an entry for each entry in reset-names.
+ See ../reset/reset.txt for details.
+ - reset-names: Must include the following entries:
+ - xusb_host
+ - xusb_ss
+ - xusb_src
+ Note that xusb_src is the shared reset for xusb_{ss,hs,fs,falcon,host}_src.
+ - avddio-pex-supply: PCIe/USB3 analog logic power supply. Must supply 1.05V.
+ - dvddio-pex-supply: PCIe/USB3 digital logic power supply. Must supply 1.05V.
+ - avdd-usb-supply: USB controller power supply. Must supply 3.3V.
+ - avdd-pll-utmip-supply: UTMI PLL power supply. Must supply 1.8V.
+ - avdd-pll-erefe-supply: PLLE reference PLL power supply. Must supply 1.05V.
+ - avdd-usb-ss-pll-supply: PCIe/USB3 PLL power supply. Must supply 1.05V.
+ - hvdd-usb-ss-supply: High-voltage PCIe/USB3 power supply. Must supply 3.3V.
+ - hvdd-usb-ss-pll-e-supply: High-voltage PLLE power supply. Must supply 3.3V.
+
+Optional properties:
+
+ - phys: Must contain an entry for each entry in phy-names.
+ See ../phy/phy-bindings.txt for details.
+ - phy-names: Should include an entry for each PHY used by the controller.
+ Names must be of the form "-" where is one of "utmi",
+ "hsic", or "usb3" and is a 0-based index. On Tegra124, there may
+ be up to 3 UTMI, 2 HSIC, and 2 USB3 PHYs.
+
+Example:
+
+ usb-host@0,7009 {
+ compatible = "nvidia,tegra124-xusb";
+ reg = <0x0 0x7009 0x0 0x8000>,
+ <0x0 0x70099000 0x0 0x1000>,
+ <0x0 0x70098000 0x0 0x1000>;
+ interrupts = ,
+;
+ clocks = <&tegra_car TEGRA124_CLK_XUSB_HOST>,
+<&tegra_car TEGRA124_CLK_XUSB_HOST_SRC>,
+<&tegra_car TEGRA124_CLK_XUSB_FALCON_SRC
[PATCH 2/2] dt: update Tegra PCIe binding for Tegra210
From: Stephen Warren Reword the description of the ranges property so it is correct irrespective of how many #address-cells the PCI node's parent uses. Be more explicit about the valid values for the compatible property, and in particular point out that Tegra210 isn't fully backwards-compatible due to the introduction of some HW bugs whose workarounds are not present in drivers written solely for previous chips. with Tegra124, Still "TODO" is to fill in a complete "Power supplies for Tegra210" section. My main use-case for the binding is U-Boot, which doesn't use regulator bindings at present, so I have not yet researched this aspect of the hardware. Signed-off-by: Stephen Warren --- .../bindings/pci/nvidia,tegra20-pcie.txt | 27 +++--- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt index 75321ae23c08..3d92934a079c 100644 --- a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt +++ b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt @@ -1,10 +1,15 @@ NVIDIA Tegra PCIe controller Required properties: -- compatible: For Tegra20, must contain "nvidia,tegra20-pcie". For Tegra30, - "nvidia,tegra30-pcie". For Tegra124, must contain "nvidia,tegra124-pcie". - Otherwise, must contain "nvidia,-pcie", plus one of the above, where - is tegra132 or tegra210. +- compatible: Valid options are: + Tegra20: "nvidia,tegra20-pcie". + Tegra30: "nvidia,tegra30-pcie". + Tegra124: "nvidia,tegra124-pcie". + Tegra132: "nvidia,tegra132-pcie", "nvidia,tegra124-pcie". + Tegra210: "nvidia,tegra210-pcie". +Note that Tegra210 is not backwards-compatible with Tegra124 due to the +introduction of some HW bugs whose workarounds are not present in drivers +written solely for previous chips. - device_type: Must be "pci" - reg: A list of physical base address and length for each set of controller registers. Must contain an entry for each entry in the reg-names property. @@ -27,10 +32,16 @@ Required properties: CPU address space - #size-cells: Size representation for root ports (must be 2) - ranges: Describes the translation of addresses for root ports and standard - PCI regions. The entries must be 6 cells each, where the first three cells - correspond to the address as described for the #address-cells property - above, the fourth cell is the physical CPU address to translate to and the - fifth and six cells are as described for the #size-cells property above. + PCI regions. The entries must be (na_pcie + na_parent + ns_pcie) cells each, + where: +na_pcie refers to #address-cells in the PCIe controller, +na_parent refers to #address-cells in the PCIe controller's parent node, +ns_pcie refers to #size-cells in the PCIe controller, + The first na_pcie cells correspond to the address as described for the + #address-cells property. The next na_parent cells contain the physical CPU + address to translate to and the final ns_pcie cells are as described for the + #size-cells property above. + Multiple entries must be present: - The first two entries are expected to translate the addresses for the root port registers, which are referenced by the assigned-addresses property of the root port nodes (see below). -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] dt: update Tegra XUSB padctl binding for Tegra210
From: Stephen Warren Tegra210 introduces some new options for pad muxing. Document these in the XUSB padctl binding. Be more explicit about the valid values for the compatible property, and in particular point out that Tegra210 isn't fully backwards-compatible with Tegra124, since some registers have moved about. Signed-off-by: Stephen Warren --- .../pinctrl/nvidia,tegra124-xusb-padctl.txt| 34 +++--- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt index 30676ded85bb..3952d893635c 100644 --- a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt @@ -13,9 +13,12 @@ how to describe and reference PHYs in device trees. Required properties: -- compatible: For Tegra124, must contain "nvidia,tegra124-xusb-padctl". - Otherwise, must contain '"nvidia,-xusb-padctl", - "nvidia-tegra124-xusb-padctl"', where is tegra132 or tegra210. +- compatible: Valid options are: + Tegra124: "nvidia,tegra124-xusb-padctl". + Tegra132: "nvidia,tegra132-xusb-padctl", "nvidia-tegra124-xusb-padctl". + Tegra210: "nvidia-tegra210-xusb-padctl". +Note that Tegra210 is not backwards-compatible with Tegra124 due to some +registers having been moved. - reg: Physical base address and length of the controller's registers. - resets: Must contain an entry for each entry in reset-names. See ../reset/reset.txt for details. @@ -45,18 +48,21 @@ Unspecified is represented as an absent property, and off/on are represented as integer values 0 and 1. Required properties: -- nvidia,lanes: An array of strings. Each string is the name of a lane. +- nvidia,lanes: An array of strings. Each string is the name of a lane (pad). + Valid values for lanes are listed below. Optional properties: -- nvidia,function: A string that is the name of the function (pad) that the - pin or group should be assigned to. Valid values for function names are - listed below. +- nvidia,function: A string that is the name of the function (IO controller) + that the pin or group should be assigned to. Valid values for function names + are listed below. - nvidia,iddq: Enables IDDQ mode of the lane. (0: no, 1: yes) Note that not all of these properties are valid for all lanes. Lanes can be divided into three groups: - - otg-0, otg-1, otg-2: + - otg-0, otg-1, otg-2, otg-3, usb2-bias: + +(otg-3, usb2-bias are valid on Tegra210 only.) Valid functions for this group are: "snps", "xusb", "uart", "rsvd". @@ -64,13 +70,21 @@ divided into three groups: - ulpi-0, hsic-0, hsic-1: +(ulpi-0 is valid on Tegra124 and Tegra132 only.) + Valid functions for this group are: "snps", "xusb". The nvidia,iddq property does not apply to this group. - - pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, sata-0: + - pcie-0, pcie-1, pcie-2, pcie-3, pcie-4, pcie-5, pcie-6, sata-0: + +(pcie-5, pcie-6 are valid on Tegra210 only.) + +On Tegra124, Tegra132, valid functions for this group are: "pcie", "usb3", +"sata", "rsvd". -Valid functions for this group are: "pcie", "usb3", "sata", "rsvd". +On Tegra210, valid functions for this group are "pcie-x1", "usb3", +"sata", "pcie-x4". Example: -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] dmaengine: tegra-adma: Add support for Tegra210 ADMA
On 09/29/2015 09:34 AM, Arnd Bergmann wrote: On Tuesday 29 September 2015 15:18:07 Jon Hunter wrote: On 29/09/15 13:39, Arnd Bergmann wrote: Ok, that makes more sense then. A few questions still: * Would the admaif in turn provide a #dma-cells and have the real slaves hooked up to it? I don't think that that would be necessary. If you look at the existing tegra i2s binding [0], there is a ahub-cif-ids property which maps the actual slave to the apbif (equivalent of the adma-if). This ID is used to get the appropriate dma channel for this client interface (cif). * How do you model the xbar in this scenario? The xbar is common to existing tegra devices and today is configured via the ahub-cif-ids property. I see, so instead of modeling the xbar as part of the DMA engine in DT, you model it as part of the slave. This probably adds a bit of complexity and a somewhat nonstandard binding, but it's too late to change that anyway. The XBAR shouldn't be modelled as part of the DMA engine; it's something entirely separate. The data flow (for TX) is: There's a FIFO in the ADMA-IF. This has a register address. That address can be written to by either PIO (CPU writes) or a DMA engine (in which case a "request selector" is used to communicate flow control information from the ADMI-IF to the DMA engine). Any DMA engine that may be used to write into this FIFO is an entirely separate HW module from the ADMA-IF itself. The ADMA-IF HW drains data from this FIFO and pushes it into the XBAR. ADMA-IF is an XBAR data source. The XBAR is a programmable cross-bar matrix. Many sinks are attached to it such as I2S, SPDIF, SRC (sample rate converters), mux/demux, etc. The data flow within the XBAR is purely in logic or internal RAM/flops. There is no transfer to/from DRAM within the XBAR. For each XBAR sink, the XBAR has registers to select which XBAR source provides data to it. Thus the ADMA-IF is a DMA sink, and XBAR is unrelated to DMA; it's simply part of the HW processing of the data once it gets into the ADMA-IF. The RX flow is essentially the same in reverse; the I2S RX being an XBAR source, and a second FIFO in the ADMA-IF being an XBAR sink. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] dmaengine: tegra-adma: Add support for Tegra210 ADMA
On 09/29/2015 06:18 AM, Jon Hunter wrote:
On 28/09/15 16:07, Arnd Bergmann wrote:
On Monday 28 September 2015 15:57:46 Jon Hunter wrote:
...
Yes that makes sense, but I think that I have confused matters here a
bit and not thought through this entirely. So while we could configure
an audio interface, such as i2s, to use any adma-if port and hence any
dma channel with the appropriate hardware request signal, the mapping
between the adma-if port and request signal is fixed. For example,
adma-if rx1 port uses adma request signal 1
adma-if rx2 port uses adma request signal 2
adma-if rx3 port uses adma request signal 3
...
adma-if rx10 port uses adma request signal 10
and
adma-if tx1 port uses adma request signal 1
adma-if tx2 port uses adma request signal 2
adma-if tx3 port uses adma request signal 3
...
adma-if tx10 port uses adma request signal 10
What is connected to these adma-if tx and rx ports who knows but I think
that is where I was going wrong and made this more complex than it
should have been. So I think that the adma binding should have
#dma-cells = 1 and the adma-if binding have the following ...
admaif@0x702d {
dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
...
dma-names = "rx1", "tx1", "rx2", "tx2",
...
};
Yes, that sounds about right.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] dmaengine: tegra-adma: Add support for Tegra210 ADMA
On 09/29/2015 02:13 AM, Jon Hunter wrote: On 28/09/15 17:36, Stephen Warren wrote: On 09/28/2015 08:57 AM, Jon Hunter wrote: On 25/09/15 16:47, Arnd Bergmann wrote: On Friday 25 September 2015 16:38:55 Jon Hunter wrote: On 25/09/15 16:17, Jon Hunter wrote: On 25/09/15 16:03, Arnd Bergmann wrote: On Friday 25 September 2015 15:56:40 Jon Hunter wrote: + case DMA_MEM_TO_DEV: + burst_size = fls(tdc->config.dst_maxburst); + ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1); + ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) | + ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id); + ch_regs->src_addr = buf_addr; + break; + + case DMA_DEV_TO_MEM: + burst_size = fls(tdc->config.src_maxburst); + ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1); + ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) | + ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id); + ch_regs->trg_addr = buf_addr; + break; Do not use the 'slave_id' field here to identify the slave device, that concept is broken. Instead, put the slave identification into the dma specifier in DT and read it out in your xlate function. Why is it broken? What happens if I don't know the slave-id? In other words, the slave-id can be dynamically allocated and associated with a given slave. I guess thinking about it some more, the driver could assign an id itself to a given channel and I could avoid using slave_id here. There are 22 channels and 10 tx and 10 rx requests. This sounds roughly right. So you could pick the ch_regs->ctrl value when you allocate the tegra_adma_chan structure, and then map it to the slave in the xlate() function. Actually, what I mentioned about would not work as it is not the DMA that should assign the requests to the channel. I think that I have poorly described the hardware and how it works, so let me try and explain a bit more. From a hardware perspective it looks like the following ... memory <-> adma <-> adma-if <-> xbar <-> clients where: - memory is the system memory - adma is the dma controller - adma-if is the dma interface to a crossbar - xbar is the crossbar - clients are various audio interfaces, such as i2s, etc The adma-if is essentially a mux with 10 tx and 10 rx ports. Any of the 22 adma channels can be mapped to any of the 10 tx or rx ports. The request signal used by the adma is determined by which port it is configured to use. However, what makes this even more configurable is the xbar is also a mux that can route adma-if ports to the various clients. So potentially, you could use any adma channel and any port to route audio to any of the clients. However, the adma-if needs to know which adma channel is mapped to which port It does? I'm pretty sure it didn't in earlier chips; what changed? I *believe* that T210 is the first one to have the ADMA controller where as previous chips used the APB-DMA controller. Yes, I believe that's true. Looking at the APB-DMA on T124 I can see that there is a fixed REQ_SEL value for each of the APBIF (equivalent of the ADMA-IF on T210). I believe that is still true on T210. I don't see any difference in the way request select values are used/configured/... The only difference is that we're using a different DMA engine, but that DMA engine is configured in the same way. For earlier chips, I believe all that's required is: When programming the DMA engine, you need to know which ADMA-IF is in use, so the correct DMA request selector can be programmed into the DMA engine for flow-control. ADMA-IF simply receives the data from DMA, and forwards it to the XBAR tagged with the ADMA-IF's own ID. The XBAR programming selects which data source (ADMA-IF TX, I2S RX, ...) each sink (ADMARX, I2S TX, ...) receives. Yes, exactly, this part sounds the same. However, just the ADMA itself allows for even more configuration. Ideally, when an I2S controller needs to start transmitting data, it should dynamically allocate an ADMA-IF, query it for its DMA slave request ID, and then forward this information to the ASoC code that sets up the DMA transfer. Agree. In practice, this means that since any I2S module could use any ADMA-IF, you probably need to list all DMA request selectors possible in the I2S's DMA-related properties, so it can choose which one to use. Possibly, but really I think that the i2s only cares about the ADMA-IF and the hardware request used by the ADMA can be abstracted by the ADMA-IF. In other words, if you allocate an ADMA channel to work with a specific ADMA-IF, then let the ADMA-IF select the hardware request because as long as one is available, you don't care which. Each ADMAIF (FIFO) has a single request select value statically assigned to it as far as I
Re: [PATCH 3/3] ARM: bcm2835: Add the auxiliary clocks to the device tree.
On 09/28/2015 01:26 PM, Eric Anholt wrote:
Stephen Warren writes:
On 09/10/2015 03:22 PM, Eric Anholt wrote:
These will be used for enabling UART1, SPI1, and SPI2.
diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
+ aux_clocks: aux-clocks@0x7e215004 {
+ compatible = "brcm,bcm2835-aux-clock";
+ #clock-cells = <1>;
+ reg = <0x7e215004 0x4>;
Actually, I take back the ack on this patch. This HW module has two
registers. The reg property should include both of those registers so
that if SW needs to start using the other register at some time in the
future, the entire set of registers is already represented in DT.
If I changed it to "reg = <0x7e215000 0x8>" and use a #define for the
clock register offset in patch 2/3, would I then have your ack?
I suspect the compatible should then be "brcm,bcm2835-aux" (or similar)
since the node would represent the entire aux module, not just the clock
gate feature of the module (assuming there are other features in the aux
module besides just the clock gate).
With the reg change and the compatible change if appropriate, you can
have my ack,
Acked-by: Stephen Warren
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] dmaengine: tegra-adma: Add support for Tegra210 ADMA
On 09/28/2015 08:57 AM, Jon Hunter wrote: On 25/09/15 16:47, Arnd Bergmann wrote: On Friday 25 September 2015 16:38:55 Jon Hunter wrote: On 25/09/15 16:17, Jon Hunter wrote: On 25/09/15 16:03, Arnd Bergmann wrote: On Friday 25 September 2015 15:56:40 Jon Hunter wrote: + case DMA_MEM_TO_DEV: + burst_size = fls(tdc->config.dst_maxburst); + ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1); + ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) | + ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id); + ch_regs->src_addr = buf_addr; + break; + + case DMA_DEV_TO_MEM: + burst_size = fls(tdc->config.src_maxburst); + ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1); + ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) | + ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id); + ch_regs->trg_addr = buf_addr; + break; Do not use the 'slave_id' field here to identify the slave device, that concept is broken. Instead, put the slave identification into the dma specifier in DT and read it out in your xlate function. Why is it broken? What happens if I don't know the slave-id? In other words, the slave-id can be dynamically allocated and associated with a given slave. I guess thinking about it some more, the driver could assign an id itself to a given channel and I could avoid using slave_id here. There are 22 channels and 10 tx and 10 rx requests. This sounds roughly right. So you could pick the ch_regs->ctrl value when you allocate the tegra_adma_chan structure, and then map it to the slave in the xlate() function. Actually, what I mentioned about would not work as it is not the DMA that should assign the requests to the channel. I think that I have poorly described the hardware and how it works, so let me try and explain a bit more. From a hardware perspective it looks like the following ... memory <-> adma <-> adma-if <-> xbar <-> clients where: - memory is the system memory - adma is the dma controller - adma-if is the dma interface to a crossbar - xbar is the crossbar - clients are various audio interfaces, such as i2s, etc The adma-if is essentially a mux with 10 tx and 10 rx ports. Any of the 22 adma channels can be mapped to any of the 10 tx or rx ports. The request signal used by the adma is determined by which port it is configured to use. However, what makes this even more configurable is the xbar is also a mux that can route adma-if ports to the various clients. So potentially, you could use any adma channel and any port to route audio to any of the clients. However, the adma-if needs to know which adma channel is mapped to which port It does? I'm pretty sure it didn't in earlier chips; what changed? For earlier chips, I believe all that's required is: When programming the DMA engine, you need to know which ADMA-IF is in use, so the correct DMA request selector can be programmed into the DMA engine for flow-control. ADMA-IF simply receives the data from DMA, and forwards it to the XBAR tagged with the ADMA-IF's own ID. The XBAR programming selects which data source (ADMA-IF TX, I2S RX, ...) each sink (ADMARX, I2S TX, ...) receives. Ideally, when an I2S controller needs to start transmitting data, it should dynamically allocate an ADMA-IF, query it for its DMA slave request ID, and then forward this information to the ASoC code that sets up the DMA transfer. In practice, this means that since any I2S module could use any ADMA-IF, you probably need to list all DMA request selectors possible in the I2S's DMA-related properties, so it can choose which one to use. Or perhaps the XBAR binding should list all the DMA requestors so that each I2S node doesn't have to. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] dmaengine: tegra-adma: Add support for Tegra210 ADMA
On 09/25/2015 09:38 AM, Jon Hunter wrote: On 25/09/15 16:17, Jon Hunter wrote: On 25/09/15 16:03, Arnd Bergmann wrote: On Friday 25 September 2015 15:56:40 Jon Hunter wrote: + case DMA_MEM_TO_DEV: + burst_size = fls(tdc->config.dst_maxburst); + ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1); + ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_MEM_TO_AHUB) | + ADMA_CH_CTRL_TX_REQ(tdc->config.slave_id); + ch_regs->src_addr = buf_addr; + break; + + case DMA_DEV_TO_MEM: + burst_size = fls(tdc->config.src_maxburst); + ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1); + ch_regs->ctrl = ADMA_CH_CTRL_XFER_DIR(ADMA_AHUB_TO_MEM) | + ADMA_CH_CTRL_RX_REQ(tdc->config.slave_id); + ch_regs->trg_addr = buf_addr; + break; Do not use the 'slave_id' field here to identify the slave device, that concept is broken. Instead, put the slave identification into the dma specifier in DT and read it out in your xlate function. Why is it broken? What happens if I don't know the slave-id? In other words, the slave-id can be dynamically allocated and associated with a given slave. I guess thinking about it some more, the driver could assign an id itself to a given channel and I could avoid using slave_id here. There are 22 channels and 10 tx and 10 rx requests. However, I could also statically assign a mapping in DT for the clients if that is preferred. The channel IDs should be dynamically assigned at run-time. DT should provide the slave ID (if there is one) for each client. The two concepts are completely orthogonal. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] serial: bcm2835: add auxiliary uart1 to device tree of bcm2835
On 09/23/2015 04:01 AM, Martin Sperl wrote:
On 22.09.2015 04:42, Stephen Warren wrote:
On 09/11/2015 05:20 AM, ker...@martin.sperl.org wrote:
From: Martin Sperl
Add the auxiliary uart1 device to the device tree of the bcm2835 SOC.
diff --git a/arch/arm/boot/dts/bcm2835.dtsi
b/arch/arm/boot/dts/bcm2835.dtsi
+uart1: uart@7e215040 {
+compatible = "ns16550";
compatible should always include a precise HW-specific value; something
like brcm,bcm2835-aux-uart. That way, if we find some other issue that
needs working around on this HW in the future, all DTs will already
contain the compatible value that SW needs in order to trigger that
workaround. That's a generally true statement; i.e. irrespective of
anything else in this series.
I don't believe "ns16550" should be in the compatible value for this
device, since the device cannot be successfully driven by SW that only
knows about a standard 16650 UART. SW must know about the different
divider, and there is no possibility of SW knowing about that before
this series.
OK - I will look into creating a separate driver then that uses the 8250
implementation as a basis...
I expect all you need to do is add a new PORT_* value to the existing
driver (which then drives some new quirk setting re: the clock rate),
and add new entry for the compatible value in of_platform_serial_table[]
in drivers/tty/serial/of_serial.c to select the correct TYPE_*.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] serial: bcm2835: add auxiliary uart1 to device tree of bcm2835
On 09/11/2015 05:20 AM, ker...@martin.sperl.org wrote:
> From: Martin Sperl
>
> Add the auxiliary uart1 device to the device tree of the bcm2835 SOC.
> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> + uart1: uart@7e215040 {
> + compatible = "ns16550";
compatible should always include a precise HW-specific value; something
like brcm,bcm2835-aux-uart. That way, if we find some other issue that
needs working around on this HW in the future, all DTs will already
contain the compatible value that SW needs in order to trigger that
workaround. That's a generally true statement; i.e. irrespective of
anything else in this series.
I don't believe "ns16550" should be in the compatible value for this
device, since the device cannot be successfully driven by SW that only
knows about a standard 16650 UART. SW must know about the different
divider, and there is no possibility of SW knowing about that before
this series.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] bcm2835: enable auxiliary uart1
On 09/14/2015 07:53 AM, Eric Anholt wrote: > ker...@martin.sperl.org writes: > >> From: Martin Sperl >> >> The bcm2835 SOC contains an auxiliary uart, which is very close >> to the ns16550 with some differences. >> >> The big difference is that the uart HW is not using an internal >> divider of 16 but 8, which results in an effictive baud-rate >> being twice the requested baud-rate. >> >> The bcm2835-aux-uart is also special in such that it is >> enabled/disabled by a gate in the clock, which is managed by the >> clk-bcm2835-aux clock driver. >> >> there are 2 options: * defining the clock-frequency property in >> the device tree to 500k instead of 250k, but this keeps the HW >> block disabled making the uart not work. Why does this keep the HW block disabled? >> * defining a clock in the device tree, but this results in a baud >> rate that is twice the requested baud-rate. >> >> To address this this patch-set introduce a new property in the >> device tree to define a clock divider other than 16. >> >> This currently just scales the clock by a factor of 16/divider. >> >> Note that the use of fixed-factor-clock has also been proposed as >> a workarround, but this does not really describe the hw in the >> device tree so another solution was needed that allows a correct >> representation of the HW in the device tree. > > I personally lean toward the fixed-factor-clock solution, but could > go either way. Serial maintainers, what do you think? The external fixed-factor-clock solution sounds more like a workaround than a real fix. It means that the UART driver isn't aware of what's going on and only "accidentally" works due to some manipulation of the clock values that it requests. That kind of thing almost always come back to bite you later. Rather than adding a DT property to configure the internal clock divider, it seems best to add a new compatible value to the list it supports, and have that compatible value set up internal data that indicates divide-by-16 vs. divide-by-8. After all, the HW isn't 100% compatible with ns16550, so the DT should not say that it is. While the clock-divider property this series adds to the DT does solve the issue, it does not prevent an older piece of SW that predates this series, and hence which does not implement clock-divider, attempting to bind to this DT but fail to operate correctly since it doesn't know about the different divider. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] dt/bindings: bcm2835: spi: add bindings for the bcm2835 auxiliary spi devices
On 09/11/2015 04:22 AM, ker...@martin.sperl.org wrote: > From: Martin Sperl > > This defines the spi1 and spi2 devices in the device-tree. Patches 1, 3, 4, Acked-by: Stephen Warren -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] ARM: bcm2835: Add the auxiliary clocks to the device tree.
On 09/10/2015 03:22 PM, Eric Anholt wrote:
> These will be used for enabling UART1, SPI1, and SPI2.
> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> + aux_clocks: aux-clocks@0x7e215004 {
> + compatible = "brcm,bcm2835-aux-clock";
> + #clock-cells = <1>;
> + reg = <0x7e215004 0x4>;
Actually, I take back the ack on this patch. This HW module has two
registers. The reg property should include both of those registers so
that if SW needs to start using the other register at some time in the
future, the entire set of registers is already represented in DT.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] ARM: bcm2835: Add the auxiliary clocks to the device tree.
On 09/10/2015 03:22 PM, Eric Anholt wrote: > These will be used for enabling UART1, SPI1, and SPI2. Patches 1, 3, Acked-by: Stephen Warren -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] clk: bcm2835: Add a driver for the auxiliary peripheral clock gates.
On 09/10/2015 03:22 PM, Eric Anholt wrote:
> There are a pair of SPI masters and a mini UART that were last minute
> additions. As a result, they didn't get integrated in the same way as
> the other gates off of the VPU clock in CPRMAN.
> diff --git a/drivers/clk/bcm/clk-bcm2835-aux.c
> b/drivers/clk/bcm/clk-bcm2835-aux.c
> +static int bcm2835_aux_clk_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct clk_onecell_data *onecell;
> + const char *parent;
> + struct clk *parent_clk;
> + void __iomem *reg;
> +
> + parent_clk = of_clk_get(dev->of_node, 0);
> + if (IS_ERR(parent_clk))
> + return PTR_ERR(parent_clk);
> + parent = __clk_get_name(parent_clk);
I think all the comments I made on probe() for the main bcm2835 clock
driver likely apply here too.
Also, is it "legal" for clock drivers to use __clk_get_name()? I thought
that was a clock core internal function, but may be wrong. I would have
expected to be able to pass a clock object when registering clocks
rather than a clock name, but oh well.
BTW, I like how this series shows how useful it is for someone with full
knowledge of the HW to come up with the DT bindings for a HW module;
once you know how the HW is actually designed, the correct binding ends
up being a lot easier to come up with, rather than guessing:-)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] ARM: bcm2835: Switch to using the new clock driver support.
On 09/10/2015 02:58 PM, Eric Anholt wrote: > This will give us the ability to set the pixel and HDMI state machine > clocks for the VC4 KMS driver, change the CPU frequency, and > potentially gate clocks in the future (once we also write a power > domain driver). It also gives the uart an explicit clock reference, > so that we don't need to change the physical addresses of the old > fixed clk_bcm2835.c clocks for Raspberry Pi 2 port. > > Two clocks get their frequencies updated as a result of this. One is > uart's apb_pclk, which was previously accidentally grabbing the fixed > uart0_pclk due to the apb_pclk not having clk_register_clkdev() > called. The uart doesn't seem to do anything with apb_pclk other than > make sure it's on, so that appears safe (also, as far as I can see, > the apb clock is actually the same as the VPU clock). The other is > EMMC, which according to the docs was supposed to be in the 50-100Mhz > range, but it turns out the firmware needed to change to running it at > the 250Mhz core clock speed to avoid a bug in clock domain crossing. > > Additionally, anything using BCM2835_CLOCK_VPU will now have a correct > clock rate if the user configures the boot-time core clock speed using > config.txt. Acked-by: Stephen Warren -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] clk: bcm2835: Add binding docs for the new platform clock driver.
On 09/10/2015 02:58 PM, Eric Anholt wrote: > Previously we've only supported a few fixed clocks based on > assumptions about how the firmware sets up the clocks, but this > binding will let us control the actual (audio power domain) clock > manager. Patches 1 and 2, Acked-by: Stephen Warren -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] clk: bcm2835: Add support for programming the audio domain clocks.
On 09/10/2015 02:58 PM, Eric Anholt wrote:
> This adds support for enabling, disabling, and setting the rate of the
> audio domain clocks. It will be necessary for setting the pixel clock
> for HDMI in the VC4 driver and let us write a cpufreq driver. It will
> also improve compatibility with user changes to the firmware's
> config.txt, since our previous fixed clocks are unaware of it.
>
> The firmware also has support for configuring the clocks through the
> mailbox channel, but the pixel clock setup by the firmware doesn't
> work, and it's Raspberry Pi specific anyway. The only conflicts we
> should have with the firmware would be if we made firmware calls that
> result in clock management (like opening firmware V3D or ISP access,
> which we don't support in upstream), or on hardware over-thermal or
> under-voltage (when the firmware would rewrite PLLB to take the ARM
> out of overclock). If that happens, our cached .recalc_rate() results
> would be incorrect, but that's no worse than our current state where
> we used fixed clocks.
>
> The existing fixed clocks in the code are left in place to provide
> backwards compatibility with old device tree files.
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> +/**
> + * DOC: BCM2835 CPRMAN (clock manager for the "audio" domain)
> + *
> + * The clock tree on the 2835 has several levels. There's a root
> + * oscillator running at 19.2Mhz. After the oscillator there are 4
Nit: s/4/5.
> +struct bcm2835_pll_data {
...
> + /* Highest rate for the VCO before we have to use the
> + * pre-divide-by-2.
> + */
Nit: /* should be on a line on its own I think. Similar elsewhere.
> +static const struct bcm2835_pll_ana_bits bcm2835_ana_default = {
> + 0,
> + 0,
> + ~((7 << 19) | (15 << 15)),
> + (2 << 19) | (8 << 15),
> + ~(7 << 7),
> + (6 << 1),
> +
> + 14
Nit: Elide the blank line? In some other structs too.
Are there #defines or names you can use for those initializers?
> +static int bcm2835_clk_probe(struct platform_device *pdev)
> + cprman = kzalloc(sizeof(*cprman), GFP_KERNEL);
> + if (!cprman)
> + return -ENOMEM;
This function allocates/initializes a lot of resources without using
devm_ versions of APIs.
> +
> + spin_lock_init(&cprman->regs_lock);
> + cprman->dev = &pdev->dev;
> + cprman->regs = of_iomap(dev->of_node, 0);
> + if (!cprman->regs)
> + return -ENODEV;
.. consequently if probe() fails later on (such as here) then resources
allocated earlier are not released. probe() should either switch to APIs
such as devm_kzalloc() or add some cleanup handling.
> + cprman->osc_name = of_clk_get_parent_name(dev->of_node, 0);
> + if (!cprman->osc_name)
> + return -ENODEV;
> +
> + platform_set_drvdata(pdev, cprman);
I'd expect that to happen right after allocating cprman, since it feels
related to having allocated the cprman object.
> + onecell = kmalloc(sizeof(*onecell), GFP_KERNEL);
> + if (!onecell)
> + return -ENOMEM;
> + onecell->clk_num = BCM2835_CLOCK_COUNT;
> + onecell->clks = kzalloc(sizeof(*onecell->clks) *
> + BCM2835_CLOCK_COUNT, GFP_KERNEL);
Does this need to be dynamically allocated? I think you can just make
these fields in struct bcm2835_cprman; even the array of clks could be:
struct clks clks[BCM2835_CLOCK_COUNT];
> + clks[BCM2835_PLLA] = bcm2835_register_pll(cprman, &bcm2835_plla_data);
> + clks[BCM2835_PLLB] = bcm2835_register_pll(cprman, &bcm2835_pllb_data);
These can fail; shouldn't there be error-checking? Or, does
of_clk_add_provider() check for this?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Potential issue with GPIO/IRQ flags
On 09/17/2015 11:21 AM, Andrew F. Davis wrote: > > > On 09/17/2015 12:20 PM, Rob Herring wrote: >> On Thu, Sep 17, 2015 at 10:53 AM, Andrew F. Davis wrote: >>> On 09/16/2015 08:26 PM, Rob Herring wrote: On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis wrote: > > Hello all, > > I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are > incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined > in: > > include/dt-bindings/gpio/gpio.h > > and are used to describe GPIO pins. IRQ types are defined in: > > include/dt-bindings/interrupt-controller/irq.h > > and are flags for IRQ pins. It is perfectly valid for the meaning of the field to be defined by the interrupt controller, and gpio interrupts could do something different. We've tried to standardize this though. >>> >>> Sure, but in this case these are not what the interrupt controller >>> is expecting. >> >> Understood. I was talking generally, not this specific case. >> > These seem to have been mixed up in a few places, take for example: > arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the > correct usage, but just before on line 1384 we see the issue. > GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If > this IRQ was not hard-coded with the correct edge in the driver > this would not work. What the author probably wanted was > IRQ_TYPE_LEVEL_HIGH. > > Now lets look at commit c21e678b256b, in this the IRQ flags did not > matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this > patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW > instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined > as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the > equivalent to IRQF_TRIGGER_LOW the author was probably looking for. > > A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more > instances of this. I found this by using one of these files as an > example and giving myself a lot of problems, so I would like to fix > this before it spreads anymore. > > I have a couple of ideas of how to go at this, first would be to > just replace the incorrect flags with what was intended, but for > some of these I don't know what was intended and do not have the > board to test. > > My other solution would be to just change all instances of the GPIO > flags to their value corresponding IRQ flags: > > - interrupts = <11 GPIO_ACTIVE_LOW>; > + interrupts = <11 IRQ_TYPE_EDGE_RISING>; > > this would not make any functional change as the defines would > still evaluate to the same value, but would make it obvious where > a problem may be and that they should probably be checked and > corrected, maybe we could even put a comment after: > > - interrupts = <11 GPIO_ACTIVE_LOW>; > + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type > > Well, what do you think? This seems fine. It is no less wrong. >>> >>> I'm not sure what you mean here. >> >> In this example, the correct value is probably IRQ_TYPE_LEVEL_LOW or >> IRQ_TYPE_EDGE_FALLING if the original text was correct in its >> intentions (but broken in implementation). Since the change you >> propose doesn't change the actual dtb at all, if it was wrong before >> it will still be wrong. >> > > I see, that's kinda what I want, maybe for this example the intentions > are obvious but my concern is with a couple others that I don't know > what the trigger was meant to be and don't have a board to test the > changes with, so I would never be sure if I causing any regressions > with the fixes. Most of the affected boards are Tegra based (that's > why I cc'd linux-tegra), I was hoping they would be interested in > testing and finding the right values. Presumably/hopefully if you send specific patches, the various maintainers/owners of those DT files will validate/ack then; you don't need to be able to test all of the changes yourself. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] dt/bindings: bcm2835: add binding documentation for bcm2835-aux
On 09/04/2015 01:26 AM, Martin Sperl wrote:
>
>> On 26.08.2015, at 03:44, Stephen Warren wrote:
>>
>> On 08/24/2015 02:40 AM, ker...@martin.sperl.org wrote:
>>
>>> +Example:
>>> +
>>> +aux_enable: aux_enable@0x7e215004 {
>>> + compatible = "bcrm,bcm2835-aux";
>>> + reg = <0x7e215004 0x04>;
>>
>> I'd expect that to be <0x7e215000 0x8>;
>
> The reason is that we just handle enable with this driver,
> which just requires access to the 0x7e215004 register.
>
> The 0x7e215000 register (interrupt mask) could be used by a
> cascaded interrupt-controller, but as the spi and uart drivers
> can run with shared interrupts this is not a necessity.
The DT is supposed to describe the HW, not any particular SW's use of
the HW. If the HW block has 2 registers, so must the DT reg property.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm: rpi: Device tree modifications for U-Boot
On 08/28/2015 11:27 AM, Simon Glass wrote:
> Hi Rob,
>
> On 25 August 2015 at 10:22, Rob Herring wrote:
>> On Sat, Aug 15, 2015 at 8:46 AM, Simon Glass wrote:
>>> Hi Rob,
>>>
>>> On 14 August 2015 at 14:29, Rob Herring wrote:
On Fri, Aug 14, 2015 at 1:34 PM, Simon Glass wrote:
> -linux-tegra
>
> Hi,
>
> On 12 August 2015 at 07:21, Simon Glass wrote:
>> Hi Lucas,
>>
>> On 11 August 2015 at 11:05, Lucas Stach wrote:
>>> Hi Simon,
>>>
>>> why did you send this to the Tegra ML?
>>>
>>> Am Dienstag, den 11.08.2015, 08:25 -0600 schrieb Simon Glass:
This updates the device tree from the kernel version to something
suitable
for U-Boot:
- Add stdout-path alias for console
- Mark the /soc node to be available pre-relocation so that the early
serial console works (we need the 'ranges' property to be available)
[... discussion of u-boot,dm-pre-reloc property]
Can't the need for that property change over time? Either as more
drivers are converted to DM you need to add this or you add some
feature that depends on a driver (e.g. get a board rev or boot mode
from GPIO). You would have backwards compatibility issues with this.
I'm somewhat less worried about that for u-boot as we should be
bundling the dtb and bootloader rather than kernel and dtb. For the
UART, you can just get which UART to initialize early from
stdout-path. But for other cases, couldn't you just have the platform
provide the list of needed drivers. Then when u-boot needs change, you
just change u-boot.
>>>
>>> Yes U-Boot and its device tree are normally built from the same tree
>>> at the same time. We don't expect to have to support an older or newer
>>> device tree with the same U-Boot binary. So I don't see a problem
>>> here.
>>
>> My point is that if I had to pick how bootloader+dtb+kernel are
>> bundled or not, I would rather see the dtb in sync with the bootloader
>> rather than the kernel. But I can't decide that for everyone and
>> neither can you. You still have a compatibility problem and that has
>> to be addressed.
>
> What are you getting at here? If I move to a new kernel and still use
> an old device tree I may be missing features, or fail to boot. Don't
> do that!
One of the central points of DT is that it is an ABI. As such, moving to
a new kernel and continuing to use the same old DTB *MUST NOT* break
anything. Of course, you won't get any features enabled/described in any
new DT if you don't use it.
...
> After reading your email I am none the wiser about what you are suggesting.
>
> This is not a screwy case. Every board will have a console. In some
> cases it is inside a 'soc {' node and in some cases it is not. The
> pure solution would be to mark every UART node with
> u-boot,dm-pre-reloc and we can do that if you prefer. It isn't
> necessary though for the reasons I previously explained.
>
> It seems reasonable that U-Boot should be able to add private
> properties to the device tree, intended for U-Boot, just as Linux
> does. What is the problem here?
Why is that reasonable? DT is intended to describe the HW. It is
supposed to be OS-agnostic. Having U-Boot-specific properties completely
goes against that.
What Linux-specific properties are you referring to? The only one I
recall you mentioning (although I'm missing a lot of sleep right now) is
the keycodes in the input binding. While the DT property name for those
is linux,... and the values happen to match internal Linux numbering,
there's absolutely nothing Linux specific about the /concept/ of a
keycode, and some numbering scheme had to be picked. There's nothing
practical or conceptual stopping any other OS/SW-stack from translating
those Linux IDs into something internally meaningful. Conversely, the
concept of e.g. "u-boot,dm-pre-reloc" is not something that translates
at all to any other OS/WW-stack.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/7] ARM: bcm2835: Add VC4 to the device tree.
On 08/18/2015 03:54 PM, Eric Anholt wrote: > VC4 is the GPU (display and 3D) present on the 2835. This patch and patch 1 seem OK to me, although I'll withhold any ack until the DT binding design discussion with Rob has been resolved. I haven't looked at the OF graph bindings he mentioned so have no clue about his suggestions. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/7] ARM: bcm2835: Add the DDC I2C controller to the device tree.
On 08/18/2015 03:54 PM, Eric Anholt wrote: > We need to use it for getting video modes over HDMI. This patch, Acked-by: Stephen Warren -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/5] spi: bcm2835: new driver implementing auxiliar spi1/spi2 on the bcm2835 soc
On 08/24/2015 02:40 AM, ker...@martin.sperl.org wrote: > From: Martin Sperl Patch description? > arch/arm/configs/bcm2835_defconfig |1 + > drivers/spi/Kconfig| 12 + > drivers/spi/Makefile |1 + > drivers/spi/spi-bcm2835aux.c | 506 > A change to the defconfig would be applied by the RPi maintainers, and a change to drivers/spi by the SPI maintainers. Those need to be in different patches. > diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c > +static int bcm2835aux_spi_probe(struct platform_device *pdev) > + clk_prepare_enable(bs->clk); Error checking? > + /* enable HW block */ > + bcm2835aux_enable(&pdev->dev, ENABLE_PROPERTY); The return value needs to be error-checked, so that deferred probe can work, and so other kinds of errors can be detected. Wasn't this correct in a previous patch version? Note that I didn't review any code besides probe(), remove() and the driver boiler-plate that refers to those functions. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/5] soc: bcm2835: auxiliar devices enable infrastructure
On 08/24/2015 02:40 AM, ker...@martin.sperl.org wrote:
> From: Martin Sperl
>
> The bcm2835 SOC contains 3 auxiliar devices (spi1, spi2 and uart1)
> that all are enabled via a shared register.
>
> To serialize access to this shared register this soc-driver
> is created that implements:
> bcm2835aux_enable(struct device *dev, const char *property);
> bcm2835aux_disable(struct device *dev, const char *property);
>
> Which will read the property from the device tree of the device
> and enable/disable that specific device as per device tree.
>
> First use of this api will be spi-bcm2835aux.
> diff --git a/drivers/soc/bcm/bcm2835-aux.c b/drivers/soc/bcm/bcm2835-aux.c
> +static void *bcm2835aux_find_base(struct device *dev, const char *property)
> +{
> + struct device *found = NULL;
> + struct device_node *np;
> +
> + /* get the phandle of the device */
> + np = of_parse_phandle(dev->of_node, property, 0);
> + if (!np) {
> + dev_err(dev, "missing property %s\n", property);
> + return ERR_PTR(-ENODEV);
> + }
> +
> + /* now find the device it points to */
> + found = driver_find_device(&bcm2835aux_driver.driver, NULL,
> +np, bcm2835aux_dev_match);
> + if (!found) {
> + dev_err(dev, "device for phandle of %s not found\n",
> + property);
> + return ERR_PTR(-ENODEV);
That should return ERR_PTR(-EPROBE_DEFER) so that client drivers know
when to defer their own probe, and not print an error. This is an
expected condition during probing. I could have sworn this was correct
in a previous patch revision.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/5] dt/bindings: bcm2835: Add binding documentation for auxiliar spi devices
On 08/24/2015 02:40 AM, ker...@martin.sperl.org wrote:
> From: Martin Sperl
Patch description?
> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm2835-aux-spi.txt
> b/Documentation/devicetree/bindings/spi/brcm,bcm2835-aux-spi.txt
> +Required properties:
> +- compatible: Should be "brcm,bcm2835-aux-spi".
> +- reg: Should contain register location and length for the spi block
> + as well as for the common aux block control
Is that meant to imply that reg should contain a single value that
covers both the common aux registers and the SPI device, or two separate
values, one for the aux common registers and another for the SPI device?
Neither of those options sound correct. I would expect only a single
entry which covered solely the SPI registers. The common aux registers
are owned by the other brcm,bcm2835-aux binding.
> +Example:
> +
> +spi1@7e215080 {
> + compatible = "brcm,bcm2835-aux-spi";
> + reg = <0x7e215080 0x40>;
That seems to match what I'd expect, but doesn't correspond to the
description above.
> +/* the necessary syscon config referenced above*/
> +aux_enable: aux_enable@0x7e215004 {
It's not a "syscon"...
> +Note that it also requires the GPIOs to be set up with the
> +correct ALT-functions.
> +
> +For spi1 the following pins need to be set as:
> +* ALT4: 19, 20, 21 (MISO, MOSI, SCK)
> +
> +For spi2 the following pins need to be set as:
> +* ALT4: 40, 41, 42 (MISO, MOSI, SCK)
> +
> +CS-GPIOS need to get set as output - typically:
> +* spi1: 18, 17, 16 (CS0, CS1, CS2)
> +* spi2: 43, 44, 45 (CS0, CS1, CS2)
That's generally true of any HW block, and has nothing to do with the
binding for the device. I would suggest removing that chunk of text.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] dt/bindings: bcm2835: add binding documentation for bcm2835-aux
On 08/24/2015 02:40 AM, ker...@martin.sperl.org wrote:
> From: Martin Sperl
Patch description?
> diff --git a/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-aux.txt
> b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-aux.txt
> +Required properties:
> +- compatible: Should be "brcm,bcm2835-aux".
> +- reg: Should contain register location and length for the
> + enable register
As I mentioned before, why not all the aux registers (that aren't part
of a sub-device like SPI).
> +Example:
> +
> +aux_enable: aux_enable@0x7e215004 {
> + compatible = "bcrm,bcm2835-aux";
> + reg = <0x7e215004 0x04>;
I'd expect that to be <0x7e215000 0x8>;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] ARM: bcm2835: Add VC4 to the device tree.
On 08/12/2015 06:56 PM, Eric Anholt wrote:
> Signed-off-by: Eric Anholt
Patch description?
> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> arm-pmu {
> compatible = "arm,arm1176-pmu";
> };
> +
> + hdmi: brcm,vc4-hdmi@7e902000 {
It'd be nice to keep all the DT nodes with a reg property together, and
sorted in reg order.
As before, I think any DT node for a HW block that may-or-may-not-be
used based on board connectivity/features should be disabled in the SoC
.dtsi file, and enabled in the board's DT file if the feature is useful
for that board.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] ARM: bcm2835: Add the DDC I2C controller to the device tree.
On 08/12/2015 06:56 PM, Eric Anholt wrote:
> We need to use it for getting video modes over HDMI.
> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> + i2c2: i2c@7e805000 {
> + compatible = "brcm,bcm2835-i2c";
> + reg = <0x7e805000 0x1000>;
> + interrupts = <2 21>;
> + clocks = <&clk_i2c>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
In an SoC .dtsi file, you'd typically write:
status = "disabled";
... in all nodes that represent IO controllers that interface to
external HW, so that board DT files can/must explicitly choose to enable
the device if it's actually in use on the board. Some systems might not
have HDMI and hence might not hook up the HDMI_SCL/SDA pads.
BCM2835-ARM-Peripherals.pdf states "Note that the BSC2 master is used
dedicated with the HDMI interface and should not be accessed by user
programs.". Does this imply the Linux kernel shouldn't be touching this
I2C controller; that the VC4 firmware might also be attempting to use
it? I wonder how any such sharing of the HW would work.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] drm/vc4: Add KMS support for Raspberry Pi.
On 08/12/2015 06:56 PM, Eric Anholt wrote: > This is the start of a full VC4 driver. Right now this just supports > configuring the display using a pre-existing video mode (because > changing the pixel clock isn't available yet, and doesn't work when it > is). However, this is enough for fbcon and bringing up X using > xf86-video-modesetting. > diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig > +config DRM_VC4 > + tristate "Broadcom VC4 Graphics" > + help > + Choose this option if you have a system that has a Broadcom > + VC4 GPU, such as the Raspberry Pi or other BCM2708/BCM2835. > + > + This driver requires that "avoid_warnings=2" be present in > + the config.txt for the firmware, to keep it from smashing > + our display setup. The need for "avoid_warnings=2" seems like it will trip people up. I don't think it's in any config.txt I've seen. Can you expand more on that? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] MAINTAINERS: Add myself for the new VC4 (RPi GPU) graphics driver.
On 08/12/2015 06:56 PM, Eric Anholt wrote: > diff --git a/MAINTAINERS b/MAINTAINERS > +DRM DRIVERS FOR VC4 > +M: Eric Anholt > +T: git git://github.com/anholt/linux > +S: Maintained > +F: drivers/gpu/drm/vc4/* S: Supported ? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] drm/vc4: Add devicetree bindings for VC4.
On 08/12/2015 06:56 PM, Eric Anholt wrote:
> Signed-off-by: Eric Anholt
This one definitely needs a patch description, since someone might not
know what a VC4 is, and "git log" won't show the text from the binding
doc itself. I'd suggest adding the initial paragraph of the binding doc
as the patch description, or more.
> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt
> b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt
> +Required properties for VC4:
> +- compatible:Should be "brcm,vc4"
> +- crtcs: List of references to pixelvalve scanout engines
s/references to/phandles of/ would be more typical DT language.
> +- hvss: List of references to HVS video scalers
> +- encoders: List of references to output encoders (HDMI, SDTV)
Would it make sense to make all those nodes child node of the vc4
object. That way, there's no need to have these lists of objects; they
can be automatically built up as the DT is enumerated. I know that e.g.
the NVIDIA Tegra host1x binding works this way, and I think it may have
been inspired by other similar cases.
Of course, this is only appropriate if the HW modules really are
logically children of the VC4 HW module. Perhaps they aren't. If they
aren't though, I wonder what this "vc4" module actually represents in HW?
> +Required properties for HDMI
> +- compatible:Should be "brcm,vc4-hdmi"
> +- reg: Physical base address and length of the two register
> ranges
> + ("HDMI" and "HD")
I'd add "in that order" right before ")".
> +Example:
> +/ {
> + soc {
Minor nit: Examples often don't include any nodes "above" the nodes
whose bindings are being documented.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/3] clk: Add a Raspberry Pi-specific clock driver.
On 08/13/2015 05:05 PM, Eric Anholt wrote:
> Unfortunately, the clock manager's registers are not accessible by the
> ARM, so we have to request that the firmware modify our clocks for us.
>
> This driver only registers the clocks at the point they are requested
> by a client driver. This is partially to support returning
> -EPROBE_DEFER when the firmware driver isn't supported yet, but it
> also avoids issues with disabling "unused" clocks due to them not yet
> being connected to their consumers in the DT.
> diff --git a/drivers/clk/clk-raspberrypi.c b/drivers/clk/clk-raspberrypi.c
> +static const struct {
> + const char *name;
> + int flags;
> +} rpi_clocks[] = {
> + [RPI_CLOCK_EMMC] = { "emmc", CLK_IS_ROOT },
> + [RPI_CLOCK_UART0] = { "uart0", CLK_IS_ROOT },
> + [RPI_CLOCK_ARM] = { "arm", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> + [RPI_CLOCK_CORE] = { "core", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> + [RPI_CLOCK_V3D] = { "v3d", CLK_IS_ROOT },
> + [RPI_CLOCK_H264] = { "h264", CLK_IS_ROOT },
> + [RPI_CLOCK_ISP] = { "isp", CLK_IS_ROOT },
> + [RPI_CLOCK_SDRAM] = { "sdram", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> + [RPI_CLOCK_PIXEL] = { "pixel", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> + [RPI_CLOCK_PWM] = { "pwm", CLK_IS_ROOT },
> +};
> +
> +struct rpi_firmware_clock {
> + /* Clock definitions in our static struct. */
> + const char *name;
> + int flags;
Are these duplicates of the values in rpi_clocks[]? Why not just store a
pointer to or index of the entry in that array?
> +static int rpi_clk_set_state(struct clk_hw *hw, bool on)
> +{
> + struct rpi_firmware_clock *rpi_clk =
> + container_of(hw, struct rpi_firmware_clock, hw);
> + u32 packet[2];
> + int ret;
> +
> + if (on == (rpi_clk->last_rate != 0))
> + return 0;
The overloading of last_rate to represent both rate information and
on/off status is slightly confusing. I would have expected this function
to clear last_rate to 0 when switching the clock off, and some specific
rate when turning a clock on. Is there some guarantee that the clock
core will always call recalc_rate() at certain times, thus ensuring that
last_rate is always accurate?
Wouldn't it be simpler to let last_rate always represent that actual
rate, and have a separate last_on or is_on field to represent the
enable/disable state?
> +static unsigned long rpi_clk_get_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
...
> + rpi_clk->last_rate = packet[1];
Since this is a query API, I wouldn't have expected it to have
side-effects like this. Don't we know what rate the clock runs at based
on the firmware's response in set_rate()?
> +static int rpi_clk_probe(struct platform_device *pdev)
> + onecell = devm_kmalloc(dev, sizeof(*onecell), GFP_KERNEL);
> + if (!onecell)
> + return -ENOMEM;
> + onecell->clk_num = ARRAY_SIZE(rpi_clocks);
> + onecell->clks = devm_kzalloc(dev, sizeof(*onecell->clks), GFP_KERNEL);
Don't you need to multiply the size by ARRAY_SIZE(rpi_clocks)? I assume
onecell->clks is an array with one entry per each of onecell->clk_num?
Yes, the for loop right after that allocation confirms this.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/3] ARM: bcm2835: Add DT for the firmware clocks driver.
On 08/13/2015 05:05 PM, Eric Anholt wrote:
> Signed-off-by: Eric Anholt
> Acked-by: Lee Jones
A patch description might be nice, although admittedly the subject seems
clear enough.
> diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi
> b/arch/arm/boot/dts/bcm2835-rpi.dtsi
> + firmware_clocks: firmware-clocks {
> + compatible = "raspberrypi,bcm2835-firmware-clocks";
> + #clock-cells = <1>;
> + raspberrypi,firmware = <&firmware>;
> + };
No need to resend for this, but just a background note: DT node names
usually contain a device type not a device identity, so "clocks" not
"firmware-clocks" would be typical.
This patch,
Acked-by: Stephen Warren
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm: rpi: Device tree modifications for U-Boot
On 08/12/2015 07:21 AM, Simon Glass wrote:
> Hi Lucas,
>
> On 11 August 2015 at 11:05, Lucas Stach wrote:
>> Hi Simon,
>>
>> why did you send this to the Tegra ML?
>>
>> Am Dienstag, den 11.08.2015, 08:25 -0600 schrieb Simon Glass:
>>> This updates the device tree from the kernel version to something suitable
>>> for U-Boot:
>>>
>>> - Add stdout-path alias for console
>>> - Mark the /soc node to be available pre-relocation so that the early
>>> serial console works (we need the 'ranges' property to be available)
>>>
>>> Signed-off-by: Simon Glass
>>> ---
>>>
>>> arch/arm/boot/dts/bcm2835.dtsi | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
>>> index 301c73f..bd6bff6 100644
>>> --- a/arch/arm/boot/dts/bcm2835.dtsi
>>> +++ b/arch/arm/boot/dts/bcm2835.dtsi
>>> @@ -8,6 +8,7 @@
>>>
>>> chosen {
>>> bootargs = "earlyprintk console=ttyAMA0";
>>> + stdout-path = &uart;
>>> };
>>>
>>> soc {
>>> @@ -16,6 +17,7 @@
>>> #size-cells = <1>;
>>> ranges = <0x7e00 0x2000 0x0200>;
>>> dma-ranges = <0x4000 0x 0x2000>;
>>> + u-boot,dm-pre-reloc;
>>
>> Why do you need this and why should upstream carry your favourite
>> bootloaders configuration? This is in no way hardware description.
>
> I'm not sure how much you know about U-Boot, so let me know if you
> need more info.
>
> U-Boot normally starts up by setting up its serial UART and displaying
> a banner message. At this stage typically only a few devices are
> initialised (e.g. maybe just the UART). It then relocates itself to
> the top of memory and starts up all the devices. It throws away any
> previous devices that it set up before relocation and starts again.
>
> U-Boot uses a thing called driver model (dm) which handles driver
> binding and probing. Driver model has the device tree and would
> normally scan through it and create devices for everything it finds.
>
> Before relocation we don't need every device. Also the CPU is often
> running slowly, perhaps without the cache enabled. SDRAM may not be
> available yet so space is short. We want to avoid starting up things
> that will not be used.
>
> So this property indicates that the device is needed before relocation
> and should be set up by driver model. We need it to avoid a very slow
> and memory-hungry startup.
>
> As to why upstream should accept it, my understanding of upstream is
> that people can send patches to it and in fact are encouraged to do
> so, to avoid misunderstandings and duplication. The device tree files
> are stored in Linux so any binding or source file changes should end
> up there. Otherwise the files tend to diverge and we end up with
> multiple bindings and multiple versions of the same source file.
On many platforms, we have U-Boot SPL running first, then the main
U-Boot. The main U-Boot binary contains both the code to do the
relocation and the binary that runs after relocation. It seems like it'd
be simpler to split these up into 3 binaries that each do a single job:
1) SPL, roughly as-is today (varying jobs depending on platform)
2) Relocator, which does nothing but work out where to copy U-Boot,
memcpy()s it there, relocates the image (if not PIE), and jumps to it.
3) The main U-Boot.
Item (2) above should be simple enough that it can use a very simple
debug mechanism rather like DEBUG_LL in the Linux kernel. Similar to
what Rob mentioned in his email.
Item (3) could use DM and DT/ACPI/... to get device information in a
complete non-hard-coded manner.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [U-Boot] [PATCH v3 01/11] dm: serial: Update binding for PL01x serial UART
Ian Lepore wrote at Friday, August 14, 2015 11:46 AM: > On Fri, 2015-08-14 at 09:27 -0500, Rob Herring wrote: > > On Thu, Aug 13, 2015 at 2:04 PM, Ian Lepore wrote: > > > On Thu, 2015-08-13 at 14:13 -0400, Tom Rini wrote: > > >> On Thu, Aug 13, 2015 at 10:02:58AM -0600, Stephen Warren wrote: ... (Replying from my NVIDIA address, since one of the mail servers in the path to you bounced my last reply. Apologies for any formatting issues.) > > > A great case in point would be i2c eeproms. What a perfect > > > opportunity DT would be to describe everything about the eeprom > > > parts (total capacity, page read/write size, whether the page > > > address bits extend into the bus-slave address bits, etc). It seems > > > to me that anything claiming to be an independent description of the > > > hardware would have to include such things. Instead, all the > > > bindings define is the compatible string. That's crazy. Why? > > > Well, when I went and looked at the Linux eeprom drivers it became > > > clear why: that's all they need to know, because everything else is hard- > > > coded in tables in the driver source. > > > > Think of compatible as a PID/VID pair like USB or PCI. It is simply a > > unique identifier. You don't get any more information with PCI or > > non-class USB devices. DT was originally only solving the problem of > > finding the non-probeable h/w. It's grown to be a lot more with > > today's SOCs. > > > > The more we put into DT the more chance it can be wrong and then we > > have to work around not h/w quirks, but DT quirks. > > > > That said we can always add to the bindings. It would have to be > > worded something like "optional, but required for new dts's". You > > would have to have a new DTB if the OS is dependent on the new > > properties. > > > > > So if I want to write a FreeBSD i2c eeprom driver that uses DT data, > > > what are my choices? I have exactly one: make my driver > > > essentially a clone of the Linux driver, with all the same data hard-coded > > in source. > > > > Don't you already have these drivers w/o using DT? If you did, you > > would have this information already in the drivers. It wouldn't be a > > question of the binding being Linux specific, but rather can we move > > more of the data out of drivers and into DT. That is fundamentally a > > different issue than is a binding Linux specific. > > This last paragraph most eloquently illustrates the point I was trying to > make: > From the point of view of someone who only knows about the existing linux > driver and how it is written, the current DT data is perfect. It's exactly > what > that existing driver needs to know, and from that position you can argue that > it is thus the ONLY thing any driver written by anyone would need to know. > That assumes that everyone wants to just clone the linux drivers (or in our > case, because of licensing, rewrite the drivers in a completely linux- > compatible way that somehow isn't simply copying them in violation of the > GPL). I believe this is nothing to do with encoding Linuxisms into the DT. It's part of DT itself. DT practice is generally to encode the device model name/number into DT (perhaps with a few properties for extra details) rather than to encode a completely generic device type (e.g. "I2C EEPROM") along with all the possible information anyone could ever want to know about that device. As Rob and Geert mentioned before, "all the possible information" is something that's generally not possible to enumerate fully and/or correctly. If DT weren't an ABI, that might not be a problem; we'd just change the schema, fix the bug and move on. However since DT is an ABI, we can't just go back and fix mistakes. The fact the way DT represents devices aligns well with some aspects of the Linux driver model is nice, but in no way a Linuxism; I believe DT was like this well before Linux used DT. -- nvpbulic --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [U-Boot] [PATCH v3 01/11] dm: serial: Update binding for PL01x serial UART
On 08/13/2015 01:04 PM, Ian Lepore wrote:
On Thu, 2015-08-13 at 14:13 -0400, Tom Rini wrote:
On Thu, Aug 13, 2015 at 10:02:58AM -0600, Stephen Warren wrote:
On 08/13/2015 09:59 AM, Simon Glass wrote:
Hi Linus,
On 11 August 2015 at 07:00, Linus Walleij wrote:
On Fri, Aug 7, 2015 at 3:42 PM, Simon Glass wrote:
This binding differs from that of Linux. Update it and change existing
users.
Signed-off-by: Simon Glass
(...)
doc/device-tree-bindings/serial/pl01x.txt | 55 ---
So why does U-Boot have its own copy of any bindings at all?
This is forking the ontology of who gets to define bindings I fear.
It's a bit like have two bibles both claiming to be the word of god.
(OK maybe a hyperbolic statement, but you see what I mean.)
Can't we just have the bindings in the Linux kernel tree please?
Is there any plan to move them out of Linux and put them in a separate place?
We should make an effort to sync the device tree files with Linux periodically.
I quite like having the bindings in U-Boot since it makes people think
about what they are adding. Are you worried that the bindings in
U-Boot might evolve separately? Certainly there has been some of that.
However I recently sent a series to add a few things for Raspberry Pi
("arm: rpi: Device tree modifications for U-Boot") and I don't yet see
a willingness to add what some see as 'U-Boot things' to the binding.
How do we address that?
DT isn't supposed to contain "U-Boot things", but "an OS-agnostic
description of the hardware". So, I imagine the solution is not to
attempt to do that:-)
This always makes me ask if the FreeBSD folks or VxWorks folks have
adopted the "Linux" bindings or if the DT files continue to be
"OS-agnostic" and "only functional with Linux". It was a while ago last
I looked but it made my head hurt a little doing a quick translation for
an SoC that I was familiar with.
As the FreeBSD person who got our first SoC (imx6, only partially
supported) converted to use the Linux DT files rather than our own
homebrew mess we started with, I would say that my opinion of the
existing DT information is that it is an extension of Linux device
drivers written in a different language.
The information available is in no way independent of the Linux device
drivers, it is exactly the information those drivers need. It is often
not the information needed in another OS with independently written
drivers. And especially it is not ordered and structured in a way that
works well with the device enumeration and instantiation models used by
another OS.
A great case in point would be i2c eeproms. What a perfect opportunity
DT would be to describe everything about the eeprom parts (total
capacity, page read/write size, whether the page address bits extend
into the bus-slave address bits, etc). It seems to me that anything
claiming to be an independent description of the hardware would have to
include such things. Instead, all the bindings define is the compatible
string. That's crazy. Why? Well, when I went and looked at the Linux
eeprom drivers it became clear why: that's all they need to know,
because everything else is hard-coded in tables in the driver source.
So if I want to write a FreeBSD i2c eeprom driver that uses DT data,
what are my choices? I have exactly one: make my driver essentially a
clone of the Linux driver, with all the same data hard-coded in source.
All in all, it's not impossible for another OS to work with the DT
information that begins its life in Linux, but it's not really easy.
In fairness, that's got nothing to do with Linux, but it's a general
decision re: the level of detail to put into DT. There's always a
discussion about which level of detail to represent in DT when new
bindings are created.
The type of an I2C device completely defines all of its properties; the
model name/... is enough to fully describe its behaviour. That's a good
reason to put just that information into DT, to avoid redundancy.
In some cases, bindings have tended towards placing just the compatible
value into the DT (e.g. your example). This does require drivers to be
able to look up that information from the compatible value.
That case tends to be more common since what's really important about DT
is cleanly representing the resource interactions between devices; let
the drivers know all the details of the device's internals, and let DT
describe any point where the device/driver has to interact with the
system or other devices/drivers around it.
Often when a driver supports a ton of devices, it needs explicit code to
deal with each individual device's quirks, so it may as well have the
table of quirks inside it too, rather than having the table in DT, and
the code to handle them in the driver, and have to do work to link the
two
Re: [PATCH v3 01/11] dm: serial: Update binding for PL01x serial UART
On 08/13/2015 09:59 AM, Simon Glass wrote:
Hi Linus,
On 11 August 2015 at 07:00, Linus Walleij wrote:
On Fri, Aug 7, 2015 at 3:42 PM, Simon Glass wrote:
This binding differs from that of Linux. Update it and change existing
users.
Signed-off-by: Simon Glass
(...)
doc/device-tree-bindings/serial/pl01x.txt | 55 ---
So why does U-Boot have its own copy of any bindings at all?
This is forking the ontology of who gets to define bindings I fear.
It's a bit like have two bibles both claiming to be the word of god.
(OK maybe a hyperbolic statement, but you see what I mean.)
Can't we just have the bindings in the Linux kernel tree please?
Is there any plan to move them out of Linux and put them in a separate place?
We should make an effort to sync the device tree files with Linux periodically.
I quite like having the bindings in U-Boot since it makes people think
about what they are adding. Are you worried that the bindings in
U-Boot might evolve separately? Certainly there has been some of that.
However I recently sent a series to add a few things for Raspberry Pi
("arm: rpi: Device tree modifications for U-Boot") and I don't yet see
a willingness to add what some see as 'U-Boot things' to the binding.
How do we address that?
DT isn't supposed to contain "U-Boot things", but "an OS-agnostic
description of the hardware". So, I imagine the solution is not to
attempt to do that:-)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm: rpi: Device tree modifications for U-Boot
On 08/11/2015 11:05 AM, Lucas Stach wrote:
Hi Simon,
why did you send this to the Tegra ML?
At my request, so that the DT changes could be reviewed by the kernel DT
reviewers and added to the copy of the DT files in the kernel source
tree if approved.
My assertion is that DT content should be independent of SW stack. Or
put another way, all SW stacks should use the same DT content. As such,
if these properties are needed by U-Boot, and contained in the copy of
the DT files in the U-Boot source tree, they should also be present in
the copy of the DT files in the kernel source tree, so the two do not
become out of sync.
Am Dienstag, den 11.08.2015, 08:25 -0600 schrieb Simon Glass:
This updates the device tree from the kernel version to something suitable
for U-Boot:
- Add stdout-path alias for console
- Mark the /soc node to be available pre-relocation so that the early
serial console works (we need the 'ranges' property to be available)
diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index 301c73f..bd6bff6 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -8,6 +8,7 @@
chosen {
bootargs = "earlyprintk console=ttyAMA0";
+ stdout-path = &uart;
};
soc {
@@ -16,6 +17,7 @@
#size-cells = <1>;
ranges = <0x7e00 0x2000 0x0200>;
dma-ranges = <0x4000 0x 0x2000>;
+ u-boot,dm-pre-reloc;
Why do you need this and why should upstream carry your favourite
bootloaders configuration? This is in no way hardware description.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] dm: serial: Update binding for PL01x serial UART
On 08/10/2015 10:11 PM, Simon Glass wrote: > HI Stephen, > > On 10 August 2015 at 21:57, Stephen Warren wrote: >> On 08/07/2015 07:42 AM, Simon Glass wrote: >>> This binding differs from that of Linux. Update it and change existing >>> users. >> >> Is that meant to imply that this patch fixes the copy of the binding doc >> in U-Boot so it does match the kernel's copy? >> >>> Changes in v3: >>> - Rename binding file to pl01x.txt >> >> The file is named pl011.txt in the kernel. Shouldn't U-Boot's copy be >> named the same? > > In the previous discussion a reviewed asked for this change, since it > covers both drivers. I think it's more important to match the DT docs in the Linux kernel since they are a shared resource, and should eventually be split off into a separate project at some point, forking from whatever Linux has at that time. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] dm: serial: Update binding for PL01x serial UART
On 08/07/2015 07:42 AM, Simon Glass wrote:
> This binding differs from that of Linux. Update it and change existing
> users.
Is that meant to imply that this patch fixes the copy of the binding doc
in U-Boot so it does match the kernel's copy?
> Changes in v3:
> - Rename binding file to pl01x.txt
The file is named pl011.txt in the kernel. Shouldn't U-Boot's copy be
named the same?
> diff --git a/arch/arm/dts/stv0991.dts b/arch/arm/dts/stv0991.dts
> uart0: serial@0x80406000 {
> compatible = "arm,pl011", "arm,primecell";
> reg = <0x80406000 0x1000>;
> - clock = <270>;
> + clock-frequency = <270>;
I don't see either "clock" or "clock-frequency" mentioned in the Linux
binding doc.
> diff --git a/doc/device-tree-bindings/serial/pl01x.txt
> b/doc/device-tree-bindings/serial/pl01x.txt
> Required properties:
> -- compatible: must be "arm,primecell", "arm,pl011" or "arm,pl010"
> +- compatible: must be "arm,primecell", "arm,pl011"
It'd be worth mentioning which version of Linux this binding doc came
from; that text has changed in linux-next since v4.1 which is what I
assume you're importing.
> diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c
> @@ -365,13 +365,15 @@ static int pl01x_serial_ofdata_to_platdata(struct
> udevice *dev)
> struct pl01x_serial_platdata *plat = dev_get_platdata(dev);
> fdt_addr_t addr;
>
> - addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
> + addr = dev_get_addr(dev);
> if (addr == FDT_ADDR_T_NONE)
> return -EINVAL;
That looks like an unrelated change.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2
On 07/28/2015 04:48 AM, Martin Sperl wrote: > On 28.07.2015 08:18, Martin Sperl wrote: >> Hi Stephen! >> But the bigger question you have not answered is: “where should such an >> auxiliar driver go in the kernel tree?” i.e. which directory? > > One thing: could the "module" be a regulator? The HW docs aren't very detailed on this point, but they certainly don't state that these bits control a regulator (or power domain as mentioned later in the thread). I guess it's possible, but it feels like an assumption to me. The HW modules seem small enough that it doesn't feel likely anyone would have bothered with power-gating them. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2
On 07/28/2015 12:18 AM, Martin Sperl wrote: > Hi Stephen! > >> On 28.07.2015, at 04:51, Stephen Warren wrote: >> >> >>> If this is not acceptable, then where should such a driver go in the >>> kernel tree? >>> >>> It would essentially implement the following: >>> bcm2835aux_enable(dev, device-id); >>> bcm2835aux_disable(dev, device-id); >>> >>> Which would just set/clean the bits in the register while holding a lock. >> >> That sounds reasonable. I'd also expect a function that the client >> drivers could call during probe() to look up the device (and implement >> deferred probe) and another to release the reference during the client's >> remove(). > > But the bigger question you have not answered is: “where should such an > auxiliar driver go in the kernel tree?” i.e. which directory? drivers/soc seems made for this. > I really do not want to implement it and then get told: “that should not > go here” - been thru too many iterations already… > > Also I am not sure I understood the “defer” thingy. > I thought of actually implementing only 2 functions to use during probe > and removal: > * bcm2835aux_enable(dev) > * bcm2835aux_disable(dev) > > Both would pick up the “bcrm,aux” property from the device tree (as per > “bcrm,aux = <&bcm2835aux 4>”) and set the enable register accordingly > holding a lock. That'd probably be fine. The important thing is to get the DT right since that's an ABI. The implementation can be refactored/rewritten at will later provided the right info is in the DT. If you go this route, deferred probe isn't relevant. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2
On 07/24/2015 12:47 AM, Martin Sperl wrote:
>
>> On 24.07.2015, at 06:09, Stephen Warren wrote:
>>
>> I think I'd expect the shared registers to be:
>>
>> bcm2835aux: misc@0x7e215000 {
>>compatible = "brcm,bcm2835-aux";
>>reg = <0x7e215000 0x08>;
>> };
>>
>> There are two 4-byte registers outside the UART/SPI blocks, and the
>> compatible value should reflect what the block is, not that Linux may
>> use a syscon driver to implement the driver for it.
>>
>> In the client, I'd expect a more semantic naming for the reference;
>> something like:
>>
>> brcm,aux = <&bcm2835aux 4>;
>>
>> brcm, since it's a custom/vendor-specific property. aux is the name of
>> the object that's referenced. Packing the phandle and data together into
>> a single property reduces the number of separate properties, and is a
>> typical thing to do in a client of a service in DT.
>
> So in the end you are saying we need a separate driver to get written
> (because of ‘compatible = "brcm,bcm2835-aux”;’)
It's fine if some existing driver matches that compatible value.
> That is unless you would allow:
> compatible = compatible = "brcm,bcm2835-aux”, “syscon”;
"syscon" definitely shouldn't be in a DT.
> If this is not acceptable, then where should such a driver go in the
> kernel tree?
>
> It would essentially implement the following:
> bcm2835aux_enable(dev, device-id);
> bcm2835aux_disable(dev, device-id);
>
> Which would just set/clean the bits in the register while holding a lock.
That sounds reasonable. I'd also expect a function that the client
drivers could call during probe() to look up the device (and implement
deferred probe) and another to release the reference during the client's
remove().
> As an alternative: maybe we could implement it as an IRQ driver
> and when the irq is requested for that device, then the HW-block gets
> enabled automatically (and disabled when released).
That seems a bit too implicit; there's no strict requirement that a
driver for e.g. the SPI block has to use an interrupt; it's merely
extremely likely.
> That way we may not need to have a separate driver that would enable
> the uart1, but it would be sufficient to configure it as follows:
Surely the IRQ driver would be a separate driver either way? The only
difference is whether it exposes custom APIs, or does things as a
side-effect of the existing IRQ API.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] irqchip: bcm2835: If a parent interrupt is registered, chain from it.
On 07/22/2015 12:17 PM, Eric Anholt wrote: > Stephen Warren writes: > >> On 07/13/2015 07:35 PM, Eric Anholt wrote: >>> The BCM2836 (Raspberry Pi 2) uses two levels of interrupt >>> handling with the CPU-local interrupts being the root, so we >>> need to register ours as chained off of the CPU's local >>> interrupt. >> >> Sorry for the slow review; laziness after vacation! >> >>> diff --git >>> a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt >>> b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt >> >>> >>> +The BCM2836 contains the same interrupt controller with the same >>> +interrupts, but the per-CPU interrupt controller is the root, >>> and an +interrupt there indicates that the ARMCTRL has an >>> interrupt to handle. + Required properties: >>> >>> - compatible : should be "brcm,bcm2835-armctrl-ic" >> >> Since there are some differences between the bcm2835 and bcm2836 >> HW blocks, I'd expect the compatible value to be different for >> each. In particular... > > Well, there are actually no differences within this block of the HW > (HDL is unmodified), it's just where the output interrupt line gets > consumed. But it's not much extra to add a new compatible value, so > sure. Mmm. I suppose that's true indeed. So, I guess either of the following is fine for bcm2836 by me: compatible = "brcm,bcm2836-armctrl-ic"; compatible = "brcm,bcm2836-armctrl-ic", "brcm,bcm2835-armctrl-ic"; The 2836 value is always needed since DT should contain the most specific compatible value for the implementation. The 2835 value is optional based on whether the HW block is 100% backwards-compatible with the older HW block; a driver for the old block can run unmodified against the new block. It's debatable whether that's true here; the interface to this HW block itself is unchanged between implementations, yet the way the driver for it integrates into the system differs since it either is/isn't a top-level IRQ chip. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2
On 07/22/2015 10:28 AM, Martin Sperl wrote:
>
>
>> On 22.07.2015, at 03:55, Stephen Warren wrote:
>>
>> However, I'd like to see a "semantic" driver for the shared register
>> region rather than a "syscon". IIUC, "syscon" simply provides a stylized
>> way for one driver to touch some shared registers directly without any
>> semantics. I'd strongly prefer to see a real driver inside Linux rather
>> than something that just lets drivers fiddle with the shared registers
>> willy nilly. Still, this aspect is an internal implementation detail
>> inside the kernel that we can change without external impact later if we
>> need.
>>
>> More concerning: The bcm283x HW doesn't implement a "syscon" module, but
>> some semantic IP block. The DT should contain a real compatible value
>> that describes what the HW block really is, not just "syscon". We could
>> bind the syscon driver to this compatible value if we have to for
>
> So, do I understand you correctly that if we would call the node
> bcm2835aux_enable as syscon with only the enable bit field register in it
> and add a enable_reg (pointing to the above) and enable_reg_mask=2 or 4
> to the spi1/2 nodes then it would be acceptable?
>
> Would look like this:
> spi2@7e2150c0 {
> +compatible = "brcm,bcm2835-aux-spi";
> +reg = <0x7e2150c0 0x40>;
> +interrupts = <1 29>;
> +clocks = <&clk_spi>;
> +#address-cells = <1>;
> +#size-cells = <0>;
> +cs-gpios = <&gpio 43>, <&gpio 44>, <&gpio 45>;
> +enable_reg = <&bcm2835aux_enable>;
> +enable_reg_mask = 4;
> +};
> +
> +/* the necessary syscon config referenced above */
> +bcm2835aux_enable: bcm2835aux_enable@0x7e215004 {
> +compatible = "syscon";
> +reg = <0x7e215004 0x04>;
> +};
>
>
> The uart aux driver would use:
> +enable_reg = <&bcm2835aux_enable>;
> +enable_reg_mask = 1;
I think I'd expect the shared registers to be:
bcm2835aux: misc@0x7e215000 {
compatible = "brcm,bcm2835-aux";
reg = <0x7e215000 0x08>;
};
There are two 4-byte registers outside the UART/SPI blocks, and the
compatible value should reflect what the block is, not that Linux may
use a syscon driver to implement the driver for it.
In the client, I'd expect a more semantic naming for the reference;
something like:
brcm,aux = <&bcm2835aux 4>;
brcm, since it's a custom/vendor-specific property. aux is the name of
the object that's referenced. Packing the phandle and data together into
a single property reduces the number of separate properties, and is a
typical thing to do in a client of a service in DT.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] irqchip: Add bcm2836 interrupt controller for Raspberry Pi 2.
On 07/13/2015 07:35 PM, Eric Anholt wrote:
> This interrupt controller is the new root interrupt controller with
> the timer, PMU events, and IPIs, and the bcm2835's interrupt
> controller is chained off of it to handle the peripherals.
> diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
> +static void bcm2836_arm_irqchip_mask_pmu_irq(struct irq_data *d)
> +{
> + pr_err("%d: mask PMU\n", smp_processor_id());
> + writel(1 << smp_processor_id(), intc.base + LOCAL_PM_ROUTING_CLR);
> +}
> +
> +static void bcm2836_arm_irqchip_unmask_pmu_irq(struct irq_data *d)
> +{
> + pr_err("%d: unmask PMU\n", smp_processor_id());
> + writel(1 << smp_processor_id(), intc.base + LOCAL_PM_ROUTING_SET);
> +}
Are those pr_err() calls left-over debug, or is there some reason it's
an error to call those functions?
Aside from this and the other minor comment, the series,
Acked-by: Stephen Warren
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] irqchip: bcm2835: If a parent interrupt is registered, chain from it.
On 07/13/2015 07:35 PM, Eric Anholt wrote: > The BCM2836 (Raspberry Pi 2) uses two levels of interrupt handling > with the CPU-local interrupts being the root, so we need to register > ours as chained off of the CPU's local interrupt. Sorry for the slow review; laziness after vacation! > diff --git > a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt > > b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt > +The BCM2836 contains the same interrupt controller with the same > +interrupts, but the per-CPU interrupt controller is the root, and an > +interrupt there indicates that the ARMCTRL has an interrupt to handle. > + > Required properties: > > - compatible : should be "brcm,bcm2835-armctrl-ic" Since there are some differences between the bcm2835 and bcm2836 HW blocks, I'd expect the compatible value to be different for each. In particular... > +Optional properties: > +- interrupt-parent : Specifies the parent interrupt controller when this > + controller is the second level. > +- interrupts : Specifies the interrupt on the parent for this interrupt > + controller to handle. I'd classify that as "additional required properties for brcm,bcm2836-armctrl-ic" ... and with different compatible values for the two chips, you would know when probe() should require vs. reject the property. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2
On 07/14/2015 06:39 AM, Martin Sperl wrote: > >> On 14.07.2015, at 14:56, Stephen Warren wrote: >> >> I don't care so much about the internal code details; anything there can >> be trivially changed. But please do make sure the DT correctly >> represents the HW by: >> >> * Having a separate DT node for each HW block (SPI controller, UART, and >> any shared interrupt mux/demux/controller/...) > > That is what we have with the v3 patch: > * spi1 > * spi2 > * syscon controlling the aux-enable register > (only used to enable/disable the hw block) OK, having separate nodes is great. However, I'd like to see a "semantic" driver for the shared register region rather than a "syscon". IIUC, "syscon" simply provides a stylized way for one driver to touch some shared registers directly without any semantics. I'd strongly prefer to see a real driver inside Linux rather than something that just lets drivers fiddle with the shared registers willy nilly. Still, this aspect is an internal implementation detail inside the kernel that we can change without external impact later if we need. More concerning: The bcm283x HW doesn't implement a "syscon" module, but some semantic IP block. The DT should contain a real compatible value that describes what the HW block really is, not just "syscon". We could bind the syscon driver to this compatible value if we have to for now. >> * If e.g. the SPI controller driver is going to need to manipulate the >> "shared interrupt mux/demux/controller/...", there should be a phandle >> from the SPI controller node to the "shared interrupt >> mux/demux/controller/..." node, rather than listing the shared registers >> in each "client"'s reg property. > That is not what is in the v3 release of the patch any longer. > > Each of the above dt-nodes has their own (non-overlapping) register > ranges and interrupts are enabled/disabled in the respective ranges > for spi1/spi2. Also the status register of spi1/2 contain the > corresponding flags for the reason of an interrupt - so no need > to access any of the shared aux registers for that - hence the use > of IRQF_SHARED. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] dt: paz00: define nvec as child of i2c bus
On 07/20/2015 02:35 PM, Andrey Danin wrote:
NVEC driver was reimplemented to use tegra i2c. Use common i2c bindings
for NVEC node.
diff --git a/arch/arm/boot/dts/tegra20-paz00.dts
b/arch/arm/boot/dts/tegra20-paz00.dts
+ nvec: nvec@45 {
+ compatible = "nvidia,nvec-slave";
+ reg = <0x45>;
I think you need to or in I2C_OWN_SLAVE_ADDRESS from
here?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/4] staging/nvec: reimplement on top of tegra i2c driver
On 07/20/2015 02:35 PM, Andrey Danin wrote: Remove i2c controller related code and use tegra i2c driver in slave mode. Update nvec documentation. diff --git a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt I would expect this patch to add a new binding file nvidia,nvec-slave.txt so that the filename continues to match the compatible value it documents. This patch introduces a new binding for the NVEC slave as opposed to modifying the existing binding. +- compatible : should be "nvidia,nvec-slave". +- reg: the i2c address of the slave controller I think "the I2C address to respond to" would be clearer? You might also mention that this needs to include flags from . -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] irqchip: Add bcm2836 interrupt controller for Raspberry Pi 2.
On 07/11/2015 01:51 AM, Thomas Gleixner wrote: > On Fri, 10 Jul 2015, Stephen Warren wrote: >> On 07/07/2015 03:13 PM, Eric Anholt wrote: >>> +static struct arm_local_intc intc __read_mostly; >> >> It'd be nice to give everything (types, functions, variables) a >> consistent symbol prefix; bcm2836_arm_irqchip_ sounds like a good >> bikeshed to me, but perhaps just propagating the above arm_local_ to the >> functions too would be good, although that seems to risk symbol name >> collisions with other ARM SoCs. > > Which is irrelevant because its static. Except if you want to look up a symbol name without having to qualify it by filename. Or, does e.g. gdb require statics always be qualified even if they're globally unique? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] irqchip: Add documentation for the bcm2836 interrupt controller.
On 07/11/2015 12:01 AM, Eric Anholt wrote:
> Stephen Warren writes:
>
>> On 07/07/2015 03:13 PM, Eric Anholt wrote:
>>> This is a new per-cpu root interrupt controller on the
>>> Raspberry Pi 2, which will chain to the bcm2835 interrupt
>>> controller for peripheral interrupts.
>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt
>>> b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt
>>
>>>
>>>
+local_intc: local_intc {
>>
>>> + interrupt-parent = <&local_intc>;
>>
>> I think that property shouldn't be there?
>
> If you don't have it there, the core finds the interrupt-parent in
> the parent node, and waits for that one before initializing (which
> is in turn waiting for us). Note that for original 2835, you're
> finding the parent node as well.
Ah yes. It does indeed look like it's typical that the root IRQ
controller points at itself.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2
On 07/11/2015 08:10 PM, Martin Sperl wrote: > I am on a business-trip right now, so I can not check the mailing list > that often and my access to a rpi to develop is also limited hence > the V3 patch set took some time to get out of the door and crossed > the responses you had sent. > >> On 11.07.2015, at 14:53, Stephen Warren wrote: >> Hopefully the license of that tar file allows for that. I didn't look. > Well, I just took the values from the released video driver for those values > where doe documentation is obviously wrong - essentially clarifying > the SPI_STAT register on page 25. > >> As mentioned elsewhere, I'd hope all the shared aux register >> manipulations can be pushed into a shared driver. > > I just found this email after having sent the latest version of the patch > that makes use of syscon/regmap to serialize access to the > bcm2835_AUX_ENABLE register - that is all that it is really needed for. > > From my perspective it seems that a new driver just would produce > more code (to maintain) for something that the syscon driver already > provides. If someone starts to enable aux-uart, then it could go with > a new framework, but that is still more code that needs to get maintained > than just using syscon as that "shared" driver. > > As for hardcoded ids, that may be the case, but I just wanted to keep > Device tree as minimal as necessary... > If we get to that situation then we can easily move that to some > other logic... > > Finally with regards to interrupts: these are shared anyway so I > wonder why we would want to write an extra irq chip driver when > it works as is anyway. > > If someone thinks that is of a performance advantage then this > extra layer of indirection could get written and we would only need > to change the device-tree to use the new irq vectors instead. > > But we are really only talking about 2 drivers and 3 interrupt sources. I don't care so much about the internal code details; anything there can be trivially changed. But please do make sure the DT correctly represents the HW by: * Having a separate DT node for each HW block (SPI controller, UART, and any shared interrupt mux/demux/controller/...) * If e.g. the SPI controller driver is going to need to manipulate the "shared interrupt mux/demux/controller/...", there should be a phandle from the SPI controller node to the "shared interrupt mux/demux/controller/..." node, rather than listing the shared registers in each "client"'s reg property. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2
On 07/04/2015 07:14 PM, Martin Sperl wrote: > >> On 02.07.2015, at 06:57, Noralf Trønnes wrote: >> >> >> Den 01.07.2015 21:39, skrev Martin Sperl: On 30.06.2015, at 19:42, Mark Brown wrote: This looks relevant: >>> On 22.06.2015, at 16:55, Jakub Kiciński wrote: >>> As mentioned by Noralf UART1 is quite commonly used on Compute Modules. >>> Proper driver - perhaps modelled as a bus - seems like a prerequisite >>> for this work. You are also not using IRQ mux in DT binding example >>> which is very misleading. >> >> [...] >> >>> Finally asking for a recommendation with regards to using a bus >>> to arbitrate access to the enable register there was no feedback >>> how this could be get implemented... >> >> Maybe you can use drivers/mfd/syscon.c to enable shared access to the >> aux enable register. Then the spi driver could get the regmap with: >> aux_regmap = syscon_regmap_lookup_by_phandle(np, "syscon"); > > Seems as if you found an answer to my original question of if > there is a framework to handle this. I'd suggest avoiding syscon. IIUC, it just exposes raw register IO for the shared register region, which is very "non-semantic", hence completely hides what the reason the registers are being touched. I'd much prefer a custom driver with use-case-specific APIs. If the registers implement an IRQ mux/demux, an explicit IRQ controller driver would be much better. >> About sharing the aux interrupt, could this be implemented in irq-bcm2835 >> as a Bank 3? > > Obviously it could, but the question is if it is not more overhead than using > a shared interrupt (requesting an interrupt with the shared flag set) in the > first place (like this driver currently does or i2c and USB do). IIUC, these registers aren't part of the main IRQ controller's register set, and hence shouldn't be touched by the main IRQ controller driver. Create a separate driver for entirely separate HW blocks/functionality/... > I am working on a new version to make use of syscon. :-( -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2
On 06/22/2015 09:26 AM, Martin Sperl wrote: >> On 22.06.2015, at 16:55, Jakub Kiciński wrote: >> As mentioned by Noralf UART1 is quite commonly used on Compute Modules. >> Proper driver - perhaps modelled as a bus - seems like a prerequisite >> for this work. You are also not using IRQ mux in DT binding example >> which is very misleading. > > Well - there is no support far for uart1 in upstream as of now. > And I am not even sure if the compute module is supported as there > is no device tree available in upstream for that... IIRC I've run the CM with the RPi B DT. I've been meaning to upstream explicit DTs for all the various RPi models, but haven't manage to get around to that yet. > So I have taken the simple route of using shared interrupts and providing > a minimal framework to enable the spi1/spi2 hw blocks with proper locking. > And I have mentioned that it would need to get modified when uart1 support > comes along. > > If you know of a better solution how to control this and that also > incorporates a patch to enable uart1, then please share it! I haven't looked at the registers to be sure of the structure and hence if this is good advice, but creating an IRQ controller driver for the aux registers (as Mark intimated) sounds like a good idea. > Just modify the bcm2835aux_spi_enable/disable to use a different framework > than just bcm2835_aux_modify_enable. > > The patch Noralf mentions only applies downstream and only sets the > enable-register during the boot sequence, so it would not impact the > solution as is. > >> Do we have any indication weather this AUX hardware is available on >> RPi2 as well? IIRC bcm2836 differs only in CPU cores, peripherals >> remain identical. Perhaps this driver could be used on RPi2 and >> naming it 283x would be more appropriate? > > I have not checked, but I would guess that it exists. > > As for naming: I have asked around and bcm2835aux seemed fine. > Also if we go that route we should start renaming ALL driver instances that > contain 2835. I'd suggest leaving named after the "first" chip the driver supports, in a similar fashion to how DT compatible values work. There's zero benefit from renaming existing drivers, and having new drivers continue with the existing naming convention would leave everything nice and consistent. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] dt: brcm,bcm2835-aux-spi: add binding documentation for new spi-bcm2835aux
On 06/25/2015 04:54 AM, ker...@martin.sperl.org wrote: > From: Martin Sperl Patch description? I'd suggest deriving this from the first paragraph in the binding doc. > diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm2835-aux-spi.txt > b/Documentation/devicetree/bindings/spi/brcm,bcm2835-aux-spi.txt > +The BCM2835 contains two forms of SPI master controller, one known simply as > +SPI0, and the other known as the "Universal SPI Master"; part of the > +auxiliary block. This binding applies to the SPI1/2 controller. Rather than "SPI1/2", I'd say "this universal SPI master", since the description of the two types of controller doesn't mention that SPI1/2 are the universal SPI master. > +- reg: Should contain register location and length for the spi block > + as well as for the common aux block control Sharing a reg entry between multiple devices almost always turns out to be a mistake. At the very least, the drivers can't claim the region since regions can't be claimed multiple times. Is there any way to avoid this entirely? If not, perhaps you need to create a separate driver to manage the shared register block. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] irqchip: Add bcm2836 interrupt controller for Raspberry Pi 2.
On 07/07/2015 03:13 PM, Eric Anholt wrote:
> This interrupt controller is the new root interrupt controller with
> the timer, PMU events, and IPIs, and the bcm2835's interrupt
> controller is chained off of it to handle the peripherals.
>
> SMP IPI support was mostly written by Andrea Merello, while I wrote
> most of the rest of the IRQ handling.
>
> Signed-off-by: Andrea Merello
> Signed-off-by: Eric Anholt
I'd expect the git patch author to be Andrea if he wrote the original
patch and you enhanced it.
> diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
> +struct arm_local_intc {
> + struct irq_domain *domain;
> + void __iomem *base;
> +};
> +
> +static struct arm_local_intc intc __read_mostly;
It'd be nice to give everything (types, functions, variables) a
consistent symbol prefix; bcm2836_arm_irqchip_ sounds like a good
bikeshed to me, but perhaps just propagating the above arm_local_ to the
functions too would be good, although that seems to risk symbol name
collisions with other ARM SoCs.
> +static void bcm2836_mask_per_cpu_irq(unsigned int reg, unsigned int bit)
> +{
> + void __iomem *reg_base = intc.base + reg;
> + unsigned int i;
> +
> + for (i = 0; i < 4; i++)
Is "4" there the CPU count? Perhaps this should use one of the Linux
APIs to query the CPU count rather than hard-coding it?
Should per-CPU IRQs automatically be masked on all CPUs at once, or only
on the current CPU? A very quick look at the ARM GIC driver implies it
doesn't iterate over all CPUs when masking per-CPU IRQs.
> +static void bcm2836_mask_gpu_irq(struct irq_data *d)
> +{
> +}
> +
> +static void bcm2836_unmask_gpu_irq(struct irq_data *d)
> +{
> +}
If the IRQs can't be masked, should these functions actually be implemented?
> +static void __exception_irq_entry bcm2836_handle_irq(struct pt_regs *regs)
> +{
> + int cpu = smp_processor_id();
> + u32 stat;
> +
> + stat = readl_relaxed(intc.base + LOCAL_IRQ_PENDING0 + 4 * cpu);
> + if (stat & 0x10) {
> + void __iomem *mailbox0 = (intc.base +
> + LOCAL_MAILBOX0_CLR0 + 16 * cpu);
> + u32 mbox_val = readl(mailbox0);
> + u32 ipi = ffs(mbox_val) - 1;
> +
> + writel(1 << ipi, mailbox0);
> + handle_IPI(ipi, regs);
Given that bcm2836_send_ipi() is #ifdef CONFIG_SMP, should this code be too?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] irqchip: Add documentation for the bcm2836 interrupt controller.
On 07/07/2015 03:13 PM, Eric Anholt wrote:
> This is a new per-cpu root interrupt controller on the Raspberry Pi 2,
> which will chain to the bcm2835 interrupt controller for peripheral
> interrupts.
> diff --git
> a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt
>
> b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.txt
> +local_intc: local_intc {
> + interrupt-parent = <&local_intc>;
I think that property shouldn't be there?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2
On 06/22/2015 07:40 AM, ker...@martin.sperl.org wrote:
> From: Martin Sperl
>
> This driver does NOT make use of native chip-selects but uses the
> generic cs-gpios facilities provided by the framework allowing for
> more than 3 chip-selects.
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> +/*
> + * spi register defines
> + *
> + * note there is garbage in the "official" documentation,
> + * so somedata taken from the file:
> + * brcm_usrlib/dag/vmcsx/vcinclude/bcm2708_chip/aux_io.h
> + * inside of:
> + *
> http://www.broadcom.com/docs/support/videocore/Brcm_Android_ICS_Graphics_Stack.tar.gz
> + */
Hopefully the license of that tar file allows for that. I didn't look.
> +DEFINE_SPINLOCK(__bcm2835_aux_lock);
> +static void bcm2835_aux_modify_enable(struct bcm2835aux_spi *bs,
> + u32 mask, u32 val)
> +{
> + unsigned long flags;
> + u32 r;
> +
> + spin_lock_irqsave(&__bcm2835_aux_lock, flags);
> +
> + r = readl(bs->aux_regs + BCM2835_AUX_ENABLE);
> + r &= mask;
> + r |= val;
> + writel(r, bs->aux_regs + BCM2835_AUX_ENABLE);
> +
> + spin_unlock_irqrestore(&__bcm2835_aux_lock, flags);
> +}
As mentioned elsewhere, I'd hope all the shared aux register
manipulations can be pushed into a shared driver.
> +static void bcm2835aux_spi_enable(struct bcm2835aux_spi *bs)
> +{
> + /* identify the spi device - needed to activate the right HW-block */
> + u32 mask = (size_t)bs->regs & 0x0040 ?
> +BCM2835_AUX_BIT_SPI2 : BCM2835_AUX_BIT_SPI1;
> +
> + bcm2835_aux_modify_enable(bs, ~mask, mask);
> +}
I expect that function will go away when my previous comment is
resolved. If not, it'd be nice to encode the "ID" of the device into DT,
so that the driver has no hard-coded idea of what register addresses
exist; what happens when (a hypothetical) bcm2837 comes along with 3
instances of this HW block?
> +static int bcm2835aux_spi_remove(struct platform_device *pdev)
> +{
> + struct spi_master *master = platform_get_drvdata(pdev);
> + struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
> +
> + /* Clear FIFOs, and disable the HW block */
> + clk_disable_unprepare(bs->clk);
You probably want remove() to do things in the reverse order of probe().
In particular, disable the clock after anything that could touch
registers in the HW.
> + bcm2835aux_spi_reset_hw(bs);
> +
> + /* disable HW block */
> + bcm2835aux_spi_disable(bs);
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
On 07/10/2015 10:21 AM, Tomeu Vizoso wrote: On 10 July 2015 at 17:27, Stephen Warren wrote: On 07/10/2015 03:29 AM, Tomeu Vizoso wrote: On 1 July 2015 at 19:36, Rob Herring wrote: On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso wrote: When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if the pin controller isn't available. Otherwise, the GPIO range wouldn't be set at all unless the pin controller probed always before the GPIO chip. With this change, the probe of the GPIO chip will be deferred and will be retried at a later point, hopefully once the pin controller has been registered and probed already. This will break cases where the pinctrl driver does not exist, but the DT contains pinctrl bindings. We can have similar problems already with clocks though. However, IMO this problem is a bit different in that pinctrl is more likely entirely optional while clocks are often required. You may do all pin setup in bootloader/firmware on some boards and not others. Of course then why put pinctrl in the DT in that case? They could be present just due to how chip vs. board dts files are structured. I see. My instinct tells me that it would be better if the gpio-ranges property was set in the board dts, but I don't really know what each mach does with its DTSs. That doesn't make sense; the mapping between GPIO controller pins and pin controller pins is a property of the SoC not the board. From what Rob said above, apparently some boards will rely on the pin setup done by the bootloader, and some other boards with the same soc will want to do it in the kernel. So it's not really a difference in the hw itself, but what expectations exist about the firmware on a specific board. Sure, but none of that changes the mapping between the GPIO and pin controller pins. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
On 07/10/2015 03:29 AM, Tomeu Vizoso wrote: On 1 July 2015 at 19:36, Rob Herring wrote: On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso wrote: When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if the pin controller isn't available. Otherwise, the GPIO range wouldn't be set at all unless the pin controller probed always before the GPIO chip. With this change, the probe of the GPIO chip will be deferred and will be retried at a later point, hopefully once the pin controller has been registered and probed already. This will break cases where the pinctrl driver does not exist, but the DT contains pinctrl bindings. We can have similar problems already with clocks though. However, IMO this problem is a bit different in that pinctrl is more likely entirely optional while clocks are often required. You may do all pin setup in bootloader/firmware on some boards and not others. Of course then why put pinctrl in the DT in that case? They could be present just due to how chip vs. board dts files are structured. I see. My instinct tells me that it would be better if the gpio-ranges property was set in the board dts, but I don't really know what each mach does with its DTSs. That doesn't make sense; the mapping between GPIO controller pins and pin controller pins is a property of the SoC not the board. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] ARM: tegra: Add gpio-ranges property
On 07/01/2015 06:45 AM, Tomeu Vizoso wrote: Specify how the GPIOs map to the pins in Tegra SoCs, so the dependency is explicit. This currently will add a duplicated entry in the map from pins to gpios in the pinmux controller but it should be harmless and will be fixed in a later commit. Isn't it in an earlier commit now (patch 2/3)? :-) At a quick glance, I think this approach will be fine, so the series, Acked-by: Stephen Warren -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree
On 06/23/2015 05:56 AM, Roger Quadros wrote: + Kukjin, Stephen, for board specific USB question. On Tue, 23 Jun 2015 16:35:49 +0800 Li Jun wrote: On Tue, Jun 23, 2015 at 10:43:28AM +0300, Roger Quadros wrote: If the dr_mode was "otg" for such case and we want OTG disabled then it is really the DT fault. It's ID pin detect for dual role switch as many current OTG controllers have. not DT fault, its dt only has a dr_mode = "otg". We don't need to tackle this case. Just fix up the DT to dr_mode = "peripheral" if OTG behaviour is not needed. OTG behaviour is not needed, so we need disable HNP/SRP/ADP. but dr_mode = "otg" as it already works fine with ID pin detect. Do you know which platform has plain non-otg dual-role working as of today with dr_mode set to "otg"? I don't know. The dt property dr_mode is already there, and currently there are some platforms as its user, so I assume those platforms either are non-otg dual-role, or real OTG with HNP supported, I guess most are the former cases; chipidea is the later case. For TI platforms none of them have it working currently. So for Ti platforms, some enables non-otg dual-role function but do not use dr_mode = "otg"? for TI we have only musb based and dwc3 based platforms. MUSB based are OTG. dwc3 doesn't support dual-role or OTG yet so we use either "peripheral" or "Host" For those only have non-otg dual-role platforms, no matter using dr_mode or not, we need keep it's real OTG disabled for legacy users after it's controller driver adds real OTG later. I wouldn't even bother about platforms expecting dual-role behaviour with dr_mode not set to "otg". That is just plain wrong and can't be supported. These are the dts files having dr_mode == "otg". I've sorted them as per Soc vendor and USB controller ... Nvidia -- arm/boot/dts/tegra20-seaboard.dts: dr_mode = "otg"; compatible = "nvidia,tegra20-ehci", "usb-ehci"; arm/boot/dts/tegra30-colibri-eval-v3.dts: dr_mode = "otg"; compatible = "nvidia,tegra30-ehci", "usb-ehci"; I couldn't find gadget side implementation for this. Stephen, any comments on whether this board supports true OTG operation or just dual-role operation? Sorry for the slow reply; I was on vacation. I'm not sure what the difference is? I won't be able to answer for the Colibri board since I'm not familiar with the HW. I have the schematics for Seaboard, so I should be able to answer once I understand the question:-) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH 1/2] pinctrl: change function behavior for per pin muxing controllers
On 06/17/2015 06:38 AM, Ludovic Desroches wrote: Hi Stephen, On Mon, Jun 15, 2015 at 09:58:05AM -0600, Stephen Warren wrote: On 06/10/2015 09:04 AM, Ludovic Desroches wrote: When having a controller which allows per pin muxing, declaring with which groups a function can be used is a useless constraint since groups are something virtual. This isn't true. Irrespective of whether a particular piece of pinmux HW can control the mux function for each pin individually, or only in groups, it's quite likely that each function can only be selected onto a subset of those pins or groups. Requiring the pinctrl driver to inform the core which set of pins/groups particular functions can be selected onto seems quite reasonable. In my opinion at least, for HW that can select the mux function at the per-pin level, the only sensible set of groups is one group per pin with each group containing a single pin. Any other use of groups is a SW/user-level construct, and is something unrelated to why the pinctrl subsystem supports groups. If we want to represent those groups in pinctrl, there should be two separate sets of groups; one to represent the actual HW capabilities, and one to represent the SW/user-level convenience abstractions. Groups that I would like to use are not something related to the user or software. It's an hardware reality but they would be more flexibles. Usually the muxing is done by selecting a function (which seems to be device related: usart, spi, etc.), then you select on which pins you want this function. In my case, I can't select a function only by choosing a mux. It is a combination of the mux and the pin on which it is applied. So my functions will be GPIO, A, B, C, etc. If I use function A on pin 12, I will have my i2c clock signal but I can have this signal on pin 58 if I use function C. Do you understand what I mean? It's not very easy to explain... So here is a real example: -- | | pio peripheral| -- | signal | dir | func | signal | dir | ioset | -- | PA8| I/O | A| SDMMC0_DAT6 | I/O | 1 | || | B| QSPI1_IO1| I/O | 1 | || | D| TCLK5| I | 1 | || | E| FLEXCOM2_IO2 | I/O | 1 | || | F| NWE/NANDWE | O | 2 | -- | PD28 | I/O | A| SPI1_NPCS0 | I/O | 3 | || | B| TDI | I/O | 3 | || | C| FLEXCOM2_IO2 | I | 2 | -- You are right that having a group per pin is a solution. If I follow your proposal (tell me if I'm wrong): - I will have 128 groups since I have 128 pins. Yes. - I will have functions GPIO, A, B, C, D, E, F. You could have functions A..F, and require the user to determine what option they want for each pin, find the pin-specific mux function value for each pin, and put that into the DT (or other pinmux data source). For example, the bcm2835 pinctrl driver works this way. In that case, each of the functions A..F could be selected on each pin, so you'd have a very simple "get pins for function" implementation. Alternatively, you could define a logic function per IO controller or signal that gets pinmuxed. In the example above, FLEXCOM2_IO2 is one such example. Given that set of functions, you'd need a mapping table to convert from the logical functions seen by the pinctrl subsystem to the HW values that need to be written into registers. For example, the Tegra pinctrl drivers work this way. In that case, each (pinctrl) function could only be selected on a specific subset of all pins/groups, and so you'd probably need a table-based implementation of "get pins for function". - I have to give the groups which can achieve a function so I will have 128 groups for each function. It means 128 x 7 = 896 groups. I don't think so no. I'm not sure why you'd consider multiplying 128 and 7 here. I'd expect 128 groups since you have 128 pins[1]. Well, it's possible to have slightly more groups if, say, mux function is selectable per pin, whereas something else like drive strength is configured by a register that affects multiple pins at once. You'd need separate sets of groups for muxing and for drive strength configuration. Some Tegra SoCs are like this. Still, we just add the different sets of groups together here, not multiply. The overall set of groups is not that much larger than the set of pins. Does it seems to be something reasonable and intelligible? From my point of view no. And what about the sysfs readability? With my current implementation, I have something quite understandable for the user if
Re: [PATCH v2 2/3] watchdog: bcm2835: Add poweroff code for the Raspberry Pi
On 06/16/2015 03:39 AM, Noralf Trønnes wrote: > > Den 16.06.2015 05:07, skrev Stephen Warren: >> On 06/13/2015 05:39 AM, Noralf Trønnes wrote: >>> This adds a new poweroff function to the watchdog driver for the >>> Raspberry Pi. Currently poweroff/halt results in a reboot. >>> >>> The Raspberry Pi firmware uses the RSTS register to know which >>> partiton to boot from. The partiton value is spread into bits >>> 0, 2, 4, 6, 8, 10. Partiton 63 is a special partition used by >>> the firmware to indicate halt. >>> >>> The firmware made this change in 19 Aug 2013 and was matched >>> by the downstream commit: >>> Changes for new NOOBS multi partition booting from gsh >> I don't understand why we need a new compatible value here; why not >> simply modify the existing bcm2835_power_off() function. That is written >> to do something that's interpreted by the RPi firmware, not something >> that the bcm2835 HW does. >> >> Admittedly the current name is a bit misleading, but fixing that should >> be a separate change to fixing the implementation to do what the current >> firmware expects. > > There are other boards that use the BCM2835 and I didn't want to break the > behaviour for those that use the reference firmware. We don't support those other board in mainline Linux AFAIK. In other discussions, Eric Anholt stated that the Roku 2 for example doesn't use the same firmware (albeit they were derived from the same base a long way back apparently) so I have no good reason to believe this logic is a standard across difference bcm2835 devices. Do you know more specific details? > Roku 2 device uses > this soc, and changing bcm2835_power_off() would break support for it. > ODROID-W also use BCM2835, but this is a Pi clone so I don't know if they > have matched their firmware behaviour to that of the Pi (admittedly not > many boards were made, their source of chips went dry). -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/21] ARM: tegra: Add gpio-ranges property
On 06/16/2015 02:42 AM, Tomeu Vizoso wrote: On 2 June 2015 at 17:40, Stephen Warren wrote: On 06/02/2015 05:28 AM, Linus Walleij wrote: On Tue, May 26, 2015 at 9:41 PM, Stephen Warren wrote: On 05/25/2015 08:53 AM, Tomeu Vizoso wrote: Specify how the GPIOs map to the pins in T124, so the dependency is explicit. Signed-off-by: Tomeu Vizoso --- arch/arm/boot/dts/tegra124.dtsi | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi index 13cc7ca..5d1d35f 100644 --- a/arch/arm/boot/dts/tegra124.dtsi +++ b/arch/arm/boot/dts/tegra124.dtsi @@ -254,6 +254,7 @@ gpio-controller; #interrupt-cells = <2>; interrupt-controller; + gpio-ranges = <&pinmux 0 0 250>; We should be consistent between SoCs. Why not make the same change for all Tegra SoCs? Agreed. I think this change will cause the GPIO subsystem to call into the pinctrl subsystem and create/add/register a new GPIO<->pinctrl range structure. The pinctrl driver already does this, so I think we'll end up with two duplicate entries in the pinctrl device's gpio_ranges list. This probably won't cause a problem, but I wanted to make sure you'd thought about it to make sure. That sounds like duplication indeed, I would expect that first a patch adds the ranges to the dts[i] files and then a second patch delete the same ranges from the pinctrl driver then, if these shall come in from the device tree. We can't delete the gpio-range-registration code from the Tegra pinmux driver, or old DTs won't work correctly. We could make it conditional based upon whether the DT contains the property or not. I've been looking at this and haven't found a good solution. From what I see, the pinctrl driver doesn't have a reference to the gpio device node so cannot tell if it needs to add the range or not. Well, we know what the node must be called, so the pinctrl driver could search for it by name. The gpio driver can tell whether it should add the range or not, but if it has to because the gpio-ranges property isn't there, then it doesn't have the reference to the pinctrl device to set the range to. So, given that pinctrl_add_gpio_range is deprecated already, wonder if the lesser evil isn't leaving the duplicated entries for now. On newer SoC revisions such as T210 we can stop calling pinctrl_add_gpio_range at all. Or, we can accept that nobody is going to boot a newer kernel with an older DT on the affected boards and just rely on the presence of the gpio-ranges property :) Isn't the simplest solution to just leave it as it is? Nothing's broken is it? For any new SoCs we add, we can certainly switch to a new scheme if we want, but we need to catch/implement that early before the base .dtsi file is included in its first kernel release. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] ARM: dts: bcm2835-rpi: Add "brcm,raspberrypi-pm-wdt" to wdt compatible
On 06/13/2015 05:39 AM, Noralf Trønnes wrote: > The Raspberry Pi uses a new value for halt in the PM_RSTS watchdog > register. Expand the compatible string to cover this. FWIW, the series, Tested-by: Stephen Warren ... but that doesn't imply my ack for the patches. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] watchdog: bcm2835: Add poweroff code for the Raspberry Pi
On 06/13/2015 05:39 AM, Noralf Trønnes wrote: > This adds a new poweroff function to the watchdog driver for the > Raspberry Pi. Currently poweroff/halt results in a reboot. > > The Raspberry Pi firmware uses the RSTS register to know which > partiton to boot from. The partiton value is spread into bits > 0, 2, 4, 6, 8, 10. Partiton 63 is a special partition used by > the firmware to indicate halt. > > The firmware made this change in 19 Aug 2013 and was matched > by the downstream commit: > Changes for new NOOBS multi partition booting from gsh I don't understand why we need a new compatible value here; why not simply modify the existing bcm2835_power_off() function. That is written to do something that's interpreted by the RPi firmware, not something that the bcm2835 HW does. Admittedly the current name is a bit misleading, but fixing that should be a separate change to fixing the implementation to do what the current firmware expects. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: tegra124: pmu support
On 06/15/2015 10:45 AM, Jon Hunter wrote:
Adding linux-tegra ML ...
Oh, still none of the Tegra maintainers are CC'd. scripts/get_maintainer.pl.
On 13/06/15 05:21, Kyle Huey wrote:
This patch modifies the device tree for tegra124 based devices to enable the
Cortex A15 PMU. The interrupt numbers are taken from NVIDIA TRM
DP-06905-001_v03p. This patch was tested on a Jetson TK1.
Signed-off-by: Kyle Huey
---
arch/arm/boot/dts/tegra124.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
+ pmu {
+ compatible = "arm,cortex-a15-pmu";
+ interrupts = ,
+,
+,
+;
+ };
+
timer@0,60005000 {
This looks like it's sorted in the wrong location. Nodes are currently
grouped by those with reg property (sorted by reg) and those without a
reg property (sorted alphabetically).
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH 2/2] pinctrl: introduce complex pin description
On 06/10/2015 09:04 AM, Ludovic Desroches wrote: Using a string to describe a pin in the device tree can be not enough. Some controllers may need extra information to fully describe a pin. It concerns mainly controllers which have a per pin muxing approach which don't fit well the notions of groups and functions. Instead of using a pin name, a 32 bit value is used. The 16 least significant bits are used for the pin number. Other 16 bits can be used to store extra parameters. The driver for the pin controller is supposed to provide this information in a table. The whole point of having a driver, rather than a table/list of raw register values in the DT, is so the driver can provide this information at a semantic level. This information is fixed per SoC and so make sense to put into a driver, while the board-specific configuration varies wildly, and hence makes sense to put into DT. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH 1/2] pinctrl: change function behavior for per pin muxing controllers
On 06/10/2015 09:04 AM, Ludovic Desroches wrote: When having a controller which allows per pin muxing, declaring with which groups a function can be used is a useless constraint since groups are something virtual. This isn't true. Irrespective of whether a particular piece of pinmux HW can control the mux function for each pin individually, or only in groups, it's quite likely that each function can only be selected onto a subset of those pins or groups. Requiring the pinctrl driver to inform the core which set of pins/groups particular functions can be selected onto seems quite reasonable. In my opinion at least, for HW that can select the mux function at the per-pin level, the only sensible set of groups is one group per pin with each group containing a single pin. Any other use of groups is a SW/user-level construct, and is something unrelated to why the pinctrl subsystem supports groups. If we want to represent those groups in pinctrl, there should be two separate sets of groups; one to represent the actual HW capabilities, and one to represent the SW/user-level convenience abstractions. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] get pinctrl more flexible for per pin muxing controllers
On 06/10/2015 01:33 AM, Linus Walleij wrote: On Wed, Jun 3, 2015 at 3:31 PM, Nicolas Ferre wrote: Le 04/05/2015 10:56, Ludovic Desroches a écrit : The way pins, groups and functions are tied is too constraining for some controllers. It concerns mainly the ones we don't care about groups and functions, each pin can be muxed to any functions. The goal of these two patches is too remove some of the constraints. I have added the prototype of a pin controller and device tree to show the way I want to use these changes. I couldn't test it on boards using generic pinconf so I am not sure that I don't break something... Ludovic Desroches (4): pinctrl: change function behavior for per pin muxing controllers pinctrl: introduce complex pin description Linus, Ludovic sent this series nearly one month ago. It was posted after a RFC series on the same topic two months ago. As we don't see any comment on neither of them we assume that it's okay to include them. It's a quite big patch and I need help reviewing it and thinking of some possible consequences. Stephen, can you give me a hand with this? I don't have the patch in my list archive, which goes back 60 days. Judging purely by the patch description, the patch sounds incorrect. There's nothing in pinctrl that prevents a particular pin controller from supporting all mux functions on all pins or groups. Simply return the same list of functions for every pin. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver
On 06/05/2015 01:21 PM, Lee Jones wrote: On Thu, 04 Jun 2015, Stephen Warren wrote: On 06/04/2015 02:11 PM, Eric Anholt wrote: This gives us a function for making mailbox property channel requests of the firmware, which is most notable in that it will let us get and set clock rates. Acked-by: Stephen Warren Does anyone know how drivers/firmware/ is managed? I don't seem to be able to find a maintainer/supporter listed in MAINTAINERS. git log implies lots of different committers. I'd suggest send it through arm-soc with the rest of the series. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver
On 06/04/2015 02:11 PM, Eric Anholt wrote: > This gives us a function for making mailbox property channel requests > of the firmware, which is most notable in that it will let us get and > set clock rates. Acked-by: Stephen Warren -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] ARM: bcm2835: Add a Raspberry Pi-specific clock driver.
On 05/29/2015 03:02 PM, Eric Anholt wrote:
> Stephen Warren writes:
>
>> On 05/18/2015 01:43 PM, Eric Anholt wrote:
>>> +static struct clk *rpi_firmware_delayed_get_clk(struct
>>> of_phandle_args *clkspec, + void *_data)
>>
>>> + rpi_clk = &rpi_clocks[clkspec->args[0]]; + +firmware_node =
>>> of_parse_phandle(of_node, "firmware", 0); + if (!firmware_node)
>>> { + dev_err(dev, "%s: Missing firmware node\n",
>>> rpi_clk->name); + return ERR_PTR(-ENODEV); + } + + /* Try a
>>> no-op transaction to see if the driver is loaded yet. */ + ret
>>> = rpi_firmware_property_list(firmware_node, NULL, 0); + if
>>> (ret) + return ERR_PTR(ret);
>>
>> I would move all that into this driver's probe().
>
> We can't move all this into the driver's probe, because this is
> where we're returning -EPROBE_DEFER. We could potentially do just
> the phandle parse up front and allocate some memory to pass it and
> our own device node to this function through the _data arg, but I
> don't see much point.
Well, once the clock core correctly supports deferred probe, that can
be moved.
Aside from that, I think all your other replies to my replies in this
thread/series make sense.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
