Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On Wednesday, January 14, 2015 10:42:23 AM David Woodhouse wrote:
>
> --=-3dIl43yXcWwu/nzOqQWw
> Content-Type: multipart/mixed; boundary="=-ca0AFM5hvqL+pJIndiHh"
>
>
> --=-ca0AFM5hvqL+pJIndiHh
> Content-Type: text/plain; charset="UTF-8"
> Content-Transfer-Encoding: quoted-printable
>
> I'm looking again at updating of_serial to work with ACPI properties.=20
>
> Specifically, I want to support a serial port with a non-standard baud
> rate, something like this:
>
> Device(COM1) {
> Name(_HID, EisaId("PNP0501"))
> Name(_CID, EisaId("PRP0001"))
> Name(_DSD, Package() {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"compatible", Package () {"ns16550a"}},
> Package () {"clock-frequency", 2457600},
> }
> })
> ...
> }
>
> Firstly, the of_serial driver doesn't even get *invoked* unless I do
> this:
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 0d08373..eb1201a 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2083,6 +2086,8 @@ static int acpi_add_single_object(struct acpi_device =
> **child,
> ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Added %s [%s] parent %s\n",
> dev_name(&device->dev), (char *) buffer.pointer,
> device->parent ? dev_name(&device->parent->dev) : "(null)"));
> + if (device->data.of_compatible)
> + acpi_create_platform_device(device);
> kfree(buffer.pointer);
> *child =3D device;
> return 0;
>
> Now it doesn't work because it uses of_match_device() to look the device
> up and find the corresponding *data* for that entry in its match table.
> And without CONFIG_OF, I don't *have* of_match_device().
>
> We've talked about the fact that the platform bus probe function doesn't
> pass you the match ID. Is that something we could potentially fix now
> that things are a little more unified?
We can do that in my view.
> Or do we expect drivers still to have to use something like
> of_match_id() to do the lookup again for themselves... and in that case
> should we make of_match_id() available or produce a new
> device_match_id() that they are expected to switch to?
So far we've been targeting drivers that already have of_match_id() rather
than ones that want to use the unified interface from the start and not
necessarily with CONFIG_OF set. That looks like an overlooked use case to me
to be honest.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
I'm looking again at updating of_serial to work with ACPI properties.
Specifically, I want to support a serial port with a non-standard baud
rate, something like this:
Device(COM1) {
Name(_HID, EisaId("PNP0501"))
Name(_CID, EisaId("PRP0001"))
Name(_DSD, Package() {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () {"compatible", Package () {"ns16550a"}},
Package () {"clock-frequency", 2457600},
}
})
...
}
Firstly, the of_serial driver doesn't even get *invoked* unless I do
this:
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 0d08373..eb1201a 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2083,6 +2086,8 @@ static int acpi_add_single_object(struct acpi_device
**child,
ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Added %s [%s] parent %s\n",
dev_name(&device->dev), (char *) buffer.pointer,
device->parent ? dev_name(&device->parent->dev) : "(null)"));
+ if (device->data.of_compatible)
+ acpi_create_platform_device(device);
kfree(buffer.pointer);
*child = device;
return 0;
Now it doesn't work because it uses of_match_device() to look the device
up and find the corresponding *data* for that entry in its match table.
And without CONFIG_OF, I don't *have* of_match_device().
We've talked about the fact that the platform bus probe function doesn't
pass you the match ID. Is that something we could potentially fix now
that things are a little more unified?
Or do we expect drivers still to have to use something like
of_match_id() to do the lookup again for themselves... and in that case
should we make of_match_id() available or produce a new
device_match_id() that they are expected to switch to?
--
dwmw2
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index c79b43c..65cf850 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1114,14 +1114,14 @@ config SERIAL_NETX_CONSOLE
you can make it the console by answering Y to this option.
config SERIAL_OF_PLATFORM
- tristate "Serial port on Open Firmware platform bus"
- depends on OF
+ tristate "Serial port on firmware platform bus"
+ depends on OF || ACPI
depends on SERIAL_8250 || SERIAL_OF_PLATFORM_NWPSERIAL
help
- If you have a PowerPC based system that has serial ports
- on a platform specific bus, you should enable this option.
- Currently, only 8250 compatible ports are supported, but
- others can easily be added.
+ If you have a system which advertises its serial ports through
+ devicetree or ACPI, you should enable this option. Currently
+ only 8250 compatible and NWP ports and are supported, but others
+ can easily be added.
config SERIAL_OMAP
tristate "OMAP serial port support"
diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
index 64f1bab..54110e6 100644
--- a/drivers/tty/serial/of_serial.c
+++ b/drivers/tty/serial/of_serial.c
@@ -15,8 +15,7 @@
#include
#include
#include
-#include
-#include
+#include
#include
#include
#include
@@ -54,22 +53,22 @@ static inline void tegra_serial_handle_break(struct uart_port *port)
/*
* Fill a struct uart_port for a given device node
*/
-static int of_platform_serial_setup(struct platform_device *ofdev,
+static int of_platform_serial_setup(struct platform_device *pdev,
int type, struct uart_port *port,
struct of_serial_info *info)
{
- struct resource resource;
- struct device_node *np = ofdev->dev.of_node;
u32 clk, spd, prop;
- int ret;
+ int iotype = -1;
+ u32 res_start;
+ int ret, i;
memset(port, 0, sizeof *port);
- if (of_property_read_u32(np, "clock-frequency", &clk)) {
+ if (device_property_read_u32(&pdev->dev, "clock-frequency", &clk)) {
/* Get clk rate through clk driver if present */
- info->clk = clk_get(&ofdev->dev, NULL);
+ info->clk = clk_get(&pdev->dev, NULL);
if (IS_ERR(info->clk)) {
- dev_warn(&ofdev->dev,
+ dev_warn(&pdev->dev,
"clk or clock-frequency not defined\n");
return PTR_ERR(info->clk);
}
@@ -78,57 +77,63 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
clk = clk_get_rate(info->clk);
}
/* If current-speed was set, then try not to change it. */
- if (of_property_read_u32(np, "current-speed", &spd) == 0)
+ if (device_property_read_u32(&pdev->dev, "current-speed", &spd) == 0)
port->custom_divisor = clk / (16 * spd);
- ret = of_address_to_resource(np, 0, &resource);
- if (ret) {
- dev_warn(&ofdev->dev, "invalid address\n");
+ /* Check for shifted address mapping */
+ if (device_property_read_u32(&pdev->dev, "reg-offset", &prop) != 0)
+ prop = 0;
+
+ for (i = 0; iotype == -1 && i < pdev->num_resources; i++) {
+ struct resource *resource = &pdev->resource[i];
+ if (resource_type(resource) == IORESOURCE_MEM) {
+ iotype = UPIO_MEM;
+ port->mapbase = res_start +
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties? support
On Sat, Oct 18, 2014 at 10:35:49AM +0200, Grant Likely wrote: > On Wed, 15 Oct 2014 17:43:01 +0200 > , Darren Hart > wrote: > > > > > > On 10/15/14 17:17, Mark Rutland wrote: > > > On Wed, Oct 15, 2014 at 03:46:39PM +0100, Darren Hart wrote: > > > > >> Mark, what would you propose we do differently to enable this driver to > > >> be firmware-type agnostic? > > > > > > For this particular driver, all I'm asking for is that the > > > "used-by-rtas" property is not moved over from of_find_property to > > > device_get_property. It is irrelevant for all ACPI systems. Evidently my > > > comment was unclear; I apologise for that. > > > > So my objection here is that by keeping the of_* terms in the driver we > > are required to include of, although it does safely convert to returning > > NULL if !CONFIG_OF I suppose. > > This shouldn't be that controversial. There will be things that only make > sense for DT or only ACPI. Allowing the property to be processed when > the other interface is being used may tempt firmware authors to use the > property because it just happens to have a side effect that looks right > to them. > > I don't see any problem with factoring out those bits into a function > that is only called (or built) when the associated firmware interface is > used. In these situations, the driver isn't 100% generic, so having > small per-firmware hooks is absolutely okay and not a burden to > maintain. Hrm... well, I suppose this isn't a hill I want to die on. I can disagree and commit here :-) -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On Thu, 16 Oct 2014 16:55:56 +0200 , David Woodhouse wrote: > On Wed, 2014-10-15 at 17:43 +0200, Darren Hart wrote: > > > > So my objection here is that by keeping the of_* terms in the driver we > > are required to include of, although it does safely convert to returning > > NULL if !CONFIG_OF I suppose. > > New version removes everything but the of_match_id bits which we need to > match ACPI devices too. Perhaps they ought to be renamed, but I'm not > sure it's worth it. > > This also removes the call to platform_get_resource(IORESOURCE_MEM) and > fall back to platform_get_resource(IORESOURCE_IO) as discussed IRL with > Rafael. I'm not sure it's much of an improvement, mind you :) > > Still untested. I think it's OK to switch to platform_get_irq() and then > drop the irq_dispose_mapping() call, right? The platform_device takes > care of all of that for us? Well, the irq management code is all messed up, but what you are doing is indeed okay. Unfortunately for platform devices we can never free an IRQ once we've claimed it. That's a completely separate problem and you don't need to worry about it for this patch. g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On Wed, 15 Oct 2014 17:43:01 +0200 , Darren Hart wrote: > > > On 10/15/14 17:17, Mark Rutland wrote: > > On Wed, Oct 15, 2014 at 03:46:39PM +0100, Darren Hart wrote: > > >> Mark, what would you propose we do differently to enable this driver to > >> be firmware-type agnostic? > > > > For this particular driver, all I'm asking for is that the > > "used-by-rtas" property is not moved over from of_find_property to > > device_get_property. It is irrelevant for all ACPI systems. Evidently my > > comment was unclear; I apologise for that. > > So my objection here is that by keeping the of_* terms in the driver we > are required to include of, although it does safely convert to returning > NULL if !CONFIG_OF I suppose. This shouldn't be that controversial. There will be things that only make sense for DT or only ACPI. Allowing the property to be processed when the other interface is being used may tempt firmware authors to use the property because it just happens to have a side effect that looks right to them. I don't see any problem with factoring out those bits into a function that is only called (or built) when the associated firmware interface is used. In these situations, the driver isn't 100% generic, so having small per-firmware hooks is absolutely okay and not a burden to maintain. g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On Thu, 16 Oct 2014 16:55:56 +0200
, David Woodhouse
wrote:
> On Wed, 2014-10-15 at 17:43 +0200, Darren Hart wrote:
> >
> > So my objection here is that by keeping the of_* terms in the driver we
> > are required to include of, although it does safely convert to returning
> > NULL if !CONFIG_OF I suppose.
>
> New version removes everything but the of_match_id bits which we need to
> match ACPI devices too. Perhaps they ought to be renamed, but I'm not
> sure it's worth it.
>
> This also removes the call to platform_get_resource(IORESOURCE_MEM) and
> fall back to platform_get_resource(IORESOURCE_IO) as discussed IRL with
> Rafael. I'm not sure it's much of an improvement, mind you :)
>
> Still untested. I think it's OK to switch to platform_get_irq() and then
> drop the irq_dispose_mapping() call, right? The platform_device takes
> care of all of that for us?
>
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 26cec64..be95a4c 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1094,14 +1094,14 @@ config SERIAL_NETX_CONSOLE
> you can make it the console by answering Y to this option.
>
> config SERIAL_OF_PLATFORM
> - tristate "Serial port on Open Firmware platform bus"
> - depends on OF
> + tristate "Serial port on firmware platform bus"
> + depends on OF || ACPI
> depends on SERIAL_8250 || SERIAL_OF_PLATFORM_NWPSERIAL
> help
> - If you have a PowerPC based system that has serial ports
> - on a platform specific bus, you should enable this option.
> - Currently, only 8250 compatible ports are supported, but
> - others can easily be added.
> + If you have a system which advertises its serial ports through
> + devicetree or ACPI, you should enable this option. Currently
> + only 8250 compatible and NWP ports and are supported, but others
> + can easily be added.
>
> config SERIAL_OMAP
> tristate "OMAP serial port support"
> diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
> index 68d4455..cc6c99b 100644
> --- a/drivers/tty/serial/of_serial.c
> +++ b/drivers/tty/serial/of_serial.c
> @@ -14,8 +14,7 @@
> #include
> #include
> #include
> -#include
> -#include
> +#include
> #include
> #include
> #include
> @@ -53,22 +52,22 @@ static inline void tegra_serial_handle_break(struct
> uart_port *port)
> /*
> * Fill a struct uart_port for a given device node
> */
> -static int of_platform_serial_setup(struct platform_device *ofdev,
> +static int of_platform_serial_setup(struct platform_device *pdev,
> int type, struct uart_port *port,
> struct of_serial_info *info)
> {
> - struct resource resource;
> - struct device_node *np = ofdev->dev.of_node;
> u32 clk, spd, prop;
> - int ret;
> + int iotype = -1;
> + u32 res_start;
> + int ret, i;
>
> memset(port, 0, sizeof *port);
> - if (of_property_read_u32(np, "clock-frequency", &clk)) {
> + if (device_property_read_u32(&pdev->dev, "clock-frequency", &clk)) {
>
> /* Get clk rate through clk driver if present */
> - info->clk = clk_get(&ofdev->dev, NULL);
> + info->clk = clk_get(&pdev->dev, NULL);
> if (IS_ERR(info->clk)) {
> - dev_warn(&ofdev->dev,
> + dev_warn(&pdev->dev,
> "clk or clock-frequency not defined\n");
> return PTR_ERR(info->clk);
> }
> @@ -77,57 +76,63 @@ static int of_platform_serial_setup(struct
> platform_device *ofdev,
> clk = clk_get_rate(info->clk);
> }
> /* If current-speed was set, then try not to change it. */
> - if (of_property_read_u32(np, "current-speed", &spd) == 0)
> + if (device_property_read_u32(&pdev->dev, "current-speed", &spd) == 0)
> port->custom_divisor = clk / (16 * spd);
>
> - ret = of_address_to_resource(np, 0, &resource);
> - if (ret) {
> - dev_warn(&ofdev->dev, "invalid address\n");
> + /* Check for shifted address mapping */
> + if (device_property_read_u32(&pdev->dev, "reg-offset", &prop) != 0)
> + prop = 0;
> +
> + for (i = 0; iotype == -1 && i < pdev->num_resources; i++) {
> + struct resource *resource = &pdev->resource[i];
> + if (resource_type(resource) == IORESOURCE_MEM) {
> + iotype = UPIO_MEM;
> + port->mapbase = res_start + prop;
> + } else if (resource_type(resource) == IORESOURCE_IO) {
> + iotype = UPIO_PORT;
> + port->iobase = res_start + prop;
> + }
> +
> + res_start = resource->start;
> + }
> + if (iotype == -1) {
> + dev_warn(&pdev->dev, "invalid address\n");
> goto out;
> }
>
> spin_lock_in
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On Wed, 2014-10-15 at 17:43 +0200, Darren Hart wrote:
>
> So my objection here is that by keeping the of_* terms in the driver we
> are required to include of, although it does safely convert to returning
> NULL if !CONFIG_OF I suppose.
New version removes everything but the of_match_id bits which we need to
match ACPI devices too. Perhaps they ought to be renamed, but I'm not
sure it's worth it.
This also removes the call to platform_get_resource(IORESOURCE_MEM) and
fall back to platform_get_resource(IORESOURCE_IO) as discussed IRL with
Rafael. I'm not sure it's much of an improvement, mind you :)
Still untested. I think it's OK to switch to platform_get_irq() and then
drop the irq_dispose_mapping() call, right? The platform_device takes
care of all of that for us?
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 26cec64..be95a4c 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1094,14 +1094,14 @@ config SERIAL_NETX_CONSOLE
you can make it the console by answering Y to this option.
config SERIAL_OF_PLATFORM
- tristate "Serial port on Open Firmware platform bus"
- depends on OF
+ tristate "Serial port on firmware platform bus"
+ depends on OF || ACPI
depends on SERIAL_8250 || SERIAL_OF_PLATFORM_NWPSERIAL
help
- If you have a PowerPC based system that has serial ports
- on a platform specific bus, you should enable this option.
- Currently, only 8250 compatible ports are supported, but
- others can easily be added.
+ If you have a system which advertises its serial ports through
+ devicetree or ACPI, you should enable this option. Currently
+ only 8250 compatible and NWP ports and are supported, but others
+ can easily be added.
config SERIAL_OMAP
tristate "OMAP serial port support"
diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
index 68d4455..cc6c99b 100644
--- a/drivers/tty/serial/of_serial.c
+++ b/drivers/tty/serial/of_serial.c
@@ -14,8 +14,7 @@
#include
#include
#include
-#include
-#include
+#include
#include
#include
#include
@@ -53,22 +52,22 @@ static inline void tegra_serial_handle_break(struct
uart_port *port)
/*
* Fill a struct uart_port for a given device node
*/
-static int of_platform_serial_setup(struct platform_device *ofdev,
+static int of_platform_serial_setup(struct platform_device *pdev,
int type, struct uart_port *port,
struct of_serial_info *info)
{
- struct resource resource;
- struct device_node *np = ofdev->dev.of_node;
u32 clk, spd, prop;
- int ret;
+ int iotype = -1;
+ u32 res_start;
+ int ret, i;
memset(port, 0, sizeof *port);
- if (of_property_read_u32(np, "clock-frequency", &clk)) {
+ if (device_property_read_u32(&pdev->dev, "clock-frequency", &clk)) {
/* Get clk rate through clk driver if present */
- info->clk = clk_get(&ofdev->dev, NULL);
+ info->clk = clk_get(&pdev->dev, NULL);
if (IS_ERR(info->clk)) {
- dev_warn(&ofdev->dev,
+ dev_warn(&pdev->dev,
"clk or clock-frequency not defined\n");
return PTR_ERR(info->clk);
}
@@ -77,57 +76,63 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
clk = clk_get_rate(info->clk);
}
/* If current-speed was set, then try not to change it. */
- if (of_property_read_u32(np, "current-speed", &spd) == 0)
+ if (device_property_read_u32(&pdev->dev, "current-speed", &spd) == 0)
port->custom_divisor = clk / (16 * spd);
- ret = of_address_to_resource(np, 0, &resource);
- if (ret) {
- dev_warn(&ofdev->dev, "invalid address\n");
+ /* Check for shifted address mapping */
+ if (device_property_read_u32(&pdev->dev, "reg-offset", &prop) != 0)
+ prop = 0;
+
+ for (i = 0; iotype == -1 && i < pdev->num_resources; i++) {
+ struct resource *resource = &pdev->resource[i];
+ if (resource_type(resource) == IORESOURCE_MEM) {
+ iotype = UPIO_MEM;
+ port->mapbase = res_start + prop;
+ } else if (resource_type(resource) == IORESOURCE_IO) {
+ iotype = UPIO_PORT;
+ port->iobase = res_start + prop;
+ }
+
+ res_start = resource->start;
+ }
+ if (iotype == -1) {
+ dev_warn(&pdev->dev, "invalid address\n");
goto out;
}
spin_lock_init(&port->lock);
- port->mapbase = resource.start;
-
- /* Check for shifted address mapping */
- if (of_property_read_u32(np, "reg-offset", &prop) == 0)
-
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On Wednesday, October 15, 2014 05:43:01 PM Darren Hart wrote: > > On 10/15/14 17:17, Mark Rutland wrote: > > On Wed, Oct 15, 2014 at 03:46:39PM +0100, Darren Hart wrote: > > >> Mark, what would you propose we do differently to enable this driver to > >> be firmware-type agnostic? > > > > For this particular driver, all I'm asking for is that the > > "used-by-rtas" property is not moved over from of_find_property to > > device_get_property. It is irrelevant for all ACPI systems. Evidently my > > comment was unclear; I apologise for that. > > So my objection here is that by keeping the of_* terms in the driver we > are required to include of, although it does safely convert to returning > NULL if !CONFIG_OF I suppose. Agreed. > > We have status = "disabled" as a less specific mechanism for telling the > > OS to ignore a node in DT. I was under the impression that ACPI already > > had a mechanism for marking devices to be ignored, but perhaps I am > > mistaken. > > That is correct, in ACPI this would be properly implemented with the > _STA reserved named method. In which case it wouldn't enumerate. > > > > > The concerns I mentioned at the end of my original reply were of a more > > general nature than this particular device description. Moreover, to me, the question really is "Does this driver need to be any different depending on whether DT or ACPI is used by the platform and if so, then why?". In my opinion, there is no technical reason for such differences to be present in this particular case. The fact that the "used-by-rtas" property does not make sense for the ACPI case doesn't imply that the driver should not be allowed to check it then. -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On 10/15/14 17:17, Mark Rutland wrote: > On Wed, Oct 15, 2014 at 03:46:39PM +0100, Darren Hart wrote: >> Mark, what would you propose we do differently to enable this driver to >> be firmware-type agnostic? > > For this particular driver, all I'm asking for is that the > "used-by-rtas" property is not moved over from of_find_property to > device_get_property. It is irrelevant for all ACPI systems. Evidently my > comment was unclear; I apologise for that. So my objection here is that by keeping the of_* terms in the driver we are required to include of, although it does safely convert to returning NULL if !CONFIG_OF I suppose. > We have status = "disabled" as a less specific mechanism for telling the > OS to ignore a node in DT. I was under the impression that ACPI already > had a mechanism for marking devices to be ignored, but perhaps I am > mistaken. That is correct, in ACPI this would be properly implemented with the _STA reserved named method. In which case it wouldn't enumerate. > > The concerns I mentioned at the end of my original reply were of a more > general nature than this particular device description. > > Thanks, > Mark. > -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On Wed, Oct 15, 2014 at 03:46:39PM +0100, Darren Hart wrote: > > > On 10/15/14 16:08, David Woodhouse wrote: > > > >> We have been checking for all DT platforms, and that's a bug for DT. > >> Copying that bug to ACPI is inexcusable given we know it's a bug to do > >> so. > > > > We'll, perhaps it should be named 'used-by-firmware' and actually it's > > just as valid under ACPI as it is on RTAS systems. All it does is stop the > > OS from using the port. > > > >> I understand that. However, where a binding doesn't make sense (as in > >> this case), it shouldn't be enabled for ACPI as it provides a larger > >> surface area for misuse, for no benefit. > > > > These are *optional* properties. They were optional precisely *because* > > they only make sense in some cases. I don't know that it makes sense to > > take them away. The benefit we get is *consistency*. For example if > > someone *does* use the property in question as 'used-by-firmware' and > > expects the OS not to touch it, we don't want that to change behaviour > > between ACPI and fdt boots. > > My comment was going to be along the same lines. It is an optional > parameter, which is what I would expect for a firmware-specific type of > property. > > I also don't agree that this is "copying that bug to ACPI". This line of > code has no impact to ACPI. No ACPI implementation should add this, > certainly not if it was actually tested as it would not run if it was > present in the _DSD. So... what's the problem exactly? Or perhaps more > specifically: > > Mark, what would you propose we do differently to enable this driver to > be firmware-type agnostic? For this particular driver, all I'm asking for is that the "used-by-rtas" property is not moved over from of_find_property to device_get_property. It is irrelevant for all ACPI systems. Evidently my comment was unclear; I apologise for that. We have status = "disabled" as a less specific mechanism for telling the OS to ignore a node in DT. I was under the impression that ACPI already had a mechanism for marking devices to be ignored, but perhaps I am mistaken. The concerns I mentioned at the end of my original reply were of a more general nature than this particular device description. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
> We have been checking for all DT platforms, and that's a bug for DT. > Copying that bug to ACPI is inexcusable given we know it's a bug to do > so. We'll, perhaps it should be named 'used-by-firmware' and actually it's just as valid under ACPI as it is on RTAS systems. All it does is stop the OS from using the port. > I understand that. However, where a binding doesn't make sense (as in > this case), it shouldn't be enabled for ACPI as it provides a larger > surface area for misuse, for no benefit. These are *optional* properties. They were optional precisely *because* they only make sense in some cases. I don't know that it makes sense to take them away. The benefit we get is *consistency*. For example if someone *does* use the property in question as 'used-by-firmware' and expects the OS not to touch it, we don't want that to change behaviour between ACPI and fdt boots. -- dwmw2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
> My comment was going to be along the same lines. It is an optional > parameter, which is what I would expect for a firmware-specific type of > property. Right. Fundamentally, device properties (in DT or ACPI) exist to describe the hardware in a generic and abstract fashion. It's a slippery slope from saying "you don't need this property because you know you whether you are on the *foo* architecture", to saying "you don't this property because you know whether you're on a Assabet or not." I think it's wrong to go down that path. -- dwmw2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On 10/15/14 16:08, David Woodhouse wrote: > >> We have been checking for all DT platforms, and that's a bug for DT. >> Copying that bug to ACPI is inexcusable given we know it's a bug to do >> so. > > We'll, perhaps it should be named 'used-by-firmware' and actually it's > just as valid under ACPI as it is on RTAS systems. All it does is stop the > OS from using the port. > >> I understand that. However, where a binding doesn't make sense (as in >> this case), it shouldn't be enabled for ACPI as it provides a larger >> surface area for misuse, for no benefit. > > These are *optional* properties. They were optional precisely *because* > they only make sense in some cases. I don't know that it makes sense to > take them away. The benefit we get is *consistency*. For example if > someone *does* use the property in question as 'used-by-firmware' and > expects the OS not to touch it, we don't want that to change behaviour > between ACPI and fdt boots. My comment was going to be along the same lines. It is an optional parameter, which is what I would expect for a firmware-specific type of property. I also don't agree that this is "copying that bug to ACPI". This line of code has no impact to ACPI. No ACPI implementation should add this, certainly not if it was actually tested as it would not run if it was present in the _DSD. So... what's the problem exactly? Or perhaps more specifically: Mark, what would you propose we do differently to enable this driver to be firmware-type agnostic? -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On Wed, Oct 15, 2014 at 02:28:56PM +0100, David Woodhouse wrote: > On Wed, 2014-10-15 at 14:15 +0100, Mark Rutland wrote: > > > @@ -155,7 +168,7 @@ static int of_platform_serial_probe(struct > > > platform_device *ofdev) > > > if (!match) > > > return -EINVAL; > > > > > > - if (of_find_property(ofdev->dev.of_node, "used-by-rtas", NULL)) > > > + if (!device_get_property(&ofdev->dev, "used-by-rtas", NULL)) > > > return -EBUSY; > > > > This property should never be present on an ACPI system. RTAS is a > > completely different firmware interface on PowerPC. > > Yes, I sincerely hope we never see used-by-rtas being set on a non-PPC > system. But this isn't a new consideration; we were already checking for > 'used-by-rtas' on *all* platforms. Perhaps we shouldn't be. But that's > almost orthogonal to the issue at hand. We have been checking for all DT platforms, and that's a bug for DT. Copying that bug to ACPI is inexcusable given we know it's a bug to do so. No-one is using ACPI+RTAS, so there is no reason to enable this property for ACPI. The only reason anyone would (mis)use this property with ACPI is because Linux happened to support that combination, which need not be the case because it doesn't make any sense. > > As a general note, I would hope that we're not going to blindly convert > > drivers and subsystems over to a common property interface without > > considering each property w.r.t. the particular FW interface. > > > > Each addition to _DSD, especially if through a common accessor needs > > _more_ scrutiny than is applied to DT bindings, and we hardly manage to > > review DT bindings. > > The whole point here is to use existing bindings rather than having to > reinvent the wheel. Sure, where the existing binding really makes no > sense for certain subsystems, we should come up with something > different. I understand that. However, where a binding doesn't make sense (as in this case), it shouldn't be enabled for ACPI as it provides a larger surface area for misuse, for no benefit. > But in the general case for 'leaf-node' peripherals we would hope that > we don't really have to change *anything* other than to make sure the > driver is using generic property accessor functions instead of the old > OF-specific ones. The point here is *consistency*. We really don't want > to make a habit of reinventing different bindings to be exposed through > the different firmware types. Where we have a property of a leaf-node that is independent of the description format, sure. I am not asking us to re-invent bindings, but we must consider the differences between ACPI and DT systems when trying to port a DT binding to ACPI. My concern here is that we don't have the mechanisms in place for sufficient scrutiny to be applied when bindings are ported over. We _will_ end up porting over things that don't make sense (as this patch shows), and some of those are likely to cause unnecessary pain for everyone. It does not make sense to say that is OK. Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On Wed, 2014-10-15 at 14:15 +0100, Mark Rutland wrote: > > @@ -155,7 +168,7 @@ static int of_platform_serial_probe(struct > > platform_device *ofdev) > > if (!match) > > return -EINVAL; > > > > - if (of_find_property(ofdev->dev.of_node, "used-by-rtas", NULL)) > > + if (!device_get_property(&ofdev->dev, "used-by-rtas", NULL)) > > return -EBUSY; > > This property should never be present on an ACPI system. RTAS is a > completely different firmware interface on PowerPC. Yes, I sincerely hope we never see used-by-rtas being set on a non-PPC system. But this isn't a new consideration; we were already checking for 'used-by-rtas' on *all* platforms. Perhaps we shouldn't be. But that's almost orthogonal to the issue at hand. > As a general note, I would hope that we're not going to blindly convert > drivers and subsystems over to a common property interface without > considering each property w.r.t. the particular FW interface. > > Each addition to _DSD, especially if through a common accessor needs > _more_ scrutiny than is applied to DT bindings, and we hardly manage to > review DT bindings. The whole point here is to use existing bindings rather than having to reinvent the wheel. Sure, where the existing binding really makes no sense for certain subsystems, we should come up with something different. But in the general case for 'leaf-node' peripherals we would hope that we don't really have to change *anything* other than to make sure the driver is using generic property accessor functions instead of the old OF-specific ones. The point here is *consistency*. We really don't want to make a habit of reinventing different bindings to be exposed through the different firmware types. -- David WoodhouseOpen Source Technology Centre david.woodho...@intel.com Intel Corporation smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On Wed, Oct 15, 2014 at 02:04:31PM +0100, David Woodhouse wrote: > Here's a completely untested patch to convert of_serial to be usable via > ACPI properties too. The properties themselves were fairly > straightforward; the interesting part is converting to > platform_get_irq() and platform_get_resource() — in the latter case > first trying IORESOURCE_MEM then IORESOURCE_IO if that fails. > > Does this look sane? We'll probably want to think about renaming the > module and the config option too, but that can come after the basic > functionality. The majority of these properties look like they constrained to the device in question, so make sense for _DSD too. However... > @@ -155,7 +168,7 @@ static int of_platform_serial_probe(struct > platform_device *ofdev) > if (!match) > return -EINVAL; > > - if (of_find_property(ofdev->dev.of_node, "used-by-rtas", NULL)) > + if (!device_get_property(&ofdev->dev, "used-by-rtas", NULL)) > return -EBUSY; This property should never be present on an ACPI system. RTAS is a completely different firmware interface on PowerPC. As a general note, I would hope that we're not going to blindly convert drivers and subsystems over to a common property interface without considering each property w.r.t. the particular FW interface. Each addition to _DSD, especially if through a common accessor needs _more_ scrutiny than is applied to DT bindings, and we hardly manage to review DT bindings. Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
Here's a completely untested patch to convert of_serial to be usable via
ACPI properties too. The properties themselves were fairly
straightforward; the interesting part is converting to
platform_get_irq() and platform_get_resource() — in the latter case
first trying IORESOURCE_MEM then IORESOURCE_IO if that fails.
Does this look sane? We'll probably want to think about renaming the
module and the config option too, but that can come after the basic
functionality.
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 26cec64..be95a4c 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1094,14 +1094,14 @@ config SERIAL_NETX_CONSOLE
you can make it the console by answering Y to this option.
config SERIAL_OF_PLATFORM
- tristate "Serial port on Open Firmware platform bus"
- depends on OF
+ tristate "Serial port on firmware platform bus"
+ depends on OF || ACPI
depends on SERIAL_8250 || SERIAL_OF_PLATFORM_NWPSERIAL
help
- If you have a PowerPC based system that has serial ports
- on a platform specific bus, you should enable this option.
- Currently, only 8250 compatible ports are supported, but
- others can easily be added.
+ If you have a system which advertises its serial ports through
+ devicetree or ACPI, you should enable this option. Currently
+ only 8250 compatible and NWP ports and are supported, but others
+ can easily be added.
config SERIAL_OMAP
tristate "OMAP serial port support"
diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
index 68d4455..73ee9af 100644
--- a/drivers/tty/serial/of_serial.c
+++ b/drivers/tty/serial/of_serial.c
@@ -57,13 +57,14 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
int type, struct uart_port *port,
struct of_serial_info *info)
{
- struct resource resource;
- struct device_node *np = ofdev->dev.of_node;
+ struct resource *resource;
u32 clk, spd, prop;
+ unsigned char iotype = UPIO_MEM;
+ u32 res_start;
int ret;
memset(port, 0, sizeof *port);
- if (of_property_read_u32(np, "clock-frequency", &clk)) {
+ if (device_property_read_u32(&ofdev->dev, "clock-frequency", &clk)) {
/* Get clk rate through clk driver if present */
info->clk = clk_get(&ofdev->dev, NULL);
@@ -77,40 +78,52 @@ static int of_platform_serial_setup(struct platform_device
*ofdev,
clk = clk_get_rate(info->clk);
}
/* If current-speed was set, then try not to change it. */
- if (of_property_read_u32(np, "current-speed", &spd) == 0)
+ if (device_property_read_u32(&ofdev->dev, "current-speed", &spd) == 0)
port->custom_divisor = clk / (16 * spd);
- ret = of_address_to_resource(np, 0, &resource);
- if (ret) {
+ resource = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
+ if (!resource) {
+ resource = platform_get_resource(ofdev, IORESOURCE_IO, 0);
+ iotype = UPIO_PORT;
+ }
+ if (!resource) {
dev_warn(&ofdev->dev, "invalid address\n");
goto out;
}
spin_lock_init(&port->lock);
- port->mapbase = resource.start;
+ res_start = resource->start;
/* Check for shifted address mapping */
- if (of_property_read_u32(np, "reg-offset", &prop) == 0)
- port->mapbase += prop;
+ if (device_property_read_u32(&ofdev->dev, "reg-offset", &prop) == 0)
+ res_start += prop;
+
+ if (iotype == UPIO_PORT)
+ port->iobase = res_start;
+ else
+ port->mapbase = res_start;
/* Check for registers offset within the devices address range */
- if (of_property_read_u32(np, "reg-shift", &prop) == 0)
+ if (device_property_read_u32(&ofdev->dev, "reg-shift", &prop) == 0)
port->regshift = prop;
/* Check for fifo size */
- if (of_property_read_u32(np, "fifo-size", &prop) == 0)
+ if (device_property_read_u32(&ofdev->dev, "fifo-size", &prop) == 0)
port->fifosize = prop;
- port->irq = irq_of_parse_and_map(np, 0);
- port->iotype = UPIO_MEM;
- if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
+ port->irq = platform_get_irq(ofdev, 0);
+ port->iotype = iotype;
+ if (device_property_read_u32(&ofdev->dev, "reg-io-width", &prop) == 0) {
switch (prop) {
case 1:
- port->iotype = UPIO_MEM;
+ port->iotype = iotype;
break;
case 4:
- port->iotype = UPIO_MEM32;
- break;
+ if (iotype == UPIO_MEM) {
+
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On Tue, Oct 07, 2014 at 02:10:56AM +0200, Rafael J. Wysocki wrote: > Hi Everyone, > > This is version 4 of the unified device properties interface patchset. > > The original cover letter from Mika is here: > > http://marc.info/?l=devicetree&m=141087052200600&w=4 > > My cover letter for version 3 is here: > > http://marc.info/?l=linux-acpi&m=141212903816560&w=4 > > One major change from the previous iteration is that now we will > use the "compatible" property to match drivers to devices having > "PRP0001" as their _HID, so for example the at25 driver doesn't > have to add the extra ACPI match table as part of the conversion > to the unified interface (patch [05/13] in this series). > > The second major change is that I've split the driver core patch > in two, where the first one ([02/13]) does not contain any stuff > related to iterating over the child nodes of a given device. > Accordingly, the whole patch series has been rearranged so that > the relatively non-controversial patches [01-09/13], most of which > have been ACKed already, go first and then goes the second driver > core patch ([10/13]) and the other patches related to it. > > In patches [10-13/13] I used the Arnd's suggestion to implement > device_for_each_child_node() as a macro which makes the changes > in patches [12-13/13] look more straightforward among other things. > > I've retained the Greg's ACKs on patches [02/13] and [10/13], because > the first of them is things ACKed by Greg only and the change in the > second one is just an implementation detail in my opinion (Greg, please > let me know if that's inappropriate). No objection from me, looks good. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 00/13] Add ACPI _DSD and unified device properties support
On Tuesday, October 07, 2014 02:10:56 AM Rafael J. Wysocki wrote: > Hi Everyone, > > This is version 4 of the unified device properties interface patchset. > > The original cover letter from Mika is here: > > http://marc.info/?l=devicetree&m=141087052200600&w=4 > > My cover letter for version 3 is here: > > http://marc.info/?l=linux-acpi&m=141212903816560&w=4 > > One major change from the previous iteration is that now we will > use the "compatible" property to match drivers to devices having > "PRP0001" as their _HID, so for example the at25 driver doesn't > have to add the extra ACPI match table as part of the conversion > to the unified interface (patch [05/13] in this series). > > The second major change is that I've split the driver core patch > in two, where the first one ([02/13]) does not contain any stuff > related to iterating over the child nodes of a given device. > Accordingly, the whole patch series has been rearranged so that > the relatively non-controversial patches [01-09/13], most of which > have been ACKed already, go first and then goes the second driver > core patch ([10/13]) and the other patches related to it. > > In patches [10-13/13] I used the Arnd's suggestion to implement > device_for_each_child_node() as a macro which makes the changes > in patches [12-13/13] look more straightforward among other things. > > I've retained the Greg's ACKs on patches [02/13] and [10/13], because > the first of them is things ACKed by Greg only and the change in the > second one is just an implementation detail in my opinion (Greg, please > let me know if that's inappropriate). I should have mentioned that the whole series is available in the device-properties branch of the linux-pm.git tree at kernel.org. -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
