| Message ID | 20191114055155.20446-3-pgaj@cadence.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add data hold delay support | expand |
On Thu, 14 Nov 2019 06:51:54 +0100 Przemyslaw Gaj <pgaj@cadence.com> wrote: > Documenting THD_DEL (Data Hold Delay) feature of Cadence I3C > master controller. > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > --- > Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > index 1cf6182f888c..7d6349354390 100644 > --- a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > +++ b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > @@ -21,6 +21,12 @@ Documentation/devicetree/bindings/i3c/i3c.txt for more details): > - i2c-scl-hz > - i3c-scl-hz > > +Optional properties, Cadence I3C master controller specific: > + > +- thd_del: Data Hold Delay. Sets data hold delay (i.e. SDA_OUT data > + line will change its value with extra delay that specified > + in this register). If it's a Cadence specific property, it should be prefixed with 'cdns,': - cdns,thd-delay Quick question about this delay, is it related to the IP integration in a SoC or is it board specific? In former case, I'd recommend attaching this piece of information to the compatible and have one compatible per SoC. > + > I3C device connected on the bus follow the generic description (see > Documentation/devicetree/bindings/i3c/i3c.txt for more details). >
On Thu, 14 Nov 2019 10:15:49 +0100 Boris Brezillon <boris.brezillon@collabora.com> wrote: > On Thu, 14 Nov 2019 06:51:54 +0100 > Przemyslaw Gaj <pgaj@cadence.com> wrote: > > > Documenting THD_DEL (Data Hold Delay) feature of Cadence I3C > > master controller. > > > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > > --- > > Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > > index 1cf6182f888c..7d6349354390 100644 > > --- a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > > +++ b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > > @@ -21,6 +21,12 @@ Documentation/devicetree/bindings/i3c/i3c.txt for more details): > > - i2c-scl-hz > > - i3c-scl-hz > > > > +Optional properties, Cadence I3C master controller specific: > > + > > +- thd_del: Data Hold Delay. Sets data hold delay (i.e. SDA_OUT data > > + line will change its value with extra delay that specified > > + in this register). > > If it's a Cadence specific property, it should be prefixed with 'cdns,': > > - cdns,thd-delay Oh, and you need to specify the unit. Given the code, I suspect it's in clk-cycles, which is not great, since you probably want the delay to always be the same no matter the frequency of the clk driving the I3C master block. > > Quick question about this delay, is it related to the IP integration in > a SoC or is it board specific? In former case, I'd recommend attaching > this piece of information to the compatible and have one compatible per > SoC. > > > + > > I3C device connected on the bus follow the generic description (see > > Documentation/devicetree/bindings/i3c/i3c.txt for more details). > > >
The 11/14/2019 10:26, Boris Brezillon wrote: > > On Thu, 14 Nov 2019 10:15:49 +0100 > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > On Thu, 14 Nov 2019 06:51:54 +0100 > > Przemyslaw Gaj <pgaj@cadence.com> wrote: > > > > > Documenting THD_DEL (Data Hold Delay) feature of Cadence I3C > > > master controller. > > > > > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > > > --- > > > Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > > > index 1cf6182f888c..7d6349354390 100644 > > > --- a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > > > +++ b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > > > @@ -21,6 +21,12 @@ Documentation/devicetree/bindings/i3c/i3c.txt for more details): > > > - i2c-scl-hz > > > - i3c-scl-hz > > > > > > +Optional properties, Cadence I3C master controller specific: > > > + > > > +- thd_del: Data Hold Delay. Sets data hold delay (i.e. SDA_OUT data > > > + line will change its value with extra delay that specified > > > + in this register). > > > > If it's a Cadence specific property, it should be prefixed with 'cdns,': > > > > - cdns,thd-delay Ok. > > Oh, and you need to specify the unit. Given the code, I suspect it's in > clk-cycles, which is not great, since you probably want the delay to > always be the same no matter the frequency of the clk driving the I3C > master block. > Actually, this is encoded value. 3 means no delay, 2 - 1x clk delay, 1 - 2x clk dealy, 0 - 3x clk delay. I should mention about that in the documentation. > > > > Quick question about this delay, is it related to the IP integration in > > a SoC or is it board specific? In former case, I'd recommend attaching > > this piece of information to the compatible and have one compatible per > > SoC. > > This is spec requirement, slave shouldn't see SDA change before SCL. It is possible to achive this requirement during SoC physical design. If this is not the case, you can achive this using thd_del functionality. For now this is generic driver for our controller. So the question is, what should I do? Should I add default value for existing compatible and wait for different SoCs compatibility? > > > + > > > I3C device connected on the bus follow the generic description (see > > > Documentation/devicetree/bindings/i3c/i3c.txt for more details). > > > > > >
On Thu, 14 Nov 2019 11:54:32 +0100 Przemyslaw Gaj <pgaj@cadence.com> wrote: > The 11/14/2019 10:26, Boris Brezillon wrote: > > > > On Thu, 14 Nov 2019 10:15:49 +0100 > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > On Thu, 14 Nov 2019 06:51:54 +0100 > > > Przemyslaw Gaj <pgaj@cadence.com> wrote: > > > > > > > Documenting THD_DEL (Data Hold Delay) feature of Cadence I3C > > > > master controller. > > > > > > > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > > > > --- > > > > Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > > > > index 1cf6182f888c..7d6349354390 100644 > > > > --- a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > > > > +++ b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > > > > @@ -21,6 +21,12 @@ Documentation/devicetree/bindings/i3c/i3c.txt for more details): > > > > - i2c-scl-hz > > > > - i3c-scl-hz > > > > > > > > +Optional properties, Cadence I3C master controller specific: > > > > + > > > > +- thd_del: Data Hold Delay. Sets data hold delay (i.e. SDA_OUT data > > > > + line will change its value with extra delay that specified > > > > + in this register). > > > > > > If it's a Cadence specific property, it should be prefixed with 'cdns,': > > > > > > - cdns,thd-delay > > Ok. > > > > > Oh, and you need to specify the unit. Given the code, I suspect it's in > > clk-cycles, which is not great, since you probably want the delay to > > always be the same no matter the frequency of the clk driving the I3C > > master block. > > > > Actually, this is encoded value. 3 means no delay, 2 - 1x clk delay, > 1 - 2x clk dealy, 0 - 3x clk delay. I should mention about that in the > documentation. By clk you mean SCL or the clock driving the I3C master logic (which is likely to run at a higher freq)? > > > > > > > Quick question about this delay, is it related to the IP integration in > > > a SoC or is it board specific? In former case, I'd recommend attaching > > > this piece of information to the compatible and have one compatible per > > > SoC. > > > > > This is spec requirement, slave shouldn't see SDA change before SCL. It > is possible to achive this requirement during SoC physical design. If this > is not the case, you can achive this using thd_del functionality. For > now this is generic driver for our controller. So the question is, what > should I do? Should I add default value for existing compatible and wait > for different SoCs compatibility? Yes, exactly.
The 11/14/2019 12:09, Boris Brezillon wrote: > EXTERNAL MAIL > > > On Thu, 14 Nov 2019 11:54:32 +0100 > Przemyslaw Gaj <pgaj@cadence.com> wrote: > > > The 11/14/2019 10:26, Boris Brezillon wrote: > > > > > > On Thu, 14 Nov 2019 10:15:49 +0100 > > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > On Thu, 14 Nov 2019 06:51:54 +0100 > > > > Przemyslaw Gaj <pgaj@cadence.com> wrote: > > > > > > > > > Documenting THD_DEL (Data Hold Delay) feature of Cadence I3C > > > > > master controller. > > > > > > > > > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > > > > > --- > > > > > Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt | 6 ++++++ > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > > > > > index 1cf6182f888c..7d6349354390 100644 > > > > > --- a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > > > > > +++ b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > > > > > @@ -21,6 +21,12 @@ Documentation/devicetree/bindings/i3c/i3c.txt for more details): > > > > > - i2c-scl-hz > > > > > - i3c-scl-hz > > > > > > > > > > +Optional properties, Cadence I3C master controller specific: > > > > > + > > > > > +- thd_del: Data Hold Delay. Sets data hold delay (i.e. SDA_OUT data > > > > > + line will change its value with extra delay that specified > > > > > + in this register). > > > > > > > > If it's a Cadence specific property, it should be prefixed with 'cdns,': > > > > > > > > - cdns,thd-delay > > > > Ok. > > > > > > > > Oh, and you need to specify the unit. Given the code, I suspect it's in > > > clk-cycles, which is not great, since you probably want the delay to > > > always be the same no matter the frequency of the clk driving the I3C > > > master block. > > > > > > > Actually, this is encoded value. 3 means no delay, 2 - 1x clk delay, > > 1 - 2x clk dealy, 0 - 3x clk delay. I should mention about that in the > > documentation. > > By clk you mean SCL or the clock driving the I3C master logic (which is > likely to run at a higher freq)? Yes, this is the clock driving I3C master logic. > > > > > > > > > > > Quick question about this delay, is it related to the IP integration in > > > > a SoC or is it board specific? In former case, I'd recommend attaching > > > > this piece of information to the compatible and have one compatible per > > > > SoC. > > > > > > > > This is spec requirement, slave shouldn't see SDA change before SCL. It > > is possible to achive this requirement during SoC physical design. If this > > is not the case, you can achive this using thd_del functionality. For > > now this is generic driver for our controller. So the question is, what > > should I do? Should I add default value for existing compatible and wait > > for different SoCs compatibility? > > Yes, exactly. Ok. In that case I don't need dt binding.
diff --git a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt index 1cf6182f888c..7d6349354390 100644 --- a/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt +++ b/Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt @@ -21,6 +21,12 @@ Documentation/devicetree/bindings/i3c/i3c.txt for more details): - i2c-scl-hz - i3c-scl-hz +Optional properties, Cadence I3C master controller specific: + +- thd_del: Data Hold Delay. Sets data hold delay (i.e. SDA_OUT data + line will change its value with extra delay that specified + in this register). + I3C device connected on the bus follow the generic description (see Documentation/devicetree/bindings/i3c/i3c.txt for more details).
Documenting THD_DEL (Data Hold Delay) feature of Cadence I3C master controller. Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> --- Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt | 6 ++++++ 1 file changed, 6 insertions(+)