[U-Boot] [PATCH 2/5] arm: socfpga: move SDR reset handling to driver

[Marek Vasut]

On 1/29/19 9:23 AM, Simon Goldschmidt wrote:
> On Mon, Jan 28, 2019 at 8:25 PM Marek Vasut <marex at denx.de> wrote:
>>
>> On 1/28/19 8:17 PM, Simon Goldschmidt wrote:
>>> Am 28.01.2019 um 20:02 schrieb Marek Vasut:
>>>> On 1/28/19 1:38 PM, Simon Goldschmidt wrote:
>>>>> On Mon, Jan 28, 2019 at 12:59 PM Marek Vasut <marex at denx.de> wrote:
>>>>>>
>>>>>> On 1/28/19 12:49 PM, Simon Goldschmidt wrote:
>>>>>>> On Mon, Jan 28, 2019 at 12:30 PM Marek Vasut <marex at denx.de> wrote:
>>>>>>>>
>>>>>>>> On 1/27/19 9:47 AM, Simon Goldschmidt wrote:
>>>>>>>>> Am 26.01.2019 um 09:58 schrieb Marek Vasut:
>>>>>>>>>> On 1/25/19 9:30 PM, Simon Goldschmidt wrote:
>>>>>>>>>>> To clean up reset handling for socfpga gen5, let's move the
>>>>>>>>>>> code snippet
>>>>>>>>>>> taking the DDR controller out of reset from SPL to the DDR driver.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>>    arch/arm/mach-socfpga/spl_gen5.c | 1 -
>>>>>>>>>>>    drivers/ddr/altera/sdram_gen5.c  | 4 ++++
>>>>>>>>>>>    2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/arch/arm/mach-socfpga/spl_gen5.c
>>>>>>>>>>> b/arch/arm/mach-socfpga/spl_gen5.c
>>>>>>>>>>> index ccdc661d05..f9bea892b1 100644
>>>>>>>>>>> --- a/arch/arm/mach-socfpga/spl_gen5.c
>>>>>>>>>>> +++ b/arch/arm/mach-socfpga/spl_gen5.c
>>>>>>>>>>> @@ -98,7 +98,6 @@ void board_init_f(ulong dummy)
>>>>>>>>>>>            socfpga_bridges_reset(1);
>>>>>>>>>>>        }
>>>>>>>>>>>    -    socfpga_per_reset(SOCFPGA_RESET(SDR), 0);
>>>>>>>>>>>        socfpga_per_reset(SOCFPGA_RESET(UART0), 0);
>>>>>>>>>>>        socfpga_per_reset(SOCFPGA_RESET(OSC1TIMER0), 0);
>>>>>>>>>>>    diff --git a/drivers/ddr/altera/sdram_gen5.c
>>>>>>>>>>> b/drivers/ddr/altera/sdram_gen5.c
>>>>>>>>>>> index 821060459c..bd54c420f8 100644
>>>>>>>>>>> --- a/drivers/ddr/altera/sdram_gen5.c
>>>>>>>>>>> +++ b/drivers/ddr/altera/sdram_gen5.c
>>>>>>>>>>> @@ -7,6 +7,7 @@
>>>>>>>>>>>    #include <div64.h>
>>>>>>>>>>>    #include <watchdog.h>
>>>>>>>>>>>    #include <asm/arch/fpga_manager.h>
>>>>>>>>>>> +#include <asm/arch/reset_manager.h>
>>>>>>>>>>>    #include <asm/arch/sdram.h>
>>>>>>>>>>>    #include <asm/arch/system_manager.h>
>>>>>>>>>>>    #include <asm/io.h>
>>>>>>>>>>> @@ -434,6 +435,9 @@ int sdram_mmr_init_full(unsigned int
>>>>>>>>>>> sdr_phy_reg)
>>>>>>>>>>>                SDR_CTRLGRP_DRAMADDRW_ROWBITS_LSB;
>>>>>>>>>>>        int ret;
>>>>>>>>>>>    +    /* release DDR from reset */
>>>>>>>>>>> +    socfpga_per_reset(SOCFPGA_RESET(SDR), 0);
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>> Can the reset framework do this ?
>>>>>>>>>
>>>>>>>>> Hmm, it probably could, but I see that as a diferent patch. The
>>>>>>>>> altera
>>>>>>>>> DDR driver currently does not work with devicetree, so using the
>>>>>>>>> reset
>>>>>>>>> framework here would be a bigger patch, I think.
>>>>>>>>>
>>>>>>>>> Can we do that later and clean up this by just moving the code?
>>>>>>>>
>>>>>>>> How much effort is it to switch this driver over to DM ?
>>>>>>>
>>>>>>> I really don't know. Searching through drivers/ddr does not seem to
>>>>>>> give me
>>>>>>> an example of a DTS-enabled ddr driver. Given that, I'd rather just
>>>>>>> push this
>>>>>>> patch now. While it's true that it doesn't clean up everything, it's
>>>>>>> not as if it
>>>>>>> would make things worse.
>>>>>>
>>>>>> That's a valid point.
>>>>>>
>>>>>> I guess you can add DRAM uclass and just probe the driver, which should
>>>>>> be all that's needed.
>>>>>
>>>>> Hmm, there *is* a UCLASS_RAM, but its drivers are in 'drivers/ram'.
>>>>> Is there
>>>>> any reason those are separated from 'drivers/ddr'?
>>>>
>>>> I don't think so, seems like these two directories should be merged.
>>>
>>> Yes, I think so too by now.
>>>
>>> I'll see if I can change this patch to use UCLASS_RAM. A patch/series to
>>> merge drivers/ddr wih drivers/ram should be separate.
>>
>> Sounds good.
> 
> It kind of works with UCLASS_RAM, but there's a problem with the devicetree:
> it describes the SDR controller starting at 0xffc25000 where the official memory
> map from Intel says it starts at 0xffc20000, but describes its
> registers starting
> from offset 0x5000. However, in U-Boot, the file
> 'drivers/ddr/altera/sequencer.c'
> uses those lower addresses.
> 
> So now I can either change the dts to let SDR registers start at 0xffc20000
> instead of 0xffc25000 (and in consequence adapt Linux, too) or add a new driver
> for the sequencer that occupies this lower range (and make sdram_gen5.c use
> it somehow).

Fix the DT, I think the DT is buggy. The DRAM controller probably starts
at 0xffc2_0000 and the various 4k chunks above 0xffc2_0000 are various
subcomponents of the DDR controller.

> Before I spin another round, what would be the preferred way to take?
> 
> Anyway, I failed to find public documentation for this sequencer thing. Do you
> happen to know why it's done like this?

I don't have one, but I _think_ it's a HW instance of the altera DDR3
controller which you can also put into the FPGA, and that one is in quartus.

-- 
Best regards,
Marek Vasut