Re: çå: çå: çå: [PATCH v6] mfd: Add support for RTS5250S power saving

[Bjorn Helgaas]

On Fri, Jan 19, 2018 at 07:38:22AM +0000, åé wrote:
> > On Wed, Dec 27, 2017 at 05:37:50PM -0600, Bjorn Helgaas wrote:
> > > On Tue, Dec 19, 2017 at 08:15:24AM +0000, åé wrote:
> > > > > On Fri, Dec 15, 2017 at 09:42:45AM +0000, åé wrote:
> > > > > > > [+cc Hans, Dave, linux-pci]
> > > > > > >
> > > > > > > On Thu, Sep 07, 2017 at 04:26:39PM +0800,
> > > > > > > rui_feng@xxxxxxxxxxxxxx
> > > > > wrote:
> > > > > > > > From: Rui Feng <rui_feng@xxxxxxxxxxxxxx>
> > > > > > >
> > > > > > > I wish this had been posted to linux-pci before being merged.
> > > > > > >
> > > > > > > I'm concerned because some of this appears to overlap and
> > > > > > > conflict with PCI core management of ASPM.
> > > > > > >
> > > > > > > I assume these devices advertise ASPM support in their Link
> > > > > > > Capabilites registers, right?  If so, why isn't the existing
> > > > > > > PCI core ASPM support sufficient?
> > > > > > >
> > > > > > When L1SS is configured, the device(hardware) can't enter L1SS
> > > > > > status automatically, it need driver(software) to do some work
> > > > > > to achieve the
> > > > > function.
> > > > >
> > > > > So this is a hardware defect in the device?  As far as I know,
> > > > > ASPM and L1SS are specified such that they should work without special
> > driver support.
> > > > >
> > > > Yes, you can say that.
> > > >
> > > > > > > > Enable power saving for RTS5250S as following steps:
> > > > > > > > 1.Set 0xFE58 to enable clock power management.
> > > > > > >
> > > > > > > Is this clock power management something specific to RTS5250S,
> > > > > > > or is it standard PCIe architected stuff?
> > > > > > >
> > > > > > 0xFE58 is specific register to RTS5250S not standard PCIe architected
> > stuff.
> > > > >
> > > > > OK.  I asked because devices often mirror architected PCIe config
> > > > > things in device-specific MMIO space, and if I squint just right,
> > > > > I can sort of match up the register bits you used with things in the PCIe
> > spec.
> > > > >
> > > > > > > > 2.Check cfg space whether support L1SS or not.
> > > > > > >
> > > > > > > This sounds like standard PCIe ASPM L1 Substates, right?
> > > > > > >
> > > > > > Yes.
> > > > > >
> > > > > > > > 3.If support L1SS, set 0xFF03 to free clkreq.
> > > > > > > > 4.When entering idle status, enable aspm
> > > > > > > >   and set parameters for L1SS and LTR.
> > > > > > > > 5.Wnen entering run status, disable aspm
> > > > > > > >   and set parameters for L1SS and LTR.
> > > > > > >
> > > > > > > In general, drivers should not configure ASPM, L1SS, and LTR
> > > > > > > themselves; the PCI core should do that.
> > > > > > >
> > > > > > > If a driver needs to tweak ASPM at run-time, it should use
> > > > > > > interfaces exported by the PCI core to do so.
> > > > > > >
> > > > > > Which interface I can use to set ASPM? I use "pci_write_config_byte"
> > now.
> > > > >
> > > > > What do you need to do?  include/linux/pci-aspm.h exports
> > > > > pci_disable_link_state(), which is mainly used to avoid ASPM
> > > > > states that have hardware errata.
> > > > >
> > > > I want to enable ASPM(L0 -> L1) and disable ASPM(L1 -> L0), which
> > > > interface can I use?
> > >
> > > You can use pci_disable_link_state() to disable usage of L1.
> > >
> > > Currently there is no corresponding pci_enable_link_state().  What if
> > > we added something like the following (untested)?  Would that work for
> > > you?
> > 
> > Hi Rui,
> > 
> > Any thoughts on the patch below?
> 
> I'm busy with other work, the patch seems ok, I will test it later.

Any idea when you might get back to this?

If you can't do it, I can try doing it myself, but of course, I don't
know the details about the device errata and I wouldn't be able to
test it.

Bjorn