diff mbox series

[U-Boot,v3,5/5] ARM: dm: spi: Add support DM/DTS for i.MX28 mxs SPI driver (DM_SPI conversion)

Message ID 20190615223442.12246-6-lukma@denx.de
State Superseded
Delegated to: Stefano Babic
Headers show
Series DM: Convert i.MX28 gpio, pinmux, spi and eth drivers to DM/DTS | expand

Commit Message

Lukasz Majewski June 15, 2019, 10:34 p.m. UTC
This commit converts mxs_spi driver to support DM/DTS.

Signed-off-by: Lukasz Majewski <lukma@denx.de>

---

Changes in v3:
- Set more apropriate tags

Changes in v2:
- New patch (conversion of mxs_spi.c to DM_SPI)

 drivers/spi/mxs_spi.c | 393 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 310 insertions(+), 83 deletions(-)

Comments

Marek Vasut June 15, 2019, 11:02 p.m. UTC | #1
On 6/16/19 12:34 AM, Lukasz Majewski wrote:
> This commit converts mxs_spi driver to support DM/DTS.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>

Is the non-DM part needed for anything ? I recall the SPL jumps back to
BootROM when loading the U-Boot proper. So if not, drop it.

Also, please don't do partial conversion for iMX28 only, do the iMX23
part as well, it cannot be hard.
Lukasz Majewski June 17, 2019, 6:49 a.m. UTC | #2
Hi Marek,

> On 6/16/19 12:34 AM, Lukasz Majewski wrote:
> > This commit converts mxs_spi driver to support DM/DTS.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>  
> 
> Is the non-DM part needed for anything ? 

Do you mean the non-DM part of the mxs_gpio driver? Yes, it is used by
not converted boards.

> I recall the SPL jumps back
> to BootROM when loading the U-Boot proper. So if not, drop it.
> 
> Also, please don't do partial conversion for iMX28 only, do the iMX23
> part as well, it cannot be hard.

Maybe it is not hard, but I cannot test it properly as I don't have
i.MX23 device. If you are offering your help with testing (i.e. you do
have the access to i.MX23 device and you will test those changes) I can
add support for it.

Otherwise, NO, I will not add ANY new untested code.

> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Marek Vasut June 17, 2019, 10:06 a.m. UTC | #3
On 6/17/19 8:49 AM, Lukasz Majewski wrote:
> Hi Marek,
> 
>> On 6/16/19 12:34 AM, Lukasz Majewski wrote:
>>> This commit converts mxs_spi driver to support DM/DTS.
>>>
>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>  
>>
>> Is the non-DM part needed for anything ? 
> 
> Do you mean the non-DM part of the mxs_gpio driver? Yes, it is used by
> not converted boards.

This is a SPI driver though.

>> I recall the SPL jumps back
>> to BootROM when loading the U-Boot proper. So if not, drop it.
>>
>> Also, please don't do partial conversion for iMX28 only, do the iMX23
>> part as well, it cannot be hard.
> 
> Maybe it is not hard, but I cannot test it properly as I don't have
> i.MX23 device. If you are offering your help with testing (i.e. you do
> have the access to i.MX23 device and you will test those changes) I can
> add support for it.
> 
> Otherwise, NO, I will not add ANY new untested code.

In general, you don't have to add any code, the iMX23/28 SPI IP is very
much the same hardware, DTTO for most of the other blocks. If there are
any differences between iMX23/28, they are already handled in the
existing driver(s).

Half-way converted drivers in fact increase maintenance burden, because
then we have to deal with two different variants of the code, instead of
only one. That's why I would like to see this practice go away wherever
possible, and in this case it is possible.

If you need someone to test your changes, CC the board maintainers,
that's standard practice. If they don't respond, that cannot be helped.
Lukasz Majewski June 17, 2019, 12:27 p.m. UTC | #4
Hi Marek,

> On 6/17/19 8:49 AM, Lukasz Majewski wrote:
> > Hi Marek,
> >   
> >> On 6/16/19 12:34 AM, Lukasz Majewski wrote:  
> >>> This commit converts mxs_spi driver to support DM/DTS.
> >>>
> >>> Signed-off-by: Lukasz Majewski <lukma@denx.de>    
> >>
> >> Is the non-DM part needed for anything ?   
> > 
> > Do you mean the non-DM part of the mxs_gpio driver? Yes, it is used
> > by not converted boards.  
> 
> This is a SPI driver though.
> 
> >> I recall the SPL jumps back
> >> to BootROM when loading the U-Boot proper. So if not, drop it.
> >>
> >> Also, please don't do partial conversion for iMX28 only, do the
> >> iMX23 part as well, it cannot be hard.  
> > 
> > Maybe it is not hard, but I cannot test it properly as I don't have
> > i.MX23 device. If you are offering your help with testing (i.e. you
> > do have the access to i.MX23 device and you will test those
> > changes) I can add support for it.
> > 
> > Otherwise, NO, I will not add ANY new untested code.  
> 
> In general, you don't have to add any code, the iMX23/28 SPI IP is
> very much the same hardware, DTTO for most of the other blocks. If
> there are any differences between iMX23/28, they are already handled
> in the existing driver(s).
> 
> Half-way converted drivers in fact increase maintenance burden,
> because then we have to deal with two different variants of the code,
> instead of only one.

I cannot agree with this sentence. The conversion would be done for
i.MX28, which is then tested and validated (and clearly stated in the
cover letter/commit message that only supported was i.MX28).

If I don't need to adjust common, reused code (which already supports
both variants as it is the case with mxs_spi.c), then I don't mind.

For more intrusive changes - the driver needs to be tested and
validated (by somebody who has the HW for testing).

> That's why I would like to see this practice go
> away wherever possible, and in this case it is possible.

In this particular case it is possible to add support for both as SoC
specific changes (i.MX23 vs i.MX28) is performed in common code (e.g.
mxs_spi_xfer_dma).


> 
> If you need someone to test your changes, CC the board maintainers,
> that's standard practice.

As fair as I remember only Angelo and Michael had also interest in
testing converted code for i.MX28 based board.

There was NO reply from other people when this (and few others) driver
was marked as DEPRECATED.

> If they don't respond, that cannot be
> helped.
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Marek Vasut June 17, 2019, 1:23 p.m. UTC | #5
On 6/17/19 2:27 PM, Lukasz Majewski wrote:
> Hi Marek,
> 
>> On 6/17/19 8:49 AM, Lukasz Majewski wrote:
>>> Hi Marek,
>>>   
>>>> On 6/16/19 12:34 AM, Lukasz Majewski wrote:  
>>>>> This commit converts mxs_spi driver to support DM/DTS.
>>>>>
>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>    
>>>>
>>>> Is the non-DM part needed for anything ?   
>>>
>>> Do you mean the non-DM part of the mxs_gpio driver? Yes, it is used
>>> by not converted boards.  
>>
>> This is a SPI driver though.
>>
>>>> I recall the SPL jumps back
>>>> to BootROM when loading the U-Boot proper. So if not, drop it.
>>>>
>>>> Also, please don't do partial conversion for iMX28 only, do the
>>>> iMX23 part as well, it cannot be hard.  
>>>
>>> Maybe it is not hard, but I cannot test it properly as I don't have
>>> i.MX23 device. If you are offering your help with testing (i.e. you
>>> do have the access to i.MX23 device and you will test those
>>> changes) I can add support for it.
>>>
>>> Otherwise, NO, I will not add ANY new untested code.  
>>
>> In general, you don't have to add any code, the iMX23/28 SPI IP is
>> very much the same hardware, DTTO for most of the other blocks. If
>> there are any differences between iMX23/28, they are already handled
>> in the existing driver(s).
>>
>> Half-way converted drivers in fact increase maintenance burden,
>> because then we have to deal with two different variants of the code,
>> instead of only one.
> 
> I cannot agree with this sentence.

Do you think maintaining - one DM driver which supports both iMX23 and
iMX28 - is more burden than maintaining - one driver which supports DM,
but only for iMX28 and non-DM for iMX23 and iMX28 ? I don't think so.

> The conversion would be done for
> i.MX28, which is then tested and validated (and clearly stated in the
> cover letter/commit message that only supported was i.MX28).>
> If I don't need to adjust common, reused code (which already supports
> both variants as it is the case with mxs_spi.c), then I don't mind.

Well, that is what I said above, you don't.

> For more intrusive changes - the driver needs to be tested and
> validated (by somebody who has the HW for testing).

That's up to board maintainers.

>> That's why I would like to see this practice go
>> away wherever possible, and in this case it is possible.
> 
> In this particular case it is possible to add support for both as SoC
> specific changes (i.MX23 vs i.MX28) is performed in common code (e.g.
> mxs_spi_xfer_dma).

Both SPI and DMA blocks are basically the same on iMX23 and iMX28.

>> If you need someone to test your changes, CC the board maintainers,
>> that's standard practice.
> 
> As fair as I remember only Angelo and Michael had also interest in
> testing converted code for i.MX28 based board.
> 
> There was NO reply from other people when this (and few others) driver
> was marked as DEPRECATED.

Well, too bad, clearly the interest in this platform is low.
That does not mean we should do sub-par upstream work, does it ?
Lukasz Majewski June 17, 2019, 1:41 p.m. UTC | #6
On Mon, 17 Jun 2019 15:23:55 +0200
Marek Vasut <marex@denx.de> wrote:

> On 6/17/19 2:27 PM, Lukasz Majewski wrote:
> > Hi Marek,
> >   
> >> On 6/17/19 8:49 AM, Lukasz Majewski wrote:  
> >>> Hi Marek,
> >>>     
> >>>> On 6/16/19 12:34 AM, Lukasz Majewski wrote:    
> >>>>> This commit converts mxs_spi driver to support DM/DTS.
> >>>>>
> >>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>      
> >>>>
> >>>> Is the non-DM part needed for anything ?     
> >>>
> >>> Do you mean the non-DM part of the mxs_gpio driver? Yes, it is
> >>> used by not converted boards.    
> >>
> >> This is a SPI driver though.
> >>  
> >>>> I recall the SPL jumps back
> >>>> to BootROM when loading the U-Boot proper. So if not, drop it.
> >>>>
> >>>> Also, please don't do partial conversion for iMX28 only, do the
> >>>> iMX23 part as well, it cannot be hard.    
> >>>
> >>> Maybe it is not hard, but I cannot test it properly as I don't
> >>> have i.MX23 device. If you are offering your help with testing
> >>> (i.e. you do have the access to i.MX23 device and you will test
> >>> those changes) I can add support for it.
> >>>
> >>> Otherwise, NO, I will not add ANY new untested code.    
> >>
> >> In general, you don't have to add any code, the iMX23/28 SPI IP is
> >> very much the same hardware, DTTO for most of the other blocks. If
> >> there are any differences between iMX23/28, they are already
> >> handled in the existing driver(s).
> >>
> >> Half-way converted drivers in fact increase maintenance burden,
> >> because then we have to deal with two different variants of the
> >> code, instead of only one.  
> > 
> > I cannot agree with this sentence.  
> 
> Do you think maintaining - one DM driver which supports both iMX23 and
> iMX28 - is more burden than maintaining - one driver which supports
> DM, but only for iMX28 and non-DM for iMX23 and iMX28 ? I don't think
> so.
> 
> > The conversion would be done for
> > i.MX28, which is then tested and validated (and clearly stated in
> > the cover letter/commit message that only supported was i.MX28).>
> > If I don't need to adjust common, reused code (which already
> > supports both variants as it is the case with mxs_spi.c), then I
> > don't mind.  
> 
> Well, that is what I said above, you don't.

To make myself clear - If I can reuse the common code (which supports
both imx23 and 28) for DM/DTS conversion then I'm OK with doing so.

If you require me to add untested code specific to i.MX23 - then NO.

> 
> > For more intrusive changes - the driver needs to be tested and
> > validated (by somebody who has the HW for testing).  
> 
> That's up to board maintainers.
> 
> >> That's why I would like to see this practice go
> >> away wherever possible, and in this case it is possible.  
> > 
> > In this particular case it is possible to add support for both as
> > SoC specific changes (i.MX23 vs i.MX28) is performed in common code
> > (e.g. mxs_spi_xfer_dma).  
> 
> Both SPI and DMA blocks are basically the same on iMX23 and iMX28.

If I can reuse the common code, then I'm fine to do it.

> 
> >> If you need someone to test your changes, CC the board maintainers,
> >> that's standard practice.  
> > 
> > As fair as I remember only Angelo and Michael had also interest in
> > testing converted code for i.MX28 based board.
> > 
> > There was NO reply from other people when this (and few others)
> > driver was marked as DEPRECATED.  
> 
> Well, too bad, clearly the interest in this platform is low.

This means that people are using either some old U-Boot version, or
there are a few people who want to refurbish the old HW with new code
(e.g. Michael, Angelo).

> That does not mean we should do sub-par upstream work, does it ?
> 

We shall not add untested code.



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Marek Vasut June 17, 2019, 1:46 p.m. UTC | #7
On 6/17/19 3:41 PM, Lukasz Majewski wrote:
> On Mon, 17 Jun 2019 15:23:55 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
>> On 6/17/19 2:27 PM, Lukasz Majewski wrote:
>>> Hi Marek,
>>>   
>>>> On 6/17/19 8:49 AM, Lukasz Majewski wrote:  
>>>>> Hi Marek,
>>>>>     
>>>>>> On 6/16/19 12:34 AM, Lukasz Majewski wrote:    
>>>>>>> This commit converts mxs_spi driver to support DM/DTS.
>>>>>>>
>>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>      
>>>>>>
>>>>>> Is the non-DM part needed for anything ?     
>>>>>
>>>>> Do you mean the non-DM part of the mxs_gpio driver? Yes, it is
>>>>> used by not converted boards.    
>>>>
>>>> This is a SPI driver though.
>>>>  
>>>>>> I recall the SPL jumps back
>>>>>> to BootROM when loading the U-Boot proper. So if not, drop it.
>>>>>>
>>>>>> Also, please don't do partial conversion for iMX28 only, do the
>>>>>> iMX23 part as well, it cannot be hard.    
>>>>>
>>>>> Maybe it is not hard, but I cannot test it properly as I don't
>>>>> have i.MX23 device. If you are offering your help with testing
>>>>> (i.e. you do have the access to i.MX23 device and you will test
>>>>> those changes) I can add support for it.
>>>>>
>>>>> Otherwise, NO, I will not add ANY new untested code.    
>>>>
>>>> In general, you don't have to add any code, the iMX23/28 SPI IP is
>>>> very much the same hardware, DTTO for most of the other blocks. If
>>>> there are any differences between iMX23/28, they are already
>>>> handled in the existing driver(s).
>>>>
>>>> Half-way converted drivers in fact increase maintenance burden,
>>>> because then we have to deal with two different variants of the
>>>> code, instead of only one.  
>>>
>>> I cannot agree with this sentence.  
>>
>> Do you think maintaining - one DM driver which supports both iMX23 and
>> iMX28 - is more burden than maintaining - one driver which supports
>> DM, but only for iMX28 and non-DM for iMX23 and iMX28 ? I don't think
>> so.
>>
>>> The conversion would be done for
>>> i.MX28, which is then tested and validated (and clearly stated in
>>> the cover letter/commit message that only supported was i.MX28).>
>>> If I don't need to adjust common, reused code (which already
>>> supports both variants as it is the case with mxs_spi.c), then I
>>> don't mind.  
>>
>> Well, that is what I said above, you don't.
> 
> To make myself clear - If I can reuse the common code (which supports
> both imx23 and 28) for DM/DTS conversion then I'm OK with doing so.
> 
> If you require me to add untested code specific to i.MX23 - then NO.

Yes, you can.

>>
>>> For more intrusive changes - the driver needs to be tested and
>>> validated (by somebody who has the HW for testing).  
>>
>> That's up to board maintainers.
>>
>>>> That's why I would like to see this practice go
>>>> away wherever possible, and in this case it is possible.  
>>>
>>> In this particular case it is possible to add support for both as
>>> SoC specific changes (i.MX23 vs i.MX28) is performed in common code
>>> (e.g. mxs_spi_xfer_dma).  
>>
>> Both SPI and DMA blocks are basically the same on iMX23 and iMX28.
> 
> If I can reuse the common code, then I'm fine to do it.
> 
>>
>>>> If you need someone to test your changes, CC the board maintainers,
>>>> that's standard practice.  
>>>
>>> As fair as I remember only Angelo and Michael had also interest in
>>> testing converted code for i.MX28 based board.
>>>
>>> There was NO reply from other people when this (and few others)
>>> driver was marked as DEPRECATED.  
>>
>> Well, too bad, clearly the interest in this platform is low.
> 
> This means that people are using either some old U-Boot version, or
> there are a few people who want to refurbish the old HW with new code
> (e.g. Michael, Angelo).

Yes

>> That does not mean we should do sub-par upstream work, does it ?
>>
> 
> We shall not add untested code.

You cannot test every single platform in existence. Post patches and let
board maintainers test them ; if they won't, too bad, their platform
might become broken. There's no other way to move forward without
dragging behind a tremendous amount of ancient baggage, and that in turn
is not viable.
Lukasz Majewski June 17, 2019, 2:57 p.m. UTC | #8
Hi Marek,

> On 6/17/19 3:41 PM, Lukasz Majewski wrote:
> > On Mon, 17 Jun 2019 15:23:55 +0200
> > Marek Vasut <marex@denx.de> wrote:
> >   
> >> On 6/17/19 2:27 PM, Lukasz Majewski wrote:  
> >>> Hi Marek,
> >>>     
> >>>> On 6/17/19 8:49 AM, Lukasz Majewski wrote:    
> >>>>> Hi Marek,
> >>>>>       
> >>>>>> On 6/16/19 12:34 AM, Lukasz Majewski wrote:      
> >>>>>>> This commit converts mxs_spi driver to support DM/DTS.
> >>>>>>>
> >>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>        
> >>>>>>
> >>>>>> Is the non-DM part needed for anything ?       
> >>>>>
> >>>>> Do you mean the non-DM part of the mxs_gpio driver? Yes, it is
> >>>>> used by not converted boards.      
> >>>>
> >>>> This is a SPI driver though.
> >>>>    
> >>>>>> I recall the SPL jumps back
> >>>>>> to BootROM when loading the U-Boot proper. So if not, drop it.
> >>>>>>
> >>>>>> Also, please don't do partial conversion for iMX28 only, do the
> >>>>>> iMX23 part as well, it cannot be hard.      
> >>>>>
> >>>>> Maybe it is not hard, but I cannot test it properly as I don't
> >>>>> have i.MX23 device. If you are offering your help with testing
> >>>>> (i.e. you do have the access to i.MX23 device and you will test
> >>>>> those changes) I can add support for it.
> >>>>>
> >>>>> Otherwise, NO, I will not add ANY new untested code.      
> >>>>
> >>>> In general, you don't have to add any code, the iMX23/28 SPI IP
> >>>> is very much the same hardware, DTTO for most of the other
> >>>> blocks. If there are any differences between iMX23/28, they are
> >>>> already handled in the existing driver(s).
> >>>>
> >>>> Half-way converted drivers in fact increase maintenance burden,
> >>>> because then we have to deal with two different variants of the
> >>>> code, instead of only one.    
> >>>
> >>> I cannot agree with this sentence.    
> >>
> >> Do you think maintaining - one DM driver which supports both iMX23
> >> and iMX28 - is more burden than maintaining - one driver which
> >> supports DM, but only for iMX28 and non-DM for iMX23 and iMX28 ? I
> >> don't think so.
> >>  
> >>> The conversion would be done for
> >>> i.MX28, which is then tested and validated (and clearly stated in
> >>> the cover letter/commit message that only supported was i.MX28).>
> >>> If I don't need to adjust common, reused code (which already
> >>> supports both variants as it is the case with mxs_spi.c), then I
> >>> don't mind.    
> >>
> >> Well, that is what I said above, you don't.  
> > 
> > To make myself clear - If I can reuse the common code (which
> > supports both imx23 and 28) for DM/DTS conversion then I'm OK with
> > doing so.
> > 
> > If you require me to add untested code specific to i.MX23 - then
> > NO.  
> 
> Yes, you can.

If possible, by reusing the old, common working code, I can add this to
the converted driver.

But I will not any new untested code.

> 
> >>  
> >>> For more intrusive changes - the driver needs to be tested and
> >>> validated (by somebody who has the HW for testing).    
> >>
> >> That's up to board maintainers.
> >>  
> >>>> That's why I would like to see this practice go
> >>>> away wherever possible, and in this case it is possible.    
> >>>
> >>> In this particular case it is possible to add support for both as
> >>> SoC specific changes (i.MX23 vs i.MX28) is performed in common
> >>> code (e.g. mxs_spi_xfer_dma).    
> >>
> >> Both SPI and DMA blocks are basically the same on iMX23 and
> >> iMX28.  
> > 
> > If I can reuse the common code, then I'm fine to do it.
> >   
> >>  
> >>>> If you need someone to test your changes, CC the board
> >>>> maintainers, that's standard practice.    
> >>>
> >>> As fair as I remember only Angelo and Michael had also interest in
> >>> testing converted code for i.MX28 based board.
> >>>
> >>> There was NO reply from other people when this (and few others)
> >>> driver was marked as DEPRECATED.    
> >>
> >> Well, too bad, clearly the interest in this platform is low.  
> > 
> > This means that people are using either some old U-Boot version, or
> > there are a few people who want to refurbish the old HW with new
> > code (e.g. Michael, Angelo).  
> 
> Yes
> 
> >> That does not mean we should do sub-par upstream work, does it ?
> >>  
> > 
> > We shall not add untested code.  
> 
> You cannot test every single platform in existence. Post patches and
> let board maintainers test them ; if they won't, too bad, their
> platform might become broken. There's no other way to move forward
> without dragging behind a tremendous amount of ancient baggage, and
> that in turn is not viable.
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Marek Vasut June 17, 2019, 3 p.m. UTC | #9
On 6/17/19 4:57 PM, Lukasz Majewski wrote:
> Hi Marek,
> 
>> On 6/17/19 3:41 PM, Lukasz Majewski wrote:
>>> On Mon, 17 Jun 2019 15:23:55 +0200
>>> Marek Vasut <marex@denx.de> wrote:
>>>   
>>>> On 6/17/19 2:27 PM, Lukasz Majewski wrote:  
>>>>> Hi Marek,
>>>>>     
>>>>>> On 6/17/19 8:49 AM, Lukasz Majewski wrote:    
>>>>>>> Hi Marek,
>>>>>>>       
>>>>>>>> On 6/16/19 12:34 AM, Lukasz Majewski wrote:      
>>>>>>>>> This commit converts mxs_spi driver to support DM/DTS.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>        
>>>>>>>>
>>>>>>>> Is the non-DM part needed for anything ?       
>>>>>>>
>>>>>>> Do you mean the non-DM part of the mxs_gpio driver? Yes, it is
>>>>>>> used by not converted boards.      
>>>>>>
>>>>>> This is a SPI driver though.
>>>>>>    
>>>>>>>> I recall the SPL jumps back
>>>>>>>> to BootROM when loading the U-Boot proper. So if not, drop it.
>>>>>>>>
>>>>>>>> Also, please don't do partial conversion for iMX28 only, do the
>>>>>>>> iMX23 part as well, it cannot be hard.      
>>>>>>>
>>>>>>> Maybe it is not hard, but I cannot test it properly as I don't
>>>>>>> have i.MX23 device. If you are offering your help with testing
>>>>>>> (i.e. you do have the access to i.MX23 device and you will test
>>>>>>> those changes) I can add support for it.
>>>>>>>
>>>>>>> Otherwise, NO, I will not add ANY new untested code.      
>>>>>>
>>>>>> In general, you don't have to add any code, the iMX23/28 SPI IP
>>>>>> is very much the same hardware, DTTO for most of the other
>>>>>> blocks. If there are any differences between iMX23/28, they are
>>>>>> already handled in the existing driver(s).
>>>>>>
>>>>>> Half-way converted drivers in fact increase maintenance burden,
>>>>>> because then we have to deal with two different variants of the
>>>>>> code, instead of only one.    
>>>>>
>>>>> I cannot agree with this sentence.    
>>>>
>>>> Do you think maintaining - one DM driver which supports both iMX23
>>>> and iMX28 - is more burden than maintaining - one driver which
>>>> supports DM, but only for iMX28 and non-DM for iMX23 and iMX28 ? I
>>>> don't think so.
>>>>  
>>>>> The conversion would be done for
>>>>> i.MX28, which is then tested and validated (and clearly stated in
>>>>> the cover letter/commit message that only supported was i.MX28).>
>>>>> If I don't need to adjust common, reused code (which already
>>>>> supports both variants as it is the case with mxs_spi.c), then I
>>>>> don't mind.    
>>>>
>>>> Well, that is what I said above, you don't.  
>>>
>>> To make myself clear - If I can reuse the common code (which
>>> supports both imx23 and 28) for DM/DTS conversion then I'm OK with
>>> doing so.
>>>
>>> If you require me to add untested code specific to i.MX23 - then
>>> NO.  
>>
>> Yes, you can.
> 
> If possible, by reusing the old, common working code, I can add this to
> the converted driver.

Again, yes, you can. All the code is already in the driver.

[...]
diff mbox series

Patch

diff --git a/drivers/spi/mxs_spi.c b/drivers/spi/mxs_spi.c
index 5065e407f8..6ae76309d7 100644
--- a/drivers/spi/mxs_spi.c
+++ b/drivers/spi/mxs_spi.c
@@ -2,6 +2,9 @@ 
 /*
  * Freescale i.MX28 SPI driver
  *
+ * Copyright (C) 2019 DENX Software Engineering
+ * Lukasz Majewski, DENX Software Engineering, lukma@denx.de
+ *
  * Copyright (C) 2011 Marek Vasut <marek.vasut@gmail.com>
  * on behalf of DENX Software Engineering GmbH
  *
@@ -27,6 +30,19 @@ 
 
 #define MXSSSP_SMALL_TRANSFER	512
 
+static void mxs_spi_start_xfer(struct mxs_ssp_regs *ssp_regs)
+{
+	writel(SSP_CTRL0_LOCK_CS, &ssp_regs->hw_ssp_ctrl0_set);
+	writel(SSP_CTRL0_IGNORE_CRC, &ssp_regs->hw_ssp_ctrl0_clr);
+}
+
+static void mxs_spi_end_xfer(struct mxs_ssp_regs *ssp_regs)
+{
+	writel(SSP_CTRL0_LOCK_CS, &ssp_regs->hw_ssp_ctrl0_clr);
+	writel(SSP_CTRL0_IGNORE_CRC, &ssp_regs->hw_ssp_ctrl0_set);
+}
+
+#if !CONFIG_IS_ENABLED(DM_SPI)
 struct mxs_spi_slave {
 	struct spi_slave	slave;
 	uint32_t		max_khz;
@@ -38,94 +54,38 @@  static inline struct mxs_spi_slave *to_mxs_slave(struct spi_slave *slave)
 {
 	return container_of(slave, struct mxs_spi_slave, slave);
 }
+#else
+#include <dm.h>
+#include <errno.h>
+struct mxs_spi_platdata {
+	s32 frequency;		/* Default clock frequency, -1 for none */
+	fdt_addr_t base;        /* SPI IP block base address */
+	int num_cs;             /* Number of CSes supported */
+	int dma_id;             /* ID of the DMA channel */
+	int clk_id;             /* ID of the SSP clock */
+};
 
-int spi_cs_is_valid(unsigned int bus, unsigned int cs)
-{
-	/* MXS SPI: 4 ports and 3 chip selects maximum */
-	if (!mxs_ssp_bus_id_valid(bus) || cs > 2)
-		return 0;
-	else
-		return 1;
-}
-
-struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
-				  unsigned int max_hz, unsigned int mode)
-{
-	struct mxs_spi_slave *mxs_slave;
-
-	if (!spi_cs_is_valid(bus, cs)) {
-		printf("mxs_spi: invalid bus %d / chip select %d\n", bus, cs);
-		return NULL;
-	}
-
-	mxs_slave = spi_alloc_slave(struct mxs_spi_slave, bus, cs);
-	if (!mxs_slave)
-		return NULL;
-
-	if (mxs_dma_init_channel(MXS_DMA_CHANNEL_AHB_APBH_SSP0 + bus))
-		goto err_init;
-
-	mxs_slave->max_khz = max_hz / 1000;
-	mxs_slave->mode = mode;
-	mxs_slave->regs = mxs_ssp_regs_by_bus(bus);
-
-	return &mxs_slave->slave;
-
-err_init:
-	free(mxs_slave);
-	return NULL;
-}
-
-void spi_free_slave(struct spi_slave *slave)
-{
-	struct mxs_spi_slave *mxs_slave = to_mxs_slave(slave);
-	free(mxs_slave);
-}
-
-int spi_claim_bus(struct spi_slave *slave)
-{
-	struct mxs_spi_slave *mxs_slave = to_mxs_slave(slave);
-	struct mxs_ssp_regs *ssp_regs = mxs_slave->regs;
-	uint32_t reg = 0;
-
-	mxs_reset_block(&ssp_regs->hw_ssp_ctrl0_reg);
-
-	writel((slave->cs << MXS_SSP_CHIPSELECT_SHIFT) |
-	       SSP_CTRL0_BUS_WIDTH_ONE_BIT,
-	       &ssp_regs->hw_ssp_ctrl0);
-
-	reg = SSP_CTRL1_SSP_MODE_SPI | SSP_CTRL1_WORD_LENGTH_EIGHT_BITS;
-	reg |= (mxs_slave->mode & SPI_CPOL) ? SSP_CTRL1_POLARITY : 0;
-	reg |= (mxs_slave->mode & SPI_CPHA) ? SSP_CTRL1_PHASE : 0;
-	writel(reg, &ssp_regs->hw_ssp_ctrl1);
-
-	writel(0, &ssp_regs->hw_ssp_cmd0);
-
-	mxs_set_ssp_busclock(slave->bus, mxs_slave->max_khz);
-
-	return 0;
-}
-
-void spi_release_bus(struct spi_slave *slave)
-{
-}
-
-static void mxs_spi_start_xfer(struct mxs_ssp_regs *ssp_regs)
-{
-	writel(SSP_CTRL0_LOCK_CS, &ssp_regs->hw_ssp_ctrl0_set);
-	writel(SSP_CTRL0_IGNORE_CRC, &ssp_regs->hw_ssp_ctrl0_clr);
-}
-
-static void mxs_spi_end_xfer(struct mxs_ssp_regs *ssp_regs)
-{
-	writel(SSP_CTRL0_LOCK_CS, &ssp_regs->hw_ssp_ctrl0_clr);
-	writel(SSP_CTRL0_IGNORE_CRC, &ssp_regs->hw_ssp_ctrl0_set);
-}
+struct mxs_spi_priv {
+	struct mxs_ssp_regs *regs;
+	unsigned int dma_channel;
+	unsigned int max_freq;
+	unsigned int clk_id;
+	unsigned int mode;
+};
+#endif
 
+#if !CONFIG_IS_ENABLED(DM_SPI)
 static int mxs_spi_xfer_pio(struct mxs_spi_slave *slave,
 			char *data, int length, int write, unsigned long flags)
 {
 	struct mxs_ssp_regs *ssp_regs = slave->regs;
+#else
+static int mxs_spi_xfer_pio(struct mxs_spi_priv *priv,
+			    char *data, int length, int write,
+			    unsigned long flags)
+{
+	struct mxs_ssp_regs *ssp_regs = priv->regs;
+#endif
 
 	if (flags & SPI_XFER_BEGIN)
 		mxs_spi_start_xfer(ssp_regs);
@@ -181,12 +141,18 @@  static int mxs_spi_xfer_pio(struct mxs_spi_slave *slave,
 	return 0;
 }
 
+#if !CONFIG_IS_ENABLED(DM_SPI)
 static int mxs_spi_xfer_dma(struct mxs_spi_slave *slave,
 			char *data, int length, int write, unsigned long flags)
 {
+	struct mxs_ssp_regs *ssp_regs = slave->regs;
+#else
+static int mxs_spi_xfer_dma(struct mxs_spi_priv *priv,
+			char *data, int length, int write, unsigned long flags)
+{	struct mxs_ssp_regs *ssp_regs = priv->regs;
+#endif
 	const int xfer_max_sz = 0xff00;
 	const int desc_count = DIV_ROUND_UP(length, xfer_max_sz) + 1;
-	struct mxs_ssp_regs *ssp_regs = slave->regs;
 	struct mxs_dma_desc *dp;
 	uint32_t ctrl0;
 	uint32_t cache_data_count;
@@ -225,7 +191,11 @@  static int mxs_spi_xfer_dma(struct mxs_spi_slave *slave,
 	/* Invalidate the area, so no writeback into the RAM races with DMA */
 	invalidate_dcache_range(dstart, dstart + cache_data_count);
 
+#if !CONFIG_IS_ENABLED(DM_SPI)
 	dmach = MXS_DMA_CHANNEL_AHB_APBH_SSP0 + slave->slave.bus;
+#else
+	dmach = priv->dma_channel;
+#endif
 
 	dp = desc;
 	while (length) {
@@ -302,11 +272,20 @@  static int mxs_spi_xfer_dma(struct mxs_spi_slave *slave,
 	return ret;
 }
 
+#if !CONFIG_IS_ENABLED(DM_SPI)
 int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
 		const void *dout, void *din, unsigned long flags)
 {
 	struct mxs_spi_slave *mxs_slave = to_mxs_slave(slave);
 	struct mxs_ssp_regs *ssp_regs = mxs_slave->regs;
+#else
+int mxs_spi_xfer(struct udevice *dev, unsigned int bitlen,
+		const void *dout, void *din, unsigned long flags)
+{
+	struct udevice *bus = dev_get_parent(dev);
+	struct mxs_spi_priv *priv = dev_get_priv(bus);
+	struct mxs_ssp_regs *ssp_regs = priv->regs;
+#endif
 	int len = bitlen / 8;
 	char dummy;
 	int write = 0;
@@ -350,9 +329,257 @@  int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
 
 	if (!dma || (len < MXSSSP_SMALL_TRANSFER)) {
 		writel(SSP_CTRL1_DMA_ENABLE, &ssp_regs->hw_ssp_ctrl1_clr);
+#if !CONFIG_IS_ENABLED(DM_SPI)
 		return mxs_spi_xfer_pio(mxs_slave, data, len, write, flags);
+#else
+		return mxs_spi_xfer_pio(priv, data, len, write, flags);
+#endif
 	} else {
 		writel(SSP_CTRL1_DMA_ENABLE, &ssp_regs->hw_ssp_ctrl1_set);
+#if !CONFIG_IS_ENABLED(DM_SPI)
 		return mxs_spi_xfer_dma(mxs_slave, data, len, write, flags);
+#else
+		return mxs_spi_xfer_dma(priv, data, len, write, flags);
+#endif
 	}
 }
+
+#if !CONFIG_IS_ENABLED(DM_SPI)
+int spi_cs_is_valid(unsigned int bus, unsigned int cs)
+{
+	/* MXS SPI: 4 ports and 3 chip selects maximum */
+	if (!mxs_ssp_bus_id_valid(bus) || cs > 2)
+		return 0;
+	else
+		return 1;
+}
+
+struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
+				  unsigned int max_hz, unsigned int mode)
+{
+	struct mxs_spi_slave *mxs_slave;
+
+	if (!spi_cs_is_valid(bus, cs)) {
+		printf("mxs_spi: invalid bus %d / chip select %d\n", bus, cs);
+		return NULL;
+	}
+
+	mxs_slave = spi_alloc_slave(struct mxs_spi_slave, bus, cs);
+	if (!mxs_slave)
+		return NULL;
+
+	if (mxs_dma_init_channel(MXS_DMA_CHANNEL_AHB_APBH_SSP0 + bus))
+		goto err_init;
+
+	mxs_slave->max_khz = max_hz / 1000;
+	mxs_slave->mode = mode;
+	mxs_slave->regs = mxs_ssp_regs_by_bus(bus);
+
+	return &mxs_slave->slave;
+
+err_init:
+	free(mxs_slave);
+	return NULL;
+}
+
+void spi_free_slave(struct spi_slave *slave)
+{
+	struct mxs_spi_slave *mxs_slave = to_mxs_slave(slave);
+	free(mxs_slave);
+}
+
+int spi_claim_bus(struct spi_slave *slave)
+{
+	struct mxs_spi_slave *mxs_slave = to_mxs_slave(slave);
+	struct mxs_ssp_regs *ssp_regs = mxs_slave->regs;
+	uint32_t reg = 0;
+
+	mxs_reset_block(&ssp_regs->hw_ssp_ctrl0_reg);
+
+	writel((slave->cs << MXS_SSP_CHIPSELECT_SHIFT) |
+	       SSP_CTRL0_BUS_WIDTH_ONE_BIT,
+	       &ssp_regs->hw_ssp_ctrl0);
+
+	reg = SSP_CTRL1_SSP_MODE_SPI | SSP_CTRL1_WORD_LENGTH_EIGHT_BITS;
+	reg |= (mxs_slave->mode & SPI_CPOL) ? SSP_CTRL1_POLARITY : 0;
+	reg |= (mxs_slave->mode & SPI_CPHA) ? SSP_CTRL1_PHASE : 0;
+	writel(reg, &ssp_regs->hw_ssp_ctrl1);
+
+	writel(0, &ssp_regs->hw_ssp_cmd0);
+
+	mxs_set_ssp_busclock(slave->bus, mxs_slave->max_khz);
+
+	return 0;
+}
+
+void spi_release_bus(struct spi_slave *slave)
+{
+}
+
+#else /* CONFIG_DM_SPI */
+/* Base number of i.MX28 clk for ssp0 IP block */
+#define MXS_SSP_IMX28_CLKID_SSP0 46
+
+static int mxs_spi_probe(struct udevice *bus)
+{
+	struct mxs_spi_platdata *plat = dev_get_platdata(bus);
+	struct mxs_spi_priv *priv = dev_get_priv(bus);
+	int ret;
+
+	debug("%s: probe\n", __func__);
+	priv->regs = (struct mxs_ssp_regs *)plat->base;
+	priv->max_freq = plat->frequency;
+
+	priv->dma_channel = plat->dma_id;
+	priv->clk_id = plat->clk_id;
+
+	ret = mxs_dma_init_channel(priv->dma_channel);
+	if (ret) {
+		printf("%s: DMA init channel error %d\n", __func__, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int mxs_spi_claim_bus(struct udevice *dev)
+{
+	struct udevice *bus = dev_get_parent(dev);
+	struct mxs_spi_priv *priv = dev_get_priv(bus);
+	struct mxs_ssp_regs *ssp_regs = priv->regs;
+	int cs = spi_chip_select(dev);
+
+	/*
+	 * i.MX28 supports up to 3 CS (SSn0, SSn1, SSn2)
+	 * To set them it uses following tuple (WAIT_FOR_IRQ,WAIT_FOR_CMD),
+	 * where:
+	 *
+	 * WAIT_FOR_IRQ is bit 21 of HW_SSP_CTRL0
+	 * WAIT_FOR_CMD is bit 20 (#defined as MXS_SSP_CHIPSELECT_SHIFT here) of
+	 *                        HW_SSP_CTRL0
+	 * SSn0 b00
+	 * SSn1 b01
+	 * SSn2 b10 (which require setting WAIT_FOR_IRQ)
+	 *
+	 * However, for now i.MX28 SPI driver will support up till 2 CSes
+	 * (SSn0, and SSn1).
+	 */
+
+	/* Ungate SSP clock and set active CS */
+	clrsetbits_le32(&ssp_regs->hw_ssp_ctrl0,
+			(1 << MXS_SSP_CHIPSELECT_SHIFT) |
+			SSP_CTRL0_CLKGATE, (cs << MXS_SSP_CHIPSELECT_SHIFT));
+
+	return 0;
+}
+
+static int mxs_spi_release_bus(struct udevice *dev)
+{
+	struct udevice *bus = dev_get_parent(dev);
+	struct mxs_spi_priv *priv = dev_get_priv(bus);
+	struct mxs_ssp_regs *ssp_regs = priv->regs;
+
+	/* Gate SSP clock */
+	setbits_le32(&ssp_regs->hw_ssp_ctrl0, SSP_CTRL0_CLKGATE);
+
+	return 0;
+}
+
+static int mxs_spi_set_speed(struct udevice *bus, uint speed)
+{
+	struct mxs_spi_priv *priv = dev_get_priv(bus);
+	int clkid = priv->clk_id - MXS_SSP_IMX28_CLKID_SSP0;
+
+	if (speed > priv->max_freq)
+		speed = priv->max_freq;
+
+	debug("%s speed: %u [Hz] clkid: %d\n", __func__, speed, clkid);
+	mxs_set_ssp_busclock(clkid, speed / 1000);
+
+	return 0;
+}
+
+static int mxs_spi_set_mode(struct udevice *bus, uint mode)
+{
+	struct mxs_spi_priv *priv = dev_get_priv(bus);
+	struct mxs_ssp_regs *ssp_regs = priv->regs;
+	u32 reg;
+
+	priv->mode = mode;
+	debug("%s: mode 0x%x\n", __func__, mode);
+
+	reg = SSP_CTRL1_SSP_MODE_SPI | SSP_CTRL1_WORD_LENGTH_EIGHT_BITS;
+	reg |= (priv->mode & SPI_CPOL) ? SSP_CTRL1_POLARITY : 0;
+	reg |= (priv->mode & SPI_CPHA) ? SSP_CTRL1_PHASE : 0;
+	writel(reg, &ssp_regs->hw_ssp_ctrl1);
+
+	/* Single bit SPI support */
+	writel(SSP_CTRL0_BUS_WIDTH_ONE_BIT, &ssp_regs->hw_ssp_ctrl0);
+
+	return 0;
+}
+
+static const struct dm_spi_ops mxs_spi_ops = {
+	.claim_bus	= mxs_spi_claim_bus,
+	.release_bus    = mxs_spi_release_bus,
+	.xfer		= mxs_spi_xfer,
+	.set_speed	= mxs_spi_set_speed,
+	.set_mode	= mxs_spi_set_mode,
+	/*
+	 * cs_info is not needed, since we require all chip selects to be
+	 * in the device tree explicitly
+	 */
+};
+
+#if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)
+static int mxs_ofdata_to_platdata(struct udevice *bus)
+{
+	struct mxs_spi_platdata *plat = bus->platdata;
+	u32 prop[2];
+	int ret;
+
+	plat->base = dev_read_addr(bus);
+	plat->frequency =
+		dev_read_u32_default(bus, "spi-max-frequency", 40000000);
+	plat->num_cs = dev_read_u32_default(bus, "num-cs", 2);
+
+	ret = dev_read_u32_array(bus, "dmas", prop, ARRAY_SIZE(prop));
+	if (ret) {
+		printf("%s: Reading 'dmas' property failed!\n", __func__);
+		return ret;
+	}
+	plat->dma_id = prop[1];
+
+	ret = dev_read_u32_array(bus, "clocks", prop, ARRAY_SIZE(prop));
+	if (ret) {
+		printf("%s: Reading 'clocks' property failed!\n", __func__);
+		return ret;
+	}
+	plat->clk_id = prop[1];
+
+	debug("%s: base=0x%x, max-frequency=%d num-cs=%d dma_id=%d clk_id=%d\n",
+	      __func__, (uint)plat->base, plat->frequency, plat->num_cs,
+	      plat->dma_id, plat->clk_id);
+
+	return 0;
+}
+#endif
+
+static const struct udevice_id mxs_spi_ids[] = {
+	{ .compatible = "fsl,imx28-spi" },
+	{ }
+};
+
+U_BOOT_DRIVER(mxs_spi) = {
+	.name	= "mxs_spi",
+	.id	= UCLASS_SPI,
+#if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)
+	.of_match = mxs_spi_ids,
+	.ofdata_to_platdata = mxs_ofdata_to_platdata,
+#endif
+	.priv_auto_alloc_size = sizeof(struct mxs_spi_platdata),
+	.ops	= &mxs_spi_ops,
+	.priv_auto_alloc_size = sizeof(struct mxs_spi_priv),
+	.probe	= mxs_spi_probe,
+};
+#endif