diff mbox

[RFC,v2,2/7] arm64: Add definitions for fwnode_handle

Message ID 1505954230-18892-3-git-send-email-sgoel@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Goel, Sameer Sept. 21, 2017, 12:37 a.m. UTC 
This will be used as a device property to match the DMA capable devices
with the associated SMMU. The header file is a port from linux. The code
was changed to remove the types that were not needed for Xen.

Linux ChangeId:ce793486e23e: driver core / ACPI: Represent ACPI
companions using fwnode_handle

Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
---
 xen/include/asm-arm/device.h |  2 ++
 xen/include/xen/fwnode.h     | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 xen/include/xen/fwnode.h

Comments

Julien Grall Oct. 12, 2017, 12:45 p.m. UTC | #1 
Hi,

On 21/09/17 01:37, Sameer Goel wrote:
> This will be used as a device property to match the DMA capable devices
> with the associated SMMU. The header file is a port from linux. The code
> was changed to remove the types that were not needed for Xen.

I think you probably want a bit more context in the commit message about 
implement fwnode.h in common code.

Within this series, fwnode seems to only be used by Arm. So what would 
be the advantage to get that in xen/? Is it going to be used by x86 or 
taken advantage in common code?

> 
> Linux ChangeId:ce793486e23e: driver core / ACPI: Represent ACPI
> companions using fwnode_handle
> 
> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
> ---
>   xen/include/asm-arm/device.h |  2 ++
>   xen/include/xen/fwnode.h     | 33 +++++++++++++++++++++++++++++++++
>   2 files changed, 35 insertions(+)
>   create mode 100644 xen/include/xen/fwnode.h
> 
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 6734ae8..78c38fe 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -2,6 +2,7 @@
>   #define __ASM_ARM_DEVICE_H
>   
>   #include <xen/init.h>
> +#include <xen/fwnode.h>
>   
>   enum device_type
>   {
> @@ -19,6 +20,7 @@ struct device
>   #ifdef CONFIG_HAS_DEVICE_TREE
>       struct dt_device_node *of_node; /* Used by drivers imported from Linux */

I was expecting a todo in the code after the discussion about leave 
of_node here.

>   #endif
> +    struct fwnode_handle *fwnode; /*fw device node identifier */

Space missing before "fw".

>       struct dev_archdata archdata;
>   };
>   
> diff --git a/xen/include/xen/fwnode.h b/xen/include/xen/fwnode.h
> new file mode 100644
> index 0000000..0fed958
> --- /dev/null
> +++ b/xen/include/xen/fwnode.h
> @@ -0,0 +1,33 @@
> +/*
> + * fwnode.h - Firmware device node object handle type definition.
> + *
> + * Copyright (C) 2015, Intel Corporation
> + * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.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
> + * published by the Free Software Foundation.
> + *
> + * Ported from Linux include/linux/fwnode.h
> + *  => commit ce793486e23e0162a732c605189c8028e0910e86
> + *
> + * No functional Xen modifications.
> + */
> +
> +#ifndef __XEN_FWNODE_H_
> +#define __XEN_FWNODE_H_
> +
> +enum fwnode_type {
> +	FWNODE_INVALID = 0,
> +	FWNODE_OF,
> +	FWNODE_ACPI,
> +	FWNODE_ACPI_STATIC,
> +	FWNODE_IRQCHIP
> +};
> +

Looking at Linux code, the fwnode_type already disappeared from Linux 
(see commit db3e50f3234b "device property: Get rid of struct 
fwnode_handle type field").

I understand the goal on using fwnode is to help porting drivers from 
Linux. So how much this has changed now?

Cheers,

> +struct fwnode_handle {
> +	enum fwnode_type type;
> +	struct fwnode_handle *secondary;
> +};
> +
> +#endif
>
Goel, Sameer Oct. 19, 2017, 2:53 p.m. UTC | #2 
On 10/12/2017 6:45 AM, Julien Grall wrote:
> Hi,
> 
> On 21/09/17 01:37, Sameer Goel wrote:
>> This will be used as a device property to match the DMA capable devices
>> with the associated SMMU. The header file is a port from linux. The code
>> was changed to remove the types that were not needed for Xen.
> 
> I think you probably want a bit more context in the commit message about implement fwnode.h in common code.
> 
> Within this series, fwnode seems to only be used by Arm. So what would be the advantage to get that in xen/? Is it going to be used by x86 or taken advantage in common code?
> 
>>
>> Linux ChangeId:ce793486e23e: driver core / ACPI: Represent ACPI
>> companions using fwnode_handle
>>
>> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
>> ---
>>   xen/include/asm-arm/device.h |  2 ++
>>   xen/include/xen/fwnode.h     | 33 +++++++++++++++++++++++++++++++++
>>   2 files changed, 35 insertions(+)
>>   create mode 100644 xen/include/xen/fwnode.h
>>
>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>> index 6734ae8..78c38fe 100644
>> --- a/xen/include/asm-arm/device.h
>> +++ b/xen/include/asm-arm/device.h
>> @@ -2,6 +2,7 @@
>>   #define __ASM_ARM_DEVICE_H
>>     #include <xen/init.h>
>> +#include <xen/fwnode.h>
>>     enum device_type
>>   {
>> @@ -19,6 +20,7 @@ struct device
>>   #ifdef CONFIG_HAS_DEVICE_TREE
>>       struct dt_device_node *of_node; /* Used by drivers imported from Linux */
> 
> I was expecting a todo in the code after the discussion about leave of_node here.
> 
>>   #endif
>> +    struct fwnode_handle *fwnode; /*fw device node identifier */
The fwnode handle was provide a match cookie for the SMMUs and not much else. Even with this around we will need the 
dt info in the device node. I agree that this rolls up into fw spec and I can look at the code cleanup for the next patch.
> 
> Space missing before "fw".
> 
>>       struct dev_archdata archdata;
>>   };
>>   diff --git a/xen/include/xen/fwnode.h b/xen/include/xen/fwnode.h
>> new file mode 100644
>> index 0000000..0fed958
>> --- /dev/null
>> +++ b/xen/include/xen/fwnode.h
>> @@ -0,0 +1,33 @@
>> +/*
>> + * fwnode.h - Firmware device node object handle type definition.
>> + *
>> + * Copyright (C) 2015, Intel Corporation
>> + * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.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
>> + * published by the Free Software Foundation.
>> + *
>> + * Ported from Linux include/linux/fwnode.h
>> + *  => commit ce793486e23e0162a732c605189c8028e0910e86
>> + *
>> + * No functional Xen modifications.
>> + */
>> +
>> +#ifndef __XEN_FWNODE_H_
>> +#define __XEN_FWNODE_H_
>> +
>> +enum fwnode_type {
>> +    FWNODE_INVALID = 0,
>> +    FWNODE_OF,
>> +    FWNODE_ACPI,
>> +    FWNODE_ACPI_STATIC,
>> +    FWNODE_IRQCHIP
>> +};
>> +
> 
> Looking at Linux code, the fwnode_type already disappeared from Linux (see commit db3e50f3234b "device property: Get rid of struct fwnode_handle type field").
> 
> I understand the goal on using fwnode is to help porting drivers from Linux. So how much this has changed now?
This was not very useful in any case. So, I will move over to the new version.

> 
> Cheers,
> 
>> +struct fwnode_handle {
>> +    enum fwnode_type type;
>> +    struct fwnode_handle *secondary;
>> +};
>> +
>> +#endif
>>
>
Julien Grall Oct. 24, 2017, 2:08 p.m. UTC | #3 
Hi Sameer,

On 19/10/17 15:53, Goel, Sameer wrote:
> On 10/12/2017 6:45 AM, Julien Grall wrote:
>> On 21/09/17 01:37, Sameer Goel wrote:
>>> This will be used as a device property to match the DMA capable devices
>>> with the associated SMMU. The header file is a port from linux. The code
>>> was changed to remove the types that were not needed for Xen.
>>
>> I think you probably want a bit more context in the commit message about implement fwnode.h in common code.
>>
>> Within this series, fwnode seems to only be used by Arm. So what would be the advantage to get that in xen/? Is it going to be used by x86 or taken advantage in common code?
>>
>>>
>>> Linux ChangeId:ce793486e23e: driver core / ACPI: Represent ACPI
>>> companions using fwnode_handle
>>>
>>> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
>>> ---
>>>    xen/include/asm-arm/device.h |  2 ++
>>>    xen/include/xen/fwnode.h     | 33 +++++++++++++++++++++++++++++++++
>>>    2 files changed, 35 insertions(+)
>>>    create mode 100644 xen/include/xen/fwnode.h
>>>
>>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>>> index 6734ae8..78c38fe 100644
>>> --- a/xen/include/asm-arm/device.h
>>> +++ b/xen/include/asm-arm/device.h
>>> @@ -2,6 +2,7 @@
>>>    #define __ASM_ARM_DEVICE_H
>>>      #include <xen/init.h>
>>> +#include <xen/fwnode.h>
>>>      enum device_type
>>>    {
>>> @@ -19,6 +20,7 @@ struct device
>>>    #ifdef CONFIG_HAS_DEVICE_TREE
>>>        struct dt_device_node *of_node; /* Used by drivers imported from Linux */
>>
>> I was expecting a todo in the code after the discussion about leave of_node here.
>>
>>>    #endif
>>> +    struct fwnode_handle *fwnode; /*fw device node identifier */
> The fwnode handle was provide a match cookie for the SMMUs and not much else. Even with this around we will need the
> dt info in the device node. I agree that this rolls up into fw spec and I can look at the code cleanup for the next patch.

A clean-up patch would be great but not necessary. What I expect is a 
TODO mentioning the possible clean-up.

Cheers,
Goel, Sameer Nov. 9, 2017, 12:56 a.m. UTC | #4 
On 10/24/2017 8:08 AM, Julien Grall wrote:
> Hi Sameer,
> 
> On 19/10/17 15:53, Goel, Sameer wrote:
>> On 10/12/2017 6:45 AM, Julien Grall wrote:
>>> On 21/09/17 01:37, Sameer Goel wrote:
>>>> This will be used as a device property to match the DMA capable devices
>>>> with the associated SMMU. The header file is a port from linux. The code
>>>> was changed to remove the types that were not needed for Xen.
>>>
>>> I think you probably want a bit more context in the commit message about implement fwnode.h in common code.
>>>
>>> Within this series, fwnode seems to only be used by Arm. So what would be the advantage to get that in xen/? Is it going to be used by x86 or taken advantage in common code?
>>>
>>>>
>>>> Linux ChangeId:ce793486e23e: driver core / ACPI: Represent ACPI
>>>> companions using fwnode_handle
>>>>
>>>> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
>>>> ---
>>>>    xen/include/asm-arm/device.h |  2 ++
>>>>    xen/include/xen/fwnode.h     | 33 +++++++++++++++++++++++++++++++++
>>>>    2 files changed, 35 insertions(+)
>>>>    create mode 100644 xen/include/xen/fwnode.h
>>>>
>>>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>>>> index 6734ae8..78c38fe 100644
>>>> --- a/xen/include/asm-arm/device.h
>>>> +++ b/xen/include/asm-arm/device.h
>>>> @@ -2,6 +2,7 @@
>>>>    #define __ASM_ARM_DEVICE_H
>>>>      #include <xen/init.h>
>>>> +#include <xen/fwnode.h>
>>>>      enum device_type
>>>>    {
>>>> @@ -19,6 +20,7 @@ struct device
>>>>    #ifdef CONFIG_HAS_DEVICE_TREE
>>>>        struct dt_device_node *of_node; /* Used by drivers imported from Linux */
>>>
>>> I was expecting a todo in the code after the discussion about leave of_node here.
>>>
>>>>    #endif
>>>> +    struct fwnode_handle *fwnode; /*fw device node identifier */
>> The fwnode handle was provide a match cookie for the SMMUs and not much else. Even with this around we will need the
>> dt info in the device node. I agree that this rolls up into fw spec and I can look at the code cleanup for the next patch.
> 
> A clean-up patch would be great but not necessary. What I expect is a TODO mentioning the possible clean-up.
> 
I looked through the Linux code a bit more. The reason for pulling in the fwnode was to port the iort code as is. So, really I do not need this at all. All I need is a dummy struct for fwnode.

I will remove this header file and add a new definition in the linux compat header. What is a good location for this header?
> Cheers,
>
diff mbox

Patch

diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 6734ae8..78c38fe 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -2,6 +2,7 @@ 
 #define __ASM_ARM_DEVICE_H
 
 #include <xen/init.h>
+#include <xen/fwnode.h>
 
 enum device_type
 {
@@ -19,6 +20,7 @@  struct device
 #ifdef CONFIG_HAS_DEVICE_TREE
     struct dt_device_node *of_node; /* Used by drivers imported from Linux */
 #endif
+    struct fwnode_handle *fwnode; /*fw device node identifier */
     struct dev_archdata archdata;
 };
 
diff --git a/xen/include/xen/fwnode.h b/xen/include/xen/fwnode.h
new file mode 100644
index 0000000..0fed958
--- /dev/null
+++ b/xen/include/xen/fwnode.h
@@ -0,0 +1,33 @@ 
+/*
+ * fwnode.h - Firmware device node object handle type definition.
+ *
+ * Copyright (C) 2015, Intel Corporation
+ * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.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
+ * published by the Free Software Foundation.
+ *
+ * Ported from Linux include/linux/fwnode.h
+ *  => commit ce793486e23e0162a732c605189c8028e0910e86
+ *
+ * No functional Xen modifications.
+ */
+
+#ifndef __XEN_FWNODE_H_
+#define __XEN_FWNODE_H_
+
+enum fwnode_type {
+	FWNODE_INVALID = 0,
+	FWNODE_OF,
+	FWNODE_ACPI,
+	FWNODE_ACPI_STATIC,
+	FWNODE_IRQCHIP
+};
+
+struct fwnode_handle {
+	enum fwnode_type type;
+	struct fwnode_handle *secondary;
+};
+
+#endif