On Sat, 2016-10-08 at 15:49 +0200, Lukas Wunner wrote:
> Hi Andy,
>
> thanks a lot for testing the patch!
>
> On Fri, Oct 07, 2016 at 11:55:34PM +0300, Andy Shevchenko wrote:
> >
> > On Thu, 2016-10-06 at 08:24 +0200, Lukas Wunner wrote:
> > >
> > > +pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev)
> > > +{
> > > + struct mid_pwr *pwr = midpwr;
> > > + int id, reg, bit;
> > > + u32 power;
> > > +
> > > + if (!pwr || !pwr->available)
> > > + return PCI_UNKNOWN;
> >
> > I'm not sure it's a right value. In arch/x86/pci/intel_mid_pci.c we
> > assign D3hot to PMCSR for all devices. I dunno how to proceed here,
> > though your case works for me.
>
> Generally returning PCI_UNKNOWN from the ->get_state hook is fine
> if the platform fails to determine the state. The ACPI equivalent
> acpi_pci_get_power_state() does this as well.
>
> >
> >
> > >
> > > +
> > > + id = intel_mid_pwr_get_lss_id(pdev);
> > > + if (id < 0)
> > > + return PCI_UNKNOWN;
> >
> > Similar here, not all PCI devices are using PWRMU (or P-Unit, which
> > support is absent) and might be AON, or be controllable via PMCSR
> > only.
>
> Hm, then mid_pci_power_manageable() is broken, you let it return true
> unconditionally.
Till now it works for me and others who tested my code / patches, I
would say "might be broken". I don't have [access to] all possible
documentation that shed a light on this, so, let's stick on your version
for now until some bug appears.
> I'm not familiar at all with Intel MID devices, do
> you have a way to clearly identify if a PCI device is power managed
> by the PWRMU versus PMCSR? My guess is that at the very least,
> intel_mid_pwr_get_lss_id() needs to be called and false needs to
> be returned by mid_pci_power_manageable() if the return value is
> negative.
PCI bus is kinda fake on those platforms (which implement SFI) and don't
always follow [PCI] specification. The vendor register represents so
called Logical Subsystem ID of the device in question. Some of them are
related to PWRMU, some to P-Unit, some just has no such register.
In PWRMU/P-Unit cases PMCSR is present and writing to it is needed.
For the rest I don't remember what is actual state, perhaps writing is
just ignored and OS reads D0 all the time from it.
And I don't remember if P-Unit manageable devices require anything else
than plain update of PMCSR.
> As said it's not a problem for the single existing caller of
> mid_pci_get_power_state(), which is pci_target_state(), since it
> only honors the return value if it's PCI_D3cold. Otherwise it
> defers to the PMCSR. The rationale is that reading the PMCSR is
> usually the most reliable way to determine the power state, but
> D3cold cannot be determined by reading the PMCSR.
Yep.
>
> What you say sounds to me like you need to amend the callbacks
> in mid_pci_platform_pm to differentiate between PWRMU-manageable
> versus non-PWRMU-manageable devices, but that's not specific to
> the single ->get_state callback added by this commit and can thus
> go into a separate commit.
See above.
So, from PWRMU code the function behaves correctly for now. Whenever P-
Unit support will be added the code will have to be modified
accordingly.
P.S. Btw, Asus Zenfone 2 and Lenovo K80m phones are based on Intel Moorefield which is, AFAIK, latest [widely distributed] successor of Intel MID devices. I dunno if anyone inside or outside Intel ever tried [fresh] upstream kernels there.
>
> >
> >
> > >
> > > +
> > > + reg = (id * LSS_PWS_BITS) / 32;
> > > + bit = (id * LSS_PWS_BITS) % 32;
> > > + power = mid_pwr_get_state(pwr, reg);
> > > + return (power >> bit) & 3;
> >
> > Don't add sparse warnings:
> >
> > return (__force pci_power_t)((power >> bit) & 3);
>
> Okay, I'll fix this in v2.
>
> Thanks again,
>
> Lukas
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
