* [PATCH v2 0/1] hw/arm/aspeed: Initialize AST2600 UART clock selection registers @ 2021-09-06 13:40 pdel 2021-09-06 13:40 ` [PATCH v2 1/1] " pdel 0 siblings, 1 reply; 5+ messages in thread From: pdel @ 2021-09-06 13:40 UTC (permalink / raw) Cc: clg, joel, qemu-devel, qemu-arm, f4bug, Peter Delevoryas From: Peter Delevoryas <pdel@fb.com> After fixing some commit log issues in another patch: https://lore.kernel.org/qemu-devel/2F2F44C6-4817-4D58-A7A0-496446AC77AF@fb.com/ I noticed that I had similar issues in another patch I submitted: https://lore.kernel.org/qemu-devel/20210831142502.279485-1-pdel@fb.com/ So I'm resubmitting this patch with the URL in the commit description changed to a permalink using a commit hash and removing the test instructions that I had at the end. Thanks, Peter Peter Delevoryas (1): hw/arm/aspeed: Initialize AST2600 UART clock selection registers hw/misc/aspeed_scu.c | 4 ++++ 1 file changed, 4 insertions(+) -- 2.30.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/1] hw/arm/aspeed: Initialize AST2600 UART clock selection registers 2021-09-06 13:40 [PATCH v2 0/1] hw/arm/aspeed: Initialize AST2600 UART clock selection registers pdel @ 2021-09-06 13:40 ` pdel 2021-09-07 8:24 ` Joel Stanley 0 siblings, 1 reply; 5+ messages in thread From: pdel @ 2021-09-06 13:40 UTC (permalink / raw) Cc: clg, joel, qemu-devel, qemu-arm, f4bug, Peter Delevoryas From: Peter Delevoryas <pdel@fb.com> UART5 is typically used as the default debug UART on the AST2600, but UART1 is also designed to be a debug UART. All the AST2600 UART's have semi-configurable clock rates through registers in the System Control Unit (SCU), but only UART5 works out of the box with zero-initialized values. The rest of the UART's expect a few of the registers to be initialized to non-zero values, or else the clock rate calculation will yield zero or undefined (due to a divide-by-zero). For reference, the U-Boot clock rate driver here shows the calculation: https://github.com/facebook/openbmc-uboot/blob/15f7e0dc01d8/drivers/clk/aspeed/clk_ast2600.c#L357 To summarize, UART5 allows selection from 4 rates: 24 MHz, 192 MHz, 24 / 13 MHz, and 192 / 13 MHz. The other UART's allow selecting either the "low" rate (UARTCLK) or the "high" rate (HUARTCLK). UARTCLK and HUARTCLK are configurable themselves: UARTCLK = UXCLK * R / (N * 2) HUARTCLK = HUXCLK * HR / (HN * 2) UXCLK and HUXCLK are also configurable, and depend on the APLL and/or HPLL clock rates, which also derive from complicated calculations. Long story short, there's lots of multiplication and division from configurable registers, and most of these registers are zero-initialized in QEMU, which at best is unexpected and at worst causes this clock rate driver to hang from divide-by-zero's. This can also be difficult to diagnose, because it may cause U-Boot to hang before serial console initialization completes, requiring intervention from gdb. This change just initializes all of these registers with default values from the datasheet. To test this, I used Facebook's AST2600 OpenBMC image for "fuji", with the following diff applied (because fuji uses UART1 for console output, not UART5). @@ -323,8 +323,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) } /* UART - attach an 8250 to the IO space as our UART5 */ - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2, - aspeed_soc_get_irq(s, ASPEED_DEV_UART5), + serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART1], 2, + aspeed_soc_get_irq(s, ASPEED_DEV_UART1), 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN); /* I2C */ Without these clock rate registers being initialized, U-Boot hangs in the clock rate driver from a divide-by-zero, because the UART1 clock rate register reads return zero, and there's no console output. After initializing them with default values, fuji boots successfully. Signed-off-by: Peter Delevoryas <pdel@fb.com> --- hw/misc/aspeed_scu.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c index 05edebedeb..a95dca65f2 100644 --- a/hw/misc/aspeed_scu.c +++ b/hw/misc/aspeed_scu.c @@ -119,6 +119,8 @@ #define AST2600_CLK_SEL3 TO_REG(0x308) #define AST2600_CLK_SEL4 TO_REG(0x310) #define AST2600_CLK_SEL5 TO_REG(0x314) +#define AST2600_UARTCLK_PARAM TO_REG(0x338) +#define AST2600_HUARTCLK_PARAM TO_REG(0x33C) #define AST2600_HW_STRAP1 TO_REG(0x500) #define AST2600_HW_STRAP1_CLR TO_REG(0x504) #define AST2600_HW_STRAP1_PROT TO_REG(0x508) @@ -681,6 +683,8 @@ static const uint32_t ast2600_a3_resets[ASPEED_AST2600_SCU_NR_REGS] = { [AST2600_CLK_SEL3] = 0x00000000, [AST2600_CLK_SEL4] = 0xF3F40000, [AST2600_CLK_SEL5] = 0x30000000, + [AST2600_UARTCLK_PARAM] = 0x00014506, + [AST2600_HUARTCLK_PARAM] = 0x000145C0, [AST2600_CHIP_ID0] = 0x1234ABCD, [AST2600_CHIP_ID1] = 0x88884444, }; -- 2.30.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] hw/arm/aspeed: Initialize AST2600 UART clock selection registers 2021-09-06 13:40 ` [PATCH v2 1/1] " pdel @ 2021-09-07 8:24 ` Joel Stanley 2021-09-07 13:37 ` Peter Delevoryas 0 siblings, 1 reply; 5+ messages in thread From: Joel Stanley @ 2021-09-07 8:24 UTC (permalink / raw) To: pdel Cc: Philippe Mathieu-Daudé, qemu-arm, Cédric Le Goater, QEMU Developers On Mon, 6 Sept 2021 at 13:40, <pdel@fb.com> wrote: > > From: Peter Delevoryas <pdel@fb.com> > > UART5 is typically used as the default debug UART on the AST2600, but > UART1 is also designed to be a debug UART. All the AST2600 UART's have > semi-configurable clock rates through registers in the System Control > Unit (SCU), but only UART5 works out of the box with zero-initialized > values. The rest of the UART's expect a few of the registers to be > initialized to non-zero values, or else the clock rate calculation will > yield zero or undefined (due to a divide-by-zero). > > For reference, the U-Boot clock rate driver here shows the calculation: > > https://github.com/facebook/openbmc-uboot/blob/15f7e0dc01d8/drivers/clk/aspeed/clk_ast2600.c#L357 > > To summarize, UART5 allows selection from 4 rates: 24 MHz, 192 MHz, 24 / > 13 MHz, and 192 / 13 MHz. The other UART's allow selecting either the > "low" rate (UARTCLK) or the "high" rate (HUARTCLK). UARTCLK and HUARTCLK > are configurable themselves: > > UARTCLK = UXCLK * R / (N * 2) > HUARTCLK = HUXCLK * HR / (HN * 2) > > UXCLK and HUXCLK are also configurable, and depend on the APLL and/or > HPLL clock rates, which also derive from complicated calculations. Long > story short, there's lots of multiplication and division from > configurable registers, and most of these registers are zero-initialized > in QEMU, which at best is unexpected and at worst causes this clock rate > driver to hang from divide-by-zero's. This can also be difficult to > diagnose, because it may cause U-Boot to hang before serial console > initialization completes, requiring intervention from gdb. > > This change just initializes all of these registers with default values > from the datasheet. > > To test this, I used Facebook's AST2600 OpenBMC image for "fuji", with > the following diff applied (because fuji uses UART1 for console output, > not UART5). > > @@ -323,8 +323,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) > } > > /* UART - attach an 8250 to the IO space as our UART5 */ > - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2, > - aspeed_soc_get_irq(s, ASPEED_DEV_UART5), > + serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART1], 2, > + aspeed_soc_get_irq(s, ASPEED_DEV_UART1), > 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN); > > /* I2C */ > > Without these clock rate registers being initialized, U-Boot hangs in > the clock rate driver from a divide-by-zero, because the UART1 clock > rate register reads return zero, and there's no console output. After > initializing them with default values, fuji boots successfully. > > Signed-off-by: Peter Delevoryas <pdel@fb.com> > --- > hw/misc/aspeed_scu.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c > index 05edebedeb..a95dca65f2 100644 > --- a/hw/misc/aspeed_scu.c > +++ b/hw/misc/aspeed_scu.c > @@ -119,6 +119,8 @@ > #define AST2600_CLK_SEL3 TO_REG(0x308) > #define AST2600_CLK_SEL4 TO_REG(0x310) > #define AST2600_CLK_SEL5 TO_REG(0x314) > +#define AST2600_UARTCLK_PARAM TO_REG(0x338) > +#define AST2600_HUARTCLK_PARAM TO_REG(0x33C) It would be consistent with the existing naming if we dropped the _PARAM. Not worth re-spinning just for that though. > #define AST2600_HW_STRAP1 TO_REG(0x500) > #define AST2600_HW_STRAP1_CLR TO_REG(0x504) > #define AST2600_HW_STRAP1_PROT TO_REG(0x508) > @@ -681,6 +683,8 @@ static const uint32_t ast2600_a3_resets[ASPEED_AST2600_SCU_NR_REGS] = { > [AST2600_CLK_SEL3] = 0x00000000, > [AST2600_CLK_SEL4] = 0xF3F40000, > [AST2600_CLK_SEL5] = 0x30000000, > + [AST2600_UARTCLK_PARAM] = 0x00014506, > + [AST2600_HUARTCLK_PARAM] = 0x000145C0, These match v9 of the datasheet, so they look good to me. Reviewed-by: Joel Stanley <joel@jms.id.au> > [AST2600_CHIP_ID0] = 0x1234ABCD, > [AST2600_CHIP_ID1] = 0x88884444, > }; > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] hw/arm/aspeed: Initialize AST2600 UART clock selection registers 2021-09-07 8:24 ` Joel Stanley @ 2021-09-07 13:37 ` Peter Delevoryas 2021-09-09 10:25 ` Cédric Le Goater 0 siblings, 1 reply; 5+ messages in thread From: Peter Delevoryas @ 2021-09-07 13:37 UTC (permalink / raw) To: Joel Stanley Cc: Cédric Le Goater, QEMU Developers, qemu-arm, Philippe Mathieu-Daudé > On Sep 7, 2021, at 1:24 AM, Joel Stanley <joel@jms.id.au> wrote: > > On Mon, 6 Sept 2021 at 13:40, <pdel@fb.com> wrote: >> >> From: Peter Delevoryas <pdel@fb.com> >> >> UART5 is typically used as the default debug UART on the AST2600, but >> UART1 is also designed to be a debug UART. All the AST2600 UART's have >> semi-configurable clock rates through registers in the System Control >> Unit (SCU), but only UART5 works out of the box with zero-initialized >> values. The rest of the UART's expect a few of the registers to be >> initialized to non-zero values, or else the clock rate calculation will >> yield zero or undefined (due to a divide-by-zero). >> >> For reference, the U-Boot clock rate driver here shows the calculation: >> >> https://github.com/facebook/openbmc-uboot/blob/15f7e0dc01d8/drivers/clk/aspeed/clk_ast2600.c#L357 >> >> To summarize, UART5 allows selection from 4 rates: 24 MHz, 192 MHz, 24 / >> 13 MHz, and 192 / 13 MHz. The other UART's allow selecting either the >> "low" rate (UARTCLK) or the "high" rate (HUARTCLK). UARTCLK and HUARTCLK >> are configurable themselves: >> >> UARTCLK = UXCLK * R / (N * 2) >> HUARTCLK = HUXCLK * HR / (HN * 2) >> >> UXCLK and HUXCLK are also configurable, and depend on the APLL and/or >> HPLL clock rates, which also derive from complicated calculations. Long >> story short, there's lots of multiplication and division from >> configurable registers, and most of these registers are zero-initialized >> in QEMU, which at best is unexpected and at worst causes this clock rate >> driver to hang from divide-by-zero's. This can also be difficult to >> diagnose, because it may cause U-Boot to hang before serial console >> initialization completes, requiring intervention from gdb. >> >> This change just initializes all of these registers with default values >> from the datasheet. >> >> To test this, I used Facebook's AST2600 OpenBMC image for "fuji", with >> the following diff applied (because fuji uses UART1 for console output, >> not UART5). >> >> @@ -323,8 +323,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >> } >> >> /* UART - attach an 8250 to the IO space as our UART5 */ >> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2, >> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5), >> + serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART1], 2, >> + aspeed_soc_get_irq(s, ASPEED_DEV_UART1), >> 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN); >> >> /* I2C */ >> >> Without these clock rate registers being initialized, U-Boot hangs in >> the clock rate driver from a divide-by-zero, because the UART1 clock >> rate register reads return zero, and there's no console output. After >> initializing them with default values, fuji boots successfully. >> >> Signed-off-by: Peter Delevoryas <pdel@fb.com> >> --- >> hw/misc/aspeed_scu.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c >> index 05edebedeb..a95dca65f2 100644 >> --- a/hw/misc/aspeed_scu.c >> +++ b/hw/misc/aspeed_scu.c >> @@ -119,6 +119,8 @@ >> #define AST2600_CLK_SEL3 TO_REG(0x308) >> #define AST2600_CLK_SEL4 TO_REG(0x310) >> #define AST2600_CLK_SEL5 TO_REG(0x314) >> +#define AST2600_UARTCLK_PARAM TO_REG(0x338) >> +#define AST2600_HUARTCLK_PARAM TO_REG(0x33C) > > It would be consistent with the existing naming if we dropped the > _PARAM. Not worth re-spinning just for that though. Yeah sorry about that, I wasn’t sure what name to use here. The datasheet names “Generate UARTCLK from UXCLK” and “Generate HUARTCLK from HUXCLK” seemed too long and didn’t match anything else. I thought these registers were kind of similar to the HPLL_PARAM register, just for UARTCLK and HUARTCLK. If there are any further revisions I can change it though. > >> #define AST2600_HW_STRAP1 TO_REG(0x500) >> #define AST2600_HW_STRAP1_CLR TO_REG(0x504) >> #define AST2600_HW_STRAP1_PROT TO_REG(0x508) >> @@ -681,6 +683,8 @@ static const uint32_t ast2600_a3_resets[ASPEED_AST2600_SCU_NR_REGS] = { >> [AST2600_CLK_SEL3] = 0x00000000, >> [AST2600_CLK_SEL4] = 0xF3F40000, >> [AST2600_CLK_SEL5] = 0x30000000, >> + [AST2600_UARTCLK_PARAM] = 0x00014506, >> + [AST2600_HUARTCLK_PARAM] = 0x000145C0, > > These match v9 of the datasheet, so they look good to me. > > Reviewed-by: Joel Stanley <joel@jms.id.au> Thanks! Peter > >> [AST2600_CHIP_ID0] = 0x1234ABCD, >> [AST2600_CHIP_ID1] = 0x88884444, >> }; >> -- >> 2.30.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] hw/arm/aspeed: Initialize AST2600 UART clock selection registers 2021-09-07 13:37 ` Peter Delevoryas @ 2021-09-09 10:25 ` Cédric Le Goater 0 siblings, 0 replies; 5+ messages in thread From: Cédric Le Goater @ 2021-09-09 10:25 UTC (permalink / raw) To: Peter Delevoryas, Joel Stanley Cc: qemu-arm, QEMU Developers, Philippe Mathieu-Daudé On 9/7/21 3:37 PM, Peter Delevoryas wrote: > > >> On Sep 7, 2021, at 1:24 AM, Joel Stanley <joel@jms.id.au> wrote: >> >> On Mon, 6 Sept 2021 at 13:40, <pdel@fb.com> wrote: >>> >>> From: Peter Delevoryas <pdel@fb.com> >>> >>> UART5 is typically used as the default debug UART on the AST2600, but >>> UART1 is also designed to be a debug UART. All the AST2600 UART's have >>> semi-configurable clock rates through registers in the System Control >>> Unit (SCU), but only UART5 works out of the box with zero-initialized >>> values. The rest of the UART's expect a few of the registers to be >>> initialized to non-zero values, or else the clock rate calculation will >>> yield zero or undefined (due to a divide-by-zero). >>> >>> For reference, the U-Boot clock rate driver here shows the calculation: >>> >>> https://github.com/facebook/openbmc-uboot/blob/15f7e0dc01d8/drivers/clk/aspeed/clk_ast2600.c#L357 >>> >>> To summarize, UART5 allows selection from 4 rates: 24 MHz, 192 MHz, 24 / >>> 13 MHz, and 192 / 13 MHz. The other UART's allow selecting either the >>> "low" rate (UARTCLK) or the "high" rate (HUARTCLK). UARTCLK and HUARTCLK >>> are configurable themselves: >>> >>> UARTCLK = UXCLK * R / (N * 2) >>> HUARTCLK = HUXCLK * HR / (HN * 2) >>> >>> UXCLK and HUXCLK are also configurable, and depend on the APLL and/or >>> HPLL clock rates, which also derive from complicated calculations. Long >>> story short, there's lots of multiplication and division from >>> configurable registers, and most of these registers are zero-initialized >>> in QEMU, which at best is unexpected and at worst causes this clock rate >>> driver to hang from divide-by-zero's. This can also be difficult to >>> diagnose, because it may cause U-Boot to hang before serial console >>> initialization completes, requiring intervention from gdb. >>> >>> This change just initializes all of these registers with default values >>> from the datasheet. >>> >>> To test this, I used Facebook's AST2600 OpenBMC image for "fuji", with >>> the following diff applied (because fuji uses UART1 for console output, >>> not UART5). >>> >>> @@ -323,8 +323,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) >>> } >>> >>> /* UART - attach an 8250 to the IO space as our UART5 */ >>> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2, >>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5), >>> + serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART1], 2, >>> + aspeed_soc_get_irq(s, ASPEED_DEV_UART1), >>> 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN); >>> >>> /* I2C */ >>> >>> Without these clock rate registers being initialized, U-Boot hangs in >>> the clock rate driver from a divide-by-zero, because the UART1 clock >>> rate register reads return zero, and there's no console output. After >>> initializing them with default values, fuji boots successfully. >>> >>> Signed-off-by: Peter Delevoryas <pdel@fb.com> >>> --- >>> hw/misc/aspeed_scu.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c >>> index 05edebedeb..a95dca65f2 100644 >>> --- a/hw/misc/aspeed_scu.c >>> +++ b/hw/misc/aspeed_scu.c >>> @@ -119,6 +119,8 @@ >>> #define AST2600_CLK_SEL3 TO_REG(0x308) >>> #define AST2600_CLK_SEL4 TO_REG(0x310) >>> #define AST2600_CLK_SEL5 TO_REG(0x314) >>> +#define AST2600_UARTCLK_PARAM TO_REG(0x338) >>> +#define AST2600_HUARTCLK_PARAM TO_REG(0x33C) >> >> It would be consistent with the existing naming if we dropped the >> _PARAM. Not worth re-spinning just for that though. > > Yeah sorry about that, I wasn’t sure what name to use here. The > datasheet names “Generate UARTCLK from UXCLK” and “Generate HUARTCLK > from HUXCLK” seemed too long and didn’t match anything else. I thought > these registers were kind of similar to the HPLL_PARAM register, just > for UARTCLK and HUARTCLK. If there are any further revisions I can > change it though. I will fix that. Thanks, C. > >> >>> #define AST2600_HW_STRAP1 TO_REG(0x500) >>> #define AST2600_HW_STRAP1_CLR TO_REG(0x504) >>> #define AST2600_HW_STRAP1_PROT TO_REG(0x508) >>> @@ -681,6 +683,8 @@ static const uint32_t ast2600_a3_resets[ASPEED_AST2600_SCU_NR_REGS] = { >>> [AST2600_CLK_SEL3] = 0x00000000, >>> [AST2600_CLK_SEL4] = 0xF3F40000, >>> [AST2600_CLK_SEL5] = 0x30000000, >>> + [AST2600_UARTCLK_PARAM] = 0x00014506, >>> + [AST2600_HUARTCLK_PARAM] = 0x000145C0, >> >> These match v9 of the datasheet, so they look good to me. >> >> Reviewed-by: Joel Stanley <joel@jms.id.au> > > Thanks! > Peter > >> >>> [AST2600_CHIP_ID0] = 0x1234ABCD, >>> [AST2600_CHIP_ID1] = 0x88884444, >>> }; >>> -- >>> 2.30.2 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-09 10:27 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-09-06 13:40 [PATCH v2 0/1] hw/arm/aspeed: Initialize AST2600 UART clock selection registers pdel 2021-09-06 13:40 ` [PATCH v2 1/1] " pdel 2021-09-07 8:24 ` Joel Stanley 2021-09-07 13:37 ` Peter Delevoryas 2021-09-09 10:25 ` Cédric Le Goater
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).