Patchwork [0/6] Description for PCI patches using platform driver

login
register
mail settings
Submitter Bharat Bhushan
Date June 11, 2012, 1:24 p.m.
Message ID <6A3DF150A5B70D4F9B66A25E3F7C888D03D78FAE@039-SN2MPN1-022.039d.mgd.msft.net>
Download mbox | patch
Permalink /patch/164133/
State Not Applicable
Headers show

Comments

Bharat Bhushan - June 11, 2012, 1:24 p.m.
> -----Original Message-----
> From: Jia Hongtao-B38951
> Sent: Monday, June 11, 2012 8:03 AM
> To: Bhushan Bharat-R65777; linuxppc-dev@lists.ozlabs.org;
> galak@kernel.crashing.org
> Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-B07421
> Subject: RE: [PATCH 0/6] Description for PCI patches using platform driver
> 
> > -----Original Message-----
> > From: Bhushan Bharat-R65777
> > Sent: Friday, June 08, 2012 6:47 PM
> > To: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org;
> > galak@kernel.crashing.org
> > Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-B07421
> > Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> > driver
> >
> >
> > > -----Original Message-----
> > > From: Jia Hongtao-B38951
> > > Sent: Friday, June 08, 2012 3:12 PM
> > > To: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org
> > > Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-B07421;
> > Bhushan Bharat-
> > > R65777; Jia Hongtao-B38951
> > > Subject: [PATCH 0/6] Description for PCI patches using platform
> > > driver
> > >
> > > This series of patches are to unify pci initialization code and add
> > > PM
> > support
> > > for all 85xx/86xx powerpc boards. But two side effects are
> > > introduced
> > by this
> > > mechanism which listed below:
> > >
> > > 1. of_platform_bus_probe() will be called twice but in some cases
> > duplication
> > >    warning occured. We fix this in [PATCH 5/6].
> > >
> > > 2. Edac driver failed to register pci nodes as platform devices. We
> > > fix
> > this
> > >    in [PATCH 6/6].
> >
> > With these patches will not the SWIOTLB will not be initialized even
> > if PCI/PCIe demanded?
> >
> > Thanks
> > -Bharat
> >
> 
> These patches still have the swiotlb init problem if "ppc_swiotlb_enable" is
> only demanded by PCI/PCIe. One of the purposes of sending out these patches is
> to let us start a discussion for this problem in upstream.

Ok, I did not find any mention of that, so I thought that you have resolved the issue by some means in these patches which I did not catch.

So, these patches introduces the issue, that SWIOTLB will not be initialized if requested by pci/pcie. The request is raised by setting the flag ppc_swiotlb_enable. The swiotlb_init() will be called in mem_init() if ppc_swiotlb_enable is set. Now with these patches, the request is raised after mem_init() is called. So request not handled :).

Following are the solutions we have thought of during our internal discussions (if I did not missed any):

1. These patches move the code from platform init to device init (arch_initcall()). Rather than moving the whole code, let us divide the code into two. First, which is needed to raise the swiotlb init request and second the rest. Define this first as an function in arch/powerpc/sysdev/fsl_pci.c and call this from platform init code of the SOCs.

2. All known devices, the lowest PCIe outbound range starts at 0x80000000, but there's nothing above 0xc0000000. So the inbound of size 0x8000_0000 is always availbe on all devices. Hardcode the check in platform code to check memblock_end_of_DRAM() to 0x80000000.

Something like this:

3. Always do swiotlb_init() in mem_init() and later after PCI init, if the swiotlb is not needed then free it (swiotlb_free()). 

4. etc, please provide some other better way.

