2022-03-28 22:17:00

by Serge Semin

[permalink] [raw]
Subject: [PATCH 00/21] ata: ahci: Add DWC/Baikal-T1 AHCI SATA support

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.

Traditionally the series starts with converting the legacy AHCI SATA
controllers text-based DT-bindings to the DT-schema. The bindings are
split up 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 before 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.

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 (21):
dt-bindings: ata: sata: Extend number of SATA ports
dt-bindings: ata: Convert AHCI-bindings to DT schema
ata: libahci_platform: Explicitly set rc on devres_alloc failure
ata: libahci_platform: Convert to using handy devm-ioremap methods
ata: libahci_platform: Convert to using devm bulk clocks API
ata: libahci_platform: Add function returning a clock-handle by id
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: 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 | 176 ++++++
.../devicetree/bindings/ata/ahci-platform.txt | 79 ---
.../bindings/ata/baikal,bt1-ahci.yaml | 132 +++++
.../devicetree/bindings/ata/generic-ahci.yaml | 98 ++++
.../devicetree/bindings/ata/sata-common.yaml | 7 +-
.../bindings/ata/snps,dwc-ahci.yaml | 121 ++++
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 | 525 ++++++++++++++++++
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 | 234 +++++---
include/linux/ahci_platform.h | 8 +-
18 files changed, 1321 insertions(+), 178 deletions(-)
create mode 100644 Documentation/devicetree/bindings/ata/ahci-common.yaml
delete mode 100644 Documentation/devicetree/bindings/ata/ahci-platform.txt
create mode 100644 Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml
create mode 100644 Documentation/devicetree/bindings/ata/generic-ahci.yaml
create mode 100644 Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
create mode 100644 drivers/ata/ahci_dwc.c

--
2.35.1


2022-03-28 22:20:06

by Serge Semin

[permalink] [raw]
Subject: [PATCH 21/21] MAINTAINERS: Add maintainers for DWC AHCI SATA driver

Add myself as a maintainer of the new DWC AHCI SATA driver and
its DT-bindings schema.

Signed-off-by: Serge Semin <[email protected]>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index cd0f68d4a34a..19c9ea0758cc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10919,6 +10919,15 @@ F: drivers/ata/ahci_platform.c
F: drivers/ata/libahci_platform.c
F: include/linux/ahci_platform.h

+LIBATA SATA AHCI SYNOPSYS DWC CONTROLLER DRIVER
+M: Serge Semin <[email protected]>
+L: [email protected]
+S: Maintained
+T: git git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
+F: Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml
+F: Documentation/devicetree/bindings/ata/snps,dwc-ahci.yaml
+F: drivers/ata/ahci_dwc.c
+
LIBATA SATA PROMISE TX2/TX4 CONTROLLER DRIVER
M: Mikael Pettersson <[email protected]>
L: [email protected]
--
2.35.1

2022-03-28 22:20:31

by Serge Semin

[permalink] [raw]
Subject: [PATCH 08/21] ata: libahci_platform: Parse ports-implemented property in resources getter

The ports-implemented property is mainly used on the OF-based platforms
with no ports mapping initialized by a bootloader/BIOS firmware. Seeing
the same of_property_read_u32()-based pattern has already been implemented
in the generic AHCI glue driver and in the Mediatek, St AHCI drivers let's
move the property read procedure to the generic
ahci_platform_get_resources() method. Thus we'll have the forced ports
mapping feature supported for each OF-based platform which requires that,
and stop re-implementing the same pattern in there a bit simplifying the
code.

Signed-off-by: Serge Semin <[email protected]>
---
drivers/ata/ahci_mtk.c | 2 --
drivers/ata/ahci_platform.c | 3 ---
drivers/ata/ahci_st.c | 3 ---
drivers/ata/libahci_platform.c | 3 +++
4 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/ahci_mtk.c b/drivers/ata/ahci_mtk.c
index d9b08ae7c3b2..8f8539952af1 100644
--- a/drivers/ata/ahci_mtk.c
+++ b/drivers/ata/ahci_mtk.c
@@ -118,8 +118,6 @@ static int mtk_ahci_parse_property(struct ahci_host_priv *hpriv,
SYS_CFG_SATA_EN);
}

- of_property_read_u32(np, "ports-implemented", &hpriv->force_port_map);
-
return 0;
}

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 3aab2e3d57f3..24c25f076f37 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -56,9 +56,6 @@ static int ahci_probe(struct platform_device *pdev)
if (rc)
return rc;

- of_property_read_u32(dev->of_node,
- "ports-implemented", &hpriv->force_port_map);
-
if (of_device_is_compatible(dev->of_node, "hisilicon,hisi-ahci"))
hpriv->flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ;

diff --git a/drivers/ata/ahci_st.c b/drivers/ata/ahci_st.c
index c268264c2129..e010f2fd1fa2 100644
--- a/drivers/ata/ahci_st.c
+++ b/drivers/ata/ahci_st.c
@@ -168,9 +168,6 @@ static int st_ahci_probe(struct platform_device *pdev)

st_ahci_configure_oob(hpriv->mmio);

- of_property_read_u32(dev->of_node,
- "ports-implemented", &hpriv->force_port_map);
-
err = ahci_platform_init_host(pdev, hpriv, &st_ahci_port_info,
&ahci_platform_sht);
if (err) {
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 845042295b97..5998e735a813 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -501,6 +501,9 @@ 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->force_port_map);
+
if (child_nodes) {
for_each_child_of_node(dev->of_node, child) {
u32 port;
--
2.35.1

2022-03-28 22:26:24

by Serge Semin

[permalink] [raw]
Subject: [PATCH 05/21] ata: libahci_platform: Convert to using devm bulk clocks API

In order to simplify the clock-related code there is a way to convert the
current fixed clocks array into using the common bulk clocks kernel API
with dynamic set of the clock handlers and device-managed clock-resource
tracking. It's a bit tricky due to the complication coming from the
requirement to support the platforms (da850, spear13xx) with the
non-OF-based clock source, but still doable.

Before this modification there are two methods have been used to get the
clocks connected to an AHCI device: clk_get() - to get the very first
clock in the list and of_clk_get() - to get the rest of them. Basically
the platforms with non-OF-based clocks definition could specify only a
single reference clock source. The platforms with OF-hw clocks have been
luckier and could setup up to AHCI_MAX_CLKS clocks. Such semantic can be
retained with using devm_clk_bulk_get_all() to retrieve the clocks defined
via the DT firmware and devm_clk_get_optional() otherwise. In both cases
using the device-managed version of the methods will cause the automatic
resources deallocation on the AHCI device removal event. The only
complicated part in the suggested approach is the explicit allocation and
initialization of the clk_bulk_data structure instance for the non-OF
reference clocks. It's required in order to use the Bulk Clocks API for
the both denoted cases of the clocks definition.

Note aside with the clock-related code reduction and natural
simplification, there are several bonuses the suggested modification
provides. First of all the limitation of having no greater than
AHCI_MAX_CLKS clocks is now removed, since the devm_clk_bulk_get_all()
method will allocate as many reference clocks data descriptors as there
are clocks specified for the device. Secondly the clock names are
auto-detected. So the glue drivers can make sure that the required clocks
are specified just by checking the clock IDs in the clk_bulk_data array.
Thirdly using the handy Bulk Clocks kernel API improves the
clocks-handling code readability. And the last but not least this
modification implements a true optional clocks support to the
ahci_platform_get_resources() method. Indeed the previous clocks getting
procedure just stopped getting the clocks on any errors (aside from
non-critical -EPROBE_DEFER) in a way so the callee wasn't even informed
about abnormal loop termination. The new implementation lacks of such
problem. The ahci_platform_get_resources() will return an error code if
the corresponding clocks getting method ends execution abnormally.

Signed-off-by: Serge Semin <[email protected]>
---
drivers/ata/ahci.h | 4 +-
drivers/ata/libahci_platform.c | 82 +++++++++++++++-------------------
2 files changed, 37 insertions(+), 49 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index eeac5482f1d1..1564c691094a 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -38,7 +38,6 @@

enum {
AHCI_MAX_PORTS = 32,
- AHCI_MAX_CLKS = 5,
AHCI_MAX_SG = 168, /* hardware max is 64K */
AHCI_DMA_BOUNDARY = 0xffffffff,
AHCI_MAX_CMDS = 32,
@@ -341,7 +340,8 @@ struct ahci_host_priv {
u32 em_msg_type; /* EM message type */
u32 remapped_nvme; /* NVMe remapped device count */
bool got_runtime_pm; /* Did we do pm_runtime_get? */
- struct clk *clks[AHCI_MAX_CLKS]; /* Optional */
+ unsigned int n_clks;
+ struct clk_bulk_data *clks; /* Optional */
struct reset_control *rsts; /* Optional */
struct regulator **target_pwrs; /* Optional */
struct regulator *ahci_regulator;/* Optional */
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 8eabbb5f208c..d805ddc3a024 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -8,6 +8,7 @@
* Anton Vorontsov <[email protected]>
*/

+#include <linux/clk-provider.h>
#include <linux/clk.h>
#include <linux/kernel.h>
#include <linux/gfp.h>
@@ -97,28 +98,14 @@ EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);
* ahci_platform_enable_clks - Enable platform clocks
* @hpriv: host private area to store config values
*
- * This function enables all the clks found in hpriv->clks, starting at
- * index 0. If any clk fails to enable it disables all the clks already
- * enabled in reverse order, and then returns an error.
+ * This function enables all the clks found for the AHCI device.
*
* RETURNS:
* 0 on success otherwise a negative error code
*/
int ahci_platform_enable_clks(struct ahci_host_priv *hpriv)
{
- int c, rc;
-
- for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {
- rc = clk_prepare_enable(hpriv->clks[c]);
- if (rc)
- goto disable_unprepare_clk;
- }
- return 0;
-
-disable_unprepare_clk:
- while (--c >= 0)
- clk_disable_unprepare(hpriv->clks[c]);
- return rc;
+ return clk_bulk_prepare_enable(hpriv->n_clks, hpriv->clks);
}
EXPORT_SYMBOL_GPL(ahci_platform_enable_clks);

@@ -126,16 +113,13 @@ EXPORT_SYMBOL_GPL(ahci_platform_enable_clks);
* ahci_platform_disable_clks - Disable platform clocks
* @hpriv: host private area to store config values
*
- * This function disables all the clks found in hpriv->clks, in reverse
- * order of ahci_platform_enable_clks (starting at the end of the array).
+ * This function disables all the clocks enabled before
+ * (bulk-clocks-disable function is supposed to do that in reverse
+ * from the enabling procedure order).
*/
void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)
{
- int c;
-
- for (c = AHCI_MAX_CLKS - 1; c >= 0; c--)
- if (hpriv->clks[c])
- clk_disable_unprepare(hpriv->clks[c]);
+ clk_bulk_disable_unprepare(hpriv->n_clks, hpriv->clks);
}
EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);

@@ -292,8 +276,6 @@ static void ahci_platform_put_resources(struct device *dev, void *res)
pm_runtime_disable(dev);
}

- for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)
- clk_put(hpriv->clks[c]);
/*
* The regulators are tied to child node device and not to the
* SATA device itself. So we can't use devm for automatically
@@ -374,8 +356,8 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
* 1) mmio registers (IORESOURCE_MEM 0, mandatory)
* 2) regulator for controlling the targets power (optional)
* regulator for controlling the AHCI controller (optional)
- * 3) 0 - AHCI_MAX_CLKS clocks, as specified in the devs devicetree node,
- * or for non devicetree enabled platforms a single clock
+ * 3) all clocks specified in the devicetree node, or a single
+ * clock for non-OF platforms (optional)
* 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
* 5) phys (optional)
*
@@ -385,11 +367,10 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
unsigned int flags)
{
+ int enabled_ports = 0, rc = 0, child_nodes;
struct device *dev = &pdev->dev;
struct ahci_host_priv *hpriv;
- struct clk *clk;
struct device_node *child;
- int i, enabled_ports = 0, rc = 0, child_nodes;
u32 mask_port_map = 0;

if (!devres_open_group(dev, NULL, GFP_KERNEL))
@@ -413,25 +394,32 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
}
}

- for (i = 0; i < AHCI_MAX_CLKS; i++) {
- /*
- * For now we must use clk_get(dev, NULL) for the first clock,
- * because some platforms (da850, spear13xx) are not yet
- * converted to use devicetree for clocks. For new platforms
- * this is equivalent to of_clk_get(dev->of_node, 0).
- */
- if (i == 0)
- clk = clk_get(dev, NULL);
- else
- clk = of_clk_get(dev->of_node, i);
-
- if (IS_ERR(clk)) {
- rc = PTR_ERR(clk);
- if (rc == -EPROBE_DEFER)
- goto err_out;
- break;
+ /*
+ * Bulk clock get procedure can fail to find any clock due to running
+ * an a non-OF platform or due to the clocks being defined in bypass
+ * from the DT firmware (like da850, spear13xx). In that case we
+ * fallback to getting a single clock source right from the dev clocks
+ * list.
+ */
+ rc = devm_clk_bulk_get_all(dev, &hpriv->clks);
+ if (rc > 0) {
+ hpriv->n_clks = rc;
+ } else if (!rc) {
+ hpriv->clks = devm_kzalloc(dev, sizeof(*hpriv->clks), GFP_KERNEL);
+ if (!hpriv->clks) {
+ rc = -ENOMEM;
+ goto err_out;
}
- hpriv->clks[i] = clk;
+ hpriv->clks->clk = devm_clk_get_optional(dev, NULL);
+ if (IS_ERR(hpriv->clks->clk)) {
+ rc = PTR_ERR(hpriv->clks->clk);
+ goto err_out;
+ } else if (hpriv->clks->clk) {
+ hpriv->clks->id = __clk_get_name(hpriv->clks->clk);
+ hpriv->n_clks = 1;
+ }
+ } else {
+ goto err_out;
}

hpriv->ahci_regulator = devm_regulator_get(dev, "ahci");
--
2.35.1

2022-03-28 22:28:22

by Serge Semin

[permalink] [raw]
Subject: [PATCH 18/21] dt-bindings: ata: ahci: Add Baikal-T1 AHCI SATA controller DT schema

Baikal-T1 AHCI controller is based on the DWC AHCI SATA IP-core v4.10a
with the next specific settings: two SATA ports, cascaded CSR access based
on two clock domains (APB and AXI), selectable source of the reference
clock (though stable work is currently available from the external source
only), two reset lanes for the application and SATA ports domains. Other
than that the device is fully compatible with the generic DWC AHCI SATA
bindings.

Signed-off-by: Serge Semin <[email protected]>
---
.../bindings/ata/baikal,bt1-ahci.yaml | 132 ++++++++++++++++++
1 file changed, 132 insertions(+)
create mode 100644 Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml

diff --git a/Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml b/Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml
new file mode 100644
index 000000000000..960d88d97926
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/baikal,bt1-ahci.yaml
@@ -0,0 +1,132 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/ata/baikal,bt1-ahci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Baikal-T1 SoC AHCI SATA controller
+
+maintainers:
+ - Serge Semin <[email protected]>
+
+description: |
+ AHCI SATA controller embedded into the Baikal-T1 SoC is based on the
+ DWC AHCI SATA v4.10a IP-core.
+
+allOf:
+ - $ref: snps,dwc-ahci.yaml#
+
+properties:
+ compatible:
+ contains:
+ const: baikal,bt1-ahci
+
+ clocks:
+ items:
+ - description: Peripheral APB bus clock source
+ - description: Application AXI BIU clock
+ - description: Internal SATA Ports reference clock
+ - description: External SATA Ports reference clock
+
+ clock-names:
+ items:
+ - const: pclk
+ - const: aclk
+ - const: ref_int
+ - const: ref_ext
+
+ resets:
+ items:
+ - description: Application AXI BIU domain reset
+ - description: SATA Ports clock domain reset
+
+ reset-names:
+ items:
+ - const: arst
+ - const: ref
+
+ syscon:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description:
+ Phandle reference to the CCU system controller. It is required to
+ switch between internal and external SATA reference clock sources.
+
+ ports-implemented:
+ maximum: 0x3
+
+patternProperties:
+ "^[email protected][0-9a-e]$":
+ type: object
+
+ properties:
+ reg:
+ minimum: 0
+ maximum: 1
+
+ snps,tx-ts-max:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Due to having AXI3 bus interface utilized the maximum Tx DMA
+ transaction size can't exceed 16 beats (AxLEN[3:0]).
+ minimum: 1
+ maximum: 16
+
+ snps,rx-ts-max:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ Due to having AXI3 bus interface utilized the maximum Rx DMA
+ transaction size can't exceed 16 beats (AxLEN[3:0]).
+ minimum: 1
+ maximum: 16
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+ - resets
+ - syscon
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/mips-gic.h>
+ #include <dt-bindings/clock/bt1-ccu.h>
+ #include <dt-bindings/reset/bt1-ccu.h>
+
+ [email protected] {
+ compatible = "baikal,bt1-ahci", "snps,dwc-ahci";
+ reg = <0x1f050000 0x2000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ interrupts = <GIC_SHARED 64 IRQ_TYPE_LEVEL_HIGH>;
+
+ clocks = <&ccu_sys CCU_SYS_APB_CLK>, <&ccu_axi CCU_AXI_SATA_CLK>,
+ <&ccu_sys CCU_SYS_SATA_REF_CLK>, <&clk_sata>;
+ clock-names = "pclk", "aclk", "ref_int", "ref_ext";
+
+ resets = <&ccu_axi CCU_AXI_SATA_RST>, <&ccu_sys CCU_SYS_SATA_REF_RST>;
+ reset-names = "arst", "ref";
+
+ syscon = <&syscon>;
+
+ ports-implemented = <0x3>;
+
+ [email protected] {
+ reg = <0>;
+
+ snps,tx-ts-max = <4>;
+ snps,rx-ts-max = <4>;
+ };
+
+ [email protected] {
+ reg = <1>;
+
+ snps,tx-ts-max = <4>;
+ snps,rx-ts-max = <4>;
+ };
+ };
+...
--
2.35.1

2022-03-28 22:34:04

by Serge Semin

[permalink] [raw]
Subject: [PATCH 10/21] dt-bindings: ata: ahci: Add platform capability properties

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 can define a set AHCI DT-node properties to describe
all of HW-init capabilities flags. 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 whole HBA by
means of the "hba-sss" and "hba-smps" properties. Each port can have the
"hba-{hpcp,mpsp,cpd,esp,fbscp}" defined indicatating that the port
supports the next functionality: 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]>

---

Alternatively we could define them as a bitfield, but having a set of
boolean properties seemed a better idea since in that case we can
implement a simple inter-dependency rules for them, which can't be done
should we take the bitfields path.
---
.../devicetree/bindings/ata/ahci-common.yaml | 66 +++++++++++++++++++
.../devicetree/bindings/ata/generic-ahci.yaml | 9 +++
2 files changed, 75 insertions(+)

diff --git a/Documentation/devicetree/bindings/ata/ahci-common.yaml b/Documentation/devicetree/bindings/ata/ahci-common.yaml
index 054819930538..1901c55a5468 100644
--- a/Documentation/devicetree/bindings/ata/ahci-common.yaml
+++ b/Documentation/devicetree/bindings/ata/ahci-common.yaml
@@ -67,6 +67,19 @@ properties:
phy-names:
const: sata-phy

+ hba-sss:
+ type: boolean
+ description:
+ Staggered Spin-up Support. Indicates whether the HBA supports the
+ staggered spin-up on its ports, for use in balancing power spikes.
+
+ hba-smps:
+ type: boolean
+ description:
+ Mechanical Presence Switch Support. Indicates whether the HBA supports
+ mechanical presence switches on its ports for use in hot plug
+ operations.
+
ports-implemented:
$ref: /schemas/types.yaml#/definitions/uint32
description:
@@ -88,6 +101,40 @@ patternProperties:
minimum: 0
maximum: 31

+ hba-hpcp:
+ type: boolean
+ description:
+ Hot Plug Capable Port. Indicates that this port’s signal and power
+ connectors are externally accessible via a joint signal and power
+ connector for blindmate device hot plug. It is mutually exclusive
+ with the ESP feature.
+
+ hba-mpsp:
+ type: boolean
+ description:
+ Mechanical Presence Switch Attached to Port. Indicates whether
+ the platform an mechanical presence switch attached to this
+ port.
+
+ hba-cpd:
+ type: boolean
+ description:
+ Cold Presence Detection. Indicates whether the platform supports
+ cold presence detection on this port.
+
+ hba-esp:
+ type: boolean
+ description:
+ External SATA Port. Indicates that this port’s signal connector
+ is externally accessible on a signal only connector (e.g. eSATA
+ connector).
+
+ hba-fbscp:
+ type: boolean
+ description:
+ FIS-based Switching Capable Port. Indicates whether this port
+ supports Port Multiplier FIS-based switching.
+
phys:
description: Individual AHCI SATA port PHY
maxItems: 1
@@ -101,6 +148,25 @@ patternProperties:
required:
- reg

+ # eSATA can't be enabled together with the HotPlug capability
+ oneOf:
+ - required:
+ - hba-hpcp
+ - required:
+ - hba-esp
+ - not:
+ anyOf:
+ - required:
+ - hba-hpcp
+ - required:
+ - hba-esp
+
+ # HotPlug capability must be enabled together with Cold Plug
+ # Detection and Mechanical Presence Switching.
+ dependencies:
+ hba-cpd: ["hba-hpcp"]
+ hba-mpsp: ["hba-hpcp"]
+
required:
- reg
- interrupts
diff --git a/Documentation/devicetree/bindings/ata/generic-ahci.yaml b/Documentation/devicetree/bindings/ata/generic-ahci.yaml
index 957f45c4f488..6c147350b5f9 100644
--- a/Documentation/devicetree/bindings/ata/generic-ahci.yaml
+++ b/Documentation/devicetree/bindings/ata/generic-ahci.yaml
@@ -74,14 +74,23 @@ examples:

interrupts = <GIC_SPI 438 IRQ_TYPE_LEVEL_HIGH>;

+ hba-smps;
+
[email protected] {
reg = <0>;
+
+ hba-fbscp;
+ hba-esp;
phys = <&sata_phy0>;
phy-names = "sata-phy";
};

[email protected] {
reg = <1>;
+
+ hba-fbscp;
+ hba-hpcp;
+ hba-mpsp;
phys = <&sata_phy1>;
phy-names = "sata-phy";
};
--
2.35.1

2022-03-28 22:39:16

by Serge Semin

[permalink] [raw]
Subject: [PATCH 06/21] ata: libahci_platform: Add function returning a clock-handle by id

Since all the clocks are retrieved by the method
ahci_platform_get_resources() there is no need for the 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.

Signed-off-by: Serge Semin <[email protected]>
---
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 d805ddc3a024..4fb9629c03ab 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -94,6 +94,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 point to the clock descriptor of the clock with
+ * 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 49e5383d4222..fd964e6a68d6 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_enable_regulators(struct ahci_host_priv *hpriv);
--
2.35.1

2022-03-28 22:41:08

by Serge Semin

[permalink] [raw]
Subject: [PATCH 12/21] ata: libahci: Discard redundant force_port_map parameter

Currently there are four port-map-related fields declared in the
ahci_host_priv structure and used to setup the HBA ports mapping. First
the ports-mapping is read from the PI register and immediately stored in
the saved_port_map field. If forced_port_map is initialized with non-zero
value then its value will have greater priority over the value read from
PI, thus it will override the saved_port_map field. That value will be then
masked by a non-zero mask_port_map field and after some sanity checks it
will be stored in the ahci_host_priv.port_map field as a final port
mapping.

As you can see the logic is a bit too complicated for such a simple task.
We can freely get rid from at least one of the fields with no change to
the implemented semantic. The force_port_map field can be replaced with
taking non-zero saved_port_map value into account. So if saved_port_map is
pre-initialized by the glue-driver/platform-specific code then it will
have greater priority over the value read from PI register and will be
used as actual HBA ports mapping later on. Thus the ports map forcing task
will be just transferred from the force_port_map to saved_port_map field.

This modification will perfectly fit into the feature of having OF-based
initialization of the HW-init HBA CSR fields we are about to introduce in
the next commit.

Signed-off-by: Serge Semin <[email protected]>
---
drivers/ata/ahci.c | 2 +-
drivers/ata/ahci.h | 1 -
drivers/ata/libahci.c | 10 ++++++----
drivers/ata/libahci_platform.c | 2 +-
4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index ab5811ef5a53..8ce0d166cc8d 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -654,7 +654,7 @@ static void ahci_pci_save_initial_config(struct pci_dev *pdev,
{
if (pdev->vendor == PCI_VENDOR_ID_JMICRON && pdev->device == 0x2361) {
dev_info(&pdev->dev, "JMB361 has only one port\n");
- hpriv->force_port_map = 1;
+ hpriv->saved_port_map = 1;
}

/*
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 04690b4168a3..519d148ecaea 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -329,7 +329,6 @@ struct ahci_port_priv {
struct ahci_host_priv {
/* Input fields */
unsigned int flags; /* AHCI_HFLAG_* */
- u32 force_port_map; /* force port map */
u32 mask_port_map; /* mask out particular bits */

void __iomem * mmio; /* bus-independent mem map */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 0ed484e04fd6..011175e82174 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -453,7 +453,6 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
* reset. Values without are used for driver operation.
*/
hpriv->saved_cap = cap = readl(mmio + HOST_CAP);
- hpriv->saved_port_map = port_map = readl(mmio + HOST_PORTS_IMPL);

/* CAP2 register is only defined for AHCI 1.2 and later */
vers = readl(mmio + HOST_VERSION);
@@ -517,10 +516,13 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
cap &= ~HOST_CAP_SXS;
}

- if (hpriv->force_port_map && port_map != hpriv->force_port_map) {
+ /* 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",
- port_map, hpriv->force_port_map);
- port_map = hpriv->force_port_map;
+ port_map, hpriv->saved_port_map);
+ port_map = hpriv->saved_port_map;
+ } else {
hpriv->saved_port_map = port_map;
}

diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index febad33aa43c..5cbc2c42164d 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -539,7 +539,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
}

of_property_read_u32(dev->of_node,
- "ports-implemented", &hpriv->force_port_map);
+ "ports-implemented", &hpriv->saved_port_map);

if (child_nodes) {
for_each_child_of_node(dev->of_node, child) {
--
2.35.1

2022-03-28 22:42:04

by Serge Semin

[permalink] [raw]
Subject: [PATCH 01/21] dt-bindings: ata: sata: Extend number of SATA ports

The denoted in the description upper limit only concerns the Port
Multipliers, but not the actual SATA ports. It's an external device
attached to a SATA port in order to access more than one SATA-drive. So
when it's attached to a SATA port it just extends the port capability
while the number of actual SATA ports stays the same. For instance on AHCI
controllers the number of actual ports is determined by the CAP.NP field
and the PI (Ports Implemented) register. AFAICS in general the maximum
number of SATA ports depends on the particular controller implementation.
Generic AHCI controller can't have more than 32 ports (since CAP.NP is of
5 bits wide and PI register is 32-bits size), while DWC AHCI SATA
controller can't be configured with more than 8 ports activated. So let's
discard the SATA ports reg-property restrictions and just make sure that
it consists of a single reg-item.

Signed-off-by: Serge Semin <[email protected]>
---
Documentation/devicetree/bindings/ata/sata-common.yaml | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/sata-common.yaml b/Documentation/devicetree/bindings/ata/sata-common.yaml
index 7ac77b1c5850..c619f0ae72fb 100644
--- a/Documentation/devicetree/bindings/ata/sata-common.yaml
+++ b/Documentation/devicetree/bindings/ata/sata-common.yaml
@@ -41,11 +41,10 @@ patternProperties:
properties:
reg:
minimum: 0
- maximum: 14
description:
- The ID number of the drive port SATA can potentially use a port
- multiplier making it possible to connect up to 15 disks to a single
- SATA port.
+ The ID number of the SATA port. Aside with being directly used
+ each port can have a Port Multiplier attached thus allowing to
+ access more than one drive by means of a single channel.

additionalProperties: true

--
2.35.1

2022-03-28 23:03:07

by Serge Semin

[permalink] [raw]
Subject: [PATCH 11/21] ata: libahci: Extend port-cmd flags set with port capabilities

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_DEV_MPS 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.7

Signed-off-by: Serge Semin <[email protected]>
---
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 0b1d5c24cb8c..04690b4168a3 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

2022-03-28 23:20:34

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 05/21] ata: libahci_platform: Convert to using devm bulk clocks API

Hi Serge,

I love your patch! Yet something to improve:

[auto build test ERROR on v5.17]
[also build test ERROR on next-20220328]
[cannot apply to axboe-block/for-next robh/for-next linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/intel-lab-lkp/linux/commits/Serge-Semin/ata-ahci-Add-DWC-Baikal-T1-AHCI-SATA-support/20220328-234809
base: f443e374ae131c168a065ea1748feac6b2e76613
config: arm-buildonly-randconfig-r005-20220327 (https://download.01.org/0day-ci/archive/20220329/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0f6d9501cf49ce02937099350d08f20c4af86f3d)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/28cf1dcfb31bfca35af403a8774d0d880923fab3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Serge-Semin/ata-ahci-Add-DWC-Baikal-T1-AHCI-SATA-support/20220328-234809
git checkout 28cf1dcfb31bfca35af403a8774d0d880923fab3
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/ata/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> drivers/ata/ahci_dm816.c:72:6: error: invalid argument type 'struct clk_bulk_data' to unary expression
if (!hpriv->clks[1]) {
^~~~~~~~~~~~~~~
>> drivers/ata/ahci_dm816.c:77:29: error: passing 'struct clk_bulk_data' to parameter of incompatible type 'struct clk *'
refclk_rate = clk_get_rate(hpriv->clks[1]);
^~~~~~~~~~~~~~
include/linux/clk.h:584:40: note: passing argument to parameter 'clk' here
unsigned long clk_get_rate(struct clk *clk);
^
2 errors generated.


vim +72 drivers/ata/ahci_dm816.c

df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 60
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 61 static int ahci_dm816_phy_init(struct ahci_host_priv *hpriv, struct device *dev)
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 62 {
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 63 unsigned long refclk_rate;
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 64 int mpy;
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 65 u32 val;
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 66
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 67 /*
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 68 * We should have been supplied two clocks: the functional and
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 69 * keep-alive clock and the external reference clock. We need the
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 70 * rate of the latter to calculate the correct value of MPY bits.
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 71 */
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 @72 if (!hpriv->clks[1]) {
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 73 dev_err(dev, "reference clock not supplied\n");
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 74 return -EINVAL;
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 75 }
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 76
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 @77 refclk_rate = clk_get_rate(hpriv->clks[1]);
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 78 if ((refclk_rate % 100) != 0) {
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 79 dev_err(dev, "reference clock rate must be divisible by 100\n");
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 80 return -EINVAL;
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 81 }
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 82
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 83 mpy = ahci_dm816_get_mpy_bits(refclk_rate);
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 84 if (mpy < 0) {
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 85 dev_err(dev, "can't calculate the MPY bits value\n");
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 86 return -EINVAL;
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 87 }
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 88
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 89 /* Enable the PHY and configure the first HBA port. */
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 90 val = AHCI_DM816_PHY_MPY(mpy) | AHCI_DM816_PHY_LOS(1) |
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 91 AHCI_DM816_PHY_RXCDR(4) | AHCI_DM816_PHY_RXEQ(1) |
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 92 AHCI_DM816_PHY_TXSWING(3) | AHCI_DM816_PHY_ENPLL(1);
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 93 writel(val, hpriv->mmio + AHCI_DM816_P0PHYCR_REG);
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 94
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 95 /* Configure the second HBA port. */
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 96 val = AHCI_DM816_PHY_LOS(1) | AHCI_DM816_PHY_RXCDR(4) |
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 97 AHCI_DM816_PHY_RXEQ(1) | AHCI_DM816_PHY_TXSWING(3);
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 98 writel(val, hpriv->mmio + AHCI_DM816_P1PHYCR_REG);
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 99
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 100 return 0;
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 101 }
df46e6a4c06c89 Bartosz Golaszewski 2017-03-14 102

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-03-29 00:13:09

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 05/21] ata: libahci_platform: Convert to using devm bulk clocks API

Hi Serge,

I love your patch! Yet something to improve:

