[PATCH v1] Revert "clocksource/drivers/tcb_clksrc: Use 32 bit tcb as sched_clock"

[Romain Izard]

This reverts commit 7b9f1d16e6d1 ("clocksource/drivers/tcb_clksrc: Use
32 bit tcb as sched_clock"). In the current state, the kernel warns
against a late registration of the new sched_clock, the printk clock
resets after only a few minutes, and it seems that scheduling can be
affected as well.

Signed-off-by: Romain Izard 
---
 drivers/clocksource/tcb_clksrc.c | 16 +---
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
index 745844ee973e..d4ca9962a759 100644
--- a/drivers/clocksource/tcb_clksrc.c
+++ b/drivers/clocksource/tcb_clksrc.c
@@ -10,7 +10,6 @@
 #include 
 #include 
 #include 
-#include 
 
 
 /*
@@ -57,14 +56,9 @@ static u64 tc_get_cycles(struct clocksource *cs)
return (upper << 16) | lower;
 }
 
-static u32 tc_get_cv32(void)
-{
-   return __raw_readl(tcaddr + ATMEL_TC_REG(0, CV));
-}
-
 static u64 tc_get_cycles32(struct clocksource *cs)
 {
-   return tc_get_cv32();
+   return __raw_readl(tcaddr + ATMEL_TC_REG(0, CV));
 }
 
 static struct clocksource clksrc = {
@@ -75,11 +69,6 @@ static struct clocksource clksrc = {
.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
 };
 
-static u64 notrace tc_read_sched_clock(void)
-{
-   return tc_get_cv32();
-}
-
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 
 struct tc_clkevt_device {
@@ -350,9 +339,6 @@ static int __init tcb_clksrc_init(void)
clksrc.read = tc_get_cycles32;
/* setup ony channel 0 */
tcb_setup_single_chan(tc, best_divisor_idx);
-
-   /* register sched_clock on chips with single 32 bit counter */
-   sched_clock_register(tc_read_sched_clock, 32, divided_rate);
} else {
/* tclib will give us three clocks no matter what the
 * underlying platform supports.
-- 
2.9.3

[PATCH] mmc: sdhci-of-at91: Support external regulators

[Romain Izard]

The SDHCI controller in the SAMA5D2 chip requires a valid voltage set
in the power control register, otherwise commands will fail with a
timeout error.

When using the regulator framework to specify the regulator used by the
mmc device, the voltage is not configured, and it is not possible to use
the connected device.

Implement a custom 'set_power' function for this specific hardware, that
configures the voltage in the register in all cases.

Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
 drivers/mmc/host/sdhci-of-at91.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 2f9ad213377a..291a74955a4c 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -85,11 +85,30 @@ static void sdhci_at91_set_clock(struct sdhci_host *host, 
unsigned int clock)
sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 }
 
+/*
+ * In this specific implementation of the SDHCI contoller, the power register
+ * needs to have a valid voltage set even when the power supply is managed by
+ * an external regulator.
+ */
+static void sdhci_at91_set_power(struct sdhci_host *host, unsigned char mode,
+unsigned short vdd)
+{
+   if (!IS_ERR(host->mmc->supply.vmmc)) {
+   struct mmc_host *mmc = host->mmc;
+
+   spin_unlock_irq(>lock);
+   mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
+   spin_lock_irq(>lock);
+   }
+   sdhci_set_power_noreg(host, mode, vdd);
+}
+
 static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
.set_clock  = sdhci_at91_set_clock,
.set_bus_width  = sdhci_set_bus_width,
.reset  = sdhci_reset,
.set_uhs_signaling  = sdhci_set_uhs_signaling,
+   .set_power  = sdhci_at91_set_power,
 };
 
 static const struct sdhci_pltfm_data soc_data_sama5d2 = {
-- 
2.9.3

[PATCH] mmc: sdhci-of-at91: Support external regulators

[Romain Izard]

The SDHCI controller in the SAMA5D2 chip requires a valid voltage set
in the power control register, otherwise commands will fail with a
timeout error.

When using the regulator framework to specify the regulator used by the
mmc device, the voltage is not configured, and it is not possible to use
the connected device.

Implement a custom 'set_power' function for this specific hardware, that
configures the voltage in the register in all cases.

Signed-off-by: Romain Izard 
---
 drivers/mmc/host/sdhci-of-at91.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
index 2f9ad213377a..291a74955a4c 100644
--- a/drivers/mmc/host/sdhci-of-at91.c
+++ b/drivers/mmc/host/sdhci-of-at91.c
@@ -85,11 +85,30 @@ static void sdhci_at91_set_clock(struct sdhci_host *host, 
unsigned int clock)
sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 }
 
