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

[Cédric Le Goater]

On 03/02/2017 03:55 AM, Andrew Jeffery wrote:
> On Wed, 2017-03-01 at 08:26 +0100, Cédric Le Goater wrote:
>> On 03/01/2017 04:05 AM, Andrew Jeffery wrote:
>>> 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.
>>
>> These are the default values : 
>>
>> SPIR30: SPI1 CE0 Address Decoding Range Register Init = 0x6460 0000
>> SPIR34: SPI1 CE1 Address Decoding Range Register Init = 0x7064 0000
>>
>> We should let the driver configure these values I think.
>> see below.
>>
> 
> Agreed.
> 
>>>
>>> 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.
>>
>> yes, not in 4.7 yet. It is a little "complex" to configure the 
>> segments up to 5 chips handling the possible overlaps, the 
>> different chip sizes, which might be bigger than the maximum 
>> window size, and the HW bugs. AST2500 SPI controller has a bug 
>> when the CE0 chip size exceeds 120MB.
> 
> Yeah, I appreciate it's a finicky problem.
> 
>>
>> Also the controller only really makes use of them in command 
>> mode. In user mode, the start address is mostly used to 
>> identify the chip when writing commands on the AHB Bus.    
>>
>>> Cédric: Have you looked into supporting configuration of the segment
>>> mapping?
>>
>> yes. QEMU (openbmc and mainline) supports them already. 
>>
>> As for the kernel, here is an initial commit :
>>
>>    https://github.com/legoater/linux/commit/c08765882b4091cb1b9998d872415c072ecfe8af
>>
>> which I will send with a bunch of improvements when we switch 
>> to linux 4.10. I can eventually rebase on 4.7 but I would have
>> preferred not maintaining too much branches.
> 
> That's fair enough. If we apply Lei's patch to 4.7 as a hack it means
> that we can ignore the issue from then on. Any new systems will be
> based on at least 4.10, which should have your improvements.
>
>>
>>> 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?
>>
>> In the aspeed-smc bindings, the mapping range is for the overall 
>> AHB window. This is a HW constant you can not change. 
>>
>> As for the settings of the each chip window, I would let the 
>> driver discover and configure them as you can ruin the controller 
>> behavior in command mode if they are wrong. 
> 
> I wasn't concerned about the overall AHB window so much as the chip
> windows. Letting the driver discover and configure them was what I was
> hoping for, and was investigating providing the information via the DT.

Well, we possibly could but given that we can change the 
chip (and possibly their size), it is better to try to 
detect them I think.

> If we have an alternative approach (querying the chip?) that would be
> great.

That is what my branch is trying to do but it still needs 
a couple of improvements. All segments should be configured, 
even those for which there are no chips. Lei just reminded 
me of that problem.    

>> What is the specific need ? Is it to have a window covering all 
>> the SPI CEx chips ? 
> 
> In this case it was to have CE0 window cover 64MB rather than 32MB.
> Supporting more than one chip would be a nice-to-have rather than
> something Lei needs as far as I understand.

There is still that segment overlap problem to cover even if
there are no chips.

>> If so, my tree should have that support 
>> already. But, please note that the HW is broken for chips > 128MB, 
>> on witherspoon for instance.
>>
> 
> I guess the question is about what support is going to be in which
> branch of openbmc/linux. As it stands it looks like dev-4.7 will not
> have chip window configuration support in the driver, and dev-4.10
> will.

Unless we start using what's in my branch which has a backport 
of the 4.11 driver on openbmc 4.7. It should be fine I think. 
I just need to cover the above case, moving the start address 
of the next segment should be it, plus a couple of checks.

Cheers,

C.