Re: [PATCH] net: broadcom: fix a mistake about ioremap resource
On Tue, May 5, 2020 at 12:20 AM Markus Elfring wrote:
>
> > Commit d7a5502b0bb8b ("net: broadcom: convert to
> > devm_platform_ioremap_resource_byname()") will broke this driver.
> > idm_base and nicpm_base were optional, after this change, they are
> > mandatory. it will probe fails with -22 when the dtb doesn't have them
> > defined. so revert part of this commit and make idm_base and nicpm_base
> > as optional.
>
> I hope that other contributors can convince you to improve also this
> commit message considerably.
> Would you like to fix the spelling besides other wording weaknesses?
How about this wording:
Commit d7a5502b0bb8b ("net: broadcom: convert to
devm_platform_ioremap_resource_byname()")
inadvertently made idm_base and nicpm_base mandatory. These are
optional properties.
probe will fail when they're not defined. The commit is partially
reverted so that they are
obtained by platform_get_resource_byname() as before. amac_base can
still be obtained
by devm_platform_ioremap_resource_byname().
Re: [PATCH v1 1/3] dt-bindings: Document Broadcom OTP controller driver
On 16-10-30 06:38 PM, Rob Herring wrote:
> On Mon, Oct 24, 2016 at 12:12:02PM -0700, Jonathan Richardson wrote:
>> From: Jonathan Richardson <jonat...@broadcom.com>
>>
>> Reviewed-by: Ray Jui <ray....@broadcom.com>
>> Tested-by: Jonathan Richardson <jonathan.richard...@broadcom.com>
>> Signed-off-by: Scott Branden <scott.bran...@broadcom.com>
>> Signed-off-by: Oza Pawandeep <o...@broadcom.com>
>> Signed-off-by: Jonathan Richardson <jonathan.richard...@broadcom.com>
>> ---
>> Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt | 17
>> +
>> 1 file changed, 17 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
>> b/Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
>> new file mode 100644
>> index 000..6462e12
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
>> @@ -0,0 +1,17 @@
>> +Broadcom OTP memory controller
>> +
>> +Required Properties:
>> +- compatible: "brcm,ocotp" for the first generation Broadcom OTPC which is
>> used
>> + in Cygnus and supports 32 bit read/write. Use "brcm,ocotp-v2" for the
>> second
>> + generation Broadcom OTPC which is used in SoC's such as Stingray and
>> supports
>> + 64-bit read/write.
>
> These should be SoC specific. While I'd guess this block is simple
> enough, having the SoC can define what all the bits are. Yes, there is a
> binding to define those, but you may not use that.
Hi Rob. This block isn't SoC specific. It is used on multiple SoC's.
There are older SoC's using v1 that we may upstream drivers for in the
future. v1 isn't specific to cygnus/iproc and v2 isn't specific to stingray.
>
>
>> +- reg: Base address of the OTP controller.
>> +- brcm,ocotp-size: Amount of memory available, in 32 bit words
>> +
>> +Example:
>> +
>> +otp: otp@0301c800 {
>> +compatible = "brcm,ocotp";
>> +reg = <0x0301c800 0x2c>;
>> +brcm,ocotp-size = <2048>;
>> +};
>> --
>> 1.9.1
>>
Re: [PATCH v1 1/3] dt-bindings: Document Broadcom OTP controller driver
On 16-10-30 06:38 PM, Rob Herring wrote:
> On Mon, Oct 24, 2016 at 12:12:02PM -0700, Jonathan Richardson wrote:
>> From: Jonathan Richardson
>>
>> Reviewed-by: Ray Jui
>> Tested-by: Jonathan Richardson
>> Signed-off-by: Scott Branden
>> Signed-off-by: Oza Pawandeep
>> Signed-off-by: Jonathan Richardson
>> ---
>> Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt | 17
>> +
>> 1 file changed, 17 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
>> b/Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
>> new file mode 100644
>> index 000..6462e12
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
>> @@ -0,0 +1,17 @@
>> +Broadcom OTP memory controller
>> +
>> +Required Properties:
>> +- compatible: "brcm,ocotp" for the first generation Broadcom OTPC which is
>> used
>> + in Cygnus and supports 32 bit read/write. Use "brcm,ocotp-v2" for the
>> second
>> + generation Broadcom OTPC which is used in SoC's such as Stingray and
>> supports
>> + 64-bit read/write.
>
> These should be SoC specific. While I'd guess this block is simple
> enough, having the SoC can define what all the bits are. Yes, there is a
> binding to define those, but you may not use that.
Hi Rob. This block isn't SoC specific. It is used on multiple SoC's.
There are older SoC's using v1 that we may upstream drivers for in the
future. v1 isn't specific to cygnus/iproc and v2 isn't specific to stingray.
>
>
>> +- reg: Base address of the OTP controller.
>> +- brcm,ocotp-size: Amount of memory available, in 32 bit words
>> +
>> +Example:
>> +
>> +otp: otp@0301c800 {
>> +compatible = "brcm,ocotp";
>> +reg = <0x0301c800 0x2c>;
>> +brcm,ocotp-size = <2048>;
>> +};
>> --
>> 1.9.1
>>
[PATCH v1 3/3] ARM: dts: Add node for Broadcom OTP controller driver
From: Jonathan Richardson <jonat...@broadcom.com>
Reviewed-by: Ray Jui <ray@broadcom.com>
Tested-by: Jonathan Richardson <jonathan.richard...@broadcom.com>
Signed-off-by: Scott Branden <scott.bran...@broadcom.com>
Signed-off-by: Oza Pawandeep <o...@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathan.richard...@broadcom.com>
---
arch/arm/boot/dts/bcm-cygnus.dtsi | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
b/arch/arm/boot/dts/bcm-cygnus.dtsi
index fabc9f3..a74a430 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -91,6 +91,13 @@
#address-cells = <1>;
#size-cells = <1>;
+ otp: otp@0301c800 {
+ compatible = "brcm,ocotp";
+ reg = <0x0301c800 0x2c>;
+ brcm,ocotp-size = <2048>;
+ status = "disabled";
+ };
+
pcie_phy: phy@0301d0a0 {
compatible = "brcm,cygnus-pcie-phy";
reg = <0x0301d0a0 0x14>;
--
1.9.1
[PATCH v1 2/3] nvmem: Add the Broadcom OTP controller driver
From: Jonathan Richardson <jonat...@broadcom.com>
Add support for 32 and 64-bit versions of Broadcom's On-Chip OTP
controller. These controllers are used on SoC's such as Cygnus and
Stingray.
Reviewed-by: Ray Jui <ray@broadcom.com>
Tested-by: Jonathan Richardson <jonathan.richard...@broadcom.com>
Signed-off-by: Scott Branden <scott.bran...@broadcom.com>
Signed-off-by: Oza Pawandeep <o...@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathan.richard...@broadcom.com>
---
drivers/nvmem/Kconfig | 12 ++
drivers/nvmem/Makefile| 2 +
drivers/nvmem/bcm-ocotp.c | 335 ++
3 files changed, 349 insertions(+)
create mode 100644 drivers/nvmem/bcm-ocotp.c
diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index ba140ea..06935a7 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -80,6 +80,18 @@ config ROCKCHIP_EFUSE
This driver can also be built as a module. If so, the module
will be called nvmem_rockchip_efuse.
+config NVMEM_BCM_OCOTP
+ tristate "Broadcom On-Chip OTP Controller support"
+ depends on ARCH_BCM_IPROC || COMPILE_TEST
+ depends on HAS_IOMEM
+ default ARCH_BCM_IPROC
+ help
+ Say y here to enable read/write access to the Broadcom OTP
+ controller.
+
+ This driver can also be built as a module. If so, the module
+ will be called nvmem-bcm-ocotp.
+
config NVMEM_SUNXI_SID
tristate "Allwinner SoCs SID support"
depends on ARCH_SUNXI
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index 8f942a0..71781ca 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -6,6 +6,8 @@ obj-$(CONFIG_NVMEM) += nvmem_core.o
nvmem_core-y := core.o
# Devices
+obj-$(CONFIG_NVMEM_BCM_OCOTP) += nvmem-bcm-ocotp.o
+nvmem-bcm-ocotp-y := bcm-ocotp.o
obj-$(CONFIG_NVMEM_IMX_OCOTP) += nvmem-imx-ocotp.o
nvmem-imx-ocotp-y := imx-ocotp.o
obj-$(CONFIG_NVMEM_LPC18XX_EEPROM) += nvmem_lpc18xx_eeprom.o
diff --git a/drivers/nvmem/bcm-ocotp.c b/drivers/nvmem/bcm-ocotp.c
new file mode 100644
index 000..646cadb
--- /dev/null
+++ b/drivers/nvmem/bcm-ocotp.c
@@ -0,0 +1,335 @@
+/*
+ * Copyright (C) 2016 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/*
+ * # of tries for OTP Status. The time to execute a command varies. The slowest
+ * commands are writes which also vary based on the # of bits turned on.
Writing
+ * 0x takes ~3800 us.
+ */
+#define OTPC_RETRIES 5000
+
+/* Sequence to enable OTP program */
+#define OTPC_PROG_EN_SEQ { 0xf, 0x4, 0x8, 0xd }
+
+/* OTPC Commands */
+#define OTPC_CMD_READ0x0
+#define OTPC_CMD_OTP_PROG_ENABLE 0x2
+#define OTPC_CMD_OTP_PROG_DISABLE0x3
+#define OTPC_CMD_PROGRAM 0xA
+
+/* OTPC Status Bits */
+#define OTPC_STAT_CMD_DONE BIT(1)
+#define OTPC_STAT_PROG_OKBIT(2)
+
+/* OTPC register definition */
+#define OTPC_MODE_REG_OFFSET 0x0
+#define OTPC_MODE_REG_OTPC_MODE 0
+#define OTPC_COMMAND_OFFSET 0x4
+#define OTPC_COMMAND_COMMAND_WIDTH 6
+#define OTPC_CMD_START_OFFSET0x8
+#define OTPC_CMD_START_START 0
+#define OTPC_CPU_STATUS_OFFSET 0xc
+#define OTPC_CPUADDR_REG_OFFSET 0x28
+#define OTPC_CPUADDR_REG_OTPC_CPU_ADDRESS_WIDTH 16
+#define OTPC_CPU_WRITE_REG_OFFSET0x2c
+
+#define OTPC_CMD_MASK (BIT(OTPC_COMMAND_COMMAND_WIDTH) - 1)
+#define OTPC_ADDR_MASK (BIT(OTPC_CPUADDR_REG_OTPC_CPU_ADDRESS_WIDTH) - 1)
+
+
+struct otpc_map {
+ /* in words. */
+ u32 otpc_row_size;
+ /* 128 bit row / 4 words support. */
+ u16 data_r_offset[4];
+ /* 128 bit row / 4 words support. */
+ u16 data_w_offset[4];
+};
+
+static struct otpc_map otp_map = {
+ .otpc_row_size = 1,
+ .data_r_offset = {0x10},
+ .data_w_offset = {0x2c},
+};
+
+static struct otpc_map otp_map_v2 = {
+ .otpc_row_size = 2,
+ .data_r_offset = {0x10, 0x5c},
+ .data_w_offset = {0x2c, 0x64},
+};
+
+struct otpc_priv {
+ struct device *dev;
+ void __iomem*base;
+ struct otpc_map *map;
+ struct nvmem_config *config;
+};
+
+static inline void set_command(void __iomem *base, u32 command)
+{
+ writel(command & OTPC_CMD_MASK, base + OTPC_COMMAND_OFFSET);
[PATCH v1 3/3] ARM: dts: Add node for Broadcom OTP controller driver
From: Jonathan Richardson
Reviewed-by: Ray Jui
Tested-by: Jonathan Richardson
Signed-off-by: Scott Branden
Signed-off-by: Oza Pawandeep
Signed-off-by: Jonathan Richardson
---
arch/arm/boot/dts/bcm-cygnus.dtsi | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
b/arch/arm/boot/dts/bcm-cygnus.dtsi
index fabc9f3..a74a430 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -91,6 +91,13 @@
#address-cells = <1>;
#size-cells = <1>;
+ otp: otp@0301c800 {
+ compatible = "brcm,ocotp";
+ reg = <0x0301c800 0x2c>;
+ brcm,ocotp-size = <2048>;
+ status = "disabled";
+ };
+
pcie_phy: phy@0301d0a0 {
compatible = "brcm,cygnus-pcie-phy";
reg = <0x0301d0a0 0x14>;
--
1.9.1
[PATCH v1 2/3] nvmem: Add the Broadcom OTP controller driver
From: Jonathan Richardson
Add support for 32 and 64-bit versions of Broadcom's On-Chip OTP
controller. These controllers are used on SoC's such as Cygnus and
Stingray.
Reviewed-by: Ray Jui
Tested-by: Jonathan Richardson
Signed-off-by: Scott Branden
Signed-off-by: Oza Pawandeep
Signed-off-by: Jonathan Richardson
---
drivers/nvmem/Kconfig | 12 ++
drivers/nvmem/Makefile| 2 +
drivers/nvmem/bcm-ocotp.c | 335 ++
3 files changed, 349 insertions(+)
create mode 100644 drivers/nvmem/bcm-ocotp.c
diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index ba140ea..06935a7 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -80,6 +80,18 @@ config ROCKCHIP_EFUSE
This driver can also be built as a module. If so, the module
will be called nvmem_rockchip_efuse.
+config NVMEM_BCM_OCOTP
+ tristate "Broadcom On-Chip OTP Controller support"
+ depends on ARCH_BCM_IPROC || COMPILE_TEST
+ depends on HAS_IOMEM
+ default ARCH_BCM_IPROC
+ help
+ Say y here to enable read/write access to the Broadcom OTP
+ controller.
+
+ This driver can also be built as a module. If so, the module
+ will be called nvmem-bcm-ocotp.
+
config NVMEM_SUNXI_SID
tristate "Allwinner SoCs SID support"
depends on ARCH_SUNXI
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index 8f942a0..71781ca 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -6,6 +6,8 @@ obj-$(CONFIG_NVMEM) += nvmem_core.o
nvmem_core-y := core.o
# Devices
+obj-$(CONFIG_NVMEM_BCM_OCOTP) += nvmem-bcm-ocotp.o
+nvmem-bcm-ocotp-y := bcm-ocotp.o
obj-$(CONFIG_NVMEM_IMX_OCOTP) += nvmem-imx-ocotp.o
nvmem-imx-ocotp-y := imx-ocotp.o
obj-$(CONFIG_NVMEM_LPC18XX_EEPROM) += nvmem_lpc18xx_eeprom.o
diff --git a/drivers/nvmem/bcm-ocotp.c b/drivers/nvmem/bcm-ocotp.c
new file mode 100644
index 000..646cadb
--- /dev/null
+++ b/drivers/nvmem/bcm-ocotp.c
@@ -0,0 +1,335 @@
+/*
+ * Copyright (C) 2016 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/*
+ * # of tries for OTP Status. The time to execute a command varies. The slowest
+ * commands are writes which also vary based on the # of bits turned on.
Writing
+ * 0x takes ~3800 us.
+ */
+#define OTPC_RETRIES 5000
+
+/* Sequence to enable OTP program */
+#define OTPC_PROG_EN_SEQ { 0xf, 0x4, 0x8, 0xd }
+
+/* OTPC Commands */
+#define OTPC_CMD_READ0x0
+#define OTPC_CMD_OTP_PROG_ENABLE 0x2
+#define OTPC_CMD_OTP_PROG_DISABLE0x3
+#define OTPC_CMD_PROGRAM 0xA
+
+/* OTPC Status Bits */
+#define OTPC_STAT_CMD_DONE BIT(1)
+#define OTPC_STAT_PROG_OKBIT(2)
+
+/* OTPC register definition */
+#define OTPC_MODE_REG_OFFSET 0x0
+#define OTPC_MODE_REG_OTPC_MODE 0
+#define OTPC_COMMAND_OFFSET 0x4
+#define OTPC_COMMAND_COMMAND_WIDTH 6
+#define OTPC_CMD_START_OFFSET0x8
+#define OTPC_CMD_START_START 0
+#define OTPC_CPU_STATUS_OFFSET 0xc
+#define OTPC_CPUADDR_REG_OFFSET 0x28
+#define OTPC_CPUADDR_REG_OTPC_CPU_ADDRESS_WIDTH 16
+#define OTPC_CPU_WRITE_REG_OFFSET0x2c
+
+#define OTPC_CMD_MASK (BIT(OTPC_COMMAND_COMMAND_WIDTH) - 1)
+#define OTPC_ADDR_MASK (BIT(OTPC_CPUADDR_REG_OTPC_CPU_ADDRESS_WIDTH) - 1)
+
+
+struct otpc_map {
+ /* in words. */
+ u32 otpc_row_size;
+ /* 128 bit row / 4 words support. */
+ u16 data_r_offset[4];
+ /* 128 bit row / 4 words support. */
+ u16 data_w_offset[4];
+};
+
+static struct otpc_map otp_map = {
+ .otpc_row_size = 1,
+ .data_r_offset = {0x10},
+ .data_w_offset = {0x2c},
+};
+
+static struct otpc_map otp_map_v2 = {
+ .otpc_row_size = 2,
+ .data_r_offset = {0x10, 0x5c},
+ .data_w_offset = {0x2c, 0x64},
+};
+
+struct otpc_priv {
+ struct device *dev;
+ void __iomem*base;
+ struct otpc_map *map;
+ struct nvmem_config *config;
+};
+
+static inline void set_command(void __iomem *base, u32 command)
+{
+ writel(command & OTPC_CMD_MASK, base + OTPC_COMMAND_OFFSET);
+}
+
+static inline void set_cpu_address(void __iomem *base, u32 addr)
+{
+ writel(addr & OTPC_ADDR_MASK, base + OTPC_CPUADDR_REG_OFFSET);
+}
+
+static inline void set_start_bit(void __iomem *base)
+{
+
[PATCH v1 1/3] dt-bindings: Document Broadcom OTP controller driver
From: Jonathan Richardson <jonat...@broadcom.com>
Reviewed-by: Ray Jui <ray@broadcom.com>
Tested-by: Jonathan Richardson <jonathan.richard...@broadcom.com>
Signed-off-by: Scott Branden <scott.bran...@broadcom.com>
Signed-off-by: Oza Pawandeep <o...@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathan.richard...@broadcom.com>
---
Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt | 17 +
1 file changed, 17 insertions(+)
create mode 100644 Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
diff --git a/Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
b/Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
new file mode 100644
index 000..6462e12
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
@@ -0,0 +1,17 @@
+Broadcom OTP memory controller
+
+Required Properties:
+- compatible: "brcm,ocotp" for the first generation Broadcom OTPC which is used
+ in Cygnus and supports 32 bit read/write. Use "brcm,ocotp-v2" for the second
+ generation Broadcom OTPC which is used in SoC's such as Stingray and supports
+ 64-bit read/write.
+- reg: Base address of the OTP controller.
+- brcm,ocotp-size: Amount of memory available, in 32 bit words
+
+Example:
+
+otp: otp@0301c800 {
+ compatible = "brcm,ocotp";
+ reg = <0x0301c800 0x2c>;
+ brcm,ocotp-size = <2048>;
+};
--
1.9.1
[PATCH v1 1/3] dt-bindings: Document Broadcom OTP controller driver
From: Jonathan Richardson
Reviewed-by: Ray Jui
Tested-by: Jonathan Richardson
Signed-off-by: Scott Branden
Signed-off-by: Oza Pawandeep
Signed-off-by: Jonathan Richardson
---
Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt | 17 +
1 file changed, 17 insertions(+)
create mode 100644 Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
diff --git a/Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
b/Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
new file mode 100644
index 000..6462e12
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt
@@ -0,0 +1,17 @@
+Broadcom OTP memory controller
+
+Required Properties:
+- compatible: "brcm,ocotp" for the first generation Broadcom OTPC which is used
+ in Cygnus and supports 32 bit read/write. Use "brcm,ocotp-v2" for the second
+ generation Broadcom OTPC which is used in SoC's such as Stingray and supports
+ 64-bit read/write.
+- reg: Base address of the OTP controller.
+- brcm,ocotp-size: Amount of memory available, in 32 bit words
+
+Example:
+
+otp: otp@0301c800 {
+ compatible = "brcm,ocotp";
+ reg = <0x0301c800 0x2c>;
+ brcm,ocotp-size = <2048>;
+};
--
1.9.1
[PATCH v1 0/3] Add support for Broadcom OTP controller
This patch set adds support for Broadcom's OTP controller found on chips such as Cygnus and Stingray. A node has been added to the Cygnus dts. Jonathan Richardson (3): dt-bindings: Document Broadcom OTP controller driver nvmem: Add the Broadcom OTP controller driver ARM: dts: Add node for Broadcom OTP controller driver .../devicetree/bindings/nvmem/brcm,ocotp.txt | 17 ++ arch/arm/boot/dts/bcm-cygnus.dtsi | 7 + drivers/nvmem/Kconfig | 12 + drivers/nvmem/Makefile | 2 + drivers/nvmem/bcm-ocotp.c | 335 + 5 files changed, 373 insertions(+) create mode 100644 Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt create mode 100644 drivers/nvmem/bcm-ocotp.c -- 1.9.1
[PATCH v1 0/3] Add support for Broadcom OTP controller
This patch set adds support for Broadcom's OTP controller found on chips such as Cygnus and Stingray. A node has been added to the Cygnus dts. Jonathan Richardson (3): dt-bindings: Document Broadcom OTP controller driver nvmem: Add the Broadcom OTP controller driver ARM: dts: Add node for Broadcom OTP controller driver .../devicetree/bindings/nvmem/brcm,ocotp.txt | 17 ++ arch/arm/boot/dts/bcm-cygnus.dtsi | 7 + drivers/nvmem/Kconfig | 12 + drivers/nvmem/Makefile | 2 + drivers/nvmem/bcm-ocotp.c | 335 + 5 files changed, 373 insertions(+) create mode 100644 Documentation/devicetree/bindings/nvmem/brcm,ocotp.txt create mode 100644 drivers/nvmem/bcm-ocotp.c -- 1.9.1
Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable
_softirq) from [] >>>>>>>> (__do_softirq+0x134/0x2c0) >>>>>>>> [ 16.149982] [] (__do_softirq) from [] >>>>>>>> (irq_exit+0xd0/0x114) >>>>>>>> [ 16.157255] [] (irq_exit) from [] >>>>>>>> (__handle_domain_irq+0x70/0xe4) >>>>>>>> [ 16.165056] [] (__handle_domain_irq) from [] >>>>>>>> (gic_handle_irq+0x4c/0x94) >>>>>>>> [ 16.173376] [] (gic_handle_irq) from [] >>>>>>>> (__irq_svc+0x58/0x98) >>>>>>>> [ 16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0) >>>>>>>> [ 16.185823] df40: >>>>>>>>c0010dcc >>>>>>>> [ 16.193997] df60: c08cdfa8 c05f57c4 c08ca520 >>>>>>>> c08ce4bc c08c7364 c08ce4b4 >>>>>>>> [ 16.202141] df80: c09121ca 0001 c08cdfa8 c0010dcc >>>>>>>> c0010dd0 600f0013 >>>>>>>> [ 16.210291] [] (__irq_svc) from [] >>>>>>>> (arch_cpu_idle+0x20/0x3c) >>>>>>>> [ 16.217661] [] (arch_cpu_idle) from [] >>>>>>>> (cpu_startup_entry+0x17c/0x26c) >>>>>>>> [ 16.225899] [] (cpu_startup_entry) from [] >>>>>>>> (start_kernel+0x368/0x3d0) >>>>>>>> >>>>>>>> Best regards, >>>>>>>> Krzysztof >>>>>>>> >>>>>>>> >>>>>>>>> [2.701737] Modules linked in: >>>>>>>>> [2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted >>>>>>>>> 4.4.0-rc1-xu4f >>>>>>>>> #24 >>>>>>>>> [2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) >>>>>>>>> [2.701787] [] (unwind_backtrace) from [] >>>>>>>>> (show_stack+0x10/0x14) >>>>>>>>> [2.701808] [] (show_stack) from [] >>>>>>>>> (dump_stack+0x84/0xc4) >>>>>>>>> [2.701824] [] (dump_stack) from [] >>>>>>>>> (warn_slowpath_common+0x80/0xb0) >>>>>>>>> [2.701836] [] (warn_slowpath_common) from [] >>>>>>>>> (warn_slowpath_fmt+0x30/0x40) >>>>>>>>> [2.701849] [] (warn_slowpath_fmt) from [] >>>>>>>>> (__mutex_lock_slowpath+0x3bc/0x404) >>>>>>>>> [2.701864] [] (__mutex_lock_slowpath) from [] >>>>>>>>> (mutex_lock+0xc/0x24) >>>>>>>>> [2.701884] [] (mutex_lock) from [] >>>>>>>>> (pwm_enable+0x20/0x7c) >>>>>>>>> [2.701903] [] (pwm_enable) from [] >>>>>>>>> (led_heartbeat_function+0x74/0x144) >>>>>>>>> [2.701919] [] (led_heartbeat_function) from [] >>>>>>>>> (call_timer_fn+0x24/0x98) >>>>>>>>> [2.701932] [] (call_timer_fn) from [] >>>>>>>>> (run_timer_softirq+0x160/0x21c) >>>>>>>>> [2.701946] [] (run_timer_softirq) from [] >>>>>>>>> (__do_softirq+0x110/0x228) >>>>>>>>> [2.701959] [] (__do_softirq) from [] >>>>>>>>> (irq_exit+0xc0/0xfc) >>>>>>>>> [2.701976] [] (irq_exit) from [] >>>>>>>>> (__handle_domain_irq+0x80/0xec) >>>>>>>>> [2.701990] [] (__handle_domain_irq) from [] >>>>>>>>> (gic_handle_irq+0x54/0x94) >>>>>>>>> [2.702001] [] (gic_handle_irq) from [] >>>>>>>>> (__irq_svc+0x54/0x90) >>>>>>>>> [2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0) >>>>>>>>> [2.702019] df80: 0001 >>>>>>>>> c001b720 ee8bc000 c0573354 >>>>>>>>> [2.702031] dfa0: ee8bdfe0 c07f92e4 c08004b4 >>>>>>>>> c08004bc f0806640 ee8bdfd8 >>>>>>>>> [2.702039] dfc0: c0010180 c0010184 6013 >>>>>>>>> [2.702053] [] (__irq_svc) from [] >>>>>>>>> (arch_cpu_idle+0x38/0x3c) >>>>>>>>> [2.702073] [] (arch_cpu_idle) from
Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable
>>>>> [ 16.133606] [] (call_timer_fn) from [] >>>>>>>> (run_timer_softirq+0x1a8/0x230) >>>>>>>> [ 16.141846] [] (run_timer_softirq) from [] >>>>>>>> (__do_softirq+0x134/0x2c0) >>>>>>>> [ 16.149982] [] (__do_softirq) from [] >>>>>>>> (irq_exit+0xd0/0x114) >>>>>>>> [ 16.157255] [] (irq_exit) from [] >>>>>>>> (__handle_domain_irq+0x70/0xe4) >>>>>>>> [ 16.165056] [] (__handle_domain_irq) from [] >>>>>>>> (gic_handle_irq+0x4c/0x94) >>>>>>>> [ 16.173376] [] (gic_handle_irq) from [] >>>>>>>> (__irq_svc+0x58/0x98) >>>>>>>> [ 16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0) >>>>>>>> [ 16.185823] df40: >>>>>>>>c0010dcc >>>>>>>> [ 16.193997] df60: c08cdfa8 c05f57c4 c08ca520 >>>>>>>> c08ce4bc c08c7364 c08ce4b4 >>>>>>>> [ 16.202141] df80: c09121ca 0001 c08cdfa8 c0010dcc >>>>>>>> c0010dd0 600f0013 >>>>>>>> [ 16.210291] [] (__irq_svc) from [] >>>>>>>> (arch_cpu_idle+0x20/0x3c) >>>>>>>> [ 16.217661] [] (arch_cpu_idle) from [] >>>>>>>> (cpu_startup_entry+0x17c/0x26c) >>>>>>>> [ 16.225899] [] (cpu_startup_entry) from [] >>>>>>>> (start_kernel+0x368/0x3d0) >>>>>>>> >>>>>>>> Best regards, >>>>>>>> Krzysztof >>>>>>>> >>>>>>>> >>>>>>>>> [2.701737] Modules linked in: >>>>>>>>> [2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted >>>>>>>>> 4.4.0-rc1-xu4f >>>>>>>>> #24 >>>>>>>>> [2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) >>>>>>>>> [2.701787] [] (unwind_backtrace) from [] >>>>>>>>> (show_stack+0x10/0x14) >>>>>>>>> [2.701808] [] (show_stack) from [] >>>>>>>>> (dump_stack+0x84/0xc4) >>>>>>>>> [2.701824] [] (dump_stack) from [] >>>>>>>>> (warn_slowpath_common+0x80/0xb0) >>>>>>>>> [2.701836] [] (warn_slowpath_common) from [] >>>>>>>>> (warn_slowpath_fmt+0x30/0x40) >>>>>>>>> [2.701849] [] (warn_slowpath_fmt) from [] >>>>>>>>> (__mutex_lock_slowpath+0x3bc/0x404) >>>>>>>>> [2.701864] [] (__mutex_lock_slowpath) from [] >>>>>>>>> (mutex_lock+0xc/0x24) >>>>>>>>> [2.701884] [] (mutex_lock) from [] >>>>>>>>> (pwm_enable+0x20/0x7c) >>>>>>>>> [2.701903] [] (pwm_enable) from [] >>>>>>>>> (led_heartbeat_function+0x74/0x144) >>>>>>>>> [2.701919] [] (led_heartbeat_function) from [] >>>>>>>>> (call_timer_fn+0x24/0x98) >>>>>>>>> [2.701932] [] (call_timer_fn) from [] >>>>>>>>> (run_timer_softirq+0x160/0x21c) >>>>>>>>> [2.701946] [] (run_timer_softirq) from [] >>>>>>>>> (__do_softirq+0x110/0x228) >>>>>>>>> [2.701959] [] (__do_softirq) from [] >>>>>>>>> (irq_exit+0xc0/0xfc) >>>>>>>>> [2.701976] [] (irq_exit) from [] >>>>>>>>> (__handle_domain_irq+0x80/0xec) >>>>>>>>> [2.701990] [] (__handle_domain_irq) from [] >>>>>>>>> (gic_handle_irq+0x54/0x94) >>>>>>>>> [2.702001] [] (gic_handle_irq) from [] >>>>>>>>> (__irq_svc+0x54/0x90) >>>>>>>>> [2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0) >>>>>>>>> [2.702019] df80: 0001 >>>>>>>>> c001b720 ee8bc000 c0573354 >>>>>>>>> [2.702031] dfa0: ee8bdfe0 c07f92e4 c08004b4 >>>>>>>>> c08004bc f0806640 ee8bdfd8 >>>>>>>>> [2.702039] dfc0: c0010180 c0010184 6013 >>>>>>>
[PATCH v10 0/1] Fix bugs in kona pwm driver and pwm core
Fixes a bug in the pwm core where the enabled state was incorrect on failed calls to enable. Changes from v9: - pwm_set_polarity() enabled check is protected using the same mutex as pwm_enable() in pwm core. Implemented as suggested by Thierry. Changes from v8: - Accepted patches not included. Patch to introduce debug statements to config in kona pwm driver not included - will be addressed later. - Added mutex to pwm core enable function to prevent potential concurrency issue, as suggested by Thierry. - Fixed commit message for kona pwm changes. Also changed wording in comments from enable to trigger. Changes from v7: - Polarity changes take effect immediately instead of being deferred until enable is called. Changes from v6: - Move new debugging info for duty cycle and period in config function into its own commit. - Add kona_pwmc_prepare_for_settings() function to remove duplicated code. Changes from v5: - Address Dmitry's comment on code cleanup of pwm_enable() in pwm core. - Including all patches from different contributors in this patchset. Some were left out in v4. Changes from v4: - Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity. - Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework. Jonathan Richardson (1): pwm: core: Set enable state properly on failed call to enable drivers/pwm/core.c | 33 ++--- include/linux/pwm.h | 2 ++ 2 files changed, 28 insertions(+), 7 deletions(-) -- 1.9.1 -- 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 v10 1/1] pwm: core: Set enable state properly on failed call to enable
The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.
Tested-by: Jonathan Richardson
Reviewed-by: Dmitry Torokhov
Signed-off-by: Jonathan Richardson
---
drivers/pwm/core.c | 33 ++---
include/linux/pwm.h | 2 ++
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3f9df3e..b8f6c30 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -269,6 +269,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
pwm->pwm = chip->base + i;
pwm->hwpwm = i;
pwm->polarity = polarity;
+ mutex_init(>lock);
radix_tree_insert(_tree, pwm->pwm, pwm);
}
@@ -473,16 +474,22 @@ int pwm_set_polarity(struct pwm_device *pwm, enum
pwm_polarity polarity)
if (!pwm->chip->ops->set_polarity)
return -ENOSYS;
- if (pwm_is_enabled(pwm))
- return -EBUSY;
+ mutex_lock(>lock);
+
+ if (pwm_is_enabled(pwm)) {
+ err = -EBUSY;
+ goto unlock;
+ }
err = pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
if (err)
- return err;
+ goto unlock;
pwm->polarity = polarity;
- return 0;
+unlock:
+ mutex_unlock(>lock);
+ return err;
}
EXPORT_SYMBOL_GPL(pwm_set_polarity);
@@ -494,10 +501,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
*/
int pwm_enable(struct pwm_device *pwm)
{
- if (pwm && !test_and_set_bit(PWMF_ENABLED, >flags))
- return pwm->chip->ops->enable(pwm->chip, pwm);
+ int err = 0;
+
+ if (!pwm)
+ return -EINVAL;
+
+ mutex_lock(>lock);
+
+ if (!test_and_set_bit(PWMF_ENABLED, >flags)) {
+ err = pwm->chip->ops->enable(pwm->chip, pwm);
+ if (err)
+ clear_bit(PWMF_ENABLED, >flags);
+ }
+
+ mutex_unlock(>lock);
- return pwm ? 0 : -EINVAL;
+ return err;
}
EXPORT_SYMBOL_GPL(pwm_enable);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d681f68..dee7d65 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -3,6 +3,7 @@
#include
#include
+#include
struct pwm_device;
struct seq_file;
@@ -98,6 +99,7 @@ struct pwm_device {
unsigned int pwm;
struct pwm_chip *chip;
void *chip_data;
+ struct mutex lock;
unsigned int period;
unsigned int duty_cycle;
--
1.9.1
--
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 v9 2/2] pwm: core: Set enable state properly on failed call to enable
On 15-08-17 07:31 AM, Thierry Reding wrote:
> On Mon, Jun 15, 2015 at 04:22:27PM -0700, Jonathan Richardson wrote:
>> On 15-06-15 02:21 PM, Jonathan Richardson wrote:
>>> The pwm_enable function didn't clear the enabled bit if a call to a
>>> clients enable function returned an error. The result was that the state
>>> of the pwm core was wrong. Clearing the bit when enable returns an error
>>> ensures the state is properly set.
>>>
>>> Tested-by: Jonathan Richardson
>>> Reviewed-by: Dmitry Torokhov
>>> Signed-off-by: Jonathan Richardson
>>> ---
>>> drivers/pwm/core.c | 19 ---
>>> include/linux/pwm.h |2 ++
>>> 2 files changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>> index 76b0386..c255267 100644
>>> --- a/drivers/pwm/core.c
>>> +++ b/drivers/pwm/core.c
>>> @@ -263,6 +263,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>>> pwm->pwm = chip->base + i;
>>> pwm->hwpwm = i;
>>> pwm->polarity = polarity;
>>> + mutex_init(>lock);
>>>
>>> radix_tree_insert(_tree, pwm->pwm, pwm);
>>> }
>>> @@ -474,10 +475,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
>>> */
>>> int pwm_enable(struct pwm_device *pwm)
>>> {
>>> - if (pwm && !test_and_set_bit(PWMF_ENABLED, >flags))
>>> - return pwm->chip->ops->enable(pwm->chip, pwm);
>>> + int err = 0;
>>>
>>> - return pwm ? 0 : -EINVAL;
>>> + if (!pwm)
>>> + return -EINVAL;
>>> +
>>> + mutex_lock(>lock);
>>> +
>>> + if (!test_and_set_bit(PWMF_ENABLED, >flags)) {
>>> + err = pwm->chip->ops->enable(pwm->chip, pwm);
>>> + if (err)
>>> + clear_bit(PWMF_ENABLED, >flags);
>>> + }
>>> +
>>> + mutex_unlock(>lock);
>>> +
>>> + return err;
>>> }
>>> EXPORT_SYMBOL_GPL(pwm_enable);
>>
>> I meant to add the mutex check in disable also, but what about when
>> PWMF_ENABLED is checked in pwm_set_polarity() and pwm_dbg_show()?
>
> I think for debugfs we're fine since there's no potential to race there.
> It'll simply show the state of the PWM at the point where it was queried
> even though that may change immediately after. I suppose we could keep
> the lock across the body of the loop just to make sure debugfs will show
> a consistent view of the PWM.
>
> For pwm_disable() I don't think we need the lock, since the test_and_
> clear_bit() is atomic and ->disable() cannot fail.
>
> As for pwm_set_polarity(), I think it would need to be something like
> the below:
>
> >8
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3f9df3ea3350..8488c7a19bf6 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -473,16 +473,22 @@ int pwm_set_polarity(struct pwm_device *pwm, enum
> pwm_polarity polarity)
> if (!pwm->chip->ops->set_polarity)
> return -ENOSYS;
>
> - if (pwm_is_enabled(pwm))
> - return -EBUSY;
> + mutex_lock(>lock);
> +
> + if (pwm_is_enabled(pwm)) {
> + err = -EBUSY;
> + goto unlock;
> + }
>
> err = pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
> if (err)
> - return err;
> + goto unlock;
>
> pwm->polarity = polarity;
>
> - return 0;
> +unlock:
> + mutex_unlock(>lock);
> + return err;
> }
> EXPORT_SYMBOL_GPL(pwm_set_polarity);
>
>
Thierry,
Sounds good to me. I'll send a patch out for this hopefully today. I
don't see a need to complicate debugfs with obscure functionality so the
patch will just add polarity as you have shown it here and the enable
routine as the previous patchset.
Jon
--
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 v10 1/1] pwm: core: Set enable state properly on failed call to enable
The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.
Tested-by: Jonathan Richardson <jonat...@broadcom.com>
Reviewed-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
Signed-off-by: Jonathan Richardson <jonat...@broadcom.com>
---
drivers/pwm/core.c | 33 ++---
include/linux/pwm.h | 2 ++
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3f9df3e..b8f6c30 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -269,6 +269,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
pwm->pwm = chip->base + i;
pwm->hwpwm = i;
pwm->polarity = polarity;
+ mutex_init(>lock);
radix_tree_insert(_tree, pwm->pwm, pwm);
}
@@ -473,16 +474,22 @@ int pwm_set_polarity(struct pwm_device *pwm, enum
pwm_polarity polarity)
if (!pwm->chip->ops->set_polarity)
return -ENOSYS;
- if (pwm_is_enabled(pwm))
- return -EBUSY;
+ mutex_lock(>lock);
+
+ if (pwm_is_enabled(pwm)) {
+ err = -EBUSY;
+ goto unlock;
+ }
err = pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
if (err)
- return err;
+ goto unlock;
pwm->polarity = polarity;
- return 0;
+unlock:
+ mutex_unlock(>lock);
+ return err;
}
EXPORT_SYMBOL_GPL(pwm_set_polarity);
@@ -494,10 +501,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
*/
int pwm_enable(struct pwm_device *pwm)
{
- if (pwm && !test_and_set_bit(PWMF_ENABLED, >flags))
- return pwm->chip->ops->enable(pwm->chip, pwm);
+ int err = 0;
+
+ if (!pwm)
+ return -EINVAL;
+
+ mutex_lock(>lock);
+
+ if (!test_and_set_bit(PWMF_ENABLED, >flags)) {
+ err = pwm->chip->ops->enable(pwm->chip, pwm);
+ if (err)
+ clear_bit(PWMF_ENABLED, >flags);
+ }
+
+ mutex_unlock(>lock);
- return pwm ? 0 : -EINVAL;
+ return err;
}
EXPORT_SYMBOL_GPL(pwm_enable);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d681f68..dee7d65 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -3,6 +3,7 @@
#include
#include
+#include
struct pwm_device;
struct seq_file;
@@ -98,6 +99,7 @@ struct pwm_device {
unsigned int pwm;
struct pwm_chip *chip;
void *chip_data;
+ struct mutex lock;
unsigned int period;
unsigned int duty_cycle;
--
1.9.1
--
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 v10 0/1] Fix bugs in kona pwm driver and pwm core
Fixes a bug in the pwm core where the enabled state was incorrect on failed calls to enable. Changes from v9: - pwm_set_polarity() enabled check is protected using the same mutex as pwm_enable() in pwm core. Implemented as suggested by Thierry. Changes from v8: - Accepted patches not included. Patch to introduce debug statements to config in kona pwm driver not included - will be addressed later. - Added mutex to pwm core enable function to prevent potential concurrency issue, as suggested by Thierry. - Fixed commit message for kona pwm changes. Also changed wording in comments from enable to trigger. Changes from v7: - Polarity changes take effect immediately instead of being deferred until enable is called. Changes from v6: - Move new debugging info for duty cycle and period in config function into its own commit. - Add kona_pwmc_prepare_for_settings() function to remove duplicated code. Changes from v5: - Address Dmitry's comment on code cleanup of pwm_enable() in pwm core. - Including all patches from different contributors in this patchset. Some were left out in v4. Changes from v4: - Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity. - Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework. Jonathan Richardson (1): pwm: core: Set enable state properly on failed call to enable drivers/pwm/core.c | 33 ++--- include/linux/pwm.h | 2 ++ 2 files changed, 28 insertions(+), 7 deletions(-) -- 1.9.1 -- 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 v9 2/2] pwm: core: Set enable state properly on failed call to enable
On 15-08-17 07:31 AM, Thierry Reding wrote:
> On Mon, Jun 15, 2015 at 04:22:27PM -0700, Jonathan Richardson wrote:
>> On 15-06-15 02:21 PM, Jonathan Richardson wrote:
>>> The pwm_enable function didn't clear the enabled bit if a call to a
>>> clients enable function returned an error. The result was that the state
>>> of the pwm core was wrong. Clearing the bit when enable returns an error
>>> ensures the state is properly set.
>>>
>>> Tested-by: Jonathan Richardson <jonat...@broadcom.com>
>>> Reviewed-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
>>> Signed-off-by: Jonathan Richardson <jonat...@broadcom.com>
>>> ---
>>> drivers/pwm/core.c | 19 ---
>>> include/linux/pwm.h |2 ++
>>> 2 files changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>> index 76b0386..c255267 100644
>>> --- a/drivers/pwm/core.c
>>> +++ b/drivers/pwm/core.c
>>> @@ -263,6 +263,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>>> pwm->pwm = chip->base + i;
>>> pwm->hwpwm = i;
>>> pwm->polarity = polarity;
>>> + mutex_init(>lock);
>>>
>>> radix_tree_insert(_tree, pwm->pwm, pwm);
>>> }
>>> @@ -474,10 +475,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
>>> */
>>> int pwm_enable(struct pwm_device *pwm)
>>> {
>>> - if (pwm && !test_and_set_bit(PWMF_ENABLED, >flags))
>>> - return pwm->chip->ops->enable(pwm->chip, pwm);
>>> + int err = 0;
>>>
>>> - return pwm ? 0 : -EINVAL;
>>> + if (!pwm)
>>> + return -EINVAL;
>>> +
>>> + mutex_lock(>lock);
>>> +
>>> + if (!test_and_set_bit(PWMF_ENABLED, >flags)) {
>>> + err = pwm->chip->ops->enable(pwm->chip, pwm);
>>> + if (err)
>>> + clear_bit(PWMF_ENABLED, >flags);
>>> + }
>>> +
>>> + mutex_unlock(>lock);
>>> +
>>> + return err;
>>> }
>>> EXPORT_SYMBOL_GPL(pwm_enable);
>>
>> I meant to add the mutex check in disable also, but what about when
>> PWMF_ENABLED is checked in pwm_set_polarity() and pwm_dbg_show()?
>
> I think for debugfs we're fine since there's no potential to race there.
> It'll simply show the state of the PWM at the point where it was queried
> even though that may change immediately after. I suppose we could keep
> the lock across the body of the loop just to make sure debugfs will show
> a consistent view of the PWM.
>
> For pwm_disable() I don't think we need the lock, since the test_and_
> clear_bit() is atomic and ->disable() cannot fail.
>
> As for pwm_set_polarity(), I think it would need to be something like
> the below:
>
> >8
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3f9df3ea3350..8488c7a19bf6 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -473,16 +473,22 @@ int pwm_set_polarity(struct pwm_device *pwm, enum
> pwm_polarity polarity)
> if (!pwm->chip->ops->set_polarity)
> return -ENOSYS;
>
> - if (pwm_is_enabled(pwm))
> - return -EBUSY;
> + mutex_lock(>lock);
> +
> + if (pwm_is_enabled(pwm)) {
> + err = -EBUSY;
> + goto unlock;
> + }
>
> err = pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity);
> if (err)
> - return err;
> + goto unlock;
>
> pwm->polarity = polarity;
>
> - return 0;
> +unlock:
> + mutex_unlock(>lock);
> + return err;
> }
> EXPORT_SYMBOL_GPL(pwm_set_polarity);
>
>
Thierry,
Sounds good to me. I'll send a patch out for this hopefully today. I
don't see a need to complicate debugfs with obscure functionality so the
patch will just add polarity as you have shown it here and the enable
routine as the previous patchset.
Jon
--
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 v9 1/2] pwm: kona: Modify settings application sequence
Hi Theirry,
If there are no more comments can this patch be applied?
Thanks,
Jon
On 15-06-15 02:21 PM, Jonathan Richardson wrote:
> Update the driver so that settings are applied in accordance with the
> most recent version of the hardware spec. The revised sequence clears
> the trigger bit, waits 400ns, writes settings, sets the trigger bit,
> and waits another 400ns. This corrects an issue where occasionally a
> requested change was not properly reflected in the PWM output.
>
> Reviewed-by: Arun Ramamurthy
> Reviewed-by: Scott Branden
> Tested-by: Scott Branden
> Reviewed-by: Tim Kryger
> Signed-off-by: Jonathan Richardson
> ---
> drivers/pwm/pwm-bcm-kona.c | 47
> +++-
> 1 file changed, 38 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index 7af8fea..9b1d397 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -76,19 +76,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct
> pwm_chip *_chip)
> return container_of(_chip, struct kona_pwmc, chip);
> }
>
> -static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> +/*
> + * Clear trigger bit but set smooth bit to maintain old output.
> + */
> +static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
> + unsigned int chan)
> {
> unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>
> - /* Clear trigger bit but set smooth bit to maintain old output */
> value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
> value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
> writel(value, kp->base + PWM_CONTROL_OFFSET);
>
> + /*
> + * There must be a min 400ns delay between clearing trigger and setting
> + * it. Failing to do this may result in no PWM signal.
> + */
> + ndelay(400);
> +}
> +
> +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> +{
> + unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
> +
> /* Set trigger bit and clear smooth bit to apply new settings */
> value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
> value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
> writel(value, kp->base + PWM_CONTROL_OFFSET);
> +
> + /* Trigger bit must be held high for at least 400 ns. */
> + ndelay(400);
> }
>
> static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> @@ -133,8 +150,14 @@ static int kona_pwmc_config(struct pwm_chip *chip,
> struct pwm_device *pwm,
> return -EINVAL;
> }
>
> - /* If the PWM channel is enabled, write the settings to the HW */
> + /*
> + * Don't apply settings if disabled. The period and duty cycle are
> + * always calculated above to ensure the new values are
> + * validated immediately instead of on enable.
> + */
> if (test_bit(PWMF_ENABLED, >flags)) {
> + kona_pwmc_prepare_for_settings(kp, chan);
> +
> value = readl(kp->base + PRESCALE_OFFSET);
> value &= ~PRESCALE_MASK(chan);
> value |= prescale << PRESCALE_SHIFT(chan);
> @@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
> struct pwm_device *pwm,
> return ret;
> }
>
> + kona_pwmc_prepare_for_settings(kp, chan);
> +
> value = readl(kp->base + PWM_CONTROL_OFFSET);
>
> if (polarity == PWM_POLARITY_NORMAL)
> @@ -175,9 +200,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
> struct pwm_device *pwm,
>
> kona_pwmc_apply_settings(kp, chan);
>
> - /* Wait for waveform to settle before gating off the clock */
> - ndelay(400);
> -
> clk_disable_unprepare(kp->clk);
>
> return 0;
> @@ -207,13 +229,20 @@ static void kona_pwmc_disable(struct pwm_chip *chip,
> struct pwm_device *pwm)
> {
> struct kona_pwmc *kp = to_kona_pwmc(chip);
> unsigned int chan = pwm->hwpwm;
> + unsigned int value;
> +
> + kona_pwmc_prepare_for_settings(kp, chan);
>
> /* Simulate a disable by configuring for zero duty */
> writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> - kona_pwmc_apply_settings(kp, chan);
> + writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
>
> - /* Wait for waveform to settle before gating off the clock */
> - ndelay(400);
> + /* Set prescale to 0 for this channel */
> + value = readl(kp->base + PRESCALE_OFFSET);
> + value &= ~PRESCALE_MASK(chan);
> + writel(value, kp->base + PRESCALE_OFFSET);
> +
> + kona_pwmc_apply_settings(kp, chan);
>
> clk_disable_unprepare(kp->clk);
> }
>
--
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 v9 1/2] pwm: kona: Modify settings application sequence
Hi Theirry,
If there are no more comments can this patch be applied?
Thanks,
Jon
On 15-06-15 02:21 PM, Jonathan Richardson wrote:
Update the driver so that settings are applied in accordance with the
most recent version of the hardware spec. The revised sequence clears
the trigger bit, waits 400ns, writes settings, sets the trigger bit,
and waits another 400ns. This corrects an issue where occasionally a
requested change was not properly reflected in the PWM output.
Reviewed-by: Arun Ramamurthy arunr...@broadcom.com
Reviewed-by: Scott Branden sbran...@broadcom.com
Tested-by: Scott Branden sbran...@broadcom.com
Reviewed-by: Tim Kryger tim.kry...@gmail.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/pwm-bcm-kona.c | 47
+++-
1 file changed, 38 insertions(+), 9 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 7af8fea..9b1d397 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -76,19 +76,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct
pwm_chip *_chip)
return container_of(_chip, struct kona_pwmc, chip);
}
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+/*
+ * Clear trigger bit but set smooth bit to maintain old output.
+ */
+static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
+ unsigned int chan)
{
unsigned int value = readl(kp-base + PWM_CONTROL_OFFSET);
- /* Clear trigger bit but set smooth bit to maintain old output */
value |= 1 PWM_CONTROL_SMOOTH_SHIFT(chan);
value = ~(1 PWM_CONTROL_TRIGGER_SHIFT(chan));
writel(value, kp-base + PWM_CONTROL_OFFSET);
+ /*
+ * There must be a min 400ns delay between clearing trigger and setting
+ * it. Failing to do this may result in no PWM signal.
+ */
+ ndelay(400);
+}
+
+static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+{
+ unsigned int value = readl(kp-base + PWM_CONTROL_OFFSET);
+
/* Set trigger bit and clear smooth bit to apply new settings */
value = ~(1 PWM_CONTROL_SMOOTH_SHIFT(chan));
value |= 1 PWM_CONTROL_TRIGGER_SHIFT(chan);
writel(value, kp-base + PWM_CONTROL_OFFSET);
+
+ /* Trigger bit must be held high for at least 400 ns. */
+ ndelay(400);
}
static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -133,8 +150,14 @@ static int kona_pwmc_config(struct pwm_chip *chip,
struct pwm_device *pwm,
return -EINVAL;
}
- /* If the PWM channel is enabled, write the settings to the HW */
+ /*
+ * Don't apply settings if disabled. The period and duty cycle are
+ * always calculated above to ensure the new values are
+ * validated immediately instead of on enable.
+ */
if (test_bit(PWMF_ENABLED, pwm-flags)) {
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp-base + PRESCALE_OFFSET);
value = ~PRESCALE_MASK(chan);
value |= prescale PRESCALE_SHIFT(chan);
@@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
return ret;
}
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp-base + PWM_CONTROL_OFFSET);
if (polarity == PWM_POLARITY_NORMAL)
@@ -175,9 +200,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
kona_pwmc_apply_settings(kp, chan);
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
-
clk_disable_unprepare(kp-clk);
return 0;
@@ -207,13 +229,20 @@ static void kona_pwmc_disable(struct pwm_chip *chip,
struct pwm_device *pwm)
{
struct kona_pwmc *kp = to_kona_pwmc(chip);
unsigned int chan = pwm-hwpwm;
+ unsigned int value;
+
+ kona_pwmc_prepare_for_settings(kp, chan);
/* Simulate a disable by configuring for zero duty */
writel(0, kp-base + DUTY_CYCLE_HIGH_OFFSET(chan));
- kona_pwmc_apply_settings(kp, chan);
+ writel(0, kp-base + PERIOD_COUNT_OFFSET(chan));
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
+ /* Set prescale to 0 for this channel */
+ value = readl(kp-base + PRESCALE_OFFSET);
+ value = ~PRESCALE_MASK(chan);
+ writel(value, kp-base + PRESCALE_OFFSET);
+
+ kona_pwmc_apply_settings(kp, chan);
clk_disable_unprepare(kp-clk);
}
--
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 v9 2/2] pwm: core: Set enable state properly on failed call to enable
On 15-06-15 02:21 PM, Jonathan Richardson wrote:
> The pwm_enable function didn't clear the enabled bit if a call to a
> clients enable function returned an error. The result was that the state
> of the pwm core was wrong. Clearing the bit when enable returns an error
> ensures the state is properly set.
>
> Tested-by: Jonathan Richardson
> Reviewed-by: Dmitry Torokhov
> Signed-off-by: Jonathan Richardson
> ---
> drivers/pwm/core.c | 19 ---
> include/linux/pwm.h |2 ++
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 76b0386..c255267 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -263,6 +263,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> pwm->pwm = chip->base + i;
> pwm->hwpwm = i;
> pwm->polarity = polarity;
> + mutex_init(>lock);
>
> radix_tree_insert(_tree, pwm->pwm, pwm);
> }
> @@ -474,10 +475,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
> */
> int pwm_enable(struct pwm_device *pwm)
> {
> - if (pwm && !test_and_set_bit(PWMF_ENABLED, >flags))
> - return pwm->chip->ops->enable(pwm->chip, pwm);
> + int err = 0;
>
> - return pwm ? 0 : -EINVAL;
> + if (!pwm)
> + return -EINVAL;
> +
> + mutex_lock(>lock);
> +
> + if (!test_and_set_bit(PWMF_ENABLED, >flags)) {
> + err = pwm->chip->ops->enable(pwm->chip, pwm);
> + if (err)
> + clear_bit(PWMF_ENABLED, >flags);
> + }
> +
> + mutex_unlock(>lock);
> +
> + return err;
> }
> EXPORT_SYMBOL_GPL(pwm_enable);
I meant to add the mutex check in disable also, but what about when
PWMF_ENABLED is checked in pwm_set_polarity() and pwm_dbg_show()?
Thanks.
--
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 v9 1/2] pwm: kona: Modify settings application sequence
Update the driver so that settings are applied in accordance with the
most recent version of the hardware spec. The revised sequence clears
the trigger bit, waits 400ns, writes settings, sets the trigger bit,
and waits another 400ns. This corrects an issue where occasionally a
requested change was not properly reflected in the PWM output.
Reviewed-by: Arun Ramamurthy
Reviewed-by: Scott Branden
Tested-by: Scott Branden
Reviewed-by: Tim Kryger
Signed-off-by: Jonathan Richardson
---
drivers/pwm/pwm-bcm-kona.c | 47 +++-
1 file changed, 38 insertions(+), 9 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 7af8fea..9b1d397 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -76,19 +76,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct
pwm_chip *_chip)
return container_of(_chip, struct kona_pwmc, chip);
}
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+/*
+ * Clear trigger bit but set smooth bit to maintain old output.
+ */
+static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
+ unsigned int chan)
{
unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
- /* Clear trigger bit but set smooth bit to maintain old output */
value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
writel(value, kp->base + PWM_CONTROL_OFFSET);
+ /*
+* There must be a min 400ns delay between clearing trigger and setting
+* it. Failing to do this may result in no PWM signal.
+*/
+ ndelay(400);
+}
+
+static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+{
+ unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
/* Set trigger bit and clear smooth bit to apply new settings */
value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
writel(value, kp->base + PWM_CONTROL_OFFSET);
+
+ /* Trigger bit must be held high for at least 400 ns. */
+ ndelay(400);
}
static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -133,8 +150,14 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct
pwm_device *pwm,
return -EINVAL;
}
- /* If the PWM channel is enabled, write the settings to the HW */
+ /*
+* Don't apply settings if disabled. The period and duty cycle are
+* always calculated above to ensure the new values are
+* validated immediately instead of on enable.
+*/
if (test_bit(PWMF_ENABLED, >flags)) {
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp->base + PRESCALE_OFFSET);
value &= ~PRESCALE_MASK(chan);
value |= prescale << PRESCALE_SHIFT(chan);
@@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
return ret;
}
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp->base + PWM_CONTROL_OFFSET);
if (polarity == PWM_POLARITY_NORMAL)
@@ -175,9 +200,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
kona_pwmc_apply_settings(kp, chan);
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
-
clk_disable_unprepare(kp->clk);
return 0;
@@ -207,13 +229,20 @@ static void kona_pwmc_disable(struct pwm_chip *chip,
struct pwm_device *pwm)
{
struct kona_pwmc *kp = to_kona_pwmc(chip);
unsigned int chan = pwm->hwpwm;
+ unsigned int value;
+
+ kona_pwmc_prepare_for_settings(kp, chan);
/* Simulate a disable by configuring for zero duty */
writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
- kona_pwmc_apply_settings(kp, chan);
+ writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
+ /* Set prescale to 0 for this channel */
+ value = readl(kp->base + PRESCALE_OFFSET);
+ value &= ~PRESCALE_MASK(chan);
+ writel(value, kp->base + PRESCALE_OFFSET);
+
+ kona_pwmc_apply_settings(kp, chan);
clk_disable_unprepare(kp->clk);
}
--
1.7.9.5
--
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 v9 2/2] pwm: core: Set enable state properly on failed call to enable
The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.
Tested-by: Jonathan Richardson
Reviewed-by: Dmitry Torokhov
Signed-off-by: Jonathan Richardson
---
drivers/pwm/core.c | 19 ---
include/linux/pwm.h |2 ++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 76b0386..c255267 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -263,6 +263,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
pwm->pwm = chip->base + i;
pwm->hwpwm = i;
pwm->polarity = polarity;
+ mutex_init(>lock);
radix_tree_insert(_tree, pwm->pwm, pwm);
}
@@ -474,10 +475,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
*/
int pwm_enable(struct pwm_device *pwm)
{
- if (pwm && !test_and_set_bit(PWMF_ENABLED, >flags))
- return pwm->chip->ops->enable(pwm->chip, pwm);
+ int err = 0;
- return pwm ? 0 : -EINVAL;
+ if (!pwm)
+ return -EINVAL;
+
+ mutex_lock(>lock);
+
+ if (!test_and_set_bit(PWMF_ENABLED, >flags)) {
+ err = pwm->chip->ops->enable(pwm->chip, pwm);
+ if (err)
+ clear_bit(PWMF_ENABLED, >flags);
+ }
+
+ mutex_unlock(>lock);
+
+ return err;
}
EXPORT_SYMBOL_GPL(pwm_enable);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 9daf0ef..50f28e6 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -3,6 +3,7 @@
#include
#include
+#include
struct pwm_device;
struct seq_file;
@@ -86,6 +87,7 @@ struct pwm_device {
unsigned intpwm;
struct pwm_chip *chip;
void*chip_data;
+ struct mutexlock;
unsigned intperiod; /* in nanoseconds */
unsigned intduty_cycle; /* in nanoseconds */
--
1.7.9.5
--
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 v9 0/2] Fix bugs in kona pwm driver and pwm core
This patchset fixes a bug in the Broadcom Kona pwm driver. A new procedure for changing the pwm settings has been introduced. Also fixed a bug in the pwm core where the enabled state was incorrect on failed calls to enable. These changes are required for the Kona PWM driver to work on Cygnus. The same PWM IP is being used. Changes from v8: - Accepted patches not included. Patch to introduce debug statements to config in kona pwm driver not included - will be addressed later. - Added mutex to pwm core enable function to prevent potential concurrency issue, as suggested by Thierry. - Fixed commit message for kona pwm changes. Also changed wording in comments from enable to trigger. Changes from v7: - Polarity changes take effect immediately instead of being deferred until enable is called. Changes from v6: - Move new debugging info for duty cycle and period in config function into its own commit. - Add kona_pwmc_prepare_for_settings() function to remove duplicated code. Changes from v5: - Address Dmitry's comment on code cleanup of pwm_enable() in pwm core. - Including all patches from different contributors in this patchset. Some were left out in v4. Changes from v4: - Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity. - Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework. Jonathan Richardson (2): pwm: kona: Modify settings application sequence pwm: core: Set enable state properly on failed call to enable drivers/pwm/core.c | 19 +++--- drivers/pwm/pwm-bcm-kona.c | 47 +++- include/linux/pwm.h|2 ++ 3 files changed, 56 insertions(+), 12 deletions(-) -- 1.7.9.5 -- 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 v9 1/2] pwm: kona: Modify settings application sequence
Update the driver so that settings are applied in accordance with the
most recent version of the hardware spec. The revised sequence clears
the trigger bit, waits 400ns, writes settings, sets the trigger bit,
and waits another 400ns. This corrects an issue where occasionally a
requested change was not properly reflected in the PWM output.
Reviewed-by: Arun Ramamurthy arunr...@broadcom.com
Reviewed-by: Scott Branden sbran...@broadcom.com
Tested-by: Scott Branden sbran...@broadcom.com
Reviewed-by: Tim Kryger tim.kry...@gmail.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/pwm-bcm-kona.c | 47 +++-
1 file changed, 38 insertions(+), 9 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 7af8fea..9b1d397 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -76,19 +76,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct
pwm_chip *_chip)
return container_of(_chip, struct kona_pwmc, chip);
}
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+/*
+ * Clear trigger bit but set smooth bit to maintain old output.
+ */
+static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
+ unsigned int chan)
{
unsigned int value = readl(kp-base + PWM_CONTROL_OFFSET);
- /* Clear trigger bit but set smooth bit to maintain old output */
value |= 1 PWM_CONTROL_SMOOTH_SHIFT(chan);
value = ~(1 PWM_CONTROL_TRIGGER_SHIFT(chan));
writel(value, kp-base + PWM_CONTROL_OFFSET);
+ /*
+* There must be a min 400ns delay between clearing trigger and setting
+* it. Failing to do this may result in no PWM signal.
+*/
+ ndelay(400);
+}
+
+static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+{
+ unsigned int value = readl(kp-base + PWM_CONTROL_OFFSET);
+
/* Set trigger bit and clear smooth bit to apply new settings */
value = ~(1 PWM_CONTROL_SMOOTH_SHIFT(chan));
value |= 1 PWM_CONTROL_TRIGGER_SHIFT(chan);
writel(value, kp-base + PWM_CONTROL_OFFSET);
+
+ /* Trigger bit must be held high for at least 400 ns. */
+ ndelay(400);
}
static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -133,8 +150,14 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct
pwm_device *pwm,
return -EINVAL;
}
- /* If the PWM channel is enabled, write the settings to the HW */
+ /*
+* Don't apply settings if disabled. The period and duty cycle are
+* always calculated above to ensure the new values are
+* validated immediately instead of on enable.
+*/
if (test_bit(PWMF_ENABLED, pwm-flags)) {
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp-base + PRESCALE_OFFSET);
value = ~PRESCALE_MASK(chan);
value |= prescale PRESCALE_SHIFT(chan);
@@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
return ret;
}
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp-base + PWM_CONTROL_OFFSET);
if (polarity == PWM_POLARITY_NORMAL)
@@ -175,9 +200,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
kona_pwmc_apply_settings(kp, chan);
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
-
clk_disable_unprepare(kp-clk);
return 0;
@@ -207,13 +229,20 @@ static void kona_pwmc_disable(struct pwm_chip *chip,
struct pwm_device *pwm)
{
struct kona_pwmc *kp = to_kona_pwmc(chip);
unsigned int chan = pwm-hwpwm;
+ unsigned int value;
+
+ kona_pwmc_prepare_for_settings(kp, chan);
/* Simulate a disable by configuring for zero duty */
writel(0, kp-base + DUTY_CYCLE_HIGH_OFFSET(chan));
- kona_pwmc_apply_settings(kp, chan);
+ writel(0, kp-base + PERIOD_COUNT_OFFSET(chan));
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
+ /* Set prescale to 0 for this channel */
+ value = readl(kp-base + PRESCALE_OFFSET);
+ value = ~PRESCALE_MASK(chan);
+ writel(value, kp-base + PRESCALE_OFFSET);
+
+ kona_pwmc_apply_settings(kp, chan);
clk_disable_unprepare(kp-clk);
}
--
1.7.9.5
--
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 v9 2/2] pwm: core: Set enable state properly on failed call to enable
The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.
Tested-by: Jonathan Richardson jonat...@broadcom.com
Reviewed-by: Dmitry Torokhov dmitry.torok...@gmail.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/core.c | 19 ---
include/linux/pwm.h |2 ++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 76b0386..c255267 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -263,6 +263,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
pwm-pwm = chip-base + i;
pwm-hwpwm = i;
pwm-polarity = polarity;
+ mutex_init(pwm-lock);
radix_tree_insert(pwm_tree, pwm-pwm, pwm);
}
@@ -474,10 +475,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
*/
int pwm_enable(struct pwm_device *pwm)
{
- if (pwm !test_and_set_bit(PWMF_ENABLED, pwm-flags))
- return pwm-chip-ops-enable(pwm-chip, pwm);
+ int err = 0;
- return pwm ? 0 : -EINVAL;
+ if (!pwm)
+ return -EINVAL;
+
+ mutex_lock(pwm-lock);
+
+ if (!test_and_set_bit(PWMF_ENABLED, pwm-flags)) {
+ err = pwm-chip-ops-enable(pwm-chip, pwm);
+ if (err)
+ clear_bit(PWMF_ENABLED, pwm-flags);
+ }
+
+ mutex_unlock(pwm-lock);
+
+ return err;
}
EXPORT_SYMBOL_GPL(pwm_enable);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 9daf0ef..50f28e6 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -3,6 +3,7 @@
#include linux/err.h
#include linux/of.h
+#include linux/mutex.h
struct pwm_device;
struct seq_file;
@@ -86,6 +87,7 @@ struct pwm_device {
unsigned intpwm;
struct pwm_chip *chip;
void*chip_data;
+ struct mutexlock;
unsigned intperiod; /* in nanoseconds */
unsigned intduty_cycle; /* in nanoseconds */
--
1.7.9.5
--
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 v9 0/2] Fix bugs in kona pwm driver and pwm core
This patchset fixes a bug in the Broadcom Kona pwm driver. A new procedure for changing the pwm settings has been introduced. Also fixed a bug in the pwm core where the enabled state was incorrect on failed calls to enable. These changes are required for the Kona PWM driver to work on Cygnus. The same PWM IP is being used. Changes from v8: - Accepted patches not included. Patch to introduce debug statements to config in kona pwm driver not included - will be addressed later. - Added mutex to pwm core enable function to prevent potential concurrency issue, as suggested by Thierry. - Fixed commit message for kona pwm changes. Also changed wording in comments from enable to trigger. Changes from v7: - Polarity changes take effect immediately instead of being deferred until enable is called. Changes from v6: - Move new debugging info for duty cycle and period in config function into its own commit. - Add kona_pwmc_prepare_for_settings() function to remove duplicated code. Changes from v5: - Address Dmitry's comment on code cleanup of pwm_enable() in pwm core. - Including all patches from different contributors in this patchset. Some were left out in v4. Changes from v4: - Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity. - Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework. Jonathan Richardson (2): pwm: kona: Modify settings application sequence pwm: core: Set enable state properly on failed call to enable drivers/pwm/core.c | 19 +++--- drivers/pwm/pwm-bcm-kona.c | 47 +++- include/linux/pwm.h|2 ++ 3 files changed, 56 insertions(+), 12 deletions(-) -- 1.7.9.5 -- 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 v9 2/2] pwm: core: Set enable state properly on failed call to enable
On 15-06-15 02:21 PM, Jonathan Richardson wrote:
The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.
Tested-by: Jonathan Richardson jonat...@broadcom.com
Reviewed-by: Dmitry Torokhov dmitry.torok...@gmail.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/core.c | 19 ---
include/linux/pwm.h |2 ++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 76b0386..c255267 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -263,6 +263,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
pwm-pwm = chip-base + i;
pwm-hwpwm = i;
pwm-polarity = polarity;
+ mutex_init(pwm-lock);
radix_tree_insert(pwm_tree, pwm-pwm, pwm);
}
@@ -474,10 +475,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
*/
int pwm_enable(struct pwm_device *pwm)
{
- if (pwm !test_and_set_bit(PWMF_ENABLED, pwm-flags))
- return pwm-chip-ops-enable(pwm-chip, pwm);
+ int err = 0;
- return pwm ? 0 : -EINVAL;
+ if (!pwm)
+ return -EINVAL;
+
+ mutex_lock(pwm-lock);
+
+ if (!test_and_set_bit(PWMF_ENABLED, pwm-flags)) {
+ err = pwm-chip-ops-enable(pwm-chip, pwm);
+ if (err)
+ clear_bit(PWMF_ENABLED, pwm-flags);
+ }
+
+ mutex_unlock(pwm-lock);
+
+ return err;
}
EXPORT_SYMBOL_GPL(pwm_enable);
I meant to add the mutex check in disable also, but what about when
PWMF_ENABLED is checked in pwm_set_polarity() and pwm_dbg_show()?
Thanks.
--
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 v8 4/5] pwm: kona: Add debug info to config function
On 15-05-30 09:42 AM, Tim Kryger wrote:
> On Tue, May 26, 2015 at 1:08 PM, Jonathan Richardson
> wrote:
>> Adds debugging info to config function where duty cycle and period
>> are calculated and verified.
>>
>> Signed-off-by: Jonathan Richardson
>> ---
>> drivers/pwm/pwm-bcm-kona.c | 25 +++--
>> 1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>> index c87621f..0ddf19b 100644
>> --- a/drivers/pwm/pwm-bcm-kona.c
>> +++ b/drivers/pwm/pwm-bcm-kona.c
>> @@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip,
>> struct pwm_device *pwm,
>> dc = div64_u64(val, div);
>>
>> /* If duty_ns or period_ns are not achievable then return */
>> - if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
>
> The original code was based on the SPEAr PWM driver which has a non-zero
> PWMDCR_MIN_DUTY such that the second condition here can evaluate to true.
>
> This isn't the case for the Kona PWM where DUTY_CYCLE_HIGH_MIN is zero.
>
>> + if (pc < PERIOD_COUNT_MIN) {
>> + dev_warn(chip->dev,
>> + "%s: pwm[%d]: period=%d is not achievable,
>> pc=%lu, prescale=%lu\n",
>> + __func__, chan, period_ns, pc, prescale);
>> return -EINVAL;
>> + }
>
> Why not just print the minimum allowable period with the provided clock?
>
> I don't think pc and prescale will be particularly helpful to users.
>
> Also, do we really need to print __func__ here?
>
>> +
>> + if (dc < DUTY_CYCLE_HIGH_MIN) {
>> + if (0 != duty_ns) {
>> + dev_warn(chip->dev,
>> + "%s: pwm[%d]: duty cycle=%d is not
>> achievable, dc=%lu, prescale=%lu\n",
>> + __func__, chan, duty_ns, dc,
>> prescale);
>> + }
>> + return -EINVAL;
>> + }
>
> The above block is unreachable code.
>
>>
>> /* If pc and dc are in bounds, the calculation is done */
>> if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
>> break;
>>
>> /* Otherwise, increase prescale and recalculate pc and dc */
>> - if (++prescale > PRESCALE_MAX)
>> + if (++prescale > PRESCALE_MAX) {
>> + dev_warn(chip->dev,
>> + "%s: pwm[%d]: Prescale (=%lu) within max
>> (=%d) for period=%d and duty cycle=%d is not achievable\n",
>> + __func__, chan, prescale, PRESCALE_MAX,
>> + period_ns, duty_ns);
>> return -EINVAL;
>> + }
>> }
>
> The user got here because they specified a period larger than the maximum
> supported so why not tell them largest value that can be supported instead
> of confusing them with prescale and PRESCALE_MAX?
>
>>
>> + dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu,
>> prescale=%lu\n",
>> + chan, pc, dc, prescale);
>> +
>
> This could be more clear. It prints pc but calls it period.
>
>> /*
>> * Don't apply settings if disabled. The period and duty cycle are
>> * always calculated above to ensure the new values are
>> --
>> 1.7.9.5
>>
We can defer this for now until I can look into it further. It's not a
priority. I'm more concerned with core changes and kona pwm fix.
Thanks,
Jon
--
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 v8 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures
On 15-05-30 09:41 AM, Tim Kryger wrote:
> On Tue, May 26, 2015 at 1:08 PM, Jonathan Richardson
> wrote:
>> The config procedure didn't follow the spec which periodically resulted
>> in failing to enable the output signal. This happened one in ten or
>> twenty attempts. Following the spec and adding a 400ns delay in the
>> appropriate locations resolves this problem.
>>
>> The disable and polarity procedures now also follow the spec. The old
>> procedures would result in no change in signal when called.
>
> I think you may want to adjust your commit title and message to more
> clearly describe what this change is doing. Perhaps something like:
>
> pwm: kona: Modify settings application sequence
>
> Update the driver so that settings are applied in accordance with the
> most recent version of the hardware spec. The revised sequence clears
> the trigger bit, waits 400ns, writes settings, sets the trigger bit,
> and waits another 400ns. This corrects an issue where occasionally a
> requested change was not properly reflected in the PWM output.
>
> Otherwise, this patch looks reasonable so
>
> Reviewed-by: Tim Kryger
Fine with me. Same with two comments below. Will re-spin when I can see
Thierry's modification to use pwmchip_add_with_polarity() instead of
pwmchip_add_inversed().
Thanks.
>
>>
>> Reviewed-by: Arun Ramamurthy
>> Reviewed-by: Scott Branden
>> Tested-by: Scott Branden
>> Signed-off-by: Jonathan Richardson
>> ---
>> drivers/pwm/pwm-bcm-kona.c | 47
>> +++-
>> 1 file changed, 38 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>> index 32b3ec6..c87621f 100644
>> --- a/drivers/pwm/pwm-bcm-kona.c
>> +++ b/drivers/pwm/pwm-bcm-kona.c
>> @@ -76,19 +76,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct
>> pwm_chip *_chip)
>> return container_of(_chip, struct kona_pwmc, chip);
>> }
>>
>> -static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int
>> chan)
>> +/*
>> + * Clear trigger bit but set smooth bit to maintain old output.
>> + */
>> +static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
>> + unsigned int chan)
>> {
>> unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>>
>> - /* Clear trigger bit but set smooth bit to maintain old output */
>> value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
>> value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
>> writel(value, kp->base + PWM_CONTROL_OFFSET);
>>
>> + /*
>> +* There must be a min 400ns delay between clearing enable and
>> setting
>> +* it. Failing to do this may result in no PWM signal.
>> +*/
>> + ndelay(400);
>> +}
>
> Since it doesn't function as an enable, please call it the trigger bit.
>
>> +
>> +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int
>> chan)
>> +{
>> + unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
>> +
>> /* Set trigger bit and clear smooth bit to apply new settings */
>> value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
>> value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
>> writel(value, kp->base + PWM_CONTROL_OFFSET);
>> +
>> + /* PWMOUT_ENABLE must be held high for at least 400 ns. */
>> + ndelay(400);
>> }
>
> Same here.
>
>>
>> static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> @@ -133,8 +150,14 @@ static int kona_pwmc_config(struct pwm_chip *chip,
>> struct pwm_device *pwm,
>> return -EINVAL;
>> }
>>
>> - /* If the PWM channel is enabled, write the settings to the HW */
>> + /*
>> +* Don't apply settings if disabled. The period and duty cycle are
>> +* always calculated above to ensure the new values are
>> +* validated immediately instead of on enable.
>> +*/
>> if (test_bit(PWMF_ENABLED, >flags)) {
>> + kona_pwmc_prepare_for_settings(kp, chan);
>> +
>> value = readl(kp->base + PRESCALE_OFFSET);
>> value &= ~PRESCALE_MASK(chan);
>> value |= prescale << PRESCALE_SHIFT(chan);
>> @@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
>> struct pwm_device *pwm,
>>
Re: [PATCH v8 5/5] pwm: core: Set enable state properly on failed call to enable
On 15-06-12 02:49 AM, Thierry Reding wrote:
> On Tue, May 26, 2015 at 01:08:20PM -0700, Jonathan Richardson wrote:
>> The pwm_enable function didn't clear the enabled bit if a call to a
>> clients enable function returned an error. The result was that the state
>> of the pwm core was wrong. Clearing the bit when enable returns an error
>> ensures the state is properly set.
>>
>> Tested-by: Jonathan Richardson
>> Reviewed-by: Dmitry Torokhov
>> Signed-off-by: Jonathan Richardson
>> ---
>> drivers/pwm/core.c | 16 +---
>> 1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index 224645f..18f5ac4 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -477,10 +477,20 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
>> */
>> int pwm_enable(struct pwm_device *pwm)
>> {
>> -if (pwm && !test_and_set_bit(PWMF_ENABLED, >flags))
>> -return pwm->chip->ops->enable(pwm->chip, pwm);
>> +int err;
>> +
>> +if (!pwm)
>> +return -EINVAL;
>> +
>> +if (!test_and_set_bit(PWMF_ENABLED, >flags)) {
>> +err = pwm->chip->ops->enable(pwm->chip, pwm);
>> +if (err) {
>> +clear_bit(PWMF_ENABLED, >flags);
>> +return err;
>> +}
>> +}
>
> I think with this new pattern we're now actually going to need a lock to
> make sure pwm->flags doesn't change between the test_and_set_bit() and
> clear_bit() calls.
>
> Thierry
>
Ok. I'll add a lock per pwm_device and re-send the patch.
Jon
--
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 v8 5/5] pwm: core: Set enable state properly on failed call to enable
On 15-06-12 02:49 AM, Thierry Reding wrote:
On Tue, May 26, 2015 at 01:08:20PM -0700, Jonathan Richardson wrote:
The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.
Tested-by: Jonathan Richardson jonat...@broadcom.com
Reviewed-by: Dmitry Torokhov dmitry.torok...@gmail.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/core.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 224645f..18f5ac4 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -477,10 +477,20 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
*/
int pwm_enable(struct pwm_device *pwm)
{
-if (pwm !test_and_set_bit(PWMF_ENABLED, pwm-flags))
-return pwm-chip-ops-enable(pwm-chip, pwm);
+int err;
+
+if (!pwm)
+return -EINVAL;
+
+if (!test_and_set_bit(PWMF_ENABLED, pwm-flags)) {
+err = pwm-chip-ops-enable(pwm-chip, pwm);
+if (err) {
+clear_bit(PWMF_ENABLED, pwm-flags);
+return err;
+}
+}
I think with this new pattern we're now actually going to need a lock to
make sure pwm-flags doesn't change between the test_and_set_bit() and
clear_bit() calls.
Thierry
Ok. I'll add a lock per pwm_device and re-send the patch.
Jon
--
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 v8 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures
On 15-05-30 09:41 AM, Tim Kryger wrote:
On Tue, May 26, 2015 at 1:08 PM, Jonathan Richardson
jonat...@broadcom.com wrote:
The config procedure didn't follow the spec which periodically resulted
in failing to enable the output signal. This happened one in ten or
twenty attempts. Following the spec and adding a 400ns delay in the
appropriate locations resolves this problem.
The disable and polarity procedures now also follow the spec. The old
procedures would result in no change in signal when called.
I think you may want to adjust your commit title and message to more
clearly describe what this change is doing. Perhaps something like:
pwm: kona: Modify settings application sequence
Update the driver so that settings are applied in accordance with the
most recent version of the hardware spec. The revised sequence clears
the trigger bit, waits 400ns, writes settings, sets the trigger bit,
and waits another 400ns. This corrects an issue where occasionally a
requested change was not properly reflected in the PWM output.
Otherwise, this patch looks reasonable so
Reviewed-by: Tim Kryger tim.kry...@gmail.com
Fine with me. Same with two comments below. Will re-spin when I can see
Thierry's modification to use pwmchip_add_with_polarity() instead of
pwmchip_add_inversed().
Thanks.
Reviewed-by: Arun Ramamurthy arunr...@broadcom.com
Reviewed-by: Scott Branden sbran...@broadcom.com
Tested-by: Scott Branden sbran...@broadcom.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/pwm-bcm-kona.c | 47
+++-
1 file changed, 38 insertions(+), 9 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 32b3ec6..c87621f 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -76,19 +76,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct
pwm_chip *_chip)
return container_of(_chip, struct kona_pwmc, chip);
}
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int
chan)
+/*
+ * Clear trigger bit but set smooth bit to maintain old output.
+ */
+static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
+ unsigned int chan)
{
unsigned int value = readl(kp-base + PWM_CONTROL_OFFSET);
- /* Clear trigger bit but set smooth bit to maintain old output */
value |= 1 PWM_CONTROL_SMOOTH_SHIFT(chan);
value = ~(1 PWM_CONTROL_TRIGGER_SHIFT(chan));
writel(value, kp-base + PWM_CONTROL_OFFSET);
+ /*
+* There must be a min 400ns delay between clearing enable and
setting
+* it. Failing to do this may result in no PWM signal.
+*/
+ ndelay(400);
+}
Since it doesn't function as an enable, please call it the trigger bit.
+
+static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int
chan)
+{
+ unsigned int value = readl(kp-base + PWM_CONTROL_OFFSET);
+
/* Set trigger bit and clear smooth bit to apply new settings */
value = ~(1 PWM_CONTROL_SMOOTH_SHIFT(chan));
value |= 1 PWM_CONTROL_TRIGGER_SHIFT(chan);
writel(value, kp-base + PWM_CONTROL_OFFSET);
+
+ /* PWMOUT_ENABLE must be held high for at least 400 ns. */
+ ndelay(400);
}
Same here.
static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -133,8 +150,14 @@ static int kona_pwmc_config(struct pwm_chip *chip,
struct pwm_device *pwm,
return -EINVAL;
}
- /* If the PWM channel is enabled, write the settings to the HW */
+ /*
+* Don't apply settings if disabled. The period and duty cycle are
+* always calculated above to ensure the new values are
+* validated immediately instead of on enable.
+*/
if (test_bit(PWMF_ENABLED, pwm-flags)) {
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp-base + PRESCALE_OFFSET);
value = ~PRESCALE_MASK(chan);
value |= prescale PRESCALE_SHIFT(chan);
@@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
return ret;
}
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp-base + PWM_CONTROL_OFFSET);
if (polarity == PWM_POLARITY_NORMAL)
@@ -175,9 +200,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
kona_pwmc_apply_settings(kp, chan);
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
-
clk_disable_unprepare(kp-clk);
return 0;
@@ -207,13 +229,20 @@ static void kona_pwmc_disable(struct pwm_chip *chip,
struct pwm_device *pwm)
{
struct kona_pwmc *kp = to_kona_pwmc(chip);
unsigned int chan = pwm-hwpwm;
+ unsigned int value
Re: [PATCH v8 4/5] pwm: kona: Add debug info to config function
On 15-05-30 09:42 AM, Tim Kryger wrote:
On Tue, May 26, 2015 at 1:08 PM, Jonathan Richardson
jonat...@broadcom.com wrote:
Adds debugging info to config function where duty cycle and period
are calculated and verified.
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/pwm-bcm-kona.c | 25 +++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index c87621f..0ddf19b 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip,
struct pwm_device *pwm,
dc = div64_u64(val, div);
/* If duty_ns or period_ns are not achievable then return */
- if (pc PERIOD_COUNT_MIN || dc DUTY_CYCLE_HIGH_MIN)
The original code was based on the SPEAr PWM driver which has a non-zero
PWMDCR_MIN_DUTY such that the second condition here can evaluate to true.
This isn't the case for the Kona PWM where DUTY_CYCLE_HIGH_MIN is zero.
+ if (pc PERIOD_COUNT_MIN) {
+ dev_warn(chip-dev,
+ %s: pwm[%d]: period=%d is not achievable,
pc=%lu, prescale=%lu\n,
+ __func__, chan, period_ns, pc, prescale);
return -EINVAL;
+ }
Why not just print the minimum allowable period with the provided clock?
I don't think pc and prescale will be particularly helpful to users.
Also, do we really need to print __func__ here?
+
+ if (dc DUTY_CYCLE_HIGH_MIN) {
+ if (0 != duty_ns) {
+ dev_warn(chip-dev,
+ %s: pwm[%d]: duty cycle=%d is not
achievable, dc=%lu, prescale=%lu\n,
+ __func__, chan, duty_ns, dc,
prescale);
+ }
+ return -EINVAL;
+ }
The above block is unreachable code.
/* If pc and dc are in bounds, the calculation is done */
if (pc = PERIOD_COUNT_MAX dc = DUTY_CYCLE_HIGH_MAX)
break;
/* Otherwise, increase prescale and recalculate pc and dc */
- if (++prescale PRESCALE_MAX)
+ if (++prescale PRESCALE_MAX) {
+ dev_warn(chip-dev,
+ %s: pwm[%d]: Prescale (=%lu) within max
(=%d) for period=%d and duty cycle=%d is not achievable\n,
+ __func__, chan, prescale, PRESCALE_MAX,
+ period_ns, duty_ns);
return -EINVAL;
+ }
}
The user got here because they specified a period larger than the maximum
supported so why not tell them largest value that can be supported instead
of confusing them with prescale and PRESCALE_MAX?
+ dev_dbg(chip-dev, pwm[%d]: period=%lu, duty_high=%lu,
prescale=%lu\n,
+ chan, pc, dc, prescale);
+
This could be more clear. It prints pc but calls it period.
/*
* Don't apply settings if disabled. The period and duty cycle are
* always calculated above to ensure the new values are
--
1.7.9.5
We can defer this for now until I can look into it further. It's not a
priority. I'm more concerned with core changes and kona pwm fix.
Thanks,
Jon
--
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 v8 5/5] pwm: core: Set enable state properly on failed call to enable
The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.
Tested-by: Jonathan Richardson
Reviewed-by: Dmitry Torokhov
Signed-off-by: Jonathan Richardson
---
drivers/pwm/core.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 224645f..18f5ac4 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -477,10 +477,20 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
*/
int pwm_enable(struct pwm_device *pwm)
{
- if (pwm && !test_and_set_bit(PWMF_ENABLED, >flags))
- return pwm->chip->ops->enable(pwm->chip, pwm);
+ int err;
+
+ if (!pwm)
+ return -EINVAL;
+
+ if (!test_and_set_bit(PWMF_ENABLED, >flags)) {
+ err = pwm->chip->ops->enable(pwm->chip, pwm);
+ if (err) {
+ clear_bit(PWMF_ENABLED, >flags);
+ return err;
+ }
+ }
- return pwm ? 0 : -EINVAL;
+ return 0;
}
EXPORT_SYMBOL_GPL(pwm_enable);
--
1.7.9.5
--
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 v8 2/5] drivers: pwm: bcm-kona: Dont set polarity in probe
From: Arun Ramamurthy
Omit setting the polarity to normal during probe and instead use the
new pwmchip_add_inversed function to register a PWM chip with default
polarity of inversed for all channels as this is the actual hardware
default.
Signed-off-by: Arun Ramamurthy
Reviewed-by: Ray Jui
Signed-off-by: Scott Branden
Signed-off-by: Tim Kryger
Signed-off-by: Jonathan Richardson
---
drivers/pwm/pwm-bcm-kona.c |9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 02bc048..32b3ec6 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -266,18 +266,15 @@ static int kona_pwmc_probe(struct platform_device *pdev)
return ret;
}
- /* Set smooth mode, push/pull, and normal polarity for all channels */
- for (chan = 0; chan < kp->chip.npwm; chan++) {
- value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+ /* Set push/pull for all channels */
+ for (chan = 0; chan < kp->chip.npwm; chan++)
value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
- value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan));
- }
writel(value, kp->base + PWM_CONTROL_OFFSET);
clk_disable_unprepare(kp->clk);
- ret = pwmchip_add(>chip);
+ ret = pwmchip_add_inversed(>chip);
if (ret < 0)
dev_err(>dev, "failed to add PWM chip: %d\n", ret);
--
1.7.9.5
--
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 v8 1/5] drivers: pwm: core: Add pwmchip_add_inversed
From: Tim Kryger
Add a new function to register a PWM chip with channels that have their
initial polarity as inversed. This benefits drivers of controllers that
by default operate with inversed polarity by removing the need to modify
the polarity during initialization.
Signed-off-by: Tim Kryger
Signed-off-by: Jonathan Richardson
---
drivers/pwm/core.c | 36
include/linux/pwm.h |6 ++
2 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index ba34c7d..224645f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -222,14 +222,8 @@ void *pwm_get_chip_data(struct pwm_device *pwm)
}
EXPORT_SYMBOL_GPL(pwm_get_chip_data);
-/**
- * pwmchip_add() - register a new PWM chip
- * @chip: the PWM chip to add
- *
- * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
- * will be used.
- */
-int pwmchip_add(struct pwm_chip *chip)
+static int pwmchip_add_with_polarity(struct pwm_chip *chip,
+enum pwm_polarity polarity)
{
struct pwm_device *pwm;
unsigned int i;
@@ -259,6 +253,7 @@ int pwmchip_add(struct pwm_chip *chip)
pwm->chip = chip;
pwm->pwm = chip->base + i;
pwm->hwpwm = i;
+ pwm->polarity = polarity;
radix_tree_insert(_tree, pwm->pwm, pwm);
}
@@ -279,9 +274,34 @@ out:
mutex_unlock(_lock);
return ret;
}
+
+/**
+ * pwmchip_add() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
+ * will be used. The initial polarity for all channels is normal.
+ */
+int pwmchip_add(struct pwm_chip *chip)
+{
+ return pwmchip_add_with_polarity(chip, PWM_POLARITY_NORMAL);
+}
EXPORT_SYMBOL_GPL(pwmchip_add);
/**
+ * pwmchip_add_inversed() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
+ * will be used. The initial polarity for all channels is inversed.
+ */
+int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+ return pwmchip_add_with_polarity(chip, PWM_POLARITY_INVERSED);
+}
+EXPORT_SYMBOL_GPL(pwmchip_add_inversed);
+
+/**
* pwmchip_remove() - remove a PWM chip
* @chip: the PWM chip to remove
*
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e90628c..358547f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -183,6 +183,7 @@ int pwm_set_chip_data(struct pwm_device *pwm, void *data);
void *pwm_get_chip_data(struct pwm_device *pwm);
int pwmchip_add(struct pwm_chip *chip);
+int pwmchip_add_inversed(struct pwm_chip *chip);
int pwmchip_remove(struct pwm_chip *chip);
struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
unsigned int index,
@@ -217,6 +218,11 @@ static inline int pwmchip_add(struct pwm_chip *chip)
return -EINVAL;
}
+static inline int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+ return -EINVAL;
+}
+
static inline int pwmchip_remove(struct pwm_chip *chip)
{
return -EINVAL;
--
1.7.9.5
--
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 v8 0/5] Fix bugs in kona pwm driver and pwm core
This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also fixes a bug in the pwm core where the enabled state was incorrect on failed calls to enable. Also, a new function was added to the pwm core to add pwm chips with inversed polarity for chips that have a different default polarity than the core. The prevents incorrect polarity being reported. Debug info has been added to help debug configuring duty cycle and period. These changes are required for the Kona PWM driver to work on Cygnus. The same PWM IP is being used. Changes from v7: - Polarity changes take effect immediately instead of being deferred until enable is called. Changes from v6: - Move new debugging info for duty cycle and period in config function into its own commit. - Add kona_pwmc_prepare_for_settings() function to remove duplicated code. Changes from v5: - Address Dmitry's comment on code cleanup of pwm_enable() in pwm core. - Including all patches from different contributors in this patchset. Some were left out in v4. Changes from v4: - Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity. - Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework. Arun Ramamurthy (1): drivers: pwm: bcm-kona: Dont set polarity in probe Jonathan Richardson (3): pwm: kona: Fix incorrect config, disable, and polarity procedures pwm: kona: Add debug info to config function pwm: core: Set enable state properly on failed call to enable Tim Kryger (1): drivers: pwm: core: Add pwmchip_add_inversed drivers/pwm/core.c | 52 ++-- drivers/pwm/pwm-bcm-kona.c | 81 ++-- include/linux/pwm.h|6 3 files changed, 111 insertions(+), 28 deletions(-) -- 1.7.9.5 -- 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 v8 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures
The config procedure didn't follow the spec which periodically resulted
in failing to enable the output signal. This happened one in ten or
twenty attempts. Following the spec and adding a 400ns delay in the
appropriate locations resolves this problem.
The disable and polarity procedures now also follow the spec. The old
procedures would result in no change in signal when called.
Reviewed-by: Arun Ramamurthy
Reviewed-by: Scott Branden
Tested-by: Scott Branden
Signed-off-by: Jonathan Richardson
---
drivers/pwm/pwm-bcm-kona.c | 47 +++-
1 file changed, 38 insertions(+), 9 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 32b3ec6..c87621f 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -76,19 +76,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct
pwm_chip *_chip)
return container_of(_chip, struct kona_pwmc, chip);
}
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+/*
+ * Clear trigger bit but set smooth bit to maintain old output.
+ */
+static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
+ unsigned int chan)
{
unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
- /* Clear trigger bit but set smooth bit to maintain old output */
value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
writel(value, kp->base + PWM_CONTROL_OFFSET);
+ /*
+* There must be a min 400ns delay between clearing enable and setting
+* it. Failing to do this may result in no PWM signal.
+*/
+ ndelay(400);
+}
+
+static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+{
+ unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
/* Set trigger bit and clear smooth bit to apply new settings */
value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
writel(value, kp->base + PWM_CONTROL_OFFSET);
+
+ /* PWMOUT_ENABLE must be held high for at least 400 ns. */
+ ndelay(400);
}
static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -133,8 +150,14 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct
pwm_device *pwm,
return -EINVAL;
}
- /* If the PWM channel is enabled, write the settings to the HW */
+ /*
+* Don't apply settings if disabled. The period and duty cycle are
+* always calculated above to ensure the new values are
+* validated immediately instead of on enable.
+*/
if (test_bit(PWMF_ENABLED, >flags)) {
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp->base + PRESCALE_OFFSET);
value &= ~PRESCALE_MASK(chan);
value |= prescale << PRESCALE_SHIFT(chan);
@@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
return ret;
}
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp->base + PWM_CONTROL_OFFSET);
if (polarity == PWM_POLARITY_NORMAL)
@@ -175,9 +200,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
kona_pwmc_apply_settings(kp, chan);
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
-
clk_disable_unprepare(kp->clk);
return 0;
@@ -207,13 +229,20 @@ static void kona_pwmc_disable(struct pwm_chip *chip,
struct pwm_device *pwm)
{
struct kona_pwmc *kp = to_kona_pwmc(chip);
unsigned int chan = pwm->hwpwm;
+ unsigned int value;
+
+ kona_pwmc_prepare_for_settings(kp, chan);
/* Simulate a disable by configuring for zero duty */
writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
- kona_pwmc_apply_settings(kp, chan);
+ writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
+ /* Set prescale to 0 for this channel */
+ value = readl(kp->base + PRESCALE_OFFSET);
+ value &= ~PRESCALE_MASK(chan);
+ writel(value, kp->base + PRESCALE_OFFSET);
+
+ kona_pwmc_apply_settings(kp, chan);
clk_disable_unprepare(kp->clk);
}
--
1.7.9.5
--
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 v8 4/5] pwm: kona: Add debug info to config function
Adds debugging info to config function where duty cycle and period
are calculated and verified.
Signed-off-by: Jonathan Richardson
---
drivers/pwm/pwm-bcm-kona.c | 25 +++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index c87621f..0ddf19b 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct
pwm_device *pwm,
dc = div64_u64(val, div);
/* If duty_ns or period_ns are not achievable then return */
- if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
+ if (pc < PERIOD_COUNT_MIN) {
+ dev_warn(chip->dev,
+ "%s: pwm[%d]: period=%d is not achievable,
pc=%lu, prescale=%lu\n",
+ __func__, chan, period_ns, pc, prescale);
return -EINVAL;
+ }
+
+ if (dc < DUTY_CYCLE_HIGH_MIN) {
+ if (0 != duty_ns) {
+ dev_warn(chip->dev,
+ "%s: pwm[%d]: duty cycle=%d is not
achievable, dc=%lu, prescale=%lu\n",
+ __func__, chan, duty_ns, dc, prescale);
+ }
+ return -EINVAL;
+ }
/* If pc and dc are in bounds, the calculation is done */
if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
break;
/* Otherwise, increase prescale and recalculate pc and dc */
- if (++prescale > PRESCALE_MAX)
+ if (++prescale > PRESCALE_MAX) {
+ dev_warn(chip->dev,
+ "%s: pwm[%d]: Prescale (=%lu) within max (=%d)
for period=%d and duty cycle=%d is not achievable\n",
+ __func__, chan, prescale, PRESCALE_MAX,
+ period_ns, duty_ns);
return -EINVAL;
+ }
}
+ dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
+ chan, pc, dc, prescale);
+
/*
* Don't apply settings if disabled. The period and duty cycle are
* always calculated above to ensure the new values are
--
1.7.9.5
--
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 v8 0/5] Fix bugs in kona pwm driver and pwm core
This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also fixes a bug in the pwm core where the enabled state was incorrect on failed calls to enable. Also, a new function was added to the pwm core to add pwm chips with inversed polarity for chips that have a different default polarity than the core. The prevents incorrect polarity being reported. Debug info has been added to help debug configuring duty cycle and period. These changes are required for the Kona PWM driver to work on Cygnus. The same PWM IP is being used. Changes from v7: - Polarity changes take effect immediately instead of being deferred until enable is called. Changes from v6: - Move new debugging info for duty cycle and period in config function into its own commit. - Add kona_pwmc_prepare_for_settings() function to remove duplicated code. Changes from v5: - Address Dmitry's comment on code cleanup of pwm_enable() in pwm core. - Including all patches from different contributors in this patchset. Some were left out in v4. Changes from v4: - Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity. - Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework. Arun Ramamurthy (1): drivers: pwm: bcm-kona: Dont set polarity in probe Jonathan Richardson (3): pwm: kona: Fix incorrect config, disable, and polarity procedures pwm: kona: Add debug info to config function pwm: core: Set enable state properly on failed call to enable Tim Kryger (1): drivers: pwm: core: Add pwmchip_add_inversed drivers/pwm/core.c | 52 ++-- drivers/pwm/pwm-bcm-kona.c | 81 ++-- include/linux/pwm.h|6 3 files changed, 111 insertions(+), 28 deletions(-) -- 1.7.9.5 -- 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 v8 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures
The config procedure didn't follow the spec which periodically resulted
in failing to enable the output signal. This happened one in ten or
twenty attempts. Following the spec and adding a 400ns delay in the
appropriate locations resolves this problem.
The disable and polarity procedures now also follow the spec. The old
procedures would result in no change in signal when called.
Reviewed-by: Arun Ramamurthy arunr...@broadcom.com
Reviewed-by: Scott Branden sbran...@broadcom.com
Tested-by: Scott Branden sbran...@broadcom.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/pwm-bcm-kona.c | 47 +++-
1 file changed, 38 insertions(+), 9 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 32b3ec6..c87621f 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -76,19 +76,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct
pwm_chip *_chip)
return container_of(_chip, struct kona_pwmc, chip);
}
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+/*
+ * Clear trigger bit but set smooth bit to maintain old output.
+ */
+static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
+ unsigned int chan)
{
unsigned int value = readl(kp-base + PWM_CONTROL_OFFSET);
- /* Clear trigger bit but set smooth bit to maintain old output */
value |= 1 PWM_CONTROL_SMOOTH_SHIFT(chan);
value = ~(1 PWM_CONTROL_TRIGGER_SHIFT(chan));
writel(value, kp-base + PWM_CONTROL_OFFSET);
+ /*
+* There must be a min 400ns delay between clearing enable and setting
+* it. Failing to do this may result in no PWM signal.
+*/
+ ndelay(400);
+}
+
+static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+{
+ unsigned int value = readl(kp-base + PWM_CONTROL_OFFSET);
+
/* Set trigger bit and clear smooth bit to apply new settings */
value = ~(1 PWM_CONTROL_SMOOTH_SHIFT(chan));
value |= 1 PWM_CONTROL_TRIGGER_SHIFT(chan);
writel(value, kp-base + PWM_CONTROL_OFFSET);
+
+ /* PWMOUT_ENABLE must be held high for at least 400 ns. */
+ ndelay(400);
}
static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -133,8 +150,14 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct
pwm_device *pwm,
return -EINVAL;
}
- /* If the PWM channel is enabled, write the settings to the HW */
+ /*
+* Don't apply settings if disabled. The period and duty cycle are
+* always calculated above to ensure the new values are
+* validated immediately instead of on enable.
+*/
if (test_bit(PWMF_ENABLED, pwm-flags)) {
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp-base + PRESCALE_OFFSET);
value = ~PRESCALE_MASK(chan);
value |= prescale PRESCALE_SHIFT(chan);
@@ -164,6 +187,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
return ret;
}
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp-base + PWM_CONTROL_OFFSET);
if (polarity == PWM_POLARITY_NORMAL)
@@ -175,9 +200,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
kona_pwmc_apply_settings(kp, chan);
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
-
clk_disable_unprepare(kp-clk);
return 0;
@@ -207,13 +229,20 @@ static void kona_pwmc_disable(struct pwm_chip *chip,
struct pwm_device *pwm)
{
struct kona_pwmc *kp = to_kona_pwmc(chip);
unsigned int chan = pwm-hwpwm;
+ unsigned int value;
+
+ kona_pwmc_prepare_for_settings(kp, chan);
/* Simulate a disable by configuring for zero duty */
writel(0, kp-base + DUTY_CYCLE_HIGH_OFFSET(chan));
- kona_pwmc_apply_settings(kp, chan);
+ writel(0, kp-base + PERIOD_COUNT_OFFSET(chan));
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
+ /* Set prescale to 0 for this channel */
+ value = readl(kp-base + PRESCALE_OFFSET);
+ value = ~PRESCALE_MASK(chan);
+ writel(value, kp-base + PRESCALE_OFFSET);
+
+ kona_pwmc_apply_settings(kp, chan);
clk_disable_unprepare(kp-clk);
}
--
1.7.9.5
--
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 v8 1/5] drivers: pwm: core: Add pwmchip_add_inversed
From: Tim Kryger tim.kry...@gmail.com
Add a new function to register a PWM chip with channels that have their
initial polarity as inversed. This benefits drivers of controllers that
by default operate with inversed polarity by removing the need to modify
the polarity during initialization.
Signed-off-by: Tim Kryger tim.kry...@gmail.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/core.c | 36
include/linux/pwm.h |6 ++
2 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index ba34c7d..224645f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -222,14 +222,8 @@ void *pwm_get_chip_data(struct pwm_device *pwm)
}
EXPORT_SYMBOL_GPL(pwm_get_chip_data);
-/**
- * pwmchip_add() - register a new PWM chip
- * @chip: the PWM chip to add
- *
- * Register a new PWM chip. If chip-base 0 then a dynamically assigned base
- * will be used.
- */
-int pwmchip_add(struct pwm_chip *chip)
+static int pwmchip_add_with_polarity(struct pwm_chip *chip,
+enum pwm_polarity polarity)
{
struct pwm_device *pwm;
unsigned int i;
@@ -259,6 +253,7 @@ int pwmchip_add(struct pwm_chip *chip)
pwm-chip = chip;
pwm-pwm = chip-base + i;
pwm-hwpwm = i;
+ pwm-polarity = polarity;
radix_tree_insert(pwm_tree, pwm-pwm, pwm);
}
@@ -279,9 +274,34 @@ out:
mutex_unlock(pwm_lock);
return ret;
}
+
+/**
+ * pwmchip_add() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip-base 0 then a dynamically assigned base
+ * will be used. The initial polarity for all channels is normal.
+ */
+int pwmchip_add(struct pwm_chip *chip)
+{
+ return pwmchip_add_with_polarity(chip, PWM_POLARITY_NORMAL);
+}
EXPORT_SYMBOL_GPL(pwmchip_add);
/**
+ * pwmchip_add_inversed() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip-base 0 then a dynamically assigned base
+ * will be used. The initial polarity for all channels is inversed.
+ */
+int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+ return pwmchip_add_with_polarity(chip, PWM_POLARITY_INVERSED);
+}
+EXPORT_SYMBOL_GPL(pwmchip_add_inversed);
+
+/**
* pwmchip_remove() - remove a PWM chip
* @chip: the PWM chip to remove
*
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e90628c..358547f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -183,6 +183,7 @@ int pwm_set_chip_data(struct pwm_device *pwm, void *data);
void *pwm_get_chip_data(struct pwm_device *pwm);
int pwmchip_add(struct pwm_chip *chip);
+int pwmchip_add_inversed(struct pwm_chip *chip);
int pwmchip_remove(struct pwm_chip *chip);
struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
unsigned int index,
@@ -217,6 +218,11 @@ static inline int pwmchip_add(struct pwm_chip *chip)
return -EINVAL;
}
+static inline int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+ return -EINVAL;
+}
+
static inline int pwmchip_remove(struct pwm_chip *chip)
{
return -EINVAL;
--
1.7.9.5
--
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 v8 5/5] pwm: core: Set enable state properly on failed call to enable
The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.
Tested-by: Jonathan Richardson jonat...@broadcom.com
Reviewed-by: Dmitry Torokhov dmitry.torok...@gmail.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/core.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 224645f..18f5ac4 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -477,10 +477,20 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
*/
int pwm_enable(struct pwm_device *pwm)
{
- if (pwm !test_and_set_bit(PWMF_ENABLED, pwm-flags))
- return pwm-chip-ops-enable(pwm-chip, pwm);
+ int err;
+
+ if (!pwm)
+ return -EINVAL;
+
+ if (!test_and_set_bit(PWMF_ENABLED, pwm-flags)) {
+ err = pwm-chip-ops-enable(pwm-chip, pwm);
+ if (err) {
+ clear_bit(PWMF_ENABLED, pwm-flags);
+ return err;
+ }
+ }
- return pwm ? 0 : -EINVAL;
+ return 0;
}
EXPORT_SYMBOL_GPL(pwm_enable);
--
1.7.9.5
--
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 v8 2/5] drivers: pwm: bcm-kona: Dont set polarity in probe
From: Arun Ramamurthy arunr...@broadcom.com
Omit setting the polarity to normal during probe and instead use the
new pwmchip_add_inversed function to register a PWM chip with default
polarity of inversed for all channels as this is the actual hardware
default.
Signed-off-by: Arun Ramamurthy arunr...@broadcom.com
Reviewed-by: Ray Jui r...@broadcom.com
Signed-off-by: Scott Branden sbran...@broadcom.com
Signed-off-by: Tim Kryger tim.kry...@gmail.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/pwm-bcm-kona.c |9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 02bc048..32b3ec6 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -266,18 +266,15 @@ static int kona_pwmc_probe(struct platform_device *pdev)
return ret;
}
- /* Set smooth mode, push/pull, and normal polarity for all channels */
- for (chan = 0; chan kp-chip.npwm; chan++) {
- value |= (1 PWM_CONTROL_SMOOTH_SHIFT(chan));
+ /* Set push/pull for all channels */
+ for (chan = 0; chan kp-chip.npwm; chan++)
value |= (1 PWM_CONTROL_TYPE_SHIFT(chan));
- value |= (1 PWM_CONTROL_POLARITY_SHIFT(chan));
- }
writel(value, kp-base + PWM_CONTROL_OFFSET);
clk_disable_unprepare(kp-clk);
- ret = pwmchip_add(kp-chip);
+ ret = pwmchip_add_inversed(kp-chip);
if (ret 0)
dev_err(pdev-dev, failed to add PWM chip: %d\n, ret);
--
1.7.9.5
--
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 v8 4/5] pwm: kona: Add debug info to config function
Adds debugging info to config function where duty cycle and period
are calculated and verified.
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/pwm-bcm-kona.c | 25 +++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index c87621f..0ddf19b 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct
pwm_device *pwm,
dc = div64_u64(val, div);
/* If duty_ns or period_ns are not achievable then return */
- if (pc PERIOD_COUNT_MIN || dc DUTY_CYCLE_HIGH_MIN)
+ if (pc PERIOD_COUNT_MIN) {
+ dev_warn(chip-dev,
+ %s: pwm[%d]: period=%d is not achievable,
pc=%lu, prescale=%lu\n,
+ __func__, chan, period_ns, pc, prescale);
return -EINVAL;
+ }
+
+ if (dc DUTY_CYCLE_HIGH_MIN) {
+ if (0 != duty_ns) {
+ dev_warn(chip-dev,
+ %s: pwm[%d]: duty cycle=%d is not
achievable, dc=%lu, prescale=%lu\n,
+ __func__, chan, duty_ns, dc, prescale);
+ }
+ return -EINVAL;
+ }
/* If pc and dc are in bounds, the calculation is done */
if (pc = PERIOD_COUNT_MAX dc = DUTY_CYCLE_HIGH_MAX)
break;
/* Otherwise, increase prescale and recalculate pc and dc */
- if (++prescale PRESCALE_MAX)
+ if (++prescale PRESCALE_MAX) {
+ dev_warn(chip-dev,
+ %s: pwm[%d]: Prescale (=%lu) within max (=%d)
for period=%d and duty cycle=%d is not achievable\n,
+ __func__, chan, prescale, PRESCALE_MAX,
+ period_ns, duty_ns);
return -EINVAL;
+ }
}
+ dev_dbg(chip-dev, pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n,
+ chan, pc, dc, prescale);
+
/*
* Don't apply settings if disabled. The period and duty cycle are
* always calculated above to ensure the new values are
--
1.7.9.5
--
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 v7 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures
On 15-05-17 09:53 PM, Tim Kryger wrote:
> On Tue, May 12, 2015 at 4:28 PM, Jonathan Richardson
> wrote:
>
>> The polarity procedure no longer applies the settings to change the
>> output signal because it can't be called when the pwm is enabled anyway.
>> The polarity is only updated in the control register. The correct
>> polarity will be applied on enable. The old method of applying changes
>> would result in no signal when the polarity was changed. The new
>> apply_settings function would fix this problem but it isn't required
>> anyway.
>
> Thanks for incorporating some of my suggestions in your latest version.
>
> I'm still concerned about delaying when polarity changes take effect.
>
> Since backlight is a common use of PWM, consider the following situation.
>
> backlight {
> compatible = "pwm-backlight";
> pwms = < 0 500 PWM_POLARITY_NORMAL>;
> brightness-levels = <0 4 8 16 32 64 128 255>;
> default-brightness-level = <0>;
> };
>
> The Kona PWM hardware starts in inversed mode so it will drive output high
> once its clock is enabled during the probe.
>
> Polarity is not adjusted during probe so it stays high and it registers with
> the PWM core using the new pwmchip_add_inversed() function.
>
> Next, the pwm-backlight driver probe executes and it calls devm_pwm_get()
> which then calls pwm_set_period() and most importantly pwm_set_polarity().
>
> The output would change to constant low at this point in the original driver
> but with your proposed change it will remain high.
>
> The driver sets bl->props.brightness and calls backlight_update_status() but,
> since in this case the default brightness is zero, it assumes it doesn't need
> to enable the PWM.
>
> The backlight driver probe then returns and the PWM output is incorrect.
Thanks for the detailed use case. You are right - the problem does
occur. I assumed if the pwm signal was being changed at all that it
should first be enabled. Since the bl driver can't know what 0
brightness means with different polarities, shouldn't it always enable
the pwm first, config 0 period, then disable (when brightness is 0)?
Then the polarity would have been updated properly also. 0 can mean full
brightness or off depending on polarity.
It seems odd that changing the polarity should affect the output signal
when the pwm is disabled. If using sysfs and you change the polarity,
you can't defer the signal change to when it's enabled.
If this is correct - polarity changes affect the output signal
immediately, then I can change our driver. Could you confirm first this
is what we want? This would cause polarity changes to affect all devices
immediately which is what I thought enable was for. The pwm core wanting
the pwm disabled to change the polarity implied to me that it shouldn't
cause a change in the signal until it was enabled.
Thanks.
--
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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
On 15-05-20 11:33 PM, Richard Cochran wrote: > On Wed, May 20, 2015 at 04:38:02PM -0700, Jonathan Richardson wrote: >> >> Richard, this design isn't going to work. We need to have both kernel >> and user space consumers. > > But you did not implement even a single kernel consumer. > >> We don't want all GPIO's in a common timestamp >> buffer either, as it presents problems I mentioned previously. Currently >> the network input is a gpio. > > That won't work. Consider what happens when the MAC drops a packet. > >> After some discussion here I think we'll >> have to keep this driver out of the kernel for now. > > Fine with me. We'll keep this discussion and try to figure out the best interface keeping in mind what you've said as we move further along with it. Not a wasted discussion. Requirements of this need to be better defined and thought out also. Thanks, Jon > > Thanks, > Richard > -- 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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
On 15-05-20 11:33 PM, Richard Cochran wrote: On Wed, May 20, 2015 at 04:38:02PM -0700, Jonathan Richardson wrote: Richard, this design isn't going to work. We need to have both kernel and user space consumers. But you did not implement even a single kernel consumer. We don't want all GPIO's in a common timestamp buffer either, as it presents problems I mentioned previously. Currently the network input is a gpio. That won't work. Consider what happens when the MAC drops a packet. After some discussion here I think we'll have to keep this driver out of the kernel for now. Fine with me. We'll keep this discussion and try to figure out the best interface keeping in mind what you've said as we move further along with it. Not a wasted discussion. Requirements of this need to be better defined and thought out also. Thanks, Jon Thanks, Richard -- 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 v7 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures
On 15-05-17 09:53 PM, Tim Kryger wrote:
On Tue, May 12, 2015 at 4:28 PM, Jonathan Richardson
jonat...@broadcom.com wrote:
The polarity procedure no longer applies the settings to change the
output signal because it can't be called when the pwm is enabled anyway.
The polarity is only updated in the control register. The correct
polarity will be applied on enable. The old method of applying changes
would result in no signal when the polarity was changed. The new
apply_settings function would fix this problem but it isn't required
anyway.
Thanks for incorporating some of my suggestions in your latest version.
I'm still concerned about delaying when polarity changes take effect.
Since backlight is a common use of PWM, consider the following situation.
backlight {
compatible = pwm-backlight;
pwms = pwm 0 500 PWM_POLARITY_NORMAL;
brightness-levels = 0 4 8 16 32 64 128 255;
default-brightness-level = 0;
};
The Kona PWM hardware starts in inversed mode so it will drive output high
once its clock is enabled during the probe.
Polarity is not adjusted during probe so it stays high and it registers with
the PWM core using the new pwmchip_add_inversed() function.
Next, the pwm-backlight driver probe executes and it calls devm_pwm_get()
which then calls pwm_set_period() and most importantly pwm_set_polarity().
The output would change to constant low at this point in the original driver
but with your proposed change it will remain high.
The driver sets bl-props.brightness and calls backlight_update_status() but,
since in this case the default brightness is zero, it assumes it doesn't need
to enable the PWM.
The backlight driver probe then returns and the PWM output is incorrect.
Thanks for the detailed use case. You are right - the problem does
occur. I assumed if the pwm signal was being changed at all that it
should first be enabled. Since the bl driver can't know what 0
brightness means with different polarities, shouldn't it always enable
the pwm first, config 0 period, then disable (when brightness is 0)?
Then the polarity would have been updated properly also. 0 can mean full
brightness or off depending on polarity.
It seems odd that changing the polarity should affect the output signal
when the pwm is disabled. If using sysfs and you change the polarity,
you can't defer the signal change to when it's enabled.
If this is correct - polarity changes affect the output signal
immediately, then I can change our driver. Could you confirm first this
is what we want? This would cause polarity changes to affect all devices
immediately which is what I thought enable was for. The pwm core wanting
the pwm disabled to change the polarity implied to me that it shouldn't
cause a change in the signal until it was enabled.
Thanks.
--
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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
On 15-05-14 04:30 AM, Richard Cochran wrote:
> On Wed, May 13, 2015 at 04:25:49PM -0700, Jonathan Richardson wrote:
>> On 15-05-13 01:27 PM, Richard Cochran wrote:
>>> If the ISR is delivering batches of time stamps, then the interrupt
>>> rate aught to be the highest rate that the system can support.
>>
>> It to nanosecond precision so it can be ridiculously quick.
>
> Your interrupt rate directly controls the worst case delay between the
> time the event occurs and the time the application (or stack) obtains
> the time stamp. You want this delay to be as short as possible, but
> you cannot afford to bog down the entire operating system. I would
> suggest a fixed value of 1000 Hz (with perhaps a debugfs knob).
>
> What a poor hardware design. Oh well.
>
>> Having separate FIFO's allows process A to only retrieve channel 1
>> timestamps.
>
> Having lots of different processes reading time stamps directly, and
> trying to match them up with various HW events after the fact, is not
> way to go. Instead, kernel time stamp consumers should pair the time
> stamps with the associated events and then provide the time
> information conveniently over the existing APIs. (See also my last
> comment.)
>
>> Our original non PTP driver ioctl (DTE_GET_TIMESTAMP) solved that
>> problem because we could specify a channel. We're now trying to adapt it
>> to PTP so we don't have to write a new "DTE" user and kernel side API.
>
> For mainline, we want the best interface possible. Sometimes that
> means that programs based on out-of-tree interfaces will need to be
> changed.
>
>>> Ideally, there will be kernel consumers for most of the channels, and
>>> they will forward the time stamps in a way appropriate to their
>>> subsystem. For example, network devices will use so_timestamping.
>>> Any "leftover" channels can go through the generic PTP interface.
>>
>> I'll look more into so_timestamping to see how that's used but we wanted
>> one generic interface to get timestamps from channels because anything
>> can be hooked up to a channel.
>
> I definitely do *not* want one generic interface. The task of
> matching time stamps with hardware events belongs to the kernel. For
> random GPIOs, the existing PTP ioctl is plenty good enough, but for
> other devices (network, audio) more work is needed.
Richard, this design isn't going to work. We need to have both kernel
and user space consumers. We don't want all GPIO's in a common timestamp
buffer either, as it presents problems I mentioned previously. Currently
the network input is a gpio. After some discussion here I think we'll
have to keep this driver out of the kernel for now.
Thanks.
>
> One huge lacuna in your patch series is the code that associates the
> time stamps with events. How is this supposed to work?
>
> So far you have:
>
> +enum dte_client {
> + DTE_CLIENT_MIN = 0,
> + DTE_CLIENT_I2S0_BITCLOCK = 0,
> + DTE_CLIENT_I2S1_BITCLOCK,
> + DTE_CLIENT_I2S2_BITCLOCK,
> + DTE_CLIENT_I2S0_WORDCLOCK,
> + DTE_CLIENT_I2S1_WORDCLOCK,
> + DTE_CLIENT_I2S2_WORDCLOCK,
> + DTE_CLIENT_LCD_CLFP,
> + DTE_CLIENT_LCD_CLLP,
> + DTE_CLIENT_GPIO14,
> + DTE_CLIENT_GPIO15,
> + DTE_CLIENT_GPIO22,
> + DTE_CLIENT_GPIO23,
> + DTE_CLIENT_MAX,
> +};
>
> Network devices are not present at all. No idea why LCD signals are
> included. For the i2s, this appears to stamp the audio clock. But
> which audio sample has been stamped? Or do you only care about
> frequency and not phase?
>
> For the next round, please include John Stulz, Thomas Gleixner,
> netdev, and the appropriate audio list on CC.
>
> Thanks,
> Richard
>
--
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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
On 15-05-14 04:30 AM, Richard Cochran wrote:
On Wed, May 13, 2015 at 04:25:49PM -0700, Jonathan Richardson wrote:
On 15-05-13 01:27 PM, Richard Cochran wrote:
If the ISR is delivering batches of time stamps, then the interrupt
rate aught to be the highest rate that the system can support.
It to nanosecond precision so it can be ridiculously quick.
Your interrupt rate directly controls the worst case delay between the
time the event occurs and the time the application (or stack) obtains
the time stamp. You want this delay to be as short as possible, but
you cannot afford to bog down the entire operating system. I would
suggest a fixed value of 1000 Hz (with perhaps a debugfs knob).
What a poor hardware design. Oh well.
Having separate FIFO's allows process A to only retrieve channel 1
timestamps.
Having lots of different processes reading time stamps directly, and
trying to match them up with various HW events after the fact, is not
way to go. Instead, kernel time stamp consumers should pair the time
stamps with the associated events and then provide the time
information conveniently over the existing APIs. (See also my last
comment.)
Our original non PTP driver ioctl (DTE_GET_TIMESTAMP) solved that
problem because we could specify a channel. We're now trying to adapt it
to PTP so we don't have to write a new DTE user and kernel side API.
For mainline, we want the best interface possible. Sometimes that
means that programs based on out-of-tree interfaces will need to be
changed.
Ideally, there will be kernel consumers for most of the channels, and
they will forward the time stamps in a way appropriate to their
subsystem. For example, network devices will use so_timestamping.
Any leftover channels can go through the generic PTP interface.
I'll look more into so_timestamping to see how that's used but we wanted
one generic interface to get timestamps from channels because anything
can be hooked up to a channel.
I definitely do *not* want one generic interface. The task of
matching time stamps with hardware events belongs to the kernel. For
random GPIOs, the existing PTP ioctl is plenty good enough, but for
other devices (network, audio) more work is needed.
Richard, this design isn't going to work. We need to have both kernel
and user space consumers. We don't want all GPIO's in a common timestamp
buffer either, as it presents problems I mentioned previously. Currently
the network input is a gpio. After some discussion here I think we'll
have to keep this driver out of the kernel for now.
Thanks.
One huge lacuna in your patch series is the code that associates the
time stamps with events. How is this supposed to work?
So far you have:
+enum dte_client {
+ DTE_CLIENT_MIN = 0,
+ DTE_CLIENT_I2S0_BITCLOCK = 0,
+ DTE_CLIENT_I2S1_BITCLOCK,
+ DTE_CLIENT_I2S2_BITCLOCK,
+ DTE_CLIENT_I2S0_WORDCLOCK,
+ DTE_CLIENT_I2S1_WORDCLOCK,
+ DTE_CLIENT_I2S2_WORDCLOCK,
+ DTE_CLIENT_LCD_CLFP,
+ DTE_CLIENT_LCD_CLLP,
+ DTE_CLIENT_GPIO14,
+ DTE_CLIENT_GPIO15,
+ DTE_CLIENT_GPIO22,
+ DTE_CLIENT_GPIO23,
+ DTE_CLIENT_MAX,
+};
Network devices are not present at all. No idea why LCD signals are
included. For the i2s, this appears to stamp the audio clock. But
which audio sample has been stamped? Or do you only care about
frequency and not phase?
For the next round, please include John Stulz, Thomas Gleixner,
netdev, and the appropriate audio list on CC.
Thanks,
Richard
--
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/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
On 15-05-14 12:08 PM, Mark Brown wrote: > On Thu, May 14, 2015 at 11:19:01AM -0700, Scott Branden wrote: >> On 15-05-14 03:31 AM, Mark Brown wrote: > >>> Chip vendors often say this sort of thing and then get surprised by what >>> their users choose to do, and even if it only ever gets used with flash >>> all it would take is some new flash command which can use full duplex >>> for something. Please write the code so it at least tries to handle >>> full duplex operation, if you can't test it fully that's not the end of >>> the world. It doesn't look like it should be particularly difficult. > >> Yes, there is always room for improvements in code. In this case - it >> really is not worth our time to add code we can't test. We try to deliver >> code that we can test and actually works. Yes, if anyone needs to use the > > While I try to not apply code that has obvious problems with silent data > corruption in it which is what we have just now. > >> mspi for full duplex operation code can be added in the future - it is >> software. This block has gone through many generations of our SoCs and has >> only been used for this purpose - the bootROM boots from this SPI only. It >> is dedicated for this purpose. > > All it takes is one hardware engineer who sees a SPI controller and a > GPIO they can use for chip select; I wouldn't be so sure that nobody > ever fixed this up locally (or happened to use a device that only needed > single duplex). > Hi Mark. Would it help if we just set the flags to half duplex using SPI_MASTER_HALF_DUPLEX when we register the master? -- 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/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
On 15-05-14 11:36 AM, Scott Branden wrote: > On 15-05-14 11:28 AM, Florian Fainelli wrote: >> On 14/05/15 11:19, Scott Branden wrote: >>> Hi Mark, >>> >>> On 15-05-14 03:31 AM, Mark Brown wrote: On Wed, May 13, 2015 at 05:19:06PM -0700, Scott Branden wrote: > The purpose of this mspi interface is to connect to NOR flash. There > are > other SPI interfaces on the devices used to connect to other SPI > devies. We > don't have any need to support full duplex slaves on this port (NOR > have any > hardware wired this way - pun realized after typing this). Chip vendors often say this sort of thing and then get surprised by what their users choose to do, and even if it only ever gets used with flash all it would take is some new flash command which can use full duplex for something. Please write the code so it at least tries to handle full duplex operation, if you can't test it fully that's not the end of the world. It doesn't look like it should be particularly difficult. >>> Yes, there is always room for improvements in code. In this case - it >>> really is not worth our time to add code we can't test. We try to >>> deliver code that we can test and actually works. Yes, if anyone needs >>> to use the mspi for full duplex operation code can be added in the >>> future - it is software. This block has gone through many generations >>> of our SoCs and has only been used for this purpose - the bootROM boots >>> from this SPI only. It is dedicated for this purpose. >>> >>> Also, there are other SPI blocks on the SoC that are used for connect >>> other SPI devices. These SPI blocks are properly designed for >>> full-duplex operation. This mspi block is designed for connecting to >>> NOR flash. >> >> This is an implementation detail and architectural choice that is >> specific to the Cygnus SoCs where a single MSPI is present, right? >> >> This same MSPI block is instantiated multiple times in BCM7xxx chips to >> interface to general purpose SPI slaves, ranging from Ethernet switches >> to bluetooth chips etc... those will definitively want full-duplex to be >> working. >> >> I would greatly appreciate if some thought to supporting full duplex >> transfers was given in this driver drop. If you cannot test that, then >> maybe just flag the driver has been half-duplex only for now... >> >> Thank you. >> > Hi Florian, > Yes, we can flag that - but we have no understanding of how the block > operates in this mode. It would be great if your team could add the > necessary full duplex support in a patch enhancement to the driver? > Going blind on this one isn't the way to go. If I had a real full duplex slave I could experiment with it to get it working. But with the docs I have on this and considering how finicky the block is I think we're wasting our time. Jon -- 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/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
On 15-05-14 11:36 AM, Scott Branden wrote: On 15-05-14 11:28 AM, Florian Fainelli wrote: On 14/05/15 11:19, Scott Branden wrote: Hi Mark, On 15-05-14 03:31 AM, Mark Brown wrote: On Wed, May 13, 2015 at 05:19:06PM -0700, Scott Branden wrote: The purpose of this mspi interface is to connect to NOR flash. There are other SPI interfaces on the devices used to connect to other SPI devies. We don't have any need to support full duplex slaves on this port (NOR have any hardware wired this way - pun realized after typing this). Chip vendors often say this sort of thing and then get surprised by what their users choose to do, and even if it only ever gets used with flash all it would take is some new flash command which can use full duplex for something. Please write the code so it at least tries to handle full duplex operation, if you can't test it fully that's not the end of the world. It doesn't look like it should be particularly difficult. Yes, there is always room for improvements in code. In this case - it really is not worth our time to add code we can't test. We try to deliver code that we can test and actually works. Yes, if anyone needs to use the mspi for full duplex operation code can be added in the future - it is software. This block has gone through many generations of our SoCs and has only been used for this purpose - the bootROM boots from this SPI only. It is dedicated for this purpose. Also, there are other SPI blocks on the SoC that are used for connect other SPI devices. These SPI blocks are properly designed for full-duplex operation. This mspi block is designed for connecting to NOR flash. This is an implementation detail and architectural choice that is specific to the Cygnus SoCs where a single MSPI is present, right? This same MSPI block is instantiated multiple times in BCM7xxx chips to interface to general purpose SPI slaves, ranging from Ethernet switches to bluetooth chips etc... those will definitively want full-duplex to be working. I would greatly appreciate if some thought to supporting full duplex transfers was given in this driver drop. If you cannot test that, then maybe just flag the driver has been half-duplex only for now... Thank you. Hi Florian, Yes, we can flag that - but we have no understanding of how the block operates in this mode. It would be great if your team could add the necessary full duplex support in a patch enhancement to the driver? Going blind on this one isn't the way to go. If I had a real full duplex slave I could experiment with it to get it working. But with the docs I have on this and considering how finicky the block is I think we're wasting our time. Jon -- 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/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
On 15-05-14 12:08 PM, Mark Brown wrote: On Thu, May 14, 2015 at 11:19:01AM -0700, Scott Branden wrote: On 15-05-14 03:31 AM, Mark Brown wrote: Chip vendors often say this sort of thing and then get surprised by what their users choose to do, and even if it only ever gets used with flash all it would take is some new flash command which can use full duplex for something. Please write the code so it at least tries to handle full duplex operation, if you can't test it fully that's not the end of the world. It doesn't look like it should be particularly difficult. Yes, there is always room for improvements in code. In this case - it really is not worth our time to add code we can't test. We try to deliver code that we can test and actually works. Yes, if anyone needs to use the While I try to not apply code that has obvious problems with silent data corruption in it which is what we have just now. mspi for full duplex operation code can be added in the future - it is software. This block has gone through many generations of our SoCs and has only been used for this purpose - the bootROM boots from this SPI only. It is dedicated for this purpose. All it takes is one hardware engineer who sees a SPI controller and a GPIO they can use for chip select; I wouldn't be so sure that nobody ever fixed this up locally (or happened to use a device that only needed single duplex). Hi Mark. Would it help if we just set the flags to half duplex using SPI_MASTER_HALF_DUPLEX when we register the master? -- 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/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
On 15-05-12 12:17 PM, Mark Brown wrote:
> On Tue, May 12, 2015 at 10:38:13AM -0700, Jonathan Richardson wrote:
>
>> +/* Wait for interrupt indicating transfer is complete. */
>> +if (!wait_for_completion_timeout(>xfer_done,
>> +msecs_to_jiffies(10))) {
>
> What if we have a large transfer on a slow bus?
I guess it depends on the reference clock frequency (as well as spbr)
which we may not know because I made it optional. I could possibly
calculate it. I'll look into this. I used the slowest possible spbr
based on our ref clock frequency and tested large transfers which never
timed out.
>
>> +while (bytes_processed < transfer->len) {
>
>> +/* Start transfer and wait until complete. */
>> +err = bcm_mspi_start_transfer(mspi, !last_slot, slot);
>> +if (err)
>> +return err;
>
> This isn't really start_transfer() is it? It's doing the entire
> operation.
Yes, the entire operation.
>
>> +/* Delay requested amount before next transfer. */
>> +udelay(transfer->delay_usecs);
>> +}
>
> This is buggy, it's applying the per-transfer delay every timme we fill
> the FIFO.
Thanks, I'll fix.
>
>> +/* The rx data will go into RXRAM0/1 + last tx length. */
>> +if (slot + 1 >= NUM_SLOTS)
>> +mspi->rx_slot = 0;
>> +else
>> +mspi->rx_slot = slot + 1;
>
> How is this going to work for full duplex transfers if we had to fill
> the FIFO more than once?
See below, it's not.
>
>> +static int bcm_mspi_transfer_one(struct spi_master *master,
>> +struct spi_device *spidev, struct spi_transfer *transfer)
>> +{
>> +int err;
>> +
>> +/* 8 bit transfers only are currently supported. */
>> +if (transfer->bits_per_word > 8)
>> +return -ENOTSUPP;
>
> Tell the core what the device supports and it will check for you.
>
>> +
>> +err = bcm_mspi_tx_data(master, spidev, transfer);
>> +if (err)
>> +return err;
>> +
>> +err = bcm_mspi_rx_data(master, spidev, transfer);
>> +if (err)
>> +return err;
>> +
>> +return 0;
>> +}
>
> I would expect the recieve and transmit operations to be running in
> parallel, not executed one after another, given the need to keep
> manually filling and draining the FIFOs.
The driver was only written with NOR flash in mind as the slave. Since
this is really just half duplex, it works, though it won't with a real
full duplex slave. m25p80 never calls transfer_one with both tx and rx
buffers. The rx bytes that we don't care about from a tx request are
dropped.
We don't have any full duplex slaves so it's a hard to test. I could
possibly re-write this so that tx/rx is interleaved and that should be
good for full duplex or close enough that it won't have to be completely
overhauled should we ever connect a full duplex slave to it. Maybe I
should do that. This came from a pre-existing driver that had the same
limitation.
>
>> +struct spi_master *master;
>> +struct device *dev = >dev;
>> +int err;
>> +struct resource *res;
>> +unsigned int irq;
>> +
>> +dev_info(dev, "Initializing BCM MSPI\n");
>
> Don't spam the logs like this, there's no content in this message.
>
>> +master = spi_alloc_master(dev, sizeof(*data));
>> +if (!master) {
>
> devm_spi_alloc_master().
This doesn't exist but devm_spi_register_master() does which I used.
>
>> +/* Map base memory address. */
>> +res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +data->base = devm_ioremap_resource(dev, res);
>> +if (IS_ERR(data->base)) {
>> +dev_err(>dev, "unable to map base address\n");
>
> devm_ioremap_resource() will complain for you.
>
Thanks,
Jon
--
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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
On 15-05-13 01:27 PM, Richard Cochran wrote:
> On Wed, May 13, 2015 at 12:50:02PM -0700, Jonathan Richardson wrote:
>> ptp_clock_adjtime() casts it to an unsigned and returns an error:
>>
>>if ((unsigned long) ts.tv_nsec >= NSEC_PER_SEC)
>>return -EINVAL;
>
> The value of a timeval is the sum of its fields, but the field tv_usec
> must always be non-negative. The tv_sec field can be negative. So,
> your application simply needs to do this:
>
> if (tx.time.tv_usec < 0) {
> tx.time.tv_sec -= 1;
> tx.time.tv_usec += 10;
> }
That works, thanks.
>
>>>> IRQ interval: I mentioned before that we may be able to calculate the
>>>> isochronous interrupt rate automatically but this isn't possible because
>>>> the DTE doesn't know the frequency of the clients. One solution is to
>>>> use the 'PTP_PEROUT_REQUEST' ioctl to set a periodic timer frequency.
>>>> Not really a timer but good enough for our purposes.
>>>
>>> As I said in my other reply, I don't understand what the problem is.
>>
>> See reply to previous email. We can use this ioctl or add a new one as
>> Arnd suggested. It doesn't matter to me.
>
> Still makes no sense. Why should the interrupt rate depend on the
> clock frequency?
The isochronous interrupt rate is just a way to poll the hardware FIFO
for timestamps and transfer them into a s/w FIFO where they're available
to be read from user space.
The rate depends on how frequently clients will be generating timestamps
at and the driver doesn't know that. That's why we set it from user
space. If the interrupt rate is too fast it could bog down the system,
and it can be very fast. The highest rate is almost always going to be
too fast. If it's set too slow then there will be FIFO overflows. Since
only user space knows the frequency of the input channel, it's up to the
user to set an appropriate rate. A good rate may be twice the frequency
of the highest frequency client. Since what clients are being used at
runtime is unknown to us and will vary from system to system we really
don't know what to set this rate to. One user may only be interested in
audio, another may only be interested in something connected to a GPIO.
One user may be running audio at a fast sample rate, another very slow,
etc.
The only way I can see it working is if we enabled a client (external
time stamping channel in PTP) with an expected frequency, then set the
IRQ interval to some pre-determined rate (ie- 2x) and then adjust the
rate as new clients are enabled and divider values are set.
>
> If the ISR is delivering batches of time stamps, then the interrupt
> rate aught to be the highest rate that the system can support.
It to nanosecond precision so it can be ridiculously quick.
>
>>>> Set divider: There is no ability to set a frequency or do anything to a
>>>> channel. We could re-use the PTP_EXTTS_REQUEST ioctl but extend 'struct
>>>> ptp_extts_request' by using the reserved field rsv to allow setting an
>>>> integer value representing either a frequency or divider value in our
>>>> case - some value to configure a channel. A new flag would have to be
>>>> added to the existing PTP_ENABLE_FEATURE, RISING and FALLING EDGE.
>>>
>>> I don't get this, either.
>>
>> See reply to previous email.
>
> Didn't help me too much :(
Hope the description above helped. Timestamping can be controlled from
user space. Since we don't know which clients are being or the frequency
of the input, we don't know what to set the divider to. The user knows
what they have hooked up to a channel, the rate of it, and the rate they
want timestamps generated at (divided down to).
>
>>> The PTP interface supports poll/read just fine already.
>>
>> Yes that's why I wanted to re-use it. As it currently is, it's not going
>> to work for reasons I explained in previous follow up yesterday:
>>
>> http://marc.info/?l=linux-kernel=143147907431947=2
>
> You said:
>
>one user space process would have to read timestamps for all
>channels/clients and then leave it up to user space IPC to get them
>to other processes, which isn't great.
>
> I disagree. I think having tons of fifos isn't great.
Maybe an example will help. We have user space process A interested in
channel 1, and B in channel 2. A timestamp for channel 2 is in the FIFO
(the FIFO holds timestamps for all channels). What happens if client A
does a read for a timestamp and it's for channel 2? The timestamp has
been pulled out of the s/w FIFO in the driver. What do we do with it now?
Having separate FIFO's allows process A to only retrieve channel 1
t
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
On 15-05-13 08:35 AM, Richard Cochran wrote: > On Fri, May 08, 2015 at 01:02:17PM -0700, Jonathan Richardson wrote: >> For the clock functions I think we can use the existing framework >> unchanged with one exception: ptp_clock_adjtime() doesn't allow negative >> time adjustments and we would like to allow this. > > ??? > > /** > * struct ptp_clock_info - decribes a PTP hardware clock >... > > * @adjtime: Shifts the time of the hardware clock. > *parameter delta: Desired change in nanoseconds. >... > > int (*adjtime)(struct ptp_clock_info *ptp, s64 delta); > > That s64 is 's' as in "signed". ptp_clock_adjtime() casts it to an unsigned and returns an error: if ((unsigned long) ts.tv_nsec >= NSEC_PER_SEC) return -EINVAL; > >> IRQ interval: I mentioned before that we may be able to calculate the >> isochronous interrupt rate automatically but this isn't possible because >> the DTE doesn't know the frequency of the clients. One solution is to >> use the 'PTP_PEROUT_REQUEST' ioctl to set a periodic timer frequency. >> Not really a timer but good enough for our purposes. > > As I said in my other reply, I don't understand what the problem is. See reply to previous email. We can use this ioctl or add a new one as Arnd suggested. It doesn't matter to me. > >> Set divider: There is no ability to set a frequency or do anything to a >> channel. We could re-use the PTP_EXTTS_REQUEST ioctl but extend 'struct >> ptp_extts_request' by using the reserved field rsv to allow setting an >> integer value representing either a frequency or divider value in our >> case - some value to configure a channel. A new flag would have to be >> added to the existing PTP_ENABLE_FEATURE, RISING and FALLING EDGE. > > I don't get this, either. See reply to previous email. > >> Get timestamp: This is a bit more complicated. Currently the PTP driver >> does list management for timestamps from external timestamp channels. >> Timestamps from all channels go into the same list. In our driver we >> have a s/w FIFO for each client and it closely matches the h/w FIFO and >> handles any overflow. We would like to keep it this way because it also >> allows multiple user space processes to only fetch timestamps for the >> client it's handling. > > But having many readers is less efficient and more complex. > > Also, we can adjust the buffer if needed to prevent HW FIFO overflows. > >> We could add a new ioctl to get a timestamp from >> the driver instead of doing it through ptp_read() but it would be nice >> if we could let ptp_read() allow the driver to do timestamp management >> instead of PTP. Maybe provide an option to obtain the timestamps from a >> container in the driver instead of the one managed by PTP. I like being >> able to use read/poll to obtain data instead of polling the kernel with >> ioctls as we are currently doing. > > The PTP interface supports poll/read just fine already. Yes that's why I wanted to re-use it. As it currently is, it's not going to work for reasons I explained in previous follow up yesterday: http://marc.info/?l=linux-kernel=143147907431947=2 > >> Also, avoiding the kmalloc in ptp_read >> would be nice because this of the frequency it would be called at. Do >> you have any preference on how to handle this? > > Originally I had the buffer on the stack, but DaveM didn't like it, > saying performance is no excuse for not doing it "the right way". > > Thanks, > Richard > -- 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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Richard, glad you retrieved our driver. Comments below. On 15-05-13 08:21 AM, Richard Cochran wrote: > On Fri, May 01, 2015 at 12:01:07PM -0700, Jonathan Richardson wrote: >> The DTE creates timestamps of hardware based events such as GPIO, I2S >> signals for audio, etc. It was also intended to provide 802.1AS / PTP >> timestamps for network packets. The h/w has up to 32 "clients" -- the >> hardware inputs into a timestamping engine. These clients are specific >> to the chip the DTE is used on. For Cygnus you can see what they are in >> our 'enum dte_client' from bcm_cygnus_dte.h. > > These 32 channels should either go to kernel consumers (i2s audio) or, > if there aren't any, to the generic PTP ioctl. > >> The DTE timestamper creates timestamps based on the current clock wall >> time. > > What do you mean by "wall time"? CLOCK_REALTIME? It's just the incrementing local NCO time. Whatever the value of it is is the timestamp time. When we add the timestamp to the s/w FIFO we adjust the time to a reference time. > >> When an event occurs it stores the timestamp in a h/w FIFO. Each >> client also has a divider that can be set to control the rate that >> timestamps are generated at by the timestamper and these are adjustable >> at run time. > > This does not make sense. If you time stamp events, then the rate is > determined by the events themselves. The input events may occur at a rate greater than we want to generate timestamps at. Maybe we need a better term than divider. The h/w sees x input signals over x period of time and then samples the input signal at a divided down period of time to limit the timestamp rate. Timestamp input sample rate? > >> It's a bit more than a PTP hardware clock on a NIC. It's a clock for PTP >> plus timestamping 32 other hardware inputs that can be enabled at any >> time with timestamps being generated at varying frequencies. As clients >> are enabled that generate timestamps at higher frequencies, the >> isochronous interrupt frequency needs to be increased so that overflows >> in the the h/w and s/w FIFO's don't occur (this frequency could possibly >> be automated instead of changing it manually as we currently do). > > Yes, the driver should configure an appropriate rate automatically. I was wrong. It can't. The DTE doesn't know anything about the inputs hooked up to it nor the frequency of the input events. It needs to be set by user space and the value can change at run time whenever we want to change it. > >> It looks like the driver could almost be a PTP driver instead of a char >> driver controlled with ioctls. PTP does this already using >> clock_gettime(), clock_settime(), clock_adjtime(). But we want to set >> the frequency as well as adjust the clock and I don't see how that is >> possible with the stripped down timex data passed to the driver from >> ptp_clock_adjtime(). > > You can convert ppb into your time base using simple math. Yes, this isn't a problem. > >> We have the additional requirement of controlling multiple clients and >> retrieving the timestamps, etc. The PTP driver allows for some control >> of external time stamp channels already using 'n_ext_ts' in struct >> ptp_clock_info. We could use that to enable clients and get timestamps, >> but we also need to be able to change dividers for the clients at run >> time if desired. It doesn't look like additional ioctls could be passed >> to a PTP driver because ptp_ioctl() is the ioctl handler. > > I don't see any need for use space to set dividers. Let the driver do > that. See above. Thanks. > > Thanks, > Richard > -- 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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Richard, glad you retrieved our driver. Comments below. On 15-05-13 08:21 AM, Richard Cochran wrote: > On Fri, May 01, 2015 at 12:01:07PM -0700, Jonathan Richardson wrote: >> The DTE creates timestamps of hardware based events such as GPIO, I2S >> signals for audio, etc. It was also intended to provide 802.1AS / PTP >> timestamps for network packets. The h/w has up to 32 "clients" -- the >> hardware inputs into a timestamping engine. These clients are specific >> to the chip the DTE is used on. For Cygnus you can see what they are in >> our 'enum dte_client' from bcm_cygnus_dte.h. > > These 32 channels should either go to kernel consumers (i2s audio) or, > if there aren't any, to the generic PTP ioctl. > >> The DTE timestamper creates timestamps based on the current clock wall >> time. > > What do you mean by "wall time"? CLOCK_REALTIME? It's just the incrementing local NCO time. Whatever the value of it is is the timestamp time. When we add the timestamp to the s/w FIFO we adjust the time to a reference time. > >> When an event occurs it stores the timestamp in a h/w FIFO. Each >> client also has a divider that can be set to control the rate that >> timestamps are generated at by the timestamper and these are adjustable >> at run time. > > This does not make sense. If you time stamp events, then the rate is > determined by the events themselves. The input events may occur at a rate greater than we want to generate timestamps at. Maybe we need a better term than divider. The h/w sees x input signals over x period of time and then samples the input signal at a divided down period of time to limit the timestamp rate. Timestamp input sample rate? > >> It's a bit more than a PTP hardware clock on a NIC. It's a clock for PTP >> plus timestamping 32 other hardware inputs that can be enabled at any >> time with timestamps being generated at varying frequencies. As clients >> are enabled that generate timestamps at higher frequencies, the >> isochronous interrupt frequency needs to be increased so that overflows >> in the the h/w and s/w FIFO's don't occur (this frequency could possibly >> be automated instead of changing it manually as we currently do). > > Yes, the driver should configure an appropriate rate automatically. I was wrong. It can't. The DTE doesn't know anything about the inputs hooked up to it nor the frequency of the input events. It needs to be set by user space and the value can change at run time whenever we want to change it. > >> It looks like the driver could almost be a PTP driver instead of a char >> driver controlled with ioctls. PTP does this already using >> clock_gettime(), clock_settime(), clock_adjtime(). But we want to set >> the frequency as well as adjust the clock and I don't see how that is >> possible with the stripped down timex data passed to the driver from >> ptp_clock_adjtime(). > > You can convert ppb into your time base using simple math. Yes, this isn't a problem. > >> We have the additional requirement of controlling multiple clients and >> retrieving the timestamps, etc. The PTP driver allows for some control >> of external time stamp channels already using 'n_ext_ts' in struct >> ptp_clock_info. We could use that to enable clients and get timestamps, >> but we also need to be able to change dividers for the clients at run >> time if desired. It doesn't look like additional ioctls could be passed >> to a PTP driver because ptp_ioctl() is the ioctl handler. > > I don't see any need for use space to set dividers. Let the driver do > that. See above. Thanks. > > Thanks, > Richard > -- 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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Richard, glad you retrieved our driver. Comments below. On 15-05-13 08:21 AM, Richard Cochran wrote: On Fri, May 01, 2015 at 12:01:07PM -0700, Jonathan Richardson wrote: The DTE creates timestamps of hardware based events such as GPIO, I2S signals for audio, etc. It was also intended to provide 802.1AS / PTP timestamps for network packets. The h/w has up to 32 clients -- the hardware inputs into a timestamping engine. These clients are specific to the chip the DTE is used on. For Cygnus you can see what they are in our 'enum dte_client' from bcm_cygnus_dte.h. These 32 channels should either go to kernel consumers (i2s audio) or, if there aren't any, to the generic PTP ioctl. The DTE timestamper creates timestamps based on the current clock wall time. What do you mean by wall time? CLOCK_REALTIME? It's just the incrementing local NCO time. Whatever the value of it is is the timestamp time. When we add the timestamp to the s/w FIFO we adjust the time to a reference time. When an event occurs it stores the timestamp in a h/w FIFO. Each client also has a divider that can be set to control the rate that timestamps are generated at by the timestamper and these are adjustable at run time. This does not make sense. If you time stamp events, then the rate is determined by the events themselves. The input events may occur at a rate greater than we want to generate timestamps at. Maybe we need a better term than divider. The h/w sees x input signals over x period of time and then samples the input signal at a divided down period of time to limit the timestamp rate. Timestamp input sample rate? It's a bit more than a PTP hardware clock on a NIC. It's a clock for PTP plus timestamping 32 other hardware inputs that can be enabled at any time with timestamps being generated at varying frequencies. As clients are enabled that generate timestamps at higher frequencies, the isochronous interrupt frequency needs to be increased so that overflows in the the h/w and s/w FIFO's don't occur (this frequency could possibly be automated instead of changing it manually as we currently do). Yes, the driver should configure an appropriate rate automatically. I was wrong. It can't. The DTE doesn't know anything about the inputs hooked up to it nor the frequency of the input events. It needs to be set by user space and the value can change at run time whenever we want to change it. It looks like the driver could almost be a PTP driver instead of a char driver controlled with ioctls. PTP does this already using clock_gettime(), clock_settime(), clock_adjtime(). But we want to set the frequency as well as adjust the clock and I don't see how that is possible with the stripped down timex data passed to the driver from ptp_clock_adjtime(). You can convert ppb into your time base using simple math. Yes, this isn't a problem. We have the additional requirement of controlling multiple clients and retrieving the timestamps, etc. The PTP driver allows for some control of external time stamp channels already using 'n_ext_ts' in struct ptp_clock_info. We could use that to enable clients and get timestamps, but we also need to be able to change dividers for the clients at run time if desired. It doesn't look like additional ioctls could be passed to a PTP driver because ptp_ioctl() is the ioctl handler. I don't see any need for use space to set dividers. Let the driver do that. See above. Thanks. Thanks, Richard -- 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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Richard, glad you retrieved our driver. Comments below. On 15-05-13 08:21 AM, Richard Cochran wrote: On Fri, May 01, 2015 at 12:01:07PM -0700, Jonathan Richardson wrote: The DTE creates timestamps of hardware based events such as GPIO, I2S signals for audio, etc. It was also intended to provide 802.1AS / PTP timestamps for network packets. The h/w has up to 32 clients -- the hardware inputs into a timestamping engine. These clients are specific to the chip the DTE is used on. For Cygnus you can see what they are in our 'enum dte_client' from bcm_cygnus_dte.h. These 32 channels should either go to kernel consumers (i2s audio) or, if there aren't any, to the generic PTP ioctl. The DTE timestamper creates timestamps based on the current clock wall time. What do you mean by wall time? CLOCK_REALTIME? It's just the incrementing local NCO time. Whatever the value of it is is the timestamp time. When we add the timestamp to the s/w FIFO we adjust the time to a reference time. When an event occurs it stores the timestamp in a h/w FIFO. Each client also has a divider that can be set to control the rate that timestamps are generated at by the timestamper and these are adjustable at run time. This does not make sense. If you time stamp events, then the rate is determined by the events themselves. The input events may occur at a rate greater than we want to generate timestamps at. Maybe we need a better term than divider. The h/w sees x input signals over x period of time and then samples the input signal at a divided down period of time to limit the timestamp rate. Timestamp input sample rate? It's a bit more than a PTP hardware clock on a NIC. It's a clock for PTP plus timestamping 32 other hardware inputs that can be enabled at any time with timestamps being generated at varying frequencies. As clients are enabled that generate timestamps at higher frequencies, the isochronous interrupt frequency needs to be increased so that overflows in the the h/w and s/w FIFO's don't occur (this frequency could possibly be automated instead of changing it manually as we currently do). Yes, the driver should configure an appropriate rate automatically. I was wrong. It can't. The DTE doesn't know anything about the inputs hooked up to it nor the frequency of the input events. It needs to be set by user space and the value can change at run time whenever we want to change it. It looks like the driver could almost be a PTP driver instead of a char driver controlled with ioctls. PTP does this already using clock_gettime(), clock_settime(), clock_adjtime(). But we want to set the frequency as well as adjust the clock and I don't see how that is possible with the stripped down timex data passed to the driver from ptp_clock_adjtime(). You can convert ppb into your time base using simple math. Yes, this isn't a problem. We have the additional requirement of controlling multiple clients and retrieving the timestamps, etc. The PTP driver allows for some control of external time stamp channels already using 'n_ext_ts' in struct ptp_clock_info. We could use that to enable clients and get timestamps, but we also need to be able to change dividers for the clients at run time if desired. It doesn't look like additional ioctls could be passed to a PTP driver because ptp_ioctl() is the ioctl handler. I don't see any need for use space to set dividers. Let the driver do that. See above. Thanks. Thanks, Richard -- 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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
On 15-05-13 08:35 AM, Richard Cochran wrote: On Fri, May 08, 2015 at 01:02:17PM -0700, Jonathan Richardson wrote: For the clock functions I think we can use the existing framework unchanged with one exception: ptp_clock_adjtime() doesn't allow negative time adjustments and we would like to allow this. ??? /** * struct ptp_clock_info - decribes a PTP hardware clock ... * @adjtime: Shifts the time of the hardware clock. *parameter delta: Desired change in nanoseconds. ... int (*adjtime)(struct ptp_clock_info *ptp, s64 delta); That s64 is 's' as in signed. ptp_clock_adjtime() casts it to an unsigned and returns an error: if ((unsigned long) ts.tv_nsec = NSEC_PER_SEC) return -EINVAL; IRQ interval: I mentioned before that we may be able to calculate the isochronous interrupt rate automatically but this isn't possible because the DTE doesn't know the frequency of the clients. One solution is to use the 'PTP_PEROUT_REQUEST' ioctl to set a periodic timer frequency. Not really a timer but good enough for our purposes. As I said in my other reply, I don't understand what the problem is. See reply to previous email. We can use this ioctl or add a new one as Arnd suggested. It doesn't matter to me. Set divider: There is no ability to set a frequency or do anything to a channel. We could re-use the PTP_EXTTS_REQUEST ioctl but extend 'struct ptp_extts_request' by using the reserved field rsv to allow setting an integer value representing either a frequency or divider value in our case - some value to configure a channel. A new flag would have to be added to the existing PTP_ENABLE_FEATURE, RISING and FALLING EDGE. I don't get this, either. See reply to previous email. Get timestamp: This is a bit more complicated. Currently the PTP driver does list management for timestamps from external timestamp channels. Timestamps from all channels go into the same list. In our driver we have a s/w FIFO for each client and it closely matches the h/w FIFO and handles any overflow. We would like to keep it this way because it also allows multiple user space processes to only fetch timestamps for the client it's handling. But having many readers is less efficient and more complex. Also, we can adjust the buffer if needed to prevent HW FIFO overflows. We could add a new ioctl to get a timestamp from the driver instead of doing it through ptp_read() but it would be nice if we could let ptp_read() allow the driver to do timestamp management instead of PTP. Maybe provide an option to obtain the timestamps from a container in the driver instead of the one managed by PTP. I like being able to use read/poll to obtain data instead of polling the kernel with ioctls as we are currently doing. The PTP interface supports poll/read just fine already. Yes that's why I wanted to re-use it. As it currently is, it's not going to work for reasons I explained in previous follow up yesterday: http://marc.info/?l=linux-kernelm=143147907431947w=2 Also, avoiding the kmalloc in ptp_read would be nice because this of the frequency it would be called at. Do you have any preference on how to handle this? Originally I had the buffer on the stack, but DaveM didn't like it, saying performance is no excuse for not doing it the right way. Thanks, Richard -- 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/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
On 15-05-12 12:17 PM, Mark Brown wrote:
On Tue, May 12, 2015 at 10:38:13AM -0700, Jonathan Richardson wrote:
+/* Wait for interrupt indicating transfer is complete. */
+if (!wait_for_completion_timeout(mspi-xfer_done,
+msecs_to_jiffies(10))) {
What if we have a large transfer on a slow bus?
I guess it depends on the reference clock frequency (as well as spbr)
which we may not know because I made it optional. I could possibly
calculate it. I'll look into this. I used the slowest possible spbr
based on our ref clock frequency and tested large transfers which never
timed out.
+while (bytes_processed transfer-len) {
+/* Start transfer and wait until complete. */
+err = bcm_mspi_start_transfer(mspi, !last_slot, slot);
+if (err)
+return err;
This isn't really start_transfer() is it? It's doing the entire
operation.
Yes, the entire operation.
+/* Delay requested amount before next transfer. */
+udelay(transfer-delay_usecs);
+}
This is buggy, it's applying the per-transfer delay every timme we fill
the FIFO.
Thanks, I'll fix.
+/* The rx data will go into RXRAM0/1 + last tx length. */
+if (slot + 1 = NUM_SLOTS)
+mspi-rx_slot = 0;
+else
+mspi-rx_slot = slot + 1;
How is this going to work for full duplex transfers if we had to fill
the FIFO more than once?
See below, it's not.
+static int bcm_mspi_transfer_one(struct spi_master *master,
+struct spi_device *spidev, struct spi_transfer *transfer)
+{
+int err;
+
+/* 8 bit transfers only are currently supported. */
+if (transfer-bits_per_word 8)
+return -ENOTSUPP;
Tell the core what the device supports and it will check for you.
+
+err = bcm_mspi_tx_data(master, spidev, transfer);
+if (err)
+return err;
+
+err = bcm_mspi_rx_data(master, spidev, transfer);
+if (err)
+return err;
+
+return 0;
+}
I would expect the recieve and transmit operations to be running in
parallel, not executed one after another, given the need to keep
manually filling and draining the FIFOs.
The driver was only written with NOR flash in mind as the slave. Since
this is really just half duplex, it works, though it won't with a real
full duplex slave. m25p80 never calls transfer_one with both tx and rx
buffers. The rx bytes that we don't care about from a tx request are
dropped.
We don't have any full duplex slaves so it's a hard to test. I could
possibly re-write this so that tx/rx is interleaved and that should be
good for full duplex or close enough that it won't have to be completely
overhauled should we ever connect a full duplex slave to it. Maybe I
should do that. This came from a pre-existing driver that had the same
limitation.
+struct spi_master *master;
+struct device *dev = pdev-dev;
+int err;
+struct resource *res;
+unsigned int irq;
+
+dev_info(dev, Initializing BCM MSPI\n);
Don't spam the logs like this, there's no content in this message.
+master = spi_alloc_master(dev, sizeof(*data));
+if (!master) {
devm_spi_alloc_master().
This doesn't exist but devm_spi_register_master() does which I used.
+/* Map base memory address. */
+res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+data-base = devm_ioremap_resource(dev, res);
+if (IS_ERR(data-base)) {
+dev_err(pdev-dev, unable to map base address\n);
devm_ioremap_resource() will complain for you.
Thanks,
Jon
--
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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
On 15-05-13 01:27 PM, Richard Cochran wrote:
On Wed, May 13, 2015 at 12:50:02PM -0700, Jonathan Richardson wrote:
ptp_clock_adjtime() casts it to an unsigned and returns an error:
if ((unsigned long) ts.tv_nsec = NSEC_PER_SEC)
return -EINVAL;
The value of a timeval is the sum of its fields, but the field tv_usec
must always be non-negative. The tv_sec field can be negative. So,
your application simply needs to do this:
if (tx.time.tv_usec 0) {
tx.time.tv_sec -= 1;
tx.time.tv_usec += 10;
}
That works, thanks.
IRQ interval: I mentioned before that we may be able to calculate the
isochronous interrupt rate automatically but this isn't possible because
the DTE doesn't know the frequency of the clients. One solution is to
use the 'PTP_PEROUT_REQUEST' ioctl to set a periodic timer frequency.
Not really a timer but good enough for our purposes.
As I said in my other reply, I don't understand what the problem is.
See reply to previous email. We can use this ioctl or add a new one as
Arnd suggested. It doesn't matter to me.
Still makes no sense. Why should the interrupt rate depend on the
clock frequency?
The isochronous interrupt rate is just a way to poll the hardware FIFO
for timestamps and transfer them into a s/w FIFO where they're available
to be read from user space.
The rate depends on how frequently clients will be generating timestamps
at and the driver doesn't know that. That's why we set it from user
space. If the interrupt rate is too fast it could bog down the system,
and it can be very fast. The highest rate is almost always going to be
too fast. If it's set too slow then there will be FIFO overflows. Since
only user space knows the frequency of the input channel, it's up to the
user to set an appropriate rate. A good rate may be twice the frequency
of the highest frequency client. Since what clients are being used at
runtime is unknown to us and will vary from system to system we really
don't know what to set this rate to. One user may only be interested in
audio, another may only be interested in something connected to a GPIO.
One user may be running audio at a fast sample rate, another very slow,
etc.
The only way I can see it working is if we enabled a client (external
time stamping channel in PTP) with an expected frequency, then set the
IRQ interval to some pre-determined rate (ie- 2x) and then adjust the
rate as new clients are enabled and divider values are set.
If the ISR is delivering batches of time stamps, then the interrupt
rate aught to be the highest rate that the system can support.
It to nanosecond precision so it can be ridiculously quick.
Set divider: There is no ability to set a frequency or do anything to a
channel. We could re-use the PTP_EXTTS_REQUEST ioctl but extend 'struct
ptp_extts_request' by using the reserved field rsv to allow setting an
integer value representing either a frequency or divider value in our
case - some value to configure a channel. A new flag would have to be
added to the existing PTP_ENABLE_FEATURE, RISING and FALLING EDGE.
I don't get this, either.
See reply to previous email.
Didn't help me too much :(
Hope the description above helped. Timestamping can be controlled from
user space. Since we don't know which clients are being or the frequency
of the input, we don't know what to set the divider to. The user knows
what they have hooked up to a channel, the rate of it, and the rate they
want timestamps generated at (divided down to).
The PTP interface supports poll/read just fine already.
Yes that's why I wanted to re-use it. As it currently is, it's not going
to work for reasons I explained in previous follow up yesterday:
http://marc.info/?l=linux-kernelm=143147907431947w=2
You said:
one user space process would have to read timestamps for all
channels/clients and then leave it up to user space IPC to get them
to other processes, which isn't great.
I disagree. I think having tons of fifos isn't great.
Maybe an example will help. We have user space process A interested in
channel 1, and B in channel 2. A timestamp for channel 2 is in the FIFO
(the FIFO holds timestamps for all channels). What happens if client A
does a read for a timestamp and it's for channel 2? The timestamp has
been pulled out of the s/w FIFO in the driver. What do we do with it now?
Having separate FIFO's allows process A to only retrieve channel 1
timestamps.
Our original non PTP driver ioctl (DTE_GET_TIMESTAMP) solved that
problem because we could specify a channel. We're now trying to adapt it
to PTP so we don't have to write a new DTE user and kernel side API.
Ideally, there will be kernel consumers for most of the channels, and
they will forward the time stamps in a way appropriate to their
subsystem. For example, network devices will use so_timestamping.
Any leftover channels can go through the generic PTP
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
A bit more info is required here. > Get timestamp: This is a bit more complicated. Currently the PTP driver > does list management for timestamps from external timestamp channels. > Timestamps from all channels go into the same list. In our driver we > have a s/w FIFO for each client and it closely matches the h/w FIFO and > handles any overflow. We would like to keep it this way because it also > allows multiple user space processes to only fetch timestamps for the > client it's handling. We could add a new ioctl to get a timestamp from > the driver instead of doing it through ptp_read() but it would be nice > if we could let ptp_read() allow the driver to do timestamp management > instead of PTP. Maybe provide an option to obtain the timestamps from a > container in the driver instead of the one managed by PTP. I like being > able to use read/poll to obtain data instead of polling the kernel with > ioctls as we are currently doing. Also, avoiding the kmalloc in ptp_read > would be nice because this of the frequency it would be called at. Do > you have any preference on how to handle this? > > I've tried to minimize the changes to PTP. Using read() we can't specify a channel to get the timestamp for. It would imply that one user space process would have to read timestamps for all channels/clients and then leave it up to user space IPC to get them to other processes, which isn't great. That means either we introduce an ioctl such as the DTE_GET_TIMESTAMP we had before which allows us to specify a channel, or we need to look at creating one dev node per external timestamping channel. The ioctl limitation is that it pounds the kernel polling for timestamps and the multiple dev nodes per channel is a big change to PTP. I will have to look further into this to really have a good idea of what the implications would be. Advice/ideas? Thanks. -- 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 v7 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures
The config procedure didn't follow the spec which periodically resulted
in failing to enable the output signal. This happened one in ten or
twenty attempts. Following the spec and adding a 400ns delay in the
appropriate locations resolves this problem.
The disable procedure now also follows the spec. The old disable
procedure would result in no change in signal when called.
The polarity procedure no longer applies the settings to change the
output signal because it can't be called when the pwm is enabled anyway.
The polarity is only updated in the control register. The correct
polarity will be applied on enable. The old method of applying changes
would result in no signal when the polarity was changed. The new
apply_settings function would fix this problem but it isn't required
anyway.
Reviewed-by: Arun Ramamurthy
Reviewed-by: Scott Branden
Tested-by: Scott Branden
Signed-off-by: Jonathan Richardson
---
drivers/pwm/pwm-bcm-kona.c | 47 +---
1 file changed, 36 insertions(+), 11 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 32b3ec6..0654981 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -76,19 +76,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct
pwm_chip *_chip)
return container_of(_chip, struct kona_pwmc, chip);
}
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+/*
+ * Clear trigger bit but set smooth bit to maintain old output.
+ */
+static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
+ unsigned int chan)
{
unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
- /* Clear trigger bit but set smooth bit to maintain old output */
value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
writel(value, kp->base + PWM_CONTROL_OFFSET);
+ /*
+* There must be a min 400ns delay between clearing enable and setting
+* it. Failing to do this may result in no PWM signal.
+*/
+ ndelay(400);
+}
+
+static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+{
+ unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
/* Set trigger bit and clear smooth bit to apply new settings */
value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
writel(value, kp->base + PWM_CONTROL_OFFSET);
+
+ /* PWMOUT_ENABLE must be held high for at least 400 ns. */
+ ndelay(400);
}
static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -133,8 +150,14 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct
pwm_device *pwm,
return -EINVAL;
}
- /* If the PWM channel is enabled, write the settings to the HW */
+ /*
+* Don't apply settings if disabled. The period and duty cycle are
+* always calculated above to ensure the new values are
+* validated immediately instead of on enable.
+*/
if (test_bit(PWMF_ENABLED, >flags)) {
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp->base + PRESCALE_OFFSET);
value &= ~PRESCALE_MASK(chan);
value |= prescale << PRESCALE_SHIFT(chan);
@@ -173,11 +196,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
writel(value, kp->base + PWM_CONTROL_OFFSET);
- kona_pwmc_apply_settings(kp, chan);
-
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
-
clk_disable_unprepare(kp->clk);
return 0;
@@ -207,13 +225,20 @@ static void kona_pwmc_disable(struct pwm_chip *chip,
struct pwm_device *pwm)
{
struct kona_pwmc *kp = to_kona_pwmc(chip);
unsigned int chan = pwm->hwpwm;
+ unsigned int value;
+
+ kona_pwmc_prepare_for_settings(kp, chan);
/* Simulate a disable by configuring for zero duty */
writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
- kona_pwmc_apply_settings(kp, chan);
+ writel(0, kp->base + PERIOD_COUNT_OFFSET(chan));
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
+ /* Set prescale to 0 for this channel */
+ value = readl(kp->base + PRESCALE_OFFSET);
+ value &= ~PRESCALE_MASK(chan);
+ writel(value, kp->base + PRESCALE_OFFSET);
+
+ kona_pwmc_apply_settings(kp, chan);
clk_disable_unprepare(kp->clk);
}
--
1.7.9.5
--
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 v7 2/5] drivers: pwm: bcm-kona: Dont set polarity in probe
From: Arun Ramamurthy
Omit setting the polarity to normal during probe and instead use the
new pwmchip_add_inversed function to register a PWM chip with default
polarity of inversed for all channels as this is the actual hardware
default.
Signed-off-by: Arun Ramamurthy
Reviewed-by: Ray Jui
Signed-off-by: Scott Branden
Signed-off-by: Tim Kryger
Signed-off-by: Jonathan Richardson
---
drivers/pwm/pwm-bcm-kona.c |9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 02bc048..32b3ec6 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -266,18 +266,15 @@ static int kona_pwmc_probe(struct platform_device *pdev)
return ret;
}
- /* Set smooth mode, push/pull, and normal polarity for all channels */
- for (chan = 0; chan < kp->chip.npwm; chan++) {
- value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
+ /* Set push/pull for all channels */
+ for (chan = 0; chan < kp->chip.npwm; chan++)
value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
- value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan));
- }
writel(value, kp->base + PWM_CONTROL_OFFSET);
clk_disable_unprepare(kp->clk);
- ret = pwmchip_add(>chip);
+ ret = pwmchip_add_inversed(>chip);
if (ret < 0)
dev_err(>dev, "failed to add PWM chip: %d\n", ret);
--
1.7.9.5
--
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 v7 5/5] pwm: core: Set enable state properly on failed call to enable
The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.
Tested-by: Jonathan Richardson
Reviewed-by: Dmitry Torokhov
Signed-off-by: Jonathan Richardson
---
drivers/pwm/core.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 224645f..18f5ac4 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -477,10 +477,20 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
*/
int pwm_enable(struct pwm_device *pwm)
{
- if (pwm && !test_and_set_bit(PWMF_ENABLED, >flags))
- return pwm->chip->ops->enable(pwm->chip, pwm);
+ int err;
+
+ if (!pwm)
+ return -EINVAL;
+
+ if (!test_and_set_bit(PWMF_ENABLED, >flags)) {
+ err = pwm->chip->ops->enable(pwm->chip, pwm);
+ if (err) {
+ clear_bit(PWMF_ENABLED, >flags);
+ return err;
+ }
+ }
- return pwm ? 0 : -EINVAL;
+ return 0;
}
EXPORT_SYMBOL_GPL(pwm_enable);
--
1.7.9.5
--
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 v7 4/5] pwm: kona: Add debug info to config function
Adds debugging info to config function where duty cycle and period
are calculated and verified.
Signed-off-by: Jonathan Richardson
---
drivers/pwm/pwm-bcm-kona.c | 25 +++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 0654981..c6069c7 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct
pwm_device *pwm,
dc = div64_u64(val, div);
/* If duty_ns or period_ns are not achievable then return */
- if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
+ if (pc < PERIOD_COUNT_MIN) {
+ dev_warn(chip->dev,
+ "%s: pwm[%d]: period=%d is not achievable,
pc=%lu, prescale=%lu\n",
+ __func__, chan, period_ns, pc, prescale);
return -EINVAL;
+ }
+
+ if (dc < DUTY_CYCLE_HIGH_MIN) {
+ if (0 != duty_ns) {
+ dev_warn(chip->dev,
+ "%s: pwm[%d]: duty cycle=%d is not
achievable, dc=%lu, prescale=%lu\n",
+ __func__, chan, duty_ns, dc, prescale);
+ }
+ return -EINVAL;
+ }
/* If pc and dc are in bounds, the calculation is done */
if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
break;
/* Otherwise, increase prescale and recalculate pc and dc */
- if (++prescale > PRESCALE_MAX)
+ if (++prescale > PRESCALE_MAX) {
+ dev_warn(chip->dev,
+ "%s: pwm[%d]: Prescale (=%lu) within max (=%d)
for period=%d and duty cycle=%d is not achievable\n",
+ __func__, chan, prescale, PRESCALE_MAX,
+ period_ns, duty_ns);
return -EINVAL;
+ }
}
+ dev_dbg(chip->dev, "pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n",
+ chan, pc, dc, prescale);
+
/*
* Don't apply settings if disabled. The period and duty cycle are
* always calculated above to ensure the new values are
--
1.7.9.5
--
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 v7 1/5] drivers: pwm: core: Add pwmchip_add_inversed
From: Tim Kryger
Add a new function to register a PWM chip with channels that have their
initial polarity as inversed. This benefits drivers of controllers that
by default operate with inversed polarity by removing the need to modify
the polarity during initialization.
Signed-off-by: Tim Kryger
Signed-off-by: Jonathan Richardson
---
drivers/pwm/core.c | 36
include/linux/pwm.h |6 ++
2 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index ba34c7d..224645f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -222,14 +222,8 @@ void *pwm_get_chip_data(struct pwm_device *pwm)
}
EXPORT_SYMBOL_GPL(pwm_get_chip_data);
-/**
- * pwmchip_add() - register a new PWM chip
- * @chip: the PWM chip to add
- *
- * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
- * will be used.
- */
-int pwmchip_add(struct pwm_chip *chip)
+static int pwmchip_add_with_polarity(struct pwm_chip *chip,
+enum pwm_polarity polarity)
{
struct pwm_device *pwm;
unsigned int i;
@@ -259,6 +253,7 @@ int pwmchip_add(struct pwm_chip *chip)
pwm->chip = chip;
pwm->pwm = chip->base + i;
pwm->hwpwm = i;
+ pwm->polarity = polarity;
radix_tree_insert(_tree, pwm->pwm, pwm);
}
@@ -279,9 +274,34 @@ out:
mutex_unlock(_lock);
return ret;
}
+
+/**
+ * pwmchip_add() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
+ * will be used. The initial polarity for all channels is normal.
+ */
+int pwmchip_add(struct pwm_chip *chip)
+{
+ return pwmchip_add_with_polarity(chip, PWM_POLARITY_NORMAL);
+}
EXPORT_SYMBOL_GPL(pwmchip_add);
/**
+ * pwmchip_add_inversed() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip->base < 0 then a dynamically assigned base
+ * will be used. The initial polarity for all channels is inversed.
+ */
+int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+ return pwmchip_add_with_polarity(chip, PWM_POLARITY_INVERSED);
+}
+EXPORT_SYMBOL_GPL(pwmchip_add_inversed);
+
+/**
* pwmchip_remove() - remove a PWM chip
* @chip: the PWM chip to remove
*
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e90628c..358547f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -183,6 +183,7 @@ int pwm_set_chip_data(struct pwm_device *pwm, void *data);
void *pwm_get_chip_data(struct pwm_device *pwm);
int pwmchip_add(struct pwm_chip *chip);
+int pwmchip_add_inversed(struct pwm_chip *chip);
int pwmchip_remove(struct pwm_chip *chip);
struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
unsigned int index,
@@ -217,6 +218,11 @@ static inline int pwmchip_add(struct pwm_chip *chip)
return -EINVAL;
}
+static inline int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+ return -EINVAL;
+}
+
static inline int pwmchip_remove(struct pwm_chip *chip)
{
return -EINVAL;
--
1.7.9.5
--
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 v7 0/5] Fix bugs in kona pwm driver and pwm core
This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also fixes a bug in the pwm core where the enabled state was incorrect on failed calls to enable. Also, a new function was added to the pwm core to add pwm chips with inversed polarity for chips that have a different default polarity than the core. The prevents incorrect polarity being reported. Debug info has been added to help debug configuring duty cycle and period. These changes are required for the Kona pwm driver to work on Cygnus and the same PWM IP is being used. Changes from v6: - Move new debugging info for duty cycle and period in config function into its own commit. - Add kona_pwmc_prepare_for_settings() function to remove duplicated code. Changes from v5: - Address Dmitry's comment on code cleanup of pwm_enable() in pwm core. - Including all patches from different contributors in this patchset. Some were left out in v4. Changes from v4: - Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity. - Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework. Arun Ramamurthy (1): drivers: pwm: bcm-kona: Dont set polarity in probe Jonathan Richardson (3): pwm: kona: Fix incorrect config, disable, and polarity procedures pwm: kona: Add debug info to config function pwm: core: Set enable state properly on failed call to enable Tim Kryger (1): drivers: pwm: core: Add pwmchip_add_inversed drivers/pwm/core.c | 52 ++-- drivers/pwm/pwm-bcm-kona.c | 81 +--- include/linux/pwm.h|6 3 files changed, 109 insertions(+), 30 deletions(-) -- 1.7.9.5 -- 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 0/2] Add Broadcom MSPI driver
This patchset adds support for Broadcoms MSPI SPI controller. It can be found on various SoC's such as Cygnus. Jonathan Richardson (2): ARM: dts: Add binding for Broadcom MSPI driver. spi: bcm-mspi: Add support for Broadcom MSPI driver. .../devicetree/bindings/spi/brcm,mspi-spi.txt | 39 ++ drivers/spi/Kconfig|5 + drivers/spi/Makefile |1 + drivers/spi/spi-bcm-mspi.c | 505 4 files changed, 550 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt create mode 100644 drivers/spi/spi-bcm-mspi.c -- 1.7.9.5 -- 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 1/2] ARM: dts: Add binding for Broadcom MSPI driver.
Signed-off-by: Jonathan Richardson
---
.../devicetree/bindings/spi/brcm,mspi-spi.txt | 39
1 file changed, 39 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
diff --git a/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
b/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
new file mode 100644
index 000..e86a7a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
@@ -0,0 +1,39 @@
+Broadcom MSPI controller
+
+The Broadcom MSPI controller is a SPI controller found on various chips such
+as Cygnus.
+
+Required properties:
+- compatible: Must be "brcm,mspi-v0".
+
+- reg: The first register is the physical base address of the MSPI controller.
+ The second register is the address of the MSPI interrupt control registers.
It
+ is only required for chips that have a separate register set for extended
+ interrupt control. This is required for Cygnus.
+
+- interrupts: Interrupt ID of the controller.
+
+Optional properties:
+- clocks: The MSPI reference clock. If not provided then it is assumed a clock
+ is enabled by default and no control of clock-frequency (see below) is
+ possible.
+
+- clock-names: The name of the reference clock.
+
+- clock-frequency: Desired frequency of the clock. This will set the serial
+ clock baud rate (SPBR) based on the reference clock frequency. The frequency
+ of the SPBR is mspi_clk / (2 * SPBR) where SPBR is a value between 1-255
+ determined by the desired 'clock-frequency'. If not provided then the default
+ baud rate of the controller is used.
+
+Example:
+
+mspi: mspi@18047200 {
+ compatible = "brcm,mspi-v0";
+ reg = <0x18047200 0x188>,
+ <0x180473a0 0x1c>;
+ interrupts = ;
+ clocks = <_clk>;
+ clock-names = "mspi_clk";
+ clock-frequency = <1250>;
+};
--
1.7.9.5
--
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 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
The MSPI controller is a SPI controller found on various Broadcom
SoC's such as Cygnus.
Signed-off-by: Jonathan Richardson
---
drivers/spi/Kconfig|5 +
drivers/spi/Makefile |1 +
drivers/spi/spi-bcm-mspi.c | 505
3 files changed, 511 insertions(+)
create mode 100644 drivers/spi/spi-bcm-mspi.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ab8dfbe..23622c2 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -121,6 +121,11 @@ config SPI_BCM53XX
help
Enable support for the SPI controller on Broadcom BCM53xx ARM SoCs.
+config SPI_BCM_MSPI
+ tristate "Broadcom MSPI controller"
+ help
+ Enable support for the Broadcom MSPI controller.
+
config SPI_BCM63XX
tristate "Broadcom BCM63xx SPI controller"
depends on BCM63XX
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index d8cbf65..5ebecba 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_SPI_BCM2835) += spi-bcm2835.o
obj-$(CONFIG_SPI_BCM53XX) += spi-bcm53xx.o
obj-$(CONFIG_SPI_BCM63XX) += spi-bcm63xx.o
obj-$(CONFIG_SPI_BCM63XX_HSSPI)+= spi-bcm63xx-hsspi.o
+obj-$(CONFIG_SPI_BCM_MSPI) += spi-bcm-mspi.o
obj-$(CONFIG_SPI_BFIN5XX) += spi-bfin5xx.o
obj-$(CONFIG_SPI_ADI_V3)+= spi-adi-v3.o
obj-$(CONFIG_SPI_BFIN_SPORT) += spi-bfin-sport.o
diff --git a/drivers/spi/spi-bcm-mspi.c b/drivers/spi/spi-bcm-mspi.c
new file mode 100644
index 000..9f6cc03
--- /dev/null
+++ b/drivers/spi/spi-bcm-mspi.c
@@ -0,0 +1,505 @@
+/*
+ * Copyright (C) 2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define MSPI_SPCR0_LSB_OFFSET 0x0
+#define MSPI_SPCR0_LSB_SHIFT0
+#define SPBR_MIN8U
+#define SPBR_MAX255U
+#define MSPI_NEWQP_OFFSET 0x10
+#define MSPI_ENDQP_OFFSET 0x14
+#define MSPI_SPCR2_OFFSET 0x18
+#define MSPI_SPCR2_SPE_SHIFT6
+#define MSPI_SPCR2_SPIFIE_SHIFT 5
+#define MSPI_SPCR2_CONT_AFTER_CMD_SHIFT 7
+#define MSPI_STATUS_OFFSET 0x20
+#define MSPI_TXRAM_OFFSET 0x40
+#define MSPI_RXRAM_OFFSET 0xc0
+#define MSPI_CDRAM_OFFSET 0x140
+#define MSPI_CDRAM_CONT_SHIFT 7
+#define MSPI_CDRAM_BITSE_SHIFT 6
+#define MSPI_CDRAM_PCS_SHIFT0
+#define MSPI_WRITE_LOCK_OFFSET 0x180
+#define INTERRUPT_MSPI_DONE_OFFSET 0x14
+
+#define NUM_SLOTS 16
+
+struct bcm_mspi {
+ struct platform_device *pdev;
+ void __iomem*base;
+/* Base of interrupt control regs. */
+ void __iomem*int_base;
+ struct spi_master *master;
+ struct clk *clk;
+ u32 spbr;
+ struct completion xfer_done;
+ int rx_slot;
+};
+
+/*
+ * Calculate TXRAM offset for the requested slot.
+ */
+static inline u32 tx_offset(int slot)
+{
+ BUG_ON(slot > NUM_SLOTS);
+ return 4 * (slot * 2);
+}
+
+/*
+ * Calculate RXRAM offset for the requested slot.
+ */
+static inline u32 rx_offset(int slot)
+{
+ BUG_ON(slot > NUM_SLOTS);
+ return 4 * (1 + slot * 2);
+}
+
+/*
+ * Calculate CDRAM offset for the requested slot.
+ */
+static inline u32 cdram_offset(int slot)
+{
+ BUG_ON(slot > NUM_SLOTS);
+ return MSPI_CDRAM_OFFSET + (4 * slot);
+}
+
+/*
+ * Start tx or rx SPI transfer and wait until complete. The queue start pointer
+ * is always 0. The end queue pointer is passed in to end_slot.
+ *
+ * @set_cont Set the CONT bit if true, clear if false. The CONT bit must be set
+ * if all slots are filled and there is more data to be transferred.
+ * @end_slot The end queue pointer.
+ * Returns 0 if successful, -EIO if transfer timed out.
+ */
+static int bcm_mspi_start_transfer(struct bcm_mspi *mspi, bool set_cont,
+ int end_slot)
+{
+ int val;
+ int err = 0;
+
+ /* Set queue pointers for transmit. */
+ writel(0, mspi->base + MSPI_NEWQP_OFFSET);
+ writel(end_slot, mspi->base + MSPI_ENDQP_OFFSET);
+ dev_dbg(>pdev->dev, "NEWQP: %d ENDQP: %d\n", 0, end_slot);
+
+
[PATCH 0/2] Add Broadcom MSPI driver
This patchset adds support for Broadcoms MSPI SPI controller. It can be found on various SoC's such as Cygnus. Jonathan Richardson (2): ARM: dts: Add binding for Broadcom MSPI driver. spi: bcm-mspi: Add support for Broadcom MSPI driver. .../devicetree/bindings/spi/brcm,mspi-spi.txt | 39 ++ drivers/spi/Kconfig|5 + drivers/spi/Makefile |1 + drivers/spi/spi-bcm-mspi.c | 505 4 files changed, 550 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt create mode 100644 drivers/spi/spi-bcm-mspi.c -- 1.7.9.5 -- 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 1/2] ARM: dts: Add binding for Broadcom MSPI driver.
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
.../devicetree/bindings/spi/brcm,mspi-spi.txt | 39
1 file changed, 39 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
diff --git a/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
b/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
new file mode 100644
index 000..e86a7a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
@@ -0,0 +1,39 @@
+Broadcom MSPI controller
+
+The Broadcom MSPI controller is a SPI controller found on various chips such
+as Cygnus.
+
+Required properties:
+- compatible: Must be brcm,mspi-v0.
+
+- reg: The first register is the physical base address of the MSPI controller.
+ The second register is the address of the MSPI interrupt control registers.
It
+ is only required for chips that have a separate register set for extended
+ interrupt control. This is required for Cygnus.
+
+- interrupts: Interrupt ID of the controller.
+
+Optional properties:
+- clocks: The MSPI reference clock. If not provided then it is assumed a clock
+ is enabled by default and no control of clock-frequency (see below) is
+ possible.
+
+- clock-names: The name of the reference clock.
+
+- clock-frequency: Desired frequency of the clock. This will set the serial
+ clock baud rate (SPBR) based on the reference clock frequency. The frequency
+ of the SPBR is mspi_clk / (2 * SPBR) where SPBR is a value between 1-255
+ determined by the desired 'clock-frequency'. If not provided then the default
+ baud rate of the controller is used.
+
+Example:
+
+mspi: mspi@18047200 {
+ compatible = brcm,mspi-v0;
+ reg = 0x18047200 0x188,
+ 0x180473a0 0x1c;
+ interrupts = GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH;
+ clocks = axi41_clk;
+ clock-names = mspi_clk;
+ clock-frequency = 1250;
+};
--
1.7.9.5
--
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 2/2] spi: bcm-mspi: Add support for Broadcom MSPI driver.
The MSPI controller is a SPI controller found on various Broadcom
SoC's such as Cygnus.
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/spi/Kconfig|5 +
drivers/spi/Makefile |1 +
drivers/spi/spi-bcm-mspi.c | 505
3 files changed, 511 insertions(+)
create mode 100644 drivers/spi/spi-bcm-mspi.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ab8dfbe..23622c2 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -121,6 +121,11 @@ config SPI_BCM53XX
help
Enable support for the SPI controller on Broadcom BCM53xx ARM SoCs.
+config SPI_BCM_MSPI
+ tristate Broadcom MSPI controller
+ help
+ Enable support for the Broadcom MSPI controller.
+
config SPI_BCM63XX
tristate Broadcom BCM63xx SPI controller
depends on BCM63XX
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index d8cbf65..5ebecba 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_SPI_BCM2835) += spi-bcm2835.o
obj-$(CONFIG_SPI_BCM53XX) += spi-bcm53xx.o
obj-$(CONFIG_SPI_BCM63XX) += spi-bcm63xx.o
obj-$(CONFIG_SPI_BCM63XX_HSSPI)+= spi-bcm63xx-hsspi.o
+obj-$(CONFIG_SPI_BCM_MSPI) += spi-bcm-mspi.o
obj-$(CONFIG_SPI_BFIN5XX) += spi-bfin5xx.o
obj-$(CONFIG_SPI_ADI_V3)+= spi-adi-v3.o
obj-$(CONFIG_SPI_BFIN_SPORT) += spi-bfin-sport.o
diff --git a/drivers/spi/spi-bcm-mspi.c b/drivers/spi/spi-bcm-mspi.c
new file mode 100644
index 000..9f6cc03
--- /dev/null
+++ b/drivers/spi/spi-bcm-mspi.c
@@ -0,0 +1,505 @@
+/*
+ * Copyright (C) 2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed as is WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/delay.h
+#include linux/spi/spi.h
+#include linux/of.h
+#include linux/clk.h
+#include linux/irq.h
+#include linux/interrupt.h
+#include linux/of_irq.h
+
+#define MSPI_SPCR0_LSB_OFFSET 0x0
+#define MSPI_SPCR0_LSB_SHIFT0
+#define SPBR_MIN8U
+#define SPBR_MAX255U
+#define MSPI_NEWQP_OFFSET 0x10
+#define MSPI_ENDQP_OFFSET 0x14
+#define MSPI_SPCR2_OFFSET 0x18
+#define MSPI_SPCR2_SPE_SHIFT6
+#define MSPI_SPCR2_SPIFIE_SHIFT 5
+#define MSPI_SPCR2_CONT_AFTER_CMD_SHIFT 7
+#define MSPI_STATUS_OFFSET 0x20
+#define MSPI_TXRAM_OFFSET 0x40
+#define MSPI_RXRAM_OFFSET 0xc0
+#define MSPI_CDRAM_OFFSET 0x140
+#define MSPI_CDRAM_CONT_SHIFT 7
+#define MSPI_CDRAM_BITSE_SHIFT 6
+#define MSPI_CDRAM_PCS_SHIFT0
+#define MSPI_WRITE_LOCK_OFFSET 0x180
+#define INTERRUPT_MSPI_DONE_OFFSET 0x14
+
+#define NUM_SLOTS 16
+
+struct bcm_mspi {
+ struct platform_device *pdev;
+ void __iomem*base;
+/* Base of interrupt control regs. */
+ void __iomem*int_base;
+ struct spi_master *master;
+ struct clk *clk;
+ u32 spbr;
+ struct completion xfer_done;
+ int rx_slot;
+};
+
+/*
+ * Calculate TXRAM offset for the requested slot.
+ */
+static inline u32 tx_offset(int slot)
+{
+ BUG_ON(slot NUM_SLOTS);
+ return 4 * (slot * 2);
+}
+
+/*
+ * Calculate RXRAM offset for the requested slot.
+ */
+static inline u32 rx_offset(int slot)
+{
+ BUG_ON(slot NUM_SLOTS);
+ return 4 * (1 + slot * 2);
+}
+
+/*
+ * Calculate CDRAM offset for the requested slot.
+ */
+static inline u32 cdram_offset(int slot)
+{
+ BUG_ON(slot NUM_SLOTS);
+ return MSPI_CDRAM_OFFSET + (4 * slot);
+}
+
+/*
+ * Start tx or rx SPI transfer and wait until complete. The queue start pointer
+ * is always 0. The end queue pointer is passed in to end_slot.
+ *
+ * @set_cont Set the CONT bit if true, clear if false. The CONT bit must be set
+ * if all slots are filled and there is more data to be transferred.
+ * @end_slot The end queue pointer.
+ * Returns 0 if successful, -EIO if transfer timed out.
+ */
+static int bcm_mspi_start_transfer(struct bcm_mspi *mspi, bool set_cont,
+ int end_slot)
+{
+ int val;
+ int err = 0;
+
+ /* Set queue pointers for transmit. */
+ writel(0, mspi-base + MSPI_NEWQP_OFFSET);
+ writel(end_slot, mspi-base
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
A bit more info is required here. Get timestamp: This is a bit more complicated. Currently the PTP driver does list management for timestamps from external timestamp channels. Timestamps from all channels go into the same list. In our driver we have a s/w FIFO for each client and it closely matches the h/w FIFO and handles any overflow. We would like to keep it this way because it also allows multiple user space processes to only fetch timestamps for the client it's handling. We could add a new ioctl to get a timestamp from the driver instead of doing it through ptp_read() but it would be nice if we could let ptp_read() allow the driver to do timestamp management instead of PTP. Maybe provide an option to obtain the timestamps from a container in the driver instead of the one managed by PTP. I like being able to use read/poll to obtain data instead of polling the kernel with ioctls as we are currently doing. Also, avoiding the kmalloc in ptp_read would be nice because this of the frequency it would be called at. Do you have any preference on how to handle this? I've tried to minimize the changes to PTP. Using read() we can't specify a channel to get the timestamp for. It would imply that one user space process would have to read timestamps for all channels/clients and then leave it up to user space IPC to get them to other processes, which isn't great. That means either we introduce an ioctl such as the DTE_GET_TIMESTAMP we had before which allows us to specify a channel, or we need to look at creating one dev node per external timestamping channel. The ioctl limitation is that it pounds the kernel polling for timestamps and the multiple dev nodes per channel is a big change to PTP. I will have to look further into this to really have a good idea of what the implications would be. Advice/ideas? Thanks. -- 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 v7 2/5] drivers: pwm: bcm-kona: Dont set polarity in probe
From: Arun Ramamurthy arunr...@broadcom.com
Omit setting the polarity to normal during probe and instead use the
new pwmchip_add_inversed function to register a PWM chip with default
polarity of inversed for all channels as this is the actual hardware
default.
Signed-off-by: Arun Ramamurthy arunr...@broadcom.com
Reviewed-by: Ray Jui r...@broadcom.com
Signed-off-by: Scott Branden sbran...@broadcom.com
Signed-off-by: Tim Kryger tim.kry...@gmail.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/pwm-bcm-kona.c |9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 02bc048..32b3ec6 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -266,18 +266,15 @@ static int kona_pwmc_probe(struct platform_device *pdev)
return ret;
}
- /* Set smooth mode, push/pull, and normal polarity for all channels */
- for (chan = 0; chan kp-chip.npwm; chan++) {
- value |= (1 PWM_CONTROL_SMOOTH_SHIFT(chan));
+ /* Set push/pull for all channels */
+ for (chan = 0; chan kp-chip.npwm; chan++)
value |= (1 PWM_CONTROL_TYPE_SHIFT(chan));
- value |= (1 PWM_CONTROL_POLARITY_SHIFT(chan));
- }
writel(value, kp-base + PWM_CONTROL_OFFSET);
clk_disable_unprepare(kp-clk);
- ret = pwmchip_add(kp-chip);
+ ret = pwmchip_add_inversed(kp-chip);
if (ret 0)
dev_err(pdev-dev, failed to add PWM chip: %d\n, ret);
--
1.7.9.5
--
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 v7 5/5] pwm: core: Set enable state properly on failed call to enable
The pwm_enable function didn't clear the enabled bit if a call to a
clients enable function returned an error. The result was that the state
of the pwm core was wrong. Clearing the bit when enable returns an error
ensures the state is properly set.
Tested-by: Jonathan Richardson jonat...@broadcom.com
Reviewed-by: Dmitry Torokhov dmitry.torok...@gmail.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/core.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 224645f..18f5ac4 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -477,10 +477,20 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
*/
int pwm_enable(struct pwm_device *pwm)
{
- if (pwm !test_and_set_bit(PWMF_ENABLED, pwm-flags))
- return pwm-chip-ops-enable(pwm-chip, pwm);
+ int err;
+
+ if (!pwm)
+ return -EINVAL;
+
+ if (!test_and_set_bit(PWMF_ENABLED, pwm-flags)) {
+ err = pwm-chip-ops-enable(pwm-chip, pwm);
+ if (err) {
+ clear_bit(PWMF_ENABLED, pwm-flags);
+ return err;
+ }
+ }
- return pwm ? 0 : -EINVAL;
+ return 0;
}
EXPORT_SYMBOL_GPL(pwm_enable);
--
1.7.9.5
--
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 v7 4/5] pwm: kona: Add debug info to config function
Adds debugging info to config function where duty cycle and period
are calculated and verified.
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/pwm-bcm-kona.c | 25 +++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 0654981..c6069c7 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -138,18 +138,39 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct
pwm_device *pwm,
dc = div64_u64(val, div);
/* If duty_ns or period_ns are not achievable then return */
- if (pc PERIOD_COUNT_MIN || dc DUTY_CYCLE_HIGH_MIN)
+ if (pc PERIOD_COUNT_MIN) {
+ dev_warn(chip-dev,
+ %s: pwm[%d]: period=%d is not achievable,
pc=%lu, prescale=%lu\n,
+ __func__, chan, period_ns, pc, prescale);
return -EINVAL;
+ }
+
+ if (dc DUTY_CYCLE_HIGH_MIN) {
+ if (0 != duty_ns) {
+ dev_warn(chip-dev,
+ %s: pwm[%d]: duty cycle=%d is not
achievable, dc=%lu, prescale=%lu\n,
+ __func__, chan, duty_ns, dc, prescale);
+ }
+ return -EINVAL;
+ }
/* If pc and dc are in bounds, the calculation is done */
if (pc = PERIOD_COUNT_MAX dc = DUTY_CYCLE_HIGH_MAX)
break;
/* Otherwise, increase prescale and recalculate pc and dc */
- if (++prescale PRESCALE_MAX)
+ if (++prescale PRESCALE_MAX) {
+ dev_warn(chip-dev,
+ %s: pwm[%d]: Prescale (=%lu) within max (=%d)
for period=%d and duty cycle=%d is not achievable\n,
+ __func__, chan, prescale, PRESCALE_MAX,
+ period_ns, duty_ns);
return -EINVAL;
+ }
}
+ dev_dbg(chip-dev, pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n,
+ chan, pc, dc, prescale);
+
/*
* Don't apply settings if disabled. The period and duty cycle are
* always calculated above to ensure the new values are
--
1.7.9.5
--
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 v7 1/5] drivers: pwm: core: Add pwmchip_add_inversed
From: Tim Kryger tim.kry...@gmail.com
Add a new function to register a PWM chip with channels that have their
initial polarity as inversed. This benefits drivers of controllers that
by default operate with inversed polarity by removing the need to modify
the polarity during initialization.
Signed-off-by: Tim Kryger tim.kry...@gmail.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/core.c | 36
include/linux/pwm.h |6 ++
2 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index ba34c7d..224645f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -222,14 +222,8 @@ void *pwm_get_chip_data(struct pwm_device *pwm)
}
EXPORT_SYMBOL_GPL(pwm_get_chip_data);
-/**
- * pwmchip_add() - register a new PWM chip
- * @chip: the PWM chip to add
- *
- * Register a new PWM chip. If chip-base 0 then a dynamically assigned base
- * will be used.
- */
-int pwmchip_add(struct pwm_chip *chip)
+static int pwmchip_add_with_polarity(struct pwm_chip *chip,
+enum pwm_polarity polarity)
{
struct pwm_device *pwm;
unsigned int i;
@@ -259,6 +253,7 @@ int pwmchip_add(struct pwm_chip *chip)
pwm-chip = chip;
pwm-pwm = chip-base + i;
pwm-hwpwm = i;
+ pwm-polarity = polarity;
radix_tree_insert(pwm_tree, pwm-pwm, pwm);
}
@@ -279,9 +274,34 @@ out:
mutex_unlock(pwm_lock);
return ret;
}
+
+/**
+ * pwmchip_add() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip-base 0 then a dynamically assigned base
+ * will be used. The initial polarity for all channels is normal.
+ */
+int pwmchip_add(struct pwm_chip *chip)
+{
+ return pwmchip_add_with_polarity(chip, PWM_POLARITY_NORMAL);
+}
EXPORT_SYMBOL_GPL(pwmchip_add);
/**
+ * pwmchip_add_inversed() - register a new PWM chip
+ * @chip: the PWM chip to add
+ *
+ * Register a new PWM chip. If chip-base 0 then a dynamically assigned base
+ * will be used. The initial polarity for all channels is inversed.
+ */
+int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+ return pwmchip_add_with_polarity(chip, PWM_POLARITY_INVERSED);
+}
+EXPORT_SYMBOL_GPL(pwmchip_add_inversed);
+
+/**
* pwmchip_remove() - remove a PWM chip
* @chip: the PWM chip to remove
*
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e90628c..358547f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -183,6 +183,7 @@ int pwm_set_chip_data(struct pwm_device *pwm, void *data);
void *pwm_get_chip_data(struct pwm_device *pwm);
int pwmchip_add(struct pwm_chip *chip);
+int pwmchip_add_inversed(struct pwm_chip *chip);
int pwmchip_remove(struct pwm_chip *chip);
struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
unsigned int index,
@@ -217,6 +218,11 @@ static inline int pwmchip_add(struct pwm_chip *chip)
return -EINVAL;
}
+static inline int pwmchip_add_inversed(struct pwm_chip *chip)
+{
+ return -EINVAL;
+}
+
static inline int pwmchip_remove(struct pwm_chip *chip)
{
return -EINVAL;
--
1.7.9.5
--
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 v7 0/5] Fix bugs in kona pwm driver and pwm core
This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also fixes a bug in the pwm core where the enabled state was incorrect on failed calls to enable. Also, a new function was added to the pwm core to add pwm chips with inversed polarity for chips that have a different default polarity than the core. The prevents incorrect polarity being reported. Debug info has been added to help debug configuring duty cycle and period. These changes are required for the Kona pwm driver to work on Cygnus and the same PWM IP is being used. Changes from v6: - Move new debugging info for duty cycle and period in config function into its own commit. - Add kona_pwmc_prepare_for_settings() function to remove duplicated code. Changes from v5: - Address Dmitry's comment on code cleanup of pwm_enable() in pwm core. - Including all patches from different contributors in this patchset. Some were left out in v4. Changes from v4: - Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity. - Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework. Arun Ramamurthy (1): drivers: pwm: bcm-kona: Dont set polarity in probe Jonathan Richardson (3): pwm: kona: Fix incorrect config, disable, and polarity procedures pwm: kona: Add debug info to config function pwm: core: Set enable state properly on failed call to enable Tim Kryger (1): drivers: pwm: core: Add pwmchip_add_inversed drivers/pwm/core.c | 52 ++-- drivers/pwm/pwm-bcm-kona.c | 81 +--- include/linux/pwm.h|6 3 files changed, 109 insertions(+), 30 deletions(-) -- 1.7.9.5 -- 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 v7 3/5] pwm: kona: Fix incorrect config, disable, and polarity procedures
The config procedure didn't follow the spec which periodically resulted
in failing to enable the output signal. This happened one in ten or
twenty attempts. Following the spec and adding a 400ns delay in the
appropriate locations resolves this problem.
The disable procedure now also follows the spec. The old disable
procedure would result in no change in signal when called.
The polarity procedure no longer applies the settings to change the
output signal because it can't be called when the pwm is enabled anyway.
The polarity is only updated in the control register. The correct
polarity will be applied on enable. The old method of applying changes
would result in no signal when the polarity was changed. The new
apply_settings function would fix this problem but it isn't required
anyway.
Reviewed-by: Arun Ramamurthy arunr...@broadcom.com
Reviewed-by: Scott Branden sbran...@broadcom.com
Tested-by: Scott Branden sbran...@broadcom.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
---
drivers/pwm/pwm-bcm-kona.c | 47 +---
1 file changed, 36 insertions(+), 11 deletions(-)
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 32b3ec6..0654981 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -76,19 +76,36 @@ static inline struct kona_pwmc *to_kona_pwmc(struct
pwm_chip *_chip)
return container_of(_chip, struct kona_pwmc, chip);
}
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+/*
+ * Clear trigger bit but set smooth bit to maintain old output.
+ */
+static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
+ unsigned int chan)
{
unsigned int value = readl(kp-base + PWM_CONTROL_OFFSET);
- /* Clear trigger bit but set smooth bit to maintain old output */
value |= 1 PWM_CONTROL_SMOOTH_SHIFT(chan);
value = ~(1 PWM_CONTROL_TRIGGER_SHIFT(chan));
writel(value, kp-base + PWM_CONTROL_OFFSET);
+ /*
+* There must be a min 400ns delay between clearing enable and setting
+* it. Failing to do this may result in no PWM signal.
+*/
+ ndelay(400);
+}
+
+static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+{
+ unsigned int value = readl(kp-base + PWM_CONTROL_OFFSET);
+
/* Set trigger bit and clear smooth bit to apply new settings */
value = ~(1 PWM_CONTROL_SMOOTH_SHIFT(chan));
value |= 1 PWM_CONTROL_TRIGGER_SHIFT(chan);
writel(value, kp-base + PWM_CONTROL_OFFSET);
+
+ /* PWMOUT_ENABLE must be held high for at least 400 ns. */
+ ndelay(400);
}
static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -133,8 +150,14 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct
pwm_device *pwm,
return -EINVAL;
}
- /* If the PWM channel is enabled, write the settings to the HW */
+ /*
+* Don't apply settings if disabled. The period and duty cycle are
+* always calculated above to ensure the new values are
+* validated immediately instead of on enable.
+*/
if (test_bit(PWMF_ENABLED, pwm-flags)) {
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp-base + PRESCALE_OFFSET);
value = ~PRESCALE_MASK(chan);
value |= prescale PRESCALE_SHIFT(chan);
@@ -173,11 +196,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip,
struct pwm_device *pwm,
writel(value, kp-base + PWM_CONTROL_OFFSET);
- kona_pwmc_apply_settings(kp, chan);
-
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
-
clk_disable_unprepare(kp-clk);
return 0;
@@ -207,13 +225,20 @@ static void kona_pwmc_disable(struct pwm_chip *chip,
struct pwm_device *pwm)
{
struct kona_pwmc *kp = to_kona_pwmc(chip);
unsigned int chan = pwm-hwpwm;
+ unsigned int value;
+
+ kona_pwmc_prepare_for_settings(kp, chan);
/* Simulate a disable by configuring for zero duty */
writel(0, kp-base + DUTY_CYCLE_HIGH_OFFSET(chan));
- kona_pwmc_apply_settings(kp, chan);
+ writel(0, kp-base + PERIOD_COUNT_OFFSET(chan));
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
+ /* Set prescale to 0 for this channel */
+ value = readl(kp-base + PRESCALE_OFFSET);
+ value = ~PRESCALE_MASK(chan);
+ writel(value, kp-base + PRESCALE_OFFSET);
+
+ kona_pwmc_apply_settings(kp, chan);
clk_disable_unprepare(kp-clk);
}
--
1.7.9.5
--
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 v6 3/4] pwm: kona: Fix incorrect config, disable, and polarity procedures
Tim, just one comment below. All other suggestions will be made for the
next patch set.
On 15-05-06 09:26 PM, Tim Kryger wrote:
> On Fri, Apr 10, 2015 at 11:58 AM, Jonathan Richardson
> wrote:
>
>> The polarity procedure no longer applies the settings to change the
>> output signal because it can't be called when the pwm is enabled anyway.
>> The polarity is only updated in the control register. The correct
>> polarity will be applied on enable. The old method of applying changes
>> would result in no signal when the polarity was changed. The new
>> apply_settings function would fix this problem but it isn't required
>> anyway.
>
> Why does this bug fix need to alter when polarity changes take effect?
The original kona_pwmc_set_polarity() function didn't work on Cygnus
because of the updated kona_pwmc_apply_settings() that we fixed for to
use the updated method received from the chip team. In the process of
fixing it I just took out the apply settings completely because the
polarity can only be changed when the pwm is disabled (from core.c) and
it seemed simpler to just write the polarity to the config register and
let the enable procedure handle the application of polarity when the
signal was actually enabled. I didn't see the point of trying to apply
the polarity when the output signal is disabled. It's the same as
writing the duty cycle and period when the pwm is disabled - the
settings are stored but don't take effect until enabled. I didn't see
any change in signal behaviour by simplifying the polarity function.
>
> That is an an unrelated change and I don't really agree with it.
>
>> /* If duty_ns or period_ns are not achievable then return */
>> - if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
>> + if (pc < PERIOD_COUNT_MIN) {
>> + dev_warn(chip->dev,
>> + "%s: pwm[%d]: period=%d is not achievable,
>> pc=%lu, prescale=%lu\n",
>> + __func__, chan, period_ns, pc, prescale);
>> + return -EINVAL;
>> + }
>> +
>> + /* If duty_ns is not achievable then return */
>> + if (dc < DUTY_CYCLE_HIGH_MIN) {
>> + if (0 != duty_ns) {
>> + dev_warn(chip->dev,
>> + "%s: pwm[%d]: duty cycle=%d is not
>> achievable, dc=%lu, prescale=%lu\n",
>> + __func__, chan, duty_ns, dc,
>> prescale);
>> + }
>> return -EINVAL;
>> + }
>>
>> /* If pc and dc are in bounds, the calculation is done */
>> if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
>> break;
>>
>> /* Otherwise, increase prescale and recalculate pc and dc */
>> - if (++prescale > PRESCALE_MAX)
>> + if (++prescale > PRESCALE_MAX) {
>> + dev_warn(chip->dev,
>> + "%s: pwm[%d]: Prescale (=%lu) within max
>> (=%d) for period=%d and duty cycle=%d is not achievable\n",
>> + __func__, chan, prescale, PRESCALE_MAX,
>> + period_ns, duty_ns);
>> return -EINVAL;
>> + }
>> }
>
> This extra debug output seems helpful but could you put it in its own
> commit and keep this focused on fixing the instability you observed?
>
>> + /*
>> +* Clear trigger bit but set smooth bit to maintain old
>> +* output.
>> +*/
>> + value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
>> + value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
>> + writel(value, kp->base + PWM_CONTROL_OFFSET);
>> +
>
> The above lines are duplicated below. Perhaps a function is in order?
>
>> + /* Set smooth type to 1 and disable */
>> + value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan);
>> + value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
>> + writel(value, kp->base + PWM_CONTROL_OFFSET);
>
> It seems like the important parts of the fix could be distilled down
> to something like this:
>
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index 02bc048..4ff500c 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm
Re: [PATCH v6 3/4] pwm: kona: Fix incorrect config, disable, and polarity procedures
Tim, just one comment below. All other suggestions will be made for the
next patch set.
On 15-05-06 09:26 PM, Tim Kryger wrote:
On Fri, Apr 10, 2015 at 11:58 AM, Jonathan Richardson
jonat...@broadcom.com wrote:
The polarity procedure no longer applies the settings to change the
output signal because it can't be called when the pwm is enabled anyway.
The polarity is only updated in the control register. The correct
polarity will be applied on enable. The old method of applying changes
would result in no signal when the polarity was changed. The new
apply_settings function would fix this problem but it isn't required
anyway.
Why does this bug fix need to alter when polarity changes take effect?
The original kona_pwmc_set_polarity() function didn't work on Cygnus
because of the updated kona_pwmc_apply_settings() that we fixed for to
use the updated method received from the chip team. In the process of
fixing it I just took out the apply settings completely because the
polarity can only be changed when the pwm is disabled (from core.c) and
it seemed simpler to just write the polarity to the config register and
let the enable procedure handle the application of polarity when the
signal was actually enabled. I didn't see the point of trying to apply
the polarity when the output signal is disabled. It's the same as
writing the duty cycle and period when the pwm is disabled - the
settings are stored but don't take effect until enabled. I didn't see
any change in signal behaviour by simplifying the polarity function.
That is an an unrelated change and I don't really agree with it.
/* If duty_ns or period_ns are not achievable then return */
- if (pc PERIOD_COUNT_MIN || dc DUTY_CYCLE_HIGH_MIN)
+ if (pc PERIOD_COUNT_MIN) {
+ dev_warn(chip-dev,
+ %s: pwm[%d]: period=%d is not achievable,
pc=%lu, prescale=%lu\n,
+ __func__, chan, period_ns, pc, prescale);
+ return -EINVAL;
+ }
+
+ /* If duty_ns is not achievable then return */
+ if (dc DUTY_CYCLE_HIGH_MIN) {
+ if (0 != duty_ns) {
+ dev_warn(chip-dev,
+ %s: pwm[%d]: duty cycle=%d is not
achievable, dc=%lu, prescale=%lu\n,
+ __func__, chan, duty_ns, dc,
prescale);
+ }
return -EINVAL;
+ }
/* If pc and dc are in bounds, the calculation is done */
if (pc = PERIOD_COUNT_MAX dc = DUTY_CYCLE_HIGH_MAX)
break;
/* Otherwise, increase prescale and recalculate pc and dc */
- if (++prescale PRESCALE_MAX)
+ if (++prescale PRESCALE_MAX) {
+ dev_warn(chip-dev,
+ %s: pwm[%d]: Prescale (=%lu) within max
(=%d) for period=%d and duty cycle=%d is not achievable\n,
+ __func__, chan, prescale, PRESCALE_MAX,
+ period_ns, duty_ns);
return -EINVAL;
+ }
}
This extra debug output seems helpful but could you put it in its own
commit and keep this focused on fixing the instability you observed?
+ /*
+* Clear trigger bit but set smooth bit to maintain old
+* output.
+*/
+ value |= 1 PWM_CONTROL_SMOOTH_SHIFT(chan);
+ value = ~(1 PWM_CONTROL_TRIGGER_SHIFT(chan));
+ writel(value, kp-base + PWM_CONTROL_OFFSET);
+
The above lines are duplicated below. Perhaps a function is in order?
+ /* Set smooth type to 1 and disable */
+ value |= 1 PWM_CONTROL_SMOOTH_SHIFT(chan);
+ value = ~(1 PWM_CONTROL_TRIGGER_SHIFT(chan));
+ writel(value, kp-base + PWM_CONTROL_OFFSET);
It seems like the important parts of the fix could be distilled down
to something like this:
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 02bc048..4ff500c 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -76,7 +76,8 @@ static inline struct kona_pwmc *to_kona_pwmc(struct
pwm_chip *_chip)
return container_of(_chip, struct kona_pwmc, chip);
}
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
+static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
+ unsigned int chan)
{
unsigned int value = readl(kp-base + PWM_CONTROL_OFFSET);
@@ -85,10 +86,19 @@ static void kona_pwmc_apply_settings(struct
kona_pwmc *kp, unsigned int chan)
value = ~(1 PWM_CONTROL_TRIGGER_SHIFT(chan));
writel(value, kp-base + PWM_CONTROL_OFFSET
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
On 15-05-01 12:40 PM, Arnd Bergmann wrote: > On Friday 01 May 2015 20:30:04 One Thousand Gnomes wrote: >> >>> channel control, unless I'm missing something. If people are flexible >>> with extending that I could propose something. Let me know which way you >>> prefer to go. Thanks. >> >> I would strongly favour fixing PTP to do this right. Otherwise we will >> have 2 or 3 adhoc drivers, then end up moving them to PTP and then end up >> dealing with the compat mess. > > Agreed. I was hoping that there was already a subsystem in the kernel that > could be extended to work with this driver. If all that is needed is to > pass more fields of the existing timex to ptp drivers, that should be > fairly easy to do. > > We also have some padding bytes available in struct timex in case some > extra data needs to be passed, or we could add another clock_* system call > if it's absolutely required to have another entry into the driver, and > connect that through struct k_clock and posix_clock_operations(). I would > hope we don't even need that. > > Arnd > Thanks Arnd/Alan for the feedback. I looked into PTP further to see what exactly would need changing. For the clock functions I think we can use the existing framework unchanged with one exception: ptp_clock_adjtime() doesn't allow negative time adjustments and we would like to allow this. The other client events (set IRQ interval, enable client, set client divider, get client timestamp) could be handled as follows: IRQ interval: I mentioned before that we may be able to calculate the isochronous interrupt rate automatically but this isn't possible because the DTE doesn't know the frequency of the clients. One solution is to use the 'PTP_PEROUT_REQUEST' ioctl to set a periodic timer frequency. Not really a timer but good enough for our purposes. Enable Client: The external timestamping channel handling in PTP allows channels to be enabled so we can re-use this (PTP_EXTTS_REQUEST ioctl). Set divider: There is no ability to set a frequency or do anything to a channel. We could re-use the PTP_EXTTS_REQUEST ioctl but extend 'struct ptp_extts_request' by using the reserved field rsv to allow setting an integer value representing either a frequency or divider value in our case - some value to configure a channel. A new flag would have to be added to the existing PTP_ENABLE_FEATURE, RISING and FALLING EDGE. Get timestamp: This is a bit more complicated. Currently the PTP driver does list management for timestamps from external timestamp channels. Timestamps from all channels go into the same list. In our driver we have a s/w FIFO for each client and it closely matches the h/w FIFO and handles any overflow. We would like to keep it this way because it also allows multiple user space processes to only fetch timestamps for the client it's handling. We could add a new ioctl to get a timestamp from the driver instead of doing it through ptp_read() but it would be nice if we could let ptp_read() allow the driver to do timestamp management instead of PTP. Maybe provide an option to obtain the timestamps from a container in the driver instead of the one managed by PTP. I like being able to use read/poll to obtain data instead of polling the kernel with ioctls as we are currently doing. Also, avoiding the kmalloc in ptp_read would be nice because this of the frequency it would be called at. Do you have any preference on how to handle this? I've tried to minimize the changes to PTP. Alan, what is IIO? I'm not familiar. Thanks. -- 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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
On 15-05-01 12:40 PM, Arnd Bergmann wrote: On Friday 01 May 2015 20:30:04 One Thousand Gnomes wrote: channel control, unless I'm missing something. If people are flexible with extending that I could propose something. Let me know which way you prefer to go. Thanks. I would strongly favour fixing PTP to do this right. Otherwise we will have 2 or 3 adhoc drivers, then end up moving them to PTP and then end up dealing with the compat mess. Agreed. I was hoping that there was already a subsystem in the kernel that could be extended to work with this driver. If all that is needed is to pass more fields of the existing timex to ptp drivers, that should be fairly easy to do. We also have some padding bytes available in struct timex in case some extra data needs to be passed, or we could add another clock_* system call if it's absolutely required to have another entry into the driver, and connect that through struct k_clock and posix_clock_operations(). I would hope we don't even need that. Arnd Thanks Arnd/Alan for the feedback. I looked into PTP further to see what exactly would need changing. For the clock functions I think we can use the existing framework unchanged with one exception: ptp_clock_adjtime() doesn't allow negative time adjustments and we would like to allow this. The other client events (set IRQ interval, enable client, set client divider, get client timestamp) could be handled as follows: IRQ interval: I mentioned before that we may be able to calculate the isochronous interrupt rate automatically but this isn't possible because the DTE doesn't know the frequency of the clients. One solution is to use the 'PTP_PEROUT_REQUEST' ioctl to set a periodic timer frequency. Not really a timer but good enough for our purposes. Enable Client: The external timestamping channel handling in PTP allows channels to be enabled so we can re-use this (PTP_EXTTS_REQUEST ioctl). Set divider: There is no ability to set a frequency or do anything to a channel. We could re-use the PTP_EXTTS_REQUEST ioctl but extend 'struct ptp_extts_request' by using the reserved field rsv to allow setting an integer value representing either a frequency or divider value in our case - some value to configure a channel. A new flag would have to be added to the existing PTP_ENABLE_FEATURE, RISING and FALLING EDGE. Get timestamp: This is a bit more complicated. Currently the PTP driver does list management for timestamps from external timestamp channels. Timestamps from all channels go into the same list. In our driver we have a s/w FIFO for each client and it closely matches the h/w FIFO and handles any overflow. We would like to keep it this way because it also allows multiple user space processes to only fetch timestamps for the client it's handling. We could add a new ioctl to get a timestamp from the driver instead of doing it through ptp_read() but it would be nice if we could let ptp_read() allow the driver to do timestamp management instead of PTP. Maybe provide an option to obtain the timestamps from a container in the driver instead of the one managed by PTP. I like being able to use read/poll to obtain data instead of polling the kernel with ioctls as we are currently doing. Also, avoiding the kmalloc in ptp_read would be nice because this of the frequency it would be called at. Do you have any preference on how to handle this? I've tried to minimize the changes to PTP. Alan, what is IIO? I'm not familiar. Thanks. -- 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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Arnd, some more info is provided and also some questions before I can
proceed.
On 15-04-23 01:04 AM, Arnd Bergmann wrote:
> On Wednesday 22 April 2015 16:22:03 Jonathan Richardson wrote:
>> Reviewed-by: Scott Branden
>> Tested-by: Scott Branden
>> Signed-off-by: Jonathan Richardson
>
> No description at all?
The DTE creates timestamps of hardware based events such as GPIO, I2S
signals for audio, etc. It was also intended to provide 802.1AS / PTP
timestamps for network packets. The h/w has up to 32 "clients" -- the
hardware inputs into a timestamping engine. These clients are specific
to the chip the DTE is used on. For Cygnus you can see what they are in
our 'enum dte_client' from bcm_cygnus_dte.h.
The DTE timestamper creates timestamps based on the current clock wall
time. When an event occurs it stores the timestamp in a h/w FIFO. Each
client also has a divider that can be set to control the rate that
timestamps are generated at by the timestamper and these are adjustable
at run time.
The isochronous interrupt is used to pull the timestamps out of the h/w
FIFO and store them in a s/w FIFO until they are pulled out of that from
user space.
The API we provide in the driver enables clients (inputs into the
timestamper) using dte_enable_timestamp(). The clients divider is set
with dte_set_client_divider(). The isochronous interrupt frequency - the
rate at which the ISR fires to pull timestamps from h/w to s/w FIFO is
dte_set_irq_interval().
Timestamps generated by the timestamper for clients are polled for using
dte_get_timestamp().
The clock is controlled with the remaining functions: dte_set_time(),
dte_get_time(), dte_adj_time(), dte_adj_freq(). These functions provide
the time base for the timestamper but also provide a clock that was
meant to be used later by an Ethernet driver for PTP.
The ioctls correspond to all of these functions for the user space API.
The idea for future PTP support was to use the dte_get_time() to get
timestamps for the packets. This may make the driver more suitable to be
PTP driver.
It's a bit more than a PTP hardware clock on a NIC. It's a clock for PTP
plus timestamping 32 other hardware inputs that can be enabled at any
time with timestamps being generated at varying frequencies. As clients
are enabled that generate timestamps at higher frequencies, the
isochronous interrupt frequency needs to be increased so that overflows
in the the h/w and s/w FIFO's don't occur (this frequency could possibly
be automated instead of changing it manually as we currently do).
>
> You are introducing a new subsystem here, which means you get to put
> a lot of thought into the API design, to ensure that it works with
> other drivers of the same type, and that we don't already have
> a subsystem that does what you need here.
>
> Please write a few pages of text about the tradeoffs that went into
> the internal and user-facing API design, and explain the differences
> to the existing infrastructure we have for the clocksource, clockevent,
> k_clock, posix timers, posix timers, timerfd, rtc, and ptp frameworks,
> in particular why your hardware cannot fit into the existing frameworks
> and has to have a new one.
>
>> +struct bcm_cygnus_dte *dte_get_dev_from_devname(const char *devname)
>> +{
>> +struct bcm_cygnus_dte *cygnus_dte = NULL;
>> +bool found = false;
>> +
>> +if (!devname)
>> +return NULL;
>> +
>> +list_for_each_entry(cygnus_dte, _list, node) {
>> +if (!strcmp(dev_name(_dte->pdev->dev), devname)) {
>> +/* Matched on device name */
>> +found = true;
>> +break;
>> +}
>> +}
>> +
>> +return found ? cygnus_dte : NULL;
>> +}
>> +EXPORT_SYMBOL(dte_get_dev_from_devname);
>
> No, don't match on a device name. If you must have a reference, use
> a phandle in DT.
>
>> +int dte_get_timestamp(
>> +struct bcm_cygnus_dte *cygnus_dte,
>> +enum dte_client client,
>> +struct timespec *ts)
>
> Please use timespec64 or ktime_t for internal interfaces.
>
>> +case DTE_IOCTL_SET_DIVIDER:
>
> For the IOCTLs, please write a documentation in man-page form. No need
> for troff formatting, plain text is fine.
Sure, where should this man page live in the kernel?
>
>> +/**
>> + * DTE Client
>> + */
>> +enum dte_client {
>> +DTE_CLIENT_MIN = 0,
>> +DTE_CLIENT_I2S0_BITCLOCK = 0,
>> +DTE_CLIENT_I2S1_BITCLOCK,
>> +DTE_CLIENT_I2S2_BITCLOCK,
>> +DTE_CLIENT_I2S0_WORDCLOCK,
>> +DTE_CLIENT_I2S1_WORDCLOCK,
>> +DTE_CLIENT_I2S2_WORDCLOCK,
>> +DTE_CLIENT_LCD_CLFP,
>> +DTE_CLIENT
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Arnd, some more info is provided and also some questions before I can
proceed.
On 15-04-23 01:04 AM, Arnd Bergmann wrote:
On Wednesday 22 April 2015 16:22:03 Jonathan Richardson wrote:
Reviewed-by: Scott Branden sbran...@broadcom.com
Tested-by: Scott Branden sbran...@broadcom.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
No description at all?
The DTE creates timestamps of hardware based events such as GPIO, I2S
signals for audio, etc. It was also intended to provide 802.1AS / PTP
timestamps for network packets. The h/w has up to 32 clients -- the
hardware inputs into a timestamping engine. These clients are specific
to the chip the DTE is used on. For Cygnus you can see what they are in
our 'enum dte_client' from bcm_cygnus_dte.h.
The DTE timestamper creates timestamps based on the current clock wall
time. When an event occurs it stores the timestamp in a h/w FIFO. Each
client also has a divider that can be set to control the rate that
timestamps are generated at by the timestamper and these are adjustable
at run time.
The isochronous interrupt is used to pull the timestamps out of the h/w
FIFO and store them in a s/w FIFO until they are pulled out of that from
user space.
The API we provide in the driver enables clients (inputs into the
timestamper) using dte_enable_timestamp(). The clients divider is set
with dte_set_client_divider(). The isochronous interrupt frequency - the
rate at which the ISR fires to pull timestamps from h/w to s/w FIFO is
dte_set_irq_interval().
Timestamps generated by the timestamper for clients are polled for using
dte_get_timestamp().
The clock is controlled with the remaining functions: dte_set_time(),
dte_get_time(), dte_adj_time(), dte_adj_freq(). These functions provide
the time base for the timestamper but also provide a clock that was
meant to be used later by an Ethernet driver for PTP.
The ioctls correspond to all of these functions for the user space API.
The idea for future PTP support was to use the dte_get_time() to get
timestamps for the packets. This may make the driver more suitable to be
PTP driver.
It's a bit more than a PTP hardware clock on a NIC. It's a clock for PTP
plus timestamping 32 other hardware inputs that can be enabled at any
time with timestamps being generated at varying frequencies. As clients
are enabled that generate timestamps at higher frequencies, the
isochronous interrupt frequency needs to be increased so that overflows
in the the h/w and s/w FIFO's don't occur (this frequency could possibly
be automated instead of changing it manually as we currently do).
You are introducing a new subsystem here, which means you get to put
a lot of thought into the API design, to ensure that it works with
other drivers of the same type, and that we don't already have
a subsystem that does what you need here.
Please write a few pages of text about the tradeoffs that went into
the internal and user-facing API design, and explain the differences
to the existing infrastructure we have for the clocksource, clockevent,
k_clock, posix timers, posix timers, timerfd, rtc, and ptp frameworks,
in particular why your hardware cannot fit into the existing frameworks
and has to have a new one.
+struct bcm_cygnus_dte *dte_get_dev_from_devname(const char *devname)
+{
+struct bcm_cygnus_dte *cygnus_dte = NULL;
+bool found = false;
+
+if (!devname)
+return NULL;
+
+list_for_each_entry(cygnus_dte, dtedev_list, node) {
+if (!strcmp(dev_name(cygnus_dte-pdev-dev), devname)) {
+/* Matched on device name */
+found = true;
+break;
+}
+}
+
+return found ? cygnus_dte : NULL;
+}
+EXPORT_SYMBOL(dte_get_dev_from_devname);
No, don't match on a device name. If you must have a reference, use
a phandle in DT.
+int dte_get_timestamp(
+struct bcm_cygnus_dte *cygnus_dte,
+enum dte_client client,
+struct timespec *ts)
Please use timespec64 or ktime_t for internal interfaces.
+case DTE_IOCTL_SET_DIVIDER:
For the IOCTLs, please write a documentation in man-page form. No need
for troff formatting, plain text is fine.
Sure, where should this man page live in the kernel?
+/**
+ * DTE Client
+ */
+enum dte_client {
+DTE_CLIENT_MIN = 0,
+DTE_CLIENT_I2S0_BITCLOCK = 0,
+DTE_CLIENT_I2S1_BITCLOCK,
+DTE_CLIENT_I2S2_BITCLOCK,
+DTE_CLIENT_I2S0_WORDCLOCK,
+DTE_CLIENT_I2S1_WORDCLOCK,
+DTE_CLIENT_I2S2_WORDCLOCK,
+DTE_CLIENT_LCD_CLFP,
+DTE_CLIENT_LCD_CLLP,
+DTE_CLIENT_GPIO14,
+DTE_CLIENT_GPIO15,
+DTE_CLIENT_GPIO22,
+DTE_CLIENT_GPIO23,
+DTE_CLIENT_MAX,
+};
Make this more abstract, so we can reuse the API for other vendors.
+#define DTE_IOCTL_BASE 'd'
+#define DTE_IO(nr) _IO(DTE_IOCTL_BASE, nr)
+#define DTE_IOR(nr, type) _IOR(DTE_IOCTL_BASE, nr, type
Re: [PATCH v6 0/4] Fix bugs in kona pwm driver and pwm core
Pinging maintainer and others. Could you please let me know when you can provide feedback on this patchset. Tim, were you ok with these changes now or do you still have concerns about changing the driver? I didn't hear back from after our last conversation. If there are still concerns we could possibly make a new cygnus pwm driver, though it would be mostly the same as the existing kona driver. Or add a DT version to the existing driver that has the new modified behaviour - though I think this would be messy. Thanks. On 15-04-10 11:58 AM, Jonathan Richardson wrote: > This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also > fixes a bug in the pwm core where the enabled state was incorrect on failed > calls to enable. Also, a new function was added to the pwm core to add pwm > chips > with inversed polarity for chips that have a different default polarity than > the > core. The prevents incorrect polarity being reported. > > These changes are required for the Kona pwm driver to work on Cygnus and the > same pwm IP is being used. > > Changes from v5: > - Address Dmitry's comment on code cleanup of pwm_enable() in pwm core. > - Including all patches from different contributors in this patchset. Some > were left out in v4. > > Changes from v4: > - Rebased to Tim Kryger's patch that adds support in pwm core to add driver > with inversed polarity. > - Removed patch 2 that resolved difference between hardware default polarity > and pwm framework default polarity. The default polarity is set properly > now > when the pwm driver is registered with the pwm framework. > > Arun Ramamurthy (1): > drivers: pwm: bcm-kona: Dont set polarity in probe > > Jonathan Richardson (2): > pwm: kona: Fix incorrect config, disable, and polarity procedures > pwm: core: Set enable state properly on failed call to enable > > Tim Kryger (1): > drivers: pwm: core: Add pwmchip_add_inversed > > drivers/pwm/core.c | 52 +-- > drivers/pwm/pwm-bcm-kona.c | 84 > +--- > include/linux/pwm.h|6 > 3 files changed, 110 insertions(+), 32 deletions(-) > -- 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 v6 0/4] Fix bugs in kona pwm driver and pwm core
Pinging maintainer and others. Could you please let me know when you can provide feedback on this patchset. Tim, were you ok with these changes now or do you still have concerns about changing the driver? I didn't hear back from after our last conversation. If there are still concerns we could possibly make a new cygnus pwm driver, though it would be mostly the same as the existing kona driver. Or add a DT version to the existing driver that has the new modified behaviour - though I think this would be messy. Thanks. On 15-04-10 11:58 AM, Jonathan Richardson wrote: This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also fixes a bug in the pwm core where the enabled state was incorrect on failed calls to enable. Also, a new function was added to the pwm core to add pwm chips with inversed polarity for chips that have a different default polarity than the core. The prevents incorrect polarity being reported. These changes are required for the Kona pwm driver to work on Cygnus and the same pwm IP is being used. Changes from v5: - Address Dmitry's comment on code cleanup of pwm_enable() in pwm core. - Including all patches from different contributors in this patchset. Some were left out in v4. Changes from v4: - Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity. - Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework. Arun Ramamurthy (1): drivers: pwm: bcm-kona: Dont set polarity in probe Jonathan Richardson (2): pwm: kona: Fix incorrect config, disable, and polarity procedures pwm: core: Set enable state properly on failed call to enable Tim Kryger (1): drivers: pwm: core: Add pwmchip_add_inversed drivers/pwm/core.c | 52 +-- drivers/pwm/pwm-bcm-kona.c | 84 +--- include/linux/pwm.h|6 3 files changed, 110 insertions(+), 32 deletions(-) -- 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/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Hi Arnd,
Thanks for the initial feedback. I'll investigate your suggestions and
get back to you if I have any questions before making some of the API
changes you've suggested.
On 15-04-23 01:04 AM, Arnd Bergmann wrote:
> On Wednesday 22 April 2015 16:22:03 Jonathan Richardson wrote:
>> Reviewed-by: Scott Branden
>> Tested-by: Scott Branden
>> Signed-off-by: Jonathan Richardson
>
> No description at all?
>
> You are introducing a new subsystem here, which means you get to put
> a lot of thought into the API design, to ensure that it works with
> other drivers of the same type, and that we don't already have
> a subsystem that does what you need here.
>
> Please write a few pages of text about the tradeoffs that went into
> the internal and user-facing API design, and explain the differences
> to the existing infrastructure we have for the clocksource, clockevent,
> k_clock, posix timers, posix timers, timerfd, rtc, and ptp frameworks,
> in particular why your hardware cannot fit into the existing frameworks
> and has to have a new one.
>
>> +struct bcm_cygnus_dte *dte_get_dev_from_devname(const char *devname)
>> +{
>> +struct bcm_cygnus_dte *cygnus_dte = NULL;
>> +bool found = false;
>> +
>> +if (!devname)
>> +return NULL;
>> +
>> +list_for_each_entry(cygnus_dte, _list, node) {
>> +if (!strcmp(dev_name(_dte->pdev->dev), devname)) {
>> +/* Matched on device name */
>> +found = true;
>> +break;
>> +}
>> +}
>> +
>> +return found ? cygnus_dte : NULL;
>> +}
>> +EXPORT_SYMBOL(dte_get_dev_from_devname);
>
> No, don't match on a device name. If you must have a reference, use
> a phandle in DT.
>
>> +int dte_get_timestamp(
>> +struct bcm_cygnus_dte *cygnus_dte,
>> +enum dte_client client,
>> +struct timespec *ts)
>
> Please use timespec64 or ktime_t for internal interfaces.
>
>> +case DTE_IOCTL_SET_DIVIDER:
>
> For the IOCTLs, please write a documentation in man-page form. No need
> for troff formatting, plain text is fine.
>
>> +/**
>> + * DTE Client
>> + */
>> +enum dte_client {
>> +DTE_CLIENT_MIN = 0,
>> +DTE_CLIENT_I2S0_BITCLOCK = 0,
>> +DTE_CLIENT_I2S1_BITCLOCK,
>> +DTE_CLIENT_I2S2_BITCLOCK,
>> +DTE_CLIENT_I2S0_WORDCLOCK,
>> +DTE_CLIENT_I2S1_WORDCLOCK,
>> +DTE_CLIENT_I2S2_WORDCLOCK,
>> +DTE_CLIENT_LCD_CLFP,
>> +DTE_CLIENT_LCD_CLLP,
>> +DTE_CLIENT_GPIO14,
>> +DTE_CLIENT_GPIO15,
>> +DTE_CLIENT_GPIO22,
>> +DTE_CLIENT_GPIO23,
>> +DTE_CLIENT_MAX,
>> +};
>
> Make this more abstract, so we can reuse the API for other vendors.
>
>> +#define DTE_IOCTL_BASE 'd'
>> +#define DTE_IO(nr) _IO(DTE_IOCTL_BASE, nr)
>> +#define DTE_IOR(nr, type) _IOR(DTE_IOCTL_BASE, nr, type)
>> +#define DTE_IOW(nr, type) _IOW(DTE_IOCTL_BASE, nr, type)
>> +#define DTE_IOWR(nr, type) _IOWR(DTE_IOCTL_BASE, nr, type)
>> +
>> +#define DTE_IOCTL_SET_DIVIDER DTE_IOW(0x00, struct dte_data)
>> +#define DTE_IOCTL_ENABLE_TIMESTAMP DTE_IOW(0x01, struct dte_data)
>> +#define DTE_IOCTL_SET_IRQ_INTERVAL DTE_IOW(0x02, struct dte_data)
>> +#define DTE_IOCTL_GET_TIMESTAMP DTE_IOWR(0x03, struct dte_timestamp)
>> +#define DTE_IOCTL_SET_TIME DTE_IOW(0x04, struct timespec)
>> +#define DTE_IOCTL_GET_TIME DTE_IOR(0x05, struct timespec)
>
> Instead of timespec, use a pair of '__u64' values, or alternatively
> just a '__u64' for nanoseconds if the API does not have to cover
> times before 1970 or after 2262.
>
>> +#define DTE_IOCTL_ADJ_TIME DTE_IOW(0x06, int64_t)
>> +#define DTE_IOCTL_ADJ_FREQ DTE_IOW(0x07, int32_t)
>
> Maybe 'struct timex' for the adjustment?
>
> Also, how about adding new syscalls along the lines of
> the timerfd stuff instead of using ioctl?
>
>> +struct dte_data {
>> +enum dte_client client;
>> +unsigned int data;
>> +};
>
> No 'enum' in ioctl data, always use '__u32' etc.
>
>> +
>> +struct dte_timestamp {
>> +enum dte_client client;
>> +struct timespec ts;
>> +};
>
> Instead of timespec, use a pair of '__u64' values, or alternatively
> just a '__u64' for nanoseconds if the API does not have to cover
> times before 1970 or after 2262.
>
>> +struct bcm_cygnus_dte;
>> +
>> +extern struct bcm_cygnus_dte *dte_get_dev_from_devnam
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Hi Arnd,
Thanks for the initial feedback. I'll investigate your suggestions and
get back to you if I have any questions before making some of the API
changes you've suggested.
On 15-04-23 01:04 AM, Arnd Bergmann wrote:
On Wednesday 22 April 2015 16:22:03 Jonathan Richardson wrote:
Reviewed-by: Scott Branden sbran...@broadcom.com
Tested-by: Scott Branden sbran...@broadcom.com
Signed-off-by: Jonathan Richardson jonat...@broadcom.com
No description at all?
You are introducing a new subsystem here, which means you get to put
a lot of thought into the API design, to ensure that it works with
other drivers of the same type, and that we don't already have
a subsystem that does what you need here.
Please write a few pages of text about the tradeoffs that went into
the internal and user-facing API design, and explain the differences
to the existing infrastructure we have for the clocksource, clockevent,
k_clock, posix timers, posix timers, timerfd, rtc, and ptp frameworks,
in particular why your hardware cannot fit into the existing frameworks
and has to have a new one.
+struct bcm_cygnus_dte *dte_get_dev_from_devname(const char *devname)
+{
+struct bcm_cygnus_dte *cygnus_dte = NULL;
+bool found = false;
+
+if (!devname)
+return NULL;
+
+list_for_each_entry(cygnus_dte, dtedev_list, node) {
+if (!strcmp(dev_name(cygnus_dte-pdev-dev), devname)) {
+/* Matched on device name */
+found = true;
+break;
+}
+}
+
+return found ? cygnus_dte : NULL;
+}
+EXPORT_SYMBOL(dte_get_dev_from_devname);
No, don't match on a device name. If you must have a reference, use
a phandle in DT.
+int dte_get_timestamp(
+struct bcm_cygnus_dte *cygnus_dte,
+enum dte_client client,
+struct timespec *ts)
Please use timespec64 or ktime_t for internal interfaces.
+case DTE_IOCTL_SET_DIVIDER:
For the IOCTLs, please write a documentation in man-page form. No need
for troff formatting, plain text is fine.
+/**
+ * DTE Client
+ */
+enum dte_client {
+DTE_CLIENT_MIN = 0,
+DTE_CLIENT_I2S0_BITCLOCK = 0,
+DTE_CLIENT_I2S1_BITCLOCK,
+DTE_CLIENT_I2S2_BITCLOCK,
+DTE_CLIENT_I2S0_WORDCLOCK,
+DTE_CLIENT_I2S1_WORDCLOCK,
+DTE_CLIENT_I2S2_WORDCLOCK,
+DTE_CLIENT_LCD_CLFP,
+DTE_CLIENT_LCD_CLLP,
+DTE_CLIENT_GPIO14,
+DTE_CLIENT_GPIO15,
+DTE_CLIENT_GPIO22,
+DTE_CLIENT_GPIO23,
+DTE_CLIENT_MAX,
+};
Make this more abstract, so we can reuse the API for other vendors.
+#define DTE_IOCTL_BASE 'd'
+#define DTE_IO(nr) _IO(DTE_IOCTL_BASE, nr)
+#define DTE_IOR(nr, type) _IOR(DTE_IOCTL_BASE, nr, type)
+#define DTE_IOW(nr, type) _IOW(DTE_IOCTL_BASE, nr, type)
+#define DTE_IOWR(nr, type) _IOWR(DTE_IOCTL_BASE, nr, type)
+
+#define DTE_IOCTL_SET_DIVIDER DTE_IOW(0x00, struct dte_data)
+#define DTE_IOCTL_ENABLE_TIMESTAMP DTE_IOW(0x01, struct dte_data)
+#define DTE_IOCTL_SET_IRQ_INTERVAL DTE_IOW(0x02, struct dte_data)
+#define DTE_IOCTL_GET_TIMESTAMP DTE_IOWR(0x03, struct dte_timestamp)
+#define DTE_IOCTL_SET_TIME DTE_IOW(0x04, struct timespec)
+#define DTE_IOCTL_GET_TIME DTE_IOR(0x05, struct timespec)
Instead of timespec, use a pair of '__u64' values, or alternatively
just a '__u64' for nanoseconds if the API does not have to cover
times before 1970 or after 2262.
+#define DTE_IOCTL_ADJ_TIME DTE_IOW(0x06, int64_t)
+#define DTE_IOCTL_ADJ_FREQ DTE_IOW(0x07, int32_t)
Maybe 'struct timex' for the adjustment?
Also, how about adding new syscalls along the lines of
the timerfd stuff instead of using ioctl?
+struct dte_data {
+enum dte_client client;
+unsigned int data;
+};
No 'enum' in ioctl data, always use '__u32' etc.
+
+struct dte_timestamp {
+enum dte_client client;
+struct timespec ts;
+};
Instead of timespec, use a pair of '__u64' values, or alternatively
just a '__u64' for nanoseconds if the API does not have to cover
times before 1970 or after 2262.
+struct bcm_cygnus_dte;
+
+extern struct bcm_cygnus_dte *dte_get_dev_from_devname(
+const char *devname);
+extern int dte_enable_timestamp(
+struct bcm_cygnus_dte *cygnus_dte,
+enum dte_client client,
+int enable);
Put the internal declarations into one header in include/linux, and the
user space facing ones in another one in include/uapi/linux.
Arnd
--
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 1/2] misc: Add DT binding for cygnus Digital Timing Engine (DTE) driver.
Reviewed-by: Scott Branden
Tested-by: Scott Branden
Signed-off-by: Jonathan Richardson
---
.../bindings/arm/bcm/brcm,cygnus-dte.txt | 24
1 file changed, 24 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/bcm/brcm,cygnus-dte.txt
diff --git a/Documentation/devicetree/bindings/arm/bcm/brcm,cygnus-dte.txt
b/Documentation/devicetree/bindings/arm/bcm/brcm,cygnus-dte.txt
new file mode 100644
index 000..f30ca86
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/bcm/brcm,cygnus-dte.txt
@@ -0,0 +1,24 @@
+Broadcom Cygnus DTE Digital Timing Engine device tree bindings
+
+The DTE block allows for hardware timestamping of network packets,
+audio/video samples and/or GPIO inputs in a common adjustable time base.
+
+Required SoC Specific Properties:
+- compatible: should be "brcm,cygnus-dte"
+
+- reg: Requires two separate register sets
+ Base address of the Audio EAV Regsiters for DTE block and length of
memory
+ mapped region.
+
+- interrupts: The DTE interrupt number to the cpu.
+
+Example:
+
+/ {
+ dte: dte@0x180af000 {
+ compatible = "brcm,cygnus-dte";
+ reg = <0x180af000 0x1000>;
+ interrupts = ;
+
+ };
+};
--
1.7.9.5
--
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 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Reviewed-by: Scott Branden Tested-by: Scott Branden Signed-off-by: Jonathan Richardson --- drivers/misc/Kconfig | 11 + drivers/misc/Makefile |1 + drivers/misc/bcm_cygnus_dte.c | 927 include/linux/bcm_cygnus_dte.h | 99 + 4 files changed, 1038 insertions(+) create mode 100644 drivers/misc/bcm_cygnus_dte.c create mode 100644 include/linux/bcm_cygnus_dte.h diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 006242c..2a223c3 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -515,6 +515,17 @@ config VEXPRESS_SYSCFG bus. System Configuration interface is one of the possible means of generating transactions on this bus. +config BCM_CYGNUS_DTE + tristate "Cygnus DTE support" + depends on ARCH_BCM_CYGNUS + default ARCH_BCM_CYGNUS + help + Enable support for a Digital Timing Engine (DTE) that allows for hardware + time stamping of network packets, audio/video samples and/or GPIO inputs + in a common adjustable time base. + + If unsure, say N. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig" diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 7d5c4cd..4067b95 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -56,3 +56,4 @@ obj-$(CONFIG_GENWQE) += genwqe/ obj-$(CONFIG_ECHO) += echo/ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o obj-$(CONFIG_CXL_BASE) += cxl/ +obj-$(CONFIG_BCM_CYGNUS_DTE) += bcm_cygnus_dte.o diff --git a/drivers/misc/bcm_cygnus_dte.c b/drivers/misc/bcm_cygnus_dte.c new file mode 100644 index 000..8bf4f93 --- /dev/null +++ b/drivers/misc/bcm_cygnus_dte.c @@ -0,0 +1,927 @@ +/* + * Copyright 2015 Broadcom Corporation. All rights reserved. + * + * Unless you and Broadcom execute a separate written software license + * agreement governing use of this software, this software is licensed to you + * under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* + * This driver implements the Broadcom DTE Digital Timing Engine Driver (DTE). + * The DTE allows for hardware time stamping of network packets, audio/video + * samples and/or GPIO inputs in a common adjustable time base. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define DTE_SW_FIFO_SIZE 64 +#define DTE_HW_FIFO_SIZE 16 +#define DTE_MAX_DEVS 1 +#define DTE_DEVICE_NAME"dte" + +/* Registers */ +#define DTE_CTRL_REG_BASE0x600 +#define DTE_CTRL_REG__INTERRUPT 16 +#define DTE_NEXT_SOI_REG_BASE0x610 +#define DTE_ILEN_REG_BASE0x614 +#define DTE_LTS_FIFO_REG_BASE0x640 +#define DTE_LTS_CSR_REG_BASE 0x644 +#define DTE_LTS_CSR_REG__FIFO_EMPTY 4 +#define DTE_LTS_CSR_REG__FIFO_OVERFLOW 3 +#define DTE_LTS_CSR_REG__FIFO_UNDERFLOW 2 +#define DTE_NCO_LOW_TIME_REG_BASE0x650 +#define DTE_NCO_TIME_REG_BASE0x654 +#define DTE_NCO_OVERFLOW_REG_BASE0x658 +#define DTE_NCO_INC_REG_BASE 0x65c +#define DTE_LTS_DIV_54_REG_BASE 0x660 +#define DTE_LTS_DIV_76_REG_BASE 0x664 +#define DTE_LTS_DIV_98_REG_BASE 0x668 +#define DTE_LTS_DIV_1110_REG_BASE0x66c +#define DTE_LTS_DIV_1312_REG_BASE0x670 +#define DTE_LTS_DIV_14_REG_BASE 0x674 +#define DTE_LTS_DIV_MASK 0x +#define DTE_LTS_DIV_54_REG__DIV_VAL_4_R 0 +#define DTE_LTS_DIV_54_REG__DIV_VAL_5_R 16 +#define DTE_LTS_DIV_76_REG__DIV_VAL_6_R 0 +#define DTE_LTS_DIV_76_REG__DIV_VAL_7_R 16 +#define DTE_LTS_DIV_98_REG__DIV_VAL_8_R 0 +#define DTE_LTS_DIV_98_REG__DIV_VAL_9_R 16 +#define DTE_LTS_DIV_1110_REG__DIV_VAL_10_R 0 +#define DTE_LTS_DIV_1110_REG__DIV_VAL_11_R 16 +#define DTE_LTS_DIV_1312_REG__DIV_VAL_12_R 0 +#define DTE_LTS_DIV_1312_REG__DIV_VAL_13_R 16 +#define DTE_LTS_DIV_14_REG__DIV_VAL_15_R 0 +#define DTE_LTS_DIV_14_REG__DIV_VAL_14_R 16 +#define DTE_LTS_SRC_EN_REG_BASE 0x680 +#define DTE_INTR_STATUS_REG 0x6A0 +#define DTE_INTR_STATUS_ISO_INTR_SHIFT 0 +#define DTE_INTR_STATUS_ISO_INTR_MASK
[PATCH 0/2] Add DTE driver for Cygnus
This patchset includes the Digital Timing Engine (DTE) driver for Cygnus SoC's. Jonathan Richardson (2): misc: Add DT binding for cygnus Digital Timing Engine (DTE) driver. misc: Add initial Digital Timing Engine (DTE) driver for cygnus .../bindings/arm/bcm/brcm,cygnus-dte.txt | 24 + drivers/misc/Kconfig | 11 + drivers/misc/Makefile |1 + drivers/misc/bcm_cygnus_dte.c | 927 include/linux/bcm_cygnus_dte.h | 99 +++ 5 files changed, 1062 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/bcm/brcm,cygnus-dte.txt create mode 100644 drivers/misc/bcm_cygnus_dte.c create mode 100644 include/linux/bcm_cygnus_dte.h -- 1.7.9.5 -- 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/
