Hi Vladimir, On Wed, 12 Apr 2023 at 06:45, Vladimir Zapolskiy <vladimir.zapols...@linaro.org> wrote: > > On 3/31/23 04:23, Konrad Dybcio wrote: > > > > > > On 30.03.2023 21:47, Vladimir Zapolskiy wrote: > >> This change adds a Qualcomm GENI SE QUP device driver as a wrapper for > >> actually enabled and used serial devices found on a board. > >> > >> At the moment the driver is pretty simple, its intention is to populate > >> childred devices and provide I/O mem read interface to them as clients, > >> this is needed for GENI UART driver to set up a proper clock divider > >> and provide the actually asked baud rate. > >> > >> Signed-off-by: Vladimir Zapolskiy <vladimir.zapols...@linaro.org> > >> --- > >> drivers/misc/Kconfig | 6 ++++++ > >> drivers/misc/Makefile | 1 + > >> drivers/misc/qcom-geni-se.c | 42 +++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 49 insertions(+) > >> create mode 100644 drivers/misc/qcom-geni-se.c > >> > >> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > >> index b5707a15c504..348e1ab407ad 100644 > >> --- a/drivers/misc/Kconfig > >> +++ b/drivers/misc/Kconfig > >> @@ -511,6 +511,12 @@ config WINBOND_W83627 > >> legacy UART or other devices in the Winbond Super IO chips > >> on X86 platforms. > >> > >> +config QCOM_GENI_SE > >> + bool "Qualcomm GENI Serial Engine Driver" > >> + help > >> + The driver manages Generic Interface (GENI) firmware based > >> + Qualcomm Technologies, Inc. Universal Peripheral (QUP) Wrapper. > >> + > >> config QFW > >> bool > >> help > >> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > >> index 3b792f2a14ce..52aed096021f 100644 > >> --- a/drivers/misc/Makefile > >> +++ b/drivers/misc/Makefile > >> @@ -60,6 +60,7 @@ obj-$(CONFIG_NUVOTON_NCT6102D) += nuvoton_nct6102d.o > >> obj-$(CONFIG_P2SB) += p2sb-uclass.o > >> obj-$(CONFIG_PCA9551_LED) += pca9551_led.o > >> obj-$(CONFIG_$(SPL_)PWRSEQ) += pwrseq-uclass.o > >> +obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o > >> ifdef CONFIG_QFW > >> obj-y += qfw.o > >> obj-$(CONFIG_QFW_PIO) += qfw_pio.o > >> diff --git a/drivers/misc/qcom-geni-se.c b/drivers/misc/qcom-geni-se.c > >> new file mode 100644 > >> index 000000000000..4f1775b11f62 > >> --- /dev/null > >> +++ b/drivers/misc/qcom-geni-se.c > >> @@ -0,0 +1,42 @@ > >> +// SPDX-License-Identifier: GPL-2.0+ > >> +/* > >> + * Qualcomm Generic Interface (GENI) Serial Engine (SE) Wrapper > >> + * > >> + * (C) Copyright 2023 Vladimir Zapolskiy <vladimir.zapols...@linaro.org> > >> + */ > >> + > >> +#include <common.h> > >> +#include <dm.h> > >> +#include <misc.h> > >> +#include <asm/io.h> > >> + > >> +static int geni_se_qup_read(struct udevice *dev, int offset, > >> + void *buf, int size) > >> +{ > >> + fdt_addr_t base = dev_read_addr(dev); > >> + > >> + if (size != sizeof(u32)) > >> + return -EINVAL; > >> + > >> + *(u32 *)buf = readl(base + offset); > > Maybe > > > > u32 *buffer = buf; > > > > [...] > > > > *buf = readl.. > > Well, I'd rather keep it as is, since it's one code of line less. > > > would be more stylish, but that's a nit and I don't have > > any other complaints! :D > > In fact there is a minor issue with the driver, namely after some > testing I have to remove '.bind = dm_scan_fdt_dev' from the ops, > since it's already done on an upper level. > > > Reviewed-by: Konrad Dybcio <konrad.dyb...@linaro.org> > > > > Thanks for review! > > Best wishes, > Vladimir > > >> + > >> + return 0; > >> +} > >> + > >> +static struct misc_ops geni_se_qup_ops = { > >> + .read = geni_se_qup_read, > >> +}; > >> + > >> +static const struct udevice_id geni_se_qup_ids[] = { > >> + { .compatible = "qcom,geni-se-qup" }, > >> + {} > >> +}; > >> + > >> +U_BOOT_DRIVER(geni_se_qup) = { > >> + .name = "geni_se_qup", > >> + .id = UCLASS_MISC, > >> + .of_match = geni_se_qup_ids, > >> + .bind = dm_scan_fdt_dev, > >> + .ops = &geni_se_qup_ops, > >> + .flags = DM_FLAG_PRE_RELOC, > >> +};
Please note that misc_read() should return the number of bytes read, not 0. Regards, Simon