mbox series

[v1,0/7] Add rtc refclk support for PolarFire SoC

Message ID 20220408143646.3693104-1-conor.dooley@microchip.com
Headers show
Series Add rtc refclk support for PolarFire SoC | expand

Message

Conor Dooley April 8, 2022, 2:36 p.m. UTC
Hey,
As I mentioned in my fixes for 5.18 [0], found out that the reference
clock for the rtc is actually missing from the clock driver (and the
dt binding). 

Currently the mpfs clock driver uses a reference clock called the
"msspll", set in the device tree, as the parent for the cpu/axi/ahb
(config) clocks. The frequency of the msspll is determined by the FPGA
bitstream & the bootloader configures the clock to match the bitstream.
The real reference is provided by a 100 or 125 MHz off chip oscillator.

However, the msspll clock is not actually the parent of all clocks on
the system - the reference clock for the rtc/mtimer actually has the
off chip oscillator as its parent.

This series enables reading the rate of the msspll clock, converts
the refclock in the device tree to the external reference & adds
the missing rtc reference clock.

I assume it is okay not to add fixes tags for the rtc dt binding?
Since the clock was previously missing, the binding is wrong, but
idk if that qualifies as a fix?

Clock driver changes depend on the fixes I sent in [0].
Please lmk if you want me to respin into a single series w/ the fixes.
Thanks,
Conor.

[0]: https://lore.kernel.org/linux-riscv/20220408133543.3537118-1-conor.dooley@microchip.com/

Conor Dooley (7):
  dt-bindings: clk: mpfs document msspll dri registers
  dt-bindings: clk: mpfs: add defines for two new clocks
  dt-bindings: rtc: add refclk to mpfs-rtc
  clk: microchip: mpfs: re-parent the configurable clocks
  clk: microchip: mpfs: rename sys_base to base
  clk: microchip: mpfs: add RTCREF clock control
  riscv: dts: microchip: reparent mpfs clocks

 .../bindings/clock/microchip,mpfs.yaml        |  11 +-
 .../bindings/rtc/microchip,mfps-rtc.yaml      |  14 +-
 .../microchip/microchip-mpfs-icicle-kit.dts   |   2 +-
 .../boot/dts/microchip/microchip-mpfs.dtsi    |   8 +-
 drivers/clk/microchip/clk-mpfs.c              | 205 +++++++++++++++---
 .../dt-bindings/clock/microchip,mpfs-clock.h  |   5 +-
 6 files changed, 199 insertions(+), 46 deletions(-)

Comments

Krzysztof Kozlowski April 8, 2022, 2:54 p.m. UTC | #1
On 08/04/2022 16:36, Conor Dooley wrote:
> As there are two sections of registers that are responsible for clock
> configuration on the PolarFire SoC: add the dynamic reconfiguration
> interface section to the binding & describe what each of the sections
> are used for.

(...)

>  
>    reg:
> -    maxItems: 1
> +    items:
> +      - description: |
> +          clock config registers:
> +          These registers contain enable, reset & divider tables for the, cpu, axi, ahb and
> +          rtc/mtimer reference clocks as well as enable and reset for the peripheral clocks.
> +      - description: |
> +          mss pll dri registers:
> +          Block of registers responsible for dynamic reconfiguration of the mss pll
>  

This breaks all of DTS - in and out of tree.

Best regards,
Krzysztof
Krzysztof Kozlowski April 8, 2022, 2:57 p.m. UTC | #2
On 08/04/2022 16:36, Conor Dooley wrote:
> Hey,
> As I mentioned in my fixes for 5.18 [0], found out that the reference
> clock for the rtc is actually missing from the clock driver (and the
> dt binding). 
> 
> Currently the mpfs clock driver uses a reference clock called the
> "msspll", set in the device tree, as the parent for the cpu/axi/ahb
> (config) clocks. The frequency of the msspll is determined by the FPGA
> bitstream & the bootloader configures the clock to match the bitstream.
> The real reference is provided by a 100 or 125 MHz off chip oscillator.
> 
> However, the msspll clock is not actually the parent of all clocks on
> the system - the reference clock for the rtc/mtimer actually has the
> off chip oscillator as its parent.
> 
> This series enables reading the rate of the msspll clock, converts
> the refclock in the device tree to the external reference & adds
> the missing rtc reference clock.
> 
> I assume it is okay not to add fixes tags for the rtc dt binding?
> Since the clock was previously missing, the binding is wrong, but
> idk if that qualifies as a fix?

Usually ABI breakage, even if accepted, should be be tagged as fix
because it is clearly then a break of other peoples' trees...

Best regards,
Krzysztof
Krzysztof Kozlowski April 9, 2022, 10:45 a.m. UTC | #3
On 08/04/2022 17:29, Conor Dooley wrote:
> 
> 
> On 08/04/2022 15:57, Krzysztof Kozlowski wrote:
>> On 08/04/2022 16:36, Conor Dooley wrote:
>>> Hey,
>>> As I mentioned in my fixes for 5.18 [0], found out that the reference
>>> clock for the rtc is actually missing from the clock driver (and the
>>> dt binding).
>>>
>>> Currently the mpfs clock driver uses a reference clock called the
>>> "msspll", set in the device tree, as the parent for the cpu/axi/ahb
>>> (config) clocks. The frequency of the msspll is determined by the FPGA
>>> bitstream & the bootloader configures the clock to match the bitstream.
>>> The real reference is provided by a 100 or 125 MHz off chip oscillator.
>>>
>>> However, the msspll clock is not actually the parent of all clocks on
>>> the system - the reference clock for the rtc/mtimer actually has the
>>> off chip oscillator as its parent.
>>>
>>> This series enables reading the rate of the msspll clock, converts
>>> the refclock in the device tree to the external reference & adds
>>> the missing rtc reference clock.
>>>
>>> I assume it is okay not to add fixes tags for the rtc dt binding?
>>> Since the clock was previously missing, the binding is wrong, but
>>> idk if that qualifies as a fix?
>>
>> Usually ABI breakage, even if accepted, should be be tagged as fix
>> because it is clearly then a break of other peoples' trees...
>>
> 
> That means either a) do something messy in the clock driver or b) mark
> the whole series as fixes (and roll it into [0]).
> 
> The second option seems far more sensible to me, do you agree?

I think ate part of my sentence... it should be:
"Usually ABI breakage, even if accepted, should NOT be tagged as fix..."

So usually it should not be a fix.

The binding actually could be backported, because the driver changes
bring the real ABI breakage.

Best regards,
Krzysztof
Krzysztof Kozlowski April 9, 2022, 10:48 a.m. UTC | #4
On 09/04/2022 09:14, Conor Dooley wrote:
> 
> 
> On 08/04/2022 16:29, Conor Dooley wrote:
>>
>>
>> On 08/04/2022 15:57, Krzysztof Kozlowski wrote:
>>> On 08/04/2022 16:36, Conor Dooley wrote:
>>>> Hey,
>>>> As I mentioned in my fixes for 5.18 [0], found out that the reference
>>>> clock for the rtc is actually missing from the clock driver (and the
>>>> dt binding).
>>>>
>>>> Currently the mpfs clock driver uses a reference clock called the
>>>> "msspll", set in the device tree, as the parent for the cpu/axi/ahb
>>>> (config) clocks. The frequency of the msspll is determined by the FPGA
>>>> bitstream & the bootloader configures the clock to match the bitstream.
>>>> The real reference is provided by a 100 or 125 MHz off chip oscillator.
>>>>
>>>> However, the msspll clock is not actually the parent of all clocks on
>>>> the system - the reference clock for the rtc/mtimer actually has the
>>>> off chip oscillator as its parent.
>>>>
>>>> This series enables reading the rate of the msspll clock, converts
>>>> the refclock in the device tree to the external reference & adds
>>>> the missing rtc reference clock.
>>>>
>>>> I assume it is okay not to add fixes tags for the rtc dt binding?
>>>> Since the clock was previously missing, the binding is wrong, but
>>>> idk if that qualifies as a fix?
>>>
>>> Usually ABI breakage, even if accepted, should be be tagged as fix
>>> because it is clearly then a break of other peoples' trees...
>>>
>>
>> That means either a) do something messy in the clock driver or b) mark
>> the whole series as fixes (and roll it into [0]).
>>
>> The second option seems far more sensible to me, do you agree?
> 
> Having thought some more about it, patches 2, 3 and the rtc part of 7
> should be moved into [0] since they're fixing a binding that only
> arrived in 5.18-rc1.
> For the rest, make the second part of the reg optional and if it doesnt
> exist just return prate for the msspll clock?

Ah, so this got into v5.18-rc1? I think I missed that information from
the patches description and focused on backporting to stables. Then
indeed you could combine all fixes together, mark them with Fixes.

Best regards,
Krzysztof
Conor Dooley April 9, 2022, 8:17 p.m. UTC | #5
On 09/04/2022 11:48, Krzysztof Kozlowski wrote:
> On 09/04/2022 09:14, Conor Dooley wrote:
>> On 08/04/2022 16:29, Conor Dooley wrote:
>>> On 08/04/2022 15:57, Krzysztof Kozlowski wrote:
>>>> On 08/04/2022 16:36, Conor Dooley wrote:
>>>>> Hey,
>>>>> As I mentioned in my fixes for 5.18 [0], found out that the reference
>>>>> clock for the rtc is actually missing from the clock driver (and the
>>>>> dt binding).
>>>>>
>>>>> Currently the mpfs clock driver uses a reference clock called the
>>>>> "msspll", set in the device tree, as the parent for the cpu/axi/ahb
>>>>> (config) clocks. The frequency of the msspll is determined by the FPGA
>>>>> bitstream & the bootloader configures the clock to match the bitstream.
>>>>> The real reference is provided by a 100 or 125 MHz off chip oscillator.
>>>>>
>>>>> However, the msspll clock is not actually the parent of all clocks on
>>>>> the system - the reference clock for the rtc/mtimer actually has the
>>>>> off chip oscillator as its parent.
>>>>>
>>>>> This series enables reading the rate of the msspll clock, converts
>>>>> the refclock in the device tree to the external reference & adds
>>>>> the missing rtc reference clock.
>>>>>
>>>>> I assume it is okay not to add fixes tags for the rtc dt binding?
>>>>> Since the clock was previously missing, the binding is wrong, but
>>>>> idk if that qualifies as a fix?
>>>>
>>>> Usually ABI breakage, even if accepted, should be be tagged as fix
>>>> because it is clearly then a break of other peoples' trees...
>>>>
>>>
>>> That means either a) do something messy in the clock driver or b) mark
>>> the whole series as fixes (and roll it into [0]).
>>>
>>> The second option seems far more sensible to me, do you agree?
>>
>> Having thought some more about it, patches 2, 3 and the rtc part of 7
>> should be moved into [0] since they're fixing a binding that only
>> arrived in 5.18-rc1.
>> For the rest, make the second part of the reg optional and if it doesnt
>> exist just return prate for the msspll clock?
> 
> Ah, so this got into v5.18-rc1?

Yeah, so for context he clock driver & relevant the dt-bindings only
arrived in 5.18-rc1. The device tree itself has been around (I think)
5.12 but it wasn't bootable until now. The rtc stanza is also new.
The clock stanza & "wrong" refclk existed since the device tree was
first added.

> I think I missed that information from
> the patches description and focused on backporting to stables.

Yeah, zero intention of backporting any of this. Pretty pointless since
the board hasn't booted until now anyway.

> Then indeed you could combine all fixes together, mark them with Fixes.

I had split the series (plural) since the clock driver change is fairly
big, adding a new "layer" of clocks & like 200 lines to a 400 line
driver. I will respin then, with the dt binding patches marked as fixes
and combined with the other series.

Do I have to maintain backwards compatibility with the device tree
from before the board actually booted mainline? If not, I'll merge
it all into one series, marked as fixes. Otherwise I'll keep the clock 
changes in this series out of the fixes, mark the second reg entry in
the clock binding as optional & handle the old, "naive" dt stanza in the
driver.

Thanks!
Conor.