On Thu, Feb 23, 2017 at 01:03:57PM +1100, Jonathan Gray wrote:
> On Wed, Feb 22, 2017 at 06:45:52PM -0500, Dale Rahn wrote:
> > Add psci 2.0 features to spin up/down/suspend processors.
> >
> > This change uses extern weak symbols to determine if the platform supports
> > the cpu_on/cpu_off/cpu_suspend features. If available, it saves the fdt
> > provided argument values.
> >
> > Some PSCI calls will return an int status, so the callfn has been updated
> > to return that status (eg cpu_on fail)
> >
> > diff --git a/sys/dev/fdt/psci.c b/sys/dev/fdt/psci.c
> > index fceafd0e9ba..3177ff06beb 100644
> > --- a/sys/dev/fdt/psci.c
> > +++ b/sys/dev/fdt/psci.c
> > @@ -29,12 +29,19 @@
> > extern void (*cpuresetfn)(void);
> > extern void (*powerdownfn)(void);
> >
> > +extern int (*cpu_suspend_fn)(void) __attribute__((weak)) ;
> > +extern int (*cpu_off_fn)(void) __attribute__((weak)) ;
> > +extern int (*cpu_on_fn)(uint64_t, uint64_t) __attribute__((weak)) ;
> > +
> > #define SYSTEM_OFF 0x84000008
> > #define SYSTEM_RESET 0x84000009
> >
> > struct psci_softc {
> > struct device sc_dev;
> > - void (*callfn)(uint32_t, uint32_t, uint32_t,
> > uint32_t);
> > + int (*callfn)(uint32_t, uint32_t, uint32_t,
> > uint32_t);
> > + int sc_cpu_on;
> > + int sc_cpu_off;
> > + int sc_cpu_suspend;
> > };
>
> spacing is out here
>
sure.
> >
> > struct psci_softc *psci_sc;
> > @@ -43,9 +50,12 @@ int psci_match(struct device *, void *, void *);
> > void psci_attach(struct device *, struct device *, void *);
> > void psci_reset(void);
> > void psci_powerdown(void);
> > +int psci_cpu_suspend(void);
> > +int psci_cpu_off(void);
> > +int psci_cpu_on(uint64_t, uint64_t);
> >
> > -extern void hvc_call(uint32_t, uint32_t, uint32_t, uint32_t);
> > -extern void smc_call(uint32_t, uint32_t, uint32_t, uint32_t);
> > +extern int hvc_call(uint32_t, uint32_t, uint32_t, uint32_t);
> > +extern int smc_call(uint32_t, uint32_t, uint32_t, uint32_t);
> >
> > struct cfattach psci_ca = {
> > sizeof(struct psci_softc), psci_match, psci_attach
> > @@ -83,6 +93,24 @@ psci_attach(struct device *parent, struct device *self,
> > void *aux)
> > psci_sc = sc;
> > cpuresetfn = psci_reset;
> > powerdownfn = psci_powerdown;
> > +
> > + if (&cpu_suspend_fn != NULL) {
> > + sc->sc_cpu_suspend = OF_getpropint(faa->fa_node, "cpu_suspend",
> > 0);
> > + if (sc->sc_cpu_suspend != 0)
> > + cpu_suspend_fn = psci_cpu_suspend;
> > + }
> > +
> > + if (&cpu_on_fn != NULL) {
> > + sc->sc_cpu_on = OF_getpropint(faa->fa_node, "cpu_on", 0);
> > + if (sc->sc_cpu_on != 0)
> > + cpu_on_fn = psci_cpu_on;
> > + }
> > +
> > + if (&cpu_off_fn != NULL) {
> > + sc->sc_cpu_off = OF_getpropint(faa->fa_node, "cpu_off", 0);
> > + if (sc->sc_cpu_off != 0)
> > + cpu_off_fn = psci_cpu_off;
> > + }
> > }
>
> Can these nodes be relied on as being present?
>
> Outside of qemu -M virt
>
> psci {
> migrate = <0x84000005>;
> cpu_on = <0x84000003>;
> cpu_off = <0x84000002>;
> cpu_suspend = <0x84000001>;
> method = "hvc";
> compatible = "arm,psci-0.2", "arm,psci";
> };
>
> I can only find
>
> arm/boot/dts/artpec6.dtsi: cpu_on = <0x84000003>;
> arm/boot/dts/ecx-common.dtsi: cpu_on = <0x84000006>;
> arm/boot/dts/keystone.dtsi: cpu_on = <0x84000003>;
> arm/boot/dts/xenvm-4.2.dts: cpu_on = <2>;
> arm64/boot/dts/al/alpine-v2.dtsi: cpu_on = <0x84000003>;
> arm64/boot/dts/exynos/exynos5433.dtsi: cpu_on = <0xC4000003>;
> arm64/boot/dts/lg/lg1312.dtsi: cpu_on = <0x84000003>;
> arm64/boot/dts/lg/lg1313.dtsi: cpu_on = <0x84000003>;
> arm64/boot/dts/mediatek/mt8173.dtsi: cpu_on = <0x84000003>;
> arm64/boot/dts/sprd/sc9836.dtsi: cpu_on =
> <0xc4000003>;
>
> 0x84000003 is SMC32, 0xC4000003 is SMC64 and there is actually
> a different calling convention for these...
>
> The existing SYSTEM_OFF and SYSTEM_RESET uses are fine as they are just
> a 32 bit function id with no arguments. But these new functions
> need to care about the arguments.
>
> There is also PSCI_FEATURES in PSCI 1.0 that tests if function ids are
> supported.
>
No, these will not always be present, eg on the amd arm64 machines,
it only implements PSCI 1.0 and uses a different method for cpu spinup.
This was set up to only enable the callback if the property is present,
however you have a point that it might be wise to check that the callback
value matches the expected 64 bit callback value (0xc4000000 | callidx)
It could make sense to only look for these if compatible indicates 2.0,
but if it were only 1.0, why would it have the attribute?
>
> > void
> > @@ -100,3 +128,31 @@ psci_powerdown(void)
> > if (sc->callfn)
> > (*sc->callfn)(SYSTEM_OFF, 0, 0, 0);
> > }
>
> Shouldn't all of these have a sc->sc_cpu_suspend != 0 type test as well
> to avoid calls with a function id of 0?
>
> You could skip the earlier != 0 tests and do them in these functions.
If the property doesn't exist, the callback will not be registered.
psci_cpu_on() et al should not be called directly , but only thru the callback.
However, in the interest in preventing problems, either making the functions
static
or adding the check could make sense.
> + if (sc->sc_cpu_off != 0)
> + cpu_off_fn = psci_cpu_off;
>
> > +
> > +int
> > +psci_cpu_suspend()
> > +{
> > + struct psci_softc *sc = psci_sc;
> > + if (sc->callfn)
> > + return (*sc->callfn)(sc->sc_cpu_suspend, 0, 0, 0);
> > + return -1;
> > +}
> > +
> > +int
> > +psci_cpu_off()
> > +{
> > + struct psci_softc *sc = psci_sc;
> > + if (sc->callfn)
> > + return (*sc->callfn)(sc->sc_cpu_off, 0, 0, 0);
> > + return -1;
> > +
> > +}
> > +
> > +int
> > +psci_cpu_on(uint64_t mpidr, uint64_t pc)
> > +{
> > + struct psci_softc *sc = psci_sc;
> > + if (sc->callfn)
> > + return (*sc->callfn)(sc->sc_cpu_on, mpidr, pc, 0);
> > + return -1;
> > +}
> >
> > Dale Rahn dr...@dalerahn.com
> >
>
Dale Rahn dr...@dalerahn.com
