* Re: [RFC PATCH v2 3/4] powerpc: refactor of_get_cpu_node to support other architectures
From: Benjamin Herrenschmidt @ 2013-08-16 22:13 UTC (permalink / raw)
To: Sudeep KarkadaNagesha
Cc: Jonas Bonn, devicetree, Michal Simek, linux-pm, linux-kernel,
Rob Herring, Rafael J. Wysocki, Grant Likely, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1376674791-28244-2-git-send-email-Sudeep.KarkadaNagesha@arm.com>
On Fri, 2013-08-16 at 18:39 +0100, Sudeep KarkadaNagesha wrote:
> +static bool __of_find_n_match_cpu_property(struct device_node *cpun,
> + const char *prop_name, int cpu, unsigned int
> *thread)
> +{
> + const __be32 *cell;
> + int ac, prop_len, tid;
> + u64 hwid;
> +
> + ac = of_n_addr_cells(cpun);
> + cell = of_get_property(cpun, prop_name, &prop_len);
> + if (!cell)
> + return false;
> + prop_len /= sizeof(*cell);
> + for (tid = 0; tid < prop_len; tid++) {
> + hwid = of_read_number(cell, ac);
> + if (arch_match_cpu_phys_id(cpu, hwid)) {
> + if (thread)
> + *thread = tid;
> + return true;
> + }
> + cell += ac;
> + }
> + return false;
> +}
The only problem I can see here is if "ac" is not 1, that will not work
for the ibm,ppc-interrupt-server#s case. IE. The latter is always 1 cell
per entry, only "reg" depends on #address-cells.
However that's only a theorical problem since on ppc #address-cells of
/cpus is always 1...
Cheers,
Ben.
^ permalink raw reply
* RE: [RFC PATCH v2 00/11] Add (de)compression support to pstore
From: Luck, Tony @ 2013-08-16 22:15 UTC (permalink / raw)
To: Aruna Balakrishnaiah, linuxppc-dev@ozlabs.org,
linux-kernel@vger.kernel.org, keescook@chromium.org
Cc: jkenisto@linux.vnet.ibm.com, cbouatmailru@gmail.com,
mahesh@linux.vnet.ibm.com, ccross@android.com
In-Reply-To: <20130816131403.3338.82330.stgit@aruna-ThinkPad-T420>
PiBOZWVkcyB0ZXN0aW5nIHdpdGggZXJzdCBiYWNrZW5kLCBlZml2YXJzIGFuZCBwZXJzaXN0ZW50
IHJhbS4NCg0KVGVzdGVkIGFnYWluc3QgRVJTVCAtIHdvcmtzIGZpbmUgZm9yIG1lIG5vdy4NCg0K
TmVlZCB0byBzdGFyZSBhdCB0aGUgY29kZSB0byBzZWUgaWYgdGhlcmUgYXJlIGFueSBtb3JlIGJp
dHMgdGhhdCBjb3VsZCBiZSBjbGVhbmVkIHVwLg0KDQpUaGFua3MgZm9yIGFkZHJlc3NpbmcgbXkg
aXNzdWVzIGZyb20gdjENCg0KLVRvbnkNCg==
^ permalink raw reply
* Re: [RFC PATCH v2 4/4] of: move of_get_cpu_node implementation to DT core library
From: Benjamin Herrenschmidt @ 2013-08-16 22:14 UTC (permalink / raw)
To: Sudeep KarkadaNagesha
Cc: Jonas Bonn, devicetree, Michal Simek, linux-pm, linux-kernel,
Rob Herring, Rafael J. Wysocki, Grant Likely, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1376674791-28244-3-git-send-email-Sudeep.KarkadaNagesha@arm.com>
On Fri, 2013-08-16 at 18:39 +0100, Sudeep KarkadaNagesha wrote:
> +#ifdef CONFIG_PPC
> + /* Check for historical "ibm,ppc-interrupt-server#s" property
> + * for thread ids on PowerPC. If it doesn't exist fallback to
> + * standard "reg" property.
> + */
> + if (__of_find_n_match_cpu_property(cpun,
> + "ibm,ppc-interrupt-server#s", cpu, thread))
> + return cpun;
> +#endif
It's not "historical". It's still very much in use and well defined in PAPR :-)
Cheers,
Ben.
^ permalink raw reply
* Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
From: Stephen Warren @ 2013-08-16 22:49 UTC (permalink / raw)
To: Shawn Guo
Cc: devicetree, alsa-devel, lars, linuxppc-dev, Nicolin Chen, timur,
rob.herring, broonie, p.zabel, Fabio Estevam
In-Reply-To: <20130815092428.GI16915@S2101-09.ap.freescale.net>
On 08/15/2013 03:24 AM, Shawn Guo wrote:
> On Thu, Aug 15, 2013 at 10:18:23AM +0800, Nicolin Chen wrote:
>> Hi Stephen,
>>
>> On Wed, Aug 14, 2013 at 09:47:19AM -0600, Stephen Warren wrote:
>>> If the clock source name list is different, then it needs a different
>>> compatible value, so that each compatible value can specify which clock
>>> names are required.
>>>
>>> Also, the compatible value itself should always include the exact HW
>>> that's present (most specific HW version), as well as any other HW it's
>>> compatible with.
>>
>> Thank you for the comments. Yes, I did so in v1-v3, but after rethinking
>> about the situation (Actually both the HW version and the clock mux itself
>> are same, just the clock sources connecting to the mux might be different),
>> so I decided to do this by abstracting the driver from those source info
>> and letting DT binding to pass such information. Because I think putting
>> the clock sources into the driver differed by compatible value would make
>> the driver more like SoC-specified, not the ideal way -- SoC-independent,
>> since the clock sources are based on SoC design, not on itself.
>
> +1
>
> It's pretty much the differences at SoC integration level not the IP
> itself, and it just happens to be handled in a register of the IP.
OK, if the difference are the sources of the clocks and not the set of
clocks, then there's no issue. I can't remember what triggered my
comments above, but obviously it wasn't clear that this was the case.
^ permalink raw reply
* MPC8315 reboot failure, lockdep splat possibly related?
From: Anthony Foiani @ 2013-08-17 1:39 UTC (permalink / raw)
To: linuxppc-dev
Greetings.
I've been experiencing occasional lockups at reboot for a few weeks,
but only once every 10-20 boots. A good reboot looks like this:
[47529.721640] lm77 0-0048: shutdown
[47529.725160] rtc-m41t80 0-0068: shutdown
[47529.729169] i2c i2c-0: shutdown
[47529.732534] fsl-ehci fsl-ehci.0: shutdown
[47529.736842] sd 1:0:0:0: shutdown
[47529.740239] sd 1:0:0:0: [sda] Synchronizing SCSI cache
[47529.747091] uio_pci_generic 0000:00:0a.0: shutdown
[47529.752079] pci 0000:00:00.0: shutdown
[47529.756021] Restarting system.
While a bad one fails after the EHCI shutdown:
[ 747.578001] lm77 0-0048: shutdown
[ 747.581522] rtc-m41t80 0-0068: shutdown
[ 747.585538] i2c i2c-0: shutdown
[ 747.588909] sd 1:0:0:0: shutdown
[ 747.592304] sd 1:0:0:0: [sda] Synchronizing SCSI cache
[ 747.597973] fsl-ehci fsl-ehci.0: shutdown
I enabled lockdep, and I get this splat on every boot, regardless of
whether it locks up at reboot or not. Could it possibly be related?
Any other ideas on how to avoid the reboot lockup?
[ 9.086051] =================================
[ 9.090393] [ INFO: inconsistent lock state ]
[ 9.094744] 3.9.7-ajf-gc39503d #1 Not tainted
[ 9.099087] ---------------------------------
[ 9.103432] inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
[ 9.109431] scsi_eh_1/39 [HC1[1]:SC0[0]:HE0:SE1] takes:
[ 9.114642] (&(&host->lock)->rlock){?.+...}, at: [<c02f4168>] sata_fsl_interrupt+0x50/0x250
[ 9.123137] {HARDIRQ-ON-W} state was registered at:
[ 9.128004] [<c006cdb8>] lock_acquire+0x90/0xf4
[ 9.132737] [<c043ef04>] _raw_spin_lock+0x34/0x4c
[ 9.137645] [<c02f3560>] fsl_sata_set_irq_coalescing+0x68/0x100
[ 9.143750] [<c02f36a0>] sata_fsl_init_controller+0xa8/0xc0
[ 9.149505] [<c02f3f10>] sata_fsl_probe+0x17c/0x2e8
[ 9.154568] [<c02acc90>] driver_probe_device+0x90/0x248
[ 9.159987] [<c02acf0c>] __driver_attach+0xc4/0xc8
[ 9.164964] [<c02aae74>] bus_for_each_dev+0x5c/0xa8
[ 9.170028] [<c02ac218>] bus_add_driver+0x100/0x26c
[ 9.175091] [<c02ad638>] driver_register+0x88/0x198
[ 9.180155] [<c0003a24>] do_one_initcall+0x58/0x1b4
[ 9.185226] [<c05aeeac>] kernel_init_freeable+0x118/0x1c0
[ 9.190823] [<c0004110>] kernel_init+0x18/0x108
[ 9.195542] [<c000f6b8>] ret_from_kernel_thread+0x64/0x6c
[ 9.201142] irq event stamp: 160
[ 9.204366] hardirqs last enabled at (159): [<c043f778>] _raw_spin_unlock_irq+0x30/0x50
[ 9.212469] hardirqs last disabled at (160): [<c000f414>] reenable_mmu+0x30/0x88
[ 9.219867] softirqs last enabled at (144): [<c002ae5c>] __do_softirq+0x168/0x218
[ 9.227435] softirqs last disabled at (137): [<c002b0d4>] irq_exit+0xa8/0xb4
[ 9.234481]
[ 9.234481] other info that might help us debug this:
[ 9.240995] Possible unsafe locking scenario:
[ 9.240995]
[ 9.246898] CPU0
[ 9.249337] ----
[ 9.251776] lock(&(&host->lock)->rlock);
[ 9.255878] <Interrupt>
[ 9.258492] lock(&(&host->lock)->rlock);
[ 9.262765]
[ 9.262765] *** DEADLOCK ***
[ 9.262765]
[ 9.268684] no locks held by scsi_eh_1/39.
[ 9.272767]
[ 9.272767] stack backtrace:
[ 9.277117] Call Trace:
[ 9.279589] [cfff9da0] [c0008504] show_stack+0x48/0x150 (unreliable)
[ 9.285972] [cfff9de0] [c0447d5c] print_usage_bug.part.35+0x268/0x27c
[ 9.292425] [cfff9e10] [c006ace4] mark_lock+0x2ac/0x658
[ 9.297660] [cfff9e40] [c006b7e4] __lock_acquire+0x754/0x1840
[ 9.303414] [cfff9ee0] [c006cdb8] lock_acquire+0x90/0xf4
[ 9.308745] [cfff9f20] [c043ef04] _raw_spin_lock+0x34/0x4c
[ 9.314250] [cfff9f30] [c02f4168] sata_fsl_interrupt+0x50/0x250
[ 9.320187] [cfff9f70] [c0079ff0] handle_irq_event_percpu+0x90/0x254
[ 9.326547] [cfff9fc0] [c007a1fc] handle_irq_event+0x48/0x78
[ 9.332220] [cfff9fe0] [c007c95c] handle_level_irq+0x9c/0x104
[ 9.337981] [cfff9ff0] [c000d978] call_handle_irq+0x18/0x28
[ 9.343568] [cc7139f0] [c000608c] do_IRQ+0xf0/0x1a8
[ 9.348464] [cc713a20] [c000fc8c] ret_from_except+0x0/0x14
[ 9.353983] --- Exception: 501 at _raw_spin_unlock_irq+0x40/0x50
[ 9.353983] LR = _raw_spin_unlock_irq+0x30/0x50
[ 9.364839] [cc713af0] [c043db10] wait_for_common+0xac/0x188
[ 9.370513] [cc713b30] [c02ddee4] ata_exec_internal_sg+0x2b0/0x4f0
[ 9.376699] [cc713be0] [c02de18c] ata_exec_internal+0x68/0xa8
[ 9.382454] [cc713c20] [c02de4b8] ata_dev_read_id+0x158/0x594
[ 9.388205] [cc713ca0] [c02ec244] ata_eh_recover+0xd88/0x13d0
[ 9.393962] [cc713d20] [c02f2520] sata_pmp_error_handler+0xc0/0x8ac
[ 9.400234] [cc713dd0] [c02ecdc8] ata_scsi_port_error_handler+0x464/0x5e8
[ 9.407023] [cc713e10] [c02ecfd0] ata_scsi_error+0x84/0xb8
[ 9.412528] [cc713e40] [c02c4974] scsi_error_handler+0xd8/0x47c
[ 9.418457] [cc713eb0] [c004737c] kthread+0xa8/0xac
[ 9.423355] [cc713f40] [c000f6b8] ret_from_kernel_thread+0x64/0x6c
A full set of kernel messages from a hanging (fails to reboot) session
is here:
http://scrye.com/~tkil/linux/fsl-sata-lockdep-201308/hang-log.txt
And a full set of messages for a boot that reboots successfully:
http://scrye.com/~tkil/linux/fsl-sata-lockdep-201308/no-hang-log.txt
The associated config file:
http://scrye.com/~tkil/linux/fsl-sata-lockdep-201308/config.txt
The only addition I've made to this section of the kernel is my "SATA
speed limit patch", discussed a few weeks back:
http://permalink.gmane.org/gmane.linux.ports.ppc.embedded/58969
That patch does touch sata_fsl_probe, but it just sets a value -- it
doesn't do any locking (for better or for worse), nor does it call any
other functions (and it seems that it's a function further down the
stack that is triggering lockdep).
I took a quick look at the diffs between 3.9.7 (both upstream and my
variant) and the current head of linux-stable, and didn't see anything
that looked relevant. The two main changes I saw were switching from
dev_get_drvdata to platform_get_drvdata and adding the rx_watermark.
I did see that dev_set_drvdata was removed from sata_fsl_probe's exit
path; not sure if that could cause this sort of error.
If anyone has ideas on how to avoid the reboot lockup, I would greatly
appreciate it.
Thank you for your time!
Best regards,
Anthony Foiani
^ permalink raw reply
* RE: MPC8315 reboot failure, lockdep splat possibly related?
From: Bhushan Bharat-R65777 @ 2013-08-17 4:58 UTC (permalink / raw)
To: Anthony Foiani, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <g61v5p02b.fsf@dworkin.scrye.com>
> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+bharat.bhushan=3Dfreescale.com@lists.ozlabs.org] On Behalf Of Ant=
hony
> Foiani
> Sent: Saturday, August 17, 2013 7:10 AM
> To: linuxppc-dev@lists.ozlabs.org
> Subject: MPC8315 reboot failure, lockdep splat possibly related?
>=20
>=20
> Greetings.
>=20
> I've been experiencing occasional lockups at reboot for a few weeks, but =
only
> once every 10-20 boots. A good reboot looks like this:
>=20
> [47529.721640] lm77 0-0048: shutdown
> [47529.725160] rtc-m41t80 0-0068: shutdown
> [47529.729169] i2c i2c-0: shutdown
> [47529.732534] fsl-ehci fsl-ehci.0: shutdown
> [47529.736842] sd 1:0:0:0: shutdown
> [47529.740239] sd 1:0:0:0: [sda] Synchronizing SCSI cache
> [47529.747091] uio_pci_generic 0000:00:0a.0: shutdown
> [47529.752079] pci 0000:00:00.0: shutdown
> [47529.756021] Restarting system.
>=20
> While a bad one fails after the EHCI shutdown:
>=20
> [ 747.578001] lm77 0-0048: shutdown
> [ 747.581522] rtc-m41t80 0-0068: shutdown
> [ 747.585538] i2c i2c-0: shutdown
> [ 747.588909] sd 1:0:0:0: shutdown
> [ 747.592304] sd 1:0:0:0: [sda] Synchronizing SCSI cache
> [ 747.597973] fsl-ehci fsl-ehci.0: shutdown
>=20
> I enabled lockdep, and I get this splat on every boot, regardless of whet=
her it
> locks up at reboot or not. Could it possibly be related?
> Any other ideas on how to avoid the reboot lockup?
>=20
> [ 9.086051] =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> [ 9.090393] [ INFO: inconsistent lock state ]
> [ 9.094744] 3.9.7-ajf-gc39503d #1 Not tainted
> [ 9.099087] ---------------------------------
> [ 9.103432] inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
> [ 9.109431] scsi_eh_1/39 [HC1[1]:SC0[0]:HE0:SE1] takes:
> [ 9.114642] (&(&host->lock)->rlock){?.+...}, at: [<c02f4168>]
> sata_fsl_interrupt+0x50/0x250
> [ 9.123137] {HARDIRQ-ON-W} state was registered at:
> [ 9.128004] [<c006cdb8>] lock_acquire+0x90/0xf4
> [ 9.132737] [<c043ef04>] _raw_spin_lock+0x34/0x4c
> [ 9.137645] [<c02f3560>] fsl_sata_set_irq_coalescing+0x68/0x100
> [ 9.143750] [<c02f36a0>] sata_fsl_init_controller+0xa8/0xc0
> [ 9.149505] [<c02f3f10>] sata_fsl_probe+0x17c/0x2e8
> [ 9.154568] [<c02acc90>] driver_probe_device+0x90/0x248
> [ 9.159987] [<c02acf0c>] __driver_attach+0xc4/0xc8
> [ 9.164964] [<c02aae74>] bus_for_each_dev+0x5c/0xa8
> [ 9.170028] [<c02ac218>] bus_add_driver+0x100/0x26c
> [ 9.175091] [<c02ad638>] driver_register+0x88/0x198
> [ 9.180155] [<c0003a24>] do_one_initcall+0x58/0x1b4
> [ 9.185226] [<c05aeeac>] kernel_init_freeable+0x118/0x1c0
> [ 9.190823] [<c0004110>] kernel_init+0x18/0x108
> [ 9.195542] [<c000f6b8>] ret_from_kernel_thread+0x64/0x6c
> [ 9.201142] irq event stamp: 160
> [ 9.204366] hardirqs last enabled at (159): [<c043f778>]
> _raw_spin_unlock_irq+0x30/0x50
> [ 9.212469] hardirqs last disabled at (160): [<c000f414>]
> reenable_mmu+0x30/0x88
> [ 9.219867] softirqs last enabled at (144): [<c002ae5c>]
> __do_softirq+0x168/0x218
> [ 9.227435] softirqs last disabled at (137): [<c002b0d4>]
> irq_exit+0xa8/0xb4
> [ 9.234481]
> [ 9.234481] other info that might help us debug this:
> [ 9.240995] Possible unsafe locking scenario:
> [ 9.240995]
> [ 9.246898] CPU0
> [ 9.249337] ----
> [ 9.251776] lock(&(&host->lock)->rlock);
> [ 9.255878] <Interrupt>
> [ 9.258492] lock(&(&host->lock)->rlock);
> [ 9.262765]
> [ 9.262765] *** DEADLOCK ***
You should get rid of this by changing spin_lock/unlock() in fsl_sata_set_i=
rq_coalescing() to spin_lock_irqsave/restore()
-Bharat
> [ 9.262765]
> [ 9.268684] no locks held by scsi_eh_1/39.
> [ 9.272767]
> [ 9.272767] stack backtrace:
> [ 9.277117] Call Trace:
> [ 9.279589] [cfff9da0] [c0008504] show_stack+0x48/0x150 (unreliable)
> [ 9.285972] [cfff9de0] [c0447d5c] print_usage_bug.part.35+0x268/0x27=
c
> [ 9.292425] [cfff9e10] [c006ace4] mark_lock+0x2ac/0x658
> [ 9.297660] [cfff9e40] [c006b7e4] __lock_acquire+0x754/0x1840
> [ 9.303414] [cfff9ee0] [c006cdb8] lock_acquire+0x90/0xf4
> [ 9.308745] [cfff9f20] [c043ef04] _raw_spin_lock+0x34/0x4c
> [ 9.314250] [cfff9f30] [c02f4168] sata_fsl_interrupt+0x50/0x250
> [ 9.320187] [cfff9f70] [c0079ff0] handle_irq_event_percpu+0x90/0x254
> [ 9.326547] [cfff9fc0] [c007a1fc] handle_irq_event+0x48/0x78
> [ 9.332220] [cfff9fe0] [c007c95c] handle_level_irq+0x9c/0x104
> [ 9.337981] [cfff9ff0] [c000d978] call_handle_irq+0x18/0x28
> [ 9.343568] [cc7139f0] [c000608c] do_IRQ+0xf0/0x1a8
> [ 9.348464] [cc713a20] [c000fc8c] ret_from_except+0x0/0x14
> [ 9.353983] --- Exception: 501 at _raw_spin_unlock_irq+0x40/0x50
> [ 9.353983] LR =3D _raw_spin_unlock_irq+0x30/0x50
> [ 9.364839] [cc713af0] [c043db10] wait_for_common+0xac/0x188
> [ 9.370513] [cc713b30] [c02ddee4] ata_exec_internal_sg+0x2b0/0x4f0
> [ 9.376699] [cc713be0] [c02de18c] ata_exec_internal+0x68/0xa8
> [ 9.382454] [cc713c20] [c02de4b8] ata_dev_read_id+0x158/0x594
> [ 9.388205] [cc713ca0] [c02ec244] ata_eh_recover+0xd88/0x13d0
> [ 9.393962] [cc713d20] [c02f2520] sata_pmp_error_handler+0xc0/0x8ac
> [ 9.400234] [cc713dd0] [c02ecdc8] ata_scsi_port_error_handler+0x464/=
0x5e8
> [ 9.407023] [cc713e10] [c02ecfd0] ata_scsi_error+0x84/0xb8
> [ 9.412528] [cc713e40] [c02c4974] scsi_error_handler+0xd8/0x47c
> [ 9.418457] [cc713eb0] [c004737c] kthread+0xa8/0xac
> [ 9.423355] [cc713f40] [c000f6b8] ret_from_kernel_thread+0x64/0x6c
>=20
> A full set of kernel messages from a hanging (fails to reboot) session is=
here:
>=20
> http://scrye.com/~tkil/linux/fsl-sata-lockdep-201308/hang-log.txt
>=20
> And a full set of messages for a boot that reboots successfully:
>=20
> http://scrye.com/~tkil/linux/fsl-sata-lockdep-201308/no-hang-log.txt
>=20
> The associated config file:
>=20
> http://scrye.com/~tkil/linux/fsl-sata-lockdep-201308/config.txt
>=20
> The only addition I've made to this section of the kernel is my "SATA spe=
ed
> limit patch", discussed a few weeks back:
>=20
> http://permalink.gmane.org/gmane.linux.ports.ppc.embedded/58969
>=20
> That patch does touch sata_fsl_probe, but it just sets a value -- it does=
n't do
> any locking (for better or for worse), nor does it call any other functio=
ns (and
> it seems that it's a function further down the stack that is triggering
> lockdep).
>=20
> I took a quick look at the diffs between 3.9.7 (both upstream and my
> variant) and the current head of linux-stable, and didn't see anything th=
at
> looked relevant. The two main changes I saw were switching from dev_get_=
drvdata
> to platform_get_drvdata and adding the rx_watermark.
> I did see that dev_set_drvdata was removed from sata_fsl_probe's exit pat=
h; not
> sure if that could cause this sort of error.
>=20
> If anyone has ideas on how to avoid the reboot lockup, I would greatly
> appreciate it.
>=20
> Thank you for your time!
>=20
> Best regards,
> Anthony Foiani
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [RFC PATCH v2 3/4] powerpc: refactor of_get_cpu_node to support other architectures
From: Tomasz Figa @ 2013-08-17 10:50 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Jonas Bonn, devicetree, Michal Simek, linux-pm,
Sudeep KarkadaNagesha, linux-kernel, Rob Herring,
Rafael J. Wysocki, Grant Likely, linuxppc-dev
In-Reply-To: <1376674791-28244-2-git-send-email-Sudeep.KarkadaNagesha@arm.com>
Hi Sudeep,
This looks good to me overall, but I have one more question inline.
On Friday 16 of August 2013 18:39:50 Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>
> Currently different drivers requiring to access cpu device node are
> parsing the device tree themselves. Since the ordering in the DT need
> not match the logical cpu ordering, the parsing logic needs to consider
> that. However, this has resulted in lots of code duplication and in some
> cases even incorrect logic.
>
> It's better to consolidate them by adding support for getting cpu
> device node for a given logical cpu index in DT core library. However
> logical to physical index mapping can be architecture specific.
>
> PowerPC has it's own implementation to get the cpu node for a given
> logical index.
>
> This patch refactors the current implementation of of_get_cpu_node.
> This in preparation to move the implementation to DT core library.
> It separates out the logical to physical mapping so that a default
> matching of the physical id to the logical cpu index can be added
> when moved to common code. Architecture specific code can override it.
>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> ---
> arch/powerpc/kernel/prom.c | 76
> ++++++++++++++++++++++++++++------------------ 1 file changed, 47
> insertions(+), 29 deletions(-)
>
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index eb23ac9..fb12be6 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -865,45 +865,63 @@ static int __init prom_reconfig_setup(void)
> __initcall(prom_reconfig_setup);
> #endif
>
> +bool arch_match_cpu_phys_id(int cpu, u64 phys_id)
> +{
> + return (int)phys_id == get_hard_smp_processor_id(cpu);
> +}
> +
> +static bool __of_find_n_match_cpu_property(struct device_node *cpun,
> + const char *prop_name, int cpu, unsigned int
*thread)
> +{
> + const __be32 *cell;
> + int ac, prop_len, tid;
> + u64 hwid;
> +
> + ac = of_n_addr_cells(cpun);
> + cell = of_get_property(cpun, prop_name, &prop_len);
> + if (!cell)
> + return false;
I wonder how would this handle uniprocessor ARM (pre-v7) cores, for which
the updated bindings[1] define #address-cells = <0> and so no reg
property.
[1] - http://thread.gmane.org/gmane.linux.ports.arm.kernel/260795
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
From: Tomasz Figa @ 2013-08-17 12:26 UTC (permalink / raw)
To: Sascha Hauer
Cc: mark.rutland, devicetree, alsa-devel, lars, ian.campbell,
pawel.moll, swarren, festevam, Nicolin Chen, timur, rob.herring,
broonie, p.zabel, galak, shawn.guo, linuxppc-dev
In-Reply-To: <20130816085632.GO26614@pengutronix.de>
On Friday 16 of August 2013 10:56:32 Sascha Hauer wrote:
> On Fri, Aug 16, 2013 at 04:01:25PM +0800, Nicolin Chen wrote:
> > Hi Sascha,
> >
> > Thank you for the detailed comments.
> >
> > On Fri, Aug 16, 2013 at 09:08:18AM +0200, Sascha Hauer wrote:
> > > Which of them the driver should use is configuration and thus
> > > normally
> > > should *not* be described in the devicetree. However, there may be
> > > no
> > > good way for the driver to know which clock to use in which case.
> > > There
> > > may be additional board requirements which are unknown to the
> > > driver. So in this case it might be valid to put the information
> > > which clock to use into the devicetree. But be aware that from the
> > > moment you put this information into the devicetree the driver is
> > > no longer free to chose the best clock, even if in future we find a
> > > good way to automatically guess the best clock. Do you have some
> > > insights in which case I would use which input clock? Is this only
> > > about which clock has the best suitable input frequency or is this
> > > also about synchronization of the audio signal with some other
> > > unit?
> >
> > I understand. What I'm thinking now is to let the driver find the best
> > clock source for tx clock and a correspond divisor like this:
> >
> > "tx<0-8>" Optional Tx clock source for spdif playback.
> >
> > If absent, will use core clock.
> > The index from 0 to 8 is identical
> > to the clock source list described
> > in TxClk_Source bit of register STC.
> > Multiple clock source are allowed
> > for this tx clock source. The driver
> > will select one source from them for
> > each supported sample rate according
> > to the clock rates of these provided
> > clock sources.
>
> You mean tx<0-7>.
>
> Also I would make this option required. Use a dummy clock for mux inputs
> that are grounded for a specific SoC.
Why do you need a dummy clock?
The driver can simply try to grab all the possible clocks and discard
those that failed, so you can just keep those grounded clocks unspecified.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
From: Tomasz Figa @ 2013-08-17 12:28 UTC (permalink / raw)
To: Sascha Hauer
Cc: mark.rutland, devicetree, alsa-devel, lars, ian.campbell,
pawel.moll, swarren, festevam, Nicolin Chen, timur, rob.herring,
broonie, p.zabel, galak, shawn.guo, linuxppc-dev
In-Reply-To: <20130816101151.GQ26614@pengutronix.de>
On Friday 16 of August 2013 12:11:51 Sascha Hauer wrote:
> On Fri, Aug 16, 2013 at 05:53:58PM +0800, Nicolin Chen wrote:
> > On Fri, Aug 16, 2013 at 10:56:32AM +0200, Sascha Hauer wrote:
> > > > "tx<0-8>" Optional Tx clock source for spdif
playback.
> > > >
> > > > If absent, will use core clock.
> > > > The index from 0 to 8 is identical
> > > > to the clock source list described
> > > > in TxClk_Source bit of register
STC.
> > > > Multiple clock source are allowed
> > > > for this tx clock source. The
driver
> > > > will select one source from them
for
> > > > each supported sample rate
according
> > > > to the clock rates of these
provided
> > > > clock sources.
> > >
> > > You mean tx<0-7>
> >
> > Yes. Thank you.
> >
> > > Also I would make this option required. Use a dummy clock for mux
> > > inputs that are grounded for a specific SoC.
> >
> > Some clocks are not from CCM and we haven't defined in imx6q-clk.txt,
> > so in most cases we can't provide a phandle for them, eg: spdif_ext.
> > I think it's a bit hard to force it to be 'required'. An 'optional'
> > looks more flexible to me and a default one is ensured even if it's
> > missing.
>
> <&clks 0> is the dummy clock. This can be used for all input clocks not
> defined by the SoC.
Where does this assumption come from? Is it documented anywhere? What
about cases when you have full description of clocks in device tree, with
one node per clock and #clock-cells = <0>?
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
From: Tomasz Figa @ 2013-08-17 12:56 UTC (permalink / raw)
To: Nicolin Chen
Cc: mark.rutland, devicetree, alsa-devel, lars, ian.campbell,
pawel.moll, swarren, festevam, s.hauer, timur, rob.herring,
broonie, p.zabel, galak, shawn.guo, linuxppc-dev
In-Reply-To: <20130816044330.GD1846@MrMyself>
Hi Nicolin,
On Friday 16 of August 2013 12:43:31 Nicolin Chen wrote:
> Hi Tomasz,
>
> Thank you for the comments.
You're welcome.
> I'll revise them in v6.
> And below is my reply for you comments.
Thanks for your reply.
> On Thu, Aug 15, 2013 at 02:18:22PM +0200, Tomasz Figa wrote:
> > > + - clock-names : Includes the following entries:
> > > + name type comments
> > > + "core" Required The core clock of spdif controller
> > > +
> > > + "rx" Optional Rx clock source for spdif record.
> > > + If absent, will use core clock.
> > > +
> > > + "tx" Optional Tx clock source for spdif
playback.
> > > + If absent, will use core clock.
> > > +
> > > + "tx-32000" Optional Tx clock source for 32000Hz sample
rate
> > > + playback. If absent, will use tx
clock.
> > > +
> > > + "tx-44100" Optional Tx clock source for 44100Hz sample
rate
> > > + playback. If absent, will use tx
clock.
> > > +
> > > + "tx-48000" Optional Tx clock source for 48000Hz sample
rate
> > > + playback. If absent, will use tx
clock.
> > > +
> > > + "src<0-7>" Optional Clock source list for tx and rx
clock
> > > + to look up their clock source
indexes.
> > > + This clock list should be
identical to
> > > + the list of TxClk_Source bit value
of
> > > + register SPDIF_STC. If absent or
failed
> > > + to look up, tx and rx clock would
then
> > > + ignore the "rx", "tx" "tx-32000",
> > > + "tx-44100", "tx-48000" clock
phandles
> > > + and select the core clock as
default
> > > + tx and rx clock.
> >
> > I suspect a little abuse of clocks property here. From the description
> > of "core" and "src<0-7>" clocks I assume that the IP can have up to 9
> > clock inputs - core clock and up to 8 extra source clocks. Is it
> > correct?
> >
> > If yes, this makes the "tx", "rx" and "tx-*" clocks describe
> > configuration, not hardware. IMHO it should be up to the driver which
> > source clocks to use for tx and rx channels and for each sampling
> > rate.
>
> First, you are right that all the properties you just commented are
> software configurations. And I got the point that device tree now
> can't allow any software configuration even if the actual hardware
> connection will depend on it.
>
> If so, I would like to remove those abused clocks and also drop the
> unused clocks in src<0-7>, then just remain those needed clocks src.
> I think that can be plausible because there'll be no more clock abuse
> and the driver will be able to get the source index from the name
> 'src<num>'.
OK.
> And you are right about the 9 clock inputs, just there're not only 9
> inputs but also an extra external clock from S/PDIF transmitter via
> coaxial cable or optical fiber -- RxCLK. Please check the following
> list:
>
> 0000 if (DPLL Locked) SPDIF_RxClk else extal
> 0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk
> 0010 if (DPLL Locked) SPDIF_RxClk else asrc_clk
> 0011 if (DPLL Locked) SPDIF_RxClk else spdif_extclk
> 0100 if (DPLL Locked) SPDIF_Rxclk else esai_hckt
> 0101 extal_clk
> 0110 spdif_clk
> 0111 asrc_clk
> 1000 spdif_extclk
> 1001 esai_hckt
> 1010 if (DPLL Locked) SPDIF_RxClk else mlb_clk
> 1011 if (DPLL Locked) SPDIF_RxClk else mlb_phy_clk
> 1100 mkb_clk
> 1101 mlb_phy_clk
Could you explain what the above values are? If they are values written to
a 4-bit mux that selects RX clock source, then all the 16 clocks should be
specified from device tree, even if they are duplicated.
Are the clock names you used above names of clock inputs of the S/PDIF
block or names of SoC-wide clocks?
Can the assignment of clock inputs change? For example on one SoC
0000 if (DPLL Locked) SPDIF_RxClk else extal_clk
0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk
...
0101 extal_clk
0110 spdif_clk
...
and on another
0000 if (DPLL Locked) SPDIF_RxClk else spdif_clk
0001 if (DPLL Locked) SPDIF_RxClk else extal_clk
...
0101 extal_clk
0110 spdif_clk
...
(notice the swapped 0000 and 0001 inputs)
Same goes for the TX mux. If it is a 3-bit, 8-input mux, then it should be
described in device tree separately, as it is different than the 4-bit,
16-input RX mux.
> When (DPLL Locked) condition matches, the rx clock can ignore the 8
> input clocks from clock mux then use the external one from a S/PDIF
> transmitter.
What happens if the "DPLL locked" condition doesn't match? When this can
happen?
> So for the below part:
> > > +Optional properties:
> > > +
> > > + - rx-clksrc-lock: This is a boolean property. If present,
> > > ClkSrc_Sel
> > > bit + of SPDIF_SRPC would be set a clock source that cares DPLL
> > > locked
> > > condition. +
> >
> > This again looks like software configuration, not hardware
> > description.
> > Could you elaborate a bit more on meaning of this property?
>
> I think the rx-clksrc-lock property should be included in DT as well,
> since it's exactly a available clock source for rx. But I guess I just
> need to figure out a better way or a more elaborated description.
I still don't understand the meaning of this property. Could you explain
it please?
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
From: Sascha Hauer @ 2013-08-17 14:53 UTC (permalink / raw)
To: Tomasz Figa
Cc: mark.rutland, devicetree, alsa-devel, lars, ian.campbell,
pawel.moll, swarren, festevam, Nicolin Chen, timur, rob.herring,
broonie, p.zabel, galak, shawn.guo, linuxppc-dev
In-Reply-To: <2805432.jMVhHxhr1m@flatron>
On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote:
> > > > Also I would make this option required. Use a dummy clock for mux
> > > > inputs that are grounded for a specific SoC.
> > >
> > > Some clocks are not from CCM and we haven't defined in imx6q-clk.txt,
> > > so in most cases we can't provide a phandle for them, eg: spdif_ext.
> > > I think it's a bit hard to force it to be 'required'. An 'optional'
> > > looks more flexible to me and a default one is ensured even if it's
> > > missing.
> >
> > <&clks 0> is the dummy clock. This can be used for all input clocks not
> > defined by the SoC.
>
> Where does this assumption come from? Is it documented anywhere?
This is how all i.MX clock bindings currently are. See
Documentation/devicetree/bindings/clock/imx*-clock.txt
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
From: Sascha Hauer @ 2013-08-17 15:00 UTC (permalink / raw)
To: Tomasz Figa
Cc: mark.rutland, devicetree, alsa-devel, lars, ian.campbell,
pawel.moll, swarren, festevam, Nicolin Chen, timur, rob.herring,
broonie, p.zabel, galak, shawn.guo, linuxppc-dev
In-Reply-To: <137675716.iLf1T1CVZo@flatron>
On Sat, Aug 17, 2013 at 02:26:40PM +0200, Tomasz Figa wrote:
> > You mean tx<0-7>.
> >
> > Also I would make this option required. Use a dummy clock for mux inputs
> > that are grounded for a specific SoC.
>
> Why do you need a dummy clock?
>
> The driver can simply try to grab all the possible clocks and discard
> those that failed, so you can just keep those grounded clocks unspecified.
We don't need dummy clocks. My motivation saying this that I was afraid
people try to configure the driver by skipping the clocks they don't
want from the devicetree.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
From: Tomasz Figa @ 2013-08-17 15:13 UTC (permalink / raw)
To: Sascha Hauer
Cc: mark.rutland, devicetree, alsa-devel, lars, ian.campbell,
pawel.moll, swarren, festevam, Nicolin Chen, timur, rob.herring,
broonie, p.zabel, galak, shawn.guo, linuxppc-dev
In-Reply-To: <20130817150002.GU26614@pengutronix.de>
On Saturday 17 of August 2013 17:00:02 Sascha Hauer wrote:
> On Sat, Aug 17, 2013 at 02:26:40PM +0200, Tomasz Figa wrote:
> > > You mean tx<0-7>.
> > >
> > > Also I would make this option required. Use a dummy clock for mux
> > > inputs that are grounded for a specific SoC.
> >
> > Why do you need a dummy clock?
> >
> > The driver can simply try to grab all the possible clocks and discard
> > those that failed, so you can just keep those grounded clocks
> > unspecified.
> We don't need dummy clocks. My motivation saying this that I was afraid
> people try to configure the driver by skipping the clocks they don't
> want from the devicetree.
I'm not really sure if the same abuse couldn't be easily achieved by
putting dummy clocks in place of those skipped clocks.
Adding a note in binding documentation that says that all clocks that are
fed to the IP shall be specified should be fine IMHO.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
From: Sascha Hauer @ 2013-08-17 15:14 UTC (permalink / raw)
To: Tomasz Figa
Cc: mark.rutland, devicetree, alsa-devel, lars, ian.campbell,
pawel.moll, swarren, festevam, Nicolin Chen, timur, rob.herring,
broonie, p.zabel, galak, shawn.guo, linuxppc-dev
In-Reply-To: <2362958.X2QCBUPAyI@flatron>
On Sat, Aug 17, 2013 at 02:56:11PM +0200, Tomasz Figa wrote:
> Hi Nicolin,
> > First, you are right that all the properties you just commented are
> > software configurations. And I got the point that device tree now
> > can't allow any software configuration even if the actual hardware
> > connection will depend on it.
> >
> > If so, I would like to remove those abused clocks and also drop the
> > unused clocks in src<0-7>, then just remain those needed clocks src.
> > I think that can be plausible because there'll be no more clock abuse
> > and the driver will be able to get the source index from the name
> > 'src<num>'.
>
> OK.
>
> > And you are right about the 9 clock inputs, just there're not only 9
> > inputs but also an extra external clock from S/PDIF transmitter via
> > coaxial cable or optical fiber -- RxCLK. Please check the following
> > list:
> >
> > 0000 if (DPLL Locked) SPDIF_RxClk else extal
> > 0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk
> > 0010 if (DPLL Locked) SPDIF_RxClk else asrc_clk
> > 0011 if (DPLL Locked) SPDIF_RxClk else spdif_extclk
> > 0100 if (DPLL Locked) SPDIF_Rxclk else esai_hckt
> > 0101 extal_clk
> > 0110 spdif_clk
> > 0111 asrc_clk
> > 1000 spdif_extclk
> > 1001 esai_hckt
> > 1010 if (DPLL Locked) SPDIF_RxClk else mlb_clk
> > 1011 if (DPLL Locked) SPDIF_RxClk else mlb_phy_clk
> > 1100 mkb_clk
> > 1101 mlb_phy_clk
>
> Could you explain what the above values are? If they are values written to
> a 4-bit mux that selects RX clock source, then all the 16 clocks should be
> specified from device tree, even if they are duplicated.
The S/PDIF core can recover the clock for the tx signal from the rx
signal. So if you have an S/PDIF input signal, then the DPLL will be
locked and the SPDIF_RxClk can be used for tx. So the above are really 8
clocks and one "If DPLL locked, use it" bit.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
From: Sascha Hauer @ 2013-08-17 15:14 UTC (permalink / raw)
To: Tomasz Figa
Cc: mark.rutland, devicetree, alsa-devel, lars, ian.campbell,
pawel.moll, swarren, festevam, Nicolin Chen, timur, rob.herring,
broonie, p.zabel, galak, shawn.guo, linuxppc-dev
In-Reply-To: <2782793.dUculLxO6S@flatron>
On Sat, Aug 17, 2013 at 05:13:20PM +0200, Tomasz Figa wrote:
> On Saturday 17 of August 2013 17:00:02 Sascha Hauer wrote:
> > On Sat, Aug 17, 2013 at 02:26:40PM +0200, Tomasz Figa wrote:
> > > > You mean tx<0-7>.
> > > >
> > > > Also I would make this option required. Use a dummy clock for mux
> > > > inputs that are grounded for a specific SoC.
> > >
> > > Why do you need a dummy clock?
> > >
> > > The driver can simply try to grab all the possible clocks and discard
> > > those that failed, so you can just keep those grounded clocks
> > > unspecified.
> > We don't need dummy clocks. My motivation saying this that I was afraid
> > people try to configure the driver by skipping the clocks they don't
> > want from the devicetree.
>
> I'm not really sure if the same abuse couldn't be easily achieved by
> putting dummy clocks in place of those skipped clocks.
That's right, yes.
>
> Adding a note in binding documentation that says that all clocks that are
> fed to the IP shall be specified should be fine IMHO.
ok.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
From: Tomasz Figa @ 2013-08-17 15:17 UTC (permalink / raw)
To: Sascha Hauer
Cc: mark.rutland, devicetree, alsa-devel, lars, mturquette,
ian.campbell, pawel.moll, swarren, festevam, Nicolin Chen, timur,
rob.herring, broonie, p.zabel, galak, shawn.guo, linuxppc-dev
In-Reply-To: <20130817145316.GT26614@pengutronix.de>
On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote:
> On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote:
> > > > > Also I would make this option required. Use a dummy clock for
> > > > > mux
> > > > > inputs that are grounded for a specific SoC.
> > > >
> > > > Some clocks are not from CCM and we haven't defined in
> > > > imx6q-clk.txt,
> > > > so in most cases we can't provide a phandle for them, eg:
> > > > spdif_ext.
> > > > I think it's a bit hard to force it to be 'required'. An
> > > > 'optional'
> > > > looks more flexible to me and a default one is ensured even if
> > > > it's
> > > > missing.
> > >
> > > <&clks 0> is the dummy clock. This can be used for all input clocks
> > > not
> > > defined by the SoC.
> >
> > Where does this assumption come from? Is it documented anywhere?
>
> This is how all i.MX clock bindings currently are. See
> Documentation/devicetree/bindings/clock/imx*-clock.txt
OK, thanks.
I guess we need some discussion on dummy clocks vs skipped clocks. I think
we want some consistency on this, don't we?
If we really need a dummy clock, then we might also want a generic way to
specify it.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
From: Tomasz Figa @ 2013-08-17 15:38 UTC (permalink / raw)
To: Sascha Hauer
Cc: mark.rutland, devicetree, alsa-devel, lars, ian.campbell,
pawel.moll, swarren, festevam, Nicolin Chen, timur, rob.herring,
broonie, p.zabel, galak, shawn.guo, linuxppc-dev
In-Reply-To: <20130817151409.GW26614@pengutronix.de>
On Saturday 17 of August 2013 17:14:09 Sascha Hauer wrote:
> On Sat, Aug 17, 2013 at 02:56:11PM +0200, Tomasz Figa wrote:
> > Hi Nicolin,
> >
> > > First, you are right that all the properties you just commented are
> > > software configurations. And I got the point that device tree now
> > > can't allow any software configuration even if the actual hardware
> > > connection will depend on it.
> > >
> > > If so, I would like to remove those abused clocks and also drop the
> > > unused clocks in src<0-7>, then just remain those needed clocks src.
> > > I think that can be plausible because there'll be no more clock
> > > abuse
> > > and the driver will be able to get the source index from the name
> > > 'src<num>'.
> >
> > OK.
> >
> > > And you are right about the 9 clock inputs, just there're not only 9
> > > inputs but also an extra external clock from S/PDIF transmitter via
> > > coaxial cable or optical fiber -- RxCLK. Please check the following
> > > list:
> > >
> > > 0000 if (DPLL Locked) SPDIF_RxClk else extal
> > > 0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk
> > > 0010 if (DPLL Locked) SPDIF_RxClk else asrc_clk
> > > 0011 if (DPLL Locked) SPDIF_RxClk else spdif_extclk
> > > 0100 if (DPLL Locked) SPDIF_Rxclk else esai_hckt
> > > 0101 extal_clk
> > > 0110 spdif_clk
> > > 0111 asrc_clk
> > > 1000 spdif_extclk
> > > 1001 esai_hckt
> > > 1010 if (DPLL Locked) SPDIF_RxClk else mlb_clk
> > > 1011 if (DPLL Locked) SPDIF_RxClk else mlb_phy_clk
> > > 1100 mkb_clk
> > > 1101 mlb_phy_clk
> >
> > Could you explain what the above values are? If they are values
> > written to a 4-bit mux that selects RX clock source, then all the 16
> > clocks should be specified from device tree, even if they are
> > duplicated.
>
> The S/PDIF core can recover the clock for the tx signal from the rx
> signal. So if you have an S/PDIF input signal, then the DPLL will be
> locked and the SPDIF_RxClk can be used for tx. So the above are really 8
> clocks and one "If DPLL locked, use it" bit.
Yes, I'm aware of this and the solution you proposed is fully acceptable,
but it complicates the driver a bit.
If you look at the mux inputs above, you can see that there is no single
bit that specifies "if DPLL locked, use it" mode. Instead the conditional
clock sources are mixed with fixed clock sources, without a linear
translation to 0-7 range, which would have to be hardcoded in the driver.
I guess it's a matter of preference, but if the IP has a RX clock
selection mux that has 16 inputs, the natural representation in device
tree that comes to my mind is 16 clocks - one for each input of the mux.
One might choose to abstract this as 8 clocks, though, since each clock is
duplicated.
In this particular case, the driver would have to know which mux inputs
can use DPLL anyway, so some hardcoding in the driver is still going to
take place, so I guess both solutions are equally right.
Best regards,
Tomasz
^ permalink raw reply
* Re: MPC8315 reboot failure, lockdep splat possibly related?
From: Anthony Foiani @ 2013-08-17 17:17 UTC (permalink / raw)
To: Bhushan Bharat-R65777; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D07114F7C@039-SN2MPN1-013.039d.mgd.msft.net>
Bharat, greetings --
Bhushan Bharat-R65777 <R65777@freescale.com> writes:
> You should get rid of this by changing spin_lock/unlock() in
> fsl_sata_set_irq_coalescing() to spin_lock_irqsave/restore()
I'll do that -- thanks!
Looks like linux-next still has plain spin_lock, not the irqsave variant.
Is this something that needs to be pushed to mainline and/or stable?
If it should go upstream, is this something that Freescale can do, or
should I try to put together a patch? (I'd like to avoid having to do
it myself, because I'm carrying out-of-tree patches that I'd have to
port to linux-next before it'd be accepted there, I fear.)
Either way, I'll definitely report if it fixes the splat on my 3.9.7
system.
Thanks again!
Best regards,
Anthony Foiani
^ permalink raw reply
* Re: [RFC PATCH v2 11/11] pstore/ram: Read and write to the 'compressed' flag of pstore
From: Kees Cook @ 2013-08-17 18:26 UTC (permalink / raw)
To: Aruna Balakrishnaiah
Cc: jkenisto, Tony Luck, Colin Cross, LKML, mahesh, linuxppc-dev,
Anton Vorontsov
In-Reply-To: <20130816131907.3338.76154.stgit@aruna-ThinkPad-T420>
On Fri, Aug 16, 2013 at 6:19 AM, Aruna Balakrishnaiah
<aruna@linux.vnet.ibm.com> wrote:
> In pstore write, add character 'C'(compressed) or 'D'(decompressed)
> in the header while writing to Ram persistent buffer. In pstore read,
> read the header and update the 'compressed' flag accordingly.
>
> Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
Nice work!
Acked-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [RFC PATCH v2 00/11] Add (de)compression support to pstore
From: Kees Cook @ 2013-08-17 18:32 UTC (permalink / raw)
To: Luck, Tony
Cc: jkenisto@linux.vnet.ibm.com, ccross@android.com,
linux-kernel@vger.kernel.org, mahesh@linux.vnet.ibm.com,
linuxppc-dev@ozlabs.org, Aruna Balakrishnaiah,
cbouatmailru@gmail.com
In-Reply-To: <3908561D78D1C84285E8C5FCA982C28F31CBF152@ORSMSX106.amr.corp.intel.com>
On Fri, Aug 16, 2013 at 3:15 PM, Luck, Tony <tony.luck@intel.com> wrote:
>> Needs testing with erst backend, efivars and persistent ram.
>
> Tested against ERST - works fine for me now.
>
> Need to stare at the code to see if there are any more bits that could be cleaned up.
>
> Thanks for addressing my issues from v1
Yeah, this is great. While I haven't tested it myself yet, the code
seems to be in good shape. I acked the ram piece separately, but
consider the entire series:
Reviewed-by: Kees Cook <keescook@chromium.org>
Thanks!
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* [PATCH 1/4] hotplug, x86: Fix online state in cpu0 debug interface
From: Toshi Kani @ 2013-08-17 19:46 UTC (permalink / raw)
To: mingo, hpa, tglx, gregkh, benh
Cc: fenghua.yu, Toshi Kani, x86, linux-kernel, rjw, isimatu.yasuaki,
srivatsa.bhat, bp, linuxppc-dev
In-Reply-To: <1376768819-28975-1-git-send-email-toshi.kani@hp.com>
_debug_hotplug_cpu() is a debug interface that puts cpu0 offline during
boot-up when CONFIG_DEBUG_HOTPLUG_CPU0 is set. After cpu0 is put offline
in this interface, however, /sys/devices/system/cpu/cpu0/online still
shows 1 (online).
This patch fixes _debug_hotplug_cpu() to update dev->offline when CPU
online/offline operation succeeded.
Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
arch/x86/kernel/topology.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/topology.c b/arch/x86/kernel/topology.c
index 6e60b5f..5823bbd 100644
--- a/arch/x86/kernel/topology.c
+++ b/arch/x86/kernel/topology.c
@@ -72,16 +72,19 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
ret = cpu_down(cpu);
if (!ret) {
pr_info("CPU %u is now offline\n", cpu);
+ dev->offline = true;
kobject_uevent(&dev->kobj, KOBJ_OFFLINE);
} else
pr_debug("Can't offline CPU%d.\n", cpu);
break;
case 1:
ret = cpu_up(cpu);
- if (!ret)
+ if (!ret) {
+ dev->offline = false;
kobject_uevent(&dev->kobj, KOBJ_ONLINE);
- else
+ } else {
pr_debug("Can't online CPU%d.\n", cpu);
+ }
break;
default:
ret = -EINVAL;
^ permalink raw reply related
* [PATCH 4/4] hotplug, powerpc, x86: Remove cpu_hotplug_driver_lock()
From: Toshi Kani @ 2013-08-17 19:46 UTC (permalink / raw)
To: mingo, hpa, tglx, gregkh, benh
Cc: fenghua.yu, Toshi Kani, x86, linux-kernel, rjw, isimatu.yasuaki,
srivatsa.bhat, bp, linuxppc-dev
In-Reply-To: <1376768819-28975-1-git-send-email-toshi.kani@hp.com>
cpu_hotplug_driver_lock() serializes CPU online/offline operations
when ARCH_CPU_PROBE_RELEASE is set. This lock interface is no longer
necessary with the following reason:
- lock_device_hotplug() now protects CPU online/offline operations,
including the probe & release interfaces enabled by
ARCH_CPU_PROBE_RELEASE. The use of cpu_hotplug_driver_lock() is
redundant.
- cpu_hotplug_driver_lock() is only valid when ARCH_CPU_PROBE_RELEASE
is defined, which is misleading and is only enabled on powerpc.
This patch removes the cpu_hotplug_driver_lock() interface. As
a result, ARCH_CPU_PROBE_RELEASE only enables / disables the cpu
probe & release interface as intended. There is no functional change
in this patch.
Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
Performed build test only on powerpc.
---
arch/powerpc/kernel/smp.c | 12 ----------
arch/powerpc/platforms/pseries/dlpar.c | 40 ++++++++++++--------------------
arch/x86/kernel/topology.c | 2 --
drivers/base/cpu.c | 10 +-------
include/linux/cpu.h | 13 ----------
5 files changed, 16 insertions(+), 61 deletions(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 38b0ba6..1667269 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -763,18 +763,6 @@ void __cpu_die(unsigned int cpu)
smp_ops->cpu_die(cpu);
}
-static DEFINE_MUTEX(powerpc_cpu_hotplug_driver_mutex);
-
-void cpu_hotplug_driver_lock()
-{
- mutex_lock(&powerpc_cpu_hotplug_driver_mutex);
-}
-
-void cpu_hotplug_driver_unlock()
-{
- mutex_unlock(&powerpc_cpu_hotplug_driver_mutex);
-}
-
void cpu_die(void)
{
if (ppc_md.cpu_die)
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index a1a7b9a..e39325d 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -387,18 +387,13 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
char *cpu_name;
int rc;
- cpu_hotplug_driver_lock();
rc = strict_strtoul(buf, 0, &drc_index);
- if (rc) {
- rc = -EINVAL;
- goto out;
- }
+ if (rc)
+ return -EINVAL;
dn = dlpar_configure_connector(drc_index);
- if (!dn) {
- rc = -EINVAL;
- goto out;
- }
+ if (!dn)
+ return -EINVAL;
/* configure-connector reports cpus as living in the base
* directory of the device tree. CPUs actually live in the
@@ -407,8 +402,7 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
cpu_name = kasprintf(GFP_KERNEL, "/cpus%s", dn->full_name);
if (!cpu_name) {
dlpar_free_cc_nodes(dn);
- rc = -ENOMEM;
- goto out;
+ return -ENOMEM;
}
kfree(dn->full_name);
@@ -417,22 +411,21 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
rc = dlpar_acquire_drc(drc_index);
if (rc) {
dlpar_free_cc_nodes(dn);
- rc = -EINVAL;
- goto out;
+ return -EINVAL;
}
rc = dlpar_attach_node(dn);
if (rc) {
dlpar_release_drc(drc_index);
dlpar_free_cc_nodes(dn);
- goto out;
+ return rc;
}
rc = dlpar_online_cpu(dn);
-out:
- cpu_hotplug_driver_unlock();
+ if (rc)
+ return rc;
- return rc ? rc : count;
+ return count;
}
static int dlpar_offline_cpu(struct device_node *dn)
@@ -505,30 +498,27 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)
return -EINVAL;
}
- cpu_hotplug_driver_lock();
rc = dlpar_offline_cpu(dn);
if (rc) {
of_node_put(dn);
- rc = -EINVAL;
- goto out;
+ return -EINVAL;
}
rc = dlpar_release_drc(*drc_index);
if (rc) {
of_node_put(dn);
- goto out;
+ return rc;
}
rc = dlpar_detach_node(dn);
if (rc) {
dlpar_acquire_drc(*drc_index);
- goto out;
+ return rc;
}
of_node_put(dn);
-out:
- cpu_hotplug_driver_unlock();
- return rc ? rc : count;
+
+ return count;
}
static int __init pseries_dlpar_init(void)
diff --git a/arch/x86/kernel/topology.c b/arch/x86/kernel/topology.c
index a3f35eb..649b010 100644
--- a/arch/x86/kernel/topology.c
+++ b/arch/x86/kernel/topology.c
@@ -66,7 +66,6 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
return -EINVAL;
lock_device_hotplug();
- cpu_hotplug_driver_lock();
switch (action) {
case 0:
@@ -91,7 +90,6 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
ret = -EINVAL;
}
- cpu_hotplug_driver_unlock();
unlock_device_hotplug();
return ret;
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 4cc6928..b4ebd7b 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -45,8 +45,6 @@ static int __ref cpu_subsys_online(struct device *dev)
int from_nid, to_nid;
int ret;
- cpu_hotplug_driver_lock();
-
from_nid = cpu_to_node(cpuid);
ret = cpu_up(cpuid);
/*
@@ -57,18 +55,12 @@ static int __ref cpu_subsys_online(struct device *dev)
if (from_nid != to_nid)
change_cpu_under_node(cpu, from_nid, to_nid);
- cpu_hotplug_driver_unlock();
return ret;
}
static int cpu_subsys_offline(struct device *dev)
{
- int ret;
-
- cpu_hotplug_driver_lock();
- ret = cpu_down(dev->id);
- cpu_hotplug_driver_unlock();
- return ret;
+ return cpu_down(dev->id);
}
void unregister_cpu(struct cpu *cpu)
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index ab0eade..e847ef8 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -182,19 +182,6 @@ extern void cpu_hotplug_enable(void);
void clear_tasks_mm_cpumask(int cpu);
int cpu_down(unsigned int cpu);
-#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
-extern void cpu_hotplug_driver_lock(void);
-extern void cpu_hotplug_driver_unlock(void);
-#else
-static inline void cpu_hotplug_driver_lock(void)
-{
-}
-
-static inline void cpu_hotplug_driver_unlock(void)
-{
-}
-#endif
-
#else /* CONFIG_HOTPLUG_CPU */
#define get_online_cpus() do { } while (0)
^ permalink raw reply related
* [PATCH 2/4] hotplug, x86: Add hotplug lock to missing places
From: Toshi Kani @ 2013-08-17 19:46 UTC (permalink / raw)
To: mingo, hpa, tglx, gregkh, benh
Cc: fenghua.yu, Toshi Kani, x86, linux-kernel, rjw, isimatu.yasuaki,
srivatsa.bhat, bp, linuxppc-dev
In-Reply-To: <1376768819-28975-1-git-send-email-toshi.kani@hp.com>
lock_device_hotplug() serializes CPU & Memory online/offline and hotplug
operations. However, this lock is not held in the debug interfaces below
that initiate CPU online/offline operations.
- _debug_hotplug_cpu(), cpu0 hotplug test interface enabled by
CONFIG_DEBUG_HOTPLUG_CPU0.
- cpu_probe_store() and cpu_release_store(), cpu hotplug test interface
enabled by CONFIG_ARCH_CPU_PROBE_RELEASE.
This patch changes the above interfaces to hold lock_device_hotplug().
Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
arch/x86/kernel/topology.c | 2 ++
drivers/base/cpu.c | 16 ++++++++++++++--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/topology.c b/arch/x86/kernel/topology.c
index 5823bbd..a3f35eb 100644
--- a/arch/x86/kernel/topology.c
+++ b/arch/x86/kernel/topology.c
@@ -65,6 +65,7 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
if (!cpu_is_hotpluggable(cpu))
return -EINVAL;
+ lock_device_hotplug();
cpu_hotplug_driver_lock();
switch (action) {
@@ -91,6 +92,7 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
}
cpu_hotplug_driver_unlock();
+ unlock_device_hotplug();
return ret;
}
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 4c358bc..4cc6928 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -88,7 +88,13 @@ static ssize_t cpu_probe_store(struct device *dev,
const char *buf,
size_t count)
{
- return arch_cpu_probe(buf, count);
+ ssize_t ret;
+
+ lock_device_hotplug();
+ ret = arch_cpu_probe(buf, count);
+ unlock_device_hotplug();
+
+ return ret;
}
static ssize_t cpu_release_store(struct device *dev,
@@ -96,7 +102,13 @@ static ssize_t cpu_release_store(struct device *dev,
const char *buf,
size_t count)
{
- return arch_cpu_release(buf, count);
+ ssize_t ret;
+
+ lock_device_hotplug();
+ ret = arch_cpu_release(buf, count);
+ unlock_device_hotplug();
+
+ return ret;
}
static DEVICE_ATTR(probe, S_IWUSR, NULL, cpu_probe_store);
^ permalink raw reply related
* [PATCH 3/4] hotplug, x86: Disable ARCH_CPU_PROBE_RELEASE on x86
From: Toshi Kani @ 2013-08-17 19:46 UTC (permalink / raw)
To: mingo, hpa, tglx, gregkh, benh
Cc: fenghua.yu, Toshi Kani, x86, linux-kernel, rjw, isimatu.yasuaki,
srivatsa.bhat, bp, linuxppc-dev
In-Reply-To: <1376768819-28975-1-git-send-email-toshi.kani@hp.com>
Commit d7c53c9e enabled ARCH_CPU_PROBE_RELEASE on x86 in order to
serialize CPU online/offline operations. Although it is the config
option to enable CPU hotplug test interfaces, probe & release, it is
also the option to enable cpu_hotplug_driver_lock() as well. Therefore,
this option had to be enabled on x86 with dummy arch_cpu_probe() and
arch_cpu_release().
Since then, lock_device_hotplug() was introduced to serialize CPU
online/offline & hotplug operations. Therefore, this config option
is no longer required for the serialization. This patch disables
this config option on x86 and revert the changes made by commit
d7c53c9e.
Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
arch/x86/Kconfig | 4 ----
arch/x86/kernel/smpboot.c | 21 ---------------------
2 files changed, 25 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b32ebf9..c87e49a 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -255,10 +255,6 @@ config ARCH_HWEIGHT_CFLAGS
default "-fcall-saved-ecx -fcall-saved-edx" if X86_32
default "-fcall-saved-rdi -fcall-saved-rsi -fcall-saved-rdx -fcall-saved-rcx -fcall-saved-r8 -fcall-saved-r9 -fcall-saved-r10 -fcall-saved-r11" if X86_64
-config ARCH_CPU_PROBE_RELEASE
- def_bool y
- depends on HOTPLUG_CPU
-
config ARCH_SUPPORTS_UPROBES
def_bool y
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index aecc98a..5b24a9d 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -82,27 +82,6 @@
/* State of each CPU */
DEFINE_PER_CPU(int, cpu_state) = { 0 };
-#ifdef CONFIG_HOTPLUG_CPU
-/*
- * We need this for trampoline_base protection from concurrent accesses when
- * off- and onlining cores wildly.
- */
-static DEFINE_MUTEX(x86_cpu_hotplug_driver_mutex);
-
-void cpu_hotplug_driver_lock(void)
-{
- mutex_lock(&x86_cpu_hotplug_driver_mutex);
-}
-
-void cpu_hotplug_driver_unlock(void)
-{
- mutex_unlock(&x86_cpu_hotplug_driver_mutex);
-}
-
-ssize_t arch_cpu_probe(const char *buf, size_t count) { return -1; }
-ssize_t arch_cpu_release(const char *buf, size_t count) { return -1; }
-#endif
-
/* Number of siblings per CPU package */
int smp_num_siblings = 1;
EXPORT_SYMBOL(smp_num_siblings);
^ permalink raw reply related
* [PATCH 0/4] Unify CPU hotplug lock interface
From: Toshi Kani @ 2013-08-17 19:46 UTC (permalink / raw)
To: mingo, hpa, tglx, gregkh, benh
Cc: fenghua.yu, Toshi Kani, x86, linux-kernel, rjw, isimatu.yasuaki,
srivatsa.bhat, bp, linuxppc-dev
lock_device_hotplug() was recently introduced to serialize CPU & Memory
online/offline and hotplug operations, along with sysfs online interface
restructure (commit 4f3549d7). With this new locking scheme,
cpu_hotplug_driver_lock() is redundant and is no longer necessary.
This patchset makes sure that lock_device_hotplug() covers all CPU online/
offline interfaces, and then removes cpu_hotplug_driver_lock().
The patchset is based on Linus's tree, 3.11.0-rc5.
---
Toshi Kani (4):
hotplug, x86: Fix online state in cpu0 debug interface
hotplug, x86: Add hotplug lock to missing places
hotplug, x86: Disable ARCH_CPU_PROBE_RELEASE on x86
hotplug, powerpc, x86: Remove cpu_hotplug_driver_lock()
---
arch/powerpc/kernel/smp.c | 12 ----------
arch/powerpc/platforms/pseries/dlpar.c | 40 +++++++++++++---------------------
arch/x86/Kconfig | 4 ----
arch/x86/kernel/smpboot.c | 21 ------------------
arch/x86/kernel/topology.c | 11 ++++++----
drivers/base/cpu.c | 26 ++++++++++++----------
include/linux/cpu.h | 13 -----------
7 files changed, 37 insertions(+), 90 deletions(-)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox