On 12/01/2015 07:52 AM, Stephen Boyd wrote: > On 11/26, Rajendra Nayak wrote: >> @@ -58,30 +58,34 @@ static int gdsc_toggle_logic(struct gdsc *sc, bool en) >> { >> int ret; >> u32 val = en ? 0 : SW_COLLAPSE_MASK; >> - u32 check = en ? PWR_ON_MASK : 0; >> unsigned long timeout; >> + unsigned int status_reg = sc->gdscr; >> >> ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val); >> if (ret) >> return ret; >> >> timeout = jiffies + usecs_to_jiffies(TIMEOUT_US); >> - do { >> - ret = regmap_read(sc->regmap, sc->gdscr, &val); >> - if (ret) >> - return ret; >> >> - if ((val & PWR_ON_MASK) == check) >> + if (sc->gds_hw_ctrl) { >> + status_reg = sc->gds_hw_ctrl; >> + /* >> + * The gds hw controller asserts/de-asserts the status bit soon >> + * after it receives a power on/off request from a master. >> + * The controller then takes around 8 xo cycles to start its internal >> + * state machine and update the status bit. During this time, the >> + * status bit does not reflect the true status of the core. >> + * Add a delay of 1 us between writing to the SW_COLLAPSE bit and >> + * polling the status bit >> + */ > > Please indent this correctly to the udelay indent level.
will do. > >> + udelay(1); >> + } >> + >> + do { >> + if (gdsc_is_enabled(sc, status_reg) == en) >> return 0; >> } while (time_before(jiffies, timeout)); >> >> - ret = regmap_read(sc->regmap, sc->gdscr, &val); >> - if (ret) >> - return ret; >> - >> - if ((val & PWR_ON_MASK) == check) >> - return 0; >> - > > This opens a bug where we timeout and then the status bit changes > after the timeout. One more check is good and should stay. We > could also change this to ktime instead of jiffies. That would be > a good improvement. If the status bit does change after timeout maybe the timeout isn't really enough and needs to be increased? > >> return -ETIMEDOUT; >> } >> >> @@ -165,6 +169,7 @@ static int gdsc_init(struct gdsc *sc) >> { >> u32 mask, val; >> int on, ret; >> + unsigned int reg; >> >> /* >> * Disable HW trigger: collapse/restore occur based on registers writes. >> @@ -185,7 +190,8 @@ static int gdsc_init(struct gdsc *sc) >> return ret; >> } >> >> - on = gdsc_is_enabled(sc); >> + reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr; >> + on = gdsc_is_enabled(sc, reg); > > If the gdsc is voteable, then we need to make sure that the vote > is from us when we boot up. Otherwise the kernel may think that > the gdsc is enabled, but it gets turned off by some other master > later on. I don't know if this causes some sort of problem for > the power domain framework, but we can't rely on the status bit > unless we're sure that we've actually set the register to enable > it. In the normal enable/disable path we'll always know we set > the register, so this really only matters once when we boot up. right, thanks for catching this. However if we vote for a votable GDSC just because its ON at boot (due to someone else having voted) we won't ever remove the vote keeping it always enabled. I think a safe way would be to consider all votable gdscs for which *we* haven't voted explicitly to be disabled at boot? > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html