[auto build test ERROR on v5.17]
[also build test ERROR on next-20220328]
[cannot apply to axboe-block/for-next robh/for-next linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/intel-lab-lkp/linux/commits/Serge-Semin/ata-ahci-Add-DWC-Baikal-T1-AHCI-SATA-support/20220328-234809
base: f443e374ae131c168a065ea1748feac6b2e76613
config: alpha-randconfig-r034-20220327 (https://download.01.org/0day-ci/archive/20220329/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/28cf1dcfb31bfca35af403a8774d0d880923fab3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Serge-Semin/ata-ahci-Add-DWC-Baikal-T1-AHCI-SATA-support/20220328-234809
git checkout 28cf1dcfb31bfca35af403a8774d0d880923fab3
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash drivers/ata/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

drivers/ata/ahci_da850.c: In function 'ahci_da850_probe':
>> drivers/ata/ahci_da850.c:181:13: error: wrong type argument to unary exclamation mark
181 | if (!hpriv->clks[0]) {
| ^
>> drivers/ata/ahci_da850.c:186:34: error: incompatible types when assigning to type 'struct clk_bulk_data' from type 'struct clk *'
186 | hpriv->clks[0] = clk;
| ^~~
drivers/ata/ahci_da850.c:194:13: error: wrong type argument to unary exclamation mark
194 | if (!hpriv->clks[1]) {
| ^
drivers/ata/ahci_da850.c:201:34: error: incompatible types when assigning to type 'struct clk_bulk_data' from type 'struct clk *'
201 | hpriv->clks[1] = clk;
| ^~~
>> drivers/ata/ahci_da850.c:204:64: error: incompatible type for argument 1 of 'clk_get_rate'
204 | mpy = ahci_da850_calculate_mpy(clk_get_rate(hpriv->clks[1]));
| ~~~~~~~~~~~^~~
| |
| struct clk_bulk_data
In file included from drivers/ata/ahci.h:23,
from drivers/ata/ahci_da850.c:13:
include/linux/clk.h:880:54: note: expected 'struct clk *' but argument is of type 'struct clk_bulk_data'
880 | static inline unsigned long clk_get_rate(struct clk *clk)
| ~~~~~~~~~~~~^~~
--
drivers/ata/ahci_dm816.c: In function 'ahci_dm816_phy_init':
>> drivers/ata/ahci_dm816.c:72:13: error: wrong type argument to unary exclamation mark
72 | if (!hpriv->clks[1]) {
| ^
>> drivers/ata/ahci_dm816.c:77:47: error: incompatible type for argument 1 of 'clk_get_rate'
77 | refclk_rate = clk_get_rate(hpriv->clks[1]);
| ~~~~~~~~~~~^~~
| |
| struct clk_bulk_data
In file included from drivers/ata/ahci.h:23,
from drivers/ata/ahci_dm816.c:16:
include/linux/clk.h:880:54: note: expected 'struct clk *' but argument is of type 'struct clk_bulk_data'
880 | static inline unsigned long clk_get_rate(struct clk *clk)
| ~~~~~~~~~~~~^~~


vim +186 drivers/ata/ahci_da850.c

018d5ef2048fca Akinobu Mita 2015-01-29 159
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 160 static int ahci_da850_probe(struct platform_device *pdev)
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 161 {
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 162 struct device *dev = &pdev->dev;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 163 struct ahci_host_priv *hpriv;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 164 void __iomem *pwrdn_reg;
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 165 struct resource *res;
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 166 struct clk *clk;
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 167 u32 mpy;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 168 int rc;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 169
16af2d65842d34 Kunihiko Hayashi 2018-08-22 170 hpriv = ahci_platform_get_resources(pdev, 0);
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 171 if (IS_ERR(hpriv))
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 172 return PTR_ERR(hpriv);
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 173
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 174 /*
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 175 * Internally ahci_platform_get_resources() calls clk_get(dev, NULL)
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 176 * when trying to obtain the functional clock. This SATA controller
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 177 * uses two clocks for which we specify two connection ids. If we don't
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 178 * have the functional clock at this point - call clk_get() again with
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 179 * con_id = "fck".
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 180 */
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 @181 if (!hpriv->clks[0]) {
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 182 clk = clk_get(dev, "fck");
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 183 if (IS_ERR(clk))
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 184 return PTR_ERR(clk);
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 185
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 @186 hpriv->clks[0] = clk;
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 187 }
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 188
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 189 /*
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 190 * The second clock used by ahci-da850 is the external REFCLK. If we
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 191 * didn't get it from ahci_platform_get_resources(), let's try to
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 192 * specify the con_id in clk_get().
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 193 */
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 194 if (!hpriv->clks[1]) {
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 195 clk = clk_get(dev, "refclk");
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 196 if (IS_ERR(clk)) {
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 197 dev_err(dev, "unable to obtain the reference clock");
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 198 return -ENODEV;
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 199 }
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 200
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 201 hpriv->clks[1] = clk;
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 202 }
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 203
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 @204 mpy = ahci_da850_calculate_mpy(clk_get_rate(hpriv->clks[1]));
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 205 if (mpy == 0) {
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 206 dev_err(dev, "invalid REFCLK multiplier value: 0x%x", mpy);
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 207 return -EINVAL;
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 208 }
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 209
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 210 rc = ahci_platform_enable_resources(hpriv);
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 211 if (rc)
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 212 return rc;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 213
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 214 res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
c88c094985ad38 Christophe JAILLET 2017-08-16 215 if (!res) {
c88c094985ad38 Christophe JAILLET 2017-08-16 216 rc = -ENODEV;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 217 goto disable_resources;
c88c094985ad38 Christophe JAILLET 2017-08-16 218 }
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 219
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 220 pwrdn_reg = devm_ioremap(dev, res->start, resource_size(res));
c88c094985ad38 Christophe JAILLET 2017-08-16 221 if (!pwrdn_reg) {
c88c094985ad38 Christophe JAILLET 2017-08-16 222 rc = -ENOMEM;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 223 goto disable_resources;
c88c094985ad38 Christophe JAILLET 2017-08-16 224 }
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 225
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 226 da850_sata_init(dev, pwrdn_reg, hpriv->mmio, mpy);
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 227
018d5ef2048fca Akinobu Mita 2015-01-29 228 rc = ahci_platform_init_host(pdev, hpriv, &ahci_da850_port_info,
018d5ef2048fca Akinobu Mita 2015-01-29 229 &ahci_platform_sht);
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 230 if (rc)
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 231 goto disable_resources;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 232
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 233 return 0;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 234 disable_resources:
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 235 ahci_platform_disable_resources(hpriv);
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 236 return rc;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 237 }
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 238

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-03-29 00:29:51

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 05/21] ata: libahci_platform: Convert to using devm bulk clocks API

Hi Serge,

I love your patch! Yet something to improve:

[auto build test ERROR on v5.17]
[also build test ERROR on next-20220328]
[cannot apply to axboe-block/for-next robh/for-next linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/intel-lab-lkp/linux/commits/Serge-Semin/ata-ahci-Add-DWC-Baikal-T1-AHCI-SATA-support/20220328-234809
base: f443e374ae131c168a065ea1748feac6b2e76613
config: arm-randconfig-r015-20220327 (https://download.01.org/0day-ci/archive/20220329/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0f6d9501cf49ce02937099350d08f20c4af86f3d)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/28cf1dcfb31bfca35af403a8774d0d880923fab3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Serge-Semin/ata-ahci-Add-DWC-Baikal-T1-AHCI-SATA-support/20220328-234809
git checkout 28cf1dcfb31bfca35af403a8774d0d880923fab3
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/ata/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> drivers/ata/ahci_da850.c:181:6: error: invalid argument type 'struct clk_bulk_data' to unary expression
if (!hpriv->clks[0]) {
^~~~~~~~~~~~~~~
>> drivers/ata/ahci_da850.c:186:18: error: assigning to 'struct clk_bulk_data' from incompatible type 'struct clk *'
hpriv->clks[0] = clk;
^ ~~~
drivers/ata/ahci_da850.c:194:6: error: invalid argument type 'struct clk_bulk_data' to unary expression
if (!hpriv->clks[1]) {
^~~~~~~~~~~~~~~
drivers/ata/ahci_da850.c:201:18: error: assigning to 'struct clk_bulk_data' from incompatible type 'struct clk *'
hpriv->clks[1] = clk;
^ ~~~
>> drivers/ata/ahci_da850.c:204:46: error: passing 'struct clk_bulk_data' to parameter of incompatible type 'struct clk *'
mpy = ahci_da850_calculate_mpy(clk_get_rate(hpriv->clks[1]));
^~~~~~~~~~~~~~
include/linux/clk.h:880:54: note: passing argument to parameter 'clk' here
static inline unsigned long clk_get_rate(struct clk *clk)
^
5 errors generated.


vim +181 drivers/ata/ahci_da850.c

018d5ef2048fca Akinobu Mita 2015-01-29 159
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 160 static int ahci_da850_probe(struct platform_device *pdev)
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 161 {
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 162 struct device *dev = &pdev->dev;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 163 struct ahci_host_priv *hpriv;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 164 void __iomem *pwrdn_reg;
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 165 struct resource *res;
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 166 struct clk *clk;
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 167 u32 mpy;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 168 int rc;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 169
16af2d65842d34 Kunihiko Hayashi 2018-08-22 170 hpriv = ahci_platform_get_resources(pdev, 0);
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 171 if (IS_ERR(hpriv))
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 172 return PTR_ERR(hpriv);
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 173
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 174 /*
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 175 * Internally ahci_platform_get_resources() calls clk_get(dev, NULL)
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 176 * when trying to obtain the functional clock. This SATA controller
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 177 * uses two clocks for which we specify two connection ids. If we don't
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 178 * have the functional clock at this point - call clk_get() again with
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 179 * con_id = "fck".
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 180 */
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 @181 if (!hpriv->clks[0]) {
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 182 clk = clk_get(dev, "fck");
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 183 if (IS_ERR(clk))
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 184 return PTR_ERR(clk);
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 185
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 @186 hpriv->clks[0] = clk;
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 187 }
82dbe1a68fd65a Bartosz Golaszewski 2017-01-30 188
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 189 /*
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 190 * The second clock used by ahci-da850 is the external REFCLK. If we
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 191 * didn't get it from ahci_platform_get_resources(), let's try to
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 192 * specify the con_id in clk_get().
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 193 */
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 194 if (!hpriv->clks[1]) {
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 195 clk = clk_get(dev, "refclk");
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 196 if (IS_ERR(clk)) {
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 197 dev_err(dev, "unable to obtain the reference clock");
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 198 return -ENODEV;
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 199 }
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 200
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 201 hpriv->clks[1] = clk;
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 202 }
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 203
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 @204 mpy = ahci_da850_calculate_mpy(clk_get_rate(hpriv->clks[1]));
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 205 if (mpy == 0) {
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 206 dev_err(dev, "invalid REFCLK multiplier value: 0x%x", mpy);
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 207 return -EINVAL;
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 208 }
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 209
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 210 rc = ahci_platform_enable_resources(hpriv);
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 211 if (rc)
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 212 return rc;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 213
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 214 res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
c88c094985ad38 Christophe JAILLET 2017-08-16 215 if (!res) {
c88c094985ad38 Christophe JAILLET 2017-08-16 216 rc = -ENODEV;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 217 goto disable_resources;
c88c094985ad38 Christophe JAILLET 2017-08-16 218 }
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 219
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 220 pwrdn_reg = devm_ioremap(dev, res->start, resource_size(res));
c88c094985ad38 Christophe JAILLET 2017-08-16 221 if (!pwrdn_reg) {
c88c094985ad38 Christophe JAILLET 2017-08-16 222 rc = -ENOMEM;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 223 goto disable_resources;
c88c094985ad38 Christophe JAILLET 2017-08-16 224 }
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 225
cdf0ead3747200 Bartosz Golaszewski 2017-01-30 226 da850_sata_init(dev, pwrdn_reg, hpriv->mmio, mpy);
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 227
018d5ef2048fca Akinobu Mita 2015-01-29 228 rc = ahci_platform_init_host(pdev, hpriv, &ahci_da850_port_info,
018d5ef2048fca Akinobu Mita 2015-01-29 229 &ahci_platform_sht);
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 230 if (rc)
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 231 goto disable_resources;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 232
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 233 return 0;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 234 disable_resources:
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 235 ahci_platform_disable_resources(hpriv);
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 236 return rc;
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 237 }
ae8723f8a9c8e8 Bartlomiej Zolnierkiewicz 2014-03-25 238

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-03-30 07:26:15

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH 01/21] dt-bindings: ata: sata: Extend number of SATA ports

On 3/24/22 09:16, Serge Semin wrote:
> The denoted in the description upper limit only concerns the Port
> Multipliers, but not the actual SATA ports. It's an external device
> attached to a SATA port in order to access more than one SATA-drive. So
> when it's attached to a SATA port it just extends the port capability
> while the number of actual SATA ports stays the same. For instance on AHCI
> controllers the number of actual ports is determined by the CAP.NP field
> and the PI (Ports Implemented) register. AFAICS in general the maximum
> number of SATA ports depends on the particular controller implementation.
> Generic AHCI controller can't have more than 32 ports (since CAP.NP is of
> 5 bits wide and PI register is 32-bits size), while DWC AHCI SATA
> controller can't be configured with more than 8 ports activated. So let's
> discard the SATA ports reg-property restrictions and just make sure that
> it consists of a single reg-item.
>
> Signed-off-by: Serge Semin <[email protected]>
> ---
> Documentation/devicetree/bindings/ata/sata-common.yaml | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/ata/sata-common.yaml b/Documentation/devicetree/bindings/ata/sata-common.yaml
> index 7ac77b1c5850..c619f0ae72fb 100644
> --- a/Documentation/devicetree/bindings/ata/sata-common.yaml
> +++ b/Documentation/devicetree/bindings/ata/sata-common.yaml
> @@ -41,11 +41,10 @@ patternProperties:
> properties:
> reg:
> minimum: 0
> - maximum: 14

Why remove this ? Since AHCI can have up to 32 ports, then change the
value to 31. As the comment at the top of the file says, this is not
intended to be a device tree binding spec, but defines properties common
to most adapters.

> description:
> - The ID number of the drive port SATA can potentially use a port
> - multiplier making it possible to connect up to 15 disks to a single
> - SATA port.
> + The ID number of the SATA port. Aside with being directly used
> + each port can have a Port Multiplier attached thus allowing to
> + access more than one drive by means of a single channel.

Please add a comma after "Aside with being directly used", otherwise the
sentence is hard to understand. And replace "channel" with "SATA port" to
stick with the terms defined here.

>
> additionalProperties: true
>


--
Damien Le Moal
Western Digital Research

2022-04-12 07:18:53

by Serge Semin

[permalink] [raw]
Subject: Re: [PATCH 12/21] ata: libahci: Discard redundant force_port_map parameter

On Mon, Apr 11, 2022 at 09:25:03PM +0900, Damien Le Moal wrote:
> On 4/11/22 21:11, Serge Semin wrote:
> > On Thu, Mar 24, 2022 at 11:05:58AM +0900, Damien Le Moal wrote:
> >> On 3/24/22 09:16, Serge Semin wrote:
> >>> Currently there are four port-map-related fields declared in the
> >>> ahci_host_priv structure and used to setup the HBA ports mapping. First
> >>> the ports-mapping is read from the PI register and immediately stored in
> >>> the saved_port_map field. If forced_port_map is initialized with non-zero
> >>> value then its value will have greater priority over the value read from
> >>> PI, thus it will override the saved_port_map field. That value will be then
> >>> masked by a non-zero mask_port_map field and after some sanity checks it
> >>> will be stored in the ahci_host_priv.port_map field as a final port
> >>> mapping.
> >>>
> >>> As you can see the logic is a bit too complicated for such a simple task.
> >>> We can freely get rid from at least one of the fields with no change to
> >>> the implemented semantic. The force_port_map field can be replaced with
> >>> taking non-zero saved_port_map value into account. So if saved_port_map is
> >>> pre-initialized by the glue-driver/platform-specific code then it will
> >>
> >
> >> glue-driver == LLDD (low level device driver), for the entire series please.
> >
> > Why? It's a normal term and well known amongst developers. I've used it
> > in a plenty of my patches before and none of them has been questioned in that
> > part so far.
>

> This term is not used in libata, nor do I remember seeing it used in SCSI
> or block subsystem either. We always talk about mid-layer (ahci platform)
> and LLDD (adapter driver).

Great, do we need to learn the subsystem-specific dictionary now
before submitting the patches for it? =)
Really, you are asking me to change one term to its synonym just
because it's mainly unused here. Now you know what it means, the
others can easily google it and get to learn something new. Win-win.)

-Sergey

>
> >
> > -Sergey
> >
> >>
> >>> have greater priority over the value read from PI register and will be
> >>> used as actual HBA ports mapping later on. Thus the ports map forcing task
> >>> will be just transferred from the force_port_map to saved_port_map field.
> >>>
> >>> This modification will perfectly fit into the feature of having OF-based
> >>> initialization of the HW-init HBA CSR fields we are about to introduce in
> >>> the next commit.
> >>>
> >>> Signed-off-by: Serge Semin <[email protected]>
> >>> ---
> >>> drivers/ata/ahci.c | 2 +-
> >>> drivers/ata/ahci.h | 1 -
> >>> drivers/ata/libahci.c | 10 ++++++----
> >>> drivers/ata/libahci_platform.c | 2 +-
> >>> 4 files changed, 8 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> >>> index ab5811ef5a53..8ce0d166cc8d 100644
> >>> --- a/drivers/ata/ahci.c
> >>> +++ b/drivers/ata/ahci.c
> >>> @@ -654,7 +654,7 @@ static void ahci_pci_save_initial_config(struct pci_dev *pdev,
> >>> {
> >>> if (pdev->vendor == PCI_VENDOR_ID_JMICRON && pdev->device == 0x2361) {
> >>> dev_info(&pdev->dev, "JMB361 has only one port\n");
> >>> - hpriv->force_port_map = 1;
> >>> + hpriv->saved_port_map = 1;
> >>> }
> >>>
> >>> /*
> >>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> >>> index 04690b4168a3..519d148ecaea 100644
> >>> --- a/drivers/ata/ahci.h
> >>> +++ b/drivers/ata/ahci.h
> >>> @@ -329,7 +329,6 @@ struct ahci_port_priv {
> >>> struct ahci_host_priv {
> >>> /* Input fields */
> >>> unsigned int flags; /* AHCI_HFLAG_* */
> >>> - u32 force_port_map; /* force port map */
> >>> u32 mask_port_map; /* mask out particular bits */
> >>>
> >>> void __iomem * mmio; /* bus-independent mem map */
> >>> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> >>> index 0ed484e04fd6..011175e82174 100644
> >>> --- a/drivers/ata/libahci.c
> >>> +++ b/drivers/ata/libahci.c
> >>> @@ -453,7 +453,6 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> >>> * reset. Values without are used for driver operation.
> >>> */
> >>> hpriv->saved_cap = cap = readl(mmio + HOST_CAP);
> >>> - hpriv->saved_port_map = port_map = readl(mmio + HOST_PORTS_IMPL);
> >>>
> >>> /* CAP2 register is only defined for AHCI 1.2 and later */
> >>> vers = readl(mmio + HOST_VERSION);
> >>> @@ -517,10 +516,13 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> >>> cap &= ~HOST_CAP_SXS;
> >>> }
> >>>
> >>> - if (hpriv->force_port_map && port_map != hpriv->force_port_map) {
> >>> + /* 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",
> >>> - port_map, hpriv->force_port_map);
> >>> - port_map = hpriv->force_port_map;
> >>> + port_map, hpriv->saved_port_map);
> >>> + port_map = hpriv->saved_port_map;
> >>> + } else {
> >>> hpriv->saved_port_map = port_map;
> >>> }
> >>>
> >>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> >>> index febad33aa43c..5cbc2c42164d 100644
> >>> --- a/drivers/ata/libahci_platform.c
> >>> +++ b/drivers/ata/libahci_platform.c
> >>> @@ -539,7 +539,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >>> }
> >>>
> >>> of_property_read_u32(dev->of_node,
> >>> - "ports-implemented", &hpriv->force_port_map);
> >>> + "ports-implemented", &hpriv->saved_port_map);
> >>>
> >>> if (child_nodes) {
> >>> for_each_child_of_node(dev->of_node, child) {
> >>
> >>
> >> --
> >> Damien Le Moal
> >> Western Digital Research
>
>
> --
> Damien Le Moal
> Western Digital Research

2022-04-12 13:14:33

by Serge Semin

[permalink] [raw]
Subject: Re: [PATCH 01/21] dt-bindings: ata: sata: Extend number of SATA ports

On Tue, Mar 29, 2022 at 05:15:12PM +0900, Damien Le Moal wrote:
> On 3/24/22 09:16, Serge Semin wrote:
> > The denoted in the description upper limit only concerns the Port
> > Multipliers, but not the actual SATA ports. It's an external device
> > attached to a SATA port in order to access more than one SATA-drive. So
> > when it's attached to a SATA port it just extends the port capability
> > while the number of actual SATA ports stays the same. For instance on AHCI
> > controllers the number of actual ports is determined by the CAP.NP field
> > and the PI (Ports Implemented) register. AFAICS in general the maximum
> > number of SATA ports depends on the particular controller implementation.
> > Generic AHCI controller can't have more than 32 ports (since CAP.NP is of
> > 5 bits wide and PI register is 32-bits size), while DWC AHCI SATA
> > controller can't be configured with more than 8 ports activated. So let's
> > discard the SATA ports reg-property restrictions and just make sure that
> > it consists of a single reg-item.
> >
> > Signed-off-by: Serge Semin <[email protected]>
> > ---
> > Documentation/devicetree/bindings/ata/sata-common.yaml | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/ata/sata-common.yaml b/Documentation/devicetree/bindings/ata/sata-common.yaml
> > index 7ac77b1c5850..c619f0ae72fb 100644
> > --- a/Documentation/devicetree/bindings/ata/sata-common.yaml
> > +++ b/Documentation/devicetree/bindings/ata/sata-common.yaml
> > @@ -41,11 +41,10 @@ patternProperties:
> > properties:
> > reg:
> > minimum: 0
> > - maximum: 14
>

> Why remove this ? Since AHCI can have up to 32 ports, then change the
> value to 31. As the comment at the top of the file says, this is not
> intended to be a device tree binding spec, but defines properties common
> to most adapters.

Right, but the schema determines the common !SATA! controllers properties,
while you are referring to the AHCI-specific limit. As I said in the patch
log AFAICS in general the SATA controllers may have any number of ports.
The number is determined by the hardware designers needs only. Thus the
actual constraints needs to be specified in the controller-specific
YAML-schema (the one which will $ref to sata-common.yaml).

Though I couldn't find any ATA device driver in the kernel which would
handle a device with even 32 ports, not to mention a greater number.
So replacing it with 31 might be reasonable after all. But me failing
to find any such device doesn't me it can't exist. Thus I've decided to
drop the upper limit completely.

>
> > description:
> > - The ID number of the drive port SATA can potentially use a port
> > - multiplier making it possible to connect up to 15 disks to a single
> > - SATA port.
> > + The ID number of the SATA port. Aside with being directly used
> > + each port can have a Port Multiplier attached thus allowing to
> > + access more than one drive by means of a single channel.
>

> Please add a comma after "Aside with being directly used", otherwise the
> sentence is hard to understand. And replace "channel" with "SATA port" to
> stick with the terms defined here.

Ok.

-Sergey

>
> >
> > additionalProperties: true
> >
>
>
> --
> Damien Le Moal
> Western Digital Research

2022-04-12 19:51:19

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH 12/21] ata: libahci: Discard redundant force_port_map parameter

On 4/11/22 21:11, Serge Semin wrote:
> On Thu, Mar 24, 2022 at 11:05:58AM +0900, Damien Le Moal wrote:
>> On 3/24/22 09:16, Serge Semin wrote:
>>> Currently there are four port-map-related fields declared in the
>>> ahci_host_priv structure and used to setup the HBA ports mapping. First
>>> the ports-mapping is read from the PI register and immediately stored in
>>> the saved_port_map field. If forced_port_map is initialized with non-zero
>>> value then its value will have greater priority over the value read from
>>> PI, thus it will override the saved_port_map field. That value will be then
>>> masked by a non-zero mask_port_map field and after some sanity checks it
>>> will be stored in the ahci_host_priv.port_map field as a final port
>>> mapping.
>>>
>>> As you can see the logic is a bit too complicated for such a simple task.
>>> We can freely get rid from at least one of the fields with no change to
>>> the implemented semantic. The force_port_map field can be replaced with
>>> taking non-zero saved_port_map value into account. So if saved_port_map is
>>> pre-initialized by the glue-driver/platform-specific code then it will
>>
>
>> glue-driver == LLDD (low level device driver), for the entire series please.
>
> Why? It's a normal term and well known amongst developers. I've used it
> in a plenty of my patches before and none of them has been questioned in that
> part so far.

This term is not used in libata, nor do I remember seeing it used in SCSI
or block subsystem either. We always talk about mid-layer (ahci platform)
and LLDD (adapter driver).

>
> -Sergey
>
>>
>>> have greater priority over the value read from PI register and will be
>>> used as actual HBA ports mapping later on. Thus the ports map forcing task
>>> will be just transferred from the force_port_map to saved_port_map field.
>>>
>>> This modification will perfectly fit into the feature of having OF-based
>>> initialization of the HW-init HBA CSR fields we are about to introduce in
>>> the next commit.
>>>
>>> Signed-off-by: Serge Semin <[email protected]>
>>> ---
>>> drivers/ata/ahci.c | 2 +-
>>> drivers/ata/ahci.h | 1 -
>>> drivers/ata/libahci.c | 10 ++++++----
>>> drivers/ata/libahci_platform.c | 2 +-
>>> 4 files changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>> index ab5811ef5a53..8ce0d166cc8d 100644
>>> --- a/drivers/ata/ahci.c
>>> +++ b/drivers/ata/ahci.c
>>> @@ -654,7 +654,7 @@ static void ahci_pci_save_initial_config(struct pci_dev *pdev,
>>> {
>>> if (pdev->vendor == PCI_VENDOR_ID_JMICRON && pdev->device == 0x2361) {
>>> dev_info(&pdev->dev, "JMB361 has only one port\n");
>>> - hpriv->force_port_map = 1;
>>> + hpriv->saved_port_map = 1;
>>> }
>>>
>>> /*
>>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>>> index 04690b4168a3..519d148ecaea 100644
>>> --- a/drivers/ata/ahci.h
>>> +++ b/drivers/ata/ahci.h
>>> @@ -329,7 +329,6 @@ struct ahci_port_priv {
>>> struct ahci_host_priv {
>>> /* Input fields */
>>> unsigned int flags; /* AHCI_HFLAG_* */
>>> - u32 force_port_map; /* force port map */
>>> u32 mask_port_map; /* mask out particular bits */
>>>
>>> void __iomem * mmio; /* bus-independent mem map */
>>> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
>>> index 0ed484e04fd6..011175e82174 100644
>>> --- a/drivers/ata/libahci.c
>>> +++ b/drivers/ata/libahci.c
>>> @@ -453,7 +453,6 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
>>> * reset. Values without are used for driver operation.
>>> */
>>> hpriv->saved_cap = cap = readl(mmio + HOST_CAP);
>>> - hpriv->saved_port_map = port_map = readl(mmio + HOST_PORTS_IMPL);
>>>
>>> /* CAP2 register is only defined for AHCI 1.2 and later */
>>> vers = readl(mmio + HOST_VERSION);
>>> @@ -517,10 +516,13 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
>>> cap &= ~HOST_CAP_SXS;
>>> }
>>>
>>> - if (hpriv->force_port_map && port_map != hpriv->force_port_map) {
>>> + /* 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",
>>> - port_map, hpriv->force_port_map);
>>> - port_map = hpriv->force_port_map;
>>> + port_map, hpriv->saved_port_map);
>>> + port_map = hpriv->saved_port_map;
>>> + } else {
>>> hpriv->saved_port_map = port_map;
>>> }
>>>
>>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>>> index febad33aa43c..5cbc2c42164d 100644
>>> --- a/drivers/ata/libahci_platform.c
>>> +++ b/drivers/ata/libahci_platform.c
>>> @@ -539,7 +539,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>>> }
>>>
>>> of_property_read_u32(dev->of_node,
>>> - "ports-implemented", &hpriv->force_port_map);
>>> + "ports-implemented", &hpriv->saved_port_map);
>>>
>>> if (child_nodes) {
>>> for_each_child_of_node(dev->of_node, child) {
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research


--
Damien Le Moal
Western Digital Research

2022-04-12 21:39:15

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH 12/21] ata: libahci: Discard redundant force_port_map parameter

On 4/12/22 05:52, Serge Semin wrote:
> On Mon, Apr 11, 2022 at 09:25:03PM +0900, Damien Le Moal wrote:
>> On 4/11/22 21:11, Serge Semin wrote:
>>> On Thu, Mar 24, 2022 at 11:05:58AM +0900, Damien Le Moal wrote:
>>>> On 3/24/22 09:16, Serge Semin wrote:
>>>>> Currently there are four port-map-related fields declared in the
>>>>> ahci_host_priv structure and used to setup the HBA ports mapping. First
>>>>> the ports-mapping is read from the PI register and immediately stored in
>>>>> the saved_port_map field. If forced_port_map is initialized with non-zero
>>>>> value then its value will have greater priority over the value read from
>>>>> PI, thus it will override the saved_port_map field. That value will be then
>>>>> masked by a non-zero mask_port_map field and after some sanity checks it
>>>>> will be stored in the ahci_host_priv.port_map field as a final port
>>>>> mapping.
>>>>>
>>>>> As you can see the logic is a bit too complicated for such a simple task.
>>>>> We can freely get rid from at least one of the fields with no change to
>>>>> the implemented semantic. The force_port_map field can be replaced with
>>>>> taking non-zero saved_port_map value into account. So if saved_port_map is
>>>>> pre-initialized by the glue-driver/platform-specific code then it will
>>>>
>>>
>>>> glue-driver == LLDD (low level device driver), for the entire series please.
>>>
>>> Why? It's a normal term and well known amongst developers. I've used it
>>> in a plenty of my patches before and none of them has been questioned in that
>>> part so far.
>>
>
>> This term is not used in libata, nor do I remember seeing it used in SCSI
>> or block subsystem either. We always talk about mid-layer (ahci platform)
>> and LLDD (adapter driver).
>
> Great, do we need to learn the subsystem-specific dictionary now
> before submitting the patches for it? =)
> Really, you are asking me to change one term to its synonym just
> because it's mainly unused here. Now you know what it means, the
> others can easily google it and get to learn something new. Win-win.)

I already knew what it meant, but it was unclear if my idea of what you
meant was actually the same as yours. Sticking with the vocabulary already
used since *a long time ago* makes life easier for reviewers and avoids
confusion.

--
Damien Le Moal
Western Digital Research

2022-04-12 22:06:51

by Serge Semin

[permalink] [raw]
Subject: Re: [PATCH 12/21] ata: libahci: Discard redundant force_port_map parameter

On Tue, Apr 12, 2022 at 07:48:50AM +0900, Damien Le Moal wrote:
> On 4/12/22 05:52, Serge Semin wrote:
> > On Mon, Apr 11, 2022 at 09:25:03PM +0900, Damien Le Moal wrote:
> >> On 4/11/22 21:11, Serge Semin wrote:
> >>> On Thu, Mar 24, 2022 at 11:05:58AM +0900, Damien Le Moal wrote:
> >>>> On 3/24/22 09:16, Serge Semin wrote:
> >>>>> Currently there are four port-map-related fields declared in the
> >>>>> ahci_host_priv structure and used to setup the HBA ports mapping. First
> >>>>> the ports-mapping is read from the PI register and immediately stored in
> >>>>> the saved_port_map field. If forced_port_map is initialized with non-zero
> >>>>> value then its value will have greater priority over the value read from
> >>>>> PI, thus it will override the saved_port_map field. That value will be then
> >>>>> masked by a non-zero mask_port_map field and after some sanity checks it
> >>>>> will be stored in the ahci_host_priv.port_map field as a final port
> >>>>> mapping.
> >>>>>
> >>>>> As you can see the logic is a bit too complicated for such a simple task.
> >>>>> We can freely get rid from at least one of the fields with no change to
> >>>>> the implemented semantic. The force_port_map field can be replaced with
> >>>>> taking non-zero saved_port_map value into account. So if saved_port_map is
> >>>>> pre-initialized by the glue-driver/platform-specific code then it will
> >>>>
> >>>
> >>>> glue-driver == LLDD (low level device driver), for the entire series please.
> >>>
> >>> Why? It's a normal term and well known amongst developers. I've used it
> >>> in a plenty of my patches before and none of them has been questioned in that
> >>> part so far.
> >>
> >
> >> This term is not used in libata, nor do I remember seeing it used in SCSI
> >> or block subsystem either. We always talk about mid-layer (ahci platform)
> >> and LLDD (adapter driver).
> >
> > Great, do we need to learn the subsystem-specific dictionary now
> > before submitting the patches for it? =)
> > Really, you are asking me to change one term to its synonym just
> > because it's mainly unused here. Now you know what it means, the
> > others can easily google it and get to learn something new. Win-win.)
>

> I already knew what it meant, but it was unclear if my idea of what you
> meant was actually the same as yours. Sticking with the vocabulary already
> used since *a long time ago* makes life easier for reviewers and avoids
> confusion.

I believe there can't be too many possible meaning of that term to
have much doubts. Especially when we refer to the kernel drivers. One
more time requesting to settle some implicit subsystem-specific
conventions, not having them described in some kernel documents seems
very much wrong. The ahci_ prefixing and the specific vocabulary
concerns each driver in very many aspects. Seeing there are not a few
drivers which don't follow those conventions, you give no chance for
the developers to get their code accepted with no requests to fix the
corresponding parts. So to speak you need to thoroughly describe these
and the others conventions in the kernel documentation otherwise
you'll always end up requesting the same fixes wasting your and
developers time again and again.

Really if you had that document available, you could have used as a
reference and just said something like "please, follow the coding
style convention described here..." and no question would have raised.
Meanwhile currently both ahci_-prefix change and using the LLDD term
seem more like a personal desire to me.

-Sergey

>
> --
> Damien Le Moal
> Western Digital Research