* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-18 20:26 UTC (permalink / raw)
To: Peter De Schrijver
Cc: Sowjanya Komatineni, sboyd, Michael Turquette, Joseph Lo,
thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <20190718194222.GH12715@pdeschrijver-desktop.Nvidia.com>
18.07.2019 22:42, Peter De Schrijver пишет:
> On Thu, Jul 18, 2019 at 02:44:56AM +0300, Dmitry Osipenko wrote:
>>>
>>> dependencies I am referring are dfll_ref, dfll_soc, and DVFS peripheral
>>> clocks which need to be restored prior to DFLL reinit.
>>
>> Okay, but that shouldn't be a problem if clock dependencies are set up
>> properly.
>>
>>>>> reverse list order during restore might not work as all other clocks are
>>>>> in proper order no with any ref clocks for plls getting restored prior
>>>>> to their clients
>>>> Why? The ref clocks should be registered first and be the roots for PLLs
>>>> and the rest. If it's not currently the case, then this need to be
>>>> fixed. You need to ensure that each clock is modeled properly. If some
>>>> child clock really depends on multiple parents, then the parents need to
>>>> in the correct order or CCF need to be taught about such
>>>> multi-dependencies.
>>>>
>>>> If some required feature is missed, then you have to implement it
>>>> properly and for all, that's how things are done in upstream. Sometimes
>>>> it's quite a lot of extra work that everyone are benefiting from in
>>>> the end.
>>>>
>>>> [snip]
>>>
>>> Yes, we should register ref/parents before their clients.
>>>
>>> cclk_g clk is registered last after all pll and peripheral clocks are
>>> registers during clock init.
>>>
>>> dfllCPU_out clk is registered later during dfll-fcpu driver probe and
>>> gets added to the clock list.
>>>
>>> Probably the issue seems to be not linking dfll_ref and dfll_soc
>>> dependencies for dfllCPU_out thru clock list.
>>>
>>> clk-dfll driver during dfll_init_clks gets ref_clk and soc_clk reference
>>> thru DT.
>
> The dfll does not have any parents. It has some clocks which are needed
> for the logic part of the dfll to function, but there's no parent clock
> as such unlike for peripheral clocks or PLLs where the parent is at
> least used as a reference. The I2C controller of the DFLL shares the
> lines with a normal I2C controller using some arbitration logic. That
> logic only works if the clock for the normal I2C controller is enabled.
> So you need probably 3 clocks enabled to initialize the dfll in that
> case. I don't think it makes sense to add complicated logic to the clock
> core to deal with this rather strange case. To me it makes more sense to
> use pmops and open code the sequence there.
It looks to me that dfllCPU is a PLL and dfll_ref is its reference
parent, while dfll_soc clocks the logic that dynamically reconfigures
dfllCPU in background. I see that PLLP is defined as a parent for
dfll_ref and dfll_soc in the code. Hence seems dfll_ref should be set as
a parent for dfllCPU, no?
Either way is good to me, given that DFLL will be disabled during
suspend. Resetting DFLL on DFLL's driver resume using PM ops should be
good. And then it also will be better to error out if DFLL is active
during suspend on the DFLL's driver suspend.
^ permalink raw reply
* [PATCH v1] dt-bindings: fec: explicitly mark deprecated properties
From: Sven Van Asbroeck @ 2019-07-18 20:14 UTC (permalink / raw)
To: Fugang Duan, Rob Herring, Mark Rutland
Cc: David S . Miller, netdev, devicetree, linux-kernel, Andrew Lunn,
Fabio Estevam, Lucas Stach
fec's gpio phy reset properties have been deprecated.
Update the dt-bindings documentation to explicitly mark
them as such, and provide a short description of the
recommended alternative.
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
.../devicetree/bindings/net/fsl-fec.txt | 30 +++++++++++--------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
index 2d41fb96ce0a..5b88fae0307d 100644
--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -7,18 +7,6 @@ Required properties:
- phy-mode : See ethernet.txt file in the same directory
Optional properties:
-- phy-reset-gpios : Should specify the gpio for phy reset
-- phy-reset-duration : Reset duration in milliseconds. Should present
- only if property "phy-reset-gpios" is available. Missing the property
- will have the duration be 1 millisecond. Numbers greater than 1000 are
- invalid and 1 millisecond will be used instead.
-- phy-reset-active-high : If present then the reset sequence using the GPIO
- specified in the "phy-reset-gpios" property is reversed (H=reset state,
- L=operation state).
-- phy-reset-post-delay : Post reset delay in milliseconds. If present then
- a delay of phy-reset-post-delay milliseconds will be observed after the
- phy-reset-gpios has been toggled. Can be omitted thus no delay is
- observed. Delay is in range of 1ms to 1000ms. Other delays are invalid.
- phy-supply : regulator that powers the Ethernet PHY.
- phy-handle : phandle to the PHY device connected to this device.
- fixed-link : Assume a fixed link. See fixed-link.txt in the same directory.
@@ -47,11 +35,27 @@ Optional properties:
For imx6sx, "int0" handles all 3 queues and ENET_MII. "pps" is for the pulse
per second interrupt associated with 1588 precision time protocol(PTP).
-
Optional subnodes:
- mdio : specifies the mdio bus in the FEC, used as a container for phy nodes
according to phy.txt in the same directory
+Deprecated optional properties:
+ To avoid these, create a phy node according to phy.txt in the same
+ directory, and point the fec's "phy-handle" property to it. Then use
+ the phy's reset binding, again described by phy.txt.
+- phy-reset-gpios : Should specify the gpio for phy reset
+- phy-reset-duration : Reset duration in milliseconds. Should present
+ only if property "phy-reset-gpios" is available. Missing the property
+ will have the duration be 1 millisecond. Numbers greater than 1000 are
+ invalid and 1 millisecond will be used instead.
+- phy-reset-active-high : If present then the reset sequence using the GPIO
+ specified in the "phy-reset-gpios" property is reversed (H=reset state,
+ L=operation state).
+- phy-reset-post-delay : Post reset delay in milliseconds. If present then
+ a delay of phy-reset-post-delay milliseconds will be observed after the
+ phy-reset-gpios has been toggled. Can be omitted thus no delay is
+ observed. Delay is in range of 1ms to 1000ms. Other delays are invalid.
+
Example:
ethernet@83fec000 {
--
2.17.1
^ permalink raw reply related
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-18 20:11 UTC (permalink / raw)
To: Sowjanya Komatineni, Peter De Schrijver
Cc: Joseph Lo, 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: <2274fccb-59d3-824c-cb97-55c23a4eaa75@nvidia.com>
18.07.2019 22:24, Sowjanya Komatineni пишет:
>
> On 7/18/19 12:18 PM, Peter De Schrijver wrote:
>> On Tue, Jul 16, 2019 at 09:43:16PM +0300, Dmitry Osipenko wrote:
>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher
>>>> rate
>>>> so switching to PLL_P during CPUFreq probe prior to dfll clock enable
>>>> should be safe.
>>> AFAIK, PLLX could run at ~200MHz. There is also a divided output of PLLP
>>> which CCLKG supports, the PLLP_OUT4.
>>>
>>> Probably, realistically, CPU is always running off a fast PLLX during
>>> boot, but I'm wondering what may happen on KEXEC. I guess ideally
>>> CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
>>> on a reboot, but likely that there are other clock-related problems as
>>> well that may break KEXEC and thus it is not very important at the
>>> moment.
>>>
>> If you turn off the DFLL, you have to be aware that the voltage margins
>> for DFLL use are lower than for PLL use. So you either need to be sure
>> to switch to a frequency below fmax @ Vmin or you program the boot
>> voltage and then you can use PLLX as setup by the bootloader. For OVR
>> regulators you can't program a voltage without the DFLL, so you have to
>> tristate the PWM output which will give you a hardwired boot voltage.
>>
>> Peter.
>
> Yes, we switch CPU to PLLP and then disable DFLL during suspend.
I'm wondering what happens to T124 on resume from suspend, given that it
switches CPU to PLLX [1]. I imagine that CPU voltage could be lower than
needed if suspend happened on DFLL. I'm also now vaguely recalling that
CPUFreq driver was disabled for T124 because of some problems.
Or maybe warmboot code is actually touching the voltage regulators?
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-tegra/sleep-tegra30.S#n389
That is also should be a problem for T30 if voltage scaling is happening
and I have some patches in works that are switching CPU to PLLP instead
of PLLX on suspend/resume.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Peter De Schrijver @ 2019-07-18 19:42 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Sowjanya Komatineni, sboyd, Michael Turquette, Joseph Lo,
thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pgaikwad, linux-clk,
linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
spatra, robh+dt, devicetree
In-Reply-To: <89f23878-d4b2-2305-03e5-8a3e781c2b02@gmail.com>
On Thu, Jul 18, 2019 at 02:44:56AM +0300, Dmitry Osipenko wrote:
> >
> > dependencies I am referring are dfll_ref, dfll_soc, and DVFS peripheral
> > clocks which need to be restored prior to DFLL reinit.
>
> Okay, but that shouldn't be a problem if clock dependencies are set up
> properly.
>
> >>> reverse list order during restore might not work as all other clocks are
> >>> in proper order no with any ref clocks for plls getting restored prior
> >>> to their clients
> >> Why? The ref clocks should be registered first and be the roots for PLLs
> >> and the rest. If it's not currently the case, then this need to be
> >> fixed. You need to ensure that each clock is modeled properly. If some
> >> child clock really depends on multiple parents, then the parents need to
> >> in the correct order or CCF need to be taught about such
> >> multi-dependencies.
> >>
> >> If some required feature is missed, then you have to implement it
> >> properly and for all, that's how things are done in upstream. Sometimes
> >> it's quite a lot of extra work that everyone are benefiting from in
> >> the end.
> >>
> >> [snip]
> >
> > Yes, we should register ref/parents before their clients.
> >
> > cclk_g clk is registered last after all pll and peripheral clocks are
> > registers during clock init.
> >
> > dfllCPU_out clk is registered later during dfll-fcpu driver probe and
> > gets added to the clock list.
> >
> > Probably the issue seems to be not linking dfll_ref and dfll_soc
> > dependencies for dfllCPU_out thru clock list.
> >
> > clk-dfll driver during dfll_init_clks gets ref_clk and soc_clk reference
> > thru DT.
The dfll does not have any parents. It has some clocks which are needed
for the logic part of the dfll to function, but there's no parent clock
as such unlike for peripheral clocks or PLLs where the parent is at
least used as a reference. The I2C controller of the DFLL shares the
lines with a normal I2C controller using some arbitration logic. That
logic only works if the clock for the normal I2C controller is enabled.
So you need probably 3 clocks enabled to initialize the dfll in that
case. I don't think it makes sense to add complicated logic to the clock
core to deal with this rather strange case. To me it makes more sense to
use pmops and open code the sequence there.
Peter.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-18 19:24 UTC (permalink / raw)
To: Peter De Schrijver, Dmitry Osipenko
Cc: Joseph Lo, 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: <20190718191820.GG12715@pdeschrijver-desktop.Nvidia.com>
On 7/18/19 12:18 PM, Peter De Schrijver wrote:
> On Tue, Jul 16, 2019 at 09:43:16PM +0300, Dmitry Osipenko wrote:
>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher rate
>>> so switching to PLL_P during CPUFreq probe prior to dfll clock enable
>>> should be safe.
>> AFAIK, PLLX could run at ~200MHz. There is also a divided output of PLLP
>> which CCLKG supports, the PLLP_OUT4.
>>
>> Probably, realistically, CPU is always running off a fast PLLX during
>> boot, but I'm wondering what may happen on KEXEC. I guess ideally
>> CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
>> on a reboot, but likely that there are other clock-related problems as
>> well that may break KEXEC and thus it is not very important at the moment.
>>
> If you turn off the DFLL, you have to be aware that the voltage margins
> for DFLL use are lower than for PLL use. So you either need to be sure
> to switch to a frequency below fmax @ Vmin or you program the boot
> voltage and then you can use PLLX as setup by the bootloader. For OVR
> regulators you can't program a voltage without the DFLL, so you have to
> tristate the PWM output which will give you a hardwired boot voltage.
>
> Peter.
Yes, we switch CPU to PLLP and then disable DFLL during suspend.
^ permalink raw reply
* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Brendan Higgins @ 2019-07-18 19:22 UTC (permalink / raw)
To: Stephen Boyd
Cc: Petr Mladek, open list:DOCUMENTATION, Peter Zijlstra,
Amir Goldstein, dri-devel, Sasha Levin, Masahiro Yamada,
Michael Ellerman, open list:KERNEL SELFTEST FRAMEWORK, shuah,
Rob Herring, linux-nvdimm, Frank Rowand, Knut Omang,
Kieran Bingham, wfg-VuQAYsv1563Yd54FQh9/CA, Joel Stanley,
David Rientjes, Jeff Dike, Dan Carpenter, devicetree,
linux-kbuild
In-Reply-To: <20190718175024.C3EC421019-+nuXSHJNwjE76Z2rM5mHXA@public.gmane.org>
On Thu, Jul 18, 2019 at 10:50 AM Stephen Boyd <sboyd-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> Quoting Brendan Higgins (2019-07-16 11:52:01)
> > On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > >
> >
> > > The only hypothetical case where this can't be done is a complicated
> > > assertion or expectation that does more than one check and can't be
> > > written as a function that dumps out what went wrong. Is this a real
> > > problem? Maybe such an assertion should just open code that logic so we
> > > don't have to build up a string for all the other simple cases.
> >
> > I have some expectations in follow up patchsets for which I created a
> > set of composable matchers for matching structures and function calls
> > that by their nature cannot be written as a single function. The
> > matcher thing is a bit speculative, I know, but for any kind of
> > function call matching, you need to store a record of functions you
> > are expecting to have called and then each one needs to have a set of
> > expectations defined by the user; I don't think there is a way to do
> > that that doesn't involve having multiple separate functions each
> > having some information useful to constructing the message.
> >
> > I know the code in question isn't in this patchset; the function
> > matching code was in one of the earlier versions of the RFC, but I
> > dropped it to make this patchset smaller and more manageable. So I get
> > it if you would like me to drop it and add it back in when I try to
> > get the function and structure matching stuff in, but I would really
> > prefer to keep it as is if you don't care too much.
>
> Do you have a link to those earlier patches?
This is the first patchset:
https://www.mail-archive.com/linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg1788057.html
In particular you can see the code for matching functions here:
https://www.mail-archive.com/linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg1788073.html
And parameter matching code here:
https://www.mail-archive.com/linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg1788072.html
https://www.mail-archive.com/linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg1788086.html
My apologies in advance, but the code at this early stage had not
adopted the kunit_* prefix and was still using the test_* and mock_*
prefix. (Hence, struct kunit_stream was known as struct test_stream).
> > > It seems far simpler to get rid of the string stream API and just have a
> > > struct for this.
> > >
> > > struct kunit_fail_msg {
> > > const char *line;
> > > const char *file;
> > > const char *func;
> > > const char *msg;
> > > };
> > >
> > > Then you can have the assertion macros create this on the stack (with
> > > another macro?).
> > >
> > > #define DEFINE_KUNIT_FAIL_MSG(name, _msg) \
> > > struct kunit_fail_msg name = { \
> > > .line = __LINE__, \
> > > .file = __FILE__, \
> > > .func = __func__, \
> > > .msg = _msg, \
> > > }
> > >
> > > I don't want to derail this whole series on this topic, but it seems
> > > like a bunch of code is there to construct this same set of information
> > > over and over again into a buffer a little bit at a time and then throw
> > > it away when nothing fails just because we may want to support the case
> > > where we have some unstructured data to inform the user about.
> >
> > Yeah, that's fair. I think there are a number of improvements to be
> > made with how the expectations are defined other than that, but I was
> > hoping I could do that after this patchset is merged. I just figured
> > with the kinds of things I would like to do, it would lead to a whole
> > new round of discussion.
> >
> > In either case, I think I would still like to use the `struct
> > kunit_stream` as part of the interface to share the failure message
> > with the test case runner code in test.c, at least eventually, so that
> > I only have to have one way to receive data from expectations, but I
> > think I can do that and still do what you suggest by just constructing
> > the kunit_stream at the end of expectations where it is feasible.
> >
> > All in all I agree with what you are saying, but I would rather do it
> > as a follow up possibly once we have some more code on the table. I
> > could just see this opening up a whole new can of worms where we
> > debate about exactly how expectations and assertions work for another
> > several months, only to rip it all out shortly there after. I know
> > that's how these things go, but that's my preference.
> >
> > I can do what you suggest if you feel strongly about it, but I would
> > prefer to hold off until later. It's your call.
> >
>
> The crux of my complaint is that the string stream API is too loosely
> defined to be usable. It allows tests to build up a string of
> unstructured information, but with certain calling constraints so we
> have to tread carefully. If there was more structure to the data that's
> being recorded then the test case runner could operate on the data
> without having to do string/stream operations, allocations, etc. This
> would make the assertion logic much more concrete and specific to kunit,
> instead of this small kunit wrapper that's been placed on top of string
> stream.
Yeah, I can see the point of wanting something that provides more
structure than the raw `struct kunit_stream` interface. In fact, it is
something I had already started working on, when I had determined it
would be a large effort to capture all the variations. I was further
put off from the idea when I had been asked to convert the KUnit
intermediate format from what I was using to TAP, because, as it is,
the current data printed out by KUnit doesn't contain all the data I
would like to put in it in a way that best takes advantage of the TAP
specification. One problematic area in particular: TAP already
provides a way to present a lot of the data I would like to export,
but it involves JSON serialization which was an idea that some of the
other reviewers understandably weren't too keen on. TAP also wants to
report data some time after it is available, which is generally not a
good idea for test debug information; you want to make it available as
soon as you can or you risk crashing with the data still inside.
Hence, I decided we could probably spend a good long while debating
how I present the information. So the idea of having a loose
definition seemed attractive to me in its own right since it would
likely conform to whatever we ended up deciding in the long run. Also,
all the better that it was what I already had and no one seemed to
mind too much.
The only constant I expect is that `struct kunit` will likely need to
take an abstract object with a `commit` method, or a `format` method
or whatever so it could control when data was going to be printed out
to the user. We will probably also use a string builder in there
somewhere.
> TL;DR: If we can get rid of the string stream API I'd view that as an
> improvement because building arbitrary strings in the kernel is complex,
> error prone and has calling context concerns.
True. No argument there.
> Is the intention that other code besides unit tests will use this string
> stream API to build up strings? Any targets in mind? This would be a
> good way to get the API merged upstream given that its 2019 and we
> haven't had such an API in the kernel so far.
Someone, (was it you?) asked about code sharing with a string builder
thingy that was used for creating structured human readable files, but
that seemed like a pretty massive undertaking.
Aside from that, no. I would kind of prefered that nobody used it for
anything else because I the issues you described.
Nevertheless, I think the debate over the usefulness of the
string_stream and kunit_stream are separate topics. Even if we made
kunit_stream more structured, I am pretty sure I would want to use
string_stream or some variation for constructing the message.
> An "object oriented" (strong quotes!) approach where kunit_fail_msg is
> the innermost struct in some assertion specific structure might work
> nicely and allow the test runner to call a generic 'format' function to
> print out the message based on the type of assertion/expectation it is.
> It probably would mean less code size too because the strings that are
> common will be in the common printing function instead of created twice,
> in the macros/code and then copied to the heap for the string stream.
>
> struct kunit_assert {
> const char *line;
> const char *file;
> const char *func;
> void (*format)(struct kunit_assert *assert);
> };
>
> struct kunit_comparison_assert {
> enum operator operator;
> const char *left;
> const char *right;
> struct kunit_assert assert;
> };
>
> struct kunit_bool_assert {
> const char *truth;
> const char *statement;
> struct kunit_assert assert;
> };
>
> void kunit_format_comparison(struct kunit_assert *assert)
> {
> struct kunit_comparison_assert *comp = container_of(assert, ...)
>
> kunit_printk(...)
> }
I started working on something similarish, but by the time I ended up
coming up with a parent object whose definition was loose enough to
satisfy all the properties required by the child classes it ended up
basically being the same as what I have now just with a more complex
hierarchy of message manipulation logic.
On the other hand, I didn't have the idea of doing the parent object
quite the way you did and that would clean up a lot of the duplicated
first line logic.
I would like to give it a try, but I am afraid I am going to get
sucked down a really deep rabbit hole.
> Maybe other people have opinions here on if you should do it now or
> later. Future coding is not a great argument because it's hard to
> predict the future. On the other hand, this patchset is in good shape to
Yeah, that's kind of why I am afraid to go down this road when I have
something that works now and I know works with the mocking stuff I
want to do.
I would like to try your suggestion, but I want to try to make it work
with my mocking patches before I commit to it because otherwise I am
just going to have to back it out in a follow up patchset.
> merge and I'd like to use it to write unit tests for code I maintain so
> I don't want to see this stall out. Sorry if I'm opening the can of
> worms you're talking about.
Don't be sorry. I agree with you that the kunit_stream stuff is not very pretty.
Shuah, have we missed the merge window for 5.3?
I saw you only sent one PR out so far for this release, and there
wasn't much in it; I imagine you are going to send at least one more?
I figure, if we still got time to try out your suggestion, Stephen, no
harm in trying.
Also if we missed it, then I have another couple months to play around with it.
What do you think?
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Peter De Schrijver @ 2019-07-18 19:18 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Sowjanya Komatineni, Joseph Lo, 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: <5b2945c5-fcb2-2ac0-2bf2-df869dc9c713@gmail.com>
On Tue, Jul 16, 2019 at 09:43:16PM +0300, Dmitry Osipenko wrote:
> > CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher rate
> > so switching to PLL_P during CPUFreq probe prior to dfll clock enable
> > should be safe.
>
> AFAIK, PLLX could run at ~200MHz. There is also a divided output of PLLP
> which CCLKG supports, the PLLP_OUT4.
>
> Probably, realistically, CPU is always running off a fast PLLX during
> boot, but I'm wondering what may happen on KEXEC. I guess ideally
> CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
> on a reboot, but likely that there are other clock-related problems as
> well that may break KEXEC and thus it is not very important at the moment.
>
If you turn off the DFLL, you have to be aware that the voltage margins
for DFLL use are lower than for PLL use. So you either need to be sure
to switch to a frequency below fmax @ Vmin or you program the boot
voltage and then you can use PLLX as setup by the bootloader. For OVR
regulators you can't program a voltage without the DFLL, so you have to
tristate the PWM output which will give you a hardwired boot voltage.
Peter.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Peter De Schrijver @ 2019-07-18 19:15 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Sowjanya Komatineni, Joseph Lo, 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: <76e341be-6f38-2bc1-048e-1aa6883f9b88@gmail.com>
On Tue, Jul 16, 2019 at 09:25:43PM +0300, Dmitry Osipenko wrote:
> 16.07.2019 21:19, Sowjanya Komatineni пишет:
> >
> > On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
> >>
> >> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
> >>> 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
> >>>
> >> Will go thru and add...
>
> Looks like I initially confused this case with getting orphaned clock.
> I'm now seeing that the DFLL driver registers the clock and then
> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
> probed, hence everything should be fine as-is and there is no real need
> for the 'device link'. Sorry for the confusion!
>
> >>>>> 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.
> >>
> >> PLLP freq is safe to work for any CPU voltage. So no need to enforce
> >> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
> >> suspend
> >>
> > Sorry, please ignore my above comment. During suspend, need to change
> > CCLK_G source to PLLP when dfll is in closed loop mode first and then
> > dfll need to be set to open loop.
>
> Okay.
>
> >>>>> 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
> >>>>
> > I see during switch away from DFLL (suspend), cclk_g parent is not
> > changed to PLLP before changing dfll to open loop mode.
> >
> > Will add this ...
>
> The CPUFreq driver switches parent to PLLP during the probe, similar
> should be done on suspend.
>
> I'm also wondering if it's always safe to switch to PLLP in the probe.
> If CPU is running on a lower freq than PLLP, then some other more
> appropriate intermediate parent should be selected.
>
Yes it is always safe because 408MHz is lower than fmax @ Vmin.
Peter.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-18 19:11 UTC (permalink / raw)
To: Dmitry Osipenko, sboyd, Michael Turquette
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <8bca130c-c95c-591e-2f6e-f02538f8f8b8@nvidia.com>
On 7/18/19 11:29 AM, Sowjanya Komatineni wrote:
>
> On 7/18/19 10:41 AM, Sowjanya Komatineni wrote:
>>
>> On 7/18/19 10:22 AM, Sowjanya Komatineni wrote:
>>>
>>> On 7/18/19 9:34 AM, Dmitry Osipenko wrote:
>>>> 18.07.2019 4:15, Sowjanya Komatineni пишет:
>>>>
>>>> [snip]
>>>>
>>>>>>> Please try to fix all missing dependencies and orderings.
>>>>>> Peter,
>>>>>>
>>>>>> dfllCPU_OUT is the first one to go thru restore when
>>>>>> clk_restore_context traverses thru the list.
>>>>>>
>>>>>> dfllCPU_OUT has dependency on DFLL_ref and DFLL_SOC but this
>>>>>> dependency is unknown to clock-tree.
>>>>>>
>>>>>> We can add DFLL_REF and DFLL_SOC as parents to dfllCPU_OUT during
>>>>>> register so dfllCPU_OUT save/restore happens after their parents are
>>>>>> restored.
>>>>>>
>>>>>> But DFLL needs both of these to be restored before DFLLCPU_Out
>>>>>> and as
>>>>>> DFLL_SOC restore always happens after the REF, thinking to add
>>>>>> DFLL_SOC as parent to dfllCPU_OUT so save/restore follows after
>>>>>> their
>>>>>> dependencies.
>>>>>>
>>>>>> Please comment.
>>>>>>
>>>>> Did quick try and I see by adding dfll-soc as parent to
>>>>> dfllCPU_OUT, its
>>>>> in proper order after all its dependencies.
>>>>>
>>>>> Can now add dfll save/restore to do dfll reinit during restore..
>>>>>
>>>> If dfllCPU_OUT can work properly with dfll-soc being disabled, then
>>>> this
>>>> kind of dependency isn't very correct and just papers over the real
>>>> problem, which is that there should be a way for CCF to specify
>>>> multiple
>>>> dependencies for the clock or the reverse ordering should be used for
>>>> the restoring.
>>>
>>> dfll will not work without dfll-soc enabled.
>>>
>>> CLDVFS control logic is split into 2 clock domains. dvfs_ref_clk and
>>> dvfs_soc_clk.
>>>
>>> Majority of the control logic is clocked from dvfs_soc_clk for
>>> interfacing control registers.
>>>
>> Note on reverse ordering for restore. Currently restore order goes
>> thru clock list and for each root goes thru parent -> child restore.
>>
>> this order is correct and also all clocks are parented properly so
>> they follow proper order.
>>
>> dfllCPU is the only one where current driver doesn't take care of
>> dependency in dfll_soc which gets enabled only after dfll_ref.
>>
>>
>> Based on dfllCPU control logic module design, dfll_ref and dfll_soc
>> should be enabled prior to dfll init/enable.
>>
>> So parenting dfll_soc to dfllCPU keeps proper order.
>>
>
> 1. With dfllCPU parenting to dfll_soc, its keeps it in expected order
> and we don't define any parent clk_ops anyway for this, so should be OK?
>
> OR
>
> 2. Any suggestion on how to define/specify dependencies for clock
> other than parenting to follow proper order in clock tree as
> clk_save_context and clk_restore_context strictly goes thru clock tree
> order and all other clocks are parented properly except for dfllCPU
> where there is no parent. Techinically dfll_ref & dfll_soc are not
> parents but they need to be configured prior to dfll reinit.
>
> OR
>
> 3. I don't see way to override clk_save_context/clk_restore_context
> APIs to change the way of traversal so I can modify to traverse in
> expected order without dfllCPU parenting.
instead of using core API of save/restore context, probably can change
traversing to skip the 1st root in clock tree during initial traversing
and at the end invoke restore for 1st node.
>
> OR
>
> 4. dfll re-init can be done in dfll-fcpu driver pm_ops which actually
> registers dfll or at the end of tegra210_clock resume
>
Please suggest if you agree with either 1/3/4.
^ permalink raw reply
* Re: [PATCH 3/3] dt-bindings: dsp: fsl: Add DSP core binding support
From: Leonard Crestez @ 2019-07-18 18:40 UTC (permalink / raw)
To: Daniel Baluta
Cc: Rob Herring, Daniel Baluta, Shawn Guo, Mark Rutland, Sascha Hauer,
Sascha Hauer, Fabio Estevam, dl-linux-imx, S.j. Wang, Paul Olaru,
Aisheng Dong, Anson Huang, Peng Fan, Frank Li, Devicetree List,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel@vger.kernel.org,
"sound-open-firmware@alsa-project.org" <sound-open-fi>
In-Reply-To: <CAEnQRZBubFz90Xf8irDwc=erTXmByXX4rkzZy9r8ymfAuQEsZA@mail.gmail.com>
On 18.07.2019 21:24, Daniel Baluta wrote:
> On Thu, Jul 18, 2019 at 7:41 PM Rob Herring <robh+dt@kernel.org> wrote:
>>
>> On Thu, Jul 18, 2019 at 9:13 AM Daniel Baluta <daniel.baluta@nxp.com> wrote:
>>>
>>> This describes the DSP device tree node.
>>>
>>> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>>> + power-domains:
>>> + description:
>>> + List of phandle and PM domain specifier as documented in
>>> + Documentation/devicetree/bindings/power/power_domain.txt
>>
>> How many? 4?
>
> Yes, 4 for i.MX8QXP. Also, the same number is for i.MX8QM. Anyhow, I didn't
> added added a limit here because I really don't know how many will be
> in upcoming i.MX platforms.
Which 4? It might help to use power-domain-names explicitly just like
it's done for clocks and mboxes.
This is very common for phandle lists.
--
Regards,
Leonard
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-18 18:29 UTC (permalink / raw)
To: Dmitry Osipenko, sboyd, Michael Turquette
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <419e1b16-683e-1b56-7334-50d87368c1b9@nvidia.com>
On 7/18/19 10:41 AM, Sowjanya Komatineni wrote:
>
> On 7/18/19 10:22 AM, Sowjanya Komatineni wrote:
>>
>> On 7/18/19 9:34 AM, Dmitry Osipenko wrote:
>>> 18.07.2019 4:15, Sowjanya Komatineni пишет:
>>>
>>> [snip]
>>>
>>>>>> Please try to fix all missing dependencies and orderings.
>>>>> Peter,
>>>>>
>>>>> dfllCPU_OUT is the first one to go thru restore when
>>>>> clk_restore_context traverses thru the list.
>>>>>
>>>>> dfllCPU_OUT has dependency on DFLL_ref and DFLL_SOC but this
>>>>> dependency is unknown to clock-tree.
>>>>>
>>>>> We can add DFLL_REF and DFLL_SOC as parents to dfllCPU_OUT during
>>>>> register so dfllCPU_OUT save/restore happens after their parents are
>>>>> restored.
>>>>>
>>>>> But DFLL needs both of these to be restored before DFLLCPU_Out and as
>>>>> DFLL_SOC restore always happens after the REF, thinking to add
>>>>> DFLL_SOC as parent to dfllCPU_OUT so save/restore follows after their
>>>>> dependencies.
>>>>>
>>>>> Please comment.
>>>>>
>>>> Did quick try and I see by adding dfll-soc as parent to
>>>> dfllCPU_OUT, its
>>>> in proper order after all its dependencies.
>>>>
>>>> Can now add dfll save/restore to do dfll reinit during restore..
>>>>
>>> If dfllCPU_OUT can work properly with dfll-soc being disabled, then
>>> this
>>> kind of dependency isn't very correct and just papers over the real
>>> problem, which is that there should be a way for CCF to specify
>>> multiple
>>> dependencies for the clock or the reverse ordering should be used for
>>> the restoring.
>>
>> dfll will not work without dfll-soc enabled.
>>
>> CLDVFS control logic is split into 2 clock domains. dvfs_ref_clk and
>> dvfs_soc_clk.
>>
>> Majority of the control logic is clocked from dvfs_soc_clk for
>> interfacing control registers.
>>
> Note on reverse ordering for restore. Currently restore order goes
> thru clock list and for each root goes thru parent -> child restore.
>
> this order is correct and also all clocks are parented properly so
> they follow proper order.
>
> dfllCPU is the only one where current driver doesn't take care of
> dependency in dfll_soc which gets enabled only after dfll_ref.
>
>
> Based on dfllCPU control logic module design, dfll_ref and dfll_soc
> should be enabled prior to dfll init/enable.
>
> So parenting dfll_soc to dfllCPU keeps proper order.
>
1. With dfllCPU parenting to dfll_soc, its keeps it in expected order
and we don't define any parent clk_ops anyway for this, so should be OK?
OR
2. Any suggestion on how to define/specify dependencies for clock other
than parenting to follow proper order in clock tree as clk_save_context
and clk_restore_context strictly goes thru clock tree order and all
other clocks are parented properly except for dfllCPU where there is no
parent. Techinically dfll_ref & dfll_soc are not parents but they need
to be configured prior to dfll reinit.
OR
3. I don't see way to override clk_save_context/clk_restore_context APIs
to change the way of traversal so I can modify to traverse in expected
order without dfllCPU parenting.
OR
4. dfll re-init can be done in dfll-fcpu driver pm_ops which actually
registers dfll or at the end of tegra210_clock resume
^ permalink raw reply
* Re: [PATCH 3/3] dt-bindings: dsp: fsl: Add DSP core binding support
From: Daniel Baluta @ 2019-07-18 18:24 UTC (permalink / raw)
To: Rob Herring
Cc: Daniel Baluta, Shawn Guo, Mark Rutland, Sascha Hauer,
Sascha Hauer, Fabio Estevam, NXP Linux Team, S.j. Wang,
paul.olaru, Dong Aisheng, Leonard Crestez, Anson Huang, Peng Fan,
Frank Li, Devicetree List,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel@vger.kernel.org, sound-open-firmware
In-Reply-To: <CAL_JsqJ6o9mTjLYjnfcYgfSFKb95W8FseZBBb8RLosB__GNBcw@mail.gmail.com>
On Thu, Jul 18, 2019 at 7:41 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Jul 18, 2019 at 9:13 AM Daniel Baluta <daniel.baluta@nxp.com> wrote:
> >
> > This describes the DSP device tree node.
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> > .../devicetree/bindings/dsp/fsl,dsp.yaml | 87 +++++++++++++++++++
> > 1 file changed, 87 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > new file mode 100644
> > index 000000000000..d112486eda0e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > @@ -0,0 +1,87 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/freescale/fsl,dsp.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP i.MX8 DSP core
> > +
> > +maintainers:
> > + - Daniel Baluta <daniel.baluta@nxp.com>
> > +
> > +description: |
> > + Some boards from i.MX8 family contain a DSP core used for
> > + advanced pre- and post- audio processing.
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - fsl,imx8qxp-dsp
> > +
> > + reg:
> > + description: Should contain register location and length
> > +
> > + clocks:
> > + items:
> > + - description: ipg clock
> > + - description: ocram clock
> > + - description: core clock
> > +
> > + clock-names:
> > + items:
> > + - const: ipg
> > + - const: ocram
> > + - const: core
> > +
> > + power-domains:
> > + description:
> > + List of phandle and PM domain specifier as documented in
> > + Documentation/devicetree/bindings/power/power_domain.txt
>
> How many? 4?
Yes, 4 for i.MX8QXP. Also, the same number is for i.MX8QM. Anyhow, I didn't
added added a limit here because I really don't know how many will be
in upcoming
i.MX platforms.
>
> > +
> > + mboxes:
> > + description:
> > + List of <&phandle type channel> - 2 channels for TXDB, 2 channels for RXDB
> > + (see mailbox/fsl,mu.txt)
> > + maxItems: 4
> > +
> > + mbox-names:
> > + items:
> > + - const: txdb0
> > + - const: txdb1
> > + - const: rxdb0
> > + - const: rxdb1
> > +
> > + memory-region:
> > + description:
> > + phandle to a node describing reserved memory (System RAM memory)
> > + used by DSP (see bindings/reserved-memory/reserved-memory.txt)
> > + maxItems: 1
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > + - clock-names
> > + - power-domains
> > + - mboxes
> > + - mbox-names
> > + - memory-region
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/firmware/imx/rsrc.h>
> > + #include <dt-bindings/clock/imx8-clock.h>
> > + dsp@596e8000 {
> > + compatbile = "fsl,imx8qxp-dsp";
> > + reg = <0x596e8000 0x88000>;
> > + clocks = <&adma_lpcg IMX_ADMA_LPCG_DSP_IPG_CLK>,
> > + <&adma_lpcg IMX_ADMA_LPCG_OCRAM_IPG_CLK>,
> > + <&adma_lpcg IMX_ADMA_LPCG_DSP_CORE_CLK>;
> > + clock-names = "ipg", "ocram", "core";
> > + power-domains = <&pd IMX_SC_R_MU_13A>,
> > + <&pd IMX_SC_R_MU_13B>,
> > + <&pd IMX_SC_R_DSP>,
> > + <&pd IMX_SC_R_DSP_RAM>;
> > + mbox-names = "txdb0", "txdb1", "rxdb0", "rxdb1";
> > + mboxes = <&lsio_mu13 2 0>, <&lsio_mu13 2 1>, <&lsio_mu13 3 0>, <&lsio_mu13 3 1>;
> > + };
> > --
> > 2.17.1
> >
^ permalink raw reply
* Re: [PATCH v2 3/5] memremap: Add support for read-only memory mappings
From: Stephen Boyd @ 2019-07-18 18:00 UTC (permalink / raw)
To: Will Deacon
Cc: Dan Williams, linux-kernel, linux-arm-msm, linux-arm-kernel,
devicetree, Evan Green, Rob Herring, Bjorn Andersson, Andy Gross,
Will Deacon, Catalin Marinas
In-Reply-To: <20190710141433.7ama3gncss3y6dcx@willie-the-truck>
Quoting Will Deacon (2019-07-10 07:14:34)
> On Fri, Jun 14, 2019 at 01:37:15PM -0700, Stephen Boyd wrote:
> > @@ -84,7 +91,10 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags)
> > }
> >
> > /* Try all mapping types requested until one returns non-NULL */
> > - if (flags & MEMREMAP_WB) {
> > + if ((flags & MEMREMAP_RO) && is_ram != REGION_INTERSECTS)
> > + addr = arch_memremap_ro(offset, size);
> > +
> > + if (!addr && (flags & MEMREMAP_WB)) {
> > /*
> > * MEMREMAP_WB is special in that it can be satisfied
> > * from the direct map. Some archs depend on the
> > @@ -103,7 +113,8 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags)
> > * address mapping. Enforce that this mapping is not aliasing
> > * System RAM.
> > */
> > - if (!addr && is_ram == REGION_INTERSECTS && flags != MEMREMAP_WB) {
> > + if (!addr && is_ram == REGION_INTERSECTS &&
> > + (flags != MEMREMAP_WB || flags != MEMREMAP_RO)) {
> > WARN_ONCE(1, "memremap attempted on ram %pa size: %#lx\n",
> > &offset, (unsigned long) size);
> > return NULL;
>
> This function seems a little confused about whether 'flags' is really a
> bitmap of flags, or whether it is equal to exactly one entry in the enum.
> Given that I think it's sensible for somebody to specify 'MEMREMAP_RO |
> MEMREMAP_WT', then we probably need to start checking these things a bit
> more thoroughly so we can reject unsupported combinations at the very least.
>
I'm also confused about the same thing. I thought it was a "getting
worse via best effort" type of thing based on the comment above the
function.
* In the case of multiple flags, the different
* mapping types will be attempted in the order listed below until one of
* them succeeds.
(I now realize I should have documented the new flag so that this order
would be known. I'll resend this series again with the documentation
fix.)
I also thought that the combination of read-only and write through would
be OK because the flags are more of a best effort approach to making a
mapping. Given that, is there anything to reject? Or do we just keep
trying until we can't try anymore?
^ permalink raw reply
* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Stephen Boyd @ 2019-07-18 17:50 UTC (permalink / raw)
To: Brendan Higgins
Cc: Petr Mladek, open list:DOCUMENTATION, Peter Zijlstra,
Amir Goldstein, dri-devel, Sasha Levin, Masahiro Yamada,
Michael Ellerman, open list:KERNEL SELFTEST FRAMEWORK, shuah,
linux-nvdimm, Frank Rowand, Knut Omang, Kieran Bingham, wfg,
Joel Stanley, David Rientjes, Jeff Dike, Dan Carpenter,
devicetree, linux-kbuild,
"Bird, Timothy" <Tim.Bird>
In-Reply-To: <CAFd5g453vXeSUCZenCk_CzJ-8a1ym9RaPo0NVF=FujF9ac-5Ag@mail.gmail.com>
Quoting Brendan Higgins (2019-07-16 11:52:01)
> On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
> >
>
> > The only hypothetical case where this can't be done is a complicated
> > assertion or expectation that does more than one check and can't be
> > written as a function that dumps out what went wrong. Is this a real
> > problem? Maybe such an assertion should just open code that logic so we
> > don't have to build up a string for all the other simple cases.
>
> I have some expectations in follow up patchsets for which I created a
> set of composable matchers for matching structures and function calls
> that by their nature cannot be written as a single function. The
> matcher thing is a bit speculative, I know, but for any kind of
> function call matching, you need to store a record of functions you
> are expecting to have called and then each one needs to have a set of
> expectations defined by the user; I don't think there is a way to do
> that that doesn't involve having multiple separate functions each
> having some information useful to constructing the message.
>
> I know the code in question isn't in this patchset; the function
> matching code was in one of the earlier versions of the RFC, but I
> dropped it to make this patchset smaller and more manageable. So I get
> it if you would like me to drop it and add it back in when I try to
> get the function and structure matching stuff in, but I would really
> prefer to keep it as is if you don't care too much.
Do you have a link to those earlier patches?
>
> > It seems far simpler to get rid of the string stream API and just have a
> > struct for this.
> >
> > struct kunit_fail_msg {
> > const char *line;
> > const char *file;
> > const char *func;
> > const char *msg;
> > };
> >
> > Then you can have the assertion macros create this on the stack (with
> > another macro?).
> >
> > #define DEFINE_KUNIT_FAIL_MSG(name, _msg) \
> > struct kunit_fail_msg name = { \
> > .line = __LINE__, \
> > .file = __FILE__, \
> > .func = __func__, \
> > .msg = _msg, \
> > }
> >
> > I don't want to derail this whole series on this topic, but it seems
> > like a bunch of code is there to construct this same set of information
> > over and over again into a buffer a little bit at a time and then throw
> > it away when nothing fails just because we may want to support the case
> > where we have some unstructured data to inform the user about.
>
> Yeah, that's fair. I think there are a number of improvements to be
> made with how the expectations are defined other than that, but I was
> hoping I could do that after this patchset is merged. I just figured
> with the kinds of things I would like to do, it would lead to a whole
> new round of discussion.
>
> In either case, I think I would still like to use the `struct
> kunit_stream` as part of the interface to share the failure message
> with the test case runner code in test.c, at least eventually, so that
> I only have to have one way to receive data from expectations, but I
> think I can do that and still do what you suggest by just constructing
> the kunit_stream at the end of expectations where it is feasible.
>
> All in all I agree with what you are saying, but I would rather do it
> as a follow up possibly once we have some more code on the table. I
> could just see this opening up a whole new can of worms where we
> debate about exactly how expectations and assertions work for another
> several months, only to rip it all out shortly there after. I know
> that's how these things go, but that's my preference.
>
> I can do what you suggest if you feel strongly about it, but I would
> prefer to hold off until later. It's your call.
>
The crux of my complaint is that the string stream API is too loosely
defined to be usable. It allows tests to build up a string of
unstructured information, but with certain calling constraints so we
have to tread carefully. If there was more structure to the data that's
being recorded then the test case runner could operate on the data
without having to do string/stream operations, allocations, etc. This
would make the assertion logic much more concrete and specific to kunit,
instead of this small kunit wrapper that's been placed on top of string
stream.
TL;DR: If we can get rid of the string stream API I'd view that as an
improvement because building arbitrary strings in the kernel is complex,
error prone and has calling context concerns.
Is the intention that other code besides unit tests will use this string
stream API to build up strings? Any targets in mind? This would be a
good way to get the API merged upstream given that its 2019 and we
haven't had such an API in the kernel so far.
An "object oriented" (strong quotes!) approach where kunit_fail_msg is
the innermost struct in some assertion specific structure might work
nicely and allow the test runner to call a generic 'format' function to
print out the message based on the type of assertion/expectation it is.
It probably would mean less code size too because the strings that are
common will be in the common printing function instead of created twice,
in the macros/code and then copied to the heap for the string stream.
struct kunit_assert {
const char *line;
const char *file;
const char *func;
void (*format)(struct kunit_assert *assert);
};
struct kunit_comparison_assert {
enum operator operator;
const char *left;
const char *right;
struct kunit_assert assert;
};
struct kunit_bool_assert {
const char *truth;
const char *statement;
struct kunit_assert assert;
};
void kunit_format_comparison(struct kunit_assert *assert)
{
struct kunit_comparison_assert *comp = container_of(assert, ...)
kunit_printk(...)
}
Maybe other people have opinions here on if you should do it now or
later. Future coding is not a great argument because it's hard to
predict the future. On the other hand, this patchset is in good shape to
merge and I'd like to use it to write unit tests for code I maintain so
I don't want to see this stall out. Sorry if I'm opening the can of
worms you're talking about.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core
From: Jacek Anaszewski @ 2019-07-18 17:49 UTC (permalink / raw)
To: Jean-Jacques Hiblot, pavel, robh+dt, mark.rutland,
daniel.thompson
Cc: dmurphy, linux-leds, linux-kernel, devicetree
In-Reply-To: <49152281-059c-6006-4c0f-a6be96a12707@ti.com>
On 7/18/19 3:31 PM, Jean-Jacques Hiblot wrote:
>
> On 18/07/2019 14:24, Jacek Anaszewski wrote:
>> Hi Jean,
>>
>> Thank you for the updated patch set.
>>
>> I have some more comments below.
>>
>> On 7/17/19 3:59 PM, Jean-Jacques Hiblot wrote:
>>> +static bool __led_need_regulator_update(struct led_classdev
>>> *led_cdev,
>>> + int brightness)
>>> +{
>>> + bool new_state = (brightness != LED_OFF);
>> How about:
>>
>> bool new_state = !!brightness;
>
> Throughout the code LED_OFF is used when the LED is turned off. I think
> it would be more consistent to use it there too.
Basically brightness is a scalar and 0 always means off.
We treat enum led_brightness as a legacy type - it is no
longer valid on the whole its span since LED_FULL = 255
was depreciated with addition of max_brightness property.
IMHO use of reverse logic here only hinders code analysis.
>>> +
>>> + return led_cdev->regulator && led_cdev->regulator_state !=
>>> new_state;
>>> +}
>>> +static int __led_handle_regulator(struct led_classdev *led_cdev,
>>> + int brightness)
>>> +{
>>> + int rc;
>>> +
>>> + if (__led_need_regulator_update(led_cdev, brightness)) {
>>> +
>>> + if (brightness != LED_OFF)
>>> + rc = regulator_enable(led_cdev->regulator);
>>> + else
>>> + rc = regulator_disable(led_cdev->regulator);
>>> + if (rc)
>>> + return rc;
>>> +
>>> + led_cdev->regulator_state = (brightness != LED_OFF);
>>> + }
>>> + return 0;
>>> +}
>> Let's have these function names without leading underscores.
> OK.
>>
>>> static int __led_set_brightness(struct led_classdev *led_cdev,
>>> enum led_brightness value)
>>> {
>>> @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct
>>> work_struct *ws)
>>> if (ret == -ENOTSUPP)
>>> ret = __led_set_brightness_blocking(led_cdev,
>>> led_cdev->delayed_set_value);
>>> + __led_handle_regulator(led_cdev, led_cdev->delayed_set_value)
>> If you called it from __led_set_brightness() and
>
> We cannot call it from __led_set_brightness() because it is supposed not
> to block.
You're right. The problematic part is that with regulator handling
we cannot treat the whole brightness setting operation uniformly
for brightness_set op case, i.e. without mediation of a workqueue.
Now you have to fire workqueue in led_set_brightness_nopm()
even for brightness_set() op path, if regulator state needs update.
This is ugly and can be misleading. Can be also error prone and
have non-obvious implications for software blink state transitions.
I think we would first need to improve locking between the workqueue
and led_timer_function(). I proposed a patch [0] over a year
ago.
Only then we could think of adding another asynchronous dependency
to the brightness setting chain.
[0] https://lkml.org/lkml/2018/1/17/1144
--
Best regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-18 17:41 UTC (permalink / raw)
To: Dmitry Osipenko, sboyd, Michael Turquette
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <ab8f2441-8f4b-3a2b-5bcd-1a889555176a@nvidia.com>
On 7/18/19 10:22 AM, Sowjanya Komatineni wrote:
>
> On 7/18/19 9:34 AM, Dmitry Osipenko wrote:
>> 18.07.2019 4:15, Sowjanya Komatineni пишет:
>>
>> [snip]
>>
>>>>> Please try to fix all missing dependencies and orderings.
>>>> Peter,
>>>>
>>>> dfllCPU_OUT is the first one to go thru restore when
>>>> clk_restore_context traverses thru the list.
>>>>
>>>> dfllCPU_OUT has dependency on DFLL_ref and DFLL_SOC but this
>>>> dependency is unknown to clock-tree.
>>>>
>>>> We can add DFLL_REF and DFLL_SOC as parents to dfllCPU_OUT during
>>>> register so dfllCPU_OUT save/restore happens after their parents are
>>>> restored.
>>>>
>>>> But DFLL needs both of these to be restored before DFLLCPU_Out and as
>>>> DFLL_SOC restore always happens after the REF, thinking to add
>>>> DFLL_SOC as parent to dfllCPU_OUT so save/restore follows after their
>>>> dependencies.
>>>>
>>>> Please comment.
>>>>
>>> Did quick try and I see by adding dfll-soc as parent to dfllCPU_OUT,
>>> its
>>> in proper order after all its dependencies.
>>>
>>> Can now add dfll save/restore to do dfll reinit during restore..
>>>
>> If dfllCPU_OUT can work properly with dfll-soc being disabled, then this
>> kind of dependency isn't very correct and just papers over the real
>> problem, which is that there should be a way for CCF to specify multiple
>> dependencies for the clock or the reverse ordering should be used for
>> the restoring.
>
> dfll will not work without dfll-soc enabled.
>
> CLDVFS control logic is split into 2 clock domains. dvfs_ref_clk and
> dvfs_soc_clk.
>
> Majority of the control logic is clocked from dvfs_soc_clk for
> interfacing control registers.
>
Note on reverse ordering for restore. Currently restore order goes thru
clock list and for each root goes thru parent -> child restore.
this order is correct and also all clocks are parented properly so they
follow proper order.
dfllCPU is the only one where current driver doesn't take care of
dependency in dfll_soc which gets enabled only after dfll_ref.
Based on dfllCPU control logic module design, dfll_ref and dfll_soc
should be enabled prior to dfll init/enable.
So parenting dfll_soc to dfllCPU keeps proper order.
^ permalink raw reply
* Re: [PATCH v3 1/6] dt-bindings: opp: Introduce opp-peak-KBps and opp-avg-KBps bindings
From: Saravana Kannan @ 2019-07-18 17:26 UTC (permalink / raw)
To: Viresh Kumar
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Vincent Guittot,
Sweeney, Sean, daidavid1, Rajendra Nayak, Sibi Sankar,
Bjorn Andersson, Evan Green, Android Kernel Team, Linux PM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
In-Reply-To: <20190718043558.roi4j6jw5n4zkwky@vireshk-i7>
On Wed, Jul 17, 2019 at 9:36 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 17-07-19, 13:29, Saravana Kannan wrote:
> > On Wed, Jul 17, 2019 at 12:54 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 02-07-19, 18:10, Saravana Kannan wrote:
> > > > Interconnects often quantify their performance points in terms of
> > > > bandwidth. So, add opp-peak-KBps (required) and opp-avg-KBps (optional) to
> > > > allow specifying Bandwidth OPP tables in DT.
> > > >
> > > > opp-peak-KBps is a required property that replace opp-hz for Bandwidth OPP
> > > > tables.
> > > >
> > > > opp-avg-KBps is an optional property that can be used in Bandwidth OPP
> > > > tables.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > > Documentation/devicetree/bindings/opp/opp.txt | 15 ++++++++++++---
> > > > 1 file changed, 12 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> > > > index 76b6c79604a5..c869e87caa2a 100644
> > > > --- a/Documentation/devicetree/bindings/opp/opp.txt
> > > > +++ b/Documentation/devicetree/bindings/opp/opp.txt
> > > > @@ -83,9 +83,14 @@ properties.
> > > >
> > > > Required properties:
> > > > - opp-hz: Frequency in Hz, expressed as a 64-bit big-endian integer. This is a
> > > > - required property for all device nodes but devices like power domains. The
> > > > - power domain nodes must have another (implementation dependent) property which
> > > > - uniquely identifies the OPP nodes.
> > > > + required property for all device nodes but for devices like power domains or
> > > > + bandwidth opp tables. The power domain nodes must have another (implementation
> > > > + dependent) property which uniquely identifies the OPP nodes. The interconnect
> > > > + opps are required to have the opp-peak-bw property.
> > >
> > > ??
> >
> > Sorry, what's the question? Was this an accidental email?
>
> Too much smartness is too bad sometimes, sorry about that :)
>
> I placed the ?? right below "opp-peak-bw", there is no property like
> that. You failed to update it :)
Ah, "typo". I'll fix it.
-Saravana
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-18 17:22 UTC (permalink / raw)
To: Dmitry Osipenko, sboyd, Michael Turquette
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <4141181d-7162-0321-71b6-33abf11f631c@gmail.com>
On 7/18/19 9:34 AM, Dmitry Osipenko wrote:
> 18.07.2019 4:15, Sowjanya Komatineni пишет:
>
> [snip]
>
>>>> Please try to fix all missing dependencies and orderings.
>>> Peter,
>>>
>>> dfllCPU_OUT is the first one to go thru restore when
>>> clk_restore_context traverses thru the list.
>>>
>>> dfllCPU_OUT has dependency on DFLL_ref and DFLL_SOC but this
>>> dependency is unknown to clock-tree.
>>>
>>> We can add DFLL_REF and DFLL_SOC as parents to dfllCPU_OUT during
>>> register so dfllCPU_OUT save/restore happens after their parents are
>>> restored.
>>>
>>> But DFLL needs both of these to be restored before DFLLCPU_Out and as
>>> DFLL_SOC restore always happens after the REF, thinking to add
>>> DFLL_SOC as parent to dfllCPU_OUT so save/restore follows after their
>>> dependencies.
>>>
>>> Please comment.
>>>
>> Did quick try and I see by adding dfll-soc as parent to dfllCPU_OUT, its
>> in proper order after all its dependencies.
>>
>> Can now add dfll save/restore to do dfll reinit during restore..
>>
> If dfllCPU_OUT can work properly with dfll-soc being disabled, then this
> kind of dependency isn't very correct and just papers over the real
> problem, which is that there should be a way for CCF to specify multiple
> dependencies for the clock or the reverse ordering should be used for
> the restoring.
dfll will not work without dfll-soc enabled.
CLDVFS control logic is split into 2 clock domains. dvfs_ref_clk and
dvfs_soc_clk.
Majority of the control logic is clocked from dvfs_soc_clk for
interfacing control registers.
^ permalink raw reply
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp
From: Alexander Steffen @ 2019-07-18 17:10 UTC (permalink / raw)
To: Eyal.Cohen, jarkko.sakkinen, tmaimon77
Cc: oshrialkoby85, robh+dt, mark.rutland, peterhuewe, jgg, arnd,
gregkh, oshri.alkoby, devicetree, linux-kernel, linux-integrity,
gcwilson, kgoldman, nayna, Dan.Morav, oren.tanami
In-Reply-To: <9c8e216dbc4f43dbaa1701dc166b05e0@NTILML02.nuvoton.com>
On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote:
> Hi Jarkko and Alexander,
>
> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer.
Great :) In the meantime, I've done some experiments creating an I2C
driver based on tpm_tis_core, see
https://patchwork.kernel.org/patch/11049363/ Please have a look at that
and provide your feedback (and/or use it as a basis for further
implementations).
> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core.
> At a minimum we thought of:
> 1. Handling TPM Localities in I2C is different
It turned out not to be that different in the end, see the code
mentioned above and my comment here:
https://patchwork.kernel.org/cover/11049365/
> 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core
That is completely optional, so there is no need to implement it in the
beginning. Also, do you expect a huge benefit from that functionality?
Are bit flips that much more likely on I2C compared to SPI, which has no
CRC at all, but still works fine?
> 3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors
Right, that seems similar to the cr50 issues
(https://lkml.org/lkml/2019/7/17/677), so there should probably be a
similar way to do it.
> 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry)
Optimizations are always welcome, but I'd expect basic communication to
work already with the current code (though maybe not as efficiently as
possible).
> Besides this, during development we might encounter additional differences between SPI and I2C.
>
> We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window.
>
> Regards,
> Eyal.
^ permalink raw reply
* Re: [PATCHv3] MIPS: JZ4780: DTS: Add I2C nodes
From: Linus Torvalds @ 2019-07-18 16:46 UTC (permalink / raw)
To: Alexandre GRIVEAUX
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
linux-mips, linux-kernel, devicetree
In-Reply-To: <20190717191926.GA4571@localhost.localdomain>
Your patches lack a sign-off, but the reason I am emailing is that
they get marked as spam when going through the mailing list because
the DKIM is bad.
The email does have what appears to be a find DKIM signature, but it
adds _way_ too many headers to the list to be checked, including the
"Sender" line, but also things like "List-Id" etc.
Which is completely wrong usage of DKIM when you go through a mailing
list (which is _supposed_ to change the Sender field!).
It looks like somebody mis-understood what DKIM is about, and added
all the lines they could find to the list of DKIM-protected headers.
You should generally protect the minimal core required set of headers:
From/To/Date/Subject/Message-ID etc. Not the headers that are
intentionally going to be rewritten by any list you might want to post
to.
Linus "hate spam, try to fix dkim" Torvalds
On Wed, Jul 17, 2019 at 12:20 PM Alexandre GRIVEAUX
<agriveaux@deutnet.info> wrote:
>
> Add the devicetree nodes for the I2C core of the JZ4780 SoC, disabled
> by default.
..
^ permalink raw reply
* Re: [PATCH v2 5/7] drm/bridge: Add Analogix anx6345 support
From: Torsten Duwe @ 2019-07-18 16:42 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Maxime Ripard, Chen-Yu Tsai, Rob Herring, Mark Rutland,
Thierry Reding, David Airlie, Daniel Vetter, Laurent Pinchart,
Icenowy Zheng, Sean Paul, Vasily Khoruzhick, Harald Geyer,
Greg Kroah-Hartman, Thomas Gleixner, dri-devel, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <610ab353-7e05-81b6-2cc4-2dac09823d42@samsung.com>
On Wed, Jun 12, 2019 at 11:13:10AM +0200, Andrzej Hajda wrote:
> On 04.06.2019 14:23, Torsten Duwe wrote:
> > +
> > +static void anx6345_poweron(struct anx6345 *anx6345)
> > +{
> > + int err;
> > +
> > + /* Ensure reset is asserted before starting power on sequence */
> > + gpiod_set_value_cansleep(anx6345->gpiod_reset, 1);
>
> With fixed devm_gpiod_get below this line can be removed.
In any case, reset must be asserted for this procedure to succeed...
> > +static enum drm_mode_status
> > +anx6345_bridge_mode_valid(struct drm_bridge *bridge,
> > + const struct drm_display_mode *mode)
> > +{
> > + if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> > + return MODE_NO_INTERLACE;
> > +
> > + /* Max 1200p at 5.4 Ghz, one lane */
> > + if (mode->clock > 154000)
> > + return MODE_CLOCK_HIGH;
>
> I wonder if you shouldn't take into account training results here, ie.
> training perfrormed before validation.
Sure, but this is verbatim from the anx78xx.c sibling, code provided
by analogix. Lacking in-depth datasheets, this is probably the best effort.
> > +
> > + /* 2.5V digital core power regulator */
> > + anx6345->dvdd25 = devm_regulator_get(dev, "dvdd25-supply");
> > + if (IS_ERR(anx6345->dvdd25)) {
> > + DRM_ERROR("dvdd25-supply not found\n");
> > + return PTR_ERR(anx6345->dvdd25);
> > + }
> > +
> > + /* GPIO for chip reset */
> > + anx6345->gpiod_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>
> Shouldn't be set to GPIOD_OUT_HIGH?
It used to be in the original submission, and confused even more people ;-)
Fact is, the reset for this chip _is_ low active; I'm following
Documentation/devicetree/bindings/gpio/gpio.txt, "1.1) GPIO specifier
best practices", which I find rather comprehensive.
Any suggestions on how to phrase this even better are appreciated.
> > +};
> > +module_i2c_driver(anx6345_driver);
> > +
> > +MODULE_DESCRIPTION("ANX6345 eDP Transmitter driver");
> > +MODULE_AUTHOR("Enric Balletbo i Serra <enric.balletbo@collabora.com>");
>
> Submitter, patch author, and module author are different, this can be
> correct, but maybe somebody forgot to update some of these fields.
As mentioned in the v2 cover letter, I had a closer look on which code got
actually introduced and which lines were simply copied around, and made the
copyright and authorship stick to where they belong. *I* believe this is
correct now; specific improvements welcome.
Torsten
^ permalink raw reply
* Re: [PATCH 3/3] dt-bindings: dsp: fsl: Add DSP core binding support
From: Rob Herring @ 2019-07-18 16:40 UTC (permalink / raw)
To: Daniel Baluta
Cc: Shawn Guo, Mark Rutland, Sascha Hauer, Sascha Hauer,
Fabio Estevam, NXP Linux Team, S.j. Wang, paul.olaru,
Dong Aisheng, Leonard Crestez, Anson Huang, Peng Fan, Frank Li,
devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel@vger.kernel.org, sound-open-firmware
In-Reply-To: <20190718151346.3523-4-daniel.baluta@nxp.com>
On Thu, Jul 18, 2019 at 9:13 AM Daniel Baluta <daniel.baluta@nxp.com> wrote:
>
> This describes the DSP device tree node.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
> .../devicetree/bindings/dsp/fsl,dsp.yaml | 87 +++++++++++++++++++
> 1 file changed, 87 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
>
> diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> new file mode 100644
> index 000000000000..d112486eda0e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/freescale/fsl,dsp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP i.MX8 DSP core
> +
> +maintainers:
> + - Daniel Baluta <daniel.baluta@nxp.com>
> +
> +description: |
> + Some boards from i.MX8 family contain a DSP core used for
> + advanced pre- and post- audio processing.
> +
> +properties:
> + compatible:
> + enum:
> + - fsl,imx8qxp-dsp
> +
> + reg:
> + description: Should contain register location and length
> +
> + clocks:
> + items:
> + - description: ipg clock
> + - description: ocram clock
> + - description: core clock
> +
> + clock-names:
> + items:
> + - const: ipg
> + - const: ocram
> + - const: core
> +
> + power-domains:
> + description:
> + List of phandle and PM domain specifier as documented in
> + Documentation/devicetree/bindings/power/power_domain.txt
How many? 4?
> +
> + mboxes:
> + description:
> + List of <&phandle type channel> - 2 channels for TXDB, 2 channels for RXDB
> + (see mailbox/fsl,mu.txt)
> + maxItems: 4
> +
> + mbox-names:
> + items:
> + - const: txdb0
> + - const: txdb1
> + - const: rxdb0
> + - const: rxdb1
> +
> + memory-region:
> + description:
> + phandle to a node describing reserved memory (System RAM memory)
> + used by DSP (see bindings/reserved-memory/reserved-memory.txt)
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - power-domains
> + - mboxes
> + - mbox-names
> + - memory-region
> +
> +examples:
> + - |
> + #include <dt-bindings/firmware/imx/rsrc.h>
> + #include <dt-bindings/clock/imx8-clock.h>
> + dsp@596e8000 {
> + compatbile = "fsl,imx8qxp-dsp";
> + reg = <0x596e8000 0x88000>;
> + clocks = <&adma_lpcg IMX_ADMA_LPCG_DSP_IPG_CLK>,
> + <&adma_lpcg IMX_ADMA_LPCG_OCRAM_IPG_CLK>,
> + <&adma_lpcg IMX_ADMA_LPCG_DSP_CORE_CLK>;
> + clock-names = "ipg", "ocram", "core";
> + power-domains = <&pd IMX_SC_R_MU_13A>,
> + <&pd IMX_SC_R_MU_13B>,
> + <&pd IMX_SC_R_DSP>,
> + <&pd IMX_SC_R_DSP_RAM>;
> + mbox-names = "txdb0", "txdb1", "rxdb0", "rxdb1";
> + mboxes = <&lsio_mu13 2 0>, <&lsio_mu13 2 1>, <&lsio_mu13 3 0>, <&lsio_mu13 3 1>;
> + };
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-18 16:34 UTC (permalink / raw)
To: Sowjanya Komatineni, sboyd, Michael Turquette
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <ef7928ad-239d-eca8-41bf-f76e72a9841d@nvidia.com>
18.07.2019 4:15, Sowjanya Komatineni пишет:
[snip]
>>> Please try to fix all missing dependencies and orderings.
>>
>> Peter,
>>
>> dfllCPU_OUT is the first one to go thru restore when
>> clk_restore_context traverses thru the list.
>>
>> dfllCPU_OUT has dependency on DFLL_ref and DFLL_SOC but this
>> dependency is unknown to clock-tree.
>>
>> We can add DFLL_REF and DFLL_SOC as parents to dfllCPU_OUT during
>> register so dfllCPU_OUT save/restore happens after their parents are
>> restored.
>>
>> But DFLL needs both of these to be restored before DFLLCPU_Out and as
>> DFLL_SOC restore always happens after the REF, thinking to add
>> DFLL_SOC as parent to dfllCPU_OUT so save/restore follows after their
>> dependencies.
>>
>> Please comment.
>>
> Did quick try and I see by adding dfll-soc as parent to dfllCPU_OUT, its
> in proper order after all its dependencies.
>
> Can now add dfll save/restore to do dfll reinit during restore..
>
If dfllCPU_OUT can work properly with dfll-soc being disabled, then this
kind of dependency isn't very correct and just papers over the real
problem, which is that there should be a way for CCF to specify multiple
dependencies for the clock or the reverse ordering should be used for
the restoring.
^ permalink raw reply
* Re: [PATCH] dt-bindings: pinctrl: stm32: Fix missing 'clocks' property in examples
From: Alexandre Torgue @ 2019-07-18 16:21 UTC (permalink / raw)
To: Rob Herring, Linus Walleij
Cc: devicetree, linux-kernel, linux-gpio, Maxime Coquelin,
linux-stm32, linux-arm-kernel
In-Reply-To: <20190716215618.29757-1-robh@kernel.org>
Hi Rob
On 7/16/19 11:56 PM, Rob Herring wrote:
> Now that examples are validated against the DT schema, an error with
> required 'clocks' property missing is exposed:
>
> Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.example.dt.yaml: \
> pinctrl@40020000: gpio@0: 'clocks' is a required property
> Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.example.dt.yaml: \
> pinctrl@50020000: gpio@1000: 'clocks' is a required property
> Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.example.dt.yaml: \
> pinctrl@50020000: gpio@2000: 'clocks' is a required property
>
> Add the missing 'clocks' properties to the examples to fix the errors.
>
> Fixes: 2c9239c125f0 ("dt-bindings: pinctrl: Convert stm32 pinctrl bindings to json-schema")
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: linux-gpio@vger.kernel.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Signed-off-by: Rob Herring <robh@kernel.org>
Acked-by: Alexandre TORGUE <alexandre.torgue@st.com>
Thanks
Alex
> ---
> .../devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml
> index 3ac5d2088e49..91d3e78b3395 100644
> --- a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml
> @@ -197,6 +197,7 @@ required:
> examples:
> - |
> #include <dt-bindings/pinctrl/stm32-pinfunc.h>
> + #include <dt-bindings/mfd/stm32f4-rcc.h>
> //Example 1
> pinctrl@40020000 {
> #address-cells = <1>;
> @@ -210,6 +211,7 @@ examples:
> #gpio-cells = <2>;
> reg = <0x0 0x400>;
> resets = <&reset_ahb1 0>;
> + clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOA)>;
> st,bank-name = "GPIOA";
> };
> };
> @@ -227,6 +229,7 @@ examples:
> #gpio-cells = <2>;
> reg = <0x1000 0x400>;
> resets = <&reset_ahb1 0>;
> + clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOB)>;
> st,bank-name = "GPIOB";
> gpio-ranges = <&pinctrl 0 0 16>;
> };
> @@ -236,6 +239,7 @@ examples:
> #gpio-cells = <2>;
> reg = <0x2000 0x400>;
> resets = <&reset_ahb1 0>;
> + clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOC)>;
> st,bank-name = "GPIOC";
> ngpios = <5>;
> gpio-ranges = <&pinctrl 0 16 3>,
>
^ permalink raw reply
* Re: [PATCH] dt-bindings: Ensure child nodes are of type 'object'
From: Alexandre Torgue @ 2019-07-18 16:20 UTC (permalink / raw)
To: Rob Herring, devicetree
Cc: Vignesh Raghavendra, linux-gpio, Maxime Ripard,
Richard Weinberger, Linus Walleij, linux-kernel, linux-spi,
Marek Vasut, Chen-Yu Tsai, Mark Brown, linux-mtd, Maxime Coquelin,
Miquel Raynal, Brian Norris, David Woodhouse, linux-stm32
In-Reply-To: <20190715230457.3901-1-robh@kernel.org>
Hi Rob
On 7/16/19 1:04 AM, 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.
>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: linux-mtd@lists.infradead.org
> Cc: linux-gpio@vger.kernel.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-spi@vger.kernel.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Please ack. I will take this via the DT tree.
>
For pinctrl STM32:
Acked-by: Alexandre TORGUE <alexandre.torgue@st.com>
thanks
Alexandre
> Rob
>
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ 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