Re: [PATCH v2] x86: i8237: Register based on FADT legacy boot flag

[Thomas Gleixner]

On Mon, 19 Mar 2018, Anshuman Gupta wrote:
> On Wed, Mar 14, 2018 at 04:07:43PM +0100, Thomas Gleixner wrote:
> > On Wed, 14 Mar 2018, Rajneesh Bhardwaj wrote:
> > > +  * support 8237 DMA or bus mastering from LPC. Platform firmware
> > > +  * must announce the support for such legacy devices via
> > > +  * ACPI_FADT_LEGACY_DEVICES field in FADT table.
> > > +  */
> > > + if (!x86_platform.legacy.devices.pnpbios && dmi_get_bios_year() >= 2017)
> > 
> > Please use arch_pnpbios_disabled() and explain why you need that year
>   arch_pnpbios_disabled is defined only when CONFIG_PNPBIOS is set, which may 
>   not be true for all configurations.

The please create a helper function x86_pnpbios_disabled() and use that one
in arch_pnp_bios_disabled() and in your new code.

> > check. If there is no pnpbios then why is the year interesting and why
> > would anyone trust something which comes from BIOS?
>
>   Modern bios fsp have started using to use ACPI_FADT_LEGACY_DEVICES field in
>   FADT table, that is the reason we require a year check for BIOS.
>   https://review.coreboot.org/#/c/coreboot/+/23833/
>   https://review.coreboot.org/#/c/coreboot/+/23835/ 

Sorry, this gerrit stuff is unreadable. Please explain it in your own
words. You have to do that anyway as that information wants to be in a
comment or at least in the change log.

Thanks,

tglx

Re: [PATCH v2] x86: i8237: Register based on FADT legacy boot flag

[Thomas Gleixner]

On Mon, 19 Mar 2018, Anshuman Gupta wrote:
> On Wed, Mar 14, 2018 at 04:07:43PM +0100, Thomas Gleixner wrote:
> > On Wed, 14 Mar 2018, Rajneesh Bhardwaj wrote:
> > > +  * support 8237 DMA or bus mastering from LPC. Platform firmware
> > > +  * must announce the support for such legacy devices via
> > > +  * ACPI_FADT_LEGACY_DEVICES field in FADT table.
> > > +  */
> > > + if (!x86_platform.legacy.devices.pnpbios && dmi_get_bios_year() >= 2017)
> > 
> > Please use arch_pnpbios_disabled() and explain why you need that year
>   arch_pnpbios_disabled is defined only when CONFIG_PNPBIOS is set, which may 
>   not be true for all configurations.

The please create a helper function x86_pnpbios_disabled() and use that one
in arch_pnp_bios_disabled() and in your new code.

> > check. If there is no pnpbios then why is the year interesting and why
> > would anyone trust something which comes from BIOS?
>
>   Modern bios fsp have started using to use ACPI_FADT_LEGACY_DEVICES field in
>   FADT table, that is the reason we require a year check for BIOS.
>   https://review.coreboot.org/#/c/coreboot/+/23833/
>   https://review.coreboot.org/#/c/coreboot/+/23835/ 

Sorry, this gerrit stuff is unreadable. Please explain it in your own
words. You have to do that anyway as that information wants to be in a
comment or at least in the change log.

Thanks,

tglx

Re: [PATCH v2] x86: i8237: Register based on FADT legacy boot flag

[Thomas Gleixner]

