[PATCH 10/16] serial: sh: Add RZ/G2L SCIF support

Sat Oct 7 23:14:06 CEST 2023

On 10/5/23 14:08, Paul Barker wrote:
> On 04/10/2023 20:41, Marek Vasut wrote:
>> On 10/4/23 18:38, Paul Barker wrote:
>>> On Wed, Oct 04, 2023 at 05:17:55PM +0200, Marek Vasut wrote:
>>>> On 10/4/23 15:43, Paul Barker wrote:
>>>>> On Wed, Oct 04, 2023 at 02:26:49PM +0200, Marek Vasut wrote:
>>>>>> On 10/4/23 10:48, Paul Barker wrote:
>>>>>>> On 03/10/2023 14:23, Marek Vasut wrote:
>>>>>>>> On 9/20/23 14:42, Paul Barker wrote:
>>>>>>>>> Extend the existing driver to support the SCIF serial ports on the
>>>>>>>>> Renesas RZ/G2L (R9A07G044) SoC. This also requires us to ensure that the
>>>>>>>>> relevant reset signal is de-asserted before we try to talk to the SCIF
>>>>>>>>> module.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Paul Barker <paul.barker.ct at bp.renesas.com>
>>>>>>>>> Reviewed-by: Biju Das <biju.das.jz at bp.renesas.com>
>>>>>>>>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj at bp.renesas.com>
>>>>>>>>> ---
>>>>>>>>>       arch/arm/mach-rmobile/Kconfig |  1 +
>>>>>>>>>       drivers/serial/serial_sh.c    | 32 ++++++++++++++++++++++++++++++--
>>>>>>>>>       drivers/serial/serial_sh.h    | 19 ++++++++++++++++++-
>>>>>>>>>       3 files changed, 49 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/mach-rmobile/Kconfig b/arch/arm/mach-rmobile/Kconfig
>>>>>>>>> index 973e84fcf7ba..0ab22356aee5 100644
>>>>>>>>> --- a/arch/arm/mach-rmobile/Kconfig
>>>>>>>>> +++ b/arch/arm/mach-rmobile/Kconfig
>>>>>>>>> @@ -77,6 +77,7 @@ config RZG2L
>>>>>>>>>       	imply RENESAS_SDHI
>>>>>>>>>       	imply CLK_RZG2L
>>>>>>>>>       	imply PINCTRL_RZG2L
>>>>>>>>> +	imply SCIF_CONSOLE
>>>>>>>>>       	help
>>>>>>>>>       	  Enable support for the Renesas RZ/G2L family of SoCs, including the
>>>>>>>>>       	  the RZ/G2L itself (based on the R9A07G044 SoC).
>>>>>>>>> diff --git a/drivers/serial/serial_sh.c b/drivers/serial/serial_sh.c
>>>>>>>>> index 5e543dbf3d58..a2e9a57137a6 100644
>>>>>>>>> --- a/drivers/serial/serial_sh.c
>>>>>>>>> +++ b/drivers/serial/serial_sh.c
>>>>>>>>> @@ -17,6 +17,8 @@
>>>>>>>>>       #include <linux/compiler.h>
>>>>>>>>>       #include <dm/platform_data/serial_sh.h>
>>>>>>>>>       #include <linux/delay.h>
>>>>>>>>> +#include <dm/device_compat.h>
>>>>>>>>> +#include <reset.h>
>>>>>>>>>       #include "serial_sh.h"
>>>>>>>>>       DECLARE_GLOBAL_DATA_PTR;
>>>>>>>>> @@ -79,8 +81,16 @@ sh_serial_setbrg_generic(struct uart_port *port, int clk, int baudrate)
>>>>>>>>>       static void handle_error(struct uart_port *port)
>>>>>>>>>       {
>>>>>>>>> -	sci_in(port, SCxSR);
>>>>>>>>> -	sci_out(port, SCxSR, SCxSR_ERROR_CLEAR(port));
>>>>>>>>> +	/* The RZ/G2L datasheet says that error conditions are cleared by
>>>>>>>>> +	 * resetting the error bits in the FSR register to zero.
>>>>>>>>
>>>>>>>> Can you be more specific here ?
>>>>>>>>
>>>>>>>> It doesn't seem Linux sh-sci.c driver does anything special for G2L, so
>>>>>>>> is this special case really needed ?
>>>>>>>
>>>>>>> On page 1268 of the datasheet (R01UH0914EJ0130 Rev.1.30):
>>>>>>>
>>>>>>> "DR is cleared to 0 when DR = 1 is read and then 0 is written to the DR
>>>>>>> flag."
>>>>>>>
>>>>>>> On page 1270:
>>>>>>>
>>>>>>> "[Clearing condition]
>>>>>>> ● When 0 is written to ER after it has been read as 1"
>>>>>>>
>>>>>>> So zeros must be written to clear these errors, not ones.
>>>>>>>
>>>>>>> We have an open task to investigate the issue in the Linux driver and
>>>>>>> fix it.
>>>>>>
>>>>>> Is the G2L UART broken in Linux ?
>>>>>
>>>>> Likely yes, but we need to reproduce the issue.
>>>>>
>>>>>> Does it misbehave in U-Boot ?
>>>>>
>>>>> Yes, before I changed this, if a framing error occurred it could never
>>>>> be cleared and the serial port stopped sending/receiving data.
>>>>>
>>>>> The framing error was triggered by accident by unnecessarily re-doing
>>>>> pinmuxing for the serial port after TrustedFirmware had already set it
>>>>> up correctly. Since SCIF0 is wired to an FTDI USB/serial adaptor chip on
>>>>> the SMARC carrier board, it seems very difficult to trigger a framing
>>>>> error in any other way, the FTDI chip is too well behaved.
>>>>
>>>> Is it possible to trigger this on RZ/G2<non-L> with SCIF on those platforms
>>>> as well ?
>>>
>>> It's not been seen or reported to my knowledge, but it's on our list to
>>> look into further.
>>
>> Can you give it a quick try ? I'd really like to avoid G2L specific
>> behavior here if it can be avoided.
>>
>> Is there a testcase ?
> 
> 
> The case we're interested in here is the Receive Error (ER) & Break
> Detect (BRK) conditions. I've done some further datasheet digging...
> 
> RZ/G2L datasheet says these are cleared by writing zero to the
> appropriate bits of the FSR register.
> 
> RZ/G2{H,M,N,E} datasheet says the same (pages 50-18 & 50-19 of
> R01UH0808EJ0111 Rev.1.11).
> 
> The R-Car Gen3 Hardware Manual says the same (pages 55-18 & 55-19 of
> R19UH0105EJ0230 Rev.2.30).
> 
> For R-Car S4, there is an Excel spreadsheet attached to page 105-5 of
> the datasheet (R19UH0161EJ0100 Rev.1.00) which again says the same.
> 
> For R-Car V4H, the relevant info is on pages 105-16 & 105-18 of the
> datasheet (R19UH0172EJ0081 Rev.0.81) and says the same.
> 
> On the RZ/G2L I was able to reproduce a break condition by changing
> pinmux settings after the SCIF interface has been configured. You could
> try this out on an R-Car system, but I don't think it's guaranteed to
> cause a break condition, it may depend on how the port input is
> implemented in those SoCs. From my reading, the only guaranteed way to
> cause a break condition is to hold the Rx signal low, and you might need
> to solder on some extra wires on to a board to be able to do that.

Can't you simply send a BREAK from terminal program ?
In minicom, I think that's meta-F B .

> We should still fix the error handling here, even if the boards we have
> don't easily cause Receive Errors or Break conditions, other folks may
> have their own boards based on these SoCs.
> 
> If you're happy I'll go ahead and check IS_ENABLED(CONFIG_RCAR_64) here
> instead of IS_ENABLED(CONFIG_RZG2L).

Based on the input I am getting from you here, it seems to me we should 
just use the G2L case as the common case for all systems, without any 
special casing, right ?