diff mbox

[v2] PCI: designware: move remaining rc setup code to dw_pcie_setup_rc()

Message ID 1458128433-3020-1-git-send-email-jszhang@marvell.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Jisheng Zhang March 16, 2016, 11:40 a.m. UTC
dw_pcie_setup_rc(), as its name indicates, setups the RC. But current
dw_pcie_host_init() also contains some necessary rc setup code.

Another reason: the host may lost power during suspend to ram, the RC
need to be re-setup after resume. The rc can't be correctly resumed
without the rc setup code in dw_pcie_host_init().

So this patch moves the code to dw_pcie_setup_rc() to address the above
two issues. After this patch, each pcie designware driver users could
call dw_pcie_setup_rc() to re-setup rc when resume back.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
Since v1:
 - fix gcc warning found by lkp, thanks

 drivers/pci/host/pcie-designware.c | 39 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

Comments

Pratyush Anand March 17, 2016, 4:28 a.m. UTC | #1
On Wed, Mar 16, 2016 at 5:10 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
> dw_pcie_setup_rc(), as its name indicates, setups the RC. But current
> dw_pcie_host_init() also contains some necessary rc setup code.
>
> Another reason: the host may lost power during suspend to ram, the RC
> need to be re-setup after resume. The rc can't be correctly resumed
> without the rc setup code in dw_pcie_host_init().
>
> So this patch moves the code to dw_pcie_setup_rc() to address the above
> two issues. After this patch, each pcie designware driver users could
> call dw_pcie_setup_rc() to re-setup rc when resume back.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>

Seems reasonable. Thanks for the cleanup. :-)

Acked-by: Pratyush Anand <pratyush.anand@gmail.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 5, 2016, 11:12 p.m. UTC | #2
On Wed, Mar 16, 2016 at 07:40:33PM +0800, Jisheng Zhang wrote:
> dw_pcie_setup_rc(), as its name indicates, setups the RC. But current
> dw_pcie_host_init() also contains some necessary rc setup code.
> 
> Another reason: the host may lost power during suspend to ram, the RC
> need to be re-setup after resume. The rc can't be correctly resumed
> without the rc setup code in dw_pcie_host_init().
> 
> So this patch moves the code to dw_pcie_setup_rc() to address the above
> two issues. After this patch, each pcie designware driver users could
> call dw_pcie_setup_rc() to re-setup rc when resume back.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>

Very nice, applied with Pratyush's ack to pci/host-designware for
v4.7, thanks!

> ---
> Since v1:
>  - fix gcc warning found by lkp, thanks
> 
>  drivers/pci/host/pcie-designware.c | 39 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index a4cccd3..261e4a11 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -434,7 +434,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  	struct platform_device *pdev = to_platform_device(pp->dev);
>  	struct pci_bus *bus, *child;
>  	struct resource *cfg_res;
> -	u32 val;
>  	int i, ret;
>  	LIST_HEAD(res);
>  	struct resource_entry *win;
> @@ -544,25 +543,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  	if (pp->ops->host_init)
>  		pp->ops->host_init(pp);
>  
> -	/*
> -	 * If the platform provides ->rd_other_conf, it means the platform
> -	 * uses its own address translation component rather than ATU, so
> -	 * we should not program the ATU here.
> -	 */
> -	if (!pp->ops->rd_other_conf)
> -		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
> -					  PCIE_ATU_TYPE_MEM, pp->mem_base,
> -					  pp->mem_bus_addr, pp->mem_size);
> -
> -	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
> -
> -	/* program correct class for RC */
> -	dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
> -
> -	dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val);
> -	val |= PORT_LOGIC_SPEED_CHANGE;
> -	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
> -
>  	pp->root_bus_nr = pp->busn->start;
>  	if (IS_ENABLED(CONFIG_PCI_MSI)) {
>  		bus = pci_scan_root_bus_msi(pp->dev, pp->root_bus_nr,
> @@ -800,6 +780,25 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  	val |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
>  		PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
>  	dw_pcie_writel_rc(pp, val, PCI_COMMAND);
> +
> +	/*
> +	 * If the platform provides ->rd_other_conf, it means the platform
> +	 * uses its own address translation component rather than ATU, so
> +	 * we should not program the ATU here.
> +	 */
> +	if (!pp->ops->rd_other_conf)
> +		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
> +					  PCIE_ATU_TYPE_MEM, pp->mem_base,
> +					  pp->mem_bus_addr, pp->mem_size);
> +
> +	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
> +
> +	/* program correct class for RC */
> +	dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
> +
> +	dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val);
> +	val |= PORT_LOGIC_SPEED_CHANGE;
> +	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
>  }
>  
>  MODULE_AUTHOR("Jingoo Han <jg1.han@samsung.com>");
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gabriele Paoloni April 6, 2016, 2:50 p.m. UTC | #3
Hi, sorry to be late on this

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Jisheng Zhang
> Sent: 16 March 2016 11:41
> To: jingoohan1@gmail.com; pratyush.anand@gmail.com; bhelgaas@google.com
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Jisheng Zhang
> Subject: [PATCH v2] PCI: designware: move remaining rc setup code to
> dw_pcie_setup_rc()
> 
> dw_pcie_setup_rc(), as its name indicates, setups the RC. But current
> dw_pcie_host_init() also contains some necessary rc setup code.
> 
> Another reason: the host may lost power during suspend to ram, the RC
> need to be re-setup after resume. The rc can't be correctly resumed
> without the rc setup code in dw_pcie_host_init().
> 
> So this patch moves the code to dw_pcie_setup_rc() to address the above
> two issues. After this patch, each pcie designware driver users could
> call dw_pcie_setup_rc() to re-setup rc when resume back.

I think this patch breaks the Hisilicon driver...

Our driver performs linkup setup in UEFI therefore we do not call
dw_pcie_setup_rc(), we only call dw_pcie_host_init().

Maybe better to group the part of code to be moved in as separate
function...

Thanks and sorry for late reply.

Gab


> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
> Since v1:
>  - fix gcc warning found by lkp, thanks
> 
>  drivers/pci/host/pcie-designware.c | 39 +++++++++++++++++++-----------
> --------
>  1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-designware.c
> b/drivers/pci/host/pcie-designware.c
> index a4cccd3..261e4a11 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -434,7 +434,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  	struct platform_device *pdev = to_platform_device(pp->dev);
>  	struct pci_bus *bus, *child;
>  	struct resource *cfg_res;
> -	u32 val;
>  	int i, ret;
>  	LIST_HEAD(res);
>  	struct resource_entry *win;
> @@ -544,25 +543,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  	if (pp->ops->host_init)
>  		pp->ops->host_init(pp);
> 
> -	/*
> -	 * If the platform provides ->rd_other_conf, it means the
> platform
> -	 * uses its own address translation component rather than ATU, so
> -	 * we should not program the ATU here.
> -	 */
> -	if (!pp->ops->rd_other_conf)
> -		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
> -					  PCIE_ATU_TYPE_MEM, pp->mem_base,
> -					  pp->mem_bus_addr, pp->mem_size);
> -
> -	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
> -
> -	/* program correct class for RC */
> -	dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2,
> PCI_CLASS_BRIDGE_PCI);
> -
> -	dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val);
> -	val |= PORT_LOGIC_SPEED_CHANGE;
> -	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
> -
>  	pp->root_bus_nr = pp->busn->start;
>  	if (IS_ENABLED(CONFIG_PCI_MSI)) {
>  		bus = pci_scan_root_bus_msi(pp->dev, pp->root_bus_nr,
> @@ -800,6 +780,25 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  	val |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
>  		PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
>  	dw_pcie_writel_rc(pp, val, PCI_COMMAND);
> +
> +	/*
> +	 * If the platform provides ->rd_other_conf, it means the
> platform
> +	 * uses its own address translation component rather than ATU, so
> +	 * we should not program the ATU here.
> +	 */
> +	if (!pp->ops->rd_other_conf)
> +		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
> +					  PCIE_ATU_TYPE_MEM, pp->mem_base,
> +					  pp->mem_bus_addr, pp->mem_size);
> +
> +	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
> +
> +	/* program correct class for RC */
> +	dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2,
> PCI_CLASS_BRIDGE_PCI);
> +
> +	dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val);
> +	val |= PORT_LOGIC_SPEED_CHANGE;
> +	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
>  }
> 
>  MODULE_AUTHOR("Jingoo Han <jg1.han@samsung.com>");
> --
> 2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jisheng Zhang April 7, 2016, 2:37 a.m. UTC | #4
Hi Gabriele,

On Wed, 6 Apr 2016 14:50:29 +0000 Gabriele Paoloni wrote:

