>-----Original Message----- >From: Michal Simek <michal.si...@xilinx.com> >Sent: 2019年4月17日 13:58 >To: Peng Ma <peng...@nxp.com>; Michal Simek <michal.si...@xilinx.com>; >albert.u.b...@aribaud.net; s...@chromium.org; Fabio Estevam ><fabio.este...@nxp.com>; York Sun <york....@nxp.com>; Prabhakar >Kushwaha <prabhakar.kushw...@nxp.com> >Cc: Andy Tang <andy.t...@nxp.com>; Yinbo Zhu <yinbo....@nxp.com>; >u-boot@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.si...@xilinx.com> >>> Sent: 2019年4月16日 18:49 >>> To: Peng Ma <peng...@nxp.com>; Michal Simek >>> <michal.si...@xilinx.com>; albert.u.b...@aribaud.net; >>> s...@chromium.org; Fabio Estevam <fabio.este...@nxp.com>; York Sun >>> <york....@nxp.com>; Prabhakar Kushwaha ><prabhakar.kushw...@nxp.com> >>> Cc: Andy Tang <andy.t...@nxp.com>; Yinbo Zhu <yinbo....@nxp.com>; >>> u-boot@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.si...@xilinx.com> >>>>> Sent: 2019年4月16日 18:01 >>>>> To: Peng Ma <peng...@nxp.com>; Michal Simek >>>>> <michal.si...@xilinx.com>; albert.u.b...@aribaud.net; >>>>> s...@chromium.org; Fabio Estevam <fabio.este...@nxp.com>; York Sun >>>>> <york....@nxp.com>; Prabhakar Kushwaha >>> <prabhakar.kushw...@nxp.com> >>>>> Cc: Andy Tang <andy.t...@nxp.com>; Yinbo Zhu <yinbo....@nxp.com>; >>>>> u-boot@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.si...@xilinx.com> >>>>>>> Sent: 2019年4月16日 17:31 >>>>>>> To: Peng Ma <peng...@nxp.com>; Michal Simek >>>>>>> <michal.si...@xilinx.com>; albert.u.b...@aribaud.net; >>>>>>> s...@chromium.org; Fabio Estevam <fabio.este...@nxp.com>; York >Sun >>>>>>> <york....@nxp.com>; Prabhakar Kushwaha >>>>> <prabhakar.kushw...@nxp.com> >>>>>>> Cc: Andy Tang <andy.t...@nxp.com>; Yinbo Zhu ><yinbo....@nxp.com>; >>>>>>> u-boot@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.si...@xilinx.com> >>>>>>>>> Sent: 2019年4月16日 16:04 >>>>>>>>> To: Peng Ma <peng...@nxp.com>; albert.u.b...@aribaud.net; >>>>>>>>> s...@chromium.org; Fabio Estevam <fabio.este...@nxp.com>; York >>> Sun >>>>>>>>> <york....@nxp.com>; Prabhakar Kushwaha >>>>>>> <prabhakar.kushw...@nxp.com> >>>>>>>>> Cc: Andy Tang <andy.t...@nxp.com>; Yinbo Zhu >>> <yinbo....@nxp.com>; >>>>>>>>> michal.si...@xilinx.com; u-boot@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...@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%2Flin >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); Best Regards, Peng >Thanks, >Michal _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot