Date:   Tue, 3 Mar 2020 16:05:06 +0530
From:   Vidya Sagar <vidyas@...dia.com>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>
CC:     <bhelgaas@...gle.com>, <robh+dt@...nel.org>,
        <thierry.reding@...il.com>, <jonathanh@...dia.com>,
        <andrew.murray@....com>, <kishon@...com>,
        <gustavo.pimentel@...opsys.com>, <linux-pci@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <kthota@...dia.com>,
        <mmaddireddy@...dia.com>, <sagar.tv@...il.com>
Subject: Re: [PATCH V3 5/5] PCI: tegra: Add support for PCIe endpoint mode in
 Tegra194



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@...dia.com>
>> ---
>> 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
> 

Powered by blists - more mailing lists