CNS11XX FreeBSD port completed
Stanislav Sedov
stas at FreeBSD.org
Sun Jan 3 11:05:32 UTC 2010
On Sun, 3 Jan 2010 15:33:28 +0700
Yohanes Nugroho <yohanes at gmail.com> mentioned:
> From: Yohanes Nugroho <yohanes at gmail.com>
> To: Rui Paulo <rpaulo at freebsd.org>
> Cc: freebsd-arm at freebsd.org
> Subject: Re: CNS11XX FreeBSD port completed
> Date: Sun, 3 Jan 2010 15:33:28 +0700
> Sender: owner-freebsd-arm at freebsd.org
>
> On Tue, Dec 29, 2009 at 8:53 PM, Rui Paulo <rpaulo at freebsd.org> wrote:
>
> Hi Rui,
>
> Thank you for spending some time to read my code. Here is the new patch.
>
First of all, thanks for your hard work on this platform support!
Here're some comments you may find useful.
>
> [cns11xx_20090103.diff text/x-patch (141,2KB)]
<...>
> --- arm/arm/cpufunc_asm_fa526.S (revision 0)
> +++ arm/arm/cpufunc_asm_fa526.S (revision 0)
<...>
> +ENTRY(fa526_cpu_sleep)
> + mov r0, #0
> +/* nop
> + nop*/
> + mcr p15, 0, r0, c7, c0, 4 /* Wait for interrup*/
^^^^^^^^^^
A small typo?
> + mov pc, lr
> +
> +ENTRY(fa526_flush_prefetchbuf)
<...>
> Index: arm/econa/econa_var.h
> ===================================================================
> --- arm/econa/econa_var.h (revision 0)
> +++ arm/econa/econa_var.h (revision 0)
> @@ -0,0 +1,50 @@
> +/*-
> + * Copyright (c) 2009 Yohanes Nugroho <yohanes at gmail.com>.
> + * 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 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 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.
> + */
> +
> +#ifndef _ARM_ECONA_VAR_H
> +#define _ARM_ECONA_VAR_H
> +
> +extern bus_space_tag_t obio_tag;
> +
> +struct econa_softc {
> + device_t dev;
> + bus_space_tag_t ec_st;
> + bus_space_handle_t ec_sh;
> + bus_space_handle_t ec_sys_sh;
> + bus_space_handle_t ec_system_sh;
> + struct rman ec_irq_rman;
> + struct rman ec_mem_rman;
> +};
> +
> +struct econa_ivar {
> + struct resource_list resources;
> +};
> +
> +void power_on_network_interface(void);
> +unsigned int get_tclk(void);
^^^^^^^^
style(9) recommends using a TAB after the return type value
in prototypes.
> +
> +
> +#endif
<...>
> Index: arm/econa/econa_machdep.c
> ===================================================================
> --- arm/econa/econa_machdep.c (revision 0)
> +++ arm/econa/econa_machdep.c (revision 0)
> @@ -0,0 +1,409 @@
> +/*-
> + * Copyright (c) 2009 Yohanes Nugroho <yohanes at gmail.com>
> + * Copyright (c) 1994-1998 Mark Brinicombe.
> + * Copyright (c) 1994 Brini.
> + * All rights reserved.
> + *
> + * This code is derived from software written for Brini by Mark Brinicombe
> + *
> + * 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.
> + * 3. All advertising materials mentioning features or use of this software
> + * must display the following acknowledgement:
> + * This product includes software developed by Brini.
> + * 4. The name of the company nor the name of the author may be used to
> + * endorse or promote products derived from this software without specific
> + * prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY BRINI ``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 BRINI 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 "opt_msgbuf.h"
> +
> +#include <sys/cdefs.h>
> +__FBSDID("$FreeBSD$");
> +
> +#define _ARM32_BUS_DMA_PRIVATE
> +#include <sys/param.h>
> +#include <sys/systm.h>
> +#include <sys/sysproto.h>
> +#include <sys/signalvar.h>
> +#include <sys/imgact.h>
> +#include <sys/kernel.h>
> +#include <sys/ktr.h>
> +#include <sys/linker.h>
> +#include <sys/lock.h>
> +#include <sys/malloc.h>
> +#include <sys/mutex.h>
> +#include <sys/pcpu.h>
> +#include <sys/proc.h>
> +#include <sys/ptrace.h>
> +#include <sys/cons.h>
> +#include <sys/bio.h>
> +#include <sys/bus.h>
> +#include <sys/buf.h>
> +#include <sys/exec.h>
> +#include <sys/kdb.h>
> +#include <sys/msgbuf.h>
> +#include <machine/reg.h>
> +#include <machine/cpu.h>
> +
> +#include <vm/vm.h>
> +#include <vm/pmap.h>
> +#include <vm/vm_object.h>
> +#include <vm/vm_page.h>
> +#include <vm/vm_pager.h>
> +#include <vm/vm_map.h>
> +#include <vm/vnode_pager.h>
> +#include <machine/pmap.h>
> +#include <machine/vmparam.h>
> +#include <machine/pcb.h>
> +#include <machine/undefined.h>
> +#include <machine/machdep.h>
> +#include <machine/metadata.h>
> +#include <machine/armreg.h>
> +#include <machine/bus.h>
> +#include <sys/reboot.h>
^^^^^^^^^^^^^^^^^^^^^^^^^^^
This header appears to be out of order. sys/ headers should come first.
> +#include "econa_reg.h"
> +
> +/* Page table for mapping proc0 zero page */
> +#define KERNEL_PT_SYS 0
^^^
Extra space inserted.
> +#define KERNEL_PT_KERN 1
> +#define KERNEL_PT_KERN_NUM 22
> +/* L2 table for mapping after kernel */
> +#define KERNEL_PT_AFKERNEL KERNEL_PT_KERN + KERNEL_PT_KERN_NUM
> +#define KERNEL_PT_AFKERNEL_NUM 5
> +
> +/* this should be evenly divisable by PAGE_SIZE / L2_TABLE_SIZE_REAL (or 4) */
> +#define NUM_KERNEL_PTS (KERNEL_PT_AFKERNEL + KERNEL_PT_AFKERNEL_NUM)
> +
> +/* Define various stack sizes in pages */
> +#define IRQ_STACK_SIZE 1
> +#define ABT_STACK_SIZE 1
> +#define UND_STACK_SIZE 1
> +
> +extern u_int data_abort_handler_address;
> +extern u_int prefetch_abort_handler_address;
> +extern u_int undefined_handler_address;
> +
> +struct pv_addr kernel_pt_table[NUM_KERNEL_PTS];
> +
> +extern void *_end;
> +
> +extern int *end;
> +
> +struct pcpu __pcpu;
> +struct pcpu *pcpup = &__pcpu;
> +
> +/* Physical and virtual addresses for some global pages */
> +
> +vm_paddr_t phys_avail[10];
> +vm_paddr_t dump_avail[4];
> +vm_offset_t physical_pages;
> +
> +struct pv_addr systempage;
> +struct pv_addr msgbufpv;
> +struct pv_addr irqstack;
> +struct pv_addr undstack;
> +struct pv_addr abtstack;
> +struct pv_addr kernelstack;
> +
> +static void *boot_arg1;
> +static void *boot_arg2;
> +
> +static struct trapframe proc0_tf;
> +
> +/* Static device mappings. */
> +static const struct pmap_devmap econa_devmap[] = {
> + {
> + /*
> + * This maps DDR SDRAM
> + */
> + ECONA_SDRAM_BASE, /*virtual*/
> + ECONA_SDRAM_BASE, /*physical*/
> + ECONA_SDRAM_SIZE, /*size*/
> + VM_PROT_READ|VM_PROT_WRITE,
> + PTE_NOCACHE,
> + },
> + /*
> + * Map the on-board devices VA == PA so that we can access them
> + * with the MMU on or off.
> + */
> + {
> + /*
> + * This maps the interrupt controller, the UART
> + * and the timer.
> + */
> + ECONA_IO_BASE, /*virtual*/
> + ECONA_IO_BASE, /*physical*/
> + ECONA_IO_SIZE, /*size*/
> + VM_PROT_READ|VM_PROT_WRITE,
> + PTE_NOCACHE,
> + },
> + {
> + /*
> + * OHCI + EHCI
> + */
> + ECONA_OHCI_VBASE, /*virtual*/
> + ECONA_OHCI_PBASE, /*physical*/
> + ECONA_USB_SIZE, /*size*/
> + VM_PROT_READ|VM_PROT_WRITE,
> + PTE_NOCACHE,
> + },
> + {
> + /*
> + * CFI
> + */
> + ECONA_CFI_VBASE, /*virtual*/
> + ECONA_CFI_PBASE, /*physical*/
> + ECONA_CFI_SIZE,
> + VM_PROT_READ|VM_PROT_WRITE,
> + PTE_NOCACHE,
> + },
> + {
> + 0,
> + 0,
> + 0,
> + 0,
> + 0,
> + }
> +};
> +
> +
> +void *
> +initarm(void *arg, void *arg2)
> +{
> + struct pv_addr kernel_l1pt;
> + int loop, i;
> + u_int l1pagetable;
> + vm_offset_t freemempos;
> + vm_offset_t afterkern;
> + uint32_t memsize;
> + vm_offset_t lastaddr;
> + volatile uint32_t * ddr = (uint32_t *)0x4000000C;
> + int mem_info;
Style(9) recommends sorting local variable declarations by size, then
alphabetically. This makes reading easier.
> +
> + boot_arg1 = arg;
> + boot_arg2 = arg2;
> + boothowto = RB_VERBOSE;
> + boothowto |= RB_SINGLE;
Why are you setting boothowto here? Is it possible for the board to boot
to multiuser?
> +
> + set_cpufuncs();
> + lastaddr = fake_preload_metadata();
> + pcpu_init(pcpup, 0, sizeof(struct pcpu));
> + PCPU_SET(curthread, &thread0);
> +
> +
> + freemempos = (lastaddr + PAGE_MASK) & ~PAGE_MASK;
> + /* Define a macro to simplify memory allocation */
> +#define valloc_pages(var, np) \
> + alloc_pages((var).pv_va, (np)); \
> + (var).pv_pa = (var).pv_va + (KERNPHYSADDR - KERNVIRTADDR);
> +
> +#define alloc_pages(var, np) \
> + (var) = freemempos; \
> + freemempos += (np * PAGE_SIZE); \
> + memset((char *)(var), 0, ((np) * PAGE_SIZE));
> +
> + while (((freemempos - L1_TABLE_SIZE) & (L1_TABLE_SIZE - 1)) != 0)
> + freemempos += PAGE_SIZE;
> + valloc_pages(kernel_l1pt, L1_TABLE_SIZE / PAGE_SIZE);
> + for (loop = 0; loop < NUM_KERNEL_PTS; ++loop) {
> + if (!(loop % (PAGE_SIZE / L2_TABLE_SIZE_REAL))) {
> + valloc_pages(kernel_pt_table[loop],
> + L2_TABLE_SIZE / PAGE_SIZE);
> + } else {
> + kernel_pt_table[loop].pv_va = freemempos -
> + (loop % (PAGE_SIZE / L2_TABLE_SIZE_REAL)) *
> + L2_TABLE_SIZE_REAL;
> + kernel_pt_table[loop].pv_pa =
> + kernel_pt_table[loop].pv_va - KERNVIRTADDR +
> + KERNPHYSADDR;
> + }
> + i++;
> + }
> +
> +
Extra blank line inserted.
> + /*
> + * Allocate a page for the system page mapped to V0x00000000
> + * This page will just contain the system vectors and can be
> + * shared by all processes.
> + */
> + valloc_pages(systempage, 1);
> +
> + /* Allocate stacks for all modes */
> + valloc_pages(irqstack, IRQ_STACK_SIZE);
> + valloc_pages(abtstack, ABT_STACK_SIZE);
> + valloc_pages(undstack, UND_STACK_SIZE);
> + valloc_pages(kernelstack, KSTACK_PAGES);
> + valloc_pages(msgbufpv, round_page(MSGBUF_SIZE) / PAGE_SIZE);
> +
> + /*
> + * Now we start construction of the L1 page table
> + * We start by mapping the L2 page tables into the L1.
> + * This means that we can replace L1 mappings later on if necessary
> + */
> + l1pagetable = kernel_l1pt.pv_va;
> +
> + /* Map the L2 pages tables in the L1 page table */
> + pmap_link_l2pt(l1pagetable, ARM_VECTORS_HIGH,
> + &kernel_pt_table[KERNEL_PT_SYS]);
> + for (i = 0; i < KERNEL_PT_KERN_NUM; i++)
> + pmap_link_l2pt(l1pagetable, KERNBASE + i * L1_S_SIZE,
> + &kernel_pt_table[KERNEL_PT_KERN + i]);
> + pmap_map_chunk(l1pagetable, KERNBASE, PHYSADDR,
> + (((uint32_t)lastaddr - KERNBASE) + PAGE_SIZE) & ~(PAGE_SIZE - 1),
> + VM_PROT_READ|VM_PROT_WRITE, PTE_CACHE);
> + afterkern = round_page((lastaddr + L1_S_SIZE) & ~(L1_S_SIZE - 1));
> + for (i = 0; i < KERNEL_PT_AFKERNEL_NUM; i++) {
> + pmap_link_l2pt(l1pagetable, afterkern + i * L1_S_SIZE,
> + &kernel_pt_table[KERNEL_PT_AFKERNEL + i]);
> + }
> +
> + /* Map the vector page. */
> + pmap_map_entry(l1pagetable, ARM_VECTORS_HIGH, systempage.pv_pa,
> + VM_PROT_READ|VM_PROT_WRITE, PTE_CACHE);
> +
> +
> + /* Map the stack pages */
> + pmap_map_chunk(l1pagetable, irqstack.pv_va, irqstack.pv_pa,
> + IRQ_STACK_SIZE * PAGE_SIZE, VM_PROT_READ|VM_PROT_WRITE, PTE_CACHE);
> + pmap_map_chunk(l1pagetable, abtstack.pv_va, abtstack.pv_pa,
> + ABT_STACK_SIZE * PAGE_SIZE, VM_PROT_READ|VM_PROT_WRITE, PTE_CACHE);
> + pmap_map_chunk(l1pagetable, undstack.pv_va, undstack.pv_pa,
> + UND_STACK_SIZE * PAGE_SIZE, VM_PROT_READ|VM_PROT_WRITE, PTE_CACHE);
> + pmap_map_chunk(l1pagetable, kernelstack.pv_va, kernelstack.pv_pa,
> + KSTACK_PAGES * PAGE_SIZE, VM_PROT_READ|VM_PROT_WRITE, PTE_CACHE);
> +
> + pmap_map_chunk(l1pagetable, kernel_l1pt.pv_va, kernel_l1pt.pv_pa,
> + L1_TABLE_SIZE, VM_PROT_READ|VM_PROT_WRITE, PTE_PAGETABLE);
> + pmap_map_chunk(l1pagetable, msgbufpv.pv_va, msgbufpv.pv_pa,
> + MSGBUF_SIZE, VM_PROT_READ|VM_PROT_WRITE, PTE_CACHE);
> +
> + for (loop = 0; loop < NUM_KERNEL_PTS; ++loop) {
> + pmap_map_chunk(l1pagetable, kernel_pt_table[loop].pv_va,
> + kernel_pt_table[loop].pv_pa, L2_TABLE_SIZE,
> + VM_PROT_READ|VM_PROT_WRITE, PTE_PAGETABLE);
> + }
> +
> +
> + pmap_devmap_bootstrap(l1pagetable, econa_devmap);
> +
> + cpu_domains((DOMAIN_CLIENT << (PMAP_DOMAIN_KERNEL*2)) | DOMAIN_CLIENT);
> +
> + setttb(kernel_l1pt.pv_pa);
> +
> + cpu_tlb_flushID();
> +
> + cpu_domains(DOMAIN_CLIENT << (PMAP_DOMAIN_KERNEL*2));
> +
> + cninit();
> +
> + mem_info = ((*ddr) >> 4) & 0x3;
> +
> + memsize = (8<<mem_info)*1024*1024;
> +
> + physmem = memsize / PAGE_SIZE;
A lot of gratuitous (?) blank lines in this chunk of code. Do we really need
them? style(9) recommends to avoid extra white space.
> +
> + /*
> + * Pages were allocated during the secondary bootstrap for the
> + * stacks for different CPU modes.
> + * We must now set the r13 registers in the different CPU modes to
> + * point to these stacks.
> + * Since the ARM stacks use STMFD etc. we must set r13 to the top end
> + * of the stack memory.
> + */
> + cpu_control(CPU_CONTROL_MMU_ENABLE, CPU_CONTROL_MMU_ENABLE);
> +
> + set_stackptr(PSR_IRQ32_MODE,
> + irqstack.pv_va + IRQ_STACK_SIZE * PAGE_SIZE);
> + set_stackptr(PSR_ABT32_MODE,
> + abtstack.pv_va + ABT_STACK_SIZE * PAGE_SIZE);
> + set_stackptr(PSR_UND32_MODE,
> + undstack.pv_va + UND_STACK_SIZE * PAGE_SIZE);
> +
> + /*
> + * We must now clean the cache again....
> + * Cleaning may be done by reading new data to displace any
> + * dirty data in the cache. This will have happened in setttb()
> + * but since we are boot strapping the addresses used for the read
> + * may have just been remapped and thus the cache could be out
> + * of sync. A re-clean after the switch will cure this.
> + * After booting there are no gross relocations of the kernel thus
> + * this problem will not occur after initarm().
> + */
> + cpu_idcache_wbinv_all();
> +
> + /* Set stack for exception handlers */
> +
Extra blank line inserted.
> + data_abort_handler_address = (u_int)data_abort_handler;
> + prefetch_abort_handler_address = (u_int)prefetch_abort_handler;
> + undefined_handler_address = (u_int)undefinedinstruction_bounce;
> + undefined_init();
> +
> + proc_linkup0(&proc0, &thread0);
> + thread0.td_kstack = kernelstack.pv_va;
> + thread0.td_pcb = (struct pcb *)
> + (thread0.td_kstack + KSTACK_PAGES * PAGE_SIZE) - 1;
> + thread0.td_pcb->pcb_flags = 0;
> + thread0.td_frame = &proc0_tf;
> + pcpup->pc_curpcb = thread0.td_pcb;
> +
> +
Same here.
> + arm_vector_init(ARM_VECTORS_HIGH, ARM_VEC_ALL);
> +
> + pmap_curmaxkvaddr = afterkern + L1_S_SIZE * (KERNEL_PT_KERN_NUM - 1);
> +
> + /*
> + * ARM_USE_SMALL_ALLOC uses dump_avail, so it must be filled before
> + * calling pmap_bootstrap.
> + */
> + dump_avail[0] = PHYSADDR;
> + dump_avail[1] = PHYSADDR + memsize;
> + dump_avail[2] = 0;
> + dump_avail[3] = 0;
> +
> + pmap_bootstrap(freemempos,
> + KERNVIRTADDR + 3 * memsize,
> + &kernel_l1pt);
> +
> + msgbufp = (void*)msgbufpv.pv_va;
> + msgbufinit(msgbufp, MSGBUF_SIZE);
> +
> + mutex_init();
> +
> + i = 0;
> +#if PHYSADDR != KERNPHYSADDR
> + phys_avail[i++] = PHYSADDR;
> + phys_avail[i++] = KERNPHYSADDR;
> +#endif
> + phys_avail[i++] = virtual_avail - KERNVIRTADDR + KERNPHYSADDR;
> +
> + phys_avail[i++] = PHYSADDR + memsize;
> + phys_avail[i++] = 0;
> + phys_avail[i++] = 0;
> + /* Do basic tuning, hz etc */
> + init_param1();
> + init_param2(physmem);
> + kdb_init();
> +
> + return ((void *)(kernelstack.pv_va + USPACE_SVC_STACK_TOP -
> + sizeof(struct pcb)));
> +}
> Index: arm/econa/if_ece.c
> ===================================================================
> --- arm/econa/if_ece.c (revision 0)
> +++ arm/econa/if_ece.c (revision 0)
> @@ -0,0 +1,1939 @@
> +/*-
> + * Copyright (c) 2009 Yohanes Nugroho <yohanes at gmail.com>
> + * 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 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 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.
> + */
> +
> +
Extra white space.
> +#include <sys/cdefs.h>
> +__FBSDID("$FreeBSD: src/sys/arm/econa/if_ece.c$");
> +
> +#include <sys/param.h>
> +#include <sys/systm.h>
> +#include <sys/bus.h>
> +#include <sys/kernel.h>
> +#include <sys/mbuf.h>
> +#include <sys/malloc.h>
> +#include <sys/module.h>
> +#include <sys/rman.h>
> +#include <sys/socket.h>
> +#include <sys/sockio.h>
> +#include <sys/sysctl.h>
> +#include <machine/bus.h>
> +#include <sys/taskqueue.h>
> +
> +#include <net/ethernet.h>
> +#include <net/if.h>
> +#include <net/if_arp.h>
> +#include <net/if_dl.h>
> +#include <net/if_media.h>
> +#include <net/if_types.h>
> +#include <net/if_vlan_var.h>
> +
> +#ifdef INET
> +#include <netinet/in.h>
> +#include <netinet/in_systm.h>
> +#include <netinet/in_var.h>
> +#include <netinet/ip.h>
> +#endif
> +
> +#include <net/bpf.h>
> +#include <net/bpfdesc.h>
> +
> +#include <dev/mii/mii.h>
> +#include <dev/mii/miivar.h>
> +#include <arm/econa/if_ecereg.h>
> +#include <arm/econa/if_ecevar.h>
> +#include <arm/econa/econa_var.h>
> +#include <machine/intr.h>
> +
> +#include "miibus_if.h"
> +
> +static uint8_t
> +vlan0_mac[ETHER_ADDR_LEN] = {0x00, 0xaa, 0xbb, 0xcc, 0xdd, 0x19};
> +
> +/*
> + * Boot loader expects the hardware state to be the same when we
> + * restart the device (warm boot), so we need to save the initial
> + * config values.
> + */
> +int initial_switch_config;
> +int initial_cpu_config;
> +int initial_port0_config;
> +int initial_port1_config;
> +
> +static inline uint32_t
> +RD4(struct ece_softc *sc, bus_size_t off)
> +{
> + return (bus_read_4(sc->mem_res, off));
> +}
> +
> +static inline void
> +WR4(struct ece_softc *sc, bus_size_t off, uint32_t val)
> +{
> + bus_write_4(sc->mem_res, off, val);
> +}
Funcctions without local variable should have an extra blank line at the
beginning, per style(9). Also, all-capitals names are usually used for macros,
not function names.
> +
> +#define ECE_LOCK(_sc) mtx_lock(&(_sc)->sc_mtx)
> +#define ECE_UNLOCK(_sc) mtx_unlock(&(_sc)->sc_mtx)
> +#define ECE_LOCK_INIT(_sc) \
> + mtx_init(&_sc->sc_mtx, device_get_nameunit(_sc->dev), \
> + MTX_NETWORK_LOCK, MTX_DEF)
> +
> +#define ECE_TXLOCK(_sc) mtx_lock(&(_sc)->sc_mtx_tx)
> +#define ECE_TXUNLOCK(_sc) mtx_unlock(&(_sc)->sc_mtx_tx)
> +#define ECE_TXLOCK_INIT(_sc) \
> + mtx_init(&_sc->sc_mtx_tx, device_get_nameunit(_sc->dev), \
> + "ECE TX Lock", MTX_DEF)
> +
> +#define ECE_CLEANUPLOCK(_sc) mtx_lock(&(_sc)->sc_mtx_cleanup)
> +#define ECE_CLEANUPUNLOCK(_sc) mtx_unlock(&(_sc)->sc_mtx_cleanup)
> +#define ECE_CLEANUPLOCK_INIT(_sc) \
> + mtx_init(&_sc->sc_mtx_cleanup, device_get_nameunit(_sc->dev), \
> + "ECE cleanup Lock", MTX_DEF)
> +
> +#define ECE_RXLOCK(_sc) mtx_lock(&(_sc)->sc_mtx_rx)
> +#define ECE_RXUNLOCK(_sc) mtx_unlock(&(_sc)->sc_mtx_rx)
> +#define ECE_RXLOCK_INIT(_sc) \
> + mtx_init(&_sc->sc_mtx_rx, device_get_nameunit(_sc->dev), \
> + "ECE RX Lock", MTX_DEF)
> +
> +#define ECE_LOCK_DESTROY(_sc) mtx_destroy(&_sc->sc_mtx);
> +#define ECE_TXLOCK_DESTROY(_sc) mtx_destroy(&_sc->sc_mtx_tx);
> +#define ECE_RXLOCK_DESTROY(_sc) mtx_destroy(&_sc->sc_mtx_rx);
> +#define ECE_CLEANUPLOCK_DESTROY(_sc) \
> + mtx_destroy(&_sc->sc_mtx_cleanup);
> +
> +#define ECE_ASSERT_LOCKED(_sc) mtx_assert(&_sc->sc_mtx, MA_OWNED);
> +#define ECE_ASSERT_UNLOCKED(_sc) mtx_assert(&_sc->sc_mtx, MA_NOTOWNED);
> +
> +static devclass_t ece_devclass;
> +
> +/* ifnet entry points */
> +
> +static void eceinit_locked(void *);
> +static void ecestart_locked(struct ifnet *);
> +
> +static void eceinit(void *);
> +static void ecestart(struct ifnet *);
> +static void ecestop(struct ece_softc *);
> +static int eceioctl(struct ifnet * ifp, u_long, caddr_t);
^^^
Use TAB after the return value type in prototypes.
> +
> +/* bus entry points */
> +
> +static int ece_probe(device_t dev);
> +static int ece_attach(device_t dev);
> +static int ece_detach(device_t dev);
> +static void ece_intr(void *);
> +static void ece_intr_qf(void *);
> +static void ece_intr_status(void *xsc);
> +
> +/* helper routines */
> +static int ece_activate(device_t dev);
> +static void ece_deactivate(device_t dev);
> +static int ece_ifmedia_upd(struct ifnet *ifp);
> +static void ece_ifmedia_sts(struct ifnet *ifp, struct ifmediareq *ifmr);
> +static int ece_get_mac(struct ece_softc *sc, u_char *eaddr);
> +static void ece_set_mac(struct ece_softc *sc, u_char *eaddr);
> +static int configure_cpu_port(struct ece_softc *sc);
> +static int configure_lan_port(struct ece_softc *sc, int phy_type);
> +static void set_pvid(struct ece_softc *sc, int port0, int port1, int cpu);
> +static void set_vlan_vid(struct ece_softc *sc, int vlan);
> +static void set_vlan_member(struct ece_softc *sc, int vlan);
> +static void set_vlan_tag(struct ece_softc *sc, int vlan);
> +static int hardware_init(struct ece_softc *sc);
> +static void ece_intr_rx_locked(struct ece_softc *sc, int count);
> +
> +static void ece_free_desc_dma_tx(struct ece_softc *sc);
> +static void ece_free_desc_dma_rx(struct ece_softc *sc);
> +
> +static void ece_intr_task(void *arg, int pending __unused);
> +static void ece_tx_task(void *arg, int pending __unused);
> +static void ece_cleanup_task(void *arg, int pending __unused);
> +
> +static int ece_allocate_dma(struct ece_softc *sc);
> +
> +static void ece_intr_tx(void *xsc);
> +
> +static void clear_mac_entries(struct ece_softc *ec, int include_this_mac);
> +
> +static uint32_t read_mac_entry(struct ece_softc *ec,
> + uint8_t *mac_result,
> + int first);
> +
Use TAB after the return value type. Also, the second level alignment is
always 4 spaces.
> +/*PHY related functions*/
> +
Extra blank line.
> +static inline int
> +phy_read(struct ece_softc *sc, int phy, int reg)
> +{
> + int val;
> + int ii;
> + int status;
> +
> + WR4(sc, PHY_CONTROL, PHY_RW_OK);
> + WR4(sc, PHY_CONTROL,
> + (PHY_ADDRESS(phy)|PHY_READ_COMMAND |
> + PHY_REGISTER(reg)));
> +
> + for (ii = 0; ii < 0x1000; ii++) {
> + status = RD4(sc, PHY_CONTROL);
> + if (status & PHY_RW_OK) {
> + /* clear the rw_ok status, and clear other
> + * bits value */
> + WR4(sc, PHY_CONTROL, PHY_RW_OK);
> + val = PHY_GET_DATA(status);
> + return (val);
> + }
> + }
> + return (0);
> +}
> +
> +static inline void
> +phy_write(struct ece_softc *sc, int phy, int reg, int data)
> +{
> + int ii;
> +
> + WR4(sc, PHY_CONTROL, PHY_RW_OK);
> + WR4(sc, PHY_CONTROL,
> + PHY_ADDRESS(phy) | PHY_REGISTER(reg) |
> + PHY_WRITE_COMMAND | PHY_DATA(data));
> + for (ii = 0; ii < 0x1000; ii++) {
> + if (RD4(sc, PHY_CONTROL) & PHY_RW_OK) {
> + /* clear the rw_ok status, and clear other
> + * bits value
> + */
> + WR4(sc, PHY_CONTROL, PHY_RW_OK);
> + return;
> + }
> + }
> +}
> +
> +/*currently only ic_plus phy is supported*/
Missing spaced after the start comment marker, and before the end marker.
Also as a generic note, most of our comments are sentences and thus should
usually start with a capital and end with a period.
> +static int get_phy_type(struct ece_softc *sc)
> +{
> + uint16_t phy0_id = 0, phy1_id = 0;
> +
> + /*
> + * Use SMI (MDC/MDIO) to read Link Partner's PHY Identifier
> + * Register 1
> + */
> + phy0_id = phy_read(sc, 0, 0x2);
> + phy1_id = phy_read(sc, 1, 0x2);
> +
> + if ((phy0_id == 0xFFFF) && (phy1_id == 0x000F))
> + return (ASIX_GIGA_PHY);
> + else if ((phy0_id == 0x0243) && (phy1_id == 0x0243))
> + return (TWO_SINGLE_PHY);
> + else if ((phy0_id == 0xFFFF) && (phy1_id == 0x0007))
> + return (VSC8601_GIGA_PHY);
> + else if ((phy1_id == 0xFFFF) && (phy0_id == 0x0243))
^^^^^^^ ^^^^^^^
I think it is better to use the same order of variables as in the
previous expressions --- that makes the code more readable.
> + return (IC_PLUS_PHY);
> +
> + return (NOT_FOUND_PHY);
> +}
> +
> +static int
> +ece_probe(device_t dev)
> +{
style(9) requires extra blank line here.
> + device_set_desc(dev, "Econa Ethernet Controller");
> + return (0);
> +}
> +
> +
> +static int
> +ece_attach(device_t dev)
> +{
> + struct ece_softc *sc = device_get_softc(dev);
> + struct ifnet *ifp = NULL;
> + struct sysctl_ctx_list *sctx;
> + struct sysctl_oid *soid;
> + int err = 0;
> + u_char eaddr[ETHER_ADDR_LEN];
> + uint32_t rnd;
> + int i, rid;
Variables here should be sorted by size, than by name. style(9) also
discourages initialization in declarations.
> +
> + sc->dev = dev;
> +
> + rid = 0;
> + sc->mem_res = bus_alloc_resource_any(dev, SYS_RES_MEMORY, &rid,
> + RF_ACTIVE);
> +
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Extra blank line here.
> + if (sc->mem_res == NULL)
> + goto out;
> +
> + power_on_network_interface();
> +
> + rid = 0;
> + sc->irq_res_status = bus_alloc_resource_any(dev, SYS_RES_IRQ, &rid,
> + RF_ACTIVE);
^^^^^^^^^^^^^
All second-level alignments are 4 spaces.
> + if (sc->irq_res_status == NULL)
> + goto out;
> +
> + rid = 1; /*TSTC: Fm-Switch-Tx-Complete*/
^^^ ^^^
Missing spaces.
> + sc->irq_res_tx = bus_alloc_resource_any(dev, SYS_RES_IRQ, &rid,
> + RF_ACTIVE);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Alignment is wrong.
> + if (sc->irq_res_tx == NULL)
> + goto out;
> +
> + rid = 2; /*FSRC: Fm-Switch-Rx-Complete*/
> + sc->irq_res_rec = bus_alloc_resource_any(dev, SYS_RES_IRQ, &rid,
> + RF_ACTIVE);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Same here.
> + if (sc->irq_res_rec == NULL)
> + goto out;
> +
> + rid = 4; /*FSQF: Fm-Switch-Queue-Full*/
^^^ ^^^
Missing spaces.
> + sc->irq_res_qf = bus_alloc_resource_any(dev, SYS_RES_IRQ, &rid,
> + RF_ACTIVE);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Alignment.
> + if (sc->irq_res_qf == NULL)
> + goto out;
> +
> + err = ece_activate(dev);
> + if (err)
> + goto out;
> +
> + /* Sysctls */
> + sctx = device_get_sysctl_ctx(dev);
> + soid = device_get_sysctl_tree(dev);
> +
> + ECE_LOCK_INIT(sc);
> +
> + callout_init_mtx(&sc->tick_ch, &sc->sc_mtx, 0);
> +
> + if ((err = ece_get_mac(sc, eaddr)) != 0) {
There should be a blank line before the block comment. Or use one-line
comment instead.
> + /*
> + * No MAC address configured. Generate the random one.
> + */
> + if (bootverbose)
> + device_printf(dev,
> + "Generating random ethernet address.\n");
> + rnd = arc4random();
> +
> + /*from if_ae.c/if_ate.c*/
> + /*
> + * Set OUI to convenient locally assigned address. 'b'
> + * is 0x62, which has the locally assigned bit set, and
> + * the broadcast/multicast bit clear.
> + */
> + eaddr[0] = 'b';
> + eaddr[1] = 's';
> + eaddr[2] = 'd';
> + eaddr[3] = (rnd >> 16) & 0xff;
> + eaddr[4] = (rnd >> 8) & 0xff;
> + eaddr[5] = rnd & 0xff;
> +
> + for (i=0; i<ETHER_ADDR_LEN; i++)
^^^
Missing spaces.
> + eaddr[i] = vlan0_mac[i];
> + }
> + ece_set_mac(sc, eaddr);
> + sc->ifp = ifp = if_alloc(IFT_ETHER);
> + if (mii_phy_probe(dev, &sc->miibus, ece_ifmedia_upd,
> + ece_ifmedia_sts)) {
^^^^^^^^^^^^^^^^^^^
Alignment should be 4 spces.
> + device_printf(dev, "Cannot find my PHY.\n");
> + err = ENXIO;
> + goto out;
> + }
> + ifp->if_softc = sc;
> + if_initname(ifp, device_get_name(dev), device_get_unit(dev));
> + ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
> +
> + ifp->if_capabilities = IFCAP_HWCSUM;
> +
> + ifp->if_hwassist = (CSUM_IP | CSUM_TCP | CSUM_UDP);
> + ifp->if_capenable = ifp->if_capabilities;
> + ifp->if_start = ecestart;
> + ifp->if_ioctl = eceioctl;
> + ifp->if_init = eceinit;
> + ifp->if_snd.ifq_drv_maxlen = ECE_MAX_TX_BUFFERS-1;
^^^^
Missing spaces.
> + IFQ_SET_MAXLEN(&ifp->if_snd, ECE_MAX_TX_BUFFERS-1);
^^^
Ditto.
> + IFQ_SET_READY(&ifp->if_snd);
> +
> + /* Create local taskq. */
> +
Extra white line inserted.
> + TASK_INIT(&sc->sc_intr_task, 0, ece_intr_task, sc);
> + TASK_INIT(&sc->sc_tx_task, 1, ece_tx_task, ifp);
> + TASK_INIT(&sc->sc_cleanup_task, 2, ece_cleanup_task, sc);
> + sc->sc_tq = taskqueue_create_fast("ece_taskq", M_WAITOK,
> + taskqueue_thread_enqueue,
> + &sc->sc_tq);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Wrong alignment. I will not comment on this further.
> + if (sc->sc_tq == NULL) {
> + device_printf(sc->dev, "could not create taskqueue\n");
> + goto out;
> +
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Extra white space inserted.
> + }
> +
> + ether_ifattach(ifp, eaddr);
> +
> + /*
> + * Activate interrupts
> + */
> + err = bus_setup_intr(dev, sc->irq_res_rec, INTR_TYPE_NET | INTR_MPSAFE,
> + NULL, ece_intr, sc, &sc->intrhand);
> + if (err) {
> + ether_ifdetach(ifp);
> + ECE_LOCK_DESTROY(sc);
> + goto out;
> + }
> +
> + err = bus_setup_intr(dev, sc->irq_res_status,
> + INTR_TYPE_NET | INTR_MPSAFE,
> + NULL, ece_intr_status, sc, &sc->intrhand_status);
> + if (err) {
> + ether_ifdetach(ifp);
> + ECE_LOCK_DESTROY(sc);
> + goto out;
> + }
> +
> + err = bus_setup_intr(dev, sc->irq_res_qf, INTR_TYPE_NET | INTR_MPSAFE,
> + NULL,ece_intr_qf, sc, &sc->intrhand_qf);
> +
> + if (err) {
> + ether_ifdetach(ifp);
> + ECE_LOCK_DESTROY(sc);
> + goto out;
> + }
> +
> + err = bus_setup_intr(dev, sc->irq_res_tx, INTR_TYPE_NET | INTR_MPSAFE,
> + NULL, ece_intr_tx, sc, &sc->intrhand_tx);
> +
> + if (err) {
> + ether_ifdetach(ifp);
> + ECE_LOCK_DESTROY(sc);
> + goto out;
> + }
> +
> + ECE_TXLOCK_INIT(sc);
> + ECE_RXLOCK_INIT(sc);
> + ECE_CLEANUPLOCK_INIT(sc);
> +
> + /*enable all interrupt sources*/
> + WR4(sc, INTERRUPT_MASK, 0x00000000);
> +
> + /*enable port 0*/
> + WR4(sc, PORT_0_CONFIG, RD4(sc, PORT_0_CONFIG) & ~(PORT_DISABLE));
> +
> + taskqueue_start_threads(&sc->sc_tq, 1, PI_NET, "%s taskq",
> + device_get_nameunit(sc->dev));
> +
> +out:;
> + if (err)
> + ece_deactivate(dev);
> + if (err && ifp)
> + if_free(ifp);
> + return (err);
> +}
> +
> +static int
> +ece_detach(device_t dev)
> +{
> + /*TODO: release resources*/
> +
> + struct ece_softc *sc = device_get_softc(dev);
> + struct ifnet *ifp = sc->ifp;
> +
> + ecestop(sc);
> + if (ifp != NULL) {
> + ether_ifdetach(ifp);
> + if_free(ifp);
> + }
> + ece_deactivate(dev);
> + return (0);
> +}
> +
> +static void
> +ece_getaddr(void *arg, bus_dma_segment_t *segs, int nsegs, int error)
> +{
> + u_int32_t *paddr;
> + KASSERT(nsegs == 1, ("wrong number of segments, should be 1"));
> + paddr = arg;
> + *paddr = segs->ds_addr;
> +}
> +
> +static int
> +ece_alloc_desc_dma_tx(struct ece_softc *sc)
> +{
> + int i;
> + int error;
> +
> + /* Allocate a busdma tag and DMA safe memory for TX/RX descriptors. */
> + error = bus_dma_tag_create(sc->sc_parent_tag, /* parent */
> + 16, 0, /* alignment, boundary */
> + BUS_SPACE_MAXADDR_32BIT, /* lowaddr */
> + BUS_SPACE_MAXADDR, /* highaddr */
> + NULL, NULL, /* filtfunc, filtfuncarg */
> + sizeof(eth_tx_desc_t)*ECE_MAX_TX_BUFFERS, /*max size*/
> + 1,/*nsegments*/
^^^
Missing spaces. The same for other parts of code.
Can you, please, review your code considering these
comments and send the patch again? In gerneal, the
code looks great, but there're some minor nits that
should be fixed before it hits the base.
Thanks!
--
Stanislav Sedov
ST4096-RIPE
More information about the freebsd-arm
mailing list