[1/2] dt-bindings: w1: Add YAML DT Schema for AMD w1 master and MAINTAINERS entry

  Add YAML DT Schema for the AMD w1 master IP.

This hardware guarantees protocol timing for driving off-board devices such
as thermal sensors, proms, etc using the 1wire protocol.

Add MAINTAINERS entry for DT Schema.

Co-developed-by: Thomas Delev <thomas.delev@amd.com>
Signed-off-by: Thomas Delev <thomas.delev@amd.com>
Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
---
 .../bindings/w1/amd,axi-1wire-master.yaml     | 44 +++++++++++++++++++
 MAINTAINERS                                   |  7 +++
 2 files changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
  

On Fri, Oct 13, 2023 at 02:30:12AM -0700, Kris Chaplin wrote:
> Add YAML DT Schema for the AMD w1 master IP.
> 
> This hardware guarantees protocol timing for driving off-board devices such
> as thermal sensors, proms, etc using the 1wire protocol.
> 
> Add MAINTAINERS entry for DT Schema.
> 
> Co-developed-by: Thomas Delev <thomas.delev@amd.com>
> Signed-off-by: Thomas Delev <thomas.delev@amd.com>
> Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
> ---
>  .../bindings/w1/amd,axi-1wire-master.yaml     | 44 +++++++++++++++++++
>  MAINTAINERS                                   |  7 +++
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> 
> diff --git a/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> new file mode 100644
> index 000000000000..41f7294a84a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/w1/amd,axi-1wire-master.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMD AXI 1-wire bus master for Programmable Logic

