[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Simon Goldschmidt]

This is an initial attempt to support OF_PLATDATA for socfpga gen5.

There are two motivations for this:
a) reduce code size to eventually support secure boot (where SPL has to
   authenticate the next stage by loading/checking U-Boot from a FIT
   image)
b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
   (on warm-restart), all bytes to check need to be in one piece. With
   OF_SEPARATE, this is not the case (.bss is between .rodata and the
   DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
   be a good solution.

This series enables booting from MMC with OF_PLATDATA. Booting from SPI
does not yet work (I'm working on that, but it's tougher than expected).

Tested on the socrates board. To use it:
- use socfpga_socrates_defconfig
- disable CONFIG_SPL_SPI_FLASH_SUPPORT, CONFIG_SPL_SPI_SUPPORT and
  CONFIG_SPL_DM_RESET (those don't work yet)
- enable CONFIG_DESIGNWARE_SERIAL (required for console)


Simon Goldschmidt (4):
  arm: socfpga: imply SPL config instead of select
  arm: socfpga: fix compiling with OF_PLATDATA
  serial: add an of-platdata driver for "snps,dw-apb-uart"
  mmc: socfpga: support of-platdata

 arch/arm/Kconfig               |  4 ++--
 arch/arm/mach-socfpga/misc.c   |  2 +-
 drivers/mmc/socfpga_dw_mmc.c   | 28 ++++++++++++++++++++---
 drivers/serial/Kconfig         | 10 ++++++++
 drivers/serial/Makefile        |  1 +
 drivers/serial/serial_dw_apb.c | 42 ++++++++++++++++++++++++++++++++++
 6 files changed, 81 insertions(+), 6 deletions(-)
 create mode 100644 drivers/serial/serial_dw_apb.c

-- 
2.17.1

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

In order to build a smaller SPL, let's imply SPL_DM_RESET and
SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
via defconfig.

This also seems to be required to use OF_PLATDATA, as the reset drivers
don't seem to work with it.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

 arch/arm/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 520ea8bed9..1ca71bd323 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -815,7 +815,6 @@ config ARCH_SOCFPGA
 	select DM_SERIAL
 	select ENABLE_ARM_SOC_BOOT0_HOOK if TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10
 	select OF_CONTROL
-	select SPL_DM_RESET if DM_RESET
 	select SPL_DM_SERIAL
 	select SPL_LIBCOMMON_SUPPORT
 	select SPL_LIBGENERIC_SUPPORT
@@ -823,7 +822,6 @@ config ARCH_SOCFPGA
 	select SPL_OF_CONTROL
 	select SPL_SEPARATE_BSS if TARGET_SOCFPGA_STRATIX10
 	select SPL_SERIAL_SUPPORT
-	select SPL_WATCHDOG_SUPPORT
 	select SUPPORT_SPL
 	select SYS_NS16550
 	select SYS_THUMB_BUILD if TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10
@@ -833,12 +831,14 @@ config ARCH_SOCFPGA
 	imply DM_SPI
 	imply DM_SPI_FLASH
 	imply FAT_WRITE
+	imply SPL_DM_RESET
 	imply SPL_LIBDISK_SUPPORT
 	imply SPL_MMC_SUPPORT
 	imply SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION
 	imply SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE
 	imply SPL_SPI_FLASH_SUPPORT
 	imply SPL_SPI_SUPPORT
+	imply SPL_WATCHDOG_SUPPORT
 
 config ARCH_SUNXI
 	bool "Support sunxi (Allwinner) SoCs"
-- 
2.17.1

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> In order to build a smaller SPL, let's imply SPL_DM_RESET and
> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
> via defconfig.
> 
> This also seems to be required to use OF_PLATDATA, as the reset drivers
> don't seem to work with it.

How do you un-reset IP blocks if you disable the reset controller ?

> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
> 
>  arch/arm/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 520ea8bed9..1ca71bd323 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -815,7 +815,6 @@ config ARCH_SOCFPGA
>  	select DM_SERIAL
>  	select ENABLE_ARM_SOC_BOOT0_HOOK if TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10
>  	select OF_CONTROL
> -	select SPL_DM_RESET if DM_RESET
>  	select SPL_DM_SERIAL
>  	select SPL_LIBCOMMON_SUPPORT
>  	select SPL_LIBGENERIC_SUPPORT
> @@ -823,7 +822,6 @@ config ARCH_SOCFPGA
>  	select SPL_OF_CONTROL
>  	select SPL_SEPARATE_BSS if TARGET_SOCFPGA_STRATIX10
>  	select SPL_SERIAL_SUPPORT
> -	select SPL_WATCHDOG_SUPPORT
>  	select SUPPORT_SPL
>  	select SYS_NS16550
>  	select SYS_THUMB_BUILD if TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10
> @@ -833,12 +831,14 @@ config ARCH_SOCFPGA
>  	imply DM_SPI
>  	imply DM_SPI_FLASH
>  	imply FAT_WRITE
> +	imply SPL_DM_RESET
>  	imply SPL_LIBDISK_SUPPORT
>  	imply SPL_MMC_SUPPORT
>  	imply SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION
>  	imply SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE
>  	imply SPL_SPI_FLASH_SUPPORT
>  	imply SPL_SPI_SUPPORT
> +	imply SPL_WATCHDOG_SUPPORT
>  
>  config ARCH_SUNXI
>  	bool "Support sunxi (Allwinner) SoCs"
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> > In order to build a smaller SPL, let's imply SPL_DM_RESET and
> > SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
> > via defconfig.
> >
> > This also seems to be required to use OF_PLATDATA, as the reset drivers
> > don't seem to work with it.
>
> How do you un-reset IP blocks if you disable the reset controller ?

Here again, socfpga seems to be another bad example. Taking
peripherals out of reset
is cluttered throughout the mach-socfpga code at least in SPL. By now
I know socfpga is
lacking support for clock and reset management via devicetree. And
this is bad, I know,
but can we keep this a seperate issue from OF_PLATDATA?

That being said, drivers/reset/reset-uclass.c fails to compile with
OF_PLATDATA, so I
guess this has not been used with OF_PLATDATA before. And given that I
don't seem
to need it for socfpga either, I don't think this would be the right
series to fix that.

Regards,
Simon

>
> > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > ---
> >
> >  arch/arm/Kconfig | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 520ea8bed9..1ca71bd323 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -815,7 +815,6 @@ config ARCH_SOCFPGA
> >       select DM_SERIAL
> >       select ENABLE_ARM_SOC_BOOT0_HOOK if TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10
> >       select OF_CONTROL
> > -     select SPL_DM_RESET if DM_RESET
> >       select SPL_DM_SERIAL
> >       select SPL_LIBCOMMON_SUPPORT
> >       select SPL_LIBGENERIC_SUPPORT
> > @@ -823,7 +822,6 @@ config ARCH_SOCFPGA
> >       select SPL_OF_CONTROL
> >       select SPL_SEPARATE_BSS if TARGET_SOCFPGA_STRATIX10
> >       select SPL_SERIAL_SUPPORT
> > -     select SPL_WATCHDOG_SUPPORT
> >       select SUPPORT_SPL
> >       select SYS_NS16550
> >       select SYS_THUMB_BUILD if TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10
> > @@ -833,12 +831,14 @@ config ARCH_SOCFPGA
> >       imply DM_SPI
> >       imply DM_SPI_FLASH
> >       imply FAT_WRITE
> > +     imply SPL_DM_RESET
> >       imply SPL_LIBDISK_SUPPORT
> >       imply SPL_MMC_SUPPORT
> >       imply SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION
> >       imply SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE
> >       imply SPL_SPI_FLASH_SUPPORT
> >       imply SPL_SPI_SUPPORT
> > +     imply SPL_WATCHDOG_SUPPORT
> >
> >  config ARCH_SUNXI
> >       bool "Support sunxi (Allwinner) SoCs"
> >
>
>
> --
> Best regards,
> Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
>>> via defconfig.
>>>
>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
>>> don't seem to work with it.
>>
>> How do you un-reset IP blocks if you disable the reset controller ?
> 
> Here again, socfpga seems to be another bad example. Taking
> peripherals out of reset
> is cluttered throughout the mach-socfpga code at least in SPL. By now
> I know socfpga is
> lacking support for clock and reset management via devicetree. And
> this is bad, I know,
> but can we keep this a seperate issue from OF_PLATDATA?
> 
> That being said, drivers/reset/reset-uclass.c fails to compile with
> OF_PLATDATA, so I
> guess this has not been used with OF_PLATDATA before. And given that I
> don't seem
> to need it for socfpga either, I don't think this would be the right
> series to fix that.

Don't you need it to unreset at least the DWMMC or CQSPI ?
Anyway, I'd much prefer to start cleaning up the horrorshow that
arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
Would that be possible ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
> > On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
> >>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
> >>> via defconfig.
> >>>
> >>> This also seems to be required to use OF_PLATDATA, as the reset drivers
> >>> don't seem to work with it.
> >>
> >> How do you un-reset IP blocks if you disable the reset controller ?
> >
> > Here again, socfpga seems to be another bad example. Taking
> > peripherals out of reset
> > is cluttered throughout the mach-socfpga code at least in SPL. By now
> > I know socfpga is
> > lacking support for clock and reset management via devicetree. And
> > this is bad, I know,
> > but can we keep this a seperate issue from OF_PLATDATA?
> >
> > That being said, drivers/reset/reset-uclass.c fails to compile with
> > OF_PLATDATA, so I
> > guess this has not been used with OF_PLATDATA before. And given that I
> > don't seem
> > to need it for socfpga either, I don't think this would be the right
> > series to fix that.
>
> Don't you need it to unreset at least the DWMMC or CQSPI ?

Reading the code, it seems like that's taken care of through another hack in
spl_boot_device() ;-)

> Anyway, I'd much prefer to start cleaning up the horrorshow that
> arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
> Would that be possible ?

I would be best, yes. I don't know when I will find the time to do that, though.
I don't know how much effort that would be, either. Is there maybe a patch
where A10 got converted from "as bad as gen5" to its current state? That
would help me to see if I can do it...

Regards,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/8/19 1:09 PM, Simon Goldschmidt wrote:
> On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
>>>>> via defconfig.
>>>>>
>>>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
>>>>> don't seem to work with it.
>>>>
>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>
>>> Here again, socfpga seems to be another bad example. Taking
>>> peripherals out of reset
>>> is cluttered throughout the mach-socfpga code at least in SPL. By now
>>> I know socfpga is
>>> lacking support for clock and reset management via devicetree. And
>>> this is bad, I know,
>>> but can we keep this a seperate issue from OF_PLATDATA?
>>>
>>> That being said, drivers/reset/reset-uclass.c fails to compile with
>>> OF_PLATDATA, so I
>>> guess this has not been used with OF_PLATDATA before. And given that I
>>> don't seem
>>> to need it for socfpga either, I don't think this would be the right
>>> series to fix that.
>>
>> Don't you need it to unreset at least the DWMMC or CQSPI ?
> 
> Reading the code, it seems like that's taken care of through another hack in
> spl_boot_device() ;-)

Sigh.

>> Anyway, I'd much prefer to start cleaning up the horrorshow that
>> arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
>> Would that be possible ?
> 
> I would be best, yes. I don't know when I will find the time to do that, though.
> I don't know how much effort that would be, either. Is there maybe a patch
> where A10 got converted from "as bad as gen5" to its current state? That
> would help me to see if I can do it...

A10 got switched to reset framework recently (in last 6 months or so),
the reset driver is the same for Gen5 and A10 too, so it should be easy
to recycle.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

On Tue, Jan 8, 2019 at 1:22 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/8/19 1:09 PM, Simon Goldschmidt wrote:
> > On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
> >>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
> >>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
> >>>>> via defconfig.
> >>>>>
> >>>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
> >>>>> don't seem to work with it.
> >>>>
> >>>> How do you un-reset IP blocks if you disable the reset controller ?
> >>>
> >>> Here again, socfpga seems to be another bad example. Taking
> >>> peripherals out of reset
> >>> is cluttered throughout the mach-socfpga code at least in SPL. By now
> >>> I know socfpga is
> >>> lacking support for clock and reset management via devicetree. And
> >>> this is bad, I know,
> >>> but can we keep this a seperate issue from OF_PLATDATA?
> >>>
> >>> That being said, drivers/reset/reset-uclass.c fails to compile with
> >>> OF_PLATDATA, so I
> >>> guess this has not been used with OF_PLATDATA before. And given that I
> >>> don't seem
> >>> to need it for socfpga either, I don't think this would be the right
> >>> series to fix that.
> >>
> >> Don't you need it to unreset at least the DWMMC or CQSPI ?
> >
> > Reading the code, it seems like that's taken care of through another hack in
> > spl_boot_device() ;-)
>
> Sigh.
>
> >> Anyway, I'd much prefer to start cleaning up the horrorshow that
> >> arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
> >> Would that be possible ?
> >
> > I would be best, yes. I don't know when I will find the time to do that, though.
> > I don't know how much effort that would be, either. Is there maybe a patch
> > where A10 got converted from "as bad as gen5" to its current state? That
> > would help me to see if I can do it...
>
> A10 got switched to reset framework recently (in last 6 months or so),
> the reset driver is the same for Gen5 and A10 too, so it should be easy
> to recycle.

Hmm, ok, let me check that... it would indeed be nice to port this to gen5.

Since you seem kidn of opposed to OF_PLATDATA, does it make any sense
to continue on this? I mean, I thought I heard people here saying "use
OF_PLATDATA" if you're running out of space in SPL. After using it, I'm not too
keen on using it, either, but it does seem to give me some code space back...

Regards,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/8/19 1:46 PM, Simon Goldschmidt wrote:
> On Tue, Jan 8, 2019 at 1:22 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/8/19 1:09 PM, Simon Goldschmidt wrote:
>>> On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
>>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
>>>>>>> via defconfig.
>>>>>>>
>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
>>>>>>> don't seem to work with it.
>>>>>>
>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>
>>>>> Here again, socfpga seems to be another bad example. Taking
>>>>> peripherals out of reset
>>>>> is cluttered throughout the mach-socfpga code at least in SPL. By now
>>>>> I know socfpga is
>>>>> lacking support for clock and reset management via devicetree. And
>>>>> this is bad, I know,
>>>>> but can we keep this a seperate issue from OF_PLATDATA?
>>>>>
>>>>> That being said, drivers/reset/reset-uclass.c fails to compile with
>>>>> OF_PLATDATA, so I
>>>>> guess this has not been used with OF_PLATDATA before. And given that I
>>>>> don't seem
>>>>> to need it for socfpga either, I don't think this would be the right
>>>>> series to fix that.
>>>>
>>>> Don't you need it to unreset at least the DWMMC or CQSPI ?
>>>
>>> Reading the code, it seems like that's taken care of through another hack in
>>> spl_boot_device() ;-)
>>
>> Sigh.
>>
>>>> Anyway, I'd much prefer to start cleaning up the horrorshow that
>>>> arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
>>>> Would that be possible ?
>>>
>>> I would be best, yes. I don't know when I will find the time to do that, though.
>>> I don't know how much effort that would be, either. Is there maybe a patch
>>> where A10 got converted from "as bad as gen5" to its current state? That
>>> would help me to see if I can do it...
>>
>> A10 got switched to reset framework recently (in last 6 months or so),
>> the reset driver is the same for Gen5 and A10 too, so it should be easy
>> to recycle.
> 
> Hmm, ok, let me check that... it would indeed be nice to port this to gen5.
> 
> Since you seem kidn of opposed to OF_PLATDATA, does it make any sense
> to continue on this? I mean, I thought I heard people here saying "use
> OF_PLATDATA" if you're running out of space in SPL. After using it, I'm not too
> keen on using it, either, but it does seem to give me some code space back...

OF_PLATDATA is for platforms with really small SRAM, some 30k or below.
This platform has a massive 60k of SRAM for SPL, so if we're running out
of space, we're doing something wrong.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Tom Rini]

On Tue, Jan 08, 2019 at 01:49:14PM +0100, Marek Vasut wrote:
> On 1/8/19 1:46 PM, Simon Goldschmidt wrote:
> > On Tue, Jan 8, 2019 at 1:22 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 1/8/19 1:09 PM, Simon Goldschmidt wrote:
> >>> On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
> >>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
> >>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
> >>>>>>> via defconfig.
> >>>>>>>
> >>>>>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
> >>>>>>> don't seem to work with it.
> >>>>>>
> >>>>>> How do you un-reset IP blocks if you disable the reset controller ?
> >>>>>
> >>>>> Here again, socfpga seems to be another bad example. Taking
> >>>>> peripherals out of reset
> >>>>> is cluttered throughout the mach-socfpga code at least in SPL. By now
> >>>>> I know socfpga is
> >>>>> lacking support for clock and reset management via devicetree. And
> >>>>> this is bad, I know,
> >>>>> but can we keep this a seperate issue from OF_PLATDATA?
> >>>>>
> >>>>> That being said, drivers/reset/reset-uclass.c fails to compile with
> >>>>> OF_PLATDATA, so I
> >>>>> guess this has not been used with OF_PLATDATA before. And given that I
> >>>>> don't seem
> >>>>> to need it for socfpga either, I don't think this would be the right
> >>>>> series to fix that.
> >>>>
> >>>> Don't you need it to unreset at least the DWMMC or CQSPI ?
> >>>
> >>> Reading the code, it seems like that's taken care of through another hack in
> >>> spl_boot_device() ;-)
> >>
> >> Sigh.
> >>
> >>>> Anyway, I'd much prefer to start cleaning up the horrorshow that
> >>>> arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
> >>>> Would that be possible ?
> >>>
> >>> I would be best, yes. I don't know when I will find the time to do that, though.
> >>> I don't know how much effort that would be, either. Is there maybe a patch
> >>> where A10 got converted from "as bad as gen5" to its current state? That
> >>> would help me to see if I can do it...
> >>
> >> A10 got switched to reset framework recently (in last 6 months or so),
> >> the reset driver is the same for Gen5 and A10 too, so it should be easy
> >> to recycle.
> > 
> > Hmm, ok, let me check that... it would indeed be nice to port this to gen5.
> > 
> > Since you seem kidn of opposed to OF_PLATDATA, does it make any sense
> > to continue on this? I mean, I thought I heard people here saying "use
> > OF_PLATDATA" if you're running out of space in SPL. After using it, I'm not too
> > keen on using it, either, but it does seem to give me some code space back...
> 
> OF_PLATDATA is for platforms with really small SRAM, some 30k or below.
> This platform has a massive 60k of SRAM for SPL, so if we're running out
> of space, we're doing something wrong.

It's not for "30k or below" but "needs more space to enable all desired
features inside of SPL".

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190108/fa70070e/attachment.sig>

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/8/19 3:48 PM, Tom Rini wrote:
> On Tue, Jan 08, 2019 at 01:49:14PM +0100, Marek Vasut wrote:
>> On 1/8/19 1:46 PM, Simon Goldschmidt wrote:
>>> On Tue, Jan 8, 2019 at 1:22 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 1/8/19 1:09 PM, Simon Goldschmidt wrote:
>>>>> On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
>>>>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
>>>>>>>>> via defconfig.
>>>>>>>>>
>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
>>>>>>>>> don't seem to work with it.
>>>>>>>>
>>>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>>>
>>>>>>> Here again, socfpga seems to be another bad example. Taking
>>>>>>> peripherals out of reset
>>>>>>> is cluttered throughout the mach-socfpga code at least in SPL. By now
>>>>>>> I know socfpga is
>>>>>>> lacking support for clock and reset management via devicetree. And
>>>>>>> this is bad, I know,
>>>>>>> but can we keep this a seperate issue from OF_PLATDATA?
>>>>>>>
>>>>>>> That being said, drivers/reset/reset-uclass.c fails to compile with
>>>>>>> OF_PLATDATA, so I
>>>>>>> guess this has not been used with OF_PLATDATA before. And given that I
>>>>>>> don't seem
>>>>>>> to need it for socfpga either, I don't think this would be the right
>>>>>>> series to fix that.
>>>>>>
>>>>>> Don't you need it to unreset at least the DWMMC or CQSPI ?
>>>>>
>>>>> Reading the code, it seems like that's taken care of through another hack in
>>>>> spl_boot_device() ;-)
>>>>
>>>> Sigh.
>>>>
>>>>>> Anyway, I'd much prefer to start cleaning up the horrorshow that
>>>>>> arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
>>>>>> Would that be possible ?
>>>>>
>>>>> I would be best, yes. I don't know when I will find the time to do that, though.
>>>>> I don't know how much effort that would be, either. Is there maybe a patch
>>>>> where A10 got converted from "as bad as gen5" to its current state? That
>>>>> would help me to see if I can do it...
>>>>
>>>> A10 got switched to reset framework recently (in last 6 months or so),
>>>> the reset driver is the same for Gen5 and A10 too, so it should be easy
>>>> to recycle.
>>>
>>> Hmm, ok, let me check that... it would indeed be nice to port this to gen5.
>>>
>>> Since you seem kidn of opposed to OF_PLATDATA, does it make any sense
>>> to continue on this? I mean, I thought I heard people here saying "use
>>> OF_PLATDATA" if you're running out of space in SPL. After using it, I'm not too
>>> keen on using it, either, but it does seem to give me some code space back...
>>
>> OF_PLATDATA is for platforms with really small SRAM, some 30k or below.
>> This platform has a massive 60k of SRAM for SPL, so if we're running out
>> of space, we're doing something wrong.
> 
> It's not for "30k or below" but "needs more space to enable all desired
> features inside of SPL".

Which the SoCFPGA should have with 60k of SRAM. If U-Boot SPL became so
bloated that even platform with so much space has issues, how can we
even cater for the rest of platforms with much more limited SPL ? And if
that is the case, we have a much bigger problem ...

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Tom Rini]

On Tue, Jan 08, 2019 at 03:50:48PM +0100, Marek Vasut wrote:
> On 1/8/19 3:48 PM, Tom Rini wrote:
> > On Tue, Jan 08, 2019 at 01:49:14PM +0100, Marek Vasut wrote:
> >> On 1/8/19 1:46 PM, Simon Goldschmidt wrote:
> >>> On Tue, Jan 8, 2019 at 1:22 PM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 1/8/19 1:09 PM, Simon Goldschmidt wrote:
> >>>>> On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>> On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
> >>>>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>
> >>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
> >>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
> >>>>>>>>> via defconfig.
> >>>>>>>>>
> >>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
> >>>>>>>>> don't seem to work with it.
> >>>>>>>>
> >>>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
> >>>>>>>
> >>>>>>> Here again, socfpga seems to be another bad example. Taking
> >>>>>>> peripherals out of reset
> >>>>>>> is cluttered throughout the mach-socfpga code at least in SPL. By now
> >>>>>>> I know socfpga is
> >>>>>>> lacking support for clock and reset management via devicetree. And
> >>>>>>> this is bad, I know,
> >>>>>>> but can we keep this a seperate issue from OF_PLATDATA?
> >>>>>>>
> >>>>>>> That being said, drivers/reset/reset-uclass.c fails to compile with
> >>>>>>> OF_PLATDATA, so I
> >>>>>>> guess this has not been used with OF_PLATDATA before. And given that I
> >>>>>>> don't seem
> >>>>>>> to need it for socfpga either, I don't think this would be the right
> >>>>>>> series to fix that.
> >>>>>>
> >>>>>> Don't you need it to unreset at least the DWMMC or CQSPI ?
> >>>>>
> >>>>> Reading the code, it seems like that's taken care of through another hack in
> >>>>> spl_boot_device() ;-)
> >>>>
> >>>> Sigh.
> >>>>
> >>>>>> Anyway, I'd much prefer to start cleaning up the horrorshow that
> >>>>>> arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
> >>>>>> Would that be possible ?
> >>>>>
> >>>>> I would be best, yes. I don't know when I will find the time to do that, though.
> >>>>> I don't know how much effort that would be, either. Is there maybe a patch
> >>>>> where A10 got converted from "as bad as gen5" to its current state? That
> >>>>> would help me to see if I can do it...
> >>>>
> >>>> A10 got switched to reset framework recently (in last 6 months or so),
> >>>> the reset driver is the same for Gen5 and A10 too, so it should be easy
> >>>> to recycle.
> >>>
> >>> Hmm, ok, let me check that... it would indeed be nice to port this to gen5.
> >>>
> >>> Since you seem kidn of opposed to OF_PLATDATA, does it make any sense
> >>> to continue on this? I mean, I thought I heard people here saying "use
> >>> OF_PLATDATA" if you're running out of space in SPL. After using it, I'm not too
> >>> keen on using it, either, but it does seem to give me some code space back...
> >>
> >> OF_PLATDATA is for platforms with really small SRAM, some 30k or below.
> >> This platform has a massive 60k of SRAM for SPL, so if we're running out
> >> of space, we're doing something wrong.
> > 
> > It's not for "30k or below" but "needs more space to enable all desired
> > features inside of SPL".
> 
> Which the SoCFPGA should have with 60k of SRAM. If U-Boot SPL became so
> bloated that even platform with so much space has issues, how can we
> even cater for the rest of platforms with much more limited SPL ? And if
> that is the case, we have a much bigger problem ...

It depends, greatly, on what features you want within a single binary.
I'm not saying SoCFPGA can't fit what it wants, including verified boot,
inside of 60k.  But what I am saying is we don't have a hard-and-fast
limit on when you must not use OF_PLATDATA since it's always been easy
to make SPL too big, once you start including all of the possible
kitchen sink options (lets do falcon mode, and boot count and usb gadget
and usb host and regular ethernet and mmc and nand and oh crap, where
did all of my space go?).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190108/c17b9ec2/attachment.sig>

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Am 08.01.2019 um 15:58 schrieb Tom Rini:
> On Tue, Jan 08, 2019 at 03:50:48PM +0100, Marek Vasut wrote:
>> On 1/8/19 3:48 PM, Tom Rini wrote:
>>> On Tue, Jan 08, 2019 at 01:49:14PM +0100, Marek Vasut wrote:
>>>> On 1/8/19 1:46 PM, Simon Goldschmidt wrote:
>>>>> On Tue, Jan 8, 2019 at 1:22 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 1/8/19 1:09 PM, Simon Goldschmidt wrote:
>>>>>>> On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
>>>>>>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>
>>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
>>>>>>>>>>> via defconfig.
>>>>>>>>>>>
>>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
>>>>>>>>>>> don't seem to work with it.
>>>>>>>>>>
>>>>>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>>>>>
>>>>>>>>> Here again, socfpga seems to be another bad example. Taking
>>>>>>>>> peripherals out of reset
>>>>>>>>> is cluttered throughout the mach-socfpga code at least in SPL. By now
>>>>>>>>> I know socfpga is
>>>>>>>>> lacking support for clock and reset management via devicetree. And
>>>>>>>>> this is bad, I know,
>>>>>>>>> but can we keep this a seperate issue from OF_PLATDATA?
>>>>>>>>>
>>>>>>>>> That being said, drivers/reset/reset-uclass.c fails to compile with
>>>>>>>>> OF_PLATDATA, so I
>>>>>>>>> guess this has not been used with OF_PLATDATA before. And given that I
>>>>>>>>> don't seem
>>>>>>>>> to need it for socfpga either, I don't think this would be the right
>>>>>>>>> series to fix that.
>>>>>>>>
>>>>>>>> Don't you need it to unreset at least the DWMMC or CQSPI ?
>>>>>>>
>>>>>>> Reading the code, it seems like that's taken care of through another hack in
>>>>>>> spl_boot_device() ;-)
>>>>>>
>>>>>> Sigh.
>>>>>>
>>>>>>>> Anyway, I'd much prefer to start cleaning up the horrorshow that
>>>>>>>> arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
>>>>>>>> Would that be possible ?
>>>>>>>
>>>>>>> I would be best, yes. I don't know when I will find the time to do that, though.
>>>>>>> I don't know how much effort that would be, either. Is there maybe a patch
>>>>>>> where A10 got converted from "as bad as gen5" to its current state? That
>>>>>>> would help me to see if I can do it...
>>>>>>
>>>>>> A10 got switched to reset framework recently (in last 6 months or so),
>>>>>> the reset driver is the same for Gen5 and A10 too, so it should be easy
>>>>>> to recycle.
>>>>>
>>>>> Hmm, ok, let me check that... it would indeed be nice to port this to gen5.
>>>>>
>>>>> Since you seem kidn of opposed to OF_PLATDATA, does it make any sense
>>>>> to continue on this? I mean, I thought I heard people here saying "use
>>>>> OF_PLATDATA" if you're running out of space in SPL. After using it, I'm not too
>>>>> keen on using it, either, but it does seem to give me some code space back...
>>>>
>>>> OF_PLATDATA is for platforms with really small SRAM, some 30k or below.
>>>> This platform has a massive 60k of SRAM for SPL, so if we're running out
>>>> of space, we're doing something wrong.
>>>
>>> It's not for "30k or below" but "needs more space to enable all desired
>>> features inside of SPL".
>>
>> Which the SoCFPGA should have with 60k of SRAM. If U-Boot SPL became so
>> bloated that even platform with so much space has issues, how can we
>> even cater for the rest of platforms with much more limited SPL ? And if
>> that is the case, we have a much bigger problem ...
> 
> It depends, greatly, on what features you want within a single binary.
> I'm not saying SoCFPGA can't fit what it wants, including verified boot,
> inside of 60k.  But what I am saying is we don't have a hard-and-fast
> limit on when you must not use OF_PLATDATA since it's always been easy
> to make SPL too big, once you start including all of the possible
> kitchen sink options (lets do falcon mode, and boot count and usb gadget
> and usb host and regular ethernet and mmc and nand and oh crap, where
> did all of my space go?).

I'm not saying this was directed to me (I'm sure it wasn't). Just to 
clarify my point: I'm really just trying to get the most basic SPL to 
work that loads U-Boot as FIT from spi-flash and verifies it. It might 
well be that it's this verified FIT offset that could be reduced...

Regards,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Tom Rini]

On Tue, Jan 08, 2019 at 04:04:12PM +0100, Simon Goldschmidt wrote:
> Am 08.01.2019 um 15:58 schrieb Tom Rini:
> >On Tue, Jan 08, 2019 at 03:50:48PM +0100, Marek Vasut wrote:
> >>On 1/8/19 3:48 PM, Tom Rini wrote:
> >>>On Tue, Jan 08, 2019 at 01:49:14PM +0100, Marek Vasut wrote:
> >>>>On 1/8/19 1:46 PM, Simon Goldschmidt wrote:
> >>>>>On Tue, Jan 8, 2019 at 1:22 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>>On 1/8/19 1:09 PM, Simon Goldschmidt wrote:
> >>>>>>>On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>
> >>>>>>>>On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
> >>>>>>>>>On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>>
> >>>>>>>>>>On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>>>>>>>>>>In order to build a smaller SPL, let's imply SPL_DM_RESET and
> >>>>>>>>>>>SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
> >>>>>>>>>>>via defconfig.
> >>>>>>>>>>>
> >>>>>>>>>>>This also seems to be required to use OF_PLATDATA, as the reset drivers
> >>>>>>>>>>>don't seem to work with it.
> >>>>>>>>>>
> >>>>>>>>>>How do you un-reset IP blocks if you disable the reset controller ?
> >>>>>>>>>
> >>>>>>>>>Here again, socfpga seems to be another bad example. Taking
> >>>>>>>>>peripherals out of reset
> >>>>>>>>>is cluttered throughout the mach-socfpga code at least in SPL. By now
> >>>>>>>>>I know socfpga is
> >>>>>>>>>lacking support for clock and reset management via devicetree. And
> >>>>>>>>>this is bad, I know,
> >>>>>>>>>but can we keep this a seperate issue from OF_PLATDATA?
> >>>>>>>>>
> >>>>>>>>>That being said, drivers/reset/reset-uclass.c fails to compile with
> >>>>>>>>>OF_PLATDATA, so I
> >>>>>>>>>guess this has not been used with OF_PLATDATA before. And given that I
> >>>>>>>>>don't seem
> >>>>>>>>>to need it for socfpga either, I don't think this would be the right
> >>>>>>>>>series to fix that.
> >>>>>>>>
> >>>>>>>>Don't you need it to unreset at least the DWMMC or CQSPI ?
> >>>>>>>
> >>>>>>>Reading the code, it seems like that's taken care of through another hack in
> >>>>>>>spl_boot_device() ;-)
> >>>>>>
> >>>>>>Sigh.
> >>>>>>
> >>>>>>>>Anyway, I'd much prefer to start cleaning up the horrorshow that
> >>>>>>>>arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
> >>>>>>>>Would that be possible ?
> >>>>>>>
> >>>>>>>I would be best, yes. I don't know when I will find the time to do that, though.
> >>>>>>>I don't know how much effort that would be, either. Is there maybe a patch
> >>>>>>>where A10 got converted from "as bad as gen5" to its current state? That
> >>>>>>>would help me to see if I can do it...
> >>>>>>
> >>>>>>A10 got switched to reset framework recently (in last 6 months or so),
> >>>>>>the reset driver is the same for Gen5 and A10 too, so it should be easy
> >>>>>>to recycle.
> >>>>>
> >>>>>Hmm, ok, let me check that... it would indeed be nice to port this to gen5.
> >>>>>
> >>>>>Since you seem kidn of opposed to OF_PLATDATA, does it make any sense
> >>>>>to continue on this? I mean, I thought I heard people here saying "use
> >>>>>OF_PLATDATA" if you're running out of space in SPL. After using it, I'm not too
> >>>>>keen on using it, either, but it does seem to give me some code space back...
> >>>>
> >>>>OF_PLATDATA is for platforms with really small SRAM, some 30k or below.
> >>>>This platform has a massive 60k of SRAM for SPL, so if we're running out
> >>>>of space, we're doing something wrong.
> >>>
> >>>It's not for "30k or below" but "needs more space to enable all desired
> >>>features inside of SPL".
> >>
> >>Which the SoCFPGA should have with 60k of SRAM. If U-Boot SPL became so
> >>bloated that even platform with so much space has issues, how can we
> >>even cater for the rest of platforms with much more limited SPL ? And if
> >>that is the case, we have a much bigger problem ...
> >
> >It depends, greatly, on what features you want within a single binary.
> >I'm not saying SoCFPGA can't fit what it wants, including verified boot,
> >inside of 60k.  But what I am saying is we don't have a hard-and-fast
> >limit on when you must not use OF_PLATDATA since it's always been easy
> >to make SPL too big, once you start including all of the possible
> >kitchen sink options (lets do falcon mode, and boot count and usb gadget
> >and usb host and regular ethernet and mmc and nand and oh crap, where
> >did all of my space go?).
> 
> I'm not saying this was directed to me (I'm sure it wasn't). Just to clarify
> my point: I'm really just trying to get the most basic SPL to work that
> loads U-Boot as FIT from spi-flash and verifies it. It might well be that
> it's this verified FIT offset that could be reduced...

Oh no, I'm just listing the worst case of am335x, which is my fault, and
goes well over the generous 100k+ that we have available there.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190108/22b242a0/attachment.sig>

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/8/19 3:58 PM, Tom Rini wrote:
> On Tue, Jan 08, 2019 at 03:50:48PM +0100, Marek Vasut wrote:
>> On 1/8/19 3:48 PM, Tom Rini wrote:
>>> On Tue, Jan 08, 2019 at 01:49:14PM +0100, Marek Vasut wrote:
>>>> On 1/8/19 1:46 PM, Simon Goldschmidt wrote:
>>>>> On Tue, Jan 8, 2019 at 1:22 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 1/8/19 1:09 PM, Simon Goldschmidt wrote:
>>>>>>> On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
>>>>>>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>
>>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
>>>>>>>>>>> via defconfig.
>>>>>>>>>>>
>>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
>>>>>>>>>>> don't seem to work with it.
>>>>>>>>>>
>>>>>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>>>>>
>>>>>>>>> Here again, socfpga seems to be another bad example. Taking
>>>>>>>>> peripherals out of reset
>>>>>>>>> is cluttered throughout the mach-socfpga code at least in SPL. By now
>>>>>>>>> I know socfpga is
>>>>>>>>> lacking support for clock and reset management via devicetree. And
>>>>>>>>> this is bad, I know,
>>>>>>>>> but can we keep this a seperate issue from OF_PLATDATA?
>>>>>>>>>
>>>>>>>>> That being said, drivers/reset/reset-uclass.c fails to compile with
>>>>>>>>> OF_PLATDATA, so I
>>>>>>>>> guess this has not been used with OF_PLATDATA before. And given that I
>>>>>>>>> don't seem
>>>>>>>>> to need it for socfpga either, I don't think this would be the right
>>>>>>>>> series to fix that.
>>>>>>>>
>>>>>>>> Don't you need it to unreset at least the DWMMC or CQSPI ?
>>>>>>>
>>>>>>> Reading the code, it seems like that's taken care of through another hack in
>>>>>>> spl_boot_device() ;-)
>>>>>>
>>>>>> Sigh.
>>>>>>
>>>>>>>> Anyway, I'd much prefer to start cleaning up the horrorshow that
>>>>>>>> arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
>>>>>>>> Would that be possible ?
>>>>>>>
>>>>>>> I would be best, yes. I don't know when I will find the time to do that, though.
>>>>>>> I don't know how much effort that would be, either. Is there maybe a patch
>>>>>>> where A10 got converted from "as bad as gen5" to its current state? That
>>>>>>> would help me to see if I can do it...
>>>>>>
>>>>>> A10 got switched to reset framework recently (in last 6 months or so),
>>>>>> the reset driver is the same for Gen5 and A10 too, so it should be easy
>>>>>> to recycle.
>>>>>
>>>>> Hmm, ok, let me check that... it would indeed be nice to port this to gen5.
>>>>>
>>>>> Since you seem kidn of opposed to OF_PLATDATA, does it make any sense
>>>>> to continue on this? I mean, I thought I heard people here saying "use
>>>>> OF_PLATDATA" if you're running out of space in SPL. After using it, I'm not too
>>>>> keen on using it, either, but it does seem to give me some code space back...
>>>>
>>>> OF_PLATDATA is for platforms with really small SRAM, some 30k or below.
>>>> This platform has a massive 60k of SRAM for SPL, so if we're running out
>>>> of space, we're doing something wrong.
>>>
>>> It's not for "30k or below" but "needs more space to enable all desired
>>> features inside of SPL".
>>
>> Which the SoCFPGA should have with 60k of SRAM. If U-Boot SPL became so
>> bloated that even platform with so much space has issues, how can we
>> even cater for the rest of platforms with much more limited SPL ? And if
>> that is the case, we have a much bigger problem ...
> 
> It depends, greatly, on what features you want within a single binary.
> I'm not saying SoCFPGA can't fit what it wants, including verified boot,
> inside of 60k.  But what I am saying is we don't have a hard-and-fast
> limit on when you must not use OF_PLATDATA since it's always been easy
> to make SPL too big, once you start including all of the possible
> kitchen sink options (lets do falcon mode, and boot count and usb gadget
> and usb host and regular ethernet and mmc and nand and oh crap, where
> did all of my space go?).

Sure, but then maybe you do need to remove some of that stuff and
configure the SPL for that one single need you have.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Am 08.01.2019 um 15:50 schrieb Marek Vasut:
> On 1/8/19 3:48 PM, Tom Rini wrote:
>> On Tue, Jan 08, 2019 at 01:49:14PM +0100, Marek Vasut wrote:
>>> On 1/8/19 1:46 PM, Simon Goldschmidt wrote:
>>>> On Tue, Jan 8, 2019 at 1:22 PM Marek Vasut <marex@denx.de> wrote:
>>>>>
>>>>> On 1/8/19 1:09 PM, Simon Goldschmidt wrote:
>>>>>> On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>
>>>>>>> On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
>>>>>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>
>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
>>>>>>>>>> via defconfig.
>>>>>>>>>>
>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
>>>>>>>>>> don't seem to work with it.
>>>>>>>>>
>>>>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>>>>
>>>>>>>> Here again, socfpga seems to be another bad example. Taking
>>>>>>>> peripherals out of reset
>>>>>>>> is cluttered throughout the mach-socfpga code at least in SPL. By now
>>>>>>>> I know socfpga is
>>>>>>>> lacking support for clock and reset management via devicetree. And
>>>>>>>> this is bad, I know,
>>>>>>>> but can we keep this a seperate issue from OF_PLATDATA?
>>>>>>>>
>>>>>>>> That being said, drivers/reset/reset-uclass.c fails to compile with
>>>>>>>> OF_PLATDATA, so I
>>>>>>>> guess this has not been used with OF_PLATDATA before. And given that I
>>>>>>>> don't seem
>>>>>>>> to need it for socfpga either, I don't think this would be the right
>>>>>>>> series to fix that.
>>>>>>>
>>>>>>> Don't you need it to unreset at least the DWMMC or CQSPI ?
>>>>>>
>>>>>> Reading the code, it seems like that's taken care of through another hack in
>>>>>> spl_boot_device() ;-)
>>>>>
>>>>> Sigh.
>>>>>
>>>>>>> Anyway, I'd much prefer to start cleaning up the horrorshow that
>>>>>>> arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
>>>>>>> Would that be possible ?
>>>>>>
>>>>>> I would be best, yes. I don't know when I will find the time to do that, though.
>>>>>> I don't know how much effort that would be, either. Is there maybe a patch
>>>>>> where A10 got converted from "as bad as gen5" to its current state? That
>>>>>> would help me to see if I can do it...
>>>>>
>>>>> A10 got switched to reset framework recently (in last 6 months or so),
>>>>> the reset driver is the same for Gen5 and A10 too, so it should be easy
>>>>> to recycle.
>>>>
>>>> Hmm, ok, let me check that... it would indeed be nice to port this to gen5.
>>>>
>>>> Since you seem kidn of opposed to OF_PLATDATA, does it make any sense
>>>> to continue on this? I mean, I thought I heard people here saying "use
>>>> OF_PLATDATA" if you're running out of space in SPL. After using it, I'm not too
>>>> keen on using it, either, but it does seem to give me some code space back...
>>>
>>> OF_PLATDATA is for platforms with really small SRAM, some 30k or below.
>>> This platform has a massive 60k of SRAM for SPL, so if we're running out
>>> of space, we're doing something wrong.
>>
>> It's not for "30k or below" but "needs more space to enable all desired
>> features inside of SPL".
> 
> Which the SoCFPGA should have with 60k of SRAM. If U-Boot SPL became so
> bloated that even platform with so much space has issues, how can we
> even cater for the rest of platforms with much more limited SPL ? And if
> that is the case, we have a much bigger problem ...

I have to add here: I'm not running out of space in SPL just yet. I'm at 
45k But I have tried and the remaining 15k don't seem to be enough to 
add verified FIT loading code. I expect this to be something very rarely 
used, if it is used at all (I saw presentations doing this to implement 
verified boot, but I failed to find the code in SPL that does this)...

I don't know yet how I'll continue here. I'll check again exactly how 
much of the SRAM my current OF_PLATDATA patches save.

Regards,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Am 08.01.2019 um 16:01 schrieb Simon Goldschmidt:
> Am 08.01.2019 um 15:50 schrieb Marek Vasut:
>> On 1/8/19 3:48 PM, Tom Rini wrote:
>>> On Tue, Jan 08, 2019 at 01:49:14PM +0100, Marek Vasut wrote:
>>>> On 1/8/19 1:46 PM, Simon Goldschmidt wrote:
>>>>> On Tue, Jan 8, 2019 at 1:22 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 1/8/19 1:09 PM, Simon Goldschmidt wrote:
>>>>>>> On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
>>>>>>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>
>>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
>>>>>>>>>>> via defconfig.
>>>>>>>>>>>
>>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
>>>>>>>>>>> don't seem to work with it.
>>>>>>>>>>
>>>>>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>>>>>
>>>>>>>>> Here again, socfpga seems to be another bad example. Taking
>>>>>>>>> peripherals out of reset
>>>>>>>>> is cluttered throughout the mach-socfpga code at least in SPL. By now
>>>>>>>>> I know socfpga is
>>>>>>>>> lacking support for clock and reset management via devicetree. And
>>>>>>>>> this is bad, I know,
>>>>>>>>> but can we keep this a seperate issue from OF_PLATDATA?
>>>>>>>>>
>>>>>>>>> That being said, drivers/reset/reset-uclass.c fails to compile with
>>>>>>>>> OF_PLATDATA, so I
>>>>>>>>> guess this has not been used with OF_PLATDATA before. And given that I
>>>>>>>>> don't seem
>>>>>>>>> to need it for socfpga either, I don't think this would be the right
>>>>>>>>> series to fix that.
>>>>>>>>
>>>>>>>> Don't you need it to unreset at least the DWMMC or CQSPI ?
>>>>>>>
>>>>>>> Reading the code, it seems like that's taken care of through another hack in
>>>>>>> spl_boot_device() ;-)
>>>>>>
>>>>>> Sigh.
>>>>>>
>>>>>>>> Anyway, I'd much prefer to start cleaning up the horrorshow that
>>>>>>>> arch/arm/mach-socfpga is in terms of clock and reset, at least like A10.
>>>>>>>> Would that be possible ?
>>>>>>>
>>>>>>> I would be best, yes. I don't know when I will find the time to do that, though.
>>>>>>> I don't know how much effort that would be, either. Is there maybe a patch
>>>>>>> where A10 got converted from "as bad as gen5" to its current state? That
>>>>>>> would help me to see if I can do it...
>>>>>>
>>>>>> A10 got switched to reset framework recently (in last 6 months or so),
>>>>>> the reset driver is the same for Gen5 and A10 too, so it should be easy
>>>>>> to recycle.
>>>>>
>>>>> Hmm, ok, let me check that... it would indeed be nice to port this to gen5.
>>>>>
>>>>> Since you seem kidn of opposed to OF_PLATDATA, does it make any sense
>>>>> to continue on this? I mean, I thought I heard people here saying "use
>>>>> OF_PLATDATA" if you're running out of space in SPL. After using it, I'm not too
>>>>> keen on using it, either, but it does seem to give me some code space back...
>>>>
>>>> OF_PLATDATA is for platforms with really small SRAM, some 30k or below.
>>>> This platform has a massive 60k of SRAM for SPL, so if we're running out
>>>> of space, we're doing something wrong.
>>>
>>> It's not for "30k or below" but "needs more space to enable all desired
>>> features inside of SPL".
>>
>> Which the SoCFPGA should have with 60k of SRAM. If U-Boot SPL became so
>> bloated that even platform with so much space has issues, how can we
>> even cater for the rest of platforms with much more limited SPL ? And if
>> that is the case, we have a much bigger problem ...
> 
> I have to add here: I'm not running out of space in SPL just yet. I'm at
> 45k But I have tried and the remaining 15k don't seem to be enough to
> add verified FIT loading code. I expect this to be something very rarely
> used, if it is used at all (I saw presentations doing this to implement
> verified boot, but I failed to find the code in SPL that does this)...
> 
> I don't know yet how I'll continue here. I'll check again exactly how
> much of the SRAM my current OF_PLATDATA patches save.

Ok, so when booting from mmc (with the tiny mmc framework), this series 
saves ~5.6 KiB of the SRAM for SPL (~3.7 KiB .text reduction, ~1.7 KiB 
for the DTB binary, other sections are slightly smaller).

It's not the world, but it's more than 10% (from ~40 KiB to 34.5 KiB).
I don't insist on doing this, but I wanted to write down numbers in case 
someone wants to discuss this further.

Regards,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/8/19 9:52 PM, Simon Goldschmidt wrote:
> Am 08.01.2019 um 16:01 schrieb Simon Goldschmidt:
>> Am 08.01.2019 um 15:50 schrieb Marek Vasut:
>>> On 1/8/19 3:48 PM, Tom Rini wrote:
>>>> On Tue, Jan 08, 2019 at 01:49:14PM +0100, Marek Vasut wrote:
>>>>> On 1/8/19 1:46 PM, Simon Goldschmidt wrote:
>>>>>> On Tue, Jan 8, 2019 at 1:22 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>
>>>>>>> On 1/8/19 1:09 PM, Simon Goldschmidt wrote:
>>>>>>>> On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>
>>>>>>>>> On 1/8/19 7:24 AM, Simon Goldschmidt wrote:
>>>>>>>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can
>>>>>>>>>>>> be disabled
>>>>>>>>>>>> via defconfig.
>>>>>>>>>>>>
>>>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the
>>>>>>>>>>>> reset drivers
>>>>>>>>>>>> don't seem to work with it.
>>>>>>>>>>>
>>>>>>>>>>> How do you un-reset IP blocks if you disable the reset
>>>>>>>>>>> controller ?
>>>>>>>>>>
>>>>>>>>>> Here again, socfpga seems to be another bad example. Taking
>>>>>>>>>> peripherals out of reset
>>>>>>>>>> is cluttered throughout the mach-socfpga code at least in SPL.
>>>>>>>>>> By now
>>>>>>>>>> I know socfpga is
>>>>>>>>>> lacking support for clock and reset management via devicetree.
>>>>>>>>>> And
>>>>>>>>>> this is bad, I know,
>>>>>>>>>> but can we keep this a seperate issue from OF_PLATDATA?
>>>>>>>>>>
>>>>>>>>>> That being said, drivers/reset/reset-uclass.c fails to compile
>>>>>>>>>> with
>>>>>>>>>> OF_PLATDATA, so I
>>>>>>>>>> guess this has not been used with OF_PLATDATA before. And
>>>>>>>>>> given that I
>>>>>>>>>> don't seem
>>>>>>>>>> to need it for socfpga either, I don't think this would be the
>>>>>>>>>> right
>>>>>>>>>> series to fix that.
>>>>>>>>>
>>>>>>>>> Don't you need it to unreset at least the DWMMC or CQSPI ?
>>>>>>>>
>>>>>>>> Reading the code, it seems like that's taken care of through
>>>>>>>> another hack in
>>>>>>>> spl_boot_device() ;-)
>>>>>>>
>>>>>>> Sigh.
>>>>>>>
>>>>>>>>> Anyway, I'd much prefer to start cleaning up the horrorshow that
>>>>>>>>> arch/arm/mach-socfpga is in terms of clock and reset, at least
>>>>>>>>> like A10.
>>>>>>>>> Would that be possible ?
>>>>>>>>
>>>>>>>> I would be best, yes. I don't know when I will find the time to
>>>>>>>> do that, though.
>>>>>>>> I don't know how much effort that would be, either. Is there
>>>>>>>> maybe a patch
>>>>>>>> where A10 got converted from "as bad as gen5" to its current
>>>>>>>> state? That
>>>>>>>> would help me to see if I can do it...
>>>>>>>
>>>>>>> A10 got switched to reset framework recently (in last 6 months or
>>>>>>> so),
>>>>>>> the reset driver is the same for Gen5 and A10 too, so it should
>>>>>>> be easy
>>>>>>> to recycle.
>>>>>>
>>>>>> Hmm, ok, let me check that... it would indeed be nice to port this
>>>>>> to gen5.
>>>>>>
>>>>>> Since you seem kidn of opposed to OF_PLATDATA, does it make any sense
>>>>>> to continue on this? I mean, I thought I heard people here saying
>>>>>> "use
>>>>>> OF_PLATDATA" if you're running out of space in SPL. After using
>>>>>> it, I'm not too
>>>>>> keen on using it, either, but it does seem to give me some code
>>>>>> space back...
>>>>>
>>>>> OF_PLATDATA is for platforms with really small SRAM, some 30k or
>>>>> below.
>>>>> This platform has a massive 60k of SRAM for SPL, so if we're
>>>>> running out
>>>>> of space, we're doing something wrong.
>>>>
>>>> It's not for "30k or below" but "needs more space to enable all desired
>>>> features inside of SPL".
>>>
>>> Which the SoCFPGA should have with 60k of SRAM. If U-Boot SPL became so
>>> bloated that even platform with so much space has issues, how can we
>>> even cater for the rest of platforms with much more limited SPL ? And if
>>> that is the case, we have a much bigger problem ...
>>
>> I have to add here: I'm not running out of space in SPL just yet. I'm at
>> 45k But I have tried and the remaining 15k don't seem to be enough to
>> add verified FIT loading code. I expect this to be something very rarely
>> used, if it is used at all (I saw presentations doing this to implement
>> verified boot, but I failed to find the code in SPL that does this)...
>>
>> I don't know yet how I'll continue here. I'll check again exactly how
>> much of the SRAM my current OF_PLATDATA patches save.
> 
> Ok, so when booting from mmc (with the tiny mmc framework), this series
> saves ~5.6 KiB of the SRAM for SPL (~3.7 KiB .text reduction, ~1.7 KiB
> for the DTB binary, other sections are slightly smaller).
> 
> It's not the world, but it's more than 10% (from ~40 KiB to 34.5 KiB).
> I don't insist on doing this, but I wanted to write down numbers in case
> someone wants to discuss this further.

I'm sure if you went all the way and removed even the DT support
altogether, the SPL would be even smaller too.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Am 07.01.2019 um 23:53 schrieb Marek Vasut:
> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
>> via defconfig.
>>
>> This also seems to be required to use OF_PLATDATA, as the reset drivers
>> don't seem to work with it.
> 
> How do you un-reset IP blocks if you disable the reset controller ?

I found that out just now: there's the function 
'reset_deassert_peripherals_handoff()' in spl_gen5.c that should 
"De-assert reset for peripherals and bridges based on handoff". However, 
at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing 
that, it enables *ALL* peripherals on the SoC (except for some DMA 
channels that aren't really used) :-)

I guess that needs some cleaning up as well ;-)

I think the proper thing to do here would be to remove this function and 
convert all drivers to provide appropriate 'resets' properties in the dts?

Regards,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
>>> via defconfig.
>>>
>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
>>> don't seem to work with it.
>>
>> How do you un-reset IP blocks if you disable the reset controller ?
> 
> I found that out just now: there's the function
> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
> "De-assert reset for peripherals and bridges based on handoff". However,
> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
> that, it enables *ALL* peripherals on the SoC (except for some DMA
> channels that aren't really used) :-)
> 
> I guess that needs some cleaning up as well ;-)

Yes

> I think the proper thing to do here would be to remove this function and
> convert all drivers to provide appropriate 'resets' properties in the dts?

Indeed

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Am 11.01.2019 um 23:02 schrieb Marek Vasut:
> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be disabled
>>>> via defconfig.
>>>>
>>>> This also seems to be required to use OF_PLATDATA, as the reset drivers
>>>> don't seem to work with it.
>>>
>>> How do you un-reset IP blocks if you disable the reset controller ?
>>
>> I found that out just now: there's the function
>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>> "De-assert reset for peripherals and bridges based on handoff". However,
>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>> channels that aren't really used) :-)
>>
>> I guess that needs some cleaning up as well ;-)
> 
> Yes
> 
>> I think the proper thing to do here would be to remove this function and
>> convert all drivers to provide appropriate 'resets' properties in the dts?
> 
> Indeed

So I just did that and it works nice for SPL and U-Boot: By adding some 
"resets" properties the the main dtsi and adding reset bulk code to the 
cadence_qspi, denali_dt nand and drivers, I can nearly remove the reset 
code from arch/mach_socfpga.

The problem would be that now Linux cannot use peripherals that aren't 
enabled by U-Boot because it relies on them being enabled. How are such 
dependencies solved? Because even if I would add reset support in the 
corresponding Linux drivers, we probably could not bootolder Kernels 
(e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...

Regards,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Dinh Nguyen]

Hi Simon,

On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>> disabled
>>>>> via defconfig.
>>>>>
>>>>> This also seems to be required to use OF_PLATDATA, as the reset
>>>>> drivers
>>>>> don't seem to work with it.
>>>>
>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>
>>> I found that out just now: there's the function
>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>> "De-assert reset for peripherals and bridges based on handoff". However,
>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>>> channels that aren't really used) :-)
>>>
>>> I guess that needs some cleaning up as well ;-)
>>
>> Yes
>>
>>> I think the proper thing to do here would be to remove this function and
>>> convert all drivers to provide appropriate 'resets' properties in the
>>> dts?
>>
>> Indeed
> 
> So I just did that and it works nice for SPL and U-Boot: By adding some
> "resets" properties the the main dtsi and adding reset bulk code to the
> cadence_qspi, denali_dt nand and drivers, I can nearly remove the reset
> code from arch/mach_socfpga.
> 
> The problem would be that now Linux cannot use peripherals that aren't
> enabled by U-Boot because it relies on them being enabled. How are such
> dependencies solved? Because even if I would add reset support in the
> corresponding Linux drivers, we probably could not bootolder Kernels
> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
> 

I added an early reset driver for SoCFPGA that should take care of this.
The patch is in v5.0-rc2[1].

Dinh

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/reset?id=b3ca9888f35fa6919569cf27c929dc0ac49e9716

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Hi Dinh,

Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
> Hi Simon,
> 
> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>> disabled
>>>>>> via defconfig.
>>>>>>
>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
>>>>>> drivers
>>>>>> don't seem to work with it.
>>>>>
>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>
>>>> I found that out just now: there's the function
>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>> "De-assert reset for peripherals and bridges based on handoff". However,
>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>>>> channels that aren't really used) :-)
>>>>
>>>> I guess that needs some cleaning up as well ;-)
>>>
>>> Yes
>>>
>>>> I think the proper thing to do here would be to remove this function and
>>>> convert all drivers to provide appropriate 'resets' properties in the
>>>> dts?
>>>
>>> Indeed
>>
>> So I just did that and it works nice for SPL and U-Boot: By adding some
>> "resets" properties the the main dtsi and adding reset bulk code to the
>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the reset
>> code from arch/mach_socfpga.
>>
>> The problem would be that now Linux cannot use peripherals that aren't
>> enabled by U-Boot because it relies on them being enabled. How are such
>> dependencies solved? Because even if I would add reset support in the
>> corresponding Linux drivers, we probably could not bootolder Kernels
>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>
> 
> I added an early reset driver for SoCFPGA that should take care of this.
> The patch is in v5.0-rc2[1].

OK, it's good to know that this work is already done, I haven't 
monitored this close enough.

But am I correct that my above problem remains even in v5.0 as not all 
peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and 
qspi) and would thuse not be taken out of reset by Linux?

Plus: should U-Boot work with older Linux kernels? Because if so, we 
need fallback code in U-Boot to unreset peripherals when running with an 
older kernel...

Regards,
Simon

> 
> Dinh
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/reset?id=b3ca9888f35fa6919569cf27c929dc0ac49e9716
> 

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
> Hi Dinh,

Hi,

> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>> Hi Simon,
>>
>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>> disabled
>>>>>>> via defconfig.
>>>>>>>
>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
>>>>>>> drivers
>>>>>>> don't seem to work with it.
>>>>>>
>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>
>>>>> I found that out just now: there's the function
>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>> However,
>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
>>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>>>>> channels that aren't really used) :-)
>>>>>
>>>>> I guess that needs some cleaning up as well ;-)
>>>>
>>>> Yes
>>>>
>>>>> I think the proper thing to do here would be to remove this
>>>>> function and
>>>>> convert all drivers to provide appropriate 'resets' properties in the
>>>>> dts?
>>>>
>>>> Indeed
>>>
>>> So I just did that and it works nice for SPL and U-Boot: By adding some
>>> "resets" properties the the main dtsi and adding reset bulk code to the
>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the reset
>>> code from arch/mach_socfpga.
>>>
>>> The problem would be that now Linux cannot use peripherals that aren't
>>> enabled by U-Boot because it relies on them being enabled. How are such
>>> dependencies solved? Because even if I would add reset support in the
>>> corresponding Linux drivers, we probably could not bootolder Kernels
>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>
>>
>> I added an early reset driver for SoCFPGA that should take care of this.
>> The patch is in v5.0-rc2[1].
> 
> OK, it's good to know that this work is already done, I haven't
> monitored this close enough.

We had the same problem with A10, indeed.

> But am I correct that my above problem remains even in v5.0 as not all
> peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
> qspi) and would thuse not be taken out of reset by Linux?
> 
> Plus: should U-Boot work with older Linux kernels? Because if so, we
> need fallback code in U-Boot to unreset peripherals when running with an
> older kernel...

Yes, it'd break old broken kernels . The real question is, do we care ?

> Regards,
> Simon
> 
>>
>> Dinh
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/reset?id=b3ca9888f35fa6919569cf27c929dc0ac49e9716
>>
>>
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Am 14.01.2019 um 19:31 schrieb Marek Vasut:
> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
>> Hi Dinh,
> 
> Hi,
> 
>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>>> Hi Simon,
>>>
>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>>> disabled
>>>>>>>> via defconfig.
>>>>>>>>
>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
>>>>>>>> drivers
>>>>>>>> don't seem to work with it.
>>>>>>>
>>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>>
>>>>>> I found that out just now: there's the function
>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>>> However,
>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
>>>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>>>>>> channels that aren't really used) :-)
>>>>>>
>>>>>> I guess that needs some cleaning up as well ;-)
>>>>>
>>>>> Yes
>>>>>
>>>>>> I think the proper thing to do here would be to remove this
>>>>>> function and
>>>>>> convert all drivers to provide appropriate 'resets' properties in the
>>>>>> dts?
>>>>>
>>>>> Indeed
>>>>
>>>> So I just did that and it works nice for SPL and U-Boot: By adding some
>>>> "resets" properties the the main dtsi and adding reset bulk code to the
>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the reset
>>>> code from arch/mach_socfpga.
>>>>
>>>> The problem would be that now Linux cannot use peripherals that aren't
>>>> enabled by U-Boot because it relies on them being enabled. How are such
>>>> dependencies solved? Because even if I would add reset support in the
>>>> corresponding Linux drivers, we probably could not bootolder Kernels
>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>>
>>>
>>> I added an early reset driver for SoCFPGA that should take care of this.
>>> The patch is in v5.0-rc2[1].
>>
>> OK, it's good to know that this work is already done, I haven't
>> monitored this close enough.
> 
> We had the same problem with A10, indeed.
> 
>> But am I correct that my above problem remains even in v5.0 as not all
>> peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
>> qspi) and would thuse not be taken out of reset by Linux?
>>
>> Plus: should U-Boot work with older Linux kernels? Because if so, we
>> need fallback code in U-Boot to unreset peripherals when running with an
>> older kernel...
> 
> Yes, it'd break old broken kernels . The real question is, do we care ?

Ok, so that at leat shows me I'm going into the right direction :-)

There are some problems though:
- I do care (we're running 4.9 currently) *g*
- people running an RT kernel will care for a while (until the next 
stable RT after fixing this will be released)
- we would currently be breaking *all* kernels, since no kernel should 
yet be able to deassert reset for mmc and qspi (unless this is already 
done by U-Boot)...

So would it be OK to add a Kconfig option to U-Boot to keep the current 
behaviour (for old broken kernels like you said) until that code is 
spread widely enough? Or is that a no-go?

Regards,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/14/19 7:58 PM, Simon Goldschmidt wrote:
> Am 14.01.2019 um 19:31 schrieb Marek Vasut:
>> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
>>> Hi Dinh,
>>
>> Hi,
>>
>>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>>>> Hi Simon,
>>>>
>>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>>>> disabled
>>>>>>>>> via defconfig.
>>>>>>>>>
>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
>>>>>>>>> drivers
>>>>>>>>> don't seem to work with it.
>>>>>>>>
>>>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>>>
>>>>>>> I found that out just now: there's the function
>>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>>>> However,
>>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
>>>>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>>>>>>> channels that aren't really used) :-)
>>>>>>>
>>>>>>> I guess that needs some cleaning up as well ;-)
>>>>>>
>>>>>> Yes
>>>>>>
>>>>>>> I think the proper thing to do here would be to remove this
>>>>>>> function and
>>>>>>> convert all drivers to provide appropriate 'resets' properties in
>>>>>>> the
>>>>>>> dts?
>>>>>>
>>>>>> Indeed
>>>>>
>>>>> So I just did that and it works nice for SPL and U-Boot: By adding
>>>>> some
>>>>> "resets" properties the the main dtsi and adding reset bulk code to
>>>>> the
>>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the
>>>>> reset
>>>>> code from arch/mach_socfpga.
>>>>>
>>>>> The problem would be that now Linux cannot use peripherals that aren't
>>>>> enabled by U-Boot because it relies on them being enabled. How are
>>>>> such
>>>>> dependencies solved? Because even if I would add reset support in the
>>>>> corresponding Linux drivers, we probably could not bootolder Kernels
>>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>>>
>>>>
>>>> I added an early reset driver for SoCFPGA that should take care of
>>>> this.
>>>> The patch is in v5.0-rc2[1].
>>>
>>> OK, it's good to know that this work is already done, I haven't
>>> monitored this close enough.
>>
>> We had the same problem with A10, indeed.
>>
>>> But am I correct that my above problem remains even in v5.0 as not all
>>> peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
>>> qspi) and would thuse not be taken out of reset by Linux?
>>>
>>> Plus: should U-Boot work with older Linux kernels? Because if so, we
>>> need fallback code in U-Boot to unreset peripherals when running with an
>>> older kernel...
>>
>> Yes, it'd break old broken kernels . The real question is, do we care ?
> 
> Ok, so that at leat shows me I'm going into the right direction :-)
> 
> There are some problems though:
> - I do care (we're running 4.9 currently) *g*
> - people running an RT kernel will care for a while (until the next
> stable RT after fixing this will be released)
> - we would currently be breaking *all* kernels, since no kernel should
> yet be able to deassert reset for mmc and qspi (unless this is already
> done by U-Boot)...
> 
> So would it be OK to add a Kconfig option to U-Boot to keep the current
> behaviour (for old broken kernels like you said) until that code is
> spread widely enough? Or is that a no-go?

Would be nice to be able to tweak the reset driver behavior at runtime,
to unreset things before booting the kernel if the user desires so.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Am 14.01.2019 um 20:33 schrieb Marek Vasut:
> On 1/14/19 7:58 PM, Simon Goldschmidt wrote:
>> Am 14.01.2019 um 19:31 schrieb Marek Vasut:
>>> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
>>>> Hi Dinh,
>>>
>>> Hi,
>>>
>>>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>>>>> Hi Simon,
>>>>>
>>>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>>>>> disabled
>>>>>>>>>> via defconfig.
>>>>>>>>>>
>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
>>>>>>>>>> drivers
>>>>>>>>>> don't seem to work with it.
>>>>>>>>>
>>>>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>>>>
>>>>>>>> I found that out just now: there's the function
>>>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>>>>> However,
>>>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
>>>>>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>>>>>>>> channels that aren't really used) :-)
>>>>>>>>
>>>>>>>> I guess that needs some cleaning up as well ;-)
>>>>>>>
>>>>>>> Yes
>>>>>>>
>>>>>>>> I think the proper thing to do here would be to remove this
>>>>>>>> function and
>>>>>>>> convert all drivers to provide appropriate 'resets' properties in
>>>>>>>> the
>>>>>>>> dts?
>>>>>>>
>>>>>>> Indeed
>>>>>>
>>>>>> So I just did that and it works nice for SPL and U-Boot: By adding
>>>>>> some
>>>>>> "resets" properties the the main dtsi and adding reset bulk code to
>>>>>> the
>>>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the
>>>>>> reset
>>>>>> code from arch/mach_socfpga.
>>>>>>
>>>>>> The problem would be that now Linux cannot use peripherals that aren't
>>>>>> enabled by U-Boot because it relies on them being enabled. How are
>>>>>> such
>>>>>> dependencies solved? Because even if I would add reset support in the
>>>>>> corresponding Linux drivers, we probably could not bootolder Kernels
>>>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>>>>
>>>>>
>>>>> I added an early reset driver for SoCFPGA that should take care of
>>>>> this.
>>>>> The patch is in v5.0-rc2[1].
>>>>
>>>> OK, it's good to know that this work is already done, I haven't
>>>> monitored this close enough.
>>>
>>> We had the same problem with A10, indeed.
>>>
>>>> But am I correct that my above problem remains even in v5.0 as not all
>>>> peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
>>>> qspi) and would thuse not be taken out of reset by Linux?
>>>>
>>>> Plus: should U-Boot work with older Linux kernels? Because if so, we
>>>> need fallback code in U-Boot to unreset peripherals when running with an
>>>> older kernel...
>>>
>>> Yes, it'd break old broken kernels . The real question is, do we care ?
>>
>> Ok, so that at leat shows me I'm going into the right direction :-)
>>
>> There are some problems though:
>> - I do care (we're running 4.9 currently) *g*
>> - people running an RT kernel will care for a while (until the next
>> stable RT after fixing this will be released)
>> - we would currently be breaking *all* kernels, since no kernel should
>> yet be able to deassert reset for mmc and qspi (unless this is already
>> done by U-Boot)...
>>
>> So would it be OK to add a Kconfig option to U-Boot to keep the current
>> behaviour (for old broken kernels like you said) until that code is
>> spread widely enough? Or is that a no-go?
> 
> Would be nice to be able to tweak the reset driver behavior at runtime,
> to unreset things before booting the kernel if the user desires so.

Instead of tweaking the reset driver, we could just add a command that 
does that 'rstmgr->permodrst = 0;' thing my patch would remove.

Since noone has complained so far, I think writing 0 should be OK here. 
I don't think it would make too much sense to use the reset handoff 
defines from Quartus output for such a command. I think the way Quartus 
does this is strange anyway...

The question is if defconfigs should be able to use this to 
automatically build a U-Boot config for older kernels. If so, we'd still 
need a Kconfig option?

Thinking further about cleanup: I guess the clock driver is not that 
hard to implement, either. The only thing that's driving me mad is 
pinmux. Is there any chance to get more info from Intel to write this 
properly so we can get rid of that iocsr scanchain defines?

Thanks,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/14/19 8:43 PM, Simon Goldschmidt wrote:
> Am 14.01.2019 um 20:33 schrieb Marek Vasut:
>> On 1/14/19 7:58 PM, Simon Goldschmidt wrote:
>>> Am 14.01.2019 um 19:31 schrieb Marek Vasut:
>>>> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
>>>>> Hi Dinh,
>>>>
>>>> Hi,
>>>>
>>>>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>>>>>> Hi Simon,
>>>>>>
>>>>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>>>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>>>>>> disabled
>>>>>>>>>>> via defconfig.
>>>>>>>>>>>
>>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
>>>>>>>>>>> drivers
>>>>>>>>>>> don't seem to work with it.
>>>>>>>>>>
>>>>>>>>>> How do you un-reset IP blocks if you disable the reset
>>>>>>>>>> controller ?
>>>>>>>>>
>>>>>>>>> I found that out just now: there's the function
>>>>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>>>>>> However,
>>>>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By
>>>>>>>>> doing
>>>>>>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>>>>>>>>> channels that aren't really used) :-)
>>>>>>>>>
>>>>>>>>> I guess that needs some cleaning up as well ;-)
>>>>>>>>
>>>>>>>> Yes
>>>>>>>>
>>>>>>>>> I think the proper thing to do here would be to remove this
>>>>>>>>> function and
>>>>>>>>> convert all drivers to provide appropriate 'resets' properties in
>>>>>>>>> the
>>>>>>>>> dts?
>>>>>>>>
>>>>>>>> Indeed
>>>>>>>
>>>>>>> So I just did that and it works nice for SPL and U-Boot: By adding
>>>>>>> some
>>>>>>> "resets" properties the the main dtsi and adding reset bulk code to
>>>>>>> the
>>>>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the
>>>>>>> reset
>>>>>>> code from arch/mach_socfpga.
>>>>>>>
>>>>>>> The problem would be that now Linux cannot use peripherals that
>>>>>>> aren't
>>>>>>> enabled by U-Boot because it relies on them being enabled. How are
>>>>>>> such
>>>>>>> dependencies solved? Because even if I would add reset support in
>>>>>>> the
>>>>>>> corresponding Linux drivers, we probably could not bootolder Kernels
>>>>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>>>>>
>>>>>>
>>>>>> I added an early reset driver for SoCFPGA that should take care of
>>>>>> this.
>>>>>> The patch is in v5.0-rc2[1].
>>>>>
>>>>> OK, it's good to know that this work is already done, I haven't
>>>>> monitored this close enough.
>>>>
>>>> We had the same problem with A10, indeed.
>>>>
>>>>> But am I correct that my above problem remains even in v5.0 as not all
>>>>> peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
>>>>> qspi) and would thuse not be taken out of reset by Linux?
>>>>>
>>>>> Plus: should U-Boot work with older Linux kernels? Because if so, we
>>>>> need fallback code in U-Boot to unreset peripherals when running
>>>>> with an
>>>>> older kernel...
>>>>
>>>> Yes, it'd break old broken kernels . The real question is, do we care ?
>>>
>>> Ok, so that at leat shows me I'm going into the right direction :-)
>>>
>>> There are some problems though:
>>> - I do care (we're running 4.9 currently) *g*
>>> - people running an RT kernel will care for a while (until the next
>>> stable RT after fixing this will be released)
>>> - we would currently be breaking *all* kernels, since no kernel should
>>> yet be able to deassert reset for mmc and qspi (unless this is already
>>> done by U-Boot)...
>>>
>>> So would it be OK to add a Kconfig option to U-Boot to keep the current
>>> behaviour (for old broken kernels like you said) until that code is
>>> spread widely enough? Or is that a no-go?
>>
>> Would be nice to be able to tweak the reset driver behavior at runtime,
>> to unreset things before booting the kernel if the user desires so.
> 
> Instead of tweaking the reset driver, we could just add a command that
> does that 'rstmgr->permodrst = 0;' thing my patch would remove.

I don't want a new custom command.

> Since noone has complained so far, I think writing 0 should be OK here.
> I don't think it would make too much sense to use the reset handoff
> defines from Quartus output for such a command. I think the way Quartus
> does this is strange anyway...
> 
> The question is if defconfigs should be able to use this to
> automatically build a U-Boot config for older kernels. If so, we'd still
> need a Kconfig option?

I'd much rather have this runtime configurable.

> Thinking further about cleanup: I guess the clock driver is not that
> hard to implement, either. The only thing that's driving me mad is
> pinmux. Is there any chance to get more info from Intel to write this
> properly so we can get rid of that iocsr scanchain defines?

Clock driver should be easy, yes. Pinmux, I don't know, maybe project
chibi has some information (the cyclone I documentation project).

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Am 14.01.2019 um 21:01 schrieb Marek Vasut:
> On 1/14/19 8:43 PM, Simon Goldschmidt wrote:
>> Am 14.01.2019 um 20:33 schrieb Marek Vasut:
>>> On 1/14/19 7:58 PM, Simon Goldschmidt wrote:
>>>> Am 14.01.2019 um 19:31 schrieb Marek Vasut:
>>>>> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
>>>>>> Hi Dinh,
>>>>>
>>>>> Hi,
>>>>>
>>>>>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>>>>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>>>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>>>>>>> disabled
>>>>>>>>>>>> via defconfig.
>>>>>>>>>>>>
>>>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
>>>>>>>>>>>> drivers
>>>>>>>>>>>> don't seem to work with it.
>>>>>>>>>>>
>>>>>>>>>>> How do you un-reset IP blocks if you disable the reset
>>>>>>>>>>> controller ?
>>>>>>>>>>
>>>>>>>>>> I found that out just now: there's the function
>>>>>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>>>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>>>>>>> However,
>>>>>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By
>>>>>>>>>> doing
>>>>>>>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>>>>>>>>>> channels that aren't really used) :-)
>>>>>>>>>>
>>>>>>>>>> I guess that needs some cleaning up as well ;-)
>>>>>>>>>
>>>>>>>>> Yes
>>>>>>>>>
>>>>>>>>>> I think the proper thing to do here would be to remove this
>>>>>>>>>> function and
>>>>>>>>>> convert all drivers to provide appropriate 'resets' properties in
>>>>>>>>>> the
>>>>>>>>>> dts?
>>>>>>>>>
>>>>>>>>> Indeed
>>>>>>>>
>>>>>>>> So I just did that and it works nice for SPL and U-Boot: By adding
>>>>>>>> some
>>>>>>>> "resets" properties the the main dtsi and adding reset bulk code to
>>>>>>>> the
>>>>>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the
>>>>>>>> reset
>>>>>>>> code from arch/mach_socfpga.
>>>>>>>>
>>>>>>>> The problem would be that now Linux cannot use peripherals that
>>>>>>>> aren't
>>>>>>>> enabled by U-Boot because it relies on them being enabled. How are
>>>>>>>> such
>>>>>>>> dependencies solved? Because even if I would add reset support in
>>>>>>>> the
>>>>>>>> corresponding Linux drivers, we probably could not bootolder Kernels
>>>>>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>>>>>>
>>>>>>>
>>>>>>> I added an early reset driver for SoCFPGA that should take care of
>>>>>>> this.
>>>>>>> The patch is in v5.0-rc2[1].
>>>>>>
>>>>>> OK, it's good to know that this work is already done, I haven't
>>>>>> monitored this close enough.
>>>>>
>>>>> We had the same problem with A10, indeed.
>>>>>
>>>>>> But am I correct that my above problem remains even in v5.0 as not all
>>>>>> peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
>>>>>> qspi) and would thuse not be taken out of reset by Linux?
>>>>>>
>>>>>> Plus: should U-Boot work with older Linux kernels? Because if so, we
>>>>>> need fallback code in U-Boot to unreset peripherals when running
>>>>>> with an
>>>>>> older kernel...
>>>>>
>>>>> Yes, it'd break old broken kernels . The real question is, do we care ?
>>>>
>>>> Ok, so that at leat shows me I'm going into the right direction :-)
>>>>
>>>> There are some problems though:
>>>> - I do care (we're running 4.9 currently) *g*
>>>> - people running an RT kernel will care for a while (until the next
>>>> stable RT after fixing this will be released)
>>>> - we would currently be breaking *all* kernels, since no kernel should
>>>> yet be able to deassert reset for mmc and qspi (unless this is already
>>>> done by U-Boot)...
>>>>
>>>> So would it be OK to add a Kconfig option to U-Boot to keep the current
>>>> behaviour (for old broken kernels like you said) until that code is
>>>> spread widely enough? Or is that a no-go?
>>>
>>> Would be nice to be able to tweak the reset driver behavior at runtime,
>>> to unreset things before booting the kernel if the user desires so.
>>
>> Instead of tweaking the reset driver, we could just add a command that
>> does that 'rstmgr->permodrst = 0;' thing my patch would remove.
> 
> I don't want a new custom command.
> 
>> Since noone has complained so far, I think writing 0 should be OK here.
>> I don't think it would make too much sense to use the reset handoff
>> defines from Quartus output for such a command. I think the way Quartus
>> does this is strange anyway...
>>
>> The question is if defconfigs should be able to use this to
>> automatically build a U-Boot config for older kernels. If so, we'd still
>> need a Kconfig option?
> 
> I'd much rather have this runtime configurable.

Then I'm afraid I don't know what you mean by "runtime configurable". 
What should be the configuration source that is evaluated at runtime?

> 
>> Thinking further about cleanup: I guess the clock driver is not that
>> hard to implement, either. The only thing that's driving me mad is
>> pinmux. Is there any chance to get more info from Intel to write this
>> properly so we can get rid of that iocsr scanchain defines?
> 
> Clock driver should be easy, yes. Pinmux, I don't know, maybe project
> chibi has some information (the cyclone I documentation project).

Interesing, I didn't know that project. The only thing I found is a repo 
on github. But it seems like that one only contains FPGA-related stuff, 
nothing about the HPS side...

Regards,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/14/19 9:12 PM, Simon Goldschmidt wrote:
> Am 14.01.2019 um 21:01 schrieb Marek Vasut:
>> On 1/14/19 8:43 PM, Simon Goldschmidt wrote:
>>> Am 14.01.2019 um 20:33 schrieb Marek Vasut:
>>>> On 1/14/19 7:58 PM, Simon Goldschmidt wrote:
>>>>> Am 14.01.2019 um 19:31 schrieb Marek Vasut:
>>>>>> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
>>>>>>> Hi Dinh,
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>>>>>>>> Hi Simon,
>>>>>>>>
>>>>>>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>>>>>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>>>>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>>>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>>>>>>>> disabled
>>>>>>>>>>>>> via defconfig.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the
>>>>>>>>>>>>> reset
>>>>>>>>>>>>> drivers
>>>>>>>>>>>>> don't seem to work with it.
>>>>>>>>>>>>
>>>>>>>>>>>> How do you un-reset IP blocks if you disable the reset
>>>>>>>>>>>> controller ?
>>>>>>>>>>>
>>>>>>>>>>> I found that out just now: there's the function
>>>>>>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>>>>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>>>>>>>> However,
>>>>>>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By
>>>>>>>>>>> doing
>>>>>>>>>>> that, it enables *ALL* peripherals on the SoC (except for
>>>>>>>>>>> some DMA
>>>>>>>>>>> channels that aren't really used) :-)
>>>>>>>>>>>
>>>>>>>>>>> I guess that needs some cleaning up as well ;-)
>>>>>>>>>>
>>>>>>>>>> Yes
>>>>>>>>>>
>>>>>>>>>>> I think the proper thing to do here would be to remove this
>>>>>>>>>>> function and
>>>>>>>>>>> convert all drivers to provide appropriate 'resets'
>>>>>>>>>>> properties in
>>>>>>>>>>> the
>>>>>>>>>>> dts?
>>>>>>>>>>
>>>>>>>>>> Indeed
>>>>>>>>>
>>>>>>>>> So I just did that and it works nice for SPL and U-Boot: By adding
>>>>>>>>> some
>>>>>>>>> "resets" properties the the main dtsi and adding reset bulk
>>>>>>>>> code to
>>>>>>>>> the
>>>>>>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the
>>>>>>>>> reset
>>>>>>>>> code from arch/mach_socfpga.
>>>>>>>>>
>>>>>>>>> The problem would be that now Linux cannot use peripherals that
>>>>>>>>> aren't
>>>>>>>>> enabled by U-Boot because it relies on them being enabled. How are
>>>>>>>>> such
>>>>>>>>> dependencies solved? Because even if I would add reset support in
>>>>>>>>> the
>>>>>>>>> corresponding Linux drivers, we probably could not bootolder
>>>>>>>>> Kernels
>>>>>>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>>>>>>>
>>>>>>>>
>>>>>>>> I added an early reset driver for SoCFPGA that should take care of
>>>>>>>> this.
>>>>>>>> The patch is in v5.0-rc2[1].
>>>>>>>
>>>>>>> OK, it's good to know that this work is already done, I haven't
>>>>>>> monitored this close enough.
>>>>>>
>>>>>> We had the same problem with A10, indeed.
>>>>>>
>>>>>>> But am I correct that my above problem remains even in v5.0 as
>>>>>>> not all
>>>>>>> peripherals in socfpga.dtsi have a "resets" property set (e.g.
>>>>>>> mmc and
>>>>>>> qspi) and would thuse not be taken out of reset by Linux?
>>>>>>>
>>>>>>> Plus: should U-Boot work with older Linux kernels? Because if so, we
>>>>>>> need fallback code in U-Boot to unreset peripherals when running
>>>>>>> with an
>>>>>>> older kernel...
>>>>>>
>>>>>> Yes, it'd break old broken kernels . The real question is, do we
>>>>>> care ?
>>>>>
>>>>> Ok, so that at leat shows me I'm going into the right direction :-)
>>>>>
>>>>> There are some problems though:
>>>>> - I do care (we're running 4.9 currently) *g*
>>>>> - people running an RT kernel will care for a while (until the next
>>>>> stable RT after fixing this will be released)
>>>>> - we would currently be breaking *all* kernels, since no kernel should
>>>>> yet be able to deassert reset for mmc and qspi (unless this is already
>>>>> done by U-Boot)...
>>>>>
>>>>> So would it be OK to add a Kconfig option to U-Boot to keep the
>>>>> current
>>>>> behaviour (for old broken kernels like you said) until that code is
>>>>> spread widely enough? Or is that a no-go?
>>>>
>>>> Would be nice to be able to tweak the reset driver behavior at runtime,
>>>> to unreset things before booting the kernel if the user desires so.
>>>
>>> Instead of tweaking the reset driver, we could just add a command that
>>> does that 'rstmgr->permodrst = 0;' thing my patch would remove.
>>
>> I don't want a new custom command.
>>
>>> Since noone has complained so far, I think writing 0 should be OK here.
>>> I don't think it would make too much sense to use the reset handoff
>>> defines from Quartus output for such a command. I think the way Quartus
>>> does this is strange anyway...
>>>
>>> The question is if defconfigs should be able to use this to
>>> automatically build a U-Boot config for older kernels. If so, we'd still
>>> need a Kconfig option?
>>
>> I'd much rather have this runtime configurable.
> 
> Then I'm afraid I don't know what you mean by "runtime configurable".
> What should be the configuration source that is evaluated at runtime?

I wonder ... maybe an environment variable with a U_BOOT_ENV_CALLBACK()
hook ?

>>> Thinking further about cleanup: I guess the clock driver is not that
>>> hard to implement, either. The only thing that's driving me mad is
>>> pinmux. Is there any chance to get more info from Intel to write this
>>> properly so we can get rid of that iocsr scanchain defines?
>>
>> Clock driver should be easy, yes. Pinmux, I don't know, maybe project
>> chibi has some information (the cyclone I documentation project).
> 
> Interesing, I didn't know that project. The only thing I found is a repo
> on github. But it seems like that one only contains FPGA-related stuff,
> nothing about the HPS side...

My understanding is that the IOCSR ring is on the FPGA side and it's
exactly the same as pinmux configuration for the FPGA IO, but I might be
wrong.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

On Mon, Jan 14, 2019 at 9:23 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/14/19 9:12 PM, Simon Goldschmidt wrote:
> > Am 14.01.2019 um 21:01 schrieb Marek Vasut:
> >> On 1/14/19 8:43 PM, Simon Goldschmidt wrote:
> >>> Am 14.01.2019 um 20:33 schrieb Marek Vasut:
> >>>> On 1/14/19 7:58 PM, Simon Goldschmidt wrote:
> >>>>> Am 14.01.2019 um 19:31 schrieb Marek Vasut:
> >>>>>> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
> >>>>>>> Hi Dinh,
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
> >>>>>>>> Hi Simon,
> >>>>>>>>
> >>>>>>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
> >>>>>>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
> >>>>>>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
> >>>>>>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
> >>>>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>>>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
> >>>>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
> >>>>>>>>>>>>> disabled
> >>>>>>>>>>>>> via defconfig.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the
> >>>>>>>>>>>>> reset
> >>>>>>>>>>>>> drivers
> >>>>>>>>>>>>> don't seem to work with it.
> >>>>>>>>>>>>
> >>>>>>>>>>>> How do you un-reset IP blocks if you disable the reset
> >>>>>>>>>>>> controller ?
> >>>>>>>>>>>
> >>>>>>>>>>> I found that out just now: there's the function
> >>>>>>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
> >>>>>>>>>>> "De-assert reset for peripherals and bridges based on handoff".
> >>>>>>>>>>> However,
> >>>>>>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By
> >>>>>>>>>>> doing
> >>>>>>>>>>> that, it enables *ALL* peripherals on the SoC (except for
> >>>>>>>>>>> some DMA
> >>>>>>>>>>> channels that aren't really used) :-)
> >>>>>>>>>>>
> >>>>>>>>>>> I guess that needs some cleaning up as well ;-)
> >>>>>>>>>>
> >>>>>>>>>> Yes
> >>>>>>>>>>
> >>>>>>>>>>> I think the proper thing to do here would be to remove this
> >>>>>>>>>>> function and
> >>>>>>>>>>> convert all drivers to provide appropriate 'resets'
> >>>>>>>>>>> properties in
> >>>>>>>>>>> the
> >>>>>>>>>>> dts?
> >>>>>>>>>>
> >>>>>>>>>> Indeed
> >>>>>>>>>
> >>>>>>>>> So I just did that and it works nice for SPL and U-Boot: By adding
> >>>>>>>>> some
> >>>>>>>>> "resets" properties the the main dtsi and adding reset bulk
> >>>>>>>>> code to
> >>>>>>>>> the
> >>>>>>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the
> >>>>>>>>> reset
> >>>>>>>>> code from arch/mach_socfpga.
> >>>>>>>>>
> >>>>>>>>> The problem would be that now Linux cannot use peripherals that
> >>>>>>>>> aren't
> >>>>>>>>> enabled by U-Boot because it relies on them being enabled. How are
> >>>>>>>>> such
> >>>>>>>>> dependencies solved? Because even if I would add reset support in
> >>>>>>>>> the
> >>>>>>>>> corresponding Linux drivers, we probably could not bootolder
> >>>>>>>>> Kernels
> >>>>>>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> I added an early reset driver for SoCFPGA that should take care of
> >>>>>>>> this.
> >>>>>>>> The patch is in v5.0-rc2[1].
> >>>>>>>
> >>>>>>> OK, it's good to know that this work is already done, I haven't
> >>>>>>> monitored this close enough.
> >>>>>>
> >>>>>> We had the same problem with A10, indeed.
> >>>>>>
> >>>>>>> But am I correct that my above problem remains even in v5.0 as
> >>>>>>> not all
> >>>>>>> peripherals in socfpga.dtsi have a "resets" property set (e.g.
> >>>>>>> mmc and
> >>>>>>> qspi) and would thuse not be taken out of reset by Linux?
> >>>>>>>
> >>>>>>> Plus: should U-Boot work with older Linux kernels? Because if so, we
> >>>>>>> need fallback code in U-Boot to unreset peripherals when running
> >>>>>>> with an
> >>>>>>> older kernel...
> >>>>>>
> >>>>>> Yes, it'd break old broken kernels . The real question is, do we
> >>>>>> care ?
> >>>>>
> >>>>> Ok, so that at leat shows me I'm going into the right direction :-)
> >>>>>
> >>>>> There are some problems though:
> >>>>> - I do care (we're running 4.9 currently) *g*
> >>>>> - people running an RT kernel will care for a while (until the next
> >>>>> stable RT after fixing this will be released)
> >>>>> - we would currently be breaking *all* kernels, since no kernel should
> >>>>> yet be able to deassert reset for mmc and qspi (unless this is already
> >>>>> done by U-Boot)...
> >>>>>
> >>>>> So would it be OK to add a Kconfig option to U-Boot to keep the
> >>>>> current
> >>>>> behaviour (for old broken kernels like you said) until that code is
> >>>>> spread widely enough? Or is that a no-go?
> >>>>
> >>>> Would be nice to be able to tweak the reset driver behavior at runtime,
> >>>> to unreset things before booting the kernel if the user desires so.
> >>>
> >>> Instead of tweaking the reset driver, we could just add a command that
> >>> does that 'rstmgr->permodrst = 0;' thing my patch would remove.
> >>
> >> I don't want a new custom command.
> >>
> >>> Since noone has complained so far, I think writing 0 should be OK here.
> >>> I don't think it would make too much sense to use the reset handoff
> >>> defines from Quartus output for such a command. I think the way Quartus
> >>> does this is strange anyway...
> >>>
> >>> The question is if defconfigs should be able to use this to
> >>> automatically build a U-Boot config for older kernels. If so, we'd still
> >>> need a Kconfig option?
> >>
> >> I'd much rather have this runtime configurable.
> >
> > Then I'm afraid I don't know what you mean by "runtime configurable".
> > What should be the configuration source that is evaluated at runtime?
>
> I wonder ... maybe an environment variable with a U_BOOT_ENV_CALLBACK()
> hook ?

But when doing it like that, I'd still have to modify the U-Boot
sources to build a version of U-Boot that could successfully boot a
4.9 kernel.
I wanted to be able to do that without changing the sources, that's
why I would have added a Kconfig option...

>
> >>> Thinking further about cleanup: I guess the clock driver is not that
> >>> hard to implement, either. The only thing that's driving me mad is
> >>> pinmux. Is there any chance to get more info from Intel to write this
> >>> properly so we can get rid of that iocsr scanchain defines?
> >>
> >> Clock driver should be easy, yes. Pinmux, I don't know, maybe project
> >> chibi has some information (the cyclone I documentation project).
> >
> > Interesing, I didn't know that project. The only thing I found is a repo
> > on github. But it seems like that one only contains FPGA-related stuff,
> > nothing about the HPS side...
>
> My understanding is that the IOCSR ring is on the FPGA side and it's
> exactly the same as pinmux configuration for the FPGA IO, but I might be
> wrong.

Hmm, I really don't know. Has anyone ever tried asking Intel for this?
Is it a known fact that they won't give away such specs?

Regards,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/14/19 9:30 PM, Simon Goldschmidt wrote:
> On Mon, Jan 14, 2019 at 9:23 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/14/19 9:12 PM, Simon Goldschmidt wrote:
>>> Am 14.01.2019 um 21:01 schrieb Marek Vasut:
>>>> On 1/14/19 8:43 PM, Simon Goldschmidt wrote:
>>>>> Am 14.01.2019 um 20:33 schrieb Marek Vasut:
>>>>>> On 1/14/19 7:58 PM, Simon Goldschmidt wrote:
>>>>>>> Am 14.01.2019 um 19:31 schrieb Marek Vasut:
>>>>>>>> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
>>>>>>>>> Hi Dinh,
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>>>>>>>>>> Hi Simon,
>>>>>>>>>>
>>>>>>>>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>>>>>>>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>>>>>>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>>>>>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>>>>>>>>>> disabled
>>>>>>>>>>>>>>> via defconfig.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the
>>>>>>>>>>>>>>> reset
>>>>>>>>>>>>>>> drivers
>>>>>>>>>>>>>>> don't seem to work with it.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> How do you un-reset IP blocks if you disable the reset
>>>>>>>>>>>>>> controller ?
>>>>>>>>>>>>>
>>>>>>>>>>>>> I found that out just now: there's the function
>>>>>>>>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>>>>>>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>>>>>>>>>> However,
>>>>>>>>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By
>>>>>>>>>>>>> doing
>>>>>>>>>>>>> that, it enables *ALL* peripherals on the SoC (except for
>>>>>>>>>>>>> some DMA
>>>>>>>>>>>>> channels that aren't really used) :-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> I guess that needs some cleaning up as well ;-)
>>>>>>>>>>>>
>>>>>>>>>>>> Yes
>>>>>>>>>>>>
>>>>>>>>>>>>> I think the proper thing to do here would be to remove this
>>>>>>>>>>>>> function and
>>>>>>>>>>>>> convert all drivers to provide appropriate 'resets'
>>>>>>>>>>>>> properties in
>>>>>>>>>>>>> the
>>>>>>>>>>>>> dts?
>>>>>>>>>>>>
>>>>>>>>>>>> Indeed
>>>>>>>>>>>
>>>>>>>>>>> So I just did that and it works nice for SPL and U-Boot: By adding
>>>>>>>>>>> some
>>>>>>>>>>> "resets" properties the the main dtsi and adding reset bulk
>>>>>>>>>>> code to
>>>>>>>>>>> the
>>>>>>>>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the
>>>>>>>>>>> reset
>>>>>>>>>>> code from arch/mach_socfpga.
>>>>>>>>>>>
>>>>>>>>>>> The problem would be that now Linux cannot use peripherals that
>>>>>>>>>>> aren't
>>>>>>>>>>> enabled by U-Boot because it relies on them being enabled. How are
>>>>>>>>>>> such
>>>>>>>>>>> dependencies solved? Because even if I would add reset support in
>>>>>>>>>>> the
>>>>>>>>>>> corresponding Linux drivers, we probably could not bootolder
>>>>>>>>>>> Kernels
>>>>>>>>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I added an early reset driver for SoCFPGA that should take care of
>>>>>>>>>> this.
>>>>>>>>>> The patch is in v5.0-rc2[1].
>>>>>>>>>
>>>>>>>>> OK, it's good to know that this work is already done, I haven't
>>>>>>>>> monitored this close enough.
>>>>>>>>
>>>>>>>> We had the same problem with A10, indeed.
>>>>>>>>
>>>>>>>>> But am I correct that my above problem remains even in v5.0 as
>>>>>>>>> not all
>>>>>>>>> peripherals in socfpga.dtsi have a "resets" property set (e.g.
>>>>>>>>> mmc and
>>>>>>>>> qspi) and would thuse not be taken out of reset by Linux?
>>>>>>>>>
>>>>>>>>> Plus: should U-Boot work with older Linux kernels? Because if so, we
>>>>>>>>> need fallback code in U-Boot to unreset peripherals when running
>>>>>>>>> with an
>>>>>>>>> older kernel...
>>>>>>>>
>>>>>>>> Yes, it'd break old broken kernels . The real question is, do we
>>>>>>>> care ?
>>>>>>>
>>>>>>> Ok, so that at leat shows me I'm going into the right direction :-)
>>>>>>>
>>>>>>> There are some problems though:
>>>>>>> - I do care (we're running 4.9 currently) *g*
>>>>>>> - people running an RT kernel will care for a while (until the next
>>>>>>> stable RT after fixing this will be released)
>>>>>>> - we would currently be breaking *all* kernels, since no kernel should
>>>>>>> yet be able to deassert reset for mmc and qspi (unless this is already
>>>>>>> done by U-Boot)...
>>>>>>>
>>>>>>> So would it be OK to add a Kconfig option to U-Boot to keep the
>>>>>>> current
>>>>>>> behaviour (for old broken kernels like you said) until that code is
>>>>>>> spread widely enough? Or is that a no-go?
>>>>>>
>>>>>> Would be nice to be able to tweak the reset driver behavior at runtime,
>>>>>> to unreset things before booting the kernel if the user desires so.
>>>>>
>>>>> Instead of tweaking the reset driver, we could just add a command that
>>>>> does that 'rstmgr->permodrst = 0;' thing my patch would remove.
>>>>
>>>> I don't want a new custom command.
>>>>
>>>>> Since noone has complained so far, I think writing 0 should be OK here.
>>>>> I don't think it would make too much sense to use the reset handoff
>>>>> defines from Quartus output for such a command. I think the way Quartus
>>>>> does this is strange anyway...
>>>>>
>>>>> The question is if defconfigs should be able to use this to
>>>>> automatically build a U-Boot config for older kernels. If so, we'd still
>>>>> need a Kconfig option?
>>>>
>>>> I'd much rather have this runtime configurable.
>>>
>>> Then I'm afraid I don't know what you mean by "runtime configurable".
>>> What should be the configuration source that is evaluated at runtime?
>>
>> I wonder ... maybe an environment variable with a U_BOOT_ENV_CALLBACK()
>> hook ?
> 
> But when doing it like that, I'd still have to modify the U-Boot
> sources to build a version of U-Boot that could successfully boot a
> 4.9 kernel.
> I wanted to be able to do that without changing the sources, that's
> why I would have added a Kconfig option...

If you have an environment variable which allows you to indicate that
your kernel is broken/old, you can set it and boot.

>>>>> Thinking further about cleanup: I guess the clock driver is not that
>>>>> hard to implement, either. The only thing that's driving me mad is
>>>>> pinmux. Is there any chance to get more info from Intel to write this
>>>>> properly so we can get rid of that iocsr scanchain defines?
>>>>
>>>> Clock driver should be easy, yes. Pinmux, I don't know, maybe project
>>>> chibi has some information (the cyclone I documentation project).
>>>
>>> Interesing, I didn't know that project. The only thing I found is a repo
>>> on github. But it seems like that one only contains FPGA-related stuff,
>>> nothing about the HPS side...
>>
>> My understanding is that the IOCSR ring is on the FPGA side and it's
>> exactly the same as pinmux configuration for the FPGA IO, but I might be
>> wrong.
> 
> Hmm, I really don't know. Has anyone ever tried asking Intel for this?
> Is it a known fact that they won't give away such specs?

Dinh is on CC :)

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Am 14.01.2019 um 21:49 schrieb Marek Vasut:
> On 1/14/19 9:30 PM, Simon Goldschmidt wrote:
>> On Mon, Jan 14, 2019 at 9:23 PM Marek Vasut <marex@denx.de> wrote:
>>>
>>> On 1/14/19 9:12 PM, Simon Goldschmidt wrote:
>>>> Am 14.01.2019 um 21:01 schrieb Marek Vasut:
>>>>> On 1/14/19 8:43 PM, Simon Goldschmidt wrote:
>>>>>> Am 14.01.2019 um 20:33 schrieb Marek Vasut:
>>>>>>> On 1/14/19 7:58 PM, Simon Goldschmidt wrote:
>>>>>>>> Am 14.01.2019 um 19:31 schrieb Marek Vasut:
>>>>>>>>> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
>>>>>>>>>> Hi Dinh,
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>>>>>>>>>>> Hi Simon,
>>>>>>>>>>>
>>>>>>>>>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>>>>>>>>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>>>>>>>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>>>>>>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>>>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>>>>>>>>>>> disabled
>>>>>>>>>>>>>>>> via defconfig.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the
>>>>>>>>>>>>>>>> reset
>>>>>>>>>>>>>>>> drivers
>>>>>>>>>>>>>>>> don't seem to work with it.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> How do you un-reset IP blocks if you disable the reset
>>>>>>>>>>>>>>> controller ?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I found that out just now: there's the function
>>>>>>>>>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>>>>>>>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>>>>>>>>>>> However,
>>>>>>>>>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By
>>>>>>>>>>>>>> doing
>>>>>>>>>>>>>> that, it enables *ALL* peripherals on the SoC (except for
>>>>>>>>>>>>>> some DMA
>>>>>>>>>>>>>> channels that aren't really used) :-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I guess that needs some cleaning up as well ;-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes
>>>>>>>>>>>>>
>>>>>>>>>>>>>> I think the proper thing to do here would be to remove this
>>>>>>>>>>>>>> function and
>>>>>>>>>>>>>> convert all drivers to provide appropriate 'resets'
>>>>>>>>>>>>>> properties in
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> dts?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Indeed
>>>>>>>>>>>>
>>>>>>>>>>>> So I just did that and it works nice for SPL and U-Boot: By adding
>>>>>>>>>>>> some
>>>>>>>>>>>> "resets" properties the the main dtsi and adding reset bulk
>>>>>>>>>>>> code to
>>>>>>>>>>>> the
>>>>>>>>>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the
>>>>>>>>>>>> reset
>>>>>>>>>>>> code from arch/mach_socfpga.
>>>>>>>>>>>>
>>>>>>>>>>>> The problem would be that now Linux cannot use peripherals that
>>>>>>>>>>>> aren't
>>>>>>>>>>>> enabled by U-Boot because it relies on them being enabled. How are
>>>>>>>>>>>> such
>>>>>>>>>>>> dependencies solved? Because even if I would add reset support in
>>>>>>>>>>>> the
>>>>>>>>>>>> corresponding Linux drivers, we probably could not bootolder
>>>>>>>>>>>> Kernels
>>>>>>>>>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I added an early reset driver for SoCFPGA that should take care of
>>>>>>>>>>> this.
>>>>>>>>>>> The patch is in v5.0-rc2[1].
>>>>>>>>>>
>>>>>>>>>> OK, it's good to know that this work is already done, I haven't
>>>>>>>>>> monitored this close enough.
>>>>>>>>>
>>>>>>>>> We had the same problem with A10, indeed.
>>>>>>>>>
>>>>>>>>>> But am I correct that my above problem remains even in v5.0 as
>>>>>>>>>> not all
>>>>>>>>>> peripherals in socfpga.dtsi have a "resets" property set (e.g.
>>>>>>>>>> mmc and
>>>>>>>>>> qspi) and would thuse not be taken out of reset by Linux?
>>>>>>>>>>
>>>>>>>>>> Plus: should U-Boot work with older Linux kernels? Because if so, we
>>>>>>>>>> need fallback code in U-Boot to unreset peripherals when running
>>>>>>>>>> with an
>>>>>>>>>> older kernel...
>>>>>>>>>
>>>>>>>>> Yes, it'd break old broken kernels . The real question is, do we
>>>>>>>>> care ?
>>>>>>>>
>>>>>>>> Ok, so that at leat shows me I'm going into the right direction :-)
>>>>>>>>
>>>>>>>> There are some problems though:
>>>>>>>> - I do care (we're running 4.9 currently) *g*
>>>>>>>> - people running an RT kernel will care for a while (until the next
>>>>>>>> stable RT after fixing this will be released)
>>>>>>>> - we would currently be breaking *all* kernels, since no kernel should
>>>>>>>> yet be able to deassert reset for mmc and qspi (unless this is already
>>>>>>>> done by U-Boot)...
>>>>>>>>
>>>>>>>> So would it be OK to add a Kconfig option to U-Boot to keep the
>>>>>>>> current
>>>>>>>> behaviour (for old broken kernels like you said) until that code is
>>>>>>>> spread widely enough? Or is that a no-go?
>>>>>>>
>>>>>>> Would be nice to be able to tweak the reset driver behavior at runtime,
>>>>>>> to unreset things before booting the kernel if the user desires so.
>>>>>>
>>>>>> Instead of tweaking the reset driver, we could just add a command that
>>>>>> does that 'rstmgr->permodrst = 0;' thing my patch would remove.
>>>>>
>>>>> I don't want a new custom command.
>>>>>
>>>>>> Since noone has complained so far, I think writing 0 should be OK here.
>>>>>> I don't think it would make too much sense to use the reset handoff
>>>>>> defines from Quartus output for such a command. I think the way Quartus
>>>>>> does this is strange anyway...
>>>>>>
>>>>>> The question is if defconfigs should be able to use this to
>>>>>> automatically build a U-Boot config for older kernels. If so, we'd still
>>>>>> need a Kconfig option?
>>>>>
>>>>> I'd much rather have this runtime configurable.
>>>>
>>>> Then I'm afraid I don't know what you mean by "runtime configurable".
>>>> What should be the configuration source that is evaluated at runtime?
>>>
>>> I wonder ... maybe an environment variable with a U_BOOT_ENV_CALLBACK()
>>> hook ?
>>
>> But when doing it like that, I'd still have to modify the U-Boot
>> sources to build a version of U-Boot that could successfully boot a
>> 4.9 kernel.
>> I wanted to be able to do that without changing the sources, that's
>> why I would have added a Kconfig option...
> 
> If you have an environment variable which allows you to indicate that
> your kernel is broken/old, you can set it and boot.

Fair enough. I'll try it like that.

> 
>>>>>> Thinking further about cleanup: I guess the clock driver is not that
>>>>>> hard to implement, either. The only thing that's driving me mad is
>>>>>> pinmux. Is there any chance to get more info from Intel to write this
>>>>>> properly so we can get rid of that iocsr scanchain defines?
>>>>>
>>>>> Clock driver should be easy, yes. Pinmux, I don't know, maybe project
>>>>> chibi has some information (the cyclone I documentation project).
>>>>
>>>> Interesing, I didn't know that project. The only thing I found is a repo
>>>> on github. But it seems like that one only contains FPGA-related stuff,
>>>> nothing about the HPS side...
>>>
>>> My understanding is that the IOCSR ring is on the FPGA side and it's
>>> exactly the same as pinmux configuration for the FPGA IO, but I might be
>>> wrong.
>>
>> Hmm, I really don't know. Has anyone ever tried asking Intel for this?
>> Is it a known fact that they won't give away such specs?
> 
> Dinh is on CC :)

I know ;-)


Regards,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Tom Rini]

On Mon, Jan 14, 2019 at 07:31:26PM +0100, Marek Vasut wrote:
> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
> > Hi Dinh,
> 
> Hi,
> 
> > Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
> >> Hi Simon,
> >>
> >> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
> >>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
> >>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
> >>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
> >>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
> >>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
> >>>>>>> disabled
> >>>>>>> via defconfig.
> >>>>>>>
> >>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
> >>>>>>> drivers
> >>>>>>> don't seem to work with it.
> >>>>>>
> >>>>>> How do you un-reset IP blocks if you disable the reset controller ?
> >>>>>
> >>>>> I found that out just now: there's the function
> >>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
> >>>>> "De-assert reset for peripherals and bridges based on handoff".
> >>>>> However,
> >>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
> >>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
> >>>>> channels that aren't really used) :-)
> >>>>>
> >>>>> I guess that needs some cleaning up as well ;-)
> >>>>
> >>>> Yes
> >>>>
> >>>>> I think the proper thing to do here would be to remove this
> >>>>> function and
> >>>>> convert all drivers to provide appropriate 'resets' properties in the
> >>>>> dts?
> >>>>
> >>>> Indeed
> >>>
> >>> So I just did that and it works nice for SPL and U-Boot: By adding some
> >>> "resets" properties the the main dtsi and adding reset bulk code to the
> >>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the reset
> >>> code from arch/mach_socfpga.
> >>>
> >>> The problem would be that now Linux cannot use peripherals that aren't
> >>> enabled by U-Boot because it relies on them being enabled. How are such
> >>> dependencies solved? Because even if I would add reset support in the
> >>> corresponding Linux drivers, we probably could not bootolder Kernels
> >>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
> >>>
> >>
> >> I added an early reset driver for SoCFPGA that should take care of this.
> >> The patch is in v5.0-rc2[1].
> > 
> > OK, it's good to know that this work is already done, I haven't
> > monitored this close enough.
> 
> We had the same problem with A10, indeed.
> 
> > But am I correct that my above problem remains even in v5.0 as not all
> > peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
> > qspi) and would thuse not be taken out of reset by Linux?
> > 
> > Plus: should U-Boot work with older Linux kernels? Because if so, we
> > need fallback code in U-Boot to unreset peripherals when running with an
> > older kernel...
> 
> Yes, it'd break old broken kernels . The real question is, do we care ?

Yes, we care.  Especially since it sounds like we're talking about
something that's an LTS and not super-ancient vendor kernel.  Off the
top of my head I can't recall if we ever fully removed support in sunxi
for the vendor kernel in some cases, or just made it, eventually, opt-in
as it was a fairly annoying incompatible behavior case.

But yes, in general, we do care about old kernels and need to be loud
and clear about when we're removing support for them on a given SoC due
to it being a PITA to support both ways of doing X and people have had Y
years to migrate or correct their kernel.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190114/42d1c6ca/attachment.sig>

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/14/19 10:28 PM, Tom Rini wrote:
> On Mon, Jan 14, 2019 at 07:31:26PM +0100, Marek Vasut wrote:
>> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
>>> Hi Dinh,
>>
>> Hi,
>>
>>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>>>> Hi Simon,
>>>>
>>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>>>> disabled
>>>>>>>>> via defconfig.
>>>>>>>>>
>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
>>>>>>>>> drivers
>>>>>>>>> don't seem to work with it.
>>>>>>>>
>>>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>>>
>>>>>>> I found that out just now: there's the function
>>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>>>> However,
>>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
>>>>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>>>>>>> channels that aren't really used) :-)
>>>>>>>
>>>>>>> I guess that needs some cleaning up as well ;-)
>>>>>>
>>>>>> Yes
>>>>>>
>>>>>>> I think the proper thing to do here would be to remove this
>>>>>>> function and
>>>>>>> convert all drivers to provide appropriate 'resets' properties in the
>>>>>>> dts?
>>>>>>
>>>>>> Indeed
>>>>>
>>>>> So I just did that and it works nice for SPL and U-Boot: By adding some
>>>>> "resets" properties the the main dtsi and adding reset bulk code to the
>>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the reset
>>>>> code from arch/mach_socfpga.
>>>>>
>>>>> The problem would be that now Linux cannot use peripherals that aren't
>>>>> enabled by U-Boot because it relies on them being enabled. How are such
>>>>> dependencies solved? Because even if I would add reset support in the
>>>>> corresponding Linux drivers, we probably could not bootolder Kernels
>>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>>>
>>>>
>>>> I added an early reset driver for SoCFPGA that should take care of this.
>>>> The patch is in v5.0-rc2[1].
>>>
>>> OK, it's good to know that this work is already done, I haven't
>>> monitored this close enough.
>>
>> We had the same problem with A10, indeed.
>>
>>> But am I correct that my above problem remains even in v5.0 as not all
>>> peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
>>> qspi) and would thuse not be taken out of reset by Linux?
>>>
>>> Plus: should U-Boot work with older Linux kernels? Because if so, we
>>> need fallback code in U-Boot to unreset peripherals when running with an
>>> older kernel...
>>
>> Yes, it'd break old broken kernels . The real question is, do we care ?
> 
> Yes, we care.  Especially since it sounds like we're talking about
> something that's an LTS and not super-ancient vendor kernel.  Off the
> top of my head I can't recall if we ever fully removed support in sunxi
> for the vendor kernel in some cases, or just made it, eventually, opt-in
> as it was a fairly annoying incompatible behavior case.
> 
> But yes, in general, we do care about old kernels and need to be loud
> and clear about when we're removing support for them on a given SoC due
> to it being a PITA to support both ways of doing X and people have had Y
> years to migrate or correct their kernel.

Then we have to add some fallback mechanism, possibly the env variable
to tell reset controller to unreset everything.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Am 14.01.2019 um 22:30 schrieb Marek Vasut:
> On 1/14/19 10:28 PM, Tom Rini wrote:
>> On Mon, Jan 14, 2019 at 07:31:26PM +0100, Marek Vasut wrote:
>>> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
>>>> Hi Dinh,
>>>
>>> Hi,
>>>
>>>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>>>>> Hi Simon,
>>>>>
>>>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>>>>> disabled
>>>>>>>>>> via defconfig.
>>>>>>>>>>
>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
>>>>>>>>>> drivers
>>>>>>>>>> don't seem to work with it.
>>>>>>>>>
>>>>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>>>>
>>>>>>>> I found that out just now: there's the function
>>>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>>>>> However,
>>>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
>>>>>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>>>>>>>> channels that aren't really used) :-)
>>>>>>>>
>>>>>>>> I guess that needs some cleaning up as well ;-)
>>>>>>>
>>>>>>> Yes
>>>>>>>
>>>>>>>> I think the proper thing to do here would be to remove this
>>>>>>>> function and
>>>>>>>> convert all drivers to provide appropriate 'resets' properties in the
>>>>>>>> dts?
>>>>>>>
>>>>>>> Indeed
>>>>>>
>>>>>> So I just did that and it works nice for SPL and U-Boot: By adding some
>>>>>> "resets" properties the the main dtsi and adding reset bulk code to the
>>>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the reset
>>>>>> code from arch/mach_socfpga.
>>>>>>
>>>>>> The problem would be that now Linux cannot use peripherals that aren't
>>>>>> enabled by U-Boot because it relies on them being enabled. How are such
>>>>>> dependencies solved? Because even if I would add reset support in the
>>>>>> corresponding Linux drivers, we probably could not bootolder Kernels
>>>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>>>>
>>>>>
>>>>> I added an early reset driver for SoCFPGA that should take care of this.
>>>>> The patch is in v5.0-rc2[1].
>>>>
>>>> OK, it's good to know that this work is already done, I haven't
>>>> monitored this close enough.
>>>
>>> We had the same problem with A10, indeed.
>>>
>>>> But am I correct that my above problem remains even in v5.0 as not all
>>>> peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
>>>> qspi) and would thuse not be taken out of reset by Linux?
>>>>
>>>> Plus: should U-Boot work with older Linux kernels? Because if so, we
>>>> need fallback code in U-Boot to unreset peripherals when running with an
>>>> older kernel...
>>>
>>> Yes, it'd break old broken kernels . The real question is, do we care ?
>>
>> Yes, we care.  Especially since it sounds like we're talking about
>> something that's an LTS and not super-ancient vendor kernel.  Off the
>> top of my head I can't recall if we ever fully removed support in sunxi
>> for the vendor kernel in some cases, or just made it, eventually, opt-in
>> as it was a fairly annoying incompatible behavior case.
>>
>> But yes, in general, we do care about old kernels and need to be loud
>> and clear about when we're removing support for them on a given SoC due
>> to it being a PITA to support both ways of doing X and people have had Y
>> years to migrate or correct their kernel.
> 
> Then we have to add some fallback mechanism, possibly the env variable
> to tell reset controller to unreset everything.

So is an env variable good enough as opt-in or opt-out? How was the 
sunxi thing handled? My feeling is that it would be better to have a 
Kconfig option for such things, to make it just boot after compiling it, 
without relying on the manual act of changing the environment.

But I'm just some user, I'll leave it up to you two to decide ;-)

Regards,
Simon

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Tom Rini]

On Mon, Jan 14, 2019 at 10:30:36PM +0100, Marek Vasut wrote:
> On 1/14/19 10:28 PM, Tom Rini wrote:
> > On Mon, Jan 14, 2019 at 07:31:26PM +0100, Marek Vasut wrote:
> >> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
> >>> Hi Dinh,
> >>
> >> Hi,
> >>
> >>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
> >>>> Hi Simon,
> >>>>
> >>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
> >>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
> >>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
> >>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
> >>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
> >>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
> >>>>>>>>> disabled
> >>>>>>>>> via defconfig.
> >>>>>>>>>
> >>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
> >>>>>>>>> drivers
> >>>>>>>>> don't seem to work with it.
> >>>>>>>>
> >>>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
> >>>>>>>
> >>>>>>> I found that out just now: there's the function
> >>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
> >>>>>>> "De-assert reset for peripherals and bridges based on handoff".
> >>>>>>> However,
> >>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
> >>>>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
> >>>>>>> channels that aren't really used) :-)
> >>>>>>>
> >>>>>>> I guess that needs some cleaning up as well ;-)
> >>>>>>
> >>>>>> Yes
> >>>>>>
> >>>>>>> I think the proper thing to do here would be to remove this
> >>>>>>> function and
> >>>>>>> convert all drivers to provide appropriate 'resets' properties in the
> >>>>>>> dts?
> >>>>>>
> >>>>>> Indeed
> >>>>>
> >>>>> So I just did that and it works nice for SPL and U-Boot: By adding some
> >>>>> "resets" properties the the main dtsi and adding reset bulk code to the
> >>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the reset
> >>>>> code from arch/mach_socfpga.
> >>>>>
> >>>>> The problem would be that now Linux cannot use peripherals that aren't
> >>>>> enabled by U-Boot because it relies on them being enabled. How are such
> >>>>> dependencies solved? Because even if I would add reset support in the
> >>>>> corresponding Linux drivers, we probably could not bootolder Kernels
> >>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
> >>>>>
> >>>>
> >>>> I added an early reset driver for SoCFPGA that should take care of this.
> >>>> The patch is in v5.0-rc2[1].
> >>>
> >>> OK, it's good to know that this work is already done, I haven't
> >>> monitored this close enough.
> >>
> >> We had the same problem with A10, indeed.
> >>
> >>> But am I correct that my above problem remains even in v5.0 as not all
> >>> peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
> >>> qspi) and would thuse not be taken out of reset by Linux?
> >>>
> >>> Plus: should U-Boot work with older Linux kernels? Because if so, we
> >>> need fallback code in U-Boot to unreset peripherals when running with an
> >>> older kernel...
> >>
> >> Yes, it'd break old broken kernels . The real question is, do we care ?
> > 
> > Yes, we care.  Especially since it sounds like we're talking about
> > something that's an LTS and not super-ancient vendor kernel.  Off the
> > top of my head I can't recall if we ever fully removed support in sunxi
> > for the vendor kernel in some cases, or just made it, eventually, opt-in
> > as it was a fairly annoying incompatible behavior case.
> > 
> > But yes, in general, we do care about old kernels and need to be loud
> > and clear about when we're removing support for them on a given SoC due
> > to it being a PITA to support both ways of doing X and people have had Y
> > years to migrate or correct their kernel.
> 
> Then we have to add some fallback mechanism, possibly the env variable
> to tell reset controller to unreset everything.

Alright, so the prior art in question is:
commit accc9e446be6c3bd129315a0c5830bddccfa16da
Author: Hans de Goede <hdegoede@redhat.com>
Date:   Wed Oct 22 14:56:36 2014 +0200

    sunxi: Add CONFIG_OLD_SUNXI_KERNEL_COMPAT Kconfig option

And is about supporting the sunxi 3.4 vendor kernel tree, and something
we have still (and a single enabler of).  So, what's the range of broken
/ not broken kernels here?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190114/7a318a2f/attachment.sig>

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Am Mo., 14. Jan. 2019, 22:50 hat Tom Rini <trini@konsulko.com> geschrieben:

> On Mon, Jan 14, 2019 at 10:30:36PM +0100, Marek Vasut wrote:
> > On 1/14/19 10:28 PM, Tom Rini wrote:
> > > On Mon, Jan 14, 2019 at 07:31:26PM +0100, Marek Vasut wrote:
> > >> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
> > >>> Hi Dinh,
> > >>
> > >> Hi,
> > >>
> > >>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
> > >>>> Hi Simon,
> > >>>>
> > >>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
> > >>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
> > >>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
> > >>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
> > >>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> > >>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
> > >>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
> > >>>>>>>>> disabled
> > >>>>>>>>> via defconfig.
> > >>>>>>>>>
> > >>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
> > >>>>>>>>> drivers
> > >>>>>>>>> don't seem to work with it.
> > >>>>>>>>
> > >>>>>>>> How do you un-reset IP blocks if you disable the reset
> controller ?
> > >>>>>>>
> > >>>>>>> I found that out just now: there's the function
> > >>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
> > >>>>>>> "De-assert reset for peripherals and bridges based on handoff".
> > >>>>>>> However,
> > >>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By
> doing
> > >>>>>>> that, it enables *ALL* peripherals on the SoC (except for some
> DMA
> > >>>>>>> channels that aren't really used) :-)
> > >>>>>>>
> > >>>>>>> I guess that needs some cleaning up as well ;-)
> > >>>>>>
> > >>>>>> Yes
> > >>>>>>
> > >>>>>>> I think the proper thing to do here would be to remove this
> > >>>>>>> function and
> > >>>>>>> convert all drivers to provide appropriate 'resets' properties
> in the
> > >>>>>>> dts?
> > >>>>>>
> > >>>>>> Indeed
> > >>>>>
> > >>>>> So I just did that and it works nice for SPL and U-Boot: By adding
> some
> > >>>>> "resets" properties the the main dtsi and adding reset bulk code
> to the
> > >>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the
> reset
> > >>>>> code from arch/mach_socfpga.
> > >>>>>
> > >>>>> The problem would be that now Linux cannot use peripherals that
> aren't
> > >>>>> enabled by U-Boot because it relies on them being enabled. How are
> such
> > >>>>> dependencies solved? Because even if I would add reset support in
> the
> > >>>>> corresponding Linux drivers, we probably could not bootolder
> Kernels
> > >>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
> > >>>>>
> > >>>>
> > >>>> I added an early reset driver for SoCFPGA that should take care of
> this.
> > >>>> The patch is in v5.0-rc2[1].
> > >>>
> > >>> OK, it's good to know that this work is already done, I haven't
> > >>> monitored this close enough.
> > >>
> > >> We had the same problem with A10, indeed.
> > >>
> > >>> But am I correct that my above problem remains even in v5.0 as not
> all
> > >>> peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc
> and
> > >>> qspi) and would thuse not be taken out of reset by Linux?
> > >>>
> > >>> Plus: should U-Boot work with older Linux kernels? Because if so, we
> > >>> need fallback code in U-Boot to unreset peripherals when running
> with an
> > >>> older kernel...
> > >>
> > >> Yes, it'd break old broken kernels . The real question is, do we care
> ?
> > >
> > > Yes, we care.  Especially since it sounds like we're talking about
> > > something that's an LTS and not super-ancient vendor kernel.  Off the
> > > top of my head I can't recall if we ever fully removed support in sunxi
> > > for the vendor kernel in some cases, or just made it, eventually,
> opt-in
> > > as it was a fairly annoying incompatible behavior case.
> > >
> > > But yes, in general, we do care about old kernels and need to be loud
> > > and clear about when we're removing support for them on a given SoC due
> > > to it being a PITA to support both ways of doing X and people have had
> Y
> > > years to migrate or correct their kernel.
> >
> > Then we have to add some fallback mechanism, possibly the env variable
> > to tell reset controller to unreset everything.
>
> Alright, so the prior art in question is:
> commit accc9e446be6c3bd129315a0c5830bddccfa16da
> Author: Hans de Goede <hdegoede@redhat.com>
> Date:   Wed Oct 22 14:56:36 2014 +0200
>
>     sunxi: Add CONFIG_OLD_SUNXI_KERNEL_COMPAT Kconfig option
>
> And is about supporting the sunxi 3.4 vendor kernel tree, and something
> we have still (and a single enabler of).  So, what's the range of broken
> / not broken kernels here?
>

I haven't yet tested but my idea is that it is still broken (some of the
devices won't come out of reset in some scenarios).

So I guess such a Kconfig option would even need to be enabled by default
for some time...

Regards,
Simon

>

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Marek Vasut]

On 1/14/19 10:53 PM, Simon Goldschmidt wrote:
> 
> 
> Am Mo., 14. Jan. 2019, 22:50 hat Tom Rini <trini@konsulko.com
> <mailto:trini@konsulko.com>> geschrieben:
> 
>     On Mon, Jan 14, 2019 at 10:30:36PM +0100, Marek Vasut wrote:
>     > On 1/14/19 10:28 PM, Tom Rini wrote:
>     > > On Mon, Jan 14, 2019 at 07:31:26PM +0100, Marek Vasut wrote:
>     > >> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
>     > >>> Hi Dinh,
>     > >>
>     > >> Hi,
>     > >>
>     > >>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>     > >>>> Hi Simon,
>     > >>>>
>     > >>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>     > >>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>     > >>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>     > >>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>     > >>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>     > >>>>>>>>> In order to build a smaller SPL, let's imply
>     SPL_DM_RESET and
>     > >>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they
>     can be
>     > >>>>>>>>> disabled
>     > >>>>>>>>> via defconfig.
>     > >>>>>>>>>
>     > >>>>>>>>> This also seems to be required to use OF_PLATDATA, as
>     the reset
>     > >>>>>>>>> drivers
>     > >>>>>>>>> don't seem to work with it.
>     > >>>>>>>>
>     > >>>>>>>> How do you un-reset IP blocks if you disable the reset
>     controller ?
>     > >>>>>>>
>     > >>>>>>> I found that out just now: there's the function
>     > >>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that
>     should
>     > >>>>>>> "De-assert reset for peripherals and bridges based on
>     handoff".
>     > >>>>>>> However,
>     > >>>>>>> at least for Gen5, it just writes a 0 to
>     rstmgr->permodrst. By doing
>     > >>>>>>> that, it enables *ALL* peripherals on the SoC (except for
>     some DMA
>     > >>>>>>> channels that aren't really used) :-)
>     > >>>>>>>
>     > >>>>>>> I guess that needs some cleaning up as well ;-)
>     > >>>>>>
>     > >>>>>> Yes
>     > >>>>>>
>     > >>>>>>> I think the proper thing to do here would be to remove this
>     > >>>>>>> function and
>     > >>>>>>> convert all drivers to provide appropriate 'resets'
>     properties in the
>     > >>>>>>> dts?
>     > >>>>>>
>     > >>>>>> Indeed
>     > >>>>>
>     > >>>>> So I just did that and it works nice for SPL and U-Boot: By
>     adding some
>     > >>>>> "resets" properties the the main dtsi and adding reset bulk
>     code to the
>     > >>>>> cadence_qspi, denali_dt nand and drivers, I can nearly
>     remove the reset
>     > >>>>> code from arch/mach_socfpga.
>     > >>>>>
>     > >>>>> The problem would be that now Linux cannot use peripherals
>     that aren't
>     > >>>>> enabled by U-Boot because it relies on them being enabled.
>     How are such
>     > >>>>> dependencies solved? Because even if I would add reset
>     support in the
>     > >>>>> corresponding Linux drivers, we probably could not bootolder
>     Kernels
>     > >>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>     > >>>>>
>     > >>>>
>     > >>>> I added an early reset driver for SoCFPGA that should take
>     care of this.
>     > >>>> The patch is in v5.0-rc2[1].
>     > >>>
>     > >>> OK, it's good to know that this work is already done, I haven't
>     > >>> monitored this close enough.
>     > >>
>     > >> We had the same problem with A10, indeed.
>     > >>
>     > >>> But am I correct that my above problem remains even in v5.0 as
>     not all
>     > >>> peripherals in socfpga.dtsi have a "resets" property set (e.g.
>     mmc and
>     > >>> qspi) and would thuse not be taken out of reset by Linux?
>     > >>>
>     > >>> Plus: should U-Boot work with older Linux kernels? Because if
>     so, we
>     > >>> need fallback code in U-Boot to unreset peripherals when
>     running with an
>     > >>> older kernel...
>     > >>
>     > >> Yes, it'd break old broken kernels . The real question is, do
>     we care ?
>     > >
>     > > Yes, we care.  Especially since it sounds like we're talking about
>     > > something that's an LTS and not super-ancient vendor kernel. 
>     Off the
>     > > top of my head I can't recall if we ever fully removed support
>     in sunxi
>     > > for the vendor kernel in some cases, or just made it,
>     eventually, opt-in
>     > > as it was a fairly annoying incompatible behavior case.
>     > >
>     > > But yes, in general, we do care about old kernels and need to be
>     loud
>     > > and clear about when we're removing support for them on a given
>     SoC due
>     > > to it being a PITA to support both ways of doing X and people
>     have had Y
>     > > years to migrate or correct their kernel.
>     >
>     > Then we have to add some fallback mechanism, possibly the env variable
>     > to tell reset controller to unreset everything.
> 
>     Alright, so the prior art in question is:
>     commit accc9e446be6c3bd129315a0c5830bddccfa16da
>     Author: Hans de Goede <hdegoede at redhat.com <mailto:hdegoede@redhat.com>>
>     Date:   Wed Oct 22 14:56:36 2014 +0200
> 
>         sunxi: Add CONFIG_OLD_SUNXI_KERNEL_COMPAT Kconfig option
> 
>     And is about supporting the sunxi 3.4 vendor kernel tree, and something
>     we have still (and a single enabler of).  So, what's the range of broken
>     / not broken kernels here?
> 
> 
> I haven't yet tested but my idea is that it is still broken (some of the
> devices won't come out of reset in some scenarios).
> 
> So I guess such a Kconfig option would even need to be enabled by
> default for some time...

Well, if you had a variable to configure that at runtime, you won't need
another ad-hoc Kconfig option .

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Dinh Nguyen]

On 1/14/19 10:05 AM, Simon Goldschmidt wrote:
> Hi Dinh,
> 
> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>> Hi Simon,
>>
>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>> disabled
>>>>>>> via defconfig.
>>>>>>>
>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
>>>>>>> drivers
>>>>>>> don't seem to work with it.
>>>>>>
>>>>>> How do you un-reset IP blocks if you disable the reset controller ?
>>>>>
>>>>> I found that out just now: there's the function
>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>> However,
>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
>>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>>>>> channels that aren't really used) :-)
>>>>>
>>>>> I guess that needs some cleaning up as well ;-)
>>>>
>>>> Yes
>>>>
>>>>> I think the proper thing to do here would be to remove this
>>>>> function and
>>>>> convert all drivers to provide appropriate 'resets' properties in the
>>>>> dts?
>>>>
>>>> Indeed
>>>
>>> So I just did that and it works nice for SPL and U-Boot: By adding some
>>> "resets" properties the the main dtsi and adding reset bulk code to the
>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the reset
>>> code from arch/mach_socfpga.
>>>
>>> The problem would be that now Linux cannot use peripherals that aren't
>>> enabled by U-Boot because it relies on them being enabled. How are such
>>> dependencies solved? Because even if I would add reset support in the
>>> corresponding Linux drivers, we probably could not bootolder Kernels
>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>
>>
>> I added an early reset driver for SoCFPGA that should take care of this.
>> The patch is in v5.0-rc2[1].
> 
> OK, it's good to know that this work is already done, I haven't
> monitored this close enough.
> 
> But am I correct that my above problem remains even in v5.0 as not all
> peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
> qspi) and would thuse not be taken out of reset by Linux?
> 

Yes, I see that. I will send a patch for those.

Dinh

[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

On Mon, Jan 14, 2019 at 11:26 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
>
>
>
> On 1/14/19 10:05 AM, Simon Goldschmidt wrote:
> > Hi Dinh,
> >
> > Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
> >> Hi Simon,
> >>
> >> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
> >>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
> >>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
> >>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
> >>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
> >>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
> >>>>>>> disabled
> >>>>>>> via defconfig.
> >>>>>>>
> >>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
> >>>>>>> drivers
> >>>>>>> don't seem to work with it.
> >>>>>>
> >>>>>> How do you un-reset IP blocks if you disable the reset controller ?
> >>>>>
> >>>>> I found that out just now: there's the function
> >>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
> >>>>> "De-assert reset for peripherals and bridges based on handoff".
> >>>>> However,
> >>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By doing
> >>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
> >>>>> channels that aren't really used) :-)
> >>>>>
> >>>>> I guess that needs some cleaning up as well ;-)
> >>>>
> >>>> Yes
> >>>>
> >>>>> I think the proper thing to do here would be to remove this
> >>>>> function and
> >>>>> convert all drivers to provide appropriate 'resets' properties in the
> >>>>> dts?
> >>>>
> >>>> Indeed
> >>>
> >>> So I just did that and it works nice for SPL and U-Boot: By adding some
> >>> "resets" properties the the main dtsi and adding reset bulk code to the
> >>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the reset
> >>> code from arch/mach_socfpga.
> >>>
> >>> The problem would be that now Linux cannot use peripherals that aren't
> >>> enabled by U-Boot because it relies on them being enabled. How are such
> >>> dependencies solved? Because even if I would add reset support in the
> >>> corresponding Linux drivers, we probably could not bootolder Kernels
> >>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
> >>>
> >>
> >> I added an early reset driver for SoCFPGA that should take care of this.
> >> The patch is in v5.0-rc2[1].
> >
> > OK, it's good to know that this work is already done, I haven't
> > monitored this close enough.
> >
> > But am I correct that my above problem remains even in v5.0 as not all
> > peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
> > qspi) and would thuse not be taken out of reset by Linux?
> >
>
> Yes, I see that. I will send a patch for those.

OK, great! I don't know the driver structure in Linux as well, will that be a
dts change only or does it require changing the mmc/qspi drivers to deassert
those resets?

The reset handles are also missing for yet more peripherals, I have prepared
this as a patch for U-Boot, but I guess the devicetree is still in sync, so I
could send this patch for Linux as well?

Regards,
Simon

[U-Boot] [PATCH v1 2/4] arm: socfpga: fix compiling with OF_PLATDATA

[Simon Goldschmidt]

'socfpga_eth_reset_common' must not be compiled when OF_PLATDATA is enabled
since it uses functions accessing the devicetree.

Since this function is not used in SPL anyway, change the compile guard
to reflect this and fix compiling SPL with OF_PLATDATA.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

 arch/arm/mach-socfpga/misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
index 78fbe28724..30f74d9940 100644
--- a/arch/arm/mach-socfpga/misc.c
+++ b/arch/arm/mach-socfpga/misc.c
@@ -120,7 +120,7 @@ int arch_cpu_init(void)
 	return 0;
 }
 
-#ifdef CONFIG_ETH_DESIGNWARE
+#if defined CONFIG_ETH_DESIGNWARE && !defined CONFIG_SPL_BUILD
 static int dwmac_phymode_to_modereg(const char *phymode, u32 *modereg)
 {
 	if (!phymode)
-- 
2.17.1

[U-Boot] [PATCH v1 2/4] arm: socfpga: fix compiling with OF_PLATDATA

[Marek Vasut]

On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> 'socfpga_eth_reset_common' must not be compiled when OF_PLATDATA is enabled
> since it uses functions accessing the devicetree.
> 
> Since this function is not used in SPL anyway, change the compile guard
> to reflect this and fix compiling SPL with OF_PLATDATA.

Doesn't this break ethernet in SPL ? I think it does. The real fix is to
move this PHY mode configuration into the driver.

> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
> 
>  arch/arm/mach-socfpga/misc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
> index 78fbe28724..30f74d9940 100644
> --- a/arch/arm/mach-socfpga/misc.c
> +++ b/arch/arm/mach-socfpga/misc.c
> @@ -120,7 +120,7 @@ int arch_cpu_init(void)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_ETH_DESIGNWARE
> +#if defined CONFIG_ETH_DESIGNWARE && !defined CONFIG_SPL_BUILD
>  static int dwmac_phymode_to_modereg(const char *phymode, u32 *modereg)
>  {
>  	if (!phymode)
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 2/4] arm: socfpga: fix compiling with OF_PLATDATA

[Simon Goldschmidt]

On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> > 'socfpga_eth_reset_common' must not be compiled when OF_PLATDATA is enabled
> > since it uses functions accessing the devicetree.
> >
> > Since this function is not used in SPL anyway, change the compile guard
> > to reflect this and fix compiling SPL with OF_PLATDATA.
>
> Doesn't this break ethernet in SPL ? I think it does. The real fix is to
> move this PHY mode configuration into the driver.

Hmm, ethernet in SPL. Haven't thought of that :-)

While it could be easy to move the DTS part of  PHY mode configuration into
the driver, moving the reset portion of that code into the driver would be ugly
unless using proper reset management?

>
> > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > ---
> >
> >  arch/arm/mach-socfpga/misc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
> > index 78fbe28724..30f74d9940 100644
> > --- a/arch/arm/mach-socfpga/misc.c
> > +++ b/arch/arm/mach-socfpga/misc.c
> > @@ -120,7 +120,7 @@ int arch_cpu_init(void)
> >       return 0;
> >  }
> >
> > -#ifdef CONFIG_ETH_DESIGNWARE
> > +#if defined CONFIG_ETH_DESIGNWARE && !defined CONFIG_SPL_BUILD

Would it work for you to change this into:

#if defined CONFIG_ETH_DESIGNWARE && !CONFIG_IS_ENABLED(OF_PLATDATA)

We could still fix the reset code when actually using OF_PLATDATA with
ethernet...

Regards,
Simon

> >  static int dwmac_phymode_to_modereg(const char *phymode, u32 *modereg)
> >  {
> >       if (!phymode)
> >
>
>
> --
> Best regards,
> Marek Vasut

[U-Boot] [PATCH v1 2/4] arm: socfpga: fix compiling with OF_PLATDATA

[Marek Vasut]

On 1/8/19 7:32 AM, Simon Goldschmidt wrote:
> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>> 'socfpga_eth_reset_common' must not be compiled when OF_PLATDATA is enabled
>>> since it uses functions accessing the devicetree.
>>>
>>> Since this function is not used in SPL anyway, change the compile guard
>>> to reflect this and fix compiling SPL with OF_PLATDATA.
>>
>> Doesn't this break ethernet in SPL ? I think it does. The real fix is to
>> move this PHY mode configuration into the driver.
> 
> Hmm, ethernet in SPL. Haven't thought of that :-)
> 
> While it could be easy to move the DTS part of  PHY mode configuration into
> the driver, moving the reset portion of that code into the driver would be ugly
> unless using proper reset management?

Well, it's still better than having it live in arch/arm/ , which was at
that point the best option, but right now it's no longer. And I think
Joyce ran into similar topic with some other Altera IP.

>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>> ---
>>>
>>>  arch/arm/mach-socfpga/misc.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
>>> index 78fbe28724..30f74d9940 100644
>>> --- a/arch/arm/mach-socfpga/misc.c
>>> +++ b/arch/arm/mach-socfpga/misc.c
>>> @@ -120,7 +120,7 @@ int arch_cpu_init(void)
>>>       return 0;
>>>  }
>>>
>>> -#ifdef CONFIG_ETH_DESIGNWARE
>>> +#if defined CONFIG_ETH_DESIGNWARE && !defined CONFIG_SPL_BUILD
> 
> Would it work for you to change this into:
> 
> #if defined CONFIG_ETH_DESIGNWARE && !CONFIG_IS_ENABLED(OF_PLATDATA)
> 
> We could still fix the reset code when actually using OF_PLATDATA with
> ethernet...

Is it too much work to fix it properly ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 2/4] arm: socfpga: fix compiling with OF_PLATDATA

[Simon Goldschmidt]

On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/8/19 7:32 AM, Simon Goldschmidt wrote:
> > On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>> 'socfpga_eth_reset_common' must not be compiled when OF_PLATDATA is enabled
> >>> since it uses functions accessing the devicetree.
> >>>
> >>> Since this function is not used in SPL anyway, change the compile guard
> >>> to reflect this and fix compiling SPL with OF_PLATDATA.
> >>
> >> Doesn't this break ethernet in SPL ? I think it does. The real fix is to
> >> move this PHY mode configuration into the driver.
> >
> > Hmm, ethernet in SPL. Haven't thought of that :-)
> >
> > While it could be easy to move the DTS part of  PHY mode configuration into
> > the driver, moving the reset portion of that code into the driver would be ugly
> > unless using proper reset management?
>
> Well, it's still better than having it live in arch/arm/ , which was at
> that point the best option, but right now it's no longer. And I think
> Joyce ran into similar topic with some other Altera IP.

Hmm, ok, I could search for that.

> >>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> >>> ---
> >>>
> >>>  arch/arm/mach-socfpga/misc.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
> >>> index 78fbe28724..30f74d9940 100644
> >>> --- a/arch/arm/mach-socfpga/misc.c
> >>> +++ b/arch/arm/mach-socfpga/misc.c
> >>> @@ -120,7 +120,7 @@ int arch_cpu_init(void)
> >>>       return 0;
> >>>  }
> >>>
> >>> -#ifdef CONFIG_ETH_DESIGNWARE
> >>> +#if defined CONFIG_ETH_DESIGNWARE && !defined CONFIG_SPL_BUILD
> >
> > Would it work for you to change this into:
> >
> > #if defined CONFIG_ETH_DESIGNWARE && !CONFIG_IS_ENABLED(OF_PLATDATA)
> >
> > We could still fix the reset code when actually using OF_PLATDATA with
> > ethernet...
>
> Is it too much work to fix it properly ?

Well, just moving the functions I commented out into the eth driver would mean
it would at some point call back into arch/arm/mach-socfpga code.

If that is acceptable, it wouldn't be too much work and I could certainly do it.
But the question remains whether such a change would "fix it properly". In my
opinion, it wouldn't. But it would probably still be better than the
current state
where some code in arch/arm parses ETH DTS nodes...

Regards,
Simon

[U-Boot] [PATCH v1 2/4] arm: socfpga: fix compiling with OF_PLATDATA

[Marek Vasut]

On 1/8/19 1:14 PM, Simon Goldschmidt wrote:
> On Tue, Jan 8, 2019 at 1:05 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/8/19 7:32 AM, Simon Goldschmidt wrote:
>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>> 'socfpga_eth_reset_common' must not be compiled when OF_PLATDATA is enabled
>>>>> since it uses functions accessing the devicetree.
>>>>>
>>>>> Since this function is not used in SPL anyway, change the compile guard
>>>>> to reflect this and fix compiling SPL with OF_PLATDATA.
>>>>
>>>> Doesn't this break ethernet in SPL ? I think it does. The real fix is to
>>>> move this PHY mode configuration into the driver.
>>>
>>> Hmm, ethernet in SPL. Haven't thought of that :-)
>>>
>>> While it could be easy to move the DTS part of  PHY mode configuration into
>>> the driver, moving the reset portion of that code into the driver would be ugly
>>> unless using proper reset management?
>>
>> Well, it's still better than having it live in arch/arm/ , which was at
>> that point the best option, but right now it's no longer. And I think
>> Joyce ran into similar topic with some other Altera IP.
> 
> Hmm, ok, I could search for that.
> 
>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>>>> ---
>>>>>
>>>>>  arch/arm/mach-socfpga/misc.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
>>>>> index 78fbe28724..30f74d9940 100644
>>>>> --- a/arch/arm/mach-socfpga/misc.c
>>>>> +++ b/arch/arm/mach-socfpga/misc.c
>>>>> @@ -120,7 +120,7 @@ int arch_cpu_init(void)
>>>>>       return 0;
>>>>>  }
>>>>>
>>>>> -#ifdef CONFIG_ETH_DESIGNWARE
>>>>> +#if defined CONFIG_ETH_DESIGNWARE && !defined CONFIG_SPL_BUILD
>>>
>>> Would it work for you to change this into:
>>>
>>> #if defined CONFIG_ETH_DESIGNWARE && !CONFIG_IS_ENABLED(OF_PLATDATA)
>>>
>>> We could still fix the reset code when actually using OF_PLATDATA with
>>> ethernet...
>>
>> Is it too much work to fix it properly ?
> 
> Well, just moving the functions I commented out into the eth driver would mean
> it would at some point call back into arch/arm/mach-socfpga code.
> 
> If that is acceptable, it wouldn't be too much work and I could certainly do it.
> But the question remains whether such a change would "fix it properly". In my
> opinion, it wouldn't. But it would probably still be better than the
> current state
> where some code in arch/arm parses ETH DTS nodes...

Right. I think you can just pull that info from the DT node associated
with the DWMAC driver.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Simon Goldschmidt]

Add a driver for the "snps,dw-apb-uart" used in socfpga and others.

This driver is required to get OF_PLATDATA to work for socfpga.
It uses the ns16550 driver, converting the platdata from of-platdata
go the ns16550 format.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

 drivers/serial/Kconfig         | 10 ++++++++
 drivers/serial/Makefile        |  1 +
 drivers/serial/serial_dw_apb.c | 42 ++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+)
 create mode 100644 drivers/serial/serial_dw_apb.c

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index b7ff2960ab..10addd3309 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -511,6 +511,16 @@ config BCM283X_PL011_SERIAL
 	  that supports automatic disable, so that it only gets used when
 	  the UART is actually muxed.
 
+config DESIGNWARE_SERIAL
+	bool "DesignWare UART support"
+	depends on DM_SERIAL && SPL_OF_PLATDATA
+	help
+	  Select this to enable a UART driver for "snps,dw-apb-uart" devices
+	  when using CONFIG_SPL_OF_PLATDATA (i.e. a compiled-in device tree
+	  replacement).
+	  This uses the ns16550 driver, converting the platdata from of-platdata
+	  to the ns16550 format.
+
 config BCM6345_SERIAL
 	bool "Support for BCM6345 UART"
 	depends on DM_SERIAL
diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index 06ee30697d..f1ebb90d92 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_MESON_SERIAL) += serial_meson.o
 obj-$(CONFIG_INTEL_MID_SERIAL) += serial_intel_mid.o
 ifdef CONFIG_SPL_BUILD
 obj-$(CONFIG_ROCKCHIP_SERIAL) += serial_rockchip.o
+obj-$(CONFIG_DESIGNWARE_SERIAL) += serial_dw_apb.o
 endif
 obj-$(CONFIG_XILINX_UARTLITE) += serial_xuartlite.o
 obj-$(CONFIG_SANDBOX_SERIAL) += sandbox.o
diff --git a/drivers/serial/serial_dw_apb.c b/drivers/serial/serial_dw_apb.c
new file mode 100644
index 0000000000..26580e5ba5
--- /dev/null
+++ b/drivers/serial/serial_dw_apb.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2018 Simon Goldschmidt
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <dt-structs.h>
+#include <ns16550.h>
+#include <serial.h>
+
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+struct dw_apb_uart_platdata {
+	struct dtd_snps_dw_apb_uart dtplat;
+	struct ns16550_platdata plat;
+};
+
+static int dw_apb_serial_probe(struct udevice *dev)
+{
+	struct dw_apb_uart_platdata *plat = dev_get_platdata(dev);
+
+	/* Set up correct platform data for the standard driver */
+	plat->plat.base = plat->dtplat.reg[0];
+	plat->plat.reg_shift = plat->dtplat.reg_shift;
+	plat->plat.reg_width = plat->dtplat.reg_io_width;
+	plat->plat.clock = plat->dtplat.clock_frequency;
+	plat->plat.fcr = UART_FCR_DEFVAL;
+	dev->platdata = &plat->plat;
+
+	return ns16550_serial_probe(dev);
+}
+
+U_BOOT_DRIVER(snps_dw_apb_uart) = {
+	.name	= "snps_dw_apb_uart",
+	.id	= UCLASS_SERIAL,
+	.priv_auto_alloc_size = sizeof(struct NS16550),
+	.platdata_auto_alloc_size = sizeof(struct dw_apb_uart_platdata),
+	.probe	= dw_apb_serial_probe,
+	.ops	= &ns16550_serial_ops,
+	.flags	= DM_FLAG_PRE_RELOC,
+};
+#endif
-- 
2.17.1

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Lukasz Majewski]

Hi Simon,

> Add a driver for the "snps,dw-apb-uart" used in socfpga and others.
> 
> This driver is required to get OF_PLATDATA to work for socfpga.
> It uses the ns16550 driver, converting the platdata from of-platdata
> go the ns16550 format.
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
> 
>  drivers/serial/Kconfig         | 10 ++++++++
>  drivers/serial/Makefile        |  1 +
>  drivers/serial/serial_dw_apb.c | 42
> ++++++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+)
>  create mode 100644 drivers/serial/serial_dw_apb.c
> 
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index b7ff2960ab..10addd3309 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -511,6 +511,16 @@ config BCM283X_PL011_SERIAL
>  	  that supports automatic disable, so that it only gets used
> when the UART is actually muxed.
>  
> +config DESIGNWARE_SERIAL
> +	bool "DesignWare UART support"
> +	depends on DM_SERIAL && SPL_OF_PLATDATA
> +	help
> +	  Select this to enable a UART driver for "snps,dw-apb-uart"
> devices
> +	  when using CONFIG_SPL_OF_PLATDATA (i.e. a compiled-in
> device tree
> +	  replacement).
> +	  This uses the ns16550 driver, converting the platdata from
> of-platdata
> +	  to the ns16550 format.
> +
>  config BCM6345_SERIAL
>  	bool "Support for BCM6345 UART"
>  	depends on DM_SERIAL
> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> index 06ee30697d..f1ebb90d92 100644
> --- a/drivers/serial/Makefile
> +++ b/drivers/serial/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_MESON_SERIAL) += serial_meson.o
>  obj-$(CONFIG_INTEL_MID_SERIAL) += serial_intel_mid.o
>  ifdef CONFIG_SPL_BUILD
>  obj-$(CONFIG_ROCKCHIP_SERIAL) += serial_rockchip.o
> +obj-$(CONFIG_DESIGNWARE_SERIAL) += serial_dw_apb.o
>  endif
>  obj-$(CONFIG_XILINX_UARTLITE) += serial_xuartlite.o
>  obj-$(CONFIG_SANDBOX_SERIAL) += sandbox.o
> diff --git a/drivers/serial/serial_dw_apb.c
> b/drivers/serial/serial_dw_apb.c new file mode 100644
> index 0000000000..26580e5ba5
> --- /dev/null
> +++ b/drivers/serial/serial_dw_apb.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2018 Simon Goldschmidt
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <dt-structs.h>
> +#include <ns16550.h>
> +#include <serial.h>
> +
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> +struct dw_apb_uart_platdata {
> +	struct dtd_snps_dw_apb_uart dtplat;
> +	struct ns16550_platdata plat;
> +};
> +
> +static int dw_apb_serial_probe(struct udevice *dev)
> +{
> +	struct dw_apb_uart_platdata *plat = dev_get_platdata(dev);
> +
> +	/* Set up correct platform data for the standard driver */
> +	plat->plat.base = plat->dtplat.reg[0];
> +	plat->plat.reg_shift = plat->dtplat.reg_shift;
> +	plat->plat.reg_width = plat->dtplat.reg_io_width;
> +	plat->plat.clock = plat->dtplat.clock_frequency;
> +	plat->plat.fcr = UART_FCR_DEFVAL;
> +	dev->platdata = &plat->plat;
> +
> +	return ns16550_serial_probe(dev);
> +}
> +
> +U_BOOT_DRIVER(snps_dw_apb_uart) = {
> +	.name	= "snps_dw_apb_uart",
> +	.id	= UCLASS_SERIAL,
> +	.priv_auto_alloc_size = sizeof(struct NS16550),
> +	.platdata_auto_alloc_size = sizeof(struct
> dw_apb_uart_platdata),
> +	.probe	= dw_apb_serial_probe,
> +	.ops	= &ns16550_serial_ops,
> +	.flags	= DM_FLAG_PRE_RELOC,

Is it possible/or is your application requiring the pinmux/clock setting
for this serial?

With OF_PLATDATA we don't need to parse (and provide) the DTS (and we
use the DM/DTS driver's code in a way similar to "native" one).

I'm wondering, if it would be possible to "mimic" the dependencies
between DM drivers without DTS parsing / providing overhead.

At least in my case I would need in SPL:

-> driver XXX (working with u-boot proper's DM/DTS)
	---> CLK (uclass-clk) to enable clocks
	---> pinctrl (to config pins)

As an example: uart or eMMC.

I'm aware that it would be possible to use the old "approach" to
configure pinmux and clk separately (as it is done now) and only
re-use the DM portion of driver with OF_PLATDATA having all the
necessary info.

However, I'm wondering if there is a better way to achieve it (or if I
misunderstood something).

> +};
> +#endif




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190107/120a02ab/attachment.sig>

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Simon Goldschmidt]

On Mon, Jan 7, 2019 at 11:12 PM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Simon,
>
> > Add a driver for the "snps,dw-apb-uart" used in socfpga and others.
> >
> > This driver is required to get OF_PLATDATA to work for socfpga.
> > It uses the ns16550 driver, converting the platdata from of-platdata
> > go the ns16550 format.
> >
> > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > ---
> >
> >  drivers/serial/Kconfig         | 10 ++++++++
> >  drivers/serial/Makefile        |  1 +
> >  drivers/serial/serial_dw_apb.c | 42
> > ++++++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+)
> >  create mode 100644 drivers/serial/serial_dw_apb.c
> >
> > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> > index b7ff2960ab..10addd3309 100644
> > --- a/drivers/serial/Kconfig
> > +++ b/drivers/serial/Kconfig
> > @@ -511,6 +511,16 @@ config BCM283X_PL011_SERIAL
> >         that supports automatic disable, so that it only gets used
> > when the UART is actually muxed.
> >
> > +config DESIGNWARE_SERIAL
> > +     bool "DesignWare UART support"
> > +     depends on DM_SERIAL && SPL_OF_PLATDATA
> > +     help
> > +       Select this to enable a UART driver for "snps,dw-apb-uart"
> > devices
> > +       when using CONFIG_SPL_OF_PLATDATA (i.e. a compiled-in
> > device tree
> > +       replacement).
> > +       This uses the ns16550 driver, converting the platdata from
> > of-platdata
> > +       to the ns16550 format.
> > +
> >  config BCM6345_SERIAL
> >       bool "Support for BCM6345 UART"
> >       depends on DM_SERIAL
> > diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> > index 06ee30697d..f1ebb90d92 100644
> > --- a/drivers/serial/Makefile
> > +++ b/drivers/serial/Makefile
> > @@ -46,6 +46,7 @@ obj-$(CONFIG_MESON_SERIAL) += serial_meson.o
> >  obj-$(CONFIG_INTEL_MID_SERIAL) += serial_intel_mid.o
> >  ifdef CONFIG_SPL_BUILD
> >  obj-$(CONFIG_ROCKCHIP_SERIAL) += serial_rockchip.o
> > +obj-$(CONFIG_DESIGNWARE_SERIAL) += serial_dw_apb.o
> >  endif
> >  obj-$(CONFIG_XILINX_UARTLITE) += serial_xuartlite.o
> >  obj-$(CONFIG_SANDBOX_SERIAL) += sandbox.o
> > diff --git a/drivers/serial/serial_dw_apb.c
> > b/drivers/serial/serial_dw_apb.c new file mode 100644
> > index 0000000000..26580e5ba5
> > --- /dev/null
> > +++ b/drivers/serial/serial_dw_apb.c
> > @@ -0,0 +1,42 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2018 Simon Goldschmidt
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <dt-structs.h>
> > +#include <ns16550.h>
> > +#include <serial.h>
> > +
> > +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> > +struct dw_apb_uart_platdata {
> > +     struct dtd_snps_dw_apb_uart dtplat;
> > +     struct ns16550_platdata plat;
> > +};
> > +
> > +static int dw_apb_serial_probe(struct udevice *dev)
> > +{
> > +     struct dw_apb_uart_platdata *plat = dev_get_platdata(dev);
> > +
> > +     /* Set up correct platform data for the standard driver */
> > +     plat->plat.base = plat->dtplat.reg[0];
> > +     plat->plat.reg_shift = plat->dtplat.reg_shift;
> > +     plat->plat.reg_width = plat->dtplat.reg_io_width;
> > +     plat->plat.clock = plat->dtplat.clock_frequency;
> > +     plat->plat.fcr = UART_FCR_DEFVAL;
> > +     dev->platdata = &plat->plat;
> > +
> > +     return ns16550_serial_probe(dev);
> > +}
> > +
> > +U_BOOT_DRIVER(snps_dw_apb_uart) = {
> > +     .name   = "snps_dw_apb_uart",
> > +     .id     = UCLASS_SERIAL,
> > +     .priv_auto_alloc_size = sizeof(struct NS16550),
> > +     .platdata_auto_alloc_size = sizeof(struct
> > dw_apb_uart_platdata),
> > +     .probe  = dw_apb_serial_probe,
> > +     .ops    = &ns16550_serial_ops,
> > +     .flags  = DM_FLAG_PRE_RELOC,
>
> Is it possible/or is your application requiring the pinmux/clock setting
> for this serial?

No, the socfpga gen5 platform is somewhat of a real bad example regarding
pinmux and clocks. These are implemented completely separate, based on
files generated by the FPGA toolchain...

And as you can see above, the clock must be encoded into the DTS (and
platdata) instead of calculating it via clock references.

This might be valid for socfpga only, and this driver migth be used on other
boards as well. But I think it wouldn't hurt to start it like this and continue
improving it once it is used on other platforms - it has to be actively enabled
and is only used for SPL with OF_PLATDATA.

> With OF_PLATDATA we don't need to parse (and provide) the DTS (and we
> use the DM/DTS driver's code in a way similar to "native" one).
>
> I'm wondering, if it would be possible to "mimic" the dependencies
> between DM drivers without DTS parsing / providing overhead.

I'm actually working on something like that as I want to use QSPI with of-
platdata and there I need the parent/child connection between the SPI
master and the spi-flash...

> At least in my case I would need in SPL:
>
> -> driver XXX (working with u-boot proper's DM/DTS)
>         ---> CLK (uclass-clk) to enable clocks

Clock binding might already work with dtoc, but I haven't tried.

>         ---> pinctrl (to config pins)

As I've written above, sadly socfpga does not use pinctrl.

>
> As an example: uart or eMMC.
>
> I'm aware that it would be possible to use the old "approach" to
> configure pinmux and clk separately (as it is done now) and only
> re-use the DM portion of driver with OF_PLATDATA having all the
> necessary info.
>
> However, I'm wondering if there is a better way to achieve it (or if I
> misunderstood something).

There might be a better way, but porting socfpga to pinctrl seems
nearly unpossible without further documentation from Intel. Using CLK
might work but needs to be done...

In fact this UART is inspired by Simon's Rockchip UART for of-platdata,
which behaves about the same but uses different compatible strings.

Regards,
Simon

>
> > +};
> > +#endif
>
>
>
>
> Best regards,
>
> Lukasz Majewski

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Lukasz Majewski]

Hi Simon,

> On Mon, Jan 7, 2019 at 11:12 PM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Simon,
> >  
> > > Add a driver for the "snps,dw-apb-uart" used in socfpga and
> > > others.
> > >
> > > This driver is required to get OF_PLATDATA to work for socfpga.
> > > It uses the ns16550 driver, converting the platdata from
> > > of-platdata go the ns16550 format.
> > >
> > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > > ---
> > >
> > >  drivers/serial/Kconfig         | 10 ++++++++
> > >  drivers/serial/Makefile        |  1 +
> > >  drivers/serial/serial_dw_apb.c | 42
> > > ++++++++++++++++++++++++++++++++++ 3 files changed, 53
> > > insertions(+) create mode 100644 drivers/serial/serial_dw_apb.c
> > >
> > > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> > > index b7ff2960ab..10addd3309 100644
> > > --- a/drivers/serial/Kconfig
> > > +++ b/drivers/serial/Kconfig
> > > @@ -511,6 +511,16 @@ config BCM283X_PL011_SERIAL
> > >         that supports automatic disable, so that it only gets used
> > > when the UART is actually muxed.
> > >
> > > +config DESIGNWARE_SERIAL
> > > +     bool "DesignWare UART support"
> > > +     depends on DM_SERIAL && SPL_OF_PLATDATA
> > > +     help
> > > +       Select this to enable a UART driver for "snps,dw-apb-uart"
> > > devices
> > > +       when using CONFIG_SPL_OF_PLATDATA (i.e. a compiled-in
> > > device tree
> > > +       replacement).
> > > +       This uses the ns16550 driver, converting the platdata from
> > > of-platdata
> > > +       to the ns16550 format.
> > > +
> > >  config BCM6345_SERIAL
> > >       bool "Support for BCM6345 UART"
> > >       depends on DM_SERIAL
> > > diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> > > index 06ee30697d..f1ebb90d92 100644
> > > --- a/drivers/serial/Makefile
> > > +++ b/drivers/serial/Makefile
> > > @@ -46,6 +46,7 @@ obj-$(CONFIG_MESON_SERIAL) += serial_meson.o
> > >  obj-$(CONFIG_INTEL_MID_SERIAL) += serial_intel_mid.o
> > >  ifdef CONFIG_SPL_BUILD
> > >  obj-$(CONFIG_ROCKCHIP_SERIAL) += serial_rockchip.o
> > > +obj-$(CONFIG_DESIGNWARE_SERIAL) += serial_dw_apb.o
> > >  endif
> > >  obj-$(CONFIG_XILINX_UARTLITE) += serial_xuartlite.o
> > >  obj-$(CONFIG_SANDBOX_SERIAL) += sandbox.o
> > > diff --git a/drivers/serial/serial_dw_apb.c
> > > b/drivers/serial/serial_dw_apb.c new file mode 100644
> > > index 0000000000..26580e5ba5
> > > --- /dev/null
> > > +++ b/drivers/serial/serial_dw_apb.c
> > > @@ -0,0 +1,42 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (c) 2018 Simon Goldschmidt
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <dm.h>
> > > +#include <dt-structs.h>
> > > +#include <ns16550.h>
> > > +#include <serial.h>
> > > +
> > > +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> > > +struct dw_apb_uart_platdata {
> > > +     struct dtd_snps_dw_apb_uart dtplat;
> > > +     struct ns16550_platdata plat;
> > > +};
> > > +
> > > +static int dw_apb_serial_probe(struct udevice *dev)
> > > +{
> > > +     struct dw_apb_uart_platdata *plat = dev_get_platdata(dev);
> > > +
> > > +     /* Set up correct platform data for the standard driver */
> > > +     plat->plat.base = plat->dtplat.reg[0];
> > > +     plat->plat.reg_shift = plat->dtplat.reg_shift;
> > > +     plat->plat.reg_width = plat->dtplat.reg_io_width;
> > > +     plat->plat.clock = plat->dtplat.clock_frequency;
> > > +     plat->plat.fcr = UART_FCR_DEFVAL;
> > > +     dev->platdata = &plat->plat;
> > > +
> > > +     return ns16550_serial_probe(dev);
> > > +}
> > > +
> > > +U_BOOT_DRIVER(snps_dw_apb_uart) = {
> > > +     .name   = "snps_dw_apb_uart",
> > > +     .id     = UCLASS_SERIAL,
> > > +     .priv_auto_alloc_size = sizeof(struct NS16550),
> > > +     .platdata_auto_alloc_size = sizeof(struct
> > > dw_apb_uart_platdata),
> > > +     .probe  = dw_apb_serial_probe,
> > > +     .ops    = &ns16550_serial_ops,
> > > +     .flags  = DM_FLAG_PRE_RELOC,  
> >
> > Is it possible/or is your application requiring the pinmux/clock
> > setting for this serial?  
> 
> No, the socfpga gen5 platform is somewhat of a real bad example
> regarding pinmux and clocks. These are implemented completely
> separate, based on files generated by the FPGA toolchain...
> 
> And as you can see above, the clock must be encoded into the DTS (and
> platdata) instead of calculating it via clock references.

Just to be clear here - the FPGA SDK (Quartus ?) generates clock info
in different way than "normal" SoCs and it cannot use "clocks" and
"clocks-names" properties to define needed clocks?

> 
> This might be valid for socfpga only, and this driver migth be used
> on other boards as well.

I think yes. IMHO the goal here is to reuse DM versions of drivers in
the SPL (as non DM ones will be removed finally).

> But I think it wouldn't hurt to start it
> like this and continue improving it once it is used on other
> platforms - it has to be actively enabled and is only used for SPL
> with OF_PLATDATA.
> 
> > With OF_PLATDATA we don't need to parse (and provide) the DTS (and
> > we use the DM/DTS driver's code in a way similar to "native" one).
> >
> > I'm wondering, if it would be possible to "mimic" the dependencies
> > between DM drivers without DTS parsing / providing overhead.  
> 
> I'm actually working on something like that as I want to use QSPI
> with of- platdata and there I need the parent/child connection
> between the SPI master and the spi-flash...

This is my point - the OF_PLATDATA allows using the DM ready driver as
the standalone (no DTS parsing nor appending) entity in SPL.

The problem (at least for iMX6) is the dependency on pinmux (which
properties by default are stripped from SPL's version of DTB).

I could hack it with having relevant pins defined as "hog" group,
which would be configured when pinctrl driver is probed in SPL. However,
this would prevent me from re-using kernel DTS out of the box.

> 
> > At least in my case I would need in SPL:
> >  
> > -> driver XXX (working with u-boot proper's DM/DTS)
> >         ---> CLK (uclass-clk) to enable clocks  
> 
> Clock binding might already work with dtoc, but I haven't tried.
> 
> >         ---> pinctrl (to config pins)  
> 
> As I've written above, sadly socfpga does not use pinctrl.
> 
> >
> > As an example: uart or eMMC.
> >
> > I'm aware that it would be possible to use the old "approach" to
> > configure pinmux and clk separately (as it is done now) and only
> > re-use the DM portion of driver with OF_PLATDATA having all the
> > necessary info.
> >
> > However, I'm wondering if there is a better way to achieve it (or
> > if I misunderstood something).  
> 
> There might be a better way, but porting socfpga to pinctrl seems
> nearly unpossible without further documentation from Intel. Using CLK
> might work but needs to be done...
> 
> In fact this UART is inspired by Simon's Rockchip UART for
> of-platdata, which behaves about the same but uses different
> compatible strings.

Thanks for the info. I will look into that.

> 
> Regards,
> Simon
> 
> >  
> > > +};
> > > +#endif  
> >
> >
> >
> >
> > Best regards,
> >
> > Lukasz Majewski  




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190108/54451572/attachment.sig>

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Simon Goldschmidt]

On Tue, Jan 8, 2019 at 8:30 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Simon,
>
> > On Mon, Jan 7, 2019 at 11:12 PM Lukasz Majewski <lukma@denx.de> wrote:
> > >
> > > Hi Simon,
> > >
> > > > Add a driver for the "snps,dw-apb-uart" used in socfpga and
> > > > others.
> > > >
> > > > This driver is required to get OF_PLATDATA to work for socfpga.
> > > > It uses the ns16550 driver, converting the platdata from
> > > > of-platdata go the ns16550 format.
> > > >
> > > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > > > ---
> > > >
> > > >  drivers/serial/Kconfig         | 10 ++++++++
> > > >  drivers/serial/Makefile        |  1 +
> > > >  drivers/serial/serial_dw_apb.c | 42
> > > > ++++++++++++++++++++++++++++++++++ 3 files changed, 53
> > > > insertions(+) create mode 100644 drivers/serial/serial_dw_apb.c
> > > >
> > > > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> > > > index b7ff2960ab..10addd3309 100644
> > > > --- a/drivers/serial/Kconfig
> > > > +++ b/drivers/serial/Kconfig
> > > > @@ -511,6 +511,16 @@ config BCM283X_PL011_SERIAL
> > > >         that supports automatic disable, so that it only gets used
> > > > when the UART is actually muxed.
> > > >
> > > > +config DESIGNWARE_SERIAL
> > > > +     bool "DesignWare UART support"
> > > > +     depends on DM_SERIAL && SPL_OF_PLATDATA
> > > > +     help
> > > > +       Select this to enable a UART driver for "snps,dw-apb-uart"
> > > > devices
> > > > +       when using CONFIG_SPL_OF_PLATDATA (i.e. a compiled-in
> > > > device tree
> > > > +       replacement).
> > > > +       This uses the ns16550 driver, converting the platdata from
> > > > of-platdata
> > > > +       to the ns16550 format.
> > > > +
> > > >  config BCM6345_SERIAL
> > > >       bool "Support for BCM6345 UART"
> > > >       depends on DM_SERIAL
> > > > diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> > > > index 06ee30697d..f1ebb90d92 100644
> > > > --- a/drivers/serial/Makefile
> > > > +++ b/drivers/serial/Makefile
> > > > @@ -46,6 +46,7 @@ obj-$(CONFIG_MESON_SERIAL) += serial_meson.o
> > > >  obj-$(CONFIG_INTEL_MID_SERIAL) += serial_intel_mid.o
> > > >  ifdef CONFIG_SPL_BUILD
> > > >  obj-$(CONFIG_ROCKCHIP_SERIAL) += serial_rockchip.o
> > > > +obj-$(CONFIG_DESIGNWARE_SERIAL) += serial_dw_apb.o
> > > >  endif
> > > >  obj-$(CONFIG_XILINX_UARTLITE) += serial_xuartlite.o
> > > >  obj-$(CONFIG_SANDBOX_SERIAL) += sandbox.o
> > > > diff --git a/drivers/serial/serial_dw_apb.c
> > > > b/drivers/serial/serial_dw_apb.c new file mode 100644
> > > > index 0000000000..26580e5ba5
> > > > --- /dev/null
> > > > +++ b/drivers/serial/serial_dw_apb.c
> > > > @@ -0,0 +1,42 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Copyright (c) 2018 Simon Goldschmidt
> > > > + */
> > > > +
> > > > +#include <common.h>
> > > > +#include <dm.h>
> > > > +#include <dt-structs.h>
> > > > +#include <ns16550.h>
> > > > +#include <serial.h>
> > > > +
> > > > +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> > > > +struct dw_apb_uart_platdata {
> > > > +     struct dtd_snps_dw_apb_uart dtplat;
> > > > +     struct ns16550_platdata plat;
> > > > +};
> > > > +
> > > > +static int dw_apb_serial_probe(struct udevice *dev)
> > > > +{
> > > > +     struct dw_apb_uart_platdata *plat = dev_get_platdata(dev);
> > > > +
> > > > +     /* Set up correct platform data for the standard driver */
> > > > +     plat->plat.base = plat->dtplat.reg[0];
> > > > +     plat->plat.reg_shift = plat->dtplat.reg_shift;
> > > > +     plat->plat.reg_width = plat->dtplat.reg_io_width;
> > > > +     plat->plat.clock = plat->dtplat.clock_frequency;
> > > > +     plat->plat.fcr = UART_FCR_DEFVAL;
> > > > +     dev->platdata = &plat->plat;
> > > > +
> > > > +     return ns16550_serial_probe(dev);
> > > > +}
> > > > +
> > > > +U_BOOT_DRIVER(snps_dw_apb_uart) = {
> > > > +     .name   = "snps_dw_apb_uart",
> > > > +     .id     = UCLASS_SERIAL,
> > > > +     .priv_auto_alloc_size = sizeof(struct NS16550),
> > > > +     .platdata_auto_alloc_size = sizeof(struct
> > > > dw_apb_uart_platdata),
> > > > +     .probe  = dw_apb_serial_probe,
> > > > +     .ops    = &ns16550_serial_ops,
> > > > +     .flags  = DM_FLAG_PRE_RELOC,
> > >
> > > Is it possible/or is your application requiring the pinmux/clock
> > > setting for this serial?
> >
> > No, the socfpga gen5 platform is somewhat of a real bad example
> > regarding pinmux and clocks. These are implemented completely
> > separate, based on files generated by the FPGA toolchain...
> >
> > And as you can see above, the clock must be encoded into the DTS (and
> > platdata) instead of calculating it via clock references.
>
> Just to be clear here - the FPGA SDK (Quartus ?) generates clock info
> in different way than "normal" SoCs and it cannot use "clocks" and
> "clocks-names" properties to define needed clocks?

I'm afraid I don't know what the "normal" SoC way is in Linux. Coming from
smaller embedded systems like STM32, what Quartus generates here is
not uncommon :-)

The DTS for socfpga has clock references but at least for the gen5 devices,
these are not used.

Anyway, how do "normal" SoCs select the target speed of IP cores? The
DTS only describes what can be done. I.e. how do I tell U-Boot that I want
the CPU at say, 400 MHz, not 800 MHz? (This question of course applies to
peripherals as well).

> >
> > This might be valid for socfpga only, and this driver migth be used
> > on other boards as well.
>
> I think yes. IMHO the goal here is to reuse DM versions of drivers in
> the SPL (as non DM ones will be removed finally).

Well, this *is* a DM serial driver. It is just lacking clock and
pinctrl references
for some platforms...

>
> > But I think it wouldn't hurt to start it
> > like this and continue improving it once it is used on other
> > platforms - it has to be actively enabled and is only used for SPL
> > with OF_PLATDATA.
> >
> > > With OF_PLATDATA we don't need to parse (and provide) the DTS (and
> > > we use the DM/DTS driver's code in a way similar to "native" one).
> > >
> > > I'm wondering, if it would be possible to "mimic" the dependencies
> > > between DM drivers without DTS parsing / providing overhead.
> >
> > I'm actually working on something like that as I want to use QSPI
> > with of- platdata and there I need the parent/child connection
> > between the SPI master and the spi-flash...
>
> This is my point - the OF_PLATDATA allows using the DM ready driver as
> the standalone (no DTS parsing nor appending) entity in SPL.
>
> The problem (at least for iMX6) is the dependency on pinmux (which
> properties by default are stripped from SPL's version of DTB).

You need to stop it stripping that, of course ;-)

I'd like that for socfpga as well. Unfortunately, there is no documentation
of the pinmux configuration for these Intel FPGAs and Quartus only generates
4 big u32 arrays :-(

>
> I could hack it with having relevant pins defined as "hog" group,
> which would be configured when pinctrl driver is probed in SPL. However,
> this would prevent me from re-using kernel DTS out of the box.
>
> >
> > > At least in my case I would need in SPL:
> > >
> > > -> driver XXX (working with u-boot proper's DM/DTS)
> > >         ---> CLK (uclass-clk) to enable clocks
> >
> > Clock binding might already work with dtoc, but I haven't tried.
> >
> > >         ---> pinctrl (to config pins)
> >
> > As I've written above, sadly socfpga does not use pinctrl.
> >
> > >
> > > As an example: uart or eMMC.
> > >
> > > I'm aware that it would be possible to use the old "approach" to
> > > configure pinmux and clk separately (as it is done now) and only
> > > re-use the DM portion of driver with OF_PLATDATA having all the
> > > necessary info.
> > >
> > > However, I'm wondering if there is a better way to achieve it (or
> > > if I misunderstood something).
> >
> > There might be a better way, but porting socfpga to pinctrl seems
> > nearly unpossible without further documentation from Intel. Using CLK
> > might work but needs to be done...
> >
> > In fact this UART is inspired by Simon's Rockchip UART for
> > of-platdata, which behaves about the same but uses different
> > compatible strings.
>
> Thanks for the info. I will look into that.

It's actually pretty much the same like this, only the names change. That
might be worth combining, but the device/driver binding code would need
to allow checking for multiple compatible strings.

Regards,
Simon

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Lukasz Majewski]

Hi Simon,

> On Tue, Jan 8, 2019 at 8:30 AM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Simon,
> >  
> > > On Mon, Jan 7, 2019 at 11:12 PM Lukasz Majewski <lukma@denx.de>
> > > wrote:  
> > > >
> > > > Hi Simon,
> > > >  
> > > > > Add a driver for the "snps,dw-apb-uart" used in socfpga and
> > > > > others.
> > > > >
> > > > > This driver is required to get OF_PLATDATA to work for
> > > > > socfpga. It uses the ns16550 driver, converting the platdata
> > > > > from of-platdata go the ns16550 format.
> > > > >
> > > > > Signed-off-by: Simon Goldschmidt
> > > > > <simon.k.r.goldschmidt@gmail.com> ---
> > > > >
> > > > >  drivers/serial/Kconfig         | 10 ++++++++
> > > > >  drivers/serial/Makefile        |  1 +
> > > > >  drivers/serial/serial_dw_apb.c | 42
> > > > > ++++++++++++++++++++++++++++++++++ 3 files changed, 53
> > > > > insertions(+) create mode 100644
> > > > > drivers/serial/serial_dw_apb.c
> > > > >
> > > > > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> > > > > index b7ff2960ab..10addd3309 100644
> > > > > --- a/drivers/serial/Kconfig
> > > > > +++ b/drivers/serial/Kconfig
> > > > > @@ -511,6 +511,16 @@ config BCM283X_PL011_SERIAL
> > > > >         that supports automatic disable, so that it only gets
> > > > > used when the UART is actually muxed.
> > > > >
> > > > > +config DESIGNWARE_SERIAL
> > > > > +     bool "DesignWare UART support"
> > > > > +     depends on DM_SERIAL && SPL_OF_PLATDATA
> > > > > +     help
> > > > > +       Select this to enable a UART driver for
> > > > > "snps,dw-apb-uart" devices
> > > > > +       when using CONFIG_SPL_OF_PLATDATA (i.e. a compiled-in
> > > > > device tree
> > > > > +       replacement).
> > > > > +       This uses the ns16550 driver, converting the platdata
> > > > > from of-platdata
> > > > > +       to the ns16550 format.
> > > > > +
> > > > >  config BCM6345_SERIAL
> > > > >       bool "Support for BCM6345 UART"
> > > > >       depends on DM_SERIAL
> > > > > diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> > > > > index 06ee30697d..f1ebb90d92 100644
> > > > > --- a/drivers/serial/Makefile
> > > > > +++ b/drivers/serial/Makefile
> > > > > @@ -46,6 +46,7 @@ obj-$(CONFIG_MESON_SERIAL) += serial_meson.o
> > > > >  obj-$(CONFIG_INTEL_MID_SERIAL) += serial_intel_mid.o
> > > > >  ifdef CONFIG_SPL_BUILD
> > > > >  obj-$(CONFIG_ROCKCHIP_SERIAL) += serial_rockchip.o
> > > > > +obj-$(CONFIG_DESIGNWARE_SERIAL) += serial_dw_apb.o
> > > > >  endif
> > > > >  obj-$(CONFIG_XILINX_UARTLITE) += serial_xuartlite.o
> > > > >  obj-$(CONFIG_SANDBOX_SERIAL) += sandbox.o
> > > > > diff --git a/drivers/serial/serial_dw_apb.c
> > > > > b/drivers/serial/serial_dw_apb.c new file mode 100644
> > > > > index 0000000000..26580e5ba5
> > > > > --- /dev/null
> > > > > +++ b/drivers/serial/serial_dw_apb.c
> > > > > @@ -0,0 +1,42 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +/*
> > > > > + * Copyright (c) 2018 Simon Goldschmidt
> > > > > + */
> > > > > +
> > > > > +#include <common.h>
> > > > > +#include <dm.h>
> > > > > +#include <dt-structs.h>
> > > > > +#include <ns16550.h>
> > > > > +#include <serial.h>
> > > > > +
> > > > > +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> > > > > +struct dw_apb_uart_platdata {
> > > > > +     struct dtd_snps_dw_apb_uart dtplat;
> > > > > +     struct ns16550_platdata plat;
> > > > > +};
> > > > > +
> > > > > +static int dw_apb_serial_probe(struct udevice *dev)
> > > > > +{
> > > > > +     struct dw_apb_uart_platdata *plat =
> > > > > dev_get_platdata(dev); +
> > > > > +     /* Set up correct platform data for the standard driver
> > > > > */
> > > > > +     plat->plat.base = plat->dtplat.reg[0];
> > > > > +     plat->plat.reg_shift = plat->dtplat.reg_shift;
> > > > > +     plat->plat.reg_width = plat->dtplat.reg_io_width;
> > > > > +     plat->plat.clock = plat->dtplat.clock_frequency;
> > > > > +     plat->plat.fcr = UART_FCR_DEFVAL;
> > > > > +     dev->platdata = &plat->plat;
> > > > > +
> > > > > +     return ns16550_serial_probe(dev);
> > > > > +}
> > > > > +
> > > > > +U_BOOT_DRIVER(snps_dw_apb_uart) = {
> > > > > +     .name   = "snps_dw_apb_uart",
> > > > > +     .id     = UCLASS_SERIAL,
> > > > > +     .priv_auto_alloc_size = sizeof(struct NS16550),
> > > > > +     .platdata_auto_alloc_size = sizeof(struct
> > > > > dw_apb_uart_platdata),
> > > > > +     .probe  = dw_apb_serial_probe,
> > > > > +     .ops    = &ns16550_serial_ops,
> > > > > +     .flags  = DM_FLAG_PRE_RELOC,  
> > > >
> > > > Is it possible/or is your application requiring the pinmux/clock
> > > > setting for this serial?  
> > >
> > > No, the socfpga gen5 platform is somewhat of a real bad example
> > > regarding pinmux and clocks. These are implemented completely
> > > separate, based on files generated by the FPGA toolchain...
> > >
> > > And as you can see above, the clock must be encoded into the DTS
> > > (and platdata) instead of calculating it via clock references.  
> >
> > Just to be clear here - the FPGA SDK (Quartus ?) generates clock
> > info in different way than "normal" SoCs and it cannot use "clocks"
> > and "clocks-names" properties to define needed clocks?  
> 
> I'm afraid I don't know what the "normal" SoC way is in Linux. Coming
> from smaller embedded systems like STM32, what Quartus generates here
> is not uncommon :-)

Only Intel (ALtera) knows what Quartus generates (to some extend :-) ).

> 
> The DTS for socfpga has clock references but at least for the gen5
> devices, these are not used.

Ok. I see.

> 
> Anyway, how do "normal" SoCs select the target speed of IP cores? The
> DTS only describes what can be done. I.e. how do I tell U-Boot that I
> want the CPU at say, 400 MHz, not 800 MHz? (This question of course
> applies to peripherals as well).

In case of IMX6 there is a set of "clock" functions in
mach-imx/mx6/clock.c

Those functions are called by board code/drivers.

In Linux the CLK hierarchy is modelled in a more verbose way and
describes all the needed MUXes, PLLs, Gates, etc.
However, IMHO we don't need such verbose description in U-boot as at
most we need to increase the ROM default clock and enable it (to
improve boot time).

> 
> > >
> > > This might be valid for socfpga only, and this driver migth be
> > > used on other boards as well.  
> >
> > I think yes. IMHO the goal here is to reuse DM versions of drivers
> > in the SPL (as non DM ones will be removed finally).  
> 
> Well, this *is* a DM serial driver.

Yes. That was my point to reuse this DM code in SPL.

> It is just lacking clock and
> pinctrl references
> for some platforms...

And this is problematic to have full DM reuse in SPL with reasonable
size.

In one of IMX6Q boards the SPL grown from 35 KiB to 51 KiB when the
DM/DTS conversion has been done (without OF_PLATDATA, tiny
printf/malloc). 

> 
> >  
> > > But I think it wouldn't hurt to start it
> > > like this and continue improving it once it is used on other
> > > platforms - it has to be actively enabled and is only used for SPL
> > > with OF_PLATDATA.
> > >  
> > > > With OF_PLATDATA we don't need to parse (and provide) the DTS
> > > > (and we use the DM/DTS driver's code in a way similar to
> > > > "native" one).
> > > >
> > > > I'm wondering, if it would be possible to "mimic" the
> > > > dependencies between DM drivers without DTS parsing / providing
> > > > overhead.  
> > >
> > > I'm actually working on something like that as I want to use QSPI
> > > with of- platdata and there I need the parent/child connection
> > > between the SPI master and the spi-flash...  
> >
> > This is my point - the OF_PLATDATA allows using the DM ready driver
> > as the standalone (no DTS parsing nor appending) entity in SPL.
> >
> > The problem (at least for iMX6) is the dependency on pinmux (which
> > properties by default are stripped from SPL's version of DTB).  
> 
> You need to stop it stripping that, of course ;-)

Yes :-).

> 
> I'd like that for socfpga as well. Unfortunately, there is no
> documentation of the pinmux configuration for these Intel FPGAs and
> Quartus only generates 4 big u32 arrays :-(

Binary blob is the best way to configure things :-) -> Then just use
single "for" and everything just works :-).

> 
> >
> > I could hack it with having relevant pins defined as "hog" group,
> > which would be configured when pinctrl driver is probed in SPL.
> > However, this would prevent me from re-using kernel DTS out of the
> > box. 
> > >  
> > > > At least in my case I would need in SPL:
> > > >  
> > > > -> driver XXX (working with u-boot proper's DM/DTS)
> > > >         ---> CLK (uclass-clk) to enable clocks  
> > >
> > > Clock binding might already work with dtoc, but I haven't tried.
> > >  
> > > >         ---> pinctrl (to config pins)  
> > >
> > > As I've written above, sadly socfpga does not use pinctrl.
> > >  
> > > >
> > > > As an example: uart or eMMC.
> > > >
> > > > I'm aware that it would be possible to use the old "approach" to
> > > > configure pinmux and clk separately (as it is done now) and only
> > > > re-use the DM portion of driver with OF_PLATDATA having all the
> > > > necessary info.
> > > >
> > > > However, I'm wondering if there is a better way to achieve it
> > > > (or if I misunderstood something).  
> > >
> > > There might be a better way, but porting socfpga to pinctrl seems
> > > nearly unpossible without further documentation from Intel. Using
> > > CLK might work but needs to be done...
> > >
> > > In fact this UART is inspired by Simon's Rockchip UART for
> > > of-platdata, which behaves about the same but uses different
> > > compatible strings.  
> >
> > Thanks for the info. I will look into that.  
> 
> It's actually pretty much the same like this, only the names change.
> That might be worth combining, but the device/driver binding code
> would need to allow checking for multiple compatible strings.

Ok. I see.

> 
> Regards,
> Simon




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190108/4a29706f/attachment.sig>

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Alexey Brodkin]

Hi Simon,

