Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Bharat Kumar Gogada <bharatku@xilinx.com>
To:     Marc Zyngier <maz@kernel.org>
CC:     "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "bhelgaas@google.com" <bhelgaas@google.com>,
        "lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
        "robh@kernel.org" <robh@kernel.org>
Subject: RE: [PATCH v8 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver
Thread-Topic: [PATCH v8 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver
Thread-Index: AQHWPZdq6W68nNHNZEqBlWufdyFfUqjPAbcAgASFrZCAABADgIABo7FA
Date:   Fri, 12 Jun 2020 17:16:01 +0000
Message-ID: <BYAPR02MB55594A0981D979CAEEA02CE3A5810@BYAPR02MB5559.namprd02.prod.outlook.com>
References: <1591622338-22652-1-git-send-email-bharat.kumar.gogada@xilinx.com>
 <1591622338-22652-3-git-send-email-bharat.kumar.gogada@xilinx.com>
 <c2e4b1288ce454c6ae2b2c02946d084f@kernel.org>
 <BYAPR02MB5559D2F57E35F8881F5B608CA5800@BYAPR02MB5559.namprd02.prod.outlook.com>
 <777c4abbbfcc99ddf968d2040bc86835@kernel.org>
In-Reply-To: <777c4abbbfcc99ddf968d2040bc86835@kernel.org>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 9526978c-7c75-4a78-53d5-08d80ef44884
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Jun 2020 17:16:02.2267
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: cNp/oXZWmPCWT9A9UdVmSL6yJLa6Lo69aYZlu3NouBPBD8F7lIhLR/QFVyCfDAiWiwA9xwqcFSrVGxKcKYAVUw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR02MB5127
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> Subject: Re: [PATCH v8 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port dri=
ver
>=20
> On 2020-06-11 16:51, Bharat Kumar Gogada wrote:
>=20
> [...]
>=20
> >> > +/**
> >> > + * xilinx_cpm_pcie_init_port - Initialize hardware
> >> > + * @port: PCIe port information
> >> > + */
> >> > +static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie_port
> >> > *port)
> >> > +{
> >> > +	if (cpm_pcie_link_up(port))
> >> > +		dev_info(port->dev, "PCIe Link is UP\n");
> >> > +	else
> >> > +		dev_info(port->dev, "PCIe Link is DOWN\n");
> >> > +
> >> > +	/* Disable all interrupts */
> >> > +	pcie_write(port, ~XILINX_CPM_PCIE_IDR_ALL_MASK,
> >> > +		   XILINX_CPM_PCIE_REG_IMR);
> >> > +
> >> > +	/* Clear pending interrupts */
> >> > +	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_IDR) &
> >> > +		   XILINX_CPM_PCIE_IMR_ALL_MASK,
> >> > +		   XILINX_CPM_PCIE_REG_IDR);
> >> > +
> >> > +	/* Enable all interrupts */
> >> > +	pcie_write(port, XILINX_CPM_PCIE_IMR_ALL_MASK,
> >> > +		   XILINX_CPM_PCIE_REG_IMR);
> >> > +	pcie_write(port, XILINX_CPM_PCIE_IDRN_MASK,
> >> > +		   XILINX_CPM_PCIE_REG_IDRN_MASK);
> >>
> >> No. I've explained in the previous review why this was a terrible
> >> thing to do, and my patch got rid of it for a good reason.
> >>
> >> If the mask/unmask calls do not work, please explain what is wrong,
> >> and let's fix them. But we DO NOT enable interrupts outside of an
> >> irqchip callback, full stop.
> > The issue here is, we do not have any other register to enable
> > interrupts for above events, in order to see an interrupt assert for
> > these events, the respective mask bits shall be set to 1.
>=20
> I still disagree, because you're not explaining anything.
>=20
> We enable the interrupts as they are requested already (that's why we wri=
te
> to the these register in the respective mask/unmask callbacks). Why do yo=
u
> need to enable them ahead of the request?
HI Marc,
Yes agreed, this is not needed.=20
In xilinx_cpm_unmask_event_irq {
...
val |=3D d->hwirq; 	//which needs to be BIT(d->hwirq)
...
}=20
Did not notice this earlier that the required bit is not being set here.
Will fix it next patch.

Regards,
Bharat
=20