Re: [PATCH 07/17] dma: altera-msgdma: Fix struct documentation blocks

[Stefan Roese]

On 14.07.20 13:15, Lee Jones wrote:

Fix some misspelling/description issues, demote non-kerneldoc header
to standard comment block and provide a new description for
msgdma_desc_config()'s 'stride' parameter.

Fixes the following W=1 kernel build warning(s):

  drivers/dma/altera-msgdma.c:163: warning: Function parameter or member 'node' 
not described in 'msgdma_sw_desc'
  drivers/dma/altera-msgdma.c:163: warning: Function parameter or member 
'tx_list' not described in 'msgdma_sw_desc'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 'lock' 
not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 'dev' 
not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 
'irq_tasklet' not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 
'pending_list' not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 
'free_list' not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 
'active_list' not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 
'done_list' not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 
'desc_free_cnt' not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 'idle' 
not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 
'dmadev' not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 
'dmachan' not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 
'hw_desq' not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 
'sw_desq' not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 
'npendings' not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 
'slave_cfg' not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 'irq' 
not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 'csr' 
not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 'desc' 
not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:197: warning: Function parameter or member 'resp' 
not described in 'msgdma_device'
  drivers/dma/altera-msgdma.c:265: warning: Function parameter or member 
'stride' not described in 'msgdma_desc_config'

Cc: Stefan Roese 
Signed-off-by: Lee Jones 


Reviewed-by: Stefan Roese 

Thanks,
Stefan


---
  drivers/dma/altera-msgdma.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
index 539e785039cac..321ac3a7aa418 100644
--- a/drivers/dma/altera-msgdma.c
+++ b/drivers/dma/altera-msgdma.c
@@ -153,7 +153,8 @@ struct msgdma_extended_desc {
   * struct msgdma_sw_desc - implements a sw descriptor
   * @async_tx: support for the async_tx api
   * @hw_desc: assosiated HW descriptor
- * @free_list: node of the free SW descriprots list
+ * @node: node to move from the free list to the tx list
+ * @tx_list: transmit list node
   */
  struct msgdma_sw_desc {
struct dma_async_tx_descriptor async_tx;
@@ -162,7 +163,7 @@ struct msgdma_sw_desc {
struct list_head tx_list;
  };
  
-/**

+/*
   * struct msgdma_device - DMA device structure
   */
  struct msgdma_device {
@@ -258,6 +259,7 @@ static void msgdma_free_desc_list(struct msgdma_device 
*mdev,
   * @dst: Destination buffer address
   * @src: Source buffer address
   * @len: Transfer length
+ * @stride: Read/write stride value to set
   */
  static void msgdma_desc_config(struct msgdma_extended_desc *desc,
   dma_addr_t dst, dma_addr_t src, size_t len,




Viele Grüße,
Stefan

--
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: s...@denx.de

Re: [PATCH v2 -next] net: mediatek: remove set but not used variable 'status'

[Stefan Roese]

Hi!

On 26.08.19 09:10, René van Dorst wrote:

Let's add Stefan to the conversation.
He is the author of this commit.


Thanks Rene.
 

Quoting Mao Wenan :


Fixes gcc '-Wunused-but-set-variable' warning:
drivers/net/ethernet/mediatek/mtk_eth_soc.c: In function mtk_handle_irq:
drivers/net/ethernet/mediatek/mtk_eth_soc.c:1951:6: warning:
variable status set but not used [-Wunused-but-set-variable]

Fixes: 296c9120752b ("net: ethernet: mediatek: Add MT7628/88 SoC support")
Signed-off-by: Mao Wenan 
---
  v2: change format of 'Fixes' tag.
  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 8ddbb8d..bb7d623 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1948,9 +1948,7 @@ static irqreturn_t mtk_handle_irq_tx(int irq,
void *_eth)
  static irqreturn_t mtk_handle_irq(int irq, void *_eth)
  {
struct mtk_eth *eth = _eth;
-   u32 status;

-   status = mtk_r32(eth, MTK_PDMA_INT_STATUS);


Hi Stefan,

You added an extra MTK_PDMA_INT_STATUS read in mtk_handle_irq()
Is that read necessary to work properly?


No, its not needed. This read must have "slipped in" from some earlier
patch versions and I forgot to remove it later. Thanks for catching it.

Reviewed-by: Stefan Roese 

Thanks,
Stefan

Re: [PATCH 1/2 v9] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

Hi Geert,

On 24.06.19 17:35, Geert Uytterhoeven wrote:

On Mon, Jun 24, 2019 at 5:29 PM Stefan Roese  wrote:

On 24.06.19 10:42, Geert Uytterhoeven wrote:

On Thu, Jun 20, 2019 at 8:24 AM Stefan Roese  wrote:

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

  Device (URT4)
  {
  ...
  Name (_CRS, ResourceTemplate () {
  GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
  "\\_SB.GPO0", 0x00, ResourceConsumer)
  {
  0x003A
  }
  GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
  "\\_SB.GPO0", 0x00, ResourceConsumer)
  {
  0x003D
  }
  })

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.

Signed-off-by: Stefan Roese 



--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c



@@ -116,6 +117,19 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device 
*dev, unsigned int idx)

  for (i = 0; i < UART_GPIO_MAX; i++) {
  enum gpiod_flags flags;
+   char *gpio_str;
+   bool present;
+
+   /* Check if GPIO property exists and continue if not */
+   gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
+mctrl_gpios_desc[i].name);


This will silently break DTBs using "(cts|dsr|dcd|rng|rts|dtr)-gpio" instead
of "(cts|dsr|dcd|rng|rts|dtr)-gpios".


Should both options be supported ("cts-gpio" vs "cts-gpios")?
Documentation/devicetree/bindings/serial/serial.txt only mentions
the "-gpios" variant.


Well, the "-gpio" variant is deprecated, but still supported by
devm_gpiod_get_index_optional(), and there are active users in upstream
DTS files.

My main objection is (trying to) replicate the matching logic inside
gpiolib.c, causing subtle semantic differences. And keeping it consistent,
of course.

It would be nice if this could be fixed inside acpi_find_gpio(), so
users don't need to be updated.  There may be other subsystems where
the difference between DT and ACPI may cause issues, unbeknownst.


Sure, I can fix this. I would prefer to do this in a follow-up patch
though, if nobody objects.

Thanks,
Stefan

Re: [PATCH 1/2 v9] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

On 24.06.19 10:42, Geert Uytterhoeven wrote:

CC gpio

This is now commit d99482673f950817 ("serial: mctrl_gpio: Check if GPIO
property exisits before requesting it") in tty-next.

On Thu, Jun 20, 2019 at 8:24 AM Stefan Roese  wrote:

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

 Device (URT4)
 {
 ...
 Name (_CRS, ResourceTemplate () {
 GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
 "\\_SB.GPO0", 0x00, ResourceConsumer)
 {
 0x003A
 }
 GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
 "\\_SB.GPO0", 0x00, ResourceConsumer)
 {
 0x003D
 }
 })

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.

Signed-off-by: Stefan Roese 
Reviewed-by: Mika Westerberg 
Reviewed-by: Andy Shevchenko 
Tested-by: Yegor Yefremov 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
v9:
- Rebased on top of "tty-next", patch 2/3 dropped as its already applied

v8:
- Rebased on top of "tty-next"

v7:
- Include  to fix compile breakage on OMAP

v6:
- No change

v5:
- Simplified the code a bit (Andy)
- Added gpio_str == NULL handling (Andy)

v4:
- Add missing free() calls (Johan)
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- No change

v2:
- Include the problem description and analysis from Mika into the commit
   text, as suggested by Greg.

  drivers/tty/serial/serial_mctrl_gpio.c | 14 ++
  1 file changed, 14 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 39ed56214cd3..2b400189be91 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -12,6 +12,7 @@
  #include 
  #include 
  #include 
+#include 

  #include "serial_mctrl_gpio.h"

@@ -116,6 +117,19 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device 
*dev, unsigned int idx)

 for (i = 0; i < UART_GPIO_MAX; i++) {
 enum gpiod_flags flags;
+   char *gpio_str;
+   bool present;
+
+   /* Check if GPIO property exists and continue if not */
+   gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
+mctrl_gpios_desc[i].name);


This will silently break DTBs using "(cts|dsr|dcd|rng|rts|dtr)-gpio" instead
of "(cts|dsr|dcd|rng|rts|dtr)-gpios".


Should both options be supported ("cts-gpio" vs "cts-gpios")?
Documentation/devicetree/bindings/serial/serial.txt only mentions
the "-gpios" variant.

Thanks,
Stefan

[PATCH 2/2 v9] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

From: Yegor Yefremov 

This patch permits the usage for GPIOs to control
the CTS/RTS/DTR/DSR/DCD/RI signals.

Changed by Stefan:
Only call mctrl_gpio_init(), if the device has no ACPI companion device
to not break existing ACPI based systems. Also only use the mctrl_gpio_
functions when "gpios" is available.

Use MSR / MCR <-> TIOCM wrapper functions.

Signed-off-by: Yegor Yefremov 
Signed-off-by: Stefan Roese 
Reviewed-by: Andy Shevchenko 
Reviewed-by: Mika Westerberg 
Tested-by: Yegor Yefremov 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Giulio Benetti 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
---
v9:
- Rebased on top of "tty-next", patch 2/3 dropped as its already applied

v8:
- Rebased on top of "tty-next"

v7:
- Change serial8250_do_get_mctrl() so that systems with a "mixed setup"
  (i.e. CTS controlled by UART but other status pins controlled by GPIO)
  are also supported again (Yegor)

v6:
- Use newly introduced TIOCM <-> MCR/MSR wrapper functions
- serial8250_in_MCR(): Don't save the already read MCR bits in TIOCM
  format but "or" them later to the GPIO MCR value
- Correctly use "!up->gpios" (Andy)
- Removed Mika's reviewed by tag (because of changes)

v5:
- Dropped a few "if (up->gpios)" checks, as the mctrl_gpio_foo() API
  handles gpios == NULL (return)
- 8250_omap: Changed "IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(up->gpios, ...))"
  to "up->gpios == NULL", as mctrl_gpio_to_gpiod() does not handle
  gpios == NULL correctly.

v4:
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- Only call mctrl_gpio_init(), if the device has no ACPI companion device
  to not break existing ACPI based systems, as suggested by Andy

v2:
- No change

Please note that this patch was already applied before [1]. And later
reverted [2] because it introduced problems on some x86 based boards
(ACPI GPIO related). Here a detailed description of the issue at that
time:

https://lkml.org/lkml/2016/8/9/357
http://www.spinics.net/lists/linux-serial/msg23071.html

This is a re-send of the original patch that was applied at that time.
With patch 1/2 from this series this issue should be fixed now (please
note that I can't test it on such an x86 platform causing these
problems).

Andy (or Mika), perhaps it would be possible for you to test this
patch again, now with patch 1/2 of this series applied as well?
That would be really helpful.

Thanks,
Stefan

[1] 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
[2] 5db4f7f80d16 ("Revert "tty/serial/8250: use mctrl_gpio helpers"")

 .../devicetree/bindings/serial/8250.txt   | 19 
 drivers/tty/serial/8250/8250.h| 18 +++-
 drivers/tty/serial/8250/8250_core.c   | 17 +++
 drivers/tty/serial/8250/8250_omap.c   | 29 ++-
 drivers/tty/serial/8250/8250_port.c   | 11 ++-
 drivers/tty/serial/8250/Kconfig   |  1 +
 include/linux/serial_8250.h   |  1 +
 7 files changed, 81 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.txt 
b/Documentation/devicetree/bindings/serial/8250.txt
index 3cba12f855b7..20d351f268ef 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -53,6 +53,9 @@ Optional properties:
   programmable TX FIFO thresholds.
 - resets : phandle + reset specifier pairs
 - overrun-throttle-ms : how long to pause uart rx when input overrun is 
encountered.
+- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
+  line respectively. It will use specified GPIO instead of the peripheral
+  function pin for the UART feature. If unsure, don't specify this property.
 
 Note:
 * fsl,ns16550:
@@ -74,3 +77,19 @@ Example:
interrupts = <10>;
reg-shift = <2>;
};
+
+Example for OMAP UART using GPIO-based modem control signals:
+
+   uart4: serial@49042000 {
+   compatible = "ti,omap3-uart";
+   reg = <0x49042000 0x400>;
+   interrupts = <80>;
+   ti,hwmods = "uart4";
+   clock-frequency = <4800>;
+   cts-gpios = < 5 GPIO_ACTIVE_LOW>;
+   rts-gpios = < 6 GPIO_ACTIVE_LOW>;
+   dtr-gpios = < 12 GPIO_ACTIVE_LOW>;
+   dsr-gpios = < 13 GPIO_ACTIVE_LOW>;
+   dcd-gpios = < 14 GPIO_ACTIVE_LOW>;
+   rng-gpios = < 15 GPIO_ACTIVE_LOW>;
+   };
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 57db8c1689af..33ad9d6de532 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -11,6 +11,8 @@
 #include 
 #include 
 
+#include "../serial_mctrl_gpio.h"
+
 struct uart_8250_dma {
  

[PATCH 1/2 v9] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

Device (URT4)
{
...
Name (_CRS, ResourceTemplate () {
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003A
}
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003D
}
})

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.

Signed-off-by: Stefan Roese 
Reviewed-by: Mika Westerberg 
Reviewed-by: Andy Shevchenko 
Tested-by: Yegor Yefremov 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
v9:
- Rebased on top of "tty-next", patch 2/3 dropped as its already applied

v8:
- Rebased on top of "tty-next"

v7:
- Include  to fix compile breakage on OMAP

v6:
- No change

v5:
- Simplified the code a bit (Andy)
- Added gpio_str == NULL handling (Andy)

v4:
- Add missing free() calls (Johan)
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- No change

v2:
- Include the problem description and analysis from Mika into the commit
  text, as suggested by Greg.

 drivers/tty/serial/serial_mctrl_gpio.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 39ed56214cd3..2b400189be91 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "serial_mctrl_gpio.h"
 
@@ -116,6 +117,19 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device 
*dev, unsigned int idx)
 
for (i = 0; i < UART_GPIO_MAX; i++) {
enum gpiod_flags flags;
+   char *gpio_str;
+   bool present;
+
+   /* Check if GPIO property exists and continue if not */
+   gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
+mctrl_gpios_desc[i].name);
+   if (!gpio_str)
+   continue;
+
+   present = device_property_present(dev, gpio_str);
+   kfree(gpio_str);
+   if (!present)
+   continue;
 
if (mctrl_gpios_desc[i].dir_out)
flags = GPIOD_OUT_LOW;
-- 
2.22.0

[PATCH 3/3 v8] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

From: Yegor Yefremov 

This patch permits the usage for GPIOs to control
the CTS/RTS/DTR/DSR/DCD/RI signals.

Changed by Stefan:
Only call mctrl_gpio_init(), if the device has no ACPI companion device
to not break existing ACPI based systems. Also only use the mctrl_gpio_
functions when "gpios" is available.

Use MSR / MCR <-> TIOCM wrapper functions.

Signed-off-by: Yegor Yefremov 
Signed-off-by: Stefan Roese 
Reviewed-by: Andy Shevchenko 
Reviewed-by: Mika Westerberg 
Tested-by: Yegor Yefremov 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Giulio Benetti 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
---
v8:
- Rebased on top of "tty-next"

v7:
- Change serial8250_do_get_mctrl() so that systems with a "mixed setup"
  (i.e. CTS controlled by UART but other status pins controlled by GPIO)
  are also supported again (Yegor)

v6:
- Use newly introduced TIOCM <-> MCR/MSR wrapper functions
- serial8250_in_MCR(): Don't save the already read MCR bits in TIOCM
  format but "or" them later to the GPIO MCR value
- Correctly use "!up->gpios" (Andy)
- Removed Mika's reviewed by tag (because of changes)

v5:
- Dropped a few "if (up->gpios)" checks, as the mctrl_gpio_foo() API
  handles gpios == NULL (return)
- 8250_omap: Changed "IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(up->gpios, ...))"
  to "up->gpios == NULL", as mctrl_gpio_to_gpiod() does not handle
  gpios == NULL correctly.

v4:
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- Only call mctrl_gpio_init(), if the device has no ACPI companion device
  to not break existing ACPI based systems, as suggested by Andy

v2:
- No change

Please note that this patch was already applied before [1]. And later
reverted [2] because it introduced problems on some x86 based boards
(ACPI GPIO related). Here a detailed description of the issue at that
time:

https://lkml.org/lkml/2016/8/9/357
http://www.spinics.net/lists/linux-serial/msg23071.html

This is a re-send of the original patch that was applied at that time.
With patch 1/2 from this series this issue should be fixed now (please
note that I can't test it on such an x86 platform causing these
problems).

Andy (or Mika), perhaps it would be possible for you to test this
patch again, now with patch 1/2 of this series applied as well?
That would be really helpful.

Thanks,
Stefan

[1] 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
[2] 5db4f7f80d16 ("Revert "tty/serial/8250: use mctrl_gpio helpers"")

 .../devicetree/bindings/serial/8250.txt   | 19 
 drivers/tty/serial/8250/8250.h| 18 +++-
 drivers/tty/serial/8250/8250_core.c   | 17 +++
 drivers/tty/serial/8250/8250_omap.c   | 29 ++-
 drivers/tty/serial/8250/8250_port.c   | 11 ++-
 drivers/tty/serial/8250/Kconfig   |  1 +
 include/linux/serial_8250.h   |  1 +
 7 files changed, 81 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.txt 
b/Documentation/devicetree/bindings/serial/8250.txt
index 3cba12f855b7..20d351f268ef 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -53,6 +53,9 @@ Optional properties:
   programmable TX FIFO thresholds.
 - resets : phandle + reset specifier pairs
 - overrun-throttle-ms : how long to pause uart rx when input overrun is 
encountered.
+- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
+  line respectively. It will use specified GPIO instead of the peripheral
+  function pin for the UART feature. If unsure, don't specify this property.
 
 Note:
 * fsl,ns16550:
@@ -74,3 +77,19 @@ Example:
interrupts = <10>;
reg-shift = <2>;
};
+
+Example for OMAP UART using GPIO-based modem control signals:
+
+   uart4: serial@49042000 {
+   compatible = "ti,omap3-uart";
+   reg = <0x49042000 0x400>;
+   interrupts = <80>;
+   ti,hwmods = "uart4";
+   clock-frequency = <4800>;
+   cts-gpios = < 5 GPIO_ACTIVE_LOW>;
+   rts-gpios = < 6 GPIO_ACTIVE_LOW>;
+   dtr-gpios = < 12 GPIO_ACTIVE_LOW>;
+   dsr-gpios = < 13 GPIO_ACTIVE_LOW>;
+   dcd-gpios = < 14 GPIO_ACTIVE_LOW>;
+   rng-gpios = < 15 GPIO_ACTIVE_LOW>;
+   };
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 793da2e510e0..75c7c5449461 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -11,6 +11,8 @@
 #include 
 #include 
 
+#include "../serial_mctrl_gpio.h"
+
 struct uart_8250_dma {
int (*tx_dma)(struct uart_8250_port *p);
int (*rx_dma)(struct uart_82

[PATCH 2/3 v8] serial: 8250: Add MSR/MCR TIOCM conversion wrapper functions

[Stefan Roese]

This patch adds wrapper functions to convert MSR <-> TIOCM and also
MCR <-> TIOCM. These functions are used now in serial8250_do_set_mctrl()
and serial8250_do_get_mctrl().

Signed-off-by: Stefan Roese 
Suggested-by: Andy Shevchenko 
Reviewed-by: Andy Shevchenko 
Reviewed-by: Mika Westerberg 
Tested-by: Yegor Yefremov 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
v8:
- Rebased on top of "tty-next"

v7:
- No change

v6:
- New patch

 drivers/tty/serial/8250/8250.h  | 54 +
 drivers/tty/serial/8250/8250_port.c | 25 ++---
 2 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ebfb0bd5bef5..793da2e510e0 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -139,6 +139,60 @@ void serial8250_rpm_put_tx(struct uart_8250_port *p);
 int serial8250_em485_init(struct uart_8250_port *p);
 void serial8250_em485_destroy(struct uart_8250_port *p);
 
+/* MCR <-> TIOCM conversion */
+static inline int serial8250_TIOCM_to_MCR(int tiocm)
+{
+   int mcr = 0;
+
+   if (tiocm & TIOCM_RTS)
+   mcr |= UART_MCR_RTS;
+   if (tiocm & TIOCM_DTR)
+   mcr |= UART_MCR_DTR;
+   if (tiocm & TIOCM_OUT1)
+   mcr |= UART_MCR_OUT1;
+   if (tiocm & TIOCM_OUT2)
+   mcr |= UART_MCR_OUT2;
+   if (tiocm & TIOCM_LOOP)
+   mcr |= UART_MCR_LOOP;
+
+   return mcr;
+}
+
+static inline int serial8250_MCR_to_TIOCM(int mcr)
+{
+   int tiocm = 0;
+
+   if (mcr & UART_MCR_RTS)
+   tiocm |= TIOCM_RTS;
+   if (mcr & UART_MCR_DTR)
+   tiocm |= TIOCM_DTR;
+   if (mcr & UART_MCR_OUT1)
+   tiocm |= TIOCM_OUT1;
+   if (mcr & UART_MCR_OUT2)
+   tiocm |= TIOCM_OUT2;
+   if (mcr & UART_MCR_LOOP)
+   tiocm |= TIOCM_LOOP;
+
+   return tiocm;
+}
+
+/* MSR <-> TIOCM conversion */
+static inline int serial8250_MSR_to_TIOCM(int msr)
+{
+   int tiocm = 0;
+
+   if (msr & UART_MSR_DCD)
+   tiocm |= TIOCM_CAR;
+   if (msr & UART_MSR_RI)
+   tiocm |= TIOCM_RNG;
+   if (msr & UART_MSR_DSR)
+   tiocm |= TIOCM_DSR;
+   if (msr & UART_MSR_CTS)
+   tiocm |= TIOCM_CTS;
+
+   return tiocm;
+}
+
 static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
 {
serial_out(up, UART_MCR, value);
diff --git a/drivers/tty/serial/8250/8250_port.c 
b/drivers/tty/serial/8250/8250_port.c
index 60c9cea70128..19791d3694c6 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1945,22 +1945,12 @@ unsigned int serial8250_do_get_mctrl(struct uart_port 
*port)
 {
struct uart_8250_port *up = up_to_u8250p(port);
unsigned int status;
-   unsigned int ret;
 
serial8250_rpm_get(up);
status = serial8250_modem_status(up);
serial8250_rpm_put(up);
 
-   ret = 0;
-   if (status & UART_MSR_DCD)
-   ret |= TIOCM_CAR;
-   if (status & UART_MSR_RI)
-   ret |= TIOCM_RNG;
-   if (status & UART_MSR_DSR)
-   ret |= TIOCM_DSR;
-   if (status & UART_MSR_CTS)
-   ret |= TIOCM_CTS;
-   return ret;
+   return serial8250_MSR_to_TIOCM(status);
 }
 EXPORT_SYMBOL_GPL(serial8250_do_get_mctrl);
 
@@ -1974,18 +1964,9 @@ static unsigned int serial8250_get_mctrl(struct 
uart_port *port)
 void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
struct uart_8250_port *up = up_to_u8250p(port);
-   unsigned char mcr = 0;
+   unsigned char mcr;
 
-   if (mctrl & TIOCM_RTS)
-   mcr |= UART_MCR_RTS;
-   if (mctrl & TIOCM_DTR)
-   mcr |= UART_MCR_DTR;
-   if (mctrl & TIOCM_OUT1)
-   mcr |= UART_MCR_OUT1;
-   if (mctrl & TIOCM_OUT2)
-   mcr |= UART_MCR_OUT2;
-   if (mctrl & TIOCM_LOOP)
-   mcr |= UART_MCR_LOOP;
+   mcr = serial8250_TIOCM_to_MCR(mctrl);
 
mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
 
-- 
2.22.0

[PATCH 1/3 v8] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

Device (URT4)
{
...
Name (_CRS, ResourceTemplate () {
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003A
}
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003D
}
})

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.

Signed-off-by: Stefan Roese 
Reviewed-by: Mika Westerberg 
Reviewed-by: Andy Shevchenko 
Tested-by: Yegor Yefremov 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
v8:
- Rebased on top of "tty-next"

v7:
- Include  to fix compile breakage on OMAP

v6:
- No change

v5:
- Simplified the code a bit (Andy)
- Added gpio_str == NULL handling (Andy)

v4:
- Add missing free() calls (Johan)
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- No change

v2:
- Include the problem description and analysis from Mika into the commit
  text, as suggested by Greg.

 drivers/tty/serial/serial_mctrl_gpio.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 39ed56214cd3..2b400189be91 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "serial_mctrl_gpio.h"
 
@@ -116,6 +117,19 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device 
*dev, unsigned int idx)
 
for (i = 0; i < UART_GPIO_MAX; i++) {
enum gpiod_flags flags;
+   char *gpio_str;
+   bool present;
+
+   /* Check if GPIO property exists and continue if not */
+   gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
+mctrl_gpios_desc[i].name);
+   if (!gpio_str)
+   continue;
+
+   present = device_property_present(dev, gpio_str);
+   kfree(gpio_str);
+   if (!present)
+   continue;
 
if (mctrl_gpios_desc[i].dir_out)
flags = GPIOD_OUT_LOW;
-- 
2.22.0

[PATCH 3/3 v7] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

From: Yegor Yefremov 

This patch permits the usage for GPIOs to control
the CTS/RTS/DTR/DSR/DCD/RI signals.

Changed by Stefan:
Only call mctrl_gpio_init(), if the device has no ACPI companion device
to not break existing ACPI based systems. Also only use the mctrl_gpio_
functions when "gpios" is available.

Use MSR / MCR <-> TIOCM wrapper functions.

Signed-off-by: Yegor Yefremov 
Signed-off-by: Greg Kroah-Hartman 
Signed-off-by: Stefan Roese 
Reviewed-by: Andy Shevchenko 
Reviewed-by: Mika Westerberg 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Giulio Benetti 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
---
v7:
- Change serial8250_do_get_mctrl() so that systems with a "mixed setup"
  (i.e. CTS controlled by UART but other status pins controlled by GPIO)
  are also supported again (Yegor)

v6:
- Use newly introduced TIOCM <-> MCR/MSR wrapper functions
- serial8250_in_MCR(): Don't save the already read MCR bits in TIOCM
  format but "or" them later to the GPIO MCR value
- Correctly use "!up->gpios" (Andy)
- Removed Mika's reviewed by tag (because of changes)

v5:
- Dropped a few "if (up->gpios)" checks, as the mctrl_gpio_foo() API
  handles gpios == NULL (return)
- 8250_omap: Changed "IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(up->gpios, ...))"
  to "up->gpios == NULL", as mctrl_gpio_to_gpiod() does not handle
  gpios == NULL correctly.

v4:
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- Only call mctrl_gpio_init(), if the device has no ACPI companion device
  to not break existing ACPI based systems, as suggested by Andy

v2:
- No change

Please note that this patch was already applied before [1]. And later
reverted [2] because it introduced problems on some x86 based boards
(ACPI GPIO related). Here a detailed description of the issue at that
time:

https://lkml.org/lkml/2016/8/9/357
http://www.spinics.net/lists/linux-serial/msg23071.html

This is a re-send of the original patch that was applied at that time.
With patch 1/2 from this series this issue should be fixed now (please
note that I can't test it on such an x86 platform causing these
problems).

Andy (or Mika), perhaps it would be possible for you to test this
patch again, now with patch 1/2 of this series applied as well?
That would be really helpful.

Thanks,
Stefan

[1] 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
[2] 5db4f7f80d16 ("Revert "tty/serial/8250: use mctrl_gpio helpers"")

 .../devicetree/bindings/serial/8250.txt   | 19 
 drivers/tty/serial/8250/8250.h| 18 +++-
 drivers/tty/serial/8250/8250_core.c   | 17 +++
 drivers/tty/serial/8250/8250_omap.c   | 29 ++-
 drivers/tty/serial/8250/8250_port.c   | 11 ++-
 drivers/tty/serial/8250/Kconfig   |  1 +
 include/linux/serial_8250.h   |  1 +
 7 files changed, 81 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.txt 
b/Documentation/devicetree/bindings/serial/8250.txt
index 3cba12f855b7..20d351f268ef 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -53,6 +53,9 @@ Optional properties:
   programmable TX FIFO thresholds.
 - resets : phandle + reset specifier pairs
 - overrun-throttle-ms : how long to pause uart rx when input overrun is 
encountered.
+- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
+  line respectively. It will use specified GPIO instead of the peripheral
+  function pin for the UART feature. If unsure, don't specify this property.
 
 Note:
 * fsl,ns16550:
@@ -74,3 +77,19 @@ Example:
interrupts = <10>;
reg-shift = <2>;
};
+
+Example for OMAP UART using GPIO-based modem control signals:
+
+   uart4: serial@49042000 {
+   compatible = "ti,omap3-uart";
+   reg = <0x49042000 0x400>;
+   interrupts = <80>;
+   ti,hwmods = "uart4";
+   clock-frequency = <4800>;
+   cts-gpios = < 5 GPIO_ACTIVE_LOW>;
+   rts-gpios = < 6 GPIO_ACTIVE_LOW>;
+   dtr-gpios = < 12 GPIO_ACTIVE_LOW>;
+   dsr-gpios = < 13 GPIO_ACTIVE_LOW>;
+   dcd-gpios = < 14 GPIO_ACTIVE_LOW>;
+   rng-gpios = < 15 GPIO_ACTIVE_LOW>;
+   };
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 793da2e510e0..75c7c5449461 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -11,6 +11,8 @@
 #include 
 #include 
 
+#include "../serial_mctrl_gpio.h"
+
 struct uart_8250_dma {
int (*tx_dma)(struct uart_8250_port *p);
int (*rx_dma)(struct uart_8250_port *p);
@@ -196,11 +198,25 @@ static inl

[PATCH 1/3 v7] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

Device (URT4)
{
...
Name (_CRS, ResourceTemplate () {
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003A
}
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003D
}
})

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.

Signed-off-by: Stefan Roese 
Reviewed-by: Mika Westerberg 
Reviewed-by: Andy Shevchenko 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
v7:
- Include  to fix compile breakage on OMAP

v6:
- No change

v5:
- Simplified the code a bit (Andy)
- Added gpio_str == NULL handling (Andy)

v4:
- Add missing free() calls (Johan)
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- No change

v2:
- Include the problem description and analysis from Mika into the commit
  text, as suggested by Greg.

 drivers/tty/serial/serial_mctrl_gpio.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 39ed56214cd3..2b400189be91 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "serial_mctrl_gpio.h"
 
@@ -116,6 +117,19 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device 
*dev, unsigned int idx)
 
for (i = 0; i < UART_GPIO_MAX; i++) {
enum gpiod_flags flags;
+   char *gpio_str;
+   bool present;
+
+   /* Check if GPIO property exists and continue if not */
+   gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
+mctrl_gpios_desc[i].name);
+   if (!gpio_str)
+   continue;
+
+   present = device_property_present(dev, gpio_str);
+   kfree(gpio_str);
+   if (!present)
+   continue;
 
if (mctrl_gpios_desc[i].dir_out)
flags = GPIOD_OUT_LOW;
-- 
2.22.0

[PATCH 2/3 v7] serial: 8250: Add MSR/MCR TIOCM conversion wrapper functions

[Stefan Roese]

This patch adds wrapper functions to convert MSR <-> TIOCM and also
MCR <-> TIOCM. These functions are used now in serial8250_do_set_mctrl()
and serial8250_do_get_mctrl().

Signed-off-by: Stefan Roese 
Suggested-by: Andy Shevchenko 
Reviewed-by: Andy Shevchenko 
Reviewed-by: Mika Westerberg 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
v7:
- No change

v6:
- New patch

 drivers/tty/serial/8250/8250.h  | 54 +
 drivers/tty/serial/8250/8250_port.c | 25 ++---
 2 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ebfb0bd5bef5..793da2e510e0 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -139,6 +139,60 @@ void serial8250_rpm_put_tx(struct uart_8250_port *p);
 int serial8250_em485_init(struct uart_8250_port *p);
 void serial8250_em485_destroy(struct uart_8250_port *p);
 
