Re: [PATCH v3 rcu 3/3] rcu: Finer-grained grace-period-end checks in rcu_dump_cpu_stacks()

[Doug Anderson]

Hi,

On Wed, Oct 30, 2024 at 10:03 PM Paul E. McKenney  wrote:
>
> > > > Note that:
> > > >
> > > > * Switching to real NMIs is impossible on many existing arm64 CPUs.
> > > > The hardware support simply isn't there. Pseudo-NMIs should work fine
> > > > and are (in nearly all cases) just as good as NMIs but they have a
> > > > small performance impact. There are also compatibility issues with
> > > > some pre-existing bootloaders. ...so code can't assume even Pseudo-NMI
> > > > will work and needs to be able to fall back. Prior to my recent
> > > > changes arm64 CPUs wouldn't even do stacktraces in some scenarios. Now
> > > > at least they fall back to regular IPIs.
> > >
> > > But those IPIs are of no help whatsoever when the target CPU is spinning
> > > with interrupts disabled, which is the scenario under discussion.
> >
> > Right that we can't trace the target CPU spinning with interrupts
> > disabled.
>
> You can by invoking sched_show_task(cpu_curr(cpu)) on the CPU requesting
> the backtrace.  The resulting backtrace will not be as good as you
> would get from using an NMI, but if you don't have NMIs, it is better
> than nothing.
>
> >   ...but in the case where NMI isn't available it _still_
> > makes sense to make arch_trigger_cpumask_backtrace() fall back to IPI
> > because:
> >
> > 1. There are many cases where the trigger.*cpu.*backtrace() class of
> > functions are used to trace CPUs that _aren't_ looping with interrupts
> > disabled. We still want to be able to backtrace in that case. For
> > instance, you can see in "panic.c" that there are cases where
> > trigger_all_cpu_backtrace() is called. It's valuable to make cases
> > like this (where there's no indication that a CPU is locked) actually
> > work.
> >
> > 2. Even if there's a CPU that's looping with interrupts disabled and
> > we can't trace it because of no NMI, it could still be useful to get
> > backtraces of other CPUs. It's possible that what the other CPUs are
> > doing could give a hint about the CPU that's wedged. This isn't a
> > great hint, but some info is better than no info.
>
> And it also makes sense for an IRQ-based backtrace to fall back to
> something like the aforementioned sched_show_task(cpu_curr(cpu)) if
> the destination CPU has interrupts disabled.
>
> > > Hence my suggestion that arm64, when using IRQs to request stack
> > > backtraces, have an additional short timeout (milliseconds) in
> > > order to fall back to remote stack tracing.  In many cases, this
> > > fallback happens automatically, as you can see in dump_cpu_task().
> > > If trigger_single_cpu_backtrace() returns false, the stack is dumped
> > > remotely.
> >
> > I think the answer here is that the API needs to change. The whole
> > boolean return value for trigger.*cpu.*backtrace() is mostly ignored
> > by callers. Yes, you've pointed at one of the two places that it's not
> > ignored and it tries a reasonable fallback, but all the other callers
> > just silently do nothing when the function returns false. That led to
> > many places where arm64 devices were simply not getting _any_
> > stacktrace.
> >
> > Perhaps we need some type of API that says "I actually have a
> > fallback, so if you don't think the stracktrace will succeed then it's
> > OK to return false".
>
> Boolean is fine for trigger_single_cpu_backtrace(), which is directed at
> a single CPU.  And the one call to this function that does not currently
> check its return value could just call dump_cpu_task() instead.
>
> There are only 20 or so uses of all of these functions, so the API can
> change, give or take the pain involved in changing architecture-specific
> code.

Right. Falling back to "sched_show_task(cpu_curr(cpu))" or something
similar if the IPI doesn't go through is a good idea. Indeed, falling
back to that if the NMI doesn't go through is _also_ a good idea,
right? ...and we don't want to change all 20 callers to all add a
fallback. To that end, it feels like someone should change it so that
the common code includes the fallback and we get rid of the boolean
return value.


> > > > * Even if we decided that we absolutely had to disable stacktrades on
> > > > arm64 CPUs without some type of NMI, that won't fix arm32. arm32 has
> > > > been using regular IPIs for backtraces like this for many, many years.
> > > > Maybe folks don't care as much about arm32 anymore but it feels bad if
> > > > we totally break it.
> > >
> > > Because arm32 isn't used much for datacenter workloads, it will not
> > > be suffering from this issue.  Not enough to have motivated anyone to
> > > fix it, anyway.  In contrast, in the datacenter, CPUs really can and
> > > do have interrupts disabled for many seconds.  (No, this is not a good
> > > thing, but it is common enough for us to need to avoid disabling other
> > > debugging facilities.)
> > >
> > > So it might well be that arm32 will continue to do just fine with
> > > IRQ-based stack tracing.  Or maybe someday arm32 will also need to

Re: [PATCH v3 rcu 3/3] rcu: Finer-grained grace-period-end checks in rcu_dump_cpu_stacks()

[Doug Anderson]

Hi,

On Wed, Oct 30, 2024 at 4:27 PM Paul E. McKenney  wrote:
>
> On Wed, Oct 30, 2024 at 01:12:01PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Oct 30, 2024 at 6:54 AM Paul E. McKenney  wrote:
> > >
> > > > > We do assume that nmi_trigger_cpumask_backtrace() uses true NMIs, so,
> > > > > yes, nmi_trigger_cpumask_backtrace() should use true NMIs, just like
> > > > > the name says.  ;-)
> > > >
> > > > In the comments of following patch, the arm64 maintainers have
> > > > differing views on the use of nmi_trigger_cpumask_backtrace(). I'm a
> > > > bit confused and unsure which perspective is more reasonable.
> > > >
> > > > https://lore.kernel.org/all/20230906090246.v13.4.Ie6c132b96ebbbcddbf6954b9469ed40a6960343c@changeid/
> > >
> > > I clearly need to have a chat with the arm64 maintainers, and thank
> > > you for checking.
> > >
> > > > > /*
> > > > >  * NOTE: though nmi_trigger_cpumask_backtrace() has "nmi_" in the
> > > > name,
> > > > >  * nothing about it truly needs to be implemented using an NMI, it's
> > > > >  * just that it's _allowed_ to work with NMIs. If ipi_should_be_nmi()
> > > > >  * returned false our backtrace attempt will just use a regular IPI.
> > > > >  */
> > > >
> > > > > Alternatively, arm64 could continue using 
> > > > > nmi_trigger_cpumask_backtrace()
> > > > > with normal interrupts (for example, on SoCs not implementing true 
> > > > > NMIs),
> > > > > but have a short timeout (maybe a few jiffies?) after which its 
> > > > > returns
> > > > > false (and presumably also cancels the backtrace request so that when
> > > > > the non-NMI interrupt eventually does happen, its handler simply 
> > > > > returns
> > > > > without backtracing).  This should be implemented using atomics to 
> > > > > avoid
> > > > > deadlock issues.  This alternative approach would provide accurate 
> > > > > arm64
> > > > > backtraces in the common case where interrupts are enabled, but allow
> > > > > a graceful fallback to remote tracing otherwise.
> > > > >
> > > > > Would you be interested in working this issue, whatever solution the
> > > > > arm64 maintainers end up preferring?
> > > >
> > > > The 10-second timeout is hard-coded in nmi_trigger_cpumask_backtrace().
> > > > It is shared code and not architecture-specific. Currently, I haven't
> > > > thought of a feasible solution. I have also CC'd the authors of the
> > > > aforementioned patch to see if they have any other ideas.
> > >
> > > It should be possible for arm64 to have an architecture-specific hook
> > > that enables them to use a much shorter timeout.  Or, to eventually
> > > switch to real NMIs.
> >
> > Note that:
> >
> > * Switching to real NMIs is impossible on many existing arm64 CPUs.
> > The hardware support simply isn't there. Pseudo-NMIs should work fine
> > and are (in nearly all cases) just as good as NMIs but they have a
> > small performance impact. There are also compatibility issues with
> > some pre-existing bootloaders. ...so code can't assume even Pseudo-NMI
> > will work and needs to be able to fall back. Prior to my recent
> > changes arm64 CPUs wouldn't even do stacktraces in some scenarios. Now
> > at least they fall back to regular IPIs.
>
> But those IPIs are of no help whatsoever when the target CPU is spinning
> with interrupts disabled, which is the scenario under discussion.

Right that we can't trace the target CPU spinning with interrupts
disabled. ...but in the case where NMI isn't available it _still_
makes sense to make arch_trigger_cpumask_backtrace() fall back to IPI
because:

1. There are many cases where the trigger.*cpu.*backtrace() class of
functions are used to trace CPUs that _aren't_ looping with interrupts
disabled. We still want to be able to backtrace in that case. For
instance, you can see in "panic.c" that there are cases where
trigger_all_cpu_backtrace() is called. It's valuable to make cases
like this (where there's no indication that a CPU is locked) actually
work.

2. Even if there's a CPU that's looping with interrupts disabled and
we can't trace it because of no NMI, it could still be useful to get
backtraces of other CPUs. It's possible that

Re: [PATCH v3 rcu 3/3] rcu: Finer-grained grace-period-end checks in rcu_dump_cpu_stacks()

[Doug Anderson]

Hi,

On Wed, Oct 30, 2024 at 6:54 AM Paul E. McKenney  wrote:
>
> > > We do assume that nmi_trigger_cpumask_backtrace() uses true NMIs, so,
> > > yes, nmi_trigger_cpumask_backtrace() should use true NMIs, just like
> > > the name says.  ;-)
> >
> > In the comments of following patch, the arm64 maintainers have
> > differing views on the use of nmi_trigger_cpumask_backtrace(). I'm a
> > bit confused and unsure which perspective is more reasonable.
> >
> > https://lore.kernel.org/all/20230906090246.v13.4.Ie6c132b96ebbbcddbf6954b9469ed40a6960343c@changeid/
>
> I clearly need to have a chat with the arm64 maintainers, and thank
> you for checking.
>
> > > /*
> > >  * NOTE: though nmi_trigger_cpumask_backtrace() has "nmi_" in the
> > name,
> > >  * nothing about it truly needs to be implemented using an NMI, it's
> > >  * just that it's _allowed_ to work with NMIs. If ipi_should_be_nmi()
> > >  * returned false our backtrace attempt will just use a regular IPI.
> > >  */
> >
> > > Alternatively, arm64 could continue using nmi_trigger_cpumask_backtrace()
> > > with normal interrupts (for example, on SoCs not implementing true NMIs),
> > > but have a short timeout (maybe a few jiffies?) after which its returns
> > > false (and presumably also cancels the backtrace request so that when
> > > the non-NMI interrupt eventually does happen, its handler simply returns
> > > without backtracing).  This should be implemented using atomics to avoid
> > > deadlock issues.  This alternative approach would provide accurate arm64
> > > backtraces in the common case where interrupts are enabled, but allow
> > > a graceful fallback to remote tracing otherwise.
> > >
> > > Would you be interested in working this issue, whatever solution the
> > > arm64 maintainers end up preferring?
> >
> > The 10-second timeout is hard-coded in nmi_trigger_cpumask_backtrace().
> > It is shared code and not architecture-specific. Currently, I haven't
> > thought of a feasible solution. I have also CC'd the authors of the
> > aforementioned patch to see if they have any other ideas.
>
> It should be possible for arm64 to have an architecture-specific hook
> that enables them to use a much shorter timeout.  Or, to eventually
> switch to real NMIs.

Note that:

* Switching to real NMIs is impossible on many existing arm64 CPUs.
The hardware support simply isn't there. Pseudo-NMIs should work fine
and are (in nearly all cases) just as good as NMIs but they have a
small performance impact. There are also compatibility issues with
some pre-existing bootloaders. ...so code can't assume even Pseudo-NMI
will work and needs to be able to fall back. Prior to my recent
changes arm64 CPUs wouldn't even do stacktraces in some scenarios. Now
at least they fall back to regular IPIs.

* Even if we decided that we absolutely had to disable stacktrades on
arm64 CPUs without some type of NMI, that won't fix arm32. arm32 has
been using regular IPIs for backtraces like this for many, many years.
Maybe folks don't care as much about arm32 anymore but it feels bad if
we totally break it.

IMO waiting 10 seconds for a backtrace is pretty crazy to begin with.
What about just changing that globally to 1 second? If not, it doesn't
feel like it would be impossibly hard to make an arch-specific
callback to choose the time and that callback could even take into
account whether we managed to get an NMI. I'd be happy to review such
a patch.

-Doug

Re: [PATCH] remoteproc: qcom_q6v5_mss: Re-order writes to the IMEM region

[Doug Anderson]

Hi,

On Mon, Aug 19, 2024 at 4:40 PM Doug Anderson  wrote:
>
> Hi,
>
> On Mon, Aug 19, 2024 at 12:30 AM Sibi Sankar  wrote:
> >
> > Any write access to the IMEM region when the Q6 is setting up XPU
> > protection on it will result in a XPU violation. Fix this by ensuring
> > IMEM writes related to the MBA post-mortem logs happen before the Q6
> > is brought out of reset.
> >
> > Fixes: 318130cc9362 ("remoteproc: qcom_q6v5_mss: Add MBA log extraction 
> > support")
> > Signed-off-by: Sibi Sankar 
> > ---
> >  drivers/remoteproc/qcom_q6v5_mss.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
>
> As discussed offlist, this isn't a perfect fix since writes to this
> IMEM could happen by other drivers and those could still cause things
> to go boom if they run in parallel with this driver. That being said:
> * It seems like a more proper fix needs a coordinated effort between a
> device's built-in firmware and the modem firmware. This is difficult /
> near impossible to get done properly.
> * Even if we do a more proper fix, making this change won't hurt.
> * This change will immediately improve things by avoiding the XPU
> violation in the most common case.
>
> I've confirmed that the test case I had where things were going boom
> is fixed. Thus:
>
> Reviewed-by: Douglas Anderson 
> Tested-by: Douglas Anderson 

Just checking in to see if there's anything else needed for this patch
to land. Thanks! :-)

-Doug

Re: [PATCH] remoteproc: qcom_q6v5_mss: Re-order writes to the IMEM region

[Doug Anderson]

Hi,

On Mon, Aug 19, 2024 at 12:30 AM Sibi Sankar  wrote:
>
> Any write access to the IMEM region when the Q6 is setting up XPU
> protection on it will result in a XPU violation. Fix this by ensuring
> IMEM writes related to the MBA post-mortem logs happen before the Q6
> is brought out of reset.
>
> Fixes: 318130cc9362 ("remoteproc: qcom_q6v5_mss: Add MBA log extraction 
> support")
> Signed-off-by: Sibi Sankar 
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

As discussed offlist, this isn't a perfect fix since writes to this
IMEM could happen by other drivers and those could still cause things
to go boom if they run in parallel with this driver. That being said:
* It seems like a more proper fix needs a coordinated effort between a
device's built-in firmware and the modem firmware. This is difficult /
near impossible to get done properly.
* Even if we do a more proper fix, making this change won't hurt.
* This change will immediately improve things by avoiding the XPU
violation in the most common case.

I've confirmed that the test case I had where things were going boom
is fixed. Thus:

Reviewed-by: Douglas Anderson 
Tested-by: Douglas Anderson 

Re: [PATCH 7/9] arm64: dts: qcom: sc7280: Add CDSP node

[Doug Anderson]

Hi,

On Mon, Oct 30, 2023 at 7:43 AM Luca Weiss  wrote:
>
> On Mon Oct 30, 2023 at 3:11 PM CET, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Oct 30, 2023 at 2:12 AM Luca Weiss  wrote:
> > >
> > > On Mon Oct 30, 2023 at 10:04 AM CET, Mukesh Ojha wrote:
> > > >
> > > >
> > > > On 10/27/2023 7:50 PM, Luca Weiss wrote:
> > > > > Add the node for the ADSP found on the SC7280 SoC, using standard
> > > > > Qualcomm firmware.
> > > > >
> > > > > The memory region for sc7280-chrome-common.dtsi is taken from msm-5.4
> > > > > yupik.dtsi since the other areas also seem to match that file there,
> > > > > though I cannot be sure there.
> > > > >
> > > > > Signed-off-by: Luca Weiss 
> > > > > ---
> > > > >   arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi |   5 +
> > > > >   arch/arm64/boot/dts/qcom/sc7280.dtsi   | 138 
> > > > > +
> > > > >   2 files changed, 143 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi 
> > > > > b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> > > > > index eb55616e0892..6e5a9d4c1fda 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> > > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> > > > > @@ -29,6 +29,11 @@ adsp_mem: memory@8670 {
> > > > > no-map;
> > > > > };
> > > > >
> > > > > +   cdsp_mem: memory@88f0 {
> > > > > +   reg = <0x0 0x88f0 0x0 0x1e0>;
> > > > > +   no-map;
> > > > > +   };
> > > > > +
> > > >
> > > > Just a question, why to do it here, if chrome does not use this ?
> > >
> > > Other memory regions in sc7280.dtsi also get referenced but not actually
> > > defined in that file, like mpss_mem and wpss_mem. Alternatively we can
> > > also try and solve this differently, but then we should probably also
> > > adjust mpss and wpss to be consistent.
> > >
> > > Apart from either declaring cdsp_mem in sc7280.dtsi or
> > > "/delete-property/ memory-region;" for CDSP I don't really have better
> > > ideas though.
> > >
> > > I also imagine these ChromeOS devices will want to enable cdsp at some
> > > point but I don't know any plans there.
> >
> > Given that "remoteproc_cdsp" has status "disabled" in the dtsi, it
> > feels like the dtsi shouldn't be reserving memory. I guess maybe
> > memory regions can't be status "disabled"?
>
> Hi Doug,
>
> That's how it works in really any qcom dtsi though. I think in most/all
> cases normally the reserved-memory is already declared in the SoC dtsi
> file and also used with the memory-region property.
>
> I wouldn't be against adjusting sc7280.dtsi to match the way it's done
> in the other dtsi files though, so to have all the required labels
> already defined in the dtsi so it doesn't rely on these labels being
> defined in the device dts.
>
> In other words, currently if you include sc7280.dtsi and try to build,
> you first have to define the labels mpss_mem and wpss_mem (after this
> patch series also cdsp_mem and adsp_mem) for it to build.
>
> I'm quite neutral either way, let me know :)

I haven't done a ton of thinking about this, so if I'm spouting
gibberish then feel free to ignore me. :-P It just feels weird that
when all the "dtsi" files are combined and you look at what you end up
on a sc7280 Chrome board that you'll be reserving 32MB of memory for a
device that's set (in the same device tree) to be "disabled", right?
...the 32MB is completely wasted, I think. If we wanted to enable the
CDSP we'd have to modify the device tree anyway, so it seems like that
same modification would set the CDSP to "okay" and also reserve the
memory...

In that vein, it seems like maybe you could move the "cdsp_mem" to the
SoC .dsti file with a status of "disabled". . I guess we don't do that
elsewhere, but maybe we should be? As far as I can tell without
testing it (just looking at fdt_scan_reserved_mem()) this should
work...

-Doug

Re: [PATCH 7/9] arm64: dts: qcom: sc7280: Add CDSP node

[Doug Anderson]

Hi,

On Mon, Oct 30, 2023 at 2:12 AM Luca Weiss  wrote:
>
> On Mon Oct 30, 2023 at 10:04 AM CET, Mukesh Ojha wrote:
> >
> >
> > On 10/27/2023 7:50 PM, Luca Weiss wrote:
> > > Add the node for the ADSP found on the SC7280 SoC, using standard
> > > Qualcomm firmware.
> > >
> > > The memory region for sc7280-chrome-common.dtsi is taken from msm-5.4
> > > yupik.dtsi since the other areas also seem to match that file there,
> > > though I cannot be sure there.
> > >
> > > Signed-off-by: Luca Weiss 
> > > ---
> > >   arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi |   5 +
> > >   arch/arm64/boot/dts/qcom/sc7280.dtsi   | 138 
> > > +
> > >   2 files changed, 143 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi 
> > > b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> > > index eb55616e0892..6e5a9d4c1fda 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> > > @@ -29,6 +29,11 @@ adsp_mem: memory@8670 {
> > > no-map;
> > > };
> > >
> > > +   cdsp_mem: memory@88f0 {
> > > +   reg = <0x0 0x88f0 0x0 0x1e0>;
> > > +   no-map;
> > > +   };
> > > +
> >
> > Just a question, why to do it here, if chrome does not use this ?
>
> Other memory regions in sc7280.dtsi also get referenced but not actually
> defined in that file, like mpss_mem and wpss_mem. Alternatively we can
> also try and solve this differently, but then we should probably also
> adjust mpss and wpss to be consistent.
>
> Apart from either declaring cdsp_mem in sc7280.dtsi or
> "/delete-property/ memory-region;" for CDSP I don't really have better
> ideas though.
>
> I also imagine these ChromeOS devices will want to enable cdsp at some
> point but I don't know any plans there.

Given that "remoteproc_cdsp" has status "disabled" in the dtsi, it
feels like the dtsi shouldn't be reserving memory. I guess maybe
memory regions can't be status "disabled"?

-Doug

Re: [PATCH v2 2/7] nvmem: qfprom: Mark core clk as optional

[Doug Anderson]

Hi,

On Tue, Sep 19, 2023 at 5:46 AM Luca Weiss  wrote:
>
> On some platforms like sc7280 on non-ChromeOS devices the core clock
> cannot be touched by Linux so we cannot provide it. Mark it as optional
> as accessing qfprom for reading works without it but we still prohibit
> writing if we cannot provide the clock.
>
> Signed-off-by: Luca Weiss 
> ---
>  drivers/nvmem/qfprom.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card

[Doug Anderson]

Hi,

On Tue, Apr 20, 2021 at 10:21 AM  wrote:
>
> On 2021-04-15 01:55, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 13, 2021 at 3:59 AM  wrote:
> >>
> >> >> >>> +   required-opps =
> >> >> >>> <&rpmhpd_opp_low_svs>;
> >> >> >>> +   opp-peak-kBps = <120
> >> >> >>> 76000>;
> >> >> >>> +   opp-avg-kBps = <120
> >> >> >>> 5>;
> >> >> >> Why are the kBps numbers so vastly different than the ones on sc7180
> >> >> >> for the same OPP point. That implies:
> >> >> >>
> >> >> >> a) sc7180 is wrong.
> >> >> >>
> >> >> >> b) This patch is wrong.
> >> >> >>
> >> >> >> c) The numbers are essentially random and don't really matter.
> >> >> >>
> >> >> >> Can you identify which of a), b), or c) is correct, or propose an
> >> >> >> alternate explanation of the difference?
> >> >> >>
> >> >>
> >> >> We calculated bus votes values for both sc7180 and sc7280 with ICB
> >> >> tool,
> >> >> above mentioned values we got for sc7280.
> >> >
> >> > I don't know what an ICB tool is. Please clarify.
> >> >
> >> > Also: just because a tool spits out numbers that doesn't mean it's
> >> > correct. Presumably the tool could be wrong or incorrectly configured.
> >> > We need to understand why these numbers are different.
> >> >
> >> we checked with ICB tool team on this they conformed as Rennell &
> >> Kodiak
> >> are different chipsets,
> >> we might see delta in ib/ab values due to delta in scaling factors.
> >
> > ...but these numbers are in kbps, aren't they? As I understand it
> > these aren't supposed to be random numbers spit out by a tool but are
> > supposed to be understandable by how much bandwidth an IP block (like
> > MMC) needs from the busses it's connected to. Since the MMC IP block
> > on sc7180 and sc7280 is roughly the same there shouldn't be a big
> > difference in numbers.
> >
> > Something smells wrong.
> >
> > Adding a few people who understand interconnects better than I do,
> > though.
> >
>
> ICB team has re-checked the Rennell ICB tool and they confirmed that
> some configs were wrong in Rennell ICB tool and they corrected it.With
> the new updated Rennell ICB tool below are the values :
>
>
> Rennell LC:(Sc7180)
>
> opp-38400 {
>   opp-hz = /bits/ 64 <38400>;
>   required-opps = <&rpmhpd_opp_nom>;
>   opp-peak-kBps = <540 49>;
>   opp-avg-kBps = <660 30>;
> };
>
>
> And now, these values are near to Kodaik LC values:
>
> Kodaik LC:(SC7280)
>
> opp-38400 {
> opp-hz = /bits/ 64 <38400>;
> required-opps = <&rpmhpd_opp_nom>;
> opp-peak-kBps = <540 399000>;
> opp-avg-kBps = <600 30>;
> };

This still isn't making sense to me.

* sc7180 and sc7280 are running at the same speed. I'm glad the
numbers are closer now, but I would have thought they'd be exactly the
same.

* Aren't these supposed to be sensible? This is eMMC that does max
transfer rates of 400 megabytes / second to the external device. You
have bandwidths listed here of 5,400,000 kBps = 5,400,000 kilobytes /
second = 5400 megabytes / second. I can imagine there being some
overhead where an internal bus might need to be faster but that seems
excessive. This is 13.5x!

* I can't see how it can make sense that "average" values are higher
than "peak" values.

It still feels like there's a misconfiguration somewhere.

-Doug

Re: [PATCH v4 00/27] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems

[Doug Anderson]

Hi,

On Fri, Apr 16, 2021 at 3:40 PM Douglas Anderson  wrote:
>
> The primary goal of this series is to try to properly fix EDID reading
> for eDP panels using the ti-sn65dsi86 bridge.
>
> Previously we had a patch that added EDID reading but it turned out
> not to work at bootup. This caused some extra churn at bootup as we
> tried (and failed) to read the EDID several times and also ended up
> forcing us to use the hardcoded mode at boot. With this patch series I
> believe EDID reading is reliable at boot now and we never use the
> hardcoded mode.
>
> This series is the logical successor to the 3-part series containing
> the patch ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only
> if refclk") [1] though only one actual patch is the same between the
> two.
>
> This series starts out with some general / obvious fixes and moves on
> to some more specific and maybe controversial ones. I wouldn't object
> to some of the earlier ones landing if they look ready.
>
> This patch was developed agains linuxnext (next-20210416) on a
> sc7180-trogdor-lazor device. To get things booting for me, I had to
> use Stephen's patch [2] to keep from crashing but otherwise all the
> patches I needed were here.
>
> Primary change between v2 and v3 is to stop doing the EDID caching in
> the core. I also added Andrzej's review tags.
>
> Between v3 and v4 this series grew a whole lot. I changed it so that
> the EDID reading is actually driven by the panel driver now as was
> suggested by Andrzej. While I still believe that the old approach
> wasn't too bad I'm still switching. Why?
>
> The main reason is that I think it's useful in general for the panel
> code to have access to the DDC bus and to be able to read the
> EDID. This may allow us to more easily have the panel code support
> multiple sources of panels--it can read the EDID and possibly adjust
> timings based on the model ID. It also allows the panel code (or
> perhaps backlight code?) to send DDC commands if they are need for a
> particular panel.
>
> At the moment, once the panel is provided the DDC bus then existing
> code will assume that it should be in charge of reading the
> EDID. While it doesn't have to work that way, it seems sane to build
> on what's already there.
>
> In order to expose the DDC bus to the panel, I had to solve a bunch of
> chicken-and-egg problems in terms of probe ordering between the bridge
> and the panel. I've broken the bridge driver into several sub drivers
> to make this happen. At the moment the sub-drivers are just there to
> solve the probe problem, but conceivably someone could use them to
> break the driver up in the future if need be.
>
> I apologize in advance for the length of this series. I'm currently
> working through getting commit access to drm-misc [3] so I can land
> the first several patches which are already reviewed. There are still
> a lot of patches even after the first few, but hopefully you can see
> that there are only so many because they're broken up into nice and
> reviewable bite-sized-chunks. :-)
>
> [1] 
> https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
> [2] 
> https://lore.kernel.org/r/161706912161.3012082.17313817257247946...@swboyd.mtv.corp.google.com/
> [3] https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/348
>
> Changes in v4:
> - Reword commit mesage slightly.
>
> Changes in v3:
> - Removed "NOTES" from commit message.
>
> Changes in v2:
> - Removed 2nd paragraph in commit message.
>
> Douglas Anderson (27):
>   drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()
>   drm/bridge: ti-sn65dsi86: Simplify refclk handling
>   drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment
>   drm/bridge: ti-sn65dsi86: Reorder remove()
>   drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable()
>   drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function
>   drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare /
> prepare

I have pushed the above 7 patches to drm-misc-next now so I don't have
to spam everyone with them for v5. The first patch is technically a
"fix" but I'm not aware of it affecting anyone in mainline and so I
didn't try to direct it to a fixes branch. The next 5 are trivial /
reviewed plenty. The last one is a bigger change but has Laurent's
review on it and it's been up on the lists for a while, so I sent it
in too.

I'll wait a few more days to see if any of the other "trivial" patches
early in the series get reviews and see if there is any other feedback
on the rest of the series. If I get reviews for some of the early
patches I'll likely try to push them before the v5.

-Doug

Re: [v1 0/3] drm: Add support for backlight control of eDP panel on ti-sn65dsi86 bridge

[Doug Anderson]

Hi,

On Wed, Apr 14, 2021 at 9:41 AM Rajeev Nandan  wrote:
>
> The backlight level of an eDP panel can be controlled through the AUX
> channel using DPCD registers of the panel.
>
> The capability for the Source device to adjust backlight characteristics
> within the panel, using the Sink device DPCD registers is indicated by
> the TCON_BACKLIGHT_ADJUSTMENT_CAPABLE bit in the EDP_GENERAL_CAPABILITY_1
> register (DPCD Address 701h, bit0). In this configuration, the eDP TCON
> receives the backlight level information from the host, through the AUX
> channel.
>
> The changes in this patch series do the following:
> - Add drm_dp_aux_backlight_ APIs to support backlight control using DPCD
>   registers on the DisplayPort AUX channel.
>   The current version only supports backlight brightness control by the
>   EDP_BACKLIGHT_BRIGHTNESS_MSB/LSB registers (DPCD Addresses 722h-723h).
> - Add support for backlight control of the eDP panel connected to the
>   ti-sn65dsi86 bridge.
>
> Rajeev Nandan (3):
>   drm/dp: Add DisplayPort aux backlight control support
>   dt-bindings: drm/bridge: ti-sn65dsi86: Document use-aux-backlight
>   drm/bridge: ti-sn65dsi86: Add DisplayPort aux backlight support
>
>  .../bindings/display/bridge/ti,sn65dsi86.yaml  |   8 +
>  drivers/gpu/drm/Kconfig|   8 +
>  drivers/gpu/drm/Makefile   |   1 +
>  drivers/gpu/drm/bridge/Kconfig |   1 +
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c  |  26 +++
>  drivers/gpu/drm/drm_dp_aux_backlight.c | 191 
> +
>  include/drm/drm_dp_aux_backlight.h |  29 
>  7 files changed, 264 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_dp_aux_backlight.c
>  create mode 100644 include/drm/drm_dp_aux_backlight.h

So I haven't looked in massive detail at this patch series, but the
fact that it's touching "ti-sn65dsi86.c" is a red flag. I know in
out-of-band communications you said you weren't sure how to do better.
...but, perhaps, if folks don't hate my recent series [1] there may be
a way forward.

I wonder if perhaps now that the AUX channel can be registered early
if it gets around the circular dependency problems and now you can put
your code in some combination of the panel code and (maybe?) a new
backlight driver if it's generic enough.

It's possible that you might need to add some code to be able to look
up a "struct drm_dp_aux *" from a device tree node and you might need
to add a new device tree property like "ddc-aux-bus" in order to do
this, but I don't _think_ that would be controversial?

[1] https://lore.kernel.org/r/20210416223950.3586967-1-diand...@chromium.org

-Doug

Re: [PATCH v1] arm64: dts: qcom: sc7180: coachz: Add thermal config for skin temperature

[Doug Anderson]

Hi,

On Wed, Apr 14, 2021 at 11:10 AM Matthias Kaehlcke  wrote:
>
> Add ADC and thermal monitor configuration for skin temperature,
> plus a thermal zone that monitors the skin temperature and uses
> the big cores as cooling devices.
>
> CoachZ rev1 is stuffed with an incompatible thermistor for the
> skin temperature, disable the thermal zone for rev1 to avoid
> the use of bogus temperature values.
>
> Signed-off-by: Matthias Kaehlcke 
> ---
>
>  .../dts/qcom/sc7180-trogdor-coachz-r1.dts |  9 +++
>  .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi  | 63 +++
>  2 files changed, 72 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts
> index 86619f6c1134..80bdc4d5b523 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts
> @@ -14,6 +14,15 @@ / {
> compatible = "google,coachz-rev1", "qcom,sc7180";
>  };
>
> +/*
> + * CoachZ rev1 is stuffed with a 47k NTC as thermistor for skin temperature,
> + * which currently is not supported by the PM6150 ADC driver. Disable the
> + * skin temperature thermal zone to avoid using bogus temperature values.
> + */
> +&skin_temp_thermal {
> +   status = "disabled";
> +};
> +
>  &tlmm {
> gpio-line-names = "HUB_RST_L",
>   "AP_RAM_ID0",
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> index e2ffe71c2d52..cabe5d6b981b 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> @@ -25,6 +25,50 @@ adau7002: audio-codec-1 {
> IOVDD-supply = <&pp1800_l15a>;
> #sound-dai-cells = <0>;
> };
> +
> +   thermal-zones {
> +   skin_temp_thermal: skin-temp-thermal {
> +   polling-delay-passive = <250>;
> +   polling-delay = <0>;
> +
> +   thermal-sensors = <&pm6150_adc_tm 1>;
> +   sustainable-power = <814>;
> +
> +   trips {
> +   skin_temp_alert0: trip-point0 {
> +   temperature = <42000>;
> +   hysteresis = <1000>;
> +   type = "passive";
> +   };
> +
> +   skin_temp_alert1: trip-point1 {
> +   temperature = <45000>;
> +   hysteresis = <1000>;
> +   type = "passive";
> +   };
> +
> +   skin-temp-crit {

If it were me I would have added a label to the "crit" too to match
the alert0 and alert1. It's not needed right now, though, so it's not
worth spinning the patch for.

> +   temperature = <6>;
> +   hysteresis = <1000>;
> +   type = "critical";
> +   };
> +   };
> +
> +   cooling-maps {
> +   map0 {
> +   trip = <&skin_temp_alert0>;
> +   cooling-device = <&CPU6 
> THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +<&CPU7 
> THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +   };
> +
> +   map1 {
> +   trip = <&skin_temp_alert1>;
> +   cooling-device = <&CPU6 
> THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +<&CPU7 
> THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +   };
> +   };
> +   };
> +   };
>  };
>
>  &ap_spi_fp {
> @@ -77,6 +121,25 @@ &panel {
> compatible = "boe,nv110wtm-n61";
>  };
>
> +&pm6150_adc {
> +   skin-temp-thermistor@4e {

A part of me wonders if we should just be using the generic name
"thermistor@4e" which seems more common in dts.  ...but what you have
matches what we've already done in pompom so I think it's fine.

FWIW: I'm not an expert on any of the thermistor stuff but it looks
sane and matches how pompom is working with this thermistor...

Reviewed-by: Douglas Anderson 

Re: [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()

[Doug Anderson]

Hi,

On Wed, Apr 14, 2021 at 6:56 PM Laurent Pinchart
 wrote:
>
> Hi Doug,
>
> On Wed, Apr 14, 2021 at 06:19:13PM -0700, Doug Anderson wrote:
> > On Sun, Apr 4, 2021 at 5:50 PM Laurent Pinchart wrote:
> > > On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
> > > > The drm_bridge_chain_pre_enable() is not the proper opposite of
> > > > drm_bridge_chain_post_disable(). It continues along the chain to
> > > > _before_ the starting bridge. Let's fix that.
> > > >
> > > > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked 
> > > > list")
> > > > Signed-off-by: Douglas Anderson 
> > > > Reviewed-by: Andrzej Hajda 
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  drivers/gpu/drm/drm_bridge.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > > index 64f0effb52ac..044acd07c153 100644
> > > > --- a/drivers/gpu/drm/drm_bridge.c
> > > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > > @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge 
> > > > *bridge)
> > > >   list_for_each_entry_reverse(iter, &encoder->bridge_chain, 
> > > > chain_node) {
> > > >   if (iter->funcs->pre_enable)
> > > >   iter->funcs->pre_enable(iter);
> > > > +
> > > > + if (iter == bridge)
> > > > + break;
> > >
> > > This looks good as it matches drm_atomic_bridge_chain_disable().
> > >
> > > Reviewed-by: Laurent Pinchart 
> >
> > Thanks for your review here and several of the other patches. Can you
> > suggest any plan for getting them landed? It would at least be nice to
> > get the non-controversial ones landed.
>
> Do you have commit access to drm-misc ? If not, given your
> contributions, I think you qualify for it.

No, I don't have access. I searched for how to get it and read through
the qualifications and, you're right, I think I do. I've hopefully
followed the right flow and created an issue to give me ssh access:

https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/348

Is that something you (or someone else on this CC list) approves?


> > > I'm curious though, given that the bridge passed to the function should
> > > be the one closest to the encoder, does this make a difference ?
> >
> > Yes, that's how I discovered it originally. Let's see. So if I don't
> > have this patch but have the rest of the series then I get a splat at
> > bootup. This shows that dsi_mgr_bridge_pre_enable() must be "earlier"
> > in the chain than my bridge chip. Here's the splat:
>
> Right, I think it's caused by a later patch in the series calling this
> function with a different bridge than the one closest to the encoder.

Yup! I still wanted this patch to be first in the series, though,
since it's a bugfix that we'd want to land even if the later patches
changed in some way.

-Doug

Re: [PATCH v3 12/12] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare

[Doug Anderson]

Hi,

On Wed, Apr 14, 2021 at 5:58 PM Laurent Pinchart
 wrote:
>
> Hi Doug,
>
> Thank you for the patch.
>
> On Fri, Apr 02, 2021 at 03:28:46PM -0700, Douglas Anderson wrote:
> > Unpreparing and re-preparing a panel can be a really heavy
> > operation. Panels datasheets often specify something on the order of
> > 500ms as the delay you should insert after turning off the panel
> > before turning it on again. In addition, turning on a panel can have
> > delays on the order of 100ms - 200ms before the panel will assert HPD
> > (AKA "panel ready"). The above means that we should avoid turning a
> > panel off if we're going to turn it on again shortly.
> >
> > The above becomes a problem when we want to read the EDID of a
> > panel. The way that ordering works is that userspace wants to read the
> > EDID of the panel _before_ fully enabling it so that it can set the
> > initial mode correctly. However, we can't read the EDID until we power
> > it up. This leads to code that does this dance (like
> > ps8640_bridge_get_edid()):
> >
> > 1. When userspace requests EDID / the panel modes (through an ioctl),
> >we power on the panel just enough to read the EDID and then power
> >it off.
> > 2. Userspace then turns the panel on.
> >
> > There's likely not much time between step #1 and #2 and so we want to
> > avoid powering the panel off and on again between those two steps.
> >
> > Let's use Runtime PM to help us. We'll move the existing prepare() and
> > unprepare() to be runtime resume() and runtime suspend(). Now when we
> > want to prepare() or unprepare() we just increment or decrement the
> > refcount. We'll default to a 1 second autosuspend delay which seems
> > sane given the typical delays we see for panels.
> >
> > A few notes:
> > - It seems the existing unprepare() and prepare() are defined to be
> >   no-ops if called extra times. We'll preserve that behavior.
>
> The prepare and unprepare calls are supposed to be balanced, which
> should allow us to drop this check. Do you have a reason to suspect that
> it may not be the case ?

No, it was just code inspection. The old code definitely made an
effort to make enable of an already enabled panel a no-op and disable
of an already disabled panel a no-op. This is even before my
(somewhat) recent patch to make things timing based, though I did
touch the code.

Can I maybe suggest that getting rid of the extra check should be a
separate patch after this one? Then if it breaks someone it's easy to
just revert that one and we can still keep the runtime pm?


> > - This is a slight change in the ABI of simple panel. If something was
> >   absolutely relying on the unprepare() to happen instantly that
> >   simply won't be the case anymore. I'm not aware of anyone relying on
> >   that behavior, but if there is someone then we'll need to figure out
> >   how to enable (or disable) this new delayed behavior selectively.
> > - In order for this to work we now have a hard dependency on
> >   "PM". From memory this is a legit thing to assume these days and we
> >   don't have to find some fallback to keep working if someone wants to
> >   build their system without "PM".
>
> Sounds fine to me.
>
> The code looks good to me. Possibly with the prepared check removed,
>
> Reviewed-by: Laurent Pinchart 

Thanks!

Re: [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()

[Doug Anderson]

Hi,

On Sun, Apr 4, 2021 at 5:50 PM Laurent Pinchart
 wrote:
>
> Hi Doug,
>
> Thank you for the patch.
>
> On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
> > The drm_bridge_chain_pre_enable() is not the proper opposite of
> > drm_bridge_chain_post_disable(). It continues along the chain to
> > _before_ the starting bridge. Let's fix that.
> >
> > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked 
> > list")
> > Signed-off-by: Douglas Anderson 
> > Reviewed-by: Andrzej Hajda 
> > ---
> >
> > (no changes since v1)
> >
> >  drivers/gpu/drm/drm_bridge.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 64f0effb52ac..044acd07c153 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge 
> > *bridge)
> >   list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) 
> > {
> >   if (iter->funcs->pre_enable)
> >   iter->funcs->pre_enable(iter);
> > +
> > + if (iter == bridge)
> > + break;
>
> This looks good as it matches drm_atomic_bridge_chain_disable().
>
> Reviewed-by: Laurent Pinchart 

Thanks for your review here and several of the other patches. Can you
suggest any plan for getting them landed? It would at least be nice to
get the non-controversial ones landed.


> I'm curious though, given that the bridge passed to the function should
> be the one closest to the encoder, does this make a difference ?

Yes, that's how I discovered it originally. Let's see. So if I don't
have this patch but have the rest of the series then I get a splat at
bootup. This shows that dsi_mgr_bridge_pre_enable() must be "earlier"
in the chain than my bridge chip. Here's the splat:

 msm_dsi_host_get_phy_clk_req: unable to calc clk rate, -22
 [ cut here ]
 disp_cc_mdss_ahb_clk status stuck at 'off'
 WARNING: CPU: 7 PID: 404 at drivers/clk/qcom/clk-branch.c:92
clk_branch_toggle+0x194/0x280
 Modules linked in: joydev
 CPU: 7 PID: 404 Comm: frecon Tainted: GB 5.12.0-rc3-lockdep+ #2
 Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
 pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
 pc : clk_branch_toggle+0x194/0x280
 lr : clk_branch_toggle+0x190/0x280
 ...
 Call trace:
  clk_branch_toggle+0x194/0x280
  clk_branch2_enable+0x28/0x34
  clk_core_enable+0x2f4/0x6b4
  clk_enable+0x54/0x74
  dsi_phy_enable_resource+0x80/0xd8
  msm_dsi_phy_enable+0xa8/0x4a8
  enable_phy+0x9c/0xf4
  dsi_mgr_bridge_pre_enable+0x23c/0x4b0
  drm_bridge_chain_pre_enable+0xac/0xe4
  ti_sn_bridge_connector_get_modes+0x134/0x1b8
  drm_helper_probe_single_connector_modes+0x49c/0x1358
  drm_mode_getconnector+0x460/0xe98
  drm_ioctl_kernel+0x144/0x228
  drm_ioctl+0x418/0x7cc
  drm_compat_ioctl+0x1bc/0x230
  __arm64_compat_sys_ioctl+0x14c/0x188
  el0_svc_common+0x128/0x23c
  do_el0_svc_compat+0x50/0x60
  el0_svc_compat+0x24/0x34
  el0_sync_compat_handler+0xc0/0xf0
  el0_sync_compat+0x174/0x180

Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID

[Doug Anderson]

Hi,

On Tue, Apr 6, 2021 at 9:52 AM Andrzej Hajda  wrote:
>
> Hello again after easter,

Sorry, I was out last week and I'm just getting back to this now.


> I have looked little bit more at sn65* driver and its application to
> have better background.
>
> I miss only info what panel do you have, how it is enabled/power controlled.

I am personally working on "sc7180-trogdor" boards right now
(arch/arm64/boot/dts/qcom/sc7180-trogdor*.dts). However I believe that
this patch series also enables proper EDID reading on
"sdm850-lenovo-yoga-c630.dts". That board, while also a Qualcomm
board, has completely different heritage than the trogdor ones. It's a
laptop that ships with Windows and (as far as I know) was designed
mostly independently.

On the trogdor boards the bridge has some power rails and an enable
line hooked up to it and the bridge itself can work when these rails
are turned on. The panel is on a separate power rail and you can't
talk to the panel at all until it's powered on and then asserts HPD to
us to say it finished its boot sequence.


> W dniu 01.04.2021 o 16:57, Doug Anderson pisze:
> > Hi,
> >
> > On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda  wrote:
> >>
> >> W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
> >>> Hi,
> >>>
> >>> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda  wrote:
> >>>> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> >>>>> eDP panels won't provide their EDID unless they're powered on. Let's
> >>>>> chain a power-on before we read the EDID. This roughly matches what
> >>>>> was done in 'parade-ps8640.c'.
> >>>>>
> >>>>> NOTE: The old code attempted to call pm_runtime_get_sync() before
> >>>>> reading the EDID. While that was enough to power the bridge chip on,
> >>>>> it wasn't enough to talk to the panel for two reasons:
> >>>>> 1. Since we never ran the bridge chip's pre-enable then we never set
> >>>>>   the bit to ignore HPD. This meant the bridge chip didn't even 
> >>>>> _try_
> >>>>>   to go out on the bus and communicate with the panel.
> >>>>> 2. Even if we fixed things to ignore HPD, the EDID still wouldn't read
> >>>>>   if the panel wasn't on.
> >>>>>
> >>>>> One thing that's a bit odd here is taking advantage of the EDID that
> >>>>> the core might have cached for us. See the patch ("drm/edid: Use the
> >>>>> cached EDID in drm_get_edid() if eDP"). We manage to get at the cache
> >>>>> by:
> >>>>> - Instantly failing aux transfers if we're not powered.
> >>>>> - If the first read of the EDID fails we try again after powering.
> >>>>>
> >>>>> Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over 
> >>>>> DDC")
> >>>>> Signed-off-by: Douglas Anderson 
> >>>>> ---
> >>>>> Depending on what people think of the other patches in this series,
> >>>>> some of this could change.
> >>>>> - If everyone loves the "runtime PM" in the panel driver then we
> >>>>>  could, in theory, put the pre-enable chaining straight in the "aux
> >>>>>  transfer" function.
> >>>>> - If everyone hates the EDID cache moving to the core then we can
> >>>>>  avoid some of the awkward flow of things and keep the EDID cache in
> >>>>>  the sn65dsi86 driver.
> >>>> I wonder if this shouldn't be solved in the core - ie caller of
> >>>> get_modes callback should be responsible for powering up the pipeline,
> >>>> otherwise we need to repeat this stuff in every bridge/panel driver.
> >>>>
> >>>> Any thoughts?
> >>> Yeah, I did look at this a little bit. Presumably it would only make
> >>> sense to do it for eDP connections since:
> >>>
> >>> a) The concept of reading an EDID doesn't make sense for things like MIPI.
> >> I guess you mean MIPI DSI
> > Yes, sorry! I'll try to be more clear.
> >
> >
> >> and yes I agree, more generally it usually(!)
> >> doesn't make sense for any setup with fixed display panel.
> >>
> >> On the other hand there are DSI/HDMI or DSI/DP adapters which usually
> >> have EDID re

Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card

[Doug Anderson]

Hi,

On Tue, Apr 13, 2021 at 3:59 AM  wrote:
>
> >> >>> +   required-opps =
> >> >>> <&rpmhpd_opp_low_svs>;
> >> >>> +   opp-peak-kBps = <120
> >> >>> 76000>;
> >> >>> +   opp-avg-kBps = <120
> >> >>> 5>;
> >> >> Why are the kBps numbers so vastly different than the ones on sc7180
> >> >> for the same OPP point. That implies:
> >> >>
> >> >> a) sc7180 is wrong.
> >> >>
> >> >> b) This patch is wrong.
> >> >>
> >> >> c) The numbers are essentially random and don't really matter.
> >> >>
> >> >> Can you identify which of a), b), or c) is correct, or propose an
> >> >> alternate explanation of the difference?
> >> >>
> >>
> >> We calculated bus votes values for both sc7180 and sc7280 with ICB
> >> tool,
> >> above mentioned values we got for sc7280.
> >
> > I don't know what an ICB tool is. Please clarify.
> >
> > Also: just because a tool spits out numbers that doesn't mean it's
> > correct. Presumably the tool could be wrong or incorrectly configured.
> > We need to understand why these numbers are different.
> >
> we checked with ICB tool team on this they conformed as Rennell & Kodiak
> are different chipsets,
> we might see delta in ib/ab values due to delta in scaling factors.

...but these numbers are in kbps, aren't they? As I understand it
these aren't supposed to be random numbers spit out by a tool but are
supposed to be understandable by how much bandwidth an IP block (like
MMC) needs from the busses it's connected to. Since the MMC IP block
on sc7180 and sc7280 is roughly the same there shouldn't be a big
difference in numbers.

Something smells wrong.

Adding a few people who understand interconnects better than I do, though.

-Doug

Re: [PATCH] arm64: dts: qcom: sc7180: Fix sc7180-qmp-usb3-dp-phy reg sizes

[Doug Anderson]

Bjorn,

