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;
Received-SPF: Pass (protection.outlook.com: domain of xilinx.com designates
 149.199.62.198 as permitted sender) receiver=protection.outlook.com;
 client-ip=149.199.62.198; helo=xsj-pvapexch02.xlnx.xilinx.com;
Envelope-to: git@xilinx.com,
 linux-pci@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org,
 robh@kernel.org,
 lorenzo.pieralisi@arm.com,
 bhelgaas@google.com,
 monstr@monstr.eu,
 linux-kernel@vger.kernel.org,
 kw@linux.com
Subject: Re: [PATCH] PCI: xilinx-nwl: Enable the clock through CCF
To:     =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?= <kw@linux.com>,
        Michal Simek <michal.simek@xilinx.com>
CC:     <linux-kernel@vger.kernel.org>, <monstr@monstr.eu>,
        <git@xilinx.com>, "Hyun Kwon" <hyun.kwon@xilinx.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
        Rob Herring <robh@kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <linux-pci@vger.kernel.org>
References: <dbc0ab2e109111ca814e73abb30a1dda5d333dbe.1624449519.git.michal.simek@xilinx.com>
 <20210623122040.GA46059@rocinante>
From:   Michal Simek <michal.simek@xilinx.com>
Message-ID: <d7dd7dfe-2c48-c155-793d-1431256a070e@xilinx.com>
Date:   Wed, 23 Jun 2021 15:08:05 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210623122040.GA46059@rocinante>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jun 2021 13:08:36.0687
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: b0ac16ce-063d-4e3e-3eee-08d9364802c4
X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.62.198];Helo=[xsj-pvapexch02.xlnx.xilinx.com]
X-MS-Exchange-CrossTenant-AuthSource: DM3NAM02FT034.eop-nam02.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR02MB5437
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Krzysztof,

On 6/23/21 2:20 PM, Krzysztof Wilczyński wrote:
> Hi Michal,
> 
> Thank you for sending the patch over!

Thanks for review.

> 
>> Simply enable clocks. There is no remove function that's why
>> this should be enough for simple operation.
> 
> What clock is this?  Would it be worth mentioning what it is for
> a reference (and for posterity) the commit message?

It is reference clock coming to the IP. I will update commit message.


> 
> Also why it would need to be enabled and wasn't before?  Would this be
> a fix for some problem?  Would this warrant a "Fixes:" tag?  And would
> it need to be back-ported to stable kernels?

I will update commit message. Normally reference clock is enabled by
firmware but on some configurations this doesn't need to be truth that's
why it is necessary to enable it. It also records refcount for this
reference clock is good.

I will add Fixes tag to v2.

> 
> [...]
>> @@ -823,6 +825,11 @@ static int nwl_pcie_probe(struct platform_device *pdev)
>>  		return err;
>>  	}
>>  
>> +	pcie->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(pcie->clk))
>> +		return PTR_ERR(pcie->clk);
>> +	clk_prepare_enable(pcie->clk);
>> +
> [...]
> 
> Almost every other user of clk_prepare_enable() would check for
> potential failure, print an appropriate message, and then do the
> necessary clean-up before bailing out and returning an error.
> 
> Would adding an error check for clk_prepare_enable() and printing an
> error message using dev_err() be too much in this case?  If not, then
> I would rather follow the pattern that other users established and
> handle errors as needed.  What do you think?

Agree. I have added it. It is called very early and devm_ functions are
used that's why cleanup shouldn't be necessary.

I have also found that clock wasn't documented in dt binding for this IP
but we are setting it up for quite a long time.
9c8a47b484ed ("arm64: dts: xilinx: Add the clock nodes for zynqmp")

Thanks,
Michal