+/* MCR <-> TIOCM conversion */
+static inline int serial8250_TIOCM_to_MCR(int tiocm)
+{
+   int mcr = 0;
+
+   if (tiocm & TIOCM_RTS)
+   mcr |= UART_MCR_RTS;
+   if (tiocm & TIOCM_DTR)
+   mcr |= UART_MCR_DTR;
+   if (tiocm & TIOCM_OUT1)
+   mcr |= UART_MCR_OUT1;
+   if (tiocm & TIOCM_OUT2)
+   mcr |= UART_MCR_OUT2;
+   if (tiocm & TIOCM_LOOP)
+   mcr |= UART_MCR_LOOP;
+
+   return mcr;
+}
+
+static inline int serial8250_MCR_to_TIOCM(int mcr)
+{
+   int tiocm = 0;
+
+   if (mcr & UART_MCR_RTS)
+   tiocm |= TIOCM_RTS;
+   if (mcr & UART_MCR_DTR)
+   tiocm |= TIOCM_DTR;
+   if (mcr & UART_MCR_OUT1)
+   tiocm |= TIOCM_OUT1;
+   if (mcr & UART_MCR_OUT2)
+   tiocm |= TIOCM_OUT2;
+   if (mcr & UART_MCR_LOOP)
+   tiocm |= TIOCM_LOOP;
+
+   return tiocm;
+}
+
+/* MSR <-> TIOCM conversion */
+static inline int serial8250_MSR_to_TIOCM(int msr)
+{
+   int tiocm = 0;
+
+   if (msr & UART_MSR_DCD)
+   tiocm |= TIOCM_CAR;
+   if (msr & UART_MSR_RI)
+   tiocm |= TIOCM_RNG;
+   if (msr & UART_MSR_DSR)
+   tiocm |= TIOCM_DSR;
+   if (msr & UART_MSR_CTS)
+   tiocm |= TIOCM_CTS;
+
+   return tiocm;
+}
+
 static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
 {
serial_out(up, UART_MCR, value);
diff --git a/drivers/tty/serial/8250/8250_port.c 
b/drivers/tty/serial/8250/8250_port.c
index 2304a84eee3b..47f0a8d01a57 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1944,22 +1944,12 @@ unsigned int serial8250_do_get_mctrl(struct uart_port 
*port)
 {
struct uart_8250_port *up = up_to_u8250p(port);
unsigned int status;
-   unsigned int ret;
 
serial8250_rpm_get(up);
status = serial8250_modem_status(up);
serial8250_rpm_put(up);
 
-   ret = 0;
-   if (status & UART_MSR_DCD)
-   ret |= TIOCM_CAR;
-   if (status & UART_MSR_RI)
-   ret |= TIOCM_RNG;
-   if (status & UART_MSR_DSR)
-   ret |= TIOCM_DSR;
-   if (status & UART_MSR_CTS)
-   ret |= TIOCM_CTS;
-   return ret;
+   return serial8250_MSR_to_TIOCM(status);
 }
 EXPORT_SYMBOL_GPL(serial8250_do_get_mctrl);
 
@@ -1973,18 +1963,9 @@ static unsigned int serial8250_get_mctrl(struct 
uart_port *port)
 void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
struct uart_8250_port *up = up_to_u8250p(port);
-   unsigned char mcr = 0;
+   unsigned char mcr;
 
-   if (mctrl & TIOCM_RTS)
-   mcr |= UART_MCR_RTS;
-   if (mctrl & TIOCM_DTR)
-   mcr |= UART_MCR_DTR;
-   if (mctrl & TIOCM_OUT1)
-   mcr |= UART_MCR_OUT1;
-   if (mctrl & TIOCM_OUT2)
-   mcr |= UART_MCR_OUT2;
-   if (mctrl & TIOCM_LOOP)
-   mcr |= UART_MCR_LOOP;
+   mcr = serial8250_TIOCM_to_MCR(mctrl);
 
mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
 
-- 
2.22.0

Re: [PATCH 3/3 v6] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

On 17.06.19 11:51, Yegor Yefremov wrote:




@@ -1944,11 +1948,15 @@ unsigned int serial8250_do_get_mctrl(struct uart_port 
*port)
  {
 struct uart_8250_port *up = up_to_u8250p(port);
 unsigned int status;
+   unsigned int val = 0;

 serial8250_rpm_get(up);
 status = serial8250_modem_status(up);
 serial8250_rpm_put(up);

+   if (up->gpios)
+   return mctrl_gpio_get(up->gpios, );
+


What happens when you have a mixed setup i.e. CTS controlled by UART
but other status pins controlled by GPIO? In this case CTS status
won't be returned. Do I see it right?


Yes, your analysis does seem to be correct. Please note that I did
not intentionally did change it this way. I was not thinking about
such a "mixed design".
 

What about something like this:

unsigned int serial8250_do_get_mctrl(struct uart_port *port)
   {
   struct uart_8250_port *up = up_to_u8250p(port);
   unsigned int status;
   unsigned int val;

   serial8250_rpm_get(up);
   status = serial8250_modem_status(up);
   serial8250_rpm_put(up);

   val = serial8250_MSR_to_TIOCM(status);
   if (up->gpios)
   mctrl_gpio_get(up->gpios, );

   return val;
   }


Looks good to me, thanks. Do you have such a setup with some modem
control signal handled via GPIO and some via the UART? Could you
test such a change?

Thanks,
Stefan

Re: [PATCH 1/3 v6] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

On 14.06.19 11:04, Yegor Yefremov wrote:

On Thu, Jun 13, 2019 at 7:08 PM Andy Shevchenko
 wrote:


On Thu, Jun 13, 2019 at 05:45:40PM +0200, Stefan Roese wrote:

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

 Device (URT4)
 {
 ...
 Name (_CRS, ResourceTemplate () {
 GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
 "\\_SB.GPO0", 0x00, ResourceConsumer)
 {
 0x003A
 }
 GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
 "\\_SB.GPO0", 0x00, ResourceConsumer)
 {
 0x003D
 }
 })

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.



Reviewed-by: Andy Shevchenko 


I cannot compile the driver without adding #include 
to drivers/tty/serial/serial_mctrl_gpio.c.

My platform is AM335X (OMAP3). I've tried the patches both against the
main repo and also tty-next.

Other than that everything is working.


Thanks for reporting. I'll wait a bit for other review comments and
tests (thanks Andy) and will then send v7 with this header included
(and compile tested on OMAP3) later next week.

BTW: Could you please add a Tested-by-tag with the next version?

Thanks,
Stefan

Re: [PATCH 2/2 v5] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

On 12.06.19 11:16, Andy Shevchenko wrote:

On Wed, Jun 12, 2019 at 10:13:05AM +0200, Stefan Roese wrote:

On 11.06.19 16:48, Andy Shevchenko wrote:

On Tue, Jun 11, 2019 at 04:02:54PM +0200, Stefan Roese wrote:

On 11.06.19 14:44, Andy Shevchenko wrote:

On Tue, Jun 11, 2019 at 12:56:03PM +0200, Stefan Roese wrote:



static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
{
serial_out(up, UART_MCR, value);
+
+   if (up->gpios) {
+   int mctrl_gpio = 0;
+
+   if (value & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (value & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio_set(up->gpios, mctrl_gpio);
+   }
}



static inline int serial8250_in_MCR(struct uart_8250_port *up)
{
-   return serial_in(up, UART_MCR);
+   int mctrl;
+
+   mctrl = serial_in(up, UART_MCR);
+
+   if (up->gpios) {
+   int mctrl_gpio = 0;
+
+   /* save current MCR values */
+   if (mctrl & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (mctrl & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, _gpio);
+   if (mctrl_gpio & TIOCM_RTS)
+   mctrl |= UART_MCR_RTS;
+   else
+   mctrl &= ~UART_MCR_RTS;
+
+   if (mctrl_gpio & TIOCM_DTR)
+   mctrl |= UART_MCR_DTR;
+   else
+   mctrl &= ~UART_MCR_DTR;
+   }
+
+   return mctrl;
}


These are using OR logic with potentially volatile data. Shouldn't we mask
unused bits in UART_MCR in case of up->gpios != NULL?


Sorry, I don't see, which bits you are referring to? Could you please be
a bit more specific with the variable / macro meant (example)?


I meant that we double write values in the out() which might have some
consequences, though I hope nothing wrong with it happens.


Where is the double write to a register? Sorry, I fail to spot it.


Not to the one register. From the functional point of view the same signal is
set up twice: once per UART register, once per GPIO pins.


In the in() we read the all bits in the register.

As now I look at the implementation of mctrl_gpio_get_outputs(),
I think we rather get helpers for conversion between TIOCM and UART_MCR values,
so, they can be used in get_mctrl() / set_mctrl() and above.


Do you something like this in mind?


More likely

static inline int serial8250_MCR_to_TIOCM(int mcr)


MSR_to_TIOCM (see below) ...


{
int tiocm = 0;

if (mcr & ...)
tiocm |= ...;
...

return tiocm;
}

static inline int serial8250_TIOCM_to_MCR(int tiocm)
{
... in a similar way ...
}


While implementing such wrapper functions I noticed, that get_mctrl() /
set_mctrl() need TIOCM->MCR and MSR->TIOCM (notice MSR vs MCR here) but
serial8250_in_MCR() needs MCR->TIOCM. So there is not that much
overlay here. Additionally the wrappers would need to handle all bits
and only some of them are needed in serial8250_in/out_MCR(), so I would
need to add masking here as well.

For my taste its not really worth adding these wrappers as they won't
make things much clearer (if at all).

Thanks,
Stefan

[PATCH 3/3 v6] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

From: Yegor Yefremov 

This patch permits the usage for GPIOs to control
the CTS/RTS/DTR/DSR/DCD/RI signals.

Changed by Stefan:
Only call mctrl_gpio_init(), if the device has no ACPI companion device
to not break existing ACPI based systems. Also only use the mctrl_gpio_
functions when "gpios" is available.

Use MSR / MCR <-> TIOCM wrapper functions.

Signed-off-by: Yegor Yefremov 
Signed-off-by: Greg Kroah-Hartman 
Signed-off-by: Stefan Roese 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Giulio Benetti 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
---
v6:
- Use newly introduced TIOCM <-> MCR/MSR wrapper functions
- serial8250_in_MCR(): Don't save the already read MCR bits in TIOCM
  format but "or" them later to the GPIO MCR value
- Correctly use "!up->gpios" (Andy)
- Removed Mika's reviewed by tag (because of changes)

v5:
- Dropped a few "if (up->gpios)" checks, as the mctrl_gpio_foo() API
  handles gpios == NULL (return)
- 8250_omap: Changed "IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(up->gpios, ...))"
  to "up->gpios == NULL", as mctrl_gpio_to_gpiod() does not handle
  gpios == NULL correctly.

v4:
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- Only call mctrl_gpio_init(), if the device has no ACPI companion device
  to not break existing ACPI based systems, as suggested by Andy

v2:
- No change

Please note that this patch was already applied before [1]. And later
reverted [2] because it introduced problems on some x86 based boards
(ACPI GPIO related). Here a detailed description of the issue at that
time:

https://lkml.org/lkml/2016/8/9/357
http://www.spinics.net/lists/linux-serial/msg23071.html

This is a re-send of the original patch that was applied at that time.
With patch 1/2 from this series this issue should be fixed now (please
note that I can't test it on such an x86 platform causing these
problems).

Andy (or Mika), perhaps it would be possible for you to test this
patch again, now with patch 1/2 of this series applied as well?
That would be really helpful.

Thanks,
Stefan

[1] 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
[2] 5db4f7f80d16 ("Revert "tty/serial/8250: use mctrl_gpio helpers"")

 .../devicetree/bindings/serial/8250.txt   | 19 
 drivers/tty/serial/8250/8250.h| 18 +++-
 drivers/tty/serial/8250/8250_core.c   | 17 +++
 drivers/tty/serial/8250/8250_omap.c   | 29 ++-
 drivers/tty/serial/8250/8250_port.c   |  8 +
 drivers/tty/serial/8250/Kconfig   |  1 +
 include/linux/serial_8250.h   |  1 +
 7 files changed, 79 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.txt 
b/Documentation/devicetree/bindings/serial/8250.txt
index 3cba12f855b7..20d351f268ef 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -53,6 +53,9 @@ Optional properties:
   programmable TX FIFO thresholds.
 - resets : phandle + reset specifier pairs
 - overrun-throttle-ms : how long to pause uart rx when input overrun is 
encountered.
+- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
+  line respectively. It will use specified GPIO instead of the peripheral
+  function pin for the UART feature. If unsure, don't specify this property.
 
 Note:
 * fsl,ns16550:
@@ -74,3 +77,19 @@ Example:
interrupts = <10>;
reg-shift = <2>;
};
+
+Example for OMAP UART using GPIO-based modem control signals:
+
+   uart4: serial@49042000 {
+   compatible = "ti,omap3-uart";
+   reg = <0x49042000 0x400>;
+   interrupts = <80>;
+   ti,hwmods = "uart4";
+   clock-frequency = <4800>;
+   cts-gpios = < 5 GPIO_ACTIVE_LOW>;
+   rts-gpios = < 6 GPIO_ACTIVE_LOW>;
+   dtr-gpios = < 12 GPIO_ACTIVE_LOW>;
+   dsr-gpios = < 13 GPIO_ACTIVE_LOW>;
+   dcd-gpios = < 14 GPIO_ACTIVE_LOW>;
+   rng-gpios = < 15 GPIO_ACTIVE_LOW>;
+   };
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 793da2e510e0..75c7c5449461 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -11,6 +11,8 @@
 #include 
 #include 
 
+#include "../serial_mctrl_gpio.h"
+
 struct uart_8250_dma {
int (*tx_dma)(struct uart_8250_port *p);
int (*rx_dma)(struct uart_8250_port *p);
@@ -196,11 +198,25 @@ static inline int serial8250_MSR_to_TIOCM(int msr)
 static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
 {
serial_out(up, UART_MCR, value);
+
+   if (up->gpios)
+   mctrl_gpio_set(up->gpios, serial8250_MCR

[PATCH 1/3 v6] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

Device (URT4)
{
...
Name (_CRS, ResourceTemplate () {
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003A
}
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003D
}
})

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.

Signed-off-by: Stefan Roese 
Reviewed-by: Mika Westerberg 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
v6:
- No change

v5:
- Simplified the code a bit (Andy)
- Added gpio_str == NULL handling (Andy)

v4:
- Add missing free() calls (Johan)
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- No change

v2:
- Include the problem description and analysis from Mika into the commit
  text, as suggested by Greg.

 drivers/tty/serial/serial_mctrl_gpio.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 39ed56214cd3..65348887a749 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -116,6 +116,19 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device 
*dev, unsigned int idx)
 
for (i = 0; i < UART_GPIO_MAX; i++) {
enum gpiod_flags flags;
+   char *gpio_str;
+   bool present;
+
+   /* Check if GPIO property exists and continue if not */
+   gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
+mctrl_gpios_desc[i].name);
+   if (!gpio_str)
+   continue;
+
+   present = device_property_present(dev, gpio_str);
+   kfree(gpio_str);
+   if (!present)
+   continue;
 
if (mctrl_gpios_desc[i].dir_out)
flags = GPIOD_OUT_LOW;
-- 
2.22.0

[PATCH 2/3 v6] serial: 8250: Add MSR/MCR TIOCM conversion wrapper functions

[Stefan Roese]

This patch adds wrapper functions to convert MSR <-> TIOCM and also
MCR <-> TIOCM. These functions are used now in serial8250_do_set_mctrl()
and serial8250_do_get_mctrl().

Signed-off-by: Stefan Roese 
Suggested-by: Andy Shevchenko 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
v6:
- New patch

 drivers/tty/serial/8250/8250.h  | 54 +
 drivers/tty/serial/8250/8250_port.c | 25 ++---
 2 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ebfb0bd5bef5..793da2e510e0 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -139,6 +139,60 @@ void serial8250_rpm_put_tx(struct uart_8250_port *p);
 int serial8250_em485_init(struct uart_8250_port *p);
 void serial8250_em485_destroy(struct uart_8250_port *p);
 
+/* MCR <-> TIOCM conversion */
+static inline int serial8250_TIOCM_to_MCR(int tiocm)
+{
+   int mcr = 0;
+
+   if (tiocm & TIOCM_RTS)
+   mcr |= UART_MCR_RTS;
+   if (tiocm & TIOCM_DTR)
+   mcr |= UART_MCR_DTR;
+   if (tiocm & TIOCM_OUT1)
+   mcr |= UART_MCR_OUT1;
+   if (tiocm & TIOCM_OUT2)
+   mcr |= UART_MCR_OUT2;
+   if (tiocm & TIOCM_LOOP)
+   mcr |= UART_MCR_LOOP;
+
+   return mcr;
+}
+
+static inline int serial8250_MCR_to_TIOCM(int mcr)
+{
+   int tiocm = 0;
+
+   if (mcr & UART_MCR_RTS)
+   tiocm |= TIOCM_RTS;
+   if (mcr & UART_MCR_DTR)
+   tiocm |= TIOCM_DTR;
+   if (mcr & UART_MCR_OUT1)
+   tiocm |= TIOCM_OUT1;
+   if (mcr & UART_MCR_OUT2)
+   tiocm |= TIOCM_OUT2;
+   if (mcr & UART_MCR_LOOP)
+   tiocm |= TIOCM_LOOP;
+
+   return tiocm;
+}
+
+/* MSR <-> TIOCM conversion */
+static inline int serial8250_MSR_to_TIOCM(int msr)
+{
+   int tiocm = 0;
+
+   if (msr & UART_MSR_DCD)
+   tiocm |= TIOCM_CAR;
+   if (msr & UART_MSR_RI)
+   tiocm |= TIOCM_RNG;
+   if (msr & UART_MSR_DSR)
+   tiocm |= TIOCM_DSR;
+   if (msr & UART_MSR_CTS)
+   tiocm |= TIOCM_CTS;
+
+   return tiocm;
+}
+
 static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
 {
serial_out(up, UART_MCR, value);
diff --git a/drivers/tty/serial/8250/8250_port.c 
b/drivers/tty/serial/8250/8250_port.c
index 2304a84eee3b..47f0a8d01a57 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1944,22 +1944,12 @@ unsigned int serial8250_do_get_mctrl(struct uart_port 
*port)
 {
struct uart_8250_port *up = up_to_u8250p(port);
unsigned int status;
-   unsigned int ret;
 
serial8250_rpm_get(up);
status = serial8250_modem_status(up);
serial8250_rpm_put(up);
 
-   ret = 0;
-   if (status & UART_MSR_DCD)
-   ret |= TIOCM_CAR;
-   if (status & UART_MSR_RI)
-   ret |= TIOCM_RNG;
-   if (status & UART_MSR_DSR)
-   ret |= TIOCM_DSR;
-   if (status & UART_MSR_CTS)
-   ret |= TIOCM_CTS;
-   return ret;
+   return serial8250_MSR_to_TIOCM(status);
 }
 EXPORT_SYMBOL_GPL(serial8250_do_get_mctrl);
 
@@ -1973,18 +1963,9 @@ static unsigned int serial8250_get_mctrl(struct 
uart_port *port)
 void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
struct uart_8250_port *up = up_to_u8250p(port);
-   unsigned char mcr = 0;
+   unsigned char mcr;
 
-   if (mctrl & TIOCM_RTS)
-   mcr |= UART_MCR_RTS;
-   if (mctrl & TIOCM_DTR)
-   mcr |= UART_MCR_DTR;
-   if (mctrl & TIOCM_OUT1)
-   mcr |= UART_MCR_OUT1;
-   if (mctrl & TIOCM_OUT2)
-   mcr |= UART_MCR_OUT2;
-   if (mctrl & TIOCM_LOOP)
-   mcr |= UART_MCR_LOOP;
+   mcr = serial8250_TIOCM_to_MCR(mctrl);
 
mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
 
-- 
2.22.0

Re: [PATCH 2/2 v5] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

On 11.06.19 16:48, Andy Shevchenko wrote:

On Tue, Jun 11, 2019 at 04:02:54PM +0200, Stefan Roese wrote:

On 11.06.19 14:44, Andy Shevchenko wrote:

On Tue, Jun 11, 2019 at 12:56:03PM +0200, Stefan Roese wrote:



   static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
   {
serial_out(up, UART_MCR, value);
+
+   if (up->gpios) {
+   int mctrl_gpio = 0;
+
+   if (value & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (value & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio_set(up->gpios, mctrl_gpio);
+   }
   }



   static inline int serial8250_in_MCR(struct uart_8250_port *up)
   {
-   return serial_in(up, UART_MCR);
+   int mctrl;
+
+   mctrl = serial_in(up, UART_MCR);
+
+   if (up->gpios) {
+   int mctrl_gpio = 0;
+
+   /* save current MCR values */
+   if (mctrl & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (mctrl & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, _gpio);
+   if (mctrl_gpio & TIOCM_RTS)
+   mctrl |= UART_MCR_RTS;
+   else
+   mctrl &= ~UART_MCR_RTS;
+
+   if (mctrl_gpio & TIOCM_DTR)
+   mctrl |= UART_MCR_DTR;
+   else
+   mctrl &= ~UART_MCR_DTR;
+   }
+
+   return mctrl;
   }


These are using OR logic with potentially volatile data. Shouldn't we mask
unused bits in UART_MCR in case of up->gpios != NULL?


Sorry, I don't see, which bits you are referring to? Could you please be
a bit more specific with the variable / macro meant (example)?


I meant that we double write values in the out() which might have some
consequences, though I hope nothing wrong with it happens.


Where is the double write to a register? Sorry, I fail to spot it.
 

In the in() we read the all bits in the register.

As now I look at the implementation of mctrl_gpio_get_outputs(),
I think we rather get helpers for conversion between TIOCM and UART_MCR values,
so, they can be used in get_mctrl() / set_mctrl() and above.


Do you something like this in mind?

diff --git a/drivers/tty/serial/8250/8250_port.c 
b/drivers/tty/serial/8250/8250_port.c
index dc9354e34b60..f44561fcb941 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1954,19 +1954,12 @@ unsigned int serial8250_do_get_mctrl(struct uart_port 
*port)
status = serial8250_modem_status(up);
serial8250_rpm_put(up);
 
-   ret = 0;

-
if (up->gpios)
return mctrl_gpio_get(up->gpios, );
 
-   if (status & UART_MSR_DCD)

-   ret |= TIOCM_CAR;
-   if (status & UART_MSR_RI)
-   ret |= TIOCM_RNG;
-   if (status & UART_MSR_DSR)
-   ret |= TIOCM_DSR;
-   if (status & UART_MSR_CTS)
-   ret |= TIOCM_CTS;
+   ret = UART_MSR_TO_TIOCM_DCD(status) | UART_MSR_TO_TIOCM_RI(status) |
+   UART_MSR_TO_TIOCM_DSR(status) | UART_MSR_TO_TIOCM_CTS(status);
+
return ret;
 }
 EXPORT_SYMBOL_GPL(serial8250_do_get_mctrl);
@@ -1983,16 +1976,9 @@ void serial8250_do_set_mctrl(struct uart_port *port, 
unsigned int mctrl)
struct uart_8250_port *up = up_to_u8250p(port);
unsigned char mcr = 0;
 
-   if (mctrl & TIOCM_RTS)

-   mcr |= UART_MCR_RTS;
-   if (mctrl & TIOCM_DTR)
-   mcr |= UART_MCR_DTR;
-   if (mctrl & TIOCM_OUT1)
-   mcr |= UART_MCR_OUT1;
-   if (mctrl & TIOCM_OUT2)
-   mcr |= UART_MCR_OUT2;
-   if (mctrl & TIOCM_LOOP)
-   mcr |= UART_MCR_LOOP;
+   mcr = TIOCM_TO_UART_MCR_RTS(mctrl) | TIOCM_TO_UART_MCR_DTR(mctrl) |
+   TIOCM_TO_UART_MCR_OUT1(mctrl) | TIOCM_TO_UART_MCR_OUT2(mctrl) |
+   TIOCM_TO_UART_MCR_LOOP(mctrl);
 
mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
 
diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h

index be07b5470f4b..bda905a1b765 100644
--- a/include/uapi/linux/serial_reg.h
+++ b/include/uapi/linux/serial_reg.h
@@ -376,5 +376,22 @@
 #define UART_ALTR_EN_TXFIFO_LW 0x01/* Enable the TX FIFO Low Watermark */
 #define UART_ALTR_TX_LOW   0x41/* Tx FIFO Low Watermark */
 
+#define UART_MSR_TO_TIOCM_DCD(val) ((val & UART_MSR_DCD) ? TIOCM_CAR : 0)

+#define UART_MSR_TO_TIOCM_RI(val)  ((val & UART_MSR_RI) ? TIOCM_RNG : 0)
+#define UART_MSR_TO_TIOCM_DSR(val) ((val & UART_MSR_DSR) ? TIOCM_DSR : 0)
+#define UART_MSR_TO_TIOCM_CTS(val) ((val & UART_MSR_CTS) ? TIOCM_CTS : 0)
+#define UART_MSR_TO_TIOCM_RTS(val) ((val & UART_MSR_RTS) ? TIOCM_RTS : 0)
+#de

Re: [PATCH 2/2 v5] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

On 11.06.19 14:44, Andy Shevchenko wrote:

On Tue, Jun 11, 2019 at 12:56:03PM +0200, Stefan Roese wrote:

From: Yegor Yefremov 

This patch permits the usage for GPIOs to control
the CTS/RTS/DTR/DSR/DCD/RI signals.



  static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
  {
serial_out(up, UART_MCR, value);
+
+   if (up->gpios) {
+   int mctrl_gpio = 0;
+
+   if (value & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (value & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio_set(up->gpios, mctrl_gpio);
+   }
  }
  
  static inline int serial8250_in_MCR(struct uart_8250_port *up)

  {
-   return serial_in(up, UART_MCR);
+   int mctrl;
+
+   mctrl = serial_in(up, UART_MCR);
+
+   if (up->gpios) {
+   int mctrl_gpio = 0;
+
+   /* save current MCR values */
+   if (mctrl & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (mctrl & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, _gpio);
+   if (mctrl_gpio & TIOCM_RTS)
+   mctrl |= UART_MCR_RTS;
+   else
+   mctrl &= ~UART_MCR_RTS;
+
+   if (mctrl_gpio & TIOCM_DTR)
+   mctrl |= UART_MCR_DTR;
+   else
+   mctrl &= ~UART_MCR_DTR;
+   }
+
+   return mctrl;
  }


These are using OR logic with potentially volatile data. Shouldn't we mask
unused bits in UART_MCR in case of up->gpios != NULL?


Sorry, I don't see, which bits you are referring to? Could you please be
a bit more specific with the variable / macro meant (example)?
 

+   if (up->gpios == 0)


This is type inconsistency with this check as far as I understand.
I guess you have to do either (up->gpios == NULL), or (!up->gpios).


Ah, right. Thanks for spotting.

Thanks,
Stefan

[PATCH 2/2 v5] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

From: Yegor Yefremov 

This patch permits the usage for GPIOs to control
the CTS/RTS/DTR/DSR/DCD/RI signals.

Changed by Stefan:
Only call mctrl_gpio_init(), if the device has no ACPI companion device
to not break existing ACPI based systems. Also only use the mctrl_gpio_
functions when "gpios" is available.

Signed-off-by: Yegor Yefremov 
Signed-off-by: Greg Kroah-Hartman 
Signed-off-by: Stefan Roese 
Reviewed-by: Mika Westerberg 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Giulio Benetti 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
---
v5:
- Dropped a few "if (up->gpios)" checks, as the mctrl_gpio_foo() API
  handles gpios == NULL (return)
- 8250_omap: Changed "IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(up->gpios, ...))"
  to "up->gpios == NULL", as mctrl_gpio_to_gpiod() does not handle
  gpios == NULL correctly.

v4:
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- Only call mctrl_gpio_init(), if the device has no ACPI companion device
  to not break existing ACPI based systems, as suggested by Andy

v2:
- No change

Please note that this patch was already applied before [1]. And later
reverted [2] because it introduced problems on some x86 based boards
(ACPI GPIO related). Here a detailed description of the issue at that
time:

https://lkml.org/lkml/2016/8/9/357
http://www.spinics.net/lists/linux-serial/msg23071.html

This is a re-send of the original patch that was applied at that time.
With patch 1/2 from this series this issue should be fixed now (please
note that I can't test it on such an x86 platform causing these
problems).

Andy (or Mika), perhaps it would be possible for you to test this
patch again, now with patch 1/2 of this series applied as well?
That would be really helpful.

Thanks,
Stefan

[1] 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
[2] 5db4f7f80d16 ("Revert "tty/serial/8250: use mctrl_gpio helpers"")

 .../devicetree/bindings/serial/8250.txt   | 19 +
 drivers/tty/serial/8250/8250.h| 40 ++-
 drivers/tty/serial/8250/8250_core.c   | 17 
 drivers/tty/serial/8250/8250_omap.c   | 29 --
 drivers/tty/serial/8250/8250_port.c   |  7 
 drivers/tty/serial/8250/Kconfig   |  1 +
 include/linux/serial_8250.h   |  1 +
 7 files changed, 100 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.txt 
b/Documentation/devicetree/bindings/serial/8250.txt
index 3cba12f855b7..20d351f268ef 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -53,6 +53,9 @@ Optional properties:
   programmable TX FIFO thresholds.
 - resets : phandle + reset specifier pairs
 - overrun-throttle-ms : how long to pause uart rx when input overrun is 
encountered.
+- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
+  line respectively. It will use specified GPIO instead of the peripheral
+  function pin for the UART feature. If unsure, don't specify this property.
 
 Note:
 * fsl,ns16550:
@@ -74,3 +77,19 @@ Example:
interrupts = <10>;
reg-shift = <2>;
};
+
+Example for OMAP UART using GPIO-based modem control signals:
+
+   uart4: serial@49042000 {
+   compatible = "ti,omap3-uart";
+   reg = <0x49042000 0x400>;
+   interrupts = <80>;
+   ti,hwmods = "uart4";
+   clock-frequency = <4800>;
+   cts-gpios = < 5 GPIO_ACTIVE_LOW>;
+   rts-gpios = < 6 GPIO_ACTIVE_LOW>;
+   dtr-gpios = < 12 GPIO_ACTIVE_LOW>;
+   dsr-gpios = < 13 GPIO_ACTIVE_LOW>;
+   dcd-gpios = < 14 GPIO_ACTIVE_LOW>;
+   rng-gpios = < 15 GPIO_ACTIVE_LOW>;
+   };
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ebfb0bd5bef5..441aab94264b 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -11,6 +11,8 @@
 #include 
 #include 
 
+#include "../serial_mctrl_gpio.h"
+
 struct uart_8250_dma {
int (*tx_dma)(struct uart_8250_port *p);
int (*rx_dma)(struct uart_8250_port *p);
@@ -142,11 +144,47 @@ void serial8250_em485_destroy(struct uart_8250_port *p);
 static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
 {
serial_out(up, UART_MCR, value);
+
+   if (up->gpios) {
+   int mctrl_gpio = 0;
+
+   if (value & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (value & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio_set(up->gpios, mctrl_gpio);
+   }
 }
 
 static inline int serial8250_in_MCR(struct uart_8250_port *up)
 {
-   retur

[PATCH 1/2 v5] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

Device (URT4)
{
...
Name (_CRS, ResourceTemplate () {
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003A
}
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003D
}
})

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.

Signed-off-by: Stefan Roese 
Reviewed-by: Mika Westerberg 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
v5:
- Simplified the code a bit (Andy)
- Added gpio_str == NULL handling (Andy)

v4:
- Add missing free() calls (Johan)
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- No change

v2:
- Include the problem description and analysis from Mika into the commit
  text, as suggested by Greg.

 drivers/tty/serial/serial_mctrl_gpio.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 39ed56214cd3..65348887a749 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -116,6 +116,19 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device 
*dev, unsigned int idx)
 
for (i = 0; i < UART_GPIO_MAX; i++) {
enum gpiod_flags flags;
+   char *gpio_str;
+   bool present;
+
+   /* Check if GPIO property exists and continue if not */
+   gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
+mctrl_gpios_desc[i].name);
+   if (!gpio_str)
+   continue;
+
+   present = device_property_present(dev, gpio_str);
+   kfree(gpio_str);
+   if (!present)
+   continue;
 
if (mctrl_gpios_desc[i].dir_out)
flags = GPIOD_OUT_LOW;
-- 
2.22.0

Re: [PATCH 2/2 v4] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

On 04.06.19 18:52, Andy Shevchenko wrote:

On Mon, Jun 03, 2019 at 10:33:32AM +0200, Stefan Roese wrote:

From: Yegor Yefremov 

This patch permits the usage for GPIOs to control
the CTS/RTS/DTR/DSR/DCD/RI signals.



+   if (up->gpios) {



+   mctrl_gpio_set(up->gpios, mctrl_gpio);
+   }


...


+   if (up->gpios) {



+   mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, _gpio);



+   }


...


+   gpios = mctrl_gpio_init(>port, 0);
+   if (IS_ERR(gpios)) {
+   if (PTR_ERR(gpios) != -ENOSYS)
+   return PTR_ERR(gpios);
+   }


...


+   if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(up->gpios,
+   UART_GPIO_RTS))) {



+   }


...


-   if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW) {
+   if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW
+   && IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(up->gpios,
+   UART_GPIO_RTS))) {



}


...


+   if (up->gpios)
+   mctrl_gpio_disable_ms(up->gpios);


...


+   if (up->gpios)
+   mctrl_gpio_enable_ms(up->gpios);


...


+   if (up->gpios)
+   return mctrl_gpio_get(up->gpios, );



Can we rather make this mimic the gpiod_get_optional() API?

So, if we get an error, it's an error, otherwise with NULL pointer the
operations goes to be no-op.

[IS_ERR_OR_NULL() -> IS_ERR(), if (up->gpios) -> /dev/null, etc]


So you want me to drop all "if (up->gpios)" checks? I can do this in
some cases (e.g. serial8250_disable_ms()). But I would like to keep
it in other cases, like serial8250_out_MCR(), where this check prevents
some unnecessary code execution in the "non-gpios mode" (and vice-versa).

Would this be acceptable?

BTW: Regarding the OMAP specific code: I'm not the author of this code
and I don't have access to such hardware to do some tests here. But
changing IS_ERR_OR_NULL() -> IS_ERR() in this OMAP code does not
seem correct. IIUTC, these "if" clauses are extended here by
IS_ERR_OR_NULL(mctrl_gpio_to_gpiod()) to check if the GPIO's are not
enabled / used. Currently this will probably break, since when called
with "gpios == NULL", mctrl_gpio_to_gpiod() will crash [1].

If you don't object (or have other suggestions), I'll change this to
use "up->gpios == 0" instead. This seems to be what the original author
wanted to achieve.

Okay?

Thanks,
Stefan

[1]

struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
  enum mctrl_gpio_idx gidx)
{
return gpios->gpio[gidx];
}

[PATCH 1/2 v4] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

Device (URT4)
{
...
Name (_CRS, ResourceTemplate () {
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003A
}
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003D
}
})

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.

Signed-off-by: Stefan Roese 
Reviewed-by: Mika Westerberg 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
Cc: Johan Hovold 
---
v4:
- Add missing free() calls (Johan)
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- No change

v2:
- Include the problem description and analysis from Mika into the commit
  text, as suggested by Greg.

 drivers/tty/serial/serial_mctrl_gpio.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 39ed56214cd3..6367f389cdfc 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -116,6 +116,16 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device 
*dev, unsigned int idx)
 
for (i = 0; i < UART_GPIO_MAX; i++) {
enum gpiod_flags flags;
+   char *gpio_str;
+
+   /* Check if GPIO property exists and continue if not */
+   gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
+mctrl_gpios_desc[i].name);
+   if (!device_property_present(dev, gpio_str)) {
+   kfree(gpio_str);
+   continue;
+   }
+   kfree(gpio_str);
 
if (mctrl_gpios_desc[i].dir_out)
flags = GPIOD_OUT_LOW;
-- 
2.21.0

[PATCH 2/2 v4] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

From: Yegor Yefremov 

This patch permits the usage for GPIOs to control
the CTS/RTS/DTR/DSR/DCD/RI signals.

Changed by Stefan:
Only call mctrl_gpio_init(), if the device has no ACPI companion device
to not break existing ACPI based systems. Also only use the mctrl_gpio_
functions when "gpios" is available.

Signed-off-by: Yegor Yefremov 
Signed-off-by: Greg Kroah-Hartman 
Signed-off-by: Stefan Roese 
Reviewed-by: Mika Westerberg 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Giulio Benetti 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Johan Hovold 
---
v4:
- Added Mika's reviewed by tag
- Added Johan to Cc

v3:
- Only call mctrl_gpio_init(), if the device has no ACPI companion device
  to not break existing ACPI based systems, as suggested by Andy

v2:
- No change

Please note that this patch was already applied before [1]. And later
reverted [2] because it introduced problems on some x86 based boards
(ACPI GPIO related). Here a detailed description of the issue at that
time:

https://lkml.org/lkml/2016/8/9/357
http://www.spinics.net/lists/linux-serial/msg23071.html

This is a re-send of the original patch that was applied at that time.
With patch 1/2 from this series this issue should be fixed now (please
note that I can't test it on such an x86 platform causing these
problems).

Andy (or Mika), perhaps it would be possible for you to test this
patch again, now with patch 1/2 of this series applied as well?
That would be really helpful.

Thanks,
Stefan

[1] 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
[2] 5db4f7f80d16 ("Revert "tty/serial/8250: use mctrl_gpio helpers"")

 .../devicetree/bindings/serial/8250.txt   | 19 +
 drivers/tty/serial/8250/8250.h| 40 ++-
 drivers/tty/serial/8250/8250_core.c   | 17 
 drivers/tty/serial/8250/8250_omap.c   | 31 --
 drivers/tty/serial/8250/8250_port.c   |  9 +
 drivers/tty/serial/8250/Kconfig   |  1 +
 include/linux/serial_8250.h   |  1 +
 7 files changed, 104 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.txt 
b/Documentation/devicetree/bindings/serial/8250.txt
index 3cba12f855b7..20d351f268ef 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -53,6 +53,9 @@ Optional properties:
   programmable TX FIFO thresholds.
 - resets : phandle + reset specifier pairs
 - overrun-throttle-ms : how long to pause uart rx when input overrun is 
encountered.
+- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
+  line respectively. It will use specified GPIO instead of the peripheral
+  function pin for the UART feature. If unsure, don't specify this property.
 
 Note:
 * fsl,ns16550:
@@ -74,3 +77,19 @@ Example:
interrupts = <10>;
reg-shift = <2>;
};
+
+Example for OMAP UART using GPIO-based modem control signals:
+
+   uart4: serial@49042000 {
+   compatible = "ti,omap3-uart";
+   reg = <0x49042000 0x400>;
+   interrupts = <80>;
+   ti,hwmods = "uart4";
+   clock-frequency = <4800>;
+   cts-gpios = < 5 GPIO_ACTIVE_LOW>;
+   rts-gpios = < 6 GPIO_ACTIVE_LOW>;
+   dtr-gpios = < 12 GPIO_ACTIVE_LOW>;
+   dsr-gpios = < 13 GPIO_ACTIVE_LOW>;
+   dcd-gpios = < 14 GPIO_ACTIVE_LOW>;
+   rng-gpios = < 15 GPIO_ACTIVE_LOW>;
+   };
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ebfb0bd5bef5..441aab94264b 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -11,6 +11,8 @@
 #include 
 #include 
 
+#include "../serial_mctrl_gpio.h"
+
 struct uart_8250_dma {
int (*tx_dma)(struct uart_8250_port *p);
int (*rx_dma)(struct uart_8250_port *p);
@@ -142,11 +144,47 @@ void serial8250_em485_destroy(struct uart_8250_port *p);
 static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
 {
serial_out(up, UART_MCR, value);
+
+   if (up->gpios) {
+   int mctrl_gpio = 0;
+
+   if (value & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (value & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio_set(up->gpios, mctrl_gpio);
+   }
 }
 
 static inline int serial8250_in_MCR(struct uart_8250_port *up)
 {
-   return serial_in(up, UART_MCR);
+   int mctrl;
+
+   mctrl = serial_in(up, UART_MCR);
+
+   if (up->gpios) {
+   int mctrl_gpio = 0;
+
+   /* save current MCR values */
+   if (mctrl & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RT

Re: [PATCH 1/2 v3] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

On 29.05.19 15:44, Johan Hovold wrote:

On Mon, May 27, 2019 at 01:18:04PM +0200, Stefan Roese wrote:

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.



Signed-off-by: Stefan Roese 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
v3:
- No change

v2:
- Include the problem description and analysis from Mika into the commit
   text, as suggested by Greg.

  drivers/tty/serial/serial_mctrl_gpio.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 39ed56214cd3..cac50b20a119 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -116,6 +116,13 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device 
*dev, unsigned int idx)
  
  	for (i = 0; i < UART_GPIO_MAX; i++) {

enum gpiod_flags flags;
+   char *gpio_str;
+
+   /* Check if GPIO property exists and continue if not */
+   gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
+mctrl_gpios_desc[i].name);


Where's the corresponding kfree?


Its missing. I'll add it in v4.

Thanks,
Stefan

[PATCH 2/2 v3] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

From: Yegor Yefremov 

This patch permits the usage for GPIOs to control
the CTS/RTS/DTR/DSR/DCD/RI signals.

Changed by Stefan:
Only call mctrl_gpio_init(), if the device has no ACPI companion device
to not break existing ACPI based systems. Also only use the mctrl_gpio_
functions when "gpios" is available.

Signed-off-by: Yegor Yefremov 
Signed-off-by: Greg Kroah-Hartman 
Signed-off-by: Stefan Roese 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Giulio Benetti 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
---
v3:
- Only call mctrl_gpio_init(), if the device has no ACPI companion device
  to not break existing ACPI based systems, as suggested by Andy

v2:
- No change

Please note that this patch was already applied before [1]. And later
reverted [2] because it introduced problems on some x86 based boards
(ACPI GPIO related). Here a detailed description of the issue at that
time:

https://lkml.org/lkml/2016/8/9/357
http://www.spinics.net/lists/linux-serial/msg23071.html

This is a re-send of the original patch that was applied at that time.
With patch 1/2 from this series this issue should be fixed now (please
note that I can't test it on such an x86 platform causing these
problems).

Andy (or Mika), perhaps it would be possible for you to test this
patch again, now with patch 1/2 of this series applied as well?
That would be really helpful.

Thanks,
Stefan

[1] 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
[2] 5db4f7f80d16 ("Revert "tty/serial/8250: use mctrl_gpio helpers"")

 .../devicetree/bindings/serial/8250.txt   | 19 +
 drivers/tty/serial/8250/8250.h| 40 ++-
 drivers/tty/serial/8250/8250_core.c   | 17 
 drivers/tty/serial/8250/8250_omap.c   | 31 --
 drivers/tty/serial/8250/8250_port.c   |  9 +
 drivers/tty/serial/8250/Kconfig   |  1 +
 include/linux/serial_8250.h   |  1 +
 7 files changed, 104 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.txt 
b/Documentation/devicetree/bindings/serial/8250.txt
index 3cba12f855b7..20d351f268ef 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -53,6 +53,9 @@ Optional properties:
   programmable TX FIFO thresholds.
 - resets : phandle + reset specifier pairs
 - overrun-throttle-ms : how long to pause uart rx when input overrun is 
encountered.
+- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
+  line respectively. It will use specified GPIO instead of the peripheral
+  function pin for the UART feature. If unsure, don't specify this property.
 
 Note:
 * fsl,ns16550:
@@ -74,3 +77,19 @@ Example:
interrupts = <10>;
reg-shift = <2>;
};
+
+Example for OMAP UART using GPIO-based modem control signals:
+
+   uart4: serial@49042000 {
+   compatible = "ti,omap3-uart";
+   reg = <0x49042000 0x400>;
+   interrupts = <80>;
+   ti,hwmods = "uart4";
+   clock-frequency = <4800>;
+   cts-gpios = < 5 GPIO_ACTIVE_LOW>;
+   rts-gpios = < 6 GPIO_ACTIVE_LOW>;
+   dtr-gpios = < 12 GPIO_ACTIVE_LOW>;
+   dsr-gpios = < 13 GPIO_ACTIVE_LOW>;
+   dcd-gpios = < 14 GPIO_ACTIVE_LOW>;
+   rng-gpios = < 15 GPIO_ACTIVE_LOW>;
+   };
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ebfb0bd5bef5..441aab94264b 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -11,6 +11,8 @@
 #include 
 #include 
 
+#include "../serial_mctrl_gpio.h"
+
 struct uart_8250_dma {
int (*tx_dma)(struct uart_8250_port *p);
int (*rx_dma)(struct uart_8250_port *p);
@@ -142,11 +144,47 @@ void serial8250_em485_destroy(struct uart_8250_port *p);
 static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
 {
serial_out(up, UART_MCR, value);
+
+   if (up->gpios) {
+   int mctrl_gpio = 0;
+
+   if (value & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (value & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio_set(up->gpios, mctrl_gpio);
+   }
 }
 
 static inline int serial8250_in_MCR(struct uart_8250_port *up)
 {
-   return serial_in(up, UART_MCR);
+   int mctrl;
+
+   mctrl = serial_in(up, UART_MCR);
+
+   if (up->gpios) {
+   int mctrl_gpio = 0;
+
+   /* save current MCR values */
+   if (mctrl & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (mctrl & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+

[PATCH 1/2 v3] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

Device (URT4)
{
...
Name (_CRS, ResourceTemplate () {
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003A
}
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003D
}
})

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.

Signed-off-by: Stefan Roese 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
v3:
- No change

v2:
- Include the problem description and analysis from Mika into the commit
  text, as suggested by Greg.

 drivers/tty/serial/serial_mctrl_gpio.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 39ed56214cd3..cac50b20a119 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -116,6 +116,13 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device 
*dev, unsigned int idx)
 
for (i = 0; i < UART_GPIO_MAX; i++) {
enum gpiod_flags flags;
+   char *gpio_str;
+
+   /* Check if GPIO property exists and continue if not */
+   gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
+mctrl_gpios_desc[i].name);
+   if (!device_property_present(dev, gpio_str))
+   continue;
 
if (mctrl_gpios_desc[i].dir_out)
flags = GPIOD_OUT_LOW;
-- 
2.21.0

Re: [PATCH 1/2 v2] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

On 24.05.19 15:46, Andy Shevchenko wrote:

On Fri, May 24, 2019 at 01:29:34PM +0200, Stefan Roese wrote:

On 24.05.19 13:11, Andy Shevchenko wrote:

On Fri, May 24, 2019 at 1:21 PM Mika Westerberg
 wrote:


On Fri, May 24, 2019 at 11:48:24AM +0200, Stefan Roese wrote:

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

  Device (URT4)
  {
  ...
  Name (_CRS, ResourceTemplate () {
  GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
  "\\_SB.GPO0", 0x00, ResourceConsumer)
  {
  0x003A
  }
  GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
  "\\_SB.GPO0", 0x00, ResourceConsumer)
  {
  0x003D
  }
  })

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.



We have a board where ASL provides _DSD for CTS and RxD pins.
I'm afraid this won't work on it.


With "won't work" you mean, that the GPIO can't be used for modem
control in this case in the current implementation (with this
patchset)? Or do you mean, that the breakage (input does not work
on Broxton systems) will not be solved by this patch?


It will solve RxD case, due to mctrl doesn't count RxD as a "control" line.

Though we have CTS pin defined for the same purpose, which means the hardware
flow control won't work on a subset of Broxton boards.


If its the former, then I think that solving this issue is something
for a new patch, to support modem-control on such platforms as well
(if needed).



Please note that this patch is not trying to get modem-control working
on such ACPI based systems.


I understand that. At the same time it should not break existing systems.


Its targeted for device-tree enabled
platforms, using the 8250 serial driver, here specifically a MIPS
MT7688 based board. And just wants to fix the latter issue mentioned
above so that the 8250 modem-control support can be accepted in
mainline.


As I said already we have to distinguish *the purpose* of these GPIOs.
(like CTS).

Can we apply this if and only if the device has no ACPI companion device?

In this case DT will work as you expect and ACPI won't be broken.


So your suggestion is to add a has_acpi_companion() check before
mctrl_gpio_init() is called in serial8250_register_8250_port() and
then only use the gpio related mctrl, if the GPIO's are really used?

I can certainly change patch 2/2 to do this. It would be great though,
if you (or someone else) could test this on such a ACPI based platform,
as I don't have access to such a board.

Thanks,
Stefan

Re: [PATCH 1/2 v2] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

On 24.05.19 13:11, Andy Shevchenko wrote:

On Fri, May 24, 2019 at 1:21 PM Mika Westerberg
 wrote:


On Fri, May 24, 2019 at 11:48:24AM +0200, Stefan Roese wrote:

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

 Device (URT4)
 {
 ...
 Name (_CRS, ResourceTemplate () {
 GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
 "\\_SB.GPO0", 0x00, ResourceConsumer)
 {
 0x003A
 }
 GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
 "\\_SB.GPO0", 0x00, ResourceConsumer)
 {
 0x003D
 }
 })

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.



We have a board where ASL provides _DSD for CTS and RxD pins.
I'm afraid this won't work on it.


With "won't work" you mean, that the GPIO can't be used for modem
control in this case in the current implementation (with this
patchset)? Or do you mean, that the breakage (input does not work
on Broxton systems) will not be solved by this patch?

If its the former, then I think that solving this issue is something
for a new patch, to support modem-control on such platforms as well
(if needed).

Please note that this patch is not trying to get modem-control working
on such ACPI based systems. Its targeted for device-tree enabled
platforms, using the 8250 serial driver, here specifically a MIPS
MT7688 based board. And just wants to fix the latter issue mentioned
above so that the 8250 modem-control support can be accepted in
mainline.
 

Basically we need to understand the use of the GPIOs in UART. In our
case it's an out-of-band wake up source for UART.
Simply requiring GPIOs to be present is not enough.

Perhaps property like 'modem-control-gpio-in-use' (this seems a bad
name, given for sake of example).


Thanks,
Stefan

[PATCH 2/2 v2] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

From: Yegor Yefremov 

This patch permits the usage for GPIOs to control
the CTS/RTS/DTR/DSR/DCD/RI signals.

Signed-off-by: Yegor Yefremov 
Signed-off-by: Greg Kroah-Hartman 
Signed-off-by: Stefan Roese 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Giulio Benetti 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
---
v2:
- No change

Please note that this patch was already applied before [1]. And later
reverted [2] because it introduced problems on some x86 based boards
(ACPI GPIO related). Here a detailed description of the issue at that
time:

https://lkml.org/lkml/2016/8/9/357
http://www.spinics.net/lists/linux-serial/msg23071.html

This is a re-send of the original patch that was applied at that time.
With patch 1/2 from this series this issue should be fixed now (please
note that I can't test it on such an x86 platform causing these
problems).

Andy (or Mika), perhaps it would be possible for you to test this
patch again, now with patch 1/2 of this series applied as well?
That would be really helpful.

Thanks,
Stefan

[1] 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
[2] 5db4f7f80d16 ("Revert "tty/serial/8250: use mctrl_gpio helpers"")

 .../devicetree/bindings/serial/8250.txt   | 19 ++
 drivers/tty/serial/8250/8250.h| 35 ++-
 drivers/tty/serial/8250/8250_core.c   |  9 +
 drivers/tty/serial/8250/8250_omap.c   | 31 +---
 drivers/tty/serial/8250/8250_port.c   |  7 +++-
 drivers/tty/serial/8250/Kconfig   |  1 +
 include/linux/serial_8250.h   |  1 +
 7 files changed, 88 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.txt 
b/Documentation/devicetree/bindings/serial/8250.txt
index 3cba12f855b7..20d351f268ef 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -53,6 +53,9 @@ Optional properties:
   programmable TX FIFO thresholds.
 - resets : phandle + reset specifier pairs
 - overrun-throttle-ms : how long to pause uart rx when input overrun is 
encountered.
+- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
+  line respectively. It will use specified GPIO instead of the peripheral
+  function pin for the UART feature. If unsure, don't specify this property.
 
 Note:
 * fsl,ns16550:
@@ -74,3 +77,19 @@ Example:
interrupts = <10>;
reg-shift = <2>;
};
+
+Example for OMAP UART using GPIO-based modem control signals:
+
+   uart4: serial@49042000 {
+   compatible = "ti,omap3-uart";
+   reg = <0x49042000 0x400>;
+   interrupts = <80>;
+   ti,hwmods = "uart4";
+   clock-frequency = <4800>;
+   cts-gpios = < 5 GPIO_ACTIVE_LOW>;
+   rts-gpios = < 6 GPIO_ACTIVE_LOW>;
+   dtr-gpios = < 12 GPIO_ACTIVE_LOW>;
+   dsr-gpios = < 13 GPIO_ACTIVE_LOW>;
+   dcd-gpios = < 14 GPIO_ACTIVE_LOW>;
+   rng-gpios = < 15 GPIO_ACTIVE_LOW>;
+   };
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ebfb0bd5bef5..e59625bdb007 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -11,6 +11,8 @@
 #include 
 #include 
 
+#include "../serial_mctrl_gpio.h"
+
 struct uart_8250_dma {
int (*tx_dma)(struct uart_8250_port *p);
int (*rx_dma)(struct uart_8250_port *p);
@@ -141,12 +143,43 @@ void serial8250_em485_destroy(struct uart_8250_port *p);
 
 static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
 {
+   int mctrl_gpio = 0;
+
serial_out(up, UART_MCR, value);
+
+   if (value & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (value & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio_set(up->gpios, mctrl_gpio);
 }
 
 static inline int serial8250_in_MCR(struct uart_8250_port *up)
 {
-   return serial_in(up, UART_MCR);
+   int mctrl, mctrl_gpio = 0;
+
+   mctrl = serial_in(up, UART_MCR);
+
+   /* save current MCR values */
+   if (mctrl & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (mctrl & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, _gpio);
+
+   if (mctrl_gpio & TIOCM_RTS)
+   mctrl |= UART_MCR_RTS;
+   else
+   mctrl &= ~UART_MCR_RTS;
+
+   if (mctrl_gpio & TIOCM_DTR)
+   mctrl |= UART_MCR_DTR;
+   else
+   mctrl &= ~UART_MCR_DTR;
+
+   return mctrl;
 }
 
 #if defined(__alpha__) && !defined(CONFIG_PCI)
diff --git a/drivers/tty/serial/8250/8250_core.c 
b/drivers/tty/serial/8250/8250_core.c
index e44122

[PATCH 1/2 v2] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI.

Here Mika's comments from 2016-08-09:

"
I noticed that with v4.8-rc1 serial console of some of our Broxton
systems does not work properly anymore. I'm able to see output but input
does not work.

I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
("tty/serial/8250: use mctrl_gpio helpers").

The reason why it fails is that in ACPI we do not have names for GPIOs
(except when _DSD is used) so we use the "idx" to index into _CRS GPIO
resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
UART device in Broxton has following (simplified) ACPI description:

Device (URT4)
{
...
Name (_CRS, ResourceTemplate () {
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003A
}
GpioIo (Exclusive, PullDefault, 0x, 0x, 
IoRestrictionOutputOnly,
"\\_SB.GPO0", 0x00, ResourceConsumer)
{
0x003D
}
})

In this case it finds the first GPIO (0x003A which happens to be RX pin
for that UART), turns it into GPIO which then breaks input for the UART
device. This also breaks systems with bluetooth connected to UART (those
typically have some GPIOs in their _CRS).

Any ideas how to fix this?

We cannot just drop the _CRS index lookup fallback because that would
break many existing machines out there so maybe we can limit this to
only DT enabled machines. Or alternatively probe if the property first
exists before trying to acquire the GPIOs (using
device_property_present()).
"

This patch implements the fix suggested by Mika in his statement above.

Signed-off-by: Stefan Roese 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
v2:
- Include the problem description and analysis from Mika into the commit
  text, as suggested by Greg.

 drivers/tty/serial/serial_mctrl_gpio.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 39ed56214cd3..cac50b20a119 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -116,6 +116,13 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device 
*dev, unsigned int idx)
 
for (i = 0; i < UART_GPIO_MAX; i++) {
enum gpiod_flags flags;
+   char *gpio_str;
+
+   /* Check if GPIO property exists and continue if not */
+   gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
+mctrl_gpios_desc[i].name);
+   if (!device_property_present(dev, gpio_str))
+   continue;
 
if (mctrl_gpios_desc[i].dir_out)
flags = GPIOD_OUT_LOW;
-- 
2.21.0

[PATCH 2/2] tty/serial/8250: use mctrl_gpio helpers

[Stefan Roese]

From: Yegor Yefremov 

This patch permits the usage for GPIOs to control
the CTS/RTS/DTR/DSR/DCD/RI signals.

Signed-off-by: Yegor Yefremov 
Signed-off-by: Greg Kroah-Hartman 
Signed-off-by: Stefan Roese 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Giulio Benetti 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
---
Please note that this patch was already applied before [1]. And later
reverted [2] because it introduced problems on some x86 based boards
(ACPI GPIO related). Here a detailed description of the issue at that
time:

https://lkml.org/lkml/2016/8/9/357
http://www.spinics.net/lists/linux-serial/msg23071.html

This is a re-send of the original patch that was applied at that time.
With patch 1/2 from this series this issue should be fixed now (please
note that I can't test it on such an x86 platform causing these
problems).

Andy (or Mika), perhaps it would be possible for you to test this
patch again, now with patch 1/2 of this series applied as well?
That would be really helpful.

Thanks,
Stefan

[1] 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
[2] 5db4f7f80d16 ("Revert "tty/serial/8250: use mctrl_gpio helpers"")

 .../devicetree/bindings/serial/8250.txt   | 19 ++
 drivers/tty/serial/8250/8250.h| 35 ++-
 drivers/tty/serial/8250/8250_core.c   |  9 +
 drivers/tty/serial/8250/8250_omap.c   | 31 +---
 drivers/tty/serial/8250/8250_port.c   |  7 +++-
 drivers/tty/serial/8250/Kconfig   |  1 +
 include/linux/serial_8250.h   |  1 +
 7 files changed, 88 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.txt 
b/Documentation/devicetree/bindings/serial/8250.txt
index 3cba12f855b7..20d351f268ef 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -53,6 +53,9 @@ Optional properties:
   programmable TX FIFO thresholds.
 - resets : phandle + reset specifier pairs
 - overrun-throttle-ms : how long to pause uart rx when input overrun is 
encountered.
+- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
+  line respectively. It will use specified GPIO instead of the peripheral
+  function pin for the UART feature. If unsure, don't specify this property.
 
 Note:
 * fsl,ns16550:
@@ -74,3 +77,19 @@ Example:
interrupts = <10>;
reg-shift = <2>;
};
+
+Example for OMAP UART using GPIO-based modem control signals:
+
+   uart4: serial@49042000 {
+   compatible = "ti,omap3-uart";
+   reg = <0x49042000 0x400>;
+   interrupts = <80>;
+   ti,hwmods = "uart4";
+   clock-frequency = <4800>;
+   cts-gpios = < 5 GPIO_ACTIVE_LOW>;
+   rts-gpios = < 6 GPIO_ACTIVE_LOW>;
+   dtr-gpios = < 12 GPIO_ACTIVE_LOW>;
+   dsr-gpios = < 13 GPIO_ACTIVE_LOW>;
+   dcd-gpios = < 14 GPIO_ACTIVE_LOW>;
+   rng-gpios = < 15 GPIO_ACTIVE_LOW>;
+   };
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ebfb0bd5bef5..e59625bdb007 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -11,6 +11,8 @@
 #include 
 #include 
 
+#include "../serial_mctrl_gpio.h"
+
 struct uart_8250_dma {
int (*tx_dma)(struct uart_8250_port *p);
int (*rx_dma)(struct uart_8250_port *p);
@@ -141,12 +143,43 @@ void serial8250_em485_destroy(struct uart_8250_port *p);
 
 static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
 {
+   int mctrl_gpio = 0;
+
serial_out(up, UART_MCR, value);
+
+   if (value & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (value & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio_set(up->gpios, mctrl_gpio);
 }
 
 static inline int serial8250_in_MCR(struct uart_8250_port *up)
 {
-   return serial_in(up, UART_MCR);
+   int mctrl, mctrl_gpio = 0;
+
+   mctrl = serial_in(up, UART_MCR);
+
+   /* save current MCR values */
+   if (mctrl & UART_MCR_RTS)
+   mctrl_gpio |= TIOCM_RTS;
+   if (mctrl & UART_MCR_DTR)
+   mctrl_gpio |= TIOCM_DTR;
+
+   mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, _gpio);
+
+   if (mctrl_gpio & TIOCM_RTS)
+   mctrl |= UART_MCR_RTS;
+   else
+   mctrl &= ~UART_MCR_RTS;
+
+   if (mctrl_gpio & TIOCM_DTR)
+   mctrl |= UART_MCR_DTR;
+   else
+   mctrl &= ~UART_MCR_DTR;
+
+   return mctrl;
 }
 
 #if defined(__alpha__) && !defined(CONFIG_PCI)
diff --git a/drivers/tty/serial/8250/8250_core.c 
b/drivers/tty/serial/8250/8250_core.c
index e441221e04b9..ec0c5448c19

[PATCH 1/2] serial: mctrl_gpio: Check if GPIO property exisits before requesting it

[Stefan Roese]

This patch adds a check for the GPIOs property existence, before the
GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
support is added (2nd patch in this patch series) on x86 platforms using
ACPI. Please find a details problem description here:

https://lkml.org/lkml/2016/8/9/357

Signed-off-by: Stefan Roese 
Cc: Mika Westerberg 
Cc: Andy Shevchenko 
Cc: Yegor Yefremov 
Cc: Greg Kroah-Hartman 
Cc: Giulio Benetti 
---
 drivers/tty/serial/serial_mctrl_gpio.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 39ed56214cd3..cac50b20a119 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -116,6 +116,13 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device 
*dev, unsigned int idx)
 
for (i = 0; i < UART_GPIO_MAX; i++) {
enum gpiod_flags flags;
+   char *gpio_str;
+
+   /* Check if GPIO property exists and continue if not */
+   gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
+mctrl_gpios_desc[i].name);
+   if (!device_property_present(dev, gpio_str))
+   continue;
 
if (mctrl_gpios_desc[i].dir_out)
flags = GPIOD_OUT_LOW;
-- 
2.21.0

Re: [PATCH] mtd: cfi_cmdset_0002: dynamically determine the max sectors

[Stefan Roese]

On 22.05.19 02:06, Chris Packham wrote:

Because PPB unlocking unlocks the whole chip cfi_ppb_unlock() needs to
remember the locked status for each sector so it can re-lock the
unaddressed sectors. Dynamically calculate the maximum number of sectors
rather than using a hardcoded value that is too small for larger chips.

Tested with Spansion S29GL01GS11TFI flash device.

Signed-off-by: Chris Packham 


This hardcoded limit always annoyed me at that time, so thanks for
"fixing" this:

Reviewed-by: Stefan Roese 

Thanks,
Stefan

Re: [PATCH] spi: mediatek: Attempt to address style issues in spi-mt7621.c

[Stefan Roese]

On 14.03.19 14:14, Matthias Brugger wrote:



On 14/03/2019 12:37, Armando Miraglia wrote:

Absolutely!


Please don't top post :)



Cheers,
A.

On Thu, Mar 14, 2019 at 12:36 PM Stefan Roese  wrote:

[...]


Would it be possible for you to wait a bit with this minor cleanup?
As I'm preparing a patch to move this driver out of staging right
now. You can definitely follow-up with your cleanup, once this move
is done. Otherwise the move might be delayed even more.



Hm but shouldn't style issues be a criteria for not accepting a move out of
staging?


I would agree, if those style issues where non trivial. In the end we
are talking about one non-optimal identation now.


I think so. You could add Armandos patch in your series or rebase your
series against Greg's tree, once he took the clean-up. Normally Greg is
incredibly fast :)


I should have included the history here to make this more clean. I've
started pulling this driver out of staging a few weeks ago:

https://patchwork.kernel.org/patch/10790537/
...

Here you find Greg's comment, that the style patches should be merged
first before the move out of staging. This is what I worked on after
this first patch series:

https://patchwork.kernel.org/patch/10792455/
...

Now these 9 style issue patches from me have been merged and I would
like to proceed with the driver move.

Thanks,
Stefan

Re: [PATCH] spi: mediatek: Attempt to address style issues in spi-mt7621.c

[Stefan Roese]

Hi Armando,

On 14.03.19 12:13, Armando Miraglia wrote:

My answers are in-line below. BTW bare with me as this is my attempt to get my
feet wet in how to contribute to the linux kernel for my own pleasure and
interest :)

On Wed, Mar 13, 2019 at 03:34:54PM +0300, Dan Carpenter wrote:

On Wed, Mar 13, 2019 at 01:24:04PM +0100, Armando Miraglia wrote:

Running Lindent on the mt7621-spi.c file in drivers/staging I noticed that the
file contained style issues. This change attempts to address such style
problems.



Don't run lindent.  I think checkpatch.pl has a --fix option that might
be better, but once the code is merged then our standard become much
higher for follow up patches.


Signed-off-by: Armando Miraglia 
---
NOTE: resend this patch to include all mainteners listed by get_mantainers.pl.
  drivers/staging/mt7621-spi/spi-mt7621.c | 27 +
  1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c 
b/drivers/staging/mt7621-spi/spi-mt7621.c
index b509f9fe3346..03d53845f8c5 100644
--- a/drivers/staging/mt7621-spi/spi-mt7621.c
+++ b/drivers/staging/mt7621-spi/spi-mt7621.c
@@ -52,14 +52,14 @@
  #define MT7621_LSB_FIRST  BIT(3)
  
  struct mt7621_spi {

-   struct spi_master   *master;
-   void __iomem*base;
-   unsigned intsys_freq;
-   unsigned intspeed;
-   struct clk  *clk;
-   int pending_write;
-
-   struct mt7621_spi_ops   *ops;
+   struct spi_master *master;
+   void __iomem *base;
+   unsigned int sys_freq;
+   unsigned int speed;
+   struct clk *clk;
+   int pending_write;
+
+   struct mt7621_spi_ops *ops;


The original is fine.  I don't encourage people to do fancy indenting
with their local variable declarations inside functions but for a struct
the declarations aren't going to change a lot so people can get fancy
if they want.


Is there an explicit intent to deprecate Lindent in favor of checkpatch.pl
--fix? If one would like to contribute to fixing the tooling for linting which
of the two would be the right target for such an effort?


The problem with a local is if you need to add a new variable then you
have to re-indent a bunch of unrelated lines or have one out of
alignment line.  Most people know this intuitively so they don't get
fancy.


  };
  
  static inline struct mt7621_spi *spidev_to_mt7621_spi(struct spi_device *spi)

@@ -303,7 +303,7 @@ static int mt7621_spi_setup(struct spi_device *spi)
struct mt7621_spi *rs = spidev_to_mt7621_spi(spi);
  
  	if ((spi->max_speed_hz == 0) ||

-   (spi->max_speed_hz > (rs->sys_freq / 2)))
+   (spi->max_speed_hz > (rs->sys_freq / 2)))


Yeah.  Lindent is correct here.


Funny enough, this is something I adjusted manually :)


spi->max_speed_hz = (rs->sys_freq / 2);
  
  	if (spi->max_speed_hz < (rs->sys_freq / 4097)) {

@@ -316,9 +316,10 @@ static int mt7621_spi_setup(struct spi_device *spi)
  }
  
  static const struct of_device_id mt7621_spi_match[] = {

-   { .compatible = "ralink,mt7621-spi" },
+   {.compatible = "ralink,mt7621-spi"},


The original was better.


{},
  };
+
  MODULE_DEVICE_TABLE(of, mt7621_spi_match);


No need for a blank.  These are closely related.


Ack.

  
  static int mt7621_spi_probe(struct platform_device *pdev)

@@ -408,9 +409,9 @@ MODULE_ALIAS("platform:" DRIVER_NAME);
  
  static struct platform_driver mt7621_spi_driver = {

.driver = {
-   .name = DRIVER_NAME,
-   .of_match_table = mt7621_spi_match,
-   },
+  .name = DRIVER_NAME,
+  .of_match_table = mt7621_spi_match,
+  },


The new indenting is very wrong.


Ack. In fact, I was thinking this could be one target to fix the logic in
Lindent to do this appropriately.

I have a process question here: to post a change for the only accepted change I
have in this patch should I send out a new patch?


Would it be possible for you to wait a bit with this minor cleanup?
As I'm preparing a patch to move this driver out of staging right
now. You can definitely follow-up with your cleanup, once this move
is done. Otherwise the move might be delayed even more.

Thanks,
Stefan

Re: [PATCH] spi: mediatek: Attempt to address style issues in spi-mt7621.c

[Stefan Roese]

On 13.03.19 17:46, Matthias Brugger wrote:



On 13/03/2019 17:34, Chuanhong Guo wrote:

Hi!
On Wed, Mar 13, 2019 at 8:28 PM Matthias Brugger  wrote:




On 13/03/2019 13:24, Armando Miraglia wrote:
[...]
Apart from fixing styling issues it would be usefull to see if we can add
support for mt7621 to drivers/spi/spi-mt65xx.c

It's impossible. They are completely different IPs.


Thanks for the info. Do you know the status of the rest of the drivers in 
staging?


Just to inform you. We are using this SPI driver from staging
in one of our customer projects and I will try to move this
driver out of staging into drivers/spi very shortly.

Thanks,
Stefan

Re: [PATCH v3] spi: orion: cosmetics - alias long direct_access variables

[Stefan Roese]

On 13.08.2018 22:54, Kosta Zertsekel wrote:

This change increases the source code readability.
Instead of using `spi->child[cs].direct_access.XXX` use `dir_acc->XXX`.
Instead of using `orion_spi->child[cs].direct_access.vaddr` use `vaddr`.

Signed-off-by: Kosta Zertsekel 
Reviewed-by: Andrew Lunn 
---
  drivers/spi/spi-orion.c | 22 +++---
  1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index d01a6adc726e..184ba91abeee 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -430,6 +430,7 @@ orion_spi_write_read(struct spi_device *spi, struct 
spi_transfer *xfer)
int word_len;
struct orion_spi *orion_spi;
int cs = spi->chip_select;
+   void __iomem *vaddr;
  
  	word_len = spi->bits_per_word;

count = xfer->len;
@@ -440,8 +441,9 @@ orion_spi_write_read(struct spi_device *spi, struct 
spi_transfer *xfer)
 * Use SPI direct write mode if base address is available. Otherwise
 * fall back to PIO mode for this transfer.
 */
-   if ((orion_spi->child[cs].direct_access.vaddr) && (xfer->tx_buf) &&
-   (word_len == 8)) {
+   vaddr = orion_spi->child[cs].direct_access.vaddr;
+
+   if (vaddr && xfer->tx_buf && word_len == 8) {
unsigned int cnt = count / 4;
unsigned int rem = count % 4;
  
@@ -449,13 +451,11 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)

 * Send the TX-data to the SPI device via the direct
 * mapped address window
 */
-   iowrite32_rep(orion_spi->child[cs].direct_access.vaddr,
- xfer->tx_buf, cnt);
+   iowrite32_rep(vaddr, xfer->tx_buf, cnt);
if (rem) {
u32 *buf = (u32 *)xfer->tx_buf;
  
-			iowrite8_rep(orion_spi->child[cs].direct_access.vaddr,

-[cnt], rem);
+   iowrite8_rep(vaddr, [cnt], rem);
}
  
  		return count;

@@ -683,6 +683,7 @@ static int orion_spi_probe(struct platform_device *pdev)
  
  	/* Scan all SPI devices of this controller for direct mapped devices */

for_each_available_child_of_node(pdev->dev.of_node, np) {
+   struct orion_direct_acc *dir_acc;
u32 cs;
  
  		/* Get chip-select number from the "reg" property */

@@ -711,14 +712,13 @@ static int orion_spi_probe(struct platform_device *pdev)
 * This needs to get extended for the direct SPI-NOR / SPI-NAND
 * support, once this gets implemented.
 */
-   spi->child[cs].direct_access.vaddr = devm_ioremap(>dev,
-   r->start,
-   PAGE_SIZE);
-   if (!spi->child[cs].direct_access.vaddr) {
+   dir_acc = >child[cs].direct_access;
+   dir_acc->vaddr = devm_ioremap(>dev, r->start, PAGE_SIZE);
+   if (!dir_acc->vaddr) {
status = -ENOMEM;
goto out_rel_axi_clk;
}
-   spi->child[cs].direct_access.size = PAGE_SIZE;
+   dir_acc->size = PAGE_SIZE;
  
  		dev_info(>dev, "CS%d configured for direct access\n", cs);

}



Reviewed-by: Stefan Roese 

Thanks,
Stefan

Re: [PATCH v3] spi: orion: cosmetics - alias long direct_access variables

[Stefan Roese]

On 13.08.2018 22:54, Kosta Zertsekel wrote:

This change increases the source code readability.
Instead of using `spi->child[cs].direct_access.XXX` use `dir_acc->XXX`.
Instead of using `orion_spi->child[cs].direct_access.vaddr` use `vaddr`.

Signed-off-by: Kosta Zertsekel 
Reviewed-by: Andrew Lunn 
---
  drivers/spi/spi-orion.c | 22 +++---
  1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index d01a6adc726e..184ba91abeee 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -430,6 +430,7 @@ orion_spi_write_read(struct spi_device *spi, struct 
spi_transfer *xfer)
int word_len;
struct orion_spi *orion_spi;
int cs = spi->chip_select;
+   void __iomem *vaddr;
  
  	word_len = spi->bits_per_word;

count = xfer->len;
@@ -440,8 +441,9 @@ orion_spi_write_read(struct spi_device *spi, struct 
spi_transfer *xfer)
 * Use SPI direct write mode if base address is available. Otherwise
 * fall back to PIO mode for this transfer.
 */
-   if ((orion_spi->child[cs].direct_access.vaddr) && (xfer->tx_buf) &&
-   (word_len == 8)) {
+   vaddr = orion_spi->child[cs].direct_access.vaddr;
+
+   if (vaddr && xfer->tx_buf && word_len == 8) {
unsigned int cnt = count / 4;
unsigned int rem = count % 4;
  
@@ -449,13 +451,11 @@ orion_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)

 * Send the TX-data to the SPI device via the direct
 * mapped address window
 */
-   iowrite32_rep(orion_spi->child[cs].direct_access.vaddr,
- xfer->tx_buf, cnt);
+   iowrite32_rep(vaddr, xfer->tx_buf, cnt);
if (rem) {
u32 *buf = (u32 *)xfer->tx_buf;
  
-			iowrite8_rep(orion_spi->child[cs].direct_access.vaddr,

-[cnt], rem);
+   iowrite8_rep(vaddr, [cnt], rem);
}
  
  		return count;

@@ -683,6 +683,7 @@ static int orion_spi_probe(struct platform_device *pdev)
  
  	/* Scan all SPI devices of this controller for direct mapped devices */

for_each_available_child_of_node(pdev->dev.of_node, np) {
+   struct orion_direct_acc *dir_acc;
u32 cs;
  
  		/* Get chip-select number from the "reg" property */

@@ -711,14 +712,13 @@ static int orion_spi_probe(struct platform_device *pdev)
 * This needs to get extended for the direct SPI-NOR / SPI-NAND
 * support, once this gets implemented.
 */
-   spi->child[cs].direct_access.vaddr = devm_ioremap(>dev,
-   r->start,
-   PAGE_SIZE);
-   if (!spi->child[cs].direct_access.vaddr) {
+   dir_acc = >child[cs].direct_access;
+   dir_acc->vaddr = devm_ioremap(>dev, r->start, PAGE_SIZE);
+   if (!dir_acc->vaddr) {
status = -ENOMEM;
goto out_rel_axi_clk;
}
-   spi->child[cs].direct_access.size = PAGE_SIZE;
+   dir_acc->size = PAGE_SIZE;
  
  		dev_info(>dev, "CS%d configured for direct access\n", cs);

}



Reviewed-by: Stefan Roese 

Thanks,
Stefan

Re: [PATCH v6 7/7] PCI: Unify wait for link active into generic pci

[Stefan Roese]

On 19.01.2018 12:10, Oza Pawandeep wrote:
> Clients such as pciehp, dpc are using pcie_wait_link_active, which waits
> till the link becomes active or inactive.
> 
> Made generic function and moved it to drivers/pci/pci.c
> 
> Signed-off-by: Oza Pawandeep 
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c 
> b/drivers/pci/hotplug/pciehp_hpc.c
> index 7bab060..26afeff 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -245,25 +245,12 @@ bool pciehp_check_link_active(struct controller *ctrl)
>   return ret;
>   }
>   
> -static void __pcie_wait_link_active(struct controller *ctrl, bool active)
> +static bool pcie_wait_link_active(struct controller *ctrl)
>   {
> - int timeout = 1000;
> -
> - if (pciehp_check_link_active(ctrl) == active)
> - return;
> - while (timeout > 0) {
> - msleep(10);
> - timeout -= 10;
> - if (pciehp_check_link_active(ctrl) == active)
> - return;
> - }
> - ctrl_dbg(ctrl, "Data Link Layer Link Active not %s in 1000 msec\n",
> - active ? "set" : "cleared");
> -}
> + struct pci_dev *pdev = ctrl_dev(ctrl);
> + bool active = true;
>   
> -static void pcie_wait_link_active(struct controller *ctrl)
> -{
> - __pcie_wait_link_active(ctrl, true);
> + return pci_wait_for_link(pdev, active);
>   }
>   
>   static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 4a7c686..0de83ea 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2805,7 +2805,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
>   pci_read_config_word(dev, pos + PCI_ACS_CTRL, );
>   
>   /* Source Validation */
> - ctrl |= (cap & PCI_ACS_SV);
> +//   ctrl |= (cap & PCI_ACS_SV);

Could it be, that you missed to fix / clean something up here?

Thanks,
Stefan

Re: [PATCH v6 7/7] PCI: Unify wait for link active into generic pci

[Stefan Roese]

On 19.01.2018 12:10, Oza Pawandeep wrote:
> Clients such as pciehp, dpc are using pcie_wait_link_active, which waits
> till the link becomes active or inactive.
> 
> Made generic function and moved it to drivers/pci/pci.c
> 
> Signed-off-by: Oza Pawandeep 
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c 
> b/drivers/pci/hotplug/pciehp_hpc.c
> index 7bab060..26afeff 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -245,25 +245,12 @@ bool pciehp_check_link_active(struct controller *ctrl)
>   return ret;
>   }
>   
> -static void __pcie_wait_link_active(struct controller *ctrl, bool active)
> +static bool pcie_wait_link_active(struct controller *ctrl)
>   {
> - int timeout = 1000;
> -
> - if (pciehp_check_link_active(ctrl) == active)
> - return;
> - while (timeout > 0) {
> - msleep(10);
> - timeout -= 10;
> - if (pciehp_check_link_active(ctrl) == active)
> - return;
> - }
> - ctrl_dbg(ctrl, "Data Link Layer Link Active not %s in 1000 msec\n",
> - active ? "set" : "cleared");
> -}
> + struct pci_dev *pdev = ctrl_dev(ctrl);
> + bool active = true;
>   
> -static void pcie_wait_link_active(struct controller *ctrl)
> -{
> - __pcie_wait_link_active(ctrl, true);
> + return pci_wait_for_link(pdev, active);
>   }
>   
>   static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 4a7c686..0de83ea 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2805,7 +2805,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
>   pci_read_config_word(dev, pos + PCI_ACS_CTRL, );
>   
>   /* Source Validation */
> - ctrl |= (cap & PCI_ACS_SV);
> +//   ctrl |= (cap & PCI_ACS_SV);

Could it be, that you missed to fix / clean something up here?

Thanks,
Stefan

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Stefan Roese]

On 25.08.2017 12:40, Mika Westerberg wrote:

On Fri, Aug 25, 2017 at 01:12:51AM -0700, Bin Meng wrote:

The Intel SPI-NOR driver is dependent on LPC_ICH to get the platform
data. Select it in the Kconfig.

Signed-off-by: Bin Meng 
---

  drivers/mtd/spi-nor/Kconfig | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index bfdfb1e..e998800 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -93,6 +93,7 @@ config SPI_INTEL_SPI_PLATFORM
tristate "Intel PCH/PCU SPI flash platform driver" if EXPERT
depends on X86
select SPI_INTEL_SPI
+   select LPC_ICH


How about

depends on X86 && LPC_ICH

instead?


I prefer Bin's version, as with your patch, the MTD SPI driver will
not be "seen" / selectable, as long as the LPC_ICH support is not
enabled. I've been hunting such dependencies myself a few times and
find them unnecessary burden.

Thanks,
Stefan

Re: [PATCH] spi-nor: intel-spi: Fix Kconfig dependency to LPC_ICH

[Stefan Roese]

On 25.08.2017 12:40, Mika Westerberg wrote:

On Fri, Aug 25, 2017 at 01:12:51AM -0700, Bin Meng wrote:

The Intel SPI-NOR driver is dependent on LPC_ICH to get the platform
data. Select it in the Kconfig.

Signed-off-by: Bin Meng 
---

  drivers/mtd/spi-nor/Kconfig | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index bfdfb1e..e998800 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -93,6 +93,7 @@ config SPI_INTEL_SPI_PLATFORM
tristate "Intel PCH/PCU SPI flash platform driver" if EXPERT
depends on X86
select SPI_INTEL_SPI
+   select LPC_ICH


How about

depends on X86 && LPC_ICH

instead?


I prefer Bin's version, as with your patch, the MTD SPI driver will
not be "seen" / selectable, as long as the LPC_ICH support is not
enabled. I've been hunting such dependencies myself a few times and
find them unnecessary burden.

Thanks,
Stefan

[PATCH v2] irqchip/armada-370-xp: Enable MSI-X support

[Stefan Roese]

Armada XP does not only support MSI, but also MSI-X. This patch sets
the MSI_FLAG_PCI_MSIX flag in the interrupt controller driver which
is the only change necessary to enable MSI-X support on this SoC. As
the Linux PCI MSI-X infrastructure takes care of writing the data and
address structures into the BAR specified by the MSI-X controller.

Signed-off-by: Stefan Roese <s...@denx.de>
Reviewed-by: Thomas Petazzoni <thomas.petazz...@free-electrons.com>
Cc: Marc Zyngier <marc.zyng...@arm.com>
Cc: Jason Cooper <ja...@lakedaemon.net>
Cc: Thomas Gleixner <t...@linutronix.de>
Cc: Bjorn Helgaas <bhelg...@google.com>
Cc: Gregory CLEMENT <gregory.clem...@free-electrons.com>
---
v2:
- Added Reviewed-by tag from Thomas
- Added usual irqchip maintainers

 drivers/irqchip/irq-armada-370-xp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c 
b/drivers/irqchip/irq-armada-370-xp.c
index 33982cbd8a57..b17039ed8735 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -124,7 +124,7 @@ static struct irq_chip armada_370_xp_msi_irq_chip = {
 
 static struct msi_domain_info armada_370_xp_msi_domain_info = {
.flags  = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
-  MSI_FLAG_MULTI_PCI_MSI),
+  MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX),
.chip   = _370_xp_msi_irq_chip,
 };
 
-- 
2.12.2

[PATCH v2] irqchip/armada-370-xp: Enable MSI-X support

[Stefan Roese]

Armada XP does not only support MSI, but also MSI-X. This patch sets
the MSI_FLAG_PCI_MSIX flag in the interrupt controller driver which
is the only change necessary to enable MSI-X support on this SoC. As
the Linux PCI MSI-X infrastructure takes care of writing the data and
address structures into the BAR specified by the MSI-X controller.

Signed-off-by: Stefan Roese 
Reviewed-by: Thomas Petazzoni 
Cc: Marc Zyngier 
Cc: Jason Cooper 
Cc: Thomas Gleixner 
Cc: Bjorn Helgaas 
Cc: Gregory CLEMENT 
---
v2:
- Added Reviewed-by tag from Thomas
- Added usual irqchip maintainers

 drivers/irqchip/irq-armada-370-xp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c 
b/drivers/irqchip/irq-armada-370-xp.c
index 33982cbd8a57..b17039ed8735 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -124,7 +124,7 @@ static struct irq_chip armada_370_xp_msi_irq_chip = {
 
 static struct msi_domain_info armada_370_xp_msi_domain_info = {
.flags  = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
-  MSI_FLAG_MULTI_PCI_MSI),
+  MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX),
.chip   = _370_xp_msi_irq_chip,
 };
 
-- 
2.12.2

Re: [PATCH 00/39] ARM: dts: mvebu: Fix license text

[Stefan Roese]

On 14.12.2016 23:37, Alexandre Belloni wrote:

When the license was switched to dual GPLv2/X11, the text that was used
was missing a few characters. Fix that now.

I'll let the maintainers decide whether this change requires an ack of
every contributors. It has been separated with that in mind if
necessary.

Cc: Andrew Andrianov <and...@ncrmnt.org>
Cc: Arnaud Ebalard <a...@natisbad.org>
Cc: Arnd Bergmann <a...@arndb.de>
Cc: Ben Dooks <ben.do...@codethink.co.uk>
Cc: Benjamin Cama <ben...@dolka.fr>
Cc: Benoit Masson <ya...@perenite.com>
Cc: Ben Peddell <klightsp...@killerwolves.net>
Cc: Boris Brezillon <boris.brezil...@free-electrons.com>
Cc: Chris Packham <chris.pack...@alliedtelesis.co.nz>
Cc: Ezequiel Garcia <ezequiel.gar...@free-electrons.com>
Cc: Florian Fainelli <flor...@openwrt.org>
Cc: Geert Uytterhoeven <geert+rene...@glider.be>
Cc: Greg Ungerer <g...@uclinux.org>
Cc: Grzegorz Jaszczyk <j...@semihalf.com>
Cc: Heikki Krogerus <heikki.kroge...@linux.intel.com>
Cc: Imre Kaloz <ka...@openwrt.org>
Cc: Kevin Hilman <khil...@linaro.org>
Cc: Lior Amsalem <al...@marvell.com>
Cc: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
Cc: Marcin Wojtas <m...@semihalf.com>
Cc: Mario Lange <mario_la...@gmx.net>
Cc: Maxime Ripard <maxime.rip...@free-electrons.com>
Cc: Nadav Haklai <nad...@marvell.com>
Cc: Nobuhiro Iwamatsu <iwama...@nigauri.org>
Cc: Paul Bolle <pebo...@tiscali.nl>
Cc: Philipp Zabel <p.za...@pengutronix.de>
Cc: Rafał Miłecki <zaj...@gmail.com>
Cc: Roger Shimizu <rogershim...@gmail.com>
Cc: Russell King <rmk+ker...@arm.linux.org.uk>
Cc: Ryan Press <r...@presslab.us>
Cc: Sebastian Hesselbarth <sebastian.hesselba...@gmail.com>
Cc: Simon Baatz <gmbno...@gmail.com>
Cc: Simon Guinot <simon.gui...@sequanux.org>
Cc: Stefan Roese <s...@denx.de>


For the complete patch series:

Acked-by: Stefan Roese <s...@denx.de>

Thanks,
Stefan

Re: [PATCH 00/39] ARM: dts: mvebu: Fix license text

[Stefan Roese]

On 14.12.2016 23:37, Alexandre Belloni wrote:

When the license was switched to dual GPLv2/X11, the text that was used
was missing a few characters. Fix that now.

I'll let the maintainers decide whether this change requires an ack of
every contributors. It has been separated with that in mind if
necessary.

Cc: Andrew Andrianov 
Cc: Arnaud Ebalard 
Cc: Arnd Bergmann 
Cc: Ben Dooks 
Cc: Benjamin Cama 
Cc: Benoit Masson 
Cc: Ben Peddell 
Cc: Boris Brezillon 
Cc: Chris Packham 
Cc: Ezequiel Garcia 
Cc: Florian Fainelli 
Cc: Geert Uytterhoeven 
Cc: Greg Ungerer 
Cc: Grzegorz Jaszczyk 
Cc: Heikki Krogerus 
Cc: Imre Kaloz 
Cc: Kevin Hilman 
Cc: Lior Amsalem 
Cc: Lorenzo Pieralisi 
Cc: Marcin Wojtas 
Cc: Mario Lange 
Cc: Maxime Ripard 
Cc: Nadav Haklai 
Cc: Nobuhiro Iwamatsu 
Cc: Paul Bolle 
Cc: Philipp Zabel 
Cc: Rafał Miłecki 
Cc: Roger Shimizu 
Cc: Russell King 
Cc: Ryan Press 
Cc: Sebastian Hesselbarth 
Cc: Simon Baatz 
Cc: Simon Guinot 
Cc: Stefan Roese 


For the complete patch series:

Acked-by: Stefan Roese 

Thanks,
Stefan

Re: [PATCH v2] clocksource/drivers/time-armada-370-xp: Fix the clock reference

[Stefan Roese]

On 10.08.2016 15:14, Gregory CLEMENT wrote:

While converting the init function to return an error, the wrong clock
was get. This lead to wrong clock rate and slow down the kernel. For
example, before the patch a typical boot was around 15s after it was 1
minute slower.

Fixes: 12549e27c63c ("clocksource/drivers/time-armada-370-xp: Convert init function 
to return error")

Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>


Tested-by: Stefan Roese <s...@denx.de>

Thanks,
Stefan

Re: [PATCH v2] clocksource/drivers/time-armada-370-xp: Fix the clock reference

[Stefan Roese]

On 10.08.2016 15:14, Gregory CLEMENT wrote:

While converting the init function to return an error, the wrong clock
was get. This lead to wrong clock rate and slow down the kernel. For
example, before the patch a typical boot was around 15s after it was 1
minute slower.

Fixes: 12549e27c63c ("clocksource/drivers/time-armada-370-xp: Convert init function 
to return error")

Signed-off-by: Gregory CLEMENT 


Tested-by: Stefan Roese 

Thanks,
Stefan

Re: [mtd-next:master 30/33] drivers/mtd/spi-nor/cadence-quadspi.c:529:4: error: implicit declaration of function 'readsl'

[Stefan Roese]

On 18.07.2016 22:20, Brian Norris wrote:

On Tue, Jul 19, 2016 at 03:43:17AM +0800, kbuild test robot wrote:

tree:   git://git.infradead.org/linux-mtd-next.git master
head:   f78921b9020c510ed222a6c2402e2aa126432415
commit: 140623410536905fa6ab737b625decfde6c64a72 [30/33] mtd: spi-nor: Add 
driver for Cadence Quad SPI Flash Controller
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
 git checkout 140623410536905fa6ab737b625decfde6c64a72
 # save the attached .config to linux build tree
 make ARCH=x86_64

All errors (new ones prefixed by >>):

drivers/mtd/spi-nor/cadence-quadspi.c: In function 
'cqspi_indirect_read_execute':

drivers/mtd/spi-nor/cadence-quadspi.c:529:4: error: implicit declaration of 
function 'readsl' [-Werror=implicit-function-declaration]

readsl(ahb_base, rxbuf, DIV_ROUND_UP(bytes_to_read, 4));
^~
drivers/mtd/spi-nor/cadence-quadspi.c: In function 
'cqspi_indirect_write_execute':

drivers/mtd/spi-nor/cadence-quadspi.c:613:3: error: implicit declaration of 
function 'writesl' [-Werror=implicit-function-declaration]

   writesl(cqspi->ahb_base, txbuf, DIV_ROUND_UP(write_bytes, 4));
   ^~~
cc1: some warnings being treated as errors


Hmm, does x86 not define readsl()/writesl()? I can never tell what
accessors are supposed to be "standard" across architectures.

Either we need to drop the COMPILE_TEST or maybe make it (!X86 &&
COMPILE_TEST).


iowrite32_rep() etc should work for x86 as well.

Thanks,
Stefan

Re: [mtd-next:master 30/33] drivers/mtd/spi-nor/cadence-quadspi.c:529:4: error: implicit declaration of function 'readsl'

[Stefan Roese]

On 18.07.2016 22:20, Brian Norris wrote:

On Tue, Jul 19, 2016 at 03:43:17AM +0800, kbuild test robot wrote:

tree:   git://git.infradead.org/linux-mtd-next.git master
head:   f78921b9020c510ed222a6c2402e2aa126432415
commit: 140623410536905fa6ab737b625decfde6c64a72 [30/33] mtd: spi-nor: Add 
driver for Cadence Quad SPI Flash Controller
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
 git checkout 140623410536905fa6ab737b625decfde6c64a72
 # save the attached .config to linux build tree
 make ARCH=x86_64

All errors (new ones prefixed by >>):

drivers/mtd/spi-nor/cadence-quadspi.c: In function 
'cqspi_indirect_read_execute':

drivers/mtd/spi-nor/cadence-quadspi.c:529:4: error: implicit declaration of 
function 'readsl' [-Werror=implicit-function-declaration]

readsl(ahb_base, rxbuf, DIV_ROUND_UP(bytes_to_read, 4));
^~
drivers/mtd/spi-nor/cadence-quadspi.c: In function 
'cqspi_indirect_write_execute':

drivers/mtd/spi-nor/cadence-quadspi.c:613:3: error: implicit declaration of 
function 'writesl' [-Werror=implicit-function-declaration]

   writesl(cqspi->ahb_base, txbuf, DIV_ROUND_UP(write_bytes, 4));
   ^~~
cc1: some warnings being treated as errors


Hmm, does x86 not define readsl()/writesl()? I can never tell what
accessors are supposed to be "standard" across architectures.

Either we need to drop the COMPILE_TEST or maybe make it (!X86 &&
COMPILE_TEST).


iowrite32_rep() etc should work for x86 as well.

Thanks,
Stefan

Re: SPDX-License-Identifier

[Stefan Roese]

On 02.02.2015 17:06, Greg Kroah-Hartman wrote:

On Mon, Feb 02, 2015 at 04:43:14PM +0100, Stefan Roese wrote:

On 21.02.2014 17:18, Michal Simek wrote:

On 02/21/2014 05:12 PM, Felipe Balbi wrote:

On Fri, Feb 21, 2014 at 05:04:26PM +0100, Michal Simek wrote:

On 02/21/2014 05:04 PM, Greg Kroah-Hartman wrote:

On Fri, Feb 21, 2014 at 07:38:16AM +0100, Michal Simek wrote:

BTW: u-boot started to use SPDX-License-Identifier
which will be nice to start to use.


I agree, feel free to start sending patches to use this type of
identifier for drivers.


But we probably need to add that Licenses to one location.
Documentation/Licenses?


Just add to the drivers themselves, just like u-boot is doing. A simple:

$ git grep -e SPDX-License-Identifier

will tell you filename and license. Or did I misunderstand your question ?


But for doing this it is probably necessary to have location where
you will place that licenses and you will explain what it is how
it is done by Wolfgang in this commit.

http://git.denx.de/?p=u-boot.git;a=commitdiff;h=eca3aeb352c964bdb28b8e191d6326370245e03f

Then yes, adding one line is enough.

I would like to revive this thread regarding SPDX license identifiers in the
Linux kernel. And check how to best start / proceed with the integration
now.


Why do you want to do this?


The main idea behind these SPDX license tags in the source files is, 
that it makes license clearing for projects easier. As those tags 
simplify the automated tools that scan all source files of projects, in 
this case the Linux kernel. One of the problems with the current 
licenses in the files is, that even the same licenses are referred to by 
a number of slightly varying text blocks (full, abbreviated, different 
indentation, line wrapping and/or white space, with obsolete address 
information, ...) which makes automatic processing a nightmare.


Please note that we don't want to "disturb" any kernel developers in 
their work with this SPDX license ID addition. The licenses will not be 
changed in any way. We only want to make life easier for users that need 
to run such automated license clearing tools on the source code that 
they embed / ship / deploy. And this one additional line in the header 
is here definitely helpful.



Greg, if you are open to patches adding this one-line SPDX license tag to
the source code, how should I begin with such a task in your opinion? Should
I add those tags for just one driver directory (e.g. drivers/base/*) first?
And send those patches to the list(s) for review. Or do you have other
suggestions on how to start with this task?


Is one tag per directory sufficient?  Is one tag per file sufficient?
How about one tag per package?  If package, then isn't a single tag for
the whole kernel source tree sufficient, as we all know the overall
license for the kernel source tree.


We really need one tag per file. Of course the resulting license for the 
Linux kernel is clear. But there are many things to take into account 
here. Some of them are (I'm not telling you something new, I know - just 
a summary of arguments):


- The Linux source code is not homogeneous and neither perfect nor
  without errors. Who ensures that all parts of the kernel really
  are GPLv2 compatible?

- Some parts of the Linux source code are also used by other projects.
  Or are derived from other projects. Because of this they are
  explicitly licensed under different licenses than the GPLv2
  (compatible to it though of course). Or are dual-licensed. So that
  they can be used by these other projects.

For example "Documentation/SubmittingDrivers" mentions:

The code must be released to us under the
GNU General Public License. We don't insist on any kind
of exclusive GPL licensing, and if you wish the driver
to be useful to other communities such as BSD you may well
wish to release under multiple licenses.
See accepted licenses at include/linux/module.h

Because of this it is really important to know the exact license(s) for 
each and every file. And they can vary very much. Here some examples:


GPL v3 or later:

Documentation/filesystems/cifs/winucase_convert.pl
scripts/dtc/dtc-parser.tab.c_shipped
scripts/dtc/dtc-parser.tab.h_shipped
scripts/kconfig/zconf.tab.c_shipped
scripts/genksyms/parse.tab.h_shipped
scripts/genksyms/parse.tab.c_shipped
drivers/staging/lustre/lustre/include/lustre_dlm_flags.h

So there seems to be problem with this lustre code.

Dual BSD/GPL:

crypto/aes_generic.c
crypto/cts.c
crypto/fcrypt.c
drivers/block/skd_main.c
drivers/block/xen-blkback/blkback.c
...

Dual MIT/GPL:

lib/glob.c

Dual MPL/GPL:

drivers/ide/ide-cs.c
drivers/isdn/hisax/elsa_cs.c
drivers/isdn/hisax/sedlbauer_cs.c
drivers/mtd/ftl.c
drivers/net/ethernet/xircom/xirc2ps_cs.c
...

Re: SPDX-License-Identifier

[Stefan Roese]

On 02.02.2015 17:06, Greg Kroah-Hartman wrote:

On Mon, Feb 02, 2015 at 04:43:14PM +0100, Stefan Roese wrote:

On 21.02.2014 17:18, Michal Simek wrote:

On 02/21/2014 05:12 PM, Felipe Balbi wrote:

On Fri, Feb 21, 2014 at 05:04:26PM +0100, Michal Simek wrote:

On 02/21/2014 05:04 PM, Greg Kroah-Hartman wrote:

On Fri, Feb 21, 2014 at 07:38:16AM +0100, Michal Simek wrote:

BTW: u-boot started to use SPDX-License-Identifier
which will be nice to start to use.


I agree, feel free to start sending patches to use this type of
identifier for drivers.


But we probably need to add that Licenses to one location.
Documentation/Licenses?


Just add to the drivers themselves, just like u-boot is doing. A simple:

$ git grep -e SPDX-License-Identifier

will tell you filename and license. Or did I misunderstand your question ?


But for doing this it is probably necessary to have location where
you will place that licenses and you will explain what it is how
it is done by Wolfgang in this commit.

http://git.denx.de/?p=u-boot.git;a=commitdiff;h=eca3aeb352c964bdb28b8e191d6326370245e03f

Then yes, adding one line is enough.

I would like to revive this thread regarding SPDX license identifiers in the
Linux kernel. And check how to best start / proceed with the integration
now.


Why do you want to do this?


The main idea behind these SPDX license tags in the source files is, 
that it makes license clearing for projects easier. As those tags 
simplify the automated tools that scan all source files of projects, in 
this case the Linux kernel. One of the problems with the current 
licenses in the files is, that even the same licenses are referred to by 
a number of slightly varying text blocks (full, abbreviated, different 
indentation, line wrapping and/or white space, with obsolete address 
information, ...) which makes automatic processing a nightmare.


Please note that we don't want to disturb any kernel developers in 
their work with this SPDX license ID addition. The licenses will not be 
changed in any way. We only want to make life easier for users that need 
to run such automated license clearing tools on the source code that 
they embed / ship / deploy. And this one additional line in the header 
is here definitely helpful.



Greg, if you are open to patches adding this one-line SPDX license tag to
the source code, how should I begin with such a task in your opinion? Should
I add those tags for just one driver directory (e.g. drivers/base/*) first?
And send those patches to the list(s) for review. Or do you have other
suggestions on how to start with this task?


Is one tag per directory sufficient?  Is one tag per file sufficient?
How about one tag per package?  If package, then isn't a single tag for
the whole kernel source tree sufficient, as we all know the overall
license for the kernel source tree.


We really need one tag per file. Of course the resulting license for the 
Linux kernel is clear. But there are many things to take into account 
here. Some of them are (I'm not telling you something new, I know - just 
a summary of arguments):


- The Linux source code is not homogeneous and neither perfect nor
  without errors. Who ensures that all parts of the kernel really
  are GPLv2 compatible?

- Some parts of the Linux source code are also used by other projects.
  Or are derived from other projects. Because of this they are
  explicitly licensed under different licenses than the GPLv2
  (compatible to it though of course). Or are dual-licensed. So that
  they can be used by these other projects.

For example Documentation/SubmittingDrivers mentions:

The code must be released to us under the
GNU General Public License. We don't insist on any kind
of exclusive GPL licensing, and if you wish the driver
to be useful to other communities such as BSD you may well
wish to release under multiple licenses.
See accepted licenses at include/linux/module.h

Because of this it is really important to know the exact license(s) for 
each and every file. And they can vary very much. Here some examples:


GPL v3 or later:

Documentation/filesystems/cifs/winucase_convert.pl
scripts/dtc/dtc-parser.tab.c_shipped
scripts/dtc/dtc-parser.tab.h_shipped
scripts/kconfig/zconf.tab.c_shipped
scripts/genksyms/parse.tab.h_shipped
scripts/genksyms/parse.tab.c_shipped
drivers/staging/lustre/lustre/include/lustre_dlm_flags.h

So there seems to be problem with this lustre code.

Dual BSD/GPL:

crypto/aes_generic.c
crypto/cts.c
crypto/fcrypt.c
drivers/block/skd_main.c
drivers/block/xen-blkback/blkback.c
...

Dual MIT/GPL:

lib/glob.c

Dual MPL/GPL:

drivers/ide/ide-cs.c
drivers/isdn/hisax/elsa_cs.c
drivers/isdn/hisax/sedlbauer_cs.c
drivers/mtd/ftl.c
drivers/net/ethernet/xircom/xirc2ps_cs.c
...

and so on...

So we have

Re: SPDX-License-Identifier

[Stefan Roese]

On 21.02.2014 17:18, Michal Simek wrote:

On 02/21/2014 05:12 PM, Felipe Balbi wrote:

On Fri, Feb 21, 2014 at 05:04:26PM +0100, Michal Simek wrote:

On 02/21/2014 05:04 PM, Greg Kroah-Hartman wrote:

On Fri, Feb 21, 2014 at 07:38:16AM +0100, Michal Simek wrote:

BTW: u-boot started to use SPDX-License-Identifier
which will be nice to start to use.


I agree, feel free to start sending patches to use this type of
identifier for drivers.


But we probably need to add that Licenses to one location.
Documentation/Licenses?


Just add to the drivers themselves, just like u-boot is doing. A simple:

$ git grep -e SPDX-License-Identifier

will tell you filename and license. Or did I misunderstand your question ?


But for doing this it is probably necessary to have location where
you will place that licenses and you will explain what it is how
it is done by Wolfgang in this commit.

http://git.denx.de/?p=u-boot.git;a=commitdiff;h=eca3aeb352c964bdb28b8e191d6326370245e03f

Then yes, adding one line is enough.
I would like to revive this thread regarding SPDX license identifiers in 
the Linux kernel. And check how to best start / proceed with the 
integration now.


Greg, if you are open to patches adding this one-line SPDX license tag 
to the source code, how should I begin with such a task in your opinion? 
Should I add those tags for just one driver directory (e.g. 
drivers/base/*) first? And send those patches to the list(s) for review. 
Or do you have other suggestions on how to start with this task?


Any comments and / or suggestions are really appreciated. Thanks!

Thanks,
Stefan
--
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: SPDX-License-Identifier

[Stefan Roese]

On 21.02.2014 17:18, Michal Simek wrote:

On 02/21/2014 05:12 PM, Felipe Balbi wrote:

On Fri, Feb 21, 2014 at 05:04:26PM +0100, Michal Simek wrote:

On 02/21/2014 05:04 PM, Greg Kroah-Hartman wrote:

On Fri, Feb 21, 2014 at 07:38:16AM +0100, Michal Simek wrote:

BTW: u-boot started to use SPDX-License-Identifier
which will be nice to start to use.


I agree, feel free to start sending patches to use this type of
identifier for drivers.


But we probably need to add that Licenses to one location.
Documentation/Licenses?


Just add to the drivers themselves, just like u-boot is doing. A simple:

$ git grep -e SPDX-License-Identifier

will tell you filename and license. Or did I misunderstand your question ?


But for doing this it is probably necessary to have location where
you will place that licenses and you will explain what it is how
it is done by Wolfgang in this commit.

http://git.denx.de/?p=u-boot.git;a=commitdiff;h=eca3aeb352c964bdb28b8e191d6326370245e03f

Then yes, adding one line is enough.
I would like to revive this thread regarding SPDX license identifiers in 
the Linux kernel. And check how to best start / proceed with the 
integration now.


Greg, if you are open to patches adding this one-line SPDX license tag 
to the source code, how should I begin with such a task in your opinion? 
Should I add those tags for just one driver directory (e.g. 
drivers/base/*) first? And send those patches to the list(s) for review. 
Or do you have other suggestions on how to start with this task?


Any comments and / or suggestions are really appreciated. Thanks!

Thanks,
Stefan
--
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] Lattice ECP3 FPGA: Correct endianness

[Stefan Roese]

On 04.07.2014 15:11, Jean-Michel Hautbois wrote:

2014-07-03 18:12 GMT+02:00 Joe Perches :

trivial:


diff --git a/drivers/misc/lattice-ecp3-config.c

[]

@@ -165,8 +165,8 @@ static void firmware_load(const struct firmware
*fw, void *context)

  txbuf[0] = FPGA_CMD_READ_STATUS;
  ret = spi_write_then_read(spi, txbuf, 8, rxbuf, rx_len);
-dev_dbg(>dev, "FPGA Status=%08x\n", *(u32 *)[4]);
-status = *(u32 *)[4];
+dev_dbg(>dev, "FPGA Status=%08x\n", be32_to_cpu(*(u32 *)[4]));
+status = be32_to_cpu(*(u32 *)[4]);


This should emit a sparse error.
It'd be simpler as:

 status = be32_to_cpu(*(__be32 *)[4]);
 dev_dbg(>dev, "FPGA Status=%08x\n", status);




OK, do you want me to send a new patch including this modification ?


Yes. Please send a v2 patch version. You can add my "Acked-by:.." to the 
new version.


Thanks,
Stefan

--
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: Lattice ECP3 load firmware

[Stefan Roese]

Hi Jean-Michel,

On 04.07.2014 10:24, Jean-Michel Hautbois wrote:

I noticed you (Stefan) are using request_firmware_nowait() call. This
means user needs to explicitly call it using

$ echo 1 > /sys/class/firmware/lattice-ecp3.0/loading
$ cat lattice-ecp3.bit > /sys/class/firmware/lattice-ecp3.0/data
$ echo 0 > /sys/class/firmware/lattice-ecp3.0/loading

Or did I miss something ?


No, thats correct. This is how we use it on our platform.


I would like to have it load the firmware bitstream when booting if
there is a specific .bit file in /lib/firmware. Maybe don't you want
to have this behaviour, though... ?


No. We don't want this. Please don't ask me about the details. Its quite 
a while ago that I worked on this platform. And I don't have access to 
this platform any more.


Thanks,
Stefan

--
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: Lattice ECP3 load firmware

[Stefan Roese]

Hi Jean-Michel,

On 04.07.2014 10:24, Jean-Michel Hautbois wrote:

I noticed you (Stefan) are using request_firmware_nowait() call. This
means user needs to explicitly call it using

$ echo 1  /sys/class/firmware/lattice-ecp3.0/loading
$ cat lattice-ecp3.bit  /sys/class/firmware/lattice-ecp3.0/data
$ echo 0  /sys/class/firmware/lattice-ecp3.0/loading

Or did I miss something ?


No, thats correct. This is how we use it on our platform.


I would like to have it load the firmware bitstream when booting if
there is a specific .bit file in /lib/firmware. Maybe don't you want
to have this behaviour, though... ?


No. We don't want this. Please don't ask me about the details. Its quite 
a while ago that I worked on this platform. And I don't have access to 
this platform any more.


Thanks,
Stefan

--
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] Lattice ECP3 FPGA: Correct endianness

[Stefan Roese]

On 04.07.2014 15:11, Jean-Michel Hautbois wrote:

2014-07-03 18:12 GMT+02:00 Joe Perches j...@perches.com:

trivial:


diff --git a/drivers/misc/lattice-ecp3-config.c

[]

@@ -165,8 +165,8 @@ static void firmware_load(const struct firmware
*fw, void *context)

  txbuf[0] = FPGA_CMD_READ_STATUS;
  ret = spi_write_then_read(spi, txbuf, 8, rxbuf, rx_len);
-dev_dbg(spi-dev, FPGA Status=%08x\n, *(u32 *)rxbuf[4]);
-status = *(u32 *)rxbuf[4];
+dev_dbg(spi-dev, FPGA Status=%08x\n, be32_to_cpu(*(u32 *)rxbuf[4]));
+status = be32_to_cpu(*(u32 *)rxbuf[4]);


This should emit a sparse error.
It'd be simpler as:

 status = be32_to_cpu(*(__be32 *)rxbuf[4]);
 dev_dbg(spi-dev, FPGA Status=%08x\n, status);




OK, do you want me to send a new patch including this modification ?


Yes. Please send a v2 patch version. You can add my Acked-by:.. to the 
new version.


Thanks,
Stefan

--
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] Lattice ECP3 FPGA: Correct endianness

[Stefan Roese]

On 03.07.2014 17:54, Jean-Michel Hautbois wrote:
> This patch corrects three big/little endian issues. Tested on i.MX6.
> 
> From: Jean-Michel Hautbois 
> Date: Thu, 3 Jul 2014 17:49:47 +0200
> Subject: [PATCH] Endianness corrections
> 
> ---
>   drivers/misc/lattice-ecp3-config.c | 8 
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/lattice-ecp3-config.c
> b/drivers/misc/lattice-ecp3-config.c
> index bb26f08..23d5c01 100644
> --- a/drivers/misc/lattice-ecp3-config.c
> +++ b/drivers/misc/lattice-ecp3-config.c
> @@ -93,7 +93,7 @@ static void firmware_load(const struct firmware *fw,
> void *context)
>   txbuf[0] = FPGA_CMD_READ_ID;
>   ret = spi_write_then_read(spi, txbuf, 8, rxbuf, rx_len);
>   dev_dbg(>dev, "FPGA JTAG ID=%08x\n", *(u32 *)[4]);
> -jedec_id = *(u32 *)[4];
> +jedec_id = be32_to_cpu(*(u32 *)[4]);
> 
>   for (i = 0; i < ARRAY_SIZE(ecp3_dev); i++) {
>   if (jedec_id == ecp3_dev[i].jedec_id)
> @@ -142,7 +142,7 @@ static void firmware_load(const struct firmware
> *fw, void *context)
>   for (i = 0; i < FPGA_CLEAR_LOOP_COUNT; i++) {
>   txbuf[0] = FPGA_CMD_READ_STATUS;
>   ret = spi_write_then_read(spi, txbuf, 8, rxbuf, rx_len);
> -status = *(u32 *)[4];
> +status = be32_to_cpu(*(u32 *)[4]);
>   if (status == FPGA_STATUS_CLEARED)
>   break;
> 
> @@ -165,8 +165,8 @@ static void firmware_load(const struct firmware
> *fw, void *context)
> 
>   txbuf[0] = FPGA_CMD_READ_STATUS;
>   ret = spi_write_then_read(spi, txbuf, 8, rxbuf, rx_len);
> -dev_dbg(>dev, "FPGA Status=%08x\n", *(u32 *)[4]);
> -status = *(u32 *)[4];
> +dev_dbg(>dev, "FPGA Status=%08x\n", be32_to_cpu(*(u32 *)[4]));
> +status = be32_to_cpu(*(u32 *)[4]);

I know you didn't introduce this, but this re-ordering does look better:

+status = be32_to_cpu(*(u32 *)[4]);
+dev_dbg(>dev, "FPGA Status=%08x\n", status);

Other than that:

Acked-by: Stefan Roese 

Thanks,
Stefan

--
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: Lattice ECP3 FPGA with i.MX6

[Stefan Roese]

On 03.07.2014 14:37, Jean-Michel Hautbois wrote:

I have a board, with a Freescale i.MX6 chip and a ECP3-35 FPGA on SPI.
I tried to load the firmware using the lattice-ecp3-config driver, but
it fails with this error :

lattice-ecp3 spi32766.3: FPGA bitstream configuration driver registered
lattice-ecp3 spi32766.3: Error: No supported FPGA detected (JEDEC_ID=808004c2)!

In the driver, the id is :

#define ID_ECP3_35 0xc2048080

Obviously, there is a big/little endian issue... Do I need to instruct
the device-tree in a specific way in order to get the bus in the
correct order ? Or is this a known issue maybe ?


No. This driver was implemented and tested in a MPC5200 system. Most 
likely I missed some endian issues as you already noticed. I suggest you 
start with looking at this line:


jedec_id = *(u32 *)[4];

And add some endian functions here, e.g. be32_to_cpu(). This might help 
with the detection. But other endian related issues might still be 
present in other parts of the driver as well.


HTP.

Thanks,
Stefan

--
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: Lattice ECP3 FPGA with i.MX6

[Stefan Roese]

On 03.07.2014 14:37, Jean-Michel Hautbois wrote:

I have a board, with a Freescale i.MX6 chip and a ECP3-35 FPGA on SPI.
I tried to load the firmware using the lattice-ecp3-config driver, but
it fails with this error :

lattice-ecp3 spi32766.3: FPGA bitstream configuration driver registered
lattice-ecp3 spi32766.3: Error: No supported FPGA detected (JEDEC_ID=808004c2)!

In the driver, the id is :

#define ID_ECP3_35 0xc2048080

Obviously, there is a big/little endian issue... Do I need to instruct
the device-tree in a specific way in order to get the bus in the
correct order ? Or is this a known issue maybe ?


No. This driver was implemented and tested in a MPC5200 system. Most 
likely I missed some endian issues as you already noticed. I suggest you 
start with looking at this line:


jedec_id = *(u32 *)rxbuf[4];

And add some endian functions here, e.g. be32_to_cpu(). This might help 
with the detection. But other endian related issues might still be 
present in other parts of the driver as well.


HTP.

Thanks,
Stefan

--
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] Lattice ECP3 FPGA: Correct endianness

[Stefan Roese]

On 03.07.2014 17:54, Jean-Michel Hautbois wrote:
 This patch corrects three big/little endian issues. Tested on i.MX6.
 
 From: Jean-Michel Hautbois jean-michel.hautb...@vodalys.com
 Date: Thu, 3 Jul 2014 17:49:47 +0200
 Subject: [PATCH] Endianness corrections
 
 ---
   drivers/misc/lattice-ecp3-config.c | 8 
   1 file changed, 4 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/misc/lattice-ecp3-config.c
 b/drivers/misc/lattice-ecp3-config.c
 index bb26f08..23d5c01 100644
 --- a/drivers/misc/lattice-ecp3-config.c
 +++ b/drivers/misc/lattice-ecp3-config.c
 @@ -93,7 +93,7 @@ static void firmware_load(const struct firmware *fw,
 void *context)
   txbuf[0] = FPGA_CMD_READ_ID;
   ret = spi_write_then_read(spi, txbuf, 8, rxbuf, rx_len);
   dev_dbg(spi-dev, FPGA JTAG ID=%08x\n, *(u32 *)rxbuf[4]);
 -jedec_id = *(u32 *)rxbuf[4];
 +jedec_id = be32_to_cpu(*(u32 *)rxbuf[4]);
 
   for (i = 0; i  ARRAY_SIZE(ecp3_dev); i++) {
   if (jedec_id == ecp3_dev[i].jedec_id)
 @@ -142,7 +142,7 @@ static void firmware_load(const struct firmware
 *fw, void *context)
   for (i = 0; i  FPGA_CLEAR_LOOP_COUNT; i++) {
   txbuf[0] = FPGA_CMD_READ_STATUS;
   ret = spi_write_then_read(spi, txbuf, 8, rxbuf, rx_len);
 -status = *(u32 *)rxbuf[4];
 +status = be32_to_cpu(*(u32 *)rxbuf[4]);
   if (status == FPGA_STATUS_CLEARED)
   break;
 
 @@ -165,8 +165,8 @@ static void firmware_load(const struct firmware
 *fw, void *context)
 
   txbuf[0] = FPGA_CMD_READ_STATUS;
   ret = spi_write_then_read(spi, txbuf, 8, rxbuf, rx_len);
 -dev_dbg(spi-dev, FPGA Status=%08x\n, *(u32 *)rxbuf[4]);
 -status = *(u32 *)rxbuf[4];
 +dev_dbg(spi-dev, FPGA Status=%08x\n, be32_to_cpu(*(u32 *)rxbuf[4]));
 +status = be32_to_cpu(*(u32 *)rxbuf[4]);

I know you didn't introduce this, but this re-ordering does look better:

+status = be32_to_cpu(*(u32 *)rxbuf[4]);
+dev_dbg(spi-dev, FPGA Status=%08x\n, status);

Other than that:

Acked-by: Stefan Roese s...@denx.de

Thanks,
Stefan

--
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] net: sun4i-emac: remove erroneous assignment

[Stefan Roese]

On 06/03/2013 11:36 PM, Arnd Bergmann wrote:
> The newly added sun4i-emac driver causes a build error when
> CONFIG_NET_POLL_CONTROLLER is set, because it attempts to
> assign a pointer to netdev->poll_controller, which has
> been replaced with ops->ndo_poll_controller in 2.6.31!
> 
> The correct assignment is present as well, so we just need
> to remove the wrong one.
> 
> Signed-off-by: Arnd Bergmann 
> Cc: Stefan Roese 
> Cc: Maxime Ripard 
> Cc: Richard Genoud 

Thanks for fixing this:

Acked-by: Stefan Roese 

Thanks,
Stefan

--
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] net: sun4i-emac: remove erroneous assignment

[Stefan Roese]

On 06/03/2013 11:36 PM, Arnd Bergmann wrote:
 The newly added sun4i-emac driver causes a build error when
 CONFIG_NET_POLL_CONTROLLER is set, because it attempts to
 assign a pointer to netdev-poll_controller, which has
 been replaced with ops-ndo_poll_controller in 2.6.31!
 
 The correct assignment is present as well, so we just need
 to remove the wrong one.
 
 Signed-off-by: Arnd Bergmann a...@arndb.de
 Cc: Stefan Roese s...@denx.de
 Cc: Maxime Ripard maxime.rip...@free-electrons.com
 Cc: Richard Genoud richard.gen...@gmail.com

Thanks for fixing this:

Acked-by: Stefan Roese s...@denx.de

Thanks,
Stefan

--
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/

[PATCH] misc: Add Lattice XP2 FPGA internal flash programming via SPI

[Stefan Roese]

This option enables support for programming of the FPGA's internal
flash of the Lattice XP2 FPGA family via SPI. The newly programmed
bitstream will also be loaded into the internal SRAM (refresh).

Note: The XP2 needs at least 3 clocks to be sent to the FPGA with
inactive CS for some commands. Since the MPC5200 PSC SPI
controller does not support de-activating the CS, we need
to re-configure the PSC port from SPI to GPIO and generate
the clocks with inactive CS this way. After generating these
clocks, the PSC is configured to SPI controller again. So this
current implementation is very SPI controller specific and this
driver will only work on the MPC5200 PSC SPI controller.

Here an example on my custom MPC5200 based board:

$ echo 1 > /sys/class/firmware/lattice-xp2.bit/loading
$ cat a3m071-fpga.jed > /sys/class/firmware/lattice-xp2.bit/data
$ echo 0 > /sys/class/firmware/lattice-xp2.bit/loading

leads to these messages:

lattice-xp2 spi1.0: Security fuse will be set!
lattice-xp2 spi1.0: FPGA Lattice XP2-17 detected
lattice-xp2 spi1.0: FPGA data verified!
lattice-xp2 spi1.0: FPGA security fuse programmed
lattice-xp2 spi1.0: FPGA DONE fuse programmed
lattice-xp2 spi1.0: FPGA successfully refreshed!

Signed-off-by: Stefan Roese 
Cc: Arnd Bergmann 
Cc: Greg Kroah-Hartman 
---
I know that the re-configuration of the pins (SPI vs GPIO) as described
above could/should be done in a "clean way" via pinctrl. But currently there
is no pinctrl support for MPC52xx and the scope of this project doesn't
really allow me to implement this infrastructure. Perhaps somebody else
will find the time to do this later. But I'm pretty sure that this driver
is "as-is" helpful for other users/developers.

  Stefan

 drivers/misc/Kconfig  |  12 +
 drivers/misc/Makefile |   1 +
 drivers/misc/lattice-xp2-config.c | 827 ++
 3 files changed, 840 insertions(+)
 create mode 100644 drivers/misc/lattice-xp2-config.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index c002d86..dea2ed3 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -518,6 +518,18 @@ config LATTICE_ECP3_CONFIG
 
  If unsure, say N.
 
+config LATTICE_XP2_CONFIG
+   tristate "Lattice XP2 FPGA internal flash programming via SPI"
+   depends on SPI && SYSFS && PPC_MPC52xx
+   select FW_LOADER
+   default n
+   help
+ This option enables support for programming of the FPGA's internal
+ flash of the Lattice XP2 FPGA family via SPI. The newly programmed
+ bitstream will also be loaded into the internal SRAM (refresh).
+
+ If unsure, say N.
+
 config SRAM
bool "Generic on-chip SRAM driver"
depends on HAS_IOMEM
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c235d5b..4fb82cf 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -52,4 +52,5 @@ obj-$(CONFIG_ALTERA_STAPL)+=altera-stapl/
 obj-$(CONFIG_INTEL_MEI)+= mei/
 obj-$(CONFIG_VMWARE_VMCI)  += vmw_vmci/
 obj-$(CONFIG_LATTICE_ECP3_CONFIG)  += lattice-ecp3-config.o
+obj-$(CONFIG_LATTICE_XP2_CONFIG)   += lattice-xp2-config.o
 obj-$(CONFIG_SRAM) += sram.o
diff --git a/drivers/misc/lattice-xp2-config.c 
b/drivers/misc/lattice-xp2-config.c
new file mode 100644
index 000..ff8130a
--- /dev/null
+++ b/drivers/misc/lattice-xp2-config.c
@@ -0,0 +1,827 @@
+/*
+ * linux/drivers/firmware/lattice-xp2-config.c
+ *
+ * Copyright (C) 2013 Stefan Roese 
+ *
+ * Based on code provided by Martin Mittelberger 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DRIVER_NAME"lattice-xp2"
+#define DRIVER_VER "1.1"
+#define FIRMWARE_NAME  "lattice-xp2.bit"
+
+#define JEDEC_DATA_START   "L000"
+#define JEDEC_SECURITY "G1*"
+
+/* FPGA commands */
+#define FPGA_CMD_READ_ID   0x98
+#define FPGA_CMD_READ_DEVICE_STATUS0x4d
+#define FPGA_CMD_READ_PROGRAM_STATUS   0x4a
+#define FPGA_CMD_READ_SED_CRC  0x22
+#define FPGA_CMD_X_PROGRAM_EN  0xac
+#define FPGA_CMD_X_SRAM_READ_EN0xae
+#define FPGA_CMD_PROGRAM_DIS   0x78
+#define FPGA_CMD_DISABLE_DONE  0x24
+#define FPGA_CMD_ERASE 0xc0
+#define FPGA_CMD_INIT_ADDRESS  0x84
+#define FPGA_CMD_REFRESH   0xc4
+#define FPGA_CMD_PROGRAM_INC   0xe6
+#define FPGA_CMD_VERIFY_INC0x56
+#define FPGA_CMD_PROGRAM_TAG   0x8e
+#define FPGA_CMD_READ_TAG  0x4e
+#define FPGA_CMD_PROGR

[PATCH] misc: Add Lattice XP2 FPGA internal flash programming via SPI

[Stefan Roese]

This option enables support for programming of the FPGA's internal
flash of the Lattice XP2 FPGA family via SPI. The newly programmed
bitstream will also be loaded into the internal SRAM (refresh).

Note: The XP2 needs at least 3 clocks to be sent to the FPGA with
inactive CS for some commands. Since the MPC5200 PSC SPI
controller does not support de-activating the CS, we need
to re-configure the PSC port from SPI to GPIO and generate
the clocks with inactive CS this way. After generating these
clocks, the PSC is configured to SPI controller again. So this
current implementation is very SPI controller specific and this
driver will only work on the MPC5200 PSC SPI controller.

Here an example on my custom MPC5200 based board:

$ echo 1  /sys/class/firmware/lattice-xp2.bit/loading
$ cat a3m071-fpga.jed  /sys/class/firmware/lattice-xp2.bit/data
$ echo 0  /sys/class/firmware/lattice-xp2.bit/loading

leads to these messages:

lattice-xp2 spi1.0: Security fuse will be set!
lattice-xp2 spi1.0: FPGA Lattice XP2-17 detected
lattice-xp2 spi1.0: FPGA data verified!
lattice-xp2 spi1.0: FPGA security fuse programmed
lattice-xp2 spi1.0: FPGA DONE fuse programmed
lattice-xp2 spi1.0: FPGA successfully refreshed!

Signed-off-by: Stefan Roese s...@denx.de
Cc: Arnd Bergmann a...@arndb.de
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
---
I know that the re-configuration of the pins (SPI vs GPIO) as described
above could/should be done in a clean way via pinctrl. But currently there
is no pinctrl support for MPC52xx and the scope of this project doesn't
really allow me to implement this infrastructure. Perhaps somebody else
will find the time to do this later. But I'm pretty sure that this driver
is as-is helpful for other users/developers.

  Stefan

 drivers/misc/Kconfig  |  12 +
 drivers/misc/Makefile |   1 +
 drivers/misc/lattice-xp2-config.c | 827 ++
 3 files changed, 840 insertions(+)
 create mode 100644 drivers/misc/lattice-xp2-config.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index c002d86..dea2ed3 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -518,6 +518,18 @@ config LATTICE_ECP3_CONFIG
 
  If unsure, say N.
 
+config LATTICE_XP2_CONFIG
+   tristate Lattice XP2 FPGA internal flash programming via SPI
+   depends on SPI  SYSFS  PPC_MPC52xx
+   select FW_LOADER
+   default n
+   help
+ This option enables support for programming of the FPGA's internal
+ flash of the Lattice XP2 FPGA family via SPI. The newly programmed
+ bitstream will also be loaded into the internal SRAM (refresh).
+
+ If unsure, say N.
+
 config SRAM
bool Generic on-chip SRAM driver
depends on HAS_IOMEM
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c235d5b..4fb82cf 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -52,4 +52,5 @@ obj-$(CONFIG_ALTERA_STAPL)+=altera-stapl/
 obj-$(CONFIG_INTEL_MEI)+= mei/
 obj-$(CONFIG_VMWARE_VMCI)  += vmw_vmci/
 obj-$(CONFIG_LATTICE_ECP3_CONFIG)  += lattice-ecp3-config.o
+obj-$(CONFIG_LATTICE_XP2_CONFIG)   += lattice-xp2-config.o
 obj-$(CONFIG_SRAM) += sram.o
diff --git a/drivers/misc/lattice-xp2-config.c 
b/drivers/misc/lattice-xp2-config.c
new file mode 100644
index 000..ff8130a
--- /dev/null
+++ b/drivers/misc/lattice-xp2-config.c
@@ -0,0 +1,827 @@
+/*
+ * linux/drivers/firmware/lattice-xp2-config.c
+ *
+ * Copyright (C) 2013 Stefan Roese s...@denx.de
+ *
+ * Based on code provided by Martin Mittelberger mar...@mittelberger.at
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include linux/device.h
+#include linux/firmware.h
+#include linux/module.h
+#include linux/errno.h
+#include linux/kernel.h
+#include linux/init.h
+#include linux/spi/spi.h
+#include linux/platform_device.h
+#include linux/delay.h
+
+#define DRIVER_NAMElattice-xp2
+#define DRIVER_VER 1.1
+#define FIRMWARE_NAME  lattice-xp2.bit
+
+#define JEDEC_DATA_START   L000
+#define JEDEC_SECURITY G1*
+
+/* FPGA commands */
+#define FPGA_CMD_READ_ID   0x98
+#define FPGA_CMD_READ_DEVICE_STATUS0x4d
+#define FPGA_CMD_READ_PROGRAM_STATUS   0x4a
+#define FPGA_CMD_READ_SED_CRC  0x22
+#define FPGA_CMD_X_PROGRAM_EN  0xac
+#define FPGA_CMD_X_SRAM_READ_EN0xae
+#define FPGA_CMD_PROGRAM_DIS   0x78
+#define FPGA_CMD_DISABLE_DONE  0x24
+#define FPGA_CMD_ERASE 0xc0
+#define FPGA_CMD_INIT_ADDRESS  0x84
+#define FPGA_CMD_REFRESH   0xc4
+#define FPGA_CMD_PROGRAM_INC   0xe6
+#define FPGA_CMD_VERIFY_INC0x56
+#define FPGA_CMD_PROGRAM_TAG

Re: [PATCH 1/5] net: Add EMAC ethernet driver found on Allwinner A10 SoC's

[Stefan Roese]

Hi Florian,

On 24.03.2013 20:03, Florian Fainelli wrote:
> Your phylib implementation looks good now, just some minor comments below:

Thanks for the review. I'll try to address your new comments in a few
days (currently swamped).

Thanks,
Stefan

--
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 1/5] net: Add EMAC ethernet driver found on Allwinner A10 SoC's

[Stefan Roese]

Hi Florian,

On 24.03.2013 20:03, Florian Fainelli wrote:
 Your phylib implementation looks good now, just some minor comments below:

Thanks for the review. I'll try to address your new comments in a few
days (currently swamped).

Thanks,
Stefan

--
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: linux-next: build warnings after merge of the l2-mtd tree

[Stefan Roese]

Hi,

On 02/02/2013 04:38 AM, Stephen Rothwell wrote:
> Hi Artem,
> 
> After merging the l2-mtd tree, today's linux-next build (x86_64
> allmodconfig) produced these warnings:
> 
> In file included from drivers/mtd/chips/cfi_cmdset_0002.c:36:0:
> include/linux/of_platform.h:107:13: warning: 'struct of_device_id' declared 
> inside parameter list [enabled by default]
> include/linux/of_platform.h:107:13: warning: its scope is only this 
> definition or declaration, which is probably not what you want [enabled by 
> default]
> include/linux/of_platform.h:107:13: warning: 'struct device_node' declared 
> inside parameter list [enabled by default]
> drivers/mtd/chips/cfi_cmdset_0002.c: In function 'cfi_cmdset_0002':
> drivers/mtd/chips/cfi_cmdset_0002.c:504:22: warning: unused variable 'np' 
> [-Wunused-variable]
> drivers/mtd/chips/cfi_cmdset_0002.c: At top level:
> drivers/mtd/chips/cfi_cmdset_0002.c:2279:12: warning: 'cfi_ppb_lock' defined 
> but not used [-Wunused-function]
> drivers/mtd/chips/cfi_cmdset_0002.c:2285:12: warning: 'cfi_ppb_unlock' 
> defined but not used [-Wunused-function]
> drivers/mtd/chips/cfi_cmdset_0002.c:2382:12: warning: 'cfi_ppb_is_locked' 
> defined but not used [-Wunused-function]
> 
> Introduced by commit c3a02f171365 ("mtd: cfi_cmdset_0002: Support
> Persistent Protection Bits (PPB) locking").

Sorry about that. I'll send a fixup patch shortly.

Thanks,
Stefan
--
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: linux-next: build warnings after merge of the l2-mtd tree

[Stefan Roese]

Hi,

On 02/02/2013 04:38 AM, Stephen Rothwell wrote:
 Hi Artem,
 
 After merging the l2-mtd tree, today's linux-next build (x86_64
 allmodconfig) produced these warnings:
 
 In file included from drivers/mtd/chips/cfi_cmdset_0002.c:36:0:
 include/linux/of_platform.h:107:13: warning: 'struct of_device_id' declared 
 inside parameter list [enabled by default]
 include/linux/of_platform.h:107:13: warning: its scope is only this 
 definition or declaration, which is probably not what you want [enabled by 
 default]
 include/linux/of_platform.h:107:13: warning: 'struct device_node' declared 
 inside parameter list [enabled by default]
 drivers/mtd/chips/cfi_cmdset_0002.c: In function 'cfi_cmdset_0002':
 drivers/mtd/chips/cfi_cmdset_0002.c:504:22: warning: unused variable 'np' 
 [-Wunused-variable]
 drivers/mtd/chips/cfi_cmdset_0002.c: At top level:
 drivers/mtd/chips/cfi_cmdset_0002.c:2279:12: warning: 'cfi_ppb_lock' defined 
 but not used [-Wunused-function]
 drivers/mtd/chips/cfi_cmdset_0002.c:2285:12: warning: 'cfi_ppb_unlock' 
 defined but not used [-Wunused-function]
 drivers/mtd/chips/cfi_cmdset_0002.c:2382:12: warning: 'cfi_ppb_is_locked' 
 defined but not used [-Wunused-function]
 
 Introduced by commit c3a02f171365 (mtd: cfi_cmdset_0002: Support
 Persistent Protection Bits (PPB) locking).

Sorry about that. I'll send a fixup patch shortly.

Thanks,
Stefan
--
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/

[PATCH v5] misc: Add Lattice ECP3 FPGA configuration via SPI

[Stefan Roese]

This patch adds support for bitstream configuration (programming /
loading) of the Lattice ECP3 FPGA's via the SPI bus.

Here an example on my custom MPC5200 based board:

$ echo 1 > /sys/class/firmware/spi0.0/loading
$ cat fpga_a4m2k.bit > /sys/class/firmware/spi0.0/data
$ echo 0 > /sys/class/firmware/spi0.0/loading

leads to these messages:

lattice-ecp3 spi0.0: FPGA Lattice ECP3-35 detected
lattice-ecp3 spi0.0: Configuring the FPGA...
lattice-ecp3 spi0.0: FPGA succesfully configured!

Signed-off-by: Stefan Roese 
Acked-by: Ming Lei 
Reviewed-by: Grant Likely 
Cc: Arnd Bergmann 
Cc: Greg Kroah-Hartman 
---
v5:
- Moved to drivers/misc as suggested by Grant
- Removed DRIVER_* macros
- Added Acked-by from Ming Lei
- Added Reviewed-by from Grant Likely

v4:
- Allocate per-device struct to store the completion
  variable unique per device

v3:
- Removed unnecessary goto (return instead)
- Added waiting for completion in remove

v2:
- Moved from drivers/firmware to drivers/spi as suggested by
  Ming Lei
- Removed pseudo device
- Removed static buffer pointer usage

 drivers/misc/Kconfig   |  11 ++
 drivers/misc/Makefile  |   1 +
 drivers/misc/lattice-ecp3-config.c | 243 +
 3 files changed, 255 insertions(+)
 create mode 100644 drivers/misc/lattice-ecp3-config.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b151b7c..50a0840 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -499,6 +499,17 @@ config USB_SWITCH_FSA9480
  stereo and mono audio, video, microphone and UART data to use
  a common connector port.
 
+config LATTICE_ECP3_CONFIG
+   tristate "Lattice ECP3 FPGA bitstream configuration via SPI"
+   depends on SPI && SYSFS
+   select FW_LOADER
+   default n
+   help
+ This option enables support for bitstream configuration (programming
+ or loading) of the Lattice ECP3 FPGA family via SPI.
+
+ If unsure, say N.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 2129377..d19d364 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -49,3 +49,4 @@ obj-y += carma/
 obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
 obj-$(CONFIG_ALTERA_STAPL) +=altera-stapl/
 obj-$(CONFIG_INTEL_MEI)+= mei/
+obj-$(CONFIG_LATTICE_ECP3_CONFIG)  += lattice-ecp3-config.o
diff --git a/drivers/misc/lattice-ecp3-config.c 
b/drivers/misc/lattice-ecp3-config.c
new file mode 100644
index 000..ff3106c
--- /dev/null
+++ b/drivers/misc/lattice-ecp3-config.c
@@ -0,0 +1,243 @@
+/*
+ * Copyright (C) 2012 Stefan Roese 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define FIRMWARE_NAME  "lattice-ecp3.bit"
+
+/*
+ * The JTAG ID's of the supported FPGA's. The ID is 32bit wide
+ * reversed as noted in the manual.
+ */
+#define ID_ECP3_17 0xc2088080
+#define ID_ECP3_35 0xc2048080
+
+/* FPGA commands */
+#define FPGA_CMD_READ_ID   0x07/* plus 24 bits */
+#define FPGA_CMD_READ_STATUS   0x09/* plus 24 bits */
+#define FPGA_CMD_CLEAR 0x70
+#define FPGA_CMD_REFRESH   0x71
+#define FPGA_CMD_WRITE_EN  0x4a/* plus 2 bits */
+#define FPGA_CMD_WRITE_DIS 0x4f/* plus 8 bits */
+#define FPGA_CMD_WRITE_INC 0x41/* plus 0 bits */
+
+/*
+ * The status register is 32bit revered, DONE is bit 17 from the TN1222.pdf
+ * (LatticeECP3 Slave SPI Port User's Guide)
+ */
+#define FPGA_STATUS_DONE   0x4000
+#define FPGA_STATUS_CLEARED0x0001
+
+#define FPGA_CLEAR_TIMEOUT 5000/* max. 5000ms for FPGA clear */
+#define FPGA_CLEAR_MSLEEP  10
+#define FPGA_CLEAR_LOOP_COUNT  (FPGA_CLEAR_TIMEOUT / FPGA_CLEAR_MSLEEP)
+
+struct fpga_data {
+   struct completion fw_loaded;
+};
+
+struct ecp3_dev {
+   u32 jedec_id;
+   char *name;
+};
+
+static const struct ecp3_dev ecp3_dev[] = {
+   {
+   .jedec_id = ID_ECP3_17,
+   .name = "Lattice ECP3-17",
+   },
+   {
+   .jedec_id = ID_ECP3_35,
+   .name = "Lattice ECP3-35",
+   },
+};
+
+static void firmware_load(const struct firmware *fw, void *context)
+{
+   struct spi_device *spi = (struct spi_device *)context;
+   struct fpga_data *data = dev_get_drvdata(>dev);
+   u8 *buffer;
+   int ret;
+   u8 txbuf[8];
+   u8 rxbuf[8];
+   int rx_len = 8;
+   int i;
+   u32 jedec_id;
+   u32 status;

[PATCH v5] misc: Add Lattice ECP3 FPGA configuration via SPI

[Stefan Roese]

This patch adds support for bitstream configuration (programming /
loading) of the Lattice ECP3 FPGA's via the SPI bus.

Here an example on my custom MPC5200 based board:

$ echo 1  /sys/class/firmware/spi0.0/loading
$ cat fpga_a4m2k.bit  /sys/class/firmware/spi0.0/data
$ echo 0  /sys/class/firmware/spi0.0/loading

leads to these messages:

lattice-ecp3 spi0.0: FPGA Lattice ECP3-35 detected
lattice-ecp3 spi0.0: Configuring the FPGA...
lattice-ecp3 spi0.0: FPGA succesfully configured!

Signed-off-by: Stefan Roese s...@denx.de
Acked-by: Ming Lei ming@canonical.com
Reviewed-by: Grant Likely grant.lik...@secretlab.ca
Cc: Arnd Bergmann a...@arndb.de
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
---
v5:
- Moved to drivers/misc as suggested by Grant
- Removed DRIVER_* macros
- Added Acked-by from Ming Lei
- Added Reviewed-by from Grant Likely

v4:
- Allocate per-device struct to store the completion
  variable unique per device

v3:
- Removed unnecessary goto (return instead)
- Added waiting for completion in remove

v2:
- Moved from drivers/firmware to drivers/spi as suggested by
  Ming Lei
- Removed pseudo device
- Removed static buffer pointer usage

 drivers/misc/Kconfig   |  11 ++
 drivers/misc/Makefile  |   1 +
 drivers/misc/lattice-ecp3-config.c | 243 +
 3 files changed, 255 insertions(+)
 create mode 100644 drivers/misc/lattice-ecp3-config.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b151b7c..50a0840 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -499,6 +499,17 @@ config USB_SWITCH_FSA9480
  stereo and mono audio, video, microphone and UART data to use
  a common connector port.
 
+config LATTICE_ECP3_CONFIG
+   tristate Lattice ECP3 FPGA bitstream configuration via SPI
+   depends on SPI  SYSFS
+   select FW_LOADER
+   default n
+   help
+ This option enables support for bitstream configuration (programming
+ or loading) of the Lattice ECP3 FPGA family via SPI.
+
+ If unsure, say N.
+
 source drivers/misc/c2port/Kconfig
 source drivers/misc/eeprom/Kconfig
 source drivers/misc/cb710/Kconfig
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 2129377..d19d364 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -49,3 +49,4 @@ obj-y += carma/
 obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
 obj-$(CONFIG_ALTERA_STAPL) +=altera-stapl/
 obj-$(CONFIG_INTEL_MEI)+= mei/
+obj-$(CONFIG_LATTICE_ECP3_CONFIG)  += lattice-ecp3-config.o
diff --git a/drivers/misc/lattice-ecp3-config.c 
b/drivers/misc/lattice-ecp3-config.c
new file mode 100644
index 000..ff3106c
--- /dev/null
+++ b/drivers/misc/lattice-ecp3-config.c
@@ -0,0 +1,243 @@
+/*
+ * Copyright (C) 2012 Stefan Roese s...@denx.de
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include linux/device.h
+#include linux/firmware.h
+#include linux/module.h
+#include linux/errno.h
+#include linux/kernel.h
+#include linux/init.h
+#include linux/spi/spi.h
+#include linux/platform_device.h
+#include linux/delay.h
+
+#define FIRMWARE_NAME  lattice-ecp3.bit
+
+/*
+ * The JTAG ID's of the supported FPGA's. The ID is 32bit wide
+ * reversed as noted in the manual.
+ */
+#define ID_ECP3_17 0xc2088080
+#define ID_ECP3_35 0xc2048080
+
+/* FPGA commands */
+#define FPGA_CMD_READ_ID   0x07/* plus 24 bits */
+#define FPGA_CMD_READ_STATUS   0x09/* plus 24 bits */
+#define FPGA_CMD_CLEAR 0x70
+#define FPGA_CMD_REFRESH   0x71
+#define FPGA_CMD_WRITE_EN  0x4a/* plus 2 bits */
+#define FPGA_CMD_WRITE_DIS 0x4f/* plus 8 bits */
+#define FPGA_CMD_WRITE_INC 0x41/* plus 0 bits */
+
+/*
+ * The status register is 32bit revered, DONE is bit 17 from the TN1222.pdf
+ * (LatticeECP3 Slave SPI Port User's Guide)
+ */
+#define FPGA_STATUS_DONE   0x4000
+#define FPGA_STATUS_CLEARED0x0001
+
+#define FPGA_CLEAR_TIMEOUT 5000/* max. 5000ms for FPGA clear */
+#define FPGA_CLEAR_MSLEEP  10
+#define FPGA_CLEAR_LOOP_COUNT  (FPGA_CLEAR_TIMEOUT / FPGA_CLEAR_MSLEEP)
+
+struct fpga_data {
+   struct completion fw_loaded;
+};
+
+struct ecp3_dev {
+   u32 jedec_id;
+   char *name;
+};
+
+static const struct ecp3_dev ecp3_dev[] = {
+   {
+   .jedec_id = ID_ECP3_17,
+   .name = Lattice ECP3-17,
+   },
+   {
+   .jedec_id = ID_ECP3_35,
+   .name = Lattice ECP3-35,
+   },
+};
+
+static void firmware_load(const struct firmware *fw, void *context)
+{
+   struct spi_device *spi = (struct spi_device *)context;
+   struct fpga_data *data = dev_get_drvdata(spi-dev);
+   u8 *buffer;
+   int ret

Re: [PATCH] firmware: Add Lattice ECP3 FPGA configuration via SPI

[Stefan Roese]

On 11/26/2012 02:35 AM, Ming Lei wrote:
> On Fri, Nov 23, 2012 at 3:58 PM, Stefan Roese  wrote:
>> This patch adds support for bitstream configuration (programming /
>> loading) of the Lattice ECP3 FPGA's via the SPI bus.
>>
>> Here an example on my custom MPC5200 based board:
>>
>> $ echo 1 > /sys/class/firmware/lattice-ecp3.0/loading
>> $ cat fpga_a4m2k.bit > /sys/class/firmware/lattice-ecp3.0/data
>> $ echo 0 > /sys/class/firmware/lattice-ecp3.0/loading
>>
>> leads to these messages:
>>
>> lattice-ecp3 spi32766.0: FPGA Lattice ECP3-35 detected
>> lattice-ecp3 spi32766.0: Configuring the FPGA...
>> lattice-ecp3 spi32766.0: FPGA succesfully configured!
>>
>> Signed-off-by: Stefan Roese 
>> Cc: Ming Lei 
>> ---
>>  arch/powerpc/Kconfig   |   2 +
>>  drivers/firmware/Kconfig   |  11 ++
>>  drivers/firmware/Makefile  |   1 +
>>  drivers/firmware/lattice-ecp3-config.c | 254 
>> +
> 
> The driver is just a firmware loader, so looks 'drivers/firmware/' is not
> a good place for it. And it is better to make the driver as part  the
> of the FPGA driver, such as other drivers which need downloading firmware,
> or you can put it under 'drivers/spi' if there is no such fpga driver.

This FPGA loading via the firmware infrastructure is completely
independent from the FPGA device driver. Its common for all Lattice ECP3
FPGA's. So I would like to place this loading driver into a generic
directory. If firmware is wrong then I'll move this driver for the next
version to drivers/spi.

> BTW, you'd better to CC maintainers of this directory.

Will do.



>> +static struct platform_device pseudo_dev = {
>> +   .name = DRIVER_NAME,
>> +   .id   = 0,
>> +   .num_resources = 0,
>> +   .dev  = {
>> +   .release = dev_release,
>> +   }
>> +};
> 
> Why do you introduce one such device? If you just make it
> as the parent of firmware device, it is not correct and unnecessary,
> see below.

Good point. Will fix in next version.

>> +
>> +static struct device *ecp3_device = _dev.dev;
>> +
>> +static void firmware_load(const struct firmware *fw, void *context)
>> +{
>> +   struct spi_device *spi = (struct spi_device *)context;
>> +   static u8 *buffer;
> 
> Defining the buffer as static is dangerous and will make the buffer shared by
> more than one such FPGA device, so buffer data may become broken and
> cause downloading a mistaken firmware.

Fixed.



>> +static int __devinit lattice_ecp3_probe(struct spi_device *spi)
>> +{
>> +   int err;
>> +
>> +   if (platform_device_register(_dev))
>> +   return -ENODEV;
>> +
>> +   err = request_firmware_nowait(THIS_MODULE, FW_ACTION_NOHOTPLUG,
>> + FIRMWARE_NAME, ecp3_device,
> 
> The >dev should be passed to request_firmware_nowait() instead of
> the pseudo-device, which is wrong. It is a device lifetime thing. When you
> download firmware via spi device, you should make sure the spi device
> is live during firmware downloading, so the spi device should be passed to
> request_firmware_nowait() to make it as the parent of the firmware device.

Fixed.

Thanks for the review,
Stefan

--
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] firmware: Add Lattice ECP3 FPGA configuration via SPI

[Stefan Roese]

On 11/26/2012 02:35 AM, Ming Lei wrote:
 On Fri, Nov 23, 2012 at 3:58 PM, Stefan Roese s...@denx.de wrote:
 This patch adds support for bitstream configuration (programming /
 loading) of the Lattice ECP3 FPGA's via the SPI bus.

 Here an example on my custom MPC5200 based board:

 $ echo 1  /sys/class/firmware/lattice-ecp3.0/loading
 $ cat fpga_a4m2k.bit  /sys/class/firmware/lattice-ecp3.0/data
 $ echo 0  /sys/class/firmware/lattice-ecp3.0/loading

 leads to these messages:

 lattice-ecp3 spi32766.0: FPGA Lattice ECP3-35 detected
 lattice-ecp3 spi32766.0: Configuring the FPGA...
 lattice-ecp3 spi32766.0: FPGA succesfully configured!

 Signed-off-by: Stefan Roese s...@denx.de
 Cc: Ming Lei ming@canonical.com
 ---
  arch/powerpc/Kconfig   |   2 +
  drivers/firmware/Kconfig   |  11 ++
  drivers/firmware/Makefile  |   1 +
  drivers/firmware/lattice-ecp3-config.c | 254 
 +
 
 The driver is just a firmware loader, so looks 'drivers/firmware/' is not
 a good place for it. And it is better to make the driver as part  the
 of the FPGA driver, such as other drivers which need downloading firmware,
 or you can put it under 'drivers/spi' if there is no such fpga driver.

This FPGA loading via the firmware infrastructure is completely
independent from the FPGA device driver. Its common for all Lattice ECP3
FPGA's. So I would like to place this loading driver into a generic
directory. If firmware is wrong then I'll move this driver for the next
version to drivers/spi.

 BTW, you'd better to CC maintainers of this directory.

Will do.

snip

 +static struct platform_device pseudo_dev = {
 +   .name = DRIVER_NAME,
 +   .id   = 0,
 +   .num_resources = 0,
 +   .dev  = {
 +   .release = dev_release,
 +   }
 +};
 
 Why do you introduce one such device? If you just make it
 as the parent of firmware device, it is not correct and unnecessary,
 see below.

Good point. Will fix in next version.

 +
 +static struct device *ecp3_device = pseudo_dev.dev;
 +
 +static void firmware_load(const struct firmware *fw, void *context)
 +{
 +   struct spi_device *spi = (struct spi_device *)context;
 +   static u8 *buffer;
 
 Defining the buffer as static is dangerous and will make the buffer shared by
 more than one such FPGA device, so buffer data may become broken and
 cause downloading a mistaken firmware.

Fixed.

snip

 +static int __devinit lattice_ecp3_probe(struct spi_device *spi)
 +{
 +   int err;
 +
 +   if (platform_device_register(pseudo_dev))
 +   return -ENODEV;
 +
 +   err = request_firmware_nowait(THIS_MODULE, FW_ACTION_NOHOTPLUG,
 + FIRMWARE_NAME, ecp3_device,
 
 The spi-dev should be passed to request_firmware_nowait() instead of
 the pseudo-device, which is wrong. It is a device lifetime thing. When you
 download firmware via spi device, you should make sure the spi device
 is live during firmware downloading, so the spi device should be passed to
 request_firmware_nowait() to make it as the parent of the firmware device.

Fixed.

Thanks for the review,
Stefan

--
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/

[PATCH] firmware: Add Lattice ECP3 FPGA configuration via SPI

[Stefan Roese]

This patch adds support for bitstream configuration (programming /
loading) of the Lattice ECP3 FPGA's via the SPI bus.

Here an example on my custom MPC5200 based board:

$ echo 1 > /sys/class/firmware/lattice-ecp3.0/loading
$ cat fpga_a4m2k.bit > /sys/class/firmware/lattice-ecp3.0/data
$ echo 0 > /sys/class/firmware/lattice-ecp3.0/loading

leads to these messages:

lattice-ecp3 spi32766.0: FPGA Lattice ECP3-35 detected
lattice-ecp3 spi32766.0: Configuring the FPGA...
lattice-ecp3 spi32766.0: FPGA succesfully configured!

Signed-off-by: Stefan Roese 
Cc: Ming Lei 
---
 arch/powerpc/Kconfig   |   2 +
 drivers/firmware/Kconfig   |  11 ++
 drivers/firmware/Makefile  |   1 +
 drivers/firmware/lattice-ecp3-config.c | 254 +
 4 files changed, 268 insertions(+)
 create mode 100644 drivers/firmware/lattice-ecp3-config.c

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index a902a5c..0c138d2 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -1006,6 +1006,8 @@ source "net/Kconfig"
 
 source "drivers/Kconfig"
 
+source "drivers/firmware/Kconfig"
+
 source "fs/Kconfig"
 
 source "arch/powerpc/sysdev/qe_lib/Kconfig"
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 9b00072..f7f864f 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -145,6 +145,17 @@ config ISCSI_IBFT
  detect iSCSI boot parameters dynamically during system boot, say Y.
  Otherwise, say N.
 
+config LATTICE_ECP3_CONFIG
+   tristate "Lattice ECP3 FPGA bitstream configuration via SPI module"
+   select FW_LOADER
+   depends on SPI
+   default n
+   help
+ This option enables support for bitstream configuration (programming
+ or loading) of the Lattice ECP3 FPGA family via SPI.
+
+ If unsure, say N.
+
 source "drivers/firmware/google/Kconfig"
 
 endmenu
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 5a7e273..9dadb3f 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -12,5 +12,6 @@ obj-$(CONFIG_DMIID)   += dmi-id.o
 obj-$(CONFIG_ISCSI_IBFT_FIND)  += iscsi_ibft_find.o
 obj-$(CONFIG_ISCSI_IBFT)   += iscsi_ibft.o
 obj-$(CONFIG_FIRMWARE_MEMMAP)  += memmap.o
+obj-$(CONFIG_LATTICE_ECP3_CONFIG) += lattice-ecp3-config.o
 
 obj-$(CONFIG_GOOGLE_FIRMWARE)  += google/
diff --git a/drivers/firmware/lattice-ecp3-config.c 
b/drivers/firmware/lattice-ecp3-config.c
new file mode 100644
index 000..213c526
--- /dev/null
+++ b/drivers/firmware/lattice-ecp3-config.c
@@ -0,0 +1,254 @@
+/*
+ * linux/drivers/firmware/lattice-ecp3-config.c
+ *
+ * Copyright (C) 2012 Stefan Roese 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DRIVER_NAME"lattice-ecp3"
+#define DRIVER_VER "1.0"
+#define FIRMWARE_NAME  "lattice-ecp3.bit"
+
+/*
+ * The JTAG ID's of the supported FPGA's. The ID is 32bit wide
+ * reversed as noted in the manual.
+ */
+#define ID_ECP3_17 0xc2088080
+#define ID_ECP3_35 0xc2048080
+
+/* FPGA commands */
+#define FPGA_CMD_READ_ID   0x07/* plus 24 bits */
+#define FPGA_CMD_READ_STATUS   0x09/* plus 24 bits */
+#define FPGA_CMD_CLEAR 0x70
+#define FPGA_CMD_REFRESH   0x71
+#define FPGA_CMD_WRITE_EN  0x4a/* plus 2 bits */
+#define FPGA_CMD_WRITE_DIS 0x4f/* plus 8 bits */
+#define FPGA_CMD_WRITE_INC 0x41/* plus 0 bits */
+
+/*
+ * The status register is 32bit revered, DONE is bit 17 from the TN1222.pdf
+ * (LatticeECP3 Slave SPI Port User's Guide)
+ */
+#define FPGA_STATUS_DONE   0x4000
+#define FPGA_STATUS_CLEARED0x0001
+
+#define FPGA_CLEAR_TIMEOUT 5000/* max. 5000ms for FPGA clear */
+#define FPGA_CLEAR_MSLEEP  10
+#define FPGA_CLEAR_LOOP_COUNT  (FPGA_CLEAR_TIMEOUT / FPGA_CLEAR_MSLEEP)
+
+struct ecp3_dev {
+   u32 jedec_id;
+   char *name;
+};
+
+static const struct ecp3_dev ecp3_dev[] = {
+   {
+   .jedec_id = ID_ECP3_17,
+   .name = "Lattice ECP3-17",
+   },
+   {
+   .jedec_id = ID_ECP3_35,
+   .name = "Lattice ECP3-35",
+   },
+};
+static void dev_release(struct device *dev)
+{
+   return;
+}
+
+static struct platform_device pseudo_dev = {
+   .name = DRIVER_NAME,
+   .id   = 0,
+   .num_resources = 0,
+   .dev  = {
+   .release = dev_release,
+   }
+};
+
+static struct device *ecp3_device = _dev.dev;
+
+static void firmware_load(const struct firmware *fw, void *

[PATCH] firmware: Add Lattice ECP3 FPGA configuration via SPI

[Stefan Roese]

This patch adds support for bitstream configuration (programming /
loading) of the Lattice ECP3 FPGA's via the SPI bus.

Here an example on my custom MPC5200 based board:

$ echo 1  /sys/class/firmware/lattice-ecp3.0/loading
$ cat fpga_a4m2k.bit  /sys/class/firmware/lattice-ecp3.0/data
$ echo 0  /sys/class/firmware/lattice-ecp3.0/loading

leads to these messages:

lattice-ecp3 spi32766.0: FPGA Lattice ECP3-35 detected
lattice-ecp3 spi32766.0: Configuring the FPGA...
lattice-ecp3 spi32766.0: FPGA succesfully configured!

Signed-off-by: Stefan Roese s...@denx.de
Cc: Ming Lei ming@canonical.com
---
 arch/powerpc/Kconfig   |   2 +
 drivers/firmware/Kconfig   |  11 ++
 drivers/firmware/Makefile  |   1 +
 drivers/firmware/lattice-ecp3-config.c | 254 +
 4 files changed, 268 insertions(+)
 create mode 100644 drivers/firmware/lattice-ecp3-config.c

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index a902a5c..0c138d2 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -1006,6 +1006,8 @@ source net/Kconfig
 
 source drivers/Kconfig
 
+source drivers/firmware/Kconfig
+
 source fs/Kconfig
 
 source arch/powerpc/sysdev/qe_lib/Kconfig
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 9b00072..f7f864f 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -145,6 +145,17 @@ config ISCSI_IBFT
  detect iSCSI boot parameters dynamically during system boot, say Y.
  Otherwise, say N.
 
+config LATTICE_ECP3_CONFIG
+   tristate Lattice ECP3 FPGA bitstream configuration via SPI module
+   select FW_LOADER
+   depends on SPI
+   default n
+   help
+ This option enables support for bitstream configuration (programming
+ or loading) of the Lattice ECP3 FPGA family via SPI.
+
+ If unsure, say N.
+
 source drivers/firmware/google/Kconfig
 
 endmenu
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 5a7e273..9dadb3f 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -12,5 +12,6 @@ obj-$(CONFIG_DMIID)   += dmi-id.o
 obj-$(CONFIG_ISCSI_IBFT_FIND)  += iscsi_ibft_find.o
 obj-$(CONFIG_ISCSI_IBFT)   += iscsi_ibft.o
 obj-$(CONFIG_FIRMWARE_MEMMAP)  += memmap.o
+obj-$(CONFIG_LATTICE_ECP3_CONFIG) += lattice-ecp3-config.o
 
 obj-$(CONFIG_GOOGLE_FIRMWARE)  += google/
diff --git a/drivers/firmware/lattice-ecp3-config.c 
b/drivers/firmware/lattice-ecp3-config.c
new file mode 100644
index 000..213c526
--- /dev/null
+++ b/drivers/firmware/lattice-ecp3-config.c
@@ -0,0 +1,254 @@
+/*
+ * linux/drivers/firmware/lattice-ecp3-config.c
+ *
+ * Copyright (C) 2012 Stefan Roese s...@denx.de
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include linux/device.h
+#include linux/firmware.h
+#include linux/module.h
+#include linux/errno.h
+#include linux/kernel.h
+#include linux/init.h
+#include linux/spi/spi.h
+#include linux/platform_device.h
+#include linux/delay.h
+
+#define DRIVER_NAMElattice-ecp3
+#define DRIVER_VER 1.0
+#define FIRMWARE_NAME  lattice-ecp3.bit
+
+/*
+ * The JTAG ID's of the supported FPGA's. The ID is 32bit wide
+ * reversed as noted in the manual.
+ */
+#define ID_ECP3_17 0xc2088080
+#define ID_ECP3_35 0xc2048080
+
+/* FPGA commands */
+#define FPGA_CMD_READ_ID   0x07/* plus 24 bits */
+#define FPGA_CMD_READ_STATUS   0x09/* plus 24 bits */
+#define FPGA_CMD_CLEAR 0x70
+#define FPGA_CMD_REFRESH   0x71
+#define FPGA_CMD_WRITE_EN  0x4a/* plus 2 bits */
+#define FPGA_CMD_WRITE_DIS 0x4f/* plus 8 bits */
+#define FPGA_CMD_WRITE_INC 0x41/* plus 0 bits */
+
+/*
+ * The status register is 32bit revered, DONE is bit 17 from the TN1222.pdf
+ * (LatticeECP3 Slave SPI Port User's Guide)
+ */
+#define FPGA_STATUS_DONE   0x4000
+#define FPGA_STATUS_CLEARED0x0001
+
+#define FPGA_CLEAR_TIMEOUT 5000/* max. 5000ms for FPGA clear */
+#define FPGA_CLEAR_MSLEEP  10
+#define FPGA_CLEAR_LOOP_COUNT  (FPGA_CLEAR_TIMEOUT / FPGA_CLEAR_MSLEEP)
+
+struct ecp3_dev {
+   u32 jedec_id;
+   char *name;
+};
+
+static const struct ecp3_dev ecp3_dev[] = {
+   {
+   .jedec_id = ID_ECP3_17,
+   .name = Lattice ECP3-17,
+   },
+   {
+   .jedec_id = ID_ECP3_35,
+   .name = Lattice ECP3-35,
+   },
+};
+static void dev_release(struct device *dev)
+{
+   return;
+}
+
+static struct platform_device pseudo_dev = {
+   .name = DRIVER_NAME,
+   .id   = 0,
+   .num_resources = 0,
+   .dev  = {
+   .release = dev_release,
+   }
+};
+
+static struct device *ecp3_device = pseudo_dev.dev;
+
+static void firmware_load(const

Re: [PATCH] drivers/mtd/devices/spear_smi.c: use devm_ functions consistently

[Stefan Roese]

On 08/24/2012 01:35 PM, Artem Bityutskiy wrote:
>> @@ -1073,21 +1043,13 @@ static int __devexit spear_smi_remove(struct 
>> platform_device *pdev)
>>  ret = mtd_device_unregister(>mtd);
>>  if (ret)
>>  dev_err(>dev, "error removing mtd\n");
>> -
>> -iounmap(flash->base_addr);
>> -kfree(flash);
>>  }
>>  
>>  irq = platform_get_irq(pdev, 0);
>> -free_irq(irq, dev);
> 
> I guess 'platform_get_irq()' should be killed as well? Stefan, this is
> strange code - we get irq, without checking for error, and then free it?
> What is the rationale?

Yes, this seems bogus. platform_get_irq() definitely should be removed
from spear_smi_remove().

>>  clk_disable_unprepare(dev->clk);
>> -clk_put(dev->clk);
>> -iounmap(dev->io_base);
>> -kfree(dev);
>>  
>>  smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> -release_mem_region(smi_base->start, resource_size(smi_base));
>>  platform_set_drvdata(pdev, NULL);
> 
> Why do we set platform data to NULL, is this needed?

It seems to be common practice to use this call to clear the drvdata in
the driver remove function. I have to admit, that I'm not sure if its
really needed though.

Thanks,
Stefan
--
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] drivers/mtd/devices/spear_smi.c: use devm_ functions consistently

[Stefan Roese]

On 08/24/2012 01:35 PM, Artem Bityutskiy wrote:
 @@ -1073,21 +1043,13 @@ static int __devexit spear_smi_remove(struct 
 platform_device *pdev)
  ret = mtd_device_unregister(flash-mtd);
  if (ret)
  dev_err(pdev-dev, error removing mtd\n);
 -
 -iounmap(flash-base_addr);
 -kfree(flash);
  }
  
  irq = platform_get_irq(pdev, 0);
 -free_irq(irq, dev);
 
 I guess 'platform_get_irq()' should be killed as well? Stefan, this is
 strange code - we get irq, without checking for error, and then free it?
 What is the rationale?

Yes, this seems bogus. platform_get_irq() definitely should be removed
from spear_smi_remove().

  clk_disable_unprepare(dev-clk);
 -clk_put(dev-clk);
 -iounmap(dev-io_base);
 -kfree(dev);
  
  smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 -release_mem_region(smi_base-start, resource_size(smi_base));
  platform_set_drvdata(pdev, NULL);
 
 Why do we set platform data to NULL, is this needed?

It seems to be common practice to use this call to clear the drvdata in
the driver remove function. I have to admit, that I'm not sure if its
really needed though.

Thanks,
Stefan
--
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 07/11] net/stmmac: mark probe function as __devinit

[Stefan Roese]

On 08/08/2012 04:47 PM, Arnd Bergmann wrote:
> Driver probe functions are generally __devinit so they will be
> discarded after initialization for non-hotplug kernels.
> This was found by a new warning after patch 6a228452d "stmmac: Add
> device-tree support" adds a new __devinit function that is called
> from stmmac_pltfr_probe.
> 
> Without this patch, building socfpga_defconfig results in:
> 
> WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): Section 
> mismatch in reference from the function stmmac_pltfr_probe() to the function 
> .devinit.text:stmmac_probe_config_dt()
> The function stmmac_pltfr_probe() references
> the function __devinit stmmac_probe_config_dt().
> This is often because stmmac_pltfr_probe lacks a __devinit
> annotation or the annotation of stmmac_probe_config_dt is wrong.
> 
> Signed-off-by: Arnd Bergmann 
> Cc: Stefan Roese 
> Cc: Giuseppe Cavallaro 
> Cc: David S. Miller 
> Cc: net...@vger.kernel.org

Acked-by: Stefan Roese 

Thanks,
Stefan
--
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 07/11] net/stmmac: mark probe function as __devinit

[Stefan Roese]

On 08/08/2012 04:47 PM, Arnd Bergmann wrote:
 Driver probe functions are generally __devinit so they will be
 discarded after initialization for non-hotplug kernels.
 This was found by a new warning after patch 6a228452d stmmac: Add
 device-tree support adds a new __devinit function that is called
 from stmmac_pltfr_probe.
 
 Without this patch, building socfpga_defconfig results in:
 
 WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): Section 
 mismatch in reference from the function stmmac_pltfr_probe() to the function 
 .devinit.text:stmmac_probe_config_dt()
 The function stmmac_pltfr_probe() references
 the function __devinit stmmac_probe_config_dt().
 This is often because stmmac_pltfr_probe lacks a __devinit
 annotation or the annotation of stmmac_probe_config_dt is wrong.
 
 Signed-off-by: Arnd Bergmann a...@arndb.de
 Cc: Stefan Roese s...@denx.de
 Cc: Giuseppe Cavallaro peppe.cavall...@st.com
 Cc: David S. Miller da...@davemloft.net
 Cc: net...@vger.kernel.org

Acked-by: Stefan Roese s...@denx.de

Thanks,
Stefan
--
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] drivers/mtd/devices/spear_smi.c: failure test for null rather than negative integer

[Stefan Roese]

On Wednesday 11 July 2012 10:58:38 Julia Lawall wrote:
> From: Julia Lawall 
> 
> dev_get_platdata returns a pointer, so the failure value would be NULL
> rather than a negative integer.
> 
> The semantic match that finds this problem is: (http://coccinelle.lip6.fr/)
> 
> // 
> @@
> expression x,e;
> statement S1,S2;
> @@
> 
> *x = dev_get_platdata(...)
> ... when != x = e
> *if (x < 0) S1 else S2
> // 
> 
> Signed-off-by: Julia Lawall 

Acked-by: Stefan Roese 
 
Thanks,
Stefan
--
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] drivers/mtd/devices/spear_smi.c: failure test for null rather than negative integer

[Stefan Roese]

On Wednesday 11 July 2012 10:58:38 Julia Lawall wrote:
 From: Julia Lawall julia.law...@lip6.fr
 
 dev_get_platdata returns a pointer, so the failure value would be NULL
 rather than a negative integer.
 
 The semantic match that finds this problem is: (http://coccinelle.lip6.fr/)
 
 // smpl
 @@
 expression x,e;
 statement S1,S2;
 @@
 
 *x = dev_get_platdata(...)
 ... when != x = e
 *if (x  0) S1 else S2
 // /smpl
 
 Signed-off-by: Julia Lawall julia.law...@lip6.fr

Acked-by: Stefan Roese s...@denx.de
 
Thanks,
Stefan
--
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 1/2] pci: Fix bus resource assignment on 32 bits with 64b resources

[Stefan Roese]

On Monday 10 December 2007, Benjamin Herrenschmidt wrote:
> The current pci_assign_unassigned_resources() code doesn't work properly
> on 32 bits platforms with 64 bits resources. The main reason is the use
> of unsigned long in various places instead of resource_size_t.
>
> This fixes it, along with some tricks to avoid casting to 64 bits on
> platforms that don't need it in every printk around.
>
> This is a pre-requisite for making powerpc use the generic code instead of
> its own half-useful implementation.
>
> Signed-off-by: Benjamin Herrenschmidt <[EMAIL PROTECTED]>

Checking Linus's latest git repository, it seems this patch (and the 2nd from 
this series) hasn't been applied till now. This is just a reminder, that it 
gets in in this merge-window.

Thanks.

Best regards,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] pci: Fix bus resource assignment on 32 bits with 64b resources

[Stefan Roese]

On Monday 10 December 2007, Benjamin Herrenschmidt wrote:
 The current pci_assign_unassigned_resources() code doesn't work properly
 on 32 bits platforms with 64 bits resources. The main reason is the use
 of unsigned long in various places instead of resource_size_t.

 This fixes it, along with some tricks to avoid casting to 64 bits on
 platforms that don't need it in every printk around.

 This is a pre-requisite for making powerpc use the generic code instead of
 its own half-useful implementation.

 Signed-off-by: Benjamin Herrenschmidt [EMAIL PROTECTED]

Checking Linus's latest git repository, it seems this patch (and the 2nd from 
this series) hasn't been applied till now. This is just a reminder, that it 
gets in in this merge-window.

Thanks.

Best regards,
Stefan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c

[Stefan Roese]

On Thursday 24 January 2008, Josh Boyer wrote:
> On Wed, 23 Jan 2008 23:37:33 +0100
>
> Roel Kluin <[EMAIL PROTECTED]> wrote:
> > logical/bitand typo
> >
> > Signed-off-by: Roel Kluin <[EMAIL PROTECTED]>
> > ---
> > diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c
> > index ebf9e21..dcfb459 100644
> > --- a/arch/powerpc/boot/4xx.c
> > +++ b/arch/powerpc/boot/4xx.c
> > @@ -104,7 +104,7 @@ void ibm4xx_denali_fixup_memsize(void)
> > val = DDR_GET_VAL(val, DDR_CS_MAP, DDR_CS_MAP_SHIFT);
> > cs = 0;
> > while (val) {
> > -   if (val && 0x1)
> > +   if (val & 0x1)
> > cs++;
> > val = val >> 1;
> > }
>
> Hm, good catch.
>
> Stefan, have you had problems with this code at all?

I'm not using the bootwrapper on a 4xx with Denali core.

Best regards,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH][ppc] logical/bitand typo in powerpc/boot/4xx.c

[Stefan Roese]

On Thursday 24 January 2008, Josh Boyer wrote:
 On Wed, 23 Jan 2008 23:37:33 +0100

 Roel Kluin [EMAIL PROTECTED] wrote:
  logical/bitand typo
 
  Signed-off-by: Roel Kluin [EMAIL PROTECTED]
  ---
  diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c
  index ebf9e21..dcfb459 100644
  --- a/arch/powerpc/boot/4xx.c
  +++ b/arch/powerpc/boot/4xx.c
  @@ -104,7 +104,7 @@ void ibm4xx_denali_fixup_memsize(void)
  val = DDR_GET_VAL(val, DDR_CS_MAP, DDR_CS_MAP_SHIFT);
  cs = 0;
  while (val) {
  -   if (val  0x1)
  +   if (val  0x1)
  cs++;
  val = val  1;
  }

 Hm, good catch.

 Stefan, have you had problems with this code at all?

I'm not using the bootwrapper on a 4xx with Denali core.

Best regards,
Stefan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

ppc: 4xx: sysctl table check failed: /kernel/l2cr .1.31 Missing strategy

[Stefan Roese]

I'm seeing this error message when booting an recent arch/ppc kernel on
4xx platforms (tested on Ocotea and other 4xx platforms). Booting NFS
rootfs still works fine, but this message kind of makes me "nervous".
This is not seen on 4xx arch/powerpc platforms. Here the bootlog:

Linux version 2.6.24-rc8 ([EMAIL PROTECTED]) (gcc version 4.2.2) #1 Wed Jan 16 
11:51:57 CET 2008
IBM Ocotea port (MontaVista Software, Inc. <[EMAIL PROTECTED]>)
Zone PFN ranges:
  DMA 0 ->65536
  Normal  65536 ->65536
Movable zone start PFN for each node
early_node_map[1] active PFN ranges
0:0 ->65536
Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 65024
Kernel command line: root=/dev/nfs rw nfsroot=192.168.1.1:/opt/eldk-4.1/ppc_4xx 
ip=192.168.80.2:192.168.1.1::255.255.0.0:ocotea:eth0:off panic=1 
console=ttyS0,115200
PID hash table entries: 1024 (order: 10, 4096 bytes)
console [ttyS0] enabled
Dentry cache hash table entries: 32768 (order: 5, 131072 bytes)
Inode-cache hash table entries: 16384 (order: 4, 65536 bytes)
Memory: 257536k available (1660k kernel code, 540k data, 104k init, 0k highmem)
SLUB: Genslabs=11, HWalign=32, Order=0-1, MinObjects=4, CPUs=1, Nodes=1
Mount-cache hash table entries: 512
net_namespace: 64 bytes
NET: Registered protocol family 16
PCI: Probing PCI hardware
NET: Registered protocol family 2
IP route cache hash table entries: 2048 (order: 1, 8192 bytes)
TCP established hash table entries: 8192 (order: 4, 65536 bytes)
TCP bind hash table entries: 8192 (order: 3, 32768 bytes)
TCP: Hash tables configured (established 8192 bind 8192)
TCP reno registered
sysctl table check failed: /kernel/l2cr .1.31 Missing strategy
Call Trace:
[cfc11df0] [c00082d0] show_stack+0x44/0x1ac (unreliable)
[cfc11e30] [c0034ffc] set_fail+0x50/0x68
[cfc11e50] [c0035428] sysctl_check_table+0x414/0x70c
[cfc11ec0] [c003543c] sysctl_check_table+0x428/0x70c
[cfc11f30] [c0021fe4] register_sysctl_table+0x64/0xb4
[cfc11f50] [c0213858] register_ppc_htab_sysctl+0x18/0x2c
[cfc11f60] [c02121f4] kernel_init+0xd0/0x2b0
[cfc11ff0] [c0003b3c] kernel_thread+0x44/0x60
io scheduler noop registered
io scheduler anticipatory registered
io scheduler deadline registered
io scheduler cfq registered (default)
...

Any ideas?

Thanks.

Best regards,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

ppc: 4xx: sysctl table check failed: /kernel/l2cr .1.31 Missing strategy

[Stefan Roese]

I'm seeing this error message when booting an recent arch/ppc kernel on
4xx platforms (tested on Ocotea and other 4xx platforms). Booting NFS
rootfs still works fine, but this message kind of makes me nervous.
This is not seen on 4xx arch/powerpc platforms. Here the bootlog:

Linux version 2.6.24-rc8 ([EMAIL PROTECTED]) (gcc version 4.2.2) #1 Wed Jan 16 
11:51:57 CET 2008
IBM Ocotea port (MontaVista Software, Inc. [EMAIL PROTECTED])
Zone PFN ranges:
  DMA 0 -65536
  Normal  65536 -65536
Movable zone start PFN for each node
early_node_map[1] active PFN ranges
0:0 -65536
Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 65024
Kernel command line: root=/dev/nfs rw nfsroot=192.168.1.1:/opt/eldk-4.1/ppc_4xx 
ip=192.168.80.2:192.168.1.1::255.255.0.0:ocotea:eth0:off panic=1 
console=ttyS0,115200
PID hash table entries: 1024 (order: 10, 4096 bytes)
console [ttyS0] enabled
Dentry cache hash table entries: 32768 (order: 5, 131072 bytes)
Inode-cache hash table entries: 16384 (order: 4, 65536 bytes)
Memory: 257536k available (1660k kernel code, 540k data, 104k init, 0k highmem)
SLUB: Genslabs=11, HWalign=32, Order=0-1, MinObjects=4, CPUs=1, Nodes=1
Mount-cache hash table entries: 512
net_namespace: 64 bytes
NET: Registered protocol family 16
PCI: Probing PCI hardware
NET: Registered protocol family 2
IP route cache hash table entries: 2048 (order: 1, 8192 bytes)
TCP established hash table entries: 8192 (order: 4, 65536 bytes)
TCP bind hash table entries: 8192 (order: 3, 32768 bytes)
TCP: Hash tables configured (established 8192 bind 8192)
TCP reno registered
sysctl table check failed: /kernel/l2cr .1.31 Missing strategy
Call Trace:
[cfc11df0] [c00082d0] show_stack+0x44/0x1ac (unreliable)
[cfc11e30] [c0034ffc] set_fail+0x50/0x68
[cfc11e50] [c0035428] sysctl_check_table+0x414/0x70c
[cfc11ec0] [c003543c] sysctl_check_table+0x428/0x70c
[cfc11f30] [c0021fe4] register_sysctl_table+0x64/0xb4
[cfc11f50] [c0213858] register_ppc_htab_sysctl+0x18/0x2c
[cfc11f60] [c02121f4] kernel_init+0xd0/0x2b0
[cfc11ff0] [c0003b3c] kernel_thread+0x44/0x60
io scheduler noop registered
io scheduler anticipatory registered
io scheduler deadline registered
io scheduler cfq registered (default)
...

Any ideas?

Thanks.

Best regards,
Stefan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/5] Convert PowerPC MPC i2c to of_platform_driver from platform_driver

[Stefan Roese]

On Thursday 20 December 2007, David Gibson wrote:
> On Wed, Dec 19, 2007 at 11:41:44PM -0500, Jon Smirl wrote:
> > Convert MPC i2c driver from being a platform_driver to an open
> > firmware version. Error returns were improved. Routine names were
> > changed from fsl_ to mpc_ to make them match the file name.
>
> In discussions BenH and I have had, we've actually concluded that
> moving this from platform drivers to of_platform drives is not
> actually a good idea.
>
> In fact we're planning to move away from of_platform devices and
> drivers and instead develop a framework for instantiating platform
> devices or i2c devices or whatever devices from the device tree nodes.

Now that is interesting news. I like this idea.

But what should be done to support the still missing devices in the 4xx 
arch/powerpc tree, like I2C, NAND etc.? Should we wait with those driver till 
this framework is available?

Thanks.

Cheers,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/5] Convert PowerPC MPC i2c to of_platform_driver from platform_driver

[Stefan Roese]

On Thursday 20 December 2007, David Gibson wrote:
 On Wed, Dec 19, 2007 at 11:41:44PM -0500, Jon Smirl wrote:
  Convert MPC i2c driver from being a platform_driver to an open
  firmware version. Error returns were improved. Routine names were
  changed from fsl_ to mpc_ to make them match the file name.

 In discussions BenH and I have had, we've actually concluded that
 moving this from platform drivers to of_platform drives is not
 actually a good idea.

 In fact we're planning to move away from of_platform devices and
 drivers and instead develop a framework for instantiating platform
 devices or i2c devices or whatever devices from the device tree nodes.

Now that is interesting news. I like this idea.

But what should be done to support the still missing devices in the 4xx 
arch/powerpc tree, like I2C, NAND etc.? Should we wait with those driver till 
this framework is available?

Thanks.

Cheers,
Stefan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Bootup support for watchdog with short timeout (touch_nmi_watchdog()?)

[Stefan Roese]

[added linuxppc-dev since it's PPC relevant too]

On Tuesday 30 October 2007, Josh Boyer wrote:
> On Mon, 29 Oct 2007 15:45:03 -0400
>
> [EMAIL PROTECTED] (Lennart Sorensen) wrote:
> > On Mon, Oct 29, 2007 at 03:22:27PM +0100, Stefan Roese wrote:
> > > I'm trying to implement support for a board specific watchdog on a
> > > PPC440EPx board with a very short timeout. In this case, the watchdog
> > > has to be "kicked" at least every 100ms, even while booting and the
> > > real watchdog driver not running yet. While looking for trigger places
> > > in the kernel source, I noticed the already existing
> > > "touch_nmi_watchdog()" function, which seems to be doing what I need.
> > > Even if the name not exactly matches my hardware setup.
> > >
> > > My question now is, is it recommended to use this
> > > touch_nmi_watchdog() "infrastructure" for my PPC custom specific
> > > watchdog during bootup? And if yes, should it perhaps be renamed to a
> > > more generic name, like "touch_watchdog"?
> > >
> > > Please advise. Thanks.
> >
> > No idea really.  Who would design a watchdog with such a short trigger
> > time?  That doesn't seem to be useful in any way.

It definitely is useful in our case, since its a requirement for 
this "critical" project. It's not needed to have such a short trigger time 
while booting, but unfortunately this external watchdog only supports one 
fixed timeout.

> To some degree, it's configurable.

No, I'm afraid it's not configurable in this case.

> But the generic question still 
> stands.  It seems like a decent idea to me.  Making touch_watchdog (or
> whatever it winds up being called) nice across arches might be fun.

I already have it running on my system using a quick hack (see patch below) in 
include/asm-ppc/nmi.h (yes, still arch/ppc for now :-( ). But for a clean 
implementation, that has chances for upstream merge (in arch/powerpc later), 
I would really like to hear if I should move on further this way. 

My impression is, that changing the name from touch_nmi_watchdog() to 
something like touch_watchdog(), and therefore touching lots of files, makes 
it more unlikely that this resulting patch will get accepted. But 
implementing this bootup watchdog support in asm-ppc(asm-powerpc)/nmi.h 
header seems also not totally correct, since it's not really an NMI in this 
case.

Any thoughts on this?

Thanks.

Best regards,
Stefan

diff --git a/include/asm-ppc/nmi.h b/include/asm-ppc/nmi.h
new file mode 100644
index 000..f18862b
--- /dev/null
+++ b/include/asm-ppc/nmi.h
@@ -0,0 +1,16 @@
+/*
+ *  linux/include/asm-ppc/nmi.h
+ */
+#ifndef ASM_NMI_H
+#define ASM_NMI_H
+
+#ifdef BOARD_WATCHDOG_FUNC
+#define touch_nmi_watchdog BOARD_WATCHDOG_FUNC
+#else
+static inline void touch_nmi_watchdog(void)
+{
+   touch_softlockup_watchdog();
+}
+#endif
+
+#endif /* ASM_NMI_H */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Bootup support for watchdog with short timeout (touch_nmi_watchdog()?)

[Stefan Roese]

[added linuxppc-dev since it's PPC relevant too]

On Tuesday 30 October 2007, Josh Boyer wrote:
 On Mon, 29 Oct 2007 15:45:03 -0400

 [EMAIL PROTECTED] (Lennart Sorensen) wrote:
  On Mon, Oct 29, 2007 at 03:22:27PM +0100, Stefan Roese wrote:
   I'm trying to implement support for a board specific watchdog on a
   PPC440EPx board with a very short timeout. In this case, the watchdog
   has to be kicked at least every 100ms, even while booting and the
   real watchdog driver not running yet. While looking for trigger places
   in the kernel source, I noticed the already existing
   touch_nmi_watchdog() function, which seems to be doing what I need.
   Even if the name not exactly matches my hardware setup.
  
   My question now is, is it recommended to use this
   touch_nmi_watchdog() infrastructure for my PPC custom specific
   watchdog during bootup? And if yes, should it perhaps be renamed to a
   more generic name, like touch_watchdog?
  
   Please advise. Thanks.
 
  No idea really.  Who would design a watchdog with such a short trigger
  time?  That doesn't seem to be useful in any way.

It definitely is useful in our case, since its a requirement for 
this critical project. It's not needed to have such a short trigger time 
while booting, but unfortunately this external watchdog only supports one 
fixed timeout.

 To some degree, it's configurable.

No, I'm afraid it's not configurable in this case.

 But the generic question still 
 stands.  It seems like a decent idea to me.  Making touch_watchdog (or
 whatever it winds up being called) nice across arches might be fun.

I already have it running on my system using a quick hack (see patch below) in 
include/asm-ppc/nmi.h (yes, still arch/ppc for now :-( ). But for a clean 
implementation, that has chances for upstream merge (in arch/powerpc later), 
I would really like to hear if I should move on further this way. 

My impression is, that changing the name from touch_nmi_watchdog() to 
something like touch_watchdog(), and therefore touching lots of files, makes 
it more unlikely that this resulting patch will get accepted. But 
implementing this bootup watchdog support in asm-ppc(asm-powerpc)/nmi.h 
header seems also not totally correct, since it's not really an NMI in this 
case.

Any thoughts on this?

Thanks.

Best regards,
Stefan

diff --git a/include/asm-ppc/nmi.h b/include/asm-ppc/nmi.h
new file mode 100644
index 000..f18862b
--- /dev/null
+++ b/include/asm-ppc/nmi.h
@@ -0,0 +1,16 @@
+/*
+ *  linux/include/asm-ppc/nmi.h
+ */
+#ifndef ASM_NMI_H
+#define ASM_NMI_H
+
+#ifdef BOARD_WATCHDOG_FUNC
+#define touch_nmi_watchdog BOARD_WATCHDOG_FUNC
+#else
+static inline void touch_nmi_watchdog(void)
+{
+   touch_softlockup_watchdog();
+}
+#endif
+
+#endif /* ASM_NMI_H */
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Bootup support for watchdog with short timeout (touch_nmi_watchdog()?)

[Stefan Roese]

I'm trying to implement support for a board specific watchdog on a PPC440EPx 
board with a very short timeout. In this case, the watchdog has to 
be "kicked" at least every 100ms, even while booting and the real watchdog 
driver not running yet. While looking for trigger places in the kernel 
source, I noticed the already existing "touch_nmi_watchdog()" function, which 
seems to be doing what I need. Even if the name not exactly matches my 
hardware setup.

My question now is, is it recommended to use this 
touch_nmi_watchdog() "infrastructure" for my PPC custom specific watchdog 
during bootup? And if yes, should it perhaps be renamed to a more generic 
name, like "touch_watchdog"?

Please advise. Thanks.

Best regards,
Stefan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Bootup support for watchdog with short timeout (touch_nmi_watchdog()?)

[Stefan Roese]

I'm trying to implement support for a board specific watchdog on a PPC440EPx 
board with a very short timeout. In this case, the watchdog has to 
be kicked at least every 100ms, even while booting and the real watchdog 
driver not running yet. While looking for trigger places in the kernel 
source, I noticed the already existing touch_nmi_watchdog() function, which 
seems to be doing what I need. Even if the name not exactly matches my 
hardware setup.

My question now is, is it recommended to use this 
touch_nmi_watchdog() infrastructure for my PPC custom specific watchdog 
during bootup? And if yes, should it perhaps be renamed to a more generic 
name, like touch_watchdog?

Please advise. Thanks.

Best regards,
Stefan
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add support for Xilinx SystemACE CompactFlash interface.

[Stefan Roese]

On Monday 07 May 2007, Grant Likely wrote:
> Tested on Xilinx Virtex ppc405, Katmai 440SPe, and Microblaze
>
> Signed-off-by: Grant Likely <[EMAIL PROTECTED]>

Acked-by: Stefan Roese <[EMAIL PROTECTED]>

Best regards,
Stefan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add support for Xilinx SystemACE CompactFlash interface.

[Stefan Roese]

On Monday 07 May 2007, Grant Likely wrote:
 Tested on Xilinx Virtex ppc405, Katmai 440SPe, and Microblaze

 Signed-off-by: Grant Likely [EMAIL PROTECTED]

Acked-by: Stefan Roese [EMAIL PROTECTED]

Best regards,
Stefan
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Correct location for ADC/DAC drivers

[Stefan Roese]

On Friday 04 May 2007 10:24, Robert Schwebel wrote:
> On Tue, May 01, 2007 at 02:35:44PM +0200, Stefan Roese wrote:
> > I'm in the stage of integrating some ADC and DAC drivers for the AMCC
> > 405EZ PPC and looking for the correct location to place these drivers
> > in the Linux source tree. The drivers are basically character-drivers,
> > so my first thought is to put them in "drivers/char/adc/foo.c" or
> > "drivers/char/adc_foo.c". Is this a good solution?
> >
> > Any suggestions welcome (could be that I missed an already existing
> > example).
> >
> > BTW: I am aware of the hwmon subsystem, but I don't think it fits my
> > needs in this case.
>
> Could you elaborate the requirements a bit more? ADC is not ADC, because
> slow i2c ADCs which measure a temperature every five minutes have other
> requirements than multi-megabyte-per-second-dma-driven ADCs.

The hardware (PPC405EZ) actually implements an high speed, dma capable, ADC 
controller with 10-bit resolution and up to 4MHz sample rate. The current 
driver doesn't support all these features though (dma is not supported right 
now for example). Could be that this will be added in future releases. It 
would be good though, to have the driver located at the "correct" place in 
the kernel tree right away.

Best regards,
Stefan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Correct location for ADC/DAC drivers

[Stefan Roese]

On Wednesday 02 May 2007 21:11, Russell King wrote:
> > > Is there a maintainer for this "drivers/mfd" directory?
> >
> > rmk
>
> I wouldn't go that far.  There's no real infrastructure there
> to maintain, so I'd actually say that the directory was
> maintainerless.  However, I'll own up to the UCB/MCP drivers
> in there.

So perhaps you could answer is you feel that these ADC & DAC chrdev device 
drivers would fit into this drivers/mfd directory, or are better suited for 
the drivers/char directory?

Thanks.

Best regards,
Stefan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/