+/*
+ * In this specific implementation of the SDHCI contoller, the power register
+ * needs to have a valid voltage set even when the power supply is managed by
+ * an external regulator.
+ */
+static void sdhci_at91_set_power(struct sdhci_host *host, unsigned char mode,
+unsigned short vdd)
+{
+   if (!IS_ERR(host->mmc->supply.vmmc)) {
+   struct mmc_host *mmc = host->mmc;
+
+   spin_unlock_irq(>lock);
+   mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
+   spin_lock_irq(>lock);
+   }
+   sdhci_set_power_noreg(host, mode, vdd);
+}
+
 static const struct sdhci_ops sdhci_at91_sama5d2_ops = {
.set_clock  = sdhci_at91_set_clock,
.set_bus_width  = sdhci_set_bus_width,
.reset  = sdhci_reset,
.set_uhs_signaling  = sdhci_set_uhs_signaling,
+   .set_power  = sdhci_at91_set_power,
 };
 
 static const struct sdhci_pltfm_data soc_data_sama5d2 = {
-- 
2.9.3

[PATCH v2] usb: gadget: legacy gadgets are optional

[Romain Izard]

With commit "usb: gadget: don't couple configfs to legacy gadgets"
it is possible to build a modular kernel with both built-in configfs
support and modular legacy gadget drivers.

But when building a kernel without modules, it is also necessary to be
able to build with configfs but without any legacy gadget driver.

Mark the choice for legacy gadget drivers as optional.

Fixes: bc49d1d17dcf ("usb: gadget: don't couple configfs to legacy gadgets")
Cc: <sta...@vger.kernel.org> # 4.9+
Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
changes in v2:
 - Reword description

 drivers/usb/gadget/Kconfig | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 8ad203296079..e157e9aa4f3d 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -212,7 +212,7 @@ config USB_F_TCM
 # this first set of drivers all depend on bulk-capable hardware.
 
 config USB_CONFIGFS
-   tristate "USB functions configurable through configfs"
+   tristate "USB Gadget functions configurable through configfs"
select USB_LIBCOMPOSITE
help
  A Linux USB "gadget" can be set up through configfs.
@@ -458,8 +458,9 @@ config USB_CONFIGFS_F_TCM
  UAS utilizes the USB 3.0 feature called streams support.
 
 choice
-   tristate "USB Gadget Drivers"
+   tristate "USB Gadget precomposed configurations"
default USB_ETH
+   optional
help
  A Linux "Gadget Driver" talks to the USB Peripheral Controller
  driver through the abstract "gadget" API.  Some other operating
@@ -476,6 +477,12 @@ choice
  not be able work with that controller, or might need to implement
  a less common variant of a device class protocol.
 
+ The available choices each represent a single precomposed USB
+ gadget configuration. In the device model, each option contains
+ both the device instanciation as a child for a USB gadget
+ controller, and the relevant drivers for each function declared
+ by the device.
+
 source "drivers/usb/gadget/legacy/Kconfig"
 
 endchoice
-- 
2.9.3

[PATCH v2] usb: gadget: legacy gadgets are optional

[Romain Izard]

With commit "usb: gadget: don't couple configfs to legacy gadgets"
it is possible to build a modular kernel with both built-in configfs
support and modular legacy gadget drivers.

But when building a kernel without modules, it is also necessary to be
able to build with configfs but without any legacy gadget driver.

Mark the choice for legacy gadget drivers as optional.

Fixes: bc49d1d17dcf ("usb: gadget: don't couple configfs to legacy gadgets")
Cc:  # 4.9+
Signed-off-by: Romain Izard 
---
changes in v2:
 - Reword description

 drivers/usb/gadget/Kconfig | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 8ad203296079..e157e9aa4f3d 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -212,7 +212,7 @@ config USB_F_TCM
 # this first set of drivers all depend on bulk-capable hardware.
 
 config USB_CONFIGFS
-   tristate "USB functions configurable through configfs"
+   tristate "USB Gadget functions configurable through configfs"
select USB_LIBCOMPOSITE
help
  A Linux USB "gadget" can be set up through configfs.
@@ -458,8 +458,9 @@ config USB_CONFIGFS_F_TCM
  UAS utilizes the USB 3.0 feature called streams support.
 
 choice
-   tristate "USB Gadget Drivers"
+   tristate "USB Gadget precomposed configurations"
default USB_ETH
+   optional
help
  A Linux "Gadget Driver" talks to the USB Peripheral Controller
  driver through the abstract "gadget" API.  Some other operating
@@ -476,6 +477,12 @@ choice
  not be able work with that controller, or might need to implement
  a less common variant of a device class protocol.
 
+ The available choices each represent a single precomposed USB
+ gadget configuration. In the device model, each option contains
+ both the device instanciation as a child for a USB gadget
+ controller, and the relevant drivers for each function declared
+ by the device.
+
 source "drivers/usb/gadget/legacy/Kconfig"
 
 endchoice
-- 
2.9.3

Re: Warning on boot on SAMA5D2 with Linux 4.11-rc1

[Romain Izard]

2017-03-06 12:28 GMT+01:00 Romain Izard <romain.izard@gmail.com>:
>
> While looking for another issue, I tried Linux 4.11-rc1 on a SAMA5D2 Xplained
> board. The boot log contains the following warning:
>
> [0.10] [ cut here ]
> [0.10] WARNING: CPU: 0 PID: 1 at
> ../kernel/time/sched_clock.c:180 sched_clock_register+0x44/0x1e4
> [0.10] CPU: 0 PID: 1 Comm: swapper Not tainted 4.11.0-rc1+ #3
> [0.10] Hardware name: Atmel SAMA5
> [0.10] [] (unwind_backtrace) from []
> (show_stack+0x10/0x14)
> [0.10] [] (show_stack) from [] (__warn+0xe0/0xf8)
> [0.10] [] (__warn) from []
> (warn_slowpath_null+0x20/0x28)
> [0.10] [] (warn_slowpath_null) from []
> (sched_clock_register+0x44/0x1e4)
> [0.10] [] (sched_clock_register) from []
> (tcb_clksrc_init+0x1ac/0x360)
> [0.10] [] (tcb_clksrc_init) from []
> (do_one_initcall+0xb4/0x15c)
> [0.10] [] (do_one_initcall) from []
> (kernel_init_freeable+0x134/0x1c4)
> [0.10] [] (kernel_init_freeable) from []
> (kernel_init+0x8/0x10c)
> [0.10] [] (kernel_init) from []
> (ret_from_fork+0x14/0x3c)
> [0.10] ---[ end trace 7ce9be9d7cf6f800 ]---
> [0.100012] sched_clock: 32 bits at 10MHz, resolution 96ns, wraps
> every 206986376143ns
>
> This is related to the following commit:
> 7b9f1d16e6d1 clocksource/drivers/tcb_clksrc: Use 32 bit tcb as sched_clock
>
> When we call sched_clock_register from tcb_clksrc_init from
> arch_initcall, we are too late as sched expects all the candidates for
> its clock to be registered before interrupts are enabled. This warning
> does not prevent the tcb clock from being used.
>

After some more use with 4.11-rc1, I also noticed that the timestamp for
printk rolls over to 0 after only 413s. Reverting the aforementioned commit
fixes it.

-- 
Romain Izard

Re: Warning on boot on SAMA5D2 with Linux 4.11-rc1

[Romain Izard]

2017-03-06 12:28 GMT+01:00 Romain Izard :
>
> While looking for another issue, I tried Linux 4.11-rc1 on a SAMA5D2 Xplained
> board. The boot log contains the following warning:
>
> [0.10] [ cut here ]
> [0.10] WARNING: CPU: 0 PID: 1 at
> ../kernel/time/sched_clock.c:180 sched_clock_register+0x44/0x1e4
> [0.10] CPU: 0 PID: 1 Comm: swapper Not tainted 4.11.0-rc1+ #3
> [0.10] Hardware name: Atmel SAMA5
> [0.10] [] (unwind_backtrace) from []
> (show_stack+0x10/0x14)
> [0.10] [] (show_stack) from [] (__warn+0xe0/0xf8)
> [0.10] [] (__warn) from []
> (warn_slowpath_null+0x20/0x28)
> [0.10] [] (warn_slowpath_null) from []
> (sched_clock_register+0x44/0x1e4)
> [0.10] [] (sched_clock_register) from []
> (tcb_clksrc_init+0x1ac/0x360)
> [0.10] [] (tcb_clksrc_init) from []
> (do_one_initcall+0xb4/0x15c)
> [0.10] [] (do_one_initcall) from []
> (kernel_init_freeable+0x134/0x1c4)
> [0.10] [] (kernel_init_freeable) from []
> (kernel_init+0x8/0x10c)
> [0.10] [] (kernel_init) from []
> (ret_from_fork+0x14/0x3c)
> [0.10] ---[ end trace 7ce9be9d7cf6f800 ]---
> [0.100012] sched_clock: 32 bits at 10MHz, resolution 96ns, wraps
> every 206986376143ns
>
> This is related to the following commit:
> 7b9f1d16e6d1 clocksource/drivers/tcb_clksrc: Use 32 bit tcb as sched_clock
>
> When we call sched_clock_register from tcb_clksrc_init from
> arch_initcall, we are too late as sched expects all the candidates for
> its clock to be registered before interrupts are enabled. This warning
> does not prevent the tcb clock from being used.
>

After some more use with 4.11-rc1, I also noticed that the timestamp for
printk rolls over to 0 after only 413s. Reverting the aforementioned commit
fixes it.

-- 
Romain Izard

Warning on boot on SAMA5D2 with Linux 4.11-rc1

[Romain Izard]

Hello,

While looking for another issue, I tried Linux 4.11-rc1 on a SAMA5D2 Xplained
board. The boot log contains the following warning:

[0.10] [ cut here ]
[0.10] WARNING: CPU: 0 PID: 1 at
../kernel/time/sched_clock.c:180 sched_clock_register+0x44/0x1e4
[0.10] CPU: 0 PID: 1 Comm: swapper Not tainted 4.11.0-rc1+ #3
[0.10] Hardware name: Atmel SAMA5
[0.10] [] (unwind_backtrace) from []
(show_stack+0x10/0x14)
[0.10] [] (show_stack) from [] (__warn+0xe0/0xf8)
[0.10] [] (__warn) from []
(warn_slowpath_null+0x20/0x28)
[0.10] [] (warn_slowpath_null) from []
(sched_clock_register+0x44/0x1e4)
[0.10] [] (sched_clock_register) from []
(tcb_clksrc_init+0x1ac/0x360)
[0.10] [] (tcb_clksrc_init) from []
(do_one_initcall+0xb4/0x15c)
[0.10] [] (do_one_initcall) from []
(kernel_init_freeable+0x134/0x1c4)
[0.10] [] (kernel_init_freeable) from []
(kernel_init+0x8/0x10c)
[0.10] [] (kernel_init) from []
(ret_from_fork+0x14/0x3c)
[0.10] ---[ end trace 7ce9be9d7cf6f800 ]---
[0.100012] sched_clock: 32 bits at 10MHz, resolution 96ns, wraps
every 206986376143ns

This is related to the following commit:
7b9f1d16e6d1 clocksource/drivers/tcb_clksrc: Use 32 bit tcb as sched_clock

When we call sched_clock_register from tcb_clksrc_init from
arch_initcall, we are too late as sched expects all the candidates for
its clock to be registered before interrupts are enabled. This warning
does not prevent the tcb clock from being used.

-- 
Romain Izard

Warning on boot on SAMA5D2 with Linux 4.11-rc1

[Romain Izard]

Hello,

While looking for another issue, I tried Linux 4.11-rc1 on a SAMA5D2 Xplained
board. The boot log contains the following warning:

[0.10] [ cut here ]
[0.10] WARNING: CPU: 0 PID: 1 at
../kernel/time/sched_clock.c:180 sched_clock_register+0x44/0x1e4
[0.10] CPU: 0 PID: 1 Comm: swapper Not tainted 4.11.0-rc1+ #3
[0.10] Hardware name: Atmel SAMA5
[0.10] [] (unwind_backtrace) from []
(show_stack+0x10/0x14)
[0.10] [] (show_stack) from [] (__warn+0xe0/0xf8)
[0.10] [] (__warn) from []
(warn_slowpath_null+0x20/0x28)
[0.10] [] (warn_slowpath_null) from []
(sched_clock_register+0x44/0x1e4)
[0.10] [] (sched_clock_register) from []
(tcb_clksrc_init+0x1ac/0x360)
[0.10] [] (tcb_clksrc_init) from []
(do_one_initcall+0xb4/0x15c)
[0.10] [] (do_one_initcall) from []
(kernel_init_freeable+0x134/0x1c4)
[0.10] [] (kernel_init_freeable) from []
(kernel_init+0x8/0x10c)
[0.10] [] (kernel_init) from []
(ret_from_fork+0x14/0x3c)
[0.10] ---[ end trace 7ce9be9d7cf6f800 ]---
[0.100012] sched_clock: 32 bits at 10MHz, resolution 96ns, wraps
every 206986376143ns

This is related to the following commit:
7b9f1d16e6d1 clocksource/drivers/tcb_clksrc: Use 32 bit tcb as sched_clock

When we call sched_clock_register from tcb_clksrc_init from
arch_initcall, we are too late as sched expects all the candidates for
its clock to be registered before interrupts are enabled. This warning
does not prevent the tcb clock from being used.

-- 
Romain Izard

[PATCH] Revert "ARM: at91/dt: sama5d2: Use new compatible for ohci node"

[Romain Izard]

This reverts commit cab43282682e ("ARM: at91/dt: sama5d2: Use new
compatible for ohci node")

It depends from commit 7150bc9b4d43 ("usb: ohci-at91: Forcibly suspend
ports while USB suspend") which was reverted and implemented
differently. With the new implementation, the compatible string must
remain the same.

The compatible string introduced by this commit has been used in the
default SAMA5D2 dtsi starting from Linux 4.8. As it has never been
working correctly in an official release, removing it should not be
breaking the stability rules.

Fixes: cab43282682e ("ARM: at91/dt: sama5d2: Use new compatible for ohci node")
Signed-off-by: Romain Izard <romain.izard@gmail.com>
cc: <sta...@vger.kernel.org>
---
 arch/arm/boot/dts/sama5d2.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index ceb9783ff7e1..ff7eae833a6d 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -266,7 +266,7 @@
};
 
usb1: ohci@0040 {
-   compatible = "atmel,sama5d2-ohci", "usb-ohci";
+   compatible = "atmel,at91rm9200-ohci", "usb-ohci";
reg = <0x0040 0x10>;
interrupts = <41 IRQ_TYPE_LEVEL_HIGH 2>;
clocks = <_clk>, <_clk>, <>;
-- 
2.9.3

[PATCH] Revert "ARM: at91/dt: sama5d2: Use new compatible for ohci node"

[Romain Izard]

This reverts commit cab43282682e ("ARM: at91/dt: sama5d2: Use new
compatible for ohci node")

It depends from commit 7150bc9b4d43 ("usb: ohci-at91: Forcibly suspend
ports while USB suspend") which was reverted and implemented
differently. With the new implementation, the compatible string must
remain the same.

The compatible string introduced by this commit has been used in the
default SAMA5D2 dtsi starting from Linux 4.8. As it has never been
working correctly in an official release, removing it should not be
breaking the stability rules.

Fixes: cab43282682e ("ARM: at91/dt: sama5d2: Use new compatible for ohci node")
Signed-off-by: Romain Izard 
cc: 
---
 arch/arm/boot/dts/sama5d2.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index ceb9783ff7e1..ff7eae833a6d 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -266,7 +266,7 @@
};
 
usb1: ohci@0040 {
-   compatible = "atmel,sama5d2-ohci", "usb-ohci";
+   compatible = "atmel,at91rm9200-ohci", "usb-ohci";
reg = <0x0040 0x10>;
interrupts = <41 IRQ_TYPE_LEVEL_HIGH 2>;
clocks = <_clk>, <_clk>, <>;
-- 
2.9.3

[PATCH] usb: gadget: legacy gadgets are optional

[Romain Izard]

When building without modules, it makes sense to configure the kernel to
only use configfs for USB Gadget drivers.

Mark the choice for legacy gadget drivers as optional.

Signed-off-by: Romain Izard <romain.izard@gmail.com>
cc: <sta...@vger.kernel.org> # 4.9
---
 drivers/usb/gadget/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 8ad203296079..f3ee80ece682 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -460,6 +460,7 @@ config USB_CONFIGFS_F_TCM
 choice
tristate "USB Gadget Drivers"
default USB_ETH
+   optional
help
  A Linux "Gadget Driver" talks to the USB Peripheral Controller
  driver through the abstract "gadget" API.  Some other operating
-- 
2.9.3

[PATCH] usb: gadget: legacy gadgets are optional

[Romain Izard]

When building without modules, it makes sense to configure the kernel to
only use configfs for USB Gadget drivers.

Mark the choice for legacy gadget drivers as optional.

Signed-off-by: Romain Izard 
cc:  # 4.9
---
 drivers/usb/gadget/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 8ad203296079..f3ee80ece682 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -460,6 +460,7 @@ config USB_CONFIGFS_F_TCM
 choice
tristate "USB Gadget Drivers"
default USB_ETH
+   optional
help
  A Linux "Gadget Driver" talks to the USB Peripheral Controller
  driver through the abstract "gadget" API.  Some other operating
-- 
2.9.3

[PATCH] atmel_serial: Use the fractional divider when possible

[Romain Izard]

The fractional baud rate generator is available when using the
asynchronous mode of Atmel USART controllers. It makes it possible to
use higher baudrates, in exchange for a less precise clock with a
variable duty cycle.

The existing code restricts its use to the normal mode of the USART
controller, following the recommendation from the datasheet for the
first chip embedding this type of controller. This recommendation has
been removed from the documentation for the newer chips. After
verification, all revisions of this controller should be able to use the
fractional baud rate generator with the different asynchronous modes.

Removing the condition on ATMEL_US_USMODE makes it possible to get
correct baudrates at high speed in more cases.

This was tested with a board using an Atmel SAMA5D2 chip and a TI
WL1831 WiFi/Bluetooth combo chip at 3 Mbauds, with hardware flow control
enabled.

Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
 drivers/tty/serial/atmel_serial.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c 
b/drivers/tty/serial/atmel_serial.c
index fabbe76203bb..6684456dca9e 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1758,7 +1758,9 @@ static void atmel_get_ip_name(struct uart_port *port)
 
/*
 * Only USART devices from at91sam9260 SOC implement fractional
-* baudrate.
+* baudrate. It is available for all asynchronous modes, with the
+* following restriction: the sampling clock's duty cycle is not
+* constant.
 */
atmel_port->has_frac_baudrate = false;
atmel_port->has_hw_timer = false;
@@ -2202,8 +2204,7 @@ static void atmel_set_termios(struct uart_port *port, 
struct ktermios *termios,
 * then
 * 8 CD + FP = selected clock / (2 * baudrate)
 */
-   if (atmel_port->has_frac_baudrate &&
-   (mode & ATMEL_US_USMODE) == ATMEL_US_USMODE_NORMAL) {
+   if (atmel_port->has_frac_baudrate) {
div = DIV_ROUND_CLOSEST(port->uartclk, baud * 2);
cd = div >> 3;
fp = div & ATMEL_US_FP_MASK;
-- 
2.9.3

[PATCH] atmel_serial: Use the fractional divider when possible

[Romain Izard]

The fractional baud rate generator is available when using the
asynchronous mode of Atmel USART controllers. It makes it possible to
use higher baudrates, in exchange for a less precise clock with a
variable duty cycle.

The existing code restricts its use to the normal mode of the USART
controller, following the recommendation from the datasheet for the
first chip embedding this type of controller. This recommendation has
been removed from the documentation for the newer chips. After
verification, all revisions of this controller should be able to use the
fractional baud rate generator with the different asynchronous modes.

Removing the condition on ATMEL_US_USMODE makes it possible to get
correct baudrates at high speed in more cases.

This was tested with a board using an Atmel SAMA5D2 chip and a TI
WL1831 WiFi/Bluetooth combo chip at 3 Mbauds, with hardware flow control
enabled.

Signed-off-by: Romain Izard 
---
 drivers/tty/serial/atmel_serial.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c 
b/drivers/tty/serial/atmel_serial.c
index fabbe76203bb..6684456dca9e 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1758,7 +1758,9 @@ static void atmel_get_ip_name(struct uart_port *port)
 
/*
 * Only USART devices from at91sam9260 SOC implement fractional
-* baudrate.
+* baudrate. It is available for all asynchronous modes, with the
+* following restriction: the sampling clock's duty cycle is not
+* constant.
 */
atmel_port->has_frac_baudrate = false;
atmel_port->has_hw_timer = false;
@@ -2202,8 +2204,7 @@ static void atmel_set_termios(struct uart_port *port, 
struct ktermios *termios,
 * then
 * 8 CD + FP = selected clock / (2 * baudrate)
 */
-   if (atmel_port->has_frac_baudrate &&
-   (mode & ATMEL_US_USMODE) == ATMEL_US_USMODE_NORMAL) {
+   if (atmel_port->has_frac_baudrate) {
div = DIV_ROUND_CLOSEST(port->uartclk, baud * 2);
cd = div >> 3;
fp = div & ATMEL_US_FP_MASK;
-- 
2.9.3

Re: Fractional divider on the Atmel USART controller

[Romain Izard]

2017-02-06 16:14 GMT+01:00 Romain Izard <romain.izard@gmail.com>:
>
> For verification, I checked the SAMA5D2 datasheet, and (in rev. D) the
> warning is still present in §41.7.1.2.

It looks like I got confused with between the versions. In fact, the warning
disappeared between versions C & D, and the last version (E) does not
have it.

-- 
Romain Izard

Re: Fractional divider on the Atmel USART controller

[Romain Izard]

2017-02-06 16:14 GMT+01:00 Romain Izard :
>
> For verification, I checked the SAMA5D2 datasheet, and (in rev. D) the
> warning is still present in §41.7.1.2.

It looks like I got confused with between the versions. In fact, the warning
disappeared between versions C & D, and the last version (E) does not
have it.

-- 
Romain Izard

Re: Fractional divider on the Atmel USART controller

[Romain Izard]

2017-02-06 14:42 GMT+01:00 Ludovic Desroches <ludovic.desroc...@microchip.com>:
> Hello Romain,
>
> On Mon, Feb 06, 2017 at 12:56:42PM +0100, Romain Izard wrote:
>>
>> Unfortunately, I know that this will work on SAMA5D2, but this driver is
>> used for many other Atmel chips. I do not know if the existing code is
>> meant to respect a known limitation on other devices that use the same
>> controller, or if it is just a bug.
>
> It depends on the device used. In the SAMA5D4 datasheet, it is
> mentionned: "This feature is only available when using USART normal mode".
>
> In the SAMA5D2 datasheet, this constraint is no longer mentionned you
> confirm that is works, that is great.
>

For verification, I checked the SAMA5D2 datasheet, and (in rev. D) the
warning is still present in §41.7.1.2. Unfortunately, I did not test the
CTS/RTS mode directly, and I would not go as far as claiming that it
works without any reservation.

In my original use case, the hardware handshaking was activated
automatically when loading the HCILL line discipline to access the
Bluetooth part of a TI WL1831 chip. Fixing the baud rate issue was
enough to restore the Bluetooth stack to the working state I had when I
used the linux4sam 5.3 branch, but I am not sure of how the handshaking
is really used in practice.

Best regards,
-- 
Romain Izard

Re: Fractional divider on the Atmel USART controller

[Romain Izard]

2017-02-06 14:42 GMT+01:00 Ludovic Desroches :
> Hello Romain,
>
> On Mon, Feb 06, 2017 at 12:56:42PM +0100, Romain Izard wrote:
>>
>> Unfortunately, I know that this will work on SAMA5D2, but this driver is
>> used for many other Atmel chips. I do not know if the existing code is
>> meant to respect a known limitation on other devices that use the same
>> controller, or if it is just a bug.
>
> It depends on the device used. In the SAMA5D4 datasheet, it is
> mentionned: "This feature is only available when using USART normal mode".
>
> In the SAMA5D2 datasheet, this constraint is no longer mentionned you
> confirm that is works, that is great.
>

For verification, I checked the SAMA5D2 datasheet, and (in rev. D) the
warning is still present in §41.7.1.2. Unfortunately, I did not test the
CTS/RTS mode directly, and I would not go as far as claiming that it
works without any reservation.

In my original use case, the hardware handshaking was activated
automatically when loading the HCILL line discipline to access the
Bluetooth part of a TI WL1831 chip. Fixing the baud rate issue was
enough to restore the Bluetooth stack to the working state I had when I
used the linux4sam 5.3 branch, but I am not sure of how the handshaking
is really used in practice.

Best regards,
-- 
Romain Izard

Fractional divider on the Atmel USART controller

[Romain Izard]

Hello,

On Atmel SAMA5D2, when trying to configure a serial port for 3 Mbauds
operation, I do not always get the requested baud rate. If the hardware
flow control is disabled by software, the line works correctly. But if I
set the crtscts option, the line does not work, and after checking the
line I can observe that the signal is sent at 2.6 Mbauds.

This is due to the code used to manage fractional baud rate divisor: the
existing code prevents the fractional bits from being used if the line
is not configured in normal mode. This case occurs when the hardware
flow control or the RS485 mode is set.

If I apply the following patch to drivers/tty/serial/atmel_serial.c,
I get the required baudrate.

8<

@@ -2204,14 +2204,13
  * baudrate = selected clock / (8 * (2 - OVER) * (CD + FP / 8))
  * Currently, OVER is always set to 0 so we get
  * baudrate = selected clock / (16 * (CD + FP / 8))
  * then
  * 8 CD + FP = selected clock / (2 * baudrate)
  */
-if (atmel_port->has_frac_baudrate &&
-(mode & ATMEL_US_USMODE) == ATMEL_US_USMODE_NORMAL) {
+if (atmel_port->has_frac_baudrate) {
 div = DIV_ROUND_CLOSEST(port->uartclk, baud * 2);
 cd = div >> 3;
 fp = div & ATMEL_US_FP_MASK;
 } else {
 cd = uart_get_divisor(port, baud);
 }

8<

Unfortunately, I know that this will work on SAMA5D2, but this driver is
used for many other Atmel chips. I do not know if the existing code is
meant to respect a known limitation on other devices that use the same
controller, or if it is just a bug.

Ludovic, Nicolas,  what is your opinion on that matter? Should I just
propose this as a patch, or is it necessary to add a limitation for
supported devices only ?

Best regards,
-- 
Romain Izard

Fractional divider on the Atmel USART controller

[Romain Izard]

Hello,

On Atmel SAMA5D2, when trying to configure a serial port for 3 Mbauds
operation, I do not always get the requested baud rate. If the hardware
flow control is disabled by software, the line works correctly. But if I
set the crtscts option, the line does not work, and after checking the
line I can observe that the signal is sent at 2.6 Mbauds.

This is due to the code used to manage fractional baud rate divisor: the
existing code prevents the fractional bits from being used if the line
is not configured in normal mode. This case occurs when the hardware
flow control or the RS485 mode is set.

If I apply the following patch to drivers/tty/serial/atmel_serial.c,
I get the required baudrate.

8<

@@ -2204,14 +2204,13
  * baudrate = selected clock / (8 * (2 - OVER) * (CD + FP / 8))
  * Currently, OVER is always set to 0 so we get
  * baudrate = selected clock / (16 * (CD + FP / 8))
  * then
  * 8 CD + FP = selected clock / (2 * baudrate)
  */
-if (atmel_port->has_frac_baudrate &&
-(mode & ATMEL_US_USMODE) == ATMEL_US_USMODE_NORMAL) {
+if (atmel_port->has_frac_baudrate) {
 div = DIV_ROUND_CLOSEST(port->uartclk, baud * 2);
 cd = div >> 3;
 fp = div & ATMEL_US_FP_MASK;
 } else {
 cd = uart_get_divisor(port, baud);
 }

8<

Unfortunately, I know that this will work on SAMA5D2, but this driver is
used for many other Atmel chips. I do not know if the existing code is
meant to respect a known limitation on other devices that use the same
controller, or if it is just a bug.

Ludovic, Nicolas,  what is your opinion on that matter? Should I just
propose this as a patch, or is it necessary to add a limitation for
supported devices only ?

Best regards,
-- 
Romain Izard

Re: [PATCH] tcb_clksrc: Use 32 bit tcb as sched_clock

[Romain Izard]

On 2017-01-11, David Engraf <david.eng...@sysgo.com> wrote:
> On newer boards the TC can be read as single 32 bit value without locking.
> Thus the clock can be used as reference for sched_clock which is much more
> accurate than the jiffies implementation.
>
> Tested on a Atmel SAMA5D2 board.
>
> Signed-off-by: David Engraf <david.eng...@sysgo.com>

Unfortunately, this leads to the current boot warning:

[ cut here ]
WARNING: CPU: 0 PID: 1 at ../kernel/time/sched_clock.c:179 
sched_clock_register+0x4c/0x21c
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.4-00041-ge780a8100f0d #1
Hardware name: Atmel SAMA5
[] (unwind_backtrace) from [] (show_stack+0x20/0x24)
[] (show_stack) from [] (dump_stack+0x24/0x28)
[] (dump_stack) from [] (__warn+0xf4/0x10c)
[] (__warn) from [] (warn_slowpath_null+0x30/0x38)
[] (warn_slowpath_null) from [] 
(sched_clock_register+0x4c/0x21c)
[] (sched_clock_register) from [] 
(tcb_clksrc_init+0x1c8/0x424)
[] (tcb_clksrc_init) from [] (do_one_initcall+0x50/0x184)
[] (do_one_initcall) from [] 
(kernel_init_freeable+0x13c/0x1e0)
[] (kernel_init_freeable) from [] (kernel_init+0x18/0x124)
[] (kernel_init) from [] (ret_from_fork+0x14/0x20)
---[ end trace 3d13186881cd5c91 ]---

"sched_clock_register" expects to be called with interrupts disabled, but
the tcb_clksrc initialization is called as an arch_initcall, which runs too
late in the boot sequence.

Best regards,
-- 
Romain Izard

Re: [PATCH] tcb_clksrc: Use 32 bit tcb as sched_clock

[Romain Izard]

On 2017-01-11, David Engraf  wrote:
> On newer boards the TC can be read as single 32 bit value without locking.
> Thus the clock can be used as reference for sched_clock which is much more
> accurate than the jiffies implementation.
>
> Tested on a Atmel SAMA5D2 board.
>
> Signed-off-by: David Engraf 

Unfortunately, this leads to the current boot warning:

[ cut here ]
WARNING: CPU: 0 PID: 1 at ../kernel/time/sched_clock.c:179 
sched_clock_register+0x4c/0x21c
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.4-00041-ge780a8100f0d #1
Hardware name: Atmel SAMA5
[] (unwind_backtrace) from [] (show_stack+0x20/0x24)
[] (show_stack) from [] (dump_stack+0x24/0x28)
[] (dump_stack) from [] (__warn+0xf4/0x10c)
[] (__warn) from [] (warn_slowpath_null+0x30/0x38)
[] (warn_slowpath_null) from [] 
(sched_clock_register+0x4c/0x21c)
[] (sched_clock_register) from [] 
(tcb_clksrc_init+0x1c8/0x424)
[] (tcb_clksrc_init) from [] (do_one_initcall+0x50/0x184)
[] (do_one_initcall) from [] 
(kernel_init_freeable+0x13c/0x1e0)
[] (kernel_init_freeable) from [] (kernel_init+0x18/0x124)
[] (kernel_init) from [] (ret_from_fork+0x14/0x20)
---[ end trace 3d13186881cd5c91 ]---

"sched_clock_register" expects to be called with interrupts disabled, but
the tcb_clksrc initialization is called as an arch_initcall, which runs too
late in the boot sequence.

Best regards,
-- 
Romain Izard

Re: [PATCH v2] usb: gadget: configfs: log function unbinding as debug

[Romain Izard]

2016-08-29 11:07 GMT+02:00 Romain Izard <romain.izard@gmail.com>:
> Disabling USB gadget functions configured through configfs is something
> that can happen in normal use cases. Keep the existing log for this type
> of event, but only as debug, not as an error.
>
> Signed-off-by: Romain Izard <romain.izard@gmail.com>
> ---
> v1 -> v2:
> - use dev_dbg instead of dev_info
>
>  drivers/usb/gadget/configfs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index f9237fe2be05..3984787f8e97 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -1211,8 +1211,9 @@ static void purge_configs_funcs(struct gadget_info *gi)
>
> list_move_tail(>list, >func_list);
> if (f->unbind) {
> -   dev_err(>cdev.gadget->dev, "unbind 
> function"
> -   " '%s'/%p\n", f->name, f);
> +   dev_dbg(>cdev.gadget->dev,
> +"unbind function '%s'/%p\n",
> +f->name, f);
> f->unbind(c, f);
> }
> }
> --
> 2.7.4
>

Ping ?

Re: [PATCH v2] usb: gadget: configfs: log function unbinding as debug

[Romain Izard]

2016-08-29 11:07 GMT+02:00 Romain Izard :
> Disabling USB gadget functions configured through configfs is something
> that can happen in normal use cases. Keep the existing log for this type
> of event, but only as debug, not as an error.
>
> Signed-off-by: Romain Izard 
> ---
> v1 -> v2:
> - use dev_dbg instead of dev_info
>
>  drivers/usb/gadget/configfs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index f9237fe2be05..3984787f8e97 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -1211,8 +1211,9 @@ static void purge_configs_funcs(struct gadget_info *gi)
>
> list_move_tail(>list, >func_list);
> if (f->unbind) {
> -   dev_err(>cdev.gadget->dev, "unbind 
> function"
> -   " '%s'/%p\n", f->name, f);
> +   dev_dbg(>cdev.gadget->dev,
> +"unbind function '%s'/%p\n",
> +f->name, f);
> f->unbind(c, f);
> }
> }
> --
> 2.7.4
>

Ping ?

[PATCH v2] usb: gadget: configfs: log function unbinding as debug

[Romain Izard]

Disabling USB gadget functions configured through configfs is something
that can happen in normal use cases. Keep the existing log for this type
of event, but only as debug, not as an error.

Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
v1 -> v2:
- use dev_dbg instead of dev_info

 drivers/usb/gadget/configfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index f9237fe2be05..3984787f8e97 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1211,8 +1211,9 @@ static void purge_configs_funcs(struct gadget_info *gi)
 
list_move_tail(>list, >func_list);
if (f->unbind) {
-   dev_err(>cdev.gadget->dev, "unbind function"
-   " '%s'/%p\n", f->name, f);
+   dev_dbg(>cdev.gadget->dev,
+"unbind function '%s'/%p\n",
+f->name, f);
f->unbind(c, f);
}
}
-- 
2.7.4

[PATCH v2] usb: gadget: configfs: log function unbinding as debug

[Romain Izard]

Disabling USB gadget functions configured through configfs is something
that can happen in normal use cases. Keep the existing log for this type
of event, but only as debug, not as an error.

Signed-off-by: Romain Izard 
---
v1 -> v2:
- use dev_dbg instead of dev_info

 drivers/usb/gadget/configfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index f9237fe2be05..3984787f8e97 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1211,8 +1211,9 @@ static void purge_configs_funcs(struct gadget_info *gi)
 
list_move_tail(>list, >func_list);
if (f->unbind) {
-   dev_err(>cdev.gadget->dev, "unbind function"
-   " '%s'/%p\n", f->name, f);
+   dev_dbg(>cdev.gadget->dev,
+"unbind function '%s'/%p\n",
+f->name, f);
f->unbind(c, f);
}
}
-- 
2.7.4

Re: [PATCH v1] usb: gadget: configfs: log function unbinding as information

[Romain Izard]

2016-08-29 10:13 GMT+02:00 Felipe Balbi <ba...@kernel.org>:
>
> Hi,
>
> Romain Izard <romain.izard@gmail.com> writes:
>> Disabling USB gadget functions configured through configfs is something
>> that can happen in normal use cases. Keep the existing log for this type
>> of event, but only as information, not as an error.
>>
>> Signed-off-by: Romain Izard <romain.izard@gmail.com>
>> ---
>>  drivers/usb/gadget/configfs.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> index 70cf3477f951..11f3a649d9e5 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -1211,8 +1211,9 @@ static void purge_configs_funcs(struct gadget_info *gi)
>>
>>   list_move_tail(>list, >func_list);
>>   if (f->unbind) {
>> - dev_err(>cdev.gadget->dev, "unbind 
>> function"
>> - " '%s'/%p\n", f->name, f);
>> + dev_info(>cdev.gadget->dev,
>
> seems to me dev_dbg() is a far better alternative. We really don't need
> this on everybody's log buffer unless they really _are_ debugging some
> possible issues.

OK, I'll send a v2.

-- 
Romain Izard

Re: [PATCH v1] usb: gadget: configfs: log function unbinding as information

[Romain Izard]

2016-08-29 10:13 GMT+02:00 Felipe Balbi :
>
> Hi,
>
> Romain Izard  writes:
>> Disabling USB gadget functions configured through configfs is something
>> that can happen in normal use cases. Keep the existing log for this type
>> of event, but only as information, not as an error.
>>
>> Signed-off-by: Romain Izard 
>> ---
>>  drivers/usb/gadget/configfs.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> index 70cf3477f951..11f3a649d9e5 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -1211,8 +1211,9 @@ static void purge_configs_funcs(struct gadget_info *gi)
>>
>>   list_move_tail(>list, >func_list);
>>   if (f->unbind) {
>> - dev_err(>cdev.gadget->dev, "unbind 
>> function"
>> - " '%s'/%p\n", f->name, f);
>> + dev_info(>cdev.gadget->dev,
>
> seems to me dev_dbg() is a far better alternative. We really don't need
> this on everybody's log buffer unless they really _are_ debugging some
> possible issues.

OK, I'll send a v2.

-- 
Romain Izard

Re: [PATCH v1] usb: gadget: configfs: log function unbinding as information

[Romain Izard]

2016-07-26 18:21 GMT+02:00 Romain Izard <romain.izard@gmail.com>:
> Disabling USB gadget functions configured through configfs is something
> that can happen in normal use cases. Keep the existing log for this type
> of event, but only as information, not as an error.
>
> Signed-off-by: Romain Izard <romain.izard@gmail.com>
> ---
>  drivers/usb/gadget/configfs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 70cf3477f951..11f3a649d9e5 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -1211,8 +1211,9 @@ static void purge_configs_funcs(struct gadget_info *gi)
>
> list_move_tail(>list, >func_list);
> if (f->unbind) {
> -   dev_err(>cdev.gadget->dev, "unbind 
> function"
> -   " '%s'/%p\n", f->name, f);
> +   dev_info(>cdev.gadget->dev,
> +"unbind function '%s'/%p\n",
> +f->name, f);
> f->unbind(c, f);
> }
> }
> --
> 2.7.4
>

Ping ?

Re: [PATCH v1] usb: gadget: configfs: log function unbinding as information

[Romain Izard]

2016-07-26 18:21 GMT+02:00 Romain Izard :
> Disabling USB gadget functions configured through configfs is something
> that can happen in normal use cases. Keep the existing log for this type
> of event, but only as information, not as an error.
>
> Signed-off-by: Romain Izard 
> ---
>  drivers/usb/gadget/configfs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 70cf3477f951..11f3a649d9e5 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -1211,8 +1211,9 @@ static void purge_configs_funcs(struct gadget_info *gi)
>
> list_move_tail(>list, >func_list);
> if (f->unbind) {
> -   dev_err(>cdev.gadget->dev, "unbind 
> function"
> -   " '%s'/%p\n", f->name, f);
> +   dev_info(>cdev.gadget->dev,
> +"unbind function '%s'/%p\n",
> +f->name, f);
> f->unbind(c, f);
> }
> }
> --
> 2.7.4
>

Ping ?

[PATCH v1] usb: gadget: configfs: log function unbinding as information

[Romain Izard]

Disabling USB gadget functions configured through configfs is something
that can happen in normal use cases. Keep the existing log for this type
of event, but only as information, not as an error.

Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
 drivers/usb/gadget/configfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 70cf3477f951..11f3a649d9e5 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1211,8 +1211,9 @@ static void purge_configs_funcs(struct gadget_info *gi)
 
list_move_tail(>list, >func_list);
if (f->unbind) {
-   dev_err(>cdev.gadget->dev, "unbind function"
-   " '%s'/%p\n", f->name, f);
+   dev_info(>cdev.gadget->dev,
+"unbind function '%s'/%p\n",
+f->name, f);
f->unbind(c, f);
}
}
-- 
2.7.4

[PATCH v1] usb: gadget: configfs: log function unbinding as information

[Romain Izard]

Disabling USB gadget functions configured through configfs is something
that can happen in normal use cases. Keep the existing log for this type
of event, but only as information, not as an error.

Signed-off-by: Romain Izard 
---
 drivers/usb/gadget/configfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 70cf3477f951..11f3a649d9e5 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1211,8 +1211,9 @@ static void purge_configs_funcs(struct gadget_info *gi)
 
list_move_tail(>list, >func_list);
if (f->unbind) {
-   dev_err(>cdev.gadget->dev, "unbind function"
-   " '%s'/%p\n", f->name, f);
+   dev_info(>cdev.gadget->dev,
+"unbind function '%s'/%p\n",
+f->name, f);
f->unbind(c, f);
}
}
-- 
2.7.4

Re: [PATCH 1/2] Revert "mtd: atmel_nand: Support variable RB_EDGE interrupts"

[Romain Izard]

Hi Boris,

2016-05-10 10:55 GMT+02:00 Boris Brezillon <boris.brezil...@free-electrons.com>:
>
> Romain, I thought you had a real use case on sama5d4 where this patch
> was needed to make the whole thing work. Not sure why you submitted
> this patch if you couldn't test it on a real board.

My target CPU is SAMA5D2, but I chose the SAMA5D4 for the compatible
string as it was the earliest design whose datasheet mentioned the
RB_EDGE3 bit.

I wrote the SAMA5D2 support in advance, as I was waiting for my board to
be ready, basing myself on what was found in the datasheet.  The changes
included both the variable RB_EDGE support, and the PMECC register
layout changes, which were a prerequisite to use the SAMA5D2 NAND
controller.

I tested the code against regressions on sama5d3xek, and Wenyou reported
that he tested it on Atmel's SAMA5D2 PTC, which was the only existing
board at that time with both a SAMA5D2 SoC and a NAND chip.
Unfortunately, as the bug is only seen when writing on the flash, and it
only affects the speed of the device, he did not notice it.

I sent the patches early because I expected the submission process to
be long, and I wanted to be able to freeze the kernel version to be used
on my board as soon as possible.

Best regards,
-- 
Romain Izard

Re: [PATCH 1/2] Revert "mtd: atmel_nand: Support variable RB_EDGE interrupts"

[Romain Izard]

Hi Boris,

2016-05-10 10:55 GMT+02:00 Boris Brezillon :
>
> Romain, I thought you had a real use case on sama5d4 where this patch
> was needed to make the whole thing work. Not sure why you submitted
> this patch if you couldn't test it on a real board.

My target CPU is SAMA5D2, but I chose the SAMA5D4 for the compatible
string as it was the earliest design whose datasheet mentioned the
RB_EDGE3 bit.

I wrote the SAMA5D2 support in advance, as I was waiting for my board to
be ready, basing myself on what was found in the datasheet.  The changes
included both the variable RB_EDGE support, and the PMECC register
layout changes, which were a prerequisite to use the SAMA5D2 NAND
controller.

I tested the code against regressions on sama5d3xek, and Wenyou reported
that he tested it on Atmel's SAMA5D2 PTC, which was the only existing
board at that time with both a SAMA5D2 SoC and a NAND chip.
Unfortunately, as the bug is only seen when writing on the flash, and it
only affects the speed of the device, he did not notice it.

I sent the patches early because I expected the submission process to
be long, and I wanted to be able to freeze the kernel version to be used
on my board as soon as possible.

Best regards,
-- 
Romain Izard

Re: [PATCH] ARM: dts: at91: sama5d2: use "atmel,sama5d3-nfc" compatible for nfc

[Romain Izard]

2016-05-09 18:34 GMT+02:00 Nicolas Ferre <nicolas.fe...@atmel.com>:
> From: Wenyou Yang <wenyou.y...@atmel.com>
>
> An error in documentation of the NAND Flash Controller (NFC) led to choose
> another compatibility string for sama5d2 with an impact on the NAND flash
> ready/busy information. It was producing the error message:
>
> atmel_nand 8000.nand: Time out to wait for interrupt: 0x0800
>
> and had an impact on performance.
>
> So, switch back to the classical "atmel,sama5d3-nfc" compatibility string for
> this SoC which gives the proper ready/busy bit information. The NAND flash
> driver will be updated to remove the support for this different
> implementation.
>
> Signed-off-by: Wenyou Yang <wenyou.y...@atmel.com>
> [nicolas.fe...@atmel.com: change commit message]
> Signed-off-by: Nicolas Ferre <nicolas.fe...@atmel.com>

Acked-by: Romain Izard <romain.izard@gmail.com>

> ---
> Wenyou, Romain,
>
> I plan to send this patch for 4.6-rc as a fix tomorrow (my time). Can you
> please tell me if it's okay on your side.
> I revised the commit message just to justify the very late "fix" sending of
> this patch...
>
> Thanks for your help, bye.
>   Nicolas.
>
>
>  arch/arm/boot/dts/sama5d2.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
> index 78996bdbd3df..9817090c1b73 100644
> --- a/arch/arm/boot/dts/sama5d2.dtsi
> +++ b/arch/arm/boot/dts/sama5d2.dtsi
> @@ -280,7 +280,7 @@
> status = "disabled";
>
> nfc@c000 {
> -   compatible = "atmel,sama5d4-nfc";
> +   compatible = "atmel,sama5d3-nfc";
> #address-cells = <1>;
> #size-cells = <1>;
> reg = < /* NFC Command Registers */
> --
> 2.1.3
>

Re: [PATCH] ARM: dts: at91: sama5d2: use "atmel,sama5d3-nfc" compatible for nfc

[Romain Izard]

2016-05-09 18:34 GMT+02:00 Nicolas Ferre :
> From: Wenyou Yang 
>
> An error in documentation of the NAND Flash Controller (NFC) led to choose
> another compatibility string for sama5d2 with an impact on the NAND flash
> ready/busy information. It was producing the error message:
>
> atmel_nand 8000.nand: Time out to wait for interrupt: 0x0800
>
> and had an impact on performance.
>
> So, switch back to the classical "atmel,sama5d3-nfc" compatibility string for
> this SoC which gives the proper ready/busy bit information. The NAND flash
> driver will be updated to remove the support for this different
> implementation.
>
> Signed-off-by: Wenyou Yang 
> [nicolas.fe...@atmel.com: change commit message]
> Signed-off-by: Nicolas Ferre 

Acked-by: Romain Izard 

> ---
> Wenyou, Romain,
>
> I plan to send this patch for 4.6-rc as a fix tomorrow (my time). Can you
> please tell me if it's okay on your side.
> I revised the commit message just to justify the very late "fix" sending of
> this patch...
>
> Thanks for your help, bye.
>   Nicolas.
>
>
>  arch/arm/boot/dts/sama5d2.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
> index 78996bdbd3df..9817090c1b73 100644
> --- a/arch/arm/boot/dts/sama5d2.dtsi
> +++ b/arch/arm/boot/dts/sama5d2.dtsi
> @@ -280,7 +280,7 @@
> status = "disabled";
>
> nfc@c000 {
> -   compatible = "atmel,sama5d4-nfc";
> +   compatible = "atmel,sama5d3-nfc";
> #address-cells = <1>;
> #size-cells = <1>;
> reg = < /* NFC Command Registers */
> --
> 2.1.3
>

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

Hi Lothar,

2016-03-04 15:59 GMT+01:00 Lothar Waßmann <l...@karo-electronics.de>:
>> >>>>> I also check the WDT_MR register before and after enabling
>> >>>>> watchdog, the WDV and WDD fields are correct.
>> >>>>>
>> >>>>> Can you check it again? thank you.
>> >>>
>> >>>
>> >>> Working case:
>> >>> MR on kernel startup:   0x3fffafff
>> >>> MR after watchdog init: 0x0fffafff
>> >>> MR after start: 0x0fff2fff
>> >>>
>> >>> Problem case:
>> >>> MR on kernel startup:   0x8000
>> >>> MR after watchdog init: 0x0fffafff
>> >>> MR after start: 0x0fff2fff
>> >>>
>> >>> So this means that the counter reload does not seem to work very well
>> >>> if WDD/WDV have been set to 0 in the past. The other question is why
>> >>> does U-Boot (from the Atmel branch based on 2015.1) put this stange
>> >>> value in this register.
>> >>>
>> >>
>> >> Can you check the value of AT91_WDT_SR ? Maybe it tells us something.
>> >>
>> > I didn't report it because it contained 0 at all times. So no information.
>> >
>> >> Also, in the error case, can you check if the watchdog times out at all
>> >> after you applied your patch ?
>> >
>> > It times out after 16s as expected, and reboot occurs correctly.
>> >
>>
>> Interesting. So it looks like AT91_WDT_WDRSTT has to be set if the timer
>> values in MR are changed from 0 to another value, or maybe after each
>> timer value change. Wonder if that should be done in the init function,
>> after MR is set (with the watchdog disabled).
>>
>> Thoughts, anyone ?
>>

> Are you aware of the Notes in the SAMA5D4 Reference Manual (Chapter
> 19.5.2 Watchdog Timer Mode Register):
>
> |Note: The first write access prevents any further modification of
> |  the value of this register. Read accesses remain possible.
> |Note: The WDD and WDV values must not be modified within three slow
> |  clock periods following a restart of the watchdog performed by
> |  a write access in WDT_CR. Any modification will cause the watchdog
> |  to trigger an end of period earlier than expected.

This text is valid for older versions of the Watchdog controller, found
in AT91SAM9 and SAMA5D3 chips. But SAMA5D4 & SAMA5D2 have a newer
revision, which supports multiple writes to the MR register.

Are you sure about your datasheet? I have this in the latest version
found on Atmel's site.

> Atmel-11238B-ATARM-SAMA5D4-Datasheet_24-Aug-15
> Section 18.5.2
>
> Note: Write access to this register has no effect if the LOCKMR
>   command is issued in WDT_CR (unlocked on hardware reset).
> Note: The WDT_MR register values must not be modified within three slow
>   clock periods following a restart of the watchdog performed by
>   a write access in WDT_CR. Any modification will cause the watchdog
>   to trigger an end of period earlier than expected.
>

It matches the comments from Wenyou when he committed the sama5d4
watchdog driver to replace the existing at91sam9 watchdog.

Best regards,
-- 
Romain Izard

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

Hi Lothar,

2016-03-04 15:59 GMT+01:00 Lothar Waßmann :
>> >>>>> I also check the WDT_MR register before and after enabling
>> >>>>> watchdog, the WDV and WDD fields are correct.
>> >>>>>
>> >>>>> Can you check it again? thank you.
>> >>>
>> >>>
>> >>> Working case:
>> >>> MR on kernel startup:   0x3fffafff
>> >>> MR after watchdog init: 0x0fffafff
>> >>> MR after start: 0x0fff2fff
>> >>>
>> >>> Problem case:
>> >>> MR on kernel startup:   0x8000
>> >>> MR after watchdog init: 0x0fffafff
>> >>> MR after start: 0x0fff2fff
>> >>>
>> >>> So this means that the counter reload does not seem to work very well
>> >>> if WDD/WDV have been set to 0 in the past. The other question is why
>> >>> does U-Boot (from the Atmel branch based on 2015.1) put this stange
>> >>> value in this register.
>> >>>
>> >>
>> >> Can you check the value of AT91_WDT_SR ? Maybe it tells us something.
>> >>
>> > I didn't report it because it contained 0 at all times. So no information.
>> >
>> >> Also, in the error case, can you check if the watchdog times out at all
>> >> after you applied your patch ?
>> >
>> > It times out after 16s as expected, and reboot occurs correctly.
>> >
>>
>> Interesting. So it looks like AT91_WDT_WDRSTT has to be set if the timer
>> values in MR are changed from 0 to another value, or maybe after each
>> timer value change. Wonder if that should be done in the init function,
>> after MR is set (with the watchdog disabled).
>>
>> Thoughts, anyone ?
>>

> Are you aware of the Notes in the SAMA5D4 Reference Manual (Chapter
> 19.5.2 Watchdog Timer Mode Register):
>
> |Note: The first write access prevents any further modification of
> |  the value of this register. Read accesses remain possible.
> |Note: The WDD and WDV values must not be modified within three slow
> |  clock periods following a restart of the watchdog performed by
> |  a write access in WDT_CR. Any modification will cause the watchdog
> |  to trigger an end of period earlier than expected.

This text is valid for older versions of the Watchdog controller, found
in AT91SAM9 and SAMA5D3 chips. But SAMA5D4 & SAMA5D2 have a newer
revision, which supports multiple writes to the MR register.

Are you sure about your datasheet? I have this in the latest version
found on Atmel's site.

> Atmel-11238B-ATARM-SAMA5D4-Datasheet_24-Aug-15
> Section 18.5.2
>
> Note: Write access to this register has no effect if the LOCKMR
>   command is issued in WDT_CR (unlocked on hardware reset).
> Note: The WDT_MR register values must not be modified within three slow
>   clock periods following a restart of the watchdog performed by
>   a write access in WDT_CR. Any modification will cause the watchdog
>   to trigger an end of period earlier than expected.
>

It matches the comments from Wenyou when he committed the sama5d4
watchdog driver to replace the existing at91sam9 watchdog.

Best regards,
-- 
Romain Izard

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

2016-03-04 14:09 GMT+01:00 Guenter Roeck <li...@roeck-us.net>:
> On 03/04/2016 01:06 AM, Romain Izard wrote:
>> 2016-03-04 6:23 GMT+01:00 Guenter Roeck <li...@roeck-us.net>:
>>> On 03/03/2016 05:35 PM, Yang, Wenyou wrote:
>>>> On 2016/3/3 18:29, Romain Izard wrote:
>>>>>
>>>>> If the internal counter is not refreshed when the watchdog is
>>>>> started for the first time, the watchdog will trigger very
>>>>> rapidly.  For example, opening /dev/watchdog without writing in it
>>>>> will immediately trigger a reboot, instead of waiting for the
>>>>> delay to expire.
>>>>>
>>>>> To avoid this problem, reload the timer on opening the watchdog
>>>>> device.
>>>>>
>>>>> Command: "while sleep 5; do echo 1; done > /dev/watchdog" Before:
>>>>> system reset After: the watchdog runs correctly
>>>>
>>>>
>>>> I didn't reproduce your issue on my side,
>>>>
>>>> run the your commands as follows, it works fine,  the system reset
>>>> doesn't happen.
>>
>>
>> I've just verified with the factory image provided on the SAMA5D2
>> Xplained board. It does not display this behaviour.
>>
>> But the difference is that in the case without the issue, I'm using
>> the AT91bootstrap SPL, U-Boot, and the kernel from the QSPI chip.
>> When I have the issue, I have a U-Boot based SPL, U-Boot itself and
>> the kernel that come from the FAT partition of an SD-Card.
>>
>> Userspace does not seem to be involved in the issue, as I can
>> reproduce it both with my buildroot environment, and the Yocto
>> environment from the factory image.
>>
>>> Different chip revision ? Different chip type ? Different chip
>>> initialization by ROMMON ?
>>>
>>> Can we get exact chip revisions and types for both cases (working
>>> and not working), and (if it might be relevant) a dump of all
>>> associated chip registers ?
>>
>>
>>
>>>> I also check the WDT_MR register before and after enabling
>>>> watchdog, the WDV and WDD fields are correct.
>>>>
>>>> Can you check it again? thank you.
>>
>>
>> Working case:
>> MR on kernel startup:   0x3fffafff
>> MR after watchdog init: 0x0fffafff
>> MR after start: 0x0fff2fff
>>
>> Problem case:
>> MR on kernel startup:   0x8000
>> MR after watchdog init: 0x0fffafff
>> MR after start: 0x0fff2fff
>>
>> So this means that the counter reload does not seem to work very well
>> if WDD/WDV have been set to 0 in the past. The other question is why
>> does U-Boot (from the Atmel branch based on 2015.1) put this stange
>> value in this register.
>>
>
> Can you check the value of AT91_WDT_SR ? Maybe it tells us something.
>
I didn't report it because it contained 0 at all times. So no information.

> Also, in the error case, can you check if the watchdog times out at all
> after you applied your patch ?

It times out after 16s as expected, and reboot occurs correctly.

-- 
Romain Izard

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

2016-03-04 14:09 GMT+01:00 Guenter Roeck :
> On 03/04/2016 01:06 AM, Romain Izard wrote:
>> 2016-03-04 6:23 GMT+01:00 Guenter Roeck :
>>> On 03/03/2016 05:35 PM, Yang, Wenyou wrote:
>>>> On 2016/3/3 18:29, Romain Izard wrote:
>>>>>
>>>>> If the internal counter is not refreshed when the watchdog is
>>>>> started for the first time, the watchdog will trigger very
>>>>> rapidly.  For example, opening /dev/watchdog without writing in it
>>>>> will immediately trigger a reboot, instead of waiting for the
>>>>> delay to expire.
>>>>>
>>>>> To avoid this problem, reload the timer on opening the watchdog
>>>>> device.
>>>>>
>>>>> Command: "while sleep 5; do echo 1; done > /dev/watchdog" Before:
>>>>> system reset After: the watchdog runs correctly
>>>>
>>>>
>>>> I didn't reproduce your issue on my side,
>>>>
>>>> run the your commands as follows, it works fine,  the system reset
>>>> doesn't happen.
>>
>>
>> I've just verified with the factory image provided on the SAMA5D2
>> Xplained board. It does not display this behaviour.
>>
>> But the difference is that in the case without the issue, I'm using
>> the AT91bootstrap SPL, U-Boot, and the kernel from the QSPI chip.
>> When I have the issue, I have a U-Boot based SPL, U-Boot itself and
>> the kernel that come from the FAT partition of an SD-Card.
>>
>> Userspace does not seem to be involved in the issue, as I can
>> reproduce it both with my buildroot environment, and the Yocto
>> environment from the factory image.
>>
>>> Different chip revision ? Different chip type ? Different chip
>>> initialization by ROMMON ?
>>>
>>> Can we get exact chip revisions and types for both cases (working
>>> and not working), and (if it might be relevant) a dump of all
>>> associated chip registers ?
>>
>>
>>
>>>> I also check the WDT_MR register before and after enabling
>>>> watchdog, the WDV and WDD fields are correct.
>>>>
>>>> Can you check it again? thank you.
>>
>>
>> Working case:
>> MR on kernel startup:   0x3fffafff
>> MR after watchdog init: 0x0fffafff
>> MR after start: 0x0fff2fff
>>
>> Problem case:
>> MR on kernel startup:   0x8000
>> MR after watchdog init: 0x0fffafff
>> MR after start: 0x0fff2fff
>>
>> So this means that the counter reload does not seem to work very well
>> if WDD/WDV have been set to 0 in the past. The other question is why
>> does U-Boot (from the Atmel branch based on 2015.1) put this stange
>> value in this register.
>>
>
> Can you check the value of AT91_WDT_SR ? Maybe it tells us something.
>
I didn't report it because it contained 0 at all times. So no information.

> Also, in the error case, can you check if the watchdog times out at all
> after you applied your patch ?

It times out after 16s as expected, and reboot occurs correctly.

-- 
Romain Izard

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

Hi Wenyou, Guenter,

2016-03-04 6:23 GMT+01:00 Guenter Roeck <li...@roeck-us.net>:
> On 03/03/2016 05:35 PM, Yang, Wenyou wrote:
>> On 2016/3/3 18:29, Romain Izard wrote:
>>>
>>> If the internal counter is not refreshed when the watchdog is
>>> started for the first time, the watchdog will trigger very rapidly.
>>> For example, opening /dev/watchdog without writing in it will
>>> immediately trigger a reboot, instead of waiting for the delay to
>>> expire.
>>>
>>> To avoid this problem, reload the timer on opening the watchdog
>>> device.
>>>
>>> Command: "while sleep 5; do echo 1; done > /dev/watchdog" Before:
>>> system reset After: the watchdog runs correctly
>>
>> I didn't reproduce your issue on my side,
>>
>> run the your commands as follows, it works fine,  the system reset
>> doesn't happen.

I've just verified with the factory image provided on the SAMA5D2
Xplained board. It does not display this behaviour.

But the difference is that in the case without the issue, I'm using the
AT91bootstrap SPL, U-Boot, and the kernel from the QSPI chip. When I
have the issue, I have a U-Boot based SPL, U-Boot itself and the kernel
that come from the FAT partition of an SD-Card.

Userspace does not seem to be involved in the issue, as I can reproduce
it both with my buildroot environment, and the Yocto environment from
the factory image.

> Different chip revision ? Different chip type ? Different chip
> initialization by ROMMON ?
>
> Can we get exact chip revisions and types for both cases (working and
> not working), and (if it might be relevant) a dump of all associated
> chip registers ?


>> I also check the WDT_MR register before and after enabling watchdog,
>> the WDV and WDD fields are correct.
>>
>> Can you check it again? thank you.

Working case:
MR on kernel startup:   0x3fffafff
MR after watchdog init: 0x0fffafff
MR after start: 0x0fff2fff

Problem case:
MR on kernel startup:   0x8000
MR after watchdog init: 0x0fffafff
MR after start: 0x0fff2fff

So this means that the counter reload does not seem to work very well if
WDD/WDV have been set to 0 in the past. The other question is why does
U-Boot (from the Atmel branch based on 2015.1) put this stange value in
this register.

Best regards,
-- 
Romain Izard

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

Hi Wenyou, Guenter,

2016-03-04 6:23 GMT+01:00 Guenter Roeck :
> On 03/03/2016 05:35 PM, Yang, Wenyou wrote:
>> On 2016/3/3 18:29, Romain Izard wrote:
>>>
>>> If the internal counter is not refreshed when the watchdog is
>>> started for the first time, the watchdog will trigger very rapidly.
>>> For example, opening /dev/watchdog without writing in it will
>>> immediately trigger a reboot, instead of waiting for the delay to
>>> expire.
>>>
>>> To avoid this problem, reload the timer on opening the watchdog
>>> device.
>>>
>>> Command: "while sleep 5; do echo 1; done > /dev/watchdog" Before:
>>> system reset After: the watchdog runs correctly
>>
>> I didn't reproduce your issue on my side,
>>
>> run the your commands as follows, it works fine,  the system reset
>> doesn't happen.

I've just verified with the factory image provided on the SAMA5D2
Xplained board. It does not display this behaviour.

But the difference is that in the case without the issue, I'm using the
AT91bootstrap SPL, U-Boot, and the kernel from the QSPI chip. When I
have the issue, I have a U-Boot based SPL, U-Boot itself and the kernel
that come from the FAT partition of an SD-Card.

Userspace does not seem to be involved in the issue, as I can reproduce
it both with my buildroot environment, and the Yocto environment from
the factory image.

> Different chip revision ? Different chip type ? Different chip
> initialization by ROMMON ?
>
> Can we get exact chip revisions and types for both cases (working and
> not working), and (if it might be relevant) a dump of all associated
> chip registers ?


>> I also check the WDT_MR register before and after enabling watchdog,
>> the WDV and WDD fields are correct.
>>
>> Can you check it again? thank you.

Working case:
MR on kernel startup:   0x3fffafff
MR after watchdog init: 0x0fffafff
MR after start: 0x0fff2fff

Problem case:
MR on kernel startup:   0x8000
MR after watchdog init: 0x0fffafff
MR after start: 0x0fff2fff

So this means that the counter reload does not seem to work very well if
WDD/WDV have been set to 0 in the past. The other question is why does
U-Boot (from the Atmel branch based on 2015.1) put this stange value in
this register.

Best regards,
-- 
Romain Izard

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

Hi Guenter,

2016-03-03 13:10 GMT+01:00 Guenter Roeck <li...@roeck-us.net>:
> On 03/03/2016 02:29 AM, Romain Izard wrote:
>>
>> If the internal counter is not refreshed when the watchdog is started
>> for the first time, the watchdog will trigger very rapidly. For
>> example, opening /dev/watchdog without writing in it will immediately
>> trigger a reboot, instead of waiting for the delay to expire.
>>
>> To avoid this problem, reload the timer on opening the watchdog
>> device.
>>
>> Command: "while sleep 5; do echo 1; done > /dev/watchdog"
>> Before: system reset
>> After: the watchdog runs correctly
>>
>> Signed-off-by: Romain Izard <romain.izard@gmail.com>
>
>
> Subject might better read "ping watchdog on start" or similar.
>
OK. I'll change it for a v2.

> Does the watchdog have to be pinged before it is enabled ?  I am a bit
> concerned that there may still be a 125 uS window during which the
> system could restart.
>

According to the SAMA5D2 & SAMA5D4 datasheets, the timer ought to be
reloaded when the watchdog is enabled by a write in the MR register.
Unfortunately, it does not work as described, as I encountered the
problem on a SAMA5D2 Xplained board.

The 4 clock delay is not in the datasheet either, but without any delay
the timer is clearly not reloaded, as my issue stays the same. As there
is a required delay before writing to MR after writing to CR, I applied
the same type of delay in the reverse case.

Perhaps Nicolas or Wenyou have more information on this.

Best regards,
-- 
Romain Izard

Re: [PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

Hi Guenter,

2016-03-03 13:10 GMT+01:00 Guenter Roeck :
> On 03/03/2016 02:29 AM, Romain Izard wrote:
>>
>> If the internal counter is not refreshed when the watchdog is started
>> for the first time, the watchdog will trigger very rapidly. For
>> example, opening /dev/watchdog without writing in it will immediately
>> trigger a reboot, instead of waiting for the delay to expire.
>>
>> To avoid this problem, reload the timer on opening the watchdog
>> device.
>>
>> Command: "while sleep 5; do echo 1; done > /dev/watchdog"
>> Before: system reset
>> After: the watchdog runs correctly
>>
>> Signed-off-by: Romain Izard 
>
>
> Subject might better read "ping watchdog on start" or similar.
>
OK. I'll change it for a v2.

> Does the watchdog have to be pinged before it is enabled ?  I am a bit
> concerned that there may still be a 125 uS window during which the
> system could restart.
>

According to the SAMA5D2 & SAMA5D4 datasheets, the timer ought to be
reloaded when the watchdog is enabled by a write in the MR register.
Unfortunately, it does not work as described, as I encountered the
problem on a SAMA5D2 Xplained board.

The 4 clock delay is not in the datasheet either, but without any delay
the timer is clearly not reloaded, as my issue stays the same. As there
is a required delay before writing to MR after writing to CR, I applied
the same type of delay in the reverse case.

Perhaps Nicolas or Wenyou have more information on this.

Best regards,
-- 
Romain Izard

[PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

If the internal counter is not refreshed when the watchdog is started
for the first time, the watchdog will trigger very rapidly. For example,
opening /dev/watchdog without writing in it will immediately trigger a
reboot, instead of waiting for the delay to expire.

To avoid this problem, reload the timer on opening the watchdog device.

Command: "while sleep 5; do echo 1; done > /dev/watchdog"
Before: system reset
After: the watchdog runs correctly

Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
 drivers/watchdog/sama5d4_wdt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index a49634cdc1cc..e162fe140ae1 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "at91sam9_wdt.h"
 
@@ -58,6 +59,8 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
reg = wdt_read(wdt, AT91_WDT_MR);
reg &= ~AT91_WDT_WDDIS;
wdt_write(wdt, AT91_WDT_MR, reg);
+   udelay(125); /* > 4 cycles at 32,768 Hz */
+   wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
 
return 0;
 }
-- 
2.5.0

[PATCH v1] watchdog: sama5d4_wdt: Reset delay on start

[Romain Izard]

If the internal counter is not refreshed when the watchdog is started
for the first time, the watchdog will trigger very rapidly. For example,
opening /dev/watchdog without writing in it will immediately trigger a
reboot, instead of waiting for the delay to expire.

To avoid this problem, reload the timer on opening the watchdog device.

Command: "while sleep 5; do echo 1; done > /dev/watchdog"
Before: system reset
After: the watchdog runs correctly

Signed-off-by: Romain Izard 
---
 drivers/watchdog/sama5d4_wdt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index a49634cdc1cc..e162fe140ae1 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "at91sam9_wdt.h"
 
@@ -58,6 +59,8 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
reg = wdt_read(wdt, AT91_WDT_MR);
reg &= ~AT91_WDT_WDDIS;
wdt_write(wdt, AT91_WDT_MR, reg);
+   udelay(125); /* > 4 cycles at 32,768 Hz */
+   wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
 
return 0;
 }
-- 
2.5.0

[PATCH v3] tty/serial: at91: restore dynamic driver binding

[Romain Izard]

In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular support
code for atmel_serial was removed, as the driver cannot be built as a
module. Because no use case was proposed, the dynamic driver binding
support was removed as well.

The atmel_serial driver can manage up to 7 serial controllers, which are
multiplexed with other functions. For example, in the Atmel SAMA5D2, the
Flexcom controllers can work as USART, SPI or I2C controllers, and on
all Atmel devices serial lines can be reconfigured as GPIOs.

My use case uses GPIOs to transfer a firmware update using a custom
protocol on the lines used as a serial port during the normal life of
the device. If it is not possible to unbind the atmel_serial driver, the
GPIO lines remain reserved and prevent this case from working.

This patch reinstates the atmel_serial_remove function, and fixes it as
it failed to clear the "clk" field on removal, triggering an oops when
a device was bound again after being unbound.

Acked-by: Nicolas Ferre <nicolas.fe...@atmel.com>
Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
Changelog:
v2: Add the rationale for keeping the "remove" function as a comment.
v3: Do not cleanup the spaces in the driver initializer, as requested by
  the maintainer

 drivers/tty/serial/atmel_serial.c | 35 ++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/atmel_serial.c 
b/drivers/tty/serial/atmel_serial.c
index 1c0884d8ef32..969084455f94 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2759,14 +2759,47 @@ err:
return ret;
 }
 
+/*
+ * Even if the driver is not modular, it makes sense to be able to
+ * unbind a device: there can be many bound devices, and there are
+ * situations where dynamic binding and unbinding can be useful.
+ *
+ * For example, a connected device can require a specific firmware update
+ * protocol that needs bitbanging on IO lines, but use the regular serial
+ * port in the normal case.
+ */
+static int atmel_serial_remove(struct platform_device *pdev)
+{
+   struct uart_port *port = platform_get_drvdata(pdev);
+   struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+   int ret = 0;
+
+   tasklet_kill(_port->tasklet);
+
+   device_init_wakeup(>dev, 0);
+
+   ret = uart_remove_one_port(_uart, port);
+
+   kfree(atmel_port->rx_ring.buf);
+
+   /* "port" is allocated statically, so we shouldn't free it */
+
+   clear_bit(port->line, atmel_ports_in_use);
+
+   clk_put(atmel_port->clk);
+   atmel_port->clk = NULL;
+
+   return ret;
+}
+
 static struct platform_driver atmel_serial_driver = {
.probe  = atmel_serial_probe,
+   .remove = atmel_serial_remove,
.suspend= atmel_serial_suspend,
.resume = atmel_serial_resume,
.driver = {
.name   = "atmel_usart",
.of_match_table = of_match_ptr(atmel_serial_dt_ids),
-   .suppress_bind_attrs= true,
},
 };
 
-- 
2.5.0

[PATCH v3] tty/serial: at91: restore dynamic driver binding

[Romain Izard]

In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular support
code for atmel_serial was removed, as the driver cannot be built as a
module. Because no use case was proposed, the dynamic driver binding
support was removed as well.

The atmel_serial driver can manage up to 7 serial controllers, which are
multiplexed with other functions. For example, in the Atmel SAMA5D2, the
Flexcom controllers can work as USART, SPI or I2C controllers, and on
all Atmel devices serial lines can be reconfigured as GPIOs.

My use case uses GPIOs to transfer a firmware update using a custom
protocol on the lines used as a serial port during the normal life of
the device. If it is not possible to unbind the atmel_serial driver, the
GPIO lines remain reserved and prevent this case from working.

This patch reinstates the atmel_serial_remove function, and fixes it as
it failed to clear the "clk" field on removal, triggering an oops when
a device was bound again after being unbound.

Acked-by: Nicolas Ferre 
Signed-off-by: Romain Izard 
---
Changelog:
v2: Add the rationale for keeping the "remove" function as a comment.
v3: Do not cleanup the spaces in the driver initializer, as requested by
  the maintainer

 drivers/tty/serial/atmel_serial.c | 35 ++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/atmel_serial.c 
b/drivers/tty/serial/atmel_serial.c
index 1c0884d8ef32..969084455f94 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2759,14 +2759,47 @@ err:
return ret;
 }
 
+/*
+ * Even if the driver is not modular, it makes sense to be able to
+ * unbind a device: there can be many bound devices, and there are
+ * situations where dynamic binding and unbinding can be useful.
+ *
+ * For example, a connected device can require a specific firmware update
+ * protocol that needs bitbanging on IO lines, but use the regular serial
+ * port in the normal case.
+ */
+static int atmel_serial_remove(struct platform_device *pdev)
+{
+   struct uart_port *port = platform_get_drvdata(pdev);
+   struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+   int ret = 0;
+
+   tasklet_kill(_port->tasklet);
+
+   device_init_wakeup(>dev, 0);
+
+   ret = uart_remove_one_port(_uart, port);
+
+   kfree(atmel_port->rx_ring.buf);
+
+   /* "port" is allocated statically, so we shouldn't free it */
+
+   clear_bit(port->line, atmel_ports_in_use);
+
+   clk_put(atmel_port->clk);
+   atmel_port->clk = NULL;
+
+   return ret;
+}
+
 static struct platform_driver atmel_serial_driver = {
.probe  = atmel_serial_probe,
+   .remove = atmel_serial_remove,
.suspend= atmel_serial_suspend,
.resume = atmel_serial_resume,
.driver = {
.name   = "atmel_usart",
.of_match_table = of_match_ptr(atmel_serial_dt_ids),
-   .suppress_bind_attrs= true,
},
 };
 
-- 
2.5.0

Re: [PATCH v2] tty/serial: at91: restore dynamic driver binding

[Romain Izard]

2016-02-25 17:09 GMT+01:00 Greg Kroah-Hartman
<gre...@linuxfoundation.org>:
> On Thu, Feb 25, 2016 at 11:01:07AM +0100, Nicolas Ferre wrote:
>> Le 25/02/2016 10:23, Romain Izard a écrit :
>> > In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular
>> > support code for atmel_serial was removed, as the driver cannot be
>> > built as a module. Because no use case was proposed, the dynamic
>> > driver binding support was removed as well.
>> >
>> > The atmel_serial driver can manage up to 7 serial controllers,
>> > which are multiplexed with other functions. For example, in the
>> > Atmel SAMA5D2, the Flexcom controllers can work as USART, SPI or
>> > I2C controllers, and on all Atmel devices serial lines can be
>> > reconfigured as GPIOs.
>> >
>> > My use case uses GPIOs to transfer a firmware update using a custom
>> > protocol on the lines used as a serial port during the normal life
>> > of the device. If it is not possible to unbind the atmel_serial
>> > driver, the GPIO lines remain reserved and prevent this case from
>> > working.
>> >
>> > This patch reinstates the atmel_serial_remove function, and fixes
>> > it as it failed to clear the "clk" field on removal, triggering an
>> > oops when a device was bound again after being unbound.
>> >
>> > Signed-off-by: Romain Izard <romain.izard@gmail.com>
>>
>> Even if you didn't follow my advice for not including unneeded
>> changes in of the last patch chunk, there's no use delaying the patch
>> just for this. So, here is my:
>
> Yes there is, I'm not going to take this, Romain please fix it
> properly.

Are we really arguing about the alignement of of_match_table in the
platform_driver initializer?

Among other things, Paul's patch changed the alignment to match the
width of the "suppress_bind_attrs" member. As I simply used 'git revert
-p' to revert the parts of the patch that bothered me, the alignment
returned to what it was before.

Or am I missing something else ?

-- 
Romain Izard

Re: [PATCH v2] tty/serial: at91: restore dynamic driver binding

[Romain Izard]

2016-02-25 17:09 GMT+01:00 Greg Kroah-Hartman
:
> On Thu, Feb 25, 2016 at 11:01:07AM +0100, Nicolas Ferre wrote:
>> Le 25/02/2016 10:23, Romain Izard a écrit :
>> > In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular
>> > support code for atmel_serial was removed, as the driver cannot be
>> > built as a module. Because no use case was proposed, the dynamic
>> > driver binding support was removed as well.
>> >
>> > The atmel_serial driver can manage up to 7 serial controllers,
>> > which are multiplexed with other functions. For example, in the
>> > Atmel SAMA5D2, the Flexcom controllers can work as USART, SPI or
>> > I2C controllers, and on all Atmel devices serial lines can be
>> > reconfigured as GPIOs.
>> >
>> > My use case uses GPIOs to transfer a firmware update using a custom
>> > protocol on the lines used as a serial port during the normal life
>> > of the device. If it is not possible to unbind the atmel_serial
>> > driver, the GPIO lines remain reserved and prevent this case from
>> > working.
>> >
>> > This patch reinstates the atmel_serial_remove function, and fixes
>> > it as it failed to clear the "clk" field on removal, triggering an
>> > oops when a device was bound again after being unbound.
>> >
>> > Signed-off-by: Romain Izard 
>>
>> Even if you didn't follow my advice for not including unneeded
>> changes in of the last patch chunk, there's no use delaying the patch
>> just for this. So, here is my:
>
> Yes there is, I'm not going to take this, Romain please fix it
> properly.

Are we really arguing about the alignement of of_match_table in the
platform_driver initializer?

Among other things, Paul's patch changed the alignment to match the
width of the "suppress_bind_attrs" member. As I simply used 'git revert
-p' to revert the parts of the patch that bothered me, the alignment
returned to what it was before.

Or am I missing something else ?

-- 
Romain Izard

[PATCH v2] tty/serial: at91: restore dynamic driver binding

[Romain Izard]

In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular support
code for atmel_serial was removed, as the driver cannot be built as a
module. Because no use case was proposed, the dynamic driver binding
support was removed as well.

The atmel_serial driver can manage up to 7 serial controllers, which are
multiplexed with other functions. For example, in the Atmel SAMA5D2, the
Flexcom controllers can work as USART, SPI or I2C controllers, and on
all Atmel devices serial lines can be reconfigured as GPIOs.

My use case uses GPIOs to transfer a firmware update using a custom
protocol on the lines used as a serial port during the normal life of
the device. If it is not possible to unbind the atmel_serial driver, the
GPIO lines remain reserved and prevent this case from working.

This patch reinstates the atmel_serial_remove function, and fixes it as
it failed to clear the "clk" field on removal, triggering an oops when
a device was bound again after being unbound.

Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
Changelog:
v2: Add the rationale for keeping the "remove" function as a comment.


 drivers/tty/serial/atmel_serial.c | 39 ---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c 
b/drivers/tty/serial/atmel_serial.c
index 1c0884d8ef32..b1f3a5b26830 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2759,14 +2759,47 @@ err:
return ret;
 }
 
+/*
+ * Even if the driver is not modular, it makes sense to be able to
+ * unbind a device: there can be many bound devices, and there are
+ * situations where dynamic binding and unbinding can be useful.
+ *
+ * For example, a connected device can require a specific firmware update
+ * protocol that needs bitbanging on IO lines, but use the regular serial
+ * port in the normal case.
+ */
+static int atmel_serial_remove(struct platform_device *pdev)
+{
+   struct uart_port *port = platform_get_drvdata(pdev);
+   struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+   int ret = 0;
+
+   tasklet_kill(_port->tasklet);
+
+   device_init_wakeup(>dev, 0);
+
+   ret = uart_remove_one_port(_uart, port);
+
+   kfree(atmel_port->rx_ring.buf);
+
+   /* "port" is allocated statically, so we shouldn't free it */
+
+   clear_bit(port->line, atmel_ports_in_use);
+
+   clk_put(atmel_port->clk);
+   atmel_port->clk = NULL;
+
+   return ret;
+}
+
 static struct platform_driver atmel_serial_driver = {
.probe  = atmel_serial_probe,
+   .remove = atmel_serial_remove,
.suspend= atmel_serial_suspend,
.resume = atmel_serial_resume,
.driver = {
-   .name   = "atmel_usart",
-   .of_match_table = of_match_ptr(atmel_serial_dt_ids),
-   .suppress_bind_attrs= true,
+   .name   = "atmel_usart",
+   .of_match_table = of_match_ptr(atmel_serial_dt_ids),
},
 };
 
-- 
2.5.0

[PATCH v2] tty/serial: at91: restore dynamic driver binding

[Romain Izard]

In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular support
code for atmel_serial was removed, as the driver cannot be built as a
module. Because no use case was proposed, the dynamic driver binding
support was removed as well.

The atmel_serial driver can manage up to 7 serial controllers, which are
multiplexed with other functions. For example, in the Atmel SAMA5D2, the
Flexcom controllers can work as USART, SPI or I2C controllers, and on
all Atmel devices serial lines can be reconfigured as GPIOs.

My use case uses GPIOs to transfer a firmware update using a custom
protocol on the lines used as a serial port during the normal life of
the device. If it is not possible to unbind the atmel_serial driver, the
GPIO lines remain reserved and prevent this case from working.

This patch reinstates the atmel_serial_remove function, and fixes it as
it failed to clear the "clk" field on removal, triggering an oops when
a device was bound again after being unbound.

Signed-off-by: Romain Izard 
---
Changelog:
v2: Add the rationale for keeping the "remove" function as a comment.


 drivers/tty/serial/atmel_serial.c | 39 ---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c 
b/drivers/tty/serial/atmel_serial.c
index 1c0884d8ef32..b1f3a5b26830 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2759,14 +2759,47 @@ err:
return ret;
 }
 
+/*
+ * Even if the driver is not modular, it makes sense to be able to
+ * unbind a device: there can be many bound devices, and there are
+ * situations where dynamic binding and unbinding can be useful.
+ *
+ * For example, a connected device can require a specific firmware update
+ * protocol that needs bitbanging on IO lines, but use the regular serial
+ * port in the normal case.
+ */
+static int atmel_serial_remove(struct platform_device *pdev)
+{
+   struct uart_port *port = platform_get_drvdata(pdev);
+   struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+   int ret = 0;
+
+   tasklet_kill(_port->tasklet);
+
+   device_init_wakeup(>dev, 0);
+
+   ret = uart_remove_one_port(_uart, port);
+
+   kfree(atmel_port->rx_ring.buf);
+
+   /* "port" is allocated statically, so we shouldn't free it */
+
+   clear_bit(port->line, atmel_ports_in_use);
+
+   clk_put(atmel_port->clk);
+   atmel_port->clk = NULL;
+
+   return ret;
+}
+
 static struct platform_driver atmel_serial_driver = {
.probe  = atmel_serial_probe,
+   .remove = atmel_serial_remove,
.suspend= atmel_serial_suspend,
.resume = atmel_serial_resume,
.driver = {
-   .name   = "atmel_usart",
-   .of_match_table = of_match_ptr(atmel_serial_dt_ids),
-   .suppress_bind_attrs= true,
+   .name   = "atmel_usart",
+   .of_match_table = of_match_ptr(atmel_serial_dt_ids),
},
 };
 
-- 
2.5.0

Re: [PATCH v2] clocksource: atmel-pit: register as a sched_clock

[Romain Izard]

2016-02-24 17:20 GMT+01:00 Sylvain Rochet <sylvain.roc...@finsecur.com>:
> Hi,
>
> On Wed, Feb 24, 2016 at 05:04:42PM +0100, Romain Izard wrote:
>> Register the counter of the Periodic Interval Timer as a possible
>> source for sched_clock. Keep the timer running even if the related
>> clockevent is disabled.
>>
>> This provides a better precision than the jiffies-based default. The
>> TCB clocksource does not work, as it is registered too late in the
>> initialization sequence.
>
> I have mixed feelings about that, but this is probably just a
> misunderstanding from my part.
>
My goal is to get a better precision on my printk and trace logs,
because the precision of jiffies is really very bad compared to
everything else I have encountered until now. But it looks like reading
a timer is quite complicated on AT91...

> The PIT timer should not be used for systems with PM_SUSPEND enabled
> and used because it takes ages to synchronize on resume, how does this
> patch affect that ?
>
> Ref: commit ac34ad27fc ("clockevents: Do not suspend/resume if
> unused")

So your advice would be to stay clear of the PIT, because it is
(globally) useless.

I'll keep it in mind.

Best regards,
-- 
Romain Izard

Re: [PATCH v2] clocksource: atmel-pit: register as a sched_clock

[Romain Izard]

2016-02-24 17:20 GMT+01:00 Sylvain Rochet :
> Hi,
>
> On Wed, Feb 24, 2016 at 05:04:42PM +0100, Romain Izard wrote:
>> Register the counter of the Periodic Interval Timer as a possible
>> source for sched_clock. Keep the timer running even if the related
>> clockevent is disabled.
>>
>> This provides a better precision than the jiffies-based default. The
>> TCB clocksource does not work, as it is registered too late in the
>> initialization sequence.
>
> I have mixed feelings about that, but this is probably just a
> misunderstanding from my part.
>
My goal is to get a better precision on my printk and trace logs,
because the precision of jiffies is really very bad compared to
everything else I have encountered until now. But it looks like reading
a timer is quite complicated on AT91...

> The PIT timer should not be used for systems with PM_SUSPEND enabled
> and used because it takes ages to synchronize on resume, how does this
> patch affect that ?
>
> Ref: commit ac34ad27fc ("clockevents: Do not suspend/resume if
> unused")

So your advice would be to stay clear of the PIT, because it is
(globally) useless.

I'll keep it in mind.

Best regards,
-- 
Romain Izard

[PATCH v2] clocksource: atmel-pit: register as a sched_clock

[Romain Izard]

Register the counter of the Periodic Interval Timer as a possible source
for sched_clock. Keep the timer running even if the related clockevent
is disabled.

This provides a better precision than the jiffies-based default. The
TCB clocksource does not work, as it is registered too late in the
initialization sequence.

Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
Changelog:
v2:
- Keep the tick counter updated when the clocksource is disabled
- Ensure that sched_clock is a 64 bit counter

 drivers/clocksource/timer-atmel-pit.c | 31 ---
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/clocksource/timer-atmel-pit.c 
b/drivers/clocksource/timer-atmel-pit.c
index d911c5dca8f1..609892bbf9da 100644
--- a/drivers/clocksource/timer-atmel-pit.c
+++ b/drivers/clocksource/timer-atmel-pit.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define AT91_PIT_MR0x00/* Mode Register */
 #define AT91_PIT_PITIENBIT(25) /* 
Timer Interrupt Enable */
@@ -44,7 +45,7 @@ struct pit_data {
 
void __iomem*base;
u32 cycle;
-   u32 cnt;
+   u64 cnt;
unsigned intirq;
struct clk  *mck;
 };
@@ -77,7 +78,7 @@ static cycle_t read_pit_clk(struct clocksource *cs)
 {
struct pit_data *data = clksrc_to_pit_data(cs);
unsigned long flags;
-   u32 elapsed;
+   u64 elapsed;
u32 t;
 
raw_local_irq_save(flags);
@@ -90,15 +91,6 @@ static cycle_t read_pit_clk(struct clocksource *cs)
return elapsed;
 }
 
-static int pit_clkevt_shutdown(struct clock_event_device *dev)
-{
-   struct pit_data *data = clkevt_to_pit_data(dev);
-
-   /* disable irq, leaving the clocksource active */
-   pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
-   return 0;
-}
-
 /*
  * Clockevent device:  interrupts every 1/HZ (== pit_cycles * MCK/16)
  */
@@ -156,15 +148,15 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, 
void *dev_id)
WARN_ON_ONCE(!irqs_disabled());
 
/* The PIT interrupt may be disabled, and is shared */
-   if (clockevent_state_periodic(>clkevt) &&
-   (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS)) {
+   if (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS) {
unsigned nr_ticks;
 
/* Get number of ticks performed before irq, and ack it */
nr_ticks = PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
do {
data->cnt += data->cycle;
-   data->clkevt.event_handler(>clkevt);
+   if (clockevent_state_periodic(>clkevt))
+   data->clkevt.event_handler(>clkevt);
nr_ticks--;
} while (nr_ticks);
 
@@ -174,6 +166,13 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void 
*dev_id)
return IRQ_NONE;
 }
 
+static struct clocksource *pit_sched_clock;
+
+static u64 pit_sched_clock_read(void)
+{
+   return read_pit_clk(pit_sched_clock);
+}
+
 /*
  * Set up both clocksource and clockevent support.
  */
@@ -206,6 +205,9 @@ static void __init at91sam926x_pit_common_init(struct 
pit_data *data)
data->clksrc.flags = CLOCK_SOURCE_IS_CONTINUOUS;
clocksource_register_hz(>clksrc, pit_rate);
 
+   pit_sched_clock = >clksrc;
+   sched_clock_register(pit_sched_clock_read, bits, pit_rate);
+
/* Set up irq handler */
ret = request_irq(data->irq, at91sam926x_pit_interrupt,
  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
@@ -221,7 +223,6 @@ static void __init at91sam926x_pit_common_init(struct 
pit_data *data)
data->clkevt.rating = 100;
data->clkevt.cpumask = cpumask_of(0);
 
-   data->clkevt.set_state_shutdown = pit_clkevt_shutdown;
data->clkevt.set_state_periodic = pit_clkevt_set_periodic;
data->clkevt.resume = at91sam926x_pit_resume;
data->clkevt.suspend = at91sam926x_pit_suspend;
-- 
2.5.0

[PATCH v2] clocksource: atmel-pit: register as a sched_clock

[Romain Izard]

Register the counter of the Periodic Interval Timer as a possible source
for sched_clock. Keep the timer running even if the related clockevent
is disabled.

This provides a better precision than the jiffies-based default. The
TCB clocksource does not work, as it is registered too late in the
initialization sequence.

Signed-off-by: Romain Izard 
---
Changelog:
v2:
- Keep the tick counter updated when the clocksource is disabled
- Ensure that sched_clock is a 64 bit counter

 drivers/clocksource/timer-atmel-pit.c | 31 ---
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/clocksource/timer-atmel-pit.c 
b/drivers/clocksource/timer-atmel-pit.c
index d911c5dca8f1..609892bbf9da 100644
--- a/drivers/clocksource/timer-atmel-pit.c
+++ b/drivers/clocksource/timer-atmel-pit.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define AT91_PIT_MR0x00/* Mode Register */
 #define AT91_PIT_PITIENBIT(25) /* 
Timer Interrupt Enable */
@@ -44,7 +45,7 @@ struct pit_data {
 
void __iomem*base;
u32 cycle;
-   u32 cnt;
+   u64 cnt;
unsigned intirq;
struct clk  *mck;
 };
@@ -77,7 +78,7 @@ static cycle_t read_pit_clk(struct clocksource *cs)
 {
struct pit_data *data = clksrc_to_pit_data(cs);
unsigned long flags;
-   u32 elapsed;
+   u64 elapsed;
u32 t;
 
raw_local_irq_save(flags);
@@ -90,15 +91,6 @@ static cycle_t read_pit_clk(struct clocksource *cs)
return elapsed;
 }
 
-static int pit_clkevt_shutdown(struct clock_event_device *dev)
-{
-   struct pit_data *data = clkevt_to_pit_data(dev);
-
-   /* disable irq, leaving the clocksource active */
-   pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
-   return 0;
-}
-
 /*
  * Clockevent device:  interrupts every 1/HZ (== pit_cycles * MCK/16)
  */
@@ -156,15 +148,15 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, 
void *dev_id)
WARN_ON_ONCE(!irqs_disabled());
 
/* The PIT interrupt may be disabled, and is shared */
-   if (clockevent_state_periodic(>clkevt) &&
-   (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS)) {
+   if (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS) {
unsigned nr_ticks;
 
/* Get number of ticks performed before irq, and ack it */
nr_ticks = PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
do {
data->cnt += data->cycle;
-   data->clkevt.event_handler(>clkevt);
+   if (clockevent_state_periodic(>clkevt))
+   data->clkevt.event_handler(>clkevt);
nr_ticks--;
} while (nr_ticks);
 
@@ -174,6 +166,13 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void 
*dev_id)
return IRQ_NONE;
 }
 
+static struct clocksource *pit_sched_clock;
+
+static u64 pit_sched_clock_read(void)
+{
+   return read_pit_clk(pit_sched_clock);
+}
+
 /*
  * Set up both clocksource and clockevent support.
  */
@@ -206,6 +205,9 @@ static void __init at91sam926x_pit_common_init(struct 
pit_data *data)
data->clksrc.flags = CLOCK_SOURCE_IS_CONTINUOUS;
clocksource_register_hz(>clksrc, pit_rate);
 
+   pit_sched_clock = >clksrc;
+   sched_clock_register(pit_sched_clock_read, bits, pit_rate);
+
/* Set up irq handler */
ret = request_irq(data->irq, at91sam926x_pit_interrupt,
  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
@@ -221,7 +223,6 @@ static void __init at91sam926x_pit_common_init(struct 
pit_data *data)
data->clkevt.rating = 100;
data->clkevt.cpumask = cpumask_of(0);
 
-   data->clkevt.set_state_shutdown = pit_clkevt_shutdown;
data->clkevt.set_state_periodic = pit_clkevt_set_periodic;
data->clkevt.resume = at91sam926x_pit_resume;
data->clkevt.suspend = at91sam926x_pit_suspend;
-- 
2.5.0

Re: [PATCH] tty/serial: at91: restore dynamic driver binding

[romain izard]

2016-02-24 15:53 GMT+01:00 Nicolas Ferre <nicolas.fe...@atmel.com>:
> Le 23/02/2016 17:59, Romain Izard a écrit :
>> In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular support
>> code for atmel_serial was removed, as the driver cannot be built as a
>> module. Because no use case was proposed, the dynamic driver binding
>> support was removed as well.
>>
>> The atmel_serial driver can manage up to 7 serial controllers, which are
>> multiplexed with other functions. For example, in the Atmel SAMA5D2, the
>> Flexcom controllers can work as USART, SPI or I2C controllers, and on
>> all Atmel devices serial lines can be reconfigured as GPIOs.
>
> Well this paragraph somehow puzzled me and made me think that you only
> have to keep the serial port as "disabled" in the DT to achieve what you
> had had in mind.
>
>> My use case uses GPIOs to transfer a firmware update using a custom
>> protocol on the lines used as a serial port during the normal life of
>> the device. If it is not possible to unbind the atmel_serial driver, the
>> GPIO lines remain reserved and prevent this case from working.
>
> Yes, here I understand better. Your use case is somewhat uncommon as
> your SoC pads can be configured for two different uses with two
> different drivers in front of your hardware device...
>
>> This patch reinstates the atmel_serial_remove function, and fixes it as
>> it failed to clear the "clk" field on removal, triggering an oops when
>> a device was bound again after being unbound.
>
> Well, okay. As the modification is not that big and that the solution is
> pretty elegant, I'll take it.
>
>
>> Signed-off-by: Romain Izard <romain.izard@gmail.com>
>> ---
>>  drivers/tty/serial/atmel_serial.c | 30 +++---
>>  1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c 
>> b/drivers/tty/serial/atmel_serial.c
>> index 1c0884d8ef32..59e241723edc 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -2759,14 +2759,38 @@ err:
>>   return ret;
>>  }
>>
>> +static int atmel_serial_remove(struct platform_device *pdev)
>> +{
>> + struct uart_port *port = platform_get_drvdata(pdev);
>> + struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>> + int ret = 0;
>> +
>> + tasklet_kill(_port->tasklet);
>> +
>> + device_init_wakeup(>dev, 0);
>> +
>> + ret = uart_remove_one_port(_uart, port);
>> +
>> + kfree(atmel_port->rx_ring.buf);
>> +
>> + /* "port" is allocated statically, so we shouldn't free it */
>> +
>> + clear_bit(port->line, atmel_ports_in_use);
>> +
>> + clk_put(atmel_port->clk);
>> + atmel_port->clk = NULL;
>> +
>> + return ret;
>> +}
>> +
>>  static struct platform_driver atmel_serial_driver = {
>>   .probe  = atmel_serial_probe,
>> + .remove = atmel_serial_remove,
>>   .suspend= atmel_serial_suspend,
>>   .resume = atmel_serial_resume,
>>   .driver = {
>> - .name   = "atmel_usart",
>> - .of_match_table = of_match_ptr(atmel_serial_dt_ids),
>> - .suppress_bind_attrs= true,
>> +     .name   = "atmel_usart",
>> + .of_match_table = of_match_ptr(atmel_serial_dt_ids),
>
> The 2 modifications above are not related to the patch: keep them like
> they were event if it's not as pretty as you would like...
>

These modifications were introduced by the patch I reversed, so the
alignment just returned to what it was before Paul's patch. And I need
to remove "suppress_bind_attrs", otherwise it's not possible to unbind
the device and the driver.

Best regards,
-- 
Romain Izard

Re: [PATCH] tty/serial: at91: restore dynamic driver binding

[romain izard]

2016-02-24 15:53 GMT+01:00 Nicolas Ferre :
> Le 23/02/2016 17:59, Romain Izard a écrit :
>> In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular support
>> code for atmel_serial was removed, as the driver cannot be built as a
>> module. Because no use case was proposed, the dynamic driver binding
>> support was removed as well.
>>
>> The atmel_serial driver can manage up to 7 serial controllers, which are
>> multiplexed with other functions. For example, in the Atmel SAMA5D2, the
>> Flexcom controllers can work as USART, SPI or I2C controllers, and on
>> all Atmel devices serial lines can be reconfigured as GPIOs.
>
> Well this paragraph somehow puzzled me and made me think that you only
> have to keep the serial port as "disabled" in the DT to achieve what you
> had had in mind.
>
>> My use case uses GPIOs to transfer a firmware update using a custom
>> protocol on the lines used as a serial port during the normal life of
>> the device. If it is not possible to unbind the atmel_serial driver, the
>> GPIO lines remain reserved and prevent this case from working.
>
> Yes, here I understand better. Your use case is somewhat uncommon as
> your SoC pads can be configured for two different uses with two
> different drivers in front of your hardware device...
>
>> This patch reinstates the atmel_serial_remove function, and fixes it as
>> it failed to clear the "clk" field on removal, triggering an oops when
>> a device was bound again after being unbound.
>
> Well, okay. As the modification is not that big and that the solution is
> pretty elegant, I'll take it.
>
>
>> Signed-off-by: Romain Izard 
>> ---
>>  drivers/tty/serial/atmel_serial.c | 30 +++---
>>  1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c 
>> b/drivers/tty/serial/atmel_serial.c
>> index 1c0884d8ef32..59e241723edc 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -2759,14 +2759,38 @@ err:
>>   return ret;
>>  }
>>
>> +static int atmel_serial_remove(struct platform_device *pdev)
>> +{
>> + struct uart_port *port = platform_get_drvdata(pdev);
>> + struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>> + int ret = 0;
>> +
>> + tasklet_kill(_port->tasklet);
>> +
>> + device_init_wakeup(>dev, 0);
>> +
>> + ret = uart_remove_one_port(_uart, port);
>> +
>> + kfree(atmel_port->rx_ring.buf);
>> +
>> + /* "port" is allocated statically, so we shouldn't free it */
>> +
>> + clear_bit(port->line, atmel_ports_in_use);
>> +
>> + clk_put(atmel_port->clk);
>> + atmel_port->clk = NULL;
>> +
>> + return ret;
>> +}
>> +
>>  static struct platform_driver atmel_serial_driver = {
>>   .probe  = atmel_serial_probe,
>> + .remove = atmel_serial_remove,
>>   .suspend= atmel_serial_suspend,
>>   .resume = atmel_serial_resume,
>>   .driver = {
>> - .name   = "atmel_usart",
>> - .of_match_table = of_match_ptr(atmel_serial_dt_ids),
>> - .suppress_bind_attrs= true,
>> + .name   = "atmel_usart",
>> + .of_match_table = of_match_ptr(atmel_serial_dt_ids),
>
> The 2 modifications above are not related to the patch: keep them like
> they were event if it's not as pretty as you would like...
>

These modifications were introduced by the patch I reversed, so the
alignment just returned to what it was before Paul's patch. And I need
to remove "suppress_bind_attrs", otherwise it's not possible to unbind
the device and the driver.

Best regards,
-- 
Romain Izard

Re: [PATCH] tty/serial: at91: restore dynamic driver binding

[Romain Izard]

2016-02-23 20:18 GMT+01:00 Paul Gortmaker <paul.gortma...@windriver.com>:
> [[PATCH] tty/serial: at91: restore dynamic driver binding] On 23/02/2016 (Tue 
> 17:59) Romain Izard wrote:
>
>> In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular support
>> code for atmel_serial was removed, as the driver cannot be built as a
>> module. Because no use case was proposed, the dynamic driver binding
>> support was removed as well.
>>
>> The atmel_serial driver can manage up to 7 serial controllers, which are
>> multiplexed with other functions. For example, in the Atmel SAMA5D2, the
>> Flexcom controllers can work as USART, SPI or I2C controllers, and on
>> all Atmel devices serial lines can be reconfigured as GPIOs.
>>
>> My use case uses GPIOs to transfer a firmware update using a custom
>> protocol on the lines used as a serial port during the normal life of
>> the device. If it is not possible to unbind the atmel_serial driver, the
>> GPIO lines remain reserved and prevent this case from working.
>>
>> This patch reinstates the atmel_serial_remove function, and fixes it as
>> it failed to clear the "clk" field on removal, triggering an oops when
>> a device was bound again after being unbound.
>
> I'd suggest that you add a comment above the remove fcn that gives the
> executive summary of the above; i.e. an unbind allows a fw update via
> blah blah and hence the .remove makes sense even though the driver is
> not modular.
>
OK, I'll send a v2.


Best regards,

Re: [PATCH] tty/serial: at91: restore dynamic driver binding

[Romain Izard]

2016-02-23 20:18 GMT+01:00 Paul Gortmaker :
> [[PATCH] tty/serial: at91: restore dynamic driver binding] On 23/02/2016 (Tue 
> 17:59) Romain Izard wrote:
>
>> In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular support
>> code for atmel_serial was removed, as the driver cannot be built as a
>> module. Because no use case was proposed, the dynamic driver binding
>> support was removed as well.
>>
>> The atmel_serial driver can manage up to 7 serial controllers, which are
>> multiplexed with other functions. For example, in the Atmel SAMA5D2, the
>> Flexcom controllers can work as USART, SPI or I2C controllers, and on
>> all Atmel devices serial lines can be reconfigured as GPIOs.
>>
>> My use case uses GPIOs to transfer a firmware update using a custom
>> protocol on the lines used as a serial port during the normal life of
>> the device. If it is not possible to unbind the atmel_serial driver, the
>> GPIO lines remain reserved and prevent this case from working.
>>
>> This patch reinstates the atmel_serial_remove function, and fixes it as
>> it failed to clear the "clk" field on removal, triggering an oops when
>> a device was bound again after being unbound.
>
> I'd suggest that you add a comment above the remove fcn that gives the
> executive summary of the above; i.e. an unbind allows a fw update via
> blah blah and hence the .remove makes sense even though the driver is
> not modular.
>
OK, I'll send a v2.


Best regards,

[PATCH] tty/serial: at91: restore dynamic driver binding

[Romain Izard]

In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular support
code for atmel_serial was removed, as the driver cannot be built as a
module. Because no use case was proposed, the dynamic driver binding
support was removed as well.

The atmel_serial driver can manage up to 7 serial controllers, which are
multiplexed with other functions. For example, in the Atmel SAMA5D2, the
Flexcom controllers can work as USART, SPI or I2C controllers, and on
all Atmel devices serial lines can be reconfigured as GPIOs.

My use case uses GPIOs to transfer a firmware update using a custom
protocol on the lines used as a serial port during the normal life of
the device. If it is not possible to unbind the atmel_serial driver, the
GPIO lines remain reserved and prevent this case from working.

This patch reinstates the atmel_serial_remove function, and fixes it as
it failed to clear the "clk" field on removal, triggering an oops when
a device was bound again after being unbound.

Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
 drivers/tty/serial/atmel_serial.c | 30 +++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c 
b/drivers/tty/serial/atmel_serial.c
index 1c0884d8ef32..59e241723edc 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2759,14 +2759,38 @@ err:
return ret;
 }
 
+static int atmel_serial_remove(struct platform_device *pdev)
+{
+   struct uart_port *port = platform_get_drvdata(pdev);
+   struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+   int ret = 0;
+
+   tasklet_kill(_port->tasklet);
+
+   device_init_wakeup(>dev, 0);
+
+   ret = uart_remove_one_port(_uart, port);
+
+   kfree(atmel_port->rx_ring.buf);
+
+   /* "port" is allocated statically, so we shouldn't free it */
+
+   clear_bit(port->line, atmel_ports_in_use);
+
+   clk_put(atmel_port->clk);
+   atmel_port->clk = NULL;
+
+   return ret;
+}
+
 static struct platform_driver atmel_serial_driver = {
.probe  = atmel_serial_probe,
+   .remove = atmel_serial_remove,
.suspend= atmel_serial_suspend,
.resume = atmel_serial_resume,
.driver = {
-   .name   = "atmel_usart",
-   .of_match_table = of_match_ptr(atmel_serial_dt_ids),
-   .suppress_bind_attrs= true,
+   .name   = "atmel_usart",
+   .of_match_table = of_match_ptr(atmel_serial_dt_ids),
},
 };
 
-- 
2.5.0

[PATCH] tty/serial: at91: restore dynamic driver binding

[Romain Izard]

In commit c39dfebc7798956fd2140ae6321786ff35da30c3, the modular support
code for atmel_serial was removed, as the driver cannot be built as a
module. Because no use case was proposed, the dynamic driver binding
support was removed as well.

The atmel_serial driver can manage up to 7 serial controllers, which are
multiplexed with other functions. For example, in the Atmel SAMA5D2, the
Flexcom controllers can work as USART, SPI or I2C controllers, and on
all Atmel devices serial lines can be reconfigured as GPIOs.

My use case uses GPIOs to transfer a firmware update using a custom
protocol on the lines used as a serial port during the normal life of
the device. If it is not possible to unbind the atmel_serial driver, the
GPIO lines remain reserved and prevent this case from working.

This patch reinstates the atmel_serial_remove function, and fixes it as
it failed to clear the "clk" field on removal, triggering an oops when
a device was bound again after being unbound.

Signed-off-by: Romain Izard 
---
 drivers/tty/serial/atmel_serial.c | 30 +++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c 
b/drivers/tty/serial/atmel_serial.c
index 1c0884d8ef32..59e241723edc 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2759,14 +2759,38 @@ err:
return ret;
 }
 
+static int atmel_serial_remove(struct platform_device *pdev)
+{
+   struct uart_port *port = platform_get_drvdata(pdev);
+   struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+   int ret = 0;
+
+   tasklet_kill(_port->tasklet);
+
+   device_init_wakeup(>dev, 0);
+
+   ret = uart_remove_one_port(_uart, port);
+
+   kfree(atmel_port->rx_ring.buf);
+
+   /* "port" is allocated statically, so we shouldn't free it */
+
+   clear_bit(port->line, atmel_ports_in_use);
+
+   clk_put(atmel_port->clk);
+   atmel_port->clk = NULL;
+
+   return ret;
+}
+
 static struct platform_driver atmel_serial_driver = {
.probe  = atmel_serial_probe,
+   .remove = atmel_serial_remove,
.suspend= atmel_serial_suspend,
.resume = atmel_serial_resume,
.driver = {
-   .name   = "atmel_usart",
-   .of_match_table = of_match_ptr(atmel_serial_dt_ids),
-   .suppress_bind_attrs= true,
+   .name   = "atmel_usart",
+   .of_match_table = of_match_ptr(atmel_serial_dt_ids),
},
 };
 
-- 
2.5.0

Re: [PATCH v1] serial: mctrl_gpio: Add missing module license

[Romain Izard]

2016-02-23 16:21 GMT+01:00 Fabio Estevam <feste...@gmail.com>:
> On Tue, Feb 23, 2016 at 11:54 AM, Romain Izard
> <romain.izard@gmail.com> wrote:
>> As the mctrl_gpio driver can be built as a module, it needs to have its
>> license specified with MODULE_LICENSE. Otherwise, it cannot access
>> required symbols exported through EXPORT_SYMBOL_GPL.
>>
>> Signed-off-by: Romain Izard <romain.izard@gmail.com>
>> ---
>>  drivers/tty/serial/serial_mctrl_gpio.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
>> b/drivers/tty/serial/serial_mctrl_gpio.c
>> index 226ad23b136c..02147361eaa9 100644
>> --- a/drivers/tty/serial/serial_mctrl_gpio.c
>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
>> @@ -20,6 +20,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "serial_mctrl_gpio.h"
>>
>> @@ -249,3 +250,5 @@ void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
>> }
>>  }
>>  EXPORT_SYMBOL_GPL(mctrl_gpio_disable_ms);
>> +
>> +MODULE_LICENSE("GPL");
>
> Shouldn't it be "GPL v2" instead to match the licensing text in the
> top of the file?

According to include/linux/module.h, "GPL" means "GNU Public License
v2 or later",
which is the license text in the file header.

-- 
Romain Izard

Re: [PATCH v1] serial: mctrl_gpio: Add missing module license

[Romain Izard]

2016-02-23 16:21 GMT+01:00 Fabio Estevam :
> On Tue, Feb 23, 2016 at 11:54 AM, Romain Izard
>  wrote:
>> As the mctrl_gpio driver can be built as a module, it needs to have its
>> license specified with MODULE_LICENSE. Otherwise, it cannot access
>> required symbols exported through EXPORT_SYMBOL_GPL.
>>
>> Signed-off-by: Romain Izard 
>> ---
>>  drivers/tty/serial/serial_mctrl_gpio.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
>> b/drivers/tty/serial/serial_mctrl_gpio.c
>> index 226ad23b136c..02147361eaa9 100644
>> --- a/drivers/tty/serial/serial_mctrl_gpio.c
>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
>> @@ -20,6 +20,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "serial_mctrl_gpio.h"
>>
>> @@ -249,3 +250,5 @@ void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
>> }
>>  }
>>  EXPORT_SYMBOL_GPL(mctrl_gpio_disable_ms);
>> +
>> +MODULE_LICENSE("GPL");
>
> Shouldn't it be "GPL v2" instead to match the licensing text in the
> top of the file?

According to include/linux/module.h, "GPL" means "GNU Public License
v2 or later",
which is the license text in the file header.

-- 
Romain Izard

[PATCH v1] serial: mctrl_gpio: Add missing module license

[Romain Izard]

As the mctrl_gpio driver can be built as a module, it needs to have its
license specified with MODULE_LICENSE. Otherwise, it cannot access
required symbols exported through EXPORT_SYMBOL_GPL.

Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
 drivers/tty/serial/serial_mctrl_gpio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 226ad23b136c..02147361eaa9 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "serial_mctrl_gpio.h"
 
@@ -249,3 +250,5 @@ void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
}
 }
 EXPORT_SYMBOL_GPL(mctrl_gpio_disable_ms);
+
+MODULE_LICENSE("GPL");
-- 
2.5.0

[PATCH v1] serial: mctrl_gpio: Add missing module license

[Romain Izard]

As the mctrl_gpio driver can be built as a module, it needs to have its
license specified with MODULE_LICENSE. Otherwise, it cannot access
required symbols exported through EXPORT_SYMBOL_GPL.

Signed-off-by: Romain Izard 
---
 drivers/tty/serial/serial_mctrl_gpio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c 
b/drivers/tty/serial/serial_mctrl_gpio.c
index 226ad23b136c..02147361eaa9 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "serial_mctrl_gpio.h"
 
@@ -249,3 +250,5 @@ void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
}
 }
 EXPORT_SYMBOL_GPL(mctrl_gpio_disable_ms);
+
+MODULE_LICENSE("GPL");
-- 
2.5.0

Re: Device probing proceeds even when the default pinctrl state is invalid

[romain izard]

2016-02-18 21:07 GMT+01:00 Linus Walleij <linus.wall...@linaro.org>:
> On Thu, Feb 18, 2016 at 11:37 AM, Romain Izard
> <romain.izard@gmail.com> wrote:
>
>> The current code for device probing tries to map the default pinctrl
>> state (in pinctrl_bind_pins), but is returning 0 or -EDEFER. If there
>> is an other error, it is not reported. This means that devices that
>> do not have any specified pinctrl state can be probed, which is a
>> correct behaviour that should be conserved, but it can also be an
>> issue, as it will fail to report any other issue with the specified
>> pinctrl state.
>>
>> Did I miss something that would explain why all other errors are
>> ignored ?
>
> Yeah we should probably respect a few errors and let some pass. Please
> make a patch!
>

I have a hard time choosing which errors should be ignored. For my
tests, I simply removed the error filtering at the end of
pinctrl_bind_pins, which works because devices with no pinctrl info are
already handled in the body of the function. It worked with my board,
but I am not sure that it covers all use cases. There are no hogs in
there, for example.

8<--

diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c
index 076297592754..2d876103fa29 100644
--- a/drivers/base/pinctrl.c
+++ b/drivers/base/pinctrl.c
@@ -91,9 +91,5 @@ cleanup_alloc:
devm_kfree(dev, dev->pins);
dev->pins = NULL;

-   /* Only return deferrals */
-   if (ret != -EPROBE_DEFER)
-   ret = 0;
-
return ret;
 }

