| Message ID | 1284027632-32573-3-git-send-email-tie-fei.zang@freescale.com |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 9, 2010 at 12:20, Roy Zang <tie-fei.zang@freescale.com> wrote: > From: Lan Chunhe-B25806 <b25806@freescale.com> > > When system uses 36bit physical address, res.start is 36bit > physical address. But the function of in_be32 returns 32bit > physical address. Then both of them compared each other is > wrong. So by converting the address of res.start into > the right format fixes this issue. > /** > + * fsl_lbc_addr - convert the base address > + * @addr_base: base address of the memory bank > + * > + * This function converts a base address of lbc into the right format for the BR > + * registers. If the SOC has eLBC then it returns 32bit physical address else > + * it returns 34bit physical address for local bus(Example: MPC8641). > + */ > +unsigned int fsl_lbc_addr(phys_addr_t addr_base) ^^^^^^^^^^^^ Shouldn't this be u32 or __be32, for consistency with the actual comparisons below? > @@ -52,7 +76,7 @@ int fsl_lbc_find(phys_addr_t addr_base) > __be32 br = in_be32(&lbc->bank[i].br); > __be32 or = in_be32(&lbc->bank[i].or); > > - if (br & BR_V && (br & or & BR_BA) == addr_base) > + if (br & BR_V && (br & or & BR_BA) == fsl_lbc_addr(addr_base)) > return i; > @@ -851,7 +851,7 @@ static int __devinit fsl_elbc_nand_probe(struct platform_device *dev, > (in_be32(&lbc->bank[bank].br) & BR_MSEL) == BR_MS_FCM && > (in_be32(&lbc->bank[bank].br) & > in_be32(&lbc->bank[bank].or) & BR_BA) > - == res.start) > + == fsl_lbc_addr(res.start)) > break; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, Sep 09, 2010 at 06:20:32PM +0800, Roy Zang wrote: [...] > /** > + * fsl_lbc_addr - convert the base address > + * @addr_base: base address of the memory bank > + * > + * This function converts a base address of lbc into the right format for the BR > + * registers. If the SOC has eLBC then it returns 32bit physical address else > + * it returns 34bit physical address for local bus(Example: MPC8641). > + */ It returns 34bit physical address encoded in a 32 bit word, right? Because, IIRC, 'unsigned int' is always 32 bit. Worth mentioning this fact. > +unsigned int fsl_lbc_addr(phys_addr_t addr_base) > +{ > + void *dev; struct device_node *np; > + int compatible; > + > + dev = fsl_lbc_ctrl_dev->dev->of_node; > + compatible = of_device_is_compatible(dev, "fsl,elbc"); > + > + if (compatible) > + return addr_base & 0xffff8000; > + else > + return (addr_base & 0x0ffff8000ull) > + | ((addr_base & 0x300000000ull) >> 19); > +} > +EXPORT_SYMBOL(fsl_lbc_addr); Almost perfect. I'm not sure if 'unsigned int' is technically correct return type for this function though. I guess it should be u32. Also, the function may be a bit more understandable and shorter: u32 fsl_lbc_addr(phys_addr_t addr) { struct device_node *np = fsl_lbc_ctrl_dev->dev->of_node; u32 addrl = addr & 0xffff8000; if (of_device_is_compatible(np, "fsl,elbc")) return addrl; return addrl | ((addr & 0x300000000ull) >> 19); } EXPORT_SYMBOL(fsl_lbc_addr); Thanks,
> -----Original Message----- > From: geert.uytterhoeven@gmail.com [mailto:geert.uytterhoeven@gmail.com] On > Behalf Of Geert Uytterhoeven > Sent: Thursday, September 09, 2010 19:06 PM > To: Zang Roy-R61911 > Cc: linux-mtd@lists.infradead.org; Wood Scott-B07421; dedekind1@gmail.com; Lan > Chunhe-B25806; linuxppc-dev@ozlabs.org; akpm@linux-foundation.org; > dwmw2@infradead.org; Gala Kumar-B11780 > Subject: Re: [PATCH v2 3/3][MTD] P4080/mtd: Fix the freescale lbc issue with > 36bit mode > > On Thu, Sep 9, 2010 at 12:20, Roy Zang <tie-fei.zang@freescale.com> wrote: > > From: Lan Chunhe-B25806 <b25806@freescale.com> > > > > When system uses 36bit physical address, res.start is 36bit > > physical address. But the function of in_be32 returns 32bit > > physical address. Then both of them compared each other is > > wrong. So by converting the address of res.start into > > the right format fixes this issue. > > > /** > > + * fsl_lbc_addr - convert the base address > > + * @addr_base: base address of the memory bank > > + * > > + * This function converts a base address of lbc into the right format for > the BR > > + * registers. If the SOC has eLBC then it returns 32bit physical address > else > > + * it returns 34bit physical address for local bus(Example: MPC8641). > > + */ > > +unsigned int fsl_lbc_addr(phys_addr_t addr_base) > ^^^^^^^^^^^^ > Shouldn't this be u32 or __be32, for consistency with the actual > comparisons below? __be32 is better. Thanks. Roy
> -----Original Message----- > From: Anton Vorontsov [mailto:cbouatmailru@gmail.com] > Sent: Thursday, September 09, 2010 19:42 PM > To: Zang Roy-R61911 > Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org; dedekind1@gmail.com; > akpm@linux-foundation.org; Lan Chunhe-B25806; Wood Scott-B07421; Gala Kumar- > B11780; linuxppc-dev@ozlabs.org > Subject: Re: [PATCH v2 3/3][MTD] P4080/mtd: Fix the freescale lbc issue with > 36bit mode > > On Thu, Sep 09, 2010 at 06:20:32PM +0800, Roy Zang wrote: > [...] > > /** > > + * fsl_lbc_addr - convert the base address > > + * @addr_base: base address of the memory bank > > + * > > + * This function converts a base address of lbc into the right format for > the BR > > + * registers. If the SOC has eLBC then it returns 32bit physical address > else > > + * it returns 34bit physical address for local bus(Example: MPC8641). > > + */ > > It returns 34bit physical address encoded in a 32 bit word, > right? Yes. >Because, IIRC, 'unsigned int' is always 32 bit. > > Worth mentioning this fact. Agree. The comment is a little bit confusing. Will update. > > > +unsigned int fsl_lbc_addr(phys_addr_t addr_base) > > +{ > > + void *dev; > > struct device_node *np; > > > + int compatible; > > + > > + dev = fsl_lbc_ctrl_dev->dev->of_node; > > + compatible = of_device_is_compatible(dev, "fsl,elbc"); > > + > > + if (compatible) > > + return addr_base & 0xffff8000; > > + else > > + return (addr_base & 0x0ffff8000ull) > > + | ((addr_base & 0x300000000ull) >> 19); > > +} > > +EXPORT_SYMBOL(fsl_lbc_addr); > > Almost perfect. I'm not sure if 'unsigned int' is technically > correct return type for this function though. I guess it should > be u32. > What is the different for unsigned int and u32? I think they are same. > Also, the function may be a bit more understandable and shorter: > > u32 fsl_lbc_addr(phys_addr_t addr) > { > struct device_node *np = fsl_lbc_ctrl_dev->dev->of_node; > u32 addrl = addr & 0xffff8000; if addr is 34 bit long, is this correct? Thanks. Roy
On Mon, Sep 13, 2010 at 2:30 AM, Zang Roy-R61911 <r61911@freescale.com> wrote:
> What is the different for unsigned int and u32? I think they are same.
Roy, please don't ever write code that assumes that sizeof(int) == 4.
There's a reason why we have unsized integer types (like int, long,
and short) and sized integer types (like u8, u16, u32). If you want
an integer of a specific size, you should use a sized integer type.
On Mon, 2010-09-13 at 09:10 -0500, Timur Tabi wrote: > On Mon, Sep 13, 2010 at 2:30 AM, Zang Roy-R61911 <r61911@freescale.com> wrote: > > > What is the different for unsigned int and u32? I think they are same. > > Roy, please don't ever write code that assumes that sizeof(int) == 4. > There's a reason why we have unsized integer types (like int, long, > and short) and sized integer types (like u8, u16, u32). If you want > an integer of a specific size, you should use a sized integer type. Yes, sizeof(int) == 4 assumption is not good. But sizeof(int) >= 4 is perfectly fine. It is OK to rely on the fact that unsigned int is _at least_ 32-bit, not less. And if you know 32 bits is enough, and you are fine with more, it is _better_ to avoid u32. Simply because with unsigned int you do not limit the compiler and CPU and let them use native integers, rather than strictly 32-bit. This potentially gives the compiler and CPU more room for optimization. I see people use u32 and the friends too much. It is safe and better to use native types, unless you really have to make the variable to be strictly u32. I did not follow this particular conversation and do not judge which type is better in this case. I am talking in general. :-)
On Mon, Sep 13, 2010 at 9:27 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > Yes, sizeof(int) == 4 assumption is not good. > > But sizeof(int) >= 4 is perfectly fine. I have to disagree. I don't see how you can say that == is not okay, but >= is okay. That just doesn't make any sense. Obviously, you have to make some assumptions about sizeof(int), but that assumption should be nothing more than "it should be big enough for these small numbers that I'm using". > I see people use u32 and the friends too much. I agree with that.
On Mon, 13 Sep 2010 00:22:10 -0700 "Zang Roy-R61911" <r61911@freescale.com> wrote: > > > > -----Original Message----- > > From: geert.uytterhoeven@gmail.com [mailto:geert.uytterhoeven@gmail.com] On > > Behalf Of Geert Uytterhoeven > > Sent: Thursday, September 09, 2010 19:06 PM > > To: Zang Roy-R61911 > > Cc: linux-mtd@lists.infradead.org; Wood Scott-B07421; dedekind1@gmail.com; Lan > > Chunhe-B25806; linuxppc-dev@ozlabs.org; akpm@linux-foundation.org; > > dwmw2@infradead.org; Gala Kumar-B11780 > > Subject: Re: [PATCH v2 3/3][MTD] P4080/mtd: Fix the freescale lbc issue with > > 36bit mode > > > > On Thu, Sep 9, 2010 at 12:20, Roy Zang <tie-fei.zang@freescale.com> wrote: > > > From: Lan Chunhe-B25806 <b25806@freescale.com> > > > > > > When system uses 36bit physical address, res.start is 36bit > > > physical address. But the function of in_be32 returns 32bit > > > physical address. Then both of them compared each other is > > > wrong. So by converting the address of res.start into > > > the right format fixes this issue. > > > > > /** > > > + * fsl_lbc_addr - convert the base address > > > + * @addr_base: base address of the memory bank > > > + * > > > + * This function converts a base address of lbc into the right format for > > the BR > > > + * registers. If the SOC has eLBC then it returns 32bit physical address > > else > > > + * it returns 34bit physical address for local bus(Example: MPC8641). > > > + */ > > > +unsigned int fsl_lbc_addr(phys_addr_t addr_base) > > ^^^^^^^^^^^^ > > Shouldn't this be u32 or __be32, for consistency with the actual > > comparisons below? > __be32 is better. I disagree, the return from this function is native-endian. It should be u32. -Scott
On Mon, 2010-09-13 at 09:35 -0500, Timur Tabi wrote: > On Mon, Sep 13, 2010 at 9:27 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > > > Yes, sizeof(int) == 4 assumption is not good. > > > > But sizeof(int) >= 4 is perfectly fine. > > I have to disagree. I don't see how you can say that == is not okay, > but >= is okay. If I assume that int == 4, which is true for all platform ATM, and will most probably stay true for very long time, I still make an unnecessary assumption which C99 does not guarantee. This is just not very clean. So for things which require exactly 32 bits, u32 or C99 uint32_t should be used. On the other hand, if a have a local variable "var" which needs to store values which I know will always fit 32 bits, and I do not use it in operations (I/O, etc) which require exactly 32 bits, it is better to use unsigned int for it. Unsigned int is reliably >= 4 in Linux. This is wat I meant that sizeof(int) >= 4 is perfectly fine. One everywhere, of course, but in many cases. To put it differently, if there is no special reason to limit the variable by exactly 32 bits, it is better to use int.
On Mon, Sep 13, 2010 at 11:45 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Unsigned int is reliably >= 4 in Linux.
Not exactly. sizeof(unsigned int) is effectively never greater than 4
in Linux (I think it's still 32 bits even on a 64-bit kernel), so it
makes no sense to say >=. So since you can't say that sizeof(int) ==
4, and you can't say that sizeof(int) > 4, then you certainly can't
say that sizeof(int) >= 4.
On Mon, 2010-09-13 at 13:36 -0500, Timur Tabi wrote: > On Mon, Sep 13, 2010 at 11:45 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > > Unsigned int is reliably >= 4 in Linux. > > Not exactly. sizeof(unsigned int) is effectively never greater than 4 > in Linux I know and said the same in the other paragraph which you cut. > (I think it's still 32 bits even on a 64-bit kernel) Yes. > , so it > makes no sense to say >=. So since you can't say that sizeof(int) == > 4, and you can't say that sizeof(int) > 4, then you certainly can't > say that sizeof(int) >= 4. It also makes no sense to send "makes no sense" answer 2 times in a row and it makes no sense to not read and not try to understand the e-mail you are replying (even though the english of the writer is not perfect) So it makes no sense to continue arguing. But it makes some sense to go sleep, good night.
On Mon, 13 Sep 2010 13:36:45 -0500 Timur Tabi <timur.tabi@gmail.com> wrote: > On Mon, Sep 13, 2010 at 11:45 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > > Unsigned int is reliably >= 4 in Linux. > > Not exactly. sizeof(unsigned int) is effectively never greater than 4 > in Linux (I think it's still 32 bits even on a 64-bit kernel), so it > makes no sense to say >=. I think the point is that if a machine/ABI comes along with sizeof(int) > 4 (e.g. if there's a significant penalty for working with anything < 64-bit), Linux might consider supporting that. Whereas if someone points to their microcontroller or ancient 286 with sizeof(int) == 2, the answer would be, "Here's a nickel, kid -- get a real computer". -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Tuesday, September 14, 2010 0:28 AM > To: Zang Roy-R61911 > Cc: Geert Uytterhoeven; linux-mtd@lists.infradead.org; Wood Scott-B07421; > dedekind1@gmail.com; Lan Chunhe-B25806; linuxppc-dev@ozlabs.org; akpm@linux- > foundation.org; dwmw2@infradead.org; Gala Kumar-B11780 > Subject: Re: [PATCH v2 3/3][MTD] P4080/mtd: Fix the freescale lbc issue with > 36bit mode > > On Mon, 13 Sep 2010 00:22:10 -0700 > "Zang Roy-R61911" <r61911@freescale.com> wrote: > > > > > > > > -----Original Message----- > > > From: geert.uytterhoeven@gmail.com [mailto:geert.uytterhoeven@gmail.com] > On > > > Behalf Of Geert Uytterhoeven > > > Sent: Thursday, September 09, 2010 19:06 PM > > > To: Zang Roy-R61911 > > > Cc: linux-mtd@lists.infradead.org; Wood Scott-B07421; dedekind1@gmail.com; > Lan > > > Chunhe-B25806; linuxppc-dev@ozlabs.org; akpm@linux-foundation.org; > > > dwmw2@infradead.org; Gala Kumar-B11780 > > > Subject: Re: [PATCH v2 3/3][MTD] P4080/mtd: Fix the freescale lbc issue > with > > > 36bit mode > > > > > > On Thu, Sep 9, 2010 at 12:20, Roy Zang <tie-fei.zang@freescale.com> wrote: > > > > From: Lan Chunhe-B25806 <b25806@freescale.com> > > > > > > > > When system uses 36bit physical address, res.start is 36bit > > > > physical address. But the function of in_be32 returns 32bit > > > > physical address. Then both of them compared each other is > > > > wrong. So by converting the address of res.start into > > > > the right format fixes this issue. > > > > > > > /** > > > > + * fsl_lbc_addr - convert the base address > > > > + * @addr_base: base address of the memory bank > > > > + * > > > > + * This function converts a base address of lbc into the right format > for > > > the BR > > > > + * registers. If the SOC has eLBC then it returns 32bit physical > address > > > else > > > > + * it returns 34bit physical address for local bus(Example: MPC8641). > > > > + */ > > > > +unsigned int fsl_lbc_addr(phys_addr_t addr_base) > > > ^^^^^^^^^^^^ > > > Shouldn't this be u32 or __be32, for consistency with the actual > > > comparisons below? > > __be32 is better. > > I disagree, the return from this function is native-endian. It should > be u32. But the return of this value is ONLY used for br comparing. It is big endia. Thanks. Roy
> -----Original Message----- > From: Artem Bityutskiy [mailto:dedekind1@gmail.com] > Sent: Monday, September 13, 2010 22:28 PM > To: Timur Tabi > Cc: Zang Roy-R61911; Anton Vorontsov; Wood Scott-B07421; Lan Chunhe-B25806; > linuxppc-dev@ozlabs.org; linux-mtd@lists.infradead.org; akpm@linux- > foundation.org; dwmw2@infradead.org; Gala Kumar-B11780 > Subject: Re: [PATCH v2 3/3][MTD] P4080/mtd: Fix the freescale lbc issue with > 36bit mode > > On Mon, 2010-09-13 at 09:10 -0500, Timur Tabi wrote: > > On Mon, Sep 13, 2010 at 2:30 AM, Zang Roy-R61911 <r61911@freescale.com> > wrote: > > > > > What is the different for unsigned int and u32? I think they are same. > > > > Roy, please don't ever write code that assumes that sizeof(int) == 4. > > There's a reason why we have unsized integer types (like int, long, > > and short) and sized integer types (like u8, u16, u32). If you want > > an integer of a specific size, you should use a sized integer type. > > Yes, sizeof(int) == 4 assumption is not good. > > But sizeof(int) >= 4 is perfectly fine. > > It is OK to rely on the fact that unsigned int is _at least_ 32-bit, not > less. And if you know 32 bits is enough, and you are fine with more, it > is _better_ to avoid u32. Simply because with unsigned int you do not > limit the compiler and CPU and let them use native integers, rather than > strictly 32-bit. This potentially gives the compiler and CPU more room > for optimization. > > I see people use u32 and the friends too much. It is safe and better to > use native types, unless you really have to make the variable to be > strictly u32. > > I did not follow this particular conversation and do not judge which > type is better in this case. I am talking in general. :-) The function will return fixed 32bit. so u32 should be OK. Thanks. Roy
On Mon, Sep 13, 2010 at 11:09 PM, Zang Roy-R61911 <r61911@freescale.com> wrote: >> > > Shouldn't this be u32 or __be32, for consistency with the actual >> > > comparisons below? >> > __be32 is better. >> >> I disagree, the return from this function is native-endian. It should >> be u32. > But the return of this value is ONLY used for br comparing. It is big endia. > Thanks. No, Scott is right. The return value is native-endian (NE). You've converted it from BE to NE already.
diff --git a/arch/powerpc/include/asm/fsl_lbc.h b/arch/powerpc/include/asm/fsl_lbc.h index 9b95eab..bff85c8 100644 --- a/arch/powerpc/include/asm/fsl_lbc.h +++ b/arch/powerpc/include/asm/fsl_lbc.h @@ -249,6 +249,7 @@ struct fsl_upm { int width; }; +extern unsigned int fsl_lbc_addr(phys_addr_t addr_base); extern int fsl_lbc_find(phys_addr_t addr_base); extern int fsl_upm_find(phys_addr_t addr_base, struct fsl_upm *upm); diff --git a/arch/powerpc/sysdev/fsl_lbc.c b/arch/powerpc/sysdev/fsl_lbc.c index f4eca8d..3a09e90 100644 --- a/arch/powerpc/sysdev/fsl_lbc.c +++ b/arch/powerpc/sysdev/fsl_lbc.c @@ -31,6 +31,30 @@ struct fsl_lbc_ctrl *fsl_lbc_ctrl_dev; EXPORT_SYMBOL(fsl_lbc_ctrl_dev); /** + * fsl_lbc_addr - convert the base address + * @addr_base: base address of the memory bank + * + * This function converts a base address of lbc into the right format for the BR + * registers. If the SOC has eLBC then it returns 32bit physical address else + * it returns 34bit physical address for local bus(Example: MPC8641). + */ +unsigned int fsl_lbc_addr(phys_addr_t addr_base) +{ + void *dev; + int compatible; + + dev = fsl_lbc_ctrl_dev->dev->of_node; + compatible = of_device_is_compatible(dev, "fsl,elbc"); + + if (compatible) + return addr_base & 0xffff8000; + else + return (addr_base & 0x0ffff8000ull) + | ((addr_base & 0x300000000ull) >> 19); +} +EXPORT_SYMBOL(fsl_lbc_addr); + +/** * fsl_lbc_find - find Localbus bank * @addr_base: base address of the memory bank * @@ -52,7 +76,7 @@ int fsl_lbc_find(phys_addr_t addr_base) __be32 br = in_be32(&lbc->bank[i].br); __be32 or = in_be32(&lbc->bank[i].or); - if (br & BR_V && (br & or & BR_BA) == addr_base) + if (br & BR_V && (br & or & BR_BA) == fsl_lbc_addr(addr_base)) return i; } diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c index 64c840f..6dec268 100644 --- a/drivers/mtd/nand/fsl_elbc_nand.c +++ b/drivers/mtd/nand/fsl_elbc_nand.c @@ -851,7 +851,7 @@ static int __devinit fsl_elbc_nand_probe(struct platform_device *dev, (in_be32(&lbc->bank[bank].br) & BR_MSEL) == BR_MS_FCM && (in_be32(&lbc->bank[bank].br) & in_be32(&lbc->bank[bank].or) & BR_BA) - == res.start) + == fsl_lbc_addr(res.start)) break; if (bank >= MAX_BANKS) {