[PATCH v2] of_firmware: fix lookup of fpga manager

[Michael Tretter]

The of_parse_phandle() looks for the phandle in the root device tree,
but as np is resolved for the target device tree, the phandle refers to
the target device tree and may return a wrong node in the root tree.

Therefore, we must ensure that we look for the manager-node in the
target device tree, which is the root node of np, and look for the
manager with that name.

firmwaremgr_find_by_node already uses the name for the lookup.

Signed-off-by: Michael Tretter 
---
Changelog:

v1 -> v2:

- Use of_parse_phandle_from instead of open coding the phandle lookup
---
 drivers/of/of_firmware.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_firmware.c b/drivers/of/of_firmware.c
index 80feb3b90dba..c1b69aac045c 100644
--- a/drivers/of/of_firmware.c
+++ b/drivers/of/of_firmware.c
@@ -11,7 +11,8 @@ static struct firmware_mgr *of_node_get_mgr(struct 
device_node *np)
struct device_node *mgr_node;
 
do {
-   mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
+   mgr_node = of_parse_phandle_from(np, of_find_root_node(np),
+"fpga-mgr", 0);
if (mgr_node)
return firmwaremgr_find_by_node(mgr_node);
} while ((np = of_get_parent(np)) != NULL);
-- 
2.39.2

Re: [PATCH] of_firmware: fix lookup of fpga manager

[Michael Tretter]

On Wed, 26 Jul 2023 14:15:58 +0200, Sascha Hauer wrote:
> On Tue, Jul 11, 2023 at 12:19:07PM +0200, Michael Tretter wrote:
> > The of_parse_phandle() looks for the phandle in the root device tree,
> > but as np is resolved for the target device tree, the phandle refers to
> > the target device tree and may return a wrong node in the root tree.
> > 
> > Therefore, we must ensure that we look for the manager-node in the
> > target device tree and look for the manager with that name.
> > 
> > firmwaremgr_find_by_node already uses the name for the lookup.
> > 
> > Signed-off-by: Michael Tretter 
> > ---
> >  drivers/of/of_firmware.c | 30 --
> >  1 file changed, 24 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/of/of_firmware.c b/drivers/of/of_firmware.c
> > index 80feb3b90dba..0429875a26c4 100644
> > --- a/drivers/of/of_firmware.c
> > +++ b/drivers/of/of_firmware.c
> > @@ -9,14 +9,32 @@
> >  static struct firmware_mgr *of_node_get_mgr(struct device_node *np)
> >  {
> > struct device_node *mgr_node;
> > +   const __be32 *property;
> > +   phandle phandle;
> > +   int size;
> >  
> > +   /* Find fpga-mgr phandle, which may be set in a parent fpga-region. */
> > do {
> > -   mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
> > -   if (mgr_node)
> > -   return firmwaremgr_find_by_node(mgr_node);
> > -   } while ((np = of_get_parent(np)) != NULL);
> 
> Not sure if I get this completely. Can't you just replace
> of_parse_phandle() with of_parse_phandle_from() with the correct
> root_node and be done with it?

I don't remember, why I implemented it as it is. Using of_parse_phandle_from()
should work. I will test it and send a v2.

Michael

> 
> Sascha
> 
> > -
> > -   return NULL;
> > +   property = of_get_property(np, "fpga-mgr", &size);
> > +   if (property && size == sizeof(*property))
> > +   break;
> > +   } while ((np = of_get_parent(np)) != NULL &&
> > +of_device_is_compatible(np, "fpga-region"));
> > +   if (!property)
> > +   return NULL;
> > +   phandle = be32_to_cpup(property);
> > +
> > +   /*
> > +* The phandle in the np will refer to a device node in the target
> > +* tree, which may differ from the phandle in the root tree. We need
> > +* the name of the target node, as firmwaremgr_find_by_node performs
> > +* the lookup by the node name.
> > +*/
> > +   mgr_node = of_find_node_by_phandle_from(phandle, of_find_root_node(np));
> > +   if (!mgr_node)
> > +   return NULL;
> > +
> > +   return firmwaremgr_find_by_node(mgr_node);
> >  }
> >  
> >  struct fw_load_entry {
> > -- 
> > 2.39.2
> > 
> > 
> > 

Re: [PATCH] common: bootm: support locating kernel in FIT image in zero page

[Michael Tretter]

On Tue, 18 Jul 2023 12:02:48 +0200, Ahmad Fatoum wrote:
> Since commit 7e2f6a1ffd64 ("uimage: disable zero page when loading to
> SDRAM at address 0x0"), we allow locating images loaded from files at
> address zero if that's within the SDRAM. This only applied to images
> loaded with file_to_sdram() and images that were already in RAM were
> still not allowed to overlap the zero page.
> 
> Fix this by doing in bootm_load_os() as was done in file_to_sdram(),
> namely, disabling zero page trapping for the duration of the memcpy.
> We need no further zero page handling afterwards, because kernel is
> booted after paging is disabled.
> 
> Cc: Michael Tretter 
> Reported-by: Steffen Trumtrar 
> Signed-off-by: Ahmad Fatoum 

Reviewed-by: Michael Tretter 

> ---
>  common/bootm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/common/bootm.c b/common/bootm.c
> index 791d6b8fbbf1..4845c40958ae 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -13,6 +13,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  static LIST_HEAD(handler_list);
>  
> @@ -119,7 +120,7 @@ int bootm_load_os(struct image_data *data, unsigned long 
> load_address)
>   (unsigned long long)load_address + kernel_size 
> - 1);
>   return -ENOMEM;
>   }
> - memcpy((void *)load_address, kernel, kernel_size);
> + zero_page_memcpy((void *)load_address, kernel, kernel_size);
>   return 0;
>   }
>  
> -- 
> 2.39.2
> 
> 

[PATCH] of_firmware: fix lookup of fpga manager

[Michael Tretter]

The of_parse_phandle() looks for the phandle in the root device tree,
but as np is resolved for the target device tree, the phandle refers to
the target device tree and may return a wrong node in the root tree.

Therefore, we must ensure that we look for the manager-node in the
target device tree and look for the manager with that name.

firmwaremgr_find_by_node already uses the name for the lookup.

Signed-off-by: Michael Tretter 
---
 drivers/of/of_firmware.c | 30 --
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/of/of_firmware.c b/drivers/of/of_firmware.c
index 80feb3b90dba..0429875a26c4 100644
--- a/drivers/of/of_firmware.c
+++ b/drivers/of/of_firmware.c
@@ -9,14 +9,32 @@
 static struct firmware_mgr *of_node_get_mgr(struct device_node *np)
 {
struct device_node *mgr_node;
+   const __be32 *property;
+   phandle phandle;
+   int size;
 
+   /* Find fpga-mgr phandle, which may be set in a parent fpga-region. */
do {
-   mgr_node = of_parse_phandle(np, "fpga-mgr", 0);
-   if (mgr_node)
-   return firmwaremgr_find_by_node(mgr_node);
-   } while ((np = of_get_parent(np)) != NULL);
-
-   return NULL;
+   property = of_get_property(np, "fpga-mgr", &size);
+   if (property && size == sizeof(*property))
+   break;
+   } while ((np = of_get_parent(np)) != NULL &&
+of_device_is_compatible(np, "fpga-region"));
+   if (!property)
+   return NULL;
+   phandle = be32_to_cpup(property);
+
+   /*
+* The phandle in the np will refer to a device node in the target
+* tree, which may differ from the phandle in the root tree. We need
+* the name of the target node, as firmwaremgr_find_by_node performs
+* the lookup by the node name.
+*/
+   mgr_node = of_find_node_by_phandle_from(phandle, of_find_root_node(np));
+   if (!mgr_node)
+   return NULL;
+
+   return firmwaremgr_find_by_node(mgr_node);
 }
 
 struct fw_load_entry {
-- 
2.39.2

Re: [PATCH] firmware: zynqmp: fix loading for PMU FWv1.1

[Michael Tretter]

On Fri, 23 Jun 2023 13:42:56 +0200, Steffen Trumtrar wrote:
> FPGA programming on ZynqMP is done by the PMU. The SoC just hands a
> file to the PMU and lets it load this file to the FPGA.
> 
> The bitstream that is sent to the FPGA consists of two headers, the
> optional struct bs_header and a binary-header. See comment in driver:
> 
>  (...)
>  * Bitstream can be provided with an optinal header (`struct bs_header`).
>  * The true bitstream starts with the binary-header composed of 21 words:
>  (...)
> 
> The PMUFW v1.0 had a feature for taking the bitstream only, skipping
> the header, or including the binary-header and only skipping the
> optional header. This is controlled via the BIT_ONLY_BIN flag. When
> the flag is set, the complete header should be skipped.
> 
> The current implementation sets the flag *and* sends the binary-header
> to the PMU:
> 
>   header_length = get_header_length(mgr->buf, mgr->size);
>   (...)
>   body = (u32 *)&mgr->buf[header_length];
> 
> get_header_length searches for the first DUMMY_WORD, i.e. the beginning
> of the binary header. Then we set body to this offset, skipping the
> optional header.
> 
> The flag is always set to ZYNQMP_FPGA_BIT_ONLY_BIN, which is then obviously
> incorrect. It seems like the PMUFW v1.0 was capable of still handling this
> although the flag was set.
> 
> The newer PMUFW v1.1 doesn't support this and expects the flag to be
> unset and getting handed binary-header+bitstream. This is the same as
> the linux kernel handles bitstream loading on the ZynqMP. There the flag
> is also always unset.
> 
> As this wasn't correct in the first place (although it most likely still
> worked depending on PMUFW and its configuration), we won't break anything
> when we just set the flag to zero as the PMUFW v1.1 expects it.
> 
> TLDR: set fpga_load flags to zero to fix firmwareloading with newer
> PMUFW versions.
> 
> Signed-off-by: Steffen Trumtrar 

Reviewed-by: Michael Tretter 

> ---
>  drivers/firmware/zynqmp-fpga.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
> index 9c160e9098..1a9a5c1b2c 100644
> --- a/drivers/firmware/zynqmp-fpga.c
> +++ b/drivers/firmware/zynqmp-fpga.c
> @@ -206,7 +206,7 @@ static int fpgamgr_program_finish(struct firmware_handler 
> *fh)
>   enum xilinx_byte_order byte_order;
>   dma_addr_t addr;
>   int status = 0;
> - u8 flags = ZYNQMP_FPGA_BIT_ONLY_BIN;
> + u8 flags = 0;
>  
>   if (!mgr->buf) {
>   status = -ENOBUFS;
> -- 
> 2.41.0
> 
> 

Re: [PATCH 1/2] of: overlay: improve error handling in of_overlay_apply_tree

[Michael Tretter]

On Wed, 21 Sep 2022 08:55:12 +0200, Michael Riesch wrote:
> On 9/5/22 12:07, Michael Riesch wrote:
> > Propagate any error from of_overlay_apply_symbols and let the user
> > know if the provided overlay is not applicable.
> > 
> > Signed-off-by: Michael Riesch 
> > ---
> >  drivers/of/overlay.c | 14 +-
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index 20a43f5170..20686db511 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -115,8 +115,8 @@ static char *of_overlay_fix_path(struct device_node 
> > *root,
> > return basprintf("%s%s", target->full_name, path_tail);
> >  }
> >  
> > -static void of_overlay_apply_symbols(struct device_node *root,
> > -struct device_node *overlay)
> > +static int of_overlay_apply_symbols(struct device_node *root,
> > +   struct device_node *overlay)
> >  {
> > const char *old_path;
> > char *new_path;
> > @@ -129,12 +129,12 @@ static void of_overlay_apply_symbols(struct 
> > device_node *root,
> >  
> > if (!overlay_symbols) {
> > pr_debug("overlay doesn't have a __symbols__ node\n");
> > -   return;
> > +   return -EINVAL;
> 
> Come to think of it, do all overlays need to provide a __symbols__ node?
> If not, this check is overly strict.

Overlays don't need a __symbols__ node. It would be only required, if overlays
are stacked and the second overlay refers to nodes of the first overlay by
labels. Having no __symbols__ in the overlay is a success path and the message
is just a debug message.

> 
> > }
> >  
> > if (!root_symbols) {
> > pr_info("root doesn't have a __symbols__ node\n");
> > -   return;
> > +   return -EINVAL;
> 
> Ditto for the root.

I'm not sure what should happen, if the root does not have __symbols__.
Barebox wouldn't be able to copy the __symbols__ of the overlay, but this
still wouldn't be a problem unless overlays are stacked. In the stacking case,
only applying the second overlay should fail.

Maybe, we should add a new __symbols__ node, if the root doesn't have a
__symbols__ node?

> 
> > }
> >  
> > list_for_each_entry(prop, &overlay_symbols->properties, list) {
> > @@ -148,6 +148,8 @@ static void of_overlay_apply_symbols(struct device_node 
> > *root,
> >  prop->name, new_path);
> > of_property_write_string(root_symbols, prop->name, new_path);
> > }
> > +
> > +   return 0;
> >  }
> >  
> >  static int of_overlay_apply_fragment(struct device_node *root,
> > @@ -190,7 +192,9 @@ int of_overlay_apply_tree(struct device_node *root,
> > goto out_err;
> >  
> > /* Copy symbols from resolved overlay to base device tree */
> > -   of_overlay_apply_symbols(root, resolved);
> > +   err = of_overlay_apply_symbols(root, resolved);
> > +   if (err)
> > +   goto out_err;
> 
> If both checks need to be relaxed, the complete patch should be reverted
> I guess :-/

What did you do to run into this error? What was your expectation?

Michael

Re: [PATCH] firmware: zynqmp-fpga: do not load PL with ONLY_BIN flag unless necessary

[Michael Tretter]

On Sun, 01 May 2022 20:26:07 +0200, Matthias Fend wrote:
> Since pmu-fw release 2018.3, the ZYNQMP_FPGA_BIT_ONLY_BIN flag is no
> longer used. This wasn't a problem for a while, but in newer versions a
> validation sequence will fail if this flag is set. This means that the PL
> can no longer be loaded.
> 
> Do not set the ZYNQMP_FPGA_BIT_ONLY_BIN flag unless absolutely necessary
> to avoid this problem.

Thanks for the patch.

> 
> Signed-off-by: Matthias Fend 
> ---
>  drivers/firmware/zynqmp-fpga.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
> index 63d7398fd..db34ac2be 100644
> --- a/drivers/firmware/zynqmp-fpga.c
> +++ b/drivers/firmware/zynqmp-fpga.c
> @@ -261,9 +261,10 @@ static int fpgamgr_program_finish(struct 
> firmware_handler *fh)
>   goto err_free_dma;
>   }
>  
> - if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED)
> + if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) {
> + flags &= ~ZYNQMP_FPGA_BIT_ONLY_BIN;

I would prefer if the flag is initialized as unset and only set, if the
bitstream does not have headers. This would be a lot clearer, than resetting
the flag based on a version dependent feature flag with a different name.

If I understand correctly, the newer versions of the PMUFW don't support
loading a bitstream without headers at all. Maybe we should have a feature
flag for bitstreams without headers, but maybe it is enough to just let the
PMUFW reject the bitstream in these cases.

Michael

>   buf_size = body_length;
> - else
> + } else
>   buf_size = addr + body_length;
>  
>   status = mgr->eemi_ops->fpga_load((u64)addr, buf_size, flags);
> -- 
> 2.25.1
> 
> 
> ___
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.   | Michael Tretter |
Steuerwalder Str. 21   | https://www.pengutronix.de/ |
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

[PATCH] serial: cadence: add ZynqMP compatibles

[Michael Tretter]

Commit 3f2f5980d517 ("dts: update to v5.16-rc1") changes the compatible
of the ZynqMP uarts to "xlnx,zynqmp-uart" and drops the "xlnx,xuartps"
compatible.

The driver worked just fine before and the difference between the r1p8
and r1p12 compatibles is the use of the RX byte status register which is
only used with interrupts.

Add the "xlnx,zynqmp-uart" and the "cdns,uart-r1p12" compatibles to the
driver.

Signed-off-by: Michael Tretter 
---
 drivers/serial/serial_cadence.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/serial/serial_cadence.c b/drivers/serial/serial_cadence.c
index 6cf18aa25ac7..84dcd1b76b6e 100644
--- a/drivers/serial/serial_cadence.c
+++ b/drivers/serial/serial_cadence.c
@@ -230,6 +230,12 @@ static __maybe_unused struct of_device_id 
cadence_serial_dt_ids[] = {
{
.compatible = "xlnx,xuartps",
.data = &cadence_r1p08_data,
+   }, {
+   .compatible = "cdns,uart-r1p12",
+   .data = &cadence_r1p08_data,
+   }, {
+   .compatible = "xlnx,zynqmp-uart",
+   .data = &cadence_r1p08_data,
}, {
/* sentinel */
}
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH] firmware: zynqmp-fpga: fix use of uninitialized addr

[Michael Tretter]

The bitstream loading API of the firmware is a bit clunky, as the driver
needs to either pass the size of the bitstream or a pointer to the size
of the bitstream.

Commit 2f29ee311f1d ("firmware: zynqmp-fpga: do not use DMA coherent
memory for bitstream") broke the loading by address, as the pointer to
the bitstream size was set using the uninitialized DMA address.

Fix it by determining the argument that is passed to the firmware after
the bitstream has been mapped and always write the size of the bitstream
at the end of the passed buffer.

Signed-off-by: Michael Tretter 
---
 drivers/firmware/zynqmp-fpga.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
index a76600d4c96d..63d7398fd4e8 100644
--- a/drivers/firmware/zynqmp-fpga.c
+++ b/drivers/firmware/zynqmp-fpga.c
@@ -252,13 +252,7 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
   body_length / sizeof(u32));
else
memcpy((u32 *)buf_aligned, body, body_length);
-
-   if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) {
-   buf_size = body_length;
-   } else {
-   buf_aligned[body_length / sizeof(*buf_aligned)] = body_length;
-   buf_size = addr + body_length;
-   }
+   buf_aligned[body_length / sizeof(*buf_aligned)] = body_length;
 
addr = dma_map_single(&mgr->dev, buf_aligned,
  body_length + sizeof(buf_size), DMA_TO_DEVICE);
@@ -267,6 +261,11 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
goto err_free_dma;
}
 
+   if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED)
+   buf_size = body_length;
+   else
+   buf_size = addr + body_length;
+
status = mgr->eemi_ops->fpga_load((u64)addr, buf_size, flags);
dma_unmap_single(&mgr->dev, addr, body_length + sizeof(buf_size),
 DMA_TO_DEVICE);
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 4/8] video: ssd1307fb: pass par instead of i2c client to write

[Michael Tretter]

By pushing the dependency to i2c down into the write function, the
remaining driver is less dependent on i2c. This allows to delay the
decision to use i2c until the actual bus write.

Signed-off-by: Michael Tretter 
---
Changelog:

v2: none
---
 drivers/video/ssd1307fb.c | 69 ---
 1 file changed, 35 insertions(+), 34 deletions(-)

diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 5d160ddf338a..61d0e083a3f7 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -93,9 +93,10 @@ static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 
len, u8 type)
return array;
 }
 
-static int ssd1307fb_write_array(struct i2c_client *client,
+static int ssd1307fb_write_array(struct ssd1307fb_par *par,
 struct ssd1307fb_array *array, u32 len)
 {
+   struct i2c_client *client = par->client;
int ret;
 
len += sizeof(struct ssd1307fb_array);
@@ -109,7 +110,7 @@ static int ssd1307fb_write_array(struct i2c_client *client,
return 0;
 }
 
-static inline int ssd1307fb_write_cmd(struct i2c_client *client, u8 cmd)
+static inline int ssd1307fb_write_cmd(struct ssd1307fb_par *par, u8 cmd)
 {
struct ssd1307fb_array *array;
int ret;
@@ -120,7 +121,7 @@ static inline int ssd1307fb_write_cmd(struct i2c_client 
*client, u8 cmd)
 
array->data[0] = cmd;
 
-   ret = ssd1307fb_write_array(client, array, 1);
+   ret = ssd1307fb_write_array(par, array, 1);
kfree(array);
 
return ret;
@@ -181,20 +182,20 @@ static void ssd1307fb_update_display(struct ssd1307fb_par 
*par)
}
}
 
-   ssd1307fb_write_array(par->client, array, par->width * par->height / 8);
+   ssd1307fb_write_array(par, array, par->width * par->height / 8);
kfree(array);
 }
 
 static void ssd1307fb_enable(struct fb_info *info)
 {
struct ssd1307fb_par *par = info->priv;
-   ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_ON);
+   ssd1307fb_write_cmd(par, SSD1307FB_DISPLAY_ON);
 }
 
 static void ssd1307fb_disable(struct fb_info *info)
 {
struct ssd1307fb_par *par = info->priv;
-   ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_OFF);
+   ssd1307fb_write_cmd(par, SSD1307FB_DISPLAY_OFF);
 }
 
 static void ssd1307fb_flush(struct fb_info *info)
@@ -215,134 +216,134 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
u32 precharge, dclk, com_invdir, compins;
 
/* Set initial contrast */
-   ret = ssd1307fb_write_cmd(par->client, SSD1307FB_CONTRAST);
+   ret = ssd1307fb_write_cmd(par, SSD1307FB_CONTRAST);
if (ret < 0)
return ret;
 
-   ret = ssd1307fb_write_cmd(par->client, par->contrast);
+   ret = ssd1307fb_write_cmd(par, par->contrast);
if (ret < 0)
return ret;
 
/* Set segment re-map */
if (par->seg_remap) {
-   ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SEG_REMAP_ON);
+   ret = ssd1307fb_write_cmd(par, SSD1307FB_SEG_REMAP_ON);
if (ret < 0)
return ret;
};
 
/* Set COM direction */
com_invdir = 0xc0 | (par->com_invdir & 0x1) << 3;
-   ret = ssd1307fb_write_cmd(par->client,  com_invdir);
+   ret = ssd1307fb_write_cmd(par,  com_invdir);
if (ret < 0)
return ret;
 
/* Set multiplex ratio value */
-   ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_MULTIPLEX_RATIO);
+   ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_MULTIPLEX_RATIO);
if (ret < 0)
return ret;
 
-   ret = ssd1307fb_write_cmd(par->client, par->height - 1);
+   ret = ssd1307fb_write_cmd(par, par->height - 1);
if (ret < 0)
return ret;
 
/* set display offset value */
-   ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_DISPLAY_OFFSET);
+   ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_DISPLAY_OFFSET);
if (ret < 0)
return ret;
 
-   ret = ssd1307fb_write_cmd(par->client, par->com_offset);
+   ret = ssd1307fb_write_cmd(par, par->com_offset);
if (ret < 0)
return ret;
 
/* Set clock frequency */
-   ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_CLOCK_FREQ);
+   ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_CLOCK_FREQ);
if (ret < 0)
return ret;
 
dclk = ((par->dclk_div - 1) & 0xf) | (par->dclk_frq & 0xf) << 4;
-   ret = ssd1307fb_write_cmd(par->client, dclk);
+   ret = ssd1307fb_write_cmd(par, dclk);
if (ret < 0)
return ret;
 
/* Set precharge period in number of ticks from the internal clock */
-   ret = ssd1307fb_write_cmd(par->client, SS

[PATCH v2 2/8] spi: add to_spi_device helper

[Michael Tretter]

Port the helper to get the spi_device from the device_d from Linux.
This macro makes SPI device drivers look a bit nicer.

Signed-off-by: Michael Tretter 
---
Changelog:

v2:

- new patch
---
 include/spi/spi.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/include/spi/spi.h b/include/spi/spi.h
index bca996d8d8f9..c5ad6bd39ff9 100644
--- a/include/spi/spi.h
+++ b/include/spi/spi.h
@@ -108,6 +108,11 @@ struct spi_device {
 */
 };
 
+static inline struct spi_device *to_spi_device(struct device_d *dev)
+{
+return dev ? container_of(dev, struct spi_device, dev) : NULL;
+}
+
 struct spi_message;
 
 /**
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 1/8] spi: stub device_spi_driver if SPI is disabled

[Michael Tretter]

This allows drivers that support multiple buses to keep the code for
registering their SPI variant even if SPI is disabled.

Signed-off-by: Michael Tretter 
---
Changelog:

v2:

- new patch
---
 include/spi/spi.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/include/spi/spi.h b/include/spi/spi.h
index c5efca1cc31c..bca996d8d8f9 100644
--- a/include/spi/spi.h
+++ b/include/spi/spi.h
@@ -515,9 +515,14 @@ static inline int spi_driver_register(struct driver_d *drv)
return register_driver(drv);
 }
 
+#ifdef CONFIG_SPI
 #define coredevice_spi_driver(drv) \
register_driver_macro(coredevice,spi,drv)
 #define device_spi_driver(drv) \
register_driver_macro(device,spi,drv)
+#else
+#define coredevice_spi_driver(drv)
+#define device_spi_driver(drv)
+#endif
 
 #endif /* __INCLUDE_SPI_H */
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 8/8] video: ssd1307fb: add spi support

[Michael Tretter]

The Solomon display drivers also support SPI in addition to the I2C.
Add SPI support to the driver that already supports I2C by implementing
the bus write function for SPI and registering an SPI driver.

While the driver needs I2C or SPI, either subsystem is optional as long
as one is enabled.

Signed-off-by: Michael Tretter 
---
Changelog:

v2:

- use new to_spi_device helper
- move warning about undocumented compatible into driver
- remove use of config macros in driver if possible
---
 drivers/video/Kconfig |  2 +-
 drivers/video/ssd1307fb.c | 62 +++
 2 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index a87e8c063899..cfbd541a956e 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -15,7 +15,7 @@ config FRAMEBUFFER_CONSOLE
 
 config DRIVER_VIDEO_FB_SSD1307
bool "Solomon SSD1307 framebuffer support"
-   depends on I2C && GPIOLIB
+   depends on (I2C || SPI) && GPIOLIB
 
 config VIDEO_VPL
depends on OFTREE
diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index d0df073b8ef2..9e141a851f51 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define SSD1307FB_DATA  0x40
 #define SSD1307FB_COMMAND   0x80
@@ -73,12 +74,14 @@ struct ssd1307fb_par {
u32 dclk_frq;
const struct ssd1307fb_deviceinfo *device_info;
struct i2c_client *client;
+   struct spi_device *spi;
u32 height;
struct fb_info *info;
u32 page_offset;
u32 prechargep1;
u32 prechargep2;
int reset;
+   int dc;
struct regulator *vbat;
u32 seg_remap;
u32 vcomh;
@@ -100,6 +103,27 @@ static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 
len, u8 type)
return array;
 }
 
+static int ssd1307fb_spi_write_array(struct ssd1307fb_par *par,
+struct ssd1307fb_array *array, u32 len)
+{
+   struct spi_device *spi = par->spi;
+   int ret;
+
+   if (array->type == SSD1307FB_COMMAND)
+   gpio_direction_output(par->dc, 0);
+   else
+   gpio_direction_output(par->dc, 1);
+
+   ret = spi_write(spi, array->data, len);
+   if (ret)
+   dev_err(&spi->dev, "Couldn't send SPI command.\n");
+
+   /* Ensure that we remain in data mode. */
+   gpio_direction_output(par->dc, 1);
+
+   return ret;
+}
+
 static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
 struct ssd1307fb_array *array, u32 len)
 {
@@ -385,6 +409,14 @@ static const struct of_device_id ssd1307fb_of_match[] = {
.compatible = "solomon,ssd1306fb-i2c",
.data = (void *)&ssd1307fb_ssd1306_deviceinfo,
},
+   {
+   /*
+* The compatible of the SPI connected ssd1306 is not
+* documented as device tree binding.
+*/
+   .compatible = "solomon,ssd1306",
+   .data = (void *)&ssd1307fb_ssd1306_deviceinfo,
+   },
{
.compatible = "solomon,ssd1309fb-i2c",
.data = (void *)&ssd1307fb_ssd1309_deviceinfo,
@@ -419,9 +451,20 @@ static int ssd1307fb_probe(struct device_d *dev)
 
par->device_info = (struct ssd1307fb_deviceinfo *)match->data;
 
-   par->client = to_i2c_client(dev);
-   i2c_set_clientdata(par->client, par);
-   par->write_array = ssd1307fb_i2c_write_array;
+   if (IS_ENABLED(CONFIG_I2C) && dev->bus == &i2c_bus) {
+   par->client = to_i2c_client(dev);
+   i2c_set_clientdata(par->client, par);
+   par->write_array = ssd1307fb_i2c_write_array;
+   }
+   if (IS_ENABLED(CONFIG_SPI) && dev->bus == &spi_bus) {
+   par->spi = to_spi_device(dev);
+   par->dc = of_get_named_gpio(node, "dc-gpios", 0);
+   if (!gpio_is_valid(par->dc)) {
+   ret = par->dc;
+   goto fb_alloc_error;
+   }
+   par->write_array = ssd1307fb_spi_write_array;
+   }
 
par->reset = of_get_named_gpio_flags(node,
 "reset-gpios", 0, &of_flags);
@@ -591,9 +634,16 @@ fb_alloc_error:
return ret;
 }
 
-static struct driver_d ssd1307fb_driver = {
-   .name = "ssd1307fb",
+static __maybe_unused struct driver_d ssd1307fb_i2c_driver = {
+   .name = "ssd1307fb-i2c",
+   .probe = ssd1307fb_probe,
+   .of_compatible = DRV_OF_COMPAT(ssd1307fb_of_match),
+};
+device_i2c_driver(ssd1307fb_i2c_driver);
+
+stati

[PATCH v2 5/8] video: ssd1307fb: don't use i2c client for logging

[Michael Tretter]

We can use the device directly and don't have to use the device that is
attached to the I2C client. This reduces the dependency on i2c.

Signed-off-by: Michael Tretter 
---
Changelog:

v2: none
---
 drivers/video/ssd1307fb.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 61d0e083a3f7..88c88e3253cf 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -401,7 +401,7 @@ static int ssd1307fb_probe(struct device_d *dev)
int i, j;
 
if (!node) {
-   dev_err(&client->dev, "No device tree data found!\n");
+   dev_err(dev, "No device tree data found!\n");
return -EINVAL;
}
 
@@ -421,22 +421,22 @@ static int ssd1307fb_probe(struct device_d *dev)
goto fb_alloc_error;
}
 
-   par->vbat = regulator_get(&client->dev, "vbat");
+   par->vbat = regulator_get(dev, "vbat");
if (IS_ERR(par->vbat)) {
-   dev_info(&client->dev, "Will not use VBAT");
+   dev_info(dev, "Will not use VBAT");
par->vbat = NULL;
}
 
ret = of_property_read_u32(node, "solomon,width", &par->width);
if (ret) {
-   dev_err(&client->dev,
+   dev_err(dev,
"Couldn't find 'solomon,width' in device tree.\n");
goto panel_init_error;
}
 
ret = of_property_read_u32(node, "solomon,height", &par->height);
if (ret) {
-   dev_err(&client->dev,
+   dev_err(dev,
"Couldn't find 'solomon,height' in device tree.\n");
goto panel_init_error;
}
@@ -444,7 +444,7 @@ static int ssd1307fb_probe(struct device_d *dev)
ret = of_property_read_u32(node, "solomon,page-offset",
   &par->page_offset);
if (ret) {
-   dev_err(&client->dev,
+   dev_err(dev,
"Couldn't find 'solomon,page_offset' in device 
tree.\n");
goto panel_init_error;
}
@@ -477,7 +477,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 
vmem = malloc(vmem_size);
if (!vmem) {
-   dev_err(&client->dev, "Couldn't allocate graphical memory.\n");
+   dev_err(dev, "Couldn't allocate graphical memory.\n");
ret = -ENOMEM;
goto fb_alloc_error;
}
@@ -508,7 +508,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 
ret = gpio_request_one(par->reset, flags, "oled-reset");
if (ret) {
-   dev_err(&client->dev,
+   dev_err(dev,
"failed to request gpio %d: %d\n",
par->reset, ret);
goto reset_oled_error;
@@ -546,7 +546,7 @@ static int ssd1307fb_probe(struct device_d *dev)
info->dev.parent = dev;
ret = register_framebuffer(info);
if (ret) {
-   dev_err(&client->dev, "Couldn't register the framebuffer\n");
+   dev_err(dev, "Couldn't register the framebuffer\n");
goto panel_init_error;
}
 
@@ -568,7 +568,7 @@ static int ssd1307fb_probe(struct device_d *dev)
ssd1307fb_write_array(par, array, par->width * par->height / 8);
kfree(array);
 
-   dev_info(&client->dev,
+   dev_info(dev,
 "ssd1307 framebuffer device registered, using %d bytes of 
video memory\n",
 vmem_size);
 
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 7/8] video: ssd1307fb: use function pointer for write

[Michael Tretter]

The function pointer is an abstraction the I2C accesses to be able to
add other bus protocols underneath the driver.

The functionality kind of reminds of regmap, but the driver does only
write data and does not actually use registers. Therefore, using regmap
with the register abstraction is not appropriate.

Signed-off-by: Michael Tretter 
---
Changelog:

v2: none
---
 drivers/video/ssd1307fb.c | 26 +-
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 1538a1b640a3..d0df073b8ef2 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -53,6 +53,16 @@ struct ssd1307fb_deviceinfo {
int need_chargepump;
 };
 
+struct ssd1307fb_array {
+   u8 type;
+   u8 data[0];
+};
+
+struct ssd1307fb_par;
+
+typedef int (*ssd1307fb_write_array)(struct ssd1307fb_par *par,
+struct ssd1307fb_array *array, u32 len);
+
 struct ssd1307fb_par {
u32 com_invdir;
u32 com_lrremap;
@@ -73,11 +83,8 @@ struct ssd1307fb_par {
u32 seg_remap;
u32 vcomh;
u32 width;
-};
 
-struct ssd1307fb_array {
-   u8 type;
-   u8 data[0];
+   ssd1307fb_write_array write_array;
 };
 
 static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 len, u8 type)
@@ -93,8 +100,8 @@ static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 
len, u8 type)
return array;
 }
 
-static int ssd1307fb_write_array(struct ssd1307fb_par *par,
-struct ssd1307fb_array *array, u32 len)
+static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
+struct ssd1307fb_array *array, u32 len)
 {
struct i2c_client *client = par->client;
int ret;
@@ -121,7 +128,7 @@ static inline int ssd1307fb_write_cmd(struct ssd1307fb_par 
*par, u8 cmd)
 
array->data[0] = cmd;
 
-   ret = ssd1307fb_write_array(par, array, 1);
+   ret = par->write_array(par, array, 1);
kfree(array);
 
return ret;
@@ -182,7 +189,7 @@ static void ssd1307fb_update_display(struct ssd1307fb_par 
*par)
}
}
 
-   ssd1307fb_write_array(par, array, par->width * par->height / 8);
+   par->write_array(par, array, par->width * par->height / 8);
kfree(array);
 }
 
@@ -414,6 +421,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 
par->client = to_i2c_client(dev);
i2c_set_clientdata(par->client, par);
+   par->write_array = ssd1307fb_i2c_write_array;
 
par->reset = of_get_named_gpio_flags(node,
 "reset-gpios", 0, &of_flags);
@@ -564,7 +572,7 @@ static int ssd1307fb_probe(struct device_d *dev)
}
}
 
