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

[Tanmay Inamdar]

On Wed, May 9, 2012 at 10:57 AM, Tanmay Inamdar <tinamdar@xxxxxxx> wrote:
> On Wed, May 2, 2012 at 7:08 PM, Josh Boyer <jwboyer@xxxxxxxxx> wrote:
>> On Mon, Apr 9, 2012 at 3:20 AM, Tanmay Inamdar <tinamdar@xxxxxxx> 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@xxxxxxx>
>>> ---
>>> :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.
>>
>
> You are right. There is no need to use legacy serial driver. However I
> realized that when 40x is selected, 'PPC_UDBG_16550' is by default
> selected. This enables legacy serial driver.
>
> Is it now required to enable 'PPC_UDBG_16550' by default for every SOC
> that uses 40x processor?
>
Josh, Ben,

Please let me know if you have any comments regarding above question.

>>> +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
>
> Thanks,
> Tanmay
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, 
is for the sole use of the intended recipient(s) and contains information 
that is confidential and proprietary to AppliedMicro Corporation or its subsidiaries. 
It is to be used solely for the purpose of furthering the parties' business relationship. 
All unauthorized review, use, disclosure or distribution is prohibited. 
If you are not the intended recipient, please contact the sender by reply e-mail 
and destroy all copies of the original message.

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