mbox series

[v9,0/2] PWM support for HiFive Unleashed

Message ID 1552378289-27245-1-git-send-email-yash.shah@sifive.com (mailing list archive)
Headers show
Series PWM support for HiFive Unleashed | expand

Message

Yash Shah March 12, 2019, 8:11 a.m. UTC 
This patch series adds a PWM driver and DT documentation
for HiFive Unleashed board. The patches are mostly based on
Wesley's patch.

v9
- Use appropriate bitfield macros
- Add approx_period in pwm_sifive_ddata struct and related changes
- Correct the eqn for calculation of frac (in pwm_sifive_apply)
- Other minor fixes

v8
- Typo corrections
- Remove active_user and related code
- Do not clear PWM_SIFIVE_PWMCFG_EN_ALWAYS
- Other minor fixes

v7
- Modify description of compatible property in DT documentation
- Use mutex locks at appropriate places
- Fix all bad line breaks
- Allow enabling/disabling PWM only when the user is the only active user
- Remove Deglitch logic
- Other minor fixes

v6
- Remove the global property 'sifive,period-ns'
- Implement free and request callbacks to maintain user counts.
- Add user_count member to struct pwm_sifive_ddata
- Allow period change only if user_count is one
- Add pwm_sifive_enable function to enable/disable PWM
- Change calculation logic of frac (in pwm_sifive_apply)
- Remove state correction
- Remove pwm_sifive_xlate function
- Clock to be enabled only when PWM is enabled
- Other minor fixes

v5
- Correct the order of compatible string properties
- PWM state correction to be done always
- Other minor fixes based upon feedback on v4

v4
- Rename macros with appropriate names
- Remove unused macros
- Rename struct sifive_pwm_device to struct pwm_sifive_ddata
- Rename function prefix as per driver name
- Other minor fixes based upon feedback on v3

v3
- Add a link to the reference manaul
- Use appropriate apis for division operation
- Add check for polarity
- Enable clk before calling clk_get_rate
- Other minor fixes based upon feedback on v2

V2 changed from V1:
- Remove inclusion of dt-bindings/pwm/pwm.h
- Remove artificial alignments
- Replace ioread32/iowrite32 with readl/writel
- Remove camelcase
- Change dev_info to dev_dbg for unnecessary log
- Correct typo in driver name
- Remove use of of_match_ptr macro
- Update the DT compatible strings and Add reference to a common
  versioning document

Yash Shah (2):
  pwm: sifive: Add DT documentation for SiFive PWM Controller
  pwm: sifive: Add a driver for SiFive SoC PWM

 .../devicetree/bindings/pwm/pwm-sifive.txt         |  33 ++
 drivers/pwm/Kconfig                                |  11 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-sifive.c                           | 338 +++++++++++++++++++++
 4 files changed, 383 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt
 create mode 100644 drivers/pwm/pwm-sifive.c

Comments

Andreas Schwab March 12, 2019, 10:14 a.m. UTC | #1 
On Mär 12 2019, Yash Shah <yash.shah@sifive.com> wrote:

> This patch series adds a PWM driver and DT documentation
> for HiFive Unleashed board. The patches are mostly based on
> Wesley's patch.

Heartbeat trigger still doesn't work for me.

Andreas.
Yash Shah March 15, 2019, 11:49 a.m. UTC | #2 
On Tue, Mar 12, 2019 at 3:44 PM Andreas Schwab <schwab@suse.de> wrote:
>
> On Mär 12 2019, Yash Shah <yash.shah@sifive.com> wrote:
>
> > This patch series adds a PWM driver and DT documentation
> > for HiFive Unleashed board. The patches are mostly based on
> > Wesley's patch.
>
> Heartbeat trigger still doesn't work for me.

You need to make sure the period setting is passed via the
conventional way in DT file.
Example:
pwmleds {
    compatible = "pwm-leds";
    heartbeat {
        pwms = <&L45 0 10000000 0>;
        max-brightness = <255>;
        linux,default-trigger = "heartbeat";
    };
};

I have tested on HiFive unleashed board. To modify DT file I performed
the following steps:

Use the open-source FSBL from:
https://github.com/sifive/freedom-u540-c000-bootloader

Modify the fsbl/ux00_fsbl.dts file and re-build the fsbl.bin

Steps for using fsbl.bin in HiFive Unleashed board:
https://github.com/sifive/freedom-u540-c000-bootloader/issues/9

>
> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."
Andreas Schwab March 18, 2019, 9:24 a.m. UTC | #3 
On Mär 15 2019, Yash Shah <yash.shah@sifive.com> wrote:

> Use the open-source FSBL from:
> https://github.com/sifive/freedom-u540-c000-bootloader
>
> Modify the fsbl/ux00_fsbl.dts file and re-build the fsbl.bin

That doesn't even compile.

Andreas.
Andreas Schwab March 18, 2019, 5:26 p.m. UTC | #4 
On Mär 15 2019, Yash Shah <yash.shah@sifive.com> wrote:

> You need to make sure the period setting is passed via the
> conventional way in DT file.
> Example:
> pwmleds {
>     compatible = "pwm-leds";
>     heartbeat {
>         pwms = <&L45 0 10000000 0>;
>         max-brightness = <255>;
>         linux,default-trigger = "heartbeat";
>     };
> };

I've now managed to build a working FSBL with that change, but that
didn't change anything.  There is not even a heartbeat option in
/sys/class/leds/heartbeat/trigger any more.

Andreas.
Yash Shah March 19, 2019, 6:26 a.m. UTC | #5 
On Mon, Mar 18, 2019 at 10:56 PM Andreas Schwab <schwab@suse.de> wrote:
>
> On Mär 15 2019, Yash Shah <yash.shah@sifive.com> wrote:
>
> > You need to make sure the period setting is passed via the
> > conventional way in DT file.
> > Example:
> > pwmleds {
> >     compatible = "pwm-leds";
> >     heartbeat {
> >         pwms = <&L45 0 10000000 0>;
> >         max-brightness = <255>;
> >         linux,default-trigger = "heartbeat";
> >     };
> > };
>
> I've now managed to build a working FSBL with that change, but that
> didn't change anything.  There is not even a heartbeat option in
> /sys/class/leds/heartbeat/trigger any more.

Below is the copy of the pwm nodes in my DTS file:

L45: pwm@10020000 {
        compatible = "sifive,pwm0";
        interrupt-parent = <&plic0>;
        interrupts = <42 43 44 45>;
        reg = <0x0 0x10020000 0x0 0x1000>;
        reg-names = "control";
        clocks = <&prci 3>;
        #pwm-cells = <2>;
};
L46: pwm@10021000 {
        compatible = "sifive,pwm0";
        interrupt-parent = <&plic0>;
        interrupts = <46 47 48 49>;
        reg = <0x0 0x10021000 0x0 0x1000>;
        reg-names = "control";
        clocks = <&prci 3>;
        #pwm-cells = <2>;
};
pwmleds {
        compatible = "pwm-leds";
        heartbeat {
                pwms = <&L45 0 10000000 0>;
                max-brightness = <255>;
                linux,default-trigger = "heartbeat";
        };
};

The above works for me.
I just noticed that I have been using pwm-cells = 2, instead of 3.
Maybe that is the problem here.
I will suggest you test it on v11 patch in which I will fix this
pwm-cells issue.

Thanks for pointing it out.

>
> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."
Yash Shah March 25, 2019, 11:43 a.m. UTC | #6 
Hi Andreas,

On Tue, Mar 19, 2019 at 11:56 AM Yash Shah <yash.shah@sifive.com> wrote:
>
> On Mon, Mar 18, 2019 at 10:56 PM Andreas Schwab <schwab@suse.de> wrote:
> >
> > On Mär 15 2019, Yash Shah <yash.shah@sifive.com> wrote:
> >
> > > You need to make sure the period setting is passed via the
> > > conventional way in DT file.
> > > Example:
> > > pwmleds {
> > >     compatible = "pwm-leds";
> > >     heartbeat {
> > >         pwms = <&L45 0 10000000 0>;
> > >         max-brightness = <255>;
> > >         linux,default-trigger = "heartbeat";
> > >     };
> > > };
> >
> > I've now managed to build a working FSBL with that change, but that
> > didn't change anything.  There is not even a heartbeat option in
> > /sys/class/leds/heartbeat/trigger any more.
>
...
>
> The above works for me.
> I just noticed that I have been using pwm-cells = 2, instead of 3.
> Maybe that is the problem here.
> I will suggest you test it on v11 patch in which I will fix this
> pwm-cells issue.

I have sent out the v11 patchset, you can test the heartbeat
application with that patchset.
You still need to make that DT file modification which you previously
did, using fsbl.bin
Just for your reference, I am copying my DT file and kernel config
which I used for my test.
The same is available at dev/yashs/pwm_5.0-rc1 branch of
https://github.com/yashshah7/riscv-linux.git

/dts-v1/;

/*#include <linux/clk/sifive-fu540-prci.h>*/
#define PRCI_CLK_TLCLK 3

/ {
#address-cells = <2>;
#size-cells = <2>;
compatible = "sifive,fu540-c000";

aliases {
serial0 = &uart0;
serial1 = &uart1;
};

chosen {
};

cpus {
#address-cells = <1>;
#size-cells = <0>;
timebase-frequency = <1000000>;
cpu0: cpu@0 {
clock-frequency = <0>;
compatible = "sifive,u51", "sifive,rocket0", "riscv";
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <128>;
i-cache-size = <16384>;
reg = <0>;
riscv,isa = "rv64imac";
status = "okay";
cpu0_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu1: cpu@1 {
clock-frequency = <0>;
compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
d-cache-block-size = <64>;
d-cache-sets = <64>;
d-cache-size = <32768>;
d-tlb-sets = <1>;
d-tlb-size = <32>;
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <64>;
i-cache-size = <32768>;
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
reg = <1>;
riscv,isa = "rv64imafdc";
status = "okay";
tlb-split;
cpu1_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu2: cpu@2 {
clock-frequency = <0>;
compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
d-cache-block-size = <64>;
d-cache-sets = <64>;
d-cache-size = <32768>;
d-tlb-sets = <1>;
d-tlb-size = <32>;
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <64>;
i-cache-size = <32768>;
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
reg = <2>;
riscv,isa = "rv64imafdc";
status = "okay";
tlb-split;
cpu2_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu3: cpu@3 {
clock-frequency = <0>;
compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
d-cache-block-size = <64>;
d-cache-sets = <64>;
d-cache-size = <32768>;
d-tlb-sets = <1>;
d-tlb-size = <32>;
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <64>;
i-cache-size = <32768>;
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
reg = <3>;
riscv,isa = "rv64imafdc";
status = "okay";
tlb-split;
cpu3_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu4: cpu@4 {
clock-frequency = <0>;
compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
d-cache-block-size = <64>;
d-cache-sets = <64>;
d-cache-size = <32768>;
d-tlb-sets = <1>;
d-tlb-size = <32>;
device_type = "cpu";
i-cache-block-size = <64>;
i-cache-sets = <64>;
i-cache-size = <32768>;
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
reg = <4>;
riscv,isa = "rv64imafdc";
status = "okay";
tlb-split;
cpu4_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
};
soc {
#address-cells = <2>;
#size-cells = <2>;
compatible = "sifive,fu540-soc", "simple-bus";
ranges;
prci: prci@10000000 {
compatible = "sifive,fu540-c000-prci";
reg = <0x0 0x10000000 0x0 0x1000>;
clocks = <&hfclk>, <&rtcclk>;
#clock-cells = <1>;
};
uart0: serial@10010000 {
compatible = "sifive,fu540-c000-uart", "sifive,uart0";
interrupt-parent = <&plic0>;
interrupts = <4>;
reg = <0x0 0x10010000 0x0 0x1000>;
clocks = <&prci PRCI_CLK_TLCLK>;
};
uart1: serial@10011000 {
compatible = "sifive,fu540-c000-uart", "sifive,uart0";
interrupt-parent = <&plic0>;
interrupts = <5>;
reg = <0x0 0x10011000 0x0 0x1000>;
clocks = <&prci PRCI_CLK_TLCLK>;
};
L5: clint@2000000 {
compatible = "riscv,clint0";
interrupts-extended = <
&cpu0_intc 3 &cpu0_intc 7
&cpu1_intc 3 &cpu1_intc 7
&cpu2_intc 3 &cpu2_intc 7
&cpu3_intc 3 &cpu3_intc 7
&cpu4_intc 3 &cpu4_intc 7>;
reg = <0x0 0x2000000 0x0 0x10000>;
                };
plic0: interrupt-controller@c000000 {
#interrupt-cells = <1>;
compatible = "riscv,plic0";
interrupt-controller;
interrupts-extended = <
&cpu0_intc 11
&cpu1_intc 11 &cpu1_intc 9
&cpu2_intc 11 &cpu2_intc 9
&cpu3_intc 11 &cpu3_intc 9
&cpu4_intc 11 &cpu4_intc 9>;
reg = <0x0 0xc000000 0x0 0x4000000>;
                        riscv,ndev = <53>;
};
L45: pwm@10020000 {
                        compatible = "sifive,pwm0";
                        interrupt-parent = <&plic0>;
                        interrupts = <42 43 44 45>;
                        reg = <0x0 0x10020000 0x0 0x1000>;
                        reg-names = "control";
                        clocks = <&prci 3>;
                        #pwm-cells = <3>;
                };
                L46: pwm@10021000 {
                        compatible = "sifive,pwm0";
                        interrupt-parent = <&plic0>;
                        interrupts = <46 47 48 49>;
                        reg = <0x0 0x10021000 0x0 0x1000>;
                        reg-names = "control";
                        clocks = <&prci 3>;
                        #pwm-cells = <3>;
                };
pwmleds {
                        compatible = "pwm-leds";
                        heartbeat {
                                pwms = <&L45 0 100000 0>;
                                max-brightness = <255>;
                                linux,default-trigger = "heartbeat";
                        };
                        mtd {
                                pwms = <&L45 1 100000 0>;
                                max-brightness = <255>;
                                linux,default-trigger = "mtd";
                        };
                        netdev {
                                pwms = <&L45 2 100000 0>;
                                max-brightness = <255>;
                                linux,default-trigger = "netdev";
                        };
                        panic {
                                pwms = <&L45 3 100000 0>;
                                max-brightness = <255>;
                                linux,default-trigger = "panic";
                        };
};
};
};


kernel config:
CONFIG_SYSVIPC=y
CONFIG_POSIX_MQUEUE=y
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y
CONFIG_CGROUPS=y
CONFIG_CGROUP_SCHED=y
CONFIG_CFS_BANDWIDTH=y
CONFIG_CGROUP_BPF=y
CONFIG_NAMESPACES=y
CONFIG_USER_NS=y
CONFIG_CHECKPOINT_RESTORE=y
CONFIG_BLK_DEV_INITRD=y
CONFIG_EXPERT=y
CONFIG_BPF_SYSCALL=y
CONFIG_SMP=y
CONFIG_PCI=y
CONFIG_PCIE_XILINX=y
CONFIG_MODULES=y
CONFIG_MODULE_UNLOAD=y
CONFIG_NET=y
CONFIG_PACKET=y
CONFIG_UNIX=y
CONFIG_INET=y
CONFIG_IP_MULTICAST=y
CONFIG_IP_ADVANCED_ROUTER=y
CONFIG_IP_PNP=y
CONFIG_IP_PNP_DHCP=y
CONFIG_IP_PNP_BOOTP=y
CONFIG_IP_PNP_RARP=y
CONFIG_NETLINK_DIAG=y
CONFIG_DEVTMPFS=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_VIRTIO_BLK=y
CONFIG_BLK_DEV_SD=y
CONFIG_BLK_DEV_SR=y
CONFIG_ATA=y
CONFIG_SATA_AHCI=y
CONFIG_SATA_AHCI_PLATFORM=y
CONFIG_NETDEVICES=y
CONFIG_VIRTIO_NET=y
CONFIG_MACB=y
CONFIG_E1000E=y
CONFIG_R8169=y
CONFIG_MICROSEMI_PHY=y
CONFIG_INPUT_MOUSEDEV=y
CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8250_CONSOLE=y
CONFIG_SERIAL_OF_PLATFORM=y
CONFIG_SERIAL_EARLYCON_RISCV_SBI=y
CONFIG_SERIAL_SIFIVE=y
CONFIG_SERIAL_SIFIVE_CONSOLE=y
CONFIG_HVC_RISCV_SBI=y
# CONFIG_PTP_1588_CLOCK is not set
CONFIG_DRM=y
CONFIG_DRM_RADEON=y
CONFIG_FRAMEBUFFER_CONSOLE=y
CONFIG_USB=y
CONFIG_USB_XHCI_HCD=y
CONFIG_USB_XHCI_PLATFORM=y
CONFIG_USB_EHCI_HCD=y
CONFIG_USB_EHCI_HCD_PLATFORM=y
CONFIG_USB_OHCI_HCD=y
CONFIG_USB_OHCI_HCD_PLATFORM=y
CONFIG_USB_STORAGE=y
CONFIG_USB_UAS=y
CONFIG_VIRTIO_MMIO=y
CONFIG_SIFIVE_PLIC=y
CONFIG_RAS=y
CONFIG_EXT4_FS=y
CONFIG_EXT4_FS_POSIX_ACL=y
CONFIG_AUTOFS4_FS=y
CONFIG_MSDOS_FS=y
CONFIG_VFAT_FS=y
CONFIG_TMPFS=y
CONFIG_TMPFS_POSIX_ACL=y
CONFIG_NFS_FS=y
CONFIG_NFS_V4=y
CONFIG_NFS_V4_1=y
CONFIG_NFS_V4_2=y
CONFIG_ROOT_NFS=y
CONFIG_CRYPTO_USER_API_HASH=y
CONFIG_PRINTK_TIME=y
# CONFIG_RCU_TRACE is not set
CONFIG_CMDLINE_BOOL=y
CONFIG_CMDLINE="console=ttySIF0,115200 ignore_loglevel debug"
CONFIG_CLK_SIFIVE=y
CONFIG_CLK_SIFIVE_FU540_PRCI=y

- Yash
> >
> > Andreas.
> >
> > --
> > Andreas Schwab, SUSE Labs, schwab@suse.de
> > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> > "And now for something completely different."
Andreas Schwab March 25, 2019, 11:58 a.m. UTC | #7 
On Mär 25 2019, Yash Shah <yash.shah@sifive.com> wrote:

> I have sent out the v11 patchset, you can test the heartbeat
> application with that patchset.
> You still need to make that DT file modification which you previously
> did, using fsbl.bin

Why can't the driver make use of sifive,approx-period?

Andreas.
Yash Shah March 25, 2019, 12:09 p.m. UTC | #8 
On Mon, Mar 25, 2019 at 5:28 PM Andreas Schwab <schwab@suse.de> wrote:
>
> On Mär 25 2019, Yash Shah <yash.shah@sifive.com> wrote:
>
> > I have sent out the v11 patchset, you can test the heartbeat
> > application with that patchset.
> > You still need to make that DT file modification which you previously
> > did, using fsbl.bin
>
> Why can't the driver make use of sifive,approx-period?

Because as per the review comments and discussions, it has been
decided that the driver will make use of the conventional interface to
pass period settings instead of 'sifive,approx-period'.

For your reference:
https://lkml.org/lkml/2019/2/6/159

>
> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."