-   ssd1307fb_write_array(par, array, par->width * par->height / 8);
+   par->write_array(par, array, par->width * par->height / 8);
kfree(array);
 
dev_info(dev,
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 3/8] i2c: stub device_i2c_driver if I2C is disabled

[Michael Tretter]

This allows drivers that support multiple buses to keep the code for
registering their I2C variant even if I2C is disabled.

Signed-off-by: Michael Tretter 
---
Changelog:

v2:

- new patch
---
 include/i2c/i2c.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/include/i2c/i2c.h b/include/i2c/i2c.h
index af6287602ca5..7207b1180e1d 100644
--- a/include/i2c/i2c.h
+++ b/include/i2c/i2c.h
@@ -333,9 +333,14 @@ static inline int i2c_driver_register(struct driver_d *drv)
return register_driver(drv);
 }
 
+#ifdef CONFIG_I2C
 #define coredevice_i2c_driver(drv) \
register_driver_macro(coredevice, i2c, drv)
 #define device_i2c_driver(drv) \
register_driver_macro(device, i2c, drv)
+#else
+#define coredevice_i2c_driver(drv)
+#define device_i2c_driver(drv)
+#endif
 
 #endif /* I2C_I2C_H */
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 6/8] video: ssd1307fb: move i2c setup to single place

[Michael Tretter]

By having the entire i2c dependent initialzation in a single place, it
is easier to make it optional later.

Signed-off-by: Michael Tretter 
---
Changelog:

v2: none
---
 drivers/video/ssd1307fb.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 88c88e3253cf..1538a1b640a3 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -387,7 +387,6 @@ static const struct of_device_id ssd1307fb_of_match[] = {
 
 static int ssd1307fb_probe(struct device_d *dev)
 {
-   struct i2c_client *client = to_i2c_client(dev);
struct fb_info *info;
struct device_node *node = dev->device_node;
const struct of_device_id *match =
@@ -410,10 +409,12 @@ static int ssd1307fb_probe(struct device_d *dev)
 
info->priv = par;
par->info = info;
-   par->client = client;
 
par->device_info = (struct ssd1307fb_deviceinfo *)match->data;
 
+   par->client = to_i2c_client(dev);
+   i2c_set_clientdata(par->client, par);
+
par->reset = of_get_named_gpio_flags(node,
 "reset-gpios", 0, &of_flags);
if (!gpio_is_valid(par->reset) && par->reset == -EPROBE_DEFER) {
@@ -519,8 +520,6 @@ static int ssd1307fb_probe(struct device_d *dev)
if (ret < 0)
goto reset_oled_error;
 
-   i2c_set_clientdata(client, info);
-
if (par->reset > 0) {
/* Reset the screen */
gpio_set_active(par->reset, 1);
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 0/8] video: ssd1307fb: Add SPI support

[Michael Tretter]

Hello,

The Solomon single-chip CMOS OLED/PLED driver with controller can be connected
to I2C or SPI. The driver already supports I2C. This series adds support for
SPI connected displays to the driver.

The bindings for the SPI connected display are still not documented. The
driver uses the (undocumented) solomon,ssd1306 compatible of the staging
driver in Linux, but uses properties defined for the solomon,ssd1306fb-i2c
compatible of the I2C driver. I moved the warning about the compatible from
the commit message into the driver to have it in the same place as the use of
the compatible and to allow to eventually remove it.

The driver still allows to use SPI and I2C with its own hand-rolled
abstraction, because the controller does not actually expose registers, but
simply accepts commands or data. I followed Ahmad's suggestions how to remove
the ugly #ifdefs and the driver looks a lot nicer now.

Patches 1-3 adjust the SPI and I2C frameworks to make them nicer to use for
drivers that support devices that may be connected via SPI or I2C.

Patches 4-7 refactor the driver to have fewer locations that refer to I2C to
simplify disabling the I2C support.

Patch 8 actually adds the SPI support and makes I2C optional.

Michael

---

Changelog:

v2:

- add new Patches 1-3 for the SPI and I2C frameworks
- use new SPI and I2C helpers to get rid of use of config macros
- move warning about undocumented compatible into driver

Michael Tretter (8):
  spi: stub device_spi_driver if SPI is disabled
  spi: add to_spi_device helper
  i2c: stub device_i2c_driver if I2C is disabled
  video: ssd1307fb: pass par instead of i2c client to write
  video: ssd1307fb: don't use i2c client for logging
  video: ssd1307fb: move i2c setup to single place
  video: ssd1307fb: use function pointer for write
  video: ssd1307fb: add spi support

 drivers/video/Kconfig |   2 +-
 drivers/video/ssd1307fb.c | 170 +-
 include/i2c/i2c.h |   5 ++
 include/spi/spi.h |  10 +++
 4 files changed, 130 insertions(+), 57 deletions(-)

-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/5] video: ssd1307fb: add spi support

[Michael Tretter]

On Fri, 17 Dec 2021 20:00:16 +0100, Ahmad Fatoum wrote:
> On 17.12.21 19:23, Michael Tretter wrote:
> > The Solomon display drivers also support SPI in addition to the I2C.
> > Add SPI support to the driver that already supports I2C by implementing
> > the bus write function for SPI and registering an SPI driver.
> > 
> > While the driver needs I2C or SPI, either subsystem is optional as long
> > as one is enabled.
> > 
> > WARNING: The device tree bindings for the ssd1306 with SPI are not
> > documented!
> > 
> > Signed-off-by: Michael Tretter 
> > ---
> >  drivers/video/Kconfig |  2 +-
> >  drivers/video/ssd1307fb.c | 72 +++
> >  2 files changed, 67 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > index a87e8c063899..cfbd541a956e 100644
> > --- a/drivers/video/Kconfig
> > +++ b/drivers/video/Kconfig
> > @@ -15,7 +15,7 @@ config FRAMEBUFFER_CONSOLE
> >  
> >  config DRIVER_VIDEO_FB_SSD1307
> > bool "Solomon SSD1307 framebuffer support"
> > -   depends on I2C && GPIOLIB
> > +   depends on (I2C || SPI) && GPIOLIB
> >  
> >  config VIDEO_VPL
> > depends on OFTREE
> > diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
> > index d0df073b8ef2..2939d4348405 100644
> > --- a/drivers/video/ssd1307fb.c
> > +++ b/drivers/video/ssd1307fb.c
> > @@ -23,6 +23,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #define SSD1307FB_DATA  0x40
> >  #define SSD1307FB_COMMAND   0x80
> > @@ -73,12 +74,14 @@ struct ssd1307fb_par {
> > u32 dclk_frq;
> > const struct ssd1307fb_deviceinfo *device_info;
> > struct i2c_client *client;
> > +   struct spi_device *spi;
> > u32 height;
> > struct fb_info *info;
> > u32 page_offset;
> > u32 prechargep1;
> > u32 prechargep2;
> > int reset;
> > +   int dc;
> > struct regulator *vbat;
> > u32 seg_remap;
> > u32 vcomh;
> > @@ -100,6 +103,30 @@ static struct ssd1307fb_array 
> > *ssd1307fb_alloc_array(u32 len, u8 type)
> > return array;
> >  }
> >  
> > +#if IS_ENABLED(CONFIG_SPI)
> > +static int ssd1307fb_spi_write_array(struct ssd1307fb_par *par,
> > +struct ssd1307fb_array *array, u32 len)
> 
> __maybe_unused and drop the #if?

Why doesn't this fail to link? The function uses spi_write, which is static
inline, but spi_write uses spi_sync, which is only defined in spi.c and is not
stubbed if !CONFIG_SPI. I would have expected to not be able to use spi_write
if CONFIG_SPI is disabled. Is there some dead code elimination happening
before linking?

Michael

> 
> > +{
> > +   struct spi_device *spi = par->spi;
> > +   int ret;
> > +
> > +   if (array->type == SSD1307FB_COMMAND)
> > +   gpio_direction_output(par->dc, 0);
> > +   else
> > +   gpio_direction_output(par->dc, 1);
> > +
> > +   ret = spi_write(spi, array->data, len);
> > +   if (ret)
> > +   dev_err(&spi->dev, "Couldn't send SPI command.\n");
> > +
> > +   /* Ensure that we remain in data mode. */
> > +   gpio_direction_output(par->dc, 1);
> > +
> > +   return ret;
> > +}
> > +#endif
> > +
> > +#if IS_ENABLED(CONFIG_I2C)
> 
> Ditto
> 
> >  static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
> >  struct ssd1307fb_array *array, u32 len)
> >  {
> > @@ -116,6 +143,7 @@ static int ssd1307fb_i2c_write_array(struct 
> > ssd1307fb_par *par,
> >  
> > return 0;
> >  }
> > +#endif
> >  
> >  static inline int ssd1307fb_write_cmd(struct ssd1307fb_par *par, u8 cmd)
> >  {
> > @@ -385,6 +413,10 @@ static const struct of_device_id ssd1307fb_of_match[] 
> > = {
> > .compatible = "solomon,ssd1306fb-i2c",
> > .data = (void *)&ssd1307fb_ssd1306_deviceinfo,
> > },
> > +   {
> > +   .compatible = "solomon,ssd1306",
> > +   .data = (void *)&ssd1307fb_ssd1306_deviceinfo,
> > +   },
> > {
> > .compatible = "solomon,ssd1309fb-i2c",
> > .data = (void *)&ssd1307fb_ssd1309_deviceinfo,
> > @@ -419,9 +451,24 @@ static int ssd1307fb_probe(struct device_d *dev)
>

[PATCH 1/5] video: ssd1307fb: pass par instead of i2c client to write

[Michael Tretter]

By pushing the dependency to i2c down into the write function, the
remaining driver is less dependent on i2c. This allows to delay the
decision to use i2c until the actual bus write.

Signed-off-by: Michael Tretter 
---
 drivers/video/ssd1307fb.c | 69 ---
 1 file changed, 35 insertions(+), 34 deletions(-)

diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 5d160ddf338a..61d0e083a3f7 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -93,9 +93,10 @@ static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 
len, u8 type)
return array;
 }
 
-static int ssd1307fb_write_array(struct i2c_client *client,
+static int ssd1307fb_write_array(struct ssd1307fb_par *par,
 struct ssd1307fb_array *array, u32 len)
 {
+   struct i2c_client *client = par->client;
int ret;
 
len += sizeof(struct ssd1307fb_array);
@@ -109,7 +110,7 @@ static int ssd1307fb_write_array(struct i2c_client *client,
return 0;
 }
 
-static inline int ssd1307fb_write_cmd(struct i2c_client *client, u8 cmd)
+static inline int ssd1307fb_write_cmd(struct ssd1307fb_par *par, u8 cmd)
 {
struct ssd1307fb_array *array;
int ret;
@@ -120,7 +121,7 @@ static inline int ssd1307fb_write_cmd(struct i2c_client 
*client, u8 cmd)
 
array->data[0] = cmd;
 
-   ret = ssd1307fb_write_array(client, array, 1);
+   ret = ssd1307fb_write_array(par, array, 1);
kfree(array);
 
return ret;
@@ -181,20 +182,20 @@ static void ssd1307fb_update_display(struct ssd1307fb_par 
*par)
}
}
 
-   ssd1307fb_write_array(par->client, array, par->width * par->height / 8);
+   ssd1307fb_write_array(par, array, par->width * par->height / 8);
kfree(array);
 }
 
 static void ssd1307fb_enable(struct fb_info *info)
 {
struct ssd1307fb_par *par = info->priv;
-   ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_ON);
+   ssd1307fb_write_cmd(par, SSD1307FB_DISPLAY_ON);
 }
 
 static void ssd1307fb_disable(struct fb_info *info)
 {
struct ssd1307fb_par *par = info->priv;
-   ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_OFF);
+   ssd1307fb_write_cmd(par, SSD1307FB_DISPLAY_OFF);
 }
 
 static void ssd1307fb_flush(struct fb_info *info)
@@ -215,134 +216,134 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
u32 precharge, dclk, com_invdir, compins;
 
/* Set initial contrast */
-   ret = ssd1307fb_write_cmd(par->client, SSD1307FB_CONTRAST);
+   ret = ssd1307fb_write_cmd(par, SSD1307FB_CONTRAST);
if (ret < 0)
return ret;
 
-   ret = ssd1307fb_write_cmd(par->client, par->contrast);
+   ret = ssd1307fb_write_cmd(par, par->contrast);
if (ret < 0)
return ret;
 
/* Set segment re-map */
if (par->seg_remap) {
-   ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SEG_REMAP_ON);
+   ret = ssd1307fb_write_cmd(par, SSD1307FB_SEG_REMAP_ON);
if (ret < 0)
return ret;
};
 
/* Set COM direction */
com_invdir = 0xc0 | (par->com_invdir & 0x1) << 3;
-   ret = ssd1307fb_write_cmd(par->client,  com_invdir);
+   ret = ssd1307fb_write_cmd(par,  com_invdir);
if (ret < 0)
return ret;
 
/* Set multiplex ratio value */
-   ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_MULTIPLEX_RATIO);
+   ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_MULTIPLEX_RATIO);
if (ret < 0)
return ret;
 
-   ret = ssd1307fb_write_cmd(par->client, par->height - 1);
+   ret = ssd1307fb_write_cmd(par, par->height - 1);
if (ret < 0)
return ret;
 
/* set display offset value */
-   ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_DISPLAY_OFFSET);
+   ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_DISPLAY_OFFSET);
if (ret < 0)
return ret;
 
-   ret = ssd1307fb_write_cmd(par->client, par->com_offset);
+   ret = ssd1307fb_write_cmd(par, par->com_offset);
if (ret < 0)
return ret;
 
/* Set clock frequency */
-   ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_CLOCK_FREQ);
+   ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_CLOCK_FREQ);
if (ret < 0)
return ret;
 
dclk = ((par->dclk_div - 1) & 0xf) | (par->dclk_frq & 0xf) << 4;
-   ret = ssd1307fb_write_cmd(par->client, dclk);
+   ret = ssd1307fb_write_cmd(par, dclk);
if (ret < 0)
return ret;
 
/* Set precharge period in number of ticks from the internal clock */
-   ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_PRECHARGE_PERIOD);
+   re

[PATCH 0/5] video: ssd1307fb: Add SPI support

[Michael Tretter]

Hello,

The Solomon single-chip CMOS OLED/PLED driver with controller can be connected
to I2C or SPI. The driver already supports I2C. This series adds support for
SPI connected displays to the driver.

Unfortunately, the bindings for the SPI connected display are not documented.
This driver uses the (undocumented) solomon,ssd1306 compatible of the staging
driver in Linux, but uses properties defined for the solomon,ssd1306fb-i2c
compatible of the I2C driver.

While the driver allows to use SPI and I2C, which would be a use case for
regmap, the driver does not use regmap, because the controller does not
actually expose registers, but simply accepts commands or data. This does not
match the regmap API. Therefore, the driver uses its own abstraction for the
bus.

The updated driver also allows to disable either SPI or I2C and uses #if
statements in the driver code, which I don't really like. I considered it
better than making the driver dependent on SPI and I2C, but if there is a
better way to handle this either/or dependency, I will gladly update the
driver accordingly.

Patches 1-4 refactor the driver to have fewer locations that refer to I2C to
simplify disabling the I2C support.

Patch 5 actually adds the SPI support and makes I2C optional.

Michael

Michael Tretter (5):
  video: ssd1307fb: pass par instead of i2c client to write
  video: ssd1307fb: don't use i2c client for logging
  video: ssd1307fb: move i2c setup to single place
  video: ssd1307fb: use function pointer for write
  video: ssd1307fb: add spi support

 drivers/video/Kconfig |   2 +-
 drivers/video/ssd1307fb.c | 180 ++
 2 files changed, 125 insertions(+), 57 deletions(-)

-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/5] video: ssd1307fb: add spi support

[Michael Tretter]

The Solomon display drivers also support SPI in addition to the I2C.
Add SPI support to the driver that already supports I2C by implementing
the bus write function for SPI and registering an SPI driver.

While the driver needs I2C or SPI, either subsystem is optional as long
as one is enabled.

WARNING: The device tree bindings for the ssd1306 with SPI are not
documented!

Signed-off-by: Michael Tretter 
---
 drivers/video/Kconfig |  2 +-
 drivers/video/ssd1307fb.c | 72 +++
 2 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index a87e8c063899..cfbd541a956e 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -15,7 +15,7 @@ config FRAMEBUFFER_CONSOLE
 
 config DRIVER_VIDEO_FB_SSD1307
bool "Solomon SSD1307 framebuffer support"
-   depends on I2C && GPIOLIB
+   depends on (I2C || SPI) && GPIOLIB
 
 config VIDEO_VPL
depends on OFTREE
diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index d0df073b8ef2..2939d4348405 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define SSD1307FB_DATA  0x40
 #define SSD1307FB_COMMAND   0x80
@@ -73,12 +74,14 @@ struct ssd1307fb_par {
u32 dclk_frq;
const struct ssd1307fb_deviceinfo *device_info;
struct i2c_client *client;
+   struct spi_device *spi;
u32 height;
struct fb_info *info;
u32 page_offset;
u32 prechargep1;
u32 prechargep2;
int reset;
+   int dc;
struct regulator *vbat;
u32 seg_remap;
u32 vcomh;
@@ -100,6 +103,30 @@ static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 
len, u8 type)
return array;
 }
 
+#if IS_ENABLED(CONFIG_SPI)
+static int ssd1307fb_spi_write_array(struct ssd1307fb_par *par,
+struct ssd1307fb_array *array, u32 len)
+{
+   struct spi_device *spi = par->spi;
+   int ret;
+
+   if (array->type == SSD1307FB_COMMAND)
+   gpio_direction_output(par->dc, 0);
+   else
+   gpio_direction_output(par->dc, 1);
+
+   ret = spi_write(spi, array->data, len);
+   if (ret)
+   dev_err(&spi->dev, "Couldn't send SPI command.\n");
+
+   /* Ensure that we remain in data mode. */
+   gpio_direction_output(par->dc, 1);
+
+   return ret;
+}
+#endif
+
+#if IS_ENABLED(CONFIG_I2C)
 static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
 struct ssd1307fb_array *array, u32 len)
 {
@@ -116,6 +143,7 @@ static int ssd1307fb_i2c_write_array(struct ssd1307fb_par 
*par,
 
return 0;
 }
+#endif
 
 static inline int ssd1307fb_write_cmd(struct ssd1307fb_par *par, u8 cmd)
 {
@@ -385,6 +413,10 @@ static const struct of_device_id ssd1307fb_of_match[] = {
.compatible = "solomon,ssd1306fb-i2c",
.data = (void *)&ssd1307fb_ssd1306_deviceinfo,
},
+   {
+   .compatible = "solomon,ssd1306",
+   .data = (void *)&ssd1307fb_ssd1306_deviceinfo,
+   },
{
.compatible = "solomon,ssd1309fb-i2c",
.data = (void *)&ssd1307fb_ssd1309_deviceinfo,
@@ -419,9 +451,24 @@ static int ssd1307fb_probe(struct device_d *dev)
 
par->device_info = (struct ssd1307fb_deviceinfo *)match->data;
 
-   par->client = to_i2c_client(dev);
-   i2c_set_clientdata(par->client, par);
-   par->write_array = ssd1307fb_i2c_write_array;
+#if IS_ENABLED(CONFIG_I2C)
+   if (dev->bus == &i2c_bus) {
+   par->client = to_i2c_client(dev);
+   i2c_set_clientdata(par->client, par);
+   par->write_array = ssd1307fb_i2c_write_array;
+   }
+#endif
+#if IS_ENABLED(CONFIG_SPI)
+   if (dev->bus == &spi_bus) {
+   par->spi = (struct spi_device *)dev->type_data;
+   par->dc = of_get_named_gpio(node, "dc-gpios", 0);
+   if (!gpio_is_valid(par->dc)) {
+   ret = par->dc;
+   goto fb_alloc_error;
+   }
+   par->write_array = ssd1307fb_spi_write_array;
+   }
+#endif
 
par->reset = of_get_named_gpio_flags(node,
 "reset-gpios", 0, &of_flags);
@@ -591,9 +638,22 @@ fb_alloc_error:
return ret;
 }
 
-static struct driver_d ssd1307fb_driver = {
-   .name = "ssd1307fb",
+static __maybe_unused struct driver_d ssd1307fb_i2c_driver = {
+   .name = "ssd1307fb-i2c",
.probe = ssd1307fb_probe,
.of_compatible = DRV_OF_COMPAT(ssd1307fb_of_match),
 };
-de

[PATCH 4/5] video: ssd1307fb: use function pointer for write

[Michael Tretter]

The function pointer is an abstraction the I2C accesses to be able to
add other bus protocols underneath the driver.

The functionality kind of reminds of regmap, but the driver does only
write data and does not actually use registers. Therefore, using regmap
with the register abstraction is not appropriate.

Signed-off-by: Michael Tretter 
---
 drivers/video/ssd1307fb.c | 26 +-
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 1538a1b640a3..d0df073b8ef2 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -53,6 +53,16 @@ struct ssd1307fb_deviceinfo {
int need_chargepump;
 };
 
+struct ssd1307fb_array {
+   u8 type;
+   u8 data[0];
+};
+
+struct ssd1307fb_par;
+
+typedef int (*ssd1307fb_write_array)(struct ssd1307fb_par *par,
+struct ssd1307fb_array *array, u32 len);
+
 struct ssd1307fb_par {
u32 com_invdir;
u32 com_lrremap;
@@ -73,11 +83,8 @@ struct ssd1307fb_par {
u32 seg_remap;
u32 vcomh;
u32 width;
-};
 
-struct ssd1307fb_array {
-   u8 type;
-   u8 data[0];
+   ssd1307fb_write_array write_array;
 };
 
 static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 len, u8 type)
@@ -93,8 +100,8 @@ static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 
len, u8 type)
return array;
 }
 
-static int ssd1307fb_write_array(struct ssd1307fb_par *par,
-struct ssd1307fb_array *array, u32 len)
+static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
+struct ssd1307fb_array *array, u32 len)
 {
struct i2c_client *client = par->client;
int ret;
@@ -121,7 +128,7 @@ static inline int ssd1307fb_write_cmd(struct ssd1307fb_par 
*par, u8 cmd)
 
array->data[0] = cmd;
 
-   ret = ssd1307fb_write_array(par, array, 1);
+   ret = par->write_array(par, array, 1);
kfree(array);
 
return ret;
@@ -182,7 +189,7 @@ static void ssd1307fb_update_display(struct ssd1307fb_par 
*par)
}
}
 
-   ssd1307fb_write_array(par, array, par->width * par->height / 8);
+   par->write_array(par, array, par->width * par->height / 8);
kfree(array);
 }
 
@@ -414,6 +421,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 
par->client = to_i2c_client(dev);
i2c_set_clientdata(par->client, par);
+   par->write_array = ssd1307fb_i2c_write_array;
 
par->reset = of_get_named_gpio_flags(node,
 "reset-gpios", 0, &of_flags);
@@ -564,7 +572,7 @@ static int ssd1307fb_probe(struct device_d *dev)
}
}
 
-   ssd1307fb_write_array(par, array, par->width * par->height / 8);
+   par->write_array(par, array, par->width * par->height / 8);
kfree(array);
 
dev_info(dev,
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/5] video: ssd1307fb: don't use i2c client for logging

[Michael Tretter]

We can use the device directly and don't have to use the device that is
attached to the I2C client. This reduces the dependency on i2c.

Signed-off-by: Michael Tretter 
---
 drivers/video/ssd1307fb.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 61d0e083a3f7..88c88e3253cf 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -401,7 +401,7 @@ static int ssd1307fb_probe(struct device_d *dev)
int i, j;
 
if (!node) {
-   dev_err(&client->dev, "No device tree data found!\n");
+   dev_err(dev, "No device tree data found!\n");
return -EINVAL;
}
 
@@ -421,22 +421,22 @@ static int ssd1307fb_probe(struct device_d *dev)
goto fb_alloc_error;
}
 
-   par->vbat = regulator_get(&client->dev, "vbat");
+   par->vbat = regulator_get(dev, "vbat");
if (IS_ERR(par->vbat)) {
-   dev_info(&client->dev, "Will not use VBAT");
+   dev_info(dev, "Will not use VBAT");
par->vbat = NULL;
}
 
ret = of_property_read_u32(node, "solomon,width", &par->width);
if (ret) {
-   dev_err(&client->dev,
+   dev_err(dev,
"Couldn't find 'solomon,width' in device tree.\n");
goto panel_init_error;
}
 
ret = of_property_read_u32(node, "solomon,height", &par->height);
if (ret) {
-   dev_err(&client->dev,
+   dev_err(dev,
"Couldn't find 'solomon,height' in device tree.\n");
goto panel_init_error;
}
@@ -444,7 +444,7 @@ static int ssd1307fb_probe(struct device_d *dev)
ret = of_property_read_u32(node, "solomon,page-offset",
   &par->page_offset);
if (ret) {
-   dev_err(&client->dev,
+   dev_err(dev,
"Couldn't find 'solomon,page_offset' in device 
tree.\n");
goto panel_init_error;
}
@@ -477,7 +477,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 
vmem = malloc(vmem_size);
if (!vmem) {
-   dev_err(&client->dev, "Couldn't allocate graphical memory.\n");
+   dev_err(dev, "Couldn't allocate graphical memory.\n");
ret = -ENOMEM;
goto fb_alloc_error;
}
@@ -508,7 +508,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 
ret = gpio_request_one(par->reset, flags, "oled-reset");
if (ret) {
-   dev_err(&client->dev,
+   dev_err(dev,
"failed to request gpio %d: %d\n",
par->reset, ret);
goto reset_oled_error;
@@ -546,7 +546,7 @@ static int ssd1307fb_probe(struct device_d *dev)
info->dev.parent = dev;
ret = register_framebuffer(info);
if (ret) {
-   dev_err(&client->dev, "Couldn't register the framebuffer\n");
+   dev_err(dev, "Couldn't register the framebuffer\n");
goto panel_init_error;
}
 
@@ -568,7 +568,7 @@ static int ssd1307fb_probe(struct device_d *dev)
ssd1307fb_write_array(par, array, par->width * par->height / 8);
kfree(array);
 
-   dev_info(&client->dev,
+   dev_info(dev,
 "ssd1307 framebuffer device registered, using %d bytes of 
video memory\n",
 vmem_size);
 
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/5] video: ssd1307fb: move i2c setup to single place

[Michael Tretter]

By having the entire i2c dependent initialzation in a single place, it
is easier to make it optional later.

Signed-off-by: Michael Tretter 
---
 drivers/video/ssd1307fb.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 88c88e3253cf..1538a1b640a3 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -387,7 +387,6 @@ static const struct of_device_id ssd1307fb_of_match[] = {
 
 static int ssd1307fb_probe(struct device_d *dev)
 {
-   struct i2c_client *client = to_i2c_client(dev);
struct fb_info *info;
struct device_node *node = dev->device_node;
const struct of_device_id *match =
@@ -410,10 +409,12 @@ static int ssd1307fb_probe(struct device_d *dev)
 
info->priv = par;
par->info = info;
-   par->client = client;
 
par->device_info = (struct ssd1307fb_deviceinfo *)match->data;
 
+   par->client = to_i2c_client(dev);
+   i2c_set_clientdata(par->client, par);
+
par->reset = of_get_named_gpio_flags(node,
 "reset-gpios", 0, &of_flags);
if (!gpio_is_valid(par->reset) && par->reset == -EPROBE_DEFER) {
@@ -519,8 +520,6 @@ static int ssd1307fb_probe(struct device_d *dev)
if (ret < 0)
goto reset_oled_error;
 
-   i2c_set_clientdata(client, info);
-
if (par->reset > 0) {
/* Reset the screen */
gpio_set_active(par->reset, 1);
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] arm: zynqmp: add boot source support

[Michael Tretter]

On Fri, 10 Sep 2021 15:43:23 +0200, Michael Riesch wrote:
> The ZynqMP reports the mode pins sampled at POR via the register
> ZYNQMP_CRL_APB_BOOT_MODE_USER. This commit adds a function that reads
> the register and populates the boot source.
> 
> Signed-off-by: Michael Riesch 
> ---
>  arch/arm/mach-zynqmp/zynqmp.c | 43 +++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/arch/arm/mach-zynqmp/zynqmp.c b/arch/arm/mach-zynqmp/zynqmp.c
> index 5871c145b..262bc0dd4 100644
> --- a/arch/arm/mach-zynqmp/zynqmp.c
> +++ b/arch/arm/mach-zynqmp/zynqmp.c
> @@ -6,9 +6,11 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #define ZYNQMP_CRL_APB_BASE  0xff5e
> +#define ZYNQMP_CRL_APB_BOOT_MODE_USER(ZYNQMP_CRL_APB_BASE + 0x200)
>  #define ZYNQMP_CRL_APB_RESET_REASON  (ZYNQMP_CRL_APB_BASE + 0x220)
>  
>  /* External POR: The PS_POR_B reset signal pin was asserted. */
> @@ -26,6 +28,46 @@
>  /* Software debugger reset: Write to BLOCKONLY_RST [debug_only]. */
>  #define ZYNQMP_CRL_APB_RESET_REASON_DEBUG_SYS  BIT(6)
>  
> +struct zynqmp_bootsource {
> + enum bootsource src;
> + int instance;
> +};
> +
> +/* cf. Table 11-1 "Boot Modes" in UG1085 Zynq UltraScale+ Device TRM */
> +static struct zynqmp_bootsource boot_modes[] = {
> + { .src = BOOTSOURCE_JTAG, .instance = 0 },
> + { .src = BOOTSOURCE_SPI, .instance = 0 },
> + { .src = BOOTSOURCE_SPI, .instance = 0 },
> + { .src = BOOTSOURCE_MMC, .instance = 0 },
> + { .src = BOOTSOURCE_SPI_NAND, .instance = 0 },
> + { .src = BOOTSOURCE_MMC, .instance = 1 },
> + { .src = BOOTSOURCE_MMC, .instance = 0 },
> + { .src = BOOTSOURCE_USB, .instance = 0 },
> + { .src = BOOTSOURCE_JTAG, .instance = 0 },
> + { .src = BOOTSOURCE_JTAG, .instance = 0 },
> + { .src = BOOTSOURCE_UNKNOWN, .instance = 0 },
> + { .src = BOOTSOURCE_UNKNOWN, .instance = 0 },
> + { .src = BOOTSOURCE_UNKNOWN, .instance = 0 },
> + { .src = BOOTSOURCE_UNKNOWN, .instance = 0 },
> + { .src = BOOTSOURCE_MMC, .instance = 1 },
> +};

Thanks for the patch.

Please make the mapping of the Boot Mode value to the BOOTSOURCE explicit
instead of hiding in as the index into this array.

> +
> +static enum bootsource zynqmp_bootsource(void)
> +{
> + u32 v;
> +
> + v = readl(ZYNQMP_CRL_APB_BOOT_MODE_USER);
> + v &= 0x0F;
> +
> + if (v >= ARRAY_SIZE(boot_modes))
> + return BOOTSOURCE_UNKNOWN;
> +
> + bootsource_set(boot_modes[v].src);
> + bootsource_set_instance(boot_modes[v].instance);

Don't set the bootsource as a side effect of this function. This function
should only lookup of the boot mode and zynqmp_init should actually set it.

Michael

> +
> + return boot_modes[v].src;
> +}
> +
>  struct zynqmp_reset_reason {
>   u32 mask;
>   enum reset_src_type type;
> @@ -65,6 +107,7 @@ static enum reset_src_type zynqmp_get_reset_src(void)
>  
>  static int zynqmp_init(void)
>  {
> + zynqmp_bootsource();
>   reset_source_set(zynqmp_get_reset_src());
>  
>   return 0;
> -- 
> 2.17.1
> 
> 
> ___
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream

[Michael Tretter]

On Wed, 18 Aug 2021 15:47:02 +0200, Lucas Stach wrote:
> Am Mittwoch, dem 18.08.2021 um 15:35 +0200 schrieb Michael Tretter:
> > Trying to do unaligned access of coherent memory on AArch64 will lead to
> > an abort. This can happen when the FPGA loader copies the bitstream to
> > the temporary buffer for the transfer to the FPGA.
> > 
> > Convert the driver to use regular memory for the temporary buffer to
> > prevent the issue.
> > 
> > Signed-off-by: Michael Tretter 
> > ---
> >  drivers/firmware/zynqmp-fpga.c | 20 +---
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
> > index 667910479aa7..0a0e7e880849 100644
> > --- a/drivers/firmware/zynqmp-fpga.c
> > +++ b/drivers/firmware/zynqmp-fpga.c
> > @@ -203,7 +203,7 @@ static int fpgamgr_program_finish(struct 
> > firmware_handler *fh)
> > size_t body_length;
> > int header_length = 0;
> > enum xilinx_byte_order byte_order;
> > -   u64 addr;
> > +   dma_addr_t addr;
> > int status = 0;
> > u8 flags = ZYNQMP_FPGA_BIT_ONLY_BIN;
> >  
> > @@ -240,13 +240,19 @@ static int fpgamgr_program_finish(struct 
> > firmware_handler *fh)
> >  * memory. Allocate some extra space at the end of the buffer for the
> >  * bitstream size.
> >  */
> > -   buf_aligned = dma_alloc_coherent(body_length + sizeof(buf_size),
> > -DMA_ADDRESS_BROKEN);
> > +   buf_aligned = dma_alloc(body_length + sizeof(u32));
> > if (!buf_aligned) {
> > status = -ENOBUFS;
> > goto err_free;
> > }
> >  
> > +   addr = dma_map_single(&mgr->dev, buf_aligned,
> > + body_length + sizeof(u32), DMA_TO_DEVICE);
> > +   if (dma_mapping_error(&mgr->dev, addr)) {
> > +   status = -EFAULT;
> > +   goto err_free;
> > +   }
> > +
> Usage of both dma_map_single and explicit dma_sync_single_for_* for a
> single transfer looks odd. dma_map_single already does the cache sync,
> which you then do a second time in the sync calls.
> 
> Instead you should move this dma_map_single call to the place where you
> added the dma_sync_single_for_device and replace the
> dma_sync_single_for_cpu with a dma_unmap_single.

Thanks, I just sent a v2.

Michael

> 
> Regards,
> Lucas
> 
> > if (!(mgr->features & ZYNQMP_PM_FEATURE_BYTE_ORDER_IRREL) &&
> > byte_order == XILINX_BYTE_ORDER_BIN)
> > copy_words_swapped((u32 *)buf_aligned, body,
> > @@ -254,8 +260,6 @@ static int fpgamgr_program_finish(struct 
> > firmware_handler *fh)
> > else
> > memcpy((u32 *)buf_aligned, body, body_length);
> >  
> > -   addr = (u64)buf_aligned;
> > -
> > if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) {
> > buf_size = body_length;
> > } else {
> > @@ -263,11 +267,13 @@ static int fpgamgr_program_finish(struct 
> > firmware_handler *fh)
> > buf_size = addr + body_length;
> > }
> >  
> > -   status = mgr->eemi_ops->fpga_load(addr, buf_size, flags);
> > +   dma_sync_single_for_device(addr, body_length + sizeof(u32), 
> > DMA_TO_DEVICE);
> > +   status = mgr->eemi_ops->fpga_load((u64)addr, buf_size, flags);
> > +   dma_sync_single_for_cpu(addr, body_length + sizeof(u32), DMA_TO_DEVICE);
> > if (status < 0)
> > dev_err(&mgr->dev, "unable to load fpga\n");
> >  
> > -   dma_free_coherent(buf_aligned, 0, body_length + sizeof(buf_size));
> > +   dma_free(buf_aligned);
> >  
> >   err_free:
> > free(mgr->buf);
> 
> 
> 

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/3] firmware: zynqmp-fpga: avoid additional buffer for size argument

[Michael Tretter]

There are two different APIs for loading an FPGA via the pmu-fw as
indicated by the ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED feature flag.
The pmu-fw expects as second argument either the size of the bitstream or a
pointer to the size of the bitstream.

The driver allocates a separate buffer for the size, which results in
the allocation of a 4k page for storing a 32 bit value.

Allocate some more memory for the bitstream and append the size of the
bitstream at the end of the bitstream to avoid the additional memory
allocation.

Add a comment to explain the surprising size of the allocation.

Signed-off-by: Michael Tretter 
---
 drivers/firmware/zynqmp-fpga.c | 37 +++---
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
index 736d1950fa5e..667910479aa7 100644
--- a/drivers/firmware/zynqmp-fpga.c
+++ b/drivers/firmware/zynqmp-fpga.c
@@ -197,8 +197,8 @@ static void zynqmp_fpga_show_header(const struct device_d 
*dev,
 static int fpgamgr_program_finish(struct firmware_handler *fh)
 {
struct fpgamgr *mgr = container_of(fh, struct fpgamgr, fh);
-   char *buf_aligned;
-   u32 *buf_size = NULL;
+   u32 *buf_aligned;
+   u32 buf_size;
u32 *body;
size_t body_length;
int header_length = 0;
@@ -234,17 +234,14 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
goto err_free;
}
 
-   if (!(mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED)) {
-   buf_size = dma_alloc_coherent(sizeof(*buf_size),
-   DMA_ADDRESS_BROKEN);
-   if (!buf_size) {
-   status = -ENOBUFS;
-   goto err_free;
-   }
-   *buf_size = body_length;
-   }
-
-   buf_aligned = dma_alloc_coherent(body_length, DMA_ADDRESS_BROKEN);
+   /*
+* The PMU FW variants without the ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED
+* feature expect a pointer to the bitstream size, which is stored in
+* memory. Allocate some extra space at the end of the buffer for the
+* bitstream size.
+*/
+   buf_aligned = dma_alloc_coherent(body_length + sizeof(buf_size),
+DMA_ADDRESS_BROKEN);
if (!buf_aligned) {
status = -ENOBUFS;
goto err_free;
@@ -259,20 +256,18 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
 
addr = (u64)buf_aligned;
 
-   if (!(mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) && buf_size) {
-   status = mgr->eemi_ops->fpga_load(addr,
-   (u32)(uintptr_t)buf_size,
-   flags);
-   dma_free_coherent(buf_size, 0, sizeof(*buf_size));
+   if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) {
+   buf_size = body_length;
} else {
-   status = mgr->eemi_ops->fpga_load(addr, (u32)(body_length),
- flags);
+   buf_aligned[body_length / sizeof(*buf_aligned)] = body_length;
+   buf_size = addr + body_length;
}
 
+   status = mgr->eemi_ops->fpga_load(addr, buf_size, flags);
if (status < 0)
dev_err(&mgr->dev, "unable to load fpga\n");
 
-   dma_free_coherent(buf_aligned, 0, body_length);
+   dma_free_coherent(buf_aligned, 0, body_length + sizeof(buf_size));
 
  err_free:
free(mgr->buf);
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 3/3] firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream

[Michael Tretter]

Trying to do unaligned access of coherent memory on AArch64 will lead to
an abort. This can happen when the FPGA loader copies the bitstream to
the temporary buffer for the transfer to the FPGA.

Convert the driver to use regular memory for the temporary buffer to
prevent the issue.

Signed-off-by: Michael Tretter 
---
Changelog:

v2:
- drop dma_sync_single_for_device and use dma_map_single to flush
- fix missing free of DMA memory in case of mapping error
- use size_of(buf_size) to clarify extra space in buffer
---
 drivers/firmware/zynqmp-fpga.c | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
index 667910479aa7..b76607f7eec1 100644
--- a/drivers/firmware/zynqmp-fpga.c
+++ b/drivers/firmware/zynqmp-fpga.c
@@ -203,7 +203,7 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
size_t body_length;
int header_length = 0;
enum xilinx_byte_order byte_order;
-   u64 addr;
+   dma_addr_t addr;
int status = 0;
u8 flags = ZYNQMP_FPGA_BIT_ONLY_BIN;
 
@@ -237,11 +237,10 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
/*
 * The PMU FW variants without the ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED
 * feature expect a pointer to the bitstream size, which is stored in
-* memory. Allocate some extra space at the end of the buffer for the
+* memory. Allocate extra space at the end of the buffer for the
 * bitstream size.
 */
-   buf_aligned = dma_alloc_coherent(body_length + sizeof(buf_size),
-DMA_ADDRESS_BROKEN);
+   buf_aligned = dma_alloc(body_length + sizeof(buf_size));
if (!buf_aligned) {
status = -ENOBUFS;
goto err_free;
@@ -254,8 +253,6 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
else
memcpy((u32 *)buf_aligned, body, body_length);
 
-   addr = (u64)buf_aligned;
-
if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) {
buf_size = body_length;
} else {
@@ -263,11 +260,21 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
buf_size = addr + body_length;
}
 
-   status = mgr->eemi_ops->fpga_load(addr, buf_size, flags);
+   addr = dma_map_single(&mgr->dev, buf_aligned,
+ body_length + sizeof(buf_size), DMA_TO_DEVICE);
+   if (dma_mapping_error(&mgr->dev, addr)) {
+   status = -EFAULT;
+   goto err_free_dma;
+   }
+
+   status = mgr->eemi_ops->fpga_load((u64)addr, buf_size, flags);
+   dma_unmap_single(&mgr->dev, addr, body_length + sizeof(buf_size),
+DMA_TO_DEVICE);
if (status < 0)
dev_err(&mgr->dev, "unable to load fpga\n");
 
-   dma_free_coherent(buf_aligned, 0, body_length + sizeof(buf_size));
+err_free_dma:
+   dma_free(buf_aligned);
 
  err_free:
free(mgr->buf);
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 0/3] firmware: zynqmp-fpga: fix fpga loading with optimized string functions

[Michael Tretter]

If CONFIG_ARM_OPTIMZED_STRING_FUNCTIONS is enabled, loading the FPGA fails
with an abort, because the optimized memcpy can only be used on cached memory.
As the bitstream can be several MBs large, we want to use the optimized
functions. Fix the abort by using a cached mapping with streaming DMA.

v2 drops the explicit dma_sync_single_for_device and instead uses
dma_map_single to flush the temporary buffer. I also fixed the error handling
in case the mapping fails and made the size of the extra space at the end of
the temporary buffer more explicit.

Michael

Changelog:

Michael Tretter (3):
  firmware: zynqmp-fpga: initialize flags at function start
  firmware: zynqmp-fpga: avoid additional buffer for size argument
  firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream

 drivers/firmware/zynqmp-fpga.c | 55 +-
 1 file changed, 27 insertions(+), 28 deletions(-)

-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 1/3] firmware: zynqmp-fpga: initialize flags at function start

[Michael Tretter]

The ZYNQMP_FPGA_BIT_ONLY_BIN flag is always set when programming the
FPGA. Simplify the code by initializing the flags with
ZYNQMP_FPGA_BIT_ONLY_BIN already set.

Signed-off-by: Michael Tretter 
---
 drivers/firmware/zynqmp-fpga.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
index 0fc229bfd3dd..736d1950fa5e 100644
--- a/drivers/firmware/zynqmp-fpga.c
+++ b/drivers/firmware/zynqmp-fpga.c
@@ -205,7 +205,7 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
enum xilinx_byte_order byte_order;
u64 addr;
int status = 0;
-   u8 flags = 0;
+   u8 flags = ZYNQMP_FPGA_BIT_ONLY_BIN;
 
if (!mgr->buf) {
status = -ENOBUFS;
@@ -259,9 +259,6 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
 
addr = (u64)buf_aligned;
 
-   /* we do not provide a header */
-   flags |= ZYNQMP_FPGA_BIT_ONLY_BIN;
-
if (!(mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) && buf_size) {
status = mgr->eemi_ops->fpga_load(addr,
(u32)(uintptr_t)buf_size,
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream

[Michael Tretter]

Trying to do unaligned access of coherent memory on AArch64 will lead to
an abort. This can happen when the FPGA loader copies the bitstream to
the temporary buffer for the transfer to the FPGA.

Convert the driver to use regular memory for the temporary buffer to
prevent the issue.

Signed-off-by: Michael Tretter 
---
 drivers/firmware/zynqmp-fpga.c | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
index 667910479aa7..0a0e7e880849 100644
--- a/drivers/firmware/zynqmp-fpga.c
+++ b/drivers/firmware/zynqmp-fpga.c
@@ -203,7 +203,7 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
size_t body_length;
int header_length = 0;
enum xilinx_byte_order byte_order;
-   u64 addr;
+   dma_addr_t addr;
int status = 0;
u8 flags = ZYNQMP_FPGA_BIT_ONLY_BIN;
 
@@ -240,13 +240,19 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
 * memory. Allocate some extra space at the end of the buffer for the
 * bitstream size.
 */
-   buf_aligned = dma_alloc_coherent(body_length + sizeof(buf_size),
-DMA_ADDRESS_BROKEN);
+   buf_aligned = dma_alloc(body_length + sizeof(u32));
if (!buf_aligned) {
status = -ENOBUFS;
goto err_free;
}
 
+   addr = dma_map_single(&mgr->dev, buf_aligned,
+ body_length + sizeof(u32), DMA_TO_DEVICE);
+   if (dma_mapping_error(&mgr->dev, addr)) {
+   status = -EFAULT;
+   goto err_free;
+   }
+
if (!(mgr->features & ZYNQMP_PM_FEATURE_BYTE_ORDER_IRREL) &&
byte_order == XILINX_BYTE_ORDER_BIN)
copy_words_swapped((u32 *)buf_aligned, body,
@@ -254,8 +260,6 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
else
memcpy((u32 *)buf_aligned, body, body_length);
 
-   addr = (u64)buf_aligned;
-
if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) {
buf_size = body_length;
} else {
@@ -263,11 +267,13 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
buf_size = addr + body_length;
}
 
-   status = mgr->eemi_ops->fpga_load(addr, buf_size, flags);
+   dma_sync_single_for_device(addr, body_length + sizeof(u32), 
DMA_TO_DEVICE);
+   status = mgr->eemi_ops->fpga_load((u64)addr, buf_size, flags);
+   dma_sync_single_for_cpu(addr, body_length + sizeof(u32), DMA_TO_DEVICE);
if (status < 0)
dev_err(&mgr->dev, "unable to load fpga\n");
 
-   dma_free_coherent(buf_aligned, 0, body_length + sizeof(buf_size));
+   dma_free(buf_aligned);
 
  err_free:
free(mgr->buf);
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] firmware: zynqmp-fpga: avoid additional buffer for size argument

[Michael Tretter]

There are two different APIs for loading an FPGA via the pmu-fw as
indicated by the ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED feature flag.
The pmu-fw expects as second argument either the size of the bitstream or a
pointer to the size of the bitstream.

The driver allocates a separate buffer for the size, which results in
the allocation of a 4k page for storing a 32 bit value.

Allocate some more memory for the bitstream and append the size of the
bitstream at the end of the bitstream to avoid the additional memory
allocation.

Add a comment to explain the surprising size of the allocation.

Signed-off-by: Michael Tretter 
---
 drivers/firmware/zynqmp-fpga.c | 37 +++---
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
index 736d1950fa5e..667910479aa7 100644
--- a/drivers/firmware/zynqmp-fpga.c
+++ b/drivers/firmware/zynqmp-fpga.c
@@ -197,8 +197,8 @@ static void zynqmp_fpga_show_header(const struct device_d 
*dev,
 static int fpgamgr_program_finish(struct firmware_handler *fh)
 {
struct fpgamgr *mgr = container_of(fh, struct fpgamgr, fh);
-   char *buf_aligned;
-   u32 *buf_size = NULL;
+   u32 *buf_aligned;
+   u32 buf_size;
u32 *body;
size_t body_length;
int header_length = 0;
@@ -234,17 +234,14 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
goto err_free;
}
 
-   if (!(mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED)) {
-   buf_size = dma_alloc_coherent(sizeof(*buf_size),
-   DMA_ADDRESS_BROKEN);
-   if (!buf_size) {
-   status = -ENOBUFS;
-   goto err_free;
-   }
-   *buf_size = body_length;
-   }
-
-   buf_aligned = dma_alloc_coherent(body_length, DMA_ADDRESS_BROKEN);
+   /*
+* The PMU FW variants without the ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED
+* feature expect a pointer to the bitstream size, which is stored in
+* memory. Allocate some extra space at the end of the buffer for the
+* bitstream size.
+*/
+   buf_aligned = dma_alloc_coherent(body_length + sizeof(buf_size),
+DMA_ADDRESS_BROKEN);
if (!buf_aligned) {
status = -ENOBUFS;
goto err_free;
@@ -259,20 +256,18 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
 
addr = (u64)buf_aligned;
 
-   if (!(mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) && buf_size) {
-   status = mgr->eemi_ops->fpga_load(addr,
-   (u32)(uintptr_t)buf_size,
-   flags);
-   dma_free_coherent(buf_size, 0, sizeof(*buf_size));
+   if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) {
+   buf_size = body_length;
} else {
-   status = mgr->eemi_ops->fpga_load(addr, (u32)(body_length),
- flags);
+   buf_aligned[body_length / sizeof(*buf_aligned)] = body_length;
+   buf_size = addr + body_length;
}
 
+   status = mgr->eemi_ops->fpga_load(addr, buf_size, flags);
if (status < 0)
dev_err(&mgr->dev, "unable to load fpga\n");
 
-   dma_free_coherent(buf_aligned, 0, body_length);
+   dma_free_coherent(buf_aligned, 0, body_length + sizeof(buf_size));
 
  err_free:
free(mgr->buf);
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/3] firmware: zynqmp-fpga: initialize flags at function start

[Michael Tretter]

The ZYNQMP_FPGA_BIT_ONLY_BIN flag is always set when programming the
FPGA. Simplify the code by initializing the flags with
ZYNQMP_FPGA_BIT_ONLY_BIN already set.

Signed-off-by: Michael Tretter 
---
 drivers/firmware/zynqmp-fpga.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
index 0fc229bfd3dd..736d1950fa5e 100644
--- a/drivers/firmware/zynqmp-fpga.c
+++ b/drivers/firmware/zynqmp-fpga.c
@@ -205,7 +205,7 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
enum xilinx_byte_order byte_order;
u64 addr;
int status = 0;
-   u8 flags = 0;
+   u8 flags = ZYNQMP_FPGA_BIT_ONLY_BIN;
 
if (!mgr->buf) {
status = -ENOBUFS;
@@ -259,9 +259,6 @@ static int fpgamgr_program_finish(struct firmware_handler 
*fh)
 
addr = (u64)buf_aligned;
 
-   /* we do not provide a header */
-   flags |= ZYNQMP_FPGA_BIT_ONLY_BIN;
-
if (!(mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) && buf_size) {
status = mgr->eemi_ops->fpga_load(addr,
(u32)(uintptr_t)buf_size,
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 0/3] firmware: zynqmp-fpga: fix fpga loading with optimized string functions

[Michael Tretter]

If CONFIG_ARM_OPTIMZED_STRING_FUNCTIONS is enabled, loading the FPGA fails
with an abort, because the optimized memcpy can only be used on cached memory.
As the bitstream can be several MBs large, we want to use the optimized
functions. Fix the abort by using a cached mapping with streaming DMA.

Michael Tretter (3):
  firmware: zynqmp-fpga: initialize flags at function start
  firmware: zynqmp-fpga: avoid additional buffer for size argument
  firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream

 drivers/firmware/zynqmp-fpga.c | 54 --
 1 file changed, 26 insertions(+), 28 deletions(-)

-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH] ARM: zynqmp: use std_file_update as update handler

[Michael Tretter]

The update handler for zynqmp copies the boot.bin file into an existing
fat partition. There is already a better implementation by
bbu_register_std_file_update(). Drop the custom implementation.

Keep the previous functions with its signature to have an obvious common
update handler for all ZynqMP boards.

Suggested-by: Ahmad Fatoum 
Signed-off-by: Michael Tretter 
---
 arch/arm/mach-zynqmp/zynqmp-bbu.c | 40 +++
 1 file changed, 3 insertions(+), 37 deletions(-)

diff --git a/arch/arm/mach-zynqmp/zynqmp-bbu.c 
b/arch/arm/mach-zynqmp/zynqmp-bbu.c
index d1197c01dc41..7ac8c2b8a994 100644
--- a/arch/arm/mach-zynqmp/zynqmp-bbu.c
+++ b/arch/arm/mach-zynqmp/zynqmp-bbu.c
@@ -4,45 +4,11 @@
  */
 
 #include 
-#include 
 #include 
 
-static int zynqmp_bbu_handler(struct bbu_handler *handler,
-   struct bbu_data *data)
-{
-   int ret = 0;
-
-   ret = bbu_confirm(data);
-   if (ret)
-   return ret;
-
-   ret = copy_file(data->imagefile, data->devicefile, 1);
-   if (ret < 0) {
-   pr_err("update failed: %s", strerror(-ret));
-   return ret;
-   }
-
-   return ret;
-}
-
 int zynqmp_bbu_register_handler(const char *name, char *devicefile,
-   unsigned long flags)
+   unsigned long flags)
 {
-   struct bbu_handler *handler;
-   int ret = 0;
-
-   if (!name || !devicefile)
-   return -EINVAL;
-
-   handler = xzalloc(sizeof(*handler));
-   handler->name = name;
-   handler->devicefile = devicefile;
-   handler->flags = flags;
-   handler->handler = zynqmp_bbu_handler;
-
-   ret = bbu_register_handler(handler);
-   if (ret)
-   free(handler);
-
-   return ret;
+   return bbu_register_std_file_update(name, flags, devicefile,
+   filetype_zynq_image);
 }
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/7] ARM: zynqmp: add update handler

[Michael Tretter]

On Thu, 24 Jun 2021 15:34:33 +0200, Ahmad Fatoum wrote:
> On 24.06.21 15:24, Michael Tretter wrote:
> > On Thu, 24 Jun 2021 12:52:58 +0200, Ahmad Fatoum wrote:
> >> On 24.06.21 12:23, Michael Tretter wrote:
> >>> The ZynqMP boots from an SDHCI device by reading a boot.bin file from
> >>> the FAT16/32 partition, which is the first partition in the MBR.
> >>>
> >>> The update handler copies a boot.bin image to this partition, which
> >>> might be board specific.
> >>
> >> Is barebox the boot.bin or is that a separate first stage bootloader?
> > 
> > Both. The boot.bin contains at least the first stage bootloader, TF-A, and
> > Barebox. You cannot boot a "standalone" Barebox image on the ZynqMP.
> > 
> > I also documented this in the zynqmp documentation as part of this patch
> > series.
> 
> I saw that after writing the mail here. Thanks for clarifying.
> 
> > 
> > Michael
> > 
> >>
> >>>
> >>> Signed-off-by: Michael Tretter 
> >>> ---
> >>>  arch/arm/mach-zynqmp/Makefile |  1 +
> >>>  .../arm/mach-zynqmp/include/mach/zynqmp-bbu.h | 21 
> >>>  arch/arm/mach-zynqmp/zynqmp-bbu.c | 48 +++
> >>>  3 files changed, 70 insertions(+)
> >>>  create mode 100644 arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
> >>>  create mode 100644 arch/arm/mach-zynqmp/zynqmp-bbu.c
> >>>
> >>> diff --git a/arch/arm/mach-zynqmp/Makefile b/arch/arm/mach-zynqmp/Makefile
> >>> index 14b8a4e46b87..e24a43c0d59f 100644
> >>> --- a/arch/arm/mach-zynqmp/Makefile
> >>> +++ b/arch/arm/mach-zynqmp/Makefile
> >>> @@ -1,3 +1,4 @@
> >>>  # SPDX-License-Identifier: GPL-2.0-or-later
> >>>  obj-y += firmware-zynqmp.o
> >>>  obj-y += zynqmp.o
> >>> +obj-$(CONFIG_BAREBOX_UPDATE) += zynqmp-bbu.o
> >>> diff --git a/arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h 
> >>> b/arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
> >>> new file mode 100644
> >>> index ..8502791ee0f7
> >>> --- /dev/null
> >>> +++ b/arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
> >>> @@ -0,0 +1,21 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-only */
> >>> +/*
> >>> + * Copyright (C) 2020 Michael Tretter 
> >>> + */
> >>> +#ifndef __MACH_ZYNQMP_BBU_H
> >>> +#define __MACH_ZYNQMP_BBU_H
> >>> +
> >>> +#include 
> >>> +
> >>> +#ifdef CONFIG_BAREBOX_UPDATE
> >>> +int zynqmp_bbu_register_handler(const char *name, char *devicefile,
> >>> +     unsigned long flags);
> >>> +#else
> >>> +static int zynqmp_bbu_register_handler(const char *name, char 
> >>> *devicefile,
> >>> +unsigned long flags)
> >>> +{
> >>> + return 0;
> >>> +};
> >>> +#endif
> >>> +
> >>> +#endif /* __MACH_ZYNQMP_BBU_H */
> >>> diff --git a/arch/arm/mach-zynqmp/zynqmp-bbu.c 
> >>> b/arch/arm/mach-zynqmp/zynqmp-bbu.c
> >>> new file mode 100644
> >>> index ..d1197c01dc41
> >>> --- /dev/null
> >>> +++ b/arch/arm/mach-zynqmp/zynqmp-bbu.c
> >>> @@ -0,0 +1,48 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-only
> >>> +/*
> >>> + * Copyright (C) 2020 Michael Tretter 
> >>> + */
> >>> +
> >>> +#include 
> >>> +#include 
> >>> +#include 
> >>> +
> >>> +static int zynqmp_bbu_handler(struct bbu_handler *handler,
> >>> + struct bbu_data *data)
> >>> +{
> >>> + int ret = 0;
> >>> +
> >>> + ret = bbu_confirm(data);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + ret = copy_file(data->imagefile, data->devicefile, 1);
> >>> + if (ret < 0) {
> >>> + pr_err("update failed: %s", strerror(-ret));
> >>> + return ret;
> >>> + }
> 
> EFI also registers an barebox update handler for a file: 
> /boot/EFI/BOOT/BOOTx64.EFI,
> but there apparently bbu_register_std_file_update suffices.
> 
> Did you check if that would work for you as well?
> 
> It's still a good idea to have a zynqmp specific wrapper 

[PATCH v2 7/7] Documentation: zynqmp: add some documentation

[Michael Tretter]

Add at least some information how Barebox can be used on the ZynqMP and
what is required to create a bootable image.

Signed-off-by: Michael Tretter 
---
Changelog:

v2: none
---
 Documentation/boards/zynqmp.rst | 40 +
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/boards/zynqmp.rst

diff --git a/Documentation/boards/zynqmp.rst b/Documentation/boards/zynqmp.rst
new file mode 100644
index ..05d41c401dc2
--- /dev/null
+++ b/Documentation/boards/zynqmp.rst
@@ -0,0 +1,40 @@
+Xilinx ZynqMP Ultrascale+
+=
+
+Barebox has support as a second stage boot loader for the Xilinx ZynqMP
+Ultrascale+.
+
+Image creation
+--
+
+Currently, Barebox only supports booting as a second stage boot loader from an
+SD-card. It relies on the FSBL_ to initialize the base system including sdram
+setup and pin muxing.
+
+The ZynqMP defconfig supports the ZCU104 reference board. Use it to build the
+Barebox image::
+
+   make ARCH=arm64 zynqmp_defconfig
+   make ARCH=arm64
+
+.. note:: The resulting image ``images/barebox-zynqmp-zcu104.img`` is **not** 
an image
+  that can directly be booted on the ZynqMP.
+
+For a bootable BOOT.BIN image, you also need to build the FSBL_ and a ZynqMP
+TF-A. Prepare these separately using the respective instructions.
+
+Use bootgen_ or ``mkimage -T zynqmpbif`` from the U-boot tools to build the
+final BOOT.BIN image that can be loaded by the ROM code. Check the
+instructions for these tools how to prepare the BOOT.BIN image.
+
+Create a FAT partition as the first partition of the SD card and copy the
+produced BOOT.BIN into this partition.
+
+.. _FSBL: `https://github.com/Xilinx/embeddedsw/`
+.. _bootgen: `https://github.com/Xilinx/bootgen`
+
+Booting Barebox
+---
+
+The FSBL loads the TF-A and Barebox, jumps to the TF-A, which will then return
+to Barebox. Afterwards, you can use Barebox as usual.
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 1/7] ARM: zynqmp: set reset source

[Michael Tretter]

The reset reason is available in the APB register set on the ZynqMP.
Read the reset reason and set the reset source accordingly.

There might be multiple bits set in the APB register. Use the MSB for
determining the actual reset source.

Signed-off-by: Michael Tretter 
---
Changelog:

v2:

- Use RESET_JTAG for debugger reset
- Remove unused zynqmp.h
---
 arch/arm/mach-zynqmp/Makefile |  1 +
 arch/arm/mach-zynqmp/zynqmp.c | 72 +++
 2 files changed, 73 insertions(+)
 create mode 100644 arch/arm/mach-zynqmp/zynqmp.c

diff --git a/arch/arm/mach-zynqmp/Makefile b/arch/arm/mach-zynqmp/Makefile
index 021efc94afaf..14b8a4e46b87 100644
--- a/arch/arm/mach-zynqmp/Makefile
+++ b/arch/arm/mach-zynqmp/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 obj-y += firmware-zynqmp.o
+obj-y += zynqmp.o
diff --git a/arch/arm/mach-zynqmp/zynqmp.c b/arch/arm/mach-zynqmp/zynqmp.c
new file mode 100644
index ..5871c145bef2
--- /dev/null
+++ b/arch/arm/mach-zynqmp/zynqmp.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 Michael Tretter 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#define ZYNQMP_CRL_APB_BASE0xff5e
+#define ZYNQMP_CRL_APB_RESET_REASON(ZYNQMP_CRL_APB_BASE + 0x220)
+
+/* External POR: The PS_POR_B reset signal pin was asserted. */
+#define ZYNQMP_CRL_APB_RESET_REASON_EXTERNAL   BIT(0)
+/* Internal POR: A system error triggered a POR reset. */
+#define ZYNQMP_CRL_APB_RESET_REASON_INTERNAL   BIT(1)
+/* Internal system reset; A system error triggered a system reset. */
+#define ZYNQMP_CRL_APB_RESET_REASON_PMUBIT(2)
+/* PS-only reset: Write to PMU_GLOBAL.GLOBAL_RESET [PS_ONLY_RST]. */
+#define ZYNQMP_CRL_APB_RESET_REASON_PSONLY BIT(3)
+/* External system reset: The PS_SRST_B reset signal pin was asserted. */
+#define ZYNQMP_CRL_APB_RESET_REASON_SRST   BIT(4)
+/* Software system reset: Write to RESET_CTRL [soft_reset]. */
+#define ZYNQMP_CRL_APB_RESET_REASON_SOFT   BIT(5)
+/* Software debugger reset: Write to BLOCKONLY_RST [debug_only]. */
+#define ZYNQMP_CRL_APB_RESET_REASON_DEBUG_SYS  BIT(6)
+
+struct zynqmp_reset_reason {
+   u32 mask;
+   enum reset_src_type type;
+};
+
+static const struct zynqmp_reset_reason reset_reasons[] = {
+   { ZYNQMP_CRL_APB_RESET_REASON_DEBUG_SYS,RESET_JTAG },
+   { ZYNQMP_CRL_APB_RESET_REASON_SOFT, RESET_RST },
+   { ZYNQMP_CRL_APB_RESET_REASON_SRST, RESET_POR },
+   { ZYNQMP_CRL_APB_RESET_REASON_PSONLY,   RESET_POR },
+   { ZYNQMP_CRL_APB_RESET_REASON_PMU,  RESET_POR },
+   { ZYNQMP_CRL_APB_RESET_REASON_INTERNAL, RESET_POR },
+   { ZYNQMP_CRL_APB_RESET_REASON_EXTERNAL, RESET_POR },
+   { /* sentinel */ }
+};
+
+static enum reset_src_type zynqmp_get_reset_src(void)
+{
+   enum reset_src_type type = RESET_UKWN;
+   unsigned int i;
+   u32 val;
+
+   val = readl(ZYNQMP_CRL_APB_RESET_REASON);
+
+   for (i = 0; i < ARRAY_SIZE(reset_reasons); i++) {
+   if (val & reset_reasons[i].mask) {
+   type = reset_reasons[i].type;
+   break;
+   }
+   }
+
+   pr_info("ZynqMP reset reason %s (ZYNQMP_CRL_APB_RESET_REASON: 
0x%08x)\n",
+   reset_source_to_string(type), val);
+
+   return type;
+}
+
+static int zynqmp_init(void)
+{
+   reset_source_set(zynqmp_get_reset_src());
+
+   return 0;
+}
+postcore_initcall(zynqmp_init);
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 5/7] ARM: zynqmp: zcu104: register update handler

[Michael Tretter]

As the zcu104 stores the environment in the same partition as the
barebox image, the partition is already mounted in ENV_MNT_DIR
("/boot"). Therefore, the update handler has to use the already
existing mount point for the update.

Signed-off-by: Michael Tretter 
---
Changelog:

v2: none
---
 arch/arm/boards/xilinx-zcu104/Makefile |  1 +
 arch/arm/boards/xilinx-zcu104/board.c  | 18 ++
 2 files changed, 19 insertions(+)
 create mode 100644 arch/arm/boards/xilinx-zcu104/board.c

diff --git a/arch/arm/boards/xilinx-zcu104/Makefile 
b/arch/arm/boards/xilinx-zcu104/Makefile
index 884d6e63b019..297f77d57ab1 100644
--- a/arch/arm/boards/xilinx-zcu104/Makefile
+++ b/arch/arm/boards/xilinx-zcu104/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
+obj-y += board.o
 lwl-y += lowlevel.o lowlevel_init.o
diff --git a/arch/arm/boards/xilinx-zcu104/board.c 
b/arch/arm/boards/xilinx-zcu104/board.c
new file mode 100644
index ..7654d2bfac90
--- /dev/null
+++ b/arch/arm/boards/xilinx-zcu104/board.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 Michael Tretter 
+ */
+
+#include 
+#include 
+#include 
+
+static int zcu104_register_update_handler(void)
+{
+   if (!of_machine_is_compatible("xlnx,zynqmp-zcu104"))
+   return 0;
+
+   return zynqmp_bbu_register_handler("SD", "/boot/BOOT.BIN",
+  BBU_HANDLER_FLAG_DEFAULT);
+}
+device_initcall(zcu104_register_update_handler);
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 6/7] ARM: zynqmp: defconfig: enable more features

[Michael Tretter]

Enable network, file system, and PSCI support in the defconfig to get a
Barebox with a useful feature set when building the ZynqMP defconfig.

Signed-off-by: Michael Tretter 
---
Changelog:

v2: none
---
 arch/arm/configs/zynqmp_defconfig | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/configs/zynqmp_defconfig 
b/arch/arm/configs/zynqmp_defconfig
index 6f5612fa92a0..2cd878133223 100644
--- a/arch/arm/configs/zynqmp_defconfig
+++ b/arch/arm/configs/zynqmp_defconfig
@@ -1,5 +1,6 @@
 CONFIG_ARCH_ZYNQMP=y
 CONFIG_MACH_XILINX_ZCU104=y
+CONFIG_ARM_PSCI_CLIENT=y
 CONFIG_MMU=y
 CONFIG_MALLOC_SIZE=0x0
 CONFIG_MALLOC_TLSF=y
@@ -12,32 +13,54 @@ CONFIG_BOOTM_SHOW_TYPE=y
 CONFIG_BOOTM_VERBOSE=y
 CONFIG_BOOTM_INITRD=y
 CONFIG_BOOTM_OFTREE=y
+CONFIG_BLSPEC=y
 CONFIG_DEFAULT_ENVIRONMENT_GENERIC_NEW=y
+CONFIG_RESET_SOURCE=y
 CONFIG_LONGHELP=y
 CONFIG_CMD_MEMINFO=y
 CONFIG_CMD_GO=y
 CONFIG_CMD_RESET=y
 CONFIG_CMD_PARTITION=y
 CONFIG_CMD_EXPORT=y
+CONFIG_CMD_DEFAULTENV=y
 CONFIG_CMD_PRINTENV=y
 CONFIG_CMD_MAGICVAR=y
 CONFIG_CMD_MAGICVAR_HELP=y
 CONFIG_CMD_SAVEENV=y
 CONFIG_CMD_LN=y
 CONFIG_CMD_SLEEP=y
+CONFIG_CMD_DHCP=y
+CONFIG_CMD_MIITOOL=y
+CONFIG_CMD_PING=y
+CONFIG_CMD_TFTP=y
 CONFIG_CMD_EDIT=y
 CONFIG_CMD_MENU=y
 CONFIG_CMD_MENU_MANAGEMENT=y
 CONFIG_CMD_READLINE=y
 CONFIG_CMD_TIMEOUT=y
 CONFIG_CMD_CLK=y
+CONFIG_CMD_DETECT=y
+CONFIG_CMD_BAREBOX_UPDATE=y
+CONFIG_CMD_FIRMWARELOAD=y
+CONFIG_CMD_OF_OVERLAY=y
 CONFIG_CMD_OFTREE=y
 CONFIG_CMD_TIME=y
 CONFIG_NET=y
+CONFIG_NET_NFS=y
+CONFIG_OF_BAREBOX_DRIVERS=y
+CONFIG_OF_BAREBOX_ENV_IN_FS=y
+CONFIG_OF_OVERLAY_LIVE=y
 CONFIG_DRIVER_SERIAL_CADENCE=y
 CONFIG_DRIVER_NET_MACB=y
+CONFIG_DP83867_PHY=y
 # CONFIG_SPI is not set
 CONFIG_MCI=y
 CONFIG_MCI_ARASAN=y
 CONFIG_FIRMWARE_ZYNQMP_FPGA=y
+# CONFIG_VIRTIO_MENU is not set
+CONFIG_FS_EXT4=y
+CONFIG_FS_TFTP=y
+CONFIG_FS_NFS=y
+CONFIG_FS_FAT=y
+CONFIG_FS_FAT_WRITE=y
 CONFIG_DIGEST=y
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 0/7] ZynqMP: Cleanup and extend board support

[Michael Tretter]

This is v2 of the series to extend the ZynqMP ZCU104 board support.

This series adds support for reset source detection, Barebox environment,
barebox_update and soft reset (via PSCI) for the ZynqMP ZCU104 board. It
extends the zynqmp_defconfig to enable more features that you would probably
want by default on a ZynqMP board. I also added some introductory information
how to build and use Barebox on the ZynqMP.

Compared to v1, I removed the actually unused zynqmp.h file, and changed the
reset source from RESET_UKWN to RESET_JTAG on debugger reset.

Michael

Changelog:

- Set RESET_JTAG on debugger reset
- Remove unused zynqmp.h

Michael Tretter (7):
  ARM: zynqmp: set reset source
  clk: zynqmp: do not enable already enabled clocks
  dts: zcu104: add Barebox environment
  ARM: zynqmp: add update handler
  ARM: zynqmp: zcu104: register update handler
  ARM: zynqmp: defconfig: enable more features
  Documentation: zynqmp: add some documentation

 Documentation/boards/zynqmp.rst   | 40 +++
 arch/arm/boards/xilinx-zcu104/Makefile|  1 +
 arch/arm/boards/xilinx-zcu104/board.c | 18 +
 arch/arm/configs/zynqmp_defconfig | 23 ++
 arch/arm/dts/zynqmp-zcu104-revA.dts   | 10 +++
 arch/arm/mach-zynqmp/Makefile |  2 +
 .../arm/mach-zynqmp/include/mach/zynqmp-bbu.h | 21 ++
 arch/arm/mach-zynqmp/zynqmp-bbu.c | 48 +
 arch/arm/mach-zynqmp/zynqmp.c | 72 +++
 drivers/clk/zynqmp/clk-gate-zynqmp.c  |  3 +
 10 files changed, 238 insertions(+)
 create mode 100644 Documentation/boards/zynqmp.rst
 create mode 100644 arch/arm/boards/xilinx-zcu104/board.c
 create mode 100644 arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
 create mode 100644 arch/arm/mach-zynqmp/zynqmp-bbu.c
 create mode 100644 arch/arm/mach-zynqmp/zynqmp.c

-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 4/7] ARM: zynqmp: add update handler

[Michael Tretter]

The ZynqMP boots from an SDHCI device by reading a boot.bin file from
the FAT16/32 partition, which is the first partition in the MBR.

The update handler copies a boot.bin image to this partition, which
might be board specific.

Signed-off-by: Michael Tretter 
---
Changelog:

v2: none
---
 arch/arm/mach-zynqmp/Makefile |  1 +
 .../arm/mach-zynqmp/include/mach/zynqmp-bbu.h | 21 
 arch/arm/mach-zynqmp/zynqmp-bbu.c | 48 +++
 3 files changed, 70 insertions(+)
 create mode 100644 arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
 create mode 100644 arch/arm/mach-zynqmp/zynqmp-bbu.c

diff --git a/arch/arm/mach-zynqmp/Makefile b/arch/arm/mach-zynqmp/Makefile
index 14b8a4e46b87..e24a43c0d59f 100644
--- a/arch/arm/mach-zynqmp/Makefile
+++ b/arch/arm/mach-zynqmp/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 obj-y += firmware-zynqmp.o
 obj-y += zynqmp.o
+obj-$(CONFIG_BAREBOX_UPDATE) += zynqmp-bbu.o
diff --git a/arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h 
b/arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
new file mode 100644
index ..8502791ee0f7
--- /dev/null
+++ b/arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2020 Michael Tretter 
+ */
+#ifndef __MACH_ZYNQMP_BBU_H
+#define __MACH_ZYNQMP_BBU_H
+
+#include 
+
+#ifdef CONFIG_BAREBOX_UPDATE
+int zynqmp_bbu_register_handler(const char *name, char *devicefile,
+   unsigned long flags);
+#else
+static int zynqmp_bbu_register_handler(const char *name, char *devicefile,
+  unsigned long flags)
+{
+   return 0;
+};
+#endif
+
+#endif /* __MACH_ZYNQMP_BBU_H */
diff --git a/arch/arm/mach-zynqmp/zynqmp-bbu.c 
b/arch/arm/mach-zynqmp/zynqmp-bbu.c
new file mode 100644
index ..d1197c01dc41
--- /dev/null
+++ b/arch/arm/mach-zynqmp/zynqmp-bbu.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 Michael Tretter 
+ */
+
+#include 
+#include 
+#include 
+
+static int zynqmp_bbu_handler(struct bbu_handler *handler,
+   struct bbu_data *data)
+{
+   int ret = 0;
+
+   ret = bbu_confirm(data);
+   if (ret)
+   return ret;
+
+   ret = copy_file(data->imagefile, data->devicefile, 1);
+   if (ret < 0) {
+   pr_err("update failed: %s", strerror(-ret));
+   return ret;
+   }
+
+   return ret;
+}
+
+int zynqmp_bbu_register_handler(const char *name, char *devicefile,
+   unsigned long flags)
+{
+   struct bbu_handler *handler;
+   int ret = 0;
+
+   if (!name || !devicefile)
+   return -EINVAL;
+
+   handler = xzalloc(sizeof(*handler));
+   handler->name = name;
+   handler->devicefile = devicefile;
+   handler->flags = flags;
+   handler->handler = zynqmp_bbu_handler;
+
+   ret = bbu_register_handler(handler);
+   if (ret)
+   free(handler);
+
+   return ret;
+}
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 3/7] dts: zcu104: add Barebox environment

[Michael Tretter]

Use the same partition on the SD-card that is used by the ROM loader to
find the BOOT.BIN (which contains the FSBL and Barebox) to store the
Barebox environment.

Signed-off-by: Michael Tretter 
---
Changelog:

v2: none
---
 arch/arm/dts/zynqmp-zcu104-revA.dts | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/dts/zynqmp-zcu104-revA.dts 
b/arch/arm/dts/zynqmp-zcu104-revA.dts
index 8c467ee97045..95b60a6b1d69 100644
--- a/arch/arm/dts/zynqmp-zcu104-revA.dts
+++ b/arch/arm/dts/zynqmp-zcu104-revA.dts
@@ -8,3 +8,13 @@
  */
 
 #include 
+
+/ {
+   chosen {
+   environment {
+   compatible = "barebox,environment";
+   device-path = &sdhci1, "partname:0";
+   file-path = "barebox.env";
+   };
+   };
+};
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/7] clk: zynqmp: do not enable already enabled clocks

[Michael Tretter]

The pmu fw manages the permissions who can enable/disable the clocks.

There are a few clocks (TOPSW_LSBUS and LSBUS) which are exposed to
Barebox and Barebox assumes that is has to enable the clocks. However,
the pmu fw considers the clocks under its control and returns a
permission denied for the clock enable request.

Assume that clocks that are already enabled don't need to be enable by
Barebox to avoid the permission denied errors.

Signed-off-by: Michael Tretter 
---
Changelog:

v2: none
---
 drivers/clk/zynqmp/clk-gate-zynqmp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/zynqmp/clk-gate-zynqmp.c 
b/drivers/clk/zynqmp/clk-gate-zynqmp.c
index a3b9ee21e506..493c1dfeaaa3 100644
--- a/drivers/clk/zynqmp/clk-gate-zynqmp.c
+++ b/drivers/clk/zynqmp/clk-gate-zynqmp.c
@@ -28,6 +28,9 @@ static int zynqmp_clk_gate_enable(struct clk_hw *hw)
 {
struct zynqmp_clk_gate *gate = to_zynqmp_clk_gate(hw);
 
+   if (clk_hw_is_enabled(hw))
+   return 0;
+
return gate->ops->clock_enable(gate->clk_id);
 }
 
-- 
2.30.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/7] ARM: zynqmp: add update handler

[Michael Tretter]

On Thu, 24 Jun 2021 12:52:58 +0200, Ahmad Fatoum wrote:
> On 24.06.21 12:23, Michael Tretter wrote:
> > The ZynqMP boots from an SDHCI device by reading a boot.bin file from
> > the FAT16/32 partition, which is the first partition in the MBR.
> > 
> > The update handler copies a boot.bin image to this partition, which
> > might be board specific.
> 
> Is barebox the boot.bin or is that a separate first stage bootloader?

Both. The boot.bin contains at least the first stage bootloader, TF-A, and
Barebox. You cannot boot a "standalone" Barebox image on the ZynqMP.

I also documented this in the zynqmp documentation as part of this patch
series.

Michael

> 
> > 
> > Signed-off-by: Michael Tretter 
> > ---
> >  arch/arm/mach-zynqmp/Makefile |  1 +
> >  .../arm/mach-zynqmp/include/mach/zynqmp-bbu.h | 21 
> >  arch/arm/mach-zynqmp/zynqmp-bbu.c | 48 +++
> >  3 files changed, 70 insertions(+)
> >  create mode 100644 arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
> >  create mode 100644 arch/arm/mach-zynqmp/zynqmp-bbu.c
> > 
> > diff --git a/arch/arm/mach-zynqmp/Makefile b/arch/arm/mach-zynqmp/Makefile
> > index 14b8a4e46b87..e24a43c0d59f 100644
> > --- a/arch/arm/mach-zynqmp/Makefile
> > +++ b/arch/arm/mach-zynqmp/Makefile
> > @@ -1,3 +1,4 @@
> >  # SPDX-License-Identifier: GPL-2.0-or-later
> >  obj-y += firmware-zynqmp.o
> >  obj-y += zynqmp.o
> > +obj-$(CONFIG_BAREBOX_UPDATE) += zynqmp-bbu.o
> > diff --git a/arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h 
> > b/arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
> > new file mode 100644
> > index ..8502791ee0f7
> > --- /dev/null
> > +++ b/arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2020 Michael Tretter 
> > + */
> > +#ifndef __MACH_ZYNQMP_BBU_H
> > +#define __MACH_ZYNQMP_BBU_H
> > +
> > +#include 
> > +
> > +#ifdef CONFIG_BAREBOX_UPDATE
> > +int zynqmp_bbu_register_handler(const char *name, char *devicefile,
> > +   unsigned long flags);
> > +#else
> > +static int zynqmp_bbu_register_handler(const char *name, char *devicefile,
> > +  unsigned long flags)
> > +{
> > +   return 0;
> > +};
> > +#endif
> > +
> > +#endif /* __MACH_ZYNQMP_BBU_H */
> > diff --git a/arch/arm/mach-zynqmp/zynqmp-bbu.c 
> > b/arch/arm/mach-zynqmp/zynqmp-bbu.c
> > new file mode 100644
> > index ..d1197c01dc41
> > --- /dev/null
> > +++ b/arch/arm/mach-zynqmp/zynqmp-bbu.c
> > @@ -0,0 +1,48 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2020 Michael Tretter 
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +static int zynqmp_bbu_handler(struct bbu_handler *handler,
> > +   struct bbu_data *data)
> > +{
> > +   int ret = 0;
> > +
> > +   ret = bbu_confirm(data);
> > +   if (ret)
> > +   return ret;
> > +
> > +   ret = copy_file(data->imagefile, data->devicefile, 1);
> > +   if (ret < 0) {
> > +   pr_err("update failed: %s", strerror(-ret));
> > +   return ret;
> > +   }
> > +
> > +   return ret;
> > +}
> > +
> > +int zynqmp_bbu_register_handler(const char *name, char *devicefile,
> > +   unsigned long flags)
> > +{
> > +   struct bbu_handler *handler;
> > +   int ret = 0;
> > +
> > +   if (!name || !devicefile)
> > +   return -EINVAL;
> > +
> > +   handler = xzalloc(sizeof(*handler));
> > +   handler->name = name;
> > +   handler->devicefile = devicefile;
> > +   handler->flags = flags;
> > +   handler->handler = zynqmp_bbu_handler;
> > +
> > +   ret = bbu_register_handler(handler);
> > +   if (ret)
> > +   free(handler);
> > +
> > +   return ret;
> > +}
> > 

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/7] ARM: zynqmp: set reset source

[Michael Tretter]

On Thu, 24 Jun 2021 12:48:44 +0200, Ahmad Fatoum wrote:
> On 24.06.21 12:23, Michael Tretter wrote:
> > The reset reason is available in the APB register set on the ZynqMP.
> > Read the reset reason and set the reset source accordingly.
> > 
> > There might be multiple bits set in the APB register. Use the MSB for
> > determining the actual reset source.
> 
> APB is usually the AMBA Advanced Peripheral Bus. Perhaps CRL is the
> actual name of the register and APB just tells that it's mapped there?

Ack. The data sheet calls it "Clock and Reset control registers for LPD." I
will fix it in v2.

> 
> > 
> > Signed-off-by: Michael Tretter 
> > ---
> >  arch/arm/mach-zynqmp/Makefile  |  1 +
> >  arch/arm/mach-zynqmp/include/mach/zynqmp.h |  6 ++
> >  arch/arm/mach-zynqmp/zynqmp.c  | 74 ++
> >  3 files changed, 81 insertions(+)
> >  create mode 100644 arch/arm/mach-zynqmp/include/mach/zynqmp.h
> >  create mode 100644 arch/arm/mach-zynqmp/zynqmp.c
> > 
> > diff --git a/arch/arm/mach-zynqmp/Makefile b/arch/arm/mach-zynqmp/Makefile
> > index 021efc94afaf..14b8a4e46b87 100644
> > --- a/arch/arm/mach-zynqmp/Makefile
> > +++ b/arch/arm/mach-zynqmp/Makefile
> > @@ -1,2 +1,3 @@
> >  # SPDX-License-Identifier: GPL-2.0-or-later
> >  obj-y += firmware-zynqmp.o
> > +obj-y += zynqmp.o
> > diff --git a/arch/arm/mach-zynqmp/include/mach/zynqmp.h 
> > b/arch/arm/mach-zynqmp/include/mach/zynqmp.h
> > new file mode 100644
> > index ..f6c05f35a470
> > --- /dev/null
> > +++ b/arch/arm/mach-zynqmp/include/mach/zynqmp.h
> 
> Nitpick: revision.h sounds more self-describing.

Ok. I thought there might be some more soc specific function instead of
revision specific functions here, but revision.h is fine for me as well.

> 
> > @@ -0,0 +1,6 @@
> > +#ifndef __MACH_ZYNQMP_H
> > +#define __MACH_ZYNQMP_H
> > +
> > +int zynqmp_soc_revision(void);
> > +
> > +#endif /* __MACH_ZYNQMP_H */
> > diff --git a/arch/arm/mach-zynqmp/zynqmp.c b/arch/arm/mach-zynqmp/zynqmp.c
> > new file mode 100644
> > index ..2b3bd8406ce9
> > --- /dev/null
> > +++ b/arch/arm/mach-zynqmp/zynqmp.c
> > @@ -0,0 +1,74 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2020 Michael Tretter 
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +
> > +#define ZYNQMP_CRL_APB_BASE0xff5e
> > +#define ZYNQMP_CRL_APB_RESET_REASON(ZYNQMP_CRL_APB_BASE + 0x220)
> > +
> > +/* External POR: The PS_POR_B reset signal pin was asserted. */
> > +#define ZYNQMP_CRL_APB_RESET_REASON_EXTERNAL   BIT(0)
> > +/* Internal POR: A system error triggered a POR reset. */
> > +#define ZYNQMP_CRL_APB_RESET_REASON_INTERNAL   BIT(1)
> > +/* Internal system reset; A system error triggered a system reset. */
> > +#define ZYNQMP_CRL_APB_RESET_REASON_PMUBIT(2)
> > +/* PS-only reset: Write to PMU_GLOBAL.GLOBAL_RESET [PS_ONLY_RST]. */
> > +#define ZYNQMP_CRL_APB_RESET_REASON_PSONLY BIT(3)
> > +/* External system reset: The PS_SRST_B reset signal pin was asserted. */
> > +#define ZYNQMP_CRL_APB_RESET_REASON_SRST   BIT(4)
> > +/* Software system reset: Write to RESET_CTRL [soft_reset]. */
> > +#define ZYNQMP_CRL_APB_RESET_REASON_SOFT   BIT(5)
> > +/* Software debugger reset: Write to BLOCKONLY_RST [debug_only]. */
> > +#define ZYNQMP_CRL_APB_RESET_REASON_DEBUG_SYS  BIT(6)
> > +
> > +struct zynqmp_reset_reason {
> > +   u32 mask;
> > +   enum reset_src_type type;
> > +};
> > +
> > +static const struct zynqmp_reset_reason reset_reasons[] = {
> > +   { ZYNQMP_CRL_APB_RESET_REASON_DEBUG_SYS,RESET_UKWN },
> 
> RESET_JTAG?

I am not sure, if I understand RESET_JTAG correctly. The reference manual says
that it is like a soft reset while preserving the debug logic and originates
in the DAP controller. Is this RESET_JTAG?

> 
> > +   { ZYNQMP_CRL_APB_RESET_REASON_SOFT, RESET_RST },
> > +   { ZYNQMP_CRL_APB_RESET_REASON_SRST, RESET_POR },
> > +   { ZYNQMP_CRL_APB_RESET_REASON_PSONLY,   RESET_POR },
> > +   { ZYNQMP_CRL_APB_RESET_REASON_PMU,  RESET_POR },
> > +   { ZYNQMP_CRL_APB_RESET_REASON_INTERNAL, RESET_POR },
> > +   { ZYNQMP_CRL_APB_RESET_REASON_EXTERNAL, RESET_POR },
> 
> RESET_EXT?

This bit is set for a normal POR, too.

> 
> > +   { /* sentinel */ }
> > +};
> > +
> > +static 

[PATCH 6/7] ARM: zynqmp: defconfig: enable more features

[Michael Tretter]

Enable network, file system, and PSCI support in the defconfig to get a
Barebox with a useful feature set when building the ZynqMP defconfig.

Signed-off-by: Michael Tretter 
---
 arch/arm/configs/zynqmp_defconfig | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/configs/zynqmp_defconfig 
b/arch/arm/configs/zynqmp_defconfig
index 6f5612fa92a0..2cd878133223 100644
--- a/arch/arm/configs/zynqmp_defconfig
+++ b/arch/arm/configs/zynqmp_defconfig
@@ -1,5 +1,6 @@
 CONFIG_ARCH_ZYNQMP=y
 CONFIG_MACH_XILINX_ZCU104=y
+CONFIG_ARM_PSCI_CLIENT=y
 CONFIG_MMU=y
 CONFIG_MALLOC_SIZE=0x0
 CONFIG_MALLOC_TLSF=y
@@ -12,32 +13,54 @@ CONFIG_BOOTM_SHOW_TYPE=y
 CONFIG_BOOTM_VERBOSE=y
 CONFIG_BOOTM_INITRD=y
 CONFIG_BOOTM_OFTREE=y
+CONFIG_BLSPEC=y
 CONFIG_DEFAULT_ENVIRONMENT_GENERIC_NEW=y
+CONFIG_RESET_SOURCE=y
 CONFIG_LONGHELP=y
 CONFIG_CMD_MEMINFO=y
 CONFIG_CMD_GO=y
 CONFIG_CMD_RESET=y
 CONFIG_CMD_PARTITION=y
 CONFIG_CMD_EXPORT=y
+CONFIG_CMD_DEFAULTENV=y
 CONFIG_CMD_PRINTENV=y
 CONFIG_CMD_MAGICVAR=y
 CONFIG_CMD_MAGICVAR_HELP=y
 CONFIG_CMD_SAVEENV=y
 CONFIG_CMD_LN=y
 CONFIG_CMD_SLEEP=y
+CONFIG_CMD_DHCP=y
+CONFIG_CMD_MIITOOL=y
+CONFIG_CMD_PING=y
+CONFIG_CMD_TFTP=y
 CONFIG_CMD_EDIT=y
 CONFIG_CMD_MENU=y
 CONFIG_CMD_MENU_MANAGEMENT=y
 CONFIG_CMD_READLINE=y
 CONFIG_CMD_TIMEOUT=y
 CONFIG_CMD_CLK=y
+CONFIG_CMD_DETECT=y
+CONFIG_CMD_BAREBOX_UPDATE=y
+CONFIG_CMD_FIRMWARELOAD=y
+CONFIG_CMD_OF_OVERLAY=y
 CONFIG_CMD_OFTREE=y
 CONFIG_CMD_TIME=y
 CONFIG_NET=y
+CONFIG_NET_NFS=y
+CONFIG_OF_BAREBOX_DRIVERS=y
+CONFIG_OF_BAREBOX_ENV_IN_FS=y
+CONFIG_OF_OVERLAY_LIVE=y
 CONFIG_DRIVER_SERIAL_CADENCE=y
 CONFIG_DRIVER_NET_MACB=y
+CONFIG_DP83867_PHY=y
 # CONFIG_SPI is not set
 CONFIG_MCI=y
 CONFIG_MCI_ARASAN=y
 CONFIG_FIRMWARE_ZYNQMP_FPGA=y
+# CONFIG_VIRTIO_MENU is not set
+CONFIG_FS_EXT4=y
+CONFIG_FS_TFTP=y
+CONFIG_FS_NFS=y
+CONFIG_FS_FAT=y
+CONFIG_FS_FAT_WRITE=y
 CONFIG_DIGEST=y
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 7/7] Documentation: zynqmp: add some documentation

[Michael Tretter]

Add at least some information how Barebox can be used on the ZynqMP and
what is required to create a bootable image.

Signed-off-by: Michael Tretter 
---
 Documentation/boards/zynqmp.rst | 40 +
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/boards/zynqmp.rst

diff --git a/Documentation/boards/zynqmp.rst b/Documentation/boards/zynqmp.rst
new file mode 100644
index ..05d41c401dc2
--- /dev/null
+++ b/Documentation/boards/zynqmp.rst
@@ -0,0 +1,40 @@
+Xilinx ZynqMP Ultrascale+
+=
+
+Barebox has support as a second stage boot loader for the Xilinx ZynqMP
+Ultrascale+.
+
+Image creation
+--
+
+Currently, Barebox only supports booting as a second stage boot loader from an
+SD-card. It relies on the FSBL_ to initialize the base system including sdram
+setup and pin muxing.
+
+The ZynqMP defconfig supports the ZCU104 reference board. Use it to build the
+Barebox image::
+
+   make ARCH=arm64 zynqmp_defconfig
+   make ARCH=arm64
+
+.. note:: The resulting image ``images/barebox-zynqmp-zcu104.img`` is **not** 
an image
+  that can directly be booted on the ZynqMP.
+
+For a bootable BOOT.BIN image, you also need to build the FSBL_ and a ZynqMP
+TF-A. Prepare these separately using the respective instructions.
+
+Use bootgen_ or ``mkimage -T zynqmpbif`` from the U-boot tools to build the
+final BOOT.BIN image that can be loaded by the ROM code. Check the
+instructions for these tools how to prepare the BOOT.BIN image.
+
+Create a FAT partition as the first partition of the SD card and copy the
+produced BOOT.BIN into this partition.
+
+.. _FSBL: `https://github.com/Xilinx/embeddedsw/`
+.. _bootgen: `https://github.com/Xilinx/bootgen`
+
+Booting Barebox
+---
+
+The FSBL loads the TF-A and Barebox, jumps to the TF-A, which will then return
+to Barebox. Afterwards, you can use Barebox as usual.
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/7] clk: zynqmp: do not enable already enabled clocks

[Michael Tretter]

The pmu fw manages the permissions who can enable/disable the clocks.

There are a few clocks (TOPSW_LSBUS and LSBUS) which are exposed to
Barebox and Barebox assumes that is has to enable the clocks. However,
the pmu fw considers the clocks under its control and returns a
permission denied for the clock enable request.

Assume that clocks that are already enabled don't need to be enable by
Barebox to avoid the permission denied errors.

Signed-off-by: Michael Tretter 
---
 drivers/clk/zynqmp/clk-gate-zynqmp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/zynqmp/clk-gate-zynqmp.c 
b/drivers/clk/zynqmp/clk-gate-zynqmp.c
index a3b9ee21e506..493c1dfeaaa3 100644
--- a/drivers/clk/zynqmp/clk-gate-zynqmp.c
+++ b/drivers/clk/zynqmp/clk-gate-zynqmp.c
@@ -28,6 +28,9 @@ static int zynqmp_clk_gate_enable(struct clk_hw *hw)
 {
struct zynqmp_clk_gate *gate = to_zynqmp_clk_gate(hw);
 
+   if (clk_hw_is_enabled(hw))
+   return 0;
+
return gate->ops->clock_enable(gate->clk_id);
 }
 
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 0/7] ZynqMP: Cleanup and extend board support

[Michael Tretter]

Hello,

There were still a few things missing for real support for the ZynqMP ZCU104
in Barebox.

This series adds support for reset source detection, Barebox environment,
barebox_update and soft reset (via PSCI) for the ZynqMP ZCU104 board. It
extends the zynqmp_defconfig to enable more features that you would probably
want by default on a ZynqMP board. I also added some introductory information
how to build and use Barebox on the ZynqMP.

Michael

Michael Tretter (7):
  ARM: zynqmp: set reset source
  clk: zynqmp: do not enable already enabled clocks
  dts: zcu104: add Barebox environment
  ARM: zynqmp: add update handler
  ARM: zynqmp: zcu104: register update handler
  ARM: zynqmp: defconfig: enable more features
  Documentation: zynqmp: add some documentation

 Documentation/boards/zynqmp.rst   | 40 ++
 arch/arm/boards/xilinx-zcu104/Makefile|  1 +
 arch/arm/boards/xilinx-zcu104/board.c | 18 +
 arch/arm/configs/zynqmp_defconfig | 23 ++
 arch/arm/dts/zynqmp-zcu104-revA.dts   | 10 +++
 arch/arm/mach-zynqmp/Makefile |  2 +
 .../arm/mach-zynqmp/include/mach/zynqmp-bbu.h | 21 ++
 arch/arm/mach-zynqmp/include/mach/zynqmp.h|  6 ++
 arch/arm/mach-zynqmp/zynqmp-bbu.c | 48 
 arch/arm/mach-zynqmp/zynqmp.c | 74 +++
 drivers/clk/zynqmp/clk-gate-zynqmp.c  |  3 +
 11 files changed, 246 insertions(+)
 create mode 100644 Documentation/boards/zynqmp.rst
 create mode 100644 arch/arm/boards/xilinx-zcu104/board.c
 create mode 100644 arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
 create mode 100644 arch/arm/mach-zynqmp/include/mach/zynqmp.h
 create mode 100644 arch/arm/mach-zynqmp/zynqmp-bbu.c
 create mode 100644 arch/arm/mach-zynqmp/zynqmp.c

-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/7] ARM: zynqmp: add update handler

[Michael Tretter]

The ZynqMP boots from an SDHCI device by reading a boot.bin file from
the FAT16/32 partition, which is the first partition in the MBR.

The update handler copies a boot.bin image to this partition, which
might be board specific.

Signed-off-by: Michael Tretter 
---
 arch/arm/mach-zynqmp/Makefile |  1 +
 .../arm/mach-zynqmp/include/mach/zynqmp-bbu.h | 21 
 arch/arm/mach-zynqmp/zynqmp-bbu.c | 48 +++
 3 files changed, 70 insertions(+)
 create mode 100644 arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
 create mode 100644 arch/arm/mach-zynqmp/zynqmp-bbu.c

diff --git a/arch/arm/mach-zynqmp/Makefile b/arch/arm/mach-zynqmp/Makefile
index 14b8a4e46b87..e24a43c0d59f 100644
--- a/arch/arm/mach-zynqmp/Makefile
+++ b/arch/arm/mach-zynqmp/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 obj-y += firmware-zynqmp.o
 obj-y += zynqmp.o
+obj-$(CONFIG_BAREBOX_UPDATE) += zynqmp-bbu.o
diff --git a/arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h 
b/arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
new file mode 100644
index ..8502791ee0f7
--- /dev/null
+++ b/arch/arm/mach-zynqmp/include/mach/zynqmp-bbu.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2020 Michael Tretter 
+ */
+#ifndef __MACH_ZYNQMP_BBU_H
+#define __MACH_ZYNQMP_BBU_H
+
+#include 
+
+#ifdef CONFIG_BAREBOX_UPDATE
+int zynqmp_bbu_register_handler(const char *name, char *devicefile,
+   unsigned long flags);
+#else
+static int zynqmp_bbu_register_handler(const char *name, char *devicefile,
+  unsigned long flags)
+{
+   return 0;
+};
+#endif
+
+#endif /* __MACH_ZYNQMP_BBU_H */
diff --git a/arch/arm/mach-zynqmp/zynqmp-bbu.c 
b/arch/arm/mach-zynqmp/zynqmp-bbu.c
new file mode 100644
index ..d1197c01dc41
--- /dev/null
+++ b/arch/arm/mach-zynqmp/zynqmp-bbu.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 Michael Tretter 
+ */
+
+#include 
+#include 
+#include 
+
+static int zynqmp_bbu_handler(struct bbu_handler *handler,
+   struct bbu_data *data)
+{
+   int ret = 0;
+
+   ret = bbu_confirm(data);
+   if (ret)
+   return ret;
+
+   ret = copy_file(data->imagefile, data->devicefile, 1);
+   if (ret < 0) {
+   pr_err("update failed: %s", strerror(-ret));
+   return ret;
+   }
+
+   return ret;
+}
+
+int zynqmp_bbu_register_handler(const char *name, char *devicefile,
+   unsigned long flags)
+{
+   struct bbu_handler *handler;
+   int ret = 0;
+
+   if (!name || !devicefile)
+   return -EINVAL;
+
+   handler = xzalloc(sizeof(*handler));
+   handler->name = name;
+   handler->devicefile = devicefile;
+   handler->flags = flags;
+   handler->handler = zynqmp_bbu_handler;
+
+   ret = bbu_register_handler(handler);
+   if (ret)
+   free(handler);
+
+   return ret;
+}
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/7] ARM: zynqmp: zcu104: register update handler

[Michael Tretter]

As the zcu104 stores the environment in the same partition as the
barebox image, the partition is already mounted in ENV_MNT_DIR
("/boot"). Therefore, the update handler has to use the already
existing mount point for the update.

Signed-off-by: Michael Tretter 
---
 arch/arm/boards/xilinx-zcu104/Makefile |  1 +
 arch/arm/boards/xilinx-zcu104/board.c  | 18 ++
 2 files changed, 19 insertions(+)
 create mode 100644 arch/arm/boards/xilinx-zcu104/board.c

diff --git a/arch/arm/boards/xilinx-zcu104/Makefile 
b/arch/arm/boards/xilinx-zcu104/Makefile
index 884d6e63b019..297f77d57ab1 100644
--- a/arch/arm/boards/xilinx-zcu104/Makefile
+++ b/arch/arm/boards/xilinx-zcu104/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
+obj-y += board.o
 lwl-y += lowlevel.o lowlevel_init.o
diff --git a/arch/arm/boards/xilinx-zcu104/board.c 
b/arch/arm/boards/xilinx-zcu104/board.c
new file mode 100644
index ..7654d2bfac90
--- /dev/null
+++ b/arch/arm/boards/xilinx-zcu104/board.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 Michael Tretter 
+ */
+
+#include 
+#include 
+#include 
+
+static int zcu104_register_update_handler(void)
+{
+   if (!of_machine_is_compatible("xlnx,zynqmp-zcu104"))
+   return 0;
+
+   return zynqmp_bbu_register_handler("SD", "/boot/BOOT.BIN",
+  BBU_HANDLER_FLAG_DEFAULT);
+}
+device_initcall(zcu104_register_update_handler);
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/7] ARM: zynqmp: set reset source

[Michael Tretter]

The reset reason is available in the APB register set on the ZynqMP.
Read the reset reason and set the reset source accordingly.

There might be multiple bits set in the APB register. Use the MSB for
determining the actual reset source.

Signed-off-by: Michael Tretter 
---
 arch/arm/mach-zynqmp/Makefile  |  1 +
 arch/arm/mach-zynqmp/include/mach/zynqmp.h |  6 ++
 arch/arm/mach-zynqmp/zynqmp.c  | 74 ++
 3 files changed, 81 insertions(+)
 create mode 100644 arch/arm/mach-zynqmp/include/mach/zynqmp.h
 create mode 100644 arch/arm/mach-zynqmp/zynqmp.c

diff --git a/arch/arm/mach-zynqmp/Makefile b/arch/arm/mach-zynqmp/Makefile
index 021efc94afaf..14b8a4e46b87 100644
--- a/arch/arm/mach-zynqmp/Makefile
+++ b/arch/arm/mach-zynqmp/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 obj-y += firmware-zynqmp.o
+obj-y += zynqmp.o
diff --git a/arch/arm/mach-zynqmp/include/mach/zynqmp.h 
b/arch/arm/mach-zynqmp/include/mach/zynqmp.h
new file mode 100644
index ..f6c05f35a470
--- /dev/null
+++ b/arch/arm/mach-zynqmp/include/mach/zynqmp.h
@@ -0,0 +1,6 @@
+#ifndef __MACH_ZYNQMP_H
+#define __MACH_ZYNQMP_H
+
+int zynqmp_soc_revision(void);
+
+#endif /* __MACH_ZYNQMP_H */
diff --git a/arch/arm/mach-zynqmp/zynqmp.c b/arch/arm/mach-zynqmp/zynqmp.c
new file mode 100644
index ..2b3bd8406ce9
--- /dev/null
+++ b/arch/arm/mach-zynqmp/zynqmp.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 Michael Tretter 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define ZYNQMP_CRL_APB_BASE0xff5e
+#define ZYNQMP_CRL_APB_RESET_REASON(ZYNQMP_CRL_APB_BASE + 0x220)
+
+/* External POR: The PS_POR_B reset signal pin was asserted. */
+#define ZYNQMP_CRL_APB_RESET_REASON_EXTERNAL   BIT(0)
+/* Internal POR: A system error triggered a POR reset. */
+#define ZYNQMP_CRL_APB_RESET_REASON_INTERNAL   BIT(1)
+/* Internal system reset; A system error triggered a system reset. */
+#define ZYNQMP_CRL_APB_RESET_REASON_PMUBIT(2)
+/* PS-only reset: Write to PMU_GLOBAL.GLOBAL_RESET [PS_ONLY_RST]. */
+#define ZYNQMP_CRL_APB_RESET_REASON_PSONLY BIT(3)
+/* External system reset: The PS_SRST_B reset signal pin was asserted. */
+#define ZYNQMP_CRL_APB_RESET_REASON_SRST   BIT(4)
+/* Software system reset: Write to RESET_CTRL [soft_reset]. */
+#define ZYNQMP_CRL_APB_RESET_REASON_SOFT   BIT(5)
+/* Software debugger reset: Write to BLOCKONLY_RST [debug_only]. */
+#define ZYNQMP_CRL_APB_RESET_REASON_DEBUG_SYS  BIT(6)
+
+struct zynqmp_reset_reason {
+   u32 mask;
+   enum reset_src_type type;
+};
+
+static const struct zynqmp_reset_reason reset_reasons[] = {
+   { ZYNQMP_CRL_APB_RESET_REASON_DEBUG_SYS,RESET_UKWN },
+   { ZYNQMP_CRL_APB_RESET_REASON_SOFT, RESET_RST },
+   { ZYNQMP_CRL_APB_RESET_REASON_SRST, RESET_POR },
+   { ZYNQMP_CRL_APB_RESET_REASON_PSONLY,   RESET_POR },
+   { ZYNQMP_CRL_APB_RESET_REASON_PMU,  RESET_POR },
+   { ZYNQMP_CRL_APB_RESET_REASON_INTERNAL, RESET_POR },
+   { ZYNQMP_CRL_APB_RESET_REASON_EXTERNAL, RESET_POR },
+   { /* sentinel */ }
+};
+
+static enum reset_src_type zynqmp_get_reset_src(void)
+{
+   enum reset_src_type type = RESET_UKWN;
+   unsigned int i;
+   u32 val;
+
+   val = readl(ZYNQMP_CRL_APB_RESET_REASON);
+
+   for (i = 0; i < ARRAY_SIZE(reset_reasons); i++) {
+   if (val & reset_reasons[i].mask) {
+   type = reset_reasons[i].type;
+   break;
+   }
+   }
+
+   pr_info("ZynqMP reset reason %s (ZYNQMP_CRL_APB_RESET_REASON: 
0x%08x)\n",
+   reset_source_to_string(type), val);
+
+   return type;
+}
+
+static int zynqmp_init(void)
+{
+   reset_source_set(zynqmp_get_reset_src());
+
+   return 0;
+}
+postcore_initcall(zynqmp_init);
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/7] dts: zcu104: add Barebox environment

[Michael Tretter]

Use the same partition on the SD-card that is used by the ROM loader to
find the BOOT.BIN (which contains the FSBL and Barebox) to store the
Barebox environment.

Signed-off-by: Michael Tretter 
---
 arch/arm/dts/zynqmp-zcu104-revA.dts | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/dts/zynqmp-zcu104-revA.dts 
b/arch/arm/dts/zynqmp-zcu104-revA.dts
index 8c467ee97045..95b60a6b1d69 100644
--- a/arch/arm/dts/zynqmp-zcu104-revA.dts
+++ b/arch/arm/dts/zynqmp-zcu104-revA.dts
@@ -8,3 +8,13 @@
  */
 
 #include 
+
+/ {
+   chosen {
+   environment {
+   compatible = "barebox,environment";
+   device-path = &sdhci1, "partname:0";
+   file-path = "barebox.env";
+   };
+   };
+};
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/5] mci: arasan: fix most checkpatch warnings

[Michael Tretter]

checkpatch reports many warnings for the arasan driver. Fix most of the
warnings.

I didn't fix the long lines in the wait_on_timeout() calls, because
trying to fix them actually makes things worse by introducing either
unreadable code or multiple additional helper functions, which I don't
consider worth it.

Signed-off-by: Michael Tretter 
---
 drivers/mci/arasan-sdhci.c | 46 ++
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
index 04fce62bf46d..7306dac70b69 100644
--- a/drivers/mci/arasan-sdhci.c
+++ b/drivers/mci/arasan-sdhci.c
@@ -12,20 +12,20 @@
 #define SDHCI_ARASAN_HCAP_CLK_FREQ_MASK0xFF00
 #define SDHCI_ARASAN_HCAP_CLK_FREQ_SHIFT   8
 #define SDHCI_INT_ADMAEBIT(29)
-#define SDHCI_ARASAN_INT_DATA_MASK SDHCI_INT_XFER_COMPLETE | \
+#define SDHCI_ARASAN_INT_DATA_MASK (SDHCI_INT_XFER_COMPLETE | \
SDHCI_INT_DMA | \
SDHCI_INT_SPACE_AVAIL | \
SDHCI_INT_DATA_AVAIL | \
SDHCI_INT_DATA_TIMEOUT | \
SDHCI_INT_DATA_CRC | \
SDHCI_INT_DATA_END_BIT | \
-   SDHCI_INT_ADMAE
+   SDHCI_INT_ADMAE)
 
-#define SDHCI_ARASAN_INT_CMD_MASK  SDHCI_INT_CMD_COMPLETE | \
+#define SDHCI_ARASAN_INT_CMD_MASK  (SDHCI_INT_CMD_COMPLETE | \
SDHCI_INT_TIMEOUT | \
SDHCI_INT_CRC | \
SDHCI_INT_END_BIT | \
-   SDHCI_INT_INDEX
+   SDHCI_INT_INDEX)
 
 #define SDHCI_ARASAN_BUS_WIDTH 4
 #define TIMEOUT_VAL0xE
@@ -39,7 +39,6 @@ struct arasan_sdhci_host {
 #define SDHCI_ARASAN_QUIRK_NO_1_8_VBIT(1)
 };
 
-
 static inline
 struct arasan_sdhci_host *to_arasan_sdhci_host(struct mci_host *mci)
 {
@@ -55,15 +54,21 @@ struct arasan_sdhci_host *sdhci_to_arasan(struct sdhci 
*sdhci)
 static int arasan_sdhci_card_present(struct mci_host *mci)
 {
struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
+   u32 val;
+
+   val = sdhci_read32(&host->sdhci, SDHCI_PRESENT_STATE);
 
-   return !!(sdhci_read32(&host->sdhci, SDHCI_PRESENT_STATE) & 
SDHCI_CARD_DETECT);
+   return !!(val & SDHCI_CARD_DETECT);
 }
 
 static int arasan_sdhci_card_write_protected(struct mci_host *mci)
 {
struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
+   u32 val;
+
+   val = sdhci_read32(&host->sdhci, SDHCI_PRESENT_STATE);
 
-   return !(sdhci_read32(&host->sdhci, SDHCI_PRESENT_STATE) & 
SDHCI_WRITE_PROTECT);
+   return !(val & SDHCI_WRITE_PROTECT);
 }
 
 static int arasan_sdhci_reset(struct arasan_sdhci_host *host, u8 mask)
@@ -72,7 +77,7 @@ static int arasan_sdhci_reset(struct arasan_sdhci_host *host, 
u8 mask)
 
/* wait for reset completion */
if (wait_on_timeout(100 * MSECOND,
-   !(sdhci_read8(&host->sdhci, SDHCI_SOFTWARE_RESET) & 
mask))){
+   !(sdhci_read8(&host->sdhci, SDHCI_SOFTWARE_RESET) & 
mask))) {
dev_err(host->mci.hw_dev, "SDHCI reset timeout\n");
return -ETIMEDOUT;
}
@@ -98,13 +103,12 @@ static int arasan_sdhci_init(struct mci_host *mci, struct 
device_d *dev)
return ret;
 
sdhci_write8(&host->sdhci, SDHCI_POWER_CONTROL,
-   SDHCI_BUS_VOLTAGE_330 | SDHCI_BUS_POWER_EN);
+SDHCI_BUS_VOLTAGE_330 | SDHCI_BUS_POWER_EN);
udelay(400);
 
sdhci_write32(&host->sdhci, SDHCI_INT_ENABLE,
-   SDHCI_ARASAN_INT_DATA_MASK |
-   SDHCI_ARASAN_INT_CMD_MASK);
-   sdhci_write32(&host->sdhci, SDHCI_SIGNAL_ENABLE, 0x00);
+ SDHCI_ARASAN_INT_DATA_MASK | SDHCI_ARASAN_INT_CMD_MASK);
+   sdhci_write32(&host->sdhci, SDHCI_SIGNAL_ENABLE, 0);
 
return 0;
 }
@@ -136,18 +140,21 @@ static int arasan_sdhci_wait_for_done(struct 
arasan_sdhci_host *host, u32 mask)
 {
u64 start = get_time_ns();
u16 stat;
+   u16 error;
 
do {
stat = sdhci_read16(&host->sdhci, SDHCI_INT_NORMAL_STATUS);
if (stat & SDHCI_INT_ERROR) {
+   error = sdhci_read16(&host->sdhci,
+SDHCI_INT_ERROR_STA

[PATCH 0/5] Cleanup and fix arasan-sdhci

[Michael Tretter]

Hi,

These are a few misc patches for the arasan sd controller. Nothing particular
stands out and each patch is useful on its own. I put them into a single
series, because all of them affect the arasan driver.

Michael

Michael Tretter (5):
  mci: mci-core: respect disable-wp property
  mci: arasan: fix most checkpatch warnings
  mci: arasan: remove duplicate stop clock
  mci: arasan: wait for XFER_COMPLETE for busy response
  ARM: zynqmp: defconfig: enable MCI_ARASAN

 arch/arm/configs/zynqmp_defconfig |  2 ++
 drivers/mci/arasan-sdhci.c| 51 ++-
 drivers/mci/mci-core.c|  4 ++-
 include/mci.h |  1 +
 4 files changed, 35 insertions(+), 23 deletions(-)

-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/5] ARM: zynqmp: defconfig: enable MCI_ARASAN

[Michael Tretter]

The ZynqMP has an arasan SD controller. Enable it in the respective
defconfig.

Signed-off-by: Michael Tretter 
---
 arch/arm/configs/zynqmp_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/zynqmp_defconfig 
b/arch/arm/configs/zynqmp_defconfig
index 762103c54105..6f5612fa92a0 100644
--- a/arch/arm/configs/zynqmp_defconfig
+++ b/arch/arm/configs/zynqmp_defconfig
@@ -37,5 +37,7 @@ CONFIG_NET=y
 CONFIG_DRIVER_SERIAL_CADENCE=y
 CONFIG_DRIVER_NET_MACB=y
 # CONFIG_SPI is not set
+CONFIG_MCI=y
+CONFIG_MCI_ARASAN=y
 CONFIG_FIRMWARE_ZYNQMP_FPGA=y
 CONFIG_DIGEST=y
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/5] mci: arasan: remove duplicate stop clock

[Michael Tretter]

The clock is already stopped in sdhci_set_clock(). Stopping the clock in
the arasan driver is not necessary.

Signed-off-by: Michael Tretter 
---
 drivers/mci/arasan-sdhci.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
index 7306dac70b69..732f838d8395 100644
--- a/drivers/mci/arasan-sdhci.c
+++ b/drivers/mci/arasan-sdhci.c
@@ -118,9 +118,6 @@ static void arasan_sdhci_set_ios(struct mci_host *mci, 
struct mci_ios *ios)
struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
u16 val;
 
-   /* stop clock */
-   sdhci_write16(&host->sdhci, SDHCI_CLOCK_CONTROL, 0);
-
if (ios->clock)
sdhci_set_clock(&host->sdhci, ios->clock, host->sdhci.max_clk);
 
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/5] mci: arasan: wait for XFER_COMPLETE for busy response

[Michael Tretter]

I observed errors on the ZynqMP during reading the EXT_CSD registers
using CMD8. The Zynq UltraScale+ Device TRM UG1085 (v2.2) p. 777 states
that the driver shall wait 2 ms after sending CMD6 for setting a EXT_CSD
register.

The JEDEC Standard No. 84-A43 p. 35 does not specify the delay but
states that CMD6 expects an R1b response and that the host has to wait
until the busy signal is de-asserted. This is signaled via the
SDHCI_INT_XFER_COMPLETE interrupt.

Wait for the XFER_COMPLETE interrupt after sending a command that
expects an R1b response.

Signed-off-by: Michael Tretter 
---
 drivers/mci/arasan-sdhci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
index 732f838d8395..d45f9184cd1d 100644
--- a/drivers/mci/arasan-sdhci.c
+++ b/drivers/mci/arasan-sdhci.c
@@ -193,6 +193,8 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, 
struct mci_cmd *cmd,
mask = SDHCI_INT_CMD_COMPLETE;
if (data && data->flags == MMC_DATA_READ)
mask |= SDHCI_INT_DATA_AVAIL;
+   if (cmd->resp_type & MMC_RSP_BUSY)
+   mask |= SDHCI_INT_XFER_COMPLETE;
 
sdhci_set_cmd_xfer_mode(&host->sdhci,
cmd, data, false, &command, &xfer);
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/5] mci: mci-core: respect disable-wp property

[Michael Tretter]

Systems without write-protect pin should ignore the write protect logic
and assume that an SD card is always read-write. This is expressed by
the disable-wp dt property.

Respect the disable-wp property and don't call the write protection
check in these cases.

Signed-off-by: Michael Tretter 
---
 drivers/mci/mci-core.c | 4 +++-
 include/mci.h  | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index a160b9889459..a094f3cbf522 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -1358,7 +1358,8 @@ static int __maybe_unused mci_sd_write(struct 
block_device *blk,
 
mci_blk_part_switch(part);
 
-   if (host->card_write_protected && host->card_write_protected(host)) {
+   if (!host->disable_wp &&
+   host->card_write_protected && host->card_write_protected(host)) {
dev_err(&mci->dev, "card write protected\n");
return -EPERM;
}
@@ -2016,6 +2017,7 @@ void mci_of_parse_node(struct mci_host *host,
 
host->non_removable = of_property_read_bool(np, "non-removable");
host->no_sd = of_property_read_bool(np, "no-sd");
+   host->disable_wp = of_property_read_bool(np, "disable-wp");
 }
 
 void mci_of_parse(struct mci_host *host)
diff --git a/include/mci.h b/include/mci.h
index df2437f6181b..922aeaecf3de 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -406,6 +406,7 @@ struct mci_host {
int use_dsr;/**< optional dsr usage flag */
bool non_removable; /**< device is non removable */
bool no_sd; /**< do not send SD commands during 
initialization */
+   bool disable_wp;/**< ignore write-protect detection logic */
struct regulator *supply;
 
/** init the host interface */
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] mci: arasan: wait for data available only on read

[Michael Tretter]

Only READ data transfers actually send a data available interrupt.
Therefore, check if the transfer is a read and wait for the data only in
this case.

Signed-off-by: Michael Tretter 
---

Hi Michael,

On Tue, 18 May 2021 08:09:47 +, Michael Graichen wrote:
> > This looks familiar. I have a similar patch that sets SDHCI_INT_DATA_AVAIL
> > only if the command is a READ, but I didn't yet have time to verify, that 
> > this
> > is the correct fix.
>
> Can you please send it to me?

Here you are. I am not sure, if the fix is correct. I also added another
patch to handle situations where data is NULL, but I am not entirely sure
about that one either.

Michael
---
 drivers/mci/arasan-sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
index 520bf30ff952..399966e8cf10 100644
--- a/drivers/mci/arasan-sdhci.c
+++ b/drivers/mci/arasan-sdhci.c
@@ -277,7 +277,7 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, 
struct mci_cmd *cmd,
sdhci_write32(&host->sdhci, SDHCI_INT_STATUS, ~0);
 
mask = SDHCI_INT_CMD_COMPLETE;
-   if (data)
+   if (data && data->flags == MMC_DATA_READ)
mask |= SDHCI_INT_DATA_AVAIL;
 
sdhci_set_cmd_xfer_mode(&host->sdhci, cmd, data, false, &command, 
&xfer);
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] mci: arasan: configure data transfer only if we actually have data

[Michael Tretter]

If we don't have any data to transfer, we must not set the block size
and block count.

If data is NULL, accessing data to get the block size and block count is
a NULL pointer dereference.

Signed-off-by: Michael Tretter 
---
 drivers/mci/arasan-sdhci.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
index 399966e8cf10..3d738774e825 100644
--- a/drivers/mci/arasan-sdhci.c
+++ b/drivers/mci/arasan-sdhci.c
@@ -283,10 +283,12 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, 
struct mci_cmd *cmd,
sdhci_set_cmd_xfer_mode(&host->sdhci, cmd, data, false, &command, 
&xfer);
 
sdhci_write8(&host->sdhci, SDHCI_TIMEOUT_CONTROL, TIMEOUT_VAL);
-   sdhci_write16(&host->sdhci, SDHCI_TRANSFER_MODE, xfer);
-   sdhci_write16(&host->sdhci, SDHCI_BLOCK_SIZE, SDHCI_DMA_BOUNDARY_512K |
-   SDHCI_TRANSFER_BLOCK_SIZE(data->blocksize));
-   sdhci_write16(&host->sdhci, SDHCI_BLOCK_COUNT, data->blocks);
+   if (data) {
+   sdhci_write16(&host->sdhci, SDHCI_TRANSFER_MODE, xfer);
+   sdhci_write16(&host->sdhci, SDHCI_BLOCK_SIZE,
+ SDHCI_DMA_BOUNDARY_512K | 
SDHCI_TRANSFER_BLOCK_SIZE(data->blocksize));
+   sdhci_write16(&host->sdhci, SDHCI_BLOCK_COUNT, data->blocks);
+   }
sdhci_write32(&host->sdhci, SDHCI_ARGUMENT, cmd->cmdarg);
sdhci_write16(&host->sdhci, SDHCI_COMMAND, command);
 
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] Zynq: add support to chainload another barebox

[Michael Tretter]

On Wed, 05 May 2021 09:40:29 +, Michael Graichen wrote:
[...]
> I'm currently booting from SDcard and i have not realised any problems with
> the FAT support so far. But with the arasan-sdhci driver when writing, see
> my patch below which fixes it for me but I haven't dived into the drivers
> very deeply yet.
> 
> best regards
> Michael
> 
> 
>  drivers/mci/arasan-sdhci.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
> index 520bf30ff..669675369 100644
> --- a/drivers/mci/arasan-sdhci.c
> +++ b/drivers/mci/arasan-sdhci.c
> @@ -265,7 +265,7 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, 
> struct mci_cmd *cmd,
>   if (cmd->cmdidx != MMC_CMD_STOP_TRANSMISSION)
>   mask |= SDHCI_CMD_INHIBIT_DATA;
>  
> - ret = wait_on_timeout(10 * MSECOND,
> + ret = wait_on_timeout(100 * MSECOND,
>   !(sdhci_read32(&host->sdhci, SDHCI_PRESENT_STATE) & 
> mask));
>  
>   if (ret) {
> @@ -277,8 +277,6 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, 
> struct mci_cmd *cmd,
>   sdhci_write32(&host->sdhci, SDHCI_INT_STATUS, ~0);
>  
>   mask = SDHCI_INT_CMD_COMPLETE;
> - if (data)
> - mask |= SDHCI_INT_DATA_AVAIL;

This looks familiar. I have a similar patch that sets SDHCI_INT_DATA_AVAIL
only if the command is a READ, but I didn't yet have time to verify, that this
is the correct fix.

Michael

>  
>   sdhci_set_cmd_xfer_mode(&host->sdhci, cmd, data, false, &command, 
> &xfer);

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] added support for zynq7000-fpga-manager

[Michael Tretter]

Hi Michael,

On Tue, 30 Mar 2021 12:05:31 +, Michael Graichen wrote:
> > I think, it would be fine to use only "zynq" instead of zynqmp for the
> > firmware loader/fpga manager. (I didn't compare the Zynq7000 and ZynqMP low
> > level interfaces for programming the FPGA, yet, but I guess that programming
> > the FPGA on the ZynqMP in EL3 instead of EL2/EL1 is actually the same as on
> > Zynq7000.) I am also ok with treating ZynqMP as a second class citizen in 
> > the
> > driver. 
> 
> As I compared
> https://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
> page 211ff and
> https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
> page 261ff the programming of the PCAP seem to be similar, but PCAP is
> embedded in CSU/PMU on ZynqMP, which looks much more complex then DEVC on
> Zynq7000. But I don't know how much of CSUs functionality is acctually
> needed to programm the FPGA.

The PCAP behave similar and have similar registers and bits, but the register
layouts are completely different. It seems that for programming the FPGA on
the ZynqMP, one would at least have to configure the internal stream switch
and use CSU DMA engine for the image transfer. For encrypted bitstreams, the
AES engine in the CSU has to be used as well.

> 
> Is the CSU within ZynqMP used for something other than programming the
> firmware in barebox?

At the moment, Barebox doesn't directly use the CSU. Barebox is not even
allowed to access the CSU register. It uses SMCs via TF-A to instruct the
PMU-FW to program the FPGA. The PMU-FW then talks to the CSU for actually
programming the FPGA.

If Barebox would run as first stage bootloader, it might use the CSU to read
the PS version or might use some of the authentication/encryption features of
the CSU, but this is currently not the case.

Watch out for the overloaded term "firmware". The firmware manager in the
zynqmp-fpga driver in Barebox is able to _load_ a "firmware", in this case the
FPGA bitstream. The firmware-zynqmp driver is a driver to _talk to_ the PMU
firmware, which is a firmware that is already running on the system.

Michael

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] added support for zynq7000-fpga-manager

[Michael Tretter]

On Thu, 25 Mar 2021 07:53:33 +, Michael Graichen wrote:
> i have reworked the ZynqMP FPGA manager to program the FPGA within the 
> Zynq7000. 

Great, thanks.

> 
> Michael Tretter, would you be so kind an test this on your ZynqMP board?
> Since I have no access to a ZynqMP chip I have only tested compiling. 

I will test the patch and I already have a few comments down below.

> 
> The name "ZynqMP" may now be missleading for the firmwareload tool, because
> I intended to use it on both, ZynqMP and Zynq7000.  I would be happy to get
> read your feedback.

I think, it would be fine to use only "zynq" instead of zynqmp for the
firmware loader/fpga manager. (I didn't compare the Zynq7000 and ZynqMP low
level interfaces for programming the FPGA, yet, but I guess that programming
the FPGA on the ZynqMP in EL3 instead of EL2/EL1 is actually the same as on
Zynq7000.) I am also ok with treating ZynqMP as a second class citizen in the
driver.

> 
> Best Regards 
> Michael 
> 
> 
> Signed-off-by: Michael Graichen 
> ---
>  arch/arm/configs/zynq_defconfig   |   2 +
>  arch/arm/mach-zynq/Makefile   |   2 +-
>  arch/arm/mach-zynq/firmware-zynq.c| 124 ++
>  .../mach-zynq/include/mach/firmware-zynq.h|  85 
>  .../include/mach/firmware-zynqmp.h|  46 +++
>  drivers/firmware/Kconfig  |   7 +
>  drivers/firmware/Makefile |   1 +
>  drivers/firmware/zynqmp-fpga.c| 115 
>  8 files changed, 324 insertions(+), 58 deletions(-)
>  create mode 100644 arch/arm/mach-zynq/firmware-zynq.c
>  create mode 100644 arch/arm/mach-zynq/include/mach/firmware-zynq.h
> 
> diff --git a/arch/arm/configs/zynq_defconfig b/arch/arm/configs/zynq_defconfig
> index a16c57d5c..82ea899e2 100644
> --- a/arch/arm/configs/zynq_defconfig
> +++ b/arch/arm/configs/zynq_defconfig
> @@ -36,6 +36,7 @@ CONFIG_CMD_MENU_MANAGEMENT=y
>  CONFIG_CMD_READLINE=y
>  CONFIG_CMD_TIMEOUT=y
>  CONFIG_CMD_CLK=y
> +CONFIG_CMD_FIRMWARELOAD=y
>  CONFIG_CMD_OFTREE=y
>  CONFIG_CMD_TIME=y
>  CONFIG_NET=y
> @@ -43,5 +44,6 @@ CONFIG_DRIVER_SERIAL_CADENCE=y
>  CONFIG_DRIVER_NET_MACB=y
>  # CONFIG_SPI is not set
>  # CONFIG_PINCTRL is not set
> +CONFIG_FIRMWARE_ZYNQ7000_FPGA=y
>  CONFIG_FS_TFTP=y
>  CONFIG_DIGEST=y
> diff --git a/arch/arm/mach-zynq/Makefile b/arch/arm/mach-zynq/Makefile
> index 06c2ce996..2484abe5c 100644
> --- a/arch/arm/mach-zynq/Makefile
> +++ b/arch/arm/mach-zynq/Makefile
> @@ -1,2 +1,2 @@
> -obj-y += zynq.o bootm-zynqimg.o
> +obj-y += bootm-zynqimg.o firmware-zynq.o zynq.o
>  lwl-y += cpu_init.o
> diff --git a/arch/arm/mach-zynq/firmware-zynq.c 
> b/arch/arm/mach-zynq/firmware-zynq.c
> new file mode 100644
> index 0..307b22fe5
> --- /dev/null
> +++ b/arch/arm/mach-zynq/firmware-zynq.c
> @@ -0,0 +1,124 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * zynq_devc_fpga_load - Perform the fpga load
> + * @mgr: FPGA-Manager
> + * @address: Address to write to
> + * @size:PL bitstream size
> + * @flags:   Flags - unused
> + *
> + * This function provides access to PCAP to transfer
> + * the required bitstream into PL.
> + *
> + * Return:   Returns status, either success or error+reason
> + */
> +static int zynq_devc_fpga_load(struct fpgamgr *mgr, u64 address,
> + u32 size, u32 flags)
> +{
> + unsigned long reg;
> +
> + if (!address || !size)
> + return -EINVAL;
> +
> + /*
> +  * The Programming Seqenze, see ug585 (v.12.2) Juny 1, 2018 Chapter
> +  * 6.4.2 on page 211 Configure the PL via PCAP Bridge Example for
> +  * detailed information to this Sequenze
> +  */
> +
> + /* Enable the PCAP bridge and select PCAP for reconfiguration */
> + reg = readl(mgr->regs + CTRL_OFFSET);
> + reg |= ( CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK );
> + writel(reg, mgr->regs + CTRL_OFFSET);
> +
> + /* Clear the Interrupts */
> + writel(0x, mgr->regs + INT_STS_OFFSET);
> +
> + /* Initialize the PL */
> + reg = readl(mgr->regs + CTRL_OFFSET);
> + reg |= CTRL_PCFG_PROG_B_MASK;
> + writel(reg, mgr->regs + CTRL_OFFSET);
> +
> + reg = readl(mgr->regs + CTRL_OFFSET);
> + reg &= ~CTRL_PCFG_PROG_B_MASK;
> + writel(reg, mgr->regs + CTRL_OFFSET);
> +
> + readl_poll_timeout(mgr->regs + STATUS_OFFSET, reg,
> + !(reg & STATUS_PCFG_INIT_MASK), 100 * USEC_PER_MSEC);
> +
> + reg = readl(mgr->regs + 

[PATCH] net: macb: add tx clock rate for 10 MBit link

[Michael Tretter]

If the phy reports a 10 MBit link, which can happen during link
negotiation, the macb prints a warning, because it does not know the
clock rate for the TX clock.

Implement setting the TX clock rate for 10 MBit to avoid the warnings.

Signed-off-by: Michael Tretter 
---
 drivers/net/macb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index 188dbf2d8c15..14a0b45322bf 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -269,6 +269,9 @@ static int macb_set_tx_clk(struct macb_device *macb, int 
speed)
}
 
switch (speed) {
+   case SPEED_10:
+   rate = 250;
+   break;
case SPEED_100:
rate = 2500;
break;
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 6/7] net: phy: dp83867: read status from PHY status register PHYSTS

[Michael Tretter]

From: Thomas Haemmerle 

Read link status information in dp83867 specific register instead of
using `genphy_read_status()`.

In case of a link downshift, `genphy_read_status()` can return
`SPEED_1000` even if the negotiated speed is 100 Mbit.

A downshift can happen, if both link-partners are gigabit-capable, but
the connection supports only 100 MBit, for example because of a 100 MBit
PoE injector that connects only 2 of the 4 twisted pairs.

Signed-off-by: Thomas Haemmerle 
Signed-off-by: Michael Tretter 
---
 drivers/net/phy/dp83867.c | 36 
 1 file changed, 36 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index dd769b4d3e38..c19f6ecba267 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -16,6 +16,7 @@
 #define DP83867_DEVADDRMDIO_MMD_VEND2
 
 #define MII_DP83867_PHYCTRL0x10
+#define MII_DP83867_PHYSTS 0x11
 #define MII_DP83867_MICR   0x12
 #define MII_DP83867_ISR0x13
 #define MII_DP83867_CFG2   0x14
@@ -65,6 +66,11 @@
 #define DP83867_PHYCTRL_TXFIFO_SHIFT   14
 #define DP83867_PHYCR_RESERVED_MASKBIT(11)
 
+/* PHY STS bits */
+#define DP83867_PHYSTS_SPEED_1000  BIT(15)
+#define DP83867_PHYSTS_SPEED_100   BIT(14)
+#define DP83867_PHYSTS_DUPLEX_FULL BIT(13)
+
 /* RGMIIDCTL bits */
 #define DP83867_RGMII_TX_CLK_DELAY_SHIFT   4
 
@@ -109,6 +115,35 @@ struct dp83867_private {
bool rxctrl_strap_quirk;
 };
 
+static int dp83867_read_status(struct phy_device *phydev)
+{
+   int status;
+   int ret;
+
+   ret = genphy_update_link(phydev);
+   if (ret)
+   return ret;
+
+   status = phy_read(phydev, MII_DP83867_PHYSTS);
+   if (status < 0)
+   return status;
+
+   phydev->speed = SPEED_10;
+   phydev->duplex = DUPLEX_HALF;
+
+   if (status & DP83867_PHYSTS_SPEED_1000)
+   phydev->speed = SPEED_1000;
+   else if (status & DP83867_PHYSTS_SPEED_100)
+   phydev->speed = SPEED_100;
+
+   if (status & DP83867_PHYSTS_DUPLEX_FULL)
+   phydev->duplex = DUPLEX_FULL;
+
+   phydev->pause = phydev->asym_pause = 0;
+
+   return 0;
+}
+
 static int dp83867_config_port_mirroring(struct phy_device *phydev)
 {
struct dp83867_private *dp83867 = phydev->priv;
@@ -279,6 +314,7 @@ static struct phy_driver dp83867_driver[] = {
.drv.name = "TI DP83867",
.features = PHY_GBIT_FEATURES,
.config_init = dp83867_config_init,
+   .read_status = dp83867_read_status,
},
 };
 
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 7/7] net: phy: dp83867: enable link downshift by default

[Michael Tretter]

From: Thomas Haemmerle 

Linux enables link downshift by default. Use the same bit definitions as
Linux and enable downshift in Barebox, as well.

Signed-off-by: Thomas Haemmerle 
Signed-off-by: Michael Tretter 
---
 drivers/net/phy/dp83867.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index c19f6ecba267..05af83f076a8 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -75,12 +75,12 @@
 #define DP83867_RGMII_TX_CLK_DELAY_SHIFT   4
 
 /* CFG2 bits */
-#define MII_DP83867_CFG2_SPEEDOPT_10EN 0x0040
-#define MII_DP83867_CFG2_SGMII_AUTONEGEN   0x0080
-#define MII_DP83867_CFG2_SPEEDOPT_ENH  0x0100
-#define MII_DP83867_CFG2_SPEEDOPT_CNT  0x0800
-#define MII_DP83867_CFG2_SPEEDOPT_INTLOW   0x2000
-#define MII_DP83867_CFG2_MASK  0x003F
+#define DP83867_DOWNSHIFT_EN   (BIT(8) | BIT(9))
+#define DP83867_DOWNSHIFT_ATTEMPT_MASK (BIT(10) | BIT(11))
+#define DP83867_DOWNSHIFT_1_COUNT_VAL  0
+#define DP83867_DOWNSHIFT_2_COUNT_VAL  1
+#define DP83867_DOWNSHIFT_4_COUNT_VAL  2
+#define DP83867_DOWNSHIFT_8_COUNT_VAL  3
 
 /* CFG4 bits */
 #define DP83867_CFG4_SGMII_AUTONEG_TIMER_MASK  0x60
@@ -301,6 +301,10 @@ static int dp83867_config_init(struct phy_device *phydev)
phy_write(phydev, MII_DP83867_BISCR, 0x0);
}
 
+   val = phy_read(phydev, MII_DP83867_CFG2);
+   val |= DP83867_DOWNSHIFT_EN;
+   phy_write(phydev, MII_DP83867_CFG2, val);
+
if (dp83867->port_mirroring != DP83867_PORT_MIRROING_KEEP)
dp83867_config_port_mirroring(phydev);
 
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/7] net: phy: dp83867: remove explicit setting of cfg2

[Michael Tretter]

From: Thomas Haemmerle 

Setting cfg2 is superfluous, because it just sets the default value.
Remove it.

Signed-off-by: Thomas Haemmerle 
Signed-off-by: Michael Tretter 
---
 drivers/net/phy/dp83867.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index e796498c4a18..dd769b4d3e38 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -180,7 +180,7 @@ static int dp83867_config_init(struct phy_device *phydev)
 {
struct dp83867_private *dp83867;
int ret;
-   u16 val, delay, cfg2;
+   u16 val, delay;
 
if (!phydev->priv) {
dp83867 = kzalloc(sizeof(*dp83867), GFP_KERNEL);
@@ -254,15 +254,6 @@ static int dp83867_config_init(struct phy_device *phydev)
phy_write(phydev, MII_BMCR,
  BMCR_ANENABLE | BMCR_FULLDPLX | BMCR_SPEED1000);
 
-   cfg2 = phy_read(phydev, MII_DP83867_CFG2);
-   cfg2 &= MII_DP83867_CFG2_MASK;
-   cfg2 |= MII_DP83867_CFG2_SPEEDOPT_10EN |
-   MII_DP83867_CFG2_SGMII_AUTONEGEN |
-   MII_DP83867_CFG2_SPEEDOPT_ENH |
-   MII_DP83867_CFG2_SPEEDOPT_CNT |
-   MII_DP83867_CFG2_SPEEDOPT_INTLOW;
-   phy_write(phydev, MII_DP83867_CFG2, cfg2);
-
phy_write_mmd_indirect(phydev, DP83867_RGMIICTL,
   DP83867_DEVADDR, 0x0);
 
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/7] net: phy: dp83867: remove useless call to genphy_config_aneg

[Michael Tretter]

genphy_config_aneg will be called by the generic code anyway. No need to
call it from the driver.

As at it, do not explicitly set the config_aneg and read_status
pointers, because they will be automatically set anyway.

Remove the "hello world" message by the driver, too.

Signed-off-by: Michael Tretter 
---
 drivers/net/phy/dp83867.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index deee7e3ae7ca..8a14927071b9 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -278,13 +278,9 @@ static int dp83867_config_init(struct phy_device *phydev)
}
}
 
-   genphy_config_aneg(phydev);
-
if (dp83867->port_mirroring != DP83867_PORT_MIRROING_KEEP)
dp83867_config_port_mirroring(phydev);
 
-   dev_info(&phydev->dev, "DP83867\n");
-
return 0;
 }
 
@@ -294,11 +290,7 @@ static struct phy_driver dp83867_driver[] = {
.phy_id_mask = 0xfff0,
.drv.name = "TI DP83867",
.features = PHY_GBIT_FEATURES,
-
.config_init = dp83867_config_init,
-
-   .config_aneg = genphy_config_aneg,
-   .read_status = genphy_read_status,
},
 };
 
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/7] net: phy: dp83867: convert driver to spdx

[Michael Tretter]

Signed-off-by: Michael Tretter 
---
 drivers/net/phy/dp83867.c | 10 +-
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 8131e8c9d6c6..153b60def320 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -1,16 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Driver for the Texas Instruments DP83867 PHY
  *
  * Copyright (C) 2015 Texas Instruments Inc.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
  */
 
 #include 
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/7] net: phy: dp83867: simplify dp83867_config_init

[Michael Tretter]

From: Thomas Haemmerle 

Reorder the code in dp83867_config_init to remove duplicated check of
the phy interface.

Signed-off-by: Thomas Haemmerle 
Signed-off-by: Michael Tretter 
---
 drivers/net/phy/dp83867.c | 49 ++-
 1 file changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 8a14927071b9..e796498c4a18 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -213,33 +213,7 @@ static int dp83867_config_init(struct phy_device *phydev)
ret = phy_write(phydev, MII_DP83867_PHYCTRL, val);
if (ret)
return ret;
-   } else if (phy_interface_is_sgmii(phydev)) {
-   phy_write(phydev, MII_BMCR,
- BMCR_ANENABLE | BMCR_FULLDPLX | BMCR_SPEED1000);
-
-   cfg2 = phy_read(phydev, MII_DP83867_CFG2);
-   cfg2 &= MII_DP83867_CFG2_MASK;
-   cfg2 |= MII_DP83867_CFG2_SPEEDOPT_10EN |
-   MII_DP83867_CFG2_SGMII_AUTONEGEN |
-   MII_DP83867_CFG2_SPEEDOPT_ENH |
-   MII_DP83867_CFG2_SPEEDOPT_CNT |
-   MII_DP83867_CFG2_SPEEDOPT_INTLOW;
-
-   phy_write(phydev, MII_DP83867_CFG2, cfg2);
-
-   phy_write_mmd_indirect(phydev, DP83867_RGMIICTL,
-  DP83867_DEVADDR, 0x0);
-
-   val = DP83867_PHYCTRL_SGMIIEN |
- DP83867_MDI_CROSSOVER_MDIX << DP83867_MDI_CROSSOVER |
- dp83867->fifo_depth << DP83867_PHYCTRL_RXFIFO_SHIFT |
- dp83867->fifo_depth << DP83867_PHYCTRL_TXFIFO_SHIFT;
-
-   phy_write(phydev, MII_DP83867_PHYCTRL, val);
-   phy_write(phydev, MII_DP83867_BISCR, 0x0);
-   }
 
-   if (phy_interface_is_rgmii(phydev)) {
val = phy_read_mmd_indirect(phydev, DP83867_RGMIICTL,
DP83867_DEVADDR);
 
@@ -276,6 +250,29 @@ static int dp83867_config_init(struct phy_device *phydev)
phy_write_mmd_indirect(phydev, DP83867_IO_MUX_CFG,
   DP83867_DEVADDR, val);
}
+   } else if (phy_interface_is_sgmii(phydev)) {
+   phy_write(phydev, MII_BMCR,
+ BMCR_ANENABLE | BMCR_FULLDPLX | BMCR_SPEED1000);
+
+   cfg2 = phy_read(phydev, MII_DP83867_CFG2);
+   cfg2 &= MII_DP83867_CFG2_MASK;
+   cfg2 |= MII_DP83867_CFG2_SPEEDOPT_10EN |
+   MII_DP83867_CFG2_SGMII_AUTONEGEN |
+   MII_DP83867_CFG2_SPEEDOPT_ENH |
+   MII_DP83867_CFG2_SPEEDOPT_CNT |
+   MII_DP83867_CFG2_SPEEDOPT_INTLOW;
+   phy_write(phydev, MII_DP83867_CFG2, cfg2);
+
+   phy_write_mmd_indirect(phydev, DP83867_RGMIICTL,
+  DP83867_DEVADDR, 0x0);
+
+   val = DP83867_PHYCTRL_SGMIIEN |
+ DP83867_MDI_CROSSOVER_MDIX << DP83867_MDI_CROSSOVER |
+ dp83867->fifo_depth << DP83867_PHYCTRL_RXFIFO_SHIFT |
+ dp83867->fifo_depth << DP83867_PHYCTRL_TXFIFO_SHIFT;
+
+   phy_write(phydev, MII_DP83867_PHYCTRL, val);
+   phy_write(phydev, MII_DP83867_BISCR, 0x0);
}
 
if (dp83867->port_mirroring != DP83867_PORT_MIRROING_KEEP)
-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/7] net: phy: dp83867: fix checkpatch checks and warnings

[Michael Tretter]

Mostly indentation changes to make checkpatch happy and make the code a
bit easier to the eyes.

Signed-off-by: Michael Tretter 
---
 drivers/net/phy/dp83867.c | 75 +++
 1 file changed, 37 insertions(+), 38 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 153b60def320..deee7e3ae7ca 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -111,7 +111,7 @@ struct dp83867_private {
 
 static int dp83867_config_port_mirroring(struct phy_device *phydev)
 {
-   struct dp83867_private *dp83867 = (struct dp83867_private 
*)phydev->priv;
+   struct dp83867_private *dp83867 = phydev->priv;
u16 val;
 
val = phy_read_mmd_indirect(phydev, DP83867_CFG4, DP83867_DEVADDR);
@@ -146,18 +146,18 @@ static int dp83867_of_init(struct phy_device *phydev)
 
dp83867->rxctrl_strap_quirk =
of_property_read_bool(of_node,
-   "ti,dp83867-rxctrl-strap-quirk");
+ "ti,dp83867-rxctrl-strap-quirk");
 
ret = of_property_read_u32(of_node, "ti,rx-internal-delay",
-   &dp83867->rx_id_delay);
+  &dp83867->rx_id_delay);
if (ret && (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-   phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID))
+   phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID))
return ret;
 
ret = of_property_read_u32(of_node, "ti,tx-internal-delay",
-   &dp83867->tx_id_delay);
+  &dp83867->tx_id_delay);
if (ret && (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-   phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID))
+   phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID))
return ret;
 
if (of_property_read_bool(of_node, "enet-phy-lane-swap"))
@@ -167,13 +167,13 @@ static int dp83867_of_init(struct phy_device *phydev)
dp83867->port_mirroring = DP83867_PORT_MIRROING_DIS;
 
return of_property_read_u32(of_node, "ti,fifo-depth",
-   &dp83867->fifo_depth);
+   &dp83867->fifo_depth);
 }
 
 static inline bool phy_interface_is_sgmii(struct phy_device *phydev)
 {
return phydev->interface == PHY_INTERFACE_MODE_SGMII ||
-   phydev->interface == PHY_INTERFACE_MODE_QSGMII;
+  phydev->interface == PHY_INTERFACE_MODE_QSGMII;
 }
 
 static int dp83867_config_init(struct phy_device *phydev)
@@ -192,7 +192,7 @@ static int dp83867_config_init(struct phy_device *phydev)
if (ret)
return ret;
} else {
-   dp83867 = (struct dp83867_private *)phydev->priv;
+   dp83867 = phydev->priv;
}
 
/* Restart the PHY.  */
@@ -201,22 +201,21 @@ static int dp83867_config_init(struct phy_device *phydev)
 
if (dp83867->rxctrl_strap_quirk) {
val = phy_read_mmd_indirect(phydev, DP83867_CFG4,
-   DP83867_DEVADDR);
+   DP83867_DEVADDR);
val &= ~BIT(7);
-   phy_write_mmd_indirect(phydev, DP83867_CFG4, DP83867_DEVADDR,
-   val);
+   phy_write_mmd_indirect(phydev, DP83867_CFG4,
+  DP83867_DEVADDR, val);
}
 
if (phy_interface_is_rgmii(phydev)) {
val = DP83867_MDI_CROSSOVER_AUTO << DP83867_MDI_CROSSOVER |
-   dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT;
+ dp83867->fifo_depth << DP83867_PHYCR_FIFO_DEPTH_SHIFT;
ret = phy_write(phydev, MII_DP83867_PHYCTRL, val);
if (ret)
return ret;
} else if (phy_interface_is_sgmii(phydev)) {
-   phy_write(phydev, MII_BMCR, BMCR_ANENABLE |
-   BMCR_FULLDPLX |
-   BMCR_SPEED1000);
+   phy_write(phydev, MII_BMCR,
+ BMCR_ANENABLE | BMCR_FULLDPLX | BMCR_SPEED1000);
 
cfg2 = phy_read(phydev, MII_DP83867_CFG2);
cfg2 &= MII_DP83867_CFG2_MASK;
@@ -229,12 +228,12 @@ static int dp83867_config_init(struct phy_device *phydev)
phy_write(phydev, MII_DP83867_CFG2, cfg2);
 
phy_write_mmd_indirect(phydev, DP83867_RGMIICTL,
-   DP83867_DEVADDR, 0x0);
+  DP83867_DEVADDR, 0x0);
 
val = DP83867_PHYCTRL_SGMIIEN |
-

[PATCH 0/7] net: phy: dp83867: cleanup and link downshift support

[Michael Tretter]

Hello,

the dp83867 network phy supports link downshift, if a gigabit link cannot be
established. For example, this can happen if the cabling does not support
gigabit Ethernet.

This series enables the downshift by default (Linux also enables it by
default). Additionally, the link status is read from the dp83867-specific
register to take the possible downshift into account when reporting the link
speed.

Furthermore, the series contains a handful of cleanup patches.

Michael

Michael Tretter (3):
  net: phy: dp83867: convert driver to spdx
  net: phy: dp83867: fix checkpatch checks and warnings
  net: phy: dp83867: remove useless call to genphy_config_aneg

Thomas Haemmerle (4):
  net: phy: dp83867: simplify dp83867_config_init
  net: phy: dp83867: remove explicit setting of cfg2
  net: phy: dp83867: read status from PHY status register PHYSTS
  net: phy: dp83867: enable link downshift by default

 drivers/net/phy/dp83867.c | 169 --
 1 file changed, 90 insertions(+), 79 deletions(-)

-- 
2.29.2


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH] dts: zcu104: remove unnecessary ZynqMP clock dtsi

[Michael Tretter]

The clock definitions are now available from the Linux device tree as
dts/src/arm64/xilinx/zynqmp-clk-ccf.dtsi and the clock-controller is
defined in dts/src/arm64/xilinx/zynqmp.dtsi. Defining the clocks in a
Barebox-specific file is not necessary anymore.

Remove zynqmp-clk.dtsi and the include from the board device tree.

The files are already correctly included by the imported board device
trees (dts/src/arm64/xilinx/zynqmp-zcu104-revA.dts).

Signed-off-by: Michael Tretter 
---
 arch/arm/dts/zynqmp-clk.dtsi| 155 
 arch/arm/dts/zynqmp-zcu104-revA.dts |   1 -
 2 files changed, 156 deletions(-)
 delete mode 100644 arch/arm/dts/zynqmp-clk.dtsi

