Re: [PATCH V3 5/5] PCI: tegra: Add support for PCIe endpoint mode in Tegra194

[Vidya Sagar]

On 2/27/2020 5:18 PM, Lorenzo Pieralisi wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Jan 13, 2020 at 11:44:11PM +0530, Vidya Sagar wrote:
> > Add support for the endpoint mode of Synopsys DesignWare core based
> > dual mode PCIe controllers present in Tegra194 SoC.
> >
> > Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
> > ---
> > V3:
> > * Addressed Thierry's review comments
>
> I need his ACK to merge this series.
>
> [...]
>
> > +static int tegra_pcie_ep_work_thread(void *p)
> > +{
> > +     struct tegra_pcie_dw *pcie = (struct tegra_pcie_dw *)p;
> > +     u32 event;
> > +
> > +     while (true) {
> > +             wait_event_interruptible(pcie->wq,
> > +                                      !kfifo_is_empty(&pcie->event_fifo));
> > +
> > +             if (kthread_should_stop())
> > +                     break;
> > +
> > +             if (!kfifo_get(&pcie->event_fifo, &event)) {
> > +                     dev_warn(pcie->dev, "EVENT FIFO is empty\n");
> > +                     continue;
> > +             }
> > +
> > +             switch (event) {
> > +             case EP_PEX_RST_DEASSERT:
> > +                     dev_info(pcie->dev, "EVENT: EP_PEX_RST_DEASSERT\n");
> > +                     pex_ep_event_pex_rst_deassert(pcie);
> > +                     break;
> > +
> > +             case EP_PEX_RST_ASSERT:
> > +                     dev_info(pcie->dev, "EVENT: EP_PEX_RST_ASSERT\n");
> > +                     pex_ep_event_pex_rst_assert(pcie);
> > +                     break;
> > +
> > +             case EP_HOT_RST_DONE:
> > +                     dev_info(pcie->dev, "EVENT: EP_HOT_RST_DONE\n");
> > +                     pex_ep_event_hot_rst_done(pcie);
> > +                     break;
> > +
> > +             case EP_BME_CHANGE:
> > +                     dev_info(pcie->dev, "EVENT: EP_BME_CHANGE\n");
> > +                     pex_ep_event_bme_change(pcie);
> > +                     break;
> > +
> > +             case EP_EVENT_EXIT:
> > +                     dev_info(pcie->dev, "EVENT: EP_EVENT_EXIT\n");
> > +                     return 0;
> > +
> > +             default:
> > +                     dev_warn(pcie->dev, "Invalid PCIe EP event: %u\n",
> > +                              event);
> > +                     break;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static irqreturn_t tegra_pcie_ep_pex_rst_irq(int irq, void *arg)
> > +{
> > +     struct tegra_pcie_dw *pcie = arg;
> > +
> > +     if (gpiod_get_value(pcie->pex_rst_gpiod)) {
> > +             if (!kfifo_put(&pcie->event_fifo, EP_PEX_RST_ASSERT)) {
> > +                     dev_err(pcie->dev, "EVENT FIFO is full\n");
> > +                     return IRQ_HANDLED;
> > +             }
> > +     } else {
> > +             if (!kfifo_put(&pcie->event_fifo, EP_PEX_RST_DEASSERT)) {
> > +                     dev_err(pcie->dev, "EVENT FIFO is full\n");
> > +                     return IRQ_HANDLED;
> > +             }
> > +     }
> > +
> > +     wake_up(&pcie->wq);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
>
> [...]
>
> > +static int tegra_pcie_config_ep(struct tegra_pcie_dw *pcie,
> > +                             struct platform_device *pdev)
> > +{
> > +     struct dw_pcie *pci = &pcie->pci;
> > +     struct device *dev = pcie->dev;
> > +     struct dw_pcie_ep *ep;
> > +     struct resource *res;
> > +     char *name;
> > +     int ret;
> > +
> > +     ep = &pci->ep;
> > +     ep->ops = &pcie_ep_ops;
> > +
> > +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> > +     if (!res)
> > +             return -EINVAL;
> > +
> > +     ep->phys_base = res->start;
> > +     ep->addr_size = resource_size(res);
> > +     ep->page_size = SZ_64K;
> > +
> > +     ret = gpiod_set_debounce(pcie->pex_rst_gpiod, PERST_DEBOUNCE_TIME);
> > +     if (ret < 0) {
> > +             dev_err(dev, "Failed to set PERST GPIO debounce time: %d\n",
> > +                     ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = gpiod_to_irq(pcie->pex_rst_gpiod);
> > +     if (ret < 0) {
> > +             dev_err(dev, "Failed to get IRQ for PERST GPIO: %d\n", ret);
> > +             return ret;
> > +     }
> > +     pcie->pex_rst_irq = (unsigned int)ret;
> > +
> > +     name = devm_kasprintf(dev, GFP_KERNEL, "tegra_pcie_%u_pex_rst_irq",
> > +                           pcie->cid);
> > +     if (!name) {
> > +             dev_err(dev, "Failed to create PERST IRQ string\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     irq_set_status_flags(pcie->pex_rst_irq, IRQ_NOAUTOEN);
> > +
> > +     ret = devm_request_irq(dev, pcie->pex_rst_irq,
> > +                            tegra_pcie_ep_pex_rst_irq,
> > +                            IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> > +                            name, (void *)pcie);
>
> I have the impression that a threaded IRQ is what you need, which
> will also remove some boilerplate in the process. Any reason why
> you can't use a threaded IRQ instead of a standalone kthread ?
No reason specifically. We started with kthreads and continued with 
that. I'll post a new series with threaded IRQ. It should be doable.

Thanks,
Vidya Sagar
> Thanks,
> Lorenzo