>-----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%2Fg >>>>> 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%2Fg >>>> it >>>> .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux. >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; + 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?
Best Regards, Peng > >Thanks, >Michal _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
