linux-kernel - Re: [V2 5/5] powerpc: kernel: 16650 UART reg-shift support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+5PVA4sjX_Dk6ecySG4ZZoM=xhaWu2jGrQ=LtFgxGL88G=-Nw@mail.gmail.com>
Date:	Wed, 2 May 2012 09:38:14 -0400
From:	Josh Boyer <jwboyer@...il.com>
To:	Tanmay Inamdar <tinamdar@....com>
Cc:	benh@...nel.crashing.org, grant.likely@...retlab.ca,
	linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
	devicetree-discuss@...ts.ozlabs.org
Subject: Re: [V2 5/5] powerpc: kernel: 16650 UART reg-shift support

On Mon, Apr 9, 2012 at 3:20 AM, Tanmay Inamdar <tinamdar@....com> wrote:
> In APM8018X SOC, UART register address space has been relocated to 32-bit
> data boundaries for APB bus implementation.
> Current legacy_serial driver ignores the reg-shift property. This patch
> modifies legacy_serial.c and udbg_16550.c to work with above mentioned UARTs.
>
> Signed-off-by: Tanmay Inamdar <tinamdar@....com>
> ---
> :100644 100644 8338aef... f5fc106... M  arch/powerpc/include/asm/udbg.h
> :100644 100644 bedd12e... d523b7d... M  arch/powerpc/kernel/legacy_serial.c
> :100644 100644 6837f83... e0cb7dc... M  arch/powerpc/kernel/udbg_16550.c
>  arch/powerpc/include/asm/udbg.h     |    2 +-
>  arch/powerpc/kernel/legacy_serial.c |   16 +++++---
>  arch/powerpc/kernel/udbg_16550.c    |   64 ++++++++++++++++++++++------------
>  3 files changed, 52 insertions(+), 30 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/udbg.h b/arch/powerpc/include/asm/udbg.h
> index 8338aef..f5fc106 100644
> --- a/arch/powerpc/include/asm/udbg.h
> +++ b/arch/powerpc/include/asm/udbg.h
> @@ -29,7 +29,7 @@ extern void udbg_printf(const char *fmt, ...)
>  extern void udbg_progress(char *s, unsigned short hex);
>
>  extern void udbg_init_uart(void __iomem *comport, unsigned int speed,
> -                          unsigned int clock);
> +                          unsigned int clock,  unsigned int regshift);
>  extern unsigned int udbg_probe_uart_speed(void __iomem *comport,
>                                          unsigned int clock);
>
> diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
> index bedd12e..d523b7d 100644
> --- a/arch/powerpc/kernel/legacy_serial.c
> +++ b/arch/powerpc/kernel/legacy_serial.c
> @@ -33,6 +33,7 @@ static struct legacy_serial_info {
>        unsigned int                    clock;
>        int                             irq_check_parent;
>        phys_addr_t                     taddr;
> +       unsigned int                    regshift;
>  } legacy_serial_infos[MAX_LEGACY_SERIAL_PORTS];
>
>  static struct __initdata of_device_id legacy_serial_parents[] = {
> @@ -42,6 +43,7 @@ static struct __initdata of_device_id legacy_serial_parents[] = {
>        {.compatible = "ibm,opb",},
>        {.compatible = "simple-bus",},
>        {.compatible = "wrs,epld-localbus",},
> +       {.compatible = "apm,apb",},
>        {},
>  };
>
> @@ -163,11 +165,6 @@ static int __init add_legacy_soc_port(struct device_node *np,
>        if (of_get_property(np, "clock-frequency", NULL) == NULL)
>                return -1;
>
> -       /* if reg-shift or offset, don't try to use it */
> -       if ((of_get_property(np, "reg-shift", NULL) != NULL) ||
> -               (of_get_property(np, "reg-offset", NULL) != NULL))
> -               return -1;
> -

So we explicitly didn't support reg-shift before.  I'm guessing there is
a reason for that, but I don't recall what.  Ben?

Also, why do you need to use the legacy serial driver at all for this
SOC?  As far as I remember, the OF serial driver should be sufficient.

> +static unsigned int reg_shift;
> +#define ns16550_offset(addr) (addr - (unsigned char *)udbg_comport)
> +
>  static struct NS16550 __iomem *udbg_comport;
>
> +static inline u8 serial_read(unsigned char *addr)
> +{
> +       u32 offset = ns16550_offset(addr) << reg_shift;
> +       return readb(udbg_comport + offset);
> +}
> +
> +static inline void serial_write(unsigned char *addr, char val)
> +{
> +       u32 offset = ns16550_offset(addr) << reg_shift;
> +       writeb(val, udbg_comport + offset);
> +}
> +

I don't think readb/writeb are correct here.  Why did you switch to
using those instead of sticking with in_8/out_8?

josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/