[PATCH] serial/efm32: parse location property

[PATCH] serial/efm32: parse location property

[Uwe Kleine-König]

The non-dt probing allowed passing the location via platform data from
the beginning. So make up leeway for device tree probing.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

I wonder if the location property should be mandatory. Do you have an
opinion? 

BTW, I was surprised that I had to use of_property_read_u32 but not
of_property_read_u8. I would have expected that the only difference
between those two is the returned type, but actually the format to
specify an u8 is different. Is this intended? (Probably it is. If so, is
it maybe sensible to accentuate the difference by using a different
function name for the u32 case?)

Best regards
Uwe

 .../devicetree/bindings/tty/serial/efm32-uart.txt  |    6 ++++
 drivers/tty/serial/efm32-uart.c                    |   31 +++++++++++++++-----
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/tty/serial/efm32-uart.txt b/Documentation/devicetree/bindings/tty/serial/efm32-uart.txt
index 6588b69..8e080b8 100644
--- a/Documentation/devicetree/bindings/tty/serial/efm32-uart.txt
+++ b/Documentation/devicetree/bindings/tty/serial/efm32-uart.txt
@@ -5,10 +5,16 @@ Required properties:
 - reg : Address and length of the register set
 - interrupts : Should contain uart interrupt
 
+Optional properties:
+- location : Decides the location of the USART I/O pins.
+  Allowed range : [0 .. 5]
+  Default: 0
+
 Example:
 
 uart@0x4000c400 {
 	compatible = "efm32,uart";
 	reg = <0x4000c400 0x400>;
 	interrupts = <15>;
+	location = <0>;
 };
diff --git a/drivers/tty/serial/efm32-uart.c b/drivers/tty/serial/efm32-uart.c
index a8cbb26..67ff23b 100644
--- a/drivers/tty/serial/efm32-uart.c
+++ b/drivers/tty/serial/efm32-uart.c
@@ -81,6 +81,7 @@ struct efm32_uart_port {
 	struct uart_port port;
 	unsigned int txirq;
 	struct clk *clk;
+	struct efm32_uart_pdata pdata;
 };
 #define to_efm_port(_port) container_of(_port, struct efm32_uart_port, port)
 #define efm_debug(efm_port, format, arg...)			\
