Messages in this thread | | | Date | Fri, 20 Jan 2023 15:51:55 -0500 | Subject | Re: [PATCH v1 1/2] riscv: Kconfig: Allow RV32 to build with no MMU | From | Jesse Taube <> |
| |
On 1/20/23 15:48, Conor Dooley wrote: > On Fri, Jan 20, 2023 at 08:44:10PM +0000, Conor Dooley wrote: >> On Fri, Jan 20, 2023 at 12:39:06PM -0500, Jesse Taube wrote: >>> On 1/20/23 02:59, Conor Dooley wrote: >>>> Since you'll have to re-submit, making sure that allowing !MMU on rv32 >>>> doesn't break the build due to canaan k210 drivers being enabled despite >>>> relying on 64-bit divisions, I've got some nits for you. >>> Not sure what driver needs 64bit, but sense !MMU was only selected by 64BIT. >> >> LKP reported a build error for it: >> https://lore.kernel.org/linux-riscv/202301201538.zNlqgE4L-lkp@intel.com/ >> >>> This should work. >>> diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs >>> index 69774bb362d6..b9835b8ede86 100644 >>> --- a/arch/riscv/Kconfig.socs >>> +++ b/arch/riscv/Kconfig.socs >>> @@ -43,7 +43,7 @@ config SOC_VIRT >>> >>> config SOC_CANAAN >>> bool "Canaan Kendryte K210 SoC" >>> - depends on !MMU >>> + depends on !MMU && 64BIT >>> select CLINT_TIMER if RISCV_M_MODE >>> select SERIAL_SIFIVE if TTY >>> select SERIAL_SIFIVE_CONSOLE if TTY >> >> I don't think this is the correct fix for the problem - the drivers >> really should not do implicit 64-bit divisions IMO. >> Linux has division helpers for them in math64.h. >> None of the other SoCs have a dependency on 64BIT and I'd not been keen >> on adding on here. >> >> I suspect the fix is as simple as the below, but I'd need to go test it. >> >> Thanks, >> Conor. >> >> --- 8< --- >> From ecfa79ad1b24f68cfccb77d666e443293d52d066 Mon Sep 17 00:00:00 2001 >> From: Conor Dooley <conor.dooley@microchip.com> >> Date: Fri, 20 Jan 2023 20:36:29 +0000 >> Subject: [PATCH] clk: k210: remove an implicit 64-bit division >> >> The K210 clock driver depends on SOC_CANAAN, which is only selectable >> when !MMU on RISC-V. !MMU is not possible on 32-bit yet, but patches >> have been sent for its enabling. The kernel test robot reported this >> implicit 64-bit division there. Oh I missed the bots email oops.
undefined reference to `__udivdi3' Poor linker isn't linking with libgcc
>> >> Replace the implicit division with an explicit one. >> >> Reported-by: kernel test robot <lkp@intel.com> >> Link: https://lore.kernel.org/linux-riscv/202301201538.zNlqgE4L-lkp@intel.com/ >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >> --- >> Since it was always guarded such that it only ever built for 64-bit, I >> am not sure that a fixes tag is needed, but it would be: >> Fixes: c6ca7616f7d5 ("clk: Add RISC-V Canaan Kendryte K210 clock driver") >> --- >> drivers/clk/clk-k210.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/clk/clk-k210.c b/drivers/clk/clk-k210.c >> index 67a7cb3503c3..17c5bfb384ad 100644 >> --- a/drivers/clk/clk-k210.c >> +++ b/drivers/clk/clk-k210.c >> @@ -495,7 +495,7 @@ static unsigned long k210_pll_get_rate(struct clk_hw *hw, >> f = FIELD_GET(K210_PLL_CLKF, reg) + 1; >> od = FIELD_GET(K210_PLL_CLKOD, reg) + 1; >> >> - return (u64)parent_rate * f / (r * od); >> + return div_u64(parent_rate * f, r * od); > > Nope, that's wrong. I omitted the cast... > > return div_u64((u64)parent_rate * f, r * od); Ah that's a much better fix, shall I prepend this to the set and author you?
> >> } >> >> static const struct clk_ops k210_pll_ops = { >> -- >> 2.39.0 >> > > > >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv >
| |