> Hi, sorry to be late on this
> 
> > -----Original Message-----
> > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> > owner@vger.kernel.org] On Behalf Of Jisheng Zhang
> > Sent: 16 March 2016 11:41
> > To: jingoohan1@gmail.com; pratyush.anand@gmail.com; bhelgaas@google.com
> > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; Jisheng Zhang
> > Subject: [PATCH v2] PCI: designware: move remaining rc setup code to
> > dw_pcie_setup_rc()
> > 
> > dw_pcie_setup_rc(), as its name indicates, setups the RC. But current
> > dw_pcie_host_init() also contains some necessary rc setup code.
> > 
> > Another reason: the host may lost power during suspend to ram, the RC
> > need to be re-setup after resume. The rc can't be correctly resumed
> > without the rc setup code in dw_pcie_host_init().
> > 
> > So this patch moves the code to dw_pcie_setup_rc() to address the above
> > two issues. After this patch, each pcie designware driver users could
> > call dw_pcie_setup_rc() to re-setup rc when resume back.  
> 
> I think this patch breaks the Hisilicon driver...
> 
> Our driver performs linkup setup in UEFI therefore we do not call
> dw_pcie_setup_rc(), we only call dw_pcie_host_init().

Thanks for the information. So pcie-hisi rely on UEFI to do something similar
in dw_pcie_setup_rc(), this comes to a common driver implement question: should
linux device driver rely on bootloader to configure HW device?

Is it acceptable that pcie-hisi adds a call to dw_pcie_setup_rc() in
hisi_add_pcie_port()?

Thanks,
Jisheng

> 
> Maybe better to group the part of code to be moved in as separate
> function...
> 
> Thanks and sorry for late reply.
> 
> Gab
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pratyush Anand April 7, 2016, 6:59 a.m. UTC | #5
Hi Gab,

Thanks for bringing it.


On Wed, Apr 6, 2016 at 8:20 PM, Gabriele Paoloni
<gabriele.paoloni@huawei.com> wrote:
> Hi, sorry to be late on this
>
>> -----Original Message-----
>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>> owner@vger.kernel.org] On Behalf Of Jisheng Zhang
>> Sent: 16 March 2016 11:41
>> To: jingoohan1@gmail.com; pratyush.anand@gmail.com; bhelgaas@google.com
>> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; Jisheng Zhang
>> Subject: [PATCH v2] PCI: designware: move remaining rc setup code to
>> dw_pcie_setup_rc()
>>
>> dw_pcie_setup_rc(), as its name indicates, setups the RC. But current
>> dw_pcie_host_init() also contains some necessary rc setup code.
>>
>> Another reason: the host may lost power during suspend to ram, the RC
>> need to be re-setup after resume. The rc can't be correctly resumed
>> without the rc setup code in dw_pcie_host_init().
>>
>> So this patch moves the code to dw_pcie_setup_rc() to address the above
>> two issues. After this patch, each pcie designware driver users could
>> call dw_pcie_setup_rc() to re-setup rc when resume back.
>
> I think this patch breaks the Hisilicon driver...
>
> Our driver performs linkup setup in UEFI therefore we do not call
> dw_pcie_setup_rc(), we only call dw_pcie_host_init().
>
> Maybe better to group the part of code to be moved in as separate
> function...
>
> Thanks and sorry for late reply.
>

I am just wondering, should n't then what ever we do in
dw_pcie_setup_rc() be done in your boot loader and not just link up.

~Pratyush
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gabriele Paoloni April 7, 2016, 8:14 a.m. UTC | #6
Hi Pratyush

Many thanks for quick replying

> -----Original Message-----

> From: Pratyush Anand [mailto:pratyush.anand@gmail.com]

> Sent: 07 April 2016 07:59

> To: Gabriele Paoloni

> Cc: Jisheng Zhang; jingoohan1@gmail.com; bhelgaas@google.com; linux-

> pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-

> kernel@lists.infradead.org

> Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code

> to dw_pcie_setup_rc()

> 

> Hi Gab,

> 

> Thanks for bringing it.

> 

> 

> On Wed, Apr 6, 2016 at 8:20 PM, Gabriele Paoloni

> <gabriele.paoloni@huawei.com> wrote:

> > Hi, sorry to be late on this

> >

> >> -----Original Message-----

> >> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-

> >> owner@vger.kernel.org] On Behalf Of Jisheng Zhang

> >> Sent: 16 March 2016 11:41

> >> To: jingoohan1@gmail.com; pratyush.anand@gmail.com;

> bhelgaas@google.com

> >> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux-

> arm-

> >> kernel@lists.infradead.org; Jisheng Zhang

> >> Subject: [PATCH v2] PCI: designware: move remaining rc setup code to

> >> dw_pcie_setup_rc()

> >>

> >> dw_pcie_setup_rc(), as its name indicates, setups the RC. But

> current

> >> dw_pcie_host_init() also contains some necessary rc setup code.

> >>

> >> Another reason: the host may lost power during suspend to ram, the

> RC

> >> need to be re-setup after resume. The rc can't be correctly resumed

> >> without the rc setup code in dw_pcie_host_init().