8<--

>> This also leads to a larger problem, as currently the device tree for
>> existing boards may specify invalid pinctrl configurations, but the
>> boards look like they work correctly, as long as nothing else tries
>> to use the same pins.
>
> Well I guess if you look in the debugfs files it looks crazy does it
> not? That is how I verify that the pins get bound right.  Especially
> in the file pinmux-pins
>
>> Correcting the issue may require a new 'strict-mapping' property in
>> the pinctrl node in the device tree, otherwise this correction would
>> be an ABI regression.
>
> Bah if the device tree is wrong it is wrong, we certainly do not
> expect erroneous device trees that just "happen to work" to keep
> working.
>
>> Is this pattern really a good one ? We're moving away from describing
>> hardware in here.
>
> I don't understand.
>
I wanted to have your opinion about the using a "strict-mapping"
property, as this property does not describe the hardware, but describes
the status of the device tree iself. From the rest of your mail, I
understand that your point of view is that it's not really needed.

>> For an existing example, in the device tree for Atmel's
>> SAMA5D2_Xplained board,
>
> Where is this device tree, so I can look at it?
>
>> the mapping for the Ethernet transceiver's IRQ line was missing it
>> bias configuration, and thus the pins were not reserved for the
>> Ethernet use. I've just send a patch to correct it, but breaking
>> Ethernet on kernel upgrade for the boards using the previous
>> revisions would be an issue.
>
> Hm, ask the Atmel DT maintainers what to do about this...  Nicolas:
> how real is this ABI issue?

I've sent the patch to Nicolas and the ARM mailing list, with cc to you.
See http://permalink.gmane.org/gmane.linux.kernel/2155589

In this precise case, the mapping for the ethernel interrupt is invalid
and skipped. It does not lead to a problem, because the line is mapped
later during the probe sequence to install the interrupt handler.

When I integrate mapping validation, the probing stops as soon as the
invalid mapping is detected, and the ethernet controller remains
unbound, without any loaded driver. As a result, the network interface
has disappeared. For someone who uses NFS for its rootfs, or anyone
using relying on the network to use the board, it is broken.


What I fear is that among the 300+ existing dts files that have a
"pinctrl-0" property, a fair proportion has an "invalid but working"
device tree, and the change of behaviour of the pinctrl driver will
break them. Hence my proposition to mark in the device trees those that
are known to work correctly.


Best regards,
-- 
Romain Izard

Re: Device probing proceeds even when the default pinctrl state is invalid

[romain izard]

2016-02-18 21:07 GMT+01:00 Linus Walleij :
> On Thu, Feb 18, 2016 at 11:37 AM, Romain Izard
>  wrote:
>
>> The current code for device probing tries to map the default pinctrl
>> state (in pinctrl_bind_pins), but is returning 0 or -EDEFER. If there
>> is an other error, it is not reported. This means that devices that
>> do not have any specified pinctrl state can be probed, which is a
>> correct behaviour that should be conserved, but it can also be an
>> issue, as it will fail to report any other issue with the specified
>> pinctrl state.
>>
>> Did I miss something that would explain why all other errors are
>> ignored ?
>
> Yeah we should probably respect a few errors and let some pass. Please
> make a patch!
>

I have a hard time choosing which errors should be ignored. For my
tests, I simply removed the error filtering at the end of
pinctrl_bind_pins, which works because devices with no pinctrl info are
already handled in the body of the function. It worked with my board,
but I am not sure that it covers all use cases. There are no hogs in
there, for example.

8<--

diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c
index 076297592754..2d876103fa29 100644
--- a/drivers/base/pinctrl.c
+++ b/drivers/base/pinctrl.c
@@ -91,9 +91,5 @@ cleanup_alloc:
devm_kfree(dev, dev->pins);
dev->pins = NULL;

-   /* Only return deferrals */
-   if (ret != -EPROBE_DEFER)
-   ret = 0;
-
return ret;
 }

8<--

>> This also leads to a larger problem, as currently the device tree for
>> existing boards may specify invalid pinctrl configurations, but the
>> boards look like they work correctly, as long as nothing else tries
>> to use the same pins.
>
> Well I guess if you look in the debugfs files it looks crazy does it
> not? That is how I verify that the pins get bound right.  Especially
> in the file pinmux-pins
>
>> Correcting the issue may require a new 'strict-mapping' property in
>> the pinctrl node in the device tree, otherwise this correction would
>> be an ABI regression.
>
> Bah if the device tree is wrong it is wrong, we certainly do not
> expect erroneous device trees that just "happen to work" to keep
> working.
>
>> Is this pattern really a good one ? We're moving away from describing
>> hardware in here.
>
> I don't understand.
>
I wanted to have your opinion about the using a "strict-mapping"
property, as this property does not describe the hardware, but describes
the status of the device tree iself. From the rest of your mail, I
understand that your point of view is that it's not really needed.

>> For an existing example, in the device tree for Atmel's
>> SAMA5D2_Xplained board,
>
> Where is this device tree, so I can look at it?
>
>> the mapping for the Ethernet transceiver's IRQ line was missing it
>> bias configuration, and thus the pins were not reserved for the
>> Ethernet use. I've just send a patch to correct it, but breaking
>> Ethernet on kernel upgrade for the boards using the previous
>> revisions would be an issue.
>
> Hm, ask the Atmel DT maintainers what to do about this...  Nicolas:
> how real is this ABI issue?

I've sent the patch to Nicolas and the ARM mailing list, with cc to you.
See http://permalink.gmane.org/gmane.linux.kernel/2155589

In this precise case, the mapping for the ethernel interrupt is invalid
and skipped. It does not lead to a problem, because the line is mapped
later during the probe sequence to install the interrupt handler.

When I integrate mapping validation, the probing stops as soon as the
invalid mapping is detected, and the ethernet controller remains
unbound, without any loaded driver. As a result, the network interface
has disappeared. For someone who uses NFS for its rootfs, or anyone
using relying on the network to use the board, it is broken.


What I fear is that among the 300+ existing dts files that have a
"pinctrl-0" property, a fair proportion has an "invalid but working"
device tree, and the change of behaviour of the pinctrl driver will
break them. Hence my proposition to mark in the device trees those that
are known to work correctly.


