* Re: [PATCH v2] tty: serial: msm_serial: Fix XON/XOFF
From: Jorge Ramirez @ 2019-05-20 18:57 UTC (permalink / raw)
To: Bjorn Andersson
Cc: agross, david.brown, gregkh, sboyd, jslaby, keescook, anton,
ccross, tony.luck, linux-arm-msm, linux-serial, linux-kernel
In-Reply-To: <20190520185008.GX2085@tuxbook-pro>
On 5/20/19 20:50, Bjorn Andersson wrote:
> On Mon 20 May 11:38 PDT 2019, Jorge Ramirez-Ortiz wrote:
>
>> When the tty layer requests the uart to throttle, the current code
>> executing in msm_serial will trigger "Bad mode in Error Handler" and
>> generate an invalid stack frame in pstore before rebooting (that is if
>> pstore is indeed configured: otherwise the user shall just notice a
>> reboot with no further information dumped to the console).
>>
>> This patch replaces the PIO byte accessor with the word accessor
>> already used in PIO mode.
>>
>> Fixes: 68252424a7c7 ("tty: serial: msm: Support big-endian CPUs")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> You missed Stephen's
>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
argh sorry Stephen. can the maintainer add it when it gets merged or
shall I post V3?
>
> Regards,
> Bjorn
>
>> ---
>> drivers/tty/serial/msm_serial.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>> index 109096033bb1..23833ad952ba 100644
>> --- a/drivers/tty/serial/msm_serial.c
>> +++ b/drivers/tty/serial/msm_serial.c
>> @@ -860,6 +860,7 @@ static void msm_handle_tx(struct uart_port *port)
>> struct circ_buf *xmit = &msm_port->uart.state->xmit;
>> struct msm_dma *dma = &msm_port->tx_dma;
>> unsigned int pio_count, dma_count, dma_min;
>> + char buf[4] = { 0 };
>> void __iomem *tf;
>> int err = 0;
>>
>> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
>> else
>> tf = port->membase + UART_TF;
>>
>> + buf[0] = port->x_char;
>> +
>> if (msm_port->is_uartdm)
>> msm_reset_dm_count(port, 1);
>>
>> - iowrite8_rep(tf, &port->x_char, 1);
>> + iowrite32_rep(tf, buf, 1);
>> port->icount.tx++;
>> port->x_char = 0;
>> return;
>> --
>> 2.21.0
>>
>
^ permalink raw reply
* Re: [PATCH v2] tty: serial: msm_serial: Fix XON/XOFF
From: Bjorn Andersson @ 2019-05-20 18:50 UTC (permalink / raw)
To: Jorge Ramirez-Ortiz
Cc: agross, david.brown, gregkh, sboyd, jslaby, keescook, anton,
ccross, tony.luck, linux-arm-msm, linux-serial, linux-kernel
In-Reply-To: <20190520183848.27719-1-jorge.ramirez-ortiz@linaro.org>
On Mon 20 May 11:38 PDT 2019, Jorge Ramirez-Ortiz wrote:
> When the tty layer requests the uart to throttle, the current code
> executing in msm_serial will trigger "Bad mode in Error Handler" and
> generate an invalid stack frame in pstore before rebooting (that is if
> pstore is indeed configured: otherwise the user shall just notice a
> reboot with no further information dumped to the console).
>
> This patch replaces the PIO byte accessor with the word accessor
> already used in PIO mode.
>
> Fixes: 68252424a7c7 ("tty: serial: msm: Support big-endian CPUs")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
You missed Stephen's
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Regards,
Bjorn
> ---
> drivers/tty/serial/msm_serial.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> index 109096033bb1..23833ad952ba 100644
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -860,6 +860,7 @@ static void msm_handle_tx(struct uart_port *port)
> struct circ_buf *xmit = &msm_port->uart.state->xmit;
> struct msm_dma *dma = &msm_port->tx_dma;
> unsigned int pio_count, dma_count, dma_min;
> + char buf[4] = { 0 };
> void __iomem *tf;
> int err = 0;
>
> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
> else
> tf = port->membase + UART_TF;
>
> + buf[0] = port->x_char;
> +
> if (msm_port->is_uartdm)
> msm_reset_dm_count(port, 1);
>
> - iowrite8_rep(tf, &port->x_char, 1);
> + iowrite32_rep(tf, buf, 1);
> port->icount.tx++;
> port->x_char = 0;
> return;
> --
> 2.21.0
>
^ permalink raw reply
* [PATCH v2] tty: serial: msm_serial: Fix XON/XOFF
From: Jorge Ramirez-Ortiz @ 2019-05-20 18:38 UTC (permalink / raw)
To: jorge.ramirez-ortiz, agross, david.brown, gregkh, sboyd,
bjorn.andersson
Cc: jslaby, keescook, anton, ccross, tony.luck, linux-arm-msm,
linux-serial, linux-kernel
When the tty layer requests the uart to throttle, the current code
executing in msm_serial will trigger "Bad mode in Error Handler" and
generate an invalid stack frame in pstore before rebooting (that is if
pstore is indeed configured: otherwise the user shall just notice a
reboot with no further information dumped to the console).
This patch replaces the PIO byte accessor with the word accessor
already used in PIO mode.
Fixes: 68252424a7c7 ("tty: serial: msm: Support big-endian CPUs")
Cc: stable@vger.kernel.org
Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/tty/serial/msm_serial.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index 109096033bb1..23833ad952ba 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -860,6 +860,7 @@ static void msm_handle_tx(struct uart_port *port)
struct circ_buf *xmit = &msm_port->uart.state->xmit;
struct msm_dma *dma = &msm_port->tx_dma;
unsigned int pio_count, dma_count, dma_min;
+ char buf[4] = { 0 };
void __iomem *tf;
int err = 0;
@@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
else
tf = port->membase + UART_TF;
+ buf[0] = port->x_char;
+
if (msm_port->is_uartdm)
msm_reset_dm_count(port, 1);
- iowrite8_rep(tf, &port->x_char, 1);
+ iowrite32_rep(tf, buf, 1);
port->icount.tx++;
port->x_char = 0;
return;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF
From: Jorge Ramirez @ 2019-05-20 15:20 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Stephen Boyd, agross, david.brown, gregkh, jslaby, keescook,
anton, ccross, tony.luck, linux-arm-msm, linux-serial,
linux-kernel, khasim.mohammed, agsumit
In-Reply-To: <20190520151206.GO2085@tuxbook-pro>
On 5/20/19 17:12, Bjorn Andersson wrote:
> On Mon 20 May 08:11 PDT 2019, Bjorn Andersson wrote:
>
>> On Mon 20 May 07:58 PDT 2019, Jorge Ramirez wrote:
>>
>>> On 5/20/19 16:56, Jorge Ramirez wrote:
>>>> On 5/20/19 16:51, Stephen Boyd wrote:
>>>>> Quoting Jorge Ramirez-Ortiz (2019-05-20 03:34:35)
>>>>>> When the tty layer requests the uart to throttle, the current code
>>>>>> executing in msm_serial will trigger "Bad mode in Error Handler" and
>>>>>> generate an invalid stack frame in pstore before rebooting (that is if
>>>>>> pstore is indeed configured: otherwise the user shall just notice a
>>>>>> reboot with no further information dumped to the console).
>>>>>>
>>>>>> This patch replaces the PIO byte accessor with the word accessor
>>>>>> already used in PIO mode.
>>>>>
>>>>> Because the hardware only accepts word based accessors and fails
>>>>> otherwise? I can believe that.
>>>>>
>>>>> I wonder if the earlier UART hardware this driver used to support (i.e.
>>>>> pre-DM) would accept byte access to the registers. It's possible, but we
>>>>> don't really care because those boards aren't supported.
>>>>
>>>> ok.
>>>>
>>>> also the PIO path uses iowrite32_rep to write a number of bytes (from 1
>>>> to 4) so I think it is also appropriate to use it for XON/XOFF.
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>>>>>> ---
>>>>>
>>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>>>
>>>>>> drivers/tty/serial/msm_serial.c | 5 ++++-
>>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>>>>>> index 109096033bb1..23833ad952ba 100644
>>>>>> --- a/drivers/tty/serial/msm_serial.c
>>>>>> +++ b/drivers/tty/serial/msm_serial.c
>>>>>> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
>>>>>> else
>>>>>> tf = port->membase + UART_TF;
>>>>>>
>>>>>> + buf[0] = port->x_char;
>>>>>> +
>>>>>> if (msm_port->is_uartdm)
>>>>>> msm_reset_dm_count(port, 1);
>>>>>>
>>>>>> - iowrite8_rep(tf, &port->x_char, 1);
>>>>>> + iowrite32_rep(tf, buf, 1);
>>>>>
>>>>> I suppose it's OK to write some extra zeroes here?
>>>>>
>>>>>
>>>>
>>>> yeah, semantically confusing msm_reset_dm_count is what really matters:
>>>> it tells the hardware to only take n bytes (in this case only one) so
>>>> the others will be ignored
>>>
>>> um after I said this, maybe iowrite32_rep should only be applied to
>>> uartdm ... what do you think?
>>>
>>
>> If I read the history correctly this write was a writel() up until
>> 68252424a7c7 ("tty: serial: msm: Support big-endian CPUs").
>>
>> So I think you should just change this back to a iowrite32_rep() and add
>> a Fixes tag.
>>
>
> I mean...
ok. cool
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> Regards,
> Bjorn
>
^ permalink raw reply
* Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF
From: Bjorn Andersson @ 2019-05-20 15:12 UTC (permalink / raw)
To: Jorge Ramirez
Cc: Stephen Boyd, agross, david.brown, gregkh, jslaby, keescook,
anton, ccross, tony.luck, linux-arm-msm, linux-serial,
linux-kernel, khasim.mohammed, agsumit
In-Reply-To: <20190520151101.GN2085@tuxbook-pro>
On Mon 20 May 08:11 PDT 2019, Bjorn Andersson wrote:
> On Mon 20 May 07:58 PDT 2019, Jorge Ramirez wrote:
>
> > On 5/20/19 16:56, Jorge Ramirez wrote:
> > > On 5/20/19 16:51, Stephen Boyd wrote:
> > >> Quoting Jorge Ramirez-Ortiz (2019-05-20 03:34:35)
> > >>> When the tty layer requests the uart to throttle, the current code
> > >>> executing in msm_serial will trigger "Bad mode in Error Handler" and
> > >>> generate an invalid stack frame in pstore before rebooting (that is if
> > >>> pstore is indeed configured: otherwise the user shall just notice a
> > >>> reboot with no further information dumped to the console).
> > >>>
> > >>> This patch replaces the PIO byte accessor with the word accessor
> > >>> already used in PIO mode.
> > >>
> > >> Because the hardware only accepts word based accessors and fails
> > >> otherwise? I can believe that.
> > >>
> > >> I wonder if the earlier UART hardware this driver used to support (i.e.
> > >> pre-DM) would accept byte access to the registers. It's possible, but we
> > >> don't really care because those boards aren't supported.
> > >
> > > ok.
> > >
> > > also the PIO path uses iowrite32_rep to write a number of bytes (from 1
> > > to 4) so I think it is also appropriate to use it for XON/XOFF.
> > >
> > >>
> > >>>
> > >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> > >>> ---
> > >>
> > >> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > >>
> > >>> drivers/tty/serial/msm_serial.c | 5 ++++-
> > >>> 1 file changed, 4 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> > >>> index 109096033bb1..23833ad952ba 100644
> > >>> --- a/drivers/tty/serial/msm_serial.c
> > >>> +++ b/drivers/tty/serial/msm_serial.c
> > >>> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
> > >>> else
> > >>> tf = port->membase + UART_TF;
> > >>>
> > >>> + buf[0] = port->x_char;
> > >>> +
> > >>> if (msm_port->is_uartdm)
> > >>> msm_reset_dm_count(port, 1);
> > >>>
> > >>> - iowrite8_rep(tf, &port->x_char, 1);
> > >>> + iowrite32_rep(tf, buf, 1);
> > >>
> > >> I suppose it's OK to write some extra zeroes here?
> > >>
> > >>
> > >
> > > yeah, semantically confusing msm_reset_dm_count is what really matters:
> > > it tells the hardware to only take n bytes (in this case only one) so
> > > the others will be ignored
> >
> > um after I said this, maybe iowrite32_rep should only be applied to
> > uartdm ... what do you think?
> >
>
> If I read the history correctly this write was a writel() up until
> 68252424a7c7 ("tty: serial: msm: Support big-endian CPUs").
>
> So I think you should just change this back to a iowrite32_rep() and add
> a Fixes tag.
>
I mean...
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF
From: Bjorn Andersson @ 2019-05-20 15:11 UTC (permalink / raw)
To: Jorge Ramirez
Cc: Stephen Boyd, agross, david.brown, gregkh, jslaby, keescook,
anton, ccross, tony.luck, linux-arm-msm, linux-serial,
linux-kernel, khasim.mohammed, agsumit
In-Reply-To: <f0c89b84-7c3d-596d-06e1-cb5172e62970@linaro.org>
On Mon 20 May 07:58 PDT 2019, Jorge Ramirez wrote:
> On 5/20/19 16:56, Jorge Ramirez wrote:
> > On 5/20/19 16:51, Stephen Boyd wrote:
> >> Quoting Jorge Ramirez-Ortiz (2019-05-20 03:34:35)
> >>> When the tty layer requests the uart to throttle, the current code
> >>> executing in msm_serial will trigger "Bad mode in Error Handler" and
> >>> generate an invalid stack frame in pstore before rebooting (that is if
> >>> pstore is indeed configured: otherwise the user shall just notice a
> >>> reboot with no further information dumped to the console).
> >>>
> >>> This patch replaces the PIO byte accessor with the word accessor
> >>> already used in PIO mode.
> >>
> >> Because the hardware only accepts word based accessors and fails
> >> otherwise? I can believe that.
> >>
> >> I wonder if the earlier UART hardware this driver used to support (i.e.
> >> pre-DM) would accept byte access to the registers. It's possible, but we
> >> don't really care because those boards aren't supported.
> >
> > ok.
> >
> > also the PIO path uses iowrite32_rep to write a number of bytes (from 1
> > to 4) so I think it is also appropriate to use it for XON/XOFF.
> >
> >>
> >>>
> >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> >>> ---
> >>
> >> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> >>
> >>> drivers/tty/serial/msm_serial.c | 5 ++++-
> >>> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> >>> index 109096033bb1..23833ad952ba 100644
> >>> --- a/drivers/tty/serial/msm_serial.c
> >>> +++ b/drivers/tty/serial/msm_serial.c
> >>> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
> >>> else
> >>> tf = port->membase + UART_TF;
> >>>
> >>> + buf[0] = port->x_char;
> >>> +
> >>> if (msm_port->is_uartdm)
> >>> msm_reset_dm_count(port, 1);
> >>>
> >>> - iowrite8_rep(tf, &port->x_char, 1);
> >>> + iowrite32_rep(tf, buf, 1);
> >>
> >> I suppose it's OK to write some extra zeroes here?
> >>
> >>
> >
> > yeah, semantically confusing msm_reset_dm_count is what really matters:
> > it tells the hardware to only take n bytes (in this case only one) so
> > the others will be ignored
>
> um after I said this, maybe iowrite32_rep should only be applied to
> uartdm ... what do you think?
>
If I read the history correctly this write was a writel() up until
68252424a7c7 ("tty: serial: msm: Support big-endian CPUs").
So I think you should just change this back to a iowrite32_rep() and add
a Fixes tag.
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF
From: Jorge Ramirez @ 2019-05-20 15:07 UTC (permalink / raw)
To: Stephen Boyd, agross, david.brown, gregkh
Cc: jslaby, keescook, anton, ccross, tony.luck, linux-arm-msm,
linux-serial, linux-kernel, khasim.mohammed, agsumit
In-Reply-To: <20190520150320.5DBC520856@mail.kernel.org>
On 5/20/19 17:03, Stephen Boyd wrote:
> Quoting Jorge Ramirez (2019-05-20 07:58:54)
>> On 5/20/19 16:56, Jorge Ramirez wrote:
>>>
>>> yeah, semantically confusing msm_reset_dm_count is what really matters:
>>> it tells the hardware to only take n bytes (in this case only one) so
>>> the others will be ignored
>>
>> um after I said this, maybe iowrite32_rep should only be applied to
>> uartdm ... what do you think?
>>
>
> Probably. The uartdm hardware typically required words everywhere while
> the pre-dm hardware didn't. It's an if condition so it should be OK.
>
> It may be time to remove non-uartdm support from this driver
> all-together. From what I recall the only devices that are upstream are
> the uartdm ones, so it may be easier to just remove the legacy stuff
> that nobody has tested in many years.
>
>
should this be merged before removing the non-uartdm support or after?
I also have some other changes - in particular with respect to the usage
of fifosize which according to the documentation is in words not in bytes.
^ permalink raw reply
* Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF
From: Stephen Boyd @ 2019-05-20 15:03 UTC (permalink / raw)
To: Jorge Ramirez, agross, david.brown, gregkh
Cc: jslaby, keescook, anton, ccross, tony.luck, linux-arm-msm,
linux-serial, linux-kernel, khasim.mohammed, agsumit
In-Reply-To: <f0c89b84-7c3d-596d-06e1-cb5172e62970@linaro.org>
Quoting Jorge Ramirez (2019-05-20 07:58:54)
> On 5/20/19 16:56, Jorge Ramirez wrote:
> >
> > yeah, semantically confusing msm_reset_dm_count is what really matters:
> > it tells the hardware to only take n bytes (in this case only one) so
> > the others will be ignored
>
> um after I said this, maybe iowrite32_rep should only be applied to
> uartdm ... what do you think?
>
Probably. The uartdm hardware typically required words everywhere while
the pre-dm hardware didn't. It's an if condition so it should be OK.
It may be time to remove non-uartdm support from this driver
all-together. From what I recall the only devices that are upstream are
the uartdm ones, so it may be easier to just remove the legacy stuff
that nobody has tested in many years.
^ permalink raw reply
* Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF
From: Jorge Ramirez @ 2019-05-20 14:58 UTC (permalink / raw)
To: Stephen Boyd, agross, david.brown, gregkh
Cc: jslaby, keescook, anton, ccross, tony.luck, linux-arm-msm,
linux-serial, linux-kernel, khasim.mohammed, agsumit
In-Reply-To: <254704a2-ee20-30cd-8362-6e1bd23ec090@linaro.org>
On 5/20/19 16:56, Jorge Ramirez wrote:
> On 5/20/19 16:51, Stephen Boyd wrote:
>> Quoting Jorge Ramirez-Ortiz (2019-05-20 03:34:35)
>>> When the tty layer requests the uart to throttle, the current code
>>> executing in msm_serial will trigger "Bad mode in Error Handler" and
>>> generate an invalid stack frame in pstore before rebooting (that is if
>>> pstore is indeed configured: otherwise the user shall just notice a
>>> reboot with no further information dumped to the console).
>>>
>>> This patch replaces the PIO byte accessor with the word accessor
>>> already used in PIO mode.
>>
>> Because the hardware only accepts word based accessors and fails
>> otherwise? I can believe that.
>>
>> I wonder if the earlier UART hardware this driver used to support (i.e.
>> pre-DM) would accept byte access to the registers. It's possible, but we
>> don't really care because those boards aren't supported.
>
> ok.
>
> also the PIO path uses iowrite32_rep to write a number of bytes (from 1
> to 4) so I think it is also appropriate to use it for XON/XOFF.
>
>>
>>>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>>> ---
>>
>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>
>>> drivers/tty/serial/msm_serial.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>>> index 109096033bb1..23833ad952ba 100644
>>> --- a/drivers/tty/serial/msm_serial.c
>>> +++ b/drivers/tty/serial/msm_serial.c
>>> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
>>> else
>>> tf = port->membase + UART_TF;
>>>
>>> + buf[0] = port->x_char;
>>> +
>>> if (msm_port->is_uartdm)
>>> msm_reset_dm_count(port, 1);
>>>
>>> - iowrite8_rep(tf, &port->x_char, 1);
>>> + iowrite32_rep(tf, buf, 1);
>>
>> I suppose it's OK to write some extra zeroes here?
>>
>>
>
> yeah, semantically confusing msm_reset_dm_count is what really matters:
> it tells the hardware to only take n bytes (in this case only one) so
> the others will be ignored
um after I said this, maybe iowrite32_rep should only be applied to
uartdm ... what do you think?
>
>
^ permalink raw reply
* Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF
From: Jorge Ramirez @ 2019-05-20 14:56 UTC (permalink / raw)
To: Stephen Boyd, agross, david.brown, gregkh
Cc: jslaby, keescook, anton, ccross, tony.luck, linux-arm-msm,
linux-serial, linux-kernel, khasim.mohammed, agsumit
In-Reply-To: <20190520145110.7BDAE21721@mail.kernel.org>
On 5/20/19 16:51, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz (2019-05-20 03:34:35)
>> When the tty layer requests the uart to throttle, the current code
>> executing in msm_serial will trigger "Bad mode in Error Handler" and
>> generate an invalid stack frame in pstore before rebooting (that is if
>> pstore is indeed configured: otherwise the user shall just notice a
>> reboot with no further information dumped to the console).
>>
>> This patch replaces the PIO byte accessor with the word accessor
>> already used in PIO mode.
>
> Because the hardware only accepts word based accessors and fails
> otherwise? I can believe that.
>
> I wonder if the earlier UART hardware this driver used to support (i.e.
> pre-DM) would accept byte access to the registers. It's possible, but we
> don't really care because those boards aren't supported.
ok.
also the PIO path uses iowrite32_rep to write a number of bytes (from 1
to 4) so I think it is also appropriate to use it for XON/XOFF.
>
>>
>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> ---
>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>
>> drivers/tty/serial/msm_serial.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>> index 109096033bb1..23833ad952ba 100644
>> --- a/drivers/tty/serial/msm_serial.c
>> +++ b/drivers/tty/serial/msm_serial.c
>> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
>> else
>> tf = port->membase + UART_TF;
>>
>> + buf[0] = port->x_char;
>> +
>> if (msm_port->is_uartdm)
>> msm_reset_dm_count(port, 1);
>>
>> - iowrite8_rep(tf, &port->x_char, 1);
>> + iowrite32_rep(tf, buf, 1);
>
> I suppose it's OK to write some extra zeroes here?
>
>
yeah, semantically confusing msm_reset_dm_count is what really matters:
it tells the hardware to only take n bytes (in this case only one) so
the others will be ignored
^ permalink raw reply
* Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF
From: Stephen Boyd @ 2019-05-20 14:51 UTC (permalink / raw)
To: agross, david.brown, gregkh, jorge.ramirez-ortiz
Cc: jslaby, keescook, anton, ccross, tony.luck, linux-arm-msm,
linux-serial, linux-kernel, khasim.mohammed, agsumit
In-Reply-To: <20190520103435.30850-1-jorge.ramirez-ortiz@linaro.org>
Quoting Jorge Ramirez-Ortiz (2019-05-20 03:34:35)
> When the tty layer requests the uart to throttle, the current code
> executing in msm_serial will trigger "Bad mode in Error Handler" and
> generate an invalid stack frame in pstore before rebooting (that is if
> pstore is indeed configured: otherwise the user shall just notice a
> reboot with no further information dumped to the console).
>
> This patch replaces the PIO byte accessor with the word accessor
> already used in PIO mode.
Because the hardware only accepts word based accessors and fails
otherwise? I can believe that.
I wonder if the earlier UART hardware this driver used to support (i.e.
pre-DM) would accept byte access to the registers. It's possible, but we
don't really care because those boards aren't supported.
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> drivers/tty/serial/msm_serial.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> index 109096033bb1..23833ad952ba 100644
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
> else
> tf = port->membase + UART_TF;
>
> + buf[0] = port->x_char;
> +
> if (msm_port->is_uartdm)
> msm_reset_dm_count(port, 1);
>
> - iowrite8_rep(tf, &port->x_char, 1);
> + iowrite32_rep(tf, buf, 1);
I suppose it's OK to write some extra zeroes here?
^ permalink raw reply
* [PATCH] tty: serial: msm_serial: Fix XON/XOFF
From: Jorge Ramirez-Ortiz @ 2019-05-20 10:34 UTC (permalink / raw)
To: jorge.ramirez-ortiz, agross, david.brown, gregkh
Cc: jslaby, keescook, anton, ccross, tony.luck, linux-arm-msm,
linux-serial, linux-kernel, khasim.mohammed, agsumit
When the tty layer requests the uart to throttle, the current code
executing in msm_serial will trigger "Bad mode in Error Handler" and
generate an invalid stack frame in pstore before rebooting (that is if
pstore is indeed configured: otherwise the user shall just notice a
reboot with no further information dumped to the console).
This patch replaces the PIO byte accessor with the word accessor
already used in PIO mode.
Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
drivers/tty/serial/msm_serial.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index 109096033bb1..23833ad952ba 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -860,6 +860,7 @@ static void msm_handle_tx(struct uart_port *port)
struct circ_buf *xmit = &msm_port->uart.state->xmit;
struct msm_dma *dma = &msm_port->tx_dma;
unsigned int pio_count, dma_count, dma_min;
+ char buf[4] = { 0 };
void __iomem *tf;
int err = 0;
@@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
else
tf = port->membase + UART_TF;
+ buf[0] = port->x_char;
+
if (msm_port->is_uartdm)
msm_reset_dm_count(port, 1);
- iowrite8_rep(tf, &port->x_char, 1);
+ iowrite32_rep(tf, buf, 1);
port->icount.tx++;
port->x_char = 0;
return;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit
From: Johan Hovold @ 2019-05-17 7:55 UTC (permalink / raw)
To: YueHaibing
Cc: jacmet, gregkh, jslaby, shubhrajyoti.datta, linux-kernel,
linux-serial
In-Reply-To: <20190516040931.16276-1-yuehaibing@huawei.com>
On Thu, May 16, 2019 at 12:09:31PM +0800, YueHaibing wrote:
> If ulite_probe is not called or failed to registed
> uart_register_driver, unload the module will call
> uart_unregister_driver, which will tigger NULL
> pointer dereference like this:
>
> BUG: KASAN: null-ptr-deref in tty_unregister_driver+0x19/0x100
> Read of size 4 at addr 0000000000000034 by task syz-executor.0/4246
> This patch fix this by moving uart_unregister_driver
> to ulite_remove.
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> drivers/tty/serial/uartlite.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index b8b912b..2e49fb6 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -867,6 +867,7 @@ static int ulite_remove(struct platform_device *pdev)
> pm_runtime_disable(&pdev->dev);
> pm_runtime_set_suspended(&pdev->dev);
> pm_runtime_dont_use_autosuspend(&pdev->dev);
> + uart_unregister_driver(&ulite_uart_driver);
This broken. Consider what happens if you have tho ports registered and
you unbind the first.
Someone else sent a fix for this here
https://lkml.kernel.org/r/20190514033219.169947-1-wangkefeng.wang@huawei.com
That fix also has some issues, but is still better given the current
state this driver is in.
> return rc;
> }
>
> @@ -897,7 +898,6 @@ static int __init ulite_init(void)
> static void __exit ulite_exit(void)
> {
> platform_driver_unregister(&ulite_platform_driver);
> - uart_unregister_driver(&ulite_uart_driver);
> }
>
> module_init(ulite_init);
Johan
^ permalink raw reply
* Re: [PATCH 4/4] serial: 8250-mtk: modify uart DMA rx
From: Long Cheng @ 2019-05-17 7:36 UTC (permalink / raw)
To: Nicolas Boichat
Cc: Mark Rutland, devicetree, Ryder Lee, Zhenbao Liu, linux-serial,
srv_heupstream, Greg Kroah-Hartman, Randy Dunlap, lkml, Sean Wang,
YT Shen, dmaengine, Vinod Koul, Rob Herring,
moderated list:ARM/Mediatek SoC support, Sean Wang, Jiri Slaby,
Matthias Brugger, Yingjoe Chen, Dan Williams,
linux-arm Mailing List
In-Reply-To: <CANMq1KDTyu48joV6uMksGBMz9EmjFH9SEpGAm93YCZ40jxgBpQ@mail.gmail.com>
On Wed, 2019-05-15 at 21:48 +0800, Nicolas Boichat wrote:
> On Sat, Apr 27, 2019 at 11:36 AM Long Cheng <long.cheng@mediatek.com> wrote:
> >
> > Modify uart rx and complete for DMA.
>
> I don't know much about the DMA framework, but can you please explain
> why you are making the changes in this CL? I see that you are dropping
> dma_sync_single_for_device calls, for example, why?
>
the rx buffer is create by 'dma_alloc_coherent'. in the function, the
buffer is uncache. We don't need to sync between CPU and DMA. So I
remove it.
> >
> > Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> > ---
> > drivers/tty/serial/8250/8250_mtk.c | 53 ++++++++++++++++--------------------
> > 1 file changed, 23 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> > index c1fdbc0..04081a6 100644
> > --- a/drivers/tty/serial/8250/8250_mtk.c
> > +++ b/drivers/tty/serial/8250/8250_mtk.c
> > @@ -30,7 +30,6 @@
> > #define MTK_UART_DMA_EN_TX 0x2
> > #define MTK_UART_DMA_EN_RX 0x5
> >
> > -#define MTK_UART_TX_SIZE UART_XMIT_SIZE
> > #define MTK_UART_RX_SIZE 0x8000
> > #define MTK_UART_TX_TRIGGER 1
> > #define MTK_UART_RX_TRIGGER MTK_UART_RX_SIZE
> > @@ -64,28 +63,30 @@ static void mtk8250_dma_rx_complete(void *param)
> > struct mtk8250_data *data = up->port.private_data;
> > struct tty_port *tty_port = &up->port.state->port;
> > struct dma_tx_state state;
> > + int copied, cnt, tmp;
> > unsigned char *ptr;
> > - int copied;
> >
> > - dma_sync_single_for_cpu(dma->rxchan->device->dev, dma->rx_addr,
> > - dma->rx_size, DMA_FROM_DEVICE);
> > + if (data->rx_status == DMA_RX_SHUTDOWN)
> > + return;
> >
> > dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> > + cnt = dma->rx_size - state.residue;
> > + tmp = cnt;
>
> I ponder, maybe we should rename cnt to left? (like, how many bytes
> are left to transfer, in total) Or maybe "total"
> Then maybe rename tmp to cnt.
>
like better.
> >
> > - if (data->rx_status == DMA_RX_SHUTDOWN)
> > - return;
> > + if ((data->rx_pos + cnt) > dma->rx_size)
> > + tmp = dma->rx_size - data->rx_pos;
>
> Maybe replace this and the line above:
> tmp = max_t(int, cnt, dma->rx_size - data->rx_pos);
>
Yes. It's better.
> >
> > - if ((data->rx_pos + state.residue) <= dma->rx_size) {
> > - ptr = (unsigned char *)(data->rx_pos + dma->rx_buf);
> > - copied = tty_insert_flip_string(tty_port, ptr, state.residue);
> > - } else {
> > - ptr = (unsigned char *)(data->rx_pos + dma->rx_buf);
> > - copied = tty_insert_flip_string(tty_port, ptr,
> > - dma->rx_size - data->rx_pos);
> > + ptr = (unsigned char *)(data->rx_pos + dma->rx_buf);
> > + copied = tty_insert_flip_string(tty_port, ptr, tmp);
> > + data->rx_pos += tmp;
> > +
> > + if (cnt > tmp) {
> > ptr = (unsigned char *)(dma->rx_buf);
> > - copied += tty_insert_flip_string(tty_port, ptr,
> > - data->rx_pos + state.residue - dma->rx_size);
> > + tmp = cnt - tmp;
> > + copied += tty_insert_flip_string(tty_port, ptr, tmp);
> > + data->rx_pos = tmp;
> > }
> > +
> > up->port.icount.rx += copied;
> >
> > tty_flip_buffer_push(tty_port);
> > @@ -96,9 +97,7 @@ static void mtk8250_dma_rx_complete(void *param)
> > static void mtk8250_rx_dma(struct uart_8250_port *up)
> > {
> > struct uart_8250_dma *dma = up->dma;
> > - struct mtk8250_data *data = up->port.private_data;
> > struct dma_async_tx_descriptor *desc;
> > - struct dma_tx_state state;
> >
> > desc = dmaengine_prep_slave_single(dma->rxchan, dma->rx_addr,
> > dma->rx_size, DMA_DEV_TO_MEM,
> > @@ -113,12 +112,6 @@ static void mtk8250_rx_dma(struct uart_8250_port *up)
> >
> > dma->rx_cookie = dmaengine_submit(desc);
> >
> > - dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> > - data->rx_pos = state.residue;
> > -
> > - dma_sync_single_for_device(dma->rxchan->device->dev, dma->rx_addr,
> > - dma->rx_size, DMA_FROM_DEVICE);
> > -
> > dma_async_issue_pending(dma->rxchan);
> > }
> >
> > @@ -131,13 +124,13 @@ static void mtk8250_dma_enable(struct uart_8250_port *up)
> > if (data->rx_status != DMA_RX_START)
> > return;
> >
> > - dma->rxconf.direction = DMA_DEV_TO_MEM;
> > - dma->rxconf.src_addr_width = dma->rx_size / 1024;
> > - dma->rxconf.src_addr = dma->rx_addr;
> > + dma->rxconf.direction = DMA_DEV_TO_MEM;
> > + dma->rxconf.src_port_window_size = dma->rx_size;
> > + dma->rxconf.src_addr = dma->rx_addr;
> >
> > - dma->txconf.direction = DMA_MEM_TO_DEV;
> > - dma->txconf.dst_addr_width = MTK_UART_TX_SIZE / 1024;
> > - dma->txconf.dst_addr = dma->tx_addr;
> > + dma->txconf.direction = DMA_MEM_TO_DEV;
> > + dma->txconf.dst_port_window_size = UART_XMIT_SIZE;
> > + dma->txconf.dst_addr = dma->tx_addr;
> >
> > serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
> > UART_FCR_CLEAR_XMIT);
> > @@ -217,7 +210,7 @@ static void mtk8250_shutdown(struct uart_port *port)
> > * Mediatek UARTs use an extra highspeed register (UART_MTK_HIGHS)
> > *
> > * We need to recalcualte the quot register, as the claculation depends
> > - * on the vaule in the highspeed register.
> > + * on the value in the highspeed register.
>
> Since you're doing some cosmetic changes here, you might as well fix
> recalcualte => recalculate and claculation => calculation on the line
> above.
>
I see.
> But technically, this should belong in another patch...
>
> > *
> > * Some baudrates are not supported by the chip, so we use the next
> > * lower rate supported and update termios c_flag.
> > --
> > 1.7.9.5
> >
^ permalink raw reply
* Re: [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit
From: Shubhrajyoti Datta @ 2019-05-16 14:52 UTC (permalink / raw)
To: YueHaibing
Cc: jacmet, Greg Kroah-Hartman, jslaby, Shubhrajyoti Datta,
linux-kernel, linux-serial
In-Reply-To: <20190516040931.16276-1-yuehaibing@huawei.com>
Hi YueHaibing,
Thanks for the patch.
On Thu, May 16, 2019 at 9:42 AM YueHaibing <yuehaibing@huawei.com> wrote:
>
> If ulite_probe is not called or failed to registed
s/registed/register
> uart_register_driver, unload the module will call
> uart_unregister_driver, which will tigger NULL
s/tigger/trigger
> pointer dereference like this:
>
> BUG: KASAN: null-ptr-deref in tty_unregister_driver+0x19/0x100
> Read of size 4 at addr 0000000000000034 by task syz-executor.0/4246
>
> CPU: 0 PID: 4246 Comm: syz-executor.0 Tainted: G C 5.1.0+ #26
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
> Call Trace:
> dump_stack+0xa9/0x10e
> ? tty_unregister_driver+0x19/0x100
> ? tty_unregister_driver+0x19/0x100
> __kasan_report+0x171/0x18d
> ? tty_unregister_driver+0x19/0x100
> kasan_report+0xe/0x20
> tty_unregister_driver+0x19/0x100
> uart_unregister_driver+0x30/0xc0
> __x64_sys_delete_module+0x244/0x330
> ? __ia32_sys_delete_module+0x330/0x330
> ? __x64_sys_clock_gettime+0xe3/0x160
> ? trace_hardirqs_on_thunk+0x1a/0x1c
> ? trace_hardirqs_off_caller+0x3e/0x130
> ? lockdep_hardirqs_off+0xb5/0x100
> ? mark_held_locks+0x1a/0x90
> ? do_syscall_64+0x14/0x2a0
> do_syscall_64+0x72/0x2a0
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> This patch fix this by moving uart_unregister_driver
> to ulite_remove.
>
Reviewed-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> drivers/tty/serial/uartlite.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index b8b912b..2e49fb6 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -867,6 +867,7 @@ static int ulite_remove(struct platform_device *pdev)
> pm_runtime_disable(&pdev->dev);
> pm_runtime_set_suspended(&pdev->dev);
> pm_runtime_dont_use_autosuspend(&pdev->dev);
> + uart_unregister_driver(&ulite_uart_driver);
> return rc;
> }
>
> @@ -897,7 +898,6 @@ static int __init ulite_init(void)
> static void __exit ulite_exit(void)
> {
> platform_driver_unregister(&ulite_platform_driver);
> - uart_unregister_driver(&ulite_uart_driver);
> }
>
> module_init(ulite_init);
> --
> 1.8.3.1
>
>
^ permalink raw reply
* [PATCH] serial-uartlite: Fix null-ptr-deref in ulite_exit
From: YueHaibing @ 2019-05-16 4:09 UTC (permalink / raw)
To: jacmet, gregkh, jslaby, shubhrajyoti.datta
Cc: linux-kernel, linux-serial, YueHaibing
If ulite_probe is not called or failed to registed
uart_register_driver, unload the module will call
uart_unregister_driver, which will tigger NULL
pointer dereference like this:
BUG: KASAN: null-ptr-deref in tty_unregister_driver+0x19/0x100
Read of size 4 at addr 0000000000000034 by task syz-executor.0/4246
CPU: 0 PID: 4246 Comm: syz-executor.0 Tainted: G C 5.1.0+ #26
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
Call Trace:
dump_stack+0xa9/0x10e
? tty_unregister_driver+0x19/0x100
? tty_unregister_driver+0x19/0x100
__kasan_report+0x171/0x18d
? tty_unregister_driver+0x19/0x100
kasan_report+0xe/0x20
tty_unregister_driver+0x19/0x100
uart_unregister_driver+0x30/0xc0
__x64_sys_delete_module+0x244/0x330
? __ia32_sys_delete_module+0x330/0x330
? __x64_sys_clock_gettime+0xe3/0x160
? trace_hardirqs_on_thunk+0x1a/0x1c
? trace_hardirqs_off_caller+0x3e/0x130
? lockdep_hardirqs_off+0xb5/0x100
? mark_held_locks+0x1a/0x90
? do_syscall_64+0x14/0x2a0
do_syscall_64+0x72/0x2a0
entry_SYSCALL_64_after_hwframe+0x49/0xbe
This patch fix this by moving uart_unregister_driver
to ulite_remove.
Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/tty/serial/uartlite.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index b8b912b..2e49fb6 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -867,6 +867,7 @@ static int ulite_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
pm_runtime_set_suspended(&pdev->dev);
pm_runtime_dont_use_autosuspend(&pdev->dev);
+ uart_unregister_driver(&ulite_uart_driver);
return rc;
}
@@ -897,7 +898,6 @@ static int __init ulite_init(void)
static void __exit ulite_exit(void)
{
platform_driver_unregister(&ulite_platform_driver);
- uart_unregister_driver(&ulite_uart_driver);
}
module_init(ulite_init);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 4/4] serial: 8250-mtk: modify uart DMA rx
From: Nicolas Boichat @ 2019-05-15 13:48 UTC (permalink / raw)
To: Long Cheng
Cc: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang, Matthias Brugger, Dan Williams, Greg Kroah-Hartman,
Jiri Slaby, Sean Wang, dmaengine, devicetree,
linux-arm Mailing List, moderated list:ARM/Mediatek SoC support,
lkml, linux-serial, srv_heupstream, Yingjoe Chen
In-Reply-To: <1556336193-15198-5-git-send-email-long.cheng@mediatek.com>
On Sat, Apr 27, 2019 at 11:36 AM Long Cheng <long.cheng@mediatek.com> wrote:
>
> Modify uart rx and complete for DMA.
I don't know much about the DMA framework, but can you please explain
why you are making the changes in this CL? I see that you are dropping
dma_sync_single_for_device calls, for example, why?
>
> Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> ---
> drivers/tty/serial/8250/8250_mtk.c | 53 ++++++++++++++++--------------------
> 1 file changed, 23 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> index c1fdbc0..04081a6 100644
> --- a/drivers/tty/serial/8250/8250_mtk.c
> +++ b/drivers/tty/serial/8250/8250_mtk.c
> @@ -30,7 +30,6 @@
> #define MTK_UART_DMA_EN_TX 0x2
> #define MTK_UART_DMA_EN_RX 0x5
>
> -#define MTK_UART_TX_SIZE UART_XMIT_SIZE
> #define MTK_UART_RX_SIZE 0x8000
> #define MTK_UART_TX_TRIGGER 1
> #define MTK_UART_RX_TRIGGER MTK_UART_RX_SIZE
> @@ -64,28 +63,30 @@ static void mtk8250_dma_rx_complete(void *param)
> struct mtk8250_data *data = up->port.private_data;
> struct tty_port *tty_port = &up->port.state->port;
> struct dma_tx_state state;
> + int copied, cnt, tmp;
> unsigned char *ptr;
> - int copied;
>
> - dma_sync_single_for_cpu(dma->rxchan->device->dev, dma->rx_addr,
> - dma->rx_size, DMA_FROM_DEVICE);
> + if (data->rx_status == DMA_RX_SHUTDOWN)
> + return;
>
> dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> + cnt = dma->rx_size - state.residue;
> + tmp = cnt;
I ponder, maybe we should rename cnt to left? (like, how many bytes
are left to transfer, in total) Or maybe "total"
Then maybe rename tmp to cnt.
>
> - if (data->rx_status == DMA_RX_SHUTDOWN)
> - return;
> + if ((data->rx_pos + cnt) > dma->rx_size)
> + tmp = dma->rx_size - data->rx_pos;
Maybe replace this and the line above:
tmp = max_t(int, cnt, dma->rx_size - data->rx_pos);
>
> - if ((data->rx_pos + state.residue) <= dma->rx_size) {
> - ptr = (unsigned char *)(data->rx_pos + dma->rx_buf);
> - copied = tty_insert_flip_string(tty_port, ptr, state.residue);
> - } else {
> - ptr = (unsigned char *)(data->rx_pos + dma->rx_buf);
> - copied = tty_insert_flip_string(tty_port, ptr,
> - dma->rx_size - data->rx_pos);
> + ptr = (unsigned char *)(data->rx_pos + dma->rx_buf);
> + copied = tty_insert_flip_string(tty_port, ptr, tmp);
> + data->rx_pos += tmp;
> +
> + if (cnt > tmp) {
> ptr = (unsigned char *)(dma->rx_buf);
> - copied += tty_insert_flip_string(tty_port, ptr,
> - data->rx_pos + state.residue - dma->rx_size);
> + tmp = cnt - tmp;
> + copied += tty_insert_flip_string(tty_port, ptr, tmp);
> + data->rx_pos = tmp;
> }
> +
> up->port.icount.rx += copied;
>
> tty_flip_buffer_push(tty_port);
> @@ -96,9 +97,7 @@ static void mtk8250_dma_rx_complete(void *param)
> static void mtk8250_rx_dma(struct uart_8250_port *up)
> {
> struct uart_8250_dma *dma = up->dma;
> - struct mtk8250_data *data = up->port.private_data;
> struct dma_async_tx_descriptor *desc;
> - struct dma_tx_state state;
>
> desc = dmaengine_prep_slave_single(dma->rxchan, dma->rx_addr,
> dma->rx_size, DMA_DEV_TO_MEM,
> @@ -113,12 +112,6 @@ static void mtk8250_rx_dma(struct uart_8250_port *up)
>
> dma->rx_cookie = dmaengine_submit(desc);
>
> - dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> - data->rx_pos = state.residue;
> -
> - dma_sync_single_for_device(dma->rxchan->device->dev, dma->rx_addr,
> - dma->rx_size, DMA_FROM_DEVICE);
> -
> dma_async_issue_pending(dma->rxchan);
> }
>
> @@ -131,13 +124,13 @@ static void mtk8250_dma_enable(struct uart_8250_port *up)
> if (data->rx_status != DMA_RX_START)
> return;
>
> - dma->rxconf.direction = DMA_DEV_TO_MEM;
> - dma->rxconf.src_addr_width = dma->rx_size / 1024;
> - dma->rxconf.src_addr = dma->rx_addr;
> + dma->rxconf.direction = DMA_DEV_TO_MEM;
> + dma->rxconf.src_port_window_size = dma->rx_size;
> + dma->rxconf.src_addr = dma->rx_addr;
>
> - dma->txconf.direction = DMA_MEM_TO_DEV;
> - dma->txconf.dst_addr_width = MTK_UART_TX_SIZE / 1024;
> - dma->txconf.dst_addr = dma->tx_addr;
> + dma->txconf.direction = DMA_MEM_TO_DEV;
> + dma->txconf.dst_port_window_size = UART_XMIT_SIZE;
> + dma->txconf.dst_addr = dma->tx_addr;
>
> serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
> UART_FCR_CLEAR_XMIT);
> @@ -217,7 +210,7 @@ static void mtk8250_shutdown(struct uart_port *port)
> * Mediatek UARTs use an extra highspeed register (UART_MTK_HIGHS)
> *
> * We need to recalcualte the quot register, as the claculation depends
> - * on the vaule in the highspeed register.
> + * on the value in the highspeed register.
Since you're doing some cosmetic changes here, you might as well fix
recalcualte => recalculate and claculation => calculation on the line
above.
But technically, this should belong in another patch...
> *
> * Some baudrates are not supported by the chip, so we use the next
> * lower rate supported and update termios c_flag.
> --
> 1.7.9.5
>
^ permalink raw reply
* Re: [PATCH] tty: serial_core: Fix the incorrect configuration of baud rate and data length at the console serial port resume
From: hhome liu @ 2019-05-15 9:15 UTC (permalink / raw)
To: Johan Hovold
Cc: Rob Herring, Greg Kroah-Hartman, baolin.wang, Baolin Wang, jslaby,
刘岚清 (Lanqing Liu), linux-serial,
linux-kernel, chunyan.zhang, orson.zhai, zhang.lyra
In-Reply-To: <20190514073457.GI9651@localhost>
Johan Hovold <johan@kernel.org> 于2019年5月14日周二 下午3:35写道：
>
> On Thu, May 09, 2019 at 01:42:39PM +0800, Lanqing Liu wrote:
> > When userspace opens a serial port for console, uart_port_startup()
> > is called. This function assigns the uport->cons->cflag value to
> > TTY->termios.c_cflag, then it is cleared to 0. When the user space
> > closes this serial port, the TTY structure will be released, and at
> > this time uport->cons->cflag has also been cleared.
> >
> > On the Spreadtrum platform, in some special scenarios, like charging mode,
> > userspace needs to close the console, which means the uport->cons->cflag
> > has also been cleared. But printing logs is still needed in the kernel. So
> > when system enters suspend and resume, the console needs to be configure
> > the baud rate and data length of the serial port according to its own cflag
> > when resuming the console port. At this time, the cflag is 0, which will
> > cause serial port to produce configuration errors that do not meet user
> > expectations.
>
> This is actually yet another regression due to 761ed4a94582 ("tty:
> serial_core: convert uart_close to use tty_port_close") which
> incidentally removed the call to uart_shutdown() where the cflag was
> being saved precisely to avoid the problem you're describing:
>
> ae84db9661ca ("serial: core: Preserve termios c_cflag for console resume")
Yes, agree with you.
>
> Judging from a quick look it seems the xmit buf, which is released in
> that function may now be leaking too.
We haven't found this issue before, but we can try to reproduce it on
our platform.
>
> > To fix this, assigning the TTY->termios.c_cflag value to uport->cons->cflag
> > before the userspace closes this console serial port. It will ensure that
> > the correct cflag value can be gotten when the console serial port was
> > resumed.
>
> Not sure this is the right fix, but I don't have time to look at this
> right now.
OK. Thanks for your comments.
^ permalink raw reply
* [PATCH v3] serial: sh-sci: disable DMA for uart_console
From: George G. Davis @ 2019-05-15 3:29 UTC (permalink / raw)
To: Eugeniu Rosca, Geert Uytterhoeven, Simon Horman, Wolfram Sang,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
open list
Cc: Chris Brandt, Ulrich Hecht, Andy Lowe, Linux-Renesas,
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Magnus Damm,
Rob Herring, Mark Rutland, George G. Davis, stable
As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
console UART"), UART console lines use low-level PIO only access functions
which will conflict with use of the line when DMA is enabled, e.g. when
the console line is also used for systemd messages. So disable DMA
support for UART console lines.
Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
Link: https://patchwork.kernel.org/patch/10929511/
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: stable@vger.kernel.org
Signed-off-by: George G. Davis <george_davis@mentor.com>
---
v2: Clarify comment regarding DMA support on kernel console,
add {Tested,Reviewed}-by:, and Cc: linux-stable lines.
v3: Change Fixes: reference to Link: reference and add another Reviewed-by.
---
drivers/tty/serial/sh-sci.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 3cd139752d3f..abc705716aa0 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1557,6 +1557,13 @@ static void sci_request_dma(struct uart_port *port)
dev_dbg(port->dev, "%s: port %d\n", __func__, port->line);
+ /*
+ * DMA on console may interfere with Kernel log messages which use
+ * plain putchar(). So, simply don't use it with a console.
+ */
+ if (uart_console(port))
+ return;
+
if (!port->dev->of_node)
return;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2] serial: sh-sci: disable DMA for uart_console
From: George G. Davis @ 2019-05-14 16:16 UTC (permalink / raw)
To: Wolfram Sang
Cc: Geert Uytterhoeven, Eugeniu Rosca, Simon Horman, Wolfram Sang,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
open list, Chris Brandt, Ulrich Hecht, Andy Lowe, Linux-Renesas,
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Magnus Damm,
Rob Herring, Mark Rutland, stable
In-Reply-To: <20190514155450.GB6508@kunai>
Hello Wolfram,
On Tue, May 14, 2019 at 05:54:51PM +0200, Wolfram Sang wrote:
>
> > > > Fixes: https://patchwork.kernel.org/patch/10929511/
> > >
> > > I don't think this is an appropriate reference, as it points to a patch that
> > > was never applied.
> >
> > I included it as a link to an upstream problem report similar to other commits
> > that I previewed. The link provides the extra context that I was perhaps to
> > lazy to note in the commit header.
>
> We have a "Link:" tag for things like this, e.g.:
>
> Link: https://patchwork.kernel.org/patch/10929511/
Right, I've changed it to a Link instead.
Thanks!
--
Regards,
George
^ permalink raw reply
* Re: [PATCH v2] serial: sh-sci: disable DMA for uart_console
From: Wolfram Sang @ 2019-05-14 15:54 UTC (permalink / raw)
To: George G. Davis
Cc: Geert Uytterhoeven, Eugeniu Rosca, Simon Horman, Wolfram Sang,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
open list, Chris Brandt, Ulrich Hecht, Andy Lowe, Linux-Renesas,
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Magnus Damm,
Rob Herring, Mark Rutland, stable
In-Reply-To: <20190514153021.GC18528@mam-gdavis-lt>
[-- Attachment #1: Type: text/plain, Size: 485 bytes --]
> > > Fixes: https://patchwork.kernel.org/patch/10929511/
> >
> > I don't think this is an appropriate reference, as it points to a patch that
> > was never applied.
>
> I included it as a link to an upstream problem report similar to other commits
> that I previewed. The link provides the extra context that I was perhaps to
> lazy to note in the commit header.
We have a "Link:" tag for things like this, e.g.:
Link: https://patchwork.kernel.org/patch/10929511/
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2] serial: sh-sci: disable DMA for uart_console
From: George G. Davis @ 2019-05-14 15:30 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Eugeniu Rosca, Simon Horman, Wolfram Sang, Greg Kroah-Hartman,
Jiri Slaby, open list:SERIAL DRIVERS, open list, Chris Brandt,
Ulrich Hecht, Andy Lowe, Linux-Renesas,
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Magnus Damm,
Rob Herring, Mark Rutland, stable
In-Reply-To: <CAMuHMdVaNWa=Q-7K-+_rM-8yYWB0-+4_o4hgACK6o-4BOrY07A@mail.gmail.com>
Hello Geert,
On Tue, May 14, 2019 at 10:28:34AM +0200, Geert Uytterhoeven wrote:
> Hi George,
>
> On Mon, May 13, 2019 at 5:48 PM George G. Davis <george_davis@mentor.com> wrote:
> > As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> > console UART"), UART console lines use low-level PIO only access functions
> > which will conflict with use of the line when DMA is enabled, e.g. when
> > the console line is also used for systemd messages. So disable DMA
> > support for UART console lines.
> >
> > Fixes: https://patchwork.kernel.org/patch/10929511/
>
> I don't think this is an appropriate reference, as it points to a patch that
> was never applied.
I included it as a link to an upstream problem report similar to other commits
that I previewed. The link provides the extra context that I was perhaps to
lazy to note in the commit header.
> As the problem has basically existed forever,
Agreed
> IMHO no Fixes tag
> is needed.
I've dropped the Fixes line.
> > Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: George G. Davis <george_davis@mentor.com>
> > ---
> > v2: Clarify comment regarding DMA support on kernel console,
> > add {Tested,Reviewed}-by:, and Cc: linux-stable lines.
>
> Thanks for the update!
Thanks!
I'll submit v3 later today.
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
--
Regards,
George
^ permalink raw reply
* Re: [PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip
From: Thomas Bogendoerfer @ 2019-05-14 14:29 UTC (permalink / raw)
To: Lee Jones
Cc: Ralf Baechle, Paul Burton, James Hogan, Dmitry Torokhov,
David S. Miller, Alessandro Zummo, Alexandre Belloni,
Greg Kroah-Hartman, Jiri Slaby, linux-mips, linux-kernel,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190510071419.GB7321@dell>
On Fri, 10 May 2019 08:14:19 +0100
Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 09 May 2019, Thomas Bogendoerfer wrote:
> > > > + }
> > > > + pr_err("ioc3: CRC error in NIC address\n");
> > > > +}
> > >
> > > This all looks like networking code. If this is the case, it should
> > > be moved to drivers/networking or similar.
> >
> > no it's not. nic stands for number in a can produced by Dallas Semi also
> > known under the name 1-Wire (https://en.wikipedia.org/wiki/1-Wire).
> > SGI used them to provide partnumber, serialnumber and mac addresses.
> > By placing the code to read the NiCs inside ioc3 driver there is no need
> > for locking and adding library code for accessing these informations.
>
> Great. So it looks like you should be using this, no?
>
> drivers/base/regmap/regmap-w1.c
not sure about regmap-w1, but drivers/w1 contains usefull stuff
like w1_crc8. The only drawback I see right now is, that I need
information about part numbers at ioc3 probe time, but for using
w1 framework I need to create a platform device, which will give
me this information not synchronous. I'll see how I could solve that.
> > > > +static struct resource ioc3_uarta_resources[] = {
> > > > + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
> > >
> > > You are the first user of offsetof() in MFD. Could you tell me why
> > > it's required please?
> >
> > to get the offsets of different chip functions out of a struct.
>
> I can see what it does on a coding level.
>
> What are you using it for in practical/real terms?
ioc3 has one PCI bar, where all different functions are accessible.
The current ioc3 register map has all these functions set up in one
struct. The base address of these registers comes out of the PCI
framework and to use the MFD framework I need offsets for the different
functions. And because there was already struct ioc3 I'm using
offsetof on this struct.
> Why wouldn't any other MFD driver require this, but you do?
the other PCI MFD drivers I've looked at, have a PCI BAR per function,
which makes live easier and no need for offsetof. Other MFD drivers
#define the offsets and don't have a big struct, which contains all
function registers. If you really insist on using #defines I need
to go through a few parts of the kernel where struct ioc3 is still used.
> > > > + if (ipd->info->funcs & IOC3_SER) {
> > > > + writel(GPCR_UARTA_MODESEL | GPCR_UARTB_MODESEL,
> > > > + &ipd->regs->gpcr_s);
> > > > + writel(0, &ipd->regs->gppr[6]);
> > > > + writel(0, &ipd->regs->gppr[7]);
> > > > + udelay(100);
> > > > + writel(readl(&ipd->regs->port_a.sscr) & ~SSCR_DMA_EN,
> > > > + &ipd->regs->port_a.sscr);
> > > > + writel(readl(&ipd->regs->port_b.sscr) & ~SSCR_DMA_EN,
> > > > + &ipd->regs->port_b.sscr);
> > > > + udelay(1000);
> > >
> > > No idea what any of this does.
> > >
> > > It looks like it belongs in the serial driver (and needs comments).
> >
> > it configures the IOC3 chip for serial usage. This is not part of
> > the serial register set, so it IMHO belongs in the MFD driver.
>
> So it does serial things, but doesn't belong in the serial driver?
It sets up IOC3 GPIOs and IOC3 serial mode in way the 8250 driver
can work with the connected superio.
> Could you please go into a bit more detail as to why you think that?
>
> Why is it better here?
access to gpio and serial mode is outside of the 8250 register space.
So either I need to export with some additional resources/new special
platform data or just set it where it is done.
> It's also totally unreadable by the way!
sure, I'll add comments.
> > > > + }
> > > > +#if defined(CONFIG_SGI_IP27)
> > >
> > > What is this? Can't you obtain this dynamically by probing the H/W?
> >
> > that's the machine type and the #ifdef CONFIG_xxx are just for saving space,
> > when compiled for other machines and it's easy to remove.
>
> Please find other ways to save the space. #ifery can get very messy,
> very quickly and is almost always avoidable.
space isn't a problem at all, so removing #ifdef CONFIG is easy.
>
> > > > + if (ipd->info->irq_offset) {
> > >
> > > What does this really signify?
> >
> > IOC3 ASICs are most of the time connected to a SGI bridge chip. IOC3 can
> > provide two interrupt lines, which are wired to the bridge chip. The first
> > interrupt is assigned via the PCI core, but since IOC3 is not a PCI multi
> > function device the second interrupt must be treated here. And the used
> > interrupt line on the bridge chip differs between boards.
>
> Please provide a MACRO, function or something else which results in
> more readable code. Whatever you choose to use, please add this text
> above, it will be helpful for future readers.
will do.
Thomas.
--
SUSE Linux GmbH
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Andy Shevchenko @ 2019-05-14 12:42 UTC (permalink / raw)
To: Esben Haabendal
Cc: Lee Jones, open list:SERIAL DRIVERS, Enrico Weigelt,
Greg Kroah-Hartman, Jiri Slaby, Darwin Dingel, Jisheng Zhang,
Sebastian Andrzej Siewior, He Zhe, Marek Vasut, Douglas Anderson,
Paul Burton, Linux Kernel Mailing List
In-Reply-To: <87zhnpkzvj.fsf@haabendal.dk>
On Tue, May 14, 2019 at 02:02:40PM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>
> > On Tue, May 14, 2019 at 12:23 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
> >
> >> > Please take a look at https://lkml.org/lkml/2019/4/9/576
> >> > ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
> >>
> >> Thank you for this link.
> >> Now, look at this comment:
> >>
> >> + /*
> >> + * Map all IOC3 registers. These are shared between subdevices
> >> + * so the main IOC3 module manages them.
> >> + */
> >>
> >> Is it your case? Can we see the code?
> >
> > They do not request resources by the way.
>
> Actually, that looks like a bug in ioc3.c driver.
Nope. This is the right thing to do.
> It is using mfd_add_devices() with a mem_base that has not been properly
> requested, and the platform_get_resource() calls made by child drivers
> does not guarantee exclusive access to the memory resources, as they are
> not inserted in the root memory resource tree.
Should platform_get_resource() guarantee that? I think no, otherwise entire MFD
and other logic will collapse.
> > You may do the same, I told you this several times.
>
> In drivers/mfd/ioc3.c:
>
> First, the uart resources are defined. The register memory resource is
> defined relative to the mfd driver memory resource.
>
> +static struct resource ioc3_uarta_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
> + sizeof_field(struct ioc3, sregs.uarta)),
> + DEFINE_RES_IRQ(6)
> +};
>
> This is then used when creating the uart cell.
>
> + cell->name = "ioc3-serial8250";
> + cell->id = ioc3_serial_id++;
> + cell->resources = ioc3_uarta_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_uarta_resources);
>
> Finally, the mfd_add_devices() call is made, giving the resource for the
> BAR0 region (&ipd->pdev->resource[0]) as mem_base argument:
>
> + mfd_add_devices(&ipd->pdev->dev, -1, ioc3_mfd_cells,
> + cell - ioc3_mfd_cells, &ipd->pdev->resource[0],
> + 0, ipd->domain);
>
> This is just what I want to do.
>
> But in order to guarantee exclusive access to the memory resource, I
> need to have it requested.
Here the root of our misunderstanding each other.
Every driver till now works fine and entire model works fine without resources
being requested.
I told you already that if you want your way that has to be done not in 8250
driver, but in generic code (driver core or even resource framework).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function
From: Esben Haabendal @ 2019-05-14 12:41 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Lee Jones, linux-serial, Jiri Slaby, Nishanth Menon, Vignesh R,
Tony Lindgren, Lokesh Vutla, Florian Fainelli, linux-kernel
In-Reply-To: <20190514122618.GA18859@kroah.com>
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Tue, May 14, 2019 at 11:47:41AM +0100, Lee Jones wrote:
>> On Tue, 14 May 2019, Esben Haabendal wrote:
>>
>> > Lee Jones <lee.jones@linaro.org> writes:
>> >
>> > > On Tue, 07 May 2019, Esben Haabendal wrote:
>> > >
>> > >> Lee Jones <lee.jones@linaro.org> writes:
>> > >>
>> > >> > On Fri, 26 Apr 2019, Esben Haabendal wrote:
>> > >> >
>> > >> >> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
>> > >> >> to an MFD driver.
>> > >> >>
>> > >> >> When calling mfd_add_device(), platform_data should be a pointer to a
>> > >> >> struct plat_serial8250_port, with proper settings like .flags, .type,
>> > >> >> .iotype, .regshift and .uartclk. Memory (or ioport) and IRQ should be
>> > >> >> passed as cell resources.
>> > >> >
>> > >> > What? No, please!
>> > >> >
>> > >> > If you *must* create a whole driver just to be able to use
>> > >> > platform_*() helpers (which I don't think you should), then please
>> > >> > call it something else. This doesn't have anything to do with MFD.
>> > >>
>> > >> True.
>> > >>
>> > >> I really don't think it is a good idea to create a whole driver just to
>> > >> be able to use platform_get_*() helpers. And if I am forced to do this,
>> > >> because I am unable to convince Andy to improve the standard serial8250
>> > >> driver to support that, it should be called MFD. The driver would be
>> > >
>> > > I assume you mean "shouldn't"?
>> >
>> > Of-course.
>> >
>> > >> generally usable for all usecases where platform_get_*() works.
>> > >>
>> > >> I don't have any idea what to call such a driver. It really would just
>> > >> be a fork of the current serial8250 driver, just allowing use of
>> > >> platform_get_*(), supporting exactly the same hardware.
>> > >>
>> > >> I am still hoping that we can find a way to improve serial8250 to be
>> > >> usable in these cases.
>> > >
>> > > Me too.
>> >
>> > Unfortunately, I don't seem to be able to convince Andy to accept
>> > something like that.
>>
>> Andy is not he Maintainer.
>>
>> What are Greg and Jiri's opinions?
>
> I've been ignoring all of this at the moment because of the 5.2-rc merge
> window. I'll look at it after -rc1 is out.
>
> thanks,
> greg k-h
Great, thanks!
I will try ad hold back with this thread until you get back to it.
/Esben
^ 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