| Message ID | CAHOvCC5aEc=p_6Yxkyr8n9BXZug_-NA_CJswu8vtkM8aOBhWvg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: Add gpio and pwmleds to DTS(/arch/riscv/boot/dts/sifive/) | expand |
I have a similar patch, but still using old GPIO driver (non upstream). See comments below. On Tue, Jan 21, 2020 at 8:59 AM JaeJoon Jung <rgbi3307@gmail.com> wrote: > > I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board. > > diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > index a2e3d54e830c..b03bf570020c 100644 > --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > + gpio0: gpio@10060000 { > + compatible = "sifive,fu540-c000-gpio", "sifive,gpio0"; > + reg = <0x0 0x10060000 0x0 0x1000>; > + reg-names = "control"; > + gpio-controller; > + #gpio-cells = <2>; > + ngpios = <16>; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupt-parent = <&plic0>; > + interrupts = <15 16 17 18 19 20 21 22 23 24 > 25 26 27 28 29 30>; > + status = "disabled"; > + }; > > > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > index 88cfcb96bf23..f3f55dbbf737 100644 > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > cpus { > @@ -41,6 +41,39 @@ > clock-frequency = <RTCCLK_FREQ>; > clock-output-names = "rtcclk"; > }; > + > + > + pwmleds { > + compatible = "pwm-leds"; > + heartbeat { > + label = "led1"; Could we have labels based on schematics and prints on PCB? I use: label = "green:d1"; label = "green:d2"; label = "green:d3"; label = "green:d4"; > + max-brightness = <255>; > + active-low = <1>; > + pwms = <&pwm0 0 7812500 0>; > + linux,default-trigger = "heartbeat"; > + }; > + mtd { > + label = "led2"; > + max-brightness = <255>; > + active-low = <1>; > + pwms = <&pwm0 1 7812500 0>; > + linux,default-trigger = "mtd"; > + }; > + netdev { Just a quick note. For this to work properly you also need to have udev rule. I think, there was a patch posted in 2019 to enable netdev configuration in DTS to avoid having configuration via udev or similar, but that wasn't merged the last time I checked (late 2019). See: [PATCH 4/4] leds: netdev trigger: allow setting initial values in device tree https://www.spinics.net/lists/netdev/msg557736.html > + label = "led3"; > + max-brightness = <255>; > + active-low = <1>; > + pwms = <&pwm0 2 7812500 0>; > + linux,default-trigger = "netdev"; > + }; > + panic { I find that panic PWM LED doesn't actually work for me. IIUC this might only work for GPIO attached LEDs based on comments I found. I have this LED remapped for mmc0 activity: mmc0 { label = "green:d4"; pwms = <&pwm0 3 2000000 0>; max-brightness = <255>; linux,default-trigger = "mmc0"; }; > + label = "led4"; > + max-brightness = <255>; > + active-low = <1>; > + pwms = <&pwm0 3 7812500 0>; > + linux,default-trigger = "panic"; > + }; > + }; > }; > > &uart0 { > @@ -94,3 +127,7 @@ > &pwm1 { > status = "okay"; > }; > + > +&gpio0 { > + status = "okay"; > +}; > > > If apply above DTS, the gpio-sifive driver works well without source > modification. > drivers/gpio/gpio-sifive.c > drivers/leds/leds-pwm.c > > I have checked below: > > led1(D1) is acting well as heartbeat. > > RISCV-FU540:/sys/class/leds# pwd > /sys/class/leds > RISCV-FU540:/sys/class/leds# ll > total 0 > drwxr-xr-x 2 root root 0 Jan 1 00:00 ./ > drwxr-xr-x 32 root root 0 Jan 1 00:00 ../ > lrwxrwxrwx 1 root root 0 Jan 1 00:00 led1 -> > ../../devices/platform/pwmleds/leds/led1/ > lrwxrwxrwx 1 root root 0 Jan 1 00:00 led2 -> > ../../devices/platform/pwmleds/leds/led2/ > lrwxrwxrwx 1 root root 0 Jan 1 00:00 led3 -> > ../../devices/platform/pwmleds/leds/led3/ > lrwxrwxrwx 1 root root 0 Jan 1 00:00 led4 -> > ../../devices/platform/pwmleds/leds/led4/ > > RISCV-FU540:/sys/class/leds# cd led3 > RISCV-FU540:/sys/class/leds/led3# ll > total 0 > drwxr-xr-x 2 root root 0 Jan 1 00:00 ./ > drwxr-xr-x 6 root root 0 Jan 1 00:00 ../ > -rw-r--r-- 1 root root 4096 Jan 1 00:01 brightness > lrwxrwxrwx 1 root root 0 Jan 1 00:00 device -> > ../../../pwmleds/ > -r--r--r-- 1 root root 4096 Jan 1 00:00 max_brightness > lrwxrwxrwx 1 root root 0 Jan 1 00:00 subsystem -> > ../../../../../class/leds/ > -rw-r--r-- 1 root root 4096 Jan 1 00:00 trigger > -rw-r--r-- 1 root root 4096 Jan 1 00:00 uevent > RISCV-FU540:/sys/class/leds/led3# echo 1 > brightness > RISCV-FU540:/sys/class/leds/led3# echo 127 > brightness > RISCV-FU540:/sys/class/leds/led3# echo 255 > brightness > > leds(D1, D2, D3, D4) are acting well as pwm brightness. >
On Tue, 21 Jan 2020, JaeJoon Jung wrote: > I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board. > > diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > index a2e3d54e830c..b03bf570020c 100644 > --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > + gpio0: gpio@10060000 { > + compatible = "sifive,fu540-c000-gpio", "sifive,gpio0"; > + reg = <0x0 0x10060000 0x0 0x1000>; > + reg-names = "control"; > + gpio-controller; > + #gpio-cells = <2>; > + ngpios = <16>; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupt-parent = <&plic0>; > + interrupts = <15 16 17 18 19 20 21 22 23 24 > 25 26 27 28 29 30>; > + status = "disabled"; > + }; Yash posted this a while ago: https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf > > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > index 88cfcb96bf23..f3f55dbbf737 100644 > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > cpus { > @@ -41,6 +41,39 @@ > clock-frequency = <RTCCLK_FREQ>; > clock-output-names = "rtcclk"; > }; > + > + > + pwmleds { > + compatible = "pwm-leds"; > + heartbeat { > + label = "led1"; > + max-brightness = <255>; > + active-low = <1>; > + pwms = <&pwm0 0 7812500 0>; > + linux,default-trigger = "heartbeat"; > + }; > + mtd { > + label = "led2"; > + max-brightness = <255>; > + active-low = <1>; > + pwms = <&pwm0 1 7812500 0>; > + linux,default-trigger = "mtd"; > + }; > + netdev { > + label = "led3"; > + max-brightness = <255>; > + active-low = <1>; > + pwms = <&pwm0 2 7812500 0>; > + linux,default-trigger = "netdev"; > + }; > + panic { > + label = "led4"; > + max-brightness = <255>; > + active-low = <1>; > + pwms = <&pwm0 3 7812500 0>; > + linux,default-trigger = "panic"; > + }; > + }; > }; I don't think it's good to add these pwmleds to the default board DTS file. I realize that many upstream ARM development boards have added this type of configuration, but it gets in the way of reusing this DT file when integrators wish to have the LEDs used for different purposes. If the Unleashed silkscreen had text on it describing the LEDs as having these specific functions, or if Unleashed was sold in a case with similar markings on the outside, it'd be a different story, and then a change like the above could make sense. - Paul
I agree with you because LEDs are user defined using method. And, I am sorry about that I confirmed lately below posted by Yash. https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf It is helpful for me and I am testing it. If I find a bug, I am going to share about it. Thanks a lot, Have a nice day. Yours, JaeJoon Jung On Thu, 30 Jan 2020 at 12:35, Paul Walmsley <paul.walmsley@sifive.com> wrote: > > On Tue, 21 Jan 2020, JaeJoon Jung wrote: > > > I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board. > > > > diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > index a2e3d54e830c..b03bf570020c 100644 > > --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > > > + gpio0: gpio@10060000 { > > + compatible = "sifive,fu540-c000-gpio", "sifive,gpio0"; > > + reg = <0x0 0x10060000 0x0 0x1000>; > > + reg-names = "control"; > > + gpio-controller; > > + #gpio-cells = <2>; > > + ngpios = <16>; > > + interrupt-controller; > > + #interrupt-cells = <2>; > > + interrupt-parent = <&plic0>; > > + interrupts = <15 16 17 18 19 20 21 22 23 24 > > 25 26 27 28 29 30>; > > + status = "disabled"; > > + }; > > Yash posted this a while ago: > > https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf > > > > > > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > index 88cfcb96bf23..f3f55dbbf737 100644 > > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > > > cpus { > > @@ -41,6 +41,39 @@ > > clock-frequency = <RTCCLK_FREQ>; > > clock-output-names = "rtcclk"; > > }; > > + > > + > > + pwmleds { > > + compatible = "pwm-leds"; > > + heartbeat { > > + label = "led1"; > > + max-brightness = <255>; > > + active-low = <1>; > > + pwms = <&pwm0 0 7812500 0>; > > + linux,default-trigger = "heartbeat"; > > + }; > > + mtd { > > + label = "led2"; > > + max-brightness = <255>; > > + active-low = <1>; > > + pwms = <&pwm0 1 7812500 0>; > > + linux,default-trigger = "mtd"; > > + }; > > + netdev { > > + label = "led3"; > > + max-brightness = <255>; > > + active-low = <1>; > > + pwms = <&pwm0 2 7812500 0>; > > + linux,default-trigger = "netdev"; > > + }; > > + panic { > > + label = "led4"; > > + max-brightness = <255>; > > + active-low = <1>; > > + pwms = <&pwm0 3 7812500 0>; > > + linux,default-trigger = "panic"; > > + }; > > + }; > > }; > > I don't think it's good to add these pwmleds to the default board DTS > file. I realize that many upstream ARM development boards have added this > type of configuration, but it gets in the way of reusing this DT file when > integrators wish to have the LEDs used for different purposes. If the > Unleashed silkscreen had text on it describing the LEDs as having these > specific functions, or if Unleashed was sold in a case with similar > markings on the outside, it'd be a different story, and then a change like > the above could make sense. > > > - Paul
On Thu, Jan 30, 2020 at 7:24 AM JaeJoon Jung <rgbi3307@gmail.com> wrote: > > I agree with you because LEDs are user defined using method. > > And, I am sorry about that I confirmed lately below posted by Yash. > > https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf > > It is helpful for me and I am testing it. > If I find a bug, I am going to share about it. > > Thanks a lot, Have a nice day. > > Yours, > JaeJoon Jung > > On Thu, 30 Jan 2020 at 12:35, Paul Walmsley <paul.walmsley@sifive.com> wrote: > > > > On Tue, 21 Jan 2020, JaeJoon Jung wrote: > > > > > I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board. > > > > > > diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > > b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > > index a2e3d54e830c..b03bf570020c 100644 > > > --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > > +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > > > > > + gpio0: gpio@10060000 { > > > + compatible = "sifive,fu540-c000-gpio", "sifive,gpio0"; > > > + reg = <0x0 0x10060000 0x0 0x1000>; > > > + reg-names = "control"; > > > + gpio-controller; > > > + #gpio-cells = <2>; > > > + ngpios = <16>; > > > + interrupt-controller; > > > + #interrupt-cells = <2>; > > > + interrupt-parent = <&plic0>; > > > + interrupts = <15 16 17 18 19 20 21 22 23 24 > > > 25 26 27 28 29 30>; > > > + status = "disabled"; > > > + }; > > > > Yash posted this a while ago: > > > > https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf > > > > > > > > > > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > > index 88cfcb96bf23..f3f55dbbf737 100644 > > > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > > > > > cpus { > > > @@ -41,6 +41,39 @@ > > > clock-frequency = <RTCCLK_FREQ>; > > > clock-output-names = "rtcclk"; > > > }; > > > + > > > + > > > + pwmleds { > > > + compatible = "pwm-leds"; > > > + heartbeat { > > > + label = "led1"; > > > + max-brightness = <255>; > > > + active-low = <1>; > > > + pwms = <&pwm0 0 7812500 0>; > > > + linux,default-trigger = "heartbeat"; > > > + }; > > > + mtd { > > > + label = "led2"; > > > + max-brightness = <255>; > > > + active-low = <1>; > > > + pwms = <&pwm0 1 7812500 0>; > > > + linux,default-trigger = "mtd"; > > > + }; > > > + netdev { > > > + label = "led3"; > > > + max-brightness = <255>; > > > + active-low = <1>; > > > + pwms = <&pwm0 2 7812500 0>; > > > + linux,default-trigger = "netdev"; > > > + }; > > > + panic { > > > + label = "led4"; > > > + max-brightness = <255>; > > > + active-low = <1>; > > > + pwms = <&pwm0 3 7812500 0>; > > > + linux,default-trigger = "panic"; > > > + }; > > > + }; > > > }; > > > > I don't think it's good to add these pwmleds to the default board DTS > > file. I realize that many upstream ARM development boards have added this > > type of configuration, but it gets in the way of reusing this DT file when > > integrators wish to have the LEDs used for different purposes. If the > > Unleashed silkscreen had text on it describing the LEDs as having these > > specific functions, or if Unleashed was sold in a case with similar > > markings on the outside, it'd be a different story, and then a change like > > the above could make sense. I think, there is a middle solution. On SiFive Unleashed PWM LEDs still exist. So let's define those LEDs, but disable them (i.e. no default trigger). In that case user has an options to write udev rules to setup those PWM LEDs as he/she likes. I already use udev rule for netdev LED configuration as today DTS does not provide options to configure it [within DTS] (patch posted in 2019). It would look something like below (not tested). We use labels from PCB and schematics, but do not assign any default functions/triggers. Once the distro boots udev rules will set default triggers and do required configuration. --- .../riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 27 ++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts index 828f406..eb793b5 100644 --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts @@ -26,6 +26,33 @@ }; soc { + pwmleds { + compatible = "pwm-leds"; + d1 { + label = "green:d1"; + pwms = <&pwm0 0 2000000 0>; + max-brightness = <255>; + linux,default-trigger = "none"; + }; + d2 { + label = "green:d2"; + pwms = <&pwm0 1 2000000 0>; + max-brightness = <255>; + linux,default-trigger = "none"; + }; + d3 { + label = "green:d3"; + pwms = <&pwm0 2 2000000 0>; + max-brightness = <255>; + linux,default-trigger = "none"; + }; + d4 { + label = "green:d4"; + pwms = <&pwm0 3 2000000 0>; + max-brightness = <255>; + linux,default-trigger = "none"; + }; + }; }; hfclk: hfclk { -- 2.7.4
Hello David Abdurachmanov,
Thanks to your deep opinions,
but it's not that important, but there are things to check.
If you apply below DTS, It works all fine without source modification.
I think that you are missing the active-low = <1> and
linux,default-trigger = "heartbeat"...
The following setting is not set by default, but it works well if you set it.
CONFIG_PWM_SIFIVE
CONFIG_NEW_LEDS
CONFIG_LEDS_PWM
CONFIG_LEDS_CLASS
CONFIG_LEDS_TRIGGERS
CONFIG_LEDS_TRIGGER_HEARTBEAT
--- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
+++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
cpus {
@@ -41,6 +41,39 @@
clock-frequency = <RTCCLK_FREQ>;
clock-output-names = "rtcclk";
};
+
+
+ pwmleds {
+ compatible = "pwm-leds";
+ heartbeat {
+ label = "led1";
+ max-brightness = <255>;
+ active-low = <1>;
+ pwms = <&pwm0 0 1000000 0>;
+ linux,default-trigger = "heartbeat";
+ };
+ mtd {
+ label = "led2";
+ max-brightness = <255>;
+ active-low = <1>;
+ pwms = <&pwm0 1 1000000 0>;
+ linux,default-trigger = "mtd";
+ };
+ netdev {
+ label = "led3";
+ max-brightness = <255>;
+ active-low = <1>;
+ pwms = <&pwm0 2 1000000 0>;
+ linux,default-trigger = "netdev";
+ };
+ panic {
+ label = "led4";
+ max-brightness = <255>;
+ active-low = <1>;
+ pwms = <&pwm0 3 1000000 0>;
+ linux,default-trigger = "panic";
+ };
+ };
};
And I think that the simpler the name, the better.
labels are displayed at /sys/class/leds and user can access and control it here.
RISCV-FU540:/sys/class/leds# pwd
/sys/class/leds
RISCV-FU540:/sys/class/leds# ll
total 0
drwxr-xr-x 2 root root 0 Jan 1 00:00 ./
drwxr-xr-x 33 root root 0 Jan 1 00:00 ../
lrwxrwxrwx 1 root root 0 Jan 1 00:00 led1 ->
../../devices/platform/pwmleds/leds/led1/
lrwxrwxrwx 1 root root 0 Jan 1 00:00 led2 ->
../../devices/platform/pwmleds/leds/led2/
lrwxrwxrwx 1 root root 0 Jan 1 00:00 led3 ->
../../devices/platform/pwmleds/leds/led3/
lrwxrwxrwx 1 root root 0 Jan 1 00:00 led4 ->
../../devices/platform/pwmleds/leds/led4/
RISCV-FU540:/sys/class/leds# cd led1
RISCV-FU540:/sys/class/leds/led1# ll
total 0
drwxr-xr-x 2 root root 0 Jan 1 00:00 ./
drwxr-xr-x 6 root root 0 Jan 1 00:00 ../
-rw-r--r-- 1 root root 4096 Jan 1 00:00 brightness
lrwxrwxrwx 1 root root 0 Jan 1 00:00 device ->
../../../pwmleds/
-r--r--r-- 1 root root 4096 Jan 1 00:00 max_brightness
lrwxrwxrwx 1 root root 0 Jan 1 00:00 subsystem ->
../../../../../class/leds/
-rw-r--r-- 1 root root 0 Jan 1 00:00 trigger
-rw-r--r-- 1 root root 4096 Jan 1 00:00 uevent
Yours,
JaeJoon Jung
On Fri, 31 Jan 2020 at 01:14, David Abdurachmanov
<david.abdurachmanov@gmail.com> wrote:
>
> On Thu, Jan 30, 2020 at 7:24 AM JaeJoon Jung <rgbi3307@gmail.com> wrote:
> >
> > I agree with you because LEDs are user defined using method.
> >
> > And, I am sorry about that I confirmed lately below posted by Yash.
> >
> > https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf
> >
> > It is helpful for me and I am testing it.
> > If I find a bug, I am going to share about it.
> >
> > Thanks a lot, Have a nice day.
> >
> > Yours,
> > JaeJoon Jung
> >
> > On Thu, 30 Jan 2020 at 12:35, Paul Walmsley <paul.walmsley@sifive.com> wrote:
> > >
> > > On Tue, 21 Jan 2020, JaeJoon Jung wrote:
> > >
> > > > I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board.
> > > >
> > > > diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > > b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > > index a2e3d54e830c..b03bf570020c 100644
> > > > --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > > +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > >
> > > > + gpio0: gpio@10060000 {
> > > > + compatible = "sifive,fu540-c000-gpio", "sifive,gpio0";
> > > > + reg = <0x0 0x10060000 0x0 0x1000>;
> > > > + reg-names = "control";
> > > > + gpio-controller;
> > > > + #gpio-cells = <2>;
> > > > + ngpios = <16>;
> > > > + interrupt-controller;
> > > > + #interrupt-cells = <2>;
> > > > + interrupt-parent = <&plic0>;
> > > > + interrupts = <15 16 17 18 19 20 21 22 23 24
> > > > 25 26 27 28 29 30>;
> > > > + status = "disabled";
> > > > + };
> > >
> > > Yash posted this a while ago:
> > >
> > > https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf
> > >
> > >
> > > >
> > > > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > > index 88cfcb96bf23..f3f55dbbf737 100644
> > > > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > >
> > > > cpus {
> > > > @@ -41,6 +41,39 @@
> > > > clock-frequency = <RTCCLK_FREQ>;
> > > > clock-output-names = "rtcclk";
> > > > };
> > > > +
> > > > +
> > > > + pwmleds {
> > > > + compatible = "pwm-leds";
> > > > + heartbeat {
> > > > + label = "led1";
> > > > + max-brightness = <255>;
> > > > + active-low = <1>;
> > > > + pwms = <&pwm0 0 7812500 0>;
> > > > + linux,default-trigger = "heartbeat";
> > > > + };
> > > > + mtd {
> > > > + label = "led2";
> > > > + max-brightness = <255>;
> > > > + active-low = <1>;
> > > > + pwms = <&pwm0 1 7812500 0>;
> > > > + linux,default-trigger = "mtd";
> > > > + };
> > > > + netdev {
> > > > + label = "led3";
> > > > + max-brightness = <255>;
> > > > + active-low = <1>;
> > > > + pwms = <&pwm0 2 7812500 0>;
> > > > + linux,default-trigger = "netdev";
> > > > + };
> > > > + panic {
> > > > + label = "led4";
> > > > + max-brightness = <255>;
> > > > + active-low = <1>;
> > > > + pwms = <&pwm0 3 7812500 0>;
> > > > + linux,default-trigger = "panic";
> > > > + };
> > > > + };
> > > > };
> > >
> > > I don't think it's good to add these pwmleds to the default board DTS
> > > file. I realize that many upstream ARM development boards have added this
> > > type of configuration, but it gets in the way of reusing this DT file when
> > > integrators wish to have the LEDs used for different purposes. If the
> > > Unleashed silkscreen had text on it describing the LEDs as having these
> > > specific functions, or if Unleashed was sold in a case with similar
> > > markings on the outside, it'd be a different story, and then a change like
> > > the above could make sense.
>
> I think, there is a middle solution. On SiFive Unleashed PWM LEDs still exist.
> So let's define those LEDs, but disable them (i.e. no default trigger). In that
> case user has an options to write udev rules to setup those PWM LEDs as
> he/she likes. I already use udev rule for netdev LED configuration as today
> DTS does not provide options to configure it [within DTS] (patch
> posted in 2019).
>
> It would look something like below (not tested). We use labels from PCB and
> schematics, but do not assign any default functions/triggers. Once the
> distro boots
> udev rules will set default triggers and do required configuration.
>
> ---
> .../riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 27 ++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> index 828f406..eb793b5 100644
> --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> @@ -26,6 +26,33 @@
> };
>
> soc {
> + pwmleds {
> + compatible = "pwm-leds";
> + d1 {
> + label = "green:d1";
> + pwms = <&pwm0 0 2000000 0>;
> + max-brightness = <255>;
> + linux,default-trigger = "none";
> + };
> + d2 {
> + label = "green:d2";
> + pwms = <&pwm0 1 2000000 0>;
> + max-brightness = <255>;
> + linux,default-trigger = "none";
> + };
> + d3 {
> + label = "green:d3";
> + pwms = <&pwm0 2 2000000 0>;
> + max-brightness = <255>;
> + linux,default-trigger = "none";
> + };
> + d4 {
> + label = "green:d4";
> + pwms = <&pwm0 3 2000000 0>;
> + max-brightness = <255>;
> + linux,default-trigger = "none";
> + };
> + };
> };
>
> hfclk: hfclk {
> --
> 2.7.4
On Thu, Jan 30, 2020 at 6:20 PM JaeJoon Jung <rgbi3307@gmail.com> wrote: > > Hello David Abdurachmanov, > Thanks to your deep opinions, > but it's not that important, but there are things to check. > If you apply below DTS, It works all fine without source modification. > I think that you are missing the active-low = <1> and Everything works fine for me as-is, but I am using the old (non-upstream) GPIO driver right now. I am rebasing to a new one. > linux,default-trigger = "heartbeat"... The idea is not to do that (even if it's documented in SiFive Unleashed Manual). We leave the decision of triggers to udev rules (distro) and a user. You can change the trigger via sysfs knobs as a user (I tried it). Without these entries you don't have those PWM LEDs available on sysfs for udev/user to configure triggers. Just reminder again. We just upstream the fact that PWM LEDs exist on Unleashed, but not what they do. By default they do nothing until configured by udev rules (distro) or a user (e.g. via sysfs knobs directly). Example rules I use: SUBSYSTEM=="leds", KERNEL=="green:d3", ATTR{link}="1", ATTR{tx}="1", ATTR{rx}="1", ATTR{device_name}="eth0" This does require having PWM LEDs in DTS. david > > The following setting is not set by default, but it works well if you set it. > CONFIG_PWM_SIFIVE > CONFIG_NEW_LEDS > CONFIG_LEDS_PWM > CONFIG_LEDS_CLASS > CONFIG_LEDS_TRIGGERS > CONFIG_LEDS_TRIGGER_HEARTBEAT > > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > cpus { > @@ -41,6 +41,39 @@ > clock-frequency = <RTCCLK_FREQ>; > clock-output-names = "rtcclk"; > }; > + > + > + pwmleds { > + compatible = "pwm-leds"; > + heartbeat { > + label = "led1"; > + max-brightness = <255>; > + active-low = <1>; > + pwms = <&pwm0 0 1000000 0>; > + linux,default-trigger = "heartbeat"; > + }; > + mtd { > + label = "led2"; > + max-brightness = <255>; > + active-low = <1>; > + pwms = <&pwm0 1 1000000 0>; > + linux,default-trigger = "mtd"; > + }; > + netdev { > + label = "led3"; > + max-brightness = <255>; > + active-low = <1>; > + pwms = <&pwm0 2 1000000 0>; > + linux,default-trigger = "netdev"; > + }; > + panic { > + label = "led4"; > + max-brightness = <255>; > + active-low = <1>; > + pwms = <&pwm0 3 1000000 0>; > + linux,default-trigger = "panic"; > + }; > + }; > }; > > And I think that the simpler the name, the better. > labels are displayed at /sys/class/leds and user can access and control it here. > > RISCV-FU540:/sys/class/leds# pwd > /sys/class/leds > RISCV-FU540:/sys/class/leds# ll > total 0 > drwxr-xr-x 2 root root 0 Jan 1 00:00 ./ > drwxr-xr-x 33 root root 0 Jan 1 00:00 ../ > lrwxrwxrwx 1 root root 0 Jan 1 00:00 led1 -> > ../../devices/platform/pwmleds/leds/led1/ > lrwxrwxrwx 1 root root 0 Jan 1 00:00 led2 -> > ../../devices/platform/pwmleds/leds/led2/ > lrwxrwxrwx 1 root root 0 Jan 1 00:00 led3 -> > ../../devices/platform/pwmleds/leds/led3/ > lrwxrwxrwx 1 root root 0 Jan 1 00:00 led4 -> > ../../devices/platform/pwmleds/leds/led4/ > > RISCV-FU540:/sys/class/leds# cd led1 > RISCV-FU540:/sys/class/leds/led1# ll > total 0 > drwxr-xr-x 2 root root 0 Jan 1 00:00 ./ > drwxr-xr-x 6 root root 0 Jan 1 00:00 ../ > -rw-r--r-- 1 root root 4096 Jan 1 00:00 brightness > lrwxrwxrwx 1 root root 0 Jan 1 00:00 device -> > ../../../pwmleds/ > -r--r--r-- 1 root root 4096 Jan 1 00:00 max_brightness > lrwxrwxrwx 1 root root 0 Jan 1 00:00 subsystem -> > ../../../../../class/leds/ > -rw-r--r-- 1 root root 0 Jan 1 00:00 trigger > -rw-r--r-- 1 root root 4096 Jan 1 00:00 uevent > > Yours, > JaeJoon Jung > > > On Fri, 31 Jan 2020 at 01:14, David Abdurachmanov > <david.abdurachmanov@gmail.com> wrote: > > > > On Thu, Jan 30, 2020 at 7:24 AM JaeJoon Jung <rgbi3307@gmail.com> wrote: > > > > > > I agree with you because LEDs are user defined using method. > > > > > > And, I am sorry about that I confirmed lately below posted by Yash. > > > > > > https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf > > > > > > It is helpful for me and I am testing it. > > > If I find a bug, I am going to share about it. > > > > > > Thanks a lot, Have a nice day. > > > > > > Yours, > > > JaeJoon Jung > > > > > > On Thu, 30 Jan 2020 at 12:35, Paul Walmsley <paul.walmsley@sifive.com> wrote: > > > > > > > > On Tue, 21 Jan 2020, JaeJoon Jung wrote: > > > > > > > > > I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board. > > > > > > > > > > diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > > > > b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > > > > index a2e3d54e830c..b03bf570020c 100644 > > > > > --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > > > > +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi > > > > > > > > > > + gpio0: gpio@10060000 { > > > > > + compatible = "sifive,fu540-c000-gpio", "sifive,gpio0"; > > > > > + reg = <0x0 0x10060000 0x0 0x1000>; > > > > > + reg-names = "control"; > > > > > + gpio-controller; > > > > > + #gpio-cells = <2>; > > > > > + ngpios = <16>; > > > > > + interrupt-controller; > > > > > + #interrupt-cells = <2>; > > > > > + interrupt-parent = <&plic0>; > > > > > + interrupts = <15 16 17 18 19 20 21 22 23 24 > > > > > 25 26 27 28 29 30>; > > > > > + status = "disabled"; > > > > > + }; > > > > > > > > Yash posted this a while ago: > > > > > > > > https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf > > > > > > > > > > > > > > > > > > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > > > > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > > > > index 88cfcb96bf23..f3f55dbbf737 100644 > > > > > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > > > > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > > > > > > > > > cpus { > > > > > @@ -41,6 +41,39 @@ > > > > > clock-frequency = <RTCCLK_FREQ>; > > > > > clock-output-names = "rtcclk"; > > > > > }; > > > > > + > > > > > + > > > > > + pwmleds { > > > > > + compatible = "pwm-leds"; > > > > > + heartbeat { > > > > > + label = "led1"; > > > > > + max-brightness = <255>; > > > > > + active-low = <1>; > > > > > + pwms = <&pwm0 0 7812500 0>; > > > > > + linux,default-trigger = "heartbeat"; > > > > > + }; > > > > > + mtd { > > > > > + label = "led2"; > > > > > + max-brightness = <255>; > > > > > + active-low = <1>; > > > > > + pwms = <&pwm0 1 7812500 0>; > > > > > + linux,default-trigger = "mtd"; > > > > > + }; > > > > > + netdev { > > > > > + label = "led3"; > > > > > + max-brightness = <255>; > > > > > + active-low = <1>; > > > > > + pwms = <&pwm0 2 7812500 0>; > > > > > + linux,default-trigger = "netdev"; > > > > > + }; > > > > > + panic { > > > > > + label = "led4"; > > > > > + max-brightness = <255>; > > > > > + active-low = <1>; > > > > > + pwms = <&pwm0 3 7812500 0>; > > > > > + linux,default-trigger = "panic"; > > > > > + }; > > > > > + }; > > > > > }; > > > > > > > > I don't think it's good to add these pwmleds to the default board DTS > > > > file. I realize that many upstream ARM development boards have added this > > > > type of configuration, but it gets in the way of reusing this DT file when > > > > integrators wish to have the LEDs used for different purposes. If the > > > > Unleashed silkscreen had text on it describing the LEDs as having these > > > > specific functions, or if Unleashed was sold in a case with similar > > > > markings on the outside, it'd be a different story, and then a change like > > > > the above could make sense. > > > > I think, there is a middle solution. On SiFive Unleashed PWM LEDs still exist. > > So let's define those LEDs, but disable them (i.e. no default trigger). In that > > case user has an options to write udev rules to setup those PWM LEDs as > > he/she likes. I already use udev rule for netdev LED configuration as today > > DTS does not provide options to configure it [within DTS] (patch > > posted in 2019). > > > > It would look something like below (not tested). We use labels from PCB and > > schematics, but do not assign any default functions/triggers. Once the > > distro boots > > udev rules will set default triggers and do required configuration. > > > > --- > > .../riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 27 ++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > index 828f406..eb793b5 100644 > > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts > > @@ -26,6 +26,33 @@ > > }; > > > > soc { > > + pwmleds { > > + compatible = "pwm-leds"; > > + d1 { > > + label = "green:d1"; > > + pwms = <&pwm0 0 2000000 0>; > > + max-brightness = <255>; > > + linux,default-trigger = "none"; > > + }; > > + d2 { > > + label = "green:d2"; > > + pwms = <&pwm0 1 2000000 0>; > > + max-brightness = <255>; > > + linux,default-trigger = "none"; > > + }; > > + d3 { > > + label = "green:d3"; > > + pwms = <&pwm0 2 2000000 0>; > > + max-brightness = <255>; > > + linux,default-trigger = "none"; > > + }; > > + d4 { > > + label = "green:d4"; > > + pwms = <&pwm0 3 2000000 0>; > > + max-brightness = <255>; > > + linux,default-trigger = "none"; > > + }; > > + }; > > }; > > > > hfclk: hfclk { > > -- > > 2.7.4
diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi index a2e3d54e830c..b03bf570020c 100644 --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts index 88cfcb96bf23..f3f55dbbf737 100644 --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board. +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi + gpio0: gpio@10060000 { + compatible = "sifive,fu540-c000-gpio", "sifive,gpio0"; + reg = <0x0 0x10060000 0x0 0x1000>; + reg-names = "control"; + gpio-controller; + #gpio-cells = <2>; + ngpios = <16>; + interrupt-controller; + #interrupt-cells = <2>; + interrupt-parent = <&plic0>; + interrupts = <15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30>; + status = "disabled"; + }; +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts cpus { @@ -41,6 +41,39 @@ clock-frequency = <RTCCLK_FREQ>; clock-output-names = "rtcclk"; }; + + + pwmleds { + compatible = "pwm-leds"; + heartbeat { + label = "led1"; + max-brightness = <255>; + active-low = <1>; + pwms = <&pwm0 0 7812500 0>; + linux,default-trigger = "heartbeat"; + }; + mtd { + label = "led2"; + max-brightness = <255>; + active-low = <1>; + pwms = <&pwm0 1 7812500 0>; + linux,default-trigger = "mtd"; + }; + netdev { + label = "led3"; + max-brightness = <255>; + active-low = <1>; + pwms = <&pwm0 2 7812500 0>; + linux,default-trigger = "netdev"; + }; + panic { + label = "led4"; + max-brightness = <255>; + active-low = <1>; + pwms = <&pwm0 3 7812500 0>; + linux,default-trigger = "panic"; + }; + }; }; &uart0 { @@ -94,3 +127,7 @@ &pwm1 { status = "okay"; }; + +&gpio0 { + status = "okay"; +}; If apply above DTS, the gpio-sifive driver works well without source modification. drivers/gpio/gpio-sifive.c drivers/leds/leds-pwm.c I have checked below: led1(D1) is acting well as heartbeat. RISCV-FU540:/sys/class/leds# pwd /sys/class/leds RISCV-FU540:/sys/class/leds# ll total 0 drwxr-xr-x 2 root root 0 Jan 1 00:00 ./ drwxr-xr-x 32 root root 0 Jan 1 00:00 ../ lrwxrwxrwx 1 root root 0 Jan 1 00:00 led1 -> ../../devices/platform/pwmleds/leds/led1/ lrwxrwxrwx 1 root root 0 Jan 1 00:00 led2 -> ../../devices/platform/pwmleds/leds/led2/ lrwxrwxrwx 1 root root 0 Jan 1 00:00 led3 -> ../../devices/platform/pwmleds/leds/led3/ lrwxrwxrwx 1 root root 0 Jan 1 00:00 led4 -> ../../devices/platform/pwmleds/leds/led4/ RISCV-FU540:/sys/class/leds# cd led3 RISCV-FU540:/sys/class/leds/led3# ll total 0 drwxr-xr-x 2 root root 0 Jan 1 00:00 ./ drwxr-xr-x 6 root root 0 Jan 1 00:00 ../ -rw-r--r-- 1 root root 4096 Jan 1 00:01 brightness lrwxrwxrwx 1 root root 0 Jan 1 00:00 device -> ../../../pwmleds/ -r--r--r-- 1 root root 4096 Jan 1 00:00 max_brightness lrwxrwxrwx 1 root root 0 Jan 1 00:00 subsystem -> ../../../../../class/leds/ -rw-r--r-- 1 root root 4096 Jan 1 00:00 trigger -rw-r--r-- 1 root root 4096 Jan 1 00:00 uevent RISCV-FU540:/sys/class/leds/led3# echo 1 > brightness RISCV-FU540:/sys/class/leds/led3# echo 127 > brightness RISCV-FU540:/sys/class/leds/led3# echo 255 > brightness leds(D1, D2, D3, D4) are acting well as pwm brightness.