Re: [v1] PCI: mediatek: Remove MSI inner domain

From: Lorenzo Pieralisi
Date: Thu May 02 2019 - 12:44:06 EST

Next message: Amit Kucheria: "Re: [PATCH] ARM: multi_v7_defconfig: Enable panic and disk LED triggers"
Previous message: Jacob Pan: "Re: [PATCH v7 05/23] iommu: Introduce cache_invalidate API"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Jan 31, 2019 at 08:49:57PM +0800, Jianjun Wang wrote:
> On Thu, 2019-01-31 at 09:44 +0000, Marc Zyngier wrote:
> > On 31/01/2019 09:19, Honghui Zhang wrote:
> > > On Tue, 2019-01-22 at 17:37 +0800, Jianjun Wang wrote:
> > >> There is no need to create the inner domain as a parent for MSI domian,
> > >> some feature has been implemented by MSI framework.
> > >>
> > >> Remove the inner domain and its irq chip, it will be more closer to
> > >> hardware implementation.
> >
> > This is not about being closer to any HW implementation. This is about
> > having a uniform way to deal with MSI controllers, no matter how they
> > are implemented by the HW.
> >
> > So maybe you could start by explaining what this is trying to achieve.
> >
> > >>
> > > Hi, jianjun, I'm not quite familiar with the irq_chip framework, It was
> > > under Marc's great help with the first version of irq_chip solution
> > > code. I would like you to add him for the review.
> > >
> > > Thanks.
> > >
> > >> Signed-off-by: Jianjun Wang <jianjun.wang@xxxxxxxxxxxx>
> > >> ---
> > >> drivers/pci/controller/pcie-mediatek.c | 86 +++++++++++---------------
> > >> 1 file changed, 37 insertions(+), 49 deletions(-)
> > >>
> > >> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > >> index 8d05df56158b..f996a9a6331f 100644
> > >> --- a/drivers/pci/controller/pcie-mediatek.c
> > >> +++ b/drivers/pci/controller/pcie-mediatek.c
> > >> @@ -169,7 +169,6 @@ struct mtk_pcie_soc {
> > >> * @slot: port slot
> > >> * @irq: GIC irq
> > >> * @irq_domain: legacy INTx IRQ domain
> > >> - * @inner_domain: inner IRQ domain
> > >> * @msi_domain: MSI IRQ domain
> > >> * @lock: protect the msi_irq_in_use bitmap
> > >> * @msi_irq_in_use: bit map for assigned MSI IRQ
> > >> @@ -190,7 +189,6 @@ struct mtk_pcie_port {
> > >> u32 slot;
> > >> int irq;
> > >> struct irq_domain *irq_domain;
> > >> - struct irq_domain *inner_domain;
> > >> struct irq_domain *msi_domain;
> > >> struct mutex lock;
> > >> DECLARE_BITMAP(msi_irq_in_use, MTK_MSI_IRQS_NUM);
> > >> @@ -418,22 +416,15 @@ static void mtk_msi_ack_irq(struct irq_data *data)
> > >> u32 hwirq = data->hwirq;
> > >>
> > >> writel(1 << hwirq, port->base + PCIE_IMSI_STATUS);
> > >> + writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > >> }
> > >>
> > >> -static struct irq_chip mtk_msi_bottom_irq_chip = {
> > >> - .name = "MTK MSI",
> > >> - .irq_compose_msi_msg = mtk_compose_msi_msg,
> > >> - .irq_set_affinity = mtk_msi_set_affinity,
> > >> - .irq_ack = mtk_msi_ack_irq,
> > >> -};
> > >> -
> > >> -static int mtk_pcie_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > >> - unsigned int nr_irqs, void *args)
> > >> +static irq_hw_number_t mtk_pcie_msi_get_hwirq(struct msi_domain_info *info,
> > >> + msi_alloc_info_t *arg)
> > >> {
> > >> - struct mtk_pcie_port *port = domain->host_data;
> > >> - unsigned long bit;
> > >> + struct mtk_pcie_port *port = info->chip_data;
> > >> + irq_hw_number_t bit;
> > >>
> > >> - WARN_ON(nr_irqs != 1);
> > >> mutex_lock(&port->lock);
> > >>
> > >> bit = find_first_zero_bit(port->msi_irq_in_use, MTK_MSI_IRQS_NUM);
> > >> @@ -446,18 +437,14 @@ static int mtk_pcie_irq_domain_alloc(struct irq_domain *domain, unsigned int vir
> > >>
> > >> mutex_unlock(&port->lock);
> > >>
> > >> - irq_domain_set_info(domain, virq, bit, &mtk_msi_bottom_irq_chip,
> > >> - domain->host_data, handle_edge_irq,
> > >> - NULL, NULL);
> > >> -
> > >> - return 0;
> > >> + return bit;
> >
> > Why do you need to override the get_hwirq method? Using the generic
> > PCI/MSI version has the advantage of giving you a universal encoding
> > which makes debugging much easier.
>
> Hi Marc,
>
> In previous patch, we create a inner_domain as a parent for msi_domain,
> when we allocate a irq for MSI, the work flow of each domain will be the
> following:
>
> inner_domain:
> 1. Allocated a irq bit from bitmap as this domain's hwirq;
> 2. Mapping with system virtual irq number;
> 3. Set irq chip and irq handler;
> 4. Send MSI message to EP.
>
> msi_domain:
> 1. Calculate a hwirq;
> 2. Mapping with system virtual irq number;
> 3. Set irq chip which from info->chip and irq handler if defined in
> info.
> 4. Send MSI message to EP or trigger parent domain to send the message.
>
> The last three steps looks similar, if we override the get_hwirq method
> and set irq chip and handler to info structure, MSI framework will do
> the rest of thing. I think it will be more simple and easy to understand
> the driver's work flow.
>
> Further more, if we try to enhance the interrupt performance, such as
> connect the MSI interrupt line to GIC directly in hardware, we will need
> to set gic domain as the parent, in that case, there will be a lot of
> work to do to replace the inner domain.

I do not understand what you mean, I am sorry. I won't review v2 until
we have an understanding of what this patch should achieve and we
have a clear reason why we need it, more specifically I do not
understand what it has to do with performance (keeping in mind what
Marc said about the IRQ controllers representation, which has a
reason to be there on its own).

Thanks,
Lorenzo

> Thanks.
> >
> > >> }
> > >>
> > >> -static void mtk_pcie_irq_domain_free(struct irq_domain *domain,
> > >> - unsigned int virq, unsigned int nr_irqs)
> > >> +static void mtk_pcie_msi_free(struct irq_domain *domain,
> > >> + struct msi_domain_info *info, unsigned int virq)
> > >> {
> > >> struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > >> - struct mtk_pcie_port *port = irq_data_get_irq_chip_data(d);
> > >> + struct mtk_pcie_port *port = info->chip_data;
> > >>
> > >> mutex_lock(&port->lock);
> > >>
> > >> @@ -468,46 +455,50 @@ static void mtk_pcie_irq_domain_free(struct irq_domain *domain,
> > >> __clear_bit(d->hwirq, port->msi_irq_in_use);
> > >>
> > >> mutex_unlock(&port->lock);
> > >> -
> > >> - irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> > >> }
> > >>
> > >> -static const struct irq_domain_ops msi_domain_ops = {
> > >> - .alloc = mtk_pcie_irq_domain_alloc,
> > >> - .free = mtk_pcie_irq_domain_free,
> > >> +static struct msi_domain_ops mtk_msi_domain_ops = {
> > >> + .get_hwirq = mtk_pcie_msi_get_hwirq,
> > >> + .msi_free = mtk_pcie_msi_free,
> > >> };
> > >>
> > >> static struct irq_chip mtk_msi_irq_chip = {
> > >> - .name = "MTK PCIe MSI",
> > >> - .irq_ack = irq_chip_ack_parent,
> > >> - .irq_mask = pci_msi_mask_irq,
> > >> - .irq_unmask = pci_msi_unmask_irq,
> > >> + .name = "MTK PCIe",
> > >> + .irq_compose_msi_msg = mtk_compose_msi_msg,
> > >> + .irq_write_msi_msg = pci_msi_domain_write_msg,
> > >> + .irq_set_affinity = mtk_msi_set_affinity,
> > >> + .irq_ack = mtk_msi_ack_irq,
> > >> + .irq_mask = pci_msi_mask_irq,
> > >> + .irq_unmask = pci_msi_unmask_irq,
> > >> };
> > >>
> > >> static struct msi_domain_info mtk_msi_domain_info = {
> > >> - .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > >> - MSI_FLAG_PCI_MSIX),
> > >> - .chip = &mtk_msi_irq_chip,
> > >> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS |
> > >> + MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_PCI_MSIX),
> > >> + .ops = &mtk_msi_domain_ops,
> > >> + .chip = &mtk_msi_irq_chip,
> > >> + .handler = handle_edge_irq,
> > >> + .handler_name = "MSI",
> > >> };
> > >>
> > >> static int mtk_pcie_allocate_msi_domains(struct mtk_pcie_port *port)
> > >> {
> > >> - struct fwnode_handle *fwnode = of_node_to_fwnode(port->pcie->dev->of_node);
> > >> + struct device *dev = port->pcie->dev;
> > >> + struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node);
> > >> + struct msi_domain_info *info;
> > >>
> > >> mutex_init(&port->lock);
> > >>
> > >> - port->inner_domain = irq_domain_create_linear(fwnode, MTK_MSI_IRQS_NUM,
> > >> - &msi_domain_ops, port);
> > >> - if (!port->inner_domain) {
> > >> - dev_err(port->pcie->dev, "failed to create IRQ domain\n");
> > >> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> > >> + if (!info)
> > >> return -ENOMEM;
> > >> - }
> > >>
> > >> - port->msi_domain = pci_msi_create_irq_domain(fwnode, &mtk_msi_domain_info,
> > >> - port->inner_domain);
> > >> + memcpy(info, &mtk_msi_domain_info, sizeof(*info));
> > >> + info->chip_data = port;
> > >> +
> > >
> > > I'm not really like this memcpy of msi_domain_info, but I do not have a
> > > better idea to prevent the mixed of mtk_pcie_port data.
> >
> > So we're basically trading an indirection for another. What's the gain?
>
> There is usually more than one PCIe port in each SoC, we use
> mtk_pcie_port data to describe it, in previous version, we pass the port
> data as inner domain's host_data. When remove the inner domain, we also
> need to pass the port data and should prevent to mix with another port,
> so I thank maybe we can make a copy for each port and set port data as
> it's chip_data.
> >
> > >
> > >> + port->msi_domain = pci_msi_create_irq_domain(fwnode, info, NULL);
> > >> if (!port->msi_domain) {
> > >> - dev_err(port->pcie->dev, "failed to create MSI domain\n");
> > >> - irq_domain_remove(port->inner_domain);
> > >> + dev_err(dev, "failed to create MSI domain\n");
> > >> return -ENOMEM;
> > >> }
> > >>
> > >> @@ -541,8 +532,6 @@ static void mtk_pcie_irq_teardown(struct mtk_pcie *pcie)
> > >> if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > >> if (port->msi_domain)
> > >> irq_domain_remove(port->msi_domain);
> > >> - if (port->inner_domain)
> > >> - irq_domain_remove(port->inner_domain);
> > >> }
> > >>
> > >> irq_dispose_mapping(port->irq);
> > >> @@ -619,12 +608,11 @@ static void mtk_pcie_intr_handler(struct irq_desc *desc)
> > >>
> > >> while ((imsi_status = readl(port->base + PCIE_IMSI_STATUS))) {
> > >> for_each_set_bit(bit, &imsi_status, MTK_MSI_IRQS_NUM) {
> > >> - virq = irq_find_mapping(port->inner_domain, bit);
> > >> + virq = irq_find_mapping(
> > >> + port->msi_domain, bit);
> > >> generic_handle_irq(virq);
> > >> }
> > >> }
> > >> - /* Clear MSI interrupt status */
> > >> - writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > >> }
> > >
> > > why change this irq status clear flow?
> >
> > I think this is trying move everything to the irq_ack callback. But
> > that's a change of semantics, and I'd like it explained. It certainly
> > feels wrong.
> Yes, I confused with each irq's ack callback, it doesn't need to be
> changed.
>
> Thanks.
> >
> > Overall, this patch as it stands (without any real explanation) doesn't
> > feel me with confidence. It introduces significant differences in the
> > way we build PCI/MSI domains, and I'd like to understand why.
> >
> > Thanks,
> >
> > M.
>
>

Next message: Amit Kucheria: "Re: [PATCH] ARM: multi_v7_defconfig: Enable panic and disk LED triggers"
Previous message: Jacob Pan: "Re: [PATCH v7 05/23] iommu: Introduce cache_invalidate API"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]