| Message ID | 20240418-pci-epf-rework-v3-3-222a5d1ed2e5@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: endpoint: Make host reboot handling more robust | expand |
On Thu, Apr 18, 2024 at 05:28:31PM +0530, Manivannan Sadhasivam wrote: > BME which stands for 'Bus Master Enable' is not defined in the PCIe base > spec even though it is commonly referred in many places (vendor docs). But > to align with the spec, let's rename it to its expansion 'Bus Master > Enable'. > > Suggested-by: Damien Le Moal <dlemoal@kernel.org> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- Reviewed-by: Niklas Cassel <cassel@kernel.org> Outside the scope of this patch/series: Do we perhaps want to add a bus_master_enable() callback also for the pci-epf-test driver? In my opinion, the test driver should be "the driver" that tests that all the EPF features/callbacks work, at least a basic test "does it work at all". Other EPF drivers can implement the callbacks, and do more intelligent things, i.e. more than just seeing that "it works". Kind regards, Niklas
On Thu, Apr 18, 2024 at 05:28:31PM +0530, Manivannan Sadhasivam wrote: > BME which stands for 'Bus Master Enable' is not defined in the PCIe base > spec even though it is commonly referred in many places (vendor docs). But > to align with the spec, let's rename it to its expansion 'Bus Master > Enable'. Thanks for doing this. I'm always in favor of using terms from the spec. > - dev_dbg(dev, "Received BME event. Link is enabled!\n"); > + dev_dbg(dev, "Received Bus Master Enable event. Link is enabled!\n"); Nothing to do with *this* patch, but this message reads a little weird to me because setting Bus Master Enable has nothing to do with link enablement. Also incidental: some of these messages and comments refer to a "Bus Master Enable *event*". Does "event" here refer to the act of the host setting the Bus Master Enable bit in the Command register? This is in qcom_pcie_ep_global_irq_thread(), so I assume there's something in the endpoint hardware that generates an IRQ when the Command register is written? > - * pci_epc_bme_notify() - Notify the EPF device that the EPC device has received > - * the BME event from the Root complex > - * @epc: the EPC device that received the BME event > + * pci_epc_bus_master_enable_notify() - Notify the EPF device that the EPC > + * device has received the Bus Master > + * Enable event from the Root complex > + * @epc: the EPC device that received the Bus Master Enable event > * > * Invoke to Notify the EPF device that the EPC device has received the Bus > - * Master Enable (BME) event from the Root complex > + * Master Enable event from the Root complex There's no "set Bus Master Enable" transaction that would appear on the PCIe link, so I assume "the Bus Master Enable event from the Root Complex" is a way of saying something like "host has written the Command register to set the Bus Master Enable bit"? Bjorn
On Thu, Apr 18, 2024 at 04:49:04PM +0200, Niklas Cassel wrote: > On Thu, Apr 18, 2024 at 05:28:31PM +0530, Manivannan Sadhasivam wrote: > > BME which stands for 'Bus Master Enable' is not defined in the PCIe base > > spec even though it is commonly referred in many places (vendor docs). But > > to align with the spec, let's rename it to its expansion 'Bus Master > > Enable'. > > > > Suggested-by: Damien Le Moal <dlemoal@kernel.org> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > Reviewed-by: Niklas Cassel <cassel@kernel.org> > > > Outside the scope of this patch/series: > Do we perhaps want to add a bus_master_enable() callback also for the > pci-epf-test driver? > Makes sense to me. > In my opinion, the test driver should be "the driver" that tests that > all the EPF features/callbacks work, at least a basic test "does it > work at all". Other EPF drivers can implement the callbacks, and do > more intelligent things, i.e. more than just seeing that "it works". > Agree. Feel free to send a patch :) - Mani
On Thu, Apr 18, 2024 at 11:12:09AM -0500, Bjorn Helgaas wrote: > On Thu, Apr 18, 2024 at 05:28:31PM +0530, Manivannan Sadhasivam wrote: > > BME which stands for 'Bus Master Enable' is not defined in the PCIe base > > spec even though it is commonly referred in many places (vendor docs). But > > to align with the spec, let's rename it to its expansion 'Bus Master > > Enable'. > > Thanks for doing this. I'm always in favor of using terms from the > spec. > > > - dev_dbg(dev, "Received BME event. Link is enabled!\n"); > > + dev_dbg(dev, "Received Bus Master Enable event. Link is enabled!\n"); > > Nothing to do with *this* patch, but this message reads a little weird > to me because setting Bus Master Enable has nothing to do with link > enablement. > That's my bad. I'll remove it. > Also incidental: some of these messages and comments refer to a "Bus > Master Enable *event*". Does "event" here refer to the act of the > host setting the Bus Master Enable bit in the Command register? This > is in qcom_pcie_ep_global_irq_thread(), so I assume there's something > in the endpoint hardware that generates an IRQ when the Command > register is written? > Yes, the PCIe endpoint controller generates an IRQ when host sets Bus Master Enable bit. > > - * pci_epc_bme_notify() - Notify the EPF device that the EPC device has received > > - * the BME event from the Root complex > > - * @epc: the EPC device that received the BME event > > + * pci_epc_bus_master_enable_notify() - Notify the EPF device that the EPC > > + * device has received the Bus Master > > + * Enable event from the Root complex > > + * @epc: the EPC device that received the Bus Master Enable event > > * > > * Invoke to Notify the EPF device that the EPC device has received the Bus > > - * Master Enable (BME) event from the Root complex > > + * Master Enable event from the Root complex > > There's no "set Bus Master Enable" transaction that would appear on > the PCIe link, so I assume "the Bus Master Enable event from the Root > Complex" is a way of saying something like "host has written the > Command register to set the Bus Master Enable bit"? > Yes. But looking at it again, it could be reworded as below: 'Invoke to notify the EPF device that the EPC device has generated the Bus Master Enable event due to host setting the Bus Master Enable bit in the Command register.' - Mani
On Fri, Apr 19, 2024 at 03:08:47PM +0530, Manivannan Sadhasivam wrote: > On Thu, Apr 18, 2024 at 11:12:09AM -0500, Bjorn Helgaas wrote: > > On Thu, Apr 18, 2024 at 05:28:31PM +0530, Manivannan Sadhasivam wrote: > ... > > > - * pci_epc_bme_notify() - Notify the EPF device that the EPC device has received > > > - * the BME event from the Root complex > > > - * @epc: the EPC device that received the BME event > > > + * pci_epc_bus_master_enable_notify() - Notify the EPF device that the EPC > > > + * device has received the Bus Master > > > + * Enable event from the Root complex > > > + * @epc: the EPC device that received the Bus Master Enable event > > > * > > > * Invoke to Notify the EPF device that the EPC device has received the Bus > > > - * Master Enable (BME) event from the Root complex > > > + * Master Enable event from the Root complex > > > > There's no "set Bus Master Enable" transaction that would appear on > > the PCIe link, so I assume "the Bus Master Enable event from the Root > > Complex" is a way of saying something like "host has written the > > Command register to set the Bus Master Enable bit"? > > > > Yes. But looking at it again, it could be reworded as below: > > 'Invoke to notify the EPF device that the EPC device has generated the Bus > Master Enable event due to host setting the Bus Master Enable bit in the > Command register.' Sounds good. Could even s/Invoke to notify/Notify/.
diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c index 50b1635e3cbb..f6e925d434f6 100644 --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c @@ -636,10 +636,10 @@ static irqreturn_t qcom_pcie_ep_global_irq_thread(int irq, void *data) pcie_ep->link_status = QCOM_PCIE_EP_LINK_DOWN; pci_epc_linkdown(pci->ep.epc); } else if (FIELD_GET(PARF_INT_ALL_BME, status)) { - dev_dbg(dev, "Received BME event. Link is enabled!\n"); + dev_dbg(dev, "Received Bus Master Enable event. Link is enabled!\n"); pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED; qcom_pcie_ep_icc_update(pcie_ep); - pci_epc_bme_notify(pci->ep.epc); + pci_epc_bus_master_enable_notify(pci->ep.epc); } else if (FIELD_GET(PARF_INT_ALL_PM_TURNOFF, status)) { dev_dbg(dev, "Received PM Turn-off event! Entering L23\n"); val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL); diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c index 95c3206f609f..b662905e2532 100644 --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c @@ -819,7 +819,7 @@ static int pci_epf_mhi_link_down(struct pci_epf *epf) return 0; } -static int pci_epf_mhi_bme(struct pci_epf *epf) +static int pci_epf_mhi_bus_master_enable(struct pci_epf *epf) { struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf); const struct pci_epf_mhi_ep_info *info = epf_mhi->info; @@ -882,8 +882,8 @@ static void pci_epf_mhi_unbind(struct pci_epf *epf) /* * Forcefully power down the MHI EP stack. Only way to bring the MHI EP - * stack back to working state after successive bind is by getting BME - * from host. + * stack back to working state after successive bind is by getting Bus + * Master Enable event from host. */ if (mhi_cntrl->mhi_dev) { mhi_ep_power_down(mhi_cntrl); @@ -900,7 +900,7 @@ static const struct pci_epc_event_ops pci_epf_mhi_event_ops = { .epc_init = pci_epf_mhi_epc_init, .link_up = pci_epf_mhi_link_up, .link_down = pci_epf_mhi_link_down, - .bme = pci_epf_mhi_bme, + .bus_master_enable = pci_epf_mhi_bus_master_enable, }; static int pci_epf_mhi_probe(struct pci_epf *epf, diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c index e6bffa37a948..917dc56dfbbe 100644 --- a/drivers/pci/endpoint/pci-epc-core.c +++ b/drivers/pci/endpoint/pci-epc-core.c @@ -775,14 +775,15 @@ void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf) EXPORT_SYMBOL_GPL(pci_epc_notify_pending_init); /** - * pci_epc_bme_notify() - Notify the EPF device that the EPC device has received - * the BME event from the Root complex - * @epc: the EPC device that received the BME event + * pci_epc_bus_master_enable_notify() - Notify the EPF device that the EPC + * device has received the Bus Master + * Enable event from the Root complex + * @epc: the EPC device that received the Bus Master Enable event * * Invoke to Notify the EPF device that the EPC device has received the Bus - * Master Enable (BME) event from the Root complex + * Master Enable event from the Root complex */ -void pci_epc_bme_notify(struct pci_epc *epc) +void pci_epc_bus_master_enable_notify(struct pci_epc *epc) { struct pci_epf *epf; @@ -792,13 +793,13 @@ void pci_epc_bme_notify(struct pci_epc *epc) mutex_lock(&epc->list_lock); list_for_each_entry(epf, &epc->pci_epf, list) { mutex_lock(&epf->lock); - if (epf->event_ops && epf->event_ops->bme) - epf->event_ops->bme(epf); + if (epf->event_ops && epf->event_ops->bus_master_enable) + epf->event_ops->bus_master_enable(epf); mutex_unlock(&epf->lock); } mutex_unlock(&epc->list_lock); } -EXPORT_SYMBOL_GPL(pci_epc_bme_notify); +EXPORT_SYMBOL_GPL(pci_epc_bus_master_enable_notify); /** * pci_epc_destroy() - destroy the EPC device diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h index acc5f96161fe..11115cd0fe5b 100644 --- a/include/linux/pci-epc.h +++ b/include/linux/pci-epc.h @@ -226,7 +226,7 @@ void pci_epc_linkup(struct pci_epc *epc); void pci_epc_linkdown(struct pci_epc *epc); void pci_epc_init_notify(struct pci_epc *epc); void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf); -void pci_epc_bme_notify(struct pci_epc *epc); +void pci_epc_bus_master_enable_notify(struct pci_epc *epc); void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf, enum pci_epc_interface_type type); int pci_epc_write_header(struct pci_epc *epc, u8 func_no, u8 vfunc_no, diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h index afe3bfd88742..dc759eb7157c 100644 --- a/include/linux/pci-epf.h +++ b/include/linux/pci-epf.h @@ -73,13 +73,13 @@ struct pci_epf_ops { * @epc_init: Callback for the EPC initialization complete event * @link_up: Callback for the EPC link up event * @link_down: Callback for the EPC link down event - * @bme: Callback for the EPC BME (Bus Master Enable) event + * @bus_master_enable: Callback for the EPC Bus Master Enable event */ struct pci_epc_event_ops { int (*epc_init)(struct pci_epf *epf); int (*link_up)(struct pci_epf *epf); int (*link_down)(struct pci_epf *epf); - int (*bme)(struct pci_epf *epf); + int (*bus_master_enable)(struct pci_epf *epf); }; /**
BME which stands for 'Bus Master Enable' is not defined in the PCIe base spec even though it is commonly referred in many places (vendor docs). But to align with the spec, let's rename it to its expansion 'Bus Master Enable'. Suggested-by: Damien Le Moal <dlemoal@kernel.org> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/pci/controller/dwc/pcie-qcom-ep.c | 4 ++-- drivers/pci/endpoint/functions/pci-epf-mhi.c | 8 ++++---- drivers/pci/endpoint/pci-epc-core.c | 17 +++++++++-------- include/linux/pci-epc.h | 2 +- include/linux/pci-epf.h | 4 ++-- 5 files changed, 18 insertions(+), 17 deletions(-)