RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

Ryan Chen
2025-03-25T09:53:30Z

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
>
> On 24/03/2025 11:01, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 24/03/2025 09:30, Ryan Chen wrote:
> >>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 19/03/2025 12:12, Ryan Chen wrote:
> >>>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support
> >>>>>> for
> >>>>>> AST2600-i2cv2
> >>>>>>
> >>>>>> On 17/03/2025 10:21, Ryan Chen wrote:
> >>>>>>>> Neither this.
> >>>>>>>>
> >>>>>>>> So it seems you describe already existing and documented I2C,
> >>>>>>>> but for some reason you want second compatible. The problem is
> >>>>>>>> that you do not provide reason from the point of view of bindings.
> >>>>>>>>
> >>>>>>>> To summarize: what your users want - don't care. Start properly
> >>>>>>>> describing hardware and your SoC.
> >>>>>>>
> >>>>>>> OK, for ast2600 i2c controller have two register mode setting.
> >>>>>>> One, I call it is old register setting, that is right now
> >>>>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there
> >>>>>>> have a global register
> >>>>>> that can set i2c controller as new mode register set.
> >>>>>>> That I am going to drive. That I post is all register in new an
> >>>>>>> old register
> >>>> list.
> >>>>>>>
> >>>>>>> For example,
> >>>>>>> Global register [2] = 0 => i2c present as old register set
> >>>>>>> Global register [2] = 1 => i2c present as new register set
> >>>>>> It's the same device though, so the same compatible.
> >>>>>
> >>>>> Sorry, it is different design, and it share the same register space.
> >>>>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for
> >>>>> this
> >>>> driver.
> >>>>> It is different register layout.
> >>>>
> >>>> Which device is described by the existing "aspeed,ast2600-i2c-bus"
> >>>> compatible? And which device is described by new compatible?
> >>>>
> >>> On the AST2600 SoC, there are up to 16 I2C controller instances
> >>> (I2C1 ~
> >> I2C16).
> >>
> >> So you have 16 same devices.
> > Yes, one driver instance for 16 i2c device.
> >>
> >>> Each of these controllers is hardwired at the SoC level to use
> >>> either the
> >> legacy register layout or the new v2 register layout.
> >>> The mode is selected by a bit in the global register, these
> >>> represent two
> >> different hardware blocks:
> >>> "aspeed,ast2600-i2c-bus" describes controllers using the legacy
> >>> register
> >> layout.
> >>> "aspeed,ast2600-i2cv2" describes controllers using the new register
> >>> layout
> >>
> >> Which part of "same device" is not clear? You have one device, one
> compatible.
> >> Whatever you do with register layout, is already defined by that
> >> compatible. It does not matter that you forgot to implement it in the Linux
> kernel.
> >
> > Sorry, I still can't catch your point.
>
>
> I repeated twice "You have one device, one compatible.", provided few more
> sentences and if this is still unclear, I don't know what to say more.
>
> > I separated the support into two drivers:
>
> I don't care about your drivers, why are you bringing them here?
>
> > i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
> > i2c-ast2600.c for the new register set, compatible compatible
> "aspeed,ast2600-i2cv2"
> > Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be
> separate two driver?
>
> What is this patch about? Bindings. Not drivers. Look at email month ago:
>
> "All this describes driver, not hardware."
>
> Why are you keep bringing here drivers since a month?
Let me try to rephrase the reasoning from a hardware point of view:

On AST2600, each I2C controller instance is functionally the same type of device (I2C controller),
but it can present two different and incompatible register layouts,
depending on a hardware-controlled configuration (via global register bits that are fixed in production SoCs).
The layouts are not backward-compatible:
Registers are at different offsets. Bit definitions differ.
The programming sequence is not the same.
Because of this incompatibility at the register level, software cannot handle both layouts in a generic way without explicit knowledge of which version is present.
That’s why I proposed a new compatible string — to represent a hardware-visible difference in register interface, even though the logical function (I2C controller) is the same.

>
> Best regards,
> Krzysztof