| Message ID | 1557242108-13580-2-git-send-email-sagar.kadam@sifive.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Extend dt bindings to support I2C on sifive devices and a fix broken IRQ in polling mode. | expand |
> +or > + /* > + An Opencore based I2C node in FU540-C000 chip from SiFive > + This chip has a hardware erratum for broken IRQ > + so it's recommended not to define interrupt in the device node > + */ > + i2c@10030000 { > + compatible = "sifive,i2c0","sifive,fu540-c000-i2c"; The most specific compatible comes first. You also need to be forward thinking. The next revision of the silicon could have the bug fixed. You can then use plain "sifive,i2c0" to indicate just polling is needed, not bug workaround. But for that to work, you need "sifive,fu540-c000-i2c" first. Andrew
On Tue, May 07, 2019 at 08:45:06PM +0530, Sagar Shrikant Kadam wrote: > Add FU540-C000 specific device tree bindings to already > available i2-ocores file. This device is available on > HiFive Unleashed Rev A00 board. > > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com> > --- > Documentation/devicetree/bindings/i2c/i2c-ocores.txt | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt > index 17bef9a..f6bcf90 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt > @@ -2,6 +2,7 @@ Device tree configuration for i2c-ocores > > Required properties: > - compatible : "opencores,i2c-ocores" or "aeroflexgaisler,i2cmst" > + "sifive,fu540-c000-i2c" or "sifive,i2c0" If this is Opencores IP, does it really follow the Sifive versioning convention? If so, please reference sifive-blocks-ip-versioning.txt (which appears to have missed going upstream). Also, referencing the IP repository would be good too. If this IP block doesn't follow the same convention, then don't try using it for this binding. > - reg : bus address start and address range size of device > - interrupts : interrupt number > - clocks : handle to the controller clock; see the note below. > @@ -67,3 +68,22 @@ or > reg = <0x60>; > }; > }; > +or Just a new compatible isn't really a reason to add an example. > + /* > + An Opencore based I2C node in FU540-C000 chip from SiFive > + This chip has a hardware erratum for broken IRQ > + so it's recommended not to define interrupt in the device node Then interrupts needs to be optional. > + */ > + i2c@10030000 { > + compatible = "sifive,i2c0","sifive,fu540-c000-i2c"; > + reg = <0x0 0x10030000 0x0 0x1000>; > + reg-names = "i2c-control"; Not doucmented. > + clocks = <&tlclk>; > + clock-frequency = <100000>; > + > + reg-shift = <2>; > + reg-io-width = <1>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > -- > 1.9.1 > > > -- > The information transmitted is intended only for the person or entity to > which it is addressed and may contain confidential and/or privileged > material. If you are not the intended recipient of this message please do > not read, copy, use or disclose this communication and notify the sender > immediately. It should be noted that any review, retransmission, > dissemination or other use of, or taking action or reliance upon, this > information by persons or entities other than the intended recipient is > prohibited.
Hello Rob, Thank you for the review. On Tue, May 14, 2019 at 2:26 AM Rob Herring <robh@kernel.org> wrote: > > On Tue, May 07, 2019 at 08:45:06PM +0530, Sagar Shrikant Kadam wrote: > > Add FU540-C000 specific device tree bindings to already > > available i2-ocores file. This device is available on > > HiFive Unleashed Rev A00 board. > > > > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com> > > --- > > Documentation/devicetree/bindings/i2c/i2c-ocores.txt | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt > > index 17bef9a..f6bcf90 100644 > > --- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt > > +++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt > > @@ -2,6 +2,7 @@ Device tree configuration for i2c-ocores > > > > Required properties: > > - compatible : "opencores,i2c-ocores" or "aeroflexgaisler,i2cmst" > > + "sifive,fu540-c000-i2c" or "sifive,i2c0" > > If this is Opencores IP, does it really follow the Sifive versioning > convention? If so, please reference sifive-blocks-ip-versioning.txt > (which appears to have missed going upstream). Also, referencing the IP > repository would be good too. If this IP block doesn't follow the same > convention, then don't try using it for this binding. > Yes, the sifive,fu540-c000-i2c is a SoC specific compatibility string, this way SoC specific workaround's or bugs, can be handled in the software and the ip-block specific compatibility string "sifive,<ip-block-name><integer version number>" i.e. sifive,i2c0 is IP block specific compatibility string. Please let me know if I need some correction here? I will also update reference for sifive-blocks-ip-versioning and the ip repository into next version of patch. > > - reg : bus address start and address range size of device > > - interrupts : interrupt number > > - clocks : handle to the controller clock; see the note below. > > @@ -67,3 +68,22 @@ or > > reg = <0x60>; > > }; > > }; > > +or > > Just a new compatible isn't really a reason to add an example. > > > + /* > > + An Opencore based I2C node in FU540-C000 chip from SiFive > > + This chip has a hardware erratum for broken IRQ > > + so it's recommended not to define interrupt in the device node > > Then interrupts needs to be optional. True, I will move interrupts and interrupt parent into optional section > > > + */ > > + i2c@10030000 { > > + compatible = "sifive,i2c0","sifive,fu540-c000-i2c"; > > + reg = <0x0 0x10030000 0x0 0x1000>; > > + reg-names = "i2c-control"; > > Not doucmented. In v1, I had added a new binding file as sifive-i2c-ocores.txt for SiFive i2c core. After Andrew's suggestion, extending the available i2c-ocores.txt seemed to be a better idea rather than adding a new file. so added an example node which is HiFive specific in the existing file. Please let me know if I need to handle this in a different way. > > > + clocks = <&tlclk>; > > + clock-frequency = <100000>; > > + > > + reg-shift = <2>; > > + reg-io-width = <1>; > > + > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > -- > > 1.9.1 > > > > > > -- > > The information transmitted is intended only for the person or entity to > > which it is addressed and may contain confidential and/or privileged > > material. If you are not the intended recipient of this message please do > > not read, copy, use or disclose this communication and notify the sender > > immediately. It should be noted that any review, retransmission, > > dissemination or other use of, or taking action or reliance upon, this > > information by persons or entities other than the intended recipient is > > prohibited
> > > + */ > > > + i2c@10030000 { > > > + compatible = "sifive,i2c0","sifive,fu540-c000-i2c"; > > > + reg = <0x0 0x10030000 0x0 0x1000>; > > > + reg-names = "i2c-control"; > > > > Not doucmented. > In v1, I had added a new binding file as sifive-i2c-ocores.txt for > SiFive i2c core. > After Andrew's suggestion, extending the available i2c-ocores.txt > seemed to be a better idea rather than adding a new file. > so added an example node which is HiFive specific in the existing file. > Please let me know if I need to handle this in a different way. Hi Sagar Is reg-names actually needed? Andrew
On Tue, May 14, 2019 at 7:50 AM Sagar Kadam <sagar.kadam@sifive.com> wrote: > > Hello Rob, > > Thank you for the review. > > On Tue, May 14, 2019 at 2:26 AM Rob Herring <robh@kernel.org> wrote: > > > > On Tue, May 07, 2019 at 08:45:06PM +0530, Sagar Shrikant Kadam wrote: > > > Add FU540-C000 specific device tree bindings to already > > > available i2-ocores file. This device is available on > > > HiFive Unleashed Rev A00 board. > > > > > > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com> > > > --- > > > Documentation/devicetree/bindings/i2c/i2c-ocores.txt | 20 ++++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt > > > index 17bef9a..f6bcf90 100644 > > > --- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt > > > +++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt > > > @@ -2,6 +2,7 @@ Device tree configuration for i2c-ocores > > > > > > Required properties: > > > - compatible : "opencores,i2c-ocores" or "aeroflexgaisler,i2cmst" > > > + "sifive,fu540-c000-i2c" or "sifive,i2c0" > > > > If this is Opencores IP, does it really follow the Sifive versioning > > convention? If so, please reference sifive-blocks-ip-versioning.txt > > (which appears to have missed going upstream). Also, referencing the IP > > repository would be good too. If this IP block doesn't follow the same > > convention, then don't try using it for this binding. > > > Yes, the sifive,fu540-c000-i2c is a SoC specific compatibility string, > this way SoC specific > workaround's or bugs, can be handled in the software and the ip-block > specific compatibility > string "sifive,<ip-block-name><integer version number>" i.e. > sifive,i2c0 is IP block specific compatibility > string. Please let me know if I need some correction here? > I will also update reference for sifive-blocks-ip-versioning and the > ip repository into next version of patch. My question is whether I can correlate v0 to a specific revision of the IP and versions will be tracked in the same way as SiFive IP blocks? > > > - reg : bus address start and address range size of device > > > - interrupts : interrupt number > > > - clocks : handle to the controller clock; see the note below. > > > @@ -67,3 +68,22 @@ or > > > reg = <0x60>; > > > }; > > > }; > > > +or > > > > Just a new compatible isn't really a reason to add an example. > > > > > + /* > > > + An Opencore based I2C node in FU540-C000 chip from SiFive > > > + This chip has a hardware erratum for broken IRQ > > > + so it's recommended not to define interrupt in the device node > > > > Then interrupts needs to be optional. > True, I will move interrupts and interrupt parent into optional section > > > > > + */ > > > + i2c@10030000 { > > > + compatible = "sifive,i2c0","sifive,fu540-c000-i2c"; > > > + reg = <0x0 0x10030000 0x0 0x1000>; > > > + reg-names = "i2c-control"; > > > > Not doucmented. > In v1, I had added a new binding file as sifive-i2c-ocores.txt for > SiFive i2c core. > After Andrew's suggestion, extending the available i2c-ocores.txt > seemed to be a better idea rather than adding a new file. > so added an example node which is HiFive specific in the existing file. > Please let me know if I need to handle this in a different way. That has nothing to do with whether reg-names is documented. Being in the example is not documented. You either need to add it to the property list or drop it from the example. IMO, you should drop it as it is not necessary with only 1 entry. Rob
diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt index 17bef9a..f6bcf90 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt @@ -2,6 +2,7 @@ Device tree configuration for i2c-ocores Required properties: - compatible : "opencores,i2c-ocores" or "aeroflexgaisler,i2cmst" + "sifive,fu540-c000-i2c" or "sifive,i2c0" - reg : bus address start and address range size of device - interrupts : interrupt number - clocks : handle to the controller clock; see the note below. @@ -67,3 +68,22 @@ or reg = <0x60>; }; }; +or + /* + An Opencore based I2C node in FU540-C000 chip from SiFive + This chip has a hardware erratum for broken IRQ + so it's recommended not to define interrupt in the device node + */ + i2c@10030000 { + compatible = "sifive,i2c0","sifive,fu540-c000-i2c"; + reg = <0x0 0x10030000 0x0 0x1000>; + reg-names = "i2c-control"; + clocks = <&tlclk>; + clock-frequency = <100000>; + + reg-shift = <2>; + reg-io-width = <1>; + + #address-cells = <1>; + #size-cells = <0>; + };
Add FU540-C000 specific device tree bindings to already available i2-ocores file. This device is available on HiFive Unleashed Rev A00 board. Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com> --- Documentation/devicetree/bindings/i2c/i2c-ocores.txt | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)