[PATCH 0/6] 8250: AMD Carrizo UART PL300 DMA enablement

[PATCH 0/6] 8250: AMD Carrizo UART PL300 DMA enablement

[Wang Hongcheng]

Hi, all

As AMD carrizo UART device is compatible with 8250 and has pl330 DMA
IP, our uart driver is serial:8250 and DMA engine is registered by
driver/dma/pl330. The following patches are made, in order to enable
DMA.

V1:
http://lkml.kernel.org/g/1449199466-6081-1-git-send-email-annie.wang@amd.com

Major changes from V1->V2:
- Create an amba device in driver/acpi/acpi_apd.c, as the DMA device
  relies on UART device and it can only be used by UART device, they
  are not siblings. UART device should be the parent of DMA device.
- base_request_line and num are added to acpi dma controller register

Thanks,
Hongcheng (Annie)

Wang Hongcheng (6):
  8250/Kconfig: add config option CONFIG_SERIAL_8250_AMD
  ACPI: create setup_quirk in acpi_apd
  ACPI: add 2 parameters to function acpi dma controller register
  dmaengine: pl330: add new items for pl330 private data
  dmaengine: pl330: provide ACPI dmaengine interface
  Serial:8250: New Port Type PORT_AMD_8250

 drivers/acpi/acpi_apd.c               | 175 ++++++++++++++++++++++++++++++++--
 drivers/dma/acpi-dma.c                |  25 ++++-
 drivers/dma/dw/platform.c             |   2 +-
 drivers/dma/pl330.c                   |  36 ++++++-
 drivers/tty/serial/8250/8250_dw.c     |  15 +++
 drivers/tty/serial/8250/8250_port.c   |   9 ++
 drivers/tty/serial/8250/Kconfig       |   8 ++
 include/linux/acpi_dma.h              |   6 ++
 include/linux/amba/pl330.h            |   6 ++
 include/linux/platform_data/8250-dw.h |   8 ++
 include/uapi/linux/serial_core.h      |   3 +-
 include/uapi/linux/serial_reg.h       |   1 +
 12 files changed, 276 insertions(+), 18 deletions(-)
 create mode 100644 include/linux/platform_data/8250-dw.h

-- 
1.9.1

[PATCH 0/6] 8250: AMD Carrizo UART PL300 DMA enablement

[Wang Hongcheng]

Hi, all

As AMD carrizo UART device is compatible with 8250 and has pl330 DMA
IP, our uart driver is serial:8250 and DMA engine is registered by
driver/dma/pl330. The following patches are made, in order to enable
DMA.

V1:
http://lkml.kernel.org/g/1449199466-6081-1-git-send-email-annie.wang@amd.com

Major changes from V1->V2:
- Create an amba device in driver/acpi/acpi_apd.c, as the DMA device
  relies on UART device and it can only be used by UART device, they
  are not siblings. UART device should be the parent of DMA device.
- base_request_line and num are added to acpi dma controller register

Thanks,
Hongcheng (Annie)

Wang Hongcheng (6):
  8250/Kconfig: add config option CONFIG_SERIAL_8250_AMD
  ACPI: create setup_quirk in acpi_apd
  ACPI: add 2 parameters to function acpi dma controller register
  dmaengine: pl330: add new items for pl330 private data
  dmaengine: pl330: provide ACPI dmaengine interface
  Serial:8250: New Port Type PORT_AMD_8250

 drivers/acpi/acpi_apd.c               | 175 ++++++++++++++++++++++++++++++++--
 drivers/dma/acpi-dma.c                |  25 ++++-
 drivers/dma/dw/platform.c             |   2 +-
 drivers/dma/pl330.c                   |  36 ++++++-
 drivers/tty/serial/8250/8250_dw.c     |  15 +++
 drivers/tty/serial/8250/8250_port.c   |   9 ++
 drivers/tty/serial/8250/Kconfig       |   8 ++
 include/linux/acpi_dma.h              |   6 ++
 include/linux/amba/pl330.h            |   6 ++
 include/linux/platform_data/8250-dw.h |   8 ++
 include/uapi/linux/serial_core.h      |   3 +-
 include/uapi/linux/serial_reg.h       |   1 +
 12 files changed, 276 insertions(+), 18 deletions(-)
 create mode 100644 include/linux/platform_data/8250-dw.h

-- 
1.9.1

[PATCH 1/6] 8250/Kconfig: add config option CONFIG_SERIAL_8250_AMD

[Wang Hongcheng]

Add config option  CONFIG_SERIAL_8250_AMD in use of AMD carrizo.
Because carrizo's UART DMA device is an amba device, it selects
ARM_AMBA option. Anything uses amba devices must select ARM_AMBA.

Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
---
 drivers/tty/serial/8250/Kconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 6412f14..c9ebc31 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -378,3 +378,11 @@ config SERIAL_8250_MID
 	  Selecting this option will enable handling of the extra features
 	  present on the UART found on Intel Medfield SOC and various other
 	  Intel platforms.
+
+config SERIAL_8250_AMD
+	bool "AMD carrizo serial port support"
+	depends on SERIAL_8250
+	select ARM_AMBA
+	help
+	  If you have a Family 15h, models 0x60-0x6F based board and want to
+	  use the serial port, say Y to this option. If unsure, say N.
-- 
1.9.1

[PATCH 1/6] 8250/Kconfig: add config option CONFIG_SERIAL_8250_AMD

[Wang Hongcheng]

Add config option  CONFIG_SERIAL_8250_AMD in use of AMD carrizo.
Because carrizo's UART DMA device is an amba device, it selects
ARM_AMBA option. Anything uses amba devices must select ARM_AMBA.

Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
---
 drivers/tty/serial/8250/Kconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 6412f14..c9ebc31 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -378,3 +378,11 @@ config SERIAL_8250_MID
 	  Selecting this option will enable handling of the extra features
 	  present on the UART found on Intel Medfield SOC and various other
 	  Intel platforms.
+
+config SERIAL_8250_AMD
+	bool "AMD carrizo serial port support"
+	depends on SERIAL_8250
+	select ARM_AMBA
+	help
+	  If you have a Family 15h, models 0x60-0x6F based board and want to
+	  use the serial port, say Y to this option. If unsure, say N.
-- 
1.9.1

Re: [PATCH 1/6] 8250/Kconfig: add config option CONFIG_SERIAL_8250_AMD

[Borislav Petkov]

On Mon, Jan 04, 2016 at 01:31:36PM +0800, Wang Hongcheng wrote:
> Add config option  CONFIG_SERIAL_8250_AMD in use of AMD carrizo.
> Because carrizo's UART DMA device is an amba device, it selects
> ARM_AMBA option. Anything uses amba devices must select ARM_AMBA.
> 
> Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
> ---
>  drivers/tty/serial/8250/Kconfig | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 6412f14..c9ebc31 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -378,3 +378,11 @@ config SERIAL_8250_MID
>  	  Selecting this option will enable handling of the extra features
>  	  present on the UART found on Intel Medfield SOC and various other
>  	  Intel platforms.
> +
> +config SERIAL_8250_AMD
> +	bool "AMD carrizo serial port support"
> +	depends on SERIAL_8250
> +	select ARM_AMBA
> +	help
> +	  If you have a Family 15h, models 0x60-0x6F based board and want to
> +	  use the serial port, say Y to this option. If unsure, say N.

Hmm, so you're adding this config option here only to have
acpi_apd_setup_quirks() defined in an already AMD-specific compilation
unit drivers/acpi/acpi_apd.c.

So why not make drivers/acpi/acpi_apd.c depend on CPU_SUP_AMD
and this way it is automatically enabled on AMD and then check
family/model/stepping when assigning that

+       .post_setup = acpi_apd_setup_quirks,

thing?

You need it only on F15h, models 0x60.. only so you don't really need
the config option when you can find out on what hardware you're running
without the user having to configure the kernel.

Hmmm?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

RE: [PATCH 1/6] 8250/Kconfig: add config option CONFIG_SERIAL_8250_AMD

[Wang, Annie]

Hi Boris,

>-----Original Message-----
>From: Borislav Petkov [mailto:bp@alien8.de]
>Sent: Monday, January 04, 2016 10:41 PM
>To: Wang, Annie
>Cc: Andy Shevchenko; Vinod Koul; Mika Westerberg; Greg Kroah-Hartman; Rafael
>J. Wysocki; linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>serial@vger.kernel.org; dmaengine@vger.kernel.org; Huang, Ray; Wan, Vincent;
>Xue, Ken; Robin Murphy; Graeme Gregory; Li, Tony; Yu, Xiangliang
>Subject: Re: [PATCH 1/6] 8250/Kconfig: add config option
>CONFIG_SERIAL_8250_AMD
>
>On Mon, Jan 04, 2016 at 01:31:36PM +0800, Wang Hongcheng wrote:
>> Add config option  CONFIG_SERIAL_8250_AMD in use of AMD carrizo.
>> Because carrizo's UART DMA device is an amba device, it selects
>> ARM_AMBA option. Anything uses amba devices must select ARM_AMBA.
>>
>> Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
>> ---
>>  drivers/tty/serial/8250/Kconfig | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/tty/serial/8250/Kconfig
>> b/drivers/tty/serial/8250/Kconfig index 6412f14..c9ebc31 100644
>> --- a/drivers/tty/serial/8250/Kconfig
>> +++ b/drivers/tty/serial/8250/Kconfig
>> @@ -378,3 +378,11 @@ config SERIAL_8250_MID
>>  	  Selecting this option will enable handling of the extra features
>>  	  present on the UART found on Intel Medfield SOC and various other
>>  	  Intel platforms.
>> +
>> +config SERIAL_8250_AMD
>> +	bool "AMD carrizo serial port support"
>> +	depends on SERIAL_8250
>> +	select ARM_AMBA
>> +	help
>> +	  If you have a Family 15h, models 0x60-0x6F based board and want to
>> +	  use the serial port, say Y to this option. If unsure, say N.
>
>Hmm, so you're adding this config option here only to have
>acpi_apd_setup_quirks() defined in an already AMD-specific compilation unit
>drivers/acpi/acpi_apd.c.
>

How about I add select ARM_AMBA and SERIAL_8250 in arch/x86/Kconfig?

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index db3622f..0fe6657 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -537,11 +537,15 @@ config X86_AMD_PLATFORM_DEVICE
        depends on ACPI
        select COMMON_CLK
        select PINCTRL
+       select SERIAL_8250
+       select ARM_AMBA
        ---help---
          Select to interpret AMD specific ACPI device to platform device
          such as I2C, UART, GPIO found on AMD Carrizo and later chipsets.
          I2C and UART depend on COMMON_CLK to set clock. GPIO driver is
-         implemented under PINCTRL subsystem.
+         implemented under PINCTRL subsystem. Carrizo's UART is implemented
+         under SERIAL_8250. Carrizo's UART DMA device is an amba device,
+         it selects ARM_AMBA option.

 config IOSF_MBI
        tristate "Intel SoC IOSF Sideband support for SoC platforms"
--

>So why not make drivers/acpi/acpi_apd.c depend on CPU_SUP_AMD and this way
>it is automatically enabled on AMD and then check family/model/stepping when
>assigning that
>
>+       .post_setup = acpi_apd_setup_quirks,
>
>thing?
>
>You need it only on F15h, models 0x60.. only so you don't really need the config
>option when you can find out on what hardware you're running without the user
>having to configure the kernel.
>
>Hmmm?

CPU_SUP_AMD is only configured in X86 arch. AMD future  ARM64 processors may
also need acpi to platform support. 
  

RE: [PATCH 1/6] 8250/Kconfig: add config option CONFIG_SERIAL_8250_AMD

[Wang, Annie]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3457 bytes --]

Hi Boris,

>-----Original Message-----
>From: Borislav Petkov [mailto:bp@alien8.de]
>Sent: Monday, January 04, 2016 10:41 PM
>To: Wang, Annie
>Cc: Andy Shevchenko; Vinod Koul; Mika Westerberg; Greg Kroah-Hartman; Rafael
>J. Wysocki; linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>serial@vger.kernel.org; dmaengine@vger.kernel.org; Huang, Ray; Wan, Vincent;
>Xue, Ken; Robin Murphy; Graeme Gregory; Li, Tony; Yu, Xiangliang
>Subject: Re: [PATCH 1/6] 8250/Kconfig: add config option
>CONFIG_SERIAL_8250_AMD
>
>On Mon, Jan 04, 2016 at 01:31:36PM +0800, Wang Hongcheng wrote:
>> Add config option  CONFIG_SERIAL_8250_AMD in use of AMD carrizo.
>> Because carrizo's UART DMA device is an amba device, it selects
>> ARM_AMBA option. Anything uses amba devices must select ARM_AMBA.
>>
>> Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
>> ---
>>  drivers/tty/serial/8250/Kconfig | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/tty/serial/8250/Kconfig
>> b/drivers/tty/serial/8250/Kconfig index 6412f14..c9ebc31 100644
>> --- a/drivers/tty/serial/8250/Kconfig
>> +++ b/drivers/tty/serial/8250/Kconfig
>> @@ -378,3 +378,11 @@ config SERIAL_8250_MID
>>  	  Selecting this option will enable handling of the extra features
>>  	  present on the UART found on Intel Medfield SOC and various other
>>  	  Intel platforms.
>> +
>> +config SERIAL_8250_AMD
>> +	bool "AMD carrizo serial port support"
>> +	depends on SERIAL_8250
>> +	select ARM_AMBA
>> +	help
>> +	  If you have a Family 15h, models 0x60-0x6F based board and want to
>> +	  use the serial port, say Y to this option. If unsure, say N.
>
>Hmm, so you're adding this config option here only to have
>acpi_apd_setup_quirks() defined in an already AMD-specific compilation unit
>drivers/acpi/acpi_apd.c.
>

How about I add select ARM_AMBA and SERIAL_8250 in arch/x86/Kconfig?

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index db3622f..0fe6657 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -537,11 +537,15 @@ config X86_AMD_PLATFORM_DEVICE
        depends on ACPI
        select COMMON_CLK
        select PINCTRL
+       select SERIAL_8250
+       select ARM_AMBA
        ---help---
          Select to interpret AMD specific ACPI device to platform device
          such as I2C, UART, GPIO found on AMD Carrizo and later chipsets.
          I2C and UART depend on COMMON_CLK to set clock. GPIO driver is
-         implemented under PINCTRL subsystem.
+         implemented under PINCTRL subsystem. Carrizo's UART is implemented
+         under SERIAL_8250. Carrizo's UART DMA device is an amba device,
+         it selects ARM_AMBA option.

 config IOSF_MBI
        tristate "Intel SoC IOSF Sideband support for SoC platforms"
--

>So why not make drivers/acpi/acpi_apd.c depend on CPU_SUP_AMD and this way
>it is automatically enabled on AMD and then check family/model/stepping when
>assigning that
>
>+       .post_setup = acpi_apd_setup_quirks,
>
>thing?
>
>You need it only on F15h, models 0x60.. only so you don't really need the config
>option when you can find out on what hardware you're running without the user
>having to configure the kernel.
>
>Hmmm?

CPU_SUP_AMD is only configured in X86 arch. AMD future  ARM64 processors may
also need acpi to platform support. 
  

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/6] 8250/Kconfig: add config option CONFIG_SERIAL_8250_AMD

[Borislav Petkov]

On Wed, Jan 06, 2016 at 02:08:18AM +0000, Wang, Annie wrote:
> How about I add select ARM_AMBA and SERIAL_8250 in arch/x86/Kconfig?

Yeah, select sounds good in that case, except in that particular case ...

> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index db3622f..0fe6657 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -537,11 +537,15 @@ config X86_AMD_PLATFORM_DEVICE
>         depends on ACPI
>         select COMMON_CLK
>         select PINCTRL
> +       select SERIAL_8250
> +       select ARM_AMBA

... that's a X86_AMD_PLATFORM_DEVICE which selects ARM thing? i.e.,
ARM_AMBA. Can that even work?

[ Rant on the side: And that ARM_AMBA thing has, of course, no effing
  help text. Dammit, people need to start explaining those cryptic
  abbreviations. Somewhere in the code I found "Advanced Microcontroller
  Bus Architecture". This is clearly suboptimal. ]

So why does the X86 platform device need to select the AMBA crap?

>         ---help---
>           Select to interpret AMD specific ACPI device to platform device
>           such as I2C, UART, GPIO found on AMD Carrizo and later chipsets.
>           I2C and UART depend on COMMON_CLK to set clock. GPIO driver is
> -         implemented under PINCTRL subsystem.
> +         implemented under PINCTRL subsystem. Carrizo's UART is implemented
> +         under SERIAL_8250. Carrizo's UART DMA device is an amba device,
> +         it selects ARM_AMBA option.

As I already said before, please refrain from using platform names like
Carrizo because people have no clue what those are. Only the marketing
people do. Use CPU family + models instead.

> CPU_SUP_AMD is only configured in X86 arch. AMD future  ARM64 processors may
> also need acpi to platform support.

So this code is going to be shared between X86 and ARM64?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

RE: [PATCH 1/6] 8250/Kconfig: add config option CONFIG_SERIAL_8250_AMD

[Wang, Annie]

>-----Original Message-----
>From: Borislav Petkov [mailto:bp@alien8.de]
>Sent: Wednesday, January 06, 2016 6:46 PM
>To: Wang, Annie
>Cc: Andy Shevchenko; Vinod Koul; Mika Westerberg; Greg Kroah-Hartman; Rafael
>J. Wysocki; linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>serial@vger.kernel.org; dmaengine@vger.kernel.org; Huang, Ray; Wan, Vincent;
>Xue, Ken; Robin Murphy; Graeme Gregory; Li, Tony; Yu, Xiangliang
>Subject: Re: [PATCH 1/6] 8250/Kconfig: add config option
>CONFIG_SERIAL_8250_AMD
>
>On Wed, Jan 06, 2016 at 02:08:18AM +0000, Wang, Annie wrote:
>> How about I add select ARM_AMBA and SERIAL_8250 in arch/x86/Kconfig?
>
>Yeah, select sounds good in that case, except in that particular case ...
>
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index
>> db3622f..0fe6657 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -537,11 +537,15 @@ config X86_AMD_PLATFORM_DEVICE
>>         depends on ACPI
>>         select COMMON_CLK
>>         select PINCTRL
>> +       select SERIAL_8250
>> +       select ARM_AMBA
>
>... that's a X86_AMD_PLATFORM_DEVICE which selects ARM thing? i.e.,
>ARM_AMBA. Can that even work?
>
>[ Rant on the side: And that ARM_AMBA thing has, of course, no effing
>  help text. Dammit, people need to start explaining those cryptic
>  abbreviations. Somewhere in the code I found "Advanced Microcontroller
>  Bus Architecture". This is clearly suboptimal. ]
>
>So why does the X86 platform device need to select the AMBA crap?


Russell, 

The AMBA bus is already leveraged  in AMD X86 arch hardware design for UART
controller and UART DMA. And may will be used in other arch as well, however,
it is rather confusing if we select ARM_AMBA in other arch, such as X86.

How about rename  CONFIG_ARM_AMBA to CONFIG_AMBA? So different arch
can select it without causing misunderstanding. 

Thank you very much.
Regards,
Hongcheng(Annie)

[PATCH 1/6] 8250/Kconfig: add config option CONFIG_SERIAL_8250_AMD

[Wang, Annie]

>-----Original Message-----
>From: Borislav Petkov [mailto:bp at alien8.de]
>Sent: Wednesday, January 06, 2016 6:46 PM
>To: Wang, Annie
>Cc: Andy Shevchenko; Vinod Koul; Mika Westerberg; Greg Kroah-Hartman; Rafael
>J. Wysocki; linux-acpi at vger.kernel.org; linux-kernel at vger.kernel.org; linux-
>serial at vger.kernel.org; dmaengine at vger.kernel.org; Huang, Ray; Wan, Vincent;
>Xue, Ken; Robin Murphy; Graeme Gregory; Li, Tony; Yu, Xiangliang
>Subject: Re: [PATCH 1/6] 8250/Kconfig: add config option
>CONFIG_SERIAL_8250_AMD
>
>On Wed, Jan 06, 2016 at 02:08:18AM +0000, Wang, Annie wrote:
>> How about I add select ARM_AMBA and SERIAL_8250 in arch/x86/Kconfig?
>
>Yeah, select sounds good in that case, except in that particular case ...
>
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index
>> db3622f..0fe6657 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -537,11 +537,15 @@ config X86_AMD_PLATFORM_DEVICE
>>         depends on ACPI
>>         select COMMON_CLK
>>         select PINCTRL
>> +       select SERIAL_8250
>> +       select ARM_AMBA
>
>... that's a X86_AMD_PLATFORM_DEVICE which selects ARM thing? i.e.,
>ARM_AMBA. Can that even work?
>
>[ Rant on the side: And that ARM_AMBA thing has, of course, no effing
>  help text. Dammit, people need to start explaining those cryptic
>  abbreviations. Somewhere in the code I found "Advanced Microcontroller
>  Bus Architecture". This is clearly suboptimal. ]
>
>So why does the X86 platform device need to select the AMBA crap?


Russell, 

The AMBA bus is already leveraged  in AMD X86 arch hardware design for UART
controller and UART DMA. And may will be used in other arch as well, however,
it is rather confusing if we select ARM_AMBA in other arch, such as X86.

How about rename  CONFIG_ARM_AMBA to CONFIG_AMBA? So different arch
can select it without causing misunderstanding. 

Thank you very much.
Regards,
Hongcheng(Annie)

RE: [PATCH 1/6] 8250/Kconfig: add config option CONFIG_SERIAL_8250_AMD

[Wang, Annie]

>-----Original Message-----
>From: Borislav Petkov [mailto:bp@alien8.de]
>Sent: Wednesday, January 06, 2016 6:46 PM
>To: Wang, Annie
>Cc: Andy Shevchenko; Vinod Koul; Mika Westerberg; Greg Kroah-Hartman; Rafael
>J. Wysocki; linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>serial@vger.kernel.org; dmaengine@vger.kernel.org; Huang, Ray; Wan, Vincent;
>Xue, Ken; Robin Murphy; Graeme Gregory; Li, Tony; Yu, Xiangliang
>Subject: Re: [PATCH 1/6] 8250/Kconfig: add config option
>CONFIG_SERIAL_8250_AMD
>
>On Wed, Jan 06, 2016 at 02:08:18AM +0000, Wang, Annie wrote:
>> How about I add select ARM_AMBA and SERIAL_8250 in arch/x86/Kconfig?
>
>Yeah, select sounds good in that case, except in that particular case ...
>
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index
>> db3622f..0fe6657 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -537,11 +537,15 @@ config X86_AMD_PLATFORM_DEVICE
>>         depends on ACPI
>>         select COMMON_CLK
>>         select PINCTRL
>> +       select SERIAL_8250
>> +       select ARM_AMBA
>
>... that's a X86_AMD_PLATFORM_DEVICE which selects ARM thing? i.e.,
>ARM_AMBA. Can that even work?
>
>[ Rant on the side: And that ARM_AMBA thing has, of course, no effing
>  help text. Dammit, people need to start explaining those cryptic
>  abbreviations. Somewhere in the code I found "Advanced Microcontroller
>  Bus Architecture". This is clearly suboptimal. ]
>
>So why does the X86 platform device need to select the AMBA crap?


Russell, 

The AMBA bus is already leveraged  in AMD X86 arch hardware design for UART
controller and UART DMA. And may will be used in other arch as well, however,
it is rather confusing if we select ARM_AMBA in other arch, such as X86.

How about rename  CONFIG_ARM_AMBA to CONFIG_AMBA? So different arch
can select it without causing misunderstanding. 

Thank you very much.
Regards,
Hongcheng(Annie)

[PATCH 2/6] ACPI: create setup_quirk in acpi_apd

[Wang Hongcheng]

The post_setup hook of AMD0020 device will create an amba device as
the child of a previously created platform device.

Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
---
 drivers/acpi/acpi_apd.c | 131 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 121 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
index a450e7a..9520daf 100644
--- a/drivers/acpi/acpi_apd.c
+++ b/drivers/acpi/acpi_apd.c
@@ -3,7 +3,8 @@
  *
  * Copyright (c) 2014,2015 AMD Corporation.
  * Authors: Ken Xue <Ken.Xue@amd.com>
- *	Wu, Jeff <Jeff.Wu@amd.com>
+ *          Jeff Wu <15618388108@163.com>
+ *          Wang Hongcheng <Annie.Wang@amd.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -17,6 +18,8 @@
 #include <linux/acpi.h>
 #include <linux/err.h>
 #include <linux/pm.h>
+#include <linux/amba/bus.h>
+#include <linux/dma-mapping.h>
 
 #include "internal.h"
 
@@ -36,19 +39,23 @@ struct apd_private_data;
  * @fixed_clk_rate: fixed rate input clock source for acpi device;
  *			0 means no fixed rate input clock source
  * @setup: a hook routine to set device resource during create platform device
- *
+ * @post_setup: an additional hook routine
  * Device description defined as acpi_device_id.driver_data
  */
 struct apd_device_desc {
 	unsigned int flags;
 	unsigned int fixed_clk_rate;
+	unsigned int base_offset;
+	unsigned int periphid;
 	int (*setup)(struct apd_private_data *pdata);
+	int (*post_setup)(struct apd_private_data *pdata);
 };
 
 struct apd_private_data {
 	struct clk *clk;
 	struct acpi_device *adev;
 	const struct apd_device_desc *dev_desc;
+	struct platform_device *pdev;
 };
 
 #ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
@@ -71,6 +78,100 @@ static int acpi_apd_setup(struct apd_private_data *pdata)
 	return 0;
 }
 
+#ifdef CONFIG_SERIAL_8250_AMD
+static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
+{
+	struct amba_device *amba_dev = NULL;
+	struct platform_device *pdev = pdata->pdev;
+	struct resource *presource, *resource = NULL;
+	int count = 0;
+	int ret = 0;
+	unsigned int i;
+	unsigned int irq[AMBA_NR_IRQS];
+	struct clk *clk = ERR_PTR(-ENODEV);
+	char amba_devname[100];
+
+	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
+	if (!resource)
+		goto resource_alloc_err;
+
+	presource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	resource->parent = presource;
+
+	/*
+	 * The memory address of AMD pl330 has an offset of ACPI
+	 * mem resource.
+	 */
+	resource->start += presource->start + pdata->dev_desc->base_offset;
+	resource->end = presource->end;
+
+	presource = pdev->resource;
+	for (i = 0; i < resource_size(presource); i++) {
+		if (presource[i].flags & IORESOURCE_IRQ)
+			irq[count++] = presource[i].start;
+	}
+
+	sprintf(amba_devname, "%s%s", dev_name(&pdev->dev), "DMA");
+
+	amba_dev = amba_device_alloc(amba_devname,
+				     resource->start,
+				     resource_size(resource));
+
+	if (!amba_dev)
+		goto amba_alloc_err;
+
+	amba_dev->dev.coherent_dma_mask
+		= acpi_dma_supported(ACPI_COMPANION(&pdev->dev)) ? DMA_BIT_MASK(64) : 0;
+	amba_dev->dev.fwnode = acpi_fwnode_handle(ACPI_COMPANION(&pdev->dev));
+
+	amba_dev->dev.parent = &pdev->dev;
+	amba_dev->periphid = pdata->dev_desc->periphid;
+
+	WARN_ON_ONCE(count > AMBA_NR_IRQS);
+
+	for (i = 0; i < count; i++)
+		amba_dev->irq[i] = irq[i];
+
+	clk = clk_register_fixed_rate(&amba_dev->dev,
+				      dev_name(&amba_dev->dev),
+				      NULL, CLK_IS_ROOT,
+				      pdata->dev_desc->fixed_clk_rate);
+	if (IS_ERR_OR_NULL(clk))
+		goto amba_register_err;
+
+	ret = clk_register_clkdev(clk, "apb_pclk",
+				  dev_name(&amba_dev->dev));
+	if (ret)
+		goto amba_register_err;
+
+	amba_dev->dev.init_name = NULL;
+	ret = amba_device_add(amba_dev, resource);
+	if (ret)
+		goto amba_register_err;
+
+	kfree(resource);
+	return 0;
+
+amba_register_err:
+	amba_device_put(amba_dev);
+
+amba_alloc_err:
+	kfree(resource);
+
+resource_alloc_err:
+	dev_info(&pdev->dev, "AMBA device created failed.\n");
+	return 0;
+}
+
+#else
+
+static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
+{
+	return 0;
+}
+
+#endif
+
 static struct apd_device_desc cz_i2c_desc = {
 	.setup = acpi_apd_setup,
 	.fixed_clk_rate = 133000000,
@@ -78,7 +179,10 @@ static struct apd_device_desc cz_i2c_desc = {
 
 static struct apd_device_desc cz_uart_desc = {
 	.setup = acpi_apd_setup,
+	.post_setup = acpi_apd_setup_quirks,
 	.fixed_clk_rate = 48000000,
+	.periphid = 0x00041330,
+	.base_offset = SZ_4K,
 };
 
 #else
@@ -88,11 +192,11 @@ static struct apd_device_desc cz_uart_desc = {
 #endif /* CONFIG_X86_AMD_PLATFORM_DEVICE */
 
 /**
-* Create platform device during acpi scan attach handle.
-* Return value > 0 on success of creating device.
-*/
+ * Create platform device during acpi scan attach handle.
+ * Return value > 0 on success of creating device.
+ */
 static int acpi_apd_create_device(struct acpi_device *adev,
-				   const struct acpi_device_id *id)
+				  const struct acpi_device_id *id)
 {
 	const struct apd_device_desc *dev_desc = (void *)id->driver_data;
 	struct apd_private_data *pdata;
@@ -118,14 +222,21 @@ static int acpi_apd_create_device(struct acpi_device *adev,
 	}
 
 	adev->driver_data = pdata;
-	pdev = acpi_create_platform_device(adev);
-	if (!IS_ERR_OR_NULL(pdev))
-		return 1;
 
+	pdev = acpi_create_platform_device(adev);
 	ret = PTR_ERR(pdev);
-	adev->driver_data = NULL;
+	if (IS_ERR_OR_NULL(pdev))
+		goto err_out;
+
+	if (dev_desc->post_setup) {
+		pdata->pdev = pdev;
+		dev_desc->post_setup(pdata);
+	}
+
+	return ret;
 
  err_out:
+	adev->driver_data = NULL;
 	kfree(pdata);
 	return ret;
 }
-- 
1.9.1

[PATCH 2/6] ACPI: create setup_quirk in acpi_apd

[Wang Hongcheng]

The post_setup hook of AMD0020 device will create an amba device as
the child of a previously created platform device.

Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
---
 drivers/acpi/acpi_apd.c | 131 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 121 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
index a450e7a..9520daf 100644
--- a/drivers/acpi/acpi_apd.c
+++ b/drivers/acpi/acpi_apd.c
@@ -3,7 +3,8 @@
  *
  * Copyright (c) 2014,2015 AMD Corporation.
  * Authors: Ken Xue <Ken.Xue@amd.com>
- *	Wu, Jeff <Jeff.Wu@amd.com>
+ *          Jeff Wu <15618388108@163.com>
+ *          Wang Hongcheng <Annie.Wang@amd.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -17,6 +18,8 @@
 #include <linux/acpi.h>
 #include <linux/err.h>
 #include <linux/pm.h>
+#include <linux/amba/bus.h>
+#include <linux/dma-mapping.h>
 
 #include "internal.h"
 
@@ -36,19 +39,23 @@ struct apd_private_data;
  * @fixed_clk_rate: fixed rate input clock source for acpi device;
  *			0 means no fixed rate input clock source
  * @setup: a hook routine to set device resource during create platform device
- *
+ * @post_setup: an additional hook routine
  * Device description defined as acpi_device_id.driver_data
  */
 struct apd_device_desc {
 	unsigned int flags;
 	unsigned int fixed_clk_rate;
+	unsigned int base_offset;
+	unsigned int periphid;
 	int (*setup)(struct apd_private_data *pdata);
+	int (*post_setup)(struct apd_private_data *pdata);
 };
 
 struct apd_private_data {
 	struct clk *clk;
 	struct acpi_device *adev;
 	const struct apd_device_desc *dev_desc;
+	struct platform_device *pdev;
 };
 
 #ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
@@ -71,6 +78,100 @@ static int acpi_apd_setup(struct apd_private_data *pdata)
 	return 0;
 }
 
+#ifdef CONFIG_SERIAL_8250_AMD
+static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
+{
+	struct amba_device *amba_dev = NULL;
+	struct platform_device *pdev = pdata->pdev;
+	struct resource *presource, *resource = NULL;
+	int count = 0;
+	int ret = 0;
+	unsigned int i;
+	unsigned int irq[AMBA_NR_IRQS];
+	struct clk *clk = ERR_PTR(-ENODEV);
+	char amba_devname[100];
+
+	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
+	if (!resource)
+		goto resource_alloc_err;
+
+	presource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	resource->parent = presource;
+
+	/*
+	 * The memory address of AMD pl330 has an offset of ACPI
+	 * mem resource.
+	 */
+	resource->start += presource->start + pdata->dev_desc->base_offset;
+	resource->end = presource->end;
+
+	presource = pdev->resource;
+	for (i = 0; i < resource_size(presource); i++) {
+		if (presource[i].flags & IORESOURCE_IRQ)
+			irq[count++] = presource[i].start;
+	}
+
+	sprintf(amba_devname, "%s%s", dev_name(&pdev->dev), "DMA");
+
+	amba_dev = amba_device_alloc(amba_devname,
+				     resource->start,
+				     resource_size(resource));
+
+	if (!amba_dev)
+		goto amba_alloc_err;
+
+	amba_dev->dev.coherent_dma_mask
+		= acpi_dma_supported(ACPI_COMPANION(&pdev->dev)) ? DMA_BIT_MASK(64) : 0;
+	amba_dev->dev.fwnode = acpi_fwnode_handle(ACPI_COMPANION(&pdev->dev));
+
+	amba_dev->dev.parent = &pdev->dev;
+	amba_dev->periphid = pdata->dev_desc->periphid;
+
+	WARN_ON_ONCE(count > AMBA_NR_IRQS);
+
+	for (i = 0; i < count; i++)
+		amba_dev->irq[i] = irq[i];
+
+	clk = clk_register_fixed_rate(&amba_dev->dev,
+				      dev_name(&amba_dev->dev),
+				      NULL, CLK_IS_ROOT,
+				      pdata->dev_desc->fixed_clk_rate);
+	if (IS_ERR_OR_NULL(clk))
+		goto amba_register_err;
+
+	ret = clk_register_clkdev(clk, "apb_pclk",
+				  dev_name(&amba_dev->dev));
+	if (ret)
+		goto amba_register_err;
+
+	amba_dev->dev.init_name = NULL;
+	ret = amba_device_add(amba_dev, resource);
+	if (ret)
+		goto amba_register_err;
+
+	kfree(resource);
+	return 0;
+
+amba_register_err:
+	amba_device_put(amba_dev);
+
+amba_alloc_err:
+	kfree(resource);
+
+resource_alloc_err:
+	dev_info(&pdev->dev, "AMBA device created failed.\n");
+	return 0;
+}
+
+#else
+
+static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
+{
+	return 0;
+}
+
+#endif
+
 static struct apd_device_desc cz_i2c_desc = {
 	.setup = acpi_apd_setup,
 	.fixed_clk_rate = 133000000,
@@ -78,7 +179,10 @@ static struct apd_device_desc cz_i2c_desc = {
 
 static struct apd_device_desc cz_uart_desc = {
 	.setup = acpi_apd_setup,
+	.post_setup = acpi_apd_setup_quirks,
 	.fixed_clk_rate = 48000000,
+	.periphid = 0x00041330,
+	.base_offset = SZ_4K,
 };
 
 #else
@@ -88,11 +192,11 @@ static struct apd_device_desc cz_uart_desc = {
 #endif /* CONFIG_X86_AMD_PLATFORM_DEVICE */
 
 /**
-* Create platform device during acpi scan attach handle.
-* Return value > 0 on success of creating device.
-*/
+ * Create platform device during acpi scan attach handle.
+ * Return value > 0 on success of creating device.
+ */
 static int acpi_apd_create_device(struct acpi_device *adev,
-				   const struct acpi_device_id *id)
+				  const struct acpi_device_id *id)
 {
 	const struct apd_device_desc *dev_desc = (void *)id->driver_data;
 	struct apd_private_data *pdata;
@@ -118,14 +222,21 @@ static int acpi_apd_create_device(struct acpi_device *adev,
 	}
 
 	adev->driver_data = pdata;
-	pdev = acpi_create_platform_device(adev);
-	if (!IS_ERR_OR_NULL(pdev))
-		return 1;
 
+	pdev = acpi_create_platform_device(adev);
 	ret = PTR_ERR(pdev);
-	adev->driver_data = NULL;
+	if (IS_ERR_OR_NULL(pdev))
+		goto err_out;
+
+	if (dev_desc->post_setup) {
+		pdata->pdev = pdev;
+		dev_desc->post_setup(pdata);
+	}
+
+	return ret;
 
  err_out:
+	adev->driver_data = NULL;
 	kfree(pdata);
 	return ret;
 }
-- 
1.9.1

Re: [PATCH 2/6] ACPI: create setup_quirk in acpi_apd

[Andy Shevchenko]

On Mon, Jan 4, 2016 at 7:31 AM, Wang Hongcheng <annie.wang@amd.com> wrote:
> The post_setup hook of AMD0020 device will create an amba device as
> the child of a previously created platform device.
>
> Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
> ---
>  drivers/acpi/acpi_apd.c | 131 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 121 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> index a450e7a..9520daf 100644
> --- a/drivers/acpi/acpi_apd.c
> +++ b/drivers/acpi/acpi_apd.c
> @@ -3,7 +3,8 @@
>   *
>   * Copyright (c) 2014,2015 AMD Corporation.
>   * Authors: Ken Xue <Ken.Xue@amd.com>
> - *     Wu, Jeff <Jeff.Wu@amd.com>
> + *          Jeff Wu <15618388108@163.com>
> + *          Wang Hongcheng <Annie.Wang@amd.com>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -17,6 +18,8 @@
>  #include <linux/acpi.h>
>  #include <linux/err.h>
>  #include <linux/pm.h>
> +#include <linux/amba/bus.h>
> +#include <linux/dma-mapping.h>
>
>  #include "internal.h"
>
> @@ -36,19 +39,23 @@ struct apd_private_data;
>   * @fixed_clk_rate: fixed rate input clock source for acpi device;
>   *                     0 means no fixed rate input clock source
>   * @setup: a hook routine to set device resource during create platform device
> - *
> + * @post_setup: an additional hook routine
>   * Device description defined as acpi_device_id.driver_data
>   */
>  struct apd_device_desc {
>         unsigned int flags;
>         unsigned int fixed_clk_rate;
> +       unsigned int base_offset;
> +       unsigned int periphid;
>         int (*setup)(struct apd_private_data *pdata);
> +       int (*post_setup)(struct apd_private_data *pdata);
>  };
>
>  struct apd_private_data {
>         struct clk *clk;
>         struct acpi_device *adev;
>         const struct apd_device_desc *dev_desc;
> +       struct platform_device *pdev;
>  };
>
>  #ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> @@ -71,6 +78,100 @@ static int acpi_apd_setup(struct apd_private_data *pdata)
>         return 0;
>  }
>
> +#ifdef CONFIG_SERIAL_8250_AMD
> +static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
> +{
> +       struct amba_device *amba_dev = NULL;
> +       struct platform_device *pdev = pdata->pdev;
> +       struct resource *presource, *resource = NULL;
> +       int count = 0;
> +       int ret = 0;
> +       unsigned int i;
> +       unsigned int irq[AMBA_NR_IRQS];
> +       struct clk *clk = ERR_PTR(-ENODEV);
> +       char amba_devname[100];
> +
> +       resource = kzalloc(sizeof(*resource), GFP_KERNEL);
> +       if (!resource)
> +               goto resource_alloc_err;
> +
> +       presource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       resource->parent = presource;
> +
> +       /*
> +        * The memory address of AMD pl330 has an offset of ACPI
> +        * mem resource.
> +        */
> +       resource->start += presource->start + pdata->dev_desc->base_offset;
> +       resource->end = presource->end;
> +
> +       presource = pdev->resource;
> +       for (i = 0; i < resource_size(presource); i++) {
> +               if (presource[i].flags & IORESOURCE_IRQ)
> +                       irq[count++] = presource[i].start;
> +       }
> +
> +       sprintf(amba_devname, "%s%s", dev_name(&pdev->dev), "DMA");
> +
> +       amba_dev = amba_device_alloc(amba_devname,
> +                                    resource->start,
> +                                    resource_size(resource));
> +
> +       if (!amba_dev)
> +               goto amba_alloc_err;
> +

struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);

?

> +       amba_dev->dev.coherent_dma_mask
> +               = acpi_dma_supported(ACPI_COMPANION(&pdev->dev)) ? DMA_BIT_MASK(64) : 0;
> +       amba_dev->dev.fwnode = acpi_fwnode_handle(ACPI_COMPANION(&pdev->dev));

^^^

> +
> +       amba_dev->dev.parent = &pdev->dev;
> +       amba_dev->periphid = pdata->dev_desc->periphid;
> +
> +       WARN_ON_ONCE(count > AMBA_NR_IRQS);
> +
> +       for (i = 0; i < count; i++)
> +               amba_dev->irq[i] = irq[i];

memcpy() ?

> +
> +       clk = clk_register_fixed_rate(&amba_dev->dev,
> +                                     dev_name(&amba_dev->dev),
> +                                     NULL, CLK_IS_ROOT,
> +                                     pdata->dev_desc->fixed_clk_rate);
> +       if (IS_ERR_OR_NULL(clk))
> +               goto amba_register_err;
> +
> +       ret = clk_register_clkdev(clk, "apb_pclk",
> +                                 dev_name(&amba_dev->dev));

clkdev_create()?

Or mention that driver shall not be unloaded, otherwise it will leak resources.

> +       if (ret)
> +               goto amba_register_err;
> +
> +       amba_dev->dev.init_name = NULL;
> +       ret = amba_device_add(amba_dev, resource);
> +       if (ret)
> +               goto amba_register_err;
> +
> +       kfree(resource);
> +       return 0;
> +
> +amba_register_err:
> +       amba_device_put(amba_dev);
> +
> +amba_alloc_err:
> +       kfree(resource);
> +
> +resource_alloc_err:
> +       dev_info(&pdev->dev, "AMBA device created failed.\n");

dev_err();

> +       return 0;

return ret;

> +}
> +
> +#else
> +
> +static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
> +{
> +       return 0;
> +}
> +
> +#endif
> +
>  static struct apd_device_desc cz_i2c_desc = {
>         .setup = acpi_apd_setup,
>         .fixed_clk_rate = 133000000,
> @@ -78,7 +179,10 @@ static struct apd_device_desc cz_i2c_desc = {
>
>  static struct apd_device_desc cz_uart_desc = {
>         .setup = acpi_apd_setup,
> +       .post_setup = acpi_apd_setup_quirks,
>         .fixed_clk_rate = 48000000,
> +       .periphid = 0x00041330,
> +       .base_offset = SZ_4K,
>  };
>
>  #else
> @@ -88,11 +192,11 @@ static struct apd_device_desc cz_uart_desc = {
>  #endif /* CONFIG_X86_AMD_PLATFORM_DEVICE */
>
>  /**
> -* Create platform device during acpi scan attach handle.
> -* Return value > 0 on success of creating device.
> -*/
> + * Create platform device during acpi scan attach handle.
> + * Return value > 0 on success of creating device.
> + */

Separate change, doesn't belong to the topic.

>  static int acpi_apd_create_device(struct acpi_device *adev,
> -                                  const struct acpi_device_id *id)
> +                                 const struct acpi_device_id *id)

Ditto

>  {
>         const struct apd_device_desc *dev_desc = (void *)id->driver_data;
>         struct apd_private_data *pdata;
> @@ -118,14 +222,21 @@ static int acpi_apd_create_device(struct acpi_device *adev,
>         }
>
>         adev->driver_data = pdata;
> -       pdev = acpi_create_platform_device(adev);
> -       if (!IS_ERR_OR_NULL(pdev))
> -               return 1;
>
> +       pdev = acpi_create_platform_device(adev);
>         ret = PTR_ERR(pdev);
> -       adev->driver_data = NULL;
> +       if (IS_ERR_OR_NULL(pdev))
> +               goto err_out;
> +
> +       if (dev_desc->post_setup) {
> +               pdata->pdev = pdev;
> +               dev_desc->post_setup(pdata);
> +       }
> +
> +       return ret;
>
>   err_out:
> +       adev->driver_data = NULL;
>         kfree(pdata);
>         return ret;
>  }
> --
> 1.9.1
>



-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/6] ACPI: add 2 parameters to function acpi dma controller register

[Wang Hongcheng]

As CSRT table will not always work, 2 arguments, base_request_line
and num are added to acpi dma controller register
as another way to get device request line.

Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
---
 drivers/dma/acpi-dma.c    | 25 ++++++++++++++++++++-----
 drivers/dma/dw/platform.c |  2 +-
 include/linux/acpi_dma.h  |  6 ++++++
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/acpi-dma.c b/drivers/dma/acpi-dma.c
index 16d0daa..e2c27c3 100644
--- a/drivers/dma/acpi-dma.c
+++ b/drivers/dma/acpi-dma.c
@@ -105,7 +105,7 @@ static int acpi_dma_parse_resource_group(const struct acpi_csrt_group *grp,
  * We are using this table to get the request line range of the specific DMA
  * controller to be used later.
  */
-static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
+static int acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
 {
 	struct acpi_csrt_group *grp, *end;
 	struct acpi_table_csrt *csrt;
@@ -117,7 +117,7 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
 	if (ACPI_FAILURE(status)) {
 		if (status != AE_NOT_FOUND)
 			dev_warn(&adev->dev, "failed to get the CSRT table\n");
-		return;
+		return -ENOENT;
 	}
 
 	grp = (struct acpi_csrt_group *)(csrt + 1);
@@ -128,11 +128,12 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
 		if (ret < 0) {
 			dev_warn(&adev->dev,
 				 "error in parsing resource group\n");
-			return;
+			return -EINVAL;
 		}
 
 		grp = (struct acpi_csrt_group *)((void *)grp + grp->length);
 	}
+	return 0;
 }
 
 /**
@@ -140,6 +141,8 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
  * @dev:		struct device of DMA controller
  * @acpi_dma_xlate:	translation function which converts a dma specifier
  *			into a dma_chan structure
+ * @base_request_line:  device request line base
+ * @num:                device request line range
  * @data		pointer to controller specific data to be used by
  *			translation function
  *
@@ -152,10 +155,13 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
 int acpi_dma_controller_register(struct device *dev,
 		struct dma_chan *(*acpi_dma_xlate)
 		(struct acpi_dma_spec *, struct acpi_dma *),
+		unsigned short base_request_line,
+		unsigned short num,
 		void *data)
 {
 	struct acpi_device *adev;
 	struct acpi_dma	*adma;
+	int ret;
 
 	if (!dev || !acpi_dma_xlate)
 		return -EINVAL;
@@ -173,7 +179,12 @@ int acpi_dma_controller_register(struct device *dev,
 	adma->acpi_dma_xlate = acpi_dma_xlate;
 	adma->data = data;
 
-	acpi_dma_parse_csrt(adev, adma);
+	ret = acpi_dma_parse_csrt(adev, adma);
+
+	if (ret < 0) {
+		adma->base_request_line = base_request_line;
+		adma->end_request_line = base_request_line + num;
+	}
 
 	/* Now queue acpi_dma controller structure in list */
 	mutex_lock(&acpi_dma_lock);
@@ -236,6 +247,8 @@ static void devm_acpi_dma_release(struct device *dev, void *res)
 int devm_acpi_dma_controller_register(struct device *dev,
 		struct dma_chan *(*acpi_dma_xlate)
 		(struct acpi_dma_spec *, struct acpi_dma *),
+		unsigned short base_request_line,
+		unsigned short num,
 		void *data)
 {
 	void *res;
@@ -245,7 +258,9 @@ int devm_acpi_dma_controller_register(struct device *dev,
 	if (!res)
 		return -ENOMEM;
 
-	ret = acpi_dma_controller_register(dev, acpi_dma_xlate, data);
+	ret = acpi_dma_controller_register(dev, acpi_dma_xlate,
+					   base_request_line,
+					   num, data);
 	if (ret) {
 		devres_free(res);
 		return ret;
diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index 68a4815..54b898d 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -88,7 +88,7 @@ static void dw_dma_acpi_controller_register(struct dw_dma *dw)
 	info->filter_fn = dw_dma_acpi_filter;
 
 	ret = devm_acpi_dma_controller_register(dev, acpi_dma_simple_xlate,
-						info);
+						0, 0, info);
 	if (ret)
 		dev_err(dev, "could not register acpi_dma_controller\n");
 }
diff --git a/include/linux/acpi_dma.h b/include/linux/acpi_dma.h
index 329436d..d90febe 100644
--- a/include/linux/acpi_dma.h
+++ b/include/linux/acpi_dma.h
@@ -62,11 +62,15 @@ struct acpi_dma_filter_info {
 int acpi_dma_controller_register(struct device *dev,
 		struct dma_chan *(*acpi_dma_xlate)
 		(struct acpi_dma_spec *, struct acpi_dma *),
+		unsigned short base_request_line,
+		unsigned short num,
 		void *data);
 int acpi_dma_controller_free(struct device *dev);
 int devm_acpi_dma_controller_register(struct device *dev,
 		struct dma_chan *(*acpi_dma_xlate)
 		(struct acpi_dma_spec *, struct acpi_dma *),
+		unsigned short base_request_line,
+		unsigned short num,
 		void *data);
 void devm_acpi_dma_controller_free(struct device *dev);
 
@@ -82,6 +86,8 @@ struct dma_chan *acpi_dma_simple_xlate(struct acpi_dma_spec *dma_spec,
 static inline int acpi_dma_controller_register(struct device *dev,
 		struct dma_chan *(*acpi_dma_xlate)
 		(struct acpi_dma_spec *, struct acpi_dma *),
+		unsigned short base_request_line,
+		unsigned short num,
 		void *data)
 {
 	return -ENODEV;
-- 
1.9.1

[PATCH 3/6] ACPI: add 2 parameters to function acpi dma controller register

[Wang Hongcheng]

As CSRT table will not always work, 2 arguments, base_request_line
and num are added to acpi dma controller register
as another way to get device request line.

Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
---
 drivers/dma/acpi-dma.c    | 25 ++++++++++++++++++++-----
 drivers/dma/dw/platform.c |  2 +-
 include/linux/acpi_dma.h  |  6 ++++++
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/acpi-dma.c b/drivers/dma/acpi-dma.c
index 16d0daa..e2c27c3 100644
--- a/drivers/dma/acpi-dma.c
+++ b/drivers/dma/acpi-dma.c
@@ -105,7 +105,7 @@ static int acpi_dma_parse_resource_group(const struct acpi_csrt_group *grp,
  * We are using this table to get the request line range of the specific DMA
  * controller to be used later.
  */
-static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
+static int acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
 {
 	struct acpi_csrt_group *grp, *end;
 	struct acpi_table_csrt *csrt;
@@ -117,7 +117,7 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
 	if (ACPI_FAILURE(status)) {
 		if (status != AE_NOT_FOUND)
 			dev_warn(&adev->dev, "failed to get the CSRT table\n");
-		return;
+		return -ENOENT;
 	}
 
 	grp = (struct acpi_csrt_group *)(csrt + 1);
@@ -128,11 +128,12 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
 		if (ret < 0) {
 			dev_warn(&adev->dev,
 				 "error in parsing resource group\n");
-			return;
+			return -EINVAL;
 		}
 
 		grp = (struct acpi_csrt_group *)((void *)grp + grp->length);
 	}
+	return 0;
 }
 
 /**
@@ -140,6 +141,8 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
  * @dev:		struct device of DMA controller
  * @acpi_dma_xlate:	translation function which converts a dma specifier
  *			into a dma_chan structure
+ * @base_request_line:  device request line base
+ * @num:                device request line range
  * @data		pointer to controller specific data to be used by
  *			translation function
  *
@@ -152,10 +155,13 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
 int acpi_dma_controller_register(struct device *dev,
 		struct dma_chan *(*acpi_dma_xlate)
 		(struct acpi_dma_spec *, struct acpi_dma *),
+		unsigned short base_request_line,
+		unsigned short num,
 		void *data)
 {
 	struct acpi_device *adev;
 	struct acpi_dma	*adma;
+	int ret;
 
 	if (!dev || !acpi_dma_xlate)
 		return -EINVAL;
@@ -173,7 +179,12 @@ int acpi_dma_controller_register(struct device *dev,
 	adma->acpi_dma_xlate = acpi_dma_xlate;
 	adma->data = data;
 
-	acpi_dma_parse_csrt(adev, adma);
+	ret = acpi_dma_parse_csrt(adev, adma);
+
+	if (ret < 0) {
+		adma->base_request_line = base_request_line;
+		adma->end_request_line = base_request_line + num;
+	}
 
 	/* Now queue acpi_dma controller structure in list */
 	mutex_lock(&acpi_dma_lock);
@@ -236,6 +247,8 @@ static void devm_acpi_dma_release(struct device *dev, void *res)
 int devm_acpi_dma_controller_register(struct device *dev,
 		struct dma_chan *(*acpi_dma_xlate)
 		(struct acpi_dma_spec *, struct acpi_dma *),
+		unsigned short base_request_line,
+		unsigned short num,
 		void *data)
 {
 	void *res;
@@ -245,7 +258,9 @@ int devm_acpi_dma_controller_register(struct device *dev,
 	if (!res)
 		return -ENOMEM;
 
-	ret = acpi_dma_controller_register(dev, acpi_dma_xlate, data);
+	ret = acpi_dma_controller_register(dev, acpi_dma_xlate,
+					   base_request_line,
+					   num, data);
 	if (ret) {
 		devres_free(res);
 		return ret;
diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index 68a4815..54b898d 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -88,7 +88,7 @@ static void dw_dma_acpi_controller_register(struct dw_dma *dw)
 	info->filter_fn = dw_dma_acpi_filter;
 
 	ret = devm_acpi_dma_controller_register(dev, acpi_dma_simple_xlate,
-						info);
+						0, 0, info);
 	if (ret)
 		dev_err(dev, "could not register acpi_dma_controller\n");
 }
diff --git a/include/linux/acpi_dma.h b/include/linux/acpi_dma.h
index 329436d..d90febe 100644
--- a/include/linux/acpi_dma.h
+++ b/include/linux/acpi_dma.h
@@ -62,11 +62,15 @@ struct acpi_dma_filter_info {
 int acpi_dma_controller_register(struct device *dev,
 		struct dma_chan *(*acpi_dma_xlate)
 		(struct acpi_dma_spec *, struct acpi_dma *),
+		unsigned short base_request_line,
+		unsigned short num,
 		void *data);
 int acpi_dma_controller_free(struct device *dev);
 int devm_acpi_dma_controller_register(struct device *dev,
 		struct dma_chan *(*acpi_dma_xlate)
 		(struct acpi_dma_spec *, struct acpi_dma *),
+		unsigned short base_request_line,
+		unsigned short num,
 		void *data);
 void devm_acpi_dma_controller_free(struct device *dev);
 
@@ -82,6 +86,8 @@ struct dma_chan *acpi_dma_simple_xlate(struct acpi_dma_spec *dma_spec,
 static inline int acpi_dma_controller_register(struct device *dev,
 		struct dma_chan *(*acpi_dma_xlate)
 		(struct acpi_dma_spec *, struct acpi_dma *),
+		unsigned short base_request_line,
+		unsigned short num,
 		void *data)
 {
 	return -ENODEV;
-- 
1.9.1

Re: [PATCH 3/6] ACPI: add 2 parameters to function acpi dma controller register

[Andy Shevchenko]

On Mon, Jan 4, 2016 at 7:31 AM, Wang Hongcheng <annie.wang@amd.com> wrote:
> As CSRT table will not always work, 2 arguments, base_request_line
> and num are added to acpi dma controller register
> as another way to get device request line.

Please, check my comments below.

>
> Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
> ---
>  drivers/dma/acpi-dma.c    | 25 ++++++++++++++++++++-----
>  drivers/dma/dw/platform.c |  2 +-
>  include/linux/acpi_dma.h  |  6 ++++++
>  3 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma/acpi-dma.c b/drivers/dma/acpi-dma.c
> index 16d0daa..e2c27c3 100644
> --- a/drivers/dma/acpi-dma.c
> +++ b/drivers/dma/acpi-dma.c
> @@ -105,7 +105,7 @@ static int acpi_dma_parse_resource_group(const struct acpi_csrt_group *grp,
>   * We are using this table to get the request line range of the specific DMA
>   * controller to be used later.
>   */
> -static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
> +static int acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
>  {
>         struct acpi_csrt_group *grp, *end;
>         struct acpi_table_csrt *csrt;
> @@ -117,7 +117,7 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
>         if (ACPI_FAILURE(status)) {
>                 if (status != AE_NOT_FOUND)
>                         dev_warn(&adev->dev, "failed to get the CSRT table\n");
> -               return;
> +               return -ENOENT;
>         }
>
>         grp = (struct acpi_csrt_group *)(csrt + 1);
> @@ -128,11 +128,12 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
>                 if (ret < 0) {
>                         dev_warn(&adev->dev,
>                                  "error in parsing resource group\n");
> -                       return;
> +                       return -EINVAL;
>                 }
>
>                 grp = (struct acpi_csrt_group *)((void *)grp + grp->length);
>         }
> +       return 0;
>  }
>

Good.

>  /**
> @@ -140,6 +141,8 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
>   * @dev:               struct device of DMA controller
>   * @acpi_dma_xlate:    translation function which converts a dma specifier
>   *                     into a dma_chan structure
> + * @base_request_line:  device request line base
> + * @num:                device request line range

Not ok.

>   * @data               pointer to controller specific data to be used by
>   *                     translation function
>   *
> @@ -152,10 +155,13 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
>  int acpi_dma_controller_register(struct device *dev,
>                 struct dma_chan *(*acpi_dma_xlate)
>                 (struct acpi_dma_spec *, struct acpi_dma *),
> +               unsigned short base_request_line,
> +               unsigned short num,

Add them after void *data

>                 void *data)

>  {
>         struct acpi_device *adev;
>         struct acpi_dma *adma;
> +       int ret;
>
>         if (!dev || !acpi_dma_xlate)
>                 return -EINVAL;
> @@ -173,7 +179,12 @@ int acpi_dma_controller_register(struct device *dev,
>         adma->acpi_dma_xlate = acpi_dma_xlate;
>         adma->data = data;
>
> -       acpi_dma_parse_csrt(adev, adma);
> +       ret = acpi_dma_parse_csrt(adev, adma);
> +

> +       if (ret < 0) {
> +               adma->base_request_line = base_request_line;
> +               adma->end_request_line = base_request_line + num;
> +       }

No need to assign parameters there.
Create a new function and call it as a fallback of acpi_dma_parse_csrt();

ret = acpi_dma_parse_csrt();
if (ret)
 acpi_dma_assign_request_line();

> @@ -236,6 +247,8 @@ static void devm_acpi_dma_release(struct device *dev, void *res)
>  int devm_acpi_dma_controller_register(struct device *dev,
>                 struct dma_chan *(*acpi_dma_xlate)
>                 (struct acpi_dma_spec *, struct acpi_dma *),
> +               unsigned short base_request_line,
> +               unsigned short num,

After *data.

>                 void *data)
>  {
>         void *res;
> @@ -245,7 +258,9 @@ int devm_acpi_dma_controller_register(struct device *dev,
>         if (!res)
>                 return -ENOMEM;
>
> -       ret = acpi_dma_controller_register(dev, acpi_dma_xlate, data);
> +       ret = acpi_dma_controller_register(dev, acpi_dma_xlate,
> +                                          base_request_line,
> +                                          num, data);

Ditto.

>         if (ret) {
>                 devres_free(res);
>                 return ret;
> diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
> index 68a4815..54b898d 100644
> --- a/drivers/dma/dw/platform.c
> +++ b/drivers/dma/dw/platform.c
> @@ -88,7 +88,7 @@ static void dw_dma_acpi_controller_register(struct dw_dma *dw)
>         info->filter_fn = dw_dma_acpi_filter;
>
>         ret = devm_acpi_dma_controller_register(dev, acpi_dma_simple_xlate,
> -                                               info);
> +                                               0, 0, info);

Ditto.

>         if (ret)
>                 dev_err(dev, "could not register acpi_dma_controller\n");
>  }
> diff --git a/include/linux/acpi_dma.h b/include/linux/acpi_dma.h
> index 329436d..d90febe 100644
> --- a/include/linux/acpi_dma.h
> +++ b/include/linux/acpi_dma.h
> @@ -62,11 +62,15 @@ struct acpi_dma_filter_info {
>  int acpi_dma_controller_register(struct device *dev,
>                 struct dma_chan *(*acpi_dma_xlate)
>                 (struct acpi_dma_spec *, struct acpi_dma *),
> +               unsigned short base_request_line,
> +               unsigned short num,

Ditto.

>                 void *data);
>  int acpi_dma_controller_free(struct device *dev);
>  int devm_acpi_dma_controller_register(struct device *dev,
>                 struct dma_chan *(*acpi_dma_xlate)
>                 (struct acpi_dma_spec *, struct acpi_dma *),
> +               unsigned short base_request_line,
> +               unsigned short num,

Ditto.

>                 void *data);
>  void devm_acpi_dma_controller_free(struct device *dev);
>
> @@ -82,6 +86,8 @@ struct dma_chan *acpi_dma_simple_xlate(struct acpi_dma_spec *dma_spec,
>  static inline int acpi_dma_controller_register(struct device *dev,
>                 struct dma_chan *(*acpi_dma_xlate)
>                 (struct acpi_dma_spec *, struct acpi_dma *),
> +               unsigned short base_request_line,
> +               unsigned short num,
>                 void *data)

Ditto.

>  {
>         return -ENODEV;
> --
> 1.9.1
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/6] ACPI: add 2 parameters to function acpi dma controller register

[Mika Westerberg]

On Mon, Jan 04, 2016 at 01:31:38PM +0800, Wang Hongcheng wrote:
> As CSRT table will not always work, 2 arguments, base_request_line
> and num are added to acpi dma controller register
> as another way to get device request line.
> 
> Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
> ---
>  drivers/dma/acpi-dma.c    | 25 ++++++++++++++++++++-----
>  drivers/dma/dw/platform.c |  2 +-
>  include/linux/acpi_dma.h  |  6 ++++++
>  3 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma/acpi-dma.c b/drivers/dma/acpi-dma.c
> index 16d0daa..e2c27c3 100644
> --- a/drivers/dma/acpi-dma.c
> +++ b/drivers/dma/acpi-dma.c
> @@ -105,7 +105,7 @@ static int acpi_dma_parse_resource_group(const struct acpi_csrt_group *grp,
>   * We are using this table to get the request line range of the specific DMA
>   * controller to be used later.
>   */
> -static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
> +static int acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
>  {
>  	struct acpi_csrt_group *grp, *end;
>  	struct acpi_table_csrt *csrt;
> @@ -117,7 +117,7 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
>  	if (ACPI_FAILURE(status)) {
>  		if (status != AE_NOT_FOUND)
>  			dev_warn(&adev->dev, "failed to get the CSRT table\n");
> -		return;
> +		return -ENOENT;
>  	}
>  
>  	grp = (struct acpi_csrt_group *)(csrt + 1);
> @@ -128,11 +128,12 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
>  		if (ret < 0) {
>  			dev_warn(&adev->dev,
>  				 "error in parsing resource group\n");
> -			return;
> +			return -EINVAL;
>  		}
>  
>  		grp = (struct acpi_csrt_group *)((void *)grp + grp->length);
>  	}
> +	return 0;
>  }
>  
>  /**
> @@ -140,6 +141,8 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
>   * @dev:		struct device of DMA controller
>   * @acpi_dma_xlate:	translation function which converts a dma specifier
>   *			into a dma_chan structure
> + * @base_request_line:  device request line base
> + * @num:                device request line range
>   * @data		pointer to controller specific data to be used by
>   *			translation function
>   *
> @@ -152,10 +155,13 @@ static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
>  int acpi_dma_controller_register(struct device *dev,
>  		struct dma_chan *(*acpi_dma_xlate)
>  		(struct acpi_dma_spec *, struct acpi_dma *),
> +		unsigned short base_request_line,
> +		unsigned short num,
>  		void *data)

Can you instead provide custom acpi_dma_xlate when you register the DMA
controller? That function can then use whatever translation scheme is
suitable for your platform to get the correct channel.

RE: [PATCH 3/6] ACPI: add 2 parameters to function acpi dma controller register

[Wang, Annie]

Hi Mika,

>-----Original Message-----
>From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
>Sent: Monday, January 04, 2016 10:46 PM
>To: Wang, Annie
>Cc: Andy Shevchenko; Vinod Koul; Greg Kroah-Hartman; Rafael J. Wysocki; linux-
>acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org;
>dmaengine@vger.kernel.org; Borislav Petkov; Huang, Ray; Wan, Vincent; Xue,
>Ken; Robin Murphy; Graeme Gregory; Li, Tony; Yu, Xiangliang
>Subject: Re: [PATCH 3/6] ACPI: add 2 parameters to function acpi dma controller
>register
>
>On Mon, Jan 04, 2016 at 01:31:38PM +0800, Wang Hongcheng wrote:
>> As CSRT table will not always work, 2 arguments, base_request_line and
>> num are added to acpi dma controller register as another way to get
>> device request line.
>>
>> Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
>> ---
>>  drivers/dma/acpi-dma.c    | 25 ++++++++++++++++++++-----
>>  drivers/dma/dw/platform.c |  2 +-
>>  include/linux/acpi_dma.h  |  6 ++++++
>>  3 files changed, 27 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/dma/acpi-dma.c b/drivers/dma/acpi-dma.c index
>> 16d0daa..e2c27c3 100644
>> --- a/drivers/dma/acpi-dma.c
>> +++ b/drivers/dma/acpi-dma.c
>> @@ -105,7 +105,7 @@ static int acpi_dma_parse_resource_group(const struct
>acpi_csrt_group *grp,
>>   * We are using this table to get the request line range of the specific DMA
>>   * controller to be used later.
>>   */
>> -static void acpi_dma_parse_csrt(struct acpi_device *adev, struct
>> acpi_dma *adma)
>> +static int acpi_dma_parse_csrt(struct acpi_device *adev, struct
>> +acpi_dma *adma)
>>  {
>>  	struct acpi_csrt_group *grp, *end;
>>  	struct acpi_table_csrt *csrt;
>> @@ -117,7 +117,7 @@ static void acpi_dma_parse_csrt(struct acpi_device
>*adev, struct acpi_dma *adma)
>>  	if (ACPI_FAILURE(status)) {
>>  		if (status != AE_NOT_FOUND)
>>  			dev_warn(&adev->dev, "failed to get the CSRT table\n");
>> -		return;
>> +		return -ENOENT;
>>  	}
>>
>>  	grp = (struct acpi_csrt_group *)(csrt + 1); @@ -128,11 +128,12 @@
>> static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma
>*adma)
>>  		if (ret < 0) {
>>  			dev_warn(&adev->dev,
>>  				 "error in parsing resource group\n");
>> -			return;
>> +			return -EINVAL;
>>  		}
>>
>>  		grp = (struct acpi_csrt_group *)((void *)grp + grp->length);
>>  	}
>> +	return 0;
>>  }
>>
>>  /**
>> @@ -140,6 +141,8 @@ static void acpi_dma_parse_csrt(struct acpi_device
>*adev, struct acpi_dma *adma)
>>   * @dev:		struct device of DMA controller
>>   * @acpi_dma_xlate:	translation function which converts a dma specifier
>>   *			into a dma_chan structure
>> + * @base_request_line:  device request line base
>> + * @num:                device request line range
>>   * @data		pointer to controller specific data to be used by
>>   *			translation function
>>   *
>> @@ -152,10 +155,13 @@ static void acpi_dma_parse_csrt(struct
>> acpi_device *adev, struct acpi_dma *adma)  int
>acpi_dma_controller_register(struct device *dev,
>>  		struct dma_chan *(*acpi_dma_xlate)
>>  		(struct acpi_dma_spec *, struct acpi_dma *),
>> +		unsigned short base_request_line,
>> +		unsigned short num,
>>  		void *data)
>
>Can you instead provide custom acpi_dma_xlate when you register the DMA
>controller? That function can then use whatever translation scheme is suitable for
>your platform to get the correct channel.

Acpi_dma_xlate  mainly converts an  acpi_dma  structure into a dma_chan structure.
Without proper request line, wrong acpi_dma structure may be get. 

And as Andy says in http://article.gmane.org/gmane.linux.kernel.iommu/11675 
How about add a hook into acpi_dma_controller_register?

Regards,
Hongcheng (Annie)

Re: [PATCH 3/6] ACPI: add 2 parameters to function acpi dma controller register

[Mika Westerberg]

On Wed, Jan 06, 2016 at 06:46:37AM +0000, Wang, Annie wrote:
> Acpi_dma_xlate  mainly converts an  acpi_dma  structure into a dma_chan structure.
> Without proper request line, wrong acpi_dma structure may be get. 
>
> And as Andy says in http://article.gmane.org/gmane.linux.kernel.iommu/11675 
> How about add a hook into acpi_dma_controller_register?

Hmm, you can pass custom data in acpi_dma_controller_register() which
will be assigned to adma->data. If you put your request line
information there, I think your custom acpi_dma_xlate() should be able
to use that information to find proper channel, no?

Re: [PATCH 3/6] ACPI: add 2 parameters to function acpi dma controller register

[Andy Shevchenko]

On Thu, Jan 7, 2016 at 11:52 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Jan 06, 2016 at 06:46:37AM +0000, Wang, Annie wrote:
>> Acpi_dma_xlate  mainly converts an  acpi_dma  structure into a dma_chan structure.
>> Without proper request line, wrong acpi_dma structure may be get.
>>
>> And as Andy says in http://article.gmane.org/gmane.linux.kernel.iommu/11675
>> How about add a hook into acpi_dma_controller_register?
>
> Hmm, you can pass custom data in acpi_dma_controller_register() which
> will be assigned to adma->data. If you put your request line
> information there, I think your custom acpi_dma_xlate() should be able
> to use that information to find proper channel, no?

I like Mika's idea, so, please, go this way if there is no impediment.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 4/6] dmaengine: pl330: add new items for pl330 private data

[Wang Hongcheng]

mcbuf_sz means bytes to allocate for MicroCode buffer.
flags is for irq sharing, default is non-shared, in AMD
Carrizo, pl330 shares IRQ with its corresponding UART device.

Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
---
 drivers/acpi/acpi_apd.c    | 17 +++++++++++++++++
 drivers/dma/pl330.c        |  9 ++++++++-
 include/linux/amba/pl330.h |  2 ++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
index 9520daf..4d71a65 100644
--- a/drivers/acpi/acpi_apd.c
+++ b/drivers/acpi/acpi_apd.c
@@ -20,6 +20,9 @@
 #include <linux/pm.h>
 #include <linux/amba/bus.h>
 #include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/interrupt.h>
+#include <linux/amba/pl330.h>
 
 #include "internal.h"
 
@@ -33,6 +36,14 @@ struct apd_private_data;
 #define ACPI_APD_SYSFS	BIT(0)
 #define ACPI_APD_PM	BIT(1)
 
+static u8 peri_id[2] = { 0, 1 };
+
+static struct dma_pl330_platdata amd_pl330 = {
+	.nr_valid_peri = 2,
+	.peri_id = peri_id,
+	.mcbuf_sz = 0,
+	.flags = IRQF_SHARED,
+};
 /**
  * struct apd_device_desc - a descriptor for apd device
  * @flags: device flags like %ACPI_APD_SYSFS, %ACPI_APD_PM
@@ -150,6 +161,12 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
 		goto amba_register_err;
 
 	kfree(resource);
+
+	dma_cap_set(DMA_MEMCPY, amd_pl330.cap_mask);
+	dma_cap_set(DMA_SLAVE, amd_pl330.cap_mask);
+	dma_cap_set(DMA_CYCLIC, amd_pl330.cap_mask);
+	dma_cap_set(DMA_PRIVATE, amd_pl330.cap_mask);
+
 	return 0;
 
 amba_register_err:
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 17ee758..5e5fb46 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -26,6 +26,8 @@
 #include <linux/scatterlist.h>
 #include <linux/of.h>
 #include <linux/of_dma.h>
+#include <linux/acpi.h>
+#include <linux/acpi_dma.h>
 #include <linux/err.h>
 #include <linux/pm_runtime.h>
 
@@ -488,6 +490,9 @@ struct pl330_dmac {
 	/* Peripheral channels connected to this DMAC */
 	unsigned int num_peripherals;
 	struct dma_pl330_chan *peripherals; /* keep at end */
+
+	/*IRQ register flags */
+	unsigned int flags;
 };
 
 struct dma_pl330_desc {
@@ -2800,6 +2805,8 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 
 	pl330->mcbufsz = pdat ? pdat->mcbuf_sz : 0;
 
+	pl330->flags = pdat ? pdat->flags : IRQF_TRIGGER_NONE;
+
 	res = &adev->res;
 	pl330->base = devm_ioremap_resource(&adev->dev, res);
 	if (IS_ERR(pl330->base))
@@ -2811,7 +2818,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 		irq = adev->irq[i];
 		if (irq) {
 			ret = devm_request_irq(&adev->dev, irq,
-					       pl330_irq_handler, 0,
+					       pl330_irq_handler, pl330->flags,
 					       dev_name(&adev->dev), pl330);
 			if (ret)
 				return ret;
diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
index fe93758..cdc80f0 100644
--- a/include/linux/amba/pl330.h
+++ b/include/linux/amba/pl330.h
@@ -29,6 +29,8 @@ struct dma_pl330_platdata {
 	dma_cap_mask_t cap_mask;
 	/* Bytes to allocate for MC buffer */
 	unsigned mcbuf_sz;
+	/*flags for irq sharing, default is non-shared*/
+	unsigned flags;
 };
 
 extern bool pl330_filter(struct dma_chan *chan, void *param);
-- 
1.9.1

[PATCH 4/6] dmaengine: pl330: add new items for pl330 private data

[Wang Hongcheng]

mcbuf_sz means bytes to allocate for MicroCode buffer.
flags is for irq sharing, default is non-shared, in AMD
Carrizo, pl330 shares IRQ with its corresponding UART device.

Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
---
 drivers/acpi/acpi_apd.c    | 17 +++++++++++++++++
 drivers/dma/pl330.c        |  9 ++++++++-
 include/linux/amba/pl330.h |  2 ++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
index 9520daf..4d71a65 100644
--- a/drivers/acpi/acpi_apd.c
+++ b/drivers/acpi/acpi_apd.c
@@ -20,6 +20,9 @@
 #include <linux/pm.h>
 #include <linux/amba/bus.h>
 #include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/interrupt.h>
+#include <linux/amba/pl330.h>
 
 #include "internal.h"
 
@@ -33,6 +36,14 @@ struct apd_private_data;
 #define ACPI_APD_SYSFS	BIT(0)
 #define ACPI_APD_PM	BIT(1)
 
+static u8 peri_id[2] = { 0, 1 };
+
+static struct dma_pl330_platdata amd_pl330 = {
+	.nr_valid_peri = 2,
+	.peri_id = peri_id,
+	.mcbuf_sz = 0,
+	.flags = IRQF_SHARED,
+};
 /**
  * struct apd_device_desc - a descriptor for apd device
  * @flags: device flags like %ACPI_APD_SYSFS, %ACPI_APD_PM
@@ -150,6 +161,12 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
 		goto amba_register_err;
 
 	kfree(resource);
+
+	dma_cap_set(DMA_MEMCPY, amd_pl330.cap_mask);
+	dma_cap_set(DMA_SLAVE, amd_pl330.cap_mask);
+	dma_cap_set(DMA_CYCLIC, amd_pl330.cap_mask);
+	dma_cap_set(DMA_PRIVATE, amd_pl330.cap_mask);
+
 	return 0;
 
 amba_register_err:
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 17ee758..5e5fb46 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -26,6 +26,8 @@
 #include <linux/scatterlist.h>
 #include <linux/of.h>
 #include <linux/of_dma.h>
+#include <linux/acpi.h>
+#include <linux/acpi_dma.h>
 #include <linux/err.h>
 #include <linux/pm_runtime.h>
 
@@ -488,6 +490,9 @@ struct pl330_dmac {
 	/* Peripheral channels connected to this DMAC */
 	unsigned int num_peripherals;
 	struct dma_pl330_chan *peripherals; /* keep at end */
+
+	/*IRQ register flags */
+	unsigned int flags;
 };
 
 struct dma_pl330_desc {
@@ -2800,6 +2805,8 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 
 	pl330->mcbufsz = pdat ? pdat->mcbuf_sz : 0;
 
+	pl330->flags = pdat ? pdat->flags : IRQF_TRIGGER_NONE;
+
 	res = &adev->res;
 	pl330->base = devm_ioremap_resource(&adev->dev, res);
 	if (IS_ERR(pl330->base))
@@ -2811,7 +2818,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 		irq = adev->irq[i];
 		if (irq) {
 			ret = devm_request_irq(&adev->dev, irq,
-					       pl330_irq_handler, 0,
+					       pl330_irq_handler, pl330->flags,
 					       dev_name(&adev->dev), pl330);
 			if (ret)
 				return ret;
diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
index fe93758..cdc80f0 100644
--- a/include/linux/amba/pl330.h
+++ b/include/linux/amba/pl330.h
@@ -29,6 +29,8 @@ struct dma_pl330_platdata {
 	dma_cap_mask_t cap_mask;
 	/* Bytes to allocate for MC buffer */
 	unsigned mcbuf_sz;
+	/*flags for irq sharing, default is non-shared*/
+	unsigned flags;
 };
 
 extern bool pl330_filter(struct dma_chan *chan, void *param);
-- 
1.9.1

Re: [PATCH 4/6] dmaengine: pl330: add new items for pl330 private data

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

Hi Wang,

[auto build test WARNING on v4.4-rc8]
[cannot apply to tty/tty-testing pm/linux-next next-20151231]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Wang-Hongcheng/8250-AMD-Carrizo-UART-PL300-DMA-enablement/20160104-133639
config: x86_64-randconfig-x013-01040119 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

>> drivers/acpi/acpi_apd.c:41:34: warning: 'amd_pl330' defined but not used [-Wunused-variable]
    static struct dma_pl330_platdata amd_pl330 = {
                                     ^

vim +/amd_pl330 +41 drivers/acpi/acpi_apd.c

    25	#include <linux/amba/pl330.h>
    26	
    27	#include "internal.h"
    28	
    29	ACPI_MODULE_NAME("acpi_apd");
    30	struct apd_private_data;
    31	
    32	/**
    33	 * ACPI_APD_SYSFS : add device attributes in sysfs
    34	 * ACPI_APD_PM : attach power domain to device
    35	 */
    36	#define ACPI_APD_SYSFS	BIT(0)
    37	#define ACPI_APD_PM	BIT(1)
    38	
    39	static u8 peri_id[2] = { 0, 1 };
    40	
  > 41	static struct dma_pl330_platdata amd_pl330 = {
    42		.nr_valid_peri = 2,
    43		.peri_id = peri_id,
    44		.mcbuf_sz = 0,
    45		.flags = IRQF_SHARED,
    46	};
    47	/**
    48	 * struct apd_device_desc - a descriptor for apd device
    49	 * @flags: device flags like %ACPI_APD_SYSFS, %ACPI_APD_PM

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 19026 bytes --]

Re: [PATCH 4/6] dmaengine: pl330: add new items for pl330 private data

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

Hi Wang,

[auto build test WARNING on v4.4-rc8]
[cannot apply to tty/tty-testing pm/linux-next next-20151231]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Wang-Hongcheng/8250-AMD-Carrizo-UART-PL300-DMA-enablement/20160104-133639
config: x86_64-randconfig-x013-01040119 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

>> drivers/acpi/acpi_apd.c:41:34: warning: 'amd_pl330' defined but not used [-Wunused-variable]
    static struct dma_pl330_platdata amd_pl330 = {
                                     ^

vim +/amd_pl330 +41 drivers/acpi/acpi_apd.c

    25	#include <linux/amba/pl330.h>
    26	
    27	#include "internal.h"
    28	
    29	ACPI_MODULE_NAME("acpi_apd");
    30	struct apd_private_data;
    31	
    32	/**
    33	 * ACPI_APD_SYSFS : add device attributes in sysfs
    34	 * ACPI_APD_PM : attach power domain to device
    35	 */
    36	#define ACPI_APD_SYSFS	BIT(0)
    37	#define ACPI_APD_PM	BIT(1)
    38	
    39	static u8 peri_id[2] = { 0, 1 };
    40	
  > 41	static struct dma_pl330_platdata amd_pl330 = {
    42		.nr_valid_peri = 2,
    43		.peri_id = peri_id,
    44		.mcbuf_sz = 0,
    45		.flags = IRQF_SHARED,
    46	};
    47	/**
    48	 * struct apd_device_desc - a descriptor for apd device
    49	 * @flags: device flags like %ACPI_APD_SYSFS, %ACPI_APD_PM

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 19026 bytes --]

Re: [PATCH 4/6] dmaengine: pl330: add new items for pl330 private data

[Andy Shevchenko]

On Mon, Jan 4, 2016 at 7:31 AM, Wang Hongcheng <annie.wang@amd.com> wrote:
> mcbuf_sz means bytes to allocate for MicroCode buffer.
> flags is for irq sharing, default is non-shared, in AMD
> Carrizo, pl330 shares IRQ with its corresponding UART device.
>
> Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
> ---
>  drivers/acpi/acpi_apd.c    | 17 +++++++++++++++++
>  drivers/dma/pl330.c        |  9 ++++++++-
>  include/linux/amba/pl330.h |  2 ++
>  3 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> index 9520daf..4d71a65 100644
> --- a/drivers/acpi/acpi_apd.c
> +++ b/drivers/acpi/acpi_apd.c
> @@ -20,6 +20,9 @@
>  #include <linux/pm.h>
>  #include <linux/amba/bus.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/interrupt.h>
> +#include <linux/amba/pl330.h>
>
>  #include "internal.h"
>
> @@ -33,6 +36,14 @@ struct apd_private_data;
>  #define ACPI_APD_SYSFS BIT(0)
>  #define ACPI_APD_PM    BIT(1)
>
> +static u8 peri_id[2] = { 0, 1 };
> +
> +static struct dma_pl330_platdata amd_pl330 = {
> +       .nr_valid_peri = 2,
> +       .peri_id = peri_id,
> +       .mcbuf_sz = 0,
> +       .flags = IRQF_SHARED,
> +};
>  /**
>   * struct apd_device_desc - a descriptor for apd device
>   * @flags: device flags like %ACPI_APD_SYSFS, %ACPI_APD_PM
> @@ -150,6 +161,12 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
>                 goto amba_register_err;
>
>         kfree(resource);
> +
> +       dma_cap_set(DMA_MEMCPY, amd_pl330.cap_mask);
> +       dma_cap_set(DMA_SLAVE, amd_pl330.cap_mask);
> +       dma_cap_set(DMA_CYCLIC, amd_pl330.cap_mask);
> +       dma_cap_set(DMA_PRIVATE, amd_pl330.cap_mask);
> +
>         return 0;
>
>  amba_register_err:
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 17ee758..5e5fb46 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -26,6 +26,8 @@
>  #include <linux/scatterlist.h>
>  #include <linux/of.h>
>  #include <linux/of_dma.h>
> +#include <linux/acpi.h>
> +#include <linux/acpi_dma.h>
>  #include <linux/err.h>
>  #include <linux/pm_runtime.h>
>
> @@ -488,6 +490,9 @@ struct pl330_dmac {
>         /* Peripheral channels connected to this DMAC */
>         unsigned int num_peripherals;
>         struct dma_pl330_chan *peripherals; /* keep at end */
> +
> +       /*IRQ register flags */

+Space '/* '

> +       unsigned int flags;

irq_flags

>  };
>
>  struct dma_pl330_desc {
> @@ -2800,6 +2805,8 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>
>         pl330->mcbufsz = pdat ? pdat->mcbuf_sz : 0;
>
> +       pl330->flags = pdat ? pdat->flags : IRQF_TRIGGER_NONE;
> +
>         res = &adev->res;
>         pl330->base = devm_ioremap_resource(&adev->dev, res);
>         if (IS_ERR(pl330->base))
> @@ -2811,7 +2818,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>                 irq = adev->irq[i];
>                 if (irq) {
>                         ret = devm_request_irq(&adev->dev, irq,
> -                                              pl330_irq_handler, 0,
> +                                              pl330_irq_handler, pl330->flags,
>                                                dev_name(&adev->dev), pl330);
>                         if (ret)
>                                 return ret;
> diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
> index fe93758..cdc80f0 100644
> --- a/include/linux/amba/pl330.h
> +++ b/include/linux/amba/pl330.h
> @@ -29,6 +29,8 @@ struct dma_pl330_platdata {
>         dma_cap_mask_t cap_mask;
>         /* Bytes to allocate for MC buffer */
>         unsigned mcbuf_sz;
> +       /*flags for irq sharing, default is non-shared*/

Spaces '/* */'

> +       unsigned flags;

irq_flags

>  };

>
>  extern bool pl330_filter(struct dma_chan *chan, void *param);
> --
> 1.9.1
>



-- 
With Best Regards,
Andy Shevchenko

[PATCH 5/6] dmaengine: pl330: provide ACPI dmaengine interface

[Wang Hongcheng]

register acpi_dma controller, so ACPI devices can request pl330 DMA
channel. Request line info is get from private data.

Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
---
 drivers/acpi/acpi_apd.c    | 34 +++++++++++++++++++++++++---------
 drivers/dma/pl330.c        | 27 +++++++++++++++++++++++++++
 include/linux/amba/pl330.h |  4 ++++
 3 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
index 4d71a65..1f16cca 100644
--- a/drivers/acpi/acpi_apd.c
+++ b/drivers/acpi/acpi_apd.c
@@ -38,11 +38,23 @@ struct apd_private_data;
 
 static u8 peri_id[2] = { 0, 1 };
 
-static struct dma_pl330_platdata amd_pl330 = {
-	.nr_valid_peri = 2,
-	.peri_id = peri_id,
-	.mcbuf_sz = 0,
-	.flags = IRQF_SHARED,
+static struct dma_pl330_platdata amd_pl330[] = {
+	{
+		.nr_valid_peri = 2,
+		.peri_id = peri_id,
+		.mcbuf_sz = 0,
+		.flags = IRQF_SHARED,
+		.base_request_line = 1,
+		.num = 0,
+	},
+	{
+		.nr_valid_peri = 2,
+		.peri_id = peri_id,
+		.mcbuf_sz = 0,
+		.flags = IRQF_SHARED,
+		.base_request_line = 2,
+		.num = 0,
+	}
 };
 /**
  * struct apd_device_desc - a descriptor for apd device
@@ -101,6 +113,7 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
 	unsigned int irq[AMBA_NR_IRQS];
 	struct clk *clk = ERR_PTR(-ENODEV);
 	char amba_devname[100];
+	int devnum;
 
 	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
 	if (!resource)
@@ -131,8 +144,11 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
 	if (!amba_dev)
 		goto amba_alloc_err;
 
+	devnum = amba_devname[strlen(dev_name(&pdev->dev)) - 1] - '0';
+
 	amba_dev->dev.coherent_dma_mask
 		= acpi_dma_supported(ACPI_COMPANION(&pdev->dev)) ? DMA_BIT_MASK(64) : 0;
+	amba_dev->dev.platform_data = &amd_pl330[devnum];
 	amba_dev->dev.fwnode = acpi_fwnode_handle(ACPI_COMPANION(&pdev->dev));
 
 	amba_dev->dev.parent = &pdev->dev;
@@ -162,10 +178,10 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
 
 	kfree(resource);
 
-	dma_cap_set(DMA_MEMCPY, amd_pl330.cap_mask);
-	dma_cap_set(DMA_SLAVE, amd_pl330.cap_mask);
-	dma_cap_set(DMA_CYCLIC, amd_pl330.cap_mask);
-	dma_cap_set(DMA_PRIVATE, amd_pl330.cap_mask);
+	dma_cap_set(DMA_MEMCPY, amd_pl330[devnum].cap_mask);
+	dma_cap_set(DMA_SLAVE, amd_pl330[devnum].cap_mask);
+	dma_cap_set(DMA_CYCLIC, amd_pl330[devnum].cap_mask);
+	dma_cap_set(DMA_PRIVATE, amd_pl330[devnum].cap_mask);
 
 	return 0;
 
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 5e5fb46..e0e0b6e 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2079,6 +2079,22 @@ static struct dma_chan *of_dma_pl330_xlate(struct of_phandle_args *dma_spec,
 	return dma_get_slave_channel(&pl330->peripherals[chan_id].chan);
 }
 
+static struct dma_chan *acpi_dma_pl330_xlate(struct acpi_dma_spec *dma_spec,
+					     struct acpi_dma *adma)
+{
+	struct pl330_dmac *pl330 = adma->data;
+	struct dma_pl330_platdata *pdat;
+	unsigned int chan_id;
+
+	pdat = dev_get_platdata(adma->dev);
+
+	chan_id = dma_spec->chan_id;
+	if (chan_id >= pl330->num_peripherals)
+		return NULL;
+
+	return dma_get_slave_channel(&pl330->peripherals[chan_id].chan);
+}
+
 static int pl330_alloc_chan_resources(struct dma_chan *chan)
 {
 	struct dma_pl330_chan *pch = to_pchan(chan);
@@ -2918,6 +2934,17 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 		}
 	}
 
+	if (has_acpi_companion(&adev->dev)) {
+		ret = acpi_dma_controller_register(&adev->dev,
+						   acpi_dma_pl330_xlate,
+						   pdat->base_request_line,
+						   pdat->num, pl330);
+		if (ret) {
+			dev_err(&adev->dev,
+				"unable to register DMA to the generic ACPI DMA helpers\n");
+		}
+	}
+
 	adev->dev.dma_parms = &pl330->dma_parms;
 
 	/*
diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
index cdc80f0..1190f69 100644
--- a/include/linux/amba/pl330.h
+++ b/include/linux/amba/pl330.h
@@ -31,6 +31,10 @@ struct dma_pl330_platdata {
 	unsigned mcbuf_sz;
 	/*flags for irq sharing, default is non-shared*/
 	unsigned flags;
+	/*device base request line*/
+	unsigned short base_request_line;
+	/*device request line range*/
+	unsigned short num;
 };
 
 extern bool pl330_filter(struct dma_chan *chan, void *param);
-- 
1.9.1

[PATCH 5/6] dmaengine: pl330: provide ACPI dmaengine interface

[Wang Hongcheng]

register acpi_dma controller, so ACPI devices can request pl330 DMA
channel. Request line info is get from private data.

Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
---
 drivers/acpi/acpi_apd.c    | 34 +++++++++++++++++++++++++---------
 drivers/dma/pl330.c        | 27 +++++++++++++++++++++++++++
 include/linux/amba/pl330.h |  4 ++++
 3 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
index 4d71a65..1f16cca 100644
--- a/drivers/acpi/acpi_apd.c
+++ b/drivers/acpi/acpi_apd.c
@@ -38,11 +38,23 @@ struct apd_private_data;
 
 static u8 peri_id[2] = { 0, 1 };
 
-static struct dma_pl330_platdata amd_pl330 = {
-	.nr_valid_peri = 2,
-	.peri_id = peri_id,
-	.mcbuf_sz = 0,
-	.flags = IRQF_SHARED,
+static struct dma_pl330_platdata amd_pl330[] = {
+	{
+		.nr_valid_peri = 2,
+		.peri_id = peri_id,
+		.mcbuf_sz = 0,
+		.flags = IRQF_SHARED,
+		.base_request_line = 1,
+		.num = 0,
+	},
+	{
+		.nr_valid_peri = 2,
+		.peri_id = peri_id,
+		.mcbuf_sz = 0,
+		.flags = IRQF_SHARED,
+		.base_request_line = 2,
+		.num = 0,
+	}
 };
 /**
  * struct apd_device_desc - a descriptor for apd device
@@ -101,6 +113,7 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
 	unsigned int irq[AMBA_NR_IRQS];
 	struct clk *clk = ERR_PTR(-ENODEV);
 	char amba_devname[100];
+	int devnum;
 
 	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
 	if (!resource)
@@ -131,8 +144,11 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
 	if (!amba_dev)
 		goto amba_alloc_err;
 
+	devnum = amba_devname[strlen(dev_name(&pdev->dev)) - 1] - '0';
+
 	amba_dev->dev.coherent_dma_mask
 		= acpi_dma_supported(ACPI_COMPANION(&pdev->dev)) ? DMA_BIT_MASK(64) : 0;
+	amba_dev->dev.platform_data = &amd_pl330[devnum];
 	amba_dev->dev.fwnode = acpi_fwnode_handle(ACPI_COMPANION(&pdev->dev));
 
 	amba_dev->dev.parent = &pdev->dev;
@@ -162,10 +178,10 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
 
 	kfree(resource);
 
-	dma_cap_set(DMA_MEMCPY, amd_pl330.cap_mask);
-	dma_cap_set(DMA_SLAVE, amd_pl330.cap_mask);
-	dma_cap_set(DMA_CYCLIC, amd_pl330.cap_mask);
-	dma_cap_set(DMA_PRIVATE, amd_pl330.cap_mask);
+	dma_cap_set(DMA_MEMCPY, amd_pl330[devnum].cap_mask);
+	dma_cap_set(DMA_SLAVE, amd_pl330[devnum].cap_mask);
+	dma_cap_set(DMA_CYCLIC, amd_pl330[devnum].cap_mask);
+	dma_cap_set(DMA_PRIVATE, amd_pl330[devnum].cap_mask);
 
 	return 0;
 
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 5e5fb46..e0e0b6e 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2079,6 +2079,22 @@ static struct dma_chan *of_dma_pl330_xlate(struct of_phandle_args *dma_spec,
 	return dma_get_slave_channel(&pl330->peripherals[chan_id].chan);
 }
 
+static struct dma_chan *acpi_dma_pl330_xlate(struct acpi_dma_spec *dma_spec,
+					     struct acpi_dma *adma)
+{
+	struct pl330_dmac *pl330 = adma->data;
+	struct dma_pl330_platdata *pdat;
+	unsigned int chan_id;
+
+	pdat = dev_get_platdata(adma->dev);
+
+	chan_id = dma_spec->chan_id;
+	if (chan_id >= pl330->num_peripherals)
+		return NULL;
+
+	return dma_get_slave_channel(&pl330->peripherals[chan_id].chan);
+}
+
 static int pl330_alloc_chan_resources(struct dma_chan *chan)
 {
 	struct dma_pl330_chan *pch = to_pchan(chan);
@@ -2918,6 +2934,17 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 		}
 	}
 
+	if (has_acpi_companion(&adev->dev)) {
+		ret = acpi_dma_controller_register(&adev->dev,
+						   acpi_dma_pl330_xlate,
+						   pdat->base_request_line,
+						   pdat->num, pl330);
+		if (ret) {
+			dev_err(&adev->dev,
+				"unable to register DMA to the generic ACPI DMA helpers\n");
+		}
+	}
+
 	adev->dev.dma_parms = &pl330->dma_parms;
 
 	/*
diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
index cdc80f0..1190f69 100644
--- a/include/linux/amba/pl330.h
+++ b/include/linux/amba/pl330.h
@@ -31,6 +31,10 @@ struct dma_pl330_platdata {
 	unsigned mcbuf_sz;
 	/*flags for irq sharing, default is non-shared*/
 	unsigned flags;
+	/*device base request line*/
+	unsigned short base_request_line;
+	/*device request line range*/
+	unsigned short num;
 };
 
 extern bool pl330_filter(struct dma_chan *chan, void *param);
-- 
1.9.1

Re: [PATCH 5/6] dmaengine: pl330: provide ACPI dmaengine interface

[Andy Shevchenko]

On Mon, Jan 4, 2016 at 7:31 AM, Wang Hongcheng <annie.wang@amd.com> wrote:
> register acpi_dma controller, so ACPI devices can request pl330 DMA
> channel. Request line info is get from private data.
>
> Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
> ---
>  drivers/acpi/acpi_apd.c    | 34 +++++++++++++++++++++++++---------
>  drivers/dma/pl330.c        | 27 +++++++++++++++++++++++++++
>  include/linux/amba/pl330.h |  4 ++++
>  3 files changed, 56 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> index 4d71a65..1f16cca 100644
> --- a/drivers/acpi/acpi_apd.c
> +++ b/drivers/acpi/acpi_apd.c
> @@ -38,11 +38,23 @@ struct apd_private_data;
>
>  static u8 peri_id[2] = { 0, 1 };
>
> -static struct dma_pl330_platdata amd_pl330 = {
> -       .nr_valid_peri = 2,
> -       .peri_id = peri_id,
> -       .mcbuf_sz = 0,
> -       .flags = IRQF_SHARED,

Agreed with bot, those two looks like WIP. Please, unite them in one
clean patch.

> +static struct dma_pl330_platdata amd_pl330[] = {
> +       {
> +               .nr_valid_peri = 2,
> +               .peri_id = peri_id,
> +               .mcbuf_sz = 0,
> +               .flags = IRQF_SHARED,

> +               .base_request_line = 1,
> +               .num = 0,

(1)

> +       },

> +       {
> +               .nr_valid_peri = 2,
> +               .peri_id = peri_id,
> +               .mcbuf_sz = 0,
> +               .flags = IRQF_SHARED,

> +               .base_request_line = 2,
> +               .num = 0,

(2)

(1) and (2) seem use wrong interpretation of the parameters. num is an
amount of request lines in the range:
base_request_line..base_request_line + num - 1.

> +       }
>  };
>  /**
>   * struct apd_device_desc - a descriptor for apd device
> @@ -101,6 +113,7 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
>         unsigned int irq[AMBA_NR_IRQS];
>         struct clk *clk = ERR_PTR(-ENODEV);
>         char amba_devname[100];
> +       int devnum;
>
>         resource = kzalloc(sizeof(*resource), GFP_KERNEL);
>         if (!resource)
> @@ -131,8 +144,11 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
>         if (!amba_dev)
>                 goto amba_alloc_err;
>
> +       devnum = amba_devname[strlen(dev_name(&pdev->dev)) - 1] - '0';

This looks error prone.

> +
>         amba_dev->dev.coherent_dma_mask
>                 = acpi_dma_supported(ACPI_COMPANION(&pdev->dev)) ? DMA_BIT_MASK(64) : 0;
> +       amba_dev->dev.platform_data = &amd_pl330[devnum];
>         amba_dev->dev.fwnode = acpi_fwnode_handle(ACPI_COMPANION(&pdev->dev));
>
>         amba_dev->dev.parent = &pdev->dev;
> @@ -162,10 +178,10 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
>
>         kfree(resource);
>
> -       dma_cap_set(DMA_MEMCPY, amd_pl330.cap_mask);
> -       dma_cap_set(DMA_SLAVE, amd_pl330.cap_mask);
> -       dma_cap_set(DMA_CYCLIC, amd_pl330.cap_mask);
> -       dma_cap_set(DMA_PRIVATE, amd_pl330.cap_mask);
> +       dma_cap_set(DMA_MEMCPY, amd_pl330[devnum].cap_mask);
> +       dma_cap_set(DMA_SLAVE, amd_pl330[devnum].cap_mask);
> +       dma_cap_set(DMA_CYCLIC, amd_pl330[devnum].cap_mask);
> +       dma_cap_set(DMA_PRIVATE, amd_pl330[devnum].cap_mask);
>
>         return 0;
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 5e5fb46..e0e0b6e 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2079,6 +2079,22 @@ static struct dma_chan *of_dma_pl330_xlate(struct of_phandle_args *dma_spec,
>         return dma_get_slave_channel(&pl330->peripherals[chan_id].chan);
>  }
>
> +static struct dma_chan *acpi_dma_pl330_xlate(struct acpi_dma_spec *dma_spec,
> +                                            struct acpi_dma *adma)
> +{
> +       struct pl330_dmac *pl330 = adma->data;
> +       struct dma_pl330_platdata *pdat;
> +       unsigned int chan_id;
> +

> +       pdat = dev_get_platdata(adma->dev);

This could be part of the definition block above

> +
> +       chan_id = dma_spec->chan_id;
> +       if (chan_id >= pl330->num_peripherals)
> +               return NULL;
> +
> +       return dma_get_slave_channel(&pl330->peripherals[chan_id].chan);
> +}
> +
>  static int pl330_alloc_chan_resources(struct dma_chan *chan)
>  {
>         struct dma_pl330_chan *pch = to_pchan(chan);
> @@ -2918,6 +2934,17 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>                 }
>         }
>
> +       if (has_acpi_companion(&adev->dev)) {
> +               ret = acpi_dma_controller_register(&adev->dev,
> +                                                  acpi_dma_pl330_xlate,
> +                                                  pdat->base_request_line,
> +                                                  pdat->num, pl330);
> +               if (ret) {
> +                       dev_err(&adev->dev,
> +                               "unable to register DMA to the generic ACPI DMA helpers\n");
> +               }
> +       }
> +
>         adev->dev.dma_parms = &pl330->dma_parms;
>
>         /*
> diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
> index cdc80f0..1190f69 100644
> --- a/include/linux/amba/pl330.h
> +++ b/include/linux/amba/pl330.h
> @@ -31,6 +31,10 @@ struct dma_pl330_platdata {
>         unsigned mcbuf_sz;
>         /*flags for irq sharing, default is non-shared*/
>         unsigned flags;
> +       /*device base request line*/

Spaces and capital letters. Keep one style.

> +       unsigned short base_request_line;
> +       /*device request line range*/

Ditto.

> +       unsigned short num;
>  };
>
>  extern bool pl330_filter(struct dma_chan *chan, void *param);
> --
> 1.9.1
>



-- 
With Best Regards,
Andy Shevchenko

[PATCH 6/6] Serial:8250: New Port Type PORT_AMD_8250

[Wang Hongcheng]

Set a new port type for AMD Carrizo.  Add has_pl330_dma to 8250_dw's
private data and init fcr,ier as well as dma rx size.

Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
---
 drivers/acpi/acpi_apd.c               | 11 +++++++++++
 drivers/tty/serial/8250/8250_dw.c     | 15 +++++++++++++++
 drivers/tty/serial/8250/8250_port.c   |  9 +++++++++
 include/linux/platform_data/8250-dw.h |  8 ++++++++
 include/uapi/linux/serial_core.h      |  3 ++-
 include/uapi/linux/serial_reg.h       |  1 +
 6 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/platform_data/8250-dw.h

diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
index 1f16cca..e8e43fb 100644
--- a/drivers/acpi/acpi_apd.c
+++ b/drivers/acpi/acpi_apd.c
@@ -23,6 +23,7 @@
 #include <linux/dmaengine.h>
 #include <linux/interrupt.h>
 #include <linux/amba/pl330.h>
+#include <linux/platform_data/8250-dw.h>
 
 #include "internal.h"
 
@@ -56,6 +57,11 @@ static struct dma_pl330_platdata amd_pl330[] = {
 		.num = 0,
 	}
 };
+
+static struct plat_dw8250_data amd_dw8250 = {
+	.has_pl330_dma = 1,
+};
+
 /**
  * struct apd_device_desc - a descriptor for apd device
  * @flags: device flags like %ACPI_APD_SYSFS, %ACPI_APD_PM
@@ -115,6 +121,11 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
 	char amba_devname[100];
 	int devnum;
 
+	ret = platform_device_add_data(pdev, &amd_dw8250,
+				       sizeof(amd_dw8250));
+	if (ret)
+		goto resource_alloc_err;
+
 	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
 	if (!resource)
 		goto resource_alloc_err;
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index a5d319e..e7d9f01 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -27,6 +27,7 @@
 #include <linux/clk.h>
 #include <linux/reset.h>
 #include <linux/pm_runtime.h>
+#include <linux/platform_data/8250-dw.h>
 
 #include <asm/byteorder.h>
 
@@ -66,6 +67,7 @@ struct dw8250_data {
 
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
+	unsigned int		has_pl330_dma:1;
 };
 
 #define BYT_PRV_CLK			0x800
@@ -303,6 +305,7 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 static void dw8250_setup_port(struct uart_port *p)
 {
 	struct uart_8250_port *up = up_to_u8250p(p);
+	struct dw8250_data *data = p->private_data;
 	u32 reg;
 
 	/*
@@ -326,6 +329,14 @@ static void dw8250_setup_port(struct uart_port *p)
 		p->flags |= UPF_FIXED_TYPE;
 		p->fifosize = DW_UART_CPR_FIFO_SIZE(reg);
 		up->capabilities = UART_CAP_FIFO;
+		if (data->has_pl330_dma) {
+			p->type = PORT_AMD_8250;
+
+			up->ier |= UART_IER_PTIME | UART_IER_THRI |
+				UART_IER_RLSI | UART_IER_RDI;
+			up->fcr |= UART_FCR_R_TRIG_10 | UART_FCR_T_TRIG_11;
+			up->tx_loadsz = p->fifosize / 2;
+		}
 	}
 
 	if (reg & DW_UART_CPR_AFCE_MODE)
@@ -339,6 +350,7 @@ static int dw8250_probe(struct platform_device *pdev)
 	int irq = platform_get_irq(pdev, 0);
 	struct uart_port *p = &uart.port;
 	struct dw8250_data *data;
+	struct plat_dw8250_data *pdata = dev_get_platdata(&pdev->dev);
 	int err;
 	u32 val;
 
@@ -468,6 +480,7 @@ static int dw8250_probe(struct platform_device *pdev)
 		p->handle_irq = NULL;
 	}
 
+	data->has_pl330_dma = pdata ? pdata->has_pl330_dma : 0;
 	if (!data->skip_autocfg)
 		dw8250_setup_port(p);
 
@@ -475,6 +488,8 @@ static int dw8250_probe(struct platform_device *pdev)
 	if (p->fifosize) {
 		data->dma.rxconf.src_maxburst = p->fifosize / 4;
 		data->dma.txconf.dst_maxburst = p->fifosize / 4;
+		data->dma.rx_size
+			= data->has_pl330_dma ? (p->fifosize / 2 + 2) : 0;
 		uart.dma = &data->dma;
 	}
 
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 52d82d2..b89a326 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -269,6 +269,15 @@ configured less than Maximum supported fifo bytes */
 		.rxtrig_bytes	= {1, 4, 8, 14},
 		.flags		= UART_CAP_FIFO,
 	},
+	[PORT_AMD_8250] = {
+		.name		= "AMD_8250",
+		.fifo_size	= 256,
+		.tx_loadsz	= 128,
+		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
+				UART_FCR_T_TRIG_11,
+		.rxtrig_bytes	= {1, 4, 8},
+		.flags		= UART_CAP_FIFO,
+	},
 };
 
 /* Uart divisor latch read */
diff --git a/include/linux/platform_data/8250-dw.h b/include/linux/platform_data/8250-dw.h
new file mode 100644
index 0000000..97cdb9d
--- /dev/null
+++ b/include/linux/platform_data/8250-dw.h
@@ -0,0 +1,8 @@
+#ifndef _PLATFORM_DATA_8250_DW_H
+#define _PLATFORM_DATA_8250_DW_H
+
+struct plat_dw8250_data {
+	unsigned int has_pl330_dma:1;
+};
+
+#endif
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index 93ba148..89d44b0 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -56,7 +56,8 @@
 #define PORT_ALTR_16550_F128 28 /* Altera 16550 UART with 128 FIFOs */
 #define PORT_RT2880	29	/* Ralink RT2880 internal UART */
 #define PORT_16550A_FSL64 30	/* Freescale 16550 UART with 64 FIFOs */
-#define PORT_MAX_8250	30	/* max port ID */
+#define PORT_AMD_8250	31
+#define PORT_MAX_8250	32	/* max port ID */
 
 /*
  * ARM specific type numbers.  These are not currently guaranteed
diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
index 1e5ac4e7..66686b9 100644
--- a/include/uapi/linux/serial_reg.h
+++ b/include/uapi/linux/serial_reg.h
@@ -20,6 +20,7 @@
 #define UART_RX		0	/* In:  Receive buffer */
 #define UART_TX		0	/* Out: Transmit buffer */
 
+#define UART_IER_PTIME          0x80  /* Enable Programmable Transmitter holding register int. */
 #define UART_IER	1	/* Out: Interrupt Enable Register */
 #define UART_IER_MSI		0x08 /* Enable Modem status interrupt */
 #define UART_IER_RLSI		0x04 /* Enable receiver line status interrupt */
-- 
1.9.1

[PATCH 6/6] Serial:8250: New Port Type PORT_AMD_8250

[Wang Hongcheng]

Set a new port type for AMD Carrizo.  Add has_pl330_dma to 8250_dw's
private data and init fcr,ier as well as dma rx size.

Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
---
 drivers/acpi/acpi_apd.c               | 11 +++++++++++
 drivers/tty/serial/8250/8250_dw.c     | 15 +++++++++++++++
 drivers/tty/serial/8250/8250_port.c   |  9 +++++++++
 include/linux/platform_data/8250-dw.h |  8 ++++++++
 include/uapi/linux/serial_core.h      |  3 ++-
 include/uapi/linux/serial_reg.h       |  1 +
 6 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/platform_data/8250-dw.h

diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
index 1f16cca..e8e43fb 100644
--- a/drivers/acpi/acpi_apd.c
+++ b/drivers/acpi/acpi_apd.c
@@ -23,6 +23,7 @@
 #include <linux/dmaengine.h>
 #include <linux/interrupt.h>
 #include <linux/amba/pl330.h>
+#include <linux/platform_data/8250-dw.h>
 
 #include "internal.h"
 
@@ -56,6 +57,11 @@ static struct dma_pl330_platdata amd_pl330[] = {
 		.num = 0,
 	}
 };
+
+static struct plat_dw8250_data amd_dw8250 = {
+	.has_pl330_dma = 1,
+};
+
 /**
  * struct apd_device_desc - a descriptor for apd device
  * @flags: device flags like %ACPI_APD_SYSFS, %ACPI_APD_PM
@@ -115,6 +121,11 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
 	char amba_devname[100];
 	int devnum;
 
+	ret = platform_device_add_data(pdev, &amd_dw8250,
+				       sizeof(amd_dw8250));
+	if (ret)
+		goto resource_alloc_err;
+
 	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
 	if (!resource)
 		goto resource_alloc_err;
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index a5d319e..e7d9f01 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -27,6 +27,7 @@
 #include <linux/clk.h>
 #include <linux/reset.h>
 #include <linux/pm_runtime.h>
+#include <linux/platform_data/8250-dw.h>
 
 #include <asm/byteorder.h>
 
@@ -66,6 +67,7 @@ struct dw8250_data {
 
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
+	unsigned int		has_pl330_dma:1;
 };
 
 #define BYT_PRV_CLK			0x800
@@ -303,6 +305,7 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 static void dw8250_setup_port(struct uart_port *p)
 {
 	struct uart_8250_port *up = up_to_u8250p(p);
+	struct dw8250_data *data = p->private_data;
 	u32 reg;
 
 	/*
@@ -326,6 +329,14 @@ static void dw8250_setup_port(struct uart_port *p)
 		p->flags |= UPF_FIXED_TYPE;
 		p->fifosize = DW_UART_CPR_FIFO_SIZE(reg);
 		up->capabilities = UART_CAP_FIFO;
+		if (data->has_pl330_dma) {
+			p->type = PORT_AMD_8250;
+
+			up->ier |= UART_IER_PTIME | UART_IER_THRI |
+				UART_IER_RLSI | UART_IER_RDI;
+			up->fcr |= UART_FCR_R_TRIG_10 | UART_FCR_T_TRIG_11;
+			up->tx_loadsz = p->fifosize / 2;
+		}
 	}
 
 	if (reg & DW_UART_CPR_AFCE_MODE)
@@ -339,6 +350,7 @@ static int dw8250_probe(struct platform_device *pdev)
 	int irq = platform_get_irq(pdev, 0);
 	struct uart_port *p = &uart.port;
 	struct dw8250_data *data;
+	struct plat_dw8250_data *pdata = dev_get_platdata(&pdev->dev);
 	int err;
 	u32 val;
 
@@ -468,6 +480,7 @@ static int dw8250_probe(struct platform_device *pdev)
 		p->handle_irq = NULL;
 	}
 
+	data->has_pl330_dma = pdata ? pdata->has_pl330_dma : 0;
 	if (!data->skip_autocfg)
 		dw8250_setup_port(p);
 
@@ -475,6 +488,8 @@ static int dw8250_probe(struct platform_device *pdev)
 	if (p->fifosize) {
 		data->dma.rxconf.src_maxburst = p->fifosize / 4;
 		data->dma.txconf.dst_maxburst = p->fifosize / 4;
+		data->dma.rx_size
+			= data->has_pl330_dma ? (p->fifosize / 2 + 2) : 0;
 		uart.dma = &data->dma;
 	}
 
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 52d82d2..b89a326 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -269,6 +269,15 @@ configured less than Maximum supported fifo bytes */
 		.rxtrig_bytes	= {1, 4, 8, 14},
 		.flags		= UART_CAP_FIFO,
 	},
+	[PORT_AMD_8250] = {
+		.name		= "AMD_8250",
+		.fifo_size	= 256,
+		.tx_loadsz	= 128,
+		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
+				UART_FCR_T_TRIG_11,
+		.rxtrig_bytes	= {1, 4, 8},
+		.flags		= UART_CAP_FIFO,
+	},
 };
 
 /* Uart divisor latch read */
diff --git a/include/linux/platform_data/8250-dw.h b/include/linux/platform_data/8250-dw.h
new file mode 100644
index 0000000..97cdb9d
--- /dev/null
+++ b/include/linux/platform_data/8250-dw.h
@@ -0,0 +1,8 @@
+#ifndef _PLATFORM_DATA_8250_DW_H
+#define _PLATFORM_DATA_8250_DW_H
+
+struct plat_dw8250_data {
+	unsigned int has_pl330_dma:1;
+};
+
+#endif
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index 93ba148..89d44b0 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -56,7 +56,8 @@
 #define PORT_ALTR_16550_F128 28 /* Altera 16550 UART with 128 FIFOs */
 #define PORT_RT2880	29	/* Ralink RT2880 internal UART */
 #define PORT_16550A_FSL64 30	/* Freescale 16550 UART with 64 FIFOs */
-#define PORT_MAX_8250	30	/* max port ID */
+#define PORT_AMD_8250	31
+#define PORT_MAX_8250	32	/* max port ID */
 
 /*
  * ARM specific type numbers.  These are not currently guaranteed
diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
index 1e5ac4e7..66686b9 100644
--- a/include/uapi/linux/serial_reg.h
+++ b/include/uapi/linux/serial_reg.h
@@ -20,6 +20,7 @@
 #define UART_RX		0	/* In:  Receive buffer */
 #define UART_TX		0	/* Out: Transmit buffer */
 
+#define UART_IER_PTIME          0x80  /* Enable Programmable Transmitter holding register int. */
 #define UART_IER	1	/* Out: Interrupt Enable Register */
 #define UART_IER_MSI		0x08 /* Enable Modem status interrupt */
 #define UART_IER_RLSI		0x04 /* Enable receiver line status interrupt */
-- 
1.9.1

Re: [PATCH 6/6] Serial:8250: New Port Type PORT_AMD_8250

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]

Hi Wang,

[auto build test WARNING on v4.4-rc8]
[cannot apply to tty/tty-testing pm/linux-next next-20151231]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Wang-Hongcheng/8250-AMD-Carrizo-UART-PL300-DMA-enablement/20160104-133639
config: x86_64-randconfig-x013-01040119 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/acpi/acpi_apd.c:42:34: warning: 'amd_pl330' defined but not used [-Wunused-variable]
    static struct dma_pl330_platdata amd_pl330[] = {
                                     ^
>> drivers/acpi/acpi_apd.c:61:32: warning: 'amd_dw8250' defined but not used [-Wunused-variable]
    static struct plat_dw8250_data amd_dw8250 = {
                                   ^

vim +/amd_dw8250 +61 drivers/acpi/acpi_apd.c

    36	 */
    37	#define ACPI_APD_SYSFS	BIT(0)
    38	#define ACPI_APD_PM	BIT(1)
    39	
    40	static u8 peri_id[2] = { 0, 1 };
    41	
  > 42	static struct dma_pl330_platdata amd_pl330[] = {
    43		{
    44			.nr_valid_peri = 2,
    45			.peri_id = peri_id,
    46			.mcbuf_sz = 0,
    47			.flags = IRQF_SHARED,
    48			.base_request_line = 1,
    49			.num = 0,
    50		},
    51		{
    52			.nr_valid_peri = 2,
    53			.peri_id = peri_id,
    54			.mcbuf_sz = 0,
    55			.flags = IRQF_SHARED,
    56			.base_request_line = 2,
    57			.num = 0,
    58		}
    59	};
    60	
  > 61	static struct plat_dw8250_data amd_dw8250 = {
    62		.has_pl330_dma = 1,
    63	};
    64	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 19026 bytes --]

Re: [PATCH 6/6] Serial:8250: New Port Type PORT_AMD_8250

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]

Hi Wang,

[auto build test WARNING on v4.4-rc8]
[cannot apply to tty/tty-testing pm/linux-next next-20151231]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Wang-Hongcheng/8250-AMD-Carrizo-UART-PL300-DMA-enablement/20160104-133639
config: x86_64-randconfig-x013-01040119 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/acpi/acpi_apd.c:42:34: warning: 'amd_pl330' defined but not used [-Wunused-variable]
    static struct dma_pl330_platdata amd_pl330[] = {
                                     ^
>> drivers/acpi/acpi_apd.c:61:32: warning: 'amd_dw8250' defined but not used [-Wunused-variable]
    static struct plat_dw8250_data amd_dw8250 = {
                                   ^

vim +/amd_dw8250 +61 drivers/acpi/acpi_apd.c

    36	 */
    37	#define ACPI_APD_SYSFS	BIT(0)
    38	#define ACPI_APD_PM	BIT(1)
    39	
    40	static u8 peri_id[2] = { 0, 1 };
    41	
  > 42	static struct dma_pl330_platdata amd_pl330[] = {
    43		{
    44			.nr_valid_peri = 2,
    45			.peri_id = peri_id,
    46			.mcbuf_sz = 0,
    47			.flags = IRQF_SHARED,
    48			.base_request_line = 1,
    49			.num = 0,
    50		},
    51		{
    52			.nr_valid_peri = 2,
    53			.peri_id = peri_id,
    54			.mcbuf_sz = 0,
    55			.flags = IRQF_SHARED,
    56			.base_request_line = 2,
    57			.num = 0,
    58		}
    59	};
    60	
  > 61	static struct plat_dw8250_data amd_dw8250 = {
    62		.has_pl330_dma = 1,
    63	};
    64	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 19026 bytes --]

Re: [PATCH 6/6] Serial:8250: New Port Type PORT_AMD_8250

[Heikki Krogerus]

Hi Hongcheng,

My comments below..

On Mon, Jan 04, 2016 at 01:31:41PM +0800, Wang Hongcheng wrote:
> Set a new port type for AMD Carrizo.  Add has_pl330_dma to 8250_dw's
> private data and init fcr,ier as well as dma rx size.
> 
> Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
> ---
>  drivers/acpi/acpi_apd.c               | 11 +++++++++++
>  drivers/tty/serial/8250/8250_dw.c     | 15 +++++++++++++++
>  drivers/tty/serial/8250/8250_port.c   |  9 +++++++++
>  include/linux/platform_data/8250-dw.h |  8 ++++++++
>  include/uapi/linux/serial_core.h      |  3 ++-
>  include/uapi/linux/serial_reg.h       |  1 +
>  6 files changed, 46 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/platform_data/8250-dw.h
> 
> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> index 1f16cca..e8e43fb 100644
> --- a/drivers/acpi/acpi_apd.c
> +++ b/drivers/acpi/acpi_apd.c
> @@ -23,6 +23,7 @@
>  #include <linux/dmaengine.h>
>  #include <linux/interrupt.h>
>  #include <linux/amba/pl330.h>
> +#include <linux/platform_data/8250-dw.h>
>  
>  #include "internal.h"
>  
> @@ -56,6 +57,11 @@ static struct dma_pl330_platdata amd_pl330[] = {
>  		.num = 0,
>  	}
>  };
> +
> +static struct plat_dw8250_data amd_dw8250 = {
> +	.has_pl330_dma = 1,
> +};

Why do you need this? Can't you identify this platform in 8250_dw.c
based on the ACPI ID?

>  /**
>   * struct apd_device_desc - a descriptor for apd device
>   * @flags: device flags like %ACPI_APD_SYSFS, %ACPI_APD_PM
> @@ -115,6 +121,11 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
>  	char amba_devname[100];
>  	int devnum;
>  
> +	ret = platform_device_add_data(pdev, &amd_dw8250,
> +				       sizeof(amd_dw8250));
> +	if (ret)
> +		goto resource_alloc_err;
> +
>  	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
>  	if (!resource)
>  		goto resource_alloc_err;

The above needs to be separated to its own patch in any case.


> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index a5d319e..e7d9f01 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -27,6 +27,7 @@
>  #include <linux/clk.h>
>  #include <linux/reset.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/platform_data/8250-dw.h>
>  
>  #include <asm/byteorder.h>
>  
> @@ -66,6 +67,7 @@ struct dw8250_data {
>  
>  	unsigned int		skip_autocfg:1;
>  	unsigned int		uart_16550_compatible:1;
> +	unsigned int		has_pl330_dma:1;

I don't think there is any use for this flag. You can do all the
platform specific tricks in a single quirk that you add to
dw8250_quirks()..

>  };
>  
>  #define BYT_PRV_CLK			0x800
> @@ -303,6 +305,7 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
>  static void dw8250_setup_port(struct uart_port *p)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(p);
> +	struct dw8250_data *data = p->private_data;
>  	u32 reg;
>  
>  	/*
> @@ -326,6 +329,14 @@ static void dw8250_setup_port(struct uart_port *p)
>  		p->flags |= UPF_FIXED_TYPE;
>  		p->fifosize = DW_UART_CPR_FIFO_SIZE(reg);
>  		up->capabilities = UART_CAP_FIFO;
> +		if (data->has_pl330_dma) {
> +			p->type = PORT_AMD_8250;
> +
> +			up->ier |= UART_IER_PTIME | UART_IER_THRI |
> +				UART_IER_RLSI | UART_IER_RDI;
> +			up->fcr |= UART_FCR_R_TRIG_10 | UART_FCR_T_TRIG_11;
> +			up->tx_loadsz = p->fifosize / 2;

So these need to be set in the quirk. Btw, now up->ier and up->fcr
will just be forgotten when serial8250_register_8250_port() is
called.

The UART_IER_PTIME needs to be set based on THRE_MODE. Add a flag for
that instead of the "has_pl330_dma", and set it in this function based
on the THRE_MODE bit in CPR. Then I think the correct place to fix
up->ier is actually our up->set_termios hook.

> +		}
>  	}
>  
>  	if (reg & DW_UART_CPR_AFCE_MODE)
> @@ -339,6 +350,7 @@ static int dw8250_probe(struct platform_device *pdev)
>  	int irq = platform_get_irq(pdev, 0);
>  	struct uart_port *p = &uart.port;
>  	struct dw8250_data *data;
> +	struct plat_dw8250_data *pdata = dev_get_platdata(&pdev->dev);
>  	int err;
>  	u32 val;
>  
> @@ -468,6 +480,7 @@ static int dw8250_probe(struct platform_device *pdev)
>  		p->handle_irq = NULL;
>  	}
>  
> +	data->has_pl330_dma = pdata ? pdata->has_pl330_dma : 0;
>  	if (!data->skip_autocfg)
>  		dw8250_setup_port(p);
>  
> @@ -475,6 +488,8 @@ static int dw8250_probe(struct platform_device *pdev)
>  	if (p->fifosize) {
>  		data->dma.rxconf.src_maxburst = p->fifosize / 4;
>  		data->dma.txconf.dst_maxburst = p->fifosize / 4;
> +		data->dma.rx_size
> +			= data->has_pl330_dma ? (p->fifosize / 2 + 2) : 0;

This you also need to set in the quirk.

>  		uart.dma = &data->dma;
>  	}
>  
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 52d82d2..b89a326 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -269,6 +269,15 @@ configured less than Maximum supported fifo bytes */
>  		.rxtrig_bytes	= {1, 4, 8, 14},
>  		.flags		= UART_CAP_FIFO,
>  	},
> +	[PORT_AMD_8250] = {
> +		.name		= "AMD_8250",
> +		.fifo_size	= 256,
> +		.tx_loadsz	= 128,
> +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
> +				UART_FCR_T_TRIG_11,
> +		.rxtrig_bytes	= {1, 4, 8},
> +		.flags		= UART_CAP_FIFO,
> +	},

There is no use for this new type. In fact, there is no use for any
new types. If we need to modify fcr for example, we can do that in a
custom up->startup or up->set_termios hook.

>  };
>  
>  /* Uart divisor latch read */
> diff --git a/include/linux/platform_data/8250-dw.h b/include/linux/platform_data/8250-dw.h
> new file mode 100644
> index 0000000..97cdb9d
> --- /dev/null
> +++ b/include/linux/platform_data/8250-dw.h
> @@ -0,0 +1,8 @@
> +#ifndef _PLATFORM_DATA_8250_DW_H
> +#define _PLATFORM_DATA_8250_DW_H
> +
> +struct plat_dw8250_data {
> +	unsigned int has_pl330_dma:1;
> +};
> +
> +#endif

Let's not add any new driver specific platform data files unless we
absolutely have to. In this case there is no need for it. If you
really need to deliver this kind of detail to the driver, you need to
deliver it to the driver with build-in device property. So instead of
platform_device_add_data() you should use device_add_property_set().
Or actually, in linux-next there is already helper for platform bus
called platform_device_add_properties().

But I don't think there is any need for this kind of detail. You can
just identify your platform in 8250_dw.c based on the ACPI ID, no?


Thanks,

-- 
heikki

RE: [PATCH 6/6] Serial:8250: New Port Type PORT_AMD_8250

[Wang, Annie]

Hi Heikki,

>-----Original Message-----
>From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com]
>Sent: Tuesday, January 05, 2016 8:02 PM
>To: Wang, Annie
>Cc: Andy Shevchenko; Vinod Koul; Mika Westerberg; Greg Kroah-Hartman; Rafael
>J. Wysocki; linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>serial@vger.kernel.org; dmaengine@vger.kernel.org; Borislav Petkov; Huang,
>Ray; Wan, Vincent; Xue, Ken; Robin Murphy; Graeme Gregory; Li, Tony; Yu,
>Xiangliang
>Subject: Re: [PATCH 6/6] Serial:8250: New Port Type PORT_AMD_8250
>
>Hi Hongcheng,
>
>My comments below..
>
>On Mon, Jan 04, 2016 at 01:31:41PM +0800, Wang Hongcheng wrote:
>> Set a new port type for AMD Carrizo.  Add has_pl330_dma to 8250_dw's
>> private data and init fcr,ier as well as dma rx size.
>>
>> Signed-off-by: Wang Hongcheng <annie.wang@amd.com>
>> ---
>>  drivers/acpi/acpi_apd.c               | 11 +++++++++++
>>  drivers/tty/serial/8250/8250_dw.c     | 15 +++++++++++++++
>>  drivers/tty/serial/8250/8250_port.c   |  9 +++++++++
>>  include/linux/platform_data/8250-dw.h |  8 ++++++++
>>  include/uapi/linux/serial_core.h      |  3 ++-
>>  include/uapi/linux/serial_reg.h       |  1 +
>>  6 files changed, 46 insertions(+), 1 deletion(-)  create mode 100644
>> include/linux/platform_data/8250-dw.h
>>
>> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c index
>> 1f16cca..e8e43fb 100644
>> --- a/drivers/acpi/acpi_apd.c
>> +++ b/drivers/acpi/acpi_apd.c
>> @@ -23,6 +23,7 @@
>>  #include <linux/dmaengine.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/amba/pl330.h>
>> +#include <linux/platform_data/8250-dw.h>
>>
>>  #include "internal.h"
>>
>> @@ -56,6 +57,11 @@ static struct dma_pl330_platdata amd_pl330[] = {
>>  		.num = 0,
>>  	}
>>  };
>> +
>> +static struct plat_dw8250_data amd_dw8250 = {
>> +	.has_pl330_dma = 1,
>> +};
>
>Why do you need this? Can't you identify this platform in 8250_dw.c based on the
>ACPI ID?
>
>>  /**
>>   * struct apd_device_desc - a descriptor for apd device
>>   * @flags: device flags like %ACPI_APD_SYSFS, %ACPI_APD_PM @@ -115,6
>> +121,11 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
>>  	char amba_devname[100];
>>  	int devnum;
>>
>> +	ret = platform_device_add_data(pdev, &amd_dw8250,
>> +				       sizeof(amd_dw8250));
>> +	if (ret)
>> +		goto resource_alloc_err;
>> +
>>  	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
>>  	if (!resource)
>>  		goto resource_alloc_err;
>
>The above needs to be separated to its own patch in any case.
>
>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c
>> b/drivers/tty/serial/8250/8250_dw.c
>> index a5d319e..e7d9f01 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -27,6 +27,7 @@
>>  #include <linux/clk.h>
>>  #include <linux/reset.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/platform_data/8250-dw.h>
>>
>>  #include <asm/byteorder.h>
>>
>> @@ -66,6 +67,7 @@ struct dw8250_data {
>>
>>  	unsigned int		skip_autocfg:1;
>>  	unsigned int		uart_16550_compatible:1;
>> +	unsigned int		has_pl330_dma:1;
>
>I don't think there is any use for this flag. You can do all the platform specific
>tricks in a single quirk that you add to dw8250_quirks()..
>
>>  };
>>
>>  #define BYT_PRV_CLK			0x800
>> @@ -303,6 +305,7 @@ static void dw8250_quirks(struct uart_port *p,
>> struct dw8250_data *data)  static void dw8250_setup_port(struct
>> uart_port *p)  {
>>  	struct uart_8250_port *up = up_to_u8250p(p);
>> +	struct dw8250_data *data = p->private_data;
>>  	u32 reg;
>>
>>  	/*
>> @@ -326,6 +329,14 @@ static void dw8250_setup_port(struct uart_port *p)
>>  		p->flags |= UPF_FIXED_TYPE;
>>  		p->fifosize = DW_UART_CPR_FIFO_SIZE(reg);
>>  		up->capabilities = UART_CAP_FIFO;
>> +		if (data->has_pl330_dma) {
>> +			p->type = PORT_AMD_8250;
>> +
>> +			up->ier |= UART_IER_PTIME | UART_IER_THRI |
>> +				UART_IER_RLSI | UART_IER_RDI;
>> +			up->fcr |= UART_FCR_R_TRIG_10 |
>UART_FCR_T_TRIG_11;
>> +			up->tx_loadsz = p->fifosize / 2;
>
>So these need to be set in the quirk. Btw, now up->ier and up->fcr will just be
>forgotten when serial8250_register_8250_port() is called.
>
>The UART_IER_PTIME needs to be set based on THRE_MODE. Add a flag for that
>instead of the "has_pl330_dma", and set it in this function based on the
>THRE_MODE bit in CPR. Then I think the correct place to fix
>up->ier is actually our up->set_termios hook.
>
>> +		}
>>  	}
>>
>>  	if (reg & DW_UART_CPR_AFCE_MODE)
>> @@ -339,6 +350,7 @@ static int dw8250_probe(struct platform_device *pdev)
>>  	int irq = platform_get_irq(pdev, 0);
>>  	struct uart_port *p = &uart.port;
>>  	struct dw8250_data *data;
>> +	struct plat_dw8250_data *pdata = dev_get_platdata(&pdev->dev);
>>  	int err;
>>  	u32 val;
>>
>> @@ -468,6 +480,7 @@ static int dw8250_probe(struct platform_device *pdev)
>>  		p->handle_irq = NULL;
>>  	}
>>
>> +	data->has_pl330_dma = pdata ? pdata->has_pl330_dma : 0;
>>  	if (!data->skip_autocfg)
>>  		dw8250_setup_port(p);
>>
>> @@ -475,6 +488,8 @@ static int dw8250_probe(struct platform_device *pdev)
>>  	if (p->fifosize) {
>>  		data->dma.rxconf.src_maxburst = p->fifosize / 4;
>>  		data->dma.txconf.dst_maxburst = p->fifosize / 4;
>> +		data->dma.rx_size
>> +			= data->has_pl330_dma ? (p->fifosize / 2 + 2) : 0;
>
>This you also need to set in the quirk.

Yes, I wil set that in the quirk.

>
>>  		uart.dma = &data->dma;
>>  	}
>>
>> diff --git a/drivers/tty/serial/8250/8250_port.c
>> b/drivers/tty/serial/8250/8250_port.c
>> index 52d82d2..b89a326 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -269,6 +269,15 @@ configured less than Maximum supported fifo bytes */
>>  		.rxtrig_bytes	= {1, 4, 8, 14},
>>  		.flags		= UART_CAP_FIFO,
>>  	},
>> +	[PORT_AMD_8250] = {
>> +		.name		= "AMD_8250",
>> +		.fifo_size	= 256,
>> +		.tx_loadsz	= 128,
>> +		.fcr		= UART_FCR_ENABLE_FIFO |
>UART_FCR_R_TRIG_10 |
>> +				UART_FCR_T_TRIG_11,
>> +		.rxtrig_bytes	= {1, 4, 8},
>> +		.flags		= UART_CAP_FIFO,
>> +	},
>
>There is no use for this new type. In fact, there is no use for any new types. If we
>need to modify fcr for example, we can do that in a custom up->startup or up-
>>set_termios hook.
>
>>  };
>>
>>  /* Uart divisor latch read */
>> diff --git a/include/linux/platform_data/8250-dw.h
>> b/include/linux/platform_data/8250-dw.h
>> new file mode 100644
>> index 0000000..97cdb9d
>> --- /dev/null
>> +++ b/include/linux/platform_data/8250-dw.h
>> @@ -0,0 +1,8 @@
>> +#ifndef _PLATFORM_DATA_8250_DW_H
>> +#define _PLATFORM_DATA_8250_DW_H
>> +
>> +struct plat_dw8250_data {
>> +	unsigned int has_pl330_dma:1;
>> +};
>> +
>> +#endif
>
>Let's not add any new driver specific platform data files unless we absolutely have
>to. In this case there is no need for it. If you really need to deliver this kind of
>detail to the driver, you need to deliver it to the driver with build-in device
>property. So instead of
>platform_device_add_data() you should use device_add_property_set().
>Or actually, in linux-next there is already helper for platform bus called
>platform_device_add_properties().
>
>But I don't think there is any need for this kind of detail. You can just identify your
>platform in 8250_dw.c based on the ACPI ID, no?

Agree.

The uart and uart dma could work well, if I just set dma.rx_size in dw8250_quirks. 

Thank you for your comments.  

Regards,
Hongcheng(Annie)