Hi Simon,
Thanks,
A Santosh Sagar.
> -----Original Message-----
> From: Simon Glass <s...@chromium.org>
> Sent: Tuesday, April 25, 2023 11:31 PM
> To: Abbarapu, Venkatesh <venkatesh.abbar...@amd.com>
> Cc: u-boot@lists.denx.de; Simek, Michal <michal.si...@amd.com>;
> g...@xilinx.com; Algapally, Santosh Sagar <santoshsagar.algapa...@amd.com>
> Subject: Re: [PATCH v3 1/2] serial: zynqmp: Fetch baudrate from dtb and update
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> 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?
Ok sure
>
> >
> > 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
Will drop
>
> > +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)
On using this
if (IS_ENABLED(CONFIG_SERIAL_DT_BAUD))
The variables declared below are getting compiled and the below warning is
thrown. So, not changing this.
drivers/serial/serial-uclass.c: In function 'serial_init':
drivers/serial/serial-uclass.c:189:21: warning: initialization discards 'const'
qualifier from pointer target type [-Wdiscarded-qualifiers]
189 | char *ptr = &default_environment[0];
>
> You also should add a Kconfig option. I suggest naming it OF_SERIAL_BOARD
> since we typically put an OF_ prefix on such options.
>
Will change SERIAL_DT_BAUD to OF_SERIAL_DT_BAUD
> > + 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?
The padding is needed at the end of default environment for baudrate when
environment is writable.
>
> > #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?
This is done as DEFAULT_ENV_IS_RW is moved to Kconfig.
>
> > 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.
Will change
>
> >
> > +/**
> > + * 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
>
Will do
> > + */
> > +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
Ok sure
> > 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);
>
> ...
>
> }
Will do
> > + 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)
>
Ok sure
> > +
> > + return baud;
> > +}
> > +#endif
> > +
> > const char *fdtdec_get_chosen_prop(const void *blob, const char
> > *name) {
> > int chosen_node;
> > --
> > 2.17.1
> >
>
> Regards,
> Simon
Will do the changes and send v4
