Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
On 08/15/2012 04:22 AM, Jia Hongtao-B38951 wrote: > > >> -Original Message- >> From: Wood Scott-B07421 >> Sent: Saturday, August 11, 2012 12:00 AM >> To: Jia Hongtao-B38951 >> Cc: Gala Kumar-B11780; Wood Scott-B07421; Li Yang-R58472; linuxppc- >> d...@lists.ozlabs.org >> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie >> initialization code >> >> On 08/10/2012 03:47 AM, Jia Hongtao-B38951 wrote: >>> >>> >>>> -Original Message- >>>> From: Gala Kumar-B11780 >>>> Sent: Thursday, August 09, 2012 3:04 AM >>>> To: Wood Scott-B07421 >>>> Cc: Jia Hongtao-B38951; Wood Scott-B07421; Li Yang-R58472; linuxppc- >>>> d...@lists.ozlabs.org >>>> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie >>>> initialization code >>>> >>>>>>>>>>> As I explained before, this has to be done globally, not from >>>>>>>>>>> the probe function, so we can assign a default primary bus if >>>>>>>>>>> there >>>>>>> isn't any ISA. >>>>>>>>>>> There are bugs in the Linux PPC PCI code relating to not >>>>>>>>>>> having any primary bus. >>>>>>>>>>> >>>>>>>>>>> -Scott >>>>>>>>>> >>>>>>>>>> In my way of searching ISA you can also assign a default >>>>>>>>>> primary bus in board specific files. >>>>>>>>> >>>>>>>>> That was meant for when the board file had an alternate way of >>>>>>>>> searching for the primary bus (e.g. look for i8259), not as a >>>>>>>>> replacement for the mechanism that guarantees there's a primary >> bus. >>>>>>>>> >>>>>>>>> You are causing a regression in the qemu_e500.c platform. >>>>>>>> >>>>>>>> Can we fix the qemu device tree to address the problem if we do >>>>>>>> make it a rule to use the ISA node to indicate the primary bus? >>>>>>> >>>>>>> No. There is no ISA, and we're not going to lie and say there is. >>>>>> >>>>>> But we can assign a default primary for qemu. >>>>> >>>>> Not in the device tree. What other mechanism do you propose? And >>>>> why do you want to fix it only for QEMU and not other boards, where >>>>> things happen to work but not as designed? >>>>> >>>>> Kumar, can you speak up here as maintainer so we can stop going back >>>>> and forth endlessly? >>>> >>>> I'd rather we stick with the code that works for this purpose at this >>>> point. That would be Scott's current upstream code. Lets get the >>>> other aspects of this patchset closed (SWIOTLB, conversion to >>>> platform driver, PM, etc.). The primary bus code Scott wrote does >>>> NOT need to change at this point. >>>> >>>> - k >>> >>> >>> I just submitted a new version of PCI patch in which I improve the >> primary part. >>> The reasons I want to change the way of primary assignment listed below: >>> >>> 1. This approach is functionally equivalent to the Scott's code. In my >>> approach there might be no primary assigned but it fixed by "quick fix" >> introduced by Ben. >>> Please refer to this link: >>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-June/098586.html >> >> You might want to get Ben's input as to whether he actually wants to see >> that "quick fix" applied. >> >>> 2. Scott's and my way could not handle the situation that "the primary >>> is not the first PCI bus detected". I found that only ge_imp3a got >>> this problem so I fixed it by adding ISA node to its device tree. By >>> adding this I think the solution is logically completed. >> >> How did my approach not handle this case? As I said, ge_imp3a platform >> code needs to set fsl_pci_primary manually before PCI init runs. >> >> Adding a node to the device tree is not the answer, since that will break >> compatibility with old device trees. >> > > I assume that kernel image and dtb image are from the same tree. That's a bad assumption. Device trees get forked off for custom boards, modified by firmware, generated by firmware, etc. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
> -Original Message- > From: Wood Scott-B07421 > Sent: Saturday, August 11, 2012 12:00 AM > To: Jia Hongtao-B38951 > Cc: Gala Kumar-B11780; Wood Scott-B07421; Li Yang-R58472; linuxppc- > d...@lists.ozlabs.org > Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie > initialization code > > On 08/10/2012 03:47 AM, Jia Hongtao-B38951 wrote: > > > > > >> -Original Message- > >> From: Gala Kumar-B11780 > >> Sent: Thursday, August 09, 2012 3:04 AM > >> To: Wood Scott-B07421 > >> Cc: Jia Hongtao-B38951; Wood Scott-B07421; Li Yang-R58472; linuxppc- > >> d...@lists.ozlabs.org > >> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie > >> initialization code > >> > >>>>>>>>> As I explained before, this has to be done globally, not from > >>>>>>>>> the probe function, so we can assign a default primary bus if > >>>>>>>>> there > >>>>> isn't any ISA. > >>>>>>>>> There are bugs in the Linux PPC PCI code relating to not > >>>>>>>>> having any primary bus. > >>>>>>>>> > >>>>>>>>> -Scott > >>>>>>>> > >>>>>>>> In my way of searching ISA you can also assign a default > >>>>>>>> primary bus in board specific files. > >>>>>>> > >>>>>>> That was meant for when the board file had an alternate way of > >>>>>>> searching for the primary bus (e.g. look for i8259), not as a > >>>>>>> replacement for the mechanism that guarantees there's a primary > bus. > >>>>>>> > >>>>>>> You are causing a regression in the qemu_e500.c platform. > >>>>>> > >>>>>> Can we fix the qemu device tree to address the problem if we do > >>>>>> make it a rule to use the ISA node to indicate the primary bus? > >>>>> > >>>>> No. There is no ISA, and we're not going to lie and say there is. > >>>> > >>>> But we can assign a default primary for qemu. > >>> > >>> Not in the device tree. What other mechanism do you propose? And > >>> why do you want to fix it only for QEMU and not other boards, where > >>> things happen to work but not as designed? > >>> > >>> Kumar, can you speak up here as maintainer so we can stop going back > >>> and forth endlessly? > >> > >> I'd rather we stick with the code that works for this purpose at this > >> point. That would be Scott's current upstream code. Lets get the > >> other aspects of this patchset closed (SWIOTLB, conversion to > >> platform driver, PM, etc.). The primary bus code Scott wrote does > >> NOT need to change at this point. > >> > >> - k > > > > > > I just submitted a new version of PCI patch in which I improve the > primary part. > > The reasons I want to change the way of primary assignment listed below: > > > > 1. This approach is functionally equivalent to the Scott's code. In my > > approach there might be no primary assigned but it fixed by "quick fix" > introduced by Ben. > > Please refer to this link: > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-June/098586.html > > You might want to get Ben's input as to whether he actually wants to see > that "quick fix" applied. > > > 2. Scott's and my way could not handle the situation that "the primary > > is not the first PCI bus detected". I found that only ge_imp3a got > > this problem so I fixed it by adding ISA node to its device tree. By > > adding this I think the solution is logically completed. > > How did my approach not handle this case? As I said, ge_imp3a platform > code needs to set fsl_pci_primary manually before PCI init runs. > > Adding a node to the device tree is not the answer, since that will break > compatibility with old device trees. > I assume that kernel image and dtb image are from the same tree. -Hongtao. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
On 08/10/2012 03:47 AM, Jia Hongtao-B38951 wrote: > > >> -Original Message- >> From: Gala Kumar-B11780 >> Sent: Thursday, August 09, 2012 3:04 AM >> To: Wood Scott-B07421 >> Cc: Jia Hongtao-B38951; Wood Scott-B07421; Li Yang-R58472; linuxppc- >> d...@lists.ozlabs.org >> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie >> initialization code >> >>>>>>>>> As I explained before, this has to be done globally, not from >>>>>>>>> the probe function, so we can assign a default primary bus if >>>>>>>>> there >>>>> isn't any ISA. >>>>>>>>> There are bugs in the Linux PPC PCI code relating to not having >>>>>>>>> any primary bus. >>>>>>>>> >>>>>>>>> -Scott >>>>>>>> >>>>>>>> In my way of searching ISA you can also assign a default primary >>>>>>>> bus in board specific files. >>>>>>> >>>>>>> That was meant for when the board file had an alternate way of >>>>>>> searching for the primary bus (e.g. look for i8259), not as a >>>>>>> replacement for the mechanism that guarantees there's a primary bus. >>>>>>> >>>>>>> You are causing a regression in the qemu_e500.c platform. >>>>>> >>>>>> Can we fix the qemu device tree to address the problem if we do >>>>>> make it a rule to use the ISA node to indicate the primary bus? >>>>> >>>>> No. There is no ISA, and we're not going to lie and say there is. >>>> >>>> But we can assign a default primary for qemu. >>> >>> Not in the device tree. What other mechanism do you propose? And why >>> do you want to fix it only for QEMU and not other boards, where things >>> happen to work but not as designed? >>> >>> Kumar, can you speak up here as maintainer so we can stop going back >>> and forth endlessly? >> >> I'd rather we stick with the code that works for this purpose at this >> point. That would be Scott's current upstream code. Lets get the other >> aspects of this patchset closed (SWIOTLB, conversion to platform driver, >> PM, etc.). The primary bus code Scott wrote does NOT need to change at >> this point. >> >> - k > > > I just submitted a new version of PCI patch in which I improve the primary > part. > The reasons I want to change the way of primary assignment listed below: > > 1. This approach is functionally equivalent to the Scott's code. In my > approach > there might be no primary assigned but it fixed by "quick fix" introduced by > Ben. > Please refer to this link: > https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-June/098586.html You might want to get Ben's input as to whether he actually wants to see that "quick fix" applied. > 2. Scott's and my way could not handle the situation that "the primary is not > the > first PCI bus detected". I found that only ge_imp3a got this problem so I > fixed it > by adding ISA node to its device tree. By adding this I think the solution is > logically completed. How did my approach not handle this case? As I said, ge_imp3a platform code needs to set fsl_pci_primary manually before PCI init runs. Adding a node to the device tree is not the answer, since that will break compatibility with old device trees. > 3. The key advantage of my way is better unified for platform driver. If I use > the Scott's way I have to make an routine and called in all boards code. Only until all boards are converted, and this is *not* different with your approach. > The goal > of my PCI patch is unifying all PCI initialization code and obviously primary > determination is part of PCI code. > > 4. The other advantage is efficiency. All my search for ISA node is just under > PCI node instead of all device tree. We do so many searches over the full device tree during boot that this is meaningless. Do you have benchmarks to show that device tree iteration is a significant contributor to boot time? -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
> -Original Message- > From: Gala Kumar-B11780 > Sent: Thursday, August 09, 2012 3:04 AM > To: Wood Scott-B07421 > Cc: Jia Hongtao-B38951; Wood Scott-B07421; Li Yang-R58472; linuxppc- > d...@lists.ozlabs.org > Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie > initialization code > > >>>>>>> As I explained before, this has to be done globally, not from > >>>>>>> the probe function, so we can assign a default primary bus if > >>>>>>> there > >>> isn't any ISA. > >>>>>>> There are bugs in the Linux PPC PCI code relating to not having > >>>>>>> any primary bus. > >>>>>>> > >>>>>>> -Scott > >>>>>> > >>>>>> In my way of searching ISA you can also assign a default primary > >>>>>> bus in board specific files. > >>>>> > >>>>> That was meant for when the board file had an alternate way of > >>>>> searching for the primary bus (e.g. look for i8259), not as a > >>>>> replacement for the mechanism that guarantees there's a primary bus. > >>>>> > >>>>> You are causing a regression in the qemu_e500.c platform. > >>>> > >>>> Can we fix the qemu device tree to address the problem if we do > >>>> make it a rule to use the ISA node to indicate the primary bus? > >>> > >>> No. There is no ISA, and we're not going to lie and say there is. > >> > >> But we can assign a default primary for qemu. > > > > Not in the device tree. What other mechanism do you propose? And why > > do you want to fix it only for QEMU and not other boards, where things > > happen to work but not as designed? > > > > Kumar, can you speak up here as maintainer so we can stop going back > > and forth endlessly? > > I'd rather we stick with the code that works for this purpose at this > point. That would be Scott's current upstream code. Lets get the other > aspects of this patchset closed (SWIOTLB, conversion to platform driver, > PM, etc.). The primary bus code Scott wrote does NOT need to change at > this point. > > - k I just submitted a new version of PCI patch in which I improve the primary part. The reasons I want to change the way of primary assignment listed below: 1. This approach is functionally equivalent to the Scott's code. In my approach there might be no primary assigned but it fixed by "quick fix" introduced by Ben. Please refer to this link: https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-June/098586.html 2. Scott's and my way could not handle the situation that "the primary is not the first PCI bus detected". I found that only ge_imp3a got this problem so I fixed it by adding ISA node to its device tree. By adding this I think the solution is logically completed. 3. The key advantage of my way is better unified for platform driver. If I use the Scott's way I have to make an routine and called in all boards code. The goal of my PCI patch is unifying all PCI initialization code and obviously primary determination is part of PCI code. 4. The other advantage is efficiency. All my search for ISA node is just under PCI node instead of all device tree. - Hongtao. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
On 08/08/2012 10:48 PM, Jia Hongtao-B38951 wrote: > > >> -Original Message- >> From: Wood Scott-B07421 >> Sent: Thursday, August 09, 2012 12:02 AM >> To: Jia Hongtao-B38951 >> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; >> ga...@kernel.crashing.org; Li Yang-R58472 >> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie >> initialization code >> >> On 08/08/2012 04:39 AM, Jia Hongtao-B38951 wrote: >>> >>> >>>> -Original Message- >>>> From: Wood Scott-B07421 >>>> Sent: Tuesday, August 07, 2012 11:29 PM >>>> To: Jia Hongtao-B38951 >>>> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; >>>> ga...@kernel.crashing.org; Li Yang-R58472 >>>> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie >>>> initialization code >>>> >>>> On 08/07/2012 03:09 AM, Jia Hongtao-B38951 wrote: >>>>> I am really not sure that all boards need primary bus. Could you >>>>> give me the link of discussion about primary that you mentioned? >>>> >>>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-June/098586.html >>>> >>>> -Scott >>> >>> >>> It seems in qemu isa_io_base must be non-zero. >> >> In all cases. It just shows up worse under QEMU because of a different >> issue. >> >>> If there is no isa bridge should isa_io_base be non-zero for other >> boards? >> >> Yes, until the bugs are fixed. >> >>> If not maybe we should fix qemu bug. >> >> If you want to try to make QEMU accept I/O BARs with address zero, go >> ahead, but you don't get to assume that someone else will do it, we still >> need to be compatible with older QEMUs (this bug is not so severe that >> compatibility is unreasonable), and it still doesn't address the fact >> that things are not functioning as designed. IIRC there are some real >> hardware PCI cards that don't like getting an address of zero either. >> >>> Or "quick fix" in the link is a workaround. >> >> I think that "quick fix" may have problems if there is a primary bus but >> it's not the first one detected. In any case, any fix or workaround has >> to happen before you make changes that rely on it. >> >> -Scott > > If there is no primary assigned and accidently the primary is not the > first one this "quick fix" may have problem. But this -accident- only happened > in ge_imp3a board if I didn't miss other boards. How is it an accident? It's a perfectly legitimate situation. > So if there is no primary assigned but the primary is the first bus detected > this "quick fix" is right. That means the "quick fix" is the equivalent > substitution for "arbitrarily designate one as primary". It's not equivalent because I didn't try to convert the ge_imp3a board, and if I did I would have added special code to the ge_imp3a board to set the fsl_pci_primary before calling fsl_pci_init(). -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
On 08/08/2012 10:48 PM, Jia Hongtao-B38951 wrote: > > >> -Original Message- >> From: Wood Scott-B07421 >> Sent: Wednesday, August 08, 2012 11:58 PM >> To: Jia Hongtao-B38951 >> Cc: Wood Scott-B07421; Li Yang-R58472; linuxppc-dev@lists.ozlabs.org; >> Gala Kumar-B11780 >> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie >> initialization code >> >> On 08/08/2012 04:03 AM, Jia Hongtao-B38951 wrote: >>> >>> >>>> -Original Message- >>>> From: Wood Scott-B07421 >>>> Sent: Tuesday, August 07, 2012 11:25 PM >>>> To: Li Yang-R58472 >>>> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; Li Yang-R58472; >>>> Jia >>>> Hongtao-B38951 >>>> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie >>>> initialization code >>>> >>>> On 08/06/2012 11:20 PM, Li Yang wrote: >>>>> On Mon, Aug 6, 2012 at 11:09 PM, Scott Wood >>>>> >>>> wrote: >>>>>> On 08/05/2012 10:07 PM, Jia Hongtao-B38951 wrote: >>>>>>> >>>>>>> >>>>>>>> -Original Message- >>>>>>>> From: Wood Scott-B07421 >>>>>>>> Sent: Saturday, August 04, 2012 12:28 AM >>>>>>>> To: Jia Hongtao-B38951 >>>>>>>> Cc: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org; Li >>>>>>>> Yang- R58472; Wood Scott-B07421 >>>>>>>> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie >>>>>>>> initialization code >>>>>>>> >>>>>>>> As I explained before, this has to be done globally, not from the >>>>>>>> probe function, so we can assign a default primary bus if there >>>> isn't any ISA. >>>>>>>> There are bugs in the Linux PPC PCI code relating to not having >>>>>>>> any primary bus. >>>>>>>> >>>>>>>> -Scott >>>>>>> >>>>>>> In my way of searching ISA you can also assign a default primary >>>>>>> bus in board specific files. >>>>>> >>>>>> That was meant for when the board file had an alternate way of >>>>>> searching for the primary bus (e.g. look for i8259), not as a >>>>>> replacement for the mechanism that guarantees there's a primary bus. >>>>>> >>>>>> You are causing a regression in the qemu_e500.c platform. >>>>> >>>>> Can we fix the qemu device tree to address the problem if we do make >>>>> it a rule to use the ISA node to indicate the primary bus? >>>> >>>> No. There is no ISA, and we're not going to lie and say there is. >>> >>> But we can assign a default primary for qemu. >> >> Not in the device tree. What other mechanism do you propose? And why do >> you want to fix it only for QEMU and not other boards, where things >> happen to work but not as designed? >> >> Kumar, can you speak up here as maintainer so we can stop going back and >> forth endlessly? >> >>>> I really don't understand what the problem is with leaving the >>>> primary detection code as global. Either fix the bugs so we don't >>>> need a primary, or accept some "impurity" in the workaround. >>>> >>>> -Scott >>> >>> Global detection for primary is ok but we think our way is deeper >> unified. >> >> So my way works and "is ok", and your way doesn't work but is >> theoretically cleaner. > > Sorry, I meant global detection is ok but I didn't mean that your logic > is ok. The concern is in some cases there is no isa node in device tree > and the primary is not the first bus. Your logic assigned a wrong primary > there. Is that a problem? Take ge_imp3a as an example. > > So maybe we should fix this exceptional board. Boards like that are why the code first checks for whether fsl_pci_primary is NULL. Board code can set fsl_pci_primary based on other criteria before calling fsl_pci_init(). This is why we need to allow for a gradual conversion, so boards like that can get personal attention by someone who can test the change. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
> -Original Message- > From: Wood Scott-B07421 > Sent: Thursday, August 09, 2012 12:02 AM > To: Jia Hongtao-B38951 > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; > ga...@kernel.crashing.org; Li Yang-R58472 > Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie > initialization code > > On 08/08/2012 04:39 AM, Jia Hongtao-B38951 wrote: > > > > > >> -Original Message- > >> From: Wood Scott-B07421 > >> Sent: Tuesday, August 07, 2012 11:29 PM > >> To: Jia Hongtao-B38951 > >> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; > >> ga...@kernel.crashing.org; Li Yang-R58472 > >> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie > >> initialization code > >> > >> On 08/07/2012 03:09 AM, Jia Hongtao-B38951 wrote: > >>> I am really not sure that all boards need primary bus. Could you > >>> give me the link of discussion about primary that you mentioned? > >> > >> https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-June/098586.html > >> > >> -Scott > > > > > > It seems in qemu isa_io_base must be non-zero. > > In all cases. It just shows up worse under QEMU because of a different > issue. > > > If there is no isa bridge should isa_io_base be non-zero for other > boards? > > Yes, until the bugs are fixed. > > > If not maybe we should fix qemu bug. > > If you want to try to make QEMU accept I/O BARs with address zero, go > ahead, but you don't get to assume that someone else will do it, we still > need to be compatible with older QEMUs (this bug is not so severe that > compatibility is unreasonable), and it still doesn't address the fact > that things are not functioning as designed. IIRC there are some real > hardware PCI cards that don't like getting an address of zero either. > > > Or "quick fix" in the link is a workaround. > > I think that "quick fix" may have problems if there is a primary bus but > it's not the first one detected. In any case, any fix or workaround has > to happen before you make changes that rely on it. > > -Scott If there is no primary assigned and accidently the primary is not the first one this "quick fix" may have problem. But this -accident- only happened in ge_imp3a board if I didn't miss other boards. So if there is no primary assigned but the primary is the first bus detected this "quick fix" is right. That means the "quick fix" is the equivalent substitution for "arbitrarily designate one as primary". Maybe we can use the "quick fix" and fix ge_imp3a as an exceptional case. -Hongtao. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
> -Original Message- > From: Wood Scott-B07421 > Sent: Wednesday, August 08, 2012 11:58 PM > To: Jia Hongtao-B38951 > Cc: Wood Scott-B07421; Li Yang-R58472; linuxppc-dev@lists.ozlabs.org; > Gala Kumar-B11780 > Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie > initialization code > > On 08/08/2012 04:03 AM, Jia Hongtao-B38951 wrote: > > > > > >> -Original Message- > >> From: Wood Scott-B07421 > >> Sent: Tuesday, August 07, 2012 11:25 PM > >> To: Li Yang-R58472 > >> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; Li Yang-R58472; > >> Jia > >> Hongtao-B38951 > >> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie > >> initialization code > >> > >> On 08/06/2012 11:20 PM, Li Yang wrote: > >>> On Mon, Aug 6, 2012 at 11:09 PM, Scott Wood > >>> > >> wrote: > >>>> On 08/05/2012 10:07 PM, Jia Hongtao-B38951 wrote: > >>>>> > >>>>> > >>>>>> -Original Message- > >>>>>> From: Wood Scott-B07421 > >>>>>> Sent: Saturday, August 04, 2012 12:28 AM > >>>>>> To: Jia Hongtao-B38951 > >>>>>> Cc: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org; Li > >>>>>> Yang- R58472; Wood Scott-B07421 > >>>>>> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie > >>>>>> initialization code > >>>>>> > >>>>>> As I explained before, this has to be done globally, not from the > >>>>>> probe function, so we can assign a default primary bus if there > >> isn't any ISA. > >>>>>> There are bugs in the Linux PPC PCI code relating to not having > >>>>>> any primary bus. > >>>>>> > >>>>>> -Scott > >>>>> > >>>>> In my way of searching ISA you can also assign a default primary > >>>>> bus in board specific files. > >>>> > >>>> That was meant for when the board file had an alternate way of > >>>> searching for the primary bus (e.g. look for i8259), not as a > >>>> replacement for the mechanism that guarantees there's a primary bus. > >>>> > >>>> You are causing a regression in the qemu_e500.c platform. > >>> > >>> Can we fix the qemu device tree to address the problem if we do make > >>> it a rule to use the ISA node to indicate the primary bus? > >> > >> No. There is no ISA, and we're not going to lie and say there is. > > > > But we can assign a default primary for qemu. > > Not in the device tree. What other mechanism do you propose? And why do > you want to fix it only for QEMU and not other boards, where things > happen to work but not as designed? > > Kumar, can you speak up here as maintainer so we can stop going back and > forth endlessly? > > >> I really don't understand what the problem is with leaving the > >> primary detection code as global. Either fix the bugs so we don't > >> need a primary, or accept some "impurity" in the workaround. > >> > >> -Scott > > > > Global detection for primary is ok but we think our way is deeper > unified. > > So my way works and "is ok", and your way doesn't work but is > theoretically cleaner. Sorry, I meant global detection is ok but I didn't mean that your logic is ok. The concern is in some cases there is no isa node in device tree and the primary is not the first bus. Your logic assigned a wrong primary there. Is that a problem? Take ge_imp3a as an example. So maybe we should fix this exceptional board. > > > Is there any problem to fix the bugs? > > If you want to fix them, go ahead. You don't get to rely on the bugs > beign fixed until after they're actually fixed. > > > I really don't understand why we have to need a primary bus. > > Did you read Ben's e-mail that I posted a link to? > > -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
>>> As I explained before, this has to be done globally, not from the >>> probe function, so we can assign a default primary bus if there >>> isn't any ISA. >>> There are bugs in the Linux PPC PCI code relating to not having >>> any primary bus. >>> >>> -Scott >> >> In my way of searching ISA you can also assign a default primary bus >> in board specific files. > > That was meant for when the board file had an alternate way of > searching for the primary bus (e.g. look for i8259), not as a > replacement for the mechanism that guarantees there's a primary bus. > > You are causing a regression in the qemu_e500.c platform. Can we fix the qemu device tree to address the problem if we do make it a rule to use the ISA node to indicate the primary bus? >>> >>> No. There is no ISA, and we're not going to lie and say there is. >> >> But we can assign a default primary for qemu. > > Not in the device tree. What other mechanism do you propose? And why > do you want to fix it only for QEMU and not other boards, where things > happen to work but not as designed? > > Kumar, can you speak up here as maintainer so we can stop going back and > forth endlessly? I'd rather we stick with the code that works for this purpose at this point. That would be Scott's current upstream code. Lets get the other aspects of this patchset closed (SWIOTLB, conversion to platform driver, PM, etc.). The primary bus code Scott wrote does NOT need to change at this point. - k ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
On 08/08/2012 04:39 AM, Jia Hongtao-B38951 wrote: > > >> -Original Message- >> From: Wood Scott-B07421 >> Sent: Tuesday, August 07, 2012 11:29 PM >> To: Jia Hongtao-B38951 >> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; >> ga...@kernel.crashing.org; Li Yang-R58472 >> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie >> initialization code >> >> On 08/07/2012 03:09 AM, Jia Hongtao-B38951 wrote: >>> I am really not sure that all boards need primary bus. Could you give >>> me the link of discussion about primary that you mentioned? >> >> https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-June/098586.html >> >> -Scott > > > It seems in qemu isa_io_base must be non-zero. In all cases. It just shows up worse under QEMU because of a different issue. > If there is no isa bridge should isa_io_base be non-zero for other boards? Yes, until the bugs are fixed. > If not maybe we should fix qemu bug. If you want to try to make QEMU accept I/O BARs with address zero, go ahead, but you don't get to assume that someone else will do it, we still need to be compatible with older QEMUs (this bug is not so severe that compatibility is unreasonable), and it still doesn't address the fact that things are not functioning as designed. IIRC there are some real hardware PCI cards that don't like getting an address of zero either. > Or "quick fix" in the link is a workaround. I think that "quick fix" may have problems if there is a primary bus but it's not the first one detected. In any case, any fix or workaround has to happen before you make changes that rely on it. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
On 08/08/2012 04:03 AM, Jia Hongtao-B38951 wrote: > > >> -Original Message- >> From: Wood Scott-B07421 >> Sent: Tuesday, August 07, 2012 11:25 PM >> To: Li Yang-R58472 >> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; Li Yang-R58472; Jia >> Hongtao-B38951 >> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie >> initialization code >> >> On 08/06/2012 11:20 PM, Li Yang wrote: >>> On Mon, Aug 6, 2012 at 11:09 PM, Scott Wood >> wrote: >>>> On 08/05/2012 10:07 PM, Jia Hongtao-B38951 wrote: >>>>> >>>>> >>>>>> -Original Message- >>>>>> From: Wood Scott-B07421 >>>>>> Sent: Saturday, August 04, 2012 12:28 AM >>>>>> To: Jia Hongtao-B38951 >>>>>> Cc: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org; Li >>>>>> Yang- R58472; Wood Scott-B07421 >>>>>> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie >>>>>> initialization code >>>>>> >>>>>> As I explained before, this has to be done globally, not from the >>>>>> probe function, so we can assign a default primary bus if there >> isn't any ISA. >>>>>> There are bugs in the Linux PPC PCI code relating to not having >>>>>> any primary bus. >>>>>> >>>>>> -Scott >>>>> >>>>> In my way of searching ISA you can also assign a default primary bus >>>>> in board specific files. >>>> >>>> That was meant for when the board file had an alternate way of >>>> searching for the primary bus (e.g. look for i8259), not as a >>>> replacement for the mechanism that guarantees there's a primary bus. >>>> >>>> You are causing a regression in the qemu_e500.c platform. >>> >>> Can we fix the qemu device tree to address the problem if we do make >>> it a rule to use the ISA node to indicate the primary bus? >> >> No. There is no ISA, and we're not going to lie and say there is. > > But we can assign a default primary for qemu. Not in the device tree. What other mechanism do you propose? And why do you want to fix it only for QEMU and not other boards, where things happen to work but not as designed? Kumar, can you speak up here as maintainer so we can stop going back and forth endlessly? >> I really don't understand what the problem is with leaving the primary >> detection code as global. Either fix the bugs so we don't need a primary, >> or accept some "impurity" in the workaround. >> >> -Scott > > Global detection for primary is ok but we think our way is deeper unified. So my way works and "is ok", and your way doesn't work but is theoretically cleaner. > Is there any problem to fix the bugs? If you want to fix them, go ahead. You don't get to rely on the bugs beign fixed until after they're actually fixed. > I really don't understand why we have to need a primary bus. Did you read Ben's e-mail that I posted a link to? -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
> -Original Message- > From: Wood Scott-B07421 > Sent: Tuesday, August 07, 2012 11:29 PM > To: Jia Hongtao-B38951 > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; > ga...@kernel.crashing.org; Li Yang-R58472 > Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie > initialization code > > On 08/07/2012 03:09 AM, Jia Hongtao-B38951 wrote: > > I am really not sure that all boards need primary bus. Could you give > > me the link of discussion about primary that you mentioned? > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-June/098586.html > > -Scott It seems in qemu isa_io_base must be non-zero. If there is no isa bridge should isa_io_base be non-zero for other boards? If not maybe we should fix qemu bug. Or "quick fix" in the link is a workaround. -Hongtao. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
> -Original Message- > From: Wood Scott-B07421 > Sent: Tuesday, August 07, 2012 11:25 PM > To: Li Yang-R58472 > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; Li Yang-R58472; Jia > Hongtao-B38951 > Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie > initialization code > > On 08/06/2012 11:20 PM, Li Yang wrote: > > On Mon, Aug 6, 2012 at 11:09 PM, Scott Wood > wrote: > >> On 08/05/2012 10:07 PM, Jia Hongtao-B38951 wrote: > >>> > >>> > >>>> -Original Message- > >>>> From: Wood Scott-B07421 > >>>> Sent: Saturday, August 04, 2012 12:28 AM > >>>> To: Jia Hongtao-B38951 > >>>> Cc: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org; Li > >>>> Yang- R58472; Wood Scott-B07421 > >>>> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie > >>>> initialization code > >>>> > >>>> As I explained before, this has to be done globally, not from the > >>>> probe function, so we can assign a default primary bus if there > isn't any ISA. > >>>> There are bugs in the Linux PPC PCI code relating to not having > >>>> any primary bus. > >>>> > >>>> -Scott > >>> > >>> In my way of searching ISA you can also assign a default primary bus > >>> in board specific files. > >> > >> That was meant for when the board file had an alternate way of > >> searching for the primary bus (e.g. look for i8259), not as a > >> replacement for the mechanism that guarantees there's a primary bus. > >> > >> You are causing a regression in the qemu_e500.c platform. > > > > Can we fix the qemu device tree to address the problem if we do make > > it a rule to use the ISA node to indicate the primary bus? > > No. There is no ISA, and we're not going to lie and say there is. But we can assign a default primary for qemu. > > I really don't understand what the problem is with leaving the primary > detection code as global. Either fix the bugs so we don't need a primary, > or accept some "impurity" in the workaround. > > -Scott Global detection for primary is ok but we think our way is deeper unified. Is there any problem to fix the bugs? I really don't understand why we have to need a primary bus. -Hongtao. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
On 08/07/2012 03:09 AM, Jia Hongtao-B38951 wrote: > I am really not sure that all boards need primary bus. Could you give me > the link of discussion about primary that you mentioned? https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-June/098586.html -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
On 08/06/2012 11:20 PM, Li Yang wrote: > On Mon, Aug 6, 2012 at 11:09 PM, Scott Wood wrote: >> On 08/05/2012 10:07 PM, Jia Hongtao-B38951 wrote: >>> >>> >>>> -Original Message- >>>> From: Wood Scott-B07421 >>>> Sent: Saturday, August 04, 2012 12:28 AM >>>> To: Jia Hongtao-B38951 >>>> Cc: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org; Li Yang- >>>> R58472; Wood Scott-B07421 >>>> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie >>>> initialization code >>>> >>>> As I explained before, this has to be done globally, not from the probe >>>> function, so we can assign a default primary bus if there isn't any ISA. >>>> There are bugs in the Linux PPC PCI code relating to not having any >>>> primary bus. >>>> >>>> -Scott >>> >>> In my way of searching ISA you can also assign a default primary bus in >>> board >>> specific files. >> >> That was meant for when the board file had an alternate way of searching >> for the primary bus (e.g. look for i8259), not as a replacement for the >> mechanism that guarantees there's a primary bus. >> >> You are causing a regression in the qemu_e500.c platform. > > Can we fix the qemu device tree to address the problem if we do make > it a rule to use the ISA node to indicate the primary bus? No. There is no ISA, and we're not going to lie and say there is. I really don't understand what the problem is with leaving the primary detection code as global. Either fix the bugs so we don't need a primary, or accept some "impurity" in the workaround. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
> -Original Message-
> From: Wood Scott-B07421
> Sent: Monday, August 06, 2012 11:10 PM
> To: Jia Hongtao-B38951
> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> ga...@kernel.crashing.org; Li Yang-R58472
> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie
> initialization code
>
> On 08/05/2012 10:07 PM, Jia Hongtao-B38951 wrote:
> >
> >
> >> -Original Message-
> >> From: Wood Scott-B07421
> >> Sent: Saturday, August 04, 2012 12:28 AM
> >> To: Jia Hongtao-B38951
> >> Cc: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org; Li
> >> Yang- R58472; Wood Scott-B07421
> >> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie
> >> initialization code
> >>
> >> On 08/03/2012 05:14 AM, Jia Hongtao wrote:
> >>> -void __devinit fsl_pci_init(void)
> >>> +/* Checkout if PCI contains ISA node */ static int
> >>> +of_pci_has_isa(struct device_node *pci_node) {
> >>> + struct device_node *np;
> >>> + int ret = 0;
> >>> +
> >>> + if (!pci_node)
> >>> + return 0;
> >>> +
> >>> + read_lock(&devtree_lock);
> >>> + np = pci_node->allnext;
> >>> +
> >>> + /* Only scan the children of PCI node */
> >>> + for (; np != pci_node->sibling; np = np->allnext) {
> >>> + if (np->type && (of_node_cmp(np->type, "isa") == 0)
> >>> + && of_node_get(np)) {
> >>> + ret = 1;
> >>> + break;
> >>> + }
> >>> + }
> >>> +
> >>> + of_node_put(pci_node);
> >>> + read_unlock(&devtree_lock);
> >>> +
> >>> + return ret;
> >>> +}
> >>
> >> Why do you keep insisting on substituting your ISA search code here?
> >> What advantages does it have over the code that is already there? It
> >> unnecessarily digs into the internals of the tree representation.
> >>
> >
> > I want ISA search is done from probe.
>
> Too bad. You're breaking the case where there's no ISA node.
>
> > Also this way is more efficient due
> > to we just search the children of PCI.
>
> It is not more efficient, because you're doing the search for every PCIe
> bus rather than once. Not that it matters in this context.
>
> >>> +
> >>> +static int __devinit fsl_pci_probe(struct platform_device *pdev)
> >>> {
> >>> int ret;
> >>> - struct device_node *node;
> >>> struct pci_controller *hose;
> >>> - dma_addr_t max = 0x;
> >>> + int is_primary = 0;
> >>>
> >>> - /* Callers can specify the primary bus using other means. */
> >>> if (!fsl_pci_primary) {
> >>> - /* If a PCI host bridge contains an ISA node, it's primary.
> >> */
> >>> - node = of_find_node_by_type(NULL, "isa");
> >>> - while ((fsl_pci_primary = of_get_parent(node))) {
> >>> - of_node_put(node);
> >>> - node = fsl_pci_primary;
> >>> -
> >>> - if (of_match_node(pci_ids, node))
> >>> - break;
> >>> - }
> >>> + is_primary = of_pci_has_isa(pdev->dev.of_node);
> >>> + if (is_primary)
> >>> + fsl_pci_primary = pdev->dev.of_node;
> >>> }
> >>
> >> As I explained before, this has to be done globally, not from the
> >> probe function, so we can assign a default primary bus if there isn't
> any ISA.
> >> There are bugs in the Linux PPC PCI code relating to not having any
> >> primary bus.
> >>
> >> -Scott
> >
> > In my way of searching ISA you can also assign a default primary bus
> > in board specific files.
>
> That was meant for when the board file had an alternate way of searching
> for the primary bus (e.g. look for i8259), not as a replacement for the
> mechanism that guarantees there's a primary bus.
>
> You are causing a regression in the qemu_e500.c platform.
>
> > I read your code and found that if there is no ISA node you will
> > assign the first PCI bus scanned as primary. It's not all right. Take
> > ge_imp3a as an
> > example: The second PCI bus (9000) is primary not the first one.
>
> Does that
Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
On Mon, Aug 6, 2012 at 11:09 PM, Scott Wood wrote:
> On 08/05/2012 10:07 PM, Jia Hongtao-B38951 wrote:
>>
>>
>>> -Original Message-
>>> From: Wood Scott-B07421
>>> Sent: Saturday, August 04, 2012 12:28 AM
>>> To: Jia Hongtao-B38951
>>> Cc: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org; Li Yang-
>>> R58472; Wood Scott-B07421
>>> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie
>>> initialization code
>>>
>>> On 08/03/2012 05:14 AM, Jia Hongtao wrote:
>>>> -void __devinit fsl_pci_init(void)
>>>> +/* Checkout if PCI contains ISA node */
>>>> +static int of_pci_has_isa(struct device_node *pci_node)
>>>> +{
>>>> + struct device_node *np;
>>>> + int ret = 0;
>>>> +
>>>> + if (!pci_node)
>>>> + return 0;
>>>> +
>>>> + read_lock(&devtree_lock);
>>>> + np = pci_node->allnext;
>>>> +
>>>> + /* Only scan the children of PCI node */
>>>> + for (; np != pci_node->sibling; np = np->allnext) {
>>>> + if (np->type && (of_node_cmp(np->type, "isa") == 0)
>>>> + && of_node_get(np)) {
>>>> + ret = 1;
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>>>> + of_node_put(pci_node);
>>>> + read_unlock(&devtree_lock);
>>>> +
>>>> + return ret;
>>>> +}
>>>
>>> Why do you keep insisting on substituting your ISA search code here?
>>> What advantages does it have over the code that is already there? It
>>> unnecessarily digs into the internals of the tree representation.
>>>
>>
>> I want ISA search is done from probe.
>
> Too bad. You're breaking the case where there's no ISA node.
>
We can also take care of special cases with our approach if needed.
But it's not correct to assume the first PCI controller is the primary
one if there is no ISA node. Your approach is still a band-aid to me.
We can come back to this issue when we do find a proper solution.
>> Also this way is more efficient due
>> to we just search the children of PCI.
>
> It is not more efficient, because you're doing the search for every PCIe
> bus rather than once. Not that it matters in this context.
We end up scanning at most a few PCI nodes instead of the whole device
tree for the primary.
>
>>>> +
>>>> +static int __devinit fsl_pci_probe(struct platform_device *pdev)
>>>> {
>>>> int ret;
>>>> - struct device_node *node;
>>>> struct pci_controller *hose;
>>>> - dma_addr_t max = 0x;
>>>> + int is_primary = 0;
>>>>
>>>> - /* Callers can specify the primary bus using other means. */
>>>> if (!fsl_pci_primary) {
>>>> - /* If a PCI host bridge contains an ISA node, it's primary.
>>> */
>>>> - node = of_find_node_by_type(NULL, "isa");
>>>> - while ((fsl_pci_primary = of_get_parent(node))) {
>>>> - of_node_put(node);
>>>> - node = fsl_pci_primary;
>>>> -
>>>> - if (of_match_node(pci_ids, node))
>>>> - break;
>>>> - }
>>>> + is_primary = of_pci_has_isa(pdev->dev.of_node);
>>>> + if (is_primary)
>>>> + fsl_pci_primary = pdev->dev.of_node;
>>>> }
>>>
>>> As I explained before, this has to be done globally, not from the probe
>>> function, so we can assign a default primary bus if there isn't any ISA.
>>> There are bugs in the Linux PPC PCI code relating to not having any
>>> primary bus.
>>>
>>> -Scott
>>
>> In my way of searching ISA you can also assign a default primary bus in board
>> specific files.
>
> That was meant for when the board file had an alternate way of searching
> for the primary bus (e.g. look for i8259), not as a replacement for the
> mechanism that guarantees there's a primary bus.
>
> You are causing a regression in the qemu_e500.c platform.
Can we fix the qemu device tree to address the problem if we do make
it a rule to use the ISA node to indicate the primary bus?
- Leo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
On 08/05/2012 10:07 PM, Jia Hongtao-B38951 wrote:
>
>
>> -Original Message-
>> From: Wood Scott-B07421
>> Sent: Saturday, August 04, 2012 12:28 AM
>> To: Jia Hongtao-B38951
>> Cc: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org; Li Yang-
>> R58472; Wood Scott-B07421
>> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie
>> initialization code
>>
>> On 08/03/2012 05:14 AM, Jia Hongtao wrote:
>>> -void __devinit fsl_pci_init(void)
>>> +/* Checkout if PCI contains ISA node */
>>> +static int of_pci_has_isa(struct device_node *pci_node)
>>> +{
>>> + struct device_node *np;
>>> + int ret = 0;
>>> +
>>> + if (!pci_node)
>>> + return 0;
>>> +
>>> + read_lock(&devtree_lock);
>>> + np = pci_node->allnext;
>>> +
>>> + /* Only scan the children of PCI node */
>>> + for (; np != pci_node->sibling; np = np->allnext) {
>>> + if (np->type && (of_node_cmp(np->type, "isa") == 0)
>>> + && of_node_get(np)) {
>>> + ret = 1;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + of_node_put(pci_node);
>>> + read_unlock(&devtree_lock);
>>> +
>>> + return ret;
>>> +}
>>
>> Why do you keep insisting on substituting your ISA search code here?
>> What advantages does it have over the code that is already there? It
>> unnecessarily digs into the internals of the tree representation.
>>
>
> I want ISA search is done from probe.
Too bad. You're breaking the case where there's no ISA node.
> Also this way is more efficient due
> to we just search the children of PCI.
It is not more efficient, because you're doing the search for every PCIe
bus rather than once. Not that it matters in this context.
>>> +
>>> +static int __devinit fsl_pci_probe(struct platform_device *pdev)
>>> {
>>> int ret;
>>> - struct device_node *node;
>>> struct pci_controller *hose;
>>> - dma_addr_t max = 0x;
>>> + int is_primary = 0;
>>>
>>> - /* Callers can specify the primary bus using other means. */
>>> if (!fsl_pci_primary) {
>>> - /* If a PCI host bridge contains an ISA node, it's primary.
>> */
>>> - node = of_find_node_by_type(NULL, "isa");
>>> - while ((fsl_pci_primary = of_get_parent(node))) {
>>> - of_node_put(node);
>>> - node = fsl_pci_primary;
>>> -
>>> - if (of_match_node(pci_ids, node))
>>> - break;
>>> - }
>>> + is_primary = of_pci_has_isa(pdev->dev.of_node);
>>> + if (is_primary)
>>> + fsl_pci_primary = pdev->dev.of_node;
>>> }
>>
>> As I explained before, this has to be done globally, not from the probe
>> function, so we can assign a default primary bus if there isn't any ISA.
>> There are bugs in the Linux PPC PCI code relating to not having any
>> primary bus.
>>
>> -Scott
>
> In my way of searching ISA you can also assign a default primary bus in board
> specific files.
That was meant for when the board file had an alternate way of searching
for the primary bus (e.g. look for i8259), not as a replacement for the
mechanism that guarantees there's a primary bus.
You are causing a regression in the qemu_e500.c platform.
> I read your code and found that if there is no ISA node you will assign the
> first PCI bus scanned as primary. It's not all right. Take ge_imp3a as an
> example: The second PCI bus (9000) is primary not the first one.
Does that board have ISA on it, that isn't described by the device tree?
If so, before converting to the new init mechanism, the board code will
need to set fsl_pci_primary based on its own knowledge of where that ISA
is. If it doesn't have ISA, it doesn't matter which one we designate as
primary.
> I doubt that there are bugs if no primary assigned.
Yeah, I just implemented the fallback for fun. Come on.
It was recently discussed on this list. PCI under QEMU did not work
without it.
> Like mpc85xx_rdb assigned
> no primary at all. Some other boards has no primary ether like p1022ds,
> p1021mds,
> p1010rdb, p1023rds, all corenet boards (p2041_rdb, p3041_ds, p4080_ds,
> p5020_ds,
> p5040_ds). If no primary is a bug then all these boards above are not
> correctly
> setting up.
Those boards are not being correctly set up. On real hardware things
work by chance, but not under QEMU.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
> -Original Message-
> From: Wood Scott-B07421
> Sent: Saturday, August 04, 2012 12:28 AM
> To: Jia Hongtao-B38951
> Cc: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org; Li Yang-
> R58472; Wood Scott-B07421
> Subject: Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie
> initialization code
>
> On 08/03/2012 05:14 AM, Jia Hongtao wrote:
> > -void __devinit fsl_pci_init(void)
> > +/* Checkout if PCI contains ISA node */
> > +static int of_pci_has_isa(struct device_node *pci_node)
> > +{
> > + struct device_node *np;
> > + int ret = 0;
> > +
> > + if (!pci_node)
> > + return 0;
> > +
> > + read_lock(&devtree_lock);
> > + np = pci_node->allnext;
> > +
> > + /* Only scan the children of PCI node */
> > + for (; np != pci_node->sibling; np = np->allnext) {
> > + if (np->type && (of_node_cmp(np->type, "isa") == 0)
> > + && of_node_get(np)) {
> > + ret = 1;
> > + break;
> > + }
> > + }
> > +
> > + of_node_put(pci_node);
> > + read_unlock(&devtree_lock);
> > +
> > + return ret;
> > +}
>
> Why do you keep insisting on substituting your ISA search code here?
> What advantages does it have over the code that is already there? It
> unnecessarily digs into the internals of the tree representation.
>
I want ISA search is done from probe. Also this way is more efficient due
to we just search the children of PCI.
> > +
> > +static int __devinit fsl_pci_probe(struct platform_device *pdev)
> > {
> > int ret;
> > - struct device_node *node;
> > struct pci_controller *hose;
> > - dma_addr_t max = 0x;
> > + int is_primary = 0;
> >
> > - /* Callers can specify the primary bus using other means. */
> > if (!fsl_pci_primary) {
> > - /* If a PCI host bridge contains an ISA node, it's primary.
> */
> > - node = of_find_node_by_type(NULL, "isa");
> > - while ((fsl_pci_primary = of_get_parent(node))) {
> > - of_node_put(node);
> > - node = fsl_pci_primary;
> > -
> > - if (of_match_node(pci_ids, node))
> > - break;
> > - }
> > + is_primary = of_pci_has_isa(pdev->dev.of_node);
> > + if (is_primary)
> > + fsl_pci_primary = pdev->dev.of_node;
> > }
>
> As I explained before, this has to be done globally, not from the probe
> function, so we can assign a default primary bus if there isn't any ISA.
> There are bugs in the Linux PPC PCI code relating to not having any
> primary bus.
>
> -Scott
In my way of searching ISA you can also assign a default primary bus in board
specific files.
I read your code and found that if there is no ISA node you will assign the
first PCI bus scanned as primary. It's not all right. Take ge_imp3a as an
example: The second PCI bus (9000) is primary not the first one.
I doubt that there are bugs if no primary assigned. Like mpc85xx_rdb assigned
no primary at all. Some other boards has no primary ether like p1022ds,
p1021mds,
p1010rdb, p1023rds, all corenet boards (p2041_rdb, p3041_ds, p4080_ds, p5020_ds,
p5040_ds). If no primary is a bug then all these boards above are not correctly
setting up.
-Hongtao.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
On 08/03/2012 05:14 AM, Jia Hongtao wrote:
> -void __devinit fsl_pci_init(void)
> +/* Checkout if PCI contains ISA node */
> +static int of_pci_has_isa(struct device_node *pci_node)
> +{
> + struct device_node *np;
> + int ret = 0;
> +
> + if (!pci_node)
> + return 0;
> +
> + read_lock(&devtree_lock);
> + np = pci_node->allnext;
> +
> + /* Only scan the children of PCI node */
> + for (; np != pci_node->sibling; np = np->allnext) {
> + if (np->type && (of_node_cmp(np->type, "isa") == 0)
> + && of_node_get(np)) {
> + ret = 1;
> + break;
> + }
> + }
> +
> + of_node_put(pci_node);
> + read_unlock(&devtree_lock);
> +
> + return ret;
> +}
Why do you keep insisting on substituting your ISA search code here?
What advantages does it have over the code that is already there? It
unnecessarily digs into the internals of the tree representation.
> +
> +static int __devinit fsl_pci_probe(struct platform_device *pdev)
> {
> int ret;
> - struct device_node *node;
> struct pci_controller *hose;
> - dma_addr_t max = 0x;
> + int is_primary = 0;
>
> - /* Callers can specify the primary bus using other means. */
> if (!fsl_pci_primary) {
> - /* If a PCI host bridge contains an ISA node, it's primary. */
> - node = of_find_node_by_type(NULL, "isa");
> - while ((fsl_pci_primary = of_get_parent(node))) {
> - of_node_put(node);
> - node = fsl_pci_primary;
> -
> - if (of_match_node(pci_ids, node))
> - break;
> - }
> + is_primary = of_pci_has_isa(pdev->dev.of_node);
> + if (is_primary)
> + fsl_pci_primary = pdev->dev.of_node;
> }
As I explained before, this has to be done globally, not from the probe
function, so we can assign a default primary bus if there isn't any ISA.
There are bugs in the Linux PPC PCI code relating to not having any
primary bus.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
