Re: [PATCH -mm 1/4] Blackfin: architecture update patch

Re: [PATCH -mm 1/4] Blackfin: architecture update patch

[Andrew Morton]

On Wed, 21 Mar 2007 18:19:23 +0800
"Wu, Bryan" <bryan.wu@analog.com> wrote:

> 1) Some issues are fixed according to LKML patch review.
> 2) Remove not supported BF535 code
> 3) Fixed some bugs from blackfin.uclinux.org SVN update
> Here is the updated patch for 2.6.21-rc4-mm1

1 out of 1 hunk FAILED -- saving rejects to file include/asm-blackfin/cplbinit.h.rej
1 out of 1 hunk FAILED -- saving rejects to file include/asm-blackfin/mach-bf535/bf535.h.rej
1 out of 1 hunk FAILED -- saving rejects to file include/asm-blackfin/scatterlist.h.rej

This seems to be against a kernel which did not include
blackfin-arch-balance-parenthesis-in-macros.patch.  But 2.6.21-rc4-mm1 did
include blackfin-arch-balance-parenthesis-in-macros.patch, so I'm not sure
what is going on here.

Re: [PATCH -mm 1/4] Blackfin: architecture update patch

[Wu, Bryan]

On Wed, 2007-03-21 at 15:53 -0700, Andrew Morton wrote:
> On Wed, 21 Mar 2007 18:19:23 +0800
> "Wu, Bryan" <bryan.wu@analog.com> wrote:
> 
> > 1) Some issues are fixed according to LKML patch review.
> > 2) Remove not supported BF535 code
> > 3) Fixed some bugs from blackfin.uclinux.org SVN update
> > Here is the updated patch for 2.6.21-rc4-mm1
> 
> 1 out of 1 hunk FAILED -- saving rejects to file include/asm-blackfin/cplbinit.h.rej
> 1 out of 1 hunk FAILED -- saving rejects to file include/asm-blackfin/mach-bf535/bf535.h.rej
> 1 out of 1 hunk FAILED -- saving rejects to file include/asm-blackfin/scatterlist.h.rej
> 
> This seems to be against a kernel which did not include
> blackfin-arch-balance-parenthesis-in-macros.patch.  But 2.6.21-rc4-mm1 did
> include blackfin-arch-balance-parenthesis-in-macros.patch, so I'm not sure
> what is going on here.
> 

Sorry for this mess up. you know, as
blackfin-arch-balance-parenthesis-in-macros.patch was posted, it was
applied to our SVN tree. And I just merged these weeks SVN change to
this update patch. So this patch includes
blackfin-arch-balance-parenthesis-in-macros.patch. 

Please hold on, after I fixed some issues according to Arnd and Paul's
latest review, a new update patch will be sent.

Sorry for send a all-in-one update patch. Arnd and Paul gave lots' of
good ideas about our last blackfin-arch.patch last time, and most of
their request are applied in this days. So if I split the all-in-one
update patch to small fix, there will be maybe 20 or 30 patches.

After this blackfin-arch-update.patch was merged to -mm, I will post
small fixed patches as Arnd's advise.

Thanks
-Bryan Wu

Re: [PATCH -mm 1/4] Blackfin: architecture update patch

[Andrew Morton]

On Thu, 22 Mar 2007 10:24:51 +0800 "Wu, Bryan" <bryan.wu@analog.com> wrote:

> > 1 out of 1 hunk FAILED -- saving rejects to file include/asm-blackfin/cplbinit.h.rej
> > 1 out of 1 hunk FAILED -- saving rejects to file include/asm-blackfin/mach-bf535/bf535.h.rej
> > 1 out of 1 hunk FAILED -- saving rejects to file include/asm-blackfin/scatterlist.h.rej
> > 
> > This seems to be against a kernel which did not include
> > blackfin-arch-balance-parenthesis-in-macros.patch.  But 2.6.21-rc4-mm1 did
> > include blackfin-arch-balance-parenthesis-in-macros.patch, so I'm not sure
> > what is going on here.
> > 
> 
> Sorry for this mess up. you know, as
> blackfin-arch-balance-parenthesis-in-macros.patch was posted, it was
> applied to our SVN tree. And I just merged these weeks SVN change to
> this update patch. So this patch includes
> blackfin-arch-balance-parenthesis-in-macros.patch. 

Well, you'd better get used to it.  As I told you a few weeks ago, once this
code is merged into mainline you no longer own the master copy of the blackfin
tree - Linus does.

If you're not careful, you'll end up submitting patches which revert other
people's fixes.   And there's an excellent chance that you will submit patches
which have dependencies on other stuff in your private tree and which are
insufficiently-tested-against, and possibly incorrect-against Linus's tree.

Re: [PATCH -mm 1/4] Blackfin: architecture update patch

[Wu, Bryan]

On Wed, 2007-03-21 at 20:08 -0800, Andrew Morton wrote:
> On Thu, 22 Mar 2007 10:24:51 +0800 "Wu, Bryan" <bryan.wu@analog.com> wrote:
> 
> > > 1 out of 1 hunk FAILED -- saving rejects to file include/asm-blackfin/cplbinit.h.rej
> > > 1 out of 1 hunk FAILED -- saving rejects to file include/asm-blackfin/mach-bf535/bf535.h.rej
> > > 1 out of 1 hunk FAILED -- saving rejects to file include/asm-blackfin/scatterlist.h.rej
> > > 
> > > This seems to be against a kernel which did not include
> > > blackfin-arch-balance-parenthesis-in-macros.patch.  But 2.6.21-rc4-mm1 did
> > > include blackfin-arch-balance-parenthesis-in-macros.patch, so I'm not sure
> > > what is going on here.
> > > 
> > 
> > Sorry for this mess up. you know, as
> > blackfin-arch-balance-parenthesis-in-macros.patch was posted, it was
> > applied to our SVN tree. And I just merged these weeks SVN change to
> > this update patch. So this patch includes
> > blackfin-arch-balance-parenthesis-in-macros.patch. 
> 
> Well, you'd better get used to it.  As I told you a few weeks ago, once this
> code is merged into mainline you no longer own the master copy of the blackfin
> tree - Linus does.
> 

We agree with this and please give us sometime to try to push our
patches to the mainline tree. We still have tons of code should be
submitted to mainline, and before the submission, the code should be
reviewed and fixed. This is my main task and I think this internal
review can reduce you and other kernel maintainer's effort.

But our team still have release pressure from customers. The whole
distribution including bootloader/toolchain/kernel/uClinux-dist should
be delivered to customers on time. If our kernel blackfin-arch are
merged eventually, we definitely will move to linus tree and ask our
customer to use linus mainline tree.

As you know, our latest U-Boot blackfin code is accepted by U-Boot
upstream mainline recently. Then the internal U-Boot SVN tree will be
replaced by upstream git-blackfin tree in U-Boot, our development will
move to that mainline tree.

At this moment, I will try my best to speed up the blackfin-arch and
drivers merge progress. 

> If you're not careful, you'll end up submitting patches which revert other
> people's fixes.   And there's an excellent chance that you will submit patches
> which have dependencies on other stuff in your private tree and which are
> insufficiently-tested-against, and possibly incorrect-against Linus's tree.

Apologise. I will resend the blackfin update patch based on other
patches.

Thanks Andrew
-Bryan Wu

Re: [PATCH -mm 1/4] Blackfin: architecture update patch

[Arnd Bergmann]

On Wednesday 21 March 2007, Wu, Bryan wrote:

> @@ -97,6 +97,11 @@ static inline void leds_switch(int flag)
>  /*
>   * The idle loop on BFIN
>   */
> +#ifdef CONFIG_IDLE_L1
> +static inline void default_idle(void)__attribute__((l1_text));
> +void cpu_idle(void)__attribute__((l1_text));
> +#endif
> +

A forward declaration for an inline function seems rather pointless.
Moreover, marking default_idle both l1_text and inline seems
contradicting, right?

