[U-Boot] [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
Michal Simek
michal.simek at xilinx.com
Wed Apr 17 07:38:14 UTC 2019
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.
M
More information about the U-Boot
mailing list