Message-ID:
 <OS8PR06MB7541FD8691B43EA33BDC1D22F2A72@OS8PR06MB7541.apcprd06.prod.outlook.com>
Date: Tue, 25 Mar 2025 09:52:04 +0000
From: Ryan Chen <ryan_chen@...eedtech.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: "benh@...nel.crashing.org" <benh@...nel.crashing.org>, "joel@....id.au"
	<joel@....id.au>, "andi.shyti@...nel.org" <andi.shyti@...nel.org>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"andrew@...econstruct.com.au" <andrew@...econstruct.com.au>,
	"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
	<linux-aspeed@...ts.ozlabs.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Mo Elbadry <elbadrym@...gle.com>
Subject: RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
 AST2600-i2cv2

> 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

Powered by blists - more mailing lists