Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9dabe979-f6b5-329d-f017-a8f0c00adeca@baylibre.com>
Date:   Wed, 10 Aug 2022 16:40:04 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] spi: meson-spicc: save pow2 datarate between messages
Content-Language: en-US
To:     Mark Brown <broonie@kernel.org>
Cc:     linux-spi@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org,
        Da Xue <da@libre.computer>
References: <20220809152019.461741-1-narmstrong@baylibre.com>
 <YvJ84qkuXdvVgXRm@sirena.org.uk>
 <39c2f53b-8f53-ceb1-ae0c-81e5e53d01aa@baylibre.com>
 <YvOmnDJA+ov49chC@sirena.org.uk>
 <518f22f4-1582-924c-9eaa-28ebbe53a632@baylibre.com>
 <YvPBX7zJ72RXnrpk@sirena.org.uk>
From:   Neil Armstrong <narmstrong@baylibre.com>
Organization: Baylibre
In-Reply-To: <YvPBX7zJ72RXnrpk@sirena.org.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/08/2022 16:31, Mark Brown wrote:
> On Wed, Aug 10, 2022 at 04:01:33PM +0200, Neil Armstrong wrote:
>> On 10/08/2022 14:37, Mark Brown wrote:
> 
>>> Sure, but that doesn't really address the concern - is this something
>>> that the clk driver programmed or is this the driver forgetting to
>>> restore a register that it programmed itself?  The commit message sounds
>>> like the former which is a much bigger problem.
> 
>> It's what is programmed by the Clock Framework yes, it was designed as-is
>> so the Clock Framework takes the most accurate clock path but the reset case
>> wasn't taken in account.
> 
> This seems like a bad idea, we shouldn't have two different drivers
> managing the same register without explicit and visible coordination
> with each other, this is at best asking for trouble as you've found
> here.  I've not looked in detail but I think if you want to use the
> clock framework here then this driver should register a clock provider
> for the clock hardware in the IP block.

I totally understand, this wasn't explicit until I found the bug.

I don't think it's worth adding so much code for this since we already
had an open-coded function which perfectly worked before.

> 
> How does this work with runtime PM, what happens if the clock driver
> decides to change something while the device is powered down?

There's no runtime PM implemented, and yes it would be an issue.


I'm perfectly OK to remove the CCF driver for the legacy clock path
and return back to the old open coded calculation since it perfectly
worked and stop using the legacy clock path for new SoCs since it would
never be selected anyway...
... but GX SoCs are broken so it would need an intermediate fix until
I push the refactoring to cleanup all this.

Neil