Re: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.

Re: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.

[thesven73]

>> This is why the bridge driver has a dependency on a pwm.
>> If the pwm could be enabled individually, I could drop this dependency.
>> Can you think of a way?
>
> How about using the pwm-clock device tree binding to turn the PWM into
> a clock?
> Documentation/devicetree/bindings/clock/pwm-clock.txt

Looks like a winner. Thanks !!

Re: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.

[Lee Jones]

On Thu, 25 Oct 2018, thesven73@gmail.com wrote:

> >> This is why the bridge driver has a dependency on a pwm.
> >> If the pwm could be enabled individually, I could drop this dependency.
> >> Can you think of a way?
> >
> > How about using the pwm-clock device tree binding to turn the PWM into
> > a clock?
> > Documentation/devicetree/bindings/clock/pwm-clock.txt
> 
> Looks like a winner. Thanks !!

Could you fix your mailed to reply to mails 'threaded' please?

Your mails are now spread far and wide across my Inbox.

Thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.

[Linus Walleij]

On Thu, Oct 25, 2018 at 3:55 PM <thesven73@gmail.com> wrote:

> >> +               fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
> >> +                               0x00000000 0xa0000240 0x00000000>;
> >
> > Is it just a copy/paste from
> > Documentation/devicetree/bindings/bus/imx-weim.txt
> > leftover?
>
> No. We attach the bridge to the i.MX WEIM bus. Every fdt WEIM child node
> requires a fsl,weim-cs-timing property, which provides the bus timing for
> that particular chip select. It's the weim driver that requires this,
> I'm only following its instructions.
>
> Should I just leave this out in the example? In theory, the bridge can be
> connected to any parallel bus, so fsl,weim-cs-timing is Too Much Information
> for the example?

No it's fine, I understand it now.

Whay you can do is write before the example that this example node
is using the WEIM bindings and reference that binding document.

Yours,
Linus Walleij

Re: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.

[David Lechner]

On 10/25/2018 08:55 AM, thesven73@gmail.com wrote:
> Hi Linus, thank you for the patch review !!
> 
>>> +  - pwms : the pwm connected to the bridge's 'pwm input'.
>>
>> That is really unintuitive and needs a detailed explanation. What
>> is a bridge doing with a PWM? Is it 100% certain this is a PWM,
>> it's not just a .... clock? A pwm is a pule WIDTH modulator and
>> I can't for my life understand why a bus bridge needs a signal
>> with variable pulse width, but surprise me! :D
> 
> You are 100% correct, this is a clock !
> The hardware designers attached the bridge's clock input to an iMX pwm output,
> and instructed us to provide a clock with 50% duty cycle and a certain freq.
> 
> The only way I know to activate a pwm is to connect it to a driver in the fdt,
> then inside the driver enable the pwm, like so:
> 
> +       /* PWM */
> +       pwm = devm_pwm_get(dev, NULL);
> +       if (IS_ERR(pwm)) {
> +               dev_err(dev, "pwm not found\n");
> +               return -EINVAL;
> +       }
> +       pwm_get_args(pwm, &pargs);
> +       period = pargs.period;
> +       err = pwm_config(pwm, period/2, period);
> +       if (err)
> +               return err;
> +       err = pwm_enable(pwm);
> +       if (err)
> +               return err;
> 
> This is why the bridge driver has a dependency on a pwm.
> If the pwm could be enabled individually, I could drop this dependency.
> Can you think of a way?


How about using the pwm-clock device tree binding to turn the PWM into
a clock?

Documentation/devicetree/bindings/clock/pwm-clock.txt

Re: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.

[thesven73]

Hi Linus, thank you for the patch review !!

>> +  - pwms : the pwm connected to the bridge's 'pwm input'.
>
> That is really unintuitive and needs a detailed explanation. What
> is a bridge doing with a PWM? Is it 100% certain this is a PWM,
> it's not just a .... clock? A pwm is a pule WIDTH modulator and
> I can't for my life understand why a bus bridge needs a signal
> with variable pulse width, but surprise me! :D

You are 100% correct, this is a clock !
The hardware designers attached the bridge's clock input to an iMX pwm output,
and instructed us to provide a clock with 50% duty cycle and a certain freq.

The only way I know to activate a pwm is to connect it to a driver in the fdt,
then inside the driver enable the pwm, like so:

+       /* PWM */
+       pwm = devm_pwm_get(dev, NULL);
+       if (IS_ERR(pwm)) {
+               dev_err(dev, "pwm not found\n");
+               return -EINVAL;
+       }
+       pwm_get_args(pwm, &pargs);
+       period = pargs.period;
+       err = pwm_config(pwm, period/2, period);
+       if (err)
+               return err;
+       err = pwm_enable(pwm);
+       if (err)
+               return err;

This is why the bridge driver has a dependency on a pwm.
If the pwm could be enabled individually, I could drop this dependency.
Can you think of a way?

