[v5,5/5] Driver: VMBus: Add device tree support
Commit Message
Update the driver to support device tree boot as well along with ACPI.
At present the device tree parsing only provides the mmio region info
and is not the exact copy of ACPI parsing. This is sufficient to cater
all the current device tree usecases for VMBus.
Currently device tree is supported only for x86 systems.
Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
- Removed #else for device tree parsing code. This should help better
test coverage.
- Fix macro '__maybe_unused' warning
- Added below options in Kconfig to enable device tree options for HYPERV
select OF if !ACPI
select OF_EARLY_FLATTREE if !ACPI
drivers/hv/Kconfig | 6 +++--
drivers/hv/vmbus_drv.c | 60 ++++++++++++++++++++++++++++++++++++++++--
2 files changed, 62 insertions(+), 4 deletions(-)
Comments
On Thu, Feb 9, 2023 at 6:15 AM Saurabh Sengar
<ssengar@linux.microsoft.com> wrote:
>
> Update the driver to support device tree boot as well along with ACPI.
Devicetree
> At present the device tree parsing only provides the mmio region info
Devicetree
And anywhere else.
> and is not the exact copy of ACPI parsing. This is sufficient to cater
> all the current device tree usecases for VMBus.
>
> Currently device tree is supported only for x86 systems.
>
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
> - Removed #else for device tree parsing code. This should help better
> test coverage.
> - Fix macro '__maybe_unused' warning
> - Added below options in Kconfig to enable device tree options for HYPERV
> select OF if !ACPI
> select OF_EARLY_FLATTREE if !ACPI
>
> drivers/hv/Kconfig | 6 +++--
> drivers/hv/vmbus_drv.c | 60 ++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 62 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> index 0747a8f1fcee..1a55bf32d195 100644
> --- a/drivers/hv/Kconfig
> +++ b/drivers/hv/Kconfig
> @@ -4,11 +4,13 @@ menu "Microsoft Hyper-V guest support"
>
> config HYPERV
> tristate "Microsoft Hyper-V client drivers"
> - depends on ACPI && ((X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \
> - || (ARM64 && !CPU_BIG_ENDIAN))
> + depends on (X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \
> + || (ACPI && ARM64 && !CPU_BIG_ENDIAN)
> select PARAVIRT
> select X86_HV_CALLBACK_VECTOR if X86
> select VMAP_PFN
> + select OF if !ACPI
> + select OF_EARLY_FLATTREE if !ACPI
> help
> Select this option to run Linux as a Hyper-V client operating
> system.
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 73497157a23a..02f6bab61c37 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -20,6 +20,7 @@
> #include <linux/completion.h>
> #include <linux/hyperv.h>
> #include <linux/kernel_stat.h>
> +#include <linux/of_address.h>
> #include <linux/clockchips.h>
> #include <linux/cpu.h>
> #include <linux/sched/isolation.h>
> @@ -2152,7 +2153,7 @@ void vmbus_device_unregister(struct hv_device *device_obj)
> device_unregister(&device_obj->device);
> }
>
> -
> +#ifdef CONFIG_ACPI
> /*
> * VMBUS is an acpi enumerated device. Get the information we
> * need from DSDT.
> @@ -2262,6 +2263,7 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
>
> return AE_OK;
> }
> +#endif
>
> static void vmbus_mmio_remove(void)
> {
> @@ -2282,7 +2284,7 @@ static void vmbus_mmio_remove(void)
> }
> }
>
> -static void vmbus_reserve_fb(void)
> +static void __maybe_unused vmbus_reserve_fb(void)
> {
> resource_size_t start = 0, size;
> struct pci_dev *pdev;
> @@ -2442,6 +2444,7 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
> }
> EXPORT_SYMBOL_GPL(vmbus_free_mmio);
>
> +#ifdef CONFIG_ACPI
> static int vmbus_acpi_add(struct platform_device *pdev)
> {
> acpi_status result;
> @@ -2494,10 +2497,50 @@ static int vmbus_acpi_add(struct platform_device *pdev)
> vmbus_mmio_remove();
> return ret_val;
> }
> +#endif
> +
> +static int vmbus_device_add(struct platform_device *pdev)
> +{
> + struct resource **cur_res = &hyperv_mmio;
> + struct of_range range;
> + struct of_range_parser parser;
> + struct device_node *np;
> + int ret = 0;
No need to initialize.
> +
> + hv_dev = &pdev->dev;
> + np = pdev->dev.of_node;
Set this on the declaration.
> +
> + ret = of_range_parser_init(&parser, np);
> + if (ret) {
> + dev_err(hv_dev, "Failed to parse resources.\n");
If a print is needed, put it in of_range_parser_init().
> + return ret;
> + }
> +
> + for_each_of_range(&parser, &range) {
> + struct resource *res;
> +
> + res = kzalloc(sizeof(*res), GFP_ATOMIC);
> + if (!res)
> + return -ENOMEM;
> +
> + res->name = "hyperv mmio";
> + res->flags = IORESOURCE_MEM | IORESOURCE_MEM_64;
> + res->start = range.pci_addr;
This is not PCI. It's a union, so use 'bus_addr' instead.
But wait, resources and IORESOURCE_MEM are *CPU* addresses. You need
cpu_addr here. Your DT happens to do 1:1 addresses so it happens to
work either way.
Rob
On Thu, Feb 09, 2023 at 09:50:31AM -0600, Rob Herring wrote:
> On Thu, Feb 9, 2023 at 6:15 AM Saurabh Sengar
> <ssengar@linux.microsoft.com> wrote:
> >
> > Update the driver to support device tree boot as well along with ACPI.
>
> Devicetree
>
> > At present the device tree parsing only provides the mmio region info
>
> Devicetree
>
> And anywhere else.
OK
>
> > and is not the exact copy of ACPI parsing. This is sufficient to cater
> > all the current device tree usecases for VMBus.
> >
> > Currently device tree is supported only for x86 systems.
> >
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> > - Removed #else for device tree parsing code. This should help better
> > test coverage.
> > - Fix macro '__maybe_unused' warning
> > - Added below options in Kconfig to enable device tree options for HYPERV
> > select OF if !ACPI
> > select OF_EARLY_FLATTREE if !ACPI
> >
> > drivers/hv/Kconfig | 6 +++--
> > drivers/hv/vmbus_drv.c | 60 ++++++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 62 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> > index 0747a8f1fcee..1a55bf32d195 100644
> > --- a/drivers/hv/Kconfig
> > +++ b/drivers/hv/Kconfig
> > @@ -4,11 +4,13 @@ menu "Microsoft Hyper-V guest support"
> >
> > config HYPERV
> > tristate "Microsoft Hyper-V client drivers"
> > - depends on ACPI && ((X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \
> > - || (ARM64 && !CPU_BIG_ENDIAN))
> > + depends on (X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \
> > + || (ACPI && ARM64 && !CPU_BIG_ENDIAN)
> > select PARAVIRT
> > select X86_HV_CALLBACK_VECTOR if X86
> > select VMAP_PFN
> > + select OF if !ACPI
> > + select OF_EARLY_FLATTREE if !ACPI
> > help
> > Select this option to run Linux as a Hyper-V client operating
> > system.
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index 73497157a23a..02f6bab61c37 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -20,6 +20,7 @@
> > #include <linux/completion.h>
> > #include <linux/hyperv.h>
> > #include <linux/kernel_stat.h>
> > +#include <linux/of_address.h>
> > #include <linux/clockchips.h>
> > #include <linux/cpu.h>
> > #include <linux/sched/isolation.h>
> > @@ -2152,7 +2153,7 @@ void vmbus_device_unregister(struct hv_device *device_obj)
> > device_unregister(&device_obj->device);
> > }
> >
> > -
> > +#ifdef CONFIG_ACPI
> > /*
> > * VMBUS is an acpi enumerated device. Get the information we
> > * need from DSDT.
> > @@ -2262,6 +2263,7 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
> >
> > return AE_OK;
> > }
> > +#endif
> >
> > static void vmbus_mmio_remove(void)
> > {
> > @@ -2282,7 +2284,7 @@ static void vmbus_mmio_remove(void)
> > }
> > }
> >
> > -static void vmbus_reserve_fb(void)
> > +static void __maybe_unused vmbus_reserve_fb(void)
> > {
> > resource_size_t start = 0, size;
> > struct pci_dev *pdev;
> > @@ -2442,6 +2444,7 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
> > }
> > EXPORT_SYMBOL_GPL(vmbus_free_mmio);
> >
> > +#ifdef CONFIG_ACPI
> > static int vmbus_acpi_add(struct platform_device *pdev)
> > {
> > acpi_status result;
> > @@ -2494,10 +2497,50 @@ static int vmbus_acpi_add(struct platform_device *pdev)
> > vmbus_mmio_remove();
> > return ret_val;
> > }
> > +#endif
> > +
> > +static int vmbus_device_add(struct platform_device *pdev)
> > +{
> > + struct resource **cur_res = &hyperv_mmio;
> > + struct of_range range;
> > + struct of_range_parser parser;
> > + struct device_node *np;
> > + int ret = 0;
>
> No need to initialize.
OK
>
> > +
> > + hv_dev = &pdev->dev;
> > + np = pdev->dev.of_node;
>
> Set this on the declaration.
OK
>
> > +
> > + ret = of_range_parser_init(&parser, np);
> > + if (ret) {
> > + dev_err(hv_dev, "Failed to parse resources.\n");
>
> If a print is needed, put it in of_range_parser_init().
Will remove the print
>
> > + return ret;
> > + }
> > +
> > + for_each_of_range(&parser, &range) {
> > + struct resource *res;
> > +
> > + res = kzalloc(sizeof(*res), GFP_ATOMIC);
> > + if (!res)
> > + return -ENOMEM;
> > +
> > + res->name = "hyperv mmio";
> > + res->flags = IORESOURCE_MEM | IORESOURCE_MEM_64;
> > + res->start = range.pci_addr;
>
> This is not PCI. It's a union, so use 'bus_addr' instead.
>
> But wait, resources and IORESOURCE_MEM are *CPU* addresses. You need
> cpu_addr here. Your DT happens to do 1:1 addresses so it happens to
> work either way.
bus_addr works for us, will send V6
Regards,
Saurabh
>
> Rob
On Thu, Feb 9, 2023 at 11:46 AM Saurabh Singh Sengar
<ssengar@linux.microsoft.com> wrote:
>
> On Thu, Feb 09, 2023 at 09:50:31AM -0600, Rob Herring wrote:
> > On Thu, Feb 9, 2023 at 6:15 AM Saurabh Sengar
> > <ssengar@linux.microsoft.com> wrote:
> > >
> > > Update the driver to support device tree boot as well along with ACPI.
> >
> > Devicetree
[...]
> > > + for_each_of_range(&parser, &range) {
> > > + struct resource *res;
> > > +
> > > + res = kzalloc(sizeof(*res), GFP_ATOMIC);
> > > + if (!res)
> > > + return -ENOMEM;
> > > +
> > > + res->name = "hyperv mmio";
> > > + res->flags = IORESOURCE_MEM | IORESOURCE_MEM_64;
> > > + res->start = range.pci_addr;
> >
> > This is not PCI. It's a union, so use 'bus_addr' instead.
> >
> > But wait, resources and IORESOURCE_MEM are *CPU* addresses. You need
> > cpu_addr here. Your DT happens to do 1:1 addresses so it happens to
> > work either way.
>
> bus_addr works for us, will send V6
Sigh. bus_addr may work, but is wrong as I explained.
And you've already sent v6... Please slow down your pace with sending
new versions. 4 versions in a week is too much. Give others time to
comment and me to respond to discussions. Like a week...
Rob
On Fri, Feb 10, 2023 at 04:37:28PM -0600, Rob Herring wrote:
> On Thu, Feb 9, 2023 at 11:46 AM Saurabh Singh Sengar
> <ssengar@linux.microsoft.com> wrote:
> >
> > On Thu, Feb 09, 2023 at 09:50:31AM -0600, Rob Herring wrote:
> > > On Thu, Feb 9, 2023 at 6:15 AM Saurabh Sengar
> > > <ssengar@linux.microsoft.com> wrote:
> > > >
> > > > Update the driver to support device tree boot as well along with ACPI.
> > >
> > > Devicetree
>
> [...]
>
> > > > + for_each_of_range(&parser, &range) {
> > > > + struct resource *res;
> > > > +
> > > > + res = kzalloc(sizeof(*res), GFP_ATOMIC);
> > > > + if (!res)
> > > > + return -ENOMEM;
> > > > +
> > > > + res->name = "hyperv mmio";
> > > > + res->flags = IORESOURCE_MEM | IORESOURCE_MEM_64;
> > > > + res->start = range.pci_addr;
> > >
> > > This is not PCI. It's a union, so use 'bus_addr' instead.
> > >
> > > But wait, resources and IORESOURCE_MEM are *CPU* addresses. You need
> > > cpu_addr here. Your DT happens to do 1:1 addresses so it happens to
> > > work either way.
> >
> > bus_addr works for us, will send V6
>
> Sigh. bus_addr may work, but is wrong as I explained.
>
> And you've already sent v6... Please slow down your pace with sending
> new versions. 4 versions in a week is too much. Give others time to
> comment and me to respond to discussions. Like a week...
I apologize if my actions may have come across as overly hasty. I will make
sure to allow for more time between submissions in the future, to ensure that
everyone has an adequate opportunity to review and provide feedback.
Regarding the use of bus_addr instead of cpu_addr, I found that cpu_addr was
populating as OF_BAD_ADDR while bus_addr was populating correctly. I think
this is because I should be defining a empty ranges property in parent node
for indicating 1:1 mapping between parent and child.
But once I add empty ranges in property I get other warnings by dt_binding_check
tool. After fixing all I am able to come up with below device tree example, please
let me know if there is anything to be corrected. If this is good I will send
the next version (offcource after a week :)) using cpu_addr.
soc {
#address-cells = <2>;
#size-cells = <1>;
bus {
compatible = "simple-bus";
#address-cells = <2>;
#size-cells = <1>;
ranges;
vmbus@ff0000000 {
compatible = "microsoft,vmbus";
#address-cells = <2>;
#size-cells = <1>;
ranges = <0x0f 0xf0000000 0x0f 0xf0000000 0x10000000>;
};
};
};
>
> Rob
On Mon, Feb 13, 2023 at 03:24:39AM -0800, Saurabh Singh Sengar wrote:
> On Fri, Feb 10, 2023 at 04:37:28PM -0600, Rob Herring wrote:
> > On Thu, Feb 9, 2023 at 11:46 AM Saurabh Singh Sengar
> > <ssengar@linux.microsoft.com> wrote:
> > >
> > > On Thu, Feb 09, 2023 at 09:50:31AM -0600, Rob Herring wrote:
> > > > On Thu, Feb 9, 2023 at 6:15 AM Saurabh Sengar
> > > > <ssengar@linux.microsoft.com> wrote:
> > > > >
> > > > > Update the driver to support device tree boot as well along with ACPI.
> > > >
> > > > Devicetree
> >
> > [...]
> >
> > > > > + for_each_of_range(&parser, &range) {
> > > > > + struct resource *res;
> > > > > +
> > > > > + res = kzalloc(sizeof(*res), GFP_ATOMIC);
> > > > > + if (!res)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + res->name = "hyperv mmio";
> > > > > + res->flags = IORESOURCE_MEM | IORESOURCE_MEM_64;
> > > > > + res->start = range.pci_addr;
> > > >
> > > > This is not PCI. It's a union, so use 'bus_addr' instead.
> > > >
> > > > But wait, resources and IORESOURCE_MEM are *CPU* addresses. You need
> > > > cpu_addr here. Your DT happens to do 1:1 addresses so it happens to
> > > > work either way.
> > >
> > > bus_addr works for us, will send V6
> >
> > Sigh. bus_addr may work, but is wrong as I explained.
> >
> > And you've already sent v6... Please slow down your pace with sending
> > new versions. 4 versions in a week is too much. Give others time to
> > comment and me to respond to discussions. Like a week...
>
> I apologize if my actions may have come across as overly hasty. I will make
> sure to allow for more time between submissions in the future, to ensure that
> everyone has an adequate opportunity to review and provide feedback.
>
> Regarding the use of bus_addr instead of cpu_addr, I found that cpu_addr was
> populating as OF_BAD_ADDR while bus_addr was populating correctly. I think
> this is because I should be defining a empty ranges property in parent node
> for indicating 1:1 mapping between parent and child.
>
> But once I add empty ranges in property I get other warnings by dt_binding_check
> tool. After fixing all I am able to come up with below device tree example, please
> let me know if there is anything to be corrected. If this is good I will send
> the next version (offcource after a week :)) using cpu_addr.
>
> soc {
> #address-cells = <2>;
> #size-cells = <1>;
> bus {
> compatible = "simple-bus";
> #address-cells = <2>;
> #size-cells = <1>;
> ranges;
>
> vmbus@ff0000000 {
> compatible = "microsoft,vmbus";
> #address-cells = <2>;
> #size-cells = <1>;
> ranges = <0x0f 0xf0000000 0x0f 0xf0000000 0x10000000>;
> };
> };
> };
>
> >
Rob,
If you find the above changes satisfactory, please do let me know so that
I can send the updated version.
As far as I am aware, all the pending comments and issues have been addressed
in this series. However, if there are any remaining concerns or feedback that
require attention, please let me know.
- Saurabh
> > Rob
@@ -4,11 +4,13 @@ menu "Microsoft Hyper-V guest support"
config HYPERV
tristate "Microsoft Hyper-V client drivers"
- depends on ACPI && ((X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \
- || (ARM64 && !CPU_BIG_ENDIAN))
+ depends on (X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \
+ || (ACPI && ARM64 && !CPU_BIG_ENDIAN)
select PARAVIRT
select X86_HV_CALLBACK_VECTOR if X86
select VMAP_PFN
+ select OF if !ACPI
+ select OF_EARLY_FLATTREE if !ACPI
help
Select this option to run Linux as a Hyper-V client operating
system.
@@ -20,6 +20,7 @@
#include <linux/completion.h>
#include <linux/hyperv.h>
#include <linux/kernel_stat.h>
+#include <linux/of_address.h>
#include <linux/clockchips.h>
#include <linux/cpu.h>
#include <linux/sched/isolation.h>
@@ -2152,7 +2153,7 @@ void vmbus_device_unregister(struct hv_device *device_obj)
device_unregister(&device_obj->device);
}
-
+#ifdef CONFIG_ACPI
/*
* VMBUS is an acpi enumerated device. Get the information we
* need from DSDT.
@@ -2262,6 +2263,7 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
return AE_OK;
}
+#endif
static void vmbus_mmio_remove(void)
{
@@ -2282,7 +2284,7 @@ static void vmbus_mmio_remove(void)
}
}
-static void vmbus_reserve_fb(void)
+static void __maybe_unused vmbus_reserve_fb(void)
{
resource_size_t start = 0, size;
struct pci_dev *pdev;
@@ -2442,6 +2444,7 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
}
EXPORT_SYMBOL_GPL(vmbus_free_mmio);
+#ifdef CONFIG_ACPI
static int vmbus_acpi_add(struct platform_device *pdev)
{
acpi_status result;
@@ -2494,10 +2497,50 @@ static int vmbus_acpi_add(struct platform_device *pdev)
vmbus_mmio_remove();
return ret_val;
}
+#endif
+
+static int vmbus_device_add(struct platform_device *pdev)
+{
+ struct resource **cur_res = &hyperv_mmio;
+ struct of_range range;
+ struct of_range_parser parser;
+ struct device_node *np;
+ int ret = 0;
+
+ hv_dev = &pdev->dev;
+ np = pdev->dev.of_node;
+
+ ret = of_range_parser_init(&parser, np);
+ if (ret) {
+ dev_err(hv_dev, "Failed to parse resources.\n");
+ return ret;
+ }
+
+ for_each_of_range(&parser, &range) {
+ struct resource *res;
+
+ res = kzalloc(sizeof(*res), GFP_ATOMIC);
+ if (!res)
+ return -ENOMEM;
+
+ res->name = "hyperv mmio";
+ res->flags = IORESOURCE_MEM | IORESOURCE_MEM_64;
+ res->start = range.pci_addr;
+ res->end = range.pci_addr + range.size;
+
+ *cur_res = res;
+ cur_res = &res->sibling;
+ }
+
+ return ret;
+}
static int vmbus_platform_driver_probe(struct platform_device *pdev)
{
+#ifdef CONFIG_ACPI
return vmbus_acpi_add(pdev);
+#endif
+ return vmbus_device_add(pdev);
}
static int vmbus_platform_driver_remove(struct platform_device *pdev)
@@ -2643,12 +2686,24 @@ static int vmbus_bus_resume(struct device *dev)
#define vmbus_bus_resume NULL
#endif /* CONFIG_PM_SLEEP */
+static const __maybe_unused struct of_device_id vmbus_of_match[] = {
+ {
+ .compatible = "microsoft,vmbus",
+ },
+ {
+ /* sentinel */
+ },
+};
+MODULE_DEVICE_TABLE(of, vmbus_of_match);
+
+#ifdef CONFIG_ACPI
static const struct acpi_device_id vmbus_acpi_device_ids[] = {
{"VMBUS", 0},
{"VMBus", 0},
{"", 0},
};
MODULE_DEVICE_TABLE(acpi, vmbus_acpi_device_ids);
+#endif
/*
* Note: we must use the "no_irq" ops, otherwise hibernation can not work with
@@ -2677,6 +2732,7 @@ static struct platform_driver vmbus_platform_driver = {
.driver = {
.name = "vmbus",
.acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids),
+ .of_match_table = of_match_ptr(vmbus_of_match),
.pm = &vmbus_bus_pm,
.probe_type = PROBE_FORCE_SYNCHRONOUS,
}