Re: [PATCH v3 3/5] soc: rockchip: add reboot notifier driver
On Tue, Dec 15, 2015 at 09:38:41PM +0100, Arnd Bergmann wrote: > On Tuesday 15 December 2015 18:42:36 Thierry Reding wrote: > > On Tue, Dec 15, 2015 at 05:34:00PM +0100, Arnd Bergmann wrote: > > > On Tuesday 15 December 2015 17:31:22 Thierry Reding wrote: > > > > On Mon, Dec 14, 2015 at 12:39:44PM +0100, Arnd Bergmann wrote: > > > > > On Wednesday 18 November 2015 17:56:22 Andy Yan wrote: > > > > > > rockchip platform have a protocol to pass the kernel reboot > > > > > > mode to bootloader by some special registers when system reboot. > > > > > > By this way the bootloader can take different action according > > > > > > to the different kernel reboot mode, for example, command > > > > > > "reboot loader" will reboot the board to rockusb mode, this is > > > > > > a very convenient way to get the board enter download mode. > > > > > > > > > > > > Signed-off-by: Andy Yan > > > > > > > > > > Adding John Stultz to Cc > > > > > > > > > > I just saw this thread pop up again, and had to think of John's recent > > > > > patch to unify this across platforms. > > > > > > > > > > John, can you have a look at this driver too, and see how it fits in? > > > > > I think this is yet another variant, using an MMIO register rather > > > > > than > > > > > RAM (as HTC / NVIDIA does) or SRAM (as Qualcomm does), but otherwise > > > > > it conceptually fits in with what you had. > > > > > > > > FWIW, Tegra typically does use an MMIO register as well. See > > > > drivers/soc/tegra/pmc.c:tegra_pmc_restart_notify(). I don't know what > > > > HTC does, but if it's writing somewhere in RAM it isn't using the > > > > standard way of resetting the SoC. There's early boot ROM code which I > > > > think evaluates the PMC_SCRATCH0 register on Tegra to determine which > > > > mode to boot into. That's before even any firmware gets the chance of > > > > doing anything. > > > I just checked the android lollipop tree, and I could not find a > pmc_restart_notify > function, only this file > > https://android.googlesource.com/kernel/tegra/+/android-tegra-flounder-3.10-lollipop-release/drivers/htc_debug/stability/reboot_params.c > > with the device that stores into RAM. It looks like HTC ported over > a driver that they were already using on some Qualcomm MSM8960 device, > as in > > https://gitlab.com/MaC/android_kernel_htc_msm8960/blob/859977fc723f59a6b707df1d70e80826ee1dccc4/arch/arm/mach-msm/htc/htc_restart_handler.c > > On Android marshmallow (Flounder), that file again does not exist, and > I don't see how it's done. > > > > HTC apparently uses a separate RAM area to pass the reboot reason, > > > and they have a driver to store that, which is separate from the > > > driver that they use for actually rebooting the machine. > > > > I wasn't very clear, but the PMC_SCRATCH0 register is used to store the > > reset reason. It supports the recovery mode, which I think is really an > > Android thing, "bootloader" will typically cause the bootloader not to > > boot anything, and "forced-recovery" will go into a recovery mode that > > is used to bootstrap the device (usually by uploading a "miniloader" > > that initializes RAM, downloads a bootloader for booting or flashing an > > operating system, ...). > > > > The write that resets the SoC is to a different register. > > So is this scratch register interpreted by some maskrom code, or by code that > can be provided by the OEM? My understanding is that its interpreted both by what's called BootROM on Tegra (I guess that's what you call "maskrom code") and the system's bootloader. The BootROM cannot typically be replaced by the OEM, but it is quite typical for the bootloader to differ between devices. The BootROM will interpret a subset of the bits in that register. Most notable the "force recovery" bit. That will cause the BootROM to go into a mode which can be used to bootstrap the device. It implements a simple protocol (RCM) which can be used to upload a loader (usually referred to as mini-loader) to the device's IRAM which in turn will initialize the memory and which can download a proper bootloader (such as U-Boot) to memory using a slightly more complex protocol (the standard protocol is called nv3p, but the particular choice of protocol no longer matters at this point). The bootloader can also access this register and will interpret the "bootloader" and "recovery" bits. On the, argueably small, sample of Android devices I've worked with, the "bootloader" bit will make the bootloader go into fastboot mode, whereas the "recovery" bit will make it initiate the RCK mode, which is used to update through OTA. There are a couple of other bits in this register, but they are badly documented and don't seem to relate to reboot. Thierry signature.asc Description: PGP signature
Re: [PATCH v2 0/6] pwm: lpc32xx: fixes in the LPC32xx PWM driver
On Sun, Dec 06, 2015 at 01:31:56PM +0200, Vladimir Zapolskiy wrote: > The changeset fixes a number of issues within current implementation > of LPC32xx PWM controller driver, namely > - the SoC has two independent PWM controllers with one channel each, > - runtime warnings from common clock framework, > - overflow in duty cycle calculation may result in disabled PWM, > - unsupported period values are not accepted. > > Correction of PWM channels requires a correspondent change in > arch/arm/boot/dts/lpc32xx.dtsi, it has been already sent for review. > > Changes from v1 to v2: > - corrected style of examples in documentation, > - improved commit messages > > Version v1 can be found here: > http://comments.gmane.org/gmane.linux.pwm/2828 > http://www.spinics.net/lists/devicetree/msg105398.html > > Vladimir Zapolskiy (6): > dt: lpc32xx: pwm: correct LPC32xx PWM device node example > dt: lpc32xx: pwm: update documentation of LPC32xx PWM device > pwm: lpc32xx: correct number of PWM channels from 2 to 1 > pwm: lpc32xx: fix warnings from common clock framework > pwm: lpc32xx: fix and simplify duty cycle and period calculations > pwm: lpc32xx: return ERANGE, if requested period is not supported > > .../devicetree/bindings/pwm/lpc32xx-pwm.txt| 9 +++- > drivers/pwm/pwm-lpc32xx.c | 59 > -- > 2 files changed, 29 insertions(+), 39 deletions(-) All patches applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH 1/2] devicetree: add vendor prefix for Kyocera Corporation
On Wed, Dec 02, 2015 at 07:41:10PM +0100, Lucas Stach wrote: > KYO is the stock ticker symbol of Kyocera Corporation. > > Signed-off-by: Lucas Stach > --- > Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + > 1 file changed, 1 insertion(+) Both patches applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 3/5] soc: rockchip: add reboot notifier driver
On Tue, Dec 15, 2015 at 05:34:00PM +0100, Arnd Bergmann wrote: > On Tuesday 15 December 2015 17:31:22 Thierry Reding wrote: > > On Mon, Dec 14, 2015 at 12:39:44PM +0100, Arnd Bergmann wrote: > > > On Wednesday 18 November 2015 17:56:22 Andy Yan wrote: > > > > rockchip platform have a protocol to pass the kernel reboot > > > > mode to bootloader by some special registers when system reboot. > > > > By this way the bootloader can take different action according > > > > to the different kernel reboot mode, for example, command > > > > "reboot loader" will reboot the board to rockusb mode, this is > > > > a very convenient way to get the board enter download mode. > > > > > > > > Signed-off-by: Andy Yan > > > > > > Adding John Stultz to Cc > > > > > > I just saw this thread pop up again, and had to think of John's recent > > > patch to unify this across platforms. > > > > > > John, can you have a look at this driver too, and see how it fits in? > > > I think this is yet another variant, using an MMIO register rather than > > > RAM (as HTC / NVIDIA does) or SRAM (as Qualcomm does), but otherwise > > > it conceptually fits in with what you had. > > > > FWIW, Tegra typically does use an MMIO register as well. See > > drivers/soc/tegra/pmc.c:tegra_pmc_restart_notify(). I don't know what > > HTC does, but if it's writing somewhere in RAM it isn't using the > > standard way of resetting the SoC. There's early boot ROM code which I > > think evaluates the PMC_SCRATCH0 register on Tegra to determine which > > mode to boot into. That's before even any firmware gets the chance of > > doing anything. > > HTC apparently uses a separate RAM area to pass the reboot reason, > and they have a driver to store that, which is separate from the > driver that they use for actually rebooting the machine. I wasn't very clear, but the PMC_SCRATCH0 register is used to store the reset reason. It supports the recovery mode, which I think is really an Android thing, "bootloader" will typically cause the bootloader not to boot anything, and "forced-recovery" will go into a recovery mode that is used to bootstrap the device (usually by uploading a "miniloader" that initializes RAM, downloads a bootloader for booting or flashing an operating system, ...). The write that resets the SoC is to a different register. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 3/5] soc: rockchip: add reboot notifier driver
On Mon, Dec 14, 2015 at 12:39:44PM +0100, Arnd Bergmann wrote: > On Wednesday 18 November 2015 17:56:22 Andy Yan wrote: > > rockchip platform have a protocol to pass the kernel reboot > > mode to bootloader by some special registers when system reboot. > > By this way the bootloader can take different action according > > to the different kernel reboot mode, for example, command > > "reboot loader" will reboot the board to rockusb mode, this is > > a very convenient way to get the board enter download mode. > > > > Signed-off-by: Andy Yan > > Adding John Stultz to Cc > > I just saw this thread pop up again, and had to think of John's recent > patch to unify this across platforms. > > John, can you have a look at this driver too, and see how it fits in? > I think this is yet another variant, using an MMIO register rather than > RAM (as HTC / NVIDIA does) or SRAM (as Qualcomm does), but otherwise > it conceptually fits in with what you had. FWIW, Tegra typically does use an MMIO register as well. See drivers/soc/tegra/pmc.c:tegra_pmc_restart_notify(). I don't know what HTC does, but if it's writing somewhere in RAM it isn't using the standard way of resetting the SoC. There's early boot ROM code which I think evaluates the PMC_SCRATCH0 register on Tegra to determine which mode to boot into. That's before even any firmware gets the chance of doing anything. Thierry signature.asc Description: PGP signature
Re: [PATCH 1/3] of: Add United Radiant Technology Corporation vendor prefix
On Wed, Oct 07, 2015 at 10:59:53PM +0200, Maciej S. Szmigiero wrote: > This adds vendor prefix for United Radiant Technology Corporation, > a provider of liquid crystal display technologies. > > Signed-off-by: Maciej Szmigiero > --- > Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt > b/Documentation/devicetree/bindings/vendor-prefixes.txt > index 82d2ac9..01e3cee 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt > @@ -223,6 +223,7 @@ toshiba Toshiba Corporation > toumaz Toumaz > tplink TP-LINK Technologies Co., Ltd. > trulyTruly Semiconductors Limited > +urt United Radiant Technology Corporation > usi Universal Scientific Industrial Co., Ltd. > v3 V3 Semiconductor > varisciteVariscite Ltd. Hi Rob, Acked-by on this? Thierry signature.asc Description: PGP signature
Re: [PATCH v5 0/6] MT8173 IOMMU SUPPORT
On Tue, Nov 24, 2015 at 01:58:13PM +0800, Yong Wu wrote: > On Fri, 2015-10-23 at 11:26 +0200, Joerg Roedel wrote: > > On Thu, Oct 22, 2015 at 12:40:02PM +0800, Yong Wu wrote: > > > But the mtk-iommu depend on the drivers/memory/mtk-smi.c(mtk-iommu > > > has called a function of the mtk-smi). > > > So if there is dependence here, How should we do to merge them? > > > > I can surely merge mtk-smi too, if it gets proper Reviewed-by and > > Acked-by tags from the maintainer(s). > > > > > > Joerg > > > Hi Joerg, > >About the driver/memory/, We don't know who is his maintainer. > MAINTAINERS file don't have drivers/memory maintainer. > From the history in drivers/memory/ it looks like most of the > drivers land with an ack from the architecture maintainer. > And Matthias Brugger is our "ARM/Mediatek SoC support" maintainer. > > Then do you mean that we need Matthias's ACK or whom others? Yes, I think the sub-architecture maintainer's ACK is probably going to be as good as it gets. Historically drivers/memory hasn't had enough of a common ground to instate a framework. Thierry signature.asc Description: PGP signature
Re: [PATCH v2 1/2] drm/panel: Add Sharp LS043T1LE01 panel binding documentation
On Fri, Oct 30, 2015 at 03:34:29PM -0700, Bjorn Andersson wrote: > From: Werner Johansson > > Signed-off-by: Werner Johansson > Signed-off-by: Bjorn Andersson > --- > > Change since v1: > - Dropped -vid suffix from compatible > > .../bindings/display/panel/sharp,ls043t1le01.txt | 22 > ++ > 1 file changed, 22 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/panel/sharp,ls043t1le01.txt Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH 2/3] panel/panasonic-vvx10f034n00: Add DT bindings
On Fri, Oct 30, 2015 at 05:38:27PM -0700, bj...@kryo.se wrote: > From: Werner Johansson > > This patch adds bindings for the Panasonic VVX10F034N00 > WUXGA panel. > > Signed-off-by: Werner Johansson > Signed-off-by: Bjorn Andersson > --- > .../bindings/panel/panasonic,vvx10f034n00.txt| 20 > > 1 file changed, 20 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/panel/panasonic,vvx10f034n00.txt Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH] drm/panel: simple: Add support for G121X1-L03
On Wed, Nov 18, 2015 at 03:57:47PM -0500, Akshay Bhat wrote: > Add support for Innolux CheMei 12" G121X1-L03 XGA LVDS display. > > Datasheet: http://www.azdisplays.com/PDF/G121X1-L03.pdf > Signed-off-by: Akshay Bhat > --- > .../bindings/display/panel/innolux,g121x1-l03.txt | 7 + > drivers/gpu/drm/panel/panel-simple.c | 31 > ++ > 2 files changed, 38 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/panel/innolux,g121x1-l03.txt Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 12/13] drm/panel: simple: Add boe,tv080wum-nl0 simple panel device tree binding
On Fri, Nov 20, 2015 at 04:15:38PM +0800, Chris Zhong wrote: > This binding specifies a set of common properties for display panels. It > can be used as a basis by bindings for specific panels. > Bindings for three specific panels are provided to show how the > simple panel binding can be used. > > Signed-off-by: Chris Zhong > Acked-by: Rob Herring > > --- > > Changes in v4: None > Changes in v3: > move boe,tv080wum-nl0.txt to bindings/display/panel/ > > Changes in v2: > As Thierry.Reding comment, add a documentation for this panel. > > .../devicetree/bindings/display/panel/boe,tv080wum-nl0.txt | 7 > +++ > 1 file changed, 7 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/panel/boe,tv080wum-nl0.txt Applied, with a rewritten commit message. The one above seems to be copied from the original commit that added the simple-panel binding. Thanks, Thierry signature.asc Description: PGP signature
Re: [PATCH v4 10/13] of: add vendor prefix for boe
On Fri, Nov 20, 2015 at 04:15:36PM +0800, Chris Zhong wrote: > Signed-off-by: Chris Zhong > Acked-by: Rob Herring > --- > > Changes in v4: None > Changes in v3: None > Changes in v2: None > > Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + > 1 file changed, 1 insertion(+) Applied, with slightly modified commit message. Thanks, Thierry signature.asc Description: PGP signature
Re: [PATCH v3 2/5] dt-bindings: soc: add document for rockchip reboot notifier driver
On Thu, Nov 19, 2015 at 09:39:02PM +0800, Andy Yan wrote: > Hi Thierry: > > 2015-11-19 20:56 GMT+08:00 Thierry Reding : > > > On Thu, Nov 19, 2015 at 09:17:37AM +0800, Andy Yan wrote: > > > Hi Rob: > > > > > > On 2015年11月19日 06:59, Rob Herring wrote: > > > >On Wed, Nov 18, 2015 at 05:53:30PM +0800, Andy Yan wrote: > > > >>Add devicetree binding document for rockchip reboot nofifier driver > > > >Just reading the subject this is way too specific to the Linux driver > > > >needs rather than a h/w description. Please don't create fake DT nodes > > > >just to bind to drivers. Whatever &pmu is is probably what should have > > > >the DT node. Let the driver for it create child devices if you need > > > >that. > > > > > > This is note a fake DT nodes, we really need it to tell the driver > > > which register to use to store the reboot mode. Because rockchip > > > use different register file to store the reboot mode on different > > > platform, on rk3066,rk3188, rk3288,it use one of the PMU register, > > on > > > the incoming RK3036, it use one of the GRF register, and it use > > one of > > > the PMUGRF register for arm64 platform rk3368. On the other hand, > > the > > > PMU/GRF/PMUGRF register file are mapped as "syscon", then referenced > > > by other DT nodes by phandle. So maybe let it as a separate DT node > > > here > > > is better. > > > > In that case you should probably implement a reboot notifier in each of > > the drivers you list and depending on the generation of the SoC. You can > > easily parameterize this by matching on the compatible string. > > > > Thierry > > > > There is no rockchip specific driver for PMU/GRF/PMUGRF register file I > list above， they > use the generic driver “syscon” Well, just go and write specific drivers, then. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 2/5] dt-bindings: soc: add document for rockchip reboot notifier driver
On Thu, Nov 19, 2015 at 09:17:37AM +0800, Andy Yan wrote: > Hi Rob: > > On 2015年11月19日 06:59, Rob Herring wrote: > >On Wed, Nov 18, 2015 at 05:53:30PM +0800, Andy Yan wrote: > >>Add devicetree binding document for rockchip reboot nofifier driver > >Just reading the subject this is way too specific to the Linux driver > >needs rather than a h/w description. Please don't create fake DT nodes > >just to bind to drivers. Whatever &pmu is is probably what should have > >the DT node. Let the driver for it create child devices if you need > >that. > > This is note a fake DT nodes, we really need it to tell the driver > which register to use to store the reboot mode. Because rockchip > use different register file to store the reboot mode on different > platform, on rk3066,rk3188, rk3288,it use one of the PMU register, on > the incoming RK3036, it use one of the GRF register, and it use one of > the PMUGRF register for arm64 platform rk3368. On the other hand, the > PMU/GRF/PMUGRF register file are mapped as "syscon", then referenced > by other DT nodes by phandle. So maybe let it as a separate DT node > here > is better. In that case you should probably implement a reboot notifier in each of the drivers you list and depending on the generation of the SoC. You can easily parameterize this by matching on the compatible string. Thierry signature.asc Description: PGP signature
Re: [PATCH 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support
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. > >+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. > >+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. 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. > >+ > >+ > > 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. Again, those were intentional for readability. I can remove them if you don't think they fulfil that purpose. Thierry signature.asc Description: PGP signature
Re: [PATCH 1/2] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding
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'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 --- > >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. > >+- 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 work, but it'll require a rather large rewrite of... well, everything. I think Martyn was going to look into that, so I guess I'll just drop this for now. Instead I think we'll need a phandle to the XUSB pad controller, so that we can resolve it to the proper context. Something like this perhaps? - nvidia,padctl: phandle to the XUSB pad controller that is used to configure the USB pads used by the XHCI controller. That should of course go into the XHCI controller binding. The nice thing about this would be that we get rid of the circular dependency XHCI -> padctl -> mailbox -> XHCI. > >+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,
Re: [PATCH v4] Tegra: DT: add device tree binding doc for QSPI
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. Thierry signature.asc Description: PGP signature
Re: [PATCH 03/10] pwm: sunxi: Yield some time to the pwm-block to become ready
On Mon, Oct 26, 2015 at 10:32:34PM +0100, Olliver Schinagl wrote: > The pwm-block of some of the sunxi chips feature a 'ready' flag to > indicate the software that it is ready for new commands. > > Right now, when we call pwm_config and set the period, we write the > values to the registers, and turn off the clock to the IP. Because of > this, the hardware does not have time to configure the hardware and set > the 'ready' flag. > > By running the clock just before making new changes and before checking > if the hardware is ready, the hardware has time to reconfigure itself > and set the clear the flag appropriately. > > Signed-off-by: Olliver Schinagl > --- > drivers/pwm/pwm-sun4i.c | 43 +-- > 1 file changed, 25 insertions(+), 18 deletions(-) This looks okay to me (except for one minor thing I noticed, see below), but I'd like an Acked-by from one of the sunxi people. Maxime, any comments on this? > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > index 58ff424..4d84d9d 100644 > --- a/drivers/pwm/pwm-sun4i.c > +++ b/drivers/pwm/pwm-sun4i.c > @@ -104,6 +104,22 @@ static int sun4i_pwm_config(struct pwm_chip *chip, > struct pwm_device *pwm, > u64 clk_rate, div = 0; > unsigned int prescaler = 0; > int err; > + int ret = 0; Why not reuse err? Thierry signature.asc Description: PGP signature
Re: [PATCH 02/10] pwm: sunxi: fix whitespace issue
On Mon, Oct 26, 2015 at 10:32:33PM +0100, Olliver Schinagl wrote: > From: Olliver Schinagl > > This patch changes no code, it just fixes the whitespacing > > Signed-off-by: Olliver Schinagl > --- > drivers/pwm/pwm-sun4i.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied, with a slightly modified commit message. Thanks, Thierry signature.asc Description: PGP signature
Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework
On Fri, Nov 06, 2015 at 04:46:54PM +0100, Olliver Schinagl wrote: > Hey Thierry, > > On 06-11-15 16:18, Thierry Reding wrote: > >On Mon, Oct 26, 2015 at 10:32:39PM +0100, Olliver Schinagl wrote: > >>From: Olliver Schinagl > >> > >>Some hardware PWM's have the possibility to only send out one (or more) > >>pulses. This can be quite a useful feature in case one wants or needs > >>only a single pulse, but at the exact width. > >> > >>Additionally, if multiple pulses are possible, outputting a fixed amount > >>of pulses can be useful for various timing specific purposes. > >I see how theoretically this would be nice to have. But I'm reluctant to > >merge this feature if there aren't any users. What drivers in the kernel > >would want to use this feature? Are there new drivers being worked on > >that will need this? > I should have brought this up as to why I added this, I'm working on a > stepper driver framework (inspired by the pwm framework actually) and > rotating moters by x degree's you do by sending pulses, using controlled > pulses (timing wise) you can precisely move stepper motors. Yes we can do > this reasonably accurate in software, but doing it in hardware is so much > nicer. So is this going to be a kernel framework for stepper motors? If you say you rotate the motors by sending pulses, doesn't that mean that the PWM framework with pulse support would be enough? Or are there dedicated stepper chips that you plan to support, with PWM + pulses being the fallback for when you don't have one of those chips? > >>* pwm_pulse_done() an internal function for drivers to call when > >>they have completed their pre-configured number > >>of pulses > >>* pwm_is_pulsing() tells the callers if the pwm is busy pulsing, > >>yielding a little more information than just > >>pwm_is_enabled() > >Similarily, I'd think that once the PWM is done executing the series of > >pulses that it was configured for it would be automatically disabled. A > >consumer could then simply use pwm_is_enabled() and drivers could call > >pwm_disable() on their PWM to mark them as disabled when they're done > >pulsing. > I agree, pulseating can be dropped too as we know that a) the pulse flag is > set, b) we are enabled. But I'm not sure now if the flag is exported to > sysfs, in any case, sysfs should just check the pulseating flag? Can't you derive that information simply by looking at the enable and pulses attributes? If enable == 1 and pulses > 0 you know the PWM is pulsing. If enable == 1 and pulses == 0 you know it's in regular mode. Thierry signature.asc Description: PGP signature
Re: [PATCH 07/10] pwm: gpio: Add a generic gpio based PWM driver
On Mon, Oct 26, 2015 at 10:32:38PM +0100, Olliver Schinagl wrote:
> From: Olliver Schinagl
>
> This patch adds a bit-banging gpio PWM driver. It makes use of hrtimers,
> to allow nano-second resolution, though it obviously strongly depends on
> the switching speed of the gpio pins, hrtimer and system load.
>
> Each pwm node can have 1 or more "pwm-gpio" entries, which will be
> treated as pwm's as part of a pwm chip.
>
> Signed-off-by: Olliver Schinagl
> ---
> Documentation/devicetree/bindings/pwm/pwm-gpio.txt | 18 ++
> MAINTAINERS| 5 +
> drivers/pwm/Kconfig| 15 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-gpio.c | 253
> +
> 5 files changed, 292 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-gpio.txt
> create mode 100644 drivers/pwm/pwm-gpio.c
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-gpio.txt
> b/Documentation/devicetree/bindings/pwm/pwm-gpio.txt
> new file mode 100644
> index 000..336f61f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-gpio.txt
> @@ -0,0 +1,18 @@
> +Generic GPIO bit-banged PWM driver
> +
> +Required properties:
> + - compatible: should be "pwm-gpio"
> + - #pwm-cells: should be 3, see pwm.txt in this directory for a general
> +description of the cells format.
> + - pwm-gpios: one or more gpios describing the used gpio, see the gpio
> +bindings for the used gpio driver.
I agree with Rob that a single GPIO per PWM chip would be more
straightforward here. Having multiple GPIOs per chip, and hence a
multi-channel chip, suggests that they are in some fashion grouped
whereas in reality they are really completely separate.
> +Example:
> +#include
> +
> + pwm: pwm@0 {
> + compatible = "pwm-gpio";
> + #pwm-cells = 3;
I think the correct syntax for this would be: #pwm-cells = <3>;
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0cfaf6b..c0bc296 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -170,6 +170,21 @@ config PWM_IMG
> To compile this driver as a module, choose M here: the module
> will be called pwm-img
>
> +config PWM_GPIO
> + tristate "Generic GPIO bit-banged PWM driver"
> + depends on OF
> + depends on GPIOLIB
> + help
> + Some platforms do not offer any hardware PWM capabilities but do have
> + General Purpose Input Output (GPIO) pins available. Using the kernels
> + High-Resolution Timer API this driver tries to toggle GPIO using the
> + generic kernel PWM framework. The maximum frequency and/or accuracy
> + is dependent on several factors such as system load and the maximum
> + speed a pin can be toggled at the hardware.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-gpio.
> +
This is oddly sorted. Entries should be ordered alphabetically.
> config PWM_IMX
> tristate "i.MX PWM support"
> depends on ARCH_MXC
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 69b8275..96aa9aa 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_PWM_CLPS711X) += pwm-clps711x.o
> obj-$(CONFIG_PWM_CRC)+= pwm-crc.o
> obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o
> obj-$(CONFIG_PWM_FSL_FTM)+= pwm-fsl-ftm.o
> +obj-$(CONFIG_PWM_GPIO) += pwm-gpio.o
> obj-$(CONFIG_PWM_IMG)+= pwm-img.o
> obj-$(CONFIG_PWM_IMX)+= pwm-imx.o
> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
The ordering is correct here. Perhaps just something that got introduced
by oversight in a rebase?
> diff --git a/drivers/pwm/pwm-gpio.c b/drivers/pwm/pwm-gpio.c
> new file mode 100644
> index 000..8b588fb
> --- /dev/null
> +++ b/drivers/pwm/pwm-gpio.c
> @@ -0,0 +1,253 @@
> +/*
> + * Copyright (c) 2015 Olliver Schinagl
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * This driver adds a high-resolution timer based PWM driver. Since this is a
> + * bit-banged driver, accuracy will always depend on a lot of factors, such
> as
> + * GPIO toggle speed and system load.
I'm not sure this is helpful. The Kconfig snippet already has this
information.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#inc
Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework
On Mon, Oct 26, 2015 at 10:32:39PM +0100, Olliver Schinagl wrote: > From: Olliver Schinagl > > Some hardware PWM's have the possibility to only send out one (or more) > pulses. This can be quite a useful feature in case one wants or needs > only a single pulse, but at the exact width. > > Additionally, if multiple pulses are possible, outputting a fixed amount > of pulses can be useful for various timing specific purposes. I see how theoretically this would be nice to have. But I'm reluctant to merge this feature if there aren't any users. What drivers in the kernel would want to use this feature? Are there new drivers being worked on that will need this? > A few new functions have been expanded or added for this new behavior. > > * pwm_config()now takes an additional parameter to setup the number of > pulses to output. The driver may force this to 0 or 1 > for if example if this feature is not or only partially > supported This is problematic because you need to atomically update all drivers and users (the kbuild robot already told you that you didn't do this). To make things easier I suggest you wait with this change until the atomic PWM patches have been merged, at which point it should become a lot easier to deal with this kind of extension. > * pwm_[sg]et_pulse_count()get or set the number of pulses the pwm > framework is configured for > * pwm_get_pulse_count_max() get the maximum number of pulses the pwm > driver supports > * pwm_pulse() Tell the PWM to emit a pre-configured number of pulses Isn't this essentially the same as pwm_enable()? I'd think that if the PWM is configured to output pulses, then pwm_enable() would simply do what it's been configured to do (emit the pulses). Why the need for an additional function? > * pwm_pulse_done()an internal function for drivers to call when > they have completed their pre-configured number > of pulses > * pwm_is_pulsing()tells the callers if the pwm is busy pulsing, > yielding a little more information than just > pwm_is_enabled() Similarily, I'd think that once the PWM is done executing the series of pulses that it was configured for it would be automatically disabled. A consumer could then simply use pwm_is_enabled() and drivers could call pwm_disable() on their PWM to mark them as disabled when they're done pulsing. > Signed-off-by: Olliver Schinagl > --- > drivers/pwm/core.c | 30 +++ > drivers/pwm/pwm-gpio.c | 3 ++- > drivers/pwm/pwm-sun4i.c | 3 ++- > drivers/pwm/sysfs.c | 58 ++-- > include/linux/pwm.h | 64 > ++--- > 5 files changed, 147 insertions(+), 11 deletions(-) > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 3f9df3e..e2c1c0a 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -432,22 +432,29 @@ EXPORT_SYMBOL_GPL(pwm_free); > * @pwm: PWM device > * @duty_ns: "on" time (in nanoseconds) > * @period_ns: duration (in nanoseconds) of one cycle > + * @pulse_count: number of pulses (periods) to output on pwm_pulse > * > * Returns: 0 on success or a negative error code on failure. > */ > -int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns) > +int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns, > +unsigned int pulse_count) Like I said, this is problematic because every driver and every consumer now needs to be aware of pulsing. Once the PWM atomic patches are merged this will become easier to do because the pulse configuration would be a part of the atomic state, and hence can be conveniently ignored by users and driver alike. Thierry signature.asc Description: PGP signature
Re: [PATCH 06/10] pwm: sysfs: make use of the DEVICE_ATTR_[RW][WO] macro's
On Mon, Oct 26, 2015 at 10:32:37PM +0100, Olliver Schinagl wrote: > From: Olliver Schinagl > > For the npwm property the pwm sysfs interface already made use of the > DEVICE_ATTR_RO macro. This patch expands this to the other sysfs > properties so that the code base is concise and makes use of this > helpful macro. > > This has the advantage of slightly reducing the code size, improving > readability and no longer using magic values for permissions. > > Signed-off-by: Olliver Schinagl > --- > drivers/pwm/sysfs.c | 72 > ++--- > 1 file changed, 36 insertions(+), 36 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH 05/10] pwm: sysfs: do not unnecessarily store result in var
On Mon, Oct 26, 2015 at 10:32:36PM +0100, Olliver Schinagl wrote: > From: Olliver Schinagl > > Use the result of pwm_is_enabled directly instead of storing it first. > > Signed-off-by: Olliver Schinagl > --- > drivers/pwm/sysfs.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH 04/10] pwm: core: use bitops
On Mon, Oct 26, 2015 at 10:32:35PM +0100, Olliver Schinagl wrote: > From: Olliver Schinagl > > The pwm header defines bits manually while there is a nice bitops.h with > a BIT() macro. Use the BIT() macro to set bits in pwm.h > > Signed-off-by: Olliver Schinagl > --- > include/linux/pwm.h | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) I don't think this is a useful change. The BIT() macro needs the same number of characters to type at the expense of requiring an additional include. Thierry signature.asc Description: PGP signature
Re: [PATCH] pwm: pwm-rcar: Revise the device tree binding document about compatible
On Tue, Oct 06, 2015 at 08:28:28PM +0900, Yoshihiro Shimoda wrote: > The compatible should be "renesas,pwm-rcar", and one the the SoC > specific string. So, this patch revises the documentation. > > Reported-by: Rob Herring > Signed-off-by: Yoshihiro Shimoda > --- > Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH] pwm: sun4i: Add support for pwm controller on sun5i SoCs
On Sun, Oct 11, 2015 at 11:49:57AM +0200, Hans de Goede wrote: > The pwm controller on sun5i SoCs is identical to the one found on sun7i > SoCs. On the A13 package only one of the 2 pins is routed to the outside, > so only advertise one pwm there. > > Signed-off-by: Hans de Goede > --- > .../devicetree/bindings/pwm/pwm-sun4i.txt | 2 ++ > drivers/pwm/pwm-sun4i.c| 25 > -- > 2 files changed, 25 insertions(+), 2 deletions(-) Applied, thanks (with s/pwm/PWM/). Thierry signature.asc Description: PGP signature
Re: [PATCH] MAINTAINERS: update DT binding doc locations
On Thu, Nov 05, 2015 at 01:41:15PM -0600, Rob Herring wrote: > After the recent moving of DT binding documents, some maintainers entries > are stale. Update them to the new locations. > > In bindings/fb/, there were only 2 files and I'm assuming the FB > maintainers don't want to be copied on all of bindings/display/. So I've > dropped them. > > Reported-by: Thierry Reding > Cc: Thierry Reding > Cc: Jianwei Wang > Cc: Alison Wang > Cc: Philipp Zabel > Cc: Mark Yao > Cc: Benjamin Gaignard > Cc: Vincent Abriou > Cc: Jean-Christophe Plagniol-Villard > Cc: Tomi Valkeinen > Cc: James Hogan > Cc: Hans de Goede > Cc: Vineet Gupta > Signed-off-by: Rob Herring > --- > MAINTAINERS | 19 ++++++----- > 1 file changed, 10 insertions(+), 9 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
[PATCH 0/2] Add NVIDIA Tegra XUSB pad controller bindings
From: Thierry Reding This is the result of my attempt to come up with a correct binding for the XUSB pad controller found on NVIDIA Tegra SoCs. Note how this is a completely new binding document. The old binding, which we should mark deprecated if we settle on this new one, modelled the device as a pinctrl/PHY hybrid, but it turns out that it's really not much of a pin controller after all. I have a driver, which is pretty much Andrew's work, heavily reworked and put into drivers/phy. I've tested this on Jetson TK1 (for Tegra124) and P2371-2180 (for Tegra210) and I successfully see PCIe, SATA and super-speed USB working on Jetson TK1 as well as super-speed USB on the P2371-2180 (root on NFS and USB 3.0 and USB 2.0 pen drives work fine). Moving the driver into drivers/phy also gives us a trivial way to keep backwards-compatibility by simply keeping the old driver in place and calling into it from the new driver if a legacy DTB is detected. I still need to do a bit of squashing and sorting of the driver patches but I wanted to get this out now because I know Stephen is working on a set of patches to support the XUSB pad controller on Tegra210 in U-Boot and the discussion can hence continue irrespective of the driver code. Note also that this isn't quite complete yet. I've left out most of the HSIC-specific properties for now because there is no user for those in the upstream kernel, so I didn't have an example nor test-case. Thierry Thierry Reding (2): dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support .../bindings/phy/nvidia,tegra-xusb-padctl.txt | 689 + 1 file changed, 689 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt -- 2.5.0 -- 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 2/2] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support
From: Thierry Reding
Extend the binding to cover the set of feature found in Tegra210.
Signed-off-by: Thierry Reding
---
.../bindings/phy/nvidia,tegra-xusb-padctl.txt | 330 +
1 file changed, 330 insertions(+)
diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
index 026e924ae54a..e7d7400bc981 100644
--- a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
+++ b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt
@@ -22,6 +22,7 @@ Required properties:
- compatible: Must be:
- "nvidia,tegra124-xusb-padctl": for Tegra124 and Tegra132
+ - "nvidia,tegra210-xusb-padctl": for Tegra210
- 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:
@@ -45,6 +46,44 @@ the pad and any of its lanes, this property must be set to
"okay".
For Tegra124 and Tegra132, the following pads exist: utmi, ulpi, hsic, pcie
and sata. No extra resources are required for operation of these pads.
+For Tegra210, the following pads exist: utmi, hsic, pcie and sata. Below is
+a description of the properties of each pad.
+
+UTMI pad:
+-
+
+Required properties:
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Must contain the following entries:
+ - "trk": phandle and specifier referring to the USB2 tracking clock
+
+HSIC pad:
+-
+
+Required properties:
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Must contain the following entries:
+ - "trk": phandle and specifier referring to the HSIC tracking clock
+
+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
+
+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:
==
@@ -74,6 +113,17 @@ For Tegra124 and Tegra132, the list of valid PHY nodes is
given below:
- sata: sata-0
- functions: "usb3-ss", "sata"
+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"
+
+
Port nodes:
===
@@ -129,6 +179,7 @@ Required properties:
this super-speed USB port to. The range of valid port numbers varies with
the SoC generation:
- 0-2: for Tegra124 and Tegra132
+ - 0-3: for Tegra210
Optional properties:
- nvidia,internal: A boolean property whose presence determines that a port
@@ -142,6 +193,11 @@ ports:
- 2x HSIC: hsic-0, hsic-1
- 2x super-speed USB: usb3-0, usb3-1
+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:
=
@@ -357,3 +413,277 @@ Board file:
};
};
};
+
+Tegra210:
+-
+
+SoC include:
+
+ padctl@0,7009f000 {
+ compatible = "nvidia,tegra210-xusb-padctl";
+ reg = <0x0 0x7009f000 0x0 0x1000>;
+ resets = <&tegra_car 142>;
+ reset-names = "padctl";
+ mboxes = <&xusb_mbox>;
+ mbox-names = "xusb";
+
+ status = "disabled";
+
+ pads {
+ utmi {
+ clocks = <&tegra_car TEGRA210_CLK_USB2_TRK>;
+ clock-names = "trk";
+ status = "disabled";
+
+ utmi-0 {
+ status = "disabled";
+ #phy-cells = <0>;
+ };
+
+ utmi-1 {
+ status = "disabled";
+ #phy-cells = <0>;
+ };
+
+
[PATCH 1/2] dt-bindings: phy: Add NVIDIA Tegra XUSB pad controller binding
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. Signed-off-by: Thierry Reding --- .../bindings/phy/nvidia,tegra-xusb-padctl.txt | 359 + 1 file changed, 359 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt new file mode 100644 index ..026e924ae54a --- /dev/null +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra-xusb-padctl.txt @@ -0,0 +1,359 @@ +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. Each lane can in turn be assigned to one out of a set of +different outputs. A pad contains logic common for all its lanes. Each lane +can additionally 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). + +In addition to per-lane configuration, USB 3.0 ports may require additional +settings on a per-board basis. + +Pads will be represented as children of the top-level XUSB pad controller +device tree node. Each lane exposed by the pad will be represented by its +own subnode and can be referenced by users of the lane using the standard +PHY bindings, as described by the phy-bindings.txt file in this directory. + +Required properties: + +- compatible: Must be: + - "nvidia,tegra124-xusb-padctl": for Tegra124 and Tegra132 +- 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" +- mboxes: Must contain an entry for each entry in mbox-names. +- mbox-names: Must include the following entries: + - "xusb" + + +Pad nodes: +== + +A required child node named "pads" contains a list of subnodes, one for each +of the pads exposed by the XUSB pad controller. Each pad may need additional +resources that can be referenced in its pad node. + +The "status" property is used to enable or disable the use of a pad. If set +to "disabled", the pad will not be used on the given board. In order to use +the pad and any of its lanes, this property must be set to "okay". + +For Tegra124 and Tegra132, the following pads exist: utmi, ulpi, hsic, pcie +and sata. No extra resources are required for operation of these pads. + + +PHY nodes: +== + +Each pad node has one or more children, each representing one of the lanes +controlled by the pad. + +Required properties: + +- status: Defines the operation status of the PHY. Valid values are: + - "disabled": the PHY is disabled + - "okay": the PHY is enabled +- #phy-cells: Should be 0. Since each lane represents a single PHY, there is + no need for an additional specifier. +- nvidia,function: The output function of the PHY. See below for a list of + valid functions per SoC generation. + +For Tegra124 and Tegra132, the list of valid PHY nodes is given below: +- utmi: utmi-0, utmi-1, utmi-2 + - functions: "snps", "xusb", "uart" +- ulpi: ulpi-0 + - functions: "snps", "xusb" +- hsic: hsic-0, hsic-1 + - functions: "snps", "xusb" +- pcie: pcie-0, pcie-1, pcie-2, pcie-3, pcie-4 + - functions: "pcie", "usb3-ss" +- sata: sata-0 + - functions: "usb3-ss", "sata" + +Port nodes: +=== + +A required child node named "ports" contains a list of all the ports exposed +by the XUSB pad controller. Per-port configuration is only required for USB. + +UTMI ports: +--- + +Required properties: +- status: Defines the operation status of the port. Valid values are: + - "disabled": the port is disabled + - "okay": the port is enabled +- mode: A string that determines the mode in which to run the port. Valid + values are: + - "host": for USB host mode + - "device": for USB device mode + - "otg": for USB OTG mode + +Optional properties: +- nvidia,internal: A boolean property whose presence determines that a port + is internal. In the absence of this property the port is considered to be + external. +- vbus-supply: phandle to a regulator supplying the VBUS voltage. + +ULPI ports: +--- + +Optional properties: +- status: Defines the operation status of the port. Valid values are: + - "disabled": the port is disabled + - "okay&
Re: [PATCH 1/8] dt-bindings: consolidate display related bindings
On Thu, Oct 01, 2015 at 05:12:27PM -0500, Rob Herring wrote: > This is a quite large renaming to consolidate display related bindings > into a single "display" directory from various scattered locations of > video, drm, gpu, fb, mipi, and panel. The prior location was somewhat > based on the Linux driver location, but bindings should be independent > of that. Sorry for being so late to comment on this. I hadn't seen the patches before and noticed only because various bindings that I chanced to look at had moved in linux-next. > Signed-off-by: Rob Herring > Cc: Pawel Moll > Cc: Mark Rutland > Cc: Ian Campbell > Cc: Kumar Gala [...] > diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt > b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt > similarity index 100% > rename from Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt > rename to > Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt That's not quite correct in my opinion. host1x is a top-level module that is the parent for a number of hardware blocks, only some of which are display-related. host1x itself has nothing to do with display at all, except perhaps for providing syncpoints for VBLANK counters. It's not a GPU either, though the GPU hardware blocks are children (at least on generations before Tegra124). Other children include hardware blocks that do video encoding and decoding, so aren't really related to display either. I suppose Documentation/devicetree/bindings/bus might be a more adequate location for the host1x bindings, but then we'd need to split out the display related pieces, and possibly refer to the host1x document from them. The gr2d and gr3d could then be kept in the gpu subdirectory. Also the MIPI block is a calibration block that's used for both DSI and CSI lanes, so it's used for both video display and capture. I can't think of a good location for those. Also I notice that this patch doesn't update the MAINTAINERS entries pointing at these documents, so maintainers will now no longer get Cc'ed on patches that modify the bindings. Do you plan on fixing that up your- self or do you expect maintainers to send patches, possibly after further cleanup of the bindings? Thierry signature.asc Description: PGP signature
Re: [PATCH 09/19] drm: sun4i: Add DT bindings documentation
On Fri, Oct 30, 2015 at 11:40:03AM -0500, Rob Herring wrote: > On Fri, Oct 30, 2015 at 9:20 AM, Maxime Ripard [...] > > +Optional properties: > > + - allwinner,tv-encoder: phandle to the TV Encoder in our pipeline > > + - allwinner,panel: phandle to the panel used in our RGB interface > > Use of-graph please. Why? Panels are a really simple resource and a simple phandle is fully capable of describing the relationship. Thierry signature.asc Description: PGP signature
Re: [PATCH 07/19] drm/panel: simple: Add timings for the Olimex LCD-OLinuXino-4.3TS
On Fri, Oct 30, 2015 at 03:20:53PM +0100, Maxime Ripard wrote:
> Add support for the Olimex LCD-OLinuXino-4.3TS panel to the DRM simple
> panel driver.
>
> It is a 480x272 panel connected through a 24-bits RGB interface.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/panel/panel-simple.c | 26 ++
> 1 file changed, 26 insertions(+)
I don't see a patch adding the DT binding documentation for this panel.
Also, the olimex vendor prefix isn't defined.
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index f97b73ec4713..3a9ecb64d1e6 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1096,6 +1096,29 @@ static const struct panel_desc shelly_sca07010_bfn_lnn
> = {
> .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> };
>
> +static const struct drm_display_mode olimex_lcd_olinuxino_43ts_mode = {
> + .clock = 9000,
> + .hdisplay = 480,
> + .hsync_start = 480 + 5,
> + .hsync_end = 480 + 5 + 30,
> + .htotal = 480 + 5 + 30 + 10,
> + .vdisplay = 272,
> + .vsync_start = 272 + 8,
> + .vsync_end = 272 + 8 + 5,
> + .vtotal = 272 + 8 + 5 + 3,
> + .vrefresh = 60,
> +};
> +
> +static const struct panel_desc olimex_lcd_olinuxino_43ts = {
> + .modes = &olimex_lcd_olinuxino_43ts_mode,
> + .num_modes = 1,
> + .size = {
> + .width = 105,
> + .height = 67,
> + },
> + .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> +};
> +
These mode and panel descriptors are all sorted alphabetically (by
vendor, by model), please keep it so.
> static const struct of_device_id platform_of_match[] = {
> {
> .compatible = "ampire,am800480r3tmqwa1h",
> @@ -1191,6 +1214,9 @@ static const struct of_device_id platform_of_match[] = {
> .compatible = "shelly,sca07010-bfn-lnn",
> .data = &shelly_sca07010_bfn_lnn,
> }, {
> + .compatible = "olimex,lcd-olinuxino-43-ts",
> + .data = &olimex_lcd_olinuxino_43ts,
> + }, {
> /* sentinel */
> }
Here as well.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH 1/2] drm/panel: Add Sharp LS043T1LE01 panel binding documentation
On Tue, Jul 28, 2015 at 06:37:40PM -0700, Bjorn Andersson wrote: > From: Werner Johansson > > Signed-off-by: Werner Johansson > Signed-off-by: Bjorn Andersson > --- > .../bindings/panel/sharp,ls043t1le01.txt | 22 > ++ > 1 file changed, 22 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/panel/sharp,ls043t1le01.txt > > diff --git a/Documentation/devicetree/bindings/panel/sharp,ls043t1le01.txt > b/Documentation/devicetree/bindings/panel/sharp,ls043t1le01.txt > new file mode 100644 > index ..758d48d33e46 > --- /dev/null > +++ b/Documentation/devicetree/bindings/panel/sharp,ls043t1le01.txt > @@ -0,0 +1,22 @@ > +Sharp Microelectronics 4.3" qHD TFT LCD panel > + > +Required properties: > +- compatible: should be "sharp,ls043t1le01-qhd-vid" The -vid suffix needs to go away, otherwise the binding looks good to me. Thierry signature.asc Description: PGP signature
Re: [PATCH 2/3] of: add URT UMSH-8596MD-xT panel DT bindings
On Wed, Oct 07, 2015 at 11:00:51PM +0200, Maciej S. Szmigiero wrote: > This patch adds DT bindings for United Radiant Technology > UMSH-8596MD-xT 7.0" WVGA TFT LCD panels. > > Signed-off-by: Maciej Szmigiero > --- > Documentation/devicetree/bindings/panel/urt,umsh-8596md.txt | 12 > 1 file changed, 12 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/panel/urt,umsh-8596md.txt > > diff --git a/Documentation/devicetree/bindings/panel/urt,umsh-8596md.txt > b/Documentation/devicetree/bindings/panel/urt,umsh-8596md.txt > new file mode 100644 > index 000..57c5fa4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/panel/urt,umsh-8596md.txt > @@ -0,0 +1,12 @@ > +United Radiant Technology UMSH-8596MD-xT 7.0" WVGA TFT LCD panel > + > +Supported are LVDS versions (-11T, -19T) and parallel ones > +(-T, -1T, -7T, -20T). > + > +Required properties: > +- compatible: should be one of: > + "urt,umsh-8596md-t", "urt,umsh-8596md-1t", "urt,umsh-8596md-7t", > + "urt,umsh-8596md-11t", "urt,umsh-8596md-19t" or "urt,umsh-8596md-20t". I'd probably list each of these on a single line for slightly more clarity, but no need to respin because of that. Rob, I remember there was some discussion a while back on whether or not there should be a standard way of describing lists of compatible values, do you know if anything was ever concluded on that topic? Thierry signature.asc Description: PGP signature
Re: [PATCH 3/3] drm: panel-simple: implement URT UMSH-8596MD-xT panel support
On Wed, Oct 07, 2015 at 11:02:20PM +0200, Maciej S. Szmigiero wrote: > This patch implements support for United Radiant Technology > UMSH-8596MD-xT 7.0" WVGA TFT LCD panels in DRM panel-simple > driver. > > Signed-off-by: Maciej Szmigiero > --- > This replaces "drm: panel-simple: add URT UMSH-8596MD-xT panel support" > submission. > > drivers/gpu/drm/panel/panel-simple.c | 54 > > 1 file changed, 54 insertions(+) Looks good to me. I'll wait for Rob or anyone else to ack the vendor prefix before merging this. Thierry signature.asc Description: PGP signature
Re: [PATCH][RESEND] drm: panel-simple: add URT UMSH-8596MD-xT panel support
On Mon, Oct 05, 2015 at 05:25:33PM +0200, Maciej S. Szmigiero wrote: > Hi Thierry, > > On 05.10.2015 13:01, Thierry Reding wrote: > >> On 01.09.2015 15:50, Maciej S. Szmigiero wrote: > >>> This patch adds support for United Radiant Technology > >>> UMSH-8596MD-xT 7.0" WVGA TFT LCD panels > >>> (both LVDS and parallel versions) to DRM > >>> panel-simple driver. > >>> > >>> Signed-off-by: Maciej Szmigiero > >>> --- > >>> This is a resend without changes. > >>> > >>> diff --git a/Documentation/devicetree/bindings/panel/urt,umsh-8596md.txt > >>> b/Documentation/devicetree/bindings/panel/urt,umsh-8596md.txt > >>> new file mode 100644 > >>> index 000..2990e6b > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/panel/urt,umsh-8596md.txt > >>> @@ -0,0 +1,11 @@ > >>> +United Radiant Technology UMSH-8596MD-xT 7.0" WVGA TFT LCD panel > >>> + > >>> +Supported are LVDS versions (-11T, -19T) and parallel ones > >>> +(-T, -1T, -7T, -20T). > > > > Please don't use this kind of wildcard compatible values. If these are > > different models then each of them deserves a separate compatible > > string. > > The differences between these revisions are like different maximum backlight > luminance or presence / absence of touch panel. > > None of this changes panel timings - should they be split into different > compatible values anyway? Yes, absolutely. The compatible doesn't only define what the video timings are, it defines the specific piece of hardware. While it is true that the panel-simple driver currently doesn't use any other information the DT compatible value characterizes the full hardware and therefore should take into account all of the device's properties. Presence of a touch panel sounds like a very important property and the maximum backlight brightness might also become important at some ponit. > >>> You might want to > >>> split the DT binding and vendor prefix to separate patches. > >> > >> Do you mean to first submit new vendor prefix then panel patch with docs? > >> Or even docs separately? > > > > This should be three patches: the vendor prefix is usually a separate > > patch and needs an Acked-by from one of the device tree bindings > > maintainers. The binding itself should also be a separate patch and the > > driver changes should come last. > > I will split the patch and first submit DT binding docs. Thanks, Thierry signature.asc Description: PGP signature
Re: [PATCH v7 2/3] pwm: add MediaTek display PWM driver support
On Tue, Aug 18, 2015 at 03:27:54PM +0800, YH Huang wrote: > Add display PWM driver support to modify backlight for MT8173 and MT6595. > The PWM has one channel to control the brightness of the display. > When the (high_width / period) is closer to 1, the screen is brighter; > otherwise, it is darker. > > Signed-off-by: YH Huang > --- > drivers/pwm/Kconfig| 11 +++ > drivers/pwm/Makefile |1 + > drivers/pwm/pwm-mtk-disp.c | 232 > > 3 files changed, 244 insertions(+) > create mode 100644 drivers/pwm/pwm-mtk-disp.c Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH v7 1/3] dt-bindings: pwm: add MediaTek display PWM bindings
On Tue, Aug 18, 2015 at 03:27:53PM +0800, YH Huang wrote: > Document the device-tree binding of MediaTek display PWM. > The PWM has one channel to control the backlight brightness for display. > It supports MT8173 and MT6595. > > Signed-off-by: YH Huang > --- > .../devicetree/bindings/pwm/pwm-mtk-disp.txt | 42 > > 1 file changed, 42 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-mtk-disp.txt Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 0/2] pwm: Broadcom BCM7038 PWM controller (v4)
On Mon, Sep 14, 2015 at 04:47:04PM -0700, Florian Fainelli wrote: > Hi, > > This patch series add PWM support for the Broadcom BCM7xxx > chips which feature one or more PWM controllers capable of > output periods from 148ns to ~622ms using a combination of > variable and fixed frequency settings. > > The controller does not support setting a polarity. > > This is based on Thierry's pwm/next branch. > > Florian Fainelli (2): > Documentation: dt: add Broadcom BCM7038 PWM controller binding > pwm: Add Broadcom BCM7038 PWM controller support I've applied both of these patches with a couple of stylistic changes. Thanks, Thierry signature.asc Description: PGP signature
Re: [PATCH v7 2/2] pwm: Add support for R-Car PWM Timer
On Wed, Sep 30, 2015 at 05:47:53PM +0900, Yoshihiro Shimoda wrote: > This patch adds support for R-Car SoCs PWM Timer. The PWM timer of > R-Car H2 has 7 channels. So, we can use the channels if we describe > device tree nodes. > > Signed-off-by: Yoshihiro Shimoda > Reviewed-by: Simon Horman > --- > drivers/pwm/Kconfig| 11 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-rcar.c | 266 > + > 3 files changed, 278 insertions(+) > create mode 100644 drivers/pwm/pwm-rcar.c Applied (with some bikeshedding changes squashed in), thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH v7 1/2] pwm: Add device tree binding document for R-Car PWM Timer
On Wed, Sep 30, 2015 at 05:47:52PM +0900, Yoshihiro Shimoda wrote: > Add binding document for Renesas PWM Timer on R-Car SoCs. > > Signed-off-by: Yoshihiro Shimoda > Acked-by: Geert Uytterhoeven > Reviewed-by: Simon Horman > --- > .../devicetree/bindings/pwm/renesas,pwm-rcar.txt | 27 > ++ > 1 file changed, 27 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH][RESEND] drm: panel-simple: add URT UMSH-8596MD-xT panel support
On Fri, Oct 02, 2015 at 11:40:16PM +0200, Maciej S. Szmigiero wrote: > Anybody here? > > I've already submitted this patch two times but received no response... > > Maciej Szmigiero > > On 01.09.2015 15:50, Maciej S. Szmigiero wrote: > > This patch adds support for United Radiant Technology > > UMSH-8596MD-xT 7.0" WVGA TFT LCD panels > > (both LVDS and parallel versions) to DRM > > panel-simple driver. > > > > Signed-off-by: Maciej Szmigiero > > --- > > This is a resend without changes. > > > > diff --git a/Documentation/devicetree/bindings/panel/urt,umsh-8596md.txt > > b/Documentation/devicetree/bindings/panel/urt,umsh-8596md.txt > > new file mode 100644 > > index 000..2990e6b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/panel/urt,umsh-8596md.txt > > @@ -0,0 +1,11 @@ > > +United Radiant Technology UMSH-8596MD-xT 7.0" WVGA TFT LCD panel > > + > > +Supported are LVDS versions (-11T, -19T) and parallel ones > > +(-T, -1T, -7T, -20T). Please don't use this kind of wildcard compatible values. If these are different models then each of them deserves a separate compatible string. Thierry signature.asc Description: PGP signature
Re: [PATCH][RESEND] drm: panel-simple: add URT UMSH-8596MD-xT panel support
On Mon, Oct 05, 2015 at 01:33:49AM +0200, Maciej S. Szmigiero wrote: > Hi Emil, > > Thanks for your response, > > On 04.10.2015 12:43, Emil Velikov wrote: > > Hi Maciej, > > > > On 2 October 2015 at 22:40, Maciej S. Szmigiero > > wrote: > >> Anybody here? > >> > >> I've already submitted this patch two times but received no response... > >> > > Seems that the maintainer (Thierry) isn't Cc'ed. > > Yes, he was Cc'ed - see for example > https://patchwork.ozlabs.org/patch/512858/ . Sorry, I never received any of your earlier patches. It's in none of my mailboxes nor was it classified as spam. Even searching by message ID doesn't give me a positive hit. > > You might want to > > split the DT binding and vendor prefix to separate patches. > > Do you mean to first submit new vendor prefix then panel patch with docs? > Or even docs separately? This should be three patches: the vendor prefix is usually a separate patch and needs an Acked-by from one of the device tree bindings maintainers. The binding itself should also be a separate patch and the driver changes should come last. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 0/16] Add Analogix Core Display Port Driver
On Mon, Sep 21, 2015 at 06:27:40PM +0800, Yakir Yang wrote: > Hi Thierry, > > Thanks for your suggest :) > > On 09/21/2015 05:15 PM, Thierry Reding wrote: > >On Mon, Sep 21, 2015 at 04:45:44PM +0800, Yakir Yang wrote: > >>Hi Heiko, > >> > >>On 09/02/2015 10:15 AM, Yakir Yang wrote: > >>>Hi Heiko, > >>> > >>>在 09/02/2015 05:47 AM, Heiko Stuebner 写道: > >>>>Hi Yakir, > >>>> > >>>>Am Dienstag, 1. September 2015, 13:46:11 schrieb Yakir Yang: > >>>>>The Samsung Exynos eDP controller and Rockchip RK3288 eDP > >>>>>controller > >>>>>share the same IP, so a lot of parts can be re-used. I split the common > >>>>>code into bridge directory, then rk3288 and exynos only need to keep > >>>>>some platform code. Cause I can't find the exact IP name of exynos dp > >>>>>controller, so I decide to name dp core driver with "analogix" which I > >>>>>find in rk3288 eDP TRM ;) > >>>>> > >>>>>Beyond that, there are three light registers setting differents bewteen > >>>>>exynos and rk3288. > >>>>>1. RK3288 have five special pll resigters which not indicata in exynos > >>>>>dp controller. > >>>>>2. The address of DP_PHY_PD(dp phy power manager register) are > >>>>>different > >>>>>between rk3288 and exynos. > >>>>>3. Rk3288 and exynos have different setting with AUX_HW_RETRY_CTL(dp > >>>>>debug > >>>>>register). > >>>>> > >>>>>I have verified this series on two kinds of rockchip platform board, > >>>>>one > >>>>>is rk3288 sdk board which connect with a 2K display port monitor, the > >>>>>other > >>>>>is google jerry chromebook which connect with a eDP screen > >>>>>"cnm,n116bgeea2", > >>>>>both of them works rightlly. > >>>>it looks like during the rebase something did go wrong and I found some > >>>>issues > >>>>I mentioned in the replies to individual patches. > >>>> > >>>>I did prepare a branch based on mainline [0] with both the old and the > >>>>new edp > >>>>driver - rk3288_veyron_defconfig build both drivers into the image. > >>>> > >>>>While the old driver still works, I wasn't able to make the new one work > >>>>yet > >>>>... the drm core does find the connector, but not that anything is > >>>>connected > >>>>to it. I'll try to dig deeper tomorrow, but maybe you'll see anything > >>>>interesting before then. > >>>Many thanks for your comment and debug, I would rebase on your > >>>"edp-with-veyron" branch and fix the broken, make sure v6 would > >>>work rightly at least in your side and my side. > >>Just like we talk off line, I guess there are two tricky questions which > >>make analogix_dp just crash/failed on rockchip platform: > >> > >>- One is how to reach a agreement with the common way to register > >>connector. There would be a conflict with Exynos & IMX & Rockchip. > >> On analogix_dp thread, Exynos want to register connector when that > >>connector is ready. > >> On dw_hdmi thread, IMX want to register connector when all component > >> is > >>already. > >> So Exynos & IMX & Rockchip should reach a common way to register > >>connector to fix this issue. > >> > >>- The other is atomic API. > >> The rockchip drm haven't implemented the atomic API, but the original > >>exynos_dp have used the atomic API on connector helper function. That's why > >>analogix_dp just keep crash on your side. > >There's really no reason not to convert Rockchip to atomic. It will have > >to happen eventually anyway. > > Do agree on this point, and I see Tomasz Figa have done some WIP > works on implementing the atomic_commit, maybe would upstream > in further.(https://chromium-review.googlesource.com/#/c/284560/1) > > > >That said, there's another option that would allow you to side-step both > >of the above problems at the same time. If you turn the common code into > >a helper library that should give you enough flexibility to integrate it > >into all existin
Re: [PATCH v4 0/16] Add Analogix Core Display Port Driver
On Mon, Sep 21, 2015 at 04:45:44PM +0800, Yakir Yang wrote: > Hi Heiko, > > On 09/02/2015 10:15 AM, Yakir Yang wrote: > >Hi Heiko, > > > >在 09/02/2015 05:47 AM, Heiko Stuebner 写道: > >>Hi Yakir, > >> > >>Am Dienstag, 1. September 2015, 13:46:11 schrieb Yakir Yang: > >>>The Samsung Exynos eDP controller and Rockchip RK3288 eDP > >>>controller > >>>share the same IP, so a lot of parts can be re-used. I split the common > >>>code into bridge directory, then rk3288 and exynos only need to keep > >>>some platform code. Cause I can't find the exact IP name of exynos dp > >>>controller, so I decide to name dp core driver with "analogix" which I > >>>find in rk3288 eDP TRM ;) > >>> > >>>Beyond that, there are three light registers setting differents bewteen > >>>exynos and rk3288. > >>>1. RK3288 have five special pll resigters which not indicata in exynos > >>>dp controller. > >>>2. The address of DP_PHY_PD(dp phy power manager register) are > >>>different > >>>between rk3288 and exynos. > >>>3. Rk3288 and exynos have different setting with AUX_HW_RETRY_CTL(dp > >>>debug > >>>register). > >>> > >>>I have verified this series on two kinds of rockchip platform board, > >>>one > >>>is rk3288 sdk board which connect with a 2K display port monitor, the > >>>other > >>>is google jerry chromebook which connect with a eDP screen > >>>"cnm,n116bgeea2", > >>>both of them works rightlly. > >>it looks like during the rebase something did go wrong and I found some > >>issues > >>I mentioned in the replies to individual patches. > >> > >>I did prepare a branch based on mainline [0] with both the old and the > >>new edp > >>driver - rk3288_veyron_defconfig build both drivers into the image. > >> > >>While the old driver still works, I wasn't able to make the new one work > >>yet > >>... the drm core does find the connector, but not that anything is > >>connected > >>to it. I'll try to dig deeper tomorrow, but maybe you'll see anything > >>interesting before then. > > > >Many thanks for your comment and debug, I would rebase on your > >"edp-with-veyron" branch and fix the broken, make sure v6 would > >work rightly at least in your side and my side. > > Just like we talk off line, I guess there are two tricky questions which > make analogix_dp just crash/failed on rockchip platform: > > - One is how to reach a agreement with the common way to register > connector. There would be a conflict with Exynos & IMX & Rockchip. > On analogix_dp thread, Exynos want to register connector when that > connector is ready. > On dw_hdmi thread, IMX want to register connector when all component is > already. > So Exynos & IMX & Rockchip should reach a common way to register > connector to fix this issue. > > - The other is atomic API. > The rockchip drm haven't implemented the atomic API, but the original > exynos_dp have used the atomic API on connector helper function. That's why > analogix_dp just keep crash on your side. There's really no reason not to convert Rockchip to atomic. It will have to happen eventually anyway. That said, there's another option that would allow you to side-step both of the above problems at the same time. If you turn the common code into a helper library that should give you enough flexibility to integrate it into all existing users. For example you could leave out the connector registration and let the drivers do that. Similarly since the helpers are only hooked up at registration time you could probably find a way to share the low-level code but again leave it up to the drivers to glue it all together at registration time (drivers could wrap the low-level code with atomic or non-atomic callbacks). This option may also have the benefit of loosening the coupling between DRM drivers and the helper code for this IP, which may be handy in case the drivers diverge again in the future, or ease transitions to new API. Thierry signature.asc Description: PGP signature
Re: [PATCH 00/11] arm: tegra: colibri_t30: fix hdmi, power i2c, wakeup and activate touch
On Fri, Aug 28, 2015 at 05:59:35PM +0200, Marcel Ziswiler wrote:
> This series finally continues on my previous Easter efforts (BTW:
> thanks all for the feedback and all the patches thereof already having
> been applied) and additionally to activating the STMPE811 touch
> controller also fixes HDMI, power I2C and the wake-up key.
>
>
> Marcel Ziswiler (11):
> arm: tegra: colibri_t30: update hardware revisions compatible comment
> arm: tegra: colibri_t30: fix hdmi supply
> arm: tegra: colibri_t30: improve comment about thermal alert pin
> arm: tegra: colibri_t30: add pin muxing for on-module power i2c
> arm: tegra: colibri_t30: fix comment about 3v3 fixed supply
> arm: tegra: colibri_t30: add touch pen interrupt pin muxing
> arm: tegra: colibri_t30: activate stmpe811 touch controller
> arm: tegra: colibri_t30: replace emmc label by comment
> arm: tegra: colibri_t30: fix vendor string of m41t0m6 rtc on eval
> board
> arm: tegra: colibri_t30: add comment concerning sd/mmc for eval board
> arm: tegra: colibri_t30: fix power/wakeup key for eval board
>
> arch/arm/boot/dts/tegra30-colibri-eval-v3.dts | 9 +-
> arch/arm/boot/dts/tegra30-colibri.dtsi| 124
> ++
> 2 files changed, 111 insertions(+), 22 deletions(-)
Applied all of these with fixups for the subject similar to the Apalis
series. I also applied the following on top to consistently align pin
names.
Thanks,
Thierry
--- >8 ---
From e675c68545ffc4c404cc3f4aa2062b34994efdaf Mon Sep 17 00:00:00 2001
From: Thierry Reding
Date: Tue, 15 Sep 2015 10:29:57 +0200
Subject: [PATCH] ARM: tegra: colibri: Properly align pin names
Align pin names on subsequent lines with the first the name of the first
pin in the first line.
Signed-off-by: Thierry Reding
---
arch/arm/boot/dts/tegra30-colibri.dtsi | 72 +-
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/arch/arm/boot/dts/tegra30-colibri.dtsi
b/arch/arm/boot/dts/tegra30-colibri.dtsi
index 67ba9431e386..2d8c58fd9357 100644
--- a/arch/arm/boot/dts/tegra30-colibri.dtsi
+++ b/arch/arm/boot/dts/tegra30-colibri.dtsi
@@ -39,7 +39,7 @@
/* Colibri Backlight PWM */
sdmmc3_dat3_pb4 {
- nvidia,pins = "sdmmc3_dat3_pb4";
+ nvidia,pins = "sdmmc3_dat3_pb4";
nvidia,function = "pwm0";
nvidia,pull = ;
nvidia,tristate = ;
@@ -74,11 +74,11 @@
nvidia,tristate = ;
};
kb_row11_ps3 {
- nvidia,pins = "kb_row11_ps3",
- "kb_row12_ps4",
- "kb_row13_ps5",
- "kb_row14_ps6",
- "kb_row15_ps7";
+ nvidia,pins = "kb_row11_ps3",
+ "kb_row12_ps4",
+ "kb_row13_ps5",
+ "kb_row14_ps6",
+ "kb_row15_ps7";
nvidia,function = "sdmmc2";
nvidia,pull = ;
nvidia,tristate = ;
@@ -86,17 +86,17 @@
/* Colibri SSP */
ulpi_clk_py0 {
- nvidia,pins = "ulpi_clk_py0",
- "ulpi_dir_py1",
- "ulpi_nxt_py2",
- "ulpi_stp_py3";
+ nvidia,pins = "ulpi_clk_py0",
+ "ulpi_dir_py1",
+ "ulpi_nxt_py2",
+ "ulpi_stp_py3";
nvidia,function = "spi1";
nvidia,pull = ;
nvidia,tristate = ;
};
sdmmc3_dat6_pd3 {
- nvidia,pins = "sdmmc3_dat6_pd3",
- "sdmmc3_dat7_pd4";
+ nvidia,pins = "sdmmc3_dat6_pd3",
+ "sdmmc3_dat7_pd4";
nvidia,function =
Re: [PATCH 0/9] arm: tegra: apalis_t30: fix pin mux, hdmi, wakeup and enable hda
On Fri, Aug 28, 2015 at 02:42:27PM +0200, Marcel Ziswiler wrote:
> This series finally continues on my previous Easter efforts (BTW:
> thanks all for the feedback and all the patches thereof already having
> been applied) and additionally to fixing the pin muxing and enabling
> HDA audio also fixes HDMI and the wake-up key.
>
>
> Marcel Ziswiler (9):
> arm: tegra: apalis_t30: update hardware revisions compatibility
> comment
> arm: tegra: apalis_t30: fix hdmi supply
> arm: tegra: apalis_t30: fix pin muxing
> arm: tegra: apalis_t30: add comment concerning emmc
> arm: tegra: apalis_t30: add digital audio pin muxing
> arm: tegra: apalis_t30: enable hda for eval board
> arm: tegra: apalis_t30: set otg dr_mode for eval board
> arm: tegra: apalis_t30: fix backlight pwm comment for eval board
> arm: tegra: apalis_t30: fix power/wakeup key for eval board
>
> arch/arm/boot/dts/tegra30-apalis-eval.dts | 13 +++--
> arch/arm/boot/dts/tegra30-apalis.dtsi | 80
> ---
> 2 files changed, 73 insertions(+), 20 deletions(-)
Applied with minor tweaks to the subjects (to match the "ARM: tegra: "
prefix) and commit messages.
I've also applied the following on top to make pin names consistently
aligned.
Thierry
--- >8 ---
From f330bb512f4f2bff9d7e73f2a39dc265099faefb Mon Sep 17 00:00:00 2001
From: Thierry Reding
Date: Tue, 15 Sep 2015 10:29:57 +0200
Subject: [PATCH] ARM: tegra: apalis: Properly align pins in pinmux
Align pin names on subsequent lines with the first the name of the first
pin in the first line.
Signed-off-by: Thierry Reding
---
arch/arm/boot/dts/tegra30-apalis.dtsi | 152 +-
1 file changed, 76 insertions(+), 76 deletions(-)
diff --git a/arch/arm/boot/dts/tegra30-apalis.dtsi
b/arch/arm/boot/dts/tegra30-apalis.dtsi
index 4a6ca389595c..bf361277fe10 100644
--- a/arch/arm/boot/dts/tegra30-apalis.dtsi
+++ b/arch/arm/boot/dts/tegra30-apalis.dtsi
@@ -58,14 +58,14 @@
/* Apalis BKL1_PWM */
uart3_rts_n_pc0 {
- nvidia,pins = "uart3_rts_n_pc0";
+ nvidia,pins = "uart3_rts_n_pc0";
nvidia,function = "pwm0";
nvidia,pull = ;
nvidia,tristate = ;
};
/* BKL1_PWM_EN#, disable TPS65911 PMIC PWM backlight */
uart3_cts_n_pa1 {
- nvidia,pins = "uart3_cts_n_pa1";
+ nvidia,pins = "uart3_cts_n_pa1";
nvidia,function = "rsvd2";
nvidia,pull = ;
nvidia,tristate = ;
@@ -73,10 +73,10 @@
/* Apalis CAN1 on SPI6 */
spi2_cs0_n_px3 {
- nvidia,pins = "spi2_cs0_n_px3",
- "spi2_miso_px1",
- "spi2_mosi_px0",
- "spi2_sck_px2";
+ nvidia,pins = "spi2_cs0_n_px3",
+ "spi2_miso_px1",
+ "spi2_mosi_px0",
+ "spi2_sck_px2";
nvidia,function = "spi6";
nvidia,pull = ;
nvidia,tristate = ;
@@ -92,10 +92,10 @@
/* Apalis CAN2 on SPI4 */
gmi_a16_pj7 {
- nvidia,pins = "gmi_a16_pj7",
- "gmi_a17_pb0",
- "gmi_a18_pb1",
- "gmi_a19_pk7";
+ nvidia,pins = "gmi_a16_pj7",
+ "gmi_a17_pb0",
+ "gmi_a18_pb1",
+ "gmi_a19_pk7";
nvidia,function = "spi4";
nvidia,pull = ;
nvidia,tristate = ;
@@ -111,7 +111,7 @@
/* Apalis Digital Audio */
clk1_req_pee2 {
- nvidia,pins = "clk1_req_pee2";
+ nvidia,pins = "clk1_req_pee2";
Re: [PATCH] arm: tegra: dtsi whitespace clean-up for tegra20, tegra30 and tegra124
On Thu, Aug 27, 2015 at 11:44:48AM +0200, Marcel Ziswiler wrote: > There were a few cases of eight spaces being used instead of a tab > character plus one case of using two spaces after an equal sign instead > of just one which this patch fixes. > > Signed-off-by: Marcel Ziswiler > --- > > arch/arm/boot/dts/tegra124.dtsi | 2 +- > arch/arm/boot/dts/tegra20.dtsi | 4 ++-- > arch/arm/boot/dts/tegra30.dtsi | 10 +- > 3 files changed, 8 insertions(+), 8 deletions(-) Applied with tiny fixups of the commit message. Thanks, Thierry signature.asc Description: PGP signature
Re: [RFC 0/2] drm/dsi: DSI for devices with different control bus
On Thu, Sep 10, 2015 at 11:45:35AM +0530, Archit Taneja wrote:
>
>
> On 09/08/2015 03:57 PM, Andrzej Hajda wrote:
> >On 09/07/2015 01:46 PM, Archit Taneja wrote:
> >>Thierry,
> >>
> >>On 08/21/2015 11:39 AM, Archit Taneja wrote:
> >>>
> >>>
> >>>On 08/20/2015 05:18 PM, Thierry Reding wrote:
> >>>>On Thu, Aug 20, 2015 at 09:46:14AM +0530, Archit Taneja wrote:
> >>>>>Hi Thierry, Lucas,
> >>>>>
> >>>>>
> >>>>>On 08/19/2015 08:32 PM, Thierry Reding wrote:
> >>>>>>On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote:
> >>>>>>>Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:
> >>>>>>>>On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:
> >>>>>>>>>Hi Thierry, Archit,
> >>>>>>>>>
> >>>>>>>[...]
> >>>>>>>>>>Perhaps a better way would be to invert this relationship.
> >>>>>>>>>>According to
> >>>>>>>>>>your proposal we'd have to have DT like this:
> >>>>>>>>>>
> >>>>>>>>>> i2c@... {
> >>>>>>>>>> ...
> >>>>>>>>>>
> >>>>>>>>>> dsi-device@... {
> >>>>>>>>>> ...
> >>>>>>>>>> dsi-bus = <&dsi>;
> >>>>>>>>>> ...
> >>>>>>>>>> };
> >>>>>>>>>>
> >>>>>>>>>> ...
> >>>>>>>>>> };
> >>>>>>>>>>
> >>>>>>>>>> dsi@... {
> >>>>>>>>>> ...
> >>>>>>>>>> };
> >>>>>>>>>>
> >>>>>>>>>>Inversing the relationship would become something like this:
> >>>>>>>>>>
> >>>>>>>>>> i2c@... {
> >>>>>>>>>> ...
> >>>>>>>>>> };
> >>>>>>>>>>
> >>>>>>>>>> dsi@... {
> >>>>>>>>>> ...
> >>>>>>>>>>
> >>>>>>>>>> peripheral@... {
> >>>>>>>>>> ...
> >>>>>>>>>> i2c-bus = <&i2c>;
> >>>>>>>>>> ...
> >>>>>>>>>> };
> >>>>>>>>>>
> >>>>>>>>>> ...
> >>>>>>>>>> };
> >>>>>>>>>>
> >>>>>>>>>>Both of those aren't fundamentally different, and they both have
> >>>>>>>>>>the
> >>>>>>>>>>disavantage of lacking ways to transport configuration data that
> >>>>>>>>>>the
> >>>>>>>>>>other bus needs to instantiate the dummy device (such as the reg
> >>>>>>>>>>property for example, denoting the I2C slave address or the DSI
> >>>>>>>>>>VC).
> >>>>>>>>>>
> >>>>>>>>>>So how about we create two devices in the device tree and fuse
> >>>>>>>>>>them at
> >>>>>>>>>>the driver level:
> >>>>>>>>>>
> >>>>>>>>>> i2c@... {
> >>>>>>>>>> ...
> >>>>>>>>>>
> >>>>>>>>>> i2cdsi: dsi-device@... {
> >>>>>>>>>> ...
> >>>>>>>>>> };
> >>>>>>>>>>
> >>>>>>>>>> ...
> >>>>>>>>>> };
> >>>>>>>>>>
> >>>>>>>>>> dsi@... {
> >>>>>>>>>> ...
> >>>>>>>>>>
>
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed
On Fri, Sep 04, 2015 at 11:20:03AM +0100, Russell King - ARM Linux wrote: > On Thu, Sep 03, 2015 at 11:04:40AM +0200, Thierry Reding wrote: > > Conversely, if the panel isn't capable of generating an HPD signal, then > > I don't think it would be appropriate to make it a DT property. It would > > be better to hard-code it in the driver, lest someone forget to set the > > property in DT and get stuck with a device that isn't operational. > > There is another way to deal with this: DRM supports the idea of connector > forcing - where you can force the connector to think that it's connected > or disconnected. Yes, this could work well for RGB/LVDS or DSI connectors perhaps. For eDP there is added complexity because the HPD interrupt function is also used to signal loss of link integrity. That is, after receiving an HPD interrupt you are supposed to retrain the link (or at least check the link status to see if the interrupt cause is loss of integrity). While the eDP specification makes HPD optional, it also says that if HPD isn't available, then the source must use polling to monitor link integrity instead. DRM does provide a mechanism for that as well. You can set the connector's ->polled field to DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT and have the core actively poll for the connector status (i.e. call ->detect() every 100 ms). I think use of polling would be more appropriate in case of eDP. > One of the problems is that not many ARM DRM drivers implement it - maybe > it should be a requirement for code to be accepted? :) I suspect that many drivers may roll their own. In fact I'm guilty of that myself. On Tegra we have a default implementation for outputs which will default to the state of an HPD GPIO where available and fall back to "always connected" for outputs that have a panel connected. Outputs that have a separate mechanism to signal hotplug detection (such as DP) simply use a custom ->detect() implementation. The overhead of rolling one's own is almost zero and connector forcing has the disadvantage of being available via sysfs and debugfs, so the default set by drivers could be overwritten by users at runtime with no easy way back. Given the above I'm not sure enforcing connector forcing would be beneficial. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed
On Sun, Sep 06, 2015 at 04:20:39PM +0800, Yakir Yang wrote: > Hi Rob, > > 在 09/05/2015 05:46 AM, Rob Herring 写道: > >On Wed, Sep 2, 2015 at 11:27 PM, Yakir Yang wrote: > >>Hi Rob, > >> > >>在 09/03/2015 04:17 AM, Rob Herring 写道: > >>>On Tue, Sep 1, 2015 at 1:14 AM, Yakir Yang wrote: > Some edp screen do not have hpd signal, so we can't just return > failed when hpd plug in detect failed. > >>>This is a property of the panel (or connector perhaps), so this > >>>property should be located there. At least, it is a common issue and > >>>not specific to this chip. We could have an HDMI connector and failed > >>>to hook up HPD for example. A connector node is also where hpd-gpios > >>>should be located instead (and are already defined by > >>>../bindings/video/hdmi-connector.txt). Perhaps we need a eDP connector > >>>binding, too. > >> > >>Yep, I agree with your front point, it is a property of panel, not specific > >>to eDP controller, so this code should handle in connector logic. > >> > >>But another question, if we just leave this property to connector, > >>then we would need to parse this property in analogix_dp-rockchip.c, > >>and then make an hook in analogix_dp_core.c driver. This is not nice, > >>and if there are some coming platform which alse want to use analogix_dp > >>code and meet this "no-hpd" situation, then they would need duplicate > >>to parse this property and fill the hook in analogix_dp_core.c driver. > >>So it's little bit conflict :-) > >Ideally, you would be able to just retrieve this quirk from the > >connector or panel. Getting this property from your node vs. the port > >you are attached to should not be much harder. Either the connector > >struct can have this info or there can be a DT function that can walk > >the graph and get it. Just don't open code the graph traversal in your > >driver. > > > >>Beside I can not understand your example very well. Do you mean > >>that there are some HDMI monitor which also do not have HPD > >>signal (just like some eDP panel do not have hpd too), and then > >>the "hpd-gpios" ? Hmm... I don't know how the "hpd-gpios" want > >>to help in this case, would you mind show some sample code :-D > >I don't know that there is h/w, but it is always possible HPD is not > >hooked up or hooked up incorrectly some how. > > > >If there is no HPD support, then hpd-gpios is not going to help you. > > > >I think there are 3 cases to handle: > >- HPD handled by bridge chip - The bridge driver knows it has this > >capability. No DT properties are needed and no HPD properties on the > >connector node imply the bridge chip should handle HPD functions. > >- HPD handled by GPIO line (or some other block) - Indicated by > >hpd-gpios present > >- No or broken HPD - Indicated by "hpd-force" property. > > Oh, I think the first/second case isn't suitable in this case, my panel > "cnm,n116bgeea2" haven't included HPD signal. Note that if your panel doesn't signal HPD you're supposed to poll the sink to make sure you catch loss of link integrity. I've always found it odd that people would want to save a couple of bucks on the HPD signal conductor when that means that you will consume more power because you need to periodically poll the link for integrity. > >>>Are there any eDP panels which don't have EDID and need panel details in > >>>DT? > >> > >>Oh, I think you want to collect some info that belong to panel > >>property but no indicate in panel EDID message, so those can > >>be collect in eDP connector binding, is it right ? > >Yes, and as Thierry pointed out we may need to know the exact panel even. > > Yeah, just like I reply to Thierry, this is a panel quirk. And he suggest we > should > handle this in panel driver, but I have no idea how to handle this in common > panel > driver. :) Like I said in another subthread, this panel does have an HPD line in a variant that I've used in the past. So if your variant doesn't have the HPD line, we're dealing with quirks within the same family of panels. That would make this some sort of quirk, and I'm not sure if there is a standard way for defining quirks in DT. Rob, do you know of any precedent for this? I suppose we could always add a variant-specific compatible string that would allow this quirk to be encoded in the driver, though I'm not sure if the model string in datasheets has enough detail to tell them apart. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed
On Sun, Sep 06, 2015 at 11:59:08AM +0800, Yakir Yang wrote: > Hi Thierry, > > 在 09/03/2015 05:04 PM, Thierry Reding 写道: > >On Thu, Sep 03, 2015 at 12:27:47PM +0800, Yakir Yang wrote: > >>Hi Rob, > >> > >>在 09/03/2015 04:17 AM, Rob Herring 写道: > >>>On Tue, Sep 1, 2015 at 1:14 AM, Yakir Yang wrote: > >>>>Some edp screen do not have hpd signal, so we can't just return > >>>>failed when hpd plug in detect failed. > >>>This is a property of the panel (or connector perhaps), so this > >>>property should be located there. At least, it is a common issue and > >>>not specific to this chip. We could have an HDMI connector and failed > >>>to hook up HPD for example. A connector node is also where hpd-gpios > >>>should be located instead (and are already defined by > >>>../bindings/video/hdmi-connector.txt). Perhaps we need a eDP connector > >>>binding, too. > >>Yep, I agree with your front point, it is a property of panel, not specific > >>to eDP controller, so this code should handle in connector logic. > > From your description it sounds more like this is in fact a property of > >the panel. Or maybe I should say "quirk". If the panel doesn't generate > >the HPD signal, then that should be a property of the panel, not the > >connector. The eDP specification mandates that connectors have a HPD > >signal, though it allows the "HPD conductor in the connector cable" to > >be omitted if not used by the source. I'd consider the cable to belong > >to the panel rather than the connector, so absence of HPD, either > >because the cable doesn't have the conductor or because the panel does > >not generate the signal, should be a quirk of the panel. > > > >That said you could have a panel that supports HPD connected via a cable > >that doesn't transmit it, so this would be a per-board variant and hence > >should be a device tree property rather than hard-coded in some panel > >driver. > > > >Conversely, if the panel isn't capable of generating an HPD signal, then > >I don't think it would be appropriate to make it a DT property. It would > >be better to hard-code it in the driver, lest someone forget to set the > >property in DT and get stuck with a device that isn't operational. > > Oh, you're right, if it's a cable quirk, then DT property would be okay, if > it > is a problem of panel, then maybe hard-code in driver would be better. > > After look up for the document of panel "innolux,n116bge", I haven't see > any description of hot plug signal, and even not found in PIN ASSIGNMENT. > So I believe it's a panel problem, that's to say it should handle in panel > driver. The datasheet that I have for that panel lists HPD as pin 17. Also I used to have a setup with that panel and I distinctly remember hotplug working just fine. Perhaps this is an issue with a specific variant of the panel? Or perhaps this is indeed a problem with the cable that's connecting the panel to the board. It could be one of those cases where they left out the HPD conductor to save money. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir
On Fri, Sep 04, 2015 at 11:29:30PM +0200, Heiko Stuebner wrote:
> Am Freitag, 4. September 2015, 16:06:02 schrieb Rob Herring:
> > On Tue, Sep 1, 2015 at 3:46 PM, Heiko Stuebner wrote:
> > > Am Dienstag, 1. September 2015, 13:49:58 schrieb Yakir Yang:
> > >> Split the dp core driver from exynos directory to bridge
> > >> directory, and rename the core driver to analogix_dp_*,
> > >> leave the platform code to analogix_dp-exynos.
> > >>
> > >> Signed-off-by: Yakir Yang
> > >
> > > [...]
> > >
> > >> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
> > >> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c similarity index 50%
> > >> rename from drivers/gpu/drm/exynos/exynos_dp_core.c
> > >> rename to drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > >> index bed0252..7d62f22 100644
> > >> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> > >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > >
> > > [...]
> > >
> > >> connector->polled = DRM_CONNECTOR_POLL_HPD;
> > >>
> > >> ret = drm_connector_init(dp->drm_dev, connector,
> > >>
> > >> - &exynos_dp_connector_funcs,
> > >> + &analogix_dp_connector_funcs,
> > >>
> > >>DRM_MODE_CONNECTOR_eDP);
> > >>
> > >> if (ret) {
> > >>
> > >> DRM_ERROR("Failed to initialize connector with drm\n");
> > >> return ret;
> > >>
> > >> }
> > >>
> > >> - drm_connector_helper_add(connector,
> > >> &exynos_dp_connector_helper_funcs); +
> > >> drm_connector_helper_add(connector,
> > >> + &analogix_dp_connector_helper_funcs);
> > >>
> > >> drm_connector_register(connector);
> > >
> > > this should only run on exynos, as we're doing all our connector
> > > registration in the core driver after all components are bound, so I
> > > guess something like>
> > > the following is needed:
> > >if (dp->plat_data && dp->plat_data->dev_type == EXYNOS_DP)
> > >
> > >drm_connector_register(connector);
> >
> > Yuck!
> >
> > Surely there is a better way. From what I've seen of DRM, I have no
> > doubt this is needed, but really a better solution is needed. Surely
> > there can be a more generic way for the driver to determine if it
> > should handle the connector or not. This seems like a common problem
> > including one I have seen. What I'm working on has onchip DSI encoder
> > -> ADV7533 -> HDMI. The DSI encoder can also have a direct attached
> > panel. So I have to check for a bridge in the encoder driver and only
> > register the connector for the panel if a bridge is not attached.
>
> I'm also only a part-time drm meddler, so things may be inaccurate.
>
> This conditional is not meant to prevent the registration of the dp
> connector,
> only delay it for non-exynos drms. The connector registration does publish it
> to userspace, so like i.MX we're doing that for all connectors at the same
> time after all components are bound - to also make x11 happy. Exynos on the
> other hand seems to register its connectors individually and I'm not sure if
> they would be willing to change that.
>
> see d3007dabeff4 ("drm/rockchip: register all connectors after bind") or
> simply rockchip_drm_drv.c line 178.
>
> and e355e7dd607b ("imx-drm: delay publishing sysfs connector entries")
There really shouldn't be a reason for both drivers to behave
differently in this case. Gustavo has done a lot of work on cleaning up
the Exynos driver lately, so perhaps you should sync up with him to see
if this is something that he can integrate with his changes.
If you can't reach an agreement and Exynos must keep the exact behaviour
as it has now, then perhaps a better option would be to have the
Analogix core driver not register the connector but rather leave that up
to users of the helpers. Then you don't have to clutter the core driver
with this type of platform-specific data.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed
On Thu, Sep 03, 2015 at 04:55:59PM -0500, Rob Herring wrote: > On Thu, Sep 3, 2015 at 3:47 AM, Thierry Reding wrote: > > On Wed, Sep 02, 2015 at 03:17:57PM -0500, Rob Herring wrote: > > [...] > >> Are there any eDP panels which don't have EDID and need panel details in > >> DT? > > > > Most panels need information other than EDID. They typically have some > > requirements regarding the power up sequence that aren't to be found > > anywhere in EDID or detectable by some other mechanism. A decision was > > therefore made a long time ago to require panels to be listed in DT with > > a specific compatible string. That way all of these details can be > > stashed away in drivers that know how to deal with these kinds of > > details. > > I guess I was being hopeful that eDP was improving that situation. Unfortunately not. eDP helps with a number of things (DPCD and link training take care of a lot of the issues), but it doesn't provide a solution for everything. To my knowledge in most cases the power up sequence isn't strictly necessary to get the panel to operate. But there have been instances where this is absolutely required if you want to avoid visual artifacts as you set a mode. A lot of panels that I've come across require receiving a couple of frames before they guarantee that something will actually be displayed on the screen. So if your code is too quickly enabling backlight, and your backlight doesn't happen to have just the right amount of ramp-up time you might end up seeing garbage on the screen for a couple of frames. It would be great if somebody came up with, say, an EDID extension to describe these requirements, but I'm not sure if even that would give us panels that could be driven with a generic driver. Often the power supplies or reset/enable signals are very different across panels. So describing it all generically would become messy rather quickly. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed
On Thu, Sep 03, 2015 at 12:27:47PM +0800, Yakir Yang wrote: > Hi Rob, > > 在 09/03/2015 04:17 AM, Rob Herring 写道: > >On Tue, Sep 1, 2015 at 1:14 AM, Yakir Yang wrote: > >>Some edp screen do not have hpd signal, so we can't just return > >>failed when hpd plug in detect failed. > >This is a property of the panel (or connector perhaps), so this > >property should be located there. At least, it is a common issue and > >not specific to this chip. We could have an HDMI connector and failed > >to hook up HPD for example. A connector node is also where hpd-gpios > >should be located instead (and are already defined by > >../bindings/video/hdmi-connector.txt). Perhaps we need a eDP connector > >binding, too. > > Yep, I agree with your front point, it is a property of panel, not specific > to eDP controller, so this code should handle in connector logic. From your description it sounds more like this is in fact a property of the panel. Or maybe I should say "quirk". If the panel doesn't generate the HPD signal, then that should be a property of the panel, not the connector. The eDP specification mandates that connectors have a HPD signal, though it allows the "HPD conductor in the connector cable" to be omitted if not used by the source. I'd consider the cable to belong to the panel rather than the connector, so absence of HPD, either because the cable doesn't have the conductor or because the panel does not generate the signal, should be a quirk of the panel. That said you could have a panel that supports HPD connected via a cable that doesn't transmit it, so this would be a per-board variant and hence should be a device tree property rather than hard-coded in some panel driver. Conversely, if the panel isn't capable of generating an HPD signal, then I don't think it would be appropriate to make it a DT property. It would be better to hard-code it in the driver, lest someone forget to set the property in DT and get stuck with a device that isn't operational. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed
On Wed, Sep 02, 2015 at 03:17:57PM -0500, Rob Herring wrote: [...] > Are there any eDP panels which don't have EDID and need panel details in DT? Most panels need information other than EDID. They typically have some requirements regarding the power up sequence that aren't to be found anywhere in EDID or detectable by some other mechanism. A decision was therefore made a long time ago to require panels to be listed in DT with a specific compatible string. That way all of these details can be stashed away in drivers that know how to deal with these kinds of details. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 09/16] drm: rockchip: add bpc and color mode setting
On Wed, Sep 02, 2015 at 06:02:25PM +0800, Yakir Yang wrote: > 在 2015/9/2 16:34, Thierry Reding 写道: [...] > >At the very least your code must compile when applied against a recent > >upstream tree. I would also expect you to make sure the code works at > >runtime, though, contrary to build testing, not everybody will be able > >to verify that you've actually done so. It is ultimately your platform > >maintainer's (i.e. Heiko's) responsibility to ensure that because they > >will get to deal with user complaints if people can't run an upstream > >kernel on the devices. > > Oh, first time to know this rule. So I should work on Heiko's github > kernel branch at the first time to start send upstream. It's usually not necessary to rebase on a specific platform tree. Most platform trees should feed into linux-next anyway, so linux-next would be the appropriate base in almost all cases. Note, though, that that's only true if you expect somebody else to merge your code. The reason is that whoever will end up applying your patches will likely apply to a tree that feeds into linux-next, and that way you both end up having roughly the same base. On the other hand if you are a maintainer yourself you should be keeping a branch based on the latest -rc1. That's especially important if your tree feeds into linux-next, because basing on linux-next will break very horribly that way. So for this particular case I would expect either Mark or Inki to apply these patches when they're ready. Their trees should be based on the latest -rc1. At least the Exynos DRM tree feeds into linux-next, so you should be fine if you use linux-next as a base. Mark, have you ever considered having your tree added to linux-next? I'm beginning to think that we need to make that a requirement for all DRM drivers so that we can resolve integration issues early on rather than Dave having to deal with them when he pulls code in. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 09/16] drm: rockchip: add bpc and color mode setting
On Wed, Sep 02, 2015 at 10:06:36AM +0800, Yakir Yang wrote:
> 在 09/02/2015 05:00 AM, Heiko Stuebner 写道:
> >Am Dienstag, 1. September 2015, 14:01:48 schrieb Yakir Yang:
[...]
> >>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >>b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 34b78e7..5d7f9b6 100644
> >>--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >>@@ -811,14 +811,41 @@ static const struct drm_plane_funcs vop_plane_funcs =
> >>{
> >>
> >> int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
> >> int connector_type,
> >>- int out_mode)
> >>+ int bpc, int color)
> >> {
> >>struct vop *vop = to_vop(crtc);
> >>-
> >>vop->connector_type = connector_type;
> >>vop->connector_out_mode = out_mode;
> >this line should probably go away, as the source var "out_mode" does not
> >exist
> >in the function params any more, making the compilation break, and is set
> >below anyway.
>
> Sorry for the failed, there are must be a problem when I backport those code
> to chrome-3.14 to verify.
>
> Thanks a lot, I would update next version soon.
*sigh*
I get the feeling that you're going about upstreaming the wrong way. If
you post patches to upstream mailing lists and you expect people to
apply those patches, then your target is the upstream kernel. So you
should be basing all of your work on top of a recent release candidate
directly from Linus or some recent version of linux-next.
Your current approach is already making people waste time trying to
build the patches and fail because you've been testing on something
other than mainline or linux-next.
At the very least your code must compile when applied against a recent
upstream tree. I would also expect you to make sure the code works at
runtime, though, contrary to build testing, not everybody will be able
to verify that you've actually done so. It is ultimately your platform
maintainer's (i.e. Heiko's) responsibility to ensure that because they
will get to deal with user complaints if people can't run an upstream
kernel on the devices.
I realize that the upstream kernel isn't what's going to end up in
products later on, but that doesn't change the fact that you're
submitting code for inclusion in the mainline tree. If you need to
backport code to some ChromeOS tree, then that should be done after
you've verified that things build and run upstream. Doing so makes life
a lot easier for your upstream maintainers, and that in turn makes it
more likely to get your code merged.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH 1/7] drm/vc4: Add devicetree bindings for VC4.
On Wed, Aug 26, 2015 at 01:52:29PM +0200, Daniel Vetter wrote: > On Tue, Aug 25, 2015 at 04:42:18PM -0400, Rob Clark wrote: > > On Mon, Aug 24, 2015 at 9:47 AM, Rob Herring wrote: > > > On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt wrote: > > >> Stephen Warren writes: > > >> > > >>> 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 > > >> > > +- 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. > > >> > > >> I've looked at tegra, and the component system used by msm appears to be > > >> nicer than it. To follow tegra's model, it looks like I need to build > > >> this extra bus thing corresponding to host1x that is effectively the > > >> drivers/base/component.c code, so that I can get at vc4's structure from > > >> the component drivers. > > >> > > >>> 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? > > >> > > >> It's the subsystem, same as we use a subsystem node for msm, sti, > > >> rockchip, imx, and exynos. This appears to be the common model of how > > >> the collection of graphics-related components is represented in the DT. > > > > > > I think most of these bindings are wrong. They are grouped together > > > because that is what DRM wants not because that reflects the h/w. So > > > convince me this is one block, not that it is what other people do. > > > > I think, when it comes to more complex driver subsystems (like drm in > > particular) we have a bit of mismatch between how things look from the > > "pure hw ignoring sw" perspective, and the "how sw and in particular > > userspace expects things" perspective. Maybe it is less a problem in > > other subsystems, where bindings map to things that are only visible > > in the kernel, or well defined devices like uart or sata controller. > > But when given the choice, I'm going to err on the side of not > > confusing userspace and the large software stack that sits above > > drm/kms, over dt purity. > > > > Maybe it would be nice to have a sort of dt overlay that adds the bits > > needed to tie together hw blocks that should be assembled into a > > single logical device for linux and userspace (but maybe not some > > other hypothetical operating system). But so far that doesn't exist. > > All we have is a hammer (devicetree), everything looks like a nail. > > End result is we end up adding some things in the bindings which > > aren't purely about the hw. Until someone invents a screwdriver, I'm > > not sure what else to do. In the end, other hypothetical OS is free > > to ignore those extra fields in the bindings if it doesn't need them. > > So meh? > > I thought we agreed a while back that these kind of "pull everything for > the logical device together" dt nodes which just have piles of phandles > are totally accepted? At least that's the point behind the component > helpers, and Eric even suggested to create dt-specific component helpers > to cut down a bit on the usual boilerplate. dt maintainers are also fine > with this approach afaik. From my understanding tegra with the host1x bus > really is the odd one out and not the norm. I agree that in many aspects Tegra is somewhat special. But the same principles that the host1x infrastructure uses could be implemented in a similar way for other DRM drivers. You can easily collect information about subdevices by walking the device tree and matching on known compatible strings. And you can also instantiate the top-level device from driver code rather than have it in DT. It should still be possible to make this work without an artificial device node in DT. The component and master infrastructure is largely orthogonal to that, and as far as I remember the only blocker is the need for a top-level device. I wonder if perhaps this could be made to work by binding the master to the top- level SoC device. Obviously adding the node in DT is easier, but to my knowledge easy has never been a good excuse for mangl
Re: [PATCH 1/7] drm/vc4: Add devicetree bindings for VC4.
On Fri, Aug 14, 2015 at 10:38:54PM -0600, Stephen Warren wrote: > 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. Actually the host1x binding was the first of its kind. Unfortunately for the purposes of this discussion (but fortunately otherwise) Tegra is the odd-ball it seems. host1x is indeed a physical parent of all the devices pertaining to the DRM driver, so the DT description is accurate from a hardware point of view while at the same time giving us a top-level device that we can bind against. Now for most other cases it seems like the central piece that they are missing is this top-level device, hence why the "virtual DRM subsystem device" is instantiated. I tried to argue in the past that it wasn't a proper description and proposed alternatives, but I was always pretty much the only one with this viewpoint, so my comments ended up being ignored. Technically there is nothing that would prevent other drivers from doing without the lists of phandles. On Tegra, again this might be special for this particular hardware, we've never had a need to describe these kinds of relationships. Each display controller can essentially drive each of the outputs, which we deal with elegantly by setting the .possible_crtcs mask of the encoders. Also, to pull together all devices that are needed to make up the DRM device, we use a list of compatible strings in the driver to find these devices. Then as each of them registers with the host1x bus we wait for the subdevice list to become empty and ->probe() the component host1x device. Note that while this predates component/master, this is all very similar in principle (Russell and I did have some discussions about this back at the time, but I'm not sure how much, if anything, he took as inspiration from the host1x infrastructure). After component/master was merged I did try to convert Tegra DRM to use it. Things looked pretty good, but ended up not working because each componentized device must have a unique master device. This poses a problem because on Tegra we needed the top- level (i.e. master) device to be shared among multiple drivers. I posted patches at some point to try and fix remedy the situation but wasn't able to elicit any reactions, and since I had something that was working did not pursue this any further. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Tue, Aug 25, 2015 at 10:03:52PM +0800, Yakir Yang wrote: > Hi Thierry, > > 在 2015/8/25 17:58, Thierry Reding 写道: > >On Wed, Aug 19, 2015 at 09:50:34AM -0500, Yakir Yang wrote: > >[...] > >>+ -analogix,color-space: > >>+ input video data format. > >>+ COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2 > >I don't think DT is an appropriate place to set this. To my knowledge > >this depends on the display and/or mode, so I don't think hard-coding > >it here is the right thing to do. > > Yeah, same question with my previous reply ;) I don't have an answer for you, unfortunately. But like I said, hard-coding isn't going to work. What if, for example, you set this to a fixed value and then you connect a monitor that doesn't support the specific one you set? You cited code from dw_hdmi.c earlier, it looks like it might be correct even though it doesn't cite a reference for why this was done. Perhaps someone on this thread, or someone involved with dw_hdmi can answer where that code came from. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Tue, Aug 25, 2015 at 09:48:01PM +0800, Yakir Yang wrote: > Hi Thierry & Rob, > > 在 2015/8/25 21:27, Rob Herring 写道: > >On Tue, Aug 25, 2015 at 4:15 AM, Thierry Reding wrote: > >>On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote: > >>>On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang wrote: > >>[...] > >>>>+ -analogix,link-rate: > >>>>+ max link rate supported by the eDP controller. > >>>>+ LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = > >>>>0x0A, > >>>>+ LINK_RATE_5_40GBPS = 0x14 > >>>Same here. I'd rather see something like "link-rate-mbps" and use the > >>>actual rate. > >>There is no need whatsoever to hard-code this in DT. (e)DP provides the > >>means to detect what rate the link supports and the specification > >>provides guidance on how to select an appropriate one. > >Good, even better. > > I do think we still need keep this DT prop yet. > > I think drm_dp_help.c could get the "panel" max link-rate and lane-count, > but it's not enough, we still need knew the "eDP controller" max link-rate > and lane-count. > > Let me show the exact example that happened in my side. When I connect > my board to my 2K DP-1.2 TV. Analogix dp driver would get the max link-rate > from dpcd, and the max link-rate is 5.4Gbps. So if I just set eDP controller > link-rate > to 5.4Gbps, the DP TV just broken, do not light up normally. > > This reason why TV broken is the max link-rate which support by RK3288 eDP > controller is 2.7Gbps. Here are the exact words that RK3288 eDP TRM said: > > *Compliant with DisplayPortTM Specification, Version 1.2. > Compliant with eDPTM Specification, Version 1.3. > HDCP v1.3 amendment for DisplayPortTM Revision 1.0. > Main link containing 4 physical lanes of 2.7/1.62 Gbps/lane > * > ** > > > Beside I haven't found there are some registers would indicate the eDP > controller > max link-rate and lane-count, so this is why I still instance that we need > this DT > prop to indicata "Max rate controller support". > > So, I wish you could agree with me on this point. Your driver should know what link rates it supports and restrict itself to use those. This is implied by the compatible string and doesn't need to be duplicated into device tree. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Tue, Aug 25, 2015 at 10:29:39AM +0100, Russell King - ARM Linux wrote: > On Tue, Aug 25, 2015 at 11:12:48AM +0200, Thierry Reding wrote: > > On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote: > > > It goes beyond bindings IMO. The use of the component framework or not > > > has been at the whim of driver writers as well. It is either used or > > > private APIs are created. I'm using components and my need for it > > > boils down to passing the struct drm_device pointer to the encoder. > > > Other components like panels and bridges have different ways to attach > > > to the DRM driver. > > > > I certainly support unification, but it needs to be reasonable. There > > are cases where a different structure for the binding work better than > > another and I think this always needs to be evaluated on a case by case > > basis. > > It can't be a case-by-case basis. > > The TDA998x encoder/connector is going to be component only. This is > a generic chip, which can be attached to the output of any parallel > RGB+sync+clock bus. In other words, it could appear anywhere. > > Are you really saying that we need to support multiple schemes of > attaching the driver to DRM? That's totally insane IMHO. No, what I'm saying is that we should have a single scheme, but one that doesn't put any restrictions on what kind of DT binding you use or how your driver is architected. > The problem with the drm_encoder_slave stuff is that you can't sanely > attach of-nodes to the drm-created i2c device. Yes, you can parse > them from the DT file as a sub-node of the upper device, but that > then goes against the principle of the I2C bindings, which is to > list the I2C devices as a child below the I2C adapter node. If you > try and put the DT node there, then the OF code will create the I2C > device for you, and the drm_encoder_slave stuff won't have the > control it needs to communicate through the wrapped i2c_driver > stuff. > > So, tda998x is going component-only, as that's the _only_ sane solution > for it. Has anyone ever considered turning it into a DRM bridge driver? I had always envisioned component/master to be primarily useful to glue together various SoC components to form one componentized device. Now if tda998x is an I2C slave it is external to the SoC (auxiliary), so in my opinion much better off as a bridge driver. Bridge drivers don't come with any of the disadvantages that the drm_encoder_slave stuff has. They are regular drivers that are probed via their parent busses (I2C, platform, SPI, ...) and hook into DRM via an abstract interface. The DT aspect is taken care of automatically because they get instantiated by their parent bus like any other device. > Now, what happens when some other DRM driver wants to use the tda998x > driver, and its bindings are not compatible with the component helpers? > They're pretty much stuck up the creek without a paddle. I'm sure that will be very helpful response for whoever's going to end up having to deal with that situation. > Case by case doesn't work unless you're talking about truely isolated > hardware where no one shares anything. There are two different things here. The inter-driver interface, which, in my opinion, it makes a lot of sense to standardize. Like I mentioned above I think it unwise to make this interface depend upon a framework or the firmware description such as DT in order to avoid unnecessary restrictions. The second, orthogonal, issue, is the DT bindings. Those I think should absolutely be designed case by case and select whatever most accurately describes the hardware. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Tue, Aug 25, 2015 at 05:41:19PM +0800, Yakir Yang wrote: > Hi Thierry, > > 在 2015/8/25 17:12, Thierry Reding 写道: > >On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote: > >>On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux > >> wrote: > >>>On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote: > >>>>On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang wrote: > >>>>>+ -analogix,color-depth: > >>>>>+ number of bits per colour component. > >>>>>+ COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 > >>>>>= 3 > >>>>This seems pretty generic. Just use 6, 8, 10, or 12 for values. And > >>>>drop the vendor prefix. > >>>Please think about this some more. What does "color-depth" mean? Does it > >>>mean the number of bits per colour _component_, or does it mean the total > >>>number of bits to represent a particular colour. It's confusing as it > >>>stands. > >>Then "component-color-bpp" perhaps? > >There should be no need to have this in DT at all. The BPC is a property > >of the attached panel and it should come from the panel (either the > >panel driver or parsed from EDID if available). > > Actually I have send an email about this one to you in version 2, just past > from that email: > > "samsung,color_space" and "samsung,color-depth" > > The drm_display_info's color_formats and bpc indicate the monitor display > ability, but > the edp driver could not take it as input video format directly. > > For example, with my DP TV I would found "RGB444 & YCRCB422 & & YCRCB444" > support in drm_display_info.color_formats and 16bit bpc support, but RK3288 > crtc > driver could only output RGB & ITU formats, so finally analogix_dp-rockchip > driver > config crtc to RGBaaa 10bpc mode. > > In this sutiation, the analogix_dp core driver would pazzled by the > drm_display_info, > can't chose the right color_space and bpc. > > And this is the place that confused me, wish you could give some ideas about > this one :-) Your display driver should choose whatever it is capable of outputting. If the display reports that it can do 16 bits-per-color, but your display driver can't do it, then it should choose a configuration that it supports. Similarily for the color encodings. If you can't generate YCrCb444 with your hardware, then it's the driver's job to know about that and select the next appropriate configuration. But hard-coding this is not the right solution because the value in DT may end up conflicting with what the display reports. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Wed, Aug 19, 2015 at 09:50:34AM -0500, Yakir Yang wrote: [...] > + -analogix,color-space: > + input video data format. > + COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2 I don't think DT is an appropriate place to set this. To my knowledge this depends on the display and/or mode, so I don't think hard-coding it here is the right thing to do. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote: > On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang wrote: [...] > > + -analogix,link-rate: > > + max link rate supported by the eDP controller. > > + LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A, > > + LINK_RATE_5_40GBPS = 0x14 > > Same here. I'd rather see something like "link-rate-mbps" and use the > actual rate. There is no need whatsoever to hard-code this in DT. (e)DP provides the means to detect what rate the link supports and the specification provides guidance on how to select an appropriate one. > > > + -analogix,lane-count: > > + max number of lanes supported by the eDP contoller. > > + LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4 > > And drop the vendor prefix here. Same as for the link rate. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote: > On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux > wrote: > > On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote: > >> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang wrote: > >> > + -analogix,color-depth: > >> > + number of bits per colour component. > >> > + COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 > >> > = 3 > >> > >> This seems pretty generic. Just use 6, 8, 10, or 12 for values. And > >> drop the vendor prefix. > > > > Please think about this some more. What does "color-depth" mean? Does it > > mean the number of bits per colour _component_, or does it mean the total > > number of bits to represent a particular colour. It's confusing as it > > stands. > > Then "component-color-bpp" perhaps? There should be no need to have this in DT at all. The BPC is a property of the attached panel and it should come from the panel (either the panel driver or parsed from EDID if available). > > When we adopted the graph bindings for iMX DRM, I thought exactly at that > > time "it would be nice if this could become the standard for binding DRM > > components together" but I don't have the authority from either the DT > > perspective or the DRM perspective to mandate that. Neither does anyone > > else. That's the _real_ problem here. > > > > I've seen several DRM bindings go by which don't use the of-graph stuff, > > which means that they'll never be compatible with generic components > > which do use the of-graph stuff. > > It goes beyond bindings IMO. The use of the component framework or not > has been at the whim of driver writers as well. It is either used or > private APIs are created. I'm using components and my need for it > boils down to passing the struct drm_device pointer to the encoder. > Other components like panels and bridges have different ways to attach > to the DRM driver. I certainly support unification, but it needs to be reasonable. There are cases where a different structure for the binding work better than another and I think this always needs to be evaluated on a case by case basis. Because of that I think it makes sense to make all these framework bits opt-in, otherwise we could easily end up in a situation where drivers have to be rearchitected (or even DT bindings altered!) in order to be able to reuse code. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver
On Fri, Aug 21, 2015 at 08:24:16PM +0900, Jingoo Han wrote: > On 2015. 8. 21., at PM 7:01, Yakir Yang wrote: > > > > Hi Jingoo, > > > >> 在 2015/8/21 16:20, Jingoo Han 写道: > >>> On 2015. 8. 19., at PM 11:48, Yakir Yang wrote: > >> > >> . > >> > >>> .../bindings/video/analogix_dp-rockchip.txt| 83 ++ > >>> .../devicetree/bindings/video/exynos_dp.txt| 51 +- > >>> arch/arm/boot/dts/exynos5250-arndale.dts | 10 +- > >>> arch/arm/boot/dts/exynos5250-smdk5250.dts | 10 +- > >>> arch/arm/boot/dts/exynos5250-snow.dts | 12 +- > >>> arch/arm/boot/dts/exynos5250-spring.dts| 12 +- > >>> arch/arm/boot/dts/exynos5420-peach-pit.dts | 12 +- > >>> arch/arm/boot/dts/exynos5420-smdk5420.dts | 10 +- > >>> arch/arm/boot/dts/exynos5800-peach-pi.dts | 12 +- > >>> drivers/gpu/drm/bridge/Kconfig |5 + > >>> drivers/gpu/drm/bridge/Makefile|1 + > >>> drivers/gpu/drm/bridge/analogix_dp_core.c | 1382 > >>> +++ > >>> drivers/gpu/drm/bridge/analogix_dp_core.h | 286 > >>> drivers/gpu/drm/bridge/analogix_dp_reg.c | 1294 > >>> ++ > >>> .../exynos_dp_reg.h => bridge/analogix_dp_reg.h} | 270 ++-- > >>> drivers/gpu/drm/exynos/Kconfig |5 +- > >>> drivers/gpu/drm/exynos/Makefile|2 +- > >>> drivers/gpu/drm/exynos/analogix_dp-exynos.c| 347 + > >> Would you change this file name to "exynos_dp.c"? > > > > Sorry, I don't think so ;( > > > > I think IP_name+Soc_name would be better in this re-use case. > > So? Is there the naming rule such as "IP_name+SoC_name"? > > > Beside I see > > there are lots of driver named with this format in kernel, such as dw_hdmi > > & dw_mmc > > Please look at other dw cases. > For example, look at dw_pcie. > > drivers/pci/host/ > pcie-designware.c > pci-spear13xx.c > pci-exynos.c > > In this case, pci-spear13xx.c and pci-exynos.c do not use "IP_name+SoC_name", > even though these are dw IPs. > > Also, naming consistency is more important. > Now, Exynos DRM files are using "exynos_drm_" prefix. > > drivers/gpu/drm/exynos/ > exynos_drm_buf.c > exynos_drm_core.c > > > However, "analogix_dp-exynos.c" looks very inconsistent. > > If there is no strict naming rule, please use "exynos_dp.c" > or "exynos_drm_dp.c". Exynos DRM maintainers get to pick their filenames, so Yakir, please rename as Jingoo suggested. Even if you didn't the first thing that would go into the Exynos DRM driver tree after this is merged is a rename patch anyway. Thierry signature.asc Description: PGP signature
Re: [PATCH v6 2/2] pwm: Add support for R-Car PWM Timer
On Wed, Aug 19, 2015 at 01:13:59PM +0900, Yoshihiro Shimoda wrote:
> This patch adds support for R-Car SoCs PWM Timer. The PWM timer of
> R-Car H2 has 7 channels. So, we can use the channels if we describe
> device tree nodes.
>
> Signed-off-by: Yoshihiro Shimoda
> Reviewed-by: Simon Horman
> ---
> drivers/pwm/Kconfig| 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-rcar.c | 265
> +
> 3 files changed, 277 insertions(+)
> create mode 100644 drivers/pwm/pwm-rcar.c
Found a couple more things. No need to respin for any of these, I can
make the changes when applying, but I'd like confirmation on a couple
of things below.
[...]
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
[...]
> +static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int
> duty_ns,
> + int period_ns)
> +{
> + unsigned long long one_cycle, tmp; /* 0.01 nanoseconds */
I'm not quite sure why you need the extra multiplication and division by
100 here. Is this for extra accuracy?
> + unsigned long clk_rate = clk_get_rate(rp->clk);
> + u32 cyc, ph;
> +
> + one_cycle = (unsigned long long)NSEC_PER_SEC * 100ULL * (1 << div);
> + do_div(one_cycle, clk_rate);
> +
> + tmp = period_ns * 100ULL;
> + do_div(tmp, one_cycle);
> + cyc = (tmp << RCAR_PWMCNT_CYC0_SHIFT) & RCAR_PWMCNT_CYC0_MASK;
> +
> + tmp = duty_ns * 100ULL;
> + do_div(tmp, one_cycle);
> + ph = tmp & RCAR_PWMCNT_PH0_MASK;
> +
> + /* Avoid prohibited setting */
> + if (cyc != 0 && ph != 0) {
> + rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
> + return 0;
> + } else {
> + return -EINVAL;
> + }
I think the ordering here is unintuitive, better would be:
if (cyc == 0 || ph == 0)
return -EINVAL;
rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
return 0;
> +static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +int duty_ns, int period_ns)
> +{
> + struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> + int div = rcar_pwm_get_clock_division(rp, period_ns);
> + int ret;
> +
> + if (div < 0)
> + return div;
> +
> + /* Let the core driver set pwm->period if disabled and duty_ns == 0 */
> + if (!test_bit(PWMF_ENABLED, &pwm->flags) && !duty_ns)
> + return 0;
> +
> + rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
> + ret = rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
> + rcar_pwm_set_clock_control(rp, div);
> + rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
Just making sure: is it correct to execute the above two lines even if
ret < 0?
> +static int rcar_pwm_probe(struct platform_device *pdev)
> +{
> + struct rcar_pwm_chip *rcar_pwm;
> + struct resource *res;
> + int ret;
> +
> + rcar_pwm = devm_kzalloc(&pdev->dev, sizeof(*rcar_pwm), GFP_KERNEL);
> + if (rcar_pwm == NULL)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + rcar_pwm->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(rcar_pwm->base))
> + return PTR_ERR(rcar_pwm->base);
> +
> + rcar_pwm->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(rcar_pwm->clk)) {
> + dev_err(&pdev->dev, "cannot get clock\n");
> + return PTR_ERR(rcar_pwm->clk);
> + }
> +
> + platform_set_drvdata(pdev, rcar_pwm);
> +
> + rcar_pwm->chip.dev = &pdev->dev;
> + rcar_pwm->chip.ops = &rcar_pwm_ops;
> + rcar_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
This seems to be missing a:
rcar_pwm->chip.of_pwm_n_cells = 3;
?
Thierry
signature.asc
Description: PGP signature
Re: [PATCH 2/2] pwm: Add Broadcom BCM7038 PWM controller support
On Thu, Aug 06, 2015 at 05:55:58PM -0700, Florian Fainelli wrote:
> Add support for the BCM7038-style PWM controller found in all BCM7xxx STB
> SoCs.
> This controller has a hardcoded 2 channels per controller, and cascades a
> variable frequency generator on top of a fixed frequency generator which
> offers
> a range of a 148ns period all the way to ~622ms periods.
>
> Signed-off-by: Florian Fainelli
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-brcmstb.c | 323
> ++
> 3 files changed, 334 insertions(+)
> create mode 100644 drivers/pwm/pwm-brcmstb.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f40fd8d..28f95cca70ce 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -111,6 +111,16 @@ config PWM_CLPS711X
> To compile this driver as a module, choose M here: the module
> will be called pwm-clps711x.
>
> +config PWM_BRCMSTB
> + tristate "Broadcom STB PWM support"
> + depends on ARCH_BRCMSTB
> + help
> + Generic PWM framework driver for the Broadcom Set-top-Box
> + SoCs (BCM7xxx).
> +
> + To compile this driver as a module, choose M Here: the module
> + will be called pwm-brcmstb.c.
Perhaps call it pwm-brcm7xxx? stb sounds more like a use-case
description rather than a hardware model name.
> config PWM_EP93XX
> tristate "Cirrus Logic EP93xx PWM support"
> depends on ARCH_EP93XX
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index ec50eb5b5a8f..dc7b1b82d47e 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o
> obj-$(CONFIG_PWM_BCM_KONA) += pwm-bcm-kona.o
> obj-$(CONFIG_PWM_BCM2835)+= pwm-bcm2835.o
> obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o
> +obj-$(CONFIG_PWM_BRCMSTB)+= pwm-brcmstb.o
> obj-$(CONFIG_PWM_CLPS711X) += pwm-clps711x.o
> obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o
> obj-$(CONFIG_PWM_FSL_FTM)+= pwm-fsl-ftm.o
> diff --git a/drivers/pwm/pwm-brcmstb.c b/drivers/pwm/pwm-brcmstb.c
> new file mode 100644
> index ..0c5cf5cbcf74
> --- /dev/null
> +++ b/drivers/pwm/pwm-brcmstb.c
> @@ -0,0 +1,323 @@
> +/*
> + * Broadcom BCM7038 PWM driver
> + *
> + * Copyright (C) 2015 Broadcom Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
These should be alphabetically sorted.
> +
> +/* The block has a hardcoded number of 2 channels per controller */
> +#define NUM_PWM_CHAN 2
No need for the define here. You only use this value once, so having the
literal at the place where it's needed prevents people from having to
look the value up.
> +
> +/* This block is the "UPG" clock domain which is hardcoded a 27Mhz */
> +#define PWM_DEFAULT_FREQ 2700
Do you really need this? Why not simply make the clocks property
required and get rid of this fallback?
> +
> +#define PWM_CTRL 0x00
> +#define CTRL_START BIT(0)
> +#define CTRL_OEBBIT(1)
> +#define CTRL_FORCE_HIGH BIT(2)
> +#define CTRL_OPENDRAIN BIT(3)
> +#define CTRL_CHAN_OFFS 4
> +
> +#define PWM_CTRL20x04
> +#define CTRL2_OUT_SELECTBIT(0)
> +
> +#define PWM_CWORD_MSB0x08
> +#define PWM_CWORD_LSB0x0C
> +
> +#define PWM_CH_SIZE 0x8
> +
> +/* Number of bits for the CWORD value */
> +#define CWORD_BIT_SIZE 16
> +
> +/* Maximum control word value allowed when variable-frequency PWM is used as
> a
> + * clock for the constant-frequency PMW.
> + */
Proper block-comment style is:
/*
*
*
*/
> +#define CONST_VAR_F_MAX 32768
> +#define CONST_VAR_F_MIN 1
> +
> +#define PWM_ON 0x18
> +#define PWM_ON_MIN 1
> +#define PWM_PERIOD 0x1C
> +#define PWM_PERIOD_MIN 0
Have you considered parameterizing these for ease of use, like so:
#define PWM_ON(ch) (0x18 + ((ch) * PWM_CH_SIZE))
?
> +
> +#define PWM_ON_PERIOD_MAX0xff
> +
> +struct brcmstb_pwm_dev {
> + struct platform_device *pdev;
This seems to be unused.
> + void __iomem *base;
> + struct clk *clk;
> + unsign
Re: [PATCH 1/2] Documentation: dt: add Broadcom BCM7038 PWM controller binding
On Thu, Aug 06, 2015 at 05:55:57PM -0700, Florian Fainelli wrote: > Add a binding documentation for the Broadcom BCM7038 PWM controller found in > BCM7xxx chips. > > Signed-off-by: Florian Fainelli > --- > .../devicetree/bindings/pwm/brcm,bcm7038-pwm.txt | 22 > ++ > 1 file changed, 22 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/brcm,bcm7038-pwm.txt > > diff --git a/Documentation/devicetree/bindings/pwm/brcm,bcm7038-pwm.txt > b/Documentation/devicetree/bindings/pwm/brcm,bcm7038-pwm.txt > new file mode 100644 > index ..8b9bc43b561e > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/brcm,bcm7038-pwm.txt > @@ -0,0 +1,22 @@ > +Broadcom BCM7038 PWM controller (BCM7xxx Set Top Box PWM controller) > + > +Required properties: > + > +- compatible: must be "brcm,bcm7038-pwm" > +- reg: physical base address and length for this controller > +- #pwm-cells: should be 2. See pwm.txt in this directory for a description > + of the cells format. > + > +Optional properties: > + > +- clocks: a phandle to the reference clock for this block which is fed > through > + its internal variable clock frequency generator Why is this optional? I would assume that the hardware always needs some sort of reference clock to properly function. Thierry signature.asc Description: PGP signature
Re: [PATCH v5 2/2] pwm: Add support for R-Car PWM Timer
Sorry for taking an awful long time to get around to this. The driver
looks generally okay, but I have a few minor comments...
On Mon, Jun 15, 2015 at 06:08:44PM +0900, Yoshihiro Shimoda wrote:
> This patch adds support for R-Car SoCs PWM Timer.
This could be a little more verbose. You could say for example how many
channels the driver exposes, or mention typical use-cases (if any).
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
[...]
> +static int rcar_pwm_get_clock_division(struct rcar_pwm_chip *rp, int
> period_ns)
> +{
> + int div;
Can be unsigned int.
> + unsigned long clk_rate = clk_get_rate(rp->clk);
> + unsigned long long max; /* max cycle / nanoseconds */
> +
> + if (!clk_rate)
I prefer it when these are explicit: clk_rate == 0. This goes for
numerical comparisons. For booleans, or NULL pointer comparisons the
!expression is fine.
> +static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +int duty_ns, int period_ns)
> +{
> + struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> + int div = rcar_pwm_get_clock_division(rp, period_ns);
> + int ret;
> +
> + if (div < 0)
> + return div;
> +
> + /* Let the core driver set pwm->period if disabled and duty_ns == 0 */
> + if (!test_bit(PWMF_ENABLED, &pwm->flags) && !duty_ns)
> + return 0;
> +
> + rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
> + ret = rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
> + rcar_pwm_set_clock_control(rp, div);
> + rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
> +
> + return ret;
> +}
> +
> +static int rcar_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> + u32 pwmcnt;
> +
> + /* Don't enable the PWM device if CYC0 or PH0 is 0 */
> + pwmcnt = rcar_pwm_read(rp, RCAR_PWMCNT);
> + if (!(pwmcnt & RCAR_PWMCNT_CYC0_MASK) ||
> + !(pwmcnt & RCAR_PWMCNT_PH0_MASK))
> + return -EINVAL;
This looks wrong. Any errors in configuration should've been caught by
the ->config() implementation. Why can't you return -EINVAL on this
condition in ->config()? ->enable() failing should only be the case if
truly the PWM can't be enabled, not if it's badly configured.
> +static struct platform_driver rcar_pwm_driver = {
> + .probe = rcar_pwm_probe,
> + .remove = rcar_pwm_remove,
> + .driver = {
> + .name = "pwm-rcar",
> + .of_match_table = of_match_ptr(rcar_pwm_of_table),
> + }
> +};
This doesn't need the artificial padding. A single space around = is
enough.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v6 1/3] dt-bindings: pwm: add MediaTek display PWM bindings
On Mon, Jul 20, 2015 at 04:17:15PM +0800, YH Huang wrote: > Document the device-tree binding of MediatTek display PWM. I already mentioned this a while back: s/MediatTek/MediaTek/. Thierry signature.asc Description: PGP signature
Re: [PATCH v6 2/3] pwm: add MediaTek display PWM driver support
On Mon, Jul 20, 2015 at 04:17:16PM +0800, YH Huang wrote:
> Add display PWM driver support to modify backlight for MT8173 and MT6595.
> The PWM has one channel to control the brightness of the display.
> When the (high_width / period) is closer to 1, the screen is brighter;
> otherwise, it is darker.
>
> Signed-off-by: YH Huang
> ---
> drivers/pwm/Kconfig| 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-mtk-disp.c | 232
> +
> 3 files changed, 243 insertions(+)
> create mode 100644 drivers/pwm/pwm-mtk-disp.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f4..f5b03a4 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -211,6 +211,16 @@ config PWM_LPSS_PLATFORM
> To compile this driver as a module, choose M here: the module
> will be called pwm-lpss-platform.
>
> +config PWM_MTK_DISP
> + tristate "MediaTek display PWM driver"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
This is going to break randconfig builds. From a quick look your driver
will fail to build at least on architectures that don't set HAVE_IOMEM.
I know that's quite exotic, but I've had to deal with fallout from
things like this in the past. If you want || COMPILE_TEST you should at
least add a "depends on HAVE_IOMEM".
linux/clk.h seems to have stubs for all of the functions that you use,
so those shouldn't be a problem.
> + help
> + Generic PWM framework driver for MediaTek disp-pwm device.
> + The PWM is used to control the backlight brightness for display.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-mtk-disp.
> +
> config PWM_MXS
> tristate "Freescale MXS PWM support"
> depends on ARCH_MXS && OF
[...]
> diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> new file mode 100644
> index 000..69682a0
> --- /dev/null
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -0,0 +1,232 @@
> +/*
> + * MediaTek display pulse-width-modulation controller driver.
> + * Copyright (c) 2015 MediaTek Inc.
> + * Author: YH Huang
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
The above two aren't correctly sorted.
> +#include
> +
> +#define DISP_PWM_EN 0x00
> +#define PWM_ENABLE_MASK BIT(0)
> +
> +#define DISP_PWM_COMMIT 0x08
> +#define PWM_COMMIT_MASK BIT(0)
> +
> +#define DISP_PWM_CON_0 0x10
> +#define PWM_CLKDIV_SHIFT 16
> +#define PWM_CLKDIV_MAX 0x3ff
> +#define PWM_CLKDIV_MASK (PWM_CLKDIV_MAX << PWM_CLKDIV_SHIFT)
> +
> +#define DISP_PWM_CON_1 0x14
> +#define PWM_PERIOD_MASK 0xfff
> +/* Shift log2(PWM_PERIOD_MASK + 1) as divisor */
> +#define PWM_PERIOD_BIT_SHIFT 12
There are still two names for the same value here. I thought we had
agreed on this becoming something like the below?
#define PWM_PERIOD_BIT_SHIFT 12
#define PWM_PERIOD_MASK ((1 << PWM_PERIOD_BIT_SHIFT) - 1)
Although PWM_PERIOD_BIT_WIDTH would probably be a better name. Or
PWM_PERIOD_BITS or similar.
> +static int mtk_disp_pwm_remove(struct platform_device *pdev)
> +{
> + struct mtk_disp_pwm *mdp = platform_get_drvdata(pdev);
> + int ret = pwmchip_remove(&mdp->chip);
I'd prefer this to be separate lines:
int ret;
ret = pwmchip_remove(&mdp->chip);
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v2 0/3] Have Tegra's GPIO chip depend explicitly on the pinctrl device
On Tue, Jul 14, 2015 at 10:29:53AM +0200, Tomeu Vizoso wrote: > Hello, > > these three patches make sure that there's an explicit dependency from > the GPIO chip in Tegra SoCs to the corresponding pinctrl device, without > having duplicated gpio ranges. > > By having an explicit dependency, we can do things such as probing the > pinctrl device before the GPIO chip device to avoid deferred probes. > > Thanks, > > Tomeu > > Changes in v2: > - Don't defer probe if the pinctrl node is disabled > - Remove outdated comment from the commit changelog > > Tomeu Vizoso (3): > gpio: defer probe if pinctrl cannot be found > pinctrl: tegra: Only set the gpio range if needed > ARM: tegra: Add gpio-ranges property Patches 2 and 3 applied to the Tegra tree. Linus, I've applied the pinctrl patch to a separate branch, so we could use that to resolve conflicts, should there be any. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 2/2] drm/panel: Add display timing for Okaya RS800480T-7X0GP
On Wed, Jun 10, 2015 at 06:44:23PM +0200, Gary Bisson wrote: > Add support for the Okaya RS800480T-7X0GP to the DRM simple panel > driver. > > The RS800480T-7X0GP is a WVGA (800x480) panel with an 18-bit parallel > LCD interface. It supports pixel clocks in the range of 30-40 MHz. > > This panel details can be found at: > http://boundarydevices.com/product/7-800x480-display/ > > Signed-off-by: Gary Bisson > --- > .../bindings/panel/okaya,rs800480t_7x0gp.txt | 7 + > drivers/gpu/drm/panel/panel-simple.c | 33 > ++ > 2 files changed, 40 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/panel/okaya,rs800480t_7x0gp.txt > > diff --git > a/Documentation/devicetree/bindings/panel/okaya,rs800480t_7x0gp.txt > b/Documentation/devicetree/bindings/panel/okaya,rs800480t_7x0gp.txt > new file mode 100644 > index 000..f7c729d > --- /dev/null > +++ b/Documentation/devicetree/bindings/panel/okaya,rs800480t_7x0gp.txt > @@ -0,0 +1,7 @@ > +OKAYA Electric America, Inc. RS800480T-7X0GP 7" WVGA LCD panel > + > +Required properties: > +- compatible: should be "okaya,rs800480t_7x0gp" Underscores are not typically used in compatible strings, so I've changed this to "okaya,rs800480t-7x0gp". Thierry signature.asc Description: PGP signature
Re: [PATCH v3 1/2] of: add Okaya Electric America vendor prefix
On Wed, Jun 10, 2015 at 06:44:22PM +0200, Gary Bisson wrote: > This patch adds vendor prefix for Okaya Electronic America, a provider > of LCD modules and display technologies. > > Signed-off-by: Gary Bisson > --- > Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + > 1 file changed, 1 insertion(+) Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH v14 3/6] drm/panel: simple: Add support for NEC NL4827HC19-05B 480x272 panel
On Wed, Jul 29, 2015 at 04:30:02PM +0800, Jianwei Wang wrote: > This adds support for the NEC NL4827HC19-05B 480x272 panel to the DRM > simple panel driver. > > Signed-off-by: Alison Wang > Signed-off-by: Xiubo Li > Signed-off-by: Jianwei Wang > Acked-by: Daniel Vetter > --- > .../bindings/panel/nec,nl4827hc19_05b.txt | 7 ++ > drivers/gpu/drm/panel/panel-simple.c | 26 > ++ > 2 files changed, 33 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/panel/nec,nl4827hc19_05b.txt > > diff --git a/Documentation/devicetree/bindings/panel/nec,nl4827hc19_05b.txt > b/Documentation/devicetree/bindings/panel/nec,nl4827hc19_05b.txt > new file mode 100644 > index 000..20e9473 > --- /dev/null > +++ b/Documentation/devicetree/bindings/panel/nec,nl4827hc19_05b.txt > @@ -0,0 +1,7 @@ > +NEC LCD Technologies,Ltd. WQVGA TFT LCD panel > + > +Required properties: > +- compatible: should be "nec,nl4827hc19_05b" Underscores are deprecated in compatible strings, so I've applied this with "nec,nl4827hc19-05b". Thierry signature.asc Description: PGP signature
Re: [PATCH v14 3/6] drm/panel: simple: Add support for NEC NL4827HC19-05B 480x272 panel
On Wed, Jul 29, 2015 at 04:30:02PM +0800, Jianwei Wang wrote: > This adds support for the NEC NL4827HC19-05B 480x272 panel to the DRM > simple panel driver. > > Signed-off-by: Alison Wang > Signed-off-by: Xiubo Li > Signed-off-by: Jianwei Wang > Acked-by: Daniel Vetter > --- > .../bindings/panel/nec,nl4827hc19_05b.txt | 7 ++ > drivers/gpu/drm/panel/panel-simple.c | 26 > ++ > 2 files changed, 33 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/panel/nec,nl4827hc19_05b.txt Applied, thanks. Thierry signature.asc Description: PGP signature
Re: [PATCH RFC 1/5] drm/msm/hdmi: deprecate non standard gpio properties.
On Mon, Aug 10, 2015 at 12:59:22PM +0100, Srinivas Kandagatla wrote: > This patch updates the bindings to discourage the usage of non standard > gpio properites, this will help in projects focused on upstreaming. That last part is an odd comment to make in the commit message of a patch submitted upstream... > These deprecated properties are still supported but will be remove over > the time. You can't ever remove them because you can't ever be sure that people won't be using an old DTB. > > Signed-off-by: Srinivas Kandagatla > --- > Documentation/devicetree/bindings/drm/msm/hdmi.txt | 22 > +- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt > b/Documentation/devicetree/bindings/drm/msm/hdmi.txt > index c43aa53..acba581 100644 > --- a/Documentation/devicetree/bindings/drm/msm/hdmi.txt > +++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt > @@ -11,15 +11,27 @@ Required properties: > - interrupts: The interrupt signal from the hdmi block. > - clocks: device clocks >See ../clocks/clock-bindings.txt for details. > -- qcom,hdmi-tx-ddc-clk-gpio: ddc clk pin > -- qcom,hdmi-tx-ddc-data-gpio: ddc data pin > -- qcom,hdmi-tx-hpd-gpio: hpd pin > +- qcom,hdmi-tx-ddc-clk-gpios: ddc clk pin > +- qcom,hdmi-tx-ddc-data-gpios: ddc data pin > +- qcom,hdmi-tx-hpd-gpios: hpd pin > - core-vdda-supply: phandle to supply regulator > - hdmi-mux-supply: phandle to mux regulator > > +- qcom,hdmi-tx-ddc-clk-gpio: (deprecated) use > + "qcom,hdmi-tx-ddc-clk-gpios" instead > +- qcom,hdmi-tx-ddc-data-gpio: (deprecated) use > + "qcom,hdmi-tx-ddc-data-gpios" instead > +- qcom,hdmi-tx-hpd-gpio: (deprecated) use > + "qcom,hdmi-tx-hpd-gpios" instead > + > Optional properties: > -- qcom,hdmi-tx-mux-en-gpio: hdmi mux enable pin > -- qcom,hdmi-tx-mux-sel-gpio: hdmi mux select pin > +- qcom,hdmi-tx-mux-en-gpios: hdmi mux enable pin > +- qcom,hdmi-tx-mux-sel-gpios: hdmi mux select pin > + > +- qcom,hdmi-tx-mux-en-gpio: (deprecated) use "qcom,hdmi-tx-mux-en-gpios" > + instead > +- qcom,hdmi-tx-mux-sel-gpio: (deprecated) use "qcom,hdmi-tx-mux-sel-gpio" > + instead > - pinctrl-names: the pin control state names; should contain "default" > - pinctrl-0: the default pinctrl state (active) > - pinctrl-1: the "sleep" pinctrl state I don't see much use in listing that these properties are deprecated. We already have code to catch the deprecated names, so having them in the binding will at best be distracting. Anyway, I don't know if there's been any advice on this from the device tree bindings maintainers, so adding devicetree@vger.kernel.org for visibility. Thierry signature.asc Description: PGP signature
Re: [PATCH V3 10/19] drm/tegra: dc: Prepare for generic PM domains
On Tue, Jul 28, 2015 at 09:30:04AM +0100, Jon Hunter wrote: > > On 17/07/15 11:41, Thierry Reding wrote: > > * PGP Signed by an unknown key > > > > On Mon, Jul 13, 2015 at 01:39:48PM +0100, Jon Hunter wrote: > >> Add support to the tegra dc driver for generic PM domains. However, > >> to ensure backward compatibility with older device tree blobs ensure > >> that the driver can work with or without generic PM domains. In order > >> to migrate to generic PM domain infrastructure the necessary changes > >> are: > >> > >> 1. If the "power-domains" property is present in the DT device node then > >>generic PM domains is supported and pm_runtime_enable() should be > >>called for the device. Furthermore, the enabling and disabling of the > >>power-domain is handled via calling pm_runtime_get/put, respectively. > > > > The device could be PM runtime enabled even if no power-domains property > > is set. Couldn't we check something more direct, like for example if the > > dev->pm_domain is non-NULL? > > Yes could do that instead. > > > Perhaps it'd be worth converting the driver to use runtime PM first, and > > move all the powergate and clock handling into suspend/resume functions, > > and then we can conditionalize whether or not we call the legacy API iff > > dev->pm_domain == NULL? > > May be that would be a cleaner transition than trying to do it all in > one go. I have a couple of patches in my tree to do this for DRM as part of an effort to restore DPMS. It's fairly tricky to get right in DRM and requires all sorts of changes to the driver. Thierry signature.asc Description: PGP signature
Re: [PATCH V3 02/19] memory: tegra: Add MC flush support
On Mon, Jul 20, 2015 at 12:59:41PM +0300, Peter De Schrijver wrote: > On Fri, Jul 17, 2015 at 01:31:24PM +0200, Thierry Reding wrote: > > * PGP Signed by an unknown key > > > > On Fri, Jul 17, 2015 at 01:20:49PM +0300, Peter De Schrijver wrote: > > > On Fri, Jul 17, 2015 at 11:57:55AM +0200, Thierry Reding wrote: > > > > > Old Signed by an unknown key > > > > > > > > On Mon, Jul 13, 2015 at 01:39:40PM +0100, Jon Hunter wrote: > > > > > The Tegra memory controller implements a flush feature to flush > > > > > pending > > > > > accesses and prevent further accesses from occurring. This feature is > > > > > used when powering down IP blocks to ensure the IP block is in a good > > > > > state. The flushes are organised by software groups and IP blocks are > > > > > assigned in hardware to the different software groups. Add helper > > > > > functions for requesting a handle to an MC flush for a given > > > > > software group and enabling/disabling the MC flush itself. > > > > > > > > > > This is based upon a change by Vince Hsu . > > > > > > > > > > Signed-off-by: Jon Hunter > > > > > --- > > > > > drivers/memory/tegra/mc.c | 110 > > > > > ++ > > > > > drivers/memory/tegra/mc.h | 2 + > > > > > include/soc/tegra/mc.h| 34 ++ > > > > > 3 files changed, 146 insertions(+) > > > > > > > > Do we know if this is actually necessary? I remember having a discussion > > > > with Arnd Bergmann a while ago, and the Linux driver model kind of > > > > assumes that by the time a device is disabled all outstanding accesses > > > > will have stopped. > > > > > > > > Do we have a way to determine that this even makes a difference? Can we > > > > trigger a case where not doing this would cause breakage and see that > > > > adding this fixes that particular issue? > > > > > > > > > > Most likely it is. The memory controller can still be processing requests > > > when the peripheral domain is powergated. This would mean the response > > > cannot > > > be delivered in that case. So we need to be sure there are no outstanding > > > requests before shutting down the domain. > > > > My point is that that's the driver's responsibility anyway, hence making > > the explicit flush unnecessary. > > > > The peripheral driver doesn't know how long a request is queued in the memory > controller. So it can't be responsible for ensuring this. Surely whenever the peripheral reports that the operation is done (be that via some DMA controller interrupt or syncpoint increment) the operation would have flushed from the memory controller. Drivers are already supposed to make sure this is the case when they are removed or suspended, so this would mean that we'd be adding all this code for no real gain. It would also explain why we're currently not seeing any such problems. Also note that I'm not saying no to this, I merely want to make sure that it's necessary, and in order to prove so we need tests to produce enough traffic for this to be exposed, and then we can also verify that the patches actually do what they're supposed to. Thierry signature.asc Description: PGP signature
Re: [PATCH V3 02/19] memory: tegra: Add MC flush support
On Mon, Jul 20, 2015 at 09:46:02AM +0100, Jon Hunter wrote: > > On 17/07/15 12:31, Thierry Reding wrote: > > * PGP Signed by an unknown key > > > > On Fri, Jul 17, 2015 at 01:20:49PM +0300, Peter De Schrijver wrote: > >> On Fri, Jul 17, 2015 at 11:57:55AM +0200, Thierry Reding wrote: > >>>> Old Signed by an unknown key > >>> > >>> On Mon, Jul 13, 2015 at 01:39:40PM +0100, Jon Hunter wrote: > >>>> The Tegra memory controller implements a flush feature to flush pending > >>>> accesses and prevent further accesses from occurring. This feature is > >>>> used when powering down IP blocks to ensure the IP block is in a good > >>>> state. The flushes are organised by software groups and IP blocks are > >>>> assigned in hardware to the different software groups. Add helper > >>>> functions for requesting a handle to an MC flush for a given > >>>> software group and enabling/disabling the MC flush itself. > >>>> > >>>> This is based upon a change by Vince Hsu . > >>>> > >>>> Signed-off-by: Jon Hunter > >>>> --- > >>>> drivers/memory/tegra/mc.c | 110 > >>>> ++ > >>>> drivers/memory/tegra/mc.h | 2 + > >>>> include/soc/tegra/mc.h| 34 ++ > >>>> 3 files changed, 146 insertions(+) > >>> > >>> Do we know if this is actually necessary? I remember having a discussion > >>> with Arnd Bergmann a while ago, and the Linux driver model kind of > >>> assumes that by the time a device is disabled all outstanding accesses > >>> will have stopped. > >>> > >>> Do we have a way to determine that this even makes a difference? Can we > >>> trigger a case where not doing this would cause breakage and see that > >>> adding this fixes that particular issue? > >>> > >> > >> Most likely it is. The memory controller can still be processing requests > >> when the peripheral domain is powergated. This would mean the response > >> cannot > >> be delivered in that case. So we need to be sure there are no outstanding > >> requests before shutting down the domain. > > > > My point is that that's the driver's responsibility anyway, hence making > > the explicit flush unnecessary. > > I see your point and it is interesting. The trouble is that we would > need to test every memory client in every power domain to prove this. So > I don't think that is a trivial thing to do. Furthermore, looking at > what we have done in kernel used for android products (which probably > stress PM the most) this is done and so I don't know of any shipping > product that stresses PM that does not do this. May be someone else > might. I personally would not be comfortable removing this without > testing, but as I mentioned it is not a trivial thing to test correctly. > However, I will let you and the other maintainers decide what's best here. Quite frankly, I'm fairly sure most of this is untested anyway. There isn't a good way to test gr3d for example. Generally I lean towards not merging any code that we don't know is needed. Merely because Android kernels do something doesn't mean it is necessary to do it (or even sane in some cases). Now I understand that it might be difficult to test this, but that's all the more reason not to include the code. If we find that we have bugs that this can fix, then we can come up with tests to trigger it and validate that the fix actually fixes something. In other words, I don't think it makes sense to implement mechanisms to recover from situations that we have no way of triggering in the first place. Thierry signature.asc Description: PGP signature
Re: [PATCH V3 02/19] memory: tegra: Add MC flush support
On Fri, Jul 17, 2015 at 01:20:49PM +0300, Peter De Schrijver wrote: > On Fri, Jul 17, 2015 at 11:57:55AM +0200, Thierry Reding wrote: > > * PGP Signed by an unknown key > > > > On Mon, Jul 13, 2015 at 01:39:40PM +0100, Jon Hunter wrote: > > > The Tegra memory controller implements a flush feature to flush pending > > > accesses and prevent further accesses from occurring. This feature is > > > used when powering down IP blocks to ensure the IP block is in a good > > > state. The flushes are organised by software groups and IP blocks are > > > assigned in hardware to the different software groups. Add helper > > > functions for requesting a handle to an MC flush for a given > > > software group and enabling/disabling the MC flush itself. > > > > > > This is based upon a change by Vince Hsu . > > > > > > Signed-off-by: Jon Hunter > > > --- > > > drivers/memory/tegra/mc.c | 110 > > > ++ > > > drivers/memory/tegra/mc.h | 2 + > > > include/soc/tegra/mc.h| 34 ++ > > > 3 files changed, 146 insertions(+) > > > > Do we know if this is actually necessary? I remember having a discussion > > with Arnd Bergmann a while ago, and the Linux driver model kind of > > assumes that by the time a device is disabled all outstanding accesses > > will have stopped. > > > > Do we have a way to determine that this even makes a difference? Can we > > trigger a case where not doing this would cause breakage and see that > > adding this fixes that particular issue? > > > > Most likely it is. The memory controller can still be processing requests > when the peripheral domain is powergated. This would mean the response cannot > be delivered in that case. So we need to be sure there are no outstanding > requests before shutting down the domain. My point is that that's the driver's responsibility anyway, hence making the explicit flush unnecessary. Thierry signature.asc Description: PGP signature
Re: [PATCH V3 15/19] soc: tegra: pmc: Add generic PM domain support
On Mon, Jul 13, 2015 at 01:39:53PM +0100, Jon Hunter wrote:
> Adds generic PM support to the PMC driver where the PM domains are
> populated from device-tree and the PM domain consumer devices are
> bound to their relevant PM domains via device-tree as well.
>
> Update the tegra_powergate_power_on_legacy/off_legacy() APIs so that
> internally they call the same tegra_powergate_xxx functions that are
> used by the tegra generic power domain code for consistency.
>
> This is based upon work by Thierry Reding
> and Vince Hsu .
>
> Signed-off-by: Jon Hunter
> ---
> drivers/soc/tegra/pmc.c | 545
> +++-
> include/dt-bindings/power/tegra-powergate.h | 42 +++
> include/soc/tegra/pmc.h | 52 +--
> 3 files changed, 580 insertions(+), 59 deletions(-)
> create mode 100644 include/dt-bindings/power/tegra-powergate.h
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 934653785bb7..4de92a9dae65 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -19,8 +19,11 @@
>
> #define pr_fmt(fmt) "tegra-pmc: " fmt
>
> +#include
> +
> #include
> #include
> +#include
Why are we now providing clocks?
> #include
> #include
> #include
> @@ -30,17 +33,24 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> +#include
> #include
> +#include
> #include
> +#include
What do we need this for?
> #include
> #include
>
> #include
> #include
> +#include
> #include
>
> +#define PMC_POWERGATE_ARRAY_MAX 10
There should be no need for this.
> +
> #define PMC_CNTRL0x0
> #define PMC_CNTRL_SYSCLK_POLARITY (1 << 10) /* sys clk polarity */
> #define PMC_CNTRL_SYSCLK_OE (1 << 11) /* system clock enable */
> @@ -106,6 +116,22 @@
>
> #define PMC_PWRGATE_STATE(val, id) (!!(val & BIT(id)))
>
> +struct tegra_powergate {
> + struct generic_pm_domain genpd;
> + struct tegra_pmc *pmc;
> + struct tegra_mc *mc;
> + unsigned int id;
> + struct list_head node;
> + struct device_node *of_node;
> + struct regulator *vdd;
> + struct clk **clks;
> + struct reset_control **resets;
> + const struct tegra_mc_flush **flushes;
> + u32 num_clks;
> + u32 num_resets;
> + u32 num_flushes;
These should be unsigned int, and please group them with the arrays that
they define the sizes of.
> +};
> +
> struct tegra_pmc_soc {
> unsigned int num_powergates;
> const char *const *powergates;
> @@ -118,8 +144,10 @@ struct tegra_pmc_soc {
>
> /**
> * struct tegra_pmc - NVIDIA Tegra PMC
> + * @dev: pointer to parent device
> * @base: pointer to I/O remapped register region
> * @clk: pointer to pclk clock
> + * @soc: SoC-specific data
This could be a separate patch because it isn't related to the power
domain work.
> * @rate: currently configured rate of pclk
> * @suspend_mode: lowest suspend mode available
> * @cpu_good_time: CPU power good time (in microseconds)
> @@ -133,6 +161,7 @@ struct tegra_pmc_soc {
> * @cpu_pwr_good_en: CPU power good signal is enabled
> * @lp0_vec_phys: physical base address of the LP0 warm boot code
> * @lp0_vec_size: size of the LP0 warm boot code
> + * @powergates_list: list of power gates
> * @powergates_lock: mutex for power gate register access
> */
> struct tegra_pmc {
> @@ -157,6 +186,7 @@ struct tegra_pmc {
> u32 lp0_vec_phys;
> u32 lp0_vec_size;
>
> + struct list_head powergates_list;
> struct mutex powergates_lock;
> };
>
> @@ -165,6 +195,12 @@ static struct tegra_pmc *pmc = &(struct tegra_pmc) {
> .suspend_mode = TEGRA_SUSPEND_NONE,
> };
>
> +static inline struct tegra_powergate *
> +to_powergate(struct generic_pm_domain *domain)
> +{
> + return container_of(domain, struct tegra_powergate, genpd);
> +}
> +
> static u32 tegra_pmc_readl(unsigned long offset)
> {
> return readl(pmc->base + offset);
> @@ -175,6 +211,37 @@ static void tegra_pmc_writel(u32 value, unsigned long
> offset)
> writel(value, pmc->base + offset);
> }
>
> +static int tegra_powergate_get_regulator(struct tegra_powergate *powergate)
> +{
> + struct platform_device *pdev;
> +
> + if (powergate->id != TEGRA_POWERGATE_EXT)
> + return -EINVAL;
> +
> + pdev = of_find_device_by_node(powergate->of_node);
> + if (!pdev)
> + return -EINVAL;
> +
> + powergate->vdd = devm_regulator_get_op
Re: [PATCH V3 11/19] PCI: tegra: Add support for generic PM domains
On Mon, Jul 13, 2015 at 01:39:49PM +0100, Jon Hunter wrote:
> Add support to the tegra PCI driver for generic PM domains. However,
> to ensure backward compatibility with older device tree blobs ensure
> that the driver can work with or without generic PM domains. In order
> to migrate to generic PM domain infrastructure the necessary changes
> are:
>
> 1. If the "power-domains" property is present in the DT device node then
>generic PM domains is supported and pm_runtime_enable() should be
>called for the device. Furthermore, the enabling and disabling of the
>power-domain is handled via calling pm_runtime_get/put, respectively.
>
> 2. To ensure that clocks are managed consistently when generic PM domains
>are used and are not used, drivers should be migrated to use the
>tegra_powergate_power_on_legacy() and tegra_powergate_power_off_legacy()
>functions instead of the current tegra_powergate_sequence_power_up()
>and tegra_powergate_power_off(). The purpose of the
>tegra_powergate_power_on_legacy() and tegra_powergate_power_off_legacy()
>APIs is to mimick the behaviour of the tegra generic power-domain code,
>such that if generic power domains are not supported the functionality
>is the same.
>
> 3. The main difference between the tegra_powergate_sequence_power_up() API
>and the tegra_powergate_power_on_legacy() is that the clock used to
>enable the powergate is not kept enabled when using the
>tegra_powergate_power_on_legacy() API. Therefore, drivers must enable
>the clocks they need after calling tegra_powergate_power_on_legacy()
>and disable these clocks before calling
>tegra_powergate_power_off_legacy().
This is the same comment as in the DRM patch, and it applies to all
devices that use powergating, so it should move into the preparatory
patch rather than be repeated in all patches.
>
> The helper functions for handling the powering on and off of the PCI
> controller have been updated to support generic PM domains. The
> tegra_pcie_power_off() was missing code to disable the clocks enabled in
> the tegra_pcie_power_on() function and so this has been added.
>
> Signed-off-by: Jon Hunter
> ---
> drivers/pci/host/pci-tegra.c | 49
> +---
> 1 file changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 10c05718dbfd..acd1f311eee5 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -40,6 +40,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -908,19 +909,32 @@ static int tegra_pcie_enable_controller(struct
> tegra_pcie *pcie)
>
> static void tegra_pcie_power_off(struct tegra_pcie *pcie)
> {
> + const struct tegra_pcie_soc_data *soc = pcie->soc_data;
> int err;
>
> - /* TODO: disable and unprepare clocks? */
> -
> err = phy_power_off(pcie->phy);
> if (err < 0)
> dev_warn(pcie->dev, "failed to power off PHY: %d\n", err);
>
> reset_control_assert(pcie->pcie_xrst);
> reset_control_assert(pcie->afi_rst);
> - reset_control_assert(pcie->pex_rst);
>
> - tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
> + clk_disable_unprepare(pcie->pll_e);
> + if (soc->has_cml_clk)
> + clk_disable_unprepare(pcie->cml_clk);
> + clk_disable_unprepare(pcie->afi_clk);
> + clk_disable_unprepare(pcie->pex_clk);
> +
> + if (pm_runtime_enabled(pcie->dev)) {
> + err = pm_runtime_put_sync(pcie->dev);
> + } else {
> + err = tegra_powergate_power_off_legacy(TEGRA_POWERGATE_PCIE,
> +pcie->pex_clk,
> +pcie->pex_rst);
> + }
> +
> + if (err < 0)
> + dev_warn(pcie->dev, "powergate power-down failed: %d\n", err);
>
> err = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
> if (err < 0)
> @@ -934,20 +948,28 @@ static int tegra_pcie_power_on(struct tegra_pcie *pcie)
>
> reset_control_assert(pcie->pcie_xrst);
> reset_control_assert(pcie->afi_rst);
> - reset_control_assert(pcie->pex_rst);
> -
> - tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
>
> /* enable regulators */
> err = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
> if (err < 0)
> dev_err(pcie->dev, "failed to enable regulators: %d\n", err);
>
> - err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_PCIE,
> - pcie->pex_clk,
> - pcie->pex_rst);
> - if (err) {
> - dev_err(pcie->dev, "powerup sequence failed: %d\n", err);
> + if (pm_runtime_enabled(pcie->dev)) {
> + err = pm_runtime_get_sync(pcie->dev);
> + } else {
> + err = tegra_powergat
Re: [PATCH V3 10/19] drm/tegra: dc: Prepare for generic PM domains
On Mon, Jul 13, 2015 at 01:39:48PM +0100, Jon Hunter wrote: > Add support to the tegra dc driver for generic PM domains. However, > to ensure backward compatibility with older device tree blobs ensure > that the driver can work with or without generic PM domains. In order > to migrate to generic PM domain infrastructure the necessary changes > are: > > 1. If the "power-domains" property is present in the DT device node then >generic PM domains is supported and pm_runtime_enable() should be >called for the device. Furthermore, the enabling and disabling of the >power-domain is handled via calling pm_runtime_get/put, respectively. The device could be PM runtime enabled even if no power-domains property is set. Couldn't we check something more direct, like for example if the dev->pm_domain is non-NULL? Perhaps it'd be worth converting the driver to use runtime PM first, and move all the powergate and clock handling into suspend/resume functions, and then we can conditionalize whether or not we call the legacy API iff dev->pm_domain == NULL? > 2. To ensure that clocks are managed consistently when generic PM domains >are used and are not used, drivers should be migrated to use the >tegra_powergate_power_on_legacy() and tegra_powergate_power_off_legacy() >functions instead of the current tegra_powergate_sequence_power_up() >and tegra_powergate_power_off(). The purpose of the >tegra_powergate_power_on_legacy() and tegra_powergate_power_off_legacy() >APIs is to mimick the behaviour of the tegra generic power-domain code, >such that if generic power domains are not supported the functionality >is the same. > > 3. The main difference between the tegra_powergate_sequence_power_up() API >and the tegra_powergate_power_on_legacy() is that the clock used to >enable the powergate is not kept enabled when using the >tegra_powergate_power_on_legacy() API. Therefore, drivers must enable >the clocks they need after calling tegra_powergate_power_on_legacy() >and disable these clocks before calling >tegra_powergate_power_off_legacy(). This seems like it should go into the previous patch. I'm not sure I understand why this is necessary. Wouldn't it be easier to update the drivers to properly cope with this? We'll need to move them to runtime PM at some point anyway, so if we do that first, we should be able to hide all these details within the driver's suspend/resume functions but without the need for the API churn here. Thierry signature.asc Description: PGP signature
Re: [PATCH V3 08/19] soc: tegra: pmc: Clean-up PMC helper functions
On Mon, Jul 13, 2015 at 01:39:46PM +0100, Jon Hunter wrote:
> The following clean-ups have been made to the PMC code:
>
> 1. Add a macro for determining the state of a PMC powergate
> 2. Use the readx_poll_timeout() function instead of implementing a local
>polling loop with a timeout.
> 3. Use a case-statement in the function that removes the powergate clamp
>instead of multiple if-statements to improve readability.
>
> Signed-off-by: Jon Hunter
> ---
> drivers/soc/tegra/pmc.c | 72
> -
> 1 file changed, 35 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index c0635bdd4ee3..180d434deec5 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -28,6 +28,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -56,6 +57,8 @@
> #define PWRGATE_TOGGLE_START(1 << 8)
>
> #define REMOVE_CLAMPING 0x34
> +#define REMOVE_CLAMPING_VDEC(1 << 3)
> +#define REMOVE_CLAMPING_PCIE(1 << 4)
>
> #define PWRGATE_STATUS 0x38
>
> @@ -101,6 +104,8 @@
>
> #define GPU_RG_CNTRL 0x2d4
>
> +#define PMC_PWRGATE_STATE(val, id) (!!(val & BIT(id)))
I find double-negations very difficult to read. ((value & BIT(id)) != 0)
is clearer in my opinion. Also I'd suggest status or value as the first
parameter name, for consistency with other parts of the driver.
> +
> struct tegra_pmc_soc {
> unsigned int num_powergates;
> const char *const *powergates;
> @@ -177,15 +182,14 @@ static void tegra_pmc_writel(u32 value, unsigned long
> offset)
> */
> static int tegra_powergate_set(int id, bool new_state, bool wait)
> {
> - unsigned long timeout;
> - bool status;
> int ret = 0;
> + u32 val;
>
> mutex_lock(&pmc->powergates_lock);
>
> - status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
> + val = tegra_pmc_readl(PWRGATE_STATUS);
>
> - if (status == new_state) {
> + if (PMC_PWRGATE_STATE(val, id) == new_state) {
> mutex_unlock(&pmc->powergates_lock);
> return 0;
> }
> @@ -193,17 +197,9 @@ static int tegra_powergate_set(int id, bool new_state,
> bool wait)
> tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
>
> if (wait) {
> - timeout = jiffies + msecs_to_jiffies(100);
> - ret = -ETIMEDOUT;
> -
> - while (time_before(jiffies, timeout)) {
> - status = !!(tegra_pmc_readl(PWRGATE_STATUS) & BIT(id));
> - if (status == new_state) {
> - ret = 0;
> - break;
> - }
> - udelay(10);
> - }
> + ret = readx_poll_timeout(tegra_pmc_readl, PWRGATE_STATUS,
> + val, PMC_PWRGATE_STATE(val, id) == new_state,
> + 10, 10);
> }
>
> mutex_unlock(&pmc->powergates_lock);
> @@ -242,13 +238,10 @@ EXPORT_SYMBOL(tegra_powergate_power_off);
> */
> int tegra_powergate_is_powered(int id)
> {
> - u32 status;
> -
> if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
> return -EINVAL;
>
> - status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
> - return !!status;
> + return PMC_PWRGATE_STATE(tegra_pmc_readl(PWRGATE_STATUS), id);
I'd prefer to keep the two steps here. As a general rule I try never to
mix reading or writing a register with other code on the same line.
> }
>
> /**
> @@ -257,34 +250,39 @@ int tegra_powergate_is_powered(int id)
> */
> int tegra_powergate_remove_clamping(int id)
> {
> - u32 mask;
> + u32 val, reg;
Side note: Please use value instead of val since that's the form used
elsewhere in the driver. Also reg would be more appropriately called
offset or similar because that's what it really is. It would also be
more naturally an unsized type, such as unsigned int. But...
>
> if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
> return -EINVAL;
>
> /*
> - * On Tegra124 and later, the clamps for the GPU are controlled by a
> - * separate register (with different semantics).
> + * In most cases the bit for removing the IO clamping is the same as
> + * the powergate partition ID, however, this is not always the case.
> + * Furthermore, on Tegra124 and later, the clamps for the GPU are
> + * controlled by a separate register (with different semantics). The
> + * following case-statement handles these exceptions.
>*/
> - if (id == TEGRA_POWERGATE_3D) {
> + val = 1 << id;
> + reg = REMOVE_CLAMPING;
> +
> + switch (id) {
> + case TEGRA_POWERGATE_3D:
> if (pmc->soc->has_gpu_clamps) {
> - tegra_
Re: [PATCH V3 07/19] soc: tegra: pmc: Wait for powergate state to change
On Mon, Jul 13, 2015 at 01:39:45PM +0100, Jon Hunter wrote:
> Currently, the function tegra_powergate_set() simply sets the desired
> powergate state but does not wait for the state to change. In some
> circumstances this can be desirable. However, in most cases we should
> wait for the state to change before proceeding. Therefore, add a
> parameter to tegra_powergate_set() to indicate whether we should wait
> for the state to change.
>
> By adding this feature, we can also eliminate the polling loop from
> tegra30_boot_secondary().
>
> Signed-off-by: Jon Hunter
> ---
> arch/arm/mach-tegra/platsmp.c | 18 --
> drivers/soc/tegra/pmc.c | 29 +++--
> include/soc/tegra/pmc.h | 2 +-
> 3 files changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
> index b4508648..13982b5936c0 100644
> --- a/arch/arm/mach-tegra/platsmp.c
> +++ b/arch/arm/mach-tegra/platsmp.c
> @@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu,
> struct task_struct *idle)
>* be un-gated by un-toggling the power gate register
>* manually.
>*/
> - if (!tegra_pmc_cpu_is_powered(cpu)) {
> - ret = tegra_pmc_cpu_power_on(cpu);
> - if (ret)
> - return ret;
> -
> - /* Wait for the power to come up. */
> - timeout = jiffies + msecs_to_jiffies(100);
> - while (!tegra_pmc_cpu_is_powered(cpu)) {
> - if (time_after(jiffies, timeout))
> - return -ETIMEDOUT;
> - udelay(10);
> - }
> - }
> + ret = tegra_pmc_cpu_power_on(cpu, true);
> + if (ret)
> + return ret;
>
> remove_clamps:
> /* CPU partition is powered. Enable the CPU clock. */
> @@ -162,7 +152,7 @@ static int tegra114_boot_secondary(unsigned int cpu,
> struct task_struct *idle)
>* also initial power state in flow controller. After that,
>* the CPU's power state is maintained by flow controller.
>*/
> - ret = tegra_pmc_cpu_power_on(cpu);
> + ret = tegra_pmc_cpu_power_on(cpu, false);
> }
>
> return ret;
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 300f11e0c3bb..c0635bdd4ee3 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -175,9 +175,11 @@ static void tegra_pmc_writel(u32 value, unsigned long
> offset)
> * @id: partition ID
> * @new_state: new state of the partition
The comment here isn't updated.
> */
> -static int tegra_powergate_set(int id, bool new_state)
> +static int tegra_powergate_set(int id, bool new_state, bool wait)
Can we please not chain boolean parameters, it makes the call sites
impossible to parse for humans. Instead, can we simply leave
tegra_powergate_set() as it is and add another function, such as
tegra_powergate_set_sync() or tegra_powergate_set_and_wait(), to achieve
the same? You may have to split out a tegra_powergate_set_unlocked() or
similar to make sure you get to keep the lock across both operations:
static int tegra_powergate_set_unlocked(int id, bool new_state)
{
...
}
static int tegra_powergate_set(int id, bool new_state)
{
int err;
mutex_lock(&pmc->powergates_lock);
err = tegra_powergate_set_unlocked(id, new_state);
mutex_unlock(&pmc->powergates_lock);
return err;
}
/*
* Must be called with pmc->powergates_lock mutex held.
*/
static int tegra_powergate_wait(int id, bool new_state)
{
...
}
static int tegra_powergate_set_and_wait(int id, bool new_state)
{
int err;
mutex_lock(&pmc->powergates_lock);
err = tegra_powergate_set_unlocked(id, new_state);
if (err < 0)
goto unlock;
err = tegra_powergate_wait(id, new_state);
if (err < 0)
goto unlock;
unlock:
mutex_unlock(&pmc->powergates_lock);
return err;
}
> {
> + unsigned long timeout;
> bool status;
> + int ret = 0;
>
> mutex_lock(&pmc->powergates_lock);
>
> @@ -190,9 +192,23 @@ static int tegra_powergate_set(int id, bool new_state)
>
> tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
>
> + if (wait) {
> + timeout = jiffies + msecs_to_jiffies(100);
> + ret = -ETIMEDOUT;
> +
> + while (time_before(jiffies, timeout)) {
> + status = !!(tegra_pmc_readl(PWRGATE_STATUS) & BIT(id));
> + if (status == new_state) {
> + ret = 0;
> +
Re: [PATCH V3 04/19] memory: tegra: add flush operation for Tegra114 memory clients
On Mon, Jul 13, 2015 at 01:39:42PM +0100, Jon Hunter wrote:
> From: Vince Hsu
>
> This patch adds the hot reset register table and flush related callback
> functions for Tegra114.
>
> Signed-off-by: Vince Hsu
> [jonath...@nvidia.com: Removed tegra_mc_ops and added
> metastable_flush_reads.]
> Signed-off-by: Jon Hunter
> ---
> drivers/memory/tegra/tegra114.c | 21 +
> 1 file changed, 21 insertions(+)
>
> diff --git a/drivers/memory/tegra/tegra114.c b/drivers/memory/tegra/tegra114.c
> index 9f579589e800..ba33c402ed68 100644
> --- a/drivers/memory/tegra/tegra114.c
> +++ b/drivers/memory/tegra/tegra114.c
> @@ -914,6 +914,24 @@ static const struct tegra_smmu_swgroup
> tegra114_swgroups[] = {
> { .name = "tsec", .swgroup = TEGRA_SWGROUP_TSEC, .reg = 0x294
> },
> };
>
> +static struct tegra_mc_flush tegra114_mc_flush[] = {
const
> + {TEGRA_SWGROUP_AVPC, 0x200, 0x204, 1},
> + {TEGRA_SWGROUP_DC, 0x200, 0x204, 2},
> + {TEGRA_SWGROUP_DCB,0x200, 0x204, 3},
> + {TEGRA_SWGROUP_EPP,0x200, 0x204, 4},
> + {TEGRA_SWGROUP_G2, 0x200, 0x204, 5},
> + {TEGRA_SWGROUP_HC, 0x200, 0x204, 6},
> + {TEGRA_SWGROUP_HDA,0x200, 0x204, 7},
> + {TEGRA_SWGROUP_ISP,0x200, 0x204, 8},
> + {TEGRA_SWGROUP_MPCORE, 0x200, 0x204, 9},
> + {TEGRA_SWGROUP_MPCORELP, 0x200, 0x204, 10},
> + {TEGRA_SWGROUP_MSENC, 0x200, 0x204, 11},
> + {TEGRA_SWGROUP_NV, 0x200, 0x204, 12},
> + {TEGRA_SWGROUP_PPCS, 0x200, 0x204, 14},
> + {TEGRA_SWGROUP_VDE,0x200, 0x204, 16},
> + {TEGRA_SWGROUP_VI, 0x200, 0x204, 17},
> +};
And spaces after { and before }.
> +
> static void tegra114_flush_dcache(struct page *page, unsigned long offset,
> size_t size)
> {
> @@ -945,4 +963,7 @@ const struct tegra_mc_soc tegra114_mc_soc = {
> .num_address_bits = 32,
> .atom_size = 32,
> .smmu = &tegra114_smmu_soc,
> + .flushes = tegra114_mc_flush,
> + .num_flushes = ARRAY_SIZE(tegra114_mc_flush),
> + .metastable_flush_reads = MC_FLUSH_METASTABLE_READS,
I don't think it's useful to have this define. Just use the literal
value here. Much of the purpose of having this per-SoC parameter is to
provide the context that otherwise the macro symbol would provide.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH V3 05/19] memory: tegra: add flush operation for Tegra124 memory clients
On Mon, Jul 13, 2015 at 01:39:43PM +0100, Jon Hunter wrote: > From: Vince Hsu > > This patch adds the hot reset register table and flush related callback > functions for Tegra124. > > Signed-off-by: Vince Hsu > [jonath...@nvidia.com: Removed tegra_mc_ops and added > metastable_flush_reads.] > Signed-off-by: Jon Hunter > --- > drivers/memory/tegra/tegra124.c | 31 +++ > 1 file changed, 31 insertions(+) Same comments as for Tegra114. Thierry signature.asc Description: PGP signature
Re: [PATCH V3 03/19] memory: tegra: add flush operation for Tegra30 memory clients
On Mon, Jul 13, 2015 at 01:39:41PM +0100, Jon Hunter wrote:
> From: Vince Hsu
>
> This patch adds the hot reset register table and flush related callback
> functions for Tegra30.
>
> Signed-off-by: Vince Hsu
> [jonath...@nvidia.com: Removed tegra_mc_ops and added
> metastable_flush_reads.]
> Signed-off-by: Jon Hunter
>
> ---
> v3: removal of tegra_mc_ops
> ---
> drivers/memory/tegra/tegra30.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
> index 1abcd8f6f3ba..3b4987f39b52 100644
> --- a/drivers/memory/tegra/tegra30.c
> +++ b/drivers/memory/tegra/tegra30.c
> @@ -6,6 +6,7 @@
> * published by the Free Software Foundation.
> */
>
> +#include
What do you need this for?
> #include
> #include
>
> @@ -936,6 +937,26 @@ static const struct tegra_smmu_swgroup
> tegra30_swgroups[] = {
> { .name = "isp", .swgroup = TEGRA_SWGROUP_ISP, .reg = 0x258 },
> };
>
> +static struct tegra_mc_flush tegra30_mc_flush[] = {
const
> + {TEGRA_SWGROUP_AFI,0x200, 0x204, 0},
> + {TEGRA_SWGROUP_AVPC, 0x200, 0x204, 1},
> + {TEGRA_SWGROUP_DC, 0x200, 0x204, 2},
> + {TEGRA_SWGROUP_DCB,0x200, 0x204, 3},
> + {TEGRA_SWGROUP_EPP,0x200, 0x204, 4},
> + {TEGRA_SWGROUP_G2, 0x200, 0x204, 5},
> + {TEGRA_SWGROUP_HC, 0x200, 0x204, 6},
> + {TEGRA_SWGROUP_HDA,0x200, 0x204, 7},
> + {TEGRA_SWGROUP_ISP,0x200, 0x204, 8},
> + {TEGRA_SWGROUP_MPCORE, 0x200, 0x204, 9},
> + {TEGRA_SWGROUP_MPCORELP, 0x200, 0x204, 10},
> + {TEGRA_SWGROUP_MPE,0x200, 0x204, 11},
> + {TEGRA_SWGROUP_NV, 0x200, 0x204, 12},
> + {TEGRA_SWGROUP_NV2,0x200, 0x204, 13},
> + {TEGRA_SWGROUP_PPCS, 0x200, 0x204, 14},
> + {TEGRA_SWGROUP_VDE,0x200, 0x204, 16},
> + {TEGRA_SWGROUP_VI, 0x200, 0x204, 17},
Spaces around { and }, please.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH V3 02/19] memory: tegra: Add MC flush support
On Mon, Jul 13, 2015 at 01:39:40PM +0100, Jon Hunter wrote: > The Tegra memory controller implements a flush feature to flush pending > accesses and prevent further accesses from occurring. This feature is > used when powering down IP blocks to ensure the IP block is in a good > state. The flushes are organised by software groups and IP blocks are > assigned in hardware to the different software groups. Add helper > functions for requesting a handle to an MC flush for a given > software group and enabling/disabling the MC flush itself. > > This is based upon a change by Vince Hsu . > > Signed-off-by: Jon Hunter > --- > drivers/memory/tegra/mc.c | 110 > ++ > drivers/memory/tegra/mc.h | 2 + > include/soc/tegra/mc.h| 34 ++ > 3 files changed, 146 insertions(+) Do we know if this is actually necessary? I remember having a discussion with Arnd Bergmann a while ago, and the Linux driver model kind of assumes that by the time a device is disabled all outstanding accesses will have stopped. Do we have a way to determine that this even makes a difference? Can we trigger a case where not doing this would cause breakage and see that adding this fixes that particular issue? Thierry signature.asc Description: PGP signature
Re: [PATCH V3 14/19] Documentation: DT: bindings: Add power domain info for NVIDIA PMC
On Mon, Jul 13, 2015 at 01:39:52PM +0100, Jon Hunter wrote:
> Add power-domain binding documentation for the NVIDIA PMC driver in
> order to support generic power-domains.
>
> Signed-off-by: Jon Hunter
> ---
> .../bindings/arm/tegra/nvidia,tegra20-pmc.txt | 99
> ++
> 1 file changed, 99 insertions(+)
>
> diff --git
> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> index 02c27004d4a8..93357a450576 100644
> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> @@ -1,5 +1,7 @@
> NVIDIA Tegra Power Management Controller (PMC)
>
> +== Power Management Controller Node ==
> +
> The PMC block interacts with an external Power Management Unit. The PMC
> mostly controls the entry and exit of the system from different sleep
> modes. It provides power-gating controllers for SoC and CPU power-islands.
> @@ -68,6 +70,10 @@ Optional properties for hardware-triggered thermal reset
> (inside 'i2c-thermtrip'
> Defaults to 0. Valid values are described in section
> 12.5.2
> "Pinmux Support" of the Tegra4 Technical Reference
> Manual.
>
> +Optional nodes:
> +- pm-domains : This node contains a hierarchy of PM domain nodes, which
> should
> +match the power-domains on the Tegra SoC.
Perhaps call this simply power-domains to match the property name in
consumer nodes?
> +
> Example:
>
> / SoC dts including file
> @@ -113,3 +119,96 @@ pmc@7000f400 {
> };
> ...
> };
> +
> +
> +== PM Domain Nodes ==
> +
> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
> +that can be power-gated by the PMC and should be named appropriately.
> +
> +Required properties:
> + - clocks: Must contain an entry for each clock required by
> +the PMC for controlling a power-gate. See
> +../clocks/clock-bindings.txt for details.
> + - resets: Must contain an entry for each reset required by
> +the PMC for controlling a power-gate. See
> +../reset/reset.txt for details.
> + - nvidia,powergate: Integer cell that contains an identifier for
> the
> +PMC power-gate that is associated with the
> +power-domain. Please refer to the Tegra TRM for
> +more details.
I find this really difficult to read. Can we use a more conventional
format such as:
- clocks: Must contain...
... for details.
?
> + - #power-domain-cells: Must be 0.
> +
> +Optional properties:
> + - nvidia,swgroups:Provides details of the software groups that are
> +associated with a specific power-domain. The
> +software group specifier consists of a phandle
> +pointing to the memory controller and a list of
> +one or more integer cells for each software group
> +associated with the power domain. The length of
> +the list of integer cells is specified by
> +#nvidia,swgroup-cells.
> + - #nvidia,swgroup-cells: Must be 1 or more. See nvidia,swgroups for
> +more details.
That's not how this usually works. #*-cells properties determine the
number of cells required per pair. So I think this should
be more along the lines of:
- #nvidia,swgroups: A list of software groups associated with a
specific power domain. This consists of a list of phandle and
SWGROUP ID pairs, where the phandle points to the memory
controller node.
- #nvidia,swgroup-cells: Must be 1. The single cell in the specifier
denotes the ID of the software group.
> +
> +Example:
> +
> + pmc@0,7000e400 {
> + compatible = "nvidia,tegra124-pmc";
> + reg = <0x0 0x7000e400 0x0 0x400>;
> + clocks = <&tegra_car TEGRA124_CLK_PCLK>, <&clk32k_in>;
> + clock-names = "pclk", "clk32k_in";
> +
> + pm-domains {
power-domains would convey more clearly what this is about.
> + ...
> +
> + pd_sor: sor-power-domain {
I don't think it's useful to repeat the -power-domain suffix for all
these nodes.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH V3 18/19] ARM: tegra: add GPU power supply to Jetson TK1 DT
On Mon, Jul 13, 2015 at 01:39:56PM +0100, Jon Hunter wrote:
> From: Vince Hsu
>
> Add power supply information which is board dependent for GK20A.
>
> Signed-off-by: Vince Hsu
> Signed-off-by: Jon Hunter
> ---
> arch/arm/boot/dts/tegra124-jetson-tk1.dts | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> index ce5961cd0b77..2b355957635c 100644
> --- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> +++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> @@ -1514,7 +1514,7 @@
> regulator-always-on;
> };
>
> - sd6 {
> + vdd_gpu: sd6 {
> regulator-name = "+VDD_GPU_AP";
> regulator-min-microvolt = <65>;
> regulator-max-microvolt = <120>;
> @@ -1635,6 +1635,10 @@
> };
> };
>
> + gpu-power-domain {
> + vdd-supply = <&vdd_gpu>;
> + };
This doesn't seem to match the path to the original node.
Thierry
signature.asc
Description: PGP signature
Re: [RESEND PATCH v3] ARM: tegra124: pmu support
On Mon, Jul 13, 2015 at 10:35:45AM -0700, 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.
>
> Updated for proper ordering and to add interrupt-affinity values.
>
> Signed-off-by: Kyle Huey
> ---
> arch/arm/boot/dts/tegra124.dtsi | 17 +
> 1 file changed, 13 insertions(+), 4 deletions(-)
Is there any way to test this? What are the effects of adding this? Does
it enable using perf for profiling?
> diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
> index 13cc7ca..de07d7e 100644
> --- a/arch/arm/boot/dts/tegra124.dtsi
> +++ b/arch/arm/boot/dts/tegra124.dtsi
> @@ -918,31 +918,40 @@
> #address-cells = <1>;
> #size-cells = <0>;
>
> - cpu@0 {
> + A15_0: cpu@0 {
> device_type = "cpu";
> compatible = "arm,cortex-a15";
> reg = <0>;
> };
>
> - cpu@1 {
> + A15_1: cpu@1 {
> device_type = "cpu";
> compatible = "arm,cortex-a15";
> reg = <1>;
> };
>
> - cpu@2 {
> + A15_2: cpu@2 {
> device_type = "cpu";
> compatible = "arm,cortex-a15";
> reg = <2>;
> };
>
> - cpu@3 {
> + A15_3: cpu@3 {
> device_type = "cpu";
> compatible = "arm,cortex-a15";
> reg = <3>;
> };
> };
>
> + pmu {
> + compatible = "arm,cortex-a15-pmu";
> + interrupts = ,
> + ,
> + ,
> + ;
> + interrupt-affinity = <&A15_0>, <&A15_1>, <&A15_2>, <&A15_3>;
These labels look somewhat artificial to me, perhaps we could do
something like the following instead?
interrupt-affinity = <&{/cpus/cpu@0}>, ...;
That's slightly more obvious and avoids the need to "invent" labels for
the CPUs.
No need to respin, I can fix that up when applying if nobody objects to
using the alternative notation.
Thierry
signature.asc
Description: PGP signature
Re: [PATCH v9 1/4] drm/layerscape: Add Freescale DCU DRM driver
On Thu, Jul 16, 2015 at 11:10:29AM +, Wang J.W. wrote:
[...]
> > -Original Message-
> > From: Thierry Reding [mailto:thierry.red...@gmail.com]
[...]
> > On Mon, Jul 13, 2015 at 06:51:29PM +0800, Jianwei Wang wrote:
[...]
> > > DRM DRIVERS FOR NVIDIA TEGRA
> > > M: Thierry Reding
> > > M: Terje Bergström
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index
> > > c46ca31..9cfd14e 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -231,6 +231,8 @@ source "drivers/gpu/drm/virtio/Kconfig"
> > >
> > > source "drivers/gpu/drm/msm/Kconfig"
> > >
> > > +source "drivers/gpu/drm/fsl-dcu/Kconfig"
> > > +
> >
> > As mentioned in a different email, I'm somewhat annoyed by the random
> > placement of these source statements. But we can probably clean that up in
> > one go and insist on proper ordering when there is one.
> >
>
> Ok, I plan to move it to the last line. Is it ok?
It doesn't really matter, it will be inconsistent no matter what because
the current ordering isn't consistent. Keep it where it is for now. I'll
prepare a set of patches to get some consistency into this file.
> > > +int fsl_dcu_drm_connector_create(struct fsl_dcu_drm_device *fsl_dev,
> > > + struct drm_encoder *encoder)
> > > +{
> > > + struct drm_connector *connector = &fsl_dev->connector.connector;
> > > + struct device_node *panel_node;
> > > + int ret;
> > > +
> > > + fsl_dev->connector.encoder = encoder;
> >
> > Why do you need this? The DRM core should set this for you when setting
> > the initial configuration.
> >
>
> This connector is a private structure fsl_dcu_drm_connector. I set it
> for select best encoder.
That doesn't sound right. Technically the DRM core or userspace is going
to select the encoder for your connector, so it'd be better to derive
the pointer to your driver-private structure from struct drm_encoder *,
otherwise you'll end up getting you private copy of the assignment out
of sync with what the DRM core and/or userspace set up.
Note that you might not run into problems now because you only have a
single encoder. But if you ever add support for another you're going to
run into problems with this kind of static assignment.
> > > + dev->irq_enabled = true;
> > > + dev->vblank_disable_allowed = true;
> > > +
> > > + ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0);
> > > + if (ret)
> > > + dev_err(&pdev->dev, "set DCU_INT_STATUS failed\n");
> > > + ret = regmap_read(fsl_dev->regmap, DCU_INT_MASK, &int_mask);
> > > + if (ret)
> > > + dev_err(&pdev->dev, "read DCU_INT_MASK failed\n");
> > > + ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, int_mask &
> > > +~DCU_INT_MASK_VBLANK);
> > > + if (ret)
> > > + dev_err(&pdev->dev, "set DCU_INT_MASK failed\n");
> > > + ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
> > > +DCU_UPDATE_MODE_READREG);
> >
> > What's this DCU_UPDATE_MODE_READREG bit?
> >
>
> In fact, Only when setting DCU_UPDATE_MODE_READREG bit, it begin to
> transfer register values. So setting DCU_UPDATE_MODE_READREG bit is a
> must after registers updating.
Okay, I see. That's going to be convenient for atomic updates.
> > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> > > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> > [...]
> > > +#define DCU_DISP_SIZE0x0018
> > > +#define DCU_DISP_SIZE_DELTA_Y(x) ((x) << 16)
> > > +/*Regisiter value 1/16 of horizontal resolution*/
> > > +#define DCU_DISP_SIZE_DELTA_X(x) ((x) >> 4)
> >
> > Does this mean the display controller only supports horizontal resolutions
> > that are a multiple of 16?
> >
>
> Yes.
You may want to check for this explicitly in the driver and filter out
modes that you don't support.
> > > +#ifdef CONFIG_SOC_VF610
> > > +#define DCU_TOTAL_LAYER_NUM 64
> > > +#define DCU_LAYER_NUM_MAX 6
> > > +#else
> > > +#define DCU_TOTAL_LAYER_NUM 16
> > > +#define DCU_LAYER_NUM_MAX 4
> > > +#endif
> >
> > Should this not be a runtime decision so that the same driver can run on
> > VF610 an
Re: [PATCH v9 3/4] arm/dts/ls1021a: Add DCU dts node
On Mon, Jul 13, 2015 at 06:51:31PM +0800, Jianwei Wang wrote:
> Add DCU node, DCU is a display controller of Freescale
> named 2D-ACE.
>
> Signed-off-by: Alison Wang
> Signed-off-by: Xiubo Li
> Signed-off-by: Jianwei Wang
> Reviewed-by: Alison Wang
> ---
> .../devicetree/bindings/drm/fsl-dcu/fsl,dcu.txt | 20
>
> MAINTAINERS | 1 +
> arch/arm/boot/dts/ls1021a.dtsi | 10 ++
> 3 files changed, 31 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/drm/fsl-dcu/fsl,dcu.txt
>
> diff --git a/Documentation/devicetree/bindings/drm/fsl-dcu/fsl,dcu.txt
> b/Documentation/devicetree/bindings/drm/fsl-dcu/fsl,dcu.txt
> new file mode 100644
> index 000..eb61ea5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/fsl-dcu/fsl,dcu.txt
That's not the proper location for this file. DRM is a Linux-specific
subsystem and hence shouldn't be used in anything devicetree-related.
Documentation/devicetree/bindings/video would be a better location.
Yes, I know other DRM drivers put their binding in the drm subdirectory
but that just makes them equally wrong. I'll make a note to move these
around at some point.
Also the binding really belongs in the same patch as the driver, or a
separate patch altogether.
And, no need for the extra fsl-dcu subdirectory if you have only a
single document in it.
> @@ -0,0 +1,20 @@
> +Device Tree bindings for Freescale DCU DRM Driver
> +
> +Required properties:
> +- compatible: Should be one of
> + * "fsl,ls1021a-dcu".
> + * "fsl,vf610-dcu".
> +- reg: Address and length of the register set for dcu.
> +- clocks: From common clock binding: handle to dcu clock.
> +- clock-names: From common clock binding: Shall be "dcu".
> +- panel: The phandle to panel node.
This isn't a standard property and hence should be prefixed by the
vendor prefix. That is: "fsl,panel".
Also the above uses a weird mix of spaces and tabs for padding. Please
be more consistent.
> +
> +Examples:
> +dcu: dcu@2ce {
> + compatible = "fsl,ls1021a-dcu";
> + reg = <0x0 0x2ce 0x0 0x1>;
> + clocks = <&platform_clk 0>;
> + clock-names = "dcu";
> + big-endian;
This property isn't mentioned above. I know it's pretty obvious what it
does, but might still be worth briefly describing what it is. I'm also
wondering if that's not something that could be inferred from the
compatible string.
> + panel = <&panel>;
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e191ded..fffb8c9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3410,6 +3410,7 @@ M: Alison Wang
> L: dri-de...@lists.freedesktop.org
> S: Supported
> F: drivers/gpu/drm/fsl-dcu/
> +F: Documentation/devicetree/bindings/drm/fsl-dcu/
You might want to shuffle around some of these hunks, so that this
particular hunk is included in the driver patch along with the binding
and the panel patch doesn't remove it only for it to be readded here.
Thierry
> F: Documentation/devicetree/bindings/panel/nec,nl4827hc19_05b.txt
>
> DRM DRIVERS FOR NVIDIA TEGRA
> diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
> index c70bb27..6d6e3e2 100644
> --- a/arch/arm/boot/dts/ls1021a.dtsi
> +++ b/arch/arm/boot/dts/ls1021a.dtsi
> @@ -383,6 +383,16 @@
><&platform_clk 1>;
> };
>
> + dcu: dcu@2ce {
> + compatible = "fsl,ls1021a-dcu";
> + reg = <0x0 0x2ce 0x0 0x1>;
> + interrupts = ;
> + clocks = <&platform_clk 0>;
> + clock-names = "dcu";
> + big-endian;
> + status = "disabled";
> + };
> +
> mdio0: mdio@2d24000 {
> compatible = "gianfar";
> device_type = "mdio";
> --
> 2.1.0.27.g96db324
>
signature.asc
Description: PGP signature
