<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<br>
<div class="moz-cite-prefix">On 19/03/24 3:26 pm, Krzysztof
Kozlowski wrote:<br>
</div>
<blockquote type="cite"
cite="mid:bad5df79-e040-4868-9db6-701110894ea3@linaro.org">
<pre class="moz-quote-pre" wrap="">On 19/03/2024 10:34, Manojkiran Eda wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">This commit adds the device tree bindings for aspeed eSPI
controller.
Although aspeed eSPI hardware supports 4 different channels,
this commit only adds the support for flash channel, the
bindings for other channels could be upstreamed when the driver
support for those are added.
Signed-off-by: Manojkiran Eda <a class="moz-txt-link-rfc2396E" href="mailto:manojkiran.eda@gmail.com"><manojkiran.eda@gmail.com></a>
---
.../bindings/soc/aspeed/aspeed,espi.yaml | 94 +++++++++++++++++++
1 file changed, 94 insertions(+)
create mode 100644 Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
diff --git a/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml b/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
new file mode 100644
index 000000000000..3d3ad528e3b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Why Rob's comments got ignored?
This is not a soc component.</pre>
</blockquote>
<font size="2">I did not mean to ignore, i have few reasons listed
below that provides information on why i felt this belongs into
soc.</font><br>
<blockquote type="cite"
cite="mid:bad5df79-e040-4868-9db6-701110894ea3@linaro.org">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">@@ -0,0 +1,94 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# # Copyright (c) 2024 IBM Corporation.
+# # Copyright (c) 2021 Aspeed Technology Inc.
+%YAML 1.2
+---
+$id: <a class="moz-txt-link-freetext" href="http://devicetree.org/schemas/soc/aspeed/aspeed,espi.yaml#">http://devicetree.org/schemas/soc/aspeed/aspeed,espi.yaml#</a>
+$schema: <a class="moz-txt-link-freetext" href="http://devicetree.org/meta-schemas/core.yaml#">http://devicetree.org/meta-schemas/core.yaml#</a>
+
+title: Aspeed eSPI Controller
+
+maintainers:
+ - Manojkiran Eda <a class="moz-txt-link-rfc2396E" href="mailto:manojkiran.eda@gmail.com"><manojkiran.eda@gmail.com></a>
+ - Patrick Rudolph <a class="moz-txt-link-rfc2396E" href="mailto:patrick.rudolph@9elements.com"><patrick.rudolph@9elements.com></a>
+ - Chia-Wei Wang <a class="moz-txt-link-rfc2396E" href="mailto:chiawei_wang@aspeedtech.com"><chiawei_wang@aspeedtech.com></a>
+ - Ryan Chen <a class="moz-txt-link-rfc2396E" href="mailto:ryan_chen@aspeedtech.com"><ryan_chen@aspeedtech.com></a>
+
+description:
+ Aspeed eSPI controller implements a device side eSPI endpoint device
+ supporting the flash channel.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Explain what is eSPI.</pre>
</blockquote>
<font size="2">eSPI is a serial bus interface for client and server
platforms that is based on SPI, using the same master and slave
topology but operates with a different protocol to meet new
requirements. </font><font size="2">For instance, eSPI uses I/O,
or input/output, communication, instead of MOSI/MISO used in SPI.
It also includes a transaction layer on top of the SPI protocol,
defining packets such as command and response packets that allow
both the master and slave to initiate alert and reset signals.
eSPI supports communication between Embedded Controller (EC),
Baseboard Management Controller (BMC), Super-I/O (SIO) and Port-80
debug cards. I could add this to the commit message as well in the
next patchset.<br>
</font>
<blockquote type="cite"
cite="mid:bad5df79-e040-4868-9db6-701110894ea3@linaro.org">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+properties:
+ compatible:
+ items:
+ - enum:
+ - aspeed,ast2500-espi
+ - aspeed,ast2600-espi
+ - const: simple-mfd
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
That's not simple-mfd. You have driver for this. Drop.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ - const: syscon
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
That's not syscon. Why do you have ranges then? Where is any explanation
of hardware which would justify such combination?
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+ reg:
+ maxItems: 1
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 1
+
+ ranges: true
+
+patternProperties:
+ "^espi-ctrl@[0-9a-f]+$":
+ type: object
+
+ description: Controls the flash channel of eSPI hardware
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
That explains nothing. Unless you wanted to use here MTD bindings.
This binding did not improve much. I don't understand why this is not
SPI (nothing in commit msg, nothing in description), what is eSPI,</pre>
</blockquote>
<p><font size="2">eSPI uses Peripheral, Virtual Wire, Out of Band,
and Flash Access channels to communicate different sets of data.</font></p>
<ul>
<li><font size="2">The <strong>Peripheral</strong> Channel is
used for communication between eSPI host bridge located on the
master side and eSPI endpoints located on the slave side. LPC
Host and LPC Peripherals are an example of eSPI host bridge
and eSPI endpoints respectively.</font></li>
<li><font size="2"><strong>Virtual Wire</strong> Channel: The
Virtual Wire channel is used to communicate the state of
sideband pins or GPIO tunneled through eSPI as in-band
messages. Serial IRQ interrupts are communicated through this
channel as in-band messages.</font></li>
<li><font size="2"><strong>OOB</strong> Channel: The SMBus packets
are tunneled through eSPI as Out-Of-Band (OOB) messages. The
whole SMBus packet is embedded inside the eSPI OOB message as
data.</font></li>
<li><font size="2"><strong>Flash Access</strong> Channel: The
Flash Access channel provides a path allowing the flash
components to be shared run-time between chipset and the eSPI
slaves that require flash accesses such as EC (Embedded
Controller) and BMC.</font></li>
</ul>
<p><font size="2">Although , eSPI reuses the timing and electrical
specification of Serial Peripheral Interface (SPI) but it runs
an entirely different protocol to meet a set of different
requirements. Which is why i felt probably placing this in soc
was a better choice rather than spi. Do you think otherwise ?</font><br>
</p>
<blockquote type="cite"
cite="mid:bad5df79-e040-4868-9db6-701110894ea3@linaro.org">
<pre class="moz-quote-pre" wrap=""> why
do you need child device, what are other children (commit msg is quite
vague here). Why there is no MTD bindings here?
All this looks like crafted for your driver,</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""></pre>
<font size="2">Apologies, this was not my intention. I wanted this
to be as generic as possible. But i don't really have much
knowledge on what's the right way to model things in kernel at the
moment. Still trying to learn and understand by looking at various
other drivers. Appreciate all the feedback.</font>
<blockquote type="cite"
cite="mid:bad5df79-e040-4868-9db6-701110894ea3@linaro.org">
<pre class="moz-quote-pre" wrap=""> instead of using existing
DT bindings like SPI or MTD/NAND. This is a strong no-go.
</pre>
</blockquote>
<p></p>
<blockquote type="cite"
cite="mid:bad5df79-e040-4868-9db6-701110894ea3@linaro.org">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+ properties:
+ compatible:
+ items:
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
No items, just use enum.
</pre>
</blockquote>
<font size="2">sure, will fix it.</font><br>
<blockquote type="cite"
cite="mid:bad5df79-e040-4868-9db6-701110894ea3@linaro.org">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ - enum:
+ - aspeed,ast2500-espi-ctrl
+ - aspeed,ast2600-espi-ctrl
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ required:
+ - compatible
+ - interrupts
+ - clocks
+
+required:
+ - compatible
+ - reg
+ - "#address-cells"
+ - "#size-cells"
+ - ranges
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/clock/ast2600-clock.h>
+
+ espi: espi@1e6ee000 {
+ compatible = "aspeed,ast2600-espi", "simple-mfd", "syscon";
+ reg = <0x1e6ee000 0x1000>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x1e6ee000 0x1000>;
+
+ espi_ctrl: espi-ctrl@0 {
+ compatible = "aspeed,ast2600-espi-ctrl";
+ reg = <0x0 0x800>,<0x0 0x4000000>;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Fix your style in DTS. There is always a space after ','.</pre>
</blockquote>
<font size="2">sure , will fix that. Is there a link that could help
me understand various styling requirements on the DTS files. Also
is there any formatting tool available currently ? that could fix
the styling in the DTS files automatically rather than manual
inspection/modification. Did i accidentally missed running some
tool check ? </font><br>
<blockquote type="cite"
cite="mid:bad5df79-e040-4868-9db6-701110894ea3@linaro.org">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ reg-names = "espi_ctrl","espi_flash";
+ interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>;
+ };
+ };
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Best regards,
Krzysztof
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Krzysztof, <span
style="white-space: normal">Thanks for the review comments.
I am still figuring out few of the review comments (would need a little more time, since its my first attempt into kernel development) , but mean while I wanted to make sure if the direction of choosing "soc" vs "spi" was correct, so that i could re-work on the comments.So i have selectively answered to few of your comments. Could you let me know if the reasoning that was provided in reply to your comments help ?
Thanks,
Manoj
</span></pre>
</body>
</html>