Best regards,
-- 
Romain Izard

Device probing proceeds even when the default pinctrl state is invalid

[Romain Izard]

Hello Linus,

The current code for device probing tries to map the default pinctrl
state (in pinctrl_bind_pins), but is returning 0 or -EDEFER. If there
is an other error, it is not reported. This means that devices that do
not have any specified pinctrl state can be probed, which is a correct
behaviour that should be conserved, but it can also be an issue, as it
will fail to report any other issue with the specified pinctrl state.

Did I miss something that would explain why all other errors are ignored ?

This also leads to a larger problem, as currently the device tree for
existing boards may specify invalid pinctrl configurations, but the
boards look like they work correctly, as long as nothing else tries to
use the same pins. Correcting the issue may require a new
'strict-mapping' property in the pinctrl node in the device tree,
otherwise this correction would be an ABI regression.

Is this pattern really a good one ? We're moving away from describing
hardware in here.

For an existing example, in the device tree for Atmel's
SAMA5D2_Xplained board, the mapping for the Ethernet transceiver's IRQ
line was missing it bias configuration, and thus the pins were not
reserved for the Ethernet use. I've just send a patch to correct it,
but breaking Ethernet on kernel upgrade for the boards using the
previous revisions would be an issue.

I encountered this problem because I wanted to model in device tree a
system where the main SoC running Linux is connected to a secondary
chip, using two different protocols on the same pins. Using the
SAMA5D2's pin muxing, the secondary chip can be accessed either by a
serial port in normal use, or by bitbanging on the multiplexed GPIOs
when programming it. I created two devices with conflicting pinctrl
configurations, expecting only one of them to be successfully probed,
and to use the "bind" and "unbind" sysfs files to select the correct
driver. Choosing which device to probe first on startup is an other
issue in this case, that remains to be addressed.

In the current state of things, both devices are probed successfully
as conflicting pin sets are not recognized as an issue, which means
that my use case does not work.

Is the direction I'm taking something correct ?

Best regards,
-- 
Romain Izard

Device probing proceeds even when the default pinctrl state is invalid

[Romain Izard]

Hello Linus,

The current code for device probing tries to map the default pinctrl
state (in pinctrl_bind_pins), but is returning 0 or -EDEFER. If there
is an other error, it is not reported. This means that devices that do
not have any specified pinctrl state can be probed, which is a correct
behaviour that should be conserved, but it can also be an issue, as it
will fail to report any other issue with the specified pinctrl state.

Did I miss something that would explain why all other errors are ignored ?

This also leads to a larger problem, as currently the device tree for
existing boards may specify invalid pinctrl configurations, but the
boards look like they work correctly, as long as nothing else tries to
use the same pins. Correcting the issue may require a new
'strict-mapping' property in the pinctrl node in the device tree,
otherwise this correction would be an ABI regression.

Is this pattern really a good one ? We're moving away from describing
hardware in here.

For an existing example, in the device tree for Atmel's
SAMA5D2_Xplained board, the mapping for the Ethernet transceiver's IRQ
line was missing it bias configuration, and thus the pins were not
reserved for the Ethernet use. I've just send a patch to correct it,
but breaking Ethernet on kernel upgrade for the boards using the
previous revisions would be an issue.

I encountered this problem because I wanted to model in device tree a
system where the main SoC running Linux is connected to a secondary
chip, using two different protocols on the same pins. Using the
SAMA5D2's pin muxing, the secondary chip can be accessed either by a
serial port in normal use, or by bitbanging on the multiplexed GPIOs
when programming it. I created two devices with conflicting pinctrl
configurations, expecting only one of them to be successfully probed,
and to use the "bind" and "unbind" sysfs files to select the correct
driver. Choosing which device to probe first on startup is an other
issue in this case, that remains to be addressed.

