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 <[email protected]>
---
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
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 <[email protected]>
> ---
Reviewed-by: Stephen Boyd <[email protected]>
> 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?
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 <[email protected]>
>> ---
>
> Reviewed-by: Stephen Boyd <[email protected]>
>
>> 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
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 <[email protected]>
>>> ---
>>
>> Reviewed-by: Stephen Boyd <[email protected]>
>>
>>> 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?
>
>
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 <[email protected]>
> >>> ---
> >>
> >> Reviewed-by: Stephen Boyd <[email protected]>
> >>
> >>> 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
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.
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 <[email protected]>
> > >>> ---
> > >>
> > >> Reviewed-by: Stephen Boyd <[email protected]>
> > >>
> > >>> 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 <[email protected]>
Regards,
Bjorn
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.
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 <[email protected]>
>>>>>> ---
>>>>>
>>>>> Reviewed-by: Stephen Boyd <[email protected]>
>>>>>
>>>>>> 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 <[email protected]>
>
> Regards,
> Bjorn
>