Thanks
-Bharat
Hongtao Jia - June 12, 2012, 2:24 a.m.
> -----Original Message-----
> From: Bhushan Bharat-R65777
> Sent: Monday, June 11, 2012 9:25 PM
> To: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org;
> galak@kernel.crashing.org
> Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-B07421
> Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> driver
> 
> 
> 
> > -----Original Message-----
> > From: Jia Hongtao-B38951
> > Sent: Monday, June 11, 2012 8:03 AM
> > To: Bhushan Bharat-R65777; linuxppc-dev@lists.ozlabs.org;
> > galak@kernel.crashing.org
> > Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-B07421
> > Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> driver
> >
> > > -----Original Message-----
> > > From: Bhushan Bharat-R65777
> > > Sent: Friday, June 08, 2012 6:47 PM
> > > To: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org;
> > > galak@kernel.crashing.org
> > > Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-B07421
> > > Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> > > driver
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jia Hongtao-B38951
> > > > Sent: Friday, June 08, 2012 3:12 PM
> > > > To: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org
> > > > Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-B07421;
> > > Bhushan Bharat-
> > > > R65777; Jia Hongtao-B38951
> > > > Subject: [PATCH 0/6] Description for PCI patches using platform
> > > > driver
> > > >
> > > > This series of patches are to unify pci initialization code and add
> > > > PM
> > > support
> > > > for all 85xx/86xx powerpc boards. But two side effects are
> > > > introduced
> > > by this
> > > > mechanism which listed below:
> > > >
> > > > 1. of_platform_bus_probe() will be called twice but in some cases
> > > duplication
> > > >    warning occured. We fix this in [PATCH 5/6].
> > > >
> > > > 2. Edac driver failed to register pci nodes as platform devices. We
> > > > fix
> > > this
> > > >    in [PATCH 6/6].
> > >
> > > With these patches will not the SWIOTLB will not be initialized even
> > > if PCI/PCIe demanded?
> > >
> > > Thanks
> > > -Bharat
> > >
> >
> > These patches still have the swiotlb init problem if
> "ppc_swiotlb_enable" is
> > only demanded by PCI/PCIe. One of the purposes of sending out these
> patches is
> > to let us start a discussion for this problem in upstream.
> 
> Ok, I did not find any mention of that, so I thought that you have
> resolved the issue by some means in these patches which I did not catch.
> 
> So, these patches introduces the issue, that SWIOTLB will not be
> initialized if requested by pci/pcie. The request is raised by setting
> the flag ppc_swiotlb_enable. The swiotlb_init() will be called in
> mem_init() if ppc_swiotlb_enable is set. Now with these patches, the
> request is raised after mem_init() is called. So request not handled :).
> 
> Following are the solutions we have thought of during our internal
> discussions (if I did not missed any):
> 
> 1. These patches move the code from platform init to device init
> (arch_initcall()). Rather than moving the whole code, let us divide the
> code into two. First, which is needed to raise the swiotlb init request
> and second the rest. Define this first as an function in
> arch/powerpc/sysdev/fsl_pci.c and call this from platform init code of
> the SOCs.
> 
> 2. All known devices, the lowest PCIe outbound range starts at 0x80000000,
> but there's nothing above 0xc0000000. So the inbound of size 0x8000_0000
> is always availbe on all devices. Hardcode the check in platform code to
> check memblock_end_of_DRAM() to 0x80000000.
> 
> Something like this:
> 
> diff --git a/arch/powerpc/platforms/85xx/corenet_ds.c
> b/arch/powerpc/platforms/85xx/corenet_ds.c
> index 1f7028e..ef4e215 100644
> --- a/arch/powerpc/platforms/85xx/corenet_ds.c
> +++ b/arch/powerpc/platforms/85xx/corenet_ds.c
> @@ -79,7 +79,7 @@ void __init corenet_ds_setup_arch(void)  #endif
> 
> #ifdef CONFIG_SWIOTLB
> -       if (memblock_end_of_DRAM() > 0xffffffff)
> +       if (memblock_end_of_DRAM() > 0xff000000)
>                  ppc_swiotlb_enable = 1;  #endif
>          pr_info("%s board from Freescale Semiconductor\n", ppc_md.name);
> 
> -------------
> 
> 3. Always do swiotlb_init() in mem_init() and later after PCI init, if
> the swiotlb is not needed then free it (swiotlb_free()).
> 
> 4. etc, please provide some other better way.
> 
> Thanks
> -Bharat

Thanks.
In my point of view the 2nd solution is better for it does not treat PCI/PCIe as
the special kind of devices from others.

-Jia Hongtao.
Bharat Bhushan - June 14, 2012, 9:52 a.m.
Hello Ben, Kumar, others

Please provide your comments/thoughts on this ?

Thanks
-Bharat

> > > >
> > > > > -----Original Message-----
> > > > > From: Jia Hongtao-B38951
> > > > > Sent: Friday, June 08, 2012 3:12 PM
> > > > > To: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org
> > > > > Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-B07421;
> > > > Bhushan Bharat-
> > > > > R65777; Jia Hongtao-B38951
> > > > > Subject: [PATCH 0/6] Description for PCI patches using platform
> > > > > driver
> > > > >
> > > > > This series of patches are to unify pci initialization code and
> > > > > add PM
> > > > support
> > > > > for all 85xx/86xx powerpc boards. But two side effects are
> > > > > introduced
> > > > by this
> > > > > mechanism which listed below:
> > > > >
> > > > > 1. of_platform_bus_probe() will be called twice but in some
> > > > > cases
> > > > duplication
> > > > >    warning occured. We fix this in [PATCH 5/6].
> > > > >
> > > > > 2. Edac driver failed to register pci nodes as platform devices.
> > > > > We fix
> > > > this
> > > > >    in [PATCH 6/6].
> > > >
> > > > With these patches will not the SWIOTLB will not be initialized
> > > > even if PCI/PCIe demanded?
> > > >
> > > > Thanks
> > > > -Bharat
> > > >
> > >
> > > These patches still have the swiotlb init problem if
> > "ppc_swiotlb_enable" is
> > > only demanded by PCI/PCIe. One of the purposes of sending out these
> > patches is
> > > to let us start a discussion for this problem in upstream.
> >
> > Ok, I did not find any mention of that, so I thought that you have
> > resolved the issue by some means in these patches which I did not catch.
> >
> > So, these patches introduces the issue, that SWIOTLB will not be
> > initialized if requested by pci/pcie. The request is raised by setting
> > the flag ppc_swiotlb_enable. The swiotlb_init() will be called in
> > mem_init() if ppc_swiotlb_enable is set. Now with these patches, the
> > request is raised after mem_init() is called. So request not handled :).
> >
> > Following are the solutions we have thought of during our internal
> > discussions (if I did not missed any):
> >
> > 1. These patches move the code from platform init to device init
> > (arch_initcall()). Rather than moving the whole code, let us divide
> > the code into two. First, which is needed to raise the swiotlb init
> > request and second the rest. Define this first as an function in
> > arch/powerpc/sysdev/fsl_pci.c and call this from platform init code of
> > the SOCs.
> >
> > 2. All known devices, the lowest PCIe outbound range starts at
> > 0x80000000, but there's nothing above 0xc0000000. So the inbound of
> > size 0x8000_0000 is always availbe on all devices. Hardcode the check
> > in platform code to check memblock_end_of_DRAM() to 0x80000000.
> >
> > Something like this:
> >
> > diff --git a/arch/powerpc/platforms/85xx/corenet_ds.c
> > b/arch/powerpc/platforms/85xx/corenet_ds.c
> > index 1f7028e..ef4e215 100644
> > --- a/arch/powerpc/platforms/85xx/corenet_ds.c
> > +++ b/arch/powerpc/platforms/85xx/corenet_ds.c
> > @@ -79,7 +79,7 @@ void __init corenet_ds_setup_arch(void)  #endif
> >
> > #ifdef CONFIG_SWIOTLB
> > -       if (memblock_end_of_DRAM() > 0xffffffff)
> > +       if (memblock_end_of_DRAM() > 0xff000000)
> >                  ppc_swiotlb_enable = 1;  #endif
> >          pr_info("%s board from Freescale Semiconductor\n",
> > ppc_md.name);
> >
> > -------------
> >
> > 3. Always do swiotlb_init() in mem_init() and later after PCI init, if
> > the swiotlb is not needed then free it (swiotlb_free()).
> >
> > 4. etc, please provide some other better way.
> >
> > Thanks
> > -Bharat
> 
> Thanks.
> In my point of view the 2nd solution is better for it does not treat PCI/PCIe as
> the special kind of devices from others.
> 
> -Jia Hongtao.
Hongtao Jia - June 20, 2012, 2:33 a.m.
Hello Ben, Kumar, others:

This series of patches had been pending for a long time on upstream.
We fixed some issues we found and there still some issues should be
discussed like swiotlb init thing. Do you have time for a review?

Thanks.
-Jia Hongtao.

> -----Original Message-----
> From: Bhushan Bharat-R65777
> Sent: Thursday, June 14, 2012 5:52 PM
> To: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org;
> galak@kernel.crashing.org; benh@kernel.crashing.org
> Cc: Li Yang-R58472; Wood Scott-B07421
> Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> driver
> 
> Hello Ben, Kumar, others
> 
> Please provide your comments/thoughts on this ?
> 
> Thanks
> -Bharat
> 
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jia Hongtao-B38951
> > > > > > Sent: Friday, June 08, 2012 3:12 PM
> > > > > > To: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org
> > > > > > Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-B07421;
> > > > > Bhushan Bharat-
> > > > > > R65777; Jia Hongtao-B38951
> > > > > > Subject: [PATCH 0/6] Description for PCI patches using platform
> > > > > > driver
> > > > > >
> > > > > > This series of patches are to unify pci initialization code and
> > > > > > add PM
> > > > > support
> > > > > > for all 85xx/86xx powerpc boards. But two side effects are
> > > > > > introduced
> > > > > by this
> > > > > > mechanism which listed below:
> > > > > >
> > > > > > 1. of_platform_bus_probe() will be called twice but in some
> > > > > > cases
> > > > > duplication
> > > > > >    warning occured. We fix this in [PATCH 5/6].
> > > > > >
> > > > > > 2. Edac driver failed to register pci nodes as platform devices.
> > > > > > We fix
> > > > > this
> > > > > >    in [PATCH 6/6].
> > > > >
> > > > > With these patches will not the SWIOTLB will not be initialized
> > > > > even if PCI/PCIe demanded?
> > > > >
> > > > > Thanks
> > > > > -Bharat
> > > > >
> > > >
> > > > These patches still have the swiotlb init problem if
> > > "ppc_swiotlb_enable" is
> > > > only demanded by PCI/PCIe. One of the purposes of sending out these
> > > patches is
> > > > to let us start a discussion for this problem in upstream.
> > >
> > > Ok, I did not find any mention of that, so I thought that you have
> > > resolved the issue by some means in these patches which I did not
> catch.
> > >
> > > So, these patches introduces the issue, that SWIOTLB will not be
> > > initialized if requested by pci/pcie. The request is raised by
> setting
> > > the flag ppc_swiotlb_enable. The swiotlb_init() will be called in
> > > mem_init() if ppc_swiotlb_enable is set. Now with these patches, the
> > > request is raised after mem_init() is called. So request not
> handled :).
> > >
> > > Following are the solutions we have thought of during our internal
> > > discussions (if I did not missed any):
> > >
> > > 1. These patches move the code from platform init to device init
> > > (arch_initcall()). Rather than moving the whole code, let us divide
> > > the code into two. First, which is needed to raise the swiotlb init
> > > request and second the rest. Define this first as an function in
> > > arch/powerpc/sysdev/fsl_pci.c and call this from platform init code
> of
> > > the SOCs.
> > >
> > > 2. All known devices, the lowest PCIe outbound range starts at
> > > 0x80000000, but there's nothing above 0xc0000000. So the inbound of
> > > size 0x8000_0000 is always availbe on all devices. Hardcode the check
> > > in platform code to check memblock_end_of_DRAM() to 0x80000000.
> > >
> > > Something like this:
> > >
> > > diff --git a/arch/powerpc/platforms/85xx/corenet_ds.c
> > > b/arch/powerpc/platforms/85xx/corenet_ds.c
> > > index 1f7028e..ef4e215 100644
> > > --- a/arch/powerpc/platforms/85xx/corenet_ds.c
> > > +++ b/arch/powerpc/platforms/85xx/corenet_ds.c
> > > @@ -79,7 +79,7 @@ void __init corenet_ds_setup_arch(void)  #endif
> > >
> > > #ifdef CONFIG_SWIOTLB
> > > -       if (memblock_end_of_DRAM() > 0xffffffff)
> > > +       if (memblock_end_of_DRAM() > 0xff000000)
> > >                  ppc_swiotlb_enable = 1;  #endif
> > >          pr_info("%s board from Freescale Semiconductor\n",
> > > ppc_md.name);
> > >
> > > -------------
> > >
> > > 3. Always do swiotlb_init() in mem_init() and later after PCI init,
> if
> > > the swiotlb is not needed then free it (swiotlb_free()).
> > >
> > > 4. etc, please provide some other better way.
> > >
> > > Thanks
> > > -Bharat
> >
> > Thanks.
> > In my point of view the 2nd solution is better for it does not treat
> PCI/PCIe as
> > the special kind of devices from others.
> >
> > -Jia Hongtao.
Hongtao Jia - June 26, 2012, 2:33 a.m.
Hello Ben and Kumar,