> diff -purN linux-2.6-orig/include/asm-blackfin/asm-offsets.h linux-2.6/include/asm-blackfin/asm-offsets.h
> --- linux-2.6-orig/include/asm-blackfin/asm-offsets.h	1970-01-01 08:00:00.000000000 +0800
> +++ linux-2.6/include/asm-blackfin/asm-offsets.h	2007-03-21 15:21:10.000000000 +0800
> @@ -0,0 +1,89 @@
> +#ifndef __ASM_OFFSETS_H__
> +#define __ASM_OFFSETS_H__
> +/*
> + * DO NOT MODIFY.
> + *
> + * This file was generated by Kbuild

This file should be in the exclude list for your diff, it is generally not
shipped with the kernel sources.

> +#ifndef __ASSEMBLY__
> +
> +static inline unsigned char readb(volatile unsigned char *addr)
> +{

The prototype for this should normally contain an __iomem.
This kind of error is normally caught by running 'make C=1'
to use the 'sparse' tool. If you have not run that yet,
you should start to, as it finds a number of common bugs.

> +/*
> + * Map some physical address range into the kernel address space.
> + */
> +static inline void *__ioremap(unsigned long physaddr, unsigned long size,
> +				int cacheflag)
> +{
> +	return (void *)physaddr;
> +}

Likewise, this should return an __iomem pointer.

The rest of the patch looks good to me.

	Arnd <><

Re: [PATCH -mm 1/4] Blackfin: architecture update patch

[Wu, Bryan]

On Wed, 2007-03-21 at 11:25 +0100, Arnd Bergmann wrote:
> On Wednesday 21 March 2007, Wu, Bryan wrote:
> 
> > @@ -97,6 +97,11 @@ static inline void leds_switch(int flag)
> >  /*
> >   * The idle loop on BFIN
> >   */
> > +#ifdef CONFIG_IDLE_L1
> > +static inline void default_idle(void)__attribute__((l1_text));
> > +void cpu_idle(void)__attribute__((l1_text));
> > +#endif
> > +
> 
> A forward declaration for an inline function seems rather pointless.
> Moreover, marking default_idle both l1_text and inline seems
> contradicting, right?
> 
> > diff -purN linux-2.6-orig/include/asm-blackfin/asm-offsets.h linux-2.6/include/asm-blackfin/asm-offsets.h
> > --- linux-2.6-orig/include/asm-blackfin/asm-offsets.h	1970-01-01 08:00:00.000000000 +0800
> > +++ linux-2.6/include/asm-blackfin/asm-offsets.h	2007-03-21 15:21:10.000000000 +0800
> > @@ -0,0 +1,89 @@
> > +#ifndef __ASM_OFFSETS_H__
> > +#define __ASM_OFFSETS_H__
> > +/*
> > + * DO NOT MODIFY.
> > + *
> > + * This file was generated by Kbuild
> 
> This file should be in the exclude list for your diff, it is generally not
> shipped with the kernel sources.

Got, I will fix it.

> 
> > +#ifndef __ASSEMBLY__
> > +
> > +static inline unsigned char readb(volatile unsigned char *addr)
> > +{
> 
> The prototype for this should normally contain an __iomem.
> This kind of error is normally caught by running 'make C=1'
> to use the 'sparse' tool. If you have not run that yet,
> you should start to, as it finds a number of common bugs.
> 
> > +/*
> > + * Map some physical address range into the kernel address space.
> > + */
> > +static inline void *__ioremap(unsigned long physaddr, unsigned long size,
> > +				int cacheflag)
> > +{
> > +	return (void *)physaddr;
> > +}
> 
> Likewise, this should return an __iomem pointer.

It will be fixed as soon as possible.
> 
> The rest of the patch looks good to me.
> 
> 	Arnd <><

I sent 4 mail to LKML, but this one lost. Arnd, can you receive this
email from LKML. 

Thanks a lot
-Bryan Wu

Re: [PATCH -mm 1/4] Blackfin: architecture update patch

[Arnd Bergmann]

On Wednesday 21 March 2007, Wu, Bryan wrote:
> I sent 4 mail to LKML, but this one lost. Arnd, can you receive this
> email from LKML.

The mail was around 400kb, while the limit for lkml is 100kb.

	Arnd <><

Re: [PATCH -mm 1/4] Blackfin: architecture update patch

[Paul Mundt]

