From: Jianjun Wang <hidden> Date: 2021-02-24 06:13:47
These series patches add pcie-mediatek-gen3.c and dt-bindings file to
support new generation PCIe controller.
Changes in v8:
1. Add irq_clock to protect IRQ register access;
2. Mask all INTx interrupt when startup port;
3. Remove activate/deactivate callbacks from bottom_domain_ops;
4. Add unmask/mask callbacks in mtk_msi_bottom_irq_chip;
5. Add property information for reg-names.
Changes in v7:
1. Split the driver patch to core PCIe, INTx, MSI and PM patches;
2. Reshape MSI init and handle flow, use msi_bottom_domain to cover all sets;
3. Replace readl/writel with their relaxed version;
4. Add MSI description in binding document;
5. Add pl_250m clock in binding document.
Changes in v6:
1. Export pci_pio_to_address() to support compiling as kernel module;
2. Replace usleep_range(100 * 1000, 120 * 1000) with msleep(100);
3. Replace dev_notice with dev_err;
4. Fix MSI get hwirq flow;
5. Fix warning for possible recursive locking in mtk_pcie_set_affinity.
Changes in v5:
1. Remove unused macros
2. Modify the config read/write callbacks, set the config byte field
in TLP header and use pci_generic_config_read32/write32
to access the config space
3. Fix the settings of translation window, both MEM and IO regions
works properly
4. Fix typos
Changes in v4:
1. Fix PCIe power up/down flow
2. Use "mac" and "phy" for reset names
3. Add clock names
4. Fix the variables type
Changes in v3:
1. Remove standard property in binding document
2. Return error number when get_optional* API throws an error
3. Use the bulk clk APIs
Changes in v2:
1. Fix the typo of dt-bindings patch
2. Remove the unnecessary properties in binding document
3. dispos the irq mappings of msi top domain when irq teardown
Jianjun Wang (7):
dt-bindings: PCI: mediatek-gen3: Add YAML schema
PCI: Export pci_pio_to_address() for module use
PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
PCI: mediatek-gen3: Add INTx support
PCI: mediatek-gen3: Add MSI support
PCI: mediatek-gen3: Add system PM support
MAINTAINERS: Add Jianjun Wang as MediaTek PCI co-maintainer
.../bindings/pci/mediatek-pcie-gen3.yaml | 181 ++++
MAINTAINERS | 1 +
drivers/pci/controller/Kconfig | 13 +
drivers/pci/controller/Makefile | 1 +
drivers/pci/controller/pcie-mediatek-gen3.c | 994 ++++++++++++++++++
drivers/pci/pci.c | 1 +
6 files changed, 1191 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c
--
2.25.1
From: Jianjun Wang <hidden> Date: 2021-02-24 06:14:14
This interface will be used by PCI host drivers for PIO translation,
export it to support compiling those drivers as kernel modules.
Signed-off-by: Jianjun Wang <redacted>
---
drivers/pci/pci.c | 1 +
1 file changed, 1 insertion(+)
From: Jianjun Wang <hidden> Date: 2021-02-24 06:15:23
Add suspend_noirq and resume_noirq callback functions to implement
PM system suspend hooks for MediaTek Gen3 PCIe controller.
When system suspend, trigger the PCIe link to L2 state and pull down
the PERST# pin, gating the clocks of MAC layer and power off the
physical layer for the sake of power saving.
When system resum, the PCIe link should be re-established and the
related control register values should be restored.
Signed-off-by: Jianjun Wang <redacted>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
drivers/pci/controller/pcie-mediatek-gen3.c | 84 +++++++++++++++++++++
1 file changed, 84 insertions(+)
@@ -892,6 +898,83 @@ static int mtk_pcie_remove(struct platform_device *pdev)return0;}+staticint__maybe_unusedmtk_pcie_turn_off_link(structmtk_pcie_port*port)+{+u32val;++val=readl_relaxed(port->base+PCIE_ICMD_PM_REG);+val|=PCIE_TURN_OFF_LINK;+writel_relaxed(val,port->base+PCIE_ICMD_PM_REG);++/* Check the link is L2 */+returnreadl_poll_timeout(port->base+PCIE_LTSSM_STATUS_REG,val,+(PCIE_LTSSM_STATE(val)==+PCIE_LTSSM_STATE_L2_IDLE),20,+50*USEC_PER_MSEC);+}++staticint__maybe_unusedmtk_pcie_suspend_noirq(structdevice*dev)+{+structmtk_pcie_port*port=dev_get_drvdata(dev);+interr;+u32val;++/* Trigger link to L2 state */+err=mtk_pcie_turn_off_link(port);+if(err){+dev_err(port->dev,"can not enter L2 state\n");+returnerr;+}++/* Pull down the PERST# pin */+val=readl_relaxed(port->base+PCIE_RST_CTRL_REG);+val|=PCIE_PE_RSTB;+writel_relaxed(val,port->base+PCIE_RST_CTRL_REG);++dev_dbg(port->dev,"enter L2 state success");++clk_bulk_disable_unprepare(port->num_clks,port->clks);++reset_control_assert(port->mac_reset);++phy_power_off(port->phy);+reset_control_assert(port->phy_reset);++return0;+}++staticint__maybe_unusedmtk_pcie_resume_noirq(structdevice*dev)+{+structmtk_pcie_port*port=dev_get_drvdata(dev);+interr;++reset_control_deassert(port->phy_reset);+phy_power_on(port->phy);++reset_control_deassert(port->mac_reset);++err=clk_bulk_prepare_enable(port->num_clks,port->clks);+if(err){+dev_dbg(dev,"failed to enable PCIe clocks\n");+returnerr;+}++err=mtk_pcie_startup_port(port);+if(err){+dev_err(port->dev,"resume failed\n");+returnerr;+}++dev_dbg(port->dev,"resume done\n");++return0;+}++staticconststructdev_pm_opsmtk_pcie_pm_ops={+SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,+mtk_pcie_resume_noirq)+};+staticconststructof_device_idmtk_pcie_of_match[]={{.compatible="mediatek,mt8192-pcie"},{},
@@ -197,6 +235,35 @@ static int mtk_pcie_set_trans_table(struct mtk_pcie_port *port,return0;}+staticvoidmtk_pcie_enable_msi(structmtk_pcie_port*port)+{+inti;+u32val;++val=readl_relaxed(port->base+PCIE_MSI_SET_ENABLE_REG);+val|=PCIE_MSI_SET_ENABLE;+writel_relaxed(val,port->base+PCIE_MSI_SET_ENABLE_REG);++val=readl_relaxed(port->base+PCIE_INT_ENABLE_REG);+val|=PCIE_MSI_ENABLE;+writel_relaxed(val,port->base+PCIE_INT_ENABLE_REG);++for(i=0;i<PCIE_MSI_SET_NUM;i++){+structmtk_msi_set*msi_set=&port->msi_sets[i];++msi_set->base=port->base+PCIE_MSI_SET_BASE_REG++i*PCIE_MSI_SET_OFFSET;+msi_set->msg_addr=port->reg_base+PCIE_MSI_SET_BASE_REG++i*PCIE_MSI_SET_OFFSET;++/* Configure the MSI capture address */+writel_relaxed(lower_32_bits(msi_set->msg_addr),msi_set->base);+writel_relaxed(upper_32_bits(msi_set->msg_addr),+port->base+PCIE_MSI_SET_ADDR_HI_BASE++i*PCIE_MSI_SET_ADDR_HI_OFFSET);+}+}+staticintmtk_pcie_startup_port(structmtk_pcie_port*port){structresource_entry*entry;
@@ -247,6 +314,8 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)returnerr;}+mtk_pcie_enable_msi(port);+/* Set PCIe translation windows */resource_list_for_each_entry(entry,&host->windows){structresource*res=entry->res;
@@ -290,6 +359,148 @@ static int mtk_pcie_set_affinity(struct irq_data *data,return-EINVAL;}+staticvoidmtk_pcie_irq_mask(structirq_data*data)+{+pci_msi_mask_irq(data);+irq_chip_mask_parent(data);+}++staticvoidmtk_pcie_irq_unmask(structirq_data*data)+{+pci_msi_unmask_irq(data);+irq_chip_unmask_parent(data);+}++staticstructirq_chipmtk_msi_irq_chip={+.name="MSI",+.irq_enable=mtk_pcie_irq_unmask,+.irq_disable=mtk_pcie_irq_mask,+.irq_ack=irq_chip_ack_parent,+.irq_mask=mtk_pcie_irq_mask,+.irq_unmask=mtk_pcie_irq_unmask,+};++staticstructmsi_domain_infomtk_msi_domain_info={+.flags=(MSI_FLAG_USE_DEF_DOM_OPS|MSI_FLAG_PCI_MSIX|+MSI_FLAG_USE_DEF_CHIP_OPS|MSI_FLAG_MULTI_PCI_MSI),+.chip=&mtk_msi_irq_chip,+};++staticvoidmtk_compose_msi_msg(structirq_data*data,structmsi_msg*msg)+{+structmtk_msi_set*msi_set=irq_data_get_irq_chip_data(data);+structmtk_pcie_port*port=data->domain->host_data;+unsignedlonghwirq;++hwirq=data->hwirq%PCIE_MSI_IRQS_PER_SET;++msg->address_hi=upper_32_bits(msi_set->msg_addr);+msg->address_lo=lower_32_bits(msi_set->msg_addr);+msg->data=hwirq;+dev_dbg(port->dev,"msi#%#lx address_hi %#x address_lo %#x data %d\n",+hwirq,msg->address_hi,msg->address_lo,msg->data);+}++staticvoidmtk_msi_bottom_irq_ack(structirq_data*data)+{+structmtk_msi_set*msi_set=irq_data_get_irq_chip_data(data);+unsignedlonghwirq;++hwirq=data->hwirq%PCIE_MSI_IRQS_PER_SET;++writel_relaxed(BIT(hwirq),msi_set->base+PCIE_MSI_SET_STATUS_OFFSET);+}++staticvoidmtk_msi_bottom_irq_mask(structirq_data*data)+{+structmtk_msi_set*msi_set=irq_data_get_irq_chip_data(data);+structmtk_pcie_port*port=data->domain->host_data;+unsignedlonghwirq,flags;+u32val;++hwirq=data->hwirq%PCIE_MSI_IRQS_PER_SET;++raw_spin_lock_irqsave(&port->irq_lock,flags);+val=readl_relaxed(msi_set->base+PCIE_MSI_SET_ENABLE_OFFSET);+val&=~BIT(hwirq);+writel_relaxed(val,msi_set->base+PCIE_MSI_SET_ENABLE_OFFSET);+raw_spin_unlock_irqrestore(&port->irq_lock,flags);+}++staticvoidmtk_msi_bottom_irq_unmask(structirq_data*data)+{+structmtk_msi_set*msi_set=irq_data_get_irq_chip_data(data);+structmtk_pcie_port*port=data->domain->host_data;+unsignedlonghwirq,flags;+u32val;++hwirq=data->hwirq%PCIE_MSI_IRQS_PER_SET;++raw_spin_lock_irqsave(&port->irq_lock,flags);+val=readl_relaxed(msi_set->base+PCIE_MSI_SET_ENABLE_OFFSET);+val|=BIT(hwirq);+writel_relaxed(val,msi_set->base+PCIE_MSI_SET_ENABLE_OFFSET);+raw_spin_unlock_irqrestore(&port->irq_lock,flags);+}++staticstructirq_chipmtk_msi_bottom_irq_chip={+.irq_ack=mtk_msi_bottom_irq_ack,+.irq_mask=mtk_msi_bottom_irq_mask,+.irq_unmask=mtk_msi_bottom_irq_unmask,+.irq_compose_msi_msg=mtk_compose_msi_msg,+.irq_set_affinity=mtk_pcie_set_affinity,+.name="MSI",+};++staticintmtk_msi_bottom_domain_alloc(structirq_domain*domain,+unsignedintvirq,unsignedintnr_irqs,+void*arg)+{+structmtk_pcie_port*port=domain->host_data;+structmtk_msi_set*msi_set;+inti,hwirq,set_idx;++mutex_lock(&port->lock);++hwirq=bitmap_find_free_region(port->msi_irq_in_use,PCIE_MSI_IRQS_NUM,+order_base_2(nr_irqs));++mutex_unlock(&port->lock);++if(hwirq<0)+return-ENOSPC;++set_idx=hwirq/PCIE_MSI_IRQS_PER_SET;+msi_set=&port->msi_sets[set_idx];++for(i=0;i<nr_irqs;i++)+irq_domain_set_info(domain,virq+i,hwirq+i,+&mtk_msi_bottom_irq_chip,msi_set,+handle_edge_irq,NULL,NULL);++return0;+}++staticvoidmtk_msi_bottom_domain_free(structirq_domain*domain,+unsignedintvirq,unsignedintnr_irqs)+{+structmtk_pcie_port*port=domain->host_data;+structirq_data*data=irq_domain_get_irq_data(domain,virq);++mutex_lock(&port->lock);++bitmap_clear(port->msi_irq_in_use,data->hwirq,nr_irqs);++mutex_unlock(&port->lock);++irq_domain_free_irqs_common(domain,virq,nr_irqs);+}++staticconststructirq_domain_opsmtk_msi_bottom_domain_ops={+.alloc=mtk_msi_bottom_domain_alloc,+.free=mtk_msi_bottom_domain_free,+};+staticvoidmtk_intx_mask(structirq_data*data){structmtk_pcie_port*port=irq_data_get_irq_chip_data(data);
@@ -360,6 +571,7 @@ static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port){structdevice*dev=port->dev;structdevice_node*intc_node,*node=dev->of_node;+intret;raw_spin_lock_init(&port->irq_lock);
@@ -377,7 +589,34 @@ static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port)return-ENODEV;}+/* Setup MSI */+mutex_init(&port->lock);++port->msi_bottom_domain=irq_domain_add_linear(node,PCIE_MSI_IRQS_NUM,+&mtk_msi_bottom_domain_ops,port);+if(!port->msi_bottom_domain){+dev_info(dev,"failed to create MSI bottom domain\n");+ret=-ENODEV;+gotoerr_msi_bottom_domain;+}++port->msi_domain=pci_msi_create_irq_domain(dev->fwnode,+&mtk_msi_domain_info,+port->msi_bottom_domain);+if(!port->msi_domain){+dev_info(dev,"failed to create MSI domain\n");+ret=-ENODEV;+gotoerr_msi_domain;+}+return0;++err_msi_domain:+irq_domain_remove(port->msi_bottom_domain);+err_msi_bottom_domain:+irq_domain_remove(port->intx_domain);++returnret;}staticvoidmtk_pcie_irq_teardown(structmtk_pcie_port*port)
From: Jianjun Wang <hidden> Date: 2021-02-24 06:16:55
MediaTek's PCIe host controller has three generation HWs, the new
generation HW is an individual bridge, it supports Gen3 speed and
compatible with Gen2, Gen1 speed.
Add support for new Gen3 controller which can be found on MT8192.
Signed-off-by: Jianjun Wang <redacted>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
drivers/pci/controller/Kconfig | 13 +
drivers/pci/controller/Makefile | 1 +
drivers/pci/controller/pcie-mediatek-gen3.c | 457 ++++++++++++++++++++
3 files changed, 471 insertions(+)
create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c
From: Krzysztof Wilczyński <hidden> Date: 2021-02-24 14:51:48
Hi Jianjun,
Thank you for all the work here!
[...]
+ * struct mtk_pcie_port - PCIe port information
+ * @dev: pointer to PCIe device
+ * @base: IO mapped register base
+ * @reg_base: Physical register base
+ * @mac_reset: mac reset control
+ * @phy_reset: phy reset control
+ * @phy: PHY controller block
+ * @clks: PCIe clocks
+ * @num_clks: PCIe clocks count for this port
It would be "MAC" and "PHY" in the above.
[...]
+ * mtk_pcie_config_tlp_header
+ * @bus: PCI bus to query
+ * @devfn: device/function number
+ * @where: offset in config space
+ * @size: data size in TLP header
+ *
+ * Set byte enable field and device information in configuration TLP header.
This error message does not quite convey that the mtk_pcie_startup_port()
was the function that failed, which is only a part of what you have to do
to successfully resume.
+ dev_dbg(port->dev, "resume done\n");
A nitpick. Probably not needed, as lack of error message would mean
that the device resumed successfully after being suspended.
Krzysztof
From: Krzysztof Wilczyński <hidden> Date: 2021-02-24 15:15:15
Hi Jianjun,
[...]
+/**
+ * mtk_intx_eoi
+ * @data: pointer to chip specific data
+ *
+ * As an emulated level IRQ, its interrupt status will remain
+ * until the corresponding de-assert message is received; hence that
+ * the status can only be cleared when the interrupt has been serviced.
+ */
From: Jianjun Wang <hidden> Date: 2021-02-25 03:09:11
Hi Krzysztof,
Thanks for your review, I will fix these at next version.
Thanks.
On Wed, 2021-02-24 at 14:36 +0100, Krzysztof Wilczyński wrote:
Hi Jianjun,
Thank you for all the work here!
[...]
quoted
+ * struct mtk_pcie_port - PCIe port information
+ * @dev: pointer to PCIe device
+ * @base: IO mapped register base
+ * @reg_base: Physical register base
+ * @mac_reset: mac reset control
+ * @phy_reset: phy reset control
+ * @phy: PHY controller block
+ * @clks: PCIe clocks
+ * @num_clks: PCIe clocks count for this port
It would be "MAC" and "PHY" in the above.
[...]
quoted
+ * mtk_pcie_config_tlp_header
+ * @bus: PCI bus to query
+ * @devfn: device/function number
+ * @where: offset in config space
+ * @size: data size in TLP header
+ *
+ * Set byte enable field and device information in configuration TLP header.
From: Jianjun Wang <hidden> Date: 2021-02-25 03:11:53
Hi Krzysztof,
Thanks for your review, I will fix it at next version.
On Wed, 2021-02-24 at 15:24 +0100, Krzysztof Wilczyński wrote:
Hi Jianjun,
[...]
quoted
+/**
+ * mtk_intx_eoi
+ * @data: pointer to chip specific data
+ *
+ * As an emulated level IRQ, its interrupt status will remain
+ * until the corresponding de-assert message is received; hence that
+ * the status can only be cleared when the interrupt has been serviced.
+ */
This error message does not quite convey that the mtk_pcie_startup_port()
was the function that failed, which is only a part of what you have to do
to successfully resume.
quoted
+ dev_dbg(port->dev, "resume done\n");
A nitpick. Probably not needed, as lack of error message would mean
that the device resumed successfully after being suspended.
Krzysztof
From: Krzysztof Wilczyński <hidden> Date: 2021-02-25 22:02:36
Hi Jianjun,
[...]
Thanks for your review,
Thank YOU for all the work here!
[...]
quoted
quoted
Add suspend_noirq and resume_noirq callback functions to implement
PM system suspend hooks for MediaTek Gen3 PCIe controller.
So, "systems suspend" and "resume" hooks, correct?
The callback functions is suspend_noirq and resume_noirq, should I use
"systems suspend" and "resume" in the commit message?
[...]
What I meant was something along these lines:
Add suspend_noirq and resume_noirq callback functions to implement PM
system suspend and resume hooks for the MediaTek Gen3 PCIe controller.
When the system suspends, trigger the PCIe link to enter the L2 state
and pull down the PERST# pin, gating the clocks of the MAC layer, and
then power-off the physical layer to provide power-saving.
When the system resumes, the PCIe link should be re-established and the
related control register values should be restored.
The above is just a suggestion, thus feel tree to ignore it completely,
and it's heavily based on your original commit message.
Krzysztof
From: Jianjun Wang <hidden> Date: 2021-02-26 10:10:38
Hi Krzysztof,
Thanks for your suggestion, I will fix it in the next version.
On Thu, 2021-02-25 at 23:00 +0100, Krzysztof Wilczyński wrote:
Hi Jianjun,
[...]
quoted
Thanks for your review,
Thank YOU for all the work here!
[...]
quoted
quoted
quoted
Add suspend_noirq and resume_noirq callback functions to implement
PM system suspend hooks for MediaTek Gen3 PCIe controller.
So, "systems suspend" and "resume" hooks, correct?
The callback functions is suspend_noirq and resume_noirq, should I use
"systems suspend" and "resume" in the commit message?
[...]
What I meant was something along these lines:
Add suspend_noirq and resume_noirq callback functions to implement PM
system suspend and resume hooks for the MediaTek Gen3 PCIe controller.
When the system suspends, trigger the PCIe link to enter the L2 state
and pull down the PERST# pin, gating the clocks of the MAC layer, and
then power-off the physical layer to provide power-saving.
When the system resumes, the PCIe link should be re-established and the
related control register values should be restored.
The above is just a suggestion, thus feel tree to ignore it completely,
and it's heavily based on your original commit message.
Krzysztof
@@ -197,6 +235,35 @@ static int mtk_pcie_set_trans_table(struct mtk_pcie_port *port,return0;}+staticvoidmtk_pcie_enable_msi(structmtk_pcie_port*port)+{+inti;+u32val;++val=readl_relaxed(port->base+PCIE_MSI_SET_ENABLE_REG);+val|=PCIE_MSI_SET_ENABLE;+writel_relaxed(val,port->base+PCIE_MSI_SET_ENABLE_REG);++val=readl_relaxed(port->base+PCIE_INT_ENABLE_REG);+val|=PCIE_MSI_ENABLE;+writel_relaxed(val,port->base+PCIE_INT_ENABLE_REG);
Shouldn't you configure the capture addresses *before* enabling
things? Is there any need for locking here, given that you are
modifying global registers?
quoted hunk
+
+ for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
+ struct mtk_msi_set *msi_set = &port->msi_sets[i];
+
+ msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
+ i * PCIE_MSI_SET_OFFSET;
+ msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
+ i * PCIE_MSI_SET_OFFSET;
+
+ /* Configure the MSI capture address */
+ writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
+ writel_relaxed(upper_32_bits(msi_set->msg_addr),
+ port->base + PCIE_MSI_SET_ADDR_HI_BASE +
+ i * PCIE_MSI_SET_ADDR_HI_OFFSET);
+ }
+}
+
static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
{
struct resource_entry *entry;
@@ -247,6 +314,8 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port) return err; }+ mtk_pcie_enable_msi(port);+ /* Set PCIe translation windows */ resource_list_for_each_entry(entry, &host->windows) { struct resource *res = entry->res;
Isn't this write the same thing you have for EOI in the INTx case?
While I could understand your description in that case (this is a
resampling operation), I don't get what this does here. Either this is
also an EOI, but your initial description doesn't make sense, or it is
an Ack, and it should be moved to the right place.
Which one is it?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
@@ -199,6 +216,11 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)val|=PCI_CLASS(PCI_CLASS_BRIDGE_PCI<<8);writel_relaxed(val,port->base+PCIE_PCI_IDS_1);+/* Mask all INTx interrupts */+val=readl_relaxed(port->base+PCIE_INT_ENABLE_REG);+val&=~PCIE_INTX_ENABLE;+writel_relaxed(val,port->base+PCIE_INT_ENABLE_REG);+/* Assert all reset signals */val=readl_relaxed(port->base+PCIE_RST_CTRL_REG);val|=PCIE_MAC_RSTB|PCIE_PHY_RSTB|PCIE_BRG_RSTB|PCIE_PE_RSTB;
@@ -262,6 +284,154 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)return0;}+staticintmtk_pcie_set_affinity(structirq_data*data,+conststructcpumask*mask,boolforce)+{+return-EINVAL;+}++staticvoidmtk_intx_mask(structirq_data*data)+{+structmtk_pcie_port*port=irq_data_get_irq_chip_data(data);+unsignedlongflags;+u32val;++raw_spin_lock_irqsave(&port->irq_lock,flags);+val=readl_relaxed(port->base+PCIE_INT_ENABLE_REG);+val&=~BIT(data->hwirq+PCIE_INTX_SHIFT);+writel_relaxed(val,port->base+PCIE_INT_ENABLE_REG);+raw_spin_unlock_irqrestore(&port->irq_lock,flags);+}++staticvoidmtk_intx_unmask(structirq_data*data)+{+structmtk_pcie_port*port=irq_data_get_irq_chip_data(data);+unsignedlongflags;+u32val;++raw_spin_lock_irqsave(&port->irq_lock,flags);+val=readl_relaxed(port->base+PCIE_INT_ENABLE_REG);+val|=BIT(data->hwirq+PCIE_INTX_SHIFT);+writel_relaxed(val,port->base+PCIE_INT_ENABLE_REG);+raw_spin_unlock_irqrestore(&port->irq_lock,flags);+}++/**+*mtk_intx_eoi+*@data:pointertochipspecificdata+*+*AsanemulatedlevelIRQ,itsinterruptstatuswillremain+*untilthecorrespondingde-assertmessageisreceived;hencethat+*thestatuscanonlybeclearedwhentheinterrupthasbeenserviced.+*/+staticvoidmtk_intx_eoi(structirq_data*data)+{+structmtk_pcie_port*port=irq_data_get_irq_chip_data(data);+unsignedlonghwirq;++hwirq=data->hwirq+PCIE_INTX_SHIFT;+writel_relaxed(BIT(hwirq),port->base+PCIE_INT_STATUS_REG);+}++staticstructirq_chipmtk_intx_irq_chip={+.irq_enable=mtk_intx_unmask,+.irq_disable=mtk_intx_mask,
Please get rid of enable/disable. Given that you already have
mask/unmask with the *same* implementation, this offers zero benefit.
Hi Marc,
Thanks for your review.
We need to support suspend/resume feature, the HW will be powered off
when the system is suspended, and its register value will be cleared. If
the enable/disable callback is not implemented, the unmask function will
not be called when the system resume, so INTx will remain disabled.
Can I keep the enable/disable callback? Or do we have any solutions to
restore the register value when the system resume?
Thanks.
@@ -197,6 +235,35 @@ static int mtk_pcie_set_trans_table(struct mtk_pcie_port *port,return0;}+staticvoidmtk_pcie_enable_msi(structmtk_pcie_port*port)+{+inti;+u32val;++val=readl_relaxed(port->base+PCIE_MSI_SET_ENABLE_REG);+val|=PCIE_MSI_SET_ENABLE;+writel_relaxed(val,port->base+PCIE_MSI_SET_ENABLE_REG);++val=readl_relaxed(port->base+PCIE_INT_ENABLE_REG);+val|=PCIE_MSI_ENABLE;+writel_relaxed(val,port->base+PCIE_INT_ENABLE_REG);
Shouldn't you configure the capture addresses *before* enabling
things? Is there any need for locking here, given that you are
modifying global registers?
Yes, I will move these codes to the back of the configure capture
address in the next version.
I think the lock may not be needed because this function is only
executed once when driver probe.
quoted
+
+ for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
+ struct mtk_msi_set *msi_set = &port->msi_sets[i];
+
+ msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
+ i * PCIE_MSI_SET_OFFSET;
+ msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
+ i * PCIE_MSI_SET_OFFSET;
+
+ /* Configure the MSI capture address */
+ writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
+ writel_relaxed(upper_32_bits(msi_set->msg_addr),
+ port->base + PCIE_MSI_SET_ADDR_HI_BASE +
+ i * PCIE_MSI_SET_ADDR_HI_OFFSET);
+ }
+}
+
static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
{
struct resource_entry *entry;
@@ -247,6 +314,8 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port) return err; }+ mtk_pcie_enable_msi(port);+ /* Set PCIe translation windows */ resource_list_for_each_entry(entry, &host->windows) { struct resource *res = entry->res;
Isn't this write the same thing you have for EOI in the INTx case?
While I could understand your description in that case (this is a
resampling operation), I don't get what this does here. Either this is
also an EOI, but your initial description doesn't make sense, or it is
an Ack, and it should be moved to the right place.
Which one is it?
I think it should be an EOI which used to clear the interrupt status of
a single set in the PCIe intc field, maybe I should move it to the end
of the mtk_pcie_msi_handler() function.
+-----+
| GIC |
+-----+
^
|
port->irq
|
+-+-+-+-+-+-+-+-+
|0|1|2|3|4|5|6|7| (PCIe intc)
+-+-+-+-+-+-+-+-+
^ ^ ^
| | ... |
+-------+ +------+ +-----------+
| | |
+-+-+---+--+--+ +-+-+---+--+--+ +-+-+---+--+--+
|0|1|...|30|31| |0|1|...|30|31| |0|1|...|30|31| (MSI sets)
+-+-+---+--+--+ +-+-+---+--+--+ +-+-+---+--+--+
^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^
| | | | | | | | | | | | (MSI vectors)
| | | | | | | | | | | |
(MSI SET0) (MSI SET1) ... (MSI SET7)
I would like to ask another question. In this interrupt architecture, we
cannot implement an affinity for PCIe interrupts, so we return a
negative value in the mtk_pcie_set_affinity callback as follows:
+static int mtk_pcie_set_affinity(struct irq_data *data,
+ const struct cpumask *mask, bool force)
+{
+ return -EINVAL;
+}
But there will always be error logs when hotplug a CPU:
~ # echo 0 > /sys/devices/system/cpu/cpu1/online
[ 93.633059] IRQ255: set affinity failed(-22).
[ 93.633624] IRQ256: set affinity failed(-22).
[ 93.634222] CPU1: shutdown
[ 93.634586] psci: CPU1 killed (polled 0 ms)
Or when the system suspends:
~ # echo mem > /sys/power/state
[ 93.635145] cpuhp: cpu_off cluster=0, cpu=1
[ 169.835653] PM: suspend entry (deep)
[ 169.836717] Filesystems sync: 0.000 seconds
[ 169.837924] Freezing user space processes ... (elapsed 0.001 seconds)
done.
[ 169.839922] OOM killer disabled.
[ 169.840336] Freezing remaining freezable tasks ... (elapsed 0.001
seconds) done.
[ 169.844715] Disabling non-boot CPUs ...
[ 169.846443] IRQ255: set affinity failed(-22).
[ 169.847002] IRQ256: set affinity failed(-22).
[ 169.847586] CPU2: shutdown
[ 169.847943] psci: CPU2 killed (polled 0 ms)
[ 169.848489] cpuhp: cpu_off cluster=0, cpu=2
[ 169.850285] IRQ255: set affinity failed(-22).
[ 169.851369] IRQ256: set affinity failed(-22).
...
Sometimes this can cause misunderstandings to users, do we have a chance
to prevent this error log?
On Wednesday 24 February 2021 14:11:30 Jianjun Wang wrote:
+static int mtk_msi_bottom_domain_alloc(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs,
+ void *arg)
+{
+ struct mtk_pcie_port *port = domain->host_data;
+ struct mtk_msi_set *msi_set;
+ int i, hwirq, set_idx;
+
+ mutex_lock(&port->lock);
+
+ hwirq = bitmap_find_free_region(port->msi_irq_in_use, PCIE_MSI_IRQS_NUM,
+ order_base_2(nr_irqs));
+
+ mutex_unlock(&port->lock);
+
+ if (hwirq < 0)
+ return -ENOSPC;
+
+ set_idx = hwirq / PCIE_MSI_IRQS_PER_SET;
+ msi_set = &port->msi_sets[set_idx];
+
+ for (i = 0; i < nr_irqs; i++)
+ irq_domain_set_info(domain, virq + i, hwirq + i,
+ &mtk_msi_bottom_irq_chip, msi_set,
+ handle_edge_irq, NULL, NULL);
+
+ return 0;
+}
+
+static void mtk_msi_bottom_domain_free(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs)
+{
+ struct mtk_pcie_port *port = domain->host_data;
+ struct irq_data *data = irq_domain_get_irq_data(domain, virq);
+
+ mutex_lock(&port->lock);
+
+ bitmap_clear(port->msi_irq_in_use, data->hwirq, nr_irqs);
Marc, should not be there bitmap_release_region() with order_base_2()?
bitmap_release_region(port->msi_irq_in_use, data->hwirq, order_base_2(nr_irqs));
Because mtk_msi_bottom_domain_alloc() is allocating
order_base_2(nr_irqs) interrupts, not only nr_irqs.
From: Marc Zyngier <maz@kernel.org> Date: 2021-03-11 08:20:26
On 2021-03-11 00:05, Pali Rohár wrote:
On Wednesday 24 February 2021 14:11:30 Jianjun Wang wrote:
quoted
+static int mtk_msi_bottom_domain_alloc(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs,
+ void *arg)
+{
+ struct mtk_pcie_port *port = domain->host_data;
+ struct mtk_msi_set *msi_set;
+ int i, hwirq, set_idx;
+
+ mutex_lock(&port->lock);
+
+ hwirq = bitmap_find_free_region(port->msi_irq_in_use,
PCIE_MSI_IRQS_NUM,
+ order_base_2(nr_irqs));
+
+ mutex_unlock(&port->lock);
+
+ if (hwirq < 0)
+ return -ENOSPC;
+
+ set_idx = hwirq / PCIE_MSI_IRQS_PER_SET;
+ msi_set = &port->msi_sets[set_idx];
+
+ for (i = 0; i < nr_irqs; i++)
+ irq_domain_set_info(domain, virq + i, hwirq + i,
+ &mtk_msi_bottom_irq_chip, msi_set,
+ handle_edge_irq, NULL, NULL);
+
+ return 0;
+}
+
+static void mtk_msi_bottom_domain_free(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs)
+{
+ struct mtk_pcie_port *port = domain->host_data;
+ struct irq_data *data = irq_domain_get_irq_data(domain, virq);
+
+ mutex_lock(&port->lock);
+
+ bitmap_clear(port->msi_irq_in_use, data->hwirq, nr_irqs);
Marc, should not be there bitmap_release_region() with order_base_2()?
bitmap_release_region(port->msi_irq_in_use, data->hwirq,
order_base_2(nr_irqs));
Because mtk_msi_bottom_domain_alloc() is allocating
order_base_2(nr_irqs) interrupts, not only nr_irqs.
Indeed, good catch.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
From: Jianjun Wang <hidden> Date: 2021-03-11 09:51:20
On Thu, 2021-03-11 at 08:19 +0000, Marc Zyngier wrote:
On 2021-03-11 00:05, Pali Rohár wrote:
quoted
On Wednesday 24 February 2021 14:11:30 Jianjun Wang wrote:
quoted
+static int mtk_msi_bottom_domain_alloc(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs,
+ void *arg)
+{
+ struct mtk_pcie_port *port = domain->host_data;
+ struct mtk_msi_set *msi_set;
+ int i, hwirq, set_idx;
+
+ mutex_lock(&port->lock);
+
+ hwirq = bitmap_find_free_region(port->msi_irq_in_use,
PCIE_MSI_IRQS_NUM,
+ order_base_2(nr_irqs));
+
+ mutex_unlock(&port->lock);
+
+ if (hwirq < 0)
+ return -ENOSPC;
+
+ set_idx = hwirq / PCIE_MSI_IRQS_PER_SET;
+ msi_set = &port->msi_sets[set_idx];
+
+ for (i = 0; i < nr_irqs; i++)
+ irq_domain_set_info(domain, virq + i, hwirq + i,
+ &mtk_msi_bottom_irq_chip, msi_set,
+ handle_edge_irq, NULL, NULL);
+
+ return 0;
+}
+
+static void mtk_msi_bottom_domain_free(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs)
+{
+ struct mtk_pcie_port *port = domain->host_data;
+ struct irq_data *data = irq_domain_get_irq_data(domain, virq);
+
+ mutex_lock(&port->lock);
+
+ bitmap_clear(port->msi_irq_in_use, data->hwirq, nr_irqs);
Marc, should not be there bitmap_release_region() with order_base_2()?
bitmap_release_region(port->msi_irq_in_use, data->hwirq,
order_base_2(nr_irqs));
Because mtk_msi_bottom_domain_alloc() is allocating
order_base_2(nr_irqs) interrupts, not only nr_irqs.
Indeed, good catch.
I will fix it in the next version, thanks for your review.
On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
+static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
+{
...
+
+ /* Delay 100ms to wait the reference clocks become stable */
+ msleep(100);
+
+ /* De-assert PERST# signal */
+ val &= ~PCIE_PE_RSTB;
+ writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
Hello! This is a new driver which introduce yet another custom timeout
prior PERST# signal for PCIe card is de-asserted. Timeouts for other
drivers I collected in older email [2].
Please look at my email [1] about PCIe Warm Reset if you have any clue
about it. Lorenzo and Rob already expressed that this timeout should not
be driver specific. But nobody was able to "decode" and "understand"
PCIe spec yet about these timeouts.
+
+ /* Check if the link is up or not */
+ err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,
+ !!(val & PCIE_PORT_LINKUP), 20,
+ 50 * USEC_PER_MSEC);
IIRC, you need to wait at least 100ms after de-asserting PERST# signal
as it is required by PCIe specs and also because experiments proved that
some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
wait this minimal time.
+ if (err) {
+ val = readl_relaxed(port->base + PCIE_LTSSM_STATUS_REG);
+ dev_err(port->dev, "PCIe link down, ltssm reg val: %#x\n", val);
+ return err;
+ }
From: Jianjun Wang <hidden> Date: 2021-03-13 07:48:07
On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote:
On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
quoted
+static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
+{
...
quoted
+
+ /* Delay 100ms to wait the reference clocks become stable */
+ msleep(100);
+
+ /* De-assert PERST# signal */
+ val &= ~PCIE_PE_RSTB;
+ writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
Hello! This is a new driver which introduce yet another custom timeout
prior PERST# signal for PCIe card is de-asserted. Timeouts for other
drivers I collected in older email [2].
Please look at my email [1] about PCIe Warm Reset if you have any clue
about it. Lorenzo and Rob already expressed that this timeout should not
be driver specific. But nobody was able to "decode" and "understand"
PCIe spec yet about these timeouts.
Hi Pali,
I think this is more like a platform specific timeout, which is used to
wait for the reference clocks to become stable and finish the reset flow
of HW blocks.
Here is the steps to start a link training in this HW:
1. Assert all reset signals which including the transaction layer, PIPE
interface and internal bus interface;
2. De-assert reset signals except the PERST#, this will make the
physical layer active and start to output the reference clock, but the
EP device remains in the reset state.
Before releasing the PERST# signal, the HW blocks needs at least 10ms
to finish the reset flow, and ref-clk needs about 30us to become stable.
3. De-assert PERST# signal, wait LTSSM enter L0 state.
This 100ms timeout is reference to TPVPERL in the PCIe CEM spec. Since
we are in the kernel stage, the power supply has already stabled, this
timeout may not take that long.
quoted
+
+ /* Check if the link is up or not */
+ err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,
+ !!(val & PCIE_PORT_LINKUP), 20,
+ 50 * USEC_PER_MSEC);
IIRC, you need to wait at least 100ms after de-asserting PERST# signal
as it is required by PCIe specs and also because experiments proved that
some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
wait this minimal time.
Yes, this should be 100ms, I will fix it at next version, thanks for
your review.
Thanks.
quoted
+ if (err) {
+ val = readl_relaxed(port->base + PCIE_LTSSM_STATUS_REG);
+ dev_err(port->dev, "PCIe link down, ltssm reg val: %#x\n", val);
+ return err;
+ }
On Saturday 13 March 2021 15:43:14 Jianjun Wang wrote:
On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote:
quoted
On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
quoted
+static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
+{
...
quoted
+
+ /* Delay 100ms to wait the reference clocks become stable */
+ msleep(100);
+
+ /* De-assert PERST# signal */
+ val &= ~PCIE_PE_RSTB;
+ writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
Hello! This is a new driver which introduce yet another custom timeout
prior PERST# signal for PCIe card is de-asserted. Timeouts for other
drivers I collected in older email [2].
Please look at my email [1] about PCIe Warm Reset if you have any clue
about it. Lorenzo and Rob already expressed that this timeout should not
be driver specific. But nobody was able to "decode" and "understand"
PCIe spec yet about these timeouts.
Hi Pali,
I think this is more like a platform specific timeout, which is used to
wait for the reference clocks to become stable and finish the reset flow
of HW blocks.
Here is the steps to start a link training in this HW:
1. Assert all reset signals which including the transaction layer, PIPE
interface and internal bus interface;
2. De-assert reset signals except the PERST#, this will make the
physical layer active and start to output the reference clock, but the
EP device remains in the reset state.
Before releasing the PERST# signal, the HW blocks needs at least 10ms
to finish the reset flow, and ref-clk needs about 30us to become stable.
3. De-assert PERST# signal, wait LTSSM enter L0 state.
This 100ms timeout is reference to TPVPERL in the PCIe CEM spec. Since
we are in the kernel stage, the power supply has already stabled, this
timeout may not take that long.
I think that this is not platform specific timeout or platform specific
steps. This matches generic steps as defined in PCIe CEM spec, section
2.2.1. Initial Power-Up (G3 to S0).
What is platform specific is just how to achieve these steps.
Am I right?
...
TPVPERL is one of my timeout candidates as minimal required timeout for
Warm Reset. I have wrote it in email:
https://lore.kernel.org/linux-pci/20200430082245.xblvb7xeamm4e336@pali/
But I'm not sure as specially in none diagram is described just warm
reset as defined in mPCIe CEM (3.2.4.3. PERST# Signal).
...
Anyway, I would suggest to define constants for those timeouts. I guess
that in future we could be able to define "generic" timeout constants
which would not be in private driver section, but in some common header
file.
quoted
quoted
+
+ /* Check if the link is up or not */
+ err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,
+ !!(val & PCIE_PORT_LINKUP), 20,
+ 50 * USEC_PER_MSEC);
IIRC, you need to wait at least 100ms after de-asserting PERST# signal
as it is required by PCIe specs and also because experiments proved that
some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
wait this minimal time.
Yes, this should be 100ms, I will fix it at next version, thanks for
your review.
From: Jianjun Wang <hidden> Date: 2021-03-18 05:49:14
On Thu, 2021-03-18 at 01:02 +0100, Pali Rohár wrote:
On Saturday 13 March 2021 15:43:14 Jianjun Wang wrote:
quoted
On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote:
quoted
On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
quoted
+static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
+{
...
quoted
+
+ /* Delay 100ms to wait the reference clocks become stable */
+ msleep(100);
+
+ /* De-assert PERST# signal */
+ val &= ~PCIE_PE_RSTB;
+ writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
Hello! This is a new driver which introduce yet another custom timeout
prior PERST# signal for PCIe card is de-asserted. Timeouts for other
drivers I collected in older email [2].
Please look at my email [1] about PCIe Warm Reset if you have any clue
about it. Lorenzo and Rob already expressed that this timeout should not
be driver specific. But nobody was able to "decode" and "understand"
PCIe spec yet about these timeouts.
Hi Pali,
I think this is more like a platform specific timeout, which is used to
wait for the reference clocks to become stable and finish the reset flow
of HW blocks.
Here is the steps to start a link training in this HW:
1. Assert all reset signals which including the transaction layer, PIPE
interface and internal bus interface;
2. De-assert reset signals except the PERST#, this will make the
physical layer active and start to output the reference clock, but the
EP device remains in the reset state.
Before releasing the PERST# signal, the HW blocks needs at least 10ms
to finish the reset flow, and ref-clk needs about 30us to become stable.
3. De-assert PERST# signal, wait LTSSM enter L0 state.
This 100ms timeout is reference to TPVPERL in the PCIe CEM spec. Since
we are in the kernel stage, the power supply has already stabled, this
timeout may not take that long.
I think that this is not platform specific timeout or platform specific
steps. This matches generic steps as defined in PCIe CEM spec, section
2.2.1. Initial Power-Up (G3 to S0).
What is platform specific is just how to achieve these steps.
Am I right?
...
TPVPERL is one of my timeout candidates as minimal required timeout for
Warm Reset. I have wrote it in email:
https://lore.kernel.org/linux-pci/20200430082245.xblvb7xeamm4e336@pali/
But I'm not sure as specially in none diagram is described just warm
reset as defined in mPCIe CEM (3.2.4.3. PERST# Signal).
...
Anyway, I would suggest to define constants for those timeouts. I guess
that in future we could be able to define "generic" timeout constants
which would not be in private driver section, but in some common header
file.
I agree with this, but I'm not sure if we really need that long time in
the kernel stage, because the power supply has already stable and it's
really impact the boot time, especially when the platform have multi
ports and not connect any EP device, we need to wait 200ms for each port
when system bootup.
For this PCIe controller driver, I would like to change the timeout
value to 10ms to comply with the HW design, and save some boot time.
quoted
quoted
quoted
+
+ /* Check if the link is up or not */
+ err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,
+ !!(val & PCIE_PORT_LINKUP), 20,
+ 50 * USEC_PER_MSEC);
IIRC, you need to wait at least 100ms after de-asserting PERST# signal
as it is required by PCIe specs and also because experiments proved that
some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
wait this minimal time.
Yes, this should be 100ms, I will fix it at next version, thanks for
your review.
On Thursday 18 March 2021 13:48:07 Jianjun Wang wrote:
On Thu, 2021-03-18 at 01:02 +0100, Pali Rohár wrote:
quoted
On Saturday 13 March 2021 15:43:14 Jianjun Wang wrote:
quoted
On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote:
quoted
On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
quoted
+static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
+{
...
quoted
+
+ /* Delay 100ms to wait the reference clocks become stable */
+ msleep(100);
+
+ /* De-assert PERST# signal */
+ val &= ~PCIE_PE_RSTB;
+ writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
Hello! This is a new driver which introduce yet another custom timeout
prior PERST# signal for PCIe card is de-asserted. Timeouts for other
drivers I collected in older email [2].
Please look at my email [1] about PCIe Warm Reset if you have any clue
about it. Lorenzo and Rob already expressed that this timeout should not
be driver specific. But nobody was able to "decode" and "understand"
PCIe spec yet about these timeouts.
Hi Pali,
I think this is more like a platform specific timeout, which is used to
wait for the reference clocks to become stable and finish the reset flow
of HW blocks.
Here is the steps to start a link training in this HW:
1. Assert all reset signals which including the transaction layer, PIPE
interface and internal bus interface;
2. De-assert reset signals except the PERST#, this will make the
physical layer active and start to output the reference clock, but the
EP device remains in the reset state.
Before releasing the PERST# signal, the HW blocks needs at least 10ms
to finish the reset flow, and ref-clk needs about 30us to become stable.
3. De-assert PERST# signal, wait LTSSM enter L0 state.
This 100ms timeout is reference to TPVPERL in the PCIe CEM spec. Since
we are in the kernel stage, the power supply has already stabled, this
timeout may not take that long.
I think that this is not platform specific timeout or platform specific
steps. This matches generic steps as defined in PCIe CEM spec, section
2.2.1. Initial Power-Up (G3 to S0).
What is platform specific is just how to achieve these steps.
Am I right?
...
TPVPERL is one of my timeout candidates as minimal required timeout for
Warm Reset. I have wrote it in email:
https://lore.kernel.org/linux-pci/20200430082245.xblvb7xeamm4e336@pali/
But I'm not sure as specially in none diagram is described just warm
reset as defined in mPCIe CEM (3.2.4.3. PERST# Signal).
...
Anyway, I would suggest to define constants for those timeouts. I guess
that in future we could be able to define "generic" timeout constants
which would not be in private driver section, but in some common header
file.
I agree with this, but I'm not sure if we really need that long time in
the kernel stage, because the power supply has already stable and it's
really impact the boot time, especially when the platform have multi
ports and not connect any EP device, we need to wait 200ms for each port
when system bootup.
Ports are independent. So you can initialize them in parallel, right?
If you initialize each port in separate worker then during msleep calls
kernel can schedule other kernel thread to run and so it does not
increase boot time. While pcie is sleeping kernel can do other things.
So the result is that whole boot time is not increased, just reordered.
For this PCIe controller driver, I would like to change the timeout
value to 10ms to comply with the HW design, and save some boot time.
In case you can connect _any_ PCIe card to your HW then you cannot
decrease or change timeouts required by PCIe specs. Otherwise there can
be a card which would not be initialized correctly.
I'm debugging driver for aardvark PCIe controller and I see that Compex
cards really needs these timeouts, otherwise link is down and card
cannot be detected.
So I guess that there can be also other cards which requires other
timeouts as specified in PCIe specs.
quoted
quoted
quoted
quoted
+
+ /* Check if the link is up or not */
+ err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,
+ !!(val & PCIE_PORT_LINKUP), 20,
+ 50 * USEC_PER_MSEC);
IIRC, you need to wait at least 100ms after de-asserting PERST# signal
as it is required by PCIe specs and also because experiments proved that
some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
wait this minimal time.
Yes, this should be 100ms, I will fix it at next version, thanks for
your review.
From: Jianjun Wang <hidden> Date: 2021-03-23 01:32:32
On Fri, 2021-03-19 at 19:53 +0100, Pali Rohár wrote:
On Thursday 18 March 2021 13:48:07 Jianjun Wang wrote:
quoted
On Thu, 2021-03-18 at 01:02 +0100, Pali Rohár wrote:
quoted
On Saturday 13 March 2021 15:43:14 Jianjun Wang wrote:
quoted
On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote:
quoted
On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
quoted
+static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
+{
...
quoted
+
+ /* Delay 100ms to wait the reference clocks become stable */
+ msleep(100);
+
+ /* De-assert PERST# signal */
+ val &= ~PCIE_PE_RSTB;
+ writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
Hello! This is a new driver which introduce yet another custom timeout
prior PERST# signal for PCIe card is de-asserted. Timeouts for other
drivers I collected in older email [2].
Please look at my email [1] about PCIe Warm Reset if you have any clue
about it. Lorenzo and Rob already expressed that this timeout should not
be driver specific. But nobody was able to "decode" and "understand"
PCIe spec yet about these timeouts.
Hi Pali,
I think this is more like a platform specific timeout, which is used to
wait for the reference clocks to become stable and finish the reset flow
of HW blocks.
Here is the steps to start a link training in this HW:
1. Assert all reset signals which including the transaction layer, PIPE
interface and internal bus interface;
2. De-assert reset signals except the PERST#, this will make the
physical layer active and start to output the reference clock, but the
EP device remains in the reset state.
Before releasing the PERST# signal, the HW blocks needs at least 10ms
to finish the reset flow, and ref-clk needs about 30us to become stable.
3. De-assert PERST# signal, wait LTSSM enter L0 state.
This 100ms timeout is reference to TPVPERL in the PCIe CEM spec. Since
we are in the kernel stage, the power supply has already stabled, this
timeout may not take that long.
I think that this is not platform specific timeout or platform specific
steps. This matches generic steps as defined in PCIe CEM spec, section
2.2.1. Initial Power-Up (G3 to S0).
What is platform specific is just how to achieve these steps.
Am I right?
...
TPVPERL is one of my timeout candidates as minimal required timeout for
Warm Reset. I have wrote it in email:
https://lore.kernel.org/linux-pci/20200430082245.xblvb7xeamm4e336@pali/
But I'm not sure as specially in none diagram is described just warm
reset as defined in mPCIe CEM (3.2.4.3. PERST# Signal).
...
Anyway, I would suggest to define constants for those timeouts. I guess
that in future we could be able to define "generic" timeout constants
which would not be in private driver section, but in some common header
file.
I agree with this, but I'm not sure if we really need that long time in
the kernel stage, because the power supply has already stable and it's
really impact the boot time, especially when the platform have multi
ports and not connect any EP device, we need to wait 200ms for each port
when system bootup.
Ports are independent. So you can initialize them in parallel, right?
If you initialize each port in separate worker then during msleep calls
kernel can schedule other kernel thread to run and so it does not
increase boot time. While pcie is sleeping kernel can do other things.
So the result is that whole boot time is not increased, just reordered.
quoted
For this PCIe controller driver, I would like to change the timeout
value to 10ms to comply with the HW design, and save some boot time.
In case you can connect _any_ PCIe card to your HW then you cannot
decrease or change timeouts required by PCIe specs. Otherwise there can
be a card which would not be initialized correctly.
I'm debugging driver for aardvark PCIe controller and I see that Compex
cards really needs these timeouts, otherwise link is down and card
cannot be detected.
So I guess that there can be also other cards which requires other
timeouts as specified in PCIe specs.
OK, I'll keep this timeout value.
One more question, is there any chance that we can put this linkup flow
to a more "standard" way, such as drivers provides the ops of the PERST#
pin and let the framework to decide how to start a link training, or we
just use macro to replace this timeout value in the future?
Thanks.
quoted
quoted
quoted
quoted
quoted
+
+ /* Check if the link is up or not */
+ err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,
+ !!(val & PCIE_PORT_LINKUP), 20,
+ 50 * USEC_PER_MSEC);
IIRC, you need to wait at least 100ms after de-asserting PERST# signal
as it is required by PCIe specs and also because experiments proved that
some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
wait this minimal time.
Yes, this should be 100ms, I will fix it at next version, thanks for
your review.
On Tuesday 23 March 2021 09:31:34 Jianjun Wang wrote:
One more question, is there any chance that we can put this linkup flow
to a more "standard" way, such as drivers provides the ops of the PERST#
pin and let the framework to decide how to start a link training, or we
just use macro to replace this timeout value in the future?
This is something about which I was thinking that could be useful for
pci-aardvark.c driver. But I was not sure if some other driver can
benefit from such "framework". But now I see that your driver is another
candidate which can benefit from it.
Currently there is no such "framework" in kernel and the hardest part
would be to design it.
Having this API would allow kernel to implement and export PCIe Warm
Reset (which is done via PERST# signal) and easily extend Amey's reset
patches to export also Warm Reset via sysfs.
But to implement this framework and using it for reset we first need to
answer questions which I have sent in email:
https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
Bjorn, Alex: any opinion about PERST#?
Also see Enrico's email, where confirmed that there are platforms which
shares one PERST# signal for more endpoint cards:
https://lore.kernel.org/linux-pci/1da0fa2c-8056-9ae8-6ce4-ab645317772d@metux.net/
On Thursday 18 March 2021 13:48:07 Jianjun Wang wrote:
On Thu, 2021-03-18 at 01:02 +0100, Pali Rohár wrote:
quoted
On Saturday 13 March 2021 15:43:14 Jianjun Wang wrote:
quoted
On Thu, 2021-03-11 at 13:38 +0100, Pali Rohár wrote:
quoted
On Wednesday 24 February 2021 14:11:28 Jianjun Wang wrote:
quoted
+
+ /* Check if the link is up or not */
+ err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,
+ !!(val & PCIE_PORT_LINKUP), 20,
+ 50 * USEC_PER_MSEC);
IIRC, you need to wait at least 100ms after de-asserting PERST# signal
as it is required by PCIe specs and also because experiments proved that
some Compex wifi cards (e.g. WLE900VX) are not detected if you do not
wait this minimal time.
Yes, this should be 100ms, I will fix it at next version, thanks for
your review.
Sure, I will use PCI_PM_D3COLD_WAIT macro instead in the next version.
Thanks.
Anyway, now I found out that kernel has functions for this waiting:
pcie_wait_for_link_delay() and pcie_wait_for_link()
Function is called from pci_bridge_wait_for_secondary_bus().
But in current form it is not usable for native controller drivers.
This looks like another candidate for code de-duplication or providing
"framework".
Lorenzo, as maintainer of native controller drivers, do you have some
ideas about providing "framework", common functions or something for
avoiding to implement same code patterns in every native controller
driver, which is de-facto standard PCIe codepath? Including a way how to
export PERST# reset gpio?