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?
