| Message ID | 1675188609-20913-1-git-send-email-ssengar@linux.microsoft.com |
|---|---|
| Headers | show |
| Series | Device tree support for Hyper-V VMBus driver | expand |
On Tue, Jan 31, 2023 at 01:57:36PM -0600, Rob Herring wrote: > > On Tue, 31 Jan 2023 10:10:07 -0800, Saurabh Sengar wrote: > > Rename virtio folder to more generic hypervisor, so that this can > > accommodate more devices of similar type. > > > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com> > > --- > > .../devicetree/bindings/{virtio => hypervisor}/mmio.yaml | 2 +- > > .../devicetree/bindings/{virtio => hypervisor}/pci-iommu.yaml | 2 +- > > .../bindings/{virtio => hypervisor}/virtio-device.yaml | 2 +- > > MAINTAINERS | 2 +- > > 4 files changed, 4 insertions(+), 4 deletions(-) > > rename Documentation/devicetree/bindings/{virtio => hypervisor}/mmio.yaml (95%) > > rename Documentation/devicetree/bindings/{virtio => hypervisor}/pci-iommu.yaml (98%) > > rename Documentation/devicetree/bindings/{virtio => hypervisor}/virtio-device.yaml (93%) > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > ./Documentation/devicetree/bindings/i2c/i2c-virtio.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/virtio/virtio-device.yaml > ./Documentation/devicetree/bindings/gpio/gpio-virtio.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/virtio/virtio-device.yaml > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-virtio.example.dtb: i2c: False schema does not allow {'compatible': ['virtio,device22'], '#address-cells': [[1]], '#size-cells': [[0]], 'light-sensor@20': {'compatible': ['dynaimage,al3320a'], 'reg': [[32]]}, '$nodename': ['i2c']} > From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/gpio/gpio-virtio.example.dtb: gpio: False schema does not allow {'compatible': ['virtio,device29'], 'gpio-controller': True, '#gpio-cells': [[2]], 'interrupt-controller': True, '#interrupt-cells': [[2]], '$nodename': ['gpio']} > From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hypervisor/virtio-device.example.dtb: i2c: False schema does not allow {'compatible': ['virtio,device22'], '$nodename': ['i2c']} > From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml > > doc reference errors (make refcheckdocs): > MAINTAINERS: Documentation/devicetree/bindings/virtio/ > > See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1675188609-20913-5-git-send-email-ssengar@linux.microsoft.com > > The base for the series is generally the latest rc1. A different dependency > should be noted in *this* patch. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit after running the above command yourself. Note > that DT_SCHEMA_FILES can be set to your schema file to speed up checking > your schema. However, it must be unset to test all examples with your schema. Hi Rob, I set DT_SCHEMA_FILES as below and ran "make -j32 DT_CHECKER_FLAGS=-m dt_binding_check". export DT_SCHEMA_FILES=Documentation/devicetree/bindings/hypervisor But I can see only below error: /work/upstream/linux-next/Documentation/devicetree/bindings/hypervisor/virtio-device.example.dtb: i2c: False schema does not allow {'compatible': ['virtio,device22'], '$nodename': ['i2c']} From schema: /work/upstream/linux-next/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml If I unset DT_SCHEMA_FILES, I see lot many errors which are not related to my changes. May I know what can I do to simulate the exact behaviour of your bot. Version of all the packages look latest: $ pip3 show dtschema Name: dtschema Version: 2023.1 Summary: DeviceTree validation schema and tools Home-page: https://github.com/devicetree-org/dt-schema Author: Rob Herring Author-email: robh@kernel.org License: BSD Location: /home/azureuser/.local/lib/python3.8/site-packages Requires: rfc3987, jsonschema, pylibfdt, ruamel.yaml Required-by: $ dt-doc-validate --version 2023.1 $ yamllint --version yamllint 1.20.0 Regards, Saurabh
On Tue, Jan 31, 2023 at 02:12:53PM -0600, Rob Herring wrote: > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar > <ssengar@linux.microsoft.com> wrote: > > > > 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. > > > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com> > > --- > > drivers/hv/vmbus_drv.c | 75 ++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 73 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > > index 49030e756b9f..1741f1348f9f 100644 > > --- a/drivers/hv/vmbus_drv.c > > +++ b/drivers/hv/vmbus_drv.c > > @@ -2152,7 +2152,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 +2262,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 +2283,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 +2443,7 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size) > > } > > EXPORT_SYMBOL_GPL(vmbus_free_mmio); > > > > +#ifdef CONFIG_ACPI > > It's better to put C 'if (!IS_ENABLED(CONFIG_ACPI)' code in the I wanted to have separate function for ACPI and device tree flow, which can be easily maintained with #ifdef. Please let me know if its fine. > > > static int vmbus_acpi_add(struct platform_device *pdev) > > { > > acpi_status result; > > @@ -2496,10 +2498,68 @@ static int vmbus_acpi_add(struct platform_device *pdev) > > vmbus_mmio_remove(); > > return ret_val; > > } > > +#else > > + > > +static int vmbus_device_add(struct platform_device *pdev) > > +{ > > + struct resource **cur_res = &hyperv_mmio; > > + struct device_node *np; > > + u32 *ranges, len; > > + u64 start; > > + int nr_ranges, child_cells = 2, cur_cell = 0, ret = 0; > > + > > + hv_dev = pdev; > > + np = pdev->dev.of_node; > > + > > + nr_ranges = device_property_count_u32(&pdev->dev, "ranges"); > > Parsing ranges yourself is a bad sign. It's a standard property and we > have functions which handle it. If those don't work, then something is > wrong with your DT or they need to be fixed/expanded. I find all the standard functions which parse "ranges" property are doing much more then I need. Our requirement is to only pass the mmio memory range and size, I couldn't find any standard API doing this. I see some of the drivers are using these APIs to parse ranges property hence I follwed those examples. I will be happy to improve it if I get any better alternative. > > > + if (nr_ranges < 0) > > + return nr_ranges; > > + ranges = kcalloc(nr_ranges, sizeof(u32), GFP_KERNEL); > > + if (!ranges) > > + return -ENOMEM; > > + > > + if (device_property_read_u32_array(&pdev->dev, "ranges", ranges, nr_ranges)) { > > + ret = -EINVAL; > > + goto free_ranges; > > + } > > + > > + while (cur_cell < nr_ranges) { > > + struct resource *res; > > + > > + /* The first u64 in the ranges description isn't used currently. */ > > + cur_cell = cur_cell + child_cells; > > + start = ranges[cur_cell++]; > > + start = (start << 32) | ranges[cur_cell++]; > > + len = ranges[cur_cell++]; > > To expand my last point, the format of ranges is <child_addr > parent_addr length>. That's not what your 'ranges' has. You've also > just ignored '#address-cells' and '#size-cells'. Got it. However I need to check if there is any standard API which can give me these values, otherwise I may have to parse these as well :( Regards, Saurabh > > > + > > + res = kzalloc(sizeof(*res), GFP_ATOMIC); > > + if (!res) { > > + ret = -ENOMEM; > > + goto free_ranges; > > + } > > + > > + res->name = "hyperv mmio"; > > + res->flags = IORESOURCE_MEM | IORESOURCE_MEM_64; > > + res->start = start; > > + res->end = start + len; > > + > > + *cur_res = res; > > + cur_res = &res->sibling; > > + } > > + > > +free_ranges: > > + kfree(ranges); > > + return ret; > > +} > > +#endif > > > > static int vmbus_platform_driver_probe(struct platform_device *pdev) > > { > > +#ifdef CONFIG_ACPI > > return vmbus_acpi_add(pdev); > > +#else > > + return vmbus_device_add(pdev); > > +#endif > > } > > > > static int vmbus_platform_driver_remove(struct platform_device *pdev) > > @@ -2645,6 +2705,16 @@ static int vmbus_bus_resume(struct device *dev) > > #define vmbus_bus_resume NULL > > #endif /* CONFIG_PM_SLEEP */ > > > > +static const struct of_device_id vmbus_of_match[] = { > > + { > > + .compatible = "msft,vmbus", > > + }, > > + { > > + /* sentinel */ > > + }, > > +}; > > +MODULE_DEVICE_TABLE(of, vmbus_of_match); > > + > > static const struct acpi_device_id vmbus_acpi_device_ids[] = { > > {"VMBUS", 0}, > > {"VMBus", 0}, > > @@ -2679,6 +2749,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, > > } > > -- > > 2.25.1 > >
On 01/02/2023 17:34, Saurabh Singh Sengar wrote: >> Also see my comment on v1 about running DT validation on your dtb. I'm >> sure running it would point out other issues. Such as the root level >> comaptible string(s) need to be documented. You need cpu nodes, >> interrupt controller, timers, etc. Those all have to be documented. > > I will be changing the parent node to soc node as suggested by Krzysztof > in other thread. > > soc { > #address-cells = <2>; > #size-cells = <2>; > > vmbus@ff0000000 { > #address-cells = <2>; > #size-cells = <1>; > compatible = "Microsoft,vmbus"; > ranges = <0x00 0x00 0x0f 0xf0000000 0x10000000>; > }; > }; > > This will be sufficient. It will be ok for the example, but will not be ok for supporting your use case. Please solve all the points from Rob's comment above. Where is their documentation? Best regards, Krzysztof
On Wed, Feb 01, 2023 at 11:46:38AM -0600, Rob Herring wrote: > On Wed, Feb 01, 2023 at 08:51:33AM -0800, Saurabh Singh Sengar wrote: > > On Tue, Jan 31, 2023 at 02:12:53PM -0600, Rob Herring wrote: > > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar > > > <ssengar@linux.microsoft.com> wrote: > > > > > > > > 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. > > > > > > > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com> > > > > --- > > > > drivers/hv/vmbus_drv.c | 75 ++++++++++++++++++++++++++++++++++++++++-- > > > > 1 file changed, 73 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > > > > index 49030e756b9f..1741f1348f9f 100644 > > > > --- a/drivers/hv/vmbus_drv.c > > > > +++ b/drivers/hv/vmbus_drv.c > > > > @@ -2152,7 +2152,7 @@ void vmbus_device_unregister(struct hv_device *device_obj) > > > > device_unregister(&device_obj->device); > > > > } (...) > > > > struct pci_dev *pdev; > > > > @@ -2442,6 +2443,7 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size) > > > > } > > > > EXPORT_SYMBOL_GPL(vmbus_free_mmio); > > > > > > > > +#ifdef CONFIG_ACPI > > > > > > It's better to put C 'if (!IS_ENABLED(CONFIG_ACPI)' code in the > > > > I wanted to have separate function for ACPI and device tree flow, which > > can be easily maintained with #ifdef. Please let me know if its fine. > > Yes, you can have separate functions: > > static int vmbus_acpi_add(struct platform_device *pdev) > { > if (!IS_ENABLED(CONFIG_ACPI)) > return -ENODEV; > > ... > } > > The compiler will throw away the function in the end if CONFIG_ACPI is > not enabled. > > That is easier for us to maintain because it reduces the combinations to > build. > I tried removing #ifdef CONFIG_ACPI and use C's if(!IS_ENABLED(CONFIG_ACPI)) but looks compiler is not optimizing out the rest of function, it still throwing errors for acpi functions. This doesn't look 1:1 replacement to me. Please let me know if I have missunderstood any of your suggestion. drivers/hv/vmbus_drv.c:2175:8: error: implicit declaration of function ‘acpi_dev_resource_interrupt’ [-Werror=implicit-function- > > > > > > > > > static int vmbus_acpi_add(struct platform_device *pdev) > > > > { > > > > acpi_status result; > > > > @@ -2496,10 +2498,68 @@ static int vmbus_acpi_add(struct platform_device *pdev) > > [1] https://lwn.net/Articles/443531/
On Fri, Feb 3, 2023 at 11:36 AM Saurabh Singh Sengar <ssengar@linux.microsoft.com> wrote: > > On Wed, Feb 01, 2023 at 11:46:38AM -0600, Rob Herring wrote: > > On Wed, Feb 01, 2023 at 08:51:33AM -0800, Saurabh Singh Sengar wrote: > > > On Tue, Jan 31, 2023 at 02:12:53PM -0600, Rob Herring wrote: > > > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar > > > > <ssengar@linux.microsoft.com> wrote: > > > > > > > > > > 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. > > > > > > > > > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com> > > > > > --- > > > > > drivers/hv/vmbus_drv.c | 75 ++++++++++++++++++++++++++++++++++++++++-- > > > > > 1 file changed, 73 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > > > > > index 49030e756b9f..1741f1348f9f 100644 > > > > > --- a/drivers/hv/vmbus_drv.c > > > > > +++ b/drivers/hv/vmbus_drv.c > > > > > @@ -2152,7 +2152,7 @@ void vmbus_device_unregister(struct hv_device *device_obj) > > > > > device_unregister(&device_obj->device); > > > > > } > (...) > > > > > struct pci_dev *pdev; > > > > > @@ -2442,6 +2443,7 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size) > > > > > } > > > > > EXPORT_SYMBOL_GPL(vmbus_free_mmio); > > > > > > > > > > +#ifdef CONFIG_ACPI > > > > > > > > It's better to put C 'if (!IS_ENABLED(CONFIG_ACPI)' code in the > > > > > > I wanted to have separate function for ACPI and device tree flow, which > > > can be easily maintained with #ifdef. Please let me know if its fine. > > > > Yes, you can have separate functions: > > > > static int vmbus_acpi_add(struct platform_device *pdev) > > { > > if (!IS_ENABLED(CONFIG_ACPI)) > > return -ENODEV; > > > > ... > > } > > > > The compiler will throw away the function in the end if CONFIG_ACPI is > > not enabled. > > > > That is easier for us to maintain because it reduces the combinations to > > build. > > > > I tried removing #ifdef CONFIG_ACPI and use C's if(!IS_ENABLED(CONFIG_ACPI)) but looks > compiler is not optimizing out the rest of function, it still throwing errors > for acpi functions. This doesn't look 1:1 replacement to me. > Please let me know if I have missunderstood any of your suggestion. > > drivers/hv/vmbus_drv.c:2175:8: error: implicit declaration of function ‘acpi_dev_resource_interrupt’ [-Werror=implicit-function- That's a failure of the ACPI headers not having empty function declarations. The DT functions do... Also, this is just a broken assumption: #ifdef CONFIG_ACPI #else // Assume DT #endif Both ACPI and DT can be enabled at the same time. They may be mutually exclusive for a platform, but not the kernel. For distro kernels, both will be enabled typically if the arch supports both. On arm64, DT is never disabled because the boot interface is always DT. Furthermore, this makes compile testing your code difficult. The arm64 defconfig, allmodconfig and allyesconfig all will not build the DT code. The same for x86. This means all the CI builds that happen can't build test this. Rob
This set of patches expands the VMBus driver to include device tree support. The first two patches enable compilation of Hyper-V APIs in a non-ACPI build. The third patch converts the VMBus driver from acpi to more generic platform driver. Further to add device tree documentation for VMBus, it needs to club with other virtualization driver's documentation. For this rename the virtio folder to more generic hypervisor, so that all the hypervisor based devices can co-exist in a single place in device tree documentation. The fourth patch does this renaming. The fifth patch introduces the device tree documentation for VMBus. The sixth patch adds device tree support to the VMBus driver. Currently this is tested only for x86 and it may not work for other archs. [v2] - Convert VMBus acpi device to platform device, and added device tree support in separate patch. This enables using same driver structure for both the flows. - In Device tree documentation, changed virtio folder to hypervisor and moved VMBus documentation there. - Moved bindings before Device tree patch. - Removed stale ".data" and ".name" field from of_device match table. - Removed debug print. - Fix "make DT_CHECKER_FLAGS=-m dt_binding_check" warnings. Saurabh Sengar (6): drivers/clocksource/hyper-v: non ACPI support in hyperv clock Drivers: hv: allow non ACPI compilation for hv_is_hibernation_supported Drivers: hv: vmbus: Convert acpi_device to platform_device dt-bindings: hypervisor: Rename virtio to hypervisor dt-bindings: hypervisor: Add dt-bindings for VMBus Driver: VMBus: Add device tree support .../bindings/{virtio => hypervisor}/mmio.yaml | 2 +- .../bindings/hypervisor/msft,vmbus.yaml | 50 ++++++ .../{virtio => hypervisor}/pci-iommu.yaml | 2 +- .../{virtio => hypervisor}/virtio-device.yaml | 2 +- .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 3 +- drivers/clocksource/hyperv_timer.c | 15 +- drivers/hv/hv_common.c | 4 + drivers/hv/vmbus_drv.c | 153 ++++++++++++++---- 9 files changed, 193 insertions(+), 40 deletions(-) rename Documentation/devicetree/bindings/{virtio => hypervisor}/mmio.yaml (95%) create mode 100644 Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml rename Documentation/devicetree/bindings/{virtio => hypervisor}/pci-iommu.yaml (98%) rename Documentation/devicetree/bindings/{virtio => hypervisor}/virtio-device.yaml (93%)