Re: [PATCH v1 1/2] riscv: Kconfig: Allow RV32 to build with no MMU

[Jesse Taube <mr.bossman075@gmail.com>]

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
>