> -----Original Message-----
> From: Simon Goldschmidt [mailto:simon.k.r.goldschmidt at gmail.com]
> Sent: Tuesday, January 8, 2019 12:14 AM
> To: Marek Vasut <marex@denx.de>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>; Patrice Chotard <patrice.chotard@st.com>;
> Simon Glass <sjg@chromium.org>; Anup Patel <anup@brainfault.org>; Lokesh Vutla <lokeshvutla@ti.com>;
> Alexey Brodkin <alexey.brodkin@synopsys.com>; Patrick Delaunay <patrick.delaunay@st.com>; Marek Vasut
> <marek.vasut@gmail.com>; u-boot at lists.denx.de; Álvaro Fernández Rojas <noltari@gmail.com>; Ryder Lee
> <ryder.lee@mediatek.com>; Vikas Manocha <vikas.manocha@st.com>; Alexander Graf <agraf@suse.de>; Weijie
> Gao <weijie.gao@mediatek.com>
> Subject: [PATCH v1 3/4] serial: add an of-platdata driver for "snps,dw-apb-uart"
> 
> Add a driver for the "snps,dw-apb-uart" used in socfpga and others.
> 
> This driver is required to get OF_PLATDATA to work for socfpga.
> It uses the ns16550 driver, converting the platdata from of-platdata
> go the ns16550 format.
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
> 
>  drivers/serial/Kconfig         | 10 ++++++++
>  drivers/serial/Makefile        |  1 +
>  drivers/serial/serial_dw_apb.c | 42 ++++++++++++++++++++++++++++++++++
>  3 files changed, 53 insertions(+)
>  create mode 100644 drivers/serial/serial_dw_apb.c
> 
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index b7ff2960ab..10addd3309 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -511,6 +511,16 @@ config BCM283X_PL011_SERIAL
>  	  that supports automatic disable, so that it only gets used when
>  	  the UART is actually muxed.
> 
> +config DESIGNWARE_SERIAL
> +	bool "DesignWare UART support"
> +	depends on DM_SERIAL && SPL_OF_PLATDATA

Might be a bit naïve question but why depend on SPL_OF_PLATDATA only?
What about CONFIG_OF_EMBED?

I'd happily switch my ARC boards on this driver and get rid of all
CONFIG_SYS_NS16550_xxx nonsense in include/configs/myboardname.h

-Alexey

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Simon Goldschmidt]

Hi Alexey,

On Wed, Jan 9, 2019 at 9:36 AM Alexey Brodkin
<alexey.brodkin@synopsys.com> wrote:
>
> Hi Simon,
>
> > -----Original Message-----
> > From: Simon Goldschmidt [mailto:simon.k.r.goldschmidt at gmail.com]
> > Sent: Tuesday, January 8, 2019 12:14 AM
> > To: Marek Vasut <marex@denx.de>
> > Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>; Patrice Chotard <patrice.chotard@st.com>;
> > Simon Glass <sjg@chromium.org>; Anup Patel <anup@brainfault.org>; Lokesh Vutla <lokeshvutla@ti.com>;
> > Alexey Brodkin <alexey.brodkin@synopsys.com>; Patrick Delaunay <patrick.delaunay@st.com>; Marek Vasut
> > <marek.vasut@gmail.com>; u-boot at lists.denx.de; Álvaro Fernández Rojas <noltari@gmail.com>; Ryder Lee
> > <ryder.lee@mediatek.com>; Vikas Manocha <vikas.manocha@st.com>; Alexander Graf <agraf@suse.de>; Weijie
> > Gao <weijie.gao@mediatek.com>
> > Subject: [PATCH v1 3/4] serial: add an of-platdata driver for "snps,dw-apb-uart"
> >
> > Add a driver for the "snps,dw-apb-uart" used in socfpga and others.
> >
> > This driver is required to get OF_PLATDATA to work for socfpga.
> > It uses the ns16550 driver, converting the platdata from of-platdata
> > go the ns16550 format.
> >
> > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > ---
> >
> >  drivers/serial/Kconfig         | 10 ++++++++
> >  drivers/serial/Makefile        |  1 +
> >  drivers/serial/serial_dw_apb.c | 42 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 53 insertions(+)
> >  create mode 100644 drivers/serial/serial_dw_apb.c
> >
> > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> > index b7ff2960ab..10addd3309 100644
> > --- a/drivers/serial/Kconfig
> > +++ b/drivers/serial/Kconfig
> > @@ -511,6 +511,16 @@ config BCM283X_PL011_SERIAL
> >         that supports automatic disable, so that it only gets used when
> >         the UART is actually muxed.
> >
> > +config DESIGNWARE_SERIAL
> > +     bool "DesignWare UART support"
> > +     depends on DM_SERIAL && SPL_OF_PLATDATA
>
> Might be a bit naïve question but why depend on SPL_OF_PLATDATA only?

Because my focus has been to get OF_PLATDATA running on my socfpga board.
But since Marek seems to be opposed, I guess I'll drop this series and
try to find
a different way to squeeze out the bytes I need.

> What about CONFIG_OF_EMBED?
>
> I'd happily switch my ARC boards on this driver and get rid of all
> CONFIG_SYS_NS16550_xxx nonsense in include/configs/myboardname.h

You're right there, this would be a good idea for socfpga as well. Maybe I'll
continue that line some day, but I don't have immediate plans for this.

Regards,
Simon

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Simon Goldschmidt]

Hi Alexey,

Am 09.01.2019 um 09:35 schrieb Alexey Brodkin:
> Hi Simon,
> 
>> -----Original Message-----
>> From: Simon Goldschmidt [mailto:simon.k.r.goldschmidt at gmail.com]
>> Sent: Tuesday, January 8, 2019 12:14 AM
>> To: Marek Vasut <marex@denx.de>
>> Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>; Patrice Chotard <patrice.chotard@st.com>;
>> Simon Glass <sjg@chromium.org>; Anup Patel <anup@brainfault.org>; Lokesh Vutla <lokeshvutla@ti.com>;
>> Alexey Brodkin <alexey.brodkin@synopsys.com>; Patrick Delaunay <patrick.delaunay@st.com>; Marek Vasut
>> <marek.vasut@gmail.com>; u-boot at lists.denx.de; Álvaro Fernández Rojas <noltari@gmail.com>; Ryder Lee
>> <ryder.lee@mediatek.com>; Vikas Manocha <vikas.manocha@st.com>; Alexander Graf <agraf@suse.de>; Weijie
>> Gao <weijie.gao@mediatek.com>
>> Subject: [PATCH v1 3/4] serial: add an of-platdata driver for "snps,dw-apb-uart"
>>
>> Add a driver for the "snps,dw-apb-uart" used in socfpga and others.
>>
>> This driver is required to get OF_PLATDATA to work for socfpga.
>> It uses the ns16550 driver, converting the platdata from of-platdata
>> go the ns16550 format.
>>
>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>> ---
>>
>>   drivers/serial/Kconfig         | 10 ++++++++
>>   drivers/serial/Makefile        |  1 +
>>   drivers/serial/serial_dw_apb.c | 42 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 53 insertions(+)
>>   create mode 100644 drivers/serial/serial_dw_apb.c
>>
>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
>> index b7ff2960ab..10addd3309 100644
>> --- a/drivers/serial/Kconfig
>> +++ b/drivers/serial/Kconfig
>> @@ -511,6 +511,16 @@ config BCM283X_PL011_SERIAL
>>   	  that supports automatic disable, so that it only gets used when
>>   	  the UART is actually muxed.
>>
>> +config DESIGNWARE_SERIAL
>> +	bool "DesignWare UART support"
>> +	depends on DM_SERIAL && SPL_OF_PLATDATA
> 
> Might be a bit naïve question but why depend on SPL_OF_PLATDATA only?
> What about CONFIG_OF_EMBED?
> 
> I'd happily switch my ARC boards on this driver and get rid of all
> CONFIG_SYS_NS16550_xxx nonsense in include/configs/myboardname.h

I checked include/configs/socfpga_common.h again and by its using 
Kconfig and DM_SERIAL, there are no CONFIG_SYS_NS16550_xxx defines left 
(other than CONFIG_SYS_NS16550_SERIAL, which I will remove).

So it seems it's not a matter of a new driver but a matter of cleaning 
up the boards to use DM_SERIAL, I guess?

Regards,
Simon

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Alexey Brodkin]

Hi Simon,

[snip]

> >> +config DESIGNWARE_SERIAL
> >> +	bool "DesignWare UART support"
> >> +	depends on DM_SERIAL && SPL_OF_PLATDATA
> >
> > Might be a bit naïve question but why depend on SPL_OF_PLATDATA only?
> > What about CONFIG_OF_EMBED?

Ok I completely forgot that standard ns16550 driver already covers DW APB UART,
see https://elixir.bootlin.com/u-boot/latest/source/drivers/serial/ns16550.c#L456

> > I'd happily switch my ARC boards on this driver and get rid of all
> > CONFIG_SYS_NS16550_xxx nonsense in include/configs/myboardname.h
> 
> I checked include/configs/socfpga_common.h again and by its using
> Kconfig and DM_SERIAL, there are no CONFIG_SYS_NS16550_xxx defines left
> (other than CONFIG_SYS_NS16550_SERIAL, which I will remove).

So it's not that easy apparently :)
At least CONFIG_SYS_NS16550_MEM32 is still used really required
for getting correct accessor used, see implementation of
serial_{in|out}_shift() in drivers/serial/ns16550.c.

If CONFIG_SYS_NS16550_MEM32 is not set then simple readb() is used so
that 8-bit data offset in 32-bit word is lost and we're dead in the water.

That said accessors in ns16550 are begging for significant rework to make
the driver completely OF-driven.

-Alexey

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Simon Goldschmidt]

On Fri, Jan 11, 2019 at 9:33 AM Alexey Brodkin
<alexey.brodkin@synopsys.com> wrote:
>
> Hi Simon,
>
> [snip]
>
> > >> +config DESIGNWARE_SERIAL
> > >> +  bool "DesignWare UART support"
> > >> +  depends on DM_SERIAL && SPL_OF_PLATDATA
> > >
> > > Might be a bit naïve question but why depend on SPL_OF_PLATDATA only?
> > > What about CONFIG_OF_EMBED?
>
> Ok I completely forgot that standard ns16550 driver already covers DW APB UART,
> see https://elixir.bootlin.com/u-boot/latest/source/drivers/serial/ns16550.c#L456
>
> > > I'd happily switch my ARC boards on this driver and get rid of all
> > > CONFIG_SYS_NS16550_xxx nonsense in include/configs/myboardname.h
> >
> > I checked include/configs/socfpga_common.h again and by its using
> > Kconfig and DM_SERIAL, there are no CONFIG_SYS_NS16550_xxx defines left
> > (other than CONFIG_SYS_NS16550_SERIAL, which I will remove).
>
> So it's not that easy apparently :)
> At least CONFIG_SYS_NS16550_MEM32 is still used really required
> for getting correct accessor used, see implementation of
> serial_{in|out}_shift() in drivers/serial/ns16550.c.
>
> If CONFIG_SYS_NS16550_MEM32 is not set then simple readb() is used so
> that 8-bit data offset in 32-bit word is lost and we're dead in the water.

Isn't that covered by 'plat->reg_shift' which is read from dts property
"reg-shift = <2>"?

Remember that CONFIG_SYS_NS16550_REG_SIZE is not used/required
for 32-bit socfpga as well!

> That said accessors in ns16550 are begging for significant rework to make
> the driver completely OF-driven.

I think much of the code in ns16550.c could be removed if non-DM support
was dropped. That might clean things up as well.

Regards,
Simon

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Alexey Brodkin]

Hi Simon,

> -----Original Message-----
> From: Simon Goldschmidt [mailto:simon.k.r.goldschmidt at gmail.com]
> Sent: Friday, January 11, 2019 11:41 AM
> To: Alexey Brodkin <alexey.brodkin@synopsys.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>; Simon Glass <sjg@chromium.org>; Anup Patel
> <anup@brainfault.org>; Lokesh Vutla <lokeshvutla@ti.com>; Patrick Delaunay <patrick.delaunay@st.com>;
> Marek Vasut <marek.vasut@gmail.com>; u-boot at lists.denx.de; Álvaro Fernández Rojas <noltari@gmail.com>;
> Ryder Lee <ryder.lee@mediatek.com>; Vikas Manocha <vikas.manocha@st.com>; Alexander Graf
> <agraf@suse.de>; Weijie Gao <weijie.gao@mediatek.com>; Marek Vasut <marex@denx.de>
> Subject: Re: [PATCH v1 3/4] serial: add an of-platdata driver for "snps,dw-apb-uart"
> 
> On Fri, Jan 11, 2019 at 9:33 AM Alexey Brodkin
> <alexey.brodkin@synopsys.com> wrote:
> >
> > Hi Simon,
> >
> > [snip]
> >
> > > >> +config DESIGNWARE_SERIAL
> > > >> +  bool "DesignWare UART support"
> > > >> +  depends on DM_SERIAL && SPL_OF_PLATDATA
> > > >
> > > > Might be a bit naïve question but why depend on SPL_OF_PLATDATA only?
> > > > What about CONFIG_OF_EMBED?
> >
> > Ok I completely forgot that standard ns16550 driver already covers DW APB UART,
> > see https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_u-
> 2Dboot_latest_source_drivers_serial_ns16550.c-
> 23L456&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=wyhoU5_3rGv5y
> 373_cpetmUHK9_ALMCC29QnKS2As5k&s=uMDuaOZ__YoyqakveKqByAtb1lfRQBYktcVgGH3oawE&e=
> >
> > > > I'd happily switch my ARC boards on this driver and get rid of all
> > > > CONFIG_SYS_NS16550_xxx nonsense in include/configs/myboardname.h
> > >
> > > I checked include/configs/socfpga_common.h again and by its using
> > > Kconfig and DM_SERIAL, there are no CONFIG_SYS_NS16550_xxx defines left
> > > (other than CONFIG_SYS_NS16550_SERIAL, which I will remove).
> >
> > So it's not that easy apparently :)
> > At least CONFIG_SYS_NS16550_MEM32 is still used really required
> > for getting correct accessor used, see implementation of
> > serial_{in|out}_shift() in drivers/serial/ns16550.c.
> >
> > If CONFIG_SYS_NS16550_MEM32 is not set then simple readb() is used so
> > that 8-bit data offset in 32-bit word is lost and we're dead in the water.
> 
> Isn't that covered by 'plat->reg_shift' which is read from dts property
> "reg-shift = <2>"?

Well actually CONFIG_SYS_NS16550_MEM32 is an alias to "reg-io-width = <4>"
which selects exactly accessor (right as CONFIG_SYS_NS16550_MEM32 in
serial_{in|out}_shift()). But even though we do read "reg-io-width"
from .dtb it is not used in the driver. It was added by Andy Shevchenko
recently, see http://git.denx.de/?p=u-boot.git;a=commit;h=4e7207791c05b3ecff916c1b1b1b21401ec29b0b

BTW as I may see some SoCFPGA configs do mention this define:
---------------------------------->8-----------------------------
# git grep CONFIG_SYS_NS16550_MEM32 | grep socfpga
include/configs/socfpga_arria10_socdk.h:33:#define CONFIG_SYS_NS16550_MEM32
include/configs/socfpga_stratix10_socdk.h:146:#define CONFIG_SYS_NS16550_MEM32
---------------------------------->8-----------------------------

as well as this DT property:
---------------------------------->8-----------------------------
# git grep reg-io-width | grep socfpga
arch/arm/dts/socfpga.dtsi:877:                  reg-io-width = <4>;
arch/arm/dts/socfpga.dtsi:889:                  reg-io-width = <4>;
arch/arm/dts/socfpga_arria10.dtsi:829:                  reg-io-width = <4>;
arch/arm/dts/socfpga_arria10.dtsi:840:                  reg-io-width = <4>;
arch/arm/dts/socfpga_stratix10.dtsi:252:                        reg-io-width = <4>;
arch/arm/dts/socfpga_stratix10.dtsi:265:                        reg-io-width = <4>;
arch/arm/dts/socfpga_stratix10.dtsi:314:                        reg-io-width = <4>;
arch/arm/dts/socfpga_stratix10.dtsi:326:                        reg-io-width = <4>;
---------------------------------->8-----------------------------

So I guess you may experiment with it a bit.

-Alexey

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Simon Goldschmidt]

On Fri, Jan 11, 2019 at 10:03 AM Alexey Brodkin
<alexey.brodkin@synopsys.com> wrote:
>
> Hi Simon,
>
> > -----Original Message-----
> > From: Simon Goldschmidt [mailto:simon.k.r.goldschmidt at gmail.com]
> > Sent: Friday, January 11, 2019 11:41 AM
> > To: Alexey Brodkin <alexey.brodkin@synopsys.com>
> > Cc: Patrice Chotard <patrice.chotard@st.com>; Simon Glass <sjg@chromium.org>; Anup Patel
> > <anup@brainfault.org>; Lokesh Vutla <lokeshvutla@ti.com>; Patrick Delaunay <patrick.delaunay@st.com>;
> > Marek Vasut <marek.vasut@gmail.com>; u-boot at lists.denx.de; Álvaro Fernández Rojas <noltari@gmail.com>;
> > Ryder Lee <ryder.lee@mediatek.com>; Vikas Manocha <vikas.manocha@st.com>; Alexander Graf
> > <agraf@suse.de>; Weijie Gao <weijie.gao@mediatek.com>; Marek Vasut <marex@denx.de>
> > Subject: Re: [PATCH v1 3/4] serial: add an of-platdata driver for "snps,dw-apb-uart"
> >
> > On Fri, Jan 11, 2019 at 9:33 AM Alexey Brodkin
> > <alexey.brodkin@synopsys.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > [snip]
> > >
> > > > >> +config DESIGNWARE_SERIAL
> > > > >> +  bool "DesignWare UART support"
> > > > >> +  depends on DM_SERIAL && SPL_OF_PLATDATA
> > > > >
> > > > > Might be a bit naïve question but why depend on SPL_OF_PLATDATA only?
> > > > > What about CONFIG_OF_EMBED?
> > >
> > > Ok I completely forgot that standard ns16550 driver already covers DW APB UART,
> > > see https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_u-
> > 2Dboot_latest_source_drivers_serial_ns16550.c-
> > 23L456&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=wyhoU5_3rGv5y
> > 373_cpetmUHK9_ALMCC29QnKS2As5k&s=uMDuaOZ__YoyqakveKqByAtb1lfRQBYktcVgGH3oawE&e=
> > >
> > > > > I'd happily switch my ARC boards on this driver and get rid of all
> > > > > CONFIG_SYS_NS16550_xxx nonsense in include/configs/myboardname.h
> > > >
> > > > I checked include/configs/socfpga_common.h again and by its using
> > > > Kconfig and DM_SERIAL, there are no CONFIG_SYS_NS16550_xxx defines left
> > > > (other than CONFIG_SYS_NS16550_SERIAL, which I will remove).
> > >
> > > So it's not that easy apparently :)
> > > At least CONFIG_SYS_NS16550_MEM32 is still used really required
> > > for getting correct accessor used, see implementation of
> > > serial_{in|out}_shift() in drivers/serial/ns16550.c.
> > >
> > > If CONFIG_SYS_NS16550_MEM32 is not set then simple readb() is used so
> > > that 8-bit data offset in 32-bit word is lost and we're dead in the water.
> >
> > Isn't that covered by 'plat->reg_shift' which is read from dts property
> > "reg-shift = <2>"?
>
> Well actually CONFIG_SYS_NS16550_MEM32 is an alias to "reg-io-width = <4>"
> which selects exactly accessor (right as CONFIG_SYS_NS16550_MEM32 in
> serial_{in|out}_shift()). But even though we do read "reg-io-width"
> from .dtb it is not used in the driver. It was added by Andy Shevchenko
> recently, see http://git.denx.de/?p=u-boot.git;a=commit;h=4e7207791c05b3ecff916c1b1b1b21401ec29b0b
>
> BTW as I may see some SoCFPGA configs do mention this define:
> ---------------------------------->8-----------------------------
> # git grep CONFIG_SYS_NS16550_MEM32 | grep socfpga
> include/configs/socfpga_arria10_socdk.h:33:#define CONFIG_SYS_NS16550_MEM32
> include/configs/socfpga_stratix10_socdk.h:146:#define CONFIG_SYS_NS16550_MEM32
> ---------------------------------->8-----------------------------

Right. Sorry for being unclear. I'm working on the "sub-mach" "Gen5" only. Those
two are different types that I can't really test. Seems like Arria10
and Stratix10 might
not be converted to DM yet?

>
> as well as this DT property:
> ---------------------------------->8-----------------------------
> # git grep reg-io-width | grep socfpga
> arch/arm/dts/socfpga.dtsi:877:                  reg-io-width = <4>;
> arch/arm/dts/socfpga.dtsi:889:                  reg-io-width = <4>;

Those two are of interest to my boards.

> arch/arm/dts/socfpga_arria10.dtsi:829:                  reg-io-width = <4>;
> arch/arm/dts/socfpga_arria10.dtsi:840:                  reg-io-width = <4>;
> arch/arm/dts/socfpga_stratix10.dtsi:252:                        reg-io-width = <4>;
> arch/arm/dts/socfpga_stratix10.dtsi:265:                        reg-io-width = <4>;
> arch/arm/dts/socfpga_stratix10.dtsi:314:                        reg-io-width = <4>;
> arch/arm/dts/socfpga_stratix10.dtsi:326:                        reg-io-width = <4>;
> ---------------------------------->8-----------------------------
>
> So I guess you may experiment with it a bit.

Ok, you're right there. I guess it just works for me when using 8-bit access,
but you're right that the driver should use 32-bit access when configured like
that via 'reg-io-width'. I'll have a look at that.

Thanks for insisting ;-)

Regards,
Simon

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Andy Shevchenko]

On Wed, Jan 9, 2019 at 10:36 AM Alexey Brodkin
<alexey.brodkin@synopsys.com> wrote:


> Might be a bit naïve question but why depend on SPL_OF_PLATDATA only?
> What about CONFIG_OF_EMBED?

No driver should depend on OF_EMBED IIUC. New U-Boot spils a warning
when production boards are using it.


-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Simon Goldschmidt]

On Fri, Jan 11, 2019 at 10:22 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jan 9, 2019 at 10:36 AM Alexey Brodkin
> <alexey.brodkin@synopsys.com> wrote:
>
>
> > Might be a bit naïve question but why depend on SPL_OF_PLATDATA only?
> > What about CONFIG_OF_EMBED?
>
> No driver should depend on OF_EMBED IIUC. New U-Boot spils a warning
> when production boards are using it.

By now, actually I even dislike the idea of depending on OF_PLATDATA myself.
Maybe we should fix the OF_PLATDATA to driver matching code to better handle
drivers with multiple compatible strings instead of adding dedicated drivers.

Regards,
Simon

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Simon Glass]

Hi Simon,

On Fri, 11 Jan 2019 at 03:02, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> On Fri, Jan 11, 2019 at 10:22 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Wed, Jan 9, 2019 at 10:36 AM Alexey Brodkin
> > <alexey.brodkin@synopsys.com> wrote:
> >
> >
> > > Might be a bit naïve question but why depend on SPL_OF_PLATDATA only?
> > > What about CONFIG_OF_EMBED?
> >
> > No driver should depend on OF_EMBED IIUC. New U-Boot spils a warning
> > when production boards are using it.
>
> By now, actually I even dislike the idea of depending on OF_PLATDATA myself.
> Maybe we should fix the OF_PLATDATA to driver matching code to better handle
> drivers with multiple compatible strings instead of adding dedicated drivers.

Sounds good.

One option I rejected with the initial impl was to add tags to the DT
to tell dtoc what to do. I still hope to avoid that, but it is an
option, if helpful.

Regards,
Simon

[U-Boot] [PATCH v1 3/4] serial: add an of-platdata driver for "snps, dw-apb-uart"

[Simon Glass]

On Mon, 7 Jan 2019 at 14:14, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> Add a driver for the "snps,dw-apb-uart" used in socfpga and others.
>
> This driver is required to get OF_PLATDATA to work for socfpga.
> It uses the ns16550 driver, converting the platdata from of-platdata
> go the ns16550 format.
>
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
>
>  drivers/serial/Kconfig         | 10 ++++++++
>  drivers/serial/Makefile        |  1 +
>  drivers/serial/serial_dw_apb.c | 42 ++++++++++++++++++++++++++++++++++
>  3 files changed, 53 insertions(+)
>  create mode 100644 drivers/serial/serial_dw_apb.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v1 4/4] mmc: socfpga: support of-platdata

[Simon Goldschmidt]

This adds support for OF_PLATDATA to the socfpga_dw_mmc driver.

To do that, also the name of the driver has to be changed to match the
compatible string in the platdata generated from dts.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

---

 drivers/mmc/socfpga_dw_mmc.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c
index 739c1629a2..8503dff932 100644
--- a/drivers/mmc/socfpga_dw_mmc.c
+++ b/drivers/mmc/socfpga_dw_mmc.c
@@ -8,6 +8,7 @@
 #include <asm/arch/system_manager.h>
 #include <clk.h>
 #include <dm.h>
+#include <dt-structs.h>
 #include <dwmmc.h>
 #include <errno.h>
 #include <fdtdec.h>
@@ -24,6 +25,9 @@ static const struct socfpga_system_manager *system_manager_base =
 		(void *)SOCFPGA_SYSMGR_ADDRESS;
 
 struct socfpga_dwmci_plat {
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+	struct dtd_altr_socfpga_dw_mshc dtplat;
+#endif
 	struct mmc_config cfg;
 	struct mmc mmc;
 };
@@ -100,6 +104,7 @@ static int socfpga_dwmmc_get_clk_rate(struct udevice *dev)
 
 static int socfpga_dwmmc_ofdata_to_platdata(struct udevice *dev)
 {
+#if !CONFIG_IS_ENABLED(OF_PLATDATA)
 	struct dwmci_socfpga_priv_data *priv = dev_get_priv(dev);
 	struct dwmci_host *host = &priv->host;
 	int fifo_depth;
@@ -129,13 +134,13 @@ static int socfpga_dwmmc_ofdata_to_platdata(struct udevice *dev)
 	priv->smplsel = fdtdec_get_uint(gd->fdt_blob, dev_of_offset(dev),
 					"smplsel", 0);
 	host->priv = priv;
-
+#endif
 	return 0;
 }
 
 static int socfpga_dwmmc_probe(struct udevice *dev)
 {
-#ifdef CONFIG_BLK
+#if defined(CONFIG_BLK) || CONFIG_IS_ENABLED(OF_PLATDATA)
 	struct socfpga_dwmci_plat *plat = dev_get_platdata(dev);
 #endif
 	struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
@@ -143,6 +148,23 @@ static int socfpga_dwmmc_probe(struct udevice *dev)
 	struct dwmci_host *host = &priv->host;
 	int ret;
 
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+	struct dtd_altr_socfpga_dw_mshc *dtplat = &plat->dtplat;
+
+	host->name = dev->name;
+	host->ioaddr = (void *)dtplat->reg[0];
+	host->buswidth = dtplat->bus_width;
+	host->clksel = socfpga_dwmci_clksel;
+	host->dev_index = 0;
+	host->fifoth_val = MSIZE(0x2) |
+		RX_WMARK(dtplat->fifo_depth / 2 - 1) |
+		TX_WMARK(dtplat->fifo_depth / 2);
+	host->priv = priv;
+	/* TODO: these are optional, of-platdata does not support this. */
+	priv->drvsel = 3;
+	priv->smplsel = 0;
+#endif
+
 	ret = socfpga_dwmmc_get_clk_rate(dev);
 	if (ret)
 		return ret;
@@ -185,7 +207,7 @@ static const struct udevice_id socfpga_dwmmc_ids[] = {
 };
 
 U_BOOT_DRIVER(socfpga_dwmmc_drv) = {
-	.name		= "socfpga_dwmmc",
+	.name		= "altr_socfpga_dw_mshc",
 	.id		= UCLASS_MMC,
 	.of_match	= socfpga_dwmmc_ids,
 	.ofdata_to_platdata = socfpga_dwmmc_ofdata_to_platdata,
-- 
2.17.1

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Lukasz Majewski]

Hi Simon,

> This is an initial attempt to support OF_PLATDATA for socfpga gen5.
> 
> There are two motivations for this:
> a) reduce code size to eventually support secure boot (where SPL has
> to authenticate the next stage by loading/checking U-Boot from a FIT
>    image)
> b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
>    (on warm-restart), all bytes to check need to be in one piece. With
>    OF_SEPARATE, this is not the case (.bss is between .rodata and the
>    DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
>    be a good solution.

I'm not an expert with SOCFPGA nor your application, but maybe
selecting: "SPL_SEPARATE_BSS" would fix your issue?

This option allows putting on IMX6 SPL the DTB blob into OCRAM (internal
RAM), before the DDR is setup (so it would be possible to calculate
CRC). 

I've stumbled upon such issue when I wanted to switch from OF_EMBEDDED
to OF_SEPARATE.

> 
> This series enables booting from MMC with OF_PLATDATA. Booting from
> SPI does not yet work (I'm working on that, but it's tougher than
> expected).
> 
> Tested on the socrates board. To use it:
> - use socfpga_socrates_defconfig
> - disable CONFIG_SPL_SPI_FLASH_SUPPORT, CONFIG_SPL_SPI_SUPPORT and
>   CONFIG_SPL_DM_RESET (those don't work yet)
> - enable CONFIG_DESIGNWARE_SERIAL (required for console)
> 
> 
> Simon Goldschmidt (4):
>   arm: socfpga: imply SPL config instead of select
>   arm: socfpga: fix compiling with OF_PLATDATA
>   serial: add an of-platdata driver for "snps,dw-apb-uart"
>   mmc: socfpga: support of-platdata
> 
>  arch/arm/Kconfig               |  4 ++--
>  arch/arm/mach-socfpga/misc.c   |  2 +-
>  drivers/mmc/socfpga_dw_mmc.c   | 28 ++++++++++++++++++++---
>  drivers/serial/Kconfig         | 10 ++++++++
>  drivers/serial/Makefile        |  1 +
>  drivers/serial/serial_dw_apb.c | 42
> ++++++++++++++++++++++++++++++++++ 6 files changed, 81 insertions(+),
> 6 deletions(-) create mode 100644 drivers/serial/serial_dw_apb.c
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190107/5504b18d/attachment.sig>

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Simon Goldschmidt]

On Mon, Jan 7, 2019 at 10:59 PM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Simon,
>
> > This is an initial attempt to support OF_PLATDATA for socfpga gen5.
> >
> > There are two motivations for this:
> > a) reduce code size to eventually support secure boot (where SPL has
> > to authenticate the next stage by loading/checking U-Boot from a FIT
> >    image)
> > b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
> >    (on warm-restart), all bytes to check need to be in one piece. With
> >    OF_SEPARATE, this is not the case (.bss is between .rodata and the
> >    DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
> >    be a good solution.
>
> I'm not an expert with SOCFPGA nor your application, but maybe
> selecting: "SPL_SEPARATE_BSS" would fix your issue?
>
> This option allows putting on IMX6 SPL the DTB blob into OCRAM (internal
> RAM), before the DDR is setup (so it would be possible to calculate
> CRC).

In socfpga, the whole SPL is copied into OCRAM by the boot ROM and executed
from there. How can OF_SEPARATE help here? I tried enabling it but the SPL map
file does not change. The only difference is there is no padding
between the binary
and the DTB ('u-boot-spl-nodtb.bin' vs 'u-boot-spl-dtb.bin')

Regards,
Simon

>
> I've stumbled upon such issue when I wanted to switch from OF_EMBEDDED
> to OF_SEPARATE.
>
> >
> > This series enables booting from MMC with OF_PLATDATA. Booting from
> > SPI does not yet work (I'm working on that, but it's tougher than
> > expected).
> >
> > Tested on the socrates board. To use it:
> > - use socfpga_socrates_defconfig
> > - disable CONFIG_SPL_SPI_FLASH_SUPPORT, CONFIG_SPL_SPI_SUPPORT and
> >   CONFIG_SPL_DM_RESET (those don't work yet)
> > - enable CONFIG_DESIGNWARE_SERIAL (required for console)
> >
> >
> > Simon Goldschmidt (4):
> >   arm: socfpga: imply SPL config instead of select
> >   arm: socfpga: fix compiling with OF_PLATDATA
> >   serial: add an of-platdata driver for "snps,dw-apb-uart"
> >   mmc: socfpga: support of-platdata
> >
> >  arch/arm/Kconfig               |  4 ++--
> >  arch/arm/mach-socfpga/misc.c   |  2 +-
> >  drivers/mmc/socfpga_dw_mmc.c   | 28 ++++++++++++++++++++---
> >  drivers/serial/Kconfig         | 10 ++++++++
> >  drivers/serial/Makefile        |  1 +
> >  drivers/serial/serial_dw_apb.c | 42
> > ++++++++++++++++++++++++++++++++++ 6 files changed, 81 insertions(+),
> > 6 deletions(-) create mode 100644 drivers/serial/serial_dw_apb.c
> >
>
>
>
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Marek Vasut]

On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> This is an initial attempt to support OF_PLATDATA for socfpga gen5.
> 
> There are two motivations for this:
> a) reduce code size to eventually support secure boot (where SPL has to
>    authenticate the next stage by loading/checking U-Boot from a FIT
>    image)
> b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
>    (on warm-restart), all bytes to check need to be in one piece. With
>    OF_SEPARATE, this is not the case (.bss is between .rodata and the
>    DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
>    be a good solution.

I'd much prefer parsing the DT (and thus, decoupling the SW from HW)
than having some ad-hoc plat data again if we can avoid that.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Simon Goldschmidt]