On Mon, Mar 15, 2021 at 10:39 AM Douglas Anderson  wrote:
>
> As per Dmitry Baryshkov [1]:
> a) The 2nd "reg" should be 0x3c because "Offset 0x38 is
>USB3_DP_COM_REVISION_ID3 (not used by the current driver though)."
> b) The 3rd "reg" "is a serdes region and qmp_v3_dp_serdes_tbl contains
>registers 0x148 and 0x154."
>
> I think because the 3rd "reg" is a serdes region we should just use
> the same size as the 1st "reg"?
>
> [1] https://lore.kernel.org/r/ee5695bb-a603-0dd5-7a7f-695e919b1...@linaro.org
>
> Cc: Stephen Boyd 
> Cc: Jeykumar Sankaran 
> Cc: Chandan Uddaraju 
> Cc: Vara Reddy 
> Cc: Tanmay Shah 
> Cc: Rob Clark 
> Fixes: 58fd7ae621e7 ("arm64: dts: qcom: sc7180: Update dts for DP phy inside 
> QMP phy")
> Reported-by: Dmitry Baryshkov 
> Signed-off-by: Douglas Anderson 
> ---
>
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Another wayward patch that got lost. Could you add it to your 5.14
queue? It's not very urgent but it'd be nice to clean it up
eventually.

-Doug

Re: [PATCH v3] HID: i2c-hid: Skip ELAN power-on command after reset

[Doug Anderson]

Hi,

On Mon, Apr 12, 2021 at 6:20 PM Johnny Chuang
 wrote:
>
> Fixes: 43b7029f475e ("HID: i2c-hid: Send power-on command after reset").

Note that the "Fixes" tag actually belongs down at the end. It also
shouldn't have a "." at the end. Presumably the maintainer can adjust
this when landing?


> For ELAN touchscreen, we found our boot code of IC was not flexible enough
> to receive and handle this command.
> Once the FW main code of our controller is crashed for some reason,
> the controller could not be enumerated successfully to be recognized
> by the system host. therefore, it lost touch functionality.
>
> Add quirk for skip send power-on command after reset.
> It will impact to ELAN touchscreen and touchpad on HID over I2C projects.
>
> Signed-off-by: Johnny Chuang 

This patch looks fine to me, thus:

Reviewed-by: Douglas Anderson 

I can confirm that after applying this patch I can recovery my borked
touchscreen (which got borked by a failed firmware update ages ago):

Tested-by: Douglas Anderson 

Re: [PATCH v2 1/2] arm64: dts: qcom: Add "dmic_clk_en" for sc7180-trogdor-coachz

[Doug Anderson]

Bjorn,

On Mon, Mar 15, 2021 at 1:39 PM Douglas Anderson  wrote:
>
> This was present downstream. Add upstream too. NOTE: upstream I
> managed to get some sort of halfway state and got one pinctrl entry in
> the coachz-r1 device tree. Remove that as part of this since it's now
> in the dtsi.
>
> Cc: Srinivasa Rao Mandadapu 
> Cc: Ajit Pandey 
> Cc: Judy Hsiao 
> Cc: Cheng-Yi Chiang 
> Cc: Stephen Boyd 
> Cc: Matthias Kaehlcke 
> Signed-off-by: Douglas Anderson 
> ---
> This applies atop the patch ("arm64: dts: qcom: Add sound node for
> sc7180-trogdor-coachz") [1].
>
> NOTE: downstream this property was present in each of the board
> revisions. There's actually no longer any reason for this and I'll
> shortly post a downstream patch to fix this.
>
> [1] https://lore.kernel.org/r/20210313054654.11693-3-sriva...@codeaurora.org/
>
> Changes in v2:
> - Remove the pinctrl from the -r1
>
>  .../boot/dts/qcom/sc7180-trogdor-coachz-r1.dts   | 13 -
>  .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi | 16 
>  2 files changed, 16 insertions(+), 13 deletions(-)

I guess this patch missed the boat for 5.13? Can it get queued up for
5.14 whenever that happens?

Thanks!

-Doug

Re: [PATCH v2] drm/msm: Drop mm_lock in scan loop

[Doug Anderson]

Hi,

On Fri, Apr 2, 2021 at 2:08 PM Rob Clark  wrote:
>
> From: Rob Clark 
>
> lock_stat + mmm_donut[1] say that this reduces contention on mm_lock
> significantly (~350x lower waittime-max, and ~100x lower waittime-avg)
>
> [1] 
> https://chromium.googlesource.com/chromiumos/platform/microbenchmarks/+/refs/heads/main/mmm_donut.py
>
> Signed-off-by: Rob Clark 
> ---
>  drivers/gpu/drm/msm/msm_drv.h  |  3 +-
>  drivers/gpu/drm/msm/msm_gem.c  |  2 +-
>  drivers/gpu/drm/msm/msm_gem_shrinker.c | 48 ++
>  3 files changed, 45 insertions(+), 8 deletions(-)

Looks good to me now!

Reviewed-by: Douglas Anderson 

Re: [PATCH v2 2/4] drm/msm: Avoid mutex in shrinker_count()

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 6:24 PM Rob Clark  wrote:
>
> @@ -45,6 +30,9 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct 
> shrink_control *sc)
> list_for_each_entry(msm_obj, &priv->inactive_dontneed, mm_list) {
> if (freed >= sc->nr_to_scan)
> break;
> +   /* Use trylock, because we cannot block on a obj that
> +* might be trying to acquire mm_lock
> +*/

nit: I thought the above multi-line commenting style was only for
"net" subsystem?

> if (!msm_gem_trylock(&msm_obj->base))
> continue;
> if (is_purgeable(msm_obj)) {
> @@ -56,8 +44,11 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct 
> shrink_control *sc)
>
> mutex_unlock(&priv->mm_lock);
>
> -   if (freed > 0)
> +   if (freed > 0) {
> trace_msm_gem_purge(freed << PAGE_SHIFT);
> +   } else {
> +   return SHRINK_STOP;
> +   }

It probably doesn't matter, but I wonder if we should still be
returning SHRINK_STOP if we got any trylock failures. It could
possibly be worth returning 0 in that case?


> @@ -75,6 +66,9 @@ vmap_shrink(struct list_head *mm_list)
> unsigned unmapped = 0;
>
> list_for_each_entry(msm_obj, mm_list, mm_list) {
> +   /* Use trylock, because we cannot block on a obj that
> +* might be trying to acquire mm_lock
> +*/

If you end up changing the commenting style above, should also be here.

At this point this seems fine to land to me. Though I'm not an expert
on every interaction in this code, I've spent enough time starting at
it that I'm comfortable with:

Reviewed-by: Douglas Anderson 

Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID

[Doug Anderson]

Hi,

On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda  wrote:
>
>
> W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
> > Hi,
> >
> > On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda  wrote:
> >>
> >> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> >>> eDP panels won't provide their EDID unless they're powered on. Let's
> >>> chain a power-on before we read the EDID. This roughly matches what
> >>> was done in 'parade-ps8640.c'.
> >>>
> >>> NOTE: The old code attempted to call pm_runtime_get_sync() before
> >>> reading the EDID. While that was enough to power the bridge chip on,
> >>> it wasn't enough to talk to the panel for two reasons:
> >>> 1. Since we never ran the bridge chip's pre-enable then we never set
> >>>  the bit to ignore HPD. This meant the bridge chip didn't even _try_
> >>>  to go out on the bus and communicate with the panel.
> >>> 2. Even if we fixed things to ignore HPD, the EDID still wouldn't read
> >>>  if the panel wasn't on.
> >>>
> >>> One thing that's a bit odd here is taking advantage of the EDID that
> >>> the core might have cached for us. See the patch ("drm/edid: Use the
> >>> cached EDID in drm_get_edid() if eDP"). We manage to get at the cache
> >>> by:
> >>> - Instantly failing aux transfers if we're not powered.
> >>> - If the first read of the EDID fails we try again after powering.
> >>>
> >>> Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC")
> >>> Signed-off-by: Douglas Anderson 
> >>> ---
> >>> Depending on what people think of the other patches in this series,
> >>> some of this could change.
> >>> - If everyone loves the "runtime PM" in the panel driver then we
> >>> could, in theory, put the pre-enable chaining straight in the "aux
> >>> transfer" function.
> >>> - If everyone hates the EDID cache moving to the core then we can
> >>> avoid some of the awkward flow of things and keep the EDID cache in
> >>> the sn65dsi86 driver.
> >>
> >> I wonder if this shouldn't be solved in the core - ie caller of
> >> get_modes callback should be responsible for powering up the pipeline,
> >> otherwise we need to repeat this stuff in every bridge/panel driver.
> >>
> >> Any thoughts?
> > Yeah, I did look at this a little bit. Presumably it would only make
> > sense to do it for eDP connections since:
> >
> > a) The concept of reading an EDID doesn't make sense for things like MIPI.
>
> I guess you mean MIPI DSI

Yes, sorry! I'll try to be more clear.


> and yes I agree, more generally it usually(!)
> doesn't make sense for any setup with fixed display panel.
>
> On the other hand there are DSI/HDMI or DSI/DP adapters which usually
> have EDID reading logic.
>
> And the concept makes sense for most connectors accepting external
> displays: HDMI, DP, MHL, VGA...

So, actually, IMO the concept doesn't make sense for anything with an
external connector. Here's the logic for a handful of connectors:

1. MIPI DSI: there is no EDID so this doesn't make sense.

2. An external connector (HDMI, DP, etc): the display that's plugged
in is externally powered so doesn't need us to power it up to read the
EDID. By definition, when the HPD signal is asserted then it's OK to
read the EDID and we don't even know if a display is plugged in until
HPD is asserted. Thus no special power sequencing is needed to read
the EDID.  (Yes, we need to make sure that the eDP controller itself
is powered, but that doesn't seem like it's the core's business).

3. eDP: this is where it matters. This is because:

3a) eDP displays aren't powered all the time. If you just boot up or
you blank your screen, likely the display has no power at all.

3b) Because the display has no power, the "HPD" signal doesn't assert.
In fact, for eDP the "HPD" signal really should mean "display ready"
or "display finished powering up".

3c) Even though we never get a HPD signal, we still simply assume that
a display is present because this is an "embedded" device.

So eDP is unique (as far as I know) in that it's a type of display
that has an EDID but that we will report "a display is here" before
we've powered up the display and before we can read the EDID.


> > b) For something with an external connector (DP and

Re: [PATCH v2 1/4] drm/msm: Remove unused freed llist node

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 6:23 PM Rob Clark  wrote:
>
> From: Rob Clark 
>
> Unused since commit c951a9b284b9 ("drm/msm: Remove msm_gem_free_work")
>
> Signed-off-by: Rob Clark 
> Tested-by: Douglas Anderson 
> ---
>  drivers/gpu/drm/msm/msm_gem.h | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH v2 3/4] drm/msm: Fix debugfs deadlock

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 6:24 PM Rob Clark  wrote:
>
> From: Rob Clark 
>
> In normal cases the gem obj lock is acquired first before mm_lock.  The
> exception is iterating the various object lists.  In the shrinker path,
> deadlock is avoided by using msm_gem_trylock() and skipping over objects
> that cannot be locked.  But for debugfs the straightforward thing is to
> split things out into a separate list of all objects protected by it's
> own lock.
>
> Fixes: d984457b31c4 ("drm/msm: Add priv->mm_lock to protect active/inactive 
> lists")
> Signed-off-by: Rob Clark 
> Tested-by: Douglas Anderson 

Reviewed-by: Douglas Anderson 

Re: [PATCH 1/4] drm/msm: Remove unused freed llist node

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 3:14 PM Rob Clark  wrote:
>
> From: Rob Clark 
>
> Unused since c951a9b284b907604759628d273901064c60d09f

Not terribly important, but checkpatch always yells at me when I don't
reference commits by saying:

commit c951a9b284b9 ("drm/msm: Remove msm_gem_free_work")


> Signed-off-by: Rob Clark 
> ---
>  drivers/gpu/drm/msm/msm_gem.h | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH 0/4] drm/msm: Shrinker (and related) fixes

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 3:14 PM Rob Clark  wrote:
>
> From: Rob Clark 
>
> I've been spending some time looking into how things behave under high
> memory pressure.  The first patch is a random cleanup I noticed along
> the way.  The second improves the situation significantly when we are
> getting shrinker called from many threads in parallel.  And the last
> two are $debugfs/gem fixes I needed so I could monitor the state of GEM
> objects (ie. how many are active/purgable/purged) while triggering high
> memory pressure.
>
> We could probably go a bit further with dropping the mm_lock in the
> shrinker->scan() loop, but this is already a pretty big improvement.
> The next step is probably actually to add support to unpin/evict
> inactive objects.  (We are part way there since we have already de-
> coupled the iova lifetime from the pages lifetime, but there are a
> few sharp corners to work through.)
>
> Rob Clark (4):
>   drm/msm: Remove unused freed llist node
>   drm/msm: Avoid mutex in shrinker_count()
>   drm/msm: Fix debugfs deadlock
>   drm/msm: Improved debugfs gem stats
>
>  drivers/gpu/drm/msm/msm_debugfs.c  | 14 ++
>  drivers/gpu/drm/msm/msm_drv.c  |  4 ++
>  drivers/gpu/drm/msm/msm_drv.h  | 10 -
>  drivers/gpu/drm/msm/msm_fb.c   |  3 +-
>  drivers/gpu/drm/msm/msm_gem.c  | 61 +-
>  drivers/gpu/drm/msm/msm_gem.h  | 58 +---
>  drivers/gpu/drm/msm/msm_gem_shrinker.c | 17 +--
>  7 files changed, 122 insertions(+), 45 deletions(-)

This makes a pretty big reduction in jankiness when under memory
pressure and seems to work well for me.

Tested-by: Douglas Anderson 

