Linux-SPI Archive on lore.kernel.org
 help / color / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Evan Green <evgreen@chromium.org>
Cc: Akash Asthana <akashast@codeaurora.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andy Gross <agross@kernel.org>,
	wsa@the-dreams.de, Mark Brown <broonie@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-i2c@vger.kernel.org, linux-spi@vger.kernel.org,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>, Stephen Boyd <swboyd@chromium.org>,
	Manu Gautam <mgautam@codeaurora.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	linux-serial@vger.kernel.org,
	Doug Anderson <dianders@chromium.org>
Subject: Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash
Date: Fri, 27 Mar 2020 16:23:38 -0700
Message-ID: <20200327232338.GL5063@builder> (raw)
In-Reply-To: <CAE=gft6AGkcdUAkoyevZgmtBgaiEkoQzzJcg7sYjbpy5Kh2fyA@mail.gmail.com>

On Fri 20 Mar 09:30 PDT 2020, Evan Green wrote:

> On Fri, Mar 20, 2020 at 3:22 AM Akash Asthana <akashast@codeaurora.org> wrote:
> >
> > Hi Evan, Matthias,
> >
> > On 3/20/2020 1:13 AM, Matthias Kaehlcke wrote:
> > > On Wed, Mar 18, 2020 at 02:24:35PM +0530, Akash Asthana wrote:
> > >> Hi Matthias,
> > >>
> > >> On 3/17/2020 11:59 PM, Matthias Kaehlcke wrote:
> > >>> Hi Akash,
> > >>>
> > >>> On Tue, Mar 17, 2020 at 04:27:47PM +0530, Akash Asthana wrote:
> > >>>> Hi Matthias,
> > >>>>
> > >>>> On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
> > >>>>> Hi Akash,
> > >>>>>
> > >>>>> On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
> > >>>>>> V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
> > >>>>>> to reset at boot time.
> > >>>>> The v1 patch isn't relevant in the commit message, please just describe the
> > >>>>> problem. Also the crash only occurs when earlycon is used.
> > >>>> ok
> > >>>>>> As QUP core clock is shared among all the SE drivers present on particular
> > >>>>>> QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
> > >>>>>> is put to 0 from other SE drivers before real console comes up.
> > >>>>>>
> > >>>>>> As earlycon can't vote for it's QUP core need, to fix this add ICC
> > >>>>>> support to common/QUP wrapper driver and put vote for QUP core from
> > >>>>>> probe on behalf of earlycon and remove vote during sys suspend.
> > >>>>> Only removing the vote on suspend isn't ideal, the system might never get
> > >>>>> suspended. That said I don't have a really good alternative suggestion.
> > >>>>>
> > >>>>> One thing you could possibly do is to launch a delayed work, check
> > >>>>> console_device() every second or so and remove the vote when it returns
> > >>>>> non-NULL. Not claiming this would be a great solution ...
> > >>>>>
> > >>>>> The cleanest solution might be a notifier when the early console is
> > >>>>> unregistered, it seems somewhat over-engineered though ... Then again
> > >>>>> other (future) uart drivers with interconnect support might run into
> > >>>>> the same problem.
> > >>>> We are hitting this problem because QUP core clocks are shared among all the
> > >>>> SE driver present in particular QUP wrapper, if other HW controllers has
> > >>>> similar architecture we will hit this issue.
> > >>>>
> > >>>> How about if we expose an API from common driver(geni-se) for putting QUP
> > >>>> core BW vote to 0.
> > >>>>
> > >>>> We call this from console probe just after uart_add_one_port call (console
> > >>>> resources are enabled as part of this call) to put core quota to 0 on behalf
> > >>>> of earlyconsole?
> > >>>   From my notes from earlier debugging I have doubts this would work:
> > >>>
> > >>>     There is a short window where the early console and the 'real' console coexist:
> > >>>
> > >>>     [    3.858122] printk: console [ttyMSM0] enabled
> > >>>     [    3.875692] printk: bootconsole [qcom_geni0] disabled
> > >>>
> > >>>     The reset probably occurs when the early console tries to write, but the ICC
> > >>>     is effectively disabled because ttyMSM0 and the other geni ports are runtime
> > >>>     suspended.
> > >> Code flow from console driver probe(qcom_geni_serial.c)
> > >>
> > >> uart_add_one_port--->uart_configure_port--->{ 1) uart_change_pm(enable
> > >> console resources)  2)register_console(boot to real console switch happens
> > >> here)}
> > >>
> > >> Console resources are not disabled from anywhere before the switch happens
> > >> completely. I meant to say until we saw below logs.
> > >>
> > >> [    3.875692] printk: bootconsole [qcom_geni0] disabled
> > >>
> > >> I think the board reset issue cannot occur during the window where early
> > >> console and 'real' console coexist.
> > > Thanks for the clarification! Indeed my notes were only a hypothesis, I
> > > don't see evidence that there is an actual downvote shortly after console
> > > registration.
> > >
> > >> I have validated proposed solution by me, it is working fine.
> > >>
> > >> Currently voting is done for every QUP and not only to which earlycon is
> > >> connect, with the above approach we can't remove vote from other QUPs.
> > >>
> > >> However we can limit voting only to earlycon QUP by removing interconnect
> > >> from DT node of other QUPs.
> > >>
> > >> I am not sure how clean is this solution.
> > > I'm more inclined towards a solution along the lines of what Evan
> > > proposed, i.e. delaying the votes (either in geni or ICC) until we
> > > are ready.
> >
> > Based on discussion I think the delayed solution is most suited if
> > implemented in ICC core because other ICC client might face the similar
> > problem.
> >
> > However for geni case I am more inclined towards below proposed solution.
> >
> > -----------------------------------------------------------------------------------------------------
> >
> > How about if we expose an API from common driver(geni-se) for putting QUP
> > core BW vote to 0.
> >
> > We call this from console probe just after uart_add_one_port call (console
> > resources are enabled as part of this call) to put core quota to 0 on behalf
> > of earlyconsole?
> 
> This seems ok to me. Earlycon sets up a vote, and then real probe
> tears it down. As long as in the shuffle of all of these things into
> SE library helpers you still have a way of differentiating the
> earlycon vote from the real vote.

