On Tue, Aug 29, 2017 at 08:47:28AM +0800, Shawn Lin wrote: > Hi Bjorn, > > On 2017/8/29 2:33, Bjorn Helgaas wrote: > >On Mon, Aug 28, 2017 at 10:22:24AM +0800, Shawn Lin wrote: > >>Hi Bjorn, > >> > >>On 2017/8/26 5:38, Bjorn Helgaas wrote: > >>>On Wed, Aug 23, 2017 at 03:01:13PM +0800, Shawn Lin wrote: > >>>> > >> > >>... > >> > >>> > >>>I applied these to pci/host-rockchip except for these: > >>> > >>> PCI: rockchip: fix system hang up if activating CONFIG_DEBUG_SHIRQ > >>> PCI: rockchip: fix missing phy manipulation for legacy phy > >>> > >>>I'm not really happy with the devm/clk_prepare_enable situation, so > >>>I'm waiting to see if a better solution emerges. > >>> > >>>I went ahead and applied the tweaks I proposed to the earlier "PCI: > >>>rockchip: Add per-lane PHY support" and "PCI: rockchip: Idle inactive > >>>PHY(s)" patches. I *think* those make the "fix missing phy > >>>manipulation for legacy phy" patch unnecessary. > >>> > >>>But please take a look and make sure. If I went the wrong direction, > >>>I'll gladly back it out. > >> > >>I tested both of legacy and per-lane PHY mode, and it works fine. > >> > >>But just a nit: > >> > >>with legacy PHY, we could avoid to print the following message > >>which is confusing as for legacy PHY mode, we couldn't idle > >>any inactive lanes. > >> > >>[ 0.655192] rockchip-pcie f8000000.pcie: idling lane 1 > >>[ 0.655696] rockchip-pcie f8000000.pcie: idling lane 2 > >>[ 0.656194] rockchip-pcie f8000000.pcie: idling lane 3 > >> > >>So I think we could return 0xf for legacy PHY mode like this: > >> > >>static u8 rockchip_pcie_lane_map(struct rockchip_pcie *rockchip) > >> u8 map; > >> > >> if (rockchip->legacy_phy) > >>- return BIT(0); > >>+ return GENMASK(MAX_LANE_NUM - 1, 0); > > > >When we're using legacy PHY mode, this changes lanes_map from 0x1 to > >0xf. That means we won't print the "idling lane 1" message. It > >*also* means we won't call phy_power_off() for lanes 1-3. Is that the > >correct behavior? Does the legacy PHY mode actually use all 4 PHYs? > > Yes, it's the correct behaviour. The 4 PHYs is abstraction of 4 lanes > for per-lane PHY mode, but for legacy PHY mode, PHY represents a single > component managing 4 lanes together. So no matter how many lanes are > used, we couldn't call phy_power_off for legacy PHY mode until we really > want to power off the PHY. That is why we need to recontruct the pcie > and phy driver to migrate to per-lane PHY mode as we could treat each > lane as a phy object and let phy driver do idling work in the > phy_power_off callback instead of power-off work if ref count isn't > decreased to zero. > > Also I think the key point here is that for legacy PHY mode, only > phys[0] is vaild, so there is no difference if we call phy_power_off for > phys[1..3] as you did, because PHY API will check the phy object and > won't do anything if seeing invalid/NULL phy object. So changing > lanes_map from 0x1 to 0xf is only silent the "idling lane x" log. OK, I folded in the GENMASK update above.
