Odd behavior with dpll4_m4x2_ck on omap3 + DT

Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1913 bytes --]

Hi,

I'm seeing odd clock behavior with Beagle, booting with DT. I'm using
v3.11-rc7 + DSS DT patches.

It looks to me that when setting the rate of a clock, its child clock's
rate is not updated correctly. Here's pieces of boot log (full log
attached):

Here my code prints the current clocks. This looks ok: dpll4_m4_ck's
divider is 2 (432000000/2 = 216000000), and dpll4_m4x2_ck, which is a
fixed x2 multiplier, is two times dpll4_m4_ck.

[    1.592132] before:
[    1.592163] dpll4_ck: 432000000
[    1.592193] dpll4_m4_ck: 216000000
[    1.592224] dpll4_m4x2_ck: 432000000
[    1.592254] dss1_alwon_fck_3430es2: 432000000

Here DSS driver sets dpll4_m4_ck's rate

[    1.592254] setting rate 86400000

Here we see the clocks after the clk_set_rate. dpll4_m4_ck is as
supposed, but the dpll4_m4x2_ck is not x2, resulting in wrong clock for
dss1_alwon_fck_3430es2. The rate of dpll4_m4x2_ck does get updated,
though, but not for the correct x2 rate, but x1.

[    1.592315] after:
[    1.592315] dpll4_ck: 432000000
[    1.592346] dpll4_m4_ck: 86400000
[    1.592376] dpll4_m4x2_ck: 86400000
[    1.592407] dss1_alwon_fck_3430es2: 86400000

After that the DSS probe is deferred, as there's a missing regulator. A
bit later, the driver is probed again:

Here the clocks are still wrong, as nothing has changed them

[    3.196868] before:
[    3.199340] dpll4_ck: 432000000
[    3.202667] dpll4_m4_ck: 86400000
[    3.206207] dpll4_m4x2_ck: 86400000
[    3.209991] dss1_alwon_fck_3430es2: 86400000
[    3.214508] setting rate 72000000

But after this clk_set_rate, the dpll4_m4x2_ck is correct.

[    3.218048] after:
[    3.220275] dpll4_ck: 432000000
[    3.223602] dpll4_m4_ck: 72000000
[    3.227142] dpll4_m4x2_ck: 144000000
[    3.230987] dss1_alwon_fck_3430es2: 144000000

So, for some reason, the first clk_set_rate goes wrong. Any ideas?

 Tomi

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: minicom.log --]
[-- Type: text/x-log; name="minicom.log", Size: 15034 bytes --]

boot
reading uImage

4338021 bytes read
reading ramdisk.uboot

2990586 bytes read
## Booting kernel from Legacy Image at 81000000 ...
   Image Name:   Linux
   Image Type:   ARM Linux Kernel Image (uncompressed)
   Data Size:    4337957 Bytes = 4.1 MiB
   Load Address: 80008000
   Entry Point:  80008000
   Verifying Checksum ... OK
## Loading init Ramdisk from Legacy Image at 82000000 ...
   Image Name:   
   Image Type:   ARM Linux RAMDisk Image (gzip compressed)
   Data Size:    2990522 Bytes = 2.9 MiB
   Load Address: 00000000
   Entry Point:  00000000
   Verifying Checksum ... OK
   Loading Kernel Image ... OK
OK

Starting kernel ...

[    0.000000] Booting Linux on physical CPU 0x0
[    0.000000] Linux version 3.11.0-rc7-00023-gf6a7f7c-dirty (tomba@deskari) (gcc version 4.7.3 (Ubuntu/Linaro 4.7.3-1ubuntu1) ) #40 SMP Wed Aug 28 12:03:28 EEST 2013
[    0.000000] CPU: ARMv7 Processor [411fc083] revision 3 (ARMv7), cr=10c53c7d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT nonaliasing instruction cache
[    0.000000] Machine: Generic OMAP3-GP (Flattened Device Tree), model: TI OMAP3 BeagleBoard
[    0.000000] cma: CMA: reserved 32 MiB at 8d800000
[    0.000000] Memory policy: ECC disabled, Data cache writeback
[    0.000000] CPU: All CPU(s) started in SVC mode.
[    0.000000] OMAP3430/3530 ES3.0 (l2cache iva sgx neon isp )
[    0.000000] PERCPU: Embedded 9 pages/cpu @c103f000 s14272 r8192 d14400 u36864
[    0.000000] Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 64768
[    0.000000] Kernel command line: console=ttyO2,115200
[    0.000000] PID hash table entries: 1024 (order: 0, 4096 bytes)
[    0.000000] Dentry cache hash table entries: 32768 (order: 5, 131072 bytes)
[    0.000000] Inode-cache hash table entries: 16384 (order: 4, 65536 bytes)
[    0.000000] Memory: 208484K/261120K available (5679K kernel code, 672K rwdata, 2208K rodata, 401K init, 5526K bss, 52636K reserved, 0K highmem)
[    0.000000] Virtual kernel memory layout:
[    0.000000]     vector  : 0xffff0000 - 0xffff1000   (   4 kB)
[    0.000000]     fixmap  : 0xfff00000 - 0xfffe0000   ( 896 kB)
[    0.000000]     vmalloc : 0xd0800000 - 0xff000000   ( 744 MB)
[    0.000000]     lowmem  : 0xc0000000 - 0xd0000000   ( 256 MB)
[    0.000000]     pkmap   : 0xbfe00000 - 0xc0000000   (   2 MB)
[    0.000000]     modules : 0xbf000000 - 0xbfe00000   (  14 MB)
[    0.000000]       .text : 0xc0008000 - 0xc07bbf2c   (7888 kB)
[    0.000000]       .init : 0xc07bc000 - 0xc08207c0   ( 402 kB)
[    0.000000]       .data : 0xc0822000 - 0xc08ca300   ( 673 kB)
[    0.000000]        .bss : 0xc08ca300 - 0xc0e2fdd0   (5527 kB)
[    0.000000] Hierarchical RCU implementation.
[    0.000000] 	RCU restricting CPUs from NR_CPUS=2 to nr_cpu_ids=1.
[    0.000000] NR_IRQS:16 nr_irqs:16 16
[    0.000000] IRQ: Found an INTC at 0xfa200000 (revision 4.0) with 96 interrupts
[    0.000000] Total of 96 interrupts on 1 active controller
[    0.000000] Clocking rate (Crystal/Core/MPU): 26.0/332/600 MHz
[    0.000000] OMAP clockevent source: timer12 at 32768 Hz
[    0.000000] sched_clock: 32 bits at 32kHz, resolution 30517ns, wraps every 131071999ms
[    0.000000] OMAP clocksource: 32k_counter at 32768 Hz
[    0.000000] Console: colour dummy device 80x30
[    0.000000] Lock dependency validator: Copyright (c) 2006 Red Hat, Inc., Ingo Molnar
[    0.000000] ... MAX_LOCKDEP_SUBCLASSES:  8
[    0.000000] ... MAX_LOCK_DEPTH:          48
[    0.000000] ... MAX_LOCKDEP_KEYS:        8191
[    0.000000] ... CLASSHASH_SIZE:          4096
[    0.000000] ... MAX_LOCKDEP_ENTRIES:     16384
[    0.000000] ... MAX_LOCKDEP_CHAINS:      32768
[    0.000000] ... CHAINHASH_SIZE:          16384
[    0.000000]  memory used by lock dependency info: 3695 kB
[    0.000000]  per task-struct memory footprint: 1152 bytes
[    0.001251] Calibrating delay loop... 383.38 BogoMIPS (lpj=1916928)
[    0.048248] pid_max: default: 32768 minimum: 301
[    0.048858] Security Framework initialized
[    0.049102] Mount-cache hash table entries: 512
[    0.066223] CPU: Testing write buffer coherency: ok
[    0.067779] CPU0: thread -1, cpu 0, socket -1, mpidr 0
[    0.067901] Setting up static identity map for 0xc0591940 - 0xc05919b0
[    0.076507] Brought up 1 CPUs
[    0.076568] SMP: Total of 1 processors activated (383.38 BogoMIPS).
[    0.076568] CPU: All CPU(s) started in SVC mode.
[    0.080291] devtmpfs: initialized
[    0.161651] pinctrl core: initialized pinctrl subsystem
[    0.167449] regulator-dummy: no parameters
[    0.173004] NET: Registered protocol family 16
[    0.184082] DMA: preallocated 256 KiB pool for atomic coherent allocations
[    0.205902] Reprogramming SDRC clock to 332000000 Hz
[    0.217712] omap_gpio 48310000.gpio: could not find pctldev for node /ocp/pinmux@0x48002a00/pinmux_gpio1_pins, deferring probe
[    0.217803] platform 48310000.gpio: Driver omap_gpio requests probe deferral
[    0.223022] OMAP GPIO hardware version 2.5
[    0.259246] platform 49022000.mcbsp: alias fck already exists
[    0.260955] platform 49024000.mcbsp: alias fck already exists
[    0.283477] omap-gpmc 6e000000.gpmc: GPMC revision 5.0
[    0.302062] No ATAGs?
[    0.302093] hw-breakpoint: debug architecture 0x4 unsupported.
[    0.309997] OMAP DMA hardware revision 4.0
[    0.420410] bio: create slab <bio-0> at 0
[    0.427001] edma-dma-engine edma-dma-engine.0: Can't allocate PaRAM dummy slot
[    0.427093] edma-dma-engine: probe of edma-dma-engine.0 failed with error -5
[    0.546600] omap-dma-engine 48056000.dma-controller: OMAP DMA engine driver
[    0.550231] hsusb2_reset: 3300 mV 
[    0.550994] platform hsusb2_power_reg.21: Driver reg-fixed-voltage requests probe deferral
[    0.562499] SCSI subsystem initialized
[    0.566589] usbcore: registered new interface driver usbfs
[    0.567382] usbcore: registered new interface driver hub
[    0.568511] usbcore: registered new device driver usb
[    0.569335] platform hsusb2_phy.22: Driver nop_usb_xceiv requests probe deferral
[    0.579284] omap_i2c i2c.8: did not get pins for i2c error: -19
[    0.581451] omap_i2c i2c.8: bus 0 rev3.3 at 2600 kHz
[    0.584106] omap_i2c i2c.9: did not get pins for i2c error: -19
[    0.586486] omap_i2c i2c.9: bus 1 rev3.3 at 100 kHz
[    0.586853] omap_i2c i2c.10: did not get pins for i2c error: -19
[    0.588623] omap_i2c i2c.10: bus 2 rev3.3 at 100 kHz
[    0.598876] Switched to clocksource 32k_counter
[    0.810302] NET: Registered protocol family 2
[    0.812805] TCP established hash table entries: 2048 (order: 2, 16384 bytes)
[    0.813110] TCP bind hash table entries: 2048 (order: 4, 73728 bytes)
[    0.814361] TCP: Hash tables configured (established 2048 bind 2048)
[    0.814636] TCP: reno registered
[    0.814666] UDP hash table entries: 256 (order: 2, 20480 bytes)
[    0.815032] UDP-Lite hash table entries: 256 (order: 2, 20480 bytes)
[    0.816741] NET: Registered protocol family 1
[    0.819091] RPC: Registered named UNIX socket transport module.
[    0.819122] RPC: Registered udp transport module.
[    0.819152] RPC: Registered tcp transport module.
[    0.819152] RPC: Registered tcp NFSv4.1 backchannel transport module.
[    0.826629] Trying to unpack rootfs image as initramfs...
[    1.275085] Freeing initrd memory: 2916K (c2001000 - c22da000)
[    1.275177] NetWinder Floating Point Emulator V0.97 (double precision)
[    1.275665] hw perfevents: enabled with ARMv7 Cortex-A8 PMU driver, 5 counters available
[    1.523284] VFS: Disk quotas dquot_6.5.2
[    1.523681] Dquot-cache hash table entries: 1024 (order 0, 4096 bytes)
[    1.527465] NFS: Registering the id_resolver key type
[    1.527923] Key type id_resolver registered
[    1.527954] Key type id_legacy registered
[    1.528137] jffs2: version 2.2. (NAND) (SUMMARY)  © 2001-2006 Red Hat, Inc.
[    1.529083] msgmni has been set to 476
[    1.575012] io scheduler noop registered
[    1.575073] io scheduler deadline registered
[    1.575195] io scheduler cfq registered (default)
[    1.580718] pinctrl-single 48002030.pinmux: 742 pins at pa fa002030 size 1484
[    1.581665] pinctrl-single 48002a00.pinmux: 46 pins at pa fa002a00 size 92
[    1.589172] usbcore: registered new interface driver udlfb
[    1.592132] before:
[    1.592163] dpll4_ck: 432000000
[    1.592193] dpll4_m4_ck: 216000000
[    1.592224] dpll4_m4x2_ck: 432000000
[    1.592254] dss1_alwon_fck_3430es2: 432000000
[    1.592254] setting rate 86400000
[    1.592315] after:
[    1.592315] dpll4_ck: 432000000
[    1.592346] dpll4_m4_ck: 86400000
[    1.592376] dpll4_m4x2_ck: 86400000
[    1.592407] dss1_alwon_fck_3430es2: 86400000
[    1.592590] OMAP DSS rev 2.0
[    1.619293] omapdss DPI error: can't get VDDS_DSI regulator
[    1.619354] omapfb omapfb: failed to connect default display
[    1.619354] omapfb omapfb: failed to init overlay connections
[    1.621887] omapfb omapfb: failed to setup omapfb
[    1.621948] platform omapfb: Driver omapfb requests probe deferral
[    1.626739] uvesafb: failed to execute /sbin/v86d
[    1.626770] uvesafb: make sure that the v86d helper is installed and executable
[    1.626800] uvesafb: Getting VBE info block failed (eax=0x4f00, err=-2)
[    1.626800] uvesafb: vbe_init() failed with -22
[    1.626861] uvesafb: probe of uvesafb.0 failed with error -22
[    1.628295] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
[    1.638641] omap_uart serial.5: did not get pins for uart0 error: -19
[    1.640258] serial.5: ttyO0 at MMIO 0x4806a000 (irq = 88) is a OMAP UART0
[    1.643646] omap_uart serial.6: did not get pins for uart1 error: -19
[    1.644378] serial.6: ttyO1 at MMIO 0x4806c000 (irq = 89) is a OMAP UART1
[    1.647216] serial.7: ttyO2 at MMIO 0x49020000 (irq = 90) is a OMAP UART2
[    2.504791] console [ttyO2] enabled
[    2.562103] brd: module loaded
[    2.593719] loop: module loaded
[    2.613403] twl 0-0048: PIH (irq 23) chaining IRQs 306..314
[    2.620117] twl 0-0048: power (irq 311) chaining IRQs 314..321
[    2.634368] VDD1: 600 <--> 1450 mV at 1200 mV 
[    2.643798] VDAC: 1800 mV 
[    2.651306] VPLL2: 1800 mV 
[    2.657989] VMMC1: 1850 <--> 3150 mV at 3150 mV 
[    2.667022] VMMC2: 1850 <--> 3150 mV at 2600 mV 
[    2.675720] VUSB1V5: 1500 mV 
[    2.682373] VUSB1V8: 1800 mV 
[    2.689056] VUSB3V1: 3100 mV 
[    2.695648] VSIM: 1800 <--> 3000 mV at 1800 mV 
[    2.704772] twl4030_gpio gpio.38: gpio (irq 306) chaining IRQs 322..339
[    2.721588] twl4030_usb twl4030-usb.39: Initialized TWL4030 USB module
[    2.744140] mtdoops: mtd device (mtddev=name/number) must be supplied
[    2.774627] usbcore: registered new interface driver asix
[    2.781951] usbcore: registered new interface driver ax88179_178a
[    2.789581] usbcore: registered new interface driver cdc_ether
[    2.796478] usbcore: registered new interface driver r815x
[    2.803466] usbcore: registered new interface driver smsc95xx
[    2.810638] usbcore: registered new interface driver net1080
[    2.817321] usbcore: registered new interface driver cdc_subset
[    2.824523] usbcore: registered new interface driver zaurus
[    2.831481] usbcore: registered new interface driver cdc_ncm
[    2.840087] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
[    2.847015] ehci-omap: OMAP-EHCI Host Controller driver
[    2.853576] ehci-omap 48064800.ehci: Can't get PHY device for port 1: -517
[    2.861053] platform 48064800.ehci: Driver ehci-omap requests probe deferral
[    2.870391] usbcore: registered new interface driver cdc_wdm
[    2.877166] usbcore: registered new interface driver usbtest
[    2.886657] HS USB OTG: no transceiver configured
[    2.891906] musb-hdrc musb-hdrc.0.auto: musb_init_controller failed with status -517
[    2.900329] platform musb-hdrc.0.auto: Driver musb-hdrc requests probe deferral
[    2.913513] mousedev: PS/2 mouse device common for all mice
[    2.928741] twl_rtc rtc.27: Power up reset detected.
[    2.934631] twl_rtc rtc.27: Enabling TWL-RTC
[    2.945434] twl_rtc rtc.27: rtc core: registered rtc.27 as rtc0
[    2.954559] i2c /dev entries driver
[    2.964172] Driver for 1-wire Dallas network protocol.
[    2.978027] omap_wdt: OMAP Watchdog Timer Rev 0x31: initial timeout 60 sec
[    3.000061] ledtrig-cpu: registered to indicate activity on CPUs
[    3.009094] usbcore: registered new interface driver usbhid
[    3.015014] usbhid: USB HID core driver
[    3.021972] oprofile: using arm/armv7
[    3.026855] TCP: cubic registered
[    3.031158] Initializing XFRM netlink socket
[    3.035888] NET: Registered protocol family 17
[    3.040802] NET: Registered protocol family 15
[    3.046020] Key type dns_resolver registered
[    3.050720] VFP support v0.3: implementor 41 architecture 3 part 30 variant c rev 1
[    3.061676] PM: no software I/O chain control; some wakeups may be lost
[    3.069519] ThumbEE CPU extension supported.
[    3.082122] VMMC1: disabling
[    3.085998] VPLL2: disabling
[    3.090118] VDAC: disabling
[    3.105041] hsusb2_vbus: 3300 mV 
[    3.196868] before:
[    3.199340] dpll4_ck: 432000000
[    3.202667] dpll4_m4_ck: 86400000
[    3.206207] dpll4_m4x2_ck: 86400000
[    3.209991] dss1_alwon_fck_3430es2: 86400000
[    3.214508] setting rate 72000000
[    3.218048] after:
[    3.220275] dpll4_ck: 432000000
[    3.223602] dpll4_m4_ck: 72000000
[    3.227142] dpll4_m4x2_ck: 144000000
[    3.230987] dss1_alwon_fck_3430es2: 144000000
[    3.235626] omapdss DPI: Could not find exact pixel clock. Requested 154012 kHz, got 144000 kHz
[    3.385040] ehci-omap 48064800.ehci: EHCI Host Controller
[    3.394989] ehci-omap 48064800.ehci: new USB bus registered, assigned bus number 1
[    3.406158] ehci-omap 48064800.ehci: irq 93, io mem 0x48064800
[    3.429199] ehci-omap 48064800.ehci: USB 2.0 started, EHCI 1.00
[    3.436431] usb usb1: New USB device found, idVendor=1d6b, idProduct=0002
[    3.443786] usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1
[    3.451568] usb usb1: Product: EHCI Host Controller
[    3.456756] usb usb1: Manufacturer: Linux 3.11.0-rc7-00023-gf6a7f7c-dirty ehci_hcd
[    3.464843] usb usb1: SerialNumber: 48064800.ehci
[    3.477020] hub 1-0:1.0: USB hub found
[    3.481445] hub 1-0:1.0: 3 ports detected
[    3.489349] HS USB OTG: no transceiver configured
[    3.494384] musb-hdrc musb-hdrc.0.auto: musb_init_controller failed with status -517
[    3.502777] platform musb-hdrc.0.auto: Driver musb-hdrc requests probe deferral
[    3.518096] input: gpio_keys.23 as /devices/gpio_keys.23/input/input0
[    3.529876] HS USB OTG: no transceiver configured
[    3.534912] musb-hdrc musb-hdrc.0.auto: musb_init_controller failed with status -517
[    3.543457] platform musb-hdrc.0.auto: Driver musb-hdrc requests probe deferral
[    3.554138] twl_rtc rtc.27: setting system clock to 2000-01-01 00:00:00 UTC (946684800)
[    3.636413] Freeing unused kernel memory: 400K (c07bc000 - c0820000)
Starting logging: OK
Initializing random number generator... done.
Starting network...

Welcome to Buildroot
buildroot login: 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tero Kristo]

On 08/28/2013 12:22 PM, Tomi Valkeinen wrote:
> Hi,
>
> I'm seeing odd clock behavior with Beagle, booting with DT. I'm using
> v3.11-rc7 + DSS DT patches.

I guess you are not using the clock DT patches? Just making sure I 
didn't break anything. :)

> It looks to me that when setting the rate of a clock, its child clock's
> rate is not updated correctly. Here's pieces of boot log (full log
> attached):
>
> Here my code prints the current clocks. This looks ok: dpll4_m4_ck's
> divider is 2 (432000000/2 = 216000000), and dpll4_m4x2_ck, which is a
> fixed x2 multiplier, is two times dpll4_m4_ck.
>
> [    1.592132] before:
> [    1.592163] dpll4_ck: 432000000
> [    1.592193] dpll4_m4_ck: 216000000
> [    1.592224] dpll4_m4x2_ck: 432000000
> [    1.592254] dss1_alwon_fck_3430es2: 432000000
>
> Here DSS driver sets dpll4_m4_ck's rate
>
> [    1.592254] setting rate 86400000
>
> Here we see the clocks after the clk_set_rate. dpll4_m4_ck is as
> supposed, but the dpll4_m4x2_ck is not x2, resulting in wrong clock for
> dss1_alwon_fck_3430es2. The rate of dpll4_m4x2_ck does get updated,
> though, but not for the correct x2 rate, but x1.
>
> [    1.592315] after:
> [    1.592315] dpll4_ck: 432000000
> [    1.592346] dpll4_m4_ck: 86400000
> [    1.592376] dpll4_m4x2_ck: 86400000
> [    1.592407] dss1_alwon_fck_3430es2: 86400000
>
> After that the DSS probe is deferred, as there's a missing regulator. A
> bit later, the driver is probed again:
>
> Here the clocks are still wrong, as nothing has changed them
>
> [    3.196868] before:
> [    3.199340] dpll4_ck: 432000000
> [    3.202667] dpll4_m4_ck: 86400000
> [    3.206207] dpll4_m4x2_ck: 86400000
> [    3.209991] dss1_alwon_fck_3430es2: 86400000
> [    3.214508] setting rate 72000000
>
> But after this clk_set_rate, the dpll4_m4x2_ck is correct.
>
> [    3.218048] after:
> [    3.220275] dpll4_ck: 432000000
> [    3.223602] dpll4_m4_ck: 72000000
> [    3.227142] dpll4_m4x2_ck: 144000000
> [    3.230987] dss1_alwon_fck_3430es2: 144000000
>
> So, for some reason, the first clk_set_rate goes wrong. Any ideas?

Hmm, strange. I am not seeing similar behavior, but I am calling 
clk_set_rate in different location.... also I am using clock DT patches 
(don't try the current version though, as I am reworking them.)

[    0.000000] dpll4_ck: 432000000
[    0.000000] dpll4_m4_ck: 72000000
[    0.000000] dpll4_m4x2_ck: 144000000
[    0.000000] dss1_alwon_fck_3430es2: 144000000
[    0.000000] dpll4_ck: 432000000
[    0.000000] dpll4_m4_ck: 86400000
[    0.000000] dpll4_m4x2_ck: 172800000
[    0.000000] dss1_alwon_fck_3430es2: 172800000

Do you see the error only when setting to some specific rate (86400000) 
or it doesn't matter?

-Tero

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

On 28/08/13 12:48, Tero Kristo wrote:
> On 08/28/2013 12:22 PM, Tomi Valkeinen wrote:
>> Hi,
>>
>> I'm seeing odd clock behavior with Beagle, booting with DT. I'm using
>> v3.11-rc7 + DSS DT patches.
> 
> I guess you are not using the clock DT patches? Just making sure I
> didn't break anything. :)

No, plain rc7 with my DSS DT patches.

>> So, for some reason, the first clk_set_rate goes wrong. Any ideas?
> 
> Hmm, strange. I am not seeing similar behavior, but I am calling
> clk_set_rate in different location.... also I am using clock DT patches
> (don't try the current version though, as I am reworking them.)
> 
> [    0.000000] dpll4_ck: 432000000
> [    0.000000] dpll4_m4_ck: 72000000
> [    0.000000] dpll4_m4x2_ck: 144000000
> [    0.000000] dss1_alwon_fck_3430es2: 144000000
> [    0.000000] dpll4_ck: 432000000
> [    0.000000] dpll4_m4_ck: 86400000
> [    0.000000] dpll4_m4x2_ck: 172800000
> [    0.000000] dss1_alwon_fck_3430es2: 172800000
> 
> Do you see the error only when setting to some specific rate (86400000)
> or it doesn't matter?

I also tried setting to 72000000, with the same result.

Do you know if I can somehow easily get debug prints from the clock
framework, that could lighten up the issue?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tero Kristo]

On 08/28/2013 01:14 PM, Tomi Valkeinen wrote:
> On 28/08/13 12:48, Tero Kristo wrote:
>> On 08/28/2013 12:22 PM, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> I'm seeing odd clock behavior with Beagle, booting with DT. I'm using
>>> v3.11-rc7 + DSS DT patches.
>>
>> I guess you are not using the clock DT patches? Just making sure I
>> didn't break anything. :)
>
> No, plain rc7 with my DSS DT patches.
>
>>> So, for some reason, the first clk_set_rate goes wrong. Any ideas?
>>
>> Hmm, strange. I am not seeing similar behavior, but I am calling
>> clk_set_rate in different location.... also I am using clock DT patches
>> (don't try the current version though, as I am reworking them.)
>>
>> [    0.000000] dpll4_ck: 432000000
>> [    0.000000] dpll4_m4_ck: 72000000
>> [    0.000000] dpll4_m4x2_ck: 144000000
>> [    0.000000] dss1_alwon_fck_3430es2: 144000000
>> [    0.000000] dpll4_ck: 432000000
>> [    0.000000] dpll4_m4_ck: 86400000
>> [    0.000000] dpll4_m4x2_ck: 172800000
>> [    0.000000] dss1_alwon_fck_3430es2: 172800000
>>
>> Do you see the error only when setting to some specific rate (86400000)
>> or it doesn't matter?
>
> I also tried setting to 72000000, with the same result.
>
> Do you know if I can somehow easily get debug prints from the clock
> framework, that could lighten up the issue?

There isn't any good config option for that, I would suggest add prints 
to the clk_set_rate and then for the clocks you are interested in, print 
results for the recalc_rate / set_rate ops also.

-Tero

>
>   Tomi
>
>

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Stefan Roese]

On 28.08.2013 13:40, Tero Kristo wrote:
> On 08/28/2013 01:14 PM, Tomi Valkeinen wrote:
>> On 28/08/13 12:48, Tero Kristo wrote:
>>> On 08/28/2013 12:22 PM, Tomi Valkeinen wrote:
>>>> Hi,
>>>>
>>>> I'm seeing odd clock behavior with Beagle, booting with DT. I'm using
>>>> v3.11-rc7 + DSS DT patches.
>>>
>>> I guess you are not using the clock DT patches? Just making sure I
>>> didn't break anything. :)
>>
>> No, plain rc7 with my DSS DT patches.
>>
>>>> So, for some reason, the first clk_set_rate goes wrong. Any ideas?
>>>
>>> Hmm, strange. I am not seeing similar behavior, but I am calling
>>> clk_set_rate in different location.... also I am using clock DT patches
>>> (don't try the current version though, as I am reworking them.)
>>>
>>> [    0.000000] dpll4_ck: 432000000
>>> [    0.000000] dpll4_m4_ck: 72000000
>>> [    0.000000] dpll4_m4x2_ck: 144000000
>>> [    0.000000] dss1_alwon_fck_3430es2: 144000000
>>> [    0.000000] dpll4_ck: 432000000
>>> [    0.000000] dpll4_m4_ck: 86400000
>>> [    0.000000] dpll4_m4x2_ck: 172800000
>>> [    0.000000] dss1_alwon_fck_3430es2: 172800000
>>>
>>> Do you see the error only when setting to some specific rate (86400000)
>>> or it doesn't matter?
>>
>> I also tried setting to 72000000, with the same result.
>>
>> Do you know if I can somehow easily get debug prints from the clock
>> framework, that could lighten up the issue?
> 
> There isn't any good config option for that, I would suggest add prints 
> to the clk_set_rate and then for the clocks you are interested in, print 
> results for the recalc_rate / set_rate ops also.

I debugged this a bit and found that this issue (dpll4_m4x2_ck clock is
not 2 times dpll4_m4_ck) results from this code:

arch/arm/mach-omap2/dpll3xxx.c:

unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw,
				    unsigned long parent_rate)
{
	...
	if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
		rate = parent_rate;
	else
		rate = parent_rate * 2;
	return rate;
}

As marked above, v is at that early time 0x1 (unmasked value of this
register is 0x38310037). So the DPLL4 is not locked but in low power top
mode (OMAP3XXX_EN_DPLL_LOCKED = 0x7).

Any hint whats missing here?

Thanks,
Stefan

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tero Kristo]

On 09/10/2013 02:33 PM, Stefan Roese wrote:
> On 28.08.2013 13:40, Tero Kristo wrote:
>> On 08/28/2013 01:14 PM, Tomi Valkeinen wrote:
>>> On 28/08/13 12:48, Tero Kristo wrote:
>>>> On 08/28/2013 12:22 PM, Tomi Valkeinen wrote:
>>>>> Hi,
>>>>>
>>>>> I'm seeing odd clock behavior with Beagle, booting with DT. I'm using
>>>>> v3.11-rc7 + DSS DT patches.
>>>>
>>>> I guess you are not using the clock DT patches? Just making sure I
>>>> didn't break anything. :)
>>>
>>> No, plain rc7 with my DSS DT patches.
>>>
>>>>> So, for some reason, the first clk_set_rate goes wrong. Any ideas?
>>>>
>>>> Hmm, strange. I am not seeing similar behavior, but I am calling
>>>> clk_set_rate in different location.... also I am using clock DT patches
>>>> (don't try the current version though, as I am reworking them.)
>>>>
>>>> [    0.000000] dpll4_ck: 432000000
>>>> [    0.000000] dpll4_m4_ck: 72000000
>>>> [    0.000000] dpll4_m4x2_ck: 144000000
>>>> [    0.000000] dss1_alwon_fck_3430es2: 144000000
>>>> [    0.000000] dpll4_ck: 432000000
>>>> [    0.000000] dpll4_m4_ck: 86400000
>>>> [    0.000000] dpll4_m4x2_ck: 172800000
>>>> [    0.000000] dss1_alwon_fck_3430es2: 172800000
>>>>
>>>> Do you see the error only when setting to some specific rate (86400000)
>>>> or it doesn't matter?
>>>
>>> I also tried setting to 72000000, with the same result.
>>>
>>> Do you know if I can somehow easily get debug prints from the clock
>>> framework, that could lighten up the issue?
>>
>> There isn't any good config option for that, I would suggest add prints
>> to the clk_set_rate and then for the clocks you are interested in, print
>> results for the recalc_rate / set_rate ops also.
>
> I debugged this a bit and found that this issue (dpll4_m4x2_ck clock is
> not 2 times dpll4_m4_ck) results from this code:
>
> arch/arm/mach-omap2/dpll3xxx.c:
>
> unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw,
> 				    unsigned long parent_rate)
> {
> 	...
> 	if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 		rate = parent_rate;
> 	else
> 		rate = parent_rate * 2;
> 	return rate;
> }
>
> As marked above, v is at that early time 0x1 (unmasked value of this
> register is 0x38310037). So the DPLL4 is not locked but in low power top
> mode (OMAP3XXX_EN_DPLL_LOCKED = 0x7).
>
> Any hint whats missing here?

If it claims it is not locked, it means the DPLL itself is disabled. You 
could try clk_enable for the clock before doing clk_set_rate.

-Tero

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 331 bytes --]

On 10/09/13 15:12, Tero Kristo wrote:

> If it claims it is not locked, it means the DPLL itself is disabled. You
> could try clk_enable for the clock before doing clk_set_rate.

Hmm, so is it required to enable the clock before setting the rate? If
so, I think I'm using the clocks wrong in all the places =).

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tero Kristo]

On 09/10/2013 03:19 PM, Tomi Valkeinen wrote:
> On 10/09/13 15:12, Tero Kristo wrote:
>
>> If it claims it is not locked, it means the DPLL itself is disabled. You
>> could try clk_enable for the clock before doing clk_set_rate.
>
> Hmm, so is it required to enable the clock before setting the rate? If
> so, I think I'm using the clocks wrong in all the places =).

In generic case, it is not. But DPLLs behave strangely if they go to low 
power stop mode. If there is any downstream clock enabled for a specific 
DPLL it is enabled and things work okay.

One could also argue that the API behavior in OMAP is wrong currently, 
as the bypass rate is something you are most likely never actually going 
to use for anything....

Just try the change and check the results.

-Tero

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Stefan Roese]

On 10.09.2013 14:12, Tero Kristo wrote:
>> I debugged this a bit and found that this issue (dpll4_m4x2_ck clock is
>> not 2 times dpll4_m4_ck) results from this code:
>>
>> arch/arm/mach-omap2/dpll3xxx.c:
>>
>> unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw,
>> 				    unsigned long parent_rate)
>> {
>> 	...
>> 	if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
>>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> 		rate = parent_rate;
>> 	else
>> 		rate = parent_rate * 2;
>> 	return rate;
>> }
>>
>> As marked above, v is at that early time 0x1 (unmasked value of this
>> register is 0x38310037). So the DPLL4 is not locked but in low power top
>> mode (OMAP3XXX_EN_DPLL_LOCKED = 0x7).
>>
>> Any hint whats missing here?
> 
> If it claims it is not locked, it means the DPLL itself is disabled. You 
> could try clk_enable for the clock before doing clk_set_rate.

Yes, of course. This solves this issue. Thanks.

Tomi, most likely some clk_enable() / clk_disable() calls are missing
from the PM functions as well?

Cheers,
Stefan

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

On 10/09/13 15:24, Tero Kristo wrote:
> On 09/10/2013 03:19 PM, Tomi Valkeinen wrote:
>> On 10/09/13 15:12, Tero Kristo wrote:
>>
>>> If it claims it is not locked, it means the DPLL itself is disabled. You
>>> could try clk_enable for the clock before doing clk_set_rate.
>>
>> Hmm, so is it required to enable the clock before setting the rate? If
>> so, I think I'm using the clocks wrong in all the places =).
> 
> In generic case, it is not. But DPLLs behave strangely if they go to low
> power stop mode. If there is any downstream clock enabled for a specific
> DPLL it is enabled and things work okay.
> 
> One could also argue that the API behavior in OMAP is wrong currently,
> as the bypass rate is something you are most likely never actually going
> to use for anything....
> 
> Just try the change and check the results.

Ok, so as Stefan said, enabling the clock fixes the issue.

How do you suggest we fix this? Changing omapdss to enable the clock
before changing its rate is not very difficult, so it can be used as a
quick fix. But it doesn't sound like a proper fix if this is not
normally required.

And, maybe I'm missing something as I don't have good understanding of
the PRCM's PLLs, but the current behavior sounds odd. So the DPLL is
off, and in bypass mode. When we try to change the rate of the clock
provided by the PLL, shouldn't it fail, as bypass mode's rate cannot be
changed? Or better, change the non-bypass rate.

How is the DPLL4's clock rate 432000000 anyway in bypass mode. Isn't
bypass mode usually plain sys-clock, or such?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tero Kristo]

On 09/10/2013 03:40 PM, Tomi Valkeinen wrote:
> On 10/09/13 15:24, Tero Kristo wrote:
>> On 09/10/2013 03:19 PM, Tomi Valkeinen wrote:
>>> On 10/09/13 15:12, Tero Kristo wrote:
>>>
>>>> If it claims it is not locked, it means the DPLL itself is disabled. You
>>>> could try clk_enable for the clock before doing clk_set_rate.
>>>
>>> Hmm, so is it required to enable the clock before setting the rate? If
>>> so, I think I'm using the clocks wrong in all the places =).
>>
>> In generic case, it is not. But DPLLs behave strangely if they go to low
>> power stop mode. If there is any downstream clock enabled for a specific
>> DPLL it is enabled and things work okay.
>>
>> One could also argue that the API behavior in OMAP is wrong currently,
>> as the bypass rate is something you are most likely never actually going
>> to use for anything....
>>
>> Just try the change and check the results.
>
> Ok, so as Stefan said, enabling the clock fixes the issue.
>
> How do you suggest we fix this? Changing omapdss to enable the clock
> before changing its rate is not very difficult, so it can be used as a
> quick fix. But it doesn't sound like a proper fix if this is not
> normally required.

The quick fix sounds good to me.

> And, maybe I'm missing something as I don't have good understanding of
> the PRCM's PLLs, but the current behavior sounds odd. So the DPLL is
> off, and in bypass mode. When we try to change the rate of the clock
> provided by the PLL, shouldn't it fail, as bypass mode's rate cannot be
> changed? Or better, change the non-bypass rate.

In theory, DPLLs can also be used in their bypass mode to feed customer 
nodes clocks. I just think the check in the clkoutx2_recalc is wrong, 
and should be enhanced to actually check what is the target mode for the 
clock once it is enabled. Maybe something like this would work properly:

diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
index 3a0296c..ba218fb 100644
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw,

         dd = pclk->dpll_data;

-       WARN_ON(!dd->enable_mask);
-
-       v = __raw_readl(dd->control_reg) & dd->enable_mask;
-       v >>= __ffs(dd->enable_mask);
-       if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
+       if ((dd->flags & DPLL_J_TYPE) ||
+           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk))
                 rate = parent_rate;
         else
                 rate = parent_rate * 2;
+
         return rate;
  }


>
> How is the DPLL4's clock rate 432000000 anyway in bypass mode. Isn't
> bypass mode usually plain sys-clock, or such?

This again reflects the rate that the clock has once it is enabled, the 
clkoutx2 does not.

Getting comment from someone like Paul would probably help here.

-Tero

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Mike Turquette]

Quoting Tero Kristo (2013-09-10 06:17:45)
> On 09/10/2013 03:40 PM, Tomi Valkeinen wrote:
> > On 10/09/13 15:24, Tero Kristo wrote:
> >> On 09/10/2013 03:19 PM, Tomi Valkeinen wrote:
> >>> On 10/09/13 15:12, Tero Kristo wrote:
> >>>
> >>>> If it claims it is not locked, it means the DPLL itself is disabled. You
> >>>> could try clk_enable for the clock before doing clk_set_rate.
> >>>
> >>> Hmm, so is it required to enable the clock before setting the rate? If
> >>> so, I think I'm using the clocks wrong in all the places =).
> >>
> >> In generic case, it is not. But DPLLs behave strangely if they go to low
> >> power stop mode. If there is any downstream clock enabled for a specific
> >> DPLL it is enabled and things work okay.
> >>
> >> One could also argue that the API behavior in OMAP is wrong currently,
> >> as the bypass rate is something you are most likely never actually going
> >> to use for anything....
> >>
> >> Just try the change and check the results.
> >
> > Ok, so as Stefan said, enabling the clock fixes the issue.
> >
> > How do you suggest we fix this? Changing omapdss to enable the clock
> > before changing its rate is not very difficult, so it can be used as a
> > quick fix. But it doesn't sound like a proper fix if this is not
> > normally required.
> 
> The quick fix sounds good to me.
> 
> > And, maybe I'm missing something as I don't have good understanding of
> > the PRCM's PLLs, but the current behavior sounds odd. So the DPLL is
> > off, and in bypass mode. When we try to change the rate of the clock
> > provided by the PLL, shouldn't it fail, as bypass mode's rate cannot be
> > changed? Or better, change the non-bypass rate.
> 
> In theory, DPLLs can also be used in their bypass mode to feed customer 
> nodes clocks. I just think the check in the clkoutx2_recalc is wrong, 
> and should be enhanced to actually check what is the target mode for the 
> clock once it is enabled. Maybe something like this would work properly:
> 
> diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
> index 3a0296c..ba218fb 100644
> --- a/arch/arm/mach-omap2/dpll3xxx.c
> +++ b/arch/arm/mach-omap2/dpll3xxx.c
> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw,
> 
>          dd = pclk->dpll_data;
> 
> -       WARN_ON(!dd->enable_mask);
> -
> -       v = __raw_readl(dd->control_reg) & dd->enable_mask;
> -       v >>= __ffs(dd->enable_mask);
> -       if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
> +       if ((dd->flags & DPLL_J_TYPE) ||
> +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk))

Two quick asides here:

1) might be nice if the J_TYPE pll was it's own clock type

2) would be nice if the check for bypass used something like:

if (clk_get_parent(hw->clk) == dd->bypass_clk)
	rate = parent_rate;

Which implies that the DPLLs become proper mux clocks.

Regards,
Mike

>                  rate = parent_rate;
>          else
>                  rate = parent_rate * 2;
> +
>          return rate;
>   }
> 
> 
> >
> > How is the DPLL4's clock rate 432000000 anyway in bypass mode. Isn't
> > bypass mode usually plain sys-clock, or such?
> 
> This again reflects the rate that the clock has once it is enabled, the 
> clkoutx2 does not.
> 
> Getting comment from someone like Paul would probably help here.
> 
> -Tero

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Mike Turquette]

On Tue, Sep 10, 2013 at 2:17 PM, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Tero Kristo (2013-09-10 06:17:45)
>> On 09/10/2013 03:40 PM, Tomi Valkeinen wrote:
>> > On 10/09/13 15:24, Tero Kristo wrote:
>> >> On 09/10/2013 03:19 PM, Tomi Valkeinen wrote:
>> >>> On 10/09/13 15:12, Tero Kristo wrote:
>> >>>
>> >>>> If it claims it is not locked, it means the DPLL itself is disabled. You
>> >>>> could try clk_enable for the clock before doing clk_set_rate.
>> >>>
>> >>> Hmm, so is it required to enable the clock before setting the rate? If
>> >>> so, I think I'm using the clocks wrong in all the places =).
>> >>
>> >> In generic case, it is not. But DPLLs behave strangely if they go to low
>> >> power stop mode. If there is any downstream clock enabled for a specific
>> >> DPLL it is enabled and things work okay.
>> >>
>> >> One could also argue that the API behavior in OMAP is wrong currently,
>> >> as the bypass rate is something you are most likely never actually going
>> >> to use for anything....
>> >>
>> >> Just try the change and check the results.
>> >
>> > Ok, so as Stefan said, enabling the clock fixes the issue.
>> >
>> > How do you suggest we fix this? Changing omapdss to enable the clock
>> > before changing its rate is not very difficult, so it can be used as a
>> > quick fix. But it doesn't sound like a proper fix if this is not
>> > normally required.
>>
>> The quick fix sounds good to me.
>>
>> > And, maybe I'm missing something as I don't have good understanding of
>> > the PRCM's PLLs, but the current behavior sounds odd. So the DPLL is
>> > off, and in bypass mode. When we try to change the rate of the clock
>> > provided by the PLL, shouldn't it fail, as bypass mode's rate cannot be
>> > changed? Or better, change the non-bypass rate.
>>
>> In theory, DPLLs can also be used in their bypass mode to feed customer
>> nodes clocks. I just think the check in the clkoutx2_recalc is wrong,
>> and should be enhanced to actually check what is the target mode for the
>> clock once it is enabled. Maybe something like this would work properly:
>>
>> diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
>> index 3a0296c..ba218fb 100644
>> --- a/arch/arm/mach-omap2/dpll3xxx.c
>> +++ b/arch/arm/mach-omap2/dpll3xxx.c
>> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw,
>>
>>          dd = pclk->dpll_data;
>>
>> -       WARN_ON(!dd->enable_mask);
>> -
>> -       v = __raw_readl(dd->control_reg) & dd->enable_mask;
>> -       v >>= __ffs(dd->enable_mask);
>> -       if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
>> +       if ((dd->flags & DPLL_J_TYPE) ||
>> +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk))
>
> Two quick asides here:
>
> 1) might be nice if the J_TYPE pll was it's own clock type
>
> 2) would be nice if the check for bypass used something like:
>
> if (clk_get_parent(hw->clk) == dd->bypass_clk)
>         rate = parent_rate;
>
> Which implies that the DPLLs become proper mux clocks.

Please disregard the above! I'm reviewing the OMAP v6 CCF series and
does these things already. Very cool.

Regards,
Mike

>
> Regards,
> Mike
>
>>                  rate = parent_rate;
>>          else
>>                  rate = parent_rate * 2;
>> +
>>          return rate;
>>   }
>>
>>
>> >
>> > How is the DPLL4's clock rate 432000000 anyway in bypass mode. Isn't
>> > bypass mode usually plain sys-clock, or such?
>>
>> This again reflects the rate that the clock has once it is enabled, the
>> clkoutx2 does not.
>>
>> Getting comment from someone like Paul would probably help here.
>>
>> -Tero

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

On 10/09/13 16:17, Tero Kristo wrote:

> In theory, DPLLs can also be used in their bypass mode to feed customer
> nodes clocks. I just think the check in the clkoutx2_recalc is wrong,
> and should be enhanced to actually check what is the target mode for the
> clock once it is enabled. Maybe something like this would work properly:
> 
> diff --git a/arch/arm/mach-omap2/dpll3xxx.c
> b/arch/arm/mach-omap2/dpll3xxx.c
> index 3a0296c..ba218fb 100644
> --- a/arch/arm/mach-omap2/dpll3xxx.c
> +++ b/arch/arm/mach-omap2/dpll3xxx.c
> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw
> *hw,
> 
>         dd = pclk->dpll_data;
> 
> -       WARN_ON(!dd->enable_mask);
> -
> -       v = __raw_readl(dd->control_reg) & dd->enable_mask;
> -       v >>= __ffs(dd->enable_mask);
> -       if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
> +       if ((dd->flags & DPLL_J_TYPE) ||
> +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk))
>                 rate = parent_rate;
>         else
>                 rate = parent_rate * 2;
> +
>         return rate;
>  }

Stefan, are you able to test the above?

I'd rather have a proper fix for this, than hack omapdss =).

>> How is the DPLL4's clock rate 432000000 anyway in bypass mode. Isn't
>> bypass mode usually plain sys-clock, or such?
> 
> This again reflects the rate that the clock has once it is enabled, the
> clkoutx2 does not.

Ok. Then it does sound like the clkoutx2 is calculated wrong, as you say.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Stefan Roese]

On 11.09.2013 09:21, Tomi Valkeinen wrote:
> On 10/09/13 16:17, Tero Kristo wrote:
> 
>> In theory, DPLLs can also be used in their bypass mode to feed customer
>> nodes clocks. I just think the check in the clkoutx2_recalc is wrong,
>> and should be enhanced to actually check what is the target mode for the
>> clock once it is enabled. Maybe something like this would work properly:
>>
>> diff --git a/arch/arm/mach-omap2/dpll3xxx.c
>> b/arch/arm/mach-omap2/dpll3xxx.c
>> index 3a0296c..ba218fb 100644
>> --- a/arch/arm/mach-omap2/dpll3xxx.c
>> +++ b/arch/arm/mach-omap2/dpll3xxx.c
>> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw
>> *hw,
>>
>>         dd = pclk->dpll_data;
>>
>> -       WARN_ON(!dd->enable_mask);
>> -
>> -       v = __raw_readl(dd->control_reg) & dd->enable_mask;
>> -       v >>= __ffs(dd->enable_mask);
>> -       if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
>> +       if ((dd->flags & DPLL_J_TYPE) ||
>> +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk))
>>                 rate = parent_rate;
>>         else
>>                 rate = parent_rate * 2;
>> +
>>         return rate;
>>  }
> 
> Stefan, are you able to test the above?
> 
> I'd rather have a proper fix for this, than hack omapdss =).

Okay, I finally found some time to test this. The patch above generates
this warning:

arch/arm/mach-omap2/dpll3xxx.c: In function 'omap3_clkoutx2_recalc':
arch/arm/mach-omap2/dpll3xxx.c:663:6: warning: passing argument 1 of '__clk_get_rate' from incompatible pointer type [enabled by default]
include/linux/clk-provider.h:423:15: note: expected 'struct clk *' but argument is of type 'struct clk_hw_omap *'

I then changed it (not 100% sure if correctly) to this version:

+       if ((dd->flags & DPLL_J_TYPE) ||
+           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk->hw.clk))

And this seems to work. At least the clock rate mismatch warning does not
appear with this patch applied (and without the clk_enable) in the
bootlog any more.

Thanks,
Stefan

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tero Kristo]

On 09/13/2013 10:51 AM, Stefan Roese wrote:
> On 11.09.2013 09:21, Tomi Valkeinen wrote:
>> On 10/09/13 16:17, Tero Kristo wrote:
>>
>>> In theory, DPLLs can also be used in their bypass mode to feed customer
>>> nodes clocks. I just think the check in the clkoutx2_recalc is wrong,
>>> and should be enhanced to actually check what is the target mode for the
>>> clock once it is enabled. Maybe something like this would work properly:
>>>
>>> diff --git a/arch/arm/mach-omap2/dpll3xxx.c
>>> b/arch/arm/mach-omap2/dpll3xxx.c
>>> index 3a0296c..ba218fb 100644
>>> --- a/arch/arm/mach-omap2/dpll3xxx.c
>>> +++ b/arch/arm/mach-omap2/dpll3xxx.c
>>> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw
>>> *hw,
>>>
>>>          dd = pclk->dpll_data;
>>>
>>> -       WARN_ON(!dd->enable_mask);
>>> -
>>> -       v = __raw_readl(dd->control_reg) & dd->enable_mask;
>>> -       v >>= __ffs(dd->enable_mask);
>>> -       if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
>>> +       if ((dd->flags & DPLL_J_TYPE) ||
>>> +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk))
>>>                  rate = parent_rate;
>>>          else
>>>                  rate = parent_rate * 2;
>>> +
>>>          return rate;
>>>   }
>>
>> Stefan, are you able to test the above?
>>
>> I'd rather have a proper fix for this, than hack omapdss =).
>
> Okay, I finally found some time to test this. The patch above generates
> this warning:
>
> arch/arm/mach-omap2/dpll3xxx.c: In function 'omap3_clkoutx2_recalc':
> arch/arm/mach-omap2/dpll3xxx.c:663:6: warning: passing argument 1 of '__clk_get_rate' from incompatible pointer type [enabled by default]
> include/linux/clk-provider.h:423:15: note: expected 'struct clk *' but argument is of type 'struct clk_hw_omap *'

Yea sorry about that, I just quickly hacked the patch together without 
testing it at all. :P

>
> I then changed it (not 100% sure if correctly) to this version:
>
> +       if ((dd->flags & DPLL_J_TYPE) ||
> +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk->hw.clk))
>
> And this seems to work. At least the clock rate mismatch warning does not
> appear with this patch applied (and without the clk_enable) in the
> bootlog any more.

Yea, looks good to me, however I guess I would like second opinion on 
this also.

-Tero

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Mike Turquette]

Quoting Tero Kristo (2013-09-13 04:34:54)
> On 09/13/2013 10:51 AM, Stefan Roese wrote:
> > On 11.09.2013 09:21, Tomi Valkeinen wrote:
> >> On 10/09/13 16:17, Tero Kristo wrote:
> >>
> >>> In theory, DPLLs can also be used in their bypass mode to feed customer
> >>> nodes clocks. I just think the check in the clkoutx2_recalc is wrong,
> >>> and should be enhanced to actually check what is the target mode for the
> >>> clock once it is enabled. Maybe something like this would work properly:
> >>>
> >>> diff --git a/arch/arm/mach-omap2/dpll3xxx.c
> >>> b/arch/arm/mach-omap2/dpll3xxx.c
> >>> index 3a0296c..ba218fb 100644
> >>> --- a/arch/arm/mach-omap2/dpll3xxx.c
> >>> +++ b/arch/arm/mach-omap2/dpll3xxx.c
> >>> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw
> >>> *hw,
> >>>
> >>>          dd = pclk->dpll_data;
> >>>
> >>> -       WARN_ON(!dd->enable_mask);
> >>> -
> >>> -       v = __raw_readl(dd->control_reg) & dd->enable_mask;
> >>> -       v >>= __ffs(dd->enable_mask);
> >>> -       if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
> >>> +       if ((dd->flags & DPLL_J_TYPE) ||
> >>> +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk))
> >>>                  rate = parent_rate;
> >>>          else
> >>>                  rate = parent_rate * 2;
> >>> +
> >>>          return rate;
> >>>   }
> >>
> >> Stefan, are you able to test the above?
> >>
> >> I'd rather have a proper fix for this, than hack omapdss =).
> >
> > Okay, I finally found some time to test this. The patch above generates
> > this warning:
> >
> > arch/arm/mach-omap2/dpll3xxx.c: In function 'omap3_clkoutx2_recalc':
> > arch/arm/mach-omap2/dpll3xxx.c:663:6: warning: passing argument 1 of '__clk_get_rate' from incompatible pointer type [enabled by default]
> > include/linux/clk-provider.h:423:15: note: expected 'struct clk *' but argument is of type 'struct clk_hw_omap *'
> 
> Yea sorry about that, I just quickly hacked the patch together without 
> testing it at all. :P
> 
> >
> > I then changed it (not 100% sure if correctly) to this version:
> >
> > +       if ((dd->flags & DPLL_J_TYPE) ||
> > +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk->hw.clk))
> >
> > And this seems to work. At least the clock rate mismatch warning does not
> > appear with this patch applied (and without the clk_enable) in the
> > bootlog any more.
> 
> Yea, looks good to me, however I guess I would like second opinion on 
> this also.

Looks right to me.

Regards,
Mike

> 
> -Tero

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2571 bytes --]

On 13/09/13 14:34, Kristo, Tero wrote:
> On 09/13/2013 10:51 AM, Stefan Roese wrote:
>> On 11.09.2013 09:21, Tomi Valkeinen wrote:
>>> On 10/09/13 16:17, Tero Kristo wrote:
>>>
>>>> In theory, DPLLs can also be used in their bypass mode to feed customer
>>>> nodes clocks. I just think the check in the clkoutx2_recalc is wrong,
>>>> and should be enhanced to actually check what is the target mode for the
>>>> clock once it is enabled. Maybe something like this would work properly:
>>>>
>>>> diff --git a/arch/arm/mach-omap2/dpll3xxx.c
>>>> b/arch/arm/mach-omap2/dpll3xxx.c
>>>> index 3a0296c..ba218fb 100644
>>>> --- a/arch/arm/mach-omap2/dpll3xxx.c
>>>> +++ b/arch/arm/mach-omap2/dpll3xxx.c
>>>> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw
>>>> *hw,
>>>>
>>>>          dd = pclk->dpll_data;
>>>>
>>>> -       WARN_ON(!dd->enable_mask);
>>>> -
>>>> -       v = __raw_readl(dd->control_reg) & dd->enable_mask;
>>>> -       v >>= __ffs(dd->enable_mask);
>>>> -       if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
>>>> +       if ((dd->flags & DPLL_J_TYPE) ||
>>>> +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk))
>>>>                  rate = parent_rate;
>>>>          else
>>>>                  rate = parent_rate * 2;
>>>> +
>>>>          return rate;
>>>>   }
>>>
>>> Stefan, are you able to test the above?
>>>
>>> I'd rather have a proper fix for this, than hack omapdss =).
>>
>> Okay, I finally found some time to test this. The patch above generates
>> this warning:
>>
>> arch/arm/mach-omap2/dpll3xxx.c: In function 'omap3_clkoutx2_recalc':
>> arch/arm/mach-omap2/dpll3xxx.c:663:6: warning: passing argument 1 of '__clk_get_rate' from incompatible pointer type [enabled by default]
>> include/linux/clk-provider.h:423:15: note: expected 'struct clk *' but argument is of type 'struct clk_hw_omap *'
> 
> Yea sorry about that, I just quickly hacked the patch together without 
> testing it at all. :P
> 
>>
>> I then changed it (not 100% sure if correctly) to this version:
>>
>> +       if ((dd->flags & DPLL_J_TYPE) ||
>> +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk->hw.clk))
>>
>> And this seems to work. At least the clock rate mismatch warning does not
>> appear with this patch applied (and without the clk_enable) in the
>> bootlog any more.
> 
> Yea, looks good to me, however I guess I would like second opinion on 
> this also.

Tero, can you queue this patch? Or who should handle this?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tero Kristo]

On 09/27/2013 11:41 AM, Tomi Valkeinen wrote:
> On 13/09/13 14:34, Kristo, Tero wrote:
>> On 09/13/2013 10:51 AM, Stefan Roese wrote:
>>> On 11.09.2013 09:21, Tomi Valkeinen wrote:
>>>> On 10/09/13 16:17, Tero Kristo wrote:
>>>>
>>>>> In theory, DPLLs can also be used in their bypass mode to feed customer
>>>>> nodes clocks. I just think the check in the clkoutx2_recalc is wrong,
>>>>> and should be enhanced to actually check what is the target mode for the
>>>>> clock once it is enabled. Maybe something like this would work properly:
>>>>>
>>>>> diff --git a/arch/arm/mach-omap2/dpll3xxx.c
>>>>> b/arch/arm/mach-omap2/dpll3xxx.c
>>>>> index 3a0296c..ba218fb 100644
>>>>> --- a/arch/arm/mach-omap2/dpll3xxx.c
>>>>> +++ b/arch/arm/mach-omap2/dpll3xxx.c
>>>>> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw
>>>>> *hw,
>>>>>
>>>>>           dd = pclk->dpll_data;
>>>>>
>>>>> -       WARN_ON(!dd->enable_mask);
>>>>> -
>>>>> -       v = __raw_readl(dd->control_reg) & dd->enable_mask;
>>>>> -       v >>= __ffs(dd->enable_mask);
>>>>> -       if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
>>>>> +       if ((dd->flags & DPLL_J_TYPE) ||
>>>>> +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk))
>>>>>                   rate = parent_rate;
>>>>>           else
>>>>>                   rate = parent_rate * 2;
>>>>> +
>>>>>           return rate;
>>>>>    }
>>>>
>>>> Stefan, are you able to test the above?
>>>>
>>>> I'd rather have a proper fix for this, than hack omapdss =).
>>>
>>> Okay, I finally found some time to test this. The patch above generates
>>> this warning:
>>>
>>> arch/arm/mach-omap2/dpll3xxx.c: In function 'omap3_clkoutx2_recalc':
>>> arch/arm/mach-omap2/dpll3xxx.c:663:6: warning: passing argument 1 of '__clk_get_rate' from incompatible pointer type [enabled by default]
>>> include/linux/clk-provider.h:423:15: note: expected 'struct clk *' but argument is of type 'struct clk_hw_omap *'
>>
>> Yea sorry about that, I just quickly hacked the patch together without
>> testing it at all. :P
>>
>>>
>>> I then changed it (not 100% sure if correctly) to this version:
>>>
>>> +       if ((dd->flags & DPLL_J_TYPE) ||
>>> +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk->hw.clk))
>>>
>>> And this seems to work. At least the clock rate mismatch warning does not
>>> appear with this patch applied (and without the clk_enable) in the
>>> bootlog any more.
>>
>> Yea, looks good to me, however I guess I would like second opinion on
>> this also.
>
> Tero, can you queue this patch? Or who should handle this?

I can't queue anything as I don't have maintainership on any of this 
stuff. Paul / Tony should go with this.

-Tero

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

On 27/09/13 14:24, Tero Kristo wrote:

>> Tero, can you queue this patch? Or who should handle this?
> 
> I can't queue anything as I don't have maintainership on any of this
> stuff. Paul / Tony should go with this.

Well, I mainly meant preparing a patch with proper description and
sending to relevant maintainers.

For some reason I can't reproduce the issue with 3.12-rc1. I guess some
other driver enables the clock now, which wasn't there in 3.11. So as
far as DSS is concerned, things look fine now without the patch. But if
the clock code is wrong, as I understood, maybe it's still good to have
the patch applied.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Paul Walmsley]

On Tue, 10 Sep 2013, Tero Kristo wrote:

> In theory, DPLLs can also be used in their bypass mode to feed customer nodes
> clocks. I just think the check in the clkoutx2_recalc is wrong, and should be
> enhanced to actually check what is the target mode for the clock once it is
> enabled. Maybe something like this would work properly:
> 
> diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
> index 3a0296c..ba218fb 100644
> --- a/arch/arm/mach-omap2/dpll3xxx.c
> +++ b/arch/arm/mach-omap2/dpll3xxx.c
> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw,
> 
>         dd = pclk->dpll_data;
> 
> -       WARN_ON(!dd->enable_mask);
> -
> -       v = __raw_readl(dd->control_reg) & dd->enable_mask;
> -       v >>= __ffs(dd->enable_mask);
> -       if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
> +       if ((dd->flags & DPLL_J_TYPE) ||
> +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk))
>                 rate = parent_rate;
>         else
>                 rate = parent_rate * 2;
> +
>         return rate;
>  }
> 

[...]

> Getting comment from someone like Paul would probably help here.

Looks like this is a regression from the CCF port.

Seems to me that the code above assumes that when the DPLL's rate is set 
to the same rate as the bypass clock's rate, we can assume that the DPLL 
is in bypass.  I wonder if that's valid in a case where a previous OS or 
bootloader may have programmed the DPLL.

Sounds to me like the best way to fix it would be to test whether this 
code is intended to return the "target rate" (when the struct clk 
representing the DPLL is disabled), versus the "current rate" (when the 
struct clk representing the DPLL is enabled).  If it's the target rate, 
then there's no point checking the current state of the hardware.  A check 
similar to the above would probably be fine.  Otherwise, seems best to use 
the existing code that does test the PLL state.



- Paul

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Mike Turquette]

Quoting Paul Walmsley (2013-10-07 01:21:16)
> On Tue, 10 Sep 2013, Tero Kristo wrote:
> 
> > In theory, DPLLs can also be used in their bypass mode to feed customer nodes
> > clocks. I just think the check in the clkoutx2_recalc is wrong, and should be
> > enhanced to actually check what is the target mode for the clock once it is
> > enabled. Maybe something like this would work properly:
> > 
> > diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
> > index 3a0296c..ba218fb 100644
> > --- a/arch/arm/mach-omap2/dpll3xxx.c
> > +++ b/arch/arm/mach-omap2/dpll3xxx.c
> > @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw,
> > 
> >         dd = pclk->dpll_data;
> > 
> > -       WARN_ON(!dd->enable_mask);
> > -
> > -       v = __raw_readl(dd->control_reg) & dd->enable_mask;
> > -       v >>= __ffs(dd->enable_mask);
> > -       if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE))
> > +       if ((dd->flags & DPLL_J_TYPE) ||
> > +           __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk))
> >                 rate = parent_rate;
> >         else
> >                 rate = parent_rate * 2;
> > +
> >         return rate;
> >  }
> > 
> 
> [...]
> 
> > Getting comment from someone like Paul would probably help here.
> 
> Looks like this is a regression from the CCF port.
> 
> Seems to me that the code above assumes that when the DPLL's rate is set 
> to the same rate as the bypass clock's rate, we can assume that the DPLL 
> is in bypass.  I wonder if that's valid in a case where a previous OS or 
> bootloader may have programmed the DPLL.

Well it used to be that calling clk_set_rate(dpll, bypass_rate) would
put it in bypass, I don't know if that is still the case. But you are
right that having the implicit assumption that 'bypass rate' == 'DPLL in
bypass' is not safe since a bootloader could lock this PLL to the same
rate as it's bypass rate.

I hope that the bypass rate stuff does not get swept away in the changes
since it is an interesting way to save a little power.

Regards,
Mike

> 
> Sounds to me like the best way to fix it would be to test whether this 
> code is intended to return the "target rate" (when the struct clk 
> representing the DPLL is disabled), versus the "current rate" (when the 
> struct clk representing the DPLL is enabled).  If it's the target rate, 
> then there's no point checking the current state of the hardware.  A check 
> similar to the above would probably be fine.  Otherwise, seems best to use 
> the existing code that does test the PLL state.
> 
> 
> 
> - Paul

Re: Odd behavior with dpll4_m4x2_ck on omap3 + DT

[Paul Walmsley]

On Mon, 7 Oct 2013, Mike Turquette wrote:

> Well it used to be that calling clk_set_rate(dpll, bypass_rate) would
> put it in bypass, I don't know if that is still the case. But you are
> right that having the implicit assumption that 'bypass rate' == 'DPLL in
> bypass' is not safe since a bootloader could lock this PLL to the same
> rate as it's bypass rate.
> 
> I hope that the bypass rate stuff does not get swept away in the changes
> since it is an interesting way to save a little power.

Yeah, I don't think anyone's proposing to remove it, as far as I know.

Am more concerned about any similar lurking problems in low-level clock 
code that use the current state of the clock hardware to determine a 
possible future rate.  Am hoping that this particular issue is simply due 
to the state dependency between the CLKOUTX2 node and the PLL node.  
Considering this issue, in retrospect, it probably would have been better 
to merge the CLKOUTX2 node with the PLL.


- Paul