Re: [PATCH v2] PM / AVS: rockchip-io: add driver handling Rockchip io domains

[Santosh Shilimkar]

On Thursday 11 September 2014 06:07 PM, Doug Anderson wrote:
> Santosh,
> 
> On Thu, Sep 11, 2014 at 2:31 PM, Santosh Shilimkar
> <santosh.shilimkar@xxxxxx> wrote:
>>> +Required properties:
>>> +- compatible: should be one of:
>>> +  - "rockchip,rk3188-iodomain" for rk3188
>>> +  - "rockchip,rk3288-iodomain" for rk3288
>> The key word 'voltage' is missing from the compatible. iodomain itself
>> doesn't convey what it is actually.
> 
> Sure, so you want "rockchip,rk3288-io-voltage-domain" then?
>
Sounds good for me.
 
[..]

>>> diff --git a/drivers/power/avs/rockchip-io-domain.c b/drivers/power/avs/rockchip-io-domain.c
>>> new file mode 100644
>>> index 0000000..f4e0ebc
>>> --- /dev/null
>>> +++ b/drivers/power/avs/rockchip-io-domain.c
>>> @@ -0,0 +1,333 @@
>>> +/*
>>> + * Rockchip IO Voltage Domain driver
>>> + *
>>> + * Copyright 2014 MundoReader S.L.
>>> + * Copyright 2014 Google, Inc.
>>> + *
>>> + * This software is licensed under the terms of the GNU General Public
>>> + * License version 2, as published by the Free Software Foundation, and
>>> + * may be copied, distributed, and modified under those terms.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/err.h>
>>> +#include <linux/mfd/syscon.h>
>> The bindings are not talking about syscon usage. You might
>> want to document it appropriately to make the usage clear.
> 
> Doesn't the bindings have this?
> 
>> +- rockchip,grf: phandle to the syscon managing the "general register files"
> 
> I actually forgot that in v1, but I added it to v2.
> 
Sorry didn't spot that. Ignore the comment.

> 
>>> +#define MAX_VOLTAGE_1_8              1980000
>> This is close to 2V, Is that intentional.
>>
>>> +#define MAX_VOLTAGE_3_3              3600000
>>> +
>> Same here.
> 
> I got these numbers from the document "Rocchip RK3288 datasheet".
> Under "Recommended Operating Conditions" for "Digital GPIO".  When the
> typical is 3.3V the max is 3.6V.  When the typical is 1.8V the max is
> 1.98V.
> 
> The way these numbers are used is:
> 
> If the voltage on a rail is above the "1.8" voltage (1.98V) we'll tell
> the SoC we're at 3.3.  If the voltage on a rail is above the "3.3V"
> we'll consider that to be an error.
> 
> There's one secret ulterior motive here though that I'll admit to.
> When a client uses regulator_set_voltage() they actually specify a min
> and max voltage.  They might say that they want 3.3V by saying that
> they want a voltage between 2.7V and 3.6V.  They might say that they
> want 1.8V by saying that they want a voltage between 1.7V and 1.95V.
> In our pre-change notification we are only told the possible range.
> It's nice if things work properly in that case.  If we find some case
> that doesn't work later we can always get fancier and try to figure
> out what voltage the regulator will end up at, but I haven't seen the
> need yet.
> 
> Note that the 1.7 - 1.95V is more than hypothetical.  dw_mmc requests
> those ranges.  I'll admit that I was involved in that code, but I'll
> also admit to having stolen those numbers from sdhci.
> 
Thanks for explaination. I suggest you to document it above those macro's
so that its not confusing for any reader.

> 
>>> +struct rockchip_iodomain;
>>> +
>>> +/**
>>> + * @supplies: voltage settings matching the register bits.
>>> + */
>>> +struct rockchip_iodomain_soc_data {
>>> +     int grf_offset;
>>> +     const char *supply_names[MAX_SUPPLIES];
>>> +     void (*init)(struct rockchip_iodomain *iod);
>>> +};
>>> +
>>> +struct rockchip_iodomain_supply {
>>> +     struct rockchip_iodomain *iod;
>>> +     struct regulator *reg;
>>> +     struct notifier_block nb;
>>> +     int idx;
>>> +};
>>> +
>>> +struct rockchip_iodomain {
>>> +     struct device *dev;
>>> +     struct regmap *grf;
>>> +     struct rockchip_iodomain_soc_data *soc_data;
>>> +     struct rockchip_iodomain_supply supplies[MAX_SUPPLIES];
>>> +};
>>> +
>>> +static int rockchip_iodomain_write(struct rockchip_iodomain_supply *supply,
>>> +                                int uV)
>>> +{
>>> +     struct rockchip_iodomain *iod = supply->iod;
>>> +     u32 val;
>>> +     int ret;
>>> +
>>> +     /* set value bit */
>>> +     val = (uV > MAX_VOLTAGE_1_8) ? 0 : 1;
>>> +     val <<= supply->idx;
>>> +
>>> +     /* apply hiword-mask */
>>> +     val |= (BIT(supply->idx) << 16);
>>> +
>>> +     ret = regmap_write(iod->grf, iod->soc_data->grf_offset, val);
>>> +     if (ret)
>>> +             dev_err(iod->dev, "Couldn't write to GRF\n");
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int rockchip_iodomain_notify(struct notifier_block *nb,
>>> +                                 unsigned long event,
>>> +                                 void *data)
>>> +{
>>> +     struct rockchip_iodomain_supply *supply =
>>> +                     container_of(nb, struct rockchip_iodomain_supply, nb);
>>> +     int uV;
>>> +     int ret;
>>> +
>>> +     /*
>>> +      * According to Rockchip it's important to keep the SoC IO domain
>>> +      * higher than (or equal to) the external voltage.  That means we need
>>> +      * to change it before external voltage changes happen in the case
>>> +      * of an increase.
>>> +      */
>>> +     if (event & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
>>> +             struct pre_voltage_change_data *pvc_data = data;
>>> +
>>> +             uV = max_t(unsigned long, pvc_data->old_uV, pvc_data->max_uV);
>>> +     } else if (event & (REGULATOR_EVENT_VOLTAGE_CHANGE |
>>> +                         REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE)) {
>>> +             uV = (unsigned long)data;
>>> +     } else {
>>> +             return NOTIFY_OK;
>>> +     }
>>> +
>>> +     dev_dbg(supply->iod->dev, "Setting to %d\n", uV);
>>> +
>>> +     if (uV > MAX_VOLTAGE_3_3) {
>>> +             dev_err(supply->iod->dev, "Voltage too high: %d\n", uV);
>>> +
>>> +             if (event == REGULATOR_EVENT_PRE_VOLTAGE_CHANGE)
>>> +                     return NOTIFY_BAD;
>>> +     }
>>> +
>>> +     ret = rockchip_iodomain_write(supply, uV);
>>> +     if (ret && event == REGULATOR_EVENT_PRE_VOLTAGE_CHANGE)
>>> +             return NOTIFY_BAD;
>>> +
>>> +     dev_info(supply->iod->dev, "Setting to %d done\n", uV);
>>> +     return NOTIFY_OK;
>>> +}
>>> +
>>> +#define RK3288_SOC_CON2                      0x24c
>>> +#define RK3288_SOC_CON2_FLASH0               BIT(7)
>>> +#define RK3288_SOC_FLASH_SUPPLY_NUM  2
>>> +
>> Not a strong opinion but you can club all the defines on top
>> of the file.
> 
> Sure, I'll move them.
> 
OK. Feel free to add my reviewed-by tag after the updates if you need one.

regards,
Santosh

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/