Hi Grant, 2014-02-21 17:22 GMT+01:00 Jean-Jacques Hiblot <jjhiblot@xxxxxxxxxxxxxxx>: > Hi Grygorii, > > 2014-02-21 16:37 GMT+01:00 Strashko, Grygorii <grygorii.strashko@xxxxxx>: >> Hi Jean-Jacques, >> >> Sorry for top posting. >> >> As I know, there have been several attempts to solve the same problem already:) >> [1] https://lkml.org/lkml/2013/9/18/216 >> [2] https://lkml.org/lkml/2013/11/22/520 >> [3] https://lkml.org/lkml/2014/1/8/240 >> >> There are some questions related to your approach: >> 1) How to distinguish between cases "IRQ domain not ready" and "wrong IRQ data in DT" or other IRQ parsing errors? >> Otherwise, Driver's probe will be deffered wrongly and forever, >> Thierry Reding has tried to solve this in [1]. > > This approach doesn't really care about the cause of the problem. I'm > of the opinion that never-ending deferred probing is not a big issue, > being not triggered so often after start-up (only when a new device is > probed). But if we need to make it right, then we would have to change > a bit the API of irq_create_of_mapping() and irq_of_parse_and_map() > (or maybe duplicate this one to keep the patch small) to return a real > error code instead a simple 0. Then would should be able to > distinguish the different error causes. What do you think of the 2nd version of the patch? Is it all right to allways return EPROBE_DEFER or should we try to discriminate the error cause? Jean-Jacques > >> >> 2) How will be handled driver reloading situation? >> The worst case (sparse IRQ enabled): >> - remove driver A >> - remove driver B (irq-controller) >> - load driver B <--- different set of Linux IRQ numbers can be assigned >> - load driver A <--- oops. IRQ resources table contains invalid data >> > > It's not handled in the current implementation. But if all interrupts > entries are re-parsed (see my comment for Grant), it should be all > right. > Another problem would appear if the DT is dynamically updated and the > number of resource is changed. In the 1st version of the patch, this > was handled but it made the function more expensive. > > Jean-Jacques >> >> >> Best regards, >> Grygorii Strashko >> >> ============================================= >> >> The goal of this patch is to allow drivers to be probed even if at the time of >> the DT parsing some of their ressources are not available yet. >> >> In the current situation, the resource of a platform device are filled from the >> DT at the time the device is created (of_device_alloc()). The drawbackof this >> is that a device sitting close to the top of the DT (ahb for example) but >> depending on ressources that are initialized later (IRQ domain dynamically >> created for example) will fail to probe because the ressources don't exist >> at this time. >> >> This patch fills the resource structure only before the device is probed and >> will defer the probe if the resource are not available yet. >> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@xxxxxxxxxxxxxxx> >> Reviewed-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx> >> --- >> >> Hi Grant, >> >> I reworked the patch as you proposed. To keep the overhead minimum, nirq and >> nreg are computed only the first time. >> In this implementation, only the missing IRQ ressources are re-tried for. It could >> easily be changed to re-parse all the IRQs though (replace if (!res->flags) >> with if ((!res->flags) || (res->flags & IORESOURCE_IRQ)). >> >> drivers/base/platform.c | 5 +++ >> drivers/of/platform.c | 100 +++++++++++++++++++++++++++++++++----------- >> include/linux/of_platform.h | 10 +++++ >> 3 files changed, 90 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c >> index bc78848..cee9b8d 100644 >> --- a/drivers/base/platform.c >> +++ b/drivers/base/platform.c >> @@ -481,6 +481,10 @@ static int platform_drv_probe(struct device *_dev) >> struct platform_device *dev = to_platform_device(_dev); >> int ret; >> >> + ret = of_platform_device_prepare(dev); >> + if (ret) >> + goto error; >> + >> if (ACPI_HANDLE(_dev)) >> acpi_dev_pm_attach(_dev, true); >> >> @@ -488,6 +492,7 @@ static int platform_drv_probe(struct device *_dev) >> if (ret && ACPI_HANDLE(_dev)) >> acpi_dev_pm_detach(_dev, true); >> >> +error: >> if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) { >> dev_warn(_dev, "probe deferral not supported\n"); >> ret = -ENXIO; >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c >> index 404d1da..a4e2602 100644 >> --- a/drivers/of/platform.c >> +++ b/drivers/of/platform.c >> @@ -141,36 +141,11 @@ struct platform_device *of_device_alloc(struct device_node *np, >> struct device *parent) >> { >> struct platform_device *dev; >> - int rc, i, num_reg = 0, num_irq; >> - struct resource *res, temp_res; >> >> dev = platform_device_alloc("", -1); >> if (!dev) >> return NULL; >> >> - /* count the io and irq resources */ >> - if (of_can_translate_address(np)) >> - while (of_address_to_resource(np, num_reg, &temp_res) == 0) >> - num_reg++; >> - num_irq = of_irq_count(np); >> - >> - /* Populate the resource table */ >> - if (num_irq || num_reg) { >> - res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL); >> - if (!res) { >> - platform_device_put(dev); >> - return NULL; >> - } >> - >> - dev->num_resources = num_reg + num_irq; >> - dev->resource = res; >> - for (i = 0; i < num_reg; i++, res++) { >> - rc = of_address_to_resource(np, i, res); >> - WARN_ON(rc); >> - } >> - WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq); >> - } >> - >> dev->dev.of_node = of_node_get(np); >> #if defined(CONFIG_MICROBLAZE) >> dev->dev.dma_mask = &dev->archdata.dma_mask; >> @@ -233,6 +208,81 @@ static struct platform_device *of_platform_device_create_pdata( >> return dev; >> } >> >> +static int of_reg_count(struct device_node *np) >> +{ >> + int nreg = 0; >> + if (of_can_translate_address(np)) { >> + struct resource temp_res; >> + while (of_address_to_resource(np, nreg, &temp_res) == 0) >> + nreg++; >> + } >> + return nreg; >> +} >> + >> +int of_platform_device_prepare(struct platform_device *dev) >> +{ >> + struct device_node *np; >> + int i, irq_index; >> + struct resource *res; >> + >> + /* >> + * This function applies only devices described in the DT. >> + * Other platform devices have their ressources already populated. >> + */ >> + np = dev->dev.of_node; >> + if (!np) >> + return 0; >> + >> + /* Populate the resource table */ >> + if (!dev->resource) { >> + int rc, nreg = 0, nirq; >> + /* count the io and irq resources */ >> + nreg = of_reg_count(np); >> + nirq = of_irq_count(np); >> + >> + if (!nirq && !nreg) >> + return 0; >> + >> + res = kzalloc(sizeof(*res) * (nirq + nreg), GFP_KERNEL); >> + if (!res) >> + return -ENOMEM; >> + >> + dev->resource = res; >> + dev->num_resources = nreg + nirq; >> + >> + for (i = 0; i < nreg; i++, res++) { >> + rc = of_address_to_resource(np, i, res); >> + if (WARN_ON(rc)) { >> + /* THIS IS BAD; don't try to defer probing */ >> + dev->num_resources = 0; >> + dev->resource = NULL; >> + kfree(res); >> + return rc; >> + } >> + } >> + >> + if (!rc && of_irq_to_resource_table(np, res, nirq) != nirq) { >> + /* IRQ controller is yet available. defer probing */ >> + return -EPROBE_DEFER; >> + } >> + >> + return 0; >> + } >> + >> + /* See which IRQ resources need to be redone */ >> + irq_index = 0; >> + for (i = 0, res = dev->resource; i < dev->num_resources; i++, res++) { >> + if (!res->flags) { >> + if (!of_irq_to_resource(np, irq_index, res)) >> + return -EPROBE_DEFER; >> + irq_index++; >> + } else if (res->flags & IORESOURCE_IRQ) >> + irq_index++; >> + } >> + return 0; >> +} >> +EXPORT_SYMBOL(of_platform_device_prepare); >> + >> /** >> * of_platform_device_create - Alloc, initialize and register an of_device >> * @np: pointer to node to create device for >> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h >> index 05cb4a9..4e487ff 100644 >> --- a/include/linux/of_platform.h >> +++ b/include/linux/of_platform.h >> @@ -53,6 +53,16 @@ struct of_dev_auxdata { >> >> extern const struct of_device_id of_default_bus_match_table[]; >> >> +/* Populate the resource for a platform device */ >> +#ifdef CONFIG_OF >> +int of_platform_device_prepare(struct platform_device *dev); >> +#else >> +static inline int of_platform_device_prepare( >> + struct platform_device *dev) >> +{ >> + return 0; >> +} >> +#endif >> /* Platform drivers register/unregister */ >> extern struct platform_device *of_device_alloc(struct device_node *np, >> const char *bus_id, >> -- >> 1.9.0 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