Do you have any concerns or comments on these series of patches?
Would you please have a review?

Thanks.
-Jia Hongtao.

> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+b38951=freescale.com@lists.ozlabs.org] On Behalf Of Jia Hongtao-
> B38951
> Sent: Wednesday, June 20, 2012 10:34 AM
> To: Bhushan Bharat-R65777; linuxppc-dev@lists.ozlabs.org;
> galak@kernel.crashing.org; benh@kernel.crashing.org
> Cc: Wood Scott-B07421; Li Yang-R58472
> Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> driver
> 
> Hello Ben, Kumar, others:
> 
> This series of patches had been pending for a long time on upstream.
> We fixed some issues we found and there still some issues should be
> discussed like swiotlb init thing. Do you have time for a review?
> 
> Thanks.
> -Jia Hongtao.
> 
> > -----Original Message-----
> > From: Bhushan Bharat-R65777
> > Sent: Thursday, June 14, 2012 5:52 PM
> > To: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org;
> > galak@kernel.crashing.org; benh@kernel.crashing.org
> > Cc: Li Yang-R58472; Wood Scott-B07421
> > Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> > driver
> >
> > Hello Ben, Kumar, others
> >
> > Please provide your comments/thoughts on this ?
> >
> > Thanks
> > -Bharat
> >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jia Hongtao-B38951
> > > > > > > Sent: Friday, June 08, 2012 3:12 PM
> > > > > > > To: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org
> > > > > > > Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-
> B07421;
> > > > > > Bhushan Bharat-
> > > > > > > R65777; Jia Hongtao-B38951
> > > > > > > Subject: [PATCH 0/6] Description for PCI patches using
> platform
> > > > > > > driver
> > > > > > >
> > > > > > > This series of patches are to unify pci initialization code
> and
> > > > > > > add PM
> > > > > > support
> > > > > > > for all 85xx/86xx powerpc boards. But two side effects are
> > > > > > > introduced
> > > > > > by this
> > > > > > > mechanism which listed below:
> > > > > > >
> > > > > > > 1. of_platform_bus_probe() will be called twice but in some
> > > > > > > cases
> > > > > > duplication
> > > > > > >    warning occured. We fix this in [PATCH 5/6].
> > > > > > >
> > > > > > > 2. Edac driver failed to register pci nodes as platform
> devices.
> > > > > > > We fix
> > > > > > this
> > > > > > >    in [PATCH 6/6].
> > > > > >
> > > > > > With these patches will not the SWIOTLB will not be initialized
> > > > > > even if PCI/PCIe demanded?
> > > > > >
> > > > > > Thanks
> > > > > > -Bharat
> > > > > >
> > > > >
> > > > > These patches still have the swiotlb init problem if
> > > > "ppc_swiotlb_enable" is
> > > > > only demanded by PCI/PCIe. One of the purposes of sending out
> these
> > > > patches is
> > > > > to let us start a discussion for this problem in upstream.
> > > >
> > > > Ok, I did not find any mention of that, so I thought that you have
> > > > resolved the issue by some means in these patches which I did not
> > catch.
> > > >
> > > > So, these patches introduces the issue, that SWIOTLB will not be
> > > > initialized if requested by pci/pcie. The request is raised by
> > setting
> > > > the flag ppc_swiotlb_enable. The swiotlb_init() will be called in
> > > > mem_init() if ppc_swiotlb_enable is set. Now with these patches,
> the
> > > > request is raised after mem_init() is called. So request not
> > handled :).
> > > >
> > > > Following are the solutions we have thought of during our internal
> > > > discussions (if I did not missed any):
> > > >
> > > > 1. These patches move the code from platform init to device init
> > > > (arch_initcall()). Rather than moving the whole code, let us divide
> > > > the code into two. First, which is needed to raise the swiotlb init
> > > > request and second the rest. Define this first as an function in
> > > > arch/powerpc/sysdev/fsl_pci.c and call this from platform init code
> > of
> > > > the SOCs.
> > > >
> > > > 2. All known devices, the lowest PCIe outbound range starts at
> > > > 0x80000000, but there's nothing above 0xc0000000. So the inbound of
> > > > size 0x8000_0000 is always availbe on all devices. Hardcode the
> check
> > > > in platform code to check memblock_end_of_DRAM() to 0x80000000.
> > > >
> > > > Something like this:
> > > >
> > > > diff --git a/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > b/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > index 1f7028e..ef4e215 100644
> > > > --- a/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > +++ b/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > @@ -79,7 +79,7 @@ void __init corenet_ds_setup_arch(void)  #endif
> > > >
> > > > #ifdef CONFIG_SWIOTLB
> > > > -       if (memblock_end_of_DRAM() > 0xffffffff)
> > > > +       if (memblock_end_of_DRAM() > 0xff000000)
> > > >                  ppc_swiotlb_enable = 1;  #endif
> > > >          pr_info("%s board from Freescale Semiconductor\n",
> > > > ppc_md.name);
> > > >
> > > > -------------
> > > >
> > > > 3. Always do swiotlb_init() in mem_init() and later after PCI init,
> > if
> > > > the swiotlb is not needed then free it (swiotlb_free()).
> > > >
> > > > 4. etc, please provide some other better way.
> > > >
> > > > Thanks
> > > > -Bharat
> > >
> > > Thanks.
> > > In my point of view the 2nd solution is better for it does not treat
> > PCI/PCIe as
> > > the special kind of devices from others.
> > >
> > > -Jia Hongtao.
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
Benjamin Herrenschmidt - June 26, 2012, 2:46 a.m.
On Tue, 2012-06-26 at 02:33 +0000, Jia Hongtao-B38951 wrote:
> Hello Ben and Kumar,
> 
> Do you have any concerns or comments on these series of patches?
> Would you please have a review?

My main concern is that currently the PCI code has some assumptions
about ordering of things that will get violated.

For example, the pci final fixups are an fs_initcall iirc, or something
like that. There's other similar oddities that might become problematic.
In addition, there might be locking issues if you start doing multiple
PHBs in parallel.

Now we do want to fix all that long run but it might take a while.

Cheers,
Ben.

> Thanks.
> -Jia Hongtao.
> 
> > -----Original Message-----
> > From: Linuxppc-dev [mailto:linuxppc-dev-
> > bounces+b38951=freescale.com@lists.ozlabs.org] On Behalf Of Jia Hongtao-
> > B38951
> > Sent: Wednesday, June 20, 2012 10:34 AM
> > To: Bhushan Bharat-R65777; linuxppc-dev@lists.ozlabs.org;
> > galak@kernel.crashing.org; benh@kernel.crashing.org
> > Cc: Wood Scott-B07421; Li Yang-R58472
> > Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> > driver
> > 
> > Hello Ben, Kumar, others:
> > 
> > This series of patches had been pending for a long time on upstream.
> > We fixed some issues we found and there still some issues should be
> > discussed like swiotlb init thing. Do you have time for a review?
> > 
> > Thanks.
> > -Jia Hongtao.
> > 
> > > -----Original Message-----
> > > From: Bhushan Bharat-R65777
> > > Sent: Thursday, June 14, 2012 5:52 PM
> > > To: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org;
> > > galak@kernel.crashing.org; benh@kernel.crashing.org
> > > Cc: Li Yang-R58472; Wood Scott-B07421
> > > Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> > > driver
> > >
> > > Hello Ben, Kumar, others
> > >
> > > Please provide your comments/thoughts on this ?
> > >
> > > Thanks
> > > -Bharat
> > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Jia Hongtao-B38951
> > > > > > > > Sent: Friday, June 08, 2012 3:12 PM
> > > > > > > > To: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org
> > > > > > > > Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-
> > B07421;
> > > > > > > Bhushan Bharat-
> > > > > > > > R65777; Jia Hongtao-B38951
> > > > > > > > Subject: [PATCH 0/6] Description for PCI patches using
> > platform
> > > > > > > > driver
> > > > > > > >
> > > > > > > > This series of patches are to unify pci initialization code
> > and
> > > > > > > > add PM
> > > > > > > support
> > > > > > > > for all 85xx/86xx powerpc boards. But two side effects are
> > > > > > > > introduced
> > > > > > > by this
> > > > > > > > mechanism which listed below:
> > > > > > > >
> > > > > > > > 1. of_platform_bus_probe() will be called twice but in some
> > > > > > > > cases
> > > > > > > duplication
> > > > > > > >    warning occured. We fix this in [PATCH 5/6].
> > > > > > > >
> > > > > > > > 2. Edac driver failed to register pci nodes as platform
> > devices.
> > > > > > > > We fix
> > > > > > > this
> > > > > > > >    in [PATCH 6/6].
> > > > > > >
> > > > > > > With these patches will not the SWIOTLB will not be initialized
> > > > > > > even if PCI/PCIe demanded?
> > > > > > >
> > > > > > > Thanks
> > > > > > > -Bharat
> > > > > > >
> > > > > >
> > > > > > These patches still have the swiotlb init problem if
> > > > > "ppc_swiotlb_enable" is
> > > > > > only demanded by PCI/PCIe. One of the purposes of sending out
> > these
> > > > > patches is
> > > > > > to let us start a discussion for this problem in upstream.
> > > > >
> > > > > Ok, I did not find any mention of that, so I thought that you have
> > > > > resolved the issue by some means in these patches which I did not
> > > catch.
> > > > >
> > > > > So, these patches introduces the issue, that SWIOTLB will not be
> > > > > initialized if requested by pci/pcie. The request is raised by
> > > setting
> > > > > the flag ppc_swiotlb_enable. The swiotlb_init() will be called in
> > > > > mem_init() if ppc_swiotlb_enable is set. Now with these patches,
> > the
> > > > > request is raised after mem_init() is called. So request not
> > > handled :).
> > > > >
> > > > > Following are the solutions we have thought of during our internal
> > > > > discussions (if I did not missed any):
> > > > >
> > > > > 1. These patches move the code from platform init to device init
> > > > > (arch_initcall()). Rather than moving the whole code, let us divide
> > > > > the code into two. First, which is needed to raise the swiotlb init
> > > > > request and second the rest. Define this first as an function in
> > > > > arch/powerpc/sysdev/fsl_pci.c and call this from platform init code
> > > of
> > > > > the SOCs.
> > > > >
> > > > > 2. All known devices, the lowest PCIe outbound range starts at
> > > > > 0x80000000, but there's nothing above 0xc0000000. So the inbound of
> > > > > size 0x8000_0000 is always availbe on all devices. Hardcode the
> > check
> > > > > in platform code to check memblock_end_of_DRAM() to 0x80000000.
> > > > >
> > > > > Something like this:
> > > > >
> > > > > diff --git a/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > > b/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > > index 1f7028e..ef4e215 100644
> > > > > --- a/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > > +++ b/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > > @@ -79,7 +79,7 @@ void __init corenet_ds_setup_arch(void)  #endif
> > > > >
> > > > > #ifdef CONFIG_SWIOTLB
> > > > > -       if (memblock_end_of_DRAM() > 0xffffffff)
> > > > > +       if (memblock_end_of_DRAM() > 0xff000000)
> > > > >                  ppc_swiotlb_enable = 1;  #endif
> > > > >          pr_info("%s board from Freescale Semiconductor\n",
> > > > > ppc_md.name);
> > > > >
> > > > > -------------
> > > > >
> > > > > 3. Always do swiotlb_init() in mem_init() and later after PCI init,
> > > if
> > > > > the swiotlb is not needed then free it (swiotlb_free()).
> > > > >
> > > > > 4. etc, please provide some other better way.
> > > > >
> > > > > Thanks
> > > > > -Bharat
> > > >
> > > > Thanks.
> > > > In my point of view the 2nd solution is better for it does not treat
> > > PCI/PCIe as
> > > > the special kind of devices from others.
> > > >
> > > > -Jia Hongtao.
> > 
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
>
Hongtao Jia - June 26, 2012, 2:54 a.m.
> My main concern is that currently the PCI code has some assumptions
> about ordering of things that will get violated.
> 
> For example, the pci final fixups are an fs_initcall iirc, or something
> like that. There's other similar oddities that might become problematic.
> In addition, there might be locking issues if you start doing multiple
> PHBs in parallel.
> 
> Now we do want to fix all that long run but it might take a while.
> 
> Cheers,
> Ben.
> 

Thanks for your advice. I will do some investigations on what you said.

-Jia Hongtao.



> > Thanks.
> > -Jia Hongtao.
> >
> > > -----Original Message-----
> > > From: Linuxppc-dev [mailto:linuxppc-dev-
> > > bounces+b38951=freescale.com@lists.ozlabs.org] On Behalf Of Jia
> Hongtao-
> > > B38951
> > > Sent: Wednesday, June 20, 2012 10:34 AM
> > > To: Bhushan Bharat-R65777; linuxppc-dev@lists.ozlabs.org;
> > > galak@kernel.crashing.org; benh@kernel.crashing.org
> > > Cc: Wood Scott-B07421; Li Yang-R58472
> > > Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> > > driver
> > >
> > > Hello Ben, Kumar, others:
> > >
> > > This series of patches had been pending for a long time on upstream.
> > > We fixed some issues we found and there still some issues should be
> > > discussed like swiotlb init thing. Do you have time for a review?
> > >
> > > Thanks.
> > > -Jia Hongtao.
> > >
> > > > -----Original Message-----
> > > > From: Bhushan Bharat-R65777
> > > > Sent: Thursday, June 14, 2012 5:52 PM
> > > > To: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org;
> > > > galak@kernel.crashing.org; benh@kernel.crashing.org
> > > > Cc: Li Yang-R58472; Wood Scott-B07421
> > > > Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> > > > driver
> > > >
> > > > Hello Ben, Kumar, others
> > > >
> > > > Please provide your comments/thoughts on this ?
> > > >
> > > > Thanks
> > > > -Bharat
> > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Jia Hongtao-B38951
> > > > > > > > > Sent: Friday, June 08, 2012 3:12 PM
> > > > > > > > > To: linuxppc-dev@lists.ozlabs.org;
> galak@kernel.crashing.org
> > > > > > > > > Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-
> > > B07421;
> > > > > > > > Bhushan Bharat-
> > > > > > > > > R65777; Jia Hongtao-B38951
> > > > > > > > > Subject: [PATCH 0/6] Description for PCI patches using
> > > platform
> > > > > > > > > driver
> > > > > > > > >
> > > > > > > > > This series of patches are to unify pci initialization
> code
> > > and
> > > > > > > > > add PM
> > > > > > > > support
> > > > > > > > > for all 85xx/86xx powerpc boards. But two side effects
> are
> > > > > > > > > introduced
> > > > > > > > by this
> > > > > > > > > mechanism which listed below:
> > > > > > > > >
> > > > > > > > > 1. of_platform_bus_probe() will be called twice but in
> some
> > > > > > > > > cases
> > > > > > > > duplication
> > > > > > > > >    warning occured. We fix this in [PATCH 5/6].
> > > > > > > > >
> > > > > > > > > 2. Edac driver failed to register pci nodes as platform
> > > devices.
> > > > > > > > > We fix
> > > > > > > > this
> > > > > > > > >    in [PATCH 6/6].
> > > > > > > >
> > > > > > > > With these patches will not the SWIOTLB will not be
> initialized
> > > > > > > > even if PCI/PCIe demanded?
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > > -Bharat
> > > > > > > >
> > > > > > >
> > > > > > > These patches still have the swiotlb init problem if
> > > > > > "ppc_swiotlb_enable" is
> > > > > > > only demanded by PCI/PCIe. One of the purposes of sending out
> > > these
> > > > > > patches is
> > > > > > > to let us start a discussion for this problem in upstream.
> > > > > >
> > > > > > Ok, I did not find any mention of that, so I thought that you
> have
> > > > > > resolved the issue by some means in these patches which I did
> not
> > > > catch.
> > > > > >
> > > > > > So, these patches introduces the issue, that SWIOTLB will not
> be
> > > > > > initialized if requested by pci/pcie. The request is raised by
> > > > setting
> > > > > > the flag ppc_swiotlb_enable. The swiotlb_init() will be called
> in
> > > > > > mem_init() if ppc_swiotlb_enable is set. Now with these patches,
> > > the
> > > > > > request is raised after mem_init() is called. So request not
> > > > handled :).
> > > > > >
> > > > > > Following are the solutions we have thought of during our
> internal
> > > > > > discussions (if I did not missed any):
> > > > > >
> > > > > > 1. These patches move the code from platform init to device
> init
> > > > > > (arch_initcall()). Rather than moving the whole code, let us
> divide
> > > > > > the code into two. First, which is needed to raise the swiotlb
> init
> > > > > > request and second the rest. Define this first as an function
> in
> > > > > > arch/powerpc/sysdev/fsl_pci.c and call this from platform init
> code
> > > > of
> > > > > > the SOCs.
> > > > > >
> > > > > > 2. All known devices, the lowest PCIe outbound range starts at
> > > > > > 0x80000000, but there's nothing above 0xc0000000. So the
> inbound of
> > > > > > size 0x8000_0000 is always availbe on all devices. Hardcode the
> > > check
> > > > > > in platform code to check memblock_end_of_DRAM() to 0x80000000.
> > > > > >
> > > > > > Something like this:
> > > > > >
> > > > > > diff --git a/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > > > b/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > > > index 1f7028e..ef4e215 100644
> > > > > > --- a/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > > > +++ b/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > > > @@ -79,7 +79,7 @@ void __init corenet_ds_setup_arch(void)
> #endif
> > > > > >
> > > > > > #ifdef CONFIG_SWIOTLB
> > > > > > -       if (memblock_end_of_DRAM() > 0xffffffff)
> > > > > > +       if (memblock_end_of_DRAM() > 0xff000000)
> > > > > >                  ppc_swiotlb_enable = 1;  #endif
> > > > > >          pr_info("%s board from Freescale Semiconductor\n",
> > > > > > ppc_md.name);
> > > > > >
> > > > > > -------------
> > > > > >
> > > > > > 3. Always do swiotlb_init() in mem_init() and later after PCI
> init,
> > > > if
> > > > > > the swiotlb is not needed then free it (swiotlb_free()).
> > > > > >
> > > > > > 4. etc, please provide some other better way.
> > > > > >
> > > > > > Thanks
> > > > > > -Bharat
> > > > >
> > > > > Thanks.
> > > > > In my point of view the 2nd solution is better for it does not
> treat
> > > > PCI/PCIe as
> > > > > the special kind of devices from others.
> > > > >
> > > > > -Jia Hongtao.
> > >
> > > _______________________________________________
> > > Linuxppc-dev mailing list
> > > Linuxppc-dev@lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/linuxppc-dev
> >
> 
>
Hongtao Jia - June 26, 2012, 10:05 a.m.
> 
> My main concern is that currently the PCI code has some assumptions
> about ordering of things that will get violated.
> 
> For example, the pci final fixups are an fs_initcall iirc, or something
> like that. There's other similar oddities that might become problematic.
> In addition, there might be locking issues if you start doing multiple
> PHBs in parallel.
> 
> Now we do want to fix all that long run but it might take a while.
> 
> Cheers,
> Ben.
> 

