On Mon, Mar 2, 2015 at 5:54 PM, Vince Hsu <vinceh@xxxxxxxxxx> wrote:
>>> {
>>> .id = 0x00,
>>> .name = "ptcr",
>>> @@ -959,7 +961,108 @@ static const struct tegra_smmu_swgroup
>>> tegra124_swgroups[] = {
>>> { .swgroup = TEGRA_SWGROUP_VI, .reg = 0x280 },
>>> };
>>>
>>> +static struct tegra_mc_hotreset tegra124_mc_hotreset[] = {
>>> + {TEGRA_SWGROUP_AFI, 0x200, 0x204, 0},
>>> + {TEGRA_SWGROUP_AVPC, 0x200, 0x204, 1},
>>> + {TEGRA_SWGROUP_DC, 0x200, 0x204, 2},
>>> + {TEGRA_SWGROUP_DCB, 0x200, 0x204, 3},
>>> + {TEGRA_SWGROUP_HC, 0x200, 0x204, 6},
>>> + {TEGRA_SWGROUP_HDA, 0x200, 0x204, 7},
>>> + {TEGRA_SWGROUP_ISP2, 0x200, 0x204, 8},
>>> + {TEGRA_SWGROUP_MPCORE, 0x200, 0x204, 9},
>>> + {TEGRA_SWGROUP_MPCORELP, 0x200, 0x204, 10},
>>> + {TEGRA_SWGROUP_MSENC, 0x200, 0x204, 11},
>>> + {TEGRA_SWGROUP_PPCS, 0x200, 0x204, 14},
>>> + {TEGRA_SWGROUP_SATA, 0x200, 0x204, 15},
>>> + {TEGRA_SWGROUP_VDE, 0x200, 0x204, 16},
>>> + {TEGRA_SWGROUP_VI, 0x200, 0x204, 17},
>>> + {TEGRA_SWGROUP_VIC, 0x200, 0x204, 18},
>>> + {TEGRA_SWGROUP_XUSB_HOST, 0x200, 0x204, 19},
>>> + {TEGRA_SWGROUP_XUSB_DEV, 0x200, 0x204, 20},
>>> + {TEGRA_SWGROUP_TSEC, 0x200, 0x204, 22},
>>> + {TEGRA_SWGROUP_SDMMC1A, 0x200, 0x204, 29},
>>> + {TEGRA_SWGROUP_SDMMC2A, 0x200, 0x204, 30},
>>> + {TEGRA_SWGROUP_SDMMC3A, 0x200, 0x204, 31},
>>> + {TEGRA_SWGROUP_SDMMC4A, 0x970, 0x974, 0},
>>> + {TEGRA_SWGROUP_ISP2B, 0x970, 0x974, 1},
>>> + {TEGRA_SWGROUP_GPU, 0x970, 0x974, 2},
>>> +};
>>> +
>>> #ifdef CONFIG_ARCH_TEGRA_124_SOC
>>> +
>>> +static bool tegra124_stable_hotreset_check(struct tegra_mc *mc,
>>> + u32 reg, u32 *stat)
>>> +{
>>> + int i;
>>> + u32 cur_stat;
>>> + u32 prv_stat;
>>> +
>>> + /*
>>> + * There might be a glitch seen with the status register if we
>>> program
>>> + * the control register and then read the status register in a
>>> short
>>> + * window due to a HW bug. So here we poll for a stable status
>>> read.
>>> + */
>>> + prv_stat = mc_readl(mc, reg);
>>> + for (i = 0; i < 5; i++) {
>>
>> Why 5 times?
>
> This is a magic number from downstream. It should be enough to reach a
> stable state.
Most people don't like arbitrary numbers without any explanation. And
sentences like "should be enough" make me nervous. :) Could you try to
document this number so we at least know why *this* value instead of
one of the possible other values an integer can take?
>
>>> + cur_stat = mc_readl(mc, reg);
>>> + if (cur_stat != prv_stat)
>>> + return false;
>>> + }
>>> + *stat = cur_stat;
>>> + return true;
>>> +}
>>> +
>>> +static int tegra124_mc_flush(struct tegra_mc *mc,
>>> + const struct tegra_mc_hotreset *hotreset)
>>> +{
>>> + u32 val;
>>> +
>>> + if (!mc || !hotreset)
>>> + return -EINVAL;
>>> +
>>> + /* XXX add mutex */
>>
>> I guess this is a TODO for when this series exists the RFC state. :)
>
> Oh, yes.
>
>>> +
>>> + val = mc_readl(mc, hotreset->ctrl);
>>> + val |= BIT(hotreset->bit);
>>> + mc_writel(mc, val, hotreset->ctrl);
>>> + mc_readl(mc, hotreset->ctrl);
>>> +
>>> + /* poll till the flush is done */
>>> + do {
>>> + udelay(10);
>>> + val = 0;
>>> + if (!tegra124_stable_hotreset_check(mc, hotreset->status,
>>> &val))
>>> + continue;
>>> + } while (!(val & BIT(hotreset->bit)));
>>
>> So here you are waiting for the right bit in hotreset->status to turn
>> to 1. Why is the whole thing with tegra124_stable_hotreset_check()
>> needed? Could it switch from 1 to 0 to 1 again, with the first window
>> at 1 being invalid?
>
> There is a comment which describes why we have to check this in
> tegra124_stable_hotreset_check().
>
>> In that case isn't there a risk that
>> tegra124_stable_hotreset_check() might do 5 consecutive reads on that
>> invalid state and let us continue before the flush is completed?
>
> So there is another check in while(). :)
Yeah, I guess the issue is reduced to ensuring that 6 consecutive
identical reads are enough to guarantee that the value of this
register is stable.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
