| Message ID | 20170925192109.rty2fnm7c4jnj3vx@sig21.net (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Monday, September 25, 2017 9:21:09 PM CET Johannes Stezenbach wrote: > SATA controller is enabled on Asus E200HA even though the > machine doesn't use it (it has eMMC storage), however > SATA being on blocks S0ix entry so we need to disable it. > > Signed-off-by: Johannes Stezenbach <js@sig21.net> Mika, Andy, Hans, any comments on this one? > --- > drivers/platform/x86/pmc_atom.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c > index 8d13c86cc418..b5dd38712268 100644 > --- a/drivers/platform/x86/pmc_atom.c > +++ b/drivers/platform/x86/pmc_atom.c > @@ -17,6 +17,7 @@ > > #include <linux/debugfs.h> > #include <linux/device.h> > +#include <linux/dmi.h> > #include <linux/init.h> > #include <linux/io.h> > #include <linux/platform_data/x86/clk-pmc-atom.h> > @@ -57,6 +58,9 @@ struct pmc_dev { > static struct pmc_dev pmc_device; > static u32 acpi_base_addr; > > +static u32 quirks; > +#define QUIRK_DISABLE_SATA BIT(0) > + > static const struct pmc_clk byt_clks[] = { > { > .name = "xtal", > @@ -271,6 +275,15 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc) > * - GPIO_SCORE shared IRQ > */ > pmc_reg_write(pmc, PMC_S0IX_WAKE_EN, (u32)PMC_WAKE_EN_SETTING); > + > + if (quirks & QUIRK_DISABLE_SATA) { > + u32 func_dis; > + > + pr_info("pmc: disable SATA IP\n"); > + func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS); > + func_dis |= BIT_SATA; > + pmc_reg_write(pmc, PMC_FUNC_DIS, func_dis); > + } > } > > #ifdef CONFIG_DEBUG_FS > @@ -500,6 +513,24 @@ static struct pmc_notifier_block pmc_freeze_nb = { > }; > #endif > > +static int cht_asus_e200ha_cb(const struct dmi_system_id *id) > +{ > + pr_info("pmc: Asus E200HA detected\n"); > + quirks |= QUIRK_DISABLE_SATA; > + return 1; > +} > + > +static const struct dmi_system_id cht_table[] = { > + { > + .callback = cht_asus_e200ha_cb, > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."), > + DMI_MATCH(DMI_PRODUCT_NAME, "E200HA"), > + }, > + }, > + { } > +}; > + > static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) > { > struct pmc_dev *pmc = &pmc_device; > @@ -526,6 +557,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) > > pmc->map = map; > > + dmi_check_system(cht_table); > + > /* PMC hardware registers setup */ > pmc_hw_reg_setup(pmc); > >
Hi, On 13-12-17 01:00, Rafael J. Wysocki wrote: > On Monday, September 25, 2017 9:21:09 PM CET Johannes Stezenbach wrote: >> SATA controller is enabled on Asus E200HA even though the >> machine doesn't use it (it has eMMC storage), however >> SATA being on blocks S0ix entry so we need to disable it. >> >> Signed-off-by: Johannes Stezenbach <js@sig21.net> > > Mika, Andy, Hans, any comments on this one? Seems sensible to me, I'm afraid we may need the same quirk on other devices, but I see no way around this. Although, maybe we need to have a specialized (derived) ahci driver for these Atom SoCs and in there if no disk is detected do this through the clock framework? That may be better then a long list of quirks. Johannes, question how did you test this and figure out which clocks to disable, a quick howto on this, I think a patch adding a little howto / README as say Documentation/power/intel-S0ix-debugging.txt documenting this would be great. I'm certainly interested in trying to reproduce this on some of my own Bay Trail and Cherry Trail devices and add fixes for those if necessary. Regards, Hans > >> --- >> drivers/platform/x86/pmc_atom.c | 33 +++++++++++++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c >> index 8d13c86cc418..b5dd38712268 100644 >> --- a/drivers/platform/x86/pmc_atom.c >> +++ b/drivers/platform/x86/pmc_atom.c >> @@ -17,6 +17,7 @@ >> >> #include <linux/debugfs.h> >> #include <linux/device.h> >> +#include <linux/dmi.h> >> #include <linux/init.h> >> #include <linux/io.h> >> #include <linux/platform_data/x86/clk-pmc-atom.h> >> @@ -57,6 +58,9 @@ struct pmc_dev { >> static struct pmc_dev pmc_device; >> static u32 acpi_base_addr; >> >> +static u32 quirks; >> +#define QUIRK_DISABLE_SATA BIT(0) >> + >> static const struct pmc_clk byt_clks[] = { >> { >> .name = "xtal", >> @@ -271,6 +275,15 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc) >> * - GPIO_SCORE shared IRQ >> */ >> pmc_reg_write(pmc, PMC_S0IX_WAKE_EN, (u32)PMC_WAKE_EN_SETTING); >> + >> + if (quirks & QUIRK_DISABLE_SATA) { >> + u32 func_dis; >> + >> + pr_info("pmc: disable SATA IP\n"); >> + func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS); >> + func_dis |= BIT_SATA; >> + pmc_reg_write(pmc, PMC_FUNC_DIS, func_dis); >> + } >> } >> >> #ifdef CONFIG_DEBUG_FS >> @@ -500,6 +513,24 @@ static struct pmc_notifier_block pmc_freeze_nb = { >> }; >> #endif >> >> +static int cht_asus_e200ha_cb(const struct dmi_system_id *id) >> +{ >> + pr_info("pmc: Asus E200HA detected\n"); >> + quirks |= QUIRK_DISABLE_SATA; >> + return 1; >> +} >> + >> +static const struct dmi_system_id cht_table[] = { >> + { >> + .callback = cht_asus_e200ha_cb, >> + .matches = { >> + DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."), >> + DMI_MATCH(DMI_PRODUCT_NAME, "E200HA"), >> + }, >> + }, >> + { } >> +}; >> + >> static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) >> { >> struct pmc_dev *pmc = &pmc_device; >> @@ -526,6 +557,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) >> >> pmc->map = map; >> >> + dmi_check_system(cht_table); >> + >> /* PMC hardware registers setup */ >> pmc_hw_reg_setup(pmc); >> >> > >
On Wed, Dec 13, 2017 at 09:53:21AM +0100, Hans de Goede wrote: > On 13-12-17 01:00, Rafael J. Wysocki wrote: > > On Monday, September 25, 2017 9:21:09 PM CET Johannes Stezenbach wrote: > > > SATA controller is enabled on Asus E200HA even though the > > > machine doesn't use it (it has eMMC storage), however > > > SATA being on blocks S0ix entry so we need to disable it. > > > > > > Signed-off-by: Johannes Stezenbach <js@sig21.net> > > > > Mika, Andy, Hans, any comments on this one? > > Seems sensible to me, I'm afraid we may need the same quirk on > other devices, but I see no way around this. > > Although, maybe we need to have a specialized (derived) > ahci driver for these Atom SoCs and in there if no > disk is detected do this through the clock framework? > > That may be better then a long list of quirks. > > Johannes, question how did you test this and figure out > which clocks to disable, a quick howto on this, I think > a patch adding a little howto / README as say > Documentation/power/intel-S0ix-debugging.txt > documenting this would be great. I'm certainly interested > in trying to reproduce this on some of my own Bay Trail and > Cherry Trail devices and add fixes for those if necessary. I put my E200HA aside due to lack of time, so it's unlikely I'll send documentation patches anytime soon. Basically everything is documented in bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=193891 For the SATA issue I just poked wildly around in registers using busybox devmem, after applying S0ix blocker debug patch I tried to disable some devices which were printed: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c53 (Obviously bug 193891 is misnamed, most of what's discussed there doesn't directly relate to PMIC. Sorry for creating a mess, but my understanding of the platform was very low when I created it.) The thing is that the public CHT datasheet (atom-z8000-datasheet-vol-1.pdf + vol-2) doesn't even mention SATA, and there is no PCI device for it. OTOH, baytrail datasheet (atom-e3800-family-datasheet.pdf) specifies SATA and BIT_SATA was already defined in pmc_atom.h. Besides SATA, I also needed to disable dw DMA, using a hack patch or devmem, but eventually it might be solved properly: https://bugzilla.kernel.org/show_bug.cgi?id=196861 Thanks, Johannes
Hell Hans, all, On Wed, Dec 13, 2017 at 12:53 AM, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 13-12-17 01:00, Rafael J. Wysocki wrote: >> >> On Monday, September 25, 2017 9:21:09 PM CET Johannes Stezenbach wrote: >>> >>> SATA controller is enabled on Asus E200HA even though the >>> machine doesn't use it (it has eMMC storage), however >>> SATA being on blocks S0ix entry so we need to disable it. >>> >>> Signed-off-by: Johannes Stezenbach <js@sig21.net> >> >> >> Mika, Andy, Hans, any comments on this one? > > > Seems sensible to me, I'm afraid we may need the same quirk on > other devices, but I see no way around this. Quirks go directly into the driver? Is there a Device Tree analogue for x86 that could help here? > > Although, maybe we need to have a specialized (derived) > ahci driver for these Atom SoCs and in there if no > disk is detected do this through the clock framework? Yes please. x86 is already modeling some clocks properly through the clock framework. During late init we clean up any clocks that were enabled out of reset or by the firmware/bootloader but not claimed and enabled by any Linux driver. That should ideally disable this particular clock for the case when no SATA drive is present, and require no quirk logic in the driver. Regards, Mike > > That may be better then a long list of quirks. > > Johannes, question how did you test this and figure out > which clocks to disable, a quick howto on this, I think > a patch adding a little howto / README as say > Documentation/power/intel-S0ix-debugging.txt > documenting this would be great. I'm certainly interested > in trying to reproduce this on some of my own Bay Trail and > Cherry Trail devices and add fixes for those if necessary. > > Regards, > > Hans > > > > > >> >>> --- >>> drivers/platform/x86/pmc_atom.c | 33 +++++++++++++++++++++++++++++++++ >>> 1 file changed, 33 insertions(+) >>> >>> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c >>> index 8d13c86cc418..b5dd38712268 100644 >>> --- a/drivers/platform/x86/pmc_atom.c >>> +++ b/drivers/platform/x86/pmc_atom.c >>> @@ -17,6 +17,7 @@ >>> #include <linux/debugfs.h> >>> #include <linux/device.h> >>> +#include <linux/dmi.h> >>> #include <linux/init.h> >>> #include <linux/io.h> >>> #include <linux/platform_data/x86/clk-pmc-atom.h> >>> @@ -57,6 +58,9 @@ struct pmc_dev { >>> static struct pmc_dev pmc_device; >>> static u32 acpi_base_addr; >>> +static u32 quirks; >>> +#define QUIRK_DISABLE_SATA BIT(0) >>> + >>> static const struct pmc_clk byt_clks[] = { >>> { >>> .name = "xtal", >>> @@ -271,6 +275,15 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc) >>> * - GPIO_SCORE shared IRQ >>> */ >>> pmc_reg_write(pmc, PMC_S0IX_WAKE_EN, (u32)PMC_WAKE_EN_SETTING); >>> + >>> + if (quirks & QUIRK_DISABLE_SATA) { >>> + u32 func_dis; >>> + >>> + pr_info("pmc: disable SATA IP\n"); >>> + func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS); >>> + func_dis |= BIT_SATA; >>> + pmc_reg_write(pmc, PMC_FUNC_DIS, func_dis); >>> + } >>> } >>> #ifdef CONFIG_DEBUG_FS >>> @@ -500,6 +513,24 @@ static struct pmc_notifier_block pmc_freeze_nb = { >>> }; >>> #endif >>> +static int cht_asus_e200ha_cb(const struct dmi_system_id *id) >>> +{ >>> + pr_info("pmc: Asus E200HA detected\n"); >>> + quirks |= QUIRK_DISABLE_SATA; >>> + return 1; >>> +} >>> + >>> +static const struct dmi_system_id cht_table[] = { >>> + { >>> + .callback = cht_asus_e200ha_cb, >>> + .matches = { >>> + DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."), >>> + DMI_MATCH(DMI_PRODUCT_NAME, "E200HA"), >>> + }, >>> + }, >>> + { } >>> +}; >>> + >>> static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) >>> { >>> struct pmc_dev *pmc = &pmc_device; >>> @@ -526,6 +557,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) >>> pmc->map = map; >>> + dmi_check_system(cht_table); >>> + >>> /* PMC hardware registers setup */ >>> pmc_hw_reg_setup(pmc); >>> >> >> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-clk" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 13-12-17 16:25, Michael Turquette wrote: > Hell Hans, all, > > On Wed, Dec 13, 2017 at 12:53 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 13-12-17 01:00, Rafael J. Wysocki wrote: >>> >>> On Monday, September 25, 2017 9:21:09 PM CET Johannes Stezenbach wrote: >>>> >>>> SATA controller is enabled on Asus E200HA even though the >>>> machine doesn't use it (it has eMMC storage), however >>>> SATA being on blocks S0ix entry so we need to disable it. >>>> >>>> Signed-off-by: Johannes Stezenbach <js@sig21.net> >>> >>> >>> Mika, Andy, Hans, any comments on this one? >> >> >> Seems sensible to me, I'm afraid we may need the same quirk on >> other devices, but I see no way around this. > > Quirks go directly into the driver? Is there a Device Tree analogue > for x86 that could help here? No. >> Although, maybe we need to have a specialized (derived) >> ahci driver for these Atom SoCs and in there if no >> disk is detected do this through the clock framework? > > Yes please. x86 is already modeling some clocks properly through the > clock framework. During late init we clean up any clocks that were > enabled out of reset or by the firmware/bootloader but not claimed and > enabled by any Linux driver. That should ideally disable this > particular clock for the case when no SATA drive is present, and > require no quirk logic in the driver. Ah so you're thinking a special ahci driver which knows about the clock, yes I think that could work. Or maybe do a match on the CPU model and if it is know to not have SATA (or not routed to the outside), disable the clock? That seems better because if I understood Johannes correctly there is no SATA/AHCI PCI device (so nothing for a driver to bind to) but the clock is still enabled, although in that case the clock framework should do the right thing if we revert commit d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware" Regards, Hans > > Regards, > Mike > >> >> That may be better then a long list of quirks. >> >> Johannes, question how did you test this and figure out >> which clocks to disable, a quick howto on this, I think >> a patch adding a little howto / README as say >> Documentation/power/intel-S0ix-debugging.txt >> documenting this would be great. I'm certainly interested >> in trying to reproduce this on some of my own Bay Trail and >> Cherry Trail devices and add fixes for those if necessary. >> >> Regards, >> >> Hans >> >> >> >> >> >>> >>>> --- >>>> drivers/platform/x86/pmc_atom.c | 33 +++++++++++++++++++++++++++++++++ >>>> 1 file changed, 33 insertions(+) >>>> >>>> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c >>>> index 8d13c86cc418..b5dd38712268 100644 >>>> --- a/drivers/platform/x86/pmc_atom.c >>>> +++ b/drivers/platform/x86/pmc_atom.c >>>> @@ -17,6 +17,7 @@ >>>> #include <linux/debugfs.h> >>>> #include <linux/device.h> >>>> +#include <linux/dmi.h> >>>> #include <linux/init.h> >>>> #include <linux/io.h> >>>> #include <linux/platform_data/x86/clk-pmc-atom.h> >>>> @@ -57,6 +58,9 @@ struct pmc_dev { >>>> static struct pmc_dev pmc_device; >>>> static u32 acpi_base_addr; >>>> +static u32 quirks; >>>> +#define QUIRK_DISABLE_SATA BIT(0) >>>> + >>>> static const struct pmc_clk byt_clks[] = { >>>> { >>>> .name = "xtal", >>>> @@ -271,6 +275,15 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc) >>>> * - GPIO_SCORE shared IRQ >>>> */ >>>> pmc_reg_write(pmc, PMC_S0IX_WAKE_EN, (u32)PMC_WAKE_EN_SETTING); >>>> + >>>> + if (quirks & QUIRK_DISABLE_SATA) { >>>> + u32 func_dis; >>>> + >>>> + pr_info("pmc: disable SATA IP\n"); >>>> + func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS); >>>> + func_dis |= BIT_SATA; >>>> + pmc_reg_write(pmc, PMC_FUNC_DIS, func_dis); >>>> + } >>>> } >>>> #ifdef CONFIG_DEBUG_FS >>>> @@ -500,6 +513,24 @@ static struct pmc_notifier_block pmc_freeze_nb = { >>>> }; >>>> #endif >>>> +static int cht_asus_e200ha_cb(const struct dmi_system_id *id) >>>> +{ >>>> + pr_info("pmc: Asus E200HA detected\n"); >>>> + quirks |= QUIRK_DISABLE_SATA; >>>> + return 1; >>>> +} >>>> + >>>> +static const struct dmi_system_id cht_table[] = { >>>> + { >>>> + .callback = cht_asus_e200ha_cb, >>>> + .matches = { >>>> + DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."), >>>> + DMI_MATCH(DMI_PRODUCT_NAME, "E200HA"), >>>> + }, >>>> + }, >>>> + { } >>>> +}; >>>> + >>>> static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) >>>> { >>>> struct pmc_dev *pmc = &pmc_device; >>>> @@ -526,6 +557,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) >>>> pmc->map = map; >>>> + dmi_check_system(cht_table); >>>> + >>>> /* PMC hardware registers setup */ >>>> pmc_hw_reg_setup(pmc); >>>> >>> >>> >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-clk" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Dec 13, 2017 at 05:04:34PM +0100, Hans de Goede wrote: > On 13-12-17 16:25, Michael Turquette wrote: > > On Wed, Dec 13, 2017 at 12:53 AM, Hans de Goede <hdegoede@redhat.com> wrote: > > > Although, maybe we need to have a specialized (derived) > > > ahci driver for these Atom SoCs and in there if no > > > disk is detected do this through the clock framework? > > > > Yes please. x86 is already modeling some clocks properly through the > > clock framework. During late init we clean up any clocks that were > > enabled out of reset or by the firmware/bootloader but not claimed and > > enabled by any Linux driver. That should ideally disable this > > particular clock for the case when no SATA drive is present, and > > require no quirk logic in the driver. > > Ah so you're thinking a special ahci driver which knows about > the clock, yes I think that could work. > > Or maybe do a match on the CPU model and if it is know to > not have SATA (or not routed to the outside), disable > the clock? That seems better because if I understood Johannes > correctly there is no SATA/AHCI PCI device (so nothing for > a driver to bind to) but the clock is still enabled, although > in that case the clock framework should do the right thing > if we revert commit d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware" Please don't get confused with the other thread about clocks. This issue sets the "disable IP" bit, found by doing stupid experiments to enable S0ix on E200HA. 1. no idea if Cherry Trail even has SATA IP, maybe this is a meaningless bit but PMC firmware carried over from Bay Trail looks at it 2. BIOS should have set the bit, so it is a BIOS quirk 3. or maybe there is a much better solution that I don't know about https://bugzilla.kernel.org/show_bug.cgi?id=193891 also has lspci output Thanks, Johannes
Hi, On 13-12-17 17:22, Johannes Stezenbach wrote: > Hi, > > On Wed, Dec 13, 2017 at 05:04:34PM +0100, Hans de Goede wrote: >> On 13-12-17 16:25, Michael Turquette wrote: >>> On Wed, Dec 13, 2017 at 12:53 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> Although, maybe we need to have a specialized (derived) >>>> ahci driver for these Atom SoCs and in there if no >>>> disk is detected do this through the clock framework? >>> >>> Yes please. x86 is already modeling some clocks properly through the >>> clock framework. During late init we clean up any clocks that were >>> enabled out of reset or by the firmware/bootloader but not claimed and >>> enabled by any Linux driver. That should ideally disable this >>> particular clock for the case when no SATA drive is present, and >>> require no quirk logic in the driver. >> >> Ah so you're thinking a special ahci driver which knows about >> the clock, yes I think that could work. >> >> Or maybe do a match on the CPU model and if it is know to >> not have SATA (or not routed to the outside), disable >> the clock? That seems better because if I understood Johannes >> correctly there is no SATA/AHCI PCI device (so nothing for >> a driver to bind to) but the clock is still enabled, although >> in that case the clock framework should do the right thing >> if we revert commit d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware" > > Please don't get confused with the other thread about clocks. > This issue sets the "disable IP" bit, found by doing stupid > experiments to enable S0ix on E200HA. Ah my bad. > 1. no idea if Cherry Trail even has SATA IP, maybe this is a > meaningless bit but PMC firmware carried over from > Bay Trail looks at it There are no CHT SoCs with SATA AFAIK, but Braswell SoCs, which I believe is the same die do have SATA. I think the best fix here is to look at the model-string part of the CPU-id and do a quirk based on that, setting the "disable IP" bit for the SATA on all SoC models known to not have SATA (Z8300, Z8350, Z8500, Z8550, Z8700, Z8750). Rafael, Andy how does that sound as a solution? > 2. BIOS should have set the bit, so it is a BIOS quirk > > 3. or maybe there is a much better solution that I don't know about > > https://bugzilla.kernel.org/show_bug.cgi?id=193891 > also has lspci output Regards, Hans
On Wed, 2017-12-13 at 17:37 +0100, Hans de Goede wrote: > Hi, > > On 13-12-17 17:22, Johannes Stezenbach wrote: > > > > Please don't get confused with the other thread about clocks. > > This issue sets the "disable IP" bit, found by doing stupid > > experiments to enable S0ix on E200HA. > > Ah my bad. Oh, perhaps I also need to refresh my memory from that buglink. > > 1. no idea if Cherry Trail even has SATA IP, maybe this is a > > meaningless bit but PMC firmware carried over from > > Bay Trail looks at it > > > There are no CHT SoCs with SATA AFAIK, but Braswell SoCs, > which I believe is the same die do have SATA. > > I think the best fix here is to look at the model-string part > of the CPU-id and do a quirk based on that, setting the "disable IP" > bit for the SATA on all SoC models known to not have SATA > (Z8300, Z8350, Z8500, Z8550, Z8700, Z8750). > > Rafael, Andy how does that sound as a solution? Yeah, that bit is a property of PMC microcontroller and thus belongs to its driver in Linux kernel. To make it strict we need a matching property. AFAIR CPU model ID is all the same for all CHT and BSW SoCs, so, can't be used to distinguish them. So, you are thinking about comparing CPU model name then? It might work. However, SATA itself is a part of PCH, and thus can not exactly be matched by CPU ID. Btw, Pentium Celeron N-series according to spec has SATA host.
Hi, On 13-12-17 20:33, Andy Shevchenko wrote: > On Wed, 2017-12-13 at 17:37 +0100, Hans de Goede wrote: >> Hi, >> >> On 13-12-17 17:22, Johannes Stezenbach wrote: >>> > > >>> Please don't get confused with the other thread about clocks. >>> This issue sets the "disable IP" bit, found by doing stupid >>> experiments to enable S0ix on E200HA. >> >> Ah my bad. > > Oh, perhaps I also need to refresh my memory from that buglink. > >>> 1. no idea if Cherry Trail even has SATA IP, maybe this is a >>> meaningless bit but PMC firmware carried over from >>> Bay Trail looks at it >> >> >> There are no CHT SoCs with SATA AFAIK, but Braswell SoCs, >> which I believe is the same die do have SATA. >> >> I think the best fix here is to look at the model-string part >> of the CPU-id and do a quirk based on that, setting the "disable IP" >> bit for the SATA on all SoC models known to not have SATA >> (Z8300, Z8350, Z8500, Z8550, Z8700, Z8750). >> >> Rafael, Andy how does that sound as a solution? > > Yeah, that bit is a property of PMC microcontroller and thus belongs to > its driver in Linux kernel. > > To make it strict we need a matching property. AFAIR CPU model ID is all > the same for all CHT and BSW SoCs, so, can't be used to distinguish > them. So, you are thinking about comparing CPU model name then? Yes CPU model name. > It might work. However, SATA itself is a part of PCH, and thus can not > exactly be matched by CPU ID. AFAIK the PCH is integrated, at least with the Z83xx Z85xx and X87xx models. So using a CPU model name match seems a better solution to me then DMI product-name matching, as the CPU model name match should fix this for all devices with such a SoC. > Btw, Pentium Celeron N-series according to spec has SATA host. Right as I said the Braswell models do have SATA. Regards, Hans
diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c index 8d13c86cc418..b5dd38712268 100644 --- a/drivers/platform/x86/pmc_atom.c +++ b/drivers/platform/x86/pmc_atom.c @@ -17,6 +17,7 @@ #include <linux/debugfs.h> #include <linux/device.h> +#include <linux/dmi.h> #include <linux/init.h> #include <linux/io.h> #include <linux/platform_data/x86/clk-pmc-atom.h> @@ -57,6 +58,9 @@ struct pmc_dev { static struct pmc_dev pmc_device; static u32 acpi_base_addr; +static u32 quirks; +#define QUIRK_DISABLE_SATA BIT(0) + static const struct pmc_clk byt_clks[] = { { .name = "xtal", @@ -271,6 +275,15 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc) * - GPIO_SCORE shared IRQ */ pmc_reg_write(pmc, PMC_S0IX_WAKE_EN, (u32)PMC_WAKE_EN_SETTING); + + if (quirks & QUIRK_DISABLE_SATA) { + u32 func_dis; + + pr_info("pmc: disable SATA IP\n"); + func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS); + func_dis |= BIT_SATA; + pmc_reg_write(pmc, PMC_FUNC_DIS, func_dis); + } } #ifdef CONFIG_DEBUG_FS @@ -500,6 +513,24 @@ static struct pmc_notifier_block pmc_freeze_nb = { }; #endif +static int cht_asus_e200ha_cb(const struct dmi_system_id *id) +{ + pr_info("pmc: Asus E200HA detected\n"); + quirks |= QUIRK_DISABLE_SATA; + return 1; +} + +static const struct dmi_system_id cht_table[] = { + { + .callback = cht_asus_e200ha_cb, + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."), + DMI_MATCH(DMI_PRODUCT_NAME, "E200HA"), + }, + }, + { } +}; + static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) { struct pmc_dev *pmc = &pmc_device; @@ -526,6 +557,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) pmc->map = map; + dmi_check_system(cht_table); + /* PMC hardware registers setup */ pmc_hw_reg_setup(pmc);
SATA controller is enabled on Asus E200HA even though the machine doesn't use it (it has eMMC storage), however SATA being on blocks S0ix entry so we need to disable it. Signed-off-by: Johannes Stezenbach <js@sig21.net> --- drivers/platform/x86/pmc_atom.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)