Page MenuHomeFreeBSD
Differential D2377 Diff 5366 sys/arm64/arm64/gic_v3_fdt.c

Changeset View

Standalone View

View Options

sys/arm64/arm64/gic_v3_fdt.c

  • This file was added.
/*-
* Copyright (c) 2015 The FreeBSD Foundation
* All rights reserved.
*
* This software was developed by Semihalf under
* the sponsorship of the FreeBSD Foundation.
*
* 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/bus.h>
#include <sys/kernel.h>
#include <sys/module.h>
#include <dev/fdt/fdt_common.h>
#include <dev/ofw/openfirm.h>
#include <dev/ofw/ofw_bus.h>
#include <dev/ofw/ofw_bus_subr.h>
#include "pic_if.h"
#include "gic_v3_reg.h"
andrewUnsubmitted
Not Done
Inline Actions

I suspect some of these aren't needed.

andrew: I suspect some of these aren't needed.
#include "gic_v3_var.h"
/*
* FDT glue.
*/
static int gic_v3_fdt_probe(device_t);
static int gic_v3_fdt_attach(device_t);
static device_method_t gic_v3_fdt_methods[] = {
/* Device interface */
DEVMETHOD(device_probe, gic_v3_fdt_probe),
DEVMETHOD(device_attach, gic_v3_fdt_attach),
/* End */
DEVMETHOD_END
};
DEFINE_CLASS_1(gic_v3, gic_v3_fdt_driver, gic_v3_fdt_methods,
sizeof(struct gic_v3_softc), gic_v3_driver);
static devclass_t gic_v3_fdt_devclass;
EARLY_DRIVER_MODULE(gic_v3, simplebus, gic_v3_fdt_driver, gic_v3_fdt_devclass,
0, 0, BUS_PASS_INTERRUPT + BUS_PASS_ORDER_MIDDLE);
EARLY_DRIVER_MODULE(gic_v3, ofwbus, gic_v3_fdt_driver, gic_v3_fdt_devclass,
0, 0, BUS_PASS_INTERRUPT + BUS_PASS_ORDER_MIDDLE);
/*
* Device interface.
*/
static int
gic_v3_fdt_probe(device_t dev)
{
if (!ofw_bus_status_okay(dev))
return (ENXIO);
andrewUnsubmitted
Done
Inline Actions

You should create a parent bus for these, then add them there.

andrew: You should create a parent bus for these, then add them there.
zbbAuthorUnsubmitted
Done
Inline Actions

I don't get it. What do you mean "parent bus for these"? When you did your first reviews on GitHub you asked for separate FDT-specific attachment for GICv3. Now we have some need for separate bus for GICv3? Can you point me some other example of such bus among other PIC controllers in FreeBSD?

zbb: I don't get it. What do you mean "parent bus for these"? When you did your first reviews on…
andrewUnsubmitted
Done
Inline Actions

I meant parent class. The common parts of this should be in a parent class, with the bus specific code in either the fdt, or acpi when we add this. I've done this with the gic(v2) driver in D2463.

andrew: I meant parent class. The common parts of this should be in a parent class, with the bus…
if (!ofw_bus_is_compatible(dev, "arm,gic-v3"))
return (ENXIO);
andrewUnsubmitted
Not Done
Inline Actions

These don't need to be == 0 as they are boolean (even if they return an int)

andrew: These don't need to be `== 0` as they are boolean (even if they return an int)
device_set_desc(dev, GIC_V3_DEVSTR);
return (BUS_PROBE_DEFAULT);
}
static int
gic_v3_fdt_attach(device_t dev)
{
struct gic_v3_softc *sc;
pcell_t redist_regions;
int err;
sc = device_get_softc(dev);
andrewUnsubmitted
Done
Inline Actions

We should use a common gicv3 base class, then this subclass will only be the fdt specific bits.

andrew: We should use a common gicv3 base class, then this subclass will only be the fdt specific bits.
sc->dev = dev;
/*
* Recover number of the Re-Distributor regions.
*/
if (OF_getencprop(ofw_bus_get_node(dev), "#redistributor-regions",
&redist_regions, sizeof(redist_regions)) <= 0)
sc->gic_redists.nregions = 1;
else
sc->gic_redists.nregions = redist_regions;
err = gic_v3_attach(dev);
if (err)
goto error;
return (err);
error:
if (bootverbose) {
device_printf(dev,
andrewUnsubmitted
Not Done
Inline Actions

bootverbose is a boolean

andrew: bootverbose is a boolean
"Failed to attach. Error %d\n", err);
}
/* Failure so free resources */
gic_v3_detach(dev);
return (err);
}
andrewUnsubmitted
Done
Inline Actions

You don't need this. The parent should set the detach functions to gic_v3_detach, then don't set one in the child. The child should only define a function if it needs to do something extra.

andrew: You don't need this. The parent should set the detach functions to `gic_v3_detach`, then don't…
zbbAuthorUnsubmitted
Done
Inline Actions

I don't understand what is your intention here. Can you point me to the example in the other PIC driver?

zbb: I don't understand what is your intention here. Can you point me to the example in the other…
andrewUnsubmitted
Done
Inline Actions

err is not a bool, it should be if (err != 0), or change it to

if (err != 0) {
  /* The failure case below */
  if (bootverbose) {
    ...
  }
  gic_v3_fdt_detach(dev);
}

return (err);
andrew: err is not a bool, it should be `if (err != 0)`, or change it to if (err != 0) { /* The…