In the current state of things, both devices are probed successfully
as conflicting pin sets are not recognized as an issue, which means
that my use case does not work.

Is the direction I'm taking something correct ?

Best regards,
-- 
Romain Izard

[PATCH] ARM: dts: at91: sama5d2 Xplained: Correct the macb irq pinctrl node

[Romain Izard]

All pinctrl nodes for the Atmel pinctrl controller need to have their
bias configuration explicitly defined. Otherwise, the pinctrl mapping
is not valid.

It works for now as the pinctrl driver proceeds even with invalid
mappings, but this can become an issue, if the pinctrl driver starts
to require valid mappings. Additionally, the pin is not protected from
being remapped later by an other driver.

There is an external 1kΩ pull-up to 3.3V, so no bias is required on
the Ethernet PHY's interrupt line.

Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
 arch/arm/boot/dts/at91-sama5d2_xplained.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts 
b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
index e683856c507c..75341eec2dfd 100644
--- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
@@ -308,6 +308,7 @@
 
pinctrl_macb0_phy_irq: macb0_phy_irq {
pinmux = ;
+   bias-disable;
};
 
pinctrl_pdmic_default: pdmic_default {
-- 
2.5.0

[PATCH] ARM: dts: at91: sama5d2 Xplained: Correct the macb irq pinctrl node

[Romain Izard]

All pinctrl nodes for the Atmel pinctrl controller need to have their
bias configuration explicitly defined. Otherwise, the pinctrl mapping
is not valid.

It works for now as the pinctrl driver proceeds even with invalid
mappings, but this can become an issue, if the pinctrl driver starts
to require valid mappings. Additionally, the pin is not protected from
being remapped later by an other driver.

There is an external 1kΩ pull-up to 3.3V, so no bias is required on
the Ethernet PHY's interrupt line.

Signed-off-by: Romain Izard 
---
 arch/arm/boot/dts/at91-sama5d2_xplained.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts 
b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
index e683856c507c..75341eec2dfd 100644
--- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
@@ -308,6 +308,7 @@
 
pinctrl_macb0_phy_irq: macb0_phy_irq {
pinmux = ;
+   bias-disable;
};
 
pinctrl_pdmic_default: pdmic_default {
-- 
2.5.0

[PATCH v1] clocksource: atmel-pit: register as a sched_clock

[Romain Izard]

Register the counter of the Periodic Interval Timer as a possible source
for sched_clock. This provides a better precision than the jiffies-based
default.

Signed-off-by: Romain Izard 
---

To reduce overhead and cache consumption, sched_clock_register does not
take a void* argument, as it is the case when using interrupt handlers.
As a result, the signature of the function used to read the counter is
u64 (*) (void). It is thus necessary to use a static variable inside the
driver to keep a reference to the clocksource. It is a common pattern in
other existing clocksource drivers.

All existing Atmel SoCs that rely on the PIT only have a single instance
of the controller, and future SoCs should remain this way as additional
timers are provided in 'Timer Counter' controllers. Additional code
handling the case of multiple PIT instances would thus be dead code.

 drivers/clocksource/timer-atmel-pit.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/clocksource/timer-atmel-pit.c 
b/drivers/clocksource/timer-atmel-pit.c
index d911c5dca8f1..744e1afc4b69 100644
--- a/drivers/clocksource/timer-atmel-pit.c
+++ b/drivers/clocksource/timer-atmel-pit.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define AT91_PIT_MR0x00/* Mode Register */
 #define AT91_PIT_PITIENBIT(25) /* 
Timer Interrupt Enable */
@@ -174,6 +175,13 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void 
*dev_id)
return IRQ_NONE;
 }
 
+static struct clocksource *pit_sched_clock;
+
+static u64 pit_sched_clock_read(void)
+{
+   return read_pit_clk(pit_sched_clock);
+}
+
 /*
  * Set up both clocksource and clockevent support.
  */
@@ -206,6 +214,9 @@ static void __init at91sam926x_pit_common_init(struct 
pit_data *data)
data->clksrc.flags = CLOCK_SOURCE_IS_CONTINUOUS;
clocksource_register_hz(>clksrc, pit_rate);
 
+   pit_sched_clock = >clksrc;
+   sched_clock_register(pit_sched_clock_read, bits, pit_rate);
+
/* Set up irq handler */
ret = request_irq(data->irq, at91sam926x_pit_interrupt,
  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
-- 
2.5.0

[PATCH v1] clocksource: atmel-pit: register as a sched_clock

[Romain Izard]

Register the counter of the Periodic Interval Timer as a possible source
for sched_clock. This provides a better precision than the jiffies-based
default.

Signed-off-by: Romain Izard <romain.izard@gmail.com>
---

To reduce overhead and cache consumption, sched_clock_register does not
take a void* argument, as it is the case when using interrupt handlers.
As a result, the signature of the function used to read the counter is
u64 (*) (void). It is thus necessary to use a static variable inside the
driver to keep a reference to the clocksource. It is a common pattern in
other existing clocksource drivers.

All existing Atmel SoCs that rely on the PIT only have a single instance
of the controller, and future SoCs should remain this way as additional
timers are provided in 'Timer Counter' controllers. Additional code
handling the case of multiple PIT instances would thus be dead code.

 drivers/clocksource/timer-atmel-pit.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/clocksource/timer-atmel-pit.c 
b/drivers/clocksource/timer-atmel-pit.c
index d911c5dca8f1..744e1afc4b69 100644
--- a/drivers/clocksource/timer-atmel-pit.c
+++ b/drivers/clocksource/timer-atmel-pit.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define AT91_PIT_MR0x00/* Mode Register */
 #define AT91_PIT_PITIENBIT(25) /* 
Timer Interrupt Enable */
@@ -174,6 +175,13 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void 
*dev_id)
return IRQ_NONE;
 }
 
+static struct clocksource *pit_sched_clock;
+
+static u64 pit_sched_clock_read(void)
+{
+   return read_pit_clk(pit_sched_clock);
+}
+
 /*
  * Set up both clocksource and clockevent support.
  */
@@ -206,6 +214,9 @@ static void __init at91sam926x_pit_common_init(struct 
pit_data *data)
data->clksrc.flags = CLOCK_SOURCE_IS_CONTINUOUS;
clocksource_register_hz(>clksrc, pit_rate);
 
+   pit_sched_clock = >clksrc;
+   sched_clock_register(pit_sched_clock_read, bits, pit_rate);
+
/* Set up irq handler */
ret = request_irq(data->irq, at91sam926x_pit_interrupt,
  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
-- 
2.5.0

Re: [PATCH v2] trace: module: Maintain a valid user count

[Romain Izard]

2014-03-06 18:34 GMT+01:00 Steven Rostedt :
> Ingo,
>
> You want to push this to Linus?
>
> Reviewed-by: Steven Rostedt 
>
> -- Steve
>
>
> On Tue,  4 Mar 2014 10:09:39 +0100
> Romain Izard  wrote:
>
>> The replacement of the 'count' variable by two variables 'incs' and
>> 'decs' to resolve some race conditions during module unloading was done
>> in parallel with some cleanup in the trace subsystem, and was integrated
>> as a merge.
>>
>> Unfortunately, the formula for this replacement was wrong in the tracing
>> code, and the refcount in the traces was not usable as a result.
>>
>> Use 'count = incs - decs' to compute the user count.
>>
>> Signed-off-by: Romain Izard 
>> ---
>>  include/trace/events/module.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/trace/events/module.h b/include/trace/events/module.h
>> index 161932737416..ca298c7157ae 100644
>> --- a/include/trace/events/module.h
>> +++ b/include/trace/events/module.h
>> @@ -78,7 +78,7 @@ DECLARE_EVENT_CLASS(module_refcnt,
>>
>>   TP_fast_assign(
>>   __entry->ip = ip;
>> - __entry->refcnt = __this_cpu_read(mod->refptr->incs) + 
>> __this_cpu_read(mod->refptr->decs);
>> + __entry->refcnt = __this_cpu_read(mod->refptr->incs) - 
>> __this_cpu_read(mod->refptr->decs);
>>   __assign_str(name, mod->name);
>>   ),
>>
>

Ping ?
--
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 v2] trace: module: Maintain a valid user count

[Romain Izard]

2014-03-06 18:34 GMT+01:00 Steven Rostedt rost...@goodmis.org:
 Ingo,

 You want to push this to Linus?

 Reviewed-by: Steven Rostedt rost...@goodmis.org

 -- Steve


 On Tue,  4 Mar 2014 10:09:39 +0100
 Romain Izard romain.izard@gmail.com wrote:

 The replacement of the 'count' variable by two variables 'incs' and
 'decs' to resolve some race conditions during module unloading was done
 in parallel with some cleanup in the trace subsystem, and was integrated
 as a merge.

 Unfortunately, the formula for this replacement was wrong in the tracing
 code, and the refcount in the traces was not usable as a result.

 Use 'count = incs - decs' to compute the user count.

 Signed-off-by: Romain Izard romain.izard@gmail.com
 ---
  include/trace/events/module.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/include/trace/events/module.h b/include/trace/events/module.h
 index 161932737416..ca298c7157ae 100644
 --- a/include/trace/events/module.h
 +++ b/include/trace/events/module.h
 @@ -78,7 +78,7 @@ DECLARE_EVENT_CLASS(module_refcnt,

   TP_fast_assign(
   __entry-ip = ip;
 - __entry-refcnt = __this_cpu_read(mod-refptr-incs) + 
 __this_cpu_read(mod-refptr-decs);
 + __entry-refcnt = __this_cpu_read(mod-refptr-incs) - 
 __this_cpu_read(mod-refptr-decs);
   __assign_str(name, mod-name);
   ),



Ping ?
--
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 v2] trace: module: Maintain a valid user count

[Romain Izard]

The replacement of the 'count' variable by two variables 'incs' and
'decs' to resolve some race conditions during module unloading was done
in parallel with some cleanup in the trace subsystem, and was integrated
as a merge.

Unfortunately, the formula for this replacement was wrong in the tracing
code, and the refcount in the traces was not usable as a result.

Use 'count = incs - decs' to compute the user count.

Signed-off-by: Romain Izard 
---
 include/trace/events/module.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/trace/events/module.h b/include/trace/events/module.h
index 161932737416..ca298c7157ae 100644
--- a/include/trace/events/module.h
+++ b/include/trace/events/module.h
@@ -78,7 +78,7 @@ DECLARE_EVENT_CLASS(module_refcnt,
 
TP_fast_assign(
__entry->ip = ip;
-   __entry->refcnt = __this_cpu_read(mod->refptr->incs) + 
__this_cpu_read(mod->refptr->decs);
+   __entry->refcnt = __this_cpu_read(mod->refptr->incs) - 
__this_cpu_read(mod->refptr->decs);
__assign_str(name, mod->name);
),
 
-- 
1.8.3.2

--
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 v2] trace: module: Maintain a valid user count

[Romain Izard]

The replacement of the 'count' variable by two variables 'incs' and
'decs' to resolve some race conditions during module unloading was done
in parallel with some cleanup in the trace subsystem, and was integrated
as a merge.

Unfortunately, the formula for this replacement was wrong in the tracing
code, and the refcount in the traces was not usable as a result.

Use 'count = incs - decs' to compute the user count.

Signed-off-by: Romain Izard romain.izard@gmail.com
---
 include/trace/events/module.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/trace/events/module.h b/include/trace/events/module.h
index 161932737416..ca298c7157ae 100644
--- a/include/trace/events/module.h
+++ b/include/trace/events/module.h
@@ -78,7 +78,7 @@ DECLARE_EVENT_CLASS(module_refcnt,
 
TP_fast_assign(
__entry-ip = ip;
-   __entry-refcnt = __this_cpu_read(mod-refptr-incs) + 
__this_cpu_read(mod-refptr-decs);
+   __entry-refcnt = __this_cpu_read(mod-refptr-incs) - 
__this_cpu_read(mod-refptr-decs);
__assign_str(name, mod-name);
),
 
-- 
1.8.3.2

--
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] trace: module: Maintain a valid user count

[Romain Izard]

The replacement of the 'count' variable by two variables 'incs' and
'decs' to resolve some race conditions during module unloading was done
in parallel with some cleanup in the trace subsystem, and was integrated
as a merge.

Unfortunately, the formula for this replacement was wrong in the tracing
code, and the refcount in the traces was not usable as a result.

Use 'count = incs - decs' to compute the user count.

Signed-off-by: Romain Izard 
---
 include/trace/events/module.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/module.h b/include/trace/events/module.h
index 161932737416..1228d963b6d1 100644
--- a/include/trace/events/module.h
+++ b/include/trace/events/module.h
@@ -78,7 +78,8 @@ DECLARE_EVENT_CLASS(module_refcnt,
 
TP_fast_assign(
__entry->ip = ip;
-   __entry->refcnt = __this_cpu_read(mod->refptr->incs) + 
__this_cpu_read(mod->refptr->decs);
+   __entry->refcnt = __this_cpu_read(mod->refptr->incs) -
+   __this_cpu_read(mod->refptr->decs);
__assign_str(name, mod->name);
),
 
-- 
1.8.3.2

--
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] trace: module: Maintain a valid user count

[Romain Izard]

The replacement of the 'count' variable by two variables 'incs' and
'decs' to resolve some race conditions during module unloading was done
in parallel with some cleanup in the trace subsystem, and was integrated
as a merge.

Unfortunately, the formula for this replacement was wrong in the tracing
code, and the refcount in the traces was not usable as a result.

Use 'count = incs - decs' to compute the user count.

Signed-off-by: Romain Izard romain.izard@gmail.com
---
 include/trace/events/module.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/module.h b/include/trace/events/module.h
index 161932737416..1228d963b6d1 100644
--- a/include/trace/events/module.h
+++ b/include/trace/events/module.h
@@ -78,7 +78,8 @@ DECLARE_EVENT_CLASS(module_refcnt,
 
TP_fast_assign(
__entry-ip = ip;
-   __entry-refcnt = __this_cpu_read(mod-refptr-incs) + 
__this_cpu_read(mod-refptr-decs);
+   __entry-refcnt = __this_cpu_read(mod-refptr-incs) -
+   __this_cpu_read(mod-refptr-decs);
__assign_str(name, mod-name);
),
 
-- 
1.8.3.2

--
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 2/4] drivers/mmc/host: don't use devm_pinctrl_get_select_default() in probe

[Romain Izard]

On 2013-10-13, Wolfram Sang  wrote:
> Since commit ab78029 (drivers/pinctrl: grab default handles from device core),
> we can rely on device core for setting the default pins. Compile tested only.
>
> Acked-by: Linus Walleij  (personally at LCE13)
> Signed-off-by: Wolfram Sang 
> ---
>  drivers/mmc/host/mvsdio.c  | 7 +--
>  drivers/mmc/host/omap_hsmmc.c  | 7 ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 8 
>  3 files changed, 1 insertion(+), 21 deletions(-)
>
> diff --git a/drivers/mmc/host/mvsdio.c b/drivers/mmc/host/mvsdio.c
> index 06c5b0b..8c9f448 100644
> --- a/drivers/mmc/host/mvsdio.c
> +++ b/drivers/mmc/host/mvsdio.c
> @@ -25,7 +25,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  
>  #include 
>  #include 
> @@ -685,7 +684,7 @@ static int __init mvsd_probe(struct platform_device *pdev)
>   const struct mbus_dram_target_info *dram;
>   struct resource *r;
>   int ret, irq;
> - struct pinctrl *pinctrl;
> + int gpio_card_detect, gpio_write_protect;
>  

This new line does not belong to this patch.

>   r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   irq = platform_get_irq(pdev, 0);
> @@ -702,10 +701,6 @@ static int __init mvsd_probe(struct platform_device 
> *pdev)
>   host->mmc = mmc;
>   host->dev = >dev;
>  
> - pinctrl = devm_pinctrl_get_select_default(>dev);
> - if (IS_ERR(pinctrl))
> - dev_warn(>dev, "no pins associated\n");
> -
>   /*
>    * Some non-DT platforms do not pass a clock, and the clock
>* frequency is passed through platform_data. On DT platforms,

Best regards,
-- 
Romain Izard

--
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 2/4] drivers/mmc/host: don't use devm_pinctrl_get_select_default() in probe

[Romain Izard]

On 2013-10-13, Wolfram Sang w...@the-dreams.de wrote:
 Since commit ab78029 (drivers/pinctrl: grab default handles from device core),
 we can rely on device core for setting the default pins. Compile tested only.

 Acked-by: Linus Walleij linus.wall...@linaro.org (personally at LCE13)
 Signed-off-by: Wolfram Sang w...@the-dreams.de
 ---
  drivers/mmc/host/mvsdio.c  | 7 +--
  drivers/mmc/host/omap_hsmmc.c  | 7 ---
  drivers/mmc/host/sdhci-esdhc-imx.c | 8 
  3 files changed, 1 insertion(+), 21 deletions(-)

 diff --git a/drivers/mmc/host/mvsdio.c b/drivers/mmc/host/mvsdio.c
 index 06c5b0b..8c9f448 100644
 --- a/drivers/mmc/host/mvsdio.c
 +++ b/drivers/mmc/host/mvsdio.c
 @@ -25,7 +25,6 @@
  #include linux/of_irq.h
  #include linux/mmc/host.h
  #include linux/mmc/slot-gpio.h
 -#include linux/pinctrl/consumer.h
  
  #include asm/sizes.h
  #include asm/unaligned.h
 @@ -685,7 +684,7 @@ static int __init mvsd_probe(struct platform_device *pdev)
   const struct mbus_dram_target_info *dram;
   struct resource *r;
   int ret, irq;
 - struct pinctrl *pinctrl;
 + int gpio_card_detect, gpio_write_protect;
  

This new line does not belong to this patch.

   r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   irq = platform_get_irq(pdev, 0);
 @@ -702,10 +701,6 @@ static int __init mvsd_probe(struct platform_device 
 *pdev)
   host-mmc = mmc;
   host-dev = pdev-dev;
  
 - pinctrl = devm_pinctrl_get_select_default(pdev-dev);
 - if (IS_ERR(pinctrl))
 - dev_warn(pdev-dev, no pins associated\n);
 -
   /*
* Some non-DT platforms do not pass a clock, and the clock
* frequency is passed through platform_data. On DT platforms,

Best regards,
-- 
Romain Izard

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