* [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges
@ 2024-09-06 22:54 Florian Fainelli
2024-09-11 13:46 ` Greg Kroah-Hartman
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Florian Fainelli @ 2024-09-06 22:54 UTC (permalink / raw)
To: linux-serial
Cc: Florian Fainelli, Jim Quinlan, Kevin Cernekee, Greg Kroah-Hartman,
Jiri Slaby, John Ogness, Ilpo Järvinen, Thomas Gleixner,
open list:TTY LAYER AND SERIAL DRIVERS
The write to RP2_GLOBAL_CMD followed by an immediate read of
RP2_GLOBAL_CMD in rp2_reset_asic() is intented to flush out the write,
however by then the device is already in reset and cannot respond to a
memory cycle access.
On platforms such as the Raspberry Pi 4 and others using the
pcie-brcmstb.c driver, any memory access to a device that cannot respond
is met with a fatal system error, rather than being substituted with all
1s as is usually the case on PC platforms.
Swapping the delay and the read ensures that the device has finished
resetting before we attempt to read from it.
Fixes: 7d9f49afa451 ("serial: rp2: New driver for Comtrol RocketPort 2 cards")
Suggested-by: Jim Quinlan <james.quinlan@broadcom.com>
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
drivers/tty/serial/rp2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/rp2.c b/drivers/tty/serial/rp2.c
index 4132fcff7d4e..8bab2aedc499 100644
--- a/drivers/tty/serial/rp2.c
+++ b/drivers/tty/serial/rp2.c
@@ -577,8 +577,8 @@ static void rp2_reset_asic(struct rp2_card *card, unsigned int asic_id)
u32 clk_cfg;
writew(1, base + RP2_GLOBAL_CMD);
- readw(base + RP2_GLOBAL_CMD);
msleep(100);
+ readw(base + RP2_GLOBAL_CMD);
writel(0, base + RP2_CLK_PRESCALER);
/* TDM clock configuration */
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges
2024-09-06 22:54 [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges Florian Fainelli
@ 2024-09-11 13:46 ` Greg Kroah-Hartman
2024-09-11 21:47 ` Jim Quinlan
2024-09-23 9:44 ` Jiri Slaby
2 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2024-09-11 13:46 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-serial, Jim Quinlan, Kevin Cernekee, Jiri Slaby,
John Ogness, Ilpo Järvinen, Thomas Gleixner,
open list:TTY LAYER AND SERIAL DRIVERS
On Fri, Sep 06, 2024 at 03:54:33PM -0700, Florian Fainelli wrote:
> The write to RP2_GLOBAL_CMD followed by an immediate read of
> RP2_GLOBAL_CMD in rp2_reset_asic() is intented to flush out the write,
> however by then the device is already in reset and cannot respond to a
> memory cycle access.
>
> On platforms such as the Raspberry Pi 4 and others using the
> pcie-brcmstb.c driver, any memory access to a device that cannot respond
> is met with a fatal system error, rather than being substituted with all
> 1s as is usually the case on PC platforms.
>
> Swapping the delay and the read ensures that the device has finished
> resetting before we attempt to read from it.
>
> Fixes: 7d9f49afa451 ("serial: rp2: New driver for Comtrol RocketPort 2 cards")
> Suggested-by: Jim Quinlan <james.quinlan@broadcom.com>
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
> drivers/tty/serial/rp2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- You have marked a patch with a "Fixes:" tag for a commit that is in an
older released kernel, yet you do not have a cc: stable line in the
signed-off-by area at all, which means that the patch will not be
applied to any older kernel releases. To properly fix this, please
follow the documented rules in the
Documentation/process/stable-kernel-rules.rst file for how to resolve
this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges
2024-09-06 22:54 [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges Florian Fainelli
2024-09-11 13:46 ` Greg Kroah-Hartman
@ 2024-09-11 21:47 ` Jim Quinlan
2024-09-11 22:01 ` Florian Fainelli
2024-09-23 9:44 ` Jiri Slaby
2 siblings, 1 reply; 10+ messages in thread
From: Jim Quinlan @ 2024-09-11 21:47 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-serial, Kevin Cernekee, Greg Kroah-Hartman, Jiri Slaby,
John Ogness, Ilpo Järvinen, Thomas Gleixner,
open list:TTY LAYER AND SERIAL DRIVERS
[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]
On Fri, Sep 6, 2024 at 6:54 PM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> The write to RP2_GLOBAL_CMD followed by an immediate read of
> RP2_GLOBAL_CMD in rp2_reset_asic() is intented to flush out the write,
> however by then the device is already in reset and cannot respond to a
> memory cycle access.
>
> On platforms such as the Raspberry Pi 4 and others using the
> pcie-brcmstb.c driver, any memory access to a device that cannot respond
> is met with a fatal system error, rather than being substituted with all
> 1s as is usually the case on PC platforms.
>
> Swapping the delay and the read ensures that the device has finished
> resetting before we attempt to read from it.
>
> Fixes: 7d9f49afa451 ("serial: rp2: New driver for Comtrol RocketPort 2 cards")
> Suggested-by: Jim Quinlan <james.quinlan@broadcom.com>
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
> drivers/tty/serial/rp2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/rp2.c b/drivers/tty/serial/rp2.c
> index 4132fcff7d4e..8bab2aedc499 100644
> --- a/drivers/tty/serial/rp2.c
> +++ b/drivers/tty/serial/rp2.c
> @@ -577,8 +577,8 @@ static void rp2_reset_asic(struct rp2_card *card, unsigned int asic_id)
> u32 clk_cfg;
>
> writew(1, base + RP2_GLOBAL_CMD);
> - readw(base + RP2_GLOBAL_CMD);
> msleep(100);
> + readw(base + RP2_GLOBAL_CMD);
Since the assumed purpose of the readw() was to flush the writew(),
would it make sense to add a barrier after the writew()?
Regards,
Jim Quinlan
Broadcom STB/CM
> writel(0, base + RP2_CLK_PRESCALER);
>
> /* TDM clock configuration */
> --
> 2.43.0
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges
2024-09-11 21:47 ` Jim Quinlan
@ 2024-09-11 22:01 ` Florian Fainelli
2024-09-11 22:16 ` Jim Quinlan
0 siblings, 1 reply; 10+ messages in thread
From: Florian Fainelli @ 2024-09-11 22:01 UTC (permalink / raw)
To: Jim Quinlan
Cc: linux-serial, Kevin Cernekee, Greg Kroah-Hartman, Jiri Slaby,
John Ogness, Ilpo Järvinen, Thomas Gleixner,
open list:TTY LAYER AND SERIAL DRIVERS
On 9/11/24 14:47, Jim Quinlan wrote:
> On Fri, Sep 6, 2024 at 6:54 PM Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
>>
>> The write to RP2_GLOBAL_CMD followed by an immediate read of
>> RP2_GLOBAL_CMD in rp2_reset_asic() is intented to flush out the write,
>> however by then the device is already in reset and cannot respond to a
>> memory cycle access.
>>
>> On platforms such as the Raspberry Pi 4 and others using the
>> pcie-brcmstb.c driver, any memory access to a device that cannot respond
>> is met with a fatal system error, rather than being substituted with all
>> 1s as is usually the case on PC platforms.
>>
>> Swapping the delay and the read ensures that the device has finished
>> resetting before we attempt to read from it.
>>
>> Fixes: 7d9f49afa451 ("serial: rp2: New driver for Comtrol RocketPort 2 cards")
>> Suggested-by: Jim Quinlan <james.quinlan@broadcom.com>
>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> ---
>> drivers/tty/serial/rp2.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/rp2.c b/drivers/tty/serial/rp2.c
>> index 4132fcff7d4e..8bab2aedc499 100644
>> --- a/drivers/tty/serial/rp2.c
>> +++ b/drivers/tty/serial/rp2.c
>> @@ -577,8 +577,8 @@ static void rp2_reset_asic(struct rp2_card *card, unsigned int asic_id)
>> u32 clk_cfg;
>>
>> writew(1, base + RP2_GLOBAL_CMD);
>> - readw(base + RP2_GLOBAL_CMD);
>> msleep(100);
>> + readw(base + RP2_GLOBAL_CMD);
>
> Since the assumed purpose of the readw() was to flush the writew(),
> would it make sense to add a barrier after the writew()?
AFAICT there is one which is implied within the name, as it is not a
_relaxed() variant. Did you mean a different sort of barrier to be used?
--
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges
2024-09-11 22:01 ` Florian Fainelli
@ 2024-09-11 22:16 ` Jim Quinlan
2024-09-11 22:19 ` Florian Fainelli
0 siblings, 1 reply; 10+ messages in thread
From: Jim Quinlan @ 2024-09-11 22:16 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-serial, Kevin Cernekee, Greg Kroah-Hartman, Jiri Slaby,
John Ogness, Ilpo Järvinen, Thomas Gleixner,
open list:TTY LAYER AND SERIAL DRIVERS
[-- Attachment #1: Type: text/plain, Size: 2200 bytes --]
On Wed, Sep 11, 2024 at 6:01 PM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> On 9/11/24 14:47, Jim Quinlan wrote:
> > On Fri, Sep 6, 2024 at 6:54 PM Florian Fainelli
> > <florian.fainelli@broadcom.com> wrote:
> >>
> >> The write to RP2_GLOBAL_CMD followed by an immediate read of
> >> RP2_GLOBAL_CMD in rp2_reset_asic() is intented to flush out the write,
> >> however by then the device is already in reset and cannot respond to a
> >> memory cycle access.
> >>
> >> On platforms such as the Raspberry Pi 4 and others using the
> >> pcie-brcmstb.c driver, any memory access to a device that cannot respond
> >> is met with a fatal system error, rather than being substituted with all
> >> 1s as is usually the case on PC platforms.
> >>
> >> Swapping the delay and the read ensures that the device has finished
> >> resetting before we attempt to read from it.
> >>
> >> Fixes: 7d9f49afa451 ("serial: rp2: New driver for Comtrol RocketPort 2 cards")
> >> Suggested-by: Jim Quinlan <james.quinlan@broadcom.com>
> >> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> >> ---
> >> drivers/tty/serial/rp2.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/tty/serial/rp2.c b/drivers/tty/serial/rp2.c
> >> index 4132fcff7d4e..8bab2aedc499 100644
> >> --- a/drivers/tty/serial/rp2.c
> >> +++ b/drivers/tty/serial/rp2.c
> >> @@ -577,8 +577,8 @@ static void rp2_reset_asic(struct rp2_card *card, unsigned int asic_id)
> >> u32 clk_cfg;
> >>
> >> writew(1, base + RP2_GLOBAL_CMD);
> >> - readw(base + RP2_GLOBAL_CMD);
> >> msleep(100);
> >> + readw(base + RP2_GLOBAL_CMD);
> >
> > Since the assumed purpose of the readw() was to flush the writew(),
> > would it make sense to add a barrier after the writew()?
>
> AFAICT there is one which is implied within the name, as it is not a
> _relaxed() variant. Did you mean a different sort of barrier to be used?
Not sure. The __raw_writew() is followed by __io_aw(), which is a
no-op on arm64. I don't know arm64 well enough to know if a follow-up
barrier is needed.
- Jim
> --
> Florian
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges
2024-09-11 22:16 ` Jim Quinlan
@ 2024-09-11 22:19 ` Florian Fainelli
2024-09-11 22:44 ` Jim Quinlan
0 siblings, 1 reply; 10+ messages in thread
From: Florian Fainelli @ 2024-09-11 22:19 UTC (permalink / raw)
To: Jim Quinlan
Cc: linux-serial, Kevin Cernekee, Greg Kroah-Hartman, Jiri Slaby,
John Ogness, Ilpo Järvinen, Thomas Gleixner,
open list:TTY LAYER AND SERIAL DRIVERS
On 9/11/24 15:16, Jim Quinlan wrote:
> On Wed, Sep 11, 2024 at 6:01 PM Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
>>
>> On 9/11/24 14:47, Jim Quinlan wrote:
>>> On Fri, Sep 6, 2024 at 6:54 PM Florian Fainelli
>>> <florian.fainelli@broadcom.com> wrote:
>>>>
>>>> The write to RP2_GLOBAL_CMD followed by an immediate read of
>>>> RP2_GLOBAL_CMD in rp2_reset_asic() is intented to flush out the write,
>>>> however by then the device is already in reset and cannot respond to a
>>>> memory cycle access.
>>>>
>>>> On platforms such as the Raspberry Pi 4 and others using the
>>>> pcie-brcmstb.c driver, any memory access to a device that cannot respond
>>>> is met with a fatal system error, rather than being substituted with all
>>>> 1s as is usually the case on PC platforms.
>>>>
>>>> Swapping the delay and the read ensures that the device has finished
>>>> resetting before we attempt to read from it.
>>>>
>>>> Fixes: 7d9f49afa451 ("serial: rp2: New driver for Comtrol RocketPort 2 cards")
>>>> Suggested-by: Jim Quinlan <james.quinlan@broadcom.com>
>>>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
>>>> ---
>>>> drivers/tty/serial/rp2.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/tty/serial/rp2.c b/drivers/tty/serial/rp2.c
>>>> index 4132fcff7d4e..8bab2aedc499 100644
>>>> --- a/drivers/tty/serial/rp2.c
>>>> +++ b/drivers/tty/serial/rp2.c
>>>> @@ -577,8 +577,8 @@ static void rp2_reset_asic(struct rp2_card *card, unsigned int asic_id)
>>>> u32 clk_cfg;
>>>>
>>>> writew(1, base + RP2_GLOBAL_CMD);
>>>> - readw(base + RP2_GLOBAL_CMD);
>>>> msleep(100);
>>>> + readw(base + RP2_GLOBAL_CMD);
>>>
>>> Since the assumed purpose of the readw() was to flush the writew(),
>>> would it make sense to add a barrier after the writew()?
>>
>> AFAICT there is one which is implied within the name, as it is not a
>> _relaxed() variant. Did you mean a different sort of barrier to be used?
>
> Not sure. The __raw_writew() is followed by __io_aw(), which is a
> no-op on arm64. I don't know arm64 well enough to know if a follow-up
> barrier is needed.
By definition all of the {read,write}{b,w,l,q} do include an adequate
barrier and perform the adequate endian swapping since they originated
from PCI drivers on x86. If you do not want any barrier you would have
to use the _relaxed variants, or if you want native ordering, you would
use the __raw_* variant.
--
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges
2024-09-11 22:19 ` Florian Fainelli
@ 2024-09-11 22:44 ` Jim Quinlan
2024-09-12 0:19 ` Florian Fainelli
0 siblings, 1 reply; 10+ messages in thread
From: Jim Quinlan @ 2024-09-11 22:44 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-serial, Kevin Cernekee, Greg Kroah-Hartman, Jiri Slaby,
John Ogness, Ilpo Järvinen, Thomas Gleixner,
open list:TTY LAYER AND SERIAL DRIVERS
[-- Attachment #1: Type: text/plain, Size: 3155 bytes --]
On Wed, Sep 11, 2024 at 6:19 PM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> On 9/11/24 15:16, Jim Quinlan wrote:
> > On Wed, Sep 11, 2024 at 6:01 PM Florian Fainelli
> > <florian.fainelli@broadcom.com> wrote:
> >>
> >> On 9/11/24 14:47, Jim Quinlan wrote:
> >>> On Fri, Sep 6, 2024 at 6:54 PM Florian Fainelli
> >>> <florian.fainelli@broadcom.com> wrote:
> >>>>
> >>>> The write to RP2_GLOBAL_CMD followed by an immediate read of
> >>>> RP2_GLOBAL_CMD in rp2_reset_asic() is intented to flush out the write,
> >>>> however by then the device is already in reset and cannot respond to a
> >>>> memory cycle access.
> >>>>
> >>>> On platforms such as the Raspberry Pi 4 and others using the
> >>>> pcie-brcmstb.c driver, any memory access to a device that cannot respond
> >>>> is met with a fatal system error, rather than being substituted with all
> >>>> 1s as is usually the case on PC platforms.
> >>>>
> >>>> Swapping the delay and the read ensures that the device has finished
> >>>> resetting before we attempt to read from it.
> >>>>
> >>>> Fixes: 7d9f49afa451 ("serial: rp2: New driver for Comtrol RocketPort 2 cards")
> >>>> Suggested-by: Jim Quinlan <james.quinlan@broadcom.com>
> >>>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> >>>> ---
> >>>> drivers/tty/serial/rp2.c | 2 +-
> >>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/tty/serial/rp2.c b/drivers/tty/serial/rp2.c
> >>>> index 4132fcff7d4e..8bab2aedc499 100644
> >>>> --- a/drivers/tty/serial/rp2.c
> >>>> +++ b/drivers/tty/serial/rp2.c
> >>>> @@ -577,8 +577,8 @@ static void rp2_reset_asic(struct rp2_card *card, unsigned int asic_id)
> >>>> u32 clk_cfg;
> >>>>
> >>>> writew(1, base + RP2_GLOBAL_CMD);
> >>>> - readw(base + RP2_GLOBAL_CMD);
> >>>> msleep(100);
> >>>> + readw(base + RP2_GLOBAL_CMD);
> >>>
> >>> Since the assumed purpose of the readw() was to flush the writew(),
> >>> would it make sense to add a barrier after the writew()?
> >>
> >> AFAICT there is one which is implied within the name, as it is not a
> >> _relaxed() variant. Did you mean a different sort of barrier to be used?
> >
> > Not sure. The __raw_writew() is followed by __io_aw(), which is a
> > no-op on arm64. I don't know arm64 well enough to know if a follow-up
> > barrier is needed.
>
> By definition all of the {read,write}{b,w,l,q} do include an adequate
> barrier
I do see this in the kernel, e.g. altera_edac.c, pci-hyperv.c,
oxu210hp-hcd.c, etc:
write[lw](..);
wmb();
All I am saying is that the definition of writew() for arm64 has no
explicit barrier *after* it makes the __raw_writew() call, since its
__io_aw() call is a no-op. I really don't know if this matters, just
wanted to mention it.
Regards,
Jim
and perform the adequate endian swapping since they originated
> from PCI drivers on x86. If you do not want any barrier you would have
> to use the _relaxed variants, or if you want native ordering, you would
> use the __raw_* variant.
> --
> Florian
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges
2024-09-11 22:44 ` Jim Quinlan
@ 2024-09-12 0:19 ` Florian Fainelli
0 siblings, 0 replies; 10+ messages in thread
From: Florian Fainelli @ 2024-09-12 0:19 UTC (permalink / raw)
To: Jim Quinlan
Cc: linux-serial, Kevin Cernekee, Greg Kroah-Hartman, Jiri Slaby,
John Ogness, Ilpo Järvinen, Thomas Gleixner,
open list:TTY LAYER AND SERIAL DRIVERS
On 9/11/24 15:44, Jim Quinlan wrote:
> On Wed, Sep 11, 2024 at 6:19 PM Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
>>
>> On 9/11/24 15:16, Jim Quinlan wrote:
>>> On Wed, Sep 11, 2024 at 6:01 PM Florian Fainelli
>>> <florian.fainelli@broadcom.com> wrote:
>>>>
>>>> On 9/11/24 14:47, Jim Quinlan wrote:
>>>>> On Fri, Sep 6, 2024 at 6:54 PM Florian Fainelli
>>>>> <florian.fainelli@broadcom.com> wrote:
>>>>>>
>>>>>> The write to RP2_GLOBAL_CMD followed by an immediate read of
>>>>>> RP2_GLOBAL_CMD in rp2_reset_asic() is intented to flush out the write,
>>>>>> however by then the device is already in reset and cannot respond to a
>>>>>> memory cycle access.
>>>>>>
>>>>>> On platforms such as the Raspberry Pi 4 and others using the
>>>>>> pcie-brcmstb.c driver, any memory access to a device that cannot respond
>>>>>> is met with a fatal system error, rather than being substituted with all
>>>>>> 1s as is usually the case on PC platforms.
>>>>>>
>>>>>> Swapping the delay and the read ensures that the device has finished
>>>>>> resetting before we attempt to read from it.
>>>>>>
>>>>>> Fixes: 7d9f49afa451 ("serial: rp2: New driver for Comtrol RocketPort 2 cards")
>>>>>> Suggested-by: Jim Quinlan <james.quinlan@broadcom.com>
>>>>>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
>>>>>> ---
>>>>>> drivers/tty/serial/rp2.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/tty/serial/rp2.c b/drivers/tty/serial/rp2.c
>>>>>> index 4132fcff7d4e..8bab2aedc499 100644
>>>>>> --- a/drivers/tty/serial/rp2.c
>>>>>> +++ b/drivers/tty/serial/rp2.c
>>>>>> @@ -577,8 +577,8 @@ static void rp2_reset_asic(struct rp2_card *card, unsigned int asic_id)
>>>>>> u32 clk_cfg;
>>>>>>
>>>>>> writew(1, base + RP2_GLOBAL_CMD);
>>>>>> - readw(base + RP2_GLOBAL_CMD);
>>>>>> msleep(100);
>>>>>> + readw(base + RP2_GLOBAL_CMD);
>>>>>
>>>>> Since the assumed purpose of the readw() was to flush the writew(),
>>>>> would it make sense to add a barrier after the writew()?
>>>>
>>>> AFAICT there is one which is implied within the name, as it is not a
>>>> _relaxed() variant. Did you mean a different sort of barrier to be used?
>>>
>>> Not sure. The __raw_writew() is followed by __io_aw(), which is a
>>> no-op on arm64. I don't know arm64 well enough to know if a follow-up
>>> barrier is needed.
>>
>> By definition all of the {read,write}{b,w,l,q} do include an adequate
>> barrier
> I do see this in the kernel, e.g. altera_edac.c, pci-hyperv.c,
> oxu210hp-hcd.c, etc:
>
> write[lw](..);
> wmb();
>
> All I am saying is that the definition of writew() for arm64 has no
> explicit barrier *after* it makes the __raw_writew() call, since its
> __io_aw() call is a no-op. I really don't know if this matters, just
> wanted to mention it.
Not having the documentation for this peripheral, not sure TBH.
As far as the drivers you quoted given the __io_bw() does include a
barrier, it would appear that the barrier after is possibly redundant,
or is based upon a misunderstanding of which side of the write the
barrier must be on, or maybe they are actually necessary, but
undocumented as such..
--
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges
2024-09-06 22:54 [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges Florian Fainelli
2024-09-11 13:46 ` Greg Kroah-Hartman
2024-09-11 21:47 ` Jim Quinlan
@ 2024-09-23 9:44 ` Jiri Slaby
2024-09-26 22:50 ` Florian Fainelli
2 siblings, 1 reply; 10+ messages in thread
From: Jiri Slaby @ 2024-09-23 9:44 UTC (permalink / raw)
To: Florian Fainelli, linux-serial
Cc: Jim Quinlan, Kevin Cernekee, Greg Kroah-Hartman, John Ogness,
Ilpo Järvinen, Thomas Gleixner,
open list:TTY LAYER AND SERIAL DRIVERS
On 07. 09. 24, 0:54, Florian Fainelli wrote:
> The write to RP2_GLOBAL_CMD followed by an immediate read of
> RP2_GLOBAL_CMD in rp2_reset_asic() is intented to flush out the write,
> however by then the device is already in reset and cannot respond to a
> memory cycle access.
>
> On platforms such as the Raspberry Pi 4 and others using the
> pcie-brcmstb.c driver, any memory access to a device that cannot respond
> is met with a fatal system error, rather than being substituted with all
> 1s as is usually the case on PC platforms.
>
> Swapping the delay and the read ensures that the device has finished
> resetting before we attempt to read from it.
>
> Fixes: 7d9f49afa451 ("serial: rp2: New driver for Comtrol RocketPort 2 cards")
> Suggested-by: Jim Quinlan <james.quinlan@broadcom.com>
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
> drivers/tty/serial/rp2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/rp2.c b/drivers/tty/serial/rp2.c
> index 4132fcff7d4e..8bab2aedc499 100644
> --- a/drivers/tty/serial/rp2.c
> +++ b/drivers/tty/serial/rp2.c
> @@ -577,8 +577,8 @@ static void rp2_reset_asic(struct rp2_card *card, unsigned int asic_id)
> u32 clk_cfg;
>
> writew(1, base + RP2_GLOBAL_CMD);
> - readw(base + RP2_GLOBAL_CMD);
> msleep(100);
> + readw(base + RP2_GLOBAL_CMD);
The read was there to force PCI posting to really flush the write to the
device before the sleep (and not to post). How is this ensured now? (In
fact, instead of the move, you could have deleted it completely.)
Can you actually read another register which a resetting device would reply?
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges
2024-09-23 9:44 ` Jiri Slaby
@ 2024-09-26 22:50 ` Florian Fainelli
0 siblings, 0 replies; 10+ messages in thread
From: Florian Fainelli @ 2024-09-26 22:50 UTC (permalink / raw)
To: Jiri Slaby, linux-serial
Cc: Jim Quinlan, Kevin Cernekee, Greg Kroah-Hartman, John Ogness,
Ilpo Järvinen, Thomas Gleixner,
open list:TTY LAYER AND SERIAL DRIVERS
On 9/23/24 02:44, Jiri Slaby wrote:
> On 07. 09. 24, 0:54, Florian Fainelli wrote:
>> The write to RP2_GLOBAL_CMD followed by an immediate read of
>> RP2_GLOBAL_CMD in rp2_reset_asic() is intented to flush out the write,
>> however by then the device is already in reset and cannot respond to a
>> memory cycle access.
>>
>> On platforms such as the Raspberry Pi 4 and others using the
>> pcie-brcmstb.c driver, any memory access to a device that cannot respond
>> is met with a fatal system error, rather than being substituted with all
>> 1s as is usually the case on PC platforms.
>>
>> Swapping the delay and the read ensures that the device has finished
>> resetting before we attempt to read from it.
>>
>> Fixes: 7d9f49afa451 ("serial: rp2: New driver for Comtrol RocketPort 2
>> cards")
>> Suggested-by: Jim Quinlan <james.quinlan@broadcom.com>
>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> ---
>> drivers/tty/serial/rp2.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/rp2.c b/drivers/tty/serial/rp2.c
>> index 4132fcff7d4e..8bab2aedc499 100644
>> --- a/drivers/tty/serial/rp2.c
>> +++ b/drivers/tty/serial/rp2.c
>> @@ -577,8 +577,8 @@ static void rp2_reset_asic(struct rp2_card *card,
>> unsigned int asic_id)
>> u32 clk_cfg;
>> writew(1, base + RP2_GLOBAL_CMD);
>> - readw(base + RP2_GLOBAL_CMD);
>> msleep(100);
>> + readw(base + RP2_GLOBAL_CMD);
>
> The read was there to force PCI posting to really flush the write to the
> device before the sleep (and not to post). How is this ensured now? (In
> fact, instead of the move, you could have deleted it completely.)
>
> Can you actually read another register which a resetting device would
> reply?
Sure I can do that, give me a couple more days to get back to you.
--
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-09-26 22:51 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-06 22:54 [PATCH] tty: rp2: Fix reset with non forgiving PCIe host bridges Florian Fainelli
2024-09-11 13:46 ` Greg Kroah-Hartman
2024-09-11 21:47 ` Jim Quinlan
2024-09-11 22:01 ` Florian Fainelli
2024-09-11 22:16 ` Jim Quinlan
2024-09-11 22:19 ` Florian Fainelli
2024-09-11 22:44 ` Jim Quinlan
2024-09-12 0:19 ` Florian Fainelli
2024-09-23 9:44 ` Jiri Slaby
2024-09-26 22:50 ` Florian Fainelli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).