Re: [PATCH v4 2/2] PCI: uniphier: Add UniPhier PCIe host controller support

2018-12-04 Thread Kunihiko Hayashi
Hi Lorenzo,

On Tue, 4 Dec 2018 15:12:27 +  wrote:

> On Wed, Nov 28, 2018 at 01:04:26PM +0900, Kunihiko Hayashi wrote:
> 
> [...]
> 
> > +static void uniphier_pcie_irq_ack(struct irq_data *d)
> > +{
> > +   struct pcie_port *pp = irq_data_get_irq_chip_data(d);
> > +   struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +   struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> > +   u32 val;
> > +
> > +   val = readl(priv->base + PCL_RCV_INTX);
> > +   val &= ~PCL_RCV_INTX_ALL_STATUS;
> > +   val |= BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_STATUS_SHIFT);
> > +   writel(val, priv->base + PCL_RCV_INTX);
> > +}
> > +
> > +static void uniphier_pcie_irq_mask(struct irq_data *d)
> > +{
> > +   struct pcie_port *pp = irq_data_get_irq_chip_data(d);
> > +   struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +   struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> > +   u32 val;
> > +
> > +   val = readl(priv->base + PCL_RCV_INTX);
> > +   val &= ~PCL_RCV_INTX_ALL_STATUS;
> > +   val |= BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_MASK_SHIFT);
> > +   writel(val, priv->base + PCL_RCV_INTX);
> > +}
> > +
> > +static void uniphier_pcie_irq_unmask(struct irq_data *d)
> > +{
> > +   struct pcie_port *pp = irq_data_get_irq_chip_data(d);
> > +   struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +   struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> > +   u32 val;
> > +
> > +   val = readl(priv->base + PCL_RCV_INTX);
> > +   val &= ~PCL_RCV_INTX_ALL_STATUS;
> 
> I have noticed this operation is carried out on ACK/MASK/UNMASK,
> what's its purpose ?

Thanks for pointing out. This is wrong.

The register PCL_RCV_INTX has 3 parts of bits.

#define PCL_RCV_INTX_ALL_ENABLE GENMASK(19, 16)
#define PCL_RCV_INTX_ALL_MASK   GENMASK(11, 8)
#define PCL_RCV_INTX_ALL_STATUS GENMASK(3, 0)

In the mask/unmask operation,
We should use PCL_RCV_INTX_ALL_MASK to represent 'mask bits',
not PCL_RCV_INTX_ALL_STATUS.
And the ack operation is effective by writing 1 to 'status bits'.

Thank you,

---
Best Regards,
Kunihiko Hayashi




Re: [PATCH v4 2/2] PCI: uniphier: Add UniPhier PCIe host controller support

2018-12-04 Thread Lorenzo Pieralisi
On Wed, Nov 28, 2018 at 01:04:26PM +0900, Kunihiko Hayashi wrote:

[...]

> +static void uniphier_pcie_irq_ack(struct irq_data *d)
> +{
> + struct pcie_port *pp = irq_data_get_irq_chip_data(d);
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> + u32 val;
> +
> + val = readl(priv->base + PCL_RCV_INTX);
> + val &= ~PCL_RCV_INTX_ALL_STATUS;
> + val |= BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_STATUS_SHIFT);
> + writel(val, priv->base + PCL_RCV_INTX);
> +}
> +
> +static void uniphier_pcie_irq_mask(struct irq_data *d)
> +{
> + struct pcie_port *pp = irq_data_get_irq_chip_data(d);
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> + u32 val;
> +
> + val = readl(priv->base + PCL_RCV_INTX);
> + val &= ~PCL_RCV_INTX_ALL_STATUS;
> + val |= BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_MASK_SHIFT);
> + writel(val, priv->base + PCL_RCV_INTX);
> +}
> +
> +static void uniphier_pcie_irq_unmask(struct irq_data *d)
> +{
> + struct pcie_port *pp = irq_data_get_irq_chip_data(d);
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> + u32 val;
> +
> + val = readl(priv->base + PCL_RCV_INTX);
> + val &= ~PCL_RCV_INTX_ALL_STATUS;

I have noticed this operation is carried out on ACK/MASK/UNMASK,
what's its purpose ?

Lorenzo

> + val &= ~BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_MASK_SHIFT);
> + writel(val, priv->base + PCL_RCV_INTX);
> +}
> +
> +static struct irq_chip uniphier_pcie_irq_chip = {
> + .name = "PCI",
> + .irq_ack = uniphier_pcie_irq_ack,
> + .irq_mask = uniphier_pcie_irq_mask,
> + .irq_unmask = uniphier_pcie_irq_unmask,
> +};
> +
> +static int uniphier_pcie_intx_map(struct irq_domain *domain, unsigned int 
> irq,
> +   irq_hw_number_t hwirq)
> +{
> + irq_set_chip_and_handler(irq, &uniphier_pcie_irq_chip,
> +  handle_level_irq);
> + irq_set_chip_data(irq, domain->host_data);
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops uniphier_intx_domain_ops = {
> + .map = uniphier_pcie_intx_map,
> +};
> +
> +static void uniphier_pcie_irq_handler(struct irq_desc *desc)
> +{
> + struct pcie_port *pp = irq_desc_get_handler_data(desc);
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + unsigned long reg;
> + u32 val, bit, virq;
> +
> + /* INT for debug */
> + val = readl(priv->base + PCL_RCV_INT);
> +
> + if (val & PCL_CFG_BW_MGT_STATUS)
> + dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
> + if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
> + dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
> + if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
> + dev_dbg(pci->dev, "Root Error\n");
> + if (val & PCL_CFG_PME_MSI_STATUS)
> + dev_dbg(pci->dev, "PME Interrupt\n");
> +
> + writel(val, priv->base + PCL_RCV_INT);
> +
> + /* INTx */
> + chained_irq_enter(chip, desc);
> +
> + val = readl(priv->base + PCL_RCV_INTX);
> + reg = FIELD_GET(PCL_RCV_INTX_ALL_STATUS, val);
> +
> + for_each_set_bit(bit, ®, PCI_NUM_INTX) {
> + virq = irq_linear_revmap(priv->legacy_irq_domain, bit);
> + generic_handle_irq(virq);
> + }
> +
> + chained_irq_exit(chip, desc);
> +}
> +
> +static int uniphier_pcie_config_legacy_irq(struct pcie_port *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> + struct device_node *np = pci->dev->of_node;
> + struct device_node *np_intc;
> +
> + np_intc = of_get_child_by_name(np, "legacy-interrupt-controller");
> + if (!np_intc) {
> + dev_err(pci->dev, "Failed to get legacy-interrupt-controller 
> node\n");
> + return -EINVAL;
> + }
> +
> + pp->irq = irq_of_parse_and_map(np_intc, 0);
> + if (!pp->irq) {
> + dev_err(pci->dev, "Failed to get an IRQ entry in 
> legacy-interrupt-controller\n");
> + return -EINVAL;
> + }
> +
> + priv->legacy_irq_domain = irq_domain_add_linear(np_intc, PCI_NUM_INTX,
> + &uniphier_intx_domain_ops, pp);
> + if (!priv->legacy_irq_domain) {
> + dev_err(pci->dev, "Failed to get INTx domain\n");
> + return -ENODEV;
> + }
> +
> + irq_set_chained_handler_and_data(pp->irq, uniphier_pcie_irq_handler,
> +  pp);
> +
> + return 0;
> +}
> +
> +static int uniphier_pcie_host_init(struct pcie_port *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> +