[U-Boot] [PATCH] keystone2: use appropriate HD field for destination port
Ivan Khoronzhuk
ivan.khoronzhuk at linaro.org
Wed Jul 8 22:42:04 CEST 2015
On 08.07.15 22:22, ivan.khoronzhuk wrote:
> Vitaly,
>
> On 08.07.15 21:28, Vitaly Andrianov wrote:
>>
>>
>> On 07/08/2015 01:50 PM, ivan.khoronzhuk wrote:
>>> Vitaly,
>>>
>>> On 08.07.15 20:26, Vitaly Andrianov wrote:
>>>>
>>>>
>>>> On 07/08/2015 01:05 PM, Ivan Khoronzhuk wrote:
>>>>> Vitaly,
>>>>>
>>>>> On 08.07.15 20:05, Vitaly Andrianov wrote:
>>>>>>
>>>>>>
>>>>>> On 07/08/2015 12:38 PM, Ivan Khoronzhuk wrote:
>>>>>>> Hi, Vitaly
>>>>>>>
>>>>>>> I suppose it's better to decide in upper driver how to use swinfo
>>>>>>> field.
>>>>>>> Like in drivers/net/keystone_net.c
>>>>>>>
>>>>>>> The keystone navigator supposed to be used as a tool for
>>>>>>> communicating
>>>>>>> between different IPs, and each of them decide how to use swinfo
>>>>>>> fields.
>>>>>>> It's protocol specific information and should be known only in
>>>>>>> sending
>>>>>>> parts. What if tomorrow you will decide to send some packet to PA?,
>>>>>>> you
>>>>>>> will rewrite this function again?
>>>>>>>
>>>>>>> It's not the place for such kind information.
>>>>>>>
>>>>>>> Even more, this is the h/w specific decision and no need to check
>>>>>>> this
>>>>>>> for each sent packet. You better statically assign how to use this
>>>>>>> field
>>>>>>> depending on h/w revision, using #if.
>>>>>>>
>>>>>>> On 08.07.15 18:45, Vitaly Andrianov wrote:
>>>>>>>> K2L and L2E have different from K2HK EthSS version, which uses
>>>>>>>> tag_info
>>>>>>>> field for destination slave port. This commit adds the
>>>>>>>> dest_port_info
>>>>>>>> field
>>>>>>>> to the struct pktdma_cfg, to configure which HD filed tag_info or
>>>>>>>> pkt_info
>>>>>>>> shall be used to configure descriptor.
>>>>>>>>
>>>>>>>> Before that commit the swinfo[2] was used for that purpose. Even if
>>>>>>>> that
>>>>>>>> worked on K2HK devices, the correct field for K2HK is the pkt_info.
>>>>>>>>
>>>>>>>> The netcp_send() configure appropriate HD info field depending on
>>>>>>>> the
>>>>>>>> direct_info of the currently using netcp.
>>>>>>>>
>>>>>>>> Signed-off-by: Vitaly Andrianov <vitalya at ti.com>
>>>>>>>> Acked-by: Murali Karicheri <m-karicheri2 at ti.com>
>>>>>>>> ---
>>>>>>>> arch/arm/include/asm/ti-common/keystone_nav.h | 9 ++++++++-
>>>>>>>> drivers/dma/keystone_nav.c | 12 ++++++++++--
>>>>>>>> drivers/net/keystone_net.c | 3 +--
>>>>>>>> 3 files changed, 19 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/include/asm/ti-common/keystone_nav.h
>>>>>>>> b/arch/arm/include/asm/ti-common/keystone_nav.h
>>>>>>>> index 696d8c6..5a0e391 100644
>>>>>>>> --- a/arch/arm/include/asm/ti-common/keystone_nav.h
>>>>>>>> +++ b/arch/arm/include/asm/ti-common/keystone_nav.h
>>>>>>>> @@ -152,6 +152,11 @@ struct rx_flow_regs {
>>>>>>>> u32 thresh[3];
>>>>>>>> };
>>>>>>>>
>>>>>>>> +enum dest_port_info {
>>>>>>>> + PKT_INFO,
>>>>>>>> + TAG_INFO
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> struct pktdma_cfg {
>>>>>>>> struct global_ctl_regs *global;
>>>>>>>> struct tx_chan_regs *tx_ch;
>>>>>>>> @@ -167,6 +172,7 @@ struct pktdma_cfg {
>>>>>>>> u32 tx_snd_q;
>>>>>>>>
>>>>>>>> u32 rx_flow; /* flow that is used for RX */
>>>>>>>> + enum dest_port_info dest_port_info;/* HD fiels for dest
>>>>>>>> port
>>>>>>>> bits */
>>>>>>>> };
>>>>>>>>
>>>>>>>> extern struct pktdma_cfg netcp_pktdma;
>>>>>>>> @@ -184,7 +190,8 @@ struct rx_buff_desc {
>>>>>>>>
>>>>>>>> int ksnav_close(struct pktdma_cfg *pktdma);
>>>>>>>> int ksnav_init(struct pktdma_cfg *pktdma, struct rx_buff_desc
>>>>>>>> *rx_buffers);
>>>>>>>> -int ksnav_send(struct pktdma_cfg *pktdma, u32 *pkt, int num_bytes,
>>>>>>>> u32 swinfo2);
>>>>>>>> +int ksnav_send(struct pktdma_cfg *pktdma, u32 *pkt, int num_bytes,
>>>>>>>> + u32 dest_port);
>>>>>>>> void *ksnav_recv(struct pktdma_cfg *pktdma, u32 **pkt, int
>>>>>>>> *num_bytes);
>>>>>>>> void ksnav_release_rxhd(struct pktdma_cfg *pktdma, void *hd);
>>>>>>>>
>>>>>>>> diff --git a/drivers/dma/keystone_nav.c
>>>>>>>> b/drivers/dma/keystone_nav.c
>>>>>>>> index dfca75a..64b1cee 100644
>>>>>>>> --- a/drivers/dma/keystone_nav.c
>>>>>>>> +++ b/drivers/dma/keystone_nav.c
>>>>>>>> @@ -278,7 +278,8 @@ int ksnav_close(struct pktdma_cfg *pktdma)
>>>>>>>> return QM_OK;
>>>>>>>> }
>>>>>>>>
>>>>>>>> -int ksnav_send(struct pktdma_cfg *pktdma, u32 *pkt, int num_bytes,
>>>>>>>> u32 swinfo2)
>>>>>>>> +int ksnav_send(struct pktdma_cfg *pktdma, u32 *pkt, int num_bytes,
>>>>>>>> + u32 dest_port)
>>>>>>>> {
>>>>>>>> struct qm_host_desc *hd;
>>>>>>>>
>>>>>>>> @@ -286,8 +287,15 @@ int ksnav_send(struct pktdma_cfg *pktdma, u32
>>>>>>>> *pkt, int num_bytes, u32 swinfo2)
>>>>>>>> if (hd == NULL)
>>>>>>>> return QM_ERR;
>>>>>>>>
>>>>>>>> + dest_port &= 0xf;
>>>>>>>> hd->desc_info = num_bytes;
>>>>>>>> - hd->swinfo[2] = swinfo2;
>>>>>>>> + if (pktdma->dest_port_info == PKT_INFO) {
>>>>>>>> + hd->packet_info = qm_cfg->qpool_num | (dest_port <<
>>>>>>>> 16);
>>>>>>>> + } else {
>>>>>>>> + hd->packet_info = qm_cfg->qpool_num;
>>>>>>>> + hd->tag_info = dest_port;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> hd->packet_info = qm_cfg->qpool_num;
>>>>>>>>
>>>>>>>> qm_buff_push(hd, pktdma->tx_snd_q, pkt, num_bytes);
>>>>>>>> diff --git a/drivers/net/keystone_net.c
>>>>>>>> b/drivers/net/keystone_net.c
>>>>>>>> index 0c5fdee..e2adb67 100644
>>>>>>>> --- a/drivers/net/keystone_net.c
>>>>>>>> +++ b/drivers/net/keystone_net.c
>>>>>>>> @@ -381,8 +381,7 @@ int32_t cpmac_drv_send(u32 *buffer, int
>>>>>>>> num_bytes,
>>>>>>>> int slave_port_num)
>>>>>>>> if (num_bytes < EMAC_MIN_ETHERNET_PKT_SIZE)
>>>>>>>> num_bytes = EMAC_MIN_ETHERNET_PKT_SIZE;
>>>>>>>>
>>>>>>>> - return ksnav_send(&netcp_pktdma, buffer,
>>>>>>>> - num_bytes, (slave_port_num) << 16);
>>>>>>>> + return ksnav_send(&netcp_pktdma, buffer, num_bytes,
>>>>>>>> slave_port_num);
>>>>>>>> }
>>>>>>>>
>>>>>>>> /* Eth device open */
>>>>>>>>
>>>>>>>
>>>>>> Hi Ivan,
>>>>>>
>>>>>> I agree with you. And probably we will need to implement your
>>>>>> proposal
>>>>>> in future commits. This commit is to fix the bug, which is in
>>>>>> existing
>>>>>> driver.
>>>>>>
>>>>>> Thanks,
>>>>>> Vitaly
>>>>>
>>>>> Sorry, I supposed that first msg was not sent.
>>>>> It's better to fix it in drivers/net/keystone_net.c
>>>>>
>>>>>
>>>> Ivan,
>>>>
>>>> I don't understand your proposal. The network driver doesn't know
>>>> anything about CPPI DMA engine internals. That is the navigator's
>>>> job to
>>>> fill appropriate packet BD fields and it only takes care about HW
>>>> version differences.
>>>
>>> You have Eth h/w, you have pktDMA engine to communicate through, you
>>> have network driver that knows how to use pktDMA to communicate with
>>> Ethernet switch.
>>> CPII DMA engine can be used to communicate with different h/w, not only
>>> with Ethernet switch. This can be used to send packets to PA, to SA, to
>>> QMSS PDSPs, to PA PDSPs and others parts, I don't talk even about AIF,
>>> SRIO, FFTC and others IPs that have Pktdma on board.
>>>
>>> Each of them can use swinfo for different purposes.
>>>
>>> In the chain to communicate directly with Eth you use net driver
>>> in the way keystone_net -> keystone_nav -> h/w pktdma -> eht
>>> You can use it also like keystone_srio -> keystone_nav -> h/w pktdma ->
>>> srio.
>>> etc.
>>>
>>> pktdma engine it's like a bus used to communicate with a h/w. So, it's
>>> not correct to fix driver specific issues in the bus s/w, that is used
>>> by different drivers.
>>>
>> OK. What is your proposal? How to fix the issue in the keystone_net.c,
>> which doesn't have any idea about structure of buffer descriptors. And
>> actually doesn't have to know about it.
>>
>> The proposed patch fixes the bug of existing upstreamed driver and allow
>> K2E and K2L users to work. It is fully tested on all supported devices.
>>
>> The PA PDSP and SA drivers currently doesn't exist in u-boot and I don't
>> have any information whether anybody is going to develop them in the
>> nearest future.
>>
>
> It doesn't matter exist those drivers or not. It's a structured model.
> If, for instance, you are going to fix some errata in I2C RTC, what
> will you do? fix it in I2C driver? But better place is RTC driver.
>
> It's partly an API issue. It should be allowed to assign protocol
> specific flags field in packet_info and allow others to use swinfo in
> the same time.
>
> It be better to add to pktdma_config field like protocol_flags.
>
> in keystone_nav send function do:
> hd->packet_info = qm_cfg->qpool_num |
> (qm_cfg->protocol_flags) & 0xf << 16;
>
>
> and in keystone_net, statically:
> #if (REVISION == REVISION_WITH_BUG)
> qm_cfg->protocol_flags = dest_port;
> #else
> qm_cfg->protocol_flags = 0;
>
> But I'm not sure about one thing. Do you have revisions that use swinfo
> for port number and cannot use packet_info for this goal? In this case
> you should support version when software field is used also.
>
Or even better to add new structure like pkt_data with vars:
struct pkt_data {
int protocol_flags;
u32 tags;
u32 swinfo[2];
char *data; (that is pkt)
int num_bytes;
};
and
int ksnav_send(struct pktdma_cfg *pktdma, struct pkt_data *pkt)
{
....
hd->packet_info = qm_cfg->qpool_num |
((pkt->protocol_flags) & 0xf) << 16;
hd->tag_info = tags;
....
}
and in keystone_net, statically:
#if (REVISION == REVISION_WITH_BUG)
pkt->protocol_flags = dest_port;
#else
pkt->protocol_flags = 0;
pkt->tags = dest_port;
In this case each driver will be doing what it's allowed to.
It can be done with two patches.
>>>>
>>>> Thanks,
>>>> Vitaly
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot at lists.denx.de
>>>> http://lists.denx.de/mailman/listinfo/u-boot
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
More information about the U-Boot
mailing list