LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
To: Vinod Koul <vkoul@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	linuxarm@huawei.com, mauro.chehab@huawei.com,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Kishon Vijay Abraham I <kishon@ti.com>,
	Manivannan Sadhasivam <mani@kernel.org>,
	Rob Herring <robh@kernel.org>,
	linux-kernel@vger.kernel.org, linux-phy@lists.infradead.org
Subject: Re: [PATCH v11 01/11] phy: HiSilicon: Add driver for Kirin 970 PCIe PHY
Date: Wed, 18 Aug 2021 11:01:23 +0200	[thread overview]
Message-ID: <20210818110123.33eba838@coco.lan> (raw)
In-Reply-To: <YRuSnXHSZHhBC40r@matsya>

Hi Vinod,

Em Tue, 17 Aug 2021 16:12:37 +0530
Vinod Koul <vkoul@kernel.org> escreveu:

> > +	/* FIXME: calling it causes an Asynchronous SError interrupt */
> > +//	kirin_pcie_clk_ctrl(phy, false);  
> 
> when will you fix the fixme and pls remove the deadcode

Working with clocks on this SoC is very tricky: there are lots of clock
lines (~70) that are critical for this device to work. Such lines are
enabled via the Device's firmware, and are supposed to be always
powered. Powering off such clock lines cause a SError.

Most clocks on this device are managed by the clk-hikey3670 driver.
At the current state of clk-hi3670, the only way for HiKey 970
to even boot is to add:

	clk_ignore_unused=true

as a Kernel boot parameter. That is the solution given by the downstream
official distributions for HiKey970 at 96boards.

The fix is to flag the critical clocks with CLK_IS_CRITICAL at the
clk-hi3670 driver, but finding the right clock set has been a challenge.

I spent the last couple of weeks trying to identify the critical ones,
as I'm aiming to be able to use a Kernel built with a default arm64
one of my goals is to have this device working fine with a
"make defconfig" Kernel.

So, I added this patch:

	https://lore.kernel.org/lkml/2d2de5e902ced072bcfd5e5311d6b10326b9245b.1627041240.git.mchehab+huawei@kernel.org/

to my tree (which reduces the set of clocks using CLK_IGNORE_UNUSED
from 308 to 163 clocks). Than I ran script that was dropping the
flag one by one, boots the new Kernel and do a sanity check. When it 
fails to boot, I manually dropped the patch, and re-run the script
to test the remaining clocks. After a couple of weeks, I reached a patch
with 78 clock lines that seemed critical, but the resulting patch was
not stable, as, depending on the day I boot the Kernel with such patch,
it crashes with SError in a couple of seconds after booting, or 
cause the Ethernet firmware to not load.

I intend to keep trying to find the clock lines that can't be disabled,
but this is very time consuming, as I couldn't find any documentation
about that. So, it has to be done empirically.

-

In any case, fixing it doesn't sound a critical issue for the PHY
driver. I mean, right now, this patchset allows removing and 
re-inseting the PCIe driver, which is already an improvement over the
original upstream driver, which was missing the power-off logic for
Kirin 960.

With this patchset, both power-off/power-on logic for both HiKey960
(where the PHY is inside the pcie-kirin driver) and for HiKey970,
which uses this PHY driver. On both devices, I tested an endless loop 
with rmmod/modprobe for the PCIe.

Besides that, in practice, removing PCIe in runtime is something that
people usually don't do.

So, while it would be cool to balance the clock disable logic,
I don't think this is a critical issue in this particular case.

Thanks,
Mauro

  reply	other threads:[~2021-08-18  9:01 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-12  8:02 [PATCH v11 00/11] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
2021-08-12  8:02 ` [PATCH v11 01/11] phy: HiSilicon: Add driver for Kirin 970 PCIe PHY Mauro Carvalho Chehab
2021-08-17 10:42   ` Vinod Koul
2021-08-18  9:01     ` Mauro Carvalho Chehab [this message]
2021-08-18 10:08       ` Mauro Carvalho Chehab
2021-08-18 10:10       ` Vinod Koul
2021-08-18 10:30         ` Mauro Carvalho Chehab
2021-08-18 10:37           ` Vinod Koul
2021-08-20 13:43             ` [PATCH v13] " Mauro Carvalho Chehab
2021-08-20 23:04               ` kernel test robot
2021-08-18 11:04         ` [PATCH v12] " Mauro Carvalho Chehab
2021-08-18 13:47           ` kernel test robot
2021-09-15 13:11           ` Mauro Carvalho Chehab
2021-08-12  8:02 ` [PATCH v11 02/11] PCI: kirin: Reorganize the PHY logic inside the driver Mauro Carvalho Chehab
2021-08-12  8:02 ` [PATCH v11 03/11] PCI: kirin: Add support for a PHY layer Mauro Carvalho Chehab
2021-08-12  8:02 ` [PATCH v11 04/11] PCI: kirin: Use regmap for APB registers Mauro Carvalho Chehab
2021-08-12  8:02 ` [PATCH v11 05/11] PCI: kirin: Add support for bridge slot DT schema Mauro Carvalho Chehab
2021-08-12  8:02 ` [PATCH v11 06/11] PCI: kirin: Add Kirin 970 compatible Mauro Carvalho Chehab
2021-08-12  8:02 ` [PATCH v11 07/11] PCI: kirin: Add MODULE_* macros Mauro Carvalho Chehab
2021-08-12  8:02 ` [PATCH v11 08/11] PCI: kirin: Allow building it as a module Mauro Carvalho Chehab
2021-08-12  8:02 ` [PATCH v11 09/11] PCI: kirin: Add power_off support for Kirin 960 PHY Mauro Carvalho Chehab
2021-08-12  8:02 ` [PATCH v11 10/11] PCI: kirin: fix poweroff sequence Mauro Carvalho Chehab
2021-08-12  8:02 ` [PATCH v11 11/11] PCI: kirin: Allow removing the driver Mauro Carvalho Chehab

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210818110123.33eba838@coco.lan \
    --to=mchehab+huawei@kernel.org \
    --cc=bhelgaas@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kishon@ti.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-phy@lists.infradead.org \
    --cc=linuxarm@huawei.com \
    --cc=mani@kernel.org \
    --cc=mauro.chehab@huawei.com \
    --cc=robh@kernel.org \
    --cc=vkoul@kernel.org \
    --subject='Re: [PATCH v11 01/11] phy: HiSilicon: Add driver for Kirin 970 PCIe PHY' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).