Message-ID: <201b31cb-ef17-4e18-9a4e-ff4193d06afb@denx.de>
Date: Fri, 23 Aug 2024 04:46:58 +0200
From: Marek Vasut <marex@...x.de>
To: Alexis Lothoré <alexis.lothore@...tlin.com>,
 linux-wireless@...r.kernel.org
Cc: Ajay Singh <ajay.kathat@...rochip.com>,
 "David S. Miller" <davem@...emloft.net>,
 Adham Abozaeid <adham.abozaeid@...rochip.com>,
 Claudiu Beznea <claudiu.beznea@...on.dev>, Conor Dooley
 <conor+dt@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Kalle Valo <kvalo@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org,
 netdev@...r.kernel.org, Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 2/2] wifi: wilc1000: Add WILC3000 support

On 8/22/24 2:10 PM, Alexis Lothoré wrote:
> Hello Marek,

Hi,

> I was coincidentally working on adding wilc3000 support upstream too.

I hope you weren't too far along with that and I didn't waste too much 
of your time/effort here.

> My work is
> also based on downstream tree, so my comments will likely reflect the reworks I
> was doing or intended to do.
> For the record, I have some wilc1000 and wilc3000 modules, in both  sdio and spi
> setups.

Nice, I only have this WILC3000 SDIO device .

> On 8/21/24 20:42, Marek Vasut wrote:
>> From: Ajay Singh <ajay.kathat@...rochip.com>
> 
> [...]
> 
>>   	if (!resume) {
>> -		ret = wilc_sdio_read_reg(wilc, WILC_CHIPID, &chipid);
>> -		if (ret) {
>> -			dev_err(&func->dev, "Fail cmd read chip id...\n");
>> +		chipid = wilc_get_chipid(wilc, true);
>> +		if (is_wilc3000(chipid)) {
>> +			wilc->chip = WILC_3000;
>> +		} else if (is_wilc1000(chipid)) {
>> +			wilc->chip = WILC_1000;
>> +		} else {
>> +			dev_err(&func->dev, "Unsupported chipid: %x\n", chipid);
>>   			return ret;
>>   		}
> 
> I wonder if this additional enum (enum wilc_chip_type)  is really useful. We
> already store the raw chipid, which just needs to be masked to know about the
> device type. We should likely store one or the other but not both, otherwise we
> may just risk to create desync without really saving useful info.
> 
> Also, this change makes wilc1000-sdio failing to build as module (missing symbol
> export on wilc_get_chipid)

I think I have a separate patch for this, one which folds 
wilc_get_chipid() entirely into wlan.c , and then follow up which uses 
is_wilc1000() / is_wilc3000() all over the place to discern the two MACs 
based on cached chip ID . That should work, I'll test it and submit it 
later today I hope.

> [...]
> 
>> -	/* select VMM table 0 */
>> -	if (val & SEL_VMM_TBL0)
>> -		reg |= BIT(5);
>> -	/* select VMM table 1 */
>> -	if (val & SEL_VMM_TBL1)
>> -		reg |= BIT(6);
>> -	/* enable VMM */
>> -	if (val & EN_VMM)
>> -		reg |= BIT(7);
>> +	if (wilc->chip == WILC_1000) {
> 
> wilc1000 should likely remain the default/fallback ?

I am now validating whether the hardware is either wilc1000 or wilc3000 
up front based on the chip ID early in init, so no other option can 
occur here, so there is no need for fallback, it is either wilc1000 or 
wilc3000 now (*). I think keeping them ordered alphanumerically is the 
nicer option.

> [...]
> 
>> @@ -1232,10 +1234,7 @@ static int wilc_validate_chipid(struct wilc *wilc)
>>   		dev_err(&spi->dev, "Fail cmd read chip id...\n");
>>   		return ret;
>>   	}
>> -	if (!is_wilc1000(chipid)) {
>> -		dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid);
>> -		return -ENODEV;
>> -	}
>> +
> 
> Instead of dropping any filtering (and then making the function name become
> irrelevant), why not ensuring that it is at least either a wilc1000 or a wilc3000 ?

Right, done.

[...]

>> +void chip_wakeup(struct wilc *wilc)
>> +{
>> +	if (wilc->chip == WILC_1000)
>> +		chip_wakeup_wilc1000(wilc);
>> +	else
>> +		chip_wakeup_wilc3000(wilc);
>> +}
>>   EXPORT_SYMBOL_GPL(chip_wakeup);
> 
> This new support makes a few places in wlan.c, netdev.c and in bus files
> (sdio.c, spi.c) install (sometimes big) branches on the device type (chip init,
> sleep, wakeup, read interrupt, clear interrupt, txq handling, etc), because the
> registers are different, the masks are different, the number of involved
> registers may not be the same, wilc3000 may need more operations to perform the
> same thing... I feel like it will make it harder in the long run to maintain the
> driver, especially if some new variants are added later.

I agree the code is ugly. Looking at the roadmap, it seems the next 
thing is WILCS02 which has its own driver, and for the WILC1000/3000 
inherited from atmel this seems to be the end of the road.

> Those branches tend to
> show that some operations in those files are too specific to the targeted
> device. I was examining the possibility to start creating device-type specific
> files (wilc1000.c, wilc3000.c) and move those operations as "device-specific"
> ops. Then wlan/netdev would call those chip-specific ops, which in turn may call
> the hif_func ops. It may need some rework in the bus files to fit this new
> hierarchy, but it may allow to keep netdev and wlan unaware of the device type,
> and since wilc3000 has bluetooth, it may also make it easier to introduce the
> corresponding support later. What do you think about it ? Ajay, any opinion on
> this ?

I did something like that for KSZ8851, that had bus-specific ops. I 
vaguely recall there was feedback that the function pointer indirection 
had non-trivial overhead due to spectre mitigations, and in case of the 
handle_txq() here, the chip specific ops would be called in a while() {} 
loop.

I can imagine some of the long functions like wilc_sdio_clear_int_ext or 
the handle_txq could be split up a bit, but likely only by factoring out 
some of the code into static functions. But looking at this closer, both 
pieces which are wilc1000/3000 specific in those functions manipulate 
with variables which would have to be passed in into that factored out 
code as function arguments, so I am not sure if this would improve 
readability by much either.

[...]

Powered by blists - more mailing lists