mbox series

[0/4] cpufreq support for the Raspberry Pi

Message ID 20190604173223.4229-1-nsaenzjulienne@suse.de (mailing list archive)
Headers show
Series cpufreq support for the Raspberry Pi | expand

Message

Nicolas Saenz Julienne June 4, 2019, 5:32 p.m. UTC
Hi all,
this series aims at adding cpufreq support to the Raspberry Pi family of
boards.

The previous revision can be found at: https://lkml.org/lkml/2019/5/20/431

The series first factors out 'pllb' from clk-bcm2385 and creates a new
clk driver that operates it over RPi's firmware interface[1]. We are
forced to do so as the firmware 'owns' the pll and we're not allowed to
change through the register interface directly as we might race with the
over-temperature and under-voltage protections provided by the firmware.

Next it creates a minimal cpufreq driver that populates the CPU's opp
table, and registers cpufreq-dt. Which is needed as the firmware
controls the max and min frequencies available.

This was tested on a RPi3b+ and RPI2b which are the boards I have access
to. Until this is tested broadly the cpufreq driver takes care of
filtering out the rest of boards.

That's all,
kind regards,
Nicolas

[1] https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface

---

Changes since RFC:
  - Addressed Viresh's comments in cpufreq driver
  - Addressed Stefan's comments in both cpufreq & clk drivers
  - Moved all firmware clk operations into it's own driver

Nicolas Saenz Julienne (4):
  clk: bcm2835: remove pllb
  clk: bcm283x: add driver interfacing with Raspberry Pi's firmware
  clk: bcm2835: register Raspberry Pi's firmware clk device
  cpufreq: add driver for Raspbery Pi

 drivers/clk/bcm/Makefile              |   1 +
 drivers/clk/bcm/clk-bcm2835.c         |  40 ++--
 drivers/clk/bcm/clk-raspberrypi.c     | 316 ++++++++++++++++++++++++++
 drivers/cpufreq/Kconfig.arm           |   8 +
 drivers/cpufreq/Makefile              |   1 +
 drivers/cpufreq/raspberrypi-cpufreq.c |  84 +++++++
 6 files changed, 423 insertions(+), 27 deletions(-)
 create mode 100644 drivers/clk/bcm/clk-raspberrypi.c
 create mode 100644 drivers/cpufreq/raspberrypi-cpufreq.c

Comments

Stefan Wahren June 5, 2019, 9:46 a.m. UTC | #1
Hi Nicolas,

Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
> Hi all,
> this series aims at adding cpufreq support to the Raspberry Pi family of
> boards.
>
> The previous revision can be found at: https://lkml.org/lkml/2019/5/20/431
>
> The series first factors out 'pllb' from clk-bcm2385 and creates a new
> clk driver that operates it over RPi's firmware interface[1]. We are
> forced to do so as the firmware 'owns' the pll and we're not allowed to
> change through the register interface directly as we might race with the
> over-temperature and under-voltage protections provided by the firmware.
it would be nice to preserve such design decision in the driver as a
comment, because the cover letter usually get lost.
>
> Next it creates a minimal cpufreq driver that populates the CPU's opp
> table, and registers cpufreq-dt. Which is needed as the firmware
> controls the max and min frequencies available.

I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
manually enable this drivers. During boot with Raspbian rootfs i'm
getting the following:

