On Tue, Aug 25, 2015 at 7:25 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> > +
> > +static void altera_msi_isr(unsigned int irq, struct irq_desc *desc)
> > +{
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + struct altera_msi *msi;
> > + unsigned long status;
> > + u32 num_of_vectors;
> > + u32 bit;
> > + u32 virq;
> > +
> > + chained_irq_enter(chip, desc);
> > + msi = irq_desc_get_handler_data(desc);
> > + num_of_vectors = msi->num_of_vectors;
> > +
> > + do {
> > + status = msi_readl(msi, MSI_STATUS);
> > + if (!status)
> > + break;
> > +
> > + do {
> > + bit = find_first_bit(&status, num_of_vectors);
> > + /* Dummy read from vector to clear the interrupt */
> > + readl_relaxed(msi->vector_base + (bit * sizeof(u32)));
> > +
> > + virq = irq_find_mapping(msi->inner_domain, bit);
> > + if (virq && test_bit(bit, msi->used))
>
> I still wonder under which circumstances you can obtain a valid virq and
> yet not find the bit in the bitmap. Unless you have a scenario when that
> can happen, I suggest you remove that test.
>
> > + generic_handle_irq(virq);
> > + else
> > + dev_err(&msi->pdev->dev, "unexpected MSI\n");
> > +
> > + /* Clear the bit from status and repeat without reading
> > + * again status register. */
> > + __clear_bit(bit, &status);
> > + } while (status);
> > + } while (1);
>
> I think this could be rewritten as:
>
> while ((status = msi_readl(msi, MSI_STATUS)) != 0) {
> for_each_set_bit(bit, &status, msi->num_of_vectors) {
> /* Dummy read from vector to clear the interrupt */
> readl_relaxed(msi->vector_base + (bit * sizeof(u32)));
>
> virq = irq_find_mapping(msi->inner_domain, bit);
> if (virq)
> generic_handle_irq(virq);
> else
> dev_err(&msi->pdev->dev, "unexpected MSI\n");
> }
> }
>
> which saves the __clear_bit call and the double test of status which I
> find a bit awkward.
Okay.
>
> > +
> > + chained_irq_exit(chip, desc);
> > +}
> > +
> > +static struct irq_chip altera_msi_irq_chip = {
> > + .name = "Altera PCIe MSI",
> > + .irq_enable = pci_msi_unmask_irq,
> > + .irq_disable = pci_msi_mask_irq,
> > + .irq_mask = pci_msi_mask_irq,
> > + .irq_unmask = pci_msi_unmask_irq,
>
> Why do you have both enable/disable and unmask/mask? Specially pointing
> to the same function? This is completely useless. Just stick with
> unmask/mask and remove the enable/disable.
Okay.
>
> > +};
> > +
> > +static struct msi_domain_info altera_msi_domain_info = {
> > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > + MSI_FLAG_PCI_MSIX),
> > + .chip = &altera_msi_irq_chip,
> > +};
> > +
> > +static void altera_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> > +{
> > + struct altera_msi *msi = irq_data_get_irq_chip_data(data);
> > + phys_addr_t addr = msi->vector_phy + (data->hwirq * sizeof(u32));
> > +
> > + msg->address_lo = lower_32_bits(addr);
> > + msg->address_hi = upper_32_bits(addr);
> > + msg->data = data->hwirq;
> > +
> > + dev_dbg(&msi->pdev->dev, "msi#%d address_lo 0x%x address_hi 0x%x\n",
> > + (int)data->hwirq, msg->address_hi, msg->address_lo);
> > +}
> > +
> > +static int altera_msi_set_affinity(struct irq_data *irq_data,
> > + const struct cpumask *mask, bool force)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +static struct irq_chip altera_msi_bottom_irq_chip = {
> > + .name = "Altera MSI",
> > + .irq_compose_msi_msg = altera_compose_msi_msg,
> > + .irq_set_affinity = altera_msi_set_affinity,
> > +};
> > +
> > +static int altera_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > + unsigned int nr_irqs, void *args)
> > +{
> > + struct altera_msi *msi = domain->host_data;
> > + unsigned long bit;
> > + u32 mask;
> > +
> > + WARN_ON(nr_irqs != 1);
> > + mutex_lock(&msi->lock);
> > +
> > + bit = find_first_zero_bit(msi->used, msi->num_of_vectors);
> > + if (bit >= msi->num_of_vectors)
> > + return -ENOSPC;
> > +
> > + set_bit(bit, msi->used);
> > +
> > + mutex_unlock(&msi->lock);
> > +
> > + irq_domain_set_info(domain, virq, bit, &altera_msi_bottom_irq_chip,
> > + domain->host_data, handle_simple_irq,
> > + NULL, NULL);
> > + set_irq_flags(virq, IRQF_VALID);
> > +
> > + mask = msi_readl(msi, MSI_INTMASK);
> > + mask |= 1 << bit;
> > + msi_writel(msi, mask, MSI_INTMASK);
> > +
> > + return 0;
> > +}
> > +
> > +static void altera_irq_domain_free(struct irq_domain *domain,
> > + unsigned int virq, unsigned int nr_irqs)
> > +{
> > + struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > + struct altera_msi *msi = irq_data_get_irq_chip_data(d);
> > + u32 mask;
> > +
> > + mutex_lock(&msi->lock);
> > +
> > + if (!test_bit(d->hwirq, msi->used)) {
> > + dev_err(&msi->pdev->dev, "trying to free unused MSI#%lu\n",
> > + d->hwirq);
> > + } else {
> > + __clear_bit(d->hwirq, msi->used);
> > + mask = msi_readl(msi, MSI_INTMASK);
> > + mask &= ~(1 << d->hwirq);
> > + msi_writel(msi, mask, MSI_INTMASK);
> > + }
> > +
> > + mutex_unlock(&msi->lock);
> > +}
> > +
> > +static const struct irq_domain_ops msi_domain_ops = {
> > + .alloc = altera_irq_domain_alloc,
> > + .free = altera_irq_domain_free,
> > +};
> > +
> > +static int altera_allocate_domains(struct altera_msi *msi)
> > +{
> > + msi->inner_domain = irq_domain_add_linear(NULL, msi->num_of_vectors,
> > + &msi_domain_ops, msi);
> > + if (!msi->inner_domain) {
> > + dev_err(&msi->pdev->dev, "failed to create IRQ domain\n");
> > + return -ENOMEM;
> > + }
> > +
> > + msi->msi_domain = pci_msi_create_irq_domain(msi->pdev->dev.of_node,
> > + &altera_msi_domain_info, msi->inner_domain);
> > + if (!msi->msi_domain) {
> > + dev_err(&msi->pdev->dev, "failed to create MSI domain\n");
> > + irq_domain_remove(msi->inner_domain);
> > + return -ENOMEM;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void altera_free_domains(struct altera_msi *msi)
> > +{
> > + irq_domain_remove(msi->msi_domain);
> > + irq_domain_remove(msi->inner_domain);
> > +}
> > +
> > +static int altera_msi_remove(struct platform_device *pdev)
> > +{
> > + struct altera_msi *msi = platform_get_drvdata(pdev);
> > +
> > + msi_writel(msi, 0, MSI_INTMASK);
> > + irq_set_chained_handler(msi->irq, NULL);
> > + irq_set_handler_data(msi->irq, NULL);
> > +
> > + altera_free_domains(msi);
> > +
> > + platform_set_drvdata(pdev, NULL);
> > + return 0;
> > +}
> > +
> > +static int altera_msi_probe(struct platform_device *pdev)
> > +{
> > + struct altera_msi *msi;
> > + struct device_node *np = pdev->dev.of_node;
> > + struct resource *res;
> > + int ret;
> > +
> > + msi = devm_kzalloc(&pdev->dev, sizeof(struct altera_msi),
> > + GFP_KERNEL);
> > + if (!msi)
> > + return -ENOMEM;
> > +
> > + mutex_init(&msi->lock);
> > + msi->pdev = pdev;
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "csr");
> > + if (!res) {
> > + dev_err(&pdev->dev,
> > + "no csr memory resource defined\n");
> > + return -ENODEV;
> > + }
> > +
> > + msi->csr_base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(msi->csr_base)) {
> > + dev_err(&pdev->dev, "failed to map csr memory\n");
> > + return PTR_ERR(msi->csr_base);
> > + }
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > + "vector_slave");
> > + if (!res) {
> > + dev_err(&pdev->dev,
> > + "no vector_slave memory resource defined\n");
> > + return -ENODEV;
> > + }
> > +
> > + msi->vector_base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(msi->vector_base)) {
> > + dev_err(&pdev->dev, "failed to map vector_slave memory\n");
> > + return PTR_ERR(msi->vector_base);
> > + }
> > +
> > + msi->vector_phy = res->start;
> > +
> > + if (of_property_read_u32(np, "num-vectors", &msi->num_of_vectors)) {
> > + dev_err(&pdev->dev, "failed to parse the number of vectors\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = altera_allocate_domains(msi);
> > + if (ret)
> > + return ret;
> > +
> > + msi->irq = platform_get_irq(pdev, 0);
> > + if (msi->irq <= 0) {
> > + dev_err(&pdev->dev, "failed to map IRQ: %d\n", msi->irq);
> > + ret = -ENODEV;
> > + goto err;
> > + }
> > +
> > + irq_set_chained_handler_and_data(msi->irq, altera_msi_isr, msi);
> > + platform_set_drvdata(pdev, msi);
> > +
> > + return 0;
> > +
> > +err:
> > + altera_msi_remove(pdev);
> > + return ret;
> > +}
> > +
> > +static const struct of_device_id altera_msi_of_match[] = {
> > + { .compatible = "altr,msi-1.0", NULL },
> > + { },
> > +};
> > +
> > +static struct platform_driver altera_msi_driver = {
> > + .driver = {
> > + .name = "altera-msi",
> > + .of_match_table = altera_msi_of_match,
> > + },
> > + .probe = altera_msi_probe,
> > + .remove = altera_msi_remove,
> > +};
> > +
> > +static int __init altera_msi_init(void)
> > +{
> > + return platform_driver_register(&altera_msi_driver);
> > +}
> > +
> > +subsys_initcall(altera_msi_init);
> > +
> > +MODULE_AUTHOR("Ley Foon Tan <lftan@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Altera PCIe MSI support");
> > +MODULE_LICENSE("GPL v2");
> >
>
> Once you've fixed the above issues, you can add my:
>
> Reviewed-by: Marc Zyngier <marc.zyngier@xxxxxxx>
Okay, thanks.
Thanks.
Regards
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
