[PATCH v5 2/2] serial: zynqmp: Fetch baudrate from dtb and update
Abbarapu, Venkatesh
venkatesh.abbarapu at amd.com
Wed Aug 16 11:30:15 CEST 2023
Hi Simon,
> -----Original Message-----
> From: Simon Glass <sjg at chromium.org>
> Sent: Tuesday, June 13, 2023 2:48 AM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>
> Cc: u-boot at lists.denx.de; Simek, Michal <michal.simek at amd.com>;
> git at xilinx.com; Algapally, Santosh Sagar <SantoshSagar.Algapally at amd.com>
> Subject: Re: [PATCH v5 2/2] serial: zynqmp: Fetch baudrate from dtb and
> update
>
> Hi Venkatesh,
>
> On Thu, 25 May 2023 at 05:03, Venkatesh Yadav Abbarapu
> <venkatesh.abbarapu at amd.com> wrote:
> >
> > From: Algapally Santosh Sagar <santoshsagar.algapally at 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.algapally at amd.com>
> > Signed-off-by: Venkatesh Yadav Abbarapu
> <venkatesh.abbarapu at amd.com>
> > ---
> > doc/README.serial_dt_baud | 41
> +++++++++++++++++++++++++++++++++
> > drivers/core/ofnode.c | 20 ++++++++++++++++
> > drivers/serial/Kconfig | 9 ++++++++
> > drivers/serial/serial-uclass.c | 42 ++++++++++++++++++++++++++++++++++
> > include/dm/ofnode.h | 14 ++++++++++--
> > include/env_default.h | 6 ++++-
> > include/serial.h | 15 ++++++++++++
> > 7 files changed, 144 insertions(+), 3 deletions(-) create mode
> > 100644 doc/README.serial_dt_baud
> >
> > diff --git a/doc/README.serial_dt_baud b/doc/README.serial_dt_baud
> new
> > file mode 100644 index 0000000000..02974ab1a7
> > --- /dev/null
> > +++ b/doc/README.serial_dt_baud
> > @@ -0,0 +1,41 @@
> > +Fetch serial baudrate from DT
> > +-----------------------------
> > +
> > +To support fetching of baudrate from DT, the following is done:-
> > +
> > +The baudrate configured in Kconfig symbol CONFIG_BAUDRATE is taken by
> default by serial.
> > +If change of baudrate is required then the Kconfig symbol
> > +CONFIG_BAUDRATE needs to changed and U-Boot recompilation is
> required or the U-Boot environment needs to be updated.
> > +
> > +To avoid this, add support to fetch the baudrate directly from the
> > +device tree file and update the environment.
> > +
> > +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 which is enabled by SERIAL_DT_BAUD.
>
> The baud rate is read from the env into gd->baudrate by init_baud_rate().
>
> Updating the default environment seems pretty nasty to me...why is that
> necessary?
>
> Could we read it from DT in init_baud_rate() ?
We are seeing glitches if the default env baudrate and dtb baudrate are not same. So, we are updating the default baudrate with dtb baudrate, same thing has been explained above.
>
> > +
> > +The Kconfig SPL_ENV_SUPPORT needs to be enabled to allow patching in
> SPL.
> > +
> > +The Kconfig DEFAULT_ENV_IS_RW which is enabled by SERIAL_DT_BAUD
> with
> > +making the environment writable.
> > +
> > +The ofnode_read_baud() function parses and fetches the baudrate value
> > +from the DT. This value is validated and updated to baudrate during
> > +serial init. Padding is added at the end of the default environment and the
> dt baudrate is updated with the latest value.
> > +
> > +Example:-
> > +
> > +The serial port options are of the form "bbbbpnf", where "bbbb" is
> > +the baud rate, "p" is parity ("n", "o", or "e"), "n" is number of bits, and "f"
> is flow control ("r" for RTS or omit it). Default is "115200n8".
> > +
> > +chosen {
> > + bootargs = "earlycon console=ttyPS0,115200 clk_ignore_unused
> root=/dev/ram0 rw init_fatal_sh=1";
> > + stdout-path = "serial0:115200n8";
> > + };
> > +
> > +From the chosen node, stdout-path property is obtained as string.
> > +
> > + stdout-path = "serial0:115200n8";
> > +
> > +The string is parsed to get the baudrate 115200. This string is converted to
> integer and updated to the environment.
> > diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index
> > ec574c4460..04bdb30b24 100644
> > --- a/drivers/core/ofnode.c
> > +++ b/drivers/core/ofnode.c
> > @@ -870,6 +870,26 @@ ofnode ofnode_get_chosen_node(const char
> *name)
> > return ofnode_path(prop);
> > }
> >
> > +#ifdef CONFIG_OF_SERIAL_DT_BAUD
>
> You should not need this #ifdef
Ok...I will remove this and update.
>
> > +int ofnode_read_baud(void)
> > +{
> > + const char *str, *p;
> > + u32 baud;
> > +
> > + str = ofnode_read_chosen_string("stdout-path");
> > + if (!str)
> > + return -EINVAL;
> > +
> > + /* Parse string serial0:115200n8 */
> > + p = strchr(str, ':');
> > + if (!p)
> > + return -EINVAL;
> > +
> > + baud = dectoul(p + 1, NULL);
> > + return baud;
> > +}
> > +#endif
> > +
> > const void *ofnode_read_aliases_prop(const char *propname, int
> > *sizep) {
> > ofnode node;
> > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index
> > 5c9b924e73..ea2244e5db 100644
> > --- a/drivers/serial/Kconfig
> > +++ b/drivers/serial/Kconfig
> > @@ -24,6 +24,15 @@ config BAUDRATE
> > in the SPL stage (most drivers) or for choosing a default baudrate
> > in the absence of an environment setting (serial_mxc.c).
> >
> > +config OF_SERIAL_DT_BAUD
>
> Please can we use OF_SERIAL_BAUD ?
>
> OF sort-of means DT so we don't need both
Ok...sure will change to OF_SERIAL_BAUD.
>
> > + bool "Fetch serial baudrate from device tree"
> > + depends on DM_SERIAL && SPL_ENV_SUPPORT
> > + select DEFAULT_ENV_IS_RW
> > + help
> > + Select this to enable fetching and setting of the baudrate
> > + configured in the DT. Replace the default baudrate with the DT
> > + baudrate and also set it to the environment.
> > +
> > config DEFAULT_ENV_IS_RW
> > bool "Make default environment as writable"
> > help
> > diff --git a/drivers/serial/serial-uclass.c
> > b/drivers/serial/serial-uclass.c index 067fae2614..afa25a1f19 100644
> > --- a/drivers/serial/serial-uclass.c
> > +++ b/drivers/serial/serial-uclass.c
> > @@ -154,12 +154,54 @@ static void serial_find_console_or_panic(void)
> > }
> > #endif /* CONFIG_SERIAL_PRESENT */
> >
> > +int check_valid_baudrate(int baud)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(baudrate_table); ++i) {
> > + if (baud == baudrate_table[i])
> > + return 0;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +int fetch_baud_from_dtb(void)
> > +{
> > + int baud_value, ret;
> > +
> > + baud_value = ofnode_read_baud();
> > + ret = check_valid_baudrate(baud_value);
> > + if (ret)
> > + return ret;
> > +
> > + return baud_value;
> > +}
> > +
> > /* 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_OF_SERIAL_DT_BAUD
>
> Can this use
>
> if (IS_ENABLED(...)) ...
>
> ?
Ok...will update to IS_ENABLED().
>
> > + int ret = 0;
> > + char *ptr = &default_environment[0];
> > +
> > + /*
> > + * Fetch the baudrate from the dtb and update the value in the
> > + * default environment.
> > + */
> > + ret = fetch_baud_from_dtb();
> > + 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/dm/ofnode.h b/include/dm/ofnode.h index
> > 443db6252d..5f90b6c65e 100644
> > --- a/include/dm/ofnode.h
> > +++ b/include/dm/ofnode.h
> > @@ -932,12 +932,22 @@ const char *ofnode_read_chosen_string(const
> char
> > *propname); ofnode ofnode_get_chosen_node(const char *propname);
> >
> > /**
> > - * ofnode_read_aliases_prop() - get the value of a aliases property
> > + * ofnode_read_baud() - get the baudrate from string value of chosen
> > + property
> > *
> > - * This looks for a property within the /aliases node and returns its
> > value
> > + * This looks for stdout-path property within the /chosen node and
> > + parses its
> > + * value to return baudrate.
> > *
> > * This only works with the control FDT.
> > *
> > + * Return: baudrate value if found, else error
>
> -ve error code?
Okk...Will update this.
>
> > + */
> > +int ofnode_read_baud(void);
> > +
> > +/**
> > + * ofnode_read_aliases_prop() - get the value of a aliases property
> > + *
> > + * This looks for a property within the /aliases node and returns its
> > +value
> > + *
> > * @propname: Property name to look for
> > * @sizep: Returns size of property, or `FDT_ERR_...` error code if function
> > * returns NULL
> > diff --git a/include/env_default.h b/include/env_default.h index
> > 227cad7c34..1c859a8f65 100644
> > --- a/include/env_default.h
> > +++ b/include/env_default.h
> > @@ -42,7 +42,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_OF_SERIAL_DT_BAUD) &&
> defined(CONFIG_BAUDRATE) &&
> > +(CONFIG_BAUDRATE >= 0)
> > "baudrate=" __stringify(CONFIG_BAUDRATE) "\0"
> > #endif
> > #ifdef CONFIG_LOADS_ECHO
> > @@ -118,6 +118,10 @@ const char default_environment[] = { #endif
> > #ifdef CFG_EXTRA_ENV_SETTINGS
> > CFG_EXTRA_ENV_SETTINGS
> > +#endif
> > +#ifdef CONFIG_OF_SERIAL_DT_BAUD
> > + /* Padding for baudrate at the end when environment is writable */
> > + "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"
> > #endif
> > "\0"
> > #else /* CONFIG_USE_DEFAULT_ENV_FILE */ diff --git a/include/serial.h
> > b/include/serial.h index 42bdf3759c..b01047d07a 100644
> > --- a/include/serial.h
> > +++ b/include/serial.h
> > @@ -337,6 +337,21 @@ int serial_setconfig(struct udevice *dev, uint
> config);
> > */
> > int serial_getinfo(struct udevice *dev, struct serial_device_info
> > *info);
> >
> > +/**
> > + * check_valid_baudrate() - Check whether baudrate is valid or not
> > + *
> > + * @baud: baudrate
>
> baud rate to check
Ok...will update this.
>
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int check_valid_baudrate(int baud);
> > +
> > +/**
> > + * fetch_baud_from_dtb() - Fetch the baudrate value from DT
> > + *
> > + * Return: baudrate if OK, -ve on error */ int
> > +fetch_baud_from_dtb(void);
> > +
> > void atmel_serial_initialize(void);
> > void mcf_serial_initialize(void);
> > void mpc85xx_serial_initialize(void);
> > --
> > 2.17.1
> >
>
> Regards,
> Simon
Thanks
Venkatesh
More information about the U-Boot
mailing list