[    1.177009] cpu cpu0: failed to get clock: -2
[    1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2
[    1.192117] sdhci: Secure Digital Host Controller Interface driver
[    1.198725] sdhci: Copyright(c) Pierre Ossman
[    1.207005] Synopsys Designware Multimedia Card Interface Driver
[    1.319936] sdhost-bcm2835 3f202000.mmc: loaded - DMA enabled (>1)
[    1.326641] sdhci-pltfm: SDHCI platform and OF driver helper
[    1.336568] ledtrig-cpu: registered to indicate activity on CPUs
[    1.343713] usbcore: registered new interface driver usbhid
[    1.350275] usbhid: USB HID core driver
[    1.357639] bcm2835-mbox 3f00b880.mailbox: mailbox enabled
[    1.367490] NET: Registered protocol family 10
[    1.375013] Segment Routing with IPv6
[    1.381696] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
[    1.388980] NET: Registered protocol family 17
[    1.395624] can: controller area network core (rev 20170425 abi 9)
[    1.402358] NET: Registered protocol family 29
[    1.408997] can: raw protocol (rev 20170425)
[    1.415599] can: broadcast manager protocol (rev 20170425 t)
[    1.422219] can: netlink gateway (rev 20170425) max_hops=1
[    1.429369] Key type dns_resolver registered
[    1.437190] Registering SWP/SWPB emulation handler
[    1.444443] Loading compiled-in X.509 certificates
[    1.455693] 3f201000.serial: ttyAMA0 at MMIO 0x3f201000 (irq = 81,
base_baud = 0) is a PL011 rev2
[    1.462768] serial serial0: tty port ttyAMA0 registered
[    1.478755] mmc0: host does not support reading read-only switch,
assuming write-enable
[    1.488792] mmc0: new high speed SDHC card at address 0007
[    1.495766] raspberrypi-firmware soc:firmware: Attached to firmware
from 2019-03-27 15:45
[    1.496862] mmcblk0: mmc0:0007 SDCIT 14.6 GiB
[    1.512768] raspberrypi-clk raspberrypi-clk: CPU frequency range: min
600000000, max 1400000000
[    1.513012]  mmcblk0: p1 p2
[    1.558085] dwc2 3f980000.usb: 3f980000.usb supply vusb_d not found,
using dummy regulator
[    1.565355] dwc2 3f980000.usb: 3f980000.usb supply vusb_a not found,
using dummy regulator
[    1.623246] dwc2 3f980000.usb: DWC OTG Controller
[    1.630318] dwc2 3f980000.usb: new USB bus registered, assigned bus
number 1
[    1.637439] dwc2 3f980000.usb: irq 33, io mem 0x3f980000
[    1.645268] hub 1-0:1.0: USB hub found
[    1.652317] hub 1-0:1.0: 1 port detected
[    1.665867] sdhci-iproc 3f300000.sdhci: allocated mmc-pwrseq
[    1.704788] mmc1: SDHCI controller on 3f300000.sdhci [3f300000.sdhci]
using PIO
[    1.717694] hctosys: unable to open rtc device (rtc0)
[    1.724967] sysfs: cannot create duplicate filename
'/devices/platform/cpufreq-dt'
[    1.732120] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
5.2.0-rc1-00004-g5aa6d98-dirty #2
[    1.739288] Hardware name: BCM2835
[    1.746421] [<c0312304>] (unwind_backtrace) from [<c030cc08>]
(show_stack+0x10/0x14)
[    1.753636] [<c030cc08>] (show_stack) from [<c0e7d358>]
(dump_stack+0xb4/0xc8)
[    1.760840] [<c0e7d358>] (dump_stack) from [<c0503b64>]
(sysfs_warn_dup+0x58/0x64)
[    1.768105] [<c0503b64>] (sysfs_warn_dup) from [<c0503c8c>]
(sysfs_create_dir_ns+0xd8/0xe8)
[    1.775481] [<c0503c8c>] (sysfs_create_dir_ns) from [<c0e82520>]
(kobject_add_internal+0xb0/0x2fc)
[    1.782958] [<c0e82520>] (kobject_add_internal) from [<c0e827c8>]
(kobject_add+0x5c/0xc0)
[    1.790534] [<c0e827c8>] (kobject_add) from [<c096b1cc>]
(device_add+0xf8/0x608)
[    1.798180] [<c096b1cc>] (device_add) from [<c0971098>]
(platform_device_add+0x110/0x214)
[    1.805945] [<c0971098>] (platform_device_add) from [<c0971ae4>]
(platform_device_register_full+0x130/0x148)
[    1.813866] [<c0971ae4>] (platform_device_register_full) from
[<c15a7a30>] (raspberrypi_cpufreq_driver_init+0x128/0x178)
[    1.821916] [<c15a7a30>] (raspberrypi_cpufreq_driver_init) from
[<c0302eec>] (do_one_initcall+0x54/0x21c)
[    1.830099] [<c0302eec>] (do_one_initcall) from [<c15010f8>]
(kernel_init_freeable+0x244/0x2e0)
[    1.838312] [<c15010f8>] (kernel_init_freeable) from [<c0e949d4>]
(kernel_init+0x8/0x10c)
[    1.846541] [<c0e949d4>] (kernel_init) from [<c03010e8>]
(ret_from_fork+0x14/0x2c)
[    1.854783] Exception stack(0xea89dfb0 to 0xea89dff8)
[    1.863036] dfa0:                                     00000000
00000000 00000000 00000000
[    1.871450] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    1.879860] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    1.888251] kobject_add_internal failed for cpufreq-dt with -EEXIST,
don't try to register things with the same name in the same directory.
[    1.896910] cpu cpu0: Failed to create platform device, -17

>
> This was tested on a RPi3b+ and RPI2b which are the boards I have access
> to. Until this is tested broadly the cpufreq driver takes care of
> filtering out the rest of boards.
Unfortunately this makes it harder to test on other boards. So i welcome
your decision to remove it.
>
> That's all,
> kind regards,
> Nicolas
>
> [1] https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
>
> ---
>
> Changes since RFC:
>   - Addressed Viresh's comments in cpufreq driver
>   - Addressed Stefan's comments in both cpufreq & clk drivers
Just a note for the future, this make it hard for other reviewers to
follow. I don't really consist of the credits, it is more important to
mention what has changed. But it's not necessary to mention every single
change.
Nicolas Saenz Julienne June 5, 2019, 11 a.m. UTC | #2
Hi Stefan,
thanks for the review, I took note of your code comments.

On Wed, 2019-06-05 at 11:46 +0200, Stefan Wahren wrote:
> Hi Nicolas,
> 
> Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
> > Hi all,
> > this series aims at adding cpufreq support to the Raspberry Pi family of
> > boards.
> > 
> > The previous revision can be found at: https://lkml.org/lkml/2019/5/20/431
> > 
> > The series first factors out 'pllb' from clk-bcm2385 and creates a new
> > clk driver that operates it over RPi's firmware interface[1]. We are
> > forced to do so as the firmware 'owns' the pll and we're not allowed to
> > change through the register interface directly as we might race with the
> > over-temperature and under-voltage protections provided by the firmware.
> it would be nice to preserve such design decision in the driver as a
> comment, because the cover letter usually get lost.
> > Next it creates a minimal cpufreq driver that populates the CPU's opp
> > table, and registers cpufreq-dt. Which is needed as the firmware
> > controls the max and min frequencies available.
> 
> I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
> manually enable this drivers. During boot with Raspbian rootfs i'm
> getting the following:
> 
> [    1.177009] cpu cpu0: failed to get clock: -2
> [    1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2

This is surprising, who could be creating a platform_device for cpufreq-dt
apart from raspberrypi-cpufreq? Just to make things clear, you're using the
device tree from v5.2-rc1 (as opposed to the Raspbian one)?

> [    1.192117] sdhci: Secure Digital Host Controller Interface driver
> [    1.198725] sdhci: Copyright(c) Pierre Ossman
> [    1.207005] Synopsys Designware Multimedia Card Interface Driver
> [    1.319936] sdhost-bcm2835 3f202000.mmc: loaded - DMA enabled (>1)
> [    1.326641] sdhci-pltfm: SDHCI platform and OF driver helper
> [    1.336568] ledtrig-cpu: registered to indicate activity on CPUs
> [    1.343713] usbcore: registered new interface driver usbhid
> [    1.350275] usbhid: USB HID core driver
> [    1.357639] bcm2835-mbox 3f00b880.mailbox: mailbox enabled
> [    1.367490] NET: Registered protocol family 10
> [    1.375013] Segment Routing with IPv6
> [    1.381696] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
> [    1.388980] NET: Registered protocol family 17
> [    1.395624] can: controller area network core (rev 20170425 abi 9)
> [    1.402358] NET: Registered protocol family 29
> [    1.408997] can: raw protocol (rev 20170425)
> [    1.415599] can: broadcast manager protocol (rev 20170425 t)
> [    1.422219] can: netlink gateway (rev 20170425) max_hops=1
> [    1.429369] Key type dns_resolver registered
> [    1.437190] Registering SWP/SWPB emulation handler
> [    1.444443] Loading compiled-in X.509 certificates
> [    1.455693] 3f201000.serial: ttyAMA0 at MMIO 0x3f201000 (irq = 81,
> base_baud = 0) is a PL011 rev2
> [    1.462768] serial serial0: tty port ttyAMA0 registered
> [    1.478755] mmc0: host does not support reading read-only switch,
> assuming write-enable
> [    1.488792] mmc0: new high speed SDHC card at address 0007
> [    1.495766] raspberrypi-firmware soc:firmware: Attached to firmware
> from 2019-03-27 15:45
> [    1.496862] mmcblk0: mmc0:0007 SDCIT 14.6 GiB
> [    1.512768] raspberrypi-clk raspberrypi-clk: CPU frequency range: min
> 600000000, max 1400000000
> [    1.513012]  mmcblk0: p1 p2
> [    1.558085] dwc2 3f980000.usb: 3f980000.usb supply vusb_d not found,
> using dummy regulator
> [    1.565355] dwc2 3f980000.usb: 3f980000.usb supply vusb_a not found,
> using dummy regulator
> [    1.623246] dwc2 3f980000.usb: DWC OTG Controller
> [    1.630318] dwc2 3f980000.usb: new USB bus registered, assigned bus
> number 1
> [    1.637439] dwc2 3f980000.usb: irq 33, io mem 0x3f980000
> [    1.645268] hub 1-0:1.0: USB hub found
> [    1.652317] hub 1-0:1.0: 1 port detected
> [    1.665867] sdhci-iproc 3f300000.sdhci: allocated mmc-pwrseq
> [    1.704788] mmc1: SDHCI controller on 3f300000.sdhci [3f300000.sdhci]
> using PIO
> [    1.717694] hctosys: unable to open rtc device (rtc0)
> [    1.724967] sysfs: cannot create duplicate filename
> '/devices/platform/cpufreq-dt'

For the record, this is raspberrypi-cpufreq creating the platform device for
cpufreq-dt.

> [    1.732120] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
> 5.2.0-rc1-00004-g5aa6d98-dirty #2
> [    1.739288] Hardware name: BCM2835
> [    1.746421] [<c0312304>] (unwind_backtrace) from [<c030cc08>]
> (show_stack+0x10/0x14)
> [    1.753636] [<c030cc08>] (show_stack) from [<c0e7d358>]
> (dump_stack+0xb4/0xc8)
> [    1.760840] [<c0e7d358>] (dump_stack) from [<c0503b64>]
> (sysfs_warn_dup+0x58/0x64)
> [    1.768105] [<c0503b64>] (sysfs_warn_dup) from [<c0503c8c>]
> (sysfs_create_dir_ns+0xd8/0xe8)
> [    1.775481] [<c0503c8c>] (sysfs_create_dir_ns) from [<c0e82520>]
> (kobject_add_internal+0xb0/0x2fc)
> [    1.782958] [<c0e82520>] (kobject_add_internal) from [<c0e827c8>]
> (kobject_add+0x5c/0xc0)
> [    1.790534] [<c0e827c8>] (kobject_add) from [<c096b1cc>]
> (device_add+0xf8/0x608)
> [    1.798180] [<c096b1cc>] (device_add) from [<c0971098>]
> (platform_device_add+0x110/0x214)
> [    1.805945] [<c0971098>] (platform_device_add) from [<c0971ae4>]
> (platform_device_register_full+0x130/0x148)
> [    1.813866] [<c0971ae4>] (platform_device_register_full) from
> [<c15a7a30>] (raspberrypi_cpufreq_driver_init+0x128/0x178)
> [    1.821916] [<c15a7a30>] (raspberrypi_cpufreq_driver_init) from
> [<c0302eec>] (do_one_initcall+0x54/0x21c)
> [    1.830099] [<c0302eec>] (do_one_initcall) from [<c15010f8>]
> (kernel_init_freeable+0x244/0x2e0)
> [    1.838312] [<c15010f8>] (kernel_init_freeable) from [<c0e949d4>]
> (kernel_init+0x8/0x10c)
> [    1.846541] [<c0e949d4>] (kernel_init) from [<c03010e8>]
> (ret_from_fork+0x14/0x2c)
> [    1.854783] Exception stack(0xea89dfb0 to 0xea89dff8)
> [    1.863036] dfa0:                                     00000000
> 00000000 00000000 00000000
> [    1.871450] dfc0: 00000000 00000000 00000000 00000000 00000000
> 00000000 00000000 00000000
> [    1.879860] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    1.888251] kobject_add_internal failed for cpufreq-dt with -EEXIST,
> don't try to register things with the same name in the same directory.
> [    1.896910] cpu cpu0: Failed to create platform device, -17

Regards,
Nicolas
Stefan Wahren June 5, 2019, 11:34 a.m. UTC | #3
Hi,

Am 05.06.19 um 13:00 schrieb Nicolas Saenz Julienne:
> Hi Stefan,
> thanks for the review, I took note of your code comments.
>
> On Wed, 2019-06-05 at 11:46 +0200, Stefan Wahren wrote:
>> Hi Nicolas,
>>
>> Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
>>> Hi all,
>>> this series aims at adding cpufreq support to the Raspberry Pi family of
>>> boards.
>>>
>>> The previous revision can be found at: https://lkml.org/lkml/2019/5/20/431
>>>
>>> The series first factors out 'pllb' from clk-bcm2385 and creates a new
>>> clk driver that operates it over RPi's firmware interface[1]. We are
>>> forced to do so as the firmware 'owns' the pll and we're not allowed to
>>> change through the register interface directly as we might race with the
>>> over-temperature and under-voltage protections provided by the firmware.
>> it would be nice to preserve such design decision in the driver as a
>> comment, because the cover letter usually get lost.
>>> Next it creates a minimal cpufreq driver that populates the CPU's opp
>>> table, and registers cpufreq-dt. Which is needed as the firmware
>>> controls the max and min frequencies available.
>> I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
>> manually enable this drivers. During boot with Raspbian rootfs i'm
>> getting the following:
>>
>> [    1.177009] cpu cpu0: failed to get clock: -2
>> [    1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2
> This is surprising, who could be creating a platform_device for cpufreq-dt
> apart from raspberrypi-cpufreq? Just to make things clear, you're using the
> device tree from v5.2-rc1 (as opposed to the Raspbian one)?

sorry my fault, i thought it already has been replaced. The behavior in
this unexpected case is fine, since it doesn't crash.

I replaced the the DTB with the mainline one, but now i'm getting this:

[    4.566068] cpufreq: cpufreq_online: CPU0: Running at unlisted freq:
600000 KHz
[    4.580690] cpu cpu0: dev_pm_opp_set_rate: Invalid target frequency 0
[    4.594391] cpufreq: __target_index: Failed to change cpu frequency: -22
[    4.608413] ------------[ cut here ]------------
[    4.620203] kernel BUG at drivers/cpufreq/cpufreq.c:1348!
[    4.632787] Internal error: Oops - BUG: 0 [#1] SMP ARM
[    4.645062] Modules linked in:
[    4.655147] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G        W        
5.2.0-rc1-00004-g5aa6d98-dirty #2
[    4.671891] Hardware name: BCM2835
[    4.682549] PC is at cpufreq_online+0x690/0x6b0
[    4.694409] LR is at __wake_up_common_lock+0xa0/0xc8
[    4.706744] pc : [<c0c696c0>]    lr : [<c03860d4>]    psr: a0000013
[    4.720518] sp : ea89dce8  ip : eaa1ace0  fp : c1704d54
[    4.733288] r10: eaa1acb4  r9 : c1673440  r8 : c1705038
[    4.746085] r7 : c18e6728  r6 : 000927c0  r5 : 00000000  r4 : eaa1ac00
[    4.760288] r3 : d89db0bd  r2 : d89db0bd  r1 : 60000013  r0 : ffffffea
[    4.774541] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM 
Segment none
[    4.789431] Control: 10c5383d  Table: 0020406a  DAC: 00000051
[    4.802897] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
[    4.816661] Stack: (0xea89dce8 to 0xea89e000)
[    4.828744] dce0:                   00000000 d89db0bd eaa1ac04
00000001 ea89dd28 00000000
[    4.844876] dd00: c184539c c1704c48 00000000 c186de1c 0000000f
00000000 c15dc83c c0c6975c
[    4.861033] dd20: c186d9e4 c096dbbc ea891358 ea9a5db4 00000000
d89db0bd c186de68 c18e6728
[    4.877227] dd40: c186d99c c0c66ffc eaff1800 c0c6bda0 00000000
eb80c050 eaff1800 c0c6c148
[    4.893461] dd60: eaff1810 00000000 c186de1c c097126c c18e16a4
eaff1810 c18e16a8 c096f314
[    4.909769] dd80: eaff1810 c186de1c c096f800 c1704c48 00000001
00000000 c1667b7c c096f64c
[    4.926077] dda0: c186de1c ea89ddfc eaff1810 00000000 ea89ddfc
c096f800 c1704c48 00000001
[    4.942410] ddc0: 00000000 c1667b7c c15dc83c c096d864 00000000
ea89126c eaf85138 d89db0bd
[    4.958798] dde0: eaff1810 eaff1810 c1704c48 eaff1854 c1704c48
c096f1b0 c1667b7c eaff1810
[    4.975236] de00: 00000001 d89db0bd eaff1810 eaff1810 c1845308
c1704c48 00000000 c096e574
[    4.991705] de20: eaff1810 c1845138 00000000 c096b490 00000000
c0e822d8 c1704c48 ea89dea0
[    5.008233] de40: eaff1800 d89db0bd ea89de60 eaff1800 00000000
eaff1800 eb80c050 00000000
[    5.024770] de60: eaff1810 c0971098 eaff1800 ea89dea0 c1704c48
eb80c050 00000000 05f5e100
[    5.041302] de80: c1667b7c c0971ae4 00000000 23c34600 c1704c48
eb80c050 00000000 c15a7a30
[    5.057796] dea0: 00000000 00000000 00000000 c1361770 ffffffff
00000000 00000000 00000000
[    5.074269] dec0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 d89db0bd
[    5.090693] dee0: 00000167 c1704c48 c188d420 ffffe000 00000000
c15a7908 00000167 c0302eec
[    5.107119] df00: 00000167 c0365a5c c13ff448 c130e700 00000000
00000007 00000007 c124057c
[    5.123539] df20: 00000000 c1704c48 c1251c04 c12405f0 00000000
ebfffc7d 00000000 d89db0bd
[    5.139941] df40: 00000000 c188d420 00000008 d89db0bd c188d420
00000008 c1899800 c1899800
[    5.156355] df60: c15dc838 c15010f8 00000007 00000007 00000000
c150067c c0e949cc 00000000
[    5.172779] df80: 00000000 00000000 c0e949cc 00000000 00000000
00000000 00000000 00000000
[    5.189192] dfa0: 00000000 c0e949d4 00000000 c03010e8 00000000
00000000 00000000 00000000
[    5.205577] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    5.221915] dfe0: 00000000 00000000 00000000 00000000 00000013
00000000 00000000 00000000
[    5.238205] [<c0c696c0>] (cpufreq_online) from [<c0c6975c>]
(cpufreq_add_dev+0x6c/0x78)
[    5.254372] [<c0c6975c>] (cpufreq_add_dev) from [<c096dbbc>]
(subsys_interface_register+0xa0/0xec)
[    5.271557] [<c096dbbc>] (subsys_interface_register) from
[<c0c66ffc>] (cpufreq_register_driver+0x14c/0x20c)
[    5.289700] [<c0c66ffc>] (cpufreq_register_driver) from [<c0c6c148>]
(dt_cpufreq_probe+0x94/0x114)
[    5.307002] [<c0c6c148>] (dt_cpufreq_probe) from [<c097126c>]
(platform_drv_probe+0x48/0x98)
[    5.323802] [<c097126c>] (platform_drv_probe) from [<c096f314>]
(really_probe+0xf0/0x2c8)
[    5.340390] [<c096f314>] (really_probe) from [<c096f64c>]
(driver_probe_device+0x60/0x164)
[    5.357113] [<c096f64c>] (driver_probe_device) from [<c096d864>]
(bus_for_each_drv+0x58/0xb8)
[    5.374142] [<c096d864>] (bus_for_each_drv) from [<c096f1b0>]
(__device_attach+0xd0/0x13c)
[    5.390937] [<c096f1b0>] (__device_attach) from [<c096e574>]
(bus_probe_device+0x84/0x8c)
[    5.407684] [<c096e574>] (bus_probe_device) from [<c096b490>]
(device_add+0x3bc/0x608)
[    5.424208] [<c096b490>] (device_add) from [<c0971098>]
(platform_device_add+0x110/0x214)
[    5.440983] [<c0971098>] (platform_device_add) from [<c0971ae4>]
(platform_device_register_full+0x130/0x148)
[    5.459471] [<c0971ae4>] (platform_device_register_full) from
[<c15a7a30>] (raspberrypi_cpufreq_driver_init+0x128/0x178)
[    5.479053] [<c15a7a30>] (raspberrypi_cpufreq_driver_init) from
[<c0302eec>] (do_one_initcall+0x54/0x21c)
[    5.497336] [<c0302eec>] (do_one_initcall) from [<c15010f8>]
(kernel_init_freeable+0x244/0x2e0)
[    5.514735] [<c15010f8>] (kernel_init_freeable) from [<c0e949d4>]
(kernel_init+0x8/0x10c)
[    5.531590] [<c0e949d4>] (kernel_init) from [<c03010e8>]
(ret_from_fork+0x14/0x2c)
[    5.547768] Exception stack(0xea89dfb0 to 0xea89dff8)
[    5.561323] dfa0:                                     00000000
00000000 00000000 00000000
[    5.578116] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    5.594962] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    5.610133] Code: e59f1024 e34c0136 ebdcbce4 eaffff63 (e7f001f2)
[    5.624789] ---[ end trace 7aaf0f77e232247e ]---
[    5.637981] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0000000b
[    5.654308] CPU3: stopping
[    5.665609] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G      D W        
5.2.0-rc1-00004-g5aa6d98-dirty #2
[    5.683854] Hardware name: BCM2835
[    5.695927] [<c0312304>] (unwind_backtrace) from [<c030cc08>]
(show_stack+0x10/0x14)
[    5.712502] [<c030cc08>] (show_stack) from [<c0e7d358>]
(dump_stack+0xb4/0xc8)
[    5.728537] [<c0e7d358>] (dump_stack) from [<c03107c8>]
(handle_IPI+0x3bc/0x3dc)
[    5.744710] [<c03107c8>] (handle_IPI) from [<c0301a8c>]
(__irq_svc+0x6c/0x90)
[    5.760601] Exception stack(0xea8d7f60 to 0xea8d7fa8)
[    5.774348] 7f60: 00000000 000004e4 eb854ae0 c031d840 ffffe000
c1704c6c c1704cac 00000008
[    5.791381] 7f80: 00000000 c1704c48 c1673568 00000000 00000002
ea8d7fb0 c0309164 c0309168
[    5.808421] 7fa0: 60000013 ffffffff
[    5.820668] [<c0301a8c>] (__irq_svc) from [<c0309168>]
(arch_cpu_idle+0x38/0x3c)
[    5.836950] [<c0309168>] (arch_cpu_idle) from [<c03747c4>]
(do_idle+0x1bc/0x298)
[    5.853198] [<c03747c4>] (do_idle) from [<c0374b3c>]
(cpu_startup_entry+0x18/0x1c)
[    5.869667] [<c0374b3c>] (cpu_startup_entry) from [<003025cc>]
(0x3025cc)
[    5.885362] CPU1: stopping
[    5.896857] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G      D W        
5.2.0-rc1-00004-g5aa6d98-dirty #2
[    5.915295] Hardware name: BCM2835
[    5.927623] [<c0312304>] (unwind_backtrace) from [<c030cc08>]
(show_stack+0x10/0x14)
[    5.944490] [<c030cc08>] (show_stack) from [<c0e7d358>]
(dump_stack+0xb4/0xc8)
[    5.960861] [<c0e7d358>] (dump_stack) from [<c03107c8>]
(handle_IPI+0x3bc/0x3dc)
[    5.977364] [<c03107c8>] (handle_IPI) from [<c0301a8c>]
(__irq_svc+0x6c/0x90)
[    5.993562] Exception stack(0xea8d3f60 to 0xea8d3fa8)
[    6.007632] 3f60: 00000000 00002930 eb82cae0 c031d840 ffffe000
c1704c6c c1704cac 00000002
[    6.024974] 3f80: 00000000 c1704c48 c1673568 00000000 00000000
ea8d3fb0 c0309164 c0309168
[    6.042285] 3fa0: 60000013 ffffffff
[    6.054772] [<c0301a8c>] (__irq_svc) from [<c0309168>]
(arch_cpu_idle+0x38/0x3c)
[    6.071321] [<c0309168>] (arch_cpu_idle) from [<c03747c4>]
(do_idle+0x1bc/0x298)
[    6.087880] [<c03747c4>] (do_idle) from [<c0374b3c>]
(cpu_startup_entry+0x18/0x1c)
[    6.104610] [<c0374b3c>] (cpu_startup_entry) from [<003025cc>]
(0x3025cc)
[    6.120535] CPU0: stopping
[    6.132240] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G      D W        
5.2.0-rc1-00004-g5aa6d98-dirty #2
[    6.150859] Hardware name: BCM2835
[    6.163308] [<c0312304>] (unwind_backtrace) from [<c030cc08>]
(show_stack+0x10/0x14)
[    6.180258] [<c030cc08>] (show_stack) from [<c0e7d358>]
(dump_stack+0xb4/0xc8)
[    6.196660] [<c0e7d358>] (dump_stack) from [<c03107c8>]
(handle_IPI+0x3bc/0x3dc)
[    6.213214] [<c03107c8>] (handle_IPI) from [<c0301a8c>]
(__irq_svc+0x6c/0x90)
[    6.229479] Exception stack(0xc1701f10 to 0xc1701f58)
[    6.243636] 1f00:                                     00000000
0000190c eb818ae0 c031d840
[    6.261073] 1f20: ffffe000 c1704c6c c1704cac 00000001 00000000
c1704c48 c1673568 00000000
[    6.278565] 1f40: 00000002 c1701f60 c0309164 c0309168 60000013 ffffffff
[    6.294466] [<c0301a8c>] (__irq_svc) from [<c0309168>]
(arch_cpu_idle+0x38/0x3c)
[    6.311211] [<c0309168>] (arch_cpu_idle) from [<c03747c4>]
(do_idle+0x1bc/0x298)
[    6.327979] [<c03747c4>] (do_idle) from [<c0374b3c>]
(cpu_startup_entry+0x18/0x1c)
[    6.344934] [<c0374b3c>] (cpu_startup_entry) from [<c1500e88>]
(start_kernel+0x460/0x48c)
[    6.362527] [<c1500e88>] (start_kernel) from [<00000000>] (0x0)
[    6.377824] ---[ end Kernel panic - not syncing: Attempted to kill
init! exitcode=0x0000000b ]---
Nicolas Saenz Julienne June 5, 2019, 12:27 p.m. UTC | #4
Hi Stefan,

On Wed, 2019-06-05 at 13:34 +0200, Stefan Wahren wrote:
> Hi,
> 
> Am 05.06.19 um 13:00 schrieb Nicolas Saenz Julienne:
> > Hi Stefan,
> > thanks for the review, I took note of your code comments.
> > 
> > On Wed, 2019-06-05 at 11:46 +0200, Stefan Wahren wrote:
> > > Hi Nicolas,
> > > 
> > > Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
> > > > Hi all,
> > > > this series aims at adding cpufreq support to the Raspberry Pi family of
> > > > boards.
> > > > 
> > > > The previous revision can be found at: 
> > > > https://lkml.org/lkml/2019/5/20/431
> > > > 
> > > > The series first factors out 'pllb' from clk-bcm2385 and creates a new
> > > > clk driver that operates it over RPi's firmware interface[1]. We are
> > > > forced to do so as the firmware 'owns' the pll and we're not allowed to
> > > > change through the register interface directly as we might race with the
> > > > over-temperature and under-voltage protections provided by the firmware.
> > > it would be nice to preserve such design decision in the driver as a
> > > comment, because the cover letter usually get lost.
> > > > Next it creates a minimal cpufreq driver that populates the CPU's opp
> > > > table, and registers cpufreq-dt. Which is needed as the firmware
> > > > controls the max and min frequencies available.
> > > I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
> > > manually enable this drivers. During boot with Raspbian rootfs i'm
> > > getting the following:
> > > 
> > > [    1.177009] cpu cpu0: failed to get clock: -2
> > > [    1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2
> > This is surprising, who could be creating a platform_device for cpufreq-dt
> > apart from raspberrypi-cpufreq? Just to make things clear, you're using the
> > device tree from v5.2-rc1 (as opposed to the Raspbian one)?
> 
> sorry my fault, i thought it already has been replaced. The behavior in
> this unexpected case is fine, since it doesn't crash.
> 
> I replaced the the DTB with the mainline one, but now i'm getting this:
> 
> [    4.566068] cpufreq: cpufreq_online: CPU0: Running at unlisted freq:
> 600000 KHz
> [    4.580690] cpu cpu0: dev_pm_opp_set_rate: Invalid target frequency 0
> [    4.594391] cpufreq: __target_index: Failed to change cpu frequency: -22

Ok, this looks more more like my fault, probably an overflow error somewhere. I
saw something similar while testing it on RPI2b. Which board & config was this
run with? Could you confirm the clk-raspberrypi.c message verifying the max and
min frequencies showed up and was correct.

Regards,
Nicolas
Stefan Wahren June 5, 2019, 1:12 p.m. UTC | #5
Am 05.06.19 um 14:27 schrieb Nicolas Saenz Julienne:
> Ok, this looks more more like my fault, probably an overflow error somewhere. I
> saw something similar while testing it on RPI2b. Which board & config was this
> run with?
It's an RPi 3B+ with multi_v7_defconfig
> Could you confirm the clk-raspberrypi.c message verifying the max and
> min frequencies showed up and was correct.
[    4.253294] raspberrypi-firmware soc:firmware: Attached to firmware
from 2019-03-27 15:45
[    4.269727] mmcblk0: mmc0:0007 SDCIT 14.6 GiB
[    4.282464] raspberrypi-clk raspberrypi-clk: CPU frequency range: min
600000000, max 1400000000
>
> Regards,
> Nicolas
>
Nicolas Saenz Julienne June 5, 2019, 7:11 p.m. UTC | #6
On Wed, 2019-06-05 at 13:34 +0200, Stefan Wahren wrote:
> Hi,
> 
> Am 05.06.19 um 13:00 schrieb Nicolas Saenz Julienne:
> > Hi Stefan,
> > thanks for the review, I took note of your code comments.
> > 
> > On Wed, 2019-06-05 at 11:46 +0200, Stefan Wahren wrote:
> > > Hi Nicolas,
> > > 
> > > Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
> > > > Hi all,
> > > > this series aims at adding cpufreq support to the Raspberry Pi family of
> > > > boards.
> > > > 
> > > > The previous revision can be found at: 
> > > > https://lkml.org/lkml/2019/5/20/431
> > > > 
> > > > The series first factors out 'pllb' from clk-bcm2385 and creates a new
> > > > clk driver that operates it over RPi's firmware interface[1]. We are
> > > > forced to do so as the firmware 'owns' the pll and we're not allowed to
> > > > change through the register interface directly as we might race with the
> > > > over-temperature and under-voltage protections provided by the firmware.
> > > it would be nice to preserve such design decision in the driver as a
> > > comment, because the cover letter usually get lost.
> > > > Next it creates a minimal cpufreq driver that populates the CPU's opp
> > > > table, and registers cpufreq-dt. Which is needed as the firmware
> > > > controls the max and min frequencies available.
> > > I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
> > > manually enable this drivers. During boot with Raspbian rootfs i'm
> > > getting the following:
> > > 
> > > [    1.177009] cpu cpu0: failed to get clock: -2
> > > [    1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2
> > This is surprising, who could be creating a platform_device for cpufreq-dt
> > apart from raspberrypi-cpufreq? Just to make things clear, you're using the
> > device tree from v5.2-rc1 (as opposed to the Raspbian one)?
> 
> sorry my fault, i thought it already has been replaced. The behavior in
> this unexpected case is fine, since it doesn't crash.
> 
> I replaced the the DTB with the mainline one, but now i'm getting this:
> 
> [    4.566068] cpufreq: cpufreq_online: CPU0: Running at unlisted freq:
> 600000 KHz
> [    4.580690] cpu cpu0: dev_pm_opp_set_rate: Invalid target frequency 0
> [    4.594391] cpufreq: __target_index: Failed to change cpu frequency: -22

For the record this fixes it:

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index aa51756fd4d6..edb71eefe9cf 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1293,7 +1293,7 @@ static int clk_core_determine_round_nolock(struct clk_core
*core,
        } else if (core->ops->round_rate) {
                rate = core->ops->round_rate(core->hw, req->rate,
                                             &req->best_parent_rate);
-               if (rate < 0)
+               if (IS_ERR_VALUE(rate))
                        return rate;

                req->rate = rate;

round_rate() returns a 'long' value, yet 'pllb' in rpi3b+ goes as high as
2.8GHz, which only fits in an 'unsigned long'. This explains why I didn't see
this issue with RPI2b.

I'll add the patch to the series.

Regards,
Nicolas
Stefan Wahren June 5, 2019, 8:15 p.m. UTC | #7
Hi Nicolas,

>> Am 05.06.19 um 13:00 schrieb Nicolas Saenz Julienne:
>>> Hi Stefan,
>>> thanks for the review, I took note of your code comments.
>>>
>>> On Wed, 2019-06-05 at 11:46 +0200, Stefan Wahren wrote:
>>>> Hi Nicolas,
>>>>
>>>> Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
>>>>> Hi all,
>>>>> this series aims at adding cpufreq support to the Raspberry Pi family of
>>>>> boards.
>>>>>
>>>>> The previous revision can be found at: 
>>>>> https://lkml.org/lkml/2019/5/20/431
>>>>>
>>>>> The series first factors out 'pllb' from clk-bcm2385 and creates a new
>>>>> clk driver that operates it over RPi's firmware interface[1]. We are
>>>>> forced to do so as the firmware 'owns' the pll and we're not allowed to
>>>>> change through the register interface directly as we might race with the
>>>>> over-temperature and under-voltage protections provided by the firmware.
>>>> it would be nice to preserve such design decision in the driver as a
>>>> comment, because the cover letter usually get lost.
>>>>> Next it creates a minimal cpufreq driver that populates the CPU's opp
>>>>> table, and registers cpufreq-dt. Which is needed as the firmware
>>>>> controls the max and min frequencies available.
>>>> I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
>>>> manually enable this drivers. During boot with Raspbian rootfs i'm
>>>> getting the following:
>>>>
>>>> [    1.177009] cpu cpu0: failed to get clock: -2
>>>> [    1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2
>>> This is surprising, who could be creating a platform_device for cpufreq-dt
>>> apart from raspberrypi-cpufreq? Just to make things clear, you're using the
>>> device tree from v5.2-rc1 (as opposed to the Raspbian one)?
>> sorry my fault, i thought it already has been replaced. The behavior in
>> this unexpected case is fine, since it doesn't crash.
>>
>> I replaced the the DTB with the mainline one, but now i'm getting this:
>>
>> [    4.566068] cpufreq: cpufreq_online: CPU0: Running at unlisted freq:
>> 600000 KHz
>> [    4.580690] cpu cpu0: dev_pm_opp_set_rate: Invalid target frequency 0
>> [    4.594391] cpufreq: __target_index: Failed to change cpu frequency: -22
> For the record this fixes it:
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index aa51756fd4d6..edb71eefe9cf 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1293,7 +1293,7 @@ static int clk_core_determine_round_nolock(struct clk_core
> *core,
>         } else if (core->ops->round_rate) {
>                 rate = core->ops->round_rate(core->hw, req->rate,
>                                              &req->best_parent_rate);
> -               if (rate < 0)
> +               if (IS_ERR_VALUE(rate))
>                         return rate;
>
>                 req->rate = rate;
>
> round_rate() returns a 'long' value, yet 'pllb' in rpi3b+ goes as high as
> 2.8GHz, which only fits in an 'unsigned long'. This explains why I didn't see
> this issue with RPI2b.

Okay, i understand the problem. But the patch doesn't look like the
proper fix to me.

Maybe the better way is to implement determine_rate instead of
round_rate in the clock driver. AFAICS this provides the necessary
unsigned long.

>
> I'll add the patch to the series.
>
> Regards,
> Nicolas