On Wed, Mar 21, 2007 at 06:06:58PM +0800, Wu, Bryan wrote:
> +++ linux-2.6/arch/blackfin/Kconfig	2007-03-21 14:38:42.000000000 +0800
> @@ -21,6 +21,10 @@ config RWSEM_XCHGADD_ALGORITHM
>  	bool
>  	default n
>  
> +config BLACKFIN
> +	bool
> +	default y
> +
>  config BFIN
>  	bool
>  	default y

Again, there's no reason to have both of these. Pick one and stick with
it.

> diff -purN linux-2.6-orig/arch/blackfin/kernel/flat.c linux-2.6/arch/blackfin/kernel/flat.c
> --- linux-2.6-orig/arch/blackfin/kernel/flat.c	1970-01-01 08:00:00.000000000 +0800
> +++ linux-2.6/arch/blackfin/kernel/flat.c	2007-03-21 14:43:37.000000000 +0800
[snip]
> +	switch (type) {
> +		case FLAT_BFIN_RELOC_TYPE_16_BIT:
> +		case FLAT_BFIN_RELOC_TYPE_16H_BIT:
> +			usptr = (unsigned short *)ptr;
> +#ifdef DEBUG_BFIN_RELOC
> +			pr_info(" *usptr = %x", get_unaligned(usptr));
> +#endif
> +			val = get_unaligned(usptr);

The debugging here looks broken, just use pr_debug() and DEBUG as normal,
then you can kill off this DEBUG_BFIN_RELOC thing.

> @@ -346,3 +351,46 @@ unsigned long get_wchan(struct task_stru
>  	while (count++ < 16);
>  	return 0;
>  }
> +
> +#if !defined(CONFIG_NO_ACCESS_CHECK)
> +int _access_ok(unsigned long addr, unsigned long size)

You may want to change this to CONFIG_ACCESS_CHECK instead, it's a bit
more intuitive than checking ! CONFIG_NO_ACCESS_CHECK.

> --- linux-2.6-orig/arch/blackfin/mach-bf533/boards/ezkit.c	2007-03-21 17:32:30.000000000 +0800
> +++ linux-2.6/arch/blackfin/mach-bf533/boards/ezkit.c	2007-03-21 14:42:17.000000000 +0800
> @@ -6,7 +6,7 @@
>   * Created:      2005
>   * Description:
>   *
> - * Rev:          $Id: ezkit.c,v 1.27 2006/11/24 10:23:54 aubrey Exp $
> + * Rev:          $Id: ezkit.c 2794 2007-03-05 05:27:47Z cooloney $
>   *
>   * Modified:     Robin Getz <rgetz@blackfin.uclinux.org> - Named the boards
>   *               Copyright 2005 National ICT Australia (NICTA)

The RCS tags should be killed off as well, it's clear that they don't
really mean anthing, even in this case.

> diff -purN linux-2.6-orig/arch/blackfin/mach-bf533/boards/generic_board.c linux-2.6/arch/blackfin/mach-bf533/boards/generic_board.c
> --- linux-2.6-orig/arch/blackfin/mach-bf533/boards/generic_board.c	2007-03-21 17:32:30.000000000 +0800
> +++ linux-2.6/arch/blackfin/mach-bf533/boards/generic_board.c	2007-03-21 14:42:17.000000000 +0800
> @@ -6,7 +6,7 @@
>   * Created:      2005
>   * Description:
>   *
> - * Rev:          $Id: generic_board.c,v 1.9 2006/09/23 06:35:21 vapier Exp $
> + * Rev:          $Id: generic_board.c 2684 2007-01-22 03:59:04Z vapier $
>   *
>   * Modified:
>   *               Copyright 2005 National ICT Australia (NICTA)

An then do-nothing patches like this can be left out..

> +#if defined(CONFIG_USB_ISP1760_HCD) || defined(CONFIG_USB_ISP1760_HCD_MODULE)
> +static struct resource bfin_isp1761_resources[] = {
> +	[0] = {
> +		.name	= "isp1761-regs",
> +		.start  = ISP1761_BASE + 0x00000000,
> +		.end    = ISP1761_BASE + 0x000fffff,
> +		.flags  = IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.start  = ISP1761_IRQ,
> +		.end    = ISP1761_IRQ,
> +		.flags  = IORESOURCE_IRQ | IORESOURCE_IRQ_LOWLEVEL,
> +	},

Here you have low level.

> +};
> +
> +static struct platform_device bfin_isp1761_device = {
> +	.name           = "isp1761",
> +	.id             = 0,
> +	.num_resources  = ARRAY_SIZE(bfin_isp1761_resources),
> +	.resource       = bfin_isp1761_resources,
> +};
> +
> +static struct platform_device *bfin_isp1761_devices[] = {
> +	&bfin_isp1761_device,
> +};
> +
> +int __init bfin_isp1761_init(void)
> +{
> +	unsigned int num_devices=ARRAY_SIZE(bfin_isp1761_devices);
> +
> +	printk(KERN_INFO "%s(): registering device resources\n", __FUNCTION__);
> +	set_irq_type(ISP1761_IRQ, IRQF_TRIGGER_FALLING);
> +
But here you set it to falling?

> diff -purN linux-2.6-orig/include/asm-blackfin/asm-offsets.h linux-2.6/include/asm-blackfin/asm-offsets.h
> --- linux-2.6-orig/include/asm-blackfin/asm-offsets.h	1970-01-01 08:00:00.000000000 +0800
> +++ linux-2.6/include/asm-blackfin/asm-offsets.h	2007-03-21 15:21:10.000000000 +0800

This is generated by Kbuild, there's no point in including it in the
diff.

> -
> +/* CSYNC implementation for C file */
>  #if defined(ANOMALY_05000312) && defined(ANOMALY_05000244)

Is there some reason these aren't config options? Perhaps an errata
sub-menu might be more intuitive.

> diff -purN linux-2.6-orig/include/asm-blackfin/entry.h linux-2.6/include/asm-blackfin/entry.h
> --- linux-2.6-orig/include/asm-blackfin/entry.h	2007-03-21 17:32:31.000000000 +0800
> +++ linux-2.6/include/asm-blackfin/entry.h	2007-03-21 14:15:41.000000000 +0800
> @@ -52,10 +52,8 @@
>  #define RESTORE_ALL_SYS		restore_context_no_interrupts
>  #define RESTORE_CONTEXT		restore_context_with_interrupts
>  
> -#define STR(X) STR1(X)
> -#define STR1(X) #X
> -# define PT_OFF_ORIG_P0		208
> -# define PT_OFF_SR		8
> +#define PT_OFF_ORIG_P0		208
> +#define PT_OFF_SR		8
>  

Should these be in asm-offsets?

The rest of the patch looks good, this is certainly an improvement!

Re: [PATCH -mm 1/4] Blackfin: architecture update patch

[Mike Frysinger]

On 3/21/07, Paul Mundt <lethal@linux-sh.org> wrote:
> On Wed, Mar 21, 2007 at 06:06:58PM +0800, Wu, Bryan wrote:
> > +++ linux-2.6/arch/blackfin/Kconfig   2007-03-21 14:38:42.000000000 +0800
> > +config BLACKFIN
> >  config BFIN
>
> Again, there's no reason to have both of these. Pick one and stick with
> it.

we're working on it ... not everything has been converted yet, thus
they're still both alive :/

> > +/* CSYNC implementation for C file */
> >  #if defined(ANOMALY_05000312) && defined(ANOMALY_05000244)
>
> Is there some reason these aren't config options? Perhaps an errata
> sub-menu might be more intuitive.

there is ... in the Kconfig, you select the CPU and the silicon
version ... we then have a header file that determines which anomalies
need to be worked around since the anomaly/silicon binding is static
-mike

Re: [PATCH -mm 1/4] Blackfin: architecture update patch

[Arnd Bergmann]

On Wednesday 21 March 2007, Wu, Bryan wrote:
> 1) Some issues are fixed according to LKML patch review.
> 2) Remove not supported BF535 code
> 3) Fixed some bugs from blackfin.uclinux.org SVN update
> Here is the updated patch for 2.6.21-rc4-mm1

One rather general but important comment:

You need to get used to providing smaller, one fix per mail, patches.
As long as the full tree is waiting in -mm, this is not as important,
as I assume that Andrew will fold the whole architecture support into
a big patch before submitting to Linus, but as soon as it's in, such
big patches will not be acceptable any more.

If you're not already doing it, look into how 'quilt' or similar tools
help you with this.

	Arnd <><