On 6/18/22 05:31, Serge Semin wrote:

On Thu, Jun 16, 2022 at 09:28:18AM +0900, Damien Le Moal wrote:

quoted

On 2022/06/16 5:58, Serge Semin wrote:

quoted

On Tue, Jun 14, 2022 at 05:32:41PM +0900, Damien Le Moal wrote:

quoted

On 6/10/22 17:17, Serge Semin wrote:

quoted

Currently not all of the Port-specific capabilities listed in the

s/listed/are listed

quoted

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 <redacted>
Reviewed-by: Hannes Reinecke <hare@suse.de>

---

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,

quoted

What is this one for ? A comment above it would be nice.

Isn't it obviously inferrable from the definition and the item name?

quoted

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.

quoted

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

quoted

quoted

-Sergey

quoted

quoted

+
 	/* 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