* Re: [PATCH v6 24/36] nds32: Loadable modules
From: Greentime Hu @ 2018-01-19 14:26 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <CAK8P3a3pQSxM+gJ+dJsyeo5YnuOOyFYVoJdCVmnoHR0Numya5g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-01-18 18:41 GMT+08:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
> On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>>
>> This patch adds support for loadable modules.
>
> One detail:
>
> You still seem to have both the ELF_REL and ELF_RELA based functions
> implemented here, you should drop the unused ELF_REL version:
>
>> diff --git a/arch/nds32/kernel/module.c b/arch/nds32/kernel/module.c
>> new file mode 100644
>> index 0000000..714a6d6
>> --- /dev/null
>> +++ b/arch/nds32/kernel/module.c
>> @@ -0,0 +1,286 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2005-2017 Andes Technology Corporation
>> +
>> +#include <linux/module.h>
>> +#include <linux/elf.h>
>> +#include <linux/vmalloc.h>
>> +
>> +#include <asm/pgtable.h>
>
> include <linux/moduleloader.h> to catch this.
>
>> +int
>> +apply_relocate(Elf32_Shdr * sechdrs, const char *strtab,
>> + unsigned int symindex, unsigned int relsec,
>> + struct module *module)
>> +{
>> + return 0;
>> +}
>
> and drop this.
>
> With that change,
>
> Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Hi, Arnd:
Thank you. I will include moduleloader.h and drop apply_relocate().
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 03/36] sparc: io: To use the define of ioremap_[nocache|wc|wb] in asm-generic/io.h
From: Greentime Hu @ 2018-01-19 12:50 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <CAK8P3a2qx5pZmE-=QM5Bwrsib4XcfVac7RQr-QMzEseQ_oBPEA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-01-18 17:56 GMT+08:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
> On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>>
>> It will be built failed if commit id: d25ea659 is selected. This patch
>> can fix this build error.
>>
>> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> The change is fine, but the reference to commit 'd25ea659' is not for
> two reasons:
>
> - when you rebase the tree, you will get a different ID
> - the recommended format for referring to another commit is to list it with
> 12 digits and the one-line summary like this:
>
> commit d25ea659bc37 ("asm-generic/io.h: move
> ioremap_nocache/ioremap_uc/ioremap_wc/ioremap_…")
>
> - Ideally you use a 'Fixes:' tag in the patch description.
>
> You can add these lines in your .gitconfig to help you here
>
> [alias]
> fixes = show --format='Fixes: %h (\"%s\")' -s
> [core]
> abbrev = 12
>
> That will give you a 'git fixes' command to output the Fixes: line
> in the correct format for future bug fixes. For this particular change,
> I would actually just merge the two patches into one patch that
> then doesn't break anything in the first place.
>
Hi, Arnd:
Thank you for your comments and teaching me how to add these messages.
I will update the commit messages and my .gitconfig.
Since the commit id will be changed after rebasing, I will update the
messages like this.
sparc: io: To use the define of ioremap_[nocache|wc|wb] in asm-generic/io.h
A commit for the nds32 architecture bootstrap("asm-generic/io.h: move
ioremap_nocache/ioremap_uc/ioremap_wc/ioremap_wt out of ifndef CONFIG_MMU")
will move the ioremap_nocache out of the CONFIG_MMU ifdef. This means that
in order to suppress re-definition errors we need to remove the #define
in io_32.h.
Also, the change adds a prototype for ioremap where size is size_t and
offset is phys_addr_t so fix that as well.
Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 2/2] serdev: only match serdev devices
From: Sebastian Reichel @ 2018-01-19 11:07 UTC (permalink / raw)
To: Johan Hovold
Cc: Rob Herring, Greg Kroah-Hartman, Jiri Slaby, linux-serial,
linux-kernel, Frédéric Danis, Hans de Goede
In-Reply-To: <20180109160917.30752-3-johan@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]
Hi,
On Tue, Jan 09, 2018 at 05:09:17PM +0100, Johan Hovold wrote:
> Only serdev devices (a.k.a. clients or slaves) are bound to drivers so
> bail out early from match() in case the device is not a serdev device
> (i.e. if it's a serdev controller).
>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
-- Sebastian
> drivers/tty/serdev/core.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index c8c43834477b..f710862f5c06 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -63,6 +63,11 @@ static const struct device_type serdev_device_type = {
> .release = serdev_device_release,
> };
>
> +static bool is_serdev_device(const struct device *dev)
> +{
> + return dev->type == &serdev_device_type;
> +}
> +
> static void serdev_ctrl_release(struct device *dev)
> {
> struct serdev_controller *ctrl = to_serdev_controller(dev);
> @@ -76,6 +81,9 @@ static const struct device_type serdev_ctrl_type = {
>
> static int serdev_device_match(struct device *dev, struct device_driver *drv)
> {
> + if (!is_serdev_device(dev))
> + return 0;
> +
> /* TODO: platform matching */
> if (acpi_driver_match_device(dev, drv))
> return 1;
> --
> 2.15.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/2] serdev: do not generate modaliases for controllers
From: Sebastian Reichel @ 2018-01-19 11:06 UTC (permalink / raw)
To: Johan Hovold
Cc: Rob Herring, Greg Kroah-Hartman, Jiri Slaby, linux-serial,
linux-kernel, Frédéric Danis, Hans de Goede
In-Reply-To: <20180109160917.30752-2-johan@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 4191 bytes --]
Hi,
On Tue, Jan 09, 2018 at 05:09:16PM +0100, Johan Hovold wrote:
> Serdev controllers are not bound to any drivers and it therefore makes
> no sense to generate modaliases for them.
>
> This has already been fixed separately for ACPI controllers for which
> uevent errors were also being logged during probe due to the missing
> ACPI companions (from which ACPI modaliases are generated).
>
> This patch moves the modalias handling from the bus type to the client
> device type. Specifically, this means that only serdev devices (a.k.a.
> clients or slaves) will have have MODALIAS fields in their uevent
> environments and corresponding modalias sysfs attributes.
>
> Also add the missing static keyword for the modalias device attribute
> when moving the definition.
>
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
-- Sebastian
> drivers/tty/serdev/core.c | 72 ++++++++++++++++++++++-------------------------
> 1 file changed, 34 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 5dc88f61f506..c8c43834477b 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -19,6 +19,38 @@
> static bool is_registered;
> static DEFINE_IDA(ctrl_ida);
>
> +static ssize_t modalias_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int len;
> +
> + len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
> + if (len != -ENODEV)
> + return len;
> +
> + return of_device_modalias(dev, buf, PAGE_SIZE);
> +}
> +static DEVICE_ATTR_RO(modalias);
> +
> +static struct attribute *serdev_device_attrs[] = {
> + &dev_attr_modalias.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(serdev_device);
> +
> +static int serdev_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> + int rc;
> +
> + /* TODO: platform modalias */
> +
> + rc = acpi_device_uevent_modalias(dev, env);
> + if (rc != -ENODEV)
> + return rc;
> +
> + return of_device_uevent_modalias(dev, env);
> +}
> +
> static void serdev_device_release(struct device *dev)
> {
> struct serdev_device *serdev = to_serdev_device(dev);
> @@ -26,6 +58,8 @@ static void serdev_device_release(struct device *dev)
> }
>
> static const struct device_type serdev_device_type = {
> + .groups = serdev_device_groups,
> + .uevent = serdev_device_uevent,
> .release = serdev_device_release,
> };
>
> @@ -49,23 +83,6 @@ static int serdev_device_match(struct device *dev, struct device_driver *drv)
> return of_driver_match_device(dev, drv);
> }
>
> -static int serdev_uevent(struct device *dev, struct kobj_uevent_env *env)
> -{
> - int rc;
> -
> - /* TODO: platform modalias */
> -
> - /* ACPI enumerated controllers do not have a modalias */
> - if (!dev->of_node && dev->type == &serdev_ctrl_type)
> - return 0;
> -
> - rc = acpi_device_uevent_modalias(dev, env);
> - if (rc != -ENODEV)
> - return rc;
> -
> - return of_device_uevent_modalias(dev, env);
> -}
> -
> /**
> * serdev_device_add() - add a device previously constructed via serdev_device_alloc()
> * @serdev: serdev_device to be added
> @@ -305,32 +322,11 @@ static int serdev_drv_remove(struct device *dev)
> return 0;
> }
>
> -static ssize_t modalias_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - int len;
> -
> - len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
> - if (len != -ENODEV)
> - return len;
> -
> - return of_device_modalias(dev, buf, PAGE_SIZE);
> -}
> -DEVICE_ATTR_RO(modalias);
> -
> -static struct attribute *serdev_device_attrs[] = {
> - &dev_attr_modalias.attr,
> - NULL,
> -};
> -ATTRIBUTE_GROUPS(serdev_device);
> -
> static struct bus_type serdev_bus_type = {
> .name = "serial",
> .match = serdev_device_match,
> .probe = serdev_drv_probe,
> .remove = serdev_drv_remove,
> - .uevent = serdev_uevent,
> - .dev_groups = serdev_device_groups,
> };
>
> /**
> --
> 2.15.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] serdev: add method to set parity
From: Sebastian Reichel @ 2018-01-19 10:23 UTC (permalink / raw)
To: Ulrich Hecht
Cc: linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
robh-DgEjT+Ai2ygdnm+yROfE0A,
martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg,
johan-DgEjT+Ai2ygdnm+yROfE0A, marcel-kz+m5ild9QBg9hUCZPvPmw,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
magnus.damm-Re5JQEeQqe8AvxtiuMwx3w,
laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw,
wsa-z923LK4zBo2bacvFa/9K2g, geert-Td1EMuHUCqxL1ZNQvxDV9g
In-Reply-To: <1516203739-4705-1-git-send-email-ulrich.hecht+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4490 bytes --]
Hi,
On Wed, Jan 17, 2018 at 04:42:19PM +0100, Ulrich Hecht wrote:
> Adds serdev_device_set_parity() and an implementation for ttyport.
>
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
-- Sebastian
> Broken out of the "[PATCH 0/6] serdev multiplexing support" series
> because this kind of functionality is apparently also required
> for "[RFC v2 0/9] Realtek Bluetooth serdev support (H5 protocol)",
> which contains an earlier revision of this patch.
>
> CU
> Uli
>
>
> drivers/tty/serdev/core.c | 12 ++++++++++++
> drivers/tty/serdev/serdev-ttyport.c | 18 ++++++++++++++++++
> include/linux/serdev.h | 10 ++++++++++
> 3 files changed, 40 insertions(+)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 5dc88f6..1f25896 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -290,6 +290,18 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
> }
> EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
>
> +int serdev_device_set_parity(struct serdev_device *serdev,
> + enum serdev_parity parity)
> +{
> + struct serdev_controller *ctrl = serdev->ctrl;
> +
> + if (!ctrl || !ctrl->ops->set_parity)
> + return -ENOTSUPP;
> +
> + return ctrl->ops->set_parity(ctrl, parity);
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_set_parity);
> +
> static int serdev_drv_probe(struct device *dev)
> {
> const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index a5abb05..710e9a9 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
> @@ -224,6 +224,23 @@ static int ttyport_set_tiocm(struct serdev_controller *ctrl, unsigned int set, u
> return tty->driver->ops->tiocmset(tty, set, clear);
> }
>
> +static int ttyport_set_parity(struct serdev_controller *ctrl,
> + enum serdev_parity parity)
> +{
> + struct serport *serport = serdev_controller_get_drvdata(ctrl);
> + struct tty_struct *tty = serport->tty;
> + struct ktermios ktermios = tty->termios;
> +
> + ktermios.c_cflag &= ~(PARENB | PARODD);
> + if (parity != SERDEV_PARITY_NONE) {
> + ktermios.c_cflag |= PARENB;
> + if (parity == SERDEV_PARITY_ODD)
> + ktermios.c_cflag |= PARODD;
> + }
> +
> + return tty_set_termios(tty, &ktermios);
> +}
> +
> static const struct serdev_controller_ops ctrl_ops = {
> .write_buf = ttyport_write_buf,
> .write_flush = ttyport_write_flush,
> @@ -235,6 +252,7 @@ static const struct serdev_controller_ops ctrl_ops = {
> .wait_until_sent = ttyport_wait_until_sent,
> .get_tiocm = ttyport_get_tiocm,
> .set_tiocm = ttyport_set_tiocm,
> + .set_parity = ttyport_set_parity,
> };
>
> struct device *serdev_tty_port_register(struct tty_port *port,
> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index 48d8ce2..4dd3cb7 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h
> @@ -78,6 +78,12 @@ static inline struct serdev_device_driver *to_serdev_device_driver(struct device
> return container_of(d, struct serdev_device_driver, driver);
> }
>
> +enum serdev_parity {
> + SERDEV_PARITY_NONE,
> + SERDEV_PARITY_EVEN,
> + SERDEV_PARITY_ODD,
> +};
> +
> /*
> * serdev controller structures
> */
> @@ -92,6 +98,7 @@ struct serdev_controller_ops {
> void (*wait_until_sent)(struct serdev_controller *, long);
> int (*get_tiocm)(struct serdev_controller *);
> int (*set_tiocm)(struct serdev_controller *, unsigned int, unsigned int);
> + int (*set_parity)(struct serdev_controller *, enum serdev_parity);
> };
>
> /**
> @@ -301,6 +308,9 @@ static inline int serdev_device_set_rts(struct serdev_device *serdev, bool enabl
> return serdev_device_set_tiocm(serdev, 0, TIOCM_RTS);
> }
>
> +int serdev_device_set_parity(struct serdev_device *serdev,
> + enum serdev_parity parity);
> +
> /*
> * serdev hooks into TTY core
> */
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [patch v17 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver
From: kbuild test robot @ 2018-01-19 7:28 UTC (permalink / raw)
Cc: kbuild-all-JC7UmRfGjtg, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
arnd-r2nGTMty4D4, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
joel-U3u1mxZcP9KHXe+LvDLADg, jiri-rHqAuBHg3fBzbRFIqnYvSA,
tklauser-93Khv+1bN0NyDzI6CaY1VQ,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
vadimp-VPRAkNaXOzVWk0Htik3J/w,
system-sw-low-level-VPRAkNaXOzVWk0Htik3J/w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
openocd-devel-owner-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-api-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
mchehab-DgEjT+Ai2ygdnm+yROfE0A, Oleksandr Shamray, Jiri Pirko
In-Reply-To: <1516087139-7510-3-git-send-email-oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 5219 bytes --]
Hi Oleksandr,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.15-rc8]
[cannot apply to next-20180118]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Oleksandr-Shamray/drivers-jtag-Add-JTAG-core-driver/20180119-123719
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64
Note: the linux-review/Oleksandr-Shamray/drivers-jtag-Add-JTAG-core-driver/20180119-123719 HEAD b9c3d4721186f8264960ad87c6c499cdd1b6c2e8 builds fine.
It only hurts bisectibility.
All error/warnings (new ones prefixed by >>):
drivers/jtag/jtag-aspeed.c: In function 'aspeed_jtag_init':
>> drivers/jtag/jtag-aspeed.c:657:21: error: implicit declaration of function 'devm_reset_control_get_shared'; did you mean 'devm_pinctrl_get_select'? [-Werror=implicit-function-declaration]
aspeed_jtag->rst = devm_reset_control_get_shared(aspeed_jtag->dev,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
devm_pinctrl_get_select
>> drivers/jtag/jtag-aspeed.c:657:19: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
aspeed_jtag->rst = devm_reset_control_get_shared(aspeed_jtag->dev,
^
>> drivers/jtag/jtag-aspeed.c:664:2: error: implicit declaration of function 'reset_control_deassert' [-Werror=implicit-function-declaration]
reset_control_deassert(aspeed_jtag->rst);
^~~~~~~~~~~~~~~~~~~~~~
drivers/jtag/jtag-aspeed.c: In function 'aspeed_jtag_deinit':
>> drivers/jtag/jtag-aspeed.c:707:2: error: implicit declaration of function 'reset_control_assert' [-Werror=implicit-function-declaration]
reset_control_assert(aspeed_jtag->rst);
^~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +657 drivers/jtag/jtag-aspeed.c
631
632 int aspeed_jtag_init(struct platform_device *pdev,
633 struct aspeed_jtag *aspeed_jtag)
634 {
635 struct resource *res;
636 int err;
637
638 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
639 aspeed_jtag->reg_base = devm_ioremap_resource(aspeed_jtag->dev, res);
640 if (IS_ERR(aspeed_jtag->reg_base))
641 return -ENOMEM;
642
643 aspeed_jtag->pclk = devm_clk_get(aspeed_jtag->dev, NULL);
644 if (IS_ERR(aspeed_jtag->pclk)) {
645 dev_err(aspeed_jtag->dev, "devm_clk_get failed\n");
646 return PTR_ERR(aspeed_jtag->pclk);
647 }
648
649 aspeed_jtag->irq = platform_get_irq(pdev, 0);
650 if (aspeed_jtag->irq < 0) {
651 dev_err(aspeed_jtag->dev, "no irq specified\n");
652 return -ENOENT;
653 }
654
655 clk_prepare_enable(aspeed_jtag->pclk);
656
> 657 aspeed_jtag->rst = devm_reset_control_get_shared(aspeed_jtag->dev,
658 NULL);
659 if (IS_ERR(aspeed_jtag->rst)) {
660 dev_err(aspeed_jtag->dev,
661 "missing or invalid reset controller device tree entry");
662 return PTR_ERR(aspeed_jtag->rst);
663 }
> 664 reset_control_deassert(aspeed_jtag->rst);
665
666 /* Enable clock */
667 aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_CTL_ENG_EN |
668 ASPEED_JTAG_CTL_ENG_OUT_EN, ASPEED_JTAG_CTRL);
669 aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |
670 ASPEED_JTAG_SW_MODE_TDIO, ASPEED_JTAG_SW);
671
672 err = devm_request_irq(aspeed_jtag->dev, aspeed_jtag->irq,
673 aspeed_jtag_interrupt, 0,
674 "aspeed-jtag", aspeed_jtag);
675 if (err) {
676 dev_err(aspeed_jtag->dev, "unable to get IRQ");
677 goto clk_unprep;
678 }
679 dev_dbg(&pdev->dev, "IRQ %d.\n", aspeed_jtag->irq);
680
681 aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_ISR_INST_PAUSE |
682 ASPEED_JTAG_ISR_INST_COMPLETE |
683 ASPEED_JTAG_ISR_DATA_PAUSE |
684 ASPEED_JTAG_ISR_DATA_COMPLETE |
685 ASPEED_JTAG_ISR_INST_PAUSE_EN |
686 ASPEED_JTAG_ISR_INST_COMPLETE_EN |
687 ASPEED_JTAG_ISR_DATA_PAUSE_EN |
688 ASPEED_JTAG_ISR_DATA_COMPLETE_EN,
689 ASPEED_JTAG_ISR);
690
691 aspeed_jtag->flag = 0;
692 aspeed_jtag->mode = 0;
693 init_waitqueue_head(&aspeed_jtag->jtag_wq);
694 return 0;
695
696 clk_unprep:
697 clk_disable_unprepare(aspeed_jtag->pclk);
698 return err;
699 }
700
701 int aspeed_jtag_deinit(struct platform_device *pdev,
702 struct aspeed_jtag *aspeed_jtag)
703 {
704 aspeed_jtag_write(aspeed_jtag, 0, ASPEED_JTAG_ISR);
705 /* Disable clock */
706 aspeed_jtag_write(aspeed_jtag, 0, ASPEED_JTAG_CTRL);
> 707 reset_control_assert(aspeed_jtag->rst);
708 clk_disable_unprepare(aspeed_jtag->pclk);
709 return 0;
710 }
711
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49800 bytes --]
^ permalink raw reply
* Re: [PATCH v2 7/7] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
From: Bjorn Andersson @ 2018-01-19 7:12 UTC (permalink / raw)
To: Karthikeyan Ramasubramanian
Cc: corbet, andy.gross, david.brown, robh+dt, mark.rutland, wsa,
gregkh, linux-doc, linux-arm-msm, devicetree, linux-i2c,
linux-serial, jslaby, Girish Mahadevan, Sagar Dharia
In-Reply-To: <1515805547-22816-8-git-send-email-kramasub@codeaurora.org>
On Fri 12 Jan 17:05 PST 2018, Karthikeyan Ramasubramanian wrote:
> This driver supports GENI based UART Controller in the Qualcomm SOCs. The
> Qualcomm Generic Interface (GENI) is a programmable module supporting a
> wide range of serial interfaces including UART. This driver support console
> operations using FIFO mode of transfer.
>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
Please disregard my previous answer to this patch, I hit the wrong key
combo while stashing my reply.
> ---
> drivers/tty/serial/Kconfig | 10 +
> drivers/tty/serial/Makefile | 1 +
> drivers/tty/serial/qcom_geni_serial.c | 1414 +++++++++++++++++++++++++++++++++
> 3 files changed, 1425 insertions(+)
> create mode 100644 drivers/tty/serial/qcom_geni_serial.c
>
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index b788fee..1be30e5 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1098,6 +1098,16 @@ config SERIAL_MSM_CONSOLE
> select SERIAL_CORE_CONSOLE
> select SERIAL_EARLYCON
>
> +config SERIAL_QCOM_GENI
> + tristate "QCOM on-chip GENI based serial port support"
> + depends on ARCH_QCOM
depend on the GENI wrapper as well.
[..]
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> new file mode 100644
> index 0000000..0dbd329
> --- /dev/null
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -0,0 +1,1414 @@
> +/*
> + * Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
SPDX license header.
> +
> +#include <linux/bitmap.h>
Unused
> +#include <linux/bitops.h>
Unused?
> +#include <linux/delay.h>
> +#include <linux/console.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/qcom-geni-se.h>
> +#include <linux/serial.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +
> +/* UART specific GENI registers */
> +#define SE_UART_TX_TRANS_CFG (0x25C)
Drop the parenthesis
[..]
> +static int owr;
> +module_param(owr, int, 0644);
What's this?
> +
> +struct qcom_geni_serial_port {
> + struct uart_port uport;
> + char name[20];
> + unsigned int tx_fifo_depth;
size_t is a good type for keeping track of sizes.
> + unsigned int tx_fifo_width;
> + unsigned int rx_fifo_depth;
> + unsigned int tx_wm;
> + unsigned int rx_wm;
> + unsigned int rx_rfr;
size_t for sizes.
> + int xfer_mode;
> + bool port_setup;
> + unsigned int *rx_fifo;
The fifo is typeless, so it should be void *. It is however only ever
used as a local variable in handle_rx_console() so drop this.
> + int (*handle_rx)(struct uart_port *uport,
> + unsigned int rx_fifo_wc,
> + unsigned int rx_last_byte_valid,
> + unsigned int rx_last,
> + bool drop_rx);
> + struct device *wrapper_dev;
> + struct geni_se_rsc serial_rsc;
> + unsigned int xmit_size;
In other drivers we read bytes of the xmit buffer at xmit->tail, write
to hardware FIFO and update xmit->tail. Why do you keep a going delta
between the tail and our real tail?
> + void *rx_buf;
> + unsigned int cur_baud;
> +};
> +
[..]
> +
> +#define GET_DEV_PORT(uport) \
> + container_of(uport, struct qcom_geni_serial_port, uport)
The idiomatic name for this macro would be something like "to_dev_port".
The use of "uport" as macro parameter makes this only work if the
parameter is "uport", otherwise the macro will replace the last part of
the container_of as well.
[..]
> +static struct qcom_geni_serial_port *get_port_from_line(int line)
> +{
> + struct qcom_geni_serial_port *port = NULL;
> +
> + if ((line < 0) || (line >= GENI_UART_CONS_PORTS))
> + port = ERR_PTR(-ENXIO);
You need to return here as well...
> + port = &qcom_geni_console_port;
> + return port;
Drop the "port" and just return ERR_PTR(-ENXIO) in the error case and
return &qcom_geni_serial_port otherwise.
> +}
> +
> +static int qcom_geni_serial_poll_bit(struct uart_port *uport,
> + int offset, int bit_field, bool set)
This function is returning "yes" or "no", so make it return "bool"
> +{
> + int iter = 0;
> + unsigned int reg;
> + bool met = false;
> + struct qcom_geni_serial_port *port = NULL;
> + bool cond = false;
> + unsigned int baud = 115200;
> + unsigned int fifo_bits = DEF_FIFO_DEPTH_WORDS * DEF_FIFO_WIDTH_BITS;
> + unsigned long total_iter = 2000;
Don't initialize things that will be assigned directly again.
> +
> +
> + if (uport->private_data) {
When is this function being called on a non-initialized uport?
> + port = GET_DEV_PORT(uport);
> + baud = (port->cur_baud ? port->cur_baud : 115200);
Drop the parenthesis, and consider initializing baud = port->cur_baud
and then give it 115200 if it's zero separately.
> + fifo_bits = port->tx_fifo_depth * port->tx_fifo_width;
> + /*
> + * Total polling iterations based on FIFO worth of bytes to be
> + * sent at current baud .Add a little fluff to the wait.
> + */
> + total_iter = ((fifo_bits * USEC_PER_SEC) / baud) / 10;
> + total_iter += 50;
> + }
> +
> + while (iter < total_iter) {
> + reg = geni_read_reg_nolog(uport->membase, offset);
> + cond = reg & bit_field;
> + if (cond == set) {
> + met = true;
> + break;
> + }
> + udelay(10);
> + iter++;
> + }
Use readl_poll_timeout() instead of all this.
> + return met;
> +}
> +
> +static void qcom_geni_serial_setup_tx(struct uart_port *uport,
> + unsigned int xmit_size)
> +{
> + u32 m_cmd = 0;
Don't initialize this.
> +
> + geni_write_reg_nolog(xmit_size, uport->membase, SE_UART_TX_TRANS_LEN);
> + m_cmd |= (UART_START_TX << M_OPCODE_SHFT);
Why OR this into 0?
> + geni_write_reg_nolog(m_cmd, uport->membase, SE_GENI_M_CMD0);
> + /*
> + * Writes to enable the primary sequencer should go through before
> + * exiting this function.
> + */
> + mb();
> +}
> +
> +static void qcom_geni_serial_poll_cancel_tx(struct uart_port *uport)
This function polls for command done and if that doesn't happen in time
it aborts the command. It then clear any interrupts. The function name
however indicates that we're polling for "cancel of tx".
> +{
> + int done = 0;
> + unsigned int irq_clear = M_CMD_DONE_EN;
> + unsigned int geni_status = 0;
Don't initialize done and geni_status.
> +
> + done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_DONE_EN, true);
> + if (!done) {
> + geni_write_reg_nolog(M_GENI_CMD_ABORT, uport->membase,
> + SE_GENI_M_CMD_CTRL_REG);
> + owr++;
What's owr?
> + irq_clear |= M_CMD_ABORT_EN;
> + qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_ABORT_EN, true);
> + }
> + geni_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_STATUS);
Why is this line wrapped?
> + if (geni_status & M_GENI_CMD_ACTIVE)
> + owr++;
> + geni_write_reg_nolog(irq_clear, uport->membase, SE_GENI_M_IRQ_CLEAR);
> +}
> +
> +static void qcom_geni_serial_abort_rx(struct uart_port *uport)
> +{
> + unsigned int irq_clear = S_CMD_DONE_EN;
> +
> + geni_se_abort_s_cmd(uport->membase);
> + /* Ensure this goes through before polling. */
Move the mb() to the qcom_geni_serial_poll_bit()
> + mb();
> + irq_clear |= S_CMD_ABORT_EN;
> + qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
> + S_GENI_CMD_ABORT, false);
> + geni_write_reg_nolog(irq_clear, uport->membase, SE_GENI_S_IRQ_CLEAR);
> + geni_write_reg(FORCE_DEFAULT, uport->membase, GENI_FORCE_DEFAULT_REG);
> +}
> +
> +#ifdef CONFIG_CONSOLE_POLL
> +static int qcom_geni_serial_get_char(struct uart_port *uport)
> +{
> + unsigned int rx_fifo;
> + unsigned int m_irq_status;
> + unsigned int s_irq_status;
These are u32 and the variable names would benefit from being shorter.
> +
> + if (!(qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_SEC_IRQ_EN, true)))
Drop one parenthesis.
> + return -ENXIO;
> +
> + m_irq_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_M_IRQ_STATUS);
Drop the _nolog and rename the variable to reduce the length of this
line.
> + s_irq_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_S_IRQ_STATUS);
> + geni_write_reg_nolog(m_irq_status, uport->membase,
> + SE_GENI_M_IRQ_CLEAR);
> + geni_write_reg_nolog(s_irq_status, uport->membase,
> + SE_GENI_S_IRQ_CLEAR);
Is it necessary to do this interlaced? Or can you just clear M and then
clear S? I.e. have a single variable named "status".
> +
> + if (!(qcom_geni_serial_poll_bit(uport, SE_GENI_RX_FIFO_STATUS,
> + RX_FIFO_WC_MSK, true)))
> + return -ENXIO;
> +
> + /*
> + * Read the Rx FIFO only after clearing the interrupt registers and
> + * getting valid RX fifo status.
Use non-_relaxed readl and you wouldn't have to do this explicitly.
> + */
> + mb();
> + rx_fifo = geni_read_reg_nolog(uport->membase, SE_GENI_RX_FIFOn);
> + rx_fifo &= 0xFF;
return rx_fifo & 0xff; instead of the extra step
> + return rx_fifo;
> +}
> +
> +static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
> + unsigned char c)
> +{
> + int b = (int) c;
Why create this alias?
> + struct qcom_geni_serial_port *port = GET_DEV_PORT(uport);
> +
> + geni_write_reg_nolog(port->tx_wm, uport->membase,
> + SE_GENI_TX_WATERMARK_REG);
> + qcom_geni_serial_setup_tx(uport, 1);
> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_TX_FIFO_WATERMARK_EN, true))
> + WARN_ON(1);
WARN_ON(!qcom_geni_serial_poll_bit(...));
> + geni_write_reg_nolog(b, uport->membase, SE_GENI_TX_FIFOn);
> + geni_write_reg_nolog(M_TX_FIFO_WATERMARK_EN, uport->membase,
> + SE_GENI_M_IRQ_CLEAR);
> + /*
> + * Ensure FIFO write goes through before polling for status but.
> + */
/* This fits in a one-line comment */
But if this is necessary that means that every time you call
serial_poll() you need to have this comment and a rmb(). Better move it
into the poll function then - or just stop using _relaxed()
> + mb();
> + qcom_geni_serial_poll_cancel_tx(uport);
> +}
> +#endif
> +
> +#if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL)
The Kconfig specifies that we depend on CONFIG_SERIAL_CORE_CONSOLE, so
no need to check that.
> +static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
> +{
> + geni_write_reg_nolog(ch, uport->membase, SE_GENI_TX_FIFOn);
> + /*
> + * Ensure FIFO write clear goes through before
> + * next iteration.
This comment is accurate, but unnecessarily line wrapped.
> + */
> + mb();
> +
> +}
> +
> +static void
> +__qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
> + unsigned int count)
> +{
> + int new_line = 0;
> + int i;
> + int bytes_to_send = count;
> + int fifo_depth = DEF_FIFO_DEPTH_WORDS;
> + int tx_wm = DEF_TX_WM;
> +
> + for (i = 0; i < count; i++) {
> + if (s[i] == '\n')
> + new_line++;
> + }
Why do we need to deal with this?
> +
> + bytes_to_send += new_line;
> + geni_write_reg_nolog(tx_wm, uport->membase,
> + SE_GENI_TX_WATERMARK_REG);
> + qcom_geni_serial_setup_tx(uport, bytes_to_send);
> + i = 0;
> + while (i < count) {
> + u32 chars_to_write = 0;
> + u32 avail_fifo_bytes = (fifo_depth - tx_wm);
Use size_t for these.
> +
> + /*
> + * If the WM bit never set, then the Tx state machine is not
> + * in a valid state, so break, cancel/abort any existing
> + * command. Unfortunately the current data being written is
> + * lost.
> + */
> + while (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_TX_FIFO_WATERMARK_EN, true))
> + break;
> + chars_to_write = min((unsigned int)(count - i),
> + avail_fifo_bytes);
> + if ((chars_to_write << 1) > avail_fifo_bytes)
Write chars_to_write * 2, the compiler will be cleaver for you.
> + chars_to_write = (avail_fifo_bytes >> 1);
> + uart_console_write(uport, (s + i), chars_to_write,
> + qcom_geni_serial_wr_char);
> + geni_write_reg_nolog(M_TX_FIFO_WATERMARK_EN, uport->membase,
> + SE_GENI_M_IRQ_CLEAR);
> + /* Ensure this goes through before polling for WM IRQ again.*/
> + mb();
Again, if this is necessary move it into the poll function instead of
sprinkling it throughout the driver.
> + i += chars_to_write;
> + }
> + qcom_geni_serial_poll_cancel_tx(uport);
> +}
> +
> +static void qcom_geni_serial_console_write(struct console *co, const char *s,
> + unsigned int count)
> +{
> + struct uart_port *uport;
> + struct qcom_geni_serial_port *port;
> + int locked = 1;
Use bool for booleans
> + unsigned long flags;
> +
> + WARN_ON(co->index < 0 || co->index >= GENI_UART_NR_PORTS);
> +
> + port = get_port_from_line(co->index);
> + if (IS_ERR_OR_NULL(port))
port can't be NULL.
> + return;
> +
> + uport = &port->uport;
> + if (oops_in_progress)
> + locked = spin_trylock_irqsave(&uport->lock, flags);
> + else
> + spin_lock_irqsave(&uport->lock, flags);
> +
> + if (locked) {
> + __qcom_geni_serial_console_write(uport, s, count);
> + spin_unlock_irqrestore(&uport->lock, flags);
> + }
> +}
> +
> +static int handle_rx_console(struct uart_port *uport,
> + unsigned int rx_fifo_wc,
> + unsigned int rx_last_byte_valid,
> + unsigned int rx_last,
You should be able to have a single parameter with a better name stating
that "this is the last chunk and it's only N bytes", rather than
encoding this in a bool and a count with obscure names.
> + bool drop_rx)
> +{
> + int i, c;
> + unsigned char *rx_char;
> + struct tty_port *tport;
> + struct qcom_geni_serial_port *qcom_port = GET_DEV_PORT(uport);
> +
> + tport = &uport->state->port;
> + for (i = 0; i < rx_fifo_wc; i++) {
> + int bytes = 4;
> +
> + *(qcom_port->rx_fifo) =
> + geni_read_reg_nolog(uport->membase, SE_GENI_RX_FIFOn);
Just store this value in a local variable, as the only consumer is 3
lines down.
> + if (drop_rx)
> + continue;
> + rx_char = (unsigned char *)qcom_port->rx_fifo;
> +
> + if (i == (rx_fifo_wc - 1)) {
> + if (rx_last && rx_last_byte_valid)
Also, shouldn't rx_last be redundant to the check against rx_fifo_wc?
> + bytes = rx_last_byte_valid;
> + }
> + for (c = 0; c < bytes; c++) {
> + char flag = TTY_NORMAL;
No need for a local variable here.
> + int sysrq;
> +
> + uport->icount.rx++;
> + sysrq = uart_handle_sysrq_char(uport, rx_char[c]);
> + if (!sysrq)
> + tty_insert_flip_char(tport, rx_char[c], flag);
> + }
> + }
> + if (!drop_rx)
> + tty_flip_buffer_push(tport);
> + return 0;
> +}
> +#else
> +static int handle_rx_console(struct uart_port *uport,
> + unsigned int rx_fifo_wc,
> + unsigned int rx_last_byte_valid,
> + unsigned int rx_last,
> + bool drop_rx)
> +{
> + return -EPERM;
> +}
> +
> +#endif /* (CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL)) */
> +
> +static void qcom_geni_serial_start_tx(struct uart_port *uport)
> +{
> + unsigned int geni_m_irq_en;
This is a u32, name it "irq_en"
> + struct qcom_geni_serial_port *qcom_port = GET_DEV_PORT(uport);
> + unsigned int geni_status;
geni_status should be of type u32 and there's no lack of descriptiveness
naming it "status".
> +
> + if (qcom_port->xfer_mode == FIFO_MODE) {
> + geni_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_STATUS);
> + if (geni_status & M_GENI_CMD_ACTIVE)
> + goto exit_start_tx;
Just return
> +
> + if (!qcom_geni_serial_tx_empty(uport))
> + goto exit_start_tx;
Ditto
> +
> + geni_m_irq_en = geni_read_reg_nolog(uport->membase,
> + SE_GENI_M_IRQ_EN);
> + geni_m_irq_en |= (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN);
> +
> + geni_write_reg_nolog(qcom_port->tx_wm, uport->membase,
> + SE_GENI_TX_WATERMARK_REG);
> + geni_write_reg_nolog(geni_m_irq_en, uport->membase,
> + SE_GENI_M_IRQ_EN);
> + /* Geni command setup should complete before returning.*/
If that's the case then verify that it's actually done.
> + mb();
> + }
> +exit_start_tx:
> + return;
> +}
> +
> +static void stop_tx_sequencer(struct uart_port *uport)
> +{
> + unsigned int geni_m_irq_en;
> + unsigned int geni_status;
These are u32 and can be given more succinct names.
> + struct qcom_geni_serial_port *port = GET_DEV_PORT(uport);
> +
> + geni_m_irq_en = geni_read_reg_nolog(uport->membase, SE_GENI_M_IRQ_EN);
Drop the _nolog from all of these to make code easier to read.
> + geni_m_irq_en &= ~M_CMD_DONE_EN;
> + if (port->xfer_mode == FIFO_MODE) {
> + geni_m_irq_en &= ~M_TX_FIFO_WATERMARK_EN;
> + geni_write_reg_nolog(0, uport->membase,
> + SE_GENI_TX_WATERMARK_REG);
> + }
> + port->xmit_size = 0;
> + geni_write_reg_nolog(geni_m_irq_en, uport->membase, SE_GENI_M_IRQ_EN);
> + geni_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_STATUS);
> + /* Possible stop tx is called multiple times. */
> + if (!(geni_status & M_GENI_CMD_ACTIVE))
> + return;
> +
> + geni_se_cancel_m_cmd(uport->membase);
> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_CANCEL_EN, true)) {
> + geni_se_abort_m_cmd(uport->membase);
> + qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_ABORT_EN, true);
> + geni_write_reg_nolog(M_CMD_ABORT_EN, uport->membase,
> + SE_GENI_M_IRQ_CLEAR);
> + }
> + geni_write_reg_nolog(M_CMD_CANCEL_EN, uport, SE_GENI_M_IRQ_CLEAR);
> +}
> +
> +static void qcom_geni_serial_stop_tx(struct uart_port *uport)
> +{
> + stop_tx_sequencer(uport);
Inline stop_tx_sequencer here...
> +}
> +
> +static void start_rx_sequencer(struct uart_port *uport)
> +{
> + unsigned int geni_s_irq_en;
> + unsigned int geni_m_irq_en;
> + unsigned int geni_status;
> + struct qcom_geni_serial_port *port = GET_DEV_PORT(uport);
> +
> + geni_status = geni_read_reg_nolog(uport->membase, SE_GENI_STATUS);
> + if (geni_status & S_GENI_CMD_ACTIVE)
> + qcom_geni_serial_stop_rx(uport);
> +
> + geni_se_setup_s_cmd(uport->membase, UART_START_READ, 0);
> +
> + if (port->xfer_mode == FIFO_MODE) {
> + geni_s_irq_en = geni_read_reg_nolog(uport->membase,
> + SE_GENI_S_IRQ_EN);
> + geni_m_irq_en = geni_read_reg_nolog(uport->membase,
> + SE_GENI_M_IRQ_EN);
> +
> + geni_s_irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
> + geni_m_irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
> +
> + geni_write_reg_nolog(geni_s_irq_en, uport->membase,
> + SE_GENI_S_IRQ_EN);
> + geni_write_reg_nolog(geni_m_irq_en, uport->membase,
> + SE_GENI_M_IRQ_EN);
Do you need to do have these two operations interlaced? Can't you take
care of M and then S using a single variable?
> + }
> + /*
> + * Ensure the writes to the secondary sequencer and interrupt enables
> + * go through.
This is not what mb() does.
> + */
> + mb();
> + geni_status = geni_read_reg_nolog(uport->membase, SE_GENI_STATUS);
> +}
> +
> +static void qcom_geni_serial_start_rx(struct uart_port *uport)
> +{
> + start_rx_sequencer(uport);
Inline start_rx_sequencer
> +}
> +
> +static void stop_rx_sequencer(struct uart_port *uport)
> +{
> + unsigned int geni_s_irq_en;
> + unsigned int geni_m_irq_en;
> + unsigned int geni_status;
> + struct qcom_geni_serial_port *port = GET_DEV_PORT(uport);
> + u32 irq_clear = S_CMD_DONE_EN;
> + bool done;
> +
> + if (port->xfer_mode == FIFO_MODE) {
> + geni_s_irq_en = geni_read_reg_nolog(uport->membase,
> + SE_GENI_S_IRQ_EN);
> + geni_m_irq_en = geni_read_reg_nolog(uport->membase,
> + SE_GENI_M_IRQ_EN);
> + geni_s_irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN);
> + geni_m_irq_en &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> +
> + geni_write_reg_nolog(geni_s_irq_en, uport->membase,
> + SE_GENI_S_IRQ_EN);
> + geni_write_reg_nolog(geni_m_irq_en, uport->membase,
> + SE_GENI_M_IRQ_EN);
> + }
> +
> + geni_status = geni_read_reg_nolog(uport->membase, SE_GENI_STATUS);
> + /* Possible stop rx is called multiple times. */
Shouldn't this be checked before above writes?
> + if (!(geni_status & S_GENI_CMD_ACTIVE))
> + return;
> + geni_se_cancel_s_cmd(uport->membase);
> + /*
> + * Ensure that the cancel goes through before polling for the
> + * cancel control bit.
> + */
> + mb();
> + done = qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
> + S_GENI_CMD_CANCEL, false);
> + geni_status = geni_read_reg_nolog(uport->membase, SE_GENI_STATUS);
> + geni_write_reg_nolog(irq_clear, uport->membase, SE_GENI_S_IRQ_CLEAR);
> + if ((geni_status & S_GENI_CMD_ACTIVE))
Drop the extra parenthesis. Is this checking if we're still active after
the cancel, or is this redundant?
> + qcom_geni_serial_abort_rx(uport);
> +}
> +
> +static void qcom_geni_serial_stop_rx(struct uart_port *uport)
> +{
> + stop_rx_sequencer(uport);
Just inline the function here.
> +}
> +
> +static int qcom_geni_serial_handle_rx(struct uart_port *uport, bool drop_rx)
> +{
> + int ret = 0;
> + unsigned int rx_fifo_status;
> + unsigned int rx_fifo_wc = 0;
> + unsigned int rx_last_byte_valid = 0;
> + unsigned int rx_last = 0;
The context of this function gives that we're dealing with rx and
rx_fifo, so no need to specify that in each local variable.
Also, they should all be u32 and there's no need to initialize them.
> + struct tty_port *tport;
> + struct qcom_geni_serial_port *port = GET_DEV_PORT(uport);
> +
> + tport = &uport->state->port;
> + rx_fifo_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_RX_FIFO_STATUS);
> + rx_fifo_wc = rx_fifo_status & RX_FIFO_WC_MSK;
> + rx_last_byte_valid = ((rx_fifo_status & RX_LAST_BYTE_VALID_MSK) >>
> + RX_LAST_BYTE_VALID_SHFT);
> + rx_last = rx_fifo_status & RX_LAST;
> + if (rx_fifo_wc)
> + port->handle_rx(uport, rx_fifo_wc, rx_last_byte_valid,
> + rx_last, drop_rx);
You're ignoring the return value of handle_rx.
> + return ret;
You don't care about the return value in the caller and you always
return 0 anyways, so make the function void.
> +}
> +
> +static int qcom_geni_serial_handle_tx(struct uart_port *uport)
> +{
> + int ret = 0;
> + struct qcom_geni_serial_port *qcom_port = GET_DEV_PORT(uport);
> + struct circ_buf *xmit = &uport->state->xmit;
Use size_t for sizes.
> + unsigned int avail_fifo_bytes = 0;
Naming avail_fifo_bytes just "avail" is carrying the same information
but is shorter.
> + unsigned int bytes_remaining = 0;
> + int i = 0;
> + unsigned int tx_fifo_status;
> + unsigned int xmit_size;
This is "the number of bytes we're going to take from the uart xmit
buffer and push to the hardware FIFO. Call it "chunk" or something,
because it's not the size of the xmit.
> + unsigned int fifo_width_bytes = 1;
If this really is constantly 1 you can remove the variable and remove a
bit of the complexity in this function.
> + int temp_tail = 0;
Local variables are temporary in nature, so no need to name the variable
temp_tail, it is the "tail" we operate on here.
> +
> + xmit_size = uart_circ_chars_pending(xmit);
> + tx_fifo_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_TX_FIFO_STATUS);
> + /* Both FIFO and framework buffer are drained */
> + if ((xmit_size == qcom_port->xmit_size) && !tx_fifo_status) {
> + qcom_port->xmit_size = 0;
> + uart_circ_clear(xmit);
> + qcom_geni_serial_stop_tx(uport);
> + goto exit_handle_tx;
> + }
> + xmit_size -= qcom_port->xmit_size;
> +
> + avail_fifo_bytes = (qcom_port->tx_fifo_depth - qcom_port->tx_wm) *
> + fifo_width_bytes;
> + temp_tail = (xmit->tail + qcom_port->xmit_size) & (UART_XMIT_SIZE - 1);
> + if (xmit_size > (UART_XMIT_SIZE - temp_tail))
> + xmit_size = (UART_XMIT_SIZE - temp_tail);
> + if (xmit_size > avail_fifo_bytes)
> + xmit_size = avail_fifo_bytes;
This section is confusing me, in other drivers we update xmit->tail and
the uart_circ_chars_pending() returns the number of bytes left to be
written. But in your driver we maintain qcom_port->xmit_size as a delta
between the tail and the next byte in the ring buffer to consume.
I find no place were you're actually updating the tail, so I'm wondering
how setting qcom_port->xmit_size to zero above will actually work, as it
doesn't consider that xmit->head might be elsewhere.
The math that you need to come up with is "how much data is there to be
written" and "how much FIFO space do I have" and then take the min() of
those. The prior should be the return value of uart_circ_chars_pending()
straight off.
> +
> + if (!xmit_size)
> + goto exit_handle_tx;
> +
> + qcom_geni_serial_setup_tx(uport, xmit_size);
> +
> + bytes_remaining = xmit_size;
> + while (i < xmit_size) {
> + unsigned int tx_bytes;
> + unsigned int buf = 0;
> + int c;
> +
> + tx_bytes = ((bytes_remaining < fifo_width_bytes) ?
> + bytes_remaining : fifo_width_bytes);
tx_bytes = min(remaining, fifo_width);
But fifo_width_bytes is 1, so this can be simplified.
> +
> + for (c = 0; c < tx_bytes ; c++)
> + buf |= (xmit->buf[temp_tail + c] << (c * 8));
As bytes_remaining shouldn't be able to reach 0, tx_bytes should always
be 1 and as such this is just a matter of writing
xmit->buf[xmit->tail++] to SE_GENI_TX_FIFOn.
> + geni_write_reg_nolog(buf, uport->membase, SE_GENI_TX_FIFOn);
> + i += tx_bytes;
> + temp_tail = (temp_tail + tx_bytes) & (UART_XMIT_SIZE - 1);
> + uport->icount.tx += tx_bytes;
> + bytes_remaining -= tx_bytes;
> + /* Ensure FIFO write goes through */
Writes to the same register isn't expected to be reordered, so you
shouldn't need this wmb(). Perhaps it's the same barrier that's needed
for other polls?
> + wmb();
> + }
> + qcom_geni_serial_poll_cancel_tx(uport);
> + qcom_port->xmit_size += xmit_size;
> +exit_handle_tx:
> + uart_write_wakeup(uport);
> + return ret;
> +}
> +
> +static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
> +{
> + unsigned int m_irq_status;
> + unsigned int s_irq_status;
> + struct uart_port *uport = dev;
> + unsigned long flags;
> + unsigned int m_irq_en;
> + bool drop_rx = false;
> + struct tty_port *tport = &uport->state->port;
> +
> + spin_lock_irqsave(&uport->lock, flags);
> + if (uport->suspended)
> + goto exit_geni_serial_isr;
Do you need to check this within the spinlock?
Name your labels based on their action, not the function they are in. So
a better name would be "out_unlock". But if this can be done outside the
lock just returning would be better.
> + m_irq_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_M_IRQ_STATUS);
> + s_irq_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_S_IRQ_STATUS);
> + m_irq_en = geni_read_reg_nolog(uport->membase, SE_GENI_M_IRQ_EN);
> + geni_write_reg_nolog(m_irq_status, uport->membase, SE_GENI_M_IRQ_CLEAR);
> + geni_write_reg_nolog(s_irq_status, uport->membase, SE_GENI_S_IRQ_CLEAR);
> +
> + if ((m_irq_status & M_ILLEGAL_CMD_EN)) {
Extra parenthesis, and you can better write this:
if (WARN_ON(m_irq_status & M_ILLEGAL_CMD_EN))
goto out_unlock;
> + WARN_ON(1);
> + goto exit_geni_serial_isr;
> + }
> +
> + if (s_irq_status & S_RX_FIFO_WR_ERR_EN) {
> + uport->icount.overrun++;
> + tty_insert_flip_char(tport, 0, TTY_OVERRUN);
> + }
> +
> + if ((m_irq_status & m_irq_en) &
Is M_ILLEGAL_CMD_EN part of m_irq_en? Can you mask the m_irq_status
based on the irq_en above instead of doing it like this?
> + (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
> + qcom_geni_serial_handle_tx(uport);
> +
> + if ((s_irq_status & S_GP_IRQ_0_EN) || (s_irq_status & S_GP_IRQ_1_EN)) {
Drop the parenthesis.
> + if (s_irq_status & S_GP_IRQ_0_EN)
> + uport->icount.parity++;
> + drop_rx = true;
> + } else if ((s_irq_status & S_GP_IRQ_2_EN) ||
> + (s_irq_status & S_GP_IRQ_3_EN)) {
Ditto.
> + uport->icount.brk++;
> + }
> +
> + if ((s_irq_status & S_RX_FIFO_WATERMARK_EN) ||
> + (s_irq_status & S_RX_FIFO_LAST_EN))
Ditto.
> + qcom_geni_serial_handle_rx(uport, drop_rx);
> +
> +exit_geni_serial_isr:
> + spin_unlock_irqrestore(&uport->lock, flags);
> + return IRQ_HANDLED;
> +}
> +
> +static int get_tx_fifo_size(struct qcom_geni_serial_port *port)
> +{
> + struct uart_port *uport;
> +
> + if (!port)
> + return -ENODEV;
> +
> + uport = &port->uport;
> + port->tx_fifo_depth = geni_se_get_tx_fifo_depth(uport->membase);
> + if (!port->tx_fifo_depth) {
> + dev_err(uport->dev, "%s:Invalid TX FIFO depth read\n",
> + __func__);
> + return -ENXIO;
> + }
> +
> + port->tx_fifo_width = geni_se_get_tx_fifo_width(uport->membase);
> + if (!port->tx_fifo_width) {
> + dev_err(uport->dev, "%s:Invalid TX FIFO width read\n",
> + __func__);
> + return -ENXIO;
> + }
> +
> + port->rx_fifo_depth = geni_se_get_rx_fifo_depth(uport->membase);
> + if (!port->rx_fifo_depth) {
> + dev_err(uport->dev, "%s:Invalid RX FIFO depth read\n",
> + __func__);
> + return -ENXIO;
> + }
> +
> + uport->fifosize =
> + ((port->tx_fifo_depth * port->tx_fifo_width) >> 3);
This line wrap isn't necessary, use division rather than shifting and
describe why the fifosize is 1/8 of the depth * width (is the width
expressed in bits perhaps?)
> + return 0;
> +}
> +
> +static void set_rfr_wm(struct qcom_geni_serial_port *port)
> +{
> + /*
> + * Set RFR (Flow off) to FIFO_DEPTH - 2.
> + * RX WM level at 10% RX_FIFO_DEPTH.
> + * TX WM level at 10% TX_FIFO_DEPTH.
> + */
> + port->rx_rfr = port->rx_fifo_depth - 2;
> + port->rx_wm = UART_CONSOLE_RX_WM;
> + port->tx_wm = 2;
> +}
> +
> +static void qcom_geni_serial_shutdown(struct uart_port *uport)
> +{
> + unsigned long flags;
> +
> + /* Stop the console before stopping the current tx */
> + console_stop(uport->cons);
> +
> + disable_irq(uport->irq);
> + free_irq(uport->irq, uport);
> + spin_lock_irqsave(&uport->lock, flags);
> + qcom_geni_serial_stop_tx(uport);
> + qcom_geni_serial_stop_rx(uport);
> + spin_unlock_irqrestore(&uport->lock, flags);
> +}
> +
> +static int qcom_geni_serial_port_setup(struct uart_port *uport)
> +{
> + int ret = 0;
> + struct qcom_geni_serial_port *qcom_port = GET_DEV_PORT(uport);
> + unsigned long cfg0, cfg1;
> + unsigned int rxstale = DEFAULT_BITS_PER_CHAR * STALE_TIMEOUT;
> +
> + set_rfr_wm(qcom_port);
> + geni_write_reg_nolog(rxstale, uport->membase, SE_UART_RX_STALE_CNT);
> + /*
> + * Make an unconditional cancel on the main sequencer to reset
> + * it else we could end up in data loss scenarios.
> + */
> + qcom_port->xfer_mode = FIFO_MODE;
> + qcom_geni_serial_poll_cancel_tx(uport);
> + geni_se_get_packing_config(8, 1, false, &cfg0, &cfg1);
> + geni_write_reg_nolog(cfg0, uport->membase,
> + SE_GENI_TX_PACKING_CFG0);
> + geni_write_reg_nolog(cfg1, uport->membase,
> + SE_GENI_TX_PACKING_CFG1);
> + geni_se_get_packing_config(8, 4, false, &cfg0, &cfg1);
> + geni_write_reg_nolog(cfg0, uport->membase,
> + SE_GENI_RX_PACKING_CFG0);
> + geni_write_reg_nolog(cfg1, uport->membase,
> + SE_GENI_RX_PACKING_CFG1);
These aren't above 80 chars, why are you line breaking?
> + ret = geni_se_init(uport->membase, qcom_port->rx_wm, qcom_port->rx_rfr);
> + if (ret) {
> + dev_err(uport->dev, "%s: Fail\n", __func__);
> + goto exit_portsetup;
> + }
> +
> + ret = geni_se_select_mode(uport->membase, qcom_port->xfer_mode);
> + if (ret)
Do you need to roll back any init done in the wrapper code? How about
passing the mode to the init function?
> + goto exit_portsetup;
> +
> + qcom_port->port_setup = true;
> + /*
> + * Ensure Port setup related IO completes before returning to
> + * framework.
That's not what mb does.
> + */
> + mb();
> +exit_portsetup:
> + return ret;
> +}
> +
> +static int qcom_geni_serial_startup(struct uart_port *uport)
> +{
> + int ret = 0;
> + struct qcom_geni_serial_port *qcom_port = GET_DEV_PORT(uport);
> +
> + scnprintf(qcom_port->name, sizeof(qcom_port->name),
> + "qcom_serial_geni%d", uport->line);
> +
> + if (unlikely(geni_se_get_proto(uport->membase) != UART)) {
Drop the unlikely(), in favour of readable code.
> + dev_err(uport->dev, "%s: Invalid FW %d loaded.\n",
> + __func__, geni_se_get_proto(uport->membase));
Drop the __func__, the message should describe the issue in itself.
> + ret = -ENXIO;
> + goto exit_startup;
We haven't done anything, so just return here.
> + }
> +
> + get_tx_fifo_size(qcom_port);
> + if (!qcom_port->port_setup) {
> + if (qcom_geni_serial_port_setup(uport))
> + goto exit_startup;
> + }
> +
> + /*
> + * Ensure that all the port configuration writes complete
> + * before returning to the framework.
That's not what mb() does.
> + */
> + mb();
> + ret = request_irq(uport->irq, qcom_geni_serial_isr, IRQF_TRIGGER_HIGH,
> + qcom_port->name, uport);
> + if (unlikely(ret)) {
> + dev_err(uport->dev, "%s: Failed to get IRQ ret %d\n",
> + __func__, ret);
Drop the __func__
> + goto exit_startup;
> + }
> +
> +exit_startup:
> + return ret;
> +}
> +
> +static int get_clk_cfg(unsigned long clk_freq, unsigned long *ser_clk)
> +{
> + unsigned long root_freq[] = {7372800, 14745600, 19200000, 29491200,
> + 32000000, 48000000, 64000000, 80000000, 96000000, 100000000};
> + int i;
> + int match = -1;
> +
> + for (i = 0; i < ARRAY_SIZE(root_freq); i++) {
> + if (clk_freq > root_freq[i])
> + continue;
> +
> + if (!(root_freq[i] % clk_freq)) {
> + match = i;
> + break;
> + }
> + }
> + if (match != -1)
> + *ser_clk = root_freq[match];
> + else
> + pr_err("clk_freq %ld\n", clk_freq);
Errors are for humans to consume, so make the message useful for a
human.
> + return match;
The index isn't used, so return the frequency instead of filling in the
ser_clk reference.
> +}
> +
> +static void geni_serial_write_term_regs(struct uart_port *uport,
> + u32 tx_trans_cfg, u32 tx_parity_cfg, u32 rx_trans_cfg,
> + u32 rx_parity_cfg, u32 bits_per_char, u32 stop_bit_len,
> + u32 s_clk_cfg)
> +{
> + geni_write_reg_nolog(tx_trans_cfg, uport->membase,
> + SE_UART_TX_TRANS_CFG);
> + geni_write_reg_nolog(tx_parity_cfg, uport->membase,
> + SE_UART_TX_PARITY_CFG);
> + geni_write_reg_nolog(rx_trans_cfg, uport->membase,
> + SE_UART_RX_TRANS_CFG);
> + geni_write_reg_nolog(rx_parity_cfg, uport->membase,
> + SE_UART_RX_PARITY_CFG);
> + geni_write_reg_nolog(bits_per_char, uport->membase,
> + SE_UART_TX_WORD_LEN);
> + geni_write_reg_nolog(bits_per_char, uport->membase,
> + SE_UART_RX_WORD_LEN);
> + geni_write_reg_nolog(stop_bit_len, uport->membase,
> + SE_UART_TX_STOP_BIT_LEN);
> + geni_write_reg_nolog(s_clk_cfg, uport->membase, GENI_SER_M_CLK_CFG);
> + geni_write_reg_nolog(s_clk_cfg, uport->membase, GENI_SER_S_CLK_CFG);
Drop the _nolog and you should be able to fit these within 80 chars.
> +}
> +
> +static int get_clk_div_rate(unsigned int baud, unsigned long *desired_clk_rate)
> +{
> + unsigned long ser_clk;
> + int dfs_index;
> + int clk_div = 0;
> +
> + *desired_clk_rate = baud * UART_OVERSAMPLING;
> + dfs_index = get_clk_cfg(*desired_clk_rate, &ser_clk);
> + if (dfs_index < 0) {
> + pr_err("%s: Can't find matching DFS entry for baud %d\n",
> + __func__, baud);
> + clk_div = -EINVAL;
Just return -EINVAL here, instead of using goto.
> + goto exit_get_clk_div_rate;
> + }
> +
> + clk_div = ser_clk / *desired_clk_rate;
> + *desired_clk_rate = ser_clk;
> +exit_get_clk_div_rate:
> + return clk_div;
Clock functions typically return a frequency, so I think it's better to
return that and pass the divider by reference.
> +}
> +
> +static void qcom_geni_serial_set_termios(struct uart_port *uport,
> + struct ktermios *termios, struct ktermios *old)
> +{
> + unsigned int baud;
> + unsigned int bits_per_char = 0;
> + unsigned int tx_trans_cfg;
> + unsigned int tx_parity_cfg;
> + unsigned int rx_trans_cfg;
> + unsigned int rx_parity_cfg;
> + unsigned int stop_bit_len;
> + unsigned int clk_div;
> + unsigned long ser_clk_cfg = 0;
> + struct qcom_geni_serial_port *port = GET_DEV_PORT(uport);
> + unsigned long clk_rate;
> +
> + qcom_geni_serial_stop_rx(uport);
> + /* baud rate */
> + baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
> + port->cur_baud = baud;
> + clk_div = get_clk_div_rate(baud, &clk_rate);
> + if (clk_div <= 0)
> + goto exit_set_termios;
Name your labels based on their actions; if you name it "out_restart_rx"
you don't even have to look below to know what will happen when you
jump.
> +
> + uport->uartclk = clk_rate;
> + clk_set_rate(port->serial_rsc.se_clk, clk_rate);
> + ser_clk_cfg |= SER_CLK_EN;
> + ser_clk_cfg |= (clk_div << CLK_DIV_SHFT);
> +
> + /* parity */
> + tx_trans_cfg = geni_read_reg_nolog(uport->membase,
Drop the _nolog and you don't need to line wrap
> + SE_UART_TX_TRANS_CFG);
> + tx_parity_cfg = geni_read_reg_nolog(uport->membase,
> + SE_UART_TX_PARITY_CFG);
> + rx_trans_cfg = geni_read_reg_nolog(uport->membase,
> + SE_UART_RX_TRANS_CFG);
> + rx_parity_cfg = geni_read_reg_nolog(uport->membase,
> + SE_UART_RX_PARITY_CFG);
> + if (termios->c_cflag & PARENB) {
> + tx_trans_cfg |= UART_TX_PAR_EN;
> + rx_trans_cfg |= UART_RX_PAR_EN;
> + tx_parity_cfg |= PAR_CALC_EN;
> + rx_parity_cfg |= PAR_CALC_EN;
> + if (termios->c_cflag & PARODD) {
> + tx_parity_cfg |= PAR_ODD;
> + rx_parity_cfg |= PAR_ODD;
> + } else if (termios->c_cflag & CMSPAR) {
> + tx_parity_cfg |= PAR_SPACE;
> + rx_parity_cfg |= PAR_SPACE;
> + } else {
> + tx_parity_cfg |= PAR_EVEN;
> + rx_parity_cfg |= PAR_EVEN;
> + }
> + } else {
> + tx_trans_cfg &= ~UART_TX_PAR_EN;
> + rx_trans_cfg &= ~UART_RX_PAR_EN;
> + tx_parity_cfg &= ~PAR_CALC_EN;
> + rx_parity_cfg &= ~PAR_CALC_EN;
> + }
> +
> + /* bits per char */
> + switch (termios->c_cflag & CSIZE) {
> + case CS5:
> + bits_per_char = 5;
> + break;
> + case CS6:
> + bits_per_char = 6;
> + break;
> + case CS7:
> + bits_per_char = 7;
> + break;
> + case CS8:
> + default:
> + bits_per_char = 8;
> + break;
> + }
> +
> + /* stop bits */
> + if (termios->c_cflag & CSTOPB)
> + stop_bit_len = TX_STOP_BIT_LEN_2;
> + else
> + stop_bit_len = TX_STOP_BIT_LEN_1;
> +
> + /* flow control, clear the CTS_MASK bit if using flow control. */
> + if (termios->c_cflag & CRTSCTS)
> + tx_trans_cfg &= ~UART_CTS_MASK;
> + else
> + tx_trans_cfg |= UART_CTS_MASK;
> +
> + if (likely(baud))
Don't do likely/unlikely.
> + uart_update_timeout(uport, termios->c_cflag, baud);
> +
> + geni_serial_write_term_regs(uport, tx_trans_cfg, tx_parity_cfg,
> + rx_trans_cfg, rx_parity_cfg, bits_per_char, stop_bit_len,
> + ser_clk_cfg);
It would likely be fine to just inline the register writes here, instead
of this long list of parameters.
> +exit_set_termios:
> + qcom_geni_serial_start_rx(uport);
> + return;
No need to keep an empty return last in the function, and drop the empty
line following it.
> +
> +}
> +
> +static unsigned int qcom_geni_serial_tx_empty(struct uart_port *uport)
> +{
> + unsigned int tx_fifo_status;
> + unsigned int is_tx_empty = 1;
> +
> + tx_fifo_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_TX_FIFO_STATUS);
Drop the "_nolog" and naming the result "status" would be just as
expressive, but keep you below 80 chars.
> + if (tx_fifo_status)
> + is_tx_empty = 0;
> +
> + return is_tx_empty;
Instead of what you're doing with is_tx_empty, just do:
return !readl(membase + FIFO_STATUS);
> +}
> +
> +#if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL)
> +static int __init qcom_geni_console_setup(struct console *co, char *options)
> +{
> + struct uart_port *uport;
> + struct qcom_geni_serial_port *dev_port;
> + int baud = 115200;
> + int bits = 8;
> + int parity = 'n';
> + int flow = 'n';
> + int ret = 0;
Drop initialization of all these variables.
> +
> + if (unlikely(co->index >= GENI_UART_NR_PORTS || co->index < 0))
Drop the unlikely.
> + return -ENXIO;
> +
> + dev_port = get_port_from_line(co->index);
> + if (IS_ERR_OR_NULL(dev_port)) {
port can't be NULL.
> + ret = PTR_ERR(dev_port);
> + pr_err("Invalid line %d(%d)\n", co->index, ret);
> + return ret;
Just return PTR_ERR(dev_port);
> + }
> +
> + uport = &dev_port->uport;
> +
> + if (unlikely(!uport->membase))
> + return -ENXIO;
> +
> + if (geni_se_resources_on(&dev_port->serial_rsc))
> + WARN_ON(1);
If this fails we're likely to access unclocked resources causing the
system to restart, so we should probably not continue here.
> +
> + if (unlikely(geni_se_get_proto(uport->membase) != UART)) {
> + geni_se_resources_off(&dev_port->serial_rsc);
> + return -ENXIO;
> + }
> +
[..]
> +
> +static const struct of_device_id qcom_geni_serial_match_table[] = {
> + { .compatible = "qcom,geni-debug-uart",
> + .data = (void *)&qcom_geni_console_driver},
Shouldn't need this typecast.
Is it necessary to treat the console uart differently and will there be
a patch adding support for non-console instances? How will this differ
and why would that be a different compatible?
> +};
> +
> +static int qcom_geni_serial_probe(struct platform_device *pdev)
> +{
> + int ret = 0;
> + int line = -1;
> + struct qcom_geni_serial_port *dev_port;
> + struct uart_port *uport;
> + struct resource *res;
> + struct uart_driver *drv;
> + const struct of_device_id *id;
> +
> + id = of_match_device(qcom_geni_serial_match_table, &pdev->dev);
Use of_device_get_match_data()
> + if (id) {
> + dev_dbg(&pdev->dev, "%s: %s\n", __func__, id->compatible);
> + drv = (struct uart_driver *)id->data;
> + } else {
> + dev_err(&pdev->dev, "%s: No matching device found", __func__);
> + return -ENODEV;
> + }
> +
> + if (pdev->dev.of_node) {
> + if (drv->cons)
The one and only uart_driver has a cons.
> + line = of_alias_get_id(pdev->dev.of_node, "serial");
> + } else {
> + line = pdev->id;
> + }
> +
> + if (line < 0)
> + line = atomic_inc_return(&uart_line_id) - 1;
> +
> + if ((line < 0) || (line >= GENI_UART_CONS_PORTS))
> + return -ENXIO;
> + dev_port = get_port_from_line(line);
> + if (IS_ERR_OR_NULL(dev_port)) {
port can't be NULL.
> + ret = PTR_ERR(dev_port);
> + dev_err(&pdev->dev, "Invalid line %d(%d)\n",
> + line, ret);
> + goto exit_geni_serial_probe;
You're not doing anything other than returning in
exit_geni_serial_probe, if you just return here I don't need to jump to
the bottom of the function to figure this out...
> + }
> +
> + uport = &dev_port->uport;
> +
> + /* Don't allow 2 drivers to access the same port */
> + if (uport->private_data) {
> + ret = -ENODEV;
> + goto exit_geni_serial_probe;
> + }
> +
> + uport->dev = &pdev->dev;
> + dev_port->wrapper_dev = pdev->dev.parent;
> + dev_port->serial_rsc.wrapper_dev = pdev->dev.parent;
> + dev_port->serial_rsc.se_clk = devm_clk_get(&pdev->dev, "se-clk");
> + if (IS_ERR(dev_port->serial_rsc.se_clk)) {
> + ret = PTR_ERR(dev_port->serial_rsc.se_clk);
> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
> + goto exit_geni_serial_probe;
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "se-phys");
Don't grab this by name, just pick the first one.
> + if (!res) {
> + ret = -ENXIO;
> + dev_err(&pdev->dev, "Err getting IO region\n");
> + goto exit_geni_serial_probe;
> + }
Drop the error handling here.
> + uport->mapbase = res->start;
No
> + uport->membase = devm_ioremap(&pdev->dev, res->start,
> + resource_size(res));
> + if (!uport->membase) {
> + ret = -ENOMEM;
> + dev_err(&pdev->dev, "Err IO mapping serial iomem");
> + goto exit_geni_serial_probe;
> + }
> +
> + dev_port->serial_rsc.geni_pinctrl = devm_pinctrl_get(&pdev->dev);
Drop all of the pinctrl stuff.
> + if (IS_ERR_OR_NULL(dev_port->serial_rsc.geni_pinctrl)) {
> + dev_err(&pdev->dev, "No pinctrl config specified!\n");
> + ret = PTR_ERR(dev_port->serial_rsc.geni_pinctrl);
> + goto exit_geni_serial_probe;
> + }
[..]
> + dev_port->tx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> + dev_port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> + dev_port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
> + uport->fifosize =
> + ((dev_port->tx_fifo_depth * dev_port->tx_fifo_width) >> 3);
qcom_geni_serial_startup() sets this as well.
> +
> + uport->irq = platform_get_irq(pdev, 0);
> + if (uport->irq < 0) {
> + ret = uport->irq;
> + dev_err(&pdev->dev, "Failed to get IRQ %d\n", ret);
> + goto exit_geni_serial_probe;
> + }
> +
> + uport->private_data = (void *)drv;
No need to explicitly typecast to void*
> + platform_set_drvdata(pdev, dev_port);
> + dev_port->handle_rx = handle_rx_console;
Why have a function pointer when you're only referencing a fixed
function.
> + dev_port->rx_fifo = devm_kzalloc(uport->dev, sizeof(u32),
> + GFP_KERNEL);
It takes 8 bytes to store a pointer and the allocation will have some
overhead...just to provide storage space for 4 bytes. And you don't have
any error handling...
> + dev_port->port_setup = false;
> + return uart_add_one_port(drv, uport);
> +
> +exit_geni_serial_probe:
> + return ret;
> +}
> +
> +static int qcom_geni_serial_remove(struct platform_device *pdev)
> +{
> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> + struct uart_driver *drv =
> + (struct uart_driver *)port->uport.private_data;
No need to typecast private_data.
> +
> + uart_remove_one_port(drv, &port->uport);
> + return 0;
> +}
> +
> +
> +#ifdef CONFIG_PM
Drop this and mark the functions __maybe_unused.
> +static int qcom_geni_serial_sys_suspend_noirq(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> + struct uart_port *uport = &port->uport;
> +
> + uart_suspend_port((struct uart_driver *)uport->private_data,
> + uport);
No need to typecast private_data.
> + return 0;
> +}
> +
> +static int qcom_geni_serial_sys_resume_noirq(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> + struct uart_port *uport = &port->uport;
> +
> + if (console_suspend_enabled && uport->suspended) {
> + uart_resume_port((struct uart_driver *)uport->private_data,
> + uport);
No need to typecast private_data.
> + disable_irq(uport->irq);
> + }
> + return 0;
> +}
[..]
> +static int __init qcom_geni_serial_init(void)
> +{
> + int ret = 0;
> + int i;
> +
> + for (i = 0; i < GENI_UART_CONS_PORTS; i++) {
Don't loop over [0,1) to update the one global variable.
> + qcom_geni_console_port.uport.iotype = UPIO_MEM;
> + qcom_geni_console_port.uport.ops = &qcom_geni_console_pops;
> + qcom_geni_console_port.uport.flags = UPF_BOOT_AUTOCONF;
> + qcom_geni_console_port.uport.line = i;
> + }
> +
> + ret = console_register(&qcom_geni_console_driver);
> + if (ret)
> + return ret;
> +
> + ret = platform_driver_register(&qcom_geni_serial_platform_driver);
> + if (ret) {
> + console_unregister(&qcom_geni_console_driver);
> + return ret;
> + }
> + return ret;
ret is 0 here, drop the return from within the if statement or just
return 0 for clarity.
> +}
> +module_init(qcom_geni_serial_init);
> +
> +static void __exit qcom_geni_serial_exit(void)
> +{
> + platform_driver_unregister(&qcom_geni_serial_platform_driver);
> + console_unregister(&qcom_geni_console_driver);
> +}
> +module_exit(qcom_geni_serial_exit);
> +
> +MODULE_DESCRIPTION("Serial driver for GENI based QUP cores");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("tty:qcom_geni_serial");
What will request the kernel module by this alias?
Regards,
Bjorn
^ permalink raw reply
* Re: [patch v15 4/4] Documentation: jtag: Add ABI documentation
From: Pavel Machek @ 2018-01-18 21:27 UTC (permalink / raw)
To: Oleksandr Shamray
Cc: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, arnd-r2nGTMty4D4,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
joel-U3u1mxZcP9KHXe+LvDLADg, jiri-rHqAuBHg3fBzbRFIqnYvSA,
tklauser-93Khv+1bN0NyDzI6CaY1VQ,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
vadimp-VPRAkNaXOzVWk0Htik3J/w,
system-sw-low-level-VPRAkNaXOzVWk0Htik3J/w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
openocd-devel-owner-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-api-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
mchehab-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <1514202808-29747-5-git-send-email-oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2176 bytes --]
On Mon 2017-12-25 13:53:28, Oleksandr Shamray wrote:
> Added document that describe the ABI for JTAG class drivrer
>
> Signed-off-by: Oleksandr Shamray <oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> ---
> v14->v15
> v13->v14
> v12->v13
> v11->v12
> Tobias Klauser <tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org>
> - rename /Documentation/ABI/testing/jatg-dev -> jtag-dev
> - Typo: s/interfase/interface
> v10->v11
> v9->v10
> Fixes added by Oleksandr:
> - change jtag-cdev to jtag-dev in documentation
> - update Kernel Version and Date in jtag-dev documentation;
> v8->v9
> v7->v8
> v6->v7
> Comments pointed by Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> - Added jtag-cdev documentation to Documentation/ABI/testing folder
> ---
> Documentation/ABI/testing/jtag-dev | 27 +++++++++++++++++++++++++++
> 1 files changed, 27 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/ABI/testing/jtag-dev
>
> diff --git a/Documentation/ABI/testing/jtag-dev b/Documentation/ABI/testing/jtag-dev
> new file mode 100644
> index 0000000..cab867d
> --- /dev/null
> +++ b/Documentation/ABI/testing/jtag-dev
> @@ -0,0 +1,27 @@
> +What: /dev/jtag[0-9]+
> +Date: October 2017
> +KernelVersion: 4.15
> +Contact: oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org
> +Description:
> + The misc device files /dev/jtag* are the interface
> + between JTAG master interface and userspace.
> +
> + The ioctl(2)-based ABI is defined and documented in
> + [include/uapi]<linux/jtag.h>.
> +
> + The following file operations are supported:
> +
> + open(2)
> + The argument flag currently support only one access
> + mode O_RDWR.
> +
> + ioctl(2)
> + Initiate various actions.
> + See the inline documentation in [include/uapi]<linux/jtag.h>
> + for descriptions of all ioctls.
Sorry, can we have real docs? I tried to decipher it from jtag.h, but
its not easy. We should have API docs...
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v2 7/7] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
From: Bjorn Andersson @ 2018-01-18 19:43 UTC (permalink / raw)
To: Karthikeyan Ramasubramanian
Cc: corbet, andy.gross, david.brown, robh+dt, mark.rutland, wsa,
gregkh, linux-doc, linux-arm-msm, devicetree, linux-i2c,
linux-serial, jslaby, Girish Mahadevan, Sagar Dharia
In-Reply-To: <1515805547-22816-8-git-send-email-kramasub@codeaurora.org>
On Fri 12 Jan 17:05 PST 2018, Karthikeyan Ramasubramanian wrote:
> This driver supports GENI based UART Controller in the Qualcomm SOCs. The
> Qualcomm Generic Interface (GENI) is a programmable module supporting a
> wide range of serial interfaces including UART. This driver support console
> operations using FIFO mode of transfer.
>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> ---
> drivers/tty/serial/Kconfig | 10 +
> drivers/tty/serial/Makefile | 1 +
> drivers/tty/serial/qcom_geni_serial.c | 1414 +++++++++++++++++++++++++++++++++
> 3 files changed, 1425 insertions(+)
> create mode 100644 drivers/tty/serial/qcom_geni_serial.c
>
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index b788fee..1be30e5 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1098,6 +1098,16 @@ config SERIAL_MSM_CONSOLE
> select SERIAL_CORE_CONSOLE
> select SERIAL_EARLYCON
>
> +config SERIAL_QCOM_GENI
> + tristate "QCOM on-chip GENI based serial port support"
> + depends on ARCH_QCOM
depend on the GENI wrapper as well.
> + select SERIAL_CORE
> + select SERIAL_CORE_CONSOLE
> + select SERIAL_EARLYCON
> + help
> + Serial driver for Qualcomm Technologies Inc's GENI based QUP
> + hardware.
> +
> config SERIAL_VT8500
> bool "VIA VT8500 on-chip serial port support"
> depends on ARCH_VT8500
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> index 842d185..64a8d82 100644
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_SERIAL_SGI_IOC3) += ioc3_serial.o
> obj-$(CONFIG_SERIAL_ATMEL) += atmel_serial.o
> obj-$(CONFIG_SERIAL_UARTLITE) += uartlite.o
> obj-$(CONFIG_SERIAL_MSM) += msm_serial.o
> +obj-$(CONFIG_SERIAL_QCOM_GENI) += qcom_geni_serial.o
> obj-$(CONFIG_SERIAL_NETX) += netx-serial.o
> obj-$(CONFIG_SERIAL_KS8695) += serial_ks8695.o
> obj-$(CONFIG_SERIAL_OMAP) += omap-serial.o
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> new file mode 100644
> index 0000000..0dbd329
> --- /dev/null
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -0,0 +1,1414 @@
> +/*
> + * Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
SPDX license header.
> +
> +#include <linux/bitmap.h>
Unused
> +#include <linux/bitops.h>
Unused?
> +#include <linux/delay.h>
> +#include <linux/console.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/qcom-geni-se.h>
> +#include <linux/serial.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +
> +/* UART specific GENI registers */
> +#define SE_UART_TX_TRANS_CFG (0x25C)
Drop the parenthesis
> +#define SE_UART_TX_WORD_LEN (0x268)
> +#define SE_UART_TX_STOP_BIT_LEN (0x26C)
> +#define SE_UART_TX_TRANS_LEN (0x270)
> +#define SE_UART_RX_TRANS_CFG (0x280)
> +#define SE_UART_RX_WORD_LEN (0x28C)
> +#define SE_UART_RX_STALE_CNT (0x294)
> +#define SE_UART_TX_PARITY_CFG (0x2A4)
> +#define SE_UART_RX_PARITY_CFG (0x2A8)
> +
> +/* SE_UART_TRANS_CFG */
> +#define UART_TX_PAR_EN (BIT(0))
> +#define UART_CTS_MASK (BIT(1))
> +
> +/* SE_UART_TX_WORD_LEN */
> +#define TX_WORD_LEN_MSK (GENMASK(9, 0))
> +
> +/* SE_UART_TX_STOP_BIT_LEN */
> +#define TX_STOP_BIT_LEN_MSK (GENMASK(23, 0))
> +#define TX_STOP_BIT_LEN_1 (0)
> +#define TX_STOP_BIT_LEN_1_5 (1)
> +#define TX_STOP_BIT_LEN_2 (2)
> +
> +/* SE_UART_TX_TRANS_LEN */
> +#define TX_TRANS_LEN_MSK (GENMASK(23, 0))
> +
> +/* SE_UART_RX_TRANS_CFG */
> +#define UART_RX_INS_STATUS_BIT (BIT(2))
> +#define UART_RX_PAR_EN (BIT(3))
> +
> +/* SE_UART_RX_WORD_LEN */
> +#define RX_WORD_LEN_MASK (GENMASK(9, 0))
> +
> +/* SE_UART_RX_STALE_CNT */
> +#define RX_STALE_CNT (GENMASK(23, 0))
> +
> +/* SE_UART_TX_PARITY_CFG/RX_PARITY_CFG */
> +#define PAR_CALC_EN (BIT(0))
> +#define PAR_MODE_MSK (GENMASK(2, 1))
> +#define PAR_MODE_SHFT (1)
> +#define PAR_EVEN (0x00)
> +#define PAR_ODD (0x01)
> +#define PAR_SPACE (0x10)
> +#define PAR_MARK (0x11)
> +
> +/* UART M_CMD OP codes */
> +#define UART_START_TX (0x1)
> +#define UART_START_BREAK (0x4)
> +#define UART_STOP_BREAK (0x5)
> +/* UART S_CMD OP codes */
> +#define UART_START_READ (0x1)
> +#define UART_PARAM (0x1)
> +
> +#define UART_OVERSAMPLING (32)
> +#define STALE_TIMEOUT (16)
> +#define DEFAULT_BITS_PER_CHAR (10)
> +#define GENI_UART_NR_PORTS (15)
> +#define GENI_UART_CONS_PORTS (1)
> +#define DEF_FIFO_DEPTH_WORDS (16)
> +#define DEF_TX_WM (2)
> +#define DEF_FIFO_WIDTH_BITS (32)
> +#define UART_CORE2X_VOTE (10000)
> +#define UART_CONSOLE_RX_WM (2)
> +
> +static int owr;
> +module_param(owr, int, 0644);
What's this?
> +
> +struct qcom_geni_serial_port {
> + struct uart_port uport;
> + char name[20];
> + unsigned int tx_fifo_depth;
size_t is a good type for keeping track of sizes.
> + unsigned int tx_fifo_width;
> + unsigned int rx_fifo_depth;
> + unsigned int tx_wm;
> + unsigned int rx_wm;
> + unsigned int rx_rfr;
size_t for sizes.
> + int xfer_mode;
> + bool port_setup;
> + unsigned int *rx_fifo;
The fifo is typeless, so it should be void *. It is however only ever
used as a local variable in handle_rx_console() so drop this.
> + int (*handle_rx)(struct uart_port *uport,
> + unsigned int rx_fifo_wc,
> + unsigned int rx_last_byte_valid,
> + unsigned int rx_last,
> + bool drop_rx);
> + struct device *wrapper_dev;
> + struct geni_se_rsc serial_rsc;
> + unsigned int xmit_size;
size_t for sizes.
> + void *rx_buf;
> + unsigned int cur_baud;
> +};
> +
> +static const struct uart_ops qcom_geni_serial_pops;
> +static struct uart_driver qcom_geni_console_driver;
> +static int handle_rx_console(struct uart_port *uport,
> + unsigned int rx_fifo_wc,
> + unsigned int rx_last_byte_valid,
> + unsigned int rx_last,
> + bool drop_rx);
> +static unsigned int qcom_geni_serial_tx_empty(struct uart_port *port);
> +static int qcom_geni_serial_poll_bit(struct uart_port *uport,
> + int offset, int bit_field, bool set);
> +static void qcom_geni_serial_stop_rx(struct uart_port *uport);
> +
> +static atomic_t uart_line_id = ATOMIC_INIT(0);
> +
> +#define GET_DEV_PORT(uport) \
> + container_of(uport, struct qcom_geni_serial_port, uport)
The idiomatic name for this macro would be something like "to_dev_port".
The use of "uport" as macro parameter makes this only work if the
parameter is "uport", otherwise the macro will replace the last part of
the container_of as well.
> +
> +static struct qcom_geni_serial_port qcom_geni_console_port;
> +
> +static void qcom_geni_serial_config_port(struct uart_port *uport, int cfg_flags)
> +{
> + if (cfg_flags & UART_CONFIG_TYPE)
> + uport->type = PORT_MSM;
> +}
> +
> +static unsigned int qcom_geni_cons_get_mctrl(struct uart_port *uport)
> +{
> + return TIOCM_DSR | TIOCM_CAR | TIOCM_CTS;
> +}
> +
> +static void qcom_geni_cons_set_mctrl(struct uart_port *uport,
> + unsigned int mctrl)
> +{
> +}
> +
> +static const char *qcom_geni_serial_get_type(struct uart_port *uport)
> +{
> + return "MSM";
> +}
> +
> +static struct qcom_geni_serial_port *get_port_from_line(int line)
> +{
> + struct qcom_geni_serial_port *port = NULL;
> +
> + if ((line < 0) || (line >= GENI_UART_CONS_PORTS))
> + port = ERR_PTR(-ENXIO);
You need to return here as well...
> + port = &qcom_geni_console_port;
> + return port;
Drop the "port" and just return ERR_PTR(-ENXIO) in the error case and
return &qcom_geni_serial_port otherwise.
> +}
> +
> +static int qcom_geni_serial_poll_bit(struct uart_port *uport,
> + int offset, int bit_field, bool set)
This function is returning "yes" or "no", so make it return "bool"
> +{
> + int iter = 0;
> + unsigned int reg;
> + bool met = false;
> + struct qcom_geni_serial_port *port = NULL;
> + bool cond = false;
> + unsigned int baud = 115200;
> + unsigned int fifo_bits = DEF_FIFO_DEPTH_WORDS * DEF_FIFO_WIDTH_BITS;
> + unsigned long total_iter = 2000;
Don't initialize things that will be assigned directly again.
> +
> +
> + if (uport->private_data) {
> + port = GET_DEV_PORT(uport);
> + baud = (port->cur_baud ? port->cur_baud : 115200);
> + fifo_bits = port->tx_fifo_depth * port->tx_fifo_width;
> + /*
> + * Total polling iterations based on FIFO worth of bytes to be
> + * sent at current baud .Add a little fluff to the wait.
> + */
> + total_iter = ((fifo_bits * USEC_PER_SEC) / baud) / 10;
> + total_iter += 50;
> + }
> +
> + while (iter < total_iter) {
> + reg = geni_read_reg_nolog(uport->membase, offset);
> + cond = reg & bit_field;
> + if (cond == set) {
> + met = true;
> + break;
> + }
> + udelay(10);
> + iter++;
> + }
Use readl_poll_timeout() instead of all this.
> + return met;
> +}
> +
> +static void qcom_geni_serial_setup_tx(struct uart_port *uport,
> + unsigned int xmit_size)
> +{
> + u32 m_cmd = 0;
Don't initialize this.
> +
> + geni_write_reg_nolog(xmit_size, uport->membase, SE_UART_TX_TRANS_LEN);
> + m_cmd |= (UART_START_TX << M_OPCODE_SHFT);
Why OR this into 0?
> + geni_write_reg_nolog(m_cmd, uport->membase, SE_GENI_M_CMD0);
> + /*
> + * Writes to enable the primary sequencer should go through before
> + * exiting this function.
> + */
> + mb();
> +}
> +
> +static void qcom_geni_serial_poll_cancel_tx(struct uart_port *uport)
This function polls for command done and if that doesn't happen in time
it aborts the command. It then clear any interrupts. The function name
however indicates that we're polling for "cancel of tx".
> +{
> + int done = 0;
> + unsigned int irq_clear = M_CMD_DONE_EN;
> + unsigned int geni_status = 0;
Don't initialize done and geni_status.
> +
> + done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_DONE_EN, true);
> + if (!done) {
> + geni_write_reg_nolog(M_GENI_CMD_ABORT, uport->membase,
> + SE_GENI_M_CMD_CTRL_REG);
> + owr++;
What's owr?
> + irq_clear |= M_CMD_ABORT_EN;
> + qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_ABORT_EN, true);
> + }
> + geni_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_STATUS);
> + if (geni_status & M_GENI_CMD_ACTIVE)
> + owr++;
> + geni_write_reg_nolog(irq_clear, uport->membase, SE_GENI_M_IRQ_CLEAR);
> +}
> +
> +static void qcom_geni_serial_abort_rx(struct uart_port *uport)
> +{
> + unsigned int irq_clear = S_CMD_DONE_EN;
> +
> + geni_se_abort_s_cmd(uport->membase);
> + /* Ensure this goes through before polling. */
> + mb();
> + irq_clear |= S_CMD_ABORT_EN;
> + qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
> + S_GENI_CMD_ABORT, false);
> + geni_write_reg_nolog(irq_clear, uport->membase, SE_GENI_S_IRQ_CLEAR);
> + geni_write_reg(FORCE_DEFAULT, uport->membase, GENI_FORCE_DEFAULT_REG);
> +}
> +
> +#ifdef CONFIG_CONSOLE_POLL
> +static int qcom_geni_serial_get_char(struct uart_port *uport)
> +{
> + unsigned int rx_fifo;
> + unsigned int m_irq_status;
> + unsigned int s_irq_status;
These are u32 and the variable names would benefit from being shorter.
> +
> + if (!(qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_SEC_IRQ_EN, true)))
Drop one parenthesis.
> + return -ENXIO;
> +
> + m_irq_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_M_IRQ_STATUS);
Drop the _nolog and rename the variable to reduce the length of this
line.
> + s_irq_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_S_IRQ_STATUS);
> + geni_write_reg_nolog(m_irq_status, uport->membase,
> + SE_GENI_M_IRQ_CLEAR);
> + geni_write_reg_nolog(s_irq_status, uport->membase,
> + SE_GENI_S_IRQ_CLEAR);
Is it necessary to do this interlaced? Or can you just clear M and then
clear S? I.e. have a single variable named "status".
> +
> + if (!(qcom_geni_serial_poll_bit(uport, SE_GENI_RX_FIFO_STATUS,
> + RX_FIFO_WC_MSK, true)))
> + return -ENXIO;
> +
> + /*
> + * Read the Rx FIFO only after clearing the interrupt registers and
> + * getting valid RX fifo status.
> + */
> + mb();
> + rx_fifo = geni_read_reg_nolog(uport->membase, SE_GENI_RX_FIFOn);
> + rx_fifo &= 0xFF;
return rx_fifo & 0xff; instead of the extra step
> + return rx_fifo;
> +}
> +
> +static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
> + unsigned char c)
> +{
> + int b = (int) c;
Why create this alias?
> + struct qcom_geni_serial_port *port = GET_DEV_PORT(uport);
> +
> + geni_write_reg_nolog(port->tx_wm, uport->membase,
> + SE_GENI_TX_WATERMARK_REG);
> + qcom_geni_serial_setup_tx(uport, 1);
> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_TX_FIFO_WATERMARK_EN, true))
> + WARN_ON(1);
WARN_ON(!qcom_geni_serial_poll_bit(...));
> + geni_write_reg_nolog(b, uport->membase, SE_GENI_TX_FIFOn);
> + geni_write_reg_nolog(M_TX_FIFO_WATERMARK_EN, uport->membase,
> + SE_GENI_M_IRQ_CLEAR);
> + /*
> + * Ensure FIFO write goes through before polling for status but.
> + */
/* This fits in a one-line comment */
But if this is necessary that means that every time you call
serial_poll() you need to have this comment and a rmb(). Better move it
into the poll function then - or just stop using _relaxed()
> + mb();
> + qcom_geni_serial_poll_cancel_tx(uport);
> +}
> +#endif
> +
> +#if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL)
> +static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
> +{
> + geni_write_reg_nolog(ch, uport->membase, SE_GENI_TX_FIFOn);
> + /*
> + * Ensure FIFO write clear goes through before
> + * next iteration.
This comment is accurate, but unnecessarily line wrapped.
> + */
> + mb();
> +
> +}
> +
> +static void
> +__qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
> + unsigned int count)
> +{
> + int new_line = 0;
> + int i;
> + int bytes_to_send = count;
> + int fifo_depth = DEF_FIFO_DEPTH_WORDS;
> + int tx_wm = DEF_TX_WM;
> +
> + for (i = 0; i < count; i++) {
> + if (s[i] == '\n')
> + new_line++;
> + }
Why do we need to deal with this?
> +
> + bytes_to_send += new_line;
> + geni_write_reg_nolog(tx_wm, uport->membase,
> + SE_GENI_TX_WATERMARK_REG);
> + qcom_geni_serial_setup_tx(uport, bytes_to_send);
> + i = 0;
> + while (i < count) {
> + u32 chars_to_write = 0;
> + u32 avail_fifo_bytes = (fifo_depth - tx_wm);
Use size_t for these.
> +
> + /*
> + * If the WM bit never set, then the Tx state machine is not
> + * in a valid state, so break, cancel/abort any existing
> + * command. Unfortunately the current data being written is
> + * lost.
> + */
> + while (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_TX_FIFO_WATERMARK_EN, true))
> + break;
> + chars_to_write = min((unsigned int)(count - i),
> + avail_fifo_bytes);
> + if ((chars_to_write << 1) > avail_fifo_bytes)
Write chars_to_write * 2, the compiler will be cleaver for you.
> + chars_to_write = (avail_fifo_bytes >> 1);
> + uart_console_write(uport, (s + i), chars_to_write,
> + qcom_geni_serial_wr_char);
> + geni_write_reg_nolog(M_TX_FIFO_WATERMARK_EN, uport->membase,
> + SE_GENI_M_IRQ_CLEAR);
> + /* Ensure this goes through before polling for WM IRQ again.*/
> + mb();
Again, if this is necessary move it into the poll function instead of
sprinkling it throughout the driver.
> + i += chars_to_write;
> + }
> + qcom_geni_serial_poll_cancel_tx(uport);
> +}
> +
> +static void qcom_geni_serial_console_write(struct console *co, const char *s,
> + unsigned int count)
> +{
> + struct uart_port *uport;
> + struct qcom_geni_serial_port *port;
> + int locked = 1;
Use bool for booleans
> + unsigned long flags;
> +
> + WARN_ON(co->index < 0 || co->index >= GENI_UART_NR_PORTS);
> +
> + port = get_port_from_line(co->index);
> + if (IS_ERR_OR_NULL(port))
port can't be NULL.
> + return;
> +
> + uport = &port->uport;
> + if (oops_in_progress)
> + locked = spin_trylock_irqsave(&uport->lock, flags);
> + else
> + spin_lock_irqsave(&uport->lock, flags);
> +
> + if (locked) {
> + __qcom_geni_serial_console_write(uport, s, count);
> + spin_unlock_irqrestore(&uport->lock, flags);
> + }
> +}
> +
> +static int handle_rx_console(struct uart_port *uport,
> + unsigned int rx_fifo_wc,
> + unsigned int rx_last_byte_valid,
> + unsigned int rx_last,
> + bool drop_rx)
> +{
> + int i, c;
> + unsigned char *rx_char;
> + struct tty_port *tport;
> + struct qcom_geni_serial_port *qcom_port = GET_DEV_PORT(uport);
> +
> + tport = &uport->state->port;
> + for (i = 0; i < rx_fifo_wc; i++) {
> + int bytes = 4;
> +
> + *(qcom_port->rx_fifo) =
> + geni_read_reg_nolog(uport->membase, SE_GENI_RX_FIFOn);
Just store this value in a local variable, as the only consumer is 3
lines down.
> + if (drop_rx)
> + continue;
> + rx_char = (unsigned char *)qcom_port->rx_fifo;
> +
> + if (i == (rx_fifo_wc - 1)) {
> + if (rx_last && rx_last_byte_valid)
> + bytes = rx_last_byte_valid;
> + }
> + for (c = 0; c < bytes; c++) {
> + char flag = TTY_NORMAL;
No need for a local variable here.
> + int sysrq;
> +
> + uport->icount.rx++;
> + sysrq = uart_handle_sysrq_char(uport, rx_char[c]);
> + if (!sysrq)
> + tty_insert_flip_char(tport, rx_char[c], flag);
> + }
> + }
> + if (!drop_rx)
> + tty_flip_buffer_push(tport);
> + return 0;
> +}
> +#else
> +static int handle_rx_console(struct uart_port *uport,
> + unsigned int rx_fifo_wc,
> + unsigned int rx_last_byte_valid,
> + unsigned int rx_last,
> + bool drop_rx)
> +{
> + return -EPERM;
> +}
> +
> +#endif /* (CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL)) */
> +
> +static void qcom_geni_serial_start_tx(struct uart_port *uport)
> +{
> + unsigned int geni_m_irq_en;
This is a u32, name it "irq_en"
> + struct qcom_geni_serial_port *qcom_port = GET_DEV_PORT(uport);
> + unsigned int geni_status;
geni_status should be of type u32 and there's no lack of descriptiveness
naming it "status".
> +
> + if (qcom_port->xfer_mode == FIFO_MODE) {
> + geni_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_STATUS);
> + if (geni_status & M_GENI_CMD_ACTIVE)
> + goto exit_start_tx;
Just return
> +
> + if (!qcom_geni_serial_tx_empty(uport))
> + goto exit_start_tx;
Ditto
> +
> + geni_m_irq_en = geni_read_reg_nolog(uport->membase,
> + SE_GENI_M_IRQ_EN);
> + geni_m_irq_en |= (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN);
> +
> + geni_write_reg_nolog(qcom_port->tx_wm, uport->membase,
> + SE_GENI_TX_WATERMARK_REG);
> + geni_write_reg_nolog(geni_m_irq_en, uport->membase,
> + SE_GENI_M_IRQ_EN);
> + /* Geni command setup should complete before returning.*/
If that's the case then verify that it's actually done.
> + mb();
> + }
> +exit_start_tx:
> + return;
> +}
> +
> +static void stop_tx_sequencer(struct uart_port *uport)
> +{
> + unsigned int geni_m_irq_en;
> + unsigned int geni_status;
These are u32 and can be given more succinct names.
> + struct qcom_geni_serial_port *port = GET_DEV_PORT(uport);
> +
> + geni_m_irq_en = geni_read_reg_nolog(uport->membase, SE_GENI_M_IRQ_EN);
Drop the _nolog from all of these to make code easier to read.
> + geni_m_irq_en &= ~M_CMD_DONE_EN;
> + if (port->xfer_mode == FIFO_MODE) {
> + geni_m_irq_en &= ~M_TX_FIFO_WATERMARK_EN;
> + geni_write_reg_nolog(0, uport->membase,
> + SE_GENI_TX_WATERMARK_REG);
> + }
> + port->xmit_size = 0;
> + geni_write_reg_nolog(geni_m_irq_en, uport->membase, SE_GENI_M_IRQ_EN);
> + geni_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_STATUS);
> + /* Possible stop tx is called multiple times. */
> + if (!(geni_status & M_GENI_CMD_ACTIVE))
> + return;
> +
> + geni_se_cancel_m_cmd(uport->membase);
> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_CANCEL_EN, true)) {
> + geni_se_abort_m_cmd(uport->membase);
> + qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_ABORT_EN, true);
> + geni_write_reg_nolog(M_CMD_ABORT_EN, uport->membase,
> + SE_GENI_M_IRQ_CLEAR);
> + }
> + geni_write_reg_nolog(M_CMD_CANCEL_EN, uport, SE_GENI_M_IRQ_CLEAR);
> +}
> +
> +static void qcom_geni_serial_stop_tx(struct uart_port *uport)
> +{
> + stop_tx_sequencer(uport);
> +}
> +
> +static void start_rx_sequencer(struct uart_port *uport)
> +{
> + unsigned int geni_s_irq_en;
> + unsigned int geni_m_irq_en;
> + unsigned int geni_status;
> + struct qcom_geni_serial_port *port = GET_DEV_PORT(uport);
> +
> + geni_status = geni_read_reg_nolog(uport->membase, SE_GENI_STATUS);
> + if (geni_status & S_GENI_CMD_ACTIVE)
> + qcom_geni_serial_stop_rx(uport);
> +
> + geni_se_setup_s_cmd(uport->membase, UART_START_READ, 0);
> +
> + if (port->xfer_mode == FIFO_MODE) {
> + geni_s_irq_en = geni_read_reg_nolog(uport->membase,
> + SE_GENI_S_IRQ_EN);
> + geni_m_irq_en = geni_read_reg_nolog(uport->membase,
> + SE_GENI_M_IRQ_EN);
> +
> + geni_s_irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
> + geni_m_irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
> +
> + geni_write_reg_nolog(geni_s_irq_en, uport->membase,
> + SE_GENI_S_IRQ_EN);
> + geni_write_reg_nolog(geni_m_irq_en, uport->membase,
> + SE_GENI_M_IRQ_EN);
> + }
> + /*
> + * Ensure the writes to the secondary sequencer and interrupt enables
> + * go through.
This is not what mb() does.
> + */
> + mb();
> + geni_status = geni_read_reg_nolog(uport->membase, SE_GENI_STATUS);
> +}
> +
> +static void qcom_geni_serial_start_rx(struct uart_port *uport)
> +{
> + start_rx_sequencer(uport);
> +}
> +
> +static void stop_rx_sequencer(struct uart_port *uport)
> +{
> + unsigned int geni_s_irq_en;
> + unsigned int geni_m_irq_en;
> + unsigned int geni_status;
> + struct qcom_geni_serial_port *port = GET_DEV_PORT(uport);
> + u32 irq_clear = S_CMD_DONE_EN;
> + bool done;
> +
> + if (port->xfer_mode == FIFO_MODE) {
> + geni_s_irq_en = geni_read_reg_nolog(uport->membase,
> + SE_GENI_S_IRQ_EN);
> + geni_m_irq_en = geni_read_reg_nolog(uport->membase,
> + SE_GENI_M_IRQ_EN);
> + geni_s_irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN);
> + geni_m_irq_en &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> +
> + geni_write_reg_nolog(geni_s_irq_en, uport->membase,
> + SE_GENI_S_IRQ_EN);
> + geni_write_reg_nolog(geni_m_irq_en, uport->membase,
> + SE_GENI_M_IRQ_EN);
> + }
> +
> + geni_status = geni_read_reg_nolog(uport->membase, SE_GENI_STATUS);
> + /* Possible stop rx is called multiple times. */
> + if (!(geni_status & S_GENI_CMD_ACTIVE))
> + return;
> + geni_se_cancel_s_cmd(uport->membase);
> + /*
> + * Ensure that the cancel goes through before polling for the
> + * cancel control bit.
> + */
> + mb();
> + done = qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
> + S_GENI_CMD_CANCEL, false);
> + geni_status = geni_read_reg_nolog(uport->membase, SE_GENI_STATUS);
> + geni_write_reg_nolog(irq_clear, uport->membase, SE_GENI_S_IRQ_CLEAR);
> + if ((geni_status & S_GENI_CMD_ACTIVE))
> + qcom_geni_serial_abort_rx(uport);
> +}
> +
> +static void qcom_geni_serial_stop_rx(struct uart_port *uport)
> +{
> + stop_rx_sequencer(uport);
Just inline the function here.
> +}
> +
> +static int qcom_geni_serial_handle_rx(struct uart_port *uport, bool drop_rx)
> +{
> + int ret = 0;
> + unsigned int rx_fifo_status;
> + unsigned int rx_fifo_wc = 0;
> + unsigned int rx_last_byte_valid = 0;
> + unsigned int rx_last = 0;
The context of this function gives that we're dealing with rx and
rx_fifo, so no need to specify that in each local variable.
Also, they should all be u32 and there's no need to initialize them.
> + struct tty_port *tport;
> + struct qcom_geni_serial_port *port = GET_DEV_PORT(uport);
> +
> + tport = &uport->state->port;
> + rx_fifo_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_RX_FIFO_STATUS);
> + rx_fifo_wc = rx_fifo_status & RX_FIFO_WC_MSK;
> + rx_last_byte_valid = ((rx_fifo_status & RX_LAST_BYTE_VALID_MSK) >>
> + RX_LAST_BYTE_VALID_SHFT);
> + rx_last = rx_fifo_status & RX_LAST;
> + if (rx_fifo_wc)
> + port->handle_rx(uport, rx_fifo_wc, rx_last_byte_valid,
> + rx_last, drop_rx);
You're ignoring the return value of handle_rx.
> + return ret;
You don't care about the return value in the caller and you always
return 0 anyways, so make the function void.
> +}
> +
> +static int qcom_geni_serial_handle_tx(struct uart_port *uport)
> +{
> + int ret = 0;
> + struct qcom_geni_serial_port *qcom_port = GET_DEV_PORT(uport);
> + struct circ_buf *xmit = &uport->state->xmit;
> + unsigned int avail_fifo_bytes = 0;
Naming avail_fifo_bytes just "avail" is carrying the same information
but is shorter.
> + unsigned int bytes_remaining = 0;
> + int i = 0;
> + unsigned int tx_fifo_status;
> + unsigned int xmit_size;
> + unsigned int fifo_width_bytes = 1;
If this really is constantly 1 you can remove the variable and remove a
bit of the complexity in this function.
> + int temp_tail = 0;
Use size_t for sizes.
> +
> + xmit_size = uart_circ_chars_pending(xmit);
> + tx_fifo_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_TX_FIFO_STATUS);
> + /* Both FIFO and framework buffer are drained */
> + if ((xmit_size == qcom_port->xmit_size) && !tx_fifo_status) {
Drop the parenthesis.
> + qcom_port->xmit_size = 0;
> + uart_circ_clear(xmit);
> + qcom_geni_serial_stop_tx(uport);
> + goto exit_handle_tx;
> + }
> + xmit_size -= qcom_port->xmit_size;
> +
> + avail_fifo_bytes = (qcom_port->tx_fifo_depth - qcom_port->tx_wm) *
> + fifo_width_bytes;
> + temp_tail = (xmit->tail + qcom_port->xmit_size) & (UART_XMIT_SIZE - 1);
> + if (xmit_size > (UART_XMIT_SIZE - temp_tail))
> + xmit_size = (UART_XMIT_SIZE - temp_tail);
> + if (xmit_size > avail_fifo_bytes)
> + xmit_size = avail_fifo_bytes;
> +
> + if (!xmit_size)
> + goto exit_handle_tx;
> +
> + qcom_geni_serial_setup_tx(uport, xmit_size);
> +
> + bytes_remaining = xmit_size;
> + while (i < xmit_size) {
> + unsigned int tx_bytes;
> + unsigned int buf = 0;
> + int c;
> +
> + tx_bytes = ((bytes_remaining < fifo_width_bytes) ?
> + bytes_remaining : fifo_width_bytes);
tx_bytes = min(remaining, fifo_width);
> +
> + for (c = 0; c < tx_bytes ; c++)
> + buf |= (xmit->buf[temp_tail + c] << (c * 8));
> + geni_write_reg_nolog(buf, uport->membase, SE_GENI_TX_FIFOn);
> + i += tx_bytes;
> + temp_tail = (temp_tail + tx_bytes) & (UART_XMIT_SIZE - 1);
> + uport->icount.tx += tx_bytes;
> + bytes_remaining -= tx_bytes;
> + /* Ensure FIFO write goes through */
> + wmb();
> + }
> + qcom_geni_serial_poll_cancel_tx(uport);
> + qcom_port->xmit_size += xmit_size;
> +exit_handle_tx:
> + uart_write_wakeup(uport);
> + return ret;
> +}
> +
> +static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
> +{
> + unsigned int m_irq_status;
> + unsigned int s_irq_status;
> + struct uart_port *uport = dev;
> + unsigned long flags;
> + unsigned int m_irq_en;
> + bool drop_rx = false;
> + struct tty_port *tport = &uport->state->port;
> +
> + spin_lock_irqsave(&uport->lock, flags);
> + if (uport->suspended)
> + goto exit_geni_serial_isr;
> + m_irq_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_M_IRQ_STATUS);
> + s_irq_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_S_IRQ_STATUS);
> + m_irq_en = geni_read_reg_nolog(uport->membase, SE_GENI_M_IRQ_EN);
> + geni_write_reg_nolog(m_irq_status, uport->membase, SE_GENI_M_IRQ_CLEAR);
> + geni_write_reg_nolog(s_irq_status, uport->membase, SE_GENI_S_IRQ_CLEAR);
> +
> + if ((m_irq_status & M_ILLEGAL_CMD_EN)) {
> + WARN_ON(1);
> + goto exit_geni_serial_isr;
> + }
> +
> + if (s_irq_status & S_RX_FIFO_WR_ERR_EN) {
> + uport->icount.overrun++;
> + tty_insert_flip_char(tport, 0, TTY_OVERRUN);
> + }
> +
> + if ((m_irq_status & m_irq_en) &
> + (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
> + qcom_geni_serial_handle_tx(uport);
> +
> + if ((s_irq_status & S_GP_IRQ_0_EN) || (s_irq_status & S_GP_IRQ_1_EN)) {
> + if (s_irq_status & S_GP_IRQ_0_EN)
> + uport->icount.parity++;
> + drop_rx = true;
> + } else if ((s_irq_status & S_GP_IRQ_2_EN) ||
> + (s_irq_status & S_GP_IRQ_3_EN)) {
> + uport->icount.brk++;
> + }
> +
> + if ((s_irq_status & S_RX_FIFO_WATERMARK_EN) ||
> + (s_irq_status & S_RX_FIFO_LAST_EN))
> + qcom_geni_serial_handle_rx(uport, drop_rx);
> +
> +exit_geni_serial_isr:
> + spin_unlock_irqrestore(&uport->lock, flags);
> + return IRQ_HANDLED;
> +}
> +
> +static int get_tx_fifo_size(struct qcom_geni_serial_port *port)
> +{
> + struct uart_port *uport;
> +
> + if (!port)
> + return -ENODEV;
> +
> + uport = &port->uport;
> + port->tx_fifo_depth = geni_se_get_tx_fifo_depth(uport->membase);
> + if (!port->tx_fifo_depth) {
> + dev_err(uport->dev, "%s:Invalid TX FIFO depth read\n",
> + __func__);
> + return -ENXIO;
> + }
> +
> + port->tx_fifo_width = geni_se_get_tx_fifo_width(uport->membase);
> + if (!port->tx_fifo_width) {
> + dev_err(uport->dev, "%s:Invalid TX FIFO width read\n",
> + __func__);
> + return -ENXIO;
> + }
> +
> + port->rx_fifo_depth = geni_se_get_rx_fifo_depth(uport->membase);
> + if (!port->rx_fifo_depth) {
> + dev_err(uport->dev, "%s:Invalid RX FIFO depth read\n",
> + __func__);
> + return -ENXIO;
> + }
> +
> + uport->fifosize =
> + ((port->tx_fifo_depth * port->tx_fifo_width) >> 3);
> + return 0;
> +}
> +
> +static void set_rfr_wm(struct qcom_geni_serial_port *port)
> +{
> + /*
> + * Set RFR (Flow off) to FIFO_DEPTH - 2.
> + * RX WM level at 10% RX_FIFO_DEPTH.
> + * TX WM level at 10% TX_FIFO_DEPTH.
> + */
> + port->rx_rfr = port->rx_fifo_depth - 2;
> + port->rx_wm = UART_CONSOLE_RX_WM;
> + port->tx_wm = 2;
> +}
> +
> +static void qcom_geni_serial_shutdown(struct uart_port *uport)
> +{
> + unsigned long flags;
> +
> + /* Stop the console before stopping the current tx */
> + console_stop(uport->cons);
> +
> + disable_irq(uport->irq);
> + free_irq(uport->irq, uport);
> + spin_lock_irqsave(&uport->lock, flags);
> + qcom_geni_serial_stop_tx(uport);
> + qcom_geni_serial_stop_rx(uport);
> + spin_unlock_irqrestore(&uport->lock, flags);
> +}
> +
> +static int qcom_geni_serial_port_setup(struct uart_port *uport)
> +{
> + int ret = 0;
> + struct qcom_geni_serial_port *qcom_port = GET_DEV_PORT(uport);
> + unsigned long cfg0, cfg1;
> + unsigned int rxstale = DEFAULT_BITS_PER_CHAR * STALE_TIMEOUT;
> +
> + set_rfr_wm(qcom_port);
> + geni_write_reg_nolog(rxstale, uport->membase, SE_UART_RX_STALE_CNT);
> + /*
> + * Make an unconditional cancel on the main sequencer to reset
> + * it else we could end up in data loss scenarios.
> + */
> + qcom_port->xfer_mode = FIFO_MODE;
> + qcom_geni_serial_poll_cancel_tx(uport);
> + geni_se_get_packing_config(8, 1, false, &cfg0, &cfg1);
> + geni_write_reg_nolog(cfg0, uport->membase,
> + SE_GENI_TX_PACKING_CFG0);
> + geni_write_reg_nolog(cfg1, uport->membase,
> + SE_GENI_TX_PACKING_CFG1);
> + geni_se_get_packing_config(8, 4, false, &cfg0, &cfg1);
> + geni_write_reg_nolog(cfg0, uport->membase,
> + SE_GENI_RX_PACKING_CFG0);
> + geni_write_reg_nolog(cfg1, uport->membase,
> + SE_GENI_RX_PACKING_CFG1);
> + ret = geni_se_init(uport->membase, qcom_port->rx_wm, qcom_port->rx_rfr);
> + if (ret) {
> + dev_err(uport->dev, "%s: Fail\n", __func__);
> + goto exit_portsetup;
> + }
> +
> + ret = geni_se_select_mode(uport->membase, qcom_port->xfer_mode);
> + if (ret)
> + goto exit_portsetup;
> +
> + qcom_port->port_setup = true;
> + /*
> + * Ensure Port setup related IO completes before returning to
> + * framework.
> + */
> + mb();
> +exit_portsetup:
> + return ret;
> +}
> +
> +static int qcom_geni_serial_startup(struct uart_port *uport)
> +{
> + int ret = 0;
> + struct qcom_geni_serial_port *qcom_port = GET_DEV_PORT(uport);
> +
> + scnprintf(qcom_port->name, sizeof(qcom_port->name),
> + "qcom_serial_geni%d", uport->line);
> +
> + if (unlikely(geni_se_get_proto(uport->membase) != UART)) {
Drop the unlikely(), in favour of readable code.
> + dev_err(uport->dev, "%s: Invalid FW %d loaded.\n",
> + __func__, geni_se_get_proto(uport->membase));
Drop the __func__, the message should describe the issue in itself.
> + ret = -ENXIO;
> + goto exit_startup;
We haven't done anything, so just return here.
> + }
> +
> + get_tx_fifo_size(qcom_port);
> + if (!qcom_port->port_setup) {
> + if (qcom_geni_serial_port_setup(uport))
> + goto exit_startup;
> + }
> +
> + /*
> + * Ensure that all the port configuration writes complete
> + * before returning to the framework.
That's not what mb() does.
> + */
> + mb();
> + ret = request_irq(uport->irq, qcom_geni_serial_isr, IRQF_TRIGGER_HIGH,
> + qcom_port->name, uport);
> + if (unlikely(ret)) {
> + dev_err(uport->dev, "%s: Failed to get IRQ ret %d\n",
> + __func__, ret);
Drop the __func__
> + goto exit_startup;
> + }
> +
> +exit_startup:
> + return ret;
> +}
> +
> +static int get_clk_cfg(unsigned long clk_freq, unsigned long *ser_clk)
> +{
> + unsigned long root_freq[] = {7372800, 14745600, 19200000, 29491200,
> + 32000000, 48000000, 64000000, 80000000, 96000000, 100000000};
> + int i;
> + int match = -1;
> +
> + for (i = 0; i < ARRAY_SIZE(root_freq); i++) {
> + if (clk_freq > root_freq[i])
> + continue;
> +
> + if (!(root_freq[i] % clk_freq)) {
> + match = i;
> + break;
> + }
> + }
> + if (match != -1)
> + *ser_clk = root_freq[match];
> + else
> + pr_err("clk_freq %ld\n", clk_freq);
> + return match;
> +}
> +
> +static void geni_serial_write_term_regs(struct uart_port *uport,
> + u32 tx_trans_cfg, u32 tx_parity_cfg, u32 rx_trans_cfg,
> + u32 rx_parity_cfg, u32 bits_per_char, u32 stop_bit_len,
> + u32 s_clk_cfg)
> +{
> + geni_write_reg_nolog(tx_trans_cfg, uport->membase,
> + SE_UART_TX_TRANS_CFG);
> + geni_write_reg_nolog(tx_parity_cfg, uport->membase,
> + SE_UART_TX_PARITY_CFG);
> + geni_write_reg_nolog(rx_trans_cfg, uport->membase,
> + SE_UART_RX_TRANS_CFG);
> + geni_write_reg_nolog(rx_parity_cfg, uport->membase,
> + SE_UART_RX_PARITY_CFG);
> + geni_write_reg_nolog(bits_per_char, uport->membase,
> + SE_UART_TX_WORD_LEN);
> + geni_write_reg_nolog(bits_per_char, uport->membase,
> + SE_UART_RX_WORD_LEN);
> + geni_write_reg_nolog(stop_bit_len, uport->membase,
> + SE_UART_TX_STOP_BIT_LEN);
> + geni_write_reg_nolog(s_clk_cfg, uport->membase, GENI_SER_M_CLK_CFG);
> + geni_write_reg_nolog(s_clk_cfg, uport->membase, GENI_SER_S_CLK_CFG);
> +}
> +
> +static int get_clk_div_rate(unsigned int baud, unsigned long *desired_clk_rate)
> +{
> + unsigned long ser_clk;
> + int dfs_index;
> + int clk_div = 0;
> +
> + *desired_clk_rate = baud * UART_OVERSAMPLING;
> + dfs_index = get_clk_cfg(*desired_clk_rate, &ser_clk);
> + if (dfs_index < 0) {
> + pr_err("%s: Can't find matching DFS entry for baud %d\n",
> + __func__, baud);
> + clk_div = -EINVAL;
> + goto exit_get_clk_div_rate;
> + }
> +
> + clk_div = ser_clk / *desired_clk_rate;
> + *desired_clk_rate = ser_clk;
> +exit_get_clk_div_rate:
> + return clk_div;
> +}
> +
> +static void qcom_geni_serial_set_termios(struct uart_port *uport,
> + struct ktermios *termios, struct ktermios *old)
> +{
> + unsigned int baud;
> + unsigned int bits_per_char = 0;
> + unsigned int tx_trans_cfg;
> + unsigned int tx_parity_cfg;
> + unsigned int rx_trans_cfg;
> + unsigned int rx_parity_cfg;
> + unsigned int stop_bit_len;
> + unsigned int clk_div;
> + unsigned long ser_clk_cfg = 0;
> + struct qcom_geni_serial_port *port = GET_DEV_PORT(uport);
> + unsigned long clk_rate;
> +
> + qcom_geni_serial_stop_rx(uport);
> + /* baud rate */
> + baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
> + port->cur_baud = baud;
> + clk_div = get_clk_div_rate(baud, &clk_rate);
> + if (clk_div <= 0)
> + goto exit_set_termios;
Name your labels based on their actions; if you name it "out_restart_rx"
you don't even have to look below to know what will happen when you
jump.
> +
> + uport->uartclk = clk_rate;
> + clk_set_rate(port->serial_rsc.se_clk, clk_rate);
> + ser_clk_cfg |= SER_CLK_EN;
> + ser_clk_cfg |= (clk_div << CLK_DIV_SHFT);
> +
> + /* parity */
> + tx_trans_cfg = geni_read_reg_nolog(uport->membase,
Drop the _nolog and you don't need to line wrap
> + SE_UART_TX_TRANS_CFG);
> + tx_parity_cfg = geni_read_reg_nolog(uport->membase,
> + SE_UART_TX_PARITY_CFG);
> + rx_trans_cfg = geni_read_reg_nolog(uport->membase,
> + SE_UART_RX_TRANS_CFG);
> + rx_parity_cfg = geni_read_reg_nolog(uport->membase,
> + SE_UART_RX_PARITY_CFG);
> + if (termios->c_cflag & PARENB) {
> + tx_trans_cfg |= UART_TX_PAR_EN;
> + rx_trans_cfg |= UART_RX_PAR_EN;
> + tx_parity_cfg |= PAR_CALC_EN;
> + rx_parity_cfg |= PAR_CALC_EN;
> + if (termios->c_cflag & PARODD) {
> + tx_parity_cfg |= PAR_ODD;
> + rx_parity_cfg |= PAR_ODD;
> + } else if (termios->c_cflag & CMSPAR) {
> + tx_parity_cfg |= PAR_SPACE;
> + rx_parity_cfg |= PAR_SPACE;
> + } else {
> + tx_parity_cfg |= PAR_EVEN;
> + rx_parity_cfg |= PAR_EVEN;
> + }
> + } else {
> + tx_trans_cfg &= ~UART_TX_PAR_EN;
> + rx_trans_cfg &= ~UART_RX_PAR_EN;
> + tx_parity_cfg &= ~PAR_CALC_EN;
> + rx_parity_cfg &= ~PAR_CALC_EN;
> + }
> +
> + /* bits per char */
> + switch (termios->c_cflag & CSIZE) {
> + case CS5:
> + bits_per_char = 5;
> + break;
> + case CS6:
> + bits_per_char = 6;
> + break;
> + case CS7:
> + bits_per_char = 7;
> + break;
> + case CS8:
> + default:
> + bits_per_char = 8;
> + break;
> + }
> +
> + /* stop bits */
> + if (termios->c_cflag & CSTOPB)
> + stop_bit_len = TX_STOP_BIT_LEN_2;
> + else
> + stop_bit_len = TX_STOP_BIT_LEN_1;
> +
> + /* flow control, clear the CTS_MASK bit if using flow control. */
> + if (termios->c_cflag & CRTSCTS)
> + tx_trans_cfg &= ~UART_CTS_MASK;
> + else
> + tx_trans_cfg |= UART_CTS_MASK;
> +
> + if (likely(baud))
> + uart_update_timeout(uport, termios->c_cflag, baud);
> +
> + geni_serial_write_term_regs(uport, tx_trans_cfg, tx_parity_cfg,
> + rx_trans_cfg, rx_parity_cfg, bits_per_char, stop_bit_len,
> + ser_clk_cfg);
> +exit_set_termios:
> + qcom_geni_serial_start_rx(uport);
> + return;
> +
> +}
> +
> +static unsigned int qcom_geni_serial_tx_empty(struct uart_port *uport)
> +{
> + unsigned int tx_fifo_status;
> + unsigned int is_tx_empty = 1;
> +
> + tx_fifo_status = geni_read_reg_nolog(uport->membase,
> + SE_GENI_TX_FIFO_STATUS);
Drop the "_nolog" and naming the result "status" would be just as
expressive, but keep you below 80 chars.
> + if (tx_fifo_status)
> + is_tx_empty = 0;
> +
> + return is_tx_empty;
Instead of what you're doing with is_tx_empty, just do:
return !readl(membase + FIFO_STATUS);
> +}
> +
> +#if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL)
> +static int __init qcom_geni_console_setup(struct console *co, char *options)
> +{
> + struct uart_port *uport;
> + struct qcom_geni_serial_port *dev_port;
> + int baud = 115200;
> + int bits = 8;
> + int parity = 'n';
> + int flow = 'n';
> + int ret = 0;
Drop initialization of all these variables.
> +
> + if (unlikely(co->index >= GENI_UART_NR_PORTS || co->index < 0))
Drop the unlikely.
> + return -ENXIO;
> +
> + dev_port = get_port_from_line(co->index);
> + if (IS_ERR_OR_NULL(dev_port)) {
> + ret = PTR_ERR(dev_port);
> + pr_err("Invalid line %d(%d)\n", co->index, ret);
> + return ret;
> + }
> +
> + uport = &dev_port->uport;
> +
> + if (unlikely(!uport->membase))
> + return -ENXIO;
> +
> + if (geni_se_resources_on(&dev_port->serial_rsc))
> + WARN_ON(1);
> +
> + if (unlikely(geni_se_get_proto(uport->membase) != UART)) {
> + geni_se_resources_off(&dev_port->serial_rsc);
> + return -ENXIO;
> + }
> +
> + if (!dev_port->port_setup) {
> + qcom_geni_serial_stop_rx(uport);
> + qcom_geni_serial_port_setup(uport);
> + }
> +
> + if (options)
> + uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> + return uart_set_options(uport, co, baud, parity, bits, flow);
> +}
> +
> +static int console_register(struct uart_driver *drv)
> +{
> + return uart_register_driver(drv);
> +}
> +static void console_unregister(struct uart_driver *drv)
> +{
> + uart_unregister_driver(drv);
> +}
> +
> +static struct console cons_ops = {
> + .name = "ttyMSM",
> + .write = qcom_geni_serial_console_write,
> + .device = uart_console_device,
> + .setup = qcom_geni_console_setup,
> + .flags = CON_PRINTBUFFER,
> + .index = -1,
> + .data = &qcom_geni_console_driver,
> +};
> +
> +static struct uart_driver qcom_geni_console_driver = {
> + .owner = THIS_MODULE,
> + .driver_name = "qcom_geni_console",
> + .dev_name = "ttyMSM",
> + .nr = GENI_UART_NR_PORTS,
> + .cons = &cons_ops,
> +};
> +#else
> +static int console_register(struct uart_driver *drv)
> +{
> + return 0;
> +}
> +
> +static void console_unregister(struct uart_driver *drv)
> +{
> +}
> +#endif /* defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL) */
> +
> +static void qcom_geni_serial_cons_pm(struct uart_port *uport,
> + unsigned int new_state, unsigned int old_state)
> +{
> + struct qcom_geni_serial_port *qcom_port = GET_DEV_PORT(uport);
> +
> + if (unlikely(!uart_console(uport)))
> + return;
> +
> + if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
> + geni_se_resources_on(&qcom_port->serial_rsc);
> + else if (new_state == UART_PM_STATE_OFF &&
> + old_state == UART_PM_STATE_ON)
> + geni_se_resources_off(&qcom_port->serial_rsc);
> +}
> +
> +static const struct uart_ops qcom_geni_console_pops = {
> + .tx_empty = qcom_geni_serial_tx_empty,
> + .stop_tx = qcom_geni_serial_stop_tx,
> + .start_tx = qcom_geni_serial_start_tx,
> + .stop_rx = qcom_geni_serial_stop_rx,
> + .set_termios = qcom_geni_serial_set_termios,
> + .startup = qcom_geni_serial_startup,
> + .config_port = qcom_geni_serial_config_port,
> + .shutdown = qcom_geni_serial_shutdown,
> + .type = qcom_geni_serial_get_type,
> + .set_mctrl = qcom_geni_cons_set_mctrl,
> + .get_mctrl = qcom_geni_cons_get_mctrl,
> +#ifdef CONFIG_CONSOLE_POLL
> + .poll_get_char = qcom_geni_serial_get_char,
> + .poll_put_char = qcom_geni_serial_poll_put_char,
> +#endif
> + .pm = qcom_geni_serial_cons_pm,
> +};
> +
> +static const struct of_device_id qcom_geni_serial_match_table[] = {
> + { .compatible = "qcom,geni-debug-uart",
> + .data = (void *)&qcom_geni_console_driver},
Shouldn't need this typecast.
> +};
> +
> +static int qcom_geni_serial_probe(struct platform_device *pdev)
> +{
> + int ret = 0;
> + int line = -1;
> + struct qcom_geni_serial_port *dev_port;
> + struct uart_port *uport;
> + struct resource *res;
> + struct uart_driver *drv;
> + const struct of_device_id *id;
> +
> + id = of_match_device(qcom_geni_serial_match_table, &pdev->dev);
Use of_device_get_match_data()
> + if (id) {
> + dev_dbg(&pdev->dev, "%s: %s\n", __func__, id->compatible);
> + drv = (struct uart_driver *)id->data;
> + } else {
> + dev_err(&pdev->dev, "%s: No matching device found", __func__);
> + return -ENODEV;
> + }
> +
> + if (pdev->dev.of_node) {
> + if (drv->cons)
> + line = of_alias_get_id(pdev->dev.of_node, "serial");
> + } else {
> + line = pdev->id;
> + }
> +
> + if (line < 0)
> + line = atomic_inc_return(&uart_line_id) - 1;
> +
> + if ((line < 0) || (line >= GENI_UART_CONS_PORTS))
> + return -ENXIO;
> + dev_port = get_port_from_line(line);
> + if (IS_ERR_OR_NULL(dev_port)) {
> + ret = PTR_ERR(dev_port);
> + dev_err(&pdev->dev, "Invalid line %d(%d)\n",
> + line, ret);
> + goto exit_geni_serial_probe;
> + }
> +
> + uport = &dev_port->uport;
> +
> + /* Don't allow 2 drivers to access the same port */
> + if (uport->private_data) {
> + ret = -ENODEV;
> + goto exit_geni_serial_probe;
> + }
> +
> + uport->dev = &pdev->dev;
> + dev_port->wrapper_dev = pdev->dev.parent;
> + dev_port->serial_rsc.wrapper_dev = pdev->dev.parent;
> + dev_port->serial_rsc.se_clk = devm_clk_get(&pdev->dev, "se-clk");
> + if (IS_ERR(dev_port->serial_rsc.se_clk)) {
> + ret = PTR_ERR(dev_port->serial_rsc.se_clk);
> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
> + goto exit_geni_serial_probe;
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "se-phys");
Don't grab this by name, just pick the first one.
> + if (!res) {
> + ret = -ENXIO;
> + dev_err(&pdev->dev, "Err getting IO region\n");
> + goto exit_geni_serial_probe;
> + }
Drop the error handling here.
> + uport->mapbase = res->start;
No
> + uport->membase = devm_ioremap(&pdev->dev, res->start,
> + resource_size(res));
> + if (!uport->membase) {
> + ret = -ENOMEM;
> + dev_err(&pdev->dev, "Err IO mapping serial iomem");
> + goto exit_geni_serial_probe;
> + }
> +
> + dev_port->serial_rsc.geni_pinctrl = devm_pinctrl_get(&pdev->dev);
Drop all of the pinctrl stuff.
> + if (IS_ERR_OR_NULL(dev_port->serial_rsc.geni_pinctrl)) {
> + dev_err(&pdev->dev, "No pinctrl config specified!\n");
> + ret = PTR_ERR(dev_port->serial_rsc.geni_pinctrl);
> + goto exit_geni_serial_probe;
> + }
> + dev_port->serial_rsc.geni_gpio_active =
> + pinctrl_lookup_state(dev_port->serial_rsc.geni_pinctrl,
> + PINCTRL_DEFAULT);
> + if (IS_ERR_OR_NULL(dev_port->serial_rsc.geni_gpio_active)) {
> + dev_err(&pdev->dev, "No default config specified!\n");
> + ret = PTR_ERR(dev_port->serial_rsc.geni_gpio_active);
> + goto exit_geni_serial_probe;
> + }
> +
> + dev_port->serial_rsc.geni_gpio_sleep =
> + pinctrl_lookup_state(dev_port->serial_rsc.geni_pinctrl,
> + PINCTRL_SLEEP);
> + if (IS_ERR_OR_NULL(dev_port->serial_rsc.geni_gpio_sleep)) {
> + dev_err(&pdev->dev, "No sleep config specified!\n");
> + ret = PTR_ERR(dev_port->serial_rsc.geni_gpio_sleep);
> + goto exit_geni_serial_probe;
> + }
> +
> + dev_port->tx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> + dev_port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> + dev_port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
> + uport->fifosize =
> + ((dev_port->tx_fifo_depth * dev_port->tx_fifo_width) >> 3);
> +
> + uport->irq = platform_get_irq(pdev, 0);
> + if (uport->irq < 0) {
> + ret = uport->irq;
> + dev_err(&pdev->dev, "Failed to get IRQ %d\n", ret);
> + goto exit_geni_serial_probe;
> + }
> +
> + uport->private_data = (void *)drv;
> + platform_set_drvdata(pdev, dev_port);
> + dev_port->handle_rx = handle_rx_console;
Why have a function pointer when you're only referencing a fixed
function.
> + dev_port->rx_fifo = devm_kzalloc(uport->dev, sizeof(u32),
> + GFP_KERNEL);
It takes 8 bytes to store a pointer and the allocation will have some
overhead...just to provide storage space for 4 bytes. And you don't have
any error handling...
> + dev_port->port_setup = false;
> + return uart_add_one_port(drv, uport);
> +
> +exit_geni_serial_probe:
> + return ret;
> +}
> +
> +static int qcom_geni_serial_remove(struct platform_device *pdev)
> +{
> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> + struct uart_driver *drv =
> + (struct uart_driver *)port->uport.private_data;
> +
> + uart_remove_one_port(drv, &port->uport);
> + return 0;
> +}
> +
> +
> +#ifdef CONFIG_PM
> +static int qcom_geni_serial_sys_suspend_noirq(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> + struct uart_port *uport = &port->uport;
> +
> + uart_suspend_port((struct uart_driver *)uport->private_data,
> + uport);
> + return 0;
> +}
> +
> +static int qcom_geni_serial_sys_resume_noirq(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> + struct uart_port *uport = &port->uport;
> +
> + if (console_suspend_enabled && uport->suspended) {
> + uart_resume_port((struct uart_driver *)uport->private_data,
> + uport);
> + disable_irq(uport->irq);
> + }
> + return 0;
> +}
> +#else
> +static int qcom_geni_serial_sys_suspend_noirq(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static int qcom_geni_serial_sys_resume_noirq(struct device *dev)
> +{
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
> + .suspend_noirq = qcom_geni_serial_sys_suspend_noirq,
> + .resume_noirq = qcom_geni_serial_sys_resume_noirq,
> +};
> +
> +static struct platform_driver qcom_geni_serial_platform_driver = {
> + .remove = qcom_geni_serial_remove,
> + .probe = qcom_geni_serial_probe,
> + .driver = {
> + .name = "qcom_geni_serial",
> + .of_match_table = qcom_geni_serial_match_table,
> + .pm = &qcom_geni_serial_pm_ops,
> + },
> +};
> +
> +static int __init qcom_geni_serial_init(void)
> +{
> + int ret = 0;
> + int i;
> +
> + for (i = 0; i < GENI_UART_CONS_PORTS; i++) {
> + qcom_geni_console_port.uport.iotype = UPIO_MEM;
> + qcom_geni_console_port.uport.ops = &qcom_geni_console_pops;
> + qcom_geni_console_port.uport.flags = UPF_BOOT_AUTOCONF;
> + qcom_geni_console_port.uport.line = i;
> + }
> +
> + ret = console_register(&qcom_geni_console_driver);
> + if (ret)
> + return ret;
> +
> + ret = platform_driver_register(&qcom_geni_serial_platform_driver);
> + if (ret) {
> + console_unregister(&qcom_geni_console_driver);
> + return ret;
> + }
> + return ret;
> +}
> +module_init(qcom_geni_serial_init);
> +
> +static void __exit qcom_geni_serial_exit(void)
> +{
> + platform_driver_unregister(&qcom_geni_serial_platform_driver);
> + console_unregister(&qcom_geni_console_driver);
> +}
> +module_exit(qcom_geni_serial_exit);
> +
> +MODULE_DESCRIPTION("Serial driver for GENI based QUP cores");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("tty:qcom_geni_serial");
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 3/7] soc: qcom: Add GENI based QUP Wrapper driver
From: Bjorn Andersson @ 2018-01-18 16:57 UTC (permalink / raw)
To: Rajendra Nayak
Cc: Karthikeyan Ramasubramanian, corbet, andy.gross, david.brown,
robh+dt, mark.rutland, wsa, gregkh, linux-doc, linux-arm-msm,
devicetree, linux-i2c, linux-serial, jslaby, Sagar Dharia,
Girish Mahadevan
In-Reply-To: <29ed5b45-dd70-fb70-3aa4-4e1a0006d57e@codeaurora.org>
On Thu 18 Jan 01:13 PST 2018, Rajendra Nayak wrote:
> []..
>
> >> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> >> new file mode 100644
> >> index 0000000..3f43582
> >> --- /dev/null
> >> +++ b/drivers/soc/qcom/qcom-geni-se.c
> >> @@ -0,0 +1,1016 @@
> >> +/*
> >> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 and
> >> + * only version 2 as published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> + * GNU General Public License for more details.
> >> + *
> >> + */
> >
> > Please use SPDX style license header, i.e. this file should start with:
> >
> > // SPDX-License-Identifier: GPL-2.0
> > /*
> > * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> > */
>
> Looks like Mark Brown commented elsewhere [1] that we should use the C++
> commenting style even for the Linux Foundation copyright?
>
> [1] https://marc.info/?l=linux-clk&m=151497978626135&w=2
>
While I can agree with Mark on the ugliness of the mixed commenting
style, this is the style that I found communicated and is what you find
in other files.
Regards,
Bjorn
^ permalink raw reply
* [PATCH] acpi, spcr: Make SPCR available to x86
From: Prarit Bhargava @ 2018-01-18 15:09 UTC (permalink / raw)
To: linux-kernel
Cc: Prarit Bhargava, Jonathan Corbet, x86, graeme.gregory, linux-pm,
Catalin Marinas, Bhupesh Sharma, linux-doc, Will Deacon,
mark.salter, linux-acpi, Ingo Molnar, Lv Zheng, linux-serial,
H. Peter Anvin, Thomas Gleixner, Timur Tabi, linux-arm-kernel
Note on testing: I've tested this on several ARM64 and x86 boxes (only
earlycon, console=ttyS0,115200, and with both options), verified that
functionality on ARM64 has not changed (ie, CONFIG_ACPI_SPCR_TABLE is
always =y), and verified functionality when !CONFIG_ACPI_SPCR_TABLE on
x86.
P.
----8<----
SPCR is currently only enabled or ARM64 and x86 can use SPCR to setup an
early console.
General fixes include updating Documentation & Kconfig (for x86), updating
comments, and changing parse_spcr() to acpi_parse_spcr(), and
earlycon_init_is_deferred to earlycon_acpi_spcr_enable to be more
descriptive.
On x86, many systems have a valid SPCR table but the table version is not
2 so the table version check must be a warning.
On ARM64 when the kernel parameter earlycon is used both the early console
and console are enabled. On x86, only the earlycon should be enabled by
by default. Modify acpi_parse_spcr() to allow options for initializing
the early console and console separately.
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: linux-acpi@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-pm@vger.kernel.org
Cc: linux-serial@vger.kernel.org
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Lv Zheng <lv.zheng@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: "Rafael J. Wysocki" Mrjw@rjwysocki.net>
Cc: Timur Tabi <timur@codeaurora.org>
Cc: graeme.gregory@linaro.org
Cc: mark.salter@redhat.com
---
Documentation/admin-guide/kernel-parameters.txt | 9 +++++---
arch/arm64/kernel/acpi.c | 4 ++--
arch/x86/kernel/acpi/boot.c | 3 +++
drivers/acpi/Kconfig | 7 +++++-
drivers/acpi/spcr.c | 29 +++++++++++++------------
drivers/tty/serial/earlycon.c | 15 +++++--------
include/linux/acpi.h | 7 ++++--
include/linux/serial_core.h | 4 ++--
8 files changed, 44 insertions(+), 34 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 46b26bfee27b..6f519230ed34 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -915,9 +915,12 @@
earlycon= [KNL] Output early console device and options.
- When used with no options, the early console is
- determined by the stdout-path property in device
- tree's chosen node.
+ [ARM64] The early console is determined by the
+ stdout-path property in device tree's chosen node,
+ or determined by the ACPI SPCR table.
+
+ [X86] When used with no options the early console is
+ determined by the ACPI SPCR table.
cdns,<addr>[,options]
Start an early, polled-mode console on a Cadence
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index b3162715ed78..2aa5609def27 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -230,10 +230,10 @@ void __init acpi_boot_table_init(void)
done:
if (acpi_disabled) {
- if (earlycon_init_is_deferred)
+ if (earlycon_acpi_spcr_enable)
early_init_dt_scan_chosen_stdout();
} else {
- parse_spcr(earlycon_init_is_deferred);
+ acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
if (IS_ENABLED(CONFIG_ACPI_BGRT))
acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
}
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index f4c463df8b08..0bf6a58f7a6d 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -36,6 +36,7 @@
#include <linux/ioport.h>
#include <linux/pci.h>
#include <linux/efi-bgrt.h>
+#include <linux/serial_core.h>
#include <asm/e820/api.h>
#include <asm/irqdomain.h>
@@ -1626,6 +1627,8 @@ int __init acpi_boot_init(void)
if (!acpi_noirq)
x86_init.pci.init = pci_acpi_init;
+ /* Do not enable ACPI SPCR console by default */
+ acpi_parse_spcr(earlycon_acpi_spcr_enable, false);
return 0;
}
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 46505396869e..ddcb5f40e8ee 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -79,7 +79,12 @@ config ACPI_DEBUGGER_USER
endif
config ACPI_SPCR_TABLE
- bool
+ bool "ACPI Serial Port Console Redirection Support"
+ default y if X86
+ help
+ Enable support for Serial Port Console Redirection (SPCR) Table.
+ This table provides information about the configuration of the
+ earlycon console.
config ACPI_LPIT
bool
diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index 324b35bfe781..89e97d21a89c 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -21,7 +21,7 @@
* occasionally getting stuck as 1. To avoid the potential for a hang, check
* TXFE == 0 instead of BUSY == 1. This may not be suitable for all UART
* implementations, so only do so if an affected platform is detected in
- * parse_spcr().
+ * acpi_parse_spcr().
*/
bool qdf2400_e44_present;
EXPORT_SYMBOL(qdf2400_e44_present);
@@ -74,19 +74,21 @@ static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
}
/**
- * parse_spcr() - parse ACPI SPCR table and add preferred console
+ * acpi_parse_spcr() - parse ACPI SPCR table and add preferred console
*
- * @earlycon: set up earlycon for the console specified by the table
+ * @enable_earlycon: set up earlycon for the console specified by the table
+ * @enable_console: setup the console specified by the table.
*
* For the architectures with support for ACPI, CONFIG_ACPI_SPCR_TABLE may be
* defined to parse ACPI SPCR table. As a result of the parsing preferred
- * console is registered and if @earlycon is true, earlycon is set up.
+ * console is registered and if @enable_earlycon is true, earlycon is set up.
+ * If @enable_console is true the system console is also configured.
*
* When CONFIG_ACPI_SPCR_TABLE is defined, this function should be called
* from arch initialization code as soon as the DT/ACPI decision is made.
*
*/
-int __init parse_spcr(bool earlycon)
+int __init acpi_parse_spcr(bool enable_earlycon, bool enable_console)
{
static char opts[64];
struct acpi_table_spcr *table;
@@ -105,11 +107,8 @@ int __init parse_spcr(bool earlycon)
if (ACPI_FAILURE(status))
return -ENOENT;
- if (table->header.revision < 2) {
- err = -ENOENT;
- pr_err("wrong table version\n");
- goto done;
- }
+ if (table->header.revision < 2)
+ pr_info("SPCR table version %d\n", table->header.revision);
if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
switch (ACPI_ACCESS_BIT_WIDTH((
@@ -185,7 +184,7 @@ int __init parse_spcr(bool earlycon)
*/
if (qdf2400_erratum_44_present(&table->header)) {
qdf2400_e44_present = true;
- if (earlycon)
+ if (enable_earlycon)
uart = "qdf2400_e44";
}
@@ -205,11 +204,13 @@ int __init parse_spcr(bool earlycon)
pr_info("console: %s\n", opts);
- if (earlycon)
+ if (enable_earlycon)
setup_earlycon(opts);
- err = add_preferred_console(uart, 0, opts + strlen(uart) + 1);
-
+ if (enable_console)
+ err = add_preferred_console(uart, 0, opts + strlen(uart) + 1);
+ else
+ err = 0;
done:
acpi_put_table((struct acpi_table_header *)table);
return err;
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index 4c8b80f1c688..870e84fb6e39 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -197,25 +197,20 @@ int __init setup_earlycon(char *buf)
}
/*
- * When CONFIG_ACPI_SPCR_TABLE is defined, "earlycon" without parameters in
- * command line does not start DT earlycon immediately, instead it defers
- * starting it until DT/ACPI decision is made. At that time if ACPI is enabled
- * call parse_spcr(), else call early_init_dt_scan_chosen_stdout()
+ * This defers the initialization of the early console until after ACPI has
+ * been initialized.
*/
-bool earlycon_init_is_deferred __initdata;
+bool earlycon_acpi_spcr_enable __initdata;
/* early_param wrapper for setup_earlycon() */
static int __init param_setup_earlycon(char *buf)
{
int err;
- /*
- * Just 'earlycon' is a valid param for devicetree earlycons;
- * don't generate a warning from parse_early_params() in that case
- */
+ /* Just 'earlycon' is a valid param for devicetree and ACPI SPCR. */
if (!buf || !buf[0]) {
if (IS_ENABLED(CONFIG_ACPI_SPCR_TABLE)) {
- earlycon_init_is_deferred = true;
+ earlycon_acpi_spcr_enable = true;
return 0;
} else if (!buf) {
return early_init_dt_scan_chosen_stdout();
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index dc1ebfeeb5ec..9aac8f0ebe23 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1242,9 +1242,12 @@ static inline void acpi_table_upgrade(void) { }
#ifdef CONFIG_ACPI_SPCR_TABLE
extern bool qdf2400_e44_present;
-int parse_spcr(bool earlycon);
+int acpi_parse_spcr(bool enable_earlycon, bool enable_console);
#else
-static inline int parse_spcr(bool earlycon) { return 0; }
+static inline int acpi_parse_spcr(bool enable_earlycon, bool enable_console)
+{
+ return 0;
+}
#endif
#if IS_ENABLED(CONFIG_ACPI_GENERIC_GSI)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 37b044e78333..aefd0e5115da 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -376,10 +376,10 @@ extern int of_setup_earlycon(const struct earlycon_id *match,
const char *options);
#ifdef CONFIG_SERIAL_EARLYCON
-extern bool earlycon_init_is_deferred __initdata;
+extern bool earlycon_acpi_spcr_enable __initdata;
int setup_earlycon(char *buf);
#else
-static const bool earlycon_init_is_deferred;
+static const bool earlycon_acpi_spcr_enable;
static inline int setup_earlycon(char *buf) { return 0; }
#endif
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v6 2/3] clocksource/drivers/atcpit100: VDSO support
From: Arnd Bergmann @ 2018-01-18 11:08 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <de648d211061a16c85ae0321625c51343ce5e5ce.1515766198.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:57 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Rick Chen <rickchen36@gmail.com>
>
> VDSO needs real-time cycle count to ensure the time accuracy.
> Unlike others, nds32 architecture does not define clock source,
> hence VDSO needs atcpit100 offering real-time cycle count
> to derive the correct time.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Rick Chen <rickchen36@gmail.com>
> Signed-off-by: Greentime Hu <green.hu@gmail.com>
I'm a bit puzzled by this patch, can you explain how the vdso actually
manages to access the clock hardware? It looks like you make the
physical address and the register offset available to user space, but
how does it end up accessing it?
Arnd
^ permalink raw reply
* Re: [PATCH v6 3/3] dt-bindings: timer: Add andestech atcpit100 timer binding doc
From: Arnd Bergmann @ 2018-01-18 11:04 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <baa3a7a9392a2a04e791f3e29cb7cd5707fed672.1515766198.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:57 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Rick Chen <rickchen36-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Add a document to describe Andestech atcpit100 timer and
> binding information.
>
> Signed-off-by: Rick Chen <rickchen36-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 33/36] dt-bindings: nds32 SoC Bindings
From: Arnd Bergmann @ 2018-01-18 11:03 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <b497b9429d9bc5208334b4c1abbaeb47d3eedf95.1515766253.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> This patch adds nds32 SoC(AE3XX and AG101P) binding documents.
>
> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 36/36] net: faraday add nds32 support.
From: Arnd Bergmann @ 2018-01-18 11:02 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <74b1aceeb4a8718657cb60e717b7108783f3241f.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch is used to support nds32 architecture to use these faraday
> mac IP.
>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 31/36] dt-bindings: nds32 CPU Bindings
From: Arnd Bergmann @ 2018-01-18 11:02 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <e9577ec0f6b6ca17077aa2f576c8bef191c3d7c2.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds nds32 CPU binding documents.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Rick Chen <rick@andestech.com>
> Signed-off-by: Zong Li <zong@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> Documentation/devicetree/bindings/nds32/cpus.txt | 37 ++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/nds32/cpus.txt
>
> diff --git a/Documentation/devicetree/bindings/nds32/cpus.txt b/Documentation/devicetree/bindings/nds32/cpus.txt
> new file mode 100644
> index 0000000..9a52937
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nds32/cpus.txt
> @@ -0,0 +1,37 @@
> +* Andestech Processor Binding
> +
> +This binding specifies what properties must be available in the device tree
> +representation of a Andestech Processor Core, which is the root node in the
> +tree.
> +
> +Required properties:
> +
> + - compatible:
> + Usage: required
> + Value type: <string>
> + Definition: should be one of:
> + "andestech,n13"
> + "andestech,n15"
> + "andestech,d15"
> + "andestech,n10"
> + "andestech,d10"
> + "andestech,nds32v3"
Based on https://lkml.org/lkml/2017/11/27/1290, this should say that
the device tree should always list 'andestech,nds32v3' as the most
generic 'compatible' value and list exactly one of the others in
addition.
Arnd
^ permalink raw reply
* Re: [PATCH v6 29/36] nds32: Build infrastructure
From: Arnd Bergmann @ 2018-01-18 11:00 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <e4b231f440511aa0a539ba45f316a096741f530e.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds Makefile, Kconfig and vmlinux.lds.S files required for building
> an nds32 kernel.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
I find some new details every time I look here ;-)
> @@ -0,0 +1,107 @@
> +#
> +# For a description of the syntax of this configuration file,
> +# see Documentation/kbuild/kconfig-language.txt.
> +#
> +
> +config NDS32
> + def_bool y
> + select ARCH_HAS_RAW_COPY_USER
I don't think this symbol was ever merged. Do you remember why you added it?
> + select ARCH_WANT_FRAME_POINTERS if FTRACE
> + select ARCH_WANT_IPC_PARSE_VERSION
You most certainly don't want IPC_PARSE_VERSION, please drop this
and adapt your glibc.
> + select CLKSRC_MMIO
> + select CLONE_BACKWARDS
> + select COMMON_CLK
> + select FRAME_POINTER
Do you need both ARCH_WANT_FRAME_POINTERS and FRAME_POINTER here?
> + select GENERIC_ATOMIC64
> + select GENERIC_CPU_DEVICES
> + select GENERIC_CLOCKEVENTS
> + select GENERIC_IRQ_CHIP
> + select GENERIC_IRQ_PROBE
I think it's better to drop GENERIC_IRQ_PROBE here, no modern driver
should rely on that.
> +choice
> + prompt "CPU type"
> + default CPU_V3
> +config CPU_N15
> + bool "AndesCore N15"
> +config CPU_N13
> + bool "AndesCore N13"
> + select CPU_CACHE_ALIASING if ANDES_PAGE_SIZE_4KB
> +config CPU_N10
> + bool "AndesCore N10"
> + select CPU_CACHE_ALIASING
> +config CPU_D15
> + bool "AndesCore D15"
> +config CPU_D10
> + bool "AndesCore D10"
> + select CPU_CACHE_ALIASING
> +config CPU_V3
> + bool "AndesCore v3 compatible"
> + select ANDES_PAGE_SIZE_8KB
> +endchoice
I forget what we discussed here earlier, but at the very least, there should be
some help text here to explain what the implications are. I assume that you
generally want to be able to build one kernel to run on all of the above, right?
Will selecting 'CPU_V3' result in a kernel binary that can run on all of them?
If so, please explain it here as that is not obvious.
For the other CPU types, can you list the what backwards-compatiblity
you get? E.g. will a kernel built for N13 run on any of N15, D15 or N10?
I think the 'select ANDES_PAGE_SIZE_8KB' cannot work as expected,
since ANDES_PAGE_SIZE_8KB is inside of a 'choice' statement. Since
there are only two options (4K and 8K), you can address that by making
it a simple bool option and fall back to 4K when ANDES_PAGE_SIZE_8KB
is disabled.
> +config CACHE_L2
> + bool "Support L2 cache"
> + default y
> + help
> + Say Y here to enable L2 cache if your SoC are integrated with L2CC.
> + If unsure, say N.
> +
> +menu "Memory configuration"
> +
> +choice
> + prompt "Memory split"
> + depends on MMU
> + default VMSPLIT_3G
Why not default to VMSPLIT_3G_OPT?
Arnd
^ permalink raw reply
* Re: [PATCH v6 27/36] nds32: Miscellaneous header files
From: Arnd Bergmann @ 2018-01-18 10:46 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <38d9dce03a1421378a94d3d528bb0c4741e2bf7b.1515766253.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> This patch introduces some miscellaneous header files.
>
> Signed-off-by: Vincent Chen <vincentc-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 34/36] dt-bindings: interrupt-controller: Andestech Internal Vector Interrupt Controller
From: Arnd Bergmann @ 2018-01-18 10:46 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <2cd18fba6e2ab78fc0eb50dd8d1216012f90c6d8.1515766253.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> This patch adds an irqchip driver document for the Andestech Internal Vector
> Interrupt Controller.
>
> Signed-off-by: Rick Chen <rick-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 32/36] dt-bindings: nds32 L2 cache controller Bindings
From: Arnd Bergmann @ 2018-01-18 10:45 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <11bba5e5107a0b434f0ae19775fef8d36ccd7246.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds nds32 L2 cache controller binding documents.
>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 30/36] MAINTAINERS: Add nds32
From: Arnd Bergmann @ 2018-01-18 10:45 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <3582dac9184363138e635c44f17d95f84a38f49d.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Please add a changelog text to every single commit, otherwise:
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 28/36] nds32: defconfig
From: Arnd Bergmann @ 2018-01-18 10:44 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <af61607634fa92ddd40b22b5ba215d35b40a19a7.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds nds32 defconfig.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 26/36] nds32: Device tree support
From: Arnd Bergmann @ 2018-01-18 10:43 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <623edb96d0a59433c25d71e5f6bb24a17804e960.1515766253.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> This patch adds support for device tree.
>
> Signed-off-by: Vincent Chen <vincentc-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 25/36] nds32: Generic timers support
From: Arnd Bergmann @ 2018-01-18 10:41 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <ff6346e073ae38aa96a031420f57aad3d1057ddf.1515766253.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> This patch adds support for timer.
>
> Signed-off-by: Vincent Chen <vincentc-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Reviewed-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 24/36] nds32: Loadable modules
From: Arnd Bergmann @ 2018-01-18 10:41 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <0a0789a3f776a5248ebc01706d502a3a9823cfac.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds support for loadable modules.
One detail:
You still seem to have both the ELF_REL and ELF_RELA based functions
implemented here, you should drop the unused ELF_REL version:
> diff --git a/arch/nds32/kernel/module.c b/arch/nds32/kernel/module.c
> new file mode 100644
> index 0000000..714a6d6
> --- /dev/null
> +++ b/arch/nds32/kernel/module.c
> @@ -0,0 +1,286 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2005-2017 Andes Technology Corporation
> +
> +#include <linux/module.h>
> +#include <linux/elf.h>
> +#include <linux/vmalloc.h>
> +
> +#include <asm/pgtable.h>
include <linux/moduleloader.h> to catch this.
> +int
> +apply_relocate(Elf32_Shdr * sechdrs, const char *strtab,
> + unsigned int symindex, unsigned int relsec,
> + struct module *module)
> +{
> + return 0;
> +}
and drop this.
With that change,
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox