[U-Boot] [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
Peng Ma
peng.ma at nxp.com
Tue Apr 16 10:29:33 UTC 2019
>-----Original Message-----
>From: Michal Simek <michal.simek at xilinx.com>
>Sent: 2019年4月16日 18:01
>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: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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
>> .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git
>%
>>
>2Ftree%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-fsl-qoriq.
>>
>txt%3Fh%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7C977d
>7871292f
>>
>48332dbb08d6c2526b32%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>0%7C6369
>>
>10056581930082&sdata=KQryE2RvI8jTOAPKGlGnx2NWYdawFUaTURyzd8
>oets4%3
>> D&reserved=0)
>
>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.
>
[Peng Ma] Although they are both ceva sata, our sata driver is different from yours in kernel to
Initializa Vendor-specific registers, you could see our driver at kernel driver/ata/ ahci_qoriq.c.
>> 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?
>
[Peng Ma] Some of our socs need to fixed sata errata. So we should write ecc addr.
I didn't add reg-names to keep consistency of Xilinx sata soc.
>M
More information about the U-Boot
mailing list