>-----Original Message----- >From: Michal Simek <[email protected]> >Sent: 2019年4月17日 15:38 >To: Peng Ma <[email protected]>; Michal Simek <[email protected]>; >[email protected]; [email protected]; Fabio Estevam ><[email protected]>; York Sun <[email protected]>; Prabhakar >Kushwaha <[email protected]> >Cc: Andy Tang <[email protected]>; Yinbo Zhu <[email protected]>; >[email protected] >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 <[email protected]> >>> Sent: 2019年4月17日 14:58 >>> To: Peng Ma <[email protected]>; Michal Simek >>> <[email protected]>; [email protected]; >>> [email protected]; Fabio Estevam <[email protected]>; York Sun >>> <[email protected]>; Prabhakar Kushwaha ><[email protected]> >>> Cc: Andy Tang <[email protected]>; Yinbo Zhu <[email protected]>; >>> [email protected] >>> 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 <[email protected]> >>>>> Sent: 2019年4月17日 13:58 >>>>> To: Peng Ma <[email protected]>; Michal Simek >>>>> <[email protected]>; [email protected]; >>>>> [email protected]; Fabio Estevam <[email protected]>; York Sun >>>>> <[email protected]>; Prabhakar Kushwaha >>> <[email protected]> >>>>> Cc: Andy Tang <[email protected]>; Yinbo Zhu <[email protected]>; >>>>> [email protected] >>>>> 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 <[email protected]> >>>>>>> Sent: 2019年4月16日 18:49 >>>>>>> To: Peng Ma <[email protected]>; Michal Simek >>>>>>> <[email protected]>; [email protected]; >>>>>>> [email protected]; Fabio Estevam <[email protected]>; York >Sun >>>>>>> <[email protected]>; Prabhakar Kushwaha >>>>> <[email protected]> >>>>>>> Cc: Andy Tang <[email protected]>; Yinbo Zhu ><[email protected]>; >>>>>>> [email protected] >>>>>>> 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 <[email protected]> >>>>>>>>> Sent: 2019年4月16日 18:01 >>>>>>>>> To: Peng Ma <[email protected]>; Michal Simek >>>>>>>>> <[email protected]>; [email protected]; >>>>>>>>> [email protected]; Fabio Estevam <[email protected]>; York >>> Sun >>>>>>>>> <[email protected]>; Prabhakar Kushwaha >>>>>>> <[email protected]> >>>>>>>>> Cc: Andy Tang <[email protected]>; Yinbo Zhu >>> <[email protected]>; >>>>>>>>> [email protected] >>>>>>>>> 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 <[email protected]> >>>>>>>>>>> Sent: 2019年4月16日 17:31 >>>>>>>>>>> To: Peng Ma <[email protected]>; Michal Simek >>>>>>>>>>> <[email protected]>; [email protected]; >>>>>>>>>>> [email protected]; Fabio Estevam <[email protected]>; >York >>>>> Sun >>>>>>>>>>> <[email protected]>; Prabhakar Kushwaha >>>>>>>>> <[email protected]> >>>>>>>>>>> Cc: Andy Tang <[email protected]>; Yinbo Zhu >>>>> <[email protected]>; >>>>>>>>>>> [email protected] >>>>>>>>>>> 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 <[email protected]> >>>>>>>>>>>>> Sent: 2019年4月16日 16:04 >>>>>>>>>>>>> To: Peng Ma <[email protected]>; >[email protected]; >>>>>>>>>>>>> [email protected]; Fabio Estevam <[email protected]>; >>> York >>>>>>> Sun >>>>>>>>>>>>> <[email protected]>; Prabhakar Kushwaha >>>>>>>>>>> <[email protected]> >>>>>>>>>>>>> Cc: Andy Tang <[email protected]>; Yinbo Zhu >>>>>>> <[email protected]>; >>>>>>>>>>>>> [email protected]; [email protected] >>>>>>>>>>>>> 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 <[email protected]> >>>>>>>>>>>>>> --- >>>>>>>>>>>>>> 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
