Re: [PATCH v3 05/10] litex serial: add setbrg callback

From: Ahmad Fatoum <a.fatoum@pengutronix.de>
To: Antony Pavlov <antonynpavlov@gmail.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Marek Czerski <m.czerski@ap-tech.pl>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH v3 05/10] litex serial: add setbrg callback
Date: Tue, 25 May 2021 14:48:52 +0200	[thread overview]
Message-ID: <2cea0c51-5979-a0b6-0f93-2b19eec1af4e@pengutronix.de> (raw)
In-Reply-To: <20210525103647.b4c08389183128e7abea97a0@gmail.com>

Hello Antony,

On 25.05.21 09:36, Antony Pavlov wrote:
> On Tue, 25 May 2021 10:19:47 +0300
> Antony Pavlov <antonynpavlov@gmail.com> wrote:
> 
> Hi all!
> 
> 
>> From: Marek Czerski <m.czerski@ap-tech.pl>
>>
>> setbrg callback (set baudrate) is needed by the loadx/loady commands.
>> Because litex serial has fixed baudrate the callback only checks if
>> the requested baudrate is the same as the CONFIG_BAUDRATE.
>> ---
>>  drivers/serial/serial_litex.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/serial/serial_litex.c b/drivers/serial/serial_litex.c
>> index 8562a45ecc..9d35a6e44a 100644
>> --- a/drivers/serial/serial_litex.c
>> +++ b/drivers/serial/serial_litex.c
>> @@ -58,6 +58,13 @@ static int litex_serial_tstc(struct console_device *cdev)
>>  	return !litex_serial_readb(cdev, UART_RXEMPTY);
>>  }
>>  
>> +static int litex_setial_setbaudrate(struct console_device *cdev, int baudrate)
>> +{
>> +	if (baudrate != CONFIG_BAUDRATE)
>> +		return -EINVAL;
>> +	return 0;
>> +}
>> +
> 
> I have sent this patch separately because it need special attention.
> 
> LiteX serial port hardware has fixed baudrate and setbaudrate has no sence.
> On the other hand absent setnbaudrate() litex serial makes impossible 
> to use Y-modem data trasfer.
> 
> I don't like CONFIG_BAUDRATE here. Can we use 
> 
> 	if (baudrate != cdev->baudrate)
> 		return -EINVAL;
> 	return 0;
> 
> instead?
> 
> Please comment!

There are other consoles (efi, linux, virtio) that don't support baud rate
setting either, so I think it makes sense to solve this outside of the
individual console drivers.

One way to go about it is move

if (cdev->baudrate == baudrate)

        return 0;

in common/console.c up. Then setting baud rate to the preconfigured value
would succeed everywhere.

> To: Ahmad
> It looks like CONFIG_BAUDRATE is a one more global defconfig parameter
> that complicates "one defconfig for all RISC-V boards" approach.

It's not a deal breaker, users can still use the environment for fine-grained
configuration of the baud rate on a per board basis if they indeed want to
ship the same barebox configuration for LiteX and something else.

The idea of one-defconfig-for-all is that we don't introduce unneeded mutually-
exclusive configurations. This is not the case here.

(I know I still owe you some patches, I will try to get those sent out soon-ish)

Cheers,
Ahmad

> 
> P.S. There is typo in litex_seTial_setbaudrate name. I have noted just now.
> It should be fixed of cause.
> 
> 
>>  static int litex_serial_probe(struct device_d *dev)
>>  {
>>  	struct resource *iores;
>> @@ -73,7 +80,7 @@ static int litex_serial_probe(struct device_d *dev)
>>  	cdev->tstc = &litex_serial_tstc;
>>  	cdev->putc = &litex_serial_putc;
>>  	cdev->getc = &litex_serial_getc;
>> -	cdev->setbrg = NULL;
>> +	cdev->setbrg = &litex_setial_setbaudrate;
>>  
>>  	console_register(cdev);
>>  
>> -- 
>> 2.31.1
>>
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox