Re: [RFC PATCH 1/1] powerpc/embedded6xx: Add support for Motorola/Emerson MVME5100.

Scott Wood [off-list ref] wrote on 08/21/2013 09:20:03 AM:

From: Scott Wood <redacted>
To: Stephen N Chivers <redacted>
Cc: <benh@kernel.crashing.org>, Chris Proctor <redacted>,
[off-list ref], [off-list ref]
Date: 08/21/2013 09:20 AM
Subject: Re: [RFC PATCH 1/1] powerpc/embedded6xx: Add support for 
Motorola/Emerson MVME5100.

On Tue, 2013-08-20 at 13:28 +1100, Stephen N Chivers wrote:

quoted

Scott Wood [off-list ref] wrote on 08/09/2013 11:35:20 AM:

quoted

From: Scott Wood <redacted>
To: Stephen N Chivers <redacted>
Cc: <benh@kernel.crashing.org>, <redacted>, Chris Proctor 
[off-list ref], [off-list ref]
Date: 08/09/2013 11:36 AM
Subject: Re: [RFC PATCH 1/1] powerpc/embedded6xx: Add support for 
Motorola/Emerson MVME5100.

simple-bus may be applicable here (in addition to a specific
compatible).

The HAWK ASIC is a difficult beast. I still cannot get a positive
identification as to what it is (Motorola/Freescale part number
unknown, not even the part number on the chip on the board helps....).
The best I can come up with is that it is a tsi108 without
the ethenets.
So device_type will be tsi-bridge and compatible will be
tsi108-bridge.

Don't use device_type.  compatible should include "hawk" in the name
(especially if you're not sure what's really in it), and/or the part
number on the chip.  If you're convinced it's fully compatible with
tsi108-bridge you can add that as a second compatible value, though
given the uncertainty it's probably better to just teach Linux to look
for the new compatible.

If devices on the bus can be used without any special bus setup or
knowledge, then you can add a compatible of "simple-bus" to the end.

quoted

quoted

Why not just look for a chrp,iic node directly?

I was following the model used in other places, like chrp/setup.c.

Not all examples are good examples. :-)

quoted

quoted

quoted

+       if ((np = of_find_compatible_node(NULL, "pci", 

"mpc10x-pci"))) 

quoted

{

quoted

Why insist on the device_type?

Following the model in the linkstation (kurobox) platform support. 

Drop the device_type check.

quoted

quoted

quoted

+static void
+mvme5100_restart(char *cmd)
+{
+       volatile ulong          i = 10000000;
+
+
+       local_irq_disable();
+       _nmask_and_or_msr(0, MSR_IP);

Does "mtmsr(mfmsr() | MSR_IP)" not work?

Don't know. Is from the original code by Matt Porter.

It actually appears that there are no callers remaining that use the
"and" portion of the functionality.  In fact there are no callers that
use it for anything other than setting MSR_IP. :-P

quoted

quoted

quoted

+       out_8((u_char *) BOARD_MODRST_REG, 0x01);
+
+       while (i-- > 0);

Do not use a loop to implement a delay.

Taken from the original code. But at this point the board
is going to reset and reboot via firmware, as /sbin/reboot
or /sbin/halt has been invoked.

Still, it's just a bad idea.  What's wrong with udelay()?

Or just use an infinite loop.  How much value is there really in timing
out here?

quoted

quoted

quoted

+static void __init
+mvme5100_set_bat(void)
+{
+
+
+       mb();
+       mtspr(SPRN_DBAT1U, 0xf0001ffe);
+       mtspr(SPRN_DBAT1L, 0xf000002a);
+       mb();
+       setbat(1, 0xfe000000, 0xfe000000, 0x02000000, 

PAGE_KERNEL_NCG);

quoted

quoted

+}

It is no longer allowed to squat on random virtual address space 

like

quoted

quoted

this.  If you really need a BAT you'll have to allocate the virtual
address properly.

Yes. I found that this was an anathema when researching the port in
2010 but I couldn't find any practical solution at the time.
The code is called early to ensure that the hawk registers are 

available.

quoted

sysdev/cpm_common.c does the same thing.

quoted

What is the correct solution?

ioremap() has special code to function early (using ioremap_bot).

If you still need to use a BAT that early, reserve the space with
asm/fixmap.h or by adding a function to the early ioremap code to just
reserve the space.  Or better, improve the ioremap code to be capable of
creating a BAT (or equivalent) when requested.

It is really interesting. Given that the UART implementation on the
HAWK is such that legacy_serial will not set up an early console it
is very likely that the address translation set up by the bat is not
required.
I can probably replace the physical addresses used in:

        setup_indirec_pci(hose, 0, 0xfe000cf8, 0xfe000cfc, 0);

with remapped equivalents. But, with the setbat eliminated, the
line:

        pcibios_alloc_controller(dev);

silently (remember no early console, due to UART reg-shift) panics.
It is happening at the point where the newly allocated PHB
structure is being added to the "hose_list" in pci-common.c.

So, I think there is some side effect due to the call to the setbat with 
the
PAGE_KERNEL_NCG parameter that I do not yet understand.
 

-Scott