On Wed, 14 Mar 2018, Rajneesh Bhardwaj wrote:
>  static int __init i8237A_init_ops(void)
>  {
> + /*
> +  * From SKL PCH onwards, the port 0x61 bit 4 would stop toggle and

s/would stop toggle/stops toggling/

You are describing a fact, right?

> +  * the legacy DMA device is removed in which the I/O ports (81h-83h,
> +  * 87h, 89h-8Bh, 8Fh)  related to it are removed as well. All
> +  * removed ports must return 0xff for a inb() request.
> +  *
> +  * Note: DMA_PAGE_2 (port 0x81) should not be checked for detecting
> +  * the presence of DMA device since it may be used by BIOS to decode
> +  * LPC traffic for POST codes. Original LPC only decodes one byte of
> +  * port 0x80 but some BIOS may choose to enhance PCH LPC port 0x8x
> +  * decoding.
> +  */
> + if (dma_inb(DMA_PAGE_0) == 0xFF)
> + return -ENODEV;
> +
> + /*
> +  * It should be OK to not load this driver as newer SoC may not

Should? Is this based on facts or hope?

> +  * support 8237 DMA or bus mastering from LPC. Platform firmware
> +  * must announce the support for such legacy devices via
> +  * ACPI_FADT_LEGACY_DEVICES field in FADT table.
> +  */
> + if (!x86_platform.legacy.devices.pnpbios && dmi_get_bios_year() >= 2017)

Please use arch_pnpbios_disabled() and explain why you need that year
check. If there is no pnpbios then why is the year interesting and why
would anyone trust something which comes from BIOS?

Thanks,

tglx

Re: [PATCH v2] x86: i8237: Register based on FADT legacy boot flag

[Thomas Gleixner]

On Wed, 14 Mar 2018, Rajneesh Bhardwaj wrote:
>  static int __init i8237A_init_ops(void)
>  {
> + /*
> +  * From SKL PCH onwards, the port 0x61 bit 4 would stop toggle and

s/would stop toggle/stops toggling/

You are describing a fact, right?

> +  * the legacy DMA device is removed in which the I/O ports (81h-83h,
> +  * 87h, 89h-8Bh, 8Fh)  related to it are removed as well. All
> +  * removed ports must return 0xff for a inb() request.
> +  *
> +  * Note: DMA_PAGE_2 (port 0x81) should not be checked for detecting
> +  * the presence of DMA device since it may be used by BIOS to decode
> +  * LPC traffic for POST codes. Original LPC only decodes one byte of
> +  * port 0x80 but some BIOS may choose to enhance PCH LPC port 0x8x
> +  * decoding.
> +  */
> + if (dma_inb(DMA_PAGE_0) == 0xFF)
> + return -ENODEV;
> +
> + /*
> +  * It should be OK to not load this driver as newer SoC may not

Should? Is this based on facts or hope?

> +  * support 8237 DMA or bus mastering from LPC. Platform firmware
> +  * must announce the support for such legacy devices via
> +  * ACPI_FADT_LEGACY_DEVICES field in FADT table.
> +  */
> + if (!x86_platform.legacy.devices.pnpbios && dmi_get_bios_year() >= 2017)

Please use arch_pnpbios_disabled() and explain why you need that year
check. If there is no pnpbios then why is the year interesting and why
would anyone trust something which comes from BIOS?

Thanks,

tglx

Re: [PATCH v2] x86: i8237: Register based on FADT legacy boot flag

[Andy Shevchenko]

On Wed, 2018-03-14 at 10:47 +0530, Rajneesh Bhardwaj wrote:
> From Skylake onwards, the platform controller hub (Sunrisepoint PCH)
> does
> not support legacy DMA operations to IO ports 81h-83h, 87h, 89h-8Bh,
> 8Fh.
> Currently this driver registers as syscore ops and its resume function
> is
> called on every resume from S3. On Skylake and Kabylake, this causes a
> resume delay of around 100ms due to port IO operations, which is a
> problem.
> 
> This change allows to load the driver only when the platform bios
> explicitly supports such devices or has a cut-off date earlier than
> 2017.
> 
> Please refer to chapter 21 of 6th Generation Intel® Core™ Processor
> Platform Controller Hub Family: BIOS Specification.
> 
> https://www.intel.in/content/www/in/en/embedded/products/skylake/u-mob
> ile/software-and-drivers.html
> 
> Cc: Alan Cox 
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Anshuman Gupta 
> Signed-off-by: Rajneesh Bhardwaj 
> ---
> Changes in v2:
>  * changed to dma_inb()
> 
> This depends on recently introduced dmi_get_bios_year() helper.
> https://patchwork.kernel.org/patch/10252151/
>  arch/x86/kernel/i8237.c | 25 +
>  1 file changed, 25 insertions(+)
> 
> diff --git a/arch/x86/kernel/i8237.c b/arch/x86/kernel/i8237.c
> index 8eeaa81de066..bf5d8104e17b 100644
> --- a/arch/x86/kernel/i8237.c
> +++ b/arch/x86/kernel/i8237.c
> @@ -9,6 +9,7 @@
>   * your option) any later version.
>   */
>  
> +#include 
>  #include 
>  #include 

As kbuild bot rightfully noticed, you need to add
x86_init.h inclusion to the driver.

>  
> @@ -49,6 +50,30 @@ static struct syscore_ops i8237_syscore_ops = {
>  
>  static int __init i8237A_init_ops(void)
>  {
> + /*
> +  * From SKL PCH onwards, the port 0x61 bit 4 would stop
> toggle and
> +  * the legacy DMA device is removed in which the I/O ports
> (81h-83h,
> +  * 87h, 89h-8Bh, 8Fh)  related to it are removed as well. All
> +  * removed ports must return 0xff for a inb() request.
> +  *
> +  * Note: DMA_PAGE_2 (port 0x81) should not be checked for
> detecting
> +  * the presence of DMA device since it may be used by BIOS to
> decode
> +  * LPC traffic for POST codes. Original LPC only decodes one
> byte of
> +  * port 0x80 but some BIOS may choose to enhance PCH LPC port
> 0x8x
> +  * decoding.
> +  */
> + if (dma_inb(DMA_PAGE_0) == 0xFF)
> + return -ENODEV;
> +
> + /*
> +  * It should be OK to not load this driver as newer SoC may
> not
> +  * support 8237 DMA or bus mastering from LPC. Platform
> firmware
> +  * must announce the support for such legacy devices via
> +  * ACPI_FADT_LEGACY_DEVICES field in FADT table.
> +  */
> + if (!x86_platform.legacy.devices.pnpbios &&
> dmi_get_bios_year() >= 2017)
> + return -ENODEV;
> +
>   register_syscore_ops(_syscore_ops);
>   return 0;
>  }

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v2] x86: i8237: Register based on FADT legacy boot flag

[Andy Shevchenko]

On Wed, 2018-03-14 at 10:47 +0530, Rajneesh Bhardwaj wrote:
> From Skylake onwards, the platform controller hub (Sunrisepoint PCH)
> does
> not support legacy DMA operations to IO ports 81h-83h, 87h, 89h-8Bh,
> 8Fh.
> Currently this driver registers as syscore ops and its resume function
> is
> called on every resume from S3. On Skylake and Kabylake, this causes a
> resume delay of around 100ms due to port IO operations, which is a
> problem.
> 
> This change allows to load the driver only when the platform bios
> explicitly supports such devices or has a cut-off date earlier than
> 2017.
> 
> Please refer to chapter 21 of 6th Generation Intel® Core™ Processor
> Platform Controller Hub Family: BIOS Specification.
> 
> https://www.intel.in/content/www/in/en/embedded/products/skylake/u-mob
> ile/software-and-drivers.html
> 
> Cc: Alan Cox 
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Anshuman Gupta 
> Signed-off-by: Rajneesh Bhardwaj 
> ---
> Changes in v2:
>  * changed to dma_inb()
> 
> This depends on recently introduced dmi_get_bios_year() helper.
> https://patchwork.kernel.org/patch/10252151/
>  arch/x86/kernel/i8237.c | 25 +
>  1 file changed, 25 insertions(+)
> 
> diff --git a/arch/x86/kernel/i8237.c b/arch/x86/kernel/i8237.c
> index 8eeaa81de066..bf5d8104e17b 100644
> --- a/arch/x86/kernel/i8237.c
> +++ b/arch/x86/kernel/i8237.c
> @@ -9,6 +9,7 @@
>   * your option) any later version.
>   */
>  
> +#include 
>  #include 
>  #include 

