[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