On Mon, Jan 7, 2019 at 11:59 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> > This is an initial attempt to support OF_PLATDATA for socfpga gen5.
> >
> > There are two motivations for this:
> > a) reduce code size to eventually support secure boot (where SPL has to
> >    authenticate the next stage by loading/checking U-Boot from a FIT
> >    image)
> > b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
> >    (on warm-restart), all bytes to check need to be in one piece. With
> >    OF_SEPARATE, this is not the case (.bss is between .rodata and the
> >    DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
> >    be a good solution.
>
> I'd much prefer parsing the DT (and thus, decoupling the SW from HW)
> than having some ad-hoc plat data again if we can avoid that.

So you're against the whole OF_PLATDATA thing or how should I understand
that?

It's not really ad-hoc, it's the DT converted to C structs. It's just in another
format, but it's still (sort of) decoupled SW from HW.

As written above, I have two goals I want to achieve with this. Right now, I
cannot enable verified boot in SPL because the available OCRAM cannot
hold all the code. And it seemed to me OF_PLATDATA could help me there.

Regards,
Simon

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Marek Vasut]

On 1/8/19 7:56 AM, Simon Goldschmidt wrote:
> On Mon, Jan 7, 2019 at 11:59 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>> This is an initial attempt to support OF_PLATDATA for socfpga gen5.
>>>
>>> There are two motivations for this:
>>> a) reduce code size to eventually support secure boot (where SPL has to
>>>    authenticate the next stage by loading/checking U-Boot from a FIT
>>>    image)
>>> b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
>>>    (on warm-restart), all bytes to check need to be in one piece. With
>>>    OF_SEPARATE, this is not the case (.bss is between .rodata and the
>>>    DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
>>>    be a good solution.
>>
>> I'd much prefer parsing the DT (and thus, decoupling the SW from HW)
>> than having some ad-hoc plat data again if we can avoid that.
> 
> So you're against the whole OF_PLATDATA thing or how should I understand
> that?

If we can avoid it, I'd prefer to do so.

> It's not really ad-hoc, it's the DT converted to C structs. It's just in another
> format, but it's still (sort of) decoupled SW from HW.
> 
> As written above, I have two goals I want to achieve with this. Right now, I
> cannot enable verified boot in SPL because the available OCRAM cannot
> hold all the code. And it seemed to me OF_PLATDATA could help me there.

Well this might be a long shot, but I discussed this lack of OCRAM
during 35C3 and there was a suggestion to lock L2 cache lines above ROM
(so there's some backing store) and use that as extra SRAM. Would that
help you ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Simon Goldschmidt]

On Tue, Jan 8, 2019 at 1:06 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/8/19 7:56 AM, Simon Goldschmidt wrote:
> > On Mon, Jan 7, 2019 at 11:59 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>> This is an initial attempt to support OF_PLATDATA for socfpga gen5.
> >>>
> >>> There are two motivations for this:
> >>> a) reduce code size to eventually support secure boot (where SPL has to
> >>>    authenticate the next stage by loading/checking U-Boot from a FIT
> >>>    image)
> >>> b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
> >>>    (on warm-restart), all bytes to check need to be in one piece. With
> >>>    OF_SEPARATE, this is not the case (.bss is between .rodata and the
> >>>    DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
> >>>    be a good solution.
> >>
> >> I'd much prefer parsing the DT (and thus, decoupling the SW from HW)
> >> than having some ad-hoc plat data again if we can avoid that.
> >
> > So you're against the whole OF_PLATDATA thing or how should I understand
> > that?
>
> If we can avoid it, I'd prefer to do so.
>
> > It's not really ad-hoc, it's the DT converted to C structs. It's just in another
> > format, but it's still (sort of) decoupled SW from HW.
> >
> > As written above, I have two goals I want to achieve with this. Right now, I
> > cannot enable verified boot in SPL because the available OCRAM cannot
> > hold all the code. And it seemed to me OF_PLATDATA could help me there.
>
> Well this might be a long shot, but I discussed this lack of OCRAM
> during 35C3 and there was a suggestion to lock L2 cache lines above ROM
> (so there's some backing store) and use that as extra SRAM. Would that
> help you ?

I would have joined that discussion if my Family would have let me go during the
holidays :-))

This is an interesing idea, but actually it's a lack of code/rodata
size. The Intel
docs clearly state that the binary SPL loaded from SPI/MMC must be 60 KiB at
max. I have not checked the code size increase I would get when enabling trusted
boot (SPL loading U-Boot from FIT and verifying it with a public key),
but I'm currently
at ~45 KiB for .text, .rodata and DTB and only 40 bytes for BSS. I'm
booting from SPI.
When booting from MMC, the code is about ~4 KiB smaller but BSS grows to ~600
Bytes.

Of course the stack and initial malloc area do need some bytes too, but I think
summed up, bss, stack and malloc should probably fit into 4 KiB, so I
currently have
about 15 KiB to add FIT loading and public key verification/hashing. I
don't think that's
enough just from the code size.

And on socfpga, I think all added code would use the heap, which is
changed to SDRAM
very early, so it's not the RAM that is tight.


Regards,
Simon

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Marek Vasut]

On 1/8/19 1:38 PM, Simon Goldschmidt wrote:
> On Tue, Jan 8, 2019 at 1:06 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/8/19 7:56 AM, Simon Goldschmidt wrote:
>>> On Mon, Jan 7, 2019 at 11:59 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>> This is an initial attempt to support OF_PLATDATA for socfpga gen5.
>>>>>
>>>>> There are two motivations for this:
>>>>> a) reduce code size to eventually support secure boot (where SPL has to
>>>>>    authenticate the next stage by loading/checking U-Boot from a FIT
>>>>>    image)
>>>>> b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
>>>>>    (on warm-restart), all bytes to check need to be in one piece. With
>>>>>    OF_SEPARATE, this is not the case (.bss is between .rodata and the
>>>>>    DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
>>>>>    be a good solution.
>>>>
>>>> I'd much prefer parsing the DT (and thus, decoupling the SW from HW)
>>>> than having some ad-hoc plat data again if we can avoid that.
>>>
>>> So you're against the whole OF_PLATDATA thing or how should I understand
>>> that?
>>
>> If we can avoid it, I'd prefer to do so.
>>
>>> It's not really ad-hoc, it's the DT converted to C structs. It's just in another
>>> format, but it's still (sort of) decoupled SW from HW.
>>>
>>> As written above, I have two goals I want to achieve with this. Right now, I
>>> cannot enable verified boot in SPL because the available OCRAM cannot
>>> hold all the code. And it seemed to me OF_PLATDATA could help me there.
>>
>> Well this might be a long shot, but I discussed this lack of OCRAM
>> during 35C3 and there was a suggestion to lock L2 cache lines above ROM
>> (so there's some backing store) and use that as extra SRAM. Would that
>> help you ?
> 
> I would have joined that discussion if my Family would have let me go during the
> holidays :-))
> 
> This is an interesing idea, but actually it's a lack of code/rodata
> size. The Intel
> docs clearly state that the binary SPL loaded from SPI/MMC must be 60 KiB at
> max. I have not checked the code size increase I would get when enabling trusted
> boot (SPL loading U-Boot from FIT and verifying it with a public key),
> but I'm currently
> at ~45 KiB for .text, .rodata and DTB and only 40 bytes for BSS. I'm
> booting from SPI.
> When booting from MMC, the code is about ~4 KiB smaller but BSS grows to ~600
> Bytes.

I wonder if there are some huge chunks of code which could be optimized?

> Of course the stack and initial malloc area do need some bytes too, but I think
> summed up, bss, stack and malloc should probably fit into 4 KiB, so I
> currently have
> about 15 KiB to add FIT loading and public key verification/hashing. I
> don't think that's
> enough just from the code size.
> 
> And on socfpga, I think all added code would use the heap, which is
> changed to SDRAM
> very early, so it's not the RAM that is tight.

Can you check readelf and see how the function size looks ? Maybe
there's something which is just too big ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Simon Goldschmidt]

On Tue, Jan 8, 2019 at 1:58 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/8/19 1:38 PM, Simon Goldschmidt wrote:
> > On Tue, Jan 8, 2019 at 1:06 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 1/8/19 7:56 AM, Simon Goldschmidt wrote:
> >>> On Mon, Jan 7, 2019 at 11:59 PM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>>>> This is an initial attempt to support OF_PLATDATA for socfpga gen5.
> >>>>>
> >>>>> There are two motivations for this:
> >>>>> a) reduce code size to eventually support secure boot (where SPL has to
> >>>>>    authenticate the next stage by loading/checking U-Boot from a FIT
> >>>>>    image)
> >>>>> b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
> >>>>>    (on warm-restart), all bytes to check need to be in one piece. With
> >>>>>    OF_SEPARATE, this is not the case (.bss is between .rodata and the
> >>>>>    DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
> >>>>>    be a good solution.
> >>>>
> >>>> I'd much prefer parsing the DT (and thus, decoupling the SW from HW)
> >>>> than having some ad-hoc plat data again if we can avoid that.
> >>>
> >>> So you're against the whole OF_PLATDATA thing or how should I understand
> >>> that?
> >>
> >> If we can avoid it, I'd prefer to do so.
> >>
> >>> It's not really ad-hoc, it's the DT converted to C structs. It's just in another
> >>> format, but it's still (sort of) decoupled SW from HW.
> >>>
> >>> As written above, I have two goals I want to achieve with this. Right now, I
> >>> cannot enable verified boot in SPL because the available OCRAM cannot
> >>> hold all the code. And it seemed to me OF_PLATDATA could help me there.
> >>
> >> Well this might be a long shot, but I discussed this lack of OCRAM
> >> during 35C3 and there was a suggestion to lock L2 cache lines above ROM
> >> (so there's some backing store) and use that as extra SRAM. Would that
> >> help you ?
> >
> > I would have joined that discussion if my Family would have let me go during the
> > holidays :-))
> >
> > This is an interesing idea, but actually it's a lack of code/rodata
> > size. The Intel
> > docs clearly state that the binary SPL loaded from SPI/MMC must be 60 KiB at
> > max. I have not checked the code size increase I would get when enabling trusted
> > boot (SPL loading U-Boot from FIT and verifying it with a public key),
> > but I'm currently
> > at ~45 KiB for .text, .rodata and DTB and only 40 bytes for BSS. I'm
> > booting from SPI.
> > When booting from MMC, the code is about ~4 KiB smaller but BSS grows to ~600
> > Bytes.
>
> I wonder if there are some huge chunks of code which could be optimized?
>
> > Of course the stack and initial malloc area do need some bytes too, but I think
> > summed up, bss, stack and malloc should probably fit into 4 KiB, so I
> > currently have
> > about 15 KiB to add FIT loading and public key verification/hashing. I
> > don't think that's
> > enough just from the code size.
> >
> > And on socfpga, I think all added code would use the heap, which is
> > changed to SDRAM
> > very early, so it's not the RAM that is tight.
>
> Can you check readelf and see how the function size looks ? Maybe
> there's something which is just too big ?

I'm looking at the map file all the time ;-) The only thing that looks
too big is
SDRAM initialization, which is about 16 KiB overall, I think. The rest
just seems
to be smaller parts. But the binary blob u32 arrays created by Quartus don't
help, either: rodata is about 9 KiB.

Regards,
Simon

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Marek Vasut]

On 1/8/19 2:07 PM, Simon Goldschmidt wrote:
> On Tue, Jan 8, 2019 at 1:58 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/8/19 1:38 PM, Simon Goldschmidt wrote:
>>> On Tue, Jan 8, 2019 at 1:06 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 1/8/19 7:56 AM, Simon Goldschmidt wrote:
>>>>> On Mon, Jan 7, 2019 at 11:59 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>> This is an initial attempt to support OF_PLATDATA for socfpga gen5.
>>>>>>>
>>>>>>> There are two motivations for this:
>>>>>>> a) reduce code size to eventually support secure boot (where SPL has to
>>>>>>>    authenticate the next stage by loading/checking U-Boot from a FIT
>>>>>>>    image)
>>>>>>> b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
>>>>>>>    (on warm-restart), all bytes to check need to be in one piece. With
>>>>>>>    OF_SEPARATE, this is not the case (.bss is between .rodata and the
>>>>>>>    DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
>>>>>>>    be a good solution.
>>>>>>
>>>>>> I'd much prefer parsing the DT (and thus, decoupling the SW from HW)
>>>>>> than having some ad-hoc plat data again if we can avoid that.
>>>>>
>>>>> So you're against the whole OF_PLATDATA thing or how should I understand
>>>>> that?
>>>>
>>>> If we can avoid it, I'd prefer to do so.
>>>>
>>>>> It's not really ad-hoc, it's the DT converted to C structs. It's just in another
>>>>> format, but it's still (sort of) decoupled SW from HW.
>>>>>
>>>>> As written above, I have two goals I want to achieve with this. Right now, I
>>>>> cannot enable verified boot in SPL because the available OCRAM cannot
>>>>> hold all the code. And it seemed to me OF_PLATDATA could help me there.
>>>>
>>>> Well this might be a long shot, but I discussed this lack of OCRAM
>>>> during 35C3 and there was a suggestion to lock L2 cache lines above ROM
>>>> (so there's some backing store) and use that as extra SRAM. Would that
>>>> help you ?
>>>
>>> I would have joined that discussion if my Family would have let me go during the
>>> holidays :-))
>>>
>>> This is an interesing idea, but actually it's a lack of code/rodata
>>> size. The Intel
>>> docs clearly state that the binary SPL loaded from SPI/MMC must be 60 KiB at
>>> max. I have not checked the code size increase I would get when enabling trusted
>>> boot (SPL loading U-Boot from FIT and verifying it with a public key),
>>> but I'm currently
>>> at ~45 KiB for .text, .rodata and DTB and only 40 bytes for BSS. I'm
>>> booting from SPI.
>>> When booting from MMC, the code is about ~4 KiB smaller but BSS grows to ~600
>>> Bytes.
>>
>> I wonder if there are some huge chunks of code which could be optimized?
>>
>>> Of course the stack and initial malloc area do need some bytes too, but I think
>>> summed up, bss, stack and malloc should probably fit into 4 KiB, so I
>>> currently have
>>> about 15 KiB to add FIT loading and public key verification/hashing. I
>>> don't think that's
>>> enough just from the code size.
>>>
>>> And on socfpga, I think all added code would use the heap, which is
>>> changed to SDRAM
>>> very early, so it's not the RAM that is tight.
>>
>> Can you check readelf and see how the function size looks ? Maybe
>> there's something which is just too big ?
> 
> I'm looking at the map file all the time ;-) The only thing that looks
> too big is
> SDRAM initialization, which is about 16 KiB overall, I think. The rest
> just seems
> to be smaller parts. But the binary blob u32 arrays created by Quartus don't
> help, either: rodata is about 9 KiB.

Can that be somehow optimized ? The ideal approach would be to move it
somehow to DT.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Simon Goldschmidt]

On Tue, Jan 8, 2019 at 2:38 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/8/19 2:07 PM, Simon Goldschmidt wrote:
> > On Tue, Jan 8, 2019 at 1:58 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 1/8/19 1:38 PM, Simon Goldschmidt wrote:
> >>> On Tue, Jan 8, 2019 at 1:06 PM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 1/8/19 7:56 AM, Simon Goldschmidt wrote:
> >>>>> On Mon, Jan 7, 2019 at 11:59 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
> >>>>>>> This is an initial attempt to support OF_PLATDATA for socfpga gen5.
> >>>>>>>
> >>>>>>> There are two motivations for this:
> >>>>>>> a) reduce code size to eventually support secure boot (where SPL has to
> >>>>>>>    authenticate the next stage by loading/checking U-Boot from a FIT
> >>>>>>>    image)
> >>>>>>> b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
> >>>>>>>    (on warm-restart), all bytes to check need to be in one piece. With
> >>>>>>>    OF_SEPARATE, this is not the case (.bss is between .rodata and the
> >>>>>>>    DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
> >>>>>>>    be a good solution.
> >>>>>>
> >>>>>> I'd much prefer parsing the DT (and thus, decoupling the SW from HW)
> >>>>>> than having some ad-hoc plat data again if we can avoid that.
> >>>>>
> >>>>> So you're against the whole OF_PLATDATA thing or how should I understand
> >>>>> that?
> >>>>
> >>>> If we can avoid it, I'd prefer to do so.
> >>>>
> >>>>> It's not really ad-hoc, it's the DT converted to C structs. It's just in another
> >>>>> format, but it's still (sort of) decoupled SW from HW.
> >>>>>
> >>>>> As written above, I have two goals I want to achieve with this. Right now, I
> >>>>> cannot enable verified boot in SPL because the available OCRAM cannot
> >>>>> hold all the code. And it seemed to me OF_PLATDATA could help me there.
> >>>>
> >>>> Well this might be a long shot, but I discussed this lack of OCRAM
> >>>> during 35C3 and there was a suggestion to lock L2 cache lines above ROM
> >>>> (so there's some backing store) and use that as extra SRAM. Would that
> >>>> help you ?
> >>>
> >>> I would have joined that discussion if my Family would have let me go during the
> >>> holidays :-))
> >>>
> >>> This is an interesing idea, but actually it's a lack of code/rodata
> >>> size. The Intel
> >>> docs clearly state that the binary SPL loaded from SPI/MMC must be 60 KiB at
> >>> max. I have not checked the code size increase I would get when enabling trusted
> >>> boot (SPL loading U-Boot from FIT and verifying it with a public key),
> >>> but I'm currently
> >>> at ~45 KiB for .text, .rodata and DTB and only 40 bytes for BSS. I'm
> >>> booting from SPI.
> >>> When booting from MMC, the code is about ~4 KiB smaller but BSS grows to ~600
> >>> Bytes.
> >>
> >> I wonder if there are some huge chunks of code which could be optimized?
> >>
> >>> Of course the stack and initial malloc area do need some bytes too, but I think
> >>> summed up, bss, stack and malloc should probably fit into 4 KiB, so I
> >>> currently have
> >>> about 15 KiB to add FIT loading and public key verification/hashing. I
> >>> don't think that's
> >>> enough just from the code size.
> >>>
> >>> And on socfpga, I think all added code would use the heap, which is
> >>> changed to SDRAM
> >>> very early, so it's not the RAM that is tight.
> >>
> >> Can you check readelf and see how the function size looks ? Maybe
> >> there's something which is just too big ?
> >
> > I'm looking at the map file all the time ;-) The only thing that looks
> > too big is
> > SDRAM initialization, which is about 16 KiB overall, I think. The rest
> > just seems
> > to be smaller parts. But the binary blob u32 arrays created by Quartus don't
> > help, either: rodata is about 9 KiB.
>
> Can that be somehow optimized ? The ideal approach would be to move it
> somehow to DT.

I don't know if those binary blobs (pin config, clock config etc.) can
be converted
without internal information from Intel.

The SDRAM initialization might just be bad code, I don't know.

So like you wrote in the other thread: obviously, we're doing something wrong
as those 60 KiB will not be enough for what I want SPL to do. But, I haven't yet
found something that is just obviously code bloat.

Regards,
Simon

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Marek Vasut]

On 1/8/19 2:51 PM, Simon Goldschmidt wrote:
> On Tue, Jan 8, 2019 at 2:38 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/8/19 2:07 PM, Simon Goldschmidt wrote:
>>> On Tue, Jan 8, 2019 at 1:58 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 1/8/19 1:38 PM, Simon Goldschmidt wrote:
>>>>> On Tue, Jan 8, 2019 at 1:06 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 1/8/19 7:56 AM, Simon Goldschmidt wrote:
>>>>>>> On Mon, Jan 7, 2019 at 11:59 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>> This is an initial attempt to support OF_PLATDATA for socfpga gen5.
>>>>>>>>>
>>>>>>>>> There are two motivations for this:
>>>>>>>>> a) reduce code size to eventually support secure boot (where SPL has to
>>>>>>>>>    authenticate the next stage by loading/checking U-Boot from a FIT
>>>>>>>>>    image)
>>>>>>>>> b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
>>>>>>>>>    (on warm-restart), all bytes to check need to be in one piece. With
>>>>>>>>>    OF_SEPARATE, this is not the case (.bss is between .rodata and the
>>>>>>>>>    DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
>>>>>>>>>    be a good solution.
>>>>>>>>
>>>>>>>> I'd much prefer parsing the DT (and thus, decoupling the SW from HW)
>>>>>>>> than having some ad-hoc plat data again if we can avoid that.
>>>>>>>
>>>>>>> So you're against the whole OF_PLATDATA thing or how should I understand
>>>>>>> that?
>>>>>>
>>>>>> If we can avoid it, I'd prefer to do so.
>>>>>>
>>>>>>> It's not really ad-hoc, it's the DT converted to C structs. It's just in another
>>>>>>> format, but it's still (sort of) decoupled SW from HW.
>>>>>>>
>>>>>>> As written above, I have two goals I want to achieve with this. Right now, I
>>>>>>> cannot enable verified boot in SPL because the available OCRAM cannot
>>>>>>> hold all the code. And it seemed to me OF_PLATDATA could help me there.
>>>>>>
>>>>>> Well this might be a long shot, but I discussed this lack of OCRAM
>>>>>> during 35C3 and there was a suggestion to lock L2 cache lines above ROM
>>>>>> (so there's some backing store) and use that as extra SRAM. Would that
>>>>>> help you ?
>>>>>
>>>>> I would have joined that discussion if my Family would have let me go during the
>>>>> holidays :-))
>>>>>
>>>>> This is an interesing idea, but actually it's a lack of code/rodata
>>>>> size. The Intel
>>>>> docs clearly state that the binary SPL loaded from SPI/MMC must be 60 KiB at
>>>>> max. I have not checked the code size increase I would get when enabling trusted
>>>>> boot (SPL loading U-Boot from FIT and verifying it with a public key),
>>>>> but I'm currently
>>>>> at ~45 KiB for .text, .rodata and DTB and only 40 bytes for BSS. I'm
>>>>> booting from SPI.
>>>>> When booting from MMC, the code is about ~4 KiB smaller but BSS grows to ~600
>>>>> Bytes.
>>>>
>>>> I wonder if there are some huge chunks of code which could be optimized?
>>>>
>>>>> Of course the stack and initial malloc area do need some bytes too, but I think
>>>>> summed up, bss, stack and malloc should probably fit into 4 KiB, so I
>>>>> currently have
>>>>> about 15 KiB to add FIT loading and public key verification/hashing. I
>>>>> don't think that's
>>>>> enough just from the code size.
>>>>>
>>>>> And on socfpga, I think all added code would use the heap, which is
>>>>> changed to SDRAM
>>>>> very early, so it's not the RAM that is tight.
>>>>
>>>> Can you check readelf and see how the function size looks ? Maybe
>>>> there's something which is just too big ?
>>>
>>> I'm looking at the map file all the time ;-) The only thing that looks
>>> too big is
>>> SDRAM initialization, which is about 16 KiB overall, I think. The rest
>>> just seems
>>> to be smaller parts. But the binary blob u32 arrays created by Quartus don't
>>> help, either: rodata is about 9 KiB.
>>
>> Can that be somehow optimized ? The ideal approach would be to move it
>> somehow to DT.
> 
> I don't know if those binary blobs (pin config, clock config etc.) can
> be converted
> without internal information from Intel.
> 
> The SDRAM initialization might just be bad code, I don't know.
> 
> So like you wrote in the other thread: obviously, we're doing something wrong
> as those 60 KiB will not be enough for what I want SPL to do. But, I haven't yet
> found something that is just obviously code bloat.

I wonder if the SDRAM init tables aren't a bit sparse for example. Maybe
something can be done there ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1 0/4] arm: socfgpa: support of-platdata

[Simon Goldschmidt]

Am 08.01.2019 um 15:43 schrieb Marek Vasut:
> On 1/8/19 2:51 PM, Simon Goldschmidt wrote:
>> On Tue, Jan 8, 2019 at 2:38 PM Marek Vasut <marex@denx.de> wrote:
>>>
>>> On 1/8/19 2:07 PM, Simon Goldschmidt wrote:
>>>> On Tue, Jan 8, 2019 at 1:58 PM Marek Vasut <marex@denx.de> wrote:
>>>>>
>>>>> On 1/8/19 1:38 PM, Simon Goldschmidt wrote:
>>>>>> On Tue, Jan 8, 2019 at 1:06 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>
>>>>>>> On 1/8/19 7:56 AM, Simon Goldschmidt wrote:
>>>>>>>> On Mon, Jan 7, 2019 at 11:59 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>
>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>> This is an initial attempt to support OF_PLATDATA for socfpga gen5.
>>>>>>>>>>
>>>>>>>>>> There are two motivations for this:
>>>>>>>>>> a) reduce code size to eventually support secure boot (where SPL has to
>>>>>>>>>>     authenticate the next stage by loading/checking U-Boot from a FIT
>>>>>>>>>>     image)
>>>>>>>>>> b) to support the cyclone 5 boot ROM's CRC check on the SPL in SRAM
>>>>>>>>>>     (on warm-restart), all bytes to check need to be in one piece. With
>>>>>>>>>>     OF_SEPARATE, this is not the case (.bss is between .rodata and the
>>>>>>>>>>     DTB). Since OF_EMBEDDED has been discouraged, OF_PLATDATA seems to
>>>>>>>>>>     be a good solution.
>>>>>>>>>
>>>>>>>>> I'd much prefer parsing the DT (and thus, decoupling the SW from HW)
>>>>>>>>> than having some ad-hoc plat data again if we can avoid that.
>>>>>>>>
>>>>>>>> So you're against the whole OF_PLATDATA thing or how should I understand
>>>>>>>> that?
>>>>>>>
>>>>>>> If we can avoid it, I'd prefer to do so.
>>>>>>>
>>>>>>>> It's not really ad-hoc, it's the DT converted to C structs. It's just in another
>>>>>>>> format, but it's still (sort of) decoupled SW from HW.
>>>>>>>>
>>>>>>>> As written above, I have two goals I want to achieve with this. Right now, I
>>>>>>>> cannot enable verified boot in SPL because the available OCRAM cannot
>>>>>>>> hold all the code. And it seemed to me OF_PLATDATA could help me there.
>>>>>>>
>>>>>>> Well this might be a long shot, but I discussed this lack of OCRAM
>>>>>>> during 35C3 and there was a suggestion to lock L2 cache lines above ROM
>>>>>>> (so there's some backing store) and use that as extra SRAM. Would that
>>>>>>> help you ?
>>>>>>
>>>>>> I would have joined that discussion if my Family would have let me go during the
>>>>>> holidays :-))
>>>>>>
>>>>>> This is an interesing idea, but actually it's a lack of code/rodata
>>>>>> size. The Intel
>>>>>> docs clearly state that the binary SPL loaded from SPI/MMC must be 60 KiB at
>>>>>> max. I have not checked the code size increase I would get when enabling trusted
>>>>>> boot (SPL loading U-Boot from FIT and verifying it with a public key),
>>>>>> but I'm currently
>>>>>> at ~45 KiB for .text, .rodata and DTB and only 40 bytes for BSS. I'm
>>>>>> booting from SPI.
>>>>>> When booting from MMC, the code is about ~4 KiB smaller but BSS grows to ~600
>>>>>> Bytes.
>>>>>
>>>>> I wonder if there are some huge chunks of code which could be optimized?
>>>>>
>>>>>> Of course the stack and initial malloc area do need some bytes too, but I think
>>>>>> summed up, bss, stack and malloc should probably fit into 4 KiB, so I
>>>>>> currently have
>>>>>> about 15 KiB to add FIT loading and public key verification/hashing. I
>>>>>> don't think that's
>>>>>> enough just from the code size.
>>>>>>
>>>>>> And on socfpga, I think all added code would use the heap, which is
>>>>>> changed to SDRAM
>>>>>> very early, so it's not the RAM that is tight.
>>>>>
>>>>> Can you check readelf and see how the function size looks ? Maybe
>>>>> there's something which is just too big ?
>>>>
>>>> I'm looking at the map file all the time ;-) The only thing that looks
>>>> too big is
>>>> SDRAM initialization, which is about 16 KiB overall, I think. The rest
>>>> just seems
>>>> to be smaller parts. But the binary blob u32 arrays created by Quartus don't
>>>> help, either: rodata is about 9 KiB.
>>>
>>> Can that be somehow optimized ? The ideal approach would be to move it
>>> somehow to DT.
>>
>> I don't know if those binary blobs (pin config, clock config etc.) can
>> be converted
>> without internal information from Intel.
>>
>> The SDRAM initialization might just be bad code, I don't know.
>>
>> So like you wrote in the other thread: obviously, we're doing something wrong
>> as those 60 KiB will not be enough for what I want SPL to do. But, I haven't yet
>> found something that is just obviously code bloat.
> 
> I wonder if the SDRAM init tables aren't a bit sparse for example. Maybe
> something can be done there ?

I'll drop this series. There are some more cleanups in SPL to do before 
I'll be working on platdata again and this one won't apply any more 
after those changes are done.


Regards,
Simon