Re: [PATCH 2/4] drm/msm: Avoid mutex in shrinker_count()

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 4:23 PM Rob Clark  wrote:
>
> On Wed, Mar 31, 2021 at 3:44 PM Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Wed, Mar 31, 2021 at 3:14 PM Rob Clark  wrote:
> > >
> > > @@ -818,11 +820,19 @@ static void update_inactive(struct msm_gem_object 
> > > *msm_obj)
> > > mutex_lock(&priv->mm_lock);
> > > WARN_ON(msm_obj->active_count != 0);
> > >
> > > +   if (msm_obj->dontneed)
> > > +   mark_unpurgable(msm_obj);
> > > +
> > > list_del_init(&msm_obj->mm_list);
> > > -   if (msm_obj->madv == MSM_MADV_WILLNEED)
> > > +   if (msm_obj->madv == MSM_MADV_WILLNEED) {
> > > list_add_tail(&msm_obj->mm_list, 
> > > &priv->inactive_willneed);
> > > -   else
> > > +   } else if (msm_obj->madv == MSM_MADV_DONTNEED) {
> > > list_add_tail(&msm_obj->mm_list, 
> > > &priv->inactive_dontneed);
> > > +   mark_purgable(msm_obj);
> > > +   } else {
> > > +   WARN_ON(msm_obj->madv != __MSM_MADV_PURGED);
> > > +   list_add_tail(&msm_obj->mm_list, &priv->inactive_purged);
> >
> > I'm probably being dense, but what's the point of adding it to the
> > "inactive_purged" list here? You never look at that list, right? You
> > already did a list_del_init() on this object's list pointer
> > ("mm_list"). I don't see how adding it to a bogus list helps with
> > anything.
>
> It preserves the "every bo is in one of these lists" statement, but
> other than that you are right we aren't otherwise doing anything with
> that list.  (Or we could replace the list_del_init() with list_del()..
> I tend to instinctively go for list_del_init())

If you really want this list, it wouldn't hurt to at least have a
comment saying that it's not used for anything so people like me doing
go trying to figure out what it's used for. ;-)


> > > @@ -198,6 +203,33 @@ static inline bool is_vunmapable(struct 
> > > msm_gem_object *msm_obj)
> > > return (msm_obj->vmap_count == 0) && msm_obj->vaddr;
> > >  }
> > >
> > > +static inline void mark_purgable(struct msm_gem_object *msm_obj)
> > > +{
> > > +   struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
> > > +
> > > +   WARN_ON(!mutex_is_locked(&priv->mm_lock));
> > > +
> > > +   if (WARN_ON(msm_obj->dontneed))
> > > +   return;
> >
> > The is_purgeable() function also checks other things besides just
> > "MSM_MADV_DONTNEED". Do we need to check those too? Specifically:
> >
> >  msm_obj->sgt && !msm_obj->base.dma_buf && !msm_obj->base.import_attach
> >
> > ...or is it just being paranoid?
> >
> > I guess I'm just worried that if any of those might be important then
> > we'll consistently report back that we have a count of things that can
> > be purged but then scan() won't find anything to do. That wouldn't be
> > great.
>
> Hmm, I thought msm_gem_madvise() returned an error instead of allowing
> MSM_MADV_DONTNEED to be set on imported/exported dma-bufs.. it
> probably should to be complete (but userspace already knows not to
> madvise an imported/exported buffer for other reasons.. ie. we can't
> let a shared buffer end up in the bo cache).  I'll re-work that a bit.
>
> The msm_obj->sgt case is a bit more tricky.. that will be the case of
> a freshly allocated obj that does not have backing patches yet.  But
> it seems like enough of a corner case, that I'm happy to live with
> it.. ie. the tricky thing is not leaking decrements of
> priv->shrinkable_count or underflowing priv->shrinkable_count, and
> caring about the !msm_obj->sgt case doubles the number of states an
> object can be in, and the shrinker->count() return value is just an
> estimate.

I think it's equally important to make sure that we don't constantly
have a non-zero count and then have scan() do nothing.  If there's a
transitory blip then it's fine, but it's not OK if it can be steady
state. Then you end up with:

1. How many objects do you have to free? 10
2. OK, free some. How many did you free? 0
3. Oh. You got more to do, I'll call you again.
4. Goto #1

...and it just keeps looping, right?

As long as you're confident that this case can't happen then we

Re: [PATCH 4/4] drm/msm: Improved debugfs gem stats

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 3:14 PM Rob Clark  wrote:
>
> From: Rob Clark 
>
> The last patch lost the breakdown of active vs inactive GEM objects in
> $debugfs/gem.  But we can add some better stats to summarize not just
> active vs inactive, but also purgable/purged to make up for that.
>
> Signed-off-by: Rob Clark 
> ---
>  drivers/gpu/drm/msm/msm_fb.c  |  3 ++-
>  drivers/gpu/drm/msm/msm_gem.c | 31 ---
>  drivers/gpu/drm/msm/msm_gem.h | 11 ++-
>  3 files changed, 36 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
> index d42f0665359a..887172a10c9a 100644
> --- a/drivers/gpu/drm/msm/msm_fb.c
> +++ b/drivers/gpu/drm/msm/msm_fb.c
> @@ -33,6 +33,7 @@ static const struct drm_framebuffer_funcs 
> msm_framebuffer_funcs = {
>  #ifdef CONFIG_DEBUG_FS
>  void msm_framebuffer_describe(struct drm_framebuffer *fb, struct seq_file *m)
>  {
> +   struct msm_gem_stats stats = {{0}};

nit: instead of "{{0}}", can't you just do:

struct msm_gem_stats stats = {};

...both here and for the other usage.

Other than that this seems good to me.

Reviewed-by: Douglas Anderson 

Re: [PATCH 3/4] drm/msm: Fix debugfs deadlock

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 3:14 PM Rob Clark  wrote:
>
> @@ -111,23 +111,15 @@ static const struct file_operations msm_gpu_fops = {
>  static int msm_gem_show(struct drm_device *dev, struct seq_file *m)
>  {
> struct msm_drm_private *priv = dev->dev_private;
> -   struct msm_gpu *gpu = priv->gpu;
> int ret;
>
> -   ret = mutex_lock_interruptible(&priv->mm_lock);
> +   ret = mutex_lock_interruptible(&priv->obj_lock);
> if (ret)
> return ret;
>
> -   if (gpu) {
> -   seq_printf(m, "Active Objects (%s):\n", gpu->name);
> -   msm_gem_describe_objects(&gpu->active_list, m);
> -   }
> -
> -   seq_printf(m, "Inactive Objects:\n");
> -   msm_gem_describe_objects(&priv->inactive_dontneed, m);
> -   msm_gem_describe_objects(&priv->inactive_willneed, m);
> +   msm_gem_describe_objects(&priv->objects, m);

I guess we no longer sort the by Active and Inactive but that doesn't
really matter?


> @@ -174,7 +174,13 @@ struct msm_drm_private {
> struct msm_rd_state *hangrd;   /* debugfs to dump hanging submits */
> struct msm_perf_state *perf;
>
> -   /*
> +   /**
> +* List of all GEM objects (mainly for debugfs, protected by obj_lock

It wouldn't hurt to talk about lock ordering here? Like: "If we need
the "obj_lock" and a "gem_lock" at the same time we always grab the
"obj_lock" first.

> @@ -60,13 +60,20 @@ struct msm_gem_object {
>  */
> uint8_t vmap_count;
>
> -   /* And object is either:
> -*  inactive - on priv->inactive_list
> +   /**
> +* Node in list of all objects (mainly for debugfs, protected by
> +* struct_mutex

Not "struct_mutex" in comment, right? Maybe "obj_lock" I think?

-Doug

Re: [PATCH 2/4] drm/msm: Avoid mutex in shrinker_count()

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 3:14 PM Rob Clark  wrote:
>
> @@ -818,11 +820,19 @@ static void update_inactive(struct msm_gem_object 
> *msm_obj)
> mutex_lock(&priv->mm_lock);
> WARN_ON(msm_obj->active_count != 0);
>
> +   if (msm_obj->dontneed)
> +   mark_unpurgable(msm_obj);
> +
> list_del_init(&msm_obj->mm_list);
> -   if (msm_obj->madv == MSM_MADV_WILLNEED)
> +   if (msm_obj->madv == MSM_MADV_WILLNEED) {
> list_add_tail(&msm_obj->mm_list, &priv->inactive_willneed);
> -   else
> +   } else if (msm_obj->madv == MSM_MADV_DONTNEED) {
> list_add_tail(&msm_obj->mm_list, &priv->inactive_dontneed);
> +   mark_purgable(msm_obj);
> +   } else {
> +   WARN_ON(msm_obj->madv != __MSM_MADV_PURGED);
> +   list_add_tail(&msm_obj->mm_list, &priv->inactive_purged);

I'm probably being dense, but what's the point of adding it to the
"inactive_purged" list here? You never look at that list, right? You
already did a list_del_init() on this object's list pointer
("mm_list"). I don't see how adding it to a bogus list helps with
anything.


> @@ -198,6 +203,33 @@ static inline bool is_vunmapable(struct msm_gem_object 
> *msm_obj)
> return (msm_obj->vmap_count == 0) && msm_obj->vaddr;
>  }
>
> +static inline void mark_purgable(struct msm_gem_object *msm_obj)
> +{
> +   struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
> +
> +   WARN_ON(!mutex_is_locked(&priv->mm_lock));
> +
> +   if (WARN_ON(msm_obj->dontneed))
> +   return;

The is_purgeable() function also checks other things besides just
"MSM_MADV_DONTNEED". Do we need to check those too? Specifically:

 msm_obj->sgt && !msm_obj->base.dma_buf && !msm_obj->base.import_attach

...or is it just being paranoid?

I guess I'm just worried that if any of those might be important then
we'll consistently report back that we have a count of things that can
be purged but then scan() won't find anything to do. That wouldn't be
great.


> +   priv->shrinkable_count += msm_obj->base.size >> PAGE_SHIFT;
> +   msm_obj->dontneed = true;
> +}
> +
> +static inline void mark_unpurgable(struct msm_gem_object *msm_obj)
> +{
> +   struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
> +
> +   WARN_ON(!mutex_is_locked(&priv->mm_lock));
> +
> +   if (WARN_ON(!msm_obj->dontneed))
> +   return;
> +
> +   priv->shrinkable_count -= msm_obj->base.size >> PAGE_SHIFT;
> +   WARN_ON(priv->shrinkable_count < 0);

If you changed the order maybe you could make shrinkable_count
"unsigned long" to match the shrinker API?

 new_shrinkable = msm_obj->base.size >> PAGE_SHIFT;
 WARN_ON(new_shrinkable > priv->shrinkable_count);
 priv->shrinkable_count -= new_shrinkable


-Doug

Re: [PATCH v2 05/14] drm/bridge: ti-sn65dsi86: Move MIPI detach() / unregister() to detach()

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 2:53 AM Andrzej Hajda  wrote:
>
>
> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> > The register() / attach() for MIPI happen in the bridge's
> > attach(). That means that the inverse belongs in the bridge's
> > detach().
>
>
> As I commented in previous patch, it would be better to fix mipi/bridge
> registration order in host and this driver.
>
> Have you considered this?

Fair enough. How about I drop this patch at the moment? My series
already has enough stuff in it right now and I don't believe anything
in the series depends on this patch.

-Doug

Re: [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 4:27 AM Kalyan Thota  wrote:
>
> @@ -294,6 +294,9 @@ static int dpu_kms_parse_data_bus_icc_path(struct dpu_kms 
> *dpu_kms)
> struct icc_path *path1;
> struct drm_device *dev = dpu_kms->dev;
>
> +   if (!dpu_supports_bw_scaling(dev))
> +   return 0;
> +
> path0 = of_icc_get(dev->dev, "mdp0-mem");
> path1 = of_icc_get(dev->dev, "mdp1-mem");
>

Instead of hard coding a check for specific SoC compatible strings,
why not just check to see if path0 and/or path1 are ERR_PTR(-ENODEV)?
Then change dpu_supports_bw_scaling() to just return:

!IS_ERR(dpu_kms->path[0])

It also seems like it would be nice if you did something if you got an
error other than -ENODEV. Right now this function returns it but the
caller ignores it? At least spit an error message out?


> @@ -154,6 +154,15 @@ struct vsync_info {
>
>  #define to_dpu_global_state(x) container_of(x, struct dpu_global_state, base)
>
> +/**
> + * dpu_supports_bw_scaling: returns true for drivers that support bw scaling.
> + * @dev: Pointer to drm_device structure
> + */
> +static inline int dpu_supports_bw_scaling(struct drm_device *dev)
> +{
> +   return of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss");

See above, but I think this would be better as:

  return !IS_ERR(dpu_kms->path[0]);

Specifically, I don't think of_device_is_compatible() is really
designed as something to call a lot. It's doing a whole bunch of data
structure parsing / string comparisons. It's OK-ish during probe
(though better to use the of_match_table), but you don't want to call
it on every runtime suspend / runtime resume.

Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda  wrote:
>
>
> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> > eDP panels won't provide their EDID unless they're powered on. Let's
> > chain a power-on before we read the EDID. This roughly matches what
> > was done in 'parade-ps8640.c'.
> >
> > NOTE: The old code attempted to call pm_runtime_get_sync() before
> > reading the EDID. While that was enough to power the bridge chip on,
> > it wasn't enough to talk to the panel for two reasons:
> > 1. Since we never ran the bridge chip's pre-enable then we never set
> > the bit to ignore HPD. This meant the bridge chip didn't even _try_
> > to go out on the bus and communicate with the panel.
> > 2. Even if we fixed things to ignore HPD, the EDID still wouldn't read
> > if the panel wasn't on.
> >
> > One thing that's a bit odd here is taking advantage of the EDID that
> > the core might have cached for us. See the patch ("drm/edid: Use the
> > cached EDID in drm_get_edid() if eDP"). We manage to get at the cache
> > by:
> > - Instantly failing aux transfers if we're not powered.
> > - If the first read of the EDID fails we try again after powering.
> >
> > Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC")
> > Signed-off-by: Douglas Anderson 
> > ---
> > Depending on what people think of the other patches in this series,
> > some of this could change.
> > - If everyone loves the "runtime PM" in the panel driver then we
> >could, in theory, put the pre-enable chaining straight in the "aux
> >transfer" function.
> > - If everyone hates the EDID cache moving to the core then we can
> >avoid some of the awkward flow of things and keep the EDID cache in
> >the sn65dsi86 driver.
>
>
> I wonder if this shouldn't be solved in the core - ie caller of
> get_modes callback should be responsible for powering up the pipeline,
> otherwise we need to repeat this stuff in every bridge/panel driver.
>
> Any thoughts?

Yeah, I did look at this a little bit. Presumably it would only make
sense to do it for eDP connections since:

a) The concept of reading an EDID doesn't make sense for things like MIPI.

b) For something with an external connector (DP and HDMI) you don't
even know they're inserted unless the EDID is ready to read (these
devices are, essentially, always powered).

So I started off trying to do this in the core for eDP, but then it
wasn't completely clear how to write this code in a way that was super
generic. Specifically:

1. I don't think it's a 100% guarantee that everything is powered on
in pre-enable and powered off in post-disable. In this bridge chip
it's true, but maybe not every eDP driver? Would you want me to just
assume this, or add a flag?

2. It wasn't totally clear to me which state to use for telling if the
bridge had already been pre-enabled so I could avoid double-calling
it. I could dig more if need be but I spent a bit of time looking and
was coming up empty. If you have advice I'd appreciate it, though.

3. It wasn't clear to me if I should be using the atomic version
(drm_atomic_bridge_chain_pre_enable) if I put this in the core and how
exactly to do this, though I am a self-admitted DRM noob. I can do
more digging if need be. Again, advice is appreciated.

4. Since I got feedback that the EDID caching belongs in the driver,
not in the core [1] then we might end up powering things up
pointlessly since the core wouldn't know if the driver was going to
return the cache or not.

Given that this patch isn't too much code and not too complicated (and
will be even less complicated if I move the EDID caching back into the
driver), maybe we can land it and if we see the pattern repeat a bunch
more times then think about moving it to the core?


[1] https://lore.kernel.org/dri-devel/ygmvo3pndcibm...@intel.com/

-Doug

Re: [PATCH v2 10/14] drm/bridge: ti-sn65dsi86: Stop caching the EDID ourselves

[Doug Anderson]

Hi,

On Wed, Mar 31, 2021 at 3:12 AM Andrzej Hajda  wrote:
>
>
> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> > Now that we have the patch ("drm/edid: Use the cached EDID in
> > drm_get_edid() if eDP") we no longer need to maintain our own
> > cache. Drop this code.
> >
> > Signed-off-by: Douglas Anderson 
> Reviewed-by: Andrzej Hajda 

Note that unless the advice given to me changes, I'm dropping
${SUBJECT} patch on the next spin and putting the EDID cache back in
the bridge driver. See:

https://lore.kernel.org/r/ygmvo3pndcibm...@intel.com/


-Doug

Re: [PATCH v2 09/14] drm/edid: Use the cached EDID in drm_get_edid() if eDP

[Doug Anderson]

Hi,

On Tue, Mar 30, 2021 at 7:01 AM Ville Syrjälä
 wrote:
>
> > @@ -2049,15 +2049,39 @@ struct edid *drm_get_edid(struct drm_connector 
> > *connector,
> > struct i2c_adapter *adapter)
> >  {
> >   struct edid *edid;
> > + size_t old_edid_size;
> > + const struct edid *old_edid;
> >
> >   if (connector->force == DRM_FORCE_OFF)
> >   return NULL;
> >
> > - if (connector->force == DRM_FORCE_UNSPECIFIED && 
> > !drm_probe_ddc(adapter))
> > - return NULL;
> > + if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
> > + connector->edid_blob_ptr) {
> > + /*
> > +  * eDP devices are non-removable, or at least not something
> > +  * that's expected to be hot-pluggable. We can freely use
> > +  * the cached EDID.
> > +  *
> > +  * NOTE: technically we could probably even use the cached
> > +  * EDID even for non-eDP because the cached EDID should be
> > +  * cleared if we ever notice a display is not connected, but
> > +  * we'll use an abundance of caution and only do it for eDP.
> > +  * It's more important for eDP anyway because the EDID may not
> > +  * always be readable, like when the panel is powered down.
> > +  */
> > + old_edid = (const struct edid 
> > *)connector->edid_blob_ptr->data;
> > + old_edid_size = ksize(old_edid);
> > + edid = kmalloc(old_edid_size, GFP_KERNEL);
> > + if (edid)
> > + memcpy(edid, old_edid, old_edid_size);
> > + } else {
> > + if (connector->force == DRM_FORCE_UNSPECIFIED && 
> > !drm_probe_ddc(adapter))
> > + return NULL;
> > +
> > + edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, 
> > adapter);
> > + drm_connector_update_edid_property(connector, edid);
> > + }
>
> This is a pretty low level function. Too low level for this caching
> IMO. So I think better just do it a bit higher up like other drivers.

Fair enough. In the past I'd gotten feedback that I'd been jamming too
much stuff in my own driver instead of putting it in the core, but I'm
happy to leave the EDID caching in the driver if that's what people
prefer. It actually makes a bit of the code in the driver a bit less
awkward...

-Doug

Re: [PATCH 2/2] nvmem: qfprom: Add support for fuse blowing on sc7280

[Doug Anderson]

Hi,

On Wed, Mar 24, 2021 at 10:45 PM Rajendra Nayak  wrote:
>
> @@ -111,6 +113,15 @@ static const struct qfprom_soc_compatible_data 
> sc7180_qfprom = {
> .nkeepout = ARRAY_SIZE(sc7180_qfprom_keepout)
>  };
>
> +static const struct nvmem_keepout sc7280_qfprom_keepout[] = {
> +   {.start = 0x128, .end = 0x148},
> +   {.start = 0x238, .end = 0x248}
> +};
> +
> +static const struct qfprom_soc_compatible_data sc7280_qfprom = {
> +   .keepout = sc7280_qfprom_keepout,
> +   .nkeepout = ARRAY_SIZE(sc7280_qfprom_keepout)
> +};
>  /**

nit: blank line between structure and comment?


> @@ -187,9 +199,9 @@ static int qfprom_enable_fuse_blowing(const struct 
> qfprom_priv *priv,
>  * a rail shared do don't specify a max--regulator constraints
>  * will handle.
>  */
> -   ret = regulator_set_voltage(priv->vcc, 180, INT_MAX);
> +   ret = regulator_set_voltage(priv->vcc, qfprom_blow_uV, INT_MAX);
> if (ret) {
> -   dev_err(priv->dev, "Failed to set 1.8 voltage\n");
> +   dev_err(priv->dev, "Failed to set %duV\n", qfprom_blow_uV);

nit: the comment above this block (not in the unified diff)
specifically calls out 1.8V. It'd be nice if you updated the comment
since it's no longer fixed at 1.8V.


> @@ -379,6 +399,8 @@ static int qfprom_probe(struct platform_device *pdev)
>
> if (major_version == 7 && minor_version == 8)
> priv->soc_data = &qfprom_7_8_data;
> +   if (major_version == 7 && minor_version == 15)
> +   priv->soc_data = &qfprom_7_15_data;

nit: "else if" instead of "if"?


I guess I'm a little late since I think this already got applied, but
all the above are nits. Maybe you could send a follow-up patch to
address them?

-Doug

Re: [PATCH] drm/msm: Fix removal of valid error case when checking speed_bin

[Doug Anderson]

Hi,

On Mon, Mar 29, 2021 at 6:34 PM John Stultz  wrote:
>
> Commit 7bf168c8fe8c  ("drm/msm: Fix speed-bin support not to
> access outside valid memory"), reworked the nvmem reading of
> "speed_bin", but in doing so dropped handling of the -ENOENT
> case which was previously documented as "fine".
>
> That change resulted in the db845c board display to fail to
> start, with the following error:
>
> adreno 500.gpu: [drm:a6xx_gpu_init] *ERROR* failed to read speed-bin 
> (-2). Some OPPs may not be supported by hardware
>
> Thus, this patch simply re-adds the ENOENT handling so the lack
> of the speed_bin entry isn't fatal for display, and gets things
> working on db845c.
>
> Cc: Rob Clark 
> Cc: Sean Paul 
> Cc: Jordan Crouse 
> Cc: Eric Anholt 
> Cc: Douglas Anderson 
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Cc: Bjorn Andersson 
> Cc: YongQin Liu 
> Reported-by: YongQin Liu 
> Fixes: 7bf168c8fe8c  ("drm/msm: Fix speed-bin support not to access outside 
> valid memory")
> Signed-off-by: John Stultz 
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if refclk

[Doug Anderson]

Hi,

On Tue, Mar 16, 2021 at 5:44 PM Doug Anderson  wrote:
>
> Hi,
>
> On Tue, Mar 16, 2021 at 2:46 PM Laurent Pinchart
>  wrote:
> >
> > Hi Doug,
> >
> > On Mon, Mar 15, 2021 at 09:25:37AM -0700, Doug Anderson wrote:
> > > On Sat, Mar 13, 2021 at 1:17 PM Laurent Pinchart wrote:
> > > > On Thu, Mar 04, 2021 at 03:52:01PM -0800, Douglas Anderson wrote:
> > > > > In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
> > > > > DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> > > > > EDID from the panel. That commit kinda worked but it had some serious
> > > > > problems.
> > > > >
> > > > > The problems all stem from the fact that userspace wants to be able to
> > > > > read the EDID before it explicitly enables the panel. For eDP panels,
> > > > > though, we don't actually power the panel up until the pre-enable
> > > > > stage and the pre-enable call happens right before the enable call
> > > > > with no way to interject in-between. For eDP panels, you can't read
> > > > > the EDID until you power the panel. The result was that
> > > > > ti_sn_bridge_connector_get_modes() was always failing to read the EDID
> > > > > (falling back to what drm_panel_get_modes() returned) until _after_
> > > > > the EDID was needed.
> > > > >
> > > > > To make it concrete, on my system I saw this happen:
> > > > > 1. We'd attach the bridge.
> > > > > 2. Userspace would ask for the EDID (several times). We'd try but fail
> > > > >to read the EDID over and over again and fall back to the hardcoded
> > > > >modes.
> > > > > 3. Userspace would decide on a mode based only on the hardcoded modes.
> > > > > 4. Userspace would ask to turn the panel on.
> > > > > 5. Userspace would (eventually) check the modes again (in Chrome OS
> > > > >this happens on the handoff from the boot splash screen to the
> > > > >browser). Now we'd read them properly and, if they were different,
> > > > >userspace would request to change the mode.
> > > > >
> > > > > The fact that userspace would always end up using the hardcoded modes
> > > > > at first significantly decreases the benefit of the EDID
> > > > > reading. Also: if the modes were even a tiny bit different we'd end up
> > > > > doing a wasteful modeset and at boot.
> > > >
> > > > s/and at/at/ ?
> > >
> > > Sure, I can correct if/when I respin or it can be corrected when landed.
> > >
> > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
> > > > > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > index 491c9c4f32d1..af3fb4657af6 100644
> > > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > @@ -16,6 +16,7 @@
> > > > >  #include 
> > > > >  #include 
> > > > >  #include 
> > > > > +#include 
> > > > >
> > > > >  #include 
> > > > >
> > > > > @@ -130,6 +131,12 @@
> > > > >   * @ln_assign:Value to program to the LN_ASSIGN register.
> > > > >   * @ln_polrs: Value for the 4-bit LN_POLRS field of 
> > > > > SN_ENH_FRAME_REG.
> > > > >   *
> > > > > + * @pre_enabled_early: If true we did an early pre_enable at attach.
> > > > > + * @pre_enable_timeout_work: Delayed work to undo the pre_enable 
> > > > > from attach
> > > > > + *   if a normal pre_enable never came.
> > > >
> > > > Could we simplify this by using the runtime PM autosuspend feature ? The
> > > > configuration of the bridge would be moved from pre_enable to the PM
> > > > runtime resume handler, the clk_disable_unprepare() call moved from
> > > > post_disable to the runtime suspend handler, and the work queue replaced
> > > > by usage of pm_runtime_put_autosuspend().
> > >
> > > It's an interesting idea but I don't think I can make it work, at
> > > least not in a generic enough way. Specifically we can also use this
> > > bridge chip as a generic GPIO provider in Linux. When someone asks us
> > > to read a GPIO then we 

Re: [RFC PATCH 1/3] dt-bindings: display: simple: Add the panel on sc7180-trogdor-pompom

[Doug Anderson]

Hi,

On Thu, Mar 25, 2021 at 5:09 PM Rob Herring  wrote:
>
> On Tue, Mar 16, 2021 at 02:08:19PM -0700, Douglas Anderson wrote:
> > The sc7180-trogdor-pompom board might be attached to any number of a
> > pile of eDP panels. At the moment I'm told that the list might include:
> > - KD KD116N21-30NV-A010
> > - KD KD116N09-30NH-A016
> > - Starry 2081116HHD028001-51D
> > - Sharp LQ116M1JW10
> >
> > It should be noted that while the EDID programmed in the first 3
> > panels indicates that they should run with exactly the same timing (to
> > keep things simple), the 4th panel not only needs different timing but
> > has a different resolution.
> >
> > As is true in general with eDP panels, we can figure out which panel
> > we have and all the info needed to drive its pixel clock by reading
> > the EDID. However, we can do this only after we've powered the panel
> > on. Powering on the panels requires following the timing diagram in
> > each panel's datasheet which specifies delays between certain
> > actions. This means that, while we can be quite dynamic about handling
> > things we can't just totally skip out on describing the panel like we
> > could do if it was connected to an external-facing DP port.
>
> Is this a 'standard' eDP connector? AFAICT, there does seem to be
> such a thing.

To answer this one: there's not any "standard" physical plug as far as
I can tell. There's a connector on the board side for the LCD that has
a whole hodgepodge of signals on it. Maybe USB for a camera. Some
power signals. Maybe a PWM for a backlight. Maybe some DMIC signals.
eDP signals which might be anywhere from 1 to 4 lanes. HPD (which is
really a "panel ready" signal for eDP). The size / style of connector
and the exact set of signals (and their ordering) is board specific.
You then get a board-specific cable that splits things out. Some might
go to a camera/MIC sub board. Some go to the panel and hook onto a
panel-specific connector which has pin count and orderings defined by
that panel. :-P


> I've said in the past I'd be okay with a edp-connector
> node. If that needs just the "HPD absent delay" property, I think that
> would be okay. It's just a never ending stream of new properties with
> each new panel that I don't want to see.

Thinking about this we'd need at least one other property right now
which is an enable delay. Specifically at least one panel I've
supported recently lied about HPD for a short period after bootup.
Specifically see commit 667d73d72f31 ("drm: panel: simple: Delay HPD
checking on boe_nv133fhm_n61 for 15 ms"). ...and, of course, the
existing power supply / enable signals that "simple-panel" already
has.

Also: if we weren't going to add the other delay properties in the
device tree, we'd have to add the code right away that used the EDID
to set other delays. That wouldn't be the end of the world, but it
would be code to write.


One last thought to add: I've looked at ~10 panels specs recently.
Though they are all a little different from each other, I will say
that almost every one of them seems to have the exact same timing
diagram in it just with different numbers filled in. To me that backs
up the idea that you can/should do the power sequence with a fairly
standard (parameterized) driver. I can't link the datasheets I have
but searching for "edp panel datasheet" finds me this random
datasheet:

https://www.data-modul.com/sites/default/files/products/NV156QUM-N72_specification_12039472.pdf

See "8.0 POWER SEQUENCE" in that document. All the panels have a
nearly identical diagram with different numbers filled in. You can
kinda tell it was copied from some other panel since some numbers
(like T4) aren't even defined.

-Doug

Re: [RFC PATCH 1/3] dt-bindings: display: simple: Add the panel on sc7180-trogdor-pompom

[Doug Anderson]

Hi,

On Fri, Mar 26, 2021 at 5:38 AM Thierry Reding  wrote:
>
> The point remains that unless we describe exactly which panel we're
> dealing with, we ultimately have no way of properly quirking anything if
> we ever have to.

Just to clarify here: with my initial proposal we actually could still
quirk things if we had to. If the quirk needed to be applied before
power on we'd just have to apply the quirk to the whole board (which
we'd have to do anyway). After the panel was powered on then we could
read the EDID and apply a quirk based on what the EDID tells us,
right?


> Also, once we allow this kind of wildcard we can
> suddenly get into a situation where people might want to reuse this on
> something that's not at all a google-pompom board because the same
> particular power sequence happens to work on on some other board.

That's a legit concern. Of course, people could already do that with
existing panels right? One would also hope that if they reused this
they also used the "more specific to least specific" rule, so someone
could reuse (without any problems) with:

compatible = "some-other-company,some-other-board-panel", "google,pompom-panel"

That doesn't seem like it would be terrible.


> Similarly I can imagine a situation where we could now have the same
> panel supported by multiple different wildcard compatible strings. How
> is that supposed to be any cleaner than what we have now?

I'm tempted to call this (same panel supported by multiple different
compatible strings) a feature, actually. Specifically:

Even if the exact same hardware is shipped with more than one board,
it may have a different EDID programmed into it. From what I've seen
the timings used on a panel may need to be adjusted based on the SoC
used (and what clock rates it can provide / features of the underlying
display driver), EMI concerns, and power consumption concerns. Once a
different EDID is programmed in it then it sorta becomes a "different"
panel, right? I think sometimes (?) panel vendors assign a slightly
different model number per board, but I'm not sure.


-Doug

Re: [RFC PATCH 1/3] dt-bindings: display: simple: Add the panel on sc7180-trogdor-pompom

[Doug Anderson]

Hi,

On Fri, Mar 26, 2021 at 12:48 PM Rob Herring  wrote:
>
> On Fri, Mar 26, 2021 at 9:20 AM Rob Clark  wrote:
> >
> > On Fri, Mar 26, 2021 at 8:18 AM Rob Clark  wrote:
> > >
> > > On Fri, Mar 26, 2021 at 5:38 AM Thierry Reding  
> > > wrote:
> > > >
> > > > On Wed, Mar 17, 2021 at 06:53:04PM -0700, Rob Clark wrote:
> > > > > On Wed, Mar 17, 2021 at 4:27 PM Matthias Kaehlcke  
> > > > > wrote:
> > > > > >
> > > > > > On Tue, Mar 16, 2021 at 02:08:19PM -0700, Douglas Anderson wrote:
> > > > > > > The sc7180-trogdor-pompom board might be attached to any number 
> > > > > > > of a
> > > > > > > pile of eDP panels. At the moment I'm told that the list might 
> > > > > > > include:
> > > > > > > - KD KD116N21-30NV-A010
> > > > > > > - KD KD116N09-30NH-A016
> > > > > > > - Starry 2081116HHD028001-51D
> > > > > > > - Sharp LQ116M1JW10
> > > > > > >
> > > > > > > It should be noted that while the EDID programmed in the first 3
> > > > > > > panels indicates that they should run with exactly the same 
> > > > > > > timing (to
> > > > > > > keep things simple), the 4th panel not only needs different 
> > > > > > > timing but
> > > > > > > has a different resolution.
> > > > > > >
> > > > > > > As is true in general with eDP panels, we can figure out which 
> > > > > > > panel
> > > > > > > we have and all the info needed to drive its pixel clock by 
> > > > > > > reading
> > > > > > > the EDID. However, we can do this only after we've powered the 
> > > > > > > panel
> > > > > > > on. Powering on the panels requires following the timing diagram 
> > > > > > > in
> > > > > > > each panel's datasheet which specifies delays between certain
> > > > > > > actions. This means that, while we can be quite dynamic about 
> > > > > > > handling
> > > > > > > things we can't just totally skip out on describing the panel 
> > > > > > > like we
> > > > > > > could do if it was connected to an external-facing DP port.
> > > > > > >
> > > > > > > While the different panels have slightly different delays, it's
> > > > > > > possible to come up with a set of unified delays that will work 
> > > > > > > on all
> > > > > > > the panels. From reading the datasheets:
> > > > > > > * KD KD116N21-30NV-A010 and KD KD116N09-30NH-A016
> > > > > > >   - HPD absent delay: 200 ms
> > > > > > >   - Unprepare delay: 150 ms (datasheet is confusing, might be 500 
> > > > > > > ms)
> > > > > > > * Starry 2081116HHD028001-51D
> > > > > > >   - HPD absent delay: 100 ms
> > > > > > >   - Enable delay: (link training done till enable BL): 200 ms
> > > > > > >   - Unprepare delay: 500 ms
> > > > > > > * Sharp LQ116M1JW10
> > > > > > >   - HPD absent delay: 200 ms
> > > > > > >   - Unprepare delay: 500 ms
> > > > > > >   - Prepare to enable delay (power on till backlight): 100 ms
> > > > > > >
> > > > > > > Unified:
> > > > > > > - HPD absent delay: 200 ms
> > > > > > > - Unprepare delay: 500 ms
> > > > > > > - Enable delay: 200 ms
> > > > > > >
> > > > > > > NOTE: in theory the only thing that we _really_ need unity on is 
> > > > > > > the
> > > > > > > "HPD absent delay" since once the panel asserts HPD we can read 
> > > > > > > the
> > > > > > > EDID and could make per-panel decisions if we wanted.
> > > > > > >
> > > > > > > Let's create a definition of "a panel that can be attached to 
> > > > > > > pompom"
> > > > > > > as a panel that provides a valid EDID and can work with the 
> > > > > > > standard
> > > > > > > pompom power sequencing. If more panels are later attached to 
> > > > > > > pompom
> > > > > > > then it's fine as long as they work in a compatible way.
> > > > > > >
> > > > > > > One might ask why we can't just use a generic string here and 
> > > > > > > provide
> > > > > > > the timings directly in the device tree file. As I understand it,
> > > > > > > trying to describe generic power sequencing in the device tree is
> > > > > > > frowned upon and the one instance (SD/MMC) is regarded as a 
> > > > > > > mistake
> > > > > > > that shouldn't be repeated. Specifying a power sequence per board 
> > > > > > > (or
> > > > > > > per board class) feels like a reasonable compromise. We're not 
> > > > > > > trying
> > > > > > > to define fully generic power sequence bindings but we can also 
> > > > > > > take
> > > > > > > advantage of the semi-probable properties of the attached device.
> > > > > > >
> > > > > > > NOTE: I believe that past instances of supporting this type of 
> > > > > > > thing
> > > > > > > have used the "white lie" approach. One representative panel was
> > > > > > > listed in the device tree. The power sequencings of this
> > > > > > > representative panel were OK to use across all panels that might 
> > > > > > > be
> > > > > > > attached and other differences were handled by EDID. This patch
> > > > > > > attempts to set a new precedent and avoid the need for the white 
> > > > > > > lie.
> > > > > > >
> > > > > > > Signed-off-by: Douglas Anderson 
> > > > > > > ---
> > > > > >
> > > > > > Sounds reasonable to

Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card

[Doug Anderson]

Hi,

On Thu, Mar 25, 2021 at 11:57 PM  wrote:
>
> >>> +   max-frequency = <19200>;
> >> Why do you need to specify this?
> This helps to avoid lower speed modes running in high clock rate,
> and As Veerabhadrarao Badiganti mentioned

Just to be clear, both Stephen and I agree that you should remove
"max-frequency" here (see previous discussion). Bjorn is, of course,
the file decision maker. However, unless he says "yeah, totally keep
it in" I'd suggest dropping it from the next version.


> >>> +   required-opps =
> >>> <&rpmhpd_opp_low_svs>;
> >>> +   opp-peak-kBps = <120
> >>> 76000>;
> >>> +   opp-avg-kBps = <120
> >>> 5>;
> >> Why are the kBps numbers so vastly different than the ones on sc7180
> >> for the same OPP point. That implies:
> >>
> >> a) sc7180 is wrong.
> >>
> >> b) This patch is wrong.
> >>
> >> c) The numbers are essentially random and don't really matter.
> >>
> >> Can you identify which of a), b), or c) is correct, or propose an
> >> alternate explanation of the difference?
> >>
>
> We calculated bus votes values for both sc7180 and sc7280 with ICB tool,
> above mentioned values we got for sc7280.

I don't know what an ICB tool is. Please clarify.

Also: just because a tool spits out numbers that doesn't mean it's
correct. Presumably the tool could be wrong or incorrectly configured.
We need to understand why these numbers are different.

-Doug

Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card

[Doug Anderson]

Hi,

On Wed, Mar 24, 2021 at 8:37 PM Veerabhadrarao Badiganti
 wrote:
>
>
> On 3/24/2021 9:58 PM, Stephen Boyd wrote:
> > Quoting Stephen Boyd (2021-03-24 08:57:33)
> >> Quoting sbh...@codeaurora.org (2021-03-24 08:23:55)
> >>> On 2021-03-23 12:31, Stephen Boyd wrote:
>  Quoting Shaik Sajida Bhanu (2021-03-20 11:17:00)
> > +
> > +   bus-width = <8>;
> > +   non-removable;
> > +   supports-cqe;
> > +   no-sd;
> > +   no-sdio;
> > +
> > +   max-frequency = <19200>;
>  Is this necessary?
> >>> yes, to avoid lower speed modes running with high clock rates.
> >> Is it part of the DT binding? I don't see any mention of it.
> > Nevermind, found it in mmc-controller.yaml. But I think this is to work
> > around some problem with the clk driver picking lower speeds than
> > requested? That has been fixed on the clk driver side (see commit like
> > 148ddaa89d4a "clk: qcom: gcc-sc7180: Use floor ops for the correct sdcc1
> > clk") so ideally this property can be omitted.
> This is a good have dt node.
>
> This will align clock requests between mmc core layer and sdhci-msm
> platform driver. Say, for HS200/HS400 modes of eMMC, mmc-core layer
> tries to set clock at 200Mhz, whereas sdhci-msm expects 192Mhz for
> these modes. So we have to rely on clock driver floor/ceil values.
> By having this property, mmc-core layer itself request for 192Mhz.
>
> Same is for SD card SDR104 mode, core layer expects clock at 208Mhz
> whereas sdhci-msm can max operate only at 202Mhz. By having this
> property, core layer requests only for 202Mhz for SDR104 mode.
>
> BTW, this helps only for max possible speed modes.
> In case of lower-speed modes (for DDR52) we still need to rely on clock
> floor rounding.

Just let the clock driver figure it out and remove this from the
devicetree, please. As you said, the clock driver needs to understand
how to round rates anyway for the non-maximum requests. Putting the
information here just duplicates the data.

-Doug

Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card

[Doug Anderson]

Hi,

On Wed, Mar 24, 2021 at 8:59 PM Veerabhadrarao Badiganti
 wrote:
>
> >> +   clocks = <&gcc GCC_SDCC1_APPS_CLK>,
> >> +   <&gcc GCC_SDCC1_AHB_CLK>,
> >> +   <&rpmhcc RPMH_CXO_CLK>;
> >> +   clock-names = "core", "iface", "xo";
> > I'm curious: why is the "xo" clock needed here but not for sc7180?
> Actually its needed even for sc7180. We are making use of this clock in
> msm_init_cm_dll()
> The default PoR value is also same as calculated value for
> HS200/HS400/SDR104 modes.
> But just not to rely on default register values we need this entry.

Can you post a patch for sc7180?


> >> +   bus-width = <4>;
> >> +
> >> +   no-mmc;
> >> +   no-sdio;
> > Similar question to above: why exactly would mmc not work? Are you
> > saying that if someone hooked this up to a full sized SD card slot and
> > placed an MMC card into the slot that it wouldn't work? Similar
> > question about SDIO. If someone placed an external SDIO card into your
> > slot, would it not work?
> >
> As mentioned above, its just to optimize SDcard scan time a little.

OK. ...but while the eMMC one can make sense since the eMMC is
soldered down (but in the board dts file, not in the SoC dtsi file) I
think you should just remove these for SD card because:

1. Even if only a uSD slot is exposed it's still _possible_ for
someone to insert a card that uses MMC or SDIO signaling. If nothing
else I have a (probably non-compliant) adapter that plugs into a uSD
slot and provides a full sided SD slot. I could plug an MMC card or
SDIO card in.

2. Presumably the SD card scan time optimization is tiny.


-Doug

Re: [PATCH v2] arm64: dts: qcom: c630: Add no-hpd to DSI bridge node

[Doug Anderson]

Hi,

On Wed, Mar 24, 2021 at 4:14 PM Stephen Boyd  wrote:
>
> We should indicate that we're not using the HPD pin on this device, per
> the binding document. Otherwise if code in the future wants to enable
> HPD in the bridge when this property is absent we'll be enabling HPD
> when it isn't supposed to be used. Presumably this board isn't using hpd
> on the bridge.
>
> Cc: Laurent Pinchart 
> Cc: Douglas Anderson 
> Cc: Steev Klimaszewski 
> Fixes: 956e9c85f47b ("arm64: dts: qcom: c630: Define eDP bridge and panel")
> Signed-off-by: Stephen Boyd 
> ---
>  arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Douglas Anderson 

Re: [PATCH] arm64: dts: qcom: c630: Add no-hpd to DSI bridge node

[Doug Anderson]

Hi,

On Wed, Mar 24, 2021 at 12:15 PM Stephen Boyd  wrote:
>
> We should indicate that we're not using the HPD pin on this device, per
> the binding document. Otherwise if code in the future wants to enable
> HPD in the bridge when this property is absent we'll be wasting power
> powering hpd when we don't use it.

It's not really about wasting power. It's really more about:

a) If HPD is actually hooked up on the board, it's actually _slower_
to use it than to just assume the worst case time.

b) If HPD isn't hooked up but we try to use it then everything will just fail.

I don't know which of a) or b) is true, but I'd imagine that one or
the other is.


> Presumably this board isn't using hpd
> on the bridge.
>
> Cc: Laurent Pinchart 
> Cc: Douglas Anderson 

Cc: Steev Klimaszewski 


> Fixes: 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial trogdor 
> and lazor dt")

Wrong Fixes?


