Re: [PATCH 1/6] misc: add Qualcomm GENI SE QUP device driver

2023-04-19 Thread Vladimir Zapolskiy 

Hi Simon,

On 4/19/23 04:45, Simon Glass wrote:

Hi Vladimir,

On Wed, 12 Apr 2023 at 06:45, Vladimir Zapolskiy
 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 
---
   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 ..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 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+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 



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 = _se_qup_ops,
+.flags  = DM_FLAG_PRE_RELOC,
+};


Please note that misc_read() should return the number of bytes read, not 0.


oh, definitely this should be fixed, and consequently it requires v3 to be sent.

Thank you for review!

--
Best wishes,
Vladimir

Re: [PATCH 1/6] misc: add Qualcomm GENI SE QUP device driver

2023-04-18 Thread Simon Glass 
Hi Vladimir,

On Wed, 12 Apr 2023 at 06:45, Vladimir Zapolskiy
 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 
> >> ---
> >>   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 ..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 
> >> + */
> >> +
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +
> >> +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 
> >
>
> 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 = _se_qup_ops,
> >> +.flags  = DM_FLAG_PRE_RELOC,
> >> +};

Please note that misc_read() should return the number of bytes read, not 0.

Regards,
Simon

Re: [PATCH 1/6] misc: add Qualcomm GENI SE QUP device driver

2023-04-12 Thread Vladimir Zapolskiy 

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 
---
  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 ..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 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+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 



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 = _se_qup_ops,
+   .flags  = DM_FLAG_PRE_RELOC,
+};

Re: [PATCH 1/6] misc: add Qualcomm GENI SE QUP device driver

2023-03-30 Thread Konrad Dybcio 



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 
> ---
>  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 ..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 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +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..

would be more stylish, but that's a nit and I don't have
any other complaints! :D

Reviewed-by: Konrad Dybcio 

Konrad
> +
> + 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 = _se_qup_ops,
> + .flags  = DM_FLAG_PRE_RELOC,
> +};