Re: [PATCH 09/16] OMAP3: PM: Creating separate files for handling OMAP3 voltage related operations.

Kevin Hilman
2010-03-02T20:02:47Z

Thara Gopinath <thara@ti.com> writes:

> This patch creates voltage.c and voltage.h files and
> moves all voltage processor and voltage controller specific code
> from smartreflex.c and other places in the OMAP3 codebase into
> these two files.
> This along with smartreflex class driver addition will make
> smartreflex.c a generic driver to support both Class 2 and
> Class 3 smartreflex implementaions.
>
> Signed-off-by: Thara Gopinath <thara@ti.com>

This is definitely in the right direction. Thanks Thara.

Also, is this expected to be common for OMAP4? If not, maybe
this should be named voltage3xxx.[ch]?

More comments below, primarily regarding how the various VP/VC
registers are defined and accessed. I'd rather see them as offsets
into the PRCM rather than absolute virtual addresses. More on that
below...

> ---
> arch/arm/mach-omap2/Makefile | 3 +-
> arch/arm/mach-omap2/board-3430sdp.c | 3 +-
> arch/arm/mach-omap2/pm.h | 7 -
> arch/arm/mach-omap2/pm34xx.c | 87 +-----
> arch/arm/mach-omap2/resource34xx.c | 15 +-
> arch/arm/mach-omap2/resource34xx.h | 1 -
> arch/arm/mach-omap2/smartreflex-class3.c | 4 +
> arch/arm/mach-omap2/smartreflex.c | 370 +--------------------
> arch/arm/mach-omap2/smartreflex.h | 139 --------
> arch/arm/mach-omap2/voltage.c | 550 ++++++++++++++++++++++++++++++
> arch/arm/mach-omap2/voltage.h | 74 ++++
> 11 files changed, 640 insertions(+), 613 deletions(-)
> create mode 100644 arch/arm/mach-omap2/voltage.c
> create mode 100644 arch/arm/mach-omap2/voltage.h
>

[...]

> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
> new file mode 100644
> index 0000000..c0c2c17
> --- /dev/null
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -0,0 +1,550 @@
> +/*
> + * OMAP3/OMAP4 Voltage Management Routines
> + *
> + * Copyright (C) 2007 Texas Instruments, Inc.
> + * Rajendra Nayak <rnayak@ti.com>
> + * Lesly A M <x0080970@ti.com>
> + *
> + * Copyright (C) 2008 Nokia Corporation
> + * Kalle Jokiniemi
> + *
> + * Copyright (C) 2010 Texas Instruments, Inc.
> + * Thara Gopinath <thara@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/pm.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +
> +#include <plat/omap-pm.h>
> +#include <plat/omap34xx.h>
> +#include <plat/opp.h>
> +#include <plat/opp_twl_tps.h>
> +
> +#include "prm-regbits-34xx.h"
> +#include "voltage.h"
> +
> +#define MAX_TRIES 100
> +
> +/**
> + * OMAP3 Voltage controller SR parameters. TODO: Pass this info as part of
> + * board data or PMIC data
> + */
> +#define R_SRI2C_SLAVE_ADDR 0x12
> +#define R_VDD1_SR_CONTROL 0x00
> +#define R_VDD2_SR_CONTROL 0x01
> +
> +
> +/* Voltage processor register offsets */
> +struct vp_reg_offs {
> + void __iomem *vp_vpconfig_reg;
> + void __iomem *vp_vstepmin_reg;
> + void __iomem *vp_vstepmax_reg;
> + void __iomem *vp_vlimitto_reg;
> + void __iomem *vp_status_reg;
> + void __iomem *vp_voltage_reg;
> +};
>
> +/*
> + * Voltage processor structure conttaining info about
> + * the various register offsets and the bit field values
> + * for a particular instance of voltage processor.
> + */
> +struct vp_reg_info {
> + struct vp_reg_offs vp_offs;
> + /* Bit fields for VPx_VPCONFIG */
> + u32 vp_erroroffset;
> + u32 vp_errorgain;
> + /* Bit fields for VPx_VSTEPMIN */
> + u32 vp_stepmin;
> + u32 vp_smpswaittimemin;
> + /* Bit fields for VPx_VSTEPMAX */
> + u32 vp_stepmax;
> + u32 vp_smpswaittimemax;
> + /* Bit fields for VPx_VLIMITTO */
> + u32 vp_vddmin;
> + u32 vp_vddmax;
> + u32 vp_timeout;
> +};
> +static struct vp_reg_info *vp_reg;
> +/*
> + * Number of scalable voltage domains that has an independent
> + * Voltage processor
> + */
> +static int no_scalable_vdd;
> +
> +/* OMAP3 VP register offsets and other definitions */
> +struct __init vp_reg_offs omap3_vp_offs[] = {
> + /* VP1 */
> + {
> + .vp_vpconfig_reg = OMAP3430_PRM_VP1_CONFIG,
> + .vp_vstepmin_reg = OMAP3430_PRM_VP1_VSTEPMIN,
> + .vp_vstepmax_reg = OMAP3430_PRM_VP1_VSTEPMAX,
> + .vp_vlimitto_reg = OMAP3430_PRM_VP1_VLIMITTO,
> + .vp_status_reg = OMAP3430_PRM_VP1_STATUS,
> + .vp_voltage_reg = OMAP3430_PRM_VP1_VOLTAGE,
> + },
> + /* VP2 */
> + { .vp_vpconfig_reg = OMAP3430_PRM_VP2_CONFIG,
> + .vp_vstepmin_reg = OMAP3430_PRM_VP2_VSTEPMIN,
> + .vp_vstepmax_reg = OMAP3430_PRM_VP2_VSTEPMAX,
> + .vp_vlimitto_reg = OMAP3430_PRM_VP2_VLIMITTO,
> + .vp_status_reg = OMAP3430_PRM_VP2_STATUS,
> + .vp_voltage_reg = OMAP3430_PRM_VP2_VOLTAGE,
> + },
> +};
> +

Rather than absolute physical addresses, these should all simply be u8
offset values from the GR_MOD base. Changing the type to u8
and appending _OFFSET to the name should do the trick.

Also, I think you could remove the vp_ prefix from the name.

> +#define OMAP3_NO_SCALABLE_VDD ARRAY_SIZE(omap3_vp_offs)
> +static struct vp_reg_info omap3_vp_reg[OMAP3_NO_SCALABLE_VDD];
> +
> +
> +/* TODO: OMAP4 register offsets */
> +
> +/**
> + * Voltage controller register offsets
> + */
> +struct vc_reg_info {
> + void __iomem *vc_cmdval0_reg;
> + void __iomem *vc_cmdval1_reg;
> + void __iomem *vc_bypass_val_reg;

u8 offsets from GR_MOD please.

vc_ prefix is redundant here also

> +} vc_reg;
> +/*
> + * Default voltage controller settings for OMAP3
> + */
> +static struct prm_setup_vc vc_config = {
> + .clksetup = 0xff,
> + .voltsetup_time1 = 0xfff,
> + .voltsetup_time2 = 0xfff,
> + .voltoffset = 0xff,
> + .voltsetup2 = 0xff,
> + .vdd0_on = 0x30, /* 1.2v */
> + .vdd0_onlp = 0x20, /* 1.0v */
> + .vdd0_ret = 0x1e, /* 0.975v */
> + .vdd0_off = 0x00, /* 0.6v */
> + .vdd1_on = 0x2c, /* 1.15v */
> + .vdd1_onlp = 0x20, /* 1.0v */
> + .vdd1_ret = 0x1e, /* .975v */
> + .vdd1_off = 0x00, /* 0.6v */
> +};
> +
> +static inline u32 voltage_read_reg(void __iomem *offset)
> +{
> + return __raw_readl(offset);
> +}

These should use prm_[read|write]_*

static inline u32 voltage_read_reg(u8 offset)
{
return prm_read_mod_reg(OMAP3430_GR_MOD, offset);
}

> +
> +static inline void voltage_write_reg(void __iomem *offset, u32 value)
> +{
> + __raw_writel(value, offset);
> +}
> +

Kevin