[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 03/19] xen/arm: its: Port ITS driver to xen
On Mon, Mar 16, 2015 at 7:50 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote: > On 16/03/15 14:06, Vijay Kilari wrote: >>>> @@ -343,17 +357,23 @@ static int its_queue_full(struct its_node *its) >>>> static struct its_cmd_block *its_allocate_entry(struct its_node *its) >>>> { >>>> struct its_cmd_block *cmd; >>>> - u32 count = 1000000; /* 1s! */ >>>> + bool_t timeout = 0; >>>> + s_time_t deadline = NOW() + MILLISECS(1000); >>>> >>>> while (its_queue_full(its)) { >>>> - count--; >>>> - if (!count) { >>>> - pr_err_ratelimited("ITS queue not draining\n"); >>>> - return NULL; >>>> + if ( NOW() > deadline ) >>>> + { >>>> + timeout = 1; >>>> + break; >>>> } >>>> cpu_relax(); >>>> udelay(1); >>>> } >>>> + if ( timeout ) >>>> + { >>>> + its_err("ITS queue not draining\n"); >>>> + return NULL; >>>> + } >>> >>> >>> Why do you need to modify the loop? It looks like to me it will work Xen >>> too. >> >> I remember we used similar approach for GICv3. > > The GICv3 driver is not really imported from Linux... The loop solution > was suggested by Ian in order to avoid an infinite loop. > > Anyway, that doesn't make a reason to change a valid code... > >>> >>>> /* >>>> - * irqchip functions - assumes MSI, mostly. >>>> - */ >>>> - >>>> -static void lpi_set_config(struct its_device *its_dev, u32 hwirq, >>>> - u32 id, int enable) >>>> -{ >>>> - u8 *cfg = page_address(gic_rdists->prop_page) + hwirq - 8192; >>>> - >>>> - if (enable) >>>> - *cfg |= LPI_PROP_ENABLED; >>>> - else >>>> - *cfg &= ~LPI_PROP_ENABLED; >>>> - >>>> - /* >>>> - * Make the above write visible to the redistributors. >>>> - * And yes, we're flushing exactly: One. Single. Byte. >>>> - * Humpf... >>>> - */ >>>> - if (gic_rdists->flags & RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING) >>>> - __flush_dcache_area(cfg, sizeof(*cfg)); >>>> - else >>>> - dsb(ishst); >>>> - its_send_inv(its_dev, id); >>>> -} >>>> - >>>> -static inline u16 its_msi_get_entry_nr(struct msi_desc *desc) >>>> -{ >>>> - return desc->msi_attrib.entry_nr; >>>> -} >>>> - >>>> -static void its_mask_irq(struct irq_data *d) >>>> -{ >>>> - struct its_device *its_dev = irq_data_get_irq_handler_data(d); >>>> - u32 id; >>>> - >>>> - /* If MSI, propagate the mask to the RC */ >>>> - if (IS_ENABLED(CONFIG_PCI_MSI) && d->msi_desc) { >>>> - id = its_msi_get_entry_nr(d->msi_desc); >>>> - mask_msi_irq(d); >>>> - } else { >>>> - id = d->hwirq; >>>> - } >>>> - >>>> - lpi_set_config(its_dev, d->hwirq, id, 0); >>>> -} >>>> - >>>> -static void its_unmask_irq(struct irq_data *d) >>>> -{ >>>> - struct its_device *its_dev = irq_data_get_irq_handler_data(d); >>>> - u32 id; >>>> - >>>> - /* If MSI, propagate the unmask to the RC */ >>>> - if (IS_ENABLED(CONFIG_PCI_MSI) && d->msi_desc) { >>>> - id = its_msi_get_entry_nr(d->msi_desc); >>>> - unmask_msi_irq(d); >>>> - } else { >>>> - id = d->hwirq; >>>> - } >>>> - >>>> - lpi_set_config(its_dev, d->hwirq, id, 1); >>>> -} >>>> - >>>> -static void its_eoi_irq(struct irq_data *d) >>>> -{ >>>> - gic_write_eoir(d->hwirq); >>>> -} >>>> - >>>> -static int its_set_affinity(struct irq_data *d, const struct cpumask >>>> *mask_val, >>>> - bool force) >>>> -{ >>>> - unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask); >>>> - struct its_device *its_dev = irq_data_get_irq_handler_data(d); >>>> - struct its_collection *target_col; >>>> - u32 id; >>>> - >>>> - if (cpu >= nr_cpu_ids) >>>> - return -EINVAL; >>>> - >>>> - target_col = &its_dev->its->collections[cpu]; >>>> - if (IS_ENABLED(CONFIG_PCI_MSI) && d->msi_desc) >>>> - id = its_msi_get_entry_nr(d->msi_desc); >>>> - else >>>> - id = d->hwirq; >>>> - its_send_movi(its_dev, target_col, id); >>>> - its_dev->collection = target_col; >>>> - >>>> - return IRQ_SET_MASK_OK; >>>> -} >>>> - >>>> -static struct irq_chip its_irq_chip = { >>>> - .name = "ITS", >>>> - .irq_mask = its_mask_irq, >>>> - .irq_unmask = its_unmask_irq, >>>> - .irq_eoi = its_eoi_irq, >>>> - .irq_set_affinity = its_set_affinity, >>>> -}; >>>> - >>> >>> >>> Most of those callbacks seems useful to me. Why did you drop them? >> >> There is no irq_chip in xen. So thought of removing in completely and >> adding as separate patch > > What the point to remove and re-add code later? Except making more > difficult to review. > > If this code is valid please move in an #if 0/#endif > >> >>>> >>>> - >>>> - >>>> -static int its_msi_get_vec_count(struct pci_dev *pdev, struct msi_desc >>>> *desc) >>>> -{ >>>> -#ifdef CONFIG_PCI_MSI >>>> - if (desc->msi_attrib.is_msix) >>>> - return pci_msix_vec_count(pdev); >>>> - else >>>> - return pci_msi_vec_count(pdev); >>>> -#else >>>> - return -EINVAL; >>>> -#endif >>>> -} >>>> - >>>> -int pci_requester_id(struct pci_dev *dev); >>>> -static int its_msi_setup_irq(struct msi_chip *chip, >>>> - struct pci_dev *pdev, >>>> - struct msi_desc *desc) >>>> -{ >>>> - struct its_node *its = container_of(chip, struct its_node, msi_chip); >>>> - struct its_device *its_dev; >>>> - struct msi_msg msg; >>>> - unsigned int irq; >>>> - u64 addr; >>>> - int hwirq; >>>> - int err; >>>> - u32 dev_id = pci_requester_id(pdev); >>>> - u32 vec_nr; >>>> - >>>> - its_dev = its_find_device(its, dev_id); >>>> - if (!its_dev) { >>>> - int nvec = its_msi_get_vec_count(pdev, desc); >>>> - if (WARN_ON(nvec <= 0)) >>>> - return nvec; >>>> - its_dev = its_create_device(its, dev_id, nvec); >>>> - } >>>> - if (!its_dev) >>>> - return -ENOMEM; >>>> - vec_nr = its_msi_get_entry_nr(desc); >>>> - err = its_alloc_device_irq(its_dev, vec_nr, &hwirq, &irq); >>>> - if (err) >>>> - return err; >>>> - >>>> - irq_set_msi_desc(irq, desc); >>>> - irq_set_handler_data(irq, its_dev); >>>> - >>>> - addr = its->phys_base + GITS_TRANSLATER; >>>> - >>>> - msg.address_lo = (u32)addr; >>>> - msg.address_hi = (u32)(addr >> 32); >>>> - msg.data = vec_nr; >>>> - >>>> - write_msi_msg(irq, &msg); >>>> - return 0; >>>> -} >>>> - >>>> -static void its_msi_teardown_irq(struct msi_chip *chip, unsigned int irq) >>>> -{ >>>> - struct irq_data *d = irq_get_irq_data(irq); >>>> - struct its_device *its_dev = irq_data_get_irq_handler_data(d); >>>> - >>>> - BUG_ON(d->hwirq < its_dev->lpi_base || /* OMG! */ >>>> - d->hwirq > (its_dev->lpi_base + its_dev->nr_lpis)); >>>> - >>>> - /* Stop the delivery of interrupts */ >>>> - its_send_discard(its_dev, its_msi_get_entry_nr(d->msi_desc)); >>>> - >>>> - /* Mark interrupt index as unused, and clear the mapping */ >>>> - clear_bit(d->hwirq - its_dev->lpi_base, its_dev->lpi_map); >>>> - irq_dispose_mapping(irq); >>>> - >>>> - /* If all interrupts have been freed, start mopping the floor */ >>>> - if (bitmap_empty(its_dev->lpi_map, its_dev->nr_lpis)) { >>>> - its_lpi_free(its_dev->lpi_map, >>>> - its_dev->lpi_base, >>>> - its_dev->nr_lpis); >>>> - >>>> - /* Unmap device/itt */ >>>> - its_send_mapd(its_dev, 0); >>>> - its_free_device(its_dev); >>>> - } >>>> -} >>>> - >>> >>> >>> Those 2 functions seems useful for me. Why did you drop them? >> >> These are callbacks registered for msi_chip in linux, Since we have removed >> msi_chip from its_node structure, these functions are supposed to be removed. > > How do you register/unregister MSI in this case? On receiving MAPD/MAPVI command _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |