Re: [PATCH 1/3] qe-uart: modify qe-uart to adapt both powerpc and arm
On Mon, 2014-10-13 at 11:30 -0500, Timur Tabi wrote:
> On Fri, Oct 10, 2014 at 1:05 PM, Scott Wood wrote:
> > There are many changes in here that ought to be separate patches with
> > separate justification.
> >
> > Also, some of the QE changes seem to be reasonable cleanup, but not
> > related to making the code work on ARM.
>
> I agree with Scott. This patch already makes significant code
> changes, so you should have one patch that just makes the
> out_be32->iowrite32be changes. Changes to the QE library should NOT
> be in the same patch as changes to ucc_uart.c.
>
> In addition, changes like this:
>
> - iprop = of_get_property(np, "port-number", NULL);
> - if (!iprop) {
> + ret = of_property_read_u32_index(np, "port-number", 0, &val);
> + if (ret) {
>
> should be changed to remove the OF dependency. If you're going to
> replace of_get_property, replace it with device_property_read_u32(),
> to remove the OF dependency.
>
> >> diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
> >> index dff714d..a932f99 100644
> >> --- a/arch/arm/include/asm/delay.h
> >> +++ b/arch/arm/include/asm/delay.h
> >> @@ -57,6 +57,22 @@ extern void __bad_udelay(void);
> >> __const_udelay((n) * UDELAY_MULT)) :\
> >> __udelay(n))
> >>
> >> +#define spin_event_timeout(condition, timeout, delay)
> >> \
> >> +({
> >> \
> >> + typeof(condition) __ret;
> >> \
> >> + int i = 0;
> >> \
> >> + while (!(__ret = (condition)) && (i++ < timeout)) {
> >> \
> >> + if (delay)
> >> \
> >> + udelay(delay);
> >> \
> >> + else
> >> \
> >> + cpu_relax();
> >> \
> >> + udelay(1);
> >> \
> >> + }
> >> \
> >
> > This will delay too long if "delay" is used.
>
> Shouldn't ARM have a version of tb_ticks_since() by now?
There's get_cycles(), but it's not clear to me whether loops_per_jiffy
is OK to use with get_cycles() on 32-bit ARM. Is avoiding the udelay
worth making this non-generic?
>
> >> + if (!__ret)
> >> \
> >> + __ret = (condition);
> >> \
> >> + __ret;
> >> \
> >
> > Timur, do you remember why that final "if (!__ret) __ret = (condition);"
> > is needed?
>
> powerpc: Fix spin_event_timeout() to be robust over context switches
>
> Current implementation of spin_event_timeout can be interrupted by an
> IRQ or context switch after testing the condition, but before checking
> the timeout. This can cause the loop to report a timeout when the
> condition actually became true in the middle.
>
> This patch adds one final check of the condition upon exit of the loop
> if the last test of the condition was still false.
OK, so this shouldn't be needed in the udelay version, since an
interrupt shouldn't cause a significant difference in the timeout count.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3] qe-uart: modify qe-uart to adapt both powerpc and arm
On Fri, Oct 10, 2014 at 1:05 PM, Scott Wood wrote:
> There are many changes in here that ought to be separate patches with
> separate justification.
>
> Also, some of the QE changes seem to be reasonable cleanup, but not
> related to making the code work on ARM.
I agree with Scott. This patch already makes significant code
changes, so you should have one patch that just makes the
out_be32->iowrite32be changes. Changes to the QE library should NOT
be in the same patch as changes to ucc_uart.c.
In addition, changes like this:
- iprop = of_get_property(np, "port-number", NULL);
- if (!iprop) {
+ ret = of_property_read_u32_index(np, "port-number", 0, &val);
+ if (ret) {
should be changed to remove the OF dependency. If you're going to
replace of_get_property, replace it with device_property_read_u32(),
to remove the OF dependency.
>> diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
>> index dff714d..a932f99 100644
>> --- a/arch/arm/include/asm/delay.h
>> +++ b/arch/arm/include/asm/delay.h
>> @@ -57,6 +57,22 @@ extern void __bad_udelay(void);
>> __const_udelay((n) * UDELAY_MULT)) :\
>> __udelay(n))
>>
>> +#define spin_event_timeout(condition, timeout, delay)
>>\
>> +({
>>\
>> + typeof(condition) __ret;
>> \
>> + int i = 0;
>> \
>> + while (!(__ret = (condition)) && (i++ < timeout)) {
>> \
>> + if (delay)
>> \
>> + udelay(delay);
>> \
>> + else
>> \
>> + cpu_relax();
>> \
>> + udelay(1);
>> \
>> + }
>> \
>
> This will delay too long if "delay" is used.
Shouldn't ARM have a version of tb_ticks_since() by now?
>> + if (!__ret)
>> \
>> + __ret = (condition);
>> \
>> + __ret;
>> \
>
> Timur, do you remember why that final "if (!__ret) __ret = (condition);"
> is needed?
powerpc: Fix spin_event_timeout() to be robust over context switches
Current implementation of spin_event_timeout can be interrupted by an
IRQ or context switch after testing the condition, but before checking
the timeout. This can cause the loop to report a timeout when the
condition actually became true in the middle.
This patch adds one final check of the condition upon exit of the loop
if the last test of the condition was still false.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3] qe-uart: modify qe-uart to adapt both powerpc and arm
On Fri, 2014-10-10 at 14:47 +0800, Zhao Qiang wrote:
> qe has been supported by arm board ls1021, qe-uart need
> to be supported by ls1021.
> modify the code to make qe-uart can work on both powerpc
> and ls1021.
>
> Signed-off-by: Zhao Qiang
> ---
> arch/arm/include/asm/delay.h | 16
> arch/arm/include/asm/io.h | 28 +++
> arch/arm/include/asm/irq.h| 2 +
> arch/arm/kernel/irq.c | 7 ++
> drivers/soc/qe/Kconfig| 1 -
> drivers/soc/qe/qe.c | 63 ---
> drivers/soc/qe/qe_common.c| 2 +-
> drivers/soc/qe/qe_ic.c| 7 +-
> drivers/soc/qe/qe_io.c| 53 ++---
> drivers/soc/qe/ucc_slow.c | 40 +-
> drivers/tty/serial/ucc_uart.c | 176
> +-
> include/linux/fsl/qe.h| 21 +
> 12 files changed, 245 insertions(+), 171 deletions(-)
This patch obviously depends on the patches to relocate QE support, but
there's no mention of that dependency above.
There are many changes in here that ought to be separate patches with
separate justification.
Also, some of the QE changes seem to be reasonable cleanup, but not
related to making the code work on ARM.
> diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
> index dff714d..a932f99 100644
> --- a/arch/arm/include/asm/delay.h
> +++ b/arch/arm/include/asm/delay.h
> @@ -57,6 +57,22 @@ extern void __bad_udelay(void);
> __const_udelay((n) * UDELAY_MULT)) :\
> __udelay(n))
>
> +#define spin_event_timeout(condition, timeout, delay)
> \
> +({
> \
> + typeof(condition) __ret; \
> + int i = 0; \
> + while (!(__ret = (condition)) && (i++ < timeout)) {\
> + if (delay) \
> + udelay(delay); \
> + else \
> + cpu_relax(); \
> + udelay(1); \
> + } \
This will delay too long if "delay" is used.
How about:
delay = delay ? delay : 1; \
while (!(__ret = (condition))) {\
if (i < timeout)\
break; \
i += delay; \
udelay(delay); \
} \
Also, once the dependency on timebase is removed, how about making this
generic rather than just PPC+ARM?
> + if (!__ret)\
> + __ret = (condition); \
> + __ret; \
Timur, do you remember why that final "if (!__ret) __ret = (condition);"
is needed?
> +})
> +
> /* Loop-based definitions for assembly code. */
> extern void __loop_delay(unsigned long loops);
> extern void __loop_udelay(unsigned long usecs);
> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
> index d070741..4bec694 100644
> --- a/arch/arm/include/asm/io.h
> +++ b/arch/arm/include/asm/io.h
> @@ -206,6 +206,34 @@ extern int pci_ioremap_io(unsigned int offset,
> phys_addr_t phys_addr);
> #endif
> #endif
>
> +/* access ports */
> +#define setbits32(_addr, _v) iowrite32be(ioread32be(_addr) | (_v), (_addr))
> +#define clrbits32(_addr, _v) iowrite32be(ioread32be(_addr) & ~(_v), (_addr))
> +
> +#define setbits16(_addr, _v) iowrite16be(ioread16be(_addr) | (_v), (_addr))
> +#define clrbits16(_addr, _v) iowrite16be(ioread16be(_addr) & ~(_v), (_addr))
> +
> +#define setbits8(_addr, _v) iowrite8(ioread8(_addr) | (_v), (_addr))
> +#define clrbits8(_addr, _v) iowrite8(ioread8(_addr) & ~(_v), (_addr))
This should also be a separate patch, though I don't think implicit big
endian is going to fly in arch/arm (it was a mistake in arch/powerpc).
Rename to setbits_be32 etc as is used in U-Boot.
> +/* Clear and set bits in one shot. These macros can be used to clear and
> + * set multiple bits in a register using a single read-modify-write. These
> + * macros can also be used to set a multiple-bit bit pattern using a mask,
> + * by specifying the mask in the 'clear' parameter and the new bit pattern
> + * in the 'set' parameter.
> + */
> +
> +#define clrsetbits_be32(addr, clear, set) \
> + iowrite32be((ioread32be(addr) & ~(clear)) | (set), (addr))
> +#define clrsetbits_le32(addr,
