[V3,1/5] powerpc/fsl-pci: Unify pci/pcie initialization code

We unified the Freescale pci/pcie initialization by changing the fsl_pci
to a platform driver. In previous PCI code architecture the initialization
routine is called at board_setup_arch stage. Now the initialization is done
in probe function which is architectural better. Also It's convenient for
adding PM support for PCI controller in later patch.

One issue introduced by this architecture is the timing of swiotlb_init.
During PCI initialization the need of swiotlb is determined and this should
be done before swiotlb_init. So a new function to determine swiotlb by
parsing pci ranges is made. This function is called at board_setup_arch
stage which is earlier than swiotlb_init.

Signed-off-by: Jia Hongtao <B38951@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
Changed for V3:
- Rebase the patch set on the latest tree
- merge PCI unify and swiotlb patch into one

 arch/powerpc/sysdev/fsl_pci.c |  155 ++++++++++++++++++++++++++++++++---------
 arch/powerpc/sysdev/fsl_pci.h |    9 +--
 2 files changed, 125 insertions(+), 39 deletions(-)

On Jul 26, 2012, at 7:30 AM, Jia Hongtao wrote:

> We unified the Freescale pci/pcie initialization by changing the fsl_pci
> to a platform driver. In previous PCI code architecture the initialization
> routine is called at board_setup_arch stage. Now the initialization is done
> in probe function which is architectural better. Also It's convenient for
> adding PM support for PCI controller in later patch.
> 
> One issue introduced by this architecture is the timing of swiotlb_init.
> During PCI initialization the need of swiotlb is determined and this should
> be done before swiotlb_init. So a new function to determine swiotlb by
> parsing pci ranges is made. This function is called at board_setup_arch
> stage which is earlier than swiotlb_init.
> 
> Signed-off-by: Jia Hongtao <B38951@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
> Changed for V3:
> - Rebase the patch set on the latest tree
> - merge PCI unify and swiotlb patch into one
> 
> arch/powerpc/sysdev/fsl_pci.c |  155 ++++++++++++++++++++++++++++++++---------
> arch/powerpc/sysdev/fsl_pci.h |    9 +--
> 2 files changed, 125 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
> index a7b2a60..5228b6b 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -823,56 +823,143 @@ static const struct of_device_id pci_ids[] = {
> 	{},
> };
> 
> -struct device_node *fsl_pci_primary;
> -
> -void __devinit fsl_pci_init(void)
> +#ifdef CONFIG_SWIOTLB
> +void pci_determine_swiotlb(void)
> {
> +	const u32 *ranges;
> +	int rlen;
> +	int pna;
> +	int np;
> 	struct device_node *node;
> -	struct pci_controller *hose;
> -	dma_addr_t max = 0xffffffff;
> -
> -	/* 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;
> -		}
> -	}
> +	int memno;
> +	u32 pci_space;
> +	unsigned long long pci_addr, cpu_addr, pci_next, cpu_next, size;
> +	unsigned long long pci_addr_lo = ULLONG_MAX;
> +	unsigned long long pci_addr_hi = 0x0;
> +	dma_addr_t pci_dma_sz;
> 
> -	node = NULL;
> 	for_each_node_by_type(node, "pci") {
> 		if (of_match_node(pci_ids, node)) {
> -			/*
> -			 * If there's no PCI host bridge with ISA, arbitrarily
> -			 * designate one as primary.  This can go away once
> -			 * various bugs with primary-less systems are fixed.
> -			 */
> -			if (!fsl_pci_primary)
> -				fsl_pci_primary = node;
> -
> -			fsl_add_bridge(node, fsl_pci_primary == node);
> -			hose = pci_find_hose_for_OF_device(node);
> -			max = min(max, hose->dma_window_base_cur +
> -					hose->dma_window_size);
> +			memno = 0;
> +			pna = of_n_addr_cells(node);
> +			np = pna + 5;

Don't duplicate code from pci_process_bridge_OF_ranges(), refactor the code to have a shared function:

> +			/* Get ranges property */
> +			ranges = of_get_property(node, "ranges", &rlen);
> +			if (ranges == NULL)
> +				return;
> +
> +			/* Parse outbound MEM window range */
> +			while ((rlen -= np * 4) >= 0) {
> +				/* Read next ranges element */
> +				pci_space = ranges[0];
> +				if (!((pci_space >> 24) & 0x2)) {
> +					ranges += np;
> +					break;
> +				}
> +				pci_addr = of_read_number(ranges + 1, 2);
> +				cpu_addr = of_translate_address(
> +						node, ranges + 3);
> +				size = of_read_number(ranges + pna + 3, 2);
> +				ranges += np;
> +
> +				/*
> +				 * If we failed translation or got a zero-sized
> +				 * region (some FW try to feed us with non
> +				 * sensical zero sized regions such as power3
> +				 * which look like some kind of attempt at
> +				 * exposing the VGA memory hole)
> +				 */
> +				if (cpu_addr == OF_BAD_ADDR || size == 0)
> +					continue;
> +
> +				/*
> +				 * Now consume following elements while they
> +				 * are contiguous
> +				 */
> +				for (; rlen >= np * sizeof(u32);
> +						ranges += np, rlen -= np * 4) {
> +					if (ranges[0] != pci_space)
> +						break;
> +					pci_next = of_read_number(ranges + 1,
> +							2);
> +					cpu_next = of_translate_address(node,
> +							ranges + 3);
> +					if (pci_next != pci_addr + size ||
> +						cpu_next != cpu_addr + size)
> +						break;
> +					size += of_read_number(
> +							ranges + pna + 3, 2);
> +				}
> +
> +				/* We support only 3 memory ranges */
> +				if (memno >= 3) {
> +					printk(KERN_INFO
> +							" \\--> Skipped (too many) !\n");
> +					continue;
> +				}
> +
> +				pci_addr_lo = min(pci_addr, pci_addr_lo);
> +				pci_addr_hi = max(pci_addr + size, pci_addr_hi);
> +				memno++;
> +			}
> 		}
> 	}
> 
> -#ifdef CONFIG_SWIOTLB
> +	/* Get PEXCSRBAR size (equal to CCSR size) */
> +	node = of_find_node_by_type(NULL, "soc");
> +	ranges = of_get_property(node, "ranges", &rlen);
> +	if (ranges == NULL)
> +		return;
> +
> +	size = of_read_number(ranges + 3, 1);
> +	of_node_put(node);
> +
> +	if (pci_addr_hi < (0x100000000ull - size))
> +		pci_dma_sz = pci_addr_lo;
> +	else
> +		pci_dma_sz = pci_addr_lo - size;
> +
> 	/*
> 	 * if we couldn't map all of DRAM via the dma windows
> 	 * we need SWIOTLB to handle buffers located outside of
> 	 * dma capable memory region
> 	 */
> -	if (memblock_end_of_DRAM() - 1 > max) {
> +	if (memblock_end_of_DRAM() > pci_dma_sz) {
> 		ppc_swiotlb_enable = 1;
> 		set_pci_dma_ops(&swiotlb_dma_ops);
> -		ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb;
> +		ppc_md.pci_dma_dev_setup =
> +			pci_dma_dev_setup_swiotlb;

why the line wrap change?

> 	}
> +}
> #endif
> +
> +int primary_phb_addr;
> +static int __devinit fsl_pci_probe(struct platform_device *pdev)
> +{
> +	struct pci_controller *hose;
> +	bool is_primary;
> +
> +	if (of_match_node(pci_ids, pdev->dev.of_node)) {
> +		struct resource rsrc;
> +		of_address_to_resource(pdev->dev.of_node, 0, &rsrc);
> +		is_primary = ((rsrc.start & 0xfffff) == primary_phb_addr);
> +		fsl_add_bridge(pdev->dev.of_node, is_primary);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver fsl_pci_driver = {
> +	.driver = {
> +		.name = "fsl-pci",
> +		.of_match_table = pci_ids,
> +	},
> +	.probe = fsl_pci_probe,
> +};
> +
> +static int __init fsl_pci_init(void)
> +{
> +	return platform_driver_register(&fsl_pci_driver);
> }
> +arch_initcall(fsl_pci_init);
> #endif

On Jul 26, 2012, at 7:30 AM, Jia Hongtao wrote:

> We unified the Freescale pci/pcie initialization by changing the fsl_pci
> to a platform driver. In previous PCI code architecture the initialization
> routine is called at board_setup_arch stage. Now the initialization is done
> in probe function which is architectural better. Also It's convenient for
> adding PM support for PCI controller in later patch.
> 
> One issue introduced by this architecture is the timing of swiotlb_init.
> During PCI initialization the need of swiotlb is determined and this should
> be done before swiotlb_init. So a new function to determine swiotlb by
> parsing pci ranges is made. This function is called at board_setup_arch
> stage which is earlier than swiotlb_init.
> 
> Signed-off-by: Jia Hongtao <B38951@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
> Changed for V3:
> - Rebase the patch set on the latest tree
> - merge PCI unify and swiotlb patch into one
> 
> arch/powerpc/sysdev/fsl_pci.c |  155 ++++++++++++++++++++++++++++++++---------
> arch/powerpc/sysdev/fsl_pci.h |    9 +--
> 2 files changed, 125 insertions(+), 39 deletions(-)

I'd like the SWIOTLB refactoring as a separate patch.  Additionally, the order of patches should be as follows:

1. refactor PCI node parsing code
2. add pci_determine_swiotlb (should rename to fsl_pci_determine_swiotlb)
3. Determine primary bus by looking for ISA node
4. convert all boards over to fsl_pci_init
5. convert fsl pci to platform driver (edac and other fixes should be merged in here)
6. PM support

- k

> -----Original Message-----
> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> Sent: Friday, July 27, 2012 2:15 AM
> To: Jia Hongtao-B38951
> Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Li Yang-R58472
> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
> initialization code
> 
> 
> On Jul 26, 2012, at 7:30 AM, Jia Hongtao wrote:
> 
> > We unified the Freescale pci/pcie initialization by changing the
> fsl_pci
> > to a platform driver. In previous PCI code architecture the
> initialization
> > routine is called at board_setup_arch stage. Now the initialization is
> done
> > in probe function which is architectural better. Also It's convenient
> for
> > adding PM support for PCI controller in later patch.
> >
> > One issue introduced by this architecture is the timing of swiotlb_init.
> > During PCI initialization the need of swiotlb is determined and this
> should
> > be done before swiotlb_init. So a new function to determine swiotlb by
> > parsing pci ranges is made. This function is called at board_setup_arch
> > stage which is earlier than swiotlb_init.
> >
> > Signed-off-by: Jia Hongtao <B38951@freescale.com>
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > ---
> > Changed for V3:
> > - Rebase the patch set on the latest tree
> > - merge PCI unify and swiotlb patch into one
> >
> > arch/powerpc/sysdev/fsl_pci.c |  155 ++++++++++++++++++++++++++++++++--
> -------
> > arch/powerpc/sysdev/fsl_pci.h |    9 +--
> > 2 files changed, 125 insertions(+), 39 deletions(-)
> 
> I'd like the SWIOTLB refactoring as a separate patch.  Additionally, the
> order of patches should be as follows:
> 
> 1. refactor PCI node parsing code
> 2. add pci_determine_swiotlb (should rename to fsl_pci_determine_swiotlb)
> 3. Determine primary bus by looking for ISA node
> 4. convert all boards over to fsl_pci_init
> 5. convert fsl pci to platform driver (edac and other fixes should be
> merged in here)
> 6. PM support
> 
> - k

Should I convert all boards over to fsl_pci_init first and then convert them
over to platform driver again or just convert them direct to platform driver?

Thanks.
-Hongtao.

Hi kumar,

I know "duplicate code from pci_process_bridge_OF_ranges()" is
hard to accept but "refactor the code to have a shared function"
is knotty. Actually this is the reason I didn't do the refactor.

Here is the situation:

First, pci_process_bridge_OF_ranges() is a common code using by
so many pci client.

Second, the contents of pci_process_bridge_OF_ranges() twisted
together. It's hard to decouple the function I need for determining
swiotlb with other contents. I tried and found a way to do this
but the shared function need so many parameters which is also
unacceptable. 

Third, my function to determine swiotlb should know the start
and the end of pci mem space address for all the controllers.
Note that the end of address is for determining where to map
PEXCSRBAR.

If you have any idea for this please let me know.

Thanks.
-Hongtao.


> -----Original Message-----
> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> Sent: Friday, July 27, 2012 1:53 AM
> To: Jia Hongtao-B38951
> Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Li Yang-R58472
> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
> initialization code
> 
> 
> On Jul 26, 2012, at 7:30 AM, Jia Hongtao wrote:
> 
> > We unified the Freescale pci/pcie initialization by changing the
> fsl_pci
> > to a platform driver. In previous PCI code architecture the
> initialization
> > routine is called at board_setup_arch stage. Now the initialization is
> done
> > in probe function which is architectural better. Also It's convenient
> for
> > adding PM support for PCI controller in later patch.
> >
> > One issue introduced by this architecture is the timing of swiotlb_init.
> > During PCI initialization the need of swiotlb is determined and this
> should
> > be done before swiotlb_init. So a new function to determine swiotlb by
> > parsing pci ranges is made. This function is called at board_setup_arch
> > stage which is earlier than swiotlb_init.
> >
> > Signed-off-by: Jia Hongtao <B38951@freescale.com>
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > ---
> > Changed for V3:
> > - Rebase the patch set on the latest tree
> > - merge PCI unify and swiotlb patch into one
> >
> > arch/powerpc/sysdev/fsl_pci.c |  155 ++++++++++++++++++++++++++++++++--
> -------
> > arch/powerpc/sysdev/fsl_pci.h |    9 +--
> > 2 files changed, 125 insertions(+), 39 deletions(-)
> >
> > diff --git a/arch/powerpc/sysdev/fsl_pci.c
> b/arch/powerpc/sysdev/fsl_pci.c
> > index a7b2a60..5228b6b 100644
> > --- a/arch/powerpc/sysdev/fsl_pci.c
> > +++ b/arch/powerpc/sysdev/fsl_pci.c
> > @@ -823,56 +823,143 @@ static const struct of_device_id pci_ids[] = {
> > 	{},
> > };
> >
> > -struct device_node *fsl_pci_primary;
> > -
> > -void __devinit fsl_pci_init(void)
> > +#ifdef CONFIG_SWIOTLB
> > +void pci_determine_swiotlb(void)
> > {
> > +	const u32 *ranges;
> > +	int rlen;
> > +	int pna;
> > +	int np;
> > 	struct device_node *node;
> > -	struct pci_controller *hose;
> > -	dma_addr_t max = 0xffffffff;
> > -
> > -	/* 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;
> > -		}
> > -	}
> > +	int memno;
> > +	u32 pci_space;
> > +	unsigned long long pci_addr, cpu_addr, pci_next, cpu_next, size;
> > +	unsigned long long pci_addr_lo = ULLONG_MAX;
> > +	unsigned long long pci_addr_hi = 0x0;
> > +	dma_addr_t pci_dma_sz;
> >
> > -	node = NULL;
> > 	for_each_node_by_type(node, "pci") {
> > 		if (of_match_node(pci_ids, node)) {
> > -			/*
> > -			 * If there's no PCI host bridge with ISA, arbitrarily
> > -			 * designate one as primary.  This can go away once
> > -			 * various bugs with primary-less systems are fixed.
> > -			 */
> > -			if (!fsl_pci_primary)
> > -				fsl_pci_primary = node;
> > -
> > -			fsl_add_bridge(node, fsl_pci_primary == node);
> > -			hose = pci_find_hose_for_OF_device(node);
> > -			max = min(max, hose->dma_window_base_cur +
> > -					hose->dma_window_size);
> > +			memno = 0;
> > +			pna = of_n_addr_cells(node);
> > +			np = pna + 5;
> 
> Don't duplicate code from pci_process_bridge_OF_ranges(), refactor the
> code to have a shared function:
> 
> > +			/* Get ranges property */
> > +			ranges = of_get_property(node, "ranges", &rlen);
> > +			if (ranges == NULL)
> > +				return;
> > +
> > +			/* Parse outbound MEM window range */
> > +			while ((rlen -= np * 4) >= 0) {
> > +				/* Read next ranges element */
> > +				pci_space = ranges[0];
> > +				if (!((pci_space >> 24) & 0x2)) {
> > +					ranges += np;
> > +					break;
> > +				}
> > +				pci_addr = of_read_number(ranges + 1, 2);
> > +				cpu_addr = of_translate_address(
> > +						node, ranges + 3);
> > +				size = of_read_number(ranges + pna + 3, 2);
> > +				ranges += np;
> > +
> > +				/*
> > +				 * If we failed translation or got a zero-sized
> > +				 * region (some FW try to feed us with non
> > +				 * sensical zero sized regions such as power3
> > +				 * which look like some kind of attempt at
> > +				 * exposing the VGA memory hole)
> > +				 */
> > +				if (cpu_addr == OF_BAD_ADDR || size == 0)
> > +					continue;
> > +
> > +				/*
> > +				 * Now consume following elements while they
> > +				 * are contiguous
> > +				 */
> > +				for (; rlen >= np * sizeof(u32);
> > +						ranges += np, rlen -= np * 4) {
> > +					if (ranges[0] != pci_space)
> > +						break;
> > +					pci_next = of_read_number(ranges + 1,
> > +							2);
> > +					cpu_next = of_translate_address(node,
> > +							ranges + 3);
> > +					if (pci_next != pci_addr + size ||
> > +						cpu_next != cpu_addr + size)
> > +						break;
> > +					size += of_read_number(
> > +							ranges + pna + 3, 2);
> > +				}
> > +
> > +				/* We support only 3 memory ranges */
> > +				if (memno >= 3) {
> > +					printk(KERN_INFO
> > +							" \\--> Skipped (too
> many) !\n");
> > +					continue;
> > +				}
> > +
> > +				pci_addr_lo = min(pci_addr, pci_addr_lo);
> > +				pci_addr_hi = max(pci_addr + size, pci_addr_hi);
> > +				memno++;
> > +			}
> > 		}
> > 	}
> >
> > -#ifdef CONFIG_SWIOTLB
> > +	/* Get PEXCSRBAR size (equal to CCSR size) */
> > +	node = of_find_node_by_type(NULL, "soc");
> > +	ranges = of_get_property(node, "ranges", &rlen);
> > +	if (ranges == NULL)
> > +		return;
> > +
> > +	size = of_read_number(ranges + 3, 1);
> > +	of_node_put(node);
> > +
> > +	if (pci_addr_hi < (0x100000000ull - size))
> > +		pci_dma_sz = pci_addr_lo;
> > +	else
> > +		pci_dma_sz = pci_addr_lo - size;
> > +
> > 	/*
> > 	 * if we couldn't map all of DRAM via the dma windows
> > 	 * we need SWIOTLB to handle buffers located outside of
> > 	 * dma capable memory region
> > 	 */
> > -	if (memblock_end_of_DRAM() - 1 > max) {
> > +	if (memblock_end_of_DRAM() > pci_dma_sz) {
> > 		ppc_swiotlb_enable = 1;
> > 		set_pci_dma_ops(&swiotlb_dma_ops);
> > -		ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb;
> > +		ppc_md.pci_dma_dev_setup =
> > +			pci_dma_dev_setup_swiotlb;
> 
> why the line wrap change?
> 
> > 	}
> > +}
> > #endif
> > +
> > +int primary_phb_addr;
> > +static int __devinit fsl_pci_probe(struct platform_device *pdev)
> > +{
> > +	struct pci_controller *hose;
> > +	bool is_primary;
> > +
> > +	if (of_match_node(pci_ids, pdev->dev.of_node)) {
> > +		struct resource rsrc;
> > +		of_address_to_resource(pdev->dev.of_node, 0, &rsrc);
> > +		is_primary = ((rsrc.start & 0xfffff) == primary_phb_addr);
> > +		fsl_add_bridge(pdev->dev.of_node, is_primary);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver fsl_pci_driver = {
> > +	.driver = {
> > +		.name = "fsl-pci",
> > +		.of_match_table = pci_ids,
> > +	},
> > +	.probe = fsl_pci_probe,
> > +};
> > +
> > +static int __init fsl_pci_init(void)
> > +{
> > +	return platform_driver_register(&fsl_pci_driver);
> > }
> > +arch_initcall(fsl_pci_init);
> > #endif
> 
>

On Jul 27, 2012, at 3:35 AM, Jia Hongtao-B38951 wrote:

> 
> 
>> -----Original Message-----
>> From: Kumar Gala [mailto:galak@kernel.crashing.org]
>> Sent: Friday, July 27, 2012 2:15 AM
>> To: Jia Hongtao-B38951
>> Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Li Yang-R58472
>> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
>> initialization code
>> 
>> 
>> On Jul 26, 2012, at 7:30 AM, Jia Hongtao wrote:
>> 
>>> We unified the Freescale pci/pcie initialization by changing the
>> fsl_pci
>>> to a platform driver. In previous PCI code architecture the
>> initialization
>>> routine is called at board_setup_arch stage. Now the initialization is
>> done
>>> in probe function which is architectural better. Also It's convenient
>> for
>>> adding PM support for PCI controller in later patch.
>>> 
>>> One issue introduced by this architecture is the timing of swiotlb_init.
>>> During PCI initialization the need of swiotlb is determined and this
>> should
>>> be done before swiotlb_init. So a new function to determine swiotlb by
>>> parsing pci ranges is made. This function is called at board_setup_arch
>>> stage which is earlier than swiotlb_init.
>>> 
>>> Signed-off-by: Jia Hongtao <B38951@freescale.com>
>>> Signed-off-by: Li Yang <leoli@freescale.com>
>>> ---
>>> Changed for V3:
>>> - Rebase the patch set on the latest tree
>>> - merge PCI unify and swiotlb patch into one
>>> 
>>> arch/powerpc/sysdev/fsl_pci.c |  155 ++++++++++++++++++++++++++++++++--
>> -------
>>> arch/powerpc/sysdev/fsl_pci.h |    9 +--
>>> 2 files changed, 125 insertions(+), 39 deletions(-)
>> 
>> I'd like the SWIOTLB refactoring as a separate patch.  Additionally, the
>> order of patches should be as follows:
>> 
>> 1. refactor PCI node parsing code
>> 2. add pci_determine_swiotlb (should rename to fsl_pci_determine_swiotlb)
>> 3. Determine primary bus by looking for ISA node
>> 4. convert all boards over to fsl_pci_init
>> 5. convert fsl pci to platform driver (edac and other fixes should be
>> merged in here)
>> 6. PM support
>> 
>> - k
> 
> Should I convert all boards over to fsl_pci_init first and then convert them
> over to platform driver again or just convert them direct to platform driver?

Yes do the fsl_pci_init conversion first.  The reason is we should NOT break functionality from one patch to another.

- k

On 07/27/2012 05:10 AM, Jia Hongtao-B38951 wrote:
> Hi kumar,
> 
> I know "duplicate code from pci_process_bridge_OF_ranges()" is
> hard to accept but "refactor the code to have a shared function"
> is knotty. Actually this is the reason I didn't do the refactor.

Maybe we should keep doing the init early?  We could still have a
platform device for the PM stuff, but some init would be done before probe.

Another possibility is to try to handle swiotlb init later -- possibly
by reserving memory for it if the platform indicates it's a possibility
that it will be needed, then freeing the memory if it's not needed.

-Scott

On Jul 27, 2012, at 3:24 PM, Scott Wood wrote:

> On 07/27/2012 05:10 AM, Jia Hongtao-B38951 wrote:
>> Hi kumar,
>> 
>> I know "duplicate code from pci_process_bridge_OF_ranges()" is
>> hard to accept but "refactor the code to have a shared function"
>> is knotty. Actually this is the reason I didn't do the refactor.
> 
> Maybe we should keep doing the init early?  We could still have a
> platform device for the PM stuff, but some init would be done before probe.
> 
> Another possibility is to try to handle swiotlb init later -- possibly
> by reserving memory for it if the platform indicates it's a possibility
> that it will be needed, then freeing the memory if it's not needed.
> 
> -Scott

I think the first option seems reasonable.  Can we leave fsl_pci_init() as we now have it and just have the platform driver deal with PM restore via calling setup_pci_atmu() [probably need to update setup_pci_atmu to handle restore case, but seems like minor changes]

- k

> -----Original Message-----
> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> Sent: Friday, July 27, 2012 8:47 PM
> To: Jia Hongtao-B38951
> Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Li Yang-R58472
> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
> initialization code
> 
> 
> On Jul 27, 2012, at 3:35 AM, Jia Hongtao-B38951 wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> >> Sent: Friday, July 27, 2012 2:15 AM
> >> To: Jia Hongtao-B38951
> >> Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Li Yang-R58472
> >> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
> >> initialization code
> >>
> >>
> >> On Jul 26, 2012, at 7:30 AM, Jia Hongtao wrote:
> >>
> >>> We unified the Freescale pci/pcie initialization by changing the
> >> fsl_pci
> >>> to a platform driver. In previous PCI code architecture the
> >> initialization
> >>> routine is called at board_setup_arch stage. Now the initialization
> is
> >> done
> >>> in probe function which is architectural better. Also It's convenient
> >> for
> >>> adding PM support for PCI controller in later patch.
> >>>
> >>> One issue introduced by this architecture is the timing of
> swiotlb_init.
> >>> During PCI initialization the need of swiotlb is determined and this
> >> should
> >>> be done before swiotlb_init. So a new function to determine swiotlb
> by
> >>> parsing pci ranges is made. This function is called at
> board_setup_arch
> >>> stage which is earlier than swiotlb_init.
> >>>
> >>> Signed-off-by: Jia Hongtao <B38951@freescale.com>
> >>> Signed-off-by: Li Yang <leoli@freescale.com>
> >>> ---
> >>> Changed for V3:
> >>> - Rebase the patch set on the latest tree
> >>> - merge PCI unify and swiotlb patch into one
> >>>
> >>> arch/powerpc/sysdev/fsl_pci.c |  155
> ++++++++++++++++++++++++++++++++--
> >> -------
> >>> arch/powerpc/sysdev/fsl_pci.h |    9 +--
> >>> 2 files changed, 125 insertions(+), 39 deletions(-)
> >>
> >> I'd like the SWIOTLB refactoring as a separate patch.  Additionally,
> the
> >> order of patches should be as follows:
> >>
> >> 1. refactor PCI node parsing code
> >> 2. add pci_determine_swiotlb (should rename to
> fsl_pci_determine_swiotlb)
> >> 3. Determine primary bus by looking for ISA node
> >> 4. convert all boards over to fsl_pci_init
> >> 5. convert fsl pci to platform driver (edac and other fixes should be
> >> merged in here)
> >> 6. PM support
> >>
> >> - k
> >
> > Should I convert all boards over to fsl_pci_init first and then convert
> them
> > over to platform driver again or just convert them direct to platform
> driver?
> 
> Yes do the fsl_pci_init conversion first.  The reason is we should NOT
> break functionality from one patch to another.
> 
> - k


Actually, the functionality is not broken, other boards just use the old
Way to init pci controller and it still works.

-Hongtao.

> -----Original Message-----
> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> Sent: Saturday, July 28, 2012 5:17 AM
> To: Wood Scott-B07421
> Cc: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421;
> Li Yang-R58472
> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
> initialization code
> 
> 
> On Jul 27, 2012, at 3:24 PM, Scott Wood wrote:
> 
> > On 07/27/2012 05:10 AM, Jia Hongtao-B38951 wrote:
> >> Hi kumar,
> >>
> >> I know "duplicate code from pci_process_bridge_OF_ranges()" is
> >> hard to accept but "refactor the code to have a shared function"
> >> is knotty. Actually this is the reason I didn't do the refactor.
> >
> > Maybe we should keep doing the init early?  We could still have a
> > platform device for the PM stuff, but some init would be done before
> probe.
> >
> > Another possibility is to try to handle swiotlb init later -- possibly
> > by reserving memory for it if the platform indicates it's a possibility
> > that it will be needed, then freeing the memory if it's not needed.
> >
> > -Scott
> 
> I think the first option seems reasonable.  Can we leave fsl_pci_init()
> as we now have it and just have the platform driver deal with PM restore
> via calling setup_pci_atmu() [probably need to update setup_pci_atmu to
> handle restore case, but seems like minor changes]
> 
> - k
> 


I think the second option is better if it's hard to decouple swiotlb
determination from pci init. I believe the better architecture that
PCI init in probe function of platform driver will bring us considerable
advantage. I really like to keep the completion of pci controller
platform driver not only for PM support but also for pci init.

-Hongtao.

On Jul 30, 2012, at 3:07 AM, Jia Hongtao-B38951 wrote:

>> -----Original Message-----
>> From: Kumar Gala [mailto:galak@kernel.crashing.org]
>> Sent: Friday, July 27, 2012 8:47 PM
>> To: Jia Hongtao-B38951
>> Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Li Yang-R58472
>> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
>> initialization code
>> 
>> 
>> On Jul 27, 2012, at 3:35 AM, Jia Hongtao-B38951 wrote:
>> 
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Kumar Gala [mailto:galak@kernel.crashing.org]
>>>> Sent: Friday, July 27, 2012 2:15 AM
>>>> To: Jia Hongtao-B38951
>>>> Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Li Yang-R58472
>>>> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
>>>> initialization code
>>>> 
>>>> 
>>>> On Jul 26, 2012, at 7:30 AM, Jia Hongtao wrote:
>>>> 
>>>>> We unified the Freescale pci/pcie initialization by changing the
>>>> fsl_pci
>>>>> to a platform driver. In previous PCI code architecture the
>>>> initialization
>>>>> routine is called at board_setup_arch stage. Now the initialization
>> is
>>>> done
>>>>> in probe function which is architectural better. Also It's convenient
>>>> for
>>>>> adding PM support for PCI controller in later patch.
>>>>> 
>>>>> One issue introduced by this architecture is the timing of
>> swiotlb_init.
>>>>> During PCI initialization the need of swiotlb is determined and this
>>>> should
>>>>> be done before swiotlb_init. So a new function to determine swiotlb
>> by
>>>>> parsing pci ranges is made. This function is called at
>> board_setup_arch
>>>>> stage which is earlier than swiotlb_init.
>>>>> 
>>>>> Signed-off-by: Jia Hongtao <B38951@freescale.com>
>>>>> Signed-off-by: Li Yang <leoli@freescale.com>
>>>>> ---
>>>>> Changed for V3:
>>>>> - Rebase the patch set on the latest tree
>>>>> - merge PCI unify and swiotlb patch into one
>>>>> 
>>>>> arch/powerpc/sysdev/fsl_pci.c |  155
>> ++++++++++++++++++++++++++++++++--
>>>> -------
>>>>> arch/powerpc/sysdev/fsl_pci.h |    9 +--
>>>>> 2 files changed, 125 insertions(+), 39 deletions(-)
>>>> 
>>>> I'd like the SWIOTLB refactoring as a separate patch.  Additionally,
>> the
>>>> order of patches should be as follows:
>>>> 
>>>> 1. refactor PCI node parsing code
>>>> 2. add pci_determine_swiotlb (should rename to
>> fsl_pci_determine_swiotlb)
>>>> 3. Determine primary bus by looking for ISA node
>>>> 4. convert all boards over to fsl_pci_init
>>>> 5. convert fsl pci to platform driver (edac and other fixes should be
>>>> merged in here)
>>>> 6. PM support
>>>> 
>>>> - k
>>> 
>>> Should I convert all boards over to fsl_pci_init first and then convert
>> them
>>> over to platform driver again or just convert them direct to platform
>> driver?
>> 
>> Yes do the fsl_pci_init conversion first.  The reason is we should NOT
>> break functionality from one patch to another.
>> 
>> - k
> 
> 
> Actually, the functionality is not broken, other boards just use the old
> Way to init pci controller and it still works.

How do you figure?  The platform driver is going to get called on boards not yet converted.  So than you will get 2 different inits of PCI going on.

- k

On Jul 30, 2012, at 3:26 AM, Jia Hongtao-B38951 wrote:

> 
> 
>> -----Original Message-----
>> From: Kumar Gala [mailto:galak@kernel.crashing.org]
>> Sent: Saturday, July 28, 2012 5:17 AM
>> To: Wood Scott-B07421
>> Cc: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421;
>> Li Yang-R58472
>> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
>> initialization code
>> 
>> 
>> On Jul 27, 2012, at 3:24 PM, Scott Wood wrote:
>> 
>>> On 07/27/2012 05:10 AM, Jia Hongtao-B38951 wrote:
>>>> Hi kumar,
>>>> 
>>>> I know "duplicate code from pci_process_bridge_OF_ranges()" is
>>>> hard to accept but "refactor the code to have a shared function"
>>>> is knotty. Actually this is the reason I didn't do the refactor.
>>> 
>>> Maybe we should keep doing the init early?  We could still have a
>>> platform device for the PM stuff, but some init would be done before
>> probe.
>>> 
>>> Another possibility is to try to handle swiotlb init later -- possibly
>>> by reserving memory for it if the platform indicates it's a possibility
>>> that it will be needed, then freeing the memory if it's not needed.
>>> 
>>> -Scott
>> 
>> I think the first option seems reasonable.  Can we leave fsl_pci_init()
>> as we now have it and just have the platform driver deal with PM restore
>> via calling setup_pci_atmu() [probably need to update setup_pci_atmu to
>> handle restore case, but seems like minor changes]
>> 
>> - k
>> 
> 
> 
> I think the second option is better if it's hard to decouple swiotlb
> determination from pci init. I believe the better architecture that
> PCI init in probe function of platform driver will bring us considerable
> advantage. I really like to keep the completion of pci controller
> platform driver not only for PM support but also for pci init.
> 
> -Hongtao. 
> 

Shifting of swiotlb init has a lot more issues.  Why do we need to do the PCI init in probe?

- k

> -----Original Message-----
> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> Sent: Monday, July 30, 2012 10:47 PM
> To: Jia Hongtao-B38951
> Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Li Yang-R58472
> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
> initialization code
> 
> 
> On Jul 30, 2012, at 3:07 AM, Jia Hongtao-B38951 wrote:
> 
> >> -----Original Message-----
> >> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> >> Sent: Friday, July 27, 2012 8:47 PM
> >> To: Jia Hongtao-B38951
> >> Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Li Yang-R58472
> >> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
> >> initialization code
> >>
> >>
> >> On Jul 27, 2012, at 3:35 AM, Jia Hongtao-B38951 wrote:
> >>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> >>>> Sent: Friday, July 27, 2012 2:15 AM
> >>>> To: Jia Hongtao-B38951
> >>>> Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Li Yang-R58472
> >>>> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
> >>>> initialization code
> >>>>
> >>>>
> >>>> On Jul 26, 2012, at 7:30 AM, Jia Hongtao wrote:
> >>>>
> >>>>> We unified the Freescale pci/pcie initialization by changing the
> >>>> fsl_pci
> >>>>> to a platform driver. In previous PCI code architecture the
> >>>> initialization
> >>>>> routine is called at board_setup_arch stage. Now the initialization
> >> is
> >>>> done
> >>>>> in probe function which is architectural better. Also It's
> convenient
> >>>> for
> >>>>> adding PM support for PCI controller in later patch.
> >>>>>
> >>>>> One issue introduced by this architecture is the timing of
> >> swiotlb_init.
> >>>>> During PCI initialization the need of swiotlb is determined and
> this
> >>>> should
> >>>>> be done before swiotlb_init. So a new function to determine swiotlb
> >> by
> >>>>> parsing pci ranges is made. This function is called at
> >> board_setup_arch
> >>>>> stage which is earlier than swiotlb_init.
> >>>>>
> >>>>> Signed-off-by: Jia Hongtao <B38951@freescale.com>
> >>>>> Signed-off-by: Li Yang <leoli@freescale.com>
> >>>>> ---
> >>>>> Changed for V3:
> >>>>> - Rebase the patch set on the latest tree
> >>>>> - merge PCI unify and swiotlb patch into one
> >>>>>
> >>>>> arch/powerpc/sysdev/fsl_pci.c |  155
> >> ++++++++++++++++++++++++++++++++--
> >>>> -------
> >>>>> arch/powerpc/sysdev/fsl_pci.h |    9 +--
> >>>>> 2 files changed, 125 insertions(+), 39 deletions(-)
> >>>>
> >>>> I'd like the SWIOTLB refactoring as a separate patch.  Additionally,
> >> the
> >>>> order of patches should be as follows:
> >>>>
> >>>> 1. refactor PCI node parsing code
> >>>> 2. add pci_determine_swiotlb (should rename to
> >> fsl_pci_determine_swiotlb)
> >>>> 3. Determine primary bus by looking for ISA node
> >>>> 4. convert all boards over to fsl_pci_init
> >>>> 5. convert fsl pci to platform driver (edac and other fixes should
> be
> >>>> merged in here)
> >>>> 6. PM support
> >>>>
> >>>> - k
> >>>
> >>> Should I convert all boards over to fsl_pci_init first and then
> convert
> >> them
> >>> over to platform driver again or just convert them direct to platform
> >> driver?
> >>
> >> Yes do the fsl_pci_init conversion first.  The reason is we should NOT
> >> break functionality from one patch to another.
> >>
> >> - k
> >
> >
> > Actually, the functionality is not broken, other boards just use the
> old
> > Way to init pci controller and it still works.
> 
> How do you figure?  The platform driver is going to get called on boards
> not yet converted.  So than you will get 2 different inits of PCI going
> on.
> 
> - k

In Scott's patch set no platform driver used. fsl_pci_init is just a unified
routine function for all boards to call. Now other boards in which fsl_pci_init
is not called just use the old way to init.

-Hongtao.

> -----Original Message-----
> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> Sent: Monday, July 30, 2012 10:47 PM
> To: Jia Hongtao-B38951
> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; Li Yang-R58472
> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
> initialization code
> 
> 
> On Jul 30, 2012, at 3:26 AM, Jia Hongtao-B38951 wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> >> Sent: Saturday, July 28, 2012 5:17 AM
> >> To: Wood Scott-B07421
> >> Cc: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org; Wood Scott-
> B07421;
> >> Li Yang-R58472
> >> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
> >> initialization code
> >>
> >>
> >> On Jul 27, 2012, at 3:24 PM, Scott Wood wrote:
> >>
> >>> On 07/27/2012 05:10 AM, Jia Hongtao-B38951 wrote:
> >>>> Hi kumar,
> >>>>
> >>>> I know "duplicate code from pci_process_bridge_OF_ranges()" is
> >>>> hard to accept but "refactor the code to have a shared function"
> >>>> is knotty. Actually this is the reason I didn't do the refactor.
> >>>
> >>> Maybe we should keep doing the init early?  We could still have a
> >>> platform device for the PM stuff, but some init would be done before
> >> probe.
> >>>
> >>> Another possibility is to try to handle swiotlb init later --
> possibly
> >>> by reserving memory for it if the platform indicates it's a
> possibility
> >>> that it will be needed, then freeing the memory if it's not needed.
> >>>
> >>> -Scott
> >>
> >> I think the first option seems reasonable.  Can we leave fsl_pci_init()
> >> as we now have it and just have the platform driver deal with PM
> restore
> >> via calling setup_pci_atmu() [probably need to update setup_pci_atmu
> to
> >> handle restore case, but seems like minor changes]
> >>
> >> - k
> >>
> >
> >
> > I think the second option is better if it's hard to decouple swiotlb
> > determination from pci init. I believe the better architecture that
> > PCI init in probe function of platform driver will bring us
> considerable
> > advantage. I really like to keep the completion of pci controller
> > platform driver not only for PM support but also for pci init.
> >
> > -Hongtao.
> >
> 
> Shifting of swiotlb init has a lot more issues.  Why do we need to do the
> PCI init in probe?
> 
> - k

I investigated the swiotlb init thing and found that in x86 swiotlb init is
done first and free if we don't need it.

-Hongtao.

On Mon, Jul 30, 2012 at 10:46 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
>
> On Jul 30, 2012, at 3:26 AM, Jia Hongtao-B38951 wrote:
>
>>
>>
>>> -----Original Message-----
>>> From: Kumar Gala [mailto:galak@kernel.crashing.org]
>>> Sent: Saturday, July 28, 2012 5:17 AM
>>> To: Wood Scott-B07421
>>> Cc: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421;
>>> Li Yang-R58472
>>> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
>>> initialization code
>>>
>>>
>>> On Jul 27, 2012, at 3:24 PM, Scott Wood wrote:
>>>
>>>> On 07/27/2012 05:10 AM, Jia Hongtao-B38951 wrote:
>>>>> Hi kumar,
>>>>>
>>>>> I know "duplicate code from pci_process_bridge_OF_ranges()" is
>>>>> hard to accept but "refactor the code to have a shared function"
>>>>> is knotty. Actually this is the reason I didn't do the refactor.
>>>>
>>>> Maybe we should keep doing the init early?  We could still have a
>>>> platform device for the PM stuff, but some init would be done before
>>> probe.
>>>>
>>>> Another possibility is to try to handle swiotlb init later -- possibly
>>>> by reserving memory for it if the platform indicates it's a possibility
>>>> that it will be needed, then freeing the memory if it's not needed.
>>>>
>>>> -Scott
>>>
>>> I think the first option seems reasonable.  Can we leave fsl_pci_init()
>>> as we now have it and just have the platform driver deal with PM restore
>>> via calling setup_pci_atmu() [probably need to update setup_pci_atmu to
>>> handle restore case, but seems like minor changes]
>>>
>>> - k
>>>
>>
>>
>> I think the second option is better if it's hard to decouple swiotlb
>> determination from pci init. I believe the better architecture that
>> PCI init in probe function of platform driver will bring us considerable
>> advantage. I really like to keep the completion of pci controller
>> platform driver not only for PM support but also for pci init.
>>
>> -Hongtao.
>>
>
> Shifting of swiotlb init has a lot more issues.  Why do we need to do the PCI init in probe?

The ordering issues are introduced by swiotlb.  And the ideal way is
to solve the problem within swiotlb instead of changing PCI to
workaround it.  Take the implementation of x86 as reference it's
possible to be addressed bu allocating first and free later approach.

It is common sense that the initialization of a device is in the probe
function of the driver of the device.  And the change will provide
better unification of PCI controller code.  The PCI controller is
generic enough not to be taken care of at the platform area.

Leo

On Jul 31, 2012, at 2:21 AM, Li Yang wrote:

> On Mon, Jul 30, 2012 at 10:46 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
>> 
>> On Jul 30, 2012, at 3:26 AM, Jia Hongtao-B38951 wrote:
>> 
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Kumar Gala [mailto:galak@kernel.crashing.org]
>>>> Sent: Saturday, July 28, 2012 5:17 AM
>>>> To: Wood Scott-B07421
>>>> Cc: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421;
>>>> Li Yang-R58472
>>>> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
>>>> initialization code
>>>> 
>>>> 
>>>> On Jul 27, 2012, at 3:24 PM, Scott Wood wrote:
>>>> 
>>>>> On 07/27/2012 05:10 AM, Jia Hongtao-B38951 wrote:
>>>>>> Hi kumar,
>>>>>> 
>>>>>> I know "duplicate code from pci_process_bridge_OF_ranges()" is
>>>>>> hard to accept but "refactor the code to have a shared function"
>>>>>> is knotty. Actually this is the reason I didn't do the refactor.
>>>>> 
>>>>> Maybe we should keep doing the init early?  We could still have a
>>>>> platform device for the PM stuff, but some init would be done before
>>>> probe.
>>>>> 
>>>>> Another possibility is to try to handle swiotlb init later -- possibly
>>>>> by reserving memory for it if the platform indicates it's a possibility
>>>>> that it will be needed, then freeing the memory if it's not needed.
>>>>> 
>>>>> -Scott
>>>> 
>>>> I think the first option seems reasonable.  Can we leave fsl_pci_init()
>>>> as we now have it and just have the platform driver deal with PM restore
>>>> via calling setup_pci_atmu() [probably need to update setup_pci_atmu to
>>>> handle restore case, but seems like minor changes]
>>>> 
>>>> - k
>>>> 
>>> 
>>> 
>>> I think the second option is better if it's hard to decouple swiotlb
>>> determination from pci init. I believe the better architecture that
>>> PCI init in probe function of platform driver will bring us considerable
>>> advantage. I really like to keep the completion of pci controller
>>> platform driver not only for PM support but also for pci init.
>>> 
>>> -Hongtao.
>>> 
>> 
>> Shifting of swiotlb init has a lot more issues.  Why do we need to do the PCI init in probe?
> 
> The ordering issues are introduced by swiotlb.  And the ideal way is
> to solve the problem within swiotlb instead of changing PCI to
> workaround it.  Take the implementation of x86 as reference it's
> possible to be addressed bu allocating first and free later approach.
> 
> It is common sense that the initialization of a device is in the probe
> function of the driver of the device.  And the change will provide
> better unification of PCI controller code.  The PCI controller is
> generic enough not to be taken care of at the platform area.
> 
> Leo

Than lets look at going with that approach.. Be careful with impact on other users of swiotlb on PPC, I believe one 44x board uses swiotlb.

- k

> -----Original Message-----
> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> Sent: Tuesday, July 31, 2012 9:38 PM
> To: Li Yang-R58472
> Cc: Jia Hongtao-B38951; Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> Li Yang-R58472
> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
> initialization code
> 
> 
> On Jul 31, 2012, at 2:21 AM, Li Yang wrote:
> 
> > On Mon, Jul 30, 2012 at 10:46 PM, Kumar Gala <galak@kernel.crashing.org>
> wrote:
> >>
> >> On Jul 30, 2012, at 3:26 AM, Jia Hongtao-B38951 wrote:
> >>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> >>>> Sent: Saturday, July 28, 2012 5:17 AM
> >>>> To: Wood Scott-B07421
> >>>> Cc: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org; Wood Scott-
> B07421;
> >>>> Li Yang-R58472
> >>>> Subject: Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie
> >>>> initialization code
> >>>>
> >>>>
> >>>> On Jul 27, 2012, at 3:24 PM, Scott Wood wrote:
> >>>>
> >>>>> On 07/27/2012 05:10 AM, Jia Hongtao-B38951 wrote:
> >>>>>> Hi kumar,
> >>>>>>
> >>>>>> I know "duplicate code from pci_process_bridge_OF_ranges()" is
> >>>>>> hard to accept but "refactor the code to have a shared function"
> >>>>>> is knotty. Actually this is the reason I didn't do the refactor.
> >>>>>
> >>>>> Maybe we should keep doing the init early?  We could still have a
> >>>>> platform device for the PM stuff, but some init would be done
> before
> >>>> probe.
> >>>>>
> >>>>> Another possibility is to try to handle swiotlb init later --
> possibly
> >>>>> by reserving memory for it if the platform indicates it's a
> possibility
> >>>>> that it will be needed, then freeing the memory if it's not needed.
> >>>>>
> >>>>> -Scott
> >>>>
> >>>> I think the first option seems reasonable.  Can we leave
> fsl_pci_init()
> >>>> as we now have it and just have the platform driver deal with PM
> restore
> >>>> via calling setup_pci_atmu() [probably need to update setup_pci_atmu
> to
> >>>> handle restore case, but seems like minor changes]
> >>>>
> >>>> - k
> >>>>
> >>>
> >>>
> >>> I think the second option is better if it's hard to decouple swiotlb
> >>> determination from pci init. I believe the better architecture that
> >>> PCI init in probe function of platform driver will bring us
> considerable
> >>> advantage. I really like to keep the completion of pci controller
> >>> platform driver not only for PM support but also for pci init.
> >>>
> >>> -Hongtao.
> >>>
> >>
> >> Shifting of swiotlb init has a lot more issues.  Why do we need to do
> the PCI init in probe?
> >
> > The ordering issues are introduced by swiotlb.  And the ideal way is
> > to solve the problem within swiotlb instead of changing PCI to
> > workaround it.  Take the implementation of x86 as reference it's
> > possible to be addressed bu allocating first and free later approach.
> >
> > It is common sense that the initialization of a device is in the probe
> > function of the driver of the device.  And the change will provide
> > better unification of PCI controller code.  The PCI controller is
> > generic enough not to be taken care of at the platform area.
> >
> > Leo
> 
> Than lets look at going with that approach.. Be careful with impact on
> other users of swiotlb on PPC, I believe one 44x board uses swiotlb.
> 
> - k

I will be careful with this approach.
I have already noticed 44x. Thank you all the same.

-Hongtao.

>
>
> On Jul 26, 2012, at 7:30 AM, Jia Hongtao wrote:
>
> > We unified the Freescale pci/pcie initialization by changing the fsl_pci
> > to a platform driver. In previous PCI code architecture the initialization
> > routine is called at board_setup_arch stage. Now the initialization is done
> > in probe function which is architectural better. Also It's convenient for
> > adding PM support for PCI controller in later patch.
> >
> > One issue introduced by this architecture is the timing of swiotlb_init.
> > During PCI initialization the need of swiotlb is determined and this should
> > be done before swiotlb_init. So a new function to determine swiotlb by
> > parsing pci ranges is made. This function is called at board_setup_arch
> > stage which is earlier than swiotlb_init.
> >
> > Signed-off-by: Jia Hongtao <B38951@freescale.com>
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > ---
> > Changed for V3:
> > - Rebase the patch set on the latest tree
> > - merge PCI unify and swiotlb patch into one
> >
> > arch/powerpc/sysdev/fsl_pci.c |  155 ++++++++++++++++++++++++++++++++---------
> > arch/powerpc/sysdev/fsl_pci.h |    9 +--
> > 2 files changed, 125 insertions(+), 39 deletions(-)
>
> I'd like the SWIOTLB refactoring as a separate patch.  Additionally, the order of patches should be as follows:
>
> 1. refactor PCI node parsing code
> 2. add pci_determine_swiotlb (should rename to fsl_pci_determine_swiotlb)
> 3. Determine primary bus by looking for ISA node
> 4. convert all boards over to fsl_pci_init
> 5. convert fsl pci to platform driver (edac and other fixes should be merged in here)
> 6. PM support

Could you add:
7. Fix PPC_INDIRECT_TYPE_NO_PCIE_LINK issue. The link is only probed at init and a subsequent
pci/rescan does not change that. See mail thread titled:
  mpc8xxx PCIe hotplug needs fixing, some clues ..

 Jocke