Page MenuHomeFreeBSD
Differential D28180

bsdinstall: riscv platform tweaks
ClosedPublic
Actions

Authored by mhorne on Jan 15 2021, 5:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 24, 5:24 AM
Unknown Object (File)
Thu, Mar 20, 12:24 PM
Unknown Object (File)
Mon, Mar 17, 11:46 PM
Unknown Object (File)
Sun, Mar 16, 7:11 PM
Unknown Object (File)
Sat, Mar 8, 4:39 PM
Unknown Object (File)
Fri, Mar 7, 4:51 PM
Unknown Object (File)
Thu, Mar 6, 3:11 AM
Unknown Object (File)
Sun, Mar 2, 9:41 PM
View All Files
Subscribers
andrew

Details

Reviewers
imp
emaste
dteske
gjb
Commits
rG7b08a307e88b: bsdinstall: riscv-specific tweaks
Summary

Make the installer more useful by allowing it to create a bootable
installation. RISC-V boots with UEFI only.

  • Enable ZFS root install option
  • Enable creation of bootable ESP
Test Plan

Install and boot with a ZFS root filesystem.
Install and boot with a UFS root filesystem.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 36536
Build 33425: arc lint + arc unit

Event Timeline

mhorne requested review of this revision.Jan 15 2021, 5:50 PM
mhorne created this revision.
Harbormaster completed remote builds in B36233: Diff 82326.Jan 15 2021, 5:50 PM
mhorne added a comment.Jan 15 2021, 5:53 PM

Note that partedit_riscv.c is practically identical to partedit_arm64.c. We could merge the two, I'm just not sure what it ought to be called.

gjb accepted this revision.Jan 15 2021, 6:15 PM
This revision is now accepted and ready to land.Jan 15 2021, 6:15 PM
gjb added a comment.Jan 15 2021, 6:19 PM

Note, I only looked closely at the shell-based changes, considering partedit_riscv.c is identical to its amd64 counterpart.

emaste added a comment.Jan 16 2021, 12:58 AM
In D28180#629764, @mhorne wrote:

Note that partedit_riscv.c is practically identical to partedit_arm64.c. We could merge the two, I'm just not sure what it ought to be called.

partedit_gpt_uefi.c maybe?

mhorne updated this revision to Diff 82651.Jan 20 2021, 7:47 PM

Rename and merge the partedit_*.c files.

This revision now requires review to proceed.Jan 20 2021, 7:47 PM
Herald added a subscriber: andrew. · View Herald TranscriptJan 20 2021, 7:47 PM
Harbormaster completed remote builds in B36376: Diff 82651.Jan 20 2021, 7:48 PM
mhorne added a comment.Jan 20 2021, 7:51 PM
In D28180#629992, @emaste wrote:
In D28180#629764, @mhorne wrote:

Note that partedit_riscv.c is practically identical to partedit_arm64.c. We could merge the two, I'm just not sure what it ought to be called.

partedit_gpt_uefi.c maybe?

Thanks. I dropped the gpt only because there is nothing that precludes MBR from being allowed as well, other than we've chosen not to.

mhorne updated this revision to Diff 82980.Jan 27 2021, 4:12 PM

Fix arm64 $MACHINE check.

Harbormaster completed remote builds in B36536: Diff 82980.Jan 27 2021, 4:12 PM
mhorne added a comment.Jan 27 2021, 4:14 PM

Now tested on arm64 as well. If all seems good, I will commit and MFC to stable/13 before the branch.

gjb accepted this revision.Jan 27 2021, 4:35 PM
This revision is now accepted and ready to land.Jan 27 2021, 4:35 PM
Closed by commit rG7b08a307e88b: bsdinstall: riscv-specific tweaks (authored by mhorne). · Explain WhyJan 27 2021, 11:36 PM
This revision was automatically updated to reflect the committed changes.
mhorne added a commit: rG7b08a307e88b: bsdinstall: riscv-specific tweaks.

Revision Contents
Changeset List

PathSize
usr.sbin/
bsdinstall/
partedit/
2 lines
partedit_efi.c
partedit_arm64.c
7 lines
scripts/
2 lines
3 lines
4 lines

Diff 82980

View Options

usr.sbin/bsdinstall/partedit/Makefile

  • This file was added.
/*-
* Copyright (c) 2013 Ruslan Bukin <br@bsdpad.com>
* Copyright (c) 2013 Semihalf
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*
*/
#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/bus.h>
#include <sys/lock.h>
#include <sys/mutex.h>
#include <sys/smp.h>
#include <sys/cpuset.h>
#include <machine/smp.h>
#include <machine/fdt.h>
#include <machine/intr.h>
#include <dev/fdt/fdt_common.h>
#define AL_CPU_RESUME_WATERMARK_REG 0x00
#define AL_CPU_RESUME_FLAGS_REG 0x04
#define AL_CPU_RESUME_PCPU_RADDR_REG(cpu) (0x08 + 0x04 + 8*(cpu))
#define AL_CPU_RESUME_PCPU_FLAGS(cpu) (0x08 + 8*(cpu))
/* Per-CPU flags */
#define AL_CPU_RESUME_FLG_PERCPU_DONT_RESUME (1 << 2)
/* The expected magic number for validating the resume addresses */
#define AL_CPU_RESUME_MAGIC_NUM 0xf0e1d200
#define AL_CPU_RESUME_MAGIC_NUM_MASK 0xffffff00
/* The expected minimal version number for validating the capabilities */
#define AL_CPU_RESUME_MIN_VER 0x000000c3
#define AL_CPU_RESUME_MIN_VER_MASK 0x000000ff
/* Field controlling the boot-up of companion cores */
#define AL_NB_INIT_CONTROL (0x8)
#define AL_NB_CONFIG_STATUS_PWR_CTRL(cpu) (0x2020 + (cpu)*0x100)
extern void mpentry(void);
int alpine_serdes_resource_get(uint32_t group, bus_space_tag_t *tag,
bus_addr_t *baddr);
static int platform_mp_get_core_cnt(void);
static int alpine_get_cpu_resume_base(uint32_t *pbase, uint32_t *psize);
static int alpine_get_nb_base(uint32_t *pbase, uint32_t *psize);
static int alpine_get_serdes_base(uint32_t *pbase, uint32_t *psize);
int alpine_serdes_resource_get(uint32_t group, bus_space_tag_t *tag,
bus_addr_t *baddr);
static int
platform_mp_get_core_cnt(void)
{
static int ncores = 0;
phandle_t node;
phandle_t child;
int nchilds;
/* Calculate ncores value only once */
impUnsubmitted
Done
Inline Actions

Shouldn't this be lower case?

imp: Shouldn't this be lower case?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

While this is case insensitive comparison you are right that the practice is lower case. Will fix it.

jpa-semihalf.com: While this is case insensitive comparison you are right that the practice is lower case. Will…
if (ncores)
return (ncores);
__asm __volatile("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (ncores));
ncores = ((ncores >> 24) & 3) + 1;
andrewUnsubmitted
Done
Inline Actions

This needs a newline:

boolean_t
alpine_validate_cpu(...)

Should it be static?

andrew: This needs a newline: boolean_t alpine_validate_cpu(...) Should it be static?
/* Check if number of CPU cores should be limited in SMP mode */
andrewUnsubmitted
Done
Inline Actions

You should use ofw_cpu_early_foreach for this.

andrew: You should use ofw_cpu_early_foreach for this.
if ((node = OF_finddevice("/cpus")) == -1)
goto end;
nchilds = 0;
for (child = OF_child(node); child != 0; child = OF_peer(child)) {
if (fdt_is_compatible(child, "ARM,CORTEX-A15")) {
andrewUnsubmitted
Done
Inline Actions

This should use a function from cpu-v6.h

andrew: This should use a function from cpu-v6.h
nchilds++;
andrewUnsubmitted
Done
Inline Actions

You should add something like the following to armreg.h, then use it here.

#define L2CTLR_NPROC_SHIFT 24
#define L2CTLR_NPROC(r) ((((r) >> L2CTLR_NPROC_SHIFT) & 3) + 1)
andrew: You should add something like the following to armreg.h, then use it here. #define…
}
}
/* Limit CPUs if DTS has configured less than available */
if ((nchilds > 0) && (nchilds < ncores)) {
printf("SMP: limiting number of active CPUs to %d out of %d\n",
nchilds, ncores);
ncores = nchilds;
}
ianUnsubmitted
Done
Inline Actions

the number of cores should also be limited by the hw.ncpu tunable so that you can control it from loader(8).

ian: the number of cores should also be limited by the hw.ncpu tunable so that you can control it…
jpa-semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

We are not supporting loader(8) so that would be dead code.

jpa-semihalf.com: We are not supporting loader(8) so that would be dead code.
ianUnsubmitted
Done
Inline Actions

I don't view "we are not supporting loader(8)" as a valid position without some sort of really strong justification. It's not 2011 anymore where we commit code for a new platform as soon as it can boot a bare kernel.bin that supports a serial console.

loader(8) is part of freebsd and is the standard way to load a kernel. why do you claim to not support it?

ian: I don't view "we are not supporting loader(8)" as a valid position without some sort of really…
impUnsubmitted
Done
Inline Actions

These boards are embedded network processors that live on a PCIe card. They have very limited space for extra stuff not related to the task at hand... While I generally agree we need loader(8) support, this board may be a reasonable exception to that rule.

imp: These boards are embedded network processors that live on a PCIe card. They have very limited…
ianUnsubmitted
Done
Inline Actions

Ah, I didn't realize these were not general-purpose systems oriented towards the end user. For that case, compiling in the dtb and skipping loader(8) seems reasonable.

ian: Ah, I didn't realize these were not general-purpose systems oriented towards the end user. For…
end:
return (ncores);
}
void
platform_mp_init_secondary(void)
{
arm_init_secondary_ic();
}
void
platform_mp_setmaxid(void)
{
int core_cnt;
core_cnt = platform_mp_get_core_cnt();
mp_maxid = core_cnt - 1;
}
int
platform_mp_probe(void)
{
mp_ncpus = platform_mp_get_core_cnt();
return (1);
}
static int
alpine_get_cpu_resume_base(uint32_t *pbase, uint32_t *psize)
{
phandle_t node;
u_long base=0;
u_long size=0;
if (!pbase || !psize)
impUnsubmitted
Not Done
Inline Actions

Shouldn't this be a kassert?

imp: Shouldn't this be a kassert?
jpa-semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

In general that would be the way to go, but other errors are handled with same panic message anyway, so I would rather not duplicate code unless necessary.

jpa-semihalf.com: In general that would be the way to go, but other errors are handled with same panic message…
return (EINVAL);
if ((node = OF_finddevice("/")) == -1)
return (EFAULT);
andrewUnsubmitted
Done
Inline Actions

Do these need to be fdt_ functions, or could you call an ofw_ function?

andrew: Do these need to be fdt_ functions, or could you call an ofw_ function?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I don't have device yet, so the alternative option is to use pure OF_ set of functions to find one, for example wrapped into ofw_search_tree_compatible_device("/simple-bus/al-cpu-resume");

jpa-semihalf.com: I don't have device yet, so the alternative option is to use pure OF_ set of functions to find…
if ((node = fdt_find_compatible(node, "simple-bus", 1)) == 0)
return (EFAULT);
if ((node =
fdt_find_compatible(node, "annapurna-labs,al-cpu-resume", 1)) == 0)
return (EFAULT);
if (fdt_regsize(node, &base, &size))
return (EFAULT);
*pbase = base;
*psize = size;
return (0);
}
static int
alpine_get_nb_base(uint32_t *pbase, uint32_t *psize)
{
phandle_t node;
u_long base=0;
u_long size=0;
if (!pbase || !psize)
impUnsubmitted
Done
Inline Actions

kassert?

imp: kassert?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

Same as above.

jpa-semihalf.com: Same as above.
return (EINVAL);
if ((node = OF_finddevice("/")) == -1)
andrewUnsubmitted
Done
Inline Actions

Missing space, should be base = 0;, and the same for size.

andrew: Missing space, should be `base = 0;`, and the same for size.
return (EFAULT);
andrewUnsubmitted
Done
Inline Actions
if (pbase == NULL || psize == NULL)
andrew: if (pbase == NULL || psize == NULL)
if ((node = fdt_find_compatible(node, "simple-bus", 1)) == 0)
return (EFAULT);
if ((node =
fdt_find_compatible(node, "annapurna-labs,al-nb-service", 1)) == 0)
return (EFAULT);
if (fdt_regsize(node, &base, &size))
return (EFAULT);
*pbase = base;
*psize = size;
return (0);
}
void
platform_mp_start_ap(void)
{
uint32_t physaddr;
vm_offset_t vaddr;
uint32_t val;
uint32_t start_mask;
uint32_t cpu_resume_base;
uint32_t nb_base;
andrewUnsubmitted
Done
Inline Actions

uint32_t is the wrong type for a number of these.

andrew: uint32_t is the wrong type for a number of these.
uint32_t cpu_resume_size;
uint32_t nb_size;
bus_addr_t cpu_resume_baddr;
bus_addr_t nb_baddr;
int a;
if (alpine_get_cpu_resume_base(&cpu_resume_base, &cpu_resume_size))
panic("Couldn't resolve cpu_resume_base address\n");
if (alpine_get_nb_base(&nb_base, &nb_size))
panic("Couldn't resolve_nb_base address\n");
/* Proceed with start addresses for additional CPUs */
ianUnsubmitted
Done
Inline Actions

Unless this platform has mapping control registers that allow repositioning the window for accessing hardware regs, none of these bus_space_map() calls should be using fdt_immr_pa, they should just be using the physical addresses from the fdt data.

As far as I know, only Marvel chips have such a feature (and maybe not even all of them). fdt_immr_pa should never appear outside of arm/mv (and some polution where it leaks out into dev/fdt that we really need to fix).

ian: Unless this platform has mapping control registers that allow repositioning the window for…
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I removed all references to fdt_immr_pa and used the preferred way of setting addresses (arm_devmap_add_entry())

jpa-semihalf.com: I removed all references to fdt_immr_pa and used the preferred way of setting addresses…
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + cpu_resume_base,
cpu_resume_size, 0, &cpu_resume_baddr))
panic("Couldn't map CPU-resume area");
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + nb_base,
nb_size, 0, &nb_baddr))
panic("Couldn't map NB-service area");
/* Proceed with start addresses for additional CPUs */
val = bus_space_read_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_WATERMARK_REG);
if (((val & AL_CPU_RESUME_MAGIC_NUM_MASK) != AL_CPU_RESUME_MAGIC_NUM) ||
((val & AL_CPU_RESUME_MIN_VER_MASK) < AL_CPU_RESUME_MIN_VER)) {
panic("CPU-resume device is not compatible");
}
vaddr = (vm_offset_t)mpentry;
physaddr = pmap_kextract(vaddr);
for (a = 1; a < platform_mp_get_core_cnt(); a++) {
/* Power up the core */
bus_space_write_4(fdtbus_bs_tag, nb_baddr,
AL_NB_CONFIG_STATUS_PWR_CTRL(a), 0);
mb();
/* Enable resume */
val = bus_space_read_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_FLAGS(a));
val &= ~AL_CPU_RESUME_FLG_PERCPU_DONT_RESUME;
bus_space_write_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_FLAGS(a), val);
mb();
/* Set resume physical address */
bus_space_write_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_RADDR_REG(a), physaddr);
mb();
}
/* Release cores from reset */
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + nb_base,
nb_size, 0, &nb_baddr))
panic("Couldn't map NB-service area");
start_mask = (1 << platform_mp_get_core_cnt()) - 1;
/* Release cores from reset */
val = bus_space_read_4(fdtbus_bs_tag, nb_baddr, AL_NB_INIT_CONTROL);
val |= start_mask;
andrewUnsubmitted
Done
Inline Actions

Is a dmb enough here?

andrew: Is a dmb enough here?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

armv7_sev() issues dsb internally, so I think it is even not needed here, but for the sake of future changes to armv7_sev() I will replace the mb() accordingly.

jpa-semihalf.com: armv7_sev() issues dsb internally, so I think it is even not needed here, but for the sake of…
bus_space_write_4(fdtbus_bs_tag, nb_baddr, AL_NB_INIT_CONTROL, val);
ianUnsubmitted
Done
Inline Actions

Why do we even do a SEV here? I have the feeling this has been pasted into every soc mp file and doesn't do anything useful on any of them.

ian: Why do we even do a SEV here? I have the feeling this has been pasted into every soc mp file…
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I removed it and so far so good. On the other hand I kept the dsb().

jpa-semihalf.com: I removed it and so far so good. On the other hand I kept the dsb().
mb();
armv7_sev();
bus_space_unmap(fdtbus_bs_tag, nb_baddr, nb_size);
bus_space_unmap(fdtbus_bs_tag, cpu_resume_baddr, cpu_resume_size);
}
void
platform_ipi_send(cpuset_t cpus, u_int ipi)
{
pic_ipi_send(cpus, ipi);
}
andrewUnsubmitted
Done
Inline Actions

This has the same style issues as alpine_get_nb_base.

andrew: This has the same style issues as alpine_get_nb_base.
View Options

usr.sbin/bsdinstall/partedit/partedit_arm64.c

  • This file was added.
/*-
* Copyright (c) 2013 Ruslan Bukin <br@bsdpad.com>
* Copyright (c) 2013 Semihalf
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*
*/
#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/bus.h>
#include <sys/lock.h>
#include <sys/mutex.h>
#include <sys/smp.h>
#include <sys/cpuset.h>
#include <machine/smp.h>
#include <machine/fdt.h>
#include <machine/intr.h>
#include <dev/fdt/fdt_common.h>
#define AL_CPU_RESUME_WATERMARK_REG 0x00
#define AL_CPU_RESUME_FLAGS_REG 0x04
#define AL_CPU_RESUME_PCPU_RADDR_REG(cpu) (0x08 + 0x04 + 8*(cpu))
#define AL_CPU_RESUME_PCPU_FLAGS(cpu) (0x08 + 8*(cpu))
/* Per-CPU flags */
#define AL_CPU_RESUME_FLG_PERCPU_DONT_RESUME (1 << 2)
/* The expected magic number for validating the resume addresses */
#define AL_CPU_RESUME_MAGIC_NUM 0xf0e1d200
#define AL_CPU_RESUME_MAGIC_NUM_MASK 0xffffff00
/* The expected minimal version number for validating the capabilities */
#define AL_CPU_RESUME_MIN_VER 0x000000c3
#define AL_CPU_RESUME_MIN_VER_MASK 0x000000ff
/* Field controlling the boot-up of companion cores */
#define AL_NB_INIT_CONTROL (0x8)
#define AL_NB_CONFIG_STATUS_PWR_CTRL(cpu) (0x2020 + (cpu)*0x100)
extern void mpentry(void);
int alpine_serdes_resource_get(uint32_t group, bus_space_tag_t *tag,
bus_addr_t *baddr);
static int platform_mp_get_core_cnt(void);
static int alpine_get_cpu_resume_base(uint32_t *pbase, uint32_t *psize);
static int alpine_get_nb_base(uint32_t *pbase, uint32_t *psize);
static int alpine_get_serdes_base(uint32_t *pbase, uint32_t *psize);
int alpine_serdes_resource_get(uint32_t group, bus_space_tag_t *tag,
bus_addr_t *baddr);
static int
platform_mp_get_core_cnt(void)
{
static int ncores = 0;
phandle_t node;
phandle_t child;
int nchilds;
/* Calculate ncores value only once */
impUnsubmitted
Done
Inline Actions

Shouldn't this be lower case?

imp: Shouldn't this be lower case?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

While this is case insensitive comparison you are right that the practice is lower case. Will fix it.

jpa-semihalf.com: While this is case insensitive comparison you are right that the practice is lower case. Will…
if (ncores)
return (ncores);
__asm __volatile("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (ncores));
ncores = ((ncores >> 24) & 3) + 1;
andrewUnsubmitted
Done
Inline Actions

This needs a newline:

boolean_t
alpine_validate_cpu(...)

Should it be static?

andrew: This needs a newline: boolean_t alpine_validate_cpu(...) Should it be static?
/* Check if number of CPU cores should be limited in SMP mode */
andrewUnsubmitted
Done
Inline Actions

You should use ofw_cpu_early_foreach for this.

andrew: You should use ofw_cpu_early_foreach for this.
if ((node = OF_finddevice("/cpus")) == -1)
goto end;
nchilds = 0;
for (child = OF_child(node); child != 0; child = OF_peer(child)) {
if (fdt_is_compatible(child, "ARM,CORTEX-A15")) {
andrewUnsubmitted
Done
Inline Actions

This should use a function from cpu-v6.h

andrew: This should use a function from cpu-v6.h
nchilds++;
andrewUnsubmitted
Done
Inline Actions

You should add something like the following to armreg.h, then use it here.

#define L2CTLR_NPROC_SHIFT 24
#define L2CTLR_NPROC(r) ((((r) >> L2CTLR_NPROC_SHIFT) & 3) + 1)
andrew: You should add something like the following to armreg.h, then use it here. #define…
}
}
/* Limit CPUs if DTS has configured less than available */
if ((nchilds > 0) && (nchilds < ncores)) {
printf("SMP: limiting number of active CPUs to %d out of %d\n",
nchilds, ncores);
ncores = nchilds;
}
ianUnsubmitted
Done
Inline Actions

the number of cores should also be limited by the hw.ncpu tunable so that you can control it from loader(8).

ian: the number of cores should also be limited by the hw.ncpu tunable so that you can control it…
jpa-semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

We are not supporting loader(8) so that would be dead code.

jpa-semihalf.com: We are not supporting loader(8) so that would be dead code.
ianUnsubmitted
Done
Inline Actions

I don't view "we are not supporting loader(8)" as a valid position without some sort of really strong justification. It's not 2011 anymore where we commit code for a new platform as soon as it can boot a bare kernel.bin that supports a serial console.

loader(8) is part of freebsd and is the standard way to load a kernel. why do you claim to not support it?

ian: I don't view "we are not supporting loader(8)" as a valid position without some sort of really…
impUnsubmitted
Done
Inline Actions

These boards are embedded network processors that live on a PCIe card. They have very limited space for extra stuff not related to the task at hand... While I generally agree we need loader(8) support, this board may be a reasonable exception to that rule.

imp: These boards are embedded network processors that live on a PCIe card. They have very limited…
ianUnsubmitted
Done
Inline Actions

Ah, I didn't realize these were not general-purpose systems oriented towards the end user. For that case, compiling in the dtb and skipping loader(8) seems reasonable.

ian: Ah, I didn't realize these were not general-purpose systems oriented towards the end user. For…
end:
return (ncores);
}
void
platform_mp_init_secondary(void)
{
arm_init_secondary_ic();
}
void
platform_mp_setmaxid(void)
{
int core_cnt;
core_cnt = platform_mp_get_core_cnt();
mp_maxid = core_cnt - 1;
}
int
platform_mp_probe(void)
{
mp_ncpus = platform_mp_get_core_cnt();
return (1);
}
static int
alpine_get_cpu_resume_base(uint32_t *pbase, uint32_t *psize)
{
phandle_t node;
u_long base=0;
u_long size=0;
if (!pbase || !psize)
impUnsubmitted
Not Done
Inline Actions

Shouldn't this be a kassert?

imp: Shouldn't this be a kassert?
jpa-semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

In general that would be the way to go, but other errors are handled with same panic message anyway, so I would rather not duplicate code unless necessary.

jpa-semihalf.com: In general that would be the way to go, but other errors are handled with same panic message…
return (EINVAL);
if ((node = OF_finddevice("/")) == -1)
return (EFAULT);
andrewUnsubmitted
Done
Inline Actions

Do these need to be fdt_ functions, or could you call an ofw_ function?

andrew: Do these need to be fdt_ functions, or could you call an ofw_ function?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I don't have device yet, so the alternative option is to use pure OF_ set of functions to find one, for example wrapped into ofw_search_tree_compatible_device("/simple-bus/al-cpu-resume");

jpa-semihalf.com: I don't have device yet, so the alternative option is to use pure OF_ set of functions to find…
if ((node = fdt_find_compatible(node, "simple-bus", 1)) == 0)
return (EFAULT);
if ((node =
fdt_find_compatible(node, "annapurna-labs,al-cpu-resume", 1)) == 0)
return (EFAULT);
if (fdt_regsize(node, &base, &size))
return (EFAULT);
*pbase = base;
*psize = size;
return (0);
}
static int
alpine_get_nb_base(uint32_t *pbase, uint32_t *psize)
{
phandle_t node;
u_long base=0;
u_long size=0;
if (!pbase || !psize)
impUnsubmitted
Done
Inline Actions

kassert?

imp: kassert?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

Same as above.

jpa-semihalf.com: Same as above.
return (EINVAL);
if ((node = OF_finddevice("/")) == -1)
andrewUnsubmitted
Done
Inline Actions

Missing space, should be base = 0;, and the same for size.

andrew: Missing space, should be `base = 0;`, and the same for size.
return (EFAULT);
andrewUnsubmitted
Done
Inline Actions
if (pbase == NULL || psize == NULL)
andrew: if (pbase == NULL || psize == NULL)
if ((node = fdt_find_compatible(node, "simple-bus", 1)) == 0)
return (EFAULT);
if ((node =
fdt_find_compatible(node, "annapurna-labs,al-nb-service", 1)) == 0)
return (EFAULT);
if (fdt_regsize(node, &base, &size))
return (EFAULT);
*pbase = base;
*psize = size;
return (0);
}
void
platform_mp_start_ap(void)
{
uint32_t physaddr;
vm_offset_t vaddr;
uint32_t val;
uint32_t start_mask;
uint32_t cpu_resume_base;
uint32_t nb_base;
andrewUnsubmitted
Done
Inline Actions

uint32_t is the wrong type for a number of these.

andrew: uint32_t is the wrong type for a number of these.
uint32_t cpu_resume_size;
uint32_t nb_size;
bus_addr_t cpu_resume_baddr;
bus_addr_t nb_baddr;
int a;
if (alpine_get_cpu_resume_base(&cpu_resume_base, &cpu_resume_size))
panic("Couldn't resolve cpu_resume_base address\n");
if (alpine_get_nb_base(&nb_base, &nb_size))
panic("Couldn't resolve_nb_base address\n");
/* Proceed with start addresses for additional CPUs */
ianUnsubmitted
Done
Inline Actions

Unless this platform has mapping control registers that allow repositioning the window for accessing hardware regs, none of these bus_space_map() calls should be using fdt_immr_pa, they should just be using the physical addresses from the fdt data.

As far as I know, only Marvel chips have such a feature (and maybe not even all of them). fdt_immr_pa should never appear outside of arm/mv (and some polution where it leaks out into dev/fdt that we really need to fix).

ian: Unless this platform has mapping control registers that allow repositioning the window for…
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I removed all references to fdt_immr_pa and used the preferred way of setting addresses (arm_devmap_add_entry())

jpa-semihalf.com: I removed all references to fdt_immr_pa and used the preferred way of setting addresses…
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + cpu_resume_base,
cpu_resume_size, 0, &cpu_resume_baddr))
panic("Couldn't map CPU-resume area");
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + nb_base,
nb_size, 0, &nb_baddr))
panic("Couldn't map NB-service area");
/* Proceed with start addresses for additional CPUs */
val = bus_space_read_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_WATERMARK_REG);
if (((val & AL_CPU_RESUME_MAGIC_NUM_MASK) != AL_CPU_RESUME_MAGIC_NUM) ||
((val & AL_CPU_RESUME_MIN_VER_MASK) < AL_CPU_RESUME_MIN_VER)) {
panic("CPU-resume device is not compatible");
}
vaddr = (vm_offset_t)mpentry;
physaddr = pmap_kextract(vaddr);
for (a = 1; a < platform_mp_get_core_cnt(); a++) {
/* Power up the core */
bus_space_write_4(fdtbus_bs_tag, nb_baddr,
AL_NB_CONFIG_STATUS_PWR_CTRL(a), 0);
mb();
/* Enable resume */
val = bus_space_read_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_FLAGS(a));
val &= ~AL_CPU_RESUME_FLG_PERCPU_DONT_RESUME;
bus_space_write_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_FLAGS(a), val);
mb();
/* Set resume physical address */
bus_space_write_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_RADDR_REG(a), physaddr);
mb();
}
/* Release cores from reset */
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + nb_base,
nb_size, 0, &nb_baddr))
panic("Couldn't map NB-service area");
start_mask = (1 << platform_mp_get_core_cnt()) - 1;
/* Release cores from reset */
val = bus_space_read_4(fdtbus_bs_tag, nb_baddr, AL_NB_INIT_CONTROL);
val |= start_mask;
andrewUnsubmitted
Done
Inline Actions

