[U-Boot] [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code

Peng Ma peng.ma at nxp.com
Wed Apr 17 07:40:14 UTC 2019



>-----Original Message-----
>From: Michal Simek <michal.simek at xilinx.com>
>Sent: 2019年4月17日 15:38
>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 17. 04. 19 9:27, Peng Ma wrote:
>>
>>
>>> -----Original Message-----
>>> From: Michal Simek <michal.simek at xilinx.com>
>>> Sent: 2019年4月17日 14:58
>>> 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 17. 04. 19 8:50, Peng Ma wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Michal Simek <michal.simek at xilinx.com>
>>>>> Sent: 2019年4月17日 13:58
>>>>> 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 17. 04. 19 4:50, Peng Ma wrote:
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Michal Simek <michal.simek at xilinx.com>
>>>>>>> Sent: 2019年4月16日 18:49
>>>>>>> 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 12:29, Peng Ma wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> -----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
>>>>>>>>>>> %2
>>>>>>>>>>> Fg
>>>>>>>>>>> it
>>>>>>>>>>> .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
>>>>>>>>>> %2
>>>>>>>>>> Fg
>>>>>>>>>> it
>>>>>>>>>> .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%
>2F
>>> lin
>>>>> ux.
>>>>>>> gi
>>>>>>>>>> t
>>>>>>>>> %
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>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 have briefly looked at it. They should be merged together. It
>>>>>>> happened to us too that we send driver out but didn't know who
>>>>>>> was the vendor in past and then we found out.
>>>>>>>
>>>>>>>>>> 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.
>>>>>>>
>>>>>>> Ok. I think that in this case you should pass different .data to driver.
>>>>>>>
>>>>>>> struct platform_data {
>>>>>>>        enum ceva_soc;
>>>>>>>        bool ecc_present;
>>>>>>> }
>>>>>>>
>>>>>>> It means have current enum followed by bool with "true" for
>>>>>>> specific IPs which needs to have also ECC range.
>>>>>>> Then you can better check that binding is correct.
>>>>>>>
>>>>>>> And use _index there.
>>>>>> [Peng Ma] As far as I am concerned, It is not necessary to do
>>>>>> this, I would rather lean on the former way as fallows:
>>>>>> diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c
>>>>>> index d26f712..a1d452a 100644
>>>>>> --- a/drivers/ata/sata_ceva.c
>>>>>> +++ b/drivers/ata/sata_ceva.c
>>>>>> @@ -196,6 +196,10 @@ static const struct udevice_id
>>>>>> sata_ceva_ids[] = {  static int sata_ceva_ofdata_to_platdata(struct
>udevice *dev)  {
>>>>>>         struct ceva_sata_priv *priv = dev_get_priv(dev);
>>>>>> +       const void *blob = gd->fdt_blob;
>>>>>
>>>>> gd is suggesting that you use incorrect functions.
>>>>> We are trying to get rid of gd from device drivers. Please take a
>>>>> look at live tree functions if there is any alternative there.
>>>>>
>>>>>
>>>>>> +       int node = dev_of_offset(dev);
>>>>>> +       struct fdt_resource res_regs;
>>>>>> +       int ret;
>>>>>>
>>>>>>         if (dev_read_bool(dev, "dma-coherent"))
>>>>>>                 priv->flag |= FLAG_COHERENT; @@ -204,9
>+208,13
>>> @@
>>>>>> 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);
>>>>>> -       if (priv->ecc_base == FDT_ADDR_T_NONE)
>>>>>> +       ret = fdt_get_named_resource(blob, node, "reg",
>"reg-names",
>>>>>> +                                    "ecc-addr", &res_regs);
>>>>>> +       if (ret) {
>>>>>> +               debug("Error: can't get ecc addresses(ret =
>>>>>> + %d)!\n", ret);
>>>>>>                 priv->ecc_base = 0;
>>>>>> +       } else
>>>>>> +               priv->ecc_base = res_regs.start;
>>>>>>
>>>>>>         priv->soc = dev_get_driver_data(dev); What do you think?
>>>>>
>>>>> One thing I would like to avoid is that we will get any error even
>>>>> in debug for IPs which don't have this ecc space.
>>>>> And also that we will get this errors for IPs which should have
>>>>> this ecc space.
>>>>>
>>>> [Peng Ma] OK,changed as fallows:
>>>>
>>>> diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index
>>>> d26f712..bd98d85 100644
>>>> --- a/drivers/ata/sata_ceva.c
>>>> +++ b/drivers/ata/sata_ceva.c
>>>> @@ -8,6 +8,7 @@
>>>>  #include <ahci.h>
>>>>  #include <scsi.h>
>>>>  #include <asm/io.h>
>>>> +#include <linux/ioport.h>
>>>>
>>>>  /* Vendor Specific Register Offsets */  #define AHCI_VEND_PCFG
>>>> 0xA4 @@ -196,6 +197,8 @@ static const struct udevice_id
>>>> sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice
>*dev)  {
>>>>         struct ceva_sata_priv *priv = dev_get_priv(dev);
>>>> +       struct resource res_regs;
>>>> +       int ret;
>>>>
>>>>         if (dev_read_bool(dev, "dma-coherent"))
>>>>                 priv->flag |= FLAG_COHERENT; @@ -204,9 +207,11
>>> @@
>>>> 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);
>>>> -       if (priv->ecc_base == FDT_ADDR_T_NONE)
>>>> +       ret = dev_read_resource_byname(dev, "ecc-addr", &res_regs);
>>>> +       if (ret)
>>>>                 priv->ecc_base = 0;
>>>> +       else
>>>> +               priv->ecc_base = res_regs.start;
>>>>
>>>>         priv->soc = dev_get_driver_data(dev);
>>>
>>> This looks good but still missing information that in your case there
>>> are some versions where this ecc-addr is required. It means if it is
>>> required you should error it out when it is not present in DT.
>>>
>> [Peng Ma] Ok, It will return error when some socs need ecc address
>> with the regs is not Present in DT.
>>
>> diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index
>> d26f712..6e28a38 100644
>> --- a/drivers/ata/sata_ceva.c
>> +++ b/drivers/ata/sata_ceva.c
>> @@ -8,6 +8,7 @@
>>  #include <ahci.h>
>>  #include <scsi.h>
>>  #include <asm/io.h>
>> +#include <linux/ioport.h>
>>
>>  /* Vendor Specific Register Offsets */  #define AHCI_VEND_PCFG  0xA4
>> @@ -130,8 +131,9 @@ static int ceva_init_sata(struct ceva_sata_priv *priv)
>>                 break;
>>
>>         case CEVA_LS1021A:
>> -               if (ecc_addr)
>> -                       writel(ECC_DIS_VAL_CH1, ecc_addr);
>> +               if (!ecc_addr)
>> +                       return -EINVAL;
>> +               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);
>> @@ -143,17 +145,19 @@ static int ceva_init_sata(struct ceva_sata_priv
>*priv)
>>         case CEVA_LS1012A:
>>         case CEVA_LS1043A:
>>         case CEVA_LS1046A:
>> +               if (!ecc_addr)
>> +                       return -EINVAL;
>> +               writel(ECC_DIS_VAL_CH2, ecc_addr);
>>         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);
>>                 break;
>>
>>         case CEVA_LS1028A:
>>         case CEVA_LS1088A:
>> -               if (ecc_addr)
>> -                       writel(ECC_DIS_VAL_CH3, ecc_addr);
>> +               if (!ecc_addr)
>> +                       return -EINVAL;
>> +               writel(ECC_DIS_VAL_CH3, ecc_addr);
>>                 writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG);
>>                 writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
>>                 break;
>> @@ -196,6 +200,8 @@ static const struct udevice_id sata_ceva_ids[] = {
>> static int sata_ceva_ofdata_to_platdata(struct udevice *dev)  {
>>         struct ceva_sata_priv *priv = dev_get_priv(dev);
>> +       struct resource res_regs;
>> +       int ret;
>>
>>         if (dev_read_bool(dev, "dma-coherent"))
>>                 priv->flag |= FLAG_COHERENT; @@ -204,9 +210,11
>@@
>> 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);
>> -       if (priv->ecc_base == FDT_ADDR_T_NONE)
>> +       ret = dev_read_resource_byname(dev, "ecc-addr", &res_regs);
>> +       if (ret)
>>                 priv->ecc_base = 0;
>> +       else
>> +               priv->ecc_base = res_regs.start;
>>
>>         priv->soc = dev_get_driver_data(dev);
>>
>
>ok. Looks good.
>
[Peng Ma] Thanks very much for your patient guidance.

Best Regards,
Peng
>M



More information about the U-Boot mailing list