Shouldn't this be lower case?

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

These should be #define<tab>AL_...

It's only this way on mv and at91. On all other SoCs we don't reserve a virtual address, instead we let the vm code do it for us. In the at91 case it is to help transition to fdt, however this is not the case here.

Moreover, the only place I can find that uses fdt_immr_pa, other than in the fdt code to set it, is in sys/arm/mv. I would prefer we don't use these unless they are absolutely needed, and if this is the case we would need a comment explaining why this is the case.

This should use a function from cpu-v6.h

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)

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

uint32_t is the wrong type for a number of these.

Is a dmb enough here?

I suspect what Warner was asking was to remove device_printf_dbg, and use the following in it's place:

if (bootverbose)
  device_printf(dev, "message\n");

Are you sure about this?

Why is addr a void **?

Where in this file are you referencing? I don't see anything it could be.

Can you collect these at the top of the file.

We normally name a device_t dev

fdt_

(There are a few) extra (parenthesis). And reg_size is not a boolean, it should be reg_size > 0

Extra newline

This is missing a few spaces.

This could be split out to a new review.

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");

Oops, I though I got rid of it long time ago... thanks!

Do you mean bus_addr_t * ?

This may refer to ancient times, until 2013: 7b011cd9377544934fa3928d1f40d7d5973f7a9f, I will remove the description.

Yes, that would be a better type.

Well, I could not find any suitable ofw_ function. The one needed, would be, de facto, an exact rewrite of fdt_ into ofw_ version. What do you suggest? There is one reference, from PowerPC version, but that is far from generic, I think.

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.

I agree, this is copied from the bad old way of doing things. Please use any of the armv6 platforms except mv as an example of the modern way.

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

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).

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.

Now it should just use std.armv6.

We do whatever we need to, to operate with the published dts source. Sometimes it's painful.

The old platforms are slowly being retrofitted to use the published source, and all new platforms for the past year or so have been using the source in sys/gnu. If source for this platform isn't in sys/gnu yet we can import it.

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

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

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

I switched to it, thanks!

As for now there is no official version of GNU/Linux .dts so until that appears I would rather stick to what we have here. When things calm down this should be revised.

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?

https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/alpine-db.dts
https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/alpine.dtsi

If you suggest adjusting to the available version, I am fine about that. However, the links you provided are from 4.1-rc3. In stable, 4.0.2, they are missing: https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/arch/arm/boot/dts?id=refs/tags/v4.0.2

@imp, imported into vendor and brought into sys/contrib/<something>, similar to sys/contrib/octeon-sdk?

That's my suggestion.

For reference, the appropriate top-level vendor directory: https://svnweb.freebsd.org/base/vendor-sys/

You'd import a verbatim copy of an upstream release, then merge it to sys/contrib and apply any changes necessary. Let me know if you'll need help with the process.

True, but it's a lot more awkward to retrofit the vendor import if there is a new update in the future. If nothing else we should at least reference the hal drop in the commit message - e.g. something like

The files in the hal/ subdirectory are an unmodified copy of 
<annapurna-alpine-hal-20150317.txz> with sha1 hash
f1d2d2f924e986ac86fdf7b36c94bcdf32beec15.

This needs a newline:

boolean_t
alpine_validate_cpu(...)

Should it be static?

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

This has the same style issues as alpine_get_nb_base.

So they will be released in Linux 4.1. We should try to reduce the difference between what we add and the 4.1-rc3 files.

Let's put them here for the moment. After 4.1 is out, I'd planned on doing another round of dts imports. If they are identical, that process will automatically move it there.

You can remove this and add device gic to your kernel config, I've moved it there in rS282715, and added it to all the kernel configs in rS282723.

You would need either dmb or dsb, the latter provides all the guarantees of the former.

No, I plan to add to hal when introducing support for eth. I will remove the unnecessary dmb() now.