* Re: [PATCH] dt-bindings: riscv: Limit cpus schema to only check RiscV 'cpu' nodes
From: Rob Herring @ 2019-07-16 15:08 UTC (permalink / raw)
To: Paul Walmsley, linux-riscv, Palmer Dabbelt
Cc: Lorenzo Pieralisi, devicetree, linux-kernel@vger.kernel.org
In-Reply-To: <20190626235759.3615-1-robh@kernel.org>
On Wed, Jun 26, 2019 at 6:00 PM Rob Herring <robh@kernel.org> wrote:
>
> Matching on the 'cpus' node was a bad choice because the schema is
> incorrectly applied to non-RiscV cpus nodes. As we now have a common cpus
> schema which checks the general structure, it is also redundant to do so
> in the Risc-V CPU schema.
>
> The downside is one could conceivably mix different architecture's cpu
> nodes or have typos in the compatible string. The latter problem pretty
> much exists for every schema.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> .../devicetree/bindings/riscv/cpus.yaml | 143 ++++++++----------
> 1 file changed, 61 insertions(+), 82 deletions(-)
Paul, do you plan to apply this? I have several fixes to send to Linus
if you want me to include this.
Rob
^ permalink raw reply
* Re: [PATCH] dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
From: Rob Herring @ 2019-07-16 15:04 UTC (permalink / raw)
To: Joel Stanley
Cc: devicetree, linux-aspeed, Andrew Jeffery, Linus Walleij,
open list:GPIO SUBSYSTEM, Linux ARM
In-Reply-To: <CACPK8Xdz98CQzgE2KCjz8eOhPtx=H8jTe1hVT7LvP77U_gGASQ@mail.gmail.com>
On Mon, Jul 15, 2019 at 5:17 PM Joel Stanley <joel@jms.id.au> wrote:
>
> On Mon, 15 Jul 2019 at 22:37, Rob Herring <robh@kernel.org> wrote:
> >
> > The Aspeed pinctl schema have errors in the 'compatible' schema:
> >
> > Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml: \
> > properties:compatible:enum: ['aspeed', 'ast2400-pinctrl', 'aspeed', 'g4-pinctrl'] has non-unique elements
> > Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml: \
> > properties:compatible:enum: ['aspeed', 'ast2500-pinctrl', 'aspeed', 'g5-pinctrl'] has non-unique elements
> >
> > Flow style sequences have to be quoted if the vales contain ','. Fix
> > this by using the more common one line per entry formatting.
>
> >
> > properties:
> > compatible:
> > - enum: [ aspeed,ast2400-pinctrl, aspeed,g4-pinctrl ]
> > + enum:
> > + - aspeed,ast2400-pinctrl
> > + - aspeed,g4-pinctrl
>
> Thanks for the fix. However, we've standardised on the first form for
> all of our device trees, so we can drop the second compatible string
> from the bindings.
Doing that would introduce validation warnings until the dts file is
updated. So we still need this change until that happens.
Rob
^ permalink raw reply
* Re: [RFC PATCH v2 00/15] tracing: of: Boot time tracing using devicetree
From: Masami Hiramatsu @ 2019-07-16 15:02 UTC (permalink / raw)
To: Frank Rowand
Cc: Steven Rostedt, Rob Herring, Tim Bird, Ingo Molnar, Namhyung Kim,
Jiri Olsa, Arnaldo Carvalho de Melo, Tom Zanussi, linux-kernel,
devicetree
In-Reply-To: <488a65e6-1d80-0acb-5092-80c18b7ff447@gmail.com>
Hi Frank,
On Mon, 15 Jul 2019 07:21:27 -0700
Frank Rowand <frowand.list@gmail.com> wrote:
> Hi Masami,
>
> After receiving this email, I replied to one email on the v1 thread,
> so there will be a little bit of overlap in the ordering of the two
> threads. Feel free to reply to my comments in the v1 thread in this
> thread instead.
OK, thanks for the notice :)
>
> More comments below.
>
> On 7/14/19 10:11 PM, Masami Hiramatsu wrote:> Hello,
[...]
> >
> > Discussion
> > =====
> > On the previous thread, we discussed that the this devicetree usage
> > itself was acceptable or not. Fortunately, I had a chance to discuss
> > it in a F2F meeting with Frank and Tim last week.
>
> Thanks for writing up some of what we discussed.
>
> Let me add a problem statement and use case. I'll probably get it at least
> a little bit wrong, so please update as needed.
>
> (1) You feel the ftrace kernel command line syntax is not sufficiently user
> friendly.
>
> (2) The kernel command line is too small to contain the full set of desired
> ftrace commands and options.
>
> (3) There is a desire to change the boot time ftrace commands and options
> without re-compiling or re-linking the Linux kernel.
Thank you for covering these items :) Yes, these are what I'm thinking.
> >
> > I think the advantages of using devicetree are,
> >
> > - reuse devicetree's structured syntax for complicated tracefs settings
> > - reuse OF-APIs in linux kernel to accept and parse it
> > - reuse dtc complier to compile it and validate syntax. (with yaml schema,
> > we can enhance it)
> > - reuse current bootloader (and qemu) to load it
>
> Devicetree is not a universal data structure and communication channel.
>
> Devicetree is a description of the hardware and also conveys bootloader
> specific information that is need by the kernel to boot.
Yes, I see. But I think there is a room to contain a small communication
channel under /chosen, from bootloader.
>
> > And we talked about some other ideas to avoid using devicetree.
> >
> > - expand kernel command line (ascii command strings)
> > - expand kernel command line with base64 encoded comressed ascii command
> > strings
>
> Base64 being one of possibly many ways to convert arbitrary binary data to
> ascii safe data _if_ you want to transfer the ftrace options and commands
> in a binary format.
I actually don't want it :( but if the ascii commands can be compressed
(maybe not so efficient), it is a possible way.
> > - load (compressed) ascii command strings to somewhere on memory and pass
> > the address via kernel cmdline
>
> Similar to the way initrd is handled, if I understand correctly. (I am not
> up to date on how initrd location is passed to the kernel for a non-devicetree
> kernel.)
Initrd is not passed via kernel cmdline, it is loaded and passed via architecture
dependent way. As far as I know, x86 and arm (without DT) uses own data structure,
arm64 (and arm with DT) uses devicetree /chosen node.
> Compressed or not compressed would be an ftrace design choice.
>
>
> > - load (compressed) ascii command strings to somewhere on memory and pass
> > the address via /chosen node (as same as initrd)
>
> Compressed or not compressed would be an ftrace design choice.
>
Yes, it is optional.
>
> > - load binary C data and point it from kernel cmdline
> > - load binary C data and point it from /chosen node (as same as initrd)
> > - load binary C data as a section of kernel image
>
> For the three options above:
>
> Binary data if ftrace prefers structured data.
> A list of strings if ftrace wants to use the existing kernel command line
> syntax.
Yes, any data which doesn't need complex parser is OK.
>
> For the third of the above three options, the linker would provide the start
> and end address of the ftrace options and commands section.
But that means we need to fill the data structure when we build the kernel,
isn't it?
> > The first 2 ideas expand the kernel's cmdline to pass some "magic" command
> > to setup ftrace. In both case, the problems are the maximal size of cmdline
> > and the issues related to the complexity of commands.
>
> Not a "magic" command. Either continue using the existing ftrace syntax or
> add something like: ftrace_cmd="whatever format ftrace desires".
>
> Why can the maximum size of the cmdline not be increased?
We can, but we also has to change bootloaders.
> > My example showed that the ftrace settings becomes long even if making one
> > histogram, which can be longer than 256 bytes. The long and complex data
> > can easily lead mis-typing, but cmdline has no syntax validator, it just
> > ignores the mis-typed commands.
>
> Hand typing a kernel command line is already not a fun exercise, even
> before adding ftrace commands. If you are hand typing kernel command
> lines then I suggest you improve your tools (eg bootloader or whatever
> is not allowing you to edit and store command lines).
Indeed, if we extend kernel cmdline to support it, such tool we have to
introduce (like dtc)
> > (Of course even with the devicetree, it must be smaller than 2 pages)
> >
> > Next 2 ideas are similar, but load the commands on some other memory area
> > and pass only address via cmdline. This solves the size limitation issue,
> > but still no syntax validation. Of course we can make a new structured
> > syntax validator similar to (or just forked from) dt-validate.
> > The problem (or disadvantage) of these (and following) ideas, is to change
> > the kernel and boot loaders to load another binary blobs on memory.
> >
> > Maybe if we introduce a generic structured kernel boot arguments, which is
> > a kind of /chosen node of devicetree. (But if there is already such hook,
> > why we make another one...?)
>
> I got lost in the next sentence, so for my benefit:
> GSKBA == generic structured kernel boot arguments
Oh, sorry, that's my bad.
> > Also, this "GSKBA" may introduce a parser and access APIs which will be
> > very similar to OF-APIs. This also seems redundant to me.
>
>
> > So the last 3 ideas will avoid introducing new parser and APIs, we just
> > compile the data as C data and point it from cmdline or somewhere else.
>
> Or if in a kernel data section then the linker can provide the begin and
> end address of the blob. This is already implemented for some other data
> structures.
Yeah, but does that mean we have to rebuild kernel image?
In some cases, (e.g. debugging distro kernel) we can not modify kernel image
also, I don't like to replace entire image. I would like to choose a tracing
command file from boot loader.
> > With these ideas, we still need to expand boot loaders to support
> > loading new binary blobs. (And the last one requires to add elf header
> > parser/modifier to boot loader too)
>
> Why would the boot loader need to access the elf header? The linker
> can provide the location of the new kernel data section via kernel
> variables.
Oh, I thought you meant that the new data was added boot time by
boot loader.
> >>From the above reasons, I think using devicetree's /chosen node is
> > the least intrusive way to introduce this boot-time tracing feature.
>
> This is still mis-use of the devicetree data structure. This data
> does not belong in the devicetree.
I think if the boot loader supports overlay file, we can choose
the overlay file when booting for /chosen node. That can be a
part of boot loader choice, isn't it? :)
Thank you,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply
* [PATCH v3 02/11] dt-bindings: clock: imx-lpcg: add support to parse clocks from device tree
From: Dong Aisheng @ 2019-07-16 15:00 UTC (permalink / raw)
To: linux-clk
Cc: Dong Aisheng, devicetree, sboyd, mturquette, Rob Herring,
linux-imx, kernel, fabio.estevam, shawnguo, linux-arm-kernel
In-Reply-To: <1563289265-10977-1-git-send-email-aisheng.dong@nxp.com>
MX8QM and MX8QXP LPCG Clocks are mostly the same except they may reside
in different subsystems across CPUs and also vary a bit on the availability.
Same as SCU clock, we want to move the clock definition into device tree
which can fully decouple the dependency of Clock ID definition from device
tree and make us be able to write a fully generic lpcg clock driver.
And we can also use the existence of clock nodes in device tree to address
the device and clock availability differences across different SoCs.
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
v2->v3:
* no changes
v1->v2:
* Update example
* Add power domain property
---
.../devicetree/bindings/clock/imx8qxp-lpcg.txt | 34 ++++++++++++++++++----
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
index 965cfa4..6fc2fd8 100644
--- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
+++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
@@ -11,6 +11,21 @@ enabled by these control bits, it might still not be running based
on the base resource.
Required properties:
+- compatible: Should be one of:
+ "fsl,imx8qxp-lpcg"
+ "fsl,imx8qm-lpcg" followed by "fsl,imx8qxp-lpcg".
+- reg: Address and length of the register set.
+- #clock-cells: Should be 1. One LPCG supports multiple clocks.
+- clocks: Input parent clocks phandle array for each clock.
+- bit-offset: An integer array indicating the bit offset for each clock.
+- hw-autogate: Boolean array indicating whether supports HW autogate for
+ each clock.
+- clock-output-names: Shall be the corresponding names of the outputs.
+ NOTE this property must be specified in the same order
+ as the clock bit-offset and hw-autogate property.
+- power-domains: Should contain the power domain used by this clock.
+
+Legacy binding (DEPRECATED):
- compatible: Should be one of:
"fsl,imx8qxp-lpcg-adma",
"fsl,imx8qxp-lpcg-conn",
@@ -33,10 +48,17 @@ Examples:
#include <dt-bindings/clock/imx8qxp-clock.h>
-conn_lpcg: clock-controller@5b200000 {
- compatible = "fsl,imx8qxp-lpcg-conn";
- reg = <0x5b200000 0xb0000>;
+sdhc0_lpcg: clock-controller@5b200000 {
+ compatible = "fsl,imx8qxp-lpcg";
+ reg = <0x5b200000 0x10000>;
#clock-cells = <1>;
+ clocks = <&sdhc0_clk IMX_SC_PM_CLK_PER>,
+ <&conn_ipg_clk>, <&conn_axi_clk>;
+ bit-offset = <0 16 20>;
+ clock-output-names = "sdhc0_lpcg_per_clk",
+ "sdhc0_lpcg_ipg_clk",
+ "sdhc0_lpcg_ahb_clk";
+ power-domains = <&pd IMX_SC_R_SDHC_0>;
};
usdhc1: mmc@5b010000 {
@@ -44,8 +66,8 @@ usdhc1: mmc@5b010000 {
interrupt-parent = <&gic>;
interrupts = <GIC_SPI 232 IRQ_TYPE_LEVEL_HIGH>;
reg = <0x5b010000 0x10000>;
- clocks = <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_IPG_CLK>,
- <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_PER_CLK>,
- <&conn_lpcg IMX8QXP_CONN_LPCG_SDHC0_HCLK>;
+ clocks = <&sdhc0_lpcg 1>,
+ <&sdhc0_lpcg 0>,
+ <&sdhc0_lpcg 2>;
clock-names = "ipg", "per", "ahb";
};
--
2.7.4
^ permalink raw reply related
* [PATCH v3 01/11] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree
From: Dong Aisheng @ 2019-07-16 15:00 UTC (permalink / raw)
To: linux-clk
Cc: Dong Aisheng, devicetree, sboyd, mturquette, Rob Herring,
linux-imx, kernel, fabio.estevam, shawnguo, linux-arm-kernel
In-Reply-To: <1563289265-10977-1-git-send-email-aisheng.dong@nxp.com>
There's a few limitations on the original one cell clock binding
(#clock-cells = <1>) that we have to define some SW clock IDs for device
tree to reference. This may cause troubles if we want to use common
clock IDs for multi platforms support when the clock of those platforms
are mostly the same.
e.g. Current clock IDs name are defined with SS prefix.
However the device may reside in different SS across CPUs, that means the
SS prefix may not valid anymore for a new SoC. Furthermore, the device
availability of those clocks may also vary a bit.
For such situation, we want to eliminate the using of SW Clock IDs and
change to use a more close to HW one instead.
For SCU clocks usage, only two params required: Resource id + Clock Type.
Both parameters are platform independent. So we could use two cells binding
to pass those parameters,
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
v2->v3:
* Changed to two cells binding and register all clocks in driver
instead of parse from device tree.
v1->v2:
* changed to one cell binding inspired by arm,scpi.txt
Documentation/devicetree/bindings/arm/arm,scpi.txt
Resource ID is encoded in 'reg' property.
Clock type is encoded in generic clock-indices property.
Then we don't have to search all the DT nodes to fetch
those two value to construct clocks which is relatively
low efficiency.
* Add required power-domain property as well.
---
.../devicetree/bindings/arm/freescale/fsl,scu.txt | 12 +++++++-----
include/dt-bindings/firmware/imx/rsrc.h | 17 +++++++++++++++++
2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index 5d7dbab..351d335 100644
--- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -89,7 +89,10 @@ Required properties:
"fsl,imx8qm-clock"
"fsl,imx8qxp-clock"
followed by "fsl,scu-clk"
-- #clock-cells: Should be 1. Contains the Clock ID value.
+- #clock-cells: Should be either
+ 2: Contains the Resource and Clock ID value.
+ or
+ 1: Contains the Clock ID value. (DEPRECATED)
- clocks: List of clock specifiers, must contain an entry for
each required entry in clock-names
- clock-names: Should include entries "xtal_32KHz", "xtal_24MHz"
@@ -162,7 +165,7 @@ firmware {
clk: clk {
compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
- #clock-cells = <1>;
+ #clock-cells = <2>;
};
iomuxc {
@@ -192,8 +195,7 @@ serial@5a060000 {
...
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_lpuart0>;
- clocks = <&clk IMX8QXP_UART0_CLK>,
- <&clk IMX8QXP_UART0_IPG_CLK>;
- clock-names = "per", "ipg";
+ clocks = <&uart0_clk IMX_SC_R_UART_0 IMX_SC_PM_CLK_PER>;
+ clock-names = "ipg";
power-domains = <&pd IMX_SC_R_UART_0>;
};
diff --git a/include/dt-bindings/firmware/imx/rsrc.h b/include/dt-bindings/firmware/imx/rsrc.h
index 4e61f64..fbeaca7 100644
--- a/include/dt-bindings/firmware/imx/rsrc.h
+++ b/include/dt-bindings/firmware/imx/rsrc.h
@@ -547,4 +547,21 @@
#define IMX_SC_R_ATTESTATION 545
#define IMX_SC_R_LAST 546
+/*
+ * Defines for SC PM CLK
+ */
+#define IMX_SC_PM_CLK_SLV_BUS 0 /* Slave bus clock */
+#define IMX_SC_PM_CLK_MST_BUS 1 /* Master bus clock */
+#define IMX_SC_PM_CLK_PER 2 /* Peripheral clock */
+#define IMX_SC_PM_CLK_PHY 3 /* Phy clock */
+#define IMX_SC_PM_CLK_MISC 4 /* Misc clock */
+#define IMX_SC_PM_CLK_MISC0 0 /* Misc 0 clock */
+#define IMX_SC_PM_CLK_MISC1 1 /* Misc 1 clock */
+#define IMX_SC_PM_CLK_MISC2 2 /* Misc 2 clock */
+#define IMX_SC_PM_CLK_MISC3 3 /* Misc 3 clock */
+#define IMX_SC_PM_CLK_MISC4 4 /* Misc 4 clock */
+#define IMX_SC_PM_CLK_CPU 2 /* CPU clock */
+#define IMX_SC_PM_CLK_PLL 4 /* PLL */
+#define IMX_SC_PM_CLK_BYPASS 4 /* Bypass clock */
+
#endif /* __DT_BINDINGS_RSCRC_IMX_H */
--
2.7.4
^ permalink raw reply related
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-16 15:00 UTC (permalink / raw)
To: Peter De Schrijver, Joseph Lo, Sowjanya Komatineni
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <20190716080610.GE12715@pdeschrijver-desktop.Nvidia.com>
16.07.2019 11:06, Peter De Schrijver пишет:
> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
>>> OK, Will add to CPUFreq driver...
>>>>
>>>> The other thing that also need attention is that T124 CPUFreq driver
>>>> implicitly relies on DFLL driver to be probed first, which is icky.
>>>>
>>> Should I add check for successful dfll clk register explicitly in
>>> CPUFreq driver probe and defer till dfll clk registers?
Probably you should use the "device links". See [1][2] for the example.
[1]
https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
[2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
use of_find_device_by_node() to get the DFLL's device, see [3].
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>> Sorry, I didn't follow the mail thread. Just regarding the DFLL part.
>>
>> As you know it, the DFLL clock is one of the CPU clock sources and
>> integrated with DVFS control logic with the regulator. We will not switch
>> CPU to other clock sources once we switched to DFLL. Because the CPU has
>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table you see
>> in the driver.). We shouldn't reparent it to other sources with unknew
>> freq/volt pair. That's not guaranteed to work. We allow switching to
>> open-loop mode but different sources.
Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
rate before switching to PLLP in order to have a proper CPU voltage.
>> And I don't exactly understand why we need to switch to PLLP in CPU idle
>> driver. Just keep it on CL-DVFS mode all the time.
>>
>> In SC7 entry, the dfll suspend function moves it the open-loop mode. That's
>> all. The sc7-entryfirmware will handle the rest of the sequence to turn off
>> the CPU power.
>>
>> In SC7 resume, the warmboot code will handle the sequence to turn on
>> regulator and power up the CPU cluster. And leave it on PLL_P. After
>> resuming to the kernel, we re-init DFLL, restore the CPU clock policy (CPU
>> runs on DFLL open-loop mode) and then moving to close-loop mode.
The DFLL is re-inited after switching CCLK to DFLL parent during of the
early clocks-state restoring by CaR driver. Hence instead of having odd
hacks in the CaR driver, it is much nicer to have a proper
suspend-resume sequencing of the device drivers. In this case CPUFreq
driver is the driver that enables DFLL and switches CPU to that clock
source, which means that this driver is also should be responsible for
management of the DFLL's state during of suspend/resume process. If
CPUFreq driver disables DFLL during suspend and re-enables it during
resume, then looks like the CaR driver hacks around DFLL are not needed.
>> The DFLL part looks good to me. BTW, change the patch subject to "Add
>> suspend-resume support" seems more appropriate to me.
>>
>
> To clarify this, the sequences for DFLL use are as follows (assuming all
> required DFLL hw configuration has been done)
>
> Switch to DFLL:
> 0) Save current parent and frequency
> 1) Program DFLL to open loop mode
> 2) Enable DFLL
> 3) Change cclk_g parent to DFLL
> For OVR regulator:
> 4) Change PWM output pin from tristate to output
> 5) Enable DFLL PWM output
> For I2C regulator:
> 4) Enable DFLL I2C output
> 6) Program DFLL to closed loop mode
>
> Switch away from DFLL:
> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for any vdd_cpu voltage
> 1) Program DFLL to open loop mode
>
> For OVR regulator:
> 2) Change PWM output pin from output to tristate: vdd_cpu will go back
> to hardwired boot voltage.
> 3) Disable DFLL PWM output
>
> For I2C regulator:
> 2) Program vdd_cpu regulator voltage to the boot voltage
> 3) Disable DFLL I2C output
>
> 4) Reprogram parent saved in step 0 of 'Switch to DFLL' to the saved
> frequency
> 5) Change cclk_g parent to saved parent
> 6) Disable DFLL
Thanks!
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Rob Herring @ 2019-07-16 14:57 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-mmc, Ulf Hansson, Mark Rutland, Joel Stanley, Adrian Hunter,
devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-aspeed, linux-kernel@vger.kernel.org, Ryan Chen
In-Reply-To: <3fe55ea9-b949-48a0-9eab-90ad3bc1ee2a@www.fastmail.com>
On Mon, Jul 15, 2019 at 6:36 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote:
> > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> > > data bus if only a single slot is enabled.
> > >
> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > ---
> > > In v2:
> > >
> > > * Rename to aspeed,sdhci.yaml
> > > * Rename sd-controller compatible
> > > * Add `maxItems: 1` for reg properties
> > > * Move sdhci subnode description to patternProperties
> > > * Drop sdhci compatible requirement
> > > * #address-cells and #size-cells are required
> > > * Prevent additional properties
> > > * Implement explicit ranges in example
> > > * Remove slot property
> > >
> > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> > > 1 file changed, 90 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > new file mode 100644
> > > index 000000000000..67a691c3348c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > @@ -0,0 +1,90 @@
> > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ASPEED SD/SDIO/eMMC Controller
> > > +
> > > +maintainers:
> > > + - Andrew Jeffery <andrew@aj.id.au>
> > > + - Ryan Chen <ryanchen.aspeed@gmail.com>
> > > +
> > > +description: |+
> > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> > > + only a single slot is enabled.
> > > +
> > > + The two slots are supported by a common configuration area. As the SDHCIs for
> > > + the slots are dependent on the common configuration area, they are described
> > > + as child nodes.
> > > +
> > > +properties:
> > > + compatible:
> > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
> >
> > This is actually a list of 4 strings. Please reformat to 1 per line.
>
> On reflection that's obvious, but also a somewhat subtle interaction with the
> preference for no quotes (the obvious caveat being "except where required").
It wasn't something I'd run into before. I'm working on a check, but
unfortunately we can only check for quotes not needed and can't check
for missing quotes.
> Thanks for pointing it out.
>
> I have been running `make dt_binding_check` and `make dtbs_check` over
> these, looks like I need to up my game a bit though. Do you do additional things
> in your workflow?
That should have thrown the warnings. If you aren't seeing those, do
you have dtschema package installed (see
Documentation/devicetree/writing-schema.md)? Or it could be erroring
out on something else first. There's a few breakages that I'm trying
to fix.
Rob
^ permalink raw reply
* Re: [EXT] Re: [PATCH 00/10] Improvements and fixes for mxsfb DRM driver
From: Guido Günther @ 2019-07-16 14:54 UTC (permalink / raw)
To: Robert Chiras
Cc: marex@denx.de, devicetree@vger.kernel.org, kernel@pengutronix.de,
airlied@linux.ie, shawnguo@kernel.org,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
stefan@agner.ch, robh+dt@kernel.org, dl-linux-imx,
daniel@ffwll.ch, mark.rutland@arm.com, festevam@gmail.com,
s.hauer@pengutronix.de, linux-arm-kernel@lists.infradead.org
In-Reply-To: <1562919331.3209.11.camel@nxp.com>
Hi Robert,
On Fri, Jul 12, 2019 at 08:15:32AM +0000, Robert Chiras wrote:
> Hi Guido,
>
> On Jo, 2019-07-11 at 17:04 +0200, Guido Günther wrote:
> > Hi Robert,
> > On Wed, Jun 26, 2019 at 04:32:08PM +0300, Robert Chiras wrote:
> > >
> > > This patch-set improves the use of eLCDIF block on iMX 8 SoCs (like
> > > 8MQ, 8MM
> > > and 8QXP). Following, are the new features added and fixes from
> > > this
> > > patch-set:
> > >
> > > 1. Add support for drm_bridge
> > > On 8MQ and 8MM, the LCDIF block is not directly connected to a
> > > parallel
> > > display connector, where an LCD panel can be attached, but instead
> > > it is
> > > connected to DSI controller. Since this DSI stands between the
> > > display
> > > controller (eLCDIF) and the physical connector, the DSI can be
> > > implemented
> > > as a DRM bridge. So, in order to be able to connect the mxsfb
> > > driver to
> > > the DSI driver, the support for a drm_bridge was needed in mxsfb
> > > DRM
> > > driver (the actual driver for the eLCDIF block).
> > So I wanted to test this but with both my somewhat cleaned up nwl
> > driver¹ and the nwl driver forward ported from the nxp vendor tree
> > I'm
> > looking at a black screen with current mainline - while my dcss
> > forward
> > port gives me nice output on mipi dsi. Do you have a tree that uses
> > mipi
> > dsi on imx8mq where I could look at to check for differences?
> Somewhere on the pixel path (between the display controller and the
> DSI) there is a block that inverts the polarity. I can't remember
> exactly what was the role of this block, but the polarity is inverted
> when eLCDIF is used in combination with the DSI.
> If you take a look at my DSI driver from NXP releases (I guess you have
> them), you will see there is a hack in mode_fixup:
>
> unsigned int *flags = &mode->flags;
> if (dsi->sync_pol {
> *flags |= DRM_MODE_FLAG_PHSYNC;
> *flags |= DRM_MODE_FLAG_PVSYNC;
> *flags &= ~DRM_MODE_FLAG_NHSYNC;
> *flags &= ~DRM_MODE_FLAG_NVSYNC;
> } else {
> *flags &= ~DRM_MODE_FLAG_PHSYNC;
> *flags &= ~DRM_MODE_FLAG_PVSYNC;
> *flags |= DRM_MODE_FLAG_NHSYNC;
> *flags |= DRM_MODE_FLAG_NVSYNC;
> }
Thanks for the suggestion! I'll try that.
>
> I know it's not clean, but it works for now. You can try this in your
> driver and see if it helps.
> These days I will also take your nwl-dsi driver and test it, and also
> add support for bridge and eLCDIF to see if I can make it work.
I have hacky bridge support over here already. Give me some days to
clean it up and it might safe you some work.
Cheers,
-- Guido
^ permalink raw reply
* [PATCH v4 2/2] arm64: dts: nxp: add ls1046a-frwy board support
From: Pramod Kumar @ 2019-07-16 13:43 UTC (permalink / raw)
To: robh+dt@kernel.org, mark.rutland@arm.com, shawnguo@kernel.org,
manivannan.sadhasivam@linaro.org, Aisheng Dong,
Michal.Vokac@ysoft.com, Leo Li
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Pramod Kumar, Vabhav Sharma
In-Reply-To: <1563284586-29928-1-git-send-email-pramod.kumar_1@nxp.com>
ls1046afrwy board is based on nxp ls1046a SoC.
Board support's 4GB ddr memory, i2c, microSD card,
serial console,qspi nor flash,ifc nand flash,qsgmii network interface,
usb 3.0 and serdes interface to support two x1gen3 pcie interface.
Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
Signed-off-by: Pramod Kumar <pramod.kumar_1@nxp.com>
---
arch/arm64/boot/dts/freescale/Makefile | 1 +
arch/arm64/boot/dts/freescale/fsl-ls1046a-frwy.dts | 156 +++++++++++++++++++++
2 files changed, 157 insertions(+)
create mode 100644 arch/arm64/boot/dts/freescale/fsl-ls1046a-frwy.dts
diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
index 0bd122f..1211531 100644
--- a/arch/arm64/boot/dts/freescale/Makefile
+++ b/arch/arm64/boot/dts/freescale/Makefile
@@ -8,6 +8,7 @@ dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1028a-qds.dtb
dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1028a-rdb.dtb
dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1043a-qds.dtb
dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1043a-rdb.dtb
+dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1046a-frwy.dtb
dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1046a-qds.dtb
dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1046a-rdb.dtb
dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1088a-qds.dtb
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a-frwy.dts b/arch/arm64/boot/dts/freescale/fsl-ls1046a-frwy.dts
new file mode 100644
index 0000000..cda4998
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a-frwy.dts
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Device Tree Include file for Freescale Layerscape-1046A family SoC.
+ *
+ * Copyright 2019 NXP.
+ *
+ */
+
+/dts-v1/;
+
+#include "fsl-ls1046a.dtsi"
+
+/ {
+ model = "LS1046A FRWY Board";
+ compatible = "fsl,ls1046a-frwy", "fsl,ls1046a";
+
+ aliases {
+ serial0 = &duart0;
+ serial1 = &duart1;
+ serial2 = &duart2;
+ serial3 = &duart3;
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ sb_3v3: regulator-sb3v3 {
+ compatible = "regulator-fixed";
+ regulator-name = "LT8642SEV-3.3V";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+};
+
+&duart0 {
+ status = "okay";
+};
+
+&duart1 {
+ status = "okay";
+};
+
+&duart2 {
+ status = "okay";
+};
+
+&duart3 {
+ status = "okay";
+};
+
+&i2c0 {
+ status = "okay";
+
+ i2c-mux@77 {
+ compatible = "nxp,pca9546";
+ reg = <0x77>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+
+ power-monitor@40 {
+ compatible = "ti,ina220";
+ reg = <0x40>;
+ shunt-resistor = <1000>;
+ };
+
+
+ temperature-sensor@4c {
+ compatible = "nxp,sa56004";
+ reg = <0x4c>;
+ vcc-supply = <&sb_3v3>;
+ };
+
+ rtc@51 {
+ compatible = "nxp,pcf2129";
+ reg = <0x51>;
+ };
+
+ eeprom@52 {
+ compatible = "atmel,24c512";
+ reg = <0x52>;
+ };
+
+ eeprom@53 {
+ compatible = "atmel,24c512";
+ reg = <0x53>;
+ };
+
+ };
+ };
+};
+
+&ifc {
+ #address-cells = <2>;
+ #size-cells = <1>;
+ /* NAND Flash */
+ ranges = <0x0 0x0 0x0 0x7e800000 0x00010000>;
+ status = "okay";
+
+ nand@0,0 {
+ compatible = "fsl,ifc-nand";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x0 0x0 0x10000>;
+ };
+
+};
+
+#include "fsl-ls1046-post.dtsi"
+
+&fman0 {
+ ethernet@e0000 {
+ phy-handle = <&qsgmii_phy4>;
+ phy-connection-type = "qsgmii";
+ };
+
+ ethernet@e8000 {
+ phy-handle = <&qsgmii_phy2>;
+ phy-connection-type = "qsgmii";
+ };
+
+ ethernet@ea000 {
+ phy-handle = <&qsgmii_phy1>;
+ phy-connection-type = "qsgmii";
+ };
+
+ ethernet@f2000 {
+ phy-handle = <&qsgmii_phy3>;
+ phy-connection-type = "qsgmii";
+ };
+
+ mdio@fd000 {
+ qsgmii_phy1: ethernet-phy@1c {
+ reg = <0x1c>;
+ };
+
+ qsgmii_phy2: ethernet-phy@1d {
+ reg = <0x1d>;
+ };
+
+ qsgmii_phy3: ethernet-phy@1e {
+ reg = <0x1e>;
+ };
+
+ qsgmii_phy4: ethernet-phy@1f {
+ reg = <0x1f>;
+ };
+ };
+};
--
2.7.4
^ permalink raw reply related
* [PATCH v4 1/2] dt-bindings: arm: nxp: Add device tree binding for ls1046a-frwy board
From: Pramod Kumar @ 2019-07-16 13:43 UTC (permalink / raw)
To: robh+dt@kernel.org, mark.rutland@arm.com, shawnguo@kernel.org,
manivannan.sadhasivam@linaro.org, Aisheng Dong,
Michal.Vokac@ysoft.com, Leo Li
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Pramod Kumar, Vabhav Sharma
In-Reply-To: <1563284586-29928-1-git-send-email-pramod.kumar_1@nxp.com>
Add "fsl,ls1046a-frwy" bindings for ls1046afrwy board based on ls1046a SoC
Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
Signed-off-by: Pramod Kumar <pramod.kumar_1@nxp.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/arm/fsl.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
index 407138e..86e9821 100644
--- a/Documentation/devicetree/bindings/arm/fsl.yaml
+++ b/Documentation/devicetree/bindings/arm/fsl.yaml
@@ -239,6 +239,7 @@ properties:
- description: LS1046A based Boards
items:
- enum:
+ - fsl,ls1046a-frwy
- fsl,ls1046a-qds
- fsl,ls1046a-rdb
- const: fsl,ls1046a
--
2.7.4
^ permalink raw reply related
* [PATCH v4 0/2] arm64: dts: nxp: add ls1046a frwy board support
From: Pramod Kumar @ 2019-07-16 13:43 UTC (permalink / raw)
To: robh+dt@kernel.org, mark.rutland@arm.com, shawnguo@kernel.org,
manivannan.sadhasivam@linaro.org, Aisheng Dong,
Michal.Vokac@ysoft.com, Leo Li
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Pramod Kumar
changes for v4:
incorporated shawn review comment
Pramod Kumar (2):
dt-bindings: arm: nxp: Add device tree binding for ls1046a-frwy board
arm64: dts: nxp: add ls1046a-frwy board support
Documentation/devicetree/bindings/arm/fsl.yaml | 1 +
arch/arm64/boot/dts/freescale/Makefile | 1 +
arch/arm64/boot/dts/freescale/fsl-ls1046a-frwy.dts | 156 +++++++++++++++++++++
3 files changed, 158 insertions(+)
create mode 100644 arch/arm64/boot/dts/freescale/fsl-ls1046a-frwy.dts
--
2.7.4
^ permalink raw reply
* [PATCH RFC v2] PM / devfreq: Add cpu based scaling support to passive_governor
From: Sibi Sankar @ 2019-07-16 13:36 UTC (permalink / raw)
To: saravanak, myungjoo.ham, cw00.choi, kyungmin.park
Cc: viresh.kumar, nm, sboyd, georgi.djakov, agross, david.brown,
robh+dt, mark.rutland, rjw, linux-arm-msm, devicetree,
linux-kernel, linux-pm, hsinyi, adharmap, Saravana Kannan,
Sibi Sankar
From: Saravana Kannan <skannan@codeaurora.org>
Many CPU architectures have caches that can scale independent of the
CPUs. Frequency scaling of the caches is necessary to make sure that the
cache is not a performance bottleneck that leads to poor performance and
power. The same idea applies for RAM/DDR.
To achieve this, this patch adds support for cpu based scaling to the
passive governor. This is accomplished by taking the current frequency
of each CPU frequency domain and then adjust the frequency of the cache
(or any devfreq device) based on the frequency of the CPUs. It listens
to CPU frequency transition notifiers to keep itself up to date on the
current CPU frequency.
To decide the frequency of the device, the governor does one of the
following:
* Derives the optimal devfreq device opp from required-opps property of
the parent cpu opp_table.
* Scales the device frequency in proportion to the CPU frequency. So, if
the CPUs are running at their max frequency, the device runs at its
max frequency. If the CPUs are running at their min frequency, the
device runs at its min frequency. It is interpolated for frequencies
in between.
Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
[Sibi: Integrated cpu-freqmap governor into passive_governor]
Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
v2:
* Address Chanwoo's review comments
This patch is a re-work of:
https://patchwork.kernel.org/patch/10553171/
This patch depends on Saravana's add required-opps support series:
https://patchwork.kernel.org/cover/11016423/
drivers/devfreq/Kconfig | 2 +
drivers/devfreq/governor_passive.c | 275 +++++++++++++++++++++++++++--
include/linux/devfreq.h | 40 ++++-
3 files changed, 296 insertions(+), 21 deletions(-)
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index f3b242987fd91..5db8633f94d83 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -73,6 +73,8 @@ config DEVFREQ_GOV_PASSIVE
device. This governor does not change the frequency by itself
through sysfs entries. The passive governor recommends that
devfreq device uses the OPP table to get the frequency/voltage.
+ Alternatively the governor can also be chosen to scale based on
+ the online CPUs current frequency.
comment "DEVFREQ Drivers"
diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 24ce94c80f06e..743b6093cc732 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -8,11 +8,86 @@
*/
#include <linux/module.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/cpumask.h>
#include <linux/device.h>
#include <linux/devfreq.h>
+#include <linux/slab.h>
#include "governor.h"
-static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
+static unsigned int xlate_cpufreq_to_devfreq(struct devfreq_passive_data *data,
+ unsigned int cpu)
+{
+ unsigned int cpu_min, cpu_max, dev_min, dev_max, cpu_percent, max_state;
+ struct devfreq_cpu_state *cpu_state = data->cpu_state[cpu];
+ struct devfreq *devfreq = (struct devfreq *)data->this;
+ unsigned long *freq_table = devfreq->profile->freq_table;
+ struct dev_pm_opp *opp = NULL, *cpu_opp = NULL;
+ unsigned long cpu_freq, freq;
+
+ if (!cpu_state || cpu_state->first_cpu != cpu ||
+ !cpu_state->opp_table || !devfreq->opp_table)
+ return 0;
+
+ cpu_freq = cpu_state->freq * 1000;
+ cpu_opp = devfreq_recommended_opp(cpu_state->dev, &cpu_freq, 0);
+ if (IS_ERR(cpu_opp))
+ return 0;
+
+ opp = dev_pm_opp_xlate_opp(cpu_state->opp_table,
+ devfreq->opp_table, cpu_opp);
+ dev_pm_opp_put(cpu_opp);
+
+ if (!IS_ERR(opp)) {
+ freq = dev_pm_opp_get_freq(opp);
+ dev_pm_opp_put(opp);
+ } else {
+ /* Use Interpolation if required opps is not available */
+ cpu_min = cpu_state->min_freq;
+ cpu_max = cpu_state->max_freq;
+ cpu_freq = cpu_state->freq;
+
+ if (freq_table) {
+ /* Get minimum frequency according to sorting order */
+ max_state = freq_table[devfreq->profile->max_state - 1];
+ if (freq_table[0] < max_state) {
+ dev_min = freq_table[0];
+ dev_max = max_state;
+ } else {
+ dev_min = max_state;
+ dev_max = freq_table[0];
+ }
+ } else {
+ if (devfreq->max_freq <= devfreq->min_freq)
+ return 0;
+ dev_min = devfreq->min_freq;
+ dev_max = devfreq->max_freq;
+ }
+ cpu_percent = ((cpu_freq - cpu_min) * 100) / cpu_max - cpu_min;
+ freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
+ }
+
+ return freq;
+}
+
+static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
+ unsigned long *freq)
+{
+ struct devfreq_passive_data *p_data =
+ (struct devfreq_passive_data *)devfreq->data;
+ unsigned int cpu, target_freq = 0;
+
+ for_each_online_cpu(cpu)
+ target_freq = max(target_freq,
+ xlate_cpufreq_to_devfreq(p_data, cpu));
+
+ *freq = target_freq;
+
+ return 0;
+}
+
+static int get_target_freq_with_devfreq(struct devfreq *devfreq,
unsigned long *freq)
{
struct devfreq_passive_data *p_data
@@ -22,16 +97,6 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
struct dev_pm_opp *opp = NULL, *p_opp = NULL;
int i, count, ret = 0;
- /*
- * If the devfreq device with passive governor has the specific method
- * to determine the next frequency, should use the get_target_freq()
- * of struct devfreq_passive_data.
- */
- if (p_data->get_target_freq) {
- ret = p_data->get_target_freq(devfreq, freq);
- goto out;
- }
-
/*
* If the parent and passive devfreq device uses the OPP table,
* get the next frequency by using the OPP table.
@@ -102,6 +167,37 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
return ret;
}
+static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
+ unsigned long *freq)
+{
+ struct devfreq_passive_data *p_data =
+ (struct devfreq_passive_data *)devfreq->data;
+ int ret;
+
+ /*
+ * If the devfreq device with passive governor has the specific method
+ * to determine the next frequency, should use the get_target_freq()
+ * of struct devfreq_passive_data.
+ */
+ if (p_data->get_target_freq)
+ return p_data->get_target_freq(devfreq, freq);
+
+ switch (p_data->parent_type) {
+ case DEVFREQ_PARENT_DEV:
+ ret = get_target_freq_with_devfreq(devfreq, freq);
+ break;
+ case CPUFREQ_PARENT_DEV:
+ ret = get_target_freq_with_cpufreq(devfreq, freq);
+ break;
+ default:
+ ret = -EINVAL;
+ dev_err(&devfreq->dev, "Invalid parent type\n");
+ break;
+ }
+
+ return ret;
+}
+
static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
{
int ret;
@@ -156,6 +252,140 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb,
return NOTIFY_DONE;
}
+static int cpufreq_passive_notifier_call(struct notifier_block *nb,
+ unsigned long event, void *ptr)
+{
+ struct devfreq_passive_data *data =
+ container_of(nb, struct devfreq_passive_data, nb);
+ struct devfreq *devfreq = (struct devfreq *)data->this;
+ struct devfreq_cpu_state *cpu_state;
+ struct cpufreq_freqs *freq = ptr;
+ unsigned int current_freq;
+ int ret;
+
+ if (event != CPUFREQ_POSTCHANGE || !freq ||
+ !data->cpu_state[freq->policy->cpu])
+ return 0;
+
+ cpu_state = data->cpu_state[freq->policy->cpu];
+ if (cpu_state->freq == freq->new)
+ return 0;
+
+ /* Backup current freq and pre-update cpu state freq*/
+ current_freq = cpu_state->freq;
+ cpu_state->freq = freq->new;
+
+ mutex_lock(&devfreq->lock);
+ ret = update_devfreq(devfreq);
+ mutex_unlock(&devfreq->lock);
+ if (ret) {
+ cpu_state->freq = current_freq;
+ dev_err(&devfreq->dev, "Couldn't update the frequency.\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int cpufreq_passive_register(struct devfreq_passive_data **p_data)
+{
+ struct devfreq_passive_data *data = *p_data;
+ struct devfreq *devfreq = (struct devfreq *)data->this;
+ struct device *dev = devfreq->dev.parent;
+ struct opp_table *opp_table = NULL;
+ struct devfreq_cpu_state *state;
+ struct cpufreq_policy *policy;
+ struct device *cpu_dev;
+ unsigned int cpu;
+ int ret;
+
+ get_online_cpus();
+ data->nb.notifier_call = cpufreq_passive_notifier_call;
+ ret = cpufreq_register_notifier(&data->nb,
+ CPUFREQ_TRANSITION_NOTIFIER);
+ if (ret) {
+ dev_err(dev, "Couldn't register cpufreq notifier.\n");
+ data->nb.notifier_call = NULL;
+ goto out;
+ }
+
+ /* Populate devfreq_cpu_state */
+ for_each_online_cpu(cpu) {
+ if (data->cpu_state[cpu])
+ continue;
+
+ policy = cpufreq_cpu_get(cpu);
+ if (policy) {
+ state = kzalloc(sizeof(*state), GFP_KERNEL);
+ if (!state) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ cpu_dev = get_cpu_device(cpu);
+ if (!cpu_dev) {
+ dev_err(dev, "Couldn't get cpu device.\n");
+ ret = -ENODEV;
+ goto out;
+ }
+
+ opp_table = dev_pm_opp_get_opp_table(cpu_dev);
+ if (IS_ERR(devfreq->opp_table)) {
+ ret = PTR_ERR(opp_table);
+ goto out;
+ }
+
+ state->dev = cpu_dev;
+ state->opp_table = opp_table;
+ state->first_cpu = cpumask_first(policy->related_cpus);
+ state->freq = policy->cur;
+ state->min_freq = policy->cpuinfo.min_freq;
+ state->max_freq = policy->cpuinfo.max_freq;
+ data->cpu_state[cpu] = state;
+ cpufreq_cpu_put(policy);
+ } else {
+ ret = -EPROBE_DEFER;
+ goto out;
+ }
+ }
+out:
+ put_online_cpus();
+ if (ret)
+ return ret;
+
+ /* Update devfreq */
+ mutex_lock(&devfreq->lock);
+ ret = update_devfreq(devfreq);
+ mutex_unlock(&devfreq->lock);
+ if (ret)
+ dev_err(dev, "Couldn't update the frequency.\n");
+
+ return ret;
+}
+
+static int cpufreq_passive_unregister(struct devfreq_passive_data **p_data)
+{
+ struct devfreq_passive_data *data = *p_data;
+ struct devfreq_cpu_state *cpu_state;
+ int cpu;
+
+ if (data->nb.notifier_call)
+ cpufreq_unregister_notifier(&data->nb,
+ CPUFREQ_TRANSITION_NOTIFIER);
+
+ for_each_possible_cpu(cpu) {
+ cpu_state = data->cpu_state[cpu];
+ if (cpu_state) {
+ if (cpu_state->opp_table)
+ dev_pm_opp_put_opp_table(cpu_state->opp_table);
+ kfree(cpu_state);
+ cpu_state = NULL;
+ }
+ }
+
+ return 0;
+}
+
static int devfreq_passive_event_handler(struct devfreq *devfreq,
unsigned int event, void *data)
{
@@ -166,7 +396,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
struct notifier_block *nb = &p_data->nb;
int ret = 0;
- if (!parent)
+ if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
return -EPROBE_DEFER;
switch (event) {
@@ -174,13 +404,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
if (!p_data->this)
p_data->this = devfreq;
- nb->notifier_call = devfreq_passive_notifier_call;
- ret = devm_devfreq_register_notifier(dev, parent, nb,
- DEVFREQ_TRANSITION_NOTIFIER);
+ if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
+ nb->notifier_call = devfreq_passive_notifier_call;
+ ret = devm_devfreq_register_notifier(dev, parent, nb,
+ DEVFREQ_TRANSITION_NOTIFIER);
+ } else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
+ ret = cpufreq_passive_register(&p_data);
+ } else {
+ ret = -EINVAL;
+ }
break;
case DEVFREQ_GOV_STOP:
- devm_devfreq_unregister_notifier(dev, parent, nb,
- DEVFREQ_TRANSITION_NOTIFIER);
+ if (p_data->parent_type == DEVFREQ_PARENT_DEV)
+ devm_devfreq_unregister_notifier(dev, parent, nb,
+ DEVFREQ_TRANSITION_NOTIFIER);
+ else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
+ cpufreq_passive_unregister(&p_data);
+ else
+ ret = -EINVAL;
break;
default:
break;
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 69d487329408e..f3f898dfd09b2 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -263,6 +263,32 @@ struct devfreq_simple_ondemand_data {
#endif
#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
+/**
+ * struct devfreq_cpu_state - holds the per-cpu state
+ * @freq: the current frequency of the cpu.
+ * @min_freq: the min frequency of the cpu.
+ * @max_freq: the max frequency of the cpu.
+ * @first_cpu: the cpumask of the first cpu of a policy.
+ * @dev: reference to cpu device.
+ * @opp_table: reference to cpu opp table.
+ *
+ * This structure stores the required cpu_state of a cpu.
+ * This is auto-populated by the governor.
+ */
+struct devfreq_cpu_state {
+ unsigned int freq;
+ unsigned int min_freq;
+ unsigned int max_freq;
+ unsigned int first_cpu;
+ struct device *dev;
+ struct opp_table *opp_table;
+};
+
+enum devfreq_parent_dev_type {
+ DEVFREQ_PARENT_DEV,
+ CPUFREQ_PARENT_DEV,
+};
+
/**
* struct devfreq_passive_data - void *data fed to struct devfreq
* and devfreq_add_device
@@ -274,13 +300,15 @@ struct devfreq_simple_ondemand_data {
* using governors except for passive governor.
* If the devfreq device has the specific method to decide
* the next frequency, should use this callback.
- * @this: the devfreq instance of own device.
- * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
+ * @parent_type parent type of the device
+ * @this: the devfreq instance of own device.
+ * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
+ * @cpu_state: the state min/max/current frequency of all online cpu's
*
* The devfreq_passive_data have to set the devfreq instance of parent
* device with governors except for the passive governor. But, don't need to
- * initialize the 'this' and 'nb' field because the devfreq core will handle
- * them.
+ * initialize the 'this', 'nb' and 'cpu_state' field because the devfreq core
+ * will handle them.
*/
struct devfreq_passive_data {
/* Should set the devfreq instance of parent device */
@@ -289,9 +317,13 @@ struct devfreq_passive_data {
/* Optional callback to decide the next frequency of passvice device */
int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
+ /* Should set the type of parent device */
+ enum devfreq_parent_dev_type parent_type;
+
/* For passive governor's internal use. Don't need to set them */
struct devfreq *this;
struct notifier_block nb;
+ struct devfreq_cpu_state *cpu_state[NR_CPUS];
};
#endif
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* RE: [PATCH 3/3] arm64: defconfig: Build imx8 ddr pmu as module
From: Frank Li @ 2019-07-16 13:35 UTC (permalink / raw)
To: Leonard Crestez, Shawn Guo, Will Deacon
Cc: Mark Rutland, Aisheng Dong, Jacky Bai, devicetree@vger.kernel.org,
Andrey Smirnov, Rob Herring, dl-linux-imx, kernel@pengutronix.de,
Fabio Estevam, linux-arm-kernel@lists.infradead.org
In-Reply-To: <e51a2f95044f0a9003c3be2e82c3c4b2653670a7.1562230183.git.leonard.crestez@nxp.com>
> -----Original Message-----
> From: Leonard Crestez <leonard.crestez@nxp.com>
> Sent: Thursday, July 4, 2019 3:53 AM
> To: Shawn Guo <shawnguo@kernel.org>; Will Deacon <will@kernel.org>; Frank
> Li <frank.li@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Andrey Smirnov
> <andrew.smirnov@gmail.com>; Mark Rutland <mark.rutland@arm.com>;
> Aisheng Dong <aisheng.dong@nxp.com>; Fabio Estevam
> <fabio.estevam@nxp.com>; Jacky Bai <ping.bai@nxp.com>;
> devicetree@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx <linux-
> imx@nxp.com>; linux-arm-kernel@lists.infradead.org
> Subject: [PATCH 3/3] arm64: defconfig: Build imx8 ddr pmu as module
>
> This is available on all imx8 but is not "boot critical" in any way so build as a
> module.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Acked-by: Frank Li <frank.li@nxp.com>
> ---
> arch/arm64/configs/defconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 812fe887477d..134a4067846f 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -766,10 +766,11 @@ CONFIG_PHY_ROCKCHIP_INNO_USB2=y
> CONFIG_PHY_ROCKCHIP_PCIE=m CONFIG_PHY_ROCKCHIP_TYPEC=y
> CONFIG_PHY_UNIPHIER_USB2=y CONFIG_PHY_UNIPHIER_USB3=y
> CONFIG_PHY_TEGRA_XUSB=y
> +CONFIG_FSL_IMX8_DDR_PMU=m
> CONFIG_HISI_PMU=y
> CONFIG_QCOM_L2_PMU=y
> CONFIG_QCOM_L3_PMU=y
> CONFIG_NVMEM_SUNXI_SID=y
> CONFIG_NVMEM_IMX_OCOTP=y
> --
> 2.17.1
^ permalink raw reply
* RE: [PATCH 2/3] arm64: dts: imx8m: Add ddr-pmu nodes
From: Frank Li @ 2019-07-16 13:35 UTC (permalink / raw)
To: Leonard Crestez, Shawn Guo, Will Deacon
Cc: Mark Rutland, Aisheng Dong, Jacky Bai, devicetree@vger.kernel.org,
Andrey Smirnov, Rob Herring, dl-linux-imx, kernel@pengutronix.de,
Fabio Estevam, linux-arm-kernel@lists.infradead.org
In-Reply-To: <b8aeb9caf6c2380a7c8f65b19e420e18498d5c1a.1562230183.git.leonard.crestez@nxp.com>
> -----Original Message-----
> From: Leonard Crestez <leonard.crestez@nxp.com>
> Sent: Thursday, July 4, 2019 3:53 AM
> To: Shawn Guo <shawnguo@kernel.org>; Will Deacon <will@kernel.org>; Frank
> Li <frank.li@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Andrey Smirnov
> <andrew.smirnov@gmail.com>; Mark Rutland <mark.rutland@arm.com>;
> Aisheng Dong <aisheng.dong@nxp.com>; Fabio Estevam
> <fabio.estevam@nxp.com>; Jacky Bai <ping.bai@nxp.com>;
> devicetree@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx <linux-
> imx@nxp.com>; linux-arm-kernel@lists.infradead.org
> Subject: [PATCH 2/3] arm64: dts: imx8m: Add ddr-pmu nodes
>
> The same ddr perfomance counter IP from 8qxp is also available on imx8m
> series so add it to dts.
>
> Tested with `perf stat` and `memtester` on imx8mm-evk and obtained plausible
> results.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Acked-by: Frank Li <frank.li@nxp.com>
> ---
> arch/arm64/boot/dts/freescale/imx8mm.dtsi | 7 +++++++
> arch/arm64/boot/dts/freescale/imx8mq.dtsi | 7 +++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index 232a7412755a..9a8d8cd21ef0 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -816,7 +816,14 @@
> <0x38880000 0xc0000>; /* GICR (RD_base +
> SGI_base) */
> #interrupt-cells = <3>;
> interrupt-controller;
> interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> };
> +
> + ddr-pmu@3d800000 {
> + compatible = "fsl,imx8mm-ddr-pmu", "fsl,imx8m-ddr-
> pmu";
> + reg = <0x3d800000 0x400000>;
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
> + };
> };
> };
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index d09b808eff87..fb0fdee5f889 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -1031,7 +1031,14 @@
> #interrupt-cells = <3>;
> interrupt-controller;
> interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> interrupt-parent = <&gic>;
> };
> +
> + ddr-pmu@3d800000 {
> + compatible = "fsl,imx8mq-ddr-pmu", "fsl,imx8m-ddr-
> pmu";
> + reg = <0x3d800000 0x400000>;
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
> + };
> };
> };
> --
> 2.17.1
^ permalink raw reply
* RE: [PATCH 1/3] perf/imx_ddr: Add MODULE_DEVICE_TABLE
From: Frank Li @ 2019-07-16 13:34 UTC (permalink / raw)
To: Leonard Crestez, Shawn Guo, Will Deacon
Cc: Mark Rutland, Aisheng Dong, Jacky Bai, devicetree@vger.kernel.org,
Andrey Smirnov, Rob Herring, dl-linux-imx, kernel@pengutronix.de,
Fabio Estevam, linux-arm-kernel@lists.infradead.org
In-Reply-To: <6e88abe2ed5f361469cbb8ccccd4586036fc2fa2.1562230183.git.leonard.crestez@nxp.com>
> -----Original Message-----
> From: Leonard Crestez <leonard.crestez@nxp.com>
> Sent: Thursday, July 4, 2019 3:53 AM
> To: Shawn Guo <shawnguo@kernel.org>; Will Deacon <will@kernel.org>; Frank
> Li <frank.li@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Andrey Smirnov
> <andrew.smirnov@gmail.com>; Mark Rutland <mark.rutland@arm.com>;
> Aisheng Dong <aisheng.dong@nxp.com>; Fabio Estevam
> <fabio.estevam@nxp.com>; Jacky Bai <ping.bai@nxp.com>;
> devicetree@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx <linux-
> imx@nxp.com>; linux-arm-kernel@lists.infradead.org
> Subject: [PATCH 1/3] perf/imx_ddr: Add MODULE_DEVICE_TABLE
>
> This is required for automatic probing when driver is built as a module.
>
> Fixes: 9a66d36cc7ac ("drivers/perf: imx_ddr: Add DDR performance counter
> support to perf")
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Acked-by: Frank Li <frank.li@nxp.com>
> ---
> drivers/perf/fsl_imx8_ddr_perf.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> index 63fe21600072..0e3310dbb145 100644
> --- a/drivers/perf/fsl_imx8_ddr_perf.c
> +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> @@ -45,10 +45,11 @@ static DEFINE_IDA(ddr_ida); static const struct
> of_device_id imx_ddr_pmu_dt_ids[] = {
> { .compatible = "fsl,imx8-ddr-pmu",},
> { .compatible = "fsl,imx8m-ddr-pmu",},
> { /* sentinel */ }
> };
> +MODULE_DEVICE_TABLE(of, imx_ddr_pmu_dt_ids);
>
> struct ddr_pmu {
> struct pmu pmu;
> void __iomem *base;
> unsigned int cpu;
> --
> 2.17.1
^ permalink raw reply
* [PATCH 03/14] docs: fix broken doc references due to renames
From: Mauro Carvalho Chehab @ 2019-07-16 12:10 UTC (permalink / raw)
Cc: Mark Rutland, Andrea Parri, Ajay Gupta, linux-doc, Peter Zijlstra,
Akira Yokosawa, Lai Jiangshan, Jerry Hoemann, dri-devel,
David Howells, Mauro Carvalho Chehab, linux-i2c, Joel Fernandes,
Paul E. McKenney, Will Deacon, linux-arch, linux-scsi,
Jonathan Corbet, esc.storagedev, Maxime Ripard, Ingo Molnar,
Alan Stern, Jade Alglave, Boqun Feng, Guenter
In-Reply-To: <cover.1563277838.git.mchehab+samsung@kernel.org>
Some files got renamed but probably due to some merge conflicts,
a few references still point to the old locations.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
Documentation/RCU/rculist_nulls.txt | 2 +-
Documentation/devicetree/bindings/arm/idle-states.txt | 2 +-
Documentation/locking/spinlocks.rst | 4 ++--
Documentation/memory-barriers.txt | 2 +-
Documentation/translations/ko_KR/memory-barriers.txt | 2 +-
Documentation/watchdog/hpwdt.rst | 2 +-
MAINTAINERS | 8 ++++----
drivers/gpu/drm/drm_modes.c | 2 +-
drivers/i2c/busses/i2c-nvidia-gpu.c | 2 +-
drivers/scsi/hpsa.c | 4 ++--
10 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/Documentation/RCU/rculist_nulls.txt b/Documentation/RCU/rculist_nulls.txt
index 8151f0195f76..23f115dc87cf 100644
--- a/Documentation/RCU/rculist_nulls.txt
+++ b/Documentation/RCU/rculist_nulls.txt
@@ -1,7 +1,7 @@
Using hlist_nulls to protect read-mostly linked lists and
objects using SLAB_TYPESAFE_BY_RCU allocations.
-Please read the basics in Documentation/RCU/listRCU.txt
+Please read the basics in Documentation/RCU/listRCU.rst
Using special makers (called 'nulls') is a convenient way
to solve following problem :
diff --git a/Documentation/devicetree/bindings/arm/idle-states.txt b/Documentation/devicetree/bindings/arm/idle-states.txt
index 326f29b270ad..2d325bed37e5 100644
--- a/Documentation/devicetree/bindings/arm/idle-states.txt
+++ b/Documentation/devicetree/bindings/arm/idle-states.txt
@@ -703,4 +703,4 @@ cpus {
https://www.devicetree.org/specifications/
[6] ARM Linux Kernel documentation - Booting AArch64 Linux
- Documentation/arm64/booting.txt
+ Documentation/arm64/booting.rst
diff --git a/Documentation/locking/spinlocks.rst b/Documentation/locking/spinlocks.rst
index 098107fb7d86..e93ec6645238 100644
--- a/Documentation/locking/spinlocks.rst
+++ b/Documentation/locking/spinlocks.rst
@@ -82,7 +82,7 @@ itself. The read lock allows many concurrent readers. Anything that
**changes** the list will have to get the write lock.
NOTE! RCU is better for list traversal, but requires careful
- attention to design detail (see Documentation/RCU/listRCU.txt).
+ attention to design detail (see Documentation/RCU/listRCU.rst).
Also, you cannot "upgrade" a read-lock to a write-lock, so if you at _any_
time need to do any changes (even if you don't do it every time), you have
@@ -90,7 +90,7 @@ to get the write-lock at the very beginning.
NOTE! We are working hard to remove reader-writer spinlocks in most
cases, so please don't add a new one without consensus. (Instead, see
- Documentation/RCU/rcu.txt for complete information.)
+ Documentation/RCU/rcu.rst for complete information.)
----
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 045bb8148fe9..1adbb8a371c7 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -548,7 +548,7 @@ There are certain things that the Linux kernel memory barriers do not guarantee:
[*] For information on bus mastering DMA and coherency please read:
- Documentation/PCI/pci.rst
+ Documentation/driver-api/pci/pci.rst
Documentation/DMA-API-HOWTO.txt
Documentation/DMA-API.txt
diff --git a/Documentation/translations/ko_KR/memory-barriers.txt b/Documentation/translations/ko_KR/memory-barriers.txt
index a33c2a536542..2774624ee843 100644
--- a/Documentation/translations/ko_KR/memory-barriers.txt
+++ b/Documentation/translations/ko_KR/memory-barriers.txt
@@ -569,7 +569,7 @@ ACQUIRE 는 해당 오퍼레이션의 로드 부분에만 적용되고 RELEASE
[*] 버스 마스터링 DMA 와 일관성에 대해서는 다음을 참고하시기 바랍니다:
- Documentation/PCI/pci.rst
+ Documentation/driver-api/pci/pci.rst
Documentation/DMA-API-HOWTO.txt
Documentation/DMA-API.txt
diff --git a/Documentation/watchdog/hpwdt.rst b/Documentation/watchdog/hpwdt.rst
index 94a96371113e..49c647dba8aa 100644
--- a/Documentation/watchdog/hpwdt.rst
+++ b/Documentation/watchdog/hpwdt.rst
@@ -59,7 +59,7 @@ Last reviewed: 08/20/2018
and loop forever. This is generally not what a watchdog user wants.
For those wishing to learn more please see:
- Documentation/kdump/kdump.rst
+ Documentation/admin-guide/kdump/kdump.rst
Documentation/admin-guide/kernel-parameters.txt (panic=)
Your Linux Distribution specific documentation.
diff --git a/MAINTAINERS b/MAINTAINERS
index b3a5c72f3298..b0acc138e9e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -899,7 +899,7 @@ L: linux-iio@vger.kernel.org
W: http://ez.analog.com/community/linux-device-drivers
S: Supported
F: drivers/iio/adc/ad7124.c
-F: Documentation/devicetree/bindings/iio/adc/adi,ad7124.txt
+F: Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
ANALOG DEVICES INC AD7606 DRIVER
M: Stefan Popa <stefan.popa@analog.com>
@@ -4189,7 +4189,7 @@ M: Jens Axboe <axboe@kernel.dk>
L: cgroups@vger.kernel.org
L: linux-block@vger.kernel.org
T: git git://git.kernel.dk/linux-block
-F: Documentation/cgroup-v1/blkio-controller.rst
+F: Documentation/admin-guide/cgroup-v1/blkio-controller.rst
F: block/blk-cgroup.c
F: include/linux/blk-cgroup.h
F: block/blk-throttle.c
@@ -6848,7 +6848,7 @@ R: Sagi Shahar <sagis@google.com>
R: Jon Olson <jonolson@google.com>
L: netdev@vger.kernel.org
S: Supported
-F: Documentation/networking/device_drivers/google/gve.txt
+F: Documentation/networking/device_drivers/google/gve.rst
F: drivers/net/ethernet/google
GPD POCKET FAN DRIVER
@@ -12096,7 +12096,7 @@ M: Juergen Gross <jgross@suse.com>
M: Alok Kataria <akataria@vmware.com>
L: virtualization@lists.linux-foundation.org
S: Supported
-F: Documentation/virtual/paravirt_ops.txt
+F: Documentation/virtual/paravirt_ops.rst
F: arch/*/kernel/paravirt*
F: arch/*/include/asm/paravirt*.h
F: include/linux/hypervisor.h
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 57e6408288c8..4645af681ef8 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1680,7 +1680,7 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
*
* Additionals options can be provided following the mode, using a comma to
* separate each option. Valid options can be found in
- * Documentation/fb/modedb.txt.
+ * Documentation/fb/modedb.rst.
*
* The intermediate drm_cmdline_mode structure is required to store additional
* options from the command line modline like the force-enable/disable flag.
diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
index cfc76b5de726..5a1235fd86bb 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -364,7 +364,7 @@ static void gpu_i2c_remove(struct pci_dev *pdev)
/*
* We need gpu_i2c_suspend() even if it is stub, for runtime pm to work
* correctly. Without it, lspci shows runtime pm status as "D0" for the card.
- * Documentation/power/pci.txt also insists for driver to provide this.
+ * Documentation/power/pci.rst also insists for driver to provide this.
*/
static __maybe_unused int gpu_i2c_suspend(struct device *dev)
{
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 43a6b5350775..eaf6177ac9ee 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -7798,7 +7798,7 @@ static void hpsa_free_pci_init(struct ctlr_info *h)
hpsa_disable_interrupt_mode(h); /* pci_init 2 */
/*
* call pci_disable_device before pci_release_regions per
- * Documentation/PCI/pci.rst
+ * Documentation/driver-api/pci/pci.rst
*/
pci_disable_device(h->pdev); /* pci_init 1 */
pci_release_regions(h->pdev); /* pci_init 2 */
@@ -7881,7 +7881,7 @@ static int hpsa_pci_init(struct ctlr_info *h)
clean1:
/*
* call pci_disable_device before pci_release_regions per
- * Documentation/PCI/pci.rst
+ * Documentation/driver-api/pci/pci.rst
*/
pci_disable_device(h->pdev);
pci_release_regions(h->pdev);
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH 00/14] pending doc patches for 5.3-rc
From: Mauro Carvalho Chehab @ 2019-07-16 12:10 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, linux-scsi, esc.storagedev, linuxppc-dev,
Jonathan Corbet, alsa-devel, kvm, linux-i2c, rcu, linux-pm,
linux-doc, devicetree, linux-arch, linux-arm-kernel,
linux-watchdog, x86, dri-devel, netdev, linux-crypto, linux-sh,
linux-input, linux-pci
Those are the pending documentation patches after my pull request
for this branch:
git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media.git tags/docs/v5.3-1
Patches 1 to 13 were already submitted, but got rebased. Patch 14
is a new fixup one.
Patches 1 and 2 weren't submitted before due to merge conflicts
that are now solved upstream;
Patch 3 fixes a series of random Documentation/* references that
are pointing to the wrong places.
Patch 4 fix a longstanding issue: every time a new book is added,
conf.py need changes, in order to allow generating a PDF file.
After the patch, conf.py will automatically recognize new books,
saving the trouble of keeping adding documents to it.
Patches 5 to 11 are due to fonts support when building translations.pdf.
The main focus is to add xeCJK support. While doing it, I discovered
some bugs at sphinx-pre-install script after running it with 7 different
distributions.
Patch 12 improves support for partial doc building. Currently, each
subdir needs to have its own conf.py, in order to support partial
doc build. After it, any Documentation subdir can be used to
roduce html/pdf docs with:
make SPHINXDIRS="foo bar" htmldocs
(or pdfdocs, latexdocs, epubdocs, ...)
Patch 13 is a cleanup patch: it simply get rid of all those extra
conf.py files that aren't needed anymore. The only extra config
file after it is this one:
Documentation/media/conf_nitpick.py
With enables some extra optional Sphinx features.
Patch 14 adds Documentation/virtual to the main index.rst file
and add a new *.rst file that was orphaned there.
-
After this series, there's just one more patch meant to be applied
for 5.3, with is still waiting for some patches to be merged from
linux-next:
https://git.linuxtv.org/mchehab/experimental.git/commit/?id=b1b5dc7d7bbfbbfdace2a248c6458301c6e34100
Mauro Carvalho Chehab (14):
docs: powerpc: convert docs to ReST and rename to *.rst
docs: power: add it to to the main documentation index
docs: fix broken doc references due to renames
docs: pdf: add all Documentation/*/index.rst to PDF output
docs: conf.py: add CJK package needed by translations
docs: conf.py: only use CJK if the font is available
scripts/sphinx-pre-install: fix script for RHEL/CentOS
scripts/sphinx-pre-install: don't use LaTeX with CentOS 7
scripts/sphinx-pre-install: fix latexmk dependencies
scripts/sphinx-pre-install: cleanup Gentoo checks
scripts/sphinx-pre-install: seek for Noto CJK fonts for pdf output
docs: load_config.py: avoid needing a conf.py just due to LaTeX docs
docs: remove extra conf.py files
docs: virtual: add it to the documentation body
Documentation/PCI/pci-error-recovery.rst | 5 +-
Documentation/RCU/rculist_nulls.txt | 2 +-
Documentation/admin-guide/conf.py | 10 --
Documentation/conf.py | 30 +++-
Documentation/core-api/conf.py | 10 --
Documentation/crypto/conf.py | 10 --
Documentation/dev-tools/conf.py | 10 --
.../devicetree/bindings/arm/idle-states.txt | 2 +-
Documentation/doc-guide/conf.py | 10 --
Documentation/driver-api/80211/conf.py | 10 --
Documentation/driver-api/conf.py | 10 --
Documentation/driver-api/pm/conf.py | 10 --
Documentation/filesystems/conf.py | 10 --
Documentation/gpu/conf.py | 10 --
Documentation/index.rst | 3 +
Documentation/input/conf.py | 10 --
Documentation/kernel-hacking/conf.py | 10 --
Documentation/locking/spinlocks.rst | 4 +-
Documentation/maintainer/conf.py | 10 --
Documentation/media/conf.py | 12 --
Documentation/memory-barriers.txt | 2 +-
Documentation/networking/conf.py | 10 --
Documentation/power/index.rst | 2 +-
.../{bootwrapper.txt => bootwrapper.rst} | 28 +++-
.../{cpu_families.txt => cpu_families.rst} | 23 +--
.../{cpu_features.txt => cpu_features.rst} | 6 +-
Documentation/powerpc/{cxl.txt => cxl.rst} | 46 ++++--
.../powerpc/{cxlflash.txt => cxlflash.rst} | 10 +-
.../{DAWR-POWER9.txt => dawr-power9.rst} | 15 +-
Documentation/powerpc/{dscr.txt => dscr.rst} | 18 +-
...ecovery.txt => eeh-pci-error-recovery.rst} | 108 ++++++------
...ed-dump.txt => firmware-assisted-dump.rst} | 117 +++++++------
Documentation/powerpc/{hvcs.txt => hvcs.rst} | 108 ++++++------
Documentation/powerpc/index.rst | 34 ++++
Documentation/powerpc/isa-versions.rst | 15 +-
.../powerpc/{mpc52xx.txt => mpc52xx.rst} | 12 +-
...nv.txt => pci_iov_resource_on_powernv.rst} | 15 +-
.../powerpc/{pmu-ebb.txt => pmu-ebb.rst} | 1 +
Documentation/powerpc/ptrace.rst | 156 ++++++++++++++++++
Documentation/powerpc/ptrace.txt | 151 -----------------
.../{qe_firmware.txt => qe_firmware.rst} | 37 +++--
.../{syscall64-abi.txt => syscall64-abi.rst} | 29 ++--
...al_memory.txt => transactional_memory.rst} | 45 ++---
Documentation/process/conf.py | 10 --
Documentation/sh/conf.py | 10 --
Documentation/sound/conf.py | 10 --
Documentation/sphinx/load_config.py | 27 ++-
.../translations/ko_KR/memory-barriers.txt | 2 +-
Documentation/userspace-api/conf.py | 10 --
Documentation/virtual/kvm/index.rst | 1 +
Documentation/vm/conf.py | 10 --
Documentation/watchdog/hpwdt.rst | 2 +-
Documentation/x86/conf.py | 10 --
MAINTAINERS | 14 +-
arch/powerpc/kernel/exceptions-64s.S | 2 +-
drivers/gpu/drm/drm_modes.c | 2 +-
drivers/i2c/busses/i2c-nvidia-gpu.c | 2 +-
drivers/scsi/hpsa.c | 4 +-
drivers/soc/fsl/qe/qe.c | 2 +-
drivers/tty/hvc/hvcs.c | 2 +-
include/soc/fsl/qe/qe.h | 2 +-
scripts/sphinx-pre-install | 118 ++++++++++---
62 files changed, 738 insertions(+), 678 deletions(-)
delete mode 100644 Documentation/admin-guide/conf.py
delete mode 100644 Documentation/core-api/conf.py
delete mode 100644 Documentation/crypto/conf.py
delete mode 100644 Documentation/dev-tools/conf.py
delete mode 100644 Documentation/doc-guide/conf.py
delete mode 100644 Documentation/driver-api/80211/conf.py
delete mode 100644 Documentation/driver-api/conf.py
delete mode 100644 Documentation/driver-api/pm/conf.py
delete mode 100644 Documentation/filesystems/conf.py
delete mode 100644 Documentation/gpu/conf.py
delete mode 100644 Documentation/input/conf.py
delete mode 100644 Documentation/kernel-hacking/conf.py
delete mode 100644 Documentation/maintainer/conf.py
delete mode 100644 Documentation/media/conf.py
delete mode 100644 Documentation/networking/conf.py
rename Documentation/powerpc/{bootwrapper.txt => bootwrapper.rst} (93%)
rename Documentation/powerpc/{cpu_families.txt => cpu_families.rst} (95%)
rename Documentation/powerpc/{cpu_features.txt => cpu_features.rst} (97%)
rename Documentation/powerpc/{cxl.txt => cxl.rst} (95%)
rename Documentation/powerpc/{cxlflash.txt => cxlflash.rst} (98%)
rename Documentation/powerpc/{DAWR-POWER9.txt => dawr-power9.rst} (95%)
rename Documentation/powerpc/{dscr.txt => dscr.rst} (91%)
rename Documentation/powerpc/{eeh-pci-error-recovery.txt => eeh-pci-error-recovery.rst} (82%)
rename Documentation/powerpc/{firmware-assisted-dump.txt => firmware-assisted-dump.rst} (80%)
rename Documentation/powerpc/{hvcs.txt => hvcs.rst} (91%)
create mode 100644 Documentation/powerpc/index.rst
rename Documentation/powerpc/{mpc52xx.txt => mpc52xx.rst} (91%)
rename Documentation/powerpc/{pci_iov_resource_on_powernv.txt => pci_iov_resource_on_powernv.rst} (97%)
rename Documentation/powerpc/{pmu-ebb.txt => pmu-ebb.rst} (99%)
create mode 100644 Documentation/powerpc/ptrace.rst
delete mode 100644 Documentation/powerpc/ptrace.txt
rename Documentation/powerpc/{qe_firmware.txt => qe_firmware.rst} (95%)
rename Documentation/powerpc/{syscall64-abi.txt => syscall64-abi.rst} (82%)
rename Documentation/powerpc/{transactional_memory.txt => transactional_memory.rst} (93%)
delete mode 100644 Documentation/process/conf.py
delete mode 100644 Documentation/sh/conf.py
delete mode 100644 Documentation/sound/conf.py
delete mode 100644 Documentation/userspace-api/conf.py
delete mode 100644 Documentation/vm/conf.py
delete mode 100644 Documentation/x86/conf.py
--
2.21.0
^ permalink raw reply
* Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Bartlomiej Zolnierkiewicz @ 2019-07-16 11:56 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Mark Rutland, Nishanth Menon, linux-samsung-soc, devicetree,
Stephen Boyd, Viresh Kumar, linux-pm, linux-kernel,
Krzysztof Kozlowski, Rob Herring, Kyungmin Park, Kukjin Kim,
MyungJoo Ham, Kamil Konieczny, linux-arm-kernel, Marek Szyprowski
In-Reply-To: <1a9e5752-bc2b-3b08-a36b-fc02ca51764c@samsung.com>
On 7/16/19 1:39 PM, Bartlomiej Zolnierkiewicz wrote:
>
> On 7/16/19 1:26 PM, Chanwoo Choi wrote:
[...]
>>> Doesn't seem to be needed, care to explain it more?
>>
>> In order to fix the sequence problem between clock and regulator
>> with dev_pm_opp_set_regualtor() and want to keep two functions
>> (exynos_bus_parent_parse_of() and exynos_bus_parse_of()),
>> have to change the call order as following and then modify
>> the exception handling code when error happen.
>>
>> node = of_parse_phandle(dev->of_node, "devfreq", 0);
>> if (node) {
>> of_node_put(node);
>> passive = true
>> }
>>
>> if (!passive)
>> exynos_bus_parent_parse_of()
>> dev_pm_opp_set_regulator
>>
>> exynos_bus_parse_of()
>
> OK. This seems like a solution.
PS Thanks for explaining this in detail.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Bartlomiej Zolnierkiewicz @ 2019-07-16 11:39 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Kamil Konieczny, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <29cfafc4-ee22-6d38-4c67-776c48bfed8a@samsung.com>
On 7/16/19 1:26 PM, Chanwoo Choi wrote:
> Hi,
>
> On 19. 7. 16. 오후 7:59, Bartlomiej Zolnierkiewicz wrote:
>>
>> On 7/16/19 12:33 PM, Chanwoo Choi wrote:
>>> Hi Bartlomiej,
>>>
>>> On 19. 7. 16. 오후 7:13, Bartlomiej Zolnierkiewicz wrote:
>>>>
>>>> Hi Chanwoo,
>>>>
>>>> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>>>>> Hi Kamil,
>>>>>
>>>>> Looks good to me. But, this patch has some issue.
>>>>> I added the detailed reviews.
>>>>>
>>>>> I recommend that you make the separate patches as following
>>>>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>>>>
>>>>> First patch,
>>>>> Need to consolidate the following two function into one function.
>>>>> because the original exynos-bus.c has the problem that the regulator
>>>>> of parent devfreq device have to be enabled before enabling the clock.
>>>>> This issue did not happen because bootloader enables the bus-related
>>>>> regulators before kernel booting.
>>>>> - exynos_bus_parse_of()
>>>>> - exynos_bus_parent_parse_of()
>>>>>> Second patch,
>>>>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>>>>
>>>>>
>>>>> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>>>>>> Reuse opp core code for setting bus clock and voltage. As a side
>>>>>> effect this allow useage of coupled regulators feature (required
>>>>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>>>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>>>>> while the old code used regulator_set_voltage_tol() with fixed
>>>>>> tolerance. This patch also removes no longer needed parsing of DT
>>>>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>>>>> it).
>>>>>>
>>>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>>>> ---
>>>>>> drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>>>>> 1 file changed, 66 insertions(+), 106 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>>>> index 486cc5b422f1..7fc4f76bd848 100644
>>>>>> --- a/drivers/devfreq/exynos-bus.c
>>>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>>>> @@ -25,7 +25,6 @@
>>>>>> #include <linux/slab.h>
>>>>>>
>>>>>> #define DEFAULT_SATURATION_RATIO 40
>>>>>> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>>>>>>
>>>>>> struct exynos_bus {
>>>>>> struct device *dev;
>>>>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>>>>
>>>>>> unsigned long curr_freq;
>>>>>>
>>>>>> - struct regulator *regulator;
>>>>>> + struct opp_table *opp_table;
>>>>>> +
>>>>>> struct clk *clk;
>>>>>> - unsigned int voltage_tolerance;
>>>>>> unsigned int ratio;
>>>>>> };
>>>>>>
>>>>>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>>>>> {
>>>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>>> struct dev_pm_opp *new_opp;
>>>>>> - unsigned long old_freq, new_freq, new_volt, tol;
>>>>>> int ret = 0;
>>>>>> -
>>>>>> - /* Get new opp-bus instance according to new bus clock */
>>>>>> + /*
>>>>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>>>>> + * *freq to correct value.
>>>>>> + */
>>>>>
>>>>> You better to change this comment with following styles
>>>>> to keep the consistency:
>>>>>
>>>>> /* Get correct frequency for bus ... */
>>>>>
>>>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>>> if (IS_ERR(new_opp)) {
>>>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>>>> return PTR_ERR(new_opp);
>>>>>> }
>>>>>>
>>>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>>>> - new_volt = dev_pm_opp_get_voltage(new_opp);
>>>>>> dev_pm_opp_put(new_opp);
>>>>>>
>>>>>> - old_freq = bus->curr_freq;
>>>>>> -
>>>>>> - if (old_freq == new_freq)
>>>>>> - return 0;
>>>>>> - tol = new_volt * bus->voltage_tolerance / 100;
>>>>>> -
>>>>>> /* Change voltage and frequency according to new OPP level */
>>>>>> mutex_lock(&bus->lock);
>>>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>>>> + if (!ret)
>>>>>> + bus->curr_freq = *freq;
>>>>>
>>>>> Have to print the error log if ret has minus error value.
>>>>
>>>> dev_pm_opp_set_rate() should print the error message on all
>>>> errors so wouldn't printing the error log also here be superfluous?
>>>>
>>>> [ Please also note that the other user of dev_pm_opp_set_rate()
>>>> (cpufreq-dt cpufreq driver) doesn't do this. ]
>>>
>>> OK. Thanks for the explanation.
>>>
>>>>
>>>>> Modify it as following:
>>>>>
>>>>> if (ret < 0) {
>>>>> dev_err(dev, "failed to set bus rate\n");
>>>>> goto err:
>>>>> }
>>>>> bus->curr_freq = *freq;
>>>>>
>>>>> err:
>>>>> mutex_unlock(&bus->lock);
>>>>>
>>>>> return ret;
>>>>>
>>>>>>
>>>>>> - if (old_freq < new_freq) {
>>>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>>> - if (ret < 0) {
>>>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>>>> - goto out;
>>>>>> - }
>>>>>> - }
>>>>>> -
>>>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>>>> - if (ret < 0) {
>>>>>> - dev_err(dev, "failed to change clock of bus\n");
>>>>>> - clk_set_rate(bus->clk, old_freq);
>>>>>> - goto out;
>>>>>> - }
>>>>>> -
>>>>>> - if (old_freq > new_freq) {
>>>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>>> - if (ret < 0) {
>>>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>>>> - goto out;
>>>>>> - }
>>>>>> - }
>>>>>> - bus->curr_freq = new_freq;
>>>>>> -
>>>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>>>> -out:
>>>>>> mutex_unlock(&bus->lock);
>>>>>>
>>>>>> return ret;
>>>>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>>>>> if (ret < 0)
>>>>>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>>>>
>>>>>> - if (bus->regulator)
>>>>>> - regulator_disable(bus->regulator);
>>>>>> + if (bus->opp_table)
>>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>>
>>>>> Have to disable regulator after disabling the clock
>>>>> to prevent the h/w fault.
>>>>>
>>>>> I think that you should call them with following sequence:
>>>>>
>>>>> clk_disable_unprepare(bus->clk);
>>>>> if (bus->opp_table)
>>>>> dev_pm_opp_put_regulators(bus->opp_table);
>>>>> dev_pm_opp_of_remove_table(dev);
>>>>>
>>>>>>
>>>>>> dev_pm_opp_of_remove_table(dev);
>>>>>> +
>>>>>> clk_disable_unprepare(bus->clk);
>>>>>> }
>>>>>>
>>>>>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>>>>> {
>>>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>>> struct dev_pm_opp *new_opp;
>>>>>> - unsigned long old_freq, new_freq;
>>>>>> - int ret = 0;
>>>>>> + int ret;
>>>>>>
>>>>>> - /* Get new opp-bus instance according to new bus clock */
>>>>>> + /*
>>>>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>>>>> + * *freq to correct value.
>>>>>> + */
>>>>>
>>>>> You better to change this comment with following styles
>>>>> to keep the consistency:
>>>>>
>>>>> /* Get correct frequency for bus ... */
>>>>>
>>>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>>> if (IS_ERR(new_opp)) {
>>>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>>>> return PTR_ERR(new_opp);
>>>>>> }
>>>>>>
>>>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>>>> dev_pm_opp_put(new_opp);
>>>>>>
>>>>>> - old_freq = bus->curr_freq;
>>>>>> -
>>>>>> - if (old_freq == new_freq)
>>>>>> - return 0;
>>>>>> -
>>>>>> /* Change the frequency according to new OPP level */
>>>>>> mutex_lock(&bus->lock);
>>>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>>>> + if (!ret)
>>>>>> + bus->curr_freq = *freq;
>>>>>
>>>>> ditto. Have to print the error log, check above comment.
>>>>>
>>>>>>
>>>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>>>> - if (ret < 0) {
>>>>>> - dev_err(dev, "failed to set the clock of bus\n");
>>>>>> - goto out;
>>>>>> - }
>>>>>> -
>>>>>> - *freq = new_freq;
>>>>>> - bus->curr_freq = new_freq;
>>>>>> -
>>>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>>>> -out:
>>>>>> mutex_unlock(&bus->lock);
>>>>>>
>>>>>> return ret;
>>>>>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>>> struct exynos_bus *bus)
>>>>>> {
>>>>>> struct device *dev = bus->dev;
>>>>>> - int i, ret, count, size;
>>>>>> -
>>>>>> - /* Get the regulator to provide each bus with the power */
>>>>>> - bus->regulator = devm_regulator_get(dev, "vdd");
>>>>>> - if (IS_ERR(bus->regulator)) {
>>>>>> - dev_err(dev, "failed to get VDD regulator\n");
>>>>>> - return PTR_ERR(bus->regulator);
>>>>>> - }
>>>>>> -
>>>>>> - ret = regulator_enable(bus->regulator);
>>>>>> - if (ret < 0) {
>>>>>> - dev_err(dev, "failed to enable VDD regulator\n");
>>>>>> - return ret;
>>>>>> - }
>>>>>> + int i, count, size;
>>>>>>
>>>>>> /*
>>>>>> * Get the devfreq-event devices to get the current utilization of
>>>>>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>>> count = devfreq_event_get_edev_count(dev);
>>>>>> if (count < 0) {
>>>>>> dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>>>>> - ret = count;
>>>>>> - goto err_regulator;
>>>>>> + return count;
>>>>>> }
>>>>>> +
>>>>>> bus->edev_count = count;
>>>>>>
>>>>>> size = sizeof(*bus->edev) * count;
>>>>>> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>>>>> - if (!bus->edev) {
>>>>>> - ret = -ENOMEM;
>>>>>> - goto err_regulator;
>>>>>> - }
>>>>>> + if (!bus->edev)
>>>>>> + return -ENOMEM;
>>>>>>
>>>>>> for (i = 0; i < count; i++) {
>>>>>> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>>>>> - if (IS_ERR(bus->edev[i])) {
>>>>>> - ret = -EPROBE_DEFER;
>>>>>> - goto err_regulator;
>>>>>> - }
>>>>>> + if (IS_ERR(bus->edev[i]))
>>>>>> + return -EPROBE_DEFER;
>>>>>> }
>>>>>>
>>>>>> /*
>>>>>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>>> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>>>>> bus->ratio = DEFAULT_SATURATION_RATIO;
>>>>>>
>>>>>> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>>>>> - &bus->voltage_tolerance))
>>>>>> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>>>>> -
>>>>>> return 0;
>>>>>> -
>>>>>> -err_regulator:
>>>>>> - regulator_disable(bus->regulator);
>>>>>> -
>>>>>> - return ret;
>>>>>> }
>>>>>>
>>>>>> static int exynos_bus_parse_of(struct device_node *np,
>>>>>> - struct exynos_bus *bus)
>>>>>> + struct exynos_bus *bus, bool passive)
>>>>>> {
>>>>>> struct device *dev = bus->dev;
>>>>>> + struct opp_table *opp_table;
>>>>>> + const char *vdd = "vdd";
>>>>>> struct dev_pm_opp *opp;
>>>>>> unsigned long rate;
>>>>>> int ret;
>>>>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> + if (!passive) {
>>>>>> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>>>> + if (IS_ERR(opp_table)) {
>>>>>> + ret = PTR_ERR(opp_table);
>>>>>> + dev_err(dev, "failed to set regulators %d\n", ret);
>>>>>> + goto err_clk;/
>>>>>> + }
>>>>>> +
>>>>>> + bus->opp_table = opp_table;
>>>>>> + }
>>>>>
>>>>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>>>>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>>>>> because the regulator is only used by parent devfreq device.
>>>>
>>>> exynos_bus_parse_of() is called for all devfreq devices (including
>>>> parent) and (as you've noticed) the regulator should be enabled before
>>>> enabling clock (which is done in exynos_bus_parse_of()) so adding
>>>> extra argument to exynos_bus_parse_of() (like it is done currently in
>>>> the patch)
>>>
>>> I think that this patch has still the problem about call sequence
>>> between clock and regulator as following:
>>
>> Yes, this should be fixed (though the wrong sequence between regulator
>> and clock handling is not introduced by the patchset itself and is present
>> in the original driver code).
>>
>>> 273 ret = clk_prepare_enable(bus->clk);
>>> 274 if (ret < 0) {
>>> 275 dev_err(dev, "failed to get enable clock\n");
>>> 276 return ret;
>>> 277 }
>>> 278
>>> 279 if (!passive) {
>>> 280 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>> 281 if (IS_ERR(opp_table)) {
>>> 282 ret = PTR_ERR(opp_table);
>>> 283 dev_err(dev, "failed to set regulators %d\n", ret);
>>> 284 goto err_clk;
>>> 285 }
>>> 286
>>> 287 bus->opp_table = opp_table;
>>> 288 }
>>>
>>> makes it possible to do the setup correctly without the need
>>>> of merging both functions into one huge function (which would be more
>>>> difficult to follow than two simpler functions IMHO). Is that approach
>>>> acceptable or do you prefer one big function?
>>>
>>> Actually, I don't force to make one function for both
>>> exynos_bus_parse_of() and exynos_bus_parent_parse_of().
>>>
>>> If we just keep this code, dev_pm_opp_set_regulators()
>>> should be handled in exynos_bus_parent_parse_of()
>>> because only parent devfreq device controls the regulator.
>>
>> Could your please explain rationale for this requirement (besides
>> function name)?
>
> OK. I hope to satisfy the following requirements:
>
> 1. Fix the sequence problem between clock and regulator for enabling them.
> 2. dev_pm_opp_set_regulator() have to be handled in exynos_bus_parent_parse_of()
> instead of exynos_bus_parse_of() for only parent devfreq device.
> 3. exynos_bus_parse_of() have to handle the only common properties
> of both parent devfreq device and passive devfreq device.
>
>>
>> The patch adds 'bool passive' argument (which is set to false for
>> parent devfreq device and true for child devfreq device) to
>> exynos_bus_parse_of() (which is called for *all* devfreq devices
>
> As I menteiond, exynos_bus_parse_of have to handle the only common
> properties of both parent device and passive device.
>
> I gathered the properties for parent device into exynos_bus_parent_parse_of()
> This way using 'bool passive' argument is not proper in exynos_bus_parse_of().
>
>
>> and is called before exynos_bus_parent_parse_of()) and there is
>> no hard requirement to call dev_pm_opp_set_regulators() in
>> exynos_bus_parent_parse_of() so after only changing the ordering
>> between regulator and clock handling the setup code should be
>> correct.
>>
>> [ Please note that this patch moves parent/child detection before
>> exynos_bus_parse_of() call. ]
>>
>>> In order to keep the two functions, maybe have to change
>>> the call the sequence between exynos_bus_parse_of() and
>>> exynos_bus_parent_parse_of().
>>
>> Doesn't seem to be needed, care to explain it more?
>
> In order to fix the sequence problem between clock and regulator
> with dev_pm_opp_set_regualtor() and want to keep two functions
> (exynos_bus_parent_parse_of() and exynos_bus_parse_of()),
> have to change the call order as following and then modify
> the exception handling code when error happen.
>
> node = of_parse_phandle(dev->of_node, "devfreq", 0);
> if (node) {
> of_node_put(node);
> passive = true
> }
>
> if (!passive)
> exynos_bus_parent_parse_of()
> dev_pm_opp_set_regulator
>
> exynos_bus_parse_of()
OK. This seems like a solution.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
>>
>>> Once again, I don't force any fixed method. I want to fix them
>>> with correct way.
>>>
>>>>
>>>>>> +
>>>>>> /* Get the freq and voltage from OPP table to scale the bus freq */
>>>>>> ret = dev_pm_opp_of_add_table(dev);
>>>>>> if (ret < 0) {
>>>>>> dev_err(dev, "failed to get OPP table\n");
>>>>>> - goto err_clk;
>>>>>> + goto err_regulator;
>>>>>> }
>>>>>>
>>>>>> rate = clk_get_rate(bus->clk);
>>>>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>> ret = PTR_ERR(opp);
>>>>>> goto err_opp;
>>>>>> }
>>>>>> +
>>>>>> bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>>>> dev_pm_opp_put(opp);
>>>>>>
>>>>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>>
>>>>>> err_opp:
>>>>>> dev_pm_opp_of_remove_table(dev);
>>>>>> +
>>>>>> +err_regulator:
>>>>>> + if (bus->opp_table) {
>>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>>> + bus->opp_table = NULL;
>>>>>> + }
>>>>>
>>>>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>>>>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>>>>
>>>>>> +
>>>>>> err_clk:
>>>>>> clk_disable_unprepare(bus->clk);
>>>>>>
>>>>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>> struct exynos_bus *bus;
>>>>>> int ret, max_state;
>>>>>> unsigned long min_freq, max_freq;
>>>>>> + bool passive = false;
>>>>>>
>>>>>> if (!np) {
>>>>>> dev_err(dev, "failed to find devicetree node\n");
>>>>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>> bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>>>>> if (!bus)
>>>>>> return -ENOMEM;
>>>>>> +
>>>>>> mutex_init(&bus->lock);
>>>>>> bus->dev = &pdev->dev;
>>>>>> platform_set_drvdata(pdev, bus);
>>>>>> + node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>>> + if (node) {
>>>>>> + of_node_put(node);
>>>>>> + passive = true;
>>>>>> + }
>>>>>>
>>>>>> /* Parse the device-tree to get the resource information */
>>>>>> - ret = exynos_bus_parse_of(np, bus);
>>>>>> + ret = exynos_bus_parse_of(np, bus, passive);
>>>>>> if (ret < 0)
>>>>>> return ret;
>>>>>>
>>>>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>> goto err;
>>>>>> }
>>>>>>
>>>>>> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>>> - if (node) {
>>>>>> - of_node_put(node);
>>>>>> + if (passive)
>>>>>> goto passive;
>>>>>> - } else {
>>>>>> - ret = exynos_bus_parent_parse_of(np, bus);
>>>>>> - }
>>>>>> +
>>>>>> + ret = exynos_bus_parent_parse_of(np, bus);
>>>>>>
>>>>>
>>>>> Remove unneeded blank line.
>>>>>
>>>>>> if (ret < 0)
>>>>>> goto err;
>>>>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>>
>>>>>> err:
>>>>>> dev_pm_opp_of_remove_table(dev);
>>>>>> + if (bus->opp_table) {
>>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>>> + bus->opp_table = NULL;
>>>>>> + }
>>>>>> +
>>>>>
>>>>> ditto.
>>>>> Have to disable regulator after disabling the clock
>>>>> to prevent the h/w fault.
>>>>>
>>>>> I think that you should call them with following sequence:
>>>>>
>>>>> clk_disable_unprepare(bus->clk);
>>>>> if (bus->opp_table)
>>>>> dev_pm_opp_put_regulators(bus->opp_table);
>>>>> dev_pm_opp_of_remove_table(dev);
>>>>>
>>>>>> clk_disable_unprepare(bus->clk);
>>>>>>
>>>>>> return ret;
>>>>
>>>> Best regards,
>>>> --
>>>> Bartlomiej Zolnierkiewicz
>>>> Samsung R&D Institute Poland
>>>> Samsung Electronics
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
^ permalink raw reply
* Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Chanwoo Choi @ 2019-07-16 11:26 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Kamil Konieczny, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <3ba736fa-832c-a72c-e60b-f4328e54c524@samsung.com>
Hi,
On 19. 7. 16. 오후 7:59, Bartlomiej Zolnierkiewicz wrote:
>
> On 7/16/19 12:33 PM, Chanwoo Choi wrote:
>> Hi Bartlomiej,
>>
>> On 19. 7. 16. 오후 7:13, Bartlomiej Zolnierkiewicz wrote:
>>>
>>> Hi Chanwoo,
>>>
>>> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>>>> Hi Kamil,
>>>>
>>>> Looks good to me. But, this patch has some issue.
>>>> I added the detailed reviews.
>>>>
>>>> I recommend that you make the separate patches as following
>>>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>>>
>>>> First patch,
>>>> Need to consolidate the following two function into one function.
>>>> because the original exynos-bus.c has the problem that the regulator
>>>> of parent devfreq device have to be enabled before enabling the clock.
>>>> This issue did not happen because bootloader enables the bus-related
>>>> regulators before kernel booting.
>>>> - exynos_bus_parse_of()
>>>> - exynos_bus_parent_parse_of()
>>>>> Second patch,
>>>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>>>
>>>>
>>>> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>>>>> Reuse opp core code for setting bus clock and voltage. As a side
>>>>> effect this allow useage of coupled regulators feature (required
>>>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>>>> while the old code used regulator_set_voltage_tol() with fixed
>>>>> tolerance. This patch also removes no longer needed parsing of DT
>>>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>>>> it).
>>>>>
>>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>>> ---
>>>>> drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>>>> 1 file changed, 66 insertions(+), 106 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>>> index 486cc5b422f1..7fc4f76bd848 100644
>>>>> --- a/drivers/devfreq/exynos-bus.c
>>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>>> @@ -25,7 +25,6 @@
>>>>> #include <linux/slab.h>
>>>>>
>>>>> #define DEFAULT_SATURATION_RATIO 40
>>>>> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>>>>>
>>>>> struct exynos_bus {
>>>>> struct device *dev;
>>>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>>>
>>>>> unsigned long curr_freq;
>>>>>
>>>>> - struct regulator *regulator;
>>>>> + struct opp_table *opp_table;
>>>>> +
>>>>> struct clk *clk;
>>>>> - unsigned int voltage_tolerance;
>>>>> unsigned int ratio;
>>>>> };
>>>>>
>>>>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>>>> {
>>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>> struct dev_pm_opp *new_opp;
>>>>> - unsigned long old_freq, new_freq, new_volt, tol;
>>>>> int ret = 0;
>>>>> -
>>>>> - /* Get new opp-bus instance according to new bus clock */
>>>>> + /*
>>>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>>>> + * *freq to correct value.
>>>>> + */
>>>>
>>>> You better to change this comment with following styles
>>>> to keep the consistency:
>>>>
>>>> /* Get correct frequency for bus ... */
>>>>
>>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>> if (IS_ERR(new_opp)) {
>>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>>> return PTR_ERR(new_opp);
>>>>> }
>>>>>
>>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>>> - new_volt = dev_pm_opp_get_voltage(new_opp);
>>>>> dev_pm_opp_put(new_opp);
>>>>>
>>>>> - old_freq = bus->curr_freq;
>>>>> -
>>>>> - if (old_freq == new_freq)
>>>>> - return 0;
>>>>> - tol = new_volt * bus->voltage_tolerance / 100;
>>>>> -
>>>>> /* Change voltage and frequency according to new OPP level */
>>>>> mutex_lock(&bus->lock);
>>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>>> + if (!ret)
>>>>> + bus->curr_freq = *freq;
>>>>
>>>> Have to print the error log if ret has minus error value.
>>>
>>> dev_pm_opp_set_rate() should print the error message on all
>>> errors so wouldn't printing the error log also here be superfluous?
>>>
>>> [ Please also note that the other user of dev_pm_opp_set_rate()
>>> (cpufreq-dt cpufreq driver) doesn't do this. ]
>>
>> OK. Thanks for the explanation.
>>
>>>
>>>> Modify it as following:
>>>>
>>>> if (ret < 0) {
>>>> dev_err(dev, "failed to set bus rate\n");
>>>> goto err:
>>>> }
>>>> bus->curr_freq = *freq;
>>>>
>>>> err:
>>>> mutex_unlock(&bus->lock);
>>>>
>>>> return ret;
>>>>
>>>>>
>>>>> - if (old_freq < new_freq) {
>>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>> - if (ret < 0) {
>>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>>> - goto out;
>>>>> - }
>>>>> - }
>>>>> -
>>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>>> - if (ret < 0) {
>>>>> - dev_err(dev, "failed to change clock of bus\n");
>>>>> - clk_set_rate(bus->clk, old_freq);
>>>>> - goto out;
>>>>> - }
>>>>> -
>>>>> - if (old_freq > new_freq) {
>>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>> - if (ret < 0) {
>>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>>> - goto out;
>>>>> - }
>>>>> - }
>>>>> - bus->curr_freq = new_freq;
>>>>> -
>>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>>> -out:
>>>>> mutex_unlock(&bus->lock);
>>>>>
>>>>> return ret;
>>>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>>>> if (ret < 0)
>>>>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>>>
>>>>> - if (bus->regulator)
>>>>> - regulator_disable(bus->regulator);
>>>>> + if (bus->opp_table)
>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>
>>>> Have to disable regulator after disabling the clock
>>>> to prevent the h/w fault.
>>>>
>>>> I think that you should call them with following sequence:
>>>>
>>>> clk_disable_unprepare(bus->clk);
>>>> if (bus->opp_table)
>>>> dev_pm_opp_put_regulators(bus->opp_table);
>>>> dev_pm_opp_of_remove_table(dev);
>>>>
>>>>>
>>>>> dev_pm_opp_of_remove_table(dev);
>>>>> +
>>>>> clk_disable_unprepare(bus->clk);
>>>>> }
>>>>>
>>>>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>>>> {
>>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>> struct dev_pm_opp *new_opp;
>>>>> - unsigned long old_freq, new_freq;
>>>>> - int ret = 0;
>>>>> + int ret;
>>>>>
>>>>> - /* Get new opp-bus instance according to new bus clock */
>>>>> + /*
>>>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>>>> + * *freq to correct value.
>>>>> + */
>>>>
>>>> You better to change this comment with following styles
>>>> to keep the consistency:
>>>>
>>>> /* Get correct frequency for bus ... */
>>>>
>>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>> if (IS_ERR(new_opp)) {
>>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>>> return PTR_ERR(new_opp);
>>>>> }
>>>>>
>>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>>> dev_pm_opp_put(new_opp);
>>>>>
>>>>> - old_freq = bus->curr_freq;
>>>>> -
>>>>> - if (old_freq == new_freq)
>>>>> - return 0;
>>>>> -
>>>>> /* Change the frequency according to new OPP level */
>>>>> mutex_lock(&bus->lock);
>>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>>> + if (!ret)
>>>>> + bus->curr_freq = *freq;
>>>>
>>>> ditto. Have to print the error log, check above comment.
>>>>
>>>>>
>>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>>> - if (ret < 0) {
>>>>> - dev_err(dev, "failed to set the clock of bus\n");
>>>>> - goto out;
>>>>> - }
>>>>> -
>>>>> - *freq = new_freq;
>>>>> - bus->curr_freq = new_freq;
>>>>> -
>>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>>> -out:
>>>>> mutex_unlock(&bus->lock);
>>>>>
>>>>> return ret;
>>>>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>> struct exynos_bus *bus)
>>>>> {
>>>>> struct device *dev = bus->dev;
>>>>> - int i, ret, count, size;
>>>>> -
>>>>> - /* Get the regulator to provide each bus with the power */
>>>>> - bus->regulator = devm_regulator_get(dev, "vdd");
>>>>> - if (IS_ERR(bus->regulator)) {
>>>>> - dev_err(dev, "failed to get VDD regulator\n");
>>>>> - return PTR_ERR(bus->regulator);
>>>>> - }
>>>>> -
>>>>> - ret = regulator_enable(bus->regulator);
>>>>> - if (ret < 0) {
>>>>> - dev_err(dev, "failed to enable VDD regulator\n");
>>>>> - return ret;
>>>>> - }
>>>>> + int i, count, size;
>>>>>
>>>>> /*
>>>>> * Get the devfreq-event devices to get the current utilization of
>>>>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>> count = devfreq_event_get_edev_count(dev);
>>>>> if (count < 0) {
>>>>> dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>>>> - ret = count;
>>>>> - goto err_regulator;
>>>>> + return count;
>>>>> }
>>>>> +
>>>>> bus->edev_count = count;
>>>>>
>>>>> size = sizeof(*bus->edev) * count;
>>>>> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>>>> - if (!bus->edev) {
>>>>> - ret = -ENOMEM;
>>>>> - goto err_regulator;
>>>>> - }
>>>>> + if (!bus->edev)
>>>>> + return -ENOMEM;
>>>>>
>>>>> for (i = 0; i < count; i++) {
>>>>> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>>>> - if (IS_ERR(bus->edev[i])) {
>>>>> - ret = -EPROBE_DEFER;
>>>>> - goto err_regulator;
>>>>> - }
>>>>> + if (IS_ERR(bus->edev[i]))
>>>>> + return -EPROBE_DEFER;
>>>>> }
>>>>>
>>>>> /*
>>>>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>>>> bus->ratio = DEFAULT_SATURATION_RATIO;
>>>>>
>>>>> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>>>> - &bus->voltage_tolerance))
>>>>> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>>>> -
>>>>> return 0;
>>>>> -
>>>>> -err_regulator:
>>>>> - regulator_disable(bus->regulator);
>>>>> -
>>>>> - return ret;
>>>>> }
>>>>>
>>>>> static int exynos_bus_parse_of(struct device_node *np,
>>>>> - struct exynos_bus *bus)
>>>>> + struct exynos_bus *bus, bool passive)
>>>>> {
>>>>> struct device *dev = bus->dev;
>>>>> + struct opp_table *opp_table;
>>>>> + const char *vdd = "vdd";
>>>>> struct dev_pm_opp *opp;
>>>>> unsigned long rate;
>>>>> int ret;
>>>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>> return ret;
>>>>> }
>>>>>
>>>>> + if (!passive) {
>>>>> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>>> + if (IS_ERR(opp_table)) {
>>>>> + ret = PTR_ERR(opp_table);
>>>>> + dev_err(dev, "failed to set regulators %d\n", ret);
>>>>> + goto err_clk;/
>>>>> + }
>>>>> +
>>>>> + bus->opp_table = opp_table;
>>>>> + }
>>>>
>>>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>>>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>>>> because the regulator is only used by parent devfreq device.
>>>
>>> exynos_bus_parse_of() is called for all devfreq devices (including
>>> parent) and (as you've noticed) the regulator should be enabled before
>>> enabling clock (which is done in exynos_bus_parse_of()) so adding
>>> extra argument to exynos_bus_parse_of() (like it is done currently in
>>> the patch)
>>
>> I think that this patch has still the problem about call sequence
>> between clock and regulator as following:
>
> Yes, this should be fixed (though the wrong sequence between regulator
> and clock handling is not introduced by the patchset itself and is present
> in the original driver code).
>
>> 273 ret = clk_prepare_enable(bus->clk);
>> 274 if (ret < 0) {
>> 275 dev_err(dev, "failed to get enable clock\n");
>> 276 return ret;
>> 277 }
>> 278
>> 279 if (!passive) {
>> 280 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>> 281 if (IS_ERR(opp_table)) {
>> 282 ret = PTR_ERR(opp_table);
>> 283 dev_err(dev, "failed to set regulators %d\n", ret);
>> 284 goto err_clk;
>> 285 }
>> 286
>> 287 bus->opp_table = opp_table;
>> 288 }
>>
>> makes it possible to do the setup correctly without the need
>>> of merging both functions into one huge function (which would be more
>>> difficult to follow than two simpler functions IMHO). Is that approach
>>> acceptable or do you prefer one big function?
>>
>> Actually, I don't force to make one function for both
>> exynos_bus_parse_of() and exynos_bus_parent_parse_of().
>>
>> If we just keep this code, dev_pm_opp_set_regulators()
>> should be handled in exynos_bus_parent_parse_of()
>> because only parent devfreq device controls the regulator.
>
> Could your please explain rationale for this requirement (besides
> function name)?
OK. I hope to satisfy the following requirements:
1. Fix the sequence problem between clock and regulator for enabling them.
2. dev_pm_opp_set_regulator() have to be handled in exynos_bus_parent_parse_of()
instead of exynos_bus_parse_of() for only parent devfreq device.
3. exynos_bus_parse_of() have to handle the only common properties
of both parent devfreq device and passive devfreq device.
>
> The patch adds 'bool passive' argument (which is set to false for
> parent devfreq device and true for child devfreq device) to
> exynos_bus_parse_of() (which is called for *all* devfreq devices
As I menteiond, exynos_bus_parse_of have to handle the only common
properties of both parent device and passive device.
I gathered the properties for parent device into exynos_bus_parent_parse_of()
This way using 'bool passive' argument is not proper in exynos_bus_parse_of().
> and is called before exynos_bus_parent_parse_of()) and there is
> no hard requirement to call dev_pm_opp_set_regulators() in
> exynos_bus_parent_parse_of() so after only changing the ordering
> between regulator and clock handling the setup code should be
> correct.
>
> [ Please note that this patch moves parent/child detection before
> exynos_bus_parse_of() call. ]
>
>> In order to keep the two functions, maybe have to change
>> the call the sequence between exynos_bus_parse_of() and
>> exynos_bus_parent_parse_of().
>
> Doesn't seem to be needed, care to explain it more?
In order to fix the sequence problem between clock and regulator
with dev_pm_opp_set_regualtor() and want to keep two functions
(exynos_bus_parent_parse_of() and exynos_bus_parse_of()),
have to change the call order as following and then modify
the exception handling code when error happen.
node = of_parse_phandle(dev->of_node, "devfreq", 0);
if (node) {
of_node_put(node);
passive = true
}
if (!passive)
exynos_bus_parent_parse_of()
dev_pm_opp_set_regulator
exynos_bus_parse_of()
>
>> Once again, I don't force any fixed method. I want to fix them
>> with correct way.
>>
>>>
>>>>> +
>>>>> /* Get the freq and voltage from OPP table to scale the bus freq */
>>>>> ret = dev_pm_opp_of_add_table(dev);
>>>>> if (ret < 0) {
>>>>> dev_err(dev, "failed to get OPP table\n");
>>>>> - goto err_clk;
>>>>> + goto err_regulator;
>>>>> }
>>>>>
>>>>> rate = clk_get_rate(bus->clk);
>>>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>> ret = PTR_ERR(opp);
>>>>> goto err_opp;
>>>>> }
>>>>> +
>>>>> bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>>> dev_pm_opp_put(opp);
>>>>>
>>>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>
>>>>> err_opp:
>>>>> dev_pm_opp_of_remove_table(dev);
>>>>> +
>>>>> +err_regulator:
>>>>> + if (bus->opp_table) {
>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>> + bus->opp_table = NULL;
>>>>> + }
>>>>
>>>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>>>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>>>
>>>>> +
>>>>> err_clk:
>>>>> clk_disable_unprepare(bus->clk);
>>>>>
>>>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>> struct exynos_bus *bus;
>>>>> int ret, max_state;
>>>>> unsigned long min_freq, max_freq;
>>>>> + bool passive = false;
>>>>>
>>>>> if (!np) {
>>>>> dev_err(dev, "failed to find devicetree node\n");
>>>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>> bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>>>> if (!bus)
>>>>> return -ENOMEM;
>>>>> +
>>>>> mutex_init(&bus->lock);
>>>>> bus->dev = &pdev->dev;
>>>>> platform_set_drvdata(pdev, bus);
>>>>> + node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>> + if (node) {
>>>>> + of_node_put(node);
>>>>> + passive = true;
>>>>> + }
>>>>>
>>>>> /* Parse the device-tree to get the resource information */
>>>>> - ret = exynos_bus_parse_of(np, bus);
>>>>> + ret = exynos_bus_parse_of(np, bus, passive);
>>>>> if (ret < 0)
>>>>> return ret;
>>>>>
>>>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>> goto err;
>>>>> }
>>>>>
>>>>> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>> - if (node) {
>>>>> - of_node_put(node);
>>>>> + if (passive)
>>>>> goto passive;
>>>>> - } else {
>>>>> - ret = exynos_bus_parent_parse_of(np, bus);
>>>>> - }
>>>>> +
>>>>> + ret = exynos_bus_parent_parse_of(np, bus);
>>>>>
>>>>
>>>> Remove unneeded blank line.
>>>>
>>>>> if (ret < 0)
>>>>> goto err;
>>>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>
>>>>> err:
>>>>> dev_pm_opp_of_remove_table(dev);
>>>>> + if (bus->opp_table) {
>>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>>> + bus->opp_table = NULL;
>>>>> + }
>>>>> +
>>>>
>>>> ditto.
>>>> Have to disable regulator after disabling the clock
>>>> to prevent the h/w fault.
>>>>
>>>> I think that you should call them with following sequence:
>>>>
>>>> clk_disable_unprepare(bus->clk);
>>>> if (bus->opp_table)
>>>> dev_pm_opp_put_regulators(bus->opp_table);
>>>> dev_pm_opp_of_remove_table(dev);
>>>>
>>>>> clk_disable_unprepare(bus->clk);
>>>>>
>>>>> return ret;
>>>
>>> Best regards,
>>> --
>>> Bartlomiej Zolnierkiewicz
>>> Samsung R&D Institute Poland
>>> Samsung Electronics
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH V13 12/12] PCI: tegra: Add Tegra194 PCIe support
From: Lorenzo Pieralisi @ 2019-07-16 11:22 UTC (permalink / raw)
To: Vidya Sagar
Cc: bhelgaas, robh+dt, mark.rutland, thierry.reding, jonathanh,
kishon, catalin.marinas, will.deacon, jingoohan1,
gustavo.pimentel, digetx, mperttunen, linux-pci, devicetree,
linux-tegra, linux-kernel, linux-arm-kernel, kthota, mmaddireddy,
sagar.tv
In-Reply-To: <a5f8689b-1358-dd2d-4f54-7e68a6ab158b@nvidia.com>
On Sat, Jul 13, 2019 at 12:34:34PM +0530, Vidya Sagar wrote:
[...]
> > > > > +static int tegra_pcie_bpmp_set_ctrl_state(struct tegra_pcie_dw *pcie,
> > > > > + bool enable)
> > > > > +{
> > > > > + struct mrq_uphy_response resp;
> > > > > + struct tegra_bpmp_message msg;
> > > > > + struct mrq_uphy_request req;
> > > > > + int err;
> > > > > +
> > > > > + if (pcie->cid == 5)
> > > > > + return 0;
> > > >
> > > > What's wrong with cid == 5 ? Explain please.
> > > Controller with ID=5 doesn't need any programming to enable it which is
> > > done here through calling firmware API.
> > >
> > > >
> > > > > + memset(&req, 0, sizeof(req));
> > > > > + memset(&resp, 0, sizeof(resp));
> > > > > +
> > > > > + req.cmd = CMD_UPHY_PCIE_CONTROLLER_STATE;
> > > > > + req.controller_state.pcie_controller = pcie->cid;
> > > > > + req.controller_state.enable = enable;
> > > > > +
> > > > > + memset(&msg, 0, sizeof(msg));
> > > > > + msg.mrq = MRQ_UPHY;
> > > > > + msg.tx.data = &req;
> > > > > + msg.tx.size = sizeof(req);
> > > > > + msg.rx.data = &resp;
> > > > > + msg.rx.size = sizeof(resp);
> > > > > +
> > > > > + if (irqs_disabled())
> > > >
> > > > Can you explain to me what this check is meant to achieve please ?
> > > Firmware interface provides different APIs to be called when there are
> > > no interrupts enabled in the system (noirq context) and otherwise
> > > hence checking that situation here and calling appropriate API.
> >
> > That's what I am questioning. Being called from {suspend/resume}_noirq()
> > callbacks (if that's the code path this check caters for) does not mean
> > irqs_disabled() == true.
> Agree.
> Actually, I got a hint of having this check from the following.
> Both tegra_bpmp_transfer_atomic() and tegra_bpmp_transfer() are indirectly
> called by APIs registered with .master_xfer() and .master_xfer_atomic() hooks of
> struct i2c_algorithm and the decision to call which one of these is made using the
> following check in i2c-core.h file.
> static inline bool i2c_in_atomic_xfer_mode(void)
> {
> return system_state > SYSTEM_RUNNING && irqs_disabled();
> }
> I think I should use this condition as is IIUC.
> Please let me know if there are any concerns with this.
It is not a concern, it is just that I don't understand how this code
can be called with IRQs disabled, if you can give me an execution path I
am happy to leave the check there. On top of that, when called from
suspend NOIRQ context, it is likely to use the blocking API (because
IRQs aren't disabled at CPU level) behind which there is most certainly
an IRQ required to wake the thread up and if the IRQ in question was
disabled in the suspend NOIRQ phase this code is likely to deadlock.
I want to make sure we can justify adding this check, I do not
want to add it because we think it can be needed when it may not
be needed at all (and it gets copy and pasted over and over again
in other drivers).
> > Actually, if tegra_bpmp_transfer() requires IRQs to be enabled you may
> > even end up in a situation where that blocking call does not wake up
> > because the IRQ in question was disabled in the NOIRQ suspend/resume
> > phase.
> >
> > [...]
> >
> > > > > +static int tegra_pcie_dw_probe(struct platform_device *pdev)
> > > > > +{
> > > > > + const struct tegra_pcie_soc *data;
> > > > > + struct device *dev = &pdev->dev;
> > > > > + struct resource *atu_dma_res;
> > > > > + struct tegra_pcie_dw *pcie;
> > > > > + struct resource *dbi_res;
> > > > > + struct pcie_port *pp;
> > > > > + struct dw_pcie *pci;
> > > > > + struct phy **phys;
> > > > > + char *name;
> > > > > + int ret;
> > > > > + u32 i;
> > > > > +
> > > > > + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> > > > > + if (!pcie)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + pci = &pcie->pci;
> > > > > + pci->dev = &pdev->dev;
> > > > > + pci->ops = &tegra_dw_pcie_ops;
> > > > > + pp = &pci->pp;
> > > > > + pcie->dev = &pdev->dev;
> > > > > +
> > > > > + data = (struct tegra_pcie_soc *)of_device_get_match_data(dev);
> > > > > + if (!data)
> > > > > + return -EINVAL;
> > > > > + pcie->mode = (enum dw_pcie_device_mode)data->mode;
> > > > > +
> > > > > + ret = tegra_pcie_dw_parse_dt(pcie);
> > > > > + if (ret < 0) {
> > > > > + dev_err(dev, "Failed to parse device tree: %d\n", ret);
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl");
> > > > > + if (IS_ERR(pcie->pex_ctl_supply)) {
> > > > > + dev_err(dev, "Failed to get regulator: %ld\n",
> > > > > + PTR_ERR(pcie->pex_ctl_supply));
> > > > > + return PTR_ERR(pcie->pex_ctl_supply);
> > > > > + }
> > > > > +
> > > > > + pcie->core_clk = devm_clk_get(dev, "core");
> > > > > + if (IS_ERR(pcie->core_clk)) {
> > > > > + dev_err(dev, "Failed to get core clock: %ld\n",
> > > > > + PTR_ERR(pcie->core_clk));
> > > > > + return PTR_ERR(pcie->core_clk);
> > > > > + }
> > > > > +
> > > > > + pcie->appl_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > > + "appl");
> > > > > + if (!pcie->appl_res) {
> > > > > + dev_err(dev, "Failed to find \"appl\" region\n");
> > > > > + return PTR_ERR(pcie->appl_res);
> > > > > + }
> > > > > + pcie->appl_base = devm_ioremap_resource(dev, pcie->appl_res);
> > > > > + if (IS_ERR(pcie->appl_base))
> > > > > + return PTR_ERR(pcie->appl_base);
> > > > > +
> > > > > + pcie->core_apb_rst = devm_reset_control_get(dev, "apb");
> > > > > + if (IS_ERR(pcie->core_apb_rst)) {
> > > > > + dev_err(dev, "Failed to get APB reset: %ld\n",
> > > > > + PTR_ERR(pcie->core_apb_rst));
> > > > > + return PTR_ERR(pcie->core_apb_rst);
> > > > > + }
> > > > > +
> > > > > + phys = devm_kcalloc(dev, pcie->phy_count, sizeof(*phys), GFP_KERNEL);
> > > > > + if (!phys)
> > > > > + return PTR_ERR(phys);
> > > > > +
> > > > > + for (i = 0; i < pcie->phy_count; i++) {
> > > > > + name = kasprintf(GFP_KERNEL, "p2u-%u", i);
> > > > > + if (!name) {
> > > > > + dev_err(dev, "Failed to create P2U string\n");
> > > > > + return -ENOMEM;
> > > > > + }
> > > > > + phys[i] = devm_phy_get(dev, name);
> > > > > + kfree(name);
> > > > > + if (IS_ERR(phys[i])) {
> > > > > + ret = PTR_ERR(phys[i]);
> > > > > + dev_err(dev, "Failed to get PHY: %d\n", ret);
> > > > > + return ret;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + pcie->phys = phys;
> > > > > +
> > > > > + dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> > > > > + if (!dbi_res) {
> > > > > + dev_err(dev, "Failed to find \"dbi\" region\n");
> > > > > + return PTR_ERR(dbi_res);
> > > > > + }
> > > > > + pcie->dbi_res = dbi_res;
> > > > > +
> > > > > + pci->dbi_base = devm_ioremap_resource(dev, dbi_res);
> > > > > + if (IS_ERR(pci->dbi_base))
> > > > > + return PTR_ERR(pci->dbi_base);
> > > > > +
> > > > > + /* Tegra HW locates DBI2 at a fixed offset from DBI */
> > > > > + pci->dbi_base2 = pci->dbi_base + 0x1000;
> > > > > +
> > > > > + atu_dma_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > > + "atu_dma");
> > > > > + if (!atu_dma_res) {
> > > > > + dev_err(dev, "Failed to find \"atu_dma\" region\n");
> > > > > + return PTR_ERR(atu_dma_res);
> > > > > + }
> > > > > + pcie->atu_dma_res = atu_dma_res;
> > > > > + pci->atu_base = devm_ioremap_resource(dev, atu_dma_res);
> > > > > + if (IS_ERR(pci->atu_base))
> > > > > + return PTR_ERR(pci->atu_base);
> > > > > +
> > > > > + pcie->core_rst = devm_reset_control_get(dev, "core");
> > > > > + if (IS_ERR(pcie->core_rst)) {
> > > > > + dev_err(dev, "Failed to get core reset: %ld\n",
> > > > > + PTR_ERR(pcie->core_rst));
> > > > > + return PTR_ERR(pcie->core_rst);
> > > > > + }
> > > > > +
> > > > > + pp->irq = platform_get_irq_byname(pdev, "intr");
> > > > > + if (!pp->irq) {
> > > > > + dev_err(dev, "Failed to get \"intr\" interrupt\n");
> > > > > + return -ENODEV;
> > > > > + }
> > > > > +
> > > > > + ret = devm_request_irq(dev, pp->irq, tegra_pcie_irq_handler,
> > > > > + IRQF_SHARED, "tegra-pcie-intr", pcie);
> > > > > + if (ret) {
> > > > > + dev_err(dev, "Failed to request IRQ %d: %d\n", pp->irq, ret);
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + pcie->bpmp = tegra_bpmp_get(dev);
> > > > > + if (IS_ERR(pcie->bpmp))
> > > > > + return PTR_ERR(pcie->bpmp);
> > > > > +
> > > > > + platform_set_drvdata(pdev, pcie);
> > > > > +
> > > > > + if (pcie->mode == DW_PCIE_RC_TYPE) {
> > > > > + ret = tegra_pcie_config_rp(pcie);
> > > > > + if (ret && ret != -ENOMEDIUM)
> > > > > + goto fail;
> > > > > + else
> > > > > + return 0;
> > > >
> > > > So if the link is not up we still go ahead and make probe
> > > > succeed. What for ?
> > > We may need root port to be available to support hot-plugging of
> > > endpoint devices, so, we don't fail the probe.
> >
> > We need it or we don't. If you do support hotplugging of endpoint
> > devices point me at the code, otherwise link up failure means
> > failure to probe.
> Currently hotplugging of endpoint is not supported, but it is one of
> the use cases that we may add support for in future.
You should elaborate on this, I do not understand what you mean,
either the root port(s) supports hotplug or it does not.
> But, why should we fail probe if link up doesn't happen? As such,
> nothing went wrong in terms of root port initialization right? I
> checked other DWC based implementations and following are not failing
> the probe pci-dra7xx.c, pcie-armada8k.c, pcie-artpec6.c, pcie-histb.c,
> pcie-kirin.c, pcie-spear13xx.c, pci-exynos.c, pci-imx6.c,
> pci-keystone.c, pci-layerscape.c
>
> Although following do fail the probe if link is not up. pcie-qcom.c,
> pcie-uniphier.c, pci-meson.c
>
> So, to me, it looks more like a choice we can make whether to fail the
> probe or not and in this case we are choosing not to fail.
I disagree. I had an offline chat with Bjorn and whether link-up should
fail the probe or not depends on whether the root port(s) is hotplug
capable or not and this in turn relies on the root port "Slot
implemented" bit in the PCI Express capabilities register.
It is a choice but it should be based on evidence.
Lorenzo
^ permalink raw reply
* Re: [PATCH] dt-bindings: Ensure child nodes are of type 'object'
From: Mark Brown @ 2019-07-16 11:09 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, Vignesh Raghavendra, linux-gpio, Maxime Ripard,
Richard Weinberger, Linus Walleij, linux-kernel, linux-spi,
Marek Vasut, Chen-Yu Tsai, linux-mtd, Maxime Coquelin,
Miquel Raynal, Brian Norris, David Woodhouse, linux-stm32,
Alexandre Torgue
In-Reply-To: <20190715230457.3901-1-robh@kernel.org>
[-- Attachment #1.1: Type: text/plain, Size: 375 bytes --]
On Mon, Jul 15, 2019 at 05:04:57PM -0600, Rob Herring wrote:
> Properties which are child node definitions need to have an explict
> type. Otherwise, a matching (DT) property can silently match when an
> error is desired. Fix this up tree-wide. Once this is fixed, the
> meta-schema will enforce this on any child node definitions.
Acked-by: Mark Brown <broonie@kernel.org>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 144 bytes --]
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply
* Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Bartlomiej Zolnierkiewicz @ 2019-07-16 10:59 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Kamil Konieczny, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <5612547b-47c8-0dc4-cb3c-e972782d5a26@samsung.com>
On 7/16/19 12:33 PM, Chanwoo Choi wrote:
> Hi Bartlomiej,
>
> On 19. 7. 16. 오후 7:13, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi Chanwoo,
>>
>> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>>> Hi Kamil,
>>>
>>> Looks good to me. But, this patch has some issue.
>>> I added the detailed reviews.
>>>
>>> I recommend that you make the separate patches as following
>>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>>
>>> First patch,
>>> Need to consolidate the following two function into one function.
>>> because the original exynos-bus.c has the problem that the regulator
>>> of parent devfreq device have to be enabled before enabling the clock.
>>> This issue did not happen because bootloader enables the bus-related
>>> regulators before kernel booting.
>>> - exynos_bus_parse_of()
>>> - exynos_bus_parent_parse_of()
>>>> Second patch,
>>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>>
>>>
>>> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>>>> Reuse opp core code for setting bus clock and voltage. As a side
>>>> effect this allow useage of coupled regulators feature (required
>>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>>> while the old code used regulator_set_voltage_tol() with fixed
>>>> tolerance. This patch also removes no longer needed parsing of DT
>>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>>> it).
>>>>
>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>> ---
>>>> drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>>> 1 file changed, 66 insertions(+), 106 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>> index 486cc5b422f1..7fc4f76bd848 100644
>>>> --- a/drivers/devfreq/exynos-bus.c
>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>> @@ -25,7 +25,6 @@
>>>> #include <linux/slab.h>
>>>>
>>>> #define DEFAULT_SATURATION_RATIO 40
>>>> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>>>>
>>>> struct exynos_bus {
>>>> struct device *dev;
>>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>>
>>>> unsigned long curr_freq;
>>>>
>>>> - struct regulator *regulator;
>>>> + struct opp_table *opp_table;
>>>> +
>>>> struct clk *clk;
>>>> - unsigned int voltage_tolerance;
>>>> unsigned int ratio;
>>>> };
>>>>
>>>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>>> {
>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>> struct dev_pm_opp *new_opp;
>>>> - unsigned long old_freq, new_freq, new_volt, tol;
>>>> int ret = 0;
>>>> -
>>>> - /* Get new opp-bus instance according to new bus clock */
>>>> + /*
>>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>>> + * *freq to correct value.
>>>> + */
>>>
>>> You better to change this comment with following styles
>>> to keep the consistency:
>>>
>>> /* Get correct frequency for bus ... */
>>>
>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>> if (IS_ERR(new_opp)) {
>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>> return PTR_ERR(new_opp);
>>>> }
>>>>
>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>> - new_volt = dev_pm_opp_get_voltage(new_opp);
>>>> dev_pm_opp_put(new_opp);
>>>>
>>>> - old_freq = bus->curr_freq;
>>>> -
>>>> - if (old_freq == new_freq)
>>>> - return 0;
>>>> - tol = new_volt * bus->voltage_tolerance / 100;
>>>> -
>>>> /* Change voltage and frequency according to new OPP level */
>>>> mutex_lock(&bus->lock);
>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>> + if (!ret)
>>>> + bus->curr_freq = *freq;
>>>
>>> Have to print the error log if ret has minus error value.
>>
>> dev_pm_opp_set_rate() should print the error message on all
>> errors so wouldn't printing the error log also here be superfluous?
>>
>> [ Please also note that the other user of dev_pm_opp_set_rate()
>> (cpufreq-dt cpufreq driver) doesn't do this. ]
>
> OK. Thanks for the explanation.
>
>>
>>> Modify it as following:
>>>
>>> if (ret < 0) {
>>> dev_err(dev, "failed to set bus rate\n");
>>> goto err:
>>> }
>>> bus->curr_freq = *freq;
>>>
>>> err:
>>> mutex_unlock(&bus->lock);
>>>
>>> return ret;
>>>
>>>>
>>>> - if (old_freq < new_freq) {
>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>> - if (ret < 0) {
>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>> - goto out;
>>>> - }
>>>> - }
>>>> -
>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>> - if (ret < 0) {
>>>> - dev_err(dev, "failed to change clock of bus\n");
>>>> - clk_set_rate(bus->clk, old_freq);
>>>> - goto out;
>>>> - }
>>>> -
>>>> - if (old_freq > new_freq) {
>>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>> - if (ret < 0) {
>>>> - dev_err(bus->dev, "failed to set voltage\n");
>>>> - goto out;
>>>> - }
>>>> - }
>>>> - bus->curr_freq = new_freq;
>>>> -
>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>> -out:
>>>> mutex_unlock(&bus->lock);
>>>>
>>>> return ret;
>>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>>> if (ret < 0)
>>>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>>
>>>> - if (bus->regulator)
>>>> - regulator_disable(bus->regulator);
>>>> + if (bus->opp_table)
>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>
>>> Have to disable regulator after disabling the clock
>>> to prevent the h/w fault.
>>>
>>> I think that you should call them with following sequence:
>>>
>>> clk_disable_unprepare(bus->clk);
>>> if (bus->opp_table)
>>> dev_pm_opp_put_regulators(bus->opp_table);
>>> dev_pm_opp_of_remove_table(dev);
>>>
>>>>
>>>> dev_pm_opp_of_remove_table(dev);
>>>> +
>>>> clk_disable_unprepare(bus->clk);
>>>> }
>>>>
>>>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>>> {
>>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>>> struct dev_pm_opp *new_opp;
>>>> - unsigned long old_freq, new_freq;
>>>> - int ret = 0;
>>>> + int ret;
>>>>
>>>> - /* Get new opp-bus instance according to new bus clock */
>>>> + /*
>>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>>> + * *freq to correct value.
>>>> + */
>>>
>>> You better to change this comment with following styles
>>> to keep the consistency:
>>>
>>> /* Get correct frequency for bus ... */
>>>
>>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>> if (IS_ERR(new_opp)) {
>>>> dev_err(dev, "failed to get recommended opp instance\n");
>>>> return PTR_ERR(new_opp);
>>>> }
>>>>
>>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>>> dev_pm_opp_put(new_opp);
>>>>
>>>> - old_freq = bus->curr_freq;
>>>> -
>>>> - if (old_freq == new_freq)
>>>> - return 0;
>>>> -
>>>> /* Change the frequency according to new OPP level */
>>>> mutex_lock(&bus->lock);
>>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>>> + if (!ret)
>>>> + bus->curr_freq = *freq;
>>>
>>> ditto. Have to print the error log, check above comment.
>>>
>>>>
>>>> - ret = clk_set_rate(bus->clk, new_freq);
>>>> - if (ret < 0) {
>>>> - dev_err(dev, "failed to set the clock of bus\n");
>>>> - goto out;
>>>> - }
>>>> -
>>>> - *freq = new_freq;
>>>> - bus->curr_freq = new_freq;
>>>> -
>>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>>> -out:
>>>> mutex_unlock(&bus->lock);
>>>>
>>>> return ret;
>>>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>> struct exynos_bus *bus)
>>>> {
>>>> struct device *dev = bus->dev;
>>>> - int i, ret, count, size;
>>>> -
>>>> - /* Get the regulator to provide each bus with the power */
>>>> - bus->regulator = devm_regulator_get(dev, "vdd");
>>>> - if (IS_ERR(bus->regulator)) {
>>>> - dev_err(dev, "failed to get VDD regulator\n");
>>>> - return PTR_ERR(bus->regulator);
>>>> - }
>>>> -
>>>> - ret = regulator_enable(bus->regulator);
>>>> - if (ret < 0) {
>>>> - dev_err(dev, "failed to enable VDD regulator\n");
>>>> - return ret;
>>>> - }
>>>> + int i, count, size;
>>>>
>>>> /*
>>>> * Get the devfreq-event devices to get the current utilization of
>>>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>> count = devfreq_event_get_edev_count(dev);
>>>> if (count < 0) {
>>>> dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>>> - ret = count;
>>>> - goto err_regulator;
>>>> + return count;
>>>> }
>>>> +
>>>> bus->edev_count = count;
>>>>
>>>> size = sizeof(*bus->edev) * count;
>>>> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>>> - if (!bus->edev) {
>>>> - ret = -ENOMEM;
>>>> - goto err_regulator;
>>>> - }
>>>> + if (!bus->edev)
>>>> + return -ENOMEM;
>>>>
>>>> for (i = 0; i < count; i++) {
>>>> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>>> - if (IS_ERR(bus->edev[i])) {
>>>> - ret = -EPROBE_DEFER;
>>>> - goto err_regulator;
>>>> - }
>>>> + if (IS_ERR(bus->edev[i]))
>>>> + return -EPROBE_DEFER;
>>>> }
>>>>
>>>> /*
>>>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>>> bus->ratio = DEFAULT_SATURATION_RATIO;
>>>>
>>>> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>>> - &bus->voltage_tolerance))
>>>> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>>> -
>>>> return 0;
>>>> -
>>>> -err_regulator:
>>>> - regulator_disable(bus->regulator);
>>>> -
>>>> - return ret;
>>>> }
>>>>
>>>> static int exynos_bus_parse_of(struct device_node *np,
>>>> - struct exynos_bus *bus)
>>>> + struct exynos_bus *bus, bool passive)
>>>> {
>>>> struct device *dev = bus->dev;
>>>> + struct opp_table *opp_table;
>>>> + const char *vdd = "vdd";
>>>> struct dev_pm_opp *opp;
>>>> unsigned long rate;
>>>> int ret;
>>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>> return ret;
>>>> }
>>>>
>>>> + if (!passive) {
>>>> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>> + if (IS_ERR(opp_table)) {
>>>> + ret = PTR_ERR(opp_table);
>>>> + dev_err(dev, "failed to set regulators %d\n", ret);
>>>> + goto err_clk;/
>>>> + }
>>>> +
>>>> + bus->opp_table = opp_table;
>>>> + }
>>>
>>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>>> because the regulator is only used by parent devfreq device.
>>
>> exynos_bus_parse_of() is called for all devfreq devices (including
>> parent) and (as you've noticed) the regulator should be enabled before
>> enabling clock (which is done in exynos_bus_parse_of()) so adding
>> extra argument to exynos_bus_parse_of() (like it is done currently in
>> the patch)
>
> I think that this patch has still the problem about call sequence
> between clock and regulator as following:
Yes, this should be fixed (though the wrong sequence between regulator
and clock handling is not introduced by the patchset itself and is present
in the original driver code).
> 273 ret = clk_prepare_enable(bus->clk);
> 274 if (ret < 0) {
> 275 dev_err(dev, "failed to get enable clock\n");
> 276 return ret;
> 277 }
> 278
> 279 if (!passive) {
> 280 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
> 281 if (IS_ERR(opp_table)) {
> 282 ret = PTR_ERR(opp_table);
> 283 dev_err(dev, "failed to set regulators %d\n", ret);
> 284 goto err_clk;
> 285 }
> 286
> 287 bus->opp_table = opp_table;
> 288 }
>
> makes it possible to do the setup correctly without the need
>> of merging both functions into one huge function (which would be more
>> difficult to follow than two simpler functions IMHO). Is that approach
>> acceptable or do you prefer one big function?
>
> Actually, I don't force to make one function for both
> exynos_bus_parse_of() and exynos_bus_parent_parse_of().
>
> If we just keep this code, dev_pm_opp_set_regulators()
> should be handled in exynos_bus_parent_parse_of()
> because only parent devfreq device controls the regulator.
Could your please explain rationale for this requirement (besides
function name)?
The patch adds 'bool passive' argument (which is set to false for
parent devfreq device and true for child devfreq device) to
exynos_bus_parse_of() (which is called for *all* devfreq devices
and is called before exynos_bus_parent_parse_of()) and there is
no hard requirement to call dev_pm_opp_set_regulators() in
exynos_bus_parent_parse_of() so after only changing the ordering
between regulator and clock handling the setup code should be
correct.
[ Please note that this patch moves parent/child detection before
exynos_bus_parse_of() call. ]
> In order to keep the two functions, maybe have to change
> the call the sequence between exynos_bus_parse_of() and
> exynos_bus_parent_parse_of().
Doesn't seem to be needed, care to explain it more?
> Once again, I don't force any fixed method. I want to fix them
> with correct way.
>
>>
>>>> +
>>>> /* Get the freq and voltage from OPP table to scale the bus freq */
>>>> ret = dev_pm_opp_of_add_table(dev);
>>>> if (ret < 0) {
>>>> dev_err(dev, "failed to get OPP table\n");
>>>> - goto err_clk;
>>>> + goto err_regulator;
>>>> }
>>>>
>>>> rate = clk_get_rate(bus->clk);
>>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>> ret = PTR_ERR(opp);
>>>> goto err_opp;
>>>> }
>>>> +
>>>> bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>> dev_pm_opp_put(opp);
>>>>
>>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>
>>>> err_opp:
>>>> dev_pm_opp_of_remove_table(dev);
>>>> +
>>>> +err_regulator:
>>>> + if (bus->opp_table) {
>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>> + bus->opp_table = NULL;
>>>> + }
>>>
>>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>>
>>>> +
>>>> err_clk:
>>>> clk_disable_unprepare(bus->clk);
>>>>
>>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>> struct exynos_bus *bus;
>>>> int ret, max_state;
>>>> unsigned long min_freq, max_freq;
>>>> + bool passive = false;
>>>>
>>>> if (!np) {
>>>> dev_err(dev, "failed to find devicetree node\n");
>>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>> bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>>> if (!bus)
>>>> return -ENOMEM;
>>>> +
>>>> mutex_init(&bus->lock);
>>>> bus->dev = &pdev->dev;
>>>> platform_set_drvdata(pdev, bus);
>>>> + node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>> + if (node) {
>>>> + of_node_put(node);
>>>> + passive = true;
>>>> + }
>>>>
>>>> /* Parse the device-tree to get the resource information */
>>>> - ret = exynos_bus_parse_of(np, bus);
>>>> + ret = exynos_bus_parse_of(np, bus, passive);
>>>> if (ret < 0)
>>>> return ret;
>>>>
>>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>> goto err;
>>>> }
>>>>
>>>> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>> - if (node) {
>>>> - of_node_put(node);
>>>> + if (passive)
>>>> goto passive;
>>>> - } else {
>>>> - ret = exynos_bus_parent_parse_of(np, bus);
>>>> - }
>>>> +
>>>> + ret = exynos_bus_parent_parse_of(np, bus);
>>>>
>>>
>>> Remove unneeded blank line.
>>>
>>>> if (ret < 0)
>>>> goto err;
>>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>
>>>> err:
>>>> dev_pm_opp_of_remove_table(dev);
>>>> + if (bus->opp_table) {
>>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>>> + bus->opp_table = NULL;
>>>> + }
>>>> +
>>>
>>> ditto.
>>> Have to disable regulator after disabling the clock
>>> to prevent the h/w fault.
>>>
>>> I think that you should call them with following sequence:
>>>
>>> clk_disable_unprepare(bus->clk);
>>> if (bus->opp_table)
>>> dev_pm_opp_put_regulators(bus->opp_table);
>>> dev_pm_opp_of_remove_table(dev);
>>>
>>>> clk_disable_unprepare(bus->clk);
>>>>
>>>> return ret;
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 11/13] arm64: dts: qcom: qcs404: Add CPR and populate OPP table
From: Niklas Cassel @ 2019-07-16 10:53 UTC (permalink / raw)
To: Viresh Kumar
Cc: Andy Gross, linux-arm-msm, jorge.ramirez-ortiz, sboyd, vireshk,
bjorn.andersson, ulf.hansson, Rob Herring, Mark Rutland,
devicetree, linux-kernel
In-Reply-To: <20190716103436.az5rdk6f3yoa3apz@vireshk-i7>
On Tue, Jul 16, 2019 at 04:04:36PM +0530, Viresh Kumar wrote:
> On 15-07-19, 15:24, Niklas Cassel wrote:
> > This was actually my initial thought when talking to you 6+ months ago.
> > However, the problem was that, from the CPR drivers' perspective, it
> > only sees the CPR OPP table.
> >
> >
> > So this is the order things are called,
> > from qcom-cpufreq-nvmem.c perspective:
> >
> > 1) dev_pm_opp_set_supported_hw()
> >
> > 2) dev_pm_opp_attach_genpd() ->
> > which results in
> > int cpr_pd_attach_dev(struct generic_pm_domain *domain,
> > struct device *dev)
> > being called.
> > This callback is inside the CPR driver, and here we have the
> > CPU's (genpd virtual) struct device, and this is where we would like to
> > know the opp-hz.
> > The problem here is that:
> > [ 3.114979] cpr_pd_attach_dev: dev_pm_opp_get_opp_count for dev: genpd:0:cpu0: -19
> > [ 3.119610] cpr_pd_attach_dev: dev_pm_opp_get_opp_count for dev: cpu0: 0
Here I cheated and simply used get_cpu_device(0).
Since I cheated, I used get_cpu_device(0) always,
so even when CPU1,CPU2,CPU3 is attached, dev_pm_opp_get_opp_count(cpu0) is
still 0.
I added a print in
[ 3.836533] cpr_set_performance: number of OPPs for dev: cpu0: 3
And there I can see that OPP count is 3, so it appears that with the
current code, we need to wait until cpufreq-dt.c:cpufreq_init()
has been called, maybe dev_pm_opp_of_cpumask_add_table() needs
to be called before dev_pm_opp_get_opp_count(cpu0) actually returns 3.
cpufreq_init() is called by platform_device_register_simple("cpufreq-dt", -1,
NULL, 0);
which is called after dev_pm_opp_attach_genpd().
What I don't understand is that dev_pm_opp_attach_genpd() actually returns
a OPP table. So why do we need to wait for dev_pm_opp_of_cpumask_add_table(),
before either dev_pm_opp_get_opp_count(cpu0) or
dev_pm_opp_get_opp_count(genpd_virtdev_for_cpu0) returns 3?
> > [ 3.126489] cpr_pd_attach_dev: dev_pm_opp_get_opp_count for dev: cpr@b018000: 3
> >
> > While we have the CPR OPP table in the attach callback, we don't
> > have the CPU OPP table, neither in the CPU struct device or the genpd virtual
> > struct device.
>
> If you can find CPU's physical number from the virtual device, then
> you can do get_cpu_device(X) and then life will be easy ?
>
> > Since we have called dev_pm_opp_attach_genpd(.., .., &virt_devs) which
> > attaches an OPP table to the CPU, I would have expected one of them to
> > be >= 0.
> > Especially since dev_name(virt_devs[0]) == genpd:0:cpu0
> >
> > I guess it should still be possible to parse the required-opps manually here,
> > by iterating the OF nodes, however, we won't be able to use the CPU's struct
> > opp_table (which is the nice representation of the OF nodes).
> >
> > Any suggestions?
>
> --
> viresh
^ permalink raw reply
* Re: [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core
From: Daniel Thompson @ 2019-07-16 10:50 UTC (permalink / raw)
To: Jean-Jacques Hiblot
Cc: jacek.anaszewski, pavel, robh+dt, mark.rutland, dmurphy,
linux-leds, linux-kernel, devicetree
In-Reply-To: <20190715155657.22976-3-jjhiblot@ti.com>
On Mon, Jul 15, 2019 at 05:56:57PM +0200, Jean-Jacques Hiblot wrote:
> A LED is usually powered by a voltage/current regulator. Let the LED core
This is almost certainly nitpicking but since there's enough other
review comments that you will have to respin anyway ;-)
Is an LED really "usually powered by a voltage/current regulator"? Some
LEDs have a software controlled power supply but I'm not sure it is
the usual case.
Likewise its a little confusing to be talking about LEDs with an
external current regulator since, although that is possible, it is also
one the main features provided by LED driver chips.
Daniel.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox