Hi Venkatesh,
On Tue, 25 Apr 2023 at 06:05, Venkatesh Yadav Abbarapu
<venkatesh.abbar...@amd.com> wrote:
>
> From: Algapally Santosh Sagar <santoshsagar.algapa...@amd.com>
>
> The baudrate configured in .config is taken by default by serial. If
> change of baudrate is required then the .config needs to changed and
> u-boot recompilation is required or the u-boot environment needs to be
> updated.
>
> To avoid this, support is added to fetch the baudrate directly from the
> device tree file and update.
> The serial, prints the log with the configured baudrate in the dtb.
> The commit c4df0f6f315c ("arm: mvebu: Espressobin: Set default value for
> $fdtfile env variable") is taken as reference for changing the default
> environment variable.
>
> The default environment stores the default baudrate value, When default
> baudrate and dtb baudrate are not same glitches are seen on the serial.
> So, the environment also needs to be updated with the dtb baudrate to
> avoid the glitches on the serial.
>
> Signed-off-by: Algapally Santosh Sagar <santoshsagar.algapa...@amd.com>
> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbar...@amd.com>
> ---
> drivers/serial/serial-uclass.c | 32 +++++++++++++++++++++++++++
> include/env_default.h | 7 ++++--
> include/env_internal.h | 2 +-
> include/fdtdec.h | 8 +++++++
> include/serial.h | 1 +
> lib/fdtdec.c | 40 ++++++++++++++++++++++++++++++++++
> 6 files changed, 87 insertions(+), 3 deletions(-)
Can you please add something to doc/ for this feature?
>
> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index 067fae2614..d77d3bda36 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -154,12 +154,44 @@ static void serial_find_console_or_panic(void)
> }
> #endif /* CONFIG_SERIAL_PRESENT */
>
> +#ifdef CONFIG_SERIAL_DT_BAUD
Please drop this #ifdef
> +int serial_get_valid_baudrate(int baud)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(baudrate_table); ++i) {
> + if (baud == baudrate_table[i])
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +#endif
> +
> /* Called prior to relocation */
> int serial_init(void)
> {
> #if CONFIG_IS_ENABLED(SERIAL_PRESENT)
> serial_find_console_or_panic();
> gd->flags |= GD_FLG_SERIAL_READY;
> +#ifdef CONFIG_SERIAL_DT_BAUD
if (IS_ENABLED(CONFIG_SERIAL_DT_BAUD)
You also should add a Kconfig option. I suggest naming it
OF_SERIAL_BOARD since we typically put an OF_ prefix on such options.
> + int ret = 0;
> + char *ptr = &default_environment[0];
> +
> + /*
> + * Fetch the baudrate from the dtb and update the value in the
> + * default environment.
> + */
> + ret = fdtdec_get_baud_from_dtb(gd->fdt_blob);
> + if (ret != -EINVAL && ret != -EFAULT) {
> + gd->baudrate = ret;
> +
> + while (*ptr != '\0' && *(ptr + 1) != '\0')
> + ptr++;
> + ptr += 2;
> + sprintf(ptr, "baudrate=%d", gd->baudrate);
> + }
> +#endif
> serial_setbrg();
> #endif
>
> diff --git a/include/env_default.h b/include/env_default.h
> index c0df39d62f..4f286ffc9e 100644
> --- a/include/env_default.h
> +++ b/include/env_default.h
> @@ -23,7 +23,7 @@ env_t embedded_environment
> __UBOOT_ENV_SECTION__(environment) = {
> {
> #elif defined(DEFAULT_ENV_INSTANCE_STATIC)
> static char default_environment[] = {
> -#elif defined(DEFAULT_ENV_IS_RW)
> +#elif defined(CONFIG_DEFAULT_ENV_IS_RW)
> char default_environment[] = {
> #else
> const char default_environment[] = {
> @@ -44,7 +44,7 @@ const char default_environment[] = {
> #if defined(CONFIG_BOOTDELAY)
> "bootdelay=" __stringify(CONFIG_BOOTDELAY) "\0"
> #endif
> -#if defined(CONFIG_BAUDRATE) && (CONFIG_BAUDRATE >= 0)
> +#if !defined(CONFIG_SERIAL_DT_BAUD) && defined(CONFIG_BAUDRATE) &&
> (CONFIG_BAUDRATE >= 0)
> "baudrate=" __stringify(CONFIG_BAUDRATE) "\0"
> #endif
> #ifdef CONFIG_LOADS_ECHO
> @@ -120,6 +120,9 @@ const char default_environment[] = {
> #endif
> #ifdef CFG_EXTRA_ENV_SETTINGS
> CFG_EXTRA_ENV_SETTINGS
> +#endif
> +#ifdef CONFIG_SERIAL_DT_BAUD
> + "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"
What is this for?
> #endif
> "\0"
> #else /* CONFIG_USE_DEFAULT_ENV_FILE */
> diff --git a/include/env_internal.h b/include/env_internal.h
> index 6a69494646..fcb464263f 100644
> --- a/include/env_internal.h
> +++ b/include/env_internal.h
> @@ -89,7 +89,7 @@ typedef struct environment_s {
> extern env_t embedded_environment;
> #endif /* ENV_IS_EMBEDDED */
>
> -#ifdef DEFAULT_ENV_IS_RW
> +#ifdef CONFIG_DEFAULT_ENV_IS_RW
What is this change for?
> extern char default_environment[];
> #else
> extern const char default_environment[];
> diff --git a/include/fdtdec.h b/include/fdtdec.h
> index aa61a0fca1..48937a7a46 100644
> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
> @@ -657,6 +657,14 @@ int fdtdec_get_alias_seq(const void *blob, const char
> *base, int node,
> */
> int fdtdec_get_alias_highest_id(const void *blob, const char *base);
Could you make this an ofnode_...() function instead? Then you can
call ofnode_read_chosen_string() from your function.
We are trying to drop the fdtdec API.
>
> +/**
> + * Get baudrate from the dtb
> + *
> + * @param blob Device tree blob (if NULL, then error is returned)
> + * @return Baud rate value, or -ve error .
Return: Baud ...
(we changed to that syntax to work with Sphinx)
But this should become ofnode_read_baud(void)
Exported functions should be documented in the header file, not the C file
> + */
> +int fdtdec_get_baud_from_dtb(const void *blob);
> +
> /**
> * Get a property from the /chosen node
> *
> diff --git a/include/serial.h b/include/serial.h
> index 42bdf3759c..48834b517c 100644
> --- a/include/serial.h
> +++ b/include/serial.h
> @@ -337,6 +337,7 @@ int serial_setconfig(struct udevice *dev, uint config);
> */
> int serial_getinfo(struct udevice *dev, struct serial_device_info *info);
>
> +int serial_get_valid_baudrate(int baud);
check_valid_baudrate() would be better since it is checking the
provided rate, not returning a valid rate (as I understand it).
This needs function docs
> void atmel_serial_initialize(void);
> void mcf_serial_initialize(void);
> void mpc85xx_serial_initialize(void);
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 0827e16859..2fcc12148e 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -570,6 +570,46 @@ int fdtdec_get_alias_highest_id(const void *blob, const
> char *base)
> return max;
> }
>
> +#ifdef CONFIG_SERIAL_DT_BAUD
> +int fdtdec_get_baud_from_dtb(const void *blob)
> +{
> + const char *str, *p, *baud_start;
> + u32 baud;
> +
> + if (!blob)
> + return -EFAULT;
> +
> + str = fdtdec_get_chosen_prop(blob, "stdout-path");
> + if (!str)
> + return -EINVAL;
> +
> + p = strchr(str, ':');
> + if (!p)
> + return -EINVAL;
> +
> + baud = 0;
> + baud_start = p + 1;
It would help to have a comment about the format you are parsing, e.g.
an example string.
I thought it was something like: "serial0:115200n8" in which case I
think you could do:
p = strchr(str, ':')
if (p) {
uint baud = dectoul(p + 1, NULL);
...
}
> + while (*baud_start != '\0') {
> + /*
> + * Uart binding is <baud>{<parity>{<bits>{...}}}
> + * So the baudrate either is the whole string, or
> + * ends in the parity characters.
> + */
> + if (*baud_start == 'n' || *baud_start == 'o' ||
> + *baud_start == 'e')
> + break;
> +
> + baud = baud * 10 + (*baud_start - '0');
> + baud_start++;
> + }
> +
> + if (serial_get_valid_baudrate(baud) == -EINVAL)
> + return -EINVAL;
ret = func ...
if (ret)
return ret
(i.e. don't change the error number, and catch all errors that it might return)
> +
> + return baud;
> +}
> +#endif
> +
> const char *fdtdec_get_chosen_prop(const void *blob, const char *name)
> {
> int chosen_node;
> --
> 2.17.1
>
Regards,
Simon
