re: UltraSPARC III/IIe cpufreq drivers, please test

[matthew green <mrg%eterna.com.au@localhost>]

i'm going to point out various minor style nits below, as well as
a few questions, and a few problems.

if you have a chance, can you compare the original psycho with these,
to see if at least they're compatible enough for the same functions/
bits to be used, or if these functions should be called eg,
psycho_usIIe_set_cpufreq().

thanks for working on this!


.mrg.


> +void
> +psycho_get_cpufreq(void *aux, void *freq)
> +{
[ ... ]
> +	switch(esctrl & (~0x111UL))

keywords should always have space after then, so 'switch ('.

> +	{
> +		default:

these should be indented a level back.

> +		case ESTAR_FULL:
> +			f = sc->sc_cf.cf_state[0].cfs_freq;
> +			break;
> +
> +		case ESTAR_DIV_2:
> +			f = sc->sc_cf.cf_state[1].cfs_freq;
> +			break;
> +
> +		case ESTAR_DIV_6:
> +			f = sc->sc_cf.cf_state[2].cfs_freq;
> +			break;
> +	}
> +
> +	(*((uint32_t *) freq)) = f;

yuck.  feels like we should make this value a uint64_t *.
(but this a cpufreq(9) level issue.)

> +	aprint_normal("psycho_get_cpufreq %u MHz (cpu %lu)\n", f,
> +			(unsigned long) cpu_number());
> +}
> +
> +static void
> +psycho_setup_cpufreq(device_t dev)
> +{
> +	int	ret;
> +	struct psycho_softc *sc = device_private(dev);
> +
> +	cpu_lookup(device_unit(dev))->sc = sc;

what is this trying to do?  it is clearly "cf_unit" abuse, and 
can't be commited as-is.  infact, this is really wrong.  it's
worse for the schizo below....

> +	aprint_normal("register UltraSPARC-IIe cpufreq driver\n");
> +	ret = cpufreq_register(&(sc->sc_cf));

you can write this as (&sc->sc_cf).

> +
> +	if (ret != 0)
> +		aprint_error_dev(sc->sc_dev, "failed to register cpufreq\n");
> +}
> +
> +
>  /*
>   * SUNW,psycho initialisation ..
>   *	- find the per-psycho registers
> @@ -690,6 +788,27 @@
>  	pba.pba_pc = pp->pp_pc;
>  
>  	config_found_ia(self, "pcibus", &pba, psycho_print);
> +
> +	if (strstr(cpu_getmodel(), "SUNW,UltraSPARC-IIe") != NULL)
> +	{

perhaps move the guts of all this into psycho_setup_cpufreq() itself?

> +		(void) memset(&(sc->sc_cf), 0, sizeof(struct cpufreq));
> +
> +		sc->sc_cf.cf_state[0].cfs_freq =
> +				(uint32_t) (curcpu()->ci_cpu_clockrate[1]);
> +		sc->sc_cf.cf_state[1].cfs_freq = sc->sc_cf.cf_state[0].cfs_freq / 2;
> +		sc->sc_cf.cf_state[2].cfs_freq = sc->sc_cf.cf_state[0].cfs_freq / 6;
> +		sc->sc_cf.cf_state_count = 3;
> +
> +		sc->sc_cf.cf_mp = false;
> +		sc->sc_cf.cf_cookie = NULL;
> +		sc->sc_cf.cf_get_freq = psycho_get_cpufreq;
> +		sc->sc_cf.cf_set_freq = psycho_set_cpufreq;
> +
> +		(void) strlcpy(sc->sc_cf.cf_name, "us2e cpufreq",
> +				sizeof(sc->sc_cf.cf_name));

everything down to this point ^^^.

> +		config_interrupts(sc->sc_dev, psycho_setup_cpufreq);
> +	}
>  }
>  
[ ... ]
> --- dev/psychovar.h	7 Dec 2013 11:17:24 -0000	1.21
> +++ dev/psychovar.h	13 Sep 2014 10:41:21 -0000
> @@ -125,6 +125,7 @@
>  	struct sysmon_pswitch		*sc_smcontext;	/* power switch definition */
>  	int				sc_powerpressed;/* already signaled */
>  	uint64_t			sc_last_stick;
> +	struct cpufreq sc_cf;

please align "sc_cf" with the other variables.

>  };
>  
[ ... ]
>  void
> +schizo_set_cpufreq(void *aux, void *freq)
> +{
> +	uint32_t	f;
> +
> +	uint64_t	cmd;
> +	uint64_t	esctrl;
> +
> +	struct schizo_softc *sc;
> +
> +	f = (*((uint32_t *) freq));
> +	sc = curcpu()->sc;
> +
> +	if (sc == NULL)
> +		return;

this seems like a problems.  what we really want to happen here
is for every actual schizo (which i think means, say, every time
we have a PCI-A leaf), we need to find the matching CPU for this
schizo and hook them together.

ie, given a normal booted system that has no special cf_units:

schizo0 -> cpu0
schizo1 -> nothing
schizo2 -> cpu1
schizo3 -> nothing

> +
> +	if (f == sc->sc_cf.cf_state[0].cfs_freq)
> +		cmd = SCZ_ESCTRL_CLK_MAX;
> +	else if (f == sc->sc_cf.cf_state[1].cfs_freq)
> +		cmd = SCZ_ESCTRL_CLK_DIV_2;
> +	else if (f == sc->sc_cf.cf_state[2].cfs_freq)
> +		cmd = SCZ_ESCTRL_CLK_DIV_32;
> +	else
> +	{
> +		aprint_error("cpufreq failed: switching to max frequency\n");
> +		cmd = SCZ_ESCTRL_CLK_MAX;
> +	}
> +
> +	esctrl = schizo_read(sc, SCZ_ESCTRL);
> +	esctrl &= (~SCZ_ESCTRL_CLK_MASK);
> +	esctrl |= cmd;
> +	schizo_write(sc, SCZ_ESCTRL, esctrl);
> +
> +	aprint_normal("schizo_set_cpufreq %u MHz (cpu %lu)\n", f,
> +			(unsigned long) cpu_number());
> +}
> +
> +void
> +schizo_get_cpufreq(void *aux, void *freq)
> +{
> +	uint32_t	f;
> +	uint64_t	esctrl;
> +
> +	struct schizo_softc *sc;
> +
> +	sc = curcpu()->sc;
> +
> +	if (sc == NULL)
> +		return;
> +
> +	esctrl = schizo_read(sc, SCZ_ESCTRL);
> +

same switch/indent comments here:

> +	switch(esctrl & (~SCZ_ESCTRL_CLK_MASK))
> +	{
> +		default:
> +		case SCZ_ESCTRL_CLK_MAX:
> +			f = sc->sc_cf.cf_state[0].cfs_freq;
> +			break;
> +
> +		case SCZ_ESCTRL_CLK_DIV_2:
> +			f = sc->sc_cf.cf_state[1].cfs_freq;
> +			break;
> +
> +		case SCZ_ESCTRL_CLK_DIV_32:
> +			f = sc->sc_cf.cf_state[2].cfs_freq;
> +			break;
> +	}
> +
> +	(*((uint32_t *) freq)) = f;
> +	aprint_normal("schizo_get_cpufreq %u MHz (cpu %lu)\n", f,
> +			(unsigned long) cpu_number());
> +}
> +
> +static void
> +schizo_setup_cpufreq(device_t dev)
> +{
> +	// schizo_setup_cpufreq() is calling once by each schizo_attach().
> +
> +	struct schizo_softc *sc = device_private(dev);
> +	int	ret;
> +
> +aprint_normal("device_unit(dev)=%d\n", device_unit(dev));	
> +	cpu_lookup(device_unit(dev))->sc = sc;

OK, here we're really not in a good place.  besides cf_unit abuse,
we're now possibly going to use cpu_lookup() on an invalid unit
number, because my system has 2 cpus but 4 schizo's attached.

but you can't use cf_unit to find which is which, because it is
perfectly legal to do something like this in your config:

no cpu* at mainbus0
cpu0 at mainbus0
cpu99 at mainbus0

and then you'll configure cpu's with cf_unit of 0 and 99, and
nothing in between.  there's also no reason i can't do the
exact same with the default schizo0..3 to whatever.  any sort
of lookup or matching needs to be done some other way.

i don't see anything about what to do in the case there are
two actual schizo chips in the system, which appears to be the
case for at least the sunblade 2500, which configures 4 schizo
each with a PCI-A and PCI-B leaf.  but i'm not 100% sure about
this... have you attempted looking at illumos sources?

> +
> +	if (device_unit(dev) == 0)

again, cf_unit abuse.  this should probably be a RUN_ONCE().

> +	{
> +		aprint_normal("register safari cpufreq driver\n");
> +		ret = cpufreq_register(&(sc->sc_cf));
> +
> +		if (ret != 0)
> +			aprint_error_dev(sc->sc_dev, "failed to register cpufreq\n");
> +	}
> +}
> +
> +void
>  schizo_attach(device_t parent, device_t self, void *aux)
>  {
>  	struct schizo_softc *sc = device_private(self);
> @@ -318,6 +418,26 @@
>  	schizo_set_intr(sc, pbm, PIL_HIGH, schizo_safari_error, sc,
>  	    SCZ_SERR_INO, "safari");
>  

again, it seems best to move the blow chunk into schizo_setup_cpufreq().

> +	/*
> +	 * Register with cpufreq(9)
> +	 */
> +
> +	(void) memset(&(sc->sc_cf), 0, sizeof(struct cpufreq));
> +
> +	sc->sc_cf.cf_state[0].cfs_freq = (uint32_t) (curcpu()->ci_cpu_clockrate[1]);
> +	sc->sc_cf.cf_state[1].cfs_freq = sc->sc_cf.cf_state[0].cfs_freq / 2;
> +	sc->sc_cf.cf_state[2].cfs_freq = sc->sc_cf.cf_state[0].cfs_freq / 32;
> +	sc->sc_cf.cf_state_count = 3;
> +
> +	sc->sc_cf.cf_mp = true;
> +	sc->sc_cf.cf_cookie = NULL;
> +	sc->sc_cf.cf_get_freq = schizo_get_cpufreq;
> +	sc->sc_cf.cf_set_freq = schizo_set_cpufreq;
> +
> +	(void) strlcpy(sc->sc_cf.cf_name, "safari", sizeof(sc->sc_cf.cf_name));
> +
> +	config_interrupts(sc->sc_dev, schizo_setup_cpufreq);
> +
>  	if (sc->sc_tomatillo) {
>  		/*
>  		 * Enable the IOCACHE.
> Index: dev/schizoreg.h
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/sparc64/dev/schizoreg.h,v
> retrieving revision 1.9
> diff -u -r1.9 schizoreg.h
> --- dev/schizoreg.h	25 Mar 2012 03:13:08 -0000	1.9
> +++ dev/schizoreg.h	13 Sep 2014 10:41:21 -0000
> @@ -4,6 +4,7 @@
>  /*
>   * Copyright (c) 2002 Jason L. Wright (jason%thought.net@localhost)
>   * Copyright (c) 2010 Matthew R. Green
> + * Copyright (c) 2014 Joël Bertrand (joel.bertrand%systella.fr@localhost)
>   * All rights reserved.
>   *
>   * Redistribution and use in source and binary forms, with or without
> @@ -15,7 +16,7 @@
>   *    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 ``AS IS'' AND ANY EXPRESS OR
> + e THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR

ooops. :_)

>   * 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 BE LIABLE FOR ANY DIRECT,
> @@ -76,8 +77,9 @@
>  	volatile u_int64_t	ue_afar;
>  	volatile u_int64_t	ce_afsr;
>  	volatile u_int64_t	ce_afar;
> +	volatile u_int64_t	esctrl;
>  
> -	volatile u_int64_t	_unused4[253942];
> +	volatile u_int64_t	_unused4[253941];
>  	struct schizo_pbm_regs pbm_a;
>  	struct schizo_pbm_regs pbm_b;
>  };
> @@ -101,6 +103,7 @@
>  #define	SCZ_UE_AFAR			0x10038
>  #define	SCZ_CE_AFSR			0x10040
>  #define	SCZ_CE_AFAR			0x10048
> +#define SCZ_ESCTRL			0x10050

please make this "#define<tab>SCZ_ESCTRL" like the above lines.

>  
>  /* These are relative to the PBM */
>  #define	SCZ_PCI_IOMMU_CTRL		0x00200
> @@ -298,3 +301,8 @@
>  	u_int32_t	size_hi;
>  	u_int32_t	size_lo;
>  };
> +
> +#define SCZ_ESCTRL_CLK_MAX		0x0000000000000000UL
> +#define SCZ_ESCTRL_CLK_DIV_2	0x0000000040000000UL
> +#define SCZ_ESCTRL_CLK_DIV_32	0x0000000080000000UL
> +#define SCZ_ESCTRL_CLK_MASK		0x00000000C0000000UL

these values seem wrong to me.  my copy of the schizo PRM says that
bits 0, 1 and 5 should be used.  "4.2.8 Safari Energy Star Control
Reigster" and "Table 4-17".  ie:

#define SCZ_ESCTRL_CLK_MAX	0x0000000000000001UL
#define SCZ_ESCTRL_CLK_DIV_2	0x0000000000000002UL
#define SCZ_ESCTRL_CLK_DIV_32	0x0000000000000020UL
#define SCZ_ESCTRL_CLK_MASK	0x0000000000000023UL

seems right to me.

> Index: dev/schizovar.h
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/sparc64/dev/schizovar.h,v
> retrieving revision 1.6
> diff -u -r1.6 schizovar.h
> --- dev/schizovar.h	25 Mar 2012 03:13:08 -0000	1.6
> +++ dev/schizovar.h	13 Sep 2014 10:41:21 -0000
> @@ -64,6 +64,7 @@
>  	int sc_busa;
>  	int sc_tomatillo;
>  	uint32_t sc_ver;
> +	struct cpufreq sc_cf;
>  };
>  
>  #define	schizo_read(sc,r) \
> Index: include/cpu.h
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/sparc64/include/cpu.h,v
> retrieving revision 1.112
> diff -u -r1.112 cpu.h
> --- include/cpu.h	4 Sep 2014 18:48:29 -0000	1.112
> +++ include/cpu.h	13 Sep 2014 10:41:22 -0000
> @@ -199,6 +199,9 @@
>  	bool			ci_pci_fault;
>  
>  	volatile void		*ci_ddb_regs;	/* DDB regs */
> +
> +	/* cpufreq */
> +	void				*sc;

this seems like a problem.

>  };
[ ... ]