diff --git a/arch/arm/dts/zynqmp-clk.dtsi b/arch/arm/dts/zynqmp-clk.dtsi
deleted file mode 100644
index 68ece9aa6732..
--- a/arch/arm/dts/zynqmp-clk.dtsi
+++ /dev/null
@@ -1,155 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * Clock specification for Xilinx ZynqMP
- *
- * (C) Copyright 2017, Xilinx, Inc.
- *
- * Michal Simek 
- */
-
-#include 
-
-&zynqmp_firmware {
-   zynqmp_clk: clock-controller {
-   #clock-cells = <1>;
-   compatible = "xlnx,zynqmp-clk";
-   clocks = <&pss_ref_clk>, <&video_clk>, <&pss_alt_ref_clk>, 
<&aux_ref_clk>, <>_crx_ref_clk>;
-   clock-names = "pss_ref_clk", "video_clk", "pss_alt_ref_clk", 
"aux_ref_clk", "gt_crx_ref_clk";
-   };
-};
-
-/ {
-   pss_ref_clk: pss_ref_clk {
-   compatible = "fixed-clock";
-   #clock-cells = <0>;
-   clock-frequency = <>;
-   };
-
-   video_clk: video_clk {
-   compatible = "fixed-clock";
-   #clock-cells = <0>;
-   clock-frequency = <2700>;
-   };
-
-   pss_alt_ref_clk: pss_alt_ref_clk {
-   compatible = "fixed-clock";
-   #clock-cells = <0>;
-   clock-frequency = <0>;
-   };
-
-   gt_crx_ref_clk: gt_crx_ref_clk {
-   compatible = "fixed-clock";
-   #clock-cells = <0>;
-   clock-frequency = <10800>;
-   };
-
-   aux_ref_clk: aux_ref_clk {
-   compatible = "fixed-clock";
-   #clock-cells = <0>;
-   clock-frequency = <2700>;
-   };
-};
-
-&can0 {
-   clocks = <&zynqmp_clk CAN0_REF>, <&zynqmp_clk LPD_LSBUS>;
-};
-
-&can1 {
-   clocks = <&zynqmp_clk CAN1_REF>, <&zynqmp_clk LPD_LSBUS>;
-};
-
-&cpu0 {
-   clocks = <&zynqmp_clk ACPU>;
-};
-
-&gem0 {
-   clocks = <&zynqmp_clk LPD_LSBUS>, <&zynqmp_clk LPD_LSBUS>, <&zynqmp_clk 
GEM0_TX>, <&zynqmp_clk GEM0_REF>, <&zynqmp_clk GEM_TSU>;
-   clock-names = "pclk", "hclk", "tx_clk", "rx_clk", "tsu_clk";
-};
-
-&gem1 {
-   clocks = <&zynqmp_clk LPD_LSBUS>, <&zynqmp_clk LPD_LSBUS>, <&zynqmp_clk 
GEM1_TX>, <&zynqmp_clk GEM1_REF>, <&zynqmp_clk GEM_TSU>;
-   clock-names = "pclk", "hclk", "tx_clk", "rx_clk", "tsu_clk";
-};
-
-&gem2 {
-   clocks = <&zynqmp_clk LPD_LSBUS>, <&zynqmp_clk LPD_LSBUS>, <&zynqmp_clk 
GEM2_TX>, <&zynqmp_clk GEM2_REF>, <&zynqmp_clk GEM_TSU>;
-   clock-names = "pclk", "hclk", "tx_clk", "rx_clk", "tsu_clk";
-};
-
-&gem3 {
-   clocks = <&zynqmp_clk LPD_LSBUS>, <&zynqmp_clk LPD_LSBUS>, <&zynqmp_clk 
GEM3_TX>, <&zynqmp_clk GEM3_REF>, <&zynqmp_clk GEM_TSU>;
-   clock-names = "pclk", "hclk", "tx_clk", "rx_clk", "tsu_clk";
-};
-
-&gpio {
-   clocks = <&zynqmp_clk LPD_LSBUS>;
-};
-
-&i2c0 {
-   clocks = <&zynqmp_clk I2C0_REF>;
-};
-
-&i2c1 {
-   clocks = <&zynqmp_clk I2C1_REF>;
-};
-
-&pcie {
-   clocks = <&zynqmp_clk PCIE_REF>;
-};
-
-&sata {
-   clocks = <&zynqmp_clk SATA_REF>;
-};
-
-&sdhci0 {
-   clocks = <&zynqmp_clk SDIO0_REF>, <&zynqmp_clk LPD_LSBUS>;
-};
-
-&sdhci1 {
-   clocks = <&zynqmp_clk SDIO1_REF>, <&zynqmp_clk LPD_LSBUS>;
-};
-
-&spi0 {
-   clocks = <&zynqmp_clk SPI0_REF>, <&zynqmp_clk LPD_LSBUS>;
-};
-
-&spi1 {
-   clocks = <&zynqmp_clk SPI0_REF>, <&zynqmp_clk LPD_LSBUS>;
-};
-
-&ttc0 {
-   clocks = <&zynqmp_clk LPD_LSBUS>;
-};
-
-&ttc1 {
-   clo

Re: Barebox OF-Overlay Handling

[Michael Tretter]

On Wed, 13 Jan 2021 15:52:23 +0100, Michael Tretter wrote:
> On Wed, 13 Jan 2021 15:07:48 +0100, Rouven Czerwinski wrote:
> > On Wed, 2021-01-13 at 12:07 +0100, Marco Felsch wrote:
> > > Hi there,
> > > 
> > > I have the following problem. My customer is using overlays for
> > > external devices like: display, camera, etc. The current overlay support
> > > is awesome and most of it works out of the box. There is only one
> > > nitpick: If Barebox isn't build with CONFIG_OF_OVERLAY_LIVE enabled many
> > > error's are printed during boot. Those error's are ignored but let the
> > > user assume that something went really bad:
> > > 
> > > ERROR: of_resolver: __symbols__ missing from base devicetree
> > > ERROR: of_overlay: fragment 30164b88: phandle 0x not found
> > > ERROR: of_overlay: fragment 30164c84: phandle 0x not found
> > > ERROR: of_overlay: fragment 3012fb08: phandle 0x not found
> > > ERROR: of_overlay: fragment 3012fc34: phandle 0x not found
> > > ERROR: of_overlay: fragment 3012fd60: phandle 0x not found
> > > ERROR: of_overlay: fragment 3012fe8c: phandle 0x not found
> > > ERROR: of_overlay: fragment 3012ffb8: phandle 0xwfff not found
> > > ERROR: of_overlay: fragment 301300e4: phandle 0x not found
> > > ERROR: of_overlay: fragment 30130210: phandle 0x not found
> > > ERROR: of_overlay: fragment 3013033c: phandle 0x not found
> > > ERROR: of_resolver: __symbols__ missing from base devicetree
> > > ERROR: of_overlay: fragment 30130e94: phandle 0x not found
> > > ERROR: of_overlay: fragment 30130fc4: phandle 0x not found
> > > ERROR: of_resolver: __symbols__ missing from base devicetree
> > > ERROR: of_overlay: fragment 30131a14: phandle 0x not found
> > > ERROR: of_overlay: fragment 301342d4: phandle 0x not found
> > > 
> > > The of_firmware_load_overlay() calls triggering those errors.
> > > The error's by itself are correct and I don't wanna change them but the
> > > context must be correct by context I mean:
> > > 
> > >  - barebox-dt on fpga-platform: An FPGA platform needs a barebox
> > >    base devicetree with __symbols__ and those error are correct because
> > >    the firmware manager needs to load the firmware.
> > >  - barebox-dt on non fpga-platform: Those errors are not correct.
> > >    We don't need __symbols__ for the barebox base devicetree.
> > 
> > I was wondering why you were getting this error even on a platform
> > without FPGA, turns out of_firmware_load_overlay() is always called if
> > a devicetree overlay is passed via the DT. This in itself isn't a
> > problem, but IMO of_firmware_load_overlay() should check first whether
> > the overlay contains the right compatible ("fpga-region") and property
> > ("firmware-name") before doing of_resolve_phandles and
> > of_process_overlay. This way we can skip a lot of unnecessary overlay
> > handling if we don't end up loading a firmware.
> 
> At the moment, the check and firmware load path use the same code path,
> because there is no difference except that firmware load requires a resolved
> overlay and the check does not. This is a viable solution and there is even a
> helper to iterate the fragments and call a check function for each fragment.
> However the helper calls find_target, which is responsible for most of the
> error messages, and therefore, does not completely resolve the issue.
> 
> (Indeed, the check if the fragment has the "fpga-region" compatible is
> missing. That's a bug.)

This is wrong. The overlay does not have the "fpga-region" compatible. Only
the target node determines if an overlay targets an fpga-region or not.

Michael

> 
> I would rather change the error logging to only log errors if actually
> something goes wrong. Because none of the aforementioned errors is actually
> fatal from a system perspective, but only from a function perspective.
> Furthermore, the messages are not helpful at all to actually find the problem
> in case of a unresolved overlay.
> 
> >  
> > >  - kernel-dt on any platform: Those error's are correct if
> > >    the overlays are using phandles which should be the case most the
> > >    time.
> > 
> > AFAICS this should also fix this case.
> > 
> > > My proposed solution would be a stub like this:
> > > 
> > > #ifdef CONFIG_OF_OVERLAY_LIVE
> > > int of_firmware_loa

Re: Barebox OF-Overlay Handling

[Michael Tretter]

On Wed, 13 Jan 2021 15:07:48 +0100, Rouven Czerwinski wrote:
> On Wed, 2021-01-13 at 12:07 +0100, Marco Felsch wrote:
> > Hi there,
> > 
> > I have the following problem. My customer is using overlays for
> > external devices like: display, camera, etc. The current overlay support
> > is awesome and most of it works out of the box. There is only one
> > nitpick: If Barebox isn't build with CONFIG_OF_OVERLAY_LIVE enabled many
> > error's are printed during boot. Those error's are ignored but let the
> > user assume that something went really bad:
> > 
> > ERROR: of_resolver: __symbols__ missing from base devicetree
> > ERROR: of_overlay: fragment 30164b88: phandle 0x not found
> > ERROR: of_overlay: fragment 30164c84: phandle 0x not found
> > ERROR: of_overlay: fragment 3012fb08: phandle 0x not found
> > ERROR: of_overlay: fragment 3012fc34: phandle 0x not found
> > ERROR: of_overlay: fragment 3012fd60: phandle 0x not found
> > ERROR: of_overlay: fragment 3012fe8c: phandle 0x not found
> > ERROR: of_overlay: fragment 3012ffb8: phandle 0xwfff not found
> > ERROR: of_overlay: fragment 301300e4: phandle 0x not found
> > ERROR: of_overlay: fragment 30130210: phandle 0x not found
> > ERROR: of_overlay: fragment 3013033c: phandle 0x not found
> > ERROR: of_resolver: __symbols__ missing from base devicetree
> > ERROR: of_overlay: fragment 30130e94: phandle 0x not found
> > ERROR: of_overlay: fragment 30130fc4: phandle 0x not found
> > ERROR: of_resolver: __symbols__ missing from base devicetree
> > ERROR: of_overlay: fragment 30131a14: phandle 0x not found
> > ERROR: of_overlay: fragment 301342d4: phandle 0x not found
> > 
> > The of_firmware_load_overlay() calls triggering those errors.
> > The error's by itself are correct and I don't wanna change them but the
> > context must be correct by context I mean:
> > 
> >  - barebox-dt on fpga-platform: An FPGA platform needs a barebox
> >    base devicetree with __symbols__ and those error are correct because
> >    the firmware manager needs to load the firmware.
> >  - barebox-dt on non fpga-platform: Those errors are not correct.
> >    We don't need __symbols__ for the barebox base devicetree.
> 
> I was wondering why you were getting this error even on a platform
> without FPGA, turns out of_firmware_load_overlay() is always called if
> a devicetree overlay is passed via the DT. This in itself isn't a
> problem, but IMO of_firmware_load_overlay() should check first whether
> the overlay contains the right compatible ("fpga-region") and property
> ("firmware-name") before doing of_resolve_phandles and
> of_process_overlay. This way we can skip a lot of unnecessary overlay
> handling if we don't end up loading a firmware.

At the moment, the check and firmware load path use the same code path,
because there is no difference except that firmware load requires a resolved
overlay and the check does not. This is a viable solution and there is even a
helper to iterate the fragments and call a check function for each fragment.
However the helper calls find_target, which is responsible for most of the
error messages, and therefore, does not completely resolve the issue.

(Indeed, the check if the fragment has the "fpga-region" compatible is
missing. That's a bug.)

I would rather change the error logging to only log errors if actually
something goes wrong. Because none of the aforementioned errors is actually
fatal from a system perspective, but only from a function perspective.
Furthermore, the messages are not helpful at all to actually find the problem
in case of a unresolved overlay.

>  
> >  - kernel-dt on any platform: Those error's are correct if
> >    the overlays are using phandles which should be the case most the
> >    time.
> 
> AFAICS this should also fix this case.
> 
> > My proposed solution would be a stub like this:
> > 
> > #ifdef CONFIG_OF_OVERLAY_LIVE
> > int of_firmware_load_overlay(struct device_node *overlay, const char *path);
> > #else
> > static inline int of_firmware_load_overlay(struct device_node *overlay, 
> > const char *path)
> > {
> >    return 0;
> > }
> > #endif /* CONFIG_OF_OVERLAY_LIVE */

Nack. of_firmware_load_overlay must fail if the overlay describes an
fpga-region. Otherwise the device tree that is passed to the kernel might have
an fpga-region with a firmware-name property and that per specification
requires that the fpga is programmed.

Michael

> 
> While correct in case all firmware overlays use phandles, in theory
> overlays could be purely path-based, with not dependency on the
> __symbols__ node. Not sure how common this is.

> 
> > The disadvantage of this solution is that it can happen to end in a
> > non-booting device for FPGA platforms using overlays to programm the
> > bit stream and forget to enable CONFIG_OF_OVERLAY_LIVE because the
> > kernel tries to access regions not existing.
> > 
> > I've disc

[PATCH] overlay: return error if target for firmware is missing

[Michael Tretter]

If the overlay references a firmware, the overlay must not be applied if
the firmware cannot be loaded. However, if the target node of the
firmware (i.e., the firmware manager) was not found, the fragment was
ignored and the firmware not loaded, but the overlay applied anyway.

If the overlay does not reference a firmware, of_process_overlay must
succeed even if the target node cannot be found, because the overlay
might still apply to the Linux device tree.

Always call the process function on a fragment, even if the target node
was not found. This allows the caller to decide, if a missing target is
fatal or if the overlay can be applied anyway.

Fix load_firmware to fail if a overlay references a firmware and the
target is NULL. Also, rephrase and clarify the comment that documents
this behavior.

Reported-by: Matthias Fend 
Signed-off-by: Michael Tretter 
---
 drivers/of/of_firmware.c | 11 ---
 drivers/of/overlay.c |  5 +++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/of/of_firmware.c b/drivers/of/of_firmware.c
index 0135631fb899..096f84572e63 100644
--- a/drivers/of/of_firmware.c
+++ b/drivers/of/of_firmware.c
@@ -43,6 +43,9 @@ static int load_firmware(struct device_node *target,
else if (err)
return -EINVAL;
 
+   if (!target)
+   return -EINVAL;
+
mgr = of_node_get_mgr(target);
if (!mgr)
return -EINVAL;
@@ -69,11 +72,13 @@ int of_firmware_load_overlay(struct device_node *overlay, 
const char *path)
struct device_node *ovl;
 
root = of_get_root_node();
+   resolved = of_resolve_phandles(root, overlay);
/*
-* If we cannot resolve the symbols in the overlay, ensure that the
-* overlay does depend on firmware to be loaded.
+* If the overlay cannot be resolved, use the load_firmware callback
+* with the unresolved overlay to verify that the overlay does not
+* depend on a firmware to be loaded. If a required firmware cannot be
+* loaded, the overlay must not be applied.
 */
-   resolved = of_resolve_phandles(root, overlay);
ovl = resolved ? resolved : overlay;
 
err = of_process_overlay(root, ovl,
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index b79dbff94dbf..eb47378258b6 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -215,12 +215,13 @@ int of_process_overlay(struct device_node *root,
 
target = find_target(root, fragment);
if (!target)
-   continue;
+   pr_debug("cannot find target for fragment",
+fragment->name);
 
err = process(target, ovl, data);
if (err) {
pr_warn("failed to process overlay for %s\n",
-   target->name);
+   target ? target->name : "unknown");
break;
}
}
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v3 2/2] uimage: disable zero page when loading to SDRAM at address 0x0

[Michael Tretter]

If the SDRAM is mapped to address 0x0 and an image should be loaded to
to the SDRAM without offset, Barebox would normally trap the access as a
null pointer.

However, since Linux kernel commit cfa7ede20f13 ("arm64: set TEXT_OFFSET
to 0x0 in preparation for removing it entirely") no offset is the
default for arm64. Therefore, copying the image to 0x0 of the SDRAM is
necessary.

Disable the zero page trap for copying an image to address 0x0.

Signed-off-by: Michael Tretter 
---
v3:
- none

v2:
- switch to zero_page_memcpy helper function
- read file to temporary buffer before copying to page 0
---
 common/uimage.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/common/uimage.c b/common/uimage.c
index a84b8fddc4e7..9abfbcf3bac9 100644
--- a/common/uimage.c
+++ b/common/uimage.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static inline int uimage_is_multi_image(struct uimage_handle *handle)
 {
@@ -359,7 +360,10 @@ static int uimage_sdram_flush(void *buf, unsigned int len)
}
}
 
-   memcpy(uimage_buf + uimage_size, buf, len);
+   if (zero_page_contains((unsigned long)uimage_buf + uimage_size))
+   zero_page_memcpy(uimage_buf + uimage_size, buf, len);
+   else
+   memcpy(uimage_buf + uimage_size, buf, len);
 
uimage_size += len;
 
@@ -388,7 +392,20 @@ struct resource *file_to_sdram(const char *filename, 
unsigned long adr)
goto out;
}
 
-   now = read_full(fd, (void *)(res->start + ofs), BUFSIZ);
+   if (zero_page_contains(res->start + ofs)) {
+   void *tmp = malloc(BUFSIZ);
+   if (!tmp)
+   now = -ENOMEM;
+   else
+   now = read_full(fd, tmp, BUFSIZ);
+
+   if (now > 0)
+   zero_page_memcpy((void *)(res->start + ofs), 
tmp, now);
+   free(tmp);
+   } else {
+   now = read_full(fd, (void *)(res->start + ofs), BUFSIZ);
+   }
+
if (now < 0) {
release_sdram_region(res);
res = NULL;
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v3 1/2] ARM: mmu64: allow to disable null pointer trap on zero page

[Michael Tretter]

Barebox uses the zero page to trap NULL pointer dereferences. However,
if the SDRAM starts at address 0x0, this makes the first page of the
SDRAM inaccessible and makes it impossible to load images to offset 0x0
in the SDRAM.

Trapping NULL pointer dereferences on such systems is still desirable.
Therefore, add a function to disable the traps if accessing the zero
page is necessary and to re-enable the traps after the access is done.

The zero_page_memcpy function simplifies copying to the SDRAM, because
this is the most common required functionality, but memtest also
accesses the zero page and does not use memcpy.

Signed-off-by: Michael Tretter 
---
v3:
- rename functions to zero_page_access and zero_page_faulting

v2:
- add a helper function for copying to or from the zero page

I am not a fan of having an architecture-specific memcpy for the zero
page, because there are other cases that need disabling of the zero
page, e.g. memtest. Therefore, I am going for a helper for memcpy, but
still expose the architecture-specific enable/disable logic.
---
 arch/arm/cpu/Kconfig  |  1 +
 arch/arm/cpu/mmu_64.c | 13 ++-
 include/zero_page.h   | 54 +++
 lib/Kconfig   |  3 +++
 4 files changed, 70 insertions(+), 1 deletion(-)
 create mode 100644 include/zero_page.h

diff --git a/arch/arm/cpu/Kconfig b/arch/arm/cpu/Kconfig
index f9f52a625260..ca3bd98962e2 100644
--- a/arch/arm/cpu/Kconfig
+++ b/arch/arm/cpu/Kconfig
@@ -89,6 +89,7 @@ config CPU_V8
select ARM_EXCEPTIONS
select GENERIC_FIND_NEXT_BIT
select ARCH_HAS_STACK_DUMP
+   select ARCH_HAS_ZERO_PAGE
 
 config CPU_XSC3
 bool
diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
index 7e9ae84810f6..06049e000375 100644
--- a/arch/arm/cpu/mmu_64.c
+++ b/arch/arm/cpu/mmu_64.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -168,6 +169,16 @@ static void mmu_enable(void)
set_cr(get_cr() | CR_M | CR_C | CR_I);
 }
 
+void zero_page_access(void)
+{
+   create_sections(0x0, 0x0, PAGE_SIZE, CACHED_MEM);
+}
+
+void zero_page_faulting(void)
+{
+   create_sections(0x0, 0x0, PAGE_SIZE, 0x0);
+}
+
 /*
  * Prepare MMU for usage enable it.
  */
@@ -194,7 +205,7 @@ void __mmu_init(bool mmu_on)
create_sections(bank->start, bank->start, bank->size, 
CACHED_MEM);
 
/* Make zero page faulting to catch NULL pointer derefs */
-   create_sections(0x0, 0x0, 0x1000, 0x0);
+   zero_page_faulting();
 
mmu_enable();
 }
diff --git a/include/zero_page.h b/include/zero_page.h
new file mode 100644
index ..ad6861f240c6
--- /dev/null
+++ b/include/zero_page.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ZERO_PAGE_H
+#define __ZERO_PAGE_H
+
+#include 
+
+#if defined CONFIG_ARCH_HAS_ZERO_PAGE
+
+/*
+ * zero_page_faulting - fault when accessing the zero page
+ */
+void zero_page_faulting(void);
+
+/*
+ * zero_page_access - allow accesses to the zero page
+ *
+ * Disable the null pointer trap on the zero page if access to the zero page
+ * is actually required. Disable the trap with care and re-enable it
+ * immediately after the access to properly trap null pointers.
+ */
+void zero_page_access(void);
+
+#else
+
+static inline void zero_page_faulting(void)
+{
+}
+
+static inline void zero_page_access(void)
+{
+}
+
+#endif
+
+static inline bool zero_page_contains(unsigned long addr)
+{
+   return addr < PAGE_SIZE;
+}
+
+/*
+ * zero_page_memcpy - copy to or from an address located in the zero page
+ */
+static inline void *zero_page_memcpy(void *dest, const void *src, size_t count)
+{
+   void *ret;
+
+   zero_page_access();
+   ret = memcpy(dest, src, count);
+   zero_page_faulting();
+
+   return ret;
+}
+
+#endif /* __ZERO_PAGE_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index 887f50ff003f..e5831ecdb9a7 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -182,6 +182,9 @@ config ARCH_HAS_STACK_DUMP
 config ARCH_HAS_DATA_ABORT_MASK
bool
 
+config ARCH_HAS_ZERO_PAGE
+   bool
+
 config HAVE_EFFICIENT_UNALIGNED_ACCESS
bool
 
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/2] uimage: disable zero page when loading to SDRAM at address 0x0

[Michael Tretter]

If the SDRAM is mapped to address 0x0 and an image should be loaded to
to the SDRAM without offset, Barebox would normally trap the access as a
null pointer.

However, since Linux kernel commit cfa7ede20f13 ("arm64: set TEXT_OFFSET
to 0x0 in preparation for removing it entirely") no offset is the
default for arm64. Therefore, copying the image to 0x0 of the SDRAM is
necessary.

Disable the zero page trap for copying an image to address 0x0.

Signed-off-by: Michael Tretter 
---
v2:
- switch to zero_page_memcpy helper function
- read file to temporary buffer before copying to page 0
---
 common/uimage.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/common/uimage.c b/common/uimage.c
index a84b8fddc4e7..9abfbcf3bac9 100644
--- a/common/uimage.c
+++ b/common/uimage.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static inline int uimage_is_multi_image(struct uimage_handle *handle)
 {
@@ -359,7 +360,10 @@ static int uimage_sdram_flush(void *buf, unsigned int len)
}
}
 
-   memcpy(uimage_buf + uimage_size, buf, len);
+   if (zero_page_contains((unsigned long)uimage_buf + uimage_size))
+   zero_page_memcpy(uimage_buf + uimage_size, buf, len);
+   else
+   memcpy(uimage_buf + uimage_size, buf, len);
 
uimage_size += len;
 
@@ -388,7 +392,20 @@ struct resource *file_to_sdram(const char *filename, 
unsigned long adr)
goto out;
}
 
-   now = read_full(fd, (void *)(res->start + ofs), BUFSIZ);
+   if (zero_page_contains(res->start + ofs)) {
+   void *tmp = malloc(BUFSIZ);
+   if (!tmp)
+   now = -ENOMEM;
+   else
+   now = read_full(fd, tmp, BUFSIZ);
+
+   if (now > 0)
+   zero_page_memcpy((void *)(res->start + ofs), 
tmp, now);
+   free(tmp);
+   } else {
+   now = read_full(fd, (void *)(res->start + ofs), BUFSIZ);
+   }
+
if (now < 0) {
release_sdram_region(res);
res = NULL;
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 1/2] ARM: mmu64: allow to disable null pointer trap on zero page

[Michael Tretter]

Barebox uses the zero page to trap NULL pointer dereferences. However,
if the SDRAM starts at address 0x0, this makes the first page of the
SDRAM inaccessible and makes it impossible to load images to offset 0x0
in the SDRAM.

Trapping NULL pointer dereferences on such systems is still desirable.
Therefore, add a function to disable the traps if accessing the zero
page is necessary and to re-enable the traps after the access is done.

The zero_page_memcpy function simplifies copying to the SDRAM, because
this is the most common required functionality, but memtest also
accesses the zero page and does not use memcpy.

Signed-off-by: Michael Tretter 
---
v2:
- add a helper function for copying to or from the zero page

I am not a fan of having an architecture-specific memcpy for the zero
page, because there are other cases that need disabling of the zero
page, e.g. memtest. Therefore, I am going for a helper for memcpy, but
still expose the architecture-specific enable/disable logic.
---
 arch/arm/cpu/Kconfig  |  1 +
 arch/arm/cpu/mmu_64.c | 13 ++-
 include/zero_page.h   | 54 +++
 lib/Kconfig   |  3 +++
 4 files changed, 70 insertions(+), 1 deletion(-)
 create mode 100644 include/zero_page.h

diff --git a/arch/arm/cpu/Kconfig b/arch/arm/cpu/Kconfig
index f9f52a625260..ca3bd98962e2 100644
--- a/arch/arm/cpu/Kconfig
+++ b/arch/arm/cpu/Kconfig
@@ -89,6 +89,7 @@ config CPU_V8
select ARM_EXCEPTIONS
select GENERIC_FIND_NEXT_BIT
select ARCH_HAS_STACK_DUMP
+   select ARCH_HAS_ZERO_PAGE
 
 config CPU_XSC3
 bool
diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
index 7e9ae84810f6..bd15807f9160 100644
--- a/arch/arm/cpu/mmu_64.c
+++ b/arch/arm/cpu/mmu_64.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -168,6 +169,16 @@ static void mmu_enable(void)
set_cr(get_cr() | CR_M | CR_C | CR_I);
 }
 
+void zero_page_disable(void)
+{
+   create_sections(0x0, 0x0, PAGE_SIZE, CACHED_MEM);
+}
+
+void zero_page_enable(void)
+{
+   create_sections(0x0, 0x0, PAGE_SIZE, 0x0);
+}
+
 /*
  * Prepare MMU for usage enable it.
  */
@@ -194,7 +205,7 @@ void __mmu_init(bool mmu_on)
create_sections(bank->start, bank->start, bank->size, 
CACHED_MEM);
 
/* Make zero page faulting to catch NULL pointer derefs */
-   create_sections(0x0, 0x0, 0x1000, 0x0);
+   zero_page_enable();
 
mmu_enable();
 }
diff --git a/include/zero_page.h b/include/zero_page.h
new file mode 100644
index ..14c85cb6c860
--- /dev/null
+++ b/include/zero_page.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ZERO_PAGE_H
+#define __ZERO_PAGE_H
+
+#include 
+
+#if defined CONFIG_ARCH_HAS_ZERO_PAGE
+
+/*
+ * zero_page_enable - enable null pointer trap
+ */
+void zero_page_enable(void);
+
+/*
+ * zero_page_disable - disable null pointer trap
+ *
+ * Disable the null pointer trap on the zero page if access to the zero page
+ * is actually required. Disable the trap with care and re-enable it
+ * immediately after the access to properly trap null pointers.
+ */
+void zero_page_disable(void);
+
+#else
+
+static inline void zero_page_enable(void)
+{
+}
+
+static inline void zero_page_disable(void)
+{
+}
+
+#endif
+
+static inline bool zero_page_contains(unsigned long addr)
+{
+   return addr < PAGE_SIZE;
+}
+
+/*
+ * zero_page_memcpy - copy to or from an address located in the zero page
+ */
+static inline void *zero_page_memcpy(void *dest, const void *src, size_t count)
+{
+   void *ret;
+
+   zero_page_disable();
+   ret = memcpy(dest, src, count);
+   zero_page_enable();
+
+   return ret;
+}
+
+#endif /* __ZERO_PAGE_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index 887f50ff003f..e5831ecdb9a7 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -182,6 +182,9 @@ config ARCH_HAS_STACK_DUMP
 config ARCH_HAS_DATA_ABORT_MASK
bool
 
+config ARCH_HAS_ZERO_PAGE
+   bool
+
 config HAVE_EFFICIENT_UNALIGNED_ACCESS
bool
 
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] net: macb: fix compiler warning for 64 bit systems

[Michael Tretter]

On Thu, 15 Oct 2020 15:29:01 +0200, Ahmad Fatoum wrote:
> On 10/15/20 3:20 PM, Michael Tretter wrote:
> > On arm64 the compiler prints the following warning, when the macb driver
> > is enabled:
> > 
> > warning: cast from pointer to integer of different size
> > 
> > Add the same explicit cast as implemented for all other dma addresses in
> > the macb driver.
> > 
> > Fixes: befd110b5922 ("net: macb: init multiple dummy TX queues")
> 
> I don't think this qualifies as a fix. You just silence the compiler
> warning you that your truncate the upper 32 bits of your buffer's
> address. This works because your SDRAM's location in physical memory
> is below 4G.

Correct. Which is exactly the same as assumed by the entire driver...

Michael

> 
> > Signed-off-by: Michael Tretter 
> > ---
> >  drivers/net/macb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> > index fa530cfe8e4c..188dbf2d8c15 100644
> > --- a/drivers/net/macb.c
> > +++ b/drivers/net/macb.c
> > @@ -365,7 +365,7 @@ static int gmac_init_dummy_tx_queues(struct macb_device 
> > *macb)
> > MACB_BIT(TX_LAST) | MACB_BIT(TX_USED);
> >  
> > for (i = 1; i < num_queues; i++)
> > -   gem_writel_queue_TBQP(macb, &macb->gem_q1_descs[0], i - 1);
> > +   gem_writel_queue_TBQP(macb, (ulong)macb->gem_q1_descs, i - 1);
> >  
> > return 0;
> >  }
> > 

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] net: macb: adjust clk_tx rate for link speed changes

[Michael Tretter]

Changes of the link speed might require an adjustment of the clk_tx.

Read the clk_tx from the device tree and change the rate if the link
speed changes.

If the clk_tx rate is already correct, changing the clock is not
required and, thus, not being able to get the clock rate is not fatal.

Signed-off-by: Michael Tretter 
---
 drivers/net/macb.c | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index 09b58ffd017f..fa530cfe8e4c 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -258,9 +258,38 @@ static int macb_recv(struct eth_device *edev)
return 0;
 }
 
+static int macb_set_tx_clk(struct macb_device *macb, int speed)
+{
+   int rate;
+   int rate_rounded;
+
+   if (!macb->txclk) {
+   dev_dbg(macb->dev, "txclk not available\n");
+   return 0;
+   }
+
+   switch (speed) {
+   case SPEED_100:
+   rate = 2500;
+   break;
+   case SPEED_1000:
+   rate = 12500;
+   break;
+   default:
+   return -EINVAL;
+   }
+
+   rate_rounded = clk_round_rate(macb->txclk, rate);
+   if (rate_rounded <= 0)
+   return -EINVAL;
+
+   return clk_set_rate(macb->txclk, rate_rounded);
+}
+
 static void macb_adjust_link(struct eth_device *edev)
 {
struct macb_device *macb = edev->priv;
+   int err;
u32 reg;
 
reg = macb_readl(macb, NCFGR);
@@ -276,6 +305,10 @@ static void macb_adjust_link(struct eth_device *edev)
reg |= GEM_BIT(GBE);
 
macb_or_gem_writel(macb, NCFGR, reg);
+
+   err = macb_set_tx_clk(macb, edev->phydev->speed);
+   if (err)
+   dev_warn(macb->dev, "cannot set txclk\n");
 }
 
 static int macb_open(struct eth_device *edev)
@@ -724,6 +757,8 @@ static int macb_probe(struct device_d *dev)
macb->txclk = clk_get(dev, "tx_clk");
if (!IS_ERR(macb->txclk))
clk_enable(macb->txclk);
+   else
+   macb->txclk = NULL;
 
macb->rxclk = clk_get(dev, "rx_clk");
if (!IS_ERR(macb->rxclk))
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] net: macb: fix compiler warning for 64 bit systems

[Michael Tretter]

On arm64 the compiler prints the following warning, when the macb driver
is enabled:

warning: cast from pointer to integer of different size

Add the same explicit cast as implemented for all other dma addresses in
the macb driver.

Fixes: befd110b5922 ("net: macb: init multiple dummy TX queues")
Signed-off-by: Michael Tretter 
---
 drivers/net/macb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index fa530cfe8e4c..188dbf2d8c15 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -365,7 +365,7 @@ static int gmac_init_dummy_tx_queues(struct macb_device 
*macb)
MACB_BIT(TX_LAST) | MACB_BIT(TX_USED);
 
for (i = 1; i < num_queues; i++)
-   gem_writel_queue_TBQP(macb, &macb->gem_q1_descs[0], i - 1);
+   gem_writel_queue_TBQP(macb, (ulong)macb->gem_q1_descs, i - 1);
 
return 0;
 }
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/3] net: macb: reduce DEBUG output to make it more useful

[Michael Tretter]

The macb debugging output printed the function entry for various
functions. Especially for *_recv() this debugging flooded the serial
output while conveying very little information.

Remove printing of the function calls to make enabling debugging for the
macb driver more useful.

Signed-off-by: Michael Tretter 
---
 drivers/net/macb.c | 18 --
 1 file changed, 18 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index e3e039f67988..09b58ffd017f 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -144,8 +144,6 @@ static void reclaim_rx_buffers(struct macb_device *macb,
 {
unsigned int i;
 
-   dev_dbg(macb->dev, "%s\n", __func__);
-
i = macb->rx_tail;
while (i > new_tail) {
macb->rx_ring[i].addr &= ~MACB_BIT(RX_USED);
@@ -170,8 +168,6 @@ static int gem_recv(struct eth_device *edev)
int length;
u32 status;
 
-   dev_dbg(macb->dev, "%s\n", __func__);
-
for (;;) {
barrier();
if (!(macb->rx_ring[macb->rx_tail].addr & MACB_BIT(RX_USED)))
@@ -206,8 +202,6 @@ static int macb_recv(struct eth_device *edev)
int wrapped = 0;
u32 status;
 
-   dev_dbg(macb->dev, "%s\n", __func__);
-
for (;;) {
barrier();
if (!(macb->rx_ring[rx_tail].addr & MACB_BIT(RX_USED)))
@@ -288,8 +282,6 @@ static int macb_open(struct eth_device *edev)
 {
struct macb_device *macb = edev->priv;
 
-   dev_dbg(macb->dev, "%s\n", __func__);
-
/* Enable TX and RX */
macb_writel(macb, NCR, MACB_BIT(TE) | MACB_BIT(RE));
 
@@ -350,8 +342,6 @@ static void macb_init(struct macb_device *macb)
unsigned long paddr, val = 0;
int i;
 
-   dev_dbg(macb->dev, "%s\n", __func__);
-
/*
 * macb_halt should have been called at some point before now,
 * so we'll assume the controller is idle.
@@ -441,8 +431,6 @@ static int macb_phy_read(struct mii_bus *bus, int addr, int 
reg)
int value;
uint64_t start;
 
-   dev_dbg(macb->dev, "%s\n", __func__);
-
netctl = macb_readl(macb, NCR);
netctl |= MACB_BIT(MPE);
macb_writel(macb, NCR, netctl);
@@ -478,8 +466,6 @@ static int macb_phy_write(struct mii_bus *bus, int addr, 
int reg, u16 value)
unsigned long netctl;
unsigned long frame;
 
-   dev_dbg(macb->dev, "%s\n", __func__);
-
netctl = macb_readl(macb, NCR);
netctl |= MACB_BIT(MPE);
macb_writel(macb, NCR, netctl);
@@ -510,8 +496,6 @@ static int macb_get_ethaddr(struct eth_device *edev, 
unsigned char *adr)
u8 addr[6];
int i;
 
-   dev_dbg(macb->dev, "%s\n", __func__);
-
/* Check all 4 address register for vaild address */
for (i = 0; i < 4; i++) {
bottom = macb_or_gem_readl(macb, SA1B + i * 8);
@@ -537,8 +521,6 @@ static int macb_set_ethaddr(struct eth_device *edev, const 
unsigned char *adr)
 {
struct macb_device *macb = edev->priv;
 
-   dev_dbg(macb->dev, "%s\n", __func__);
-
/* set hardware address */
macb_or_gem_writel(macb, SA1B, adr[0] | adr[1] << 8 | adr[2] << 16 | 
adr[3] << 24);
macb_or_gem_writel(macb, SA1T, adr[4] | adr[5] << 8);
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page

[Michael Tretter]

On Thu, 15 Oct 2020 10:14:41 +0200, Ahmad Fatoum wrote:
> On 10/15/20 9:33 AM, Michael Tretter wrote:
> > On Wed, 14 Oct 2020 18:29:47 +0200, Ahmad Fatoum wrote:
> >> On 10/14/20 5:08 PM, Michael Tretter wrote:
> >>> Barebox uses the zero page to trap NULL pointer dereferences. However,
> >>> if the SDRAM starts at address 0x0, this makes the first page of the
> >>> SDRAM inaccessible and makes it impossible to load images to offset 0x0
> >>> in the SDRAM.
> >>>
> >>> Trapping NULL pointer dereferences on such systems is still desirable.
> >>> Therefore, add a function to disable the traps if accessing the zero
> >>> page is necessary and to re-enable the traps after the access is done.
> >>
> >> Can't we map the (phy_addr_t)0 at some higher virtual address and
> >> change uimage to use phys_to_virt() ?
> > 
> > To which virt would you map (phy_addr_t)0?
> 
> You are on a 64-bit system. Just choose something > 4G that's suitable for
> your ZynqMP?

This would introduce very platform specific handling and diverge from the
shared mmu_64 code. I would rather go for a more general solution.

What about 32 bit ARM systems? Currently, the zero page is simply mapped on
such systems and NULL pointers will never trap.

> 
> > This would break the general virt =
> > phys assumption in Barebox. It might work for some cases, but might break in
> > unexpected ways in other. Therefore, I deem it saver to stay with virt = 
> > phys
> > and allow to disable the trap, if necessary.
> 
> Can you be more specific what kind of problems do you foresee? You map the 
> zero
> page unaccessible already, so any code that would object to having the page 0
> and 1 not be contiguous in memory would already have problems to access page 
> 0.

For example, memtest accesses page 0 and just remaps it.

Michael

> 
> > 
> > Michael
> > 
> >>
> >> Something like 
> >>
> >> static inline void *phys_to_virt(unsigned long phys)
> >> {
> >>if (!IS_ENABLED(CONFIG_ARM_MACH_CUSTOM_MAPPING) || 
> >> !arm_mach_phys_to_virt)
> >>return (void *)phys;
> >>return arm_mach_phys_to_virt(phys);
> >> }
> >>
> >>
> >>>
> >>> Signed-off-by: Michael Tretter 
> >>> ---
> >>>  arch/arm/cpu/Kconfig  |  1 +
> >>>  arch/arm/cpu/mmu_64.c | 13 -
> >>>  include/zero_page.h   | 40 
> >>>  lib/Kconfig   |  3 +++
> >>>  4 files changed, 56 insertions(+), 1 deletion(-)
> >>>  create mode 100644 include/zero_page.h
> >>>
> >>> diff --git a/arch/arm/cpu/Kconfig b/arch/arm/cpu/Kconfig
> >>> index f9f52a625260..ca3bd98962e2 100644
> >>> --- a/arch/arm/cpu/Kconfig
> >>> +++ b/arch/arm/cpu/Kconfig
> >>> @@ -89,6 +89,7 @@ config CPU_V8
> >>>   select ARM_EXCEPTIONS
> >>>   select GENERIC_FIND_NEXT_BIT
> >>>   select ARCH_HAS_STACK_DUMP
> >>> + select ARCH_HAS_ZERO_PAGE
> >>>  
> >>>  config CPU_XSC3
> >>>  bool
> >>> diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
> >>> index 7e9ae84810f6..bd15807f9160 100644
> >>> --- a/arch/arm/cpu/mmu_64.c
> >>> +++ b/arch/arm/cpu/mmu_64.c
> >>> @@ -10,6 +10,7 @@
> >>>  #include 
> >>>  #include 
> >>>  #include 
> >>> +#include 
> >>>  #include 
> >>>  #include 
> >>>  #include 
> >>> @@ -168,6 +169,16 @@ static void mmu_enable(void)
> >>>   set_cr(get_cr() | CR_M | CR_C | CR_I);
> >>>  }
> >>>  
> >>> +void zero_page_disable(void)
> >>> +{
> >>> + create_sections(0x0, 0x0, PAGE_SIZE, CACHED_MEM);
> >>> +}
> >>> +
> >>> +void zero_page_enable(void)
> >>> +{
> >>> + create_sections(0x0, 0x0, PAGE_SIZE, 0x0);
> >>> +}
> >>> +
> >>>  /*
> >>>   * Prepare MMU for usage enable it.
> >>>   */
> >>> @@ -194,7 +205,7 @@ void __mmu_init(bool mmu_on)
> >>>   create_sections(bank->start, bank->start, bank->size, 
> >>> CACHED_MEM);
> >>>  
> >>>   /* Make zero page faulting to catch NULL pointer derefs */
> >>> - create_sections(0x0, 0x0, 0x1000, 0x0);
> >>> + zero_page_enable();
> >

Re: [PATCH 2/2] uimage: disable zero page when loading to SDRAM at address 0x0

[Michael Tretter]

On Wed, 14 Oct 2020 18:33:25 +0200, Ahmad Fatoum wrote:
> On 10/14/20 5:08 PM, Michael Tretter wrote:
> > If the SDRAM is mapped to address 0x0 and an image should be loaded to
> > to the SDRAM without offset, Barebox would normally trap the access as a
> > null pointer.
> > 
> > However, since Linux kernel commit cfa7ede20f13 ("arm64: set TEXT_OFFSET
> > to 0x0 in preparation for removing it entirely") no offset is the
> > default for arm64. Therefore, copying the image to 0x0 of the SDRAM is
> > necessary.
> > 
> > Disable the zero page trap for copying an image to address 0x0.
> > 
> > Signed-off-by: Michael Tretter 
> > ---
> >  common/uimage.c | 18 --
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/common/uimage.c b/common/uimage.c
> > index a84b8fddc4e7..b1e9b402e98a 100644
> > --- a/common/uimage.c
> > +++ b/common/uimage.c
> > @@ -27,6 +27,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  static inline int uimage_is_multi_image(struct uimage_handle *handle)
> >  {
> > @@ -359,7 +360,13 @@ static int uimage_sdram_flush(void *buf, unsigned int 
> > len)
> > }
> > }
> >  
> > -   memcpy(uimage_buf + uimage_size, buf, len);
> > +   if (zero_page_contains((unsigned long)uimage_buf + uimage_size)) {
> > +   zero_page_disable();
> > +   memcpy(uimage_buf + uimage_size, buf, len);
> > +   zero_page_enable();
> 
> If this remains, please add a memcpy_notrap or something.

Should I check the destination before calling memcpy_notrap or should I always
call the memcpy_notrap if there is a possibility to copy to 0x0 and check for
the destination within the function?

I fear that having such a "simple" function would encourage to use it more
often. I would prefer to make the code to use it more clumsy and make it
(similar to data_abort_mask()) the responsibility of the caller to be aware
that bad things might happen when the zero_page is disabled.

> 
> > +   } else {
> > +   memcpy(uimage_buf + uimage_size, buf, len);
> > +   }
> >  
> > uimage_size += len;
> >  
> > @@ -388,7 +395,14 @@ struct resource *file_to_sdram(const char *filename, 
> > unsigned long adr)
> > goto out;
> > }
> >  
> > -   now = read_full(fd, (void *)(res->start + ofs), BUFSIZ);
> > +   if (zero_page_contains(res->start + ofs)) {
> > +   zero_page_disable();
> > +   now = read_full(fd, (void *)(res->start + ofs), BUFSIZ);
> > +   zero_page_enable();
> 
> And use that new memcpy_notrap here to copy from an intermediate buffer. You 
> open quite a can
> of worms when you treat NULL as a valid address. Better have this contained 
> in a single
> file instead of hoping the compiler doesn't do a NULL-can't-happen-here 
> optimization
> in all that block/cdev/fs code that read_full may call into.

Could you explain, what kind of optimization you would expect?

Michael

> 
> > +   } else {
> > +   now = read_full(fd, (void *)(res->start + ofs), BUFSIZ);
> > +   }
> > +
> > if (now < 0) {
> > release_sdram_region(res);
> > res = NULL;
> > 

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] uimage: disable zero page when loading to SDRAM at address 0x0

[Michael Tretter]

If the SDRAM is mapped to address 0x0 and an image should be loaded to
to the SDRAM without offset, Barebox would normally trap the access as a
null pointer.

However, since Linux kernel commit cfa7ede20f13 ("arm64: set TEXT_OFFSET
to 0x0 in preparation for removing it entirely") no offset is the
default for arm64. Therefore, copying the image to 0x0 of the SDRAM is
necessary.

Disable the zero page trap for copying an image to address 0x0.

Signed-off-by: Michael Tretter 
---
 common/uimage.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/common/uimage.c b/common/uimage.c
index a84b8fddc4e7..b1e9b402e98a 100644
--- a/common/uimage.c
+++ b/common/uimage.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static inline int uimage_is_multi_image(struct uimage_handle *handle)
 {
@@ -359,7 +360,13 @@ static int uimage_sdram_flush(void *buf, unsigned int len)
}
}
 
-   memcpy(uimage_buf + uimage_size, buf, len);
+   if (zero_page_contains((unsigned long)uimage_buf + uimage_size)) {
+   zero_page_disable();
+   memcpy(uimage_buf + uimage_size, buf, len);
+   zero_page_enable();
+   } else {
+   memcpy(uimage_buf + uimage_size, buf, len);
+   }
 
uimage_size += len;
 
@@ -388,7 +395,14 @@ struct resource *file_to_sdram(const char *filename, 
unsigned long adr)
goto out;
}
 
-   now = read_full(fd, (void *)(res->start + ofs), BUFSIZ);
+   if (zero_page_contains(res->start + ofs)) {
+   zero_page_disable();
+   now = read_full(fd, (void *)(res->start + ofs), BUFSIZ);
+   zero_page_enable();
+   } else {
+   now = read_full(fd, (void *)(res->start + ofs), BUFSIZ);
+   }
+
if (now < 0) {
release_sdram_region(res);
res = NULL;
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page

[Michael Tretter]

Barebox uses the zero page to trap NULL pointer dereferences. However,
if the SDRAM starts at address 0x0, this makes the first page of the
SDRAM inaccessible and makes it impossible to load images to offset 0x0
in the SDRAM.

Trapping NULL pointer dereferences on such systems is still desirable.
Therefore, add a function to disable the traps if accessing the zero
page is necessary and to re-enable the traps after the access is done.

Signed-off-by: Michael Tretter 
---
 arch/arm/cpu/Kconfig  |  1 +
 arch/arm/cpu/mmu_64.c | 13 -
 include/zero_page.h   | 40 
 lib/Kconfig   |  3 +++
 4 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 include/zero_page.h

diff --git a/arch/arm/cpu/Kconfig b/arch/arm/cpu/Kconfig
index f9f52a625260..ca3bd98962e2 100644
--- a/arch/arm/cpu/Kconfig
+++ b/arch/arm/cpu/Kconfig
@@ -89,6 +89,7 @@ config CPU_V8
select ARM_EXCEPTIONS
select GENERIC_FIND_NEXT_BIT
select ARCH_HAS_STACK_DUMP
+   select ARCH_HAS_ZERO_PAGE
 
 config CPU_XSC3
 bool
diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
index 7e9ae84810f6..bd15807f9160 100644
--- a/arch/arm/cpu/mmu_64.c
+++ b/arch/arm/cpu/mmu_64.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -168,6 +169,16 @@ static void mmu_enable(void)
set_cr(get_cr() | CR_M | CR_C | CR_I);
 }
 
+void zero_page_disable(void)
+{
+   create_sections(0x0, 0x0, PAGE_SIZE, CACHED_MEM);
+}
+
+void zero_page_enable(void)
+{
+   create_sections(0x0, 0x0, PAGE_SIZE, 0x0);
+}
+
 /*
  * Prepare MMU for usage enable it.
  */
@@ -194,7 +205,7 @@ void __mmu_init(bool mmu_on)
create_sections(bank->start, bank->start, bank->size, 
CACHED_MEM);
 
/* Make zero page faulting to catch NULL pointer derefs */
-   create_sections(0x0, 0x0, 0x1000, 0x0);
+   zero_page_enable();
 
mmu_enable();
 }
diff --git a/include/zero_page.h b/include/zero_page.h
new file mode 100644
index ..d8dd07cfe959
--- /dev/null
+++ b/include/zero_page.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ZERO_PAGE_H
+#define __ZERO_PAGE_H
+
+#include 
+
+#if defined CONFIG_ARCH_HAS_ZERO_PAGE
+
+/*
+ * zero_page_enable - enable null pointer trap
+ */
+void zero_page_enable(void);
+
+/*
+ * zero_page_disable - disable null pointer trap
+ *
+ * Disable the null pointer trap on the zero page if access to the zero page
+ * is actually required. Disable the trap with care and re-enable it
+ * immediately after the access to properly trap null pointers.
+ */
+void zero_page_disable(void);
+
+#else
+
+static inline void zero_page_enable(void)
+{
+}
+
+static inline void zero_page_disable(void)
+{
+}
+
+#endif
+
+static inline bool zero_page_contains(unsigned long addr)
+{
+   return addr < PAGE_SIZE;
+}
+
+#endif /* __ZERO_PAGE_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index 887f50ff003f..e5831ecdb9a7 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -182,6 +182,9 @@ config ARCH_HAS_STACK_DUMP
 config ARCH_HAS_DATA_ABORT_MASK
bool
 
+config ARCH_HAS_ZERO_PAGE
+   bool
+
 config HAVE_EFFICIENT_UNALIGNED_ACCESS
bool
 
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 7/7] added-zynq-fpga-manager

[Michael Tretter]

Hi Michael,

On Tue, Apr 14, 2020 at 09:23:53PM +0200, Michael Graichen wrote:
> Am 14.04.20 um 12:53 schrieb Michael Tretter:
> > On Thu, Apr 09, 2020 at 02:44:00PM +, Michael Graichen wrote:
> > > This adds support to programm the PL part of the Zynq SoC,
> > > but only the non-secure way and no partial reconfiguration. It adds the
> > > 'zynq_fpga_manager' so we can use
> > > 
> > > firmwareload -l
> > > firmwareload -t zynq-fpga-manager /mnt/mmc0.0/design_1_wrapper.bit
> > > 
> > > to programm the PL.
> > 
> > The patch copies a lot of code from the PL programming code for the ZynqMP.
> > The main differences are that the Zynq-7000 Bitstream header is 8 bytes
> > shorter than for the ZynqMP and that the Zynq does not go through the
> > firmware, but directly programs the PCAP.
> > 
> > I think that we should extend the ZynqMP FPGA driver to be able to handle 
> > the
> > PL programming of the Zynq-7000 as well instead of adding a new driver for
> > that.
> > 
> > Michael
> > 
> 
> yes, i have learned from drivers/firmware/zynqmp-fpga.c how to deal with the
> firmwareload command and how to validate the bitstream.
> In generally it would be better to only have one driver but since i have no
> access to a ZynqMP board i am kind of shy to touch these bits.

I will happily test your patches on the ZynqMP:) I would really like to have
more users of the zynqmp-fpga driver.

Please CC me on your patches to make sure that I see them.

Michael

> 
> Best regards,
> Michael
> 
> > > 
> > > Signed-off-by: Michael Graichen 
> > > ---
> > >   arch/arm/mach-zynq/Makefile   |   2 +-
> > >   arch/arm/mach-zynq/firmware-zynq.c| 124 ++
> > >   .../mach-zynq/include/mach/firmware-zynq.h|  55 +++
> > >   drivers/firmware/Kconfig  |   7 +
> > >   drivers/firmware/Makefile |   1 +
> > >   drivers/firmware/zynq-fpga.c  | 377 ++
> > >   6 files changed, 565 insertions(+), 1 deletion(-)
> > >   create mode 100644 arch/arm/mach-zynq/firmware-zynq.c
> > >   create mode 100644 arch/arm/mach-zynq/include/mach/firmware-zynq.h
> > >   create mode 100644 drivers/firmware/zynq-fpga.c
> > > 
> > > diff --git a/arch/arm/mach-zynq/Makefile b/arch/arm/mach-zynq/Makefile
> > > index 06c2ce996..2484abe5c 100644
> > > --- a/arch/arm/mach-zynq/Makefile
> > > +++ b/arch/arm/mach-zynq/Makefile
> > > @@ -1,2 +1,2 @@
> > > -obj-y += zynq.o bootm-zynqimg.o
> > > +obj-y += bootm-zynqimg.o firmware-zynq.o zynq.o
> > >   lwl-y += cpu_init.o
> > > diff --git a/arch/arm/mach-zynq/firmware-zynq.c 
> > > b/arch/arm/mach-zynq/firmware-zynq.c
> > > new file mode 100644
> > > index 0..307b22fe5
> > > --- /dev/null
> > > +++ b/arch/arm/mach-zynq/firmware-zynq.c
> > > @@ -0,0 +1,124 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + */
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +
> > > +/*
> > > + * zynq_devc_fpga_load - Perform the fpga load
> > > + * @mgr: FPGA-Manager
> > > + * @address: Address to write to
> > > + * @size:PL bitstream size
> > > + * @flags:   Flags - unused
> > > + *
> > > + * This function provides access to PCAP to transfer
> > > + * the required bitstream into PL.
> > > + *
> > > + * Return:   Returns status, either success or error+reason
> > > + */
> > > +static int zynq_devc_fpga_load(struct fpgamgr *mgr, u64 address,
> > > + u32 size, u32 flags)
> > > +{
> > > + unsigned long reg;
> > > +
> > > + if (!address || !size)
> > > + return -EINVAL;
> > > +
> > > + /*
> > > +  * The Programming Seqenze, see ug585 (v.12.2) Juny 1, 2018 Chapter
> > > +  * 6.4.2 on page 211 Configure the PL via PCAP Bridge Example for
> > > +  * detailed information to this Sequenze
> > > +  */
> > > +
> > > + /* Enable the PCAP bridge and select PCAP for reconfiguration */
> > > + reg = readl(mgr->regs + CTRL_OFFSET);
> > > + reg |= ( CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK );
> > > + writel(reg, mgr->regs + CTRL_OFFSET);
> > > +
> > > + /* Clear the Interrupts */
> > > + writel(0x, mgr->reg

Re: [PATCH v2 7/7] added-zynq-fpga-manager

[Michael Tretter]

Hi Michael,

On Thu, Apr 09, 2020 at 02:44:00PM +, Michael Graichen wrote:
> This adds support to programm the PL part of the Zynq SoC,
> but only the non-secure way and no partial reconfiguration. It adds the
> 'zynq_fpga_manager' so we can use
> 
> firmwareload -l
> firmwareload -t zynq-fpga-manager /mnt/mmc0.0/design_1_wrapper.bit
> 
> to programm the PL.

The patch copies a lot of code from the PL programming code for the ZynqMP.
The main differences are that the Zynq-7000 Bitstream header is 8 bytes
shorter than for the ZynqMP and that the Zynq does not go through the
firmware, but directly programs the PCAP.

I think that we should extend the ZynqMP FPGA driver to be able to handle the
PL programming of the Zynq-7000 as well instead of adding a new driver for
that.

Michael

> 
> Signed-off-by: Michael Graichen 
> ---
>  arch/arm/mach-zynq/Makefile   |   2 +-
>  arch/arm/mach-zynq/firmware-zynq.c| 124 ++
>  .../mach-zynq/include/mach/firmware-zynq.h|  55 +++
>  drivers/firmware/Kconfig  |   7 +
>  drivers/firmware/Makefile |   1 +
>  drivers/firmware/zynq-fpga.c  | 377 ++
>  6 files changed, 565 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/mach-zynq/firmware-zynq.c
>  create mode 100644 arch/arm/mach-zynq/include/mach/firmware-zynq.h
>  create mode 100644 drivers/firmware/zynq-fpga.c
> 
> diff --git a/arch/arm/mach-zynq/Makefile b/arch/arm/mach-zynq/Makefile
> index 06c2ce996..2484abe5c 100644
> --- a/arch/arm/mach-zynq/Makefile
> +++ b/arch/arm/mach-zynq/Makefile
> @@ -1,2 +1,2 @@
> -obj-y += zynq.o bootm-zynqimg.o
> +obj-y += bootm-zynqimg.o firmware-zynq.o zynq.o
>  lwl-y += cpu_init.o
> diff --git a/arch/arm/mach-zynq/firmware-zynq.c 
> b/arch/arm/mach-zynq/firmware-zynq.c
> new file mode 100644
> index 0..307b22fe5
> --- /dev/null
> +++ b/arch/arm/mach-zynq/firmware-zynq.c
> @@ -0,0 +1,124 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * zynq_devc_fpga_load - Perform the fpga load
> + * @mgr: FPGA-Manager
> + * @address: Address to write to
> + * @size:PL bitstream size
> + * @flags:   Flags - unused
> + *
> + * This function provides access to PCAP to transfer
> + * the required bitstream into PL.
> + *
> + * Return:   Returns status, either success or error+reason
> + */
> +static int zynq_devc_fpga_load(struct fpgamgr *mgr, u64 address,
> + u32 size, u32 flags)
> +{
> + unsigned long reg;
> +
> + if (!address || !size)
> + return -EINVAL;
> +
> + /*
> +  * The Programming Seqenze, see ug585 (v.12.2) Juny 1, 2018 Chapter
> +  * 6.4.2 on page 211 Configure the PL via PCAP Bridge Example for
> +  * detailed information to this Sequenze
> +  */
> +
> + /* Enable the PCAP bridge and select PCAP for reconfiguration */
> + reg = readl(mgr->regs + CTRL_OFFSET);
> + reg |= ( CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK );
> + writel(reg, mgr->regs + CTRL_OFFSET);
> +
> + /* Clear the Interrupts */
> + writel(0x, mgr->regs + INT_STS_OFFSET);
> +
> + /* Initialize the PL */
> + reg = readl(mgr->regs + CTRL_OFFSET);
> + reg |= CTRL_PCFG_PROG_B_MASK;
> + writel(reg, mgr->regs + CTRL_OFFSET);
> +
> + reg = readl(mgr->regs + CTRL_OFFSET);
> + reg &= ~CTRL_PCFG_PROG_B_MASK;
> + writel(reg, mgr->regs + CTRL_OFFSET);
> +
> + readl_poll_timeout(mgr->regs + STATUS_OFFSET, reg,
> + !(reg & STATUS_PCFG_INIT_MASK), 100 * USEC_PER_MSEC);
> +
> + reg = readl(mgr->regs + CTRL_OFFSET);
> + reg |= CTRL_PCFG_PROG_B_MASK;
> + writel(reg, mgr->regs + CTRL_OFFSET);
> +
> + /* Clear the Interrupts */
> + writel(0x, mgr->regs + INT_STS_OFFSET);
> +
> + /* Ensure that the PL is ready for programming */
> + readl_poll_timeout(mgr->regs + STATUS_OFFSET, reg,
> + (reg & STATUS_PCFG_INIT_MASK), 100 * USEC_PER_MSEC);
> +
> + /* Check that there is room in the Command Queue */
> + readl_poll_timeout(mgr->regs + STATUS_OFFSET, reg,
> + !(reg & STATUS_DMA_CMD_Q_F_MASK), 100 * USEC_PER_MSEC);
> +
> + /* Disable the PCAP loopback */
> + reg = readl(mgr->regs + MCTRL_OFFSET);
> + reg &= ~MCTRL_INT_PCAP_LPBK_MASK;
> + writel(reg, mgr->regs + MCTRL_OFFSET);
> +
> + /* Program the PCAP_2x clock divider */
> + reg = readl(mgr->regs + CTRL_OFFSET);
> + reg &= ~CTRL_PCAP_RATE_EN_MASK;
> + writel(reg, mgr->regs + CTRL_OFFSET);
> +
> + /* Source Address: Location of bitstream */
> + writel(address, mgr->regs + DMA_SRC_ADDR_OFFSET);
> +
> + /* Destination Address: 0x_ */
> + writel(0x, mgr->regs + DMA_DST_ADDR_OFFSET);
> +
> + /* Source Length: Total number of 32-bit words in the bitstream */
> + writel((size >> 2), mgr->regs + DMA_S

Re: [PATCH 07/21] of: port Linux of_get_compatible_child helper

[Michael Tretter]

Hello Ahmad,

On Tue, Apr 14, 2020 at 09:29:36AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 4/14/20 8:46 AM, Sascha Hauer wrote:
> > for_each_child_of_node() goes down to list_for_each_entry(). On an empty
> > list child will point to the list head after leaving the loop. Return
> > child explicitly when it's compatible and NULL when leaving the loop
> > which is better readable as well.
> 
> argh, it looked fishy to me as well, but I didn't think about comparing
> Linux and barebox implementation. I just reviewed the 70 instances
> of for_each_child_of_node we have and it seems only one other instance
> in the overlay code has made the same assumption as I did:
>  - drivers/of/resolver.c: of_resolve_phandles (grep for /if (!overlay_child)/)

Thanks for reporting. I sent a patch.

Michael

> 
> I can send a fixup to make it more readable but I'd want to change
> for_each_child_of_node to align it with Linux behavior as well.
> 
> Cheers
> Ahmad

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH] mci: arasan: fix missing select on sdhci-helpers

[Michael Tretter]

The arasan driver uses the sdhci-helpers as well, but does not select
the sdhci-helpers.

Add the missing select to fix the compilation of the arasan driver.

Signed-off-by: Michael Tretter 
---
 drivers/mci/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mci/Kconfig b/drivers/mci/Kconfig
index 80b3a26002b4..9f56bed3abbc 100644
--- a/drivers/mci/Kconfig
+++ b/drivers/mci/Kconfig
@@ -141,6 +141,7 @@ config MCI_TEGRA
 
 config MCI_ARASAN
bool "Arasan SDHCI Controller"
+   select MCI_SDHCI
help
  Enable this to support SD and MMC card read/write on systems with
  the Arasan SD3.0 / SDIO3.0 / eMMC4.51 host controller.
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH] of: use of_get_child_by_name() to find child node

[Michael Tretter]

After iterating the children of a node with for_each_child_of_node(),
the child node will never be NULL. If the node was not found,
overlay_child will always point to the first element in the list, which
might or might not be the node that was searched.

Use the of_get_child_by_name() helper function to find the child node
with the name, which does the right thing and returns NULL if the node
is not found.

Reported-by: Ahmad Fatoum 
Signed-off-by: Michael Tretter 
---
 drivers/of/resolver.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 9107c1fbb68c..4f720cf860c2 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -160,9 +160,7 @@ static int adjust_local_phandle_references(struct 
device_node *local_fixups,
}
 
for_each_child_of_node(local_fixups, child) {
-   for_each_child_of_node(overlay, overlay_child)
-   if (!of_node_cmp(child->name, overlay_child->name))
-   break;
+   overlay_child = of_get_child_by_name(overlay, child->name);
if (!overlay_child)
return -EINVAL;
 
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 0/2] Documentation: blspec: update documentation

[Michael Tretter]

Hello,

These are two minor patches to update the documentation of blspec-entries. The
first patch updates the link to the specification and the second patch removed
the documentation of the devicetree-overlay key, which is now part of the
official spec.

Michael

Michael Tretter (2):
  Documentation: blspec: update link to Boot Loader Specification
  Documentation: blspec: drop documentation of devicetree-overlay

 Documentation/user/booting-linux.rst | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] Documentation: blspec: update link to Boot Loader Specification

[Michael Tretter]

There is a big warning on the linked page that the page is obsolete and
a link to the current version of the Boot Loader Specification. The
Barebox documentation shall directly link to the current version of the
specification.

Signed-off-by: Michael Tretter 
---
 Documentation/user/booting-linux.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/user/booting-linux.rst 
b/Documentation/user/booting-linux.rst
index 12cd505e7170..4b0ead1db5a5 100644
--- a/Documentation/user/booting-linux.rst
+++ b/Documentation/user/booting-linux.rst
@@ -158,7 +158,7 @@ Bootloader Spec
 
 barebox supports booting according to the bootloader spec:
 
-http://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/
+https://systemd.io/BOOT_LOADER_SPECIFICATION/
 
 It follows another philosophy than the :ref:`boot_entries`. With Boot Entries
 booting is completely configured in the bootloader. Bootloader Spec Entries
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] Documentation: blspec: drop documentation of devicetree-overlay

[Michael Tretter]

The devicetree-overlay key is part of the official Boot Loader
Specification. There is no need to document it in Barebox, again.

Signed-off-by: Michael Tretter 
---
 Documentation/user/booting-linux.rst | 4 
 1 file changed, 4 deletions(-)

diff --git a/Documentation/user/booting-linux.rst 
b/Documentation/user/booting-linux.rst
index 4b0ead1db5a5..983b56deefb6 100644
--- a/Documentation/user/booting-linux.rst
+++ b/Documentation/user/booting-linux.rst
@@ -232,10 +232,6 @@ device where the entry is found on. This makes it possible 
to use the same rootf
 image on different devices without having to specify a different root= option 
each
 time.
 
-Additionally to the options defined in the original spec, Barebox has the
-``devicetree-overlay`` option. This is a string value that refer to overlays
-that will be applied to the device tree before passing it to Linux.
-
 Network boot
 
 
-- 
2.20.1


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2] firmware: zynqmp-fpga: drop example bin format header

[Michael Tretter]

Avoid the example bitstream header to validate the bitstream that should
be loaded into the FPGA. The header is mostly 0x with a few
special values at a certain offsets and can be better described with the
offsets and their magic values.

As a drive by, this fixes/removes a broken check in the header
validation. The != operator has a higher precedence than ?: and this
check should have had parenthesis around the ?: expression:

bin_header[i] != (byte_order == XILINX_BYTE_ORDER_BIT) ?
  bin_format[i] : __swab32(bin_format[i])

Signed-off-by: Michael Tretter 
Reviewed-by: Thomas Haemmerle 
---
Changes in v2:
 - Fix subject zynmp-fpga -> zynqmp-fpga

 drivers/firmware/zynqmp-fpga.c | 121 +
 1 file changed, 62 insertions(+), 59 deletions(-)

diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
index 1728e2a954..e02667355f 100644
--- a/drivers/firmware/zynqmp-fpga.c
+++ b/drivers/firmware/zynqmp-fpga.c
@@ -24,11 +24,32 @@
 #define ZYNQMP_PM_VERSION_1_1_FEATURES (ZYNQMP_PM_FEATURE_BYTE_ORDER_IRREL | \
 ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED)
 
+/*
+ * Xilinx KU040 Bitstream Composition:
+ *
+ * Bitstream can be provided with an optinal header (`struct bs_header`).
+ * The true bitstream starts with the binary-header composed of 21 words:
+ *
+ *  0: 0x (Dummy pad word)
+ * ...
+ * 15: 0x (Dummy pad word)
+ * 16: 0x00BB (Bus width auto detect word 1)
+ * 17: 0x11220044 (Bus width auto detect word 2)
+ * 18: 0x (Dummy pad word)
+ * 19: 0x (Dummy pad word)
+ * 20: 0xAA995566 (Sync word)
+ *
+ * See Xilinx UG570 (v1.11) September 30 2019, Chapter 9 "Configuration
+ * Details - Bitstream Composition" for further details.
+ */
 #define DUMMY_WORD 0x
-#define BUS_WIDTH_WORD_1   0x00BB
-#define BUS_WIDTH_WORD_2   0x11220044
+#define BUS_WIDTH_AUTO_DETECT1_OFFSET  16
+#define BUS_WIDTH_AUTO_DETECT1 0x00BB
+#define BUS_WIDTH_AUTO_DETECT2_OFFSET  17
+#define BUS_WIDTH_AUTO_DETECT2 0x11220044
+#define SYNC_WORD_OFFSET   20
 #define SYNC_WORD  0xAA995566
-#define SYNC_WORD_OFFS 20
+#define BIN_HEADER_LENGTH  21
 
 enum xilinx_byte_order {
XILINX_BYTE_ORDER_BIT,
@@ -58,48 +79,6 @@ struct bs_header_entry {
char data[0];
 } __attribute__ ((packed));
 
-/*
- * Xilinx KU040 Bitstream Composition:
- * Bitstream can be provided with an optinal header (`struct bs_header`).
- * The true bitstream starts with the binary-header composed of 21 words:
- *
- *  1: 0x (Dummy pad word)
- * ...
- * 16: 0x (Dummy pad word)
- * 17: 0x00BB (Bus width auto detect word 1)
- * 18: 0x11220044 (Bus width auto detect word 2)
- * 19: 0x (Dummy pad word)
- * 20: 0x (Dummy pad word)
- * 21: 0xAA995566 (Sync word)
- *
- * Please refer to  Xilinx UG570 (v1.11) September 30 2019,
- * Chapter 9 Configuration Details - Bitstream Composition
- * for further details!
- */
-static const u32 bin_format[] = {
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   BUS_WIDTH_WORD_1,
-   BUS_WIDTH_WORD_2,
-   DUMMY_WORD,
-   DUMMY_WORD,
-   SYNC_WORD,
-};
-
 static void copy_words_swapped(u32 *dst, const u32 *src, size_t size)
 {
int i;
@@ -111,36 +90,59 @@ static void copy_words_swapped(u32 *dst, const u32 *src, 
size_t size)
 static int get_byte_order(const u32 *bin_header, size_t size,
  enum xilinx_byte_order *byte_order)
 {
-   if (size < sizeof(bin_format))
+   if (size < BIN_HEADER_LENGTH * sizeof(*bin_header))
return -EINVAL;
 
-   if (bin_header[SYNC_WORD_OFFS] == SYNC_WORD) {
+   if (bin_header[SYNC_WORD_OFFSET] == SYNC_WORD) {
*byte_order = XILINX_BYTE_ORDER_BIT;
return 0;
}
 
-   if (bin_header[SYNC_WORD_OFFS] == __swab32(SYNC_WORD)) {
-   *byte_order =  XILINX_BYTE_ORDER_BIN;
+   if (bin_header[SYNC_WORD_OFFSET] == __swab32(SYNC_WORD)) {
+   *byte_order = XILINX_BYTE_ORDER_BIN;
return 0;
}
 
return -EINVAL;
 }
 
-static int is_bin_header_valid(const u32 *bin_header, size_t size,
-  enum xilinx_byte_order byte_order)
+static bool is_bin_header_valid(const u32 *bin_header, size_t size,
+   enum xilinx_byte_order byte_order)
 {
-   int i;
+   size_t i;
 
-   if (size < ARRAY_SIZE(bin_format))
-   return 0;
+   if (size < BIN_HEADER_LE