| Message ID | 1413378271.2762.77.camel@infradead.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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.
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?
> 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.
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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. >
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.
On Wed, 15 Oct 2014 17:43:01 +0200 , Darren Hart <dvhart@linux.intel.com> 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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Oct 18, 2014 at 10:35:49AM +0200, Grant Likely wrote: > On Wed, 15 Oct 2014 17:43:01 +0200 > , Darren Hart <dvhart@linux.intel.com> > 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 :-)
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) { + port->iotype = UPIO_MEM32; + break; + } + /* Fall through for non-memory */ default: dev_warn(&ofdev->dev, "unsupported reg-io-width (%d)\n", prop); @@ -124,7 +137,7 @@ static int of_platform_serial_setup(struct platform_device *ofdev, port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE; - if (of_find_property(np, "no-loopback-test", NULL)) + if (!device_get_property(&ofdev->dev, "no-loopback-test", NULL)) port->flags |= UPF_SKIP_TEST; port->dev = &ofdev->dev; @@ -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; info = kmalloc(sizeof(*info), GFP_KERNEL); @@ -179,12 +192,10 @@ static int of_platform_serial_probe(struct platform_device *ofdev) if (port.fifosize) port8250.capabilities = UART_CAP_FIFO; - if (of_property_read_bool(ofdev->dev.of_node, - "auto-flow-control")) + if (!device_get_property(&ofdev->dev, "auto-flow-control", NULL)) port8250.capabilities |= UART_CAP_AFE; - if (of_property_read_bool(ofdev->dev.of_node, - "has-hw-flow-control")) + if (!device_get_property(&ofdev->dev, "has-hw-flow-control", NULL)) port8250.port.flags |= UPF_HARD_FLOW; ret = serial8250_register_8250_port(&port8250);