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