Is a dmb enough here?

andrew: Is a dmb enough here?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

armv7_sev() issues dsb internally, so I think it is even not needed here, but for the sake of future changes to armv7_sev() I will replace the mb() accordingly.

jpa-semihalf.com: armv7_sev() issues dsb internally, so I think it is even not needed here, but for the sake of…
bus_space_write_4(fdtbus_bs_tag, nb_baddr, AL_NB_INIT_CONTROL, val);
ianUnsubmitted
Done
Inline Actions

Why do we even do a SEV here? I have the feeling this has been pasted into every soc mp file and doesn't do anything useful on any of them.

ian: Why do we even do a SEV here? I have the feeling this has been pasted into every soc mp file…
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I removed it and so far so good. On the other hand I kept the dsb().

jpa-semihalf.com: I removed it and so far so good. On the other hand I kept the dsb().
mb();
armv7_sev();
bus_space_unmap(fdtbus_bs_tag, nb_baddr, nb_size);
bus_space_unmap(fdtbus_bs_tag, cpu_resume_baddr, cpu_resume_size);
}
void
platform_ipi_send(cpuset_t cpus, u_int ipi)
{
pic_ipi_send(cpus, ipi);
}
andrewUnsubmitted
Done
Inline Actions

This has the same style issues as alpine_get_nb_base.

andrew: This has the same style issues as alpine_get_nb_base.
View Options

usr.sbin/bsdinstall/partedit/partedit_efi.c

  • This file was added.
/*-
* Copyright (c) 2013 Ruslan Bukin <br@bsdpad.com>
* Copyright (c) 2013 Semihalf
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*
*/
#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/bus.h>
#include <sys/lock.h>
#include <sys/mutex.h>
#include <sys/smp.h>
#include <sys/cpuset.h>
#include <machine/smp.h>
#include <machine/fdt.h>
#include <machine/intr.h>
#include <dev/fdt/fdt_common.h>
#define AL_CPU_RESUME_WATERMARK_REG 0x00
#define AL_CPU_RESUME_FLAGS_REG 0x04
#define AL_CPU_RESUME_PCPU_RADDR_REG(cpu) (0x08 + 0x04 + 8*(cpu))
#define AL_CPU_RESUME_PCPU_FLAGS(cpu) (0x08 + 8*(cpu))
/* Per-CPU flags */
#define AL_CPU_RESUME_FLG_PERCPU_DONT_RESUME (1 << 2)
/* The expected magic number for validating the resume addresses */
#define AL_CPU_RESUME_MAGIC_NUM 0xf0e1d200
#define AL_CPU_RESUME_MAGIC_NUM_MASK 0xffffff00
/* The expected minimal version number for validating the capabilities */
#define AL_CPU_RESUME_MIN_VER 0x000000c3
#define AL_CPU_RESUME_MIN_VER_MASK 0x000000ff
/* Field controlling the boot-up of companion cores */
#define AL_NB_INIT_CONTROL (0x8)
#define AL_NB_CONFIG_STATUS_PWR_CTRL(cpu) (0x2020 + (cpu)*0x100)
extern void mpentry(void);
int alpine_serdes_resource_get(uint32_t group, bus_space_tag_t *tag,
bus_addr_t *baddr);
static int platform_mp_get_core_cnt(void);
static int alpine_get_cpu_resume_base(uint32_t *pbase, uint32_t *psize);
static int alpine_get_nb_base(uint32_t *pbase, uint32_t *psize);
static int alpine_get_serdes_base(uint32_t *pbase, uint32_t *psize);
int alpine_serdes_resource_get(uint32_t group, bus_space_tag_t *tag,
bus_addr_t *baddr);
static int
platform_mp_get_core_cnt(void)
{
static int ncores = 0;
phandle_t node;
phandle_t child;
int nchilds;
/* Calculate ncores value only once */
impUnsubmitted
Done
Inline Actions

Shouldn't this be lower case?

imp: Shouldn't this be lower case?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

While this is case insensitive comparison you are right that the practice is lower case. Will fix it.

jpa-semihalf.com: While this is case insensitive comparison you are right that the practice is lower case. Will…
if (ncores)
return (ncores);
__asm __volatile("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (ncores));
ncores = ((ncores >> 24) & 3) + 1;
andrewUnsubmitted
Done
Inline Actions

This needs a newline:

boolean_t
alpine_validate_cpu(...)

Should it be static?

andrew: This needs a newline: boolean_t alpine_validate_cpu(...) Should it be static?
/* Check if number of CPU cores should be limited in SMP mode */
andrewUnsubmitted
Done
Inline Actions

You should use ofw_cpu_early_foreach for this.

andrew: You should use ofw_cpu_early_foreach for this.
if ((node = OF_finddevice("/cpus")) == -1)
goto end;
nchilds = 0;
for (child = OF_child(node); child != 0; child = OF_peer(child)) {
if (fdt_is_compatible(child, "ARM,CORTEX-A15")) {
andrewUnsubmitted
Done
Inline Actions

This should use a function from cpu-v6.h

andrew: This should use a function from cpu-v6.h
nchilds++;
andrewUnsubmitted
Done
Inline Actions

You should add something like the following to armreg.h, then use it here.

#define L2CTLR_NPROC_SHIFT 24
#define L2CTLR_NPROC(r) ((((r) >> L2CTLR_NPROC_SHIFT) & 3) + 1)
andrew: You should add something like the following to armreg.h, then use it here. #define…
}
}
/* Limit CPUs if DTS has configured less than available */
if ((nchilds > 0) && (nchilds < ncores)) {
printf("SMP: limiting number of active CPUs to %d out of %d\n",
nchilds, ncores);
ncores = nchilds;
}
ianUnsubmitted
Done
Inline Actions

the number of cores should also be limited by the hw.ncpu tunable so that you can control it from loader(8).

ian: the number of cores should also be limited by the hw.ncpu tunable so that you can control it…
jpa-semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

We are not supporting loader(8) so that would be dead code.

jpa-semihalf.com: We are not supporting loader(8) so that would be dead code.
ianUnsubmitted
Done
Inline Actions

I don't view "we are not supporting loader(8)" as a valid position without some sort of really strong justification. It's not 2011 anymore where we commit code for a new platform as soon as it can boot a bare kernel.bin that supports a serial console.

loader(8) is part of freebsd and is the standard way to load a kernel. why do you claim to not support it?

ian: I don't view "we are not supporting loader(8)" as a valid position without some sort of really…
impUnsubmitted
Done
Inline Actions

These boards are embedded network processors that live on a PCIe card. They have very limited space for extra stuff not related to the task at hand... While I generally agree we need loader(8) support, this board may be a reasonable exception to that rule.

imp: These boards are embedded network processors that live on a PCIe card. They have very limited…
ianUnsubmitted
Done
Inline Actions

Ah, I didn't realize these were not general-purpose systems oriented towards the end user. For that case, compiling in the dtb and skipping loader(8) seems reasonable.

ian: Ah, I didn't realize these were not general-purpose systems oriented towards the end user. For…
end:
return (ncores);
}
void
platform_mp_init_secondary(void)
{
arm_init_secondary_ic();
}
void
platform_mp_setmaxid(void)
{
int core_cnt;
core_cnt = platform_mp_get_core_cnt();
mp_maxid = core_cnt - 1;
}
int
platform_mp_probe(void)
{
mp_ncpus = platform_mp_get_core_cnt();
return (1);
}
static int
alpine_get_cpu_resume_base(uint32_t *pbase, uint32_t *psize)
{
phandle_t node;
u_long base=0;
u_long size=0;
if (!pbase || !psize)
impUnsubmitted
Not Done
Inline Actions

Shouldn't this be a kassert?

imp: Shouldn't this be a kassert?
jpa-semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

In general that would be the way to go, but other errors are handled with same panic message anyway, so I would rather not duplicate code unless necessary.

jpa-semihalf.com: In general that would be the way to go, but other errors are handled with same panic message…
return (EINVAL);
if ((node = OF_finddevice("/")) == -1)
return (EFAULT);
andrewUnsubmitted
Done
Inline Actions

Do these need to be fdt_ functions, or could you call an ofw_ function?

andrew: Do these need to be fdt_ functions, or could you call an ofw_ function?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I don't have device yet, so the alternative option is to use pure OF_ set of functions to find one, for example wrapped into ofw_search_tree_compatible_device("/simple-bus/al-cpu-resume");

jpa-semihalf.com: I don't have device yet, so the alternative option is to use pure OF_ set of functions to find…
if ((node = fdt_find_compatible(node, "simple-bus", 1)) == 0)
return (EFAULT);
if ((node =
fdt_find_compatible(node, "annapurna-labs,al-cpu-resume", 1)) == 0)
return (EFAULT);
if (fdt_regsize(node, &base, &size))
return (EFAULT);
*pbase = base;
*psize = size;
return (0);
}
static int
alpine_get_nb_base(uint32_t *pbase, uint32_t *psize)
{
phandle_t node;
u_long base=0;
u_long size=0;
if (!pbase || !psize)
impUnsubmitted
Done
Inline Actions

kassert?

imp: kassert?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

Same as above.

jpa-semihalf.com: Same as above.
return (EINVAL);
if ((node = OF_finddevice("/")) == -1)
andrewUnsubmitted
Done
Inline Actions

Missing space, should be base = 0;, and the same for size.

andrew: Missing space, should be `base = 0;`, and the same for size.
return (EFAULT);
andrewUnsubmitted
Done
Inline Actions
if (pbase == NULL || psize == NULL)
andrew: if (pbase == NULL || psize == NULL)
if ((node = fdt_find_compatible(node, "simple-bus", 1)) == 0)
return (EFAULT);
if ((node =
fdt_find_compatible(node, "annapurna-labs,al-nb-service", 1)) == 0)
return (EFAULT);
if (fdt_regsize(node, &base, &size))
return (EFAULT);
*pbase = base;
*psize = size;
return (0);
}
void
platform_mp_start_ap(void)
{
uint32_t physaddr;
vm_offset_t vaddr;
uint32_t val;
uint32_t start_mask;
uint32_t cpu_resume_base;
uint32_t nb_base;
andrewUnsubmitted
Done
Inline Actions

uint32_t is the wrong type for a number of these.

andrew: uint32_t is the wrong type for a number of these.
uint32_t cpu_resume_size;
uint32_t nb_size;
bus_addr_t cpu_resume_baddr;
bus_addr_t nb_baddr;
int a;
if (alpine_get_cpu_resume_base(&cpu_resume_base, &cpu_resume_size))
panic("Couldn't resolve cpu_resume_base address\n");
if (alpine_get_nb_base(&nb_base, &nb_size))
panic("Couldn't resolve_nb_base address\n");
/* Proceed with start addresses for additional CPUs */
ianUnsubmitted
Done
Inline Actions

Unless this platform has mapping control registers that allow repositioning the window for accessing hardware regs, none of these bus_space_map() calls should be using fdt_immr_pa, they should just be using the physical addresses from the fdt data.

As far as I know, only Marvel chips have such a feature (and maybe not even all of them). fdt_immr_pa should never appear outside of arm/mv (and some polution where it leaks out into dev/fdt that we really need to fix).

ian: Unless this platform has mapping control registers that allow repositioning the window for…
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I removed all references to fdt_immr_pa and used the preferred way of setting addresses (arm_devmap_add_entry())

jpa-semihalf.com: I removed all references to fdt_immr_pa and used the preferred way of setting addresses…
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + cpu_resume_base,
cpu_resume_size, 0, &cpu_resume_baddr))
panic("Couldn't map CPU-resume area");
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + nb_base,
nb_size, 0, &nb_baddr))
panic("Couldn't map NB-service area");
/* Proceed with start addresses for additional CPUs */
val = bus_space_read_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_WATERMARK_REG);
if (((val & AL_CPU_RESUME_MAGIC_NUM_MASK) != AL_CPU_RESUME_MAGIC_NUM) ||
((val & AL_CPU_RESUME_MIN_VER_MASK) < AL_CPU_RESUME_MIN_VER)) {
panic("CPU-resume device is not compatible");
}
vaddr = (vm_offset_t)mpentry;
physaddr = pmap_kextract(vaddr);
for (a = 1; a < platform_mp_get_core_cnt(); a++) {
/* Power up the core */
bus_space_write_4(fdtbus_bs_tag, nb_baddr,
AL_NB_CONFIG_STATUS_PWR_CTRL(a), 0);
mb();
/* Enable resume */
val = bus_space_read_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_FLAGS(a));
val &= ~AL_CPU_RESUME_FLG_PERCPU_DONT_RESUME;
bus_space_write_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_FLAGS(a), val);
mb();
/* Set resume physical address */
bus_space_write_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_RADDR_REG(a), physaddr);
mb();
}
/* Release cores from reset */
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + nb_base,
nb_size, 0, &nb_baddr))
panic("Couldn't map NB-service area");
start_mask = (1 << platform_mp_get_core_cnt()) - 1;
/* Release cores from reset */
val = bus_space_read_4(fdtbus_bs_tag, nb_baddr, AL_NB_INIT_CONTROL);
val |= start_mask;
andrewUnsubmitted
Done
Inline Actions

Is a dmb enough here?

andrew: Is a dmb enough here?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

armv7_sev() issues dsb internally, so I think it is even not needed here, but for the sake of future changes to armv7_sev() I will replace the mb() accordingly.

jpa-semihalf.com: armv7_sev() issues dsb internally, so I think it is even not needed here, but for the sake of…
bus_space_write_4(fdtbus_bs_tag, nb_baddr, AL_NB_INIT_CONTROL, val);
ianUnsubmitted
Done
Inline Actions

Why do we even do a SEV here? I have the feeling this has been pasted into every soc mp file and doesn't do anything useful on any of them.

ian: Why do we even do a SEV here? I have the feeling this has been pasted into every soc mp file…
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I removed it and so far so good. On the other hand I kept the dsb().

jpa-semihalf.com: I removed it and so far so good. On the other hand I kept the dsb().
mb();
armv7_sev();
bus_space_unmap(fdtbus_bs_tag, nb_baddr, nb_size);
bus_space_unmap(fdtbus_bs_tag, cpu_resume_baddr, cpu_resume_size);
}
void
platform_ipi_send(cpuset_t cpus, u_int ipi)
{
pic_ipi_send(cpus, ipi);
}
andrewUnsubmitted
Done
Inline Actions

This has the same style issues as alpine_get_nb_base.

andrew: This has the same style issues as alpine_get_nb_base.
View Options

usr.sbin/bsdinstall/scripts/auto

  • This file was added.
/*-
* Copyright (c) 2013 Ruslan Bukin <br@bsdpad.com>
* Copyright (c) 2013 Semihalf
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*
*/
#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/bus.h>
#include <sys/lock.h>
#include <sys/mutex.h>
#include <sys/smp.h>
#include <sys/cpuset.h>
#include <machine/smp.h>
#include <machine/fdt.h>
#include <machine/intr.h>
#include <dev/fdt/fdt_common.h>
#define AL_CPU_RESUME_WATERMARK_REG 0x00
#define AL_CPU_RESUME_FLAGS_REG 0x04
#define AL_CPU_RESUME_PCPU_RADDR_REG(cpu) (0x08 + 0x04 + 8*(cpu))
#define AL_CPU_RESUME_PCPU_FLAGS(cpu) (0x08 + 8*(cpu))
/* Per-CPU flags */
#define AL_CPU_RESUME_FLG_PERCPU_DONT_RESUME (1 << 2)
/* The expected magic number for validating the resume addresses */
#define AL_CPU_RESUME_MAGIC_NUM 0xf0e1d200
#define AL_CPU_RESUME_MAGIC_NUM_MASK 0xffffff00
/* The expected minimal version number for validating the capabilities */
#define AL_CPU_RESUME_MIN_VER 0x000000c3
#define AL_CPU_RESUME_MIN_VER_MASK 0x000000ff
/* Field controlling the boot-up of companion cores */
#define AL_NB_INIT_CONTROL (0x8)
#define AL_NB_CONFIG_STATUS_PWR_CTRL(cpu) (0x2020 + (cpu)*0x100)
extern void mpentry(void);
int alpine_serdes_resource_get(uint32_t group, bus_space_tag_t *tag,
bus_addr_t *baddr);
static int platform_mp_get_core_cnt(void);
static int alpine_get_cpu_resume_base(uint32_t *pbase, uint32_t *psize);
static int alpine_get_nb_base(uint32_t *pbase, uint32_t *psize);
static int alpine_get_serdes_base(uint32_t *pbase, uint32_t *psize);
int alpine_serdes_resource_get(uint32_t group, bus_space_tag_t *tag,
bus_addr_t *baddr);
static int
platform_mp_get_core_cnt(void)
{
static int ncores = 0;
phandle_t node;
phandle_t child;
int nchilds;
/* Calculate ncores value only once */
impUnsubmitted
Done
Inline Actions

Shouldn't this be lower case?

imp: Shouldn't this be lower case?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

While this is case insensitive comparison you are right that the practice is lower case. Will fix it.

jpa-semihalf.com: While this is case insensitive comparison you are right that the practice is lower case. Will…
if (ncores)
return (ncores);
__asm __volatile("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (ncores));
ncores = ((ncores >> 24) & 3) + 1;
andrewUnsubmitted
Done
Inline Actions

This needs a newline:

boolean_t
alpine_validate_cpu(...)

Should it be static?

andrew: This needs a newline: boolean_t alpine_validate_cpu(...) Should it be static?
/* Check if number of CPU cores should be limited in SMP mode */
andrewUnsubmitted
Done
Inline Actions

You should use ofw_cpu_early_foreach for this.

andrew: You should use ofw_cpu_early_foreach for this.
if ((node = OF_finddevice("/cpus")) == -1)
goto end;
nchilds = 0;
for (child = OF_child(node); child != 0; child = OF_peer(child)) {
if (fdt_is_compatible(child, "ARM,CORTEX-A15")) {
andrewUnsubmitted
Done
Inline Actions

This should use a function from cpu-v6.h

andrew: This should use a function from cpu-v6.h
nchilds++;
andrewUnsubmitted
Done
Inline Actions

You should add something like the following to armreg.h, then use it here.

#define L2CTLR_NPROC_SHIFT 24
#define L2CTLR_NPROC(r) ((((r) >> L2CTLR_NPROC_SHIFT) & 3) + 1)
andrew: You should add something like the following to armreg.h, then use it here. #define…
}
}
/* Limit CPUs if DTS has configured less than available */
if ((nchilds > 0) && (nchilds < ncores)) {
printf("SMP: limiting number of active CPUs to %d out of %d\n",
nchilds, ncores);
ncores = nchilds;
}
ianUnsubmitted
Done
Inline Actions

the number of cores should also be limited by the hw.ncpu tunable so that you can control it from loader(8).

ian: the number of cores should also be limited by the hw.ncpu tunable so that you can control it…
jpa-semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

We are not supporting loader(8) so that would be dead code.

jpa-semihalf.com: We are not supporting loader(8) so that would be dead code.
ianUnsubmitted
Done
Inline Actions

I don't view "we are not supporting loader(8)" as a valid position without some sort of really strong justification. It's not 2011 anymore where we commit code for a new platform as soon as it can boot a bare kernel.bin that supports a serial console.

loader(8) is part of freebsd and is the standard way to load a kernel. why do you claim to not support it?

ian: I don't view "we are not supporting loader(8)" as a valid position without some sort of really…
impUnsubmitted
Done
Inline Actions

These boards are embedded network processors that live on a PCIe card. They have very limited space for extra stuff not related to the task at hand... While I generally agree we need loader(8) support, this board may be a reasonable exception to that rule.

imp: These boards are embedded network processors that live on a PCIe card. They have very limited…
ianUnsubmitted
Done
Inline Actions

Ah, I didn't realize these were not general-purpose systems oriented towards the end user. For that case, compiling in the dtb and skipping loader(8) seems reasonable.

ian: Ah, I didn't realize these were not general-purpose systems oriented towards the end user. For…
end:
return (ncores);
}
void
platform_mp_init_secondary(void)
{
arm_init_secondary_ic();
}
void
platform_mp_setmaxid(void)
{
int core_cnt;
core_cnt = platform_mp_get_core_cnt();
mp_maxid = core_cnt - 1;
}
int
platform_mp_probe(void)
{
mp_ncpus = platform_mp_get_core_cnt();
return (1);
}
static int
alpine_get_cpu_resume_base(uint32_t *pbase, uint32_t *psize)
{
phandle_t node;
u_long base=0;
u_long size=0;
if (!pbase || !psize)
impUnsubmitted
Not Done
Inline Actions

Shouldn't this be a kassert?

imp: Shouldn't this be a kassert?
jpa-semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

In general that would be the way to go, but other errors are handled with same panic message anyway, so I would rather not duplicate code unless necessary.

jpa-semihalf.com: In general that would be the way to go, but other errors are handled with same panic message…
return (EINVAL);
if ((node = OF_finddevice("/")) == -1)
return (EFAULT);
andrewUnsubmitted
Done
Inline Actions

Do these need to be fdt_ functions, or could you call an ofw_ function?

andrew: Do these need to be fdt_ functions, or could you call an ofw_ function?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I don't have device yet, so the alternative option is to use pure OF_ set of functions to find one, for example wrapped into ofw_search_tree_compatible_device("/simple-bus/al-cpu-resume");

jpa-semihalf.com: I don't have device yet, so the alternative option is to use pure OF_ set of functions to find…
if ((node = fdt_find_compatible(node, "simple-bus", 1)) == 0)
return (EFAULT);
if ((node =
fdt_find_compatible(node, "annapurna-labs,al-cpu-resume", 1)) == 0)
return (EFAULT);
if (fdt_regsize(node, &base, &size))
return (EFAULT);
*pbase = base;
*psize = size;
return (0);
}
static int
alpine_get_nb_base(uint32_t *pbase, uint32_t *psize)
{
phandle_t node;
u_long base=0;
u_long size=0;
if (!pbase || !psize)
impUnsubmitted
Done
Inline Actions

kassert?

imp: kassert?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

Same as above.

jpa-semihalf.com: Same as above.
return (EINVAL);
if ((node = OF_finddevice("/")) == -1)
andrewUnsubmitted
Done
Inline Actions

Missing space, should be base = 0;, and the same for size.

andrew: Missing space, should be `base = 0;`, and the same for size.
return (EFAULT);
andrewUnsubmitted
Done
Inline Actions
if (pbase == NULL || psize == NULL)
andrew: if (pbase == NULL || psize == NULL)
if ((node = fdt_find_compatible(node, "simple-bus", 1)) == 0)
return (EFAULT);
if ((node =
fdt_find_compatible(node, "annapurna-labs,al-nb-service", 1)) == 0)
return (EFAULT);
if (fdt_regsize(node, &base, &size))
return (EFAULT);
*pbase = base;
*psize = size;
return (0);
}
void
platform_mp_start_ap(void)
{
uint32_t physaddr;
vm_offset_t vaddr;
uint32_t val;
uint32_t start_mask;
uint32_t cpu_resume_base;
uint32_t nb_base;
andrewUnsubmitted
Done
Inline Actions

uint32_t is the wrong type for a number of these.

andrew: uint32_t is the wrong type for a number of these.
uint32_t cpu_resume_size;
uint32_t nb_size;
bus_addr_t cpu_resume_baddr;
bus_addr_t nb_baddr;
int a;
if (alpine_get_cpu_resume_base(&cpu_resume_base, &cpu_resume_size))
panic("Couldn't resolve cpu_resume_base address\n");
if (alpine_get_nb_base(&nb_base, &nb_size))
panic("Couldn't resolve_nb_base address\n");
/* Proceed with start addresses for additional CPUs */
ianUnsubmitted
Done
Inline Actions

Unless this platform has mapping control registers that allow repositioning the window for accessing hardware regs, none of these bus_space_map() calls should be using fdt_immr_pa, they should just be using the physical addresses from the fdt data.

As far as I know, only Marvel chips have such a feature (and maybe not even all of them). fdt_immr_pa should never appear outside of arm/mv (and some polution where it leaks out into dev/fdt that we really need to fix).

ian: Unless this platform has mapping control registers that allow repositioning the window for…
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I removed all references to fdt_immr_pa and used the preferred way of setting addresses (arm_devmap_add_entry())

jpa-semihalf.com: I removed all references to fdt_immr_pa and used the preferred way of setting addresses…
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + cpu_resume_base,
cpu_resume_size, 0, &cpu_resume_baddr))
panic("Couldn't map CPU-resume area");
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + nb_base,
nb_size, 0, &nb_baddr))
panic("Couldn't map NB-service area");
/* Proceed with start addresses for additional CPUs */
val = bus_space_read_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_WATERMARK_REG);
if (((val & AL_CPU_RESUME_MAGIC_NUM_MASK) != AL_CPU_RESUME_MAGIC_NUM) ||
((val & AL_CPU_RESUME_MIN_VER_MASK) < AL_CPU_RESUME_MIN_VER)) {
panic("CPU-resume device is not compatible");
}
vaddr = (vm_offset_t)mpentry;
physaddr = pmap_kextract(vaddr);
for (a = 1; a < platform_mp_get_core_cnt(); a++) {
/* Power up the core */
bus_space_write_4(fdtbus_bs_tag, nb_baddr,
AL_NB_CONFIG_STATUS_PWR_CTRL(a), 0);
mb();
/* Enable resume */
val = bus_space_read_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_FLAGS(a));
val &= ~AL_CPU_RESUME_FLG_PERCPU_DONT_RESUME;
bus_space_write_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_FLAGS(a), val);
mb();
/* Set resume physical address */
bus_space_write_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_RADDR_REG(a), physaddr);
mb();
}
/* Release cores from reset */
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + nb_base,
nb_size, 0, &nb_baddr))
panic("Couldn't map NB-service area");
start_mask = (1 << platform_mp_get_core_cnt()) - 1;
/* Release cores from reset */
val = bus_space_read_4(fdtbus_bs_tag, nb_baddr, AL_NB_INIT_CONTROL);
val |= start_mask;
andrewUnsubmitted
Done
Inline Actions

Is a dmb enough here?

andrew: Is a dmb enough here?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

armv7_sev() issues dsb internally, so I think it is even not needed here, but for the sake of future changes to armv7_sev() I will replace the mb() accordingly.

jpa-semihalf.com: armv7_sev() issues dsb internally, so I think it is even not needed here, but for the sake of…
bus_space_write_4(fdtbus_bs_tag, nb_baddr, AL_NB_INIT_CONTROL, val);
ianUnsubmitted
Done
Inline Actions

Why do we even do a SEV here? I have the feeling this has been pasted into every soc mp file and doesn't do anything useful on any of them.

ian: Why do we even do a SEV here? I have the feeling this has been pasted into every soc mp file…
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I removed it and so far so good. On the other hand I kept the dsb().

jpa-semihalf.com: I removed it and so far so good. On the other hand I kept the dsb().
mb();
armv7_sev();
bus_space_unmap(fdtbus_bs_tag, nb_baddr, nb_size);
bus_space_unmap(fdtbus_bs_tag, cpu_resume_baddr, cpu_resume_size);
}
void
platform_ipi_send(cpuset_t cpus, u_int ipi)
{
pic_ipi_send(cpus, ipi);
}
andrewUnsubmitted
Done
Inline Actions

This has the same style issues as alpine_get_nb_base.

andrew: This has the same style issues as alpine_get_nb_base.
View Options

usr.sbin/bsdinstall/scripts/bootconfig

Loading...
View Options

usr.sbin/bsdinstall/scripts/zfsboot

Loading...