As kbuild bot rightfully noticed, you need to add
x86_init.h inclusion to the driver.

>  
> @@ -49,6 +50,30 @@ static struct syscore_ops i8237_syscore_ops = {
>  
>  static int __init i8237A_init_ops(void)
>  {
> + /*
> +  * From SKL PCH onwards, the port 0x61 bit 4 would stop
> toggle and
> +  * the legacy DMA device is removed in which the I/O ports
> (81h-83h,
> +  * 87h, 89h-8Bh, 8Fh)  related to it are removed as well. All
> +  * removed ports must return 0xff for a inb() request.
> +  *
> +  * Note: DMA_PAGE_2 (port 0x81) should not be checked for
> detecting
> +  * the presence of DMA device since it may be used by BIOS to
> decode
> +  * LPC traffic for POST codes. Original LPC only decodes one
> byte of
> +  * port 0x80 but some BIOS may choose to enhance PCH LPC port
> 0x8x
> +  * decoding.
> +  */
> + if (dma_inb(DMA_PAGE_0) == 0xFF)
> + return -ENODEV;
> +
> + /*
> +  * It should be OK to not load this driver as newer SoC may
> not
> +  * support 8237 DMA or bus mastering from LPC. Platform
> firmware
> +  * must announce the support for such legacy devices via
> +  * ACPI_FADT_LEGACY_DEVICES field in FADT table.
> +  */
> + if (!x86_platform.legacy.devices.pnpbios &&
> dmi_get_bios_year() >= 2017)
> + return -ENODEV;
> +
>   register_syscore_ops(_syscore_ops);
>   return 0;
>  }

-- 
Andy Shevchenko 
Intel Finland Oy