Re: usb: dwc2: regression on MyBook Live Duo / Canyonlands since 4.3.0-rc4

2016-05-18 Thread Christian Lamparter via Linuxppc-dev 
On Tuesday, May 17, 2016 04:50:48 PM John Youn wrote:
> On 5/14/2016 6:11 AM, Christian Lamparter wrote:
> > On Thursday, May 12, 2016 11:40:28 AM John Youn wrote:
> >> On 5/12/2016 6:30 AM, Christian Lamparter wrote:
> >>> On Thursday, May 12, 2016 01:55:44 PM Arnd Bergmann wrote:
>  On Thursday 12 May 2016 11:58:18 Christian Lamparter wrote:
>  Detecting the endianess of the
>  device is probably the best future-proof solution, but it's also
>  considerably more work to do in the driver, and comes with a
>  tiny runtime overhead.
> >>>
> >>> The runtime overhead is probably non-measurable compared with the cost
> >>> of the actual MMIOs.
> >>
> >> Right. The code size increase is probably measurable (but still small),
> >> the runtime overhead is not.
> >
> > Ok, so no rebuts or complains have been posted.
> >
> > I've tested the patch you made in: https://lkml.org/lkml/2016/5/9/354
> > and it works: 
> >
> > Tested-by: Christian Lamparter 
> >
> > So, how do we go from here? There is are two small issues with the
> > original patch (#ifdef DWC2_LOG_WRITES got converted to lower case:
> > #ifdef dwc2_log_writes) and I guess a proper subject would be nice.  
> >
> > Arnd, can you please respin and post it (cc'd stable as well)?
> > So this is can be picked up? Or what's your plan?
> 
>  (I just realized my reply was stuck in my outbox, so the patch
>  went out first)
> 
>  If I recall correctly, the rough consensus was to go with your longer
>  patch in the future (fixed up for the comments that BenH and
>  I sent), and I'd suggest basing it on top of a fixed version of
>  my patch.
> >>> Well, but it comes with the "overhead"! So this was just as I said:
> >>> "Let's look at it and see if it's any good"... And I think it isn't
> >>> since the usb/host/ehci people also opted for #ifdef CONFIG_BIG_ENDIAN
> >>> archs etc...
> >>
> >> I slightly prefer the more general patch for future kernel versions.
> >> The overhead will probably be negligible, but we can perform some
> >> testing to make sure.
> >>
> >> Can you resubmit with all gathered feedback?
> > 
> > Yes, here are the changes.
> > 
> > I've tested it on my MyBook Live Duo. The usbotg comes right up:
> > [12610.540004] dwc2 4bff8.usbotg: USB bus 1 deregistered
> > [12612.513934] dwc2 4bff8.usbotg: Specified GNPTXFDEP=1024 > 256
> > [12612.518756] dwc2 4bff8.usbotg: EPs: 3, shared fifos, 2042 entries in 
> > SPRAM
> > [12612.530112] dwc2 4bff8.usbotg: DWC OTG Controller
> > [12612.533948] dwc2 4bff8.usbotg: new USB bus registered, assigned bus 
> > number 1
> > [12612.540083] dwc2 4bff8.usbotg: irq 33, io mem 0x
> > 
> > John: Can you run some perf test with it?
> > 
> > I've based this on:
> > 
> > commit 6ea2fffc9057a67df1994d85a7c085d899eaa25a
> > Author: Arnd Bergmann 
> > Date:   Fri May 13 15:52:27 2016 +0200
> > 
> > usb: dwc2: fix regression on big-endian PowerPC/ARM systems
> > 
> > so naturally, it needs to be applied first.
> > Most of the conversion work was done by the attached
> > coccinelle semantic patches. 
> > 
> > I had to edit the __bic32 and __orr32 helpers by hand.
> > As well as some debugfs code and stuff in gadget.c.
> > 
> 
> Thanks Christian.
> 
> I'll keep this in our internal tree and send it to Felipe later. This
> causes a bunch of conflicts that I have to fix up and I should do a
> bit of testing as well.
> 
> And since there is a patch that fixes the regression this is can wait.
> 
> Regards,
> John
---
Hey, that's really nice of you to do that :-D. Please keep me in the
loop (Cc) for those then. 

Yes, this needs definitely testing on all the affected ARCHs. 
I've attached a diff to a updated version of the patch. It
drops the special MIPS case (as requested by Arnd).

BTW, I looked into the ioread32_rep and iowrite32_rep again. I'm
not entirely convinced that the hardware FIFOs are actually endian
neutral. But I can't verify it since my Western Digital My Book Live
only supports the host configuration (forces host mode), so I don't
know what a device in dual-mode or peripheral do here.

The reason why I think it was broken is because there's a PIO copy
to and from the HCFIFO(x) in dwc2_hc_write_packet and
dwc2_hc_read_packet access in the hcd.c file as well... And there,
the code was using the dwc2_readl and dwc2_writel to access the data.
I added special accessors for the FIFOS now:
dwc2_readl_rep and dwc2_writel_rep.

I went all the way and implemented the helpers to do unaligned access
if necessary (not sure if adding likely branches is a good idea, as
this could be either always true or false for a specific driver the
whole time).

NB: it also fixes a "regs variable not used in dwc2_hsotg_dump" warning
if DEBUG isn't selected.

NB2: If it you need a patch against a specific tree,

Re: [PATCH v4] usb: dwc2: fix regression on big-endian PowerPC/ARM systems

2016-05-14 Thread Christian Lamparter via Linuxppc-dev 
On Friday, May 13, 2016 03:52:27 PM Arnd Bergmann wrote:
> A patch that went into Linux-4.4 to fix big-endian mode on a Lantiq
> MIPS system unfortunately broke big-endian operation on PowerPC
> APM82181 as reported by Christian Lamparter, and likely other
> systems.
> 
> It actually introduced multiple issues:
> 
> - it broke big-endian ARM kernels: any machine that was working
>   correctly with a little-endian kernel is no longer using byteswaps
>   on big-endian kernels, which clearly breaks them.
> - On PowerPC the same thing must be true: if it was working before,
>   using big-endian kernels is now broken. Unlike ARM, 32-bit PowerPC
>   usually uses big-endian kernels, so they are likely all broken.
> - The barrier for dwc2_writel is on the wrong side of the __raw_writel(),
>   so the MMIO no longer synchronizes with DMA operations.
> - On architectures that require specific CPU instructions for MMIO
>   access, using the __raw_ variant may turn this into a pointer
>   dereference that does not have the same effect as the readl/writel.
> 
> This patch is a simple revert for all architectures other than MIPS,
> in the hope that we can more easily backport it to fix the regression
> on PowerPC and ARM systems without breaking the Lantiq system again.
> 
> We should follow this up with a more elaborate change to add runtime
> detection of endianness, to make sure it also works on all other
> combinations of architectures and implementations of the usb-dwc2
> device. That patch however will be fairly large and not appropriate
> for backports to stable kernels.
> 
> Felipe suggested a different approach, using an endianness switching
> register to always put the device into LE mode, but unfortunately
> the dwc2 hardware does not provide a generic way to do that. Also,
> I see no practical way of addressing the problem more generally by
> patching architecture specific code on MIPS.
> 
> Signed-off-by: Arnd Bergmann 
> Fixes: 95c8bc360944 ("usb: dwc2: Use platform endianness when accessing 
> registers")
Thanks Arnd,

Tested-by: Christian Lamparter 

dwc2 4bff8.usbotg: Specified GNPTXFDEP=1024 > 256
dwc2 4bff8.usbotg: EPs: 3, shared fifos, 2042 entries in SPRAM
dwc2 4bff8.usbotg: DWC OTG Controller
dwc2 4bff8.usbotg: new USB bus registered, assigned bus number 1
dwc2 4bff8.usbotg: irq 33, io mem 0x
hub 1-0:1.0: USB hub found
hub 1-0:1.0: 1 port detected

Regards,
Christian
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: usb: dwc2: regression on MyBook Live Duo / Canyonlands since 4.3.0-rc4

2016-05-12 Thread Christian Lamparter via Linuxppc-dev 
On Thursday, May 12, 2016 11:40:28 AM John Youn wrote:
> On 5/12/2016 6:30 AM, Christian Lamparter wrote:
> > On Thursday, May 12, 2016 01:55:44 PM Arnd Bergmann wrote:
> >> On Thursday 12 May 2016 11:58:18 Christian Lamparter wrote:
> >> Detecting the endianess of the
> >> device is probably the best future-proof solution, but it's also
> >> considerably more work to do in the driver, and comes with a
> >> tiny runtime overhead.
> >
> > The runtime overhead is probably non-measurable compared with the cost
> > of the actual MMIOs.
> 
>  Right. The code size increase is probably measurable (but still small),
>  the runtime overhead is not.
> >>>
> >>> Ok, so no rebuts or complains have been posted.
> >>>
> >>> I've tested the patch you made in: https://lkml.org/lkml/2016/5/9/354
> >>> and it works: 
> >>>
> >>> Tested-by: Christian Lamparter 
> >>>
> >>> So, how do we go from here? There is are two small issues with the
> >>> original patch (#ifdef DWC2_LOG_WRITES got converted to lower case:
> >>> #ifdef dwc2_log_writes) and I guess a proper subject would be nice.  
> >>>
> >>> Arnd, can you please respin and post it (cc'd stable as well)?
> >>> So this is can be picked up? Or what's your plan?
> >>
> >> (I just realized my reply was stuck in my outbox, so the patch
> >> went out first)
> >>
> >> If I recall correctly, the rough consensus was to go with your longer
> >> patch in the future (fixed up for the comments that BenH and
> >> I sent), and I'd suggest basing it on top of a fixed version of
> >> my patch.
> > Well, but it comes with the "overhead"! So this was just as I said:
> > "Let's look at it and see if it's any good"... And I think it isn't
> > since the usb/host/ehci people also opted for #ifdef CONFIG_BIG_ENDIAN
> > archs etc...
> 
> I slightly prefer the more general patch for future kernel versions.
> The overhead will probably be negligible, but we can perform some
> testing to make sure.
> 
> Can you resubmit with all gathered feedback?
Yes I think I can do that. But I would really like to get the
regression out of the way. So for that: I back Arnd's patch.
It explains the problem much better and doesn't kill MIPS 
like the revert I was doing in my initial post to the MLs.
Also, another bonus: his patch is suited to port to stable.

The auto-detection approach is not that easy to get right,
given all the stuff that's going on with BE8, LE4, ... So
can we have your "blessing" for Arnd's patch for now? since
that way, I can base my patch on top of his work about the
issues of endiannes? (Just say: ACK :) )

Arnd: do you have a version with the #ifdef lower/uppercase
fix? Or should I give it a try (and fail in a different way ;) )
 
> >> Felipe just had another idea, to change the endianess of the dwc2
> >> block by setting a registers (if that exists). That would indeed
> >> be preferable, then we can just revert the broken change that
> >> went into 4.4 and backport that fix instead.
> > Just a quick reply. I have the docs for the thing. There's something
> > like that in GAHBCFG at Bit 24... BUT it only switches the endiannes
> > for the DMA descriptors (which is not always used, there are devices
> > with PIO only)! It doesn't deal with the MMIO access at all. 
> 
> That's correct. It only affects descriptor endianness for DMA
> descriptor mode of operation.

Ok. The funny thing is that for the MyBook Live Duo this setting might
be important since the PLB_DMA engine is not part of the DWC library...
Instead it's from IBM and operates in: Big Endian :-D.

Regards,
Christian
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: usb: dwc2: regression on MyBook Live Duo / Canyonlands since 4.3.0-rc4

2016-05-12 Thread Christian Lamparter via Linuxppc-dev 
On Thursday, May 12, 2016 01:55:44 PM Arnd Bergmann wrote:
> On Thursday 12 May 2016 11:58:18 Christian Lamparter wrote:
> > > > > Detecting the endianess of the
> > > > > device is probably the best future-proof solution, but it's also
> > > > > considerably more work to do in the driver, and comes with a
> > > > > tiny runtime overhead.
> > > > 
> > > > The runtime overhead is probably non-measurable compared with the cost
> > > > of the actual MMIOs.
> > > 
> > > Right. The code size increase is probably measurable (but still small),
> > > the runtime overhead is not.
> > 
> > Ok, so no rebuts or complains have been posted.
> > 
> > I've tested the patch you made in: https://lkml.org/lkml/2016/5/9/354
> > and it works: 
> > 
> > Tested-by: Christian Lamparter 
> > 
> > So, how do we go from here? There is are two small issues with the
> > original patch (#ifdef DWC2_LOG_WRITES got converted to lower case:
> > #ifdef dwc2_log_writes) and I guess a proper subject would be nice.  
> > 
> > Arnd, can you please respin and post it (cc'd stable as well)?
> > So this is can be picked up? Or what's your plan?
> 
> (I just realized my reply was stuck in my outbox, so the patch
> went out first)
> 
> If I recall correctly, the rough consensus was to go with your longer
> patch in the future (fixed up for the comments that BenH and
> I sent), and I'd suggest basing it on top of a fixed version of
> my patch.
Well, but it comes with the "overhead"! So this was just as I said:
"Let's look at it and see if it's any good"... And I think it isn't
since the usb/host/ehci people also opted for #ifdef CONFIG_BIG_ENDIAN
archs etc...
 
> Felipe just had another idea, to change the endianess of the dwc2
> block by setting a registers (if that exists). That would indeed
> be preferable, then we can just revert the broken change that
> went into 4.4 and backport that fix instead.
Just a quick reply. I have the docs for the thing. There's something
like that in GAHBCFG at Bit 24... BUT it only switches the endiannes
for the DMA descriptors (which is not always used, there are devices
with PIO only)! It doesn't deal with the MMIO access at all. 

The pin that would select which endian the device uses is probably
connected to a DCR or GPIO but I don't know which or where so this
is more or less useless. (Or the selectable endianness was dropped
during synth).

Regards,
Christian
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: usb: dwc2: regression on MyBook Live Duo / Canyonlands since 4.3.0-rc4

2016-05-12 Thread Christian Lamparter via Linuxppc-dev 
On Tuesday, May 10, 2016 09:23:59 AM Arnd Bergmann wrote:
> On Tuesday 10 May 2016 08:37:52 Benjamin Herrenschmidt wrote:
> > On Mon, 2016-05-09 at 17:08 +0200, Arnd Bergmann wrote:
> > > 
> > > Unfortunately, I don't see any way this could be done in MIPS specific
> > > code: There is typically a byteswap between the internal bus and the PCI
> > > bus on big-endian MIPS systems, so the PCI MMIO ends up being 
> > > little-endian,
> > 
> > Ugh ... not exactly, re-watch my talk on the matter :-) While there is
> > a specific lane wiring to preserve byte addresss, in the end it's the
> > end device itself that is either BE or LE. Regardless of any "bus
> > endianness".
> 
> I found your slides on
> 
> http://www.linuxplumbersconf.org/2012/wp-content/uploads/2012/09/2012-lpc-ref-big-little-endian-herrenschmidt.odp
> 
> but there are at least two more twists that you completely missed here:
> 
> - Some architectures (e.g. ARMv5 "BE32" mode in IXP4xx, surely some others)
>   do not implement big-endian mode by wiring up the data lines between the
>   bus and the CPU differently between big- and little-endian mode like
>   powerpc and armv7 "BE8" do, but instead they swizzle the *address* lines
>   on 8-bit and 16-bit addresses. The effect of that is that normal RAM
>   accesses work as expected both ways, and devices that are accessed using
>   32-bit MMIO ops never need any byteswap (you actually get "native
>   endian") while MMIO with 8 and 16 bit width does something completely
>   unexpected and touches the wrong register. Having an explicit byteswap
>   on the PCI host bridge gets you the expected addresses again for 8-bit
>   cycles but it also means that readl()/writel() again need to swap the
>   data.
> 
> - Some other architectures (e.g. Broadcom MIPS) apparently are even fancier
>   and use a strapping pin on the SoC flips the endianess of the CPU core
>   at the same time as all the peripheral MMIO registers, with the intention
>   of never requiring any byte swaps. I believe they are implemented careful
>   enough to actually get this right, but it confuses the heck out of
>   Linux drivers that don't expect this.
> 
> > > which matches the expected behavior of readl/writel. However, drivers
> > > for non-PCI devices often use the same readl/writel accessors because
> > > that is how it's done on ARMv6/ARMv7.
> > 
> > Even then, you can have on-SoC (non-PCI) devices that also have a
> > different endianness from the main CPU. How does it work on ARM for
> > example ? The device endianness should be fixed, regardless of the
> > endianness of the core, no ?
> 
> ARMv6/v7 is uses BE8 mode like powerpc: each peripheral is fixed-endian
> and you have to know what it is. Only Freescale managed to put identical
> IP blocks on various (powerpc-derived) SoCs and have a subset of them
> treat the access as little-endian while others remain big-endian, so all
> those drivers now require runtime detection.
> 
> > > Doing it hardcoded by architecture is just the simplest way to deal
> > > with it, working on the assumption that nothing actually needs the
> > > runtime detection that Ben suggested. 
> > 
> > No, it's not an archicture problem. It's a problem specific to that one
> > SoC that the device was synthetized to be a certain endian while it was
> > synthetized differently on another SoC... that also happens to be a
> > different architecture. But doesn't have to.
> > 
> > For example, we had in the past cases of both LE and BE EHCI
> > implementations on the same architecture (PowerPC).
> 
> I understand this, but from what I see in this history of this particular
> driver, all ARM and PowerPC implementations chose to use LE registers for
> DWC2 because the normal approach for these is to not mess with endianess,
> while presumably all MIPS users of the same block wired up the endian-select
> line of the IP block to match that of the CPU core, again because it's
> what you are expected to do on a MIPS based SoC.
> 
> So hardcoding it per architecture would make an assumption based on
> the mindset of the SoC designers rather than strict technical differences,
> and that can fail as soon as someone does things differently on any of
> them (see the Freescale example), but I still think it's the easiest
> workaround for backporting to stable kernels. A revert of the original
> patch would be even easier, but that would break the one big-endian
> MIPS machine we know about.
> 
> > > Detecting the endianess of the
> > > device is probably the best future-proof solution, but it's also
> > > considerably more work to do in the driver, and comes with a
> > > tiny runtime overhead.
> > 
> > The runtime overhead is probably non-measurable compared with the cost
> > of the actual MMIOs.
> 
> Right. The code size increase is probably measurable (but still small),
> the runtime overhead is not.

Ok, so no rebuts or complains have been posted.

I've tested the patch you made in: https://lkml.org/lkml/2016/5/9/354

[PATCH] gpio: dt-bindings: add ibm,ppc4xx-gpio binding

2016-05-11 Thread Christian Lamparter via Linuxppc-dev 
This patch adds binding information for IBM/AMCC/APM GPIO
Controllers of the PowerPC 4XX series and compatible SoCs.

The "PowerPC 405EP Embedded Processor Data Sheet" has the
following to say about the GPIO controllers: "

 - Controller functions and GPIO registers are programmed
   and accessed via memory-mapped OPB bus master accesses

 - All GPIOs are pin-shared with other functions. DCRs control
   whether a particular pin that has GPIO capabilities acts
   as a GPIO or is used for another purpose.

 - Each GPIO outputs is separately programmable to emulate
   an open-drain driver (i.e. drives to zero, threestated if
   output bit is 1)

"
The ppc4xx_gpio.c driver is part of the platform/sysdev drivers
in arch/powerpc/sysdev.

Signed-off-by: Christian Lamparter 
---
I looked into arch/powerpc/sysdev/ppc4xx_gpio.c driver and
it doesn't have support for the tri-state logic (open drain
is disabled), but the hardware would support it.
(the #gpio-cells description suffers because of this, since
the high-z option isn't there).

Also there's another problem: There's no DCR pinmux driver?!
So sadly, there's not much information on how to use the DCRs
to control the which pin is muxed to the GPIO or to a SoC
function like the i2c.

This was all the valuable information I could find about the
hardware, so it is included it in the binding text, even
though there's no support for it... Is there anything else
to add?
---
 .../devicetree/bindings/gpio/ibm,ppc4xx-gpio.txt   | 24 ++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/ibm,ppc4xx-gpio.txt

diff --git a/Documentation/devicetree/bindings/gpio/ibm,ppc4xx-gpio.txt 
b/Documentation/devicetree/bindings/gpio/ibm,ppc4xx-gpio.txt
new file mode 100644
index 000..22aabb7
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/ibm,ppc4xx-gpio.txt
@@ -0,0 +1,24 @@
+* IBM/AMCC/APM GPIO Controller for PowerPC 4XX series and compatible SoCs
+
+All GPIOs are pin-shared with other functions. DCRs control whether a
+particular pin that has GPIO capabilities acts as a GPIO or is used for
+another purpose. GPIO outputs are separately programmable to emulate
+an open-drain driver.
+
+Required properties:
+   - compatible: must be "ibm,ppc4xx-gpio"
+   - reg: address and length of the register set for the device
+   - #gpio-cells: must be set to 2. The first cell is the pin number
+   and the second cell is used to specify the gpio polarity:
+   0 = active high
+   1 = active low
+   - gpio-controller: marks the device node as a gpio controller.
+
+Example:
+
+GPIO0: gpio@ef600b00 {
+   compatible = "ibm,ppc4xx-gpio";
+   reg = <0xef600b00 0x0048>;
+   #gpio-cells = <2>;
+   gpio-controller;
+};
-- 
2.8.1

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: usb: dwc2: regression on MyBook Live Duo / Canyonlands since 4.3.0-rc4

2016-05-08 Thread Christian Lamparter via Linuxppc-dev 
On Sunday, May 08, 2016 08:40:55 PM Benjamin Herrenschmidt wrote:
> On Sun, 2016-05-08 at 00:54 +0200, Christian Lamparter via Linuxppc-dev 
> wrote:
> > I've been looking in getting the MyBook Live Duo's USB OTG port
> > to function. The SoC is a APM82181. Which has a PowerPC 464 core
> > and related to the supported canyonlands architecture in
> > arch/powerpc/.
> > 
> > Currently in -next the dwc2 module doesn't load: 
> 
> Smells like the APM implementation is little endian. You might need to
> use a flag to indicate what endian to use instead and set it
> appropriately based on some DT properties.
I tried. As per common-properties[0], I added little-endian; but it has no
effect. I looked in dwc2_driver_probe and found no way of specifying the
endian of the device. It all comes down to the dwc2_readl & dwc2_writel
accessors. These - sadly - have been hardwired to use __raw_readl and
__raw_writel. So, it's always "native-endian". While common-properties
says little-endian should be preferred.

> > dwc2 4bff8.usbotg: dwc2_core_reset() HANG! AHB Idle GRSTCTL=80
> > dwc2 4bff8.usbotg: Bad value for GSNPSID: 0x0a29544f
> > 
> > Looking at the Bad GSNPSID value: 0x0a29544f. It is obvious that
> > this is an endian problem. git finds this patch:
> > 
> > commit 95c8bc3609440af5e4a4f760b8680caea7424396
> > Author: Antti Seppälä <a.sepp...@gmail.com>
> > Date:   Thu Aug 20 21:41:07 2015 +0300
> > 
> > usb: dwc2: Use platform endianness when accessing registers
> > 
> > This patch is necessary to access dwc2 registers correctly on
> > big-endian
> > systems such as the mips based SoCs made by Lantiq. Then dwc2 can
> > be
> > used to replace ifx-hcd driver for Lantiq platforms found e.g. in
> > OpenWrt.
> > 
> > The patch was autogenerated with the following commands:
> > $EDITOR core.h
> > sed -i "s/\<readl\>/dwc2_readl/g" *.c hcd.h hw.h
> > sed -i "s/\<writel\>/dwc2_writel/g" *.c hcd.h hw.h
> > 
> > Some files were then hand-edited to fix checkpatch.pl warnings
> > about
> > too long lines.
> > 
> > which unfortunately, broke the USB-OTG port on the MyBook Live Duo.
> > Reverting to the readl / writel:
> > 
> > --- 
> > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> > index 3c58d63..c021c1f 100644
> > --- a/drivers/usb/dwc2/core.h
> > +++ b/drivers/usb/dwc2/core.h
> > @@ -66,7 +66,7 @@
> >  
> >  static inline u32 dwc2_readl(const void __iomem *addr)
> >  {
> > -   u32 value = __raw_readl(addr);
> > +   u32 value = readl(addr);
> >  
> > /* In order to preserve endianness __raw_* operation is
> > used. Therefore
> >  * a barrier is needed to ensure IO access is not re-ordered 
> > across
> > @@ -78,7 +78,7 @@ static inline u32 dwc2_readl(const void __iomem
> > *addr)
> >  
> >  static inline void dwc2_writel(u32 value, void __iomem *addr)
> >  {
> > -   __raw_writel(value, addr);
> > +   writel(value, addr);
> >  
> > /*
> >  * In order to preserve endianness __raw_* operation is
> > used. Therefore
> > 
> > ---
> > 
> > restores the dwc-otg port to full working order:
> > dwc2 4bff8.usbotg: Specified GNPTXFDEP=1024 > 256
> > dwc2 4bff8.usbotg: EPs: 3, shared fifos, 2042 entries in SPRAM
> > dwc2 4bff8.usbotg: DWC OTG Controller
> > dwc2 4bff8.usbotg: new USB bus registered, assigned bus number 1
> > dwc2 4bff8.usbotg: irq 33, io mem 0x
> > hub 1-0:1.0: USB hub found
> > hub 1-0:1.0: 1 port detected
> > root@mbl:~# usb 1-1: new high-speed USB device number 2 using dwc2
> > 
> > So, what to do?
 ^^^

Regards,
Christian

[0] 
<http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/common-properties.txt>

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

usb: dwc2: regression on MyBook Live Duo / Canyonlands since 4.3.0-rc4

2016-05-07 Thread Christian Lamparter via Linuxppc-dev 
Hello,

I've been looking in getting the MyBook Live Duo's USB OTG port
to function. The SoC is a APM82181. Which has a PowerPC 464 core
and related to the supported canyonlands architecture in arch/powerpc/.

Currently in -next the dwc2 module doesn't load: 

dwc2 4bff8.usbotg: dwc2_core_reset() HANG! AHB Idle GRSTCTL=80
dwc2 4bff8.usbotg: Bad value for GSNPSID: 0x0a29544f

Looking at the Bad GSNPSID value: 0x0a29544f. It is obvious that
this is an endian problem. git finds this patch:

commit 95c8bc3609440af5e4a4f760b8680caea7424396
Author: Antti Seppälä 
Date:   Thu Aug 20 21:41:07 2015 +0300

usb: dwc2: Use platform endianness when accessing registers

This patch is necessary to access dwc2 registers correctly on big-endian
systems such as the mips based SoCs made by Lantiq. Then dwc2 can be
used to replace ifx-hcd driver for Lantiq platforms found e.g. in
OpenWrt.

The patch was autogenerated with the following commands:
$EDITOR core.h
sed -i "s/\/dwc2_readl/g" *.c hcd.h hw.h
sed -i "s/\/dwc2_writel/g" *.c hcd.h hw.h

Some files were then hand-edited to fix checkpatch.pl warnings about
too long lines.

which unfortunately, broke the USB-OTG port on the MyBook Live Duo.
Reverting to the readl / writel:

--- 
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 3c58d63..c021c1f 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -66,7 +66,7 @@
 
 static inline u32 dwc2_readl(const void __iomem *addr)
 {
-   u32 value = __raw_readl(addr);
+   u32 value = readl(addr);
 
/* In order to preserve endianness __raw_* operation is used. Therefore
 * a barrier is needed to ensure IO access is not re-ordered across
@@ -78,7 +78,7 @@ static inline u32 dwc2_readl(const void __iomem *addr)
 
 static inline void dwc2_writel(u32 value, void __iomem *addr)
 {
-   __raw_writel(value, addr);
+   writel(value, addr);
 
/*
 * In order to preserve endianness __raw_* operation is used. Therefore

---

restores the dwc-otg port to full working order:
dwc2 4bff8.usbotg: Specified GNPTXFDEP=1024 > 256
dwc2 4bff8.usbotg: EPs: 3, shared fifos, 2042 entries in SPRAM
dwc2 4bff8.usbotg: DWC OTG Controller
dwc2 4bff8.usbotg: new USB bus registered, assigned bus number 1
dwc2 4bff8.usbotg: irq 33, io mem 0x
hub 1-0:1.0: USB hub found
hub 1-0:1.0: 1 port detected
root@mbl:~# usb 1-1: new high-speed USB device number 2 using dwc2

So, what to do?

Regards,
Christian
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v1 00/23] ata: sata_dwc_460ex: make it working again

2016-04-24 Thread Christian Lamparter via Linuxppc-dev 
On Sunday, April 24, 2016 09:05:43 AM Julian Margetson wrote:
> On 4/23/2016 3:41 PM, Christian Lamparter wrote:
> > There's a known errata for the 460EX, with the CPU lockup upon
> > high AHB traffic:
> > 
> >
> > "This patch implements a fix provided by AMCC so that the lockup upon
> > simultanious traffic on AHB USB OTG, USB 2.0 and SATA doesn't occur
> > anymore:..."
> >
> > This should be fixed by u-boot. However, there's no telling if
> > there's more to this workaround in the dma engine. You could try
> > to do the testing without anything connected to the USB ports
> > and disable/remove all usb hcds modules. As for fixing this:
> > I did a quick search but couldn't find any public information.
> > There's always supp...@apm.com (contact them!), or maybe someone
> > from the Amiga community knows more?
> >
> >
> 
> Tested with kernel with all USB disabled. No sata error messages 
> during the partition copy but the copying is quite slow.
Ok. The CONFIG_DMADEVICES_DEBUG and CONFIG_DMADEVICES_VDEBUG option
have quite a large overhead, if this fixed the issue for now you
could try to disable them and look if the issue comes back or not.
(also, you can drop the mdelay patch if it's still applied). If
the issue doesn't come back, you could add your "Tested-by" tag
too.

Another thing, the sata dwc driver doesn't yet support NCQ. Do you
know if the driver for the Amiga OS does?

> so this does appear to be the problem.
So, how to fix this? I know, there's an AHB DMA Arbiter. But I can't
get any documentation for it from AMCC/APM. Maybe denx.de or someone
from the Amiga community knows how to deal with it. In theory, we 
could try if limiting the burst length, pending dma request count or
add code to retry failed dma transfers and reinit the usb-cores would
help.

Regards,
Christian
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v1 00/23] ata: sata_dwc_460ex: make it working again

2016-04-23 Thread Christian Lamparter via Linuxppc-dev 
On Saturday, April 23, 2016 11:44:09 AM Julian Margetson wrote:
> On 4/23/2016 8:02 AM, Julian Margetson wrote:
> > On 4/22/2016 7:06 AM, Christian Lamparter wrote:
> >> On Friday, April 22, 2016 06:50:44 AM Julian Margetson wrote:
> >>> On 4/21/2016 4:25 PM, Christian Lamparter wrote:
>  On Thursday, April 21, 2016 09:15:21 PM Andy Shevchenko wrote:
> > The last approach in the commit 8b3444852a2b ("sata_dwc_460ex: 
> > move to generic
> > DMA driver") to switch to generic DMA engine API wasn't tested on 
> > bare metal.
> > Besides that we expecting new board support coming with the same 
> > SATA IP but
> > with different DMA.
> >
> > The driver has been tested myself on Sam460ex and WD MyBookLive 
> > (apollo3g)
> > boards. In any case I ask Christian, Måns, and Julian to 
> > independently test and
> > provide Tested-by tag or error report.
>  I did a test run on my WD MyBook Live. I applied all the patches in
>  this series on top of the topic/dw branch of Vinod Koul:
>  
> 
>  Tested-by: Christian Lamparter
>  ---
>  results for my old ST3808110AS HDD. filesystem is ext4.
> 
>  # hdparm -t /dev/sda
> 
>  /dev/sda:
> Timing buffered disk reads: 204 MB in  3.02 seconds = 67.51 MB/sec
> 
>  # bonnie++ -u mbl
>  Using uid:1000, gid:1000.
>  Writing a byte at a time...done
>  Writing intelligently...done
>  Rewriting...done
>  Reading a byte at a time...done
>  Reading intelligently...done
>  start 'em...done...done...done...done...done...
>  Create files in sequential order...done.
>  Stat files in sequential order...done.
>  Delete files in sequential order...done.
>  Create files in random order...done.
>  Stat files in random order...done.
>  Delete files in random order...done.
>  Version  1.97   --Sequential Output-- --Sequential 
>  Input- --Random-
>  Concurrency   1 -Per Chr- --Block-- -Rewrite- -Per Chr- 
>  --Block-- --Seeks--
>  MachineSize K/sec %CP K/sec %CP K/sec %CP K/sec %CP K/sec 
>  %CP  /sec %CP
>  mbl496M98  99 26011  21 17589  20   538  99 80138  
>  39 208.9   8
>  Latency 95267us1409ms 295ms   26947us 9644us
>  1787ms
>  Version  1.97   --Sequential Create-- Random 
>  Create
>  mbl -Create-- --Read--- -Delete-- -Create-- 
>  --Read--- -Delete--
>  files  /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec 
>  %CP  /sec %CP
> 16  6959  78 + +++  5197  40  7250 79 + 
>  +++  4718  37
>  Latency   149ms6742us 212ms 177ms 767us 
>  217ms
>  1.97,1.97,mbl,1,1461269771,496M,,98,99,26011,21,17589,20,538,99,80138,39,208.9,8,16,6959,78,+,+++,5197,40,7250,79,+,+++,4718,37,95267us,1409ms,295ms,26947us,9644us,1787ms,149ms,6742us,212ms,177ms,767us,217ms
>   
> 
> 
> >>> Again on copy partitions .
> >> Ok, here's the copy from my mail off-list.
> >>
> >> Well, a unrelated driver "m41t80" caused a crash:
> >> [   12.912739] Oops: Kernel access of bad area, sig: 11 [#3]
> >> [   12.912743] PREEMPT Canyonlands
> >> [   12.912753] CPU: 0 PID: 1413 Comm: irq/45-m41t80 Tainted: G  
> >> D 4.6.0-rc4-next-20160421-sam460ex-jm #1
> >> [   12.912757] task: ea9834e0 ti: eea6c000 task.ti: eea6c000
> >> [   12.912760] NIP: c0224480 LR: c0023494 CTR: c0042508
> >> [   12.912764] REGS: eea6daf0 TRAP: 0300   Tainted: G D  
> >> (4.6.0-rc4-next-20160421-sam460ex-jm)
> >> [   12.912774] MSR: 00029000   CR: 24008282 XER: 
> >> [   12.912825] DEAR: 0008 ESR: 
> >> [...]
> >> [   12.912927] --- interrupt: 300 at mutex_lock+0x0/0x1c
> >> [   12.912927] LR = m41t80_handle_irq+0x28/0xac
> >> [   12.912932] [eea6de40] []   (null) (unreliable)
> >> [   12.912938] [eea6de60] [c004ffac] irq_thread_fn+0x2c/0x48
> >> [   12.912944] [eea6de80] [c00501cc] irq_thread+0xc4/0x160
> >> [   12.912951] [eea6ded0] [c003a3f8] kthread+0xc8/0xcc
> >> [   12.912957] [eea6df40] [c000aee8] ret_from_kernel_thread+0x5c/0x64
> >> [   12.912960] Instruction dump:
> >> [   12.912974] 80010014 7fc3f378 bbc10008 7c0803a6 38210010 4be24ca8 
> >> 9421ffd0 7c0802a6
> >> [   12.912987] bf210014 90010034 3b4302d8 812302ec <83890008> 
> >> 812302d8 7f9a4840 419e011c
> >> [   12.912995] Fixing recursive fault but reboot is needed!
> >>   ^^^ "reboot is needed!"
> >>
> >> Another thing that came to my mind: Have you checked if your hard drive
> >> and the cables are ok? Are there any pending sectors or suspicious smart
> >> values? Has the drive passed the extended offline test?
> >>   Otherwise, I can't reproduce the error with my MyBook

Re: [PATCH v1 08/23] ata: sata_dwc_460ex: don't call ata_sff_qc_issue() on DMA commands

2016-04-22 Thread Christian Lamparter via Linuxppc-dev 
On Friday, April 22, 2016 11:32:00 AM David Laight wrote:
> From: Andy Shevchenko
> > Sent: 21 April 2016 19:15
> > ata_sff_qc_issue() can't handle DMA commands and thus we have to avoid it 
> > for
> > them. Do call ata_bmdma_qc_issue() instead for this case.
> > 
> > Suggested-by: Christian Lamparter 
> > Signed-off-by: Andy Shevchenko 
> > ---
> >  drivers/ata/sata_dwc_460ex.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> > index 038e5fb..845c35d 100644
> > --- a/drivers/ata/sata_dwc_460ex.c
> > +++ b/drivers/ata/sata_dwc_460ex.c
> > @@ -1061,10 +1061,12 @@ static unsigned int sata_dwc_qc_issue(struct 
> > ata_queued_cmd *qc)
> > __func__, tag, qc->ap->link.sactive, sactive);
> > 
> > ap->ops->sff_tf_load(ap, >tf);
> > -   sata_dwc_exec_command_by_tag(ap, >tf, qc->tag,
> > +   sata_dwc_exec_command_by_tag(ap, >tf, tag,
> >  SATA_DWC_CMD_ISSUED_PEND);
> > +   } else if (ata_is_dma(qc->tf.protocol)) {
> > +   return ata_bmdma_qc_issue(qc);
> > } else {
> > -   ata_sff_qc_issue(qc);
> > +   return ata_sff_qc_issue(qc);
> > }
> > return 0;
> >  }
> 
> I'd nuke those 'else if', they make it very hard to read.
> I Think the code is:
> 
>   sata_dwc_exec_command_by_tag(...);
>   return 0;
>   }
> 
>   if (ata_is_dma(qc->tf.protocol))
>   return ata_bmdma_qc_issue(qc);
> 
>   return ata_sff_qc_issue(qc);
> }

What about something like this instead? ata_bmdma_qc_issue already calls
ata_sff_qc_issue, if it's not a dma transfere anyway [0].
---
diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 6a61184..67cce54 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1096,12 +1096,9 @@ static unsigned int sata_dwc_qc_issue(struct 
ata_queued_cmd *qc)
ap->ops->sff_tf_load(ap, >tf);
sata_dwc_exec_command_by_tag(ap, >tf, tag,
 SATA_DWC_CMD_ISSUED_PEND);
-   } else if (ata_is_dma(qc->tf.protocol)) {
+   return 0;
+   } else
return ata_bmdma_qc_issue(qc);
-   } else {
-   return ata_sff_qc_issue(qc);
-   }
-   return 0;
 }
 
 static void sata_dwc_error_handler(struct ata_port *ap)
---

[0] 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v1 00/23] ata: sata_dwc_460ex: make it working again

2016-04-22 Thread Christian Lamparter via Linuxppc-dev 
On Friday, April 22, 2016 06:50:44 AM Julian Margetson wrote:
> On 4/21/2016 4:25 PM, Christian Lamparter wrote:
> > On Thursday, April 21, 2016 09:15:21 PM Andy Shevchenko wrote:
> >> The last approach in the commit 8b3444852a2b ("sata_dwc_460ex: move to 
> >> generic
> >> DMA driver") to switch to generic DMA engine API wasn't tested on bare 
> >> metal.
> >> Besides that we expecting new board support coming with the same SATA IP 
> >> but
> >> with different DMA.
> >>
> >> The driver has been tested myself on Sam460ex and WD MyBookLive (apollo3g)
> >> boards. In any case I ask Christian, Måns, and Julian to independently 
> >> test and
> >> provide Tested-by tag or error report.
> > I did a test run on my WD MyBook Live. I applied all the patches in
> > this series on top of the topic/dw branch of Vinod Koul:
> > 
> >
> > Tested-by: Christian Lamparter 
> > ---
> > results for my old ST3808110AS HDD. filesystem is ext4.
> >
> > # hdparm -t /dev/sda
> >
> > /dev/sda:
> >   Timing buffered disk reads: 204 MB in  3.02 seconds =  67.51 MB/sec
> >
> > # bonnie++ -u mbl
> > Using uid:1000, gid:1000.
> > Writing a byte at a time...done
> > Writing intelligently...done
> > Rewriting...done
> > Reading a byte at a time...done
> > Reading intelligently...done
> > start 'em...done...done...done...done...done...
> > Create files in sequential order...done.
> > Stat files in sequential order...done.
> > Delete files in sequential order...done.
> > Create files in random order...done.
> > Stat files in random order...done.
> > Delete files in random order...done.
> > Version  1.97   --Sequential Output-- --Sequential Input- 
> > --Random-
> > Concurrency   1 -Per Chr- --Block-- -Rewrite- -Per Chr- --Block-- 
> > --Seeks--
> > MachineSize K/sec %CP K/sec %CP K/sec %CP K/sec %CP K/sec %CP  /sec 
> > %CP
> > mbl496M98  99 26011  21 17589  20   538  99 80138  39 208.9 
> >   8
> > Latency 95267us1409ms 295ms   26947us9644us
> > 1787ms
> > Version  1.97   --Sequential Create-- Random 
> > Create
> > mbl -Create-- --Read--- -Delete-- -Create-- --Read--- 
> > -Delete--
> >files  /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP  
> > /sec %CP
> >   16  6959  78 + +++  5197  40  7250  79 + +++  
> > 4718  37
> > Latency   149ms6742us 212ms 177ms 767us 
> > 217ms
> > 1.97,1.97,mbl,1,1461269771,496M,,98,99,26011,21,17589,20,538,99,80138,39,208.9,8,16,6959,78,+,+++,5197,40,7250,79,+,+++,4718,37,95267us,1409ms,295ms,26947us,9644us,1787ms,149ms,6742us,212ms,177ms,767us,217ms
> >
> Again on copy partitions .

Ok, here's the copy from my mail off-list.

Well, a unrelated driver "m41t80" caused a crash:
[   12.912739] Oops: Kernel access of bad area, sig: 11 [#3]
[   12.912743] PREEMPT Canyonlands
[   12.912753] CPU: 0 PID: 1413 Comm: irq/45-m41t80 Tainted: G  D 
4.6.0-rc4-next-20160421-sam460ex-jm #1
[   12.912757] task: ea9834e0 ti: eea6c000 task.ti: eea6c000
[   12.912760] NIP: c0224480 LR: c0023494 CTR: c0042508
[   12.912764] REGS: eea6daf0 TRAP: 0300   Tainted: G  D  
(4.6.0-rc4-next-20160421-sam460ex-jm)
[   12.912774] MSR: 00029000   CR: 24008282  XER: 
[   12.912825] DEAR: 0008 ESR:  
[...]
[   12.912927] --- interrupt: 300 at mutex_lock+0x0/0x1c
[   12.912927] LR = m41t80_handle_irq+0x28/0xac
[   12.912932] [eea6de40] []   (null) (unreliable)
[   12.912938] [eea6de60] [c004ffac] irq_thread_fn+0x2c/0x48
[   12.912944] [eea6de80] [c00501cc] irq_thread+0xc4/0x160
[   12.912951] [eea6ded0] [c003a3f8] kthread+0xc8/0xcc
[   12.912957] [eea6df40] [c000aee8] ret_from_kernel_thread+0x5c/0x64
[   12.912960] Instruction dump:
[   12.912974] 80010014 7fc3f378 bbc10008 7c0803a6 38210010 4be24ca8 9421ffd0 
7c0802a6 
[   12.912987] bf210014 90010034 3b4302d8 812302ec <83890008> 812302d8 7f9a4840 
419e011c 
[   12.912995] Fixing recursive fault but reboot is needed!
 ^^^ "reboot is needed!"

Another thing that came to my mind: Have you checked if your hard drive
and the cables are ok? Are there any pending sectors or suspicious smart
values? Has the drive passed the extended offline test?
 
Otherwise, I can't reproduce the error with my MyBook system. I've tested
your kernel and it worked on the device without crashing. (I copied/dd'ed
80GB from and back to the hard-drive. It was long and boring, but I didn't
encounter any issues and the crc32 matched).

Sorry, but I can't help you if I can't reproduce it... And short of sending
your box to test, I see no efficient way to debug it. However, what I can
do, if you are interested: I have a few "build your own" My Book Live kits.
It just needs a 3.5" hard-drive and 12v power adapter. If you are interested
PM me off-list, this

Re: [PATCH v1 00/23] ata: sata_dwc_460ex: make it working again

2016-04-21 Thread Christian Lamparter via Linuxppc-dev 
On Thursday, April 21, 2016 09:15:21 PM Andy Shevchenko wrote:
> The last approach in the commit 8b3444852a2b ("sata_dwc_460ex: move to generic
> DMA driver") to switch to generic DMA engine API wasn't tested on bare metal.
> Besides that we expecting new board support coming with the same SATA IP but
> with different DMA.
> 
> This series is targetting the following things:
> - a few bug fixes to the original driver
> - a part to fix the DMA engine usage and in particularly dw_dmac driver
> - move driver to use generic PHY and "dmas" property which leads to update in 
> DTS
> 
> The driver has been tested myself on Sam460ex and WD MyBookLive (apollo3g)
> boards. In any case I ask Christian, Måns, and Julian to independently test 
> and
> provide Tested-by tag or error report.
I did a test run on my WD MyBook Live. I applied all the patches in
this series on top of the topic/dw branch of Vinod Koul:


Tested-by: Christian Lamparter 
---
results for my old ST3808110AS HDD. filesystem is ext4.

# hdparm -t /dev/sda

/dev/sda:
 Timing buffered disk reads: 204 MB in  3.02 seconds =  67.51 MB/sec

# bonnie++ -u mbl
Using uid:1000, gid:1000.
Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done...
Create files in sequential order...done.
Stat files in sequential order...done.
Delete files in sequential order...done.
Create files in random order...done.
Stat files in random order...done.
Delete files in random order...done.
Version  1.97   --Sequential Output-- --Sequential Input- --Random-
Concurrency   1 -Per Chr- --Block-- -Rewrite- -Per Chr- --Block-- --Seeks--
MachineSize K/sec %CP K/sec %CP K/sec %CP K/sec %CP K/sec %CP  /sec %CP
mbl496M98  99 26011  21 17589  20   538  99 80138  39 208.9   8
Latency 95267us1409ms 295ms   26947us9644us1787ms
Version  1.97   --Sequential Create-- Random Create
mbl -Create-- --Read--- -Delete-- -Create-- --Read--- -Delete--
  files  /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP
 16  6959  78 + +++  5197  40  7250  79 + +++  4718  37
Latency   149ms6742us 212ms 177ms 767us 217ms
1.97,1.97,mbl,1,1461269771,496M,,98,99,26011,21,17589,20,538,99,80138,39,208.9,8,16,6959,78,+,+++,5197,40,7250,79,+,+++,4718,37,95267us,1409ms,295ms,26947us,9644us,1787ms,149ms,6742us,212ms,177ms,767us,217ms


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: crash in ppc4xx-rng on canyonland

2016-04-18 Thread Christian Lamparter via Linuxppc-dev 
On Monday, April 18, 2016 05:59:39 PM Herbert Xu wrote:
> Christian Lamparter  wrote:
> > 
> > I tried to move ppc4xx-rng into crypto4xx (see attachment - patch #1).
> > The driver works as is. But I can't come up with a way to attach the
> > crypto4xx driver to the ppc4xx-rng OF node cleanly. Basically,
> > I'm looking for a way to have one driver (with one context) be 
> > in charge of two different OF nodes (ppc4xx-rng and ppc4xx-crypto).
> > Is there any solution to this? Because otherwise, I would add a
> 
> Is it possible to have an RNG unit without the crypto unit?
No. In AMCC's product brief the TRNG (true random number generator) is
part of the security engine. The security engine also provides more 
features like a "public key authentication" core which has it's own
address range.


> If not then your first patch should be OK, provided that you add
> some error handling when the RNG probe fails.  For example, if 
> the RNG probe fails we should probably not call remove on it later.
I checked the error handling code again and verified it works on the
device. The original code resets the dev->trng_base = NULL and
core_dev->trng = NULL; in the err_out case. the "dev and core_dev"
are coming from the main crypto driver, they will always be
valid. Hence ppc4xx_trng_remove can safely be executed, even if 
ppc4xx_trng_probe fails as devm_hwrng_unregister, iounmap and kfree
can deal with the NULL properly. Nevertheless, I added an early
bailout if core_dev->trng == NULL.

what else I fixed in v1->v2: 
 - added a check to test trng device's status state with
   of_device_is_available.
 - if the hwrng device registration failed, the flag which
   enables the trng was left enabled (note: the v1 code
   disabled the hwrng device as part of crypto4xx_remove.
   so it wasn't enabled when the crypto4xx driver was
   unloaded)

---
From c0b580a50bdade97f0d06c98fc7dccbf64d25eb2 Mon Sep 17 00:00:00 2001
From: Christian Lamparter 
Date: Mon, 18 Apr 2016 12:57:41 +0200
Subject: [PATCH v2] crypto4xx: integrate ppc4xx-rng into crypto4xx

This patch integrates the ppc4xx-rng driver into the existing
crypto4xx. This is because the true random number generator
is controlled and part of the security core.

Signed-off-by: Christian Lamparter 
---
 drivers/char/hw_random/Kconfig  |  13 ---
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/ppc4xx-rng.c | 147 
 drivers/crypto/Kconfig  |   8 ++
 drivers/crypto/amcc/Makefile|   1 +
 drivers/crypto/amcc/crypto4xx_core.c|   7 +-
 drivers/crypto/amcc/crypto4xx_core.h|   4 +
 drivers/crypto/amcc/crypto4xx_reg_def.h |   1 +
 drivers/crypto/amcc/crypto4xx_trng.c| 131 
 drivers/crypto/amcc/crypto4xx_trng.h|  34 
 10 files changed, 184 insertions(+), 163 deletions(-)
 delete mode 100644 drivers/char/hw_random/ppc4xx-rng.c
 create mode 100644 drivers/crypto/amcc/crypto4xx_trng.c
 create mode 100644 drivers/crypto/amcc/crypto4xx_trng.h

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 3c5be60..abc8720 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -268,19 +268,6 @@ config HW_RANDOM_NOMADIK
 
  If unsure, say Y.
 
-config HW_RANDOM_PPC4XX
-   tristate "PowerPC 4xx generic true random number generator support"
-   depends on PPC && 4xx
-   default HW_RANDOM
-   ---help---
-This driver provides the kernel-side support for the TRNG hardware
-found in the security function of some PowerPC 4xx SoCs.
-
-To compile this driver as a module, choose M here: the
-module will be called ppc4xx-rng.
-
-If unsure, say N.
-
 config HW_RANDOM_PSERIES
tristate "pSeries HW Random Number Generator support"
depends on PPC64 && IBMVIO
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index f5a6fa7..079745f 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -22,7 +22,6 @@ obj-$(CONFIG_HW_RANDOM_TX4939) += tx4939-rng.o
 obj-$(CONFIG_HW_RANDOM_MXC_RNGA) += mxc-rnga.o
 obj-$(CONFIG_HW_RANDOM_OCTEON) += octeon-rng.o
 obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
-obj-$(CONFIG_HW_RANDOM_PPC4XX) += ppc4xx-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
 obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
 obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
diff --git a/drivers/char/hw_random/ppc4xx-rng.c 
b/drivers/char/hw_random/ppc4xx-rng.c
deleted file mode 100644
index c0db438..000
--- a/drivers/char/hw_random/ppc4xx-rng.c
+++ /dev/null
@@ -1,147 +0,0 @@
-/*
- * Generic PowerPC 44x RNG driver
- *
- * Copyright 2011 IBM Corporation
- *
- * This program is free software; you can redistribute it

Re: crash in ppc4xx-rng on canyonland

2016-04-08 Thread Christian Lamparter via Linuxppc-dev 
On Tuesday, April 05, 2016 08:11:19 PM Herbert Xu wrote:
> Christian Lamparter  wrote:
> >
> > The crash is caused by a bad read in ppc4xx_rng_enable [0]. From what I
> > can tell, the driver is mapping the crypto control registers. The
> > problem is that they are claimed by the main crypto driver: crypto4xx [1].
> > 
> > I'm not sure what to do in this case. In my opinion the crypto4xx driver
> > should just initialize the trng [see patch]. I would like to move the
> > trng into the crypto-ppc4xx, but given that this breaks some of the DTS
> > out there I'm not sure.
> 
> I think you'll also need to ensure that the crypto module is
> successfully loaded before the RNG is accessed.
> 
> It's probably easier to just merge the two drivers but still
> maintaining the original driver names.

I tried to move ppc4xx-rng into crypto4xx (see attachment - patch #1).
The driver works as is. But I can't come up with a way to attach the
crypto4xx driver to the ppc4xx-rng OF node cleanly. Basically,
I'm looking for a way to have one driver (with one context) be 
in charge of two different OF nodes (ppc4xx-rng and ppc4xx-crypto).
Is there any solution to this? Because otherwise, I would add a
bool ppc4xx_trng variable in crypto4xx_core.c and use EXPORT_SYMBOL...
[see patch #2]


---
Patch #1

From 156bcb0eb06361c1668237ed2d0c3227b0837ff3 Mon Sep 17 00:00:00 2001
From: Christian Lamparter 
Date: Fri, 25 Mar 2016 19:00:05 +0100
Subject: [PATCH] crypto4xx: integrate ppc4xx-rng into crypto4xx

This patch integrates the ppc4xx-rng driver into the existing
crypto4xx. This is because the true random number generator
is controlled and part of the security core.

Signed-off-by: Christian Lamparter 
---
 drivers/char/hw_random/Kconfig  |  13 ---
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/ppc4xx-rng.c | 147 
 drivers/crypto/Kconfig  |   8 ++
 drivers/crypto/amcc/Makefile|   1 +
 drivers/crypto/amcc/crypto4xx_core.c|   7 +-
 drivers/crypto/amcc/crypto4xx_core.h|   4 +
 drivers/crypto/amcc/crypto4xx_reg_def.h |   1 +
 drivers/crypto/amcc/crypto4xx_trng.c| 128 +++
 drivers/crypto/amcc/crypto4xx_trng.h|  34 
 10 files changed, 181 insertions(+), 163 deletions(-)
 delete mode 100644 drivers/char/hw_random/ppc4xx-rng.c
 create mode 100644 drivers/crypto/amcc/crypto4xx_trng.c
 create mode 100644 drivers/crypto/amcc/crypto4xx_trng.h

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 67ee8b0..a6d7b9b 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -268,19 +268,6 @@ config HW_RANDOM_NOMADIK
 
  If unsure, say Y.
 
-config HW_RANDOM_PPC4XX
-   tristate "PowerPC 4xx generic true random number generator support"
-   depends on PPC && 4xx
-   default HW_RANDOM
-   ---help---
-This driver provides the kernel-side support for the TRNG hardware
-found in the security function of some PowerPC 4xx SoCs.
-
-To compile this driver as a module, choose M here: the
-module will be called ppc4xx-rng.
-
-If unsure, say N.
-
 config HW_RANDOM_PSERIES
tristate "pSeries HW Random Number Generator support"
depends on PPC64 && IBMVIO
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index f5a6fa7..079745f 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -22,7 +22,6 @@ obj-$(CONFIG_HW_RANDOM_TX4939) += tx4939-rng.o
 obj-$(CONFIG_HW_RANDOM_MXC_RNGA) += mxc-rnga.o
 obj-$(CONFIG_HW_RANDOM_OCTEON) += octeon-rng.o
 obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
-obj-$(CONFIG_HW_RANDOM_PPC4XX) += ppc4xx-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
 obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
 obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
diff --git a/drivers/char/hw_random/ppc4xx-rng.c 
b/drivers/char/hw_random/ppc4xx-rng.c
deleted file mode 100644
index c0db438..000
--- a/drivers/char/hw_random/ppc4xx-rng.c
+++ /dev/null
@@ -1,147 +0,0 @@
-/*
- * Generic PowerPC 44x RNG driver
- *
- * Copyright 2011 IBM Corporation
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; version 2 of the License.
- */
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#define PPC4XX_TRNG_DEV_CTRL 0x60080
-
-#define PPC4XX_TRNGE 0x0002
-#define PPC4XX_TRNG_CTRL 0x0008
-#define PPC4XX_TRNG_CTRL_DALM 0x20
-#define PPC4XX_TRNG_STAT 0x0004
-#define PPC4XX_TRNG_STAT_B 0x1
-#define PPC4XX_TRNG_DATA 0x
-
-#define MODULE_NAME "ppc4xx_rng"
-
-static int ppc4xx_rng_data_present(struct hwrng *rng, int wait)
-{
-   void __iomem *rng_regs = (void __iomem *)

crash in ppc4xx-rng on canyonland

2016-03-25 Thread Christian Lamparter via Linuxppc-dev 
I'm currently trying to port a Western Digital MyBook Live to
a 4.4.6 kernel. The device has a APM82181 and the board is called
Apollo-3G, which is a derivative of the "amcc,canyonland".

Almost everything is working, except when I try to enable the
ppc4xx-rng in the dts. Then the machine dies with the following:

[0.801839] Machine check in kernel mode.
[0.805830] Data Read PLB Error
[0.808962] Oops: Machine check, sig: 7 [#1]
[0.813208] Canyonlands
[0.815646] Modules linked in:
[0.818710] CPU: 0 PID: 1 Comm: swapper Not tainted 4.4.6 #39
[0.824436] task: cf83 ti: cfff2000 task.ti: cf834000
[0.829808] NIP: c01b9a00 LR: c01b99e8 CTR: 
[0.834751] REGS: cfff3f10 TRAP: 0214   Not tainted  (4.4.6)
[0.840382] MSR: 00029000   CR: 820c3222  XER: 
[0.846515] 
GPR00: c01b99e8 cf835d50 cf83 d108 d110 cf857800 0020 0004 
GPR08:  d10e d10e0080 0020051b 820c3428  c0001c54  
GPR16:       c042 c03d7290 
GPR24: 0043  c044e1cc c03a500f 0001  cfffb9b8 0005 
[0.876429] NIP [c01b9a00] ppc4xx_rng_enable+0xec/0x12c
[0.881634] LR [c01b99e8] ppc4xx_rng_enable+0xd4/0x12c
[0.886752] Call Trace:
[0.889194] [cf835d50] [c01b99e8] ppc4xx_rng_enable+0xd4/0x12c (unreliable)
[0.896141] [cf835db0] [c01b9ab8] ppc4xx_rng_probe+0x2c/0x80
[0.901791] [cf835dc0] [c01c0614] platform_drv_probe+0x34/0x70
[0.907606] [cf835dd0] [c01beda8] driver_probe_device+0x110/0x264
[0.913679] [cf835e00] [c01bef74] __driver_attach+0x78/0xac
[0.919243] [cf835e20] [c01bd300] bus_for_each_dev+0x9c/0xac
[0.924885] [cf835e50] [c01be49c] bus_add_driver+0xe0/0x1fc
[0.930441] [cf835e70] [c01bf688] driver_register+0xb4/0x104
[0.936086] [cf835e90] [c0001704] do_one_initcall+0x11c/0x1ac
[0.941825] [cf835f00] [c03d7af0] kernel_init_freeable+0x128/0x1c4
[0.947989] [cf835f30] [c0001c68] kernel_init+0x14/0x114
[0.953288] [cf835f40] [c000a748] ret_from_kernel_thread+0x5c/0x64
[0.959449] Instruction dump:
[0.962415] 2f9f0004 3bff0001 409effa8 3880 7fc3f378 4806f6c5 2c03 
41a2ff68 
[0.970197] 3d230006 39490080 7c0004ac 7d00542c <0c08> 4c00012c 2f9c 
550a03da 
[0.978180] ---[ end trace cff1212128646932 ]---

The crash is caused by a bad read in ppc4xx_rng_enable [0]. From what I
can tell, the driver is mapping the crypto control registers. The
problem is that they are claimed by the main crypto driver: crypto4xx [1].

I'm not sure what to do in this case. In my opinion the crypto4xx driver
should just initialize the trng [see patch]. I would like to move the
trng into the crypto-ppc4xx, but given that this breaks some of the DTS
out there I'm not sure.

Regards,
Christian
---
From 2159e200fcb68f88a94b1d5570d6000c100133a8 Mon Sep 17 00:00:00 2001
From: Christian Lamparter 
Date: Fri, 25 Mar 2016 19:00:05 +0100
Subject: [PATCH] ppc4xx-rng: fix crash during init

This patch fixes a crash that happens in the ppc4xx-rng driver
when accessing the main crypto core to enable the true random
number generator.

Signed-off-by: Christian Lamparter 
---
 drivers/char/hw_random/ppc4xx-rng.c | 42 -
 drivers/crypto/amcc/crypto4xx_core.c|  1 +
 drivers/crypto/amcc/crypto4xx_reg_def.h |  1 +
 3 files changed, 2 insertions(+), 42 deletions(-)

diff --git a/drivers/char/hw_random/ppc4xx-rng.c 
b/drivers/char/hw_random/ppc4xx-rng.c
index c0db438..a9a8a29 100644
--- a/drivers/char/hw_random/ppc4xx-rng.c
+++ b/drivers/char/hw_random/ppc4xx-rng.c
@@ -17,9 +17,6 @@
 #include 
 #include 
 
-#define PPC4XX_TRNG_DEV_CTRL 0x60080
-
-#define PPC4XX_TRNGE 0x0002
 #define PPC4XX_TRNG_CTRL 0x0008
 #define PPC4XX_TRNG_CTRL_DALM 0x20
 #define PPC4XX_TRNG_STAT 0x0004
@@ -51,40 +48,6 @@ static int ppc4xx_rng_data_read(struct hwrng *rng, u32 *data)
return 4;
 }
 
-static int ppc4xx_rng_enable(int enable)
-{
-   struct device_node *ctrl;
-   void __iomem *ctrl_reg;
-   int err = 0;
-   u32 val;
-
-   /* Find the main crypto device node and map it to turn the TRNG on */
-   ctrl = of_find_compatible_node(NULL, NULL, "amcc,ppc4xx-crypto");
-   if (!ctrl)
-   return -ENODEV;
-
-   ctrl_reg = of_iomap(ctrl, 0);
-   if (!ctrl_reg) {
-   err = -ENODEV;
-   goto out;
-   }
-
-   val = in_le32(ctrl_reg + PPC4XX_TRNG_DEV_CTRL);
-
-   if (enable)
-   val |= PPC4XX_TRNGE;
-   else
-   val = val & ~PPC4XX_TRNGE;
-
-   out_le32(ctrl_reg + PPC4XX_TRNG_DEV_CTRL, val);
-   iounmap(ctrl_reg);
-
-out:
-   of_node_put(ctrl);
-
-   return err;
-}
-
 static struct hwrng ppc4xx_rng = {
.name = MODULE_NAME,
.data_present = ppc4xx_rng_data_present,
@@ -100,10 +63,6 @@ static int ppc4xx_rng_probe(struct