| Message ID | 20190125203051.10943-3-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | arm: socfpga: implement proper peripheral reset handling | expand |
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@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 ? > writel(rows, &sysmgr_regs->iswgrp_handoff[4]); > > sdr_load_regs(cfg); >
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@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? Regards, Simon > >> writel(rows, &sysmgr_regs->iswgrp_handoff[4]); >> >> sdr_load_regs(cfg); >> > >
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@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 ?
On Mon, Jan 28, 2019 at 12:30 PM Marek Vasut <marex@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@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. Regards, Simon
On 1/28/19 12:49 PM, Simon Goldschmidt wrote: > On Mon, Jan 28, 2019 at 12:30 PM Marek Vasut <marex@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@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.
On Mon, Jan 28, 2019 at 12:59 PM Marek Vasut <marex@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@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@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'? Regards, Simon
On 1/28/19 1:38 PM, Simon Goldschmidt wrote: > On Mon, Jan 28, 2019 at 12:59 PM Marek Vasut <marex@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@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@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.
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@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@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@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. Regads, Simon
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@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@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@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.
On Mon, Jan 28, 2019 at 8:25 PM Marek Vasut <marex@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@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@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@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). 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? Regards, Simon
On 1/29/19 9:23 AM, Simon Goldschmidt wrote: > On Mon, Jan 28, 2019 at 8:25 PM Marek Vasut <marex@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@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@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@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.
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); + writel(rows, &sysmgr_regs->iswgrp_handoff[4]); sdr_load_regs(cfg);
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@gmail.com> --- arch/arm/mach-socfpga/spl_gen5.c | 1 - drivers/ddr/altera/sdram_gen5.c | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-)