Inconsistent case for the title here bothers my OCD, but the binding is
fine as far as I can see.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> +
> +maintainers:
> +  - Kris Chaplin <kris.chaplin@amd.com>
> +
> +properties:
> +  compatible:
> +    const: amd,axi-1wire-master
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    onewire@a0000000 {
> +        compatible = "amd,axi-1wire-master";
> +        reg = <0xa0000000 0x10000>;
> +        clocks = <&zynqmp_clk 0x47>;
> +        interrupts = <GIC_SPI 0x59 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4b2c378b4fd9..6ec3922b256e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1066,6 +1066,13 @@ M:	Sanjay R Mehta <sanju.mehta@amd.com>
>  S:	Maintained
>  F:	drivers/spi/spi-amd.c
>  
> +AMD W1 DRIVER
> +M:	Kris Chaplin <kris.chaplin@amd.com>
> +R:	Thomas Delev <thomas.delev@amd.com>
> +R:	Michal Simek <michal.simek@amd.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> +
>  AMD XGBE DRIVER
>  M:	"Shyam Sundar S K" <Shyam-sundar.S-k@amd.com>
>  L:	netdev@vger.kernel.org
> -- 
> 2.42.GIT
>
  

On 13/10/2023 11:30, Kris Chaplin wrote:
> Add YAML DT Schema for the AMD w1 master IP.
> 
> This hardware guarantees protocol timing for driving off-board devices such
> as thermal sensors, proms, etc using the 1wire protocol.
> 
> Add MAINTAINERS entry for DT Schema.
> 
> Co-developed-by: Thomas Delev <thomas.delev@amd.com>
> Signed-off-by: Thomas Delev <thomas.delev@amd.com>
> Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
> ---
>  .../bindings/w1/amd,axi-1wire-master.yaml     | 44 +++++++++++++++++++
>  MAINTAINERS                                   |  7 +++
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> 
> diff --git a/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> new file mode 100644
> index 000000000000..41f7294a84a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/w1/amd,axi-1wire-master.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMD AXI 1-wire bus master for Programmable Logic
> +
> +maintainers:
> +  - Kris Chaplin <kris.chaplin@amd.com>
> +
> +properties:
> +  compatible:
> +    const: amd,axi-1wire-master

That's a quite generic compatible. axi is ARM term, 1-wire is the name
of the bus and master is the role. Concatenating three common words does
not create unique device name. Compatibles are supposed to be specific
and this is really relaxed. Anything can be over AXI, everything in
1wire is 1wire and every master device is a master.



Best regards,
Krzysztof
  

On Fri, Oct 13, 2023 at 05:04:32PM +0200, Krzysztof Kozlowski wrote:
> On 13/10/2023 11:30, Kris Chaplin wrote:
> > Add YAML DT Schema for the AMD w1 master IP.
> > 
> > This hardware guarantees protocol timing for driving off-board devices such
> > as thermal sensors, proms, etc using the 1wire protocol.
> > 
> > Add MAINTAINERS entry for DT Schema.
> > 
> > Co-developed-by: Thomas Delev <thomas.delev@amd.com>
> > Signed-off-by: Thomas Delev <thomas.delev@amd.com>
> > Signed-off-by: Kris Chaplin <kris.chaplin@amd.com>
> > ---
> >  .../bindings/w1/amd,axi-1wire-master.yaml     | 44 +++++++++++++++++++
> >  MAINTAINERS                                   |  7 +++
> >  2 files changed, 51 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> > new file mode 100644
> > index 000000000000..41f7294a84a3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/w1/amd,axi-1wire-master.yaml
> > @@ -0,0 +1,44 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/w1/amd,axi-1wire-master.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: AMD AXI 1-wire bus master for Programmable Logic
> > +
> > +maintainers:
> > +  - Kris Chaplin <kris.chaplin@amd.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: amd,axi-1wire-master
> 
> That's a quite generic compatible. axi is ARM term, 1-wire is the name
> of the bus and master is the role. Concatenating three common words does
> not create unique device name. Compatibles are supposed to be specific
> and this is really relaxed. Anything can be over AXI, everything in
> 1wire is 1wire and every master device is a master.

Given the vendor (and the title of the binding) this is almost certainly
an FPGA IP core, so the generic name is understandable. Using the exact
name of the IP in the AMD/Xilinx catalog probably is the best choice?
  

On 13/10/2023 17:07, Conor Dooley wrote:
>>> +maintainers:
>>> +  - Kris Chaplin <kris.chaplin@amd.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: amd,axi-1wire-master
>>
>> That's a quite generic compatible. axi is ARM term, 1-wire is the name
>> of the bus and master is the role. Concatenating three common words does
>> not create unique device name. Compatibles are supposed to be specific
>> and this is really relaxed. Anything can be over AXI, everything in
>> 1wire is 1wire and every master device is a master.
> 
> Given the vendor (and the title of the binding) this is almost certainly
> an FPGA IP core, so the generic name is understandable. Using the exact
> name of the IP in the AMD/Xilinx catalog probably is the best choice?

Other option is that it is a part of some Zynq SoC.

Best regards,
Krzysztof
  

On 13/10/2023 16:07, Conor Dooley wrote:
> On Fri, Oct 13, 2023 at 05:04:32PM +0200, Krzysztof Kozlowski wrote:
>>
>> That's a quite generic compatible. axi is ARM term, 1-wire is the name
>> of the bus and master is the role. Concatenating three common words does
>> not create unique device name. Compatibles are supposed to be specific
>> and this is really relaxed. Anything can be over AXI, everything in
>> 1wire is 1wire and every master device is a master.
> Given the vendor (and the title of the binding) this is almost certainly
> an FPGA IP core, so the generic name is understandable. Using the exact
> name of the IP in the AMD/Xilinx catalog probably is the best choice?

Indeed this is an Programmable Logic IP core - the official name of the 
core in our catalog is axi_1wire_master.  It is a soft HDL core.

regards,

Kris
  

On 13/10/2023 17:23, Kris Chaplin wrote:
> 
> On 13/10/2023 16:07, Conor Dooley wrote:
>> On Fri, Oct 13, 2023 at 05:04:32PM +0200, Krzysztof Kozlowski wrote:
>>>
>>> That's a quite generic compatible. axi is ARM term, 1-wire is the name
>>> of the bus and master is the role. Concatenating three common words does
>>> not create unique device name. Compatibles are supposed to be specific
>>> and this is really relaxed. Anything can be over AXI, everything in
>>> 1wire is 1wire and every master device is a master.
>> Given the vendor (and the title of the binding) this is almost certainly
>> an FPGA IP core, so the generic name is understandable. Using the exact
>> name of the IP in the AMD/Xilinx catalog probably is the best choice?
> 
> Indeed this is an Programmable Logic IP core - the official name of the 
> core in our catalog is axi_1wire_master.  It is a soft HDL core.

AMD product managers are highly skilled in naming things. Sigh.

Go ahead with AXI 1-wire master. Any future - from now to next 100 years
- product from AMD which will be different but sold under the same name,
thus creating conflict in compatible naming, should be rejected because
of that conflict or renamed to something else. If that happen I will
propose a name like "banana-wire".

Best regards,
Krzysztof
  

On 13/10/2023 16:29, Krzysztof Kozlowski wrote:
>
> AMD product managers are highly skilled in naming things. Sigh.
>
> Go ahead with AXI 1-wire master. Any future - from now to next 100 years
> - product from AMD which will be different but sold under the same name,
> thus creating conflict in compatible naming, should be rejected because
> of that conflict or renamed to something else. If that happen I will
> propose a name like "banana-wire".
>
> Best regards,
> Krzysztof

To be fair on the product managers the IP naming from a hardware 
perspective is in the context of the ex-Xilinx development tools, and as 
such there is no risk of namespace overlap with the rest of the 
business.  I'm not against changing the binding name for clarity in the 
kernel. Would adding pl within the binding (Programmable Logic) assist? 
Ie: amd,pl-axi-1wire-master

Regards
Kris
  

On Fri, Oct 13, 2023 at 04:23:15PM +0100, Kris Chaplin wrote:
> 
> On 13/10/2023 16:07, Conor Dooley wrote:
> > On Fri, Oct 13, 2023 at 05:04:32PM +0200, Krzysztof Kozlowski wrote:
> > > 
> > > That's a quite generic compatible. axi is ARM term, 1-wire is the name
> > > of the bus and master is the role. Concatenating three common words does
> > > not create unique device name. Compatibles are supposed to be specific
> > > and this is really relaxed. Anything can be over AXI, everything in
> > > 1wire is 1wire and every master device is a master.
> > Given the vendor (and the title of the binding) this is almost certainly
> > an FPGA IP core, so the generic name is understandable. Using the exact
> > name of the IP in the AMD/Xilinx catalog probably is the best choice?
> 
> Indeed this is an Programmable Logic IP core - the official name of the core
> in our catalog is axi_1wire_master.  It is a soft HDL core.

Only 1 version of it (ever)? Like other PL IP, it needs a version number 
(and not v1, v2, etc. made up by you). Really, your versioning scheme 
should be documented (like 
bindings/sifive/sifive-blocks-ip-versioning.txt), but Xilinx started 
versioning stuff some time back.

Also, 'master' is not considered great terminology nowadays. Perhaps the 
catalog name should be updated. 

Rob
  

On 13/10/2023 18:18, Rob Herring wrote:
> i_1wire_master. It is a soft HDL core.
> Only 1 version of it (ever)? Like other PL IP, it needs a version number
> (and not v1, v2, etc. made up by you). Really, your versioning scheme
> should be documented (like
> bindings/sifive/sifive-blocks-ip-versioning.txt), but Xilinx started
> versioning stuff some time back.

I've specified the PL IP to have both an ID and version number register 
in it, which is queried by the driver on probe.  As such we can version 
autodiscover.  Should there be incompatibilty in the driver due to new 
features in future PL IP, the major version number in the register will 
increment and the same driver can be extended to support the modified 
behaviour.  The default code in this first version will check to ensure 
major is at 1. I'm working with our IP group to encourage that all new 
IP have this mechanism moving forwards, as registers are a lot cheaper 
in logic than they used to be.

+  if (ver_major != 1) {
+     dev_err(dev, "AMD AXI W1 Master version %u.%u is not supported by 
this driver",
+        ver_major, ver_minor);
+     return -ENODEV;
+  }

> Also, 'master' is not considered great terminology nowadays. Perhaps the
> catalog name should be updated.

Agreed -  I've used the term to fit with others in the subsystem.  Would 
this be something that is changed and aligned across W1?  If so I'm 
happy to get the HDL IP renamed and binding / driver documentation to 
match if an alternative designator to replace master has already been 
chosen and standardised across 1Wire.

Regards
Kris