| Message ID | 1439417187-21411-3-git-send-email-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 12, 2015 at 05:06:27PM -0500, Jeremy Linton wrote: > Add ACPI bindings for the smsc911x driver. Convert the DT specific calls > to nonspecific device* calls, This allows the driver to work > with both ACPI and DT configurations. Ethernet should now work when using > ACPI on ARM Juno. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> The code looks fine to me. Currently the compulsary DT properties seem to match the "approved" ACPI NIC properties from here http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org> Thanks > --- > drivers/net/ethernet/smsc/smsc911x.c | 48 +++++++++++++++++------------------- > 1 file changed, 22 insertions(+), 26 deletions(-) > > diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c > index 959aeea..0f21aa3 100644 > --- a/drivers/net/ethernet/smsc/smsc911x.c > +++ b/drivers/net/ethernet/smsc/smsc911x.c > @@ -59,7 +59,9 @@ > #include <linux/of_device.h> > #include <linux/of_gpio.h> > #include <linux/of_net.h> > +#include <linux/acpi.h> > #include <linux/pm_runtime.h> > +#include <linux/property.h> > > #include "smsc911x.h" > > @@ -2362,59 +2364,46 @@ static const struct smsc911x_ops shifted_smsc911x_ops = { > .tx_writefifo = smsc911x_tx_writefifo_shift, > }; > > -#ifdef CONFIG_OF > -static int smsc911x_probe_config_dt(struct smsc911x_platform_config *config, > - struct device_node *np) > +static int smsc911x_probe_config(struct smsc911x_platform_config *config, > + struct device *dev) > { > - const char *mac; > u32 width = 0; > > - if (!np) > + if (!dev) > return -ENODEV; > > - config->phy_interface = of_get_phy_mode(np); > + config->phy_interface = device_get_phy_mode(dev); > > - mac = of_get_mac_address(np); > - if (mac) > - memcpy(config->mac, mac, ETH_ALEN); > + device_get_mac_address(dev, config->mac, ETH_ALEN); > > - of_property_read_u32(np, "reg-shift", &config->shift); > + device_property_read_u32(dev, "reg-shift", &config->shift); > > - of_property_read_u32(np, "reg-io-width", &width); > + device_property_read_u32(dev, "reg-io-width", &width); > if (width == 4) > config->flags |= SMSC911X_USE_32BIT; > else > config->flags |= SMSC911X_USE_16BIT; > > - if (of_get_property(np, "smsc,irq-active-high", NULL)) > + if (device_property_present(dev, "smsc,irq-active-high")) > config->irq_polarity = SMSC911X_IRQ_POLARITY_ACTIVE_HIGH; > > - if (of_get_property(np, "smsc,irq-push-pull", NULL)) > + if (device_property_present(dev, "smsc,irq-push-pull")) > config->irq_type = SMSC911X_IRQ_TYPE_PUSH_PULL; > > - if (of_get_property(np, "smsc,force-internal-phy", NULL)) > + if (device_property_present(dev, "smsc,force-internal-phy")) > config->flags |= SMSC911X_FORCE_INTERNAL_PHY; > > - if (of_get_property(np, "smsc,force-external-phy", NULL)) > + if (device_property_present(dev, "smsc,force-external-phy")) > config->flags |= SMSC911X_FORCE_EXTERNAL_PHY; > > - if (of_get_property(np, "smsc,save-mac-address", NULL)) > + if (device_property_present(dev, "smsc,save-mac-address")) > config->flags |= SMSC911X_SAVE_MAC_ADDRESS; > > return 0; > } > -#else > -static inline int smsc911x_probe_config_dt( > - struct smsc911x_platform_config *config, > - struct device_node *np) > -{ > - return -ENODEV; > -} > -#endif /* CONFIG_OF */ > > static int smsc911x_drv_probe(struct platform_device *pdev) > { > - struct device_node *np = pdev->dev.of_node; > struct net_device *dev; > struct smsc911x_data *pdata; > struct smsc911x_platform_config *config = dev_get_platdata(&pdev->dev); > @@ -2478,7 +2467,7 @@ static int smsc911x_drv_probe(struct platform_device *pdev) > goto out_disable_resources; > } > > - retval = smsc911x_probe_config_dt(&pdata->config, np); > + retval = smsc911x_probe_config(&pdata->config, &pdev->dev); > if (retval && config) { > /* copy config parameters across to pdata */ > memcpy(&pdata->config, config, sizeof(pdata->config)); > @@ -2654,6 +2643,12 @@ static const struct of_device_id smsc911x_dt_ids[] = { > MODULE_DEVICE_TABLE(of, smsc911x_dt_ids); > #endif > > +static const struct acpi_device_id smsc911x_acpi_match[] = { > + { "ARMH9118", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, smsc911x_acpi_match); > + > static struct platform_driver smsc911x_driver = { > .probe = smsc911x_drv_probe, > .remove = smsc911x_drv_remove, > @@ -2661,6 +2656,7 @@ static struct platform_driver smsc911x_driver = { > .name = SMSC_CHIPNAME, > .pm = SMSC911X_PM_OPS, > .of_match_table = of_match_ptr(smsc911x_dt_ids), > + .acpi_match_table = ACPI_PTR(smsc911x_acpi_match), > }, > }; > > -- > 2.4.3 > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Aug 13, 2015 at 09:27:59AM +0100, Graeme Gregory wrote: > On Wed, Aug 12, 2015 at 05:06:27PM -0500, Jeremy Linton wrote: > > Add ACPI bindings for the smsc911x driver. Convert the DT specific calls > > to nonspecific device* calls, This allows the driver to work > > with both ACPI and DT configurations. Ethernet should now work when using > > ACPI on ARM Juno. Last sentence does not belong in the commit log. > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > > The code looks fine to me. > > Currently the compulsary DT properties seem to match the "approved" ACPI > NIC properties from here > > http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf What about _DSD device specific properties (eg "smsc,save-mac-address") ? Are we taking 1:1 translation between DT and ACPI for granted ? I thought some process must be put in place to define the corresponding bindings in ACPI before starting this mechanical translation or maybe I missed something, I would like to understand. How does the device specific _DSD definitions work in ACPI world ? Where are they published ? How will we translate those to DT bindings if there is need ? Lorenzo > Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org> > > Thanks > > > --- > > drivers/net/ethernet/smsc/smsc911x.c | 48 +++++++++++++++++------------------- > > 1 file changed, 22 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c > > index 959aeea..0f21aa3 100644 > > --- a/drivers/net/ethernet/smsc/smsc911x.c > > +++ b/drivers/net/ethernet/smsc/smsc911x.c > > @@ -59,7 +59,9 @@ > > #include <linux/of_device.h> > > #include <linux/of_gpio.h> > > #include <linux/of_net.h> > > +#include <linux/acpi.h> > > #include <linux/pm_runtime.h> > > +#include <linux/property.h> > > > > #include "smsc911x.h" > > > > @@ -2362,59 +2364,46 @@ static const struct smsc911x_ops shifted_smsc911x_ops = { > > .tx_writefifo = smsc911x_tx_writefifo_shift, > > }; > > > > -#ifdef CONFIG_OF > > -static int smsc911x_probe_config_dt(struct smsc911x_platform_config *config, > > - struct device_node *np) > > +static int smsc911x_probe_config(struct smsc911x_platform_config *config, > > + struct device *dev) > > { > > - const char *mac; > > u32 width = 0; > > > > - if (!np) > > + if (!dev) > > return -ENODEV; > > > > - config->phy_interface = of_get_phy_mode(np); > > + config->phy_interface = device_get_phy_mode(dev); > > > > - mac = of_get_mac_address(np); > > - if (mac) > > - memcpy(config->mac, mac, ETH_ALEN); > > + device_get_mac_address(dev, config->mac, ETH_ALEN); > > > > - of_property_read_u32(np, "reg-shift", &config->shift); > > + device_property_read_u32(dev, "reg-shift", &config->shift); > > > > - of_property_read_u32(np, "reg-io-width", &width); > > + device_property_read_u32(dev, "reg-io-width", &width); > > if (width == 4) > > config->flags |= SMSC911X_USE_32BIT; > > else > > config->flags |= SMSC911X_USE_16BIT; > > > > - if (of_get_property(np, "smsc,irq-active-high", NULL)) > > + if (device_property_present(dev, "smsc,irq-active-high")) > > config->irq_polarity = SMSC911X_IRQ_POLARITY_ACTIVE_HIGH; > > > > - if (of_get_property(np, "smsc,irq-push-pull", NULL)) > > + if (device_property_present(dev, "smsc,irq-push-pull")) > > config->irq_type = SMSC911X_IRQ_TYPE_PUSH_PULL; > > > > - if (of_get_property(np, "smsc,force-internal-phy", NULL)) > > + if (device_property_present(dev, "smsc,force-internal-phy")) > > config->flags |= SMSC911X_FORCE_INTERNAL_PHY; > > > > - if (of_get_property(np, "smsc,force-external-phy", NULL)) > > + if (device_property_present(dev, "smsc,force-external-phy")) > > config->flags |= SMSC911X_FORCE_EXTERNAL_PHY; > > > > - if (of_get_property(np, "smsc,save-mac-address", NULL)) > > + if (device_property_present(dev, "smsc,save-mac-address")) > > config->flags |= SMSC911X_SAVE_MAC_ADDRESS; > > > > return 0; > > } > > -#else > > -static inline int smsc911x_probe_config_dt( > > - struct smsc911x_platform_config *config, > > - struct device_node *np) > > -{ > > - return -ENODEV; > > -} > > -#endif /* CONFIG_OF */ > > > > static int smsc911x_drv_probe(struct platform_device *pdev) > > { > > - struct device_node *np = pdev->dev.of_node; > > struct net_device *dev; > > struct smsc911x_data *pdata; > > struct smsc911x_platform_config *config = dev_get_platdata(&pdev->dev); > > @@ -2478,7 +2467,7 @@ static int smsc911x_drv_probe(struct platform_device *pdev) > > goto out_disable_resources; > > } > > > > - retval = smsc911x_probe_config_dt(&pdata->config, np); > > + retval = smsc911x_probe_config(&pdata->config, &pdev->dev); > > if (retval && config) { > > /* copy config parameters across to pdata */ > > memcpy(&pdata->config, config, sizeof(pdata->config)); > > @@ -2654,6 +2643,12 @@ static const struct of_device_id smsc911x_dt_ids[] = { > > MODULE_DEVICE_TABLE(of, smsc911x_dt_ids); > > #endif > > > > +static const struct acpi_device_id smsc911x_acpi_match[] = { > > + { "ARMH9118", 0 }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(acpi, smsc911x_acpi_match); > > + > > static struct platform_driver smsc911x_driver = { > > .probe = smsc911x_drv_probe, > > .remove = smsc911x_drv_remove, > > @@ -2661,6 +2656,7 @@ static struct platform_driver smsc911x_driver = { > > .name = SMSC_CHIPNAME, > > .pm = SMSC911X_PM_OPS, > > .of_match_table = of_match_ptr(smsc911x_dt_ids), > > + .acpi_match_table = ACPI_PTR(smsc911x_acpi_match), > > }, > > }; > > > > -- > > 2.4.3 > > > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thu, Aug 13, 2015 at 10:01:17AM +0100, Lorenzo Pieralisi wrote: > On Thu, Aug 13, 2015 at 09:27:59AM +0100, Graeme Gregory wrote: > > On Wed, Aug 12, 2015 at 05:06:27PM -0500, Jeremy Linton wrote: > > > Add ACPI bindings for the smsc911x driver. Convert the DT specific calls > > > to nonspecific device* calls, This allows the driver to work > > > with both ACPI and DT configurations. Ethernet should now work when using > > > ACPI on ARM Juno. > > Last sentence does not belong in the commit log. > > > > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > > > > The code looks fine to me. > > > > Currently the compulsary DT properties seem to match the "approved" ACPI > > NIC properties from here > > > > http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf > > What about _DSD device specific properties (eg "smsc,save-mac-address") ? > Are we taking 1:1 translation between DT and ACPI for granted ? > > I thought some process must be put in place to define the corresponding > bindings in ACPI before starting this mechanical translation or maybe > I missed something, I would like to understand. > > How does the device specific _DSD definitions work in ACPI world ? Where > are they published ? How will we translate those to DT bindings if there > is need ? > This I do not know as that discussion is still ongoing. But those options are currently marked as optional so driver should not fail if they are not present in ACPI. Graeme > Lorenzo > > > Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org> > > > > Thanks > > > > > --- > > > drivers/net/ethernet/smsc/smsc911x.c | 48 +++++++++++++++++------------------- > > > 1 file changed, 22 insertions(+), 26 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c > > > index 959aeea..0f21aa3 100644 > > > --- a/drivers/net/ethernet/smsc/smsc911x.c > > > +++ b/drivers/net/ethernet/smsc/smsc911x.c > > > @@ -59,7 +59,9 @@ > > > #include <linux/of_device.h> > > > #include <linux/of_gpio.h> > > > #include <linux/of_net.h> > > > +#include <linux/acpi.h> > > > #include <linux/pm_runtime.h> > > > +#include <linux/property.h> > > > > > > #include "smsc911x.h" > > > > > > @@ -2362,59 +2364,46 @@ static const struct smsc911x_ops shifted_smsc911x_ops = { > > > .tx_writefifo = smsc911x_tx_writefifo_shift, > > > }; > > > > > > -#ifdef CONFIG_OF > > > -static int smsc911x_probe_config_dt(struct smsc911x_platform_config *config, > > > - struct device_node *np) > > > +static int smsc911x_probe_config(struct smsc911x_platform_config *config, > > > + struct device *dev) > > > { > > > - const char *mac; > > > u32 width = 0; > > > > > > - if (!np) > > > + if (!dev) > > > return -ENODEV; > > > > > > - config->phy_interface = of_get_phy_mode(np); > > > + config->phy_interface = device_get_phy_mode(dev); > > > > > > - mac = of_get_mac_address(np); > > > - if (mac) > > > - memcpy(config->mac, mac, ETH_ALEN); > > > + device_get_mac_address(dev, config->mac, ETH_ALEN); > > > > > > - of_property_read_u32(np, "reg-shift", &config->shift); > > > + device_property_read_u32(dev, "reg-shift", &config->shift); > > > > > > - of_property_read_u32(np, "reg-io-width", &width); > > > + device_property_read_u32(dev, "reg-io-width", &width); > > > if (width == 4) > > > config->flags |= SMSC911X_USE_32BIT; > > > else > > > config->flags |= SMSC911X_USE_16BIT; > > > > > > - if (of_get_property(np, "smsc,irq-active-high", NULL)) > > > + if (device_property_present(dev, "smsc,irq-active-high")) > > > config->irq_polarity = SMSC911X_IRQ_POLARITY_ACTIVE_HIGH; > > > > > > - if (of_get_property(np, "smsc,irq-push-pull", NULL)) > > > + if (device_property_present(dev, "smsc,irq-push-pull")) > > > config->irq_type = SMSC911X_IRQ_TYPE_PUSH_PULL; > > > > > > - if (of_get_property(np, "smsc,force-internal-phy", NULL)) > > > + if (device_property_present(dev, "smsc,force-internal-phy")) > > > config->flags |= SMSC911X_FORCE_INTERNAL_PHY; > > > > > > - if (of_get_property(np, "smsc,force-external-phy", NULL)) > > > + if (device_property_present(dev, "smsc,force-external-phy")) > > > config->flags |= SMSC911X_FORCE_EXTERNAL_PHY; > > > > > > - if (of_get_property(np, "smsc,save-mac-address", NULL)) > > > + if (device_property_present(dev, "smsc,save-mac-address")) > > > config->flags |= SMSC911X_SAVE_MAC_ADDRESS; > > > > > > return 0; > > > } > > > -#else > > > -static inline int smsc911x_probe_config_dt( > > > - struct smsc911x_platform_config *config, > > > - struct device_node *np) > > > -{ > > > - return -ENODEV; > > > -} > > > -#endif /* CONFIG_OF */ > > > > > > static int smsc911x_drv_probe(struct platform_device *pdev) > > > { > > > - struct device_node *np = pdev->dev.of_node; > > > struct net_device *dev; > > > struct smsc911x_data *pdata; > > > struct smsc911x_platform_config *config = dev_get_platdata(&pdev->dev); > > > @@ -2478,7 +2467,7 @@ static int smsc911x_drv_probe(struct platform_device *pdev) > > > goto out_disable_resources; > > > } > > > > > > - retval = smsc911x_probe_config_dt(&pdata->config, np); > > > + retval = smsc911x_probe_config(&pdata->config, &pdev->dev); > > > if (retval && config) { > > > /* copy config parameters across to pdata */ > > > memcpy(&pdata->config, config, sizeof(pdata->config)); > > > @@ -2654,6 +2643,12 @@ static const struct of_device_id smsc911x_dt_ids[] = { > > > MODULE_DEVICE_TABLE(of, smsc911x_dt_ids); > > > #endif > > > > > > +static const struct acpi_device_id smsc911x_acpi_match[] = { > > > + { "ARMH9118", 0 }, > > > + { } > > > +}; > > > +MODULE_DEVICE_TABLE(acpi, smsc911x_acpi_match); > > > + > > > static struct platform_driver smsc911x_driver = { > > > .probe = smsc911x_drv_probe, > > > .remove = smsc911x_drv_remove, > > > @@ -2661,6 +2656,7 @@ static struct platform_driver smsc911x_driver = { > > > .name = SMSC_CHIPNAME, > > > .pm = SMSC911X_PM_OPS, > > > .of_match_table = of_match_ptr(smsc911x_dt_ids), > > > + .acpi_match_table = ACPI_PTR(smsc911x_acpi_match), > > > }, > > > }; > > > > > > -- > > > 2.4.3 > > > > > > > > > > > > _______________________________________________ > > > linux-arm-kernel mailing list > > > linux-arm-kernel@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Aug 13, 2015 at 10:38:38AM +0100, Graeme Gregory wrote: > On Thu, Aug 13, 2015 at 10:01:17AM +0100, Lorenzo Pieralisi wrote: > > On Thu, Aug 13, 2015 at 09:27:59AM +0100, Graeme Gregory wrote: > > > On Wed, Aug 12, 2015 at 05:06:27PM -0500, Jeremy Linton wrote: > > > > Add ACPI bindings for the smsc911x driver. Convert the DT specific calls > > > > to nonspecific device* calls, This allows the driver to work > > > > with both ACPI and DT configurations. Ethernet should now work when using > > > > ACPI on ARM Juno. > > > > Last sentence does not belong in the commit log. > > > > > > > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > > > > > > The code looks fine to me. > > > > > > Currently the compulsary DT properties seem to match the "approved" ACPI > > > NIC properties from here > > > > > > http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf > > > > What about _DSD device specific properties (eg "smsc,save-mac-address") ? > > Are we taking 1:1 translation between DT and ACPI for granted ? > > > > I thought some process must be put in place to define the corresponding > > bindings in ACPI before starting this mechanical translation or maybe > > I missed something, I would like to understand. > > > > How does the device specific _DSD definitions work in ACPI world ? Where > > are they published ? How will we translate those to DT bindings if there > > is need ? > > > > This I do not know as that discussion is still ongoing. But those > options are currently marked as optional so driver should not fail if > they are not present in ACPI. Well yes, but if they are present this patch uses them when booting with ACPI and that's what I am arguing about, because they are undocumented. This discussion should be brought to completion, the _DSD usage and bindings sharing between DT and ACPI well defined before we can enable drivers to rely on properties that in ACPI world have no binding definition at all, if we go by this route we might end up having drivers reusing DT properties that have no reason whatsoever to exist in ACPI world. So, to make this clear, the _DSD usage must be documented and there has to be a way to define the related bindings in ACPI world so that driver code can be reviewed accordingly. Lorenzo > Graeme > > > Lorenzo > > > > > Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org> > > > > > > Thanks > > > > > > > --- > > > > drivers/net/ethernet/smsc/smsc911x.c | 48 +++++++++++++++++------------------- > > > > 1 file changed, 22 insertions(+), 26 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c > > > > index 959aeea..0f21aa3 100644 > > > > --- a/drivers/net/ethernet/smsc/smsc911x.c > > > > +++ b/drivers/net/ethernet/smsc/smsc911x.c > > > > @@ -59,7 +59,9 @@ > > > > #include <linux/of_device.h> > > > > #include <linux/of_gpio.h> > > > > #include <linux/of_net.h> > > > > +#include <linux/acpi.h> > > > > #include <linux/pm_runtime.h> > > > > +#include <linux/property.h> > > > > > > > > #include "smsc911x.h" > > > > > > > > @@ -2362,59 +2364,46 @@ static const struct smsc911x_ops shifted_smsc911x_ops = { > > > > .tx_writefifo = smsc911x_tx_writefifo_shift, > > > > }; > > > > > > > > -#ifdef CONFIG_OF > > > > -static int smsc911x_probe_config_dt(struct smsc911x_platform_config *config, > > > > - struct device_node *np) > > > > +static int smsc911x_probe_config(struct smsc911x_platform_config *config, > > > > + struct device *dev) > > > > { > > > > - const char *mac; > > > > u32 width = 0; > > > > > > > > - if (!np) > > > > + if (!dev) > > > > return -ENODEV; > > > > > > > > - config->phy_interface = of_get_phy_mode(np); > > > > + config->phy_interface = device_get_phy_mode(dev); > > > > > > > > - mac = of_get_mac_address(np); > > > > - if (mac) > > > > - memcpy(config->mac, mac, ETH_ALEN); > > > > + device_get_mac_address(dev, config->mac, ETH_ALEN); > > > > > > > > - of_property_read_u32(np, "reg-shift", &config->shift); > > > > + device_property_read_u32(dev, "reg-shift", &config->shift); > > > > > > > > - of_property_read_u32(np, "reg-io-width", &width); > > > > + device_property_read_u32(dev, "reg-io-width", &width); > > > > if (width == 4) > > > > config->flags |= SMSC911X_USE_32BIT; > > > > else > > > > config->flags |= SMSC911X_USE_16BIT; > > > > > > > > - if (of_get_property(np, "smsc,irq-active-high", NULL)) > > > > + if (device_property_present(dev, "smsc,irq-active-high")) > > > > config->irq_polarity = SMSC911X_IRQ_POLARITY_ACTIVE_HIGH; > > > > > > > > - if (of_get_property(np, "smsc,irq-push-pull", NULL)) > > > > + if (device_property_present(dev, "smsc,irq-push-pull")) > > > > config->irq_type = SMSC911X_IRQ_TYPE_PUSH_PULL; > > > > > > > > - if (of_get_property(np, "smsc,force-internal-phy", NULL)) > > > > + if (device_property_present(dev, "smsc,force-internal-phy")) > > > > config->flags |= SMSC911X_FORCE_INTERNAL_PHY; > > > > > > > > - if (of_get_property(np, "smsc,force-external-phy", NULL)) > > > > + if (device_property_present(dev, "smsc,force-external-phy")) > > > > config->flags |= SMSC911X_FORCE_EXTERNAL_PHY; > > > > > > > > - if (of_get_property(np, "smsc,save-mac-address", NULL)) > > > > + if (device_property_present(dev, "smsc,save-mac-address")) > > > > config->flags |= SMSC911X_SAVE_MAC_ADDRESS; > > > > > > > > return 0; > > > > } > > > > -#else > > > > -static inline int smsc911x_probe_config_dt( > > > > - struct smsc911x_platform_config *config, > > > > - struct device_node *np) > > > > -{ > > > > - return -ENODEV; > > > > -} > > > > -#endif /* CONFIG_OF */ > > > > > > > > static int smsc911x_drv_probe(struct platform_device *pdev) > > > > { > > > > - struct device_node *np = pdev->dev.of_node; > > > > struct net_device *dev; > > > > struct smsc911x_data *pdata; > > > > struct smsc911x_platform_config *config = dev_get_platdata(&pdev->dev); > > > > @@ -2478,7 +2467,7 @@ static int smsc911x_drv_probe(struct platform_device *pdev) > > > > goto out_disable_resources; > > > > } > > > > > > > > - retval = smsc911x_probe_config_dt(&pdata->config, np); > > > > + retval = smsc911x_probe_config(&pdata->config, &pdev->dev); > > > > if (retval && config) { > > > > /* copy config parameters across to pdata */ > > > > memcpy(&pdata->config, config, sizeof(pdata->config)); > > > > @@ -2654,6 +2643,12 @@ static const struct of_device_id smsc911x_dt_ids[] = { > > > > MODULE_DEVICE_TABLE(of, smsc911x_dt_ids); > > > > #endif > > > > > > > > +static const struct acpi_device_id smsc911x_acpi_match[] = { > > > > + { "ARMH9118", 0 }, > > > > + { } > > > > +}; > > > > +MODULE_DEVICE_TABLE(acpi, smsc911x_acpi_match); > > > > + > > > > static struct platform_driver smsc911x_driver = { > > > > .probe = smsc911x_drv_probe, > > > > .remove = smsc911x_drv_remove, > > > > @@ -2661,6 +2656,7 @@ static struct platform_driver smsc911x_driver = { > > > > .name = SMSC_CHIPNAME, > > > > .pm = SMSC911X_PM_OPS, > > > > .of_match_table = of_match_ptr(smsc911x_dt_ids), > > > > + .acpi_match_table = ACPI_PTR(smsc911x_acpi_match), > > > > }, > > > > }; > > > > > > > > -- > > > > 2.4.3 > > > > > > > > > > > > > > > > _______________________________________________ > > > > linux-arm-kernel mailing list > > > > linux-arm-kernel@lists.infradead.org > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > > _______________________________________________ > > > linux-arm-kernel mailing list > > > linux-arm-kernel@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Jeremy, On 12/08/15 23:06, Jeremy Linton wrote: > Add ACPI bindings for the smsc911x driver. Convert the DT specific calls > to nonspecific device* calls, This allows the driver to work > with both ACPI and DT configurations. Ethernet should now work when using > ACPI on ARM Juno. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/net/ethernet/smsc/smsc911x.c | 48 +++++++++++++++++------------------- > 1 file changed, 22 insertions(+), 26 deletions(-) FWIW, mainline booting with this patch on Juno r1 with ACPI enabled dies a horrible death: [...] ARMH9118:00 supply vdd33a not found, using dummy regulator ARMH9118:00 supply vddvario not found, using dummy regulator random: nonblocking pool is initialized irq 192: nobody cared (try booting with the "irqpoll" option) CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.2.0+ #4727 Hardware name: ARM Juno development board (r0) (DT) Call trace: [<ffffffc000089ae8>] dump_backtrace+0x0/0x128 [<ffffffc000089c20>] show_stack+0x10/0x1c [<ffffffc00030cdf4>] dump_stack+0x8c/0xd0 [<ffffffc0000f4120>] __report_bad_irq+0x34/0xe0 [<ffffffc0000f44b4>] note_interrupt+0x21c/0x2cc [<ffffffc0000f1ca4>] handle_irq_event_percpu+0xd0/0x13c [<ffffffc0000f1d5c>] handle_irq_event+0x4c/0x80 [<ffffffc0000f4eb4>] handle_fasteoi_irq+0xb0/0x188 [<ffffffc0000f125c>] generic_handle_irq+0x30/0x4c [<ffffffc0000f1580>] __handle_domain_irq+0x5c/0xac [<ffffffc0000824dc>] gic_handle_irq+0x4c/0x9c Exception stack(0xffffffc97686f760 to 0xffffffc97686f880) f760: ffffffc97686c000 ffffffc000932000 ffffffc97686f8b0 ffffffc0000b718c f780: 0000000040000145 000000000010124c 0000000000000000 ffffffc0009a5800 f7a0: ffffffc0009a5800 ffffffc00053ad40 0000000000000020 000000000ccccccd f7c0: 002de54480000000 0000000000000020 0000000000000004 ffffffc00064dd8b f7e0: 0000000000000000 0000000000000005 ffffffc00064ddb0 0000000000000000 f800: fffffffffffffefe 000000000000000e 0000000000000007 0000000000000001 f820: 000000000000000e ffffffc97686c000 ffffffc000932000 ffffffc000915af8 f840: ffffffc97686f8b0 ffffff8000001000 0000000000000040 ffffffc0007df000 f860: 00000000ffff8b1d 0000000000000040 0000000000000202 ffffffc97686f8b0 [<ffffffc0000855a4>] el1_irq+0x64/0xd8 [<ffffffc0000b7538>] irq_exit+0x80/0xd8 [<ffffffc0000f1584>] __handle_domain_irq+0x60/0xac [<ffffffc0000824dc>] gic_handle_irq+0x4c/0x9c Exception stack(0xffffffc97686f990 to 0xffffffc97686fab0) f980: ffffffc976822200 ffffffc976051b80 f9a0: ffffffc97686fae0 ffffffc000639ca8 0000000080000045 ffffffc000385ec0 f9c0: ffffffc9768222a4 0000000000000040 0000000000000005 0000000000000000 f9e0: 0000000000000084 ffffffffffffffff 0000000000000000 ffffffc976400920 fa00: ffffffc976400948 0000000000000000 0000000000000760 00000000000000ae fa20: 0000000000000030 0000000000000000 0ffffffffffffffe ffffffffffffffff fa40: 0000000000000e7f 000000000000000c 000000000000073f ffffffc976822200 fa60: ffffffc976051b80 00000000000000c0 0000000000000000 ffffffc9768222a4 fa80: 0000000000000040 ffffffc976029000 0000000000000084 0000000000000040 faa0: ffffffc0006af168 ffffffc97686fae0 [<ffffffc0000855a4>] el1_irq+0x64/0xd8 [<ffffffc0000f36b0>] __setup_irq+0x27c/0x4e8 [<ffffffc0000f3a7c>] request_threaded_irq+0xc4/0x16c [<ffffffc0004a40fc>] smsc911x_drv_probe+0x730/0xff4 [<ffffffc0004082ac>] platform_drv_probe+0x48/0xb8 [<ffffffc00040678c>] driver_probe_device+0x1e8/0x2e0 [<ffffffc00040691c>] __driver_attach+0x98/0xa0 [<ffffffc0004049b4>] bus_for_each_dev+0x54/0x98 [<ffffffc0004061f8>] driver_attach+0x1c/0x28 [<ffffffc000405e34>] bus_add_driver+0x1c0/0x228 [<ffffffc0004071ac>] driver_register+0x64/0x108 [<ffffffc0004081e0>] __platform_driver_register+0x5c/0x68 [<ffffffc0008c957c>] smsc911x_init_module+0x14/0x20 [<ffffffc0000828c4>] do_one_initcall+0x88/0x1a4 [<ffffffc0008a6ac4>] kernel_init_freeable+0x154/0x1f8 [<ffffffc000634308>] kernel_init+0xc/0xd8 handlers: [<ffffffc0004a2518>] smsc911x_irqhandler Disabling IRQ #192 libphy: smsc911x-mdio: probed smsc911x ARMH9118:00 eth0: attached PHY driver [Generic PHY] (mii_bus:phy_addr=ARMH9118:00-ffff:01, irq=-1) VFIO - User Level meta-driver version: 0.3 ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver ehci-pci: EHCI PCI platform driver ehci-platform: EHCI generic platform driver ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver ohci-pci: OHCI PCI platform driver ohci-platform: OHCI generic platform driver usbcore: registered new interface driver usb-storage mousedev: PS/2 mouse device common for all mice rtc-efi rtc-efi: rtc core: registered rtc-efi as rtc0 sdhci: Secure Digital Host Controller Interface driver sdhci: Copyright(c) Pierre Ossman sdhci-pltfm: SDHCI platform and OF driver helper ledtrig-cpu: registered to indicate activity on CPUs usbcore: registered new interface driver usbhid usbhid: USB HID core driver NET: Registered protocol family 17 9pnet: Installing 9P2000 support Key type dns_resolver registered registered taskstats version 1 rtc-efi rtc-efi: setting system clock to 2015-08-31 18:48:50 UTC (1441046930) Waiting for root device /dev/sda1... Bad mode in Synchronous Abort handler detected, code 0x86000004 -- IABT (current EL) CPU: 0 PID: 435 Comm: kworker/0:1 Not tainted 4.2.0+ #4727 Hardware name: ARM Juno development board (r0) (DT) Workqueue: events_power_efficient phy_state_machine task: ffffffc976bf7080 ti: ffffffc976098000 task.ti: ffffffc976098000 PC is at 0x69c80203010820b0 LR is at phy_state_machine+0x38/0x3d8 pc : [<69c80203010820b0>] lr : [<ffffffc00045df10>] pstate: 60000045 sp : ffffffc97609bd60 x29: ffffffc97609bd60 x28: 0000000000000000 x27: ffffffc000932000 x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000 x23: ffffffc97fee2480 x22: ffffffc975c93368 x21: ffffffc975c933d0 x20: ffffffc975c93000 x19: ffffffc975c93368 x18: 0000000000000000 x17: 0000000000000001 x16: 0000000000000011 x15: ffffffffffffffff x14: ffffff0000000000 x13: ffffffffffffffff x12: 0000000000000008 x11: 0000000000000003 x10: 0000000000000760 x9 : ffffffc97609bd30 x8 : ffffffc976bf7840 x7 : fefdff366f6e6e6b x6 : 0033428f00000000 x5 : 0000000000000000 x4 : 0000000000800000 x3 : ffffffc97fee2480 x2 : 0000000000000000 x1 : 69c80203010820b0 x0 : ffffffc975c93000 Internal error: Oops - bad mode: 0 [#1] PREEMPT SMP Modules linked in: CPU: 0 PID: 435 Comm: kworker/0:1 Not tainted 4.2.0+ #4727 Hardware name: ARM Juno development board (r0) (DT) Workqueue: events_power_efficient phy_state_machine task: ffffffc976bf7080 ti: ffffffc976098000 task.ti: ffffffc976098000 PC is at 0x69c80203010820b0 LR is at phy_state_machine+0x38/0x3d8 pc : [<69c80203010820b0>] lr : [<ffffffc00045df10>] pstate: 60000045 sp : ffffffc97609bd60 x29: ffffffc97609bd60 x28: 0000000000000000 x27: ffffffc000932000 x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000 x23: ffffffc97fee2480 x22: ffffffc975c93368 x21: ffffffc975c933d0 x20: ffffffc975c93000 x19: ffffffc975c93368 x18: 0000000000000000 x17: 0000000000000001 x16: 0000000000000011 x15: ffffffffffffffff x14: ffffff0000000000 x13: ffffffffffffffff x12: 0000000000000008 x11: 0000000000000003 x10: 0000000000000760 x9 : ffffffc97609bd30 x8 : ffffffc976bf7840 x7 : fefdff366f6e6e6b x6 : 0033428f00000000 x5 : 0000000000000000 x4 : 0000000000800000 x3 : ffffffc97fee2480 x2 : 0000000000000000 x1 : 69c80203010820b0 x0 : ffffffc975c93000 Process kworker/0:1 (pid: 435, stack limit = 0xffffffc976098020) Stack: (0xffffffc97609bd60 to 0xffffffc97609c000) bd60: ffffffc97609bd90 ffffffc0000c8ce8 ffffffc97fee7400 0000000000000000 bd80: ffffffc976349180 ffffffc975c93368 ffffffc97609bdd0 ffffffc0000c8fec bda0: ffffffc976349180 ffffffc9763491b0 ffffffc97fee2498 ffffffc97fee2480 bdc0: ffffffc976098000 ffffffc00099f35c ffffffc97609be30 ffffffc0000ce914 bde0: ffffffc976344ec0 ffffffc0009a7070 ffffffc0007df5f0 ffffffc976349180 be00: ffffffc0000c8eb8 0000000000000000 0000000000000000 0000000000000000 be20: 0000000000000000 0000000000000000 0000000000000000 ffffffc000085c10 be40: ffffffc0000ce834 ffffffc976344ec0 0000000000000000 0000000000000000 be60: 0000000000000000 ffffffc0000d6ae0 ffffffc0000ce834 0000000000000000 be80: 0000000000000000 ffffffc976349180 0000000000000000 0000000000000000 bea0: ffffffc97609bea0 ffffffc97609bea0 0000000000000000 ffffffc000000000 bec0: ffffffc97609bec0 ffffffc97609bec0 0000000000000000 0000000000000000 bee0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bf00: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bf20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bf40: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bf60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bf80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bfa0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bfc0: 0000000000000000 0000000000000000 0000000000000000 0000000000000005 bfe0: 0000000000000000 0000000000000000 380d1498069cb864 936a64483f903586 Call trace: [<69c80203010820b0>] 0x69c80203010820b0 [<ffffffc0000c8ce4>] process_one_work+0x130/0x304 [<ffffffc0000c8fe8>] worker_thread+0x130/0x438 [<ffffffc0000ce910>] kthread+0xdc/0xf4 Code: bad PC value ---[ end trace 876730031d2f86c1 ]--- Unable to handle kernel paging request at virtual address ffffffffffffffd8 pgd = ffffffc0009d8000 [ffffffffffffffd8] *pgd=0000000000000000, *pud=0000000000000000 Internal error: Oops: 96000005 [#2] PREEMPT SMP Modules linked in: CPU: 0 PID: 435 Comm: kworker/0:1 Tainted: G D 4.2.0+ #4727 Hardware name: ARM Juno development board (r0) (DT) task: ffffffc976bf7080 ti: ffffffc976098000 task.ti: ffffffc976098000 PC is at kthread_data+0x4/0xc LR is at wq_worker_sleeping+0x10/0xc4 pc : [<ffffffc0000ceed8>] lr : [<ffffffc0000c9acc>] pstate: 600001c5 sp : ffffffc97609ba40 x29: ffffffc97609ba40 x28: 0000000000000000 x27: ffffffc9768d0000 x26: 0000000000000000 x25: 0000000000000000 x24: ffffffc976bf74d8 x23: ffffffc000636478 x22: ffffffc976bf7080 x21: ffffffc97fee2c00 x20: ffffffc000932000 x19: ffffffc000919000 x18: 000000000000000e x17: 0000000000000001 x16: 0000000000000007 x15: 000000000000000e x14: 0000000000000013 x13: 000000000000001a x12: ffffffc97609ba20 x11: 0000000000000028 x10: 0000000000000000 x9 : 0000000000000013 x8 : 0000000000000000 x7 : 0000000000000044 x6 : 0000000000000001 x5 : 0000000000000400 x4 : ffffffc97fee3530 x3 : 0000000000000000 x2 : ffffffc0009a7838 x1 : 0000000000000000 x0 : 0000000000000000 Process kworker/0:1 (pid: 435, stack limit = 0xffffffc976098020) Stack: (0xffffffc97609ba40 to 0xffffffc97609c000) ba40: ffffffc97609ba60 ffffffc0006362b8 ffffffc97609ba60 0000000000000000 ba60: ffffffc97609bab0 ffffffc000636478 ffffffc976098000 ffffffc97609b7c0 ba80: ffffffc97609bb40 ffffffc976bf73c8 0000000000000001 ffffffc000932000 baa0: 0000000000000000 0000000000000000 ffffffc97609bad0 ffffffc0000b651c bac0: ffffffc976bf7080 ffffffc0000b64dc ffffffc97609bb50 ffffffc000089dd0 bae0: ffffffc0009a4000 0000000000000000 ffffffc0007db528 ffffffc97609bc40 bb00: ffffffc976098000 ffffffc976bf7080 0000000000000000 0000000000000000 bb20: ffffffc000932000 0000000000000000 ffffffc0009a4000 ffffffc000942858 bb40: ffffffc97609bb40 ffffffc97609bb40 ffffffc97609bb90 ffffffc000089eac bb60: ffffffc97609bc40 69c80203010820b0 ffffffc97609bd60 69c80203010820b0 bb80: 0000000060000045 0000000000000021 ffffffc97609bba0 ffffffc00008a10c bba0: ffffffc97609bd60 ffffffc00045df10 ffffffc975c93368 ffffffc975c93000 bbc0: 0000000000000004 0000000000030001 69c80203010820b0 ffffffc0000d5c9c bbe0: ffffffc97609bc50 ffffffc0000d5de4 ffffffc97ff2d480 ffffffc97ff2bc08 bc00: 0000000000000000 ffffffc976842400 0000000000000005 0000000000000005 bc20: ffffffc000932000 ffffffc976098000 ffffffc97609bc80 ffffffc000100718 bc40: ffffffc975c93000 69c80203010820b0 0000000000000000 ffffffc97fee2480 bc60: 0000000000800000 0000000000000000 0033428f00000000 fefdff366f6e6e6b bc80: ffffffc976bf7840 ffffffc97609bd30 0000000000000760 0000000000000003 bca0: 0000000000000008 ffffffffffffffff ffffff0000000000 ffffffffffffffff bcc0: 0000000000000011 0000000000000001 0000000000000000 ffffffc975c93368 bce0: ffffffc975c93000 ffffffc975c933d0 ffffffc975c93368 ffffffc97fee2480 bd00: 0000000000000000 0000000000000000 0000000000000000 ffffffc000932000 bd20: 0000000000000000 ffffffc97609bd60 ffffffc00045df10 ffffffc97609bd60 bd40: 69c80203010820b0 0000000060000045 ffffffc975c93368 ffffffc9768d0000 bd60: ffffffc97609bd90 ffffffc0000c8ce8 ffffffc97fee7400 0000000000000000 bd80: ffffffc976349180 ffffffc975c93368 ffffffc97609bdd0 ffffffc0000c8fec bda0: ffffffc976349180 ffffffc9763491b0 ffffffc97fee2498 ffffffc97fee2480 bdc0: ffffffc976098000 ffffffc00099f35c ffffffc97609be30 ffffffc0000ce914 bde0: ffffffc976344ec0 ffffffc0009a7070 ffffffc0007df5f0 ffffffc976349180 be00: ffffffc0000c8eb8 0000000000000000 0000000000000000 0000000000000000 be20: 0000000000000000 0000000000000000 0000000000000000 ffffffc000085c10 be40: ffffffc0000ce834 ffffffc976344ec0 0000000000000000 0000000000000000 be60: 0000000000000000 ffffffc0000d6ae0 ffffffc0000ce834 0000000000000000 be80: 0000000000000000 ffffffc976349180 0000000000000000 0000000000000000 bea0: ffffffc97609bea0 ffffffc97609bea0 0000000000000001 ffffffc000010001 bec0: ffffffc97609bec0 ffffffc97609bec0 0000000000000000 0000000000000000 bee0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bf00: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bf20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bf40: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bf60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bf80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bfa0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 bfc0: 0000000000000000 0000000000000000 0000000000000000 0000000000000005 bfe0: 0000000000000000 0000000000000000 380d1498069cb864 936a64483f903586 Call trace: [<ffffffc0000ceed8>] kthread_data+0x4/0xc [<ffffffc0006362b4>] __schedule+0x4c8/0x64c [<ffffffc000636474>] schedule+0x3c/0xac [<ffffffc0000b6518>] do_exit+0x618/0x924 [<ffffffc000089dcc>] die+0x1a0/0x1bc [<ffffffc000089ea8>] arm64_notify_die+0x18/0x58 [<ffffffc00008a108>] bad_mode+0x88/0x98 [<ffffffc00045df0c>] phy_state_machine+0x34/0x3d8 [<ffffffc0000c8ce4>] process_one_work+0x130/0x304 [<ffffffc0000c8fe8>] worker_thread+0x130/0x438 [<ffffffc0000ce910>] kthread+0xdc/0xf4 Code: 97ffff86 a8c17bfd d65f03c0 f941fc00 (f85d8000) Disabling the driver leads to a booting system again. Booting with DT seems OK. I can see two issues here: we have a screaming interrupt, and we seem to corrupt some workqueue. How did you get this to work? Firmware release? Thanks, M.
Marc, |FWIW, mainline booting with this patch on Juno r1 with ACPI enabled dies a |horrible death: Sorry about the delay, I didn't see this message. <trimmed> |How did you get this to work? Firmware release? Yes, it's a firmware problem with the ACPI tables. It is likely you need _DSD changes for the SMSC. If your trying to run juno with ACPI there are a few other ACPI table changes you will need beyond what was in the last linaro release. I wanted to just provide a direct link to the correct firmware in this response, but it seems the required version is sort of up in the air at the moment. Be assured I will really push on this next week, and will provide a follow up here. Originally, the lack of DSD wasn't a problem, but as this patch evolved the fact that it consolidates a number of configuration code paths balancing that got tricky, and ACPI machines with "bad" tables were the victims (rather than breaking something I can't control). In the meantime, if this is blocking anyone I can provide instructions on how to update the required ACPI tables. Thanks, -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
On Wed, 23 Sep 2015 17:22:49 +0000 Jeremy Linton <Jeremy.Linton@arm.com> wrote: Hi Jeremy, > Marc, > > |FWIW, mainline booting with this patch on Juno r1 with ACPI enabled dies a > |horrible death: > > Sorry about the delay, I didn't see this message. > > <trimmed> > > |How did you get this to work? Firmware release? > > Yes, it's a firmware problem with the ACPI tables. It is likely you > need _DSD changes for the SMSC. If your trying to run juno with ACPI > there are a few other ACPI table changes you will need beyond what > was in the last linaro release. I wanted to just provide a direct > link to the correct firmware in this response, but it seems the > required version is sort of up in the air at the moment. Be assured I > will really push on this next week, and will provide a follow up here. Good. Please keep me posted on this. I understand this is still a work in progress, and it would be good to document what is the expected baseline for this to work correctly. > Originally, the lack of DSD wasn't a problem, but as this patch > evolved the fact that it consolidates a number of configuration code > paths balancing that got tricky, and ACPI machines with "bad" tables > were the victims (rather than breaking something I can't control). > > In the meantime, if this is blocking anyone I can provide > instructions on how to update the required ACPI tables. Thanks, M.
On 23/09/15 18:22, Jeremy Linton wrote: > Marc, > > |FWIW, mainline booting with this patch on Juno r1 with ACPI enabled > dies a |horrible death: > > Sorry about the delay, I didn't see this message. > > <trimmed> > > |How did you get this to work? Firmware release? > > Yes, it's a firmware problem with the ACPI tables. It is likely you > need _DSD changes for the SMSC. If your trying to run juno with ACPI > there are a few other ACPI table changes you will need beyond what > was in the last linaro release. I wanted to just provide a direct > link to the correct firmware in this response, but it seems the > required version is sort of up in the air at the moment. Be assured I > will really push on this next week, and will provide a follow up > here. > > Originally, the lack of DSD wasn't a problem, but as this patch > evolved the fact that it consolidates a number of configuration code > paths balancing that got tricky, and ACPI machines with "bad" tables > were the victims (rather than breaking something I can't control). > Even now absence of _DSD *should* continue to be just fine. We can bailout without initializing the device. Pulling the entire kernel down just because of an incomplete device entry make no sense IMO. > In the meantime, if this is blocking anyone I can provide > instructions on how to update the required ACPI tables. > I found that the driver has issues even with DT missing some of the optional properties. We need to fix it but I don't have much knowledge of the driver and also not sure how these properties are defined in ACPI/DSD world i.e. required vs optional. Regards, Sudeep
On Wed, 2015-08-12 at 17:06 -0500, Jeremy Linton wrote: > > > +static const struct acpi_device_id smsc911x_acpi_match[] = { > + { "ARMH9118", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, smsc911x_acpi_match); > + > static struct platform_driver smsc911x_driver = { > .probe = smsc911x_drv_probe, > .remove = smsc911x_drv_remove, > @@ -2661,6 +2656,7 @@ static struct platform_driver smsc911x_driver = > { > .name = SMSC_CHIPNAME, > .pm = SMSC911X_PM_OPS, > .of_match_table = of_match_ptr(smsc911x_dt_ids), > + .acpi_match_table = ACPI_PTR(smsc911x_acpi_match), > }, > }; Hm, surely you shouldn't need this part? If the ACPI device has a HID of PRP0001, and an appropriate "compatible" property, then it ought to be loaded anyway. If that doesn't work, we should fix that.
On 9/23/2015 8:41 PM, David Woodhouse wrote: > On Wed, 2015-08-12 at 17:06 -0500, Jeremy Linton wrote: >> >> +static const struct acpi_device_id smsc911x_acpi_match[] = { >> + { "ARMH9118", 0 }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(acpi, smsc911x_acpi_match); >> + >> static struct platform_driver smsc911x_driver = { >> .probe = smsc911x_drv_probe, >> .remove = smsc911x_drv_remove, >> @@ -2661,6 +2656,7 @@ static struct platform_driver smsc911x_driver = >> { >> .name = SMSC_CHIPNAME, >> .pm = SMSC911X_PM_OPS, >> .of_match_table = of_match_ptr(smsc911x_dt_ids), >> + .acpi_match_table = ACPI_PTR(smsc911x_acpi_match), >> }, >> }; > Hm, surely you shouldn't need this part? If the ACPI device has a HID > of PRP0001, and an appropriate "compatible" property, then it ought to > be loaded anyway. If that doesn't work, we should fix that. But if the device ID is assigned already, why would it hurt to actually use it? Thanks, Rafael
On Wed, 2015-09-23 at 22:51 +0200, Rafael J. Wysocki wrote: > > But if the device ID is assigned already, why would it hurt to > actually use it? It doesn't hurt at all, as long as we understand that there was no need to assign it a device ID at all, at least for Linux's benefit. And as long as it doesn't set a precedent that makes other people think they need to do so.
On 09/23/2015 02:03 PM, David Woodhouse wrote: > On Wed, 2015-09-23 at 22:51 +0200, Rafael J. Wysocki wrote: >> >> But if the device ID is assigned already, why would it hurt to >> actually use it? > > It doesn't hurt at all, as long as we understand that there was no need > to assign it a device ID at all, at least for Linux's benefit. And as > long as it doesn't set a precedent that makes other people think they > need to do so. It really depends on the people who writing the ASL code (DSDT), I'm not sure if Windows will use _DSD and how to use it, but firmware guys may just use the device ID to make the firmware to be usable both by Linux and Windows, and that's reasonable I think. Thanks Hanjun
On Wed, 2015-09-23 at 16:56 -0700, Hanjun Guo wrote: > > It really depends on the people who writing the ASL code (DSDT), > I'm not sure if Windows will use _DSD and how to use it, but > firmware guys may just use the device ID to make the firmware > to be usable both by Linux and Windows, and that's reasonable > I think. And the ideal way to do that is to add a _CID of "PRP0001" and a compatible string, and then Linux doesn't need to care about the special new HID that you've added purely to pander to the limitations of Windows. That way, once drivers are converted from the legacy of-property APIs to the new device-property APIs, there should be *nothing* required to make device drivers work under ACPI. That was kind of the point. If people want to add HIDs, fine. But let's not go down a path which *requires* modifying drivers. (I'm planning to try to learn Coccinelle and do a bombing run convertin to the new API some time soon, FWIW).
On Wed, Sep 23, 2015 at 06:57:03PM +0100, Sudeep Holla wrote: > On 23/09/15 18:22, Jeremy Linton wrote: > >|FWIW, mainline booting with this patch on Juno r1 with ACPI enabled > >dies a |horrible death: > > > >Sorry about the delay, I didn't see this message. > > > ><trimmed> > > > >|How did you get this to work? Firmware release? > > > >Yes, it's a firmware problem with the ACPI tables. It is likely you > >need _DSD changes for the SMSC. If your trying to run juno with ACPI > >there are a few other ACPI table changes you will need beyond what > >was in the last linaro release. I wanted to just provide a direct > >link to the correct firmware in this response, but it seems the > >required version is sort of up in the air at the moment. Be assured I > >will really push on this next week, and will provide a follow up > >here. > > > >Originally, the lack of DSD wasn't a problem, but as this patch > >evolved the fact that it consolidates a number of configuration code > >paths balancing that got tricky, and ACPI machines with "bad" tables > >were the victims (rather than breaking something I can't control). > > Even now absence of _DSD *should* continue to be just fine. We can > bailout without initializing the device. Pulling the entire kernel down > just because of an incomplete device entry make no sense IMO. I agree. IMO, that's a regression since a working kernel on Juno (albeit without networking) now dies unless you also upgrade the firmware.
Hi Dave, On Thu, Sep 24, 2015 at 09:16:19AM +0100, David Woodhouse wrote: > On Wed, 2015-09-23 at 16:56 -0700, Hanjun Guo wrote: > > It really depends on the people who writing the ASL code (DSDT), > > I'm not sure if Windows will use _DSD and how to use it, but > > firmware guys may just use the device ID to make the firmware > > to be usable both by Linux and Windows, and that's reasonable > > I think. > > And the ideal way to do that is to add a _CID of "PRP0001" and a > compatible string, and then Linux doesn't need to care about the > special new HID that you've added purely to pander to the limitations > of Windows. PRP0001 could make sense from a Linux driver code reuse perspective. It probably also makes sense for the x86 embedded world where there is ACPI legacy. On embedded ARM we have DT already, so we don't need to solve any ACPI issues. However, for the ARM server ecosystem (both hardware and software), IMO, PRP0001 will have long term negative implications. My view of the ARM server (incipient) world is that we have two main categories of SoC vendors targeting ACPI: 1. Vendors that have been in the server business for a long time and are looking into using ARM processors. They usually have experience with designing server hardware, ACPI firmware and targeting multiple OSes. 2. Vendors new to the server world (but not necessarily new to ARM) that would like to extend their SoC family with server products. They may not have prior experience with ACPI but are targeting it because they've probably been told so. What I'm worried about is the second category. We try hard to make such vendors follow both software and hardware standards (like ARM's SBSA and even ACPI, I don't have a problem if used properly). They really need to get into the habit of designing their hardware and firmware without thinking that they can always apply a Linux patch and recompile the kernel. In the server world, the kernel (image) is no longer owned by the SoC vendor (as it is the norm in mobile space) but by a different entity (OS vendor) which may not patch/upgrade the kernel every time (for example) a clock routing is changed on some SoC. So what we _do_ want on ARM is tight control of the _DSD properties that are going to be used in ACPI tables, avoid pitfalls like OS-specific names ("linux-trigger" is still listed as an example on uefi.org) or defining more than the minimum necessary. In this last category we want to avoid properties like clocks, voltage regulators; these should be handled in an ACPI-specific way (AML, device power states) or just fully initialised by the boot code/firmware when run-time changes not required. PRP0001 opens the door to any DT-like properties in ACPI and, knowing how the ARM ecosystem works, I'm pretty sure we'll soon lose control (if we haven't already; not all the developments are done in the open). I'm also not a fan of different firmware paths based on which OS is running, so I would rather have device _HID and _DSD properties where absolutely necessary. > (I'm planning to try to learn Coccinelle and do a bombing run convertin > to the new API some time soon, FWIW). I think such patch would address an issue with ACPI in the x86 embedded land. But I'd very much like to make PRP0001 parsing depend on !CONFIG_ARM64. What we first need to solve in the ARM (server) land is firmware and hardware standardisation rather than working around it. The pro ARM ACPI camp has been very vocal against DT in the server space. I'd like to seem them as vocal about PRP0001, unless they find it acceptable (and should apologise for bashing DT ;)). Thanks.
Hi Catalin, I understand your concerns, but I'm still not convinced of your conclusion. On Thu, 2015-09-24 at 11:31 +0100, Catalin Marinas wrote: > PRP0001 opens the door to any DT-like properties in ACPI and, knowing > how the ARM ecosystem works, I'm pretty sure we'll soon lose control (if > we haven't already; not all the developments are done in the open). No. That door is wide open already — people can already do whatever they like in _DSD properties. If they're going to screw up DT properties, they'll screw up ACPI-only _DSD properties just the same. You speak of maintaining "tight control of the _DSD properties that are going to be used in ACPI tables"... but if you're going to confiscate their crackpipe and stand over them while they work, you can do that just as well whether they're using "PRP0001" or "FOO1234" as their HID value. In that sense, the HID is entirely orthogonal. And think about it... we *really* don't want a given peripheral device to have *different* bindings depending on whether it's discovered with a specific ACPI HID, vs. when it's discovered via DT or the PRP0001 HID. That way lies complete insanity. In some ways, your proposal would be actively *counterproductive*. You say you want to train people *not* to keep patching the kernel. But where they *could* have just used PRP0001 and used a pre-existing kernel, you then tell them "oh, but now you need to patch the kernel because we want you to make up a new HID and not be compatible with what already exists." If you go down this road, I predict we'll start seeing *separate* drivers for identical components, because the bindings for DT vs. ACPI properties are different. We really don't want that. > The pro ARM ACPI camp has been very vocal against DT in the server > space. I'd like to seem them as vocal about PRP0001, unless they find it > acceptable (and should apologise for bashing DT ;)). Fundamentally, that DT vs. ACPI distinction has gone away with the introduction of _DSD. We *need* the flexibility that we gain from being able to provide device properties rather than inventing a new HID for every permutation, and with that flexibility comes a certain amount of responsibility to do things sensibly. People have not always designed their bindings sensibly. But that isn't going to be magically solved in the ACPI world, unless you *do* actually stand over them with their crackpipe in your hand, as I joked above. And eschewing PRP0001 really doesn't help you with that. It just makes things harder.
On Thu, Sep 24, 2015 at 12:52:38PM +0100, David Woodhouse wrote: [...] > And think about it... we *really* don't want a given peripheral device > to have *different* bindings depending on whether it's discovered with > a specific ACPI HID, vs. when it's discovered via DT or the PRP0001 > HID. That way lies complete insanity. There are "devices" that we do _not_ want to discover at all when booting with ACPI (eg drivers/clk, drivers/regulator) because the way they interact with ACPI standard power management AML methods may definitely clash. PRP0001 allows to pull everything in (depending on what the Coccinelle script will look like, because AFAIK that's the only thing that stops people from enabling eg drivers/clk drivers in ACPI systems with PRP0001, namely the OF to FW node API conversion). > In some ways, your proposal would be actively *counterproductive*. You > say you want to train people *not* to keep patching the kernel. But > where they *could* have just used PRP0001 and used a pre-existing > kernel, you then tell them "oh, but now you need to patch the kernel > because we want you to make up a new HID and not be compatible with > what already exists." We do not want people to reuse DT drivers that are handled in ACPI with existing methods (again, power management is a prime example). I do not think we want to enable DT PCI host controllers drivers either since the ACPI model for PCI works differently already. Should I mention CPUfreq and CPUidle DT drivers ? The list of concerns goes on, it does not stop here. > If you go down this road, I predict we'll start seeing *separate* > drivers for identical components, because the bindings for DT vs. ACPI > properties are different. We really don't want that. > > > The pro ARM ACPI camp has been very vocal against DT in the server > > space. I'd like to seem them as vocal about PRP0001, unless they find it > > acceptable (and should apologise for bashing DT ;)). > > Fundamentally, that DT vs. ACPI distinction has gone away with the > introduction of _DSD. We *need* the flexibility that we gain from being > able to provide device properties rather than inventing a new HID for > every permutation, and with that flexibility comes a certain amount of > responsibility to do things sensibly. How do you enforce that apart from limiting the number of drivers you convert to the FW node API ? What drivers are we going to convert and on what basis ? I think that's a debate worth having before making a tree-wide conversion of OF API calls to the generic FW node API. Thanks, Lorenzo
On Thu, 2015-09-24 at 15:01 +0100, Lorenzo Pieralisi wrote: > On Thu, Sep 24, 2015 at 12:52:38PM +0100, David Woodhouse wrote: > > [...] > > > And think about it... we *really* don't want a given peripheral device > > to have *different* bindings depending on whether it's discovered with > > a specific ACPI HID, vs. when it's discovered via DT or the PRP0001 > > HID. That way lies complete insanity. > > There are "devices" that we do _not_ want to discover at all when > booting with ACPI (eg drivers/clk, drivers/regulator) because the way > they interact with ACPI standard power management AML methods may > definitely clash. So it would be quite clearly a firmware bug to *expose* those things in that way, when also exposing AML methods which make use of them. But those really aren't what I'm looking at converting, anyway. The target of the Coccinelle script would be leaf-node drivers, like the one in this thread. Not subsystem code. We have, for example, a generic method for 'get the GPIO' which works equally well whether the actual information is given in the DT form, or whether it's done through the ACPI GPIO abstraction as described in Documentation/acpi/gpio-properties.txt. We certainly shouldn't be looking at a naïve 1:1 literal transliteration of properties. But on the other hand, we *do* want a direct and automatic bijective mapping between the ACPI and DT representations. > PRP0001 allows to pull everything in (depending on what the Coccinelle > script will look like, because AFAIK that's the only thing that stops > people from enabling eg drivers/clk drivers in ACPI systems with PRP0001, > namely the OF to FW node API conversion). > > > In some ways, your proposal would be actively *counterproductive*. You > > say you want to train people *not* to keep patching the kernel. But > > where they *could* have just used PRP0001 and used a pre-existing > > kernel, you then tell them "oh, but now you need to patch the kernel > > because we want you to make up a new HID and not be compatible with > > what already exists." > > We do not want people to reuse DT drivers that are handled in ACPI with > existing methods (again, power management is a prime example). > I do not think we want to enable DT PCI host controllers drivers either > since the ACPI model for PCI works differently already. > > Should I mention CPUfreq and CPUidle DT drivers ? No. Except to note that those are prime examples of the parts that *don't* want to be converted. > > Fundamentally, that DT vs. ACPI distinction has gone away with the > > introduction of _DSD. We *need* the flexibility that we gain from being > > able to provide device properties rather than inventing a new HID for > > every permutation, and with that flexibility comes a certain amount of > > responsibility to do things sensibly. > > How do you enforce that apart from limiting the number of drivers you > convert to the FW node API ? What drivers are we going to convert and > on what basis ? To start with, as I said, just leaf-node device drivers. None of the things like the above, which you are quite right don't really want a trivial conversion. It's insane to talk of "enforcement", so let's not go there. Anyone taking an existing driver and adding an ACPI HID to it would need to start with *precisely* the same patches for those properties anyway. Just as shown in the example in this thread. > I think that's a debate worth having before making a tree-wide conversion > of OF API calls to the generic FW node API. Well, if that's true, that's a debate worth having before introducing the generic FW node API. That having been kind of the *point* of the generic FW node API — to give code which doesn't *need* to be gratuitously firmware-specific, an API that works everywhere.
On Thu, Sep 24, 2015 at 12:52:38PM +0100, David Woodhouse wrote: > On Thu, 2015-09-24 at 11:31 +0100, Catalin Marinas wrote: > > PRP0001 opens the door to any DT-like properties in ACPI and, knowing > > how the ARM ecosystem works, I'm pretty sure we'll soon lose control (if > > we haven't already; not all the developments are done in the open). > > No. That door is wide open already — people can already do whatever > they like in _DSD properties. If they're going to screw up DT > properties, they'll screw up ACPI-only _DSD properties just the same. _DSD isn't something I like either. But since we now allow it, we should try to restrict its use to the bare minimum. > You speak of maintaining "tight control of the _DSD properties that > are going to be used in ACPI tables"... but if you're going to > confiscate their crackpipe and stand over them while they work, you can > do that just as well whether they're using "PRP0001" or "FOO1234" as > their HID value. With "PRP0001", they can skip the _DSD properties review process (not that they bother much currently) as long as the existing DT support covers their needs. However, we don't want to emulate DT in ACPI but first try the established ACPI methods and only use _DSD where these are insufficient. Automatically converting existing drivers and encouraging people to use "PRP0001" does not provide them with any incentive to try harder and learn the "ACPI way". > In that sense, the HID is entirely orthogonal. > > And think about it... we *really* don't want a given peripheral device > to have *different* bindings depending on whether it's discovered with > a specific ACPI HID, vs. when it's discovered via DT or the PRP0001 > HID. That way lies complete insanity. I'm fine with reusing the same property names between _DSD and DT but I strongly consider that _DSD should only cover a _subset_ of the DT bindings and they should be reviewed independently. Take the smsc911x driver for example: the DT bindings for the ARM Juno platform include things like "clocks", "vdd33a-supply". Such concepts are not available in ACPI (but we've seen people trying to emulate them), so rather than enabling such clocks in firmware, vendors will be tempted to do things the DT way, possibly with "PRP0001" HIDs covering the clock devices (though I think they currently require some kernel changes). > In some ways, your proposal would be actively *counterproductive*. You > say you want to train people *not* to keep patching the kernel. But > where they *could* have just used PRP0001 and used a pre-existing > kernel, you then tell them "oh, but now you need to patch the kernel > because we want you to make up a new HID and not be compatible with > what already exists." I gave an example above with the clocks but let's take a simpler, device-specific property like "smsc,irq-active-high". Is this documented anywhere apart from the kernel driver and the in-kernel DT smsc911x.txt? No. Are other non-Linux OS vendors going to look in the kernel source tree to implement support in their drivers? I doubt it and we could end up with two different paths in firmware for handling the same device. ACPI probably never was truly OS agnostic but with "PRP0001" we don't even try. The idea of registering a HID is to also have a process for getting corresponding _DSD properties approved, published. Some of them like "mac-address" would be more generic and not require further review but we'll have more specific properties. Patching a kernel driver to support a new HID is a (weak) method of keeping track of which devices are used with ACPI. Given how quickly these two patches were merged while still under discussion, I doubt that arch maintainers would be in a position to review/reject drivers using non-approved _DSD properties. > If you go down this road, I predict we'll start seeing *separate* > drivers for identical components, because the bindings for DT vs. ACPI > properties are different. We really don't want that. What if Linux is not the first targeted OS? We either convince the vendor to change the firmware or we'll have to support properties invented for other operating system. > > The pro ARM ACPI camp has been very vocal against DT in the server > > space. I'd like to seem them as vocal about PRP0001, unless they find it > > acceptable (and should apologise for bashing DT ;)). > > Fundamentally, that DT vs. ACPI distinction has gone away with the > introduction of _DSD. We *need* the flexibility that we gain from > being able to provide device properties rather than inventing a new > HID for every permutation, and with that flexibility comes a certain > amount of responsibility to do things sensibly. You may trust the x86 world to be responsible. I can't say the same about the ARM SoC world ;). While I understand the need for the flexibility of _DSD, I think the main difference of opinion is that I do not consider ACPI+_DSD equal to DT. It's fine to borrow some concepts from DT but I'm definitely against emulating it entirely, which is pretty much what "PRP0001" is doing. > People have not always designed their bindings sensibly. But that > isn't going to be magically solved in the ACPI world, unless you *do* > actually stand over them with their crackpipe in your hand, as I joked > above. And eschewing PRP0001 really doesn't help you with that. It > just makes things harder. I agree with you that _DSD already opened the door to even more "mess". But, IMO, our approaches differ: you are trying to avoid such mess affecting the kernel by hiding it behind "PRP0001"; what I'm trying is to restrict (as much as I can) the mess developing in the early days of ARM ACPI. I don't think either method will be fully successful, we may just end up with a combination of the two. Apart from a _DSD properties review process, what we need is (main) OS vendors enforcing it, possibly with stricter ACPI test suite. Disabling PRP0001 for ARM wouldn't solve the problem but at least it would make people think about what _DSD properties they need registered for their device (or I'm over optimistic and I should just stop caring ;)).
On Thu, 2015-09-24 at 16:15 +0100, Catalin Marinas wrote: > With "PRP0001", they can skip the _DSD properties review process (not > that they bother much currently) as long as the existing DT support > covers their needs. So no change there then. I take it the smsc911x bindings didn't go through this mythical _DSD properties review process either, right? > However, we don't want to emulate DT in ACPI but > first try the > established ACPI methods and only use _DSD where these are > insufficient. Automatically converting existing drivers and encouraging > people to use "PRP0001" does not provide them with any incentive to try > harder and learn the "ACPI way". But again, we're *not* looking at a simplistic transliteration of properties. Where there *is* an "ACPI way", such as the GPIO example I gave, that should absolutely be used. And there should be sufficiently high-level access functions that it shouldn't *matter* to the driver whether the information is coming from ACPI or DT. > > In that sense, the HID is entirely orthogonal. > > > > And think about it... we *really* don't want a given peripheral device > > to have *different* bindings depending on whether it's discovered with > > a specific ACPI HID, vs. when it's discovered via DT or the PRP0001 > > HID. That way lies complete insanity. > > I'm fine with reusing the same property names between _DSD and DT but I > strongly consider that _DSD should only cover a _subset_ of the DT > bindings and they should be reviewed independently. > > Take the smsc911x driver for example: the DT bindings for the ARM Juno > platform include things like "clocks", "vdd33a-supply". Such concepts > are not available in ACPI (but we've seen people trying to emulate > them), so rather than enabling such clocks in firmware, vendors will be > tempted to do things the DT way, possibly with "PRP0001" HIDs covering > the clock devices (though I think they currently require some kernel > changes). Aren't those optional? If nothing is provided, they are basically a no-op. AFAICT the existing patches for smsc911x don't touch that code at all. So whatever motivation you imagine there is for ACPI firmware vendors to "do things the DT way" is still there with the existing patches. Again, this is *orthogonal* to the question of whether the device is matched by PRP0001 + its compatible string, vs. a newly-invented HID. > > In some ways, your proposal would be actively *counterproductive*. You > > say you want to train people *not* to keep patching the kernel. But > > where they *could* have just used PRP0001 and used a pre-existing > > kernel, you then tell them "oh, but now you need to patch the kernel > > because we want you to make up a new HID and not be compatible with > > what already exists." > > I gave an example above with the clocks but let's take a simpler, > device-specific property like "smsc,irq-active-high". Is this documented > anywhere apart from the kernel driver and the in-kernel DT smsc911x.txt? > No. Are other non-Linux OS vendors going to look in the kernel source > tree to implement support in their drivers? I doubt it and we could end > up with two different paths in firmware for handling the same device. > ACPI probably never was truly OS agnostic but with "PRP0001" we don't > even try. Where is the documentation for the INT_CFG_IRQ_POL_ bit in the INT_CFG register, that it controls? The documentation on the bindings really ought to live near that, in an ideal world. But that's still a non-sequitur in the context of this particular discussion. The patch that was posted *already* keeps that very same (optional) smsc,irq-active-high property. Again, it isn't relevant to the question of whether the driver is matched via PRP0001 or a newly-allocated HID. The driver, as the patch was posted, *does* have the same set of properties with the same meanings — because anything else would be insane. > The idea of registering a HID is to also have a process for getting > corresponding _DSD properties approved, published. Some of them like > "mac-address" would be more generic and not require further review but > we'll have more specific properties. > > Patching a kernel driver to support a new HID is a (weak) method of > keeping track of which devices are used with ACPI. Given how quickly > these two patches were merged while still under discussion, I doubt that > arch maintainers would be in a position to review/reject drivers using > non-approved _DSD properties. Again, nothing has changed there. > Apart from a _DSD properties review process, what we need is (main) OS > vendors enforcing it, possibly with stricter ACPI test suite. Disabling > PRP0001 for ARM wouldn't solve the problem but at least it would make > people think about what _DSD properties they need registered for their > device (or I'm over optimistic and I should just stop caring ;)). I'm all for requiring pre-existing DT bindings to be "vetted" by the nascent _DSD review process, before their use with PRP0001 can be 'blessed'. But in a world where people *are* going to go off and do their own insane thing, we really might as well let them use the *same* thing that we already had — in as many cases as possible — rather than actually *making* them come up with a new insane thing all of their very own.
On Thu, Sep 24, 2015 at 07:10:38PM +0100, David Woodhouse wrote: > On Thu, 2015-09-24 at 16:15 +0100, Catalin Marinas wrote: > > With "PRP0001", they can skip the _DSD properties review process (not > > that they bother much currently) as long as the existing DT support > > covers their needs. > > So no change there then. I take it the smsc911x bindings didn't go > through this mythical _DSD properties review process either, right? Right. And that's another reason I disagree with this patchset (I don't always agree with everything that's coming out of ARM Ltd ;)). Now, in Jeremy's defence, this _DSD process looks quite mythical indeed. We had an ARM firmware ecosystem meeting earlier this year where we agreed to set up such process (at least for the ARM world): https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/CoreUpstreamNotes I can see some form of a process here: http://www.uefi.org/sites/default/files/resources/web-page-v2.pdf But, at least to me, it's not clear how you submit properties to such registry, what's the review process, who are the reviewers. I know there was a mailing list set up but I don't see it listed above. > > However, we don't want to emulate DT in ACPI but > first try the > > established ACPI methods and only use _DSD where these are > > insufficient. Automatically converting existing drivers and encouraging > > people to use "PRP0001" does not provide them with any incentive to try > > harder and learn the "ACPI way". > > But again, we're *not* looking at a simplistic transliteration of > properties. I know you and I don't look at it this way but, based on historical evidence, I'm sure that some ARM vendors will try to do exactly this (and be able to claim "ACPI support" when they were happily using DT). > > > In some ways, your proposal would be actively *counterproductive*. You > > > say you want to train people *not* to keep patching the kernel. But > > > where they *could* have just used PRP0001 and used a pre-existing > > > kernel, you then tell them "oh, but now you need to patch the kernel > > > because we want you to make up a new HID and not be compatible with > > > what already exists." > > > > I gave an example above with the clocks but let's take a simpler, > > device-specific property like "smsc,irq-active-high". Is this documented > > anywhere apart from the kernel driver and the in-kernel DT smsc911x.txt? > > No. Are other non-Linux OS vendors going to look in the kernel source > > tree to implement support in their drivers? I doubt it and we could end > > up with two different paths in firmware for handling the same device. > > ACPI probably never was truly OS agnostic but with "PRP0001" we don't > > even try. > > Where is the documentation for the INT_CFG_IRQ_POL_ bit in the INT_CFG > register, that it controls? The documentation on the bindings really > ought to live near that, in an ideal world. If not there, at least in some common place like this: http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf And they could be specific to a newly allocated HID (like we do with DT for a newly allocated "compatible" string). > But that's still a non-sequitur in the context of this particular > discussion. The patch that was posted *already* keeps that very same > (optional) smsc,irq-active-high property. > > Again, it isn't relevant to the question of whether the driver is > matched via PRP0001 or a newly-allocated HID. > > The driver, as the patch was posted, *does* have the same set of > properties with the same meanings — because anything else would be > insane. Having the same set of properties makes sense. But not documenting them outside of Documentation/devicetree/bindings/ doesn't look right to me, from an OS-agnostic ACPI perspective. Let's take it the other way. A new driver is being submitted to Linux (or another OS) with support for ACPI (only). The developers may find it easier to use PRP0001 with their own _DSD invention. Do we ask the developers to submit DT bindings even if they don't immediately target DT (even harder if Linux is not the first target)? Or we skip this properties review and documenting process altogether? At least with DT, we usually see the bindings and kernel code together and have a chance to NAK. For _DSD we need something outside the Linux kernel community. But I agree with your statement above, it's not relevant whether a new _HID is used or PRP0001 + "compatible" when we don't even control which _DSD properties are added. As I said previously, disabling PRP0001 on ARM is more of a weak method of keeping track of which drivers are used with ACPI. My concern (and it won't go away) is a thoughtless migration of existing DT drivers and ARM SoCs to ACPI. PRP0001 makes it easier though it's _DSD actually facilitating this. > > Apart from a _DSD properties review process, what we need is (main) OS > > vendors enforcing it, possibly with stricter ACPI test suite. Disabling > > PRP0001 for ARM wouldn't solve the problem but at least it would make > > people think about what _DSD properties they need registered for their > > device (or I'm over optimistic and I should just stop caring ;)). > > I'm all for requiring pre-existing DT bindings to be "vetted" by the > nascent _DSD review process, before their use with PRP0001 can be > 'blessed'. If we want something enforced here, we need a better methodology than keeping track of which patches are submitted (and this would involve OS vendors support since in many cases their kernels are seen as the "canonical" implementation). > But in a world where people *are* going to go off and do their own > insane thing, we really might as well let them use the *same* thing > that we already had — in as many cases as possible — rather than > actually *making* them come up with a new insane thing all of their > very own. This only works as long as they target an existing driver with prior DT support (usually with reviewed bindings). If they have a new driver and only ACPI in mind, I'm pretty sure we'll end up with new insane things. That's why we need some form of _DSD properties review and "compatible" is one such property.
On 9/25/2015 5:28 PM, Catalin Marinas wrote: > On Thu, Sep 24, 2015 at 07:10:38PM +0100, David Woodhouse wrote: >> On Thu, 2015-09-24 at 16:15 +0100, Catalin Marinas wrote: >>> With "PRP0001", they can skip the _DSD properties review process (not >>> that they bother much currently) as long as the existing DT support >>> covers their needs. >> So no change there then. I take it the smsc911x bindings didn't go >> through this mythical _DSD properties review process either, right? > Right. And that's another reason I disagree with this patchset (I don't > always agree with everything that's coming out of ARM Ltd ;)). > > Now, in Jeremy's defence, this _DSD process looks quite mythical indeed. > We had an ARM firmware ecosystem meeting earlier this year where we > agreed to set up such process (at least for the ARM world): > > https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/CoreUpstreamNotes > > I can see some form of a process here: > > http://www.uefi.org/sites/default/files/resources/web-page-v2.pdf That was just an attempt and the final one will most likely be different. > But, at least to me, it's not clear how you submit properties to such > registry, what's the review process, who are the reviewers. I know there > was a mailing list set up but I don't see it listed above. For the record, this is under discussion right now. Also for the record, I'm not speaking for Intel. All of my comments here are based on my personal opinions. >>> However, we don't want to emulate DT in ACPI but > first try the >>> established ACPI methods and only use _DSD where these are >>> insufficient. Automatically converting existing drivers and encouraging >>> people to use "PRP0001" does not provide them with any incentive to try >>> harder and learn the "ACPI way". >> But again, we're *not* looking at a simplistic transliteration of >> properties. > I know you and I don't look at it this way but, based on historical > evidence, I'm sure that some ARM vendors will try to do exactly this > (and be able to claim "ACPI support" when they were happily using DT). > >>>> In some ways, your proposal would be actively *counterproductive*. You >>>> say you want to train people *not* to keep patching the kernel. But >>>> where they *could* have just used PRP0001 and used a pre-existing >>>> kernel, you then tell them "oh, but now you need to patch the kernel >>>> because we want you to make up a new HID and not be compatible with >>>> what already exists." >>> I gave an example above with the clocks but let's take a simpler, >>> device-specific property like "smsc,irq-active-high". Is this documented >>> anywhere apart from the kernel driver and the in-kernel DT smsc911x.txt? >>> No. Are other non-Linux OS vendors going to look in the kernel source >>> tree to implement support in their drivers? I doubt it and we could end >>> up with two different paths in firmware for handling the same device. >>> ACPI probably never was truly OS agnostic but with "PRP0001" we don't >>> even try. >> Where is the documentation for the INT_CFG_IRQ_POL_ bit in the INT_CFG >> register, that it controls? The documentation on the bindings really >> ought to live near that, in an ideal world. > If not there, at least in some common place like this: > > http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf > > And they could be specific to a newly allocated HID (like we do with DT > for a newly allocated "compatible" string). > >> But that's still a non-sequitur in the context of this particular >> discussion. The patch that was posted *already* keeps that very same >> (optional) smsc,irq-active-high property. >> >> Again, it isn't relevant to the question of whether the driver is >> matched via PRP0001 or a newly-allocated HID. >> >> The driver, as the patch was posted, *does* have the same set of >> properties with the same meanings — because anything else would be >> insane. > Having the same set of properties makes sense. But not documenting them > outside of Documentation/devicetree/bindings/ doesn't look right to me, > from an OS-agnostic ACPI perspective. They are documented in the code too through the requirements that the existing drivers have. If I have a piece of hardware and a driver for it, it really shouldn't matter whether I use a DT or ACPI in my platform. The driver should just work in both cases. It doesn't have to work in exactly the same way, but it should work. That's not the case today, sadly. > Let's take it the other way. A new driver is being submitted to Linux > (or another OS) with support for ACPI (only). The developers may find it > easier to use PRP0001 with their own _DSD invention. Do we ask the > developers to submit DT bindings even if they don't immediately target > DT (even harder if Linux is not the first target)? Or we skip this > properties review and documenting process altogether? At least with DT, > we usually see the bindings and kernel code together and have a chance > to NAK. For _DSD we need something outside the Linux kernel community. > > But I agree with your statement above, it's not relevant whether a new > _HID is used or PRP0001 + "compatible" when we don't even control which > _DSD properties are added. > > As I said previously, disabling PRP0001 on ARM is more of a weak method > of keeping track of which drivers are used with ACPI. My concern (and it > won't go away) is a thoughtless migration of existing DT drivers and ARM > SoCs to ACPI. PRP0001 makes it easier though it's _DSD actually > facilitating this. Even if they tried to do that, infrastructure is missing for things like regulators etc (because the frameworks there use of_ helpers directly IIRC and I'm not aware of any plans to change that) and on the other hand GPIO already depends on _CRS being used in accordance with ACPI anyway (even if _DSD properties are used), so I'd say this would be practically difficult rather. IOW, using _DSD properties for individual devices/drivers is easy. Doing the same thing for the whole system, not so much. >>> Apart from a _DSD properties review process, what we need is (main) OS >>> vendors enforcing it, possibly with stricter ACPI test suite. Disabling >>> PRP0001 for ARM wouldn't solve the problem but at least it would make >>> people think about what _DSD properties they need registered for their >>> device (or I'm over optimistic and I should just stop caring ;)). >> I'm all for requiring pre-existing DT bindings to be "vetted" by the >> nascent _DSD review process, before their use with PRP0001 can be >> 'blessed'. > If we want something enforced here, we need a better methodology than > keeping track of which patches are submitted (and this would involve OS > vendors support since in many cases their kernels are seen as the > "canonical" implementation). > >> But in a world where people *are* going to go off and do their own >> insane thing, we really might as well let them use the *same* thing >> that we already had — in as many cases as possible — rather than >> actually *making* them come up with a new insane thing all of their >> very own. > This only works as long as they target an existing driver with prior DT > support (usually with reviewed bindings). Exactly. And this is what we (David and me at least) want to be possible. It would be a failure if people had to write separate drivers for ACPI-based and DT-based platforms to handle the same hardware, at least at the leaf driver level. > If they have a new driver and > only ACPI in mind, I'm pretty sure we'll end up with new insane things. We will. With or without _DSD and all. That had happened way before _DSD was introduced. And by the way, demonizing _DSD doesn't help you at all, because there was the *much* *worse* thing called _DSM way before and it allowed of the same things as _DSD does and more. And it still is there and it very well may be used to feed an entire DT in binary format to the kernel when no one is looking. So this is all about tradeoffs. If you don't give people enough rope to do something, they'll find another way and you may hate that one, but then it'll be too late. > That's why we need some form of _DSD properties review and "compatible" > is one such property. > Yes, we need and want to set up a process for registering _DSD properties. That turns out a bit more complicated than we thought it would be, though. No, that won't prevent people from doing insane things with properties (or similar) they never register and they support in their out-of-the-tree drivers they never attempt to mainline. But let's be honest that this can happen with DT just as well. And it seems to me that we're spending more time on doing politics than on solving technical problems which should really be our focus IMO. And the underlying technical problem to me is what I said before: Device drivers should work regardless of what way the configuration information is provided to the kernel. Indeed, they shouldn't even have to care about that. Do you agree with that? Thanks, Rafael
On Fri, 2015-09-25 at 16:28 +0100, Catalin Marinas wrote: > > This only works as long as they target an existing driver with prior DT > support (usually with reviewed bindings). If they have a new driver and > only ACPI in mind, I'm pretty sure we'll end up with new insane things. > That's why we need some form of _DSD properties review and "compatible" > is one such property. Sure, that makes a lot of sense. My main concern is that we don't end up with gratuitously *different* property sets for DT vs. ACPI. That way we end up with either two separate drivers, or abstracting the core of the driver out and having two bindings for it (much as we do for PCI vs. platform/etc for some devices already). We don't want that pain where we can avoid it. And we don't want people to *have* to hack the kernel driver to migrate to ACPI, if we can avoid it. Sometimes it might be worth being different — if the DT binding is utterly crap, and deserves to be thrown away. In that case, by all means invent a new binding. But if we're going to accept the pain of having multiple bindings, why not make the *new* one work via DT too anyway. I'd be happy to see the existing DT bindings put through the nascent _DSD review process — such as it is — and into the database. One at a time on a case-by-case basis as they get used, perhaps.
Adding Charles Garcia-Tobin from ARM.... On 09/25/2015 09:28 AM, Catalin Marinas wrote: > On Thu, Sep 24, 2015 at 07:10:38PM +0100, David Woodhouse wrote: >> On Thu, 2015-09-24 at 16:15 +0100, Catalin Marinas wrote: >>> With "PRP0001", they can skip the _DSD properties review process (not >>> that they bother much currently) as long as the existing DT support >>> covers their needs. >> >> So no change there then. I take it the smsc911x bindings didn't go >> through this mythical _DSD properties review process either, right? > > Right. And that's another reason I disagree with this patchset (I don't > always agree with everything that's coming out of ARM Ltd ;)). > > Now, in Jeremy's defence, this _DSD process looks quite mythical indeed. > We had an ARM firmware ecosystem meeting earlier this year where we > agreed to set up such process (at least for the ARM world): > > https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/CoreUpstreamNotes > > I can see some form of a process here: > > http://www.uefi.org/sites/default/files/resources/web-page-v2.pdf > > But, at least to me, it's not clear how you submit properties to such > registry, what's the review process, who are the reviewers. I know there > was a mailing list set up but I don't see it listed above. To repeat Rafael a bit, getting the process set up -- moving it from the mythical to the practical -- is taking longer than expected. Some folks that had agreed to do so made no progress at all for many months, but I was way too polite and waited for something to happen. So, it turns out I'm impatient at times. This _DSD stuff is bugging me and the itch needs scratching. And, it absolutely affects ACPI on ARM servers so it has my interest. I have thrown out some basic proposals, and in conjunction with the ASWG (ACPI Spec Working Group of UEFI), Rafael, and Charles, we're trying to get some things decided and moving. Here is where I think we are: -- We have defined a very clear boundary between ASWG and the definition of _DSD device properties. The bottom line is that all device property definition is outside the scope of the ASWG; the ASWG has defined the structure they must take in ASL, but the actual content of any _DSD we can discuss and define in public forums without having to concern ourselves about the spec in any way. This was an essential step in order to make IP rules clear. -- There is a mailing list set up (dsd@acpica.org), originally set up as the starting point for a review process. I have recently started up some discussion there on what needs to be submitted. I will soon be posting updates to the discussion to lay out proposed review processes and the proposed formalisms to be used in describing device properties for review. Please subscribe and read the archives (not very large) that can be found here: https://lists.acpica.org/mailman/listinfo/dsd In the meantime, device property definition review can start occurring on the dsd@acpica.org list with the caveat that process is still being defined. At least if things are put in the email archive, we've got a starting point. -- Unfortunately, the material currently on the UEFI site about _DSD (other than its definition) should be ignored -- it reflects much earlier views and was incomplete to begin with. I wrote what's there and have no qualms in saying it is now inadequate. This is being corrected but it needs to go through ASWG to be fixed. -- An initial draft of a proposal for a metalanguage for describing and storing device properties has been under review. I'm working out v2 of this formalism. This will allow us to document the device properties in a reliable way, and build a repository of the definitions that is easily shared. -- What documentation/proposals we do have I have stashed in github. Please see https://github.com/ahs3/dsd/tree/v-next/ for the work in progress. What needs to happen next? -- First, finalize the review process (discussion on dsd@apcica.org). -- Second, finalize the reviewers (discussion on dsd@). -- Third, but in parallel, finalize formalisms for describing device properties and build the tools needs to validate them, store them, maintain them, and store them for public consumption (discussion also on dsd@). -- Make a couple of decisions (see below...) >>> However, we don't want to emulate DT in ACPI but > first try the >>> established ACPI methods and only use _DSD where these are >>> insufficient. Automatically converting existing drivers and encouraging >>> people to use "PRP0001" does not provide them with any incentive to try >>> harder and learn the "ACPI way". >> >> But again, we're *not* looking at a simplistic transliteration of >> properties. > > I know you and I don't look at it this way but, based on historical > evidence, I'm sure that some ARM vendors will try to do exactly this > (and be able to claim "ACPI support" when they were happily using DT). Based on the discussion so far, I think everyone is agreeing that we do NOT want to just blindly reproduce DT using _DSD properties. Is that a fair statement? >>>> In some ways, your proposal would be actively *counterproductive*. You >>>> say you want to train people *not* to keep patching the kernel. But >>>> where they *could* have just used PRP0001 and used a pre-existing >>>> kernel, you then tell them "oh, but now you need to patch the kernel >>>> because we want you to make up a new HID and not be compatible with >>>> what already exists." >>> >>> I gave an example above with the clocks but let's take a simpler, >>> device-specific property like "smsc,irq-active-high". Is this documented >>> anywhere apart from the kernel driver and the in-kernel DT smsc911x.txt? >>> No. Are other non-Linux OS vendors going to look in the kernel source >>> tree to implement support in their drivers? I doubt it and we could end >>> up with two different paths in firmware for handling the same device. >>> ACPI probably never was truly OS agnostic but with "PRP0001" we don't >>> even try. >> >> Where is the documentation for the INT_CFG_IRQ_POL_ bit in the INT_CFG >> register, that it controls? The documentation on the bindings really >> ought to live near that, in an ideal world. > > If not there, at least in some common place like this: > > http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf > > And they could be specific to a newly allocated HID (like we do with DT > for a newly allocated "compatible" string). A common location has some other rules, too: (1) people from Microsoft or other OS vendors (e.g., BSDs) need to be able to easily access it, and (2) firmware developers needs to be able to easily find it and use it. My thinking today is that I can (and will) set up a repository in the github I'm using that will contain a human readable formal description of all the approved device properties. Further (thanks to Mark Brown for the idea), there needs to be a second repository co-located with the first that contains device properties discovered in the wild but never "approved." I'll even offer to maintain these repositories for the time being, until we as a community decide on something better. >> But that's still a non-sequitur in the context of this particular >> discussion. The patch that was posted *already* keeps that very same >> (optional) smsc,irq-active-high property. >> >> Again, it isn't relevant to the question of whether the driver is >> matched via PRP0001 or a newly-allocated HID. >> >> The driver, as the patch was posted, *does* have the same set of >> properties with the same meanings — because anything else would be >> insane. > > Having the same set of properties makes sense. But not documenting them > outside of Documentation/devicetree/bindings/ doesn't look right to me, > from an OS-agnostic ACPI perspective. > > Let's take it the other way. A new driver is being submitted to Linux > (or another OS) with support for ACPI (only). The developers may find it > easier to use PRP0001 with their own _DSD invention. Do we ask the > developers to submit DT bindings even if they don't immediately target > DT (even harder if Linux is not the first target)? Or we skip this > properties review and documenting process altogether? At least with DT, > we usually see the bindings and kernel code together and have a chance > to NAK. For _DSD we need something outside the Linux kernel community. > > But I agree with your statement above, it's not relevant whether a new > _HID is used or PRP0001 + "compatible" when we don't even control which > _DSD properties are added. > > As I said previously, disabling PRP0001 on ARM is more of a weak method > of keeping track of which drivers are used with ACPI. My concern (and it > won't go away) is a thoughtless migration of existing DT drivers and ARM > SoCs to ACPI. PRP0001 makes it easier though it's _DSD actually > facilitating this. I have to agree with Catalin here. I was on the ASWG when _DSD was proposed, and when PRP0001 was proposed and they seemed reasonable at the time. However, the more I think about PRP0001, the more I think I dislike it. It allows for short-circuiting any sort of review about device properties in the drivers, regardless of whether they support DT or ACPI or both -- and people will choose the path of least resistance given any choice at all. >>> Apart from a _DSD properties review process, what we need is (main) OS >>> vendors enforcing it, possibly with stricter ACPI test suite. Disabling >>> PRP0001 for ARM wouldn't solve the problem but at least it would make >>> people think about what _DSD properties they need registered for their >>> device (or I'm over optimistic and I should just stop caring ;)). It's not foolproof, but if we disallow PRP0001 we might be able to encourage some sort of review to occur before code gets dropped into the kernel where it will need to be maintained forever. So, here's another decision point: do we disallow the use of PRP0001 for device properties on ARMv8 servers using ACPI? And should other architectures be able to do as they wish with PRP0001? The previous discussion seems to say "yes" to both questions, correct? >> >> I'm all for requiring pre-existing DT bindings to be "vetted" by the >> nascent _DSD review process, before their use with PRP0001 can be >> 'blessed'. > > If we want something enforced here, we need a better methodology than > keeping track of which patches are submitted (and this would involve OS > vendors support since in many cases their kernels are seen as the > "canonical" implementation). Agreed. This is what Charles, Rafael and I have started discussing on the dsd@acpica.org mailing list. I have proposed a formalism for describing properties (actively being revised) and we have some basic process proposals being discussed. More voices are needed; we're just starting to get things moving after they've been in stasis so long. While I cannot speak for Red Hat as a whole, for myself I really want for there to be a single place I can go to where I can find out which device properties are actually defined, which are not, and to be able to point to when some random bit of hardware or firmware decides to do something completely different and breaks my OS. Help Charles, Rafael and I define the vetting process and the things to be vetted and I think we can have a decent methodology in place. >> But in a world where people *are* going to go off and do their own >> insane thing, we really might as well let them use the *same* thing >> that we already had — in as many cases as possible — rather than >> actually *making* them come up with a new insane thing all of their >> very own. > > This only works as long as they target an existing driver with prior DT > support (usually with reviewed bindings). If they have a new driver and > only ACPI in mind, I'm pretty sure we'll end up with new insane things. > That's why we need some form of _DSD properties review and "compatible" > is one such property. > Again, based on the emails so far, it seems everyone is agreeing that we need a review process, and that we need to make sure we don't accept anything into the kernel that has not been reviewed; it won't fix everything, but it at least gives us a fighting chance. Any disagreement with that?
On 01/10/2015 03:23, "ahs3@redhat.com" <ahs3@redhat.com> wrote: >Adding Charles Garcia-Tobin from ARM.... Thanks and sorry for chiming in late. You¹ve pretty much said it all, but I had some comments. First one, is that if folk want to see what¹s happening with the DSD process, then please join the dsd mailing list and read this posting : [DSD] DSD v2 <https://lists.acpica.org/pipermail/dsd/2015-September/000026.html> To me this thread reads like there is a lot of violent agreement on what _DSD should not be used for, and on not breaking existing ACPI<->kernel interfaces. This is good, and essentially what we tried to capture in the _DSD review process. However, a lot of concern does remain on PRP0001. I share the worry that PRP00001 facilitates ³thoughtless migration² of existing DT to _DSD. I understand the need for porting, but above all ACPI is an OS agnostic standard, and whilst PRP0001 does provide porting convenience, I don¹t think we should be looking at it in ACPI circles unless we had wider agreement among OSs to use it. AFAIK PRP00001 has not actually been approved yet in the specification forum, and that it in itself is more of a concern for me, as the code has been pushed upstream. I guess it¹s up to Catalin, but disabling for ARM seems like a good idea right now, another option is to add tests to FWTS. Cheers Charles > >On 09/25/2015 09:28 AM, Catalin Marinas wrote: >> On Thu, Sep 24, 2015 at 07:10:38PM +0100, David Woodhouse wrote: >>> On Thu, 2015-09-24 at 16:15 +0100, Catalin Marinas wrote: >>>> With "PRP0001", they can skip the _DSD properties review process (not >>>> that they bother much currently) as long as the existing DT support >>>> covers their needs. >>> >>> So no change there then. I take it the smsc911x bindings didn't go >>> through this mythical _DSD properties review process either, right? >> >> Right. And that's another reason I disagree with this patchset (I don't >> always agree with everything that's coming out of ARM Ltd ;)). >> >> Now, in Jeremy's defence, this _DSD process looks quite mythical indeed. >> We had an ARM firmware ecosystem meeting earlier this year where we >> agreed to set up such process (at least for the ARM world): >> >> https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/CoreUpstreamNotes >> >> I can see some form of a process here: >> >> http://www.uefi.org/sites/default/files/resources/web-page-v2.pdf >> >> But, at least to me, it's not clear how you submit properties to such >> registry, what's the review process, who are the reviewers. I know there >> was a mailing list set up but I don't see it listed above. > >To repeat Rafael a bit, getting the process set up -- moving it from the >mythical to the practical -- is taking longer than expected. Some folks >that had agreed to do so made no progress at all for many months, but I >was way too polite and waited for something to happen. > >So, it turns out I'm impatient at times. This _DSD stuff is bugging me >and >the itch needs scratching. And, it absolutely affects ACPI on ARM servers >so it has my interest. I have thrown out some basic proposals, and in >conjunction with the ASWG (ACPI Spec Working Group of UEFI), Rafael, and >Charles, we're trying to get some things decided and moving. > >Here is where I think we are: > > -- We have defined a very clear boundary between ASWG and the >definition > of _DSD device properties. The bottom line is that all device >property > definition is outside the scope of the ASWG; the ASWG has defined >the > structure they must take in ASL, but the actual content of any _DSD > we can discuss and define in public forums without having to concern > ourselves about the spec in any way. This was an essential step in > order to make IP rules clear. > > -- There is a mailing list set up (dsd@acpica.org), originally set up >as > the starting point for a review process. I have recently started up > some discussion there on what needs to be submitted. I will soon be > posting updates to the discussion to lay out proposed review >processes > and the proposed formalisms to be used in describing device >properties > for review. Please subscribe and read the archives (not very large) > that can be found here: >https://lists.acpica.org/mailman/listinfo/dsd > > In the meantime, device property definition review can start >occurring > on the dsd@acpica.org list with the caveat that process is still >being > defined. At least if things are put in the email archive, we've >got a > starting point. > > -- Unfortunately, the material currently on the UEFI site about _DSD >(other > than its definition) should be ignored -- it reflects much earlier >views > and was incomplete to begin with. I wrote what's there and have no >qualms > in saying it is now inadequate. This is being corrected but it >needs to > go through ASWG to be fixed. > > -- An initial draft of a proposal for a metalanguage for describing and > storing device properties has been under review. I'm working out >v2 of > this formalism. This will allow us to document the device >properties in > a reliable way, and build a repository of the definitions that is >easily > shared. > > -- What documentation/proposals we do have I have stashed in github. >Please > see https://github.com/ahs3/dsd/tree/v-next/ for the work in >progress. > >What needs to happen next? > > -- First, finalize the review process (discussion on dsd@apcica.org). > > -- Second, finalize the reviewers (discussion on dsd@). > > -- Third, but in parallel, finalize formalisms for describing device > properties and build the tools needs to validate them, store them, > maintain them, and store them for public consumption (discussion > also on dsd@). > > -- Make a couple of decisions (see below...) > >>>> However, we don't want to emulate DT in ACPI but > first try the >>>> established ACPI methods and only use _DSD where these are >>>> insufficient. Automatically converting existing drivers and >>>>encouraging >>>> people to use "PRP0001" does not provide them with any incentive to >>>>try >>>> harder and learn the "ACPI way". >>> >>> But again, we're *not* looking at a simplistic transliteration of >>> properties. >> >> I know you and I don't look at it this way but, based on historical >> evidence, I'm sure that some ARM vendors will try to do exactly this >> (and be able to claim "ACPI support" when they were happily using DT). > >Based on the discussion so far, I think everyone is agreeing that we >do NOT want to just blindly reproduce DT using _DSD properties. Is that >a fair statement? > >>>>> In some ways, your proposal would be actively *counterproductive*. >>>>>You >>>>> say you want to train people *not* to keep patching the kernel. But >>>>> where they *could* have just used PRP0001 and used a pre-existing >>>>> kernel, you then tell them "oh, but now you need to patch the kernel >>>>> because we want you to make up a new HID and not be compatible with >>>>> what already exists." >>>> >>>> I gave an example above with the clocks but let's take a simpler, >>>> device-specific property like "smsc,irq-active-high". Is this >>>>documented >>>> anywhere apart from the kernel driver and the in-kernel DT >>>>smsc911x.txt? >>>> No. Are other non-Linux OS vendors going to look in the kernel source >>>> tree to implement support in their drivers? I doubt it and we could >>>>end >>>> up with two different paths in firmware for handling the same device. >>>> ACPI probably never was truly OS agnostic but with "PRP0001" we don't >>>> even try. >>> >>> Where is the documentation for the INT_CFG_IRQ_POL_ bit in the INT_CFG >>> register, that it controls? The documentation on the bindings really >>> ought to live near that, in an ideal world. >> >> If not there, at least in some common place like this: >> >> http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf >> >> And they could be specific to a newly allocated HID (like we do with DT >> for a newly allocated "compatible" string). > >A common location has some other rules, too: (1) people from Microsoft >or other OS vendors (e.g., BSDs) need to be able to easily access it, >and (2) firmware developers needs to be able to easily find it and use >it. My thinking today is that I can (and will) set up a repository in >the github I'm using that will contain a human readable formal description >of all the approved device properties. Further (thanks to Mark Brown for >the idea), there needs to be a second repository co-located with the first >that contains device properties discovered in the wild but never >"approved." >I'll even offer to maintain these repositories for the time being, until >we as a community decide on something better. > >>> But that's still a non-sequitur in the context of this particular >>> discussion. The patch that was posted *already* keeps that very same >>> (optional) smsc,irq-active-high property. >>> >>> Again, it isn't relevant to the question of whether the driver is >>> matched via PRP0001 or a newly-allocated HID. >>> >>> The driver, as the patch was posted, *does* have the same set of >>> properties with the same meanings ‹ because anything else would be >>> insane. >> >> Having the same set of properties makes sense. But not documenting them >> outside of Documentation/devicetree/bindings/ doesn't look right to me, >> from an OS-agnostic ACPI perspective. >> >> Let's take it the other way. A new driver is being submitted to Linux >> (or another OS) with support for ACPI (only). The developers may find it >> easier to use PRP0001 with their own _DSD invention. Do we ask the >> developers to submit DT bindings even if they don't immediately target >> DT (even harder if Linux is not the first target)? Or we skip this >> properties review and documenting process altogether? At least with DT, >> we usually see the bindings and kernel code together and have a chance >> to NAK. For _DSD we need something outside the Linux kernel community. >> >> But I agree with your statement above, it's not relevant whether a new >> _HID is used or PRP0001 + "compatible" when we don't even control which >> _DSD properties are added. >> >> As I said previously, disabling PRP0001 on ARM is more of a weak method >> of keeping track of which drivers are used with ACPI. My concern (and it >> won't go away) is a thoughtless migration of existing DT drivers and ARM >> SoCs to ACPI. PRP0001 makes it easier though it's _DSD actually >> facilitating this. > >I have to agree with Catalin here. I was on the ASWG when _DSD was >proposed, >and when PRP0001 was proposed and they seemed reasonable at the time. >However, >the more I think about PRP0001, the more I think I dislike it. It allows >for >short-circuiting any sort of review about device properties in the >drivers, >regardless of whether they support DT or ACPI or both -- and people will >choose >the path of least resistance given any choice at all. > >>>> Apart from a _DSD properties review process, what we need is (main) OS >>>> vendors enforcing it, possibly with stricter ACPI test suite. >>>>Disabling >>>> PRP0001 for ARM wouldn't solve the problem but at least it would make >>>> people think about what _DSD properties they need registered for their >>>> device (or I'm over optimistic and I should just stop caring ;)). > >It's not foolproof, but if we disallow PRP0001 we might be able to >encourage >some sort of review to occur before code gets dropped into the kernel >where >it will need to be maintained forever. > >So, here's another decision point: do we disallow the use of PRP0001 for >device >properties on ARMv8 servers using ACPI? And should other architectures >be able >to do as they wish with PRP0001? The previous discussion seems to say >"yes" to >both questions, correct? > >>> >>> I'm all for requiring pre-existing DT bindings to be "vetted" by the >>> nascent _DSD review process, before their use with PRP0001 can be >>> 'blessed'. >> >> If we want something enforced here, we need a better methodology than >> keeping track of which patches are submitted (and this would involve OS >> vendors support since in many cases their kernels are seen as the >> "canonical" implementation). > >Agreed. This is what Charles, Rafael and I have started discussing on the >dsd@acpica.org mailing list. I have proposed a formalism for describing >properties (actively being revised) and we have some basic process >proposals >being discussed. More voices are needed; we're just starting to get >things >moving after they've been in stasis so long. > >While I cannot speak for Red Hat as a whole, for myself I really want for >there >to be a single place I can go to where I can find out which device >properties >are actually defined, which are not, and to be able to point to when some >random >bit of hardware or firmware decides to do something completely different >and >breaks my OS. Help Charles, Rafael and I define the vetting process and >the >things to be vetted and I think we can have a decent methodology in place. > >>> But in a world where people *are* going to go off and do their own >>> insane thing, we really might as well let them use the *same* thing >>> that we already had ‹ in as many cases as possible ‹ rather than >>> actually *making* them come up with a new insane thing all of their >>> very own. >> >> This only works as long as they target an existing driver with prior DT >> support (usually with reviewed bindings). If they have a new driver and >> only ACPI in mind, I'm pretty sure we'll end up with new insane things. >> That's why we need some form of _DSD properties review and "compatible" >> is one such property. >> > >Again, based on the emails so far, it seems everyone is agreeing that we >need a review process, and that we need to make sure we don't accept >anything >into the kernel that has not been reviewed; it won't fix everything, but >it >at least gives us a fighting chance. Any disagreement with that? > >-- >ciao, >al >----------------------------------- >Al Stone >Software Engineer >Red Hat, Inc. >ahs3@redhat.com >-----------------------------------
On Mon, 2015-10-05 at 17:20 -0700, Charles Garcia-Tobin wrote: > it in ACPI circles > unless we had wider agreement among OSs to use it. AFAIK PRP00001 has not > actually been approved yet in the specification forum, and that it in > itself is more of a concern for me,as the code has been pushed upstream. Why would that be a concern? In that context it's just one device ID. Individual devices don't *need* to be approved. OK, the 'PRP' vendor prefix is not officially assigned but that's really a trivial piece of bureaucracy. > I guess it¹s up to Catalin, but disabling for ARM seems like a good idea > right now, another option is to add tests to FWTS. I understand the motivation to avoid embracing a whole bunch of crappy bindings. But I think that eschewing PRP0001 is the wrong technical approach to achieving that. It has false negatives — as soon as you have a *single* existing DT binding, perhaps something as simple as the serial port bindings from the CHRP days, you'll be in a situation where you can't use that. I've *got* hardware where I need to advertise a serial port with a clock-frequency property because it *isn't* compatible with PNP0501. And it has false positives — there's nothing to prevent people from doing ACPI-style bindings with crappy device bindings which also aren't approved. I think it's utterly naïve to believe that simply avoiding the use of PRP0001 + compatible for matching is going to have *any* significant beneficial effect whatsoever. It only makes life harder for all concerned. Perhaps a better approach would be to introduce something like CONFIG_UNAPPROVED_BINDINGS (which can't be set on ARM64), and those drivers which use bindings that *aren't* approved by Catalin's crack team of reviewers need to depend on !UNAPPROVED_BINDINGS. To be honest, I still think even *that* is somewhat naïve, but it's still a better way of implementing what you're actually trying to achieve, however optimistic you have to be to think it'll ever work in practice.
On 10/6/2015 1:08 PM, David Woodhouse wrote: > On Mon, 2015-10-05 at 17:20 -0700, Charles Garcia-Tobin wrote: >> it in ACPI circles >> unless we had wider agreement among OSs to use it. AFAIK PRP00001 has not >> actually been approved yet in the specification forum, and that it in >> itself is more of a concern for me,as the code has been pushed upstream. > Why would that be a concern? In that context it's just one device ID. > Individual devices don't *need* to be approved. OK, the 'PRP' vendor > prefix is not officially assigned but that's really a trivial piece of > bureaucracy. > >> I guess it¹s up to Catalin, but disabling for ARM seems like a good idea >> right now, another option is to add tests to FWTS. > I understand the motivation to avoid embracing a whole bunch of crappy > bindings. But I think that eschewing PRP0001 is the wrong technical > approach to achieving that. > > It has false negatives — as soon as you have a *single* existing DT > binding, perhaps something as simple as the serial port bindings from > the CHRP days, you'll be in a situation where you can't use that. > I've *got* hardware where I need to advertise a serial port with a > clock-frequency property because it *isn't* compatible with PNP0501. > > And it has false positives — there's nothing to prevent people from > doing ACPI-style bindings with crappy device bindings which also aren't > approved. > > I think it's utterly naïve to believe that simply avoiding the use of > PRP0001 + compatible for matching is going to have *any* significant > beneficial effect whatsoever. It only makes life harder for all > concerned. > > Perhaps a better approach would be to introduce something like > CONFIG_UNAPPROVED_BINDINGS (which can't be set on ARM64), and those > drivers which use bindings that *aren't* approved by Catalin's crack > team of reviewers need to depend on !UNAPPROVED_BINDINGS. To be honest, > I still think even *that* is somewhat naïve, but it's still a better > way of implementing what you're actually trying to achieve, however > optimistic you have to be to think it'll ever work in practice. > Also, let me mention one case PRP0001 is intended for that seems to be real. Say there is a piece of hardware that becomes so popular that the majority of vendors include it in their SoCs. Now all of them have to allocate ACPI/PNP device IDs for that and (without PRP0001) all of those device IDs need to go into the driver for that thing to make it work on all of the SoCs in question. As a result, the mainline kernel doesn't work on new SoCs with ACPI without modifications, although it works on all of them with DT in principle. Moreover, unmodified binary distro kernels don't work on new SoCs with ACPI, although they may work just fine on them with DT (that sort of seems to be a case that Red Hat may be interested in for one example, but I may be wrong here). In theory, that may be addressed by allocating a "master" ACPI/PNP device ID for that device and listing it in the device's _CID on all of the SoCs in question, but then it isn't particularly clear who's going to be responsible for allocating that device ID and "propagating" the knowledge of it to everybody interested to make things consistent. [Side note: Using a different vendor's device ID in a _CID is quite questionable, so the "master" one would need to be something "vendor agnostic" if that makes sense at all ...] Let alone the fact that PRP0001 is actually quite useful at the prototyping stage when it is premature to allocate a new device ID just yet. Taking that to the extreme, if someone whittles a board in his or her garage and wants to use it to drive their homemade grass watering system, having to invent a new device ID and put it into the existing driver that otherwise doesn't require any modifications is ... you know what I mean. All in all, I'd recommend some caution to ensure that the baby is not going away along with the bath water here. Thanks, Rafael
On 09/09/2015 11:10 AM, Marc Zyngier wrote: > Jeremy, > > I can see two issues here: we have a screaming interrupt, and > we seem to corrupt some workqueue. > > How did you get this to work? Firmware release? Marc, I'm responding because its been a month or so since my last response, and I haven't forgotten about this issue. First, any custom tianocore build (*) should work. The required changes have been in the last few linaro snapshots as well (http://snapshots.linaro.org/components/kernel/linaro-edk2/, currently at 40) but I personally haven't had a lot of luck with the prebuilt images due to problems unrelated to this change. Others may have more luck. * For those that don't know, tianno core is at: https://github.com/tianocore/edk2.git Use the master branch After setting the environment variables/dependencies appropriately: make -f ArmPlatformPkg/ArmJunoPkg/Makefile all Will create a functional ACPI firmware image for all recent kernels, including ACPI/PCIe ones. Thanks for everyone's patience on this,
diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c index 959aeea..0f21aa3 100644 --- a/drivers/net/ethernet/smsc/smsc911x.c +++ b/drivers/net/ethernet/smsc/smsc911x.c @@ -59,7 +59,9 @@ #include <linux/of_device.h> #include <linux/of_gpio.h> #include <linux/of_net.h> +#include <linux/acpi.h> #include <linux/pm_runtime.h> +#include <linux/property.h> #include "smsc911x.h" @@ -2362,59 +2364,46 @@ static const struct smsc911x_ops shifted_smsc911x_ops = { .tx_writefifo = smsc911x_tx_writefifo_shift, }; -#ifdef CONFIG_OF -static int smsc911x_probe_config_dt(struct smsc911x_platform_config *config, - struct device_node *np) +static int smsc911x_probe_config(struct smsc911x_platform_config *config, + struct device *dev) { - const char *mac; u32 width = 0; - if (!np) + if (!dev) return -ENODEV; - config->phy_interface = of_get_phy_mode(np); + config->phy_interface = device_get_phy_mode(dev); - mac = of_get_mac_address(np); - if (mac) - memcpy(config->mac, mac, ETH_ALEN); + device_get_mac_address(dev, config->mac, ETH_ALEN); - of_property_read_u32(np, "reg-shift", &config->shift); + device_property_read_u32(dev, "reg-shift", &config->shift); - of_property_read_u32(np, "reg-io-width", &width); + device_property_read_u32(dev, "reg-io-width", &width); if (width == 4) config->flags |= SMSC911X_USE_32BIT; else config->flags |= SMSC911X_USE_16BIT; - if (of_get_property(np, "smsc,irq-active-high", NULL)) + if (device_property_present(dev, "smsc,irq-active-high")) config->irq_polarity = SMSC911X_IRQ_POLARITY_ACTIVE_HIGH; - if (of_get_property(np, "smsc,irq-push-pull", NULL)) + if (device_property_present(dev, "smsc,irq-push-pull")) config->irq_type = SMSC911X_IRQ_TYPE_PUSH_PULL; - if (of_get_property(np, "smsc,force-internal-phy", NULL)) + if (device_property_present(dev, "smsc,force-internal-phy")) config->flags |= SMSC911X_FORCE_INTERNAL_PHY; - if (of_get_property(np, "smsc,force-external-phy", NULL)) + if (device_property_present(dev, "smsc,force-external-phy")) config->flags |= SMSC911X_FORCE_EXTERNAL_PHY; - if (of_get_property(np, "smsc,save-mac-address", NULL)) + if (device_property_present(dev, "smsc,save-mac-address")) config->flags |= SMSC911X_SAVE_MAC_ADDRESS; return 0; } -#else -static inline int smsc911x_probe_config_dt( - struct smsc911x_platform_config *config, - struct device_node *np) -{ - return -ENODEV; -} -#endif /* CONFIG_OF */ static int smsc911x_drv_probe(struct platform_device *pdev) { - struct device_node *np = pdev->dev.of_node; struct net_device *dev; struct smsc911x_data *pdata; struct smsc911x_platform_config *config = dev_get_platdata(&pdev->dev); @@ -2478,7 +2467,7 @@ static int smsc911x_drv_probe(struct platform_device *pdev) goto out_disable_resources; } - retval = smsc911x_probe_config_dt(&pdata->config, np); + retval = smsc911x_probe_config(&pdata->config, &pdev->dev); if (retval && config) { /* copy config parameters across to pdata */ memcpy(&pdata->config, config, sizeof(pdata->config)); @@ -2654,6 +2643,12 @@ static const struct of_device_id smsc911x_dt_ids[] = { MODULE_DEVICE_TABLE(of, smsc911x_dt_ids); #endif +static const struct acpi_device_id smsc911x_acpi_match[] = { + { "ARMH9118", 0 }, + { } +}; +MODULE_DEVICE_TABLE(acpi, smsc911x_acpi_match); + static struct platform_driver smsc911x_driver = { .probe = smsc911x_drv_probe, .remove = smsc911x_drv_remove, @@ -2661,6 +2656,7 @@ static struct platform_driver smsc911x_driver = { .name = SMSC_CHIPNAME, .pm = SMSC911X_PM_OPS, .of_match_table = of_match_ptr(smsc911x_dt_ids), + .acpi_match_table = ACPI_PTR(smsc911x_acpi_match), }, };
Add ACPI bindings for the smsc911x driver. Convert the DT specific calls to nonspecific device* calls, This allows the driver to work with both ACPI and DT configurations. Ethernet should now work when using ACPI on ARM Juno. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- drivers/net/ethernet/smsc/smsc911x.c | 48 +++++++++++++++++------------------- 1 file changed, 22 insertions(+), 26 deletions(-)