LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH net-next 3/3] powerpc/mpc5xxx: add OF platform binding doc for FSL MSCAN devices
From: Wolfram Sang @ 2010-01-02 14:05 UTC (permalink / raw)
  To: Wolfgang Grandegger
  Cc: Socketcan-core, Netdev, Devicetree-discuss, Linuxppc-dev,
	Wolfgang Grandegger
In-Reply-To: <1262420274-16586-4-git-send-email-wg@grandegger.com>

[-- Attachment #1: Type: text/plain, Size: 3975 bytes --]

On Sat, Jan 02, 2010 at 09:17:54AM +0100, Wolfgang Grandegger wrote:
> From: Wolfgang Grandegger <wg@denx.de>
> 
> This patch adds documentation for the MSCAN OF device bindings for
> the MPC512x and moves the one for the MPC5200 to the new common file
> "Documentation/powerpc/dts-bindings/fsl/can.txt".
> 
> Signed-off-by: Wolfgang Grandegger <wg@denx.de>

Looks good to me (except for the typos ;))

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

> ---
>  Documentation/powerpc/dts-bindings/fsl/can.txt     |   53 ++++++++++++++++++++
>  Documentation/powerpc/dts-bindings/fsl/mpc5200.txt |    9 +---
>  2 files changed, 54 insertions(+), 8 deletions(-)
>  create mode 100644 Documentation/powerpc/dts-bindings/fsl/can.txt
> 
> diff --git a/Documentation/powerpc/dts-bindings/fsl/can.txt b/Documentation/powerpc/dts-bindings/fsl/can.txt
> new file mode 100644
> index 0000000..44cfb61
> --- /dev/null
> +++ b/Documentation/powerpc/dts-bindings/fsl/can.txt
> @@ -0,0 +1,53 @@
> +CAN Device Tree Bindings
> +------------------------
> +
> +(c) 2006-2009 Secret Lab Technologies Ltd
> +Grant Likely <grant.likely@secretlab.ca>
> +
> +fsl,mpc5200-mscan nodes
> +-----------------------
> +In addition to the required compatible-, reg- and interrupt-properites, you can

properties.

> +also specify which clock source shall be used for the controller:
> +
> +- fsl,mscan-clock-source : a string describing the clock source. Valid values
> +			   are:	"ip" for ip bus clock
> +				 "ref" for reference clock (XTAL)
> +			   "ref" is default in case this property is not
> +			   present.
> +
> +fsl,mpc5121-mscan nodes
> +-----------------------
> +In addition to the required compatible-, reg- and interrupt-properites, you can

properties.

> +also specify which clock source shall be used for the controller:
> +
> +- fsl,mscan-clock-source : a string describing the clock source. Valid values
> +			   are:	"ip" for ip bus clock
> +				"ref" for reference clock
> +				"sys" for system clock
> +			   If this property is not present, an optimal CAN
> +			   clock source and frequency based on the system
> +			   will be selected. If this is not possible, the
> +			   reference clock will be used.
> +
> +- fsl,mscan-clock-divider: for the reference and system clock an additional
> +			   clock divider can be specified. By default a

I wonder if there should be a comma after 'default'. Maybe a native speaker can
help?

> +			   value of 1 is used.
> +
> +Examples:
> +	can@1300 {
> +		compatible = "fsl,mpc5121-mscan";
> +		cell-index = <0>;
> +		interrupts = <12 0x8>;
> +		interrupt-parent = < &ipic >;
> +		reg = <0x1300 0x80>;
> +	};
> +
> +	can@1380 {
> +		compatible = "fsl,mpc5121-mscan";
> +		cell-index = <1>;
> +		interrupts = <13 0x8>;
> +		interrupt-parent = < &ipic >;
> +		reg = <0x1380 0x80>;
> +		fsl,mscan-clock-source = "ref";
> +		fsl,mscan-clock-divider = <3>;
> +	};
> diff --git a/Documentation/powerpc/dts-bindings/fsl/mpc5200.txt b/Documentation/powerpc/dts-bindings/fsl/mpc5200.txt
> index 5c6602d..4ccb2cd 100644
> --- a/Documentation/powerpc/dts-bindings/fsl/mpc5200.txt
> +++ b/Documentation/powerpc/dts-bindings/fsl/mpc5200.txt
> @@ -195,11 +195,4 @@ External interrupts:
>  
>  fsl,mpc5200-mscan nodes
>  -----------------------
> -In addition to the required compatible-, reg- and interrupt-properites, you can
> -also specify which clock source shall be used for the controller:
> -
> -- fsl,mscan-clock-source- a string describing the clock source. Valid values
> -			  are:	"ip" for ip bus clock
> -				"ref" for reference clock (XTAL)
> -			  "ref" is default in case this property is not
> -			  present.
> +See file can.txt in this directory.
> -- 
> 1.6.2.5
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply

* Re: [PATCH net-next 0/3] can: mscan-mpc5xxx: add support for the Freescale MPC512x
From: Wolfgang Grandegger @ 2010-01-02 15:32 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: Socketcan-core, Netdev, Devicetree-discuss, Linuxppc-dev
In-Reply-To: <20100102132722.GA2239@pengutronix.de>

Wolfram Sang wrote:
> On Sat, Jan 02, 2010 at 09:17:51AM +0100, Wolfgang Grandegger wrote:
>> This patch series adds support for the MPC512x from Freescale to
>> the mpc5xxx_can MSCAN driver.
> 
> Yay, cool :) I will test the patches with our board when I am back in the
> office next week. So far, a few comments by having a look will follow.

That would be nice.

> Happy new 2010,

Same to everybody.

Thanks,

Wolfgang.

^ permalink raw reply

* Re: [PATCH net-next 1/3] can: mscan: fix improper return if dlc < 8 in start_xmit function
From: Wolfgang Grandegger @ 2010-01-02 15:34 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Socketcan-core, Netdev, Devicetree-discuss, Linuxppc-dev,
	Wolfgang Grandegger
In-Reply-To: <20100102133041.GB2239@pengutronix.de>

Wolfram Sang wrote:
> On Sat, Jan 02, 2010 at 09:17:52AM +0100, Wolfgang Grandegger wrote:
>> From: Wolfgang Grandegger <wg@denx.de>
>>
>> The start_xmit function of the MSCAN Driver did return improperly if
>> the CAN dlc check failed (skb not freed and invalid return code). This
>> patch adds a proper check of the frame lenght and data size and returns
>> now correctly. Furthermore, a typo has been fixed.
> 
> A few little things, but in general:
> 
> Acked-by: Wolfram Sang <w.sang@pengutronix.de>
> 
>> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
>> ---
>>  drivers/net/can/mscan/mscan.c |   11 ++++++++---
>>  1 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
>> index 07346f8..abdf5e8 100644
>> --- a/drivers/net/can/mscan/mscan.c
>> +++ b/drivers/net/can/mscan/mscan.c
>> @@ -4,7 +4,7 @@
>>   * Copyright (C) 2005-2006 Andrey Volkov <avolkov@varma-el.com>,
>>   *                         Varma Electronics Oy
>>   * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
>> - * Copytight (C) 2008-2009 Pengutronix <kernel@pengutronix.de>
>> + * Copyright (C) 2008-2009 Pengutronix <kernel@pengutronix.de>
>>   *
>>   * This program is free software; you can redistribute it and/or modify
>>   * it under the terms of the version 2 of the GNU General Public License
>> @@ -177,8 +177,13 @@ static netdev_tx_t mscan_start_xmit(struct sk_buff *skb, struct net_device *dev)
>>  	int i, rtr, buf_id;
>>  	u32 can_id;
>>  
>> -	if (frame->can_dlc > 8)
>> -		return -EINVAL;
>> +	if (skb->len != sizeof(*frame) || frame->can_dlc > 8) {
>> +		dev_err(dev->dev.parent,
>> +			"Dropping non-conform paket: len %d, can_dlc %d\n",
> 
> s/paket/packet/
> 
>> +			skb->len, frame->can_dlc);
> 
> Also, shouldn't it rather be %u (sk_buff->len is unsigned int)?

Will fix both issue.

Wolfgang.

^ permalink raw reply

* Re: [PATCH net-next 2/3] can: mscan-mpc5xxx: add support for the MPC521x processor
From: Wolfgang Grandegger @ 2010-01-02 16:08 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Socketcan-core, Netdev, Devicetree-discuss, Linuxppc-dev,
	Wolfgang Grandegger
In-Reply-To: <20100102135734.GC2239@pengutronix.de>

Wolfram Sang wrote:
> On Sat, Jan 02, 2010 at 09:17:53AM +0100, Wolfgang Grandegger wrote:
>> From: Wolfgang Grandegger <wg@denx.de>
>>
>> The main differences compared to the MSCAN on the MPC5200 are:
>>
>> - More flexibility in choosing the CAN source clock and frequency:
>>
>>   Three different clock sources can be selected: "ip", "ref" or "sys".
>>   For the latter two, a clock divider can be defined as well. If the
>>   clock source is not specified by the device tree, we first try to
>>   find an optimal CAN source clock based on the system clock. If that
>>   is not possible, the reference clock will be used.
>>
>> - The behavior of bus-off recovery is configurable:
>>
>>   To comply with the usual handling of Socket-CAN bus-off recovery,
>>   "recovery on request" is selected (instead of automatic recovery).
>>
>> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
>> ---
>>  drivers/net/can/mscan/Kconfig       |    2 +-
>>  drivers/net/can/mscan/mpc5xxx_can.c |  234 +++++++++++++++++++++++++++++------
>>  drivers/net/can/mscan/mscan.c       |   41 +++++--
>>  drivers/net/can/mscan/mscan.h       |   81 ++++++------
>>  4 files changed, 271 insertions(+), 87 deletions(-)
>>
[snip]

>> +#else /* !CONFIG_PPC_MPC5200 */
>> +static u32 __devinit mpc52xx_can_get_clock(struct of_device *ofdev,
>> +					   const char *clock_name,
>> +					   int *mscan_clksrc)
>> +{
>> +	return 0;
>> +}
>> +#endif /* CONFIG_PPC_MPC5200 */
> 
> Hmmm, I don't really like those empty functions. I once used the data-field of
> struct of_device_id, which carried a function pointer to a specific
> init-function for the matched device. What do you think about such an approach?

Often the problem is that the function will not compile on the other MPC
arch. This is not true here. So, the main reason for the #ifdefs is
space saving. Your approach will not help in both cases.

>> +
>> +#ifdef CONFIG_PPC_MPC512x
>> +struct mpc512x_clockctl {
>> +	u32 spmr;		/* System PLL Mode Reg */
>> +	u32 sccr[2];		/* System Clk Ctrl Reg 1 & 2 */
>> +	u32 scfr1;		/* System Clk Freq Reg 1 */
>> +	u32 scfr2;		/* System Clk Freq Reg 2 */
>> +	u32 reserved;
>> +	u32 bcr;		/* Bread Crumb Reg */
>> +	u32 pccr[12];		/* PSC Clk Ctrl Reg 0-11 */
>> +	u32 spccr;		/* SPDIF Clk Ctrl Reg */
>> +	u32 cccr;		/* CFM Clk Ctrl Reg */
>> +	u32 dccr;		/* DIU Clk Cnfg Reg */
>> +	u32 mccr[4];		/* MSCAN Clk Ctrl Reg 1-3 */
>> +};
>> +
>> +static struct of_device_id mpc512x_clock_ids[] __devinitdata = {
>> +	{ .compatible = "fsl,mpc5121-clock", },
>> +	{}
>> +};
>> +
>> +static u32  __devinit mpc512x_can_get_clock(struct of_device *ofdev,
>> +					    const char *clock_name,
>> +					    int *mscan_clksrc,
>> +					    ssize_t mscan_addr)
>> +{
>> +	struct mpc512x_clockctl __iomem *clockctl;
>> +	struct device_node *np_clock;
>> +	struct clk *sys_clk, *ref_clk;
>> +	int plen, clockidx, clocksrc = -1;
>> +	u32 sys_freq, val, clockdiv = 1, freq = 0;
>> +	const u32 *pval;
>> +
>> +	np_clock = of_find_matching_node(NULL, mpc512x_clock_ids);
>> +	if (!np_clock) {
>> +		dev_err(&ofdev->dev, "couldn't find clock node\n");
>> +		return -ENODEV;
>> +	}
>> +	clockctl = of_iomap(np_clock, 0);
>> +	if (!clockctl) {
>> +		dev_err(&ofdev->dev, "couldn't map clock registers\n");
>> +		return 0;
>> +	}
>> +
>> +	/* Determine the MSCAN device index from the physical address */
>> +	clockidx = (mscan_addr & 0x80) ? 1 : 0;
>> +	if (mscan_addr & 0x2000)
>> +		clockidx += 2;
> 
> The PSCs use 'cell-index', here we use mscan_addr to derive the index. This is
> not consistent, but should be IMHO. Now, which is the preferred way? I think
> I'd go for 'cell-index', as other processors might have mscan_addr shuffled.
> Also, we could use 'of_iomap' again in the probe_routine.

I understood that "cell-index" is deprecated and it has been removed
from many nodes. That's why I used the address to derive the index.

I will fix all other issues.

Wolfgang.

^ permalink raw reply

* Re: [PATCH 1/2] pmac-zilog: add platform driver
From: Finn Thain @ 2010-01-02 16:39 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: linux-m68k, linuxppc-dev
In-Reply-To: <10f740e81001020443h78c615d2l206b99c386600108@mail.gmail.com>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2338 bytes --]


On Sat, 2 Jan 2010, Geert Uytterhoeven wrote:

> On Tue, Nov 17, 2009 at 10:04, Finn Thain <fthain@telegraphics.com.au> 
> wrote:
> > Add platform driver to the pmac-zilog driver for mac 68k, putting the 
> > powermac-specific bits inside #ifdef CONFIG_PPC_PMAC.
> 
> > --- linux-2.6.31.orig/drivers/serial/pmac_zilog.c       2009-11-17 17:07:28.000000000 +1100
> > +++ linux-2.6.31/drivers/serial/pmac_zilog.c    2009-11-17 17:07:38.000000000 +1100
> 
> > @@ -1427,6 +1439,8 @@ static struct uart_ops pmz_pops = {
> >  #endif
> >  };
> >
> > +#ifdef CONFIG_PPC_PMAC
> > +
> >  /*
> >  * Setup one port structure after probing, HW is down at this point,
> >  * Unlike sunzilog, we don't need to pre-init the spinlock as we don't
> > @@ -1823,6 +1837,88 @@ next:
> >        return 0;
> >  }
> >
> > +#else
> > +
> > +extern struct platform_device scc_a_pdev, scc_b_pdev;
> 
> scripts/checkpatch.pl doesn't like this extern, and it's right.
> Can't this be found using standard platform device/driver matching?

The console initcall and arch initcall order didn't permit me to easily 
gather the bootinfo data and populate the platform device resources early 
enough. (On powermacs there is the open firmware device tree, but of 
course, we don't have one.)

I would like to further adopt the driver model in order to ditch the 
macintosh_config global, and I'd also like to have proper nubus device 
matching. But I think that the serial console device is a bit exceptional 
so I'm not too fussed about these two globals.

Anyway, I don't know of a better way to do the serial console but I'm open 
to suggestions.

> BTW, there are a few other minor checkpatch issues with some of the 
> other patches in the series, too.

I ran checkpatch on all those patches before I submitted them. I ignored 
some of the complaints about whitespace where I felt that checkpatch got 
it wrong (space character following tab character, IIRC).

checkpatch found lots of mistakes that I did fix, but it can't determine 
the most human readable style in all cases, especially where consistency 
with the surrounding code is actually more conducive to readability than 
strict but sporadic conformance to simple rules would be.

Finn

> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 

^ permalink raw reply

* Re: How to access PPC460EX SDRAM space from PCI/PCIe.
From: Lonsn @ 2010-01-03  3:23 UTC (permalink / raw)
  To: Tirumala Reddy Marri; +Cc: sr, linuxppc-dev
In-Reply-To: <AC5E1C3367E37D44970B81A6ADD1DA2C08882EAA@SDCEXCHANGE01.ad.amcc.com>

Hi:
Thank you for your reply and Happy New Year!

According to the canyonlands.dts and arch/powerpc/sysdev/ppc4xx_pci.c
files in the linux kernel, 2GB has been mappend to inbound PCI memory
region,
The followings are PIM init codes related in the kernel:
/******************************************/
sa = (0xffffffffffffffffull << ilog2(size));
 if (res->flags & IORESOURCE_PREFETCH)
         sa |= 0x8;

 out_le32(mbase + PECFG_BAR0HMPA, RES_TO_U32_HIGH(sa));
 out_le32(mbase + PECFG_BAR0LMPA, RES_TO_U32_LOW(sa));

 /* The setup of the split looks weird to me ... let's see
  * if it works
  */
 out_le32(mbase + PECFG_PIM0LAL, 0x00000000);
 out_le32(mbase + PECFG_PIM0LAH, 0x00000000);
 out_le32(mbase + PECFG_PIM1LAL, 0x00000000);
 out_le32(mbase + PECFG_PIM1LAH, 0x00000000);
 out_le32(mbase + PECFG_PIM01SAH, 0xffff0000);
 out_le32(mbase + PECFG_PIM01SAL, 0x00000000);

 out_le32(mbase + PCI_BASE_ADDRESS_0, RES_TO_U32_LOW(res->start));
 out_le32(mbase + PCI_BASE_ADDRESS_1, RES_TO_U32_HIGH(res->start));
/*****************************************/

I have debugged these codes and found that the value of register
PECFGn_BAR0LMPA is
0x80000008 which means the LT field is 00.
But according to the PPC460EX usermanual P618:
LT bit: Type Field:
              Programming access for PECFGn_BAR0L[LT]:
              00 32-bit address memory space (not supported)
             10 64-bit address memory space
              All other values are reserved.
It shows that 32 bit address momory space is not supported for PIM.
The same question happened in the PCI controller (LT field of
PCI0_BAR0L register).

So my question is:
1: For PIM, How to access the SDRAM space using 32bit PCI master?
Using just one address(0x1F800000) or using pci dual address cycle
command.
2: Are current linux kernel PIM settings correct and verified by anybody?

Thanks a lot.

Lonsn

2010/1/1 Tirumala Reddy Marri <tmarri@amcc.com>:
> It should be able to access any region in 32bit mode as long as it is
> smaller than 4GB size. Usually whole SDRAM is mapped to inbound PCI
> memory region.
>
> -----Original Message-----
> From: linuxppc-dev-bounces+tmarri=amcc.com@lists.ozlabs.org
> [mailto:linuxppc-dev-bounces+tmarri=amcc.com@lists.ozlabs.org] On Behalf
> Of Lonsn
> Sent: Thursday, December 31, 2009 12:55 AM
> To: linuxppc-dev@lists.ozlabs.org
> Cc: sr@denx.de
> Subject: How to access PPC460EX SDRAM space from PCI/PCIe.
>
> Hi:
> I'm now using canyonlands board with latest u-boot and linux kernel
> from DENX git.
> A PCIe card is plugged in the PCIeX4 slot. The PCIe card is a PCIe-pci
> bridge(PI7C9X130) plus an Altera fpga.
> The PCIe card act as a PCI master and send data to SDRAM of 460EX
> space (total sdram 512MB, reserve 8M for PCI write data(0x1F800000)).
> Now linux can identify this card, but CPU cann't receive any data from
> PCIe and no PCIe interrupt.
> I know about the PCI card works in 32bit mode and doesn't support
> 64bit address(No pci dual address cycle support).
> Does the PCI card can access PPC460EX sdram space using just 32bit
> physical address(0x1F800000)?
>
> Best regards,
> Lonsn
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>

^ permalink raw reply

* Re: [U-Boot] [PATCH v2 3/3] powerpc: Add support for ram filesystems in FIT uImages
From: Grant Likely @ 2010-01-03  5:08 UTC (permalink / raw)
  To: Peter Tyser; +Cc: u-boot, linux-kbuild, linuxppc-dev
In-Reply-To: <1262221856.29396.104.camel@localhost.localdomain>

Hi Peter,

On Wed, Dec 30, 2009 at 6:10 PM, Peter Tyser <ptyser@xes-inc.com> wrote:
> On Wed, 2009-12-30 at 17:01 -0700, Grant Likely wrote:
>> On Wed, Dec 30, 2009 at 4:39 PM, Peter Tyser <ptyser@xes-inc.com> wrote:
>> > Hi Grant,
>> > I put U-Boot ML on CC.
>>
>> Thinking further, I do actually have another concern, at least with
>> regard to the way the current patch set implements things. =A0Is it
>> expected or even "recommended" that fit images will *always* contain a
>> .dtb image? =A0The current patch only handles the case of a .dtb
>> embedded inside the fit image.
>
> I'm not aware of any recommendations as far as what FIT images should
> contain and there isn't a requirement that it should contain a .dtb
> image - its just a flexible image format.
>
> I believe in the most common FIT usage scenario the image would contain
> both a kernel image and .dtb file though. =A0For example, most people I
> deal with just have boardX and boardY that they want to boot Linux on.
>
> Currently they have to make a "legacy" uImage, manually run the device
> tree compiler with the proper flags to generate a board-specific .dtb
> file,

... or put the .dts files into arch/powerpc/boot/dts and use 'make <board>.=
dtb'

multiple <board>.dtb targets can also be added to CONFIG_EXTRA_TARGETS
so that a plain 'make' will build all needed files.

> transfer the uImage to to boardX, transfer the .dtb file to
> boardX, set some U-Boot variables, then boot to Linux. =A0For boardY they
> would have to redo the steps with the exception of recreating the legacy
> uImage. =A0(To add to the confusion, the format of device trees is a
> somewhat moving target, so .dtb files aren't always compatible with
> different kernel versions.)

The are no guarantees that new dtb files will work with old kernels,
but old dtb files should continue to work with new kernels.  There
have been growing pains in this regard, and there are a number of
examples where this hasn't been the case especially as we were
learning how best to use the device tree.  It is a lot more stable now
than it had been and new cases can be considered a kernel bug.

>> Personally, I don't get any benefit out of the new image format, so I
>> haven't spent any time looking at it. =A0However, I'm concerned about
>> the drift back towards a different image per target when the move over
>> the last 4 years has been towards multiplatform kernel images. =A0I
>> certainly don't want to encourage embedding the device tree blob into
>> the kernel image, and I'm not very interested in merging code to do
>> that into the kernel tree. =A0If someone really needs to do that for
>> their particular target, it is certainly easy enough for them to weld
>> in the .dtb after the fact before transferring the image to the
>> target, but I want that mode to be the exception, not the rule.
>
> I see your point. =A0The main goal of the patch was to introduce FIT imag=
e
> support as its the new, more flexible, "better", standard image format
> for U-Boot going forward. =A0Also, lots people aren't aware of FIT images
> and the cool stuff they can do with them, so what better way to get the
> word out than getting support for FIT images included in the kernel
> proper:)

Define 'better'.  :-)

I do understand your use-case and what problem fit images solve for
you.  However, from a "long term strategy" point of view it is a use
case that I'm not interested in actively supporting for two reasons.
The first is that I think it is in our best interest to encourage the
mind set that the hardware description is separate from the operating
system image, and so they should be stored and updated separately.  I
look at the unexpected ecosystem of distributions that has sprung up
around wireless routers (ie OpenWRT and the like) and Linux cell
phones (ie. Cyanogen Mod) where there is a huge range of hardware.
The userspace can pretty much run on any of them excepting minor
configuration changes.  The embedded space is becoming more PCs in
this regard, and I know that multiplatform is a big deal for some of
the users.  I'm not interested in encouraging any behaviour that will
scuttle deployment of multiplatform kernels.  (That being said, I'm
not going to actively sabotage people who want to put dtb sections
into the kernel images either.  I understand that at times it is
necessary, and there are numerous examples of this already in the
kernel tree; specifically to support firmware that doesn't implement
arch/powerpc's boot interface)

The second is that I no longer think it is a good idea to be building
bootloader specific image fomats if we can help it.  U-Boot is not the
only game bootloaders, especially when talking about ARM or MIPS, and
some folks are even using kexec to go from one kernel to the next.
Having the same image format for all firmware and also kexec is far
more interesting to me than the a new u-boot format.  Some form of
zImage seems to be a more likely format to standardize around than the
FIT format, mostly because kexec and a lot of firmware already
understand zImages, and it doesn't require firmware to link in libfdt.

> I think it would be nice to generate a FIT image that contained the
> kernel + .dtb as it significantly simplifies the process of booting
> Linux for the common case for lots of U-Boot users and showcases one of
> the benefits of the FIT format. =A0If you'd prefer not to have this
> portion of the patch included, I can respin so that the FIT image
> created only contains a kernel image, not a .dtb or initramfs. =A0Or I
> could make targets of fitImage (no dtb included), fitImage.<board> (dtb
> included), and fitImage.initrd.<board> (dtb and initramfs included).
> Either way is fine with me, it'd just be nice to see FIT support get in
> in some form.

I'd be okay (perhaps not happy, but okay) with merging fitImage and
fitImage.initrd targets (no dtb).  I will resist merging fitImage.%
and fitImage.initrd.% targets because I see that very much as a
project specific deployment target and I'm not convinced yet that it
the pattern is right or that it is even needed in the kernel at all.

Cheers,
g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* Re: [PATCH v2 3/3] powerpc: Add support for ram filesystems in FIT uImages
From: Grant Likely @ 2010-01-03  5:10 UTC (permalink / raw)
  To: Wolfgang Denk; +Cc: Peter Tyser, linux-kbuild, linuxppc-dev
In-Reply-To: <20091231224439.AF5353F6D1@gemini.denx.de>

On Thu, Dec 31, 2009 at 3:44 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Grant Likely,
>
> In message <fa686aa40912301502x48785614ya4dd5c71815a7633@mail.gmail.com> =
you wrote:
>>
>> IIRC, uImage.fit.initrd.% should appear before uImage.fit.% in the
>> Makefile so that make behaves more consistently. =A0Speaking of which,
>> the number of '.' in the name is getting rather large. =A0Would you
>> consider using 'fitImage' instead of 'uImage.fit'?
>
> Why chose a different name at all? We could still call it "uImage",
> meaning "U-Boot image" - U-Boot is clever enought o detect
> automatically if we pass it an old style or a fit image.

Simply because arch/powerpc/boot/Makefile needs different target names
to decide which kind of image to build.  I don't care much about the
name, it can always be renamed at install time, but I do care that the
make build targets are sane.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* Re: [PATCH v2 3/3] powerpc: Add support for ram filesystems in FIT uImages
From: Grant Likely @ 2010-01-03  5:13 UTC (permalink / raw)
  To: Wolfgang Denk; +Cc: Peter Tyser, linux-kbuild, linuxppc-dev
In-Reply-To: <20100101104449.6DAC63F6FF@gemini.denx.de>

On Fri, Jan 1, 2010 at 3:44 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Peter,
>
> In message <1262301038.29396.137.camel@localhost.localdomain> you wrote:
>> What do you think about changing the U-Boot documentation to rename
>> those 2 image types to:
>> 1 uImages
>> 2 FIT Images
>
> Let's make this "uImage.old" (or "uImage.legacy" similar) and
> "uImage", then.

I'm not interested in renaming the target name for the current uImage
format.  I think it will cause too much breakage and pain to do so.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* Re: [PATCH v2 3/3] powerpc: Add support for ram filesystems in FIT uImages
From: Grant Likely @ 2010-01-03  5:18 UTC (permalink / raw)
  To: Wolfgang Denk; +Cc: linuxppc-dev, Peter Tyser, linux-kbuild, u-boot
In-Reply-To: <20100101141205.6F0B13F6FF@gemini.denx.de>

On Fri, Jan 1, 2010 at 7:12 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Grant,
>
> In message <fa686aa40912301601s6cd0ec4y85b88976159a36af@mail.gmail.com> y=
ou wrote:
>>
>> Thinking further, I do actually have another concern, at least with
>> regard to the way the current patch set implements things. =A0Is it
>> expected or even "recommended" that fit images will *always* contain a
>> .dtb image? =A0The current patch only handles the case of a .dtb
>> embedded inside the fit image.
>
> I think this can be expected.
>
> Historically, the need to pass the dtb image to the Linux kernel,
> too, was what actually triggered the development of the FIT image
> format, as it turned out that the old image format with it's fixed
> binary header was too inflexible. So bundling the kernel image and
> the device tree blob into one image file is the specific use case
> this image format was created for (which does not mean that other
> usage would be impossible).
>
>> Personally, I don't get any benefit out of the new image format, so I
>> haven't spent any time looking at it. =A0However, I'm concerned about
>
> Assume you want to boot over DHCP or similar, where you can provide
> just a single image file for download. Here it is definitely nice if
> you can bundle the kernel image and the DTB into one image file. We
> were able to extend the old so-called "multi-file" uImage format to
> handle this situation, too, but it was clear that further extensions
> were not really possible.
>
> We consider the old legace uImage format as something we want to move
> away from, and the new FIT image format shall be the new default.
>
>> the drift back towards a different image per target when the move over
>> the last 4 years has been towards multiplatform kernel images. =A0I
>> certainly don't want to encourage embedding the device tree blob into
>> the kernel image, and I'm not very interested in merging code to do
>> that into the kernel tree. =A0If someone really needs to do that for
>> their particular target, it is certainly easy enough for them to weld
>> in the .dtb after the fact before transferring the image to the
>> target, but I want that mode to be the exception, not the rule.
>
> This is specific for particular targets, but for =A0specific =A0modes =A0=
of
> operation, =A0like =A0booting =A0over =A0the network or other szenarios w=
here
> transferring a single image file is essential - another example where
> we often see this request is upgrade procedures for devics, where the
> vendor wants to be able to distribute a single file =A0for =A0his =A0targ=
et
> systems =A0 to =A0avoid =A0customers =A0bricking =A0their =A0devices =A0b=
y =A0chosing
> incompatible combinations.

As I said in a previous email; I understand the need for certain
scenarios, but in the general case it is not the mode that I think
should be encouraged.  I don't want to merge additional targets for
.dtb embedded in the kernel image unless absolutely necessary, and I
want developers to have the mindset that .dtbs should be separate from
the kernel; and should be quasi-stable (or at least more stable than
the kernel itself) because I think that multiplatform is important,
and is going to become more important in the future.

So I don't want to support it by default; but OTOH, I'm not going to
actively prevent embedded .dtb blobs either.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* Re: [PATCH v2 2/3] powerpc: Add support for creating FIT uImages
From: Grant Likely @ 2010-01-03  5:23 UTC (permalink / raw)
  To: Wolfgang Denk; +Cc: Peter Tyser, linux-kbuild, linuxppc-dev
In-Reply-To: <20100101141840.D6C273F6FF@gemini.denx.de>

On Fri, Jan 1, 2010 at 7:18 AM, Wolfgang Denk <wd@denx.de> wrote:
>> Rather than adding new paths to arch/powerpc/boot/wrapper, I would
>> rather see a new script used for generating FIT image that isn't
>> complicated by all the current wrapper cruft. =A0Also, the Makefile rule
>> doesn't need to depend on $(wrapperbits) which means faster build
>> times when only building uImages.
>>
>> Bonus points if you also convert the uImage target to use the new
>> script; but I'm not demanding that you do that yet.
>
> I think if this is handles in a separate script, the legacy uImage
> support must be handled in this new script, too. Otherwise it would be
> too difficult to understanmd the relation of the two image formats.

I'm fine with this.  But if I receive a patch that does one and not
the other, I'll probably still merge it.  :-)

>> Finally, you need to add documentation about the new target to
>> Documentation/powerpc/bootwrapper.txt.
>
> Let's keep in mind that the uImage formats (both the old legacy and
> the new FIT format) are inherently architecture independent. We want
> to use this same mechanism for example on ARM, and on other
> architectures as well.

That's fine, but Documentation/powerpc/bootwrapper.txt happens to also
be the place that documents how the boot image creation on
arch/powerpc works.  I'm happy for the text about uImage and fitImage
to make reference to a different Documentation file or URL.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* Re: [PATCH v2 3/3] powerpc: Add support for ram filesystems in FIT uImages
From: Peter Korsgaard @ 2010-01-03  8:06 UTC (permalink / raw)
  To: Wolfgang Denk; +Cc: Peter Tyser, linux-kbuild, linuxppc-dev
In-Reply-To: <20100101104449.6DAC63F6FF@gemini.denx.de>

>>>>> "Wolfgang" == Wolfgang Denk <wd@denx.de> writes:

 >> What do you think about changing the U-Boot documentation to rename
 >> those 2 image types to:
 >> 1 uImages
 >> 2 FIT Images

 Wolfgang> Let's make this "uImage.old" (or "uImage.legacy" similar) and
 Wolfgang> "uImage", then.

No, that would break stuff for the existing users. The existing format
make/file names shouldn't change.

The FIT images could be called fitimage or uImage.new or something.

-- 
Bye, Peter Korsgaard

^ permalink raw reply

* Re: [PATCH v2 3/3] powerpc: Add support for ram filesystems in FIT uImages
From: Wolfgang Denk @ 2010-01-03  9:50 UTC (permalink / raw)
  To: Peter Korsgaard; +Cc: Peter Tyser, linux-kbuild, linuxppc-dev
In-Reply-To: <87wrzzpq8c.fsf@macbook.be.48ers.dk>

Dear Peter,

In message <87wrzzpq8c.fsf@macbook.be.48ers.dk> you wrote:
>
>  Wolfgang> Let's make this "uImage.old" (or "uImage.legacy" similar) and
>  Wolfgang> "uImage", then.
> 
> No, that would break stuff for the existing users. The existing format
> make/file names shouldn't change.

Well, with this argument you can block all progress and freeze all
development to some ancient state.

When the arch/ppc => arch/powerpc conversionm was done, we had similar
issues: the new "powerpc" uImages would not work as the old "ppc"
uImages dis, because you also needed the device tree blob, or you had
to use the cuImage files. Note that then the decision was made to
support the _new_ version as the default, using the default name,
which is IMO the Right Thing to do.

> The FIT images could be called fitimage or uImage.new or something.

I disagree. We want to make this the new default, so let's use the
default name for this, and continue to support he legacy image format
using another name.

Also, your approach does not scale. Assume we come up with an even
more advanced image format in the future - how should we name it then?
"uImage.newer" ? "uImage.verynew" ? "uImage.new2" ?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
The world is coming to an end -- save your buffers!

^ permalink raw reply

* Re: [U-Boot] [PATCH v2 3/3] powerpc: Add support for ram filesystems in FIT uImages
From: Wolfgang Denk @ 2010-01-03 10:10 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, Peter Tyser, linux-kbuild, u-boot
In-Reply-To: <fa686aa41001022108i92596d6qdf2da0e24c34767e@mail.gmail.com>

Dear Grant Likely,

In message <fa686aa41001022108i92596d6qdf2da0e24c34767e@mail.gmail.com> you wrote:
> 
> > Currently they have to make a "legacy" uImage, manually run the device
> > tree compiler with the proper flags to generate a board-specific .dtb
> > file,
> 
> ... or put the .dts files into arch/powerpc/boot/dts and use 'make <board>.=
> dtb'
> 
> multiple <board>.dtb targets can also be added to CONFIG_EXTRA_TARGETS
> so that a plain 'make' will build all needed files.

Note that the FIT image can also be made to contain a number of DT
blobs, and selection of a "board profile" then can be used to boot the
very sane FIT image file on any of the supported boards - so FIT
images inherently support multibooting.

> > I see your point.  The main goal of the patch was to introduce FIT image
> > support as its the new, more flexible, "better", standard image format
> > for U-Boot going forward.  Also, lots people aren't aware of FIT images
> > and the cool stuff they can do with them, so what better way to get the
> > word out than getting support for FIT images included in the kernel
> > proper:)
>
> Define 'better'.  :-)

FIT images are better than the old uImage format because they:

- allow for strong checksum algorithms like MD5, SHA1, ... (the plain
  CRC32 method is not good enough if you for example want to run
  software in a slot machine in Las Vegas).

- can combine multiple kernel images, device tree blobs and root file
  system images in arbitrary combinations; this allows for example
  for multibooting the same image on different boards by selecting
  the right DTB, for software updates with automatic fall-back, etc.

- can be extended to add new features, images types or whatever in a
  standard way, using a standard technology (device tree support)
  which is already present anyway, i. e. without additional code
  overhead.

> I do understand your use-case and what problem fit images solve for
> you.  However, from a "long term strategy" point of view it is a use
> case that I'm not interested in actively supporting for two reasons.
> The first is that I think it is in our best interest to encourage the
> mind set that the hardware description is separate from the operating
> system image, and so they should be stored and updated separately.  I

How do you boot systems over network, then? Normally you always
download only a single image file.

How do you explain this to system vendors? They have a very reasonable
request to offer only one image file to their customers.

> look at the unexpected ecosystem of distributions that has sprung up
> around wireless routers (ie OpenWRT and the like) and Linux cell
> phones (ie. Cyanogen Mod) where there is a huge range of hardware.
> The userspace can pretty much run on any of them excepting minor
> configuration changes.  The embedded space is becoming more PCs in

Right. So let's be able to provide kernel images that fit intot he
same pattern - that can be used on a range of platforms, for example
by embedding multiple DTBs.

Requesting that we manage a kernel mage plus a collection of DTBs and
that eveybody has to install the only working correctly combination
seems to be a bad idea to me.

> this regard, and I know that multiplatform is a big deal for some of
> the users.  I'm not interested in encouraging any behaviour that will
> scuttle deployment of multiplatform kernels.  (That being said, I'm

You misunderstand. FIT images do not scuttle multiplatform kernels.
Instead they support this much better, as they allow to bundle the
repsective DTBs into one image file.

> not going to actively sabotage people who want to put dtb sections
> into the kernel images either.  I understand that at times it is
> necessary, and there are numerous examples of this already in the
> kernel tree; specifically to support firmware that doesn't implement
> arch/powerpc's boot interface)

Even if the boot loader supports it, it is usually pretty benefical
(especially during development) if you can do this.

> I'd be okay (perhaps not happy, but okay) with merging fitImage and
> fitImage.initrd targets (no dtb).  I will resist merging fitImage.%
> and fitImage.initrd.% targets because I see that very much as a
> project specific deployment target and I'm not convinced yet that it
> the pattern is right or that it is even needed in the kernel at all.

Is this just your personal opinion, or do you think that this is
really what a majority of kernel developers and users are wanting?

Speaking for myself, I have to admit that I don't understand and don't
share this attitude.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
The optimum committee has no members.
                                                   - Norman Augustine

^ permalink raw reply

* Re: [PATCH v2 3/3] powerpc: Add support for ram filesystems in FIT uImages
From: Wolfgang Denk @ 2010-01-03 10:12 UTC (permalink / raw)
  To: Grant Likely; +Cc: Peter Tyser, linux-kbuild, linuxppc-dev
In-Reply-To: <fa686aa41001022113o46bad9d2xf6c026f5fa3e8677@mail.gmail.com>

Dear Grant Likely,

In message <fa686aa41001022113o46bad9d2xf6c026f5fa3e8677@mail.gmail.com> you wrote:
>
> > Let's make this "uImage.old" (or "uImage.legacy" similar) and
> > "uImage", then.
> 
> I'm not interested in renaming the target name for the current uImage
> format.  I think it will cause too much breakage and pain to do so.

We did this before, and the argument then was "progress". Keep in mind
that the old arch/ppc uImage file is much more compatible to the
current arch/powerpc "cuImage" target than to the current "uImage".

I think the same should be done here.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Any sufficiently advanced bug is indistinguishable from a feature.
                                                      - Rich Kulawiec

^ permalink raw reply

* Re: [PATCH v2 3/3] powerpc: Add support for ram filesystems in FIT uImages
From: Wolfgang Denk @ 2010-01-03 10:15 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, Peter Tyser, linux-kbuild, u-boot
In-Reply-To: <fa686aa41001022118w6b41765ld145c646bfca9423@mail.gmail.com>

Dear Grant Likely,

In message <fa686aa41001022118w6b41765ld145c646bfca9423@mail.gmail.com> you wrote:
>
> As I said in a previous email; I understand the need for certain
> scenarios, but in the general case it is not the mode that I think
> should be encouraged.  I don't want to merge additional targets for
> .dtb embedded in the kernel image unless absolutely necessary, and I
> want developers to have the mindset that .dtbs should be separate from
> the kernel; and should be quasi-stable (or at least more stable than
> the kernel itself) because I think that multiplatform is important,
> and is going to become more important in the future.
> 
> So I don't want to support it by default; but OTOH, I'm not going to
> actively prevent embedded .dtb blobs either.

But by blocking the addition of such a make target you do actively
prevent this, don;t you?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
The years of peak mental activity are undoubtedly between the ages of
four and eighteen. At four we know all the questions, at eighteen all
the answers.

^ permalink raw reply

* Re: [PATCH v2 3/3] powerpc: Add support for ram filesystems in FIT uImages
From: Peter Korsgaard @ 2010-01-03 14:27 UTC (permalink / raw)
  To: Wolfgang Denk; +Cc: Peter Tyser, linux-kbuild, linuxppc-dev
In-Reply-To: <20100103095032.8F05AE34A26@gemini.denx.de>

>>>>> "Wolfgang" == Wolfgang Denk <wd@denx.de> writes:

Hi,

 >> No, that would break stuff for the existing users. The existing format
 >> make/file names shouldn't change.

 Wolfgang> Well, with this argument you can block all progress and freeze all
 Wolfgang> development to some ancient state.

We only break backwards compatibility when we have to. There's no
technical reason for renaming the existing make targets.

 >> The FIT images could be called fitimage or uImage.new or something.

 Wolfgang> I disagree. We want to make this the new default, so let's
 Wolfgang> use the default name for this, and continue to support he
 Wolfgang> legacy image format using another name.

 Wolfgang> Also, your approach does not scale. Assume we come up with an
 Wolfgang> even more advanced image format in the future - how should we
 Wolfgang> name it then?  "uImage.newer" ? "uImage.verynew" ?
 Wolfgang> "uImage.new2" ?

Hence fitimage. The next thing will presumably be called something else
again.

-- 
Bye, Peter Korsgaard

^ permalink raw reply

* /dev does not have the devices I have configured with initramfs as my root file system
From: Allan Wang @ 2010-01-03 23:15 UTC (permalink / raw)
  To: linuxppc-dev

[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]

Hello, All,

Happy New Year to everyone! 
I am running ppc440epx board which is similar Sequoia board. I have configured a I2C RTC device. From the console debug message, it is working, but I do not see /dev/rtc0 file. My kernel version is 2.6.30.4. I use initramfs as my root file system. Here is my initramfs.txt:
dir /dev 755 0 0
dir /bin 755 1000 1000
dir /proc 755 0 0
slink /bin/sh busybox 777 0 0
file /bin/busybox /home/awang/proj/linux-2.6.30.4/usr/busybox 755 0 0
dir /sys 755 0 0
dir /usr 755 0 0
dir /usr/bin 755 0 0
dir /usr/sbin 755 0 0
dir /sbin 755 0 0
dir /mnt 755 0 0
dir /etc 755 0 0
dir /var 755 0 0
file /init /home/awang/proj/linux-2.6.30.4/usr/init.sh 755 0 0

Here is my init.sh:

#!/bin/sh

/bin/busybox echo " "
/bin/busybox echo "Hello, I am alive!!!!"
busybox mkdir /mnt/flash
busybox mount -t proc /proc /proc
busybox mount -t sysfs none /sys
busybox mount -t jffs2 mtd2 /mnt/flash

busybox sleep  2
/bin/busybox --install -s
#start a shell
ifconfig eth0 192.168.0.201
/bin/sh


Here is my configurations:

RTC kernel configuration:

CONFIG_RTC_LIB=y
CONFIG_RTC_CLASS=y
CONFIG_RTC_HCTOSYS=y
CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
CONFIG_RTC_DEBUG=y

#
# RTC interfaces
#
CONFIG_RTC_INTF_SYSFS=y
CONFIG_RTC_INTF_PROC=y
CONFIG_RTC_INTF_DEV=y

dts configuration:

 IIC0: i2c@ef600700 {
                                #address-cells = <1>;
                                #size-cells = <0>;
                                compatible = "ibm,iic-440epx", "ibm,iic";
                                reg = <0xef600700 0x00000014>;
                                interrupt-parent = <&UIC0>;
                                interrupts = <0x2 0x4>;
                                rtc@68 {
                                        compatible = "stm,m41t80";
                                        reg = <0x68>;
                                };
                                eeprom0@50 {
                                        compatible = "eeprom";
                                        reg = <0x50>;
                                };
                                eeprom1@54 {
                                        compatible = "eeprom";
                                        reg = <0x54>;
                                };
                        };

The debug messages:

i2c /dev entries driver
rtc-m41t80 1-0068: chip found, driver version 0.05
rtc-m41t80 1-0068: rtc core: registered m41t80 as rtc0
ibm-iic 1ef600700.i2c: using standard (100 kHz) mode
TCP cubic registered
NET: Registered protocol family 17
RPC: Registered udp transport module.
RPC: Registered tcp transport module.
TIPC: Activated (version 1.6.4 compiled Jan  3 2010 12:21:35)
NET: Registered protocol family 30
TIPC: Started in single node mode
rtc-m41t80 1-0068: setting system clock to 2010-01-03 19:43:06 UTC (1262547786)

I have struggled for a few days. Thank you very much.

Allan


      

[-- Attachment #2: Type: text/html, Size: 6826 bytes --]

^ permalink raw reply

* Re: [PATCH v2 3/3] powerpc: Add support for ram filesystems in FIT uImages
From: Peter Tyser @ 2010-01-03 23:52 UTC (permalink / raw)
  To: Wolfgang Denk; +Cc: linuxppc-dev, linux-kbuild
In-Reply-To: <20100101104449.6DAC63F6FF@gemini.denx.de>

Hi Wolfgang,

> The "new" FIT image type should become the default, and old "legacy"
> images should only be generated upon special request (i. e. if some-
> one needs these for compatibility with an old, not yet FIT-aware
> version of the boot loader).

Agreed.

>> What do you think about changing the U-Boot documentation to rename
>> those 2 image types to:
>> 1 uImages
>> 2 FIT Images
>
> Let's make this "uImage.old" (or "uImage.legacy" similar) and
> "uImage", then.

I'm in favor of keeping the old uImage format/naming the same, and 
calling the new image format a FIT Image.  ie no mention of uImage for 
FIT images.

<snip>

>> uImages have to agree with U-Boot's header format defined in the U-Boot
>> source code, so the uImage name does make sense with respect to the
>> "legacy" uImages.
>
> Well, you can read "uImage" as "universal Image", which kind of fits
> the FIT approach :-)

I agree that the FIT image is a type of "universal Image", but I think 
"FIT image" is much more descriptive and accurate than "universal 
Image".  The FIT naming convention is designed to match device tree 
naming, which has lots of meaning.  eg:
   Flattened Device Tree (FDT) -> Flattened Image Tree (FIT)
   device tree source (.dts) -> image tree source (.its)
   device tree blob (.dtb) -> image tree blob (.itb)

>> My vote would be to make the Linux FIT target rule "fitImage" and then
>> update the U-Boot documentation to make obvious the differences between
>> uImages and FIT images.
>
> I think we should not try to support both legacy and FIT images on the
> same level - the idea is clearly that legacy images is the old, to be
> replaced format, while FIT images is the new, to be used as standard
> format.

Agreed.

In this sense I vote for using plain and simple "uImage" for
> the (new) standard format, and marking the old format by some ".old"
> or ".legacy" suffix.

I disagree here.  I don't think calling FIT images "FIT uImages" adds 
much value and it would add confusion as there are now multiple uImage 
formats that a user needs to keep straight.  Keeping the legacy uImage 
naming/format the same, and calling the new FIT images "fitImage" (or 
possibly itbImage to line up with the dtbImage target) would make more 
sense to me.  Is there a compelling reason to keep the uImage word 
connected to FIT images?

Best,
Peter

^ permalink raw reply

* Re: [U-Boot] [PATCH v2 3/3] powerpc: Add support for ram filesystems in FIT uImages
From: Peter Tyser @ 2010-01-04  1:07 UTC (permalink / raw)
  To: Wolfgang Denk; +Cc: u-boot, linuxppc-dev, linux-kbuild
In-Reply-To: <20100103101048.AF4B7E34A26@gemini.denx.de>

<snip>

> Note that the FIT image can also be made to contain a number of DT
> blobs, and selection of a "board profile" then can be used to boot the
> very sane FIT image file on any of the supported boards - so FIT
> images inherently support multibooting.

I agree with Wolfgang.  Additionally, if a FIT image does contain a .dtb 
file, firmware can ignore it and load a different .dtb file.  So for the 
fitImage.<boardname> target you could think of it as including a 
"default/fallback" .dtb file, not a mandatory one.  So the FIT image 
could still be used as a multiplatform image even if it contained dtbs.

>>> I see your point.  The main goal of the patch was to introduce FIT image
>>> support as its the new, more flexible, "better", standard image format
>>> for U-Boot going forward.  Also, lots people aren't aware of FIT images
>>> and the cool stuff they can do with them, so what better way to get the
>>> word out than getting support for FIT images included in the kernel
>>> proper:)
>>
>> Define 'better'.  :-)
>
> FIT images are better than the old uImage format because they:
>
> - allow for strong checksum algorithms like MD5, SHA1, ... (the plain
>    CRC32 method is not good enough if you for example want to run
>    software in a slot machine in Las Vegas).
>
> - can combine multiple kernel images, device tree blobs and root file
>    system images in arbitrary combinations; this allows for example
>    for multibooting the same image on different boards by selecting
>    the right DTB, for software updates with automatic fall-back, etc.
>
> - can be extended to add new features, images types or whatever in a
>    standard way, using a standard technology (device tree support)
>    which is already present anyway, i. e. without additional code
>    overhead.

Other advantages of FIT images that I see day-to-day include:
- Adding meta-data such as timestamps and version information for each 
component of the FIT image.  Its great to be able to easily display the 
contents of a FIT image to determine which kernel versions it contains, etc.

- Embed multiple OS/dtb images in one FIT image.  As a board vendor its 
nice to give a customer 1 image that has example dtb(s) and Linux, 
VxWorks, QNX, diagnostic, etc kernels.  They can boot a number of OSes 
on a number of different boards with 1 image.

- You can extract the images contained in a FIT image using 'dtc' to 
convert the FIT image to a ".dts" file that has the raw image contents. 
  Other image formats are more difficult to extract contents from.

<snip>

>> I'd be okay (perhaps not happy, but okay) with merging fitImage and
>> fitImage.initrd targets (no dtb).  I will resist merging fitImage.%
>> and fitImage.initrd.% targets because I see that very much as a
>> project specific deployment target and I'm not convinced yet that it
>> the pattern is right or that it is even needed in the kernel at all.
>
> Is this just your personal opinion, or do you think that this is
> really what a majority of kernel developers and users are wanting?
>
> Speaking for myself, I have to admit that I don't understand and don't
> share this attitude.

I agree with Wolfgang that many people would find the combined 
kernel/dtb/ramfs useful, but understand Grant's reservations.  Assuming 
I rework the patch to include a "fitImage" (no dtb) target, for the 
users that basic multiplatform images are important to, they could build 
this kernel-only FIT image.  For people such as Wolfgang, Peter, and 
myself who would like to bundle a kernel + dtb(s), we could use the 
fitImage.<boardname> target.  Wouldn't both parties be happy then?  I 
could make the FIT documentation explain the benefits of multiplatform 
images or warn about the combined kernel/dtb FIT images if that made a 
difference too.

Best,
Peter

^ permalink raw reply

* RE: ucc_geth: Fix the wrong the Rx/Tx FIFO size
From: Liu Dave-R63238 @ 2010-01-04  3:42 UTC (permalink / raw)
  To: David Miller; +Cc: linuxppc-dev, avorontsov, netdev
In-Reply-To: <20091209230015.GA13654@oksana.dev.rtsoft.ru>

> On Fri, Nov 27, 2009 at 04:16:43PM +0800, Dave Liu wrote:
> > current the Rx/Tx FIFO size settings cause problem
> > when four UEC ethernets work simultaneously.
> >=20
> > eg: GETH1, UEM-J15, GETH2, UEC-J5 on 8569MDS board
> >=20
> >  $ ifconfig eth0 10.193.20.166
> >  $ ifconfig eth1 10.193.20.167
> >  $ ifconfig eth2 10.193.20.168
> >  then
> >  $ ifconfig eth3 10.193.20.169
> >=20
> > The fourth ethernet will cause all of interface broken,
> > you cann't ping successfully any more.
> > The patch fix this issue for MPC8569 Rev1.0 and Rev2.0
> >=20
> > Signed-off-by: Dave Liu <daveliu@freescale.com>
>=20
> Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>=20
> Thanks Dave! I see that the new values actually match the QEIWRM
> spec (except UTFS... spec recommends 2048, but it also says that
> it might differ for different memory setups, though that's a
> separate issue).
>=20
> And FWIW, I did some quick netperf tests on MPC8360, and the
> patch doesn't cause any problems.

David,

How about this patch? Please consider to pick it up to your tree.

Thanks, Dave

^ permalink raw reply

* Re: ucc_geth: Fix the wrong the Rx/Tx FIFO size
From: David Miller @ 2010-01-04  5:06 UTC (permalink / raw)
  To: r63238; +Cc: linuxppc-dev, avorontsov, netdev
In-Reply-To: <D7CCA83BB0796C49BC0BB53B6AB120898BDC0E@zch01exm21.fsl.freescale.net>

From: "Liu Dave-R63238" <r63238@freescale.com>
Date: Mon, 4 Jan 2010 11:42:30 +0800

> How about this patch? Please consider to pick it up to your tree.

It's not in my inbox nor is it in the patchwork networking
queue, so it must be formally resubmitted in order to be
included.

^ permalink raw reply

* Re: PCI-PCI bridge scanning broken on 460EX
From: Benjamin Herrenschmidt @ 2010-01-04  5:55 UTC (permalink / raw)
  To: Felix Radensky; +Cc: linuxppc-dev, Stefan Roese
In-Reply-To: <4B388D9D.7010404@embedded-sol.com>

On Mon, 2009-12-28 at 12:51 +0200, Felix Radensky wrote:
> Hi,
> 
> I'm running linux-2.6.33-rc2 on Canyonlands board. When PLX 6254 
> transparent PCI-PCI
> bridge is plugged into PCI slot the kernel simply resets the board 
> without printing anything
> to console. Without PLX bridge kernel boots fine.

Sorry for the late reply...

> I've tracked down the problem to the following code in pci_scan_bridge() 
> in drivers/pci/probe.c:
> 
> if (pcibios_assign_all_busses() || broken)
>                 /* Temporarily disable forwarding of the
>                    configuration cycles on all bridges in
>                    this bus segment to avoid possible
>                    conflicts in the second pass between two
>                    bridges programmed with overlapping
>                    bus ranges. */
>                 pci_write_config_dword(dev, PCI_PRIMARY_BUS,
>                                buses & ~0xffffff);
> 
> If test for broken is removed, kernel boots fine, detects the bridge, but
> does not detect the device behind the bridge. The same device plugged
> directly into PCI slot is detected correctly.

So we would have a similar mismatch between the initial setup and the
kernel...  However, I don't quite see yet why the kernel trying to fix
it up breaks things, that will need a bit more debugging here...

Can you give it a quick try with adding something like :

 ppc_pci_add_flags(PPC_PCI_REASSIGN_ALL_BUS);

Near the end of ppc4xx_pci.c ? It looks like another case of reset
not actually resetting bridges (are we not properly doing a fundamental
reset ? Stefan what's your take there ?)

The above will cause busses to be re-assigned which is risky because it
will allow the kernel to assign numbers beyond the limits of what
ppc4xx_pci.c supports (see my comments in the thread you quotes).

The good thing is that we now have a working fixmap infrastructure, so
we could/should just move ppc4xx_pci.c to use that, and just always
re-assign busses.

> To remind you, tests for broken were added by commit 
> a1c19894b786f10c76ac40e93c6b5d70c9b946d2,
> and were intended to solve device detection problem behind PCI-E 
> switches, as discussed in this thread:
> http://lists.ozlabs.org/pipermail/linuxppc-dev/2008-October/063939.html

> PCI: Probing PCI hardware
> pci_bus 0000:00: scanning bus
> pci 0000:00:06.0: found [3388:0020] class 000604 header type 01
> pci 0000:00:06.0: supports D1 D2
> pci 0000:00:06.0: PME# supported from D0 D1 D2 D3hot
> pci 0000:00:06.0: PME# disabled
> pci_bus 0000:00: fixups for bus
> pci 0000:00:06.0: scanning behind bridge, config 000000, pass 0
> pci 0000:00:06.0: bus configuration invalid, reconfiguring

Ok so we hit a P2P bridge whose primary, secondary and subordinate bus
numbers are all 0, which is clearly unconfigured. I think this is the
root complex bridge

> pci 0000:00:06.0: scanning behind bridge, config 000000, pass 1

Now this is when the bus should be reconfigured (pass 1). Sadly the code
doesn't print much debug.

Also from that point, it should renumber things and work... 

> pci_bus 0000:01: scanning bus

Which it does to some extent. It assigned bus number 1 to it afaik so we
now start looking below the RC bridge:

> pci 0000:01:06.0: found [3388:0020] class 000604 header type 01

Hrm... class PCI bridge, vendor 3388 device 0020, is that your PLX ?
It's not the right vendor ID but maybe that's configurable by our OEM or
something...

> pci 0000:01:06.0: supports D1 D2
> pci 0000:01:06.0: PME# supported from D0 D1 D2 D3hot
> pci 0000:01:06.0: PME# disabled
> pci_bus 0000:01: fixups for bus
> pci 0000:00:06.0: PCI bridge to [bus 01-ff]
> pci 0000:00:06.0:   bridge window [io  0x0000-0x0fff]
> pci 0000:00:06.0:   bridge window [mem 0x00000000-0x000fffff]
> pci 0000:00:06.0:   bridge window [mem 0x00000000-0x000fffff 64bit pref]
> pci 0000:01:06.0: scanning behind bridge, config ff0100, pass 0

Allright, that's where it gets interesting. It tries to scan behind the
bridge. It gets something it doesn't like. IE, it gets a secondary bus
number of 1 (what the heck ? I wonder what your firmware does) which
Linux is not happy about and decides to renumber it.

> pci 0000:01:06.0: bus configuration invalid, reconfiguring

Now, that's where Linux should have written 000000 to the register,
which is what you commented out.

> pci 0000:01:06.0: scanning behind bridge, config ff0100, pass 1
> pci_bus 0000:01: bus scan returning with max=01
> pci_bus 0000:00: bus scan returning with max=01

Because of that commenting out, it doesn't see the config as 000000 and
thus doesn't re-assign a bus number in pass 1, so from there you can't
see what's behind the bus.

So we have two things here:

 - It seems like the writing of 000000 to the register in pass 0 is
causing your crash. Can you verify that ? IE. Can you verify that it's
indeed crashing on this specific statement:

pci_write_config_dword(dev, PCI_PRIMARY_BUS,
                               buses & ~0xffffff);

When writing to the bridge, and that this seems to be causing a hard
reboot of the system ?

It might be useful to ask AMCC how that is possible in HW, ie what kind
of signal can be causing that. IE, even if the bridge is causing a PCIe
error, that should not cause a reboot ... right ?

 - You can test a quick hack workaround which consists of changing:

	/* Check if setup is sensible at all */
-	if (!pass &&
-	if (1 &&
	    ((buses & 0xff) != bus->number || ((buses >> 8) & 0xff) <= bus->number)) {
		dev_dbg(&dev->dev, "bus configuration invalid, reconfiguring\n");
		broken = 1;
	}

In -addition- to your commenting out of the broken test. This will cause the
second pass to go through the re-assign code path despite the fact that you
have not written 000000 to the bus numbers.

Cheers,
Ben.

^ permalink raw reply

* Re: [PATCH 2/2] Adding PCI-E MSI support for PowerPC 460SX SOC.
From: Benjamin Herrenschmidt @ 2010-01-04  6:25 UTC (permalink / raw)
  To: tmarri; +Cc: linuxppc-dev, writetomarri, linuxppc-dev
In-Reply-To: <1261639722-12614-1-git-send-email-tmarri@amcc.com>

On Wed, 2009-12-23 at 23:28 -0800, tmarri@amcc.com wrote:
> From: Tirumala Marri <tmarri@amcc.com>
> 
> 
> Signed-off-by: Tirumala Marri <tmarri@amcc.com>
> ---
> 	When 460SX configured as root as a end point E1000(Intell Ethernet card)
> 	was plugged into the one of the PCI-E ports. I was able to run the traffic
> 	with MSI interrupts.

So before I even ack or nack that patch, I need to better understand how
your HW works. I've read the doc of the 460EX twice and still don't
quite get it :-)

So my understanding so far is that when reception of MSIs is enabled,
writes to some magic address in the first 1K of BAR0 are interpreted ad
MSIs. The MSI interrupt value (low 16 bits of the 32-bit store in little
endian) is thus interpreted as an interrupt number and send to the UIC.

Is that correct ?

Now, which UIC ? There are at least 3 in the 460EX for example :-)

Also, UICs have a limited amount of inputs and I don't see many
interrupt sources "reserved" for use as MSIs, can you enlighten me a bit
more on how you get to choose an interrupt source to use as MSI ?

Or is there some translation done ? IE. In the 460EX manual, there seem
to be specific interrupt numbers dedicated to PCI0 MSI 0, 1 2 and 3
spread between UIC0 and UIC1, and a block of 8 interrupts in UIC3
reserved for PCI-E MSIs. Is there a renumbering done in HW here ?

IE. Your table shows for 460EX for example that PCI-E MSI 2 is UIC3
input 26. Do I need thus to program the device to write a "2" in the MSI
message or "26" to hit that interrupt ?

IE. Are you running the input message through a binary decoder that then
spreads into various UIC input lines ?

Now some comments:

> diff --git a/arch/powerpc/boot/dts/redwood.dts b/arch/powerpc/boot/dts/redwood.dts
> index 81636c0..6c20faf 100644
> --- a/arch/powerpc/boot/dts/redwood.dts
> +++ b/arch/powerpc/boot/dts/redwood.dts
> @@ -357,6 +357,21 @@
>  				0x0 0x0 0x0 0x3 &UIC3 0xa 0x4 /* swizzled int C */
>  				0x0 0x0 0x0 0x4 &UIC3 0xb 0x4 /* swizzled int D */>;
>  		};
> +  		MSI: ppc4xx-msi@400300000 {
> +  			compatible = "amcc,ppc4xx-460sx-msi", "ppc4xx-msi";
> +  			reg = < 0x4 0x00300000 0x100
> +  				0x4 0x00300000 0x100>;
> +  			sdr-base = <0x3B0>;
> +  			interrupts =<0 1 2 3>;
> +  			interrupt-parent = <&MSI>;
> +  			#interrupt-cells = <1>;
> +  			#address-cells = <0>;
> +  			#size-cells = <0>;
> +  			interrupt-map = <0 &UIC0 0xC 1
> +  				1 &UIC0 0x0D 1
> +  				2 &UIC0 0x0E 1
> +  				3 &UIC0 0x0F 1>;
> +  		};

Wow ! That's the mother of all device-tree hacks :-) So you are using
the "interrupts" property of the MSI node to represent the MSI
interrupts you hand out, and you make it its own interrupt-parent, using
an interrupt-map in itself to convert them into UIC interrupts :-)
Sneaky... Hell, it will work so why not ?

> +static struct ppc4xx_msi *ppc4xx_msi;
> +
> +struct ppc4xx_msi_feature {
> +	u32 ppc4xx_pic_ip;
> +	u32 msiir_offset;
> +};
> +
> +static int ppc4xx_msi_init_allocator(struct ppc4xx_msi *msi_data)
> +{
> +	int rc;
> +
> +	rc = msi_bitmap_alloc(&msi_data->bitmap, NR_MSI_IRQS,
> +				msi_data->irqhost->of_node);
> +	if (rc)
> +		return rc;
> +	rc = msi_bitmap_reserve_dt_hwirqs(&msi_data->bitmap);
> +	if (rc < 0) {
> +		msi_bitmap_free(&msi_data->bitmap);
> +		return rc;
> +	}
> +	return 0;
> +}

Ok so here I start having problems :-) First you allocate a bitmap for
the MSIs of a fixed number, despite the fact that there seem to be a
different number of MSIs supported depending on what part/bridge you are
using. Then, you try to call msi_bitmap_reserve_dt_hwirqs()... why
that ? This is meant to be used when you don't know what interrupt
numbers are reserved for use by MSIs in your system, it's a bit fishy to
be honest, we use it on powermac mostly :-)

> +static void ppc4xx_msi_cascade(unsigned int irq, struct irq_desc *desc)
> +{
> +	unsigned int cascade_irq;
> +	struct ppc4xx_msi *msi_data = ppc4xx_msi;
> +	int msir_index = -1;
> +
> +	raw_spin_lock(&desc->lock);
> +	if (desc->chip->mask_ack) {
> +		desc->chip->mask_ack(irq);
> +	} else {
> +		desc->chip->mask(irq);
> +		desc->chip->ack(irq);
> +	}
> +
> +	if (unlikely(desc->status & IRQ_INPROGRESS))
> +		goto unlock;
> +
> +	msir_index = (int)desc->handler_data;
> +
> +	if (msir_index >= NR_MSI_IRQS)
> +		cascade_irq = NO_IRQ;
> +
> +	desc->status |= IRQ_INPROGRESS;
> +
> +	cascade_irq = irq_linear_revmap(msi_data->irqhost, msir_index);
> +	if (cascade_irq != NO_IRQ)
> +		generic_handle_irq(cascade_irq);
> +	desc->status &= ~IRQ_INPROGRESS;
> +
> +	if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
> +		desc->chip->unmask(irq);
> +unlock:
> +	raw_spin_unlock(&desc->lock);
> +}

Now, that's really a weird idea. Why are you making it a cascaded
interrupt controller ? That is just gratuitous overhead. You could have
the MSI handling directly hand out the appropriate UIC interrupts and
have them call directly into the driver. The above adds significant
overhead to MSI processing.

> +static void ppc4xx_compose_msi_msg(struct pci_dev *pdev, int hwirq,
> +					struct msi_msg *msg)
> +{
> +	struct ppc4xx_msi *msi_data = ppc4xx_msi;
> +
> +	msg->address_lo = msi_data->msi_addr_lo;
> +	msg->address_hi = msi_data->msi_addr_hi;
> +	msg->data = hwirq;
> +}
> +
> +int ppc4xx_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> +{
> +	struct msi_desc *entry;
> +	int rc, hwirq;
> +	unsigned int virq;
> +	struct msi_msg msg;
> +	struct ppc4xx_msi *msi_data = ppc4xx_msi;
> +
> +
> +	list_for_each_entry(entry, &dev->msi_list, list) {
> +		hwirq = msi_bitmap_alloc_hwirqs(&msi_data->bitmap, 1);
> +		if (hwirq < 0) {
> +			rc = hwirq;
> +			dev_err(&dev->dev, "%s: fail allocating msi\
> +					interrupt\n",	__func__);
> +			goto out_free;
> +		}
> +
> +		pr_debug(KERN_INFO"mis is %p\n", msi_data->irqhost);
> +		virq = irq_create_mapping(msi_data->irqhost, hwirq);
> +		if (virq == NO_IRQ) {
> +			dev_err(&dev->dev, "%s: fail mapping irq\n", __func__);
> +			rc = -ENOSPC;
> +			goto out_free;
> +		}
> +
> +		set_irq_msi(virq, entry);
> +		ppc4xx_compose_msi_msg(dev, hwirq, &msg);
> +		write_msi_msg(virq, &msg);
> +	}
> +
> +	return 0;
> +out_free:
> +	return rc;
> +}

Similar here. You create this whole layer of secondary interrupt host
while you could just hand out existing UIC interrupts no ?
 
 .../...

> +
> +static int __devinit ppc4xx_msi_probe(struct of_device *dev,
> +					const struct of_device_id *match)
> +{
> +	struct ppc4xx_msi *msi;
> +	struct resource res, rmsi;
> +	int i, count;
> +	int rc;
> +	int virt_msir;
> +	const u32 *p;
> +	const u32 *sdr_base;
> +	u32 *msi_virt = NULL;
> +	dma_addr_t msi_phys;
> +
> +
> +	msi = kzalloc(sizeof(struct ppc4xx_msi), GFP_KERNEL);
> +	if (!msi) {
> +		dev_err(&dev->dev, "No memory for MSI structure\n");
> +		rc = -ENOMEM;
> +		goto error_out;
> +	}
> +
> +	msi->irqhost = irq_alloc_host(dev->node, IRQ_HOST_MAP_LINEAR,
> +				      NR_MSI_IRQS, &ppc4xx_msi_host_ops, 0);
> +	if (msi->irqhost == NULL) {
> +		dev_err(&dev->dev, "No memory for MSI irqhost\n");
> +		rc = -ENOMEM;
> +		goto error_out;
> +	}

Same comment, ie, why a separate irq host ?

> +
> +	/* Get MSI ranges */
> +	rc = of_address_to_resource(dev->node, 0, &rmsi);
> +	if (rc) {
> +		dev_err(&dev->dev, "%s resource error!\n",
> +				dev->node->full_name);
> +		goto error_out;
> +	}
> +
> +
> +	/* Get the MSI reg base */
> +	rc = of_address_to_resource(dev->node, 1, &res);
> +	if (rc) {
> +		dev_err(&dev->dev, "%s resource error!\n",
> +				dev->node->full_name);
> +		goto error_out;
> +	}
> +	/* Get the sdr-base */
> +	sdr_base = (u32 *)of_get_property(dev->node, "sdr-base", NULL);
> +	if (sdr_base == NULL) {
> +		dev_err(&dev->dev, "%s resource error!\n",
> +				dev->node->full_name);
> +		goto error_out;
> +	}
> +	msi->sdr_base = *sdr_base;
> +	mtdcri(SDR0, msi->sdr_base, res.start >> 32);
> +	mtdcri(SDR0, msi->sdr_base + 1, res.start & 0xFFFFFFFF);
> +	msi->msi_regs = ioremap(res.start, res.end - res.start + 1);
> +	if (!msi->msi_regs) {
> +		dev_err(&dev->dev, "ioremap problem failed\n");
> +		goto error_out;
> +	}
> +	/* MSI region always mapped in 4GB region*/
> +	msi->msi_addr_hi = 0x0;
> +	msi_virt = dma_alloc_coherent(&dev->dev, 64, &msi_phys,
> +			GFP_KERNEL);
> +	if (msi_virt == NULL) {
> +		dev_err(&dev->dev, "No memory for MSI mem space\n");
> +		rc = -ENOMEM;
> +		goto error_out;
> +	}
> +	msi->msi_addr_lo = (u32)msi_phys;

Now, what is the above for ? Are the MSI writes done by the device
actually hitting memory as well ? I would have expected them to be
filtered by the bridge on the way up, are they not ?

I suppose it gives you a cheap way to obtain a bit of unused address
space...

> +	/* Progam the Interrupt handler Termination addr registers */
> +	out_be32(msi->msi_regs + PEIH_TERMADH, msi->msi_addr_hi);
> +	out_be32(msi->msi_regs + PEIH_TERMADL, msi->msi_addr_lo);
> +
> +	/* Program MSI Expected data and Mask bits */
> +	out_be32(msi->msi_regs + PEIH_MSIED, MSI_DATA_PATTERN);
> +	out_be32(msi->msi_regs + PEIH_MSIMK, MSI_DATA_PATTERN);

Can you tell me a bit more about what the above is about ?

Also, it looks like your code would work for other SoCs like 460EX
etc... by just updating the .dts right ? Care to update a few more
boards so I can test on my canyonlands ? :-)

Cheers,
Ben.

^ permalink raw reply

* Re: [PATCH-V2] mpc8xxx_gpio: add interrupt support
From: Peter Korsgaard @ 2010-01-04  8:20 UTC (permalink / raw)
  To: avorontsov; +Cc: linuxppc-dev
In-Reply-To: <1261398375-32545-1-git-send-email-jacmet@sunsite.dk>

>>>>> "Peter" == Peter Korsgaard <jacmet@sunsite.dk> writes:

 Peter> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>

Comments?

With the added documentation I believe it's ready to go..

 Peter> ---
 Peter>  .../powerpc/dts-bindings/fsl/8xxx_gpio.txt         |   22 +++-
 Peter>  arch/powerpc/sysdev/mpc8xxx_gpio.c                 |  147 ++++++++++++++++++++
 Peter>  2 files changed, 168 insertions(+), 1 deletions(-)

-- 
Bye, Peter Korsgaard

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox