Re: [RFC PATCH 09/16] soundwire: crc8: add constant table

[Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>]

On 12/21/23 15:42, Vinod Koul wrote:
> On 18-12-23, 14:26, Pierre-Louis Bossart wrote:
>>
>>
>> On 12/18/23 06:01, Vinod Koul wrote:
>>> On 07-12-23, 16:29, Pierre-Louis Bossart wrote:
>>>> Add the lookup table required by crc8(). All configuration values were
>>>> directly table from the MIPI SoundWire 1.x specification.
>>>>
>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
>>>> ---
>>>>  drivers/soundwire/Makefile |   4 +-
>>>>  drivers/soundwire/crc8.c   | 277 +++++++++++++++++++++++++++++++++++++
>>>>  drivers/soundwire/crc8.h   |  11 ++
>>>>  3 files changed, 291 insertions(+), 1 deletion(-)
>>>>  create mode 100644 drivers/soundwire/crc8.c
>>>>  create mode 100644 drivers/soundwire/crc8.h
>>>>
>>>> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
>>>> index 657f5888a77b..170128dd9318 100644
>>>> --- a/drivers/soundwire/Makefile
>>>> +++ b/drivers/soundwire/Makefile
>>>> @@ -5,7 +5,9 @@
>>>>  
>>>>  #Bus Objs
>>>>  soundwire-bus-y := bus_type.o bus.o master.o slave.o mipi_disco.o stream.o  \
>>>> -			sysfs_slave.o sysfs_slave_dpn.o
>>>> +			sysfs_slave.o sysfs_slave_dpn.o \
>>>> +			crc8.o
>>>> +
>>>>  obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o
>>>>  
>>>>  soundwire-generic-allocation-objs := generic_bandwidth_allocation.o
>>>> diff --git a/drivers/soundwire/crc8.c b/drivers/soundwire/crc8.c
>>>> new file mode 100644
>>>> index 000000000000..b6b984d7f39a
>>>> --- /dev/null
>>>> +++ b/drivers/soundwire/crc8.c
>>>> @@ -0,0 +1,277 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
>>>> +// Copyright(c) 2024 Intel Corporation.
>>>> +
>>>> +#include <linux/crc8.h>
>>>> +#include <linux/module.h>
>>>> +#include "crc8.h"
>>>> +
>>>> +/*
>>>> + * the MIPI SoundWire CRC8 polynomial is X^8 + X^6 + X^3 + X^2 + 1, MSB first
>>>> + * The value is (1)01001101 = 0x4D
>>>> + *
>>>> + * the table below was generated with
>>>> + *
>>>> + *	u8 crc8_lookup_table[CRC8_TABLE_SIZE];
>>>> + *	crc8_populate_msb(crc8_lookup_table, SDW_CRC8_POLY);
>>>
>>> Good that you found this API, so next question would be why should we
>>> have this static table in kernel and not generate on probe if bpt is
>>> supported..? Many subsystems use these APIs to generate the tables..
>>
>> The table is going to be the same for all hosts, it's simpler if
>> everyone uses a constant table, no? We're talking about 256 bytes added
>> for the common bus parts, be it with dynamically allocated memory or a
>> static table.
>>
>> I don't mind reverting to a dynamically allocated table populated at
>> boot if that was the consensus.
> 
> Most of the kernel users I looked have dynamically allocated table
> populated at boot, also out of many users how many would support BTP.?
> Your older platforms, current qcom dont, so not point is adding for
> everyone...

All Intel hardware supports BTP/BRA, we just didn't have a compelling
reason to enable it so far. I've seen AMD stating that they also have
BTP/BRA. That's 2/3 of controllers.

I don't mind reverting to a devm_ allocated table, I am not sure I see
the benefits for 256 bytes of constant data.