> Signed-off-by: Stephen Boyd 
> ---
>  arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts 
> b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> index 140db2d5ba31..c2a709a384e9 100644
> --- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
> @@ -376,6 +376,8 @@ sn65dsi86: bridge@2c {
> clocks = <&sn65dsi86_refclk>;
> clock-names = "refclk";
>
> +   no-hpd;
> +
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> base-commit: a38fd8748464831584a19438cbb3082b5a2dab15
> --
> https://chromeos.dev
>

Re: [PATCH] arm64: dts: qcom: trogdor: Add no-hpd to DSI bridge node

[Doug Anderson]

Hi,

On Tue, Mar 23, 2021 at 7:55 PM Stephen Boyd  wrote:
>
> We should indicate that we're not using the HPD pin on this device, per
> the binding document. Otherwise if code in the future wants to enable
> HPD in the bridge when this property is absent we'll be wasting power
> powering hpd when we don't use it on trogdor boards. We didn't notice
> this before because the kernel driver blindly disables hpd, but that
> won't be true for much longer.
>
> Cc: Laurent Pinchart 
> Cc: Douglas Anderson 
> Fixes: 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial trogdor 
> and lazor dt")
> Signed-off-by: Stephen Boyd 
> ---
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index 07c8b2c926c0..298af6d7fb4a 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -595,6 +595,8 @@ sn65dsi86_bridge: bridge@2d {
> clocks = <&rpmhcc RPMH_LN_BB_CLK3>;
> clock-names = "refclk";
>
> +   no-hpd;
> +

Sigh. I can't believe that I added this for cheza in commit
0d1ce0d14bd7 ("arm64: dts: sdm845: Add "no-hpd" to sn65dsi86 on
cheza") but forgot trogdor. Thanks.

Reviewed-by: Douglas Anderson 

NOTE: if you were feeling charitable you might consider sending a
patch for "sdm850-lenovo-yoga-c630.dts" as well.  I don't personally
know if HPD is hooked up on that system, but presumably even if it is
it's just as useless as it is on other systems where the bridge is
used for eDP. If nothing else setting it preserves existing behavior.
Someone (I forget who) requested that I word the bindings specifically
to say that "no-hpd" was OK to specify in cases like that, since the
bindings say:

>  Set if the HPD line on the bridge isn't hooked up to anything or is
>  otherwise unusable.

-Doug

Re: [PATCH v3 3/4] drm/bridge: ti-sn65dsi86: Read EDID blob over DDC

[Doug Anderson]

Hi,

On Mon, Mar 22, 2021 at 8:17 PM Stephen Boyd  wrote:
>
> Quoting Laurent Pinchart (2021-03-17 17:20:43)
> > Hi Stephen,
> >
> > Reviving a bit of an old thread, for a question.
> >
> > On Mon, Nov 02, 2020 at 10:11:43AM -0800, Stephen Boyd wrote:
> > > @@ -265,6 +267,23 @@ connector_to_ti_sn_bridge(struct drm_connector 
> > > *connector)
> > >  static int ti_sn_bridge_connector_get_modes(struct drm_connector 
> > > *connector)
> > >  {
> > >   struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> > > + struct edid *edid = pdata->edid;
> > > + int num, ret;
> > > +
> > > + if (!edid) {
> > > + pm_runtime_get_sync(pdata->dev);
> > > + edid = pdata->edid = drm_get_edid(connector, 
> > > &pdata->aux.ddc);
> > > + pm_runtime_put(pdata->dev);
> >
> > Is there any specific reason to use the indirect access method, compared
> > to the direct method that translates access to an I2C ancillary address
> > to an I2C-over-AUX transaction (see page 20 of SLLSEH2B) ? The direct
> > method seems it would be more efficient.
> >
>
> No I don't think it matters. I was just using the existing support code
> that Sean wrote instead of digging into the details. Maybe Sean ran into
> something earlier and abandoned that approach?

>From reading the docs, it sounds as if there _could_ be a reason to
use the indirect method. Specifically if the i2c host that the bridge
is on doesn't support clock stretching then the direct method wouldn't
work according to the docs. Is that something that we'd have to
reasonably worry about?

-Doug

Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card

[Doug Anderson]

Hi,

On Sat, Mar 20, 2021 at 11:18 AM Shaik Sajida Bhanu
 wrote:
>
> Add nodes for eMMC and SD card on sc7280.
>
> Signed-off-by: Shaik Sajida Bhanu 
>
> ---
> This change is depends on the below patch series:
> https://lore.kernel.org/patchwork/project/lkml/list/?series=488871
> https://lore.kernel.org/patchwork/project/lkml/list/?series=489530
> https://lore.kernel.org/patchwork/project/lkml/list/?series=488429
>
> Changes since V1:
> - Moved SDHC nodes as suggested by Bjorn Andersson.
> - Dropped "pinconf-" prefix as suggested by Bjorn Andersson.
> - Removed extra newlines as suggested by Konrad Dybcio.
> - Changed sd-cd pin to bias-pull-up in sdc2_off as suggested by
>   Veerabhadrarao Badiganti.
> - Added bandwidth votes for eMMC and SD card.
> ---
>  arch/arm64/boot/dts/qcom/sc7280-idp.dts |  25 
>  arch/arm64/boot/dts/qcom/sc7280.dtsi| 213 
> 
>  2 files changed, 238 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts 
> b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> index 54d2cb3..4105263 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> @@ -8,6 +8,7 @@
>  /dts-v1/;
>
>  #include "sc7280.dtsi"
> +#include 
>
>  / {
> model = "Qualcomm Technologies, Inc. sc7280 IDP platform";
> @@ -242,6 +243,30 @@
> status = "okay";
>  };
>
> +&sdhc_1 {
> +   status = "okay";

When I apply your patch I find that your sort order is wrong. "s"
comes before "u" and after "q" in the alphabet so "sdhc_1" and
"sdhc_2" should sort _after "qupv3_id_0" and before "uart5"


> +   pinctrl-names = "default", "sleep";
> +   pinctrl-0 = <&sdc1_on>;
> +   pinctrl-1 = <&sdc1_off>;
> +
> +   vmmc-supply = <&vreg_l7b_2p9>;
> +   vqmmc-supply = <&vreg_l19b_1p8>;
> +};
> +
> +&sdhc_2 {
> +   status = "okay";
> +
> +   pinctrl-names = "default","sleep";
> +   pinctrl-0 = <&sdc2_on>;
> +   pinctrl-1 = <&sdc2_off>;
> +
> +   vmmc-supply = <&vreg_l9c_2p9>;
> +   vqmmc-supply = <&vreg_l6c_2p9>;
> +
> +   cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;

Where is the pinctrl for the card detect?  Oh, I see it's in
"sdc2_on". Probably would be good to break it out since this is
board-specific. See below.


> +};
> +
>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>
>  &qup_uart5_default {
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 8f6b569..69eb064 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -20,6 +20,11 @@
>
> chosen { };
>
> +   aliases {
> +   mmc1 = &sdhc_1;
> +   mmc2 = &sdhc_2;
> +   };
> +
> clocks {
> xo_board: xo-board {
> compatible = "fixed-clock";
> @@ -305,6 +310,64 @@
> #power-domain-cells = <1>;
> };
>
> +   sdhc_1: sdhci@7c4000 {
> +   compatible = "qcom,sdhci-msm-v5";

Please make the compatible:
  compatible = "qcom,sc7280-sdhci", "qcom,sdhci-msm-v5";

...and add to the bindings. It should be a trivial bindings patch so
not too much trouble.

NOTE: even though the "qcom,sc7280-sdhci" should be in the bindings
and here you _shouldn't_ be adding any code for it. Just let the
fallback compatible string ("qcom,sdhci-msm-v5") do its magic. Adding
the sc7280 specific version is more of a "just in case we need it
later" type thing.


> +   reg = <0 0x7c4000 0 0x1000>,
> +   <0 0x7c5000 0 0x1000>;
> +   reg-names = "hc", "cqhci";
> +
> +   iommus = <&apps_smmu 0xC0 0x0>;
> +   interrupts = ,
> +   ;
> +   interrupt-names = "hc_irq", "pwr_irq";
> +
> +   clocks = <&gcc GCC_SDCC1_APPS_CLK>,
> +   <&gcc GCC_SDCC1_AHB_CLK>,
> +   <&rpmhcc RPMH_CXO_CLK>;
> +   clock-names = "core", "iface", "xo";

I'm curious: why is the "xo" clock needed here but not for sc7180?


> +   interconnects = <&aggre1_noc MASTER_SDCC_1 0 &mc_virt 
> SLAVE_EBI1 0>,
> +   <&gem_noc MASTER_APPSS_PROC 0 &cnoc2 
> SLAVE_SDCC_1 0>;
> +   interconnect-names = "sdhc-ddr","cpu-sdhc";
> +   power-domains = <&rpmhpd SC7280_CX>;
> +   operating-points-v2 = <&sdhc1_opp_table>;
> +
> +   bus-width = <8>;
> +   non-removable;

This was actually a problem on sc7180 too, but you probably don't want
"non-removable" in the SoC file. Board files really should be adding
this. Though the SoC might be designed with the idea that this would
be used for a 

Re: [PATCH] kgdb: fix gcc-11 warning on indentation

[Doug Anderson]

Hi,

On Mon, Mar 22, 2021 at 11:19 AM Arnd Bergmann  wrote:
>
> On Mon, Mar 22, 2021 at 6:07 PM Doug Anderson  wrote:
> > On Mon, Mar 22, 2021 at 9:43 AM Arnd Bergmann  wrote:
> > >
> > > -#define v1printk(a...) do { \
> > > -   if (verbose) \
> > > -   printk(KERN_INFO a); \
> > > -   } while (0)
> > > -#define v2printk(a...) do { \
> > > -   if (verbose > 1) \
> > > -   printk(KERN_INFO a); \
> > > -   touch_nmi_watchdog();   \
> > > -   } while (0)
> > > -#define eprintk(a...) do { \
> > > -   printk(KERN_ERR a); \
> > > -   WARN_ON(1); \
> > > -   } while (0)
> > > +#define v1printk(a...) do {\
> >
> > nit: In addition to the indentation change you're also lining up the
> > backslashes. Is that just personal preference, or is there some
> > official recommendation in the kernel? I don't really have a strong
> > opinion either way (IMO each style has its advantages).
>
> I don't think there is an official recommendation, I just think the
> style is more common, and it helped my figure out what the
> indentation should look like in this case.

OK, makes sense. I just wasn't sure if there was some standard that I
wasn't aware of. Given that you have to touch all these lines anyway
then making them all pretty like this seems fine to me.


> > > +   if (verbose)\
> > > +   printk(KERN_INFO a);\
> > > +} while (0)
> > > +#define v2printk(a...) do {\
> > > +   if (verbose > 1)\
> > > +   printk(KERN_INFO a);\
> > > +   touch_nmi_watchdog();   \
> >
> > This touch_nmi_watchdog() is pretty wonky. I guess maybe the
> > assumption is that the "verbose level 2" prints are so chatty that the
> > printing might prevent us from touching the NMI watchdog in the way
> > that we normally do and thus we need an extra one here?
> >
> > ...but, in that case, I think the old code was _wrong_ and that the
> > intention was that the touch_nmi_watchdog() should only be if "verose
> > > 1" as the indentation implied. There doesn't feel like a reason to
> > touch the watchdog if we're not doing anything slow.
>
> No idea. It was like this in Jason's original version from 2008.

Yeah, I noticed the same. I'd be curious what Daneil (or Jason if he's
reading) says. I suppose i could always wait until your patch lands
and then send a new patch that puts it inside the "if" statement and
we can debate it then.

-Doug

Re: [PATCH] kgdb: fix gcc-11 warning on indentation

[Doug Anderson]

Hi,

On Mon, Mar 22, 2021 at 9:43 AM Arnd Bergmann  wrote:
>
> -#define v1printk(a...) do { \
> -   if (verbose) \
> -   printk(KERN_INFO a); \
> -   } while (0)
> -#define v2printk(a...) do { \
> -   if (verbose > 1) \
> -   printk(KERN_INFO a); \
> -   touch_nmi_watchdog();   \
> -   } while (0)
> -#define eprintk(a...) do { \
> -   printk(KERN_ERR a); \
> -   WARN_ON(1); \
> -   } while (0)
> +#define v1printk(a...) do {\

nit: In addition to the indentation change you're also lining up the
backslashes. Is that just personal preference, or is there some
official recommendation in the kernel? I don't really have a strong
opinion either way (IMO each style has its advantages).


> +   if (verbose)\
> +   printk(KERN_INFO a);\
> +} while (0)
> +#define v2printk(a...) do {\
> +   if (verbose > 1)\
> +   printk(KERN_INFO a);\
> +   touch_nmi_watchdog();   \

This touch_nmi_watchdog() is pretty wonky. I guess maybe the
assumption is that the "verbose level 2" prints are so chatty that the
printing might prevent us from touching the NMI watchdog in the way
that we normally do and thus we need an extra one here?

...but, in that case, I think the old code was _wrong_ and that the
intention was that the touch_nmi_watchdog() should only be if "verose
> 1" as the indentation implied. There doesn't feel like a reason to
touch the watchdog if we're not doing anything slow.

-Doug

Re: [v1] drm/msm/disp/dpu1: fix display underruns during modeset.

[Doug Anderson]

Hi,

On Fri, Mar 19, 2021 at 5:54 AM Kalyan Thota  wrote:
>
> During crtc disable, display perf structures are reset to 0
> which includes state varibles which are immutable. On crtc
> enable, we use the same structures and they don't refelect
> the actual values
>
> 1) Fix is to avoid updating the state structures during disable.
> 2) Reset the perf structures during atomic check when there is no
> modeset enable.
>
> Signed-off-by: Kalyan Thota 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 1 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)

I think Stephen was the one who originally noticed this and reported it, so:

Reported-by: Stephen Boyd 

Seems to work for me. I got into the state where it was doing a
modeset at reboot (could see the underflow color for a period of time
when this happened). I added your patch and it looks better.

Tested-by: Douglas Anderson 

Re: [PATCH v3 2/3] arm64: dts: qcom: sc7180: Add pompom rev3

[Doug Anderson]

Hi,

On Mon, Mar 15, 2021 at 6:15 PM Matthias Kaehlcke  wrote:
>
> The only kernel visible change with respect to rev2 is that pompom
> rev3 changed the charger thermistor from a 47k to a 100k NTC to use
> a thermistor which is supported by the PM6150 ADC driver.
>
> Disable the charger thermal zone for pompom rev1 and rev2 to avoid
> the use of bogus temperature values from the unsupported thermistor.
>
> Signed-off-by: Matthias Kaehlcke 
> ---
>
> Changes in v3:
> - don't add LOCK key
>
> Changes in v2:
> - moved keyboard definition to sc7180-trogdor-pompom.dtsi instead
>   of duplicating it, use cros-ec keyboard for rev1
> - squashed with 'arm64: dts: qcom: sc7180: pompom: Disable charger
>   thermal zone for rev1 and rev2'
>
>  arch/arm64/boot/dts/qcom/Makefile |  2 +
>  .../dts/qcom/sc7180-trogdor-pompom-r1.dts | 12 ++
>  .../dts/qcom/sc7180-trogdor-pompom-r2-lte.dts |  4 +-
>  .../dts/qcom/sc7180-trogdor-pompom-r2.dts | 38 +--
>  .../dts/qcom/sc7180-trogdor-pompom-r3-lte.dts | 14 +++
>  .../dts/qcom/sc7180-trogdor-pompom-r3.dts | 15 
>  .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi  | 29 ++
>  7 files changed, 83 insertions(+), 31 deletions(-)
>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts
>
> diff --git a/arch/arm64/boot/dts/qcom/Makefile 
> b/arch/arm64/boot/dts/qcom/Makefile
> index a81966d59cf7..11aa83ca798f 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -49,6 +49,8 @@ dtb-$(CONFIG_ARCH_QCOM)   += 
> sc7180-trogdor-pompom-r1.dtb
>  dtb-$(CONFIG_ARCH_QCOM)+= sc7180-trogdor-pompom-r1-lte.dtb
>  dtb-$(CONFIG_ARCH_QCOM)+= sc7180-trogdor-pompom-r2.dtb
>  dtb-$(CONFIG_ARCH_QCOM)+= sc7180-trogdor-pompom-r2-lte.dtb
> +dtb-$(CONFIG_ARCH_QCOM)+= sc7180-trogdor-pompom-r3.dtb
> +dtb-$(CONFIG_ARCH_QCOM)+= sc7180-trogdor-pompom-r3-lte.dtb
>  dtb-$(CONFIG_ARCH_QCOM)+= sc7180-trogdor-r1.dtb
>  dtb-$(CONFIG_ARCH_QCOM)+= sc7180-trogdor-r1-lte.dtb
>  dtb-$(CONFIG_ARCH_QCOM)+= sdm630-sony-xperia-ganges-kirin.dtb
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dts 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dts
> index e720e7bd0d70..7f87877408c5 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dts
> @@ -9,11 +9,23 @@
>
>  #include "sc7180-trogdor-pompom.dtsi"
>
> +/delete-node/ keyboard_controller;

So I just tried to compile your patch and I found that it doesn't
compile. :( The above needs to be:

/delete-node/ &keyboard_controller;

-Doug

Re: [PATCH] drm/msm: Ratelimit invalid-fence message

[Doug Anderson]

Hi,

On Wed, Mar 17, 2021 at 9:40 AM Rob Clark  wrote:
>
> From: Rob Clark 
>
> We have seen a couple cases where low memory situations cause something
> bad to happen, followed by a flood of these messages obscuring the root
> cause.  Lets ratelimit the dmesg spam so that next time it happens we
> don't loose the kernel traces leading up to this.

s/loose/lose


> Signed-off-by: Rob Clark 
> ---
>  drivers/gpu/drm/msm/msm_fence.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if refclk

[Doug Anderson]

Hi,

On Tue, Mar 16, 2021 at 2:46 PM Laurent Pinchart
 wrote:
>
> Hi Doug,
>
> On Mon, Mar 15, 2021 at 09:25:37AM -0700, Doug Anderson wrote:
> > On Sat, Mar 13, 2021 at 1:17 PM Laurent Pinchart wrote:
> > > On Thu, Mar 04, 2021 at 03:52:01PM -0800, Douglas Anderson wrote:
> > > > In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
> > > > DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> > > > EDID from the panel. That commit kinda worked but it had some serious
> > > > problems.
> > > >
> > > > The problems all stem from the fact that userspace wants to be able to
> > > > read the EDID before it explicitly enables the panel. For eDP panels,
> > > > though, we don't actually power the panel up until the pre-enable
> > > > stage and the pre-enable call happens right before the enable call
> > > > with no way to interject in-between. For eDP panels, you can't read
> > > > the EDID until you power the panel. The result was that
> > > > ti_sn_bridge_connector_get_modes() was always failing to read the EDID
> > > > (falling back to what drm_panel_get_modes() returned) until _after_
> > > > the EDID was needed.
> > > >
> > > > To make it concrete, on my system I saw this happen:
> > > > 1. We'd attach the bridge.
> > > > 2. Userspace would ask for the EDID (several times). We'd try but fail
> > > >to read the EDID over and over again and fall back to the hardcoded
> > > >modes.
> > > > 3. Userspace would decide on a mode based only on the hardcoded modes.
> > > > 4. Userspace would ask to turn the panel on.
> > > > 5. Userspace would (eventually) check the modes again (in Chrome OS
> > > >this happens on the handoff from the boot splash screen to the
> > > >browser). Now we'd read them properly and, if they were different,
> > > >userspace would request to change the mode.
> > > >
> > > > The fact that userspace would always end up using the hardcoded modes
> > > > at first significantly decreases the benefit of the EDID
> > > > reading. Also: if the modes were even a tiny bit different we'd end up
> > > > doing a wasteful modeset and at boot.
> > >
> > > s/and at/at/ ?
> >
> > Sure, I can correct if/when I respin or it can be corrected when landed.
> >
> > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
> > > > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > index 491c9c4f32d1..af3fb4657af6 100644
> > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > @@ -16,6 +16,7 @@
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > +#include 
> > > >
> > > >  #include 
> > > >
> > > > @@ -130,6 +131,12 @@
> > > >   * @ln_assign:Value to program to the LN_ASSIGN register.
> > > >   * @ln_polrs: Value for the 4-bit LN_POLRS field of 
> > > > SN_ENH_FRAME_REG.
> > > >   *
> > > > + * @pre_enabled_early: If true we did an early pre_enable at attach.
> > > > + * @pre_enable_timeout_work: Delayed work to undo the pre_enable from 
> > > > attach
> > > > + *   if a normal pre_enable never came.
> > >
> > > Could we simplify this by using the runtime PM autosuspend feature ? The
> > > configuration of the bridge would be moved from pre_enable to the PM
> > > runtime resume handler, the clk_disable_unprepare() call moved from
> > > post_disable to the runtime suspend handler, and the work queue replaced
> > > by usage of pm_runtime_put_autosuspend().
> >
> > It's an interesting idea but I don't think I can make it work, at
> > least not in a generic enough way. Specifically we can also use this
> > bridge chip as a generic GPIO provider in Linux. When someone asks us
> > to read a GPIO then we have to power the bridge on
> > (pm_runtime_get_sync()) and when someone asks us to configure a GPIO
> > as an output then we actually leave the bridge powered until they stop
> > requesting it as an output. At the moment the only user of this
> > functionality (that I know of) is for the HPD pin on trogdor boards
> > (long story about why we don't use the dedicated HPD) but the API
> > supports using these GPIOs fo

Re: [PATCH v3 3/3] arm64: dts: qcom: sc7180: Add CoachZ rev3

[Doug Anderson]

Hi,

On Mon, Mar 15, 2021 at 6:15 PM Matthias Kaehlcke  wrote:
>
> CoachZ rev3 uses a 100k NTC thermistor for the charger temperatures,
> instead of the 47k NTC that is stuffed in earlier revisions. Add .dts
> files for rev3.
>
> The 47k NTC currently isn't supported by the PM6150 ADC driver.
> Disable the charger thermal zone for rev1 and rev2 to avoid the use
> of bogus temperature values.
>
> This also gets rid of the explicit DT files for rev2 and handles
> rev2 in the rev1 .dts instead. There was some back and forth
> downstream involving the 'dmic_clk_en' pin, after that was sorted
> out the DT for rev1 and rev2 is the same.
>
> Signed-off-by: Matthias Kaehlcke 
> ---
>
> Changes in v3:
> - get rid of separate .dts for rev2
>
> Changes in v2:
> - added CoachZ rev3
> - updated subject and commit message
>
>  arch/arm64/boot/dts/qcom/Makefile   |  4 ++--
>  .../boot/dts/qcom/sc7180-trogdor-coachz-r1-lte.dts  |  4 ++--
>  .../boot/dts/qcom/sc7180-trogdor-coachz-r1.dts  | 13 +++--
>  ...-r2-lte.dts => sc7180-trogdor-coachz-r3-lte.dts} |  6 +++---
>  ...r-coachz-r2.dts => sc7180-trogdor-coachz-r3.dts} |  4 ++--
>  5 files changed, 20 insertions(+), 11 deletions(-)

Looks keen to me now, thanks!

Reviewed-by: Douglas Anderson 

Re: [PATCH v3 2/3] arm64: dts: qcom: sc7180: Add pompom rev3

[Doug Anderson]

Hi,

On Mon, Mar 15, 2021 at 6:15 PM Matthias Kaehlcke  wrote:
>
> The only kernel visible change with respect to rev2 is that pompom
> rev3 changed the charger thermistor from a 47k to a 100k NTC to use
> a thermistor which is supported by the PM6150 ADC driver.
>
> Disable the charger thermal zone for pompom rev1 and rev2 to avoid
> the use of bogus temperature values from the unsupported thermistor.
>
> Signed-off-by: Matthias Kaehlcke 
> ---
>
> Changes in v3:
> - don't add LOCK key
>
> Changes in v2:
> - moved keyboard definition to sc7180-trogdor-pompom.dtsi instead
>   of duplicating it, use cros-ec keyboard for rev1
> - squashed with 'arm64: dts: qcom: sc7180: pompom: Disable charger
>   thermal zone for rev1 and rev2'
>
>  arch/arm64/boot/dts/qcom/Makefile |  2 +
>  .../dts/qcom/sc7180-trogdor-pompom-r1.dts | 12 ++
>  .../dts/qcom/sc7180-trogdor-pompom-r2-lte.dts |  4 +-
>  .../dts/qcom/sc7180-trogdor-pompom-r2.dts | 38 +--
>  .../dts/qcom/sc7180-trogdor-pompom-r3-lte.dts | 14 +++
>  .../dts/qcom/sc7180-trogdor-pompom-r3.dts | 15 
>  .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi  | 29 ++
>  7 files changed, 83 insertions(+), 31 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH v2 3/3] arm64: dts: qcom: sc7180: Add CoachZ rev3

[Doug Anderson]

Hi,

On Fri, Mar 12, 2021 at 10:32 AM Matthias Kaehlcke  wrote:
>
> CoachZ rev3 uses a 100k NTC thermistor for the charger temperatures,
> instead of the 47k NTC that is stuffed in earlier revisions. Add .dts
> files for rev3.
>
> The 47k NTC currently isn't supported by the PM6150 ADC driver.
> Disable the charger thermal zone for rev1 and rev2 to avoid the use
> of bogus temperature values.
>
> Signed-off-by: Matthias Kaehlcke 
> ---
>
> Changes in v2:
> - added CoachZ rev3
> - updated subject and commit message
>
>  arch/arm64/boot/dts/qcom/Makefile  |  2 ++
>  .../boot/dts/qcom/sc7180-trogdor-coachz-r1.dts |  9 +
>  .../dts/qcom/sc7180-trogdor-coachz-r2-lte.dts  |  4 ++--
>  .../boot/dts/qcom/sc7180-trogdor-coachz-r2.dts | 13 +++--
>  .../dts/qcom/sc7180-trogdor-coachz-r3-lte.dts  | 18 ++
>  .../boot/dts/qcom/sc7180-trogdor-coachz-r3.dts | 15 +++
>  6 files changed, 57 insertions(+), 4 deletions(-)

So what you have here is good and we could land it. Feel free to add
my Reviewed-by tag if you want.

...but I want to propose an alternative. It turns out that these days
coachz-r1 and coachz-r2 are actually the same. The only reason both
exist is because  ("CHROMIUM: arm64: dts:
qcom: sc7180: add dmic_clk_en back") wasn't the proper inverse of
 ("CHROMIUM: arm64: dts: qcom: sc7180:
remove dmic_clk_en").

It sorta squashes two changes into one, but if you combined your
change with one that folded "-r1" into "-r2" it would actually make a
smaller / easier to understand change, essentially, it would be:
- just a rename of the "-r2" file to be "-r3"
- add "-rev2" into the list of compatibles in "-r1" file.
- add the "disable" into the "-r1" file.

Anyway, I'll leave it up to you.


-Doug

Re: [PATCH v2 2/3] arm64: dts: qcom: sc7180: Add pompom rev3

[Doug Anderson]

Hi,

On Fri, Mar 12, 2021 at 10:32 AM Matthias Kaehlcke  wrote:
>
> +   linux,keymap = <
> +   MATRIX_KEY(0x00, 0x02, KEY_BACK)
> +   MATRIX_KEY(0x03, 0x02, KEY_REFRESH)
> +   MATRIX_KEY(0x02, 0x02, KEY_ZOOM)
> +   MATRIX_KEY(0x01, 0x02, KEY_SCALE)
> +   MATRIX_KEY(0x03, 0x04, KEY_SYSRQ)
> +   MATRIX_KEY(0x02, 0x04, KEY_BRIGHTNESSDOWN)
> +   MATRIX_KEY(0x01, 0x04, KEY_BRIGHTNESSUP)
> +   MATRIX_KEY(0x02, 0x09, KEY_MUTE)
> +   MATRIX_KEY(0x01, 0x09, KEY_VOLUMEDOWN)
> +   MATRIX_KEY(0x00, 0x04, KEY_VOLUMEUP)
> +
> +   MATRIX_KEY(0x03, 0x09, KEY_SLEEP)   /* LOCK key */

I don't think you want the LOCK key. See 


-Doug

Re: [PATCH v2 1/3] arm64: dts: qcom: sc7180: lazor: Simplify disabling of charger thermal zone

[Doug Anderson]

Hi,

On Fri, Mar 12, 2021 at 10:32 AM Matthias Kaehlcke  wrote:
>
> Commit f73558cc83d1 ("arm64: dts: qcom: sc7180: Disable charger
> thermal zone for lazor") disables the charger thermal zone for
> specific lazor revisions due to an unsupported thermistor type.
> The initial idea was to disable the thermal zone for older
> revisions and leave it enabled for newer ones that use a
> supported thermistor. Finally the thermistor won't be changed
> on newer revisions, hence the thermal zone should be disabled
> for all lazor (and limozeen) revisions. Instead of disabling
> it per revision do it once in the shared .dtsi for lazor.
>
> Signed-off-by: Matthias Kaehlcke 
> ---
>
> Changes in v2:
> - none
>
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 9 -
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 9 -
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts | 9 -
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi   | 9 +
>  4 files changed, 9 insertions(+), 27 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
> index 5c997cd90069..30e3e769d2b4 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
> @@ -14,15 +14,6 @@ / {
> compatible = "google,lazor-rev0", "qcom,sc7180";
>  };
>
> -/*
> - * Lazor is stuffed with a 47k NTC as charger thermistor which currently is
> - * not supported by the PM6150 ADC driver. Disable the charger thermal zone
> - * to avoid using bogus temperature values.
> - */
> -&charger_thermal {
> -   status = "disabled";
> -};
> -
>  &pp3300_hub {
> /* pp3300_l7c is used to power the USB hub */
> /delete-property/regulator-always-on;
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
> index d9fbcc7bc5bd..c2ef06367baf 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
> @@ -14,15 +14,6 @@ / {
> compatible = "google,lazor-rev1", "google,lazor-rev2", "qcom,sc7180";
>  };
>
> -/*
> - * Lazor is stuffed with a 47k NTC as charger thermistor which currently is
> - * not supported by the PM6150 ADC driver. Disable the charger thermal zone
> - * to avoid using bogus temperature values.
> - */
> -&charger_thermal {
> -   status = "disabled";
> -};
> -
>  &pp3300_hub {
> /* pp3300_l7c is used to power the USB hub */
> /delete-property/regulator-always-on;
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts
> index ea8c2ee09741..b474df47cd70 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts
> @@ -14,12 +14,3 @@ / {
> model = "Google Lazor (rev3+)";
> compatible = "google,lazor", "qcom,sc7180";
>  };
> -
> -/*
> - * Lazor is stuffed with a 47k NTC as charger thermistor which currently is
> - * not supported by the PM6150 ADC driver. Disable the charger thermal zone
> - * to avoid using bogus temperature values.
> - */
> -&charger_thermal {
> -   status = "disabled";
> -};
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
> index 6b10b96173e8..6d540321b4a5 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
> @@ -41,6 +41,15 @@ ap_ts: touchscreen@10 {
> };
>  };
>
> +/*
> + * Lazor is stuffed with a 47k NTC as charger thermistor which currently is
> + * not supported by the PM6150 ADC driver. Disable the charger thermal zone
> + * to avoid using bogus temperature values.
> + */
> +&charger_thermal {
> +   status = "disabled";
> +};

So this always confuses me too, but the sort order is wrong.  :(

While it _looks_like the node above you is "ap_ts", I believe the
convention for sorting is not to use labels added in this file. Yeah,
we gotta document this somewhere. Thus, this node should be sorted as
"charger_thermal" (using a label not defined in this file) and the
node above should be sorted as "touchscreen@10" and thus we should go
above it.

In general I think the reason we tend to use the node name and not any
labels is that it keeps us from having to redo the sort ordering if we
give something a new label. It also helps keep the i2c busses
together, the SPI busses together, etc.

That's just a sort order nit, though, so:

Reviewed-by: Douglas Anderson 


-Doug

Re: [PATCH 1/2] arm64: dts: qcom: Add "dmic_clk_en" for sc7180-trogdor-coachz

[Doug Anderson]

Hi,

On Mon, Mar 15, 2021 at 1:23 PM Douglas Anderson  wrote:
>
> This was present downstream. Add upstream too.
>
> Cc: Srinivasa Rao Mandadapu 
> Cc: Ajit Pandey 
> Cc: Judy Hsiao 
> Cc: Cheng-Yi Chiang 
> Cc: Stephen Boyd 
> Cc: Matthias Kaehlcke 
> Signed-off-by: Douglas Anderson 
> ---
> This applies atop the patch ("arm64: dts: qcom: Add sound node for
> sc7180-trogdor-coachz") [1].
>
> NOTE: downstream this property was present in each of the board
> revisions. There's actually no longer any reason for this and I'll
> shortly post a downstream patch to fix this.
>
> [1] https://lore.kernel.org/r/20210313054654.11693-3-sriva...@codeaurora.org/
>
>  .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi | 16 
>  1 file changed, 16 insertions(+)

Argh, I just realized that upstream actually has this in -r1 but not
in -r2. So confusing! I'll send a quick v2. Sorry for the spam.

-Doug

Re: [PATCH v7 2/2] arm64: dts: qcom: Add sound node for sc7180-trogdor-coachz

[Doug Anderson]

Hi,

On Sat, Mar 13, 2021 at 10:11 PM Srinivasa Rao Mandadapu
 wrote:
>
> This is a trgodor variant, required to have sound node variable
> for coachz specific platform.
>
> Signed-off-by: Srinivasa Rao Mandadapu 
> Reviewed-by: Stephen Boyd 
> ---
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi | 10 ++
>  1 file changed, 10 insertions(+)

Reviewed-by: Douglas Anderson 

With these two patches plus commit 9922f50f7178 ("ASoC: qcom:
lpass-cpu: Fix lpass dai ids parse") in the sound tree I get audio on
sc7180-trogdor-coachz! Thus:

Tested-by: Douglas Anderson 

Re: [PATCH v7 1/2] arm64: dts: qcom: sc7180-trogdor: Add lpass dai link for I2S driver

[Doug Anderson]

Hi,

On Sat, Mar 13, 2021 at 10:11 PM Srinivasa Rao Mandadapu
 wrote:
>
> From: Ajit Pandey 
>
> Add dai link for supporting lpass I2S driver, which is used
> for audio capture and playback.
> Add lpass-cpu node with  pin controls and i2s primary
> and secondary dai-links

You missed Stephen's comments on your commit message [1]

[1] 
https://lore.kernel.org/r/161566899554.1478170.1265435102634351...@swboyd.mtv.corp.google.com/

> Signed-off-by: Ajit Pandey 
> Signed-off-by: V Sujith Kumar Reddy 
> Signed-off-by: Srinivasa Rao Mandadapu 
> ---
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 58 
>  1 file changed, 58 insertions(+)

The commit message nits aren't terribly important and Bjorn can
presumably just fix them when applying if he cares. IMO you don't need
to re-spin.

Reviewed-by: Douglas Anderson 

With these two patches plus commit 9922f50f7178 ("ASoC: qcom:
lpass-cpu: Fix lpass dai ids parse") in the sound tree I get audio on
sc7180-trogdor-lazor! Thus:

Tested-by: Douglas Anderson 

Re: [PATCH 01/13] arm64: dts: qcom: sc7180: Update dts for DP phy inside QMP phy

[Doug Anderson]

Hi,

On Sat, Mar 13, 2021 at 4:28 PM Dmitry Baryshkov
 wrote:
>
> >   usb_1_qmpphy: phy-wrapper@88e9000 {
> > - compatible = "qcom,sc7180-qmp-usb3-phy";
> > + compatible = "qcom,sc7180-qmp-usb3-dp-phy";
> >   reg = <0 0x088e9000 0 0x18c>,
> > -   <0 0x088e8000 0 0x38>;
> > - reg-names = "reg-base", "dp_com";
> > +   <0 0x088e8000 0 0x38>,
>
> Technically this should be 0x3c. Offset 0x38 is USB3_DP_COM_REVISION_ID3
> (not used by the current driver though).
>
> > +   <0 0x088ea000 0 0x40>;
>
> I think 0x40 is not enough here.
> This is a serdes region and qmp_v3_dp_serdes_tbl contains registers
> 0x148 and 0x154.

OK!

https://lore.kernel.org/r/20210315103836.1.I9a97120319d43b42353aeac4d348624d60687df7@changeid

-Doug

Re: [PATCH v2 1/2] arm64: dts: qcom: sdm845: Move reserved-memory to devices

[Doug Anderson]

Hi,

On Fri, Mar 12, 2021 at 3:42 PM Bjorn Andersson
 wrote:
>
> The reserved-memory regions used for carrying firmware to be run on the
> various cores and co-processors in a Qualcomm platform differs in size,
> placement and presence based on each device's feature set and security
> configuration.
>
> Rather than providing some basic set that works on the MTP and then
> piecemeal patch this up on the various devices, push the configuration
> of these regions out to the individual device dts files.
>
> Signed-off-by: Bjorn Andersson 
> ---
>
> Changes since v1:
> - Added lost memory-region to the db845c wlan node, as spotted by Doug.
>
>  arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi| 90 +--
>  arch/arm64/boot/dts/qcom/sdm845-db845c.dts| 87 ++
>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts   | 87 ++
>  .../boot/dts/qcom/sdm845-oneplus-common.dtsi  | 78 +++-
>  .../boot/dts/qcom/sdm845-xiaomi-beryllium.dts | 45 ++
>  arch/arm64/boot/dts/qcom/sdm845.dtsi  | 83 -
>  .../boot/dts/qcom/sdm850-lenovo-yoga-c630.dts | 86 ++
>  7 files changed, 431 insertions(+), 125 deletions(-)

I will leave it up to you to evaluate Konrad's feedback that this will
cause a bunch of duplication since I don't have enough experience w/
Android phones to have an informed opinion. In case it matters, this
addresses the feedback I had on v1 and thus:

Reviewed-by: Douglas Anderson 

I'll also repeat the feedback that I had on v1 that I focused much
more on cheza than on other boards and didn't check every last thing
on every board to make sure no changes happened.

Re: [PATCH 2/3] drm/bridge: ti-sn65dsi86: Move code in prep for EDID read fix

[Doug Anderson]

Hi,

On Sat, Mar 13, 2021 at 1:13 PM Laurent Pinchart
 wrote:
>
> Hi Douglas,
>
> Thank you for the patch.
>
> On Thu, Mar 04, 2021 at 03:52:00PM -0800, Douglas Anderson wrote:
> > This patch is _only_ code motion to prepare for the patch
> > ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if
> > refclk") and make it easier to understand.
>
> s/make/makes/

I was never an expert at grammar, but I think either "make" or "makes"
are fine. Simple version with parenthesis:

Mine:

This patch is  to (prepare for the patch ) and (make it
easier to understand).

Yours:

This patch is  (to prepare for the patch ) and (makes it
easier to understand).

I suppose also valid would be:

This patch is  (to prepare for the patch ) and (to make it
easier to understand).


In any case if/when I spin this patch I'm fine changing it to your
version just because (as I understand) it's equally valid and maybe
looks slightly better?

> > Signed-off-by: Douglas Anderson 
>
> Reviewed-by: Laurent Pinchart 

Thanks for the reviews!

-Doug

Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if refclk

[Doug Anderson]

Hi,

On Sat, Mar 13, 2021 at 1:17 PM Laurent Pinchart
 wrote:
>
> Hi Doug,
>
> Thank you for the patch.
>
> On Thu, Mar 04, 2021 at 03:52:01PM -0800, Douglas Anderson wrote:
> > In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
> > DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> > EDID from the panel. That commit kinda worked but it had some serious
> > problems.
> >
> > The problems all stem from the fact that userspace wants to be able to
> > read the EDID before it explicitly enables the panel. For eDP panels,
> > though, we don't actually power the panel up until the pre-enable
> > stage and the pre-enable call happens right before the enable call
> > with no way to interject in-between. For eDP panels, you can't read
> > the EDID until you power the panel. The result was that
> > ti_sn_bridge_connector_get_modes() was always failing to read the EDID
> > (falling back to what drm_panel_get_modes() returned) until _after_
> > the EDID was needed.
> >
> > To make it concrete, on my system I saw this happen:
> > 1. We'd attach the bridge.
> > 2. Userspace would ask for the EDID (several times). We'd try but fail
> >to read the EDID over and over again and fall back to the hardcoded
> >modes.
> > 3. Userspace would decide on a mode based only on the hardcoded modes.
> > 4. Userspace would ask to turn the panel on.
> > 5. Userspace would (eventually) check the modes again (in Chrome OS
> >this happens on the handoff from the boot splash screen to the
> >browser). Now we'd read them properly and, if they were different,
> >userspace would request to change the mode.
> >
> > The fact that userspace would always end up using the hardcoded modes
> > at first significantly decreases the benefit of the EDID
> > reading. Also: if the modes were even a tiny bit different we'd end up
> > doing a wasteful modeset and at boot.
>
> s/and at/at/ ?

Sure, I can correct if/when I respin or it can be corrected when landed.


> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
> > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 491c9c4f32d1..af3fb4657af6 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -16,6 +16,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include 
> >
> > @@ -130,6 +131,12 @@
> >   * @ln_assign:Value to program to the LN_ASSIGN register.
> >   * @ln_polrs: Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
> >   *
> > + * @pre_enabled_early: If true we did an early pre_enable at attach.
> > + * @pre_enable_timeout_work: Delayed work to undo the pre_enable from 
> > attach
> > + *   if a normal pre_enable never came.
>
> Could we simplify this by using the runtime PM autosuspend feature ? The
> configuration of the bridge would be moved from pre_enable to the PM
> runtime resume handler, the clk_disable_unprepare() call moved from
> post_disable to the runtime suspend handler, and the work queue replaced
> by usage of pm_runtime_put_autosuspend().

It's an interesting idea but I don't think I can make it work, at
least not in a generic enough way. Specifically we can also use this
bridge chip as a generic GPIO provider in Linux. When someone asks us
to read a GPIO then we have to power the bridge on
(pm_runtime_get_sync()) and when someone asks us to configure a GPIO
as an output then we actually leave the bridge powered until they stop
requesting it as an output. At the moment the only user of this
functionality (that I know of) is for the HPD pin on trogdor boards
(long story about why we don't use the dedicated HPD) but the API
supports using these GPIOs for anything and I've tested that it works.
It wouldn't be great to have to keep the panel on in order to access
the GPIOs.

The other problem is that I think the time scales are different. At
boot time I think we'd want to leave the panel on for tens of seconds
to give userspace a chance to start up and configure the panel. After
userspace starts up I think we'd want autosuspend to be much faster.
This could probably be solved by tweaking the runtime delay in code
but I didn't fully dig because of the above problem.


-Doug

Re: [PATCH v5 2/2] arm64: dts: qcom: Add sound node for sc7180-trogdor-coachz

[Doug Anderson]

Hi,

On Fri, Mar 12, 2021 at 8:07 AM Srinivasa Rao Mandadapu
 wrote:
>
> This is a trgodor variant, required to have sound node variable
> for coachz specific platform.
>
> Signed-off-by: Srinivasa Rao Mandadapu 
> ---
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> index 4ad520f00485..7eaad739b6f9 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> @@ -89,6 +89,16 @@ &sn65dsi86_out {
> data-lanes = <0 1 2 3>;
>  };
>
> +&sound {
> +   compatible = "google,sc7180-coachz";
> +   model = "sc7180-adau7002-max98357a";
> +   audio-routing = "PDM_DAT", "DMIC";
> +};
> +
> +&multimedia0_codec {
> +   sound-dai = <&adau7002>;
> +};
> +

Much nicer! The last nit is that "multimedia0_codec" is now sorted
incorrectly. It starts with "m" so it shouldn't come after "s". Yes, I
know that this rule means that these two things are no longer next to
each other, but that's the rule we have right now.

If it's important for you for them to sort together, I suppose you
could change the label to "sound_multimedia0_codec" though that's a
bit of a mouthful. I'll leave it up to you whether to rename (and keep
the current ordering) or keep the name (and move the node).



-Doug

Re: [PATCH v5 1/2] arm64: dts: qcom: sc7180-trogdor: Add lpass dai link for I2S driver

[Doug Anderson]

Hi,

On Fri, Mar 12, 2021 at 8:06 AM Srinivasa Rao Mandadapu
 wrote:
>
> +   dai-link@0 {
> +   link-name = "MultiMedia0";
> +   reg = ;
> +   cpu {
> +   sound-dai = <&lpass_cpu MI2S_PRIMARY>;
> +   };
> +
> +   multimedia0_codec: codec {
> +   sound-dai = <&alc5682 MI2S_PRIMARY>;

Please change "MI2S_PRIMARY" to "0". "MI2S_PRIMARY" is a numbering
system for "lpass_cpu" not for "alc5682". I'm not aware of symbolic
names for the alc5682's DAIs.

>From digging through the code and testing, it appears that for alc5682:
0 - aif1
1 - aif2

Thus you could be a little more documenting by doing:

sound-dai = <&alc5682 0 /*aif1*/>;

-Doug

Re: [PATCH] arm64: dts: sc7280: Add qspi, qupv3_0 and qupv3_1 nodes

[Doug Anderson]

Hi,

On Wed, Mar 10, 2021 at 7:41 PM Roja Rani Yarubandi
 wrote:
>
> +&qspi_cs0 {
> +   pinconf {
> +   pins = "gpio15";
> +   bias-disable;
> +   };

The "pinconf" / "pinmux" subnode shouldn't be used for new SoCs. See:

http://lore.kernel.org/r/CAD=FV=UY_AFRrAY0tef5jP698LEng6oN652LcX3B4nG=awh...@mail.gmail.com

...same feedback for this whole patch.

> +   qup_spi0_default: qup-spi0-default {
> +   pinmux {
> +   pins = "gpio0", "gpio1",
> +  "gpio2", "gpio3";
> +   function = "qup00";
> +   };
> +   };

Please split these SPI nodes as per the thread above, like:

tlmm: pinctrl@... {
  qup_spi0_data_clk: qup-spi0-data-clk {
pins = "gpio0", "gpio1", "gpio2";
function = "qup0";
  };

  qup_spi0_cs: qup-spi0-cs {
pins = "gpio3";
function = "qup0";
  };

  qup_spi0_cs_gpio: qup-spi0-cs-gpio {
pins = "gpio3";
function = "gpio";
  };
};


> +   qup_uart0_default: qup-uart0-default {
> +   pinmux {
> +   pins = "gpio0", "gpio1",
> +  "gpio2", "gpio3";
> +   function = "qup00";
> +   };
> +   };

I suspect things would actually be cleaner if we broke the uart lines
up since the boards tend to have to adjust pulls differently for each
line. With the new "no pinconf / pinmux" world it's pretty clean. It's
obviously up to Bjorn, but if it were me I'd request this in the SoC
file:

qup_uart0_cts: qup-uart0-cts {
  pins = "...";
  function = "qup00";
};

qup_uart0_rts: qup-uart0-rts {
  pins = "...";
  function = "qup00";
};

qup_uart0_rx: qup-uart0-rx {
  pins = "...";
  function = "qup00";
};

qup_uart0_tx: qup-uart0-tx {
  pins = "...";
  function = "qup00";
};

...and now when the board file wants to adjust the pulls they can just
reference each one:

/*
 * Comments about why the UART0 pulls make sense.
 * Blah blah blah.
 */

&qup_uart0_cts {
  bias-pull-down;
};

&qup_uart0_rts {
  drive-strength = <2>;
  bias-disable;
};

&qup_uart0_rx {
  bias-pull-up;
};

&qup_uart0_tx {
  drive-strength = <2>;
  bias-disable;
};


> +   qspi: spi@88dc000 {

I believe the qspi node is sorted incorrectly. When I apply this to
the top of the Qualcomm tree it shows up after the "apps_smmu:
iommu@1500" node. However:

0x088dc000 < 0x1500

...which means it should be _before_.

-Doug

Re: [PATCH v4 1/2] arm64: dts: qcom: sc7180-trogdor: Add lpass dai link for I2S driver

[Doug Anderson]

Hi,

On Thu, Mar 11, 2021 at 8:49 AM Srinivasa Rao Mandadapu
 wrote:
>
> From: Ajit Pandey 
>
> Add dai link for supporting lpass I2S driver, which is used
> for audio capture and playback.
> Add lpass-cpu node with  pin controls and i2s primary
> and secondary dai-links
>
> Signed-off-by: Ajit Pandey 
> Signed-off-by: V Sujith Kumar Reddy 
> Signed-off-by: Srinivasa Rao Mandadapu 
> ---
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 58 
>  1 file changed, 58 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index 436582279dad..501e3d4c9097 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -9,6 +9,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

It seems marginally better to include "sc7180-lpass.h" to get this? I
don't really know the difference between the two but since unless
we're planning to delete the sc7180 version it seems like you might as
well include that one?


>  /* PMICs depend on spmi_bus label and so must come after SoC */
>  #include "pm6150.dtsi"
> @@ -283,6 +284,42 @@ keyboard_backlight: keyboard-backlight {
> max-brightness = <1023>;
> };
> };
> +
> +   sound: sound {
> +   compatible = "google,sc7180-trogdor";
> +   model = "sc7180-rt5682-max98357a-1mic";
> +
> +   audio-routing =
> +   "Headphone Jack", "HPOL",
> +   "Headphone Jack", "HPOR";
> +
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> +
> +   dai-link@0 {
> +   link-name = "MultiMedia0";
> +   reg = ;
> +   cpu {
> +   sound-dai = <&lpass_cpu MI2S_PRIMARY>;
> +   };
> +
> +   codec {
> +   sound-dai = <&alc5682 MI2S_PRIMARY>;

I'm an audio noob but isn't "MI2S_PRIMARY" something to be used with
"lpass_cpu", not with "alc5682" ?

I have no idea what the IDs correspond to on "alc5682". Are you sure
we even need an extra ID there? The "alc5682" bindings upstream don't
talk anything about dai-cells, but maybe they're just wrong...

-Doug

Re: [PATCH v4 2/2] arm64: dts: qcom: Add sound node for sc7180-trogdor-coachz

[Doug Anderson]

Hi,

On Thu, Mar 11, 2021 at 8:48 AM Srinivasa Rao Mandadapu
 wrote:
>
> This is a trgodor variant, required to have sound node variable
> for coachz specific platform.
>
> Signed-off-by: Srinivasa Rao Mandadapu 
> ---
>  .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi   | 18 ++
>  1 file changed, 18 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> index 4ad520f00485..7623a30a64c7 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> @@ -238,3 +238,21 @@ &tlmm {
>   "DP_HOT_PLUG_DET",
>   "EC_IN_RW_ODL";
>  };
> +
> +&sound {
> +   compatible = "google,sc7180-coachz";

You're placing this in the wrong place. Pay attention to the section
headings. Your patch is putting this in the section "PINCTRL -
board-specific pinctrl". That's not right.


> +   model = "sc7180-adau7002-max98357a";
> +   audio-routing = "PDM_DAT", "DMIC";
> +
> +   dai-link@0 {
> +   link-name = "MultiMedia0";
> +   reg = <0>;
> +   cpu {
> +   sound-dai = <&lpass_cpu 0>;

Shouldn't the 0 above be "MI2S_PRIMARY" ?  ...and the "reg" as well?


> +   };
> +
> +   codec {
> +   sound-dai = <&adau7002>;
> +   };
> +   };

Some overall notes, though:

1. You don't actually need to duplicate everything that you have
above. Whether you realize it or not the way devicetree works is that
it _merges_ the node in the "coachz" devicetree with the one from the
trogdor one (it doesn't replace it). So in trogdor you have:

dai-link@0 {
  link-name = "MultiMedia0";
  reg = ;
  cpu {
sound-dai = <&lpass_cpu MI2S_PRIMARY>;
  };

  codec {
sound-dai = <&alc5682 MI2S_PRIMARY>;
  };
};

...and in coachz you have:

dai-link@0 {
  link-name = "MultiMedia0";
  reg = ;
  cpu {
sound-dai = <&lpass_cpu MI2S_PRIMARY>;
  };

  codec {
sound-dai = <&adau7002>;
  };
};

Almost all of that is duplication. It's best not to duplicate. Thus,
one step better than what you have would be to just have this in
coachz to override what you need:

dai-link@0 {
  codec {
sound-dai = <&adau7002>;
  };
};


2. In general it's discouraged (and error prone) to try to replicate
hierarchies from your parent. So the best would be to change trogdor's
device tree to something like this:

dai-link@0 {
  link-name = "MultiMedia0";
  reg = ;
  cpu {
sound-dai = <&lpass_cpu MI2S_PRIMARY>;
  };

  multimedia0_codec: codec {
sound-dai = <&alc5682 MI2S_PRIMARY>;
  };
};

...and then in coachz you override like:

&multimedia0_codec {
   sound-dai = <&alc5682 MI2S_PRIMARY>;
};

Re: [PATCH][next] nvmem: core: Fix unintentional sign extension issue

[Doug Anderson]

Hi,

On Thu, Mar 11, 2021 at 1:53 AM Colin King  wrote:
>
> From: Colin Ian King 
>
> The shifting of the u8 integer buf[3] by 24 bits to the left will
> be promoted to a 32 bit signed int and then sign-extended to a
> u64. In the event that the top bit of buf[3] is set then all
> then all the upper 32 bits of the u64 end up as also being set
> because of the sign-extension. Fix this by casting buf[i] to
> a u64 before the shift.
>
> Addresses-Coverity: ("Unintended sign extension")
> Fixes: 097eb1136ebb ("nvmem: core: Add functions to make number reading easy")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/nvmem/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks! I had only tested the "u64" version to read smaller data and
store it in a u64. From my understanding of C rules, without your
patch it would have been even worse than just a sign extension though,
right? Shifting "buf[i]" by more than 32 bits would just not have
worked right.

In any case:

Reviewed-by: Douglas Anderson 

Re: [PATCH v2 0/5] drm/panel-simple: Patches for N116BCA-EA1

[Doug Anderson]

Hi,

On Wed, Mar 10, 2021 at 4:57 PM Linus Walleij  wrote:
>
> On Thu, Mar 11, 2021 at 12:47 AM Doug Anderson  wrote:
>
> > I guess what I'd say in summary is:
> >
> > * If you object to the retries in simple panel, I still hope the rest
> > of the series can land.
> >
> > * If somehow this panel gets out into real users hands and we find
> > that the retries are necessary and people still don't want the retries
> > in simple panel, I can fork a special panel driver just for it then.
>
> I'm fine with the retries, if it is needed outside of the "simple" (hm)
> panel driver then we can certainly factor it out as a helper or
> library.
>
> I looked at the patches at lore.
> Reviewed-by: Linus Walleij 
> I see also Stephen has reviewed some patches.
>
> Tell me if you need me to also apply them to drm-misc.
> (I guess yes?)

Yes please. I was giving Sam time to do it but I haven't heard from
him for a while. Right before you responded I poked Thierry to see if
he was available but if you're willing/able to do it then I'm sure it
would save him the trouble.

If you'd like me to re-post the patches (CCing you) I can. Please let me know.

If you happen to feel in an applying mood one other patch to
simple-panel I think is OK to land is at:

https://lore.kernel.org/r/20210222081716.1.I1a45aece5d2ac6a2e73bbec50da2086e43e0862b@changeid

-Doug

Re: [PATCH v2 0/5] drm/panel-simple: Patches for N116BCA-EA1

[Doug Anderson]

Hi,

On Wed, Mar 10, 2021 at 3:25 PM Linus Walleij  wrote:
>
> On Fri, Jan 15, 2021 at 11:44 PM Douglas Anderson  
> wrote:
>
> > - ("drm/panel-simple: Don't wait longer for HPD...") new for v2.
> > - ("drm/panel-simple: Retry if we timeout waiting for HPD") new for v2.
>
> I couldn't find these patches in my inbox

Doh! Sorry about that. I think get_maintainer tagged you only on the
patches that had the explicit "fixes" in them on something you were
involved in. I tend to rely on get_maintainer heavily unless I think
someone is particularly interested in the whole series. I will make
sure to CC you on the whole series if I post it again. In the meantime
the whole series is on lore:

https://lore.kernel.org/lkml/20210115224420.1635017-1-diand...@chromium.org/

> but my concern would
> be that at some point panel-simple will turn from simple into
> panel-rube-goldberg-machine.

Yes, it's definitely something to watch out for. I guess you're
commenting on the general number of changes I've made to simple-panel
in the last year or two? I guess my comment on those:

* Many of the changes I made were around HPD handling and supporting a
GPIO (and also supporting the "hpd absent delay"). The fact that we
use a GPIO for HPD is actually an attribute of our board design,
though, so if I had forked panel drivers for each of the panels that
needed it then it would have required copying the code lots of places
(or implementing some code sharing). I was specifically asked to do
the HPD handling code at the panel layer.

* The other big change I made recently was around allowing for
relative rather than absolute timings (instead of a fixed delay at a
given stage adding a constraint that enough time had passed since a
previous event). When I proposed that the feedback I got back was that
it was a good improvement for all panels and something that had been
on a TODO list for a while.

...so while it feels like I keep piling crap onto simple-panel, I
_think_ they've been good general improvements that many people will
be able to benefit from and I don't think they've uglified things
lots.

> Given that the talk with the manufacturer may result
> in even more quirks... maybe this should just be a separate
> panel driver?

I don't _think_ this will end up with more quirks. At least I sure
hope not. So far it seems like pure luck that I even noticed it
because only one other device in the whole batch produced had similar
problems. That leads me to believe that there could be other panels
with a similar need.

> (I expect pushback because I see how handy it is, but
> I am the guy writing new panel drivers all the time rather than
> using simple.)

I guess what I'd say in summary is:

* If you object to the retries in simple panel, I still hope the rest
of the series can land.

* If somehow this panel gets out into real users hands and we find
that the retries are necessary and people still don't want the retries
in simple panel, I can fork a special panel driver just for it then.

-Doug

Re: [PATCH] watchdog: qcom: Move suspend/resume to suspend_late/resume_early

[Doug Anderson]

Hi,

On Wed, Mar 10, 2021 at 12:20 PM Sai Prakash Ranjan
 wrote:
>
> During suspend/resume usecases and tests, it is common to see issues
> such as lockups either in suspend path or resume path because of the
> bugs in the corresponding device driver pm handling code. In such cases,
> it is important that watchdog is active to make sure that we either
> receive a watchdog pretimeout notification or a bite causing reset
> instead of a hang causing us to hard reset the machine.
>
> There are good reasons as to why we need this because:
>
> * We can have a watchdog pretimeout governor set to panic in which
>   case we can have a backtrace which would help identify the issue
>   with the particular driver and cause a normal reboot.
>
> * Even in case where there is no pretimeout support, a watchdog
>   bite is still useful because some firmware has debug support to dump
>   CPU core context on watchdog bite for post-mortem analysis.
>
> * One more usecase which comes to mind is of warm reboot. In case we
>   hard reset the target, a cold reboot could be induced resulting in
>   lose of ddr contents thereby losing all the debug info.
>
> Currently, the watchdog pm callback just invokes the usual suspend
> and resume callback which do not have any special ordering in the
> sense that a watchdog can be suspended before the buggy device driver
> suspend callback and watchdog resume can happen after the buggy device
> driver resume callback. This would mean that the watchdog will not be
> active when the buggy driver cause the lockups thereby hanging the
> system. So to make sure this doesn't happen, move the watchdog pm to
> use late/early system pm callbacks which will ensure that the watchdog
> is suspended late and resumed early so that it can catch such issues.
>
> Signed-off-by: Sai Prakash Ranjan 
> ---
>  drivers/watchdog/qcom-wdt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH 1/3] nvmem: core: Add functions to make number reading easy

[Doug Anderson]

Hi,

On Wed, Mar 10, 2021 at 2:37 AM Srinivas Kandagatla
 wrote:
>
>
>
> On 06/03/2021 00:26, Douglas Anderson wrote:
> > Sometimes the clients of nvmem just want to get a number out of
> > nvmem. They don't want to think about exactly how many bytes the nvmem
> > cell took up. They just want the number. Let's make it easy.
> >
> > In general this concept is useful because nvmem space is precious and
> > usually the fewest bits are allocated that will hold a given value on
> > a given system. However, even though small numbers might be fine on
> > one system that doesn't mean that logically the number couldn't be
> > bigger. Imagine nvmem containing a max frequency for a component. On
> > one system perhaps that fits in 16 bits. On another system it might
> > fit in 32 bits. The code reading this number doesn't care--it just
> > wants the number.
> >
> > We'll provide two functions: nvmem_cell_read_variable_le_u32() and
> > nvmem_cell_read_variable_le_u64().
> >
> > Comparing these to the existing functions like nvmem_cell_read_u32():
> > * These new functions have no problems if the value was stored in
> >nvmem in fewer bytes. It's OK to use these function as long as the
> >value stored will fit in 32-bits (or 64-bits).
> > * These functions avoid problems that the earlier APIs had with bit
> >offsets. For instance, you can't use nvmem_cell_read_u32() to read a
> >value has nbits=32 and bit_offset=4 because the nvmem cell must be
> >at least 5 bytes big to hold this value. The new API accounts for
> >this and works fine.
> > * These functions make it very explicit that they assume that the
> >number was stored in little endian format. The old functions made
> >this assumption whenever bit_offset was non-zero (see
> >nvmem_shift_read_buffer_in_place()) but didn't whenever the
> >bit_offset was zero.
> >
> > NOTE: it's assumed that we don't need an 8-bit or 16-bit version of
> > this function. The 32-bit version of the function can be used to read
> > 8-bit or 16-bit data.
> >
> > At the moment, I'm only adding the "unsigned" versions of these
> > functions, but if it ends up being useful someone could add a "signed"
> > version that did 2's complement sign extension.
> >
> > At the moment, I'm only adding the "little endian" versions of these
> > functions. Adding the "big endian" version would require adding "big
> > endian" support to nvmem_shift_read_buffer_in_place().
> >
> > Signed-off-by: Douglas Anderson 
> > ---
> > This is a logical follow-up to:
> >https://lore.kernel.org/r/20210227002603.3260599-1-diand...@chromium.org/
> > ...but since it doesn't really share any of the same patches I'm not
> > marking it as a v2.
> >
> >   drivers/nvmem/core.c   | 95 ++
> >   include/linux/nvmem-consumer.h |  4 ++
> >   2 files changed, 99 insertions(+)
> >
>
> This patch as it is LGTM.
>
> If you plan to take this via other trees, here is
>
> Reviewed-by: Srinivas Kandagatla 

Thanks! I think none of this is terribly urgent, though. Unless
someone has a different opinion, my thought would be:

* This patch lands in your tree for 5.13.

* I'll snooze the email for 2 months and poke patch #2 and #3 once
5.13-rc1 is out.

Does that sound OK to you?

-Doug

Re: [PATCH 2/3] nvmem: core: Allow nvmem_cell_read_u16/32/64 to read smaller cells

[Doug Anderson]

Hi,

On Fri, Mar 5, 2021 at 8:07 AM Srinivas Kandagatla
 wrote:
>
> > If you think it's confusing to change the behavior of the existing
> > functions, would you be opposed to me adding a new function like
> > nvmem_cell_read_le_u32_or_smaller() (or provide me a better name) that
> > would be flexible like this?
>
> This should be perfectly okay!
> may be something like:
>
> int nvmem_read_variable_cell(struct device *dev, const char *cell_id,
> void *buf, size_t sz_min, size_t sz_max);
>
> It should return number of bytes it read and fail if cell size is less
> then sz_min!

The above API isn't really what I want, though.  Specifically I want
the API to acknowledge that it's a number that is being read.  The
client just wants a number and any conversion / zero-padding /
whatever needs to be abstracted out.  The client also doesn't really
care how big the cell actually was as long as the data fits, so I
wouldn't want to return it.

OK, I've come up with a new proposal, so maybe we can continue the
conversation there.  The API for my new function actually matches
exactly with the old cpr_read_efuse() which makes me feel like it's
the right way to go...

https://lore.kernel.org/r/20210305162546.1.I323dad4343256b48af2be160b84b1e87985cc9be@changeid

-Doug

Re: [PATCH 4/4] arm64: dts: qcom: sc7180: Disable charger thermal zone for coachz rev1 and rev2

[Doug Anderson]

Hi,

On Thu, Mar 4, 2021 at 10:04 AM Matthias Kaehlcke  wrote:
>
> CoachZ rev1 and rev2 are stuffed with a 47k NTC as thermistor for the
> charger temperature which currently isn't supported by the PM6150 ADC
> driver. Disable the charger thermal zone to avoid the use of bogus
> temperature values.
>
> Signed-off-by: Matthias Kaehlcke 
> ---
>
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts | 9 +
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r2.dts | 9 +
>  2 files changed, 18 insertions(+)

I guess this patch is written with the assumption that eventually
we'll create a "-r3" or "-r4" that has a proper thermistor stuffed but
maybe we're not sure which one?  Right now you're disabling it for
both -r1 and -r2+ which is all revisions, so this could go in the
coachz.dtsi file...

-Doug

Re: [PATCH 2/4] arm64: dts: qcom: sc7180: Add pompom rev3

[Doug Anderson]

Hi,

On Thu, Mar 4, 2021 at 10:04 AM Matthias Kaehlcke  wrote:
>
> The only kernel visible change with respect to rev2 is that pompom
> rev3 changed the charger thermistor from a 47k to a 100k NTC to use
> a thermistor which is supported by the PM6150 ADC driver.
>
> Signed-off-by: Matthias Kaehlcke 
> ---
>
>  .../dts/qcom/sc7180-trogdor-pompom-r2-lte.dts |  4 +-
>  .../dts/qcom/sc7180-trogdor-pompom-r2.dts |  4 +-
>  .../dts/qcom/sc7180-trogdor-pompom-r3-lte.dts | 14 ++
>  .../dts/qcom/sc7180-trogdor-pompom-r3.dts | 46 +++
>  4 files changed, 64 insertions(+), 4 deletions(-)
>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dts 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dts
> index 791d496ad046..00e187c08eb9 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dts
> @@ -9,6 +9,6 @@
>  #include "sc7180-trogdor-lte-sku.dtsi"
>
>  / {
> -   model = "Google Pompom (rev2+) with LTE";
> -   compatible = "google,pompom-sku0", "qcom,sc7180";
> +   model = "Google Pompom (rev2) with LTE";
> +   compatible = "google,pompom-rev2-sku0", "qcom,sc7180";
>  };
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts
> index 984d7337da78..2b2bd906321d 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts
> @@ -10,8 +10,8 @@
>  #include "sc7180-trogdor-pompom.dtsi"
>
>  / {
> -   model = "Google Pompom (rev2+)";
> -   compatible = "google,pompom", "qcom,sc7180";
> +   model = "Google Pompom (rev2)";
> +   compatible = "google,pompom-rev2", "qcom,sc7180";
>  };
>
>  &keyboard_controller {
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts
> new file mode 100644
> index ..067cb75a011e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Pompom board device tree source
> + *
> + * Copyright 2020 Google LLC.
> + */
> +
> +#include "sc7180-trogdor-pompom-r3.dts"
> +#include "sc7180-trogdor-lte-sku.dtsi"
> +
> +/ {
> +   model = "Google Pompom (rev3+) with LTE";
> +   compatible = "google,pompom-sku0", "qcom,sc7180";
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts
> new file mode 100644
> index ..12d2d1e8e9e1
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Pompom board device tree source
> + *
> + * Copyright 2021 Google LLC.
> + */
> +
> +/dts-v1/;
> +
> +#include "sc7180-trogdor-pompom.dtsi"
> +
> +/ {
> +   model = "Google Pompom (rev3+)";
> +   compatible = "google,pompom", "qcom,sc7180";
> +};
> +
> +&keyboard_controller {
> +   function-row-physmap = <
> +   MATRIX_KEY(0x00, 0x02, 0)   /* T1 */
> +   MATRIX_KEY(0x03, 0x02, 0)   /* T2 */
> +   MATRIX_KEY(0x02, 0x02, 0)   /* T3 */
> +   MATRIX_KEY(0x01, 0x02, 0)   /* T4 */
> +   MATRIX_KEY(0x03, 0x04, 0)   /* T5 */
> +   MATRIX_KEY(0x02, 0x04, 0)   /* T6 */
> +   MATRIX_KEY(0x01, 0x04, 0)   /* T7 */
> +   MATRIX_KEY(0x02, 0x09, 0)   /* T8 */
> +   MATRIX_KEY(0x01, 0x09, 0)   /* T9 */
> +   MATRIX_KEY(0x00, 0x04, 0)   /* T10 */
> +   >;
> +   linux,keymap = <
> +   MATRIX_KEY(0x00, 0x02, KEY_BACK)
> +   MATRIX_KEY(0x03, 0x02, KEY_REFRESH)
> +   MATRIX_KEY(0x02, 0x02, KEY_ZOOM)
> +   MATRIX_KEY(0x01, 0x02, KEY_SCALE)
> +   MATRIX_KEY(0x03, 0x04, KEY_SYSRQ)
> +   MATRIX_KEY(0x02, 0x04, KEY_BRIGHTNESSDOWN)
> +   MATRIX_KEY(0x01, 0x04, KEY_BRIGHTNESSUP)
> +   MATRIX_KEY(0x02, 0x09, KEY_MUTE)
> +   MATRIX_KEY(0x01, 0x09, KEY_VOLUMEDOWN)
> +   MATRIX_KEY(0x00, 0x04, KEY_VOLUMEUP)
> +
> +   MATRIX_KEY(0x03, 0x09, KEY_SLEEP)   /* LOCK key */
> +
> +   CROS_STD_MAIN_KEYMAP
> +   >;
> +};

I don't love copying all this keymap stuff.  Options I can think of:

1. Just put it in "-rev3".  Have the "-rev2" dts just include the
"-rev3" dts and then override the model/compatible and disable the
charger_thermal.

2. Put the keyboard stuff in the "dtsi" file and then "-rev1" can have
something like:

/delete-node/ keyboard_controller;
#include 

In general the pref

Re: [PATCH 1/4] arm64: dts: qcom: sc7180: lazor: Simplify disabling of charger thermal zone

[Doug Anderson]

Hi,

On Thu, Mar 4, 2021 at 10:04 AM Matthias Kaehlcke  wrote:
>
> Commit f73558cc83d1 ("arm64: dts: qcom: sc7180: Disable charger
> thermal zone for lazor") disables the charger thermal zone for
> specific lazor revisions due to an unsupported thermistor type.
> The initial idea was to disable the thermal zone for older
> revisions and leave it enabled for newer ones that use a
> supported thermistor. Finally the thermistor won't be changed
> on newer revisions, hence the thermal zone should be disabled
> for all lazor (and limozeen) revisions. Instead of disabling
> it per revision do it once in the shared .dtsi for lazor.
>
> Signed-off-by: Matthias Kaehlcke 
> ---
>
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 9 -
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 9 -
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts | 9 -
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi   | 9 +
>  4 files changed, 9 insertions(+), 27 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
> index 5c997cd90069..30e3e769d2b4 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
> @@ -14,15 +14,6 @@ / {
> compatible = "google,lazor-rev0", "qcom,sc7180";
>  };
>
> -/*
> - * Lazor is stuffed with a 47k NTC as charger thermistor which currently is
> - * not supported by the PM6150 ADC driver. Disable the charger thermal zone
> - * to avoid using bogus temperature values.
> - */
> -&charger_thermal {
> -   status = "disabled";
> -};
> -
>  &pp3300_hub {
> /* pp3300_l7c is used to power the USB hub */
> /delete-property/regulator-always-on;
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
> index d9fbcc7bc5bd..c2ef06367baf 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
> @@ -14,15 +14,6 @@ / {
> compatible = "google,lazor-rev1", "google,lazor-rev2", "qcom,sc7180";
>  };
>
> -/*
> - * Lazor is stuffed with a 47k NTC as charger thermistor which currently is
> - * not supported by the PM6150 ADC driver. Disable the charger thermal zone
> - * to avoid using bogus temperature values.
> - */
> -&charger_thermal {
> -   status = "disabled";
> -};
> -
>  &pp3300_hub {
> /* pp3300_l7c is used to power the USB hub */
> /delete-property/regulator-always-on;
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts
> index 19e69adb9e04..1b9d2f46359e 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts
> @@ -13,12 +13,3 @@ / {
> model = "Google Lazor (rev3+)";
> compatible = "google,lazor", "qcom,sc7180";
>  };
> -
> -/*
> - * Lazor is stuffed with a 47k NTC as charger thermistor which currently is
> - * not supported by the PM6150 ADC driver. Disable the charger thermal zone
> - * to avoid using bogus temperature values.
> - */
> -&charger_thermal {
> -   status = "disabled";
> -};
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
> index 89e5cd29ec09..aa2c4a9098db 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
> @@ -58,6 +58,15 @@ ap_ts: touchscreen@10 {
> };
>  };
>
> +/*
> + * Lazor is stuffed with a 47k NTC as charger thermistor which currently is
> + * not supported by the PM6150 ADC driver. Disable the charger thermal zone
> + * to avoid using bogus temperature values.
> + */
> +&charger_thermal {
> +   status = "disabled";
> +};
> +
>  /* PINCTRL - modifications to sc7180-trogdor.dtsi */
>
>  &ts_reset_l {

The idea is right, but I'm having a hard time figuring out what tree
you posted your patch against. You said you did it atop my "v2" series
[1], right?  ...but the "sc7180-trogdor-lazor.dtsi" really doesn't
match. In my tree, for instance, right above the PINCTRL comment
should be:

&wifi {
  qcom,ath10k-calibration-variant = "GO_LAZOR";
};

...but that's definitely not what's there in whatever your patch was
written against... It seems like you're also missing the panel and
trackpad nodes...

[1] https://patchwork.kernel.org/project/linux-arm-msm/list/?series=440315


-Doug

Re: [PATCH v2 0/5] drm/panel-simple: Patches for N116BCA-EA1

[Doug Anderson]

Hi folks,

On Fri, Jan 15, 2021 at 2:44 PM Douglas Anderson  wrote:
>
> This series is to get the N116BCA-EA1 panel working. Most of the
> patches are simple, but on hardware I have in front of me the panel
> sometimes doesn't come up. I'm still working with the hardware
> manufacturer to get to the bottom of it, but I've got it working with
> retries. Adding the retries doesn't seem like an insane thing to do
> and makes some of the error handling more robust, so I've gone ahead
> and included those patches here. Hopefully they look OK.
>
> Changes in v2:
> - Set the "unprepared_time" so if we retry we give the proper delay.
> - ("drm/panel-simple: Don't wait longer for HPD...") new for v2.
> - ("drm/panel-simple: Retry if we timeout waiting for HPD") new for v2.
> - ("dt-bindings: dt-bindings: display: simple: Add N116BCA-EA1") new for v2.
> - ("drm/panel-simple: Add N116BCA-EA1") new for v2.
>
> Douglas Anderson (5):
>   drm/panel-simple: Undo enable if HPD never asserts
>   drm/panel-simple: Don't wait longer for HPD than hpd_absent_delay
>   drm/panel-simple: Retry if we timeout waiting for HPD
>   dt-bindings: dt-bindings: display: simple: Add N116BCA-EA1
>   drm/panel-simple: Add N116BCA-EA1
>
>  .../bindings/display/panel/panel-simple.yaml  |  2 +
>  drivers/gpu/drm/panel/panel-simple.c  | 84 +--
>  2 files changed, 80 insertions(+), 6 deletions(-)

While this isn't massively urgent, I'm hoping to get some confirmation
that it's still in someone's queue to look at.  A quick "it's still in
my queue" would be much appreciated!  :-)  If I don't hear anything
then I guess next week I'll see if I can find other ways to poke folks
or find a different route to land this series.  Thanks!

-Doug

Re: [PATCH 2/3] nvmem: core: Allow nvmem_cell_read_u16/32/64 to read smaller cells

[Doug Anderson]

Hi,

On Fri, Mar 5, 2021 at 2:27 AM Srinivas Kandagatla
 wrote:
>
>
>
> On 27/02/2021 00:26, Douglas Anderson wrote:
> > The current way that cell "length" is specified for nvmem cells is a
> > little fuzzy. For instance, let's look at the gpu speed bin currently
> > in sc7180.dtsi:
> >
> >gpu_speed_bin: gpu_speed_bin@1d2 {
> >  reg = <0x1d2 0x2>;
> >  bits = <5 8>;
> >};
> >
> > This is an 8-bit value (as specified by the "bits" field). However,
> > it has a "length" of 2 (bytes), presumably because the value spans
> > across two bytes.
> >
> > When querying this value right now, it's hard for a client to know if
> > they should be calling nvmem_cell_read_u16() or nvmem_cell_read_u8().
> > Today they must call nvmem_cell_read_u16() because the "length" of the
> > cell was 2 (bytes). However, if a later SoC ever came around and
> > didn't span across 2 bytes it would be unclear.  If a later Soc
> > specified, for instance:
> >
> >gpu_speed_bin: gpu_speed_bin@100 {
> >  reg = <0x100 0x1>;
> >  bits = <0 8>;
> >};
> >
> > ...then the caller would need to change to try calling
> > nvmem_cell_read_u8() because the u16 version would fail.
> >
>
> If the consumer driver is expecting the sizes to span around byte to
> many bytes

I guess in my mind that's outside of the scope of what the consumer
should need to know.  The consumer wants a number and they know it's
stored in nvmem.  They shouldn't need to consider the bit packing
within nvmem.  Imagine that have a structure definition:

struct example {
  int num1:6;
  int num2:6;
  int num3:6;
  int num4:6;
};
struct example e;

What I think you're saying is that you should need a different syntax
for accessing "e.num1" and "e.num4" (because they happen not to span
bytes) compared to accessing "e.num2" and "e.num3". As it is, C
abstracts this out and allows you not to care. You can just do:

e.num1 + e.num2 + e.num3 + e.num4

...and it works fine even though some of those span bytes and some
don't.  I want the same thing.


> , then, Why not just call nvmem_cell_read() which should also
> return you how many bytes it has read!

See my response to patch #1. This requires open-coding a small but
still non-trivial bit of code for all consumers. It should be in the
core.


> > Let's solve this by allowing clients to read a "larger" value. We'll
> > just fill it in with 0.
>
> That is misleading the consumer! If the consumer is expecting a u16 or
> u32, cell size should be of that size!!

If you think it's confusing to change the behavior of the existing
functions, would you be opposed to me adding a new function like
nvmem_cell_read_le_u32_or_smaller() (or provide me a better name) that
would be flexible like this?

-Doug

Re: [PATCH 1/3] drm/msm: Fix speed-bin support not to access outside valid memory

[Doug Anderson]

Hi,

On Fri, Mar 5, 2021 at 2:28 AM Srinivas Kandagatla
 wrote:
>
>
>
> On 27/02/2021 00:26, Douglas Anderson wrote:
> > When running the latest kernel on an sc7180 with KASAN I got this
> > splat:
> >BUG: KASAN: slab-out-of-bounds in a6xx_gpu_init+0x618/0x644
> >Read of size 4 at addr ff8088f36100 by task kworker/7:1/58
> >CPU: 7 PID: 58 Comm: kworker/7:1 Not tainted 5.11.0+ #3
> >Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
> >Workqueue: events deferred_probe_work_func
> >Call trace:
> > dump_backtrace+0x0/0x3a8
> > show_stack+0x24/0x30
> > dump_stack+0x174/0x1e0
> > print_address_description+0x70/0x2e4
> > kasan_report+0x178/0x1bc
> > __asan_report_load4_noabort+0x44/0x50
> > a6xx_gpu_init+0x618/0x644
> > adreno_bind+0x26c/0x438
> >
> > This is because the speed bin is defined like this:
> >gpu_speed_bin: gpu_speed_bin@1d2 {
> >  reg = <0x1d2 0x2>;
> >  bits = <5 8>;
> >};
> >
> > As you can see the "length" is 2 bytes. That means that the nvmem
> > subsystem allocates only 2 bytes. The GPU code, however, was casting
> > the pointer allocated by nvmem to a (u32 *) and dereferencing. That's
> > not so good.
> >
> > Let's fix this to just use the nvmem_cell_read_u16() accessor function
> > which simplifies things and also gets rid of the splat.
> >
> > Let's also put an explicit conversion from little endian in place just
> > to make things clear. The nvmem subsystem today is assuming little
> > endian and this makes it clear. Specifically, the way the above sc7180
> > cell is interpreted:
> >
> > NVMEM:
> >   ++++++
> >   | .. | 0x1d3  | 0x1d2  | .. | 0x000  |
> >   ++++++
> >^   ^
> >   msb lsb
> >
> > You can see that the least significant data is at the lower address
> > which is little endian.
> >
> > NOTE: someone who is truly paying attention might wonder about me
> > picking the "u16" version of this accessor instead of the "u8" (since
> > the value is 8 bits big) or the u32 version (just for fun). At the
> > moment you need to pick the accessor that exactly matches the length
> > the cell was specified as in the device tree. Hopefully future
> > patches to the nvmem subsystem will fix this.
> >
> > Fixes: fe7952c629da ("drm/msm: Add speed-bin support to a618 gpu")
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> >   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 31 +++
> >   1 file changed, 8 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
> > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > index ba8e9d3cf0fe..0e2024defd79 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > @@ -1350,35 +1350,20 @@ static int a6xx_set_supported_hw(struct device 
> > *dev, struct a6xx_gpu *a6xx_gpu,
> >   u32 revn)
> >   {
> >   struct opp_table *opp_table;
> > - struct nvmem_cell *cell;
> >   u32 supp_hw = UINT_MAX;
> > - void *buf;
> > -
> > - cell = nvmem_cell_get(dev, "speed_bin");
> > - /*
> > -  * -ENOENT means that the platform doesn't support speedbin which is
> > -  * fine
> > -  */
> > - if (PTR_ERR(cell) == -ENOENT)
> > - return 0;
> > - else if (IS_ERR(cell)) {
> > - DRM_DEV_ERROR(dev,
> > - "failed to read speed-bin. Some OPPs may not 
> > be supported by hardware");
> > - goto done;
> > - }
> > + u16 speedbin;
> > + int ret;
> >
> > - buf = nvmem_cell_read(cell, NULL);
>
> I think the issue here is not passing len pointer which should return
> how many bytes the cell is!
>
> Then from there we can decide to do le16_to_cpu or le32_to_cpu or not!
> This will also future proof the code to handle speed_bins of different
> sizes!

I think what you're saying is that you want to copy/paste this code
(or something similar) everywhere that accesses an nvmem cell.  Is
that correct?  ...or maybe you can suggest some smaller / shorter code
that I'm missing?

---

{
  struct nvmem_cell *cell;
  ssize_t len;
  char *ret;
  int i;

  *data = 0;

  cell = nvmem_cell_get(dev, cname);
  if (IS_ERR(cell)) {
if (PTR_ERR(cell) != -EPROBE_DEFER)
  dev_err(dev, "undefined cell %s\n", cname);
return PTR_ERR(cell);
  }

  ret = nvmem_cell_read(cell, &len);
  nvmem_cell_put(cell);
  if (IS_ERR(ret)) {
dev_err(dev, "can't read cell %s\n", cname);
return PTR_ERR(ret);
  }

  for (i = 0; i < len; i++)
*data |= ret[i] << (8 * i);

  kfree(ret);
  dev_dbg(dev, "efuse read(%s) = %x, bytes %zd\n", cname, *data, len);

  return 0;
}

---

The above code is from cpr_read_efuse() in "cpr.c".  I mentioned in
the cover letter that I thought about doing this and decided it wasn't
a great idea.  There should be _some_ function in the nvmem core that
says: there's an in

Re: [PATCHv2 3/4] coresight: etm4x: Add support to exclude kernel mode tracing

[Doug Anderson]

Hi,

On Mon, Mar 1, 2021 at 11:05 AM Sai Prakash Ranjan
 wrote:
>
> On production systems with ETMs enabled, it is preferred to exclude
> kernel mode(NS EL1) tracing for security concerns and support only
> userspace(NS EL0) tracing. Perf subsystem interface uses the newly
> introduced kernel config CONFIG_EXCLUDE_KERNEL_PMU_TRACE to exclude
> kernel mode tracing, but there is an additional interface via sysfs
> for ETMs which also needs to be handled to exclude kernel
> mode tracing. So we use this same generic kernel config to handle
> the sysfs mode of tracing. This config is disabled by default and
> would not affect the current configuration which has both kernel and
> userspace tracing enabled by default.
>
> Tested-by: Denis Nikitin 
> Signed-off-by: Sai Prakash Ranjan 
> ---
>  drivers/hwtracing/coresight/coresight-etm4x-core.c  | 6 +-
>  drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 6 ++
>  2 files changed, 11 insertions(+), 1 deletion(-)

Not that I'm an expert in the perf subsystem, but the concern I had
with v1 is now addressed.  FWIW this seems fine to me now.

Reviewed-by: Douglas Anderson 


> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> @@ -296,6 +296,12 @@ static ssize_t mode_store(struct device *dev,
> if (kstrtoul(buf, 16, &val))
> return -EINVAL;
>
> +   if (IS_ENABLED(CONFIG_EXCLUDE_KERNEL_PMU_TRACE) && (!(val & 
> ETM_MODE_EXCL_KERN))) {
> +   dev_warn(dev,
> +   "Kernel mode tracing is not allowed, check your 
> kernel config\n");

slight nit that I think your string needs to be indented by 1 space.  ;-)

Re: [PATCHv2 2/4] perf evsel: Print warning for excluding kernel mode instruction tracing

[Doug Anderson]

Hi,

On Mon, Mar 1, 2021 at 11:05 AM Sai Prakash Ranjan
 wrote:
>
> Add a warning message to check CONFIG_EXCLUDE_KERNEL_HW_ITRACE kernel
> config which excludes kernel mode instruction tracing to help perf tool
> users identify the perf event open failure when they attempt kernel mode
> tracing with this config enabled.
>
> Tested-by: Denis Nikitin 
> Signed-off-by: Sai Prakash Ranjan 
> ---
>  tools/perf/util/evsel.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

I'm not really knowledgeable at all about the perf subsystem so my
review doesn't hold a lot of weight.  However, Sai's patch seems sane
to me.

Reviewed-by: Douglas Anderson 

Re: [PATCHv2 1/4] perf/core: Add support to exclude kernel mode PMU tracing

[Doug Anderson]

Hi,

On Mon, Mar 1, 2021 at 11:05 AM Sai Prakash Ranjan
 wrote:
>
> Hardware assisted tracing families such as ARM Coresight, Intel PT
> provides rich tracing capabilities including instruction level
> tracing and accurate timestamps which are very useful for profiling
> and also pose a significant security risk. One such example of
> security risk is when kernel mode tracing is not excluded and these
> hardware assisted tracing can be used to analyze cryptographic code
> execution. In this case, even the root user must not be able to infer
> anything.
>
> To explain it more clearly in the words of a security team member
> (credits: Mattias Nissler),
>
> "Consider a system where disk contents are encrypted and the encryption
> key is set up by the user when mounting the file system. From that point
> on the encryption key resides in the kernel. It seems reasonable to
> expect that the disk encryption key be protected from exfiltration even
> if the system later suffers a root compromise (or even against insiders
> that have root access), at least as long as the attacker doesn't
> manage to compromise the kernel."
>
> Here the idea is to protect such important information from all users
> including root users since root privileges does not have to mean full
> control over the kernel [1] and root compromise does not have to be
> the end of the world.
>
> But "Peter said even the regular counters can be used for full branch
> trace, the information isn't as accurate as PT and friends and not easier
> but is good enough to infer plenty". This would mean that a global tunable
> config for all kernel mode pmu tracing is more appropriate than the one
> targeting the hardware assisted instruction tracing.
>
> Currently we can exclude kernel mode tracing via perf_event_paranoid
> sysctl but it has following limitations,
>
>  * No option to restrict kernel mode instruction tracing by the
>root user.
>  * Not possible to restrict kernel mode instruction tracing when the
>hardware assisted tracing IPs like ARM Coresight ETMs use an
>additional interface via sysfs for tracing in addition to perf
>interface.
>
> So introduce a new config CONFIG_EXCLUDE_KERNEL_PMU_TRACE to exclude
> kernel mode pmu tracing which will be generic and applicable to all
> hardware tracing families and which can also be used with other
> interfaces like sysfs in case of ETMs.
>
> [1] https://lwn.net/Articles/796866/
>
> Suggested-by: Suzuki K Poulose 
> Suggested-by: Al Grant 
> Tested-by: Denis Nikitin 
> Link: 
> https://lore.kernel.org/lkml/20201015124522.1876-1-saiprakash.ran...@codeaurora.org/
> Signed-off-by: Sai Prakash Ranjan 
> ---
>  init/Kconfig | 11 +++
>  kernel/events/core.c |  3 +++
>  2 files changed, 14 insertions(+)

I'm not really knowledgeable at all about the perf subsystem so my
review doesn't hold a lot of weight.  However, Sai's patch seems sane
to me.

Reviewed-by: Douglas Anderson 

Re: [PATCHv2 4/4] coresight: etm3x: Add support to exclude kernel mode tracing

[Doug Anderson]

Hi,

On Mon, Mar 1, 2021 at 11:05 AM Sai Prakash Ranjan
 wrote:
>
> On production systems with ETMs enabled, it is preferred to exclude
> kernel mode(NS EL1) tracing for security concerns and support only
> userspace(NS EL0) tracing. Perf subsystem interface uses the newly
> introduced kernel config CONFIG_EXCLUDE_KERNEL_PMU_TRACE to exclude
> kernel mode tracing, but there is an additional interface
> via sysfs for ETMs which also needs to be handled to exclude kernel
> mode tracing. So we use this same generic kernel config to handle
> the sysfs mode of tracing. This config is disabled by default and
> would not affect the current configuration which has both kernel and
> userspace tracing enabled by default.
>
> Signed-off-by: Sai Prakash Ranjan 
> ---
>  drivers/hwtracing/coresight/coresight-etm3x-core.c  | 3 +++
>  drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 6 ++
>  2 files changed, 9 insertions(+)

Reviewed-by: Douglas Anderson 


> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c 
> b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> index e8c7649f123e..f522fc2e01b3 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> @@ -116,6 +116,12 @@ static ssize_t mode_store(struct device *dev,
> if (ret)
> return ret;
>
> +   if (IS_ENABLED(CONFIG_EXCLUDE_KERNEL_PMU_TRACE) && (!(val & 
> ETM_MODE_EXCL_KERN))) {
> +   dev_warn(dev,
> +   "Kernel mode tracing is not allowed, check your 
> kernel config\n");

Same nit as in patch #3 that the above string should be indented by 1
more space.

Re: [PATCH v5] kdb: Simplify kdb commands registration

[Doug Anderson]

Hi,

On Tue, Feb 23, 2021 at 11:08 PM Sumit Garg  wrote:
>
> Simplify kdb commands registration via using linked list instead of
> static array for commands storage.
>
> Signed-off-by: Sumit Garg 
> ---
>
> Changes in v5:
> - Introduce new method: kdb_register_table() to register static kdb
>   main and breakpoint command tables instead of using statically
>   allocated commands.
>
> Changes in v4:
> - Fix kdb commands memory allocation issue prior to slab being available
>   with an array of statically allocated commands. Now it works fine with
>   kgdbwait.
> - Fix a misc checkpatch warning.
> - I have dropped Doug's review tag as I think this version includes a
>   major fix that should be reviewed again.
>
> Changes in v3:
> - Remove redundant "if" check.
> - Pick up review tag from Doug.
>
> Changes in v2:
> - Remove redundant NULL check for "cmd_name".
> - Incorporate misc. comment.
>
>  kernel/debug/kdb/kdb_bp.c  |  81 --
>  kernel/debug/kdb/kdb_main.c| 472 -
>  kernel/debug/kdb/kdb_private.h |   3 +
>  3 files changed, 343 insertions(+), 213 deletions(-)

This looks good to me, thanks!

Random notes:

* We no longer check for "duplicate" commands for any of these
statically allocated ones, but I guess that's fine.

* Presumably nothing outside of kdb/kgdb itself needs the ability to
allocate commands statically.  The only user I see now is ftrace and
it looks like it runs late enough that it should be fine.

Reviewed-by: Douglas Anderson 


-Doug

Re: [PATCH 13/13] arm64: dts: qcom: Add sc7180-lazor-coachz skus

[Doug Anderson]

Hi,

On Fri, Feb 26, 2021 at 10:45 AM Stephen Boyd  wrote:
>
> Quoting Douglas Anderson (2021-02-25 14:13:10)
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi 
> > b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> > new file mode 100644
> > index ..5def9953d82b
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> > @@ -0,0 +1,249 @@
> [...]
> > +
> > +/*
> > + * There's no SAR sensor, so i2c5 is re-purposed.  We leave the
> > + * proximity@28 node under i2c5 (from trogdor.dtsi) since it's "disabled"
> > + * and doesn't hurt.
> > + */
> > +i2c_wlc: &i2c5 {
> > +   /* Currently not connected to anything; see b/168652326 */
> > +};
>
> Can we remove this? As far as I know this will always be this way and
> thus doesn't provide anything meaningful to leave this bug comment here
> that doesn't work for people.

Yeah, that sounds good.  We just want to delete this whole comment and the node.

That seems like enough reason to repost the series.  I'll plan to do
it early next week.  Of course, I wouldn't object to any of these
things:
* This patch landing and having the node and I'll do a follow-up patch
to remove it.
* Bjorn removing the comment and node as he applies.

-Doug

Re: [PATCH 02/13] arm64: dts: qcom: Move sc7180 MI2S config to board files and make pulldown

[Doug Anderson]

Hi,

On Thu, Feb 25, 2021 at 2:55 PM Konrad Dybcio
 wrote:
>
> Hi,
>
>
> >
> > +&pri_mi2s_active {
> > + pinconf {
> > + pins = "gpio53", "gpio54", "gpio55", "gpio56";
> > + drive-strength = <2>;
> > + bias-pull-down;
> > + };
> > +};
> > +
>
> You can omit pinconf{}, so the outcome would be:
> &pri_mi2s_active {
>
> pins = ...
>
> ...
>
> };
>
>
> This makes the DTs ever so shorter and is the style that's currently used for 
> new submissions.
>
> Same goes for the nodes that are being referenced.

Yes, I agree.  That definitely makes sense going forward, but I think
it'll just add to the confusion to switch a dts for a given SoC
mid-stride.  ...or, if we do switch the style it should be done in a
separate (no-op) patch series.  This series is already giant enough...

-Doug

Re: [PATCH v2 3/4] arm64: dts: qcom: sc7180: trogdor: Fix trip point config of charger thermal zone

[Doug Anderson]

Hi,

On Thu, Feb 25, 2021 at 10:33 AM Matthias Kaehlcke  wrote:
>
> The trip point configuration of the charger thermal zone for trogdor
> is missing a node for the critical trip point. Add the missing node.
>
> Fixes: bb06eb3607e9 ("arm64: qcom: sc7180: trogdor: Add ADC nodes and thermal 
> zone for charger thermistor")
> Signed-off-by: Matthias Kaehlcke 
> ---
>
> Changes in v2:
> - patch added to the series
>
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index bda983da4eaf..ab4efaece5cb 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -22,9 +22,11 @@ charger_thermal: charger-thermal {
> thermal-sensors = <&pm6150_adc_tm 1>;
>
> trips {
> -   temperature = <125000>;
> -   hysteresis = <1000>;
> -   type = "critical";
> +   charger-crit {

If you happen to spin for some other reason, it might not hurt to add
a label to this node.  Best case it'll save a patch in the future when
some OEM decides that they need to adjust this temperature and worst
case it doesn't hurt.

In any case:

Reviewed-by: Douglas Anderson 

Re: [PATCH 12/13] arm64: dts: qcom: Add sc7180-lazor-pompom skus

[Doug Anderson]

Hi,

On Thu, Feb 25, 2021 at 2:13 PM Douglas Anderson  wrote:
>
> This is a trogdor variant.  This is mostly a grab from the downstream
> tree with notable exceptions:
> - I skip -rev0.  This was a super early build and there's no advantage
>   of long term support.
> - In -rev1 I translate the handling of the USB hub like is done for
>   similar boards.  See the difference between the downstream and
>   upstream 'sc7180-trogdor-lazor-r0.dts' for an example.  This will
>   need to be resolved when proper support for the USB hub is figured
>   out upstream.
> - I remove sound node since sound hasn't landed upstream yet.
> - In incorporate the pending  for the
>   keyboard.
>
> Cc: Philip Chen 
> Cc: Matthias Kaehlcke 
> Cc: Stephen Boyd 
> Cc: Tzung-Bi Shih 
> Cc: Judy Hsiao 
> Signed-off-by: Douglas Anderson 
> ---
>
>  arch/arm64/boot/dts/qcom/Makefile |   4 +
>  .../dts/qcom/sc7180-trogdor-pompom-r1-lte.dts |  14 +
>  .../dts/qcom/sc7180-trogdor-pompom-r1.dts |  26 ++
>  .../dts/qcom/sc7180-trogdor-pompom-r2-lte.dts |  14 +
>  .../dts/qcom/sc7180-trogdor-pompom-r2.dts |  44 +++
>  .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi  | 288 ++
>  6 files changed, 390 insertions(+)

As always I see a typo right after I hit send.  ${SUBJECT} should be
"trogdor-pompom", not "lazor-pompom" for this and the next patch.  I
copy/pastad from the previous patch (limozeen _is_ actually a lazor)
and didn't adjust properly.

I'm happy with any:
* to spin a -r2
* for Bjorn to fix when landing
* for this just to land since it's a harmless typo.

In any case I won't spin for now but I wanted to reply so if I do spin
for another reason I'll won't forget.  ;-)

-Doug

Re: [PATCH] Asoc: qcom: dts: Change MI2S GPIO configuration to pulldown

[Doug Anderson]

Hi,

On Mon, Nov 16, 2020 at 3:35 AM Srinivasa Rao Mandadapu
 wrote:
>
> From: V Sujith Kumar Reddy 
>
> Change LPASS MI2S gpio configuration to pull down from pull up.
>
> Fixes: 9b72f4e6a3f8 (arm64: dts: qcom: sc7180: Add lpass cpu node for I2S 
> driver)
>
> Signed-off-by: V Sujith Kumar Reddy 
> Signed-off-by: Srinivasa Rao Mandadapu 
> ---
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)

Should be replaced with:

[PATCH 02/13] arm64: dts: qcom: Move sc7180 MI2S config to board files
and make pulldown

https://lore.kernel.org/r/20210225141022.2.Id27e7e6f90c29bf623fa4880e18a14ba1dffd2d2@changeid

-Doug

Re: [PATCH v4 4/5] arm64: dts: qcom: sc7180: Use pdc interrupts for USB instead of GIC interrupts

[Doug Anderson]

Hi,

On Tue, Oct 27, 2020 at 1:38 PM Sandeep Maheswaram  wrote:
>
> Using pdc interrupts for USB instead of GIC interrupts to
> support wake up in case of XO shutdown.
>
> Signed-off-by: Sandeep Maheswaram 
> Reviewed-by: Stephen Boyd 
> ---
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi 
> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index a02776c..a2c56528 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -2687,10 +2687,10 @@
>   <&gcc GCC_USB30_PRIM_MASTER_CLK>;
> assigned-clock-rates = <1920>, <15000>;
>
> -   interrupts = ,
> -,
> -,
> -;
> +   interrupts-extended = <&intc GIC_SPI 131 
> IRQ_TYPE_LEVEL_HIGH>,
> + <&pdc 6 IRQ_TYPE_LEVEL_HIGH>,
> + <&pdc 8 IRQ_TYPE_LEVEL_HIGH>,
> + <&pdc 9 IRQ_TYPE_LEVEL_HIGH>;

Is there any reason that this patch can't land?  I'm not sure what the
current status of everything is, but it should be fine to go through
the PDC anyway, right?

-Doug

Re: [PATCH] kgdb: Fix to kill breakpoints on initmem after boot

[Doug Anderson]

Hi,

On Wed, Feb 24, 2021 at 12:17 AM Sumit Garg  wrote:
>
> Currently breakpoints in kernel .init.text section are not handled
> correctly while allowing to remove them even after corresponding pages
> have been freed.
>
> Fix it via killing .init.text section breakpoints just prior to initmem
> pages being freed.

It might be worth it to mention that HW breakpoints aren't handled by
this patch but it's probably not such a big deal.


> Suggested-by: Doug Anderson 
> Signed-off-by: Sumit Garg 
> ---
>  include/linux/kgdb.h  |  2 ++
>  init/main.c   |  1 +
>  kernel/debug/debug_core.c | 11 +++
>  3 files changed, 14 insertions(+)
>
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index 57b8885708e5..3aa503ef06fc 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -361,9 +361,11 @@ extern atomic_tkgdb_active;
>  extern bool dbg_is_early;
>  extern void __init dbg_late_init(void);
>  extern void kgdb_panic(const char *msg);
> +extern void kgdb_free_init_mem(void);
>  #else /* ! CONFIG_KGDB */
>  #define in_dbg_master() (0)
>  #define dbg_late_init()
>  static inline void kgdb_panic(const char *msg) {}
> +static inline void kgdb_free_init_mem(void) { }
>  #endif /* ! CONFIG_KGDB */
>  #endif /* _KGDB_H_ */
> diff --git a/init/main.c b/init/main.c
> index c68d784376ca..a446ca3d334e 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1417,6 +1417,7 @@ static int __ref kernel_init(void *unused)
> async_synchronize_full();
> kprobe_free_init_mem();
> ftrace_free_init_mem();
> +   kgdb_free_init_mem();
> free_initmem();
> mark_readonly();
>
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 229dd119f430..319381e95d1d 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -465,6 +465,17 @@ int dbg_remove_all_break(void)
> return 0;
>  }
>
> +void kgdb_free_init_mem(void)
> +{
> +   int i;
> +
> +   /* Clear init memory breakpoints. */
> +   for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> +   if (init_section_contains((void *)kgdb_break[i].bpt_addr, 0))

A nit, but instead of 0 should this be passing "BREAK_INSTR_SIZE" ?

Also: even if memory is about to get freed it still seems like it'd be
wise to call this:

  kgdb_arch_remove_breakpoint(&kgdb_break[i]);

It looks like it shouldn't matter today but just in case an
architecture decides to do something fancy in the future it might not
hurt to tell it that the breakpoint is going away.


Everything here is pretty nitty, though.  This looks good to me now.

Reviewed-by: Douglas Anderson 

Re: [PATCH v2] kdb: Remove redundant function definitions/prototypes

[Doug Anderson]

Hi,

On Tue, Feb 23, 2021 at 11:17 PM Sumit Garg  wrote:
>
> Cleanup kdb code to get rid of unused function definitions/prototypes.
>
> Signed-off-by: Sumit Garg 
> ---
>
> Changes in v2:
> - Keep kdbgetu64arg() the way it was.
>
>  kernel/debug/kdb/kdb_private.h |  2 --
>  kernel/debug/kdb/kdb_support.c | 18 --
>  2 files changed, 20 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH] kdb: Remove redundant function definitions/prototypes

[Doug Anderson]

Hi,

On Tue, Feb 23, 2021 at 4:01 AM Sumit Garg  wrote:
>
> @@ -103,7 +103,6 @@ extern int kdb_getword(unsigned long *, unsigned long, 
> size_t);
>  extern int kdb_putword(unsigned long, unsigned long, size_t);
>
>  extern int kdbgetularg(const char *, unsigned long *);
> -extern int kdbgetu64arg(const char *, u64 *);

IMO you should leave kdbgetu64arg() the way it was.  It is symmetric
to all of the other similar functions and even if there are no
external users of kdbgetu64arg() now it seems like it makes sense to
keep it matching.


> @@ -209,9 +208,7 @@ extern unsigned long kdb_task_state(const struct 
> task_struct *p,
> unsigned long mask);
>  extern void kdb_ps_suppressed(void);
>  extern void kdb_ps1(const struct task_struct *p);
> -extern void kdb_print_nameval(const char *name, unsigned long val);
>  extern void kdb_send_sig(struct task_struct *p, int sig);
> -extern void kdb_meminfo_proc_show(void);

Getting rid of kdb_print_nameval() / kdb_meminfo_proc_show() makes sense to me.


>  extern char kdb_getchar(void);
>  extern char *kdb_getstr(char *, size_t, const char *);
>  extern void kdb_gdb_state_pass(char *buf);
> diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
> index 6226502ce049..b59aad1f0b55 100644
> --- a/kernel/debug/kdb/kdb_support.c
> +++ b/kernel/debug/kdb/kdb_support.c
> @@ -665,24 +665,6 @@ unsigned long kdb_task_state(const struct task_struct 
> *p, unsigned long mask)
> return (mask & kdb_task_state_string(state)) != 0;
>  }
>
> -/*
> - * kdb_print_nameval - Print a name and its value, converting the
> - * value to a symbol lookup if possible.
> - * Inputs:
> - * namefield name to print
> - * val value of field
> - */
> -void kdb_print_nameval(const char *name, unsigned long val)
> -{
> -   kdb_symtab_t symtab;
> -   kdb_printf("  %-11.11s ", name);
> -   if (kdbnearsym(val, &symtab))
> -   kdb_symbol_print(val, &symtab,
> -KDB_SP_VALUE|KDB_SP_SYMSIZE|KDB_SP_NEWLINE);
> -   else
> -   kdb_printf("0x%lx\n", val);
> -}
> -

Getting rid of kdb_print_nameval() makes sense to me.

-Doug

Re: [PATCH] kernel: debug: Handle breakpoints in kernel .init.text section

[Doug Anderson]

Hi,

On Fri, Feb 19, 2021 at 12:03 AM Sumit Garg  wrote:
>
> Currently breakpoints in kernel .init.text section are not handled
> correctly while allowing to remove them even after corresponding pages
> have been freed.
>
> In order to keep track of .init.text section breakpoints, add another
> breakpoint state as BP_ACTIVE_INIT and don't try to free these
> breakpoints once the system is in running state.
>
> To be clear there is still a very small window between call to
> free_initmem() and system_state = SYSTEM_RUNNING which can lead to
> removal of freed .init.text section breakpoints but I think we can live
> with that.

I know kdb / kgdb tries to keep out of the way of the rest of the
system and so there's a bias to just try to infer the state of the
rest of the system, but this feels like a halfway solution when really
a cleaner solution really wouldn't intrude much on the main kernel.
It seems like it's at least worth asking if we can just add a call
like kgdb_drop_init_breakpoints() into main.c.  Then we don't have to
try to guess the state...


> Suggested-by: Peter Zijlstra 
> Signed-off-by: Sumit Garg 
> ---
>  include/linux/kgdb.h  |  3 ++-
>  kernel/debug/debug_core.c | 17 +
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index 0d6cf64..57b8885 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -71,7 +71,8 @@ enum kgdb_bpstate {
> BP_UNDEFINED = 0,
> BP_REMOVED,
> BP_SET,
> -   BP_ACTIVE
> +   BP_ACTIVE_INIT,
> +   BP_ACTIVE,
>  };
>
>  struct kgdb_bkpt {
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index af6e8b4f..229dd11 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -324,7 +324,11 @@ int dbg_activate_sw_breakpoints(void)
> }
>
> kgdb_flush_swbreak_addr(kgdb_break[i].bpt_addr);
> -   kgdb_break[i].state = BP_ACTIVE;
> +   if (system_state >= SYSTEM_RUNNING ||
> +   !init_section_contains((void *)kgdb_break[i].bpt_addr, 0))

I haven't searched through all the code, but is there any chance that
this could trigger incorrectly?  After we free the init memory could
it be re-allocated to something that would contain code that would
execute in kernel context and now we'd be unable to set breakpoints in
that area?


> +   kgdb_break[i].state = BP_ACTIVE;
> +   else
> +   kgdb_break[i].state = BP_ACTIVE_INIT;

I don't really see what the "BP_ACTIVE_INIT" state gets you.  Why not
just leave it as "BP_ACTIVE" and put all the logic fully in
dbg_deactivate_sw_breakpoints()?

...or, if we can inject a call in main.c we can do a one time delete
of all "init" breakpoints and get rid of all this logic?  Heck, even
if we can't get called by "main.c", we still only need to do a
one-time drop of all init breakpoints the first time we drop into the
debugger after they are freed, right?

Oh shoot.  I just realized another problem.  What about hardware
breakpoints?  You still need to call "remove" on them once after init
memory is freed, right?

-Doug