The main goal of this patchset was to add Baikal-T1 AHCI SATA specifics
support into the kernel AHCI subsystem. On the way of doing that we
figured out that mainly these specifics are actually DWC AHCI SATA
controller features, but still there were some Baikal-T1 SoC platform
peculiarities which we had to take into account. So the patchset
introduces two AHCI SATA controllers support and one AHCI SATA driver
with a series of preparation, optimization and cleanup patches.
The series starts used to start with converting the legacy AHCI SATA
controllers text-based DT-bindings to the DT-schema. But turned out that
has already been done in kernel v5.17. So instead we suggest to improve
the bindings usability by splitting up the AHCI DT bindings into two
schemas: one common AHCI SATA controller yaml-file, which can be reused by
any AHCI-compatible controller utilizing the kernel AHCI library
functions, and DT-bindings for the generic AHCI SATA devices indicated by
the "generic-ahci" compatible string and implemented in the
ahci_platform.c driver. Note after doing that we had to fix the
sata-common.yaml file SATA port IDs constraint.
Then a series of generic preparations-cleanups goes. First of all it
concerns the device-managed methods usage in the framework of the CSR
space remapping and the clocks requesting and enabling. Note since the
clocks handlers are requested and kept in the generic AHCI library it
seemed a good idea to add an AHCI-platform generic method to find and get
a particular clock handler from the pool of the requested ones. It was
used later in the series in the DWC/Baikal-T1-specific code. Secondly we
suggested to at least sanity check the number of SATA ports DT-sub-nodes
before using it further. Thirdly the ports-implemented DT-property
parsing was moved from the AHCI platform-driver to the AHCI-library so to
be used by the non-generic AHCI drivers if required (DT-schema is
accordingly fixed too). Finally due to having the shared-reset control
support we had to add a new AHCI-resource getter flag -
AHCI_PLATFORM_RST_TRIGGER, which indicated using a trigger-like reset
control. For such platforms the controller reset will be performed by
means of the reset_control_reset() and reset_control_rearm() methods.
AHCI-library reset functions encapsulating the way the reset procedure is
performed have been also added.
After that goes a patches series with the platform-specific
AHCI-capabilities initialization. The suggested functionality will be
useful for the platforms with no BIOS, comprehensive bootloader/firmware
installed. In that case the AHCI-related platform-specifics like drive
staggered spin-up, mechanical presence switch attached or FIS-based
switching capability usage, etc will be left uninitialized with no generic
way to be indicated as available if required. We suggested to use the AHCI
device tree node and its ports sub-nodes for that. AHCI-platform library
will be responsible fo the corresponding DT-properties parsing and
pre-initialization of the internal capability registers cache, which will
be then flashed back to the corresponding CSR after HBA reset. Thus a
supposed to be firmware-work will be done by means of the AHCI-library and
the DT-data. A set of the preparations/cleanups required to be done before
introducing the feature. First the DT-properties indicating the
corresponding capability availability were described in the common AHCI
DT-binding schema. Second we needed to add the enum items with the AHCI
Port CMD fields, which hadn't been added so far. Thirdly we suggested to
discard one of the port-map internal storage (force_port_map) in favor of
re-using another one (save_port_map) in order to simplify the port-map
initialization interface a bit by getting rid from a redundant variable.
Finally after discarding the double AHCI-version read procedure and
changing the __ahci_port_base() method prototype the platform
firmware-specific caps initialization functionality was introduced.
The main part of the series goes afterwards. A dedicated DWC AHCI SATA
controller driver was introduced together with the corresponding
DT-binding schema pre-patch. Note the driver built mode is activated
synchronously with the generic AHCI-platform driver by default so
automatically to be integrated into the kernel for the DWC AHCI-based
platforms which relied on activating the generic AHCI SATA controller
driver. Aside with the generic resources getting and AHCI-host
initialization, the driver implements the DWC-specific setups. In
particular it checks whether the platform capabilities activated by the
firmware (see the functionality described above) are actually supported by
the controller. It's done by means of the vendor-specific registers. Then
it makes sure that the embedded 1ms timer interval, which is used for the
DevSleep and CCC features, is correctly initialized based on the
application clock rate. The last but not least the driver provides a way
to tune the DMA-interface performance up by setting the Tx/Rx transactions
maximum size up. The required values are specified by means of the
"snps,tx-ts-max" and snps,rx-ts-max" DT-properties.
Finally we suggest to extend the DWC AHCI SATA controller driver
functionality with a way to add the DWC-AHCI-based platform-specific
quirks. Indeed there are many DWC AHCI-based controllers and just a few of
them are diverged too much to be handled by a dedicated AHCI-driver. The
rest of them most likely can work well either with a generic version of
the driver or require a simple normally platform-specific quirk to get up
and running. Such platforms can define a platform-data in the DWC AHCI
driver with a set of the controller-specific flags and initialization
functions. Those functions will be called at the corresponding stages of
the device probe/resume/remove procedures so to be performing the platform
setups/cleanups.
After the denoted above functionality is added we can finally introduce
the Baikal-T1 AHCI SATA controller support into the DWC AHCI SATA driver.
The controller is based on the DWC AHCI SATA IP-core v4.10a and can work
well with the generic DWC AHCI driver. The only peculiarity of it is
connected with the SATA Ports reference clock source. It can be supplied
either from the internal SoC PLL or from the chip pads. Currently we have
to prefer selecting the signal coming from the pads if the corresponding
clock source is specified because the link doesn't get stably established
when the internal clock signal is activated. In addition the platform has
trigger-based reset signals so the corresponding flag must be passed to
the generic AHCI-resource getter.
Link: https://lore.kernel.org/linux-ide/[email protected]/
Changelog v2:
- Rebase from kernel v5.17 to v5.18-rc3. (@Rob)
- Rebase onto the already available AHCI DT schema. As a result two more
patches have been added. (@Rob)
- Rename 'syscon' property to 'baikal,bt1-syscon'. (@Rob)
- Replace min/max constraints of the snps,{tx,rx}-ts-max property with
enum [ 1, 2, 4, ..., 1024 ]. (@Rob)
- Use dlemoal/libata.git git tree for the LIBATA SATA AHCI SYNOPSYS
DWC driver (@Damien).
- Change the local objects prefix from 'dwc_ahci_' to 'ahci_dwc_',
from 'bt1_ahci_' to 'ahci_bt1_'. (@Damien)
- Use LLDD term in place of 'glue-driver'. (@Damien)
- Convert the ahci_platform_assert_rsts() method to returning int status
(@Damien).
- Drop the else word from the DT child_nodes value checking if-else-if
statement (@Damien) and convert the after-else part into the ternary
operator-based statement.
- Convert to checking the error-case first in the devm_clk_bulk_get_all()
method invocation. (@Damien)
- Drop the rc variable initialization in the ahci_platform_get_resources()
method. (@Damien)
- Add comma and replace "channel" with "SATA port" in the reg property
description of the sata-common.yaml schema. (@Damien)
Link: https://lore.kernel.org/lkml/[email protected]/
Changelog v3:
- Replace Jens's email address with Damien's one in the list of the
common DT schema maintainers. (@Damien)
Link: https://lore.kernel.org/linux-ide/[email protected]/
Changelog v4:
- Drop clocks, clock-names, resets, reset-names and power-domains
properties from the AHCI common schema. (@Rob)
- Make sure the interrupts DT-property can have from 1 to 32 items
specified. (@Rob)
- Decrease the "additionalProperties" property identation in the DW AHCI
SATA DT-schema otherwise it's percieved as the node property instead of
the key one. (@Rob)
- Convert the HBA-capabilities boolean properties to the bitfield
DT-properties. (@Rob)
- Create SATA/AHCI-port properties definition hierarchy so the sub-schemas
could inherit and extend the ports properties of the super-schema. (@Rob)
- Drop Baikal-T1 syscon reference and implement the clock signal
source in the framework of the clock controller. (@Rob)
- Refactor the patch
[PATCH v3 01/23] dt-bindings: ata: ahci-platform: Drop dma-coherent property declaration
to
[PATCH v3 01/23] dt-bindings: ata: ahci-platform: Move dma-coherent to sata-common.yaml
(@Rob)
- Add a new patch:
[PATCH v4 05/24] dt-bindings: ata: sata-brcm: Apply common AHCI schema
- Drop the patch:
[PATCH v3 05/23] ata: libahci_platform: Explicitly set rc on devres_alloc failure
(@Hannes, @Damien)
- Convert ahci_dwc_plat and ahci_bt1_plat to being statically defined.
(@kbot)
- Rebase onto the kernel v5.18.
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Pavel Parkhomenko <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Serge Semin (23):
dt-bindings: ata: ahci-platform: Move dma-coherent to sata-common.yaml
dt-bindings: ata: ahci-platform: Detach common AHCI bindings
dt-bindings: ata: ahci-platform: Clarify common AHCI props constraints
dt-bindings: ata: sata: Extend number of SATA ports
dt-bindings: ata: sata-brcm: Apply common AHCI schema
ata: libahci_platform: Convert to using platform devm-ioremap methods
ata: libahci_platform: Convert to using devm bulk clocks API
ata: libahci_platform: Sanity check the DT child nodes number
ata: libahci_platform: Parse ports-implemented property in resources
getter
ata: libahci_platform: Introduce reset assertion/deassertion methods
dt-bindings: ata: ahci: Add platform capability properties
ata: libahci: Extend port-cmd flags set with port capabilities
ata: libahci: Discard redundant force_port_map parameter
ata: libahci: Don't read AHCI version twice in the save-config method
ata: ahci: Convert __ahci_port_base to accepting hpriv as arguments
ata: ahci: Introduce firmware-specific caps initialization
dt-bindings: ata: ahci: Add DWC AHCI SATA controller DT schema
ata: libahci_platform: Add function returning a clock-handle by id
ata: ahci: Add DWC AHCI SATA controller support
dt-bindings: ata: ahci: Add Baikal-T1 AHCI SATA controller DT schema
ata: ahci-dwc: Add platform-specific quirks support
ata: ahci-dwc: Add Baikal-T1 AHCI SATA interface support
MAINTAINERS: Add maintainers for DWC AHCI SATA driver
.../devicetree/bindings/ata/ahci-common.yaml | 123 +++++
.../bindings/ata/ahci-platform.yaml | 92 +---
.../bindings/ata/baikal,bt1-ahci.yaml | 116 ++++
.../bindings/ata/brcm,sata-brcm.yaml | 4 +-
.../devicetree/bindings/ata/sata-common.yaml | 17 +-
.../bindings/ata/snps,dwc-ahci.yaml | 129 +++++
MAINTAINERS | 9 +
drivers/ata/Kconfig | 11 +
drivers/ata/Makefile | 1 +
drivers/ata/ahci.c | 4 +-
drivers/ata/ahci.h | 21 +-
drivers/ata/ahci_dwc.c | 494 ++++++++++++++++++
drivers/ata/ahci_mtk.c | 2 -
drivers/ata/ahci_platform.c | 5 -
drivers/ata/ahci_st.c | 3 -
drivers/ata/libahci.c | 63 ++-
drivers/ata/libahci_platform.c | 222 ++++++--
include/dt-bindings/ata/ahci.h | 20 +
include/linux/ahci_platform.h | 8 +-
19 files changed, 1174 insertions(+), 170 deletions(-)
create mode 100644 Documentation/devicetree/bindings/ata/ahci-common.yaml
create mode 100644 Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml
create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
create mode 100644 drivers/ata/ahci_dwc.c
create mode 100644 include/dt-bindings/ata/ahci.h
--
2.35.1
There are systems with no BIOS or comprehensive embedded firmware which
could be able to properly initialize the SATA AHCI controller
platform-specific capabilities. In that case a good alternative to having
a clever bootloader is to create a device tree node with the properties
well describing all the AHCI-related platform specifics. All the settings
which are normally detected and marked as available in the HBA and its
ports capabilities fields [1] could be defined in the platform DTB by
means of a set of the dedicated properties. Such approach perfectly fits
to the DTB-philosophy - to provide hardware/platform description.
So here we suggest to extend the SATA AHCI device tree bindings with two
additional DT-properties:
1) "hba-cap" - HBA platform generic capabilities like:
- SSS - Staggered Spin-up support.
- SMPS - Mechanical Presence Switch support.
2) "hba-port-cap" - HBA platform port capabilities like:
- HPCP - Hot Plug Capable Port.
- MPSP - Mechanical Presence Switch Attached to Port.
- CPD - Cold Presence Detection.
- ESP - External SATA Port.
- FBSCP - FIS-based Switching Capable Port.
All of these capabilities require to have a corresponding hardware
configuration. Thus it's ok to have them defined in DTB.
Even though the driver currently takes into account the state of the ESP
and FBSCP flags state only, there is nothing wrong with having all of them
supported by the generic AHCI library in order to have a complete OF-based
platform-capabilities initialization procedure. These properties will be
parsed in the ahci_platform_get_resources() method and their values will
be stored in the saved_* fields of the ahci_host_priv structure, which in
its turn then will be used to restore the H.CAP, H.PI and P#.CMD
capability fields on device init and after HBA reset.
Please note this modification concerns the HW-init HBA and its ports flags
only, which are by specification [1] are supposed to be initialized by the
BIOS/platform firmware/expansion ROM and which are normally declared in
the one-time-writable-after-reset register fields. Even though these flags
aren't supposed to be cleared after HBA reset some AHCI instances may
violate that rule so we still need to perform the fields resetting after
each reset. Luckily the corresponding functionality has already been
partly implemented in the framework of the ahci_save_initial_config() and
ahci_restore_initial_config() methods.
[1] Serial ATA AHCI 1.3.1 Specification, p. 103
Signed-off-by: Serge Semin <[email protected]>
---
Changelog v4:
- Convert the boolean properties to the bitfield DT-properties. (@Rob)
---
drivers/ata/ahci.h | 1 +
drivers/ata/libahci.c | 51 ++++++++++++++++++++++++++++------
drivers/ata/libahci_platform.c | 41 +++++++++++++++++++++++++--
3 files changed, 82 insertions(+), 11 deletions(-)
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 8b9826533ae5..0de221055961 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -337,6 +337,7 @@ struct ahci_host_priv {
u32 saved_cap; /* saved initial cap */
u32 saved_cap2; /* saved initial cap2 */
u32 saved_port_map; /* saved initial port_map */
+ u32 saved_port_cap[AHCI_MAX_PORTS]; /* saved port_cap */
u32 em_loc; /* enclosure management location */
u32 em_buf_sz; /* EM buffer size in byte */
u32 em_msg_type; /* EM message type */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 1ffaa5f5f21a..954386a2b500 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -16,6 +16,7 @@
* http://www.intel.com/technology/serialata/pdf/rev1_1.pdf
*/
+#include <linux/bitops.h>
#include <linux/kernel.h>
#include <linux/gfp.h>
#include <linux/module.h>
@@ -443,16 +444,28 @@ static ssize_t ahci_show_em_supported(struct device *dev,
void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
{
void __iomem *mmio = hpriv->mmio;
- u32 cap, cap2, vers, port_map;
+ void __iomem *port_mmio;
+ unsigned long port_map;
+ u32 cap, cap2, vers;
int i;
/* make sure AHCI mode is enabled before accessing CAP */
ahci_enable_ahci(mmio);
- /* Values prefixed with saved_ are written back to host after
- * reset. Values without are used for driver operation.
+ /*
+ * Values prefixed with saved_ are written back to the HBA and ports
+ * registers after reset. Values without are used for driver operation.
+ */
+
+ /*
+ * Override HW-init HBA capability fields with the platform-specific
+ * values. The rest of the HBA capabilities are defined as Read-only
+ * and can't be modified in CSR anyway.
*/
- hpriv->saved_cap = cap = readl(mmio + HOST_CAP);
+ cap = readl(mmio + HOST_CAP);
+ if (hpriv->saved_cap)
+ cap = (cap & ~(HOST_CAP_SSS | HOST_CAP_MPS)) | hpriv->saved_cap;
+ hpriv->saved_cap = cap;
/* CAP2 register is only defined for AHCI 1.2 and later */
vers = readl(mmio + HOST_VERSION);
@@ -519,7 +532,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
/* Override the HBA ports mapping if the platform needs it */
port_map = readl(mmio + HOST_PORTS_IMPL);
if (hpriv->saved_port_map && port_map != hpriv->saved_port_map) {
- dev_info(dev, "forcing port_map 0x%x -> 0x%x\n",
+ dev_info(dev, "forcing port_map 0x%lx -> 0x%x\n",
port_map, hpriv->saved_port_map);
port_map = hpriv->saved_port_map;
} else {
@@ -527,7 +540,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
}
if (hpriv->mask_port_map) {
- dev_warn(dev, "masking port_map 0x%x -> 0x%x\n",
+ dev_warn(dev, "masking port_map 0x%lx -> 0x%lx\n",
port_map,
port_map & hpriv->mask_port_map);
port_map &= hpriv->mask_port_map;
@@ -546,7 +559,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
*/
if (map_ports > ahci_nr_ports(cap)) {
dev_warn(dev,
- "implemented port map (0x%x) contains more ports than nr_ports (%u), using nr_ports\n",
+ "implemented port map (0x%lx) contains more ports than nr_ports (%u), using nr_ports\n",
port_map, ahci_nr_ports(cap));
port_map = 0;
}
@@ -555,12 +568,26 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
/* fabricate port_map from cap.nr_ports for < AHCI 1.3 */
if (!port_map && vers < 0x10300) {
port_map = (1 << ahci_nr_ports(cap)) - 1;
- dev_warn(dev, "forcing PORTS_IMPL to 0x%x\n", port_map);
+ dev_warn(dev, "forcing PORTS_IMPL to 0x%lx\n", port_map);
/* write the fixed up value to the PI register */
hpriv->saved_port_map = port_map;
}
+ /*
+ * Preserve the ports capabilities defined by the platform. Note there
+ * is no need in storing the rest of the P#.CMD fields since they are
+ * volatile.
+ */
+ for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
+ if (hpriv->saved_port_cap[i])
+ continue;
+
+ port_mmio = __ahci_port_base(hpriv, i);
+ hpriv->saved_port_cap[i] =
+ readl(port_mmio + PORT_CMD) & PORT_CMD_CAP;
+ }
+
/* record values to use during operation */
hpriv->cap = cap;
hpriv->cap2 = cap2;
@@ -590,13 +617,21 @@ EXPORT_SYMBOL_GPL(ahci_save_initial_config);
static void ahci_restore_initial_config(struct ata_host *host)
{
struct ahci_host_priv *hpriv = host->private_data;
+ unsigned long port_map = hpriv->port_map;
void __iomem *mmio = hpriv->mmio;
+ void __iomem *port_mmio;
+ int i;
writel(hpriv->saved_cap, mmio + HOST_CAP);
if (hpriv->saved_cap2)
writel(hpriv->saved_cap2, mmio + HOST_CAP2);
writel(hpriv->saved_port_map, mmio + HOST_PORTS_IMPL);
(void) readl(mmio + HOST_PORTS_IMPL); /* flush */
+
+ for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
+ port_mmio = __ahci_port_base(hpriv, i);
+ writel(hpriv->saved_port_cap[i], port_mmio + PORT_CMD);
+ }
}
static unsigned ahci_scr_offset(struct ata_port *ap, unsigned int sc_reg)
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index efe640603f3f..8b542a8bc487 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -23,6 +23,7 @@
#include <linux/pm_runtime.h>
#include <linux/of_platform.h>
#include <linux/reset.h>
+
#include "ahci.h"
static void ahci_host_stop(struct ata_host *host);
@@ -383,6 +384,34 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
return rc;
}
+static int ahci_platform_get_firmware(struct ahci_host_priv *hpriv,
+ struct device *dev)
+{
+ struct device_node *child;
+ u32 port;
+
+ if (!of_property_read_u32(dev->of_node, "hba-cap", &hpriv->saved_cap))
+ hpriv->saved_cap &= (HOST_CAP_SSS | HOST_CAP_MPS);
+
+ of_property_read_u32(dev->of_node,
+ "ports-implemented", &hpriv->saved_port_map);
+
+ for_each_child_of_node(dev->of_node, child) {
+ if (!of_device_is_available(child))
+ continue;
+
+ if (of_property_read_u32(child, "reg", &port)) {
+ of_node_put(child);
+ return -EINVAL;
+ }
+
+ if (!of_property_read_u32(child, "hba-port-cap", &hpriv->saved_port_cap[port]))
+ hpriv->saved_port_cap[port] &= PORT_CMD_CAP;
+ }
+
+ return 0;
+}
+
/**
* ahci_platform_get_resources - Get platform resources
* @pdev: platform device to get resources for
@@ -523,9 +552,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
goto err_out;
}
- of_property_read_u32(dev->of_node,
- "ports-implemented", &hpriv->saved_port_map);
-
if (child_nodes) {
for_each_child_of_node(dev->of_node, child) {
u32 port;
@@ -590,6 +616,15 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
if (rc == -EPROBE_DEFER)
goto err_out;
}
+
+ /*
+ * Retrieve firmware-specific flags which then will be used to set
+ * the HW-init fields of HBA and its ports
+ */
+ rc = ahci_platform_get_firmware(hpriv, dev);
+ if (rc)
+ goto err_out;
+
pm_runtime_enable(dev);
pm_runtime_get_sync(dev);
hpriv->got_runtime_pm = true;
--
2.35.1
In case if the platform doesn't have BIOS or a comprehensive firmware
installed then the HBA capability flags will be left uninitialized. As a
good alternative we suggest to define the DT-properties with the AHCI
platform capabilities describing all the HW-init flags of the
corresponding capability register. Luckily there aren't too many of them.
SSS - Staggered Spin-up support and MPS - Mechanical Presence Switch
support determine the corresponding feature availability for the whole HBA
by means of the "hba-cap" property. Each port can have the "hba-port-cap"
property initialized indicating that the port supports some of the next
functionalities: HPCP - HotPlug capable port, MPSP - Mechanical Presence
Switch attached to a port, CPD - Cold Plug detection, ESP - External SATA
Port (eSATA), FBSCP - FIS-based switching capable port.
Signed-off-by: Serge Semin <[email protected]>
---
Changelog v4:
- Fix some misspelling in the patch log.
- Convert the boolean properties to the bitfield properties. (@Rob)
- Remove Hannes' rb tag due to the patch content change.
---
.../devicetree/bindings/ata/ahci-common.yaml | 16 +++++++++++++++
.../bindings/ata/ahci-platform.yaml | 10 ++++++++++
include/dt-bindings/ata/ahci.h | 20 +++++++++++++++++++
3 files changed, 46 insertions(+)
create mode 100644 include/dt-bindings/ata/ahci.h
diff --git a/Documentation/devicetree/bindings/ata/ahci-common.yaml b/Documentation/devicetree/bindings/ata/ahci-common.yaml
index 12a97b56226f..94d72aeaad0f 100644
--- a/Documentation/devicetree/bindings/ata/ahci-common.yaml
+++ b/Documentation/devicetree/bindings/ata/ahci-common.yaml
@@ -58,6 +58,14 @@ properties:
phy-names:
const: sata-phy
+ hba-cap:
+ $ref: '/schemas/types.yaml#/definitions/uint32'
+ description:
+ Bitfield of the HBA generic platform capabilities like Staggered
+ Spin-up or Mechanical Presence Switch support. It can be used to
+ appropriately initialize the HWinit fields of the HBA CAP register
+ in case if the system firmware hasn't done it.
+
ports-implemented:
$ref: '/schemas/types.yaml#/definitions/uint32'
description:
@@ -101,6 +109,14 @@ $defs:
target-supply:
description: Power regulator for SATA port target device
+ hba-port-cap:
+ $ref: '/schemas/types.yaml#/definitions/uint32'
+ description:
+ Bitfield of the HBA port-specific platform capabilities like Hot
+ plugging, eSATA, FIS-based Switching, etc (see AHCI specification
+ for details). It can be used to initialize the HWinit fields of
+ the PxCMD register in case if the system firmware hasn't done it.
+
required:
- reg
diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
index 15be98e0385b..e19cf9828e68 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
@@ -111,6 +111,8 @@ examples:
- |
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/clock/berlin2q.h>
+ #include <dt-bindings/ata/ahci.h>
+
sata@f7e90000 {
compatible = "marvell,berlin2q-ahci", "generic-ahci";
reg = <0xf7e90000 0x1000>;
@@ -119,15 +121,23 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
+ hba-cap = <HBA_SMPS>;
+
sata0: sata-port@0 {
reg = <0>;
+
phys = <&sata_phy 0>;
target-supply = <®_sata0>;
+
+ hba-port-cap = <(HBA_PORT_FBSCP | HBA_PORT_ESP)>;
};
sata1: sata-port@1 {
reg = <1>;
+
phys = <&sata_phy 1>;
target-supply = <®_sata1>;
+
+ hba-port-cap = <(HBA_PORT_HPCP | HBA_PORT_MPSP | HBA_PORT_FBSCP)>;
};
};
diff --git a/include/dt-bindings/ata/ahci.h b/include/dt-bindings/ata/ahci.h
new file mode 100644
index 000000000000..6841caebcedf
--- /dev/null
+++ b/include/dt-bindings/ata/ahci.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This header provides constants for most AHCI bindings.
+ */
+
+#ifndef _DT_BINDINGS_ATA_AHCI_H
+#define _DT_BINDINGS_ATA_AHCI_H
+
+/* Host Bus Adapter generic platform capabilities */
+#define HBA_SSS (1 << 27)
+#define HBA_SMPS (1 << 28)
+
+/* Host Bus Adapter port-specific platform capabilities */
+#define HBA_PORT_HPCP (1 << 18)
+#define HBA_PORT_MPSP (1 << 19)
+#define HBA_PORT_CPD (1 << 20)
+#define HBA_PORT_ESP (1 << 21)
+#define HBA_PORT_FBSCP (1 << 22)
+
+#endif
--
2.35.1
Since all the clocks are retrieved by the method
ahci_platform_get_resources() there is no need for the LLD (glue) drivers
to be looking for some particular of them in the kernel clocks table
again. Instead we suggest to add a simple method returning a
device-specific clock with passed connection ID if it is managed to be
found. Otherwise the function will return NULL. Thus the glue-drivers
won't need to either manually touching the hpriv->clks array or calling
clk_get()-friends. The AHCI platform drivers will be able to use the new
function right after the ahci_platform_get_resources() method invocation
and up to the device removal.
Note the method is left unused here, but will be utilized in the framework
of the DWC AHCI SATA driver being added in the next commit.
Signed-off-by: Serge Semin <[email protected]>
---
Changelog v2:
- Fix some grammar mistakes in the method description.
Changelog v4:
- Add a note regarding the new method usage.
---
drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++
include/linux/ahci_platform.h | 3 +++
2 files changed, 30 insertions(+)
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 8b542a8bc487..418961f954af 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -95,6 +95,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
}
EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);
+/**
+ * ahci_platform_find_clk - Find platform clock
+ * @hpriv: host private area to store config values
+ * @con_id: clock connection ID
+ *
+ * This function returns a pointer to the clock descriptor of the clock with
+ * the passed ID.
+ *
+ * RETURNS:
+ * Pointer to the clock descriptor on success otherwise NULL
+ */
+struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id)
+{
+ struct clk *clk = NULL;
+ int i;
+
+ for (i = 0; i < hpriv->n_clks; i++) {
+ if (!strcmp(hpriv->clks[i].id, con_id)) {
+ clk = hpriv->clks[i].clk;
+ break;
+ }
+ }
+
+ return clk;
+}
+EXPORT_SYMBOL_GPL(ahci_platform_find_clk);
+
/**
* ahci_platform_enable_clks - Enable platform clocks
* @hpriv: host private area to store config values
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 6d7dd472d370..3418980b0341 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -13,6 +13,7 @@
#include <linux/compiler.h>
+struct clk;
struct device;
struct ata_port_info;
struct ahci_host_priv;
@@ -21,6 +22,8 @@ struct scsi_host_template;
int ahci_platform_enable_phys(struct ahci_host_priv *hpriv);
void ahci_platform_disable_phys(struct ahci_host_priv *hpriv);
+struct clk *
+ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id);
int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
int ahci_platform_deassert_rsts(struct ahci_host_priv *hpriv);
--
2.35.1
Indeed in accordance with what is implemented in the AHCI platform driver
and the way the AHCI DT nodes are defined in the DT files we can add the
next AHCI DT properties constraints: AHCI CSR ID is fixed to 'ahci', PHY
name is fixed to 'sata-phy', AHCI controller can't have more than 32 ports
by design, AHCI controller can have up to 32 IRQ lines.
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Hannes Reinecke <[email protected]>
---
Changelog v2:
- This is a new patch created after rebasing v1 onto the 5.18-rc3 kernel.
Changelog v4:
- Fix spelling: 'imeplemtned' and 'paltform' in the patch log. (@Hannes)
- Add the interrupts property constraints. (@Rob)
- Add forgotten '---' patchlog-changelog separator. (@Sergei)
---
.../devicetree/bindings/ata/ahci-common.yaml | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/ata/ahci-common.yaml b/Documentation/devicetree/bindings/ata/ahci-common.yaml
index e89bda3b62cc..12a97b56226f 100644
--- a/Documentation/devicetree/bindings/ata/ahci-common.yaml
+++ b/Documentation/devicetree/bindings/ata/ahci-common.yaml
@@ -31,12 +31,16 @@ properties:
reg-names:
description: CSR space IDs
+ contains:
+ const: ahci
interrupts:
description:
Generic AHCI state change interrupt. Can be implemented either as a
single line attached to the controller or as a set of the signals
indicating the particular port events.
+ minItems: 1
+ maxItems: 32
ahci-supply:
description: Power regulator for AHCI controller
@@ -52,14 +56,13 @@ properties:
maxItems: 1
phy-names:
- maxItems: 1
+ const: sata-phy
ports-implemented:
$ref: '/schemas/types.yaml#/definitions/uint32'
description:
Mask that indicates which ports the HBA supports. Useful if PI is not
programmed by the BIOS, which is true for some embedded SoC's.
- maximum: 0x1f
patternProperties:
"^sata-port@[0-9a-f]+$":
@@ -80,8 +83,12 @@ $defs:
properties:
reg:
- description: AHCI SATA port identifier
- maxItems: 1
+ description:
+ AHCI SATA port identifier. By design AHCI controller can't have
+ more than 32 ports due to the CAP.NP fields and PI register size
+ constraints.
+ minimum: 0
+ maximum: 31
phys:
description: Individual AHCI SATA port PHY
@@ -89,7 +96,7 @@ $defs:
phy-names:
description: AHCI SATA port PHY ID
- maxItems: 1
+ const: sata-phy
target-supply:
description: Power regulator for SATA port target device
--
2.35.1
The Broadcom SATA controller is obviously based on the AHCI standard. The
device driver uses the kernel AHCI library to work with it. Therefore we
can be have a more thorough DT-bindings evaluation by referring to the
AHCI-common schema instead of using the more relaxed SATA-common one.
Signed-off-by: Serge Semin <[email protected]>
---
Changelog v4:
- This is a new patch added on v4 lap of the review procedure.
---
Documentation/devicetree/bindings/ata/brcm,sata-brcm.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/ata/brcm,sata-brcm.yaml b/Documentation/devicetree/bindings/ata/brcm,sata-brcm.yaml
index 4ee74df8e58a..fa8ebc8f243f 100644
--- a/Documentation/devicetree/bindings/ata/brcm,sata-brcm.yaml
+++ b/Documentation/devicetree/bindings/ata/brcm,sata-brcm.yaml
@@ -14,7 +14,7 @@ maintainers:
- Florian Fainelli <[email protected]>
allOf:
- - $ref: sata-common.yaml#
+ - $ref: ahci-common.yaml#
properties:
compatible:
--
2.35.1
Currently not all of the Port-specific capabilities listed in the
PORT_CMD-enumeration. Let's extend that set with the Cold Presence
Detection and Mechanical Presence Switch attached to the Port flags [1] so
to closeup the set of the platform-specific port-capabilities flags. Note
these flags are supposed to be set by the platform firmware if there is
one. Alternatively as we are about to do they can be set by means of the
OF properties.
While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DMPS and fix the
comment there. In accordance with [2] that IRQ flag is supposed to
indicate the state of the signal coming from the Mechanical Presence
Switch.
[1] Serial ATA AHCI 1.3.1 Specification, p.27
[2] Serial ATA AHCI 1.3.1 Specification, p.24, p.88
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Hannes Reinecke <[email protected]>
---
Changelog v4:
- Fix the DMPS macros name in the patch log. (@Sergei Shtylyov)
---
drivers/ata/ahci.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 7d834deefeb9..f501531bd1b3 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -138,7 +138,7 @@ enum {
PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */
PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */
- PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */
+ PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */
PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */
PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */
PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */
@@ -166,6 +166,8 @@ enum {
PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
PORT_CMD_FBSCP = (1 << 22), /* FBS Capable Port */
PORT_CMD_ESP = (1 << 21), /* External Sata Port */
+ PORT_CMD_CPD = (1 << 20), /* Cold Presence Detection */
+ PORT_CMD_MPSP = (1 << 19), /* Mechanical Presence Switch */
PORT_CMD_HPCP = (1 << 18), /* HotPlug Capable Port */
PORT_CMD_PMP = (1 << 17), /* PMP attached */
PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
@@ -181,6 +183,9 @@ enum {
PORT_CMD_ICC_PARTIAL = (0x2 << 28), /* Put i/f in partial state */
PORT_CMD_ICC_SLUMBER = (0x6 << 28), /* Put i/f in slumber state */
+ PORT_CMD_CAP = PORT_CMD_HPCP | PORT_CMD_MPSP |
+ PORT_CMD_CPD | PORT_CMD_ESP | PORT_CMD_FBSCP,
+
/* PORT_FBS bits */
PORT_FBS_DWE_OFFSET = 16, /* FBS device with error offset */
PORT_FBS_ADO_OFFSET = 12, /* FBS active dev optimization offset */
--
2.35.1
On 6/10/22 17:17, Serge Semin wrote:
> Currently not all of the Port-specific capabilities listed in the
s/listed/are listed
> PORT_CMD-enumeration. Let's extend that set with the Cold Presence
> Detection and Mechanical Presence Switch attached to the Port flags [1] so
> to closeup the set of the platform-specific port-capabilities flags. Note
> these flags are supposed to be set by the platform firmware if there is
> one. Alternatively as we are about to do they can be set by means of the
> OF properties.
>
> While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DMPS and fix the
> comment there. In accordance with [2] that IRQ flag is supposed to
> indicate the state of the signal coming from the Mechanical Presence
> Switch.
>
> [1] Serial ATA AHCI 1.3.1 Specification, p.27
> [2] Serial ATA AHCI 1.3.1 Specification, p.24, p.88
>
> Signed-off-by: Serge Semin <[email protected]>
> Reviewed-by: Hannes Reinecke <[email protected]>
>
> ---
>
> Changelog v4:
> - Fix the DMPS macros name in the patch log. (@Sergei Shtylyov)
> ---
> drivers/ata/ahci.h | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 7d834deefeb9..f501531bd1b3 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -138,7 +138,7 @@ enum {
> PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */
>
> PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */
> - PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */
> + PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */
> PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */
> PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */
> PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */
> @@ -166,6 +166,8 @@ enum {
> PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
> PORT_CMD_FBSCP = (1 << 22), /* FBS Capable Port */
> PORT_CMD_ESP = (1 << 21), /* External Sata Port */
> + PORT_CMD_CPD = (1 << 20), /* Cold Presence Detection */
> + PORT_CMD_MPSP = (1 << 19), /* Mechanical Presence Switch */
> PORT_CMD_HPCP = (1 << 18), /* HotPlug Capable Port */
> PORT_CMD_PMP = (1 << 17), /* PMP attached */
> PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
> @@ -181,6 +183,9 @@ enum {
> PORT_CMD_ICC_PARTIAL = (0x2 << 28), /* Put i/f in partial state */
> PORT_CMD_ICC_SLUMBER = (0x6 << 28), /* Put i/f in slumber state */
>
> + PORT_CMD_CAP = PORT_CMD_HPCP | PORT_CMD_MPSP |
> + PORT_CMD_CPD | PORT_CMD_ESP | PORT_CMD_FBSCP,
What is this one for ? A comment above it would be nice.
> +
> /* PORT_FBS bits */
> PORT_FBS_DWE_OFFSET = 16, /* FBS device with error offset */
> PORT_FBS_ADO_OFFSET = 12, /* FBS active dev optimization offset */
--
Damien Le Moal
Western Digital Research
On 6/10/22 17:17, Serge Semin wrote:
> Since all the clocks are retrieved by the method
> ahci_platform_get_resources() there is no need for the LLD (glue) drivers
> to be looking for some particular of them in the kernel clocks table
> again. Instead we suggest to add a simple method returning a
> device-specific clock with passed connection ID if it is managed to be
> found. Otherwise the function will return NULL. Thus the glue-drivers
> won't need to either manually touching the hpriv->clks array or calling
> clk_get()-friends. The AHCI platform drivers will be able to use the new
> function right after the ahci_platform_get_resources() method invocation
> and up to the device removal.
>
> Note the method is left unused here, but will be utilized in the framework
> of the DWC AHCI SATA driver being added in the next commit.
>
> Signed-off-by: Serge Semin <[email protected]>
>
> ---
>
> Changelog v2:
> - Fix some grammar mistakes in the method description.
>
> Changelog v4:
> - Add a note regarding the new method usage.
> ---
> drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++
> include/linux/ahci_platform.h | 3 +++
> 2 files changed, 30 insertions(+)
>
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 8b542a8bc487..418961f954af 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -95,6 +95,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
> }
> EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);
>
> +/**
> + * ahci_platform_find_clk - Find platform clock
> + * @hpriv: host private area to store config values
> + * @con_id: clock connection ID
> + *
> + * This function returns a pointer to the clock descriptor of the clock with
> + * the passed ID.
> + *
> + * RETURNS:
> + * Pointer to the clock descriptor on success otherwise NULL
> + */
> +struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id)
> +{
> + struct clk *clk = NULL;
> + int i;
> +
> + for (i = 0; i < hpriv->n_clks; i++) {
> + if (!strcmp(hpriv->clks[i].id, con_id)) {
> + clk = hpriv->clks[i].clk;
return hpriv->clks[i].clk;
> + break;
> + }
> + }
> +
> + return clk;
And then "return NULL;" here and you do not need the clk variable.
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_find_clk);
> +
> /**
> * ahci_platform_enable_clks - Enable platform clocks
> * @hpriv: host private area to store config values
> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> index 6d7dd472d370..3418980b0341 100644
> --- a/include/linux/ahci_platform.h
> +++ b/include/linux/ahci_platform.h
> @@ -13,6 +13,7 @@
>
> #include <linux/compiler.h>
>
> +struct clk;
> struct device;
> struct ata_port_info;
> struct ahci_host_priv;
> @@ -21,6 +22,8 @@ struct scsi_host_template;
>
> int ahci_platform_enable_phys(struct ahci_host_priv *hpriv);
> void ahci_platform_disable_phys(struct ahci_host_priv *hpriv);
> +struct clk *
> +ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id);
Please make this:
struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv,
const char *con_id);
> int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
> void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
> int ahci_platform_deassert_rsts(struct ahci_host_priv *hpriv);
--
Damien Le Moal
Western Digital Research
On 6/10/22 17:17, Serge Semin wrote:
> There are systems with no BIOS or comprehensive embedded firmware which
> could be able to properly initialize the SATA AHCI controller
> platform-specific capabilities. In that case a good alternative to having
> a clever bootloader is to create a device tree node with the properties
> well describing all the AHCI-related platform specifics. All the settings
> which are normally detected and marked as available in the HBA and its
> ports capabilities fields [1] could be defined in the platform DTB by
> means of a set of the dedicated properties. Such approach perfectly fits
> to the DTB-philosophy - to provide hardware/platform description.
>
> So here we suggest to extend the SATA AHCI device tree bindings with two
> additional DT-properties:
> 1) "hba-cap" - HBA platform generic capabilities like:
> - SSS - Staggered Spin-up support.
> - SMPS - Mechanical Presence Switch support.
> 2) "hba-port-cap" - HBA platform port capabilities like:
> - HPCP - Hot Plug Capable Port.
> - MPSP - Mechanical Presence Switch Attached to Port.
> - CPD - Cold Presence Detection.
> - ESP - External SATA Port.
> - FBSCP - FIS-based Switching Capable Port.
> All of these capabilities require to have a corresponding hardware
> configuration. Thus it's ok to have them defined in DTB.
>
> Even though the driver currently takes into account the state of the ESP
> and FBSCP flags state only, there is nothing wrong with having all of them
> supported by the generic AHCI library in order to have a complete OF-based
> platform-capabilities initialization procedure. These properties will be
> parsed in the ahci_platform_get_resources() method and their values will
> be stored in the saved_* fields of the ahci_host_priv structure, which in
> its turn then will be used to restore the H.CAP, H.PI and P#.CMD
> capability fields on device init and after HBA reset.
>
> Please note this modification concerns the HW-init HBA and its ports flags
> only, which are by specification [1] are supposed to be initialized by the
> BIOS/platform firmware/expansion ROM and which are normally declared in
> the one-time-writable-after-reset register fields. Even though these flags
> aren't supposed to be cleared after HBA reset some AHCI instances may
> violate that rule so we still need to perform the fields resetting after
> each reset. Luckily the corresponding functionality has already been
> partly implemented in the framework of the ahci_save_initial_config() and
> ahci_restore_initial_config() methods.
>
> [1] Serial ATA AHCI 1.3.1 Specification, p. 103
>
> Signed-off-by: Serge Semin <[email protected]>
>
> ---
>
> Changelog v4:
> - Convert the boolean properties to the bitfield DT-properties. (@Rob)
> ---
> drivers/ata/ahci.h | 1 +
> drivers/ata/libahci.c | 51 ++++++++++++++++++++++++++++------
> drivers/ata/libahci_platform.c | 41 +++++++++++++++++++++++++--
> 3 files changed, 82 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 8b9826533ae5..0de221055961 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -337,6 +337,7 @@ struct ahci_host_priv {
> u32 saved_cap; /* saved initial cap */
> u32 saved_cap2; /* saved initial cap2 */
> u32 saved_port_map; /* saved initial port_map */
> + u32 saved_port_cap[AHCI_MAX_PORTS]; /* saved port_cap */
> u32 em_loc; /* enclosure management location */
> u32 em_buf_sz; /* EM buffer size in byte */
> u32 em_msg_type; /* EM message type */
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 1ffaa5f5f21a..954386a2b500 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -16,6 +16,7 @@
> * http://www.intel.com/technology/serialata/pdf/rev1_1.pdf
> */
>
> +#include <linux/bitops.h>
> #include <linux/kernel.h>
> #include <linux/gfp.h>
> #include <linux/module.h>
> @@ -443,16 +444,28 @@ static ssize_t ahci_show_em_supported(struct device *dev,
> void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> {
> void __iomem *mmio = hpriv->mmio;
> - u32 cap, cap2, vers, port_map;
> + void __iomem *port_mmio;
> + unsigned long port_map;
> + u32 cap, cap2, vers;
> int i;
>
> /* make sure AHCI mode is enabled before accessing CAP */
> ahci_enable_ahci(mmio);
>
> - /* Values prefixed with saved_ are written back to host after
> - * reset. Values without are used for driver operation.
> + /*
> + * Values prefixed with saved_ are written back to the HBA and ports
> + * registers after reset. Values without are used for driver operation.
> + */
> +
> + /*
> + * Override HW-init HBA capability fields with the platform-specific
> + * values. The rest of the HBA capabilities are defined as Read-only
> + * and can't be modified in CSR anyway.
> */
> - hpriv->saved_cap = cap = readl(mmio + HOST_CAP);
> + cap = readl(mmio + HOST_CAP);
> + if (hpriv->saved_cap)
> + cap = (cap & ~(HOST_CAP_SSS | HOST_CAP_MPS)) | hpriv->saved_cap;
> + hpriv->saved_cap = cap;
>
> /* CAP2 register is only defined for AHCI 1.2 and later */
> vers = readl(mmio + HOST_VERSION);
> @@ -519,7 +532,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> /* Override the HBA ports mapping if the platform needs it */
> port_map = readl(mmio + HOST_PORTS_IMPL);
> if (hpriv->saved_port_map && port_map != hpriv->saved_port_map) {
> - dev_info(dev, "forcing port_map 0x%x -> 0x%x\n",
> + dev_info(dev, "forcing port_map 0x%lx -> 0x%x\n",
This change is not necessary.
> port_map, hpriv->saved_port_map);
> port_map = hpriv->saved_port_map;
> } else {
> @@ -527,7 +540,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> }
>
> if (hpriv->mask_port_map) {
> - dev_warn(dev, "masking port_map 0x%x -> 0x%x\n",
> + dev_warn(dev, "masking port_map 0x%lx -> 0x%lx\n",
Same.
> port_map,
> port_map & hpriv->mask_port_map);
> port_map &= hpriv->mask_port_map;
> @@ -546,7 +559,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> */
> if (map_ports > ahci_nr_ports(cap)) {
> dev_warn(dev,
> - "implemented port map (0x%x) contains more ports than nr_ports (%u), using nr_ports\n",
> + "implemented port map (0x%lx) contains more ports than nr_ports (%u), using nr_ports\n",
Same.
> port_map, ahci_nr_ports(cap));
> port_map = 0;
> }
> @@ -555,12 +568,26 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> /* fabricate port_map from cap.nr_ports for < AHCI 1.3 */
> if (!port_map && vers < 0x10300) {
> port_map = (1 << ahci_nr_ports(cap)) - 1;
> - dev_warn(dev, "forcing PORTS_IMPL to 0x%x\n", port_map);
> + dev_warn(dev, "forcing PORTS_IMPL to 0x%lx\n", port_map);
And again not needed.
>
> /* write the fixed up value to the PI register */
> hpriv->saved_port_map = port_map;
> }
>
> + /*
> + * Preserve the ports capabilities defined by the platform. Note there
> + * is no need in storing the rest of the P#.CMD fields since they are
> + * volatile.
> + */
> + for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
> + if (hpriv->saved_port_cap[i])
> + continue;
> +
> + port_mmio = __ahci_port_base(hpriv, i);
> + hpriv->saved_port_cap[i] =
> + readl(port_mmio + PORT_CMD) & PORT_CMD_CAP;
> + }
> +
> /* record values to use during operation */
> hpriv->cap = cap;
> hpriv->cap2 = cap2;
> @@ -590,13 +617,21 @@ EXPORT_SYMBOL_GPL(ahci_save_initial_config);
> static void ahci_restore_initial_config(struct ata_host *host)
> {
> struct ahci_host_priv *hpriv = host->private_data;
> + unsigned long port_map = hpriv->port_map;
> void __iomem *mmio = hpriv->mmio;
> + void __iomem *port_mmio;
> + int i;
>
> writel(hpriv->saved_cap, mmio + HOST_CAP);
> if (hpriv->saved_cap2)
> writel(hpriv->saved_cap2, mmio + HOST_CAP2);
> writel(hpriv->saved_port_map, mmio + HOST_PORTS_IMPL);
> (void) readl(mmio + HOST_PORTS_IMPL); /* flush */
> +
> + for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
> + port_mmio = __ahci_port_base(hpriv, i);
> + writel(hpriv->saved_port_cap[i], port_mmio + PORT_CMD);
> + }
> }
>
> static unsigned ahci_scr_offset(struct ata_port *ap, unsigned int sc_reg)
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index efe640603f3f..8b542a8bc487 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -23,6 +23,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/of_platform.h>
> #include <linux/reset.h>
> +
white line change.
> #include "ahci.h"
>
> static void ahci_host_stop(struct ata_host *host);
> @@ -383,6 +384,34 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
> return rc;
> }
>
> +static int ahci_platform_get_firmware(struct ahci_host_priv *hpriv,
> + struct device *dev)
> +{
> + struct device_node *child;
> + u32 port;
> +
> + if (!of_property_read_u32(dev->of_node, "hba-cap", &hpriv->saved_cap))
> + hpriv->saved_cap &= (HOST_CAP_SSS | HOST_CAP_MPS);
> +
> + of_property_read_u32(dev->of_node,
> + "ports-implemented", &hpriv->saved_port_map);
> +
> + for_each_child_of_node(dev->of_node, child) {
> + if (!of_device_is_available(child))
> + continue;
> +
> + if (of_property_read_u32(child, "reg", &port)) {
> + of_node_put(child);
> + return -EINVAL;
> + }
> +
> + if (!of_property_read_u32(child, "hba-port-cap", &hpriv->saved_port_cap[port]))
> + hpriv->saved_port_cap[port] &= PORT_CMD_CAP;
> + }
> +
> + return 0;
> +}
> +
> /**
> * ahci_platform_get_resources - Get platform resources
> * @pdev: platform device to get resources for
> @@ -523,9 +552,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> goto err_out;
> }
>
> - of_property_read_u32(dev->of_node,
> - "ports-implemented", &hpriv->saved_port_map);
> -
> if (child_nodes) {
> for_each_child_of_node(dev->of_node, child) {
> u32 port;
> @@ -590,6 +616,15 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> if (rc == -EPROBE_DEFER)
> goto err_out;
> }
> +
> + /*
> + * Retrieve firmware-specific flags which then will be used to set
> + * the HW-init fields of HBA and its ports
> + */
> + rc = ahci_platform_get_firmware(hpriv, dev);
> + if (rc)
> + goto err_out;
> +
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
> hpriv->got_runtime_pm = true;
--
Damien Le Moal
Western Digital Research
On 6/10/22 01:17, Serge Semin wrote:
> The Broadcom SATA controller is obviously based on the AHCI standard. The
> device driver uses the kernel AHCI library to work with it. Therefore we
> can be have a more thorough DT-bindings evaluation by referring to the
> AHCI-common schema instead of using the more relaxed SATA-common one.
>
> Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On Fri, 10 Jun 2022 11:17:41 +0300, Serge Semin wrote:
> Indeed in accordance with what is implemented in the AHCI platform driver
> and the way the AHCI DT nodes are defined in the DT files we can add the
> next AHCI DT properties constraints: AHCI CSR ID is fixed to 'ahci', PHY
> name is fixed to 'sata-phy', AHCI controller can't have more than 32 ports
> by design, AHCI controller can have up to 32 IRQ lines.
>
> Signed-off-by: Serge Semin <[email protected]>
> Reviewed-by: Hannes Reinecke <[email protected]>
>
> ---
>
> Changelog v2:
> - This is a new patch created after rebasing v1 onto the 5.18-rc3 kernel.
>
> Changelog v4:
> - Fix spelling: 'imeplemtned' and 'paltform' in the patch log. (@Hannes)
> - Add the interrupts property constraints. (@Rob)
> - Add forgotten '---' patchlog-changelog separator. (@Sergei)
> ---
> .../devicetree/bindings/ata/ahci-common.yaml | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
Reviewed-by: Rob Herring <[email protected]>
On Fri, Jun 10, 2022 at 11:17:49AM +0300, Serge Semin wrote:
> In case if the platform doesn't have BIOS or a comprehensive firmware
> installed then the HBA capability flags will be left uninitialized. As a
> good alternative we suggest to define the DT-properties with the AHCI
> platform capabilities describing all the HW-init flags of the
> corresponding capability register. Luckily there aren't too many of them.
> SSS - Staggered Spin-up support and MPS - Mechanical Presence Switch
> support determine the corresponding feature availability for the whole HBA
> by means of the "hba-cap" property. Each port can have the "hba-port-cap"
> property initialized indicating that the port supports some of the next
> functionalities: HPCP - HotPlug capable port, MPSP - Mechanical Presence
> Switch attached to a port, CPD - Cold Plug detection, ESP - External SATA
> Port (eSATA), FBSCP - FIS-based switching capable port.
>
> Signed-off-by: Serge Semin <[email protected]>
>
> ---
>
> Changelog v4:
> - Fix some misspelling in the patch log.
> - Convert the boolean properties to the bitfield properties. (@Rob)
> - Remove Hannes' rb tag due to the patch content change.
> ---
> .../devicetree/bindings/ata/ahci-common.yaml | 16 +++++++++++++++
> .../bindings/ata/ahci-platform.yaml | 10 ++++++++++
> include/dt-bindings/ata/ahci.h | 20 +++++++++++++++++++
> 3 files changed, 46 insertions(+)
> create mode 100644 include/dt-bindings/ata/ahci.h
>
> diff --git a/Documentation/devicetree/bindings/ata/ahci-common.yaml b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> index 12a97b56226f..94d72aeaad0f 100644
> --- a/Documentation/devicetree/bindings/ata/ahci-common.yaml
> +++ b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> @@ -58,6 +58,14 @@ properties:
> phy-names:
> const: sata-phy
>
> + hba-cap:
> + $ref: '/schemas/types.yaml#/definitions/uint32'
> + description:
> + Bitfield of the HBA generic platform capabilities like Staggered
> + Spin-up or Mechanical Presence Switch support. It can be used to
> + appropriately initialize the HWinit fields of the HBA CAP register
> + in case if the system firmware hasn't done it.
> +
> ports-implemented:
> $ref: '/schemas/types.yaml#/definitions/uint32'
> description:
> @@ -101,6 +109,14 @@ $defs:
> target-supply:
> description: Power regulator for SATA port target device
>
> + hba-port-cap:
> + $ref: '/schemas/types.yaml#/definitions/uint32'
> + description:
> + Bitfield of the HBA port-specific platform capabilities like Hot
> + plugging, eSATA, FIS-based Switching, etc (see AHCI specification
> + for details). It can be used to initialize the HWinit fields of
> + the PxCMD register in case if the system firmware hasn't done it.
> +
> required:
> - reg
>
> diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> index 15be98e0385b..e19cf9828e68 100644
> --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> @@ -111,6 +111,8 @@ examples:
> - |
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/clock/berlin2q.h>
> + #include <dt-bindings/ata/ahci.h>
> +
> sata@f7e90000 {
> compatible = "marvell,berlin2q-ahci", "generic-ahci";
> reg = <0xf7e90000 0x1000>;
> @@ -119,15 +121,23 @@ examples:
> #address-cells = <1>;
> #size-cells = <0>;
>
> + hba-cap = <HBA_SMPS>;
> +
> sata0: sata-port@0 {
> reg = <0>;
> +
> phys = <&sata_phy 0>;
> target-supply = <®_sata0>;
> +
> + hba-port-cap = <(HBA_PORT_FBSCP | HBA_PORT_ESP)>;
> };
>
> sata1: sata-port@1 {
> reg = <1>;
> +
> phys = <&sata_phy 1>;
> target-supply = <®_sata1>;
> +
> + hba-port-cap = <(HBA_PORT_HPCP | HBA_PORT_MPSP | HBA_PORT_FBSCP)>;
> };
> };
> diff --git a/include/dt-bindings/ata/ahci.h b/include/dt-bindings/ata/ahci.h
> new file mode 100644
> index 000000000000..6841caebcedf
> --- /dev/null
> +++ b/include/dt-bindings/ata/ahci.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
Dual license.
With that,
Reviewed-by: Rob Herring <[email protected]>
> +/*
> + * This header provides constants for most AHCI bindings.
> + */
> +
> +#ifndef _DT_BINDINGS_ATA_AHCI_H
> +#define _DT_BINDINGS_ATA_AHCI_H
> +
> +/* Host Bus Adapter generic platform capabilities */
> +#define HBA_SSS (1 << 27)
> +#define HBA_SMPS (1 << 28)
> +
> +/* Host Bus Adapter port-specific platform capabilities */
> +#define HBA_PORT_HPCP (1 << 18)
> +#define HBA_PORT_MPSP (1 << 19)
> +#define HBA_PORT_CPD (1 << 20)
> +#define HBA_PORT_ESP (1 << 21)
> +#define HBA_PORT_FBSCP (1 << 22)
> +
> +#endif
> --
> 2.35.1
>
>
On Fri, 10 Jun 2022 11:17:43 +0300, Serge Semin wrote:
> The Broadcom SATA controller is obviously based on the AHCI standard. The
> device driver uses the kernel AHCI library to work with it. Therefore we
> can be have a more thorough DT-bindings evaluation by referring to the
> AHCI-common schema instead of using the more relaxed SATA-common one.
>
> Signed-off-by: Serge Semin <[email protected]>
>
> ---
>
> Changelog v4:
> - This is a new patch added on v4 lap of the review procedure.
> ---
> Documentation/devicetree/bindings/ata/brcm,sata-brcm.yaml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Rob Herring <[email protected]>
On Tue, Jun 14, 2022 at 05:32:41PM +0900, Damien Le Moal wrote:
> On 6/10/22 17:17, Serge Semin wrote:
> > Currently not all of the Port-specific capabilities listed in the
>
> s/listed/are listed
>
> > PORT_CMD-enumeration. Let's extend that set with the Cold Presence
> > Detection and Mechanical Presence Switch attached to the Port flags [1] so
> > to closeup the set of the platform-specific port-capabilities flags. Note
> > these flags are supposed to be set by the platform firmware if there is
> > one. Alternatively as we are about to do they can be set by means of the
> > OF properties.
> >
> > While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DMPS and fix the
> > comment there. In accordance with [2] that IRQ flag is supposed to
> > indicate the state of the signal coming from the Mechanical Presence
> > Switch.
> >
> > [1] Serial ATA AHCI 1.3.1 Specification, p.27
> > [2] Serial ATA AHCI 1.3.1 Specification, p.24, p.88
> >
> > Signed-off-by: Serge Semin <[email protected]>
> > Reviewed-by: Hannes Reinecke <[email protected]>
> >
> > ---
> >
> > Changelog v4:
> > - Fix the DMPS macros name in the patch log. (@Sergei Shtylyov)
> > ---
> > drivers/ata/ahci.h | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> > index 7d834deefeb9..f501531bd1b3 100644
> > --- a/drivers/ata/ahci.h
> > +++ b/drivers/ata/ahci.h
> > @@ -138,7 +138,7 @@ enum {
> > PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */
> >
> > PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */
> > - PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */
> > + PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */
> > PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */
> > PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */
> > PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */
> > @@ -166,6 +166,8 @@ enum {
> > PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
> > PORT_CMD_FBSCP = (1 << 22), /* FBS Capable Port */
> > PORT_CMD_ESP = (1 << 21), /* External Sata Port */
> > + PORT_CMD_CPD = (1 << 20), /* Cold Presence Detection */
> > + PORT_CMD_MPSP = (1 << 19), /* Mechanical Presence Switch */
> > PORT_CMD_HPCP = (1 << 18), /* HotPlug Capable Port */
> > PORT_CMD_PMP = (1 << 17), /* PMP attached */
> > PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
> > @@ -181,6 +183,9 @@ enum {
> > PORT_CMD_ICC_PARTIAL = (0x2 << 28), /* Put i/f in partial state */
> > PORT_CMD_ICC_SLUMBER = (0x6 << 28), /* Put i/f in slumber state */
> >
> > + PORT_CMD_CAP = PORT_CMD_HPCP | PORT_CMD_MPSP |
> > + PORT_CMD_CPD | PORT_CMD_ESP | PORT_CMD_FBSCP,
>
> What is this one for ? A comment above it would be nice.
Isn't it obviously inferrable from the definition and the item name?
-Sergey
>
> > +
> > /* PORT_FBS bits */
> > PORT_FBS_DWE_OFFSET = 16, /* FBS device with error offset */
> > PORT_FBS_ADO_OFFSET = 12, /* FBS active dev optimization offset */
>
>
> --
> Damien Le Moal
> Western Digital Research
On Tue, Jun 14, 2022 at 05:42:35PM +0900, Damien Le Moal wrote:
> On 6/10/22 17:17, Serge Semin wrote:
> > There are systems with no BIOS or comprehensive embedded firmware which
> > could be able to properly initialize the SATA AHCI controller
> > platform-specific capabilities. In that case a good alternative to having
> > a clever bootloader is to create a device tree node with the properties
> > well describing all the AHCI-related platform specifics. All the settings
> > which are normally detected and marked as available in the HBA and its
> > ports capabilities fields [1] could be defined in the platform DTB by
> > means of a set of the dedicated properties. Such approach perfectly fits
> > to the DTB-philosophy - to provide hardware/platform description.
> >
> > So here we suggest to extend the SATA AHCI device tree bindings with two
> > additional DT-properties:
> > 1) "hba-cap" - HBA platform generic capabilities like:
> > - SSS - Staggered Spin-up support.
> > - SMPS - Mechanical Presence Switch support.
> > 2) "hba-port-cap" - HBA platform port capabilities like:
> > - HPCP - Hot Plug Capable Port.
> > - MPSP - Mechanical Presence Switch Attached to Port.
> > - CPD - Cold Presence Detection.
> > - ESP - External SATA Port.
> > - FBSCP - FIS-based Switching Capable Port.
> > All of these capabilities require to have a corresponding hardware
> > configuration. Thus it's ok to have them defined in DTB.
> >
> > Even though the driver currently takes into account the state of the ESP
> > and FBSCP flags state only, there is nothing wrong with having all of them
> > supported by the generic AHCI library in order to have a complete OF-based
> > platform-capabilities initialization procedure. These properties will be
> > parsed in the ahci_platform_get_resources() method and their values will
> > be stored in the saved_* fields of the ahci_host_priv structure, which in
> > its turn then will be used to restore the H.CAP, H.PI and P#.CMD
> > capability fields on device init and after HBA reset.
> >
> > Please note this modification concerns the HW-init HBA and its ports flags
> > only, which are by specification [1] are supposed to be initialized by the
> > BIOS/platform firmware/expansion ROM and which are normally declared in
> > the one-time-writable-after-reset register fields. Even though these flags
> > aren't supposed to be cleared after HBA reset some AHCI instances may
> > violate that rule so we still need to perform the fields resetting after
> > each reset. Luckily the corresponding functionality has already been
> > partly implemented in the framework of the ahci_save_initial_config() and
> > ahci_restore_initial_config() methods.
> >
> > [1] Serial ATA AHCI 1.3.1 Specification, p. 103
> >
> > Signed-off-by: Serge Semin <[email protected]>
> >
> > ---
> >
> > Changelog v4:
> > - Convert the boolean properties to the bitfield DT-properties. (@Rob)
> > ---
> > drivers/ata/ahci.h | 1 +
> > drivers/ata/libahci.c | 51 ++++++++++++++++++++++++++++------
> > drivers/ata/libahci_platform.c | 41 +++++++++++++++++++++++++--
> > 3 files changed, 82 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> > index 8b9826533ae5..0de221055961 100644
> > --- a/drivers/ata/ahci.h
> > +++ b/drivers/ata/ahci.h
> > @@ -337,6 +337,7 @@ struct ahci_host_priv {
> > u32 saved_cap; /* saved initial cap */
> > u32 saved_cap2; /* saved initial cap2 */
> > u32 saved_port_map; /* saved initial port_map */
> > + u32 saved_port_cap[AHCI_MAX_PORTS]; /* saved port_cap */
> > u32 em_loc; /* enclosure management location */
> > u32 em_buf_sz; /* EM buffer size in byte */
> > u32 em_msg_type; /* EM message type */
> > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> > index 1ffaa5f5f21a..954386a2b500 100644
> > --- a/drivers/ata/libahci.c
> > +++ b/drivers/ata/libahci.c
> > @@ -16,6 +16,7 @@
> > * http://www.intel.com/technology/serialata/pdf/rev1_1.pdf
> > */
> >
> > +#include <linux/bitops.h>
> > #include <linux/kernel.h>
> > #include <linux/gfp.h>
> > #include <linux/module.h>
> > @@ -443,16 +444,28 @@ static ssize_t ahci_show_em_supported(struct device *dev,
> > void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> > {
> > void __iomem *mmio = hpriv->mmio;
> > - u32 cap, cap2, vers, port_map;
> > + void __iomem *port_mmio;
> > + unsigned long port_map;
> > + u32 cap, cap2, vers;
> > int i;
> >
> > /* make sure AHCI mode is enabled before accessing CAP */
> > ahci_enable_ahci(mmio);
> >
> > - /* Values prefixed with saved_ are written back to host after
> > - * reset. Values without are used for driver operation.
> > + /*
> > + * Values prefixed with saved_ are written back to the HBA and ports
> > + * registers after reset. Values without are used for driver operation.
> > + */
> > +
> > + /*
> > + * Override HW-init HBA capability fields with the platform-specific
> > + * values. The rest of the HBA capabilities are defined as Read-only
> > + * and can't be modified in CSR anyway.
> > */
> > - hpriv->saved_cap = cap = readl(mmio + HOST_CAP);
> > + cap = readl(mmio + HOST_CAP);
> > + if (hpriv->saved_cap)
> > + cap = (cap & ~(HOST_CAP_SSS | HOST_CAP_MPS)) | hpriv->saved_cap;
> > + hpriv->saved_cap = cap;
> >
> > /* CAP2 register is only defined for AHCI 1.2 and later */
> > vers = readl(mmio + HOST_VERSION);
> > @@ -519,7 +532,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> > /* Override the HBA ports mapping if the platform needs it */
> > port_map = readl(mmio + HOST_PORTS_IMPL);
> > if (hpriv->saved_port_map && port_map != hpriv->saved_port_map) {
> > - dev_info(dev, "forcing port_map 0x%x -> 0x%x\n",
> > + dev_info(dev, "forcing port_map 0x%lx -> 0x%x\n",
>
> This change is not necessary.
It is. The port_map type has been changed.
>
> > port_map, hpriv->saved_port_map);
> > port_map = hpriv->saved_port_map;
> > } else {
> > @@ -527,7 +540,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> > }
> >
> > if (hpriv->mask_port_map) {
> > - dev_warn(dev, "masking port_map 0x%x -> 0x%x\n",
> > + dev_warn(dev, "masking port_map 0x%lx -> 0x%lx\n",
>
> Same.
ditto
>
> > port_map,
> > port_map & hpriv->mask_port_map);
> > port_map &= hpriv->mask_port_map;
> > @@ -546,7 +559,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> > */
> > if (map_ports > ahci_nr_ports(cap)) {
> > dev_warn(dev,
> > - "implemented port map (0x%x) contains more ports than nr_ports (%u), using nr_ports\n",
> > + "implemented port map (0x%lx) contains more ports than nr_ports (%u), using nr_ports\n",
>
> Same.
ditto.
>
> > port_map, ahci_nr_ports(cap));
> > port_map = 0;
> > }
> > @@ -555,12 +568,26 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> > /* fabricate port_map from cap.nr_ports for < AHCI 1.3 */
> > if (!port_map && vers < 0x10300) {
> > port_map = (1 << ahci_nr_ports(cap)) - 1;
> > - dev_warn(dev, "forcing PORTS_IMPL to 0x%x\n", port_map);
> > + dev_warn(dev, "forcing PORTS_IMPL to 0x%lx\n", port_map);
>
> And again not needed.
and ditto.
>
> >
> > /* write the fixed up value to the PI register */
> > hpriv->saved_port_map = port_map;
> > }
> >
> > + /*
> > + * Preserve the ports capabilities defined by the platform. Note there
> > + * is no need in storing the rest of the P#.CMD fields since they are
> > + * volatile.
> > + */
> > + for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
> > + if (hpriv->saved_port_cap[i])
> > + continue;
> > +
> > + port_mmio = __ahci_port_base(hpriv, i);
> > + hpriv->saved_port_cap[i] =
> > + readl(port_mmio + PORT_CMD) & PORT_CMD_CAP;
> > + }
> > +
> > /* record values to use during operation */
> > hpriv->cap = cap;
> > hpriv->cap2 = cap2;
> > @@ -590,13 +617,21 @@ EXPORT_SYMBOL_GPL(ahci_save_initial_config);
> > static void ahci_restore_initial_config(struct ata_host *host)
> > {
> > struct ahci_host_priv *hpriv = host->private_data;
> > + unsigned long port_map = hpriv->port_map;
> > void __iomem *mmio = hpriv->mmio;
> > + void __iomem *port_mmio;
> > + int i;
> >
> > writel(hpriv->saved_cap, mmio + HOST_CAP);
> > if (hpriv->saved_cap2)
> > writel(hpriv->saved_cap2, mmio + HOST_CAP2);
> > writel(hpriv->saved_port_map, mmio + HOST_PORTS_IMPL);
> > (void) readl(mmio + HOST_PORTS_IMPL); /* flush */
> > +
> > + for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
> > + port_mmio = __ahci_port_base(hpriv, i);
> > + writel(hpriv->saved_port_cap[i], port_mmio + PORT_CMD);
> > + }
> > }
> >
> > static unsigned ahci_scr_offset(struct ata_port *ap, unsigned int sc_reg)
> > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> > index efe640603f3f..8b542a8bc487 100644
> > --- a/drivers/ata/libahci_platform.c
> > +++ b/drivers/ata/libahci_platform.c
> > @@ -23,6 +23,7 @@
> > #include <linux/pm_runtime.h>
> > #include <linux/of_platform.h>
> > #include <linux/reset.h>
> > +
>
> white line change.
Ok. I'll drop it.
-Sergey
>
> > #include "ahci.h"
> >
> > static void ahci_host_stop(struct ata_host *host);
> > @@ -383,6 +384,34 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
> > return rc;
> > }
> >
> > +static int ahci_platform_get_firmware(struct ahci_host_priv *hpriv,
> > + struct device *dev)
> > +{
> > + struct device_node *child;
> > + u32 port;
> > +
> > + if (!of_property_read_u32(dev->of_node, "hba-cap", &hpriv->saved_cap))
> > + hpriv->saved_cap &= (HOST_CAP_SSS | HOST_CAP_MPS);
> > +
> > + of_property_read_u32(dev->of_node,
> > + "ports-implemented", &hpriv->saved_port_map);
> > +
> > + for_each_child_of_node(dev->of_node, child) {
> > + if (!of_device_is_available(child))
> > + continue;
> > +
> > + if (of_property_read_u32(child, "reg", &port)) {
> > + of_node_put(child);
> > + return -EINVAL;
> > + }
> > +
> > + if (!of_property_read_u32(child, "hba-port-cap", &hpriv->saved_port_cap[port]))
> > + hpriv->saved_port_cap[port] &= PORT_CMD_CAP;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * ahci_platform_get_resources - Get platform resources
> > * @pdev: platform device to get resources for
> > @@ -523,9 +552,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> > goto err_out;
> > }
> >
> > - of_property_read_u32(dev->of_node,
> > - "ports-implemented", &hpriv->saved_port_map);
> > -
> > if (child_nodes) {
> > for_each_child_of_node(dev->of_node, child) {
> > u32 port;
> > @@ -590,6 +616,15 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> > if (rc == -EPROBE_DEFER)
> > goto err_out;
> > }
> > +
> > + /*
> > + * Retrieve firmware-specific flags which then will be used to set
> > + * the HW-init fields of HBA and its ports
> > + */
> > + rc = ahci_platform_get_firmware(hpriv, dev);
> > + if (rc)
> > + goto err_out;
> > +
> > pm_runtime_enable(dev);
> > pm_runtime_get_sync(dev);
> > hpriv->got_runtime_pm = true;
>
>
> --
> Damien Le Moal
> Western Digital Research
On Tue, Jun 14, 2022 at 05:45:35PM +0900, Damien Le Moal wrote:
> On 6/10/22 17:17, Serge Semin wrote:
> > Since all the clocks are retrieved by the method
> > ahci_platform_get_resources() there is no need for the LLD (glue) drivers
> > to be looking for some particular of them in the kernel clocks table
> > again. Instead we suggest to add a simple method returning a
> > device-specific clock with passed connection ID if it is managed to be
> > found. Otherwise the function will return NULL. Thus the glue-drivers
> > won't need to either manually touching the hpriv->clks array or calling
> > clk_get()-friends. The AHCI platform drivers will be able to use the new
> > function right after the ahci_platform_get_resources() method invocation
> > and up to the device removal.
> >
> > Note the method is left unused here, but will be utilized in the framework
> > of the DWC AHCI SATA driver being added in the next commit.
> >
> > Signed-off-by: Serge Semin <[email protected]>
> >
> > ---
> >
> > Changelog v2:
> > - Fix some grammar mistakes in the method description.
> >
> > Changelog v4:
> > - Add a note regarding the new method usage.
> > ---
> > drivers/ata/libahci_platform.c | 27 +++++++++++++++++++++++++++
> > include/linux/ahci_platform.h | 3 +++
> > 2 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> > index 8b542a8bc487..418961f954af 100644
> > --- a/drivers/ata/libahci_platform.c
> > +++ b/drivers/ata/libahci_platform.c
> > @@ -95,6 +95,33 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
> > }
> > EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);
> >
> > +/**
> > + * ahci_platform_find_clk - Find platform clock
> > + * @hpriv: host private area to store config values
> > + * @con_id: clock connection ID
> > + *
> > + * This function returns a pointer to the clock descriptor of the clock with
> > + * the passed ID.
> > + *
> > + * RETURNS:
> > + * Pointer to the clock descriptor on success otherwise NULL
> > + */
> > +struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id)
> > +{
> > + struct clk *clk = NULL;
> > + int i;
> > +
> > + for (i = 0; i < hpriv->n_clks; i++) {
> > + if (!strcmp(hpriv->clks[i].id, con_id)) {
> > + clk = hpriv->clks[i].clk;
>
> return hpriv->clks[i].clk;
>
> > + break;
> > + }
> > + }
> > +
> > + return clk;
>
> And then "return NULL;" here and you do not need the clk variable.
Ok.
>
> > +}
> > +EXPORT_SYMBOL_GPL(ahci_platform_find_clk);
> > +
> > /**
> > * ahci_platform_enable_clks - Enable platform clocks
> > * @hpriv: host private area to store config values
> > diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> > index 6d7dd472d370..3418980b0341 100644
> > --- a/include/linux/ahci_platform.h
> > +++ b/include/linux/ahci_platform.h
> > @@ -13,6 +13,7 @@
> >
> > #include <linux/compiler.h>
> >
> > +struct clk;
> > struct device;
> > struct ata_port_info;
> > struct ahci_host_priv;
> > @@ -21,6 +22,8 @@ struct scsi_host_template;
> >
> > int ahci_platform_enable_phys(struct ahci_host_priv *hpriv);
> > void ahci_platform_disable_phys(struct ahci_host_priv *hpriv);
> > +struct clk *
> > +ahci_platform_find_clk(struct ahci_host_priv *hpriv, const char *con_id);
>
> Please make this:
>
> struct clk *ahci_platform_find_clk(struct ahci_host_priv *hpriv,
>
> const char *con_id);
Ok.
-Sergey
>
> > int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
> > void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
> > int ahci_platform_deassert_rsts(struct ahci_host_priv *hpriv);
>
>
> --
> Damien Le Moal
> Western Digital Research
On Tue, Jun 14, 2022 at 04:19:17PM -0600, Rob Herring wrote:
> On Fri, Jun 10, 2022 at 11:17:49AM +0300, Serge Semin wrote:
> > In case if the platform doesn't have BIOS or a comprehensive firmware
> > installed then the HBA capability flags will be left uninitialized. As a
> > good alternative we suggest to define the DT-properties with the AHCI
> > platform capabilities describing all the HW-init flags of the
> > corresponding capability register. Luckily there aren't too many of them.
> > SSS - Staggered Spin-up support and MPS - Mechanical Presence Switch
> > support determine the corresponding feature availability for the whole HBA
> > by means of the "hba-cap" property. Each port can have the "hba-port-cap"
> > property initialized indicating that the port supports some of the next
> > functionalities: HPCP - HotPlug capable port, MPSP - Mechanical Presence
> > Switch attached to a port, CPD - Cold Plug detection, ESP - External SATA
> > Port (eSATA), FBSCP - FIS-based switching capable port.
> >
> > Signed-off-by: Serge Semin <[email protected]>
> >
> > ---
> >
> > Changelog v4:
> > - Fix some misspelling in the patch log.
> > - Convert the boolean properties to the bitfield properties. (@Rob)
> > - Remove Hannes' rb tag due to the patch content change.
> > ---
> > .../devicetree/bindings/ata/ahci-common.yaml | 16 +++++++++++++++
> > .../bindings/ata/ahci-platform.yaml | 10 ++++++++++
> > include/dt-bindings/ata/ahci.h | 20 +++++++++++++++++++
> > 3 files changed, 46 insertions(+)
> > create mode 100644 include/dt-bindings/ata/ahci.h
> >
> > diff --git a/Documentation/devicetree/bindings/ata/ahci-common.yaml b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> > index 12a97b56226f..94d72aeaad0f 100644
> > --- a/Documentation/devicetree/bindings/ata/ahci-common.yaml
> > +++ b/Documentation/devicetree/bindings/ata/ahci-common.yaml
> > @@ -58,6 +58,14 @@ properties:
> > phy-names:
> > const: sata-phy
> >
> > + hba-cap:
> > + $ref: '/schemas/types.yaml#/definitions/uint32'
> > + description:
> > + Bitfield of the HBA generic platform capabilities like Staggered
> > + Spin-up or Mechanical Presence Switch support. It can be used to
> > + appropriately initialize the HWinit fields of the HBA CAP register
> > + in case if the system firmware hasn't done it.
> > +
> > ports-implemented:
> > $ref: '/schemas/types.yaml#/definitions/uint32'
> > description:
> > @@ -101,6 +109,14 @@ $defs:
> > target-supply:
> > description: Power regulator for SATA port target device
> >
> > + hba-port-cap:
> > + $ref: '/schemas/types.yaml#/definitions/uint32'
> > + description:
> > + Bitfield of the HBA port-specific platform capabilities like Hot
> > + plugging, eSATA, FIS-based Switching, etc (see AHCI specification
> > + for details). It can be used to initialize the HWinit fields of
> > + the PxCMD register in case if the system firmware hasn't done it.
> > +
> > required:
> > - reg
> >
> > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > index 15be98e0385b..e19cf9828e68 100644
> > --- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
> > @@ -111,6 +111,8 @@ examples:
> > - |
> > #include <dt-bindings/interrupt-controller/arm-gic.h>
> > #include <dt-bindings/clock/berlin2q.h>
> > + #include <dt-bindings/ata/ahci.h>
> > +
> > sata@f7e90000 {
> > compatible = "marvell,berlin2q-ahci", "generic-ahci";
> > reg = <0xf7e90000 0x1000>;
> > @@ -119,15 +121,23 @@ examples:
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > + hba-cap = <HBA_SMPS>;
> > +
> > sata0: sata-port@0 {
> > reg = <0>;
> > +
> > phys = <&sata_phy 0>;
> > target-supply = <®_sata0>;
> > +
> > + hba-port-cap = <(HBA_PORT_FBSCP | HBA_PORT_ESP)>;
> > };
> >
> > sata1: sata-port@1 {
> > reg = <1>;
> > +
> > phys = <&sata_phy 1>;
> > target-supply = <®_sata1>;
> > +
> > + hba-port-cap = <(HBA_PORT_HPCP | HBA_PORT_MPSP | HBA_PORT_FBSCP)>;
> > };
> > };
> > diff --git a/include/dt-bindings/ata/ahci.h b/include/dt-bindings/ata/ahci.h
> > new file mode 100644
> > index 000000000000..6841caebcedf
> > --- /dev/null
> > +++ b/include/dt-bindings/ata/ahci.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
>
> Dual license.
Ok.
>
> With that,
>
> Reviewed-by: Rob Herring <[email protected]>
Thanks.
-Sergey
>
> > +/*
> > + * This header provides constants for most AHCI bindings.
> > + */
> > +
> > +#ifndef _DT_BINDINGS_ATA_AHCI_H
> > +#define _DT_BINDINGS_ATA_AHCI_H
> > +
> > +/* Host Bus Adapter generic platform capabilities */
> > +#define HBA_SSS (1 << 27)
> > +#define HBA_SMPS (1 << 28)
> > +
> > +/* Host Bus Adapter port-specific platform capabilities */
> > +#define HBA_PORT_HPCP (1 << 18)
> > +#define HBA_PORT_MPSP (1 << 19)
> > +#define HBA_PORT_CPD (1 << 20)
> > +#define HBA_PORT_ESP (1 << 21)
> > +#define HBA_PORT_FBSCP (1 << 22)
> > +
> > +#endif
> > --
> > 2.35.1
> >
> >
On 2022/06/16 5:58, Serge Semin wrote:
> On Tue, Jun 14, 2022 at 05:32:41PM +0900, Damien Le Moal wrote:
>> On 6/10/22 17:17, Serge Semin wrote:
>>> Currently not all of the Port-specific capabilities listed in the
>>
>> s/listed/are listed
>>
>>> PORT_CMD-enumeration. Let's extend that set with the Cold Presence
>>> Detection and Mechanical Presence Switch attached to the Port flags [1] so
>>> to closeup the set of the platform-specific port-capabilities flags. Note
>>> these flags are supposed to be set by the platform firmware if there is
>>> one. Alternatively as we are about to do they can be set by means of the
>>> OF properties.
>>>
>>> While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DMPS and fix the
>>> comment there. In accordance with [2] that IRQ flag is supposed to
>>> indicate the state of the signal coming from the Mechanical Presence
>>> Switch.
>>>
>>> [1] Serial ATA AHCI 1.3.1 Specification, p.27
>>> [2] Serial ATA AHCI 1.3.1 Specification, p.24, p.88
>>>
>>> Signed-off-by: Serge Semin <[email protected]>
>>> Reviewed-by: Hannes Reinecke <[email protected]>
>>>
>>> ---
>>>
>>> Changelog v4:
>>> - Fix the DMPS macros name in the patch log. (@Sergei Shtylyov)
>>> ---
>>> drivers/ata/ahci.h | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>>> index 7d834deefeb9..f501531bd1b3 100644
>>> --- a/drivers/ata/ahci.h
>>> +++ b/drivers/ata/ahci.h
>>> @@ -138,7 +138,7 @@ enum {
>>> PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */
>>>
>>> PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */
>>> - PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */
>>> + PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */
>>> PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */
>>> PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */
>>> PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */
>>> @@ -166,6 +166,8 @@ enum {
>>> PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
>>> PORT_CMD_FBSCP = (1 << 22), /* FBS Capable Port */
>>> PORT_CMD_ESP = (1 << 21), /* External Sata Port */
>>> + PORT_CMD_CPD = (1 << 20), /* Cold Presence Detection */
>>> + PORT_CMD_MPSP = (1 << 19), /* Mechanical Presence Switch */
>>> PORT_CMD_HPCP = (1 << 18), /* HotPlug Capable Port */
>>> PORT_CMD_PMP = (1 << 17), /* PMP attached */
>>> PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
>>> @@ -181,6 +183,9 @@ enum {
>>> PORT_CMD_ICC_PARTIAL = (0x2 << 28), /* Put i/f in partial state */
>>> PORT_CMD_ICC_SLUMBER = (0x6 << 28), /* Put i/f in slumber state */
>>>
>>> + PORT_CMD_CAP = PORT_CMD_HPCP | PORT_CMD_MPSP |
>>> + PORT_CMD_CPD | PORT_CMD_ESP | PORT_CMD_FBSCP,
>>
>
>> What is this one for ? A comment above it would be nice.
>
> Isn't it obviously inferrable from the definition and the item name?
I am guessing from the name. Am I guessing OK ? A comment would still be nice.
Why just these bits ? There are more cap/support indicator bits in that port cmd
bitfield. So why this particular set of bits ? What do they mean all together ?
Sure I can go and read the specs to figure it out. But again, a comment would
avoid readers of the code to have to decrypt all that.
>
> -Sergey
>
>>
>>> +
>>> /* PORT_FBS bits */
>>> PORT_FBS_DWE_OFFSET = 16, /* FBS device with error offset */
>>> PORT_FBS_ADO_OFFSET = 12, /* FBS active dev optimization offset */
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
--
Damien Le Moal
Western Digital Research
On 2022/06/16 6:11, Serge Semin wrote:
> On Tue, Jun 14, 2022 at 05:42:35PM +0900, Damien Le Moal wrote:
>> On 6/10/22 17:17, Serge Semin wrote:
>>> There are systems with no BIOS or comprehensive embedded firmware which
>>> could be able to properly initialize the SATA AHCI controller
>>> platform-specific capabilities. In that case a good alternative to having
>>> a clever bootloader is to create a device tree node with the properties
>>> well describing all the AHCI-related platform specifics. All the settings
>>> which are normally detected and marked as available in the HBA and its
>>> ports capabilities fields [1] could be defined in the platform DTB by
>>> means of a set of the dedicated properties. Such approach perfectly fits
>>> to the DTB-philosophy - to provide hardware/platform description.
>>>
>>> So here we suggest to extend the SATA AHCI device tree bindings with two
>>> additional DT-properties:
>>> 1) "hba-cap" - HBA platform generic capabilities like:
>>> - SSS - Staggered Spin-up support.
>>> - SMPS - Mechanical Presence Switch support.
>>> 2) "hba-port-cap" - HBA platform port capabilities like:
>>> - HPCP - Hot Plug Capable Port.
>>> - MPSP - Mechanical Presence Switch Attached to Port.
>>> - CPD - Cold Presence Detection.
>>> - ESP - External SATA Port.
>>> - FBSCP - FIS-based Switching Capable Port.
>>> All of these capabilities require to have a corresponding hardware
>>> configuration. Thus it's ok to have them defined in DTB.
>>>
>>> Even though the driver currently takes into account the state of the ESP
>>> and FBSCP flags state only, there is nothing wrong with having all of them
>>> supported by the generic AHCI library in order to have a complete OF-based
>>> platform-capabilities initialization procedure. These properties will be
>>> parsed in the ahci_platform_get_resources() method and their values will
>>> be stored in the saved_* fields of the ahci_host_priv structure, which in
>>> its turn then will be used to restore the H.CAP, H.PI and P#.CMD
>>> capability fields on device init and after HBA reset.
>>>
>>> Please note this modification concerns the HW-init HBA and its ports flags
>>> only, which are by specification [1] are supposed to be initialized by the
>>> BIOS/platform firmware/expansion ROM and which are normally declared in
>>> the one-time-writable-after-reset register fields. Even though these flags
>>> aren't supposed to be cleared after HBA reset some AHCI instances may
>>> violate that rule so we still need to perform the fields resetting after
>>> each reset. Luckily the corresponding functionality has already been
>>> partly implemented in the framework of the ahci_save_initial_config() and
>>> ahci_restore_initial_config() methods.
>>>
>>> [1] Serial ATA AHCI 1.3.1 Specification, p. 103
>>>
>>> Signed-off-by: Serge Semin <[email protected]>
>>>
>>> ---
>>>
>>> Changelog v4:
>>> - Convert the boolean properties to the bitfield DT-properties. (@Rob)
>>> ---
>>> drivers/ata/ahci.h | 1 +
>>> drivers/ata/libahci.c | 51 ++++++++++++++++++++++++++++------
>>> drivers/ata/libahci_platform.c | 41 +++++++++++++++++++++++++--
>>> 3 files changed, 82 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>>> index 8b9826533ae5..0de221055961 100644
>>> --- a/drivers/ata/ahci.h
>>> +++ b/drivers/ata/ahci.h
>>> @@ -337,6 +337,7 @@ struct ahci_host_priv {
>>> u32 saved_cap; /* saved initial cap */
>>> u32 saved_cap2; /* saved initial cap2 */
>>> u32 saved_port_map; /* saved initial port_map */
>>> + u32 saved_port_cap[AHCI_MAX_PORTS]; /* saved port_cap */
>>> u32 em_loc; /* enclosure management location */
>>> u32 em_buf_sz; /* EM buffer size in byte */
>>> u32 em_msg_type; /* EM message type */
>>> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
>>> index 1ffaa5f5f21a..954386a2b500 100644
>>> --- a/drivers/ata/libahci.c
>>> +++ b/drivers/ata/libahci.c
>>> @@ -16,6 +16,7 @@
>>> * http://www.intel.com/technology/serialata/pdf/rev1_1.pdf
>>> */
>>>
>>> +#include <linux/bitops.h>
>>> #include <linux/kernel.h>
>>> #include <linux/gfp.h>
>>> #include <linux/module.h>
>>> @@ -443,16 +444,28 @@ static ssize_t ahci_show_em_supported(struct device *dev,
>>> void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
>>> {
>>> void __iomem *mmio = hpriv->mmio;
>>> - u32 cap, cap2, vers, port_map;
>>> + void __iomem *port_mmio;
>>> + unsigned long port_map;
>>> + u32 cap, cap2, vers;
>>> int i;
>>>
>>> /* make sure AHCI mode is enabled before accessing CAP */
>>> ahci_enable_ahci(mmio);
>>>
>>> - /* Values prefixed with saved_ are written back to host after
>>> - * reset. Values without are used for driver operation.
>>> + /*
>>> + * Values prefixed with saved_ are written back to the HBA and ports
>>> + * registers after reset. Values without are used for driver operation.
>>> + */
>>> +
>>> + /*
>>> + * Override HW-init HBA capability fields with the platform-specific
>>> + * values. The rest of the HBA capabilities are defined as Read-only
>>> + * and can't be modified in CSR anyway.
>>> */
>>> - hpriv->saved_cap = cap = readl(mmio + HOST_CAP);
>>> + cap = readl(mmio + HOST_CAP);
>>> + if (hpriv->saved_cap)
>>> + cap = (cap & ~(HOST_CAP_SSS | HOST_CAP_MPS)) | hpriv->saved_cap;
>>> + hpriv->saved_cap = cap;
>>>
>>> /* CAP2 register is only defined for AHCI 1.2 and later */
>>> vers = readl(mmio + HOST_VERSION);
>>> @@ -519,7 +532,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
>>> /* Override the HBA ports mapping if the platform needs it */
>>> port_map = readl(mmio + HOST_PORTS_IMPL);
>>> if (hpriv->saved_port_map && port_map != hpriv->saved_port_map) {
>>> - dev_info(dev, "forcing port_map 0x%x -> 0x%x\n",
>>> + dev_info(dev, "forcing port_map 0x%lx -> 0x%x\n",
>>
>
>> This change is not necessary.
>
> It is. The port_map type has been changed.
Ignore. When I read the patches the other day, the mailer font had that "l" look
like a "1" :) My mistake.
>
>>
>>> port_map, hpriv->saved_port_map);
>>> port_map = hpriv->saved_port_map;
>>> } else {
>>> @@ -527,7 +540,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
>>> }
>>>
>>> if (hpriv->mask_port_map) {
>>> - dev_warn(dev, "masking port_map 0x%x -> 0x%x\n",
>>> + dev_warn(dev, "masking port_map 0x%lx -> 0x%lx\n",
>>
>> Same.
>
> ditto
>
>>
>>> port_map,
>>> port_map & hpriv->mask_port_map);
>>> port_map &= hpriv->mask_port_map;
>>> @@ -546,7 +559,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
>>> */
>>> if (map_ports > ahci_nr_ports(cap)) {
>>> dev_warn(dev,
>>> - "implemented port map (0x%x) contains more ports than nr_ports (%u), using nr_ports\n",
>>> + "implemented port map (0x%lx) contains more ports than nr_ports (%u), using nr_ports\n",
>>
>> Same.
>
> ditto.
>
>>
>>> port_map, ahci_nr_ports(cap));
>>> port_map = 0;
>>> }
>>> @@ -555,12 +568,26 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
>>> /* fabricate port_map from cap.nr_ports for < AHCI 1.3 */
>>> if (!port_map && vers < 0x10300) {
>>> port_map = (1 << ahci_nr_ports(cap)) - 1;
>>> - dev_warn(dev, "forcing PORTS_IMPL to 0x%x\n", port_map);
>>> + dev_warn(dev, "forcing PORTS_IMPL to 0x%lx\n", port_map);
>>
>> And again not needed.
>
> and ditto.
>
>>
>>>
>>> /* write the fixed up value to the PI register */
>>> hpriv->saved_port_map = port_map;
>>> }
>>>
>>> + /*
>>> + * Preserve the ports capabilities defined by the platform. Note there
>>> + * is no need in storing the rest of the P#.CMD fields since they are
>>> + * volatile.
>>> + */
>>> + for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
>>> + if (hpriv->saved_port_cap[i])
>>> + continue;
>>> +
>>> + port_mmio = __ahci_port_base(hpriv, i);
>>> + hpriv->saved_port_cap[i] =
>>> + readl(port_mmio + PORT_CMD) & PORT_CMD_CAP;
>>> + }
>>> +
>>> /* record values to use during operation */
>>> hpriv->cap = cap;
>>> hpriv->cap2 = cap2;
>>> @@ -590,13 +617,21 @@ EXPORT_SYMBOL_GPL(ahci_save_initial_config);
>>> static void ahci_restore_initial_config(struct ata_host *host)
>>> {
>>> struct ahci_host_priv *hpriv = host->private_data;
>>> + unsigned long port_map = hpriv->port_map;
>>> void __iomem *mmio = hpriv->mmio;
>>> + void __iomem *port_mmio;
>>> + int i;
>>>
>>> writel(hpriv->saved_cap, mmio + HOST_CAP);
>>> if (hpriv->saved_cap2)
>>> writel(hpriv->saved_cap2, mmio + HOST_CAP2);
>>> writel(hpriv->saved_port_map, mmio + HOST_PORTS_IMPL);
>>> (void) readl(mmio + HOST_PORTS_IMPL); /* flush */
>>> +
>>> + for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
>>> + port_mmio = __ahci_port_base(hpriv, i);
>>> + writel(hpriv->saved_port_cap[i], port_mmio + PORT_CMD);
>>> + }
>>> }
>>>
>>> static unsigned ahci_scr_offset(struct ata_port *ap, unsigned int sc_reg)
>>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>>> index efe640603f3f..8b542a8bc487 100644
>>> --- a/drivers/ata/libahci_platform.c
>>> +++ b/drivers/ata/libahci_platform.c
>>> @@ -23,6 +23,7 @@
>>> #include <linux/pm_runtime.h>
>>> #include <linux/of_platform.h>
>>> #include <linux/reset.h>
>>> +
>>
>> white line change.
>
> Ok. I'll drop it.
>
> -Sergey
>
>>
>>> #include "ahci.h"
>>>
>>> static void ahci_host_stop(struct ata_host *host);
>>> @@ -383,6 +384,34 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
>>> return rc;
>>> }
>>>
>>> +static int ahci_platform_get_firmware(struct ahci_host_priv *hpriv,
>>> + struct device *dev)
>>> +{
>>> + struct device_node *child;
>>> + u32 port;
>>> +
>>> + if (!of_property_read_u32(dev->of_node, "hba-cap", &hpriv->saved_cap))
>>> + hpriv->saved_cap &= (HOST_CAP_SSS | HOST_CAP_MPS);
>>> +
>>> + of_property_read_u32(dev->of_node,
>>> + "ports-implemented", &hpriv->saved_port_map);
>>> +
>>> + for_each_child_of_node(dev->of_node, child) {
>>> + if (!of_device_is_available(child))
>>> + continue;
>>> +
>>> + if (of_property_read_u32(child, "reg", &port)) {
>>> + of_node_put(child);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (!of_property_read_u32(child, "hba-port-cap", &hpriv->saved_port_cap[port]))
>>> + hpriv->saved_port_cap[port] &= PORT_CMD_CAP;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> /**
>>> * ahci_platform_get_resources - Get platform resources
>>> * @pdev: platform device to get resources for
>>> @@ -523,9 +552,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>>> goto err_out;
>>> }
>>>
>>> - of_property_read_u32(dev->of_node,
>>> - "ports-implemented", &hpriv->saved_port_map);
>>> -
>>> if (child_nodes) {
>>> for_each_child_of_node(dev->of_node, child) {
>>> u32 port;
>>> @@ -590,6 +616,15 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>>> if (rc == -EPROBE_DEFER)
>>> goto err_out;
>>> }
>>> +
>>> + /*
>>> + * Retrieve firmware-specific flags which then will be used to set
>>> + * the HW-init fields of HBA and its ports
>>> + */
>>> + rc = ahci_platform_get_firmware(hpriv, dev);
>>> + if (rc)
>>> + goto err_out;
>>> +
>>> pm_runtime_enable(dev);
>>> pm_runtime_get_sync(dev);
>>> hpriv->got_runtime_pm = true;
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
--
Damien Le Moal
Western Digital Research
On Thu, Jun 16, 2022 at 09:29:50AM +0900, Damien Le Moal wrote:
> On 2022/06/16 6:11, Serge Semin wrote:
> > On Tue, Jun 14, 2022 at 05:42:35PM +0900, Damien Le Moal wrote:
> >> On 6/10/22 17:17, Serge Semin wrote:
> >>> There are systems with no BIOS or comprehensive embedded firmware which
> >>> could be able to properly initialize the SATA AHCI controller
> >>> platform-specific capabilities. In that case a good alternative to having
> >>> a clever bootloader is to create a device tree node with the properties
> >>> well describing all the AHCI-related platform specifics. All the settings
> >>> which are normally detected and marked as available in the HBA and its
> >>> ports capabilities fields [1] could be defined in the platform DTB by
> >>> means of a set of the dedicated properties. Such approach perfectly fits
> >>> to the DTB-philosophy - to provide hardware/platform description.
> >>>
> >>> So here we suggest to extend the SATA AHCI device tree bindings with two
> >>> additional DT-properties:
> >>> 1) "hba-cap" - HBA platform generic capabilities like:
> >>> - SSS - Staggered Spin-up support.
> >>> - SMPS - Mechanical Presence Switch support.
> >>> 2) "hba-port-cap" - HBA platform port capabilities like:
> >>> - HPCP - Hot Plug Capable Port.
> >>> - MPSP - Mechanical Presence Switch Attached to Port.
> >>> - CPD - Cold Presence Detection.
> >>> - ESP - External SATA Port.
> >>> - FBSCP - FIS-based Switching Capable Port.
> >>> All of these capabilities require to have a corresponding hardware
> >>> configuration. Thus it's ok to have them defined in DTB.
> >>>
> >>> Even though the driver currently takes into account the state of the ESP
> >>> and FBSCP flags state only, there is nothing wrong with having all of them
> >>> supported by the generic AHCI library in order to have a complete OF-based
> >>> platform-capabilities initialization procedure. These properties will be
> >>> parsed in the ahci_platform_get_resources() method and their values will
> >>> be stored in the saved_* fields of the ahci_host_priv structure, which in
> >>> its turn then will be used to restore the H.CAP, H.PI and P#.CMD
> >>> capability fields on device init and after HBA reset.
> >>>
> >>> Please note this modification concerns the HW-init HBA and its ports flags
> >>> only, which are by specification [1] are supposed to be initialized by the
> >>> BIOS/platform firmware/expansion ROM and which are normally declared in
> >>> the one-time-writable-after-reset register fields. Even though these flags
> >>> aren't supposed to be cleared after HBA reset some AHCI instances may
> >>> violate that rule so we still need to perform the fields resetting after
> >>> each reset. Luckily the corresponding functionality has already been
> >>> partly implemented in the framework of the ahci_save_initial_config() and
> >>> ahci_restore_initial_config() methods.
> >>>
> >>> [1] Serial ATA AHCI 1.3.1 Specification, p. 103
> >>>
> >>> Signed-off-by: Serge Semin <[email protected]>
> >>>
> >>> ---
> >>>
> >>> Changelog v4:
> >>> - Convert the boolean properties to the bitfield DT-properties. (@Rob)
> >>> ---
> >>> drivers/ata/ahci.h | 1 +
> >>> drivers/ata/libahci.c | 51 ++++++++++++++++++++++++++++------
> >>> drivers/ata/libahci_platform.c | 41 +++++++++++++++++++++++++--
> >>> 3 files changed, 82 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> >>> index 8b9826533ae5..0de221055961 100644
> >>> --- a/drivers/ata/ahci.h
> >>> +++ b/drivers/ata/ahci.h
> >>> @@ -337,6 +337,7 @@ struct ahci_host_priv {
> >>> u32 saved_cap; /* saved initial cap */
> >>> u32 saved_cap2; /* saved initial cap2 */
> >>> u32 saved_port_map; /* saved initial port_map */
> >>> + u32 saved_port_cap[AHCI_MAX_PORTS]; /* saved port_cap */
> >>> u32 em_loc; /* enclosure management location */
> >>> u32 em_buf_sz; /* EM buffer size in byte */
> >>> u32 em_msg_type; /* EM message type */
> >>> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> >>> index 1ffaa5f5f21a..954386a2b500 100644
> >>> --- a/drivers/ata/libahci.c
> >>> +++ b/drivers/ata/libahci.c
> >>> @@ -16,6 +16,7 @@
> >>> * http://www.intel.com/technology/serialata/pdf/rev1_1.pdf
> >>> */
> >>>
> >>> +#include <linux/bitops.h>
> >>> #include <linux/kernel.h>
> >>> #include <linux/gfp.h>
> >>> #include <linux/module.h>
> >>> @@ -443,16 +444,28 @@ static ssize_t ahci_show_em_supported(struct device *dev,
> >>> void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> >>> {
> >>> void __iomem *mmio = hpriv->mmio;
> >>> - u32 cap, cap2, vers, port_map;
> >>> + void __iomem *port_mmio;
> >>> + unsigned long port_map;
> >>> + u32 cap, cap2, vers;
> >>> int i;
> >>>
> >>> /* make sure AHCI mode is enabled before accessing CAP */
> >>> ahci_enable_ahci(mmio);
> >>>
> >>> - /* Values prefixed with saved_ are written back to host after
> >>> - * reset. Values without are used for driver operation.
> >>> + /*
> >>> + * Values prefixed with saved_ are written back to the HBA and ports
> >>> + * registers after reset. Values without are used for driver operation.
> >>> + */
> >>> +
> >>> + /*
> >>> + * Override HW-init HBA capability fields with the platform-specific
> >>> + * values. The rest of the HBA capabilities are defined as Read-only
> >>> + * and can't be modified in CSR anyway.
> >>> */
> >>> - hpriv->saved_cap = cap = readl(mmio + HOST_CAP);
> >>> + cap = readl(mmio + HOST_CAP);
> >>> + if (hpriv->saved_cap)
> >>> + cap = (cap & ~(HOST_CAP_SSS | HOST_CAP_MPS)) | hpriv->saved_cap;
> >>> + hpriv->saved_cap = cap;
> >>>
> >>> /* CAP2 register is only defined for AHCI 1.2 and later */
> >>> vers = readl(mmio + HOST_VERSION);
> >>> @@ -519,7 +532,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> >>> /* Override the HBA ports mapping if the platform needs it */
> >>> port_map = readl(mmio + HOST_PORTS_IMPL);
> >>> if (hpriv->saved_port_map && port_map != hpriv->saved_port_map) {
> >>> - dev_info(dev, "forcing port_map 0x%x -> 0x%x\n",
> >>> + dev_info(dev, "forcing port_map 0x%lx -> 0x%x\n",
> >>
> >
> >> This change is not necessary.
> >
> > It is. The port_map type has been changed.
>
> Ignore. When I read the patches the other day, the mailer font had that "l" look
> like a "1" :) My mistake.
Ok.)
-Sergey
>
> >
> >>
> >>> port_map, hpriv->saved_port_map);
> >>> port_map = hpriv->saved_port_map;
> >>> } else {
> >>> @@ -527,7 +540,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> >>> }
> >>>
> >>> if (hpriv->mask_port_map) {
> >>> - dev_warn(dev, "masking port_map 0x%x -> 0x%x\n",
> >>> + dev_warn(dev, "masking port_map 0x%lx -> 0x%lx\n",
> >>
> >> Same.
> >
> > ditto
> >
> >>
> >>> port_map,
> >>> port_map & hpriv->mask_port_map);
> >>> port_map &= hpriv->mask_port_map;
> >>> @@ -546,7 +559,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> >>> */
> >>> if (map_ports > ahci_nr_ports(cap)) {
> >>> dev_warn(dev,
> >>> - "implemented port map (0x%x) contains more ports than nr_ports (%u), using nr_ports\n",
> >>> + "implemented port map (0x%lx) contains more ports than nr_ports (%u), using nr_ports\n",
> >>
> >> Same.
> >
> > ditto.
> >
> >>
> >>> port_map, ahci_nr_ports(cap));
> >>> port_map = 0;
> >>> }
> >>> @@ -555,12 +568,26 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> >>> /* fabricate port_map from cap.nr_ports for < AHCI 1.3 */
> >>> if (!port_map && vers < 0x10300) {
> >>> port_map = (1 << ahci_nr_ports(cap)) - 1;
> >>> - dev_warn(dev, "forcing PORTS_IMPL to 0x%x\n", port_map);
> >>> + dev_warn(dev, "forcing PORTS_IMPL to 0x%lx\n", port_map);
> >>
> >> And again not needed.
> >
> > and ditto.
> >
> >>
> >>>
> >>> /* write the fixed up value to the PI register */
> >>> hpriv->saved_port_map = port_map;
> >>> }
> >>>
> >>> + /*
> >>> + * Preserve the ports capabilities defined by the platform. Note there
> >>> + * is no need in storing the rest of the P#.CMD fields since they are
> >>> + * volatile.
> >>> + */
> >>> + for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
> >>> + if (hpriv->saved_port_cap[i])
> >>> + continue;
> >>> +
> >>> + port_mmio = __ahci_port_base(hpriv, i);
> >>> + hpriv->saved_port_cap[i] =
> >>> + readl(port_mmio + PORT_CMD) & PORT_CMD_CAP;
> >>> + }
> >>> +
> >>> /* record values to use during operation */
> >>> hpriv->cap = cap;
> >>> hpriv->cap2 = cap2;
> >>> @@ -590,13 +617,21 @@ EXPORT_SYMBOL_GPL(ahci_save_initial_config);
> >>> static void ahci_restore_initial_config(struct ata_host *host)
> >>> {
> >>> struct ahci_host_priv *hpriv = host->private_data;
> >>> + unsigned long port_map = hpriv->port_map;
> >>> void __iomem *mmio = hpriv->mmio;
> >>> + void __iomem *port_mmio;
> >>> + int i;
> >>>
> >>> writel(hpriv->saved_cap, mmio + HOST_CAP);
> >>> if (hpriv->saved_cap2)
> >>> writel(hpriv->saved_cap2, mmio + HOST_CAP2);
> >>> writel(hpriv->saved_port_map, mmio + HOST_PORTS_IMPL);
> >>> (void) readl(mmio + HOST_PORTS_IMPL); /* flush */
> >>> +
> >>> + for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
> >>> + port_mmio = __ahci_port_base(hpriv, i);
> >>> + writel(hpriv->saved_port_cap[i], port_mmio + PORT_CMD);
> >>> + }
> >>> }
> >>>
> >>> static unsigned ahci_scr_offset(struct ata_port *ap, unsigned int sc_reg)
> >>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> >>> index efe640603f3f..8b542a8bc487 100644
> >>> --- a/drivers/ata/libahci_platform.c
> >>> +++ b/drivers/ata/libahci_platform.c
> >>> @@ -23,6 +23,7 @@
> >>> #include <linux/pm_runtime.h>
> >>> #include <linux/of_platform.h>
> >>> #include <linux/reset.h>
> >>> +
> >>
> >> white line change.
> >
> > Ok. I'll drop it.
> >
> > -Sergey
> >
> >>
> >>> #include "ahci.h"
> >>>
> >>> static void ahci_host_stop(struct ata_host *host);
> >>> @@ -383,6 +384,34 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
> >>> return rc;
> >>> }
> >>>
> >>> +static int ahci_platform_get_firmware(struct ahci_host_priv *hpriv,
> >>> + struct device *dev)
> >>> +{
> >>> + struct device_node *child;
> >>> + u32 port;
> >>> +
> >>> + if (!of_property_read_u32(dev->of_node, "hba-cap", &hpriv->saved_cap))
> >>> + hpriv->saved_cap &= (HOST_CAP_SSS | HOST_CAP_MPS);
> >>> +
> >>> + of_property_read_u32(dev->of_node,
> >>> + "ports-implemented", &hpriv->saved_port_map);
> >>> +
> >>> + for_each_child_of_node(dev->of_node, child) {
> >>> + if (!of_device_is_available(child))
> >>> + continue;
> >>> +
> >>> + if (of_property_read_u32(child, "reg", &port)) {
> >>> + of_node_put(child);
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + if (!of_property_read_u32(child, "hba-port-cap", &hpriv->saved_port_cap[port]))
> >>> + hpriv->saved_port_cap[port] &= PORT_CMD_CAP;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> /**
> >>> * ahci_platform_get_resources - Get platform resources
> >>> * @pdev: platform device to get resources for
> >>> @@ -523,9 +552,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >>> goto err_out;
> >>> }
> >>>
> >>> - of_property_read_u32(dev->of_node,
> >>> - "ports-implemented", &hpriv->saved_port_map);
> >>> -
> >>> if (child_nodes) {
> >>> for_each_child_of_node(dev->of_node, child) {
> >>> u32 port;
> >>> @@ -590,6 +616,15 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >>> if (rc == -EPROBE_DEFER)
> >>> goto err_out;
> >>> }
> >>> +
> >>> + /*
> >>> + * Retrieve firmware-specific flags which then will be used to set
> >>> + * the HW-init fields of HBA and its ports
> >>> + */
> >>> + rc = ahci_platform_get_firmware(hpriv, dev);
> >>> + if (rc)
> >>> + goto err_out;
> >>> +
> >>> pm_runtime_enable(dev);
> >>> pm_runtime_get_sync(dev);
> >>> hpriv->got_runtime_pm = true;
> >>
> >>
> >> --
> >> Damien Le Moal
> >> Western Digital Research
>
>
> --
> Damien Le Moal
> Western Digital Research
On Thu, Jun 16, 2022 at 09:28:18AM +0900, Damien Le Moal wrote:
> On 2022/06/16 5:58, Serge Semin wrote:
> > On Tue, Jun 14, 2022 at 05:32:41PM +0900, Damien Le Moal wrote:
> >> On 6/10/22 17:17, Serge Semin wrote:
> >>> Currently not all of the Port-specific capabilities listed in the
> >>
> >> s/listed/are listed
> >>
> >>> PORT_CMD-enumeration. Let's extend that set with the Cold Presence
> >>> Detection and Mechanical Presence Switch attached to the Port flags [1] so
> >>> to closeup the set of the platform-specific port-capabilities flags. Note
> >>> these flags are supposed to be set by the platform firmware if there is
> >>> one. Alternatively as we are about to do they can be set by means of the
> >>> OF properties.
> >>>
> >>> While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DMPS and fix the
> >>> comment there. In accordance with [2] that IRQ flag is supposed to
> >>> indicate the state of the signal coming from the Mechanical Presence
> >>> Switch.
> >>>
> >>> [1] Serial ATA AHCI 1.3.1 Specification, p.27
> >>> [2] Serial ATA AHCI 1.3.1 Specification, p.24, p.88
> >>>
> >>> Signed-off-by: Serge Semin <[email protected]>
> >>> Reviewed-by: Hannes Reinecke <[email protected]>
> >>>
> >>> ---
> >>>
> >>> Changelog v4:
> >>> - Fix the DMPS macros name in the patch log. (@Sergei Shtylyov)
> >>> ---
> >>> drivers/ata/ahci.h | 7 ++++++-
> >>> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> >>> index 7d834deefeb9..f501531bd1b3 100644
> >>> --- a/drivers/ata/ahci.h
> >>> +++ b/drivers/ata/ahci.h
> >>> @@ -138,7 +138,7 @@ enum {
> >>> PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */
> >>>
> >>> PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */
> >>> - PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */
> >>> + PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */
> >>> PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */
> >>> PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */
> >>> PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */
> >>> @@ -166,6 +166,8 @@ enum {
> >>> PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
> >>> PORT_CMD_FBSCP = (1 << 22), /* FBS Capable Port */
> >>> PORT_CMD_ESP = (1 << 21), /* External Sata Port */
> >>> + PORT_CMD_CPD = (1 << 20), /* Cold Presence Detection */
> >>> + PORT_CMD_MPSP = (1 << 19), /* Mechanical Presence Switch */
> >>> PORT_CMD_HPCP = (1 << 18), /* HotPlug Capable Port */
> >>> PORT_CMD_PMP = (1 << 17), /* PMP attached */
> >>> PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
> >>> @@ -181,6 +183,9 @@ enum {
> >>> PORT_CMD_ICC_PARTIAL = (0x2 << 28), /* Put i/f in partial state */
> >>> PORT_CMD_ICC_SLUMBER = (0x6 << 28), /* Put i/f in slumber state */
> >>>
> >>> + PORT_CMD_CAP = PORT_CMD_HPCP | PORT_CMD_MPSP |
> >>> + PORT_CMD_CPD | PORT_CMD_ESP | PORT_CMD_FBSCP,
> >>
> >
> >> What is this one for ? A comment above it would be nice.
> >
> > Isn't it obviously inferrable from the definition and the item name?
>
> I am guessing from the name. Am I guessing OK ? A comment would still be nice.
> Why just these bits ? There are more cap/support indicator bits in that port cmd
> bitfield. So why this particular set of bits ? What do they mean all together ?
Normally the variable/constant name should be self-content (as the
kernel coding style doc states and what the common sense suggests). So
the reader could correctly guess its purpose/content/value. In this
case PORT_CMD_CAP - means PORT CMD capabilities mask. All of the
possible flags have been set in that mask. There are no more
capabilities in the PORT CMD register left undeclared. That's why the
name is selected the way it is and why I haven't added any comment in
here (what the kernel coding style says about the over-commenting the
code).
>
> Sure I can go and read the specs to figure it out. But again, a comment would
> avoid readers of the code to have to decrypt all that.
If you still insist on having an additional comment. I can add
something like "/* PORT_CMD capabilities mask */". Are you ok with it?
-Sergey
>
> >
> > -Sergey
> >
> >>
> >>> +
> >>> /* PORT_FBS bits */
> >>> PORT_FBS_DWE_OFFSET = 16, /* FBS device with error offset */
> >>> PORT_FBS_ADO_OFFSET = 12, /* FBS active dev optimization offset */
> >>
> >>
> >> --
> >> Damien Le Moal
> >> Western Digital Research
>
>
> --
> Damien Le Moal
> Western Digital Research
On 6/18/22 05:31, Serge Semin wrote:
> On Thu, Jun 16, 2022 at 09:28:18AM +0900, Damien Le Moal wrote:
>> On 2022/06/16 5:58, Serge Semin wrote:
>>> On Tue, Jun 14, 2022 at 05:32:41PM +0900, Damien Le Moal wrote:
>>>> On 6/10/22 17:17, Serge Semin wrote:
>>>>> Currently not all of the Port-specific capabilities listed in the
>>>>
>>>> s/listed/are listed
>>>>
>>>>> PORT_CMD-enumeration. Let's extend that set with the Cold Presence
>>>>> Detection and Mechanical Presence Switch attached to the Port flags [1] so
>>>>> to closeup the set of the platform-specific port-capabilities flags. Note
>>>>> these flags are supposed to be set by the platform firmware if there is
>>>>> one. Alternatively as we are about to do they can be set by means of the
>>>>> OF properties.
>>>>>
>>>>> While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DMPS and fix the
>>>>> comment there. In accordance with [2] that IRQ flag is supposed to
>>>>> indicate the state of the signal coming from the Mechanical Presence
>>>>> Switch.
>>>>>
>>>>> [1] Serial ATA AHCI 1.3.1 Specification, p.27
>>>>> [2] Serial ATA AHCI 1.3.1 Specification, p.24, p.88
>>>>>
>>>>> Signed-off-by: Serge Semin <[email protected]>
>>>>> Reviewed-by: Hannes Reinecke <[email protected]>
>>>>>
>>>>> ---
>>>>>
>>>>> Changelog v4:
>>>>> - Fix the DMPS macros name in the patch log. (@Sergei Shtylyov)
>>>>> ---
>>>>> drivers/ata/ahci.h | 7 ++++++-
>>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>>>>> index 7d834deefeb9..f501531bd1b3 100644
>>>>> --- a/drivers/ata/ahci.h
>>>>> +++ b/drivers/ata/ahci.h
>>>>> @@ -138,7 +138,7 @@ enum {
>>>>> PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */
>>>>>
>>>>> PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */
>>>>> - PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */
>>>>> + PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */
>>>>> PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */
>>>>> PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */
>>>>> PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */
>>>>> @@ -166,6 +166,8 @@ enum {
>>>>> PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
>>>>> PORT_CMD_FBSCP = (1 << 22), /* FBS Capable Port */
>>>>> PORT_CMD_ESP = (1 << 21), /* External Sata Port */
>>>>> + PORT_CMD_CPD = (1 << 20), /* Cold Presence Detection */
>>>>> + PORT_CMD_MPSP = (1 << 19), /* Mechanical Presence Switch */
>>>>> PORT_CMD_HPCP = (1 << 18), /* HotPlug Capable Port */
>>>>> PORT_CMD_PMP = (1 << 17), /* PMP attached */
>>>>> PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
>>>>> @@ -181,6 +183,9 @@ enum {
>>>>> PORT_CMD_ICC_PARTIAL = (0x2 << 28), /* Put i/f in partial state */
>>>>> PORT_CMD_ICC_SLUMBER = (0x6 << 28), /* Put i/f in slumber state */
>>>>>
>>>>> + PORT_CMD_CAP = PORT_CMD_HPCP | PORT_CMD_MPSP |
>>>>> + PORT_CMD_CPD | PORT_CMD_ESP | PORT_CMD_FBSCP,
>>>>
>>>
>>>> What is this one for ? A comment above it would be nice.
>>>
>>> Isn't it obviously inferrable from the definition and the item name?
>>
>
>> I am guessing from the name. Am I guessing OK ? A comment would still be nice.
>> Why just these bits ? There are more cap/support indicator bits in that port cmd
>> bitfield. So why this particular set of bits ? What do they mean all together ?
>
> Normally the variable/constant name should be self-content (as the
> kernel coding style doc states and what the common sense suggests). So
> the reader could correctly guess its purpose/content/value. In this
> case PORT_CMD_CAP - means PORT CMD capabilities mask. All of the
> possible flags have been set in that mask. There are no more
> capabilities in the PORT CMD register left undeclared. That's why the
> name is selected the way it is and why I haven't added any comment in
> here (what the kernel coding style says about the over-commenting the
> code).
Yes, I understood from the name what it is. What I do NOT understand is
why all the feature bits are not there. Why this subset only ? A comment
about that would be nice so that the reason for it is not lost.
>
>>
>> Sure I can go and read the specs to figure it out. But again, a comment would
>> avoid readers of the code to have to decrypt all that.
>
> If you still insist on having an additional comment. I can add
> something like "/* PORT_CMD capabilities mask */". Are you ok with it?
That does not help on its own. The macro name says that already. I would
like a note about why only these features are selected.
>
> -Sergey
>
>>
>>>
>>> -Sergey
>>>
>>>>
>>>>> +
>>>>> /* PORT_FBS bits */
>>>>> PORT_FBS_DWE_OFFSET = 16, /* FBS device with error offset */
>>>>> PORT_FBS_ADO_OFFSET = 12, /* FBS active dev optimization offset */
>>>>
>>>>
>>>> --
>>>> Damien Le Moal
>>>> Western Digital Research
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
--
Damien Le Moal
Western Digital Research
On Sat, Jun 18, 2022 at 03:52:28PM +0900, Damien Le Moal wrote:
> On 6/18/22 05:31, Serge Semin wrote:
> > On Thu, Jun 16, 2022 at 09:28:18AM +0900, Damien Le Moal wrote:
> >> On 2022/06/16 5:58, Serge Semin wrote:
> >>> On Tue, Jun 14, 2022 at 05:32:41PM +0900, Damien Le Moal wrote:
> >>>> On 6/10/22 17:17, Serge Semin wrote:
> >>>>> Currently not all of the Port-specific capabilities listed in the
> >>>>
> >>>> s/listed/are listed
> >>>>
> >>>>> PORT_CMD-enumeration. Let's extend that set with the Cold Presence
> >>>>> Detection and Mechanical Presence Switch attached to the Port flags [1] so
> >>>>> to closeup the set of the platform-specific port-capabilities flags. Note
> >>>>> these flags are supposed to be set by the platform firmware if there is
> >>>>> one. Alternatively as we are about to do they can be set by means of the
> >>>>> OF properties.
> >>>>>
> >>>>> While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DMPS and fix the
> >>>>> comment there. In accordance with [2] that IRQ flag is supposed to
> >>>>> indicate the state of the signal coming from the Mechanical Presence
> >>>>> Switch.
> >>>>>
> >>>>> [1] Serial ATA AHCI 1.3.1 Specification, p.27
> >>>>> [2] Serial ATA AHCI 1.3.1 Specification, p.24, p.88
> >>>>>
> >>>>> Signed-off-by: Serge Semin <[email protected]>
> >>>>> Reviewed-by: Hannes Reinecke <[email protected]>
> >>>>>
> >>>>> ---
> >>>>>
> >>>>> Changelog v4:
> >>>>> - Fix the DMPS macros name in the patch log. (@Sergei Shtylyov)
> >>>>> ---
> >>>>> drivers/ata/ahci.h | 7 ++++++-
> >>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> >>>>> index 7d834deefeb9..f501531bd1b3 100644
> >>>>> --- a/drivers/ata/ahci.h
> >>>>> +++ b/drivers/ata/ahci.h
> >>>>> @@ -138,7 +138,7 @@ enum {
> >>>>> PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */
> >>>>>
> >>>>> PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */
> >>>>> - PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */
> >>>>> + PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */
> >>>>> PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */
> >>>>> PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */
> >>>>> PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */
> >>>>> @@ -166,6 +166,8 @@ enum {
> >>>>> PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
> >>>>> PORT_CMD_FBSCP = (1 << 22), /* FBS Capable Port */
> >>>>> PORT_CMD_ESP = (1 << 21), /* External Sata Port */
> >>>>> + PORT_CMD_CPD = (1 << 20), /* Cold Presence Detection */
> >>>>> + PORT_CMD_MPSP = (1 << 19), /* Mechanical Presence Switch */
> >>>>> PORT_CMD_HPCP = (1 << 18), /* HotPlug Capable Port */
> >>>>> PORT_CMD_PMP = (1 << 17), /* PMP attached */
> >>>>> PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
> >>>>> @@ -181,6 +183,9 @@ enum {
> >>>>> PORT_CMD_ICC_PARTIAL = (0x2 << 28), /* Put i/f in partial state */
> >>>>> PORT_CMD_ICC_SLUMBER = (0x6 << 28), /* Put i/f in slumber state */
> >>>>>
> >>>>> + PORT_CMD_CAP = PORT_CMD_HPCP | PORT_CMD_MPSP |
> >>>>> + PORT_CMD_CPD | PORT_CMD_ESP | PORT_CMD_FBSCP,
> >>>>
> >>>
> >>>> What is this one for ? A comment above it would be nice.
> >>>
> >>> Isn't it obviously inferrable from the definition and the item name?
> >>
> >
> >> I am guessing from the name. Am I guessing OK ? A comment would still be nice.
> >> Why just these bits ? There are more cap/support indicator bits in that port cmd
> >> bitfield. So why this particular set of bits ? What do they mean all together ?
> >
> > Normally the variable/constant name should be self-content (as the
> > kernel coding style doc states and what the common sense suggests). So
> > the reader could correctly guess its purpose/content/value. In this
> > case PORT_CMD_CAP - means PORT CMD capabilities mask. All of the
> > possible flags have been set in that mask. There are no more
> > capabilities in the PORT CMD register left undeclared. That's why the
> > name is selected the way it is and why I haven't added any comment in
> > here (what the kernel coding style says about the over-commenting the
> > code).
>
> Yes, I understood from the name what it is. What I do NOT understand is
> why all the feature bits are not there. Why this subset only ? A comment
> about that would be nice so that the reason for it is not lost.
Well, because it's indeed "PORT_CMD capabilities mask", and not features,
not setups, not settings, not status flags, etc. As I said all the port
Capabilities have been listed in that mask:
PORT_CMD_FBSCP BIT(22) - FIS-based Switching Capable Port
PORT_CMD_ESP BIT(21) - External SATA Port
PORT_CMD_CPD BIT(20) - Cold Presence Detect
PORT_CMD_MPSP BIT(19) - Mechanical Presence Switch Attached to Port
PORT_CMD_HPCP BIT(18) - Hot Plug Capable Port
I've or'ed-them-up in a single mask => PORT_CMD_CAP in order to work
with them independently from the rest of the PORT_CMD CSR fields.
Unlike the generic controller CAP/CAP2 registers, which consists of the
device capabilities only, PORT_CMD contains various R/W settings (PM, LED
driver, etc), RO status flags (CMD-list running, FIS recv running, etc)
and amongst other the RO/Wo !port-specific capabilities!. The later ones
indicate the platform-specific device features. Since the register
contains flags with the intermixed nature, I need to have a mask to at
least get the capabilities and preserve them between the device
resets. That's why the PORT_CMD_CAP has been introduced in the
framework of this patch. Its name was chosen with a reference to the
CAP registers, see:
HOST_CAP, HOST_CAP2, and finally my PORT_CMD_CAP.
>
> >
> >>
> >> Sure I can go and read the specs to figure it out. But again, a comment would
> >> avoid readers of the code to have to decrypt all that.
> >
> > If you still insist on having an additional comment. I can add
> > something like "/* PORT_CMD capabilities mask */". Are you ok with it?
>
> That does not help on its own. The macro name says that already. I would
> like a note about why only these features are selected.
Please see the explanation above. I don't see what else to say about
that mask, because in short what I said above really means "PORT_CMD
capabilities mask". So should you have some more clever text, which
would be more suitable here, please tell me and I'll add it to the
patch.
Regarding what you said earlier. In order to fully understand the
AHCI driver a hacker would always need to read the specs. There is
just no way to do that effectively enough without the controller
manual at hands. And the PORT_CMD capabilities isn't the most
complicated part of the device.
-Sergey
>
> >
> > -Sergey
> >
> >>
> >>>
> >>> -Sergey
> >>>
> >>>>
> >>>>> +
> >>>>> /* PORT_FBS bits */
> >>>>> PORT_FBS_DWE_OFFSET = 16, /* FBS device with error offset */
> >>>>> PORT_FBS_ADO_OFFSET = 12, /* FBS active dev optimization offset */
> >>>>
> >>>>
> >>>> --
> >>>> Damien Le Moal
> >>>> Western Digital Research
> >>
> >>
> >> --
> >> Damien Le Moal
> >> Western Digital Research
>
>
> --
> Damien Le Moal
> Western Digital Research
Damien,
Any notes to the comments below?
-Sergey
On Sat, Jun 18, 2022 at 11:10:55AM +0300, Serge Semin wrote:
> On Sat, Jun 18, 2022 at 03:52:28PM +0900, Damien Le Moal wrote:
> > On 6/18/22 05:31, Serge Semin wrote:
> > > On Thu, Jun 16, 2022 at 09:28:18AM +0900, Damien Le Moal wrote:
> > >> On 2022/06/16 5:58, Serge Semin wrote:
> > >>> On Tue, Jun 14, 2022 at 05:32:41PM +0900, Damien Le Moal wrote:
> > >>>> On 6/10/22 17:17, Serge Semin wrote:
> > >>>>> Currently not all of the Port-specific capabilities listed in the
> > >>>>
> > >>>> s/listed/are listed
> > >>>>
> > >>>>> PORT_CMD-enumeration. Let's extend that set with the Cold Presence
> > >>>>> Detection and Mechanical Presence Switch attached to the Port flags [1] so
> > >>>>> to closeup the set of the platform-specific port-capabilities flags. Note
> > >>>>> these flags are supposed to be set by the platform firmware if there is
> > >>>>> one. Alternatively as we are about to do they can be set by means of the
> > >>>>> OF properties.
> > >>>>>
> > >>>>> While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DMPS and fix the
> > >>>>> comment there. In accordance with [2] that IRQ flag is supposed to
> > >>>>> indicate the state of the signal coming from the Mechanical Presence
> > >>>>> Switch.
> > >>>>>
> > >>>>> [1] Serial ATA AHCI 1.3.1 Specification, p.27
> > >>>>> [2] Serial ATA AHCI 1.3.1 Specification, p.24, p.88
> > >>>>>
> > >>>>> Signed-off-by: Serge Semin <[email protected]>
> > >>>>> Reviewed-by: Hannes Reinecke <[email protected]>
> > >>>>>
> > >>>>> ---
> > >>>>>
> > >>>>> Changelog v4:
> > >>>>> - Fix the DMPS macros name in the patch log. (@Sergei Shtylyov)
> > >>>>> ---
> > >>>>> drivers/ata/ahci.h | 7 ++++++-
> > >>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
> > >>>>>
> > >>>>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> > >>>>> index 7d834deefeb9..f501531bd1b3 100644
> > >>>>> --- a/drivers/ata/ahci.h
> > >>>>> +++ b/drivers/ata/ahci.h
> > >>>>> @@ -138,7 +138,7 @@ enum {
> > >>>>> PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */
> > >>>>>
> > >>>>> PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */
> > >>>>> - PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */
> > >>>>> + PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */
> > >>>>> PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */
> > >>>>> PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */
> > >>>>> PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */
> > >>>>> @@ -166,6 +166,8 @@ enum {
> > >>>>> PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
> > >>>>> PORT_CMD_FBSCP = (1 << 22), /* FBS Capable Port */
> > >>>>> PORT_CMD_ESP = (1 << 21), /* External Sata Port */
> > >>>>> + PORT_CMD_CPD = (1 << 20), /* Cold Presence Detection */
> > >>>>> + PORT_CMD_MPSP = (1 << 19), /* Mechanical Presence Switch */
> > >>>>> PORT_CMD_HPCP = (1 << 18), /* HotPlug Capable Port */
> > >>>>> PORT_CMD_PMP = (1 << 17), /* PMP attached */
> > >>>>> PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
> > >>>>> @@ -181,6 +183,9 @@ enum {
> > >>>>> PORT_CMD_ICC_PARTIAL = (0x2 << 28), /* Put i/f in partial state */
> > >>>>> PORT_CMD_ICC_SLUMBER = (0x6 << 28), /* Put i/f in slumber state */
> > >>>>>
> > >>>>> + PORT_CMD_CAP = PORT_CMD_HPCP | PORT_CMD_MPSP |
> > >>>>> + PORT_CMD_CPD | PORT_CMD_ESP | PORT_CMD_FBSCP,
> > >>>>
> > >>>
> > >>>> What is this one for ? A comment above it would be nice.
> > >>>
> > >>> Isn't it obviously inferrable from the definition and the item name?
> > >>
> > >
> > >> I am guessing from the name. Am I guessing OK ? A comment would still be nice.
> > >> Why just these bits ? There are more cap/support indicator bits in that port cmd
> > >> bitfield. So why this particular set of bits ? What do they mean all together ?
> > >
> > > Normally the variable/constant name should be self-content (as the
> > > kernel coding style doc states and what the common sense suggests). So
> > > the reader could correctly guess its purpose/content/value. In this
> > > case PORT_CMD_CAP - means PORT CMD capabilities mask. All of the
> > > possible flags have been set in that mask. There are no more
> > > capabilities in the PORT CMD register left undeclared. That's why the
> > > name is selected the way it is and why I haven't added any comment in
> > > here (what the kernel coding style says about the over-commenting the
> > > code).
> >
>
> > Yes, I understood from the name what it is. What I do NOT understand is
> > why all the feature bits are not there. Why this subset only ? A comment
> > about that would be nice so that the reason for it is not lost.
>
> Well, because it's indeed "PORT_CMD capabilities mask", and not features,
> not setups, not settings, not status flags, etc. As I said all the port
> Capabilities have been listed in that mask:
> PORT_CMD_FBSCP BIT(22) - FIS-based Switching Capable Port
> PORT_CMD_ESP BIT(21) - External SATA Port
> PORT_CMD_CPD BIT(20) - Cold Presence Detect
> PORT_CMD_MPSP BIT(19) - Mechanical Presence Switch Attached to Port
> PORT_CMD_HPCP BIT(18) - Hot Plug Capable Port
> I've or'ed-them-up in a single mask => PORT_CMD_CAP in order to work
> with them independently from the rest of the PORT_CMD CSR fields.
>
> Unlike the generic controller CAP/CAP2 registers, which consists of the
> device capabilities only, PORT_CMD contains various R/W settings (PM, LED
> driver, etc), RO status flags (CMD-list running, FIS recv running, etc)
> and amongst other the RO/Wo !port-specific capabilities!. The later ones
> indicate the platform-specific device features. Since the register
> contains flags with the intermixed nature, I need to have a mask to at
> least get the capabilities and preserve them between the device
> resets. That's why the PORT_CMD_CAP has been introduced in the
> framework of this patch. Its name was chosen with a reference to the
> CAP registers, see:
> HOST_CAP, HOST_CAP2, and finally my PORT_CMD_CAP.
>
> >
> > >
> > >>
> > >> Sure I can go and read the specs to figure it out. But again, a comment would
> > >> avoid readers of the code to have to decrypt all that.
> > >
> > > If you still insist on having an additional comment. I can add
> > > something like "/* PORT_CMD capabilities mask */". Are you ok with it?
> >
>
> > That does not help on its own. The macro name says that already. I would
> > like a note about why only these features are selected.
>
> Please see the explanation above. I don't see what else to say about
> that mask, because in short what I said above really means "PORT_CMD
> capabilities mask". So should you have some more clever text, which
> would be more suitable here, please tell me and I'll add it to the
> patch.
>
> Regarding what you said earlier. In order to fully understand the
> AHCI driver a hacker would always need to read the specs. There is
> just no way to do that effectively enough without the controller
> manual at hands. And the PORT_CMD capabilities isn't the most
> complicated part of the device.
>
> -Sergey
>
> >
> > >
> > > -Sergey
> > >
> > >>
> > >>>
> > >>> -Sergey
> > >>>
> > >>>>
> > >>>>> +
> > >>>>> /* PORT_FBS bits */
> > >>>>> PORT_FBS_DWE_OFFSET = 16, /* FBS device with error offset */
> > >>>>> PORT_FBS_ADO_OFFSET = 12, /* FBS active dev optimization offset */
> > >>>>
> > >>>>
> > >>>> --
> > >>>> Damien Le Moal
> > >>>> Western Digital Research
> > >>
> > >>
> > >> --
> > >> Damien Le Moal
> > >> Western Digital Research
> >
> >
> > --
> > Damien Le Moal
> > Western Digital Research
On 6/28/22 21:08, Serge Semin wrote:
> Damien,
> Any notes to the comments below?
Been very busy and had no time to look at this. Please post your latest
version of the series and we'll go from there.
>
> -Sergey
>
> On Sat, Jun 18, 2022 at 11:10:55AM +0300, Serge Semin wrote:
>> On Sat, Jun 18, 2022 at 03:52:28PM +0900, Damien Le Moal wrote:
>>> On 6/18/22 05:31, Serge Semin wrote:
>>>> On Thu, Jun 16, 2022 at 09:28:18AM +0900, Damien Le Moal wrote:
>>>>> On 2022/06/16 5:58, Serge Semin wrote:
>>>>>> On Tue, Jun 14, 2022 at 05:32:41PM +0900, Damien Le Moal wrote:
>>>>>>> On 6/10/22 17:17, Serge Semin wrote:
>>>>>>>> Currently not all of the Port-specific capabilities listed in the
>>>>>>>
>>>>>>> s/listed/are listed
>>>>>>>
>>>>>>>> PORT_CMD-enumeration. Let's extend that set with the Cold Presence
>>>>>>>> Detection and Mechanical Presence Switch attached to the Port flags [1] so
>>>>>>>> to closeup the set of the platform-specific port-capabilities flags. Note
>>>>>>>> these flags are supposed to be set by the platform firmware if there is
>>>>>>>> one. Alternatively as we are about to do they can be set by means of the
>>>>>>>> OF properties.
>>>>>>>>
>>>>>>>> While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DMPS and fix the
>>>>>>>> comment there. In accordance with [2] that IRQ flag is supposed to
>>>>>>>> indicate the state of the signal coming from the Mechanical Presence
>>>>>>>> Switch.
>>>>>>>>
>>>>>>>> [1] Serial ATA AHCI 1.3.1 Specification, p.27
>>>>>>>> [2] Serial ATA AHCI 1.3.1 Specification, p.24, p.88
>>>>>>>>
>>>>>>>> Signed-off-by: Serge Semin <[email protected]>
>>>>>>>> Reviewed-by: Hannes Reinecke <[email protected]>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changelog v4:
>>>>>>>> - Fix the DMPS macros name in the patch log. (@Sergei Shtylyov)
>>>>>>>> ---
>>>>>>>> drivers/ata/ahci.h | 7 ++++++-
>>>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>>>>>>>> index 7d834deefeb9..f501531bd1b3 100644
>>>>>>>> --- a/drivers/ata/ahci.h
>>>>>>>> +++ b/drivers/ata/ahci.h
>>>>>>>> @@ -138,7 +138,7 @@ enum {
>>>>>>>> PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */
>>>>>>>>
>>>>>>>> PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */
>>>>>>>> - PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */
>>>>>>>> + PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */
>>>>>>>> PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */
>>>>>>>> PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */
>>>>>>>> PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */
>>>>>>>> @@ -166,6 +166,8 @@ enum {
>>>>>>>> PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
>>>>>>>> PORT_CMD_FBSCP = (1 << 22), /* FBS Capable Port */
>>>>>>>> PORT_CMD_ESP = (1 << 21), /* External Sata Port */
>>>>>>>> + PORT_CMD_CPD = (1 << 20), /* Cold Presence Detection */
>>>>>>>> + PORT_CMD_MPSP = (1 << 19), /* Mechanical Presence Switch */
>>>>>>>> PORT_CMD_HPCP = (1 << 18), /* HotPlug Capable Port */
>>>>>>>> PORT_CMD_PMP = (1 << 17), /* PMP attached */
>>>>>>>> PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
>>>>>>>> @@ -181,6 +183,9 @@ enum {
>>>>>>>> PORT_CMD_ICC_PARTIAL = (0x2 << 28), /* Put i/f in partial state */
>>>>>>>> PORT_CMD_ICC_SLUMBER = (0x6 << 28), /* Put i/f in slumber state */
>>>>>>>>
>>>>>>>> + PORT_CMD_CAP = PORT_CMD_HPCP | PORT_CMD_MPSP |
>>>>>>>> + PORT_CMD_CPD | PORT_CMD_ESP | PORT_CMD_FBSCP,
>>>>>>>
>>>>>>
>>>>>>> What is this one for ? A comment above it would be nice.
>>>>>>
>>>>>> Isn't it obviously inferrable from the definition and the item name?
>>>>>
>>>>
>>>>> I am guessing from the name. Am I guessing OK ? A comment would still be nice.
>>>>> Why just these bits ? There are more cap/support indicator bits in that port cmd
>>>>> bitfield. So why this particular set of bits ? What do they mean all together ?
>>>>
>>>> Normally the variable/constant name should be self-content (as the
>>>> kernel coding style doc states and what the common sense suggests). So
>>>> the reader could correctly guess its purpose/content/value. In this
>>>> case PORT_CMD_CAP - means PORT CMD capabilities mask. All of the
>>>> possible flags have been set in that mask. There are no more
>>>> capabilities in the PORT CMD register left undeclared. That's why the
>>>> name is selected the way it is and why I haven't added any comment in
>>>> here (what the kernel coding style says about the over-commenting the
>>>> code).
>>>
>>
>>> Yes, I understood from the name what it is. What I do NOT understand is
>>> why all the feature bits are not there. Why this subset only ? A comment
>>> about that would be nice so that the reason for it is not lost.
>>
>> Well, because it's indeed "PORT_CMD capabilities mask", and not features,
>> not setups, not settings, not status flags, etc. As I said all the port
>> Capabilities have been listed in that mask:
>> PORT_CMD_FBSCP BIT(22) - FIS-based Switching Capable Port
>> PORT_CMD_ESP BIT(21) - External SATA Port
>> PORT_CMD_CPD BIT(20) - Cold Presence Detect
>> PORT_CMD_MPSP BIT(19) - Mechanical Presence Switch Attached to Port
>> PORT_CMD_HPCP BIT(18) - Hot Plug Capable Port
>> I've or'ed-them-up in a single mask => PORT_CMD_CAP in order to work
>> with them independently from the rest of the PORT_CMD CSR fields.
>>
>> Unlike the generic controller CAP/CAP2 registers, which consists of the
>> device capabilities only, PORT_CMD contains various R/W settings (PM, LED
>> driver, etc), RO status flags (CMD-list running, FIS recv running, etc)
>> and amongst other the RO/Wo !port-specific capabilities!. The later ones
>> indicate the platform-specific device features. Since the register
>> contains flags with the intermixed nature, I need to have a mask to at
>> least get the capabilities and preserve them between the device
>> resets. That's why the PORT_CMD_CAP has been introduced in the
>> framework of this patch. Its name was chosen with a reference to the
>> CAP registers, see:
>> HOST_CAP, HOST_CAP2, and finally my PORT_CMD_CAP.
>>
>>>
>>>>
>>>>>
>>>>> Sure I can go and read the specs to figure it out. But again, a comment would
>>>>> avoid readers of the code to have to decrypt all that.
>>>>
>>>> If you still insist on having an additional comment. I can add
>>>> something like "/* PORT_CMD capabilities mask */". Are you ok with it?
>>>
>>
>>> That does not help on its own. The macro name says that already. I would
>>> like a note about why only these features are selected.
>>
>> Please see the explanation above. I don't see what else to say about
>> that mask, because in short what I said above really means "PORT_CMD
>> capabilities mask". So should you have some more clever text, which
>> would be more suitable here, please tell me and I'll add it to the
>> patch.
>>
>> Regarding what you said earlier. In order to fully understand the
>> AHCI driver a hacker would always need to read the specs. There is
>> just no way to do that effectively enough without the controller
>> manual at hands. And the PORT_CMD capabilities isn't the most
>> complicated part of the device.
>>
>> -Sergey
>>
>>>
>>>>
>>>> -Sergey
>>>>
>>>>>
>>>>>>
>>>>>> -Sergey
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> /* PORT_FBS bits */
>>>>>>>> PORT_FBS_DWE_OFFSET = 16, /* FBS device with error offset */
>>>>>>>> PORT_FBS_ADO_OFFSET = 12, /* FBS active dev optimization offset */
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Damien Le Moal
>>>>>>> Western Digital Research
>>>>>
>>>>>
>>>>> --
>>>>> Damien Le Moal
>>>>> Western Digital Research
>>>
>>>
>>> --
>>> Damien Le Moal
>>> Western Digital Research
--
Damien Le Moal
Western Digital Research
On Wed, Jun 29, 2022 at 10:35:13AM +0900, Damien Le Moal wrote:
> On 6/28/22 21:08, Serge Semin wrote:
> > Damien,
> > Any notes to the comments below?
>
> Been very busy and had no time to look at this. Please post your latest
> version of the series and we'll go from there.
Ok. As soon as I get the responses from Rob.
-Sergey
>
> >
> > -Sergey
> >
> > On Sat, Jun 18, 2022 at 11:10:55AM +0300, Serge Semin wrote:
> >> On Sat, Jun 18, 2022 at 03:52:28PM +0900, Damien Le Moal wrote:
> >>> On 6/18/22 05:31, Serge Semin wrote:
> >>>> On Thu, Jun 16, 2022 at 09:28:18AM +0900, Damien Le Moal wrote:
> >>>>> On 2022/06/16 5:58, Serge Semin wrote:
> >>>>>> On Tue, Jun 14, 2022 at 05:32:41PM +0900, Damien Le Moal wrote:
> >>>>>>> On 6/10/22 17:17, Serge Semin wrote:
> >>>>>>>> Currently not all of the Port-specific capabilities listed in the
> >>>>>>>
> >>>>>>> s/listed/are listed
> >>>>>>>
> >>>>>>>> PORT_CMD-enumeration. Let's extend that set with the Cold Presence
> >>>>>>>> Detection and Mechanical Presence Switch attached to the Port flags [1] so
> >>>>>>>> to closeup the set of the platform-specific port-capabilities flags. Note
> >>>>>>>> these flags are supposed to be set by the platform firmware if there is
> >>>>>>>> one. Alternatively as we are about to do they can be set by means of the
> >>>>>>>> OF properties.
> >>>>>>>>
> >>>>>>>> While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DMPS and fix the
> >>>>>>>> comment there. In accordance with [2] that IRQ flag is supposed to
> >>>>>>>> indicate the state of the signal coming from the Mechanical Presence
> >>>>>>>> Switch.
> >>>>>>>>
> >>>>>>>> [1] Serial ATA AHCI 1.3.1 Specification, p.27
> >>>>>>>> [2] Serial ATA AHCI 1.3.1 Specification, p.24, p.88
> >>>>>>>>
> >>>>>>>> Signed-off-by: Serge Semin <[email protected]>
> >>>>>>>> Reviewed-by: Hannes Reinecke <[email protected]>
> >>>>>>>>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> Changelog v4:
> >>>>>>>> - Fix the DMPS macros name in the patch log. (@Sergei Shtylyov)
> >>>>>>>> ---
> >>>>>>>> drivers/ata/ahci.h | 7 ++++++-
> >>>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> >>>>>>>> index 7d834deefeb9..f501531bd1b3 100644
> >>>>>>>> --- a/drivers/ata/ahci.h
> >>>>>>>> +++ b/drivers/ata/ahci.h
> >>>>>>>> @@ -138,7 +138,7 @@ enum {
> >>>>>>>> PORT_IRQ_BAD_PMP = (1 << 23), /* incorrect port multiplier */
> >>>>>>>>
> >>>>>>>> PORT_IRQ_PHYRDY = (1 << 22), /* PhyRdy changed */
> >>>>>>>> - PORT_IRQ_DEV_ILCK = (1 << 7), /* device interlock */
> >>>>>>>> + PORT_IRQ_DMPS = (1 << 7), /* mechanical presence status */
> >>>>>>>> PORT_IRQ_CONNECT = (1 << 6), /* port connect change status */
> >>>>>>>> PORT_IRQ_SG_DONE = (1 << 5), /* descriptor processed */
> >>>>>>>> PORT_IRQ_UNK_FIS = (1 << 4), /* unknown FIS rx'd */
> >>>>>>>> @@ -166,6 +166,8 @@ enum {
> >>>>>>>> PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
> >>>>>>>> PORT_CMD_FBSCP = (1 << 22), /* FBS Capable Port */
> >>>>>>>> PORT_CMD_ESP = (1 << 21), /* External Sata Port */
> >>>>>>>> + PORT_CMD_CPD = (1 << 20), /* Cold Presence Detection */
> >>>>>>>> + PORT_CMD_MPSP = (1 << 19), /* Mechanical Presence Switch */
> >>>>>>>> PORT_CMD_HPCP = (1 << 18), /* HotPlug Capable Port */
> >>>>>>>> PORT_CMD_PMP = (1 << 17), /* PMP attached */
> >>>>>>>> PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
> >>>>>>>> @@ -181,6 +183,9 @@ enum {
> >>>>>>>> PORT_CMD_ICC_PARTIAL = (0x2 << 28), /* Put i/f in partial state */
> >>>>>>>> PORT_CMD_ICC_SLUMBER = (0x6 << 28), /* Put i/f in slumber state */
> >>>>>>>>
> >>>>>>>> + PORT_CMD_CAP = PORT_CMD_HPCP | PORT_CMD_MPSP |
> >>>>>>>> + PORT_CMD_CPD | PORT_CMD_ESP | PORT_CMD_FBSCP,
> >>>>>>>
> >>>>>>
> >>>>>>> What is this one for ? A comment above it would be nice.
> >>>>>>
> >>>>>> Isn't it obviously inferrable from the definition and the item name?
> >>>>>
> >>>>
> >>>>> I am guessing from the name. Am I guessing OK ? A comment would still be nice.
> >>>>> Why just these bits ? There are more cap/support indicator bits in that port cmd
> >>>>> bitfield. So why this particular set of bits ? What do they mean all together ?
> >>>>
> >>>> Normally the variable/constant name should be self-content (as the
> >>>> kernel coding style doc states and what the common sense suggests). So
> >>>> the reader could correctly guess its purpose/content/value. In this
> >>>> case PORT_CMD_CAP - means PORT CMD capabilities mask. All of the
> >>>> possible flags have been set in that mask. There are no more
> >>>> capabilities in the PORT CMD register left undeclared. That's why the
> >>>> name is selected the way it is and why I haven't added any comment in
> >>>> here (what the kernel coding style says about the over-commenting the
> >>>> code).
> >>>
> >>
> >>> Yes, I understood from the name what it is. What I do NOT understand is
> >>> why all the feature bits are not there. Why this subset only ? A comment
> >>> about that would be nice so that the reason for it is not lost.
> >>
> >> Well, because it's indeed "PORT_CMD capabilities mask", and not features,
> >> not setups, not settings, not status flags, etc. As I said all the port
> >> Capabilities have been listed in that mask:
> >> PORT_CMD_FBSCP BIT(22) - FIS-based Switching Capable Port
> >> PORT_CMD_ESP BIT(21) - External SATA Port
> >> PORT_CMD_CPD BIT(20) - Cold Presence Detect
> >> PORT_CMD_MPSP BIT(19) - Mechanical Presence Switch Attached to Port
> >> PORT_CMD_HPCP BIT(18) - Hot Plug Capable Port
> >> I've or'ed-them-up in a single mask => PORT_CMD_CAP in order to work
> >> with them independently from the rest of the PORT_CMD CSR fields.
> >>
> >> Unlike the generic controller CAP/CAP2 registers, which consists of the
> >> device capabilities only, PORT_CMD contains various R/W settings (PM, LED
> >> driver, etc), RO status flags (CMD-list running, FIS recv running, etc)
> >> and amongst other the RO/Wo !port-specific capabilities!. The later ones
> >> indicate the platform-specific device features. Since the register
> >> contains flags with the intermixed nature, I need to have a mask to at
> >> least get the capabilities and preserve them between the device
> >> resets. That's why the PORT_CMD_CAP has been introduced in the
> >> framework of this patch. Its name was chosen with a reference to the
> >> CAP registers, see:
> >> HOST_CAP, HOST_CAP2, and finally my PORT_CMD_CAP.
> >>
> >>>
> >>>>
> >>>>>
> >>>>> Sure I can go and read the specs to figure it out. But again, a comment would
> >>>>> avoid readers of the code to have to decrypt all that.
> >>>>
> >>>> If you still insist on having an additional comment. I can add
> >>>> something like "/* PORT_CMD capabilities mask */". Are you ok with it?
> >>>
> >>
> >>> That does not help on its own. The macro name says that already. I would
> >>> like a note about why only these features are selected.
> >>
> >> Please see the explanation above. I don't see what else to say about
> >> that mask, because in short what I said above really means "PORT_CMD
> >> capabilities mask". So should you have some more clever text, which
> >> would be more suitable here, please tell me and I'll add it to the
> >> patch.
> >>
> >> Regarding what you said earlier. In order to fully understand the
> >> AHCI driver a hacker would always need to read the specs. There is
> >> just no way to do that effectively enough without the controller
> >> manual at hands. And the PORT_CMD capabilities isn't the most
> >> complicated part of the device.
> >>
> >> -Sergey
> >>
> >>>
> >>>>
> >>>> -Sergey
> >>>>
> >>>>>
> >>>>>>
> >>>>>> -Sergey
> >>>>>>
> >>>>>>>
> >>>>>>>> +
> >>>>>>>> /* PORT_FBS bits */
> >>>>>>>> PORT_FBS_DWE_OFFSET = 16, /* FBS device with error offset */
> >>>>>>>> PORT_FBS_ADO_OFFSET = 12, /* FBS active dev optimization offset */
> >>>>>>>
> >>>>>>>
> >>>>>>> --
> >>>>>>> Damien Le Moal
> >>>>>>> Western Digital Research
> >>>>>
> >>>>>
> >>>>> --
> >>>>> Damien Le Moal
> >>>>> Western Digital Research
> >>>
> >>>
> >>> --
> >>> Damien Le Moal
> >>> Western Digital Research
>
>
> --
> Damien Le Moal
> Western Digital Research