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

[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
> <vladimir.zapolskiy at 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.zapolskiy at 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.zapolskiy at 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.dybcio at 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.

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

Thank you for review!

--
Best wishes,
Vladimir