Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
IronPort-SDR: SGVyj3iVs2Ww5JZA+s1tx7uxbD/FVU9M02jifvInlLjKRtFwB4vaXrJ5pDkjZ3xaZZiqTUCTle
 jo+nB1dPknfUiFGUbzr8p91wvNp6e6Q+vw63FCDlIXf1T0gj/2ZPWf/55jqqfP1tS/gDQq7Jll
 RoXQMGYZnkN07ZFwpW+2VGKnyTUL9m65tEgjwXGEvkSYJ+CnQRCLp8Cxxxoi8fXXrtpSOyyCpU
 Vpyl5yfq4jXa9pSVE1ghhTgxKh5IQa7BdWpeqUlS/XAQB0UJA8BBcTRc8j2H+uvTOCkTlWcd79
 slcfKTUPf0HTJOFFZq3Q0Jrn
IronPort-SDR: yzul8f24oz1gMEGkH7zjKFkr4uvbgjVamS0GQ85qcLirMFyhmBGENEk+UlSoMTZFBGyHdjMR/I
 s+5CJRRIC/7077/U5MS00kd3qNk1DoVD4kElc4MMNNifwDrMyN24R1O/G5TId0J4yqswCrkSTL
 lKBmWEqHThBcDlJj4w6DjU/7ZTOLDRLA/t5SpLAwFVAsRM9k0EZebo7wnMWoWRVrGouXcypAbR
 vnWtV7L2xS+9YrHzO+caknj7H5L9QIUXRCoGEsL5+0LAbkPWs0nJAfk38Vvpc9ENP7Jl8z248l
 6K0=
WDCIronportException: Internal
Message-ID: <bde34952-e244-a1c3-fc33-251d618d2bb4@opensource.wdc.com>
Date:   Mon, 11 Apr 2022 21:25:03 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH 12/21] ata: libahci: Discard redundant force_port_map
 parameter
Content-Language: en-US
To:     Serge Semin <fancer.lancer@gmail.com>
Cc:     Serge Semin <Sergey.Semin@baikalelectronics.ru>,
        Hans de Goede <hdegoede@redhat.com>,
        Jens Axboe <axboe@kernel.dk>,
        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
References: <20220324001628.13028-1-Sergey.Semin@baikalelectronics.ru>
 <20220324001628.13028-13-Sergey.Semin@baikalelectronics.ru>
 <b06a8382-d5c1-e3a5-8577-692fa82cb3c1@opensource.wdc.com>
 <20220411121151.vm6wmtalbl2lgtgo@mobilestation>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <20220411121151.vm6wmtalbl2lgtgo@mobilestation>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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 <Sergey.Semin@baikalelectronics.ru>
>>> ---
>>>  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