> >>

> >> So this patch moves the code to dw_pcie_setup_rc() to address the

> above

> >> two issues. After this patch, each pcie designware driver users

> could

> >> call dw_pcie_setup_rc() to re-setup rc when resume back.

> >

> > I think this patch breaks the Hisilicon driver...

> >

> > Our driver performs linkup setup in UEFI therefore we do not call

> > dw_pcie_setup_rc(), we only call dw_pcie_host_init().

> >

> > Maybe better to group the part of code to be moved in as separate

> > function...

> >

> > Thanks and sorry for late reply.

> >

> 

> I am just wondering, should n't then what ever we do in

> dw_pcie_setup_rc() be done in your boot loader and not just link up.


Currently the HiSilicon driver does not call dw_pcie_setup_rc() at all;
so everything is done in dw_pcie_setup_rc() is done in bootloader.

I guess your question is if we can execute in bootloader the part of
code the this patch has moved to in "dw_pcie_setup_rc()". I think the
problem here is that even if it was possible we would break backward
compatibility with previous bootloaders...

Thanks

Gab 


> 

> ~Pratyush
Gabriele Paoloni April 7, 2016, 8:20 a.m. UTC | #7
Hi Jisheng

Thanks for your reply

> -----Original Message-----
> From: Jisheng Zhang [mailto:jszhang@marvell.com]
> Sent: 07 April 2016 03:38
> To: Gabriele Paoloni; jingoohan1@gmail.com; pratyush.anand@gmail.com;
> bhelgaas@google.com
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code
> to dw_pcie_setup_rc()
> 
> Hi Gabriele,
> 
> On Wed, 6 Apr 2016 14:50:29 +0000 Gabriele Paoloni wrote:
> 
> > Hi, sorry to be late on this
> >
> > > -----Original Message-----
> > > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> > > owner@vger.kernel.org] On Behalf Of Jisheng Zhang
> > > Sent: 16 March 2016 11:41
> > > To: jingoohan1@gmail.com; pratyush.anand@gmail.com;
> bhelgaas@google.com
> > > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> arm-
> > > kernel@lists.infradead.org; Jisheng Zhang
> > > Subject: [PATCH v2] PCI: designware: move remaining rc setup code
> to
> > > dw_pcie_setup_rc()
> > >
> > > dw_pcie_setup_rc(), as its name indicates, setups the RC. But
> current
> > > dw_pcie_host_init() also contains some necessary rc setup code.
> > >
> > > Another reason: the host may lost power during suspend to ram, the
> RC
> > > need to be re-setup after resume. The rc can't be correctly resumed
> > > without the rc setup code in dw_pcie_host_init().
> > >
> > > So this patch moves the code to dw_pcie_setup_rc() to address the
> above
> > > two issues. After this patch, each pcie designware driver users
> could
> > > call dw_pcie_setup_rc() to re-setup rc when resume back.
> >
> > I think this patch breaks the Hisilicon driver...
> >
> > Our driver performs linkup setup in UEFI therefore we do not call
> > dw_pcie_setup_rc(), we only call dw_pcie_host_init().
> 
> Thanks for the information. So pcie-hisi rely on UEFI to do something
> similar
> in dw_pcie_setup_rc(), this comes to a common driver implement
> question: should
> linux device driver rely on bootloader to configure HW device?

I don't see any issue with this...

> 
> Is it acceptable that pcie-hisi adds a call to dw_pcie_setup_rc() in
> hisi_add_pcie_port()?

I don't think so...that would try to overwrite what is already set by
the bootloader; so it is wrong in principle and maybe it can lead to
undefined behaviours...

Thanks

Gab

> 
> Thanks,
> Jisheng
> 
> >
> > Maybe better to group the part of code to be moved in as separate
> > function...
> >
> > Thanks and sorry for late reply.
> >
> > Gab
> >
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jisheng Zhang April 7, 2016, 8:34 a.m. UTC | #8
Hi Gabriele,

On Thu, 7 Apr 2016 08:20:28 +0000 Gabriele Paoloni wrote:

