On Sat, Jan 14, 2017 at 9:14 AM, Simon Glass <s...@chromium.org> wrote:
> Hi Maxim,
>
> On 4 January 2017 at 12:46, Maxim Sloyko <max...@google.com> wrote:
> > This driver is ast2500 specific and is not compatible with earlier
>
> ast2500-specific
>
> > versions of this chip. The differences are not that large, but they are
> > in somewhat random places, so making it compatible with ast2400 is not
> > worth the effort at the moment.
> >
> > Signed-off-by: Maxim Sloyko <max...@google.com>
> > ---
> >
> > arch/arm/include/asm/arch-aspeed/scu_ast2500.h | 108 +++++++++++
> > drivers/clk/Makefile | 2 +
> > drivers/clk/aspeed/Makefile | 7 +
> > drivers/clk/aspeed/clk_ast2500.c | 255
> +++++++++++++++++++++++++
> > 4 files changed, 372 insertions(+)
> > create mode 100644 arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> > create mode 100644 drivers/clk/aspeed/Makefile
> > create mode 100644 drivers/clk/aspeed/clk_ast2500.c
> >
> > diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> > new file mode 100644
> > index 0000000000..febff9d2d3
> > --- /dev/null
> > +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> > @@ -0,0 +1,108 @@
> > +#ifndef _ASM_ARCH_SCU_AST2500_H
> > +#define _ASM_ARCH_SCU_AST2500_H
> > +
> > +#define SCU_UNLOCK_VALUE 0x1688a8a8
> > +
> > +#define SCU_HWSTRAP_VGAMEM_MASK 3
> > +#define SCU_HWSTRAP_VGAMEM_SHIFT 2
> > +#define SCU_HWSTRAP_DDR4 (1 << 24)
> > +#define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)
> > +
> > +#define SCU_MPLL_DENUM_SHIFT 0
> > +#define SCU_MPLL_DENUM_MASK 0x1f
> > +#define SCU_MPLL_NUM_SHIFT 5
> > +#define SCU_MPLL_NUM_MASK 0xff
> > +#define SCU_MPLL_POST_SHIFT 13
> > +#define SCU_MPLL_POST_MASK 0x3f
> > +
> > +#define SCU_HPLL_DENUM_SHIFT 0
> > +#define SCU_HPLL_DENUM_MASK 0x1f
> > +#define SCU_HPLL_NUM_SHIFT 5
> > +#define SCU_HPLL_NUM_MASK 0xff
> > +#define SCU_HPLL_POST_SHIFT 13
> > +#define SCU_HPLL_POST_MASK 0x3f
> > +
> > +#define SCU_MISC2_UARTCLK_SHIFT 24
> > +
> > +#define SCU_MISC_UARTCLK_DIV13 (1 << 12)
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +struct ast2500_clk_priv {
> > + struct ast2500_scu *scu;
> > +};
> > +
> > +struct ast2500_scu {
> > + u32 protection_key;
> > + u32 sysreset_ctrl1;
> > + u32 clk_sel1;
> > + u32 clk_stop_ctrl1;
> > + u32 freq_counter_ctrl;
> > + u32 freq_counter_cmp;
> > + u32 intr_ctrl;
> > + u32 d2_pll_param;
> > + u32 m_pll_param;
> > + u32 h_pll_param;
> > + u32 d_pll_param;
> > + u32 misc_ctrl1;
> > + u32 pci_config[3];
> > + u32 sysreset_status;
> > + u32 vga_handshake[2];
> > + u32 mac_clk_delay;
> > + u32 misc_ctrl2;
> > + u32 vga_scratch[8];
> > + u32 hwstrap;
> > + u32 rng_ctrl;
> > + u32 rng_data;
> > + u32 rev_id;
> > + u32 pinmux_ctrl[6];
> > + u32 reserved0;
> > + u32 extrst_sel;
> > + u32 pinmux_ctrl1[4];
> > + u32 reserved1[2];
> > + u32 mac_clk_delay_100M;
> > + u32 mac_clk_delay_10M;
> > + u32 wakeup_enable;
> > + u32 wakeup_control;
> > + u32 reserved2[3];
> > + u32 sysreset_ctrl2;
> > + u32 clk_sel2;
> > + u32 clk_stop_ctrl2;
> > + u32 freerun_counter;
> > + u32 freerun_counter_ext;
> > + u32 clk_duty_meas_ctrl;
> > + u32 clk_duty_meas_res;
> > + u32 reserved3[4];
> > + /* The next registers are not key protected */
>
> key-protected
>
Fixed.
>
> > + struct ast2500_cpu2 {
> > + u32 ctrl;
> > + u32 base_addr[9];
> > + u32 cache_ctrl;
> > + } cpu2;
> > + u32 reserved4;
> > + u32 d_pll_ext_param[3];
> > + u32 d2_pll_ext_param[3];
> > + u32 mh_pll_ext_param;
> > + u32 reserved5;
> > + u32 chip_id[2];
> > + u32 reserved6[2];
> > + u32 uart_clk_ctrl;
> > + u32 reserved7[7];
> > + u32 pcie_config;
> > + u32 mmio_decode;
> > + u32 reloc_ctrl_decode[2];
> > + u32 mailbox_addr;
> > + u32 shared_sram_decode[2];
> > + u32 bmc_rev_id;
> > + u32 reserved8;
> > + u32 bmc_device_id;
> > + u32 reserved9[13];
> > + u32 clk_duty_sel;
> > +};
> > +
> > +int ast_get_clk(struct udevice **devp);
> > +void *ast_get_scu(void);
>
> Function comments please.
>
Fixed.
>
> > +
> > +#endif /* __ASSEMBLY__ */
> > +
> > +#endif /* _ASM_ARCH_SCU_AST2500_H */
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index 40a5e8cae8..625513789c 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -16,3 +16,5 @@ obj-$(CONFIG_CLK_UNIPHIER) += uniphier/
> > obj-$(CONFIG_CLK_EXYNOS) += exynos/
> > obj-$(CONFIG_CLK_AT91) += at91/
> > obj-$(CONFIG_CLK_BOSTON) += clk_boston.o
> > +
> > +obj-$(CONFIG_ARCH_ASPEED) += aspeed/
> > diff --git a/drivers/clk/aspeed/Makefile b/drivers/clk/aspeed/Makefile
> > new file mode 100644
> > index 0000000000..65d1cd6e29
> > --- /dev/null
> > +++ b/drivers/clk/aspeed/Makefile
> > @@ -0,0 +1,7 @@
> > +#
> > +# Copyright (c) 2016 Google, Inc
> > +#
> > +# SPDX-License-Identifier: GPL-2.0+
> > +#
> > +
> > +obj-$(CONFIG_ASPEED_AST2500) += clk_ast2500.o
> > diff --git a/drivers/clk/aspeed/clk_ast2500.c b/drivers/clk/aspeed/clk_
> ast2500.c
> > new file mode 100644
> > index 0000000000..c888a6d35b
> > --- /dev/null
> > +++ b/drivers/clk/aspeed/clk_ast2500.c
> > @@ -0,0 +1,255 @@
> > +/*
> > + * (C) Copyright 2016 Google, Inc
> > + *
> > + * SPDX-License-Identifier: GPL-2.0
> > + */
> > +
> > +#include <common.h>
> > +#include <asm/arch/scu_ast2500.h>
> > +#include <asm/io.h>
> > +#include <clk-uclass.h>
> > +#include <dm.h>
> > +#include <dm/lists.h>
> > +#include <dt-bindings/clock/ast2500-scu.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +/*
> > + * For H-PLL and M-PLL the formula is
> > + * (Output Frequency) = CLKIN * ((M + 1) / (N + 1)) / (P + 1)
> > + * M - Numerator
> > + * N - Denumerator
> > + * P - Post Divider
> > + * They have the same layout in their control register.
> > + */
> > +
> > +/*
> > + * Get the rate of the M-PLL clock from input clock frequency and
> > + * the value of the M-PLL Parameter Register.
> > + */
> > +static ulong ast2500_get_mpll_rate(ulong clkin, u32 mpll_reg)
> > +{
> > + const ulong num = (mpll_reg >> SCU_MPLL_NUM_SHIFT) &
> SCU_MPLL_NUM_MASK;
> > + const ulong denum = (mpll_reg >> SCU_MPLL_DENUM_SHIFT)
> > + & SCU_MPLL_DENUM_MASK;
> > + const ulong post_div = (mpll_reg >> SCU_MPLL_POST_SHIFT)
> > + & SCU_MPLL_POST_MASK;
> > +
> > + return (clkin * ((num + 1) / (denum + 1))) / post_div;
> > +}
> > +
> > +/*
> > + * Get the rate of the H-PLL clock from input clock frequency and
> > + * the value of the H-PLL Parameter Register.
> > + */
> > +static ulong ast2500_get_hpll_rate(ulong clkin, u32 hpll_reg)
> > +{
> > + const ulong num = (hpll_reg >> SCU_HPLL_NUM_SHIFT) &
> SCU_HPLL_NUM_MASK;
> > + const ulong denum = (hpll_reg >> SCU_HPLL_DENUM_SHIFT)
> > + & SCU_HPLL_DENUM_MASK;
> > + const ulong post_div = (hpll_reg >> SCU_HPLL_POST_SHIFT)
> > + & SCU_HPLL_POST_MASK;
> > +
> > + return (clkin * ((num + 1) / (denum + 1))) / post_div;
> > +}
> > +
> > +static ulong ast2500_get_clkin(struct ast2500_scu *scu)
> > +{
> > + return readl(&scu->hwstrap) & SCU_HWSTRAP_CLKIN_25MHZ
> > + ? 25*1000*1000 : 24*1000*1000;
>
> Spaces around *
>
Fixed.
>
> > +}
> > +
> > +static ulong ast2500_get_uart_clk_rate(struct ast2500_scu *scu, int
> uart)
>
> Function comment. What is 'uart' for?
>
Fixed. Renamed uart to uart_index to be more clear about its purpose.
>
> > +{
> > + /*
> > + * ast2500 datasheet is very confusing when it comes to UART
> clocks,
> > + * especially when CLKIN = 25 MHz. The settings are in
> > + * different registers and it is unclear how they interact.
> > + *
> > + * This has only been tested with default settings and CLKIN =
> 24 MHz.
>
> Can you detect this and return -EINVAL if it can't work? If not, that's
> fine.
>
I can detect if CLKIN = 24 MHz or 25 MHz, but it's not clear what UART
input clock rate is for CLKIN = 25MHz
There is a setting (on by default) to generate UART 24MHz clock from 25MHz,
but there are also somewhat
conflicting settings for dividers.
I don't have access to 25MHz based hardware to test this at the moment, all
our boards use 24MHz crystal.
>
> > + */
> > + ulong uart_clkin;
> > +
> > + if (readl(&scu->misc_ctrl2) & (1 << (uart +
> SCU_MISC2_UARTCLK_SHIFT)))
> > + uart_clkin = 192 * 1000 * 1000;
> > + else
> > + uart_clkin = 24 * 1000 * 1000;
> > +
> > + if (readl(&scu->misc_ctrl1) & SCU_MISC_UARTCLK_DIV13)
> > + uart_clkin /= 13;
> > +
> > + return uart_clkin;
> > +}
> > +
> > +static ulong ast2500_clk_get_rate(struct clk *clk)
> > +{
> > + struct ast2500_clk_priv *priv = dev_get_priv(clk->dev);
> > + ulong clkin = ast2500_get_clkin(priv->scu);
> > + ulong rate;
> > +
> > + switch (clk->id) {
> > + case PLL_HPLL:
> > + case ARMCLK:
> > + /*
> > + * This ignores dynamic/static slowdown of ARMCLK and may
> > + * be inaccurate.
> > + */
> > + rate = ast2500_get_hpll_rate(clkin,
> > +
> readl(&priv->scu->h_pll_param));
> > + break;
> > + case MCLK_DDR:
> > + rate = ast2500_get_mpll_rate(clkin,
> > +
> readl(&priv->scu->m_pll_param));
> > + break;
> > + case PCLK_UART1:
> > + rate = ast2500_get_uart_clk_rate(priv->scu, 1);
> > + break;
> > + case PCLK_UART2:
> > + rate = ast2500_get_uart_clk_rate(priv->scu, 2);
> > + break;
> > + case PCLK_UART3:
> > + rate = ast2500_get_uart_clk_rate(priv->scu, 3);
> > + break;
> > + case PCLK_UART4:
> > + rate = ast2500_get_uart_clk_rate(priv->scu, 4);
> > + break;
> > + case PCLK_UART5:
> > + rate = ast2500_get_uart_clk_rate(priv->scu, 5);
> > + break;
> > + default:
> > + return -ENOENT;
> > + }
> > +
> > + return rate;
> > +}
> > +
> > +static void ast2500_scu_unlock(struct ast2500_scu *scu)
> > +{
> > + writel(SCU_UNLOCK_VALUE, &scu->protection_key);
> > + while (!readl(&scu->protection_key))
> > + ;
>
> Can this ever timeout?
>
Not according to the datasheet.
>
> > +}
> > +
> > +static void ast2500_scu_lock(struct ast2500_scu *scu)
> > +{
> > + writel(~SCU_UNLOCK_VALUE, &scu->protection_key);
> > + while (readl(&scu->protection_key))
> > + ;
> > +}
> > +
> > +static ulong ast2500_configure_ddr(struct ast2500_scu *scu, ulong rate)
> > +{
> > + ulong clkin = ast2500_get_clkin(scu);
> > + u32 mpll_reg;
> > +
> > + /*
> > + * There are not that many combinations of numerator, denumerator
> > + * and post divider, so just brute force the best combination.
> > + * However, to avoid overflow when multiplying, use kHz.
> > + */
> > + const ulong clkin_khz = clkin / 1000;
> > + const ulong rate_khz = rate / 1000;
> > +
>
> drop blank line
>
> > + ulong best_num = 0;
> > + ulong best_denum = 0;
> > + ulong best_post = 0;
> > + ulong delta = rate;
> > +
>
> drop blank line
>
> > + ulong num, denum, post;
>
> add blank line
>
> > + for (denum = 0; denum <= SCU_MPLL_DENUM_MASK; ++denum) {
> > + for (post = 0; post <= SCU_MPLL_POST_MASK; ++post) {
> > + num = (rate_khz * (post + 1) / clkin_khz) *
> (denum + 1);
> > + ulong new_rate_khz = (clkin_khz
> > + * ((num + 1) / (denum +
> 1)))
> > + / (post + 1);
> > +
> > + /* Keep the rate below requested one. */
> > + if (new_rate_khz > rate_khz)
> > + continue;
> > +
> > + if (new_rate_khz - rate_khz < delta) {
> > + delta = new_rate_khz - rate_khz;
> > +
> > + best_num = num;
> > + best_denum = denum;
> > + best_post = post;
> > +
> > + if (delta == 0)
> > + goto rate_calc_done;
> > + }
> > + }
> > + }
> > +
> > + rate_calc_done:
>
> Drop blank line
>
> Can it fail? Do you need to check delta?
>
I just check delta for an early exit, if we found a match.
It returns the best approximation for target rate we could achieve and it
can fail
in a sense that this approximation might not be good enough. In which case
DDR init will fail --
I tried to play with the configured rate during development and that's how
failure manifested itself.
However, I don't know any way of verifying the rate in advance.
>
> > +
> > + mpll_reg = readl(&scu->m_pll_param);
> > + mpll_reg &= ~((SCU_MPLL_POST_MASK << SCU_MPLL_POST_SHIFT)
> > + | (SCU_MPLL_NUM_MASK << SCU_MPLL_NUM_SHIFT)
> > + | (SCU_MPLL_DENUM_MASK << SCU_MPLL_DENUM_SHIFT));
> > + mpll_reg |= (best_post << SCU_MPLL_POST_SHIFT)
> > + | (best_num << SCU_MPLL_NUM_SHIFT)
> > + | (best_denum << SCU_MPLL_DENUM_SHIFT);
> > +
> > + ast2500_scu_unlock(scu);
> > + writel(mpll_reg, &scu->m_pll_param);
> > + ast2500_scu_lock(scu);
> > +
> > + return ast2500_get_mpll_rate(clkin, mpll_reg);
> > +}
> > +
> > +static ulong ast2500_clk_set_rate(struct clk *clk, ulong rate)
> > +{
> > + struct ast2500_clk_priv *priv = dev_get_priv(clk->dev);
> > +
> > + ulong new_rate;
> > + switch (clk->id) {
> > + case PLL_MPLL:
> > + case MCLK_DDR:
> > + new_rate = ast2500_configure_ddr(priv->scu, rate);
> > + break;
> > + default:
> > + return -ENOENT;
> > + }
> > +
> > + return new_rate;
> > +}
> > +
> > +struct clk_ops ast2500_clk_ops = {
> > + .get_rate = ast2500_clk_get_rate,
> > + .set_rate = ast2500_clk_set_rate,
> > +};
> > +
> > +static int ast2500_clk_probe(struct udevice *dev)
> > +{
> > + struct ast2500_clk_priv *priv = dev_get_priv(dev);
>
> blank line
>
Done.
>
> > + priv->scu = (struct ast2500_scu *)dev_get_addr(dev);
>
> Maybe dev_get_addr_ptr()
>
> Also should check that it is not FDT_ADDR_NONE.
>
Done.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int ast2500_clk_bind(struct udevice *dev)
> > +{
> > + int ret;
> > +
> > + /* The reset driver does not have a device node, so bind it here
> */
> > + ret = device_bind_driver(gd->dm_root, "ast_sysreset", "reset",
> &dev);
> > + if (ret)
> > + debug("Warning: No reset driver: ret=%d\n", ret);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct udevice_id ast2500_clk_ids[] = {
> > + { .compatible = "aspeed,ast2500-scu" },
> > + { }
> > +};
> > +
> > +U_BOOT_DRIVER(aspeed_ast2500_scu) = {
> > + .name = "aspeed_ast2500_scu",
> > + .id = UCLASS_CLK,
> > + .of_match = ast2500_clk_ids,
> > + .priv_auto_alloc_size = sizeof(struct ast2500_clk_priv),
> > + .ops = &ast2500_clk_ops,
> > + .bind = ast2500_clk_bind,
> > + .probe = ast2500_clk_probe,
> > +};
> > --
> > 2.11.0.390.gc69c2f50cf-goog
> >
>
> Regards,
> Simon
>
--
*M*axim *S*loyko
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
