Support for Alpine platform from Annapurna Labs

[jpa-semihalf.com]

Summary

This is an initial drop for Alpine platform support.

[jpa-semihalf.com]

Thank you all for your feedback. Unless answered to the remarks, I followed your recommendations, which results in the second version. Please, have a look at this.

[imp]

Just a quick peek between other things, looking goodish. Will look in more detail in a bit.

sys/arm/annapurna/alpine/alpine_machdep_mp.c

84

Shouldn't this be lower case?

[jpa-semihalf.com]

sys/arm/annapurna/alpine/alpine_machdep_mp.c

84

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

[andrew]

sys/arm/annapurna/alpine/alpine_machdep.c

70

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

144

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.

sys/arm/annapurna/alpine/alpine_machdep_mp.c

97

This should use a function from cpu-v6.h

98

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)

147

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

202

uint32_t is the wrong type for a number of these.

263

Is a dmb enough here?

sys/arm/annapurna/alpine/alpine_pci.c

87

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

239

Are you sure about this?

383

Why is addr a void **?

634

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

902

Can you collect these at the top of the file.

1160

We normally name a device_t dev

1180

fdt_

1181

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

1348

Extra newline

1405

This is missing a few spaces.

sys/arm/arm/machdep.c

167 ↗

(On Diff #5049)

This could be split out to a new review.

[jpa-semihalf.com]

sys/arm/annapurna/alpine/alpine_machdep_mp.c

147

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

sys/arm/annapurna/alpine/alpine_pci.c

239

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

383

Do you mean bus_addr_t * ?

634

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

[andrew]

sys/arm/annapurna/alpine/alpine_pci.c

383

Yes, that would be a better type.

[jpa-semihalf.com]

sys/arm/annapurna/alpine/alpine_pci.c

1180

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.

[jpa-semihalf.com]

sys/arm/annapurna/alpine/alpine_machdep_mp.c

263

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.

[• ian]

sys/arm/annapurna/alpine/alpine_machdep.c

145

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.

sys/arm/annapurna/alpine/alpine_machdep_mp.c

107

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

215

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

264

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.

sys/arm/conf/ALPINE

51

Now it should just use std.armv6.

sys/boot/fdt/dts/arm/annapurna-alpine.dts

3

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.

[jpa-semihalf.com]

sys/arm/annapurna/alpine/alpine_machdep_mp.c

107

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

215

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

264

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

sys/arm/conf/ALPINE

51

I switched to it, thanks!

sys/boot/fdt/dts/arm/annapurna-alpine.dts

3

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.

[jpa-semihalf.com]

This revision addresses all your pointers. I hope all problems are solved now.

[• ian]

sys/arm/annapurna/alpine/alpine_machdep_mp.c

107

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?

sys/boot/fdt/dts/arm/annapurna-alpine.dts

3

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

[jpa-semihalf.com]

sys/boot/fdt/dts/arm/annapurna-alpine.dts

3

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

[emaste]

sys/arm/annapurna/alpine/hal/al_hal_iofic.h

2

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

[imp]

sys/arm/annapurna/alpine/hal/al_hal_iofic.h

2

That's my suggestion.

[imp]

I think I'm mostly happy with this right now. While I'd like to see the SDK/HAL code done like we did Octeon's, if it is a one-shot deal, then maybe that's not so important.

sys/arm/annapurna/alpine/alpine_machdep_mp.c

107

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.

sys/arm/annapurna/alpine/hal/al_hal_iofic.h

2

But only if we think that there will be another drop. It makes sense in Oceteon land because we update that from time to time. If this is just a one-shot, it doesn't matter so much where it lives.

[emaste]

sys/arm/annapurna/alpine/hal/al_hal_iofic.h

2

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.

2

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.

[andrew]

Two points:

1. If there is any chance the vendor code will ever be updated it should be committed though the vendor area in subversion.
2. I think the pci driver isn't ready, can you create a new review for just this.

sys/arm/annapurna/alpine/alpine_machdep_mp.c

90

This needs a newline:

boolean_t
alpine_validate_cpu(...)

Should it be static?

175

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

177

if (pbase == NULL || psize == NULL)

278

This has the same style issues as alpine_get_nb_base.

sys/boot/fdt/dts/arm/annapurna-alpine.dts

3

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.

[• ian]

I'm putting in an 'accept' here but want to note that I haven't carefully reviewed the entire change, partly because of time and mostly because it's so PCI-related and I don't know much about PCI. I do however want to record that my previous comments don't amount to blocking the changes.

sys/arm/annapurna/alpine/alpine_machdep_mp.c

107

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.

sys/boot/fdt/dts/arm/annapurna-alpine.dts

3

I'm about 100% ignorant of linux versioning, release schedules, etc. If it's difficult to import the files into our sys/gnu area right now because of where they're at in the linux universe, then I agree that we should try to make the files we put into our sys/boot area as close as we can to what we think the final linux/official files will look like.

[imp]

I think I'm mostly happy with this right now. While I'd like to see the SDK/HAL code done like we did Octeon's, if it is a one-shot deal, then maybe that's not so important.

sys/boot/fdt/dts/arm/annapurna-alpine.dts

3

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.

[jpa-semihalf.com]

Thank you for discussion and suggestions. This revision fixes reported style + a bit more.
As for the discussion topics, a roadmap with regard to mentioned issues looks like this:

1. delivery the platform support as is - this is a coherent change in terms of platform and hal
2. the next planned code drop involves msix support so relates to pci enhancement and cleanup. At this stage I will deal with dts compatibility - this will let me parallelise some works.
3. meanwhile, I will move the hal to sys/contrib. This shall be a painless transition and will prelude another hal drop in for eth driver

Does it sound reasonable?

[andrew]

In D2340#46897, @jpa-semihalf.com wrote:

Thank you for discussion and suggestions. This revision fixes reported style + a bit more.
As for the discussion topics, a roadmap with regard to mentioned issues looks like this:

1. delivery the platform support as is - this is a coherent change in terms of platform and hal

Can you move the pci changes to a new review? I still have a few issues with them, but don't think they should hold up this going in the tree.

sys/arm/annapurna/alpine/files.alpine

10

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.

[imp]

In D2340#46897, @jpa-semihalf.com wrote:

Thank you for discussion and suggestions. This revision fixes reported style + a bit more.
As for the discussion topics, a roadmap with regard to mentioned issues looks like this:

1. delivery the platform support as is - this is a coherent change in terms of platform and hal
2. the next planned code drop involves msix support so relates to pci enhancement and cleanup. At this stage I will deal with dts compatibility - this will let me parallelise some works.
3. meanwhile, I will move the hal to sys/contrib. This shall be a painless transition and will prelude another hal drop in for eth driver

Does it sound reasonable?

Sounds reasonable to me. Andy's idea of splitting out PCI changes might help us with some of the Phabricator issues we've seen on other large reviews.

[jpa-semihalf.com]

This version is without PCI support - that will appear in a separate review.

[andrew]

Did you intend to change the HAL in this update?

sys/arm/annapurna/alpine/hal/al_hal_plat_services.h

297

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

[jpa-semihalf.com]

sys/arm/annapurna/alpine/hal/al_hal_plat_services.h

297

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

[jpa-semihalf.com]

Removed spare dmb() in favor to dsb() only. That change in hal appeared when fixing compilation errors after extracting PCI.

[emaste]

In D2340#46897, @jpa-semihalf.com wrote:

3. meanwhile, I will move the hal to sys/contrib. This shall be a painless transition and will prelude another hal drop in for eth driver

That sounds reasonable - let me know if you have questions or want assistance when it comes to doing the actual import into FreeBSD SVN in vendor-sys.