[PATCH 0/2] Enable smsc911x for use with ACPI

[PATCH 0/2] Enable smsc911x for use with ACPI

[Jeremy Linton]

This set of patches enables the front Ethernet port on the
ARM Juno development platform when used with an ACPI enabled kernel.

These patches covert the of_property* calls in the driver to the
DT/ACPI agnostic device_property* calls, and add the arm hardware
id to the acpi_match_table.

To support the above changes I copied a couple routines from
of_net into the properties.c file, and modified them to
be ACPI/DT agnostic. I'm not 100% sure this is the correct location
for these functions. But I think they are required to avoid having
a dozen different implementations scattered across assorted Ethernet
adapters that are being enabled to use ACPI properties.

Jeremy Linton (2):
  Add a matching set of device_ functions for determining mac/phy
  Convert smsc911x to use ACPI as well as DT

 drivers/base/property.c              | 73 ++++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/smsc/smsc911x.c | 48 +++++++++++-------------
 include/linux/property.h             |  4 ++
 3 files changed, 99 insertions(+), 26 deletions(-)

-- 
2.4.3

[PATCH 1/2] Add a matching set of device_ functions for determining mac/phy

[Jeremy Linton]

OF has some helper functions for parsing MAC and PHY settings.
In cases where the platform is providing this information rather
than the device itself, there needs to be similar functions for ACPI.

These functions are slightly modified versions of the ones in
of_net which can use information provided via DT or ACPI.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/base/property.c  | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  4 +++
 2 files changed, 77 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index f3f6d16..2e8cd14 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -16,6 +16,8 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/property.h>
+#include <linux/etherdevice.h>
+#include <linux/phy.h>
 
 /**
  * device_add_property_set - Add a collection of properties to a device object.
@@ -533,3 +535,74 @@ bool device_dma_is_coherent(struct device *dev)
 	return coherent;
 }
 EXPORT_SYMBOL_GPL(device_dma_is_coherent);
+
+/**
+ * device_get_phy_mode - Get phy mode for given device_node
+ * @dev:	Pointer to the given device
+ *
+ * The function gets phy interface string from property 'phy-mode' or
+ * 'phy-connection-type', and return its index in phy_modes table, or errno in
+ * error case.
+ */
+int device_get_phy_mode(struct device *dev)
+{
+	const char *pm;
+	int err, i;
+
+	err = device_property_read_string(dev, "phy-mode", &pm);
+	if (err < 0)
+		err = device_property_read_string(dev,
+						  "phy-connection-type", &pm);
+	if (err < 0)
+		return err;
+
+	for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++)
+		if (!strcasecmp(pm, phy_modes(i)))
+			return i;
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(device_get_phy_mode);
+
+static void *device_get_mac_addr(struct device *dev,
+				 const char *name, char *addr,
+				 int alen)
+{
+	int ret = device_property_read_u8_array(dev, name, addr, alen);
+
+	if (ret == 0 && is_valid_ether_addr(addr))
+		return addr;
+	return NULL;
+}
+
+/**
+ * Search the device tree for the best MAC address to use.  'mac-address' is
+ * checked first, because that is supposed to contain to "most recent" MAC
+ * address. If that isn't set, then 'local-mac-address' is checked next,
+ * because that is the default address.  If that isn't set, then the obsolete
+ * 'address' is checked, just in case we're using an old device tree.
+ *
+ * Note that the 'address' property is supposed to contain a virtual address of
+ * the register set, but some DTS files have redefined that property to be the
+ * MAC address.
+ *
+ * All-zero MAC addresses are rejected, because those could be properties that
+ * exist in the device tree, but were not set by U-Boot.  For example, the
+ * DTS could define 'mac-address' and 'local-mac-address', with zero MAC
+ * addresses.  Some older U-Boots only initialized 'local-mac-address'.  In
+ * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
+ * but is all zeros.
+*/
+void *device_get_mac_address(struct device *dev, char *addr, int alen)
+{
+	addr = device_get_mac_addr(dev, "mac-address", addr, alen);
+	if (addr)
+		return addr;
+
+	addr = device_get_mac_addr(dev, "local-mac-address", addr, alen);
+	if (addr)
+		return addr;
+
+	return device_get_mac_addr(dev, "address", addr, alen);
+}
+EXPORT_SYMBOL(device_get_mac_address);
diff --git a/include/linux/property.h b/include/linux/property.h
index 76ebde9..a59c6ee 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -166,4 +166,8 @@ void device_add_property_set(struct device *dev, struct property_set *pset);
 
 bool device_dma_is_coherent(struct device *dev);
 
+int device_get_phy_mode(struct device *dev);
+
+void *device_get_mac_address(struct device *dev, char *addr, int alen);
+
 #endif /* _LINUX_PROPERTY_H_ */
-- 
2.4.3

[PATCH 1/2] Add a matching set of device_ functions for determining mac/phy

[Florian Fainelli]

On 12/08/15 15:06, Jeremy Linton wrote:
> OF has some helper functions for parsing MAC and PHY settings.
> In cases where the platform is providing this information rather
> than the device itself, there needs to be similar functions for ACPI.
> 
> These functions are slightly modified versions of the ones in
> of_net which can use information provided via DT or ACPI.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---

[snip]

> +
> +static void *device_get_mac_addr(struct device *dev,
> +				 const char *name, char *addr,
> +				 int alen)
> +{
> +	int ret = device_property_read_u8_array(dev, name, addr, alen);
> +
> +	if (ret == 0 && is_valid_ether_addr(addr))
> +		return addr;
> +	return NULL;

The DT counterpart has an additional check on the properly length to be
ETH_ALEN, you might want to have such check here for consistency and
correctness.

Other than that, this looks good to me.
-- 
Florian

[PATCH 1/2] Add a matching set of device_ functions for determining mac/phy

[Jeremy Linton]

On 08/12/2015 05:13 PM, Florian Fainelli wrote:
> On 12/08/15 15:06, Jeremy Linton wrote:
>> +
>> +static void *device_get_mac_addr(struct device *dev,
>> +				 const char *name, char *addr,
>> +				 int alen)
>> +{
>> +	int ret = device_property_read_u8_array(dev, name, addr, alen);
>> +
>> +	if (ret == 0 && is_valid_ether_addr(addr))
>> +		return addr;
>> +	return NULL;
>
> The DT counterpart has an additional check on the properly length to be
> ETH_ALEN, you might want to have such check here for consistency and
> correctness.

I will add it back,

Thanks,

[PATCH 1/2] Add a matching set of device_ functions for determining mac/phy

[Robin Murphy]

Hi Jeremy,

On 12/08/15 23:06, Jeremy Linton wrote:
[...]
> +static void *device_get_mac_addr(struct device *dev,
> +				 const char *name, char *addr,
> +				 int alen)
> +{
> +	int ret = device_property_read_u8_array(dev, name, addr, alen);
> +
> +	if (ret == 0 && is_valid_ether_addr(addr))
> +		return addr;
> +	return NULL;
> +}

Not sure I understand the logic here - "return the same thing we were 
given if we updated it, or null if we didn't". It's only indicating 
success/failure (the caller can perfectly well cast its own buffer to a 
void * if it needs to), so why wouldn't you just return a normal int 
error code?

> +/**
> + * Search the device tree for the best MAC address to use.  'mac-address' is
> + * checked first, because that is supposed to contain to "most recent" MAC
> + * address. If that isn't set, then 'local-mac-address' is checked next,
> + * because that is the default address.  If that isn't set, then the obsolete
> + * 'address' is checked, just in case we're using an old device tree.
> + *
> + * Note that the 'address' property is supposed to contain a virtual address of
> + * the register set, but some DTS files have redefined that property to be the
> + * MAC address.
> + *
> + * All-zero MAC addresses are rejected, because those could be properties that
> + * exist in the device tree, but were not set by U-Boot.  For example, the
> + * DTS could define 'mac-address' and 'local-mac-address', with zero MAC
> + * addresses.  Some older U-Boots only initialized 'local-mac-address'.  In
> + * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
> + * but is all zeros.
> +*/
> +void *device_get_mac_address(struct device *dev, char *addr, int alen)
> +{
> +	addr = device_get_mac_addr(dev, "mac-address", addr, alen);
> +	if (addr)
> +		return addr;
> +
> +	addr = device_get_mac_addr(dev, "local-mac-address", addr, alen);
> +	if (addr)
> +		return addr;
> +
> +	return device_get_mac_addr(dev, "address", addr, alen);
> +}
> +EXPORT_SYMBOL(device_get_mac_address);

Same here, it's not at all apparent why this should return a void * 
instead of an int (or even possibly bool). of_get_mac_address is giving 
its caller back a _new_ pointer they didn't know about before; this isn't.

Robin.

[PATCH 1/2] Add a matching set of device_ functions for determining mac/phy

[Jeremy Linton]

Hello Robin,

On 08/13/2015 06:57 AM, Robin Murphy wrote:
>> +static void *device_get_mac_addr(struct device *dev,
>> +				 const char *name, char *addr,
>> +				 int alen)
>> +{
>> +	int ret = device_property_read_u8_array(dev, name, addr, alen);
>> +
>> +	if (ret == 0 && is_valid_ether_addr(addr))
>> +		return addr;
>> +	return NULL;
>> +}
>
> Not sure I understand the logic here - "return the same thing we were
> given if we updated it, or null if we didn't". It's only indicating
> success/failure (the caller can perfectly well cast its own buffer to a
> void * if it needs to), so why wouldn't you just return a normal int
> error code?


	No particular reason, other than initially I was trying to keep the 
function as similar as possible to the one in of_net. AKA copy paste 
job. I can convert the return types, but I was trying for a simple 
function rename. That way the users of the of version could be converted 
with relative ease, and the drivers which invented their own version of 
these functions could be changed to use this instead. Of course, that 
plan took a blow, when I added the addr/alen parameters.

	Same thing applies for the other function.


	

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Jeremy Linton]

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(-)

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

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Graeme Gregory]

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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Lorenzo Pieralisi]

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 at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Graeme Gregory]

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 at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Lorenzo Pieralisi]

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 at lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Jeremy Linton]

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

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny.

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Sudeep Holla]

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

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Catalin Marinas]

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.

-- 
Catalin

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Jeremy Linton]

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,

	

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[David Woodhouse]

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.

-- 
dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5691 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150923/20acbdc8/attachment.bin>

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Rafael J. Wysocki]

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

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[David Woodhouse]

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.

-- 
dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5691 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150923/39f45f7d/attachment.bin>

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Hanjun Guo]

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

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[David Woodhouse]

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).

-- 
dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5691 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150924/287573da/attachment.bin>

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Catalin Marinas]

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.

-- 
Catalin

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[David Woodhouse]

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.

-- 
dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5691 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150924/3c02fec3/attachment-0001.bin>

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Lorenzo Pieralisi]

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

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[David Woodhouse]

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.

-- 
dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5691 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150924/044ce685/attachment-0001.bin>

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Catalin Marinas]

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 ;)).

-- 
Catalin

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[David Woodhouse]

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.

-- 
dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5691 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150924/f2d6e027/attachment-0001.bin>

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Catalin Marinas]

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.

-- 
Catalin

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Rafael J. Wysocki]

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

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[David Woodhouse]

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.

-- 
dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5691 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150926/fe7b2387/attachment.bin>

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Al Stone]

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 at 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 at 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 at 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 at 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 at redhat.com
-----------------------------------

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Charles Garcia-Tobin]

On 01/10/2015 03:23, "ahs3 at 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 at 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 at 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 at 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 at 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 at redhat.com
>-----------------------------------

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[David Woodhouse]

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.

-- 
dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5691 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151006/8e51e461/attachment-0001.bin>

[PATCH 2/2] Convert smsc911x to use ACPI as well as DT

[Rafael J. Wysocki]

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

[PATCH 0/2] Enable smsc911x for use with ACPI

[David Miller]

From: Jeremy Linton <jeremy.linton@arm.com>
Date: Wed, 12 Aug 2015 17:06:25 -0500

> This set of patches enables the front Ethernet port on the
> ARM Juno development platform when used with an ACPI enabled kernel.
> 
> These patches covert the of_property* calls in the driver to the
> DT/ACPI agnostic device_property* calls, and add the arm hardware
> id to the acpi_match_table.
> 
> To support the above changes I copied a couple routines from
> of_net into the properties.c file, and modified them to
> be ACPI/DT agnostic. I'm not 100% sure this is the correct location
> for these functions. But I think they are required to avoid having
> a dozen different implementations scattered across assorted Ethernet
> adapters that are being enabled to use ACPI properties.

I realize that there are still some rinkles to work out, but I applied
this patch series as-is to net-next, and any fixups should be
submitted as followups.

Thanks.