[PATCH v8 2/3] ahci_xgene: Skip the PHY and clock initialization if already configured by the firmware.

[PATCH v8 2/3] ahci_xgene: Skip the PHY and clock initialization if already configured by the firmware.

[Sergei Shtylyov]

Hello.

On 08/23/2014 10:37 PM, Suman Tripathi wrote:

> This patch implements the feature to skip the PHY and clock
> initialization if it is already configured by the firmware.

> Signed-off-by: Loc Ho <lho@apm.com>
> Signed-off-by: Suman Tripathi <stripathi@apm.com>
> ---
>   drivers/ata/ahci_xgene.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)

> diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
> index f416495..fd9c137 100644
> --- a/drivers/ata/ahci_xgene.c
> +++ b/drivers/ata/ahci_xgene.c
> @@ -145,6 +145,15 @@ static unsigned int xgene_ahci_qc_issue(struct ata_queued_cmd *qc)
>   	return rc;
>   }
>
> +static bool xgene_ahci_is_memram_inited(struct xgene_ahci_context *ctx)
> +{
> +	void __iomem *diagcsr = ctx->csr_diag;
> +
> +	return (readl(diagcsr + CFG_MEM_RAM_SHUTDOWN) == 0 &&
> +	        readl(diagcsr + BLOCK_MEM_RDY) == 0xFFFFFFFF) ?
> +		true : false;

    Just return the result of &&, that should be enough.

WBR, Sergei

[PATCH v8 3/3] ahci_xgene: Fix the link down in first attempt for the APM X-Gene SoC AHCI SATA host controller driver.

[Tejun Heo]

On Sun, Aug 24, 2014 at 12:07:27AM +0530, Suman Tripathi wrote:
> This patch addresses two HW erratas as described below by retrying the
> COMRESET:
> 
> 1. During speed negotiation, controller is not able to detect ALIGN
> at GEN3(6Gbps) within 54.6us and results in a timeout. This issue can
> be recovered by issuing a COMRESET.
> 
> 2. Although ALIGN detection is successful, 8b/10b and disparity bit
> errors can occur which result in the signature FIS not received
> successfully by the Host controller. Due to this, the PHY communication
> between the host and drive is not established because of CDR(clock and
> data recovery) circuit doesn't lock. This issue can be recoverd by issuing
> a COMRESET.
> 
> The above retries are issued only if the port status register PXSTATUS
> reports device presence detected but PHY communication not established.
> The maximum retry attempts are 3.

Didn't I ask you to update the comment to explain what's going on?  Or
is the existing comment already sufficient?

Thanks.

-- 
tejun

[PATCH v8 3/3] ahci_xgene: Fix the link down in first attempt for the APM X-Gene SoC AHCI SATA host controller driver.

[Tejun Heo]

On Tue, Aug 26, 2014 at 12:17:35PM +0530, Suman Tripathi wrote:
> Didn't I ask you to update the comment to explain what's going on?
> [suman] : can you specifically tell which part of the comment is not clear
> and need more explanation?

The comment on top of the function doesn't seem to match what's being
implemented.  In addition, it's generally not very useful to list the
actual algorithm in text.  Put algorithm in code and explain the
summary and rationales for it in the comments.  Nothing explains why
the retries are being done.

> is the existing comment already sufficient?
> [suman] : The existing comment is sufficient .

No, this isn't.  You don't have to include a novel to explain it but
there's something different going on here and you should provide
information on why this sort of deviation is necessary.

-- 
tejun