On 19/12/2013 13:30, Gerhard Sittig wrote:
On Thu, Dec 19, 2013 at 11:23 +0100, Matteo Facchinetti wrote:
USB controller pin-muxing is not initialized correctly and when system boot,
causes a kernel panic.
USB controller is also connected with a USB3320 ulpi tranciever and
DTS should be includes the correct dependency for initialize and activate
this component.
Signed-off-by: Matteo Facchinetti <matteo.facchine...@sirius-es.it>
---
arch/powerpc/boot/dts/mpc5125twr.dts | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/boot/dts/mpc5125twr.dts
b/arch/powerpc/boot/dts/mpc5125twr.dts
index 806479f..85452a7 100644
--- a/arch/powerpc/boot/dts/mpc5125twr.dts
+++ b/arch/powerpc/boot/dts/mpc5125twr.dts
@@ -230,6 +230,9 @@
};
usb@3000 {
+ /* TODO correct pinmux config and fix USB3320 ulpi
dependency */
+ status = "disabled";
+
compatible = "fsl,mpc5121-usb2-dr";
reg = <0x3000 0x400>;
#address-cells = <1>;
--
1.8.3.2
I agree on the change to the board dts file, but suggest to
reword the commit description for improved reception.
I feel it's worth trying to phrase the subject line, the commit
message, and the patch such that they can get considered
independently from each other, as not all of them are necessarily
available at the same time. Often they get looked up from
different perspectives, like terse listing first for orientation,
log with description then to determine whether to have a closer
look, the patch only at the end after the other checks told you
to look into more details. Assuming that they always show up in
combination may turn out to be inaccurate.
So I suggest some text along those lines:
at the moment the USB controller's pin muxing is not setup
correctly and causes a kernel panic upon system startup, so
disable the USB1 device tree node in the MPC5125 tower board
dts file
the USB controller is connected to an USB3320 ULPI transceiver
and the device tree should receive an update to reflect correct
dependencies and required initialization data before the USB1
node can get re-enabled
Does that sound correct to you? Does it reflect your intention,
or did I put something in wrong terms?
Yes, it's exactly what I tried to explain. All is right.
Now, I learned. Thank you.
A minor nit would be that other reviewers in the past suggested
to put the 'status = "disabled"' line last in the list of
properties (right before optional children). I don't have strong
feelings about this. Putting it first might better reflect your
motivation of only re-enabling the node after fixing the lack or
inappropriateness of existing information first.
I put it as first property because is very temporary and because is the
most important information for the moment:
"the USB1 port doesn't works for these reasons...".
But, I think that if this property is usually at the and of the node
list, it's better follow this suggestion.
Then I'll send a v2 patch with the newer description and with this change.
A different matter is that I'd suggest to re-work the MPC5125
device tree. It recently escaped my attention because it did not
share any information with the MPC5121 trees. Comparing the
MPC5125 board DTS with the MPC5121 DTS include file resulted in a
lot of unnecessary "differences" that turned out to be whitespace
or comment style only, or differences in the order of nodes.
There were only few real differences in the information, and the
MPC5125 device tree appears to only describe a subset of what the
SoC actually contains.
It may be worth looking into
- identifying common parts that are shared among the MPC5121 and
MPC5125 (my recent CCF update lists differences, but does not
explicitly list similarities, and is from the clocks
perspective and may not cover all of the SoC components)
- putting those common parts into .dtsi files if possible
- making the MPC5125 tower board reference the DTS includes,
sharing as much as possible with the other SoC variants
This may involve another split of the mpc5121.dtsi into what's
common to all MPC512x variants, and what's exclusive to MPC5121
only.
But that is a bigger task than the above quick adjustment, and is
not a required fix but just an improvement in maintainability or
completeness of information. So I suggest to pick your USB1
disabling for -next and 3.14 now, and to address the DTS cleanup
and sharing later.
It was from the first commit of mpc5125twr.dts file that I would have
liked to rework all mpc5xxx DTS tree.
At the moment I have already started this task but it's better to open
another thread for this discussion and I agree with you that it's not a
priority.
Now, I'm working to the NFC controller and I'm porting to linux 3.13 the
driver that we are using in linux 3.9.4.
When I done a preliminary version I'll post for starting a discussion
about its correct integration.
I think it's time to add this driver in mainline for use NFC as well.
Best regards,
Matteo Facchinetti
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