Hi Ben,

The ordering of things is also the main concerns of our self. We also
did some researches and tried our best to make orderings right. Yes,
pci final fixups are an fs_initcall but I don't think it's a problem
cause we did not change the pci init part after this.

-Jia Hongtao.

> > Thanks.
> > -Jia Hongtao.
> >
> > > -----Original Message-----
> > > From: Linuxppc-dev [mailto:linuxppc-dev-
> > > bounces+b38951=freescale.com@lists.ozlabs.org] On Behalf Of Jia
> Hongtao-
> > > B38951
> > > Sent: Wednesday, June 20, 2012 10:34 AM
> > > To: Bhushan Bharat-R65777; linuxppc-dev@lists.ozlabs.org;
> > > galak@kernel.crashing.org; benh@kernel.crashing.org
> > > Cc: Wood Scott-B07421; Li Yang-R58472
> > > Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> > > driver
> > >
> > > Hello Ben, Kumar, others:
> > >
> > > This series of patches had been pending for a long time on upstream.
> > > We fixed some issues we found and there still some issues should be
> > > discussed like swiotlb init thing. Do you have time for a review?
> > >
> > > Thanks.
> > > -Jia Hongtao.
> > >
> > > > -----Original Message-----
> > > > From: Bhushan Bharat-R65777
> > > > Sent: Thursday, June 14, 2012 5:52 PM
> > > > To: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org;
> > > > galak@kernel.crashing.org; benh@kernel.crashing.org
> > > > Cc: Li Yang-R58472; Wood Scott-B07421
> > > > Subject: RE: [PATCH 0/6] Description for PCI patches using platform
> > > > driver
> > > >
> > > > Hello Ben, Kumar, others
> > > >
> > > > Please provide your comments/thoughts on this ?
> > > >
> > > > Thanks
> > > > -Bharat
> > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Jia Hongtao-B38951
> > > > > > > > > Sent: Friday, June 08, 2012 3:12 PM
> > > > > > > > > To: linuxppc-dev@lists.ozlabs.org;
> galak@kernel.crashing.org
> > > > > > > > > Cc: Li Yang-R58472; benh@kernel.crashing.org; Wood Scott-
> > > B07421;
> > > > > > > > Bhushan Bharat-
> > > > > > > > > R65777; Jia Hongtao-B38951
> > > > > > > > > Subject: [PATCH 0/6] Description for PCI patches using
> > > platform
> > > > > > > > > driver
> > > > > > > > >
> > > > > > > > > This series of patches are to unify pci initialization
> code
> > > and
> > > > > > > > > add PM
> > > > > > > > support
> > > > > > > > > for all 85xx/86xx powerpc boards. But two side effects
> are
> > > > > > > > > introduced
> > > > > > > > by this
> > > > > > > > > mechanism which listed below:
> > > > > > > > >
> > > > > > > > > 1. of_platform_bus_probe() will be called twice but in
> some
> > > > > > > > > cases
> > > > > > > > duplication
> > > > > > > > >    warning occured. We fix this in [PATCH 5/6].
> > > > > > > > >
> > > > > > > > > 2. Edac driver failed to register pci nodes as platform
> > > devices.
> > > > > > > > > We fix
> > > > > > > > this
> > > > > > > > >    in [PATCH 6/6].
> > > > > > > >
> > > > > > > > With these patches will not the SWIOTLB will not be
> initialized
> > > > > > > > even if PCI/PCIe demanded?
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > > -Bharat
> > > > > > > >
> > > > > > >
> > > > > > > These patches still have the swiotlb init problem if
> > > > > > "ppc_swiotlb_enable" is
> > > > > > > only demanded by PCI/PCIe. One of the purposes of sending out
> > > these
> > > > > > patches is
> > > > > > > to let us start a discussion for this problem in upstream.
> > > > > >
> > > > > > Ok, I did not find any mention of that, so I thought that you
> have
> > > > > > resolved the issue by some means in these patches which I did
> not
> > > > catch.
> > > > > >
> > > > > > So, these patches introduces the issue, that SWIOTLB will not
> be
> > > > > > initialized if requested by pci/pcie. The request is raised by
> > > > setting
> > > > > > the flag ppc_swiotlb_enable. The swiotlb_init() will be called
> in
> > > > > > mem_init() if ppc_swiotlb_enable is set. Now with these patches,
> > > the
> > > > > > request is raised after mem_init() is called. So request not
> > > > handled :).
> > > > > >
> > > > > > Following are the solutions we have thought of during our
> internal
> > > > > > discussions (if I did not missed any):
> > > > > >
> > > > > > 1. These patches move the code from platform init to device
> init
> > > > > > (arch_initcall()). Rather than moving the whole code, let us
> divide
> > > > > > the code into two. First, which is needed to raise the swiotlb
> init
> > > > > > request and second the rest. Define this first as an function
> in
> > > > > > arch/powerpc/sysdev/fsl_pci.c and call this from platform init
> code
> > > > of
> > > > > > the SOCs.
> > > > > >
> > > > > > 2. All known devices, the lowest PCIe outbound range starts at
> > > > > > 0x80000000, but there's nothing above 0xc0000000. So the
> inbound of
> > > > > > size 0x8000_0000 is always availbe on all devices. Hardcode the
> > > check
> > > > > > in platform code to check memblock_end_of_DRAM() to 0x80000000.
> > > > > >
> > > > > > Something like this:
> > > > > >
> > > > > > diff --git a/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > > > b/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > > > index 1f7028e..ef4e215 100644
> > > > > > --- a/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > > > +++ b/arch/powerpc/platforms/85xx/corenet_ds.c
> > > > > > @@ -79,7 +79,7 @@ void __init corenet_ds_setup_arch(void)
> #endif
> > > > > >
> > > > > > #ifdef CONFIG_SWIOTLB
> > > > > > -       if (memblock_end_of_DRAM() > 0xffffffff)
> > > > > > +       if (memblock_end_of_DRAM() > 0xff000000)
> > > > > >                  ppc_swiotlb_enable = 1;  #endif
> > > > > >          pr_info("%s board from Freescale Semiconductor\n",
> > > > > > ppc_md.name);
> > > > > >
> > > > > > -------------
> > > > > >
> > > > > > 3. Always do swiotlb_init() in mem_init() and later after PCI
> init,
> > > > if
> > > > > > the swiotlb is not needed then free it (swiotlb_free()).
> > > > > >
> > > > > > 4. etc, please provide some other better way.
> > > > > >
> > > > > > Thanks
> > > > > > -Bharat
> > > > >
> > > > > Thanks.
> > > > > In my point of view the 2nd solution is better for it does not
> treat
> > > > PCI/PCIe as
> > > > > the special kind of devices from others.
> > > > >
> > > > > -Jia Hongtao.
> > >
> > > _______________________________________________
> > > Linuxppc-dev mailing list
> > > Linuxppc-dev@lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/linuxppc-dev
> >
> 
>

Patch

diff --git a/arch/powerpc/platforms/85xx/corenet_ds.c
b/arch/powerpc/platforms/85xx/corenet_ds.c
index 1f7028e..ef4e215 100644
--- a/arch/powerpc/platforms/85xx/corenet_ds.c
+++ b/arch/powerpc/platforms/85xx/corenet_ds.c
@@ -79,7 +79,7 @@  void __init corenet_ds_setup_arch(void)  #endif

#ifdef CONFIG_SWIOTLB
-       if (memblock_end_of_DRAM() > 0xffffffff)
+       if (memblock_end_of_DRAM() > 0xff000000)
                 ppc_swiotlb_enable = 1;  #endif
         pr_info("%s board from Freescale Semiconductor\n", ppc_md.name);

-------------