@@ -300,13 +301,8 @@ static irqreturn_t efm32_uart_txirq(int irq, void *data)
 static int efm32_uart_startup(struct uart_port *port)
 {
 	struct efm32_uart_port *efm_port = to_efm_port(port);
-	u32 location = 0;
-	struct efm32_uart_pdata *pdata = dev_get_platdata(port->dev);
 	int ret;
 
-	if (pdata)
-		location = UARTn_ROUTE_LOCATION(pdata->location);
-
 	ret = clk_enable(efm_port->clk);
 	if (ret) {
 		efm_debug(efm_port, "failed to enable clk\n");
@@ -315,7 +311,9 @@ static int efm32_uart_startup(struct uart_port *port)
 	port->uartclk = clk_get_rate(efm_port->clk);
 
 	/* Enable pins at configured location */
-	efm32_uart_write32(efm_port, location | UARTn_ROUTE_RXPEN | UARTn_ROUTE_TXPEN,
+	efm32_uart_write32(efm_port,
+			UARTn_ROUTE_LOCATION(efm_port->pdata.location) |
+			UARTn_ROUTE_RXPEN | UARTn_ROUTE_TXPEN,
 			UARTn_ROUTE);
 
 	ret = request_irq(port->irq, efm32_uart_rxirq, 0,
@@ -674,11 +672,24 @@ static int efm32_uart_probe_dt(struct platform_device *pdev,
 		struct efm32_uart_port *efm_port)
 {
 	struct device_node *np = pdev->dev.of_node;
+	u32 location;
 	int ret;
 
 	if (!np)
 		return 1;
 
+	ret = of_property_read_u32(np, "location", &location);
+	if (!ret) {
+		if (location > 5) {
+			dev_err(&pdev->dev, "invalid location\n");
+			return -EINVAL;
+		}
+		efm_debug(efm_port, "using location %u\n", location);
+		efm_port->pdata.location = location;
+	} else {
+		efm_debug(efm_port, "fall back to location 0\n");
+	}
+
 	ret = of_alias_get_id(np, "serial");
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to get alias id: %d\n", ret);
@@ -738,10 +749,16 @@ static int efm32_uart_probe(struct platform_device *pdev)
 	efm_port->port.flags = UPF_BOOT_AUTOCONF;
 
 	ret = efm32_uart_probe_dt(pdev, efm_port);
-	if (ret > 0)
+	if (ret > 0) {
 		/* not created by device tree */
+		const struct efm32_uart_pdata *pdata = dev_get_platdata(&pdev->dev);
+
 		efm_port->port.line = pdev->id;
 
+		if (pdata)
+			efm_port->pdata = *pdata;
+	}
+
 	if (efm_port->port.line >= 0 &&
 			efm_port->port.line < ARRAY_SIZE(efm32_uart_ports))
 		efm32_uart_ports[efm_port->port.line] = efm_port;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] serial/efm32: parse location property

[Grant Likely]

[-- Attachment #1: Type: text/plain, Size: 4899 bytes --]

On Mon, 21 Jan 2013 14:22:56 +0100, Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> The non-dt probing allowed passing the location via platform data from
> the beginning. So make up leeway for device tree probing.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Linus, does this look like some thing that should be done using pinctrl?

g.

> ---
> Hello,
> 
> I wonder if the location property should be mandatory. Do you have an
> opinion? 
> 
> BTW, I was surprised that I had to use of_property_read_u32 but not
> of_property_read_u8. I would have expected that the only difference
> between those two is the returned type, but actually the format to
> specify an u8 is different. Is this intended? (Probably it is. If so, is
> it maybe sensible to accentuate the difference by using a different
> function name for the u32 case?)
> 
> Best regards
> Uwe
> 
>  .../devicetree/bindings/tty/serial/efm32-uart.txt  |    6 ++++
>  drivers/tty/serial/efm32-uart.c                    |   31 +++++++++++++++-----
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/tty/serial/efm32-uart.txt b/Documentation/devicetree/bindings/tty/serial/efm32-uart.txt
> index 6588b69..8e080b8 100644
> --- a/Documentation/devicetree/bindings/tty/serial/efm32-uart.txt
> +++ b/Documentation/devicetree/bindings/tty/serial/efm32-uart.txt
> @@ -5,10 +5,16 @@ Required properties:
>  - reg : Address and length of the register set
>  - interrupts : Should contain uart interrupt
>  
> +Optional properties:
> +- location : Decides the location of the USART I/O pins.
> +  Allowed range : [0 .. 5]
> +  Default: 0
> +
>  Example:
>  
>  uart@0x4000c400 {
>  	compatible = "efm32,uart";
>  	reg = <0x4000c400 0x400>;
>  	interrupts = <15>;
> +	location = <0>;
>  };
> diff --git a/drivers/tty/serial/efm32-uart.c b/drivers/tty/serial/efm32-uart.c
> index a8cbb26..67ff23b 100644
> --- a/drivers/tty/serial/efm32-uart.c
> +++ b/drivers/tty/serial/efm32-uart.c
> @@ -81,6 +81,7 @@ struct efm32_uart_port {
>  	struct uart_port port;
>  	unsigned int txirq;
>  	struct clk *clk;
> +	struct efm32_uart_pdata pdata;
>  };
>  #define to_efm_port(_port) container_of(_port, struct efm32_uart_port, port)
>  #define efm_debug(efm_port, format, arg...)			\
> @@ -300,13 +301,8 @@ static irqreturn_t efm32_uart_txirq(int irq, void *data)
>  static int efm32_uart_startup(struct uart_port *port)
>  {
>  	struct efm32_uart_port *efm_port = to_efm_port(port);
> -	u32 location = 0;
> -	struct efm32_uart_pdata *pdata = dev_get_platdata(port->dev);
>  	int ret;
>  
> -	if (pdata)
> -		location = UARTn_ROUTE_LOCATION(pdata->location);
> -
>  	ret = clk_enable(efm_port->clk);
>  	if (ret) {
>  		efm_debug(efm_port, "failed to enable clk\n");
> @@ -315,7 +311,9 @@ static int efm32_uart_startup(struct uart_port *port)
>  	port->uartclk = clk_get_rate(efm_port->clk);
>  
>  	/* Enable pins at configured location */
> -	efm32_uart_write32(efm_port, location | UARTn_ROUTE_RXPEN | UARTn_ROUTE_TXPEN,
> +	efm32_uart_write32(efm_port,
> +			UARTn_ROUTE_LOCATION(efm_port->pdata.location) |
> +			UARTn_ROUTE_RXPEN | UARTn_ROUTE_TXPEN,
>  			UARTn_ROUTE);
>  
>  	ret = request_irq(port->irq, efm32_uart_rxirq, 0,
> @@ -674,11 +672,24 @@ static int efm32_uart_probe_dt(struct platform_device *pdev,
>  		struct efm32_uart_port *efm_port)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> +	u32 location;
>  	int ret;
>  
>  	if (!np)
>  		return 1;
>  
> +	ret = of_property_read_u32(np, "location", &location);
> +	if (!ret) {
> +		if (location > 5) {
> +			dev_err(&pdev->dev, "invalid location\n");
> +			return -EINVAL;
> +		}
> +		efm_debug(efm_port, "using location %u\n", location);
> +		efm_port->pdata.location = location;
> +	} else {
> +		efm_debug(efm_port, "fall back to location 0\n");
> +	}
> +
>  	ret = of_alias_get_id(np, "serial");
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to get alias id: %d\n", ret);
> @@ -738,10 +749,16 @@ static int efm32_uart_probe(struct platform_device *pdev)
>  	efm_port->port.flags = UPF_BOOT_AUTOCONF;
>  
>  	ret = efm32_uart_probe_dt(pdev, efm_port);
> -	if (ret > 0)
> +	if (ret > 0) {
>  		/* not created by device tree */
> +		const struct efm32_uart_pdata *pdata = dev_get_platdata(&pdev->dev);
> +
>  		efm_port->port.line = pdev->id;
>  
> +		if (pdata)
> +			efm_port->pdata = *pdata;
> +	}
> +
>  	if (efm_port->port.line >= 0 &&
>  			efm_port->port.line < ARRAY_SIZE(efm32_uart_ports))
>  		efm32_uart_ports[efm_port->port.line] = efm_port;
> -- 
> 1.7.10.4
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH] serial/efm32: parse location property

[Linus Walleij]

On Mon, Mar 4, 2013 at 3:38 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, 21 Jan 2013 14:22:56 +0100, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
>> The non-dt probing allowed passing the location via platform data from
>> the beginning. So make up leeway for device tree probing.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> Linus, does this look like some thing that should be done using pinctrl?

Hm it's a bit weird actually.

Most platforms will have some cetral entity dealing with pin multiplexing,
and often it is combined with either a system controller register range,
or it's combined with a GPIO controller (which is more common).

This is a new oddity: it's like multiplexing is distributed out among the
peripherals on the EFM32, or atleast for this UART, so there is a
register on the peripheral itself selecting where to route it.

If all peripherals on the system follows this pattern, like if there is
some such "location" register on each and every peripheral, ideally
that should be centralized into some file like drivers/pinctrl/pinctrl-efm32.c
but the thing is that then that driver needs to own a single register
or even just part of a register in each peripheral device memory
range.

And that seems a bit complex to handle.

But if the EFM32 is using device tree exclusively it's actually
just an array of named  <&ampersand> references in the node for
the pin controller, pointing to each peripheral with a mux register.
So maybe that's not that bad after all.

Still it might be a bit overzealous to request that each device
ask a central entity to write one of its own registers. So it needs
to buy you something - like for example if it is possible to
completely screw up the muxing if different peripherals are
muxed to the same pins.

So it all depends, we need a birds-eye view of the system to
determine this. I tried looking at the datasheet but couldn't figure
it out. Uwe how does this work on EFM32?

Yours,
Linus Walleij

Re: [PATCH] serial/efm32: parse location property

[Uwe Kleine-König]

Hi Linus,

On Thu, Mar 07, 2013 at 08:37:38AM +0100, Linus Walleij wrote:
> On Mon, Mar 4, 2013 at 3:38 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Mon, 21 Jan 2013 14:22:56 +0100, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> >> The non-dt probing allowed passing the location via platform data from
> >> the beginning. So make up leeway for device tree probing.
> >>
> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >
> > Linus, does this look like some thing that should be done using pinctrl?
> 
> Hm it's a bit weird actually.
That was my first impression, too :-)

> Most platforms will have some cetral entity dealing with pin multiplexing,
> and often it is combined with either a system controller register range,
> or it's combined with a GPIO controller (which is more common).
> 
> This is a new oddity: it's like multiplexing is distributed out among the
> peripherals on the EFM32, or atleast for this UART, so there is a
> register on the peripheral itself selecting where to route it.
> 
> If all peripherals on the system follows this pattern, like if there is
> some such "location" register on each and every peripheral, ideally
I didn't check if all do, but it's a common pattern for the efm32
peripherals to have such a ROUTE register.

> that should be centralized into some file like drivers/pinctrl/pinctrl-efm32.c
> but the thing is that then that driver needs to own a single register
> or even just part of a register in each peripheral device memory
> range.
> 
> And that seems a bit complex to handle.
> 
> But if the EFM32 is using device tree exclusively it's actually
> just an array of named  <&ampersand> references in the node for
> the pin controller, pointing to each peripheral with a mux register.
> So maybe that's not that bad after all.
> 
> Still it might be a bit overzealous to request that each device
> ask a central entity to write one of its own registers. So it needs
> to buy you something - like for example if it is possible to
> completely screw up the muxing if different peripherals are
> muxed to the same pins.
I don't know what happens then.
 
> So it all depends, we need a birds-eye view of the system to
> determine this. I tried looking at the datasheet but couldn't figure
> it out. Uwe how does this work on EFM32?
OK, here comes the rough overview:

Several peripherals have a ROUTE register. They have a location field
that determines a group of pins and an enable bit for each role. E.g.
The Clock Management Unit's ROUTE register has supports three locations
and two functions (CLKOUT1 and CLKOUT2). Depending on which location is
used CLKOUT[12] is routed to the following pins:

          Location | CLKOUT0 | CLKOUT1
              0    |   PA1   |   PA1
              1    |    -    |   PD8
              2    |   PD7   |  PE12

The GPIO controller is involved, too, because that one needs to
configure the pins direction. So if you want to get out some clock to
the PA1 pin, you need to do the following in the CMU's register set:
	- CMU_ROUTE.LOCATION = 0;
	- CMU_ROUTE.CLKOUT0PEN = 1;
	- (configure which clock to output in CMU_CTRL)

and additionally in the GPIO peripheral:

	- configure PA1 as output (this includes selecting PUSHPULL or
	  WIREDOR with a pull down or WIREDAND with a filter applied or
	  one of several more alternatives)

and optionally:

	- configure drive strength (applies to all A pins)

For the USART (i.e. UART + SPI peripheral) there are 6 different
locations and 4 roles:

	CLK    \
	CS     - only used for spi mode
	TX
	RX

and analogous you have to use one of the 3 input modes for the RX pin
and an output mode for the others in the GPIO register space.

Currently I do the settings in the GPIO register space in the bootloader
and don't let Linux modify them. So the gpio driver's direction_output
callback looks like:

	direction_output(pin, initval):
		if pin isn't already configured as output:
			return -ESOMETHING
		else:
			set value of pin = initval
			return 0

To complicate the matter, the layout of the ROUTE register isn't
uniform. So for CMU the location filed occupies bits 2:4, for USART it's
bits 8:10. Also the offsets in the peripheral's register space differ
from peripheral to peripheral.

If you have an idea how to model this such that it fits nicely into the
various subsystems (most interesting: gpio and pinmux), any input is
welcome. (Implementation has to wait probably for a while, because we
currently don't have a business case for it and I have to look for the
Cortex-M3 basics first.) In case you are interested in more details, the
Reference Manual and the Data Sheet are available at

	http://www.energymicro.com/products/efm32gg980f64-efm32gg980f128-efm32gg980f256-efm32gg980f512-efm32gg980f1024

(or ask).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] serial/efm32: parse location property

[Linus Walleij]

On Thu, Mar 7, 2013 at 10:38 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

> If you have an idea how to model this such that it fits nicely into the
> various subsystems (most interesting: gpio and pinmux), any input is
> welcome.

Looking at it, and assuming you will use device tree for this, I think
the best is to have a single driver in drivers/pinctrl/pinctrl-efm32.c
that handles all muxing, pin direction/biasing/pulling and GPIO.

Let this driver's DT entry reference all the devices with local
MUX registers and pick out these nodes by reference and
manipulate them from the pin control driver.

You put ampersand nodes into the pin controller node.

In the dbx500 pin controller we have this:

                pinctrl {
                        compatible = "stericsson,nmk-pinctrl";
                        prcm = <&prcmu>;
                };

Basically this "prcm" is another peripheral with a set of misc
registers that also do some pin control, but we let the pin control
driver grab a handle to that because they are related.

Yours,
Linus Walleij

RE: [Customers.Bosch-Gecko] [PATCH] serial/efm32: parse location property

[Øyvind Grotmol]

> -----Original Message-----
> From: customers.bosch-gecko-bounces@pengutronix.de
> [mailto:customers.bosch-gecko-bounces@pengutronix.de] On Behalf Of Uwe
> Kleine-König
> Sent: 7. mars 2013 10:38
>
> > This is a new oddity: it's like multiplexing is distributed out among
> > the peripherals on the EFM32, or atleast for this UART, so there is a
> > register on the peripheral itself selecting where to route it.
> >
> > If all peripherals on the system follows this pattern, like if there
> > is some such "location" register on each and every peripheral,
> ideally
> I didn't check if all do, but it's a common pattern for the efm32
> peripherals to have such a ROUTE register.

Yes. In general, our analog modules (such as ADC and DAC) have a fixed pin
location which cannot be changed. All of our digital modules (UART etc)
have a ROUTE register inside each module. That is the general pattern.

Best regards,
Øyvind

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html