> Hi Jisheng
> 
> Thanks for your reply
> 
> > -----Original Message-----
> > From: Jisheng Zhang [mailto:jszhang@marvell.com]
> > Sent: 07 April 2016 03:38
> > To: Gabriele Paoloni; jingoohan1@gmail.com; pratyush.anand@gmail.com;
> > bhelgaas@google.com
> > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org
> > Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code
> > to dw_pcie_setup_rc()
> > 
> > Hi Gabriele,
> > 
> > On Wed, 6 Apr 2016 14:50:29 +0000 Gabriele Paoloni wrote:
> >   
> > > Hi, sorry to be late on this
> > >  
> > > > -----Original Message-----
> > > > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> > > > owner@vger.kernel.org] On Behalf Of Jisheng Zhang
> > > > Sent: 16 March 2016 11:41
> > > > To: jingoohan1@gmail.com; pratyush.anand@gmail.com;  
> > bhelgaas@google.com  
> > > > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux-  
> > arm-  
> > > > kernel@lists.infradead.org; Jisheng Zhang
> > > > Subject: [PATCH v2] PCI: designware: move remaining rc setup code  
> > to  
> > > > dw_pcie_setup_rc()
> > > >
> > > > dw_pcie_setup_rc(), as its name indicates, setups the RC. But  
> > current  
> > > > dw_pcie_host_init() also contains some necessary rc setup code.
> > > >
> > > > Another reason: the host may lost power during suspend to ram, the  
> > RC  
> > > > need to be re-setup after resume. The rc can't be correctly resumed
> > > > without the rc setup code in dw_pcie_host_init().
> > > >
> > > > So this patch moves the code to dw_pcie_setup_rc() to address the  
> > above  
> > > > two issues. After this patch, each pcie designware driver users  
> > could  
> > > > call dw_pcie_setup_rc() to re-setup rc when resume back.  
> > >
> > > I think this patch breaks the Hisilicon driver...
> > >
> > > Our driver performs linkup setup in UEFI therefore we do not call
> > > dw_pcie_setup_rc(), we only call dw_pcie_host_init().  
> > 
> > Thanks for the information. So pcie-hisi rely on UEFI to do something
> > similar
> > in dw_pcie_setup_rc(), this comes to a common driver implement
> > question: should
> > linux device driver rely on bootloader to configure HW device?  
> 
> I don't see any issue with this...
> 
> > 
> > Is it acceptable that pcie-hisi adds a call to dw_pcie_setup_rc() in
> > hisi_add_pcie_port()?  
> 
> I don't think so...that would try to overwrite what is already set by
> the bootloader; so it is wrong in principle and maybe it can lead to
> undefined behaviours...

make sense! This commit is intend to re-setup the rc when waken from s2ram (in
s2ram state, the host lost power)

I have no good solution but to introduce one function e.g 
dw_pcie_setup_rc_after_linkup(), then move related code from dw_pcie_host_init
to it, then let my host driver resume hook to call.

Hi Pratyush, Jingoo and Bjorn etc.

any suggestions are appreciated!

Thanks,
Jisheng

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index a4cccd3..261e4a11 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -434,7 +434,6 @@  int dw_pcie_host_init(struct pcie_port *pp)
 	struct platform_device *pdev = to_platform_device(pp->dev);
 	struct pci_bus *bus, *child;
 	struct resource *cfg_res;
-	u32 val;
 	int i, ret;
 	LIST_HEAD(res);
 	struct resource_entry *win;
@@ -544,25 +543,6 @@  int dw_pcie_host_init(struct pcie_port *pp)
 	if (pp->ops->host_init)
 		pp->ops->host_init(pp);
 
-	/*
-	 * If the platform provides ->rd_other_conf, it means the platform
-	 * uses its own address translation component rather than ATU, so
-	 * we should not program the ATU here.
-	 */
-	if (!pp->ops->rd_other_conf)
-		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
-					  PCIE_ATU_TYPE_MEM, pp->mem_base,
-					  pp->mem_bus_addr, pp->mem_size);
-
-	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
-
-	/* program correct class for RC */
-	dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
-
-	dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val);
-	val |= PORT_LOGIC_SPEED_CHANGE;
-	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
-
 	pp->root_bus_nr = pp->busn->start;
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		bus = pci_scan_root_bus_msi(pp->dev, pp->root_bus_nr,
@@ -800,6 +780,25 @@  void dw_pcie_setup_rc(struct pcie_port *pp)
 	val |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
 		PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
 	dw_pcie_writel_rc(pp, val, PCI_COMMAND);
+
+	/*
+	 * If the platform provides ->rd_other_conf, it means the platform
+	 * uses its own address translation component rather than ATU, so
+	 * we should not program the ATU here.
+	 */
+	if (!pp->ops->rd_other_conf)
+		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
+					  PCIE_ATU_TYPE_MEM, pp->mem_base,
+					  pp->mem_bus_addr, pp->mem_size);
+
+	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
+
+	/* program correct class for RC */
+	dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
+
+	dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val);
+	val |= PORT_LOGIC_SPEED_CHANGE;
+	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
 }
 
 MODULE_AUTHOR("Jingoo Han <jg1.han@samsung.com>");