Message ID | 20230314084627.26358-1-venkatesh.abbarapu@amd.com |
---|---|
State | Changes Requested |
Delegated to: | Tom Rini |
Headers | show |
Series | serial: zynqmp: Fetch baudrate from dtb and update | expand |
Hi Simon and Tom, On 3/14/23 09:46, Venkatesh Yadav Abbarapu wrote: > From: Algapally Santosh Sagar <santoshsagar.algapally@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@amd.com> > Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com> I reviewed this patch internally and would like to get your opinion about it. The problematic part is around ENV_RW_FILLER that's why I want to make sure that the patch is going in the right direction. Thanks, Michal
On Tue, Mar 14, 2023 at 02:16:27PM +0530, Venkatesh Yadav Abbarapu wrote: > From: Algapally Santosh Sagar <santoshsagar.algapally@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@amd.com> > Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com> > --- > drivers/serial/Kconfig | 8 +++++++ > drivers/serial/serial-uclass.c | 32 ++++++++++++++++++++++++++ > include/configs/xilinx_zynqmp.h | 10 ++++++++- > include/fdtdec.h | 8 +++++++ > include/serial.h | 1 + > lib/fdtdec.c | 40 +++++++++++++++++++++++++++++++++ > 6 files changed, 98 insertions(+), 1 deletion(-) Conceptually? OK. Implementation? So, this relies on DEFAULT_ENV_IS_RW but that's not in Kconfig. I think it's an easy enough option to move to Kconfig and select when needed (first by armada-37xx platforms, then when SERIAL_DT_BAUD is set). Then: > diff --git a/include/configs/xilinx_zynqmp.h b/include/configs/xilinx_zynqmp.h > index 011f0034c5..79d0a2a214 100644 > --- a/include/configs/xilinx_zynqmp.h > +++ b/include/configs/xilinx_zynqmp.h > @@ -15,6 +15,13 @@ > #define GICC_BASE 0xF9020000 > > /* Serial setup */ > +#if CONFIG_IS_ENABLED(SERIAL_DT_BAUD) This would end up not patching in SPL, so just #ifdef. > +#define DEFAULT_ENV_IS_RW > +#define ENV_RW_FILLER "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" Isn't this two too short? baudrate=115200 | wc -c 16 And that doesn't include that you need a null at the end. And, since this is a generic option, we need to go about modifying include/env_default.h. Given that BAUDRATE is an option, I think we should go about patching the baudrate entry, and make it be something like: #if defined(CONFIG_BAUDRATE) && (CONFIG_BAUDRATE >= 0) "baudrate=" __stringify(CONFIG_BAUDRATE) "\0" #ifdef CONFIG_SERIAL_DT_BAUD ... more padding, so that we can extend it up to 8000000 ... #endif #endif
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index bb5083201b..0a1bcd63d5 100644 --- a/drivers/serial/Kconfig +++ b/drivers/serial/Kconfig @@ -24,6 +24,14 @@ 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 SERIAL_DT_BAUD + bool "Fetch serial baudrate from device tree" + depends on DM_SERIAL + 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 REQUIRE_SERIAL_CONSOLE bool "Require a serial port for console" # Running without a serial console is not supported by the diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 77d3f37372..e41cccfdc2 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -158,12 +158,44 @@ static void serial_find_console_or_panic(void) } #endif /* CONFIG_SERIAL_PRESENT */ +#if CONFIG_IS_ENABLED(SERIAL_DT_BAUD) +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; +#if CONFIG_IS_ENABLED(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/configs/xilinx_zynqmp.h b/include/configs/xilinx_zynqmp.h index 011f0034c5..79d0a2a214 100644 --- a/include/configs/xilinx_zynqmp.h +++ b/include/configs/xilinx_zynqmp.h @@ -15,6 +15,13 @@ #define GICC_BASE 0xF9020000 /* Serial setup */ +#if CONFIG_IS_ENABLED(SERIAL_DT_BAUD) +#define DEFAULT_ENV_IS_RW +#define ENV_RW_FILLER "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" +#else +#define ENV_RW_FILLER +#endif + #define CFG_SYS_BAUDRATE_TABLE \ { 4800, 9600, 19200, 38400, 57600, 115200 } @@ -176,7 +183,8 @@ #ifndef CFG_EXTRA_ENV_SETTINGS #define CFG_EXTRA_ENV_SETTINGS \ ENV_MEM_LAYOUT_SETTINGS \ - BOOTENV + BOOTENV \ + ENV_RW_FILLER #endif /* SPL can't handle all huge variables - define just DFU */ 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); +/** + * Get baudrate from the dtb + * + * @param blob Device tree blob (if NULL, then error is returned) + * @return Baud rate value, or -ve error . + */ +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); 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..fa24659b32 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; } +#if CONFIG_IS_ENABLED(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; + 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; + + return baud; +} +#endif + const char *fdtdec_get_chosen_prop(const void *blob, const char *name) { int chosen_node;