Note though that the boot console will outlive the real console when the
kernel is booted with "keep_bootcon" on the command line (something I do
from time to time).

So rather than relying on "real probe" to signal when we can release the
earlycon's icc vote I think we should specify dev->con->exit in
qcom_geni_serial_earlycon_setup(), so that it will signal when the
earlycon actually goes away - and until that point the clocks should
just be on.

> In other words, don't reuse this
> early icc_path for the real UART vote. You should probably also
> destroy the path once you've voted zero on it.

Maintaining the early and real votes completely separate sounds like a
sure way to keep this sane; and there's no drawback on having multiple
votes for the same thing and rely on the framework to keep track of the
various users.

Regards,
Bjorn

  parent reply index

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-13 13:12 [PATCH V2 0/8] Add interconnect support to QSPI and QUP drivers Akash Asthana
     [not found] ` <1584105134-13583-1-git-send-email-akashast-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-13 13:12   ` [PATCH V2 1/8] interconnect: Add devm_of_icc_get() as exported API for users Akash Asthana
2020-03-13 16:26     ` Matthias Kaehlcke
2020-03-27 23:02     ` Bjorn Andersson
2020-03-13 13:12   ` [PATCH V2 4/8] tty: serial: qcom_geni_serial: Add interconnect support Akash Asthana
     [not found]     ` <1584105134-13583-5-git-send-email-akashast-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-13 21:28       ` Matthias Kaehlcke
     [not found]         ` <20200313212833.GK144492-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-17 11:48           ` Akash Asthana
2020-03-17 19:08             ` Matthias Kaehlcke
2020-03-18 12:23               ` Akash Asthana
     [not found]                 ` <e95bd6e0-d1fd-9d13-47df-f7d08b443e37-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-19 20:42                   ` Matthias Kaehlcke
     [not found]                     ` <20200319204248.GA204494-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-20 10:35                       ` Akash Asthana
2020-03-13 13:12   ` [PATCH V2 5/8] i2c: i2c-qcom-geni: " Akash Asthana
2020-03-14  0:17     ` Matthias Kaehlcke
2020-03-17 11:51       ` Akash Asthana
2020-03-13 13:12   ` [PATCH V2 6/8] spi: spi-geni-qcom: " Akash Asthana
     [not found]     ` <1584105134-13583-7-git-send-email-akashast-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-13 13:16       ` Mark Brown
2020-03-17  9:35         ` Akash Asthana
     [not found]           ` <aa197568-3bac-6962-d39d-3261f68c0514-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-17 13:06             ` Mark Brown
2020-03-20 13:52               ` Akash Asthana
2020-03-14  0:41     ` Matthias Kaehlcke
     [not found]       ` <20200314004106.GM144492-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-17 12:11         ` Akash Asthana
2020-03-13 13:12 ` [PATCH V2 2/8] soc: qcom: geni: Support for ICC voting Akash Asthana
2020-03-13 16:42   ` Matthias Kaehlcke
     [not found]     ` <20200313164207.GH144492-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-17  9:58       ` Akash Asthana
2020-03-17 19:06   ` Evan Green
     [not found]     ` <74851dda-296d-cdc5-2449-b9ec59bbc057@codeaurora.org>
2020-03-20 16:45       ` Evan Green
2020-03-27  5:33         ` Akash Asthana
2020-03-13 13:12 ` [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash Akash Asthana
2020-03-13 20:44   ` Matthias Kaehlcke
     [not found]     ` <20200313204441.GJ144492-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-17 10:57       ` Akash Asthana
     [not found]         ` <1f86fdf0-df7c-4e4a-d4d8-8b0162e52cb4-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-17 18:29           ` Matthias Kaehlcke
2020-03-18  8:54             ` Akash Asthana
2020-03-19 19:43               ` Matthias Kaehlcke
     [not found]                 ` <20200319194332.GA60149-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-20 10:22                   ` Akash Asthana
2020-03-20 16:30                     ` Evan Green
2020-03-27  5:04                       ` Akash Asthana
2020-03-27 23:23                       ` Bjorn Andersson [this message]
2020-03-31 10:55                         ` Akash Asthana
2020-03-17 19:08           ` Evan Green
2020-03-17 19:46             ` Doug Anderson
     [not found]             ` <CAE=gft5GcOeQ5kh1bGen_P0J98g2XaAJ7NrDsxkirDoLtL4GWg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2020-03-18 10:57               ` Akash Asthana
2020-03-18 16:22                 ` Evan Green
2020-03-13 13:12 ` [PATCH V2 7/8] spi: spi-qcom-qspi: Add interconnect support Akash Asthana
2020-03-14  0:58   ` Matthias Kaehlcke
     [not found]     ` <20200314005817.GN144492-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-17 12:13       ` Akash Asthana
2020-03-17 19:08         ` Evan Green
2020-03-18 13:48           ` Akash Asthana
     [not found]             ` <e2ee1a60-a379-5c78-355a-64aad451a944-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-18 16:30               ` Evan Green
2020-03-20  5:35                 ` Akash Asthana
2020-03-13 13:12 ` [PATCH V2 8/8] arm64: dts: sc7180: Add interconnect for QUP and QSPI Akash Asthana

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200327232338.GL5063@builder \
    --to=bjorn.andersson@linaro.org \
    --cc=agross@kernel.org \
    --cc=akashast@codeaurora.org \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=evgreen@chromium.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mgautam@codeaurora.org \
    --cc=robh+dt@kernel.org \
    --cc=swboyd@chromium.org \
    --cc=wsa@the-dreams.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-SPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-spi/0 linux-spi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-spi linux-spi/ https://lore.kernel.org/linux-spi \
		linux-spi@vger.kernel.org
	public-inbox-index linux-spi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-spi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git