Re: [PATCH v3 06/15] ARM: at91: import lowlevel clock initialization from at91bootstrap
Hi Ahmad. > >> @@ -1,4 +1,5 @@ > >> obj-y += setup.o > >> +pbl-y += at91_pmc_ll.o > > > > Will this always make my pbl image larger or do we have some linker > > magic that throws away the unused code? > > My understanding is that barebox instructs GCC to place each out-of-line > function > into a separate section (with -ffunction-sections) and then the linker prunes > all > unreferenced sections. > > > If the pbl image become larger then we need to add this in a conditional > > manner > > as we cannot allow this for all at91 variants. > > I used to have conditional function definitions in a prior version (albeit > for the SDRAM variants initialization routines) and Sascha commented that > the #ifdefery can be removed as the linker takes care of it. I should have known already that we used --function-sections as I worked with this in the kernel in my former life. Much better than ifdeffery all over the code or in Makefiles. Sam ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v3 04/15] ARM: at91: replace at91sam9_ddrsdr.h with at91bootstrap's at91_ddrsdrc.h
Hi Ahmad.
> >
> > If we introduced the following small helpers we could simplify the call
> > sites
> > to just use at91_get_ddram_size(AT91SAM9G45_BASE_DDRSDRC0) and similar.
> >
> > static inline u32 at91_get_ddram_size(void __force __iomem * addr)
> > {
> > return __at91_get_ddram_size(IOMEM(addr), true);
> > }
> >
> > static inline u32 at91sam9g45_get_ddram_size(void __force __iomem * addr)
> > {
> > return __at91_get_ddram_size(IOMEM(addr), false);
> > }
>
> That would still require the pointer cast to pacify the -Wint-conversion.
> And (void*)ADDR looks only marginally better IOMEM(ADDR) IMO
> at the cost of having sparse miss passing memory pointers
> (__force __iomem would be equivalent to just __attribute__((noderef)),
> wouldn't it?)
>
> My preference would've been that AT91SAM9G45_BASE_DDRSDRC0 already expands
> to a void __iomem *.
The main idea was to get rid of the bool argument and use more explicit function
names. Moving IOMEM() was the added extra.
> We could do that in a follow-up patch.
Agreed, maybe later if we do it.
The original patch in this mail was fine.
You can add my:
Reviewed-by: Sam Ravnborg
If you respin the series.
Sam
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v3 05/15] ARM: at91: watchdog: implement at91_wdt_disable
On Tue, Apr 02, 2019 at 08:39:56PM +0200, Ahmad Fatoum wrote:
> Hello Sam,
>
> On 2/4/19 19:38, Sam Ravnborg wrote:
> > Hi Ahmad.
> >
> > On Mon, Apr 01, 2019 at 12:18:13PM +0200, Ahmad Fatoum wrote:
> >> Low level init code might want to disable the watchdog in PBL.
> >> Provide a helper to do so.
> >>
> >> Signed-off-by: Ahmad Fatoum
> >> ---
> >> arch/arm/mach-at91/include/mach/at91_wdt.h | 16
> >> 1 file changed, 16 insertions(+)
> >>
> >> diff --git a/arch/arm/mach-at91/include/mach/at91_wdt.h
> >> b/arch/arm/mach-at91/include/mach/at91_wdt.h
> >> index 36d37b9d2d64..d295d35d1b5c 100644
> >> --- a/arch/arm/mach-at91/include/mach/at91_wdt.h
> >> +++ b/arch/arm/mach-at91/include/mach/at91_wdt.h
> >> @@ -35,4 +35,20 @@
> >> #define AT91_WDT_WDUNF (1 << 0)/*
> >> Watchdog Underflow */
> >> #define AT91_WDT_WDERR (1 << 1)/*
> >> Watchdog Error */
> >>
> >> +#ifndef __ASSEMBLY__
> >> +// SPDX-License-Identifier: BSD-1-Clause
> >> +/*
> >> + * Copyright (c) 2006, Atmel Corporation
> >> + */
> >> +
> >> +#include
> >> +
> >> +static inline void at91_wdt_disable(void __iomem *wdt_base)
> >
> > As we are copying a lot of code from at91bootstrap could
> > we then use the same name for this function:
> > driver/at91_wdt.c:void at91_disable_wdt(void)
>
> ${arch}_${subsystem}_${function} seems to be the usual naming scheme
> for "globals". Why differ? I don't see the utility of copying
> the at91bootstrap name.
Yeah, keep it in line with barebox is better.
It was more a something I stumbled upon comment.
Here you can also add my:
Reviewed-by: Sam Ravnborg
Sam
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v3 06/15] ARM: at91: import lowlevel clock initialization from at91bootstrap
Hello Sam, On 2/4/19 19:42, Sam Ravnborg wrote: > Hi Ahmad. > > On Mon, Apr 01, 2019 at 12:18:14PM +0200, Ahmad Fatoum wrote: >> For use by future at91 first stage bootloaders, this commit imports >> https://github.com/linux4sam/at91bootstrap/blob/v3.8.12/driver/pmc.c >> >> Signed-off-by: Ahmad Fatoum >> --- >> arch/arm/mach-at91/Makefile | 1 + >> arch/arm/mach-at91/at91_pmc_ll.c | 184 ++ >> arch/arm/mach-at91/include/mach/at91_pmc.h| 24 ++- >> arch/arm/mach-at91/include/mach/at91_pmc_ll.h | 78 >> 4 files changed, 284 insertions(+), 3 deletions(-) >> create mode 100644 arch/arm/mach-at91/at91_pmc_ll.c >> create mode 100644 arch/arm/mach-at91/include/mach/at91_pmc_ll.h >> >> diff --git a/arch/arm/mach-at91/Makefile b/arch/arm/mach-at91/Makefile >> index d81683ac121a..91b06c085107 100644 >> --- a/arch/arm/mach-at91/Makefile >> +++ b/arch/arm/mach-at91/Makefile >> @@ -1,4 +1,5 @@ >> obj-y += setup.o >> +pbl-y += at91_pmc_ll.o > > Will this always make my pbl image larger or do we have some linker > magic that throws away the unused code? My understanding is that barebox instructs GCC to place each out-of-line function into a separate section (with -ffunction-sections) and then the linker prunes all unreferenced sections. > If the pbl image become larger then we need to add this in a conditional > manner > as we cannot allow this for all at91 variants. I used to have conditional function definitions in a prior version (albeit for the SDRAM variants initialization routines) and Sascha commented that the #ifdefery can be removed as the linker takes care of it. > > Sam > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v3 05/15] ARM: at91: watchdog: implement at91_wdt_disable
Hello Sam,
On 2/4/19 19:38, Sam Ravnborg wrote:
> Hi Ahmad.
>
> On Mon, Apr 01, 2019 at 12:18:13PM +0200, Ahmad Fatoum wrote:
>> Low level init code might want to disable the watchdog in PBL.
>> Provide a helper to do so.
>>
>> Signed-off-by: Ahmad Fatoum
>> ---
>> arch/arm/mach-at91/include/mach/at91_wdt.h | 16
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/arch/arm/mach-at91/include/mach/at91_wdt.h
>> b/arch/arm/mach-at91/include/mach/at91_wdt.h
>> index 36d37b9d2d64..d295d35d1b5c 100644
>> --- a/arch/arm/mach-at91/include/mach/at91_wdt.h
>> +++ b/arch/arm/mach-at91/include/mach/at91_wdt.h
>> @@ -35,4 +35,20 @@
>> #define AT91_WDT_WDUNF (1 << 0)/*
>> Watchdog Underflow */
>> #define AT91_WDT_WDERR (1 << 1)/*
>> Watchdog Error */
>>
>> +#ifndef __ASSEMBLY__
>> +// SPDX-License-Identifier: BSD-1-Clause
>> +/*
>> + * Copyright (c) 2006, Atmel Corporation
>> + */
>> +
>> +#include
>> +
>> +static inline void at91_wdt_disable(void __iomem *wdt_base)
>
> As we are copying a lot of code from at91bootstrap could
> we then use the same name for this function:
> driver/at91_wdt.c:void at91_disable_wdt(void)
${arch}_${subsystem}_${function} seems to be the usual naming scheme
for "globals". Why differ? I don't see the utility of copying
the at91bootstrap name.
>
> In at91bootstrap the address is a build time constant,
> but passing it as an value is better.
> So that part I do not ask you to consider to copy.
>
> Sam
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v3 04/15] ARM: at91: replace at91sam9_ddrsdr.h with at91bootstrap's at91_ddrsdrc.h
Hi Sam,
On 2/4/19 19:25, Sam Ravnborg wrote:
> Hi Ahmad.
>
> On Mon, Apr 01, 2019 at 12:18:12PM +0200, Ahmad Fatoum wrote:
>> Instead of adding missing definitions to the existing at91sam9_ddrsdr.h
>> and adapting the incoming DDRAM initialization code from at91bootstrap,
>> just replace the lightly used existing header with:
>> https://github.com/linux4sam/at91bootstrap/blob/v3.8.12/include/arch/at91_ddrsdrc.h
>>
>> For easier comprehension, the replacement is done in three steps:
>> This last step copies the memory size querying functions from
>> at91sam9_ddrsdr.h
>> to at91_ddrsdrc.h, then deletes it and fixes all references.
>>
>> Signed-off-by: Ahmad Fatoum
>> ---
>
>
>> diff --git a/arch/arm/mach-at91/include/mach/at91_ddrsdrc.h
>> b/arch/arm/mach-at91/include/mach/at91_ddrsdrc.h
>> index 57d0d8f489c4..7e68e7dd63eb 100644
>> --- a/arch/arm/mach-at91/include/mach/at91_ddrsdrc.h
>> +++ b/arch/arm/mach-at91/include/mach/at91_ddrsdrc.h
>> @@ -285,4 +285,48 @@
>> #define AT91C_DDRC2_WPVS(0x1UL << 0)
>> #define AT91C_DDRC2_WPSRC (0xUL << 8)
>>
>> +#ifndef __ASSEMBLY__
>> +#include
>> +#include
>> +
>> +static inline u32 at91_get_ddram_size(void * __iomem base, bool is_nb)
>> +{
>
> I should have brought this up in the previous patch-set, but it continue to
> itch me.
> So you get the feedback here
>
> We have at91_get_ddram_size(IOMEM(xxx), {true or false})
>
> If we introduced the following small helpers we could simplify the call sites
> to just use at91_get_ddram_size(AT91SAM9G45_BASE_DDRSDRC0) and similar.
>
> static inline u32 at91_get_ddram_size(void __force __iomem * addr)
> {
> return __at91_get_ddram_size(IOMEM(addr), true);
> }
>
> static inline u32 at91sam9g45_get_ddram_size(void __force __iomem * addr)
> {
> return __at91_get_ddram_size(IOMEM(addr), false);
> }
That would still require the pointer cast to pacify the -Wint-conversion.
And (void*)ADDR looks only marginally better IOMEM(ADDR) IMO
at the cost of having sparse miss passing memory pointers
(__force __iomem would be equivalent to just __attribute__((noderef)), wouldn't
it?)
My preference would've been that AT91SAM9G45_BASE_DDRSDRC0 already expands
to a void __iomem *. We could do that in a follow-up patch.
>
> But then for the few call sites it is not really worth it anyway.
> Anyway, just a small itch. Just ignore it.
>
> Sam
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v3 00/15] ARM: at91: microchip-kz9477-evb: support first stage boot
Hi Ahmad. It is super that you are working on this and I hope we can have bootstrap support for more at91 variants in the near future. > This is (the hopefully final and bestest) v3 of a series that pulls in > enough of at91bootstrap to make barebox usable as first stage bootloader > for the SAMA5 family of AT91 SoCs. Sorry for gettting back to this patch set only now. I have been through all patches and there was a few minor comments here and there. Let me know if this triggers any questions. Sam ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v3 15/15] doc: microchip-ksz9477-evb: add documentation
Hi Ahmad. Thanks for adding the docs. On Mon, Apr 01, 2019 at 12:18:23PM +0200, Ahmad Fatoum wrote: > Signed-off-by: Ahmad Fatoum > --- > .../boards/at91/microchip-ksz9477-evb.rst | 38 ++- > 1 file changed, 36 insertions(+), 2 deletions(-) > > diff --git a/Documentation/boards/at91/microchip-ksz9477-evb.rst > b/Documentation/boards/at91/microchip-ksz9477-evb.rst > index 4c4c4aecbfb3..2a68c2a552cd 100644 > --- a/Documentation/boards/at91/microchip-ksz9477-evb.rst > +++ b/Documentation/boards/at91/microchip-ksz9477-evb.rst > @@ -1,11 +1,45 @@ > Microchip KSZ 9477 Evaluation board > === > > -This is an evaluation board for a switch that uses the at91sam9x5 CPU. > +This is an evaluation board for the KSZ9477 switch that uses the sama5d36 > CPU. > The board uses Device Tree and supports multi image. > > -Building barebox: > +Building barebox as second stage bootloader: > > .. code-block:: sh > >make ARCH=arm microchip_ksz9477_evb_defconfig > + > +There are also a separate defconfig for operating barebox as first stage > +bootloader originating from SD Card. > +This configuration doesn't yet support device-tree use as the NVM bootloader > +(SoC ROM code) requires the first stage bootloader to fit into 64K. > + > +Generally, the first stage may comes from any of the following boot > +sources (in that order): > + > +* SPI0 CS0 Flash > +* SD Card > +* NAND Flash > +* SPI0 CS1 Flash > +* I2C EEPROM > + > +After being loaded into SRAM by the NVM bootloader, the first stage does low > +level clock initialization, configuration of the DDRAM controller and > +bootstraps the second stage boot loader. > + > +SD Card Bootstrap > +- > + > +For boot from SD card, barebox additionally needs to be configured as > +first stage bootloader: > + > +.. code-block:: sh > + > + make ARCH=arm microchip_ksz9477_evb_bootstrap_mmc_defconfig > + > +The resulting barebox image must be renamed to ``BOOT.BIN`` When I am away from barebox development for some weeks I always have troubles what it the right file to use. I suggest to use specific filename rather than the vauge "resulting barebox image". Sam ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v3 10/15] images: at91: differentiate between first and second stage images
Hi Ahmad. On Mon, Apr 01, 2019 at 12:18:18PM +0200, Ahmad Fatoum wrote: > Incoming microchip-ksz9477-evb first stage will add one more entry point > for the first stage. As there is a little reason to use the same piggy > data for both images (BOOT.BIN, the first stage, is limited to 64K), have > CONFIG_AT91_LOAD_BAREBOX_SRAM decide which stage should be built. > > Signed-off-by: Ahmad Fatoum > --- > arch/arm/mach-at91/Kconfig | 2 +- > images/Makefile.at91 | 12 +--- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig > index 8e1bf0629ab7..60f427d7d483 100644 > --- a/arch/arm/mach-at91/Kconfig > +++ b/arch/arm/mach-at91/Kconfig > @@ -615,7 +615,7 @@ config AT91_BOOTSTRAP > select BOOTSTRAP > > config AT91_LOAD_BAREBOX_SRAM > - bool "at91 load barebox in sram" > + bool "at91 barebox image will be loaded into SRAM" > depends on SHELL_NONE || HAVE_AT91_LOAD_BAREBOX_SRAM > default y if SHELL_NONE > > diff --git a/images/Makefile.at91 b/images/Makefile.at91 > index acdb591d2452..3f1dd57f6c58 100644 > --- a/images/Makefile.at91 > +++ b/images/Makefile.at91 > @@ -4,12 +4,18 @@ > > pblb-$(CONFIG_MACH_AT91SAM9X5EK) += start_at91sam9x5ek > FILE_barebox-at91sam9x5ek.img = start_at91sam9x5ek.pblb > -image-$(CONFIG_MACH_AT91SAM9X5EK) += barebox-at91sam9x5ek.img > +at91-barebox-$(CONFIG_MACH_AT91SAM9X5EK) += barebox-at91sam9x5ek.img > > pblb-$(CONFIG_MACH_AT91SAM9263EK) += start_at91sam9263ek > FILE_barebox-at91sam9263ek.img = start_at91sam9263ek.pblb > -image-$(CONFIG_MACH_AT91SAM9263EK) += barebox-at91sam9263ek.img > +at91-barebox-$(CONFIG_MACH_AT91SAM9263EK) += barebox-at91sam9263ek.img > > pblb-$(CONFIG_MACH_MICROCHIP_KSZ9477_EVB) += start_sama5d3_xplained_ung8071 > FILE_barebox-microchip-ksz9477-evb.img = start_sama5d3_xplained_ung8071.pblb > -image-$(CONFIG_MACH_MICROCHIP_KSZ9477_EVB) += > barebox-microchip-ksz9477-evb.img > +at91-barebox-$(CONFIG_MACH_MICROCHIP_KSZ9477_EVB) += > barebox-microchip-ksz9477-evb.img > + > +ifdef CONFIG_AT91_LOAD_BAREBOX_SRAM > +image-y += $(at91-boot-bin-y) > +else > +image-y += $(at91-barebox-y) > +endif I cannot follow what you do in the line "image-y += $(at91-boot-bin-y)". Could you try to elaborate. Sam ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v3 05/15] ARM: at91: watchdog: implement at91_wdt_disable
Hi Ahmad. On Mon, Apr 01, 2019 at 12:18:13PM +0200, Ahmad Fatoum wrote: > Low level init code might want to disable the watchdog in PBL. > Provide a helper to do so. > > Signed-off-by: Ahmad Fatoum > --- > arch/arm/mach-at91/include/mach/at91_wdt.h | 16 > 1 file changed, 16 insertions(+) > > diff --git a/arch/arm/mach-at91/include/mach/at91_wdt.h > b/arch/arm/mach-at91/include/mach/at91_wdt.h > index 36d37b9d2d64..d295d35d1b5c 100644 > --- a/arch/arm/mach-at91/include/mach/at91_wdt.h > +++ b/arch/arm/mach-at91/include/mach/at91_wdt.h > @@ -35,4 +35,20 @@ > #define AT91_WDT_WDUNF (1 << 0)/* > Watchdog Underflow */ > #define AT91_WDT_WDERR (1 << 1)/* > Watchdog Error */ > > +#ifndef __ASSEMBLY__ > +// SPDX-License-Identifier: BSD-1-Clause > +/* > + * Copyright (c) 2006, Atmel Corporation > + */ > + > +#include > + > +static inline void at91_wdt_disable(void __iomem *wdt_base) As we are copying a lot of code from at91bootstrap could we then use the same name for this function: driver/at91_wdt.c:void at91_disable_wdt(void) In at91bootstrap the address is a build time constant, but passing it as an value is better. So that part I do not ask you to consider to copy. Sam ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH] clk: at91: fix compilation errors in sama5d2.c
Hi Ahmad. Thanks for looking into this. On Mon, Apr 01, 2019 at 12:32:57PM +0200, Ahmad Fatoum wrote: > sama5d2 was added along with the update to the upstream > device tree bindings, but wasn't wired in anywhere. > > To prepare for usage in future sama5d2 support, fix > compilation errors related to absence of locks and > unavailability of audio/i2s clock/pll handling. > > Signed-off-by: Ahmad Fatoum Another approach could have been to stub out the non-existing functions to make diff between the kernel and the barebox variant smaller. Did you consider that approach? The whole section in CONFIG_HAVE_AT91_AUDIO_PLL in dt-compat.c is also something we could drop btw. if we go the "drop all audio" stuff from clk. Also prototypes in pmc.h The patch itself looked fine. But I think that from a maintenance point of view should stub us out of it. Let me know what you think. Sam ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v2 11/10] clk: at91: fix warning about missing const-safety
On Mon, Apr 01, 2019 at 11:34:27PM +0200, Ahmad Fatoum wrote: > compiling clk-generated.c results in: > > warning: passing argument 1 of 'memcpy' discards 'const' qualifier from > pointer target type [-Wdiscarded-qualifiers] > memcpy(gck->hw.parent_names, parent_names, parents_array_size); > ~~~^ > > Avoid this by replacing the xzalloc+memcpy pair with xmemdup. > Zero-initialization of the buffer isn't necessary, because > memcpy spans the whole buffer. > > Suggested-by: Sam Ravnborg > Signed-off-by: Ahmad Fatoum Reviewed-by: Sam Ravnborg ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: Issues with SPI on iMX7 Sabre Board
Hello Kai, On 4/2/19 3:22 PM, Kai Che wrote: > Hi all, > > I'm trying to communicate with SPI in barebox on the MC iMX7 Sabre Board. > > I added the options in my barebox config: > > CONFIG_SPI=y > CONFIG_CMD_SPI=y How about CONFIG_DRIVER_SPI_IMX? > > But there is no SPI bus when I try to communicate with: > > barebox@Freescale i.MX7 SabreSD Board:/ spi 0x1 > spi bus 0 not found > spi: No such device > > Could someone please tell me if a SPI driver exists for the iMX7 Sabre Board? > > Thanks a lot, > > Kai > ___ > barebox mailing list > barebox@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/barebox > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Issues with SPI on iMX7 Sabre Board
Hi all, I'm trying to communicate with SPI in barebox on the MC iMX7 Sabre Board. I added the options in my barebox config: CONFIG_SPI=y CONFIG_CMD_SPI=y But there is no SPI bus when I try to communicate with: barebox@Freescale i.MX7 SabreSD Board:/ spi 0x1 spi bus 0 not found spi: No such device Could someone please tell me if a SPI driver exists for the iMX7 Sabre Board? Thanks a lot, Kai ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
