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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> > ---
> > 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
> >
