Re: [PATCH v2 1/2] part: Add a function to find ESP partition
Hi Heinrich,
On Tue, 2024-01-16 at 17:14 +0100, Heinrich Schuchardt wrote:
> On 16.01.24 14:45, Heinrich Schuchardt wrote:
> > On 16.01.24 13:36, Mayuresh Chitale wrote:
> > > If a disk has an EFI system partition (ESP) then it can be used
> > > to
> > > locate the boot files. Add a function to find the ESP.
> > >
> > > Signed-off-by: Mayuresh Chitale
> > > Reviewed-by: Heinrich Schuchardt <
> > > heinrich.schucha...@canonical.com>
>
> I ran your patches through Gitlab CI and some issues came up:
>
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/771497
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/771498
>
> Could you, please, have a look at it.
I looked into the log files below:
https://u-boot.source-pages.denx.de/-/custodians/u-boot-efi/-/jobs/771497/artifacts/test-log.html
https://u-boot.source-pages.denx.de/-/custodians/u-boot-efi/-/jobs/771498/artifacts/test-log.html
If those logs are correct, I think the failure is probably because I
removed fallback to the user provided partition in case the ESP probe
failed for some reason. I think that fallback is required here.
>
> Best regards
>
> Heinrich
>
> > > ---
> > > disk/part.c| 16
> > > include/part.h | 11 +++
> > > 2 files changed, 27 insertions(+)
> > >
> > > diff --git a/disk/part.c b/disk/part.c
> > > index 36b88205eca..6b1fbc18637 100644
> > > --- a/disk/part.c
> > > +++ b/disk/part.c
> > > @@ -848,3 +848,19 @@ int part_get_bootable(struct blk_desc *desc)
> > > return 0;
> > > }
> > > +
> > > +int part_get_esp(struct blk_desc *desc)
> > > +{
> > > +struct disk_partition info;
> > > +int p;
> > > +
> > > +for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> > > +int ret;
> > > +
> > > +ret = part_get_info(desc, p, );
> > > +if (!ret && (info.bootable & PART_EFI_SYSTEM_PARTITION))
> > > +return p;
> > > +}
> > > +
> > > +return 0;
> > > +}
> > > diff --git a/include/part.h b/include/part.h
> > > index db34bc6bb7d..30e049c8f19 100644
> > > --- a/include/part.h
> > > +++ b/include/part.h
> > > @@ -690,6 +690,14 @@ int part_get_type_by_name(const char *name);
> > >*/
> > > int part_get_bootable(struct blk_desc *desc);
> > > +/**
> > > + * part_get_esp() - Find the EFI system partition
> > > + *
> > > + * @desc: Block-device descriptor
> > > + * @Return the EFI system partition, or 0 if there is none
> >
> > We want to be able to add the include to our API documentation.
> > This
> > requires adhering to the Sphinx documentation style.
> >
> > %s/@Return the/Return:/
> >
> > Cf.
> > https://www.kernel.org/doc/html/v6.7/doc-guide/kernel-doc.html#function-documentation
> >
> > Best regards
> >
> > Heinrich
> >
> > > + */
> > > +int part_get_esp(struct blk_desc *desc);
> > > +
> > > #else
> > > static inline int part_driver_get_count(void)
> > > { return 0; }
> > > @@ -700,6 +708,9 @@ static inline struct part_driver
> > > *part_driver_get_first(void)
> > > static inline bool part_get_bootable(struct blk_desc *desc)
> > > { return false; }
> > > +static inline bool part_get_esp(struct blk_desc *desc)
> > > +{ return false; }
> > > +
> > > #endif /* CONFIG_PARTITIONS */
> > > #endif /* _PART_H */
Re: [PATCH v2] drivers: pcie_xilinx: Fix "reg" not found error
On Mon, 2023-11-13 at 10:12 +0100, Michal Simek wrote: > one more thing here. > > Subject of your next patch is > [PATCH v2] pci: xilinx: Enable MMIO region > and here you are using drivers: pcie_xilinx: > > Please use one if you target the same code. > I prefer "pci: xilinx:" style. Ok. > > Thanks, > Michal > >
Re: [PATCH v2] drivers: pcie_xilinx: Fix "reg" not found error
On Mon, 2023-11-13 at 09:10 +0100, Michal Simek wrote:
>
> On 11/11/23 18:36, Mayuresh Chitale wrote:
> > Fix the driver to use the dev_read_addr_size API to fetch the reg
> > property from the DT.
> >
> > Signed-off-by: Mayuresh Chitale
> > ---
> > Changes in v2:
> >
> > - Remove global_data.h from include
> > - Use devm_ioremap instead of map_phsymem
> >
> > drivers/pci/pcie_xilinx.c | 26 +-
> > 1 file changed, 9 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/pci/pcie_xilinx.c b/drivers/pci/pcie_xilinx.c
> > index 53fd121e90..c1f51b 100644
> > --- a/drivers/pci/pcie_xilinx.c
> > +++ b/drivers/pci/pcie_xilinx.c
> > @@ -8,11 +8,9 @@
> > #include
> > #include
> > #include
> > -#include
> > #include
> > #include
> > -
> > -#include
> > +#include
> >
> > /**
> >* struct xilinx_pcie - Xilinx PCIe controller state
> > @@ -140,20 +138,14 @@ static int pcie_xilinx_write_config(struct
> > udevice *bus, pci_dev_t bdf,
> > static int pcie_xilinx_of_to_plat(struct udevice *dev)
> > {
> > struct xilinx_pcie *pcie = dev_get_priv(dev);
> > - struct fdt_resource reg_res;
> > - DECLARE_GLOBAL_DATA_PTR;
> > - int err;
> > -
> > - err = fdt_get_resource(gd->fdt_blob, dev_of_offset(dev), "reg",
> > - 0, _res);
> > - if (err < 0) {
> > - pr_err("\"reg\" resource not found\n");
> > - return err;
> > - }
> > -
> > - pcie->cfg_base = map_physmem(reg_res.start,
> > -fdt_resource_size(_res),
> > -MAP_NOCACHE);
> > + fdt_addr_t addr;
> > + fdt_size_t size;
> > +
> > + addr = dev_read_addr_size(dev, );
> > + if (addr == FDT_ADDR_T_NONE)
> > + return -EINVAL;
> > +
> > + pcie->cfg_base = devm_ioremap(dev, addr, size);
>
> this can also fail.
>
> Just put there.
> if (IS_ERR(pcie->cfg_base))
> return PTR_ERR(pcie->cfg_base);
Ok.
>
> M
Re: [PATCH v2] net: axi_emac: Use reg property for DMA registers
Hi Michal,
On Mon, 2023-11-13 at 10:10 +0100, Michal Simek wrote:
>
> On 11/11/23 18:36, Mayuresh Chitale wrote:
> > As per the xlnx,axi-ethernet-1.00.a DT documentation in linux, the
> > AXI
> > DMA registers can be obtained via the reg property or via a
> > separate
> > node for the axistream DMA controller. Currently only the latter is
> > supported, so add support to fetch the DMA controller registers
> > from the
> > "reg" property.
> >
> > Signed-off-by: Mayuresh Chitale
> > Reviewed-by: Michal Simek
> > ---
> > Changes in v2:
> >
> > Add Reviewed-by Tag.
> >
> > drivers/net/xilinx_axi_emac.c | 9 -
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/xilinx_axi_emac.c
> > b/drivers/net/xilinx_axi_emac.c
> > index 54f2232768..ef151ee51b 100644
> > --- a/drivers/net/xilinx_axi_emac.c
> > +++ b/drivers/net/xilinx_axi_emac.c
> > @@ -903,12 +903,11 @@ static int axi_emac_of_to_plat(struct udevice
> > *dev)
> >
> > ret = dev_read_phandle_with_args(dev, "axistream-connected",
> > NULL, 0, 0,
> > _node);
> > - if (ret) {
> > - printf("%s: axistream is not found\n", __func__);
> > - return -EINVAL;
> > - }
> > + if (!ret)
> > + plat->dmatx = (struct axidma_reg
> > *)ofnode_get_addr(axistream_node.node);
> > + else
> > + plat->dmatx = (struct axidma_reg
> > *)dev_read_addr_index(dev, 1);
> >
> > - plat->dmatx = (struct axidma_reg
> > *)ofnode_get_addr(axistream_node.node);
> > if (!plat->dmatx) {
> > printf("%s: axi_dma register space not found\n",
> > __func__);
> > return -EINVAL;
>
> This is the part of pcie series but it has nothing to do with it. Can
> you please
> send it separately?
Yes, actually I intended to send this patch as well as the PCIe patches
as individual patches but the way I invoked send-email caused those to
become a series.
>
> M
Re: [PATCH v1 3/3] drivers: xilinx_spi: Probe fifo_depth at runtime
On Mon, 2023-11-13 at 10:06 +0100, Michal Simek wrote:
>
> On 11/11/23 18:31, Mayuresh Chitale wrote:
> > If the fifo-size DT parameter is not provided then probe the
> > controller's fifo depth at runtime. This is ported from a patch
> > in the Linux Xilinx SPI driver.
> >
> > Signed-off-by: Mayuresh Chitale
> > Link:
> > https://lore.kernel.org/r/1422029330-10971-5-git-send-email-ricardo.riba...@gmail.com
> > ---
> > drivers/spi/xilinx_spi.c | 23 +++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
> > index b63cda2091..99ae5be291 100644
> > --- a/drivers/spi/xilinx_spi.c
> > +++ b/drivers/spi/xilinx_spi.c
> > @@ -109,6 +109,27 @@ struct xilinx_spi_priv {
> > u8 startup;
> > };
> >
> > +static int xilinx_spi_find_buffer_size(struct xilinx_spi_regs
> > *regs)
> > +{
> > + u8 sr;
> > + int n_words = 0;
> > +
> > + /*
> > +* Before the buffer_size detection we reset the core
> > +* to make sure we start with a clean state.
>
> nit: can you please remove "we" above? It should be written in
> imperative mood.
Ok.
>
>
> > +*/
> > + writel(SPISSR_RESET_VALUE, >srr);
> > +
> > + /* Fill the Tx FIFO with as many words as possible */
> > + do {
> > + writel(0, >spidtr);
> > + sr = readl(>spisr);
> > + n_words++;
> > + } while (!(sr & SPISR_TX_FULL));
> > +
> > + return n_words;
> > +}
> > +
> > static int xilinx_spi_probe(struct udevice *bus)
> > {
> > struct xilinx_spi_priv *priv = dev_get_priv(bus);
> > @@ -116,6 +137,8 @@ static int xilinx_spi_probe(struct udevice
> > *bus)
> >
> > regs = priv->regs = dev_read_addr_ptr(bus);
> > priv->fifo_depth = dev_read_u32_default(bus, "fifo-size", 0);
> > + if (!priv->fifo_depth)
> > + priv->fifo_depth = xilinx_spi_find_buffer_size(regs);
> >
> > writel(SPISSR_RESET_VALUE, >srr);
> >
>
> When above fixed feel free to add
> Reviewed-by: Michal Simek
Ok.
>
> Thanks,
> Michal
Re: [PATCH] drivers: pcie_xilinx: Fix "reg" not found error
Hi Michal,
On Thu, 2023-11-02 at 10:05 +0100, Michal Simek wrote:
>
> On 11/2/23 09:23, Mayuresh Chitale wrote:
> > Fix the driver to use the dev_read_addr_size API to fetch the reg
> > property from the DT.
> >
> > Signed-off-by: Mayuresh Chitale
> > ---
> > drivers/pci/pcie_xilinx.c | 22 --
> > 1 file changed, 8 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/pci/pcie_xilinx.c b/drivers/pci/pcie_xilinx.c
> > index 53fd121e90..20b630027f 100644
> > --- a/drivers/pci/pcie_xilinx.c
> > +++ b/drivers/pci/pcie_xilinx.c
> > @@ -140,20 +140,14 @@ static int pcie_xilinx_write_config(struct
> > udevice *bus, pci_dev_t bdf,
> > static int pcie_xilinx_of_to_plat(struct udevice *dev)
> > {
> > struct xilinx_pcie *pcie = dev_get_priv(dev);
> > - struct fdt_resource reg_res;
> > - DECLARE_GLOBAL_DATA_PTR;
> > - int err;
> > -
> > - err = fdt_get_resource(gd->fdt_blob, dev_of_offset(dev), "reg",
>
> I don't have HW to test this but here you are removing reference to
> GD that's
> why I think you should also remove
>
> #include
Ok.
>
>
>
>
> > - 0, _res);
> > - if (err < 0) {
> > - pr_err("\"reg\" resource not found\n");
> > - return err;
> > - }
> > -
> > - pcie->cfg_base = map_physmem(reg_res.start,
> > -fdt_resource_size(_res),
> > -MAP_NOCACHE);
> > + fdt_addr_t addr;
> > + fdt_size_t size;
> > +
> > + addr = dev_read_addr_size(dev, );
> > + if (addr == FDT_ADDR_T_NONE)
> > + return -EINVAL;
> > +
> > + pcie->cfg_base = map_physmem(addr, size, MAP_NOCACHE);
>
> Definitely it is better then what was there before.
>
> But can't use use devm_ioremap() instead?
Ok.
>
> Thanks,
> Michal
Re: [PATCH] net: axi_emac: Use reg property for DMA registers
Hi Michal,
On Thu, 2023-11-02 at 10:15 +0100, Michal Simek wrote:
>
> On 11/2/23 09:23, Mayuresh Chitale wrote:
> > As per the xlnx,axi-ethernet-1.00.a DT documentation in linux, the
> > AXI
> > DMA registers can be obtained via the reg property or via a
> > separate
> > node for the axistream DMA controller. Currently only the latter is
> > supported, so add support to fetch the DMA controller registers
> > from the
> > "reg" property.
> >
> > Signed-off-by: Mayuresh Chitale
> > ---
> > drivers/net/xilinx_axi_emac.c | 9 -
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/xilinx_axi_emac.c
> > b/drivers/net/xilinx_axi_emac.c
> > index 54f2232768..ef151ee51b 100644
> > --- a/drivers/net/xilinx_axi_emac.c
> > +++ b/drivers/net/xilinx_axi_emac.c
> > @@ -903,12 +903,11 @@ static int axi_emac_of_to_plat(struct udevice
> > *dev)
> >
> > ret = dev_read_phandle_with_args(dev, "axistream-connected",
> > NULL, 0, 0,
> > _node);
> > - if (ret) {
> > - printf("%s: axistream is not found\n", __func__);
> > - return -EINVAL;
> > - }
> > + if (!ret)
> > + plat->dmatx = (struct axidma_reg
> > *)ofnode_get_addr(axistream_node.node);
> > + else
> > + plat->dmatx = (struct axidma_reg
> > *)dev_read_addr_index(dev, 1);
> >
> > - plat->dmatx = (struct axidma_reg
> > *)ofnode_get_addr(axistream_node.node);
> > if (!plat->dmatx) {
> > printf("%s: axi_dma register space not found\n",
> > __func__);
> > return -EINVAL;
>
> This looks good to me. Can you please send it separately?
> It is really independent of change from other patches.
Actually it is a separate patch.
>
> Reviewed-by: Michal Simek
>
> Thanks,
> Michal
Re: [PATCH] drivers: xilinx_spi: Fixes for MMC_SPI
Hi Michal,
On Thu, 2023-11-02 at 09:37 +0100, Michal Simek wrote:
> On 11/2/23 09:23, Mayuresh Chitale wrote:
> > Add the xfer callback which is used by the MMC_SPI driver and
> > generally by
> > the dm_spi_xfer callback. Also probe the fifo_depth during init as
> > is
> > done in the linux spi-xilinx driver and fix a compiler warning
> > while at it.
>
> They are independent things that's why please separate them by that
> topics.
>
> Also update subject to describe issue there.
Ok.
>
> > Signed-off-by: Mayuresh Chitale
> > ---
> > drivers/spi/xilinx_spi.c | 70 ++-
> > -
> > 1 file changed, 53 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
> > index b58a3f632a..0d4ff25600 100644
> > --- a/drivers/spi/xilinx_spi.c
> > +++ b/drivers/spi/xilinx_spi.c
> > @@ -67,7 +67,7 @@
> > /* SPI Slave Select Register (spissr), [1] p13, [2] p13 */
> > #define SPISSR_MASK(cs) (1 << (cs))
> > #define SPISSR_ACT(cs)~SPISSR_MASK(cs)
> > -#define SPISSR_OFF ~0UL
> > +#define SPISSR_OFF (~0U)
> >
> > /* SPI Software Reset Register (ssr) */
> > #define SPISSR_RESET_VALUE0x0a
> > @@ -109,6 +109,27 @@ struct xilinx_spi_priv {
> > u8 startup;
> > };
> >
> > +/* Detect fifo depth. Ported from linux sha: 4c9a761402d78 */
>
> don't add linux sha1 here.
> Add it to commit message or I normally just provide a link to that
> patch.
> https://lore.kernel.org/r/1422029330-10971-5-git-send-email-ricardo.riba...@gmail.com
Ok.
>
>
>
> > +static int xilinx_spi_find_buffer_size(struct xilinx_spi_regs
> > *regs)
> > +{
> > + u8 sr;
> > + int n_words = 0;
> > +
> > + /*
> > +* Before the buffer_size detection we reset the core
> > +* to make sure we start with a clean state.
> > +*/
> > + writel(SPISSR_RESET_VALUE, >srr);
> > +
> > + /* Fill the Tx FIFO with as many words as possible */
> > + do {
> > + writel(0, >spidtr);
> > + sr = readl(>spisr);
> > + n_words++;
> > + } while (!(sr & SPISR_TX_FULL));
> > +
> > + return n_words;
> > +}
>
> newline here.
Ok.
>
> > static int xilinx_spi_probe(struct udevice *bus)
> > {
> > struct xilinx_spi_priv *priv = dev_get_priv(bus);
> > @@ -116,6 +137,8 @@ static int xilinx_spi_probe(struct udevice
> > *bus)
> >
> > regs = priv->regs = dev_read_addr_ptr(bus);
> > priv->fifo_depth = dev_read_u32_default(bus, "fifo-size", 0);
> > + if (!priv->fifo_depth)
> > + priv->fifo_depth = xilinx_spi_find_buffer_size(regs);
> >
> > writel(SPISSR_RESET_VALUE, >srr);
> >
> > @@ -197,7 +220,7 @@ static u32 xilinx_spi_fill_txfifo(struct
> > udevice *bus, const u8 *txp,
> > return i;
> > }
> >
> > -static u32 xilinx_spi_read_rxfifo(struct udevice *bus, u8 *rxp,
> > u32 rxbytes)
> > +static u32 xilinx_spi_read_rxfifo(struct udevice *bus, u8 *rxp,
> > int rxbytes)
>
> Unrelated. Or do we have a case where we ask for reading negative
> number of bytes?
I will revert the change.
>
> > {
> > struct xilinx_spi_priv *priv = dev_get_priv(bus);
> > struct xilinx_spi_regs *regs = priv->regs;
> > @@ -217,9 +240,9 @@ static u32 xilinx_spi_read_rxfifo(struct
> > udevice *bus, u8 *rxp, u32 rxbytes)
> > return i;
> > }
> >
> > -static int start_transfer(struct spi_slave *spi, const void *dout,
> > void *din, u32 len)
> > +static int start_transfer(struct udevice *dev, const void *dout,
> > void *din, u32 len)
>
> All these spi -> dev changes should be in separate patch. I expect
> they are
> preparation for xfer but still better to do it separately.
Ok.
>
> > {
> > - struct udevice *bus = spi->dev->parent;
> > + struct udevice *bus = dev->parent;
> > struct xilinx_spi_priv *priv = dev_get_priv(bus);
> > struct xilinx_spi_regs *regs = priv->regs;
> > u32 count, txbytes, rxbytes;
> > @@ -259,10 +282,9 @@ static int start_transfer(struct spi_slave
> > *spi, const void *dout, void *din, u3
> > return 0;
> > }
> >
> > -static void xilinx_spi_startup_block(struct spi_slave *spi)
> > +static void xilinx_spi_startup_block(struct udevice *dev)
> > {
> > - struct dm_spi_slave_plat *slave_plat =
> > - dev_get_parent_plat(spi->dev);
> > + struct dm_spi_slave_plat *slave_plat =
> > dev_get_parent_plat(dev);
> > unsigned char txp;
> > unsigned char rxp[8];
> >
> > @@ -270,13 +292,25 @@ static void xilinx_spi_startup_block(struct
> > spi_slave *spi)
> > * Perform a dummy read as a work around for
> > * the startup block issue.
> > */
> > - spi_cs_activate(spi->dev, slave_plat->cs);
> > + spi_cs_activate(dev, slave_plat->cs);
> > txp = 0x9f;
> > - start_transfer(spi, (void *), NULL, 1);
> > + start_transfer(dev, (void *), NULL, 1);
> >
> > - start_transfer(spi, NULL, (void *)rxp, 6);
> > + start_transfer(dev, NULL, (void *)rxp, 6);
> >
> > -
Re: [PATCH v1] fs: Fix SPL build if FS_LOADER is enabled
On Thu, 2023-09-14 at 12:39 -0400, Tom Rini wrote:
> On Thu, Sep 14, 2023 at 09:35:15PM +0530, Mayuresh Chitale wrote:
>
> > If FS_LOADER is enabled for the SPL then the build fails with the
> > error:
> >
> > fs/fs.o:(.data.rel.fstypes+0x128):
> > undefined reference to `smh_fs_set_blk_dev'
> > fs/fs.o:(.data.rel.fstypes+0x140):
> > undefined reference to `smh_fs_size'
> > fs/fs.o:(.data.rel.fstypes+0x148):
> > undefined reference to `smh_fs_read'
> > fs/fs.o:(.data.rel.fstypes+0x150):
> > undefined reference to `smh_fs_write'
> >
> > Fix the error by populating the semihosting entry in the fs_types
> > array
> > only for non-SPL builds.
> >
> > Reviewed-by: Heinrich Schuchardt > >
> > Signed-off-by: Mayuresh Chitale
> > ---
> > fs/fs.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/fs.c b/fs/fs.c
> > index 2b815b1db0..074db4b20f 100644
> > --- a/fs/fs.c
> > +++ b/fs/fs.c
> > @@ -256,6 +256,7 @@ static struct fstype_info fstypes[] = {
> > .ln = fs_ln_unsupported,
> > },
> > #endif
> > +#ifndef CONFIG_SPL_BUILD
> > #ifdef CONFIG_SEMIHOSTING
> > {
> > .fstype = FS_TYPE_SEMIHOSTING,
>
> This should be CONFIG_IS_ENABLED(SEMIHOSTING).
Ok. I will update it.
>
Re: [PATCH v1 1/2] part: Add a function to find ESP partition
On Thu, 2023-09-14 at 12:29 -0400, Tom Rini wrote:
> On Thu, Sep 14, 2023 at 03:38:20PM +0530, Mayuresh Chitale wrote:
> > If a disk has an EFI system partition (ESP) then it can be used to
> > locate the boot files. Add a function to find the ESP.
> >
> > Signed-off-by: Mayuresh Chitale
> > Reviewed-by: Heinrich Schuchardt > >
> > ---
> > disk/part.c| 16
> > include/part.h | 10 ++
> > 2 files changed, 26 insertions(+)
> >
> > diff --git a/disk/part.c b/disk/part.c
> > index 72241b7b23..2be0866671 100644
> > --- a/disk/part.c
> > +++ b/disk/part.c
> > @@ -841,3 +841,19 @@ int part_get_bootable(struct blk_desc *desc)
> >
> > return 0;
> > }
> > +
> > +int part_get_esp(struct blk_desc *desc)
> > +{
> > + struct disk_partition info;
> > + int p;
> > +
> > + for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> > + int ret;
> > +
> > + ret = part_get_info(desc, p, );
> > + if (!ret && (info.bootable &
> > PART_EFI_SYSTEM_PARTITION))
> > + return p;
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/include/part.h b/include/part.h
> > index db34bc6bb7..036f9fd762 100644
> > --- a/include/part.h
> > +++ b/include/part.h
> > @@ -689,6 +689,13 @@ int part_get_type_by_name(const char *name);
> > * @return first bootable partition, or 0 if there is none
> > */
> > int part_get_bootable(struct blk_desc *desc);
> > +/**
>
> Missing blank line?
>
Ok, I will add it.
Re: [PATCH v1 2/2] spl: Add support for booting from ESP
On Thu, 2023-09-14 at 12:29 -0400, Tom Rini wrote:
> On Thu, Sep 14, 2023 at 03:38:21PM +0530, Mayuresh Chitale wrote:
>
> > Some platforms as described by EBBR specification may store images
> > in
> > the FIRMWARE directory of the UEFI system partition(ESP). Add
> > support
> > to boot from the EFI system partition if it is enabled for a
> > platform.
> >
> > Signed-off-by: Mayuresh Chitale
> [snip]
> > +config SPL_ESP_BOOT
> > + bool "Load next stage boot image from the UEFI system
> > partition"
> > + select SPL_PARTITION_TYPE_GUID
> > + help
> > + When enabled, first try to boot from the UEFI system
> > partition as
> > + described in the Ch.4 of the EBBR specification.
>
> We need to select this by perhaps BOOT_DEFAULTS or BOOTMETH_DISTRO,
> whatever is supposed to signify that yes, this is going to be a
> SystemReady IR (or higher) compliant build.
I am not sure about the SystemReady compliance but I think the config
option can be enabled for those platforms which require it.
> [snip]
> > int spl_blk_load_image(struct spl_image_info *spl_image,
> >struct spl_boot_device *bootdev,
> >enum uclass_id uclass_id, int devnum, int
> > partnum)
> > {
> > const char *filename = CONFIG_SPL_FS_LOAD_PAYLOAD_NAME;
> > - struct legacy_img_hdr *header;
> [snip]
> > @@ -63,24 +88,26 @@ int spl_blk_load_image(struct spl_image_info
> > *spl_image,
> > }
> >
> > blk_show_device(uclass_id, devnum);
> > - header = spl_get_load_buffer(-sizeof(*header),
> > sizeof(*header));
> > -
> [snip]
>
> Is this an artifact of Sean's patch series where "header" is no
> longer
> actually used? As-is this won't compile on top of next (which is
> where
> given Sean's feedback, the series needs to be rebased upon anyhow).
Yes, I will rebase on the latest next without Sean's series and resend
the patch.
> > + /*
> > +* First try to boot from EFI System partition. In case of
> > failure,
> > +* fall back to the configured partition.
> > +*/
>
> I don't know that this is the right behavior. If we're configured to
> boot from partition X, we boot from partition X. If we're configured
> to
> find the ESP and use that, we find and use that.
This is to maintain compatibility and make more attempts to boot. So
the same SPL binary can load images from devices with or without the
ESP.
Re: [PATCH v2 1/1] spl: initialize PCI before booting
On Mon, 2023-07-24 at 22:18 +0200, Heinrich Schuchardt wrote:
> MMC, SATA, and USB may be using PCI based controllers.
> Initialize the PCI sub-system before trying to boot.
>
> Remove the initialization for NVMe that is now redundant.
>
> Signed-off-by: Heinrich Schuchardt >
> ---
> v2:
> Centralize the PCI initialization
> ---
> common/spl/spl.c | 7 +++
> common/spl/spl_nvme.c | 5 -
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index f09bb97781..0062f3f45d 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -800,6 +800,13 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
> IS_ENABLED(CONFIG_SPL_ATF))
> dram_init_banksize();
>
> + if (CONFIG_IS_ENABLED(PCI)) {
> + ret = pci_init();
> + if (ret)
> + puts(SPL_TPL_PROMPT "Cannot initialize PCI\n");
> + /* Don't fail. We still can try other boot methods. */
> + }
> +
> bootcount_inc();
>
> /* Dump driver model states to aid analysis */
> diff --git a/common/spl/spl_nvme.c b/common/spl/spl_nvme.c
> index 2af63f1dc8..c8774d67ec 100644
> --- a/common/spl/spl_nvme.c
> +++ b/common/spl/spl_nvme.c
> @@ -7,7 +7,6 @@
>
> #include
> #include
> -#include
> #include
>
> static int spl_nvme_load_image(struct spl_image_info *spl_image,
> @@ -15,10 +14,6 @@ static int spl_nvme_load_image(struct
> spl_image_info *spl_image,
> {
> int ret;
>
> - ret = pci_init();
> - if (ret < 0)
> - return ret;
> -
> ret = nvme_scan_namespace();
> if (ret < 0)
> return ret;
Reviewed-by: Mayuresh Chitale
Re: [PATCH 1/1] spl: blk: partition numbers are hexadecimal
On Sat, 2023-07-22 at 12:45 +0200, Heinrich Schuchardt wrote:
> Loading u-boot.itb from device 0x00, partition 0x0f fails with:
>
> Trying to boot from NVME
>
> Device 0: Vendor: 0x4x Rev: 8.0.50 Prod: nvme-1
> Type: Hard Disk
> Capacity: 3814.6 MB = 3.7 GB (7812500 x 512)
> ** Invalid partition 21 **
> Couldn't find partition nvme 0:15
>
> Like the command line interface fs_det_blk_dev() expects that the
> device
> number and the partition number are hexadecimal.
>
> Fixes: 8ce6a2e17577 ("spl: blk: Support loading images from fs")
> Signed-off-by: Heinrich Schuchardt >
> ---
> common/spl/spl_blk_fs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/spl/spl_blk_fs.c b/common/spl/spl_blk_fs.c
> index d4161fa850..16ecece702 100644
> --- a/common/spl/spl_blk_fs.c
> +++ b/common/spl/spl_blk_fs.c
> @@ -66,7 +66,7 @@ int spl_blk_load_image(struct spl_image_info
> *spl_image,
> }
>
> dev.ifname = blk_get_uclass_name(uclass_id);
> - snprintf(dev.dev_part_str, sizeof(dev.dev_part_str) - 1,
> "%d:%d",
> + snprintf(dev.dev_part_str, sizeof(dev.dev_part_str) - 1,
> "%x:%x",
>devnum, partnum);
> ret = fs_set_blk_dev(dev.ifname, dev.dev_part_str,
> FS_TYPE_ANY);
> if (ret) {
Reviewed-by: Mayuresh Chitale
Re: [PATCH 1/1] spl: blk: use CONFIG_SPL_FS_LOAD_PAYLOAD_NAME
On Fri, 2023-07-21 at 14:09 +0200, Heinrich Schuchardt wrote:
> We should target to unify the code for different block devices in SPL
> to
> reduce code size.
>
> MMC, USB, SATA, and Semihosting use CONFIG_SPL_FS_LOAD_PAYLOAD_NAME
> to
> indicate the filename to load.
>
> NVMe uses CONFIG_SPL_PAYLOAD in spl_blk_load_image().
>
> CONFIG_SPL_PAYLOAD is meant to define which binary to integrate into
> u-boot-with-spl.bin. See commit
> 7550dbe38b3f ("spl: Add option SPL_PAYLOAD").
>
> Change spl_blk_load_image() to use CONFIG_SPL_FS_LOAD_PAYLOAD_NAME.
>
> Fixes: 8ce6a2e17577 ("spl: blk: Support loading images from fs")
> Signed-off-by: Heinrich Schuchardt >
> ---
> common/spl/spl_blk_fs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/spl/spl_blk_fs.c b/common/spl/spl_blk_fs.c
> index d97adc4d39..d4161fa850 100644
> --- a/common/spl/spl_blk_fs.c
> +++ b/common/spl/spl_blk_fs.c
> @@ -43,7 +43,7 @@ int spl_blk_load_image(struct spl_image_info
> *spl_image,
> struct spl_boot_device *bootdev,
> enum uclass_id uclass_id, int devnum, int
> partnum)
> {
> - const char *filename = CONFIG_SPL_PAYLOAD;
> + const char *filename = CONFIG_SPL_FS_LOAD_PAYLOAD_NAME;
> struct disk_partition part_info = {};
> struct legacy_img_hdr *header;
> struct blk_desc *blk_desc;
Reviewed-by: Mayuresh Chitale
Re: [PATCH v4 0/4] SPL NVMe support
On Mon, 2023-07-17 at 11:12 -0400, Tom Rini wrote:
> On Mon, Jul 17, 2023 at 01:39:52PM +0530, mchit...@ventanamicro.com
> wrote:
> > On Wed, 2023-07-12 at 13:12 -0400, Tom Rini wrote:
> > > On Wed, Jul 12, 2023 at 03:27:45PM +0200, Heinrich Schuchardt
> > > wrote:
> > > > On 12.07.23 15:06, mchit...@ventanamicro.com wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Tue, 2023-06-20 at 09:37 -0400, Tom Rini wrote:
> > > > > > On Sat, 03 Jun 2023 19:32:52 +0530, Mayuresh Chitale wrote:
> > > > > >
> > > > > > > This patchset adds support to load images of the SPL's
> > > > > > > next
> > > > > > > booting
> > > > > > > stage from a NVMe device.
> > > > > > >
> > > > > > > Changes in v4:
> > > > > > > - Drop patch 4
> > > > > > > - Modify patch 2 to use generic fs.h APIs
> > > > > > >
> > > > > > > [...]
> > > > > >
> > > > > > With one change, which is that the "disk/part.c" in 4/4
> > > > > > were
> > > > > > not
> > > > > > required for
> > > > > > any platform in tree and also broke testcases, and so was
> > > > > > dropped,
> > > > > > this has now
> > > > > > been applied to u-boot/next. If you can explain a bit more
> > > > > > what
> > > > > > the
> > > > > > problem you
> > > > > > had was, we can look in to it. I suspect you need to test
> > > > > > for
> > > > > > not
> > > > > > SPL_ENV_SUPPORT but ENV_SUPPORT itself.
> > > > > >
> > > > > Thanks.
> > > > > When SPL_NVME is enabled the build breaks with the following
> > > > > error:
> > > > > riscv64-unknown-linux-gnu-ld.bfd: disk/part.o: in function
> > > > > `blk_get_device_part_str':
> > > > > u-boot/disk/part.c:473: undefined reference to `env_get'
> > > > > make[2]: *** [u-boot/scripts/Makefile.spl:527: spl/u-boot-
> > > > > spl]
> > > > > Error 1
> > > > > make[1]: *** [/u-boot/Makefile:2053: spl/u-boot-spl] Error 2
> > > > >
> > > > > One possible fix is:
> > > > >
> > > > > if ((!IS_ENABLED(CONFIG_SPL) &&
> > > > > IS_ENABLED(CONFIG_ENV_SUPPORT))
> > > > > (IS_ENABLED(CONFIG_SPL) &&
> > > > > IS_ENABLED(CONFIG_SPL_ENV_SUPPORT)))
> > > > > if (!dev_part_str || !strlen(dev_part_str)
> > > > > > > !strcmp(dev_part_str, "-"))
> > > > > dev_part_str = env_get("bootdevice");
> > > > >
> > > > >
> > > >
> > > > I think CONFIG_SPL_ENV_SUPPORT should depend on
> > > > CONFIG_ENV_SUPPORT
> > > > in
> > > > common/spl/Kconfig.
> > >
> > > Not strictly, but checking for CONFIG_IS_ENABLED(ENV_SUPPORT)
> > > should
> > > do
> > > what's desired here?
> >
> > When SPL_NVME & SPL_BLK_FS is enabled, the spl_blk_fs driver calls
> > fs_set_blk_dev to the set the device & partition before accessing
> > it
> > and fs_set_blk_dev internally tries to get the device & partition
> > from
> > the bootdevice env variable if it was not passed by the caller.
> > However
> > for SPL build, when SPL_ENV_SUPPORT is not enabled nothing provides
> > env_get and hence the build fails.
>
> OK. So in the code we should be able to test with
> CONFIG_IS_ENABLED(ENV_SUPPORT) is that will be true for ((SPL and
> SPL environment support) or (Main U-Boot and environment support) or
> (TPL and TPL env)).
Thanks! I will send the patch with this change.
>
Re: [PATCH v4 0/4] SPL NVMe support
On Wed, 2023-07-12 at 13:12 -0400, Tom Rini wrote:
> On Wed, Jul 12, 2023 at 03:27:45PM +0200, Heinrich Schuchardt wrote:
> > On 12.07.23 15:06, mchit...@ventanamicro.com wrote:
> > > Hi Tom,
> > >
> > > On Tue, 2023-06-20 at 09:37 -0400, Tom Rini wrote:
> > > > On Sat, 03 Jun 2023 19:32:52 +0530, Mayuresh Chitale wrote:
> > > >
> > > > > This patchset adds support to load images of the SPL's next
> > > > > booting
> > > > > stage from a NVMe device.
> > > > >
> > > > > Changes in v4:
> > > > > - Drop patch 4
> > > > > - Modify patch 2 to use generic fs.h APIs
> > > > >
> > > > > [...]
> > > >
> > > > With one change, which is that the "disk/part.c" in 4/4 were
> > > > not
> > > > required for
> > > > any platform in tree and also broke testcases, and so was
> > > > dropped,
> > > > this has now
> > > > been applied to u-boot/next. If you can explain a bit more what
> > > > the
> > > > problem you
> > > > had was, we can look in to it. I suspect you need to test for
> > > > not
> > > > SPL_ENV_SUPPORT but ENV_SUPPORT itself.
> > > >
> > > Thanks.
> > > When SPL_NVME is enabled the build breaks with the following
> > > error:
> > > riscv64-unknown-linux-gnu-ld.bfd: disk/part.o: in function
> > > `blk_get_device_part_str':
> > > u-boot/disk/part.c:473: undefined reference to `env_get'
> > > make[2]: *** [u-boot/scripts/Makefile.spl:527: spl/u-boot-spl]
> > > Error 1
> > > make[1]: *** [/u-boot/Makefile:2053: spl/u-boot-spl] Error 2
> > >
> > > One possible fix is:
> > >
> > > if ((!IS_ENABLED(CONFIG_SPL) && IS_ENABLED(CONFIG_ENV_SUPPORT))
> > > ||
> > > (IS_ENABLED(CONFIG_SPL) &&
> > > IS_ENABLED(CONFIG_SPL_ENV_SUPPORT)))
> > > if (!dev_part_str || !strlen(dev_part_str)
> > > > > !strcmp(dev_part_str, "-"))
> > > dev_part_str = env_get("bootdevice");
> > >
> > >
> >
> > I think CONFIG_SPL_ENV_SUPPORT should depend on CONFIG_ENV_SUPPORT
> > in
> > common/spl/Kconfig.
>
> Not strictly, but checking for CONFIG_IS_ENABLED(ENV_SUPPORT) should
> do
> what's desired here?
When SPL_NVME & SPL_BLK_FS is enabled, the spl_blk_fs driver calls
fs_set_blk_dev to the set the device & partition before accessing it
and fs_set_blk_dev internally tries to get the device & partition from
the bootdevice env variable if it was not passed by the caller. However
for SPL build, when SPL_ENV_SUPPORT is not enabled nothing provides
env_get and hence the build fails.
Re: [PATCH v4 0/4] SPL NVMe support
Hi Tom,
On Tue, 2023-06-20 at 09:37 -0400, Tom Rini wrote:
> On Sat, 03 Jun 2023 19:32:52 +0530, Mayuresh Chitale wrote:
>
> > This patchset adds support to load images of the SPL's next booting
> > stage from a NVMe device.
> >
> > Changes in v4:
> > - Drop patch 4
> > - Modify patch 2 to use generic fs.h APIs
> >
> > [...]
>
> With one change, which is that the "disk/part.c" in 4/4 were not
> required for
> any platform in tree and also broke testcases, and so was dropped,
> this has now
> been applied to u-boot/next. If you can explain a bit more what the
> problem you
> had was, we can look in to it. I suspect you need to test for not
> SPL_ENV_SUPPORT but ENV_SUPPORT itself.
>
Thanks.
When SPL_NVME is enabled the build breaks with the following error:
riscv64-unknown-linux-gnu-ld.bfd: disk/part.o: in function
`blk_get_device_part_str':
u-boot/disk/part.c:473: undefined reference to `env_get'
make[2]: *** [u-boot/scripts/Makefile.spl:527: spl/u-boot-spl] Error 1
make[1]: *** [/u-boot/Makefile:2053: spl/u-boot-spl] Error 2
One possible fix is:
if ((!IS_ENABLED(CONFIG_SPL) && IS_ENABLED(CONFIG_ENV_SUPPORT)) ||
(IS_ENABLED(CONFIG_SPL) && IS_ENABLED(CONFIG_SPL_ENV_SUPPORT)))
if (!dev_part_str || !strlen(dev_part_str)
|| !strcmp(dev_part_str, "-"))
dev_part_str = env_get("bootdevice");
