Re: [U-Boot] [U-Boot,2/5] power: pmic: rk816: support rk816 pmic

On Wed, 13 Sep 2017, Kever Yang wrote:

From: Elaine Zhang zhangqing@rock-chips.com

Add Rockchip pmic rk816 support.

Could you summarise some of the key-features of the RK816 here? Just a few words, so we know how it's different from the others and what it's used for (e.g. "PMIC matching the RK3xxx").

Signed-off-by: Elaine Zhang zhangqing@rock-chips.com Signed-off-by: Kever Yang kever.yang@rock-chips.com

---

drivers/power/pmic/rk8xx.c | 1 + drivers/power/regulator/rk8xx.c | 212 ++++++++++++++++++++++++++++++++-------- include/power/rk8xx_pmic.h | 9 +- 3 files changed, 182 insertions(+), 40 deletions(-)

diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c index eb3ec0f..0fdea95 100644 --- a/drivers/power/pmic/rk8xx.c +++ b/drivers/power/pmic/rk8xx.c @@ -100,6 +100,7 @@ static struct dm_pmic_ops rk8xx_ops = {

static const struct udevice_id rk8xx_ids[] = { { .compatible = "rockchip,rk808" },

• { .compatible = "rockchip,rk816" }, { .compatible = "rockchip,rk818" }, { }

}; diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c index 76fc2ef..cf3566e 100644 --- a/drivers/power/regulator/rk8xx.c +++ b/drivers/power/regulator/rk8xx.c @@ -48,6 +48,21 @@ static const struct rk8xx_reg_info rk808_buck[] = { { 1800000, 100000, REG_BUCK4_ON_VSEL, RK808_BUCK4_VSEL_MASK, }, };

+static const struct rk8xx_reg_info rk816_buck[] = {

• /* buck 1 */
• { 712500, 12500, REG_BUCK1_ON_VSEL, RK818_BUCK_VSEL_MASK, },
• { 1800000, 200000, REG_BUCK1_ON_VSEL, RK818_BUCK_VSEL_MASK, },
• { 2300000, 0, REG_BUCK1_ON_VSEL, RK818_BUCK_VSEL_MASK, },
• /* buck 2 */
• { 712500, 12500, REG_BUCK2_ON_VSEL, RK818_BUCK_VSEL_MASK, },
• { 1800000, 200000, REG_BUCK2_ON_VSEL, RK818_BUCK_VSEL_MASK, },
• { 2300000, 0, REG_BUCK2_ON_VSEL, RK818_BUCK_VSEL_MASK, },
• /* buck 3 */
• { 712500, 12500, -1, RK818_BUCK_VSEL_MASK, },
• /* buck 4 */
• { 800000, 100000, REG_BUCK4_ON_VSEL, RK818_BUCK4_VSEL_MASK, },

+};

static const struct rk8xx_reg_info rk818_buck[] = { { 712500, 12500, REG_BUCK1_ON_VSEL, RK818_BUCK_VSEL_MASK, }, { 712500, 12500, REG_BUCK2_ON_VSEL, RK818_BUCK_VSEL_MASK, }, @@ -67,6 +82,15 @@ static const struct rk8xx_reg_info rk808_ldo[] = { { 1800000, 100000, REG_LDO8_ON_VSEL, RK808_LDO_VSEL_MASK, }, };

+static const struct rk8xx_reg_info rk816_ldo[] = {

• { 800000, 100000, REG_LDO1_ON_VSEL, RK818_LDO_VSEL_MASK, },
• { 800000, 100000, REG_LDO2_ON_VSEL, RK818_LDO_VSEL_MASK, },
• { 800000, 100000, REG_LDO3_ON_VSEL, RK818_LDO_VSEL_MASK, },
• { 800000, 100000, REG_LDO4_ON_VSEL, RK818_LDO_VSEL_MASK, },
• { 800000, 100000, REG_LDO5_ON_VSEL, RK818_LDO_VSEL_MASK, },
• { 800000, 100000, REG_LDO6_ON_VSEL, RK818_LDO_VSEL_MASK, },

+};

static const struct rk8xx_reg_info rk818_ldo[] = { { 1800000, 100000, REG_LDO1_ON_VSEL, RK818_LDO_VSEL_MASK, }, { 1800000, 100000, REG_LDO2_ON_VSEL, RK818_LDO_VSEL_MASK, }, @@ -88,10 +112,24 @@ static const uint rk818_chrg_shutdown_vsel_array[] = { };

static const struct rk8xx_reg_info *get_buck_reg(struct udevice *pmic,

• 			     int num)
  

• 				 int num, int uvolt)
  

{ struct rk8xx_priv *priv = dev_get_priv(pmic);

• switch (priv->variant) {
• case RK816_ID:

• switch (num) {
  

• case 0:
  

• case 1:
  

• 	if (uvolt <= 1450000)
  

• 		return &rk816_buck[num * 3 + 0];
  

• 	else if (uvolt <= 2200000)
  

• 		return &rk816_buck[num * 3 + 1];
  

• 	else
  

• 		return &rk816_buck[num * 3 + 2];
  

• default:
  

• 	return &rk816_buck[num + 4];
  

• }
  

I am very concerned with this driver turning into a large switch statement: this really is not the way driver_data is supposed to be used.

Can we have a single path through this function and use driver_data to parameterize the cut-offs? As an alternative, we'll need to start splitting this into separate drivers.

case RK818_ID: return &rk818_buck[num]; default: @@ -101,7 +139,7 @@ static const struct rk8xx_reg_info *get_buck_reg(struct udevice *pmic,

static int _buck_set_value(struct udevice *pmic, int buck, int uvolt) {

• const struct rk8xx_reg_info *info = get_buck_reg(pmic, buck - 1);

• const struct rk8xx_reg_info *info = get_buck_reg(pmic, buck, uvolt); int mask = info->vsel_mask; int val;

@@ -114,23 +152,76 @@ static int _buck_set_value(struct udevice *pmic, int buck, int uvolt) return pmic_clrsetbits(pmic, info->vsel_reg, mask, val); }

+static int _buck_get_enable(struct udevice *pmic, int buck) +{

• struct rk8xx_priv *priv = dev_get_priv(pmic);
• uint mask = 0;
• int ret = 0;
• switch (priv->variant) {
• case RK816_ID:

• if (buck >= 4) {
  

• 	mask = 1 << (buck - 4);
  

• 	ret = pmic_reg_read(pmic, RK816_REG_DCDC_EN2);
  

• } else {
  

• 	mask = 1 << buck;
  

• 	ret = pmic_reg_read(pmic, RK816_REG_DCDC_EN1);
  

• }
  

• break;
  

• case RK808_ID:
• case RK818_ID:

• mask = 1 << buck;
  

• ret = pmic_reg_read(pmic, REG_DCDC_EN);
  

• if (ret < 0)
  

• 	return ret;
  

• break;
  

• }
• return ret & mask ? true : false;

+}

Again, the RK816 appears to be a significantly different device.

static int _buck_set_enable(struct udevice *pmic, int buck, bool enable) {

• uint mask;

• uint mask, value, en_reg; int ret;
• struct rk8xx_priv *priv = dev_get_priv(pmic);

• buck--;
• mask = 1 << buck;
• if (enable) {

• ret = pmic_clrsetbits(pmic, REG_DCDC_ILMAX, 0, 3 << (buck * 2));
  

• if (ret)
  

• 	return ret;
  

• ret = pmic_clrsetbits(pmic, REG_DCDC_UV_ACT, 1 << buck, 0);
  

• if (ret)
  

• 	return ret;
  

• switch (priv->variant) {
• case RK816_ID:

• if (buck >= 4) {
  

• 	buck -= 4;
  

• 	en_reg = RK816_REG_DCDC_EN2;
  

• } else {
  

• 	en_reg = RK816_REG_DCDC_EN1;
  

• }
  

• if (enable)
  

• 	value = ((1 << buck) | (1 << (buck + 4)));
  

• else
  

• 	value = ((0 << buck) | (1 << (buck + 4)));
  

• ret = pmic_reg_write(pmic, en_reg, value);
  

• break;
  

• case RK808_ID:
• case RK818_ID:

• mask = 1 << buck;
  

• if (enable) {
  

• 	ret = pmic_clrsetbits(pmic, REG_DCDC_ILMAX,
  

• 			      0, 3 << (buck * 2));
  

• 	if (ret)
  

• 		return ret;
  

• 	ret = pmic_clrsetbits(pmic, REG_DCDC_UV_ACT,
  

• 			      1 << buck, 0);
  

• 	if (ret)
  

• 		return ret;
  

• }
  

• ret = pmic_clrsetbits(pmic, REG_DCDC_EN, mask,
  

• 		      enable ? mask : 0);
  

• break;
  

• default:

• ret = -EINVAL;
  

  }

• return pmic_clrsetbits(pmic, REG_DCDC_EN, mask, enable ? mask : 0);

• return ret;

}

#ifdef ENABLE_DRIVER

This define doesn't make any sense: I see that it's always been here, but still: it is just a wrapper around a "#ifndef CONFIG_SPL_BUILD". Can we use the #ifndef CONFIG_SPL_BUILD directly?

@@ -138,7 +229,10 @@ static const struct rk8xx_reg_info *get_ldo_reg(struct udevice *pmic, int num) { struct rk8xx_priv *priv = dev_get_priv(pmic);

• switch (priv->variant) {
• case RK816_ID:

• return &rk816_ldo[num];
  

  case RK818_ID: return &rk818_ldo[num]; default:

@@ -146,10 +240,70 @@ static const struct rk8xx_reg_info *get_ldo_reg(struct udevice *pmic, } }

+static int _ldo_get_enable(struct udevice *pmic, int ldo) +{

• struct rk8xx_priv *priv = dev_get_priv(pmic);
• uint mask = 0;
• int ret = 0;
• switch (priv->variant) {
• case RK816_ID:

• if (ldo >= 4) {
  

• 	mask = 1 << (ldo - 4);
  

• 	ret = pmic_reg_read(pmic, RK816_REG_LDO_EN2);
  

• } else {
  

• 	mask = 1 << ldo;
  

• 	ret = pmic_reg_read(pmic, RK816_REG_LDO_EN1);
  

• }
  

Same comment as above.

• break;
  

• case RK808_ID:
• case RK818_ID:

• mask = 1 << ldo;
  

• ret = pmic_reg_read(pmic, REG_LDO_EN);
  

• if (ret < 0)
  

• 	return ret;
  

• break;
  

• }
• return ret & mask ? true : false;

+}

+static int _ldo_set_enable(struct udevice *pmic, int ldo, bool enable) +{

• struct rk8xx_priv *priv = dev_get_priv(pmic);
• uint mask, value, en_reg;
• int ret = 0;
• switch (priv->variant) {
• case RK816_ID:

• if (ldo >= 4) {
  

• 	ldo -= 4;
  

• 	en_reg = RK816_REG_LDO_EN2;
  

• } else {
  

• 	en_reg = RK816_REG_LDO_EN1;
  

• }
  

• if (enable)
  

• 	value = ((1 << ldo) | (1 << (ldo + 4)));
  

• else
  

• 	value = ((0 << ldo) | (1 << (ldo + 4)));
  

• ret = pmic_reg_write(pmic, en_reg, value);
  

• break;
  

Again, it looks as if this should be a separate driver.

• case RK808_ID:
• case RK818_ID:

• mask = 1 << ldo;
  

• ret = pmic_clrsetbits(pmic, REG_LDO_EN, mask,
  

• 		       enable ? mask : 0);
  

• break;
  

• }
• return ret;

+}

static int buck_get_value(struct udevice *dev) { int buck = dev->driver_data - 1;

• const struct rk8xx_reg_info *info = get_buck_reg(dev->parent, buck);

• const struct rk8xx_reg_info *info = get_buck_reg(dev->parent, buck, 0); int mask = info->vsel_mask; int ret, val;

@@ -165,14 +319,14 @@ static int buck_get_value(struct udevice *dev)

static int buck_set_value(struct udevice *dev, int uvolt) {

• int buck = dev->driver_data;

• int buck = dev->driver_data - 1;

  return _buck_set_value(dev->parent, buck, uvolt);

}

static int buck_set_enable(struct udevice *dev, bool enable) {

• int buck = dev->driver_data;

• int buck = dev->driver_data - 1;

  return _buck_set_enable(dev->parent, buck, enable);

} @@ -180,16 +334,8 @@ static int buck_set_enable(struct udevice *dev, bool enable) static int buck_get_enable(struct udevice *dev) { int buck = dev->driver_data - 1;

• int ret;

• uint mask;

• mask = 1 << buck;

• ret = pmic_reg_read(dev->parent, REG_DCDC_EN);

• if (ret < 0)

• return ret;
  

• return ret & mask ? true : false;

• return _buck_get_enable(dev->parent, buck);

}

static int ldo_get_value(struct udevice *dev) @@ -228,27 +374,15 @@ static int ldo_set_value(struct udevice *dev, int uvolt) static int ldo_set_enable(struct udevice *dev, bool enable) { int ldo = dev->driver_data - 1;

• uint mask;

• mask = 1 << ldo;

• return pmic_clrsetbits(dev->parent, REG_LDO_EN, mask,

• 	       enable ? mask : 0);
  

• return _ldo_set_enable(dev->parent, ldo, enable);

}

static int ldo_get_enable(struct udevice *dev) { int ldo = dev->driver_data - 1;

• int ret;

• uint mask;

• mask = 1 << ldo;

• ret = pmic_reg_read(dev->parent, REG_LDO_EN);

• if (ret < 0)

• return ret;
  

• return ret & mask ? true : false;

• return _ldo_get_enable(dev->parent, ldo);

}

static int switch_set_enable(struct udevice *dev, bool enable) diff --git a/include/power/rk8xx_pmic.h b/include/power/rk8xx_pmic.h index 47a6b36..8e821c3 100644 --- a/include/power/rk8xx_pmic.h +++ b/include/power/rk8xx_pmic.h @@ -171,8 +171,15 @@ enum { };

enum {

• RK805_ID = 0x8050,

• RK816_REG_DCDC_EN1 = 0x23,
• RK816_REG_DCDC_EN2,
• RK816_REG_LDO_EN1 = 0x27,
• RK816_REG_LDO_EN2,

+};

+enum { RK808_ID = 0x0000,

• RK816_ID = 0x8160, RK818_ID = 0x8180,

};