Hi Marc, Thanks for your review. Please see my comments inline.
Thanks, Minghuan > -----Original Message----- > From: Marc Zyngier [mailto:marc.zyng...@arm.com] > Sent: Thursday, January 05, 2017 11:33 PM > To: M.H. Lian <minghuan.l...@nxp.com>; linux-arm- > ker...@lists.infradead.org; linux-kernel@vger.kernel.org; > devicet...@vger.kernel.org > Cc: Rob Herring <r...@kernel.org>; Jason Cooper > <ja...@lakedaemon.net>; Roy Zang <roy.z...@nxp.com>; Mingkai Hu > <mingkai...@nxp.com>; Stuart Yoder <stuart.yo...@nxp.com>; Leo Li > <leoyang...@nxp.com>; Scott Wood <scott.w...@nxp.com> > Subject: Re: [PATCH v2,9/9] irqchip/ls-scfg-msi: add MSI affinity support > > On 05/01/17 08:10, Minghuan Lian wrote: > > For LS1046a and LS1043a v1.1, the MSI controller has 4 MSIRs and 4 > > CPUs. A GIC SPI interrupt of MSIR can be associated with a CPU. > > When changing MSI interrupt affinity, this MSI will be moved to the > > corresponding MSIR and MSI message data will be changed according to > > MSIR. when requesting a MSI, the bits of all 4 MSIR will be reserved. > > The parameter 'msi_affinity_flag' is provide to change this mode. > > "lsmsi=no-affinity" will disable affinity, all MSI can only be > > associated with CPU 0. > > > > Signed-off-by: Minghuan Lian <minghuan.l...@nxp.com> > > --- > > v2-v1: > > - None > > > > drivers/irqchip/irq-ls-scfg-msi.c | 75 > > ++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 70 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/irqchip/irq-ls-scfg-msi.c > > b/drivers/irqchip/irq-ls-scfg-msi.c > > index dc19569..753fe39 100644 > > --- a/drivers/irqchip/irq-ls-scfg-msi.c > > +++ b/drivers/irqchip/irq-ls-scfg-msi.c > > @@ -40,6 +40,7 @@ struct ls_scfg_msir { > > unsigned int gic_irq; > > unsigned int bit_start; > > unsigned int bit_end; > > + unsigned int srs; /* Shared interrupt register select */ > > void __iomem *reg; > > }; > > > > @@ -70,6 +71,19 @@ struct ls_scfg_msi { > > .chip = &ls_scfg_msi_irq_chip, > > }; > > > > +static int msi_affinity_flag = 1; > > + > > +static int __init early_parse_ls_scfg_msi(char *p) { > > + if (p && strncmp(p, "no-affinity", 11) == 0) > > + msi_affinity_flag = 0; > > + else > > + msi_affinity_flag = 1; > > + > > + return 0; > > +} > > +early_param("lsmsi", early_parse_ls_scfg_msi); > > What is the point of this option? If feels like an unnecessary complexity. [Minghuan Lian] For LS1043a rev1.1, there are only 8 MSI interrupts for each MSI controller when enable affinity. (32 MSI interrupts are evenly distributed to 4 cores). 3 MSI controllers can only provide 32 MSI interrupts. When disable affinity, the MSI interrupts number of 3 controllers can up to 32*3= 96. 32 MSI interrupts may be not enough. For example: an Ethernet card with 4 ports, each port needs 4 RX, 4TX and 1 error interrupts, totally need 4*(4+4+1) 36 MSI interrupts. With the parameter "lsmsi=no-affinity" this Ethernet card can work well, although the performance is poor. > > > + > > static void ls_scfg_msi_compose_msg(struct irq_data *data, struct > > msi_msg *msg) { > > struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(data); > @@ > > -77,12 +91,43 @@ static void ls_scfg_msi_compose_msg(struct irq_data > *data, struct msi_msg *msg) > > msg->address_hi = upper_32_bits(msi_data->msiir_addr); > > msg->address_lo = lower_32_bits(msi_data->msiir_addr); > > msg->data = data->hwirq; > > + > > + if (msi_affinity_flag) { > > + u32 msir_index; > > + > > + msir_index = cpumask_first(data->common->affinity); > > + if (msir_index >= msi_data->msir_num) > > + msir_index = 0; > > How can this happen? [Minghuan Lian] This will never happen. I will remove this "if" sentence. > > > + > > + msg->data |= msir_index; > > How do you guarantee that the low bits were clear? Please document the > way you encode your MSI payload. [Minghuan Lian] the available message data is a bytes. 7 6 5 4 3 2 1 0 | - | ibs | srs | SRS bit 0-1 is used to select MSIR register/CPU. A MSIR is associated with a CPU. IBS bit2-6 of ls1046, bit2-4 for ls1043 is used to select bit of the MSIR. When enable affinity, only bits of MSIR0(srs = 0 cpu0) is be freed for requesting MSI. all bits of the MSIR1-3(cpu1-3) are reserved to be sure this MSI can be successfully associated with cpu 1-3. So, When requesting a MSI interrupt, srs always is 0. The hwirq always equals bits number of the MSIR0(SRS = 0). First, MSI message data payload equals hwirq, and then plus the real SRS according to smp_affinity. This MSI interrupt will be routed to the corresponding the MSIR/CPU. > > > + } > > } > > > > static int ls_scfg_msi_set_affinity(struct irq_data *irq_data, > > const struct cpumask *mask, bool force) { > > - return -EINVAL; > > + struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(irq_data); > > + u32 cpu; > > + > > + if (!msi_affinity_flag) > > + return -EINVAL; > > + > > + if (!force) > > + cpu = cpumask_any_and(mask, cpu_online_mask); > > + else > > + cpu = cpumask_first(mask); > > + > > + if (cpu >= msi_data->msir_num) > > + return -EINVAL; > > + > > + if (msi_data->msir[cpu].gic_irq <= 0) { > > + pr_warn("cannot bind the irq to cpu%d\n", cpu); > > Please don't. Returning an error is enough. If you really want to have > something, turn it into a proper debug message. [Minghuan Lian] ok, I will change it. > > > + return -EINVAL; > > + } > > + > > + cpumask_copy(irq_data->common->affinity, mask); > > + > > + return IRQ_SET_MASK_OK; > > } > > > > static struct irq_chip ls_scfg_msi_parent_chip = { @@ -158,7 +203,7 > > @@ static void ls_scfg_msi_irq_handler(struct irq_desc *desc) > > > > for_each_set_bit_from(pos, &val, size) { > > hwirq = ((msir->bit_end - pos) << msi_data->cfg->ibs_shift) | > > - msir->index; > > + msir->srs; > > virq = irq_find_mapping(msi_data->parent, hwirq); > > if (virq) > > generic_handle_irq(virq); > > @@ -221,10 +266,19 @@ static int ls_scfg_msi_setup_hwirq(struct > ls_scfg_msi *msi_data, int index) > > ls_scfg_msi_irq_handler, > > msir); > > > > + if (msi_affinity_flag) { > > + /* Associate MSIR interrupt to the cpu */ > > + irq_set_affinity(msir->gic_irq, get_cpu_mask(index)); > > + msir->srs = 0; /* This value is determined by the CPU */ > > + } else > > + msir->srs = index; > > + > > /* Release the hwirqs corresponding to this MSIR */ > > - for (i = 0; i < msi_data->cfg->msir_irqs; i++) { > > - hwirq = i << msi_data->cfg->ibs_shift | msir->index; > > - bitmap_clear(msi_data->used, hwirq, 1); > > + if (!msi_affinity_flag || msir->index == 0) { > > + for (i = 0; i < msi_data->cfg->msir_irqs; i++) { > > + hwirq = i << msi_data->cfg->ibs_shift | msir->index; > > + bitmap_clear(msi_data->used, hwirq, 1); > > + } > > } > > > > return 0; > > @@ -316,6 +370,17 @@ static int ls_scfg_msi_probe(struct > platform_device *pdev) > > bitmap_set(msi_data->used, 0, msi_data->irqs_num); > > > > msi_data->msir_num = of_irq_count(pdev->dev.of_node); > > + > > + if (msi_affinity_flag) { > > + u32 cpu_num; > > + > > + cpu_num = num_possible_cpus(); > > + if (msi_data->msir_num >= cpu_num) > > + msi_data->msir_num = cpu_num; > > + else > > + msi_affinity_flag = 0; > > + } > > + > > msi_data->msir = devm_kcalloc(&pdev->dev, msi_data->msir_num, > > sizeof(*msi_data->msir), > > GFP_KERNEL); > > > > This is a very confusing patch. Please get rid of this useless option and > document how you encode the routing in the hwirq. [Minghuan Lian] Both LS1021a and LS1043av1.0 have only a MSIR, a gic interrupt. MSI controllers cannot support affinity. Then LS1046a/LS1043av1.1 extends MSIR number to 4 same to cpu number. So each MSIR with a GIC interrupt can be associated with a cpu. To keep simple, the first patch for ls1046 and ls1043av1.1 keep the same way with ls1021 and ls1043av1.0 that does not support affinity and all interrupts of MSIR0-3 are different and can be used for requesting MSI interrupts. This patch is to enable affinity, that means, for ls1046a and ls1043av1.1, the same bit of MSIR0-3 will be looked as one interrupt using the same hwirq. And MSIRN only is used when the MSI interrupt is associated with the corresponding cpu. > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
