* Re: [PATCH v6 6/6] arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and Makefile
From: Matthias Brugger @ 2019-02-07 15:08 UTC (permalink / raw)
To: Erin Lo, Rob Herring
Cc: Mark Rutland, Ben Ho, Mars Cheng, Mengqi Zhang, linux-clk,
Hsin-Hsiung Wang, Weiyi Lu, Marc Zyngier,
open list:SERIAL DRIVERS, Yingjoe Chen, devicetree, Jason Cooper,
Seiya Wang, moderated list:ARM/Mediatek SoC support,
Thomas Gleixner, Eddie Huang,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <1548997866.26127.4.camel@mtksdaap41>
On 01/02/2019 06:11, Erin Lo wrote:
> Add back more people since mail server issue
>
> On Fri, 2019-02-01 at 11:33 +0800, Erin Lo wrote:
>> On Thu, 2019-01-31 at 15:10 -0600, Rob Herring wrote:
>>> On Wed, Jan 30, 2019 at 8:34 PM Erin Lo <erin.lo@mediatek.com> wrote:
>>>>
>>>> On Wed, 2019-01-30 at 10:22 -0600, Rob Herring wrote:
>>>>> On Thu, Jan 24, 2019 at 04:07:20PM +0800, Erin Lo wrote:
>>>>>> From: Ben Ho <Ben.Ho@mediatek.com>
>>>>>>
>>>>>> Add basic chip support for Mediatek 8183, include
>>>>>> pinctrl file, uart node with correct uart clocks, pwrap device
>>>>>>
>>>>>> Add clock controller nodes, include topckgen, infracfg,
>>>>>> apmixedsys and subsystem.
>>>>>>
>>>>>> Signed-off-by: Ben Ho <Ben.Ho@mediatek.com>
>>>>>> Signed-off-by: Erin Lo <erin.lo@mediatek.com>
>>>>>> Signed-off-by: Seiya Wang <seiya.wang@mediatek.com>
>>>>>> Signed-off-by: Zhiyong Tao <zhiyong.tao@mediatek.com>
>>>>>> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
>>>>>> Signed-off-by: Mengqi Zhang <Mengqi.Zhang@mediatek.com>
>>>>>> Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
>>>>>> Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
>>>>>> ---
>>>>>
>>>>>
>>>>>> + sysirq: intpol-controller@c530a80 {
>>>>>
>>>>> interrupt-controller@...
>>>>
>>>> I will modify it in next version.
>>>>>
>>>>>
>>>>> Place all the MMIO peripherals under one or more simple-bus nodes.
>>>>>
>>>>> Rob
>>>>>
>>>>
>>>> Do you mean need to add simple-bus like this?
>>>
>>> Yes.
>>
>> We remove soc because Matthias suggested it in former MTK SoC maybe in
>> 2015 year.
>>
>> We will add it back by your comment.
>>
>> Thank you.
>>
>> Best Regards,
>> Erin
>
> Hi, Matthias,
> Do you have any comment here?
Although I wasn't able to find it in the documentation my understanding is, that
all devices on-chip should be under soc "bus".
I'm sorry if I created confusion with comments in the past.
Regards,
Matthias
> Thanks
>
> Best Regards,
> Erin
>>>
>>>>
>>>> + soc: soc {
>>>> + #address-cells = <0x1>;
>>>> + #size-cells = <0x1>;
>>>> + ranges = <0 0 0 0xffffffff>;
>>>> + compatible = "simple-bus";
>>>>
>>>> soc_data: soc_data@08000000 {
>>>> compatible = "mediatek,mt8183-efuse",
>>>> "mediatek,efuse";
>>>> reg = <0 0x08000000 0 0x0010>;
>>>> #address-cells = <1>;
>>>> #size-cells = <1>;
>>>> status = "disabled";
>>>> };
>>>>
>>>> gic: interrupt-controller@0c000000 {
>>>> compatible = "arm,gic-v3";
>>>> #interrupt-cells = <4>;
>>>>
>>>> Best Regards,
>>>> Erin
>>>>
>>>>> _______________________________________________
>>>>> Linux-mediatek mailing list
>>>>> Linux-mediatek@lists.infradead.org
>>>>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
>
^ permalink raw reply
* Re: [RFC/PATCH 0/5] DVFS in the OPP core
From: Ulf Hansson @ 2019-02-07 13:37 UTC (permalink / raw)
To: Stephen Boyd
Cc: Viresh Kumar, Graham Roff, Mike Turquette,
Linux Kernel Mailing List, linux-arm-msm, Linux PM, linux-serial,
linux-spi, Rajendra Nayak, Doug Anderson, Vincent Guittot
In-Reply-To: <154952629766.115909.11259861549408107064@swboyd.mtv.corp.google.com>
On Thu, 7 Feb 2019 at 08:58, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Viresh Kumar (2019-01-31 01:23:49)
> > Adding few folks to the thread who might be interested in this stuff.
> >
> > On 28-01-19, 17:55, Stephen Boyd wrote:
> > > This patch series is an RFC around how we can implement DVFS for devices
> > > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > > strict set of frequencies that they have been tested at to derive some
> > > operating performance point. Instead they have a coarser set of
> > > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > > device can operate at with a given voltage.
> > >
> > > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > > as a valid frequency indicating the frequency isn't required anymore and
> > > to make the same API use the clk framework to round the frequency passed
> > > in instead of relying on the OPP table to specify each frequency that
> > > can be used. Once we have these two patches in place, we can use the OPP
> > > API to change clk rates instead of clk_set_rate() and use all the recent
> > > OPP enhancements that have been made around required-opps and genpd
> > > performance states to do DVFS for all devices.
> >
> > Generally speaking I am fine with such an approach but I am not sure
> > about what others would say on this as they had objections to using
> > OPP core for setting the rate itself.
> >
> > FWIW, I suggested exactly this solution sometime back [1]
> >
> > - Drivers need to use two API sets to change clock rate (OPP helpers)
> > and enable/disable them (CLK framework helpers) and this leads us to
> > exactly that combination. Is that acceptable ? It doesn't look great
> > to me as well..
>
> Agreed. I don't think anyone thinks this looks great, but I'll argue
> that it's improving OPP for devices that already use it so that we can
> remove voltage requirements when their clk is off. Think about CPUs that
> are in their own clk domain where we want to remove the voltage
> requirement when those CPUs are offline, or a GPU that wants to remove
> its voltage requirement when it turns off clks. The combination is
> already out there, just OPP hasn't solved this problem.
>
> The only other plan I had was to implement another API like
> dev_pm_set_state() or something like that and have that do something
> like what the OPP rate API does right now. The main difference being
> that the argument to the function would be some opaque u64 that's
> converted by the bus/class/genpd attached to the device into whatever
> frequency/voltage/performance state is desired (and sequenced in the
> right order too). And then I was thinking that runtime PM or explicit
> dev_pm_set_state() calls would be used to tell this new layer that the
> device was going to a lower power mode with some other number (sub-kHz
> integer?) and have that be translated again into some
> frequency/voltage/performance state.
>
> Either way, each driver will have to change from using the clk APIs to
> changes rates to something else like one of these APIs, so I don't see a
> huge difference. Drivers will have to change.
>
> >
> > - Do we expect the callers will disable clk before calling
> > opp-set-rate with 0 ? We should remove the regulator requirements as
> > well along with perf-state.
>
> Yes, that's the plan. Problems come along with this though, like shared
> resource constraints and actually knowing the clk on/off state,
> frequency, voltage, etc. at boot time and making sure to keep those
> constraints satisfied during normal operation.
>
> >
> > - What about enabling/disabling clock as well from OPP framework. We
> > can enable it on the very first call to opp-set-rate and disable
> > when freq is 0. That will simplify the drivers as well.
>
> It works when those drivers aren't calling clk_disable() directly from
> some irq handler. I don't think that's very common, but in those cases
> we would probably want to allow drivers to quickly gate and ungate their
> clks but then defer the sleeping stuff (voltages and off chip clks) to
> the schedulable contexts. We'll still be left with a small number of
> drivers that want to only call clk_prepare() and clk_unprepare() from
> within OPP and keep calling clk_enable() and clk_disable() from their
> driver. So introduce different APIs for those drivers to indicate this
> to OPP? And only do that when it becomes a requirement?
>
> Otherwise I don't really see a problem with the OPP call handling the
> enable state of the clk as well.
I think we also need to consider cross SoC drivers. One SoC may have
both clocks and OPPs to manage, while another may have only clocks.
Even it this may be fairly uncommon, we should consider it, before we
decide to fold in additional clock management, like
clk_prepare|unprepare() for example, behind the dev_pm_opp_set_rate()
API.
The point is, the driver may need to call clk_prepare|enable()
anyways, unless we make that conditional depending on a DT compatible
string, for example. Of course, because the clock prepare/enable is
reference counted, there may not be a problem in practice to have both
the OPP and driver to deal with it.
>
> >
> > > One nice feature of this approach is that we don't need to change the
> > > OPP binding to support this. We can specify only the max frequencies and
> > > the voltage requirements in DT with the existing binding and slightly
> > > tweak the OPP code to achieve these results.
> > >
> > > This series includes a conversion of the uart and spi drivers on
> > > qcom sdm845 where these patches are being developed. It shows how a
> > > driver is converted from the clk APIs to the OPP APIs and how
> > > enable/disable state of the clk is communicated to the OPP layer.
> > >
> > > Some open topics and initial points for discussion are:
> > >
> > > 1) The dev_pm_opp_set_rate() API changes may break something that's
> > > relying on the rate rounding that OPP provides. If those exist,
> > > we may need to implement another API that is more explicit about using
> > > the clk API instead of the OPP tables.
> > >
> > > 2) dev_pm_opp_set_rate(0) is an interesting solution to the problem of
> > > dropping the rate requirement. Is there anything better than this?
This seems like a reasonable approach to me.
At least it seams silly to invent a separate API and I can't think of
anything else that makes sense.
> > >
> > > 3) How do we handle devices that already have power-domains specified in
> > > DT? The opp binding for required-opps doesn't let us specify the power
> > > domain to target, instead it assumes that whatever power domain is
> > > attached to a device is the one that OPP needs to use to change the
> > > genpd performance state. Do we need a
> > > dev_pm_opp_set_required_opps_name() or something to be explicit about
> > > this? Can we have some way for the power domain that required-opps
> > > correspond to be expressed in the OPP tables themselves?
Is the about the multiple PM domains per device? I thought we have
already covered this case, or at least part of it [1].
Doesn't dev_pm_opp_set_genpd_virt_dev() and friends, help you with this, no?
> > >
> > > 4) How do we achieve the "full constraint" state? i.e. what do we do
> > > about some devices being active and others being inactive during boot
> > > and making sure that the voltage for the shared power domains doesn't
> > > drop until all devices requiring it have informed OPP about their
> > > power requirements?
Good question. What kind of problems do you see.
Will drivers fail to probe? Will the HW operate outside valid conditions?
> > >
>
> Any comments on these topics?
>
Kind regards
Uffe
[1]
https://lkml.org/lkml/2018/10/25/204
^ permalink raw reply
* [PATCH 1/2] tty: serial: meson_uart: Add support for kernel debugger
From: Julien Masson @ 2019-02-07 11:26 UTC (permalink / raw)
To: Kevin Hilman, Greg Kroah-Hartman
Cc: linux-amlogic, linux-arm-kernel, linux-serial, Jiri Slaby,
Julien Masson, linux-kernel
In-Reply-To: <86h8dfx2jv.fsf@gmail.com>
The kgdb invokes the poll_put_char and poll_get_char when communicating
with the host. This patch implement the serial polling hooks for the
meson_uart to be used for KGDB debugging over serial line.
Signed-off-by: Julien Masson <jmasson@baylibre.com>
---
drivers/tty/serial/meson_uart.c | 46 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 8a84259..49b20da 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -426,6 +426,48 @@ static void meson_uart_config_port(struct uart_port *port, int flags)
}
}
+#ifdef CONFIG_CONSOLE_POLL
+/*
+ * Console polling routines for writing and reading from the uart while
+ * in an interrupt or debug context (i.e. kgdb).
+ */
+
+static int meson_uart_poll_get_char(struct uart_port *port)
+{
+ u32 c;
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ if (readl(port->membase + AML_UART_STATUS) & AML_UART_RX_EMPTY)
+ c = NO_POLL_CHAR;
+ else
+ c = readl(port->membase + AML_UART_RFIFO);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ return c;
+}
+
+static void meson_uart_poll_put_char(struct uart_port *port, unsigned char c)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ while (!(readl(port->membase + AML_UART_STATUS) & AML_UART_TX_EMPTY))
+ cpu_relax();
+
+ writel(c, port->membase + AML_UART_WFIFO);
+
+ while (!(readl(port->membase + AML_UART_STATUS) & AML_UART_TX_EMPTY))
+ cpu_relax();
+
+ spin_unlock_irqrestore(&port->lock, flags);
+}
+
+#endif /* CONFIG_CONSOLE_POLL */
+
static const struct uart_ops meson_uart_ops = {
.set_mctrl = meson_uart_set_mctrl,
.get_mctrl = meson_uart_get_mctrl,
@@ -441,6 +483,10 @@ static const struct uart_ops meson_uart_ops = {
.request_port = meson_uart_request_port,
.release_port = meson_uart_release_port,
.verify_port = meson_uart_verify_port,
+#ifdef CONFIG_CONSOLE_POLL
+ .poll_get_char = meson_uart_poll_get_char,
+ .poll_put_char = meson_uart_poll_put_char,
+#endif
};
#ifdef CONFIG_SERIAL_MESON_CONSOLE
--
2.7.4
^ permalink raw reply related
* Re: [RFC/PATCH 0/5] DVFS in the OPP core
From: Stephen Boyd @ 2019-02-07 7:58 UTC (permalink / raw)
To: Viresh Kumar, grahamr, mturquette
Cc: linux-kernel, linux-arm-msm, linux-pm, linux-serial, linux-spi,
Rajendra Nayak, Ulf Hansson, Doug Anderson, vincent.guittot
In-Reply-To: <20190131092349.fksfqemm23qddkhw@vireshk-i7>
Quoting Viresh Kumar (2019-01-31 01:23:49)
> Adding few folks to the thread who might be interested in this stuff.
>
> On 28-01-19, 17:55, Stephen Boyd wrote:
> > This patch series is an RFC around how we can implement DVFS for devices
> > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > strict set of frequencies that they have been tested at to derive some
> > operating performance point. Instead they have a coarser set of
> > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > device can operate at with a given voltage.
> >
> > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > as a valid frequency indicating the frequency isn't required anymore and
> > to make the same API use the clk framework to round the frequency passed
> > in instead of relying on the OPP table to specify each frequency that
> > can be used. Once we have these two patches in place, we can use the OPP
> > API to change clk rates instead of clk_set_rate() and use all the recent
> > OPP enhancements that have been made around required-opps and genpd
> > performance states to do DVFS for all devices.
>
> Generally speaking I am fine with such an approach but I am not sure
> about what others would say on this as they had objections to using
> OPP core for setting the rate itself.
>
> FWIW, I suggested exactly this solution sometime back [1]
>
> - Drivers need to use two API sets to change clock rate (OPP helpers)
> and enable/disable them (CLK framework helpers) and this leads us to
> exactly that combination. Is that acceptable ? It doesn't look great
> to me as well..
Agreed. I don't think anyone thinks this looks great, but I'll argue
that it's improving OPP for devices that already use it so that we can
remove voltage requirements when their clk is off. Think about CPUs that
are in their own clk domain where we want to remove the voltage
requirement when those CPUs are offline, or a GPU that wants to remove
its voltage requirement when it turns off clks. The combination is
already out there, just OPP hasn't solved this problem.
The only other plan I had was to implement another API like
dev_pm_set_state() or something like that and have that do something
like what the OPP rate API does right now. The main difference being
that the argument to the function would be some opaque u64 that's
converted by the bus/class/genpd attached to the device into whatever
frequency/voltage/performance state is desired (and sequenced in the
right order too). And then I was thinking that runtime PM or explicit
dev_pm_set_state() calls would be used to tell this new layer that the
device was going to a lower power mode with some other number (sub-kHz
integer?) and have that be translated again into some
frequency/voltage/performance state.
Either way, each driver will have to change from using the clk APIs to
changes rates to something else like one of these APIs, so I don't see a
huge difference. Drivers will have to change.
>
> - Do we expect the callers will disable clk before calling
> opp-set-rate with 0 ? We should remove the regulator requirements as
> well along with perf-state.
Yes, that's the plan. Problems come along with this though, like shared
resource constraints and actually knowing the clk on/off state,
frequency, voltage, etc. at boot time and making sure to keep those
constraints satisfied during normal operation.
>
> - What about enabling/disabling clock as well from OPP framework. We
> can enable it on the very first call to opp-set-rate and disable
> when freq is 0. That will simplify the drivers as well.
It works when those drivers aren't calling clk_disable() directly from
some irq handler. I don't think that's very common, but in those cases
we would probably want to allow drivers to quickly gate and ungate their
clks but then defer the sleeping stuff (voltages and off chip clks) to
the schedulable contexts. We'll still be left with a small number of
drivers that want to only call clk_prepare() and clk_unprepare() from
within OPP and keep calling clk_enable() and clk_disable() from their
driver. So introduce different APIs for those drivers to indicate this
to OPP? And only do that when it becomes a requirement?
Otherwise I don't really see a problem with the OPP call handling the
enable state of the clk as well.
>
> > One nice feature of this approach is that we don't need to change the
> > OPP binding to support this. We can specify only the max frequencies and
> > the voltage requirements in DT with the existing binding and slightly
> > tweak the OPP code to achieve these results.
> >
> > This series includes a conversion of the uart and spi drivers on
> > qcom sdm845 where these patches are being developed. It shows how a
> > driver is converted from the clk APIs to the OPP APIs and how
> > enable/disable state of the clk is communicated to the OPP layer.
> >
> > Some open topics and initial points for discussion are:
> >
> > 1) The dev_pm_opp_set_rate() API changes may break something that's
> > relying on the rate rounding that OPP provides. If those exist,
> > we may need to implement another API that is more explicit about using
> > the clk API instead of the OPP tables.
> >
> > 2) dev_pm_opp_set_rate(0) is an interesting solution to the problem of
> > dropping the rate requirement. Is there anything better than this?
> >
> > 3) How do we handle devices that already have power-domains specified in
> > DT? The opp binding for required-opps doesn't let us specify the power
> > domain to target, instead it assumes that whatever power domain is
> > attached to a device is the one that OPP needs to use to change the
> > genpd performance state. Do we need a
> > dev_pm_opp_set_required_opps_name() or something to be explicit about
> > this? Can we have some way for the power domain that required-opps
> > correspond to be expressed in the OPP tables themselves?
> >
> > 4) How do we achieve the "full constraint" state? i.e. what do we do
> > about some devices being active and others being inactive during boot
> > and making sure that the voltage for the shared power domains doesn't
> > drop until all devices requiring it have informed OPP about their
> > power requirements?
> >
Any comments on these topics?
^ permalink raw reply
* Re: [RFC/PATCH 0/5] DVFS in the OPP core
From: Rajendra Nayak @ 2019-02-07 6:57 UTC (permalink / raw)
To: Stephen Boyd, linux-kernel
Cc: linux-arm-msm, linux-pm, linux-serial, linux-spi, Ulf Hansson,
Viresh Kumar, Doug Anderson
In-Reply-To: <20190129015547.213276-1-swboyd@chromium.org>
> 3) How do we handle devices that already have power-domains specified in
> DT? The opp binding for required-opps doesn't let us specify the power
> domain to target, instead it assumes that whatever power domain is
> attached to a device is the one that OPP needs to use to change the
> genpd performance state. Do we need a
> dev_pm_opp_set_required_opps_name() or something to be explicit about
> this? Can we have some way for the power domain that required-opps
> correspond to be expressed in the OPP tables themselves?
I was converting a few more drivers to use the proposed approach in this
RFC, in order to identify all outstanding issues we need to deal with,
and specifically for UFS, I end up with this exact scenario where UFS already
has an existing power domain (gdsc) and I need to add another one (rpmhpd) for
setting the performance state.
If I use dev_pm_opp_of_add_table() to add the opp table from DT, the opp
layer assumes its the same device on which it can do a dev_pm_genpd_set_performance_state()
with, however the device that's actually associated with the pm_domain when we
have multiple power domains is infact the one (dummy) that we create when
the driver makes a call to dev_pm_domain_attach_by_name/id().
Any thoughts on whats a good way to handle this?
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* [PATCH] serial: uartps: Fix stuck ISR if RX disabled with non-empty FIFO
From: Anssi Hannula @ 2019-02-06 14:08 UTC (permalink / raw)
To: Michal Simek, Greg Kroah-Hartman
Cc: stable, linux-arm-kernel, linux-serial, Jiri Slaby
If RX is disabled while there are still unprocessed bytes in RX FIFO,
cdns_uart_handle_rx() called from interrupt handler will get stuck in
the receive loop as read bytes will not get removed from the RX FIFO
and CDNS_UART_SR_RXEMPTY bit will never get set.
Avoid the stuck handler by checking first if RX is disabled. port->lock
protects against race with RX-disabling functions.
This HW behavior was mentioned by Nathan Rossi in 43e98facc4a3 ("tty:
xuartps: Fix RX hang, and TX corruption in termios call") which fixed a
similar issue in cdns_uart_set_termios().
The behavior can also be easily verified by e.g. setting
CDNS_UART_CR_RX_DIS at the beginning of cdns_uart_handle_rx() - the
following loop will then get stuck.
Resetting the FIFO using RXRST would not set RXEMPTY either so simply
issuing a reset after RX-disable would not work.
I observe this frequently on a ZynqMP board during heavy RX load at 1M
baudrate when the reader process exits and thus RX gets disabled.
Fixes: 61ec9016988f ("tty/serial: add support for Xilinx PS UART")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
Cc: stable@vger.kernel.org
---
drivers/tty/serial/xilinx_uartps.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 094f2958cb2b..f0c4f59d9314 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -219,6 +219,13 @@ static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
is_rxbs_support = cdns_uart->quirks & CDNS_UART_RXBS_SUPPORT;
+ /*
+ * RXEMPTY will never be set if RX is disabled as read bytes
+ * will not be removed from the FIFO
+ */
+ if (readl(port->membase + CDNS_UART_CR) & CDNS_UART_CR_RX_DIS)
+ return;
+
while ((readl(port->membase + CDNS_UART_SR) &
CDNS_UART_SR_RXEMPTY) != CDNS_UART_SR_RXEMPTY) {
if (is_rxbs_support)
--
2.17.2
^ permalink raw reply related
* Re: [PATCH v10 1/3] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
From: Vinod Koul @ 2019-02-04 7:21 UTC (permalink / raw)
To: Long Cheng
Cc: Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee, Sean Wang,
Nicolas Boichat, Matthias Brugger, Dan Williams,
Greg Kroah-Hartman, Jiri Slaby, Sean Wang, dmaengine, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, linux-serial,
srv_heupstream, Yingjoe Chen, YT Shen, Zhenbao Liu
In-Reply-To: <1547781016-890-2-git-send-email-long.cheng@mediatek.com>
On 18-01-19, 11:10, Long Cheng wrote:
> +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + enum dma_status ret;
> + unsigned long flags;
> +
> + if (!txstate)
> + return DMA_ERROR;
Why, it is not a mandatory arg!
> + ret = dma_cookie_status(chan, cookie, txstate);
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (ret == DMA_IN_PROGRESS) {
> + c->rx_status = mtk_uart_apdma_read(c, VFF_RPT) & VFF_RING_SIZE;
> + dma_set_residue(txstate, c->rx_status);
> + } else if (ret == DMA_COMPLETE && c->dir == DMA_DEV_TO_MEM) {
> + dma_set_residue(txstate, c->rx_status);
what is the point is setting residue to comleted txn, it is zero!
> + } else {
> + dma_set_residue(txstate, 0);
naah that doesnt sound correct!
> +static void mtk_uart_apdma_config_write(struct dma_chan *chan,
> + struct dma_slave_config *cfg,
> + enum dma_transfer_direction dir)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdmadev *mtkd =
> + to_mtk_uart_apdma_dev(c->vc.chan.device);
> + unsigned int tmp;
> +
> + if (mtk_uart_apdma_read(c, VFF_EN) == VFF_EN_B)
> + return;
> +
> + c->dir = dir;
> +
> + if (dir == DMA_DEV_TO_MEM) {
> + tmp = cfg->src_addr_width * 1024;
why multiply by 1024?
> +static int mtk_uart_apdma_device_pause(struct dma_chan *chan)
> +{
> + /* just for check caps pass */
> + return 0;
> +}
please remove, this is not a mandatory fn
--
~Vinod
^ permalink raw reply
* [PATCH 3.16 165/305] arch/alpha, termios: implement BOTHER, IBSHIFT and termios2
From: Ben Hutchings @ 2019-02-03 13:45 UTC (permalink / raw)
To: linux-kernel, stable
Cc: akpm, Denis Kirjanov, H. Peter Anvin (Intel), Greg Kroah-Hartman,
Ivan Kokshaysky, linux-alpha, Johan Hovold, Matt Turner,
Thomas Gleixner, Eugene Syromiatnikov, linux-serial, Al Viro,
Richard Henderson, Kate Stewart, Philippe Ombredanne, Jiri Slaby,
Alan Cox
In-Reply-To: <lsq.1549201507.384106140@decadent.org.uk>
3.16.63-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: "H. Peter Anvin (Intel)" <hpa@zytor.com>
commit d0ffb805b729322626639336986bc83fc2e60871 upstream.
Alpha has had c_ispeed and c_ospeed, but still set speeds in c_cflags
using arbitrary flags. Because BOTHER is not defined, the general
Linux code doesn't allow setting arbitrary baud rates, and because
CBAUDEX == 0, we can have an array overrun of the baud_rate[] table in
drivers/tty/tty_baudrate.c if (c_cflags & CBAUD) == 037.
Resolve both problems by #defining BOTHER to 037 on Alpha.
However, userspace still needs to know if setting BOTHER is actually
safe given legacy kernels (does anyone actually care about that on
Alpha anymore?), so enable the TCGETS2/TCSETS*2 ioctls on Alpha, even
though they use the same structure. Define struct termios2 just for
compatibility; it is the exact same structure as struct termios. In a
future patchset, this will be cleaned up so the uapi headers are
usable from libc.
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
Cc: Philippe Ombredanne <pombredanne@nexb.com>
Cc: Eugene Syromiatnikov <esyr@redhat.com>
Cc: <linux-alpha@vger.kernel.org>
Cc: <linux-serial@vger.kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
arch/alpha/include/asm/termios.h | 8 +++++++-
arch/alpha/include/uapi/asm/ioctls.h | 5 +++++
arch/alpha/include/uapi/asm/termbits.h | 17 +++++++++++++++++
3 files changed, 29 insertions(+), 1 deletion(-)
--- a/arch/alpha/include/asm/termios.h
+++ b/arch/alpha/include/asm/termios.h
@@ -72,9 +72,15 @@
})
#define user_termios_to_kernel_termios(k, u) \
- copy_from_user(k, u, sizeof(struct termios))
+ copy_from_user(k, u, sizeof(struct termios2))
#define kernel_termios_to_user_termios(u, k) \
+ copy_to_user(u, k, sizeof(struct termios2))
+
+#define user_termios_to_kernel_termios_1(k, u) \
+ copy_from_user(k, u, sizeof(struct termios))
+
+#define kernel_termios_to_user_termios_1(u, k) \
copy_to_user(u, k, sizeof(struct termios))
#endif /* _ALPHA_TERMIOS_H */
--- a/arch/alpha/include/uapi/asm/ioctls.h
+++ b/arch/alpha/include/uapi/asm/ioctls.h
@@ -31,6 +31,11 @@
#define TCXONC _IO('t', 30)
#define TCFLSH _IO('t', 31)
+#define TCGETS2 _IOR('T', 42, struct termios2)
+#define TCSETS2 _IOW('T', 43, struct termios2)
+#define TCSETSW2 _IOW('T', 44, struct termios2)
+#define TCSETSF2 _IOW('T', 45, struct termios2)
+
#define TIOCSWINSZ _IOW('t', 103, struct winsize)
#define TIOCGWINSZ _IOR('t', 104, struct winsize)
#define TIOCSTART _IO('t', 110) /* start output, like ^Q */
--- a/arch/alpha/include/uapi/asm/termbits.h
+++ b/arch/alpha/include/uapi/asm/termbits.h
@@ -25,6 +25,19 @@ struct termios {
speed_t c_ospeed; /* output speed */
};
+/* Alpha has identical termios and termios2 */
+
+struct termios2 {
+ tcflag_t c_iflag; /* input mode flags */
+ tcflag_t c_oflag; /* output mode flags */
+ tcflag_t c_cflag; /* control mode flags */
+ tcflag_t c_lflag; /* local mode flags */
+ cc_t c_cc[NCCS]; /* control characters */
+ cc_t c_line; /* line discipline (== c_cc[19]) */
+ speed_t c_ispeed; /* input speed */
+ speed_t c_ospeed; /* output speed */
+};
+
/* Alpha has matching termios and ktermios */
struct ktermios {
@@ -147,6 +160,7 @@ struct ktermios {
#define B3000000 00034
#define B3500000 00035
#define B4000000 00036
+#define BOTHER 00037
#define CSIZE 00001400
#define CS5 00000000
@@ -164,6 +178,9 @@ struct ktermios {
#define CMSPAR 010000000000 /* mark or space (stick) parity */
#define CRTSCTS 020000000000 /* flow control */
+#define CIBAUD 07600000
+#define IBSHIFT 16
+
/* c_lflag bits */
#define ISIG 0x00000080
#define ICANON 0x00000100
^ permalink raw reply
* Re: [PATCH] tty: serial: meson_uart: Add support for kernel debugger
From: Corentin Labbe @ 2019-02-01 10:10 UTC (permalink / raw)
To: Julien Masson
Cc: Greg Kroah-Hartman, Kevin Hilman, linux-kernel, linux-serial,
Jiri Slaby, linux-amlogic, linux-arm-kernel
In-Reply-To: <1549015162-17418-1-git-send-email-jmasson@baylibre.com>
On Fri, Feb 01, 2019 at 10:59:22AM +0100, Julien Masson wrote:
> The kgdb invokes the poll_put_char and poll_get_char when communicating
> with the host. This patch implement the serial polling hooks for the
> meson_uart to be used for KGDB debugging over serial line.
>
> Signed-off-by: Julien Masson <jmasson@baylibre.com>
> ---
> It has been tested on "Le Potato" board:
> https://libre.computer/products/boards/aml-s905x-cc/
>
> Kernel command line arguments:
> kgdboc=ttyAML0,115200 kgdbretry=4 nokaslr kgdbcon
>
> Kernel modules:
> CONFIG_DEBUG_INFO=y
> CONFIG_DEBUG_KERNEL=y
> CONFIG_FRAME_POINTER=y
> CONFIG_KGDB=y
> CONFIG_KGDB_SERIAL_CONSOLE=y
>
> Warning: for single step instruction I had to apply this patch:
> https://lore.kernel.org/patchwork/patch/562423/
>
> drivers/tty/serial/meson_uart.c | 46 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 8a84259..49b20da 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -426,6 +426,48 @@ static void meson_uart_config_port(struct uart_port *port, int flags)
> }
> }
>
> +#ifdef CONFIG_CONSOLE_POLL
> +/*
> + * Console polling routines for writing and reading from the uart while
> + * in an interrupt or debug context (i.e. kgdb).
> + */
> +
> +static int meson_uart_poll_get_char(struct uart_port *port)
> +{
> + u32 c;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&port->lock, flags);
> +
> + if (readl(port->membase + AML_UART_STATUS) & AML_UART_RX_EMPTY)
> + c = NO_POLL_CHAR;
> + else
> + c = readl(port->membase + AML_UART_RFIFO);
> +
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + return c;
> +}
> +
> +static void meson_uart_poll_put_char(struct uart_port *port, unsigned char c)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&port->lock, flags);
> +
> + while (!(readl(port->membase + AML_UART_STATUS) & AML_UART_TX_EMPTY))
> + cpu_relax();
Hello
Perhaps you could use read_poll_timeout() ?
This will also permit to handle the timeout case.
Regards
^ permalink raw reply
* [PATCH] tty: serial: meson_uart: Add support for kernel debugger
From: Julien Masson @ 2019-02-01 9:59 UTC (permalink / raw)
To: Greg Kroah-Hartman, Kevin Hilman
Cc: Julien Masson, Jiri Slaby, linux-serial, linux-arm-kernel,
linux-amlogic, linux-kernel
The kgdb invokes the poll_put_char and poll_get_char when communicating
with the host. This patch implement the serial polling hooks for the
meson_uart to be used for KGDB debugging over serial line.
Signed-off-by: Julien Masson <jmasson@baylibre.com>
---
It has been tested on "Le Potato" board:
https://libre.computer/products/boards/aml-s905x-cc/
Kernel command line arguments:
kgdboc=ttyAML0,115200 kgdbretry=4 nokaslr kgdbcon
Kernel modules:
CONFIG_DEBUG_INFO=y
CONFIG_DEBUG_KERNEL=y
CONFIG_FRAME_POINTER=y
CONFIG_KGDB=y
CONFIG_KGDB_SERIAL_CONSOLE=y
Warning: for single step instruction I had to apply this patch:
https://lore.kernel.org/patchwork/patch/562423/
drivers/tty/serial/meson_uart.c | 46 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 8a84259..49b20da 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -426,6 +426,48 @@ static void meson_uart_config_port(struct uart_port *port, int flags)
}
}
+#ifdef CONFIG_CONSOLE_POLL
+/*
+ * Console polling routines for writing and reading from the uart while
+ * in an interrupt or debug context (i.e. kgdb).
+ */
+
+static int meson_uart_poll_get_char(struct uart_port *port)
+{
+ u32 c;
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ if (readl(port->membase + AML_UART_STATUS) & AML_UART_RX_EMPTY)
+ c = NO_POLL_CHAR;
+ else
+ c = readl(port->membase + AML_UART_RFIFO);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ return c;
+}
+
+static void meson_uart_poll_put_char(struct uart_port *port, unsigned char c)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ while (!(readl(port->membase + AML_UART_STATUS) & AML_UART_TX_EMPTY))
+ cpu_relax();
+
+ writel(c, port->membase + AML_UART_WFIFO);
+
+ while (!(readl(port->membase + AML_UART_STATUS) & AML_UART_TX_EMPTY))
+ cpu_relax();
+
+ spin_unlock_irqrestore(&port->lock, flags);
+}
+
+#endif /* CONFIG_CONSOLE_POLL */
+
static const struct uart_ops meson_uart_ops = {
.set_mctrl = meson_uart_set_mctrl,
.get_mctrl = meson_uart_get_mctrl,
@@ -441,6 +483,10 @@ static const struct uart_ops meson_uart_ops = {
.request_port = meson_uart_request_port,
.release_port = meson_uart_release_port,
.verify_port = meson_uart_verify_port,
+#ifdef CONFIG_CONSOLE_POLL
+ .poll_get_char = meson_uart_poll_get_char,
+ .poll_put_char = meson_uart_poll_put_char,
+#endif
};
#ifdef CONFIG_SERIAL_MESON_CONSOLE
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] tty: serial: meson_uart: Add support for kernel debugger
From: Julien Masson @ 2019-02-01 7:36 UTC (permalink / raw)
To: Kevin Hilman, Greg Kroah-Hartman
Cc: linux-amlogic, linux-arm-kernel, linux-serial, Jiri Slaby,
linux-kernel
The kgdb invokes the poll_put_char and poll_get_char when communicating
with the host. This patch implement the serial polling hooks for the
meson_uart to be used for KGDB debugging over serial line.
Signed-off-by: Julien Masson <jmasson@baylibre.com>
---
drivers/tty/serial/meson_uart.c | 46 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 8a84259..49b20da 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -426,6 +426,48 @@ static void meson_uart_config_port(struct uart_port *port, int flags)
}
}
+#ifdef CONFIG_CONSOLE_POLL
+/*
+ * Console polling routines for writing and reading from the uart while
+ * in an interrupt or debug context (i.e. kgdb).
+ */
+
+static int meson_uart_poll_get_char(struct uart_port *port)
+{
+ u32 c;
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ if (readl(port->membase + AML_UART_STATUS) & AML_UART_RX_EMPTY)
+ c = NO_POLL_CHAR;
+ else
+ c = readl(port->membase + AML_UART_RFIFO);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ return c;
+}
+
+static void meson_uart_poll_put_char(struct uart_port *port, unsigned char c)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ while (!(readl(port->membase + AML_UART_STATUS) & AML_UART_TX_EMPTY))
+ cpu_relax();
+
+ writel(c, port->membase + AML_UART_WFIFO);
+
+ while (!(readl(port->membase + AML_UART_STATUS) & AML_UART_TX_EMPTY))
+ cpu_relax();
+
+ spin_unlock_irqrestore(&port->lock, flags);
+}
+
+#endif /* CONFIG_CONSOLE_POLL */
+
static const struct uart_ops meson_uart_ops = {
.set_mctrl = meson_uart_set_mctrl,
.get_mctrl = meson_uart_get_mctrl,
@@ -441,6 +483,10 @@ static const struct uart_ops meson_uart_ops = {
.request_port = meson_uart_request_port,
.release_port = meson_uart_release_port,
.verify_port = meson_uart_verify_port,
+#ifdef CONFIG_CONSOLE_POLL
+ .poll_get_char = meson_uart_poll_get_char,
+ .poll_put_char = meson_uart_poll_put_char,
+#endif
};
#ifdef CONFIG_SERIAL_MESON_CONSOLE
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v6 6/6] arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and Makefile
From: Erin Lo @ 2019-02-01 5:11 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, Ben Ho, Mars Cheng, Mengqi Zhang, linux-clk,
Hsin-Hsiung Wang, Weiyi Lu, Marc Zyngier,
open list:SERIAL DRIVERS, Yingjoe Chen, devicetree, Jason Cooper,
Seiya Wang, moderated list:ARM/Mediatek SoC support,
Matthias Brugger, Thomas Gleixner, Eddie Huang, ARM/FREESCALE
In-Reply-To: <1548992018.11367.8.camel@mtksdaap41>
Add back more people since mail server issue
On Fri, 2019-02-01 at 11:33 +0800, Erin Lo wrote:
> On Thu, 2019-01-31 at 15:10 -0600, Rob Herring wrote:
> > On Wed, Jan 30, 2019 at 8:34 PM Erin Lo <erin.lo@mediatek.com> wrote:
> > >
> > > On Wed, 2019-01-30 at 10:22 -0600, Rob Herring wrote:
> > > > On Thu, Jan 24, 2019 at 04:07:20PM +0800, Erin Lo wrote:
> > > > > From: Ben Ho <Ben.Ho@mediatek.com>
> > > > >
> > > > > Add basic chip support for Mediatek 8183, include
> > > > > pinctrl file, uart node with correct uart clocks, pwrap device
> > > > >
> > > > > Add clock controller nodes, include topckgen, infracfg,
> > > > > apmixedsys and subsystem.
> > > > >
> > > > > Signed-off-by: Ben Ho <Ben.Ho@mediatek.com>
> > > > > Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> > > > > Signed-off-by: Seiya Wang <seiya.wang@mediatek.com>
> > > > > Signed-off-by: Zhiyong Tao <zhiyong.tao@mediatek.com>
> > > > > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > > > > Signed-off-by: Mengqi Zhang <Mengqi.Zhang@mediatek.com>
> > > > > Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
> > > > > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > > > > ---
> > > >
> > > >
> > > > > + sysirq: intpol-controller@c530a80 {
> > > >
> > > > interrupt-controller@...
> > >
> > > I will modify it in next version.
> > > >
> > > >
> > > > Place all the MMIO peripherals under one or more simple-bus nodes.
> > > >
> > > > Rob
> > > >
> > >
> > > Do you mean need to add simple-bus like this?
> >
> > Yes.
>
> We remove soc because Matthias suggested it in former MTK SoC maybe in
> 2015 year.
>
> We will add it back by your comment.
>
> Thank you.
>
> Best Regards,
> Erin
Hi, Matthias,
Do you have any comment here?
Thanks
Best Regards,
Erin
> >
> > >
> > > + soc: soc {
> > > + #address-cells = <0x1>;
> > > + #size-cells = <0x1>;
> > > + ranges = <0 0 0 0xffffffff>;
> > > + compatible = "simple-bus";
> > >
> > > soc_data: soc_data@08000000 {
> > > compatible = "mediatek,mt8183-efuse",
> > > "mediatek,efuse";
> > > reg = <0 0x08000000 0 0x0010>;
> > > #address-cells = <1>;
> > > #size-cells = <1>;
> > > status = "disabled";
> > > };
> > >
> > > gic: interrupt-controller@0c000000 {
> > > compatible = "arm,gic-v3";
> > > #interrupt-cells = <4>;
> > >
> > > Best Regards,
> > > Erin
> > >
> > > > _______________________________________________
> > > > Linux-mediatek mailing list
> > > > Linux-mediatek@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> > >
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply
* Re: [PATCH 0/3] serdev support for n_gsm
From: Pavel Machek @ 2019-01-31 23:34 UTC (permalink / raw)
To: Johan Hovold
Cc: Tony Lindgren, Greg Kroah-Hartman, linux-kernel, Alan Cox,
Jiri Slaby, Peter Hurley, Rob Herring, Sebastian Reichel,
linux-serial, Marcel Holtmann, Kishon Vijay Abraham I, Mark Brown
In-Reply-To: <20190124163932.GZ3691@localhost>
[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]
Hi!
> > And without the n_gsm serdev support, it's a mess of some app
> > similar to [5] initializing n_gsm, trying to deal with the USB
> > PHY PM, dealing with Motorola custom packet numbering, kicking
> > GNSS device, feeding data to /dev/ugnss. Hmm I think I've already
> > been there just to be able to type AT commands to the modem and
> > it did not work :)
>
> It's a mess indeed, but I'd rather see user-space dealing with until we
> figure out how best to do it in the kernel. ;)
>
> > Anyways, for the serdev kernel drivers, the criteria I've tried
> > to follow is: "Can this serdev device driver make user space
> > apps use standard Linux interfaces for the hardware?"
> >
> > So for the serdev Alsa ASoC driver, user space can use the standard
> > Alsa interface for setting voice call volume. And for the serdev
> > GNSS driver, user space can use /dev/gnss0.
>
> I understand. Both drivers appears to be using AT commands for control.
> It would be interesting to hear what Mark has to say about the codec
> driver too. Moving AT handling into the kernel scares me a bit. If we
> already have a telephony stack to deal with it in user-space, my
> inclination is to let it continue to handle it.
The userspace part of the telephony stack uses ALSA mixers to
do... well... audio mixer configuration for phone calls, and it would
be good if Droid 4 could keep the same interface.
> Modem-managed GNSS is also different from receivers connected directly
> to the host. It's really the modem that drives the GNSS receiver, and
> offers a higher-level interface to the host, for example, by buffering
> output which the host can later request. It may or may not be the
> kernel's job to periodically poll the modem to recreate an NMEA feed so
> to speak.
>
> But the end-result of having it accessible through a standard interface
> is of course appealing.
Yes please. On N900, there's special (socket-based) interface to the
GPS... and it si painful.
Plus, it will take some time before modem support on Droid 4
stabilizes... and it would be very welcome to be able to use the GPS
in the meantime.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
From: shuah @ 2019-01-31 23:23 UTC (permalink / raw)
To: Johan Hovold, Marcel Holtmann
Cc: Al Viro, open list:NFC SUBSYSTEM, chris, devel, Rob Herring,
Samuel Ortiz, open list:SERIAL DRIVERS, Jiri Slaby, santhameena13,
kirk, Johan Hedberg, Arnd Bergmann, samuel.thibault,
m.maya.nakamura, zhongjiang, Greg KH, speakup,
Linux Kernel Mailing List, Bluez mailing list, netdev,
nishka.dasgupta_ug18, "David S. Miller" <dave>
In-Reply-To: <20190131153306.GT3691@localhost>
On 1/31/19 8:33 AM, Johan Hovold wrote:
> On Thu, Jan 31, 2019 at 04:18:33PM +0100, Marcel Holtmann wrote:
>
>>> I agree with Al that this change doesn't make much sense. The WARN_ON
>>> is there to catch any bugs leading to the termios being changed for a
>>> master side pty. Those should bugs should be fixed, and not worked
>>> around in order to silence a WARN_ON.
>>>
>>> The problem started with 7721383f4199 ("Bluetooth: hci_uart: Support
>>> operational speed during setup") which introduced a new way for how
>>> tty_set_termios() could end up being called for a master pty.
>>>
>>> As Al hinted at, setting these ldiscs for a master pty really makes no
>>> sense and perhaps that is what we should prevent unless simply making
>>> sure they do not call tty_set_termios() is sufficient for the time
>>> being.
>>>
>>> Finally, note that serdev never operates on a pty, and that this is only
>>> an issue for (the three) line disciplines.
>>
>> I think for PTYs we should just fail setting the HCI line discipline.
>> Fail early and just move on with life.
>
> Sounds good to me. At least for the pty master. There may be some people
> trying to use a bluetooth device connected to a remote serial port (I've
> seen descriptions of such setups at least), and maybe we need not prevent
> that.
>
Thanks for the feedback on the patch. Changes to prevent setting the HCI
line discipline from hci_uart fixes the problem.
I am sending v2 in a just a bit.
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH v6 6/6] arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and Makefile
From: Rob Herring @ 2019-01-31 21:10 UTC (permalink / raw)
To: Erin Lo
Cc: Mark Rutland, Ben Ho, Mars Cheng, Mengqi Zhang, linux-clk,
Hsin-Hsiung Wang, Weiyi Lu, Marc Zyngier,
open list:SERIAL DRIVERS, Yingjoe Chen, devicetree, Jason Cooper,
Seiya Wang, moderated list:ARM/Mediatek SoC support,
Matthias Brugger, Thomas Gleixner, Eddie Huang, ARM/FREESCALE
In-Reply-To: <1548902050.23230.4.camel@mtksdaap41>
On Wed, Jan 30, 2019 at 8:34 PM Erin Lo <erin.lo@mediatek.com> wrote:
>
> On Wed, 2019-01-30 at 10:22 -0600, Rob Herring wrote:
> > On Thu, Jan 24, 2019 at 04:07:20PM +0800, Erin Lo wrote:
> > > From: Ben Ho <Ben.Ho@mediatek.com>
> > >
> > > Add basic chip support for Mediatek 8183, include
> > > pinctrl file, uart node with correct uart clocks, pwrap device
> > >
> > > Add clock controller nodes, include topckgen, infracfg,
> > > apmixedsys and subsystem.
> > >
> > > Signed-off-by: Ben Ho <Ben.Ho@mediatek.com>
> > > Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> > > Signed-off-by: Seiya Wang <seiya.wang@mediatek.com>
> > > Signed-off-by: Zhiyong Tao <zhiyong.tao@mediatek.com>
> > > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > > Signed-off-by: Mengqi Zhang <Mengqi.Zhang@mediatek.com>
> > > Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
> > > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > > ---
> >
> >
> > > + sysirq: intpol-controller@c530a80 {
> >
> > interrupt-controller@...
>
> I will modify it in next version.
> >
> >
> > Place all the MMIO peripherals under one or more simple-bus nodes.
> >
> > Rob
> >
>
> Do you mean need to add simple-bus like this?
Yes.
>
> + soc: soc {
> + #address-cells = <0x1>;
> + #size-cells = <0x1>;
> + ranges = <0 0 0 0xffffffff>;
> + compatible = "simple-bus";
>
> soc_data: soc_data@08000000 {
> compatible = "mediatek,mt8183-efuse",
> "mediatek,efuse";
> reg = <0 0x08000000 0 0x0010>;
> #address-cells = <1>;
> #size-cells = <1>;
> status = "disabled";
> };
>
> gic: interrupt-controller@0c000000 {
> compatible = "arm,gic-v3";
> #interrupt-cells = <4>;
>
> Best Regards,
> Erin
>
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: 答复: [PATCH][V5] tty: fix race between flush_to_ldisc and tty_open
From: Greg Kroah-Hartman @ 2019-01-31 18:44 UTC (permalink / raw)
To: Li,Rongqing
Cc: linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
jslaby@suse.com, gkohli@codeaurora.org
In-Reply-To: <22a5d8639b154119af7d7661e17025f2@baidu.com>
On Thu, Jan 31, 2019 at 11:15:48AM +0000, Li,Rongqing wrote:
>
>
> > -----邮件原件-----
> > 发件人: linux-kernel-owner@vger.kernel.org
> > [mailto:linux-kernel-owner@vger.kernel.org] 代表 Greg Kroah-Hartman
> > 发送时间: 2019年1月31日 18:55
> > 收件人: Li,Rongqing <lirongqing@baidu.com>
> > 抄送: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org;
> > jslaby@suse.com; gkohli@codeaurora.org
> > 主题: Re: [PATCH][V5] tty: fix race between flush_to_ldisc and tty_open
> >
> > On Thu, Jan 31, 2019 at 05:43:16PM +0800, Li RongQing wrote:
> > > There still is a race window after the commit b027e2298bd588
> > > ("tty: fix data race between tty_init_dev and flush of buf"), and we
> > > encountered this crash issue if receive_buf call comes before tty
> > > initialization completes in tty_open and
> > > tty->driver_data may be NULL.
> > >
> > > CPU0 CPU1
> > > ---- ----
> > > tty_open
> > > tty_init_dev
> > > tty_ldisc_unlock
> > > schedule flush_to_ldisc
> > > receive_buf
> > > tty_port_default_receive_buf
> > > tty_ldisc_receive_buf
> > > n_tty_receive_buf_common
> > > __receive_buf
> > > uart_flush_chars
> > > uart_start
> > > /*tty->driver_data is NULL*/
> > > tty->ops->open
> > > /*init tty->driver_data*/
> > >
> > > it can be fixed by extending ldisc semaphore lock in tty_init_dev to
> > > driver_data initialized completely after tty->ops->open(), but this
> > > will lead to get lock on one function and unlock in some other
> > > function, and hard to maintain, so fix this race only by checking
> > > tty->driver_data when receiving, and return if tty->driver_data
> > > is NULL, and n_tty_receive_buf_common maybe calls uart_unthrottle, so
> > > add the same check
> > >
> > > Signed-off-by: Wang Li <wangli39@baidu.com>
> > > Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> > > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > > ---
> > > V5: move check into uart_start from n_tty_receive_buf_common
> > > V4: add version information
> > > V3: not used ldisc semaphore lock, only checking tty->driver_data with
> > > NULL
> > > V2: fix building error by EXPORT_SYMBOL tty_ldisc_unlock
> > > V1: extend ldisc lock to protect that tty->driver_data is inited
> > >
> > > drivers/tty/serial/serial_core.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/serial_core.c
> > > b/drivers/tty/serial/serial_core.c
> > > index 5c01bb6d1c24..556f50aa1b58 100644
> > > --- a/drivers/tty/serial/serial_core.c
> > > +++ b/drivers/tty/serial/serial_core.c
> > > @@ -130,6 +130,9 @@ static void uart_start(struct tty_struct *tty)
> > > struct uart_port *port;
> > > unsigned long flags;
> > >
> > > + if (!state)
> > > + return;
> > > +
> > > port = uart_port_lock(state, flags);
> > > __uart_start(tty);
> > > uart_port_unlock(port, flags);
> > > @@ -727,6 +730,9 @@ static void uart_unthrottle(struct tty_struct *tty)
> > > upstat_t mask = UPSTAT_SYNC_FIFO;
> > > struct uart_port *port;
> > >
> > > + if (!state)
> > > + return;
> > > +
> > > port = uart_port_ref(state);
> > > if (!port)
> > > return;
> > > --
> > > 2.16.2
> >
> >
> > Hm, I wrote this patch, not you, right? So shouldn't I get the
> > credit/blame for it? :)
> >
>
> Welcome you to add your credit/blame/signature
> and I am not clear the rule, and be afraid to become fake
No problem, I've fixed this up when committing this, and added some
wording change to the changelog text.
Thanks so much for working through all of this, it's a bug that has
always been there for forever it seems, nice catch!
greg k-h
^ permalink raw reply
* Re: [PATCH] serial: mps2-uart: Add parentheses around conditional in mps2_uart_shutdown
From: Nick Desaulniers @ 2019-01-31 18:21 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Greg Kroah-Hartman, Vladimir Murzin, Jiri Slaby, Liviu Dudau,
Sudeep Holla, Lorenzo Pieralisi, linux-serial, Linux ARM, LKML
In-Reply-To: <20190131180627.19944-1-natechancellor@gmail.com>
On Thu, Jan 31, 2019 at 10:06 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns:
>
> drivers/tty/serial/mps2-uart.c:351:6: warning: logical not is only
> applied to the left hand side of this bitwise operator
> [-Wlogical-not-parentheses]
> if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
> ^ ~
> drivers/tty/serial/mps2-uart.c:351:6: note: add parentheses after the
> '!' to evaluate the bitwise operator first
> if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
> ^
> ( )
> drivers/tty/serial/mps2-uart.c:351:6: note: add parentheses around left
> hand side expression to silence this warning
> if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
> ^
> ( )
> 1 warning generated.
>
> As it was intended for this check to be the inverse of the one at the
> bottom of mps2_init_port, add parentheses around the whole conditional.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/tty/serial/mps2-uart.c
That function currently has the code:
if (mps_port->flags & UART_PORT_COMBINED_IRQ) {
mps_port->port.irq = platform_get_irq(pdev, 0);
} else {
mps_port->rx_irq = platform_get_irq(pdev, 0);
mps_port->tx_irq = platform_get_irq(pdev, 1);
mps_port->port.irq = platform_get_irq(pdev, 2);
}
while shutdown has:
if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
free_irq(mps_port->rx_irq, mps_port);
free_irq(mps_port->tx_irq, mps_port);
}
free_irq(port->irq, mps_port);
So port->irq is always assigned an irq, and rx_irq & tx_irq are
assigned only if !(mps_port->flags & UART_PORT_COMBINED_IRQ).
Yep, patch looks good. Thanks for fixing a real bug here, Nathan.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/344
>
> Fixes: 775ea4ea2fd9 ("serial: mps2-uart: support combined irq")
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> drivers/tty/serial/mps2-uart.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/mps2-uart.c b/drivers/tty/serial/mps2-uart.c
> index 4404eb7d118f..587b42f754cb 100644
> --- a/drivers/tty/serial/mps2-uart.c
> +++ b/drivers/tty/serial/mps2-uart.c
> @@ -348,7 +348,7 @@ static void mps2_uart_shutdown(struct uart_port *port)
>
> mps2_uart_write8(port, control, UARTn_CTRL);
>
> - if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
> + if (!(mps_port->flags & UART_PORT_COMBINED_IRQ)) {
> free_irq(mps_port->rx_irq, mps_port);
> free_irq(mps_port->tx_irq, mps_port);
> }
> --
> 2.20.1
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: [PATCH] serial: mps2-uart: Add parentheses around conditional in mps2_uart_shutdown
From: Vladimir Murzin @ 2019-01-31 18:11 UTC (permalink / raw)
To: Nathan Chancellor, Greg Kroah-Hartman
Cc: Jiri Slaby, Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi,
linux-serial, linux-arm-kernel, linux-kernel, Nick Desaulniers
In-Reply-To: <20190131180627.19944-1-natechancellor@gmail.com>
On 1/31/19 6:06 PM, Nathan Chancellor wrote:
> Clang warns:
>
> drivers/tty/serial/mps2-uart.c:351:6: warning: logical not is only
> applied to the left hand side of this bitwise operator
> [-Wlogical-not-parentheses]
> if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
> ^ ~
> drivers/tty/serial/mps2-uart.c:351:6: note: add parentheses after the
> '!' to evaluate the bitwise operator first
> if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
> ^
> ( )
> drivers/tty/serial/mps2-uart.c:351:6: note: add parentheses around left
> hand side expression to silence this warning
> if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
> ^
> ( )
> 1 warning generated.
>
> As it was intended for this check to be the inverse of the one at the
> bottom of mps2_init_port, add parentheses around the whole conditional.
>
> Fixes: 775ea4ea2fd9 ("serial: mps2-uart: support combined irq")
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> drivers/tty/serial/mps2-uart.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/mps2-uart.c b/drivers/tty/serial/mps2-uart.c
> index 4404eb7d118f..587b42f754cb 100644
> --- a/drivers/tty/serial/mps2-uart.c
> +++ b/drivers/tty/serial/mps2-uart.c
> @@ -348,7 +348,7 @@ static void mps2_uart_shutdown(struct uart_port *port)
>
> mps2_uart_write8(port, control, UARTn_CTRL);
>
> - if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
> + if (!(mps_port->flags & UART_PORT_COMBINED_IRQ)) {
> free_irq(mps_port->rx_irq, mps_port);
> free_irq(mps_port->tx_irq, mps_port);
> }
>
Acked-by: Vladimir Murzin <vladimir.murzin@arm.com>
^ permalink raw reply
* [PATCH] serial: mps2-uart: Add parentheses around conditional in mps2_uart_shutdown
From: Nathan Chancellor @ 2019-01-31 18:06 UTC (permalink / raw)
To: Greg Kroah-Hartman, Vladimir Murzin
Cc: Lorenzo Pieralisi, Nick Desaulniers, Jiri Slaby, Liviu Dudau,
linux-kernel, linux-serial, Sudeep Holla, Nathan Chancellor,
linux-arm-kernel
Clang warns:
drivers/tty/serial/mps2-uart.c:351:6: warning: logical not is only
applied to the left hand side of this bitwise operator
[-Wlogical-not-parentheses]
if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
^ ~
drivers/tty/serial/mps2-uart.c:351:6: note: add parentheses after the
'!' to evaluate the bitwise operator first
if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
^
( )
drivers/tty/serial/mps2-uart.c:351:6: note: add parentheses around left
hand side expression to silence this warning
if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
^
( )
1 warning generated.
As it was intended for this check to be the inverse of the one at the
bottom of mps2_init_port, add parentheses around the whole conditional.
Fixes: 775ea4ea2fd9 ("serial: mps2-uart: support combined irq")
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/tty/serial/mps2-uart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/mps2-uart.c b/drivers/tty/serial/mps2-uart.c
index 4404eb7d118f..587b42f754cb 100644
--- a/drivers/tty/serial/mps2-uart.c
+++ b/drivers/tty/serial/mps2-uart.c
@@ -348,7 +348,7 @@ static void mps2_uart_shutdown(struct uart_port *port)
mps2_uart_write8(port, control, UARTn_CTRL);
- if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
+ if (!(mps_port->flags & UART_PORT_COMBINED_IRQ)) {
free_irq(mps_port->rx_irq, mps_port);
free_irq(mps_port->tx_irq, mps_port);
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
From: Johan Hovold @ 2019-01-31 15:33 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Johan Hovold, shuah, Al Viro, open list:NFC SUBSYSTEM, chris,
devel, Rob Herring, Samuel Ortiz, open list:SERIAL DRIVERS,
Jiri Slaby, santhameena13, kirk, Johan Hedberg, Arnd Bergmann,
samuel.thibault, m.maya.nakamura, zhongjiang, Greg KH, speakup,
Linux Kernel Mailing List, Bluez mailing list, netdev
In-Reply-To: <D0EE5C33-2930-4286-998C-822DD7898B76@holtmann.org>
On Thu, Jan 31, 2019 at 04:18:33PM +0100, Marcel Holtmann wrote:
> > I agree with Al that this change doesn't make much sense. The WARN_ON
> > is there to catch any bugs leading to the termios being changed for a
> > master side pty. Those should bugs should be fixed, and not worked
> > around in order to silence a WARN_ON.
> >
> > The problem started with 7721383f4199 ("Bluetooth: hci_uart: Support
> > operational speed during setup") which introduced a new way for how
> > tty_set_termios() could end up being called for a master pty.
> >
> > As Al hinted at, setting these ldiscs for a master pty really makes no
> > sense and perhaps that is what we should prevent unless simply making
> > sure they do not call tty_set_termios() is sufficient for the time
> > being.
> >
> > Finally, note that serdev never operates on a pty, and that this is only
> > an issue for (the three) line disciplines.
>
> I think for PTYs we should just fail setting the HCI line discipline.
> Fail early and just move on with life.
Sounds good to me. At least for the pty master. There may be some people
trying to use a bluetooth device connected to a remote serial port (I've
seen descriptions of such setups at least), and maybe we need not prevent
that.
Johan
^ permalink raw reply
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
From: Marcel Holtmann @ 2019-01-31 15:18 UTC (permalink / raw)
To: Johan Hovold
Cc: shuah, Al Viro, open list:NFC SUBSYSTEM, chris, devel,
Rob Herring, Samuel Ortiz, open list:SERIAL DRIVERS, Jiri Slaby,
santhameena13, kirk, Johan Hedberg, Arnd Bergmann,
samuel.thibault, m.maya.nakamura, zhongjiang, Greg KH, speakup,
Linux Kernel Mailing List, Bluez mailing list, netdev,
nishka.dasgupta_ug18
In-Reply-To: <20190130103227.GR3691@localhost>
Hi Johan,
>>> On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote:
>>>> tty_set_termios() has the following WARMN_ON which can be triggered with a
>>>> syscall to invoke TIOCGETD __NR_ioctl.
>
> You meant TIOCSETD here, and in fact its the call which sets the uart
> protocol that triggers the warning.
>
>>>> WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
>>>> tty->driver->subtype == PTY_TYPE_MASTER);
>>>> Reference: https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d
>>>>
>>>> A simple change would have been to print error message instead of WARN_ON.
>>>> However, the callers assume that tty_set_termios() always returns 0 and
>>>> don't check return value. The complete solution is fixing all the callers
>>>> to check error and bail out to fix the WARN_ON.
>>>>
>>>> This fix changes tty_set_termios() to return error and all the callers
>>>> to check error and bail out. The reproducer is used to reproduce the
>>>> problem and verify the fix.
>>>
>>>> --- a/drivers/bluetooth/hci_ldisc.c
>>>> +++ b/drivers/bluetooth/hci_ldisc.c
>>>> @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
>>>> status = tty_set_termios(tty, &ktermios);
>>>> BT_DBG("Disabling hardware flow control: %s",
>>>> status ? "failed" : "success");
>>>> + if (status)
>>>> + return;
>>>
>>> Can that ldisc end up set on pty master? And does it make any sense there?
>>
>> The initial objective of the patch is to prevent the WARN_ON by making
>> the change to return error instead of WARN_ON. However, without changes
>> to places that don't check the return and keep making progress, there
>> will be secondary problems.
>>
>> Without this change to return here, instead of WARN_ON, it will fail
>> with the following NULL pointer dereference at the next thing
>> hci_uart_set_flow_control() attempts.
>>
>> status = tty->driver->ops->tiocmget(tty);
>>
>> kernel: [10140.649783] BUG: unable to handle kernel NULL pointer
>
> That's a separate issue, which is being fixed:
>
> https://lkml.kernel.org/r/20190130095938.GP3691@localhost
>
>>> IOW, I don't believe that this patch makes any sense. If anything,
>>> we need to prevent unconditional tty_set_termios() on the path
>>> that *does* lead to calling it for pty.
>>
>> I don't think preventing unconditional tty_set_termios() is enough to
>> prevent secondary problems such as the one above.
>>
>> For example, the following call chain leads to the WARN_ON that was
>> reported. Even if void hci_uart_set_baudrate() prevents the very first
>> tty_set_termios() call, its caller hci_uart_setup() continues with
>> more tty setup. It goes ahead to call driver setup callback. The
>> driver callback goes on to do more setup calling tty_set_termios().
>>
>> WARN_ON call path:
>> hci_uart_set_baudrate+0x1cc/0x250 drivers/bluetooth/hci_ldisc.c:378
>> hci_uart_setup+0xa2/0x490 drivers/bluetooth/hci_ldisc.c:401
>> hci_dev_do_open+0x6b1/0x1920 net/bluetooth/hci_core.c:1423
>>
>> Once this WARN_ON is changed to return error, the following
>> happens, when hci_uart_setup() does driver setup callback.
>>
>> kernel: [10140.649836] mrvl_setup+0x17/0x80 [hci_uart]
>> kernel: [10140.649840] hci_uart_setup+0x56/0x160 [hci_uart]
>> kernel: [10140.649850] hci_dev_do_open+0xe6/0x630 [bluetooth]
>> kernel: [10140.649860] hci_power_on+0x52/0x220 [bluetooth]
>>
>> I think continuing to catch the invalid condition in tty_set_termios()
>> and preventing progress by checking return value is a straight forward
>> change to avoid secondary problems, and it might be difficult to catch
>> all the cases where it could fail.
>
> I agree with Al that this change doesn't make much sense. The WARN_ON
> is there to catch any bugs leading to the termios being changed for a
> master side pty. Those should bugs should be fixed, and not worked
> around in order to silence a WARN_ON.
>
> The problem started with 7721383f4199 ("Bluetooth: hci_uart: Support
> operational speed during setup") which introduced a new way for how
> tty_set_termios() could end up being called for a master pty.
>
> As Al hinted at, setting these ldiscs for a master pty really makes no
> sense and perhaps that is what we should prevent unless simply making
> sure they do not call tty_set_termios() is sufficient for the time
> being.
>
> Finally, note that serdev never operates on a pty, and that this is only
> an issue for (the three) line disciplines.
I think for PTYs we should just fail setting the HCI line discipline. Fail early and just move on with life.
Regards
Marcel
^ permalink raw reply
* 答复: [PATCH][V5] tty: fix race between flush_to_ldisc and tty_open
From: Li,Rongqing @ 2019-01-31 11:15 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
jslaby@suse.com, gkohli@codeaurora.org
In-Reply-To: <20190131105527.GB8271@kroah.com>
> -----邮件原件-----
> 发件人: linux-kernel-owner@vger.kernel.org
> [mailto:linux-kernel-owner@vger.kernel.org] 代表 Greg Kroah-Hartman
> 发送时间: 2019年1月31日 18:55
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org;
> jslaby@suse.com; gkohli@codeaurora.org
> 主题: Re: [PATCH][V5] tty: fix race between flush_to_ldisc and tty_open
>
> On Thu, Jan 31, 2019 at 05:43:16PM +0800, Li RongQing wrote:
> > There still is a race window after the commit b027e2298bd588
> > ("tty: fix data race between tty_init_dev and flush of buf"), and we
> > encountered this crash issue if receive_buf call comes before tty
> > initialization completes in tty_open and
> > tty->driver_data may be NULL.
> >
> > CPU0 CPU1
> > ---- ----
> > tty_open
> > tty_init_dev
> > tty_ldisc_unlock
> > schedule flush_to_ldisc
> > receive_buf
> > tty_port_default_receive_buf
> > tty_ldisc_receive_buf
> > n_tty_receive_buf_common
> > __receive_buf
> > uart_flush_chars
> > uart_start
> > /*tty->driver_data is NULL*/
> > tty->ops->open
> > /*init tty->driver_data*/
> >
> > it can be fixed by extending ldisc semaphore lock in tty_init_dev to
> > driver_data initialized completely after tty->ops->open(), but this
> > will lead to get lock on one function and unlock in some other
> > function, and hard to maintain, so fix this race only by checking
> > tty->driver_data when receiving, and return if tty->driver_data
> > is NULL, and n_tty_receive_buf_common maybe calls uart_unthrottle, so
> > add the same check
> >
> > Signed-off-by: Wang Li <wangli39@baidu.com>
> > Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> > V5: move check into uart_start from n_tty_receive_buf_common
> > V4: add version information
> > V3: not used ldisc semaphore lock, only checking tty->driver_data with
> > NULL
> > V2: fix building error by EXPORT_SYMBOL tty_ldisc_unlock
> > V1: extend ldisc lock to protect that tty->driver_data is inited
> >
> > drivers/tty/serial/serial_core.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/tty/serial/serial_core.c
> > b/drivers/tty/serial/serial_core.c
> > index 5c01bb6d1c24..556f50aa1b58 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -130,6 +130,9 @@ static void uart_start(struct tty_struct *tty)
> > struct uart_port *port;
> > unsigned long flags;
> >
> > + if (!state)
> > + return;
> > +
> > port = uart_port_lock(state, flags);
> > __uart_start(tty);
> > uart_port_unlock(port, flags);
> > @@ -727,6 +730,9 @@ static void uart_unthrottle(struct tty_struct *tty)
> > upstat_t mask = UPSTAT_SYNC_FIFO;
> > struct uart_port *port;
> >
> > + if (!state)
> > + return;
> > +
> > port = uart_port_ref(state);
> > if (!port)
> > return;
> > --
> > 2.16.2
>
>
> Hm, I wrote this patch, not you, right? So shouldn't I get the
> credit/blame for it? :)
>
Welcome you to add your credit/blame/signature
and I am not clear the rule, and be afraid to become fake
> Also, this is a bug in the serial code, not necessarily the tty layer,
> so the subject should change...
>
> And you did test this, right?
I add some delay in tty_init_dev to simulate this issue. it can fix this my issue.
Thanks
-RongQing
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH][V5] tty: fix race between flush_to_ldisc and tty_open
From: Greg Kroah-Hartman @ 2019-01-31 10:55 UTC (permalink / raw)
To: Li RongQing; +Cc: linux-serial, linux-kernel, jslaby, gkohli
In-Reply-To: <1548927796-11348-1-git-send-email-lirongqing@baidu.com>
On Thu, Jan 31, 2019 at 05:43:16PM +0800, Li RongQing wrote:
> There still is a race window after the commit b027e2298bd588
> ("tty: fix data race between tty_init_dev and flush of buf"),
> and we encountered this crash issue if receive_buf call comes
> before tty initialization completes in tty_open and
> tty->driver_data may be NULL.
>
> CPU0 CPU1
> ---- ----
> tty_open
> tty_init_dev
> tty_ldisc_unlock
> schedule
> flush_to_ldisc
> receive_buf
> tty_port_default_receive_buf
> tty_ldisc_receive_buf
> n_tty_receive_buf_common
> __receive_buf
> uart_flush_chars
> uart_start
> /*tty->driver_data is NULL*/
> tty->ops->open
> /*init tty->driver_data*/
>
> it can be fixed by extending ldisc semaphore lock in tty_init_dev
> to driver_data initialized completely after tty->ops->open(), but
> this will lead to get lock on one function and unlock in some other
> function, and hard to maintain, so fix this race only by checking
> tty->driver_data when receiving, and return if tty->driver_data
> is NULL, and n_tty_receive_buf_common maybe calls uart_unthrottle,
> so add the same check
>
> Signed-off-by: Wang Li <wangli39@baidu.com>
> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> V5: move check into uart_start from n_tty_receive_buf_common
> V4: add version information
> V3: not used ldisc semaphore lock, only checking tty->driver_data with NULL
> V2: fix building error by EXPORT_SYMBOL tty_ldisc_unlock
> V1: extend ldisc lock to protect that tty->driver_data is inited
>
> drivers/tty/serial/serial_core.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 5c01bb6d1c24..556f50aa1b58 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -130,6 +130,9 @@ static void uart_start(struct tty_struct *tty)
> struct uart_port *port;
> unsigned long flags;
>
> + if (!state)
> + return;
> +
> port = uart_port_lock(state, flags);
> __uart_start(tty);
> uart_port_unlock(port, flags);
> @@ -727,6 +730,9 @@ static void uart_unthrottle(struct tty_struct *tty)
> upstat_t mask = UPSTAT_SYNC_FIFO;
> struct uart_port *port;
>
> + if (!state)
> + return;
> +
> port = uart_port_ref(state);
> if (!port)
> return;
> --
> 2.16.2
Hm, I wrote this patch, not you, right? So shouldn't I get the
credit/blame for it? :)
Also, this is a bug in the serial code, not necessarily the tty layer,
so the subject should change...
And you did test this, right?
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC/PATCH 0/5] DVFS in the OPP core
From: Viresh Kumar @ 2019-01-31 10:41 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Stephen Boyd, Michael Turquette, grahamr,
Linux Kernel Mailing List, linux-arm-msm, Linux PM, linux-serial,
linux-spi, Rajendra Nayak, Ulf Hansson, Doug Anderson,
Vincent Guittot
In-Reply-To: <CAJZ5v0jA0U1Z=AeRrEGXH2j+AenzNaiD+tV-yKwKanKwiNVW7g@mail.gmail.com>
On 31-01-19, 11:36, Rafael J. Wysocki wrote:
> On Thu, Jan 31, 2019 at 11:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 31-01-19, 10:58, Rafael J. Wysocki wrote:
> > > On Thu, Jan 31, 2019 at 10:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >
> > > > Adding few folks to the thread who might be interested in this stuff.
> > > >
> > > > On 28-01-19, 17:55, Stephen Boyd wrote:
> > > > > This patch series is an RFC around how we can implement DVFS for devices
> > > > > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > > > > strict set of frequencies that they have been tested at to derive some
> > > > > operating performance point. Instead they have a coarser set of
> > > > > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > > > > device can operate at with a given voltage.
> > > > >
> > > > > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > > > > as a valid frequency indicating the frequency isn't required anymore and
> > > > > to make the same API use the clk framework to round the frequency passed
> > > > > in instead of relying on the OPP table to specify each frequency that
> > > > > can be used. Once we have these two patches in place, we can use the OPP
> > > > > API to change clk rates instead of clk_set_rate() and use all the recent
> > > > > OPP enhancements that have been made around required-opps and genpd
> > > > > performance states to do DVFS for all devices.
> > > >
> > > > Generally speaking I am fine with such an approach but I am not sure
> > > > about what others would say on this as they had objections to using
> > > > OPP core for setting the rate itself.
> > > >
> > > > FWIW, I suggested exactly this solution sometime back [1]
> > > >
> > > > - Drivers need to use two API sets to change clock rate (OPP helpers)
> > > > and enable/disable them (CLK framework helpers) and this leads us to
> > > > exactly that combination. Is that acceptable ? It doesn't look great
> > > > to me as well..
> > >
> > > I agree here.
> > >
> > > > - Do we expect the callers will disable clk before calling
> > > > opp-set-rate with 0 ? We should remove the regulator requirements as
> > > > well along with perf-state.
> > >
> > > Well, disabling clk affects HW in general, doesn't it?
> >
> > Yeah, but the regulator may be shared and is running at higher
> > voltages just because of the clock requirement of the device getting
> > disabled here. Or did I misunderstood what you wanted to say ?
>
> What I wanted to say is that if the caller is required to disable clk
> beforehand, that may be as good as setting its rate to zero already.
Right, but that doesn't mean that the requirements put on the
regulator and genpd are gone as well and that is why I favor the OPP
core to handle all the parts otherwise write the rules explicitly. If
the OPP core doesn't disable the clock and the caller also hasn't done
the same, then disabling the genpd/regulator may break things up.
--
viresh
^ permalink raw reply
* Re: [RFC/PATCH 0/5] DVFS in the OPP core
From: Rafael J. Wysocki @ 2019-01-31 10:36 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Stephen Boyd, Michael Turquette, grahamr,
Linux Kernel Mailing List, linux-arm-msm, Linux PM, linux-serial,
linux-spi, Rajendra Nayak, Ulf Hansson, Doug Anderson,
Vincent Guittot
In-Reply-To: <20190131100628.6ex6ic3kspuxxxt7@vireshk-i7>
On Thu, Jan 31, 2019 at 11:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 31-01-19, 10:58, Rafael J. Wysocki wrote:
> > On Thu, Jan 31, 2019 at 10:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > Adding few folks to the thread who might be interested in this stuff.
> > >
> > > On 28-01-19, 17:55, Stephen Boyd wrote:
> > > > This patch series is an RFC around how we can implement DVFS for devices
> > > > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > > > strict set of frequencies that they have been tested at to derive some
> > > > operating performance point. Instead they have a coarser set of
> > > > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > > > device can operate at with a given voltage.
> > > >
> > > > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > > > as a valid frequency indicating the frequency isn't required anymore and
> > > > to make the same API use the clk framework to round the frequency passed
> > > > in instead of relying on the OPP table to specify each frequency that
> > > > can be used. Once we have these two patches in place, we can use the OPP
> > > > API to change clk rates instead of clk_set_rate() and use all the recent
> > > > OPP enhancements that have been made around required-opps and genpd
> > > > performance states to do DVFS for all devices.
> > >
> > > Generally speaking I am fine with such an approach but I am not sure
> > > about what others would say on this as they had objections to using
> > > OPP core for setting the rate itself.
> > >
> > > FWIW, I suggested exactly this solution sometime back [1]
> > >
> > > - Drivers need to use two API sets to change clock rate (OPP helpers)
> > > and enable/disable them (CLK framework helpers) and this leads us to
> > > exactly that combination. Is that acceptable ? It doesn't look great
> > > to me as well..
> >
> > I agree here.
> >
> > > - Do we expect the callers will disable clk before calling
> > > opp-set-rate with 0 ? We should remove the regulator requirements as
> > > well along with perf-state.
> >
> > Well, disabling clk affects HW in general, doesn't it?
>
> Yeah, but the regulator may be shared and is running at higher
> voltages just because of the clock requirement of the device getting
> disabled here. Or did I misunderstood what you wanted to say ?
What I wanted to say is that if the caller is required to disable clk
beforehand, that may be as good as setting its rate to zero already.
> > > - What about enabling/disabling clock as well from OPP framework. We
> > > can enable it on the very first call to opp-set-rate and disable
> > > when freq is 0. That will simplify the drivers as well.
> >
> > That sounds compelling, but I guess there are cases in which you can
> > gate the clock regardless of the frequency setting. How would that
> > work then?
>
> Can you give any example here ? I am not sure I understood the concern
> here.
It looks like I was confused somehow, never mind. :-)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox