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;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Thu, 11 Jun 2020 16:59:31 +0100
From:   Marc Zyngier <maz@kernel.org>
To:     Bharat Kumar Gogada <bharatku@xilinx.com>
Cc:     linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        bhelgaas@google.com, lorenzo.pieralisi@arm.com, robh@kernel.org
Subject: Re: [PATCH v8 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver
In-Reply-To: <BYAPR02MB5559D2F57E35F8881F5B608CA5800@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>
User-Agent: Roundcube Webmail/1.4.4
Message-ID: <777c4abbbfcc99ddf968d2040bc86835@kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-06-11 16:51, Bharat Kumar Gogada wrote:

[...]

>> > +/**
>> > + * 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.

I still disagree, because you're not explaining anything.

We enable the interrupts as they are requested already (that's why we 
write
to the these register in the respective mask/unmask callbacks). Why do 
you
need to enable them ahead of the request?

         M.
-- 
Jazz is not dead. It just smells funny...