[PATCH linux dev-4.7] arm: aspped: romulus: Set PNOR SPI address mapping

[Andrew Jeffery]

Replying to myself again... I think I've learnt the lesson not to email
whilst in bed.

I've done some further investigation after jumping on a Romulus
machine:

On Tue, 2017-02-28 at 23:10 +1030, Andrew Jeffery wrote:
> On Tue, 2017-02-28 at 20:27 +0800, Lei YU wrote:
> > Hi Andrew, Joel,
> > 
> > This patch is for issue https://github.com/openbmc/openbmc/issues/1214
> > 
> > Here's what I have tested:
> > 1. rom1-19 with updated OpenBMC (c85f23e1), which has mboxd enabled;
> > 2. Do a power cycle;
> > 3. Once BMC is ready, check the registers by devmem:
> >    ```
> > >    root at romulus:/tmp# devmem 0x1e630034
> > 
> >    0x70640000        <== Wrong, expect 0x70680000
> > >    root at romulus:/tmp# devmem 0x1e630030
> > 
> >    0x64600000        <== Wrong, expect 0x68600000
> >    ```
> > I can see the two registers have the unexpected value.
> > 
> > Without manually setting the two registers, `obmcutil poweron` results in
> > no output at all in host console.
> 
> Okay, can you please open an issue on github? I'd like to have a play
> with a Romulus system tomorrow to better understand what's going on
> here. We shouldn't need this kernel patch.
> 
> This makes me wonder if something's wrong with Zaius and Witherspoon as
> well, and whether the problem is masked by these register writes in the
> boardfile.
> 

I had assumed here you were talking about the HICR{7,8} registers, and
if I was paying more attention I would have realised you are not.

Instead, these are the {S,F}MC segment mapping configuration registers.
The default values correspond to a 32MB part. From the datasheet if we
do the transform ((0x64600000 >> 16) >> 1) we get 0x3230 which
translates to "start the mapping at 0x30000000, end at 0x32000000" and
thus a 32MiB mapping. With your expected values: ((0x68600000 >> 16) >>
1) we get 0x3430, or "start the mapping at 0x30000000, end at
0x34000000", which is a 64MiB mapping.

Looking at the driver[1] the only time we touch the segment register is
to read the window start, we don't ever write the segment mapping
configuration.

Cédric: Have you looked into supporting configuration of the segment
mapping?

We could configure the mapping if we know the chip sizes, however
looking at the device's bindings documentation[2] we find:

    ...
    Required properties:
    ...
     - #size-cells : must be 0 corresponding to chip select child binding

and

    ...
    Child node required properties:
      - reg : must contain chip select number in first cell of address, must
    be 1 tuple long
    ...

I think we need #size-cells to be 1 in the parent so we can define the
mapping ranges. I don't know that its going to be an easy change to
make though given the bindings are already upstream. Thoughts?

Lei: So as it stands you should drop the HICRx writes from your patch.
However as it stands, we still need the writes to the SMC segment
registers until we can sort out proper driver support.

Cheers,

Andrew

[1] https://github.com/openbmc/linux/blob/dev-4.7/drivers/mtd/spi-nor/aspeed-smc.c#L728
[2] https://github.com/openbmc/linux/blob/dev-4.7/Documentation/devicetree/bindings/mtd/aspeed-smc.txt
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170301/58e479cc/attachment.sig>