>> +               fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
>> +                               0x00000000 0xa0000240 0x00000000>;
>
> Is it just a copy/paste from
> Documentation/devicetree/bindings/bus/imx-weim.txt
> leftover?

No. We attach the bridge to the i.MX WEIM bus. Every fdt WEIM child node
requires a fsl,weim-cs-timing property, which provides the bus timing for
that particular chip select. It's the weim driver that requires this,
I'm only following its instructions.

Should I just leave this out in the example? In theory, the bridge can be
connected to any parallel bus, so fsl,weim-cs-timing is Too Much Information
for the example?

Re: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.

[thesven73]

Hi Rob, thank you so much for taking a look at this patch !

>> +This chip communicates with the SoC over the WEIM bus. It is
>> +expected that its Device Tree node is specified as the child of a node
>> +corresponding to the WEIM bus used for communication.
>
>By WEIM you are referring to i.MX external parallel bus? Presumably this 
>can work on any parallel bus, not just i.MX.

That's right, in theory this hardware could work with any parallel bus, if
timed correctly. The driver also has no code dependencies on the WEIM driver.
You'll only find it in the field on top of the i.MX's WEIM bus, however.

Do you think it would be 'nicer' to drop references to i.MX and WEIM? Can do.

>> +  - compatible : The following chip-specific string:
>> +        "arcx,anybus-bridge"
>
>No version or part number?

We have id and capability registers in the chip for that. This driver is
generic, both forwards and backwards compatible. Is it ok to leave out
version/part number in this case?

The rest of your feedback will go into v2. Thanks :)

Re: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.

[Linus Walleij]

Hi Sven,

thanks for your patch!

On Wed, Oct 24, 2018 at 4:25 PM Sven Van Asbroeck <svendev@arcx.com> wrote:

> +  - pwms : the pwm connected to the bridge's 'pwm input'.

That is really unintuitive and needs a detailed explanation. What
is a bridge doing with a PWM? Is it 100% certain this is a PWM,
it's not just a .... clock? A pwm is a pule WIDTH modulator and
I can't for my life understand why a bus bridge needs a signal
with variable pulse width, but surprise me! :D

> +               fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
> +                               0x00000000 0xa0000240 0x00000000>;

Is it just a copy/paste from
Documentation/devicetree/bindings/bus/imx-weim.txt
leftover?

It was mentioned as undocumented but it is also pretty terse
with these opaque hex numbers. I would never let the
Freescale binding pass if I was reviewing it.

Look at my bindings for Qualcomm EBI in
Documentation/devicetree/bindings/bus/qcom,ebi2.txt
for example.

But maybe it's not even supposed to be there.

Yours,
Linus Walleij

Re: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.

[Lee Jones]

On Wed, 24 Oct 2018, Sven Van Asbroeck wrote:

> This patch adds devicetree binding documentation for the
> Arcx anybus bridge.
> 
> Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
> ---
>  .../bindings/mfd/arcx,anybus-bridge.txt       | 37 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>  2 files changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt b/Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt
> new file mode 100644
> index 000000000000..3c0399c4ed45
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt
> @@ -0,0 +1,37 @@
> +* Arcx anybus bridge
> +
> +This chip communicates with the SoC over the WEIM bus. It is
> +expected that its Device Tree node is specified as the child of a node
> +corresponding to the WEIM bus used for communication.
> +
> +Required properties:
> +
> +  - compatible : The following chip-specific string:
> +        "arcx,anybus-bridge"
> +
> +  - reg :
> +	weim memory area where the cpld registers are located,	followed by:
> +	weim memory area of the first  anybus-s slot,		followed by:
> +	weim memory area of the second anybus-s slot.
> +
> +  - enable-gpios : the gpio connected to the bridge's 'enable gpio'.
> +
> +  - pwms : the pwm connected to the bridge's 'pwm input'.
> +
> +  - irq-gpios : the gpios connected to the bridge's interrupt lines.
> +	note that there is no need to provide the 'interrupts' property here.
> +
> +Example of usage:
> +
> +&weim {
> +	bridge@0,0 {

I haven't seen this syntax before.

It doesn't mean it's wrong, but will needs Rob et. al to cast an eye.

> +		compatible = "arcx,anybus-bridge";
> +		reg = <0 0 0x100>, <0 0x400000 0x800>, <1 0x400000 0x800>;
> +		fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
> +				0x00000000 0xa0000240 0x00000000>;

This needs to be documented.

> +		enable-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>;
> +		pwms = <&pwm3 0 571>;
> +		irq-gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>,
> +					<&gpio1 5 GPIO_ACTIVE_HIGH>;

Tabbing.

> +	};
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 2c3fc512e746..1bf07b20a8af 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -35,6 +35,7 @@ aptina	Aptina Imaging
>  arasan	Arasan Chip Systems
>  archermind ArcherMind Technology (Nanjing) Co., Ltd.
>  arctic	Arctic Sand
> +arcx	Arcx/Archronix Inc.
>  aries	Aries Embedded GmbH
>  arm	ARM Ltd.
>  armadeus	ARMadeus Systems SARL

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.

[Rob Herring]

On Wed, Oct 24, 2018 at 10:24:54AM -0400, Sven Van Asbroeck wrote:
> This patch adds devicetree binding documentation for the
> Arcx anybus bridge.
> 
> Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
> ---
>  .../bindings/mfd/arcx,anybus-bridge.txt       | 37 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>  2 files changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt b/Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt
> new file mode 100644
> index 000000000000..3c0399c4ed45
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt
> @@ -0,0 +1,37 @@
> +* Arcx anybus bridge
> +
> +This chip communicates with the SoC over the WEIM bus. It is
> +expected that its Device Tree node is specified as the child of a node
> +corresponding to the WEIM bus used for communication.

By WEIM you are referring to i.MX external parallel bus? Presumably this 
can work on any parallel bus, not just i.MX.

> +
> +Required properties:
> +
> +  - compatible : The following chip-specific string:
> +        "arcx,anybus-bridge"

No version or part number?

> +
> +  - reg :
> +	weim memory area where the cpld registers are located,	followed by:
> +	weim memory area of the first  anybus-s slot,		followed by:
> +	weim memory area of the second anybus-s slot.
> +
> +  - enable-gpios : the gpio connected to the bridge's 'enable gpio'.
> +
> +  - pwms : the pwm connected to the bridge's 'pwm input'.
> +
> +  - irq-gpios : the gpios connected to the bridge's interrupt lines.
> +	note that there is no need to provide the 'interrupts' property here.

Why not? This should not be base on what the OS currently happens to 
support.

What are the functions of each line? The order must be defined.

> +
> +Example of usage:
> +
> +&weim {
> +	bridge@0,0 {
> +		compatible = "arcx,anybus-bridge";
> +		reg = <0 0 0x100>, <0 0x400000 0x800>, <1 0x400000 0x800>;
> +		fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
> +				0x00000000 0xa0000240 0x00000000>;
> +		enable-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>;
> +		pwms = <&pwm3 0 571>;
> +		irq-gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>,
> +					<&gpio1 5 GPIO_ACTIVE_HIGH>;
> +	};
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 2c3fc512e746..1bf07b20a8af 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -35,6 +35,7 @@ aptina	Aptina Imaging
>  arasan	Arasan Chip Systems
>  archermind ArcherMind Technology (Nanjing) Co., Ltd.
>  arctic	Arctic Sand
> +arcx	Arcx/Archronix Inc.
>  aries	Aries Embedded GmbH
>  arm	ARM Ltd.
>  armadeus	ARMadeus Systems SARL
> -- 
> 2.17.1
> 

[PATCH anybus v1 2/4] dt-bindings: anybus-bridge: document devicetree binding.

[Sven Van Asbroeck]

This patch adds devicetree binding documentation for the
Arcx anybus bridge.

Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
---
 .../bindings/mfd/arcx,anybus-bridge.txt       | 37 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt   |  1 +
 2 files changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt

diff --git a/Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt b/Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt
new file mode 100644
index 000000000000..3c0399c4ed45
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/arcx,anybus-bridge.txt
@@ -0,0 +1,37 @@
+* Arcx anybus bridge
+
+This chip communicates with the SoC over the WEIM bus. It is
+expected that its Device Tree node is specified as the child of a node
+corresponding to the WEIM bus used for communication.
+
+Required properties:
+
+  - compatible : The following chip-specific string:
+        "arcx,anybus-bridge"
+
+  - reg :
+	weim memory area where the cpld registers are located,	followed by:
+	weim memory area of the first  anybus-s slot,		followed by:
+	weim memory area of the second anybus-s slot.
+
+  - enable-gpios : the gpio connected to the bridge's 'enable gpio'.
+
+  - pwms : the pwm connected to the bridge's 'pwm input'.
+
+  - irq-gpios : the gpios connected to the bridge's interrupt lines.
+	note that there is no need to provide the 'interrupts' property here.
+
+Example of usage:
+
+&weim {
+	bridge@0,0 {
+		compatible = "arcx,anybus-bridge";
+		reg = <0 0 0x100>, <0 0x400000 0x800>, <1 0x400000 0x800>;
+		fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
+				0x00000000 0xa0000240 0x00000000>;
+		enable-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>;
+		pwms = <&pwm3 0 571>;
+		irq-gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>,
+					<&gpio1 5 GPIO_ACTIVE_HIGH>;
+	};
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 2c3fc512e746..1bf07b20a8af 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -35,6 +35,7 @@ aptina	Aptina Imaging
 arasan	Arasan Chip Systems
 archermind ArcherMind Technology (Nanjing) Co., Ltd.
 arctic	Arctic Sand
+arcx	Arcx/Archronix Inc.
 aries	Aries Embedded GmbH
 arm	ARM Ltd.
 armadeus	ARMadeus Systems SARL
-- 
2.17.1