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 <redacted>
---
Since v1:
- fix gcc warning found by lkp, thanks
drivers/pci/host/pcie-designware.c | 39 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 20 deletions(-)
@@ -434,7 +434,6 @@ int dw_pcie_host_init(struct pcie_port *pp)structplatform_device*pdev=to_platform_device(pp->dev);structpci_bus*bus,*child;structresource*cfg_res;-u32val;inti,ret;LIST_HEAD(res);structresource_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);-/*-*Iftheplatformprovides->rd_other_conf,itmeanstheplatform-*usesitsownaddresstranslationcomponentratherthanATU,so-*weshouldnotprogramtheATUhere.-*/-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);++/*+*Iftheplatformprovides->rd_other_conf,itmeanstheplatform+*usesitsownaddresstranslationcomponentratherthanATU,so+*weshouldnotprogramtheATUhere.+*/+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>");
On Wed, Mar 16, 2016 at 5:10 PM, Jisheng Zhang [off-list ref] 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 <redacted>
Seems reasonable. Thanks for the cleanup. :-)
Acked-by: Pratyush Anand <pratyush.anand@gmail.com>
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 <redacted>
Very nice, applied with Pratyush's ack to pci/host-designware for
v4.7, thanks!
quoted hunk
---
Since v1:
- fix gcc warning found by lkp, thanks
drivers/pci/host/pcie-designware.c | 39 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 20 deletions(-)
@@ -434,7 +434,6 @@ int dw_pcie_host_init(struct pcie_port *pp)structplatform_device*pdev=to_platform_device(pp->dev);structpci_bus*bus,*child;structresource*cfg_res;-u32val;inti,ret;LIST_HEAD(res);structresource_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);-/*-*Iftheplatformprovides->rd_other_conf,itmeanstheplatform-*usesitsownaddresstranslationcomponentratherthanATU,so-*weshouldnotprogramtheATUhere.-*/-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);++/*+*Iftheplatformprovides->rd_other_conf,itmeanstheplatform+*usesitsownaddresstranslationcomponentratherthanATU,so+*weshouldnotprogramtheATUhere.+*/+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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
-----Original Message-----
From: linux-kernel-owner at vger.kernel.org [mailto:linux-kernel-
owner at vger.kernel.org] On Behalf Of Jisheng Zhang
Sent: 16 March 2016 11:41
To: jingoohan1 at gmail.com; pratyush.anand at gmail.com; bhelgaas at google.com
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
kernel at 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
quoted hunk
Signed-off-by: Jisheng Zhang <redacted>
---
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
@@ -434,7 +434,6 @@ int dw_pcie_host_init(struct pcie_port *pp)structplatform_device*pdev=to_platform_device(pp->dev);structpci_bus*bus,*child;structresource*cfg_res;-u32val;inti,ret;LIST_HEAD(res);structresource_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);-/*-*Iftheplatformprovides->rd_other_conf,itmeansthe
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 [off-list ref]");
--
2.7.0
Hi Gabriele,
On Wed, 6 Apr 2016 14:50:29 +0000 Gabriele Paoloni wrote:
Hi, sorry to be late on this
quoted
-----Original Message-----
From: linux-kernel-owner at vger.kernel.org [mailto:linux-kernel-
owner at vger.kernel.org] On Behalf Of Jisheng Zhang
Sent: 16 March 2016 11:41
To: jingoohan1 at gmail.com; pratyush.anand at gmail.com; bhelgaas at google.com
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
kernel at 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
Hi Gab,
Thanks for bringing it.
On Wed, Apr 6, 2016 at 8:20 PM, Gabriele Paoloni
[off-list ref] wrote:
Hi, sorry to be late on this
quoted
-----Original Message-----
From: linux-kernel-owner at vger.kernel.org [mailto:linux-kernel-
owner at vger.kernel.org] On Behalf Of Jisheng Zhang
Sent: 16 March 2016 11:41
To: jingoohan1 at gmail.com; pratyush.anand at gmail.com; bhelgaas at google.com
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
kernel at 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
-----Original Message-----
From: Pratyush Anand [mailto:pratyush.anand at gmail.com]
Sent: 07 April 2016 07:59
To: Gabriele Paoloni
Cc: Jisheng Zhang; jingoohan1 at gmail.com; bhelgaas at google.com; linux-
pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
kernel at 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
[off-list ref] wrote:
quoted
Hi, sorry to be late on this
quoted
-----Original Message-----
From: linux-kernel-owner at vger.kernel.org [mailto:linux-kernel-
owner at vger.kernel.org] On Behalf Of Jisheng Zhang
Sent: 16 March 2016 11:41
To: jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com
quoted
quoted
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-
arm-
quoted
quoted
kernel at 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
quoted
quoted
dw_pcie_host_init() also contains some necessary rc setup code.
Another reason: the host may lost power during suspend to ram, the
RC
quoted
quoted
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
quoted
quoted
two issues. After this patch, each pcie designware driver users
could
quoted
quoted
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
-----Original Message-----
From: Jisheng Zhang [mailto:jszhang at marvell.com]
Sent: 07 April 2016 03:38
To: Gabriele Paoloni; jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
kernel at 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:
quoted
Hi, sorry to be late on this
quoted
-----Original Message-----
From: linux-kernel-owner at vger.kernel.org [mailto:linux-kernel-
owner at vger.kernel.org] On Behalf Of Jisheng Zhang
Sent: 16 March 2016 11:41
To: jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com
quoted
quoted
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-
dw_pcie_setup_rc()
dw_pcie_setup_rc(), as its name indicates, setups the RC. But
current
quoted
quoted
dw_pcie_host_init() also contains some necessary rc setup code.
Another reason: the host may lost power during suspend to ram, the
RC
quoted
quoted
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
quoted
quoted
two issues. After this patch, each pcie designware driver users
could
quoted
quoted
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
quoted
Maybe better to group the part of code to be moved in as separate
function...
Thanks and sorry for late reply.
Gab
Hi Gabriele,
On Thu, 7 Apr 2016 08:20:28 +0000 Gabriele Paoloni wrote:
Hi Jisheng
Thanks for your reply
quoted
-----Original Message-----
From: Jisheng Zhang [mailto:jszhang at marvell.com]
Sent: 07 April 2016 03:38
To: Gabriele Paoloni; jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
kernel at 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:
quoted
Hi, sorry to be late on this
quoted
-----Original Message-----
From: linux-kernel-owner at vger.kernel.org [mailto:linux-kernel-
owner at vger.kernel.org] On Behalf Of Jisheng Zhang
Sent: 16 March 2016 11:41
To: jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com
quoted
quoted
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-
dw_pcie_setup_rc()
dw_pcie_setup_rc(), as its name indicates, setups the RC. But
current
quoted
quoted
dw_pcie_host_init() also contains some necessary rc setup code.
Another reason: the host may lost power during suspend to ram, the
RC
quoted
quoted
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
quoted
quoted
two issues. After this patch, each pcie designware driver users
could
quoted
quoted
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...
quoted
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
-----Original Message-----
From: Jisheng Zhang [mailto:jszhang at marvell.com]
Sent: 07 April 2016 09:35
To: Gabriele Paoloni
Cc: jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com; linux-pci at vger.kernel.org; linux-
kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code
to dw_pcie_setup_rc()
Hi Gabriele,
On Thu, 7 Apr 2016 08:20:28 +0000 Gabriele Paoloni wrote:
quoted
Hi Jisheng
Thanks for your reply
quoted
-----Original Message-----
From: Jisheng Zhang [mailto:jszhang at marvell.com]
Sent: 07 April 2016 03:38
To: Gabriele Paoloni; jingoohan1 at gmail.com;
pratyush.anand at gmail.com;
quoted
quoted
bhelgaas at google.com
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-
to dw_pcie_setup_rc()
Hi Gabriele,
On Wed, 6 Apr 2016 14:50:29 +0000 Gabriele Paoloni wrote:
quoted
Hi, sorry to be late on this
quoted
-----Original Message-----
From: linux-kernel-owner at vger.kernel.org [mailto:linux-kernel-
owner at vger.kernel.org] On Behalf Of Jisheng Zhang
Sent: 16 March 2016 11:41
To: jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com
quoted
quoted
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org;
dw_pcie_setup_rc()
dw_pcie_setup_rc(), as its name indicates, setups the RC. But
current
quoted
quoted
dw_pcie_host_init() also contains some necessary rc setup code.
Another reason: the host may lost power during suspend to ram,
the
quoted
quoted
RC
quoted
quoted
need to be re-setup after resume. The rc can't be correctly
resumed
quoted
quoted
quoted
quoted
without the rc setup code in dw_pcie_host_init().
So this patch moves the code to dw_pcie_setup_rc() to address
the
quoted
quoted
above
quoted
quoted
two issues. After this patch, each pcie designware driver users
could
quoted
quoted
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
quoted
quoted
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...
quoted
Is it acceptable that pcie-hisi adds a call to dw_pcie_setup_rc()
in
quoted
quoted
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!
@@ -434,7 +434,6 @@ int dw_pcie_host_init(struct pcie_port *pp)structplatform_device*pdev=to_platform_device(pp->dev);structpci_bus*bus,*child;structresource*cfg_res;-u32val;inti,ret;LIST_HEAD(res);structresource_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);-/*-*Iftheplatformprovides->rd_other_conf,itmeanstheplatform-*usesitsownaddresstranslationcomponentratherthanATU,so-*weshouldnotprogramtheATUhere.-*/-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,
@@ -725,6 +705,29 @@ static struct pci_ops dw_pcie_ops = {.write=dw_pcie_wr_conf,};+voiddw_pcie_setup_own_cfg(structpcie_port*pp)+{+u32val;+/*+*Iftheplatformprovides->rd_other_conf,itmeanstheplatform+*usesitsownaddresstranslationcomponentratherthanATU,so+*weshouldnotprogramtheATUhere.+*/+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);+}+voiddw_pcie_setup_rc(structpcie_port*pp){u32val;
@@ -800,6 +803,8 @@ 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);++dw_pcie_setup_own_cfg(pp);}MODULE_AUTHOR("Jingoo Han <jg1.han@samsung.com>");
On Thu, 7 Apr 2016 10:06:45 +0000 Gabriele Paoloni wrote:
Hi Jisheng
..
quoted hunk
quoted
quoted
quoted
Is it acceptable that pcie-hisi adds a call to dw_pcie_setup_rc()
in
quoted
quoted
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!
@@ -434,7 +434,6 @@ int dw_pcie_host_init(struct pcie_port *pp)structplatform_device*pdev=to_platform_device(pp->dev);structpci_bus*bus,*child;structresource*cfg_res;-u32val;inti,ret;LIST_HEAD(res);structresource_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);-/*-*Iftheplatformprovides->rd_other_conf,itmeanstheplatform-*usesitsownaddresstranslationcomponentratherthanATU,so-*weshouldnotprogramtheATUhere.-*/-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);
If we introduced one global function, is it better to keep the RC registers
program sequence not changed at all? And ask each pcie dw users to call this
new function in its resume hook if necessary. Either is fine, and can provide
what I want, let's wait maintainers' comments.
Something like:
@@ -434,7 +434,6 @@ int dw_pcie_host_init(struct pcie_port *pp)structplatform_device*pdev=to_platform_device(pp->dev);structpci_bus*bus,*child;structresource*cfg_res;-u32val;inti,ret;LIST_HEAD(res);structresource_entry*win;
@@ -544,24 +543,7 @@ int dw_pcie_host_init(struct pcie_port *pp)if(pp->ops->host_init)pp->ops->host_init(pp);-/*-*Iftheplatformprovides->rd_other_conf,itmeanstheplatform-*usesitsownaddresstranslationcomponentratherthanATU,so-*weshouldnotprogramtheATUhere.-*/-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);+dw_pcie_setup_rc_after_linkup(pp);pp->root_bus_nr=pp->busn->start;if(IS_ENABLED(CONFIG_PCI_MSI)){
@@ -725,6 +707,30 @@ static struct pci_ops dw_pcie_ops = {.write=dw_pcie_wr_conf,};+voiddw_pcie_setup_rc_after_linkup(structpcie_port*pp)+{+u32val;++/*+*Iftheplatformprovides->rd_other_conf,itmeanstheplatform+*usesitsownaddresstranslationcomponentratherthanATU,so+*weshouldnotprogramtheATUhere.+*/+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);+}+voiddw_pcie_setup_rc(structpcie_port*pp){u32val;
On Thu, Apr 07, 2016 at 10:06:45AM +0000, Gabriele Paoloni wrote:
quoted hunk
Hi Jisheng
quoted
-----Original Message-----
From: Jisheng Zhang [mailto:jszhang at marvell.com]
Sent: 07 April 2016 09:35
To: Gabriele Paoloni
Cc: jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com; linux-pci at vger.kernel.org; linux-
kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code
to dw_pcie_setup_rc()
Hi Gabriele,
On Thu, 7 Apr 2016 08:20:28 +0000 Gabriele Paoloni wrote:
quoted
Hi Jisheng
Thanks for your reply
quoted
-----Original Message-----
From: Jisheng Zhang [mailto:jszhang at marvell.com]
Sent: 07 April 2016 03:38
To: Gabriele Paoloni; jingoohan1 at gmail.com;
pratyush.anand at gmail.com;
quoted
quoted
bhelgaas at google.com
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-
to dw_pcie_setup_rc()
Hi Gabriele,
On Wed, 6 Apr 2016 14:50:29 +0000 Gabriele Paoloni wrote:
quoted
Hi, sorry to be late on this
quoted
-----Original Message-----
From: linux-kernel-owner at vger.kernel.org [mailto:linux-kernel-
owner at vger.kernel.org] On Behalf Of Jisheng Zhang
Sent: 16 March 2016 11:41
To: jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com
quoted
quoted
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org;
dw_pcie_setup_rc()
dw_pcie_setup_rc(), as its name indicates, setups the RC. But
current
quoted
quoted
dw_pcie_host_init() also contains some necessary rc setup code.
Another reason: the host may lost power during suspend to ram,
the
quoted
quoted
RC
quoted
quoted
need to be re-setup after resume. The rc can't be correctly
resumed
quoted
quoted
quoted
quoted
without the rc setup code in dw_pcie_host_init().
So this patch moves the code to dw_pcie_setup_rc() to address
the
quoted
quoted
above
quoted
quoted
two issues. After this patch, each pcie designware driver users
could
quoted
quoted
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
quoted
quoted
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...
quoted
Is it acceptable that pcie-hisi adds a call to dw_pcie_setup_rc()
in
quoted
quoted
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!
@@ -434,7 +434,6 @@ int dw_pcie_host_init(struct pcie_port *pp)structplatform_device*pdev=to_platform_device(pp->dev);structpci_bus*bus,*child;structresource*cfg_res;-u32val;inti,ret;LIST_HEAD(res);structresource_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);-/*-*Iftheplatformprovides->rd_other_conf,itmeanstheplatform-*usesitsownaddresstranslationcomponentratherthanATU,so-*weshouldnotprogramtheATUhere.-*/-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,
@@ -725,6 +705,29 @@ static struct pci_ops dw_pcie_ops = {.write=dw_pcie_wr_conf,};+voiddw_pcie_setup_own_cfg(structpcie_port*pp)+{+u32val;+/*+*Iftheplatformprovides->rd_other_conf,itmeanstheplatform+*usesitsownaddresstranslationcomponentratherthanATU,so+*weshouldnotprogramtheATUhere.+*/+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);+}+voiddw_pcie_setup_rc(structpcie_port*pp){u32val;
@@ -800,6 +803,8 @@ 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);++dw_pcie_setup_own_cfg(pp);}MODULE_AUTHOR("Jingoo Han <jg1.han@samsung.com>");
@@ -164,6 +164,7 @@ static int hisi_add_pcie_port(struct pcie_port *pp,dev_err(&pdev->dev,"failed to initialize host\n");returnret;}+dw_pcie_setup_own_cfg(pp);return0;}
What's the hisi plan for resuming after suspend-to-RAM? How does the
RC get reprogrammed after it loses all its state?
What would break if hisi did call dw_pcie_setup_rc()? I know you said
it would overwrite what the bootloader already did, which is true.
But hisi does call dw_pcie_host_init(), so it reads pp->mem (which
determines pp->mem_base) and pp->lanes from the DT. Other drivers
then call dw_pcie_setup_rc() which programs the RC based on
pp->mem_base and pp->lanes. So hisi assumes UEFI programmed the RC to
match the DT, while the other drivers read the DT and program the RC
to match. The latter seems more robust because it enforces the
consistency rather than relying on it.
Bjorn
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas at kernel.org]
Sent: 07 April 2016 15:06
To: Gabriele Paoloni
Cc: Jisheng Zhang; jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com; linux-pci at vger.kernel.org; linux-
kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code
to dw_pcie_setup_rc()
On Thu, Apr 07, 2016 at 10:06:45AM +0000, Gabriele Paoloni wrote:
quoted
Hi Jisheng
quoted
-----Original Message-----
From: Jisheng Zhang [mailto:jszhang at marvell.com]
Sent: 07 April 2016 09:35
To: Gabriele Paoloni
Cc: jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com; linux-pci at vger.kernel.org; linux-
kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup
code
quoted
quoted
to dw_pcie_setup_rc()
Hi Gabriele,
On Thu, 7 Apr 2016 08:20:28 +0000 Gabriele Paoloni wrote:
quoted
Hi Jisheng
Thanks for your reply
quoted
-----Original Message-----
From: Jisheng Zhang [mailto:jszhang at marvell.com]
Sent: 07 April 2016 03:38
To: Gabriele Paoloni; jingoohan1 at gmail.com;
pratyush.anand at gmail.com;
quoted
quoted
bhelgaas at google.com
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org;
dw_pcie_setup_rc()
dw_pcie_setup_rc(), as its name indicates, setups the RC.
But
quoted
quoted
quoted
quoted
current
quoted
quoted
dw_pcie_host_init() also contains some necessary rc setup
code.
quoted
quoted
quoted
quoted
quoted
quoted
Another reason: the host may lost power during suspend to
ram,
quoted
quoted
the
quoted
quoted
RC
quoted
quoted
need to be re-setup after resume. The rc can't be correctly
resumed
quoted
quoted
quoted
quoted
without the rc setup code in dw_pcie_host_init().
So this patch moves the code to dw_pcie_setup_rc() to
address
quoted
quoted
the
quoted
quoted
above
quoted
quoted
two issues. After this patch, each pcie designware driver
users
quoted
quoted
quoted
quoted
could
quoted
quoted
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
quoted
quoted
quoted
quoted
quoted
dw_pcie_setup_rc(), we only call dw_pcie_host_init().
Thanks for the information. So pcie-hisi rely on UEFI to do
something
quoted
quoted
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...
quoted
Is it acceptable that pcie-hisi adds a call to
dw_pcie_setup_rc()
quoted
quoted
in
quoted
quoted
hisi_add_pcie_port()?
I don't think so...that would try to overwrite what is already
set by
quoted
quoted
quoted
the bootloader; so it is wrong in principle and maybe it can lead
to
quoted
quoted
quoted
undefined behaviours...
make sense! This commit is intend to re-setup the rc when waken
from
quoted
quoted
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!
@@ -434,7 +434,6 @@ int dw_pcie_host_init(struct pcie_port *pp)structplatform_device*pdev=to_platform_device(pp->dev);structpci_bus*bus,*child;structresource*cfg_res;-u32val;inti,ret;LIST_HEAD(res);structresource_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);-/*-*Iftheplatformprovides->rd_other_conf,itmeansthe
platform
quoted
- * 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);
quoted
-- 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,
@@ -725,6 +705,29 @@ static struct pci_ops dw_pcie_ops = { .write = dw_pcie_wr_conf, };+void dw_pcie_setup_own_cfg(struct pcie_port *pp)+{+ u32 val;+ /*+ * If the platform provides ->rd_other_conf, it means the
platform
quoted
+ * 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,
What's the hisi plan for resuming after suspend-to-RAM? How does the
RC get reprogrammed after it loses all its state?
PM is not part of the driver yet. This is planned for near
future release so haven't made such considerations yet
What would break if hisi did call dw_pcie_setup_rc()? I know you said
it would overwrite what the bootloader already did, which is true.
I am try to figure this out now with our HW team.
But hisi does call dw_pcie_host_init(), so it reads pp->mem (which
determines pp->mem_base) and pp->lanes from the DT. Other drivers
then call dw_pcie_setup_rc() which programs the RC based on
pp->mem_base and pp->lanes. So hisi assumes UEFI programmed the RC to
match the DT, while the other drivers read the DT and program the RC
to match. The latter seems more robust because it enforces the
consistency rather than relying on it.
Yes I agree with you, however we have preferred to move RC config to
BIOS to have a single driver to support multiple versions of the
same SoC.
The patch I proposed above does the same job as the original patch
proposed by Jisheng and also allows hisi driver to call the moved
code.
Do you see anything wrong with it?
Thanks
Gab
On Fri, Apr 08, 2016 at 01:33:28PM +0000, Gabriele Paoloni wrote:
Hi Bjorn
Many thanks for your reply
quoted
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas at kernel.org]
Sent: 07 April 2016 15:06
To: Gabriele Paoloni
Cc: Jisheng Zhang; jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com; linux-pci at vger.kernel.org; linux-
kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code
to dw_pcie_setup_rc()
On Thu, Apr 07, 2016 at 10:06:45AM +0000, Gabriele Paoloni wrote:
quoted
Hi Jisheng
quoted
-----Original Message-----
From: Jisheng Zhang [mailto:jszhang at marvell.com]
Sent: 07 April 2016 09:35
To: Gabriele Paoloni
Cc: jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com; linux-pci at vger.kernel.org; linux-
kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup
code
quoted
quoted
to dw_pcie_setup_rc()
Hi Gabriele,
On Thu, 7 Apr 2016 08:20:28 +0000 Gabriele Paoloni wrote:
quoted
Hi Jisheng
Thanks for your reply
quoted
-----Original Message-----
From: Jisheng Zhang [mailto:jszhang at marvell.com]
Sent: 07 April 2016 03:38
To: Gabriele Paoloni; jingoohan1 at gmail.com;
pratyush.anand at gmail.com;
quoted
quoted
bhelgaas at google.com
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org;
dw_pcie_setup_rc()
dw_pcie_setup_rc(), as its name indicates, setups the RC.
But
quoted
quoted
quoted
quoted
current
quoted
quoted
dw_pcie_host_init() also contains some necessary rc setup
code.
quoted
quoted
quoted
quoted
quoted
quoted
Another reason: the host may lost power during suspend to
ram,
quoted
quoted
the
quoted
quoted
RC
quoted
quoted
need to be re-setup after resume. The rc can't be correctly
resumed
quoted
quoted
quoted
quoted
without the rc setup code in dw_pcie_host_init().
So this patch moves the code to dw_pcie_setup_rc() to
address
quoted
quoted
the
quoted
quoted
above
quoted
quoted
two issues. After this patch, each pcie designware driver
users
quoted
quoted
quoted
quoted
could
quoted
quoted
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
quoted
quoted
quoted
quoted
quoted
dw_pcie_setup_rc(), we only call dw_pcie_host_init().
Thanks for the information. So pcie-hisi rely on UEFI to do
something
quoted
quoted
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...
quoted
Is it acceptable that pcie-hisi adds a call to
dw_pcie_setup_rc()
quoted
quoted
in
quoted
quoted
hisi_add_pcie_port()?
I don't think so...that would try to overwrite what is already
set by
quoted
quoted
quoted
the bootloader; so it is wrong in principle and maybe it can lead
to
quoted
quoted
quoted
undefined behaviours...
make sense! This commit is intend to re-setup the rc when waken
from
quoted
quoted
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!
@@ -434,7 +434,6 @@ int dw_pcie_host_init(struct pcie_port *pp)structplatform_device*pdev=to_platform_device(pp->dev);structpci_bus*bus,*child;structresource*cfg_res;-u32val;inti,ret;LIST_HEAD(res);structresource_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);-/*-*Iftheplatformprovides->rd_other_conf,itmeansthe
platform
quoted
- * 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);
quoted
-- 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,
@@ -725,6 +705,29 @@ static struct pci_ops dw_pcie_ops = { .write = dw_pcie_wr_conf, };+void dw_pcie_setup_own_cfg(struct pcie_port *pp)+{+ u32 val;+ /*+ * If the platform provides ->rd_other_conf, it means the
platform
quoted
+ * 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,
What's the hisi plan for resuming after suspend-to-RAM? How does the
RC get reprogrammed after it loses all its state?
PM is not part of the driver yet. This is planned for near
future release so haven't made such considerations yet
quoted
What would break if hisi did call dw_pcie_setup_rc()? I know you said
it would overwrite what the bootloader already did, which is true.
I am try to figure this out now with our HW team.
quoted
But hisi does call dw_pcie_host_init(), so it reads pp->mem (which
determines pp->mem_base) and pp->lanes from the DT. Other drivers
then call dw_pcie_setup_rc() which programs the RC based on
pp->mem_base and pp->lanes. So hisi assumes UEFI programmed the RC to
match the DT, while the other drivers read the DT and program the RC
to match. The latter seems more robust because it enforces the
consistency rather than relying on it.
Yes I agree with you, however we have preferred to move RC config to
BIOS to have a single driver to support multiple versions of the
same SoC.
I think there are two reasonable approaches:
1) A single generic driver that doesn't have any knowledge about the
chipset registers; it uses run-time firmware interfaces to manage
the bridge. The ACPI pci_root.c driver is the best example so far
and works very well. It supports basically all x86 and ia64
chipsets and requires no kernel work for new ones.
2) Native drivers specific to each chipset. These may get
configuration information from DT, but they do their own
register-level programming of the device without run-time help from
firmware.
I think hisi is a native driver because it uses hip05/hip06 registers
to check link state and perform config operations. And apparently you
rely on the ATU, BAR, class, and link width programming currently done
in dw_pcie_host_init(). But you want to rely on pre-boot firmware to
set up the link. That doesn't make sense to me -- if the driver wants
to twiddle the registers, it should know how to do it all. I don't
see how you can reasonably manage this half-way approach.
The patch I proposed above does the same job as the original patch
proposed by Jisheng and also allows hisi driver to call the moved
code.
Do you see anything wrong with it?
Only that it makes the structure more complicated and we haven't
identified a corresponding benefit yet.
Bjorn
What's the hisi plan for resuming after suspend-to-RAM? How does
the
quoted
quoted
RC get reprogrammed after it loses all its state?
PM is not part of the driver yet. This is planned for near
future release so haven't made such considerations yet
quoted
What would break if hisi did call dw_pcie_setup_rc()? I know you
said
quoted
quoted
it would overwrite what the bootloader already did, which is true.
I am try to figure this out now with our HW team.
quoted
But hisi does call dw_pcie_host_init(), so it reads pp->mem (which
determines pp->mem_base) and pp->lanes from the DT. Other drivers
then call dw_pcie_setup_rc() which programs the RC based on
pp->mem_base and pp->lanes. So hisi assumes UEFI programmed the RC
to
quoted
quoted
match the DT, while the other drivers read the DT and program the
RC
quoted
quoted
to match. The latter seems more robust because it enforces the
consistency rather than relying on it.
Yes I agree with you, however we have preferred to move RC config to
BIOS to have a single driver to support multiple versions of the
same SoC.
I think there are two reasonable approaches:
1) A single generic driver that doesn't have any knowledge about the
chipset registers; it uses run-time firmware interfaces to manage
the bridge. The ACPI pci_root.c driver is the best example so far
and works very well. It supports basically all x86 and ia64
chipsets and requires no kernel work for new ones.
2) Native drivers specific to each chipset. These may get
configuration information from DT, but they do their own
register-level programming of the device without run-time help from
firmware.
I think hisi is a native driver because it uses hip05/hip06 registers
to check link state and perform config operations. And apparently you
rely on the ATU, BAR, class, and link width programming currently done
in dw_pcie_host_init(). But you want to rely on pre-boot firmware to
set up the link. That doesn't make sense to me -- if the driver wants
to twiddle the registers, it should know how to do it all. I don't
see how you can reasonably manage this half-way approach.
quoted
The patch I proposed above does the same job as the original patch
proposed by Jisheng and also allows hisi driver to call the moved
code.
Do you see anything wrong with it?
Only that it makes the structure more complicated and we haven't
identified a corresponding benefit yet.
Finally I have checked that assigning .host_init function pointer
in our driver to call dw_pcie_setup_rc() will not affect the values
already set by BIOS.
Also I agree with you that a hybrid approach is not ideal.
So I will update the driver to call dw_pcie_setup_rc() from
.host_init and ask the BIOS team to update the firmware for next
releases (the driver will be backward compatible anyway).
Also during my investigation I have noticed that in dw_pcie_setup_rc()
http://lxr.free-electrons.com/source/drivers/pci/host/pcie-designware.c#L762
we use pp->mem_base rather than pp->mem_bus_addr to setup
memory base and memory limit in the Type1 header...I think this
is wrong right?
Also I do not see why this code is needed at all since we overwrite
this register when we call pci_bus_assign_resources(bus) that
will end up in calling pci_setup_bridge() and then
pci_setup_bridge_mmio()...?
Many Thanks
Gab
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, April 12, 2016 6:44 PM, Gabriele Paoloni wrote:
Hi Bjorn
[...]
quoted
quoted
quoted
What's the hisi plan for resuming after suspend-to-RAM? How does
the
quoted
quoted
RC get reprogrammed after it loses all its state?
PM is not part of the driver yet. This is planned for near
future release so haven't made such considerations yet
quoted
What would break if hisi did call dw_pcie_setup_rc()? I know you
said
quoted
quoted
it would overwrite what the bootloader already did, which is true.
I am try to figure this out now with our HW team.
quoted
But hisi does call dw_pcie_host_init(), so it reads pp->mem (which
determines pp->mem_base) and pp->lanes from the DT. Other drivers
then call dw_pcie_setup_rc() which programs the RC based on
pp->mem_base and pp->lanes. So hisi assumes UEFI programmed the RC
to
quoted
quoted
match the DT, while the other drivers read the DT and program the
RC
quoted
quoted
to match. The latter seems more robust because it enforces the
consistency rather than relying on it.
Yes I agree with you, however we have preferred to move RC config to
BIOS to have a single driver to support multiple versions of the
same SoC.
I think there are two reasonable approaches:
1) A single generic driver that doesn't have any knowledge about the
chipset registers; it uses run-time firmware interfaces to manage
the bridge. The ACPI pci_root.c driver is the best example so far
and works very well. It supports basically all x86 and ia64
chipsets and requires no kernel work for new ones.
2) Native drivers specific to each chipset. These may get
configuration information from DT, but they do their own
register-level programming of the device without run-time help from
firmware.
I think hisi is a native driver because it uses hip05/hip06 registers
to check link state and perform config operations. And apparently you
rely on the ATU, BAR, class, and link width programming currently done
in dw_pcie_host_init(). But you want to rely on pre-boot firmware to
set up the link. That doesn't make sense to me -- if the driver wants
to twiddle the registers, it should know how to do it all. I don't
see how you can reasonably manage this half-way approach.
quoted
The patch I proposed above does the same job as the original patch
proposed by Jisheng and also allows hisi driver to call the moved
code.
Do you see anything wrong with it?
Only that it makes the structure more complicated and we haven't
identified a corresponding benefit yet.
Finally I have checked that assigning .host_init function pointer
in our driver to call dw_pcie_setup_rc() will not affect the values
already set by BIOS.
Also I agree with you that a hybrid approach is not ideal.
I also agree with Bjorn's opinion.
As far as I know, two approaches are reasonable.
In the case of using UEFI, how about using 'pci-host-generic.c'?
You may consult with Linaro guys for this issue.
Good luck.
Best regards,
Jingoo Han
So I will update the driver to call dw_pcie_setup_rc() from
.host_init and ask the BIOS team to update the firmware for next
releases (the driver will be backward compatible anyway).
Also during my investigation I have noticed that in dw_pcie_setup_rc()
http://lxr.free-electrons.com/source/drivers/pci/host/pcie-designware.c#L762
we use pp->mem_base rather than pp->mem_bus_addr to setup
memory base and memory limit in the Type1 header...I think this
is wrong right?
Also I do not see why this code is needed at all since we overwrite
this register when we call pci_bus_assign_resources(bus) that
will end up in calling pci_setup_bridge() and then
pci_setup_bridge_mmio()...?
Many Thanks
Gab
quoted
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
-----Original Message-----
From: Jingoo Han [mailto:jingoohan1 at gmail.com]
Sent: 13 April 2016 06:52
To: Gabriele Paoloni; 'Bjorn Helgaas'
Cc: 'Jisheng Zhang'; pratyush.anand at gmail.com; bhelgaas at google.com;
linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
kernel at lists.infradead.org; 'Jingoo Han'
Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code
to dw_pcie_setup_rc()
On Tuesday, April 12, 2016 6:44 PM, Gabriele Paoloni wrote:
quoted
Hi Bjorn
[...]
quoted
quoted
quoted
What's the hisi plan for resuming after suspend-to-RAM? How
does
quoted
quoted
the
quoted
quoted
RC get reprogrammed after it loses all its state?
PM is not part of the driver yet. This is planned for near
future release so haven't made such considerations yet
quoted
What would break if hisi did call dw_pcie_setup_rc()? I know
you
quoted
quoted
said
quoted
quoted
it would overwrite what the bootloader already did, which is
true.
quoted
quoted
quoted
I am try to figure this out now with our HW team.
quoted
But hisi does call dw_pcie_host_init(), so it reads pp->mem
(which
quoted
quoted
quoted
quoted
determines pp->mem_base) and pp->lanes from the DT. Other
drivers
quoted
quoted
quoted
quoted
then call dw_pcie_setup_rc() which programs the RC based on
pp->mem_base and pp->lanes. So hisi assumes UEFI programmed
the RC
quoted
quoted
to
quoted
quoted
match the DT, while the other drivers read the DT and program
the
quoted
quoted
RC
quoted
quoted
to match. The latter seems more robust because it enforces the
consistency rather than relying on it.
Yes I agree with you, however we have preferred to move RC config
to
quoted
quoted
quoted
BIOS to have a single driver to support multiple versions of the
same SoC.
I think there are two reasonable approaches:
1) A single generic driver that doesn't have any knowledge about
the
quoted
quoted
chipset registers; it uses run-time firmware interfaces to manage
the bridge. The ACPI pci_root.c driver is the best example so
far
quoted
quoted
and works very well. It supports basically all x86 and ia64
chipsets and requires no kernel work for new ones.
2) Native drivers specific to each chipset. These may get
configuration information from DT, but they do their own
register-level programming of the device without run-time help
from
quoted
quoted
firmware.
I think hisi is a native driver because it uses hip05/hip06
registers
quoted
quoted
to check link state and perform config operations. And apparently
you
quoted
quoted
rely on the ATU, BAR, class, and link width programming currently
done
quoted
quoted
in dw_pcie_host_init(). But you want to rely on pre-boot firmware
to
quoted
quoted
set up the link. That doesn't make sense to me -- if the driver
wants
quoted
quoted
to twiddle the registers, it should know how to do it all. I don't
see how you can reasonably manage this half-way approach.
quoted
The patch I proposed above does the same job as the original
patch
quoted
quoted
quoted
proposed by Jisheng and also allows hisi driver to call the moved
code.
Do you see anything wrong with it?
Only that it makes the structure more complicated and we haven't
identified a corresponding benefit yet.
Finally I have checked that assigning .host_init function pointer
in our driver to call dw_pcie_setup_rc() will not affect the values
already set by BIOS.
Also I agree with you that a hybrid approach is not ideal.
I also agree with Bjorn's opinion.
As far as I know, two approaches are reasonable.
In the case of using UEFI, how about using 'pci-host-generic.c'?
You may consult with Linaro guys for this issue.
Many thanks for your suggestion, I'll take it into account for next
releases
Good luck.
Best regards,
Jingoo Han
quoted
So I will update the driver to call dw_pcie_setup_rc() from
.host_init and ask the BIOS team to update the firmware for next
releases (the driver will be backward compatible anyway).
Also during my investigation I have noticed that in
we use pp->mem_base rather than pp->mem_bus_addr to setup
memory base and memory limit in the Type1 header...I think this
is wrong right?
Also I do not see why this code is needed at all since we overwrite
this register when we call pci_bus_assign_resources(bus) that
will end up in calling pci_setup_bridge() and then
pci_setup_bridge_mmio()...?
Do you have any comment on this issue above?
quoted
Many Thanks
Gab
quoted
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-
On Wednesday, April 13, 2016 4:58 PM, Gabriele Paoloni wrote:
Hi Jingoo
On 13 April 2016 06:52, Jingoo Han wrote:
quoted
On Tuesday, April 12, 2016 6:44 PM, Gabriele Paoloni wrote:
quoted
Hi Bjorn
[...]
quoted
quoted
quoted
What's the hisi plan for resuming after suspend-to-RAM? How
does
quoted
quoted
the
quoted
quoted
RC get reprogrammed after it loses all its state?
PM is not part of the driver yet. This is planned for near
future release so haven't made such considerations yet
quoted
What would break if hisi did call dw_pcie_setup_rc()? I know
you
quoted
quoted
said
quoted
quoted
it would overwrite what the bootloader already did, which is
true.
quoted
quoted
quoted
I am try to figure this out now with our HW team.
quoted
But hisi does call dw_pcie_host_init(), so it reads pp->mem
(which
quoted
quoted
quoted
quoted
determines pp->mem_base) and pp->lanes from the DT. Other
drivers
quoted
quoted
quoted
quoted
then call dw_pcie_setup_rc() which programs the RC based on
pp->mem_base and pp->lanes. So hisi assumes UEFI programmed
the RC
quoted
quoted
to
quoted
quoted
match the DT, while the other drivers read the DT and program
the
quoted
quoted
RC
quoted
quoted
to match. The latter seems more robust because it enforces the
consistency rather than relying on it.
Yes I agree with you, however we have preferred to move RC config
to
quoted
quoted
quoted
BIOS to have a single driver to support multiple versions of the
same SoC.
I think there are two reasonable approaches:
1) A single generic driver that doesn't have any knowledge about
the
quoted
quoted
chipset registers; it uses run-time firmware interfaces to manage
the bridge. The ACPI pci_root.c driver is the best example so
far
quoted
quoted
and works very well. It supports basically all x86 and ia64
chipsets and requires no kernel work for new ones.
2) Native drivers specific to each chipset. These may get
configuration information from DT, but they do their own
register-level programming of the device without run-time help
from
quoted
quoted
firmware.
I think hisi is a native driver because it uses hip05/hip06
registers
quoted
quoted
to check link state and perform config operations. And apparently
you
quoted
quoted
rely on the ATU, BAR, class, and link width programming currently
done
quoted
quoted
in dw_pcie_host_init(). But you want to rely on pre-boot firmware
to
quoted
quoted
set up the link. That doesn't make sense to me -- if the driver
wants
quoted
quoted
to twiddle the registers, it should know how to do it all. I don't
see how you can reasonably manage this half-way approach.
quoted
The patch I proposed above does the same job as the original
patch
quoted
quoted
quoted
proposed by Jisheng and also allows hisi driver to call the moved
code.
Do you see anything wrong with it?
Only that it makes the structure more complicated and we haven't
identified a corresponding benefit yet.
Finally I have checked that assigning .host_init function pointer
in our driver to call dw_pcie_setup_rc() will not affect the values
already set by BIOS.
Also I agree with you that a hybrid approach is not ideal.
I also agree with Bjorn's opinion.
As far as I know, two approaches are reasonable.
In the case of using UEFI, how about using 'pci-host-generic.c'?
You may consult with Linaro guys for this issue.
Many thanks for your suggestion, I'll take it into account for next
releases
quoted
Good luck.
Best regards,
Jingoo Han
quoted
So I will update the driver to call dw_pcie_setup_rc() from
.host_init and ask the BIOS team to update the firmware for next
releases (the driver will be backward compatible anyway).
Also during my investigation I have noticed that in
we use pp->mem_base rather than pp->mem_bus_addr to setup
memory base and memory limit in the Type1 header...I think this
is wrong right?
Also I do not see why this code is needed at all since we overwrite
this register when we call pci_bus_assign_resources(bus) that
will end up in calling pci_setup_bridge() and then
pci_setup_bridge_mmio()...?
Do you have any comment on this issue above?
Sorry, I am not sure.
However, there are some redundant codes like this.
At that time, I was not able to decide to remove these codes.
Maybe, Pratyush Anand or other guys would give opinions about this.
Best regards,
Jingoo Han
quoted
quoted
Many Thanks
Gab
quoted
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-
Hi Gabriele,
On Thu, Apr 14, 2016 at 5:22 PM, Jingoo Han [off-list ref] wrote:
On Wednesday, April 13, 2016 4:58 PM, Gabriele Paoloni wrote:
quoted
Hi Jingoo
On 13 April 2016 06:52, Jingoo Han wrote:
quoted
On Tuesday, April 12, 2016 6:44 PM, Gabriele Paoloni wrote:
[...]
quoted
quoted
quoted
So I will update the driver to call dw_pcie_setup_rc() from
.host_init and ask the BIOS team to update the firmware for next
releases (the driver will be backward compatible anyway).
Also during my investigation I have noticed that in
we use pp->mem_base rather than pp->mem_bus_addr to setup
memory base and memory limit in the Type1 header...I think this
is wrong right?
Yes. RC's "memory base" and "memory limit" should be governed by PCI
addresses and not CPU addresses. So, it should use pp->mem_bus_addr.
quoted
quoted
quoted
Also I do not see why this code is needed at all since we overwrite
this register when we call pci_bus_assign_resources(bus) that
will end up in calling pci_setup_bridge() and then
pci_setup_bridge_mmio()...?
Do you have any comment on this issue above?
Probably thats why things are working.
Thanks for finding it. I think, /* setup memory base, memory limit */
hunk can be removed from dw_pcie_setup_rc.
~Pratyush
-----Original Message-----
From: Pratyush Anand [mailto:pratyush.anand at gmail.com]
Sent: 14 April 2016 14:08
To: Jingoo Han; Gabriele Paoloni
Cc: Bjorn Helgaas; Jisheng Zhang; Bjorn Helgaas; linux-
pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
kernel at lists.infradead.org
Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code
to dw_pcie_setup_rc()
Hi Gabriele,
On Thu, Apr 14, 2016 at 5:22 PM, Jingoo Han [off-list ref]
wrote:
quoted
On Wednesday, April 13, 2016 4:58 PM, Gabriele Paoloni wrote:
quoted
Hi Jingoo
On 13 April 2016 06:52, Jingoo Han wrote:
quoted
On Tuesday, April 12, 2016 6:44 PM, Gabriele Paoloni wrote:
[...]
quoted
quoted
quoted
quoted
So I will update the driver to call dw_pcie_setup_rc() from
.host_init and ask the BIOS team to update the firmware for next
releases (the driver will be backward compatible anyway).
Also during my investigation I have noticed that in
we use pp->mem_base rather than pp->mem_bus_addr to setup
memory base and memory limit in the Type1 header...I think this
is wrong right?
Yes. RC's "memory base" and "memory limit" should be governed by PCI
addresses and not CPU addresses. So, it should use pp->mem_bus_addr.
quoted
quoted
quoted
quoted
Also I do not see why this code is needed at all since we
overwrite
quoted
quoted
quoted
quoted
this register when we call pci_bus_assign_resources(bus) that
will end up in calling pci_setup_bridge() and then
pci_setup_bridge_mmio()...?
Do you have any comment on this issue above?
Probably thats why things are working.
Thanks for finding it. I think, /* setup memory base, memory limit */
hunk can be removed from dw_pcie_setup_rc.
Great, I'll send out a patch to remove this
Thanks
Gab
On Tue, Apr 12, 2016 at 09:43:32AM +0000, Gabriele Paoloni wrote:
Hi Bjorn
[...]
quoted
quoted
quoted
What's the hisi plan for resuming after suspend-to-RAM? How does
the
quoted
quoted
RC get reprogrammed after it loses all its state?
PM is not part of the driver yet. This is planned for near
future release so haven't made such considerations yet
quoted
What would break if hisi did call dw_pcie_setup_rc()? I know you
said
quoted
quoted
it would overwrite what the bootloader already did, which is true.
I am try to figure this out now with our HW team.
quoted
But hisi does call dw_pcie_host_init(), so it reads pp->mem (which
determines pp->mem_base) and pp->lanes from the DT. Other drivers
then call dw_pcie_setup_rc() which programs the RC based on
pp->mem_base and pp->lanes. So hisi assumes UEFI programmed the RC
to
quoted
quoted
match the DT, while the other drivers read the DT and program the
RC
quoted
quoted
to match. The latter seems more robust because it enforces the
consistency rather than relying on it.
Yes I agree with you, however we have preferred to move RC config to
BIOS to have a single driver to support multiple versions of the
same SoC.
I think there are two reasonable approaches:
1) A single generic driver that doesn't have any knowledge about the
chipset registers; it uses run-time firmware interfaces to manage
the bridge. The ACPI pci_root.c driver is the best example so far
and works very well. It supports basically all x86 and ia64
chipsets and requires no kernel work for new ones.
2) Native drivers specific to each chipset. These may get
configuration information from DT, but they do their own
register-level programming of the device without run-time help from
firmware.
I think hisi is a native driver because it uses hip05/hip06 registers
to check link state and perform config operations. And apparently you
rely on the ATU, BAR, class, and link width programming currently done
in dw_pcie_host_init(). But you want to rely on pre-boot firmware to
set up the link. That doesn't make sense to me -- if the driver wants
to twiddle the registers, it should know how to do it all. I don't
see how you can reasonably manage this half-way approach.
quoted
The patch I proposed above does the same job as the original patch
proposed by Jisheng and also allows hisi driver to call the moved
code.
Do you see anything wrong with it?
Only that it makes the structure more complicated and we haven't
identified a corresponding benefit yet.
Finally I have checked that assigning .host_init function pointer
in our driver to call dw_pcie_setup_rc() will not affect the values
already set by BIOS.
Also I agree with you that a hybrid approach is not ideal.
So I will update the driver to call dw_pcie_setup_rc() from
.host_init and ask the BIOS team to update the firmware for next
releases (the driver will be backward compatible anyway).
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas at kernel.org]
Sent: 21 April 2016 16:49
To: Gabriele Paoloni
Cc: Jisheng Zhang; jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com; linux-pci at vger.kernel.org; linux-
kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code
to dw_pcie_setup_rc()
On Tue, Apr 12, 2016 at 09:43:32AM +0000, Gabriele Paoloni wrote:
quoted
Hi Bjorn
[...]
quoted
quoted
quoted
What's the hisi plan for resuming after suspend-to-RAM? How
does
quoted
quoted
the
quoted
quoted
RC get reprogrammed after it loses all its state?
PM is not part of the driver yet. This is planned for near
future release so haven't made such considerations yet
quoted
What would break if hisi did call dw_pcie_setup_rc()? I know
you
quoted
quoted
said
quoted
quoted
it would overwrite what the bootloader already did, which is
true.
quoted
quoted
quoted
I am try to figure this out now with our HW team.
quoted
But hisi does call dw_pcie_host_init(), so it reads pp->mem
(which
quoted
quoted
quoted
quoted
determines pp->mem_base) and pp->lanes from the DT. Other
drivers
quoted
quoted
quoted
quoted
then call dw_pcie_setup_rc() which programs the RC based on
pp->mem_base and pp->lanes. So hisi assumes UEFI programmed
the RC
quoted
quoted
to
quoted
quoted
match the DT, while the other drivers read the DT and program
the
quoted
quoted
RC
quoted
quoted
to match. The latter seems more robust because it enforces the
consistency rather than relying on it.
Yes I agree with you, however we have preferred to move RC config
to
quoted
quoted
quoted
BIOS to have a single driver to support multiple versions of the
same SoC.
I think there are two reasonable approaches:
1) A single generic driver that doesn't have any knowledge about
the
quoted
quoted
chipset registers; it uses run-time firmware interfaces to manage
the bridge. The ACPI pci_root.c driver is the best example so
far
quoted
quoted
and works very well. It supports basically all x86 and ia64
chipsets and requires no kernel work for new ones.
2) Native drivers specific to each chipset. These may get
configuration information from DT, but they do their own
register-level programming of the device without run-time help
from
quoted
quoted
firmware.
I think hisi is a native driver because it uses hip05/hip06
registers
quoted
quoted
to check link state and perform config operations. And apparently
you
quoted
quoted
rely on the ATU, BAR, class, and link width programming currently
done
quoted
quoted
in dw_pcie_host_init(). But you want to rely on pre-boot firmware
to
quoted
quoted
set up the link. That doesn't make sense to me -- if the driver
wants
quoted
quoted
to twiddle the registers, it should know how to do it all. I don't
see how you can reasonably manage this half-way approach.
quoted
The patch I proposed above does the same job as the original
patch
quoted
quoted
quoted
proposed by Jisheng and also allows hisi driver to call the moved
code.
Do you see anything wrong with it?
Only that it makes the structure more complicated and we haven't
identified a corresponding benefit yet.
Finally I have checked that assigning .host_init function pointer
in our driver to call dw_pcie_setup_rc() will not affect the values
already set by BIOS.
Also I agree with you that a hybrid approach is not ideal.
So I will update the driver to call dw_pcie_setup_rc() from
.host_init and ask the BIOS team to update the firmware for next
releases (the driver will be backward compatible anyway).