[PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Ryan Chen
ryan_chen at aspeedtech.com
Fri Mar 3 21:16:32 AEDT 2023
Hello Krzysztof,
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>
> Sent: Friday, March 3, 2023 5:26 PM
> To: Ryan Chen <ryan_chen at aspeedtech.com>; Wolfram Sang
> <wsa at kernel.org>
> Cc: Joel Stanley <joel at jms.id.au>; Brendan Higgins
> <brendan.higgins at linux.dev>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt at linaro.org>; Andrew Jeffery <andrew at aj.id.au>;
> devicetree at vger.kernel.org; Philipp Zabel <p.zabel at pengutronix.de>; Rob
> Herring <robh+dt at kernel.org>; Benjamin Herrenschmidt
> <benh at kernel.crashing.org>; linux-aspeed at lists.ozlabs.org;
> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
>
> On 03/03/2023 09:55, Ryan Chen wrote:
> > Hello Krzysztof,
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>
> >> Sent: Friday, March 3, 2023 4:51 PM
> >> To: Ryan Chen <ryan_chen at aspeedtech.com>; Wolfram Sang
> >> <wsa at kernel.org>
> >> Cc: Joel Stanley <joel at jms.id.au>; Brendan Higgins
> >> <brendan.higgins at linux.dev>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt at linaro.org>; Andrew Jeffery
> >> <andrew at aj.id.au>; devicetree at vger.kernel.org; Philipp Zabel
> >> <p.zabel at pengutronix.de>; Rob Herring <robh+dt at kernel.org>;
> Benjamin
> >> Herrenschmidt <benh at kernel.crashing.org>;
> >> linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
> >> linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org;
> >> linux-i2c at vger.kernel.org
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 03/03/2023 09:28, Ryan Chen wrote:
> >>> Hello Krzysztof,
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>
> >>>> Sent: Friday, March 3, 2023 4:20 PM
> >>>> To: Ryan Chen <ryan_chen at aspeedtech.com>; Wolfram Sang
> >>>> <wsa at kernel.org>
> >>>> Cc: Joel Stanley <joel at jms.id.au>; Brendan Higgins
> >>>> <brendan.higgins at linux.dev>; Krzysztof Kozlowski
> >>>> <krzysztof.kozlowski+dt at linaro.org>; Andrew Jeffery
> >>>> <andrew at aj.id.au>; devicetree at vger.kernel.org; Philipp Zabel
> >>>> <p.zabel at pengutronix.de>; Rob Herring <robh+dt at kernel.org>;
> >>>> Benjamin Herrenschmidt <benh at kernel.crashing.org>;
> >>>> linux-aspeed at lists.ozlabs.org;
> >>>> linux-arm-kernel at lists.infradead.org;
> >>>> linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org;
> >>>> linux-i2c at vger.kernel.org
> >>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 01/03/2023 06:57, Ryan Chen wrote:
> >>>>> Hello Krzysztof,
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>
> >>>>>> Sent: Monday, February 27, 2023 4:25 PM
> >>>>>> To: Ryan Chen <ryan_chen at aspeedtech.com>; Andrew Jeffery
> >>>>>> <andrew at aj.id.au>; Brendan Higgins <brendan.higgins at linux.dev>;
> >>>>>> Benjamin Herrenschmidt <benh at kernel.crashing.org>; Joel Stanley
> >>>>>> <joel at jms.id.au>; Rob Herring <robh+dt at kernel.org>; Krzysztof
> >>>>>> Kozlowski <krzysztof.kozlowski+dt at linaro.org>; Philipp Zabel
> >>>>>> <p.zabel at pengutronix.de>; linux-i2c at vger.kernel.org;
> >>>>>> openbmc at lists.ozlabs.org; devicetree at vger.kernel.org;
> >>>>>> linux-arm-kernel at lists.infradead.org;
> >>>>>> linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
> >>>>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >>>>>> AST2600-i2cv2
> >>>>>>
> >>>>>> On 26/02/2023 04:13, Ryan Chen wrote:
> >>>>>>> Add ast2600-i2cv2 compatible and aspeed,global-regs,
> >>>>>>> aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
> >>>>>>>
> >>>>>>> Signed-off-by: Ryan Chen <ryan_chen at aspeedtech.com>
> >>>>>>> ---
> >>>>>>> .../devicetree/bindings/i2c/aspeed,i2c.yaml | 44
> >>>> +++++++++++++++++++
> >>>>>>> 1 file changed, 44 insertions(+)
> >>>>>>>
> >>>>>>> diff --git
> >>>>>>> a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>>>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>>>> index f597f73ccd87..75de3ce41cf5 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>>>> @@ -49,6 +49,25 @@ properties:
> >>>>>>> description:
> >>>>>>> states that there is another master active on this bus
> >>>>>>>
> >>>>>>> + aspeed,timeout:
> >>>>>>> + type: boolean
> >>>>>>> + description: I2C bus timeout enable for master/slave mode
> >>>>>>
> >>>>>> Nothing improved here in regards to my last comment.
> >>>>>
> >>>>> Yes, as I know your require is about " DT binding to represent
> >>>>> hardware
> >>>> setup"
> >>>>> So I add more description about aspeed,timeout as blow.
> >>>>>
> >>>>> ASPEED SOC chip is server product, i2c bus may have fingerprint
> >>>>> connect to
> >>>> another board. And also support hotplug.
> >>>>> The following is board-specific design example.
> >>>>> Board A Board B
> >>>>> ------------------------- ------------------------
> >>>>> |i2c bus#1(master/slave) <===fingerprint ===> i2c bus#x
> (master/slave)|
> >>>>> |i2c bus#2(master)-> tmp i2c device | |
> >>>> |
> >>>>> |i2c bus#3(master)-> adc i2c device | |
> >>>> |
> >>>>> ------------------------- ------------------------
> >>>>>
> >>>>> aspeed,timout properites:
> >>>>> For example I2C controller as slave mode, and suddenly
> disconnected.
> >>>>> Slave state machine will keep waiting for master clock in for
> >>>>> rx/tx
> >> transmit.
> >>>>> So it need timeout setting to enable timeout unlock controller state.
> >>>>> And in another side. In Master side also need avoid suddenly slave
> >>>> miss(un-plug), Master will timeout and release the SDA/SCL.
> >>>>>
> >>>>> Do you mean add those description into ore aspeed,timout
> >>>>> properites
> >>>> description?
> >>>>
> >>>> You are describing here one particular feature you want to enable
> >>>> in the driver which looks non-scalable and more difficult to
> configure/use.
> >>>> What I was looking for is to describe the actual configuration you have
> (e.g.
> >>>> multi-master) which leads to enable or disable such feature in your
> >> hardware.
> >>>> Especially that bool value does not scale later to actual timeout
> >>>> values in time (ms)...
> >>>>
> >>>> I don't know I2C that much, but I wonder - why this should be
> >>>> specific to Aspeed I2C and no other I2C controllers implement it?
> >>>> IOW, this looks quite generic and every I2C controller should have
> >>>> it. Adding it specific to Aspeed suggests that either we miss a
> >>>> generic property or this should not be in DT at all (because no one
> >>>> else has
> >> it...).
> >>>>
> >>>> Also I wonder, why you wouldn't enable timeout always...
> >>>>
> >>>> +Cc Wolfram,
> >>>> Maybe you know whether bool "timeout" property for one controller
> >>>> makes sense? Why we do not have it for all controllers?
> >>>>
> >>> Because, i2c bus didn’t specific timeout.
> >>> But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
> >>>
> >>> It have definition in SMBus specification.
> >>> http://smbus.org/specs/SMBus_3_1_20180319.pdf
> >>> You can check Page 18, Note3 that have timeout description.
> >>
> >> Then you have already property for this - "smbus"?
> > To be a property "smbus", that would be a big topic, I saw fsl i2c
> > also have this.
> > https://github.com/torvalds/linux/blob/master/Documentation/devicetree
> > /bindings/i2c/i2c-mpc.yaml#L43-L47
> > So, I just think the "timeout" property.
>
> Yeah and this is the only place. It also differs because it allows actual
> timeout values.
Thanks, So can I still keep the property "aspeed,timeout" here?
It is the only place.
Best regards,
Ryan Chen
More information about the Linux-aspeed
mailing list