[U-Boot] [PATCH 1/7] Tegra30: Add AVP (arm720t) files

[Tom Warren]

Stephen,

On Wed, Oct 3, 2012 at 11:23 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 10/02/2012 04:45 PM, Tom Warren wrote:
>> This provides SPL support for T30 boards - AVP early init, plus
>> CPU (A9) init/jump to main U-Boot.
>
>> diff --git a/arch/arm/cpu/arm720t/tegra30/cpu.c b/arch/arm/cpu/arm720t/tegra30/cpu.c
>
>> +/*
>> + * Timing tables for each SOC for all four oscillator options.
>> + */
>> +static struct clk_pll_table tegra_pll_x_table[TEGRA_SOC_COUNT]
>> +                                             [CLOCK_OSC_FREQ_COUNT] = {
>> +     /* T20: 1 GHz */
>
> This is odd; it's a Tegra30-specific file, yet has data tables for
> Tegra20 too, and various code that only makes sense to differentiate
> Tegra20 and Tegra30 at runtime.

I'd meant to pull this out, or commonize the cpu.c files in arm720t,
but forgot about it.

I'll move common code to arm720t/tegra/cpu.c and leave HW-specific
stuff in tegra[23]0/cpu.c on the next rev.

>
> Either everything in this file should be Tegra30-specific, or whatever
> new code is being added here should be added to a file in tegra-common
> if it needs to handle both SoCs.
>
>> +static int get_chip_type(void)
>> +{
>> +     /*
>> +      * T30 has two options. We will return TEGRA_SOC_T30 until
>> +      * we have the fdt set up when it may change to
>> +      * TEGRA_SOC_T30_408MHZ depending on what we set PLLP to.
>> +      */
>
> I'm not sure what the FDT has to do with it? Certainly at this point in
> the series, I doubt that comment is accurate. Do we actually reprogram
> the PLLs based on FDT later?

Most of this file was brought in during a merge with our internal T30
U-Boot repo, so most of these comments aren't from me, but from the
folks that brought up T30 in-house. I chose to start at the slower
PLLX rate (216?) for bringup, and planned to phase in 408MHz support
later, once I was sure of a stable SW base.

>
>> +     if (clock_get_rate(CLOCK_ID_PERIPH) == 408000000)
>> +             return TEGRA_SOC_T30_408MHZ;
>> +     else
>> +             return TEGRA_SOC_T30;
>
> I thought we'd decided not to support one of those options, but perhaps
> it's used somewhere...

See above. Once 408MHz is ported/working, I can remove the slower
clock rate entirely.

>
>> +static void adjust_pllp_out_freqs(void)
>> +{
>> +     struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE;
>> +     struct clk_pll *pll = &clkrst->crc_pll[CLOCK_ID_PERIPH];
>> +     u32 reg;
>> +
>> +     /* Set T30 PLLP_OUT1, 2, 3 & 4 freqs to 9.6, 48, 102 & 204MHz */
>> +     reg = readl(&pll->pll_out[0]);     /* OUTA, contains OUT2 / OUT1 */
>> +     reg |= (IN_408_OUT_48_DIVISOR << PLLP_OUT2_RATIO) | PLLP_OUT2_OVR
>> +             | (IN_408_OUT_9_6_DIVISOR << PLLP_OUT1_RATIO) | PLLP_OUT1_OVR;
>> +     writel(reg, &pll->pll_out[0]);
>> +
>> +     reg = readl(&pll->pll_out[1]);   /* OUTB, contains OUT4 / OUT3 */
>> +     reg |= (IN_408_OUT_204_DIVISOR << PLLP_OUT4_RATIO) | PLLP_OUT4_OVR
>> +             | (IN_408_OUT_102_DIVISOR << PLLP_OUT3_RATIO) | PLLP_OUT3_OVR;
>> +     writel(reg, &pll->pll_out[1]);
>> +}
>
> I don't think that code is ever used, since init_pllx() below is only
> ever called with slow==1, so never calls it.

Yeah, this'll be used later to get to 408MHz. I'd planned to use it
but didn't get around to it. I can remove it in v2, or try for full
speed (408MHz) instead. Depends on bandwidth & priorities.

>
>> +void tegra_i2c_ll_write_addr(uint addr, uint config)
>> +{
>> +     struct i2c_ctlr *reg = (struct i2c_ctlr *)TEGRA_DVC_BASE;
>> +
>> +     writel(addr, &reg->cmd_addr0);
>> +     writel(config, &reg->cnfg);
>> +}
>> +
>> +void tegra_i2c_ll_write_data(uint data, uint config)
>> +{
>> +     struct i2c_ctlr *reg = (struct i2c_ctlr *)TEGRA_DVC_BASE;
>> +
>> +     writel(data, &reg->cmd_data1);
>> +     writel(config, &reg->cnfg);
>> +}
>> +
>> +static void enable_cpu_power_rail(void)
>> +{
>> +     struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
>> +     u32 reg;
>> +
>> +     debug("enable_cpu_power_rail entry\n");
>> +     reg = readl(&pmc->pmc_cntrl);
>> +     reg |= CPUPWRREQ_OE;
>> +     writel(reg, &pmc->pmc_cntrl);
>> +
>> +     /*
>> +      * Pulse PWRREQ via I2C.  We need to find out what this is
>> +      * doing, tidy up the code and maybe find a better place for it.
>> +      */
>> +     tegra_i2c_ll_write_addr(0x005a, 0x0002);
>> +     tegra_i2c_ll_write_data(0x2328, 0x0a02);
>
> With a TPS65911x attached to the DVC I2C bus, that sets VDD to 1.4V (I
> assume that's both the VDD1 and VDD2 outputs, but the PMIC datasheet
> isn't too clear on that at a quick glance), and:
>
>> +     udelay(1000);
>> +     tegra_i2c_ll_write_data(0x0127, 0x0a02);
>
> ... and this enables the VDD regulator.
>
> So, this is:
> a) Entirely specific to a TPS65911x regulator. I think this warrants a
> very large FIXME comment.
> b) Nothing to do with pulsing PWRREQ via I2C.
> c) Really should be done via programming the EEPROM on the PMIC so that
> SW doesn't have to do this, but I guess that didn't happen.

Again, this comes from our internal repo & wasn't done by me, so I had
no knowledge of what exactly it was doing. But removing it resulted in
a hung system, so I had to leave it in. Your comments are helpful -
I'll revise this in v2 with new comments and better reg/data defines,
etc.

>
>> +static void reset_A9_cpu(int reset)
>> +{
>> +     /*
>> +     * NOTE:  Regardless of whether the request is to hold the CPU in reset
>> +     *        or take it out of reset, every processor in the CPU complex
>> +     *        except the master (CPU 0) will be held in reset because the
>> +     *        AVP only talks to the master. The AVP does not know that there
>> +     *        are multiple processors in the CPU complex.
>> +     */
>
> At least the last sentence there is false; this code is running on the
> AVP and there's explicit code above that determines the number of CPUs
> in the main CPU complex (get_num_cpus) and sets separate reset bits for
> each of them (enable_cpu_clock). Oh, and ~7 lines below, there's a loop
> over num_cpus:

Again, a comment from a previous dev, not me. I think they're just
saying that the AVP is only working on 1 CPU, and doesn't do full init
of the other ones, since they're aren't needed for U-Boot to
find/fetch a kernel, etc.

>
>> +     int mask = crc_rst_cpu | crc_rst_de | crc_rst_debug;
>> +     int num_cpus = get_num_cpus();
>> +     int cpu;
>> +
>> +     debug("reset_a9_cpu entry\n");
>> +     /* Hold CPUs 1 onwards in reset, and CPU 0 if asked */
>> +     for (cpu = 1; cpu < num_cpus; cpu++)
>> +             reset_cmplx_set_enable(cpu, mask, 1);
>> +     reset_cmplx_set_enable(0, mask, reset);

Thanks for the thorough review,

Tom