* [PATCH v3 05/10] serial: stm32: Use __maybe_unused instead of #if CONFIG_PM_SLEEP
From: Erwan Le Ray @ 2019-06-13 13:49 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Maxime Coquelin, Alexandre Torgue,
Rob Herring, Mark Rutland
Cc: devicetree, linux-kernel, Erwan Le Ray, linux-serial,
Fabrice Gasnier, linux-stm32, linux-arm-kernel
In-Reply-To: <1560433800-12255-1-git-send-email-erwan.leray@st.com>
Use __maybe_unused for power management related functionsinstead of
fixes: 270e5a74fe4c ("serial: stm32: add wakeup mechanism")
Signed-off-by: Erwan Le Ray <erwan.leray@st.com>
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 41898c4..b0fb420 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -1276,8 +1276,8 @@ static int stm32_console_setup(struct console *co, char *options)
.cons = STM32_SERIAL_CONSOLE,
};
-#ifdef CONFIG_PM_SLEEP
-static void stm32_serial_enable_wakeup(struct uart_port *port, bool enable)
+static void __maybe_unused stm32_serial_enable_wakeup(struct uart_port *port,
+ bool enable)
{
struct stm32_port *stm32_port = to_stm32_port(port);
struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
@@ -1301,7 +1301,7 @@ static void stm32_serial_enable_wakeup(struct uart_port *port, bool enable)
}
}
-static int stm32_serial_suspend(struct device *dev)
+static int __maybe_unused stm32_serial_suspend(struct device *dev)
{
struct uart_port *port = dev_get_drvdata(dev);
@@ -1317,7 +1317,7 @@ static int stm32_serial_suspend(struct device *dev)
return 0;
}
-static int stm32_serial_resume(struct device *dev)
+static int __maybe_unused stm32_serial_resume(struct device *dev)
{
struct uart_port *port = dev_get_drvdata(dev);
@@ -1328,7 +1328,6 @@ static int stm32_serial_resume(struct device *dev)
return uart_resume_port(&stm32_usart_driver, port);
}
-#endif /* CONFIG_PM_SLEEP */
static int __maybe_unused stm32_serial_runtime_suspend(struct device *dev)
{
--
1.9.1
^ permalink raw reply related
* [PATCH v3 04/10] serial: stm32: add pm_runtime support
From: Erwan Le Ray @ 2019-06-13 13:49 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Maxime Coquelin, Alexandre Torgue,
Rob Herring, Mark Rutland
Cc: devicetree, linux-kernel, Erwan Le Ray, linux-serial, Bich Hemon,
Fabrice Gasnier, linux-stm32, linux-arm-kernel
In-Reply-To: <1560433800-12255-1-git-send-email-erwan.leray@st.com>
Use pm_runtime for clock management.
Signed-off-by: Bich Hemon <bich.hemon@st.com>
Signed-off-by: Erwan Le Ray <erwan.leray@st.com>
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index a8f20ba..41898c4 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -810,13 +810,13 @@ static void stm32_pm(struct uart_port *port, unsigned int state,
switch (state) {
case UART_PM_STATE_ON:
- clk_prepare_enable(stm32port->clk);
+ pm_runtime_get_sync(port->dev);
break;
case UART_PM_STATE_OFF:
spin_lock_irqsave(&port->lock, flags);
stm32_clr_bits(port, ofs->cr1, BIT(cfg->uart_enable_bit));
spin_unlock_irqrestore(&port->lock, flags);
- clk_disable_unprepare(stm32port->clk);
+ pm_runtime_put_sync(port->dev);
break;
}
}
@@ -1111,6 +1111,11 @@ static int stm32_serial_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, &stm32port->port);
+ pm_runtime_get_noresume(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_put_sync(&pdev->dev);
+
return 0;
err_wirq:
@@ -1132,6 +1137,9 @@ static int stm32_serial_remove(struct platform_device *pdev)
struct uart_port *port = platform_get_drvdata(pdev);
struct stm32_port *stm32_port = to_stm32_port(port);
struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
+ int err;
+
+ pm_runtime_get_sync(&pdev->dev);
stm32_clr_bits(port, ofs->cr3, USART_CR3_DMAR);
@@ -1160,7 +1168,12 @@ static int stm32_serial_remove(struct platform_device *pdev)
clk_disable_unprepare(stm32_port->clk);
- return uart_remove_one_port(&stm32_usart_driver, port);
+ err = uart_remove_one_port(&stm32_usart_driver, port);
+
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_put_noidle(&pdev->dev);
+
+ return err;
}
@@ -1317,7 +1330,29 @@ static int stm32_serial_resume(struct device *dev)
}
#endif /* CONFIG_PM_SLEEP */
+static int __maybe_unused stm32_serial_runtime_suspend(struct device *dev)
+{
+ struct uart_port *port = dev_get_drvdata(dev);
+ struct stm32_port *stm32port = container_of(port,
+ struct stm32_port, port);
+
+ clk_disable_unprepare(stm32port->clk);
+
+ return 0;
+}
+
+static int __maybe_unused stm32_serial_runtime_resume(struct device *dev)
+{
+ struct uart_port *port = dev_get_drvdata(dev);
+ struct stm32_port *stm32port = container_of(port,
+ struct stm32_port, port);
+
+ return clk_prepare_enable(stm32port->clk);
+}
+
static const struct dev_pm_ops stm32_serial_pm_ops = {
+ SET_RUNTIME_PM_OPS(stm32_serial_runtime_suspend,
+ stm32_serial_runtime_resume, NULL)
SET_SYSTEM_SLEEP_PM_OPS(stm32_serial_suspend, stm32_serial_resume)
};
--
1.9.1
^ permalink raw reply related
* [PATCH v3 03/10] serial: stm32: select pinctrl state in each suspend/resume function
From: Erwan Le Ray @ 2019-06-13 13:49 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Maxime Coquelin, Alexandre Torgue,
Rob Herring, Mark Rutland
Cc: devicetree, linux-kernel, Erwan Le Ray, linux-serial, Bich Hemon,
Fabrice Gasnier, linux-stm32, linux-arm-kernel
In-Reply-To: <1560433800-12255-1-git-send-email-erwan.leray@st.com>
Select either pinctrl sleep state in suspend function or default state in
resume function.
Signed-off-by: Bich Hemon <bich.hemon@st.com>
Signed-off-by: Erwan Le Ray <erwan.leray@st.com>
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 9c2b04e..a8f20ba 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -24,6 +24,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_platform.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/pm_wakeirq.h>
@@ -1298,6 +1299,8 @@ static int stm32_serial_suspend(struct device *dev)
else
stm32_serial_enable_wakeup(port, false);
+ pinctrl_pm_select_sleep_state(dev);
+
return 0;
}
@@ -1305,6 +1308,8 @@ static int stm32_serial_resume(struct device *dev)
{
struct uart_port *port = dev_get_drvdata(dev);
+ pinctrl_pm_select_default_state(dev);
+
if (device_may_wakeup(dev))
stm32_serial_enable_wakeup(port, false);
--
1.9.1
^ permalink raw reply related
* [PATCH v3 02/10] dt-bindings: serial: add optional pinctrl states
From: Erwan Le Ray @ 2019-06-13 13:49 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Maxime Coquelin, Alexandre Torgue,
Rob Herring, Mark Rutland
Cc: devicetree, linux-kernel, Erwan Le Ray, linux-serial, Bich Hemon,
Fabrice Gasnier, linux-stm32, linux-arm-kernel
In-Reply-To: <1560433800-12255-1-git-send-email-erwan.leray@st.com>
From: Bich Hemon <bich.hemon@st.com>
Add options for pinctrl states:
- "sleep" for low power
- "idle" for low power and wakeup capabilities enabled
- "no_console_suspend" for enabling console messages in low power
Signed-off-by: Bich Hemon <bich.hemon@st.com>
Signed-off-by: Erwan Le Ray <erwan.leray@st.com>
diff --git a/Documentation/devicetree/bindings/serial/st,stm32-usart.txt b/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
index 5ec80c1..64a5ea9 100644
--- a/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
+++ b/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
@@ -13,7 +13,14 @@ Required properties:
- clocks: The input clock of the USART instance
Optional properties:
-- pinctrl: The reference on the pins configuration
+- pinctrl-names: Set to "default". An additional "sleep" state can be defined
+ to set pins in sleep state when in low power. In case the device is used as
+ a wakeup source, "idle" state is defined in order to keep RX pin active.
+ For a console device, an optional state "no_console_suspend" can be defined
+ to enable console messages during suspend. Typically, "no_console_suspend" and
+ "default" states can refer to the same pin configuration.
+- pinctrl-n: Phandle(s) pointing to pin configuration nodes.
+ For Pinctrl properties see ../pinctrl/pinctrl-bindings.txt
- st,hw-flow-ctrl: bool flag to enable hardware flow control.
- rs485-rts-delay, rs485-rx-during-tx, rs485-rts-active-low,
linux,rs485-enabled-at-boot-time: see rs485.txt.
@@ -31,8 +38,11 @@ usart4: serial@40004c00 {
reg = <0x40004c00 0x400>;
interrupts = <52>;
clocks = <&clk_pclk1>;
- pinctrl-names = "default";
+ pinctrl-names = "default", "sleep", "idle", "no_console_suspend";
pinctrl-0 = <&pinctrl_usart4>;
+ pinctrl-1 = <&pinctrl_usart4_sleep>;
+ pinctrl-2 = <&pinctrl_usart4_idle>;
+ pinctrl-3 = <&pinctrl_usart4>;
};
usart2: serial@40004400 {
--
1.9.1
^ permalink raw reply related
* [PATCH v3 01/10] dt-bindings: serial: stm32: add wakeup option
From: Erwan Le Ray @ 2019-06-13 13:49 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Maxime Coquelin, Alexandre Torgue,
Rob Herring, Mark Rutland
Cc: devicetree, linux-kernel, Erwan Le Ray, linux-serial, Bich Hemon,
Fabrice Gasnier, linux-stm32, linux-arm-kernel
In-Reply-To: <1560433800-12255-1-git-send-email-erwan.leray@st.com>
Add a note for enabling wakeup capabilities of usart
Signed-off-by: Bich Hemon <bich.hemon@st.com>
Signed-off-by: Erwan Le Ray <erwan.leray@st.com>
diff --git a/Documentation/devicetree/bindings/serial/st,stm32-usart.txt b/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
index 9d3efed..5ec80c1 100644
--- a/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
+++ b/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
@@ -19,6 +19,11 @@ Optional properties:
linux,rs485-enabled-at-boot-time: see rs485.txt.
- dmas: phandle(s) to DMA controller node(s). Refer to stm32-dma.txt
- dma-names: "rx" and/or "tx"
+- wakeup-source: bool flag to indicate this device has wakeup capabilities
+- interrupt-names, if optional wake-up interrupt is used, should be:
+ - "event": the name for the interrupt line of the USART instance
+ - "wakeup" the name for the optional wake-up interrupt
+
Examples:
usart4: serial@40004c00 {
--
1.9.1
^ permalink raw reply related
* [PATCH v3 00/10] STM32 usart power improvements
From: Erwan Le Ray @ 2019-06-13 13:49 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Maxime Coquelin, Alexandre Torgue,
Rob Herring, Mark Rutland
Cc: devicetree, linux-kernel, Erwan Le Ray, linux-serial,
Fabrice Gasnier, linux-stm32, linux-arm-kernel
This series delivers power improvements for stm32-usart driver.
Changes in v3:
Move pinctrl/consumer.h include from "add support for no_console_suspend"
patch to "select pinctrl state" patch in order to solve a compilation
issue.
Bich Hemon (3):
dt-bindings: serial: add optional pinctrl states
ARM: dts: stm32: Update pin states for uart4 on stm32mp157c-ed1
ARM: dts: stm32: Update UART4 pin states on stm32mp157a-dk1
Erwan Le Ray (7):
dt-bindings: serial: stm32: add wakeup option
serial: stm32: select pinctrl state in each suspend/resume function
serial: stm32: add pm_runtime support
serial: stm32: Use __maybe_unused instead of #if CONFIG_PM_SLEEP
serial: stm32: add support for no_console_suspend
ARM: dts: stm32: update uart4 pin configurations for low power
ARM: dts: stm32: add wakeup capability on each usart/uart on
stm32mp157c
.../devicetree/bindings/serial/st,stm32-usart.txt | 19 ++++-
arch/arm/boot/dts/stm32mp157-pinctrl.dtsi | 17 +++++
arch/arm/boot/dts/stm32mp157a-dk1.dts | 5 +-
arch/arm/boot/dts/stm32mp157c-ed1.dts | 5 +-
arch/arm/boot/dts/stm32mp157c.dtsi | 40 ++++++++--
drivers/tty/serial/stm32-usart.c | 88 ++++++++++++++++++++--
drivers/tty/serial/stm32-usart.h | 1 +
7 files changed, 155 insertions(+), 20 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH 2/2 v5] tty/serial/8250: use mctrl_gpio helpers
From: Andy Shevchenko @ 2019-06-13 11:05 UTC (permalink / raw)
To: Stefan Roese
Cc: linux-serial, linux-kernel, Yegor Yefremov, Mika Westerberg,
Giulio Benetti, Greg Kroah-Hartman
In-Reply-To: <6a4f1001-b023-c972-7b36-6d2f8f9a3fa8@denx.de>
On Thu, Jun 13, 2019 at 07:32:39AM +0200, Stefan Roese wrote:
> On 12.06.19 11:16, Andy Shevchenko wrote:
> > On Wed, Jun 12, 2019 at 10:13:05AM +0200, Stefan Roese wrote:
> > > On 11.06.19 16:48, Andy Shevchenko wrote:
> > > Do you something like this in mind?
> >
> > More likely
> >
> > static inline int serial8250_MCR_to_TIOCM(int mcr)
>
> MSR_to_TIOCM (see below) ...
Yes. true.
> > {
> > int tiocm = 0;
> >
> > if (mcr & ...)
> > tiocm |= ...;
> > ...
> >
> > return tiocm;
> > }
> >
> > static inline int serial8250_TIOCM_to_MCR(int tiocm)
> > {
> > ... in a similar way ...
> > }
>
> While implementing such wrapper functions I noticed, that get_mctrl() /
> set_mctrl() need TIOCM->MCR and MSR->TIOCM (notice MSR vs MCR here) but
> serial8250_in_MCR() needs MCR->TIOCM. So there is not that much
> overlay here.
It seems not only this driver is using such conversion. It's even possible to
move it to serial level for all.
> Additionally the wrappers would need to handle all bits
> and only some of them are needed in serial8250_in/out_MCR(),
> so I would
> need to add masking here as well.
I don't see this. You will get a value for exclusive bits only. No additional
mask would be needed.
> For my taste its not really worth adding these wrappers as they won't
> make things much clearer (if at all).
Hmm.. For me it would be quite clear if something with proposed name would be
called in the code.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC v2 01/11] OPP: Don't overwrite rounded clk rate
From: Viresh Kumar @ 2019-06-13 9:54 UTC (permalink / raw)
To: swboyd, Rajendra Nayak, vincent.guittot, mturquette
Cc: linux-kernel, linux-arm-msm, linux-pm, linux-serial, linux-spi,
dri-devel, linux-scsi, ulf.hansson, dianders, rafael
In-Reply-To: <20190612082506.m735bsk7bjijf2yg@vireshk-i7>
On 12-06-19, 13:55, Viresh Kumar wrote:
> Okay, I have applied this patch (alone) to the OPP tree with minor
> modifications in commit log and diff.
And I have removed it now :)
I am confused as hell on what we should be doing and what we are doing
right now. And if we should do better.
Let me explain with an example.
- The clock provider supports following frequencies: 500, 600, 700,
800, 900, 1000 MHz.
- The OPP table contains/supports only a subset: 500, 700, 1000 MHz.
Now, the request to change the frequency starts from cpufreq
governors, like schedutil when they calls:
__cpufreq_driver_target(policy, 599 MHz, CPUFREQ_RELATION_L);
CPUFREQ_RELATION_L means: lowest frequency at or above target. And so
I would expect the frequency to get set to 600MHz (if we look at clock
driver) or 700MHz (if we look at OPP table). I think we should decide
this thing from the OPP table only as that's what the platform guys
want us to use. So, we should end up with 700 MHz.
Then we land into dev_pm_opp_set_rate(), which does this (which is
code copied from earlier version of cpufreq-dt driver):
- clk_round_rate(clk, 599 MHz).
clk_round_rate() returns the highest frequency lower than target. So
it must return 500 MHz (I haven't tested this yet, all theoretical).
- _find_freq_ceil(opp_table, 500 MHz).
This works like CPUFREQ_RELATION_L, so we find lowest frequency >=
target freq. And so we should get: 500 MHz itself as OPP table has
it.
- clk_set_rate(clk, 500 MHz).
This must be doing round-rate again, but I think we will settle with
500 MHz eventually.
Now the questionnaire:
- Is this whole exercise correct ?
- We shouldn't have landed on 500 MHz, right ?
- Is there anything wrong with the above theory (I am going to test it soon though).
- Why do we need to do the first clock_round_rate() ? Should we remove
it ?
Now lets move to this patch, which makes it more confusing.
The OPP tables for CPUs and GPUs should already be somewhat like fmax
tables for particular voltage values and that's why both cpufreq and
OPP core try to find a frequency higher than target so we choose the
most optimum one power-efficiency wise.
For cases where the OPP table is only a subset of the clk-providers
table (almost always), if we let the clock provider to find the
nearest frequency (which is lower) we will run the CPU/GPU at a
not-so-optimal frequency. i.e. if 500, 600, 700 MHz all need voltage
to be 1.2 V, we should be running at 700 always, while we may end up
running at 500 MHz.
This kind of behavior (introduced by this patch) is important for
other devices which want to run at the nearest frequency to target
one, but not for CPUs/GPUs. So, we need to tag these IO devices
separately, maybe from DT ? So we select the closest match instead of
most optimal one.
But lets fix the existing issues first and then think about this
patch.
--
viresh
^ permalink raw reply
* Re: [PATCH 2/2 v5] tty/serial/8250: use mctrl_gpio helpers
From: Stefan Roese @ 2019-06-13 5:32 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-serial, linux-kernel, Yegor Yefremov, Mika Westerberg,
Giulio Benetti, Greg Kroah-Hartman
In-Reply-To: <20190612091621.GA9224@smile.fi.intel.com>
On 12.06.19 11:16, Andy Shevchenko wrote:
> On Wed, Jun 12, 2019 at 10:13:05AM +0200, Stefan Roese wrote:
>> On 11.06.19 16:48, Andy Shevchenko wrote:
>>> On Tue, Jun 11, 2019 at 04:02:54PM +0200, Stefan Roese wrote:
>>>> On 11.06.19 14:44, Andy Shevchenko wrote:
>>>>> On Tue, Jun 11, 2019 at 12:56:03PM +0200, Stefan Roese wrote:
>>>
>>>>>> static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
>>>>>> {
>>>>>> serial_out(up, UART_MCR, value);
>>>>>> +
>>>>>> + if (up->gpios) {
>>>>>> + int mctrl_gpio = 0;
>>>>>> +
>>>>>> + if (value & UART_MCR_RTS)
>>>>>> + mctrl_gpio |= TIOCM_RTS;
>>>>>> + if (value & UART_MCR_DTR)
>>>>>> + mctrl_gpio |= TIOCM_DTR;
>>>>>> +
>>>>>> + mctrl_gpio_set(up->gpios, mctrl_gpio);
>>>>>> + }
>>>>>> }
>>>
>>>>>> static inline int serial8250_in_MCR(struct uart_8250_port *up)
>>>>>> {
>>>>>> - return serial_in(up, UART_MCR);
>>>>>> + int mctrl;
>>>>>> +
>>>>>> + mctrl = serial_in(up, UART_MCR);
>>>>>> +
>>>>>> + if (up->gpios) {
>>>>>> + int mctrl_gpio = 0;
>>>>>> +
>>>>>> + /* save current MCR values */
>>>>>> + if (mctrl & UART_MCR_RTS)
>>>>>> + mctrl_gpio |= TIOCM_RTS;
>>>>>> + if (mctrl & UART_MCR_DTR)
>>>>>> + mctrl_gpio |= TIOCM_DTR;
>>>>>> +
>>>>>> + mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, &mctrl_gpio);
>>>>>> + if (mctrl_gpio & TIOCM_RTS)
>>>>>> + mctrl |= UART_MCR_RTS;
>>>>>> + else
>>>>>> + mctrl &= ~UART_MCR_RTS;
>>>>>> +
>>>>>> + if (mctrl_gpio & TIOCM_DTR)
>>>>>> + mctrl |= UART_MCR_DTR;
>>>>>> + else
>>>>>> + mctrl &= ~UART_MCR_DTR;
>>>>>> + }
>>>>>> +
>>>>>> + return mctrl;
>>>>>> }
>>>>>
>>>>> These are using OR logic with potentially volatile data. Shouldn't we mask
>>>>> unused bits in UART_MCR in case of up->gpios != NULL?
>>>>
>>>> Sorry, I don't see, which bits you are referring to? Could you please be
>>>> a bit more specific with the variable / macro meant (example)?
>>>
>>> I meant that we double write values in the out() which might have some
>>> consequences, though I hope nothing wrong with it happens.
>>
>> Where is the double write to a register? Sorry, I fail to spot it.
>
> Not to the one register. From the functional point of view the same signal is
> set up twice: once per UART register, once per GPIO pins.
>
>>> In the in() we read the all bits in the register.
>>>
>>> As now I look at the implementation of mctrl_gpio_get_outputs(),
>>> I think we rather get helpers for conversion between TIOCM and UART_MCR values,
>>> so, they can be used in get_mctrl() / set_mctrl() and above.
>>
>> Do you something like this in mind?
>
> More likely
>
> static inline int serial8250_MCR_to_TIOCM(int mcr)
MSR_to_TIOCM (see below) ...
> {
> int tiocm = 0;
>
> if (mcr & ...)
> tiocm |= ...;
> ...
>
> return tiocm;
> }
>
> static inline int serial8250_TIOCM_to_MCR(int tiocm)
> {
> ... in a similar way ...
> }
While implementing such wrapper functions I noticed, that get_mctrl() /
set_mctrl() need TIOCM->MCR and MSR->TIOCM (notice MSR vs MCR here) but
serial8250_in_MCR() needs MCR->TIOCM. So there is not that much
overlay here. Additionally the wrappers would need to handle all bits
and only some of them are needed in serial8250_in/out_MCR(), so I would
need to add masking here as well.
For my taste its not really worth adding these wrappers as they won't
make things much clearer (if at all).
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v2 5/6] serial: uartps: Do not add a trailing semicolon to macro
From: Joe Perches @ 2019-06-12 17:07 UTC (permalink / raw)
To: Michal Simek, johan, gregkh, linux-kernel, monstr
Cc: Nava kishore Manne, Jiri Slaby, linux-serial, linux-arm-kernel
In-Reply-To: <5d938d34c3c4710577df898dbf4b70c74d2e6730.1560338079.git.michal.simek@xilinx.com>
On Wed, 2019-06-12 at 13:14 +0200, Michal Simek wrote:
> From: Nava kishore Manne <nava.manne@xilinx.com>
>
> This patch fixes this checkpatch warning:
> WARNING: macros should not use a trailing semicolon
> +#define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
> + clk_rate_change_nb);
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
[]
> @@ -199,7 +199,7 @@ struct cdns_platform_data {
> u32 quirks;
> };
> #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
> - clk_rate_change_nb);
> + clk_rate_change_nb)
>
> /**
> * cdns_uart_handle_rx - Handle the received bytes along with Rx errors.
trivia:
Perhaps this is easier for humans to read with the macro
on two lines like:
#define to_cdns_uart(_nb) \
container_of(_nb, struct cdns_uart, clk_rate_change_nb)
or just ignore the 80 column limit
#define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, clk_rate_change_nb)
or because the macro is only used in one place,
just get rid of it and use container_of directly.
---
drivers/tty/serial/xilinx_uartps.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 605354fd60b1..ca5cec2b83ce 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -195,11 +195,10 @@ struct cdns_uart {
u32 quirks;
bool cts_override;
};
+
struct cdns_platform_data {
u32 quirks;
};
-#define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
- clk_rate_change_nb);
/**
* cdns_uart_handle_rx - Handle the received bytes along with Rx errors.
@@ -489,8 +488,9 @@ static int cdns_uart_clk_notifier_cb(struct notifier_block *nb,
int locked = 0;
struct clk_notifier_data *ndata = data;
unsigned long flags = 0;
- struct cdns_uart *cdns_uart = to_cdns_uart(nb);
+ struct cdns_uart *cdns_uart;
+ cdns_uart = container_of(nb, struct cdns_uart, clk_rate_change_nb);
port = cdns_uart->port;
if (port->suspended)
return NOTIFY_OK;
^ permalink raw reply related
* Re: [PATCH v2 3/6] serial: uartps: Fix multiple line dereference
From: Joe Perches @ 2019-06-12 16:56 UTC (permalink / raw)
To: Michal Simek, johan, gregkh, linux-kernel, monstr
Cc: Nava kishore Manne, Jiri Slaby, linux-serial, linux-arm-kernel
In-Reply-To: <3a5b27987c5b4fc5ec7dc7f58485db63057edbfe.1560338079.git.michal.simek@xilinx.com>
On Wed, 2019-06-12 at 13:14 +0200, Michal Simek wrote:
> From: Nava kishore Manne <nava.manne@xilinx.com>
>
> Trivial patch which fixes this checkpatch warning:
> WARNING: Avoid multiple line dereference - prefer 'port->state->xmit.tail'
> + port->state->xmit.buf[port->state->xmit.
> + tail], port->membase + CDNS_UART_FIFO);
>
> Fixes: c8dbdc842d30 ("serial: xuartps: Rewrite the interrupt handling logic")
> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> Changes in v2:
> - Split patch from v1
> - Add Fixes tag
>
> drivers/tty/serial/xilinx_uartps.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index c84db82bdaab..4cd20c036750 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -319,8 +319,8 @@ static void cdns_uart_handle_tx(void *dev_id)
> * register.
> */
> writel(
> - port->state->xmit.buf[port->state->xmit.
> - tail], port->membase + CDNS_UART_FIFO);
> + port->state->xmit.buf[port->state->xmit.tail],
> + port->membase + CDNS_UART_FIFO);
>
> port->icount.tx++;
Another way to rewrite this is to use a temporary for
port->state->xmit and also return early on empty to
avoid unnecessary indentation.
Using a temporary can also reduce object size a bit by
removing unnecessary dereferences: (defconfig x86-64)
$ size drivers/tty/serial/xilinx_uartps.o*
text data bss dec hex filename
26578 4632 320 31530 7b2a drivers/tty/serial/xilinx_uartps.o.new
26642 4632 320 31594 7b6a drivers/tty/serial/xilinx_uartps.o.old
i.e.:
---
drivers/tty/serial/xilinx_uartps.c | 54 ++++++++++++++++++--------------------
1 file changed, 25 insertions(+), 29 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 605354fd60b1..09b586aeeca3 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -305,40 +305,36 @@ static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
static void cdns_uart_handle_tx(void *dev_id)
{
struct uart_port *port = (struct uart_port *)dev_id;
+ struct circ_buf *xmit = &port->state->xmit;
unsigned int numbytes;
- if (uart_circ_empty(&port->state->xmit)) {
+ if (uart_circ_empty(xmit)) {
writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IDR);
- } else {
- numbytes = port->fifosize;
- while (numbytes && !uart_circ_empty(&port->state->xmit) &&
- !(readl(port->membase + CDNS_UART_SR) & CDNS_UART_SR_TXFULL)) {
- /*
- * Get the data from the UART circular buffer
- * and write it to the cdns_uart's TX_FIFO
- * register.
- */
- writel(
- port->state->xmit.buf[port->state->xmit.
- tail], port->membase + CDNS_UART_FIFO);
-
- port->icount.tx++;
-
- /*
- * Adjust the tail of the UART buffer and wrap
- * the buffer if it reaches limit.
- */
- port->state->xmit.tail =
- (port->state->xmit.tail + 1) &
- (UART_XMIT_SIZE - 1);
-
- numbytes--;
- }
+ return;
+ }
+
+ numbytes = port->fifosize;
+ while (numbytes && !uart_circ_empty(xmit) &&
+ !(readl(port->membase + CDNS_UART_SR) & CDNS_UART_SR_TXFULL)) {
+ /*
+ * Get the data from the UART circular buffer and write it
+ * to the cdns_uart's TX_FIFO register.
+ */
+ writel(xmit->buf[xmit->tail], port->membase + CDNS_UART_FIFO);
+
+ port->icount.tx++;
+
+ /*
+ * Adjust the tail of the UART buffer and wrap the buffer
+ * if it reaches limit.
+ */
+ xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
- if (uart_circ_chars_pending(
- &port->state->xmit) < WAKEUP_CHARS)
- uart_write_wakeup(port);
+ numbytes--;
}
+
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(port);
}
/**
^ permalink raw reply related
* [PATCH v2 6/6] serial: uartps: Remove useless return from cdns_uart_poll_put_char
From: Michal Simek @ 2019-06-12 11:14 UTC (permalink / raw)
To: johan, gregkh, linux-kernel, monstr, michal.simek
Cc: Nava kishore Manne, Jiri Slaby, linux-serial, linux-arm-kernel
In-Reply-To: <cover.1560338079.git.michal.simek@xilinx.com>
From: Nava kishore Manne <nava.manne@xilinx.com>
There is no reason to call return at the end of function which should
return void.
The patch is also remove one checkpatch warning:
WARNING: void function return statements are not generally useful
+ return;
+}
Fixes: 6ee04c6c5488 ("tty: xuartps: Add polled mode support for xuartps")
Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- Split patch from v1
- Add Fixes tag
drivers/tty/serial/xilinx_uartps.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index d4c1ae2ffca6..bcef254fa03c 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1074,8 +1074,6 @@ static void cdns_uart_poll_put_char(struct uart_port *port, unsigned char c)
cpu_relax();
spin_unlock_irqrestore(&port->lock, flags);
-
- return;
}
#endif
--
2.17.1
^ permalink raw reply related
* [PATCH v2 5/6] serial: uartps: Do not add a trailing semicolon to macro
From: Michal Simek @ 2019-06-12 11:14 UTC (permalink / raw)
To: johan, gregkh, linux-kernel, monstr, michal.simek
Cc: Nava kishore Manne, Jiri Slaby, linux-serial, linux-arm-kernel
In-Reply-To: <cover.1560338079.git.michal.simek@xilinx.com>
From: Nava kishore Manne <nava.manne@xilinx.com>
This patch fixes this checkpatch warning:
WARNING: macros should not use a trailing semicolon
+#define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
+ clk_rate_change_nb);
Fixes: d9bb3fb12685 ("tty: xuartps: Rebrand driver as Cadence UART")
Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- Split patch from v1
- Add Fixes tag
Origin patch which introduce this semicolon was
c4b0510cc1571ff44e1 ("tty: xuartps: Dynamically adjust to input frequency
changes")
---
drivers/tty/serial/xilinx_uartps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index c3949a323815..d4c1ae2ffca6 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -199,7 +199,7 @@ struct cdns_platform_data {
u32 quirks;
};
#define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
- clk_rate_change_nb);
+ clk_rate_change_nb)
/**
* cdns_uart_handle_rx - Handle the received bytes along with Rx errors.
--
2.17.1
^ permalink raw reply related
* [PATCH v2 4/6] serial: uartps: Fix long line over 80 chars
From: Michal Simek @ 2019-06-12 11:14 UTC (permalink / raw)
To: johan, gregkh, linux-kernel, monstr, michal.simek
Cc: Nava kishore Manne, Jiri Slaby, linux-serial, linux-arm-kernel
In-Reply-To: <cover.1560338079.git.michal.simek@xilinx.com>
From: Nava kishore Manne <nava.manne@xilinx.com>
Trivial patch which fixes one checkpatch warning:
WARNING: line over 80 characters
+ !(readl(port->membase + CDNS_UART_SR)
& CDNS_UART_SR_TXFULL)) {
Fixes: c8dbdc842d30 ("serial: xuartps: Rewrite the interrupt handling logic")
Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- Split patch from v1
- Add Fixes tag
drivers/tty/serial/xilinx_uartps.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 4cd20c036750..c3949a323815 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -312,7 +312,8 @@ static void cdns_uart_handle_tx(void *dev_id)
} else {
numbytes = port->fifosize;
while (numbytes && !uart_circ_empty(&port->state->xmit) &&
- !(readl(port->membase + CDNS_UART_SR) & CDNS_UART_SR_TXFULL)) {
+ !(readl(port->membase + CDNS_UART_SR) &
+ CDNS_UART_SR_TXFULL)) {
/*
* Get the data from the UART circular buffer
* and write it to the cdns_uart's TX_FIFO
--
2.17.1
^ permalink raw reply related
* [PATCH v2 3/6] serial: uartps: Fix multiple line dereference
From: Michal Simek @ 2019-06-12 11:14 UTC (permalink / raw)
To: johan, gregkh, linux-kernel, monstr, michal.simek
Cc: Nava kishore Manne, Jiri Slaby, linux-serial, linux-arm-kernel
In-Reply-To: <cover.1560338079.git.michal.simek@xilinx.com>
From: Nava kishore Manne <nava.manne@xilinx.com>
Trivial patch which fixes this checkpatch warning:
WARNING: Avoid multiple line dereference - prefer 'port->state->xmit.tail'
+ port->state->xmit.buf[port->state->xmit.
+ tail], port->membase + CDNS_UART_FIFO);
Fixes: c8dbdc842d30 ("serial: xuartps: Rewrite the interrupt handling logic")
Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- Split patch from v1
- Add Fixes tag
drivers/tty/serial/xilinx_uartps.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index c84db82bdaab..4cd20c036750 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -319,8 +319,8 @@ static void cdns_uart_handle_tx(void *dev_id)
* register.
*/
writel(
- port->state->xmit.buf[port->state->xmit.
- tail], port->membase + CDNS_UART_FIFO);
+ port->state->xmit.buf[port->state->xmit.tail],
+ port->membase + CDNS_UART_FIFO);
port->icount.tx++;
--
2.17.1
^ permalink raw reply related
* [PATCH v2 2/6] serial: uartps: Use octal permission for module_param()
From: Michal Simek @ 2019-06-12 11:14 UTC (permalink / raw)
To: johan, gregkh, linux-kernel, monstr, michal.simek
Cc: Nava kishore Manne, Jiri Slaby, linux-serial, linux-arm-kernel
In-Reply-To: <cover.1560338079.git.michal.simek@xilinx.com>
From: Nava kishore Manne <nava.manne@xilinx.com>
Octal permission is preffered compare to symbolic one.
This patch fixes checkpatch warnings:
Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal
permissions '0444'.
Fixes: 85baf542d54e ("tty: xuartps: support 64 byte FIFO size")
Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- Split patch from v1
- Fixes second S_IRUGO usage
- Add Fixes tag
drivers/tty/serial/xilinx_uartps.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 9dcc4d855ddd..c84db82bdaab 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -35,12 +35,12 @@
/* Rx Trigger level */
static int rx_trigger_level = 56;
static int uartps_major;
-module_param(rx_trigger_level, uint, S_IRUGO);
+module_param(rx_trigger_level, uint, 0444);
MODULE_PARM_DESC(rx_trigger_level, "Rx trigger level, 1-63 bytes");
/* Rx Timeout */
static int rx_timeout = 10;
-module_param(rx_timeout, uint, S_IRUGO);
+module_param(rx_timeout, uint, 0444);
MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
/* Register offsets for the UART. */
--
2.17.1
^ permalink raw reply related
* [PATCH v2 1/6] serial: uartps: Use the same dynamic major number for all ports
From: Michal Simek @ 2019-06-12 11:14 UTC (permalink / raw)
To: johan, gregkh, linux-kernel, monstr, michal.simek
Cc: Shubhrajyoti Datta, Jiri Slaby, linux-serial, linux-arm-kernel
In-Reply-To: <cover.1560338079.git.michal.simek@xilinx.com>
From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Let kernel to find out major number dynamically for the first device and
then reuse it for other instances.
This fixes the issue that each uart is registered with a
different major number.
After the patch:
crw------- 1 root root 253, 0 Jun 10 08:31 /dev/ttyPS0
crw--w---- 1 root root 253, 1 Jan 1 1970 /dev/ttyPS1
Fixes: 024ca329bfb9 ("serial: uartps: Register own uart console and driver structures")
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- Fix typo in subject line
- Swap patches compare to previous series
- Add Fixes tag
drivers/tty/serial/xilinx_uartps.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 605354fd60b1..9dcc4d855ddd 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -29,12 +29,12 @@
#define CDNS_UART_TTY_NAME "ttyPS"
#define CDNS_UART_NAME "xuartps"
-#define CDNS_UART_MAJOR 0 /* use dynamic node allocation */
#define CDNS_UART_FIFO_SIZE 64 /* FIFO size */
#define CDNS_UART_REGISTER_SPACE 0x1000
/* Rx Trigger level */
static int rx_trigger_level = 56;
+static int uartps_major;
module_param(rx_trigger_level, uint, S_IRUGO);
MODULE_PARM_DESC(rx_trigger_level, "Rx trigger level, 1-63 bytes");
@@ -1517,7 +1517,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
cdns_uart_uart_driver->owner = THIS_MODULE;
cdns_uart_uart_driver->driver_name = driver_name;
cdns_uart_uart_driver->dev_name = CDNS_UART_TTY_NAME;
- cdns_uart_uart_driver->major = CDNS_UART_MAJOR;
+ cdns_uart_uart_driver->major = uartps_major;
cdns_uart_uart_driver->minor = cdns_uart_data->id;
cdns_uart_uart_driver->nr = 1;
@@ -1546,6 +1546,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
goto err_out_id;
}
+ uartps_major = cdns_uart_uart_driver->tty_driver->major;
cdns_uart_data->cdns_uart_driver = cdns_uart_uart_driver;
/*
--
2.17.1
^ permalink raw reply related
* [PATCH v2 0/6] serial: uartps:
From: Michal Simek @ 2019-06-12 11:14 UTC (permalink / raw)
To: johan, gregkh, linux-kernel, monstr, michal.simek
Cc: Jiri Slaby, linux-serial, linux-arm-kernel
Hi,
This patchset is fixing several issues reported by checkpatch but also
major number handling for multiple allocated ports. For more information
please look at the first patch which describes more details.
When this patchset is applied driver only reports one checkpatch warning
which should be fixed via console.h first.
include/linux/console.h:147: void (*write)(struct console *, const char *, unsigned);
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
+ unsigned n)
Thanks,
Michal
v1
https://lkml.org/lkml/2019/6/10/186
https://lkml.org/lkml/2019/6/10/187
Changes in v2:
- Fix typo in subject line
- Swap patches compare to previous series
- Add Fixes tag
- Split patch from v1
- Fixes second S_IRUGO usage
- Add Fixes tag
- Split patch from v1
- Add Fixes tag
- Split patch from v1
- Add Fixes tag
- Split patch from v1
- Add Fixes tag
- Split patch from v1
- Add Fixes tag
Nava kishore Manne (5):
serial: uartps: Use octal permission for module_param()
serial: uartps: Fix multiple line dereference
serial: uartps: Fix long line over 80 chars
serial: uartps: Do not add a trailing semicolon to macro
serial: uartps: Remove useless return from cdns_uart_poll_put_char
Shubhrajyoti Datta (1):
serial: uartps: Use the same dynamic major number for all ports
drivers/tty/serial/xilinx_uartps.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH] serial: Fix an invalid comparing statement
From: Sugaya, Taichi @ 2019-06-12 9:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, Takao Orito, Kazuhiro Kasai, Shinji Kanematsu,
Jassi Brar, Masami Hiramatsu, linux-kernel, linux-serial
In-Reply-To: <20190610165655.GA397@kroah.com>
Hi,
On 2019/06/11 1:56, Greg Kroah-Hartman wrote:
> On Mon, May 27, 2019 at 02:01:27PM +0900, Sugaya Taichi wrote:
>> Drop the if-statement which refers to 8th bit field of u8 variable.
>> The bit field is no longer used.
>>
>> Fixes: ba44dc043004 ("serial: Add Milbeaut serial control")
>> Reported-by: Colin Ian King <colin.king@canonical.com>
>> Signed-off-by: Sugaya Taichi <sugaya.taichi@socionext.com>
>> ---
>> drivers/tty/serial/milbeaut_usio.c | 15 +++++----------
>> 1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/tty/serial/milbeaut_usio.c b/drivers/tty/serial/milbeaut_usio.c
>> index 949ab7e..d7207ab 100644
>> --- a/drivers/tty/serial/milbeaut_usio.c
>> +++ b/drivers/tty/serial/milbeaut_usio.c
>> @@ -56,7 +56,6 @@
>> #define MLB_USIO_SSR_FRE BIT(4)
>> #define MLB_USIO_SSR_PE BIT(5)
>> #define MLB_USIO_SSR_REC BIT(7)
>> -#define MLB_USIO_SSR_BRK BIT(8)
>> #define MLB_USIO_FCR_FE1 BIT(0)
>> #define MLB_USIO_FCR_FE2 BIT(1)
>> #define MLB_USIO_FCR_FCL1 BIT(2)
>> @@ -180,18 +179,14 @@ static void mlb_usio_rx_chars(struct uart_port *port)
>> if (status & MLB_USIO_SSR_ORE)
>> port->icount.overrun++;
>> status &= port->read_status_mask;
>> - if (status & MLB_USIO_SSR_BRK) {
>> - flag = TTY_BREAK;
>> + if (status & MLB_USIO_SSR_PE) {
>> + flag = TTY_PARITY;
>> ch = 0;
>> } else
>> - if (status & MLB_USIO_SSR_PE) {
>> - flag = TTY_PARITY;
>> + if (status & MLB_USIO_SSR_FRE) {
>> + flag = TTY_FRAME;
>> ch = 0;
>> - } else
>> - if (status & MLB_USIO_SSR_FRE) {
>> - flag = TTY_FRAME;
>> - ch = 0;
>> - }
>> + }
>> if (flag)
>> uart_insert_char(port, status, MLB_USIO_SSR_ORE,
>> ch, flag);
>
> While the code never actually supported Break, you are explicitly
> removing that logic now. So shouldn't you instead _fix_ break handling?
> The code before and after your change does not work any differently, so
> this patch isn't really needed at this point.
>
According to research, MLB_USIO_SSR_BRK was a remnant of old HW.
Since current one does not handle the Break, all logic related it should be
removed. I try to make a new fix patch.
Thanks,
Sugaya Taichi
> thanks,
>
> greg k-h
>
^ permalink raw reply
* Re: [PATCH 2/2 v5] tty/serial/8250: use mctrl_gpio helpers
From: Andy Shevchenko @ 2019-06-12 9:16 UTC (permalink / raw)
To: Stefan Roese
Cc: linux-serial, linux-kernel, Yegor Yefremov, Mika Westerberg,
Giulio Benetti, Greg Kroah-Hartman
In-Reply-To: <12e5180e-b4a0-e5fa-bcad-ddc8103d644c@denx.de>
On Wed, Jun 12, 2019 at 10:13:05AM +0200, Stefan Roese wrote:
> On 11.06.19 16:48, Andy Shevchenko wrote:
> > On Tue, Jun 11, 2019 at 04:02:54PM +0200, Stefan Roese wrote:
> > > On 11.06.19 14:44, Andy Shevchenko wrote:
> > > > On Tue, Jun 11, 2019 at 12:56:03PM +0200, Stefan Roese wrote:
> >
> > > > > static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
> > > > > {
> > > > > serial_out(up, UART_MCR, value);
> > > > > +
> > > > > + if (up->gpios) {
> > > > > + int mctrl_gpio = 0;
> > > > > +
> > > > > + if (value & UART_MCR_RTS)
> > > > > + mctrl_gpio |= TIOCM_RTS;
> > > > > + if (value & UART_MCR_DTR)
> > > > > + mctrl_gpio |= TIOCM_DTR;
> > > > > +
> > > > > + mctrl_gpio_set(up->gpios, mctrl_gpio);
> > > > > + }
> > > > > }
> >
> > > > > static inline int serial8250_in_MCR(struct uart_8250_port *up)
> > > > > {
> > > > > - return serial_in(up, UART_MCR);
> > > > > + int mctrl;
> > > > > +
> > > > > + mctrl = serial_in(up, UART_MCR);
> > > > > +
> > > > > + if (up->gpios) {
> > > > > + int mctrl_gpio = 0;
> > > > > +
> > > > > + /* save current MCR values */
> > > > > + if (mctrl & UART_MCR_RTS)
> > > > > + mctrl_gpio |= TIOCM_RTS;
> > > > > + if (mctrl & UART_MCR_DTR)
> > > > > + mctrl_gpio |= TIOCM_DTR;
> > > > > +
> > > > > + mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, &mctrl_gpio);
> > > > > + if (mctrl_gpio & TIOCM_RTS)
> > > > > + mctrl |= UART_MCR_RTS;
> > > > > + else
> > > > > + mctrl &= ~UART_MCR_RTS;
> > > > > +
> > > > > + if (mctrl_gpio & TIOCM_DTR)
> > > > > + mctrl |= UART_MCR_DTR;
> > > > > + else
> > > > > + mctrl &= ~UART_MCR_DTR;
> > > > > + }
> > > > > +
> > > > > + return mctrl;
> > > > > }
> > > >
> > > > These are using OR logic with potentially volatile data. Shouldn't we mask
> > > > unused bits in UART_MCR in case of up->gpios != NULL?
> > >
> > > Sorry, I don't see, which bits you are referring to? Could you please be
> > > a bit more specific with the variable / macro meant (example)?
> >
> > I meant that we double write values in the out() which might have some
> > consequences, though I hope nothing wrong with it happens.
>
> Where is the double write to a register? Sorry, I fail to spot it.
Not to the one register. From the functional point of view the same signal is
set up twice: once per UART register, once per GPIO pins.
> > In the in() we read the all bits in the register.
> >
> > As now I look at the implementation of mctrl_gpio_get_outputs(),
> > I think we rather get helpers for conversion between TIOCM and UART_MCR values,
> > so, they can be used in get_mctrl() / set_mctrl() and above.
>
> Do you something like this in mind?
More likely
static inline int serial8250_MCR_to_TIOCM(int mcr)
{
int tiocm = 0;
if (mcr & ...)
tiocm |= ...;
...
return tiocm;
}
static inline int serial8250_TIOCM_to_MCR(int tiocm)
{
... in a similar way ...
}
> Plus the use of these macros in this patch of course.
No macros, please.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC v2 01/11] OPP: Don't overwrite rounded clk rate
From: Viresh Kumar @ 2019-06-12 8:25 UTC (permalink / raw)
To: Rajendra Nayak
Cc: swboyd, linux-kernel, linux-arm-msm, linux-pm, linux-serial,
linux-spi, dri-devel, linux-scsi, ulf.hansson, dianders, rafael
In-Reply-To: <c173a57d-a4de-99f7-e8d8-28a7612f4ca3@codeaurora.org>
On 12-06-19, 13:12, Rajendra Nayak wrote:
> so the 'fmax' tables basically say what the max frequency the device can
> operate at for a given performance state/voltage level.
>
> so in your example it would be for instance
>
> 500M, Perf state = 2
> 1G, Perf state = 3
> 1.2G, Perf state = 4
>
> Now when the device wants to operate at say 800Mhz, you need to set the
> Perf state to 3, so this patch basically avoids you having to put those additional
> OPPs in the table which would otherwise look something like this
>
> 500M, Perf state = 2
> 800M, Perf state = 3 <-- redundant OPP
> 1G, Perf state = 3
> 1.2G, Perf state = 4
>
> Your example had just 1 missing entry in the 'fmax' tables in reality its a lot more,
> atleast on all qualcomm platforms.
Okay, I have applied this patch (alone) to the OPP tree with minor
modifications in commit log and diff.
--
viresh
-------------------------8<-------------------------
From: Stephen Boyd <swboyd@chromium.org>
Date: Wed, 20 Mar 2019 15:19:08 +0530
Subject: [PATCH] opp: Don't overwrite rounded clk rate
Doing this allows us to call this API with any rate requested and have
it no need to match in the OPP table. Instead, we'll round the rate up
to the nearest OPP, so that we can get the voltage or performance level
required for that OPP. This supports users of the OPP core that want to
specify the possible 'fmax' values corresponding to the voltage or
performance levels of each OPP. And for devices that required the exact
frequency, we can rely on the clk framework to round the rate to the
nearest supported frequency instead of the OPP framework to do so.
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
[ Viresh: Massaged changelog and use temp_opp variable instead ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 764e05a2fa66..0fbc77f05048 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -757,7 +757,7 @@ static int _set_required_opps(struct device *dev,
int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
{
struct opp_table *opp_table;
- unsigned long freq, old_freq;
+ unsigned long freq, old_freq, temp_freq;
struct dev_pm_opp *old_opp, *opp;
struct clk *clk;
int ret;
@@ -796,13 +796,15 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
goto put_opp_table;
}
- old_opp = _find_freq_ceil(opp_table, &old_freq);
+ temp_freq = old_freq;
+ old_opp = _find_freq_ceil(opp_table, &temp_freq);
if (IS_ERR(old_opp)) {
dev_err(dev, "%s: failed to find current OPP for freq %lu (%ld)\n",
__func__, old_freq, PTR_ERR(old_opp));
}
- opp = _find_freq_ceil(opp_table, &freq);
+ temp_freq = freq;
+ opp = _find_freq_ceil(opp_table, &temp_freq);
if (IS_ERR(opp)) {
ret = PTR_ERR(opp);
dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
^ permalink raw reply related
* Re: [PATCH 2/2 v5] tty/serial/8250: use mctrl_gpio helpers
From: Stefan Roese @ 2019-06-12 8:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-serial, linux-kernel, Yegor Yefremov, Mika Westerberg,
Giulio Benetti, Greg Kroah-Hartman
In-Reply-To: <20190611144828.GX9224@smile.fi.intel.com>
On 11.06.19 16:48, Andy Shevchenko wrote:
> On Tue, Jun 11, 2019 at 04:02:54PM +0200, Stefan Roese wrote:
>> On 11.06.19 14:44, Andy Shevchenko wrote:
>>> On Tue, Jun 11, 2019 at 12:56:03PM +0200, Stefan Roese wrote:
>
>>>> static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
>>>> {
>>>> serial_out(up, UART_MCR, value);
>>>> +
>>>> + if (up->gpios) {
>>>> + int mctrl_gpio = 0;
>>>> +
>>>> + if (value & UART_MCR_RTS)
>>>> + mctrl_gpio |= TIOCM_RTS;
>>>> + if (value & UART_MCR_DTR)
>>>> + mctrl_gpio |= TIOCM_DTR;
>>>> +
>>>> + mctrl_gpio_set(up->gpios, mctrl_gpio);
>>>> + }
>>>> }
>
>>>> static inline int serial8250_in_MCR(struct uart_8250_port *up)
>>>> {
>>>> - return serial_in(up, UART_MCR);
>>>> + int mctrl;
>>>> +
>>>> + mctrl = serial_in(up, UART_MCR);
>>>> +
>>>> + if (up->gpios) {
>>>> + int mctrl_gpio = 0;
>>>> +
>>>> + /* save current MCR values */
>>>> + if (mctrl & UART_MCR_RTS)
>>>> + mctrl_gpio |= TIOCM_RTS;
>>>> + if (mctrl & UART_MCR_DTR)
>>>> + mctrl_gpio |= TIOCM_DTR;
>>>> +
>>>> + mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, &mctrl_gpio);
>>>> + if (mctrl_gpio & TIOCM_RTS)
>>>> + mctrl |= UART_MCR_RTS;
>>>> + else
>>>> + mctrl &= ~UART_MCR_RTS;
>>>> +
>>>> + if (mctrl_gpio & TIOCM_DTR)
>>>> + mctrl |= UART_MCR_DTR;
>>>> + else
>>>> + mctrl &= ~UART_MCR_DTR;
>>>> + }
>>>> +
>>>> + return mctrl;
>>>> }
>>>
>>> These are using OR logic with potentially volatile data. Shouldn't we mask
>>> unused bits in UART_MCR in case of up->gpios != NULL?
>>
>> Sorry, I don't see, which bits you are referring to? Could you please be
>> a bit more specific with the variable / macro meant (example)?
>
> I meant that we double write values in the out() which might have some
> consequences, though I hope nothing wrong with it happens.
Where is the double write to a register? Sorry, I fail to spot it.
> In the in() we read the all bits in the register.
>
> As now I look at the implementation of mctrl_gpio_get_outputs(),
> I think we rather get helpers for conversion between TIOCM and UART_MCR values,
> so, they can be used in get_mctrl() / set_mctrl() and above.
Do you something like this in mind?
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index dc9354e34b60..f44561fcb941 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1954,19 +1954,12 @@ unsigned int serial8250_do_get_mctrl(struct uart_port *port)
status = serial8250_modem_status(up);
serial8250_rpm_put(up);
- ret = 0;
-
if (up->gpios)
return mctrl_gpio_get(up->gpios, &ret);
- if (status & UART_MSR_DCD)
- ret |= TIOCM_CAR;
- if (status & UART_MSR_RI)
- ret |= TIOCM_RNG;
- if (status & UART_MSR_DSR)
- ret |= TIOCM_DSR;
- if (status & UART_MSR_CTS)
- ret |= TIOCM_CTS;
+ ret = UART_MSR_TO_TIOCM_DCD(status) | UART_MSR_TO_TIOCM_RI(status) |
+ UART_MSR_TO_TIOCM_DSR(status) | UART_MSR_TO_TIOCM_CTS(status);
+
return ret;
}
EXPORT_SYMBOL_GPL(serial8250_do_get_mctrl);
@@ -1983,16 +1976,9 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
struct uart_8250_port *up = up_to_u8250p(port);
unsigned char mcr = 0;
- if (mctrl & TIOCM_RTS)
- mcr |= UART_MCR_RTS;
- if (mctrl & TIOCM_DTR)
- mcr |= UART_MCR_DTR;
- if (mctrl & TIOCM_OUT1)
- mcr |= UART_MCR_OUT1;
- if (mctrl & TIOCM_OUT2)
- mcr |= UART_MCR_OUT2;
- if (mctrl & TIOCM_LOOP)
- mcr |= UART_MCR_LOOP;
+ mcr = TIOCM_TO_UART_MCR_RTS(mctrl) | TIOCM_TO_UART_MCR_DTR(mctrl) |
+ TIOCM_TO_UART_MCR_OUT1(mctrl) | TIOCM_TO_UART_MCR_OUT2(mctrl) |
+ TIOCM_TO_UART_MCR_LOOP(mctrl);
mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
index be07b5470f4b..bda905a1b765 100644
--- a/include/uapi/linux/serial_reg.h
+++ b/include/uapi/linux/serial_reg.h
@@ -376,5 +376,22 @@
#define UART_ALTR_EN_TXFIFO_LW 0x01 /* Enable the TX FIFO Low Watermark */
#define UART_ALTR_TX_LOW 0x41 /* Tx FIFO Low Watermark */
+#define UART_MSR_TO_TIOCM_DCD(val) ((val & UART_MSR_DCD) ? TIOCM_CAR : 0)
+#define UART_MSR_TO_TIOCM_RI(val) ((val & UART_MSR_RI) ? TIOCM_RNG : 0)
+#define UART_MSR_TO_TIOCM_DSR(val) ((val & UART_MSR_DSR) ? TIOCM_DSR : 0)
+#define UART_MSR_TO_TIOCM_CTS(val) ((val & UART_MSR_CTS) ? TIOCM_CTS : 0)
+#define UART_MSR_TO_TIOCM_RTS(val) ((val & UART_MSR_RTS) ? TIOCM_RTS : 0)
+#define UART_MSR_TO_TIOCM_DTR(val) ((val & UART_MSR_DTR) ? TIOCM_DTR : 0)
+
+#define TIOCM_TO_UART_MCR_DCD(val) ((val & TIOCM_DCD) ? UART_MCR_CAR : 0)
+#define TIOCM_TO_UART_MCR_RI(val) ((val & TIOCM_RI) ? UART_MCR_RNG : 0)
+#define TIOCM_TO_UART_MCR_DSR(val) ((val & TIOCM_DSR) ? UART_MCR_DSR : 0)
+#define TIOCM_TO_UART_MCR_CTS(val) ((val & TIOCM_CTS) ? UART_MCR_CTS : 0)
+#define TIOCM_TO_UART_MCR_RTS(val) ((val & TIOCM_RTS) ? UART_MCR_RTS : 0)
+#define TIOCM_TO_UART_MCR_DTR(val) ((val & TIOCM_DTR) ? UART_MCR_DTR : 0)
+#define TIOCM_TO_UART_MCR_LOOP(val) ((val & TIOCM_LOOP) ? UART_MCR_LOOP : 0)
+#define TIOCM_TO_UART_MCR_OUT1(val) ((val & TIOCM_OUT1) ? UART_MCR_OUT1 : 0)
+#define TIOCM_TO_UART_MCR_OUT2(val) ((val & TIOCM_OUT2) ? UART_MCR_OUT2 : 0)
+
#endif /* _LINUX_SERIAL_REG_H */
Plus the use of these macros in this patch of course.
If yes, then I'll add a new patch (pretty similar to the one included
above) to this series.
Thanks,
Stefan
^ permalink raw reply related
* Re: [RFC v2 01/11] OPP: Don't overwrite rounded clk rate
From: Rajendra Nayak @ 2019-06-12 7:42 UTC (permalink / raw)
To: Viresh Kumar, swboyd
Cc: ulf.hansson, dianders, linux-scsi, linux-pm, linux-arm-msm,
rafael, linux-kernel, dri-devel, linux-spi, linux-serial
In-Reply-To: <20190611105432.x3nzqiib35t6mvyg@vireshk-i7>
On 6/11/2019 4:24 PM, Viresh Kumar wrote:
> On 20-03-19, 15:19, Rajendra Nayak wrote:
>> From: Stephen Boyd <swboyd@chromium.org>
>>
>> Doing this allows us to call this API with any rate requested and have
>> it not need to match in the OPP table. Instead, we'll round the rate up
>> to the nearest OPP that we see so that we can get the voltage or level
>> that's required for that OPP. This supports users of OPP that want to
>> specify the 'fmax' tables of a device instead of every single frequency
>> that they need. And for devices that required the exact frequency, we
>> can rely on the clk framework to round the rate to the nearest supported
>> frequency instead of the OPP framework to do so.
>>
>> Note that this may affect drivers that don't want the clk framework to
>> do rounding, but instead want the OPP table to do the rounding for them.
>> Do we have that case? Should we add some flag to the OPP table to
>> indicate this and then not have that flag set when there isn't an OPP
>> table for the device and also introduce a property like 'opp-use-clk' to
>> tell the table that it should use the clk APIs to round rates instead of
>> OPP?
>>
>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
[]...
>
> I see a logical problem with this patch.
>
> Suppose the clock driver supports following frequencies: 500M, 800M,
> 1G, 1.2G and the OPP table contains following list: 500M, 1G, 1.2G
> (i.e. missing 800M).
>
> Now 800M should never get programmed as it isn't part of the OPP
> table. But if you pass 600M to opp-set-rate, then it will end up
> selecting 800M as clock driver will round up to the closest value.
correct
>
> Even if no one is doing this right now, it is a sensible usecase,
> specially during testing of patches and I don't think we should avoid
> it.
>
> What exactly is the use case for which we need this patch ?
Like the changelog says 'This supports users of OPP that want to
specify the 'fmax' tables of a device instead of every single frequency
that they need'
so the 'fmax' tables basically say what the max frequency the device can
operate at for a given performance state/voltage level.
so in your example it would be for instance
500M, Perf state = 2
1G, Perf state = 3
1.2G, Perf state = 4
Now when the device wants to operate at say 800Mhz, you need to set the
Perf state to 3, so this patch basically avoids you having to put those additional
OPPs in the table which would otherwise look something like this
500M, Perf state = 2
800M, Perf state = 3 <-- redundant OPP
1G, Perf state = 3
1.2G, Perf state = 4
Your example had just 1 missing entry in the 'fmax' tables in reality its a lot more,
atleast on all qualcomm platforms.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH resend] serial: 8250: Add support for using platform_device resources
From: Enrico Weigelt, metux IT consult @ 2019-06-11 18:11 UTC (permalink / raw)
To: Esben Haabendal, Greg Kroah-Hartman
Cc: linux-serial, Lee Jones, Jiri Slaby, Andy Shevchenko,
Darwin Dingel, Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe,
Marek Vasut, Douglas Anderson, Paul Burton, linux-kernel
In-Reply-To: <878suzn9wx.fsf@haabendal.dk>
On 21.05.19 16:45, Esben Haabendal wrote:
> It sits here. It is a rather big and clunky mfd driver, not ready for
> upstreaming in its current form. I hope to get around to clean it up.
> But it is for a very specific hardware that is really available or
> usable for anybody else. Does it make sense to spend effort on
> submitting such a driver?
Maybe you could post your queue w/ "RFC: " prefix and add a proper
introduction, so everybody knows that isn't meant to be upstreamed,
but instead a basis for discussions ?
Personally, I'd like to have a look at it and understand, what's the
actual problem you'd like to solve.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: [PATCH 2/2 v5] tty/serial/8250: use mctrl_gpio helpers
From: Andy Shevchenko @ 2019-06-11 14:48 UTC (permalink / raw)
To: Stefan Roese
Cc: linux-serial, linux-kernel, Yegor Yefremov, Mika Westerberg,
Giulio Benetti, Greg Kroah-Hartman
In-Reply-To: <85f0d39c-e5d8-320b-e611-d956630a629f@denx.de>
On Tue, Jun 11, 2019 at 04:02:54PM +0200, Stefan Roese wrote:
> On 11.06.19 14:44, Andy Shevchenko wrote:
> > On Tue, Jun 11, 2019 at 12:56:03PM +0200, Stefan Roese wrote:
> > > static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
> > > {
> > > serial_out(up, UART_MCR, value);
> > > +
> > > + if (up->gpios) {
> > > + int mctrl_gpio = 0;
> > > +
> > > + if (value & UART_MCR_RTS)
> > > + mctrl_gpio |= TIOCM_RTS;
> > > + if (value & UART_MCR_DTR)
> > > + mctrl_gpio |= TIOCM_DTR;
> > > +
> > > + mctrl_gpio_set(up->gpios, mctrl_gpio);
> > > + }
> > > }
> > > static inline int serial8250_in_MCR(struct uart_8250_port *up)
> > > {
> > > - return serial_in(up, UART_MCR);
> > > + int mctrl;
> > > +
> > > + mctrl = serial_in(up, UART_MCR);
> > > +
> > > + if (up->gpios) {
> > > + int mctrl_gpio = 0;
> > > +
> > > + /* save current MCR values */
> > > + if (mctrl & UART_MCR_RTS)
> > > + mctrl_gpio |= TIOCM_RTS;
> > > + if (mctrl & UART_MCR_DTR)
> > > + mctrl_gpio |= TIOCM_DTR;
> > > +
> > > + mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, &mctrl_gpio);
> > > + if (mctrl_gpio & TIOCM_RTS)
> > > + mctrl |= UART_MCR_RTS;
> > > + else
> > > + mctrl &= ~UART_MCR_RTS;
> > > +
> > > + if (mctrl_gpio & TIOCM_DTR)
> > > + mctrl |= UART_MCR_DTR;
> > > + else
> > > + mctrl &= ~UART_MCR_DTR;
> > > + }
> > > +
> > > + return mctrl;
> > > }
> >
> > These are using OR logic with potentially volatile data. Shouldn't we mask
> > unused bits in UART_MCR in case of up->gpios != NULL?
>
> Sorry, I don't see, which bits you are referring to? Could you please be
> a bit more specific with the variable / macro meant (example)?
I meant that we double write values in the out() which might have some
consequences, though I hope nothing wrong with it happens.
In the in() we read the all bits in the register.
As now I look at the implementation of mctrl_gpio_get_outputs(),
I think we rather get helpers for conversion between TIOCM and UART_MCR values,
so, they can be used in get_mctrl() / set_mctrl() and above.
The logic now is understandable to me (I was confused by the conversions here
and there).
--
With Best Regards,
Andy Shevchenko
^ 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