From: Serge Semin <fancer.lancer@gmail.com>
To: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>,
Hans de Goede <hdegoede@redhat.com>, Jens Axboe <axboe@kernel.dk>,
Hannes Reinecke <hare@suse.de>,
Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>,
Rob Herring <robh+dt@kernel.org>,
linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org
Subject: Re: [PATCH v4 12/23] ata: libahci: Extend port-cmd flags set with port capabilities
Date: Wed, 29 Jun 2022 04:47:42 +0300 [thread overview]
Message-ID: <20220629014742.bajkuosd752pg2po@mobilestation> (raw)
In-Reply-To: <3dade607-71a5-28ad-fb0b-f5b75c1b5392@opensource.wdc.com>
On Wed, Jun 29, 2022 at 10:35:13AM +0900, Damien Le Moal wrote:
> On 6/28/22 21:08, Serge Semin wrote:
> > Damien,
> > Any notes to the comments below?
>
> Been very busy and had no time to look at this. Please post your latest
> version of the series and we'll go from there.
Ok. As soon as I get the responses from Rob.
-Sergey
>
> >
> > -Sergey
> >
> > On Sat, Jun 18, 2022 at 11:10:55AM +0300, Serge Semin wrote:
> >> On Sat, Jun 18, 2022 at 03:52:28PM +0900, Damien Le Moal wrote:
> >>> On 6/18/22 05:31, Serge Semin wrote:
> >>>> On Thu, Jun 16, 2022 at 09:28:18AM +0900, Damien Le Moal wrote:
> >>>>> On 2022/06/16 5:58, Serge Semin wrote:
> >>>>>> On Tue, Jun 14, 2022 at 05:32:41PM +0900, Damien Le Moal wrote:
> >>>>>>> On 6/10/22 17:17, Serge Semin wrote:
> >>>>>>>> Currently not all of the Port-specific capabilities listed in the
> >>>>>>>
> >>>>>>> s/listed/are listed
> >>>>>>>
> >>>>>>>> PORT_CMD-enumeration. Let's extend that set with the Cold Presence
> >>>>>>>> Detection and Mechanical Presence Switch attached to the Port flags [1] so
> >>>>>>>> to closeup the set of the platform-specific port-capabilities flags. Note
> >>>>>>>> these flags are supposed to be set by the platform firmware if there is
> >>>>>>>> one. Alternatively as we are about to do they can be set by means of the
> >>>>>>>> OF properties.
> >>>>>>>>
> >>>>>>>> While at it replace PORT_IRQ_DEV_ILCK with PORT_IRQ_DMPS and fix the
> >>>>>>>> comment there. In accordance with [2] that IRQ flag is supposed to
> >>>>>>>> indicate the state of the signal coming from the Mechanical Presence
> >>>>>>>> Switch.
> >>>>>>>>
> >>>>>>>> [1] Serial ATA AHCI 1.3.1 Specification, p.27
> >>>>>>>> [2] Serial ATA AHCI 1.3.1 Specification, p.24, p.88
> >>>>>>>>
> >>>>>>>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> >>>>>>>> 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,
> >>>>>>>
> >>>>>>
> >>>>>>> What is this one for ? A comment above it would be nice.
> >>>>>>
> >>>>>> Isn't it obviously inferrable from the definition and the item name?
> >>>>>
> >>>>
> >>>>> I am guessing from the name. Am I guessing OK ? A comment would still be nice.
> >>>>> Why just these bits ? There are more cap/support indicator bits in that port cmd
> >>>>> bitfield. So why this particular set of bits ? What do they mean all together ?
> >>>>
> >>>> Normally the variable/constant name should be self-content (as the
> >>>> kernel coding style doc states and what the common sense suggests). So
> >>>> the reader could correctly guess its purpose/content/value. In this
> >>>> case PORT_CMD_CAP - means PORT CMD capabilities mask. All of the
> >>>> possible flags have been set in that mask. There are no more
> >>>> capabilities in the PORT CMD register left undeclared. That's why the
> >>>> name is selected the way it is and why I haven't added any comment in
> >>>> here (what the kernel coding style says about the over-commenting the
> >>>> code).
> >>>
> >>
> >>> Yes, I understood from the name what it is. What I do NOT understand is
> >>> why all the feature bits are not there. Why this subset only ? A comment
> >>> about that would be nice so that the reason for it is not lost.
> >>
> >> Well, because it's indeed "PORT_CMD capabilities mask", and not features,
> >> not setups, not settings, not status flags, etc. As I said all the port
> >> Capabilities have been listed in that mask:
> >> PORT_CMD_FBSCP BIT(22) - FIS-based Switching Capable Port
> >> PORT_CMD_ESP BIT(21) - External SATA Port
> >> PORT_CMD_CPD BIT(20) - Cold Presence Detect
> >> PORT_CMD_MPSP BIT(19) - Mechanical Presence Switch Attached to Port
> >> PORT_CMD_HPCP BIT(18) - Hot Plug Capable Port
> >> I've or'ed-them-up in a single mask => PORT_CMD_CAP in order to work
> >> with them independently from the rest of the PORT_CMD CSR fields.
> >>
> >> Unlike the generic controller CAP/CAP2 registers, which consists of the
> >> device capabilities only, PORT_CMD contains various R/W settings (PM, LED
> >> driver, etc), RO status flags (CMD-list running, FIS recv running, etc)
> >> and amongst other the RO/Wo !port-specific capabilities!. The later ones
> >> indicate the platform-specific device features. Since the register
> >> contains flags with the intermixed nature, I need to have a mask to at
> >> least get the capabilities and preserve them between the device
> >> resets. That's why the PORT_CMD_CAP has been introduced in the
> >> framework of this patch. Its name was chosen with a reference to the
> >> CAP registers, see:
> >> HOST_CAP, HOST_CAP2, and finally my PORT_CMD_CAP.
> >>
> >>>
> >>>>
> >>>>>
> >>>>> Sure I can go and read the specs to figure it out. But again, a comment would
> >>>>> avoid readers of the code to have to decrypt all that.
> >>>>
> >>>> If you still insist on having an additional comment. I can add
> >>>> something like "/* PORT_CMD capabilities mask */". Are you ok with it?
> >>>
> >>
> >>> That does not help on its own. The macro name says that already. I would
> >>> like a note about why only these features are selected.
> >>
> >> Please see the explanation above. I don't see what else to say about
> >> that mask, because in short what I said above really means "PORT_CMD
> >> capabilities mask". So should you have some more clever text, which
> >> would be more suitable here, please tell me and I'll add it to the
> >> patch.
> >>
> >> Regarding what you said earlier. In order to fully understand the
> >> AHCI driver a hacker would always need to read the specs. There is
> >> just no way to do that effectively enough without the controller
> >> manual at hands. And the PORT_CMD capabilities isn't the most
> >> complicated part of the device.
> >>
> >> -Sergey
> >>
> >>>
> >>>>
> >>>> -Sergey
> >>>>
> >>>>>
> >>>>>>
> >>>>>> -Sergey
> >>>>>>
> >>>>>>>
> >>>>>>>> +
> >>>>>>>> /* PORT_FBS bits */
> >>>>>>>> PORT_FBS_DWE_OFFSET = 16, /* FBS device with error offset */
> >>>>>>>> PORT_FBS_ADO_OFFSET = 12, /* FBS active dev optimization offset */
> >>>>>>>
> >>>>>>>
> >>>>>>> --
> >>>>>>> Damien Le Moal
> >>>>>>> Western Digital Research
> >>>>>
> >>>>>
> >>>>> --
> >>>>> Damien Le Moal
> >>>>> Western Digital Research
> >>>
> >>>
> >>> --
> >>> Damien Le Moal
> >>> Western Digital Research
>
>
> --
> Damien Le Moal
> Western Digital Research
next prev parent reply other threads:[~2022-06-29 1:47 UTC|newest]
Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-10 8:17 [PATCH v4 00/23] ata: ahci: Add DWC/Baikal-T1 AHCI SATA support Serge Semin
2022-06-10 8:17 ` [PATCH v4 01/23] dt-bindings: ata: ahci-platform: Move dma-coherent to sata-common.yaml Serge Semin
2022-06-14 22:02 ` Rob Herring
2022-06-14 22:15 ` Florian Fainelli
2022-06-10 8:17 ` [PATCH v4 02/23] dt-bindings: ata: ahci-platform: Detach common AHCI bindings Serge Semin
2022-06-14 22:16 ` Rob Herring
2022-06-10 8:17 ` [PATCH v4 03/23] dt-bindings: ata: ahci-platform: Clarify common AHCI props constraints Serge Semin
2022-06-14 22:17 ` Rob Herring
2022-06-10 8:17 ` [PATCH v4 04/23] dt-bindings: ata: sata: Extend number of SATA ports Serge Semin
2022-06-10 8:17 ` [PATCH v4 05/23] dt-bindings: ata: sata-brcm: Apply common AHCI schema Serge Semin
2022-06-14 22:15 ` Florian Fainelli
2022-06-14 22:17 ` Rob Herring
2022-06-10 8:17 ` [PATCH v4 06/23] ata: libahci_platform: Convert to using platform devm-ioremap methods Serge Semin
2022-06-10 8:17 ` [PATCH v4 07/23] ata: libahci_platform: Convert to using devm bulk clocks API Serge Semin
2022-06-14 8:22 ` Damien Le Moal
2022-06-15 20:45 ` Serge Semin
2022-06-16 0:23 ` Damien Le Moal
2022-06-17 19:54 ` Serge Semin
2022-06-10 8:17 ` [PATCH v4 08/23] ata: libahci_platform: Sanity check the DT child nodes number Serge Semin
2022-06-14 8:23 ` Damien Le Moal
2022-06-15 20:53 ` Serge Semin
2022-06-16 0:25 ` Damien Le Moal
2022-06-17 20:18 ` Serge Semin
2022-06-18 6:49 ` Damien Le Moal
2022-06-10 8:17 ` [PATCH v4 09/23] ata: libahci_platform: Parse ports-implemented property in resources getter Serge Semin
2022-06-10 8:17 ` [PATCH v4 10/23] ata: libahci_platform: Introduce reset assertion/deassertion methods Serge Semin
2022-06-10 8:17 ` [PATCH v4 11/23] dt-bindings: ata: ahci: Add platform capability properties Serge Semin
2022-06-14 22:19 ` Rob Herring
2022-06-15 21:56 ` Serge Semin
2022-06-10 8:17 ` [PATCH v4 12/23] ata: libahci: Extend port-cmd flags set with port capabilities Serge Semin
2022-06-14 8:32 ` Damien Le Moal
2022-06-15 20:58 ` Serge Semin
2022-06-16 0:28 ` Damien Le Moal
2022-06-17 20:31 ` Serge Semin
2022-06-18 6:52 ` Damien Le Moal
2022-06-18 8:10 ` Serge Semin
2022-06-28 12:08 ` Serge Semin
2022-06-29 1:35 ` Damien Le Moal
2022-06-29 1:47 ` Serge Semin [this message]
2022-06-10 8:17 ` [PATCH v4 13/23] ata: libahci: Discard redundant force_port_map parameter Serge Semin
2022-06-10 8:17 ` [PATCH v4 14/23] ata: libahci: Don't read AHCI version twice in the save-config method Serge Semin
2022-06-10 8:17 ` [PATCH v4 15/23] ata: ahci: Convert __ahci_port_base to accepting hpriv as arguments Serge Semin
2022-06-14 8:38 ` Damien Le Moal
2022-06-15 21:25 ` Serge Semin
2022-06-10 8:17 ` [PATCH v4 16/23] ata: ahci: Introduce firmware-specific caps initialization Serge Semin
2022-06-14 8:42 ` Damien Le Moal
2022-06-15 21:11 ` Serge Semin
2022-06-16 0:29 ` Damien Le Moal
2022-06-17 20:32 ` Serge Semin
2022-06-10 8:17 ` [PATCH v4 17/23] dt-bindings: ata: ahci: Add DWC AHCI SATA controller DT schema Serge Semin
2022-06-14 22:27 ` Rob Herring
2022-06-17 19:37 ` Serge Semin
2022-06-28 12:10 ` Serge Semin
2022-07-06 22:36 ` Rob Herring
2022-07-07 15:25 ` Serge Semin
2022-07-12 20:13 ` Rob Herring
2022-07-12 20:43 ` Serge Semin
2022-06-10 8:17 ` [PATCH v4 18/23] ata: libahci_platform: Add function returning a clock-handle by id Serge Semin
2022-06-14 8:45 ` Damien Le Moal
2022-06-15 21:24 ` Serge Semin
2022-06-10 8:17 ` [PATCH v4 19/23] ata: ahci: Add DWC AHCI SATA controller support Serge Semin
2022-06-10 16:34 ` Randy Dunlap
2022-06-10 21:58 ` Serge Semin
2022-06-10 23:34 ` Randy Dunlap
2022-06-15 21:30 ` Serge Semin
2022-06-16 0:31 ` Damien Le Moal
2022-06-17 20:36 ` Serge Semin
2022-06-18 6:54 ` Damien Le Moal
2022-06-14 8:53 ` Damien Le Moal
2022-06-15 21:48 ` Serge Semin
2022-06-16 0:33 ` Damien Le Moal
2022-06-17 20:34 ` Serge Semin
2022-06-10 8:17 ` [PATCH v4 20/23] dt-bindings: ata: ahci: Add Baikal-T1 AHCI SATA controller DT schema Serge Semin
2022-06-14 22:29 ` Rob Herring
2022-06-17 19:49 ` Serge Semin
2022-06-10 8:17 ` [PATCH v4 21/23] ata: ahci-dwc: Add platform-specific quirks support Serge Semin
2022-06-10 8:18 ` [PATCH v4 22/23] ata: ahci-dwc: Add Baikal-T1 AHCI SATA interface support Serge Semin
2022-06-10 8:18 ` [PATCH v4 23/23] MAINTAINERS: Add maintainers for DWC AHCI SATA driver Serge Semin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220629014742.bajkuosd752pg2po@mobilestation \
--to=fancer.lancer@gmail.com \
--cc=Alexey.Malahov@baikalelectronics.ru \
--cc=Pavel.Parkhomenko@baikalelectronics.ru \
--cc=Sergey.Semin@baikalelectronics.ru \
--cc=axboe@kernel.dk \
--cc=damien.lemoal@opensource.wdc.com \
--cc=devicetree@vger.kernel.org \
--cc=hare@suse.de \
--cc=hdegoede@redhat.com \
--cc=linux-ide@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).