Message-Id: <20231214193658.18969-1-alex.vinarskis@gmail.com>
Date: Thu, 14 Dec 2023 20:36:58 +0100
From: Aleksandrs Vinarskis <alex.vinarskis@...il.com>
To: tiwai@...e.de
Cc: alex.vinarskis@...il.com,
	alsa-devel@...a-project.org,
	david.rhodes@...rus.com,
	james.schulman@...rus.com,
	josbeir@...il.com,
	linux-kernel@...r.kernel.org,
	patches@...nsource.cirrus.com,
	perex@...ex.cz,
	sbinding@...nsource.cirrus.com,
	stuarth@...nsource.cirrus.com,
	tiwai@...e.com
Subject: Re: [PATCH 1/1] ALSA: hda: cs35l41: Dell Fiorano add missing _DSD properties

> Can I be awkward and ask that we hold off on this patch chain until
> then?  Then we can add this laptop using the new approach.

> If/when the chain is accepted, we will add support for a few Dell
> laptops as well, including this one.

Sounds reasonable. I'll be looking forward to your new framework.
Once up, I can adjust my patch, and if everything still works as expected,
push updated version for review.

> Instead of erroring out, I wonder if we can noodle our way to the
> appropriate clk and clk_set_rate it up to 4MHz for this particular
> laptop only?  Stefan's taking a look at that.
Thanks for the initiative. Potentially that would work, however, it would
require to go up the clock tree to the divider. Since its clearly a
firmware bug causing lpss miss-configuration, I intially thought it would
be best to have it resolved there. If you need more information, I would be
happy to share results of our debugging with you via private email.

> Also, any SPI rate >~100k is probably just about usable, so we don't
> want to error on <4MHz.  Quite often the spi clock is set at some value
> just below 4MHz.  It's unclear if this is going to get fixed in the BIOS
> at this point, so we don't know what exact rate we'd eventually receive.
I'm afraid I have to disagree here, 100k is _way_ too slow. Not sure
intentionally or not, but wake up from suspend is held back by Cirrus
driver. At 100k, I got these results, on boot:

```
[    5.561244] cs35l41-hda spi1-CSC3551:00-cs35l41-hda.0: Adding DSD properties for 10280BEB
..
[   11.251145] cs35l41-hda spi1-CSC3551:00-cs35l41-hda.1: CS35L41 Bound - SSID: 10280BEB, BST: 1, VSPK: 1, CH: R, FW EN: 1, SPKID: -19
```

And on wake-up from suspend:
```
[  307.162720] cs35l41-hda spi1-CSC3551:00-cs35l41-hda.0: DSP1: Firmware version: 3
...
[  312.515588] cs35l41-hda spi1-CSC3551:00-cs35l41-hda.1: 100000 Hz actual, DMA
```

This means ~5.5 additional seconds of black screen on wake up, in my opnion
this is completely unacceptable. With 4Mhz, it takes sub 1second. Moreover,
the first time (after preconfigured by ALSA delay) sound is played, it
seems it needs to communicate with amplifier, and it takes additional few
seconds to start playing at 100Khz. With 4Mhz, its practically instant.

I agree that it is unlikely for Dell to ever fix its firmware. Thus either
in case of intel-lpss patch, or via clk_set_rate direclty from Cirrus
driver, lpss divider would be set 1:1, SPI controller would receive 100Mhz
clock directly, and set it to whatever is requested internally (currently
4Mhz). This way, lowest 'usable' rate should be irrelevant.

> Quite often the spi clock is set at some value just below 4MHz
Perhaps to address this, we could error out on say half requested rate? In
reality, it will be either requested rate/just sub requested, or something
totally off, like the default 3051Hz in this case.

> This will break once you pick up AMD's multi-cs patches, we should use
> spi_set_csgpiod instead.
Thanks, will correct.

> I suppose the error-out was due to safety reasons, but the clock
> adjustment works, it should be fine.  Let's see.
Precisely, since it is indeed unknown when exaclty/if ever firmware will
be fixed, and/or when our patch to intel-lpss will be accepted.

Regards,
Alex

Powered by blists - more mailing lists