Re: [PATCH 3/3] dt-bindings: i2c: realtek,rtl9301-i2c: extend for RTL9310 support

[Krzysztof Kozlowski <krzk@kernel.org>]

On 02/07/2025 09:34, Jonas Jelonek wrote:
> Hi Krzysztof,
> 
> thanks for taking the time to deal with this.
> 
> On 02.07.2025 08:11, Krzysztof Kozlowski wrote:
>> On 01/07/2025 16:31, Jonas Jelonek wrote:
>>> For the purpose:
>>> RTL9310 changed the register layout compared to RTL9300. Activating
>>> the SCL line is done by setting bit 12 for master 1 and bit 13 for master 2
>>> in a global register which is located before the master-specific registers.
>>> Thus, the driver needs to know which master (1 or 2) it is currently doing
>>> something for. That is what this property is intended to be used, naming
>>> to-be-discussed.
>> There is no global register space here and no syscon, so I don't
>> understand how can you access it, especially when they are located
>> BEFORE your address space.
> 
> Probably this explanation is still missing some background and 'global'
> was misleading here. The I2C controllers are part of Realtek switchcore.
> This is defined as a syscon in DTS and the I2C controller has to be
> defined as a child-node of it. (see 
> https://elixir.bootlin.com/linux/v6.15.1/source/arch/mips/boot/dts/realtek/rtl930x.dtsi#L45-L72)
> The driver takes its regmap from this syscon node as the I2C registers
> are within that switchcore address space.
> 
> Address layout in RTL9310 is (addresses relative to parent syscon node):
> 0x1004 - 0x100b    I2C 'global' registers
> 0x100c - 0x1023    I2C master 1 registers
> 0x1024 - 0x103b    I2C master 2 registers
> 
> The driver has to access both the registers for the master that it is
> configuring AND the 'global' ones (for SCL + SDA activation).
> 
> For upstream RTL9300 it's similar with the difference of having SCL
> selection in the master-specific registers and the register layout
> having the order:
> - I2C master 1 registers
> - I2C 'global' register
> - I2C master 2 registers

I think I will keep bookmark this talk because this is perfect example
of writing bindings rule: they supposed to be complete.

If people sent complete bindings, they would see that you are now in
tricky spot and this maybe has to be redone to standard approach - I2C
is not a child of syscon block, but separate device. When it is a
separate device we solve it (plenty of examples) with phandle to syscon
with offset or value argument.

But no! Some incomplete hardware description was sent, stuffing
everything into syscon and claiming that everything is child of syscon,
and now you are stuck with this:

system controller
        |
        |
   i2c-controller-for-multiple-SDA
           |
           |
       i2c-controllers-for-each-SDA

This is not only just confusing but maybe even not correct.

I understand that either i2c controller can take any SCL line. If so,
that could be a pinctrl, but again this is child of that device, so
pinctrl to parent would be odd.

Vendor (not generic) property seems the only solution, but then this
should not be part of the existing binding or you should clearly narrow
this per variant. It was made very clear that rtl9301-family has only
one SCL per controller and you cannot choose.

> 
>> Best regards,
>> Krzysztof
> 
> Just an idea to discuss regarding the DT compatibles:
> I was proposed out of this conversation to use something like
> 'i2c-otto-mango' ('otto' being the codename for their platform,
> 'mango' being the codename for the RTL9310 series). Based on, that
> technically(!) it doesn't make sense to distinguish between the
> variants of the series.

I don't understand what does it refer to, but anyway, soc compatibles
are the only preferred way.


Best regards,
Krzysztof