[U-Boot] [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
Michal Simek
michal.simek at xilinx.com
Tue Apr 16 10:00:35 UTC 2019
On 16. 04. 19 11:54, Peng Ma wrote:
>
>
>> -----Original Message-----
>> From: Michal Simek <michal.simek at xilinx.com>
>> Sent: 2019年4月16日 17:31
>> To: Peng Ma <peng.ma at nxp.com>; Michal Simek <michal.simek at xilinx.com>;
>> albert.u.boot at aribaud.net; sjg at chromium.org; Fabio Estevam
>> <fabio.estevam at nxp.com>; York Sun <york.sun at nxp.com>; Prabhakar
>> Kushwaha <prabhakar.kushwaha at nxp.com>
>> Cc: Andy Tang <andy.tang at nxp.com>; Yinbo Zhu <yinbo.zhu at nxp.com>;
>> u-boot at lists.denx.de
>> Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
>>
>> WARNING: This email was created outside of NXP. DO NOT CLICK links or
>> attachments unless you recognize the sender and know the content is safe.
>>
>>
>>
>> On 16. 04. 19 11:22, Peng Ma wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Michal Simek <michal.simek at xilinx.com>
>>>> Sent: 2019年4月16日 16:04
>>>> To: Peng Ma <peng.ma at nxp.com>; albert.u.boot at aribaud.net;
>>>> sjg at chromium.org; Fabio Estevam <fabio.estevam at nxp.com>; York Sun
>>>> <york.sun at nxp.com>; Prabhakar Kushwaha
>> <prabhakar.kushwaha at nxp.com>
>>>> Cc: Andy Tang <andy.tang at nxp.com>; Yinbo Zhu <yinbo.zhu at nxp.com>;
>>>> michal.simek at xilinx.com; u-boot at lists.denx.de
>>>> Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
>>>>
>>>> WARNING: This email was created outside of NXP. DO NOT CLICK links or
>>>> attachments unless you recognize the sender and know the content is safe.
>>>>
>>>>
>>>>
>>>> On 16. 04. 19 9:28, Peng Ma wrote:
>>>>> Distinguish the ecc val by chassis version and move the ecc addr to dts.
>>>>> Add ls1028a soc support.
>>>>>
>>>>> Signed-off-by: Peng Ma <peng.ma at nxp.com>
>>>>> ---
>>>>> drivers/ata/sata_ceva.c | 43
>>>> +++++++++++++++++++++++++------------------
>>>>> 1 files changed, 25 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index
>>>>> 8887be9..d26f712 100644
>>>>> --- a/drivers/ata/sata_ceva.c
>>>>> +++ b/drivers/ata/sata_ceva.c
>>>>> @@ -88,20 +88,16 @@
>>>>> #define LS1021_CEVA_PHY4_CFG 0x064a080b #define
>>>> LS1021_CEVA_PHY5_CFG
>>>>> 0x2aa86470
>>>>>
>>>>> -/* for ls1088a */
>>>>> -#define LS1088_ECC_DIS_ADDR_CH2 0x100520
>>>>> -#define LS1088_ECC_DIS_VAL_CH2 0x40000000
>>>>> -
>>>>> -/* ecc addr-val pair */
>>>>> -#define ECC_DIS_ADDR_CH2 0x20140520
>>>>> +/* ecc val pair */
>>>>> +#define ECC_DIS_VAL_CH1 0x00020000
>>>>> #define ECC_DIS_VAL_CH2 0x80000000
>>>>> -#define SATA_ECC_REG_ADDR 0x20220520
>>>>> -#define SATA_ECC_DISABLE 0x00020000
>>>>> +#define ECC_DIS_VAL_CH3 0x40000000
>>>>>
>>>>> enum ceva_soc {
>>>>> CEVA_1V84,
>>>>> CEVA_LS1012A,
>>>>> CEVA_LS1021A,
>>>>> + CEVA_LS1028A,
>>>>> CEVA_LS1043A,
>>>>> CEVA_LS1046A,
>>>>> CEVA_LS1088A,
>>>>> @@ -110,12 +106,14 @@ enum ceva_soc {
>>>>>
>>>>> struct ceva_sata_priv {
>>>>> ulong base;
>>>>> + ulong ecc_base;
>>>>> enum ceva_soc soc;
>>>>> ulong flag;
>>>>> };
>>>>>
>>>>> static int ceva_init_sata(struct ceva_sata_priv *priv) {
>>>>> + ulong ecc_addr = priv->ecc_base;
>>>>> ulong base = priv->base;
>>>>> ulong tmp;
>>>>>
>>>>> @@ -132,38 +130,38 @@ static int ceva_init_sata(struct
>>>>> ceva_sata_priv
>>>> *priv)
>>>>> break;
>>>>>
>>>>> case CEVA_LS1021A:
>>>>> - writel(SATA_ECC_DISABLE, SATA_ECC_REG_ADDR);
>>>>> + if (ecc_addr)
>>>>> + writel(ECC_DIS_VAL_CH1, ecc_addr);
>>>>> writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG);
>>>>> writel(LS1021_CEVA_PHY2_CFG, base +
>>>> AHCI_VEND_PP2C);
>>>>> writel(LS1021_CEVA_PHY3_CFG, base +
>>>> AHCI_VEND_PP3C);
>>>>> writel(LS1021_CEVA_PHY4_CFG, base +
>>>> AHCI_VEND_PP4C);
>>>>> writel(LS1021_CEVA_PHY5_CFG, base +
>>>> AHCI_VEND_PP5C);
>>>>> writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
>>>>> - if (priv->flag & FLAG_COHERENT)
>>>>> - writel(CEVA_AXICC_CFG, base +
>>>> LS1021_AHCI_VEND_AXICC);
>>>>> break;
>>>>>
>>>>> case CEVA_LS1012A:
>>>>> case CEVA_LS1043A:
>>>>> case CEVA_LS1046A:
>>>>> - writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2);
>>>>> - /* fallthrough */
>>>>> case CEVA_LS2080A:
>>>>> + if (ecc_addr)
>>>>> + writel(ECC_DIS_VAL_CH2, ecc_addr);
>>>>> writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG);
>>>>> writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
>>>>> - if (priv->flag & FLAG_COHERENT)
>>>>> - writel(CEVA_AXICC_CFG, base +
>>>> AHCI_VEND_AXICC);
>>>>> break;
>>>>>
>>>>> + case CEVA_LS1028A:
>>>>> case CEVA_LS1088A:
>>>>> - writel(LS1088_ECC_DIS_VAL_CH2,
>>>> LS1088_ECC_DIS_ADDR_CH2);
>>>>> + if (ecc_addr)
>>>>> + writel(ECC_DIS_VAL_CH3, ecc_addr);
>>>>> writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG);
>>>>> writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
>>>>> - if (priv->flag & FLAG_COHERENT)
>>>>> - writel(CEVA_AXICC_CFG, base +
>>>> AHCI_VEND_AXICC);
>>>>> break;
>>>>> }
>>>>>
>>>>> + if (priv->flag & FLAG_COHERENT)
>>>>> + writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC);
>>>>> +
>>>>> return 0;
>>>>> }
>>>>>
>>>>> @@ -187,6 +185,7 @@ static const struct udevice_id sata_ceva_ids[] = {
>>>>> { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 },
>>>>> { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A },
>>>>> { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A },
>>>>> + { .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A },
>>>>> { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A },
>>>>> { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A },
>>>>> { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A }, @@
>>>>> -205,8 +204,16 @@ static int sata_ceva_ofdata_to_platdata(struct
>>>>> udevice
>>>> *dev)
>>>>> if (priv->base == FDT_ADDR_T_NONE)
>>>>> return -EINVAL;
>>>>>
>>>>> + priv->ecc_base = dev_read_addr_index(dev, 1);
>>>>
>>>> It would be better to do it via reg-name instead of index. But that's
>>>> up to your binding doc.
>>>>
>>> [Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file sata node as
>> fallows?
>>>
>>> diff --git a/arch/arm/dts/zynqmp.dtsi b/arch/arm/dts/zynqmp.dtsi index
>>> dfb6ebc..9ad2d84 100644
>>> --- a/arch/arm/dts/zynqmp.dtsi
>>> +++ b/arch/arm/dts/zynqmp.dtsi
>>> @@ -692,6 +692,7 @@
>>> compatible = "ceva,ahci-1v84";
>>> status = "disabled";
>>> reg = <0x0 0xfd0c0000 0x0 0x2000>;
>>> + reg-names = "serdes";
>>> interrupt-parent = <&gic>;
>>> interrupts = <0 133 4>;
>>> #stream-id-cells = <4>;
>>
>> Unfortunately it is not that simple.
>>
>> First of all you need to reflect this in dt binding doc in the kernel.
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
>> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr
>> ee%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-ceva.txt%3F
>> h%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7Cc588ddff1b1
>> e471f645b08d6c24e54d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>> 0%7C636910039050031150&sdata=0Gsuo2uI3pot4VAhBr7SQUnCRYT7Xi
>> Ybybu6i1t%2Byzo%3D&reserved=0
>>
>> Did you record your compatible strings anywhere?
>>
> [Peng Ma] Thanks your quick reply.
> You can find our binding document at: (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/ata/ahci-fsl-qoriq.txt?h=v5.1-rc5)
Interesting. Why did you create separate binding doc if you target the
same ceva sata IP core? Any reason not to merge it together?
Anyway I see that this was added in 2015.
> I think your suggestion is better. But the first register cd address was got by function dev_read_addr(), to keep consistency, I still prefer to use dev_read_addr_index(). What do you think?
We didn't have a need to have second address range that's why reg-names
property wasn't used. Do they have all ecc addresses?
M
More information about the U-Boot
mailing list